Message-ID: <42d39f8e-0b59-4185-af1f-f778522608d2@oss.nxp.com>
Date: Mon, 9 Dec 2024 19:17:42 +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 2:41 PM, 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 13:05, Ciprian Marian Costea wrote:
>> On 12/6/2024 10:04 AM, Arnd Bergmann wrote:
>>>
>>> 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.
> 
> The confusing bit here is that the extra function just serves to
> the dividend 'cycles' from 32-bit to 64-bit, and then calling
> div_u64() implicitly casts the dividend 'hz' from 64-bit to
> 32-bit, so you definitely get a 32-by-32 division already
> if the function is inlined properly.
> 
> I think storing 'rtc_hz' as a u32 variable and adding a range
> check when filling it would help, mainly to save the next reader
> from having to understand what is going on.
> 

The confusion on my end is that I cannot see where 'div_u64() implicitly 
casts the dividend 'hz' from 64-bit to 32-bit' by following the method's 
implementation [1]

But I agree that 'rtc_hz' can be stored into a 32-bit variable with a 
range check added when it is taken from the Linux clock API to avoid any
unneeded abstractions.

[1] 
https://elixir.bootlin.com/linux/v6.12.4/source/include/linux/math64.h#L127

>>> 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.
> 
> Ah, makes sense. Can you add comments in an appropriate place in
> the code about this?
> 

Sure. I will add such comment in the S32G RTC driver in V7.

>> 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."
> 
> My bad, I really should not have skipped the patch
> description ;-)
> 
>>> 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;
>> +       }
> 
> This is the same as just removing the error handling and
> relying on unsigned integer overflow semantics.
> 
> The usual check we do in time_before()/time_after instead
> checks if the elapsed time is less than half the available
> range:
> 
> #define time_after(a,b)         \
>          (typecheck(unsigned long, a) && \
>           typecheck(unsigned long, b) && \
>           ((long)((b) - (a)) < 0))
> 

Ok. Thanks for the suggestion. I will look into using 
'time_before()/time_after()' API instead of directly checking via 
comparison operators.

>>>> +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.
> 
> Who sets that alarm though? Are you relying on custom userspace
> for this, or is that something that the kernel already does
> that I'm missing?
> 
>         Arnd

The test usage is via 'rtcwake' [2] userspace tool.
I've detailed a bit the testing scenario in the cover letter for this 
patchset [3]:

"
Following is an example of Suspend to RAM trigger on S32G2/S32G3 SoCs,
using userspace tools such as rtcwake:
# rtcwake -s 2 -m mem
# rtcwake: assuming RTC uses UTC ...
# rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Feb  6 06:28:36 2036
#
"

[2] https://man7.org/linux/man-pages/man8/rtcwake.8.html
[3] 
https://lore.kernel.org/all/20241206070955.1503412-1-ciprianmarian.costea@oss.nxp.com/

Best Regards,
Ciprian

Powered by blists - more mailing lists