Hi Chris and Geert On Fri, 21 Nov 2025 09:59:05 +0100 Geert Uytterhoeven <[email protected]> wrote:
> Hi Chris, > > On Fri, 21 Nov 2025 at 05:04, Chris Brandt <[email protected]> wrote: > > On Wed, Nov 19, 2025 1:23 PM, Hugo Villeneuve wrote: > > > > + params->pl5_fracin = div_u64((u64) > > > > + ((foutvco_rate * > > > > params->pl5_refdiv) % > > > > + (EXTAL_FREQ_IN_MEGA_HZ > > > > * MEGA)) << 24, > > > > + EXTAL_FREQ_IN_MEGA_HZ > > > > * MEGA); > > > > > > > > > > > > Also: > > > > foutvco_rate (max) = 3000000000 (3GHz) > > > > pl5_refdiv (max) = 2 > > > > > > > > so the result of (foutvco_rate * params->pl5_refdiv) could become > > > > 6GHz, which is greater than unsigned long on 32-bit platform and > > > > overflow? > > > > > > I confirm that when testing with "COMPILE_TEST" as Geert suggested on a > > > 32-bit platform, the results are not > > > valid for this combination (but they are valid on 64-bit platforms). > > > > > > I think that the kernel robot could potentially issue a build warning for > > > 32-bit platforms (if they also build with > > > COMPILE_TEST enabled, which I'm not sure about). Maybe Geert could > > > comment on this? > > > > I've got no comment here. > > > > I can't image when someone would ever want to compile this code for a > > 32-bit system. > > > > So I'll leave it as it is now unless Geert wants me to change it to > > something else. > > Pieces of code are reused all the time. So I think it is better to make > sure it doesn't overflow on 32-bit. Here is a potential idea for implementing it so that it doesn't overflow. I tested it (compile and run) on a 32-bit platform, and also with my RZG2L board. Note that I defined an intermediate variable to improve readability (extal_hz): --- a/drivers/clk/renesas/rzg2l-cpg.c +++ b/drivers/clk/renesas/rzg2l-cpg.c @@ -22,6 +22,7 @@ #include <linux/device.h> #include <linux/init.h> #include <linux/iopoll.h> +#include <linux/math64.h> #include <linux/mod_devicetable.h> #include <linux/module.h> #include <linux/of.h> @@ -583,10 +584,12 @@ rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_cpg_priv *priv, struct rzg2l_pll5_param *params, unsigned long rate) { - unsigned long foutpostdiv_rate, foutvco_rate; + unsigned long foutpostdiv_rate; + u64 foutvco_rate; unsigned long hsclk; unsigned int a, b, odd; unsigned int dsi_div_ab_calc; + const u32 extal_hz = EXTAL_FREQ_IN_MEGA_HZ * MEGA; if (dsi_div_target == PLL5_TARGET_DSI) { /* @@ -662,16 +665,16 @@ rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_cpg_priv *priv, for (params->pl5_refdiv = PLL5_REFDIV_MIN; params->pl5_refdiv <= PLL5_REFDIV_MAX; params->pl5_refdiv++) { - params->pl5_intin = (foutvco_rate * params->pl5_refdiv) / - (EXTAL_FREQ_IN_MEGA_HZ * MEGA); + u32 rem; + + params->pl5_intin = div_u64_rem(foutvco_rate * params->pl5_refdiv, extal_hz, &rem); + if (params->pl5_intin < PLL5_INTIN_MIN || params->pl5_intin > PLL5_INTIN_MAX) continue; - params->pl5_fracin = div_u64(((u64) - (foutvco_rate * params->pl5_refdiv) % - (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24, - EXTAL_FREQ_IN_MEGA_HZ * MEGA); + params->pl5_fracin = div_u64((u64)rem << 24, extal_hz); goto clk_valid; Hugo > 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 > -- Hugo Villeneuve <[email protected]>
