[PATCH 0/4] clk: mvebu: fix clk init order

[PATCH 0/4] clk: mvebu: fix clk init order

[Sebastian Hesselbarth]

This patch set fixes clk init order that went upside-down with
v3.14. I haven't really investigated what caused this, but I assume
it is related with DT node reordering by addresses.

Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
registered before core clocks driver. Unfortunately, we cannot
return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
init order for our drivers is always core clocks before clock gating,
we maintain init order ourselves by hooking CLK_OF_DECLARE to one
init function that will register core clocks before clock gating
driver.

This patch is based on pre-v3.14-rc1 mainline and should go in as
fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
I suggest Jason picks it up as a topic branch.

The patches have been boot tested on Dove and compile-tested only
for Kirkwood, Armada 370 and XP.

Sebastian Hesselbarth (4):
  clk: mvebu: armada-370: maintain clock init order
  clk: mvebu: armada-xp: maintain clock init order
  clk: mvebu: dove: maintain clock init order
  clk: mvebu: kirkwood: maintain clock init order

 drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
 drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
 drivers/clk/mvebu/dove.c       | 19 +++++++++----------
 drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
 4 files changed, 44 insertions(+), 50 deletions(-)

---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
-- 
1.8.5.2

[PATCH 1/4] clk: mvebu: armada-370: maintain clock init order

[Sebastian Hesselbarth]

Init order of CLK_OF_DECLARE'd drivers depends on compile order.
Unfortunately, clk_of_init does not allow drivers to return errors,
e.g. -EPROBE_DEFER if parent clocks have not been registered, yet.

To avoid init order woes for MVEBU clock drivers, we take care of
proper init order ourselves. This patch joins core-clk and gating-clk
init to maintain proper init order.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/mvebu/armada-370.c b/drivers/clk/mvebu/armada-370.c
index 81a202d12a7a..bef198a83863 100644
--- a/drivers/clk/mvebu/armada-370.c
+++ b/drivers/clk/mvebu/armada-370.c
@@ -141,13 +141,6 @@ static const struct coreclk_soc_desc a370_coreclks = {
 	.num_ratios = ARRAY_SIZE(a370_coreclk_ratios),
 };
 
-static void __init a370_coreclk_init(struct device_node *np)
-{
-	mvebu_coreclk_setup(np, &a370_coreclks);
-}
-CLK_OF_DECLARE(a370_core_clk, "marvell,armada-370-core-clock",
-	       a370_coreclk_init);
-
 /*
  * Clock Gating Control
  */
@@ -168,9 +161,15 @@ static const struct clk_gating_soc_desc a370_gating_desc[] __initconst = {
 	{ }
 };
 
-static void __init a370_clk_gating_init(struct device_node *np)
+static void __init a370_clk_init(struct device_node *np)
 {
-	mvebu_clk_gating_setup(np, a370_gating_desc);
+	struct device_node *cgnp =
+		of_find_compatible_node(NULL, NULL, "marvell,armada-370-gating-clock");
+
+	mvebu_coreclk_setup(np, &a370_coreclks);
+
+	if (cgnp)
+		mvebu_clk_gating_setup(cgnp, a370_gating_desc);
 }
-CLK_OF_DECLARE(a370_clk_gating, "marvell,armada-370-gating-clock",
-	       a370_clk_gating_init);
+CLK_OF_DECLARE(a370_clk, "marvell,armada-370-core-clock", a370_clk_init);
+
-- 
1.8.5.2

[PATCH 2/4] clk: mvebu: armada-xp: maintain clock init order

[Sebastian Hesselbarth]

Init order of CLK_OF_DECLARE'd drivers depends on compile order.
Unfortunately, clk_of_init does not allow drivers to return errors,
e.g. -EPROBE_DEFER if parent clocks have not been registered, yet.

To avoid init order woes for MVEBU clock drivers, we take care of
proper init order ourselves. This patch joins core-clk and gating-clk
init to maintain proper init order.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/clk/mvebu/armada-xp.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/mvebu/armada-xp.c b/drivers/clk/mvebu/armada-xp.c
index 9922c4475aa8..b3094315a3c0 100644
--- a/drivers/clk/mvebu/armada-xp.c
+++ b/drivers/clk/mvebu/armada-xp.c
@@ -158,13 +158,6 @@ static const struct coreclk_soc_desc axp_coreclks = {
 	.num_ratios = ARRAY_SIZE(axp_coreclk_ratios),
 };
 
-static void __init axp_coreclk_init(struct device_node *np)
-{
-	mvebu_coreclk_setup(np, &axp_coreclks);
-}
-CLK_OF_DECLARE(axp_core_clk, "marvell,armada-xp-core-clock",
-	       axp_coreclk_init);
-
 /*
  * Clock Gating Control
  */
@@ -202,9 +195,14 @@ static const struct clk_gating_soc_desc axp_gating_desc[] __initconst = {
 	{ }
 };
 
-static void __init axp_clk_gating_init(struct device_node *np)
+static void __init axp_clk_init(struct device_node *np)
 {
-	mvebu_clk_gating_setup(np, axp_gating_desc);
+	struct device_node *cgnp =
+		of_find_compatible_node(NULL, NULL, "marvell,armada-xp-gating-clock");
+
+	mvebu_coreclk_setup(np, &axp_coreclks);
+
+	if (cgnp)
+		mvebu_clk_gating_setup(cgnp, axp_gating_desc);
 }
-CLK_OF_DECLARE(axp_clk_gating, "marvell,armada-xp-gating-clock",
-	       axp_clk_gating_init);
+CLK_OF_DECLARE(axp_clk, "marvell,armada-xp-core-clock", axp_clk_init);
-- 
1.8.5.2

[PATCH 3/4] clk: mvebu: dove: maintain clock init order

[Sebastian Hesselbarth]

Init order of CLK_OF_DECLARE'd drivers depends on compile order.
Unfortunately, clk_of_init does not allow drivers to return errors,
e.g. -EPROBE_DEFER if parent clocks have not been registered, yet.

To avoid init order woes for MVEBU clock drivers, we take care of
proper init order ourselves. This patch joins core-clk and gating-clk
init to maintain proper init order.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/clk/mvebu/dove.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/mvebu/dove.c b/drivers/clk/mvebu/dove.c
index 38aee1e3f242..b8c2424ac926 100644
--- a/drivers/clk/mvebu/dove.c
+++ b/drivers/clk/mvebu/dove.c
@@ -154,12 +154,6 @@ static const struct coreclk_soc_desc dove_coreclks = {
 	.num_ratios = ARRAY_SIZE(dove_coreclk_ratios),
 };
 
-static void __init dove_coreclk_init(struct device_node *np)
-{
-	mvebu_coreclk_setup(np, &dove_coreclks);
-}
-CLK_OF_DECLARE(dove_core_clk, "marvell,dove-core-clock", dove_coreclk_init);
-
 /*
  * Clock Gating Control
  */
@@ -186,9 +180,14 @@ static const struct clk_gating_soc_desc dove_gating_desc[] __initconst = {
 	{ }
 };
 
-static void __init dove_clk_gating_init(struct device_node *np)
+static void __init dove_clk_init(struct device_node *np)
 {
-	mvebu_clk_gating_setup(np, dove_gating_desc);
+	struct device_node *cgnp =
+		of_find_compatible_node(NULL, NULL, "marvell,dove-gating-clock");
+
+	mvebu_coreclk_setup(np, &dove_coreclks);
+
+	if (cgnp)
+		mvebu_clk_gating_setup(cgnp, dove_gating_desc);
 }
-CLK_OF_DECLARE(dove_clk_gating, "marvell,dove-gating-clock",
-	       dove_clk_gating_init);
+CLK_OF_DECLARE(dove_clk, "marvell,dove-core-clock", dove_clk_init);
-- 
1.8.5.2

[PATCH 4/4] clk: mvebu: kirkwood: maintain clock init order

[Sebastian Hesselbarth]

Init order of CLK_OF_DECLARE'd drivers depends on compile order.
Unfortunately, clk_of_init does not allow drivers to return errors,
e.g. -EPROBE_DEFER if parent clocks have not been registered, yet.

To avoid init order woes for MVEBU clock drivers, we take care of
proper init order ourselves. This patch joins core-clk and gating-clk
init to maintain proper init order.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/clk/mvebu/kirkwood.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/mvebu/kirkwood.c b/drivers/clk/mvebu/kirkwood.c
index 2636a55f29f9..ddb666a86500 100644
--- a/drivers/clk/mvebu/kirkwood.c
+++ b/drivers/clk/mvebu/kirkwood.c
@@ -193,13 +193,6 @@ static const struct coreclk_soc_desc kirkwood_coreclks = {
 	.num_ratios = ARRAY_SIZE(kirkwood_coreclk_ratios),
 };
 
-static void __init kirkwood_coreclk_init(struct device_node *np)
-{
-	mvebu_coreclk_setup(np, &kirkwood_coreclks);
-}
-CLK_OF_DECLARE(kirkwood_core_clk, "marvell,kirkwood-core-clock",
-	       kirkwood_coreclk_init);
-
 static const struct coreclk_soc_desc mv88f6180_coreclks = {
 	.get_tclk_freq = kirkwood_get_tclk_freq,
 	.get_cpu_freq = mv88f6180_get_cpu_freq,
@@ -208,13 +201,6 @@ static const struct coreclk_soc_desc mv88f6180_coreclks = {
 	.num_ratios = ARRAY_SIZE(kirkwood_coreclk_ratios),
 };
 
-static void __init mv88f6180_coreclk_init(struct device_node *np)
-{
-	mvebu_coreclk_setup(np, &mv88f6180_coreclks);
-}
-CLK_OF_DECLARE(mv88f6180_core_clk, "marvell,mv88f6180-core-clock",
-	       mv88f6180_coreclk_init);
-
 /*
  * Clock Gating Control
  */
@@ -239,9 +225,21 @@ static const struct clk_gating_soc_desc kirkwood_gating_desc[] __initconst = {
 	{ }
 };
 
-static void __init kirkwood_clk_gating_init(struct device_node *np)
+static void __init kirkwood_clk_init(struct device_node *np)
 {
-	mvebu_clk_gating_setup(np, kirkwood_gating_desc);
+	struct device_node *cgnp =
+		of_find_compatible_node(NULL, NULL, "marvell,kirkwood-gating-clock");
+
+
+	if (of_device_is_compatible(np, "marvell,mv88f6180-core-clock"))
+		mvebu_coreclk_setup(np, &mv88f6180_coreclks);
+	else
+		mvebu_coreclk_setup(np, &kirkwood_coreclks);
+
+	if (cgnp)
+		mvebu_clk_gating_setup(cgnp, kirkwood_gating_desc);
 }
-CLK_OF_DECLARE(kirkwood_clk_gating, "marvell,kirkwood-gating-clock",
-	       kirkwood_clk_gating_init);
+CLK_OF_DECLARE(kirkwood_clk, "marvell,kirkwood-core-clock",
+	       kirkwood_clk_init);
+CLK_OF_DECLARE(mv88f6180_clk, "marvell,mv88f6180-core-clock",
+	       kirkwood_clk_init);
-- 
1.8.5.2

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Emilio López]

Hello Sebastian,

El 25/01/14 15:19, Sebastian Hesselbarth escribió:
> This patch set fixes clk init order that went upside-down with
> v3.14. I haven't really investigated what caused this, but I assume
> it is related with DT node reordering by addresses.

The framework should be able to deal with unordered registration. I am 
not very familiar with the mvebu driver though, do you have a valid 
reason to require a specific order?

> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> registered before core clocks driver. Unfortunately, we cannot
> return -EPROBE_DEFER in drivers initialized by clk_of_init.

Why would you need to do so? After a quick inspection on the code, I see 
you may have problems on mvebu_clk_gating_setup() when getting the 
default parent clock name, but I believe you could solve it in an easier 
way by using of_clk_get_parent_name().

Cheers,

Emilio

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Sebastian Hesselbarth]

On 01/25/2014 10:32 PM, Emilio López wrote:
> El 25/01/14 15:19, Sebastian Hesselbarth escribió:
>> This patch set fixes clk init order that went upside-down with
>> v3.14. I haven't really investigated what caused this, but I assume
>> it is related with DT node reordering by addresses.
>
> The framework should be able to deal with unordered registration. I am
> not very familiar with the mvebu driver though, do you have a valid
> reason to require a specific order?

Emilio,

I rather think that everthing registered with CLK_OF_DECLARE cannot
deal with unordered registration. The callback passed to CLK_OF_DECLARE
has to have void as return value, so there is no way to pass errors,
e.g. -EPROBE_DEFER, back to of_clk_init.

The reason for this ordering is that the clock gates depend on core
clocks. It is always that way, so merging both init functions isn't
that odd.

>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
>> registered before core clocks driver. Unfortunately, we cannot
>> return -EPROBE_DEFER in drivers initialized by clk_of_init.
>
> Why would you need to do so? After a quick inspection on the code, I see
> you may have problems on mvebu_clk_gating_setup() when getting the
> default parent clock name, but I believe you could solve it in an easier
> way by using of_clk_get_parent_name().

Ok, I'll look if using of_clk_get_parent_name will help here. But again,
I can see that clk-gating driver gets registered before core-clk driver.
There may be no code to give you the parent name at that time.

Sebastian

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Emilio López]

Sebastian,

El 25/01/14 18:44, Sebastian Hesselbarth escribió:
> On 01/25/2014 10:32 PM, Emilio López wrote:
>> El 25/01/14 15:19, Sebastian Hesselbarth escribió:
>>> This patch set fixes clk init order that went upside-down with
>>> v3.14. I haven't really investigated what caused this, but I assume
>>> it is related with DT node reordering by addresses.
>>
>> The framework should be able to deal with unordered registration. I am
>> not very familiar with the mvebu driver though, do you have a valid
>> reason to require a specific order?
>
> Emilio,
>
> I rather think that everthing registered with CLK_OF_DECLARE cannot
> deal with unordered registration. The callback passed to CLK_OF_DECLARE
> has to have void as return value, so there is no way to pass errors,
> e.g. -EPROBE_DEFER, back to of_clk_init.

Indeed. What I meant is that the framework works fine if you first 
register a child clock that refers to a not yet registered parent, and 
then register the parent. The registration need not be strictly ordered.

> The reason for this ordering is that the clock gates depend on core
> clocks. It is always that way, so merging both init functions isn't
> that odd.

If your only dependency is the parent name, and you can use DT or 
something else to get it, then you don't need to enforce an order.

>>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
>>> registered before core clocks driver. Unfortunately, we cannot
>>> return -EPROBE_DEFER in drivers initialized by clk_of_init.
>>
>> Why would you need to do so? After a quick inspection on the code, I see
>> you may have problems on mvebu_clk_gating_setup() when getting the
>> default parent clock name, but I believe you could solve it in an easier
>> way by using of_clk_get_parent_name().
>
> Ok, I'll look if using of_clk_get_parent_name will help here. But again,
> I can see that clk-gating driver gets registered before core-clk driver.
> There may be no code to give you the parent name at that time.

After looking at some of the armada*.dtsi, I see you don't list the 
clock names on the coreclk node, so of_clk_get_parent_name may not be of 
much value after all.

Cheers,

Emilio

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Ezequiel Garcia]

Hi Emilio,

Thanks for your help with this.

On Sat, Jan 25, 2014 at 07:11:07PM -0300, Emilio López wrote:
[..]
> >
> > Ok, I'll look if using of_clk_get_parent_name will help here. But again,
> > I can see that clk-gating driver gets registered before core-clk driver.
> > There may be no code to give you the parent name at that time.
> 
> After looking at some of the armada*.dtsi, I see you don't list the 
> clock names on the coreclk node, so of_clk_get_parent_name may not be of 
> much value after all.
> 

IIRC, we faced a similar issue with the Core Divider clock and solved it by
specifying the clock names in the DT.

I meant to complete the core and gating clocks in a similar way
(providing names on the DT), but apparently (as discussed with Gregory Clement)
Mike Turquette and others are planning to remove the clock names from
the DT entirely.

Can you guys explain about this plan a bit further? Or do you think we
should specify the names on the DT for all the clocks instead?

Notice that the latter would remove lots of strings from the kernel
itself (right?)
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Thomas Petazzoni]

Dear Sebastian Hesselbarth,

On Sat, 25 Jan 2014 19:19:06 +0100, Sebastian Hesselbarth wrote:
> This patch set fixes clk init order that went upside-down with
> v3.14. I haven't really investigated what caused this, but I assume
> it is related with DT node reordering by addresses.
> 
> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> registered before core clocks driver. Unfortunately, we cannot
> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> init order for our drivers is always core clocks before clock gating,
> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> init function that will register core clocks before clock gating
> driver.
> 
> This patch is based on pre-v3.14-rc1 mainline and should go in as
> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> I suggest Jason picks it up as a topic branch.

I'm not sure I really like the solution you're proposing here. I'd very
much prefer to keep one CLK_OF_DECLARE() per clock type, associated to
one function registering only this clock type.

Instead, shouldn't the clock framework be improved to *not* register a
clock until its parent have been registered? If the DT you have the
gatable clocks that depend on the core clocks, then the gatable clocks
should not be registered if the core clocks have not yet been
registered.

Do you think this is possible? Am I missing something here?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Sebastian Hesselbarth]

On 01/27/14 15:39, Thomas Petazzoni wrote:
> On Sat, 25 Jan 2014 19:19:06 +0100, Sebastian Hesselbarth wrote:
>> This patch set fixes clk init order that went upside-down with
>> v3.14. I haven't really investigated what caused this, but I assume
>> it is related with DT node reordering by addresses.
>>
>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
>> registered before core clocks driver. Unfortunately, we cannot
>> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
>> init order for our drivers is always core clocks before clock gating,
>> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
>> init function that will register core clocks before clock gating
>> driver.
>>
>> This patch is based on pre-v3.14-rc1 mainline and should go in as
>> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
>> I suggest Jason picks it up as a topic branch.
>
> I'm not sure I really like the solution you're proposing here. I'd very
> much prefer to keep one CLK_OF_DECLARE() per clock type, associated to
> one function registering only this clock type.

Have you ever had a look at e.g. clk-imx28.c? Not that I really like
the approach, but it is common practice to do so.

> Instead, shouldn't the clock framework be improved to *not* register a
> clock until its parent have been registered? If the DT you have the
> gatable clocks that depend on the core clocks, then the gatable clocks
> should not be registered if the core clocks have not yet been
> registered.
>
> Do you think this is possible? Am I missing something here?

As I said, clk_of_init does not care about return values from the
clock init functions. Without it, it cannot decide if a clock
driver failed horribly, failed because of missing dependencies, or
successfully installed all clocks. Also, it is early stuff and I guess
clk_of_init will have to build its own "defered_list" and loop over
until done.

BTW, this is a fix not an improvement. We should find an acceptable
solution soon. But I am still open for suggestions, too.

Sebastian

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Jason Cooper]

On Mon, Jan 27, 2014 at 07:21:38PM +0100, Sebastian Hesselbarth wrote:
> On 01/27/14 15:39, Thomas Petazzoni wrote:
> >On Sat, 25 Jan 2014 19:19:06 +0100, Sebastian Hesselbarth wrote:
> >>This patch set fixes clk init order that went upside-down with
> >>v3.14. I haven't really investigated what caused this, but I assume
> >>it is related with DT node reordering by addresses.
> >>
> >>Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> >>registered before core clocks driver. Unfortunately, we cannot
> >>return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> >>init order for our drivers is always core clocks before clock gating,
> >>we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> >>init function that will register core clocks before clock gating
> >>driver.
> >>
> >>This patch is based on pre-v3.14-rc1 mainline and should go in as
> >>fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> >>I suggest Jason picks it up as a topic branch.
> >
> >I'm not sure I really like the solution you're proposing here. I'd very
> >much prefer to keep one CLK_OF_DECLARE() per clock type, associated to
> >one function registering only this clock type.
> 
> Have you ever had a look at e.g. clk-imx28.c? Not that I really like
> the approach, but it is common practice to do so.
> 
> >Instead, shouldn't the clock framework be improved to *not* register a
> >clock until its parent have been registered? If the DT you have the
> >gatable clocks that depend on the core clocks, then the gatable clocks
> >should not be registered if the core clocks have not yet been
> >registered.
> >
> >Do you think this is possible? Am I missing something here?
> 
> As I said, clk_of_init does not care about return values from the
> clock init functions. Without it, it cannot decide if a clock
> driver failed horribly, failed because of missing dependencies, or
> successfully installed all clocks. Also, it is early stuff and I guess
> clk_of_init will have to build its own "defered_list" and loop over
> until done.
> 
> BTW, this is a fix not an improvement. We should find an acceptable
> solution soon. But I am still open for suggestions, too.

fyi: I suspect this may be the problem currently experienced by Kevin on
mirabox in the boot farm.  He sees it on current master.  Adding him to
the Cc.

thx,

Jason.

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Gregory CLEMENT]

Hi Sebastian,

On 25/01/2014 19:19, Sebastian Hesselbarth wrote:
> This patch set fixes clk init order that went upside-down with
> v3.14. I haven't really investigated what caused this, but I assume
> it is related with DT node reordering by addresses.

Can you explain what kind of issue do you observe?

I have just tested the master branch of Linus and (excepted for SATA
but Andrew will send a patch set soon), I didn't experiment any
issues on Armada 370 and Armada XP based boards.

Thanks,

Gregory


> 
> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> registered before core clocks driver. Unfortunately, we cannot
> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> init order for our drivers is always core clocks before clock gating,
> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> init function that will register core clocks before clock gating
> driver.
> 
> This patch is based on pre-v3.14-rc1 mainline and should go in as
> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> I suggest Jason picks it up as a topic branch.
> 
> The patches have been boot tested on Dove and compile-tested only
> for Kirkwood, Armada 370 and XP.
> 
> Sebastian Hesselbarth (4):
>   clk: mvebu: armada-370: maintain clock init order
>   clk: mvebu: armada-xp: maintain clock init order
>   clk: mvebu: dove: maintain clock init order
>   clk: mvebu: kirkwood: maintain clock init order
> 
>  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
>  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
>  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
>  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
>  4 files changed, 44 insertions(+), 50 deletions(-)
> 
> ---
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Sebastian Hesselbarth]

On 01/30/14 11:24, Gregory CLEMENT wrote:
> On 25/01/2014 19:19, Sebastian Hesselbarth wrote:
>> This patch set fixes clk init order that went upside-down with
>> v3.14. I haven't really investigated what caused this, but I assume
>> it is related with DT node reordering by addresses.
>
> Can you explain what kind of issue do you observe?

Sure. When probing CLK_OF_DECLAREed clock drivers, clock-gating driver
gets registered before core-clocks. It therefore cannot resolve it's
parent clock name for tclk and all clock gates will have no parent
clock.

Usually, you'll see in some drivers (e.g. v643xx_eth) div_by_zero errors
poping up, when they calculate a frequency division factors based on
clock gate frequency, which should have been tclk but is 0 now.

> I have just tested the master branch of Linus and (excepted for SATA
> but Andrew will send a patch set soon), I didn't experiment any
> issues on Armada 370 and Armada XP based boards.

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Willy Tarreau]

Hi Sebastian,

On Thu, Jan 30, 2014 at 11:31:32AM +0100, Sebastian Hesselbarth wrote:
> On 01/30/14 11:24, Gregory CLEMENT wrote:
> >On 25/01/2014 19:19, Sebastian Hesselbarth wrote:
> >>This patch set fixes clk init order that went upside-down with
> >>v3.14. I haven't really investigated what caused this, but I assume
> >>it is related with DT node reordering by addresses.
> >
> >Can you explain what kind of issue do you observe?
> 
> Sure. When probing CLK_OF_DECLAREed clock drivers, clock-gating driver
> gets registered before core-clocks. It therefore cannot resolve it's
> parent clock name for tclk and all clock gates will have no parent
> clock.
> 
> Usually, you'll see in some drivers (e.g. v643xx_eth) div_by_zero errors
> poping up, when they calculate a frequency division factors based on
> clock gate frequency, which should have been tclk but is 0 now.

Well, to be honnest, I have no idea whether your patch is the right way
to fix the problem or not, but what I can say is that it fixes such oopses
that appear in 3.14-rc1 when booting on mirabox :

Division by zero in kernel.
CPU: 0 PID: 6 Comm: kworker/u2:0 Not tainted 3.13.0-bck-mbx #6
Workqueue: kmmcd mmc_rescan
[<c0010865>] (unwind_backtrace+0x1/0x98) from [<c000e947>] (show_stack+0xb/0xc)
[<c000e947>] (show_stack+0xb/0xc) from [<c0135913>] (Ldiv0+0x9/0x12)
[<c0135913>] (Ldiv0+0x9/0x12) from [<c01f708b>] (mvsd_set_ios+0xcb/0x160)
[<c01f708b>] (mvsd_set_ios+0xcb/0x160) from [<c01ec1fd>] (__mmc_set_clock+0x2d/0
x40)
[<c01ec1fd>] (__mmc_set_clock+0x2d/0x40) from [<c01f1a59>] (mmc_sdio_init_card+0
x391/0x808)
[<c01f1a59>] (mmc_sdio_init_card+0x391/0x808) from [<c01f2163>] (mmc_attach_sdio
+0x5b/0x218)
[<c01f2163>] (mmc_attach_sdio+0x5b/0x218) from [<c01ed0d9>] (mmc_rescan+0x159/0x
1b4)
[<c01ed0d9>] (mmc_rescan+0x159/0x1b4) from [<c0024579>] (process_one_work+0xa9/0
x21c)
[<c0024579>] (process_one_work+0xa9/0x21c) from [<c002494d>] (worker_thread+0xb5
/0x248)
[<c002494d>] (worker_thread+0xb5/0x248) from [<c0027f1b>] (kthread+0x7b/0x94)
+0xa7/0x138)
[<c04228b3>] (kernel_init_freeable+0xa7/0x138) from [<c027e6cf>] (kernel_init+0x
7/0xb8)
[<c027e6cf>] (kernel_init+0x7/0xb8) from [<c000cb9d>] (ret_from_fork+0x11/0x34)
mvsdio d00d4000.mvsdio: lacking card detect (fall back to polling)

By the way, seeing how often a trick related to the DT is nedeed to solve an
oops or a panic, I'm really scared that this whole DT mess is just becoming
the exact copy of the ACPI mess (but 15 years later) and we'll experience the
same horrible things :-( Sometimes I'm wondering whether there are not too
many structural things put in there...

Regards,
Willy

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Sebastian Hesselbarth]

On 02/04/2014 12:16 AM, Willy Tarreau wrote:
> On Thu, Jan 30, 2014 at 11:31:32AM +0100, Sebastian Hesselbarth wrote:
>> On 01/30/14 11:24, Gregory CLEMENT wrote:
>>> On 25/01/2014 19:19, Sebastian Hesselbarth wrote:
>>>> This patch set fixes clk init order that went upside-down with
>>>> v3.14. I haven't really investigated what caused this, but I assume
>>>> it is related with DT node reordering by addresses.
>>>
>>> Can you explain what kind of issue do you observe?
>>
>> Sure. When probing CLK_OF_DECLAREed clock drivers, clock-gating driver
>> gets registered before core-clocks. It therefore cannot resolve it's
>> parent clock name for tclk and all clock gates will have no parent
>> clock.
>>
>> Usually, you'll see in some drivers (e.g. v643xx_eth) div_by_zero errors
>> poping up, when they calculate a frequency division factors based on
>> clock gate frequency, which should have been tclk but is 0 now.
>
> Well, to be honnest, I have no idea whether your patch is the right way
> to fix the problem or not, but what I can say is that it fixes such oopses
> that appear in 3.14-rc1 when booting on mirabox :
>
> Division by zero in kernel.

Willy,

you have hit exactly the reason for this patch.

[...]
> By the way, seeing how often a trick related to the DT is nedeed to solve an
> oops or a panic, I'm really scared that this whole DT mess is just becoming
> the exact copy of the ACPI mess (but 15 years later) and we'll experience the
> same horrible things :-( Sometimes I'm wondering whether there are not too
> many structural things put in there...

To be precise, it is not a DT-related trick at all. You would have the
same issues, if you'd register those "low-level" (i.e. early) drivers
without DT. It is more about missing init ordering, here.

There could be different ways to work this out, even elevating clock
devices to "normal" probed devices could be possible. I am sure, in the
long run, it will work out, but now this is a fix for v3.14-rc1.

@Jason, Andrew, Gregory, Thomas:
Now that v3.14 is out, anything against taking this in as fixes for rc1?

Sebastian

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Gregory CLEMENT]

On 04/02/2014 00:36, Sebastian Hesselbarth wrote:
> On 02/04/2014 12:16 AM, Willy Tarreau wrote:
>> On Thu, Jan 30, 2014 at 11:31:32AM +0100, Sebastian Hesselbarth wrote:
>>> On 01/30/14 11:24, Gregory CLEMENT wrote:
>>>> On 25/01/2014 19:19, Sebastian Hesselbarth wrote:
>>>>> This patch set fixes clk init order that went upside-down with
>>>>> v3.14. I haven't really investigated what caused this, but I assume
>>>>> it is related with DT node reordering by addresses.
>>>>
>>>> Can you explain what kind of issue do you observe?
>>>
>>> Sure. When probing CLK_OF_DECLAREed clock drivers, clock-gating driver
>>> gets registered before core-clocks. It therefore cannot resolve it's
>>> parent clock name for tclk and all clock gates will have no parent
>>> clock.
>>>
>>> Usually, you'll see in some drivers (e.g. v643xx_eth) div_by_zero errors
>>> poping up, when they calculate a frequency division factors based on
>>> clock gate frequency, which should have been tclk but is 0 now.
>>
>> Well, to be honnest, I have no idea whether your patch is the right way
>> to fix the problem or not, but what I can say is that it fixes such oopses
>> that appear in 3.14-rc1 when booting on mirabox :
>>
>> Division by zero in kernel.
> 
> Willy,
> 
> you have hit exactly the reason for this patch.
> 
> [...]
>> By the way, seeing how often a trick related to the DT is nedeed to solve an
>> oops or a panic, I'm really scared that this whole DT mess is just becoming
>> the exact copy of the ACPI mess (but 15 years later) and we'll experience the
>> same horrible things :-( Sometimes I'm wondering whether there are not too
>> many structural things put in there...
> 
> To be precise, it is not a DT-related trick at all. You would have the
> same issues, if you'd register those "low-level" (i.e. early) drivers
> without DT. It is more about missing init ordering, here.
> 
> There could be different ways to work this out, even elevating clock
> devices to "normal" probed devices could be possible. I am sure, in the
> long run, it will work out, but now this is a fix for v3.14-rc1.
> 
> @Jason, Andrew, Gregory, Thomas:
> Now that v3.14 is out, anything against taking this in as fixes for rc1?

Hi Sebastian,

I am not found of this solution I still think it should be done
at framework level. However we still have this very annoying issue,
and this fix is better than nothing. So I am not against taking this
for rc1 with the hope that it will be later revert with a better
solution.


Thanks,

Gregory


> 
> Sebastian
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Thomas Petazzoni]

Hello,

On Tue, 04 Feb 2014 15:58:44 +0100, Gregory CLEMENT wrote:

> > @Jason, Andrew, Gregory, Thomas:
> > Now that v3.14 is out, anything against taking this in as fixes for rc1?
> 
> I am not found of this solution I still think it should be done
> at framework level. However we still have this very annoying issue,
> and this fix is better than nothing. So I am not against taking this
> for rc1 with the hope that it will be later revert with a better
> solution.

Same opinion here. I'm fine with this solution as a temporary measure,
but it would be good to solve this problem in a nicer way. Also, making
this change doesn't impact the DT in any way, there is no problem in
having a temporary not perfect solution, and improve it later on.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Mike Turquette]

Quoting Sebastian Hesselbarth (2014-01-25 10:19:06)
> This patch set fixes clk init order that went upside-down with
> v3.14. I haven't really investigated what caused this, but I assume
> it is related with DT node reordering by addresses.
> 
> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> registered before core clocks driver. Unfortunately, we cannot
> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> init order for our drivers is always core clocks before clock gating,
> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> init function that will register core clocks before clock gating
> driver.
> 
> This patch is based on pre-v3.14-rc1 mainline and should go in as
> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> I suggest Jason picks it up as a topic branch.

Sebastian,

These patches look OK to me. I'd rather take Gregory Clement's "respect
the clock dependencies in of_clk_init" patch towards 3.15, so this fix
will still be necessary for the current -rc's.

Jason, will you be sending a PR?

Thanks,
Mike

> 
> The patches have been boot tested on Dove and compile-tested only
> for Kirkwood, Armada 370 and XP.
> 
> Sebastian Hesselbarth (4):
>   clk: mvebu: armada-370: maintain clock init order
>   clk: mvebu: armada-xp: maintain clock init order
>   clk: mvebu: dove: maintain clock init order
>   clk: mvebu: kirkwood: maintain clock init order
> 
>  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
>  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
>  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
>  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
>  4 files changed, 44 insertions(+), 50 deletions(-)
> 
> ---
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> -- 
> 1.8.5.2
> 

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Jason Cooper]

On Wed, Feb 05, 2014 at 06:08:02AM -0800, Mike Turquette wrote:
> Quoting Sebastian Hesselbarth (2014-01-25 10:19:06)
> > This patch set fixes clk init order that went upside-down with
> > v3.14. I haven't really investigated what caused this, but I assume
> > it is related with DT node reordering by addresses.
> > 
> > Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> > registered before core clocks driver. Unfortunately, we cannot
> > return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> > init order for our drivers is always core clocks before clock gating,
> > we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> > init function that will register core clocks before clock gating
> > driver.
> > 
> > This patch is based on pre-v3.14-rc1 mainline and should go in as
> > fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> > I suggest Jason picks it up as a topic branch.
> 
> Sebastian,
> 
> These patches look OK to me. I'd rather take Gregory Clement's "respect
> the clock dependencies in of_clk_init" patch towards 3.15, so this fix
> will still be necessary for the current -rc's.
> 
> Jason, will you be sending a PR?

Ahh, just saw this now.  ignore my previous comments about using
Gregory's series as the fix.

Sure, I'll put this together for a pr to the clk tree.

thx,

Jason.

> > The patches have been boot tested on Dove and compile-tested only
> > for Kirkwood, Armada 370 and XP.
> > 
> > Sebastian Hesselbarth (4):
> >   clk: mvebu: armada-370: maintain clock init order
> >   clk: mvebu: armada-xp: maintain clock init order
> >   clk: mvebu: dove: maintain clock init order
> >   clk: mvebu: kirkwood: maintain clock init order
> > 
> >  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
> >  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
> >  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
> >  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
> >  4 files changed, 44 insertions(+), 50 deletions(-)
> > 
> > ---
> > Cc: Mike Turquette <mturquette@linaro.org>
> > Cc: Jason Cooper <jason@lakedaemon.net>
> > Cc: Andrew Lunn <andrew@lunn.ch>
> > Cc: Gregory Clement <gregory.clement@free-electrons.com>
> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > -- 
> > 1.8.5.2
> > 

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Jason Cooper]

On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:
> This patch set fixes clk init order that went upside-down with
> v3.14. I haven't really investigated what caused this, but I assume
> it is related with DT node reordering by addresses.
> 
> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> registered before core clocks driver. Unfortunately, we cannot
> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> init order for our drivers is always core clocks before clock gating,
> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> init function that will register core clocks before clock gating
> driver.
> 
> This patch is based on pre-v3.14-rc1 mainline and should go in as
> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> I suggest Jason picks it up as a topic branch.
> 
> The patches have been boot tested on Dove and compile-tested only
> for Kirkwood, Armada 370 and XP.
> 
> Sebastian Hesselbarth (4):
>   clk: mvebu: armada-370: maintain clock init order
>   clk: mvebu: armada-xp: maintain clock init order
>   clk: mvebu: dove: maintain clock init order
>   clk: mvebu: kirkwood: maintain clock init order
> 
>  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
>  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
>  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
>  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
>  4 files changed, 44 insertions(+), 50 deletions(-)

Whole series applied to mvebu/clk-fixes.

thx,

Jason.

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Ezequiel Garcia]

On Wed, Feb 05, 2014 at 01:34:57PM -0500, Jason Cooper wrote:
> On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:
> > This patch set fixes clk init order that went upside-down with
> > v3.14. I haven't really investigated what caused this, but I assume
> > it is related with DT node reordering by addresses.
> > 
> > Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> > registered before core clocks driver. Unfortunately, we cannot
> > return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> > init order for our drivers is always core clocks before clock gating,
> > we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> > init function that will register core clocks before clock gating
> > driver.
> > 
> > This patch is based on pre-v3.14-rc1 mainline and should go in as
> > fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> > I suggest Jason picks it up as a topic branch.
> > 
> > The patches have been boot tested on Dove and compile-tested only
> > for Kirkwood, Armada 370 and XP.
> > 
> > Sebastian Hesselbarth (4):
> >   clk: mvebu: armada-370: maintain clock init order
> >   clk: mvebu: armada-xp: maintain clock init order
> >   clk: mvebu: dove: maintain clock init order
> >   clk: mvebu: kirkwood: maintain clock init order
> > 
> >  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
> >  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
> >  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
> >  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
> >  4 files changed, 44 insertions(+), 50 deletions(-)
> 
> Whole series applied to mvebu/clk-fixes.
> 

FWIW, Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Observed "division by zero" get fixed by this, on A370 Mirabox, Kirkwood
Topkick and Dove Cubox.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Jason Cooper]

On Thu, Feb 06, 2014 at 02:08:39PM -0300, Ezequiel Garcia wrote:
> On Wed, Feb 05, 2014 at 01:34:57PM -0500, Jason Cooper wrote:
> > On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:
> > > This patch set fixes clk init order that went upside-down with
> > > v3.14. I haven't really investigated what caused this, but I assume
> > > it is related with DT node reordering by addresses.
> > > 
> > > Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> > > registered before core clocks driver. Unfortunately, we cannot
> > > return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> > > init order for our drivers is always core clocks before clock gating,
> > > we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> > > init function that will register core clocks before clock gating
> > > driver.
> > > 
> > > This patch is based on pre-v3.14-rc1 mainline and should go in as
> > > fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> > > I suggest Jason picks it up as a topic branch.
> > > 
> > > The patches have been boot tested on Dove and compile-tested only
> > > for Kirkwood, Armada 370 and XP.
> > > 
> > > Sebastian Hesselbarth (4):
> > >   clk: mvebu: armada-370: maintain clock init order
> > >   clk: mvebu: armada-xp: maintain clock init order
> > >   clk: mvebu: dove: maintain clock init order
> > >   clk: mvebu: kirkwood: maintain clock init order
> > > 
> > >  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
> > >  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
> > >  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
> > >  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
> > >  4 files changed, 44 insertions(+), 50 deletions(-)
> > 
> > Whole series applied to mvebu/clk-fixes.
> > 
> 
> FWIW, Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> Observed "division by zero" get fixed by this, on A370 Mirabox, Kirkwood
> Topkick and Dove Cubox.

Added for patches 1, 3, and 4 (all but armada-xp.c)  Thanks for testing.

thx,

Jason.

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Ezequiel Garcia]

On Wed, Feb 05, 2014 at 01:34:57PM -0500, Jason Cooper wrote:
> On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:
> > This patch set fixes clk init order that went upside-down with
> > v3.14. I haven't really investigated what caused this, but I assume
> > it is related with DT node reordering by addresses.
> > 
> > Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> > registered before core clocks driver. Unfortunately, we cannot
> > return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> > init order for our drivers is always core clocks before clock gating,
> > we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> > init function that will register core clocks before clock gating
> > driver.
> > 
> > This patch is based on pre-v3.14-rc1 mainline and should go in as
> > fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> > I suggest Jason picks it up as a topic branch.
> > 
> > The patches have been boot tested on Dove and compile-tested only
> > for Kirkwood, Armada 370 and XP.
> > 
> > Sebastian Hesselbarth (4):
> >   clk: mvebu: armada-370: maintain clock init order
> >   clk: mvebu: armada-xp: maintain clock init order
> >   clk: mvebu: dove: maintain clock init order
> >   clk: mvebu: kirkwood: maintain clock init order
> > 
> >  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
> >  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
> >  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
> >  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
> >  4 files changed, 44 insertions(+), 50 deletions(-)
> 
> Whole series applied to mvebu/clk-fixes.
> 

Are we still in time to consider Emilio's oneline proposal?
(Emilio: would you mind preparing a suitable patch against dove,
kirkwood, armada370/xp, so we can see the real thing?).

Sebastian fix works perfect, and it easy to understand. However, it has
quite a large diffstat. When compared to Emilio's oneline proposal, it
seems to me it would be preferable, unless it's broken.

Workaround or not, the fact is this code will be in v3.14, so maybe we
can spend some time considering a cleaner option.

Or is this just rubbish?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Gregory CLEMENT]

On 17/02/2014 15:13, Ezequiel Garcia wrote:
> On Wed, Feb 05, 2014 at 01:34:57PM -0500, Jason Cooper wrote:
>> On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:
>>> This patch set fixes clk init order that went upside-down with
>>> v3.14. I haven't really investigated what caused this, but I assume
>>> it is related with DT node reordering by addresses.
>>>
>>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
>>> registered before core clocks driver. Unfortunately, we cannot
>>> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
>>> init order for our drivers is always core clocks before clock gating,
>>> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
>>> init function that will register core clocks before clock gating
>>> driver.
>>>
>>> This patch is based on pre-v3.14-rc1 mainline and should go in as
>>> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
>>> I suggest Jason picks it up as a topic branch.
>>>
>>> The patches have been boot tested on Dove and compile-tested only
>>> for Kirkwood, Armada 370 and XP.
>>>
>>> Sebastian Hesselbarth (4):
>>>   clk: mvebu: armada-370: maintain clock init order
>>>   clk: mvebu: armada-xp: maintain clock init order
>>>   clk: mvebu: dove: maintain clock init order
>>>   clk: mvebu: kirkwood: maintain clock init order
>>>
>>>  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
>>>  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
>>>  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
>>>  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
>>>  4 files changed, 44 insertions(+), 50 deletions(-)
>>
>> Whole series applied to mvebu/clk-fixes.
>>
> 
> Are we still in time to consider Emilio's oneline proposal?
> (Emilio: would you mind preparing a suitable patch against dove,
> kirkwood, armada370/xp, so we can see the real thing?).

I am still strongly against this proposal because hard-coded the parent
clock name in the driver seems very wrong and moreover in some circumstances
(if there is no output-name, which is our default case) this proposal
just ignored the parent clock given by the device tree and this looked
more wrong.

> 
> Sebastian fix works perfect, and it easy to understand. However, it has
> quite a large diffstat. When compared to Emilio's oneline proposal, it
> seems to me it would be preferable, unless it's broken.
> 
> Workaround or not, the fact is this code will be in v3.14, so maybe we
> can spend some time considering a cleaner option.
> 
> Or is this just rubbish?
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Emilio López]

Hi Gregory, Ezequiel,

 >> Are we still in time to consider Emilio's oneline proposal?
>> (Emilio: would you mind preparing a suitable patch against dove,
>> kirkwood, armada370/xp, so we can see the real thing?).

The patch is in a common file, so it does not need patching anything for 
each platform.

> I am still strongly against this proposal because hard-coded the parent
> clock name in the driver seems very wrong

It is hardcoded already when the parent is registered, so I do not 
understand your concern.

http://lxr.free-electrons.com/source/drivers/clk/mvebu/common.c?v=3.13#L34

> and moreover in some circumstances
> (if there is no output-name, which is our default case) this proposal
> just ignored the parent clock given by the device tree and this looked
> more wrong.

I have sent a second patch addressing this comment, but you do not seem 
to have taken too serious a look at it.

http://www.spinics.net/lists/arm-kernel/msg305922.html

Cheers,

Emilio

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Gregory CLEMENT]

On 17/02/2014 15:42, Emilio López wrote:
> Hi Gregory, Ezequiel,
> 
>  >> Are we still in time to consider Emilio's oneline proposal?
>>> (Emilio: would you mind preparing a suitable patch against dove,
>>> kirkwood, armada370/xp, so we can see the real thing?).
> 
> The patch is in a common file, so it does not need patching anything for 
> each platform.
> 
>> I am still strongly against this proposal because hard-coded the parent
>> clock name in the driver seems very wrong
> 
> It is hardcoded already when the parent is registered, so I do not 
> understand your concern.
> 
> http://lxr.free-electrons.com/source/drivers/clk/mvebu/common.c?v=3.13#L34
> 
>> and moreover in some circumstances
>> (if there is no output-name, which is our default case) this proposal
>> just ignored the parent clock given by the device tree and this looked
>> more wrong.
> 
> I have sent a second patch addressing this comment, but you do not seem 
> to have taken too serious a look at it.
> 
> http://www.spinics.net/lists/arm-kernel/msg305922.html
> 

Please read what I have written: "if there is no output-name, which is our
default case) this proposal just ignored the parent clock given by the device
tree".

Extract of your code from the link you pointed:

const char *default_parent = "tclk";

[...]

of_property_read_string_index(clkspec.np, "clock-output-names",
					      clkspec.args_count ? clkspec.args[0] : 0,
					      &default_parent);

example of a valid dts:
			gateclk: clock-gating-control@18220 {
				compatible = "marvell,foo-bar-gating-clock";
				reg = <0x18220 0x4>;
				clocks = <&coreclk 1>;
				#clock-cells = <1>;
			};

So in this fictional but still valid example, the device tree indicates that the parent
clock of the gating clock is the 2nd clock provided by the coreclk which is currently
"cpuclk". As no clock-output-names is used, then this will be totally ignore and instead
of using "cpuclk" as parent "tclk" will be used.

I hope this example will show you, what I disagree with this proposal and why it
introduce some regression.

Regards,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Ezequiel Garcia]

On Mon, Feb 17, 2014 at 03:25:22PM +0100, Gregory CLEMENT wrote:
> On 17/02/2014 15:13, Ezequiel Garcia wrote:
> > On Wed, Feb 05, 2014 at 01:34:57PM -0500, Jason Cooper wrote:
> >> On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:
> >>> This patch set fixes clk init order that went upside-down with
> >>> v3.14. I haven't really investigated what caused this, but I assume
> >>> it is related with DT node reordering by addresses.
> >>>
> >>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> >>> registered before core clocks driver. Unfortunately, we cannot
> >>> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> >>> init order for our drivers is always core clocks before clock gating,
> >>> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> >>> init function that will register core clocks before clock gating
> >>> driver.
> >>>
> >>> This patch is based on pre-v3.14-rc1 mainline and should go in as
> >>> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> >>> I suggest Jason picks it up as a topic branch.
> >>>
> >>> The patches have been boot tested on Dove and compile-tested only
> >>> for Kirkwood, Armada 370 and XP.
> >>>
> >>> Sebastian Hesselbarth (4):
> >>>   clk: mvebu: armada-370: maintain clock init order
> >>>   clk: mvebu: armada-xp: maintain clock init order
> >>>   clk: mvebu: dove: maintain clock init order
> >>>   clk: mvebu: kirkwood: maintain clock init order
> >>>
> >>>  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
> >>>  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
> >>>  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
> >>>  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
> >>>  4 files changed, 44 insertions(+), 50 deletions(-)
> >>
> >> Whole series applied to mvebu/clk-fixes.
> >>
> > 
> > Are we still in time to consider Emilio's oneline proposal?
> > (Emilio: would you mind preparing a suitable patch against dove,
> > kirkwood, armada370/xp, so we can see the real thing?).
> 
> I am still strongly against this proposal because hard-coded the parent
> clock name in the driver seems very wrong and moreover in some circumstances
> (if there is no output-name, which is our default case) this proposal
> just ignored the parent clock given by the device tree and this looked
> more wrong.
> 

So you're against the proposal as a permanent fix, *and* against the
proposal as a workaround fix?

> > 
> > Sebastian fix works perfect, and it easy to understand. However, it has
> > quite a large diffstat. When compared to Emilio's oneline proposal, it
> > seems to me it would be preferable, unless it's broken.
> > 
> > Workaround or not, the fact is this code will be in v3.14, so maybe we
> > can spend some time considering a cleaner option.
> > 

Before discussing the solution as compared to your for-v3.15 clock
registration order patch, I wanted to trigger some discussion around
replacing this big and intrusive workaround with Emilio's oneline fix.

Let's suppose we're considering them as workaround to live just one or
two releases. Wouldn't it be better to take the least instrusive?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Gregory CLEMENT]

On 17/02/2014 16:21, Ezequiel Garcia wrote:
> On Mon, Feb 17, 2014 at 03:25:22PM +0100, Gregory CLEMENT wrote:
>> On 17/02/2014 15:13, Ezequiel Garcia wrote:
>>> On Wed, Feb 05, 2014 at 01:34:57PM -0500, Jason Cooper wrote:
>>>> On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:
>>>>> This patch set fixes clk init order that went upside-down with
>>>>> v3.14. I haven't really investigated what caused this, but I assume
>>>>> it is related with DT node reordering by addresses.
>>>>>
>>>>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
>>>>> registered before core clocks driver. Unfortunately, we cannot
>>>>> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
>>>>> init order for our drivers is always core clocks before clock gating,
>>>>> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
>>>>> init function that will register core clocks before clock gating
>>>>> driver.
>>>>>
>>>>> This patch is based on pre-v3.14-rc1 mainline and should go in as
>>>>> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
>>>>> I suggest Jason picks it up as a topic branch.
>>>>>
>>>>> The patches have been boot tested on Dove and compile-tested only
>>>>> for Kirkwood, Armada 370 and XP.
>>>>>
>>>>> Sebastian Hesselbarth (4):
>>>>>   clk: mvebu: armada-370: maintain clock init order
>>>>>   clk: mvebu: armada-xp: maintain clock init order
>>>>>   clk: mvebu: dove: maintain clock init order
>>>>>   clk: mvebu: kirkwood: maintain clock init order
>>>>>
>>>>>  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
>>>>>  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
>>>>>  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
>>>>>  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
>>>>>  4 files changed, 44 insertions(+), 50 deletions(-)
>>>>
>>>> Whole series applied to mvebu/clk-fixes.
>>>>
>>>
>>> Are we still in time to consider Emilio's oneline proposal?
>>> (Emilio: would you mind preparing a suitable patch against dove,
>>> kirkwood, armada370/xp, so we can see the real thing?).
>>
>> I am still strongly against this proposal because hard-coded the parent
>> clock name in the driver seems very wrong and moreover in some circumstances
>> (if there is no output-name, which is our default case) this proposal
>> just ignored the parent clock given by the device tree and this looked
>> more wrong.
>>
> 
> So you're against the proposal as a permanent fix, *and* against the
> proposal as a workaround fix?

Yes

> 
>>>
>>> Sebastian fix works perfect, and it easy to understand. However, it has
>>> quite a large diffstat. When compared to Emilio's oneline proposal, it
>>> seems to me it would be preferable, unless it's broken.
>>>
>>> Workaround or not, the fact is this code will be in v3.14, so maybe we
>>> can spend some time considering a cleaner option.
>>>
> 
> Before discussing the solution as compared to your for-v3.15 clock
> registration order patch, I wanted to trigger some discussion around
> replacing this big and intrusive workaround with Emilio's oneline fix.
> 
> Let's suppose we're considering them as workaround to live just one or
> two releases. Wouldn't it be better to take the least instrusive?
> 

The better solution is the one which doesn't add another regression and until
today I though we had an agreement to use the patch set from Sebastian. If
I remember well Jason had sent a pull request for it.


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Emilio López]

Hi,

El 17/02/14 12:04, Gregory CLEMENT escribió:
(snip)
> Please read what I have written: "if there is no output-name, which is our
> default case) this proposal just ignored the parent clock given by the device
> tree".
>
> Extract of your code from the link you pointed:
>
> const char *default_parent = "tclk";
>
> [...]
>
> of_property_read_string_index(clkspec.np, "clock-output-names",
> 					      clkspec.args_count ? clkspec.args[0] : 0,
> 					      &default_parent);
>
> example of a valid dts:
> 			gateclk: clock-gating-control@18220 {
> 				compatible = "marvell,foo-bar-gating-clock";
> 				reg = <0x18220 0x4>;
> 				clocks = <&coreclk 1>;
> 				#clock-cells = <1>;
> 			};
>
> So in this fictional but still valid example, the device tree indicates that the parent
> clock of the gating clock is the 2nd clock provided by the coreclk which is currently
> "cpuclk". As no clock-output-names is used, then this will be totally ignore and instead
> of using "cpuclk" as parent "tclk" will be used.

I can see your point now, but as this is completely fictional, I'd say 
it's irrelevant. You can just add the names if Marvell ever makes a chip 
that sources the gates from the second coreclk. As far as I can see on 
the device trees in Linux, all mvebu hardware always sources them from 
tclk. Don't try to over-engineer your driver for something that is 
unlikely to happen in reality.

If you in the future need to support another legacy platform with a 
half-cooked DT not listing the names, you can always list the right 
parent on the divisor table (see link for example) and override the default.

http://lxr.free-electrons.com/source/drivers/clk/mvebu/kirkwood.c?v=3.13#L222

> I hope this example will show you, what I disagree with this proposal and why it
> introduce some regression.

It's not a regression if things don't break :-)

Cheers,

Emilio

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Ezequiel Garcia]

On Mon, Feb 17, 2014 at 04:28:41PM +0100, Gregory CLEMENT wrote:
> On 17/02/2014 16:21, Ezequiel Garcia wrote:
> > On Mon, Feb 17, 2014 at 03:25:22PM +0100, Gregory CLEMENT wrote:
> >> On 17/02/2014 15:13, Ezequiel Garcia wrote:
> >>> On Wed, Feb 05, 2014 at 01:34:57PM -0500, Jason Cooper wrote:
> >>>> On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:
> >>>>> This patch set fixes clk init order that went upside-down with
> >>>>> v3.14. I haven't really investigated what caused this, but I assume
> >>>>> it is related with DT node reordering by addresses.
> >>>>>
> >>>>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> >>>>> registered before core clocks driver. Unfortunately, we cannot
> >>>>> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
> >>>>> init order for our drivers is always core clocks before clock gating,
> >>>>> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
> >>>>> init function that will register core clocks before clock gating
> >>>>> driver.
> >>>>>
> >>>>> This patch is based on pre-v3.14-rc1 mainline and should go in as
> >>>>> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
> >>>>> I suggest Jason picks it up as a topic branch.
> >>>>>
> >>>>> The patches have been boot tested on Dove and compile-tested only
> >>>>> for Kirkwood, Armada 370 and XP.
> >>>>>
> >>>>> Sebastian Hesselbarth (4):
> >>>>>   clk: mvebu: armada-370: maintain clock init order
> >>>>>   clk: mvebu: armada-xp: maintain clock init order
> >>>>>   clk: mvebu: dove: maintain clock init order
> >>>>>   clk: mvebu: kirkwood: maintain clock init order
> >>>>>
> >>>>>  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
> >>>>>  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
> >>>>>  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
> >>>>>  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
> >>>>>  4 files changed, 44 insertions(+), 50 deletions(-)
> >>>>
> >>>> Whole series applied to mvebu/clk-fixes.
> >>>>
> >>>
> >>> Are we still in time to consider Emilio's oneline proposal?
> >>> (Emilio: would you mind preparing a suitable patch against dove,
> >>> kirkwood, armada370/xp, so we can see the real thing?).
> >>
> >> I am still strongly against this proposal because hard-coded the parent
> >> clock name in the driver seems very wrong and moreover in some circumstances
> >> (if there is no output-name, which is our default case) this proposal
> >> just ignored the parent clock given by the device tree and this looked
> >> more wrong.
> >>
> > 
> > So you're against the proposal as a permanent fix, *and* against the
> > proposal as a workaround fix?
> 
> Yes
> 
> > 
> >>>
> >>> Sebastian fix works perfect, and it easy to understand. However, it has
> >>> quite a large diffstat. When compared to Emilio's oneline proposal, it
> >>> seems to me it would be preferable, unless it's broken.
> >>>
> >>> Workaround or not, the fact is this code will be in v3.14, so maybe we
> >>> can spend some time considering a cleaner option.
> >>>
> > 
> > Before discussing the solution as compared to your for-v3.15 clock
> > registration order patch, I wanted to trigger some discussion around
> > replacing this big and intrusive workaround with Emilio's oneline fix.
> > 
> > Let's suppose we're considering them as workaround to live just one or
> > two releases. Wouldn't it be better to take the least instrusive?
> > 
> 
> The better solution is the one which doesn't add another regression and until
> today I though we had an agreement to use the patch set from Sebastian. If
> I remember well Jason had sent a pull request for it.
> 
> 

Right. If you think it adds a regression, then that's a perfectly valid reasons
for nacking.

However, I'd like to double-check we have such a regression. I guess you're
talking about the "tclk" name being hardcoded. IMHO, it's hardcoded in the
driver in the first place:

void __init mvebu_coreclk_setup(struct device_node *np,
				const struct coreclk_soc_desc *desc)
{
	const char *tclk_name = "tclk";
[..]

So it wouldn't be too nasty?

I think you also mentioned the hypothetical case where the name
is overloaded in the devicetree, so it's not "tclk", no? In that case,
Emilio's fix would break.

However, we don't have such situation in our 'canonical' devicetrees,
so if the devicetree is allowed to be change, it can also be
changed to add clock-output-name's so the name doesn't have to be
obtained after the clock is registered.

Which would solve it, no?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Gregory CLEMENT]

On 17/02/2014 16:44, Ezequiel Garcia wrote:
> On Mon, Feb 17, 2014 at 04:28:41PM +0100, Gregory CLEMENT wrote:
>> On 17/02/2014 16:21, Ezequiel Garcia wrote:
>>> On Mon, Feb 17, 2014 at 03:25:22PM +0100, Gregory CLEMENT wrote:
>>>> On 17/02/2014 15:13, Ezequiel Garcia wrote:
>>>>> On Wed, Feb 05, 2014 at 01:34:57PM -0500, Jason Cooper wrote:
>>>>>> On Sat, Jan 25, 2014 at 07:19:06PM +0100, Sebastian Hesselbarth wrote:
>>>>>>> This patch set fixes clk init order that went upside-down with
>>>>>>> v3.14. I haven't really investigated what caused this, but I assume
>>>>>>> it is related with DT node reordering by addresses.
>>>>>>>
>>>>>>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
>>>>>>> registered before core clocks driver. Unfortunately, we cannot
>>>>>>> return -EPROBE_DEFER in drivers initialized by clk_of_init. As the
>>>>>>> init order for our drivers is always core clocks before clock gating,
>>>>>>> we maintain init order ourselves by hooking CLK_OF_DECLARE to one
>>>>>>> init function that will register core clocks before clock gating
>>>>>>> driver.
>>>>>>>
>>>>>>> This patch is based on pre-v3.14-rc1 mainline and should go in as
>>>>>>> fixes for it. As we now send MVEBU clk pull-requests to Mike directly,
>>>>>>> I suggest Jason picks it up as a topic branch.
>>>>>>>
>>>>>>> The patches have been boot tested on Dove and compile-tested only
>>>>>>> for Kirkwood, Armada 370 and XP.
>>>>>>>
>>>>>>> Sebastian Hesselbarth (4):
>>>>>>>   clk: mvebu: armada-370: maintain clock init order
>>>>>>>   clk: mvebu: armada-xp: maintain clock init order
>>>>>>>   clk: mvebu: dove: maintain clock init order
>>>>>>>   clk: mvebu: kirkwood: maintain clock init order
>>>>>>>
>>>>>>>  drivers/clk/mvebu/armada-370.c | 21 ++++++++++-----------
>>>>>>>  drivers/clk/mvebu/armada-xp.c  | 20 +++++++++-----------
>>>>>>>  drivers/clk/mvebu/dove.c       | 19 +++++++++----------
>>>>>>>  drivers/clk/mvebu/kirkwood.c   | 34 ++++++++++++++++------------------
>>>>>>>  4 files changed, 44 insertions(+), 50 deletions(-)
>>>>>>
>>>>>> Whole series applied to mvebu/clk-fixes.
>>>>>>
>>>>>
>>>>> Are we still in time to consider Emilio's oneline proposal?
>>>>> (Emilio: would you mind preparing a suitable patch against dove,
>>>>> kirkwood, armada370/xp, so we can see the real thing?).
>>>>
>>>> I am still strongly against this proposal because hard-coded the parent
>>>> clock name in the driver seems very wrong and moreover in some circumstances
>>>> (if there is no output-name, which is our default case) this proposal
>>>> just ignored the parent clock given by the device tree and this looked
>>>> more wrong.
>>>>
>>>
>>> So you're against the proposal as a permanent fix, *and* against the
>>> proposal as a workaround fix?
>>
>> Yes
>>
>>>
>>>>>
>>>>> Sebastian fix works perfect, and it easy to understand. However, it has
>>>>> quite a large diffstat. When compared to Emilio's oneline proposal, it
>>>>> seems to me it would be preferable, unless it's broken.
>>>>>
>>>>> Workaround or not, the fact is this code will be in v3.14, so maybe we
>>>>> can spend some time considering a cleaner option.
>>>>>
>>>
>>> Before discussing the solution as compared to your for-v3.15 clock
>>> registration order patch, I wanted to trigger some discussion around
>>> replacing this big and intrusive workaround with Emilio's oneline fix.
>>>
>>> Let's suppose we're considering them as workaround to live just one or
>>> two releases. Wouldn't it be better to take the least instrusive?
>>>
>>
>> The better solution is the one which doesn't add another regression and until
>> today I though we had an agreement to use the patch set from Sebastian. If
>> I remember well Jason had sent a pull request for it.
>>
>>
> 
> Right. If you think it adds a regression, then that's a perfectly valid reasons
> for nacking.
> 
> However, I'd like to double-check we have such a regression. I guess you're
> talking about the "tclk" name being hardcoded. IMHO, it's hardcoded in the
> driver in the first place:
> 
> void __init mvebu_coreclk_setup(struct device_node *np,
> 				const struct coreclk_soc_desc *desc)
> {
> 	const char *tclk_name = "tclk";
> [..] 


Here it is just about giving a name to a clock. As in the device tree
we only refer to the clock by index, the name don't matter.

> 
> So it wouldn't be too nasty?
> 
> I think you also mentioned the hypothetical case where the name
> is overloaded in the devicetree, so it's not "tclk", no? In that case,
> Emilio's fix would break.

I don't think I mentioned this case. I mentioned that this "fix" will
ignore the device tree.

> 
> However, we don't have such situation in our 'canonical' devicetrees,
> so if the devicetree is allowed to be change, it can also be
> changed to add clock-output-name's so the name doesn't have to be
> obtained after the clock is registered.
> 
> Which would solve it, no?

I really don't understand why you want introduce potential problem, just
to save a few line of code. A code that " works perfect, and it easy
to understand" as you wrote.




-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Ezequiel Garcia]

On Mon, Feb 17, 2014 at 04:59:01PM +0100, Gregory CLEMENT wrote:
[..]
> > 
> > Right. If you think it adds a regression, then that's a perfectly valid reasons
> > for nacking.
> > 
> > However, I'd like to double-check we have such a regression. I guess you're
> > talking about the "tclk" name being hardcoded. IMHO, it's hardcoded in the
> > driver in the first place:
> > 
> > void __init mvebu_coreclk_setup(struct device_node *np,
> > 				const struct coreclk_soc_desc *desc)
> > {
> > 	const char *tclk_name = "tclk";
> > [..] 
> 
> 
> Here it is just about giving a name to a clock. As in the device tree
> we only refer to the clock by index, the name don't matter.
> 

Unless I'm really confused about what's the problem here, the *name* is
*all* that matters.

We're having a clock *registration* order issue (which is different from clock
enable). Let me try to explain the issue, in case it's still not clear.

I'll stick to the current specific problem but it can apply to any other
pair of parent/child.

Some of the mvebu clocks are registered as part of the "core" clock
group, modeled by the "marvell,armada-370-core-clock" compatible node.

Another group of mvebu clocks are registered as part of the "gating"
clock group, modeled by the "marvell,armada-370-gating-clock" compatible
node.

By default, all the gating clocks are child of the first registered core
clock. This clock is named "tclk" by default, and this name can be overloaded
in the devicetree.

So far, so good, right?

The issue we're trying to fix, arises from the mvebu_clk_gating_setup()
trying to get the name of this "tclk", as a registered clock.

In other words, the current code needs the tclk to be registered, for it
will ask his name like this:

	const char *default_parent = NULL;

	clk = of_clk_get(np, 0);
	if (!IS_ERR(clk)) {
		default_parent = __clk_get_name(clk);
		clk_put(clk);
	}

Once it gets the name, all goes smoothly. Notice how the clock is obtained
for the sole purpose of getting the name of it, which shows clearly it's the
*name* that matters.

The ordering issue happens when the gating clock group is probed, before
the core clock group. In that case, it's not possible to get the
"&coreclk 0" (which is wrongly assumed to be registered), and so it's
not possible to get the name.

So the root of the problem is that snippet above, which adds a
completely unneeded registration order requirement. Instead, we should
be looking for the names: we can hardcode the name or fetch it from the
devicetree (Emilio has posted patches for both).

I really don't see why we're not fixing this, instead of adding yet
another layer of complexity to the problem.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Gregory CLEMENT]

[-- Attachment #1: Type: text/plain, Size: 3638 bytes --]

On 17/02/2014 19:19, Ezequiel Garcia wrote:
> On Mon, Feb 17, 2014 at 04:59:01PM +0100, Gregory CLEMENT wrote:
> [..]
>>>
>>> Right. If you think it adds a regression, then that's a perfectly valid reasons
>>> for nacking.
>>>
>>> However, I'd like to double-check we have such a regression. I guess you're
>>> talking about the "tclk" name being hardcoded. IMHO, it's hardcoded in the
>>> driver in the first place:
>>>
>>> void __init mvebu_coreclk_setup(struct device_node *np,
>>> 				const struct coreclk_soc_desc *desc)
>>> {
>>> 	const char *tclk_name = "tclk";
>>> [..] 
>>
>>
>> Here it is just about giving a name to a clock. As in the device tree
>> we only refer to the clock by index, the name don't matter.
>>
> 
> Unless I'm really confused about what's the problem here, the *name* is
> *all* that matters.
> 
> We're having a clock *registration* order issue (which is different from clock
> enable). Let me try to explain the issue, in case it's still not clear.
> 
> I'll stick to the current specific problem but it can apply to any other
> pair of parent/child.
> 
> Some of the mvebu clocks are registered as part of the "core" clock
> group, modeled by the "marvell,armada-370-core-clock" compatible node.
> 
> Another group of mvebu clocks are registered as part of the "gating"
> clock group, modeled by the "marvell,armada-370-gating-clock" compatible
> node.
> 
> By default, all the gating clocks are child of the first registered core
> clock. This clock is named "tclk" by default, and this name can be overloaded
> in the devicetree.
> 
> So far, so good, right?
> 
> The issue we're trying to fix, arises from the mvebu_clk_gating_setup()
> trying to get the name of this "tclk", as a registered clock.
> 
> In other words, the current code needs the tclk to be registered, for it
> will ask his name like this:
> 
> 	const char *default_parent = NULL;
> 
> 	clk = of_clk_get(np, 0);
> 	if (!IS_ERR(clk)) {
> 		default_parent = __clk_get_name(clk);
> 		clk_put(clk);
> 	}
> 
> Once it gets the name, all goes smoothly. Notice how the clock is obtained
> for the sole purpose of getting the name of it, which shows clearly it's the
> *name* that matters.
> 
> The ordering issue happens when the gating clock group is probed, before
> the core clock group. In that case, it's not possible to get the
> "&coreclk 0" (which is wrongly assumed to be registered), and so it's
> not possible to get the name.
> 
> So the root of the problem is that snippet above, which adds a
> completely unneeded registration order requirement. Instead, we should
> be looking for the names: we can hardcode the name or fetch it from the
> devicetree (Emilio has posted patches for both).
> 
> I really don't see why we're not fixing this, instead of adding yet
> another layer of complexity to the problem.

All this have already discussed in the previous emails. And even if Emilio
denied introducing a regression, it was what the code did. See my example
here:
http://article.gmane.org/gmane.linux.kernel/1649439

Now as you both are really annoying with it, what would be an acceptable
version would be the something like the patch attached. There will be still
an issue if old dtb is used with recent kernel, but at least the user will
be warned.

This code only fix the Armada 370 case, a complete solution should modify
the dtsi for Armada XP, Armada 375, Armada 38x, Kirkwood and Dove. It should
also make the outputname mandatory for gate-clk for consistency.


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[-- Attachment #2: 0001-RFC-clk-mvebu-make-clock-output-names-mandatory.patch --]
[-- Type: text/x-diff, Size: 4907 bytes --]

>From ca82f66b8fbb8247b0ec2c407726105e1e1af419 Mon Sep 17 00:00:00 2001
From: Gregory CLEMENT <gregory.clement@free-electrons.com>
Date: Tue, 18 Feb 2014 10:38:08 +0100
Subject: [PATCH] [RFC] clk:mvebu: make clock-output-names mandatory

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 .../devicetree/bindings/clock/mvebu-core-clock.txt |  7 ++--
 arch/arm/boot/dts/armada-370.dtsi                  |  1 +
 drivers/clk/mvebu/common.c                         | 38 +++++++++++++++-------
 3 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/mvebu-core-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-core-clock.txt
index 307a503c5db8..4f2e3953b6a6 100644
--- a/Documentation/devicetree/bindings/clock/mvebu-core-clock.txt
+++ b/Documentation/devicetree/bindings/clock/mvebu-core-clock.txt
@@ -40,16 +40,15 @@ Required properties:
 	"marvell,mv88f6180-core-clock" - for Kirkwood MV88f6180 SoC
 - reg : shall be the register address of the Sample-At-Reset (SAR) register
 - #clock-cells : from common clock binding; shall be set to 1
-
-Optional properties:
-- clock-output-names : from common clock binding; allows overwrite default clock
-	output names ("tclk", "cpuclk", "l2clk", "ddrclk")
+- clock-output-names : from common clock binding; should be the clock
+	output names given above
 
 Example:
 
 core_clk: core-clocks@d0214 {
 	compatible = "marvell,dove-core-clock";
 	reg = <0xd0214 0x4>;
+	clock-output-names = "tclk", "cpuclk", "l2clk", "ddrclk";
 	#clock-cells = <1>;
 };
 
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index af1f11e9e5a0..0d5853d05bd6 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -196,6 +196,7 @@
 			coreclk: mvebu-sar@18230 {
 				compatible = "marvell,armada-370-core-clock";
 				reg = <0x18230 0x08>;
+				clock-output-names = "tclk", "cpuclk", "nbclk", "hclk", "dramclk";
 				#clock-cells = <1>;
 			};
 
diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
index 25ceccf939ad..5e7c9274e4d3 100644
--- a/drivers/clk/mvebu/common.c
+++ b/drivers/clk/mvebu/common.c
@@ -51,16 +51,21 @@ void __init mvebu_coreclk_setup(struct device_node *np,
 	}
 
 	/* Register TCLK */
-	of_property_read_string_index(np, "clock-output-names", 0,
-				      &tclk_name);
+	if (of_property_read_string_index(np, "clock-output-names", 0,
+						&tclk_name))
+		pr_err("%s[0]: clock-output-names is mandatory\n"
+			"\"%s\" will be used by default\n", np->name, tclk_name);
 	rate = desc->get_tclk_freq(base);
 	clk_data.clks[0] = clk_register_fixed_rate(NULL, tclk_name, NULL,
 						   CLK_IS_ROOT, rate);
 	WARN_ON(IS_ERR(clk_data.clks[0]));
 
 	/* Register CPU clock */
-	of_property_read_string_index(np, "clock-output-names", 1,
-				      &cpuclk_name);
+	if (of_property_read_string_index(np, "clock-output-names", 1,
+						&cpuclk_name))
+		pr_err("%s[1]: clock-output-names is mandatory\n"
+			"\"%s\" will be used by default\n",
+			np->name, cpuclk_name);
 	rate = desc->get_cpu_freq(base);
 	clk_data.clks[1] = clk_register_fixed_rate(NULL, cpuclk_name, NULL,
 						   CLK_IS_ROOT, rate);
@@ -71,8 +76,11 @@ void __init mvebu_coreclk_setup(struct device_node *np,
 		const char *rclk_name = desc->ratios[n].name;
 		int mult, div;
 
-		of_property_read_string_index(np, "clock-output-names",
-					      2+n, &rclk_name);
+		if (of_property_read_string_index(np, "clock-output-names",
+							2+n, &rclk_name))
+			pr_err("%s[%d]:clock-output-names is mandatory\n"
+				"\"%s\" will be used by default\n",
+				np->name, 2+n, rclk_name);
 		desc->get_clk_ratio(base, desc->ratios[n].id, &mult, &div);
 		clk_data.clks[2+n] = clk_register_fixed_factor(NULL, rclk_name,
 				       cpuclk_name, 0, mult, div);
@@ -119,19 +127,27 @@ void __init mvebu_clk_gating_setup(struct device_node *np,
 				   const struct clk_gating_soc_desc *desc)
 {
 	struct clk_gating_ctrl *ctrl;
-	struct clk *clk;
 	void __iomem *base;
 	const char *default_parent = NULL;
 	int n;
+	struct of_phandle_args clkspec;
 
 	base = of_iomap(np, 0);
 	if (WARN_ON(!base))
 		return;
 
-	clk = of_clk_get(np, 0);
-	if (!IS_ERR(clk)) {
-		default_parent = __clk_get_name(clk);
-		clk_put(clk);
+	if (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0, &clkspec)) {
+		of_property_read_string_index(clkspec.np, "clock-output-names",
+					clkspec.args_count ? clkspec.args[0] : 0,
+					&default_parent);
+		if (WARN_ON(default_parent == NULL)) {
+			pr_err("%s: The clock-output-names of the parent clock is mandatory.\n"
+				"%s: As this proprety is missing, this parent will be ignored.\n"
+				"%s: The tclk clock will be used as parent clock\n",
+				np->name, np->name, np->name);
+			default_parent = "tclk";
+		}
+		of_node_put(clkspec.np);
 	}
 
 	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
-- 
1.8.1.2

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Ezequiel Garcia]

On Tue, Feb 18, 2014 at 10:47:00AM +0100, Gregory CLEMENT wrote:
> On 17/02/2014 19:19, Ezequiel Garcia wrote:
> > On Mon, Feb 17, 2014 at 04:59:01PM +0100, Gregory CLEMENT wrote:
> > [..]
> >>>
> >>> Right. If you think it adds a regression, then that's a perfectly valid reasons
> >>> for nacking.
> >>>
> >>> However, I'd like to double-check we have such a regression. I guess you're
> >>> talking about the "tclk" name being hardcoded. IMHO, it's hardcoded in the
> >>> driver in the first place:
> >>>
> >>> void __init mvebu_coreclk_setup(struct device_node *np,
> >>> 				const struct coreclk_soc_desc *desc)
> >>> {
> >>> 	const char *tclk_name = "tclk";
> >>> [..] 
> >>
> >>
> >> Here it is just about giving a name to a clock. As in the device tree
> >> we only refer to the clock by index, the name don't matter.
> >>
> > 
> > Unless I'm really confused about what's the problem here, the *name* is
> > *all* that matters.
> > 
> > We're having a clock *registration* order issue (which is different from clock
> > enable). Let me try to explain the issue, in case it's still not clear.
> > 
> > I'll stick to the current specific problem but it can apply to any other
> > pair of parent/child.
> > 
> > Some of the mvebu clocks are registered as part of the "core" clock
> > group, modeled by the "marvell,armada-370-core-clock" compatible node.
> > 
> > Another group of mvebu clocks are registered as part of the "gating"
> > clock group, modeled by the "marvell,armada-370-gating-clock" compatible
> > node.
> > 
> > By default, all the gating clocks are child of the first registered core
> > clock. This clock is named "tclk" by default, and this name can be overloaded
> > in the devicetree.
> > 
> > So far, so good, right?
> > 
> > The issue we're trying to fix, arises from the mvebu_clk_gating_setup()
> > trying to get the name of this "tclk", as a registered clock.
> > 
> > In other words, the current code needs the tclk to be registered, for it
> > will ask his name like this:
> > 
> > 	const char *default_parent = NULL;
> > 
> > 	clk = of_clk_get(np, 0);
> > 	if (!IS_ERR(clk)) {
> > 		default_parent = __clk_get_name(clk);
> > 		clk_put(clk);
> > 	}
> > 
> > Once it gets the name, all goes smoothly. Notice how the clock is obtained
> > for the sole purpose of getting the name of it, which shows clearly it's the
> > *name* that matters.
> > 
> > The ordering issue happens when the gating clock group is probed, before
> > the core clock group. In that case, it's not possible to get the
> > "&coreclk 0" (which is wrongly assumed to be registered), and so it's
> > not possible to get the name.
> > 
> > So the root of the problem is that snippet above, which adds a
> > completely unneeded registration order requirement. Instead, we should
> > be looking for the names: we can hardcode the name or fetch it from the
> > devicetree (Emilio has posted patches for both).
> > 
> > I really don't see why we're not fixing this, instead of adding yet
> > another layer of complexity to the problem.
> 
> All this have already discussed in the previous emails. And even if Emilio
> denied introducing a regression, it was what the code did. See my example
> here:
> http://article.gmane.org/gmane.linux.kernel/1649439
> 
> Now as you both are really annoying with it, what would be an acceptable
> version would be the something like the patch attached. There will be still
> an issue if old dtb is used with recent kernel, but at least the user will
> be warned.
> 

The regression you're talking about is will happen if the user decides to set
an awkward parent as the *default* parent of the gate clock group.

It's just the *default* that's specified in the devicetree, not the actual parent,
since we've designed this so a particular clock parent can always be specified from
the array in the driver.

> This code only fix the Armada 370 case, a complete solution should modify
> the dtsi for Armada XP, Armada 375, Armada 38x, Kirkwood and Dove. It should
> also make the outputname mandatory for gate-clk for consistency.
> 

I think you're missing the point in discussion. If you propose a patch
for the mvebu clock driver, then I guess you admit the problem is
solvable in the driver.

So, the real questions are:

Do we want to enforce a clock registration order?

Is this a framework defect? Or are our mvebu clocks doing things wrong?

As I tried to explained in detailed above, I think the mvebu clocks are doing
really evil things by needing a registered clock, just so it can retrieve the
default clock *name*. It's not even the clock name, given we allow to
override such default name.

A clock never needed a parent clock to be registered to be able to
register itself, it can just use the parent's clock name.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/4] clk: mvebu: fix clk init order

[Emilio López]

Hi Gregory,

El 18/02/14 06:47, Gregory CLEMENT escribió:
(snip)
> (...) what would be an acceptable
> version would be the something like the patch attached. There will be still
> an issue if old dtb is used with recent kernel, but at least the user will
> be warned.

The patch you attached is similar in spirit to what I suggested, but 
with way more warnings sprinkled around. I don't really mind either way, 
and if you prefer big warnings so be it; it's your driver after all :-)

> This code only fix the Armada 370 case, a complete solution should modify
> the dtsi for Armada XP, Armada 375, Armada 38x, Kirkwood and Dove. It should
> also make the outputname mandatory for gate-clk for consistency.

Again, all of them are your calls. Fix the issue as you see fit; as long 
as it's a technically sound I'm ok with it. But please don't reinvent 
the wheel on framework code under a principle that has no proven reason 
to be to cover up for a buggy driver.

Cheers, and have a great Wednesday :)

Emilio

PS: I'd really appreciate it if you could keep me cc'ed on respins of 
patches I comment on in the future.