[V2,1/3] clk: Restructure code for __clk_reparent

Commit Message

Ulf Hansson March 21, 2013, 1:48 p.m. UTC

From: Ulf Hansson <[e-mail]>

Split __clk_reparent into three pieces, one for doing the actual
reparent for updating the clock tree topology, one for the
COMMON_CLK_DEBUG code and one for doing the rate recalculation.

This patch also makes it possible to hold the spinlock over the
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 <[e-mail]>
---
 drivers/clk/clk.c |   74 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 29 deletions(-)

Comments

Mike Turquette March 21, 2013, 8:53 p.m. UTC | #1

Quoting Ulf Hansson (2013-03-21 06:48:11)
> +void __clk_reparent(struct clk *clk, struct clk *new_parent)
> +{
> +       clk_reparent(clk, new_parent);
> +       clk_debug_reparent(clk, new_parent);
>         __clk_recalc_rates(clk, POST_RATE_CHANGE);
>  }
>  
> @@ -1364,7 +1378,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);
>  

This is an interesting change.  Why not call __clk_reparent here instead
of open coding an identical sequence?

Regards,
Mike

Ulf Hansson March 22, 2013, 11:08 a.m. UTC | #2

On 21 March 2013 21:53, Mike Turquette <[e-mail]> wrote:
> Quoting Ulf Hansson (2013-03-21 06:48:11)
>> +void __clk_reparent(struct clk *clk, struct clk *new_parent)
>> +{
>> +       clk_reparent(clk, new_parent);
>> +       clk_debug_reparent(clk, new_parent);
>>         __clk_recalc_rates(clk, POST_RATE_CHANGE);
>>  }
>>
>> @@ -1364,7 +1378,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);
>>
>
> This is an interesting change.  Why not call __clk_reparent here instead
> of open coding an identical sequence?

By lazyness when rebasing patches I decided to keep it. :-) Well, my
idea here was also to make it visible how these three calls will be
split out to be called from three different places.

Do you want me to fixup?

Br
Uffe

>
> Regards,
> Mike

Mike Turquette March 22, 2013, 8:39 p.m. UTC | #3

Quoting Ulf Hansson (2013-03-22 04:08:09)
> On 21 March 2013 21:53, Mike Turquette <[e-mail]> wrote:
> > Quoting Ulf Hansson (2013-03-21 06:48:11)
> >> +void __clk_reparent(struct clk *clk, struct clk *new_parent)
> >> +{
> >> +       clk_reparent(clk, new_parent);
> >> +       clk_debug_reparent(clk, new_parent);
> >>         __clk_recalc_rates(clk, POST_RATE_CHANGE);
> >>  }
> >>
> >> @@ -1364,7 +1378,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);
> >>
> >
> > This is an interesting change.  Why not call __clk_reparent here instead
> > of open coding an identical sequence?
> 
> By lazyness when rebasing patches I decided to keep it. :-) Well, my
> idea here was also to make it visible how these three calls will be
> split out to be called from three different places.
> 
> Do you want me to fixup?
> 

Please do.  It seems you'll be sending another version anyways so just
roll it in :)

Thanks much,
Mike

> Br
> Uffe
> 
> >
> > Regards,
> > Mike

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 253792a..d73fb0f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -280,6 +280,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
@@ -333,6 +366,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 */
@@ -1214,16 +1250,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)
@@ -1231,27 +1259,13 @@  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;
+}
 
+void __clk_reparent(struct clk *clk, struct clk *new_parent)
+{
+	clk_reparent(clk, new_parent);
+	clk_debug_reparent(clk, new_parent);
 	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 }
 
@@ -1364,7 +1378,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);