[02/11] clk: Implement clk_set_rate

Commit Message

Mark Brown Aug. 24, 2011, 1:15 p.m. UTC

From: Jeremy Kerr <[e-mail]>

Implemenent clk_set_rate by adding a set_rate callback to clk_hw_ops,
and core code to handle propagation of rate changes up and down the
clock tree.

Signed-off-by: Jeremy Kerr <[e-mail]>
Signed-off-by: Mark Brown <[e-mail]>
---
 drivers/clk/clk.c   |   74 ++++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/clk.h |   12 ++++++++
 2 files changed, 80 insertions(+), 6 deletions(-)

Comments

Sascha Hauer Aug. 26, 2011, 8:53 a.m. UTC | #1

On Wed, Aug 24, 2011 at 02:15:50PM +0100, Mark Brown wrote:
> From: Jeremy Kerr <[e-mail]>
> 
> Implemenent clk_set_rate by adding a set_rate callback to clk_hw_ops,
> and core code to handle propagation of rate changes up and down the
> clock tree.
> 
> Signed-off-by: Jeremy Kerr <[e-mail]>
> Signed-off-by: Mark Brown <[e-mail]>
> ---
>  drivers/clk/clk.c   |   74 ++++++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/clk.h |   12 ++++++++
>  2 files changed, 80 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index ad90a90..3a4d70e 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -21,6 +21,8 @@ struct clk {
>  	unsigned int		enable_count;
>  	unsigned int		prepare_count;
>  	struct clk		*parent;
> +	struct hlist_head	children;
> +	struct hlist_node	child_node;
>  	unsigned long		rate;
>  };
>  
> @@ -176,10 +178,61 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>  }
>  EXPORT_SYMBOL_GPL(clk_round_rate);
>  
> +/*
> + * clk_recalc_rates - Given a clock (with a recently updated clk->rate),
> + *	notify its children that the rate may need to be recalculated, using
> + *	ops->recalc_rate().
> + */
> +static void clk_recalc_rates(struct clk *clk)
> +{
> +	struct hlist_node *tmp;
> +	struct clk *child;
> +
> +	if (clk->ops->recalc_rate)
> +		clk->rate = clk->ops->recalc_rate(clk->hw);
> +
> +	hlist_for_each_entry(child, tmp, &clk->children, child_node)
> +		clk_recalc_rates(child);
> +}
> +
>  int clk_set_rate(struct clk *clk, unsigned long rate)
>  {
> -	/* not yet implemented */
> -	return -ENOSYS;
> +	unsigned long parent_rate, new_rate;
> +	int ret;
> +
> +	if (!clk->ops->set_rate)
> +		return -ENOSYS;
> +
> +	new_rate = rate;
> +
> +	/* prevent racing with updates to the clock topology */
> +	mutex_lock(&prepare_lock);
> +
> +propagate:
> +	ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate);

You have to check for clk->ops->set_rate != NULL here for the
propagation case.

> +
> +	if (ret < 0)
> +		return ret;

This returns with a mutex held. Also, some rates may already have
changed here. I suggest a goto recalc_rates here.

> +
> +	/* ops->set_rate may require the parent's rate to change (to
> +	 * parent_rate), we need to propagate the set_rate call to the
> +	 * parent.
> +	 */
> +	if (ret == CLK_SET_RATE_PROPAGATE) {
> +		new_rate = parent_rate;
> +		clk = clk->parent;
> +		goto propagate;

before propagating you should check if the parent already has the
desired rate. The parent may be a fixed clock which does not have
a set_rate function, still this function shall succeed in this
case.

> +	}
> +
> +	/* If successful (including propagation to the parent clock(s)),
> +	 * recalculate the rates of the clock, including children.  We'll be
> +	 * calling this on the 'parent-most' clock that we propagated to.
> +	 */

recalc_rates:

> +	clk_recalc_rates(clk);
> +
> +	mutex_unlock(&prepare_lock);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(clk_set_rate);
>  
> @@ -213,16 +266,25 @@ struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
>  	clk->hw = hw;
>  	hw->clk = clk;
>  
> -	/* Query the hardware for parent and initial rate */
> +	/* Query the hardware for parent and initial rate. We may alter
> +	 * the clock topology, making this clock available from the parent's
> +	 * children list. So, we need to protect against concurrent
> +	 * accesses through set_rate
> +	 */
> +	mutex_lock(&prepare_lock);
>  
> -	if (clk->ops->get_parent)
> -		/* We don't to lock against prepare/enable here, as
> -		 * the clock is not yet accessible from anywhere */
> +	if (clk->ops->get_parent) {
>  		clk->parent = clk->ops->get_parent(clk->hw);
> +		if (clk->parent)
> +			hlist_add_head(&clk->child_node,
> +					&clk->parent->children);
> +	}
>  
>  	if (clk->ops->recalc_rate)
>  		clk->rate = clk->ops->recalc_rate(clk->hw);
>  
> +	mutex_unlock(&prepare_lock);
> +
>  
>  	return clk;
>  }
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 93ff870..e0969d2 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -58,6 +58,12 @@ struct clk_hw {
>   *		parent. Currently only called when the clock is first
>   *		registered.
>   *
> + * @set_rate	Change the rate of this clock. If this callback returns
> + *		CLK_SET_RATE_PROPAGATE, the rate change will be propagated to
> + *		the parent clock (which may propagate again). The requested
> + *		rate of the parent is passed back from the callback in the
> + *		second 'unsigned long *' argument.
> + *
>   * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
>   * implementations to split any work between atomic (enable) and sleepable
>   * (prepare) contexts.  If a clock requires sleeping code to be turned on, this
> @@ -76,9 +82,15 @@ struct clk_hw_ops {
>  	void		(*disable)(struct clk_hw *);
>  	unsigned long	(*recalc_rate)(struct clk_hw *);
>  	long		(*round_rate)(struct clk_hw *, unsigned long);
> +	int		(*set_rate)(struct clk_hw *,
> +					unsigned long, unsigned long *);
>  	struct clk *	(*get_parent)(struct clk_hw *);
>  };
>  
> +enum {
> +	CLK_SET_RATE_PROPAGATE = 1,
> +};
> +
>  /**
>   * clk_prepare - prepare clock for atomic enabling.
>   *
> -- 
> 1.7.5.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Mark Brown Aug. 26, 2011, 8:55 a.m. UTC | #2

On Fri, Aug 26, 2011 at 10:53:27AM +0200, Sascha Hauer wrote:
> On Wed, Aug 24, 2011 at 02:15:50PM +0100, Mark Brown wrote:
> > From: Jeremy Kerr <[e-mail]>

> > +propagate:
> > +	ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate);

> You have to check for clk->ops->set_rate != NULL here for the
> propagation case.

Note that I'm just forwarding on Jeremy's patches here, I'm not likely
to be able to spend too much time developing the core.

Sascha Hauer Aug. 26, 2011, 8:58 a.m. UTC | #3

On Fri, Aug 26, 2011 at 09:55:10AM +0100, Mark Brown wrote:
> On Fri, Aug 26, 2011 at 10:53:27AM +0200, Sascha Hauer wrote:
> > On Wed, Aug 24, 2011 at 02:15:50PM +0100, Mark Brown wrote:
> > > From: Jeremy Kerr <[e-mail]>
> 
> > > +propagate:
> > > +	ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate);
> 
> > You have to check for clk->ops->set_rate != NULL here for the
> > propagation case.
> 
> Note that I'm just forwarding on Jeremy's patches here, I'm not likely
> to be able to spend too much time developing the core.

Ok, then we have at least some pointer in the archives for the one
who continues on these patches.

Sascha

Mark Brown Aug. 26, 2011, 9:01 a.m. UTC | #4

On Fri, Aug 26, 2011 at 10:58:37AM +0200, Sascha Hauer wrote:
> On Fri, Aug 26, 2011 at 09:55:10AM +0100, Mark Brown wrote:

> > Note that I'm just forwarding on Jeremy's patches here, I'm not likely
> > to be able to spend too much time developing the core.

> Ok, then we have at least some pointer in the archives for the one
> who continues on these patches.

Yes, the review is definitely valuable - I just wanted to avoid any
disappointment if you were expecting me to do anything with it (and
perhaps even encourage you to do so yourself!).

Mike Turquette Aug. 26, 2011, 11:45 p.m. UTC | #5

On Wed, Aug 24, 2011 at 6:15 AM, Mark Brown
<[e-mail]> wrote:
> From: Jeremy Kerr <[e-mail]>
>
> Implemenent clk_set_rate by adding a set_rate callback to clk_hw_ops,
> and core code to handle propagation of rate changes up and down the
> clock tree.

Propagating rates up the tree is wrong.  If a parent rate must be
changed then clk_set_rate should be called on the parent explicitly,
then clk_set_rate can be called on the target child clk.

I don't see any other way to do it that is safe, save for some batch
operation mechanism that takes predefined clock sub-trees into account
(which is likely only feasible for SoCs since their data manuals
provide all this info).

> +/*
> + * clk_recalc_rates - Given a clock (with a recently updated clk->rate),
> + *     notify its children that the rate may need to be recalculated, using
> + *     ops->recalc_rate().
> + */
> +static void clk_recalc_rates(struct clk *clk)
> +{
> +       struct hlist_node *tmp;
> +       struct clk *child;
> +
> +       if (clk->ops->recalc_rate)
> +               clk->rate = clk->ops->recalc_rate(clk->hw);
> +
> +       hlist_for_each_entry(child, tmp, &clk->children, child_node)
> +               clk_recalc_rates(child);

We need a rate post-change notifier here that drivers can subscribe
to.  Since many devices do not support having their clocks changed on
the fly while the device is enabled, a pre-change and post-change
notifier are necessary to notify devices downstream of the clock being
changed.  Imagine doing a rate change high up in the tree (PLL) and
you can see that many devices will need these notifiers propagated to
them.

> +}
> +
>  int clk_set_rate(struct clk *clk, unsigned long rate)
>  {
> -       /* not yet implemented */
> -       return -ENOSYS;
> +       unsigned long parent_rate, new_rate;
> +       int ret;
> +
> +       if (!clk->ops->set_rate)
> +               return -ENOSYS;
> +
> +       new_rate = rate;
> +
> +       /* prevent racing with updates to the clock topology */
> +       mutex_lock(&prepare_lock);
> +
> +propagate:

Need a rate pre-change notifier here that walks the tree.  Drivers
subscribing to this notifier can save context and call their
pm_runtime_put or clk_disable bits so that they can survive the rate
change without any glitches before the rate actually changes.

> +       ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate);
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       /* ops->set_rate may require the parent's rate to change (to
> +        * parent_rate), we need to propagate the set_rate call to the
> +        * parent.
> +        */
> +       if (ret == CLK_SET_RATE_PROPAGATE) {
> +               new_rate = parent_rate;
> +               clk = clk->parent;
> +               goto propagate;
> +       }

Again, I feel this is wrong.  Rate change propagation should only flow
downstream.  Any rate change arbitration that requires changes
upstream should be considered and handled by the driver.

> +
> +       /* If successful (including propagation to the parent clock(s)),
> +        * recalculate the rates of the clock, including children.  We'll be
> +        * calling this on the 'parent-most' clock that we propagated to.
> +        */
> +       clk_recalc_rates(clk);

As mentioned above, recalc should propagate rate post-change notification.

Regards,
Mike

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ad90a90..3a4d70e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -21,6 +21,8 @@  struct clk {
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
 	struct clk		*parent;
+	struct hlist_head	children;
+	struct hlist_node	child_node;
 	unsigned long		rate;
 };
 
@@ -176,10 +178,61 @@  long clk_round_rate(struct clk *clk, unsigned long rate)
 }
 EXPORT_SYMBOL_GPL(clk_round_rate);
 
+/*
+ * clk_recalc_rates - Given a clock (with a recently updated clk->rate),
+ *	notify its children that the rate may need to be recalculated, using
+ *	ops->recalc_rate().
+ */
+static void clk_recalc_rates(struct clk *clk)
+{
+	struct hlist_node *tmp;
+	struct clk *child;
+
+	if (clk->ops->recalc_rate)
+		clk->rate = clk->ops->recalc_rate(clk->hw);
+
+	hlist_for_each_entry(child, tmp, &clk->children, child_node)
+		clk_recalc_rates(child);
+}
+
 int clk_set_rate(struct clk *clk, unsigned long rate)
 {
-	/* not yet implemented */
-	return -ENOSYS;
+	unsigned long parent_rate, new_rate;
+	int ret;
+
+	if (!clk->ops->set_rate)
+		return -ENOSYS;
+
+	new_rate = rate;
+
+	/* prevent racing with updates to the clock topology */
+	mutex_lock(&prepare_lock);
+
+propagate:
+	ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate);
+
+	if (ret < 0)
+		return ret;
+
+	/* ops->set_rate may require the parent's rate to change (to
+	 * parent_rate), we need to propagate the set_rate call to the
+	 * parent.
+	 */
+	if (ret == CLK_SET_RATE_PROPAGATE) {
+		new_rate = parent_rate;
+		clk = clk->parent;
+		goto propagate;
+	}
+
+	/* If successful (including propagation to the parent clock(s)),
+	 * recalculate the rates of the clock, including children.  We'll be
+	 * calling this on the 'parent-most' clock that we propagated to.
+	 */
+	clk_recalc_rates(clk);
+
+	mutex_unlock(&prepare_lock);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(clk_set_rate);
 
@@ -213,16 +266,25 @@  struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
 	clk->hw = hw;
 	hw->clk = clk;
 
-	/* Query the hardware for parent and initial rate */
+	/* Query the hardware for parent and initial rate. We may alter
+	 * the clock topology, making this clock available from the parent's
+	 * children list. So, we need to protect against concurrent
+	 * accesses through set_rate
+	 */
+	mutex_lock(&prepare_lock);
 
-	if (clk->ops->get_parent)
-		/* We don't to lock against prepare/enable here, as
-		 * the clock is not yet accessible from anywhere */
+	if (clk->ops->get_parent) {
 		clk->parent = clk->ops->get_parent(clk->hw);
+		if (clk->parent)
+			hlist_add_head(&clk->child_node,
+					&clk->parent->children);
+	}
 
 	if (clk->ops->recalc_rate)
 		clk->rate = clk->ops->recalc_rate(clk->hw);
 
+	mutex_unlock(&prepare_lock);
+
 
 	return clk;
 }
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 93ff870..e0969d2 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -58,6 +58,12 @@  struct clk_hw {
  *		parent. Currently only called when the clock is first
  *		registered.
  *
+ * @set_rate	Change the rate of this clock. If this callback returns
+ *		CLK_SET_RATE_PROPAGATE, the rate change will be propagated to
+ *		the parent clock (which may propagate again). The requested
+ *		rate of the parent is passed back from the callback in the
+ *		second 'unsigned long *' argument.
+ *
  * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
  * implementations to split any work between atomic (enable) and sleepable
  * (prepare) contexts.  If a clock requires sleeping code to be turned on, this
@@ -76,9 +82,15 @@  struct clk_hw_ops {
 	void		(*disable)(struct clk_hw *);
 	unsigned long	(*recalc_rate)(struct clk_hw *);
 	long		(*round_rate)(struct clk_hw *, unsigned long);
+	int		(*set_rate)(struct clk_hw *,
+					unsigned long, unsigned long *);
 	struct clk *	(*get_parent)(struct clk_hw *);
 };
 
+enum {
+	CLK_SET_RATE_PROPAGATE = 1,
+};
+
 /**
  * clk_prepare - prepare clock for atomic enabling.
  *