Re: [PATCH v2] rtc: zynqmp: Enable crystal oscillator when setting time

["T, Harini" <harini.t@amd.com>]

Hi,

On 9/25/2026 10:50 AM, Daniel Viaño wrote:
> [You don't often get email from danividanivi@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> The RTC_OSC_EN macro was defined when the driver was originally
> introduced, but was never asserted by the driver. On cold boots, after
> battery exhaustion, or on platforms where firmware does not configure
> RTC_CTRL, the oscillator remains disabled and the counter never ticks.
> 
> Unconditionally enabling the oscillator during driver initialization
> would cause the RTC to free-run from an uninitialized or stale counter
> value, reporting an untrusted time to userspace. Instead, keep the
> oscillator disabled until a valid time is programmed: guard
> xlnx_rtc_read_time() on RTC_OSC_EN and return -EINVAL when it is not
> set, and assert RTC_OSC_EN in xlnx_rtc_set_time() once valid time is
> programmed so the RTC can free-run.
> 
> Per the Zynq UltraScale+ TRM (UG1085), bit 24 (OSC_CNTRL) is a static
> level enable for the crystal inverter rather than an edge-triggered reset;
> asserting it from set_time() is non-destructive and will not glitch an
> already-running oscillator.
>

Thanks, but the premise doesn't hold: Osc_Cntrl resets to "enabled", not
disabled - ZynqMP reset 0x1 (bit 24 = 1), Versal reset 0x2 (crystal 
mode, bit 24 = 0). So after a cold boot / battery exhaustion the 
oscillator is already on and the counter ticks from ~0;

- ZynqMP: bit 24 is 1 out of reset and never cleared, so the read_time
-EINVAL check is unreachable



> Tested on Zynq UltraScale+ hardware, confirming that read_time returns
> -EINVAL while the oscillator is disabled, and advances reliably after
> setting time.

Also, how was the "oscillator disabled" state produced in test? With bit 
24 = 1 out of reset I don't see how read_time hits -EINVAL on a real 
cold boot.

Thanks,
Harini T

> 
> Fixes: 11143c19eb57 ("rtc: add xilinx zynqmp rtc driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Viaño <danividanivi@gmail.com>
> ---
> v2:
>   - Leave xlnx_init_rtc() unchanged so the oscillator is not enabled on probe
>   - Return -EINVAL in xlnx_rtc_read_time() when RTC_OSC_EN is not set
>   - Enable RTC_OSC_EN in xlnx_rtc_set_time() once valid time is programmed
> 
>   drivers/rtc/rtc-zynqmp.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
> index 5bcb7536e973..6c3ba8f1aa57 100644
> --- a/drivers/rtc/rtc-zynqmp.c
> +++ b/drivers/rtc/rtc-zynqmp.c
> @@ -59,6 +59,8 @@ static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>          struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>          unsigned long new_time;
> 
> +       u32 rtc_ctrl;
> +
>          /*
>           * The value written will be updated after 1 sec into the
>           * seconds read register, so we need to program time +1 sec
> @@ -78,6 +80,14 @@ static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>           */
>          writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_STS);
> 
> +       /*
> +        * Now that the time is valid, start the crystal oscillator so the
> +        * RTC free-runs.
> +        */
> +       rtc_ctrl = readl(xrtcdev->reg_base + RTC_CTRL);
> +       rtc_ctrl |= RTC_OSC_EN;
> +       writel(rtc_ctrl, xrtcdev->reg_base + RTC_CTRL);
> +
>          return 0;
>   }
> 
> @@ -87,6 +97,9 @@ static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
>          unsigned long read_time;
>          struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> 
> +       if (!(readl(xrtcdev->reg_base + RTC_CTRL) & RTC_OSC_EN))
> +               return -EINVAL;
> +
>          status = readl(xrtcdev->reg_base + RTC_INT_STS);
> 
>          if (status & RTC_INT_SEC) {
> --
> 2.53.0