On 12/6/2024 10:04 AM, 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 08:09, Ciprian Costea wrote:
From: Ciprian Marian Costea <ciprianmarian.costea@xxxxxxxxxxx>
Add a RTC driver for NXP S32G2/S32G3 SoCs.
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.
Co-developed-by: Bogdan Hamciuc <bogdan.hamciuc@xxxxxxx>
Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@xxxxxxx>
Co-developed-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@xxxxxxx>
Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@xxxxxxx>
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@xxxxxxxxxxx>
Hello Arnd,
Thanks you for your review on this patchset.
I read through the driver and this looks all good to me, but there
are two fairly minor things I noticed:
+ u64 rtc_hz;
I see the clock rate is a 64-bit value, which is clearly what
comes from the clk interface in the kernel
+static u64 cycles_to_sec(u64 hz, u64 cycles)
+{
+ return div_u64(cycles, hz);
+}
and you divide by the clk rate to convert the register value
to seconds (as expected)
+ u32 delta_cnt;
+
+ if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_VAL))
+ return -EINVAL;
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.
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.
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."
+static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
+ u32 offset)
+{
+ u32 counter;
+
+ counter = ioread32(priv->rtc_base + offset);
+
+ if (counter < priv->base.cycles)
+ return -EINVAL;
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;
+ }
+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.
One more minor comment: you are using ioread32()/iowrite32()
to access the MMIO registers, which is a bit unusual. I would
suggest changing those to the more common readl()/writel()
that do the exact same thing on arm64.
Arnd
Makes sense. I will change to readl/writel in V7.
Best Regards,
Ciprian
