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

[Arnd Bergmann]

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>

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.

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.

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

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

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