diff mbox series

clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock multiplier and divider values

Message ID 20220913084434.1191619-1-biju.das.jz@bp.renesas.com (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock multiplier and divider values | expand

Commit Message

Biju Das Sept. 13, 2022, 8:44 a.m. UTC
As per the HW manual (Rev.1.10 Apr, 2022) clock rate for 533MHz PLL2 and
PLL3 clocks should be 533 MHz, but with current multiplier and divider
values this resulted to 533.333333 MHz.

This patch updates the multiplier and divider values for 533 MHz PLL2 and
PLL3 clocks so that we get the exact (533 MHz) values.

Fixes: 373bd6f487562e ("clk: renesas: r9a07g044: Add SDHI clock and reset entries")
Fixes: f294a0ea9d12a6 ("clk: renesas: r9a07g044: Add clock and reset entries for SPI Multi I/O Bus Controller")
Fixes: 31d5ef2f565d23 ("clk: renesas: r9a07g044: Add M4 Clock support")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/clk/renesas/r9a07g044-cpg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Geert Uytterhoeven Sept. 13, 2022, 8:57 a.m. UTC | #1
Hi Biju,

Thanks for your patch!

On Tue, Sep 13, 2022 at 9:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> As per the HW manual (Rev.1.10 Apr, 2022) clock rate for 533MHz PLL2 and
> PLL3 clocks should be 533 MHz, but with current multiplier and divider
> values this resulted to 533.333333 MHz.
>
> This patch updates the multiplier and divider values for 533 MHz PLL2 and
> PLL3 clocks so that we get the exact (533 MHz) values.

Does this matter? Is there anything that doesn't work (well) because
of this?

> Fixes: 373bd6f487562e ("clk: renesas: r9a07g044: Add SDHI clock and reset entries")
> Fixes: f294a0ea9d12a6 ("clk: renesas: r9a07g044: Add clock and reset entries for SPI Multi I/O Bus Controller")
> Fixes: 31d5ef2f565d23 ("clk: renesas: r9a07g044: Add M4 Clock support")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

> --- a/drivers/clk/renesas/r9a07g044-cpg.c
> +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> @@ -113,10 +113,10 @@ static const struct {
>                 DEF_FIXED(".osc_div1000", CLK_OSC_DIV1000, CLK_EXTAL, 1, 1000),
>                 DEF_SAMPLL(".pll1", CLK_PLL1, CLK_EXTAL, PLL146_CONF(0)),
>                 DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 200, 3),
> -               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 1, 3),
> +               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 533, 1600),

I highly doubt the actual hardware is not using a by-3 divider....

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 Sept. 13, 2022, 9:11 a.m. UTC | #2
Hi Geert,

> Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock
> multiplier and divider values
> 
> Hi Biju,
> 
> Thanks for your patch!
> 
> On Tue, Sep 13, 2022 at 9:44 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > As per the HW manual (Rev.1.10 Apr, 2022) clock rate for 533MHz PLL2
> > and
> > PLL3 clocks should be 533 MHz, but with current multiplier and divider
> > values this resulted to 533.333333 MHz.
> >
> > This patch updates the multiplier and divider values for 533 MHz PLL2
> > and
> > PLL3 clocks so that we get the exact (533 MHz) values.
> 
> Does this matter? Is there anything that doesn't work (well) because of
> this?

Yes, SDHI performance gone bad as it selects 533Mhz clock instead of 400Mhz.
Similar case for RZ/G2UL, which I am testing it now.

Previously:-
533333333->src clk0
400000000->src clk1
266666666->src clk2

Now:-
533000000->src clk0
400000000->src clk1
266500000->src clk2

If I am correct, with wrong values, it ended
up in 533333332(parent rate= 133333333 *4) and requested rate 533333333
and it selected best rate as 400000000. 
 
Cheers,
Biju

> 
> > Fixes: 373bd6f487562e ("clk: renesas: r9a07g044: Add SDHI clock and
> > reset entries")
> > Fixes: f294a0ea9d12a6 ("clk: renesas: r9a07g044: Add clock and reset
> > entries for SPI Multi I/O Bus Controller")
> > Fixes: 31d5ef2f565d23 ("clk: renesas: r9a07g044: Add M4 Clock
> > support")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > @@ -113,10 +113,10 @@ static const struct {
> >                 DEF_FIXED(".osc_div1000", CLK_OSC_DIV1000, CLK_EXTAL,
> 1, 1000),
> >                 DEF_SAMPLL(".pll1", CLK_PLL1, CLK_EXTAL,
> PLL146_CONF(0)),
> >                 DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 200, 3),
> > -               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 1, 3),
> > +               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 533,
> > + 1600),
> 
> I highly doubt the actual hardware is not using a by-3 divider....
> 
> 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 Sept. 13, 2022, 9:13 a.m. UTC | #3
> multiplier and divider values
> 
> Hi Geert,
> 
> > Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock
> > multiplier and divider values
> >
> > Hi Biju,
> >
> > Thanks for your patch!
> >
> > On Tue, Sep 13, 2022 at 9:44 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > As per the HW manual (Rev.1.10 Apr, 2022) clock rate for 533MHz PLL2
> > > and
> > > PLL3 clocks should be 533 MHz, but with current multiplier and
> > > divider values this resulted to 533.333333 MHz.
> > >
> > > This patch updates the multiplier and divider values for 533 MHz
> > > PLL2 and
> > > PLL3 clocks so that we get the exact (533 MHz) values.
> >
> > Does this matter? Is there anything that doesn't work (well) because
> > of this?
> 
> Yes, SDHI performance gone bad as it selects 533Mhz clock instead of
> 400Mhz.

Typo please read it as it selects 400MHz instead of 533MHz.

Cheers,
Biju

> Similar case for RZ/G2UL, which I am testing it now.
> 
> Previously:-
> 533333333->src clk0
> 400000000->src clk1
> 266666666->src clk2
> 
> Now:-
> 533000000->src clk0
> 400000000->src clk1
> 266500000->src clk2
> 
> If I am correct, with wrong values, it ended up in 533333332(parent rate=
> 133333333 *4) and requested rate 533333333 and it selected best rate as
> 400000000.
> 
> Cheers,
> Biju
> 
> >
> > > Fixes: 373bd6f487562e ("clk: renesas: r9a07g044: Add SDHI clock and
> > > reset entries")
> > > Fixes: f294a0ea9d12a6 ("clk: renesas: r9a07g044: Add clock and reset
> > > entries for SPI Multi I/O Bus Controller")
> > > Fixes: 31d5ef2f565d23 ("clk: renesas: r9a07g044: Add M4 Clock
> > > support")
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > @@ -113,10 +113,10 @@ static const struct {
> > >                 DEF_FIXED(".osc_div1000", CLK_OSC_DIV1000,
> > > CLK_EXTAL,
> > 1, 1000),
> > >                 DEF_SAMPLL(".pll1", CLK_PLL1, CLK_EXTAL,
> > PLL146_CONF(0)),
> > >                 DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 200, 3),
> > > -               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 1, 3),
> > > +               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 533,
> > > + 1600),
> >
> > I highly doubt the actual hardware is not using a by-3 divider....
> >
> > 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 Sept. 13, 2022, 9:41 a.m. UTC | #4
Hi Biju,

CC Wolfram (SDHI)

On Tue, Sep 13, 2022 at 10:11 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock
> > multiplier and divider values
> > On Tue, Sep 13, 2022 at 9:44 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > As per the HW manual (Rev.1.10 Apr, 2022) clock rate for 533MHz PLL2
> > > and
> > > PLL3 clocks should be 533 MHz, but with current multiplier and divider
> > > values this resulted to 533.333333 MHz.
> > >
> > > This patch updates the multiplier and divider values for 533 MHz PLL2
> > > and
> > > PLL3 clocks so that we get the exact (533 MHz) values.
> >
> > Does this matter? Is there anything that doesn't work (well) because of
> > this?
>
> Yes, SDHI performance gone bad as it selects 400Mhz clock instead of 533Mhz.
> Similar case for RZ/G2UL, which I am testing it now.
>
> Previously:-
> 533333333->src clk0
> 400000000->src clk1
> 266666666->src clk2
>
> Now:-
> 533000000->src clk0
> 400000000->src clk1
> 266500000->src clk2
>
> If I am correct, with wrong values, it ended
> up in 533333332(parent rate= 133333333 *4) and requested rate 533333333
> and it selected best rate as 400000000.

IC, that is annoying.

However, I don't think the right fix is to change the dividers to
values that do not match the hardware.
Due to the (in)accuracy of clock crystals, the least significant
digits in the above clock rates are not significant anyway.

Perhaps the "if (freq > (new_clock << i))" check in
renesas_sdhi_clk_update() can be slightly relaxed, so it allows
e.g. a 0.1% (or 1/1024th?) higher clock rate than requested?

> > > -               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 1, 3),
> > > +               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 533, 1600),
> >
> > I highly doubt the actual hardware is not using a by-3 divider....

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 Sept. 13, 2022, 9:58 a.m. UTC | #5
Hi Geert,

> Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock
> multiplier and divider values
> 
> Hi Biju,
> 
> CC Wolfram (SDHI)
> 
> On Tue, Sep 13, 2022 at 10:11 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3
> > > clock multiplier and divider values On Tue, Sep 13, 2022 at 9:44 AM
> > > Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > As per the HW manual (Rev.1.10 Apr, 2022) clock rate for 533MHz
> > > > PLL2 and
> > > > PLL3 clocks should be 533 MHz, but with current multiplier and
> > > > divider values this resulted to 533.333333 MHz.
> > > >
> > > > This patch updates the multiplier and divider values for 533 MHz
> > > > PLL2 and
> > > > PLL3 clocks so that we get the exact (533 MHz) values.
> > >
> > > Does this matter? Is there anything that doesn't work (well) because
> > > of this?
> >
> > Yes, SDHI performance gone bad as it selects 400Mhz clock instead of
> 533Mhz.
> > Similar case for RZ/G2UL, which I am testing it now.
> >
> > Previously:-
> > 533333333->src clk0
> > 400000000->src clk1
> > 266666666->src clk2
> >
> > Now:-
> > 533000000->src clk0
> > 400000000->src clk1
> > 266500000->src clk2
> >
> > If I am correct, with wrong values, it ended up in 533333332(parent
> > rate= 133333333 *4) and requested rate 533333333 and it selected best
> > rate as 400000000.
> 
> IC, that is annoying.
> 
> However, I don't think the right fix is to change the dividers to values
> that do not match the hardware.

The new values(for SDHI, SPI mult and M4) are matching with clock list
Document RZG2L_clock_list_r1.1.xlsx and HW manual(page 235/236)
Figure 7.2/7.3 Clock System Diagram. 

Yes, we need to have some relaxation for clocks as mentioned
below.

Cheers,
Biju

> Due to the (in)accuracy of clock crystals, the least significant digits
> in the above clock rates are not significant anyway.
> 
> Perhaps the "if (freq > (new_clock << i))" check in
> renesas_sdhi_clk_update() can be slightly relaxed, so it allows e.g. a
> 0.1% (or 1/1024th?) higher clock rate than requested?
> 
> > > > -               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 1,
> 3),
> > > > +               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2,
> > > > + 533, 1600),
> > >
> > > I highly doubt the actual hardware is not using a by-3 divider....
> 
> 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
Wolfram Sang Sept. 13, 2022, 10:05 a.m. UTC | #6
> Perhaps the "if (freq > (new_clock << i))" check in
> renesas_sdhi_clk_update() can be slightly relaxed, so it allows
> e.g. a 0.1% (or 1/1024th?) higher clock rate than requested?

Yes, we can do that.
Biju Das Sept. 13, 2022, 10:30 a.m. UTC | #7
Hi Wolfram and Geert,

> Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock
> multiplier and divider values
> 
> 
> > Perhaps the "if (freq > (ccccc))" check in
> > renesas_sdhi_clk_update() can be slightly relaxed, so it allows e.g. a
> > 0.1% (or 1/1024th?) higher clock rate than requested?
> 
> Yes, we can do that.

		freq = clk_round_rate(ref_clk, new_clock << i);
	      + pr_err("%s## freq/new_clock=%llu/%llu",__func__,freq,(new_clock << i));
		if (freq > (new_clock << i)) {

This is how it ended up in selecting 400MHz clk.

For (new_clock << i) = 533333332, clk_round_rate becomes 400MHz and it selected that clock.

[   18.364948] renesas_sdhi_clk_update## freq/new_clock=533333333/4266666656
[   18.364980] renesas_sdhi_clk_update## freq/new_clock=533333333/2133333328
[   18.371871] renesas_sdhi_clk_update## freq/new_clock=533333333/1066666664
[   18.381122] renesas_sdhi_clk_update## freq/new_clock=400000000/533333332
[   18.388033] renesas_sdhi_clk_update## freq/new_clock=533333333/4266666656
[   18.394936] renesas_sdhi_clk_update## freq/new_clock=533333333/2133333328
[   18.401829] renesas_sdhi_clk_update## freq/new_clock=533333333/1066666664
[   18.408760] renesas_sdhi_clk_update## freq/new_clock=400000000/533333332

Cheers,
Biju
Wolfram Sang Sept. 13, 2022, 10:37 a.m. UTC | #8
> > > Perhaps the "if (freq > (ccccc))" check in
> > > renesas_sdhi_clk_update() can be slightly relaxed, so it allows e.g. a
> > > 0.1% (or 1/1024th?) higher clock rate than requested?
> > 
> > Yes, we can do that.
...
> This is how it ended up in selecting 400MHz clk.

May I ask you to implement it? You have the HW to check which margin is
actually needed.
Biju Das Sept. 13, 2022, 10:40 a.m. UTC | #9
Hi Wolfram,

> Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock
> multiplier and divider values
> 
> 
> > > > Perhaps the "if (freq > (ccccc))" check in
> > > > renesas_sdhi_clk_update() can be slightly relaxed, so it allows
> > > > e.g. a 0.1% (or 1/1024th?) higher clock rate than requested?
> > >
> > > Yes, we can do that.
> ...
> > This is how it ended up in selecting 400MHz clk.
> 
> May I ask you to implement it? You have the HW to check which margin is
> actually needed.

Sure will do.

Cheers,
Biju
Geert Uytterhoeven Sept. 13, 2022, 4:41 p.m. UTC | #10
Hi Biju,

On Tue, Sep 13, 2022 at 10:58 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock
> > multiplier and divider values
> >
> > On Tue, Sep 13, 2022 at 10:11 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3
> > > > clock multiplier and divider values On Tue, Sep 13, 2022 at 9:44 AM
> > > > Biju Das <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > As per the HW manual (Rev.1.10 Apr, 2022) clock rate for 533MHz
> > > > > PLL2 and
> > > > > PLL3 clocks should be 533 MHz, but with current multiplier and
> > > > > divider values this resulted to 533.333333 MHz.
> > > > >
> > > > > This patch updates the multiplier and divider values for 533 MHz
> > > > > PLL2 and
> > > > > PLL3 clocks so that we get the exact (533 MHz) values.
> > > >
> > > > Does this matter? Is there anything that doesn't work (well) because
> > > > of this?
> > >
> > > Yes, SDHI performance gone bad as it selects 400Mhz clock instead of
> > 533Mhz.
> > > Similar case for RZ/G2UL, which I am testing it now.
> > >
> > > Previously:-
> > > 533333333->src clk0
> > > 400000000->src clk1
> > > 266666666->src clk2
> > >
> > > Now:-
> > > 533000000->src clk0
> > > 400000000->src clk1
> > > 266500000->src clk2
> > >
> > > If I am correct, with wrong values, it ended up in 533333332(parent
> > > rate= 133333333 *4) and requested rate 533333333 and it selected best
> > > rate as 400000000.
> >
> > IC, that is annoying.
> >
> > However, I don't think the right fix is to change the dividers to values
> > that do not match the hardware.
>
> The new values(for SDHI, SPI mult and M4) are matching with clock list
> Document RZG2L_clock_list_r1.1.xlsx and HW manual(page 235/236)
> Figure 7.2/7.3 Clock System Diagram.

All values are given in MHz, with a limited number of significant
digits, not in Hz. The most accurate value I found is 66.625 MHz,
which has 5 significant digits, and thus really means 66625xxx Hz.
Such an accuracy matches the accuracy of typical clock crystals.

The figures only mention "1600 MHz" and "533 MHz".  The latter
has only 3 significant digits, and is probably just 1600 MHz / 3.
All further dividers are documented to be powers of two.

> Yes, we need to have some relaxation for clocks as mentioned
> below.

Indeed. But this is IMHO something that should be handled by the
code that looks for the best clock rate, instead of by fiddling with the
dividers to get "nicely rounded" numbers.

> > Due to the (in)accuracy of clock crystals, the least significant digits
> > in the above clock rates are not significant anyway.
> >
> > Perhaps the "if (freq > (new_clock << i))" check in
> > renesas_sdhi_clk_update() can be slightly relaxed, so it allows e.g. a
> > 0.1% (or 1/1024th?) higher clock rate than requested?
> >
> > > > > -               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 1,
> > 3),
> > > > > +               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2,
> > > > > + 533, 1600),
> > > >
> > > > I highly doubt the actual hardware is not using a by-3 divider....

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 Sept. 13, 2022, 4:51 p.m. UTC | #11
Hi Geert,

> Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3 clock
> multiplier and divider values
> 
> Hi Biju,
> 
> On Tue, Sep 13, 2022 at 10:58 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3
> > > clock multiplier and divider values
> > >
> > > On Tue, Sep 13, 2022 at 10:11 AM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > Subject: Re: [PATCH] clk: renesas: r9a07g044: Fix 533MHz PLL2/3
> > > > > clock multiplier and divider values On Tue, Sep 13, 2022 at 9:44
> > > > > AM Biju Das <biju.das.jz@bp.renesas.com>
> > > > > wrote:
> > > > > > As per the HW manual (Rev.1.10 Apr, 2022) clock rate for
> > > > > > 533MHz
> > > > > > PLL2 and
> > > > > > PLL3 clocks should be 533 MHz, but with current multiplier and
> > > > > > divider values this resulted to 533.333333 MHz.
> > > > > >
> > > > > > This patch updates the multiplier and divider values for 533
> > > > > > MHz
> > > > > > PLL2 and
> > > > > > PLL3 clocks so that we get the exact (533 MHz) values.
> > > > >
> > > > > Does this matter? Is there anything that doesn't work (well)
> > > > > because of this?
> > > >
> > > > Yes, SDHI performance gone bad as it selects 400Mhz clock instead
> > > > of
> > > 533Mhz.
> > > > Similar case for RZ/G2UL, which I am testing it now.
> > > >
> > > > Previously:-
> > > > 533333333->src clk0
> > > > 400000000->src clk1
> > > > 266666666->src clk2
> > > >
> > > > Now:-
> > > > 533000000->src clk0
> > > > 400000000->src clk1
> > > > 266500000->src clk2
> > > >
> > > > If I am correct, with wrong values, it ended up in
> > > > 533333332(parent rate= 133333333 *4) and requested rate 533333333
> > > > and it selected best rate as 400000000.
> > >
> > > IC, that is annoying.
> > >
> > > However, I don't think the right fix is to change the dividers to
> > > values that do not match the hardware.
> >
> > The new values(for SDHI, SPI mult and M4) are matching with clock list
> > Document RZG2L_clock_list_r1.1.xlsx and HW manual(page 235/236) Figure
> > 7.2/7.3 Clock System Diagram.
> 
> All values are given in MHz, with a limited number of significant digits,
> not in Hz. The most accurate value I found is 66.625 MHz, which has 5
> significant digits, and thus really means 66625xxx Hz.
> Such an accuracy matches the accuracy of typical clock crystals.

OK, I got confused with clock list xls, where it mentions
533, 266.5, 133.25 and 66.625 MHz. As you said since it is not in Hz,
It may be the truncated values.

Cheers,
Biju

> 
> The figures only mention "1600 MHz" and "533 MHz".  The latter has only 3
> significant digits, and is probably just 1600 MHz / 3.
> All further dividers are documented to be powers of two.
> 
> > Yes, we need to have some relaxation for clocks as mentioned below.
> 
> Indeed. But this is IMHO something that should be handled by the code
> that looks for the best clock rate, instead of by fiddling with the
> dividers to get "nicely rounded" numbers.
> 
> > > Due to the (in)accuracy of clock crystals, the least significant
> > > digits in the above clock rates are not significant anyway.
> > >
> > > Perhaps the "if (freq > (new_clock << i))" check in
> > > renesas_sdhi_clk_update() can be slightly relaxed, so it allows e.g.
> > > a 0.1% (or 1/1024th?) higher clock rate than requested?
> > >
> > > > > > -               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2,
> 1,
> > > 3),
> > > > > > +               DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2,
> > > > > > + 533, 1600),
> > > > >
> > > > > I highly doubt the actual hardware is not using a by-3
> divider....
> 
> 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 mbox series

Patch

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 02a4fc41bb6e..bed41ed2c9ee 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -113,10 +113,10 @@  static const struct {
 		DEF_FIXED(".osc_div1000", CLK_OSC_DIV1000, CLK_EXTAL, 1, 1000),
 		DEF_SAMPLL(".pll1", CLK_PLL1, CLK_EXTAL, PLL146_CONF(0)),
 		DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 200, 3),
-		DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 1, 3),
+		DEF_FIXED(".pll2_533", CLK_PLL2_533, CLK_PLL2, 533, 1600),
 		DEF_FIXED(".pll3", CLK_PLL3, CLK_EXTAL, 200, 3),
 		DEF_FIXED(".pll3_400", CLK_PLL3_400, CLK_PLL3, 1, 4),
-		DEF_FIXED(".pll3_533", CLK_PLL3_533, CLK_PLL3, 1, 3),
+		DEF_FIXED(".pll3_533", CLK_PLL3_533, CLK_PLL3, 533, 1600),
 
 		DEF_FIXED(".pll5", CLK_PLL5, CLK_EXTAL, 125, 1),
 		DEF_FIXED(".pll5_fout3", CLK_PLL5_FOUT3, CLK_PLL5, 1, 6),
@@ -125,7 +125,7 @@  static const struct {
 
 		DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
 		DEF_FIXED(".clk_800", CLK_PLL2_800, CLK_PLL2, 1, 2),
-		DEF_FIXED(".clk_533", CLK_PLL2_SDHI_533, CLK_PLL2, 1, 3),
+		DEF_FIXED(".clk_533", CLK_PLL2_SDHI_533, CLK_PLL2, 533, 1600),
 		DEF_FIXED(".clk_400", CLK_PLL2_SDHI_400, CLK_PLL2_800, 1, 2),
 		DEF_FIXED(".clk_266", CLK_PLL2_SDHI_266, CLK_PLL2_SDHI_533, 1, 2),