Re: [PATCH] clk: baikal-t1: Using div64_ Ul replaces do_ Div() function

[Geert Uytterhoeven]

Hi Serge,

On Mon, Jul 24, 2023 at 3:13 PM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> On Mon, Jul 24, 2023 at 12:04:19PM +0200, Geert Uytterhoeven wrote:
> > On Wed, Jun 14, 2023 at 8:07 AM <wuyonggang001@xxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > 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

> > > @@ -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.

Bummer, that was a silly mistake on my side...
(Initially, I didn't write the cast to u64 there, as all of ref_clk, nf, and nr
 are unsigned long.  Then I realized "ref_clk * nf" might overflow on
 32-bit, thus requiring a 64-bit result. And I added the cast...)

> 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?

Given the ranges above, nr and nf can be u32 instead of unsigned long.
So perhaps it makes sense to use the mul_u64_u32_div() helper?

    return div64_ul(mul_u64_u32_div(ref_clk, nf, nr), of);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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