clk: a collection of various clean-ups and improvements

PATCH Superseded Masahiro Yamada 3 participants 22 messages Dec 28, 2015 – Feb 22, 2016

Subsystems (2)

Memory technology devices (MTD) Common clk

lore

Share & tools

lore

Masahiro Yamada

author

2015-12-28 10:10 UTC #0 lore raw

v2 cover clk: a collection of various clean-ups and improvements

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(-)

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #1 lore raw

PATCH v2 1/16 clk: remove unused first argument of __clk_init()

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +2 -3

index 3fba5ac..897e5ae 100644
@@ -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;

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +5 -8

index 897e5ae..623838f 100644
@@ -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;

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #3 lore raw

PATCH v2 3/16 clk: rename __clk_init() into __clk_core_init()

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +3 -3

index 623838f..41179f3 100644
@@ -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;

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #4 lore raw

PATCH v2 4/16 clk: remove unnecessary !core->parents conditional

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +1 -4

index 41179f3..30c0bbc 100644
@@ -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);
 		/*

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +4 -4

index 30c0bbc..98745c3 100644
@@ -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

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +19 ~~-32~~

index 98745c3..b5be02a 100644
@@ -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]);

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #7 lore raw

PATCH v2 7/16 clk: simplify clk_core_get_parent_by_index()

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +6 -7

index b5be02a..ca7849a 100644
@@ -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 *

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +0 ~~-11~~

index ca7849a..3215b2b 100644
@@ -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);
 
 	/*

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #9 lore raw

PATCH v2 9/16 clk: replace pr_warn() with pr_err() for fatal cases

In reply to Masahiro Yamada

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +5 -5

index 3215b2b..e6e10f5 100644
@@ -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;

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +5 -7

index e6e10f5..4ad0a36 100644
@@ -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",

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #11 lore raw

PATCH v2 11/16 clk: simplify __clk_init_parent()

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +4 ~~-34~~

index 4ad0a36..03b87d8 100644
@@ -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,

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #12 lore raw

PATCH v2 12/16 clk: avoid circular clock topology

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +40 -0

index 03b87d8..a4f66fe 100644
@@ -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);
 
 	/*

Signature · 1 line

1.9.1

Joachim Eastwood

2016-02-21 21:39 UTC #13 lore raw

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

Hi everyone,

Quoted 12 lines · 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.)

Quoted 5 lines

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

Masahiro Yamada

author

2016-02-22 02:29 UTC #14 lore raw

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

Hi Joachim,

2016-02-22 6:39 GMT+09:00 Joachim Eastwood <[email protected]>:

Quoted 29 lines

Hi everyone,
Quoted 12 lines · 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.)
Quoted 5 lines
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.

Quoted 8 lines

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()?

Signature · 2 lines

Best Regards
Masahiro Yamada

Stephen Boyd

2016-02-22 23:19 UTC #15 lore raw

PATCH 1/1 Revert "clk: avoid circular clock topology"

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 <[email protected]>

Cc Masahiro Yamada <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

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

drivers/clk/clk.c +0 ~~-40~~

index 58ef3dab894a..51d673370d42 100644
@@ -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);
 
 	/*

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Joachim Eastwood

2016-02-22 23:32 UTC #16 lore raw

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

In reply to Masahiro Yamada

Quoted 38 lines · Masahiro Yamada

Hi Joachim,
2016-02-22 6:39 GMT+09:00 Joachim Eastwood <[email protected]>:
Quoted 34 lines
Hi everyone,
Quoted 12 lines · 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.)
Quoted 5 lines
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.

Quoted 6 lines

Quoted 3 lines
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 :-)

Quoted 6 lines

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.

Quoted 2 lines

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

Masahiro Yamada

author

2015-12-28 10:10 UTC #17 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +6 ~~-15~~

index a4f66fe..efd093a 100644
@@ -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

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #18 lore raw

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

In reply to 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 <[email protected]>

---

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

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

drivers/clk/clk.c +3 -0

index efd093a..151cbe8 100644
@@ -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

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #19 lore raw

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

In reply to 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 <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +2 ~~-16~~

index 151cbe8..5b9aec0 100644
@@ -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;
 }

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:10 UTC #20 lore raw

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

In reply to Masahiro Yamada

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

Signed-off-by Masahiro Yamada <[email protected]>

---

Changes in v2: None

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

drivers/clk/clk.c +1 -1

index 5b9aec0..e81301b 100644
@@ -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 */

Signature · 1 line

1.9.1

Masahiro Yamada

author

2015-12-28 10:25 UTC #21 lore raw

In reply to Masahiro Yamada

I sent this series to linux-mtd by mistake.

This should have been sent to linux-clk, of course.

Sorry for noise.

2015-12-28 19:10 GMT+09:00 Masahiro Yamada <[email protected]>:

Quoted 23 lines

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()

Signature · 2 lines

Best Regards
Masahiro Yamada