Message-ID: <6f4a0be8-4def-4066-9b44-d43059b7a90d@oss.nxp.com>
Date: Fri, 6 Dec 2024 14:05:07 +0200
From: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>
To: Arnd Bergmann <arnd@...db.de>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Catalin Marinas
 <catalin.marinas@....com>, Will Deacon <will@...nel.org>
Cc: linux-rtc@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 NXP S32 Linux Team <s32@....com>, imx@...ts.linux.dev,
 Christophe Lizzi <clizzi@...hat.com>, Alberto Ruiz <aruizrui@...hat.com>,
 Enric Balletbo <eballetb@...hat.com>, Bogdan Hamciuc
 <bogdan.hamciuc@....com>, Ghennadi Procopciuc <Ghennadi.Procopciuc@....com>
Subject: Re: [PATCH v6 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

On 12/6/2024 10:04 AM, Arnd Bergmann wrote:
> [You don't often get email from arnd@...db.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Fri, Dec 6, 2024, at 08:09, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>
>>
>> Add a RTC driver for NXP S32G2/S32G3 SoCs.
>>
>> RTC tracks clock time during system suspend. It can be a wakeup source
>> for the S32G2/S32G3 SoC based boards.
>>
>> The RTC module from S32G2/S32G3 is not battery-powered and it is not kept
>> alive during system reset.
>>
>> Co-developed-by: Bogdan Hamciuc <bogdan.hamciuc@....com>
>> Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@....com>
>> Co-developed-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@....com>
>> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@....com>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>

Hello Arnd,

Thanks you for your review on this patchset.

> 
> I read through the driver and this looks all good to me, but there
> are two fairly minor things I noticed:
> 
>> +     u64 rtc_hz;
> 
> I see the clock rate is a 64-bit value, which is clearly what
> comes from the clk interface in the kernel
> 
>> +static u64 cycles_to_sec(u64 hz, u64 cycles)
>> +{
>> +     return div_u64(cycles, hz);
>> +}
> 
> and you divide by the clk rate to convert the register value
> to seconds (as expected)
> 
>> +     u32 delta_cnt;
>> +
>> +     if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_VAL))
>> +             return -EINVAL;
> 
> However, the range of the register value is only 32 bits,
> which means there is no need to ever divide it by a 64-bit
> number, and with the 32kHz clock in the binding example,
> you only have about 37 hours worth of range here.
> 

I am not sure what is the suggestion here. To cast 'cycles' variable to 
32-bit ?
If yes, indeed 'div_u64' converts 'cycles' (the divisor) to 32-bit so I 
agree it should be u32 instead of u64.
If not, I would prefer to keep using a 64-by-32 division and avoid 
casting 'hz' to 32-bit.

> It would appear that this makes the rtc unsuitable for
> storing absolute time across reboots, and only serve during
> runtime, which is a limitation you should probably document.
> 

Actually there is the option to use DIV512 and/or DIV32 hardware 
divisors for the RTC clock. The driver uses a DIV512 divisor by default 
in order to achieve higher RTC count ranges (by achieving a smaller RTC 
freq). Therefore, the 37 hours become 37 * 512 => ~ 2 years.

However, the rtc limitation of not being persistent during reboot 
remains, due to hardware RTC module registers present of NXP S32G2/S32G3 
SoCs being reset during system reboot. On the other hand, during system 
suspend, the RTC module will keep counting if a clock source is available.

Currently, this limittion is documented as follows:
"RTC tracks clock time during system suspend. It can be a wakeup source 
for the S32G2/S32G3 SoC based boards.

The RTC module from S32G2/S32G3 is not battery-powered and it is not 
kept alive during system reset."

>> +static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
>> +                                  u32 offset)
>> +{
>> +     u32 counter;
>> +
>> +     counter = ioread32(priv->rtc_base + offset);
>> +
>> +     if (counter < priv->base.cycles)
>> +             return -EINVAL;
> 
> If 'counter' wraps every 37 hours, this will inevitably fail,
> right? E.g. if priv->base.cycles was already at a large
> 32-bit number, even reading it shortly later will produce
> a small value after the wraparound.
> 
> Using something like time_before() should address this,
> but I think you may need a custom version that works on
> 32-bit numbers.
> 

This is correct. Would the following change be acceptable ?

-       if (counter < priv->base.cycles)
-               return -EINVAL;
-
-       counter -= priv->base.cycles;
+       if (counter < priv->base.cycles) {
+               /* A rollover on RTCCTN has occurred */
+               counter += RTCCNT_MAX_VAL - priv->base.cycles;
+               priv->base.cycles = 0;
+       } else {
+               counter -= priv->base.cycles;
+       }


>> +static int s32g_rtc_resume(struct device *dev)
>> +{
>> +     struct rtc_priv *priv = dev_get_drvdata(dev);
>> +     int ret;
>> +
>> +     if (!device_may_wakeup(dev))
>> +             return 0;
>> +
>> +     /* Disable wake-up interrupts */
>> +     s32g_enable_api_irq(dev, 0);
>> +
>> +     ret = rtc_clk_src_setup(priv);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /*
>> +      * Now RTCCNT has just been reset, and is out of sync with priv->base;
>> +      * reapply the saved time settings.
>> +      */
>> +     return s32g_rtc_set_time(dev, &priv->base.tm);
>> +}
> 
> This also fails if the system has been suspended for more than
> 37 hours, right?

Actually, the system would not go into suspend (returning with error) if 
the alarm setting passes the 32-bit / clk_freq range.
The check is added in 'sec_to_rtcval' which is called from the suspend 
routine.

> 
> One more minor comment: you are using ioread32()/iowrite32()
> to access the MMIO registers, which is a bit unusual. I would
> suggest changing those to the more common readl()/writel()
> that do the exact same thing on arm64.
> 
>          Arnd

Makes sense. I will change to readl/writel in V7.

Best Regards,
Ciprian

Powered by blists - more mailing lists