Re: [PATCH] rtc: mpfs: Use devm_clk_get_enabled() helper

[Conor.Dooley]

On 24/08/2022 11:53, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 24/08/2022 09:58:35+0000, Conor.Dooley@xxxxxxxxxxxxx wrote:
>> Hey Christope,
>> Thanks for your patch.
>>
>> On 24/08/2022 09:18, Christophe JAILLET wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> The devm_clk_get_enabled() helper:
>>>      - calls devm_clk_get()
>>>      - calls clk_prepare_enable() and registers what is needed in order to
>>>        call clk_disable_unprepare() when needed, as a managed resource.
>>>
>>> This simplifies the code, the error handling paths and avoid the need of
>>> a dedicated function used with devm_add_action_or_reset().
>>>
>>> That said, mpfs_rtc_init_clk() is the same as devm_clk_get_enabled(), so
>>> use this function directly instead.
>>
>> Firstly, I think something is missing from the commit description here.
>> devm_clk_get_enabled() is not just a blanket "use this instead of get(),
>> prepare_enable()" & is only intended for cases where the clock would
>> be kept prepared or enabled for the whole lifetime of the driver. I think
>> it is worth pointing that out to help people who do not keep up with
>> every helper that is added.
>>
>> I had a bit of a look through the documentation to see if the block would
>> keep track of time without the AHB clock enabled, but it does not seem to.
>> There is no reason to turn off the clock here (in fact it would seem
>> counter productive to disable it..) so it looks like the shoe fits in that
>> regard.
> 
> This would be very surprising that it doesn't keep the time with the AHB
> clock disabled, this would mean the RTC loses the time when the SoC is
> not powered. or is the AHB clock also in the always-on domain?

If the SoC is completely unpowered, the reference clock will be gone too
- not just the AHB clock. In low power states, or with the cpus disabled
etc, the rtc should keep counting. Is there something I have missed &
should have either documented, explained or set when first submitting the
driver? I remember reading in the docs about the rtc class framework
supporting systems where the battery backed rtc was external & the SoC
had an RTC with potentially more features etc. Maybe I misunderstood?

This is a "hardened" version of an FPGA core & AFAIK the clock that
clocks the AHB interface/wrapper also clocks the logic in the RTC. The
docs are very scant, so I will try to get my hands on the RTL. The
clocks themselves should be always-on..

I did some brief testing just now, and if I disable the AHB interface
time keeping is lost.

I am inclined to say that this patch is valid & the underlying clock
needs to be marked as critical - unless I have missed something..

Applying this patch seems like it will have no functional change since
the clock is already being disabled in the remove callback so:

Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>

If my conclusions here are correct, I'll submit a patch for the clock
driver marking the rtc's AHB interface clock as critical.

> 
>>
>> However...
>>
>>>
>>> This also fixes an (unlikely) unchecked devm_add_action_or_reset() error.
>>>
>>> Based on my test with allyesconfig, this reduces the .o size from:
>>>      text    data     bss     dec     hex filename
>>>      5330    2208       0    7538    1d72 drivers/rtc/rtc-mpfs.o
>>> down to:
>>>      5074    2208       0    7282    1c72 drivers/rtc/rtc-mpfs.o
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
>>> ---
>>> devm_clk_get_enabled() is new and is part of 6.0-rc1
>>> ---
>>>    drivers/rtc/rtc-mpfs.c | 19 +------------------
>>>    1 file changed, 1 insertion(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-mpfs.c b/drivers/rtc/rtc-mpfs.c
>>> index 944ad1036516..2a479d44f198 100644
>>> --- a/drivers/rtc/rtc-mpfs.c
>>> +++ b/drivers/rtc/rtc-mpfs.c
>>> @@ -193,23 +193,6 @@ static int mpfs_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>>>           return 0;
>>>    }
>>>
>>> -static inline struct clk *mpfs_rtc_init_clk(struct device *dev)
>>> -{
>>> -       struct clk *clk;
>>> -       int ret;
>>> -
>>> -       clk = devm_clk_get(dev, "rtc");
>>> -       if (IS_ERR(clk))
>>> -               return clk;
>>> -
>>> -       ret = clk_prepare_enable(clk);
>>> -       if (ret)
>>> -               return ERR_PTR(ret);
>>> -
>>> -       devm_add_action_or_reset(dev, (void (*) (void *))clk_disable_unprepare, clk);
>>
>> ... this bit here concerns me a little. I don't think we should be
>> registering a callback here at all - if we power down Linux this is
>> going to end up stopping the RTC isn't it?
>>
>> I think this is left over from the v1 driver submission that reset
>> the block during probe & should be removed.
>>
>> Thanks,
>> Conor.
>>
>>> -       return clk;
>>> -}
>>> -
>>>    static irqreturn_t mpfs_rtc_wakeup_irq_handler(int irq, void *dev)
>>>    {
>>>           struct mpfs_rtc_dev *rtcdev = dev;
>>> @@ -251,7 +234,7 @@ static int mpfs_rtc_probe(struct platform_device *pdev)
>>>           /* range is capped by alarm max, lower reg is 31:0 & upper is 10:0 */
>>>           rtcdev->rtc->range_max = GENMASK_ULL(42, 0);
>>>
>>> -       clk = mpfs_rtc_init_clk(&pdev->dev);
>>> +       clk = devm_clk_get_enabled(&pdev->dev, "rtc");
>>>           if (IS_ERR(clk))
>>>                   return PTR_ERR(clk);
>>>
>>> --
>>> 2.34.1
>>>
>>>
>>> _______________________________________________
>>> linux-riscv mailing list
>>> linux-riscv@xxxxxxxxxxxxxxxxxxx
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com