[PATCH v2 00/16] clk: a collection of various clean-ups and improvements

[PATCH v2 00/16] clk: a collection of various clean-ups and improvements

[Masahiro Yamada]

Many refactoring, with detection of circular parent looping.


Changes in v2:
  - Fix a bug.  Return -EINVAL when parent _is_ NULL.

Masahiro Yamada (16):
  clk: remove unused first argument of __clk_init()
  clk: change the argument of __clk_init() into pointer to clk_core
  clk: rename __clk_init() into __clk_core_init()
  clk: remove unnecessary !core->parents conditional
  clk: change sizeof(struct clk *) to sizeof(*core->parents)
  clk: move core->parents allocation to clk_register()
  clk: simplify clk_core_get_parent_by_index()
  clk: drop the initial core->parents look-ups from __clk_core_init()
  clk: replace pr_warn() with pr_err() for fatal cases
  clk: move checking .git_parent to __clk_core_init()
  clk: simplify __clk_init_parent()
  clk: avoid circular clock topology
  clk: walk the orphan clock list more simply
  clk: make sure parent is not NULL in clk_fetch_parent_index()
  clk: simplify clk_fetch_parent_index() function
  clk: slightly optimize clk_core_set_parent()

 drivers/clk/clk.c | 214 ++++++++++++++++++++++--------------------------------
 1 file changed, 85 insertions(+), 129 deletions(-)

-- 
1.9.1

[PATCH v2 01/16] clk: remove unused first argument of __clk_init()

[Masahiro Yamada]

The "struct device *dev" is not used at all in this function.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3fba5ac..897e5ae 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2286,13 +2286,12 @@ static inline void clk_debug_unregister(struct clk_core *core)
 
 /**
  * __clk_init - initialize the data structures in a struct clk
- * @dev:	device initializing this clk, placeholder for now
  * @clk:	clk being initialized
  *
  * Initializes the lists in struct clk_core, queries the hardware for the
  * parent and rate and sets them both.
  */
-static int __clk_init(struct device *dev, struct clk *clk_user)
+static int __clk_init(struct clk *clk_user)
 {
 	int i, ret = 0;
 	struct clk_core *orphan;
@@ -2575,7 +2574,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		goto fail_parent_names_copy;
 	}
 
-	ret = __clk_init(dev, hw->clk);
+	ret = __clk_init(hw->clk);
 	if (!ret)
 		return hw->clk;
 
-- 
1.9.1

[PATCH v2 02/16] clk: change the argument of __clk_init() into pointer to clk_core

[Masahiro Yamada]

The argument clk_user is used only for the clk_user->core.  The rest
of this function only takes care of clk_core.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 897e5ae..623838f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2285,25 +2285,22 @@ static inline void clk_debug_unregister(struct clk_core *core)
 #endif
 
 /**
- * __clk_init - initialize the data structures in a struct clk
- * @clk:	clk being initialized
+ * __clk_init - initialize the data structures in a struct clk_core
+ * @core:	clk_core being initialized
  *
  * Initializes the lists in struct clk_core, queries the hardware for the
  * parent and rate and sets them both.
  */
-static int __clk_init(struct clk *clk_user)
+static int __clk_init(struct clk_core *core)
 {
 	int i, ret = 0;
 	struct clk_core *orphan;
 	struct hlist_node *tmp2;
-	struct clk_core *core;
 	unsigned long rate;
 
-	if (!clk_user)
+	if (!core)
 		return -EINVAL;
 
-	core = clk_user->core;
-
 	clk_prepare_lock();
 
 	/* check to see if a clock with this name is already registered */
@@ -2574,7 +2571,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		goto fail_parent_names_copy;
 	}
 
-	ret = __clk_init(hw->clk);
+	ret = __clk_init(core);
 	if (!ret)
 		return hw->clk;
 
-- 
1.9.1

[PATCH v2 03/16] clk: rename __clk_init() into __clk_core_init()

[Masahiro Yamada]

Now this function takes clk_core as its argument.  __clk_core_init()
would be more suitable for the name of this function.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 623838f..41179f3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2285,13 +2285,13 @@ static inline void clk_debug_unregister(struct clk_core *core)
 #endif
 
 /**
- * __clk_init - initialize the data structures in a struct clk_core
+ * __clk_core_init - initialize the data structures in a struct clk_core
  * @core:	clk_core being initialized
  *
  * Initializes the lists in struct clk_core, queries the hardware for the
  * parent and rate and sets them both.
  */
-static int __clk_init(struct clk_core *core)
+static int __clk_core_init(struct clk_core *core)
 {
 	int i, ret = 0;
 	struct clk_core *orphan;
@@ -2571,7 +2571,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		goto fail_parent_names_copy;
 	}
 
-	ret = __clk_init(core);
+	ret = __clk_core_init(core);
 	if (!ret)
 		return hw->clk;
 
-- 
1.9.1

[PATCH v2 04/16] clk: remove unnecessary !core->parents conditional

[Masahiro Yamada]

This if-block has been here since the introduction of the common
clock framework.  Now no clock drivers are statically initialized.
core->parent is always NULL at this point.  Drop the redundant
check and the confusing comment.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 41179f3..30c0bbc 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2348,11 +2348,8 @@ static int __clk_core_init(struct clk_core *core)
 	 * in to clk_init during early boot; thus any access to core->parents[]
 	 * must always check for a NULL pointer and try to populate it if
 	 * necessary.
-	 *
-	 * If core->parents is not NULL we skip this entire block.  This allows
-	 * for clock drivers to statically initialize core->parents.
 	 */
-	if (core->num_parents > 1 && !core->parents) {
+	if (core->num_parents > 1) {
 		core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
 					GFP_KERNEL);
 		/*
-- 
1.9.1

[PATCH v2 05/16] clk: change sizeof(struct clk *) to sizeof(*core->parents)

[Masahiro Yamada]

Now, the clock parent is not "struct clk *", but "struct clk_core *".
Of course, the size of a pointer is always same, but strictly speaking,
sizeof(struct clk *) should be sizeof(struct clk_core *) here.

This mismatch happened when we split the structure into struct clk
and struct clk_core.  For the potential possibility of future renaming,
sizeof(*core->parents) would be better.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 30c0bbc..98745c3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1069,7 +1069,7 @@ static int clk_fetch_parent_index(struct clk_core *core,
 
 	if (!core->parents) {
 		core->parents = kcalloc(core->num_parents,
-					sizeof(struct clk *), GFP_KERNEL);
+					sizeof(*core->parents), GFP_KERNEL);
 		if (!core->parents)
 			return -ENOMEM;
 	}
@@ -1702,7 +1702,7 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
 
 	if (!core->parents)
 		core->parents =
-			kcalloc(core->num_parents, sizeof(struct clk *),
+			kcalloc(core->num_parents, sizeof(*core->parents),
 					GFP_KERNEL);
 
 	ret = clk_core_get_parent_by_index(core, index);
@@ -2350,8 +2350,8 @@ static int __clk_core_init(struct clk_core *core)
 	 * necessary.
 	 */
 	if (core->num_parents > 1) {
-		core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
-					GFP_KERNEL);
+		core->parents = kcalloc(core->num_parents,
+					sizeof(*core->parents), GFP_KERNEL);
 		/*
 		 * clk_core_lookup returns NULL for parents that have not been
 		 * clk_init'd; thus any access to clk->parents[] must check
-- 
1.9.1

[PATCH v2 06/16] clk: move core->parents allocation to clk_register()

[Masahiro Yamada]

Currently, __clk_core_init() allows failure of the kcalloc() for the
core->parents.  So, clk_fetch_parent_index() and __clk_init_parent()
also try to allocate core->parents in case it has not been allocated
yet.  Scattering memory allocation here and there makes things
complicated.

Like other clk_core members, allocate core->parents in clk_register()
and let it fail in case of memory shortage.  If we cannot allocate
such a small piece of memory, the system is already insane.  There is
no point to postpone the memory allocation.

Also, allocate core->parents regardless of core->num_parents.  We want
it even if core->num_parents == 1 because clk_fetch_parent_index()
might be called against the clk_core with a single parent.

If core->num_parents == 0, core->parents is set to ZERO_SIZE_PTR. It
is harmless because no access happens to core->parents in such a case.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 51 +++++++++++++++++++--------------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 98745c3..b5be02a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1067,13 +1067,6 @@ static int clk_fetch_parent_index(struct clk_core *core,
 {
 	int i;
 
-	if (!core->parents) {
-		core->parents = kcalloc(core->num_parents,
-					sizeof(*core->parents), GFP_KERNEL);
-		if (!core->parents)
-			return -ENOMEM;
-	}
-
 	/*
 	 * find index of new parent clock using cached parent ptrs,
 	 * or if not yet cached, use string name comparison and cache
@@ -1700,11 +1693,6 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
 
 	index = core->ops->get_parent(core->hw);
 
-	if (!core->parents)
-		core->parents =
-			kcalloc(core->num_parents, sizeof(*core->parents),
-					GFP_KERNEL);
-
 	ret = clk_core_get_parent_by_index(core, index);
 
 out:
@@ -2343,26 +2331,15 @@ static int __clk_core_init(struct clk_core *core)
 				__func__, core->name);
 
 	/*
-	 * Allocate an array of struct clk *'s to avoid unnecessary string
-	 * look-ups of clk's possible parents.  This can fail for clocks passed
-	 * in to clk_init during early boot; thus any access to core->parents[]
-	 * must always check for a NULL pointer and try to populate it if
-	 * necessary.
+	 * clk_core_lookup returns NULL for parents that have not been
+	 * clk_init'd; thus any access to clk->parents[] must check
+	 * for a NULL pointer.  We can always perform lazy lookups for
+	 * missing parents later on.
 	 */
-	if (core->num_parents > 1) {
-		core->parents = kcalloc(core->num_parents,
-					sizeof(*core->parents), GFP_KERNEL);
-		/*
-		 * clk_core_lookup returns NULL for parents that have not been
-		 * clk_init'd; thus any access to clk->parents[] must check
-		 * for a NULL pointer.  We can always perform lazy lookups for
-		 * missing parents later on.
-		 */
-		if (core->parents)
-			for (i = 0; i < core->num_parents; i++)
-				core->parents[i] =
-					clk_core_lookup(core->parent_names[i]);
-	}
+	if (core->parents)
+		for (i = 0; i < core->num_parents; i++)
+			core->parents[i] =
+				clk_core_lookup(core->parent_names[i]);
 
 	core->parent = __clk_init_parent(core);
 
@@ -2560,12 +2537,20 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		}
 	}
 
+	/* avoid unnecessary string look-ups of clk_core's possible parents. */
+	core->parents = kcalloc(core->num_parents, sizeof(*core->parents),
+				GFP_KERNEL);
+	if (!core->parents) {
+		ret = -ENOMEM;
+		goto fail_parents;
+	};
+
 	INIT_HLIST_HEAD(&core->clks);
 
 	hw->clk = __clk_create_clk(hw, NULL, NULL);
 	if (IS_ERR(hw->clk)) {
 		ret = PTR_ERR(hw->clk);
-		goto fail_parent_names_copy;
+		goto fail_parents;
 	}
 
 	ret = __clk_core_init(core);
@@ -2575,6 +2560,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 	__clk_free_clk(hw->clk);
 	hw->clk = NULL;
 
+fail_parents:
+	kfree(core->parents);
 fail_parent_names_copy:
 	while (--i >= 0)
 		kfree_const(core->parent_names[i]);
-- 
1.9.1

[PATCH v2 07/16] clk: simplify clk_core_get_parent_by_index()

[Masahiro Yamada]

Drop the "if (!core->parents)" case and refactor the function a bit
because core->parents is always allocated.  (Strictly speaking, it is
ZERO_SIZE_PTR if core->num_parents == 0, but such a case is omitted
by the if-conditional above.)

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b5be02a..ca7849a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -350,13 +350,12 @@ static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
 {
 	if (!core || index >= core->num_parents)
 		return NULL;
-	else if (!core->parents)
-		return clk_core_lookup(core->parent_names[index]);
-	else if (!core->parents[index])
-		return core->parents[index] =
-			clk_core_lookup(core->parent_names[index]);
-	else
-		return core->parents[index];
+
+	if (!core->parents[index])
+		core->parents[index] =
+				clk_core_lookup(core->parent_names[index]);
+
+	return core->parents[index];
 }
 
 struct clk_hw *
-- 
1.9.1

[PATCH v2 08/16] clk: drop the initial core->parents look-ups from __clk_core_init()

[Masahiro Yamada]

The core->parents is a cache to save expensive clock parent look-ups.
It will be filled as needed later.  We do not have to do it here.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ca7849a..3215b2b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2329,17 +2329,6 @@ static int __clk_core_init(struct clk_core *core)
 				"%s: invalid NULL in %s's .parent_names\n",
 				__func__, core->name);
 
-	/*
-	 * clk_core_lookup returns NULL for parents that have not been
-	 * clk_init'd; thus any access to clk->parents[] must check
-	 * for a NULL pointer.  We can always perform lazy lookups for
-	 * missing parents later on.
-	 */
-	if (core->parents)
-		for (i = 0; i < core->num_parents; i++)
-			core->parents[i] =
-				clk_core_lookup(core->parent_names[i]);
-
 	core->parent = __clk_init_parent(core);
 
 	/*
-- 
1.9.1

[PATCH v2 09/16] clk: replace pr_warn() with pr_err() for fatal cases

[Masahiro Yamada]

These three cases let clk_register() fail.  They should be considered
as error messages.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3215b2b..e6e10f5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2302,22 +2302,22 @@ static int __clk_core_init(struct clk_core *core)
 	if (core->ops->set_rate &&
 	    !((core->ops->round_rate || core->ops->determine_rate) &&
 	      core->ops->recalc_rate)) {
-		pr_warning("%s: %s must implement .round_rate or .determine_rate in addition to .recalc_rate\n",
-				__func__, core->name);
+		pr_err("%s: %s must implement .round_rate or .determine_rate in addition to .recalc_rate\n",
+		       __func__, core->name);
 		ret = -EINVAL;
 		goto out;
 	}
 
 	if (core->ops->set_parent && !core->ops->get_parent) {
-		pr_warning("%s: %s must implement .get_parent & .set_parent\n",
-				__func__, core->name);
+		pr_err("%s: %s must implement .get_parent & .set_parent\n",
+		       __func__, core->name);
 		ret = -EINVAL;
 		goto out;
 	}
 
 	if (core->ops->set_rate_and_parent &&
 			!(core->ops->set_parent && core->ops->set_rate)) {
-		pr_warn("%s: %s must implement .set_parent & .set_rate\n",
+		pr_err("%s: %s must implement .set_parent & .set_rate\n",
 				__func__, core->name);
 		ret = -EINVAL;
 		goto out;
-- 
1.9.1

[PATCH v2 10/16] clk: move checking .git_parent to __clk_core_init()

[Masahiro Yamada]

The .git_parent is mandatory for multi-parent clocks.  Move the check
to __clk_core_init(), like other callback checkings.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e6e10f5..4ad0a36 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1677,13 +1677,6 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
 		goto out;
 	}
 
-	if (!core->ops->get_parent) {
-		WARN(!core->ops->get_parent,
-			"%s: multi-parent clocks must implement .get_parent\n",
-			__func__);
-		goto out;
-	}
-
 	/*
 	 * Do our best to cache parent clocks in core->parents.  This prevents
 	 * unnecessary and expensive lookups.  We don't set core->parent here;
@@ -2315,6 +2308,11 @@ static int __clk_core_init(struct clk_core *core)
 		goto out;
 	}
 
+	if (core->num_parents > 1 && !core->ops->get_parent) {
+		pr_err("%s: %s must implement .get_parent as it has multi parents\n",
+		       __func__, core->name);
+	}
+
 	if (core->ops->set_rate_and_parent &&
 			!(core->ops->set_parent && core->ops->set_rate)) {
 		pr_err("%s: %s must implement .set_parent & .set_rate\n",
-- 
1.9.1

[PATCH v2 11/16] clk: simplify __clk_init_parent()

[Masahiro Yamada]

The translation from the index into clk_core is done by
clk_core_get_parent_by_index().  The if-block for num_parents == 1
case is duplicating the code in the clk_core_get_parent_by_index().

Drop the "if (num_parents == 1)" from the special case.  Instead,
set the index to zero if .get_parent() is missing.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 38 ++++----------------------------------
 1 file changed, 4 insertions(+), 34 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4ad0a36..03b87d8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1651,44 +1651,14 @@ struct clk *clk_get_parent(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_get_parent);
 
-/*
- * .get_parent is mandatory for clocks with multiple possible parents.  It is
- * optional for single-parent clocks.  Always call .get_parent if it is
- * available and WARN if it is missing for multi-parent clocks.
- *
- * For single-parent clocks without .get_parent, first check to see if the
- * .parents array exists, and if so use it to avoid an expensive tree
- * traversal.  If .parents does not exist then walk the tree.
- */
 static struct clk_core *__clk_init_parent(struct clk_core *core)
 {
-	struct clk_core *ret = NULL;
-	u8 index;
-
-	/* handle the trivial cases */
+	u8 index = 0;
 
-	if (!core->num_parents)
-		goto out;
-
-	if (core->num_parents == 1) {
-		if (IS_ERR_OR_NULL(core->parent))
-			core->parent = clk_core_lookup(core->parent_names[0]);
-		ret = core->parent;
-		goto out;
-	}
+	if (core->ops->get_parent)
+		index = core->ops->get_parent(core->hw);
 
-	/*
-	 * Do our best to cache parent clocks in core->parents.  This prevents
-	 * unnecessary and expensive lookups.  We don't set core->parent here;
-	 * that is done by the calling function.
-	 */
-
-	index = core->ops->get_parent(core->hw);
-
-	ret = clk_core_get_parent_by_index(core, index);
-
-out:
-	return ret;
+	return clk_core_get_parent_by_index(core, index);
 }
 
 static void clk_core_reparent(struct clk_core *core,
-- 
1.9.1

[PATCH v2 12/16] clk: avoid circular clock topology

[Masahiro Yamada]

Currently, clk_register() never checks a circular parent looping,
but clock providers could register such an insane clock topology.
For example, "clk_a" could have "clk_b" as a parent, and vice versa.
In this case, clk_core_reparent() creates a circular parent list
and __clk_recalc_accuracies() calls itself recursively forever.

The core infrastructure should be kind enough to bail out, showing
an appropriate error message in such a case.  This helps to easily
find a bug in clock providers.  (uh, I made such a silly mistake
when I was implementing my clock providers first.  I was upset
because the kernel did not respond, without any error message.)

This commit adds a new helper function, __clk_is_ancestor().  It
returns true if the second argument is a possible ancestor of the
first one.  If a clock core is a possible ancestor of itself, it
would make a loop when it were registered.  That should be detected
as an error.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 03b87d8..a4f66fe 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2235,6 +2235,38 @@ static inline void clk_debug_unregister(struct clk_core *core)
 #endif
 
 /**
+ * __clk_is_ancestor - check if a clk_core is a possible ancestor of another
+ * @core: clock core
+ * @ancestor: ancestor clock core
+ *
+ * Returns true if there is a possibility that @ancestor can be an ancestor
+ * of @core, false otherwise.
+ *
+ * This function can be used against @core or @ancestor that has not been
+ * registered yet.
+ */
+static bool __clk_is_ancestor(struct clk_core *core, struct clk_core *ancestor)
+{
+	struct clk_core *parent;
+	int i;
+
+	for (i = 0; i < core->num_parents; i++) {
+		parent = clk_core_get_parent_by_index(core, i);
+		/*
+		 * If ancestor has not been added to clk_{root,orphan}_list
+		 * yet, clk_core_lookup() cannot find it.  If parent is NULL,
+		 * compare the name strings, too.
+		 */
+		if ((parent && (parent == ancestor ||
+				__clk_is_ancestor(parent, ancestor))) ||
+		    (!parent && !strcmp(core->parent_names[i], ancestor->name)))
+			return true;
+	}
+
+	return false;
+}
+
+/**
  * __clk_core_init - initialize the data structures in a struct clk_core
  * @core:	clk_core being initialized
  *
@@ -2297,6 +2329,14 @@ static int __clk_core_init(struct clk_core *core)
 				"%s: invalid NULL in %s's .parent_names\n",
 				__func__, core->name);
 
+	/* If core is an ancestor of itself, it would make a loop. */
+	if (__clk_is_ancestor(core, core)) {
+		pr_err("%s: %s would create circular parent\n", __func__,
+		       core->name);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	core->parent = __clk_init_parent(core);
 
 	/*
-- 
1.9.1

[PATCH v2 13/16] clk: walk the orphan clock list more simply

[Masahiro Yamada]

This loop can be much simpler.  If a new parent is available for
orphan clocks, __clk_init_parent(orphan) can detect it.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a4f66fe..efd093a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2402,24 +2402,15 @@ static int __clk_core_init(struct clk_core *core)
 	core->rate = core->req_rate = rate;
 
 	/*
-	 * walk the list of orphan clocks and reparent any that are children of
-	 * this clock
+	 * walk the list of orphan clocks and reparent any that newly finds a
+	 * parent.
 	 */
 	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
-		if (orphan->num_parents && orphan->ops->get_parent) {
-			i = orphan->ops->get_parent(orphan->hw);
-			if (i >= 0 && i < orphan->num_parents &&
-			    !strcmp(core->name, orphan->parent_names[i]))
-				clk_core_reparent(orphan, core);
-			continue;
-		}
+		struct clk_core *parent = __clk_init_parent(orphan);
 
-		for (i = 0; i < orphan->num_parents; i++)
-			if (!strcmp(core->name, orphan->parent_names[i])) {
-				clk_core_reparent(orphan, core);
-				break;
-			}
-	 }
+		if (parent)
+			clk_core_reparent(orphan, parent);
+	}
 
 	/*
 	 * optional platform-specific magic
-- 
1.9.1

[PATCH v2 14/16] clk: make sure parent is not NULL in clk_fetch_parent_index()

[Masahiro Yamada]

If parent is given with NULL, clk_fetch_parent_index() could return
a positive index value.

Currently, parent is checked by the callers of this function, but
it would be safer to do it in this function.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Fix a bug.  Return -EINVAL when parent _is_ NULL.

 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index efd093a..151cbe8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1066,6 +1066,9 @@ static int clk_fetch_parent_index(struct clk_core *core,
 {
 	int i;
 
+	if (!parent)
+		return -EINVAL;
+
 	/*
 	 * find index of new parent clock using cached parent ptrs,
 	 * or if not yet cached, use string name comparison and cache
-- 
1.9.1

[PATCH v2 15/16] clk: simplify clk_fetch_parent_index() function

[Masahiro Yamada]

The clk_core_get_parent_by_index can be used as a helper function
to simplify the implementation of clk_fetch_parent_index().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 151cbe8..5b9aec0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1069,23 +1069,9 @@ static int clk_fetch_parent_index(struct clk_core *core,
 	if (!parent)
 		return -EINVAL;
 
-	/*
-	 * find index of new parent clock using cached parent ptrs,
-	 * or if not yet cached, use string name comparison and cache
-	 * them now to avoid future calls to clk_core_lookup.
-	 */
-	for (i = 0; i < core->num_parents; i++) {
-		if (core->parents[i] == parent)
-			return i;
-
-		if (core->parents[i])
-			continue;
-
-		if (!strcmp(core->parent_names[i], parent->name)) {
-			core->parents[i] = clk_core_lookup(parent->name);
+	for (i = 0; i < core->num_parents; i++)
+		if (clk_core_get_parent_by_index(core, i) == parent)
 			return i;
-		}
-	}
 
 	return -EINVAL;
 }
-- 
1.9.1

[PATCH v2 16/16] clk: slightly optimize clk_core_set_parent()

[Masahiro Yamada]

If clk_fetch_parent_index() fails, p_rate is unused.  Move the
assignment after the error checking.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5b9aec0..e81301b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1730,13 +1730,13 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 	/* try finding the new parent index */
 	if (parent) {
 		p_index = clk_fetch_parent_index(core, parent);
-		p_rate = parent->rate;
 		if (p_index < 0) {
 			pr_debug("%s: clk %s can not be parent of clk %s\n",
 					__func__, parent->name, core->name);
 			ret = p_index;
 			goto out;
 		}
+		p_rate = parent->rate;
 	}
 
 	/* propagate PRE_RATE_CHANGE notifications */
-- 
1.9.1

Re: [PATCH v2 12/16] clk: avoid circular clock topology

[Joachim Eastwood]

Hi everyone,

On 28 December 2015 at 11:10, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Currently, clk_register() never checks a circular parent looping,
> but clock providers could register such an insane clock topology.
> For example, "clk_a" could have "clk_b" as a parent, and vice versa.
> In this case, clk_core_reparent() creates a circular parent list
> and __clk_recalc_accuracies() calls itself recursively forever.
>
> The core infrastructure should be kind enough to bail out, showing
> an appropriate error message in such a case.  This helps to easily
> find a bug in clock providers.  (uh, I made such a silly mistake
> when I was implementing my clock providers first.  I was upset
> because the kernel did not respond, without any error message.)
>
> This commit adds a new helper function, __clk_is_ancestor().  It
> returns true if the second argument is a possible ancestor of the
> first one.  If a clock core is a possible ancestor of itself, it
> would make a loop when it were registered.  That should be detected
> as an error.

This commit breaks lpc18xx boot in next right now. See
http://marc.info/?l=linux-arm-kernel&m=145608597106087&w=2

The Clock Generation Unit (CGU) on lpc18xx allow for circular parents
in hardware. While it is obliviously not a good idea to configure the
clocks in that manner there is nothing that stops you either.

Please take a look at the second figure on:
https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks
All PLLs can feed clock into the dividers and the dividers can feed
clock into the PLLs.

The reason why this is made possible in the CGU is because you can
then choose where to put your divider; either before the PLL or after.


regards,
Joachim Eastwood

Re: [PATCH v2 12/16] clk: avoid circular clock topology

[Masahiro Yamada]

Hi Joachim,


2016-02-22 6:39 GMT+09:00 Joachim Eastwood <manabian@gmail.com>:
> Hi everyone,
>
> On 28 December 2015 at 11:10, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> Currently, clk_register() never checks a circular parent looping,
>> but clock providers could register such an insane clock topology.
>> For example, "clk_a" could have "clk_b" as a parent, and vice versa.
>> In this case, clk_core_reparent() creates a circular parent list
>> and __clk_recalc_accuracies() calls itself recursively forever.
>>
>> The core infrastructure should be kind enough to bail out, showing
>> an appropriate error message in such a case.  This helps to easily
>> find a bug in clock providers.  (uh, I made such a silly mistake
>> when I was implementing my clock providers first.  I was upset
>> because the kernel did not respond, without any error message.)
>>
>> This commit adds a new helper function, __clk_is_ancestor().  It
>> returns true if the second argument is a possible ancestor of the
>> first one.  If a clock core is a possible ancestor of itself, it
>> would make a loop when it were registered.  That should be detected
>> as an error.
>
> This commit breaks lpc18xx boot in next right now. See
> http://marc.info/?l=linux-arm-kernel&m=145608597106087&w=2
>
> The Clock Generation Unit (CGU) on lpc18xx allow for circular parents
> in hardware. While it is obliviously not a good idea to configure the
> clocks in that manner there is nothing that stops you either.
>
> Please take a look at the second figure on:
> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks
> All PLLs can feed clock into the dividers and the dividers can feed
> clock into the PLLs.
>
> The reason why this is made possible in the CGU is because you can
> then choose where to put your divider; either before the PLL or after.
>


Sorry for breaking your board.

I am OK with reverting b58f75aa83fb.



I guess your hardware could make clock looping for the best flexibility
but you do not make clock looping in actual use cases.

Maybe, does it make sense to check the parent looping
in clk_set_parent() or somewhere, not in clk_register()?


-- 
Best Regards
Masahiro Yamada

[PATCH] Revert "clk: avoid circular clock topology"

[Stephen Boyd]

This reverts commit 858d5881564026cbc4e6f5e25ae878a27df5d4c9.

Joachim reports that this commit breaks lpc18xx boot. This is
because the hardware has circular clk topology where PLLs can
feed into dividers and the same dividers can feed into the PLLs.
The hardware is designed this way so that you can choose to put
the divider before the PLL or after the PLL depending on what you
configure to be the parent of the divider and what you configure
to be the parent of the PLL.

So let's drop this patch for now because we have hardware that
actually has loops. A future patch could check for circular
parents when we change parents and fail the switch, but that's
probably best left to some debugging Kconfig option so that we
don't suffer the sanity checking cost all the time.

Reported-by: Joachim Eastwood <manabian@gmail.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk.c | 40 ----------------------------------------
 1 file changed, 40 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 58ef3dab894a..51d673370d42 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2242,38 +2242,6 @@ static inline void clk_debug_unregister(struct clk_core *core)
 #endif
 
 /**
- * __clk_is_ancestor - check if a clk_core is a possible ancestor of another
- * @core: clock core
- * @ancestor: ancestor clock core
- *
- * Returns true if there is a possibility that @ancestor can be an ancestor
- * of @core, false otherwise.
- *
- * This function can be used against @core or @ancestor that has not been
- * registered yet.
- */
-static bool __clk_is_ancestor(struct clk_core *core, struct clk_core *ancestor)
-{
-	struct clk_core *parent;
-	int i;
-
-	for (i = 0; i < core->num_parents; i++) {
-		parent = clk_core_get_parent_by_index(core, i);
-		/*
-		 * If ancestor has not been added to clk_{root,orphan}_list
-		 * yet, clk_core_lookup() cannot find it.  If parent is NULL,
-		 * compare the name strings, too.
-		 */
-		if ((parent && (parent == ancestor ||
-				__clk_is_ancestor(parent, ancestor))) ||
-		    (!parent && !strcmp(core->parent_names[i], ancestor->name)))
-			return true;
-	}
-
-	return false;
-}
-
-/**
  * __clk_core_init - initialize the data structures in a struct clk_core
  * @core:	clk_core being initialized
  *
@@ -2338,14 +2306,6 @@ static int __clk_core_init(struct clk_core *core)
 				"%s: invalid NULL in %s's .parent_names\n",
 				__func__, core->name);
 
-	/* If core is an ancestor of itself, it would make a loop. */
-	if (__clk_is_ancestor(core, core)) {
-		pr_err("%s: %s would create circular parent\n", __func__,
-		       core->name);
-		ret = -EINVAL;
-		goto out;
-	}
-
 	core->parent = __clk_init_parent(core);
 
 	/*
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 12/16] clk: avoid circular clock topology

[Joachim Eastwood]

On 22 February 2016 at 03:29, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Hi Joachim,
>
>
> 2016-02-22 6:39 GMT+09:00 Joachim Eastwood <manabian@gmail.com>:
>> Hi everyone,
>>
>> On 28 December 2015 at 11:10, Masahiro Yamada
>> <yamada.masahiro@socionext.com> wrote:
>>> Currently, clk_register() never checks a circular parent looping,
>>> but clock providers could register such an insane clock topology.
>>> For example, "clk_a" could have "clk_b" as a parent, and vice versa.
>>> In this case, clk_core_reparent() creates a circular parent list
>>> and __clk_recalc_accuracies() calls itself recursively forever.
>>>
>>> The core infrastructure should be kind enough to bail out, showing
>>> an appropriate error message in such a case.  This helps to easily
>>> find a bug in clock providers.  (uh, I made such a silly mistake
>>> when I was implementing my clock providers first.  I was upset
>>> because the kernel did not respond, without any error message.)
>>>
>>> This commit adds a new helper function, __clk_is_ancestor().  It
>>> returns true if the second argument is a possible ancestor of the
>>> first one.  If a clock core is a possible ancestor of itself, it
>>> would make a loop when it were registered.  That should be detected
>>> as an error.
>>
>> This commit breaks lpc18xx boot in next right now. See
>> http://marc.info/?l=linux-arm-kernel&m=145608597106087&w=2
>>
>> The Clock Generation Unit (CGU) on lpc18xx allow for circular parents
>> in hardware. While it is obliviously not a good idea to configure the
>> clocks in that manner there is nothing that stops you either.
>>
>> Please take a look at the second figure on:
>> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks
>> All PLLs can feed clock into the dividers and the dividers can feed
>> clock into the PLLs.
>>
>> The reason why this is made possible in the CGU is because you can
>> then choose where to put your divider; either before the PLL or after.
>>
>
>
> Sorry for breaking your board.

No worries, that is why we have next so we can catch it before it hits
mainline :-)


> I am OK with reverting b58f75aa83fb.
>
>
>
> I guess your hardware could make clock looping for the best flexibility
> but you do not make clock looping in actual use cases.

That's right.


> Maybe, does it make sense to check the parent looping
> in clk_set_parent() or somewhere, not in clk_register()?

I think that would be a nice addition. While the CGU can certainly be
configured with loops it is indeed something that we should prevent
from happening.


regards,
Joachim Eastwood