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

[<Tudor.Ambarus@microchip.com>]

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