Re: [PATCH v1 1/1] rtc: zynqmp: ensure correct RTC calibration

[Lothar Rubusch]

On Wed, Aug 6, 2025 at 12:24 AM Alexandre Belloni
<alexandre.belloni@xxxxxxxxxxx> wrote:
>
> On 05/08/2025 23:56:46+0200, Lothar Rubusch wrote:
> > On Mon, Aug 4, 2025 at 11:32 PM Alexandre Belloni
> > <alexandre.belloni@xxxxxxxxxxx> wrote:
> > >
> > > On 04/08/2025 15:47:50+0000, Lothar Rubusch wrote:
> > > > From: Ivan Vera <ivan.vera@xxxxxxxxxxxxx>
> > (...)
> > > > diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
> > > > index f39102b66eac..0c063c3fae52 100644
> > > > --- a/drivers/rtc/rtc-zynqmp.c
> > > > +++ b/drivers/rtc/rtc-zynqmp.c
> > > > @@ -331,9 +331,9 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
> > > >               if (ret)
> > > >                       xrtcdev->freq = RTC_CALIB_DEF;
> > > >       }
> > > > -     ret = readl(xrtcdev->reg_base + RTC_CALIB_RD);
> > > > -     if (!ret)
> > > > -             writel(xrtcdev->freq, (xrtcdev->reg_base + RTC_CALIB_WR));
> > > > +
> > > > +     /* Enable unconditional re-calibration to RTC_CALIB_DEF or DTB entry. */
> > > > +     writel(xrtcdev->freq, xrtcdev->reg_base + RTC_CALIB_WR);
> > >
> > > Doesn't this forcefully overwrite the proper value that has been set
> > > from userspace and so trashes the time at each reboot?
> > >
> > Yes, it overwrites the calibration, i.e. counting 1sec in about 1sec.
> > No, the time/date is not actually "trashed" (I double-checked that
> > with timesyncd disabled, having and not having register content and
> > over several reboots keeping a bogus date/time - it psersistet in the
> > same time space. The current patch always overwrites the calib
> > register content. So, a manual userspace setting will be lost after
> > reboot. That's true.
>
> It is about 1sec on your platform because it didn't deviate too much
> from the expected value but what if another platform needs a way
> different value? Then you are introducing the same issue as the one you
> are trying to fix but it will have it at each reboot.
>
I guess you missunderstood me here a bit. I understand that every
scenario will need individual calibration especially over time.

> >
> > Would it rather make sense to extend it, say, instead of merely
> > checking whether the calibration register contains any data - which
> > could potentially be incorrect - also check for the presence of a
> > calibration property in the devicetree (or a similar property, since
> > 'calibration' may be deprecated)? If such a property exists, perform a
> > re-calibration based on the devicetree at every reboot. Otherwise,
> > retain the current behavior of checking whether the register is empty?
> >
> > > Isn't the proper way to reset it to simply set the offset from userspace
> > > again?
> > >
> > Hm.. I'm unsure if I understood you correctly. You mean the way as
> > described in AMD's link to perform the reset by executing the devmem
> > from Linux manually? If so, why is it preferable to adjust the RTC
> > calibration manually every time this happens, rather than to simply
> > put a correction value into the devicetree properties for problematic
> > setups? Or do I miss something, is there a config file for RTC
> > calibration for doing this persistently from Linux, that I'm not aware
> > of?
> >
> > Before, the devicetree properties seemed to have generally priority
> > over userspace settings. Now, after the calibration rework, this
> > priorization seems to have changed and a devicetree calib correction
> > for such problematic cases will generally be ignored, with a
> > recommendation by Xilinx/AMD (see link in cover letter) to execute a
> > devmem command from off Linux (...). I mean, can't this be elaborated
> > a bit more to allow for a persistent correction method?
>
> The value depends on each manufactured machine/board as it is supposed
> to correct for imprecision on the input clock which is either a crystal
> or derived from a crystal. This crystal may be more or less accurate and
> its accuracy will change over time notably because of temperature
> changes or simply because it is aging. Having the value in the device
> tree is as good as having it hardcoded in the driver which is not far
> from what your are doing here. It makes the feature useless.
>
Yes, I see your point.

> What I was suggesting is simply to do the right thing, compute the
> inaccuracy and correct it from userspace, using the proper interface
> that is sysfs or the RTC_PARAM_SET ioctl for RTC_PARAM_CORRECTION
> This has to be done regularly anyway so I guess it would catch and
> correct any corrupted value in the register.
>
The degradation over time and or temperature does not match the static
approach we took of our v1 patch. I modified it still a bit to keep userspace
configurations, and use the optional devicetree property for a correction. But
this does not cancel out your argumentation, since the approach is still static.
So, I have to agree, thanks.

Best,
L

> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com