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

[Tomas Melin <tomas.melin@vaisala.com>]

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