[PATCH 0/6] Coordinated Clks

[PATCH 0/6] Coordinated Clks

[Derek Basehore]

Here's the first set of patches that I'm working on for the Common
Clk Framework. Part of this patch series adds a new clk op,
pre_rate_req. This is designed to replace the clk notifier approach
that many clk drivers use right now to setup alt parents or temporary
dividers. This should allow for the removal of the
CLK_RECALC_NEW_RATES flag and the implementation of a better locking
scheme for the prepare lock.

Derek Basehore (5):
  clk: fix clk_calc_subtree compute duplications
  clk: change rates via list iteration
  clk: add pre clk changes support
  docs: driver-api: add pre_rate_req to clk documentation
  clk: rockchip: use pre_rate_req for cpuclk

Stephen Boyd (1):
  clk: Remove recursion in clk_core_{prepare,enable}()

 Documentation/driver-api/clk.rst |   7 +-
 drivers/clk/clk.c                | 484 +++++++++++++++++++++++--------
 drivers/clk/rockchip/clk-cpu.c   | 256 ++++++++--------
 include/linux/clk-provider.h     |  10 +
 4 files changed, 509 insertions(+), 248 deletions(-)

-- 
2.19.1.568.g152ad8e336-goog

[PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[Derek Basehore]

From: Stephen Boyd <sboyd@codeaurora.org>

Enabling and preparing clocks can be written quite naturally with
recursion. We start at some point in the tree and recurse up the
tree to find the oldest parent clk that needs to be enabled or
prepared. Then we enable/prepare and return to the caller, going
back to the clk we started at and enabling/preparing along the
way.

The problem is recursion isn't great for kernel code where we
have a limited stack size. Furthermore, we may be calling this
code inside clk_set_rate() which also has recursion in it, so
we're really not looking good if we encounter a tall clk tree.

Let's create a stack instead by looping over the parent chain and
collecting clks of interest. Then the enable/prepare becomes as
simple as iterating over that list and calling enable.

Cc: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/clk/clk.c | 113 ++++++++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 49 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index af011974d4ec..95d818f5edb2 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -71,6 +71,8 @@ struct clk_core {
 	struct hlist_head	children;
 	struct hlist_node	child_node;
 	struct hlist_head	clks;
+	struct list_head	prepare_list;
+	struct list_head	enable_list;
 	unsigned int		notifier_count;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry		*dentry;
@@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
 static int clk_core_prepare(struct clk_core *core)
 {
 	int ret = 0;
+	struct clk_core *tmp, *parent;
+	LIST_HEAD(head);
 
 	lockdep_assert_held(&prepare_lock);
 
-	if (!core)
-		return 0;
+	while (core) {
+		list_add(&core->prepare_list, &head);
+		/* Stop once we see a clk that is already prepared */
+		if (core->prepare_count)
+			break;
+		core = core->parent;
+	}
 
-	if (core->prepare_count == 0) {
-		ret = clk_pm_runtime_get(core);
-		if (ret)
-			return ret;
+	list_for_each_entry_safe(core, tmp, &head, prepare_list) {
+		list_del_init(&core->prepare_list);
 
-		ret = clk_core_prepare(core->parent);
-		if (ret)
-			goto runtime_put;
+		if (core->prepare_count == 0) {
+			ret = clk_pm_runtime_get(core);
+			if (ret)
+				goto err;
 
-		trace_clk_prepare(core);
+			trace_clk_prepare(core);
 
-		if (core->ops->prepare)
-			ret = core->ops->prepare(core->hw);
+			if (core->ops->prepare)
+				ret = core->ops->prepare(core->hw);
 
-		trace_clk_prepare_complete(core);
+			trace_clk_prepare_complete(core);
 
-		if (ret)
-			goto unprepare;
+			if (ret) {
+				clk_pm_runtime_put(core);
+				goto err;
+			}
+		}
+		core->prepare_count++;
 	}
 
-	core->prepare_count++;
-
-	/*
-	 * CLK_SET_RATE_GATE is a special case of clock protection
-	 * Instead of a consumer claiming exclusive rate control, it is
-	 * actually the provider which prevents any consumer from making any
-	 * operation which could result in a rate change or rate glitch while
-	 * the clock is prepared.
-	 */
-	if (core->flags & CLK_SET_RATE_GATE)
-		clk_core_rate_protect(core);
-
 	return 0;
-unprepare:
-	clk_core_unprepare(core->parent);
-runtime_put:
-	clk_pm_runtime_put(core);
+err:
+	parent = core->parent;
+	list_for_each_entry_safe_continue(core, tmp, &head, prepare_list)
+		list_del_init(&core->prepare_list);
+	clk_core_unprepare(parent);
 	return ret;
 }
 
@@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable);
 static int clk_core_enable(struct clk_core *core)
 {
 	int ret = 0;
+	struct clk_core *tmp, *parent;
+	LIST_HEAD(head);
 
 	lockdep_assert_held(&enable_lock);
 
-	if (!core)
-		return 0;
-
-	if (WARN(core->prepare_count == 0,
-	    "Enabling unprepared %s\n", core->name))
-		return -ESHUTDOWN;
+	while (core) {
+		list_add(&core->enable_list, &head);
+		/* Stop once we see a clk that is already enabled */
+		if (core->enable_count)
+			break;
+		core = core->parent;
+	}
 
-	if (core->enable_count == 0) {
-		ret = clk_core_enable(core->parent);
+	list_for_each_entry_safe(core, tmp, &head, enable_list) {
+		list_del_init(&core->enable_list);
 
-		if (ret)
-			return ret;
+		if (WARN_ON(core->prepare_count == 0)) {
+			ret = -ESHUTDOWN;
+			goto err;
+		}
 
-		trace_clk_enable_rcuidle(core);
+		if (core->enable_count == 0) {
+			trace_clk_enable_rcuidle(core);
 
-		if (core->ops->enable)
-			ret = core->ops->enable(core->hw);
+			if (core->ops->enable)
+				ret = core->ops->enable(core->hw);
 
-		trace_clk_enable_complete_rcuidle(core);
+			trace_clk_enable_complete_rcuidle(core);
 
-		if (ret) {
-			clk_core_disable(core->parent);
-			return ret;
+			if (ret)
+				goto err;
 		}
+
+		core->enable_count++;
 	}
 
-	core->enable_count++;
 	return 0;
+err:
+	parent = core->parent;
+	list_for_each_entry_safe_continue(core, tmp, &head, enable_list)
+		list_del_init(&core->enable_list);
+	clk_core_disable(parent);
+	return ret;
 }
 
 static int clk_core_enable_lock(struct clk_core *core)
@@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 	core->num_parents = hw->init->num_parents;
 	core->min_rate = 0;
 	core->max_rate = ULONG_MAX;
+	INIT_LIST_HEAD(&core->prepare_list);
+	INIT_LIST_HEAD(&core->enable_list);
 	hw->core = core;
 
 	/* allocate local copy in case parent_names is __initdata */
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[Jerome Brunet]

On Tue, 2018-10-23 at 18:31 -0700, Derek Basehore wrote:
> From: Stephen Boyd <sboyd@codeaurora.org>
> 
> Enabling and preparing clocks can be written quite naturally with
> recursion. We start at some point in the tree and recurse up the
> tree to find the oldest parent clk that needs to be enabled or
> prepared. Then we enable/prepare and return to the caller, going
> back to the clk we started at and enabling/preparing along the
> way.
> 
> The problem is recursion isn't great for kernel code where we
> have a limited stack size. Furthermore, we may be calling this
> code inside clk_set_rate() which also has recursion in it, so
> we're really not looking good if we encounter a tall clk tree.
> 
> Let's create a stack instead by looping over the parent chain and
> collecting clks of interest. Then the enable/prepare becomes as
> simple as iterating over that list and calling enable.

Hi Derek,

What about unprepare() and disable() ?

This patch removes the recursion from the enable path but keeps it for the
disable path ... this is very odd. Assuming doing so works, It certainly makes
CCF a lot harder to understand.

What about clock protection which essentially works on the same model as prepare
and enable ?

Overall, this change does not look like something that should be merged as it
is. If you were just seeking comments, you should add the "RFC" tag to your
series.

Jerome.

> 
> Cc: Jerome Brunet <jbrunet@baylibre.com>

If you don't mind, I would prefer to get the whole series next time. It helps to
get the context.

> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/clk/clk.c | 113 ++++++++++++++++++++++++++--------------------
>  1 file changed, 64 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index af011974d4ec..95d818f5edb2 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -71,6 +71,8 @@ struct clk_core {
>  	struct hlist_head	children;
>  	struct hlist_node	child_node;
>  	struct hlist_head	clks;
> +	struct list_head	prepare_list;
> +	struct list_head	enable_list;
>  	unsigned int		notifier_count;
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry		*dentry;
> @@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
>  static int clk_core_prepare(struct clk_core *core)
>  {
>  	int ret = 0;
> +	struct clk_core *tmp, *parent;
> +	LIST_HEAD(head);
>  
>  	lockdep_assert_held(&prepare_lock);
>  
> -	if (!core)
> -		return 0;
> +	while (core) {
> +		list_add(&core->prepare_list, &head);
> +		/* Stop once we see a clk that is already prepared */
> +		if (core->prepare_count)
> +			break;
> +		core = core->parent;
> +	}
>  
> -	if (core->prepare_count == 0) {
> -		ret = clk_pm_runtime_get(core);
> -		if (ret)
> -			return ret;
> +	list_for_each_entry_safe(core, tmp, &head, prepare_list) {
> +		list_del_init(&core->prepare_list);

Is there any point in removing it from the list ?
Maybe I missed it but it does not seems useful.

Without this, we could use list_for_each_entry()

>  
> -		ret = clk_core_prepare(core->parent);
> -		if (ret)
> -			goto runtime_put;
> +		if (core->prepare_count == 0) {

Should we really check the count here ? You are not checking the count when the
put() counterpart is called below.

Since PM runtime has ref counting as well, either way would work I guess ... but
we shall be consistent

> +			ret = clk_pm_runtime_get(core);
> +			if (ret)
> +				goto err;
>  
> -		trace_clk_prepare(core);
> +			trace_clk_prepare(core);
>  
> -		if (core->ops->prepare)
> -			ret = core->ops->prepare(core->hw);
> +			if (core->ops->prepare)
> +				ret = core->ops->prepare(core->hw);
>  
> -		trace_clk_prepare_complete(core);
> +			trace_clk_prepare_complete(core);
>  
> -		if (ret)
> -			goto unprepare;
> +			if (ret) {
> +				clk_pm_runtime_put(core);
> +				goto err;
> +			}
> +		}
> +		core->prepare_count++;
>  	}
>  
> -	core->prepare_count++;
> -
> -	/*
> -	 * CLK_SET_RATE_GATE is a special case of clock protection
> -	 * Instead of a consumer claiming exclusive rate control, it is
> -	 * actually the provider which prevents any consumer from making any
> -	 * operation which could result in a rate change or rate glitch while
> -	 * the clock is prepared.
> -	 */
> -	if (core->flags & CLK_SET_RATE_GATE)
> -		clk_core_rate_protect(core);

This gets removed without anything replacing it.

is CLK_SET_RATE_GATE and clock protection support dropped after this change ?

> -
>  	return 0;
> -unprepare:
> -	clk_core_unprepare(core->parent);
> -runtime_put:
> -	clk_pm_runtime_put(core);
> +err:
> +	parent = core->parent;
> +	list_for_each_entry_safe_continue(core, tmp, &head, prepare_list)
> +		list_del_init(&core->prepare_list);
> +	clk_core_unprepare(parent);

If you get here because of failure clk_pm_runtime_get(), you will unprepare a
clock which may have not been prepared first

Overall the rework of error exit path does not seem right (or necessary)

>  	return ret;
>  }
>  
> @@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable);
>  static int clk_core_enable(struct clk_core *core)
>  {
>  	int ret = 0;
> +	struct clk_core *tmp, *parent;
> +	LIST_HEAD(head);
>  
>  	lockdep_assert_held(&enable_lock);
>  
> -	if (!core)
> -		return 0;
> -
> -	if (WARN(core->prepare_count == 0,
> -	    "Enabling unprepared %s\n", core->name))
> -		return -ESHUTDOWN;
> +	while (core) {
> +		list_add(&core->enable_list, &head);
> +		/* Stop once we see a clk that is already enabled */
> +		if (core->enable_count)
> +			break;
> +		core = core->parent;
> +	}
>  
> -	if (core->enable_count == 0) {
> -		ret = clk_core_enable(core->parent);
> +	list_for_each_entry_safe(core, tmp, &head, enable_list) {
> +		list_del_init(&core->enable_list);
>  
> -		if (ret)
> -			return ret;
> +		if (WARN_ON(core->prepare_count == 0)) {
> +			ret = -ESHUTDOWN;
> +			goto err;
> +		}
>  
> -		trace_clk_enable_rcuidle(core);
> +		if (core->enable_count == 0) {
> +			trace_clk_enable_rcuidle(core);
>  
> -		if (core->ops->enable)
> -			ret = core->ops->enable(core->hw);
> +			if (core->ops->enable)
> +				ret = core->ops->enable(core->hw);
>  
> -		trace_clk_enable_complete_rcuidle(core);
> +			trace_clk_enable_complete_rcuidle(core);
>  
> -		if (ret) {
> -			clk_core_disable(core->parent);
> -			return ret;
> +			if (ret)
> +				goto err;
>  		}
> +
> +		core->enable_count++;
>  	}
>  
> -	core->enable_count++;
>  	return 0;
> +err:
> +	parent = core->parent;
> +	list_for_each_entry_safe_continue(core, tmp, &head, enable_list)
> +		list_del_init(&core->enable_list);
> +	clk_core_disable(parent);
> +	return ret;
>  }
>  
>  static int clk_core_enable_lock(struct clk_core *core)
> @@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  	core->num_parents = hw->init->num_parents;
>  	core->min_rate = 0;
>  	core->max_rate = ULONG_MAX;
> +	INIT_LIST_HEAD(&core->prepare_list);
> +	INIT_LIST_HEAD(&core->enable_list);
>  	hw->core = core;
>  
>  	/* allocate local copy in case parent_names is __initdata */

Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[dbasehore .]

On Wed, Oct 24, 2018 at 2:51 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On Tue, 2018-10-23 at 18:31 -0700, Derek Basehore wrote:
> > From: Stephen Boyd <sboyd@codeaurora.org>
> >
> > Enabling and preparing clocks can be written quite naturally with
> > recursion. We start at some point in the tree and recurse up the
> > tree to find the oldest parent clk that needs to be enabled or
> > prepared. Then we enable/prepare and return to the caller, going
> > back to the clk we started at and enabling/preparing along the
> > way.
> >
> > The problem is recursion isn't great for kernel code where we
> > have a limited stack size. Furthermore, we may be calling this
> > code inside clk_set_rate() which also has recursion in it, so
> > we're really not looking good if we encounter a tall clk tree.
> >
> > Let's create a stack instead by looping over the parent chain and
> > collecting clks of interest. Then the enable/prepare becomes as
> > simple as iterating over that list and calling enable.
>
> Hi Derek,
>
> What about unprepare() and disable() ?
>
> This patch removes the recursion from the enable path but keeps it for the
> disable path ... this is very odd. Assuming doing so works, It certainly makes
> CCF a lot harder to understand.
>
> What about clock protection which essentially works on the same model as prepare
> and enable ?
>
> Overall, this change does not look like something that should be merged as it
> is. If you were just seeking comments, you should add the "RFC" tag to your
> series.
>
> Jerome.
>
> >
> > Cc: Jerome Brunet <jbrunet@baylibre.com>
>
> If you don't mind, I would prefer to get the whole series next time. It helps to
> get the context.
>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  drivers/clk/clk.c | 113 ++++++++++++++++++++++++++--------------------
> >  1 file changed, 64 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index af011974d4ec..95d818f5edb2 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -71,6 +71,8 @@ struct clk_core {
> >       struct hlist_head       children;
> >       struct hlist_node       child_node;
> >       struct hlist_head       clks;
> > +     struct list_head        prepare_list;
> > +     struct list_head        enable_list;
> >       unsigned int            notifier_count;
> >  #ifdef CONFIG_DEBUG_FS
> >       struct dentry           *dentry;
> > @@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
> >  static int clk_core_prepare(struct clk_core *core)
> >  {
> >       int ret = 0;
> > +     struct clk_core *tmp, *parent;
> > +     LIST_HEAD(head);
> >
> >       lockdep_assert_held(&prepare_lock);
> >
> > -     if (!core)
> > -             return 0;
> > +     while (core) {
> > +             list_add(&core->prepare_list, &head);
> > +             /* Stop once we see a clk that is already prepared */
> > +             if (core->prepare_count)
> > +                     break;
> > +             core = core->parent;
> > +     }
> >
> > -     if (core->prepare_count == 0) {
> > -             ret = clk_pm_runtime_get(core);
> > -             if (ret)
> > -                     return ret;
> > +     list_for_each_entry_safe(core, tmp, &head, prepare_list) {
> > +             list_del_init(&core->prepare_list);
>
> Is there any point in removing it from the list ?
> Maybe I missed it but it does not seems useful.
>
> Without this, we could use list_for_each_entry()
>
> >
> > -             ret = clk_core_prepare(core->parent);
> > -             if (ret)
> > -                     goto runtime_put;
> > +             if (core->prepare_count == 0) {
>
> Should we really check the count here ? You are not checking the count when the
> put() counterpart is called below.

I think I accidentally messed that up when I picked up the patch.
There were some merge conflicts with the addition of the
clk_pm_runtime code.

>
> Since PM runtime has ref counting as well, either way would work I guess ... but
> we shall be consistent
>
> > +                     ret = clk_pm_runtime_get(core);
> > +                     if (ret)
> > +                             goto err;
> >
> > -             trace_clk_prepare(core);
> > +                     trace_clk_prepare(core);
> >
> > -             if (core->ops->prepare)
> > -                     ret = core->ops->prepare(core->hw);
> > +                     if (core->ops->prepare)
> > +                             ret = core->ops->prepare(core->hw);
> >
> > -             trace_clk_prepare_complete(core);
> > +                     trace_clk_prepare_complete(core);
> >
> > -             if (ret)
> > -                     goto unprepare;
> > +                     if (ret) {
> > +                             clk_pm_runtime_put(core);
> > +                             goto err;
> > +                     }
> > +             }
> > +             core->prepare_count++;
> >       }
> >
> > -     core->prepare_count++;
> > -
> > -     /*
> > -      * CLK_SET_RATE_GATE is a special case of clock protection
> > -      * Instead of a consumer claiming exclusive rate control, it is
> > -      * actually the provider which prevents any consumer from making any
> > -      * operation which could result in a rate change or rate glitch while
> > -      * the clock is prepared.
> > -      */
> > -     if (core->flags & CLK_SET_RATE_GATE)
> > -             clk_core_rate_protect(core);
>
> This gets removed without anything replacing it.
>
> is CLK_SET_RATE_GATE and clock protection support dropped after this change ?

No, I think I just accidentally removed this when resolving conflicts.

>
> > -
> >       return 0;
> > -unprepare:
> > -     clk_core_unprepare(core->parent);
> > -runtime_put:
> > -     clk_pm_runtime_put(core);
> > +err:
> > +     parent = core->parent;
> > +     list_for_each_entry_safe_continue(core, tmp, &head, prepare_list)
> > +             list_del_init(&core->prepare_list);
> > +     clk_core_unprepare(parent);
>
> If you get here because of failure clk_pm_runtime_get(), you will unprepare a
> clock which may have not been prepared first
>
> Overall the rework of error exit path does not seem right (or necessary)
>

Yeah, all of this seems to just be a poor resolution of patch
conflicts on my part. Will fix.

> >       return ret;
> >  }
> >
> > @@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable);
> >  static int clk_core_enable(struct clk_core *core)
> >  {
> >       int ret = 0;
> > +     struct clk_core *tmp, *parent;
> > +     LIST_HEAD(head);
> >
> >       lockdep_assert_held(&enable_lock);
> >
> > -     if (!core)
> > -             return 0;
> > -
> > -     if (WARN(core->prepare_count == 0,
> > -         "Enabling unprepared %s\n", core->name))
> > -             return -ESHUTDOWN;
> > +     while (core) {
> > +             list_add(&core->enable_list, &head);
> > +             /* Stop once we see a clk that is already enabled */
> > +             if (core->enable_count)
> > +                     break;
> > +             core = core->parent;
> > +     }
> >
> > -     if (core->enable_count == 0) {
> > -             ret = clk_core_enable(core->parent);
> > +     list_for_each_entry_safe(core, tmp, &head, enable_list) {
> > +             list_del_init(&core->enable_list);
> >
> > -             if (ret)
> > -                     return ret;
> > +             if (WARN_ON(core->prepare_count == 0)) {
> > +                     ret = -ESHUTDOWN;
> > +                     goto err;
> > +             }
> >
> > -             trace_clk_enable_rcuidle(core);
> > +             if (core->enable_count == 0) {
> > +                     trace_clk_enable_rcuidle(core);
> >
> > -             if (core->ops->enable)
> > -                     ret = core->ops->enable(core->hw);
> > +                     if (core->ops->enable)
> > +                             ret = core->ops->enable(core->hw);
> >
> > -             trace_clk_enable_complete_rcuidle(core);
> > +                     trace_clk_enable_complete_rcuidle(core);
> >
> > -             if (ret) {
> > -                     clk_core_disable(core->parent);
> > -                     return ret;
> > +                     if (ret)
> > +                             goto err;
> >               }
> > +
> > +             core->enable_count++;
> >       }
> >
> > -     core->enable_count++;
> >       return 0;
> > +err:
> > +     parent = core->parent;
> > +     list_for_each_entry_safe_continue(core, tmp, &head, enable_list)
> > +             list_del_init(&core->enable_list);
> > +     clk_core_disable(parent);
> > +     return ret;
> >  }
> >
> >  static int clk_core_enable_lock(struct clk_core *core)
> > @@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> >       core->num_parents = hw->init->num_parents;
> >       core->min_rate = 0;
> >       core->max_rate = ULONG_MAX;
> > +     INIT_LIST_HEAD(&core->prepare_list);
> > +     INIT_LIST_HEAD(&core->enable_list);
> >       hw->core = core;
> >
> >       /* allocate local copy in case parent_names is __initdata */
>
>

Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[dbasehore .]

On Wed, Oct 24, 2018 at 1:15 PM dbasehore . <dbasehore@chromium.org> wrote:
>
> On Wed, Oct 24, 2018 at 2:51 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> >
> > On Tue, 2018-10-23 at 18:31 -0700, Derek Basehore wrote:
> > > From: Stephen Boyd <sboyd@codeaurora.org>
> > >
> > > Enabling and preparing clocks can be written quite naturally with
> > > recursion. We start at some point in the tree and recurse up the
> > > tree to find the oldest parent clk that needs to be enabled or
> > > prepared. Then we enable/prepare and return to the caller, going
> > > back to the clk we started at and enabling/preparing along the
> > > way.
> > >
> > > The problem is recursion isn't great for kernel code where we
> > > have a limited stack size. Furthermore, we may be calling this
> > > code inside clk_set_rate() which also has recursion in it, so
> > > we're really not looking good if we encounter a tall clk tree.
> > >
> > > Let's create a stack instead by looping over the parent chain and
> > > collecting clks of interest. Then the enable/prepare becomes as
> > > simple as iterating over that list and calling enable.
> >
> > Hi Derek,
> >
> > What about unprepare() and disable() ?
> >
> > This patch removes the recursion from the enable path but keeps it for the
> > disable path ... this is very odd. Assuming doing so works, It certainly makes
> > CCF a lot harder to understand.
> >
> > What about clock protection which essentially works on the same model as prepare
> > and enable ?
> >
> > Overall, this change does not look like something that should be merged as it
> > is. If you were just seeking comments, you should add the "RFC" tag to your
> > series.
> >
> > Jerome.
> >
> > >
> > > Cc: Jerome Brunet <jbrunet@baylibre.com>
> >
> > If you don't mind, I would prefer to get the whole series next time. It helps to
> > get the context.
> >
> > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > >  drivers/clk/clk.c | 113 ++++++++++++++++++++++++++--------------------
> > >  1 file changed, 64 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index af011974d4ec..95d818f5edb2 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -71,6 +71,8 @@ struct clk_core {
> > >       struct hlist_head       children;
> > >       struct hlist_node       child_node;
> > >       struct hlist_head       clks;
> > > +     struct list_head        prepare_list;
> > > +     struct list_head        enable_list;
> > >       unsigned int            notifier_count;
> > >  #ifdef CONFIG_DEBUG_FS
> > >       struct dentry           *dentry;
> > > @@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
> > >  static int clk_core_prepare(struct clk_core *core)
> > >  {
> > >       int ret = 0;
> > > +     struct clk_core *tmp, *parent;
> > > +     LIST_HEAD(head);
> > >
> > >       lockdep_assert_held(&prepare_lock);
> > >
> > > -     if (!core)
> > > -             return 0;
> > > +     while (core) {
> > > +             list_add(&core->prepare_list, &head);
> > > +             /* Stop once we see a clk that is already prepared */
> > > +             if (core->prepare_count)
> > > +                     break;
> > > +             core = core->parent;
> > > +     }
> > >
> > > -     if (core->prepare_count == 0) {
> > > -             ret = clk_pm_runtime_get(core);
> > > -             if (ret)
> > > -                     return ret;
> > > +     list_for_each_entry_safe(core, tmp, &head, prepare_list) {
> > > +             list_del_init(&core->prepare_list);
> >
> > Is there any point in removing it from the list ?
> > Maybe I missed it but it does not seems useful.
> >
> > Without this, we could use list_for_each_entry()
> >
> > >
> > > -             ret = clk_core_prepare(core->parent);
> > > -             if (ret)
> > > -                     goto runtime_put;
> > > +             if (core->prepare_count == 0) {
> >
> > Should we really check the count here ? You are not checking the count when the
> > put() counterpart is called below.
>
> I think I accidentally messed that up when I picked up the patch.
> There were some merge conflicts with the addition of the
> clk_pm_runtime code.
>
> >
> > Since PM runtime has ref counting as well, either way would work I guess ... but
> > we shall be consistent
> >
> > > +                     ret = clk_pm_runtime_get(core);
> > > +                     if (ret)
> > > +                             goto err;
> > >
> > > -             trace_clk_prepare(core);
> > > +                     trace_clk_prepare(core);
> > >
> > > -             if (core->ops->prepare)
> > > -                     ret = core->ops->prepare(core->hw);
> > > +                     if (core->ops->prepare)
> > > +                             ret = core->ops->prepare(core->hw);
> > >
> > > -             trace_clk_prepare_complete(core);
> > > +                     trace_clk_prepare_complete(core);
> > >
> > > -             if (ret)
> > > -                     goto unprepare;
> > > +                     if (ret) {
> > > +                             clk_pm_runtime_put(core);
> > > +                             goto err;
> > > +                     }
> > > +             }
> > > +             core->prepare_count++;
> > >       }
> > >
> > > -     core->prepare_count++;
> > > -
> > > -     /*
> > > -      * CLK_SET_RATE_GATE is a special case of clock protection
> > > -      * Instead of a consumer claiming exclusive rate control, it is
> > > -      * actually the provider which prevents any consumer from making any
> > > -      * operation which could result in a rate change or rate glitch while
> > > -      * the clock is prepared.
> > > -      */
> > > -     if (core->flags & CLK_SET_RATE_GATE)
> > > -             clk_core_rate_protect(core);
> >
> > This gets removed without anything replacing it.
> >
> > is CLK_SET_RATE_GATE and clock protection support dropped after this change ?
>
> No, I think I just accidentally removed this when resolving conflicts.
>
> >
> > > -
> > >       return 0;
> > > -unprepare:
> > > -     clk_core_unprepare(core->parent);
> > > -runtime_put:
> > > -     clk_pm_runtime_put(core);
> > > +err:
> > > +     parent = core->parent;
> > > +     list_for_each_entry_safe_continue(core, tmp, &head, prepare_list)
> > > +             list_del_init(&core->prepare_list);
> > > +     clk_core_unprepare(parent);
> >
> > If you get here because of failure clk_pm_runtime_get(), you will unprepare a
> > clock which may have not been prepared first
> >
> > Overall the rework of error exit path does not seem right (or necessary)
> >
>
> Yeah, all of this seems to just be a poor resolution of patch
> conflicts on my part. Will fix.
>

Actually, I think that the rework of the error exit path makes sense
since the prior exit path was for the recursive case, not the list
iteration case. I will fix the issue where we call clk_core_unprepare
when it shouldn't, though.

> > >       return ret;
> > >  }
> > >
> > > @@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable);
> > >  static int clk_core_enable(struct clk_core *core)
> > >  {
> > >       int ret = 0;
> > > +     struct clk_core *tmp, *parent;
> > > +     LIST_HEAD(head);
> > >
> > >       lockdep_assert_held(&enable_lock);
> > >
> > > -     if (!core)
> > > -             return 0;
> > > -
> > > -     if (WARN(core->prepare_count == 0,
> > > -         "Enabling unprepared %s\n", core->name))
> > > -             return -ESHUTDOWN;
> > > +     while (core) {
> > > +             list_add(&core->enable_list, &head);
> > > +             /* Stop once we see a clk that is already enabled */
> > > +             if (core->enable_count)
> > > +                     break;
> > > +             core = core->parent;
> > > +     }
> > >
> > > -     if (core->enable_count == 0) {
> > > -             ret = clk_core_enable(core->parent);
> > > +     list_for_each_entry_safe(core, tmp, &head, enable_list) {
> > > +             list_del_init(&core->enable_list);
> > >
> > > -             if (ret)
> > > -                     return ret;
> > > +             if (WARN_ON(core->prepare_count == 0)) {
> > > +                     ret = -ESHUTDOWN;
> > > +                     goto err;
> > > +             }
> > >
> > > -             trace_clk_enable_rcuidle(core);
> > > +             if (core->enable_count == 0) {
> > > +                     trace_clk_enable_rcuidle(core);
> > >
> > > -             if (core->ops->enable)
> > > -                     ret = core->ops->enable(core->hw);
> > > +                     if (core->ops->enable)
> > > +                             ret = core->ops->enable(core->hw);
> > >
> > > -             trace_clk_enable_complete_rcuidle(core);
> > > +                     trace_clk_enable_complete_rcuidle(core);
> > >
> > > -             if (ret) {
> > > -                     clk_core_disable(core->parent);
> > > -                     return ret;
> > > +                     if (ret)
> > > +                             goto err;
> > >               }
> > > +
> > > +             core->enable_count++;
> > >       }
> > >
> > > -     core->enable_count++;
> > >       return 0;
> > > +err:
> > > +     parent = core->parent;
> > > +     list_for_each_entry_safe_continue(core, tmp, &head, enable_list)
> > > +             list_del_init(&core->enable_list);
> > > +     clk_core_disable(parent);
> > > +     return ret;
> > >  }
> > >
> > >  static int clk_core_enable_lock(struct clk_core *core)
> > > @@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> > >       core->num_parents = hw->init->num_parents;
> > >       core->min_rate = 0;
> > >       core->max_rate = ULONG_MAX;
> > > +     INIT_LIST_HEAD(&core->prepare_list);
> > > +     INIT_LIST_HEAD(&core->enable_list);
> > >       hw->core = core;
> > >
> > >       /* allocate local copy in case parent_names is __initdata */
> >
> >

Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[dbasehore .]

On Wed, Oct 24, 2018 at 1:15 PM dbasehore . <dbasehore@chromium.org> wrote:
>
> On Wed, Oct 24, 2018 at 2:51 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> >
> > On Tue, 2018-10-23 at 18:31 -0700, Derek Basehore wrote:
> > > From: Stephen Boyd <sboyd@codeaurora.org>
> > >
> > > Enabling and preparing clocks can be written quite naturally with
> > > recursion. We start at some point in the tree and recurse up the
> > > tree to find the oldest parent clk that needs to be enabled or
> > > prepared. Then we enable/prepare and return to the caller, going
> > > back to the clk we started at and enabling/preparing along the
> > > way.
> > >
> > > The problem is recursion isn't great for kernel code where we
> > > have a limited stack size. Furthermore, we may be calling this
> > > code inside clk_set_rate() which also has recursion in it, so
> > > we're really not looking good if we encounter a tall clk tree.
> > >
> > > Let's create a stack instead by looping over the parent chain and
> > > collecting clks of interest. Then the enable/prepare becomes as
> > > simple as iterating over that list and calling enable.
> >
> > Hi Derek,
> >
> > What about unprepare() and disable() ?
> >
> > This patch removes the recursion from the enable path but keeps it for the
> > disable path ... this is very odd. Assuming doing so works, It certainly makes
> > CCF a lot harder to understand.
> >
> > What about clock protection which essentially works on the same model as prepare
> > and enable ?
> >
> > Overall, this change does not look like something that should be merged as it
> > is. If you were just seeking comments, you should add the "RFC" tag to your
> > series.
> >
> > Jerome.
> >
> > >
> > > Cc: Jerome Brunet <jbrunet@baylibre.com>
> >
> > If you don't mind, I would prefer to get the whole series next time. It helps to
> > get the context.
> >
> > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > >  drivers/clk/clk.c | 113 ++++++++++++++++++++++++++--------------------
> > >  1 file changed, 64 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index af011974d4ec..95d818f5edb2 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -71,6 +71,8 @@ struct clk_core {
> > >       struct hlist_head       children;
> > >       struct hlist_node       child_node;
> > >       struct hlist_head       clks;
> > > +     struct list_head        prepare_list;
> > > +     struct list_head        enable_list;
> > >       unsigned int            notifier_count;
> > >  #ifdef CONFIG_DEBUG_FS
> > >       struct dentry           *dentry;
> > > @@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
> > >  static int clk_core_prepare(struct clk_core *core)
> > >  {
> > >       int ret = 0;
> > > +     struct clk_core *tmp, *parent;
> > > +     LIST_HEAD(head);
> > >
> > >       lockdep_assert_held(&prepare_lock);
> > >
> > > -     if (!core)
> > > -             return 0;
> > > +     while (core) {
> > > +             list_add(&core->prepare_list, &head);
> > > +             /* Stop once we see a clk that is already prepared */
> > > +             if (core->prepare_count)
> > > +                     break;
> > > +             core = core->parent;
> > > +     }
> > >
> > > -     if (core->prepare_count == 0) {
> > > -             ret = clk_pm_runtime_get(core);
> > > -             if (ret)
> > > -                     return ret;
> > > +     list_for_each_entry_safe(core, tmp, &head, prepare_list) {
> > > +             list_del_init(&core->prepare_list);
> >
> > Is there any point in removing it from the list ?
> > Maybe I missed it but it does not seems useful.
> >
> > Without this, we could use list_for_each_entry()
> >
> > >
> > > -             ret = clk_core_prepare(core->parent);
> > > -             if (ret)
> > > -                     goto runtime_put;
> > > +             if (core->prepare_count == 0) {
> >
> > Should we really check the count here ? You are not checking the count when the
> > put() counterpart is called below.
>
> I think I accidentally messed that up when I picked up the patch.
> There were some merge conflicts with the addition of the
> clk_pm_runtime code.

Nevermind, this is incorrect. The clk_pm_runtime_put is within this if
statement too, so there isn't an issue here.

>
> >
> > Since PM runtime has ref counting as well, either way would work I guess ... but
> > we shall be consistent
> >
> > > +                     ret = clk_pm_runtime_get(core);
> > > +                     if (ret)
> > > +                             goto err;
> > >
> > > -             trace_clk_prepare(core);
> > > +                     trace_clk_prepare(core);
> > >
> > > -             if (core->ops->prepare)
> > > -                     ret = core->ops->prepare(core->hw);
> > > +                     if (core->ops->prepare)
> > > +                             ret = core->ops->prepare(core->hw);
> > >
> > > -             trace_clk_prepare_complete(core);
> > > +                     trace_clk_prepare_complete(core);
> > >
> > > -             if (ret)
> > > -                     goto unprepare;
> > > +                     if (ret) {
> > > +                             clk_pm_runtime_put(core);
> > > +                             goto err;
> > > +                     }
> > > +             }
> > > +             core->prepare_count++;
> > >       }
> > >
> > > -     core->prepare_count++;
> > > -
> > > -     /*
> > > -      * CLK_SET_RATE_GATE is a special case of clock protection
> > > -      * Instead of a consumer claiming exclusive rate control, it is
> > > -      * actually the provider which prevents any consumer from making any
> > > -      * operation which could result in a rate change or rate glitch while
> > > -      * the clock is prepared.
> > > -      */
> > > -     if (core->flags & CLK_SET_RATE_GATE)
> > > -             clk_core_rate_protect(core);
> >
> > This gets removed without anything replacing it.
> >
> > is CLK_SET_RATE_GATE and clock protection support dropped after this change ?
>
> No, I think I just accidentally removed this when resolving conflicts.
>
> >
> > > -
> > >       return 0;
> > > -unprepare:
> > > -     clk_core_unprepare(core->parent);
> > > -runtime_put:
> > > -     clk_pm_runtime_put(core);
> > > +err:
> > > +     parent = core->parent;
> > > +     list_for_each_entry_safe_continue(core, tmp, &head, prepare_list)
> > > +             list_del_init(&core->prepare_list);
> > > +     clk_core_unprepare(parent);
> >
> > If you get here because of failure clk_pm_runtime_get(), you will unprepare a
> > clock which may have not been prepared first
> >
> > Overall the rework of error exit path does not seem right (or necessary)
> >
>
> Yeah, all of this seems to just be a poor resolution of patch
> conflicts on my part. Will fix.

Nevermind, that's not the case. We add the first core that has a
non-zero prepare_count to the first (or we go all the way to root).
That core can't encounter an error since those only happen in the
prepare_count == 0 case. If it's NULL, clk_core_unprepare just
returns.

>
> > >       return ret;
> > >  }
> > >
> > > @@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable);
> > >  static int clk_core_enable(struct clk_core *core)
> > >  {
> > >       int ret = 0;
> > > +     struct clk_core *tmp, *parent;
> > > +     LIST_HEAD(head);
> > >
> > >       lockdep_assert_held(&enable_lock);
> > >
> > > -     if (!core)
> > > -             return 0;
> > > -
> > > -     if (WARN(core->prepare_count == 0,
> > > -         "Enabling unprepared %s\n", core->name))
> > > -             return -ESHUTDOWN;
> > > +     while (core) {
> > > +             list_add(&core->enable_list, &head);
> > > +             /* Stop once we see a clk that is already enabled */
> > > +             if (core->enable_count)
> > > +                     break;
> > > +             core = core->parent;
> > > +     }
> > >
> > > -     if (core->enable_count == 0) {
> > > -             ret = clk_core_enable(core->parent);
> > > +     list_for_each_entry_safe(core, tmp, &head, enable_list) {
> > > +             list_del_init(&core->enable_list);
> > >
> > > -             if (ret)
> > > -                     return ret;
> > > +             if (WARN_ON(core->prepare_count == 0)) {
> > > +                     ret = -ESHUTDOWN;
> > > +                     goto err;
> > > +             }
> > >
> > > -             trace_clk_enable_rcuidle(core);
> > > +             if (core->enable_count == 0) {
> > > +                     trace_clk_enable_rcuidle(core);
> > >
> > > -             if (core->ops->enable)
> > > -                     ret = core->ops->enable(core->hw);
> > > +                     if (core->ops->enable)
> > > +                             ret = core->ops->enable(core->hw);
> > >
> > > -             trace_clk_enable_complete_rcuidle(core);
> > > +                     trace_clk_enable_complete_rcuidle(core);
> > >
> > > -             if (ret) {
> > > -                     clk_core_disable(core->parent);
> > > -                     return ret;
> > > +                     if (ret)
> > > +                             goto err;
> > >               }
> > > +
> > > +             core->enable_count++;
> > >       }
> > >
> > > -     core->enable_count++;
> > >       return 0;
> > > +err:
> > > +     parent = core->parent;
> > > +     list_for_each_entry_safe_continue(core, tmp, &head, enable_list)
> > > +             list_del_init(&core->enable_list);
> > > +     clk_core_disable(parent);
> > > +     return ret;
> > >  }
> > >
> > >  static int clk_core_enable_lock(struct clk_core *core)
> > > @@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> > >       core->num_parents = hw->init->num_parents;
> > >       core->min_rate = 0;
> > >       core->max_rate = ULONG_MAX;
> > > +     INIT_LIST_HEAD(&core->prepare_list);
> > > +     INIT_LIST_HEAD(&core->enable_list);
> > >       hw->core = core;
> > >
> > >       /* allocate local copy in case parent_names is __initdata */
> >
> >

Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[Jerome Brunet]

On Wed, 2018-10-24 at 13:50 -0700, dbasehore . wrote:
> On Wed, Oct 24, 2018 at 1:15 PM dbasehore . <dbasehore@chromium.org> wrote:
> > 
> > On Wed, Oct 24, 2018 at 2:51 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > 
> > > On Tue, 2018-10-23 at 18:31 -0700, Derek Basehore wrote:
> > > > From: Stephen Boyd <sboyd@codeaurora.org>
> > > > 
> > > > Enabling and preparing clocks can be written quite naturally with
> > > > recursion. We start at some point in the tree and recurse up the
> > > > tree to find the oldest parent clk that needs to be enabled or
> > > > prepared. Then we enable/prepare and return to the caller, going
> > > > back to the clk we started at and enabling/preparing along the
> > > > way.
> > > > 
> > > > The problem is recursion isn't great for kernel code where we
> > > > have a limited stack size. Furthermore, we may be calling this
> > > > code inside clk_set_rate() which also has recursion in it, so
> > > > we're really not looking good if we encounter a tall clk tree.
> > > > 
> > > > Let's create a stack instead by looping over the parent chain and
> > > > collecting clks of interest. Then the enable/prepare becomes as
> > > > simple as iterating over that list and calling enable.
> > > 
> > > Hi Derek,
> > > 
> > > What about unprepare() and disable() ?
> > > 
> > > This patch removes the recursion from the enable path but keeps it for the
> > > disable path ... this is very odd. Assuming doing so works, It certainly makes
> > > CCF a lot harder to understand.
> > > 
> > > What about clock protection which essentially works on the same model as prepare
> > > and enable ?
> > > 
> > > Overall, this change does not look like something that should be merged as it
> > > is. If you were just seeking comments, you should add the "RFC" tag to your
> > > series.
> > > 
> > > Jerome.
> > > 
> > > > 
> > > > Cc: Jerome Brunet <jbrunet@baylibre.com>
> > > 
> > > If you don't mind, I would prefer to get the whole series next time. It helps to
> > > get the context.
> > > 
> > > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > > ---
> > > >  drivers/clk/clk.c | 113 ++++++++++++++++++++++++++--------------------
> > > >  1 file changed, 64 insertions(+), 49 deletions(-)
> > > > 
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index af011974d4ec..95d818f5edb2 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -71,6 +71,8 @@ struct clk_core {
> > > >       struct hlist_head       children;
> > > >       struct hlist_node       child_node;
> > > >       struct hlist_head       clks;
> > > > +     struct list_head        prepare_list;
> > > > +     struct list_head        enable_list;
> > > >       unsigned int            notifier_count;
> > > >  #ifdef CONFIG_DEBUG_FS
> > > >       struct dentry           *dentry;
> > > > @@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
> > > >  static int clk_core_prepare(struct clk_core *core)
> > > >  {
> > > >       int ret = 0;
> > > > +     struct clk_core *tmp, *parent;
> > > > +     LIST_HEAD(head);
> > > > 
> > > >       lockdep_assert_held(&prepare_lock);
> > > > 
> > > > -     if (!core)
> > > > -             return 0;
> > > > +     while (core) {
> > > > +             list_add(&core->prepare_list, &head);
> > > > +             /* Stop once we see a clk that is already prepared */
> > > > +             if (core->prepare_count)
> > > > +                     break;
> > > > +             core = core->parent;
> > > > +     }
> > > > 
> > > > -     if (core->prepare_count == 0) {
> > > > -             ret = clk_pm_runtime_get(core);
> > > > -             if (ret)
> > > > -                     return ret;
> > > > +     list_for_each_entry_safe(core, tmp, &head, prepare_list) {
> > > > +             list_del_init(&core->prepare_list);
> > > 
> > > Is there any point in removing it from the list ?
> > > Maybe I missed it but it does not seems useful.
> > > 
> > > Without this, we could use list_for_each_entry()
> > > 
> > > > 
> > > > -             ret = clk_core_prepare(core->parent);
> > > > -             if (ret)
> > > > -                     goto runtime_put;
> > > > +             if (core->prepare_count == 0) {
> > > 
> > > Should we really check the count here ? You are not checking the count when the
> > > put() counterpart is called below.
> > 
> > I think I accidentally messed that up when I picked up the patch.
> > There were some merge conflicts with the addition of the
> > clk_pm_runtime code.
> 
> Nevermind, this is incorrect. The clk_pm_runtime_put is within this if
> statement too, so there isn't an issue here.
> 
> > 
> > > 
> > > Since PM runtime has ref counting as well, either way would work I guess ... but
> > > we shall be consistent
> > > 
> > > > +                     ret = clk_pm_runtime_get(core);
> > > > +                     if (ret)
> > > > +                             goto err;
> > > > 
> > > > -             trace_clk_prepare(core);
> > > > +                     trace_clk_prepare(core);
> > > > 
> > > > -             if (core->ops->prepare)
> > > > -                     ret = core->ops->prepare(core->hw);
> > > > +                     if (core->ops->prepare)
> > > > +                             ret = core->ops->prepare(core->hw);
> > > > 
> > > > -             trace_clk_prepare_complete(core);
> > > > +                     trace_clk_prepare_complete(core);
> > > > 
> > > > -             if (ret)
> > > > -                     goto unprepare;
> > > > +                     if (ret) {
> > > > +                             clk_pm_runtime_put(core);
> > > > +                             goto err;
> > > > +                     }
> > > > +             }
> > > > +             core->prepare_count++;
> > > >       }
> > > > 
> > > > -     core->prepare_count++;
> > > > -
> > > > -     /*
> > > > -      * CLK_SET_RATE_GATE is a special case of clock protection
> > > > -      * Instead of a consumer claiming exclusive rate control, it is
> > > > -      * actually the provider which prevents any consumer from making any
> > > > -      * operation which could result in a rate change or rate glitch while
> > > > -      * the clock is prepared.
> > > > -      */
> > > > -     if (core->flags & CLK_SET_RATE_GATE)
> > > > -             clk_core_rate_protect(core);
> > > 
> > > This gets removed without anything replacing it.
> > > 
> > > is CLK_SET_RATE_GATE and clock protection support dropped after this change ?
> > 
> > No, I think I just accidentally removed this when resolving conflicts.
> > 
> > > 
> > > > -
> > > >       return 0;
> > > > -unprepare:
> > > > -     clk_core_unprepare(core->parent);
> > > > -runtime_put:
> > > > -     clk_pm_runtime_put(core);
> > > > +err:
> > > > +     parent = core->parent;
> > > > +     list_for_each_entry_safe_continue(core, tmp, &head, prepare_list)
> > > > +             list_del_init(&core->prepare_list);
> > > > +     clk_core_unprepare(parent);
> > > 
> > > If you get here because of failure clk_pm_runtime_get(), you will unprepare a
> > > clock which may have not been prepared first
> > > 
> > > Overall the rework of error exit path does not seem right (or necessary)
> > > 
> > 
> > Yeah, all of this seems to just be a poor resolution of patch
> > conflicts on my part. Will fix.
> 
> Nevermind, that's not the case. We add the first core that has a
> non-zero prepare_count to the first (or we go all the way to root).
> That core can't encounter an error since those only happen in the
> prepare_count == 0 case. If it's NULL, clk_core_unprepare just
> returns.

Indeed, the diff is bit hard to follow and I got confused. With th patch
applied, things are more clear. Sorry about that

While correct, this code could simplified a bit

* unless the prepare_list is used anywhere without starting with a list_add(),
reseting the list pointer is not necessary. It should be possible to remove the
list_del_init(). the 'err' label becomes just 'clk_core_unprepare(core->parent)'

* rolling back change under 'if' or 'goto label' are both fine IMO. It would be
easier to follow if only one method was used inside a single function, though.


> 
> > 
> > > >       return ret;
> > > >  }
> > > > 
> > > > @@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable);
> > > >  static int clk_core_enable(struct clk_core *core)
> > > >  {
> > > >       int ret = 0;
> > > > +     struct clk_core *tmp, *parent;
> > > > +     LIST_HEAD(head);
> > > > 
> > > >       lockdep_assert_held(&enable_lock);
> > > > 
> > > > -     if (!core)
> > > > -             return 0;
> > > > -
> > > > -     if (WARN(core->prepare_count == 0,
> > > > -         "Enabling unprepared %s\n", core->name))
> > > > -             return -ESHUTDOWN;
> > > > +     while (core) {
> > > > +             list_add(&core->enable_list, &head);
> > > > +             /* Stop once we see a clk that is already enabled */
> > > > +             if (core->enable_count)
> > > > +                     break;
> > > > +             core = core->parent;
> > > > +     }
> > > > 
> > > > -     if (core->enable_count == 0) {
> > > > -             ret = clk_core_enable(core->parent);
> > > > +     list_for_each_entry_safe(core, tmp, &head, enable_list) {
> > > > +             list_del_init(&core->enable_list);
> > > > 
> > > > -             if (ret)
> > > > -                     return ret;
> > > > +             if (WARN_ON(core->prepare_count == 0)) {
> > > > +                     ret = -ESHUTDOWN;
> > > > +                     goto err;
> > > > +             }
> > > > 
> > > > -             trace_clk_enable_rcuidle(core);
> > > > +             if (core->enable_count == 0) {
> > > > +                     trace_clk_enable_rcuidle(core);
> > > > 
> > > > -             if (core->ops->enable)
> > > > -                     ret = core->ops->enable(core->hw);
> > > > +                     if (core->ops->enable)
> > > > +                             ret = core->ops->enable(core->hw);
> > > > 
> > > > -             trace_clk_enable_complete_rcuidle(core);
> > > > +                     trace_clk_enable_complete_rcuidle(core);
> > > > 
> > > > -             if (ret) {
> > > > -                     clk_core_disable(core->parent);
> > > > -                     return ret;
> > > > +                     if (ret)
> > > > +                             goto err;
> > > >               }
> > > > +
> > > > +             core->enable_count++;
> > > >       }
> > > > 
> > > > -     core->enable_count++;
> > > >       return 0;
> > > > +err:
> > > > +     parent = core->parent;
> > > > +     list_for_each_entry_safe_continue(core, tmp, &head, enable_list)
> > > > +             list_del_init(&core->enable_list);
> > > > +     clk_core_disable(parent);
> > > > +     return ret;
> > > >  }
> > > > 
> > > >  static int clk_core_enable_lock(struct clk_core *core)
> > > > @@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> > > >       core->num_parents = hw->init->num_parents;
> > > >       core->min_rate = 0;
> > > >       core->max_rate = ULONG_MAX;
> > > > +     INIT_LIST_HEAD(&core->prepare_list);
> > > > +     INIT_LIST_HEAD(&core->enable_list);
> > > >       hw->core = core;
> > > > 
> > > >       /* allocate local copy in case parent_names is __initdata */
> > > 
> > > 

Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[dbasehore .]

On Wed, Oct 24, 2018 at 2:51 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On Tue, 2018-10-23 at 18:31 -0700, Derek Basehore wrote:
> > From: Stephen Boyd <sboyd@codeaurora.org>
> >
> > Enabling and preparing clocks can be written quite naturally with
> > recursion. We start at some point in the tree and recurse up the
> > tree to find the oldest parent clk that needs to be enabled or
> > prepared. Then we enable/prepare and return to the caller, going
> > back to the clk we started at and enabling/preparing along the
> > way.
> >
> > The problem is recursion isn't great for kernel code where we
> > have a limited stack size. Furthermore, we may be calling this
> > code inside clk_set_rate() which also has recursion in it, so
> > we're really not looking good if we encounter a tall clk tree.
> >
> > Let's create a stack instead by looping over the parent chain and
> > collecting clks of interest. Then the enable/prepare becomes as
> > simple as iterating over that list and calling enable.
>
> Hi Derek,
>
> What about unprepare() and disable() ?
>
> This patch removes the recursion from the enable path but keeps it for the
> disable path ... this is very odd. Assuming doing so works, It certainly makes
> CCF a lot harder to understand.

It wasn't there in the original patch. I asked Stephen, and he thinks
it may have been left that way because unprepare/disable are tail
recursion cases, which the compiler can optimize away.

>
> What about clock protection which essentially works on the same model as prepare
> and enable ?
>
> Overall, this change does not look like something that should be merged as it
> is. If you were just seeking comments, you should add the "RFC" tag to your
> series.
>
> Jerome.
>
> >
> > Cc: Jerome Brunet <jbrunet@baylibre.com>
>
> If you don't mind, I would prefer to get the whole series next time. It helps to
> get the context.
>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  drivers/clk/clk.c | 113 ++++++++++++++++++++++++++--------------------
> >  1 file changed, 64 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index af011974d4ec..95d818f5edb2 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -71,6 +71,8 @@ struct clk_core {
> >       struct hlist_head       children;
> >       struct hlist_node       child_node;
> >       struct hlist_head       clks;
> > +     struct list_head        prepare_list;
> > +     struct list_head        enable_list;
> >       unsigned int            notifier_count;
> >  #ifdef CONFIG_DEBUG_FS
> >       struct dentry           *dentry;
> > @@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
> >  static int clk_core_prepare(struct clk_core *core)
> >  {
> >       int ret = 0;
> > +     struct clk_core *tmp, *parent;
> > +     LIST_HEAD(head);
> >
> >       lockdep_assert_held(&prepare_lock);
> >
> > -     if (!core)
> > -             return 0;
> > +     while (core) {
> > +             list_add(&core->prepare_list, &head);
> > +             /* Stop once we see a clk that is already prepared */
> > +             if (core->prepare_count)
> > +                     break;
> > +             core = core->parent;
> > +     }
> >
> > -     if (core->prepare_count == 0) {
> > -             ret = clk_pm_runtime_get(core);
> > -             if (ret)
> > -                     return ret;
> > +     list_for_each_entry_safe(core, tmp, &head, prepare_list) {
> > +             list_del_init(&core->prepare_list);
>
> Is there any point in removing it from the list ?
> Maybe I missed it but it does not seems useful.
>
> Without this, we could use list_for_each_entry()
>
> >
> > -             ret = clk_core_prepare(core->parent);
> > -             if (ret)
> > -                     goto runtime_put;
> > +             if (core->prepare_count == 0) {
>
> Should we really check the count here ? You are not checking the count when the
> put() counterpart is called below.
>
> Since PM runtime has ref counting as well, either way would work I guess ... but
> we shall be consistent
>
> > +                     ret = clk_pm_runtime_get(core);
> > +                     if (ret)
> > +                             goto err;
> >
> > -             trace_clk_prepare(core);
> > +                     trace_clk_prepare(core);
> >
> > -             if (core->ops->prepare)
> > -                     ret = core->ops->prepare(core->hw);
> > +                     if (core->ops->prepare)
> > +                             ret = core->ops->prepare(core->hw);
> >
> > -             trace_clk_prepare_complete(core);
> > +                     trace_clk_prepare_complete(core);
> >
> > -             if (ret)
> > -                     goto unprepare;
> > +                     if (ret) {
> > +                             clk_pm_runtime_put(core);
> > +                             goto err;
> > +                     }
> > +             }
> > +             core->prepare_count++;
> >       }
> >
> > -     core->prepare_count++;
> > -
> > -     /*
> > -      * CLK_SET_RATE_GATE is a special case of clock protection
> > -      * Instead of a consumer claiming exclusive rate control, it is
> > -      * actually the provider which prevents any consumer from making any
> > -      * operation which could result in a rate change or rate glitch while
> > -      * the clock is prepared.
> > -      */
> > -     if (core->flags & CLK_SET_RATE_GATE)
> > -             clk_core_rate_protect(core);
>
> This gets removed without anything replacing it.
>
> is CLK_SET_RATE_GATE and clock protection support dropped after this change ?
>
> > -
> >       return 0;
> > -unprepare:
> > -     clk_core_unprepare(core->parent);
> > -runtime_put:
> > -     clk_pm_runtime_put(core);
> > +err:
> > +     parent = core->parent;
> > +     list_for_each_entry_safe_continue(core, tmp, &head, prepare_list)
> > +             list_del_init(&core->prepare_list);
> > +     clk_core_unprepare(parent);
>
> If you get here because of failure clk_pm_runtime_get(), you will unprepare a
> clock which may have not been prepared first
>
> Overall the rework of error exit path does not seem right (or necessary)
>
> >       return ret;
> >  }
> >
> > @@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable);
> >  static int clk_core_enable(struct clk_core *core)
> >  {
> >       int ret = 0;
> > +     struct clk_core *tmp, *parent;
> > +     LIST_HEAD(head);
> >
> >       lockdep_assert_held(&enable_lock);
> >
> > -     if (!core)
> > -             return 0;
> > -
> > -     if (WARN(core->prepare_count == 0,
> > -         "Enabling unprepared %s\n", core->name))
> > -             return -ESHUTDOWN;
> > +     while (core) {
> > +             list_add(&core->enable_list, &head);
> > +             /* Stop once we see a clk that is already enabled */
> > +             if (core->enable_count)
> > +                     break;
> > +             core = core->parent;
> > +     }
> >
> > -     if (core->enable_count == 0) {
> > -             ret = clk_core_enable(core->parent);
> > +     list_for_each_entry_safe(core, tmp, &head, enable_list) {
> > +             list_del_init(&core->enable_list);
> >
> > -             if (ret)
> > -                     return ret;
> > +             if (WARN_ON(core->prepare_count == 0)) {
> > +                     ret = -ESHUTDOWN;
> > +                     goto err;
> > +             }
> >
> > -             trace_clk_enable_rcuidle(core);
> > +             if (core->enable_count == 0) {
> > +                     trace_clk_enable_rcuidle(core);
> >
> > -             if (core->ops->enable)
> > -                     ret = core->ops->enable(core->hw);
> > +                     if (core->ops->enable)
> > +                             ret = core->ops->enable(core->hw);
> >
> > -             trace_clk_enable_complete_rcuidle(core);
> > +                     trace_clk_enable_complete_rcuidle(core);
> >
> > -             if (ret) {
> > -                     clk_core_disable(core->parent);
> > -                     return ret;
> > +                     if (ret)
> > +                             goto err;
> >               }
> > +
> > +             core->enable_count++;
> >       }
> >
> > -     core->enable_count++;
> >       return 0;
> > +err:
> > +     parent = core->parent;
> > +     list_for_each_entry_safe_continue(core, tmp, &head, enable_list)
> > +             list_del_init(&core->enable_list);
> > +     clk_core_disable(parent);
> > +     return ret;
> >  }
> >
> >  static int clk_core_enable_lock(struct clk_core *core)
> > @@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> >       core->num_parents = hw->init->num_parents;
> >       core->min_rate = 0;
> >       core->max_rate = ULONG_MAX;
> > +     INIT_LIST_HEAD(&core->prepare_list);
> > +     INIT_LIST_HEAD(&core->enable_list);
> >       hw->core = core;
> >
> >       /* allocate local copy in case parent_names is __initdata */
>
>

Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[Jerome Brunet]

On Wed, 2018-10-24 at 13:36 -0700, dbasehore . wrote:
> On Wed, Oct 24, 2018 at 2:51 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > 
> > On Tue, 2018-10-23 at 18:31 -0700, Derek Basehore wrote:
> > > From: Stephen Boyd <sboyd@codeaurora.org>
> > > 
> > > Enabling and preparing clocks can be written quite naturally with
> > > recursion. We start at some point in the tree and recurse up the
> > > tree to find the oldest parent clk that needs to be enabled or
> > > prepared. Then we enable/prepare and return to the caller, going
> > > back to the clk we started at and enabling/preparing along the
> > > way.
> > > 
> > > The problem is recursion isn't great for kernel code where we
> > > have a limited stack size. Furthermore, we may be calling this
> > > code inside clk_set_rate() which also has recursion in it, so
> > > we're really not looking good if we encounter a tall clk tree.
> > > 
> > > Let's create a stack instead by looping over the parent chain and
> > > collecting clks of interest. Then the enable/prepare becomes as
> > > simple as iterating over that list and calling enable.
> > 
> > Hi Derek,
> > 
> > What about unprepare() and disable() ?
> > 
> > This patch removes the recursion from the enable path but keeps it for the
> > disable path ... this is very odd. Assuming doing so works, It certainly makes
> > CCF a lot harder to understand.
> 
> It wasn't there in the original patch. I asked Stephen, and he thinks
> it may have been left that way because unprepare/disable are tail
> recursion cases, which the compiler can optimize away.

I'm sure the compiler is very clever ;) My point is more about maintainability
and how easy it is to understand for human beings.

> 
> > 
> > What about clock protection which essentially works on the same model as prepare
> > and enable ?
> > 
> > Overall, this change does not look like something that should be merged as it
> > is. If you were just seeking comments, you should add the "RFC" tag to your
> > series.
> > 
> > Jerome.
> > 
> > > 
> > > Cc: Jerome Brunet <jbrunet@baylibre.com>
> > 
> > If you don't mind, I would prefer to get the whole series next time. It helps to
> > get the context.
> > 
> > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > >  drivers/clk/clk.c | 113 ++++++++++++++++++++++++++--------------------
> > >  1 file changed, 64 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index af011974d4ec..95d818f5edb2 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -71,6 +71,8 @@ struct clk_core {
> > >       struct hlist_head       children;
> > >       struct hlist_node       child_node;
> > >       struct hlist_head       clks;
> > > +     struct list_head        prepare_list;
> > > +     struct list_head        enable_list;
> > >       unsigned int            notifier_count;
> > >  #ifdef CONFIG_DEBUG_FS
> > >       struct dentry           *dentry;
> > > @@ -740,49 +742,48 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
> > >  static int clk_core_prepare(struct clk_core *core)
> > >  {
> > >       int ret = 0;
> > > +     struct clk_core *tmp, *parent;
> > > +     LIST_HEAD(head);
> > > 
> > >       lockdep_assert_held(&prepare_lock);
> > > 
> > > -     if (!core)
> > > -             return 0;
> > > +     while (core) {
> > > +             list_add(&core->prepare_list, &head);
> > > +             /* Stop once we see a clk that is already prepared */
> > > +             if (core->prepare_count)
> > > +                     break;
> > > +             core = core->parent;
> > > +     }
> > > 
> > > -     if (core->prepare_count == 0) {
> > > -             ret = clk_pm_runtime_get(core);
> > > -             if (ret)
> > > -                     return ret;
> > > +     list_for_each_entry_safe(core, tmp, &head, prepare_list) {
> > > +             list_del_init(&core->prepare_list);
> > 
> > Is there any point in removing it from the list ?
> > Maybe I missed it but it does not seems useful.
> > 
> > Without this, we could use list_for_each_entry()
> > 
> > > 
> > > -             ret = clk_core_prepare(core->parent);
> > > -             if (ret)
> > > -                     goto runtime_put;
> > > +             if (core->prepare_count == 0) {
> > 
> > Should we really check the count here ? You are not checking the count when the
> > put() counterpart is called below.
> > 
> > Since PM runtime has ref counting as well, either way would work I guess ... but
> > we shall be consistent
> > 
> > > +                     ret = clk_pm_runtime_get(core);
> > > +                     if (ret)
> > > +                             goto err;
> > > 
> > > -             trace_clk_prepare(core);
> > > +                     trace_clk_prepare(core);
> > > 
> > > -             if (core->ops->prepare)
> > > -                     ret = core->ops->prepare(core->hw);
> > > +                     if (core->ops->prepare)
> > > +                             ret = core->ops->prepare(core->hw);
> > > 
> > > -             trace_clk_prepare_complete(core);
> > > +                     trace_clk_prepare_complete(core);
> > > 
> > > -             if (ret)
> > > -                     goto unprepare;
> > > +                     if (ret) {
> > > +                             clk_pm_runtime_put(core);
> > > +                             goto err;
> > > +                     }
> > > +             }
> > > +             core->prepare_count++;
> > >       }
> > > 
> > > -     core->prepare_count++;
> > > -
> > > -     /*
> > > -      * CLK_SET_RATE_GATE is a special case of clock protection
> > > -      * Instead of a consumer claiming exclusive rate control, it is
> > > -      * actually the provider which prevents any consumer from making any
> > > -      * operation which could result in a rate change or rate glitch while
> > > -      * the clock is prepared.
> > > -      */
> > > -     if (core->flags & CLK_SET_RATE_GATE)
> > > -             clk_core_rate_protect(core);
> > 
> > This gets removed without anything replacing it.
> > 
> > is CLK_SET_RATE_GATE and clock protection support dropped after this change ?
> > 
> > > -
> > >       return 0;
> > > -unprepare:
> > > -     clk_core_unprepare(core->parent);
> > > -runtime_put:
> > > -     clk_pm_runtime_put(core);
> > > +err:
> > > +     parent = core->parent;
> > > +     list_for_each_entry_safe_continue(core, tmp, &head, prepare_list)
> > > +             list_del_init(&core->prepare_list);
> > > +     clk_core_unprepare(parent);
> > 
> > If you get here because of failure clk_pm_runtime_get(), you will unprepare a
> > clock which may have not been prepared first
> > 
> > Overall the rework of error exit path does not seem right (or necessary)
> > 
> > >       return ret;
> > >  }
> > > 
> > > @@ -878,37 +879,49 @@ EXPORT_SYMBOL_GPL(clk_disable);
> > >  static int clk_core_enable(struct clk_core *core)
> > >  {
> > >       int ret = 0;
> > > +     struct clk_core *tmp, *parent;
> > > +     LIST_HEAD(head);
> > > 
> > >       lockdep_assert_held(&enable_lock);
> > > 
> > > -     if (!core)
> > > -             return 0;
> > > -
> > > -     if (WARN(core->prepare_count == 0,
> > > -         "Enabling unprepared %s\n", core->name))
> > > -             return -ESHUTDOWN;
> > > +     while (core) {
> > > +             list_add(&core->enable_list, &head);
> > > +             /* Stop once we see a clk that is already enabled */
> > > +             if (core->enable_count)
> > > +                     break;
> > > +             core = core->parent;
> > > +     }
> > > 
> > > -     if (core->enable_count == 0) {
> > > -             ret = clk_core_enable(core->parent);
> > > +     list_for_each_entry_safe(core, tmp, &head, enable_list) {
> > > +             list_del_init(&core->enable_list);
> > > 
> > > -             if (ret)
> > > -                     return ret;
> > > +             if (WARN_ON(core->prepare_count == 0)) {
> > > +                     ret = -ESHUTDOWN;
> > > +                     goto err;
> > > +             }
> > > 
> > > -             trace_clk_enable_rcuidle(core);
> > > +             if (core->enable_count == 0) {
> > > +                     trace_clk_enable_rcuidle(core);
> > > 
> > > -             if (core->ops->enable)
> > > -                     ret = core->ops->enable(core->hw);
> > > +                     if (core->ops->enable)
> > > +                             ret = core->ops->enable(core->hw);
> > > 
> > > -             trace_clk_enable_complete_rcuidle(core);
> > > +                     trace_clk_enable_complete_rcuidle(core);
> > > 
> > > -             if (ret) {
> > > -                     clk_core_disable(core->parent);
> > > -                     return ret;
> > > +                     if (ret)
> > > +                             goto err;
> > >               }
> > > +
> > > +             core->enable_count++;
> > >       }
> > > 
> > > -     core->enable_count++;
> > >       return 0;
> > > +err:
> > > +     parent = core->parent;
> > > +     list_for_each_entry_safe_continue(core, tmp, &head, enable_list)
> > > +             list_del_init(&core->enable_list);
> > > +     clk_core_disable(parent);
> > > +     return ret;
> > >  }
> > > 
> > >  static int clk_core_enable_lock(struct clk_core *core)
> > > @@ -3281,6 +3294,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> > >       core->num_parents = hw->init->num_parents;
> > >       core->min_rate = 0;
> > >       core->max_rate = ULONG_MAX;
> > > +     INIT_LIST_HEAD(&core->prepare_list);
> > > +     INIT_LIST_HEAD(&core->enable_list);
> > >       hw->core = core;
> > > 
> > >       /* allocate local copy in case parent_names is __initdata */
> > 
> > 

Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[Stephen Boyd]

Quoting Derek Basehore (2018-10-23 18:31:27)
> From: Stephen Boyd <sboyd@codeaurora.org>
> 
> Enabling and preparing clocks can be written quite naturally with
> recursion. We start at some point in the tree and recurse up the
> tree to find the oldest parent clk that needs to be enabled or
> prepared. Then we enable/prepare and return to the caller, going
> back to the clk we started at and enabling/preparing along the
> way.
> 
> The problem is recursion isn't great for kernel code where we
> have a limited stack size. Furthermore, we may be calling this
> code inside clk_set_rate() which also has recursion in it, so
> we're really not looking good if we encounter a tall clk tree.
> 
> Let's create a stack instead by looping over the parent chain and
> collecting clks of interest. Then the enable/prepare becomes as
> simple as iterating over that list and calling enable.
> 
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>

Did you change anything substantially? Or is it just a resend of my
patch from a while ago? If you can add a link to the original and also
describe what changed in a maintainer tag it would be much easier for me
to compare.

Re: [PATCH 1/6] clk: Remove recursion in clk_core_{prepare,enable}()

[dbasehore .]

On Wed, Oct 24, 2018 at 6:07 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Derek Basehore (2018-10-23 18:31:27)
> > From: Stephen Boyd <sboyd@codeaurora.org>
> >
> > Enabling and preparing clocks can be written quite naturally with
> > recursion. We start at some point in the tree and recurse up the
> > tree to find the oldest parent clk that needs to be enabled or
> > prepared. Then we enable/prepare and return to the caller, going
> > back to the clk we started at and enabling/preparing along the
> > way.
> >
> > The problem is recursion isn't great for kernel code where we
> > have a limited stack size. Furthermore, we may be calling this
> > code inside clk_set_rate() which also has recursion in it, so
> > we're really not looking good if we encounter a tall clk tree.
> >
> > Let's create a stack instead by looping over the parent chain and
> > collecting clks of interest. Then the enable/prepare becomes as
> > simple as iterating over that list and calling enable.
> >
> > Cc: Jerome Brunet <jbrunet@baylibre.com>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>
> Did you change anything substantially? Or is it just a resend of my
> patch from a while ago? If you can add a link to the original and also
> describe what changed in a maintainer tag it would be much easier for me
> to compare.
>

It should just be your patch with the compilation warning fixed.

This is picked up from https://lore.kernel.org/patchwork/patch/814369/

[PATCH 2/6] clk: fix clk_calc_subtree compute duplications

[Derek Basehore]

clk_calc_subtree was called at every step up the clk tree in
clk_calc_new_rates. Since it recursively calls itself for its
children, this means it would be called once on each clk for each
step above the top clk is.

This fixes this by breaking the subtree calculation into two parts.
The first part recalcs the rate for each child of the parent clk in
clk_calc_new_rates. This part is not recursive itself. The second part
recursively calls a new clk_calc_subtree on the clk_core that was
passed into clk_set_rate.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/clk/clk.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 95d818f5edb2..61de8ad3e4cf 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1717,11 +1717,19 @@ static int __clk_speculate_rates(struct clk_core *core,
 	return ret;
 }
 
-static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
-			     struct clk_core *new_parent, u8 p_index)
+static void clk_calc_subtree(struct clk_core *core)
 {
 	struct clk_core *child;
 
+	hlist_for_each_entry(child, &core->children, child_node) {
+		child->new_rate = clk_recalc(child, core->new_rate);
+		clk_calc_subtree(child);
+	}
+}
+
+static void clk_set_change(struct clk_core *core, unsigned long new_rate,
+			   struct clk_core *new_parent, u8 p_index)
+{
 	core->new_rate = new_rate;
 	core->new_parent = new_parent;
 	core->new_parent_index = p_index;
@@ -1729,11 +1737,6 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
 	core->new_child = NULL;
 	if (new_parent && new_parent != core->parent)
 		new_parent->new_child = core;
-
-	hlist_for_each_entry(child, &core->children, child_node) {
-		child->new_rate = clk_recalc(child, new_rate);
-		clk_calc_subtree(child, child->new_rate, NULL, 0);
-	}
 }
 
 /*
@@ -1744,7 +1747,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 					   unsigned long rate)
 {
 	struct clk_core *top = core;
-	struct clk_core *old_parent, *parent;
+	struct clk_core *old_parent, *parent, *child;
 	unsigned long best_parent_rate = 0;
 	unsigned long new_rate;
 	unsigned long min_rate;
@@ -1791,6 +1794,8 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		/* pass-through clock with adjustable parent */
 		top = clk_calc_new_rates(parent, rate);
 		new_rate = parent->new_rate;
+		hlist_for_each_entry(child, &parent->children, child_node)
+			child->new_rate = clk_recalc(child, new_rate);
 		goto out;
 	}
 
@@ -1813,11 +1818,14 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	}
 
 	if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
-	    best_parent_rate != parent->rate)
+	    best_parent_rate != parent->rate) {
 		top = clk_calc_new_rates(parent, best_parent_rate);
+		hlist_for_each_entry(child, &parent->children, child_node)
+			child->new_rate = clk_recalc(child, parent->new_rate);
+	}
 
 out:
-	clk_calc_subtree(core, new_rate, parent, p_index);
+	clk_set_change(core, new_rate, parent, p_index);
 
 	return top;
 }
@@ -2018,6 +2026,8 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (ret)
 		return ret;
 
+	clk_calc_subtree(core);
+
 	/* notify that we are about to change rates */
 	fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
 	if (fail_clk) {
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH 2/6] clk: fix clk_calc_subtree compute duplications

[dbasehore .]

On Tue, Oct 23, 2018 at 6:31 PM Derek Basehore <dbasehore@chromium.org> wrote:
>
> clk_calc_subtree was called at every step up the clk tree in
> clk_calc_new_rates. Since it recursively calls itself for its
> children, this means it would be called once on each clk for each
> step above the top clk is.
>
> This fixes this by breaking the subtree calculation into two parts.
> The first part recalcs the rate for each child of the parent clk in
> clk_calc_new_rates. This part is not recursive itself. The second part
> recursively calls a new clk_calc_subtree on the clk_core that was
> passed into clk_set_rate.
>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/clk/clk.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 95d818f5edb2..61de8ad3e4cf 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1717,11 +1717,19 @@ static int __clk_speculate_rates(struct clk_core *core,
>         return ret;
>  }
>
> -static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
> -                            struct clk_core *new_parent, u8 p_index)
> +static void clk_calc_subtree(struct clk_core *core)
>  {
>         struct clk_core *child;
>
> +       hlist_for_each_entry(child, &core->children, child_node) {
> +               child->new_rate = clk_recalc(child, core->new_rate);
> +               clk_calc_subtree(child);
> +       }
> +}
> +
> +static void clk_set_change(struct clk_core *core, unsigned long new_rate,
> +                          struct clk_core *new_parent, u8 p_index)
> +{
>         core->new_rate = new_rate;
>         core->new_parent = new_parent;
>         core->new_parent_index = p_index;
> @@ -1729,11 +1737,6 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
>         core->new_child = NULL;
>         if (new_parent && new_parent != core->parent)
>                 new_parent->new_child = core;
> -
> -       hlist_for_each_entry(child, &core->children, child_node) {
> -               child->new_rate = clk_recalc(child, new_rate);
> -               clk_calc_subtree(child, child->new_rate, NULL, 0);
> -       }
>  }
>
>  /*
> @@ -1744,7 +1747,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
>                                            unsigned long rate)
>  {
>         struct clk_core *top = core;
> -       struct clk_core *old_parent, *parent;
> +       struct clk_core *old_parent, *parent, *child;
>         unsigned long best_parent_rate = 0;
>         unsigned long new_rate;
>         unsigned long min_rate;
> @@ -1791,6 +1794,8 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
>                 /* pass-through clock with adjustable parent */
>                 top = clk_calc_new_rates(parent, rate);
>                 new_rate = parent->new_rate;
> +               hlist_for_each_entry(child, &parent->children, child_node)
> +                       child->new_rate = clk_recalc(child, new_rate);
>                 goto out;
>         }
>
> @@ -1813,11 +1818,14 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
>         }
>
>         if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
> -           best_parent_rate != parent->rate)
> +           best_parent_rate != parent->rate) {
>                 top = clk_calc_new_rates(parent, best_parent_rate);
> +               hlist_for_each_entry(child, &parent->children, child_node)
> +                       child->new_rate = clk_recalc(child, parent->new_rate);
> +       }
>
>  out:
> -       clk_calc_subtree(core, new_rate, parent, p_index);
> +       clk_set_change(core, new_rate, parent, p_index);
>
>         return top;
>  }
> @@ -2018,6 +2026,8 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>         if (ret)
>                 return ret;
>
> +       clk_calc_subtree(core);

This should be:
hlist_for_each_entry(child, core->new_parent? core->new_parent :
core->parent, child_node)
  clk_calc_subtree(child);

> +
>         /* notify that we are about to change rates */
>         fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
>         if (fail_clk) {
> --
> 2.19.1.568.g152ad8e336-goog
>

[PATCH 3/6] clk: change rates via list iteration

[Derek Basehore]

This changes the clk_set_rate code to use lists instead of recursion.
While making this change, also add error handling for clk_set_rate.
This means that errors in the set_rate/set_parent/set_rate_and_parent
functions will not longer be ignored. When an error occurs, the clk
rates and parents are reset, unless an error occurs here, in which we
bail and cross our fingers.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/clk/clk.c | 225 +++++++++++++++++++++++++++++++---------------
 1 file changed, 153 insertions(+), 72 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 61de8ad3e4cf..1db44b4e46b0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -42,6 +42,13 @@ static LIST_HEAD(clk_notifier_list);
 
 /***    private data structures    ***/
 
+struct clk_change {
+	struct list_head	change_list;
+	unsigned long		rate;
+	struct clk_core		*core;
+	struct clk_core		*parent;
+};
+
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;
@@ -52,11 +59,9 @@ struct clk_core {
 	const char		**parent_names;
 	struct clk_core		**parents;
 	u8			num_parents;
-	u8			new_parent_index;
 	unsigned long		rate;
 	unsigned long		req_rate;
-	unsigned long		new_rate;
-	struct clk_core		*new_parent;
+	struct clk_change	change;
 	struct clk_core		*new_child;
 	unsigned long		flags;
 	bool			orphan;
@@ -1719,20 +1724,53 @@ static int __clk_speculate_rates(struct clk_core *core,
 
 static void clk_calc_subtree(struct clk_core *core)
 {
-	struct clk_core *child;
+	LIST_HEAD(tmp_list);
 
-	hlist_for_each_entry(child, &core->children, child_node) {
-		child->new_rate = clk_recalc(child, core->new_rate);
-		clk_calc_subtree(child);
+	list_add(&core->change.change_list, &tmp_list);
+	while (!list_empty(&tmp_list)) {
+		struct clk_change *change = list_first_entry(&tmp_list,
+				struct clk_change, change_list);
+		struct clk_core *tmp = change->core;
+		struct clk_core *child;
+
+		hlist_for_each_entry(child, &tmp->children, child_node) {
+			child->change.rate = clk_recalc(child,
+					tmp->change.rate);
+			list_add_tail(&child->change.change_list, &tmp_list);
+		}
+
+		list_del_init(&change->change_list);
+	}
+}
+
+static void clk_prepare_changes(struct list_head *change_list,
+				struct clk_core *core)
+{
+	LIST_HEAD(tmp_list);
+
+	list_add(&core->change.change_list, &tmp_list);
+	while (!list_empty(&tmp_list)) {
+		struct clk_change *change = list_first_entry(&tmp_list,
+				struct clk_change, change_list);
+		struct clk_core *tmp = change->core;
+		struct clk_core *child;
+
+		hlist_for_each_entry(child, &tmp->children, child_node)
+			list_add_tail(&child->change.change_list, &tmp_list);
+
+		child = tmp->new_child;
+		if (child)
+			list_add_tail(&child->change.change_list, &tmp_list);
+
+		list_move_tail(&tmp->change.change_list, change_list);
 	}
 }
 
 static void clk_set_change(struct clk_core *core, unsigned long new_rate,
-			   struct clk_core *new_parent, u8 p_index)
+			   struct clk_core *new_parent)
 {
-	core->new_rate = new_rate;
-	core->new_parent = new_parent;
-	core->new_parent_index = p_index;
+	core->change.rate = new_rate;
+	core->change.parent = new_parent;
 	/* include clk in new parent's PRE_RATE_CHANGE notifications */
 	core->new_child = NULL;
 	if (new_parent && new_parent != core->parent)
@@ -1752,7 +1790,6 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	unsigned long new_rate;
 	unsigned long min_rate;
 	unsigned long max_rate;
-	int p_index = 0;
 	long ret;
 
 	/* sanity */
@@ -1788,14 +1825,13 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 			return NULL;
 	} else if (!parent || !(core->flags & CLK_SET_RATE_PARENT)) {
 		/* pass-through clock without adjustable parent */
-		core->new_rate = core->rate;
 		return NULL;
 	} else {
 		/* pass-through clock with adjustable parent */
 		top = clk_calc_new_rates(parent, rate);
-		new_rate = parent->new_rate;
+		new_rate = parent->change.rate;
 		hlist_for_each_entry(child, &parent->children, child_node)
-			child->new_rate = clk_recalc(child, new_rate);
+			child->change.rate = clk_recalc(child, new_rate);
 		goto out;
 	}
 
@@ -1807,25 +1843,16 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		return NULL;
 	}
 
-	/* try finding the new parent index */
-	if (parent && core->num_parents > 1) {
-		p_index = clk_fetch_parent_index(core, parent);
-		if (p_index < 0) {
-			pr_debug("%s: clk %s can not be parent of clk %s\n",
-				 __func__, parent->name, core->name);
-			return NULL;
-		}
-	}
-
 	if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
 	    best_parent_rate != parent->rate) {
 		top = clk_calc_new_rates(parent, best_parent_rate);
 		hlist_for_each_entry(child, &parent->children, child_node)
-			child->new_rate = clk_recalc(child, parent->new_rate);
+			child->change.rate = clk_recalc(child,
+					parent->change.rate);
 	}
 
 out:
-	clk_set_change(core, new_rate, parent, p_index);
+	clk_set_change(core, new_rate, parent);
 
 	return top;
 }
@@ -1841,18 +1868,18 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
 	struct clk_core *child, *tmp_clk, *fail_clk = NULL;
 	int ret = NOTIFY_DONE;
 
-	if (core->rate == core->new_rate)
+	if (core->rate == core->change.rate)
 		return NULL;
 
 	if (core->notifier_count) {
-		ret = __clk_notify(core, event, core->rate, core->new_rate);
+		ret = __clk_notify(core, event, core->rate, core->change.rate);
 		if (ret & NOTIFY_STOP_MASK)
 			fail_clk = core;
 	}
 
 	hlist_for_each_entry(child, &core->children, child_node) {
 		/* Skip children who will be reparented to another clock */
-		if (child->new_parent && child->new_parent != core)
+		if (child->change.parent && child->change.parent != core)
 			continue;
 		tmp_clk = clk_propagate_rate_change(child, event);
 		if (tmp_clk)
@@ -1873,28 +1900,30 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
  * walk down a subtree and set the new rates notifying the rate
  * change on the way
  */
-static void clk_change_rate(struct clk_core *core)
+static int clk_change_rate(struct clk_change *change)
 {
-	struct clk_core *child;
-	struct hlist_node *tmp;
+	struct clk_core *core = change->core;
 	unsigned long old_rate;
 	unsigned long best_parent_rate = 0;
 	bool skip_set_rate = false;
-	struct clk_core *old_parent;
+	struct clk_core *old_parent = NULL;
 	struct clk_core *parent = NULL;
+	int p_index;
+	int ret = 0;
 
 	old_rate = core->rate;
 
-	if (core->new_parent) {
-		parent = core->new_parent;
-		best_parent_rate = core->new_parent->rate;
+	if (change->parent) {
+		parent = change->parent;
+		best_parent_rate = parent->rate;
 	} else if (core->parent) {
 		parent = core->parent;
-		best_parent_rate = core->parent->rate;
+		best_parent_rate = parent->rate;
 	}
 
-	if (clk_pm_runtime_get(core))
-		return;
+	ret = clk_pm_runtime_get(core);
+	if (ret)
+		return ret;
 
 	if (core->flags & CLK_SET_RATE_UNGATE) {
 		unsigned long flags;
@@ -1905,35 +1934,55 @@ static void clk_change_rate(struct clk_core *core)
 		clk_enable_unlock(flags);
 	}
 
-	if (core->new_parent && core->new_parent != core->parent) {
-		old_parent = __clk_set_parent_before(core, core->new_parent);
-		trace_clk_set_parent(core, core->new_parent);
+	if (core->flags & CLK_OPS_PARENT_ENABLE)
+		clk_core_prepare_enable(parent);
+
+	if (parent != core->parent) {
+		p_index = clk_fetch_parent_index(core, parent);
+		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;
+		}
+		old_parent = __clk_set_parent_before(core, parent);
+
+		trace_clk_set_parent(core, change->parent);
 
 		if (core->ops->set_rate_and_parent) {
 			skip_set_rate = true;
-			core->ops->set_rate_and_parent(core->hw, core->new_rate,
+			ret = core->ops->set_rate_and_parent(core->hw,
+					change->rate,
 					best_parent_rate,
-					core->new_parent_index);
+					p_index);
 		} else if (core->ops->set_parent) {
-			core->ops->set_parent(core->hw, core->new_parent_index);
+			ret = core->ops->set_parent(core->hw, p_index);
 		}
 
-		trace_clk_set_parent_complete(core, core->new_parent);
-		__clk_set_parent_after(core, core->new_parent, old_parent);
-	}
+		trace_clk_set_parent_complete(core, change->parent);
+		__clk_set_parent_after(core, change->parent, old_parent);
+		if (ret)
+			goto out;
 
-	if (core->flags & CLK_OPS_PARENT_ENABLE)
-		clk_core_prepare_enable(parent);
+	}
 
-	trace_clk_set_rate(core, core->new_rate);
+	trace_clk_set_rate(core, change->rate);
 
 	if (!skip_set_rate && core->ops->set_rate)
-		core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);
+		ret = core->ops->set_rate(core->hw, change->rate,
+				best_parent_rate);
 
-	trace_clk_set_rate_complete(core, core->new_rate);
+	trace_clk_set_rate_complete(core, change->rate);
 
 	core->rate = clk_recalc(core, best_parent_rate);
 
+out:
+	if (core->flags & CLK_OPS_PARENT_ENABLE)
+		clk_core_disable_unprepare(parent);
+
+	if (core->notifier_count && old_rate != core->rate)
+		__clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate);
+
 	if (core->flags & CLK_SET_RATE_UNGATE) {
 		unsigned long flags;
 
@@ -1943,31 +1992,44 @@ static void clk_change_rate(struct clk_core *core)
 		clk_core_unprepare(core);
 	}
 
-	if (core->flags & CLK_OPS_PARENT_ENABLE)
-		clk_core_disable_unprepare(parent);
-
-	if (core->notifier_count && old_rate != core->rate)
-		__clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate);
+	clk_pm_runtime_put(core);
 
 	if (core->flags & CLK_RECALC_NEW_RATES)
-		(void)clk_calc_new_rates(core, core->new_rate);
+		(void)clk_calc_new_rates(core, change->rate);
 
 	/*
-	 * Use safe iteration, as change_rate can actually swap parents
-	 * for certain clock types.
+	 * Keep track of old parent and requested rate in case we have
+	 * to undo the change due to an error.
 	 */
-	hlist_for_each_entry_safe(child, tmp, &core->children, child_node) {
-		/* Skip children who will be reparented to another clock */
-		if (child->new_parent && child->new_parent != core)
-			continue;
-		clk_change_rate(child);
+	change->parent = old_parent;
+	change->rate = old_rate;
+	return ret;
+}
+
+static int clk_change_rates(struct list_head *list)
+{
+	struct clk_change *change, *tmp;
+	int ret = 0;
+
+	list_for_each_entry(change, list, change_list) {
+		ret = clk_change_rate(change);
+		if (ret)
+			goto err;
 	}
 
-	/* handle the new child who might not be in core->children yet */
-	if (core->new_child)
-		clk_change_rate(core->new_child);
+	return 0;
+err:
+	list_for_each_entry_safe_continue(change, tmp, list, change_list)
+		list_del_init(&change->change_list);
 
-	clk_pm_runtime_put(core);
+	list_for_each_entry(change, list, change_list) {
+		/* Give up. Driver might want to shutdown/reboot. */
+		ret = clk_change_rate(change);
+		if (WARN_ON(ret))
+			return ret;
+	}
+
+	return ret;
 }
 
 static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
@@ -2001,7 +2063,9 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 				    unsigned long req_rate)
 {
 	struct clk_core *top, *fail_clk;
+	struct clk_change *change, *tmp;
 	unsigned long rate;
+	LIST_HEAD(changes);
 	int ret = 0;
 
 	if (!core)
@@ -2028,6 +2092,9 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 
 	clk_calc_subtree(core);
 
+	/* Construct the list of changes */
+	clk_prepare_changes(&changes, top);
+
 	/* notify that we are about to change rates */
 	fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
 	if (fail_clk) {
@@ -2039,7 +2106,19 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	}
 
 	/* change the rates */
-	clk_change_rate(top);
+	ret = clk_change_rates(&changes);
+	list_for_each_entry_safe(change, tmp, &changes, change_list) {
+		change->rate = 0;
+		change->parent = NULL;
+		list_del_init(&change->change_list);
+	}
+
+	if (ret) {
+		pr_debug("%s: failed to set %s rate via top clk %s\n", __func__,
+				core->name, top->name);
+		clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
+		goto err;
+	}
 
 	core->req_rate = req_rate;
 err:
@@ -3306,6 +3385,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 	core->max_rate = ULONG_MAX;
 	INIT_LIST_HEAD(&core->prepare_list);
 	INIT_LIST_HEAD(&core->enable_list);
+	INIT_LIST_HEAD(&core->change.change_list);
+	core->change.core = core;
 	hw->core = core;
 
 	/* allocate local copy in case parent_names is __initdata */
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH 3/6] clk: change rates via list iteration

[dbasehore .]

On Tue, Oct 23, 2018 at 6:31 PM Derek Basehore <dbasehore@chromium.org> wrote:
>
> This changes the clk_set_rate code to use lists instead of recursion.
> While making this change, also add error handling for clk_set_rate.
> This means that errors in the set_rate/set_parent/set_rate_and_parent
> functions will not longer be ignored. When an error occurs, the clk
> rates and parents are reset, unless an error occurs here, in which we
> bail and cross our fingers.
>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/clk/clk.c | 225 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 153 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 61de8ad3e4cf..1db44b4e46b0 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -42,6 +42,13 @@ static LIST_HEAD(clk_notifier_list);
>
>  /***    private data structures    ***/
>
> +struct clk_change {
> +       struct list_head        change_list;
> +       unsigned long           rate;
> +       struct clk_core         *core;
> +       struct clk_core         *parent;
> +};
> +
>  struct clk_core {
>         const char              *name;
>         const struct clk_ops    *ops;
> @@ -52,11 +59,9 @@ struct clk_core {
>         const char              **parent_names;
>         struct clk_core         **parents;
>         u8                      num_parents;
> -       u8                      new_parent_index;
>         unsigned long           rate;
>         unsigned long           req_rate;
> -       unsigned long           new_rate;
> -       struct clk_core         *new_parent;
> +       struct clk_change       change;
>         struct clk_core         *new_child;
>         unsigned long           flags;
>         bool                    orphan;
> @@ -1719,20 +1724,53 @@ static int __clk_speculate_rates(struct clk_core *core,
>
>  static void clk_calc_subtree(struct clk_core *core)
>  {
> -       struct clk_core *child;
> +       LIST_HEAD(tmp_list);
>
> -       hlist_for_each_entry(child, &core->children, child_node) {
> -               child->new_rate = clk_recalc(child, core->new_rate);
> -               clk_calc_subtree(child);
> +       list_add(&core->change.change_list, &tmp_list);
> +       while (!list_empty(&tmp_list)) {
> +               struct clk_change *change = list_first_entry(&tmp_list,
> +                               struct clk_change, change_list);
> +               struct clk_core *tmp = change->core;
> +               struct clk_core *child;
> +
> +               hlist_for_each_entry(child, &tmp->children, child_node) {
> +                       child->change.rate = clk_recalc(child,
> +                                       tmp->change.rate);
> +                       list_add_tail(&child->change.change_list, &tmp_list);
> +               }
> +
> +               list_del_init(&change->change_list);
> +       }
> +}
> +
> +static void clk_prepare_changes(struct list_head *change_list,
> +                               struct clk_core *core)
> +{
> +       LIST_HEAD(tmp_list);
> +
> +       list_add(&core->change.change_list, &tmp_list);
> +       while (!list_empty(&tmp_list)) {
> +               struct clk_change *change = list_first_entry(&tmp_list,
> +                               struct clk_change, change_list);
> +               struct clk_core *tmp = change->core;
> +               struct clk_core *child;
> +
> +               hlist_for_each_entry(child, &tmp->children, child_node)
> +                       list_add_tail(&child->change.change_list, &tmp_list);
> +
> +               child = tmp->new_child;
> +               if (child)
> +                       list_add_tail(&child->change.change_list, &tmp_list);
> +
> +               list_move_tail(&tmp->change.change_list, change_list);
>         }
>  }
>
>  static void clk_set_change(struct clk_core *core, unsigned long new_rate,
> -                          struct clk_core *new_parent, u8 p_index)
> +                          struct clk_core *new_parent)
>  {
> -       core->new_rate = new_rate;
> -       core->new_parent = new_parent;
> -       core->new_parent_index = p_index;
> +       core->change.rate = new_rate;
> +       core->change.parent = new_parent;
>         /* include clk in new parent's PRE_RATE_CHANGE notifications */
>         core->new_child = NULL;
>         if (new_parent && new_parent != core->parent)
> @@ -1752,7 +1790,6 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
>         unsigned long new_rate;
>         unsigned long min_rate;
>         unsigned long max_rate;
> -       int p_index = 0;
>         long ret;
>
>         /* sanity */
> @@ -1788,14 +1825,13 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
>                         return NULL;
>         } else if (!parent || !(core->flags & CLK_SET_RATE_PARENT)) {
>                 /* pass-through clock without adjustable parent */
> -               core->new_rate = core->rate;
>                 return NULL;
>         } else {
>                 /* pass-through clock with adjustable parent */
>                 top = clk_calc_new_rates(parent, rate);
> -               new_rate = parent->new_rate;
> +               new_rate = parent->change.rate;
>                 hlist_for_each_entry(child, &parent->children, child_node)
> -                       child->new_rate = clk_recalc(child, new_rate);
> +                       child->change.rate = clk_recalc(child, new_rate);
>                 goto out;
>         }
>
> @@ -1807,25 +1843,16 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
>                 return NULL;
>         }
>
> -       /* try finding the new parent index */
> -       if (parent && core->num_parents > 1) {
> -               p_index = clk_fetch_parent_index(core, parent);
> -               if (p_index < 0) {
> -                       pr_debug("%s: clk %s can not be parent of clk %s\n",
> -                                __func__, parent->name, core->name);
> -                       return NULL;
> -               }
> -       }
> -
>         if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
>             best_parent_rate != parent->rate) {
>                 top = clk_calc_new_rates(parent, best_parent_rate);
>                 hlist_for_each_entry(child, &parent->children, child_node)
> -                       child->new_rate = clk_recalc(child, parent->new_rate);
> +                       child->change.rate = clk_recalc(child,
> +                                       parent->change.rate);
>         }
>
>  out:
> -       clk_set_change(core, new_rate, parent, p_index);
> +       clk_set_change(core, new_rate, parent);
>
>         return top;
>  }
> @@ -1841,18 +1868,18 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
>         struct clk_core *child, *tmp_clk, *fail_clk = NULL;
>         int ret = NOTIFY_DONE;
>
> -       if (core->rate == core->new_rate)
> +       if (core->rate == core->change.rate)
>                 return NULL;
>
>         if (core->notifier_count) {
> -               ret = __clk_notify(core, event, core->rate, core->new_rate);
> +               ret = __clk_notify(core, event, core->rate, core->change.rate);
>                 if (ret & NOTIFY_STOP_MASK)
>                         fail_clk = core;
>         }
>
>         hlist_for_each_entry(child, &core->children, child_node) {
>                 /* Skip children who will be reparented to another clock */
> -               if (child->new_parent && child->new_parent != core)
> +               if (child->change.parent && child->change.parent != core)
>                         continue;
>                 tmp_clk = clk_propagate_rate_change(child, event);
>                 if (tmp_clk)
> @@ -1873,28 +1900,30 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
>   * walk down a subtree and set the new rates notifying the rate
>   * change on the way
>   */
> -static void clk_change_rate(struct clk_core *core)
> +static int clk_change_rate(struct clk_change *change)
>  {
> -       struct clk_core *child;
> -       struct hlist_node *tmp;
> +       struct clk_core *core = change->core;
>         unsigned long old_rate;
>         unsigned long best_parent_rate = 0;
>         bool skip_set_rate = false;
> -       struct clk_core *old_parent;
> +       struct clk_core *old_parent = NULL;
>         struct clk_core *parent = NULL;
> +       int p_index;
> +       int ret = 0;
>
>         old_rate = core->rate;
>
> -       if (core->new_parent) {
> -               parent = core->new_parent;
> -               best_parent_rate = core->new_parent->rate;
> +       if (change->parent) {
> +               parent = change->parent;
> +               best_parent_rate = parent->rate;
>         } else if (core->parent) {
>                 parent = core->parent;
> -               best_parent_rate = core->parent->rate;
> +               best_parent_rate = parent->rate;
>         }
>
> -       if (clk_pm_runtime_get(core))
> -               return;
> +       ret = clk_pm_runtime_get(core);
> +       if (ret)
> +               return ret;
>
>         if (core->flags & CLK_SET_RATE_UNGATE) {
>                 unsigned long flags;
> @@ -1905,35 +1934,55 @@ static void clk_change_rate(struct clk_core *core)
>                 clk_enable_unlock(flags);
>         }
>
> -       if (core->new_parent && core->new_parent != core->parent) {
> -               old_parent = __clk_set_parent_before(core, core->new_parent);
> -               trace_clk_set_parent(core, core->new_parent);
> +       if (core->flags & CLK_OPS_PARENT_ENABLE)
> +               clk_core_prepare_enable(parent);
> +
> +       if (parent != core->parent) {
> +               p_index = clk_fetch_parent_index(core, parent);
> +               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;
> +               }
> +               old_parent = __clk_set_parent_before(core, parent);
> +
> +               trace_clk_set_parent(core, change->parent);
>
>                 if (core->ops->set_rate_and_parent) {
>                         skip_set_rate = true;
> -                       core->ops->set_rate_and_parent(core->hw, core->new_rate,
> +                       ret = core->ops->set_rate_and_parent(core->hw,
> +                                       change->rate,
>                                         best_parent_rate,
> -                                       core->new_parent_index);
> +                                       p_index);
>                 } else if (core->ops->set_parent) {
> -                       core->ops->set_parent(core->hw, core->new_parent_index);
> +                       ret = core->ops->set_parent(core->hw, p_index);
>                 }
>
> -               trace_clk_set_parent_complete(core, core->new_parent);
> -               __clk_set_parent_after(core, core->new_parent, old_parent);
> -       }
> +               trace_clk_set_parent_complete(core, change->parent);
> +               __clk_set_parent_after(core, change->parent, old_parent);
> +               if (ret)
> +                       goto out;

Need to handle the set_parent op failing here. __clk_set_parent_before
needs to be undone, and we need to not call __clk_set_parent_after
with the failed state.

>
> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
> -               clk_core_prepare_enable(parent);
> +       }
>
> -       trace_clk_set_rate(core, core->new_rate);
> +       trace_clk_set_rate(core, change->rate);
>
>         if (!skip_set_rate && core->ops->set_rate)
> -               core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);
> +               ret = core->ops->set_rate(core->hw, change->rate,
> +                               best_parent_rate);
>
> -       trace_clk_set_rate_complete(core, core->new_rate);
> +       trace_clk_set_rate_complete(core, change->rate);
>
>         core->rate = clk_recalc(core, best_parent_rate);
>
> +out:
> +       if (core->flags & CLK_OPS_PARENT_ENABLE)
> +               clk_core_disable_unprepare(parent);
> +
> +       if (core->notifier_count && old_rate != core->rate)
> +               __clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate);
> +
>         if (core->flags & CLK_SET_RATE_UNGATE) {
>                 unsigned long flags;
>
> @@ -1943,31 +1992,44 @@ static void clk_change_rate(struct clk_core *core)
>                 clk_core_unprepare(core);
>         }
>
> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
> -               clk_core_disable_unprepare(parent);
> -
> -       if (core->notifier_count && old_rate != core->rate)
> -               __clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate);
> +       clk_pm_runtime_put(core);
>
>         if (core->flags & CLK_RECALC_NEW_RATES)
> -               (void)clk_calc_new_rates(core, core->new_rate);
> +               (void)clk_calc_new_rates(core, change->rate);
>
>         /*
> -        * Use safe iteration, as change_rate can actually swap parents
> -        * for certain clock types.
> +        * Keep track of old parent and requested rate in case we have
> +        * to undo the change due to an error.
>          */
> -       hlist_for_each_entry_safe(child, tmp, &core->children, child_node) {
> -               /* Skip children who will be reparented to another clock */
> -               if (child->new_parent && child->new_parent != core)
> -                       continue;
> -               clk_change_rate(child);
> +       change->parent = old_parent;
> +       change->rate = old_rate;
> +       return ret;
> +}
> +
> +static int clk_change_rates(struct list_head *list)
> +{
> +       struct clk_change *change, *tmp;
> +       int ret = 0;
> +
> +       list_for_each_entry(change, list, change_list) {
> +               ret = clk_change_rate(change);
> +               if (ret)
> +                       goto err;
>         }
>
> -       /* handle the new child who might not be in core->children yet */
> -       if (core->new_child)
> -               clk_change_rate(core->new_child);
> +       return 0;
> +err:
> +       list_for_each_entry_safe_continue(change, tmp, list, change_list)
> +               list_del_init(&change->change_list);
>
> -       clk_pm_runtime_put(core);
> +       list_for_each_entry(change, list, change_list) {
> +               /* Give up. Driver might want to shutdown/reboot. */
> +               ret = clk_change_rate(change);
> +               if (WARN_ON(ret))
> +                       return ret;
> +       }
> +
> +       return ret;
>  }
>
>  static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
> @@ -2001,7 +2063,9 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>                                     unsigned long req_rate)
>  {
>         struct clk_core *top, *fail_clk;
> +       struct clk_change *change, *tmp;
>         unsigned long rate;
> +       LIST_HEAD(changes);
>         int ret = 0;
>
>         if (!core)
> @@ -2028,6 +2092,9 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>
>         clk_calc_subtree(core);
>
> +       /* Construct the list of changes */
> +       clk_prepare_changes(&changes, top);
> +
>         /* notify that we are about to change rates */
>         fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
>         if (fail_clk) {
> @@ -2039,7 +2106,19 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>         }
>
>         /* change the rates */
> -       clk_change_rate(top);
> +       ret = clk_change_rates(&changes);
> +       list_for_each_entry_safe(change, tmp, &changes, change_list) {
> +               change->rate = 0;
> +               change->parent = NULL;
> +               list_del_init(&change->change_list);
> +       }
> +
> +       if (ret) {
> +               pr_debug("%s: failed to set %s rate via top clk %s\n", __func__,
> +                               core->name, top->name);
> +               clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
> +               goto err;
> +       }
>
>         core->req_rate = req_rate;
>  err:
> @@ -3306,6 +3385,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>         core->max_rate = ULONG_MAX;
>         INIT_LIST_HEAD(&core->prepare_list);
>         INIT_LIST_HEAD(&core->enable_list);
> +       INIT_LIST_HEAD(&core->change.change_list);
> +       core->change.core = core;
>         hw->core = core;
>
>         /* allocate local copy in case parent_names is __initdata */
> --
> 2.19.1.568.g152ad8e336-goog
>

[PATCH 4/6] clk: add pre clk changes support

[Derek Basehore]

This adds a new clk_op, pre_rate_req. It allows clks to setup an
intermediate state when clk rates are changed. One use case for this
is when a clk needs to switch to a safe parent when its PLL ancestor
changes rates. This is needed when a PLL cannot guarantee that it will
not exceed the new rate before it locks. The set_rate, set_parent, and
set_rate_and_parent callbacks are used with the pre_rate_req callback.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/clk/clk.c            | 136 +++++++++++++++++++++++++++++++++--
 include/linux/clk-provider.h |  10 +++
 2 files changed, 139 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 1db44b4e46b0..36a2f929ab8d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -39,6 +39,7 @@ static int enable_refcnt;
 static HLIST_HEAD(clk_root_list);
 static HLIST_HEAD(clk_orphan_list);
 static LIST_HEAD(clk_notifier_list);
+static LIST_HEAD(pre_change_free_list);
 
 /***    private data structures    ***/
 
@@ -1896,6 +1897,74 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
 	return fail_clk;
 }
 
+static int clk_pre_rate_req(struct list_head *pre_list, struct clk_core *core)
+{
+	struct clk_core *child, *parent = core->parent;
+	struct clk_rate_request next, pre;
+	struct clk_change *change;
+	int ret;
+
+	hlist_for_each_entry(child, &core->children, child_node) {
+		ret = clk_pre_rate_req(pre_list, child);
+		if (ret)
+			return ret;
+	}
+
+	if (core->new_child) {
+		ret = clk_pre_rate_req(pre_list, child);
+		if (ret)
+			return ret;
+	}
+
+	if (!core->ops->pre_rate_req)
+		return 0;
+
+	if (core->change.parent)
+		parent = core->change.parent;
+
+	if (parent) {
+		next.best_parent_hw = parent->hw;
+		next.best_parent_rate = parent->change.rate;
+	}
+
+	next.rate = core->change.rate;
+	clk_core_get_boundaries(core, &next.min_rate, &next.max_rate);
+
+	ret = core->ops->pre_rate_req(core->hw, &next, &pre);
+	if (ret < 0)
+		return ret;
+	else if (!ret)
+		goto out;
+
+	/*
+	 * We allocate a change for each clk with the pre_rate_req op. If we run
+	 * out, that's because we wrapped around to a clk again in the
+	 * pre_rate_req step which is not allowed.
+	 */
+	change = list_first_entry(&pre_change_free_list, struct clk_change,
+				  change_list);
+	if (IS_ERR_OR_NULL(change)) {
+		pr_err("%s: pre_rate_req loop detected on clk %s. All pre_rate_req clk_change structs are used\n",
+				__func__, core->name);
+		return -EDEADLK;
+	}
+
+	change->core = core;
+	change->rate = pre.rate;
+	change->parent = pre.best_parent_hw ? pre.best_parent_hw->core : NULL;
+	list_move(&change->change_list, pre_list);
+
+out:
+	/* If the pre req req pulls in a new parent, add it to the call chain */
+	if (parent != change->parent) {
+		ret = clk_pre_rate_req(pre_list, change->parent);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /*
  * walk down a subtree and set the new rates notifying the rate
  * change on the way
@@ -2065,6 +2134,8 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	struct clk_core *top, *fail_clk;
 	struct clk_change *change, *tmp;
 	unsigned long rate;
+	LIST_HEAD(pre_changes);
+	LIST_HEAD(post_changes);
 	LIST_HEAD(changes);
 	int ret = 0;
 
@@ -2092,6 +2163,14 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 
 	clk_calc_subtree(core);
 
+	/* We need a separate list for these changes due to error handling. */
+	ret = clk_pre_rate_req(&pre_changes, top);
+	if (ret) {
+		pr_debug("%s: failed pre_rate_req via top clk %s: %d\n",
+				__func__, top->name, ret);
+		goto pre_rate_req;
+	}
+
 	/* Construct the list of changes */
 	clk_prepare_changes(&changes, top);
 
@@ -2100,11 +2179,19 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (fail_clk) {
 		pr_debug("%s: failed to set %s rate\n", __func__,
 				fail_clk->name);
-		clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
 		ret = -EBUSY;
-		goto err;
+		goto prop_rate;
+	}
+
+	ret = clk_change_rates(&pre_changes);
+	if (ret) {
+		pr_debug("%s: rate rate changes failed via top clk %s: %d\n",
+				__func__, top->name, ret);
+		goto pre_rate_req;
 	}
 
+	list_splice_tail(&post_changes, &changes);
+
 	/* change the rates */
 	ret = clk_change_rates(&changes);
 	list_for_each_entry_safe(change, tmp, &changes, change_list) {
@@ -2112,16 +2199,28 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		change->parent = NULL;
 		list_del_init(&change->change_list);
 	}
-
 	if (ret) {
 		pr_debug("%s: failed to set %s rate via top clk %s\n", __func__,
 				core->name, top->name);
-		clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
-		goto err;
+		goto change_rates;
 	}
 
+	list_splice(&pre_changes, &pre_change_free_list);
 	core->req_rate = req_rate;
-err:
+
+	return 0;
+
+change_rates:
+	WARN_ON(clk_change_rates(&pre_changes));
+pre_rate_req:
+	list_splice(&pre_changes, &pre_change_free_list);
+prop_rate:
+	clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
+	list_for_each_entry_safe(change, tmp, &changes, change_list) {
+		change->rate = 0;
+		change->parent = NULL;
+		list_del_init(&change->change_list);
+	}
 	clk_pm_runtime_put(core);
 
 	return ret;
@@ -3139,7 +3238,9 @@ static int __clk_core_init(struct clk_core *core)
 
 	/* check that clk_ops are sane.  See Documentation/driver-api/clk.rst */
 	if (core->ops->set_rate &&
-	    !((core->ops->round_rate || core->ops->determine_rate) &&
+	    !((core->ops->round_rate ||
+	       core->ops->determine_rate ||
+	       core->ops->pre_rate_req) &&
 	      core->ops->recalc_rate)) {
 		pr_err("%s: %s must implement .round_rate or .determine_rate in addition to .recalc_rate\n",
 		       __func__, core->name);
@@ -3175,6 +3276,20 @@ static int __clk_core_init(struct clk_core *core)
 				"%s: invalid NULL in %s's .parent_names\n",
 				__func__, core->name);
 
+	/* Allocate a clk_change struct for pre_rate_reqs */
+	if (core->ops->pre_rate_req) {
+		struct clk_change *change = kzalloc(sizeof(*change),
+				GFP_KERNEL);
+		if (!change) {
+			ret = -ENOMEM;
+			kfree(core->parents);
+			goto out;
+		}
+
+		INIT_LIST_HEAD(&change->change_list);
+		list_add(&pre_change_free_list, &change->change_list);
+	}
+
 	core->parent = __clk_init_parent(core);
 
 	/*
@@ -3476,6 +3591,13 @@ static void __clk_release(struct kref *ref)
 	while (--i >= 0)
 		kfree_const(core->parent_names[i]);
 
+	if (core->ops->pre_rate_req) {
+		struct clk_change *change =
+			list_first_entry(&pre_change_free_list,
+					struct clk_change, change_list);
+		list_del(&change->change_list);
+		kfree(change);
+	}
 	kfree(core->parent_names);
 	kfree_const(core->name);
 	kfree(core);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 60c51871b04b..98a65c6c326d 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -138,6 +138,13 @@ struct clk_duty {
  *		actually supported by the clock, and optionally the parent clock
  *		that should be used to provide the clock rate.
  *
+ * @pre_rate_req: Given the next state that the clk will enter via a
+ *		clk_rate_request struct, next, fill in another clk_rate_request
+ *		struct, pre, with any desired intermediate state to change to
+ *		before the state in next is applied. Returns positive to request
+ *		an intermediate state transition, 0 for no transition, and
+ *		-EERROR otherwise.
+ *
  * @set_parent:	Change the input source of this clock; for clocks with multiple
  *		possible parents specify a new parent by passing in the index
  *		as a u8 corresponding to the parent in either the .parent_names
@@ -236,6 +243,9 @@ struct clk_ops {
 					unsigned long *parent_rate);
 	int		(*determine_rate)(struct clk_hw *hw,
 					  struct clk_rate_request *req);
+	int		(*pre_rate_req)(struct clk_hw *hw,
+					const struct clk_rate_request *next,
+					struct clk_rate_request *pre);
 	int		(*set_parent)(struct clk_hw *hw, u8 index);
 	u8		(*get_parent)(struct clk_hw *hw);
 	int		(*set_rate)(struct clk_hw *hw, unsigned long rate,
-- 
2.19.1.568.g152ad8e336-goog

[PATCH 5/6] docs: driver-api: add pre_rate_req to clk documentation

[Derek Basehore]

This adds documentation for the new clk op pre_rate_req.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 Documentation/driver-api/clk.rst | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/clk.rst b/Documentation/driver-api/clk.rst
index 593cca5058b1..917f6ac29645 100644
--- a/Documentation/driver-api/clk.rst
+++ b/Documentation/driver-api/clk.rst
@@ -82,6 +82,9 @@ the operations defined in clk-provider.h::
 						unsigned long *parent_rate);
 		int		(*determine_rate)(struct clk_hw *hw,
 						  struct clk_rate_request *req);
+		int		(*pre_rate_req)(struct clk_hw *hw,
+						const struct clk_rate_request *next,
+						struct clk_rate_request *pre);
 		int		(*set_parent)(struct clk_hw *hw, u8 index);
 		u8		(*get_parent)(struct clk_hw *hw);
 		int		(*set_rate)(struct clk_hw *hw,
@@ -224,6 +227,8 @@ optional or must be evaluated on a case-by-case basis.
    +----------------+------+-------------+---------------+-------------+------+
    |.determine_rate |      | y [1]_      |               |             |      |
    +----------------+------+-------------+---------------+-------------+------+
+   |.pre_rate_req   |      | y [1]_      |               |             |      |
+   +----------------+------+-------------+---------------+-------------+------+
    |.set_rate       |      | y           |               |             |      |
    +----------------+------+-------------+---------------+-------------+------+
    +----------------+------+-------------+---------------+-------------+------+
@@ -238,7 +243,7 @@ optional or must be evaluated on a case-by-case basis.
    |.init           |      |             |               |             |      |
    +----------------+------+-------------+---------------+-------------+------+
 
-.. [1] either one of round_rate or determine_rate is required.
+.. [1] one of round_rate, determine_rate, or pre_rate_req is required.
 
 Finally, register your clock at run-time with a hardware-specific
 registration function.  This function simply populates struct clk_foo's
-- 
2.19.1.568.g152ad8e336-goog

[PATCH 6/6] clk: rockchip: use pre_rate_req for cpuclk

[Derek Basehore]

This makes the rockchip cpuclk use the pre_rate_req op to change to
the alt parent instead of the clk notifier. This has the benefit of
the clk not changing parents behind the back of the common clk
framework. It also changes the divider setting for the alt parent to
only divide when the alt parent rate is higher than both the old and
new rates.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/clk/rockchip/clk-cpu.c | 256 ++++++++++++++++++---------------
 1 file changed, 137 insertions(+), 119 deletions(-)

diff --git a/drivers/clk/rockchip/clk-cpu.c b/drivers/clk/rockchip/clk-cpu.c
index 32c19c0f1e14..3829e8e75c9e 100644
--- a/drivers/clk/rockchip/clk-cpu.c
+++ b/drivers/clk/rockchip/clk-cpu.c
@@ -45,8 +45,6 @@
  * @alt_parent:	alternate parent clock to use when switching the speed
  *		of the primary parent clock.
  * @reg_base:	base register for cpu-clock values.
- * @clk_nb:	clock notifier registered for changes in clock speed of the
- *		primary parent clock.
  * @rate_count:	number of rates in the rate_table
  * @rate_table:	pll-rates and their associated dividers
  * @reg_data:	cpu-specific register settings
@@ -60,7 +58,6 @@ struct rockchip_cpuclk {
 
 	struct clk				*alt_parent;
 	void __iomem				*reg_base;
-	struct notifier_block			clk_nb;
 	unsigned int				rate_count;
 	struct rockchip_cpuclk_rate_table	*rate_table;
 	const struct rockchip_cpuclk_reg_data	*reg_data;
@@ -78,12 +75,21 @@ static const struct rockchip_cpuclk_rate_table *rockchip_get_cpuclk_settings(
 							cpuclk->rate_table;
 	int i;
 
+	/*
+	 * Find the lowest rate settings for which the prate is greater than or
+	 * equal to the rate. Final rates should match exactly, but some
+	 * intermediate rates from pre_rate_req will not exactly match, but the
+	 * settings for a higher prate will work.
+	 */
 	for (i = 0; i < cpuclk->rate_count; i++) {
-		if (rate == rate_table[i].prate)
-			return &rate_table[i];
+		if (rate > rate_table[i].prate)
+			break;
 	}
 
-	return NULL;
+	if (i == 0 || i == cpuclk->rate_count)
+		return NULL;
+
+	return &rate_table[i - 1];
 }
 
 static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
@@ -98,9 +104,70 @@ static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
 	return parent_rate / (clksel0 + 1);
 }
 
-static const struct clk_ops rockchip_cpuclk_ops = {
-	.recalc_rate = rockchip_cpuclk_recalc_rate,
-};
+static int rockchip_cpuclk_pre_rate_req(struct clk_hw *hw,
+					const struct clk_rate_request *next,
+					struct clk_rate_request *pre)
+{
+	struct rockchip_cpuclk *cpuclk = container_of(hw,
+			struct rockchip_cpuclk, hw);
+	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
+	unsigned long alt_prate, alt_div, hi_rate;
+
+	pre->best_parent_hw = __clk_get_hw(cpuclk->alt_parent);
+	alt_prate = clk_get_rate(cpuclk->alt_parent);
+	pre->best_parent_rate = alt_prate;
+	hi_rate = max_t(unsigned long, next->rate, clk_hw_get_rate(hw));
+
+	/* Set dividers if we would go above the current or next rate. */
+	if (alt_prate > hi_rate) {
+		alt_div =  DIV_ROUND_UP(alt_prate, hi_rate);
+		if (alt_div > reg_data->div_core_mask) {
+			pr_warn("%s: limiting alt-divider %lu to %d\n",
+				__func__, alt_div, reg_data->div_core_mask);
+			alt_div = reg_data->div_core_mask;
+		}
+
+		pre->rate = alt_prate / alt_div;
+	} else {
+		pre->rate = alt_prate;
+	}
+
+	return 1;
+}
+
+static int rockchip_cpuclk_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct rockchip_cpuclk *cpuclk = container_of(hw,
+			struct rockchip_cpuclk, hw);
+	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(cpuclk->lock, flags);
+	writel(HIWORD_UPDATE(index,
+			     reg_data->mux_core_mask,
+			     reg_data->mux_core_shift),
+	       cpuclk->reg_base + reg_data->core_reg);
+	spin_unlock_irqrestore(cpuclk->lock, flags);
+
+	return 0;
+}
+
+static u8 rockchip_cpuclk_get_parent(struct clk_hw *hw)
+{
+	struct rockchip_cpuclk *cpuclk = container_of(hw,
+			struct rockchip_cpuclk, hw);
+	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(cpuclk->lock, flags);
+	val = readl_relaxed(cpuclk->reg_base + reg_data->core_reg);
+	val >>= reg_data->mux_core_shift;
+	val &= reg_data->mux_core_mask;
+	spin_unlock_irqrestore(cpuclk->lock, flags);
+
+	return val;
+}
 
 static void rockchip_cpuclk_set_dividers(struct rockchip_cpuclk *cpuclk,
 				const struct rockchip_cpuclk_rate_table *rate)
@@ -120,131 +187,92 @@ static void rockchip_cpuclk_set_dividers(struct rockchip_cpuclk *cpuclk,
 	}
 }
 
-static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk,
-					   struct clk_notifier_data *ndata)
+static int rockchip_cpuclk_set_rate(struct clk_hw *hw, unsigned long rate,
+					unsigned long parent_rate)
 {
+	struct rockchip_cpuclk *cpuclk = container_of(hw,
+			struct rockchip_cpuclk, hw);
 	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
-	const struct rockchip_cpuclk_rate_table *rate;
-	unsigned long alt_prate, alt_div;
-	unsigned long flags;
+	const struct rockchip_cpuclk_rate_table *rate_divs;
+	unsigned long div = (parent_rate / rate) - 1;
+	unsigned long old_rate, flags;
 
-	/* check validity of the new rate */
-	rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
-	if (!rate) {
-		pr_err("%s: Invalid rate : %lu for cpuclk\n",
-		       __func__, ndata->new_rate);
+	if (div > reg_data->div_core_mask || rate > parent_rate) {
+		pr_err("%s: Invalid rate : %lu %lu for cpuclk\n", __func__,
+				rate, parent_rate);
 		return -EINVAL;
 	}
 
-	alt_prate = clk_get_rate(cpuclk->alt_parent);
-
+	old_rate = clk_hw_get_rate(hw);
+	rate_divs = rockchip_get_cpuclk_settings(cpuclk, rate);
 	spin_lock_irqsave(cpuclk->lock, flags);
+	if (old_rate < rate)
+		rockchip_cpuclk_set_dividers(cpuclk, rate_divs);
 
-	/*
-	 * If the old parent clock speed is less than the clock speed
-	 * of the alternate parent, then it should be ensured that at no point
-	 * the armclk speed is more than the old_rate until the dividers are
-	 * set.
-	 */
-	if (alt_prate > ndata->old_rate) {
-		/* calculate dividers */
-		alt_div =  DIV_ROUND_UP(alt_prate, ndata->old_rate) - 1;
-		if (alt_div > reg_data->div_core_mask) {
-			pr_warn("%s: limiting alt-divider %lu to %d\n",
-				__func__, alt_div, reg_data->div_core_mask);
-			alt_div = reg_data->div_core_mask;
-		}
-
-		/*
-		 * Change parents and add dividers in a single transaction.
-		 *
-		 * NOTE: we do this in a single transaction so we're never
-		 * dividing the primary parent by the extra dividers that were
-		 * needed for the alt.
-		 */
-		pr_debug("%s: setting div %lu as alt-rate %lu > old-rate %lu\n",
-			 __func__, alt_div, alt_prate, ndata->old_rate);
-
-		writel(HIWORD_UPDATE(alt_div, reg_data->div_core_mask,
-					      reg_data->div_core_shift) |
-		       HIWORD_UPDATE(reg_data->mux_core_alt,
-				     reg_data->mux_core_mask,
-				     reg_data->mux_core_shift),
-		       cpuclk->reg_base + reg_data->core_reg);
-	} else {
-		/* select alternate parent */
-		writel(HIWORD_UPDATE(reg_data->mux_core_alt,
-				     reg_data->mux_core_mask,
-				     reg_data->mux_core_shift),
-		       cpuclk->reg_base + reg_data->core_reg);
-	}
+	writel(HIWORD_UPDATE(div,
+			     reg_data->div_core_mask,
+			     reg_data->div_core_shift),
+	       cpuclk->reg_base + reg_data->core_reg);
+	if (old_rate > rate)
+		rockchip_cpuclk_set_dividers(cpuclk, rate_divs);
 
 	spin_unlock_irqrestore(cpuclk->lock, flags);
+
 	return 0;
 }
 
-static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk *cpuclk,
-					    struct clk_notifier_data *ndata)
+static int rockchip_cpuclk_set_rate_and_parent(struct clk_hw *hw,
+					unsigned long rate,
+					unsigned long parent_rate,
+					u8 index)
 {
+	struct rockchip_cpuclk *cpuclk = container_of(hw,
+			struct rockchip_cpuclk, hw);
 	const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
-	const struct rockchip_cpuclk_rate_table *rate;
-	unsigned long flags;
+	const struct rockchip_cpuclk_rate_table *rate_divs;
+	unsigned long div = (parent_rate / rate) - 1;
+	unsigned long old_rate, flags;
 
-	rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
-	if (!rate) {
-		pr_err("%s: Invalid rate : %lu for cpuclk\n",
-		       __func__, ndata->new_rate);
+	if (div > reg_data->div_core_mask || rate > parent_rate) {
+		pr_err("%s: Invalid rate : %lu %lu for cpuclk\n", __func__,
+				rate, parent_rate);
 		return -EINVAL;
 	}
 
+	old_rate = clk_hw_get_rate(hw);
+	rate_divs = rockchip_get_cpuclk_settings(cpuclk, rate);
 	spin_lock_irqsave(cpuclk->lock, flags);
-
-	if (ndata->old_rate < ndata->new_rate)
-		rockchip_cpuclk_set_dividers(cpuclk, rate);
-
 	/*
-	 * post-rate change event, re-mux to primary parent and remove dividers.
-	 *
-	 * NOTE: we do this in a single transaction so we're never dividing the
-	 * primary parent by the extra dividers that were needed for the alt.
+	 * TODO: This ain't great... Should change the get_cpuclk_settings code
+	 * to work with inexact matches to work with alt parent rates.
 	 */
-
-	writel(HIWORD_UPDATE(0, reg_data->div_core_mask,
-				reg_data->div_core_shift) |
-	       HIWORD_UPDATE(reg_data->mux_core_main,
-				reg_data->mux_core_mask,
-				reg_data->mux_core_shift),
+	if (old_rate < rate)
+		rockchip_cpuclk_set_dividers(cpuclk, rate_divs);
+
+	writel(HIWORD_UPDATE(div,
+			     reg_data->div_core_mask,
+			     reg_data->div_core_shift) |
+	       HIWORD_UPDATE(index,
+			     reg_data->mux_core_mask,
+			     reg_data->mux_core_shift),
 	       cpuclk->reg_base + reg_data->core_reg);
-
-	if (ndata->old_rate > ndata->new_rate)
-		rockchip_cpuclk_set_dividers(cpuclk, rate);
+	/* Not technically correct */
+	if (old_rate > rate)
+		rockchip_cpuclk_set_dividers(cpuclk, rate_divs);
 
 	spin_unlock_irqrestore(cpuclk->lock, flags);
+
 	return 0;
 }
 
-/*
- * This clock notifier is called when the frequency of the parent clock
- * of cpuclk is to be changed. This notifier handles the setting up all
- * the divider clocks, remux to temporary parent and handling the safe
- * frequency levels when using temporary parent.
- */
-static int rockchip_cpuclk_notifier_cb(struct notifier_block *nb,
-					unsigned long event, void *data)
-{
-	struct clk_notifier_data *ndata = data;
-	struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_nb(nb);
-	int ret = 0;
-
-	pr_debug("%s: event %lu, old_rate %lu, new_rate: %lu\n",
-		 __func__, event, ndata->old_rate, ndata->new_rate);
-	if (event == PRE_RATE_CHANGE)
-		ret = rockchip_cpuclk_pre_rate_change(cpuclk, ndata);
-	else if (event == POST_RATE_CHANGE)
-		ret = rockchip_cpuclk_post_rate_change(cpuclk, ndata);
-
-	return notifier_from_errno(ret);
-}
+static const struct clk_ops rockchip_cpuclk_ops = {
+	.recalc_rate = rockchip_cpuclk_recalc_rate,
+	.pre_rate_req = rockchip_cpuclk_pre_rate_req,
+	.set_parent = rockchip_cpuclk_set_parent,
+	.get_parent = rockchip_cpuclk_get_parent,
+	.set_rate = rockchip_cpuclk_set_rate,
+	.set_rate_and_parent = rockchip_cpuclk_set_rate_and_parent,
+};
 
 struct clk *rockchip_clk_register_cpuclk(const char *name,
 			const char *const *parent_names, u8 num_parents,
@@ -267,8 +295,8 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
 		return ERR_PTR(-ENOMEM);
 
 	init.name = name;
-	init.parent_names = &parent_names[reg_data->mux_core_main];
-	init.num_parents = 1;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
 	init.ops = &rockchip_cpuclk_ops;
 
 	/* only allow rate changes when we have a rate table */
@@ -282,7 +310,6 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
 	cpuclk->reg_base = reg_base;
 	cpuclk->lock = lock;
 	cpuclk->reg_data = reg_data;
-	cpuclk->clk_nb.notifier_call = rockchip_cpuclk_notifier_cb;
 	cpuclk->hw.init = &init;
 
 	cpuclk->alt_parent = __clk_lookup(parent_names[reg_data->mux_core_alt]);
@@ -309,13 +336,6 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
 		goto free_alt_parent;
 	}
 
-	ret = clk_notifier_register(clk, &cpuclk->clk_nb);
-	if (ret) {
-		pr_err("%s: failed to register clock notifier for %s\n",
-				__func__, name);
-		goto free_alt_parent;
-	}
-
 	if (nrates > 0) {
 		cpuclk->rate_count = nrates;
 		cpuclk->rate_table = kmemdup(rates,
@@ -323,7 +343,7 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
 					     GFP_KERNEL);
 		if (!cpuclk->rate_table) {
 			ret = -ENOMEM;
-			goto unregister_notifier;
+			goto free_alt_parent;
 		}
 	}
 
@@ -338,8 +358,6 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
 
 free_rate_table:
 	kfree(cpuclk->rate_table);
-unregister_notifier:
-	clk_notifier_unregister(clk, &cpuclk->clk_nb);
 free_alt_parent:
 	clk_disable_unprepare(cpuclk->alt_parent);
 free_cpuclk:
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH 6/6] clk: rockchip: use pre_rate_req for cpuclk

[dbasehore .]

On Tue, Oct 23, 2018 at 6:31 PM Derek Basehore <dbasehore@chromium.org> wrote:
>
> This makes the rockchip cpuclk use the pre_rate_req op to change to
> the alt parent instead of the clk notifier. This has the benefit of
> the clk not changing parents behind the back of the common clk
> framework. It also changes the divider setting for the alt parent to
> only divide when the alt parent rate is higher than both the old and
> new rates.
>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/clk/rockchip/clk-cpu.c | 256 ++++++++++++++++++---------------
>  1 file changed, 137 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/clk/rockchip/clk-cpu.c b/drivers/clk/rockchip/clk-cpu.c
> index 32c19c0f1e14..3829e8e75c9e 100644
> --- a/drivers/clk/rockchip/clk-cpu.c
> +++ b/drivers/clk/rockchip/clk-cpu.c
> @@ -45,8 +45,6 @@
>   * @alt_parent:        alternate parent clock to use when switching the speed
>   *             of the primary parent clock.
>   * @reg_base:  base register for cpu-clock values.
> - * @clk_nb:    clock notifier registered for changes in clock speed of the
> - *             primary parent clock.
>   * @rate_count:        number of rates in the rate_table
>   * @rate_table:        pll-rates and their associated dividers
>   * @reg_data:  cpu-specific register settings
> @@ -60,7 +58,6 @@ struct rockchip_cpuclk {
>
>         struct clk                              *alt_parent;
>         void __iomem                            *reg_base;
> -       struct notifier_block                   clk_nb;
>         unsigned int                            rate_count;
>         struct rockchip_cpuclk_rate_table       *rate_table;
>         const struct rockchip_cpuclk_reg_data   *reg_data;
> @@ -78,12 +75,21 @@ static const struct rockchip_cpuclk_rate_table *rockchip_get_cpuclk_settings(
>                                                         cpuclk->rate_table;
>         int i;
>
> +       /*
> +        * Find the lowest rate settings for which the prate is greater than or
> +        * equal to the rate. Final rates should match exactly, but some
> +        * intermediate rates from pre_rate_req will not exactly match, but the
> +        * settings for a higher prate will work.
> +        */
>         for (i = 0; i < cpuclk->rate_count; i++) {
> -               if (rate == rate_table[i].prate)
> -                       return &rate_table[i];
> +               if (rate > rate_table[i].prate)
> +                       break;
>         }
>
> -       return NULL;
> +       if (i == 0 || i == cpuclk->rate_count)
> +               return NULL;
> +
> +       return &rate_table[i - 1];
>  }
>
>  static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
> @@ -98,9 +104,70 @@ static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
>         return parent_rate / (clksel0 + 1);
>  }
>
> -static const struct clk_ops rockchip_cpuclk_ops = {
> -       .recalc_rate = rockchip_cpuclk_recalc_rate,
> -};
> +static int rockchip_cpuclk_pre_rate_req(struct clk_hw *hw,
> +                                       const struct clk_rate_request *next,
> +                                       struct clk_rate_request *pre)
> +{
> +       struct rockchip_cpuclk *cpuclk = container_of(hw,
> +                       struct rockchip_cpuclk, hw);
> +       const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
> +       unsigned long alt_prate, alt_div, hi_rate;
> +
> +       pre->best_parent_hw = __clk_get_hw(cpuclk->alt_parent);
> +       alt_prate = clk_get_rate(cpuclk->alt_parent);

Should use clk_hw_get_rate here to remove lock recursion and since we
just got the parent hw.

> +       pre->best_parent_rate = alt_prate;
> +       hi_rate = max_t(unsigned long, next->rate, clk_hw_get_rate(hw));
> +
> +       /* Set dividers if we would go above the current or next rate. */
> +       if (alt_prate > hi_rate) {
> +               alt_div =  DIV_ROUND_UP(alt_prate, hi_rate);
> +               if (alt_div > reg_data->div_core_mask) {
> +                       pr_warn("%s: limiting alt-divider %lu to %d\n",
> +                               __func__, alt_div, reg_data->div_core_mask);
> +                       alt_div = reg_data->div_core_mask;
> +               }
> +
> +               pre->rate = alt_prate / alt_div;
> +       } else {
> +               pre->rate = alt_prate;
> +       }
> +
> +       return 1;
> +}
> +
> +static int rockchip_cpuclk_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct rockchip_cpuclk *cpuclk = container_of(hw,
> +                       struct rockchip_cpuclk, hw);
> +       const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(cpuclk->lock, flags);
> +       writel(HIWORD_UPDATE(index,
> +                            reg_data->mux_core_mask,
> +                            reg_data->mux_core_shift),
> +              cpuclk->reg_base + reg_data->core_reg);
> +       spin_unlock_irqrestore(cpuclk->lock, flags);
> +
> +       return 0;
> +}
> +
> +static u8 rockchip_cpuclk_get_parent(struct clk_hw *hw)
> +{
> +       struct rockchip_cpuclk *cpuclk = container_of(hw,
> +                       struct rockchip_cpuclk, hw);
> +       const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
> +       unsigned long flags;
> +       u32 val;
> +
> +       spin_lock_irqsave(cpuclk->lock, flags);
> +       val = readl_relaxed(cpuclk->reg_base + reg_data->core_reg);
> +       val >>= reg_data->mux_core_shift;
> +       val &= reg_data->mux_core_mask;
> +       spin_unlock_irqrestore(cpuclk->lock, flags);
> +
> +       return val;
> +}
>
>  static void rockchip_cpuclk_set_dividers(struct rockchip_cpuclk *cpuclk,
>                                 const struct rockchip_cpuclk_rate_table *rate)
> @@ -120,131 +187,92 @@ static void rockchip_cpuclk_set_dividers(struct rockchip_cpuclk *cpuclk,
>         }
>  }
>
> -static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk *cpuclk,
> -                                          struct clk_notifier_data *ndata)
> +static int rockchip_cpuclk_set_rate(struct clk_hw *hw, unsigned long rate,
> +                                       unsigned long parent_rate)
>  {
> +       struct rockchip_cpuclk *cpuclk = container_of(hw,
> +                       struct rockchip_cpuclk, hw);
>         const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
> -       const struct rockchip_cpuclk_rate_table *rate;
> -       unsigned long alt_prate, alt_div;
> -       unsigned long flags;
> +       const struct rockchip_cpuclk_rate_table *rate_divs;
> +       unsigned long div = (parent_rate / rate) - 1;
> +       unsigned long old_rate, flags;
>
> -       /* check validity of the new rate */
> -       rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
> -       if (!rate) {
> -               pr_err("%s: Invalid rate : %lu for cpuclk\n",
> -                      __func__, ndata->new_rate);
> +       if (div > reg_data->div_core_mask || rate > parent_rate) {
> +               pr_err("%s: Invalid rate : %lu %lu for cpuclk\n", __func__,
> +                               rate, parent_rate);
>                 return -EINVAL;
>         }
>
> -       alt_prate = clk_get_rate(cpuclk->alt_parent);
> -
> +       old_rate = clk_hw_get_rate(hw);
> +       rate_divs = rockchip_get_cpuclk_settings(cpuclk, rate);
>         spin_lock_irqsave(cpuclk->lock, flags);
> +       if (old_rate < rate)
> +               rockchip_cpuclk_set_dividers(cpuclk, rate_divs);
>
> -       /*
> -        * If the old parent clock speed is less than the clock speed
> -        * of the alternate parent, then it should be ensured that at no point
> -        * the armclk speed is more than the old_rate until the dividers are
> -        * set.
> -        */
> -       if (alt_prate > ndata->old_rate) {
> -               /* calculate dividers */
> -               alt_div =  DIV_ROUND_UP(alt_prate, ndata->old_rate) - 1;
> -               if (alt_div > reg_data->div_core_mask) {
> -                       pr_warn("%s: limiting alt-divider %lu to %d\n",
> -                               __func__, alt_div, reg_data->div_core_mask);
> -                       alt_div = reg_data->div_core_mask;
> -               }
> -
> -               /*
> -                * Change parents and add dividers in a single transaction.
> -                *
> -                * NOTE: we do this in a single transaction so we're never
> -                * dividing the primary parent by the extra dividers that were
> -                * needed for the alt.
> -                */
> -               pr_debug("%s: setting div %lu as alt-rate %lu > old-rate %lu\n",
> -                        __func__, alt_div, alt_prate, ndata->old_rate);
> -
> -               writel(HIWORD_UPDATE(alt_div, reg_data->div_core_mask,
> -                                             reg_data->div_core_shift) |
> -                      HIWORD_UPDATE(reg_data->mux_core_alt,
> -                                    reg_data->mux_core_mask,
> -                                    reg_data->mux_core_shift),
> -                      cpuclk->reg_base + reg_data->core_reg);
> -       } else {
> -               /* select alternate parent */
> -               writel(HIWORD_UPDATE(reg_data->mux_core_alt,
> -                                    reg_data->mux_core_mask,
> -                                    reg_data->mux_core_shift),
> -                      cpuclk->reg_base + reg_data->core_reg);
> -       }
> +       writel(HIWORD_UPDATE(div,
> +                            reg_data->div_core_mask,
> +                            reg_data->div_core_shift),
> +              cpuclk->reg_base + reg_data->core_reg);
> +       if (old_rate > rate)
> +               rockchip_cpuclk_set_dividers(cpuclk, rate_divs);
>
>         spin_unlock_irqrestore(cpuclk->lock, flags);
> +
>         return 0;
>  }
>
> -static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk *cpuclk,
> -                                           struct clk_notifier_data *ndata)
> +static int rockchip_cpuclk_set_rate_and_parent(struct clk_hw *hw,
> +                                       unsigned long rate,
> +                                       unsigned long parent_rate,
> +                                       u8 index)
>  {
> +       struct rockchip_cpuclk *cpuclk = container_of(hw,
> +                       struct rockchip_cpuclk, hw);
>         const struct rockchip_cpuclk_reg_data *reg_data = cpuclk->reg_data;
> -       const struct rockchip_cpuclk_rate_table *rate;
> -       unsigned long flags;
> +       const struct rockchip_cpuclk_rate_table *rate_divs;
> +       unsigned long div = (parent_rate / rate) - 1;
> +       unsigned long old_rate, flags;
>
> -       rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
> -       if (!rate) {
> -               pr_err("%s: Invalid rate : %lu for cpuclk\n",
> -                      __func__, ndata->new_rate);
> +       if (div > reg_data->div_core_mask || rate > parent_rate) {
> +               pr_err("%s: Invalid rate : %lu %lu for cpuclk\n", __func__,
> +                               rate, parent_rate);
>                 return -EINVAL;
>         }
>
> +       old_rate = clk_hw_get_rate(hw);
> +       rate_divs = rockchip_get_cpuclk_settings(cpuclk, rate);
>         spin_lock_irqsave(cpuclk->lock, flags);
> -
> -       if (ndata->old_rate < ndata->new_rate)
> -               rockchip_cpuclk_set_dividers(cpuclk, rate);
> -
>         /*
> -        * post-rate change event, re-mux to primary parent and remove dividers.
> -        *
> -        * NOTE: we do this in a single transaction so we're never dividing the
> -        * primary parent by the extra dividers that were needed for the alt.
> +        * TODO: This ain't great... Should change the get_cpuclk_settings code
> +        * to work with inexact matches to work with alt parent rates.
>          */
> -
> -       writel(HIWORD_UPDATE(0, reg_data->div_core_mask,
> -                               reg_data->div_core_shift) |
> -              HIWORD_UPDATE(reg_data->mux_core_main,
> -                               reg_data->mux_core_mask,
> -                               reg_data->mux_core_shift),
> +       if (old_rate < rate)
> +               rockchip_cpuclk_set_dividers(cpuclk, rate_divs);
> +
> +       writel(HIWORD_UPDATE(div,
> +                            reg_data->div_core_mask,
> +                            reg_data->div_core_shift) |
> +              HIWORD_UPDATE(index,
> +                            reg_data->mux_core_mask,
> +                            reg_data->mux_core_shift),
>                cpuclk->reg_base + reg_data->core_reg);
> -
> -       if (ndata->old_rate > ndata->new_rate)
> -               rockchip_cpuclk_set_dividers(cpuclk, rate);
> +       /* Not technically correct */
> +       if (old_rate > rate)
> +               rockchip_cpuclk_set_dividers(cpuclk, rate_divs);
>
>         spin_unlock_irqrestore(cpuclk->lock, flags);
> +
>         return 0;
>  }
>
> -/*
> - * This clock notifier is called when the frequency of the parent clock
> - * of cpuclk is to be changed. This notifier handles the setting up all
> - * the divider clocks, remux to temporary parent and handling the safe
> - * frequency levels when using temporary parent.
> - */
> -static int rockchip_cpuclk_notifier_cb(struct notifier_block *nb,
> -                                       unsigned long event, void *data)
> -{
> -       struct clk_notifier_data *ndata = data;
> -       struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_nb(nb);
> -       int ret = 0;
> -
> -       pr_debug("%s: event %lu, old_rate %lu, new_rate: %lu\n",
> -                __func__, event, ndata->old_rate, ndata->new_rate);
> -       if (event == PRE_RATE_CHANGE)
> -               ret = rockchip_cpuclk_pre_rate_change(cpuclk, ndata);
> -       else if (event == POST_RATE_CHANGE)
> -               ret = rockchip_cpuclk_post_rate_change(cpuclk, ndata);
> -
> -       return notifier_from_errno(ret);
> -}
> +static const struct clk_ops rockchip_cpuclk_ops = {
> +       .recalc_rate = rockchip_cpuclk_recalc_rate,
> +       .pre_rate_req = rockchip_cpuclk_pre_rate_req,
> +       .set_parent = rockchip_cpuclk_set_parent,
> +       .get_parent = rockchip_cpuclk_get_parent,
> +       .set_rate = rockchip_cpuclk_set_rate,
> +       .set_rate_and_parent = rockchip_cpuclk_set_rate_and_parent,
> +};
>
>  struct clk *rockchip_clk_register_cpuclk(const char *name,
>                         const char *const *parent_names, u8 num_parents,
> @@ -267,8 +295,8 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
>                 return ERR_PTR(-ENOMEM);
>
>         init.name = name;
> -       init.parent_names = &parent_names[reg_data->mux_core_main];
> -       init.num_parents = 1;
> +       init.parent_names = parent_names;
> +       init.num_parents = num_parents;
>         init.ops = &rockchip_cpuclk_ops;
>
>         /* only allow rate changes when we have a rate table */
> @@ -282,7 +310,6 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
>         cpuclk->reg_base = reg_base;
>         cpuclk->lock = lock;
>         cpuclk->reg_data = reg_data;
> -       cpuclk->clk_nb.notifier_call = rockchip_cpuclk_notifier_cb;
>         cpuclk->hw.init = &init;
>
>         cpuclk->alt_parent = __clk_lookup(parent_names[reg_data->mux_core_alt]);
> @@ -309,13 +336,6 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
>                 goto free_alt_parent;
>         }
>
> -       ret = clk_notifier_register(clk, &cpuclk->clk_nb);
> -       if (ret) {
> -               pr_err("%s: failed to register clock notifier for %s\n",
> -                               __func__, name);
> -               goto free_alt_parent;
> -       }
> -
>         if (nrates > 0) {
>                 cpuclk->rate_count = nrates;
>                 cpuclk->rate_table = kmemdup(rates,
> @@ -323,7 +343,7 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
>                                              GFP_KERNEL);
>                 if (!cpuclk->rate_table) {
>                         ret = -ENOMEM;
> -                       goto unregister_notifier;
> +                       goto free_alt_parent;
>                 }
>         }
>
> @@ -338,8 +358,6 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
>
>  free_rate_table:
>         kfree(cpuclk->rate_table);
> -unregister_notifier:
> -       clk_notifier_unregister(clk, &cpuclk->clk_nb);
>  free_alt_parent:
>         clk_disable_unprepare(cpuclk->alt_parent);
>  free_cpuclk:
> --
> 2.19.1.568.g152ad8e336-goog
>

Re: [PATCH 0/6] Coordinated Clks

[Stephen Boyd]

Quoting Derek Basehore (2018-10-23 18:31:26)
> Here's the first set of patches that I'm working on for the Common
> Clk Framework. Part of this patch series adds a new clk op,
> pre_rate_req. This is designed to replace the clk notifier approach
> that many clk drivers use right now to setup alt parents or temporary
> dividers. This should allow for the removal of the
> CLK_RECALC_NEW_RATES flag and the implementation of a better locking
> scheme for the prepare lock.

Are you going to resend this series? I can do a review but I wonder how
much it will change anyway.

Re: [PATCH 0/6] Coordinated Clks

[dbasehore .]

On Thu, Dec 20, 2018 at 1:15 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Derek Basehore (2018-10-23 18:31:26)
> > Here's the first set of patches that I'm working on for the Common
> > Clk Framework. Part of this patch series adds a new clk op,
> > pre_rate_req. This is designed to replace the clk notifier approach
> > that many clk drivers use right now to setup alt parents or temporary
> > dividers. This should allow for the removal of the
> > CLK_RECALC_NEW_RATES flag and the implementation of a better locking
> > scheme for the prepare lock.
>
> Are you going to resend this series? I can do a review but I wonder how
> much it will change anyway.
>

I'm going to resend next year. I got distracted trying to get other
code to work. This code has bugs in it, so a new patch set is
required.