Hi Geert,
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Monday, October 14, 2024 3:32 PM
> Subject: Re: [PATCH v2] clk: renesas: rzg2l: Fix FOUTPOSTDIV clk
>
> Hi Biju,
>
> On Mon, Oct 14, 2024 at 12:37 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> On Mon, Oct 14, 2024
> > > at 11:55 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> 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
> > >
> > > But that is not obvious from the code (and foutvco_rate is u64 soon?).
> > > Also, is that guaranteed? What if the user plugs in a 4K or 8K HDMI display?
> >
> > 1080p@60Hz-->148.5MHz --> this is the max dot clock frequency supported[1].
> >
> > 3000MHz is the reset values of the pll.
> >
> > 4k@60-->594 MHz and 8k@60-->2856MHz
>
> OK, if you're sure it can never exceed 32-bit, then keep on using unsigned long for foutvco_rate is
> fine, and using
> DIV_ROUND_CLOSEST_ULL() is fine, too.
It never exceeds 32-bit, The max value is 148.5MHz which is very less compared to 2^32= 4294 MHz
> But the "EXTAL_FREQ_IN_MEGA_HZ * MEGA * ((params->pl5_intin << 24) + params->pl5_fracin)"
> intermediate definitely needs to use mul_u32_u32().
Sure, will use mul_u32_u32()
Cheers,
Biju
