Message-Id: <94cba886-86cb-41f1-96ee-501623add7db@app.fastmail.com>
Date: Fri, 06 Dec 2024 13:41:55 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Ciprian Costea" <ciprianmarian.costea@....nxp.com>,
 "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 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.

>> 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?

> 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))

>>> +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

Powered by blists - more mailing lists