Re: [PATCH v5 12/44] clk: davinci: Add platform information for TI DA850 PSC

[Sekhar Nori]

Hi Bart,

On Thursday 05 April 2018 09:21 PM, Bartosz Golaszewski wrote:
> 2018-04-05 16:36 GMT+02:00 Sekhar Nori <nsekhar@xxxxxx>:
>> On Thursday 05 April 2018 07:14 PM, Bartosz Golaszewski wrote:
>>> 2018-04-05 15:09 GMT+02:00 Sekhar Nori <nsekhar@xxxxxx>:
>>>> Hi Bartosz,
>>>>
>>>> On Friday 09 February 2018 10:18 PM, Michael Turquette wrote:
>>>>> On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>>>>>> 2018-01-08 3:17 GMT+01:00 David Lechner <david@xxxxxxxxxxxxxx>:
>>>>
>>>>>> Hi David,
>>>>>>
>>>>>> I've been working on moving the genpd code from its own driver to the
>>>>>> psc one. I couldn't get the system to boot though and problems
>>>>>> happened very early in the boot sequence. I struggled to figure out
>>>>>> what's happening, but eventually I noticed that psc uses
>>>>>> CLK_OF_DECLARE() to initialize clocks. The functions registered this
>>>>>> way are called very early in the boot sequence, way before
>>>>>> late_initcall() in which the genpd framework is initialized. This of
>>>>
>>>> late_initcall() is too late for genpd to be initialized. As you may have
>>>> seen with the latest set of patches, we have problems with timer
>>>> initialization. After converting to platform devices, PSC and PLL clocks
>>>> get initialized post time_init(). We are working that around using
>>>> fixed-clocks, which hopefully will work (I still need to test many of
>>>> the affected platforms).
>>>>
>>>> Can you please reply with the exact issue you faced with genpd framework
>>>> initialization so we do have that on record.
>>>>
>>>
>>> The exact issue manifested itself in a NULL-pointer dereference panic
>>> when I tried moving the genpd code I had initially implemented as a
>>> separate platform driver to what I believe was v6 or v7 of David's
>>> series (before the psc driver became a platform driver, when it was
>>> still using CLK_OF_DECLARE()). When I had tested a simple conversion
>>> of that version to a platform_driver, genpd worked fine.
>>>
>>> I don't have the stack traces from these panics, but I recall some
>>> debugfs functions being involved and the genpd late_initcalls are
>>> related to debugfs. Looking at it now I don't see how exactly it could
>>> fail though.
>>
>> Do you have the code where you faced the problem stashed somewhere? I am
>> not (yet) advocating going back to CLK_OF_DECLARE(). But there is a
>> definite issue with timer being ready when not using CLK_OF_DECLARE().
>> So, I want to make sure there the reason why we are going down the
>> platform device path is a amply clear.
>>
>> Thanks,
>> Sekhar
> 
> Yes, you can still find it on my github[1].
> 
> Bart
> 
> [1] github.com:brgl/linux.git  topic/davinci-genpd-final-v2

The panic issue in your branch is not related to genpd. Its because you 
are accessing platform bus before it is initialized. The attached[1] 
patch on top of your branch made it boot again.

With your branch booting, I can see genpd related debugfs entries 
getting created. I don't see devices being attached to the domains you 
have though. I did not debug that. I suspect some matching issue.

Can you please check that and confirm there is no issue with genpd and 
using CLK_OF_DECLARE() to initialize clocks?

Unless you report an issue back, or Mike and Stephen have ideas about 
how to handle the dependency between PSC/PLL derived timer clock 
initialization and and timer_probe(), I think we need to move back to 
using CLK_OF_DECLARE(). 

Thanks,
Sekhar

---8<---
diff --git a/drivers/clk/davinci/psc.c b/drivers/clk/davinci/psc.c
index 9d9f94eee544..7e3d114efdac 100644
--- a/drivers/clk/davinci/psc.c
+++ b/drivers/clk/davinci/psc.c
@@ -281,12 +281,18 @@ int __davinci_psc_register_clocks(const struct davinci_psc_clk_info *info,
 		}
 	}
 
-	DO_ONCE(pm_clk_add_notifier,
-		&platform_bus_type, &platform_bus_notifier);
-
 	return 0;
 }
 
+static int __init davinci_pm_runtime_init(void)
+{
+       pm_clk_add_notifier(&platform_bus_type, &platform_bus_notifier);
+
+       return 0;
+}
+core_initcall(davinci_pm_runtime_init);
+
 int davinci_psc_register_clocks(const struct davinci_psc_clk_info *info,
 				void __iomem *base)
 {