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 <sb...@kernel.org> wrote:
>>
>> Quoting Saravana Kannan (2021-01-28 09:01:41)
>>> On Thu, Jan 28, 2021 at 2:45 AM Tudor Ambarus
>>> <tudor.amba...@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.amba...@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-sarava...@google.com/
[2]
https://lore.kernel.org/lkml/20210203154332.470587-1-tudor.amba...@microchip.com/
Cheers,
ta
