[v3,0/2] clk: improve handling of orphan clocks

Commit Message

Stephen Boyd May 1, 2015, 8:52 p.m. UTC

On 05/01/15 12:59, Heiko Stübner wrote:
> Am Donnerstag, 30. April 2015, 17:19:01 schrieb Stephen Boyd:
>> On 04/22, Heiko Stuebner wrote:
>>> Using orphan clocks can introduce strange behaviour as they don't have
>>> rate information at all and also of course don't track
>>>
>>> This v2/v3 takes into account suggestions from Stephen Boyd to not try to
>>> walk the clock tree at runtime but instead keep track of orphan states
>>> on clock tree changes and making it mandatory for everybody from the
>>> start as orphaned clocks should not be used at all.
>>>
>>>
>>> This fixes an issue on most rk3288 platforms, where some soc-clocks
>>> are supplied by a 32khz clock from an external i2c-chip which often
>>> is only probed later in the boot process and maybe even after the
>>> drivers using these soc-clocks like the tsadc temperature sensor.
>>> In this case the driver using the clock should of course defer probing
>>> until the clock is actually usable.
>>>
>>>
>>> As this changes the behaviour for orphan clocks, it would of course
>>> benefit from more testing than on my Rockchip boards. To keep the
>>> recipent-list reasonable and not spam to much I selected one (the topmost)
>>> from the get_maintainer output of each drivers/clk entry.
>>> Hopefully some will provide Tested-by-tags :-)
>> <grumble> I don't see any Tested-by: tags yet </grumble>. I've
>> put these two patches on a separate branch "defer-orphans" and
>> pushed it to clk-next so we can give it some more exposure.
>>
>> Unfortunately this doesn't solve the orphan problem for non-OF
>> providers. What if we did the orphan check in __clk_create_clk()
>> instead and returned an error pointer for orphans? I suspect that
>> will solve all cases, right?
> hmm, clk_register also uses __clk_create_clk, which in turn would prevent
> registering orphan-clocks at all, I'd think.
> As on my platform I'm dependant on orphan clocks (the soc-level clock gets
> registerted as part of the big clock controller way before the i2c-based
> supplying clock), I'd rather not touch this :-) .

Have no fear! We should just change clk_register() to call a
__clk_create_clk() type function that doesn't check for orphan status.

>
> Instead I guess we could hook it less deep into clk_get_sys, like in the
> following patch?

It looks like it will work at least, but still I'd prefer to keep the
orphan check contained to clk.c. How about this compile tested only patch?

This also brings up an existing problem with clk_unregister() where
orphaned clocks are sitting out there useable by drivers when their
parent is unregistered. That code could use some work to atomically
switch all the orphaned clocks over to use the nodrv_ops.

----8<-----

Comments

Heiko Stübner May 1, 2015, 10:07 p.m. UTC | #1

Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> On 05/01/15 12:59, Heiko Stübner wrote:
> > Am Donnerstag, 30. April 2015, 17:19:01 schrieb Stephen Boyd:
> >> On 04/22, Heiko Stuebner wrote:
> >>> Using orphan clocks can introduce strange behaviour as they don't have
> >>> rate information at all and also of course don't track
> >>> 
> >>> This v2/v3 takes into account suggestions from Stephen Boyd to not try
> >>> to
> >>> walk the clock tree at runtime but instead keep track of orphan states
> >>> on clock tree changes and making it mandatory for everybody from the
> >>> start as orphaned clocks should not be used at all.
> >>> 
> >>> 
> >>> This fixes an issue on most rk3288 platforms, where some soc-clocks
> >>> are supplied by a 32khz clock from an external i2c-chip which often
> >>> is only probed later in the boot process and maybe even after the
> >>> drivers using these soc-clocks like the tsadc temperature sensor.
> >>> In this case the driver using the clock should of course defer probing
> >>> until the clock is actually usable.
> >>> 
> >>> 
> >>> As this changes the behaviour for orphan clocks, it would of course
> >>> benefit from more testing than on my Rockchip boards. To keep the
> >>> recipent-list reasonable and not spam to much I selected one (the
> >>> topmost)
> >>> from the get_maintainer output of each drivers/clk entry.
> >>> Hopefully some will provide Tested-by-tags :-)
> >> 
> >> <grumble> I don't see any Tested-by: tags yet </grumble>. I've
> >> put these two patches on a separate branch "defer-orphans" and
> >> pushed it to clk-next so we can give it some more exposure.
> >> 
> >> Unfortunately this doesn't solve the orphan problem for non-OF
> >> providers. What if we did the orphan check in __clk_create_clk()
> >> instead and returned an error pointer for orphans? I suspect that
> >> will solve all cases, right?
> > 
> > hmm, clk_register also uses __clk_create_clk, which in turn would prevent
> > registering orphan-clocks at all, I'd think.
> > As on my platform I'm dependant on orphan clocks (the soc-level clock gets
> > registerted as part of the big clock controller way before the i2c-based
> > supplying clock), I'd rather not touch this :-) .
> 
> Have no fear! We should just change clk_register() to call a
> __clk_create_clk() type function that doesn't check for orphan status.

ok :-D


> > Instead I guess we could hook it less deep into clk_get_sys, like in the
> > following patch?
> 
> It looks like it will work at least, but still I'd prefer to keep the
> orphan check contained to clk.c. How about this compile tested only patch?

I gave this a spin on my rk3288-firefly board. It still boots, the clock tree 
looks the same and it also still defers nicely in the scenario I needed it 
for. The implementation also looks nice - and of course much more compact than 
my check in two places :-) . I don't know if you want to put this as follow-up 
on top or fold it into the original orphan-check, so in any case

Tested-by: Heiko Stuebner <[e-mail]>
Reviewed-by: Heiko Stuebner <[e-mail]>


> This also brings up an existing problem with clk_unregister() where
> orphaned clocks are sitting out there useable by drivers when their
> parent is unregistered. That code could use some work to atomically
> switch all the orphaned clocks over to use the nodrv_ops.

Not sure I understand this correctly yet, but when these children get 
orphaned, switched to the clk_nodrv_ops, they won't get their original ops 
back if the parent reappears.

So I guess we would need to store the original ops in secondary property of 
struct clk_core and I guess simply bind the ops-switch to the orphan state 
update?


> 
> ----8<-----
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 30d45c657a07..1d23daa42dd2 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2221,14 +2221,6 @@ static inline void clk_debug_unregister(struct
> clk_core *core) }
>  #endif
> 
> -static bool clk_is_orphan(const struct clk *clk)
> -{
> -	if (!clk)
> -		return false;
> -
> -	return clk->core->orphan;
> -}
> -
>  /**
>   * __clk_init - initialize the data structures in a struct clk
>   * @dev:	device initializing this clk, placeholder for now
> @@ -2420,15 +2412,11 @@ out:
>  	return ret;
>  }
> 
> -struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
> -			     const char *con_id)
> +static struct clk *clk_hw_create_clk(struct clk_hw *hw, const char *dev_id,
> +				     const char *con_id)
>  {
>  	struct clk *clk;
> 
> -	/* This is to allow this function to be chained to others */
> -	if (!hw || IS_ERR(hw))
> -		return (struct clk *) hw;
> -
>  	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>  	if (!clk)
>  		return ERR_PTR(-ENOMEM);
> @@ -2445,6 +2433,19 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const
> char *dev_id, return clk;
>  }
> 
> +struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
> +			     const char *con_id)
> +{
> +	/* This is to allow this function to be chained to others */
> +	if (!hw || IS_ERR(hw))
> +		return (struct clk *) hw;
> +
> +	if (hw->core->orphan)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	return clk_hw_create_clk(hw, dev_id, con_id);
> +}
> +
>  void __clk_free_clk(struct clk *clk)
>  {
>  	clk_prepare_lock();
> @@ -2511,7 +2512,7 @@ struct clk *clk_register(struct device *dev, struct
> clk_hw *hw)
> 
>  	INIT_HLIST_HEAD(&core->clks);
> 
> -	hw->clk = __clk_create_clk(hw, NULL, NULL);
> +	hw->clk = clk_hw_create_clk(hw, NULL, NULL);
>  	if (IS_ERR(hw->clk)) {
>  		ret = PTR_ERR(hw->clk);
>  		goto fail_parent_names_copy;
> @@ -2958,10 +2959,6 @@ struct clk *__of_clk_get_from_provider(struct
> of_phandle_args *clkspec, if (provider->node == clkspec->np)
>  			clk = provider->get(clkspec, provider->data);
>  		if (!IS_ERR(clk)) {
> -			if (clk_is_orphan(clk)) {
> -				clk = ERR_PTR(-EPROBE_DEFER);
> -				break;
> -			}
> 
>  			clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
>  					       con_id);

Stephen Boyd May 1, 2015, 11:40 p.m. UTC | #2

On 05/01/15 15:07, Heiko Stübner wrote:
> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>
>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>>> following patch?
>> It looks like it will work at least, but still I'd prefer to keep the
>> orphan check contained to clk.c. How about this compile tested only patch?
> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree 
> looks the same and it also still defers nicely in the scenario I needed it 
> for. The implementation also looks nice - and of course much more compact than 
> my check in two places :-) . I don't know if you want to put this as follow-up 
> on top or fold it into the original orphan-check, so in any case
>
> Tested-by: Heiko Stuebner <[e-mail]>
> Reviewed-by: Heiko Stuebner <[e-mail]>

Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
my patch and a note that it's based on an earlier patch from you.

>
>
>> This also brings up an existing problem with clk_unregister() where
>> orphaned clocks are sitting out there useable by drivers when their
>> parent is unregistered. That code could use some work to atomically
>> switch all the orphaned clocks over to use the nodrv_ops.
> Not sure I understand this correctly yet, but when these children get 
> orphaned, switched to the clk_nodrv_ops, they won't get their original ops 
> back if the parent reappears.
>
> So I guess we would need to store the original ops in secondary property of 
> struct clk_core and I guess simply bind the ops-switch to the orphan state 
> update?

Yep. We'll need to store away the original ops in case we need to put
them back. Don't feel obligated to fix this either. It would certainly
be nice if someone tried to fix this case at some point, but it's not
like things are any worse off right now.

Tero Kristo May 7, 2015, 8:22 a.m. UTC | #3

On 05/02/2015 02:40 AM, Stephen Boyd wrote:
> On 05/01/15 15:07, Heiko Stübner wrote:
>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>
>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>>>> following patch?
>>> It looks like it will work at least, but still I'd prefer to keep the
>>> orphan check contained to clk.c. How about this compile tested only patch?
>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
>> looks the same and it also still defers nicely in the scenario I needed it
>> for. The implementation also looks nice - and of course much more compact than
>> my check in two places :-) . I don't know if you want to put this as follow-up
>> on top or fold it into the original orphan-check, so in any case
>>
>> Tested-by: Heiko Stuebner <[e-mail]>
>> Reviewed-by: Heiko Stuebner <[e-mail]>
>
> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> my patch and a note that it's based on an earlier patch from you.

FWIW, just gave a try for these two patches on all TI boards I have 
access to.

Tested-by: Tero Kristo <[e-mail]>

I didn't try your evolved patch though, as you don't seem to have made 
your mind yet.

-Tero

>
>>
>>
>>> This also brings up an existing problem with clk_unregister() where
>>> orphaned clocks are sitting out there useable by drivers when their
>>> parent is unregistered. That code could use some work to atomically
>>> switch all the orphaned clocks over to use the nodrv_ops.
>> Not sure I understand this correctly yet, but when these children get
>> orphaned, switched to the clk_nodrv_ops, they won't get their original ops
>> back if the parent reappears.
>>
>> So I guess we would need to store the original ops in secondary property of
>> struct clk_core and I guess simply bind the ops-switch to the orphan state
>> update?
>
> Yep. We'll need to store away the original ops in case we need to put
> them back. Don't feel obligated to fix this either. It would certainly
> be nice if someone tried to fix this case at some point, but it's not
> like things are any worse off right now.
>

Kevin Hilman May 7, 2015, 3:17 p.m. UTC | #4

On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[e-mail]> wrote:
> On 05/01/15 15:07, Heiko Stübner wrote:
>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>
>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>>>> following patch?
>>> It looks like it will work at least, but still I'd prefer to keep the
>>> orphan check contained to clk.c. How about this compile tested only patch?
>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
>> looks the same and it also still defers nicely in the scenario I needed it
>> for. The implementation also looks nice - and of course much more compact than
>> my check in two places :-) . I don't know if you want to put this as follow-up
>> on top or fold it into the original orphan-check, so in any case
>>
>> Tested-by: Heiko Stuebner <[e-mail]>
>> Reviewed-by: Heiko Stuebner <[e-mail]>
>
> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> my patch and a note that it's based on an earlier patch from you.

It appears this has landed in linux-next in the form of 882667c1fcf1
clk: prevent orphan clocks from being used.  A bunch of boot failures
for sunxi in today's linux-next[1] were bisected down to that patch.

I confirmed that reverting that commit on top of next/master gets
sunxi booting again.

Kevin

[1] http://kernelci.org/boot/all/job/next/kernel/next-20150507/

Stephen Boyd May 7, 2015, 6:18 p.m. UTC | #5

On 05/07/15 01:22, Tero Kristo wrote:
> On 05/02/2015 02:40 AM, Stephen Boyd wrote:
>> On 05/01/15 15:07, Heiko Stübner wrote:
>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>>
>>>>> Instead I guess we could hook it less deep into clk_get_sys, like
>>>>> in the
>>>>> following patch?
>>>> It looks like it will work at least, but still I'd prefer to keep the
>>>> orphan check contained to clk.c. How about this compile tested only
>>>> patch?
>>> I gave this a spin on my rk3288-firefly board. It still boots, the
>>> clock tree
>>> looks the same and it also still defers nicely in the scenario I
>>> needed it
>>> for. The implementation also looks nice - and of course much more
>>> compact than
>>> my check in two places :-) . I don't know if you want to put this as
>>> follow-up
>>> on top or fold it into the original orphan-check, so in any case
>>>
>>> Tested-by: Heiko Stuebner <[e-mail]>
>>> Reviewed-by: Heiko Stuebner <[e-mail]>
>>
>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
>> my patch and a note that it's based on an earlier patch from you.
>
> FWIW, just gave a try for these two patches on all TI boards I have
> access to.
>
> Tested-by: Tero Kristo <[e-mail]>
>
> I didn't try your evolved patch though, as you don't seem to have made
> your mind yet.
>

Thanks. Can you try the evolved patch? It's in linux-next now as commit
882667c1fcf1, and it seems to at least break sunxi boot. I'd be
interested if it broke TI boards.

Tero Kristo May 8, 2015, 11:41 a.m. UTC | #6

On 05/07/2015 09:18 PM, Stephen Boyd wrote:
> On 05/07/15 01:22, Tero Kristo wrote:
>> On 05/02/2015 02:40 AM, Stephen Boyd wrote:
>>> On 05/01/15 15:07, Heiko Stübner wrote:
>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>>>
>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like
>>>>>> in the
>>>>>> following patch?
>>>>> It looks like it will work at least, but still I'd prefer to keep the
>>>>> orphan check contained to clk.c. How about this compile tested only
>>>>> patch?
>>>> I gave this a spin on my rk3288-firefly board. It still boots, the
>>>> clock tree
>>>> looks the same and it also still defers nicely in the scenario I
>>>> needed it
>>>> for. The implementation also looks nice - and of course much more
>>>> compact than
>>>> my check in two places :-) . I don't know if you want to put this as
>>>> follow-up
>>>> on top or fold it into the original orphan-check, so in any case
>>>>
>>>> Tested-by: Heiko Stuebner <[e-mail]>
>>>> Reviewed-by: Heiko Stuebner <[e-mail]>
>>>
>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
>>> my patch and a note that it's based on an earlier patch from you.
>>
>> FWIW, just gave a try for these two patches on all TI boards I have
>> access to.
>>
>> Tested-by: Tero Kristo <[e-mail]>
>>
>> I didn't try your evolved patch though, as you don't seem to have made
>> your mind yet.
>>
>
> Thanks. Can you try the evolved patch? It's in linux-next now as commit
> 882667c1fcf1, and it seems to at least break sunxi boot. I'd be
> interested if it broke TI boards.

Just tried it out, boots fine on all these:

   : Board           : Boot commit                    log
  1: am335x-evm      : PASS 4.1.0-rc2-next-20150507   am335x-evm.txt
  2: am335x-evmsk    : PASS 4.1.0-rc2-next-20150507   am335x-sk.txt
  3: am3517-evm      : PASS 4.1.0-rc2-next-20150507   am3517-evm.txt
  4: am43x-epos-evm  : PASS 4.1.0-rc2-next-20150507   am43xx-epos.txt
  5: am437x-gp-evm   : PASS 4.1.0-rc2-next-20150507   am43xx-gpevm.txt
  6: am57xx-evm      : PASS 4.1.0-rc2-next-20150507   am57xx-evm.txt
  7: omap3-beagle-xm : PASS 4.1.0-rc2-next-20150507   beagleboard.txt
  8: omap3-beagle    : PASS 4.1.0-rc2-next-20150507 
beagleboard-vanilla.txt
  9: am335x-boneblack: PASS 4.1.0-rc2-next-20150507   beaglebone-black.txt
10: am335x-bone     : PASS 4.1.0-rc2-next-20150507   beaglebone.txt
11: dra7xx-evm      : PASS 4.1.0-rc2-next-20150507   dra7xx-evm.txt
12: omap3-n900      : PASS 4.1.0-rc2-next-20150507   n900.txt
13: omap5-uevm      : PASS 4.1.0-rc2-next-20150507   omap5-evm.txt
14: omap4-panda-es  : PASS 4.1.0-rc2-next-20150507   pandaboard-es.txt
15: omap4-panda     : PASS 4.1.0-rc2-next-20150507   pandaboard-vanilla.txt
16: omap2430-sdp    : PASS 4.1.0-rc2-next-20150507   sdp2430.txt
17: omap3430-sdp    : PASS 4.1.0-rc2-next-20150507   sdp3430.txt
18: omap4-sdp-es23plus: PASS 4.1.0-rc2-next-20150507   sdp4430.txt
TOTAL = 18 boards, Booted Boards = 18, No Boot boards = 0

TI boards do not have any orphan clocks.

-Tero

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 30d45c657a07..1d23daa42dd2 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2221,14 +2221,6 @@  static inline void clk_debug_unregister(struct clk_core *core)
 }
 #endif
 
-static bool clk_is_orphan(const struct clk *clk)
-{
-	if (!clk)
-		return false;
-
-	return clk->core->orphan;
-}
-
 /**
  * __clk_init - initialize the data structures in a struct clk
  * @dev:	device initializing this clk, placeholder for now
@@ -2420,15 +2412,11 @@  out:
 	return ret;
 }
 
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
-			     const char *con_id)
+static struct clk *clk_hw_create_clk(struct clk_hw *hw, const char *dev_id,
+				     const char *con_id)
 {
 	struct clk *clk;
 
-	/* This is to allow this function to be chained to others */
-	if (!hw || IS_ERR(hw))
-		return (struct clk *) hw;
-
 	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 	if (!clk)
 		return ERR_PTR(-ENOMEM);
@@ -2445,6 +2433,19 @@  struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
 	return clk;
 }
 
+struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
+			     const char *con_id)
+{
+	/* This is to allow this function to be chained to others */
+	if (!hw || IS_ERR(hw))
+		return (struct clk *) hw;
+
+	if (hw->core->orphan)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	return clk_hw_create_clk(hw, dev_id, con_id);
+}
+
 void __clk_free_clk(struct clk *clk)
 {
 	clk_prepare_lock();
@@ -2511,7 +2512,7 @@  struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 
 	INIT_HLIST_HEAD(&core->clks);
 
-	hw->clk = __clk_create_clk(hw, NULL, NULL);
+	hw->clk = clk_hw_create_clk(hw, NULL, NULL);
 	if (IS_ERR(hw->clk)) {
 		ret = PTR_ERR(hw->clk);
 		goto fail_parent_names_copy;
@@ -2958,10 +2959,6 @@  struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 		if (provider->node == clkspec->np)
 			clk = provider->get(clkspec, provider->data);
 		if (!IS_ERR(clk)) {
-			if (clk_is_orphan(clk)) {
-				clk = ERR_PTR(-EPROBE_DEFER);
-				break;
-			}
 
 			clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
 					       con_id);