[PATCH v2 0/3] clk: implement remuxing during set_rate

[PATCH v2 0/3] clk: implement remuxing during set_rate

[James Hogan]

This patchset adds support for automatic selection of the best parent
for a clock mux, i.e. the one which can provide the closest clock rate
to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
so that it doesn't happen unless explicitly allowed.

This works by way of adding a new op, determine_rate, similar to
round_rate but with an extra parameter to allow the clock driver to
optionally select a different parent clock. This is used in
clk_calc_new_rates to decide whether to initiate a set_parent operation.

Changes in v2:

I've moved the mux determine_rate implementation into a core helper, but
I haven't pushed it fully into the core, as I think it just wouldn't
work correctly for more complex clocks, e.g. if you (theoretically) had
a combined mux and divide, you'd want to intercept the determine_rate
and ask for a larger rate from the parent clocks, then return the
divided rate. This should be possible by wrapping the mux determine_rate
helper.

Patch 1 still exports the __clk_get_parent_by_index as it seems like it
might be a useful thing for clock implementations to have access to if
they ever wanted to do something more fancy with changing clock parents.

I haven't made any attempt to implement the atomic set_parent+set_rate
as I don't have hardware that could take proper advantage of it, but it
shouldn't be too difficult for others to implement if they wanted since
they're fairly close to one another (in clk_change_rate()).

* switched to using new determine_rate op rather than adding an argument
  to round_rate.
* moved mux implementation into a single helper which should be usable
  from more complex clocks which can mux.
* rewrite main implementation so that no changes are made until after
  the PRE notifications have been sent, and in a way that should ensure
  correct notifications without duplicates, and I think should be safe
  in the event of a notification failing.
* various tidy ups and fixes.

James Hogan (3):
  clk: abstract parent cache
  clk: add support for clock reparent on set_rate
  clk: clk-mux: implement remuxing on set_rate

 Documentation/clk.txt        |   4 +
 drivers/clk/clk-mux.c        |   1 +
 drivers/clk/clk.c            | 216 +++++++++++++++++++++++++++++++++----------
 include/linux/clk-private.h  |   2 +
 include/linux/clk-provider.h |  12 +++
 5 files changed, 185 insertions(+), 50 deletions(-)

-- 
1.8.1.2

[PATCH v2 1/3] clk: abstract parent cache

[James Hogan]

Abstract access to the clock parent cache by defining
__clk_get_parent_by_index(clk, index). This allows access to parent
clocks from clock drivers.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 drivers/clk/clk.c            | 21 ++++++++++++++-------
 include/linux/clk-provider.h |  1 +
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ed87b24..79d5deb 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -415,6 +415,19 @@ struct clk *__clk_get_parent(struct clk *clk)
 	return !clk ? NULL : clk->parent;
 }
 
+struct clk *__clk_get_parent_by_index(struct clk *clk, u8 index)
+{
+	if (!clk || index >= clk->num_parents)
+		return NULL;
+	else if (!clk->parents)
+		return __clk_lookup(clk->parent_names[index]);
+	else if (!clk->parents[index])
+		return clk->parents[index] =
+			__clk_lookup(clk->parent_names[index]);
+	else
+		return clk->parents[index];
+}
+
 unsigned int __clk_get_enable_count(struct clk *clk)
 {
 	return !clk ? 0 : clk->enable_count;
@@ -1150,13 +1163,7 @@ static struct clk *__clk_init_parent(struct clk *clk)
 			kzalloc((sizeof(struct clk*) * clk->num_parents),
 					GFP_KERNEL);
 
-	if (!clk->parents)
-		ret = __clk_lookup(clk->parent_names[index]);
-	else if (!clk->parents[index])
-		ret = clk->parents[index] =
-			__clk_lookup(clk->parent_names[index]);
-	else
-		ret = clk->parents[index];
+	ret = __clk_get_parent_by_index(clk, index);
 
 out:
 	return ret;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 7f197d7..4e0b634 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -347,6 +347,7 @@ const char *__clk_get_name(struct clk *clk);
 struct clk_hw *__clk_get_hw(struct clk *clk);
 u8 __clk_get_num_parents(struct clk *clk);
 struct clk *__clk_get_parent(struct clk *clk);
+struct clk *__clk_get_parent_by_index(struct clk *clk, u8 index);
 unsigned int __clk_get_enable_count(struct clk *clk);
 unsigned int __clk_get_prepare_count(struct clk *clk);
 unsigned long __clk_get_rate(struct clk *clk);
-- 
1.8.1.2

[PATCH v2 2/3] clk: add support for clock reparent on set_rate

[James Hogan]

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 two 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, passing NULL to the new parent argument if not
needed (e.g. in __clk_round_rate). This restructures a few of the call
sites to simplify the logic into if/else blocks.

A new __clk_set_parent_no_recalc() is created similar to
clk_set_parent() but without calling __clk_recalc_rates(). This is for
clk_change_rate() to use, where rate recalculation and notifications are
already handled.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 Documentation/clk.txt        |   4 ++
 drivers/clk/clk.c            | 146 ++++++++++++++++++++++++++++++-------------
 include/linux/clk-private.h  |   2 +
 include/linux/clk-provider.h |   7 +++
 4 files changed, 116 insertions(+), 43 deletions(-)

diff --git a/Documentation/clk.txt b/Documentation/clk.txt
index 1943fae..502c033 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 79d5deb..cc0e618 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -27,6 +27,8 @@ static HLIST_HEAD(clk_root_list);
 static HLIST_HEAD(clk_orphan_list);
 static LIST_HEAD(clk_notifier_list);
 
+static int __clk_set_parent_no_recalc(struct clk *clk, struct clk *parent);
+
 /***        debugfs support        ***/
 
 #ifdef CONFIG_COMMON_CLK_DEBUG
@@ -727,17 +729,20 @@ unsigned long __clk_round_rate(struct clk *clk, unsigned long rate)
 	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);
+	if (clk->ops->determine_rate)
+		return clk->ops->determine_rate(clk->hw, rate, &parent_rate,
+						NULL);
+	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;
+
 }
 
 /**
@@ -906,18 +911,23 @@ 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)
 {
 	struct clk *child;
 
 	clk->new_rate = new_rate;
+	clk->new_parent = new_parent;
+	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);
 	}
 }
 
@@ -928,6 +938,7 @@ 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;
 
@@ -936,42 +947,43 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
 		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;
-	}
-
-	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);
 
 	return top;
 }
@@ -983,7 +995,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)
@@ -996,9 +1008,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;
@@ -1016,6 +1038,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_no_recalc(clk, clk->new_parent);
+
 	if (clk->parent)
 		best_parent_rate = clk->parent->rate;
 
@@ -1030,8 +1056,15 @@ 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);
+	}
+
+	if (clk->new_child)
+		clk_change_rate(clk->new_child);
 }
 
 /**
@@ -1169,7 +1202,7 @@ out:
 	return ret;
 }
 
-void __clk_reparent(struct clk *clk, struct clk *new_parent)
+void __clk_reparent_no_recalc(struct clk *clk, struct clk *new_parent)
 {
 #ifdef CONFIG_COMMON_CLK_DEBUG
 	struct dentry *d;
@@ -1179,6 +1212,9 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 	if (!clk || !new_parent)
 		return;
 
+	if (new_parent->new_child == clk)
+		new_parent->new_child = NULL;
+
 	hlist_del(&clk->child_node);
 
 	if (new_parent)
@@ -1206,6 +1242,11 @@ out:
 #endif
 
 	clk->parent = new_parent;
+}
+
+void __clk_reparent(struct clk *clk, struct clk *new_parent)
+{
+	__clk_reparent_no_recalc(clk, new_parent);
 
 	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 }
@@ -1270,6 +1311,25 @@ out:
 	return ret;
 }
 
+static int __clk_set_parent_no_recalc(struct clk *clk, struct clk *parent)
+{
+	int ret = 0;
+
+	if (clk->parent == parent)
+		goto out;
+
+	/* only re-parent if the clock is not in use */
+	ret = __clk_set_parent(clk, parent);
+	if (ret)
+		goto out;
+
+	/* reparent, but don't propagate rate recalculation downstream */
+	__clk_reparent_no_recalc(clk, parent);
+
+out:
+	return ret;
+}
+
 /**
  * clk_set_parent - switch the parent of a mux clk
  * @clk: the mux clk whose input we are switching
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index 9c7f580..0826a60 100644
--- a/include/linux/clk-private.h
+++ b/include/linux/clk-private.h
@@ -35,6 +35,8 @@ struct clk {
 	u8			num_parents;
 	unsigned long		rate;
 	unsigned long		new_rate;
+	struct clk		*new_parent;
+	struct clk		*new_child;
 	unsigned long		flags;
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 4e0b634..5fe1d38 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -71,6 +71,10 @@ struct clk_hw;
  * @round_rate:	Given a target rate as input, returns the closest rate actually
  * 		supported by the clock.
  *
+ * @determine_rate: Given a target rate as input, returns the closest rate
+ *		actually supported by the clock, and optionally the parent clock
+ *		that should be used to provide the clock rate.
+ *
  * @get_parent:	Queries the hardware to determine the parent of a clock.  The
  * 		return value is a u8 which specifies the index corresponding to
  * 		the parent clock.  This index can be applied to either the
@@ -116,6 +120,9 @@ struct clk_ops {
 					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,
-- 
1.8.1.2

[PATCH v2 3/3] clk: clk-mux: implement remuxing on set_rate

[James Hogan]

Add a new clock flag called CLK_SET_RATE_REMUX to indicate that the
clock can have it's parent changed automatically in response to a
set_rate.

Implement clk-mux remuxing if the CLK_SET_RATE_REMUX flag is set. This
implements determine_rate for clk-mux to propagate to each parent and to
choose the best one (like clk-divider this chooses the parent which
provides the fastest rate <= the requested rate).

The determine_rate op is implemented as a core helper function so that
it can be easily used by more complex clocks which incorporate muxes.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 drivers/clk/clk-mux.c        |  1 +
 drivers/clk/clk.c            | 49 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  4 ++++
 3 files changed, 54 insertions(+)

diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 508c032..3a42b96 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -85,6 +85,7 @@ static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
 const struct clk_ops clk_mux_ops = {
 	.get_parent = clk_mux_get_parent,
 	.set_parent = clk_mux_set_parent,
+	.determine_rate = __clk_mux_determine_rate,
 };
 EXPORT_SYMBOL_GPL(clk_mux_ops);
 
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cc0e618..8c4b714 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -529,6 +529,55 @@ struct clk *__clk_lookup(const char *name)
 	return NULL;
 }
 
+/*
+ * Helper for finding best parent to provide a given frequency. This can be used
+ * directly as a determine_rate callback (e.g. for a mux), or from a more
+ * complex clock that may combine a mux with other operations.
+ */
+long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long *best_parent_rate,
+			      struct clk **best_parent_p)
+{
+	struct clk *clk = hw->clk, *parent, *best_parent = NULL;
+	int i, num_parents;
+	unsigned long parent_rate, best = 0;
+
+	/* if remux flag not set, pass through to current parent */
+	if (!(clk->flags & CLK_SET_RATE_REMUX)) {
+		parent = clk->parent;
+		if (clk->flags & CLK_SET_RATE_PARENT)
+			best = __clk_round_rate(parent, rate);
+		else if (parent)
+			best = __clk_get_rate(parent);
+		else
+			best = __clk_get_rate(clk);
+		goto out;
+	}
+
+	/* find the parent that can provide the fastest rate <= rate */
+	num_parents = clk->num_parents;
+	for (i = 0; i < num_parents; i++) {
+		parent = __clk_get_parent_by_index(clk, i);
+		if (!parent)
+			continue;
+		if (clk->flags & CLK_SET_RATE_PARENT)
+			parent_rate = __clk_round_rate(parent, rate);
+		else
+			parent_rate = __clk_get_rate(parent);
+		if (parent_rate <= rate && parent_rate > best) {
+			best_parent = parent;
+			best = parent_rate;
+		}
+	}
+
+out:
+	if (best_parent_p && best_parent)
+		*best_parent_p = best_parent;
+	*best_parent_rate = best;
+
+	return best;
+}
+
 /***        clk api        ***/
 
 void __clk_unprepare(struct clk *clk)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5fe1d38..0eb4532 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -27,6 +27,7 @@
 #define CLK_IS_ROOT		BIT(4) /* root clk, has no parent */
 #define CLK_IS_BASIC		BIT(5) /* Basic clk, can't do a to_clk_foo() */
 #define CLK_GET_RATE_NOCACHE	BIT(6) /* do not use the cached clk rate */
+#define CLK_SET_RATE_REMUX	BIT(7) /* find best parent for rate change */
 
 struct clk_hw;
 
@@ -361,6 +362,9 @@ unsigned long __clk_get_rate(struct clk *clk);
 unsigned long __clk_get_flags(struct clk *clk);
 bool __clk_is_enabled(struct clk *clk);
 struct clk *__clk_lookup(const char *name);
+long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long *best_parent_rate,
+			      struct clk **best_parent_p);
 
 /*
  * FIXME clock api without lock protection
-- 
1.8.1.2

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[Stephen Boyd]

On 04/19/13 09:28, James Hogan wrote:
> This patchset adds support for automatic selection of the best parent
> for a clock mux, i.e. the one which can provide the closest clock rate
> to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
> so that it doesn't happen unless explicitly allowed.
>
> This works by way of adding a new op, determine_rate, similar to
> round_rate but with an extra parameter to allow the clock driver to
> optionally select a different parent clock. This is used in
> clk_calc_new_rates to decide whether to initiate a set_parent operation.

Which tree is this based on? I get failures with git am on patch 2.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[Saravana Kannan]

On 05/08/2013 04:36 PM, Stephen Boyd wrote:
> On 04/19/13 09:28, James Hogan wrote:
>> This patchset adds support for automatic selection of the best parent
>> for a clock mux, i.e. the one which can provide the closest clock rate
>> to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
>> so that it doesn't happen unless explicitly allowed.
>>
>> This works by way of adding a new op, determine_rate, similar to
>> round_rate but with an extra parameter to allow the clock driver to
>> optionally select a different parent clock. This is used in
>> clk_calc_new_rates to decide whether to initiate a set_parent operation.
>
> Which tree is this based on? I get failures with git am on patch 2.
>

Mike's clk-for-3.10 branch.

-Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[James Hogan]

On 09/05/13 00:36, Stephen Boyd wrote:
> On 04/19/13 09:28, James Hogan wrote:
>> This patchset adds support for automatic selection of the best parent
>> for a clock mux, i.e. the one which can provide the closest clock rate
>> to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
>> so that it doesn't happen unless explicitly allowed.
>>
>> This works by way of adding a new op, determine_rate, similar to
>> round_rate but with an extra parameter to allow the clock driver to
>> optionally select a different parent clock. This is used in
>> clk_calc_new_rates to decide whether to initiate a set_parent operation.
> 
> Which tree is this based on? I get failures with git am on patch 2.
> 

It was based on v3.9-rc4.

Cheers
James

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[Stephen Boyd]

On 05/09/13 02:02, James Hogan wrote:
> On 09/05/13 00:36, Stephen Boyd wrote:
>> On 04/19/13 09:28, James Hogan wrote:
>>> This patchset adds support for automatic selection of the best parent
>>> for a clock mux, i.e. the one which can provide the closest clock rate
>>> to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
>>> so that it doesn't happen unless explicitly allowed.
>>>
>>> This works by way of adding a new op, determine_rate, similar to
>>> round_rate but with an extra parameter to allow the clock driver to
>>> optionally select a different parent clock. This is used in
>>> clk_calc_new_rates to decide whether to initiate a set_parent operation.
>> Which tree is this based on? I get failures with git am on patch 2.
>>
> It was based on v3.9-rc4.
>

Thanks. Would it be possible for you to update to Mike's for-next
branch? I've done the rebase myself for now and it seems to work well
enough. I'll need to add the set_rate_and_parent() op on top.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 2/3] clk: add support for clock reparent on set_rate

[Stephen Boyd]

On 04/19/13 09:28, 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 two 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, passing NULL to the new parent argument if not
> needed (e.g. in __clk_round_rate). This restructures a few of the call
> sites to simplify the logic into if/else blocks.
>
> A new __clk_set_parent_no_recalc() is created similar to
> clk_set_parent() but without calling __clk_recalc_rates(). This is for
> clk_change_rate() to use, where rate recalculation and notifications are
> already handled.
>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>

I believe we'll need to update the check in __clk_init() to allow you to
have either a .round_rate or a .determine_rate op if you have a
.recalc_rate op. Right now it fails to register the clock and then
oopses later on while setting up clock debugfs.

Here's a (probably whitespace damaged) patch for the first one.

----8<-----

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e6e759e..d56291c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1668,8 +1668,8 @@ 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;

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 2/3] clk: add support for clock reparent on set_rate

[James Hogan]

On 09/05/13 21:01, Stephen Boyd wrote:
> I believe we'll need to update the check in __clk_init() to allow you to
> have either a .round_rate or a .determine_rate op if you have a
> .recalc_rate op. Right now it fails to register the clock and then
> oopses later on while setting up clock debugfs.
> 
> Here's a (probably whitespace damaged) patch for the first one.

Thanks, I'll incorporate that.

Cheers
James

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[James Hogan]

On 09/05/13 20:48, Stephen Boyd wrote:
> On 05/09/13 02:02, James Hogan wrote:
>> On 09/05/13 00:36, Stephen Boyd wrote:
>>> Which tree is this based on? I get failures with git am on patch 2.
>>>
>> It was based on v3.9-rc4.
>>
> 
> Thanks. Would it be possible for you to update to Mike's for-next
> branch? I've done the rebase myself for now and it seems to work well
> enough. I'll need to add the set_rate_and_parent() op on top.
> 

Yes, I will do that before reposting. Thanks for giving it a spin.

Cheers
James

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[Mike Turquette]

Quoting James Hogan (2013-04-19 09:28:21)
> This patchset adds support for automatic selection of the best parent
> for a clock mux, i.e. the one which can provide the closest clock rate
> to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
> so that it doesn't happen unless explicitly allowed.
> 

I'm not sure about the new flag.  One of my regrets during the early
development of the common struct clk is the CLK_SET_RATE_PARENT flag.
In hindsight I would have preferred to make parent-propagation of rate
changes the default behavior, and instead provide a flag to prevent that
behavior, such as CLK_SET_RATE_NO_PROPAGATE.

One reason for this is the difficulty some have had with setting flags
from DT bindings.  Another reason is that a core goal of the framework
is to remove topology info from drivers; having to set a flag explicitly
on each clock to propagate rate changes is an impediment to that goal.

So perhaps CLK_SET_RATE_REMUX is a similar situation.  What do you think
about making remuxing the default and providing a flag to prevent it?
And also we can assume for the immediate future that users of
.determine_rate WANT to remux, since .round_rate won't do it.  Though
eventually it might be wise to convert all users of .round_rate over to
.determine_rate and deprecate .round_rate entirely.

Regards,
Mike

> This works by way of adding a new op, determine_rate, similar to
> round_rate but with an extra parameter to allow the clock driver to
> optionally select a different parent clock. This is used in
> clk_calc_new_rates to decide whether to initiate a set_parent operation.
> 
> Changes in v2:
> 
> I've moved the mux determine_rate implementation into a core helper, but
> I haven't pushed it fully into the core, as I think it just wouldn't
> work correctly for more complex clocks, e.g. if you (theoretically) had
> a combined mux and divide, you'd want to intercept the determine_rate
> and ask for a larger rate from the parent clocks, then return the
> divided rate. This should be possible by wrapping the mux determine_rate
> helper.
> 
> Patch 1 still exports the __clk_get_parent_by_index as it seems like it
> might be a useful thing for clock implementations to have access to if
> they ever wanted to do something more fancy with changing clock parents.
> 
> I haven't made any attempt to implement the atomic set_parent+set_rate
> as I don't have hardware that could take proper advantage of it, but it
> shouldn't be too difficult for others to implement if they wanted since
> they're fairly close to one another (in clk_change_rate()).
> 
> * switched to using new determine_rate op rather than adding an argument
>   to round_rate.
> * moved mux implementation into a single helper which should be usable
>   from more complex clocks which can mux.
> * rewrite main implementation so that no changes are made until after
>   the PRE notifications have been sent, and in a way that should ensure
>   correct notifications without duplicates, and I think should be safe
>   in the event of a notification failing.
> * various tidy ups and fixes.
> 
> James Hogan (3):
>   clk: abstract parent cache
>   clk: add support for clock reparent on set_rate
>   clk: clk-mux: implement remuxing on set_rate
> 
>  Documentation/clk.txt        |   4 +
>  drivers/clk/clk-mux.c        |   1 +
>  drivers/clk/clk.c            | 216 +++++++++++++++++++++++++++++++++----------
>  include/linux/clk-private.h  |   2 +
>  include/linux/clk-provider.h |  12 +++
>  5 files changed, 185 insertions(+), 50 deletions(-)
> 
> -- 
> 1.8.1.2

Re: [PATCH v2 1/3] clk: abstract parent cache

[Mike Turquette]

Quoting James Hogan (2013-04-19 09:28:22)
> Abstract access to the clock parent cache by defining
> __clk_get_parent_by_index(clk, index). This allows access to parent
> clocks from clock drivers.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> ---
>  drivers/clk/clk.c            | 21 ++++++++++++++-------
>  include/linux/clk-provider.h |  1 +
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index ed87b24..79d5deb 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -415,6 +415,19 @@ struct clk *__clk_get_parent(struct clk *clk)
>         return !clk ? NULL : clk->parent;
>  }
>  
> +struct clk *__clk_get_parent_by_index(struct clk *clk, u8 index)

This looks good, but in the next version can you remove the double
underscore?  I'm getting rid of a lot of those double underscore
helpers now that the reentrancy patch is merged and I think this is a
reasonable top-level API for a clock provider to have.

Thanks,
Mike

> +{
> +       if (!clk || index >= clk->num_parents)
> +               return NULL;
> +       else if (!clk->parents)
> +               return __clk_lookup(clk->parent_names[index]);
> +       else if (!clk->parents[index])
> +               return clk->parents[index] =
> +                       __clk_lookup(clk->parent_names[index]);
> +       else
> +               return clk->parents[index];
> +}
> +
>  unsigned int __clk_get_enable_count(struct clk *clk)
>  {
>         return !clk ? 0 : clk->enable_count;
> @@ -1150,13 +1163,7 @@ static struct clk *__clk_init_parent(struct clk *clk)
>                         kzalloc((sizeof(struct clk*) * clk->num_parents),
>                                         GFP_KERNEL);
>  
> -       if (!clk->parents)
> -               ret = __clk_lookup(clk->parent_names[index]);
> -       else if (!clk->parents[index])
> -               ret = clk->parents[index] =
> -                       __clk_lookup(clk->parent_names[index]);
> -       else
> -               ret = clk->parents[index];
> +       ret = __clk_get_parent_by_index(clk, index);
>  
>  out:
>         return ret;
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 7f197d7..4e0b634 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -347,6 +347,7 @@ const char *__clk_get_name(struct clk *clk);
>  struct clk_hw *__clk_get_hw(struct clk *clk);
>  u8 __clk_get_num_parents(struct clk *clk);
>  struct clk *__clk_get_parent(struct clk *clk);
> +struct clk *__clk_get_parent_by_index(struct clk *clk, u8 index);
>  unsigned int __clk_get_enable_count(struct clk *clk);
>  unsigned int __clk_get_prepare_count(struct clk *clk);
>  unsigned long __clk_get_rate(struct clk *clk);
> -- 
> 1.8.1.2

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[James Hogan]

On 13 May 2013 20:57, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting James Hogan (2013-04-19 09:28:21)
>> This patchset adds support for automatic selection of the best parent
>> for a clock mux, i.e. the one which can provide the closest clock rate
>> to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
>> so that it doesn't happen unless explicitly allowed.
>>
>
> I'm not sure about the new flag.  One of my regrets during the early
> development of the common struct clk is the CLK_SET_RATE_PARENT flag.
> In hindsight I would have preferred to make parent-propagation of rate
> changes the default behavior, and instead provide a flag to prevent that
> behavior, such as CLK_SET_RATE_NO_PROPAGATE.
>
> One reason for this is the difficulty some have had with setting flags
> from DT bindings.

Could you elaborate on this? I've been adding flags to DT bindings for
this sort of thing, but it feels a bit like it's in that grey area of
not really describing the hardware itself. This information needs to
be specified somehow though.

> Another reason is that a core goal of the framework
> is to remove topology info from drivers; having to set a flag explicitly
> on each clock to propagate rate changes is an impediment to that goal.

(Something that I have found also where CLK_SET_RATE_PARENT doesn't
fit well is when you have a mux where you want the set_rate propagated
to one possible parent (e.g. a dedicated PLL) but not the other (e.g.
the system clock - which happens to be a divider). In this case what I
really want to say is "don't change the rate of this particular
clock", in fact I added a CLK_DIVIDER_READ_ONLY divider flag locally
for this purpose. Was there a particular use case of
CLK_SET_RATE_PARENT where you'd want a clock to be changed from one
child but not another?)

>
> So perhaps CLK_SET_RATE_REMUX is a similar situation.  What do you think
> about making remuxing the default and providing a flag to prevent it?

Yes, fine by me.

> And also we can assume for the immediate future that users of
> .determine_rate WANT to remux, since .round_rate won't do it.  Though
> eventually it might be wise to convert all users of .round_rate over to
> .determine_rate and deprecate .round_rate entirely.

True.

Cheers
James

Re: [PATCH v2 2/3] clk: add support for clock reparent on set_rate

[Mike Turquette]

Quoting James Hogan (2013-04-19 09:28:23)
> 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.
> 

Hi James,

Can you rebase this onto -rc1?  It does not apply cleanly.

Thanks,
Mike

> The parent change takes place with the help of two 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, passing NULL to the new parent argument if not
> needed (e.g. in __clk_round_rate). This restructures a few of the call
> sites to simplify the logic into if/else blocks.
> 
> A new __clk_set_parent_no_recalc() is created similar to
> clk_set_parent() but without calling __clk_recalc_rates(). This is for
> clk_change_rate() to use, where rate recalculation and notifications are
> already handled.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> ---
>  Documentation/clk.txt        |   4 ++
>  drivers/clk/clk.c            | 146 ++++++++++++++++++++++++++++++-------------
>  include/linux/clk-private.h  |   2 +
>  include/linux/clk-provider.h |   7 +++
>  4 files changed, 116 insertions(+), 43 deletions(-)
> 
> diff --git a/Documentation/clk.txt b/Documentation/clk.txt
> index 1943fae..502c033 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 79d5deb..cc0e618 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -27,6 +27,8 @@ static HLIST_HEAD(clk_root_list);
>  static HLIST_HEAD(clk_orphan_list);
>  static LIST_HEAD(clk_notifier_list);
>  
> +static int __clk_set_parent_no_recalc(struct clk *clk, struct clk *parent);
> +
>  /***        debugfs support        ***/
>  
>  #ifdef CONFIG_COMMON_CLK_DEBUG
> @@ -727,17 +729,20 @@ unsigned long __clk_round_rate(struct clk *clk, unsigned long rate)
>         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);
> +       if (clk->ops->determine_rate)
> +               return clk->ops->determine_rate(clk->hw, rate, &parent_rate,
> +                                               NULL);
> +       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;
> +
>  }
>  
>  /**
> @@ -906,18 +911,23 @@ 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)
>  {
>         struct clk *child;
>  
>         clk->new_rate = new_rate;
> +       clk->new_parent = new_parent;
> +       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);
>         }
>  }
>  
> @@ -928,6 +938,7 @@ 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;
>  
> @@ -936,42 +947,43 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
>                 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;
> -       }
> -
> -       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);
>  
>         return top;
>  }
> @@ -983,7 +995,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)
> @@ -996,9 +1008,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;
> @@ -1016,6 +1038,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_no_recalc(clk, clk->new_parent);
> +
>         if (clk->parent)
>                 best_parent_rate = clk->parent->rate;
>  
> @@ -1030,8 +1056,15 @@ 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);
> +       }
> +
> +       if (clk->new_child)
> +               clk_change_rate(clk->new_child);
>  }
>  
>  /**
> @@ -1169,7 +1202,7 @@ out:
>         return ret;
>  }
>  
> -void __clk_reparent(struct clk *clk, struct clk *new_parent)
> +void __clk_reparent_no_recalc(struct clk *clk, struct clk *new_parent)
>  {
>  #ifdef CONFIG_COMMON_CLK_DEBUG
>         struct dentry *d;
> @@ -1179,6 +1212,9 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>         if (!clk || !new_parent)
>                 return;
>  
> +       if (new_parent->new_child == clk)
> +               new_parent->new_child = NULL;
> +
>         hlist_del(&clk->child_node);
>  
>         if (new_parent)
> @@ -1206,6 +1242,11 @@ out:
>  #endif
>  
>         clk->parent = new_parent;
> +}
> +
> +void __clk_reparent(struct clk *clk, struct clk *new_parent)
> +{
> +       __clk_reparent_no_recalc(clk, new_parent);
>  
>         __clk_recalc_rates(clk, POST_RATE_CHANGE);
>  }
> @@ -1270,6 +1311,25 @@ out:
>         return ret;
>  }
>  
> +static int __clk_set_parent_no_recalc(struct clk *clk, struct clk *parent)
> +{
> +       int ret = 0;
> +
> +       if (clk->parent == parent)
> +               goto out;
> +
> +       /* only re-parent if the clock is not in use */
> +       ret = __clk_set_parent(clk, parent);
> +       if (ret)
> +               goto out;
> +
> +       /* reparent, but don't propagate rate recalculation downstream */
> +       __clk_reparent_no_recalc(clk, parent);
> +
> +out:
> +       return ret;
> +}
> +
>  /**
>   * clk_set_parent - switch the parent of a mux clk
>   * @clk: the mux clk whose input we are switching
> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
> index 9c7f580..0826a60 100644
> --- a/include/linux/clk-private.h
> +++ b/include/linux/clk-private.h
> @@ -35,6 +35,8 @@ struct clk {
>         u8                      num_parents;
>         unsigned long           rate;
>         unsigned long           new_rate;
> +       struct clk              *new_parent;
> +       struct clk              *new_child;
>         unsigned long           flags;
>         unsigned int            enable_count;
>         unsigned int            prepare_count;
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 4e0b634..5fe1d38 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -71,6 +71,10 @@ struct clk_hw;
>   * @round_rate:        Given a target rate as input, returns the closest rate actually
>   *             supported by the clock.
>   *
> + * @determine_rate: Given a target rate as input, returns the closest rate
> + *             actually supported by the clock, and optionally the parent clock
> + *             that should be used to provide the clock rate.
> + *
>   * @get_parent:        Queries the hardware to determine the parent of a clock.  The
>   *             return value is a u8 which specifies the index corresponding to
>   *             the parent clock.  This index can be applied to either the
> @@ -116,6 +120,9 @@ struct clk_ops {
>                                         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,
> -- 
> 1.8.1.2

Re: [PATCH v2 2/3] clk: add support for clock reparent on set_rate

[James Hogan]

On 14 May 2013 19:13, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting James Hogan (2013-04-19 09:28:23)
>> 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.
>>
>
> Hi James,
>
> Can you rebase this onto -rc1?  It does not apply cleanly.

Hi Mike,

Sure, I rebased onto clk-next the other day so I'll update to rc1 and
resubmit tomorrow.

Cheers
James

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[James Hogan]

Hi Mike,

On 14/05/13 17:59, Mike Turquette wrote:
> Quoting James Hogan (2013-05-13 14:30:46)
>> On 13 May 2013 20:57, Mike Turquette <mturquette@linaro.org> wrote:
>>> One reason for this is the difficulty some have had with setting flags
>>> from DT bindings.
>>
>> Could you elaborate on this? I've been adding flags to DT bindings for
>> this sort of thing, but it feels a bit like it's in that grey area of
>> not really describing the hardware itself. This information needs to
>> be specified somehow though.
>>
> 
> It depends on the flag.  A good example is the CLK_DIVIDER_ONE_BASED
> flag which does describe the hardware.  It informs the binding that
> indexing starts at 1, not 0, which is a valid part of the hardware
> description.
> 
> However flags that deal with software policy do not belong on DT.
> CLK_SET_RATE_PARENT certainly does not belong in the DT binding since
> this is a pure Linux-ism.  Every binding just needs to be reviewed on a
> case-by-case basis to make sure the flags are related only to the
> hardware.

So given the desire to eliminate platform code, is there a particular
way that these other flags can be specified instead of DT bindings?

Cheers
James

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[Saravana Kannan]

On 04/19/2013 09:28 AM, James Hogan wrote:
> This patchset adds support for automatic selection of the best parent
> for a clock mux, i.e. the one which can provide the closest clock rate
> to that requested. It can be controlled by a new CLK_SET_RATE_REMUX flag
> so that it doesn't happen unless explicitly allowed.
>
> This works by way of adding a new op, determine_rate, similar to
> round_rate but with an extra parameter to allow the clock driver to
> optionally select a different parent clock. This is used in
> clk_calc_new_rates to decide whether to initiate a set_parent operation.
>
> Changes in v2:
>
> I've moved the mux determine_rate implementation into a core helper, but
> I haven't pushed it fully into the core, as I think it just wouldn't
> work correctly for more complex clocks, e.g. if you (theoretically) had
> a combined mux and divide, you'd want to intercept the determine_rate
> and ask for a larger rate from the parent clocks, then return the
> divided rate. This should be possible by wrapping the mux determine_rate
> helper.
>
> Patch 1 still exports the __clk_get_parent_by_index as it seems like it
> might be a useful thing for clock implementations to have access to if
> they ever wanted to do something more fancy with changing clock parents.
>
> I haven't made any attempt to implement the atomic set_parent+set_rate
> as I don't have hardware that could take proper advantage of it, but it
> shouldn't be too difficult for others to implement if they wanted since
> they're fairly close to one another (in clk_change_rate()).
>
> * switched to using new determine_rate op rather than adding an argument
>    to round_rate.
> * moved mux implementation into a single helper which should be usable
>    from more complex clocks which can mux.
> * rewrite main implementation so that no changes are made until after
>    the PRE notifications have been sent, and in a way that should ensure
>    correct notifications without duplicates, and I think should be safe
>    in the event of a notification failing.
> * various tidy ups and fixes.
>
> James Hogan (3):
>    clk: abstract parent cache
>    clk: add support for clock reparent on set_rate
>    clk: clk-mux: implement remuxing on set_rate
>
>   Documentation/clk.txt        |   4 +
>   drivers/clk/clk-mux.c        |   1 +
>   drivers/clk/clk.c            | 216 +++++++++++++++++++++++++++++++++----------
>   include/linux/clk-private.h  |   2 +
>   include/linux/clk-provider.h |  12 +++
>   5 files changed, 185 insertions(+), 50 deletions(-)
>

I really want to review this because I solved the same problem in our 
internal clock framework and want to make sure upstream doesn't go 
through the same mistakes. But haven't had the time. Hopefully, I'll go 
it before this gets accepted by Mike. I'll try to do it this week/weekend.

Thanks,
Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 0/3] clk: implement remuxing during set_rate

[James Hogan]

On 16/05/13 05:11, Saravana Kannan wrote:
> I really want to review this because I solved the same problem in our
> internal clock framework and want to make sure upstream doesn't go
> through the same mistakes. But haven't had the time. Hopefully, I'll go
> it before this gets accepted by Mike. I'll try to do it this week/weekend.

Great! FYI I submitted a v3 yesterday, but screwed up and forgot to CC
LKML :-(

It can be found on linux-arm-kernel archives here (or if it's a real
PITA I can resubmit cc'ing LKML):

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168485.html

Cheers
James