Re: [PATCH E 10/14] OMAP clock: support "dry run" rate and parent changes
Hello Russell,
On Sun, 8 Feb 2009, Russell King - ARM Linux wrote:
> On Wed, Jan 28, 2009 at 12:27:56PM -0700, Paul Walmsley wrote:
> > For upcoming notifier support, modify the rate recalculation code to
> > take parent rate and rate storage parameters. The goal here is to
> > allow the clock code to determine what the clock's rate would be after
> > a parent change or a rate change, without actually changing the
> > hardware registers. This is used by the upcoming notifier patches to
> > pass a clock's current and planned rates to the notifier callbacks.
>
> In addition to my previous comments, there's more reason to reject this
> patch - it's rather buggy.
>
> > Also add a new clock flag, RECALC_ON_ENABLE, which causes the clock
> > framework code to recalculate the current clock's rate and propagate
> > down the tree after a clk_enable() or clk_disable(). This is used by
> > the OMAP3 DPLLs, which change rates when they enable or disable, unlike
> > most clocks.
>
> ... the reasoning for this change being?
For most clocks in the clock tree, there's no need to recalculate the
clock rate and the downstream clock rates when the clock is enabled
or disabled. clk->rate is not adjusted; the rate is simply considered to
be 0 if the clock is disabled.
There is one type of OMAP clock, though, that this may not hold true for:
DPLLs. Currently, the "disabled" state for DPLLs can mean one of two
general modes: stop (in which the DPLL output rate is zero), or bypass (in
which the DPLL output rate is the bypass clock rate). RECALC_ON_ENABLE is
intended to fix the latter case.
As we've discussed before, this may be better implemented as a reparent
operation; but there is a twist: DPLLs can autoidle (if programmed to do
so). In this case the DPLL is considered to be enabled, but is actually
emitting a bypass rate or is stopped. The OMAP hardware automatically
restarts the DPLL when a downstream clock is enabled, with no notification
to the operating system. So some type of RECALC_ON_ENABLE logic may need
to stay.
> > diff --git a/arch/arm/mach-omap1/clock.c b/arch/arm/mach-omap1/clock.c
> > index ae2b304..f3cf6f8 100644
> > --- a/arch/arm/mach-omap1/clock.c
> > +++ b/arch/arm/mach-omap1/clock.c
> >
> > -static void omap1_sossi_recalc(struct clk *clk)
> > +static void omap1_sossi_recalc(struct clk *clk, unsigned long parent_rate,
> > + u8 rate_storage)
> > {
> > + unsigned long new_rate;
> > u32 div = omap_readl(MOD_CONF_CTRL_1);
> >
> > div = (div >> 17) & 0x7;
> > div++;
> > - clk->rate = clk->parent->rate / div;
> > + new_rate = clk->parent->rate / div;
>
> This continues to use clk->parent->rate rather than the value passed in.
Indeed, this should be fixed.
> > @@ -215,21 +238,32 @@ static int calc_dsor_exp(struct clk *clk, unsigned
> > long rate)
> > return dsor_exp;
> > }
> >
> > -static void omap1_ckctl_recalc(struct clk * clk)
> > +static void omap1_ckctl_recalc(struct clk *clk, unsigned long parent_rate,
> > + u8 rate_storage)
> > {
> > int dsor;
> > + unsigned long new_rate;
> >
> > /* Calculate divisor encoded as 2-bit exponent */
> > dsor = 1 << (3 & (omap_readw(ARM_CKCTL) >> clk->rate_offset));
> >
> > - if (unlikely(clk->rate == clk->parent->rate / dsor))
> > + new_rate = parent_rate / dsor;
> > +
> > + if (unlikely(clk->rate == new_rate))
> > return; /* No change, quick exit */
> > - clk->rate = clk->parent->rate / dsor;
> > +
> > + if (rate_storage == CURRENT_RATE)
> > + clk->rate = new_rate;
> > + else if (rate_storage == TEMP_RATE)
> > + clk->temp_rate = new_rate;
>
> The above will result in 'temp_rate' not being set if there is no change
> in actual clock rate
Indeed, this also needs to be fixed.
> This can all be avoided by moving the assignment of clk->rate out of the
> recalc functions and into the caller. That also eliminates this
> rate_storage argument as well, _and_ removes any possibility of broken
> "caching" code surviving since it forces you to always return the rate
> you intend from the function.
>
> See the bottom of this mail for the first step to implement this.
>
> > @@ -242,9 +276,15 @@ static void omap1_ckctl_recalc_dsp_domain(struct clk *
> > clk)
> > dsor = 1 << (3 & (__raw_readw(DSP_CKCTL) >> clk->rate_offset));
> > omap1_clk_disable(&api_ck.clk);
> >
> > - if (unlikely(clk->rate == clk->parent->rate / dsor))
> > + new_rate = parent_rate / dsor;
> > +
> > + if (unlikely(clk->rate == new_rate))
> > return; /* No change, quick exit */
>
> More buggy caching.
Yep.
> > diff --git a/arch/arm/mach-omap2/clock24xx.c
> > b/arch/arm/mach-omap2/clock24xx.c
> > index 57cd85b..cd9fa0d 100644
> > --- a/arch/arm/mach-omap2/clock24xx.c
> > +++ b/arch/arm/mach-omap2/clock24xx.c
> > @@ -89,6 +91,30 @@ static u32 omap2xxx_clk_get_core_rate(struct clk *clk)
> > return core_clk;
Re: [PATCH E 10/14] OMAP clock: support "dry run" rate and parent changes
Hello Russell, On Sun, 8 Feb 2009, Russell King - ARM Linux wrote: > On Wed, Jan 28, 2009 at 12:27:56PM -0700, Paul Walmsley wrote: > > For upcoming notifier support, modify the rate recalculation code to > > take parent rate and rate storage parameters. The goal here is to > > allow the clock code to determine what the clock's rate would be after > > a parent change or a rate change, without actually changing the > > hardware registers. This is used by the upcoming notifier patches to > > pass a clock's current and planned rates to the notifier callbacks. > > NAK. I'm not sure whether you've realised, but this is exactly what the > 'round_rate' method is supposed to be doing. It provides a method by > which you can find out what the clock rate would be if you asked for > 'set_rate' to set the hardware to the requested rate. There's been some miscommunication about the purpose of this patch - most likely my fault for not grouping this patch with the clock notifier patch set, which has not yet been submitted for upstream merging. Most of this patch is not needed until notifiers are implemented, so I'd propose that we defer consideration of most of this patch until the clock notifiers are submitted. What would be useful, though, is to split out the RECALC_ON_ENABLE portion of this patch and merge that. That is a bug fix, and I should have sent that as a separate patch. For the sake of the current discussion though, here's what this patch was intended to accomplish. We have some patches under development that implement clock rate change notifiers, allowing drivers to register for pre- and post-notification on clock rate changes. The notifier callbacks are passed the old clock rate and the new clock rate. Since a rate change on a clock in the middle of the clock tree (e.g., dpll3_m2_ck) can affect all clocks downstream, the clock code must compute the new rate for not only dpll3_m2_ck, but also all downstream clocks, before clk->rate is updated and the hardware registers are actually changed. This presents a problem with the current code. Consider the pre-notification case, in which the clock code needs to compute the target clock rate without actually updating clk->rate. But the round_rate() functions assume that the parent clock's current rate is what should be used. If the parent clock's rate would also change as part of the clk_set_rate(), the target rate computation will return an incorrect rate. So this patch allows temporary rates to be computed for all of the clocks that would be affected by the rate or parent change, so the correct target rates can be passed to each clock's notifier (assuming one exists for the clock). N.B., this temporary rate computation is also the motivation behind the recursive propagate_rate() calls that we discussed earlier. It also may turn out that it's unnecessary to pass the new rate to the notifiers, in which case most of this code won't need to reappear. > A far better way to approach this would be to split the set_rate/recalc > functionality into two parts: > > 1. a method which returns the both the new clock rate and the hardware >programming information > 2. a method to commit the hardware programming information to the registers > > (1) can be used for implementing clk_round_rate() (which is totally lacking > in OMAP2+, yet is implemented in OMAP1), clk_set_rate(), clk_set_parent() > etc. The OMAP2/3 clock code does implement clk_round_rate() - most commonly via mach-omap2/clock.c:omap2_clksel_round_rate(). There are a few special cases which use other round_rate() functions. > (2) can be used when it's required to actually reprogram the hardware. > > So, rather than the current situation where we have recalc, round_rate > and set_rate methods, we have calc_rate() and commit() methods instead > and have the core clock code sort out calling these. I haven't yet spent much time thinking about this new arrangement -- it seems to be orthogonal to the problem that this patch is intended to solve -- but it might make sense for a different purpose. Many of our set_rate() functions wind up calling round_rate() first to determine whether the passed-in rate is valid and to return the hardware programming data from the clksel structures. This extra call to round_rate() just wastes cycles. So this new layout might be more efficient, although it appears that calc_rate() is called twice in the new arrangement as well. I appreciate the ongoing technical review, - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH E 10/14] OMAP clock: support "dry run" rate and parent changes
On Sunday 08 February 2009, Russell King - ARM Linux wrote: > A far better way to approach this would be to split the set_rate/recalc > functionality into two parts: > > 1. a method which returns the both the new clock rate and the hardware >programming information > 2. a method to commit the hardware programming information to the registers Much simpler to just pass a "commit" flag and not try to come up with some generic way to represent "hardware programming information"; in general that will be very different between clocks. Then clk_round_rate() passes "false" for commit, while clk_set_rate() passes "true". And in both cases the value returned is the rate, or negative errno to indicate a fault. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH E 10/14] OMAP clock: support "dry run" rate and parent changes
On Wed, Jan 28, 2009 at 12:27:56PM -0700, Paul Walmsley wrote:
> For upcoming notifier support, modify the rate recalculation code to
> take parent rate and rate storage parameters. The goal here is to
> allow the clock code to determine what the clock's rate would be after
> a parent change or a rate change, without actually changing the
> hardware registers. This is used by the upcoming notifier patches to
> pass a clock's current and planned rates to the notifier callbacks.
In addition to my previous comments, there's more reason to reject this
patch - it's rather buggy.
> Also add a new clock flag, RECALC_ON_ENABLE, which causes the clock
> framework code to recalculate the current clock's rate and propagate
> down the tree after a clk_enable() or clk_disable(). This is used by
> the OMAP3 DPLLs, which change rates when they enable or disable, unlike
> most clocks.
... the reasoning for this change being?
> diff --git a/arch/arm/mach-omap1/clock.c b/arch/arm/mach-omap1/clock.c
> index ae2b304..f3cf6f8 100644
> --- a/arch/arm/mach-omap1/clock.c
> +++ b/arch/arm/mach-omap1/clock.c
> @@ -34,27 +34,50 @@ __u32 arm_idlect1_mask;
> * Omap1 specific clock functions
> *-*/
>
> -static void omap1_watchdog_recalc(struct clk * clk)
> +static void omap1_watchdog_recalc(struct clk *clk, unsigned long parent_rate,
> + u8 rate_storage)
> {
> - clk->rate = clk->parent->rate / 14;
> + unsigned long new_rate;
> +
> + new_rate = parent_rate / 14;
> +
> + if (rate_storage == CURRENT_RATE)
> + clk->rate = new_rate;
> + else if (rate_storage == TEMP_RATE)
> + clk->temp_rate = new_rate;
Ok.
> }
>
> -static void omap1_uart_recalc(struct clk * clk)
> +static void omap1_uart_recalc(struct clk *clk, unsigned long parent_rate,
> + u8 rate_storage)
> {
> + unsigned long new_rate;
> unsigned int val = __raw_readl(clk->enable_reg);
> +
> if (val & clk->enable_bit)
> - clk->rate = 4800;
> + new_rate = 4800;
> else
> - clk->rate = 1200;
> + new_rate = 1200;
> +
> + if (rate_storage == CURRENT_RATE)
> + clk->rate = new_rate;
> + else if (rate_storage == TEMP_RATE)
> + clk->temp_rate = new_rate;
Ok.
> }
>
> -static void omap1_sossi_recalc(struct clk *clk)
> +static void omap1_sossi_recalc(struct clk *clk, unsigned long parent_rate,
> +u8 rate_storage)
> {
> + unsigned long new_rate;
> u32 div = omap_readl(MOD_CONF_CTRL_1);
>
> div = (div >> 17) & 0x7;
> div++;
> - clk->rate = clk->parent->rate / div;
> + new_rate = clk->parent->rate / div;
This continues to use clk->parent->rate rather than the value passed in.
> +
> + if (rate_storage == CURRENT_RATE)
> + clk->rate = new_rate;
> + else if (rate_storage == TEMP_RATE)
> + clk->temp_rate = new_rate;
> }
>
> static int omap1_clk_enable_dsp_domain(struct clk *clk)
> @@ -215,21 +238,32 @@ static int calc_dsor_exp(struct clk *clk, unsigned long
> rate)
> return dsor_exp;
> }
>
> -static void omap1_ckctl_recalc(struct clk * clk)
> +static void omap1_ckctl_recalc(struct clk *clk, unsigned long parent_rate,
> +u8 rate_storage)
> {
> int dsor;
> + unsigned long new_rate;
>
> /* Calculate divisor encoded as 2-bit exponent */
> dsor = 1 << (3 & (omap_readw(ARM_CKCTL) >> clk->rate_offset));
>
> - if (unlikely(clk->rate == clk->parent->rate / dsor))
> + new_rate = parent_rate / dsor;
> +
> + if (unlikely(clk->rate == new_rate))
> return; /* No change, quick exit */
> - clk->rate = clk->parent->rate / dsor;
> +
> + if (rate_storage == CURRENT_RATE)
> + clk->rate = new_rate;
> + else if (rate_storage == TEMP_RATE)
> + clk->temp_rate = new_rate;
The above will result in 'temp_rate' not being set if there is no change
in actual clock rate - because of the idiotic caching. Why is this
idiotic? Well, think about it for a moment:
if (clk->rate == clk->parent->rate / dsor)
return; /* No change, quick exit */
clk->rate = clk->parent->rate / dsor;
What exactly are we trying to avoid doing here? The expensive divide?
That divide has to happen in the if statement anyway, so that can't be
it. Maybe it's trying to avoid the cycles consumed by doing the actual
store? That's insignificant WRT the divide.
The truth is that nothing significant is being saved by this obscure
complicated code - the only thing that this code is doing is causing
bugs when additional changes happen.
This can all be avoided by moving the assignment of clk->rate out of the
recalc functions and into the caller. That also eliminates this
rate_storage argument as well, _a
Re: [PATCH E 10/14] OMAP clock: support "dry run" rate and parent changes
On Wed, Jan 28, 2009 at 12:27:56PM -0700, Paul Walmsley wrote:
> For upcoming notifier support, modify the rate recalculation code to
> take parent rate and rate storage parameters. The goal here is to
> allow the clock code to determine what the clock's rate would be after
> a parent change or a rate change, without actually changing the
> hardware registers. This is used by the upcoming notifier patches to
> pass a clock's current and planned rates to the notifier callbacks.
NAK. I'm not sure whether you've realised, but this is exactly what the
'round_rate' method is supposed to be doing. It provides a method by
which you can find out what the clock rate would be if you asked for
'set_rate' to set the hardware to the requested rate.
A far better way to approach this would be to split the set_rate/recalc
functionality into two parts:
1. a method which returns the both the new clock rate and the hardware
programming information
2. a method to commit the hardware programming information to the registers
(1) can be used for implementing clk_round_rate() (which is totally lacking
in OMAP2+, yet is implemented in OMAP1), clk_set_rate(), clk_set_parent()
etc.
(2) can be used when it's required to actually reprogram the hardware.
So, rather than the current situation where we have recalc, round_rate
and set_rate methods, we have calc_rate() and commit() methods instead
and have the core clock code sort out calling these.
IOW, clk_set_rate() and clk_round_rate() becomes:
int clk_set_rate(struct clk *clk, unsigned long rate)
{
unsigned long flags;
int ret = -EINVAL;
spin_lock_irqsave(&clockfw_lock, flags);
if (clk->calc_rate) {
unsigned long parent_rate = 0;
struct clk_hw hw;
if (clk->parent)
parent_rate = clk->parent->rate;
ret = clk->calc_rate(clk, parent_rate, &rate, &hw);
if (ret == 0) {
clk->rate = rate;
clk->commit(clk, &hw);
}
}
spin_unlock_irqrestore(&clockfw_lock, flags);
return ret;
}
long clk_round_rate(struct clk *clk, unsigned long rate)
{
unsigned long flags;
long ret;
spin_lock_irqsave(&clockfw_lock, flags);
if (clk->calc_rate) {
unsigned long parent_rate = 0;
struct clk_hw hw;
if (clk->parent)
parent_rate = clk->parent->rate;
ret = clk->calc_rate(clk, parent_rate, &rate, &hw);
if (ret == 0)
ret = rate;
}
spin_unlock_irqrestore(&clockfw_lock, flags);
return ret;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH E 10/14] OMAP clock: support "dry run" rate and parent changes
For upcoming notifier support, modify the rate recalculation code to
take parent rate and rate storage parameters. The goal here is to
allow the clock code to determine what the clock's rate would be after
a parent change or a rate change, without actually changing the
hardware registers. This is used by the upcoming notifier patches to
pass a clock's current and planned rates to the notifier callbacks.
Also add a new clock flag, RECALC_ON_ENABLE, which causes the clock
framework code to recalculate the current clock's rate and propagate
down the tree after a clk_enable() or clk_disable(). This is used by
the OMAP3 DPLLs, which change rates when they enable or disable, unlike
most clocks.
linux-omap source commit is 33d000c99ee393fe2042f93e8422f94976d276ce.
Signed-off-by: Paul Walmsley
Signed-off-by: Tony Lindgren
---
arch/arm/mach-omap1/clock.c | 74 +
arch/arm/mach-omap1/clock.h | 16 +++--
arch/arm/mach-omap2/clock.c | 34 +++---
arch/arm/mach-omap2/clock.h |8 +-
arch/arm/mach-omap2/clock24xx.c | 107 ++-
arch/arm/mach-omap2/clock24xx.h | 13 ++--
arch/arm/mach-omap2/clock34xx.c | 38 ---
arch/arm/mach-omap2/clock34xx.h | 21 --
arch/arm/plat-omap/clock.c | 59 -
arch/arm/plat-omap/include/mach/clock.h | 15 +++-
10 files changed, 277 insertions(+), 108 deletions(-)
diff --git a/arch/arm/mach-omap1/clock.c b/arch/arm/mach-omap1/clock.c
index ae2b304..f3cf6f8 100644
--- a/arch/arm/mach-omap1/clock.c
+++ b/arch/arm/mach-omap1/clock.c
@@ -34,27 +34,50 @@ __u32 arm_idlect1_mask;
* Omap1 specific clock functions
*-*/
-static void omap1_watchdog_recalc(struct clk * clk)
+static void omap1_watchdog_recalc(struct clk *clk, unsigned long parent_rate,
+ u8 rate_storage)
{
- clk->rate = clk->parent->rate / 14;
+ unsigned long new_rate;
+
+ new_rate = parent_rate / 14;
+
+ if (rate_storage == CURRENT_RATE)
+ clk->rate = new_rate;
+ else if (rate_storage == TEMP_RATE)
+ clk->temp_rate = new_rate;
}
-static void omap1_uart_recalc(struct clk * clk)
+static void omap1_uart_recalc(struct clk *clk, unsigned long parent_rate,
+ u8 rate_storage)
{
+ unsigned long new_rate;
unsigned int val = __raw_readl(clk->enable_reg);
+
if (val & clk->enable_bit)
- clk->rate = 4800;
+ new_rate = 4800;
else
- clk->rate = 1200;
+ new_rate = 1200;
+
+ if (rate_storage == CURRENT_RATE)
+ clk->rate = new_rate;
+ else if (rate_storage == TEMP_RATE)
+ clk->temp_rate = new_rate;
}
-static void omap1_sossi_recalc(struct clk *clk)
+static void omap1_sossi_recalc(struct clk *clk, unsigned long parent_rate,
+ u8 rate_storage)
{
+ unsigned long new_rate;
u32 div = omap_readl(MOD_CONF_CTRL_1);
div = (div >> 17) & 0x7;
div++;
- clk->rate = clk->parent->rate / div;
+ new_rate = clk->parent->rate / div;
+
+ if (rate_storage == CURRENT_RATE)
+ clk->rate = new_rate;
+ else if (rate_storage == TEMP_RATE)
+ clk->temp_rate = new_rate;
}
static int omap1_clk_enable_dsp_domain(struct clk *clk)
@@ -215,21 +238,32 @@ static int calc_dsor_exp(struct clk *clk, unsigned long
rate)
return dsor_exp;
}
-static void omap1_ckctl_recalc(struct clk * clk)
+static void omap1_ckctl_recalc(struct clk *clk, unsigned long parent_rate,
+ u8 rate_storage)
{
int dsor;
+ unsigned long new_rate;
/* Calculate divisor encoded as 2-bit exponent */
dsor = 1 << (3 & (omap_readw(ARM_CKCTL) >> clk->rate_offset));
- if (unlikely(clk->rate == clk->parent->rate / dsor))
+ new_rate = parent_rate / dsor;
+
+ if (unlikely(clk->rate == new_rate))
return; /* No change, quick exit */
- clk->rate = clk->parent->rate / dsor;
+
+ if (rate_storage == CURRENT_RATE)
+ clk->rate = new_rate;
+ else if (rate_storage == TEMP_RATE)
+ clk->temp_rate = new_rate;
}
-static void omap1_ckctl_recalc_dsp_domain(struct clk * clk)
+static void omap1_ckctl_recalc_dsp_domain(struct clk *clk,
+ unsigned long parent_rate,
+ u8 rate_storage)
{
int dsor;
+ unsigned long new_rate;
/* Calculate divisor encoded as 2-bit exponent
*
@@ -242,9 +276,15 @@ static void omap1_ckctl_recalc_dsp_domain(struct clk * clk)
dsor = 1 << (3 & (__raw_readw(DSP_CKCTL) >> clk->rate_offset));
omap1_clk_dis
