| Message ID | 1305876469.326620.351525457111.2.gpush@pororo (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 20, 2011 at 12:27 AM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote: > Implemenent clk_set_rate by adding a set_rate callback to clk_hw_ops, > and core code to handle propagation of rate changes up and down the > clock tree. > > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com> > > --- > drivers/clk/clk.c | 74 ++++++++++++++++++++++++++++++++++++++++---- > include/linux/clk.h | 12 +++++++ > 2 files changed, 80 insertions(+), 6 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index ad90a90..3a4d70e 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -21,6 +21,8 @@ struct clk { > unsigned int enable_count; > unsigned int prepare_count; > struct clk *parent; > + struct hlist_head children; > + struct hlist_node child_node; > unsigned long rate; > }; > > @@ -176,10 +178,61 @@ long clk_round_rate(struct clk *clk, unsigned long rate) > } > EXPORT_SYMBOL_GPL(clk_round_rate); > > +/* > + * clk_recalc_rates - Given a clock (with a recently updated clk->rate), > + * notify its children that the rate may need to be recalculated, using > + * ops->recalc_rate(). > + */ > +static void clk_recalc_rates(struct clk *clk) > +{ > + struct hlist_node *tmp; > + struct clk *child; > + > + if (clk->ops->recalc_rate) > + clk->rate = clk->ops->recalc_rate(clk->hw); > + > + hlist_for_each_entry(child, tmp, &clk->children, child_node) > + clk_recalc_rates(child); > +} > + > int clk_set_rate(struct clk *clk, unsigned long rate) > { > - /* not yet implemented */ > - return -ENOSYS; > + unsigned long parent_rate, new_rate; > + int ret; > + > + if (!clk->ops->set_rate) > + return -ENOSYS; > + > + new_rate = rate; > + > + /* prevent racing with updates to the clock topology */ > + mutex_lock(&prepare_lock); > + > +propagate: > + ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate); > + > + if (ret < 0) > + return ret; > + > + /* ops->set_rate may require the parent's rate to change (to > + * parent_rate), we need to propagate the set_rate call to the > + * parent. > + */ > + if (ret == CLK_SET_RATE_PROPAGATE) { > + new_rate = parent_rate; > + clk = clk->parent; > + goto propagate; > + } > + > + /* If successful (including propagation to the parent clock(s)), > + * recalculate the rates of the clock, including children. We'll be > + * calling this on the 'parent-most' clock that we propagated to. > + */ > + clk_recalc_rates(clk); > + > + mutex_unlock(&prepare_lock); > + > + return 0; > } > EXPORT_SYMBOL_GPL(clk_set_rate); > > @@ -213,16 +266,25 @@ struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw, > clk->hw = hw; > hw->clk = clk; > > - /* Query the hardware for parent and initial rate */ > + /* Query the hardware for parent and initial rate. We may alter > + * the clock topology, making this clock available from the parent's > + * children list. So, we need to protect against concurrent > + * accesses through set_rate > + */ > + mutex_lock(&prepare_lock); > > - if (clk->ops->get_parent) > - /* We don't to lock against prepare/enable here, as > - * the clock is not yet accessible from anywhere */ > + if (clk->ops->get_parent) { > clk->parent = clk->ops->get_parent(clk->hw); > + if (clk->parent) > + hlist_add_head(&clk->child_node, > + &clk->parent->children); > + } > > if (clk->ops->recalc_rate) > clk->rate = clk->ops->recalc_rate(clk->hw); > > + mutex_unlock(&prepare_lock); > + > > return clk; > } > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 93ff870..e0969d2 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -58,6 +58,12 @@ struct clk_hw { > * parent. Currently only called when the clock is first > * registered. > * > + * @set_rate Change the rate of this clock. If this callback returns > + * CLK_SET_RATE_PROPAGATE, the rate change will be propagated to > + * the parent clock (which may propagate again). The requested > + * rate of the parent is passed back from the callback in the > + * second 'unsigned long *' argument. This seems backwards to me, it requires children to have knowledge of the best states for their parents. I don't think it can ever be implemented safely, either. If the child's set rate needs to decrease its divider value to increase its rate, but increase the parents divider to decrease the rate, the clock can end up running too fast, and out of spec, after set_rate on the child clock has finished, but set_rate on the parent clock has not been called yet. And if the parent clock errors out, clk_set_rate returns an error, but the rate has still changed to some random intermediate value. Can you explain a use case where propagation is necessary? It doesn't match the Sascha's reply to my comments on the main patch, where he said users are best suited to make the decision on the correct parent, but child clocks are best suited to make the decision on the parent's rate? Can you point me to a current clock implementation that does anything like this? > + * > * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow > * implementations to split any work between atomic (enable) and sleepable > * (prepare) contexts. If a clock requires sleeping code to be turned on, this > @@ -76,9 +82,15 @@ struct clk_hw_ops { > void (*disable)(struct clk_hw *); > unsigned long (*recalc_rate)(struct clk_hw *); > long (*round_rate)(struct clk_hw *, unsigned long); > + int (*set_rate)(struct clk_hw *, > + unsigned long, unsigned long *); > struct clk * (*get_parent)(struct clk_hw *); > }; > > +enum { > + CLK_SET_RATE_PROPAGATE = 1, > +}; > + > /** > * clk_prepare - prepare clock for atomic enabling. > * > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi Jeremy, On Fri, May 20, 2011 at 03:27:49PM +0800, Jeremy Kerr wrote: > Implemenent clk_set_rate by adding a set_rate callback to clk_hw_ops, > and core code to handle propagation of rate changes up and down the > clock tree. > > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com> > > + > +propagate: > + ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate); > + > + if (ret < 0) > + return ret; > + > + /* ops->set_rate may require the parent's rate to change (to > + * parent_rate), we need to propagate the set_rate call to the > + * parent. > + */ > + if (ret == CLK_SET_RATE_PROPAGATE) { > + new_rate = parent_rate; > + clk = clk->parent; > + goto propagate; > + } I'm unsure about this one. Every clock should have the ability to stop or continue the rate propagation to the parent. This suggests to leave the decision whether or not to propagate to the core and not to the individual clocks. Right now each mux/div/gate needs an individual propagate flag. By adding the flag to the core the building block implementations could be simpler and the knowledge about propagatability might become handy for the core later. Sascha
[put the lists back on cc] On Thu, May 26, 2011 at 09:37:47AM +0800, Jeremy Kerr wrote: > Hi Sascha, > > > > + > > > +propagate: > > > + ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate); > > > + > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* ops->set_rate may require the parent's rate to change (to > > > + * parent_rate), we need to propagate the set_rate call to the > > > + * parent. > > > + */ > > > + if (ret == CLK_SET_RATE_PROPAGATE) { > > > + new_rate = parent_rate; > > > + clk = clk->parent; > > > + goto propagate; > > > + } > > > > I'm unsure about this one. Every clock should have the ability to stop > > or continue the rate propagation to the parent. This suggests to leave > > the decision whether or not to propagate to the core and not to the > > individual clocks. > > Maybe I'm not understanding you correctly, but that's exactly what this > code does. The decision to propagate is left up to the > implementation-specific set_rate callback - if it returns > CLK_SET_RATE_PROPAGATE (and populate the parent_rate argument with the > requested parent rate), then we propagate the rate change to the parent. I understood how the code is meant to work. It's just that IMO the place where the propagation flag is stored is the wrong one, given that it's a flag that all clocks (can) have. > > > Right now each mux/div/gate needs an individual propagate flag. By > > adding the flag to the core the building block implementations could be > > simpler and the knowledge about propagatability might become handy for > > the core later. > > We could do this with a flag too, yes. But then there's no way of > altering the rate (which we need to do with a divider) as we propagate > it upwards. The current set_rate code lets us do that. Hm, the core could pass a NULL pointer as the third argument to set_rate to indicate that the parent rate is not allowed to change. Then we could initialize &parent_rate to zero before calling set_rate. If the set_rate function does not change it, we don't have to propagate, otherwise yes. Or instead we could just use the CLK_SET_RATE_PROPAGATE return value like we do in the current version. Sascha
On Friday, May 20, 2011 03:27:49 Jeremy Kerr wrote: > int clk_set_rate(struct clk *clk, unsigned long rate) > { > - /* not yet implemented */ > - return -ENOSYS; > + unsigned long parent_rate, new_rate; > + int ret; > + > + if (!clk->ops->set_rate) > + return -ENOSYS; i thought ENOSYS is only for syscalls. shouldnt this be ENODEV or perhaps EOPNOTSUPP ? -mike
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index ad90a90..3a4d70e 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -21,6 +21,8 @@ struct clk { unsigned int enable_count; unsigned int prepare_count; struct clk *parent; + struct hlist_head children; + struct hlist_node child_node; unsigned long rate; }; @@ -176,10 +178,61 @@ long clk_round_rate(struct clk *clk, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_round_rate); +/* + * clk_recalc_rates - Given a clock (with a recently updated clk->rate), + * notify its children that the rate may need to be recalculated, using + * ops->recalc_rate(). + */ +static void clk_recalc_rates(struct clk *clk) +{ + struct hlist_node *tmp; + struct clk *child; + + if (clk->ops->recalc_rate) + clk->rate = clk->ops->recalc_rate(clk->hw); + + hlist_for_each_entry(child, tmp, &clk->children, child_node) + clk_recalc_rates(child); +} + int clk_set_rate(struct clk *clk, unsigned long rate) { - /* not yet implemented */ - return -ENOSYS; + unsigned long parent_rate, new_rate; + int ret; + + if (!clk->ops->set_rate) + return -ENOSYS; + + new_rate = rate; + + /* prevent racing with updates to the clock topology */ + mutex_lock(&prepare_lock); + +propagate: + ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate); + + if (ret < 0) + return ret; + + /* ops->set_rate may require the parent's rate to change (to + * parent_rate), we need to propagate the set_rate call to the + * parent. + */ + if (ret == CLK_SET_RATE_PROPAGATE) { + new_rate = parent_rate; + clk = clk->parent; + goto propagate; + } + + /* If successful (including propagation to the parent clock(s)), + * recalculate the rates of the clock, including children. We'll be + * calling this on the 'parent-most' clock that we propagated to. + */ + clk_recalc_rates(clk); + + mutex_unlock(&prepare_lock); + + return 0; } EXPORT_SYMBOL_GPL(clk_set_rate); @@ -213,16 +266,25 @@ struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw, clk->hw = hw; hw->clk = clk; - /* Query the hardware for parent and initial rate */ + /* Query the hardware for parent and initial rate. We may alter + * the clock topology, making this clock available from the parent's + * children list. So, we need to protect against concurrent + * accesses through set_rate + */ + mutex_lock(&prepare_lock); - if (clk->ops->get_parent) - /* We don't to lock against prepare/enable here, as - * the clock is not yet accessible from anywhere */ + if (clk->ops->get_parent) { clk->parent = clk->ops->get_parent(clk->hw); + if (clk->parent) + hlist_add_head(&clk->child_node, + &clk->parent->children); + } if (clk->ops->recalc_rate) clk->rate = clk->ops->recalc_rate(clk->hw); + mutex_unlock(&prepare_lock); + return clk; } diff --git a/include/linux/clk.h b/include/linux/clk.h index 93ff870..e0969d2 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -58,6 +58,12 @@ struct clk_hw { * parent. Currently only called when the clock is first * registered. * + * @set_rate Change the rate of this clock. If this callback returns + * CLK_SET_RATE_PROPAGATE, the rate change will be propagated to + * the parent clock (which may propagate again). The requested + * rate of the parent is passed back from the callback in the + * second 'unsigned long *' argument. + * * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow * implementations to split any work between atomic (enable) and sleepable * (prepare) contexts. If a clock requires sleeping code to be turned on, this @@ -76,9 +82,15 @@ struct clk_hw_ops { void (*disable)(struct clk_hw *); unsigned long (*recalc_rate)(struct clk_hw *); long (*round_rate)(struct clk_hw *, unsigned long); + int (*set_rate)(struct clk_hw *, + unsigned long, unsigned long *); struct clk * (*get_parent)(struct clk_hw *); }; +enum { + CLK_SET_RATE_PROPAGATE = 1, +}; + /** * clk_prepare - prepare clock for atomic enabling. *
Implemenent clk_set_rate by adding a set_rate callback to clk_hw_ops, and core code to handle propagation of rate changes up and down the clock tree. Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com> --- drivers/clk/clk.c | 74 ++++++++++++++++++++++++++++++++++++++++---- include/linux/clk.h | 12 +++++++ 2 files changed, 80 insertions(+), 6 deletions(-)