[PATCH 0/4] rtc: zynqmp: fixes for read and set offset

[PATCH 0/4] rtc: zynqmp: fixes for read and set offset

[Tomas Melin]

Add improvements for read and set offset functions.
The basic functionality is still the same, but offset correction values
are now updated to match with expected.

The RTC calibration value operates with full ticks,
and fractional ticks which are a 1/16 of a full tick.
The 16 lowest bits in the calibration registers are for the full ticks
and value matches the external oscillator in Hz. Through that,
the maximum and minimum offset values can be calculated dynamically,
as they depend on the input frequency used.

For docs on the calibration register, see
https://docs.amd.com/r/en-US/ug1087-zynq-ultrascale-registers/CALIB_READ-RTC-Register

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
Tomas Melin (4):
      rtc: zynqmp: correct frequency value
      rtc: zynqmp: rework read_offset
      rtc: zynqmp: rework set_offset
      rtc: zynqmp: use dynamic max and min offset ranges

 drivers/rtc/rtc-zynqmp.c | 65 ++++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 32 deletions(-)
---
base-commit: cd635e33b0113287c94021be53d2a7c61a1614e9
change-id: 20251201-zynqmp-rtc-updates-d260364cc01b

Best regards,
-- 
Tomas Melin <tomas.melin@vaisala.com>

[PATCH 1/4] rtc: zynqmp: correct frequency value

[Tomas Melin]

Fix calibration value in case a clock reference is provided.
The actual calibration value written into register is
frequency - 1.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/rtc/rtc-zynqmp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
index 3baa2b481d9f2008750046005283b98a0d546c5c..856bc1678e7d31144f320ae9f75fc58c742a2a64 100644
--- a/drivers/rtc/rtc-zynqmp.c
+++ b/drivers/rtc/rtc-zynqmp.c
@@ -345,7 +345,10 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
 					   &xrtcdev->freq);
 		if (ret)
 			xrtcdev->freq = RTC_CALIB_DEF;
+	} else {
+		xrtcdev->freq--;
 	}
+
 	ret = readl(xrtcdev->reg_base + RTC_CALIB_RD);
 	if (!ret)
 		writel(xrtcdev->freq, (xrtcdev->reg_base + RTC_CALIB_WR));

-- 
2.47.3

[PATCH 2/4] rtc: zynqmp: rework read_offset

[Tomas Melin]

read_offset() was using static frequency for determining
the tick offset. It was also using remainder from do_div()
operation as tick_mult value which caused the offset to be
incorrect.

At the same time, rework function to improve readability.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/rtc/rtc-zynqmp.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
index 856bc1678e7d31144f320ae9f75fc58c742a2a64..7af5f6f99538f961a53ff56bfc656c907611b900 100644
--- a/drivers/rtc/rtc-zynqmp.c
+++ b/drivers/rtc/rtc-zynqmp.c
@@ -178,21 +178,28 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev *xrtcdev)
 static int xlnx_rtc_read_offset(struct device *dev, long *offset)
 {
 	struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
-	unsigned long long rtc_ppb = RTC_PPB;
-	unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
-	unsigned int calibval;
+	unsigned int calibval, fract_data, fract_part;
+	int max_tick, tick_mult;
+	int freq = xrtcdev->freq;
 	long offset_val;
 
+	/* ticks to reach RTC_PPB */
+	tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, freq);
+
 	calibval = readl(xrtcdev->reg_base + RTC_CALIB_RD);
 	/* Offset with seconds ticks */
-	offset_val = calibval & RTC_TICK_MASK;
-	offset_val = offset_val - RTC_CALIB_DEF;
-	offset_val = offset_val * tick_mult;
+	max_tick = calibval & RTC_TICK_MASK;
+	offset_val = max_tick - freq;
+	/* Convert to ppb */
+	offset_val *= tick_mult;
 
 	/* Offset with fractional ticks */
-	if (calibval & RTC_FR_EN)
-		offset_val += ((calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT)
-			* (tick_mult / RTC_FR_MAX_TICKS);
+	if (calibval & RTC_FR_EN) {
+		fract_data = (calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT;
+		fract_part = DIV_ROUND_UP(tick_mult, RTC_FR_MAX_TICKS);
+		offset_val += (fract_part * fract_data);
+	}
+
 	*offset = offset_val;
 
 	return 0;

-- 
2.47.3

[PATCH 3/4] rtc: zynqmp: rework set_offset

[Tomas Melin]

set_offset was using remainder of do_div as tick_mult which resulted in
wrong offset. Calibration value also assumed builtin calibration default.
Update fract_offset to correctly calculate the value for
negative offset and replace the for loop with division.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/rtc/rtc-zynqmp.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
index 7af5f6f99538f961a53ff56bfc656c907611b900..3bc8831ba2c4c4c701a49506b67ae6174f3ade3d 100644
--- a/drivers/rtc/rtc-zynqmp.c
+++ b/drivers/rtc/rtc-zynqmp.c
@@ -208,13 +208,13 @@ static int xlnx_rtc_read_offset(struct device *dev, long *offset)
 static int xlnx_rtc_set_offset(struct device *dev, long offset)
 {
 	struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
-	unsigned long long rtc_ppb = RTC_PPB;
-	unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
+	unsigned int calibval, tick_mult, fract_part;
 	unsigned char fract_tick = 0;
-	unsigned int calibval;
-	short int  max_tick;
-	int fract_offset;
+	int freq = xrtcdev->freq;
+	int max_tick, fract_offset;
 
+	/* ticks to reach RTC_PPB */
+	tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, xrtcdev->freq);
 	if (offset < RTC_MIN_OFFSET || offset > RTC_MAX_OFFSET)
 		return -ERANGE;
 
@@ -223,29 +223,22 @@ static int xlnx_rtc_set_offset(struct device *dev, long offset)
 
 	/* Number fractional ticks for given offset */
 	if (fract_offset) {
+		/* round up here so we stay below a full tick */
+		fract_part = DIV_ROUND_UP(tick_mult, RTC_FR_MAX_TICKS);
 		if (fract_offset < 0) {
-			fract_offset = fract_offset + tick_mult;
+			fract_offset += (fract_part * RTC_FR_MAX_TICKS);
 			max_tick--;
 		}
-		if (fract_offset > (tick_mult / RTC_FR_MAX_TICKS)) {
-			for (fract_tick = 1; fract_tick < 16; fract_tick++) {
-				if (fract_offset <=
-				    (fract_tick *
-				     (tick_mult / RTC_FR_MAX_TICKS)))
-					break;
-			}
-		}
+		fract_tick = fract_offset / fract_part;
 	}
 
 	/* Zynqmp RTC uses second and fractional tick
 	 * counters for compensation
 	 */
-	calibval = max_tick + RTC_CALIB_DEF;
+	calibval = max_tick + freq;
 
 	if (fract_tick)
-		calibval |= RTC_FR_EN;
-
-	calibval |= (fract_tick << RTC_FR_DATSHIFT);
+		calibval |= (RTC_FR_EN | (fract_tick << RTC_FR_DATSHIFT));
 
 	writel(calibval, (xrtcdev->reg_base + RTC_CALIB_WR));
 

-- 
2.47.3

[PATCH 4/4] rtc: zynqmp: use dynamic max and min offset ranges

[Tomas Melin]

Maximum and minimum offsets in ppb that can be handled are dependent on
the rtc clock frequency and what can fit in the 16-bit register field.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/rtc/rtc-zynqmp.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
index 3bc8831ba2c4c4c701a49506b67ae6174f3ade3d..0cebc99b15a6de2440a60afc2bd1769eccfa84b3 100644
--- a/drivers/rtc/rtc-zynqmp.c
+++ b/drivers/rtc/rtc-zynqmp.c
@@ -44,8 +44,6 @@
 #define RTC_FR_MASK		0xF0000
 #define RTC_FR_MAX_TICKS	16
 #define RTC_PPB			1000000000LL
-#define RTC_MIN_OFFSET		-32768000
-#define RTC_MAX_OFFSET		32767000
 
 struct xlnx_rtc_dev {
 	struct rtc_device	*rtc;
@@ -215,12 +213,12 @@ static int xlnx_rtc_set_offset(struct device *dev, long offset)
 
 	/* ticks to reach RTC_PPB */
 	tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, xrtcdev->freq);
-	if (offset < RTC_MIN_OFFSET || offset > RTC_MAX_OFFSET)
-		return -ERANGE;
-
 	/* Number ticks for given offset */
 	max_tick = div_s64_rem(offset, tick_mult, &fract_offset);
 
+	if (freq + max_tick > RTC_TICK_MASK || (freq + max_tick < 1))
+		return -ERANGE;
+
 	/* Number fractional ticks for given offset */
 	if (fract_offset) {
 		/* round up here so we stay below a full tick */

-- 
2.47.3

RE: [PATCH 1/4] rtc: zynqmp: correct frequency value

[T, Harini]

[Public]

Hi,

> -----Original Message-----
> From: Tomas Melin <tomas.melin@vaisala.com>
> Sent: Monday, December 1, 2025 6:20 PM
> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
> <michal.simek@amd.com>
> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
> Subject: [PATCH 1/4] rtc: zynqmp: correct frequency value
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Fix calibration value in case a clock reference is provided.
> The actual calibration value written into register is
> frequency - 1.
>
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/rtc/rtc-zynqmp.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
> index
> 3baa2b481d9f2008750046005283b98a0d546c5c..856bc1678e7d31144f320ae
> 9f75fc58c742a2a64 100644
> --- a/drivers/rtc/rtc-zynqmp.c
> +++ b/drivers/rtc/rtc-zynqmp.c
> @@ -345,7 +345,10 @@ static int xlnx_rtc_probe(struct platform_device
> *pdev)
>                                            &xrtcdev->freq);
>                 if (ret)
>                         xrtcdev->freq = RTC_CALIB_DEF;
> +       } else {
> +               xrtcdev->freq--;
If freq > 65536, the 16-bit register silently truncates.
Please add some checks for above mentioned scenario.
>         }
> +
>         ret = readl(xrtcdev->reg_base + RTC_CALIB_RD);
>         if (!ret)
>                 writel(xrtcdev->freq, (xrtcdev->reg_base + RTC_CALIB_WR));
>
> --
> 2.47.3
>
Thanks,
Harini T

RE: [PATCH 2/4] rtc: zynqmp: rework read_offset

[T, Harini]

[Public]

Hi,

> -----Original Message-----
> From: Tomas Melin <tomas.melin@vaisala.com>
> Sent: Monday, December 1, 2025 6:20 PM
> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
> <michal.simek@amd.com>
> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
> Subject: [PATCH 2/4] rtc: zynqmp: rework read_offset
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> read_offset() was using static frequency for determining the tick offset. It was
> also using remainder from do_div() operation as tick_mult value which
> caused the offset to be incorrect.
>
> At the same time, rework function to improve readability.
>
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/rtc/rtc-zynqmp.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c index
> 856bc1678e7d31144f320ae9f75fc58c742a2a64..7af5f6f99538f961a53ff56bfc6
> 56c907611b900 100644
> --- a/drivers/rtc/rtc-zynqmp.c
> +++ b/drivers/rtc/rtc-zynqmp.c
> @@ -178,21 +178,28 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev
> *xrtcdev)  static int xlnx_rtc_read_offset(struct device *dev, long *offset)  {
>         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> -       unsigned long long rtc_ppb = RTC_PPB;
> -       unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
> -       unsigned int calibval;
> +       unsigned int calibval, fract_data, fract_part;
Prefer one variable assignment per line for readability.
> +       int max_tick, tick_mult;
It would be better to explain why tick_mult is changed to int in the commit message.
> +       int freq = xrtcdev->freq;
Please follow reverse xmas tree variable ordering.
>         long offset_val;
>
> +       /* ticks to reach RTC_PPB */
The comment is misleading. Its tick_mult is nanoseconds per tick, not a tick count.
> +       tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, freq);
> +
>         calibval = readl(xrtcdev->reg_base + RTC_CALIB_RD);
>         /* Offset with seconds ticks */
> -       offset_val = calibval & RTC_TICK_MASK;
> -       offset_val = offset_val - RTC_CALIB_DEF;
> -       offset_val = offset_val * tick_mult;
> +       max_tick = calibval & RTC_TICK_MASK;
> +       offset_val = max_tick - freq;
> +       /* Convert to ppb */
> +       offset_val *= tick_mult;
>
>         /* Offset with fractional ticks */
> -       if (calibval & RTC_FR_EN)
> -               offset_val += ((calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT)
> -                       * (tick_mult / RTC_FR_MAX_TICKS);
> +       if (calibval & RTC_FR_EN) {
> +               fract_data = (calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT;
> +               fract_part = DIV_ROUND_UP(tick_mult, RTC_FR_MAX_TICKS);
> +               offset_val += (fract_part * fract_data);
> +       }
> +
>         *offset = offset_val;
>
>         return 0;
>
> --
> 2.47.3
>
Regards,
Harini T

RE: [PATCH 3/4] rtc: zynqmp: rework set_offset

[T, Harini]

[Public]

Hi,

> -----Original Message-----
> From: Tomas Melin <tomas.melin@vaisala.com>
> Sent: Monday, December 1, 2025 6:20 PM
> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
> <michal.simek@amd.com>
> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
> Subject: [PATCH 3/4] rtc: zynqmp: rework set_offset
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> set_offset was using remainder of do_div as tick_mult which resulted in
> wrong offset. Calibration value also assumed builtin calibration default.
> Update fract_offset to correctly calculate the value for negative offset and
> replace the for loop with division.
>
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/rtc/rtc-zynqmp.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c index
> 7af5f6f99538f961a53ff56bfc656c907611b900..3bc8831ba2c4c4c701a49506b6
> 7ae6174f3ade3d 100644
> --- a/drivers/rtc/rtc-zynqmp.c
> +++ b/drivers/rtc/rtc-zynqmp.c
> @@ -208,13 +208,13 @@ static int xlnx_rtc_read_offset(struct device *dev,
> long *offset)  static int xlnx_rtc_set_offset(struct device *dev, long offset)  {
>         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> -       unsigned long long rtc_ppb = RTC_PPB;
> -       unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
> +       unsigned int calibval, tick_mult, fract_part;
tick_mult is mentioned as int in previous patch and unsigned here. Justify the type in commit description.
>         unsigned char fract_tick = 0;
> -       unsigned int calibval;
> -       short int  max_tick;
> -       int fract_offset;
> +       int freq = xrtcdev->freq;
> +       int max_tick, fract_offset;
Please follow reverse xmas tree variable ordering.
Also keep the frac_* variables uniform in both set and read offset functions.
>
> +       /* ticks to reach RTC_PPB */
The comment is misleading. Its tick_mult is nanoseconds per tick, not a tick count.
> +       tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, xrtcdev->freq);
We can first validate offset and then calculate tick_mult to reduce CPU instructions incase of invalid inputs
>         if (offset < RTC_MIN_OFFSET || offset > RTC_MAX_OFFSET)
>                 return -ERANGE;
>
> @@ -223,29 +223,22 @@ static int xlnx_rtc_set_offset(struct device *dev,
> long offset)
>
>         /* Number fractional ticks for given offset */
>         if (fract_offset) {
> +               /* round up here so we stay below a full tick */
> +               fract_part = DIV_ROUND_UP(tick_mult, RTC_FR_MAX_TICKS);
>                 if (fract_offset < 0) {
> -                       fract_offset = fract_offset + tick_mult;
> +                       fract_offset += (fract_part * RTC_FR_MAX_TICKS);
It would be better to add comment to explain on the negative offset borrowing logic
>                         max_tick--;
>                 }
> -               if (fract_offset > (tick_mult / RTC_FR_MAX_TICKS)) {
> -                       for (fract_tick = 1; fract_tick < 16; fract_tick++) {
> -                               if (fract_offset <=
> -                                   (fract_tick *
> -                                    (tick_mult / RTC_FR_MAX_TICKS)))
> -                                       break;
> -                       }
> -               }
> +               fract_tick = fract_offset / fract_part;
Its better to use DIV_ROUND_UP()
>         }
>
>         /* Zynqmp RTC uses second and fractional tick
>          * counters for compensation
>          */
> -       calibval = max_tick + RTC_CALIB_DEF;
> +       calibval = max_tick + freq;
>
>         if (fract_tick)
> -               calibval |= RTC_FR_EN;
> -
> -       calibval |= (fract_tick << RTC_FR_DATSHIFT);
> +               calibval |= (RTC_FR_EN | (fract_tick <<
> + RTC_FR_DATSHIFT));
>
>         writel(calibval, (xrtcdev->reg_base + RTC_CALIB_WR));
>
>
> --
> 2.47.3
>

Regards,
Harini T

RE: [PATCH 4/4] rtc: zynqmp: use dynamic max and min offset ranges

[T, Harini]

[Public]

Hi,

> -----Original Message-----
> From: Tomas Melin <tomas.melin@vaisala.com>
> Sent: Monday, December 1, 2025 6:20 PM
> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
> <michal.simek@amd.com>
> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
> Subject: [PATCH 4/4] rtc: zynqmp: use dynamic max and min offset ranges
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Maximum and minimum offsets in ppb that can be handled are dependent
> on the rtc clock frequency and what can fit in the 16-bit register field.
>
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/rtc/rtc-zynqmp.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c index
> 3bc8831ba2c4c4c701a49506b67ae6174f3ade3d..0cebc99b15a6de2440a60afc
> 2bd1769eccfa84b3 100644
> --- a/drivers/rtc/rtc-zynqmp.c
> +++ b/drivers/rtc/rtc-zynqmp.c
> @@ -44,8 +44,6 @@
>  #define RTC_FR_MASK            0xF0000
>  #define RTC_FR_MAX_TICKS       16
>  #define RTC_PPB                        1000000000LL
> -#define RTC_MIN_OFFSET         -32768000
> -#define RTC_MAX_OFFSET         32767000
>
>  struct xlnx_rtc_dev {
>         struct rtc_device       *rtc;
> @@ -215,12 +213,12 @@ static int xlnx_rtc_set_offset(struct device *dev,
> long offset)
>
>         /* ticks to reach RTC_PPB */
>         tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, xrtcdev->freq);
> -       if (offset < RTC_MIN_OFFSET || offset > RTC_MAX_OFFSET)
> -               return -ERANGE;
> -
>         /* Number ticks for given offset */
>         max_tick = div_s64_rem(offset, tick_mult, &fract_offset);
>
> +       if (freq + max_tick > RTC_TICK_MASK || (freq + max_tick < 1))
The check 'freq + max_tick < 1' should be '<2' to prevent writing 0 to the calibration register when fract_offset < 0 causes max_tick--.
Example: freq=32767, max_tick=-32766 passes (sum=1), but after decrement becomes calibval=0.
> +               return -ERANGE;
> +
>         /* Number fractional ticks for given offset */
>         if (fract_offset) {
>                 /* round up here so we stay below a full tick */
>
> --
> 2.47.3
>

Thanks,
Harini T

Re: [PATCH 2/4] rtc: zynqmp: rework read_offset

[Tomas Melin]

Hi,

On 09/12/2025 19:28, T, Harini wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Tomas Melin <tomas.melin@vaisala.com>
>> Sent: Monday, December 1, 2025 6:20 PM
>> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
>> <michal.simek@amd.com>
>> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
>> Subject: [PATCH 2/4] rtc: zynqmp: rework read_offset
>>
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> read_offset() was using static frequency for determining the tick offset. It was
>> also using remainder from do_div() operation as tick_mult value which
>> caused the offset to be incorrect.
>>
>> At the same time, rework function to improve readability.
>>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>> ---
>>  drivers/rtc/rtc-zynqmp.c | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c index
>> 856bc1678e7d31144f320ae9f75fc58c742a2a64..7af5f6f99538f961a53ff56bfc6
>> 56c907611b900 100644
>> --- a/drivers/rtc/rtc-zynqmp.c
>> +++ b/drivers/rtc/rtc-zynqmp.c
>> @@ -178,21 +178,28 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev
>> *xrtcdev)  static int xlnx_rtc_read_offset(struct device *dev, long *offset)  {
>>         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>> -       unsigned long long rtc_ppb = RTC_PPB;
>> -       unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
>> -       unsigned int calibval;
>> +       unsigned int calibval, fract_data, fract_part;
> Prefer one variable assignment per line for readability.
This is after all quite common practice, and in a function like this
where several variables are needed, I would argue that this is more
readable than the alternative. Is there some convention I'm not aware of?

>> +       int max_tick, tick_mult;
> It would be better to explain why tick_mult is changed to int in the commit message.
This is part of the refactoring, mixing signed and unsigned variables in
operations is more risky than having same type.

>> +       int freq = xrtcdev->freq;
> Please follow reverse xmas tree variable ordering.
Ok fixing this and other occurances.

>>         long offset_val;
>>
>> +       /* ticks to reach RTC_PPB */
> The comment is misleading. Its tick_mult is nanoseconds per tick, not a tick count.
Perhaps the comment was not well formulated. I suggest changing to
/* Tick to offset multiplier */
 as that it what it is primarily used for. Would that be okay for You?

Thanks,
Tomas

>> +       tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, freq);
>> +
>>         calibval = readl(xrtcdev->reg_base + RTC_CALIB_RD);
>>         /* Offset with seconds ticks */
>> -       offset_val = calibval & RTC_TICK_MASK;
>> -       offset_val = offset_val - RTC_CALIB_DEF;
>> -       offset_val = offset_val * tick_mult;
>> +       max_tick = calibval & RTC_TICK_MASK;
>> +       offset_val = max_tick - freq;
>> +       /* Convert to ppb */
>> +       offset_val *= tick_mult;
>>
>>         /* Offset with fractional ticks */
>> -       if (calibval & RTC_FR_EN)
>> -               offset_val += ((calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT)
>> -                       * (tick_mult / RTC_FR_MAX_TICKS);
>> +       if (calibval & RTC_FR_EN) {
>> +               fract_data = (calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT;
>> +               fract_part = DIV_ROUND_UP(tick_mult, RTC_FR_MAX_TICKS);
>> +               offset_val += (fract_part * fract_data);
>> +       }
>> +
>>         *offset = offset_val;
>>
>>         return 0;
>>
>> --
>> 2.47.3
>>
> Regards,
> Harini T
> 

Re: [PATCH 3/4] rtc: zynqmp: rework set_offset

[Tomas Melin]

Hi,

On 09/12/2025 21:03, T, Harini wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Tomas Melin <tomas.melin@vaisala.com>
>> Sent: Monday, December 1, 2025 6:20 PM
>> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
>> <michal.simek@amd.com>
>> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
>> Subject: [PATCH 3/4] rtc: zynqmp: rework set_offset
>>
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> set_offset was using remainder of do_div as tick_mult which resulted in
>> wrong offset. Calibration value also assumed builtin calibration default.
>> Update fract_offset to correctly calculate the value for negative offset and
>> replace the for loop with division.
>>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>> ---
>>  drivers/rtc/rtc-zynqmp.c | 29 +++++++++++------------------
>>  1 file changed, 11 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c index
>> 7af5f6f99538f961a53ff56bfc656c907611b900..3bc8831ba2c4c4c701a49506b6
>> 7ae6174f3ade3d 100644
>> --- a/drivers/rtc/rtc-zynqmp.c
>> +++ b/drivers/rtc/rtc-zynqmp.c
>> @@ -208,13 +208,13 @@ static int xlnx_rtc_read_offset(struct device *dev,
>> long *offset)  static int xlnx_rtc_set_offset(struct device *dev, long offset)  {
>>         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>> -       unsigned long long rtc_ppb = RTC_PPB;
>> -       unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
>> +       unsigned int calibval, tick_mult, fract_part;
> tick_mult is mentioned as int in previous patch and unsigned here. Justify the type in commit description.
>>         unsigned char fract_tick = 0;
>> -       unsigned int calibval;
>> -       short int  max_tick;
>> -       int fract_offset;
>> +       int freq = xrtcdev->freq;
>> +       int max_tick, fract_offset;
> Please follow reverse xmas tree variable ordering.
> Also keep the frac_* variables uniform in both set and read offset functions.
Agreed, I will use same name of variables and types in next version.

>>
>> +       /* ticks to reach RTC_PPB */
> The comment is misleading. Its tick_mult is nanoseconds per tick, not a tick count.
Answered in patch 2/4.
>> +       tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, xrtcdev->freq);
> We can first validate offset and then calculate tick_mult to reduce CPU instructions incase of invalid inputs
In this patch it would in theory apply, but when looking at patch 4/4
You will notice that we need to first calculate the helpers so offset is
then performed as soon as possible.

>>         if (offset < RTC_MIN_OFFSET || offset > RTC_MAX_OFFSET)
>>                 return -ERANGE;
>>
>> @@ -223,29 +223,22 @@ static int xlnx_rtc_set_offset(struct device *dev,
>> long offset)
>>
>>         /* Number fractional ticks for given offset */
>>         if (fract_offset) {
>> +               /* round up here so we stay below a full tick */
>> +               fract_part = DIV_ROUND_UP(tick_mult, RTC_FR_MAX_TICKS);
>>                 if (fract_offset < 0) {
>> -                       fract_offset = fract_offset + tick_mult;
>> +                       fract_offset += (fract_part * RTC_FR_MAX_TICKS);
> It would be better to add comment to explain on the negative offset borrowing logic
I will add comment about this.


>>                         max_tick--;
>>                 }
>> -               if (fract_offset > (tick_mult / RTC_FR_MAX_TICKS)) {
>> -                       for (fract_tick = 1; fract_tick < 16; fract_tick++) {
>> -                               if (fract_offset <=
>> -                                   (fract_tick *
>> -                                    (tick_mult / RTC_FR_MAX_TICKS)))
>> -                                       break;
>> -                       }
>> -               }
>> +               fract_tick = fract_offset / fract_part;
> Its better to use DIV_ROUND_UP()
Please explain why, that would change the end result from what is is now.

Thanks,
Tomas

>>         }
>>
>>         /* Zynqmp RTC uses second and fractional tick
>>          * counters for compensation
>>          */
>> -       calibval = max_tick + RTC_CALIB_DEF;
>> +       calibval = max_tick + freq;
>>
>>         if (fract_tick)
>> -               calibval |= RTC_FR_EN;
>> -
>> -       calibval |= (fract_tick << RTC_FR_DATSHIFT);
>> +               calibval |= (RTC_FR_EN | (fract_tick <<
>> + RTC_FR_DATSHIFT));
>>
>>         writel(calibval, (xrtcdev->reg_base + RTC_CALIB_WR));
>>
>>
>> --
>> 2.47.3
>>
> 
> Regards,
> Harini T

Re: [PATCH 4/4] rtc: zynqmp: use dynamic max and min offset ranges

[Tomas Melin]

Hi,

On 09/12/2025 21:28, T, Harini wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Tomas Melin <tomas.melin@vaisala.com>
>> Sent: Monday, December 1, 2025 6:20 PM
>> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
>> <michal.simek@amd.com>
>> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
>> Subject: [PATCH 4/4] rtc: zynqmp: use dynamic max and min offset ranges
>>
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> Maximum and minimum offsets in ppb that can be handled are dependent
>> on the rtc clock frequency and what can fit in the 16-bit register field.
>>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>> ---
>>  drivers/rtc/rtc-zynqmp.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c index
>> 3bc8831ba2c4c4c701a49506b67ae6174f3ade3d..0cebc99b15a6de2440a60afc
>> 2bd1769eccfa84b3 100644
>> --- a/drivers/rtc/rtc-zynqmp.c
>> +++ b/drivers/rtc/rtc-zynqmp.c
>> @@ -44,8 +44,6 @@
>>  #define RTC_FR_MASK            0xF0000
>>  #define RTC_FR_MAX_TICKS       16
>>  #define RTC_PPB                        1000000000LL
>> -#define RTC_MIN_OFFSET         -32768000
>> -#define RTC_MAX_OFFSET         32767000
>>
>>  struct xlnx_rtc_dev {
>>         struct rtc_device       *rtc;
>> @@ -215,12 +213,12 @@ static int xlnx_rtc_set_offset(struct device *dev,
>> long offset)
>>
>>         /* ticks to reach RTC_PPB */
>>         tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, xrtcdev->freq);
>> -       if (offset < RTC_MIN_OFFSET || offset > RTC_MAX_OFFSET)
>> -               return -ERANGE;
>> -
>>         /* Number ticks for given offset */
>>         max_tick = div_s64_rem(offset, tick_mult, &fract_offset);
>>
>> +       if (freq + max_tick > RTC_TICK_MASK || (freq + max_tick < 1))
> The check 'freq + max_tick < 1' should be '<2' to prevent writing 0 to the calibration register when fract_offset < 0 causes max_tick--.
> Example: freq=32767, max_tick=-32766 passes (sum=1), but after decrement becomes calibval=0.
calibval=0 is not documented as invalid calibration value. AFAIS it
would mean a frequency of 1Hz. Can You provide more info on this?

Thanks,
Tomas


>> +               return -ERANGE;
>> +
>>         /* Number fractional ticks for given offset */
>>         if (fract_offset) {
>>                 /* round up here so we stay below a full tick */
>>
>> --
>> 2.47.3
>>
> 
> Thanks,
> Harini T
> 

Re: [PATCH 1/4] rtc: zynqmp: correct frequency value

[Tomas Melin]

Hi,

On 09/12/2025 18:51, T, Harini wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Tomas Melin <tomas.melin@vaisala.com>
>> Sent: Monday, December 1, 2025 6:20 PM
>> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
>> <michal.simek@amd.com>
>> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
>> Subject: [PATCH 1/4] rtc: zynqmp: correct frequency value
>>
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> Fix calibration value in case a clock reference is provided.
>> The actual calibration value written into register is
>> frequency - 1.
>>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>> ---
>>  drivers/rtc/rtc-zynqmp.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
>> index
>> 3baa2b481d9f2008750046005283b98a0d546c5c..856bc1678e7d31144f320ae
>> 9f75fc58c742a2a64 100644
>> --- a/drivers/rtc/rtc-zynqmp.c
>> +++ b/drivers/rtc/rtc-zynqmp.c
>> @@ -345,7 +345,10 @@ static int xlnx_rtc_probe(struct platform_device
>> *pdev)
>>                                            &xrtcdev->freq);
>>                 if (ret)
>>                         xrtcdev->freq = RTC_CALIB_DEF;
>> +       } else {
>> +               xrtcdev->freq--;
> If freq > 65536, the 16-bit register silently truncates.
> Please add some checks for above mentioned scenario.
That is indeed a scenario that is not accounted for in the current
driver. I can add a separate patch for that as part of this series.

Thanks,
Tomas


>>         }
>> +
>>         ret = readl(xrtcdev->reg_base + RTC_CALIB_RD);
>>         if (!ret)
>>                 writel(xrtcdev->freq, (xrtcdev->reg_base + RTC_CALIB_WR));
>>
>> --
>> 2.47.3
>>
> Thanks,
> Harini T

RE: [PATCH 2/4] rtc: zynqmp: rework read_offset

[T, Harini]

[Public]

Hi,

> -----Original Message-----
> From: Tomas Melin <tomas.melin@vaisala.com>
> Sent: Wednesday, December 10, 2025 5:35 PM
> To: T, Harini <Harini.T@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Simek, Michal <michal.simek@amd.com>
> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/4] rtc: zynqmp: rework read_offset
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi,
>
> On 09/12/2025 19:28, T, Harini wrote:
> > [Public]
> >
> > Hi,
> >
> >> -----Original Message-----
> >> From: Tomas Melin <tomas.melin@vaisala.com>
> >> Sent: Monday, December 1, 2025 6:20 PM
> >> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
> >> <michal.simek@amd.com>
> >> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux- kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
> >> Subject: [PATCH 2/4] rtc: zynqmp: rework read_offset
> >>
> >> Caution: This message originated from an External Source. Use proper
> >> caution when opening attachments, clicking links, or responding.
> >>
> >>
> >> read_offset() was using static frequency for determining the tick
> >> offset. It was also using remainder from do_div() operation as
> >> tick_mult value which caused the offset to be incorrect.
> >>
> >> At the same time, rework function to improve readability.
> >>
> >> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> >> ---
> >>  drivers/rtc/rtc-zynqmp.c | 25 ++++++++++++++++---------
> >>  1 file changed, 16 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
> >> index
> >>
> 856bc1678e7d31144f320ae9f75fc58c742a2a64..7af5f6f99538f961a53ff56
> bfc6
> >> 56c907611b900 100644
> >> --- a/drivers/rtc/rtc-zynqmp.c
> >> +++ b/drivers/rtc/rtc-zynqmp.c
> >> @@ -178,21 +178,28 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev
> >> *xrtcdev)  static int xlnx_rtc_read_offset(struct device *dev, long *offset)  {
> >>         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> >> -       unsigned long long rtc_ppb = RTC_PPB;
> >> -       unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
> >> -       unsigned int calibval;
> >> +       unsigned int calibval, fract_data, fract_part;
> > Prefer one variable assignment per line for readability.
> This is after all quite common practice, and in a function like this where several
> variables are needed, I would argue that this is more readable than the
> alternative. Is there some convention I'm not aware of?
There is no such mandatory convention. It's up to the RTC maintainer.
>
> >> +       int max_tick, tick_mult;
> > It would be better to explain why tick_mult is changed to int in the commit
> message.
> This is part of the refactoring, mixing signed and unsigned variables in
> operations is more risky than having same type.
I agree. But tick_mult is int in read_offset and unsigned in set_offset.
It would be better if both uses int to maintain consistency during the math operations.
>
> >> +       int freq = xrtcdev->freq;
> > Please follow reverse xmas tree variable ordering.
> Ok fixing this and other occurances.
>
> >>         long offset_val;
> >>
> >> +       /* ticks to reach RTC_PPB */
> > The comment is misleading. Its tick_mult is nanoseconds per tick, not a tick
> count.
> Perhaps the comment was not well formulated. I suggest changing to
> /* Tick to offset multiplier */
>  as that it what it is primarily used for. Would that be okay for You?
Yeah
>
> Thanks,
> Tomas
>
> >> +       tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, freq);
> >> +
> >>         calibval = readl(xrtcdev->reg_base + RTC_CALIB_RD);
> >>         /* Offset with seconds ticks */
> >> -       offset_val = calibval & RTC_TICK_MASK;
> >> -       offset_val = offset_val - RTC_CALIB_DEF;
> >> -       offset_val = offset_val * tick_mult;
> >> +       max_tick = calibval & RTC_TICK_MASK;
> >> +       offset_val = max_tick - freq;
> >> +       /* Convert to ppb */
> >> +       offset_val *= tick_mult;
> >>
> >>         /* Offset with fractional ticks */
> >> -       if (calibval & RTC_FR_EN)
> >> -               offset_val += ((calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT)
> >> -                       * (tick_mult / RTC_FR_MAX_TICKS);
> >> +       if (calibval & RTC_FR_EN) {
> >> +               fract_data = (calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT;
> >> +               fract_part = DIV_ROUND_UP(tick_mult, RTC_FR_MAX_TICKS);
> >> +               offset_val += (fract_part * fract_data);
> >> +       }
> >> +
> >>         *offset = offset_val;
> >>
> >>         return 0;
> >>
> >> --
> >> 2.47.3
> >>
> > Regards,
> > Harini T
> >
>

Regards,
Harini T

RE: [PATCH 3/4] rtc: zynqmp: rework set_offset

[T, Harini]

[Public]

Hi,

> -----Original Message-----
> From: Tomas Melin <tomas.melin@vaisala.com>
> Sent: Wednesday, December 10, 2025 5:48 PM
> To: T, Harini <Harini.T@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Simek, Michal <michal.simek@amd.com>
> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] rtc: zynqmp: rework set_offset
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi,
>
> On 09/12/2025 21:03, T, Harini wrote:
> > [Public]
> >
> > Hi,
> >
> >> -----Original Message-----
> >> From: Tomas Melin <tomas.melin@vaisala.com>
> >> Sent: Monday, December 1, 2025 6:20 PM
> >> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
> >> <michal.simek@amd.com>
> >> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux- kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
> >> Subject: [PATCH 3/4] rtc: zynqmp: rework set_offset
> >>
> >> Caution: This message originated from an External Source. Use proper
> >> caution when opening attachments, clicking links, or responding.
> >>
> >>
> >> set_offset was using remainder of do_div as tick_mult which resulted
> >> in wrong offset. Calibration value also assumed builtin calibration default.
> >> Update fract_offset to correctly calculate the value for negative
> >> offset and replace the for loop with division.
> >>
> >> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> >> ---
> >>  drivers/rtc/rtc-zynqmp.c | 29 +++++++++++------------------
> >>  1 file changed, 11 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
> >> index
> >>
> 7af5f6f99538f961a53ff56bfc656c907611b900..3bc8831ba2c4c4c701a495
> 06b6
> >> 7ae6174f3ade3d 100644
> >> --- a/drivers/rtc/rtc-zynqmp.c
> >> +++ b/drivers/rtc/rtc-zynqmp.c
> >> @@ -208,13 +208,13 @@ static int xlnx_rtc_read_offset(struct device
> >> *dev, long *offset)  static int xlnx_rtc_set_offset(struct device *dev, long
> offset)  {
> >>         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> >> -       unsigned long long rtc_ppb = RTC_PPB;
> >> -       unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
> >> +       unsigned int calibval, tick_mult, fract_part;
> > tick_mult is mentioned as int in previous patch and unsigned here. Justify
> the type in commit description.
> >>         unsigned char fract_tick = 0;
> >> -       unsigned int calibval;
> >> -       short int  max_tick;
> >> -       int fract_offset;
> >> +       int freq = xrtcdev->freq;
> >> +       int max_tick, fract_offset;
> > Please follow reverse xmas tree variable ordering.
> > Also keep the frac_* variables uniform in both set and read offset functions.
> Agreed, I will use same name of variables and types in next version.
>
> >>
> >> +       /* ticks to reach RTC_PPB */
> > The comment is misleading. Its tick_mult is nanoseconds per tick, not a tick
> count.
> Answered in patch 2/4.
> >> +       tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, xrtcdev->freq);
> > We can first validate offset and then calculate tick_mult to reduce
> > CPU instructions incase of invalid inputs
> In this patch it would in theory apply, but when looking at patch 4/4 You will
> notice that we need to first calculate the helpers so offset is then performed as
> soon as possible.
>
> >>         if (offset < RTC_MIN_OFFSET || offset > RTC_MAX_OFFSET)
> >>                 return -ERANGE;
> >>
> >> @@ -223,29 +223,22 @@ static int xlnx_rtc_set_offset(struct device
> >> *dev, long offset)
> >>
> >>         /* Number fractional ticks for given offset */
> >>         if (fract_offset) {
> >> +               /* round up here so we stay below a full tick */
> >> +               fract_part = DIV_ROUND_UP(tick_mult,
> >> + RTC_FR_MAX_TICKS);
> >>                 if (fract_offset < 0) {
> >> -                       fract_offset = fract_offset + tick_mult;
> >> +                       fract_offset += (fract_part *
> >> + RTC_FR_MAX_TICKS);
> > It would be better to add comment to explain on the negative offset
> > borrowing logic
> I will add comment about this.
>
>
> >>                         max_tick--;
> >>                 }
> >> -               if (fract_offset > (tick_mult / RTC_FR_MAX_TICKS)) {
> >> -                       for (fract_tick = 1; fract_tick < 16; fract_tick++) {
> >> -                               if (fract_offset <=
> >> -                                   (fract_tick *
> >> -                                    (tick_mult / RTC_FR_MAX_TICKS)))
> >> -                                       break;
> >> -                       }
> >> -               }
> >> +               fract_tick = fract_offset / fract_part;
> > Its better to use DIV_ROUND_UP()
> Please explain why, that would change the end result from what is is now.
The truncating division is acceptable as-is. Given the hardware's discrete
4-bit fractional field (16 levels), the quantization error from truncation
is negligible compared to the fundamental hardware precision limit. If you
want to optimize, DIV_ROUND_CLOSEST() would minimize average error, but it's
not required for correctness.
>
> Thanks,
> Tomas
>
> >>         }
> >>
> >>         /* Zynqmp RTC uses second and fractional tick
> >>          * counters for compensation
> >>          */
> >> -       calibval = max_tick + RTC_CALIB_DEF;
> >> +       calibval = max_tick + freq;
> >>
> >>         if (fract_tick)
> >> -               calibval |= RTC_FR_EN;
> >> -
> >> -       calibval |= (fract_tick << RTC_FR_DATSHIFT);
> >> +               calibval |= (RTC_FR_EN | (fract_tick <<
> >> + RTC_FR_DATSHIFT));
> >>
> >>         writel(calibval, (xrtcdev->reg_base + RTC_CALIB_WR));
> >>
> >>
> >> --
> >> 2.47.3
> >>
> >
> > Regards,
> > Harini T

Regards,
Harini T

RE: [PATCH 4/4] rtc: zynqmp: use dynamic max and min offset ranges

[T, Harini]

[Public]

Hi,

> -----Original Message-----
> From: Tomas Melin <tomas.melin@vaisala.com>
> Sent: Wednesday, December 10, 2025 5:55 PM
> To: T, Harini <Harini.T@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Simek, Michal <michal.simek@amd.com>
> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 4/4] rtc: zynqmp: use dynamic max and min offset ranges
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi,
>
> On 09/12/2025 21:28, T, Harini wrote:
> > [Public]
> >
> > Hi,
> >
> >> -----Original Message-----
> >> From: Tomas Melin <tomas.melin@vaisala.com>
> >> Sent: Monday, December 1, 2025 6:20 PM
> >> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
> >> <michal.simek@amd.com>
> >> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux- kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
> >> Subject: [PATCH 4/4] rtc: zynqmp: use dynamic max and min offset
> >> ranges
> >>
> >> Caution: This message originated from an External Source. Use proper
> >> caution when opening attachments, clicking links, or responding.
> >>
> >>
> >> Maximum and minimum offsets in ppb that can be handled are dependent
> >> on the rtc clock frequency and what can fit in the 16-bit register field.
> >>
> >> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> >> ---
> >>  drivers/rtc/rtc-zynqmp.c | 8 +++-----
> >>  1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
> >> index
> >>
> 3bc8831ba2c4c4c701a49506b67ae6174f3ade3d..0cebc99b15a6de2440a6
> 0afc
> >> 2bd1769eccfa84b3 100644
> >> --- a/drivers/rtc/rtc-zynqmp.c
> >> +++ b/drivers/rtc/rtc-zynqmp.c
> >> @@ -44,8 +44,6 @@
> >>  #define RTC_FR_MASK            0xF0000
> >>  #define RTC_FR_MAX_TICKS       16
> >>  #define RTC_PPB                        1000000000LL
> >> -#define RTC_MIN_OFFSET         -32768000
> >> -#define RTC_MAX_OFFSET         32767000
> >>
> >>  struct xlnx_rtc_dev {
> >>         struct rtc_device       *rtc;
> >> @@ -215,12 +213,12 @@ static int xlnx_rtc_set_offset(struct device
> >> *dev, long offset)
> >>
> >>         /* ticks to reach RTC_PPB */
> >>         tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, xrtcdev->freq);
> >> -       if (offset < RTC_MIN_OFFSET || offset > RTC_MAX_OFFSET)
> >> -               return -ERANGE;
> >> -
> >>         /* Number ticks for given offset */
> >>         max_tick = div_s64_rem(offset, tick_mult, &fract_offset);
> >>
> >> +       if (freq + max_tick > RTC_TICK_MASK || (freq + max_tick < 1))
> > The check 'freq + max_tick < 1' should be '<2' to prevent writing 0 to the
> calibration register when fract_offset < 0 causes max_tick--.
> > Example: freq=32767, max_tick=-32766 passes (sum=1), but after
> decrement becomes calibval=0.
> calibval=0 is not documented as invalid calibration value. AFAIS it would mean
> a frequency of 1Hz. Can You provide more info on this?
>
You're right - calibval=0 is hardware-spec compliant and produces 1Hz
operation. The patch is correct as-is with '< 1'.
There's no issue with allowing calibval=0 from a hardware perspective.
> Thanks,
> Tomas
>
>
> >> +               return -ERANGE;
> >> +
> >>         /* Number fractional ticks for given offset */
> >>         if (fract_offset) {
> >>                 /* round up here so we stay below a full tick */
> >>
> >> --
> >> 2.47.3
> >>
> >
> > Thanks,
> > Harini T
> >
Regards,
Harini T

Re: [PATCH 2/4] rtc: zynqmp: rework read_offset

[Alexandre Belloni]

On 17/12/2025 18:14:30+0000, T, Harini wrote:
> [Public]
> 
> Hi,
> 
> > -----Original Message-----
> > From: Tomas Melin <tomas.melin@vaisala.com>
> > Sent: Wednesday, December 10, 2025 5:35 PM
> > To: T, Harini <Harini.T@amd.com>; Alexandre Belloni
> > <alexandre.belloni@bootlin.com>; Simek, Michal <michal.simek@amd.com>
> > Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/4] rtc: zynqmp: rework read_offset
> >
> > Caution: This message originated from an External Source. Use proper caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > Hi,
> >
> > On 09/12/2025 19:28, T, Harini wrote:
> > > [Public]
> > >
> > > Hi,
> > >
> > >> -----Original Message-----
> > >> From: Tomas Melin <tomas.melin@vaisala.com>
> > >> Sent: Monday, December 1, 2025 6:20 PM
> > >> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
> > >> <michal.simek@amd.com>
> > >> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > >> linux- kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
> > >> Subject: [PATCH 2/4] rtc: zynqmp: rework read_offset
> > >>
> > >> Caution: This message originated from an External Source. Use proper
> > >> caution when opening attachments, clicking links, or responding.
> > >>
> > >>
> > >> read_offset() was using static frequency for determining the tick
> > >> offset. It was also using remainder from do_div() operation as
> > >> tick_mult value which caused the offset to be incorrect.
> > >>
> > >> At the same time, rework function to improve readability.
> > >>
> > >> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> > >> ---
> > >>  drivers/rtc/rtc-zynqmp.c | 25 ++++++++++++++++---------
> > >>  1 file changed, 16 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
> > >> index
> > >>
> > 856bc1678e7d31144f320ae9f75fc58c742a2a64..7af5f6f99538f961a53ff56
> > bfc6
> > >> 56c907611b900 100644
> > >> --- a/drivers/rtc/rtc-zynqmp.c
> > >> +++ b/drivers/rtc/rtc-zynqmp.c
> > >> @@ -178,21 +178,28 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev
> > >> *xrtcdev)  static int xlnx_rtc_read_offset(struct device *dev, long *offset)  {
> > >>         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> > >> -       unsigned long long rtc_ppb = RTC_PPB;
> > >> -       unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
> > >> -       unsigned int calibval;
> > >> +       unsigned int calibval, fract_data, fract_part;
> > > Prefer one variable assignment per line for readability.
> > This is after all quite common practice, and in a function like this where several
> > variables are needed, I would argue that this is more readable than the
> > alternative. Is there some convention I'm not aware of?
> There is no such mandatory convention. It's up to the RTC maintainer.

I don't mind having multiple variable declarations on a single line.



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