diff mbox series

[v2] clk: renesas: rzg2l: Fix FOUTPOSTDIV clk

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

Commit Message

Biju Das Oct. 11, 2024, 4:20 p.m. UTC
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(-)

Comments

Geert Uytterhoeven Oct. 14, 2024, 7:43 a.m. UTC | #1
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
Biju Das Oct. 14, 2024, 9:54 a.m. UTC | #2
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
Geert Uytterhoeven Oct. 14, 2024, 10:20 a.m. UTC | #3
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
Biju Das Oct. 14, 2024, 10:36 a.m. UTC | #4
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
Biju Das Oct. 14, 2024, 10:50 a.m. UTC | #5
> -----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
Geert Uytterhoeven Oct. 14, 2024, 2:31 p.m. UTC | #6
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
Biju Das Oct. 14, 2024, 3:08 p.m. UTC | #7
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 mbox series

Patch

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;
 }