Re: [PATCH] clk: at91: sama5d2: Mark device OF_POPULATED after setup

[<Tudor.Ambarus@microchip.com>]

Hi, Saravana,

On 2/9/21 9:06 PM, Saravana Kannan wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Feb 9, 2021 at 7:21 AM <Tudor.Ambarus@microchip.com> wrote:
>>
>> Hi, Saravana,
>>
>> On 2/9/21 11:11 AM, Saravana Kannan wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, Feb 8, 2021 at 11:55 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>
>>>> Quoting Saravana Kannan (2021-01-28 09:01:41)
>>>>> On Thu, Jan 28, 2021 at 2:45 AM Tudor Ambarus
>>>>> <tudor.ambarus@microchip.com> wrote:
>>>>>>
>>>>>> The sama5d2 requires the clock provider initialized before timers.
>>>>>> We can't use a platform driver for the sama5d2-pmc driver, as the
>>>>>> platform_bus_init() is called later on, after time_init().
>>>>>>
>>>>>> As fw_devlink considers only devices, it does not know that the
>>>>>> pmc is ready. Hence probing of devices that depend on it fail:
>>>>>> probe deferral - supplier f0014000.pmc not ready
>>>>>>
>>>>>> Fix this by setting the OF_POPULATED flag for the sama5d2_pmc
>>>>>> device node after successful setup. This will make
>>>>>> of_link_to_phandle() ignore the sama5d2_pmc device node as a
>>>>>> dependency, and consumer devices will be probed again.
>>>>>>
>>>>>> Fixes: e590474768f1cc04 ("driver core: Set fw_devlink=on by default")
>>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>>>> ---
>>>>>> I'll be out of office, will check the rest of the at91 SoCs
>>>>>> at the begining of next week.
>>>>>>
>>>>>>  drivers/clk/at91/sama5d2.c | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/clk/at91/sama5d2.c b/drivers/clk/at91/sama5d2.c
>>>>>> index 9a5cbc7cd55a..5eea2b4a63dd 100644
>>>>>> --- a/drivers/clk/at91/sama5d2.c
>>>>>> +++ b/drivers/clk/at91/sama5d2.c
>>>>>> @@ -367,6 +367,8 @@ static void __init sama5d2_pmc_setup(struct device_node *np)
>>>>>>
>>>>>>         of_clk_add_hw_provider(np, of_clk_hw_pmc_get, sama5d2_pmc);
>>>>>>
>>>>>> +       of_node_set_flag(np, OF_POPULATED);
>>>>>> +
>>>>>>         return;
>>>>>
>>>>> Hi Tudor,
>>>>>
>>>>> Thanks for looking into this.
>>>>>
>>>>> I already accounted for early clocks like this when I designed
>>>>> fw_devlink. Each driver shouldn't need to set OF_POPULATED.
>>>>> drivers/clk/clk.c already does this for you.
>>>>>
>>>>> I think the problem is that your driver is using
>>>>> CLK_OF_DECLARE_DRIVER() instead of CLK_OF_DECLARE(). The comments for
>>>>> CLK_OF_DECLARE_DRIVER() says:
>>>>> /*
>>>>>  * Use this macro when you have a driver that requires two initialization
>>>>>  * routines, one at of_clk_init(), and one at platform device probe
>>>>>  */
>>>>>
>>>>> In your case, you are explicitly NOT having a driver bind to this
>>>>> clock later. So you shouldn't be using CLK_OF_DECLARE() instead.
>>>>>
>>>>
>>>> I see
>>>>
>>>> drivers/power/reset/at91-sama5d2_shdwc.c:       { .compatible = "atmel,sama5d2-pmc" },
>>>>
>>>> so isn't that the driver that wants to bind to the same device node
>>>> again? First at of_clk_init() time here and then second for the reset
>>>> driver?
>>>
>>> You are right. I assumed that when Tudor was setting OF_POPULATED,
>>
>> No, there's a single driver that binds to that compatible.
>>
>>> they didn't want to create a struct device and they knew it was right
>>> for their platform.
>>>
>>> However...
>>> $ git grep "atmel,sama5d2-pmc"
>>> arch/arm/boot/dts/sama5d2.dtsi:                         compatible =
>>> "atmel,sama5d2-pmc", "syscon";
>>> arch/arm/mach-at91/pm.c:        { .compatible = "atmel,sama5d2-pmc",
>>> .data = &pmc_infos[1] },
>>> drivers/clk/at91/pmc.c: { .compatible = "atmel,sama5d2-pmc" },
>>> drivers/clk/at91/sama5d2.c:CLK_OF_DECLARE_DRIVER(sama5d2_pmc,
>>> "atmel,sama5d2-pmc", sama5d2_pmc_setup);
>>> drivers/power/reset/at91-sama5d2_shdwc.c:       { .compatible =
>>> "atmel,sama5d2-pmc" },
>>>
>>> Geez! How many drivers are there for this one device. Clearly not all
>>> of them are going to bind. But I'm not going to dig into this. You can
>>
>> From this entire list only the drivers/clk/at91/sama5d2.c driver binds to the
>> "atmel,sama5d2-pmc" compatible, the rest are just using the compatible to
>> map the PMC memory.
>>
>>> reject this patch. I expect this series [1] to take care of the issue
>>> Tudor was trying to fix.
>>>
>>> Tudor,
>>>
>>> Want to give this series [1] a shot?
>>
>> The series at [1] doesn't apply clean neither on next-20210209, nor on
>> driver-core-next. On top of which sha1 should I apply them?
> 
> It's on top of driver-core-next:
> 4731210c09f5 gpiolib: Bind gpio_device to a driver to enable
> fw_devlink=on by default

I see Greg took your series. I tried the driver-core-next (with your series
included), it doesn't solve my boot problem on sama5d2_xplained.

With [2] applied, sama5d2_xplained can boot again.

Cheers,
ta

> 
>> Anyway, I think the patch at [2] is still needed, regardless of the outcome
>> of [1].
> 
> Right, [2] is still a good clean up based on your comment above.
> 
> -Saravana
> 
>>>
>>> [1] - https://lore.kernel.org/lkml/20210205222644.2357303-1-saravanak@google.com/
>>
>> [2] https://lore.kernel.org/lkml/20210203154332.470587-1-tudor.ambarus@microchip.com/
>>
>> Cheers,
>> ta
>>