Linux-Amlogic Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] clk: walk orphan list on clock provider registration
@ 2019-11-29 16:16 Jerome Brunet
  2019-12-02 18:19 ` Stephen Boyd
  0 siblings, 1 reply; 3+ messages in thread
From: Jerome Brunet @ 2019-11-29 16:16 UTC (permalink / raw)
  To: Stephen Boyd, Michael Turquette
  Cc: Kevin Hilman, linux-amlogic, linux-clk, linux-kernel, Jerome Brunet

So far, we walked the orphan list every time a new clock was registered
in CCF. This was fine since the clocks were only referenced by name.

Now that the clock can be referenced through DT, it is not enough:
* Controller A register first a reference clocks from controller B
  through DT.
* Controller B register all its clocks then register the provider.

Each time controller B registers a new clock, the orphan list is walked
but it can't match since the provider is registered yet. When the
provider is finally registered, the orphan list is not walked unless
another clock is registered afterward.

This can lead to situation where some clocks remain orphaned even if
the parent is available.

Walking the orphan list on provider registration solves the problem.

Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ef4416721777..917ba37c3b9d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3249,6 +3249,34 @@ static inline void clk_debug_unregister(struct clk_core *core)
 }
 #endif
 
+static void __clk_core_reparent_orphan(void)
+{
+	struct clk_core *orphan;
+	struct hlist_node *tmp2;
+
+	/*
+	 * walk the list of orphan clocks and reparent any that newly finds a
+	 * parent.
+	 */
+	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
+		struct clk_core *parent = __clk_init_parent(orphan);
+
+		/*
+		 * We need to use __clk_set_parent_before() and _after() to
+		 * to properly migrate any prepare/enable count of the orphan
+		 * clock. This is important for CLK_IS_CRITICAL clocks, which
+		 * are enabled during init but might not have a parent yet.
+		 */
+		if (parent) {
+			/* update the clk tree topology */
+			__clk_set_parent_before(orphan, parent);
+			__clk_set_parent_after(orphan, parent, NULL);
+			__clk_recalc_accuracies(orphan);
+			__clk_recalc_rates(orphan, 0);
+		}
+	}
+}
+
 /**
  * __clk_core_init - initialize the data structures in a struct clk_core
  * @core:	clk_core being initialized
@@ -3259,8 +3287,6 @@ static inline void clk_debug_unregister(struct clk_core *core)
 static int __clk_core_init(struct clk_core *core)
 {
 	int ret;
-	struct clk_core *orphan;
-	struct hlist_node *tmp2;
 	unsigned long rate;
 
 	if (!core)
@@ -3416,27 +3442,8 @@ static int __clk_core_init(struct clk_core *core)
 		clk_enable_unlock(flags);
 	}
 
-	/*
-	 * walk the list of orphan clocks and reparent any that newly finds a
-	 * parent.
-	 */
-	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
-		struct clk_core *parent = __clk_init_parent(orphan);
+	__clk_core_reparent_orphan();
 
-		/*
-		 * We need to use __clk_set_parent_before() and _after() to
-		 * to properly migrate any prepare/enable count of the orphan
-		 * clock. This is important for CLK_IS_CRITICAL clocks, which
-		 * are enabled during init but might not have a parent yet.
-		 */
-		if (parent) {
-			/* update the clk tree topology */
-			__clk_set_parent_before(orphan, parent);
-			__clk_set_parent_after(orphan, parent, NULL);
-			__clk_recalc_accuracies(orphan);
-			__clk_recalc_rates(orphan, 0);
-		}
-	}
 
 	kref_init(&core->ref);
 out:
@@ -4288,6 +4295,10 @@ int of_clk_add_provider(struct device_node *np,
 	mutex_unlock(&of_clk_mutex);
 	pr_debug("Added clock from %pOF\n", np);
 
+	clk_prepare_lock();
+	__clk_core_reparent_orphan();
+	clk_prepare_unlock();
+
 	ret = of_clk_set_defaults(np, true);
 	if (ret < 0)
 		of_clk_del_provider(np);
@@ -4323,6 +4334,10 @@ int of_clk_add_hw_provider(struct device_node *np,
 	mutex_unlock(&of_clk_mutex);
 	pr_debug("Added clk_hw provider from %pOF\n", np);
 
+	clk_prepare_lock();
+	__clk_core_reparent_orphan();
+	clk_prepare_unlock();
+
 	ret = of_clk_set_defaults(np, true);
 	if (ret < 0)
 		of_clk_del_provider(np);
-- 
2.23.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] clk: walk orphan list on clock provider registration
  2019-11-29 16:16 [PATCH] clk: walk orphan list on clock provider registration Jerome Brunet
@ 2019-12-02 18:19 ` Stephen Boyd
  2019-12-02 18:59   ` Jerome Brunet
  0 siblings, 1 reply; 3+ messages in thread
From: Stephen Boyd @ 2019-12-02 18:19 UTC (permalink / raw)
  To: Jerome Brunet, Michael Turquette
  Cc: Kevin Hilman, linux-amlogic, linux-clk, linux-kernel, Jerome Brunet

Quoting Jerome Brunet (2019-11-29 08:16:58)
> So far, we walked the orphan list every time a new clock was registered
> in CCF. This was fine since the clocks were only referenced by name.
> 
> Now that the clock can be referenced through DT, it is not enough:
> * Controller A register first a reference clocks from controller B
>   through DT.
> * Controller B register all its clocks then register the provider.
> 
> Each time controller B registers a new clock, the orphan list is walked
> but it can't match since the provider is registered yet. When the
> provider is finally registered, the orphan list is not walked unless
> another clock is registered afterward.
> 
> This can lead to situation where some clocks remain orphaned even if
> the parent is available.
> 
> Walking the orphan list on provider registration solves the problem.
> 
> Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---

Sounds right. Thanks for making the fix!

I suspect there should be a reported-by tag though?

>  drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index ef4416721777..917ba37c3b9d 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3249,6 +3249,34 @@ static inline void clk_debug_unregister(struct clk_core *core)
>  }
>  #endif
>  
> +static void __clk_core_reparent_orphan(void)

Maybe drop the double underscore. clk_core prefix already means "private
to this file".

> +{
> +       struct clk_core *orphan;
> +       struct hlist_node *tmp2;
> +
> +       /*
> +        * walk the list of orphan clocks and reparent any that newly finds a
> +        * parent.
> +        */
> +       hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
> +               struct clk_core *parent = __clk_init_parent(orphan);
> +
> +               /*
> +                * We need to use __clk_set_parent_before() and _after() to
> +                * to properly migrate any prepare/enable count of the orphan
> +                * clock. This is important for CLK_IS_CRITICAL clocks, which
> +                * are enabled during init but might not have a parent yet.
> +                */
> +               if (parent) {
> +                       /* update the clk tree topology */
> +                       __clk_set_parent_before(orphan, parent);
> +                       __clk_set_parent_after(orphan, parent, NULL);
> +                       __clk_recalc_accuracies(orphan);
> +                       __clk_recalc_rates(orphan, 0);
> +               }
> +       }
> +}
> +
>  /**
>   * __clk_core_init - initialize the data structures in a struct clk_core
>   * @core:      clk_core being initialized
> @@ -3259,8 +3287,6 @@ static inline void clk_debug_unregister(struct clk_core *core)
>  static int __clk_core_init(struct clk_core *core)
>  {
>         int ret;
> -       struct clk_core *orphan;
> -       struct hlist_node *tmp2;
>         unsigned long rate;
>  
>         if (!core)
> @@ -3416,27 +3442,8 @@ static int __clk_core_init(struct clk_core *core)
>                 clk_enable_unlock(flags);
>         }
>  
> -       /*
> -        * walk the list of orphan clocks and reparent any that newly finds a
> -        * parent.
> -        */
> -       hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
> -               struct clk_core *parent = __clk_init_parent(orphan);
> +       __clk_core_reparent_orphan();
>  
> -               /*
> -                * We need to use __clk_set_parent_before() and _after() to
> -                * to properly migrate any prepare/enable count of the orphan
> -                * clock. This is important for CLK_IS_CRITICAL clocks, which
> -                * are enabled during init but might not have a parent yet.
> -                */
> -               if (parent) {
> -                       /* update the clk tree topology */
> -                       __clk_set_parent_before(orphan, parent);
> -                       __clk_set_parent_after(orphan, parent, NULL);
> -                       __clk_recalc_accuracies(orphan);
> -                       __clk_recalc_rates(orphan, 0);
> -               }
> -       }
>  
>         kref_init(&core->ref);
>  out:
> @@ -4288,6 +4295,10 @@ int of_clk_add_provider(struct device_node *np,
>         mutex_unlock(&of_clk_mutex);
>         pr_debug("Added clock from %pOF\n", np);
>  
> +       clk_prepare_lock();
> +       __clk_core_reparent_orphan();
> +       clk_prepare_unlock();
> +

Maybe make a locked version of this function and an unlocked version?

>         ret = of_clk_set_defaults(np, true);
>         if (ret < 0)
>                 of_clk_del_provider(np);
> @@ -4323,6 +4334,10 @@ int of_clk_add_hw_provider(struct device_node *np,
>         mutex_unlock(&of_clk_mutex);
>         pr_debug("Added clk_hw provider from %pOF\n", np);
>  
> +       clk_prepare_lock();
> +       __clk_core_reparent_orphan();
> +       clk_prepare_unlock();
> +

So we don't duplicate this twice.

>         ret = of_clk_set_defaults(np, true);
>         if (ret < 0)

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] clk: walk orphan list on clock provider registration
  2019-12-02 18:19 ` Stephen Boyd
@ 2019-12-02 18:59   ` Jerome Brunet
  0 siblings, 0 replies; 3+ messages in thread
From: Jerome Brunet @ 2019-12-02 18:59 UTC (permalink / raw)
  To: Stephen Boyd, Michael Turquette
  Cc: Kevin Hilman, linux-clk, linux-kernel, linux-amlogic


On Mon 02 Dec 2019 at 19:19, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2019-11-29 08:16:58)
>> So far, we walked the orphan list every time a new clock was registered
>> in CCF. This was fine since the clocks were only referenced by name.
>> 
>> Now that the clock can be referenced through DT, it is not enough:
>> * Controller A register first a reference clocks from controller B
>>   through DT.
>> * Controller B register all its clocks then register the provider.
>> 
>> Each time controller B registers a new clock, the orphan list is walked
>> but it can't match since the provider is registered yet. When the
>> provider is finally registered, the orphan list is not walked unless
>> another clock is registered afterward.
>> 
>> This can lead to situation where some clocks remain orphaned even if
>> the parent is available.
>> 
>> Walking the orphan list on provider registration solves the problem.
>> 
>> Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names")
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>
> Sounds right. Thanks for making the fix!
>
> I suspect there should be a reported-by tag though?

laboriously, yes

>
>>  drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++------------------
>>  1 file changed, 37 insertions(+), 22 deletions(-)
>> 
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index ef4416721777..917ba37c3b9d 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -3249,6 +3249,34 @@ static inline void clk_debug_unregister(struct clk_core *core)
>>  }
>>  #endif
>>  
>> +static void __clk_core_reparent_orphan(void)
>
> Maybe drop the double underscore. clk_core prefix already means "private
> to this file".

I've done it this way just to keep coherent with __clk_core_init(),
which the code was extracted from

In the end, I don't mind so I'll drop it in the v2

>
>> +{
>> +       struct clk_core *orphan;
>> +       struct hlist_node *tmp2;
>> +
>> +       /*
>> +        * walk the list of orphan clocks and reparent any that newly finds a
>> +        * parent.
>> +        */
>> +       hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
>> +               struct clk_core *parent = __clk_init_parent(orphan);
>> +
>> +               /*
>> +                * We need to use __clk_set_parent_before() and _after() to
>> +                * to properly migrate any prepare/enable count of the orphan
>> +                * clock. This is important for CLK_IS_CRITICAL clocks, which
>> +                * are enabled during init but might not have a parent yet.
>> +                */
>> +               if (parent) {
>> +                       /* update the clk tree topology */
>> +                       __clk_set_parent_before(orphan, parent);
>> +                       __clk_set_parent_after(orphan, parent, NULL);
>> +                       __clk_recalc_accuracies(orphan);
>> +                       __clk_recalc_rates(orphan, 0);
>> +               }
>> +       }
>> +}
>> +
>>  /**
>>   * __clk_core_init - initialize the data structures in a struct clk_core
>>   * @core:      clk_core being initialized
>> @@ -3259,8 +3287,6 @@ static inline void clk_debug_unregister(struct clk_core *core)
>>  static int __clk_core_init(struct clk_core *core)
>>  {
>>         int ret;
>> -       struct clk_core *orphan;
>> -       struct hlist_node *tmp2;
>>         unsigned long rate;
>>  
>>         if (!core)
>> @@ -3416,27 +3442,8 @@ static int __clk_core_init(struct clk_core *core)
>>                 clk_enable_unlock(flags);
>>         }
>>  
>> -       /*
>> -        * walk the list of orphan clocks and reparent any that newly finds a
>> -        * parent.
>> -        */
>> -       hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
>> -               struct clk_core *parent = __clk_init_parent(orphan);
>> +       __clk_core_reparent_orphan();
>>  
>> -               /*
>> -                * We need to use __clk_set_parent_before() and _after() to
>> -                * to properly migrate any prepare/enable count of the orphan
>> -                * clock. This is important for CLK_IS_CRITICAL clocks, which
>> -                * are enabled during init but might not have a parent yet.
>> -                */
>> -               if (parent) {
>> -                       /* update the clk tree topology */
>> -                       __clk_set_parent_before(orphan, parent);
>> -                       __clk_set_parent_after(orphan, parent, NULL);
>> -                       __clk_recalc_accuracies(orphan);
>> -                       __clk_recalc_rates(orphan, 0);
>> -               }
>> -       }
>>  
>>         kref_init(&core->ref);
>>  out:
>> @@ -4288,6 +4295,10 @@ int of_clk_add_provider(struct device_node *np,
>>         mutex_unlock(&of_clk_mutex);
>>         pr_debug("Added clock from %pOF\n", np);
>>  
>> +       clk_prepare_lock();
>> +       __clk_core_reparent_orphan();
>> +       clk_prepare_unlock();
>> +
>
> Maybe make a locked version of this function and an unlocked version?
>
>>         ret = of_clk_set_defaults(np, true);
>>         if (ret < 0)
>>                 of_clk_del_provider(np);
>> @@ -4323,6 +4334,10 @@ int of_clk_add_hw_provider(struct device_node *np,
>>         mutex_unlock(&of_clk_mutex);
>>         pr_debug("Added clk_hw provider from %pOF\n", np);
>>  
>> +       clk_prepare_lock();
>> +       __clk_core_reparent_orphan();
>> +       clk_prepare_unlock();
>> +
>
> So we don't duplicate this twice.
>

Sure.

>>         ret = of_clk_set_defaults(np, true);
>>         if (ret < 0)


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 16:16 [PATCH] clk: walk orphan list on clock provider registration Jerome Brunet
2019-12-02 18:19 ` Stephen Boyd
2019-12-02 18:59   ` Jerome Brunet

Linux-Amlogic Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-amlogic/0 linux-amlogic/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-amlogic linux-amlogic/ https://lore.kernel.org/linux-amlogic \
		linux-amlogic@lists.infradead.org
	public-inbox-index linux-amlogic

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-amlogic


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git