Re: [PATCH v6 2/3] clk: introduce the common clock framework

[Turquette, Mike]

On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:
> On Mon, Mar 12, 2012 at 08:16:36PM -0700, Turquette, Mike wrote:
>> On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:
>> > I tried another
>> > approach on the weekend which basically does not try to do all in a
>> > single recursion but instead sets the rate in multiple steps:
>> >
>> > 1) call a function which calculates all new rates of affected clocks
>> >   in a rate change and safes the value in a clk->new_rate field. This
>> >   function returns the topmost clock which has to be changed.
>> > 2) starting from the topmost clock notify all clients. This walks the
>> >   whole subtree even if a notfifier refuses the change. If necessary
>> >   we can walk the whole subtree again to abort the change.
>> > 3) actually change rates starting from the topmost clocks and notify
>> >   all clients on the way. I changed the set_rate callback to void.
>> >   Instead of failing (what is failing in case of set_rate? The clock
>> >   will still have some rate) I check for the result with
>> >   clk_ops->recalc_rate.
>
> The way described above works for me now, see this branch:
>
> git://git.pengutronix.de/git/imx/linux-2.6.git v3.3-rc6-clkv6-fixup
>
> You may not necessarily like it as it changes quite a lot in the rate
> changing code.

I tried that code and I really like it!  It is much more readable and
feels less "fragile" than the previous recursive __clk_set_rate.  I
did quite a bit of testing with this code today.  One of the tests
looks like this:

    pll         (adjustable to anything)
     |
clk_divider     (5 bits wide)
     |
   dummy        (no clk_ops)

The new code did a fine job arbitrating rates for the PLL and the
intermediate divider even if I put weird constraints on the PLL.  For
instance if I artificially limited it to a minimum of 600MHz and then
ran clk_set_rate(dummy, 300MHz) it would lock at 600MHz and set
clk_divider to divide-by-2.  Setting to 600MHz or more set the divider
back to 1 and relocked the PLL appropriately.  Pretty cool.

I also tested the notifiers with this code and they seem to function
properly.  I'll take this code in for v7.  Thanks a lot for this
helpful contribution.

I did find that MULT_ROUND_UP caused trouble for my PLL's round_rate
implementation.  Maybe my PLL code is fragile but a quick fix was to
make sure that we send the exact value we want to the round_rate code.
 I also feel this is more correct.  Let me know what you think:

8<---------------------------------------------------------------

commit 189fecedb175d0366759246c4192f45b0bc39a50
Author: Mike Turquette <mturquette@xxxxxxxxxx>
Date:   Wed Mar 14 17:29:51 2012 -0700

    clk-divider.c: round the actual rate we care about

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 86ca9cd..06ef4a0 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -47,12 +47,6 @@ static unsigned long clk_divider_recalc_rate(struct
clk_hw *hw,
 }
 EXPORT_SYMBOL_GPL(clk_divider_recalc_rate);

-/*
- * The reverse of DIV_ROUND_UP: The maximum number which
- * divided by m is r
- */
-#define MULT_ROUND_UP(r, m) ((r) * (m) + (m) - 1)
-
 static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
 		unsigned long *best_parent_rate)
 {
@@ -84,9 +78,9 @@ static int clk_divider_bestdiv(struct clk_hw *hw,
unsigned long rate,

 	for (i = 1; i <= maxdiv; i++) {
 		parent_rate = __clk_round_rate(__clk_get_parent(hw->clk),
-				MULT_ROUND_UP(rate, i));
+				(rate * i));
 		now = parent_rate / i;
-		if (now <= rate && now >= best) {
+		if (now <= rate && now > best) {
 			bestdiv = i;
 			best = now;
 			*best_parent_rate = parent_rate;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/