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

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

[Tudor Ambarus]

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;
 
 err_free:
-- 
2.25.1

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

[Saravana Kannan]

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.

Thanks,
Saravana

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

[Geert Uytterhoeven]

Hi Saravana,

On Thu, Jan 28, 2021 at 6:08 PM Saravana Kannan <saravanak@google.com> wrote:
> 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.

Unless I'm missing something, name##_of_clk_init_driver() clearing
OF_POPULATED again causes consumer driver probing to be postponed by
fw_devlink until the second initialization phase of the provider has been
completed?

This is wrong if the consumer only needs a clock instantiated during the
first phase, and may cause issues if the consumer is a critical device.
E.g. a timer, on ARM SoCs lacking an architecture timer (pre-Cortex
A7/A15) or global timer (pre-Cortex A9, or single-core Cortex A9).
Probably there are more examples.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Saravana Kannan]

On Mon, Feb 1, 2021 at 2:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Thu, Jan 28, 2021 at 6:08 PM Saravana Kannan <saravanak@google.com> wrote:
> > 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.
>

Typo. I meant to say this driver SHOULD be using CLK_OF_DECLARE()
instead. I wonder if this is what caused you to send the email --
because we are saying the same thing.

> Unless I'm missing something, name##_of_clk_init_driver() clearing
> OF_POPULATED again causes consumer driver probing to be postponed by
> fw_devlink until the second initialization phase of the provider has been
> completed?

Right, if they use CLK_OF_DECLARE_DRIVER() what you said above will
happen and that's the issue they are trying to fix.

> This is wrong if the consumer only needs a clock instantiated during the
> first phase, and may cause issues if the consumer is a critical device.
> E.g. a timer, on ARM SoCs lacking an architecture timer (pre-Cortex
> A7/A15) or global timer (pre-Cortex A9, or single-core Cortex A9).
> Probably there are more examples.

So, needing devices like IRQ, timer and clocks early is fine.
fw_devlink can handle that correctly if the proper macros are used
(since most frameworks set the OF_POPULATED flag for these devices).

-Saravana

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

[Stephen Boyd]

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?

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

[Saravana Kannan]

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,
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
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?

[1] - https://lore.kernel.org/lkml/20210205222644.2357303-1-saravanak@google.com/
-Saravana

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

[Tudor.Ambarus]

Hi, Stephen,

On 2/9/21 9:55 AM, Stephen Boyd wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?

No, it isn't. at91_shdwc_driver binds the compatibles from at91_shdwc_of_match:

static const struct of_device_id at91_shdwc_of_match[] = {                      
        {                                                                       
                .compatible = "atmel,sama5d2-shdwc",                            
                .data = &sama5d2_reg_config,                                    
        },                                                                      
        {                                                                       
                .compatible = "microchip,sam9x60-shdwc",                        
                .data = &sam9x60_reg_config,                                    
        }, {                                                                    
                /*sentinel*/                                                    
        }                                                                       
};                                                                              
MODULE_DEVICE_TABLE(of, at91_shdwc_of_match);   

 
The pmc compatibles are later on used in the probe function in order to
get with of_iomap() the pmc_base, that is later used in at91_poweroff()
method.

Just for the reference, this patch is superseded by the following:
https://lore.kernel.org/lkml/20210203154332.470587-1-tudor.ambarus@microchip.com/

Cheers,
ta

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

[Tudor.Ambarus]

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

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

[Saravana Kannan]

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

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

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

[Tudor.Ambarus]

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