Re: [PATCH 0/3] clk: divider: three exactness fixes (and a rant)

[Mike Turquette <mturquette@linaro.org>]

Quoting Uwe Kleine-König (2015-02-21 02:40:22)
> Hello,
> 
> TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST.
> 
> I stared at clk-divider.c for some time now given Sascha's failing test
> case. I found a fix for the failure (which happens to be what Sascha
> suspected).
> 
> The other two patches fix problems only present when handling dividers
> that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still
> heavily broken however. So having a 4bit-divider and a parent clk of
> 10000 (as in Sascha's test case) requesting
> 
>         clk_set_rate(clk, 666)
> 
> sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the
> choice of parent_rate in clk_divider_bestdiv's loop is wrong for
> CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is
> non-trivial and for sure more than one rate must be tested here. This is
> complicated by the fact that clk_round_rate might return a value bigger
> than the requested rate which convinces me (once more) that it's a bad
> idea to allow that. Even if this was fixed for .round_rate,
> clk_divider_set_rate is still broken because it also uses
> 
>         div = DIV_ROUND_UP(parent_rate, rate);
> 
> to calculate the (pretended) best divider to get near rate.
> 
> Note this makes at least two reasons to remove support for
> CLK_DIVIDER_ROUND_CLOSEST!
> 
> Instead I'd favour creating a function
> 
>         clk_round_rate_nearest
> 
> as was suggested some time ago by Soren Brinkmann and me[1] that doesn't

Uwe,

Thanks for the fixes. I'm thinking of taking all three for 4.0. I also
agree on clk_round_rate_nearest (along with a _ceil and _floor version
as well). That's something we can do for 4.1 probably.

There are currently 3 users of CLK_DIVIDER_ROUND_CLOSEST:

Loongson
QCOM
ST

So now is probably the right time to remove the flag if we're going to
do it.

What do you think?

Regards,
Mike

> need any clk type specific knowledge. This would mean that not the
> divider (or clk in general) would have to know that returning a slightly
> bigger rate than requested is OK but the caller which is fine (and even
> better) in my eyes. This would simplify clk-divider.c (and probably
> others) and give support for "nearest match" for all clock types without
> type specific implementation. (Note that it might even make sense to use
> a different metric for "nearest", instead of minimizing
> 
>         abs(target - rate)
> 
> you might want to minimize
> 
>         abs(target / rate - 1)
> 
> instead.
> 
> Converting the clk framework to 64 bit rates was discussed earlier
> already, too, and I wonder if we should fix rounding issues (a bit) in
> the same transition such that 
> 
>         clk_set_rate(clk, 333)
> 
> allows the clk to be set to 333.3333333333 Hz and let clk_get_rate
> return 333 in this case.
> 
> Also I'd vote to return 0 or -ESOMETHING if a requested rate is too low
> to be set. This would simplify some special casing I think and makes the
> request
> 
>         clk_round_rate(clk, x) <= x
> 
> consistent.
> 
> Best regards
> Uwe
> 
> [1] https://lkml.org/lkml/2014/5/14/698
> 
> Uwe Kleine-König (3):
>   clk: divider: fix calculation of maximal parent rate for a given
>     divider
>   clk: divider: fix selection of divider when rounding to closest
>   clk: divider: fix calculation of initial best divider when rounding to
>     closest
> 
>  drivers/clk/clk-divider.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> -- 
> 2.1.4
>