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 <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
--
1.8.5.2
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 <[email protected]>
---
Cc: Mike Turquette <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
---
Cc: Mike Turquette <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
---
Cc: Mike Turquette <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
---
Cc: Mike Turquette <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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
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
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
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
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
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
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.
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 <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> Cc: Gregory Clement <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>
> Cc: Ezequiel Garcia <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
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.
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
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
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
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
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 <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> Cc: Gregory Clement <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>
> Cc: Ezequiel Garcia <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> --
> 1.8.5.2
>
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 <[email protected]>
> > Cc: Jason Cooper <[email protected]>
> > Cc: Andrew Lunn <[email protected]>
> > Cc: Gregory Clement <[email protected]>
> > Cc: Thomas Petazzoni <[email protected]>
> > Cc: Ezequiel Garcia <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > --
> > 1.8.5.2
> >
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.
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 <[email protected]>
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
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 <[email protected]>
>
> 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.
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
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
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
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
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
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
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
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
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
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
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
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
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.