[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <daccab41116d4c88823ab7fc84846077@AcuMS.aculab.com>
Date: Mon, 24 Jul 2023 14:01:27 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Geert Uytterhoeven' <geert@...ux-m68k.org>,
Serge Semin <fancer.lancer@...il.com>
CC: "wuyonggang001@...suo.com" <wuyonggang001@...suo.com>,
"mturquette@...libre.com" <mturquette@...libre.com>,
"sboyd@...nel.org" <sboyd@...nel.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] clk: baikal-t1: Using div64_ Ul replaces do_ Div()
function
From: Geert Uytterhoeven
> Sent: 24 July 2023 14:39
>
> Hi Serge,
>
> On Mon, Jul 24, 2023 at 3:13 PM Serge Semin <fancer.lancer@...il.com> wrote:
> > On Mon, Jul 24, 2023 at 12:04:19PM +0200, Geert Uytterhoeven wrote:
> > > 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
>
> > > > @@ -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...)
But on 32bit the result is 'long'.
So it will overflow unless do_div() is also valid.
The analysis need to look at the domain of the values.
The warning and suggestion to use div64_ul() is pretty much always
wrong.
div64_ul() is going to be horribly slow on 32bit.
Also on 64bit Intel cpu the 128/64 divide takes twice as long as 64/32
even when the values are small.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Powered by blists - more mailing lists