Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Thu, Mar 15, 2012 at 2:43 AM, Sascha Hauer s.ha...@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(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 s.ha...@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.ha...@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 mturque...@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(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/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Tue, Mar 13, 2012 at 5:05 AM, Sascha Hauer s.ha...@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.ha...@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 mturque...@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));
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
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 s.ha...@pengutronix.de 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 s.ha...@pengutronix.de
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 it I verified the rate change
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 mturque...@linaro.org Signed-off-by: Mike Turquette mturque...@ti.com Cc: Russell King li...@arm.linux.org.uk Cc: Jeremy Kerr jeremy.k...@canonical.com Cc: Thomas Gleixner t...@linutronix.de Cc: Arnd Bergman arnd.bergm...@linaro.org Cc: Paul Walmsley p...@pwsan.com Cc: Shawn Guo shawn@freescale.com Cc: Sascha Hauer s.ha...@pengutronix.de Cc: Richard Zhao richard.z...@linaro.org Cc: Saravana Kannan skan...@codeaurora.org Cc: Magnus Damm magnus.d...@gmail.com Cc: Rob Herring rob.herr...@calxeda.com Cc: Mark Brown broo...@opensource.wolfsonmicro.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Stephen Boyd sb...@codeaurora.org Cc: Amit Kucheria amit.kuche...@linaro.org Cc: Deepak Saxena dsax...@linaro.org Cc: Grant Likely grant.lik...@secretlab.ca Cc: Andrew Lunn and...@lunn.ch 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
On Tue, Mar 13, 2012 at 2:48 PM, Rob Herring robherri...@gmail.com 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 mturque...@linaro.org Signed-off-by: Mike Turquette mturque...@ti.com Cc: Russell King li...@arm.linux.org.uk Cc: Jeremy Kerr jeremy.k...@canonical.com Cc: Thomas Gleixner t...@linutronix.de Cc: Arnd Bergman arnd.bergm...@linaro.org Cc: Paul Walmsley p...@pwsan.com Cc: Shawn Guo shawn@freescale.com Cc: Sascha Hauer s.ha...@pengutronix.de Cc: Richard Zhao richard.z...@linaro.org Cc: Saravana Kannan skan...@codeaurora.org Cc: Magnus Damm magnus.d...@gmail.com Cc: Rob Herring rob.herr...@calxeda.com Cc: Mark Brown broo...@opensource.wolfsonmicro.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Stephen Boyd sb...@codeaurora.org Cc: Amit Kucheria amit.kuche...@linaro.org Cc: Deepak Saxena dsax...@linaro.org Cc: Grant Likely grant.lik...@secretlab.ca Cc: Andrew Lunn and...@lunn.ch 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
On Sun, Mar 11, 2012 at 02:24:46PM -0700, Turquette, Mike wrote:
On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer s.ha...@pengutronix.de 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 of set_rate? The clock
will still have some rate) I check for the result with
clk_ops-recalc_rate.
In the end what's more important than the
Re: [PATCH v6 2/3] clk: introduce the common clock framework
On Mon, Mar 12, 2012 at 4:51 AM, Sascha Hauer s.ha...@pengutronix.de 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 s.ha...@pengutronix.de 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 it I verified the rate change notifiers +
clk_set_parent, which also work (I had not touched these since v4 and
wanted to make sure nothing was broken)
Here is where things get interesting. I tried the same parent rate
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
On Sun, Mar 11, 2012 at 4:34 AM, Sascha Hauer s.ha...@pengutronix.de 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 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 and...@lunn.ch Andrew ___ 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 linux/clk-provider.h
+#include linux/list.h
+
+/*
+ * 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 t...@linutronix.de
___
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
[PATCH v6 2/3] clk: introduce the common clock framework
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 mturque...@linaro.org Signed-off-by: Mike Turquette mturque...@ti.com Cc: Russell King li...@arm.linux.org.uk Cc: Jeremy Kerr jeremy.k...@canonical.com Cc: Thomas Gleixner t...@linutronix.de Cc: Arnd Bergman arnd.bergm...@linaro.org Cc: Paul Walmsley p...@pwsan.com Cc: Shawn Guo shawn@freescale.com Cc: Sascha Hauer s.ha...@pengutronix.de Cc: Richard Zhao richard.z...@linaro.org Cc: Saravana Kannan skan...@codeaurora.org Cc: Magnus Damm magnus.d...@gmail.com Cc: Rob Herring rob.herr...@calxeda.com Cc: Mark Brown broo...@opensource.wolfsonmicro.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Stephen Boyd sb...@codeaurora.org Cc: Amit Kucheria amit.kuche...@linaro.org Cc: Deepak Saxena dsax...@linaro.org Cc: Grant Likely grant.lik...@secretlab.ca Cc: Andrew Lunn and...@lunn.ch --- Changes since v5: * new CONFIG_COMMON_CLK_DISABLE_UNUSED feature * results in a new clk_op callback, .is_enabled * new helpers * __clk_get_prepare_count * __clk_get_enable_count * __clk_is_enabled * fix bug in __clk_get_rate for orphan clocks drivers/clk/Kconfig | 39 ++ drivers/clk/Makefile |1 + drivers/clk/clk.c| 1424 ++ include/linux/clk-private.h | 68 ++ include/linux/clk-provider.h | 171 + include/linux/clk.h | 68 ++- 6 files changed, 1766 insertions(+), 5 deletions(-) create mode 100644 drivers/clk/clk.c create mode 100644 include/linux/clk-private.h create mode 100644 include/linux/clk-provider.h diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 9b3cd08..31ceb27 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -8,3 +8,42 @@ config HAVE_CLK_PREPARE config HAVE_MACH_CLKDEV bool + +menuconfig COMMON_CLK + bool Common Clock Framework + select HAVE_CLK_PREPARE + ---help--- + The common clock framework is a single definition of struct + clk, useful across many platforms, as well as an + implementation of the clock API in include/linux/clk.h. + Architectures utilizing the common struct clk should select + this automatically, but it may be necessary to manually select + this option for loadable modules requiring the common clock + framework. + + If in doubt, say N. + +if COMMON_CLK + +config COMMON_CLK_DISABLE_UNUSED + bool Disabled unused clocks at boot + depends on COMMON_CLK + ---help--- + Traverses the entire clock tree and disables any clocks that are + enabled in hardware but have not been enabled by any device drivers. + This saves power and keeps the software model of the clock in line + with reality. + + If in doubt, say N. + +config COMMON_CLK_DEBUG + bool DebugFS representation of clock tree + depends on COMMON_CLK + ---help--- + Creates a directory hierchy in debugfs for visualizing the clk + tree structure. Each directory contains read-only members + that export information specific to that clk node: clk_rate, + clk_flags, clk_prepare_count, clk_enable_count + clk_notifier_count. + +endif diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 07613fa..ff362c4 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_CLKDEV_LOOKUP)+= clkdev.o +obj-$(CONFIG_COMMON_CLK) += clk.o diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c new file mode 100644 index 000..c7c3bc5 --- /dev/null +++ b/drivers/clk/clk.c @@ -0,0 +1,1424 @@ +/* + * Copyright (C) 2010-2011 Canonical Ltd jeremy.k...@canonical.com + * Copyright (C) 2011-2012 Linaro Ltd mturque...@linaro.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * Standard functionality for the common clock API. See Documentation/clk.txt + */ + +#include linux/clk-private.h +#include linux/module.h +#include linux/mutex.h +#include linux/spinlock.h +#include linux/err.h +#include linux/list.h +#include linux/slab.h + +static DEFINE_SPINLOCK(enable_lock);
