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

[Jean-Michel Hautbois]

Hi Geert,

Le 02/06/2026 à 12:04, Geert Uytterhoeven a écrit :
> 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().

As wait_valid() is called after spin_lock_irqsave() is called, I suppose 
I should use read_poll_timeout_atomic() ? Because read_poll_timeout() 
sleeps ?

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

Done.

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

Sure, thanks !

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