linux-kernel - RE: [PATCH 2/4] rtc: zynqmp: rework read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <LV5PR12MB980448D3F4109DC0775A56AA92ABA@LV5PR12MB9804.namprd12.prod.outlook.com>
Date: Wed, 17 Dec 2025 18:14:30 +0000
From: "T, Harini" <Harini.T@....com>
To: Tomas Melin <tomas.melin@...sala.com>, Alexandre Belloni
	<alexandre.belloni@...tlin.com>, "Simek, Michal" <michal.simek@....com>
CC: "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/4] rtc: zynqmp: rework read_offset

[Public]

Hi,

> -----Original Message-----
> From: Tomas Melin <tomas.melin@...sala.com>
> Sent: Wednesday, December 10, 2025 5:35 PM
> To: T, Harini <Harini.T@....com>; Alexandre Belloni
> <alexandre.belloni@...tlin.com>; Simek, Michal <michal.simek@....com>
> Cc: linux-rtc@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.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@...sala.com>
> >> Sent: Monday, December 1, 2025 6:20 PM
> >> To: Alexandre Belloni <alexandre.belloni@...tlin.com>; Simek, Michal
> >> <michal.simek@....com>
> >> Cc: linux-rtc@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> >> linux- kernel@...r.kernel.org; Tomas Melin <tomas.melin@...sala.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@...sala.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