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 <[email protected]>
---
drivers/clk/baikal-t1/ccu-pll.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/baikal-t1/ccu-pll.c
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);
return us;
}
@@ -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;
}
Quoting [email protected] (2023-06-13 22:45:48)
> 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 <[email protected]>
> ---
Applied to clk-next
I had to manually apply it though and I had to fix the author to match
the SoB. Please take more care next time.
Hi Yonggang,
CC Serge
On Wed, Jun 14, 2023 at 8:07 AM <[email protected]> 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 <[email protected]>
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.
Have you tested your patch?
>
> return us;
So this should become:
return div64_ul(500ULL * nr * USEC_PER_SEC, ref_clk);
> }
> @@ -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.
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);
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;
But doing that requires intimate knowledge about the range of nf to
avoid overflow, so I leave that to Serge.
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
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 <[email protected]> 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 <[email protected]>
>
> 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 -- [email protected]
>
> 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
Hi Serge,
On Mon, Jul 24, 2023 at 3:13 PM Serge Semin <[email protected]> wrote:
> On Mon, Jul 24, 2023 at 12:04:19PM +0200, Geert Uytterhoeven wrote:
> > On Wed, Jun 14, 2023 at 8:07 AM <[email protected]> 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 <[email protected]>
> >
> > 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 -- [email protected]
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
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 <[email protected]> wrote:
> > On Mon, Jul 24, 2023 at 12:04:19PM +0200, Geert Uytterhoeven wrote:
> > > On Wed, Jun 14, 2023 at 8:07 AM <[email protected]> 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 <[email protected]>
> > >
> > > 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);
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 -- [email protected]
>
> 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
From: Geert Uytterhoeven
> Sent: 24 July 2023 14:39
>
> Hi Serge,
>
> On Mon, Jul 24, 2023 at 3:13 PM Serge Semin <[email protected]> wrote:
> > On Mon, Jul 24, 2023 at 12:04:19PM +0200, Geert Uytterhoeven wrote:
> > > On Wed, Jun 14, 2023 at 8:07 AM <[email protected]> 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 <[email protected]>
> > >
> > > 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)
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 <[email protected]> wrote:
> > > On Mon, Jul 24, 2023 at 12:04:19PM +0200, Geert Uytterhoeven wrote:
> > > > On Wed, Jun 14, 2023 at 8:07 AM <[email protected]> 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 <[email protected]>
> > > >
> > > > 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 -- [email protected]
> >
> > 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
Quoting Serge Semin (2023-07-27 05:28:47)
> 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:
> > > > >
> > > > > 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?
>
I've dropped it.
On Fri, Jul 28, 2023 at 08:24:19PM -0700, Stephen Boyd wrote:
> Quoting Serge Semin (2023-07-27 05:28:47)
> > 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:
> > > > > >
> > > > > > 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?
> >
>
> I've dropped it.
Great! Thanks.
-Serge(y)