Messages in this thread | | | Date | Thu, 27 Jul 2023 15:28:47 +0300 | From | Serge Semin <> | Subject | Re: [PATCH] clk: baikal-t1: Using div64_ Ul replaces do_ Div() function |
| |
Hi Geert, Stephen
On Mon, Jul 24, 2023 at 05:11:23PM +0300, Serge Semin wrote: > On Mon, Jul 24, 2023 at 03:38:49PM +0200, Geert Uytterhoeven wrote: > > Hi Serge, > > > > On Mon, Jul 24, 2023 at 3:13 PM Serge Semin <fancer.lancer@gmail.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@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 > > > > > > > @@ -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.
No news from Yonggang meanwhile the patch will certainly break the driver. Is it still possible to drop it from the clk-cleanup and clk-next branches then before it gets to the mainline kernel?
-Serge(y)
> > > > > > > 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); > > Just a day of discoveries today.) Didn't know about the > mul_u64_u32_div() existence either. Thanks for suggestion. Anyway > seeing "unsigned long" is 32-bits wide on my platform, nr/nf/od will > always be within the specified ranges, why not. Although using two > div64_ul()'s seems a bit more readable. But it might be just because > of me not being used to the mul_u64_u32_div() prototype notation. > > -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
| |