Message-ID: <nt6kbounehvfqo4hpfj3wbr7baukuhr22dafvoykgyehs4imsp@pc6bajyo6ugn>
Date:   Mon, 24 Jul 2023 16:13:12 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>, wuyonggang001@...suo.com
Cc:     wuyonggang001@...suo.com, mturquette@...libre.com,
        sboyd@...nel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org
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@...suo.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@...suo.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@...ux-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

Powered by blists - more mailing lists