On 12/6/2024 2:41 PM, Arnd Bergmann wrote:
[You don't often get email from arnd@xxxxxxxx. 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@xxxxxxxxxxx/
Best Regards,
Ciprian
