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