Hi Biju,
On Wed, Sep 14, 2022 at 6:44 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > Subject: RE: [PATCH] mmc: renesas_sdhi: Add margins for main clock and hs
> > clock
> > > Subject: Re: [PATCH] mmc: renesas_sdhi: Add margins for main clock and
> > > hs clock
> > > On Tue, Sep 13, 2022 at 5:15 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > wrote:
> > > > The SDHI high speed clock is 4 times that of the main clock.
> > > > Currently, there is no margin added for setting the rate associated
> > > > with these clocks. On RZ/G2L platforms, the lack of these margins
> > > > leads to the selection of a clock source with a lower clock rate
> > > > compared to a higher one.
> > > >
> > > > RZ/G2L example case:
> > > > SD0 hs clock is 533333333 Hz and SD0 main clock is 133333333 Hz.
> > > > When we set the rate for the main clock 133333333, the request rate
> > > > for the parent becomes 533333332 (133333333 * 4) and the SD0 hs
> > > > clock is set as 400000000 Hz.
> > > >
> > > > This patch adds a margin of (1/1024) higher hs clock and main clock
> > > rate.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > > > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > > > @@ -147,6 +147,7 @@ static unsigned int
> > > > renesas_sdhi_clk_update(struct
> > > tmio_mmc_host *host,
> > > > }
> > > >
> > > > new_clock = wanted_clock << clkh_shift;
> > > > + new_clock += new_clock >> 10;
> > >
> > > This changes the requested clock rate. Is that really a good idea?
> > >
> > > Isn't it sufficient to change the test "if (freq > (new_clock << i))"
> > > slightly?
> >
> > We hardly enter this test, after it request for proper wanted_clk.
> >
> > freq is clk_round_rate(ref_clk, new_clock << i); and it compares 400MHz
> > vs 533.332 MHz.
> >
> > Basically wanted_clock= 133.33333 MHz and is scaled to power of 2 and
> > then each iteration it scale down to power of 2 compare with round rate
> > value.
> >
> > [ 4.020781] ####rzg2l_cpg_sd_clk_mux_determine_rate####### req-
> > >rate=4266666656
> > [ 4.028013] ####rzg2l_cpg_sd_clk_mux_determine_rate####### req-
> > >rate=2133333328
> > [ 4.035330] ####rzg2l_cpg_sd_clk_mux_determine_rate####### req-
> > >rate=1066666664
> > [ 4.042639] ####rzg2l_cpg_sd_clk_mux_determine_rate####### req-
> > >rate=533333332
> >
> > >
> > > >
> > > > /*
> > > > * We want the bus clock to be as close as possible to, but
> > > > no @@ -172,6 +173,7 @@ static unsigned int
> > > > renesas_sdhi_clk_update(struct tmio_mmc_host *host,
> > > >
> > > > clk_set_rate(ref_clk, best_freq);
> > > >
> > > > + best_freq += best_freq >> 10;
> > >
> > > Can you please elaborate why this is needed?
> > > It looks counter-intuitive to me.
> >
> > When we try to set 133.333 MHz clk, the determine rate calculates req-
> > >rate for sd clk as 533.332 and since available clock source for sd0 clks
> > are 266.6666, 400 and
> > 533.333 MHz, so it select the clock source as 400MHz.
>
> Just to add here the main issue is coming from math calculation.
>
> Consider any value A
>
> B = A / 4
> C = B * 4
>
> Ideally, we expect A = C, But in the below example
> It is not the case (it is A != C).
>
> A = 533333333 (1600/3 MHz)
> B = 533333333/4 = 133333333
> C = 133333333 * 4 = 533333332
>
> Since A != C we are ending up in this situation.
I am fully aware of that ;-)
However, clk_round_rate() is supposed to return the closest
matching rate. Hence when passed 533333332, it should return 533333333.
AFAIU, this is then rejected by "if (freq > (new_clock << i))", due to
being slightly too large, causing 400000000 to be picked instead.
Am I missing something?
I'm currently not in the position to test/verify what is actually happening.
I do see clk_mux_determine_rate_flags() has a comment:
/* find the parent that can provide the fastest rate <= rate */
So perhaps the issue is that it does not return the closest rate, but a
slower rate?
Thanks!
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
