[PATCH v4 3/5] clk: add support for clock reparent on set_rate
Saravana Kannan
skannan at codeaurora.org
Tue May 21 01:10:55 EDT 2013
On 05/20/2013 06:28 AM, James Hogan wrote:
> Add core support to allow clock implementations to select the best
> parent clock when rounding a rate, e.g. the one which can provide the
> closest clock rate to that requested. This is by way of adding a new
> clock op, determine_rate(), which is like round_rate() but has an extra
> parameter to allow the clock implementation to optionally select a
> different parent clock. The core then takes care of reparenting the
> clock when setting the rate.
>
> The parent change takes place with the help of some new private data
> members. struct clk::new_parent specifies a clock's new parent (NULL
> indicates no change), and struct clk::new_child specifies a clock's new
> child (whose new_parent member points back to it). The purpose of these
> are to allow correct walking of the future tree for notifications prior
> to actually reparenting any clocks, specifically to skip child clocks
> who are being reparented to another clock (they will be notified via the
> new parent), and to include any new child clock. These pointers are set
> by clk_calc_subtree(), and the new_child pointer gets cleared when a
> child is actually reparented to avoid duplicate POST_RATE_CHANGE
> notifications.
>
> Each place where round_rate() is called, determine_rate() is checked
> first and called in preference. This restructures a few of the call
> sites to simplify the logic into if/else blocks.
>
> Signed-off-by: James Hogan <james.hogan at imgtec.com>
> Cc: Mike Turquette <mturquette at linaro.org>
> Cc: linux-arm-kernel at lists.infradead.org
> ---
> Changes in v4:
>
> * (not included Stephen Boyd's Reviewed-by due to changes)
> * replace __clk_set_parent_no_recalc with __clk_set_parent.
> * never pass NULL to determine_rate's best_parent_clk parameter, and
> slight refactor of __clk_round_rate to use local copy of clk->parent.
> * a few new comments around use of clk::new_child.
>
> Changes in v3:
>
> * rebased on v3.10-rc1.
> * store new_parent_index in struct clk too (calculated from
> clk_fetch_parent_index, and passed through __clk_set_parent_no_recalc
> to __clk_set_parent).
> * allow determine_rate to satisfy recalc_rate check in __clk_init.
>
> Documentation/clk.txt | 4 ++
> drivers/clk/clk.c | 142 +++++++++++++++++++++++++++++--------------
> include/linux/clk-private.h | 3 +
> include/linux/clk-provider.h | 7 +++
> 4 files changed, 110 insertions(+), 46 deletions(-)
>
> diff --git a/Documentation/clk.txt b/Documentation/clk.txt
> index b9911c2..3110ba4 100644
> --- a/Documentation/clk.txt
> +++ b/Documentation/clk.txt
> @@ -70,6 +70,10 @@ the operations defined in clk.h:
> unsigned long parent_rate);
> long (*round_rate)(struct clk_hw *hw, unsigned long,
> unsigned long *);
> + long (*determine_rate)(struct clk_hw *hw,
> + unsigned long rate,
> + unsigned long *best_parent_rate,
> + struct clk **best_parent_clk);
> int (*set_parent)(struct clk_hw *hw, u8 index);
> u8 (*get_parent)(struct clk_hw *hw);
> int (*set_rate)(struct clk_hw *hw, unsigned long);
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a4cd6bc..3ce4810 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -888,21 +888,24 @@ EXPORT_SYMBOL_GPL(clk_enable);
> unsigned long __clk_round_rate(struct clk *clk, unsigned long rate)
> {
> unsigned long parent_rate = 0;
> + struct clk *parent;
>
> if (!clk)
> return 0;
>
> - if (!clk->ops->round_rate) {
> - if (clk->flags & CLK_SET_RATE_PARENT)
> - return __clk_round_rate(clk->parent, rate);
> - else
> - return clk->rate;
> - }
> -
> - if (clk->parent)
> - parent_rate = clk->parent->rate;
> -
> - return clk->ops->round_rate(clk->hw, rate, &parent_rate);
> + parent = clk->parent;
> + if (parent)
> + parent_rate = parent->rate;
> +
> + if (clk->ops->determine_rate)
> + return clk->ops->determine_rate(clk->hw, rate, &parent_rate,
> + &parent);
> + else if (clk->ops->round_rate)
> + return clk->ops->round_rate(clk->hw, rate, &parent_rate);
> + else if (clk->flags & CLK_SET_RATE_PARENT)
> + return __clk_round_rate(clk->parent, rate);
> + else
> + return clk->rate;
> }
>
> /**
> @@ -1055,6 +1058,10 @@ static u8 clk_fetch_parent_index(struct clk *clk, struct clk *parent)
>
> static void clk_reparent(struct clk *clk, struct clk *new_parent)
> {
> + /* avoid duplicate POST_RATE_CHANGE notifications */
> + if (new_parent->new_child == clk)
> + new_parent->new_child = NULL;
> +
> hlist_del(&clk->child_node);
>
> if (new_parent)
> @@ -1173,18 +1180,25 @@ out:
> return ret;
> }
>
> -static void clk_calc_subtree(struct clk *clk, unsigned long new_rate)
> +static void clk_calc_subtree(struct clk *clk, unsigned long new_rate,
> + struct clk *new_parent, u8 p_index)
> {
> struct clk *child;
>
> clk->new_rate = new_rate;
> + clk->new_parent = new_parent;
> + clk->new_parent_index = p_index;
> + /* include clk in new parent's PRE_RATE_CHANGE notifications */
> + clk->new_child = NULL;
> + if (new_parent && new_parent != clk->parent)
> + new_parent->new_child = clk;
>
> hlist_for_each_entry(child, &clk->children, child_node) {
> if (child->ops->recalc_rate)
> child->new_rate = child->ops->recalc_rate(child->hw, new_rate);
> else
> child->new_rate = new_rate;
> - clk_calc_subtree(child, child->new_rate);
> + clk_calc_subtree(child, child->new_rate, NULL, 0);
> }
> }
>
> @@ -1195,50 +1209,63 @@ static void clk_calc_subtree(struct clk *clk, unsigned long new_rate)
> static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
> {
> struct clk *top = clk;
> + struct clk *old_parent, *parent;
> unsigned long best_parent_rate = 0;
> unsigned long new_rate;
> + u8 p_index = 0;
>
> /* sanity */
> if (IS_ERR_OR_NULL(clk))
> return NULL;
>
> /* save parent rate, if it exists */
> - if (clk->parent)
> - best_parent_rate = clk->parent->rate;
> -
> - /* never propagate up to the parent */
> - if (!(clk->flags & CLK_SET_RATE_PARENT)) {
> - if (!clk->ops->round_rate) {
> - clk->new_rate = clk->rate;
> - return NULL;
> - }
> - new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
> + parent = old_parent = clk->parent;
> + if (parent)
> + best_parent_rate = parent->rate;
> +
> + /* find the closest rate and parent clk/rate */
> + if (clk->ops->determine_rate) {
> + new_rate = clk->ops->determine_rate(clk->hw, rate,
> + &best_parent_rate,
> + &parent);
> + } else if (clk->ops->round_rate) {
> + new_rate = clk->ops->round_rate(clk->hw, rate,
> + &best_parent_rate);
> + } else if (!parent || !(clk->flags & CLK_SET_RATE_PARENT)) {
> + /* pass-through clock without adjustable parent */
> + clk->new_rate = clk->rate;
> + return NULL;
> + } else {
> + /* pass-through clock with adjustable parent */
> + top = clk_calc_new_rates(parent, rate);
> + new_rate = parent->new_rate;
> goto out;
> }
>
> - /* need clk->parent from here on out */
> - if (!clk->parent) {
> - pr_debug("%s: %s has NULL parent\n", __func__, clk->name);
> + /* some clocks must be gated to change parent */
> + if (parent != old_parent &&
> + (clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) {
> + pr_debug("%s: %s not gated but wants to reparent\n",
> + __func__, clk->name);
> return NULL;
> }
>
> - if (!clk->ops->round_rate) {
> - top = clk_calc_new_rates(clk->parent, rate);
> - new_rate = clk->parent->new_rate;
> -
> - goto out;
> + /* try finding the new parent index */
> + if (parent) {
> + p_index = clk_fetch_parent_index(clk, parent);
> + if (p_index == clk->num_parents) {
> + pr_debug("%s: clk %s can not be parent of clk %s\n",
> + __func__, parent->name, clk->name);
> + return NULL;
> + }
> }
>
> - new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
> -
> - if (best_parent_rate != clk->parent->rate) {
> - top = clk_calc_new_rates(clk->parent, best_parent_rate);
> -
> - goto out;
> - }
> + if ((clk->flags & CLK_SET_RATE_PARENT) && parent &&
> + best_parent_rate != parent->rate)
> + top = clk_calc_new_rates(parent, best_parent_rate);
>
> out:
> - clk_calc_subtree(clk, new_rate);
> + clk_calc_subtree(clk, new_rate, parent, p_index);
>
> return top;
> }
> @@ -1250,7 +1277,7 @@ out:
> */
> static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long event)
> {
> - struct clk *child, *fail_clk = NULL;
> + struct clk *child, *tmp_clk, *fail_clk = NULL;
> int ret = NOTIFY_DONE;
>
> if (clk->rate == clk->new_rate)
> @@ -1263,9 +1290,19 @@ static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long even
> }
>
> hlist_for_each_entry(child, &clk->children, child_node) {
> - clk = clk_propagate_rate_change(child, event);
> - if (clk)
> - fail_clk = clk;
> + /* Skip children who will be reparented to another clock */
> + if (child->new_parent && child->new_parent != clk)
> + continue;
> + tmp_clk = clk_propagate_rate_change(child, event);
> + if (tmp_clk)
> + fail_clk = tmp_clk;
> + }
> +
> + /* handle the new child who might not be in clk->children yet */
> + if (clk->new_child) {
> + tmp_clk = clk_propagate_rate_change(clk->new_child, event);
> + if (tmp_clk)
> + fail_clk = tmp_clk;
> }
>
> return fail_clk;
> @@ -1283,6 +1320,10 @@ static void clk_change_rate(struct clk *clk)
>
> old_rate = clk->rate;
>
> + /* set parent */
> + if (clk->new_parent && clk->new_parent != clk->parent)
> + __clk_set_parent(clk, clk->new_parent, clk->new_parent_index);
> +
> if (clk->parent)
> best_parent_rate = clk->parent->rate;
>
> @@ -1297,8 +1338,16 @@ static void clk_change_rate(struct clk *clk)
> if (clk->notifier_count && old_rate != clk->rate)
> __clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate);
>
> - hlist_for_each_entry(child, &clk->children, child_node)
> + hlist_for_each_entry(child, &clk->children, child_node) {
> + /* Skip children who will be reparented to another clock */
> + if (child->new_parent && child->new_parent != clk)
> + continue;
> clk_change_rate(child);
> + }
> +
> + /* handle the new child who might not be in clk->children yet */
> + if (clk->new_child)
> + clk_change_rate(clk->new_child);
> }
>
> /**
> @@ -1545,8 +1594,9 @@ int __clk_init(struct device *dev, struct clk *clk)
>
> /* check that clk_ops are sane. See Documentation/clk.txt */
> if (clk->ops->set_rate &&
> - !(clk->ops->round_rate && clk->ops->recalc_rate)) {
> - pr_warning("%s: %s must implement .round_rate & .recalc_rate\n",
> + !((clk->ops->round_rate || clk->ops->determine_rate) &&
> + clk->ops->recalc_rate)) {
> + pr_warning("%s: %s must implement .round_rate or .determine_rate in addition to .recalc_rate\n",
> __func__, clk->name);
> ret = -EINVAL;
> goto out;
> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
> index dd7adff..8138c94 100644
> --- a/include/linux/clk-private.h
> +++ b/include/linux/clk-private.h
> @@ -33,8 +33,11 @@ struct clk {
> const char **parent_names;
> struct clk **parents;
> u8 num_parents;
> + u8 new_parent_index;
Why do you need this? Given the new_parent, can't the specific clock
implementation just look it up when set_rate() is called? Wouldn't that
be the only time you would actually need the index?
If it's just for optimization of some error cases, I think we should
drop this to keep the code simpler. One less state to keep track of when
reading, writing or reviewing the clock framework.
I would like to do a more thorough review of this patch, but it's been a
while since I have looked at clk.c. So, I might not be able to do it
soon enough. But I can't ask to hold off this patch just because I don't
have time. So, I'll see what I can do.
-Saravana
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
More information about the linux-arm-kernel
mailing list