| Message ID | 20241011162030.104489-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] clk: renesas: rzg2l: Fix FOUTPOSTDIV clk | expand |
Hi Biju, On Fri, Oct 11, 2024 at 6:20 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> > --- > 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. > > 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>. > + 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). > > 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
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > 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@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> > > --- > > 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@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
Hi Biju, On Mon, Oct 14, 2024 at 11:55 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > On Fri, Oct 11, 2024 at 6:20 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> > > > --- > > > 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? > > 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 You mean https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L477 ? #define round_closest(x, y) round_down((x) + (y)/2, (y)) And round_down()" https://elixir.bootlin.com/linux/v6.0-rc4/source/include/linux/math.h#L35 which states that @y must be a power of 2: https://elixir.bootlin.com/linux/v6.0-rc4/source/include/linux/math.h#L30 Gr{oetje,eeting}s, Geert
Hi Geert, > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Monday, October 14, 2024 11:21 AM > Subject: Re: [PATCH v2] clk: renesas: rzg2l: Fix FOUTPOSTDIV clk > > Hi Biju, > > On Mon, Oct 14, 2024 at 11:55 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Fri, Oct 11, 2024 > > > at 6:20 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> > > > > --- > > > > 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 [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/renesas/rzg2l-cpg.c?h=next-20241014#n608 > > > > > 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 > > You mean > https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L477 > ? > > #define round_closest(x, y) round_down((x) + (y)/2, (y)) > > And round_down()" > https://elixir.bootlin.com/linux/v6.0-rc4/source/include/linux/math.h#L35 > > which states that @y must be a power of 2: > https://elixir.bootlin.com/linux/v6.0-rc4/source/include/linux/math.h#L30 For DIV_ROUND_CLOSEST_ULL, @y need not be power of 2. https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/gpu/drm/drm_modes.c#L826 Cheers, Biju
> -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Monday, October 14, 2024 11:21 AM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; linux-renesas- > soc@vger.kernel.org; linux-clk@vger.kernel.org; Prabhakar Mahadev Lad <prabhakar.mahadev- > lad.rj@bp.renesas.com>; biju.das.au <biju.das.au@gmail.com>; Hien Huynh <hien.huynh.px@renesas.com> > Subject: Re: [PATCH v2] clk: renesas: rzg2l: Fix FOUTPOSTDIV clk > > Hi Biju, > > On Mon, Oct 14, 2024 at 11:55 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Fri, Oct 11, 2024 > > > at 6:20 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> > > > > --- > > > > 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? > > > > > 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 > > You mean > https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L477 > ? Sorry, I meant for DIV_ROUND_CLOSEST_ULL, I incorrectly wrote it as round_closest(x,y) https://elixir.bootlin.com/linux/v6.0-rc4/source/include/linux/math.h#L101 Cheers, Biju
Hi Biju, On Mon, Oct 14, 2024 at 12:37 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > On Mon, Oct 14, 2024 at 11:55 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Fri, Oct 11, 2024 > > > > at 6:20 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> > > > > > --- > > > > > 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. But the "EXTAL_FREQ_IN_MEGA_HZ * MEGA * ((params->pl5_intin << 24) + params->pl5_fracin)" intermediate definitely needs to use mul_u32_u32(). Gr{oetje,eeting}s, Geert
Hi Geert, > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > 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@bp.renesas.com> wrote: > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, Oct 14, 2024 > > > at 11:55 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Fri, Oct 11, > > > > > 2024 at 6:20 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> > > > > > > --- > > > > > > 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
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; 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; + 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> --- 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(-)