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