| Message ID | 20241009140251.135085-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | clk: renesas: rzg2l: Fix FOUTPOSTDIV clk | expand |
Hi Biju, On Wed, Oct 9, 2024 at 4:03 PM Biju Das <biju.das.jz@bp.renesas.com> 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@renesas.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- 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; > > 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_refdiv; > + foutvco_rate = mul_u64_u32_shr(foutvco_rate, > + (params->pl5_intin << 24) + params->pl5_fracin, > + 24); The first parameter is not u64, but unsigned long, and its value always fits in u32, so "mul_u32_u32(..., ...) >> 24" should do? However, if you care about precision, the division by params->pl5_refdiv should be done after all multiplication, too. > + foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate, > + params->pl5_postdiv1 * params->pl5_postdiv2); > > return foutpostdiv_rate; > } Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Friday, October 11, 2024 3:07 PM > Subject: Re: [PATCH] clk: renesas: rzg2l: Fix FOUTPOSTDIV clk > > Hi Biju, > > On Wed, Oct 9, 2024 at 4:03 PM Biju Das <biju.das.jz@bp.renesas.com> 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@renesas.com> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- 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; > > > > 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_refdiv; > > + foutvco_rate = mul_u64_u32_shr(foutvco_rate, > > + (params->pl5_intin << 24) + params->pl5_fracin, > > + 24); > > The first parameter is not u64, but unsigned long, and its value always fits in u32, so > "mul_u32_u32(..., ...) >> 24" should do? > > However, if you care about precision, the division by params->pl5_refdiv should be done after all > multiplication, too. I do care about precision. I willlike below in next version. + foutvco_rate = + (EXTAL_FREQ_IN_MEGA_HZ * MEGA * + ((params->pl5_intin << 24) + params->pl5_fracin) / + params->pl5_refdiv) >> 24; Cheers, Biju > > > + foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate, > > + params->pl5_postdiv1 > > + * params->pl5_postdiv2); > > > > return foutpostdiv_rate; > > } > > 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
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c index 88bf39e8c79c..58b7cbb24b5a 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; 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_refdiv; + foutvco_rate = mul_u64_u32_shr(foutvco_rate, + (params->pl5_intin << 24) + params->pl5_fracin, + 24); + foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate, + params->pl5_postdiv1 * params->pl5_postdiv2); return foutpostdiv_rate; }
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@renesas.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- Display resolution:1920x1080@60-->148.5 MHz dot clock Before applying patch: foutpostdiv_rate=1776000000 Dot clock = 1776000000/12 = 148 MHz After applying patch: foutpostdiv_rate=1782000000 Dot clock = 1782000000/12 = 148.5 MHz --- drivers/clk/renesas/rzg2l-cpg.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)