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

[<Tudor.Ambarus@microchip.com>]

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?

Anyway, I think the patch at [2] is still needed, regardless of the outcome
of [1].

> 
> [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