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

Message ID CAJOA=zMT4vK7WPF7iEH9kN6GHHAv-LChHoadzieqV-Wd3Qj=WQ@mail.gmail.com
State New
Headers show

Commit Message

Mike Turquette March 15, 2012, 12:51 a.m.
On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer <s.hauer@pengutronix.de> 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@pengutronix.de> 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@linaro.org>
Date:   Wed Mar 14 17:29:51 2012 -0700

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

 {
@@ -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;

Comments

Sascha Hauer March 15, 2012, 9:43 a.m. | #1
On Wed, Mar 14, 2012 at 05:51:48PM -0700, Turquette, Mike wrote:
> On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer <s.hauer@pengutronix.de> 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@pengutronix.de> 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@linaro.org>
> 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));

I think MULT_ROUND_UP is the right thing to use here (not sure if this
is a good name though)
Consider we want to have an output rate of 33Hz. Now acceptable input
rates for a divider value of 3 would be 99, 100 and 101Hz, so we have
to call round_rate for the parent with 101Hz which includes 100 and
99Hz.

If you have problems with your PLL than most likely because it does
something different on clk_round_rate than it does in clk_set_rate,
for example clk_round_rate(10000) returns 10000, but clk_set_rate then
sets the rate 9999 due to some rounding error. Being consistent between
round_rate and set_rate is very important for this mechanism to work
properly. It did cost me some nerves to get it right for the divider
(and even more nerves to figure out why it is correct the way it works)

>  		now = parent_rate / i;
> -		if (now <= rate && now >= best) {
> +		if (now <= rate && now > best) {

This change is an optimization, but should be unrelated to your PLL
problem, right?

Sascha
Mike Turquette March 16, 2012, 6:22 a.m. | #2
On Thu, Mar 15, 2012 at 2:43 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Mar 14, 2012 at 05:51:48PM -0700, Turquette, Mike wrote:
>> @@ -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));
>
> I think MULT_ROUND_UP is the right thing to use here (not sure if this
> is a good name though)
> Consider we want to have an output rate of 33Hz. Now acceptable input
> rates for a divider value of 3 would be 99, 100 and 101Hz, so we have
> to call round_rate for the parent with 101Hz which includes 100 and
> 99Hz.

We're back to the point Rob brought up about .round_rate rounding up
or down.  We really need a least-upper-bounds or greatest-lower-bounds
flag, similar to how CPUfreq selects target frequencies today.  I'm
freezing features for this patchset now, so I'll keep your
MULT_ROUND_UP approach and some day I'll fix .round_rate for good.

> If you have problems with your PLL than most likely because it does
> something different on clk_round_rate than it does in clk_set_rate,
> for example clk_round_rate(10000) returns 10000, but clk_set_rate then
> sets the rate 9999 due to some rounding error. Being consistent between
> round_rate and set_rate is very important for this mechanism to work
> properly. It did cost me some nerves to get it right for the divider
> (and even more nerves to figure out why it is correct the way it works)

What I'd like to do is request the *exact* frequency I want when
passing in a rate to my PLLs .round_rate.  Due to the way that my MN
dividers are calculated, and due to some jitter avoidance code, it is
bad for me to request 600000001 Hz when I really want 600MHz exactly.
Anyways I fixed this up on my end enough to work with MULT_ROUND_UP,
so that can stay for the immediate future.

>
>>               now = parent_rate / i;
>> -             if (now <= rate && now >= best) {
>> +             if (now <= rate && now > best) {
>
> This change is an optimization, but should be unrelated to your PLL
> problem, right?

MULT_ROUND_UP yields multiples of 'rate' plus an incrementing value (m
- 1).  Without that incrementing value added to the rate passed into
__clk_round_rate the for loop above will always max out the divider.
To illustrate:

The rate we want is 300MHz.  Without MULT_ROUND_UP the for loop will
start yielding these combinations:

__clk_round_rate(parent, 300MHz), divide-by-1
__clk_round_rate(parent, 600MHz), divide-by-2
__clk_round_rate(parent, 900MHz), divide-by-3
__clk_round_rate(parent, 1200MHz), divide-by-4
...etc...

These all yield the desired 300MHz for our divider so it just keeps
going until you max out the divider and requests some crazy rate for
the parent.  On most hardware that I am aware of it is desirable to
keep the divider as low as possible so the change to the conditional
prevents us from overwriting best every single time while keeping the
divider low.  So yes, it is an optimization, but it is also quite
necessary without MULT_ROUND_UP.

Anyways I've kept MULT_ROUND_UP exactly as-is with the exception that
I've added that small optimization to keep dividers low.  I hope there
are no objections to that.

Regards,
Mike

>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Patch

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)