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

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

[Lothar Rubusch]

We've observed intermittent failures with the current implementation on our
Xilinx hardware. These issues were not present prior to the commit
referenced by the Fixes tag, using the same hardware setup. The problem
appears to be identical to the one described here:
https://adaptivesupport.amd.com/s/article/000036886?language=en_US

Instead of explicitly writing 0x7FFF to the calibration register using
devmem, the patch now resets the register.

Ivan Vera (1):  rtc: zynqmp: ensure correct RTC calibration

 drivers/rtc/rtc-zynqmp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: 260f6f4fda93c8485c8037865c941b42b9cba5d2
-- 
2.39.5

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

[Lothar Rubusch]

From: Ivan Vera <ivan.vera@enclustra.com>

In the event of an uninitialized calibration register, ensure the register
is reset and properly programmed during the probe sequence.

At present, only the calibration register is evaluated. If it holds invalid
values after a power cycle, there's no longer a way to reset it, for
instance, via a devicetree entry to 0x7FFF. This issue is documented here:
https://adaptivesupport.amd.com/s/article/000036886?language=en_US 

The fix prioritizes an optional calibration value provided via the
devicetree over the value in the register.

Fixes: 07dcc6f9c76275d6679f28a69e042a2f9dc8f128 ("rtc: zynqmp: Add calibration set and get support")
Signed-off-by: Ivan Vera <ivan.vera@enclustra.com>
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/rtc/rtc-zynqmp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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);
 
 	xlnx_init_rtc(xrtcdev);
 
-- 
2.39.5

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

[Alexandre Belloni]

On 04/08/2025 15:47:50+0000, Lothar Rubusch wrote:
> From: Ivan Vera <ivan.vera@enclustra.com>
> 
> In the event of an uninitialized calibration register, ensure the register
> is reset and properly programmed during the probe sequence.
> 
> At present, only the calibration register is evaluated. If it holds invalid
> values after a power cycle, there's no longer a way to reset it, for
> instance, via a devicetree entry to 0x7FFF. This issue is documented here:
> https://adaptivesupport.amd.com/s/article/000036886?language=en_US 
> 
> The fix prioritizes an optional calibration value provided via the
> devicetree over the value in the register.
> 
> Fixes: 07dcc6f9c76275d6679f28a69e042a2f9dc8f128 ("rtc: zynqmp: Add calibration set and get support")
> Signed-off-by: Ivan Vera <ivan.vera@enclustra.com>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/rtc/rtc-zynqmp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> 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?

Isn't the proper way to reset it to simply set the offset from userspace
again?

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

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

[Lothar Rubusch]

On Mon, Aug 4, 2025 at 11:32 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 04/08/2025 15:47:50+0000, Lothar Rubusch wrote:
> > From: Ivan Vera <ivan.vera@enclustra.com>
(...)
> > 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.

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?

Just, let me know what you think about. Thank you for the feedback.
Best,
L

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

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

[Alexandre Belloni]

On 05/08/2025 23:56:46+0200, Lothar Rubusch wrote:
> On Mon, Aug 4, 2025 at 11:32 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > On 04/08/2025 15:47:50+0000, Lothar Rubusch wrote:
> > > From: Ivan Vera <ivan.vera@enclustra.com>
> (...)
> > > 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.

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

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.

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

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@bootlin.com> 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@bootlin.com> wrote:
> > >
> > > On 04/08/2025 15:47:50+0000, Lothar Rubusch wrote:
> > > > From: Ivan Vera <ivan.vera@enclustra.com>
> > (...)
> > > > 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