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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> > > 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@xxxxxxxxxxxxxx>
> > > > > 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
