clk: respect the clock dependencies in of_clk_init

Commit Message

Emilio López Feb. 7, 2014, 1:06 p.m. UTC

Hi all,

El 04/02/14 19:59, Gregory CLEMENT escribió:
> Until now the clock providers were initialized in the order found in
> the device tree. This led to have the dependencies between the clocks
> not respected: children clocks could be initialized before their
> parent clocks.
>
> Instead of forcing each platform to manage its own initialization order,
> this patch adds this work inside the framework itself.
>
> Using the data of the device tree the of_clk_init function now delayed
> the initialization of a clock provider if its parent provider was not
> ready yet.
>
> Signed-off-by: Gregory CLEMENT <[e-mail]>

After discussing this on IRC with Ezequiel, I must say I'm not really 
fond of this solution, as it adds unnecessary complexity to the 
framework and slows down clock registration to solve one particular 
problem that can be resolved in a simpler, less intrusive way.

The triggering issue which has resulted on this patch is really a simple 
dependency on the name of the parent clock - something that should not 
be an issue if DT is used to provide them (see of_clk_get_parent_name), 
and even when this is not the case, it should not require poking the 
clock tree to find out. It wouldn't be hard to fix however, even if you 
cannot modify your device trees to provide proper naming; a tiny patch 
like the one below should suffice (Ezequiel has kindly tested it today 
on a a370-rd and found it working correctly).

In other words, I feel this patch is a pretty good demonstration of 
over-engineering and bloating of an otherwise good framework to cover up 
for bugs/bad design on other parts of the kernel. Think about it; why 
does the framework require char *parent instead of struct clk *parent to 
link a child with their parent? Randomly-ordered registration is a 
pretty core principle of it; whether by design or not.

So, until someone has a real reason to enforce dependencies on clock 
registration time, I would like this to stay uncommited. And, if the 
time comes and something like this is ever needed, *please* make it 
optional (ie, have a special macro and separate table to register clocks 
which are dependency-sensitive, and do so after registering all the 
non-dependency-sensitive clocks).

Cheers,

Emilio

-----8<------

 From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[e-mail]>
Date: Thu, 6 Feb 2014 18:07:07 -0300
Subject: [PATCH] clk: mvebu: fix name dependency during registration time

Currently, mvebu_clk_gating_setup has a silly dependency on clock
registration order just to gather the parent clock name. This is
completely unnecesary, as it supports using an already provided name
via the clk_gating_soc_desc structs, and we can therefore solve this
issue with a 69+/- line patch. But, given that the parent name is
always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
just default-hardcode it here too and get away with solving this
problem with a one-liner.
---
  drivers/clk/mvebu/common.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

  	base = of_iomap(np, 0);

Comments

Jason Cooper Feb. 7, 2014, 2:24 p.m. UTC | #1

On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:

[snip a great explanation]

Guys, can I get some Tested-by's on this?

thx,

Jason.

> -----8<------
> 
> From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[e-mail]>
> Date: Thu, 6 Feb 2014 18:07:07 -0300
> Subject: [PATCH] clk: mvebu: fix name dependency during registration time
> 
> Currently, mvebu_clk_gating_setup has a silly dependency on clock
> registration order just to gather the parent clock name. This is
> completely unnecesary, as it supports using an already provided name
> via the clk_gating_soc_desc structs, and we can therefore solve this
> issue with a 69+/- line patch. But, given that the parent name is
> always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
> just default-hardcode it here too and get away with solving this
> problem with a one-liner.
> ---
>  drivers/clk/mvebu/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
> index 25ceccf..6c63b43 100644
> --- a/drivers/clk/mvebu/common.c
> +++ b/drivers/clk/mvebu/common.c
> @@ -121,7 +121,7 @@ void __init mvebu_clk_gating_setup(struct
> device_node *np,
>  	struct clk_gating_ctrl *ctrl;
>  	struct clk *clk;
>  	void __iomem *base;
> -	const char *default_parent = NULL;
> +	const char *default_parent = "tclk";
>  	int n;
> 
>  	base = of_iomap(np, 0);
> -- 
> 1.8.5.3

Ezequiel Garcia Feb. 7, 2014, 2:43 p.m. UTC | #2

On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
> 
> [snip a great explanation]
> 
> Guys, can I get some Tested-by's on this?
> 

In case someone missed Emilio's comment about it, I gave his oneliner
a test on A370 Reference Design. It worked just as well as Sebastian's.

Tested-by: Ezequiel Garcia <[e-mail]>

> > -----8<------
> > 
> > From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[e-mail]>
> > Date: Thu, 6 Feb 2014 18:07:07 -0300
> > Subject: [PATCH] clk: mvebu: fix name dependency during registration time
> > 
> > Currently, mvebu_clk_gating_setup has a silly dependency on clock
> > registration order just to gather the parent clock name. This is
> > completely unnecesary, as it supports using an already provided name
> > via the clk_gating_soc_desc structs, and we can therefore solve this
> > issue with a 69+/- line patch. But, given that the parent name is
> > always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
> > just default-hardcode it here too and get away with solving this
> > problem with a one-liner.
> > ---
> >  drivers/clk/mvebu/common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
> > index 25ceccf..6c63b43 100644
> > --- a/drivers/clk/mvebu/common.c
> > +++ b/drivers/clk/mvebu/common.c
> > @@ -121,7 +121,7 @@ void __init mvebu_clk_gating_setup(struct
> > device_node *np,
> >  	struct clk_gating_ctrl *ctrl;
> >  	struct clk *clk;
> >  	void __iomem *base;
> > -	const char *default_parent = NULL;
> > +	const char *default_parent = "tclk";
> >  	int n;
> > 
> >  	base = of_iomap(np, 0);
> > -- 
> > 1.8.5.3

Gregory CLEMENT Feb. 7, 2014, 2:49 p.m. UTC | #3

On 07/02/2014 15:43, Ezequiel Garcia wrote:
> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>
>> [snip a great explanation]
>>
>> Guys, can I get some Tested-by's on this?
>>
> 
> In case someone missed Emilio's comment about it, I gave his oneliner
> a test on A370 Reference Design. It worked just as well as Sebastian's.

Well ok it's working but this patch is not better than Sebastian, it is
even worth. I don't think it is a good idea at all to totally ignore the
information given by the device tree.

> 
> Tested-by: Ezequiel Garcia <[e-mail]>
> 
>>> -----8<------
>>>
>>> From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[e-mail]>
>>> Date: Thu, 6 Feb 2014 18:07:07 -0300
>>> Subject: [PATCH] clk: mvebu: fix name dependency during registration time
>>>
>>> Currently, mvebu_clk_gating_setup has a silly dependency on clock
>>> registration order just to gather the parent clock name. This is
>>> completely unnecesary, as it supports using an already provided name
>>> via the clk_gating_soc_desc structs, and we can therefore solve this
>>> issue with a 69+/- line patch. But, given that the parent name is
>>> always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
>>> just default-hardcode it here too and get away with solving this
>>> problem with a one-liner.
>>> ---
>>>  drivers/clk/mvebu/common.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
>>> index 25ceccf..6c63b43 100644
>>> --- a/drivers/clk/mvebu/common.c
>>> +++ b/drivers/clk/mvebu/common.c
>>> @@ -121,7 +121,7 @@ void __init mvebu_clk_gating_setup(struct
>>> device_node *np,
>>>  	struct clk_gating_ctrl *ctrl;
>>>  	struct clk *clk;
>>>  	void __iomem *base;
>>> -	const char *default_parent = NULL;
>>> +	const char *default_parent = "tclk";
>>>  	int n;
>>>
>>>  	base = of_iomap(np, 0);
>>> -- 
>>> 1.8.5.3
>

Emilio López Feb. 7, 2014, 3 p.m. UTC | #4

El 07/02/14 11:49, Gregory CLEMENT escribió:
> On 07/02/2014 15:43, Ezequiel Garcia wrote:
>> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>>
>>> [snip a great explanation]
>>>
>>> Guys, can I get some Tested-by's on this?
>>>
>>
>> In case someone missed Emilio's comment about it, I gave his oneliner
>> a test on A370 Reference Design. It worked just as well as Sebastian's.
>
> Well ok it's working but this patch is not better than Sebastian, it is
> even worth. I don't think it is a good idea at all to totally ignore the
> information given by the device tree.

With a bit more work, you can replace the clk_get magic with a call to 
of_clk_get_parent_name() or similar to be able to keep overriding stuff 
from DT. This way it would completely match the behaviour on 
mvebu_coreclk_setup (default to "tclk", allow overriding with DT).

Emilio

Gregory CLEMENT Feb. 7, 2014, 3:12 p.m. UTC | #5

On 07/02/2014 16:00, Emilio López wrote:
> El 07/02/14 11:49, Gregory CLEMENT escribió:
>> On 07/02/2014 15:43, Ezequiel Garcia wrote:
>>> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>>>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>>>
>>>> [snip a great explanation]
>>>>
>>>> Guys, can I get some Tested-by's on this?
>>>>
>>>
>>> In case someone missed Emilio's comment about it, I gave his oneliner
>>> a test on A370 Reference Design. It worked just as well as Sebastian's.
>>
>> Well ok it's working but this patch is not better than Sebastian, it is
>> even worth. I don't think it is a good idea at all to totally ignore the
>> information given by the device tree.
> 
> With a bit more work, you can replace the clk_get magic with a call to 
> of_clk_get_parent_name() or similar to be able to keep overriding stuff 
> from DT. This way it would completely match the behaviour on 
> mvebu_coreclk_setup (default to "tclk", allow overriding with DT).
> 

I think you didn't have a look on our implementation: the name of the clock
are created by the driver during the initialization. That's why we need that
the parent clock are initialized before the gating clock. I know that for the
sunxi clock you choose to list all your clock name in the device tree, but we
didn't make this choice on purpose. It is not as trivial as you suggested.

I didn't have a look on the atmel clocks, and I don't know if they have this
kind of issue, as they also have to deal with multiple parents, they may
have different issues.

Gregory

> Emilio
>

Sebastian Hesselbarth Feb. 7, 2014, 11:15 p.m. UTC | #6

On 02/07/2014 03:49 PM, Gregory CLEMENT wrote:
> On 07/02/2014 15:43, Ezequiel Garcia wrote:
>> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>>
>>> [snip a great explanation]
>>>
>>> Guys, can I get some Tested-by's on this?
>>>
>>
>> In case someone missed Emilio's comment about it, I gave his oneliner
>> a test on A370 Reference Design. It worked just as well as Sebastian's.
>
> Well ok it's working but this patch is not better than Sebastian, it is
> even worth. I don't think it is a good idea at all to totally ignore the

Tstststs.. Gregory please re-read the above slooooowly.. and think about
where you'd put me in.

Hint: "this patch", "not better than", "even worse". ;)

> information given by the device tree.

Actually, I have no strong opinion about how we fix the issue now. 
Emilio's patch is short and very suitable for a fix. I'll test later
this weekend.

For the long run, I definitely prefer probe ordering within clk frame
work or even better some early_device stuff.

>> Tested-by: Ezequiel Garcia <[e-mail]>
>>
>>>> -----8<------
>>>>
>>>>  From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
>>>> From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[e-mail]>
>>>> Date: Thu, 6 Feb 2014 18:07:07 -0300
>>>> Subject: [PATCH] clk: mvebu: fix name dependency during registration time
>>>>
>>>> Currently, mvebu_clk_gating_setup has a silly dependency on clock
>>>> registration order just to gather the parent clock name. This is

Of course, the dependency is not silly but we are just ignoring that
some clocks actually have a different parent clock. As long as we are
not interested in the frequency and they all depend on a fixed-clock
it makes no difference. Also, on some gates, we do some tweaking, e.g.
on Dove there is a clock gate for the GBE ip that we loop back to the
GBE PHY gate.. some day I may be so bored to get it straight.

>>>> completely unnecesary, as it supports using an already provided name
>>>> via the clk_gating_soc_desc structs, and we can therefore solve this
>>>> issue with a 69+/- line patch. But, given that the parent name is
>>>> always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
>>>> just default-hardcode it here too and get away with solving this
>>>> problem with a one-liner.

I agree with the default hard-coded "tclk" for now. But in general,
clocking can become very nasty and rather than coding the whole fscking
clock tree like the imx clock tree beast does, it would be much more
readable to be able separate the clock drivers into different parts.

Sebastian

>>>> ---
>>>>   drivers/clk/mvebu/common.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
>>>> index 25ceccf..6c63b43 100644
>>>> --- a/drivers/clk/mvebu/common.c
>>>> +++ b/drivers/clk/mvebu/common.c
>>>> @@ -121,7 +121,7 @@ void __init mvebu_clk_gating_setup(struct
>>>> device_node *np,
>>>>   	struct clk_gating_ctrl *ctrl;
>>>>   	struct clk *clk;
>>>>   	void __iomem *base;
>>>> -	const char *default_parent = NULL;
>>>> +	const char *default_parent = "tclk";
>>>>   	int n;
>>>>
>>>>   	base = of_iomap(np, 0);
>>>> --
>>>> 1.8.5.3

Patch

diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
index 25ceccf..6c63b43 100644
--- a/drivers/clk/mvebu/common.c
+++ b/drivers/clk/mvebu/common.c
@@ -121,7 +121,7 @@  void __init mvebu_clk_gating_setup(struct 
device_node *np,
  	struct clk_gating_ctrl *ctrl;
  	struct clk *clk;
  	void __iomem *base;
-	const char *default_parent = NULL;
+	const char *default_parent = "tclk";
  	int n;