Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Thu, Mar 15, 2012 at 2:43 AM, Sascha Hauer 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(1) returns 1, but clk_set_rate then
> sets the rate 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 60001 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- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Wed, Mar 14, 2012 at 05:51:48PM -0700, Turquette, Mike wrote:
> On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer 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
> >> 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
> 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(1) returns 1, but clk_set_rate then
sets the rate 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
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer 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 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
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;
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Tue, Mar 13, 2012 at 2:48 PM, Rob Herring wrote: > Mike, > > On 03/10/2012 01:54 AM, Mike Turquette wrote: >> The common clock framework defines a common struct clk useful across >> most platforms as well as an implementation of the clk api that drivers >> can use safely for managing clocks. >> >> The net result is consolidation of many different struct clk definitions >> and platform-specific clock framework implementations. >> >> This patch introduces the common struct clk, struct clk_ops and an >> implementation of the well-known clock api in include/clk/clk.h. >> Platforms may define their own hardware-specific clock structure and >> their own clock operation callbacks, so long as it wraps an instance of >> struct clk_hw. >> >> See Documentation/clk.txt for more details. >> >> This patch is based on the work of Jeremy Kerr, which in turn was based >> on the work of Ben Herrenschmidt. >> >> Signed-off-by: Mike Turquette >> Signed-off-by: Mike Turquette >> Cc: Russell King >> Cc: Jeremy Kerr >> Cc: Thomas Gleixner >> Cc: Arnd Bergman >> Cc: Paul Walmsley >> Cc: Shawn Guo >> Cc: Sascha Hauer >> Cc: Richard Zhao >> Cc: Saravana Kannan >> Cc: Magnus Damm >> Cc: Rob Herring >> Cc: Mark Brown >> Cc: Linus Walleij >> Cc: Stephen Boyd >> Cc: Amit Kucheria >> Cc: Deepak Saxena >> Cc: Grant Likely >> Cc: Andrew Lunn > > snip > >> + >> + /* >> + * walk the list of orphan clocks and reparent any that are children of >> + * this clock >> + */ >> + hlist_for_each_entry(orphan, tmp, &clk_orphan_list, child_node) > > In __clk_init, this needs to be hlist_for_each_entry_safe as entries can > be removed. Thanks for the catch Rob. I'll take this in. Regards, Mike > > Rob > > ___ > linaro-dev mailing list > linaro-dev@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-dev ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
Mike, On 03/10/2012 01:54 AM, Mike Turquette wrote: > The common clock framework defines a common struct clk useful across > most platforms as well as an implementation of the clk api that drivers > can use safely for managing clocks. > > The net result is consolidation of many different struct clk definitions > and platform-specific clock framework implementations. > > This patch introduces the common struct clk, struct clk_ops and an > implementation of the well-known clock api in include/clk/clk.h. > Platforms may define their own hardware-specific clock structure and > their own clock operation callbacks, so long as it wraps an instance of > struct clk_hw. > > See Documentation/clk.txt for more details. > > This patch is based on the work of Jeremy Kerr, which in turn was based > on the work of Ben Herrenschmidt. > > Signed-off-by: Mike Turquette > Signed-off-by: Mike Turquette > Cc: Russell King > Cc: Jeremy Kerr > Cc: Thomas Gleixner > Cc: Arnd Bergman > Cc: Paul Walmsley > Cc: Shawn Guo > Cc: Sascha Hauer > Cc: Richard Zhao > Cc: Saravana Kannan > Cc: Magnus Damm > Cc: Rob Herring > Cc: Mark Brown > Cc: Linus Walleij > Cc: Stephen Boyd > Cc: Amit Kucheria > Cc: Deepak Saxena > Cc: Grant Likely > Cc: Andrew Lunn snip > + > + /* > + * walk the list of orphan clocks and reparent any that are children of > + * this clock > + */ > + hlist_for_each_entry(orphan, tmp, &clk_orphan_list, child_node) In __clk_init, this needs to be hlist_for_each_entry_safe as entries can be removed. Rob ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
Hi Mike,
On Mon, Mar 12, 2012 at 08:16:36PM -0700, Turquette, Mike wrote:
> On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer wrote:
> > On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote:
> >> On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer
> >> wrote:
> >> > Hi Mike,
> >> >
> >> > I was about to give my tested-by when I decided to test the set_rate
> >> > function. Unfortunately this is broken for several reasons. I'll try
> >> > to come up with a fixup series later the day.
> >>
> >> I haven't tested clk_set_rate since V4, but I also haven't changed the
> >> code appreciably. I'll retest on my end also.
> >>
> >> > On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
> >> >> + /* find the new rate and see if parent rate should change too */
> >> >> + WARN_ON(!clk->ops->round_rate);
> >> >> +
> >> >> + new_rate = clk->ops->round_rate(clk->hw, rate, &parent_new_rate);
> >> >
> >> > You don't need a WARN_ON when you derefence clk->ops->round_rate anyway.
> >>
> >> Agreed that the WARN_ON should not be there.
> >>
> >> The v6 Documentation/clk.txt states that .round_rate is mandatory for
> >> clocks that can adjust their rate, but I need to clarify this a bit
> >> more. Ideally we want to be able to call clk_set_rate on any clock
> >> and get a changed rate (if possible) by either adjusting that clocks
> >> rate direction (e.g. a PLL or an adjustable divider) or by propagating
> >> __clk_set_rate up the parents (assuming of course that
> >> CLK_SET_RATE_PARENT flag is set appropriately).
> >>
> >> > Also, even when the current clock does not have a set_rate function it
> >> > can still change its rate when the CLK_SET_RATE_PARENT is set.
> >>
> >> Correct. I'll clean this up and make the documentation a bit more
> >> verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.
> >>
> >> >
> >> >> +
> >> >> + /* NOTE: pre-rate change notifications will stack */
> >> >> + if (clk->notifier_count)
> >> >> + ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate,
> >> >> new_rate);
> >> >> +
> >> >> + if (ret == NOTIFY_BAD)
> >> >> + return clk;
> >> >> +
> >> >> + /* speculate rate changes down the tree */
> >> >> + hlist_for_each_entry(child, tmp, &clk->children, child_node) {
> >> >> + ret = __clk_speculate_rates(child, new_rate);
> >> >> + if (ret == NOTIFY_BAD)
> >> >> + return clk;
> >> >> + }
> >> >> +
> >> >> + /* change the rate of this clk */
> >> >> + if (clk->ops->set_rate)
> >> >> + ret = clk->ops->set_rate(clk->hw, new_rate);
> >> >
> >> > I don't know the reason why you change the child clock before the parent
> >> > clock, but it cannot work since this clock will change its rate based on
> >> > the old parent rate and not the new one.
> >>
> >> This depends on the .round_rate implementation, which I admit to
> >> having lost some sleep over. A clever .round_rate will request the
> >> "intermediate" rate for a clock when propagating a request to change
> >> the parent rate later on. Take for instance the following:
> >>
> >> pll @ 200MHz (locked)
> >> |
> >> parent @ 100MHz (can divide by 1 or 2; currently divider is 2)
> >> |
> >> child @ 25MHz (can divide by 2 or 4; currently divider is 4)
> >>
> >> If we want child to run at 100MHz then the desirable configuration
> >> would be to have parent divide-by-1 and child divide-by-2. When we
> >> call,
> >>
> >> clk_set_rate(child, 100MHz);
> >>
> >> Its .round_rate should return 50MHz, and &parent_new_rate should be
> >> 200MHz. So 50MHz is an "intermediate" rate, but it gets us the
> >> divider we want. And in fact 50MHz reflects reality because that will
> >> be the rate of child until the parent propagation completes and we can
> >> adjust parent's dividers. (this is one reason why I prefer for
> >> pre-rate change notifiers to stack on top of each other).
> >>
> >> So now that &parent_new_rate is > 0, __clk_set_rate will propagate the
> >> request up and parent's .round_rate will simply return 200MHz and
> >> leave it's own &parent_new_rate at 0. This will change from
> >> divide-by-2 to divide-by-1 and from this highest point in the tree we
> >> will propagate post-rate change notifiers downstream, as part of the
> >> recalc_rate tree walk.
> >>
> >> I have tested this with OMAP4's CPUfreq driver and I think, while
> >> complicated, it is a sound way to approach the problem. Maybe the API
> >> can be cleaned up, if you have any suggestions.
> >
> > I cannot see all implications this way will have. All this rate
> > propagation is more complex than I thought it would be.
>
> Hi Sascha,
>
> Yes it is very complicated. The solution I have now (recursive
> __clk_set_rate, clever .round_rate which requests parent rate) was not
> something I arrived at immediately.
>
> I decided to validate the v6 patches more thoroughly today, based on
> your claim that clk_set_rate is broken and h
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer wrote:
> On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote:
>> On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer wrote:
>> > Hi Mike,
>> >
>> > I was about to give my tested-by when I decided to test the set_rate
>> > function. Unfortunately this is broken for several reasons. I'll try
>> > to come up with a fixup series later the day.
>>
>> I haven't tested clk_set_rate since V4, but I also haven't changed the
>> code appreciably. I'll retest on my end also.
>>
>> > On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
>> >> + /* find the new rate and see if parent rate should change too */
>> >> + WARN_ON(!clk->ops->round_rate);
>> >> +
>> >> + new_rate = clk->ops->round_rate(clk->hw, rate, &parent_new_rate);
>> >
>> > You don't need a WARN_ON when you derefence clk->ops->round_rate anyway.
>>
>> Agreed that the WARN_ON should not be there.
>>
>> The v6 Documentation/clk.txt states that .round_rate is mandatory for
>> clocks that can adjust their rate, but I need to clarify this a bit
>> more. Ideally we want to be able to call clk_set_rate on any clock
>> and get a changed rate (if possible) by either adjusting that clocks
>> rate direction (e.g. a PLL or an adjustable divider) or by propagating
>> __clk_set_rate up the parents (assuming of course that
>> CLK_SET_RATE_PARENT flag is set appropriately).
>>
>> > Also, even when the current clock does not have a set_rate function it
>> > can still change its rate when the CLK_SET_RATE_PARENT is set.
>>
>> Correct. I'll clean this up and make the documentation a bit more
>> verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.
>>
>> >
>> >> +
>> >> + /* NOTE: pre-rate change notifications will stack */
>> >> + if (clk->notifier_count)
>> >> + ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate,
>> >> new_rate);
>> >> +
>> >> + if (ret == NOTIFY_BAD)
>> >> + return clk;
>> >> +
>> >> + /* speculate rate changes down the tree */
>> >> + hlist_for_each_entry(child, tmp, &clk->children, child_node) {
>> >> + ret = __clk_speculate_rates(child, new_rate);
>> >> + if (ret == NOTIFY_BAD)
>> >> + return clk;
>> >> + }
>> >> +
>> >> + /* change the rate of this clk */
>> >> + if (clk->ops->set_rate)
>> >> + ret = clk->ops->set_rate(clk->hw, new_rate);
>> >
>> > I don't know the reason why you change the child clock before the parent
>> > clock, but it cannot work since this clock will change its rate based on
>> > the old parent rate and not the new one.
>>
>> This depends on the .round_rate implementation, which I admit to
>> having lost some sleep over. A clever .round_rate will request the
>> "intermediate" rate for a clock when propagating a request to change
>> the parent rate later on. Take for instance the following:
>>
>> pll @ 200MHz (locked)
>> |
>> parent @ 100MHz (can divide by 1 or 2; currently divider is 2)
>> |
>> child @ 25MHz (can divide by 2 or 4; currently divider is 4)
>>
>> If we want child to run at 100MHz then the desirable configuration
>> would be to have parent divide-by-1 and child divide-by-2. When we
>> call,
>>
>> clk_set_rate(child, 100MHz);
>>
>> Its .round_rate should return 50MHz, and &parent_new_rate should be
>> 200MHz. So 50MHz is an "intermediate" rate, but it gets us the
>> divider we want. And in fact 50MHz reflects reality because that will
>> be the rate of child until the parent propagation completes and we can
>> adjust parent's dividers. (this is one reason why I prefer for
>> pre-rate change notifiers to stack on top of each other).
>>
>> So now that &parent_new_rate is > 0, __clk_set_rate will propagate the
>> request up and parent's .round_rate will simply return 200MHz and
>> leave it's own &parent_new_rate at 0. This will change from
>> divide-by-2 to divide-by-1 and from this highest point in the tree we
>> will propagate post-rate change notifiers downstream, as part of the
>> recalc_rate tree walk.
>>
>> I have tested this with OMAP4's CPUfreq driver and I think, while
>> complicated, it is a sound way to approach the problem. Maybe the API
>> can be cleaned up, if you have any suggestions.
>
> I cannot see all implications this way will have. All this rate
> propagation is more complex than I thought it would be.
Hi Sascha,
Yes it is very complicated. The solution I have now (recursive
__clk_set_rate, clever .round_rate which requests parent rate) was not
something I arrived at immediately.
I decided to validate the v6 patches more thoroughly today, based on
your claim that clk_set_rate is broken and here is what I found:
1) clk_set_rate works. I pulled in the latest OMAP4 CPUfreq code into
my common clk branch and it Just Worked. This is a dumb
implementation involving no upwards parent propagation, and the clock
changing is of type struct clk_hw_omap (relocking a PLL)
2) while I was at
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote:
> On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer wrote:
> > Hi Mike,
> >
> > I was about to give my tested-by when I decided to test the set_rate
> > function. Unfortunately this is broken for several reasons. I'll try
> > to come up with a fixup series later the day.
>
> I haven't tested clk_set_rate since V4, but I also haven't changed the
> code appreciably. I'll retest on my end also.
>
> > On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
> >> + /* find the new rate and see if parent rate should change too */
> >> + WARN_ON(!clk->ops->round_rate);
> >> +
> >> + new_rate = clk->ops->round_rate(clk->hw, rate, &parent_new_rate);
> >
> > You don't need a WARN_ON when you derefence clk->ops->round_rate anyway.
>
> Agreed that the WARN_ON should not be there.
>
> The v6 Documentation/clk.txt states that .round_rate is mandatory for
> clocks that can adjust their rate, but I need to clarify this a bit
> more. Ideally we want to be able to call clk_set_rate on any clock
> and get a changed rate (if possible) by either adjusting that clocks
> rate direction (e.g. a PLL or an adjustable divider) or by propagating
> __clk_set_rate up the parents (assuming of course that
> CLK_SET_RATE_PARENT flag is set appropriately).
>
> > Also, even when the current clock does not have a set_rate function it
> > can still change its rate when the CLK_SET_RATE_PARENT is set.
>
> Correct. I'll clean this up and make the documentation a bit more
> verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.
>
> >
> >> +
> >> + /* NOTE: pre-rate change notifications will stack */
> >> + if (clk->notifier_count)
> >> + ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate,
> >> new_rate);
> >> +
> >> + if (ret == NOTIFY_BAD)
> >> + return clk;
> >> +
> >> + /* speculate rate changes down the tree */
> >> + hlist_for_each_entry(child, tmp, &clk->children, child_node) {
> >> + ret = __clk_speculate_rates(child, new_rate);
> >> + if (ret == NOTIFY_BAD)
> >> + return clk;
> >> + }
> >> +
> >> + /* change the rate of this clk */
> >> + if (clk->ops->set_rate)
> >> + ret = clk->ops->set_rate(clk->hw, new_rate);
> >
> > I don't know the reason why you change the child clock before the parent
> > clock, but it cannot work since this clock will change its rate based on
> > the old parent rate and not the new one.
>
> This depends on the .round_rate implementation, which I admit to
> having lost some sleep over. A clever .round_rate will request the
> "intermediate" rate for a clock when propagating a request to change
> the parent rate later on. Take for instance the following:
>
> pll @ 200MHz (locked)
> |
> parent @ 100MHz (can divide by 1 or 2; currently divider is 2)
> |
> child @ 25MHz (can divide by 2 or 4; currently divider is 4)
>
> If we want child to run at 100MHz then the desirable configuration
> would be to have parent divide-by-1 and child divide-by-2. When we
> call,
>
> clk_set_rate(child, 100MHz);
>
> Its .round_rate should return 50MHz, and &parent_new_rate should be
> 200MHz. So 50MHz is an "intermediate" rate, but it gets us the
> divider we want. And in fact 50MHz reflects reality because that will
> be the rate of child until the parent propagation completes and we can
> adjust parent's dividers. (this is one reason why I prefer for
> pre-rate change notifiers to stack on top of each other).
>
> So now that &parent_new_rate is > 0, __clk_set_rate will propagate the
> request up and parent's .round_rate will simply return 200MHz and
> leave it's own &parent_new_rate at 0. This will change from
> divide-by-2 to divide-by-1 and from this highest point in the tree we
> will propagate post-rate change notifiers downstream, as part of the
> recalc_rate tree walk.
>
> I have tested this with OMAP4's CPUfreq driver and I think, while
> complicated, it is a sound way to approach the problem. Maybe the API
> can be cleaned up, if you have any suggestions.
I cannot see all implications this way will have. All this rate
propagation is more complex than I thought it would be. 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
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer wrote:
> Hi Mike,
>
> I was about to give my tested-by when I decided to test the set_rate
> function. Unfortunately this is broken for several reasons. I'll try
> to come up with a fixup series later the day.
I haven't tested clk_set_rate since V4, but I also haven't changed the
code appreciably. I'll retest on my end also.
> On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
>> + /* find the new rate and see if parent rate should change too */
>> + WARN_ON(!clk->ops->round_rate);
>> +
>> + new_rate = clk->ops->round_rate(clk->hw, rate, &parent_new_rate);
>
> You don't need a WARN_ON when you derefence clk->ops->round_rate anyway.
Agreed that the WARN_ON should not be there.
The v6 Documentation/clk.txt states that .round_rate is mandatory for
clocks that can adjust their rate, but I need to clarify this a bit
more. Ideally we want to be able to call clk_set_rate on any clock
and get a changed rate (if possible) by either adjusting that clocks
rate direction (e.g. a PLL or an adjustable divider) or by propagating
__clk_set_rate up the parents (assuming of course that
CLK_SET_RATE_PARENT flag is set appropriately).
> Also, even when the current clock does not have a set_rate function it
> can still change its rate when the CLK_SET_RATE_PARENT is set.
Correct. I'll clean this up and make the documentation a bit more
verbose on when .set_rate/.round_rate/.recalc_rate are mandatory.
>
>> +
>> + /* NOTE: pre-rate change notifications will stack */
>> + if (clk->notifier_count)
>> + ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate);
>> +
>> + if (ret == NOTIFY_BAD)
>> + return clk;
>> +
>> + /* speculate rate changes down the tree */
>> + hlist_for_each_entry(child, tmp, &clk->children, child_node) {
>> + ret = __clk_speculate_rates(child, new_rate);
>> + if (ret == NOTIFY_BAD)
>> + return clk;
>> + }
>> +
>> + /* change the rate of this clk */
>> + if (clk->ops->set_rate)
>> + ret = clk->ops->set_rate(clk->hw, new_rate);
>
> I don't know the reason why you change the child clock before the parent
> clock, but it cannot work since this clock will change its rate based on
> the old parent rate and not the new one.
This depends on the .round_rate implementation, which I admit to
having lost some sleep over. A clever .round_rate will request the
"intermediate" rate for a clock when propagating a request to change
the parent rate later on. Take for instance the following:
pll @ 200MHz (locked)
|
parent @ 100MHz (can divide by 1 or 2; currently divider is 2)
|
child @ 25MHz (can divide by 2 or 4; currently divider is 4)
If we want child to run at 100MHz then the desirable configuration
would be to have parent divide-by-1 and child divide-by-2. When we
call,
clk_set_rate(child, 100MHz);
Its .round_rate should return 50MHz, and &parent_new_rate should be
200MHz. So 50MHz is an "intermediate" rate, but it gets us the
divider we want. And in fact 50MHz reflects reality because that will
be the rate of child until the parent propagation completes and we can
adjust parent's dividers. (this is one reason why I prefer for
pre-rate change notifiers to stack on top of each other).
So now that &parent_new_rate is > 0, __clk_set_rate will propagate the
request up and parent's .round_rate will simply return 200MHz and
leave it's own &parent_new_rate at 0. This will change from
divide-by-2 to divide-by-1 and from this highest point in the tree we
will propagate post-rate change notifiers downstream, as part of the
recalc_rate tree walk.
I have tested this with OMAP4's CPUfreq driver and I think, while
complicated, it is a sound way to approach the problem. Maybe the API
can be cleaned up, if you have any suggestions.
Regards,
Mike
>
> There are more things, as said I'll try to come up with a fixup series.
>
> 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- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Sat, Mar 10, 2012 at 11:52 PM, Richard Zhao wrote: > Looks like you didn't take my comments for v5. > http://www.spinics.net/lists/arm-kernel/msg162903.html Sorry Richard, that one slipped through the cracks. I'll publish a new version tomorrow with some of those fixes. Some of the others (such as multiple pre-rate change notifiers) I won't take in, but we can discuss more. Regards, Mike > > Regards > Richard ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
Hi Mike,
I was about to give my tested-by when I decided to test the set_rate
function. Unfortunately this is broken for several reasons. I'll try
to come up with a fixup series later the day.
On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote:
> +
> +/**
> + * DOC: Using the CLK_SET_RATE_PARENT flag
> + *
> + * __clk_set_rate changes the child's rate before the parent's to more
> + * easily handle failure conditions.
> + *
> + * This means clk might run out of spec for a short time if its rate is
> + * increased before the parent's rate is updated.
> + *
> + * To prevent this consider setting the CLK_SET_RATE_GATE flag on any
> + * clk where you also set the CLK_SET_RATE_PARENT flag
> + *
> + * PRE_RATE_CHANGE notifications are supposed to stack as a rate change
> + * request propagates up the clk tree. This reflects the different
> + * rates that a downstream clk might experience if left enabled while
> + * upstream parents change their rates.
> + */
> +static struct clk *__clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> + struct clk *fail_clk = NULL;
> + int ret = NOTIFY_DONE;
> + unsigned long old_rate = clk->rate;
> + unsigned long new_rate;
> + unsigned long parent_old_rate;
> + unsigned long parent_new_rate = 0;
> + struct clk *child;
> + struct hlist_node *tmp;
> +
> + /* bail early if we can't change rate while clk is enabled */
> + if ((clk->flags & CLK_SET_RATE_GATE) && clk->enable_count)
> + return clk;
> +
> + /* find the new rate and see if parent rate should change too */
> + WARN_ON(!clk->ops->round_rate);
> +
> + new_rate = clk->ops->round_rate(clk->hw, rate, &parent_new_rate);
You don't need a WARN_ON when you derefence clk->ops->round_rate anyway.
Also, even when the current clock does not have a set_rate function it
can still change its rate when the CLK_SET_RATE_PARENT is set.
> +
> + /* NOTE: pre-rate change notifications will stack */
> + if (clk->notifier_count)
> + ret = __clk_notify(clk, PRE_RATE_CHANGE, clk->rate, new_rate);
> +
> + if (ret == NOTIFY_BAD)
> + return clk;
> +
> + /* speculate rate changes down the tree */
> + hlist_for_each_entry(child, tmp, &clk->children, child_node) {
> + ret = __clk_speculate_rates(child, new_rate);
> + if (ret == NOTIFY_BAD)
> + return clk;
> + }
> +
> + /* change the rate of this clk */
> + if (clk->ops->set_rate)
> + ret = clk->ops->set_rate(clk->hw, new_rate);
I don't know the reason why you change the child clock before the parent
clock, but it cannot work since this clock will change its rate based on
the old parent rate and not the new one.
There are more things, as said I'll try to come up with a fixup series.
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- |
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
Looks like you didn't take my comments for v5. http://www.spinics.net/lists/arm-kernel/msg162903.html Regards Richard ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Fri, 9 Mar 2012, Mike Turquette wrote:
> +inline unsigned long __clk_get_enable_count(struct clk *clk)
> +{
> + return !clk ? -EINVAL : clk->enable_count;
Returning negative error codes in a function with a return value
unsigned long is a bit strange at least. Shouldn't that be long ?
> +#ifndef __LINUX_CLK_PRIVATE_H
> +#define __LINUX_CLK_PRIVATE_H
> +
> +#include
> +#include
> +
> +/*
> + * WARNING: Do not include clk-private.h from any file that implements struct
> + * clk_ops. Doing so is a layering violation!
> + *
> + * This header exists only to allow for statically initialized clock data.
> Any
> + * static clock data must be defined in a separate file from the logic that
> + * implements the clock operations for that same data.
Now the question is whether you should provide a data structure which
is explicitely used for static initialization and instead of having
struct clk static you register the static initializer structure, which
would be initdata. I don't think that anything needs clocks before the
memory allocators are up and running. The clocks which are necessary
to get that far have to be enabled in the boot loader anyway.
The static initialization question should not hold off this set from
being merged, though settling it before growing users would be nice.
Otherwise this is a very well done infrastructure implementation!
Thanks a lot Mike!
Reviewed-by: Thomas Gleixner
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Fri, Mar 09, 2012 at 11:54:23PM -0800, Mike Turquette wrote: > The common clock framework defines a common struct clk useful across > most platforms as well as an implementation of the clk api that drivers > can use safely for managing clocks. Hi Mike Please feel free to add: Tested-by: Andrew Lunn Andrew ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
