Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Monday, October 14, 2024 8:44 AM
> Subject: Re: [PATCH v2] clk: renesas: rzg2l: Fix FOUTPOSTDIV clk
>
> Hi Biju,
>
> On Fri, Oct 11, 2024 at 6:20 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > While computing foutpostdiv_rate, the value of params->pl5_fracin is
> > discarded, which results in the wrong refresh rate. Fix the formula
> > for computing foutpostdiv_rate.
> >
> > Fixes: 1561380ee72f ("clk: renesas: rzg2l: Add FOUTPOSTDIV clk
> > support")
> > Signed-off-by: Hien Huynh <hien.huynh.px@xxxxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> > * Improved the precision by division of params->pl5_refdiv
> > done after all multiplication.
> > ---
> > drivers/clk/renesas/rzg2l-cpg.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/clk/renesas/rzg2l-cpg.c
> > b/drivers/clk/renesas/rzg2l-cpg.c index 88bf39e8c79c..a1e22d353689
> > 100644
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -548,7 +548,7 @@ static unsigned long
> > rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_pll5_param *params,
> > unsigned long rate) {
> > - unsigned long foutpostdiv_rate;
> > + unsigned long foutpostdiv_rate, foutvco_rate;
>
> While the resulting 64-bit value fits in foutvco_rate because unsigned
> long is 64-bit on the target platform, I'd rather play it safe
> (reuse!) and use u64 explicitly.
OK will use u64.
>
> >
> > params->pl5_intin = rate / MEGA;
> > params->pl5_fracin = div_u64(((u64)rate % MEGA) << 24, MEGA);
> > @@ -557,10 +557,12 @@ rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_pll5_param *params,
> > params->pl5_postdiv2 = 1;
> > params->pl5_spread = 0x16;
> >
> > - foutpostdiv_rate =
> > - EXTAL_FREQ_IN_MEGA_HZ * MEGA / params->pl5_refdiv *
> > - ((((params->pl5_intin << 24) + params->pl5_fracin)) >> 24) /
> > - (params->pl5_postdiv1 * params->pl5_postdiv2);
> > + foutvco_rate =
> > + (EXTAL_FREQ_IN_MEGA_HZ * MEGA *
> > + ((params->pl5_intin << 24) + params->pl5_fracin) /
> > + params->pl5_refdiv) >> 24;
>
> Shouldn't this use mul_u32_u32(EXTAL_FREQ_IN_MEGA_HZ * MEGA,
> ((params->pl5_intin << 24) + params->pl5_fracin)) instead of a plain
> multiplication?
> See also the comment for mul_u32_u32() in <linux/math64.h>.
OK. Will use mul_u32_u32().
>
> > + foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate,
> > + params->pl5_postdiv1 * params->pl5_postdiv2);
>
> Unfortunately we don't have a helper macro yet to round the result of
> div_u64(), so you will have to open-code that (for now).
As per [1], round_closest(x,y) where x is u64 and y is u32
In this case max value of x is 3000MHz < 2^32
and
y < 50
So, do we need open-code? Am I missing anything here?
[1] https://elixir.bootlin.com/linux/v6.0-rc4/source/include/linux/math.h#L101
Cheers,
Biju
>
> >
> > return foutpostdiv_rate;
> > }
> 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
