[RESEND PATCH 0/3] clk: Fixup issues for clk_set_parent

[RESEND PATCH 0/3] clk: Fixup issues for clk_set_parent

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

Issues with racing for fetching spinlocks is present in the
clk_set_parent API. This patchset is trying to fixup these issues.

In short, while updating the clock tree toplogy the spinlock must be held
to prevent enable_count from being messed up.

Patch 1 and 2 prepares for patch 3, which is where the real issue are
resolved.

Ulf Hansson (3):
  clk: Remove _clk_reparent from API and restructure code
  clk: Improve errorhandling for clk_set_parent
  clk: Fixup locking issues for clk_set_parent

 drivers/clk/clk.c            |  192 +++++++++++++++++++++++++++---------------
 include/linux/clk-provider.h |    1 -
 2 files changed, 125 insertions(+), 68 deletions(-)

-- 
1.7.10

[RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

There shall be no reason for keeping this API available for clock
providers. So we remove it from the API and restrcuture the code so
for example the COMMON_CLK_DEBUG part is separated.

This patch will also make it possible to hold the spinlock over the
actual update of the clock tree topology, which could not be done
before when both debugfs updates and clock rate updates was done
within the same function.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/clk/clk.c            |   82 ++++++++++++++++++++++++------------------
 include/linux/clk-provider.h |    1 -
 2 files changed, 48 insertions(+), 35 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 593a2e4..2e10cc1 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -284,6 +284,39 @@ out:
 }
 
 /**
+ * clk_debug_reparent - reparent clk node in the debugfs clk tree
+ * @clk: the clk being reparented
+ * @new_parent: the new clk parent, may be NULL
+ *
+ * Rename clk entry in the debugfs clk tree if debugfs has been
+ * initialized.  Otherwise it bails out early since the debugfs clk tree
+ * will be created lazily by clk_debug_init as part of a late_initcall.
+ *
+ * Caller must hold prepare_lock.
+ */
+static void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
+{
+	struct dentry *d;
+	struct dentry *new_parent_d;
+
+	if (!inited)
+		return;
+
+	if (new_parent)
+		new_parent_d = new_parent->dentry;
+	else
+		new_parent_d = orphandir;
+
+	d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
+			new_parent_d, clk->name);
+	if (d)
+		clk->dentry = d;
+	else
+		pr_debug("%s: failed to rename debugfs entry for %s\n",
+				__func__, clk->name);
+}
+
+/**
  * clk_debug_init - lazily create the debugfs clk tree visualization
  *
  * clks are often initialized very early during boot before memory can
@@ -338,6 +371,9 @@ static int __init clk_debug_init(void)
 late_initcall(clk_debug_init);
 #else
 static inline int clk_debug_register(struct clk *clk) { return 0; }
+static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
+{
+}
 #endif
 
 /* caller must hold prepare_lock */
@@ -1179,16 +1215,8 @@ out:
 	return ret;
 }
 
-void __clk_reparent(struct clk *clk, struct clk *new_parent)
+static void clk_reparent(struct clk *clk, struct clk *new_parent)
 {
-#ifdef CONFIG_COMMON_CLK_DEBUG
-	struct dentry *d;
-	struct dentry *new_parent_d;
-#endif
-
-	if (!clk || !new_parent)
-		return;
-
 	hlist_del(&clk->child_node);
 
 	if (new_parent)
@@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 	else
 		hlist_add_head(&clk->child_node, &clk_orphan_list);
 
-#ifdef CONFIG_COMMON_CLK_DEBUG
-	if (!inited)
-		goto out;
-
-	if (new_parent)
-		new_parent_d = new_parent->dentry;
-	else
-		new_parent_d = orphandir;
-
-	d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
-			new_parent_d, clk->name);
-	if (d)
-		clk->dentry = d;
-	else
-		pr_debug("%s: failed to rename debugfs entry for %s\n",
-				__func__, clk->name);
-out:
-#endif
-
 	clk->parent = new_parent;
-
-	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 }
 
 static int __clk_set_parent(struct clk *clk, struct clk *parent)
@@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	}
 
 	/* propagate rate recalculation downstream */
-	__clk_reparent(clk, parent);
+	clk_reparent(clk, parent);
+	clk_debug_reparent(clk, parent);
+	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 
 out:
 	mutex_unlock(&prepare_lock);
@@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk)
 	hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) {
 		if (orphan->ops->get_parent) {
 			i = orphan->ops->get_parent(orphan->hw);
-			if (!strcmp(clk->name, orphan->parent_names[i]))
-				__clk_reparent(orphan, clk);
+			if (!strcmp(clk->name, orphan->parent_names[i])) {
+				clk_reparent(orphan, clk);
+				clk_debug_reparent(orphan, clk);
+				__clk_recalc_rates(orphan, POST_RATE_CHANGE);
+			}
 			continue;
 		}
 
 		for (i = 0; i < orphan->num_parents; i++)
 			if (!strcmp(clk->name, orphan->parent_names[i])) {
-				__clk_reparent(orphan, clk);
+				clk_reparent(orphan, clk);
+				clk_debug_reparent(orphan, clk);
+				__clk_recalc_rates(orphan, POST_RATE_CHANGE);
 				break;
 			}
 	 }
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 4989b8a..87a7c2c 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name);
  */
 int __clk_prepare(struct clk *clk);
 void __clk_unprepare(struct clk *clk);
-void __clk_reparent(struct clk *clk, struct clk *new_parent);
 unsigned long __clk_round_rate(struct clk *clk, unsigned long rate);
 
 struct of_device_id;
-- 
1.7.10

[RESEND PATCH 2/3] clk: Improve errorhandling for clk_set_parent

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

By verifying all the needed static information before doing the clk
notifications, we minimize number of unwanted rollback notifications
with ABORT_RATE_CHANGE message to occur.

Additionally make sure the parent are valid pointer before using it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/clk/clk.c |   48 +++++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2e10cc1..a61cf6d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1227,15 +1227,10 @@ static void clk_reparent(struct clk *clk, struct clk *new_parent)
 	clk->parent = new_parent;
 }
 
-static int __clk_set_parent(struct clk *clk, struct clk *parent)
+static u8 clk_fetch_parent_index(struct clk *clk, struct clk *parent)
 {
-	struct clk *old_parent;
-	unsigned long flags;
-	int ret = -EINVAL;
 	u8 i;
 
-	old_parent = clk->parent;
-
 	if (!clk->parents)
 		clk->parents = kzalloc((sizeof(struct clk*) * clk->num_parents),
 								GFP_KERNEL);
@@ -1255,11 +1250,14 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 		}
 	}
 
-	if (i == clk->num_parents) {
-		pr_debug("%s: clock %s is not a possible parent of clock %s\n",
-				__func__, parent->name, clk->name);
-		goto out;
-	}
+	return i;
+}
+
+static int __clk_set_parent(struct clk *clk, struct clk *parent, u8 p_index)
+{
+	unsigned long flags;
+	int ret;
+	struct clk *old_parent = clk->parent;
 
 	/* migrate prepare and enable */
 	if (clk->prepare_count)
@@ -1272,7 +1270,7 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	spin_unlock_irqrestore(&enable_lock, flags);
 
 	/* change clock input source */
-	ret = clk->ops->set_parent(clk->hw, i);
+	ret = clk->ops->set_parent(clk->hw, p_index);
 
 	/* clean up old prepare and enable */
 	spin_lock_irqsave(&enable_lock, flags);
@@ -1283,7 +1281,6 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	if (clk->prepare_count)
 		__clk_unprepare(old_parent);
 
-out:
 	return ret;
 }
 
@@ -1302,8 +1299,9 @@ out:
 int clk_set_parent(struct clk *clk, struct clk *parent)
 {
 	int ret = 0;
+	u8 p_index;
 
-	if (!clk || !clk->ops)
+	if (!clk || !clk->ops || !parent)
 		return -EINVAL;
 
 	if (!clk->ops->set_parent)
@@ -1315,6 +1313,21 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	if (clk->parent == parent)
 		goto out;
 
+	/* check that we are allowed to re-parent if the clock is in use */
+	if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	/* try finding the new parent index */
+	p_index = clk_fetch_parent_index(clk, parent);
+	if (p_index == clk->num_parents) {
+		pr_debug("%s: clock %s is not a possible parent of clock %s\n",
+				__func__, parent->name, clk->name);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* propagate PRE_RATE_CHANGE notifications */
 	if (clk->notifier_count)
 		ret = __clk_speculate_rates(clk, parent->rate);
@@ -1323,11 +1336,8 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	if (ret == NOTIFY_STOP)
 		goto out;
 
-	/* only re-parent if the clock is not in use */
-	if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count)
-		ret = -EBUSY;
-	else
-		ret = __clk_set_parent(clk, parent);
+	/* do the re-parent */
+	ret = __clk_set_parent(clk, parent, p_index);
 
 	/* propagate ABORT_RATE_CHANGE if .set_parent failed */
 	if (ret) {
-- 
1.7.10

[RESEND PATCH 3/3] clk: Fixup locking issues for clk_set_parent

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

Updating the clock tree topology must be protected with the spinlock
when doing clk_set_parent, otherwise we can not handle the migration
of the enable_count in a safe manner.

While issuing the .set_parent callback to make the clk-hw perform the
switch to the new parent, we can not hold the spinlock since it is must
be allowed to be slow path. This complicates error handling, but is still
possible to achieve.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/clk/clk.c |   68 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a61cf6d..0c19420 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1258,30 +1258,69 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent, u8 p_index)
 	unsigned long flags;
 	int ret;
 	struct clk *old_parent = clk->parent;
+	bool migrated_enable = false;
 
-	/* migrate prepare and enable */
+	/* migrate prepare */
 	if (clk->prepare_count)
 		__clk_prepare(parent);
 
-	/* FIXME replace with clk_is_enabled(clk) someday */
 	spin_lock_irqsave(&enable_lock, flags);
-	if (clk->enable_count)
+
+	/* migrate enable */
+	if (clk->enable_count) {
 		__clk_enable(parent);
+		migrated_enable = true;
+	}
+
+	/* update the clk tree topology */
+	clk_reparent(clk, parent);
+
 	spin_unlock_irqrestore(&enable_lock, flags);
 
 	/* change clock input source */
 	ret = clk->ops->set_parent(clk->hw, p_index);
+	if (ret) {
+		/*
+		 * The error handling is tricky due to that we need to release
+		 * the spinlock while issuing the .set_parent callback. This
+		 * means the new parent might have been enabled/disabled in
+		 * between, which must be considered when doing rollback.
+		 */
+		spin_lock_irqsave(&enable_lock, flags);
 
-	/* clean up old prepare and enable */
-	spin_lock_irqsave(&enable_lock, flags);
-	if (clk->enable_count)
+		clk_reparent(clk, old_parent);
+
+		if (migrated_enable && clk->enable_count) {
+			__clk_disable(parent);
+		} else if (migrated_enable && (clk->enable_count == 0)) {
+			__clk_disable(old_parent);
+		} else if (!migrated_enable && clk->enable_count) {
+			__clk_disable(parent);
+			__clk_enable(old_parent);
+		}
+
+		spin_unlock_irqrestore(&enable_lock, flags);
+
+		if (clk->prepare_count)
+			__clk_unprepare(parent);
+
+		return ret;
+	}
+
+	/* clean up enable for old parent if migration was done */
+	if (migrated_enable) {
+		spin_lock_irqsave(&enable_lock, flags);
 		__clk_disable(old_parent);
-	spin_unlock_irqrestore(&enable_lock, flags);
+		spin_unlock_irqrestore(&enable_lock, flags);
+	}
 
+	/* clean up prepare for old parent if migration was done */
 	if (clk->prepare_count)
 		__clk_unprepare(old_parent);
 
-	return ret;
+	/* update debugfs with new clk tree topology */
+	clk_debug_reparent(clk, parent);
+	return 0;
 }
 
 /**
@@ -1339,16 +1378,11 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	/* do the re-parent */
 	ret = __clk_set_parent(clk, parent, p_index);
 
-	/* propagate ABORT_RATE_CHANGE if .set_parent failed */
-	if (ret) {
+	/* propagate rate recalculation accordingly */
+	if (ret)
 		__clk_recalc_rates(clk, ABORT_RATE_CHANGE);
-		goto out;
-	}
-
-	/* propagate rate recalculation downstream */
-	clk_reparent(clk, parent);
-	clk_debug_reparent(clk, parent);
-	__clk_recalc_rates(clk, POST_RATE_CHANGE);
+	else
+		__clk_recalc_rates(clk, POST_RATE_CHANGE);
 
 out:
 	mutex_unlock(&prepare_lock);
-- 
1.7.10

[RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code

[Mike Turquette]

Quoting Ulf Hansson (2013-03-12 12:20:48)
> From: Ulf Hansson <ulf.hansson@linaro.org>
> 
> There shall be no reason for keeping this API available for clock
> providers. So we remove it from the API and restrcuture the code so
> for example the COMMON_CLK_DEBUG part is separated.
> 

Hi Ulf,

There is one reason to keep this api.  OMAP currently uses it to change
the parent of a PLL during .set_rate.  This is kind of a legacy
mechanism however, and the reentrancy series I posted actually takes
care of this one corner case:
http://article.gmane.org/gmane.linux.kernel/1448449

Let me see how the OMAP folks feel about taking that patch in, which
eliminates the only external user of the API.  +Rajendra & Benoit

Thanks,
Mike

> This patch will also make it possible to hold the spinlock over the
> actual update of the clock tree topology, which could not be done
> before when both debugfs updates and clock rate updates was done
> within the same function.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/clk/clk.c            |   82 ++++++++++++++++++++++++------------------
>  include/linux/clk-provider.h |    1 -
>  2 files changed, 48 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 593a2e4..2e10cc1 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -284,6 +284,39 @@ out:
>  }
>  
>  /**
> + * clk_debug_reparent - reparent clk node in the debugfs clk tree
> + * @clk: the clk being reparented
> + * @new_parent: the new clk parent, may be NULL
> + *
> + * Rename clk entry in the debugfs clk tree if debugfs has been
> + * initialized.  Otherwise it bails out early since the debugfs clk tree
> + * will be created lazily by clk_debug_init as part of a late_initcall.
> + *
> + * Caller must hold prepare_lock.
> + */
> +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
> +{
> +       struct dentry *d;
> +       struct dentry *new_parent_d;
> +
> +       if (!inited)
> +               return;
> +
> +       if (new_parent)
> +               new_parent_d = new_parent->dentry;
> +       else
> +               new_parent_d = orphandir;
> +
> +       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
> +                       new_parent_d, clk->name);
> +       if (d)
> +               clk->dentry = d;
> +       else
> +               pr_debug("%s: failed to rename debugfs entry for %s\n",
> +                               __func__, clk->name);
> +}
> +
> +/**
>   * clk_debug_init - lazily create the debugfs clk tree visualization
>   *
>   * clks are often initialized very early during boot before memory can
> @@ -338,6 +371,9 @@ static int __init clk_debug_init(void)
>  late_initcall(clk_debug_init);
>  #else
>  static inline int clk_debug_register(struct clk *clk) { return 0; }
> +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
> +{
> +}
>  #endif
>  
>  /* caller must hold prepare_lock */
> @@ -1179,16 +1215,8 @@ out:
>         return ret;
>  }
>  
> -void __clk_reparent(struct clk *clk, struct clk *new_parent)
> +static void clk_reparent(struct clk *clk, struct clk *new_parent)
>  {
> -#ifdef CONFIG_COMMON_CLK_DEBUG
> -       struct dentry *d;
> -       struct dentry *new_parent_d;
> -#endif
> -
> -       if (!clk || !new_parent)
> -               return;
> -
>         hlist_del(&clk->child_node);
>  
>         if (new_parent)
> @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>         else
>                 hlist_add_head(&clk->child_node, &clk_orphan_list);
>  
> -#ifdef CONFIG_COMMON_CLK_DEBUG
> -       if (!inited)
> -               goto out;
> -
> -       if (new_parent)
> -               new_parent_d = new_parent->dentry;
> -       else
> -               new_parent_d = orphandir;
> -
> -       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
> -                       new_parent_d, clk->name);
> -       if (d)
> -               clk->dentry = d;
> -       else
> -               pr_debug("%s: failed to rename debugfs entry for %s\n",
> -                               __func__, clk->name);
> -out:
> -#endif
> -
>         clk->parent = new_parent;
> -
> -       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>  }
>  
>  static int __clk_set_parent(struct clk *clk, struct clk *parent)
> @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>         }
>  
>         /* propagate rate recalculation downstream */
> -       __clk_reparent(clk, parent);
> +       clk_reparent(clk, parent);
> +       clk_debug_reparent(clk, parent);
> +       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>  
>  out:
>         mutex_unlock(&prepare_lock);
> @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk)
>         hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) {
>                 if (orphan->ops->get_parent) {
>                         i = orphan->ops->get_parent(orphan->hw);
> -                       if (!strcmp(clk->name, orphan->parent_names[i]))
> -                               __clk_reparent(orphan, clk);
> +                       if (!strcmp(clk->name, orphan->parent_names[i])) {
> +                               clk_reparent(orphan, clk);
> +                               clk_debug_reparent(orphan, clk);
> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
> +                       }
>                         continue;
>                 }
>  
>                 for (i = 0; i < orphan->num_parents; i++)
>                         if (!strcmp(clk->name, orphan->parent_names[i])) {
> -                               __clk_reparent(orphan, clk);
> +                               clk_reparent(orphan, clk);
> +                               clk_debug_reparent(orphan, clk);
> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
>                                 break;
>                         }
>          }
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 4989b8a..87a7c2c 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name);
>   */
>  int __clk_prepare(struct clk *clk);
>  void __clk_unprepare(struct clk *clk);
> -void __clk_reparent(struct clk *clk, struct clk *new_parent);
>  unsigned long __clk_round_rate(struct clk *clk, unsigned long rate);
>  
>  struct of_device_id;
> -- 
> 1.7.10

[RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code

[Ulf Hansson]

On 19 March 2013 22:20, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Ulf Hansson (2013-03-12 12:20:48)
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> There shall be no reason for keeping this API available for clock
>> providers. So we remove it from the API and restrcuture the code so
>> for example the COMMON_CLK_DEBUG part is separated.
>>
>
> Hi Ulf,
>
> There is one reason to keep this api.  OMAP currently uses it to change
> the parent of a PLL during .set_rate.  This is kind of a legacy
> mechanism however, and the reentrancy series I posted actually takes
> care of this one corner case:
> http://article.gmane.org/gmane.linux.kernel/1448449

Hi Mike,

It was a while ago I created this patch, so I guess this has beed
merged without me noticing, sorry.

>
> Let me see how the OMAP folks feel about taking that patch in, which
> eliminates the only external user of the API.  +Rajendra & Benoit
>

I have no problem rebasing the patch and keep the API, if that is the
easiest way forward.
Although, I guess in the end you want to remove these kind of internal
clk API functions. So, if we get ack from Rajendra & Benoit, it is
better to remove the API right?

Kind regards
Ulf Hansson

> Thanks,
> Mike
>
>> This patch will also make it possible to hold the spinlock over the
>> actual update of the clock tree topology, which could not be done
>> before when both debugfs updates and clock rate updates was done
>> within the same function.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/clk/clk.c            |   82 ++++++++++++++++++++++++------------------
>>  include/linux/clk-provider.h |    1 -
>>  2 files changed, 48 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 593a2e4..2e10cc1 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -284,6 +284,39 @@ out:
>>  }
>>
>>  /**
>> + * clk_debug_reparent - reparent clk node in the debugfs clk tree
>> + * @clk: the clk being reparented
>> + * @new_parent: the new clk parent, may be NULL
>> + *
>> + * Rename clk entry in the debugfs clk tree if debugfs has been
>> + * initialized.  Otherwise it bails out early since the debugfs clk tree
>> + * will be created lazily by clk_debug_init as part of a late_initcall.
>> + *
>> + * Caller must hold prepare_lock.
>> + */
>> +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
>> +{
>> +       struct dentry *d;
>> +       struct dentry *new_parent_d;
>> +
>> +       if (!inited)
>> +               return;
>> +
>> +       if (new_parent)
>> +               new_parent_d = new_parent->dentry;
>> +       else
>> +               new_parent_d = orphandir;
>> +
>> +       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
>> +                       new_parent_d, clk->name);
>> +       if (d)
>> +               clk->dentry = d;
>> +       else
>> +               pr_debug("%s: failed to rename debugfs entry for %s\n",
>> +                               __func__, clk->name);
>> +}
>> +
>> +/**
>>   * clk_debug_init - lazily create the debugfs clk tree visualization
>>   *
>>   * clks are often initialized very early during boot before memory can
>> @@ -338,6 +371,9 @@ static int __init clk_debug_init(void)
>>  late_initcall(clk_debug_init);
>>  #else
>>  static inline int clk_debug_register(struct clk *clk) { return 0; }
>> +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
>> +{
>> +}
>>  #endif
>>
>>  /* caller must hold prepare_lock */
>> @@ -1179,16 +1215,8 @@ out:
>>         return ret;
>>  }
>>
>> -void __clk_reparent(struct clk *clk, struct clk *new_parent)
>> +static void clk_reparent(struct clk *clk, struct clk *new_parent)
>>  {
>> -#ifdef CONFIG_COMMON_CLK_DEBUG
>> -       struct dentry *d;
>> -       struct dentry *new_parent_d;
>> -#endif
>> -
>> -       if (!clk || !new_parent)
>> -               return;
>> -
>>         hlist_del(&clk->child_node);
>>
>>         if (new_parent)
>> @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>>         else
>>                 hlist_add_head(&clk->child_node, &clk_orphan_list);
>>
>> -#ifdef CONFIG_COMMON_CLK_DEBUG
>> -       if (!inited)
>> -               goto out;
>> -
>> -       if (new_parent)
>> -               new_parent_d = new_parent->dentry;
>> -       else
>> -               new_parent_d = orphandir;
>> -
>> -       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
>> -                       new_parent_d, clk->name);
>> -       if (d)
>> -               clk->dentry = d;
>> -       else
>> -               pr_debug("%s: failed to rename debugfs entry for %s\n",
>> -                               __func__, clk->name);
>> -out:
>> -#endif
>> -
>>         clk->parent = new_parent;
>> -
>> -       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>>  }
>>
>>  static int __clk_set_parent(struct clk *clk, struct clk *parent)
>> @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>>         }
>>
>>         /* propagate rate recalculation downstream */
>> -       __clk_reparent(clk, parent);
>> +       clk_reparent(clk, parent);
>> +       clk_debug_reparent(clk, parent);
>> +       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>>
>>  out:
>>         mutex_unlock(&prepare_lock);
>> @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk)
>>         hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) {
>>                 if (orphan->ops->get_parent) {
>>                         i = orphan->ops->get_parent(orphan->hw);
>> -                       if (!strcmp(clk->name, orphan->parent_names[i]))
>> -                               __clk_reparent(orphan, clk);
>> +                       if (!strcmp(clk->name, orphan->parent_names[i])) {
>> +                               clk_reparent(orphan, clk);
>> +                               clk_debug_reparent(orphan, clk);
>> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
>> +                       }
>>                         continue;
>>                 }
>>
>>                 for (i = 0; i < orphan->num_parents; i++)
>>                         if (!strcmp(clk->name, orphan->parent_names[i])) {
>> -                               __clk_reparent(orphan, clk);
>> +                               clk_reparent(orphan, clk);
>> +                               clk_debug_reparent(orphan, clk);
>> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
>>                                 break;
>>                         }
>>          }
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index 4989b8a..87a7c2c 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name);
>>   */
>>  int __clk_prepare(struct clk *clk);
>>  void __clk_unprepare(struct clk *clk);
>> -void __clk_reparent(struct clk *clk, struct clk *new_parent);
>>  unsigned long __clk_round_rate(struct clk *clk, unsigned long rate);
>>
>>  struct of_device_id;
>> --
>> 1.7.10

[RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code

[Rajendra Nayak]

On Wednesday 20 March 2013 03:15 PM, Ulf Hansson wrote:
> On 19 March 2013 22:20, Mike Turquette <mturquette@linaro.org> wrote:
>> Quoting Ulf Hansson (2013-03-12 12:20:48)
>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> There shall be no reason for keeping this API available for clock
>>> providers. So we remove it from the API and restrcuture the code so
>>> for example the COMMON_CLK_DEBUG part is separated.
>>>
>>
>> Hi Ulf,
>>
>> There is one reason to keep this api.  OMAP currently uses it to change
>> the parent of a PLL during .set_rate.  This is kind of a legacy
>> mechanism however, and the reentrancy series I posted actually takes
>> care of this one corner case:
>> http://article.gmane.org/gmane.linux.kernel/1448449
> 
> Hi Mike,
> 
> It was a while ago I created this patch, so I guess this has beed
> merged without me noticing, sorry.
> 
>>
>> Let me see how the OMAP folks feel about taking that patch in, which
>> eliminates the only external user of the API.  +Rajendra & Benoit
>>
> 
> I have no problem rebasing the patch and keep the API, if that is the
> easiest way forward.

Ulf, It would be good if you could keep the api for now. I seemed to have
missed out on Mikes patch getting rid of the the api from OMAP PLL .set_rate.
I will take a stab at that to get rid of that api along with the other OMAP
only things that exist today as part of Common clk.

regards,
Rajendra

> Although, I guess in the end you want to remove these kind of internal
> clk API functions. So, if we get ack from Rajendra & Benoit, it is
> better to remove the API right?
> 
> Kind regards
> Ulf Hansson
> 
>> Thanks,
>> Mike
>>
>>> This patch will also make it possible to hold the spinlock over the
>>> actual update of the clock tree topology, which could not be done
>>> before when both debugfs updates and clock rate updates was done
>>> within the same function.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/clk/clk.c            |   82 ++++++++++++++++++++++++------------------
>>>  include/linux/clk-provider.h |    1 -
>>>  2 files changed, 48 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index 593a2e4..2e10cc1 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -284,6 +284,39 @@ out:
>>>  }
>>>
>>>  /**
>>> + * clk_debug_reparent - reparent clk node in the debugfs clk tree
>>> + * @clk: the clk being reparented
>>> + * @new_parent: the new clk parent, may be NULL
>>> + *
>>> + * Rename clk entry in the debugfs clk tree if debugfs has been
>>> + * initialized.  Otherwise it bails out early since the debugfs clk tree
>>> + * will be created lazily by clk_debug_init as part of a late_initcall.
>>> + *
>>> + * Caller must hold prepare_lock.
>>> + */
>>> +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
>>> +{
>>> +       struct dentry *d;
>>> +       struct dentry *new_parent_d;
>>> +
>>> +       if (!inited)
>>> +               return;
>>> +
>>> +       if (new_parent)
>>> +               new_parent_d = new_parent->dentry;
>>> +       else
>>> +               new_parent_d = orphandir;
>>> +
>>> +       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
>>> +                       new_parent_d, clk->name);
>>> +       if (d)
>>> +               clk->dentry = d;
>>> +       else
>>> +               pr_debug("%s: failed to rename debugfs entry for %s\n",
>>> +                               __func__, clk->name);
>>> +}
>>> +
>>> +/**
>>>   * clk_debug_init - lazily create the debugfs clk tree visualization
>>>   *
>>>   * clks are often initialized very early during boot before memory can
>>> @@ -338,6 +371,9 @@ static int __init clk_debug_init(void)
>>>  late_initcall(clk_debug_init);
>>>  #else
>>>  static inline int clk_debug_register(struct clk *clk) { return 0; }
>>> +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
>>> +{
>>> +}
>>>  #endif
>>>
>>>  /* caller must hold prepare_lock */
>>> @@ -1179,16 +1215,8 @@ out:
>>>         return ret;
>>>  }
>>>
>>> -void __clk_reparent(struct clk *clk, struct clk *new_parent)
>>> +static void clk_reparent(struct clk *clk, struct clk *new_parent)
>>>  {
>>> -#ifdef CONFIG_COMMON_CLK_DEBUG
>>> -       struct dentry *d;
>>> -       struct dentry *new_parent_d;
>>> -#endif
>>> -
>>> -       if (!clk || !new_parent)
>>> -               return;
>>> -
>>>         hlist_del(&clk->child_node);
>>>
>>>         if (new_parent)
>>> @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>>>         else
>>>                 hlist_add_head(&clk->child_node, &clk_orphan_list);
>>>
>>> -#ifdef CONFIG_COMMON_CLK_DEBUG
>>> -       if (!inited)
>>> -               goto out;
>>> -
>>> -       if (new_parent)
>>> -               new_parent_d = new_parent->dentry;
>>> -       else
>>> -               new_parent_d = orphandir;
>>> -
>>> -       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
>>> -                       new_parent_d, clk->name);
>>> -       if (d)
>>> -               clk->dentry = d;
>>> -       else
>>> -               pr_debug("%s: failed to rename debugfs entry for %s\n",
>>> -                               __func__, clk->name);
>>> -out:
>>> -#endif
>>> -
>>>         clk->parent = new_parent;
>>> -
>>> -       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>>>  }
>>>
>>>  static int __clk_set_parent(struct clk *clk, struct clk *parent)
>>> @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>>>         }
>>>
>>>         /* propagate rate recalculation downstream */
>>> -       __clk_reparent(clk, parent);
>>> +       clk_reparent(clk, parent);
>>> +       clk_debug_reparent(clk, parent);
>>> +       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>>>
>>>  out:
>>>         mutex_unlock(&prepare_lock);
>>> @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk)
>>>         hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) {
>>>                 if (orphan->ops->get_parent) {
>>>                         i = orphan->ops->get_parent(orphan->hw);
>>> -                       if (!strcmp(clk->name, orphan->parent_names[i]))
>>> -                               __clk_reparent(orphan, clk);
>>> +                       if (!strcmp(clk->name, orphan->parent_names[i])) {
>>> +                               clk_reparent(orphan, clk);
>>> +                               clk_debug_reparent(orphan, clk);
>>> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
>>> +                       }
>>>                         continue;
>>>                 }
>>>
>>>                 for (i = 0; i < orphan->num_parents; i++)
>>>                         if (!strcmp(clk->name, orphan->parent_names[i])) {
>>> -                               __clk_reparent(orphan, clk);
>>> +                               clk_reparent(orphan, clk);
>>> +                               clk_debug_reparent(orphan, clk);
>>> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
>>>                                 break;
>>>                         }
>>>          }
>>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>>> index 4989b8a..87a7c2c 100644
>>> --- a/include/linux/clk-provider.h
>>> +++ b/include/linux/clk-provider.h
>>> @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name);
>>>   */
>>>  int __clk_prepare(struct clk *clk);
>>>  void __clk_unprepare(struct clk *clk);
>>> -void __clk_reparent(struct clk *clk, struct clk *new_parent);
>>>  unsigned long __clk_round_rate(struct clk *clk, unsigned long rate);
>>>
>>>  struct of_device_id;
>>> --
>>> 1.7.10

[RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code

[Mike Turquette]

Quoting Rajendra Nayak (2013-03-20 03:13:36)
> On Wednesday 20 March 2013 03:15 PM, Ulf Hansson wrote:
> > On 19 March 2013 22:20, Mike Turquette <mturquette@linaro.org> wrote:
> >> Quoting Ulf Hansson (2013-03-12 12:20:48)
> >>> From: Ulf Hansson <ulf.hansson@linaro.org>
> >>>
> >>> There shall be no reason for keeping this API available for clock
> >>> providers. So we remove it from the API and restrcuture the code so
> >>> for example the COMMON_CLK_DEBUG part is separated.
> >>>
> >>
> >> Hi Ulf,
> >>
> >> There is one reason to keep this api.  OMAP currently uses it to change
> >> the parent of a PLL during .set_rate.  This is kind of a legacy
> >> mechanism however, and the reentrancy series I posted actually takes
> >> care of this one corner case:
> >> http://article.gmane.org/gmane.linux.kernel/1448449
> > 
> > Hi Mike,
> > 
> > It was a while ago I created this patch, so I guess this has beed
> > merged without me noticing, sorry.
> > 
> >>
> >> Let me see how the OMAP folks feel about taking that patch in, which
> >> eliminates the only external user of the API.  +Rajendra & Benoit
> >>
> > 
> > I have no problem rebasing the patch and keep the API, if that is the
> > easiest way forward.
> 
> Ulf, It would be good if you could keep the api for now. I seemed to have
> missed out on Mikes patch getting rid of the the api from OMAP PLL .set_rate.
> I will take a stab at that to get rid of that api along with the other OMAP
> only things that exist today as part of Common clk.
> 

Rajendra,

Yeah, the patch I linked to above was actually just "example code"
showing how to reentrantly call clk_set_parent from within a .set_rate
callback.  It's no suprise you missed it since the patch $SUBJECT starts
with "HACK:" ;-)

The trick is to recognize that OMAP's PLLs are mux clocks and have them
properly support clk_set_parent.  This would have made my life easier in
the bad days of DPLL cascading back in 2011.

> regards,
> Rajendra
> 
> > Although, I guess in the end you want to remove these kind of internal
> > clk API functions. So, if we get ack from Rajendra & Benoit, it is
> > better to remove the API right?
> > 

Ulf,

Correct, as we discussed at LCA I do want to get rid of all of these
internal accessor functions.

Thanks,
Mike

> > Kind regards
> > Ulf Hansson
> > 
> >> Thanks,
> >> Mike
> >>
> >>> This patch will also make it possible to hold the spinlock over the
> >>> actual update of the clock tree topology, which could not be done
> >>> before when both debugfs updates and clock rate updates was done
> >>> within the same function.
> >>>
> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>> ---
> >>>  drivers/clk/clk.c            |   82 ++++++++++++++++++++++++------------------
> >>>  include/linux/clk-provider.h |    1 -
> >>>  2 files changed, 48 insertions(+), 35 deletions(-)
> >>>
> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >>> index 593a2e4..2e10cc1 100644
> >>> --- a/drivers/clk/clk.c
> >>> +++ b/drivers/clk/clk.c
> >>> @@ -284,6 +284,39 @@ out:
> >>>  }
> >>>
> >>>  /**
> >>> + * clk_debug_reparent - reparent clk node in the debugfs clk tree
> >>> + * @clk: the clk being reparented
> >>> + * @new_parent: the new clk parent, may be NULL
> >>> + *
> >>> + * Rename clk entry in the debugfs clk tree if debugfs has been
> >>> + * initialized.  Otherwise it bails out early since the debugfs clk tree
> >>> + * will be created lazily by clk_debug_init as part of a late_initcall.
> >>> + *
> >>> + * Caller must hold prepare_lock.
> >>> + */
> >>> +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
> >>> +{
> >>> +       struct dentry *d;
> >>> +       struct dentry *new_parent_d;
> >>> +
> >>> +       if (!inited)
> >>> +               return;
> >>> +
> >>> +       if (new_parent)
> >>> +               new_parent_d = new_parent->dentry;
> >>> +       else
> >>> +               new_parent_d = orphandir;
> >>> +
> >>> +       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
> >>> +                       new_parent_d, clk->name);
> >>> +       if (d)
> >>> +               clk->dentry = d;
> >>> +       else
> >>> +               pr_debug("%s: failed to rename debugfs entry for %s\n",
> >>> +                               __func__, clk->name);
> >>> +}
> >>> +
> >>> +/**
> >>>   * clk_debug_init - lazily create the debugfs clk tree visualization
> >>>   *
> >>>   * clks are often initialized very early during boot before memory can
> >>> @@ -338,6 +371,9 @@ static int __init clk_debug_init(void)
> >>>  late_initcall(clk_debug_init);
> >>>  #else
> >>>  static inline int clk_debug_register(struct clk *clk) { return 0; }
> >>> +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
> >>> +{
> >>> +}
> >>>  #endif
> >>>
> >>>  /* caller must hold prepare_lock */
> >>> @@ -1179,16 +1215,8 @@ out:
> >>>         return ret;
> >>>  }
> >>>
> >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent)
> >>> +static void clk_reparent(struct clk *clk, struct clk *new_parent)
> >>>  {
> >>> -#ifdef CONFIG_COMMON_CLK_DEBUG
> >>> -       struct dentry *d;
> >>> -       struct dentry *new_parent_d;
> >>> -#endif
> >>> -
> >>> -       if (!clk || !new_parent)
> >>> -               return;
> >>> -
> >>>         hlist_del(&clk->child_node);
> >>>
> >>>         if (new_parent)
> >>> @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
> >>>         else
> >>>                 hlist_add_head(&clk->child_node, &clk_orphan_list);
> >>>
> >>> -#ifdef CONFIG_COMMON_CLK_DEBUG
> >>> -       if (!inited)
> >>> -               goto out;
> >>> -
> >>> -       if (new_parent)
> >>> -               new_parent_d = new_parent->dentry;
> >>> -       else
> >>> -               new_parent_d = orphandir;
> >>> -
> >>> -       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
> >>> -                       new_parent_d, clk->name);
> >>> -       if (d)
> >>> -               clk->dentry = d;
> >>> -       else
> >>> -               pr_debug("%s: failed to rename debugfs entry for %s\n",
> >>> -                               __func__, clk->name);
> >>> -out:
> >>> -#endif
> >>> -
> >>>         clk->parent = new_parent;
> >>> -
> >>> -       __clk_recalc_rates(clk, POST_RATE_CHANGE);
> >>>  }
> >>>
> >>>  static int __clk_set_parent(struct clk *clk, struct clk *parent)
> >>> @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
> >>>         }
> >>>
> >>>         /* propagate rate recalculation downstream */
> >>> -       __clk_reparent(clk, parent);
> >>> +       clk_reparent(clk, parent);
> >>> +       clk_debug_reparent(clk, parent);
> >>> +       __clk_recalc_rates(clk, POST_RATE_CHANGE);
> >>>
> >>>  out:
> >>>         mutex_unlock(&prepare_lock);
> >>> @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk)
> >>>         hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) {
> >>>                 if (orphan->ops->get_parent) {
> >>>                         i = orphan->ops->get_parent(orphan->hw);
> >>> -                       if (!strcmp(clk->name, orphan->parent_names[i]))
> >>> -                               __clk_reparent(orphan, clk);
> >>> +                       if (!strcmp(clk->name, orphan->parent_names[i])) {
> >>> +                               clk_reparent(orphan, clk);
> >>> +                               clk_debug_reparent(orphan, clk);
> >>> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
> >>> +                       }
> >>>                         continue;
> >>>                 }
> >>>
> >>>                 for (i = 0; i < orphan->num_parents; i++)
> >>>                         if (!strcmp(clk->name, orphan->parent_names[i])) {
> >>> -                               __clk_reparent(orphan, clk);
> >>> +                               clk_reparent(orphan, clk);
> >>> +                               clk_debug_reparent(orphan, clk);
> >>> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
> >>>                                 break;
> >>>                         }
> >>>          }
> >>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> >>> index 4989b8a..87a7c2c 100644
> >>> --- a/include/linux/clk-provider.h
> >>> +++ b/include/linux/clk-provider.h
> >>> @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name);
> >>>   */
> >>>  int __clk_prepare(struct clk *clk);
> >>>  void __clk_unprepare(struct clk *clk);
> >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent);
> >>>  unsigned long __clk_round_rate(struct clk *clk, unsigned long rate);
> >>>
> >>>  struct of_device_id;
> >>> --
> >>> 1.7.10

[RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code

[Ulf Hansson]

On 20 March 2013 16:03, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Rajendra Nayak (2013-03-20 03:13:36)
>> On Wednesday 20 March 2013 03:15 PM, Ulf Hansson wrote:
>> > On 19 March 2013 22:20, Mike Turquette <mturquette@linaro.org> wrote:
>> >> Quoting Ulf Hansson (2013-03-12 12:20:48)
>> >>> From: Ulf Hansson <ulf.hansson@linaro.org>
>> >>>
>> >>> There shall be no reason for keeping this API available for clock
>> >>> providers. So we remove it from the API and restrcuture the code so
>> >>> for example the COMMON_CLK_DEBUG part is separated.
>> >>>
>> >>
>> >> Hi Ulf,
>> >>
>> >> There is one reason to keep this api.  OMAP currently uses it to change
>> >> the parent of a PLL during .set_rate.  This is kind of a legacy
>> >> mechanism however, and the reentrancy series I posted actually takes
>> >> care of this one corner case:
>> >> http://article.gmane.org/gmane.linux.kernel/1448449
>> >
>> > Hi Mike,
>> >
>> > It was a while ago I created this patch, so I guess this has beed
>> > merged without me noticing, sorry.
>> >
>> >>
>> >> Let me see how the OMAP folks feel about taking that patch in, which
>> >> eliminates the only external user of the API.  +Rajendra & Benoit
>> >>
>> >
>> > I have no problem rebasing the patch and keep the API, if that is the
>> > easiest way forward.
>>
>> Ulf, It would be good if you could keep the api for now. I seemed to have
>> missed out on Mikes patch getting rid of the the api from OMAP PLL .set_rate.
>> I will take a stab at that to get rid of that api along with the other OMAP
>> only things that exist today as part of Common clk.
>>
>
> Rajendra,
>
> Yeah, the patch I linked to above was actually just "example code"
> showing how to reentrantly call clk_set_parent from within a .set_rate
> callback.  It's no suprise you missed it since the patch $SUBJECT starts
> with "HACK:" ;-)
>
> The trick is to recognize that OMAP's PLLs are mux clocks and have them
> properly support clk_set_parent.  This would have made my life easier in
> the bad days of DPLL cascading back in 2011.
>
>> regards,
>> Rajendra
>>
>> > Although, I guess in the end you want to remove these kind of internal
>> > clk API functions. So, if we get ack from Rajendra & Benoit, it is
>> > better to remove the API right?
>> >
>
> Ulf,
>
> Correct, as we discussed at LCA I do want to get rid of all of these
> internal accessor functions.
>
> Thanks,
> Mike
>

Thanks for your responses. I will send a new version which keeps the
API for now.

Kind regards
Uffe

>> > Kind regards
>> > Ulf Hansson
>> >
>> >> Thanks,
>> >> Mike
>> >>
>> >>> This patch will also make it possible to hold the spinlock over the
>> >>> actual update of the clock tree topology, which could not be done
>> >>> before when both debugfs updates and clock rate updates was done
>> >>> within the same function.
>> >>>
>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >>> ---
>> >>>  drivers/clk/clk.c            |   82 ++++++++++++++++++++++++------------------
>> >>>  include/linux/clk-provider.h |    1 -
>> >>>  2 files changed, 48 insertions(+), 35 deletions(-)
>> >>>
>> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> >>> index 593a2e4..2e10cc1 100644
>> >>> --- a/drivers/clk/clk.c
>> >>> +++ b/drivers/clk/clk.c
>> >>> @@ -284,6 +284,39 @@ out:
>> >>>  }
>> >>>
>> >>>  /**
>> >>> + * clk_debug_reparent - reparent clk node in the debugfs clk tree
>> >>> + * @clk: the clk being reparented
>> >>> + * @new_parent: the new clk parent, may be NULL
>> >>> + *
>> >>> + * Rename clk entry in the debugfs clk tree if debugfs has been
>> >>> + * initialized.  Otherwise it bails out early since the debugfs clk tree
>> >>> + * will be created lazily by clk_debug_init as part of a late_initcall.
>> >>> + *
>> >>> + * Caller must hold prepare_lock.
>> >>> + */
>> >>> +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
>> >>> +{
>> >>> +       struct dentry *d;
>> >>> +       struct dentry *new_parent_d;
>> >>> +
>> >>> +       if (!inited)
>> >>> +               return;
>> >>> +
>> >>> +       if (new_parent)
>> >>> +               new_parent_d = new_parent->dentry;
>> >>> +       else
>> >>> +               new_parent_d = orphandir;
>> >>> +
>> >>> +       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
>> >>> +                       new_parent_d, clk->name);
>> >>> +       if (d)
>> >>> +               clk->dentry = d;
>> >>> +       else
>> >>> +               pr_debug("%s: failed to rename debugfs entry for %s\n",
>> >>> +                               __func__, clk->name);
>> >>> +}
>> >>> +
>> >>> +/**
>> >>>   * clk_debug_init - lazily create the debugfs clk tree visualization
>> >>>   *
>> >>>   * clks are often initialized very early during boot before memory can
>> >>> @@ -338,6 +371,9 @@ static int __init clk_debug_init(void)
>> >>>  late_initcall(clk_debug_init);
>> >>>  #else
>> >>>  static inline int clk_debug_register(struct clk *clk) { return 0; }
>> >>> +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
>> >>> +{
>> >>> +}
>> >>>  #endif
>> >>>
>> >>>  /* caller must hold prepare_lock */
>> >>> @@ -1179,16 +1215,8 @@ out:
>> >>>         return ret;
>> >>>  }
>> >>>
>> >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent)
>> >>> +static void clk_reparent(struct clk *clk, struct clk *new_parent)
>> >>>  {
>> >>> -#ifdef CONFIG_COMMON_CLK_DEBUG
>> >>> -       struct dentry *d;
>> >>> -       struct dentry *new_parent_d;
>> >>> -#endif
>> >>> -
>> >>> -       if (!clk || !new_parent)
>> >>> -               return;
>> >>> -
>> >>>         hlist_del(&clk->child_node);
>> >>>
>> >>>         if (new_parent)
>> >>> @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>> >>>         else
>> >>>                 hlist_add_head(&clk->child_node, &clk_orphan_list);
>> >>>
>> >>> -#ifdef CONFIG_COMMON_CLK_DEBUG
>> >>> -       if (!inited)
>> >>> -               goto out;
>> >>> -
>> >>> -       if (new_parent)
>> >>> -               new_parent_d = new_parent->dentry;
>> >>> -       else
>> >>> -               new_parent_d = orphandir;
>> >>> -
>> >>> -       d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
>> >>> -                       new_parent_d, clk->name);
>> >>> -       if (d)
>> >>> -               clk->dentry = d;
>> >>> -       else
>> >>> -               pr_debug("%s: failed to rename debugfs entry for %s\n",
>> >>> -                               __func__, clk->name);
>> >>> -out:
>> >>> -#endif
>> >>> -
>> >>>         clk->parent = new_parent;
>> >>> -
>> >>> -       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>> >>>  }
>> >>>
>> >>>  static int __clk_set_parent(struct clk *clk, struct clk *parent)
>> >>> @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>> >>>         }
>> >>>
>> >>>         /* propagate rate recalculation downstream */
>> >>> -       __clk_reparent(clk, parent);
>> >>> +       clk_reparent(clk, parent);
>> >>> +       clk_debug_reparent(clk, parent);
>> >>> +       __clk_recalc_rates(clk, POST_RATE_CHANGE);
>> >>>
>> >>>  out:
>> >>>         mutex_unlock(&prepare_lock);
>> >>> @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk)
>> >>>         hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) {
>> >>>                 if (orphan->ops->get_parent) {
>> >>>                         i = orphan->ops->get_parent(orphan->hw);
>> >>> -                       if (!strcmp(clk->name, orphan->parent_names[i]))
>> >>> -                               __clk_reparent(orphan, clk);
>> >>> +                       if (!strcmp(clk->name, orphan->parent_names[i])) {
>> >>> +                               clk_reparent(orphan, clk);
>> >>> +                               clk_debug_reparent(orphan, clk);
>> >>> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
>> >>> +                       }
>> >>>                         continue;
>> >>>                 }
>> >>>
>> >>>                 for (i = 0; i < orphan->num_parents; i++)
>> >>>                         if (!strcmp(clk->name, orphan->parent_names[i])) {
>> >>> -                               __clk_reparent(orphan, clk);
>> >>> +                               clk_reparent(orphan, clk);
>> >>> +                               clk_debug_reparent(orphan, clk);
>> >>> +                               __clk_recalc_rates(orphan, POST_RATE_CHANGE);
>> >>>                                 break;
>> >>>                         }
>> >>>          }
>> >>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> >>> index 4989b8a..87a7c2c 100644
>> >>> --- a/include/linux/clk-provider.h
>> >>> +++ b/include/linux/clk-provider.h
>> >>> @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name);
>> >>>   */
>> >>>  int __clk_prepare(struct clk *clk);
>> >>>  void __clk_unprepare(struct clk *clk);
>> >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent);
>> >>>  unsigned long __clk_round_rate(struct clk *clk, unsigned long rate);
>> >>>
>> >>>  struct of_device_id;
>> >>> --
>> >>> 1.7.10