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
