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

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
> <[email protected]> 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 <[email protected]>
> 
> 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
>