Messages in this thread | | | Date | Mon, 24 Jul 2023 16:13:12 +0300 | From | Serge Semin <> | Subject | Re: [PATCH] clk: baikal-t1: Using div64_ Ul replaces do_ Div() function |
| |
Hi Geert
On Mon, Jul 24, 2023 at 12:04:19PM +0200, Geert Uytterhoeven wrote: > Hi Yonggang, > > CC Serge
Thanks for Cc-ing me.
> > On Wed, Jun 14, 2023 at 8:07 AM <wuyonggang001@208suo.com> wrote: > > Fix the following coccicheck warning: > > > > drivers/clk/baikal-t1/ccu-pll.c:81:1-7: WARNING: do_div() does a > > 64-by-32 division, please consider using div64_ul instead. > > > > Signed-off-by: Yonggang Wu <wuyonggang001@208suo.com> > > Thanks for your patch, which is now commit b93d1331ea266dea > ("clk: baikal-t1: Using div64_ Ul replaces do_ Div() function") > in clk/clk-next. > > > b/drivers/clk/baikal-t1/ccu-pll.c > > index 13ef28001439..d41735c6956a 100644 > > --- a/drivers/clk/baikal-t1/ccu-pll.c > > +++ b/drivers/clk/baikal-t1/ccu-pll.c > > @@ -66,7 +66,7 @@ static inline unsigned long > > ccu_pll_lock_delay_us(unsigned long ref_clk, > > { > > u64 us = 500ULL * nr * USEC_PER_SEC; > >
> > - do_div(us, ref_clk); > > + div64_ul(us, ref_clk); > > The above is not equivalent: > - do_div() returned the quotient as an output parameter in us, > - div64_ul() returns the quotient using the return value.
Indeed, leaving the patch as is will break the driver for sure. do_div() and div64_ul() aren't equivalent in regard of the return values. So this update will cause the ccu_pll_lock_delay_us() returning "500ULL * nr * USEC_PER_SEC" instead of "(500ULL * nr * USEC_PER_SEC) / ref_clk".
> > Have you tested your patch? > > > > > return us; > > So this should become: > > return div64_ul(500ULL * nr * USEC_PER_SEC, ref_clk);
This would be the correct fix. But I would either retain the local "us" variable here or fixed the drivers/clk/baikal-t1/ccu-div.c:ccu_div_lock_delay_ns() function too for the sake of the driver unification. The later is preferable though.
> > > } > > @@ -78,9 +78,9 @@ static inline unsigned long ccu_pll_calc_freq(unsigned > > long ref_clk, > > { > > u64 tmp = ref_clk; > >
> > - do_div(tmp, nr); > > + div64_ul(tmp, nr); > > tmp *= nf; > > - do_div(tmp, od); > > + div64_ul(tmp, od); > > > > return tmp; > > Likewise.
Right. This will also break the driver.
> But as ref_clk is unsigned long, there is no need to use div64_ul() > for the first division, and this can be simplified to: > > u64 tmp = (u64)(ref_clk / nr) * nf; > return div64_ul(tmp, od);
Absolutely right. My intention of using the do_div() anyway was for the sake of the code unification.
> > To avoid loss of precision, it might be better to reverse the order > of the division and multiplication: >
> u64 tmp = (u64)ref_clk * nf / nr;
Alas exactly this code will cause the compilation error on the 32-bit platform: ccu-pll.c:(.text+0x458): undefined reference to `__udivdi3'
That's why I am using the do_div() here. I would have rather used the div64_ul() instead as this patch suggests, but I haven't known about its existence up to this moment.
Anyway my intention of dividing before multiplying had twofold justification. Firstly I didn't want to use the "/" operator and do_div() macro in the statements used to implement the same formulae. Since I couldn't use the operator I decided to use the macro only for the code unification. Secondly the PLL is designed in a way so the signal is first divided by NR, then multiplied by NF and then divided by OD. That's why I decided to preserve the same order in the calculations here. I assumed back then that the NR-divider performs the integer division in the analog circuitry. I have doubts now that my assumption was correct since it's analog device and most likely divides the source signal with no integer rounding-up. So using the order suggested by you would have likely given a more exact result.
> > But doing that requires intimate knowledge about the range of nf to > avoid overflow, so I leave that to Serge.
nr: 1 - 2^6 nf: 1 - 2^13 ref_clk: normally 25'000'000 Hz. Using "unsigned long"/u32 multiplication will give the integer overflow. Meanwhile the u64 arithmetics will be more than enough here.
So to speak the next alteration seems more correct here: +return div64_ul(div64_ul((u64)ref_clk * nf, nr), od);
What do you think?
Yonggang, several comments: 1. Could you please include the "linux/math64.h" header file to the driver? 2. Could you please fix the same thing in the ccu-div.c file too?
-Serge(y)
> > > } > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
| |