Re: [PATCH] clk: sunxi: Fix mod0 clock calculation to return stable results and check divisor size limits
From: Maxime Ripard
Date: Tue Jan 26 2016 - 15:59:03 EST
Hi,
On Thu, Jan 14, 2016 at 11:40:15AM +0100, Marcus Weseloh wrote:
> > On Mon, Dec 28, 2015 at 06:31:32PM +0100, Marcus Weseloh wrote:
> >> This patch fixes some problems in the mod0 clock calculation. It has
> >> the potential to break stuff, as the issues explained below had the
> >> effect that clk_set_rate would always return successfully, sometimes
> >> setting a frequency that is higher than the requested value.
> >
> > That's actually the expected behaviour of clk_set_rate.
> >
> > clk_set_rate is supposed to adjust the given clock rate to something
> > that the clock drivers seems fit. It should only return an error in a
> > case where you can't change the rate at all (because you didn't pass a
> > valid struct clk pointer, because changing the rate would violate some
> > clock flags, etc.). Otherwise, clk_set_rate should succeed.
> >
> > By returning an error code the clock is higher than the one passed,
> > you violate that expectation, especially since that is relative to the
> > clock you passed.
> >
> > It makes sense in your case to never exceed the given rate, it might
> > not for a different clock in the tree, or even for a different
> > instance of the same clock. For example, you could very well have
> > another case in your system where you should not have rates set that
> > are below the one given because that would prevent the consumer
> > device to be usable.
> >
> > This is why the adjustment is left to the clock driver, and is not
> > enforced by the framework itself, simply because the framework has no
> > idea how you want to round your clock rate on that particular clock in
> > your system.
>
> I understand now, thanks a lot for the good explanation! So my
> thinking is wrong for the general case of the clock framework itself,
> and that actually makes a lot of sense.
>
> But the clk_factors_determine_rate function in
> drivers/clk/sunxi/clk-factors.c works on the assumption that the
> returned rate must be less or equal to the requested rate. At least
> that is what the code in that function tries to do. That the mod0
> factor calculation doesn't check the m and div variables for overflow
> undermines the intended behaviour, as it "lies" about the frequencies
> that the hardware can support.
Yeah, that's totally something that needs fixing.
> And for very low frequencies below 80kHz, clk_set_rate does
> currently return -EINVAL. There are even cases when it results in a
> division by zero error, for example if you request a rate of 94kHz
> from a 24Mhz parent (24000000 / 94000 = 255,32, rounded up to 256 =
> 0 on the u8 variable).
Yeah, the division by 0 is bad... However, can you even generate 80kHz
frequencies using a module clock? The lowest I can see here is 180kHz
(24M / 8 / 16).
> Now I'm unsure what to do here... If the clock driver should only
> return an error in real error cases and not when the requested
> frequency isn't reachable, then clk_factors_determine_rate needs to
> be changed as well?
If it returns an error in such a case, yeah. Also, I think in such a
case, we can simply round to the minimal frequency we can reach
(without an error or a kernel panic, that would be ideal ;))
> >> An example:
> >> parent_rate = 24Mhz, freq = 1.4Mhz results in p=1, m=9, freq=1333333,3333
> >> (which gets rounded down to 1333333).
> >> Calling the function again with parent_rate = 24Mhz and freq = 1333333
> >> results in p=1, m=10, freq=1200000.
> >>
> >> Rounding up the returned frequency removes this problem.
> >>
> >> Signed-off-by: Marcus Weseloh <mweseloh42@xxxxxxxxx>
> >> ---
> >> drivers/clk/sunxi/clk-mod0.c | 7 +++++--
> >> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/clk/sunxi/clk-mod0.c b/drivers/clk/sunxi/clk-mod0.c
> >> index d167e1e..d03f099 100644
> >> --- a/drivers/clk/sunxi/clk-mod0.c
> >> +++ b/drivers/clk/sunxi/clk-mod0.c
> >> @@ -31,7 +31,8 @@
> >> static void sun4i_a10_get_mod0_factors(u32 *freq, u32 parent_rate,
> >> u8 *n, u8 *k, u8 *m, u8 *p)
> >> {
> >> - u8 div, calcm, calcp;
> >> + unsigned int div, calcm;
> >> + u8 calcp;
> >>
> >> /* These clocks can only divide, so we will never be able to achieve
> >> * frequencies higher than the parent frequency */
> >> @@ -50,8 +51,10 @@ static void sun4i_a10_get_mod0_factors(u32 *freq, u32 parent_rate,
> >> calcp = 3;
> >>
> >> calcm = DIV_ROUND_UP(div, 1 << calcp);
> >> + if (calcm > 16)
> >> + calcm = 16;
> >>
> >> - *freq = (parent_rate >> calcp) / calcm;
> >> + *freq = DIV_ROUND_UP(parent_rate >> calcp, calcm);
> >
> > While the two above seems harmless, this one concerns me a bit. Did
> > you test the various mod0 clock users and made sure that they were
> > still working as they used to?
>
> No, I didn't do a thorough test, only booted the board with some mod0
> users (mmc, spi, ss) and watched them request their frequencies
> successfully. But this is an edge case, only affecting certain "weird"
> frequencies. And the only effect is that the chosen frequency is not
> the optimal one. So maybe I should drop it because it looks too
> disruptive for too little gain?
I'm guessing the other changes are fine. I'm a bit worried about this
one, but we still have quite some time before the 4.6 release. We can
always merge it, and deal with the fallout.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Attachment:
signature.asc
Description: Digital signature