lkml.org 
[lkml]   [2015]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 0/3] clk: divider: three exactness fixes (and a rant)
Date
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
>


\
 
 \ /
  Last update: 2015-03-06 20:01    [W:0.146 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site