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

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

[Christophe JAILLET]

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.

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@wanadoo.fr>
---
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);
-	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

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

[Conor.Dooley]

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.

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@wanadoo.fr>
> ---
> 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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Alexandre Belloni]

On 24/08/2022 09:58:35+0000, Conor.Dooley@microchip.com 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?

> 
> 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@wanadoo.fr>
> > ---
> > 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@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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@microchip.com 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@microchip.com>

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@wanadoo.fr>
>>> ---
>>> 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@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

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

[Christophe JAILLET]

Le 24/08/2022 à 11:58, Conor.Dooley@microchip.com a écrit :
> 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.

Ok, I'll update my commit log for other similar patches or should a v2 
be needed.

> 
> 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.
> 
> 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@wanadoo.fr>
>> ---
>> 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.

My point is only that what is done must be undone at some point.

What if an error occurs in the probe after the clk_get("rtc")?
Is there any point keeping it prepared and enabled?


There is a .remove function in this driver, so, it looks that it is 
expected that it can be unloaded.

So undoing this clk operations via a managed resource looks the correct 
thing to do.

Just my 2c, you must know this driver and the expected behavior better 
than me.

CJ

> 
> 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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

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

[Conor.Dooley]

On 24/08/2022 12:27, Christophe JAILLET wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Le 24/08/2022 à 11:58, Conor.Dooley@microchip.com a écrit :
>> 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.
> 
> Ok, I'll update my commit log for other similar patches or should a v2
> be needed.
> 
>>
>> 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.
>>
>> However...
>>

>>> -       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.
> 
> My point is only that what is done must be undone at some point.
> 
> What if an error occurs in the probe after the clk_get("rtc")?
> Is there any point keeping it prepared and enabled?
> 
> 
> There is a .remove function in this driver, so, it looks that it is
> expected that it can be unloaded.
> 
> So undoing this clk operations via a managed resource looks the correct
> thing to do.
> 
> Just my 2c, you must know this driver and the expected behavior better
> than me.
>

I am doing some more testing rn, before replying to Alexandre - I guess
I am just wondering if actually disabling the clock prior to to removing
the driver makes any sense. Maybe the lock just needs to be explicitly
marked as critical, in which case this patch makes complete sense to me.

Thanks,
Conor.

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

[Alexandre Belloni]

On 24/08/2022 13:27:02+0200, Christophe JAILLET wrote:
> Le 24/08/2022 à 11:58, Conor.Dooley@microchip.com a écrit :
> > 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.
> 
> Ok, I'll update my commit log for other similar patches or should a v2 be
> needed.
> 
> > 
> > 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.
> > 
> > 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@wanadoo.fr>
> > > ---
> > > 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.
> 
> My point is only that what is done must be undone at some point.
> 
> What if an error occurs in the probe after the clk_get("rtc")?
> Is there any point keeping it prepared and enabled?
> 
> 
> There is a .remove function in this driver, so, it looks that it is expected
> that it can be unloaded.
> 
> So undoing this clk operations via a managed resource looks the correct
> thing to do.
> 
> Just my 2c, you must know this driver and the expected behavior better than
> me.
> 

BTW, I thought you actually tested your changes on the other patch I
took, not that you were doing a blanket conversion of the subsystem.
This is the kind of info that must appear in the commit log. I would
definitively not have taken the patch.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Christophe JAILLET]

Le 24/08/2022 à 14:28, Alexandre Belloni a écrit :
> 
> BTW, I thought you actually tested your changes on the other patch I
> took, not that you were doing a blanket conversion of the subsystem.
> This is the kind of info that must appear in the commit log. I would
> definitively not have taken the patch.
> 

Ok, noted for future contribution.


In fact I first sent only one patch to see if it got some interest for 
such transformation.
I only sent some other after your Ack.


Nothing is never trivial, but such patches looks fine to me.
It saves some LoC, reduce the size of the .o and slightly saves some 
runtime memory.

And unless, I missed something, the order of operation remains the same, 
both when resources are allocated and freed.


Why wouldn't you have taken such a patch?
(just for my understanding and in order to avoid spamming others with 
useless/risky stuff)

CJ

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

[Alexandre Belloni]

On Wed, 24 Aug 2022 10:18:25 +0200, Christophe JAILLET wrote:
> 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().
> 
> [...]

Applied, thanks!

[1/1] rtc: mpfs: Use devm_clk_get_enabled() helper
      commit: 24fb316155a5f6ba278a8b110c60e67b79900356

Best regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Conor.Dooley]

On 13/10/2022 22:32, Alexandre Belloni wrote:
> On Wed, 24 Aug 2022 10:18:25 +0200, Christophe JAILLET wrote:
>> 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().
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] rtc: mpfs: Use devm_clk_get_enabled() helper
>       commit: 24fb316155a5f6ba278a8b110c60e67b79900356
> 
> Best regards,
> 

While this is on my mind, making the rtc's ahb clock critical came
up in previous discussion about this patch. A fix for that was applied
to v6.0.

Thanks,
Conor.