Re: [PATCH 1/2] rtc: m5441x: add MCF5441x on-chip RTC driver

[Geert Uytterhoeven]

Hi Jean-Michel,

On Tue, 2 Jun 2026 at 10:36, Jean-Michel Hautbois
<jeanmichel.hautbois@xxxxxxxxxx> wrote:
> Add an rtc-class driver for the Freescale MCF5441x on-chip "robust" RTC.
> It provides the time/calendar and alarm, and exposes the 2KB
> battery-backed standby RAM through the nvmem framework so userspace can
> preserve data across a main-power loss (the RAM is retained while
> VSTBY_RTC is supplied).
>
> Register and standby-RAM writes go through the RTC_CR[WE] knock
> sequence; the base-2112 year encoding and register map follow the
> MCF54418 reference manual. Based on the out-of-tree Freescale 3.0.x
> rtc-m5441x driver, rewritten for the current RTC and nvmem APIs.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxx>

Thanks for your patch!

> +/*
> + * The time counters are unstable for an oscillator cycle either side of
> + * the one-second boundary. RTC_SR[INVAL] flags this; reads during the
> + * window return 0xffff and writes are nullified. Spin until it clears.
> + * The window is only a couple of 32kHz cycles (~60us), so bound the
> + * busy-wait tightly: it runs with the lock held and interrupts off.
> + * Caller holds p->lock.
> + */
> +static int m5441x_rtc_wait_valid(struct m5441x_rtc *p)
> +{
> +       unsigned int tries = 10;
> +
> +       while (rtc_rd(p, M5441X_RTC_SR) & M5441X_RTC_SR_INVAL) {
> +               if (!--tries)
> +                       return -EIO;
> +               udelay(10);
> +       }

Please use read_poll_timeout().

> +
> +       return 0;
> +}

> +static int m5441x_rtc_nvram_read(void *priv, unsigned int offset,
> +                                void *val, size_t bytes)
> +{
> +       struct m5441x_rtc *p = priv;
> +       u8 *buf = val;
> +       size_t done;
> +
> +       /*
> +        * In-kernel nvmem_device_read() forwards offset/bytes verbatim, so
> +        * range-check here rather than trust the caller.
> +        */
> +       if (offset >= M5441X_RTC_SRAM_SIZE ||
> +           bytes > M5441X_RTC_SRAM_SIZE - offset)
> +               return -EINVAL;
> +
> +       /*
> +        * Process the transfer in chunks, releasing the lock between them, so
> +        * a full 2KB access does not keep hard interrupts disabled across
> +        * thousands of slow on-chip MMIO cycles and wreck IRQ latency.
> +        */
> +       for (done = 0; done < bytes; done += M5441X_RTC_SRAM_CHUNK) {
> +               size_t chunk = min_t(size_t, bytes - done, M5441X_RTC_SRAM_CHUNK);

size_t looks like overkill to me.

> +               unsigned long flags;
> +               size_t i;

Likewise

> +
> +               spin_lock_irqsave(&p->lock, flags);

scoped_guard(spinlock, &p->lock)?

> +               for (i = 0; i < chunk; i++)
> +                       buf[done + i] = ioread8(p->base + M5441X_RTC_SRAM_OFFSET +
> +                                               offset + done + i);
> +               spin_unlock_irqrestore(&p->lock, flags);
> +       }
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds