Re: [PATCH v6 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

On Mon, Dec 9, 2024, at 18:17, Ciprian Marian Costea wrote:

On 12/6/2024 2:41 PM, Arnd Bergmann wrote:

quoted

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]

I mean passing a 64-bit variable into a function that takes a
32-bit argument truncates the range.

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.

ok

quoted

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.

To be clear: you can't directly use time_before() here because
that takes an 'unsigned long' argument, so you want the
same logic, but for u32 values. I have not found an existing
helper for that, but it's possible I missed it.

quoted

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?

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

Got it. I feel this also needs either some documentation in
the source code, or some infrastructure in the rtc layer if
this is a common problem in other drivers as well. If there
is a maximum time that the system can be suspended for without
a wakeup, why not just set an earlier wakeup in the kernel
when you have all the information for it?

Or maybe this should not actually be an 'rtc' driver at all?
In the old days, we used drivers like
arch/arm/mach-omap1/timer32k.c to register a handler
for read_persistent_clock64(), which completely bypasses
the RTC layer and provides both automatic wakeup and more
accurate accounting of sleep time. 

Another example was the tegra clocksource driver, which used
to use read_persistent_clock64() but changed to being
a CLOCK_SOURCE_SUSPEND_NONSTOP source in 95170f0708f2
("clocksource/drivers/tegra: Rework for compensation of
suspend time"). The same seems true for timer-ti-32k.c and
timer-sprd.c.

Alexandre, Daniel, any recommendations here?

     Arnd