Using orphan clocks can introduce strange behaviour as they don't have
rate information at all and also of course don't track
This v2/v3 takes into account suggestions from Stephen Boyd to not try to
walk the clock tree at runtime but instead keep track of orphan states
on clock tree changes and making it mandatory for everybody from the
start as orphaned clocks should not be used at all.
This fixes an issue on most rk3288 platforms, where some soc-clocks
are supplied by a 32khz clock from an external i2c-chip which often
is only probed later in the boot process and maybe even after the
drivers using these soc-clocks like the tsadc temperature sensor.
In this case the driver using the clock should of course defer probing
until the clock is actually usable.
As this changes the behaviour for orphan clocks, it would of course
benefit from more testing than on my Rockchip boards. To keep the
recipent-list reasonable and not spam to much I selected one (the topmost)
from the get_maintainer output of each drivers/clk entry.
Hopefully some will provide Tested-by-tags :-)
Thanks
Heiko
changes since v2:
adapt to comments from Stephen Boyd
- rename to clk_core_update_orphan_status
- use bools instead of 0/1 for the status
- remove redundant check in clk_is_orphan
changes since v1:
- track orphan status on clock tree changes instead of walking the
tree on clk_get operations
- make get-deferals mandatory for everybody
Cc: Boris Brezillon <[email protected]>
Cc: Alex Elder <[email protected]>
Cc: Alexandre Belloni <[email protected]>
Cc: Stephen Warren <[email protected]>
Cc: Max Filippov <[email protected]>
Cc: [email protected]
Cc: Zhangfei Gao <[email protected]>
Cc: Santosh Shilimkar <[email protected]>
Cc: Chao Xie <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Stefan Wahren <[email protected]>
Cc: Andrew Bresticker <[email protected]>
Cc: Robert Jarzmik <[email protected]>
Cc: Georgi Djakov <[email protected]>
Cc: Sylwester Nawrocki <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Barry Song <[email protected]>
Cc: Dinh Nguyen <[email protected]>
Cc: Viresh Kumar <[email protected]>
Cc: Gabriel FERNANDEZ <[email protected]>
Cc: [email protected]
Cc: Peter De Schrijver <[email protected]>
Cc: Tero Kristo <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pawel Moll <[email protected]>
Cc: Michal Simek <[email protected]>
Heiko Stuebner (2):
clk: track the orphan status of clocks and their children
clk: prevent orphan clocks from being used
drivers/clk/clk.c | 46 +++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 43 insertions(+), 3 deletions(-)
--
2.1.4
While children of orphan clocks are not carried in the orphan-list itself,
they're nevertheless orphans in their own right as they also don't have an
input-rate available. To ease tracking if a clock is an orphan or has an
orphan in its parent path introduce an orphan field into struct clk and
update it and the fields in child-clocks when a clock gets added or removed
from the orphan-list.
Suggested-by: Stephen Boyd <[email protected]>
Signed-off-by: Heiko Stuebner <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Alex Elder <[email protected]>
Cc: Alexandre Belloni <[email protected]>
Cc: Stephen Warren <[email protected]>
Cc: Max Filippov <[email protected]>
Cc: [email protected]
Cc: Zhangfei Gao <[email protected]>
Cc: Santosh Shilimkar <[email protected]>
Cc: Chao Xie <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Stefan Wahren <[email protected]>
Cc: Andrew Bresticker <[email protected]>
Cc: Robert Jarzmik <[email protected]>
Cc: Georgi Djakov <[email protected]>
Cc: Sylwester Nawrocki <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Barry Song <[email protected]>
Cc: Dinh Nguyen <[email protected]>
Cc: Viresh Kumar <[email protected]>
Cc: Gabriel FERNANDEZ <[email protected]>
Cc: [email protected]
Cc: Peter De Schrijver <[email protected]>
Cc: Tero Kristo <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pawel Moll <[email protected]>
Cc: Michal Simek <[email protected]>
---
drivers/clk/clk.c | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b2361d4..341904f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -62,6 +62,7 @@ struct clk_core {
struct clk_core *new_parent;
struct clk_core *new_child;
unsigned long flags;
+ bool orphan;
unsigned int enable_count;
unsigned int prepare_count;
unsigned long accuracy;
@@ -1401,18 +1402,40 @@ static int clk_fetch_parent_index(struct clk_core *clk,
return -EINVAL;
}
+/*
+ * Update the orphan status of @clk and all its children.
+ */
+static void clk_core_update_orphan_status(struct clk_core *clk, bool is_orphan)
+{
+ struct clk_core *child;
+
+ clk->orphan = is_orphan;
+
+ hlist_for_each_entry(child, &clk->children, child_node)
+ clk_core_update_orphan_status(child, is_orphan);
+}
+
static void clk_reparent(struct clk_core *clk, struct clk_core *new_parent)
{
+ bool was_orphan = clk->orphan;
+
hlist_del(&clk->child_node);
if (new_parent) {
+ bool becomes_orphan = new_parent->orphan;
+
/* avoid duplicate POST_RATE_CHANGE notifications */
if (new_parent->new_child == clk)
new_parent->new_child = NULL;
hlist_add_head(&clk->child_node, &new_parent->children);
+
+ if (was_orphan != becomes_orphan)
+ clk_core_update_orphan_status(clk, becomes_orphan);
} else {
hlist_add_head(&clk->child_node, &clk_orphan_list);
+ if (!was_orphan)
+ clk_core_update_orphan_status(clk, true);
}
clk->parent = new_parent;
@@ -2302,13 +2325,17 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
* clocks and re-parent any that are children of the clock currently
* being clk_init'd.
*/
- if (clk->parent)
+ if (clk->parent) {
hlist_add_head(&clk->child_node,
&clk->parent->children);
- else if (clk->flags & CLK_IS_ROOT)
+ clk->orphan = clk->parent->orphan;
+ } else if (clk->flags & CLK_IS_ROOT) {
hlist_add_head(&clk->child_node, &clk_root_list);
- else
+ clk->orphan = false;
+ } else {
hlist_add_head(&clk->child_node, &clk_orphan_list);
+ clk->orphan = true;
+ }
/*
* Set clk's accuracy. The preferred method is to use
--
2.1.4
Orphan clocks or children of orphan clocks don't have rate information at
all and can produce strange results if they're allowed to be used and the
parent becomes available later on.
So using the newly introduced orphan status bit defer
__of_clk_get_from_provider calls for orphan clocks.
Signed-off-by: Heiko Stuebner <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Alex Elder <[email protected]>
Cc: Alexandre Belloni <[email protected]>
Cc: Stephen Warren <[email protected]>
Cc: Max Filippov <[email protected]>
Cc: [email protected]
Cc: Zhangfei Gao <[email protected]>
Cc: Santosh Shilimkar <[email protected]>
Cc: Chao Xie <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Stefan Wahren <[email protected]>
Cc: Andrew Bresticker <[email protected]>
Cc: Robert Jarzmik <[email protected]>
Cc: Georgi Djakov <[email protected]>
Cc: Sylwester Nawrocki <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Barry Song <[email protected]>
Cc: Dinh Nguyen <[email protected]>
Cc: Viresh Kumar <[email protected]>
Cc: Gabriel FERNANDEZ <[email protected]>
Cc: [email protected]
Cc: Peter De Schrijver <[email protected]>
Cc: Tero Kristo <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pawel Moll <[email protected]>
Cc: Michal Simek <[email protected]>
---
drivers/clk/clk.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 341904f..27d2d4b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2226,6 +2226,14 @@ bool clk_is_match(const struct clk *p, const struct clk *q)
}
EXPORT_SYMBOL_GPL(clk_is_match);
+static bool clk_is_orphan(const struct clk *clk)
+{
+ if (!clk)
+ return false;
+
+ return clk->core->orphan;
+}
+
/**
* __clk_init - initialize the data structures in a struct clk
* @dev: device initializing this clk, placeholder for now
@@ -2968,6 +2976,11 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
if (provider->node == clkspec->np)
clk = provider->get(clkspec, provider->data);
if (!IS_ERR(clk)) {
+ if (clk_is_orphan(clk)) {
+ clk = ERR_PTR(-EPROBE_DEFER);
+ break;
+ }
+
clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
con_id);
--
2.1.4
Hi Heiko,
> Heiko Stuebner <[email protected]> hat am 22. April 2015 um 22:53 geschrieben:
>
>
> Using orphan clocks can introduce strange behaviour as they don't have
> rate information at all and also of course don't track
>
> [...]
>
>
> As this changes the behaviour for orphan clocks, it would of course
> benefit from more testing than on my Rockchip boards. To keep the
> recipent-list reasonable and not spam to much I selected one (the topmost)
> from the get_maintainer output of each drivers/clk entry.
> Hopefully some will provide Tested-by-tags :-)
>
excuse me for my naive question, but what kind of tests do you expect (beside
applying the patches)?
Stefan
>
> Thanks
> Heiko
>
Hi Stefan,
Am Samstag, 25. April 2015, 14:23:39 schrieb Stefan Wahren:
> > Heiko Stuebner <[email protected]> hat am 22. April 2015 um 22:53
> > geschrieben:
> >
> >
> > Using orphan clocks can introduce strange behaviour as they don't have
> > rate information at all and also of course don't track
> >
> > [...]
> >
> >
> > As this changes the behaviour for orphan clocks, it would of course
> > benefit from more testing than on my Rockchip boards. To keep the
> > recipent-list reasonable and not spam to much I selected one (the topmost)
> > from the get_maintainer output of each drivers/clk entry.
> > Hopefully some will provide Tested-by-tags :-)
>
> excuse me for my naive question, but what kind of tests do you expect
> (beside applying the patches)?
just a "everything that worked before still works" :-)
Using orphaned clocks should already produce strange issues most of the time -
for example I see clk_disable mismatches when suspending a rk3288 board,
before this patchset.
But nevertheless we now disallow the usage of orphaned clocks completely while
before it was possible to knowingly/unknowingly use them.
And while hopefully most drivers should handle an EPROBE_DEFER from clk_get
just fine, there may still be one or two lurking around that would need fixing
then ;-)
Heiko
Heiko Stuebner <[email protected]> writes:
> Using orphan clocks can introduce strange behaviour as they don't have
> rate information at all and also of course don't track
Ok, everything works as before on some pxa platforms.
As pxa clocks support is not even fully mainline, this test is not the best you
could get.
Cheers.
--
Robert
On 04/22, Heiko Stuebner wrote:
> @@ -1401,18 +1402,40 @@ static int clk_fetch_parent_index(struct clk_core *clk,
> return -EINVAL;
> }
>
> +/*
> + * Update the orphan status of @clk and all its children.
> + */
> +static void clk_core_update_orphan_status(struct clk_core *clk, bool is_orphan)
Missed s/clk/core/ here. I did it, no need to resend.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 04/22, Heiko Stuebner wrote:
> Orphan clocks or children of orphan clocks don't have rate information at
> all and can produce strange results if they're allowed to be used and the
> parent becomes available later on.
>
> So using the newly introduced orphan status bit defer
> __of_clk_get_from_provider calls for orphan clocks.
>
> Signed-off-by: Heiko Stuebner <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Cc: Alex Elder <[email protected]>
> Cc: Alexandre Belloni <[email protected]>
> Cc: Stephen Warren <[email protected]>
> Cc: Max Filippov <[email protected]>
> Cc: [email protected]
> Cc: Zhangfei Gao <[email protected]>
> Cc: Santosh Shilimkar <[email protected]>
> Cc: Chao Xie <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Stefan Wahren <[email protected]>
> Cc: Andrew Bresticker <[email protected]>
> Cc: Robert Jarzmik <[email protected]>
> Cc: Georgi Djakov <[email protected]>
> Cc: Sylwester Nawrocki <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Barry Song <[email protected]>
> Cc: Dinh Nguyen <[email protected]>
> Cc: Viresh Kumar <[email protected]>
> Cc: Gabriel FERNANDEZ <[email protected]>
> Cc: [email protected]
> Cc: Peter De Schrijver <[email protected]>
> Cc: Tero Kristo <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Pawel Moll <[email protected]>
> Cc: Michal Simek <[email protected]>
Applied to clk-next.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 04/22, Heiko Stuebner wrote:
> Using orphan clocks can introduce strange behaviour as they don't have
> rate information at all and also of course don't track
>
> This v2/v3 takes into account suggestions from Stephen Boyd to not try to
> walk the clock tree at runtime but instead keep track of orphan states
> on clock tree changes and making it mandatory for everybody from the
> start as orphaned clocks should not be used at all.
>
>
> This fixes an issue on most rk3288 platforms, where some soc-clocks
> are supplied by a 32khz clock from an external i2c-chip which often
> is only probed later in the boot process and maybe even after the
> drivers using these soc-clocks like the tsadc temperature sensor.
> In this case the driver using the clock should of course defer probing
> until the clock is actually usable.
>
>
> As this changes the behaviour for orphan clocks, it would of course
> benefit from more testing than on my Rockchip boards. To keep the
> recipent-list reasonable and not spam to much I selected one (the topmost)
> from the get_maintainer output of each drivers/clk entry.
> Hopefully some will provide Tested-by-tags :-)
>
<grumble> I don't see any Tested-by: tags yet </grumble>. I've
put these two patches on a separate branch "defer-orphans" and
pushed it to clk-next so we can give it some more exposure.
Unfortunately this doesn't solve the orphan problem for non-OF
providers. What if we did the orphan check in __clk_create_clk()
instead and returned an error pointer for orphans? I suspect that
will solve all cases, right?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Am Donnerstag, 30. April 2015, 17:19:01 schrieb Stephen Boyd:
> On 04/22, Heiko Stuebner wrote:
> > Using orphan clocks can introduce strange behaviour as they don't have
> > rate information at all and also of course don't track
> >
> > This v2/v3 takes into account suggestions from Stephen Boyd to not try to
> > walk the clock tree at runtime but instead keep track of orphan states
> > on clock tree changes and making it mandatory for everybody from the
> > start as orphaned clocks should not be used at all.
> >
> >
> > This fixes an issue on most rk3288 platforms, where some soc-clocks
> > are supplied by a 32khz clock from an external i2c-chip which often
> > is only probed later in the boot process and maybe even after the
> > drivers using these soc-clocks like the tsadc temperature sensor.
> > In this case the driver using the clock should of course defer probing
> > until the clock is actually usable.
> >
> >
> > As this changes the behaviour for orphan clocks, it would of course
> > benefit from more testing than on my Rockchip boards. To keep the
> > recipent-list reasonable and not spam to much I selected one (the topmost)
> > from the get_maintainer output of each drivers/clk entry.
> > Hopefully some will provide Tested-by-tags :-)
>
> <grumble> I don't see any Tested-by: tags yet </grumble>. I've
> put these two patches on a separate branch "defer-orphans" and
> pushed it to clk-next so we can give it some more exposure.
>
> Unfortunately this doesn't solve the orphan problem for non-OF
> providers. What if we did the orphan check in __clk_create_clk()
> instead and returned an error pointer for orphans? I suspect that
> will solve all cases, right?
hmm, clk_register also uses __clk_create_clk, which in turn would prevent
registering orphan-clocks at all, I'd think.
As on my platform I'm dependant on orphan clocks (the soc-level clock gets
registerted as part of the big clock controller way before the i2c-based
supplying clock), I'd rather not touch this :-) .
Instead I guess we could hook it less deep into clk_get_sys, like in the
following patch?
------------ 8< ------------------------- >8 -----------------
From: Heiko Stuebner <[email protected]>
Date: Fri, 1 May 2015 21:50:46 +0200
Subject: [PATCH] clk: prevent orphan access on non-devicetree platforms too
The orphan-check in __of_clk_get_from_provider only prevents orphan-access
on devicetree platforms. To bring non-dt platforms to the same level of
functionality let clk_get_sys (called from clk_get for non-dt platforms)
also check for orphans and return -EPROBE_DEFER in that case.
Signed-off-by: Heiko Stuebner <[email protected]>
---
drivers/clk/clk.c | 2 +-
drivers/clk/clk.h | 5 +++++
drivers/clk/clkdev.c | 5 +++++
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 36d1a01..167d0bf 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2220,7 +2220,7 @@ static inline void clk_debug_unregister(struct clk_core *core)
}
#endif
-static bool clk_is_orphan(const struct clk *clk)
+bool clk_is_orphan(const struct clk *clk)
{
if (!clk)
return false;
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 00b35a1..b8a6061 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -20,6 +20,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
const char *con_id);
void __clk_free_clk(struct clk *clk);
+bool clk_is_orphan(const struct clk *clk);
#else
/* All these casts to avoid ifdefs in clkdev... */
static inline struct clk *
@@ -32,5 +33,9 @@ static struct clk_hw *__clk_get_hw(struct clk *clk)
{
return (struct clk_hw *)clk;
}
+static inline bool clk_is_orphan(const struct clk *clk)
+{
+ return false;
+}
#endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 1fcb6ef..ad96775 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -177,6 +177,11 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
if (!cl)
goto out;
+ if (clk_is_orphan(cl->clk)) {
+ clk = ERR_PTR(-EPROBE_DEFER);
+ goto out;
+ }
+
clk = __clk_create_clk(__clk_get_hw(cl->clk), dev_id, con_id);
if (IS_ERR(clk))
goto out;
--
2.1.4
On 05/01/15 12:59, Heiko St?bner wrote:
> Am Donnerstag, 30. April 2015, 17:19:01 schrieb Stephen Boyd:
>> On 04/22, Heiko Stuebner wrote:
>>> Using orphan clocks can introduce strange behaviour as they don't have
>>> rate information at all and also of course don't track
>>>
>>> This v2/v3 takes into account suggestions from Stephen Boyd to not try to
>>> walk the clock tree at runtime but instead keep track of orphan states
>>> on clock tree changes and making it mandatory for everybody from the
>>> start as orphaned clocks should not be used at all.
>>>
>>>
>>> This fixes an issue on most rk3288 platforms, where some soc-clocks
>>> are supplied by a 32khz clock from an external i2c-chip which often
>>> is only probed later in the boot process and maybe even after the
>>> drivers using these soc-clocks like the tsadc temperature sensor.
>>> In this case the driver using the clock should of course defer probing
>>> until the clock is actually usable.
>>>
>>>
>>> As this changes the behaviour for orphan clocks, it would of course
>>> benefit from more testing than on my Rockchip boards. To keep the
>>> recipent-list reasonable and not spam to much I selected one (the topmost)
>>> from the get_maintainer output of each drivers/clk entry.
>>> Hopefully some will provide Tested-by-tags :-)
>> <grumble> I don't see any Tested-by: tags yet </grumble>. I've
>> put these two patches on a separate branch "defer-orphans" and
>> pushed it to clk-next so we can give it some more exposure.
>>
>> Unfortunately this doesn't solve the orphan problem for non-OF
>> providers. What if we did the orphan check in __clk_create_clk()
>> instead and returned an error pointer for orphans? I suspect that
>> will solve all cases, right?
> hmm, clk_register also uses __clk_create_clk, which in turn would prevent
> registering orphan-clocks at all, I'd think.
> As on my platform I'm dependant on orphan clocks (the soc-level clock gets
> registerted as part of the big clock controller way before the i2c-based
> supplying clock), I'd rather not touch this :-) .
Have no fear! We should just change clk_register() to call a
__clk_create_clk() type function that doesn't check for orphan status.
>
> Instead I guess we could hook it less deep into clk_get_sys, like in the
> following patch?
It looks like it will work at least, but still I'd prefer to keep the
orphan check contained to clk.c. How about this compile tested only patch?
This also brings up an existing problem with clk_unregister() where
orphaned clocks are sitting out there useable by drivers when their
parent is unregistered. That code could use some work to atomically
switch all the orphaned clocks over to use the nodrv_ops.
----8<-----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 30d45c657a07..1d23daa42dd2 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2221,14 +2221,6 @@ static inline void clk_debug_unregister(struct clk_core *core)
}
#endif
-static bool clk_is_orphan(const struct clk *clk)
-{
- if (!clk)
- return false;
-
- return clk->core->orphan;
-}
-
/**
* __clk_init - initialize the data structures in a struct clk
* @dev: device initializing this clk, placeholder for now
@@ -2420,15 +2412,11 @@ out:
return ret;
}
-struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
- const char *con_id)
+static struct clk *clk_hw_create_clk(struct clk_hw *hw, const char *dev_id,
+ const char *con_id)
{
struct clk *clk;
- /* This is to allow this function to be chained to others */
- if (!hw || IS_ERR(hw))
- return (struct clk *) hw;
-
clk = kzalloc(sizeof(*clk), GFP_KERNEL);
if (!clk)
return ERR_PTR(-ENOMEM);
@@ -2445,6 +2433,19 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
return clk;
}
+struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
+ const char *con_id)
+{
+ /* This is to allow this function to be chained to others */
+ if (!hw || IS_ERR(hw))
+ return (struct clk *) hw;
+
+ if (hw->core->orphan)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ return clk_hw_create_clk(hw, dev_id, con_id);
+}
+
void __clk_free_clk(struct clk *clk)
{
clk_prepare_lock();
@@ -2511,7 +2512,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
INIT_HLIST_HEAD(&core->clks);
- hw->clk = __clk_create_clk(hw, NULL, NULL);
+ hw->clk = clk_hw_create_clk(hw, NULL, NULL);
if (IS_ERR(hw->clk)) {
ret = PTR_ERR(hw->clk);
goto fail_parent_names_copy;
@@ -2958,10 +2959,6 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
if (provider->node == clkspec->np)
clk = provider->get(clkspec, provider->data);
if (!IS_ERR(clk)) {
- if (clk_is_orphan(clk)) {
- clk = ERR_PTR(-EPROBE_DEFER);
- break;
- }
clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
con_id);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> On 05/01/15 12:59, Heiko St?bner wrote:
> > Am Donnerstag, 30. April 2015, 17:19:01 schrieb Stephen Boyd:
> >> On 04/22, Heiko Stuebner wrote:
> >>> Using orphan clocks can introduce strange behaviour as they don't have
> >>> rate information at all and also of course don't track
> >>>
> >>> This v2/v3 takes into account suggestions from Stephen Boyd to not try
> >>> to
> >>> walk the clock tree at runtime but instead keep track of orphan states
> >>> on clock tree changes and making it mandatory for everybody from the
> >>> start as orphaned clocks should not be used at all.
> >>>
> >>>
> >>> This fixes an issue on most rk3288 platforms, where some soc-clocks
> >>> are supplied by a 32khz clock from an external i2c-chip which often
> >>> is only probed later in the boot process and maybe even after the
> >>> drivers using these soc-clocks like the tsadc temperature sensor.
> >>> In this case the driver using the clock should of course defer probing
> >>> until the clock is actually usable.
> >>>
> >>>
> >>> As this changes the behaviour for orphan clocks, it would of course
> >>> benefit from more testing than on my Rockchip boards. To keep the
> >>> recipent-list reasonable and not spam to much I selected one (the
> >>> topmost)
> >>> from the get_maintainer output of each drivers/clk entry.
> >>> Hopefully some will provide Tested-by-tags :-)
> >>
> >> <grumble> I don't see any Tested-by: tags yet </grumble>. I've
> >> put these two patches on a separate branch "defer-orphans" and
> >> pushed it to clk-next so we can give it some more exposure.
> >>
> >> Unfortunately this doesn't solve the orphan problem for non-OF
> >> providers. What if we did the orphan check in __clk_create_clk()
> >> instead and returned an error pointer for orphans? I suspect that
> >> will solve all cases, right?
> >
> > hmm, clk_register also uses __clk_create_clk, which in turn would prevent
> > registering orphan-clocks at all, I'd think.
> > As on my platform I'm dependant on orphan clocks (the soc-level clock gets
> > registerted as part of the big clock controller way before the i2c-based
> > supplying clock), I'd rather not touch this :-) .
>
> Have no fear! We should just change clk_register() to call a
> __clk_create_clk() type function that doesn't check for orphan status.
ok :-D
> > Instead I guess we could hook it less deep into clk_get_sys, like in the
> > following patch?
>
> It looks like it will work at least, but still I'd prefer to keep the
> orphan check contained to clk.c. How about this compile tested only patch?
I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
looks the same and it also still defers nicely in the scenario I needed it
for. The implementation also looks nice - and of course much more compact than
my check in two places :-) . I don't know if you want to put this as follow-up
on top or fold it into the original orphan-check, so in any case
Tested-by: Heiko Stuebner <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
> This also brings up an existing problem with clk_unregister() where
> orphaned clocks are sitting out there useable by drivers when their
> parent is unregistered. That code could use some work to atomically
> switch all the orphaned clocks over to use the nodrv_ops.
Not sure I understand this correctly yet, but when these children get
orphaned, switched to the clk_nodrv_ops, they won't get their original ops
back if the parent reappears.
So I guess we would need to store the original ops in secondary property of
struct clk_core and I guess simply bind the ops-switch to the orphan state
update?
>
> ----8<-----
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 30d45c657a07..1d23daa42dd2 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2221,14 +2221,6 @@ static inline void clk_debug_unregister(struct
> clk_core *core) }
> #endif
>
> -static bool clk_is_orphan(const struct clk *clk)
> -{
> - if (!clk)
> - return false;
> -
> - return clk->core->orphan;
> -}
> -
> /**
> * __clk_init - initialize the data structures in a struct clk
> * @dev: device initializing this clk, placeholder for now
> @@ -2420,15 +2412,11 @@ out:
> return ret;
> }
>
> -struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
> - const char *con_id)
> +static struct clk *clk_hw_create_clk(struct clk_hw *hw, const char *dev_id,
> + const char *con_id)
> {
> struct clk *clk;
>
> - /* This is to allow this function to be chained to others */
> - if (!hw || IS_ERR(hw))
> - return (struct clk *) hw;
> -
> clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> if (!clk)
> return ERR_PTR(-ENOMEM);
> @@ -2445,6 +2433,19 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const
> char *dev_id, return clk;
> }
>
> +struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
> + const char *con_id)
> +{
> + /* This is to allow this function to be chained to others */
> + if (!hw || IS_ERR(hw))
> + return (struct clk *) hw;
> +
> + if (hw->core->orphan)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + return clk_hw_create_clk(hw, dev_id, con_id);
> +}
> +
> void __clk_free_clk(struct clk *clk)
> {
> clk_prepare_lock();
> @@ -2511,7 +2512,7 @@ struct clk *clk_register(struct device *dev, struct
> clk_hw *hw)
>
> INIT_HLIST_HEAD(&core->clks);
>
> - hw->clk = __clk_create_clk(hw, NULL, NULL);
> + hw->clk = clk_hw_create_clk(hw, NULL, NULL);
> if (IS_ERR(hw->clk)) {
> ret = PTR_ERR(hw->clk);
> goto fail_parent_names_copy;
> @@ -2958,10 +2959,6 @@ struct clk *__of_clk_get_from_provider(struct
> of_phandle_args *clkspec, if (provider->node == clkspec->np)
> clk = provider->get(clkspec, provider->data);
> if (!IS_ERR(clk)) {
> - if (clk_is_orphan(clk)) {
> - clk = ERR_PTR(-EPROBE_DEFER);
> - break;
> - }
>
> clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
> con_id);
On 05/01/15 15:07, Heiko St?bner wrote:
> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>
>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>>> following patch?
>> It looks like it will work at least, but still I'd prefer to keep the
>> orphan check contained to clk.c. How about this compile tested only patch?
> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
> looks the same and it also still defers nicely in the scenario I needed it
> for. The implementation also looks nice - and of course much more compact than
> my check in two places :-) . I don't know if you want to put this as follow-up
> on top or fold it into the original orphan-check, so in any case
>
> Tested-by: Heiko Stuebner <[email protected]>
> Reviewed-by: Heiko Stuebner <[email protected]>
Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
my patch and a note that it's based on an earlier patch from you.
>
>
>> This also brings up an existing problem with clk_unregister() where
>> orphaned clocks are sitting out there useable by drivers when their
>> parent is unregistered. That code could use some work to atomically
>> switch all the orphaned clocks over to use the nodrv_ops.
> Not sure I understand this correctly yet, but when these children get
> orphaned, switched to the clk_nodrv_ops, they won't get their original ops
> back if the parent reappears.
>
> So I guess we would need to store the original ops in secondary property of
> struct clk_core and I guess simply bind the ops-switch to the orphan state
> update?
Yep. We'll need to store away the original ops in case we need to put
them back. Don't feel obligated to fix this either. It would certainly
be nice if someone tried to fix this case at some point, but it's not
like things are any worse off right now.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 05/02/2015 02:40 AM, Stephen Boyd wrote:
> On 05/01/15 15:07, Heiko St?bner wrote:
>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>
>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>>>> following patch?
>>> It looks like it will work at least, but still I'd prefer to keep the
>>> orphan check contained to clk.c. How about this compile tested only patch?
>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
>> looks the same and it also still defers nicely in the scenario I needed it
>> for. The implementation also looks nice - and of course much more compact than
>> my check in two places :-) . I don't know if you want to put this as follow-up
>> on top or fold it into the original orphan-check, so in any case
>>
>> Tested-by: Heiko Stuebner <[email protected]>
>> Reviewed-by: Heiko Stuebner <[email protected]>
>
> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> my patch and a note that it's based on an earlier patch from you.
FWIW, just gave a try for these two patches on all TI boards I have
access to.
Tested-by: Tero Kristo <[email protected]>
I didn't try your evolved patch though, as you don't seem to have made
your mind yet.
-Tero
>
>>
>>
>>> This also brings up an existing problem with clk_unregister() where
>>> orphaned clocks are sitting out there useable by drivers when their
>>> parent is unregistered. That code could use some work to atomically
>>> switch all the orphaned clocks over to use the nodrv_ops.
>> Not sure I understand this correctly yet, but when these children get
>> orphaned, switched to the clk_nodrv_ops, they won't get their original ops
>> back if the parent reappears.
>>
>> So I guess we would need to store the original ops in secondary property of
>> struct clk_core and I guess simply bind the ops-switch to the orphan state
>> update?
>
> Yep. We'll need to store away the original ops in case we need to put
> them back. Don't feel obligated to fix this either. It would certainly
> be nice if someone tried to fix this case at some point, but it's not
> like things are any worse off right now.
>
On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
> On 05/01/15 15:07, Heiko Stübner wrote:
>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>
>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>>>> following patch?
>>> It looks like it will work at least, but still I'd prefer to keep the
>>> orphan check contained to clk.c. How about this compile tested only patch?
>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
>> looks the same and it also still defers nicely in the scenario I needed it
>> for. The implementation also looks nice - and of course much more compact than
>> my check in two places :-) . I don't know if you want to put this as follow-up
>> on top or fold it into the original orphan-check, so in any case
>>
>> Tested-by: Heiko Stuebner <[email protected]>
>> Reviewed-by: Heiko Stuebner <[email protected]>
>
> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> my patch and a note that it's based on an earlier patch from you.
It appears this has landed in linux-next in the form of 882667c1fcf1
clk: prevent orphan clocks from being used. A bunch of boot failures
for sunxi in today's linux-next[1] were bisected down to that patch.
I confirmed that reverting that commit on top of next/master gets
sunxi booting again.
Kevin
[1] http://kernelci.org/boot/all/job/next/kernel/next-20150507/
On 05/07/15 01:22, Tero Kristo wrote:
> On 05/02/2015 02:40 AM, Stephen Boyd wrote:
>> On 05/01/15 15:07, Heiko St?bner wrote:
>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>>
>>>>> Instead I guess we could hook it less deep into clk_get_sys, like
>>>>> in the
>>>>> following patch?
>>>> It looks like it will work at least, but still I'd prefer to keep the
>>>> orphan check contained to clk.c. How about this compile tested only
>>>> patch?
>>> I gave this a spin on my rk3288-firefly board. It still boots, the
>>> clock tree
>>> looks the same and it also still defers nicely in the scenario I
>>> needed it
>>> for. The implementation also looks nice - and of course much more
>>> compact than
>>> my check in two places :-) . I don't know if you want to put this as
>>> follow-up
>>> on top or fold it into the original orphan-check, so in any case
>>>
>>> Tested-by: Heiko Stuebner <[email protected]>
>>> Reviewed-by: Heiko Stuebner <[email protected]>
>>
>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
>> my patch and a note that it's based on an earlier patch from you.
>
> FWIW, just gave a try for these two patches on all TI boards I have
> access to.
>
> Tested-by: Tero Kristo <[email protected]>
>
> I didn't try your evolved patch though, as you don't seem to have made
> your mind yet.
>
Thanks. Can you try the evolved patch? It's in linux-next now as commit
882667c1fcf1, and it seems to at least break sunxi boot. I'd be
interested if it broke TI boards.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 05/07/15 08:17, Kevin Hilman wrote:
> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
>> On 05/01/15 15:07, Heiko Stübner wrote:
>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>>
>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>>>>> following patch?
>>>> It looks like it will work at least, but still I'd prefer to keep the
>>>> orphan check contained to clk.c. How about this compile tested only patch?
>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
>>> looks the same and it also still defers nicely in the scenario I needed it
>>> for. The implementation also looks nice - and of course much more compact than
>>> my check in two places :-) . I don't know if you want to put this as follow-up
>>> on top or fold it into the original orphan-check, so in any case
>>>
>>> Tested-by: Heiko Stuebner <[email protected]>
>>> Reviewed-by: Heiko Stuebner <[email protected]>
>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
>> my patch and a note that it's based on an earlier patch from you.
> It appears this has landed in linux-next in the form of 882667c1fcf1
> clk: prevent orphan clocks from being used. A bunch of boot failures
> for sunxi in today's linux-next[1] were bisected down to that patch.
>
> I confirmed that reverting that commit on top of next/master gets
> sunxi booting again.
>
>
Thanks for the report. I've removed the two clk orphan patches from
clk-next. Would it be possible to try with next-20150507 and
clk_ignore_unused on the command line? Also we can try to see if
critical clocks aren't being forced on by applying this patch and
looking for clk_get() failures
------8<------
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 7e1e2bd189b6..d88585b680bb 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -1347,6 +1347,9 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
if (!IS_ERR(clk))
clk_prepare_enable(clk);
+ else
+ pr_err("Failed to enable critical clock %s\n",
+ clocks[i]);
}
}
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <[email protected]> wrote:
> On 05/07/15 08:17, Kevin Hilman wrote:
>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
>>> On 05/01/15 15:07, Heiko Stübner wrote:
>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>>>
>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>>>>>> following patch?
>>>>> It looks like it will work at least, but still I'd prefer to keep the
>>>>> orphan check contained to clk.c. How about this compile tested only patch?
>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
>>>> looks the same and it also still defers nicely in the scenario I needed it
>>>> for. The implementation also looks nice - and of course much more compact than
>>>> my check in two places :-) . I don't know if you want to put this as follow-up
>>>> on top or fold it into the original orphan-check, so in any case
>>>>
>>>> Tested-by: Heiko Stuebner <[email protected]>
>>>> Reviewed-by: Heiko Stuebner <[email protected]>
>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
>>> my patch and a note that it's based on an earlier patch from you.
>> It appears this has landed in linux-next in the form of 882667c1fcf1
>> clk: prevent orphan clocks from being used. A bunch of boot failures
>> for sunxi in today's linux-next[1] were bisected down to that patch.
>>
>> I confirmed that reverting that commit on top of next/master gets
>> sunxi booting again.
>>
>>
>
> Thanks for the report. I've removed the two clk orphan patches from
> clk-next. Would it be possible to try with next-20150507 and
> clk_ignore_unused on the command line?
That doesn't help. I tried on cubieboard2 and bananapi.
> Also we can try to see if
> critical clocks aren't being forced on by applying this patch and
> looking for clk_get() failures
>From cubieboard2, there's a few that look rather important:
[ 0.000000] Additional per-CPU info printed with stalls.
[ 0.000000] Build-time adjustment of leaf fanout to 32.
[ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2.
[ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2
[ 0.000000] NR_IRQS:16 nr_irqs:16 16
[ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054
[ 0.000000] Failed to enable critical clock cpu
[ 0.000000] Failed to enable critical clock pll5_ddr
[ 0.000000] Failed to enable critical clock ahb_sdram
[ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt).
Kevin
On 05/07, Kevin Hilman wrote:
> On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <[email protected]> wrote:
> > On 05/07/15 08:17, Kevin Hilman wrote:
> >> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
> >>> On 05/01/15 15:07, Heiko St?bner wrote:
> >>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> >>>>
> >>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
> >>>>>> following patch?
> >>>>> It looks like it will work at least, but still I'd prefer to keep the
> >>>>> orphan check contained to clk.c. How about this compile tested only patch?
> >>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
> >>>> looks the same and it also still defers nicely in the scenario I needed it
> >>>> for. The implementation also looks nice - and of course much more compact than
> >>>> my check in two places :-) . I don't know if you want to put this as follow-up
> >>>> on top or fold it into the original orphan-check, so in any case
> >>>>
> >>>> Tested-by: Heiko Stuebner <[email protected]>
> >>>> Reviewed-by: Heiko Stuebner <[email protected]>
> >>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> >>> my patch and a note that it's based on an earlier patch from you.
> >> It appears this has landed in linux-next in the form of 882667c1fcf1
> >> clk: prevent orphan clocks from being used. A bunch of boot failures
> >> for sunxi in today's linux-next[1] were bisected down to that patch.
> >>
> >> I confirmed that reverting that commit on top of next/master gets
> >> sunxi booting again.
> >>
> >>
> >
> > Thanks for the report. I've removed the two clk orphan patches from
> > clk-next. Would it be possible to try with next-20150507 and
> > clk_ignore_unused on the command line?
>
> That doesn't help. I tried on cubieboard2 and bananapi.
Thanks for trying.
>
> > Also we can try to see if
> > critical clocks aren't being forced on by applying this patch and
> > looking for clk_get() failures
>
> From cubieboard2, there's a few that look rather important:
>
> [ 0.000000] Additional per-CPU info printed with stalls.
> [ 0.000000] Build-time adjustment of leaf fanout to 32.
> [ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2.
> [ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2
> [ 0.000000] NR_IRQS:16 nr_irqs:16 16
> [ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054
> [ 0.000000] Failed to enable critical clock cpu
> [ 0.000000] Failed to enable critical clock pll5_ddr
> [ 0.000000] Failed to enable critical clock ahb_sdram
> [ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt).
Ok. So it seems we need to come up with some solution to the
"critical clocks" problem that doesn't require the individual
clock drivers to call clk_prepare_enable().
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, May 07, 2015 at 11:53:18PM -0700, Stephen Boyd wrote:
> On 05/07, Kevin Hilman wrote:
> > On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <[email protected]> wrote:
> > > On 05/07/15 08:17, Kevin Hilman wrote:
> > >> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
> > >>> On 05/01/15 15:07, Heiko St?bner wrote:
> > >>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> > >>>>
> > >>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
> > >>>>>> following patch?
> > >>>>> It looks like it will work at least, but still I'd prefer to keep the
> > >>>>> orphan check contained to clk.c. How about this compile tested only patch?
> > >>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
> > >>>> looks the same and it also still defers nicely in the scenario I needed it
> > >>>> for. The implementation also looks nice - and of course much more compact than
> > >>>> my check in two places :-) . I don't know if you want to put this as follow-up
> > >>>> on top or fold it into the original orphan-check, so in any case
> > >>>>
> > >>>> Tested-by: Heiko Stuebner <[email protected]>
> > >>>> Reviewed-by: Heiko Stuebner <[email protected]>
> > >>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> > >>> my patch and a note that it's based on an earlier patch from you.
> > >> It appears this has landed in linux-next in the form of 882667c1fcf1
> > >> clk: prevent orphan clocks from being used. A bunch of boot failures
> > >> for sunxi in today's linux-next[1] were bisected down to that patch.
> > >>
> > >> I confirmed that reverting that commit on top of next/master gets
> > >> sunxi booting again.
> > >>
> > >>
> > >
> > > Thanks for the report. I've removed the two clk orphan patches from
> > > clk-next. Would it be possible to try with next-20150507 and
> > > clk_ignore_unused on the command line?
> >
> > That doesn't help. I tried on cubieboard2 and bananapi.
>
> Thanks for trying.
>
> >
> > > Also we can try to see if
> > > critical clocks aren't being forced on by applying this patch and
> > > looking for clk_get() failures
> >
> > From cubieboard2, there's a few that look rather important:
> >
> > [ 0.000000] Additional per-CPU info printed with stalls.
> > [ 0.000000] Build-time adjustment of leaf fanout to 32.
> > [ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2.
> > [ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2
> > [ 0.000000] NR_IRQS:16 nr_irqs:16 16
> > [ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054
> > [ 0.000000] Failed to enable critical clock cpu
> > [ 0.000000] Failed to enable critical clock pll5_ddr
> > [ 0.000000] Failed to enable critical clock ahb_sdram
> > [ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt).
>
> Ok. So it seems we need to come up with some solution to the
> "critical clocks" problem that doesn't require the individual
> clock drivers to call clk_prepare_enable().
I'm getting more and more unsure if we can really handle the complexity
we get by allowing to register orphaned clocks. On one hand we can't
handle the orphaned clocks properly when we do a clk_prepare/enable on
them, on the other hand we run into trouble when we forbid to
prepare/enable them. The fact that clocks can become orphans by
reparenting them makes it even more complicated.
Maybe allowing orphans is something that has to be revisited.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Am Freitag, 8. Mai 2015, 10:13:55 schrieb Sascha Hauer:
> On Thu, May 07, 2015 at 11:53:18PM -0700, Stephen Boyd wrote:
> > On 05/07, Kevin Hilman wrote:
> > > On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <[email protected]>
wrote:
> > > > On 05/07/15 08:17, Kevin Hilman wrote:
> > > >> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]>
wrote:
> > > >>> On 05/01/15 15:07, Heiko St?bner wrote:
> > > >>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> > > >>>>>> Instead I guess we could hook it less deep into clk_get_sys, like
> > > >>>>>> in the
> > > >>>>>> following patch?
> > > >>>>>
> > > >>>>> It looks like it will work at least, but still I'd prefer to keep
> > > >>>>> the
> > > >>>>> orphan check contained to clk.c. How about this compile tested
> > > >>>>> only patch?
> > > >>>>
> > > >>>> I gave this a spin on my rk3288-firefly board. It still boots, the
> > > >>>> clock tree looks the same and it also still defers nicely in the
> > > >>>> scenario I needed it for. The implementation also looks nice - and
> > > >>>> of course much more compact than my check in two places :-) . I
> > > >>>> don't know if you want to put this as follow-up on top or fold it
> > > >>>> into the original orphan-check, so in any case
> > > >>>>
> > > >>>> Tested-by: Heiko Stuebner <[email protected]>
> > > >>>> Reviewed-by: Heiko Stuebner <[email protected]>
> > > >>>
> > > >>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it
> > > >>> with
> > > >>> my patch and a note that it's based on an earlier patch from you.
> > > >>
> > > >> It appears this has landed in linux-next in the form of 882667c1fcf1
> > > >> clk: prevent orphan clocks from being used. A bunch of boot failures
> > > >> for sunxi in today's linux-next[1] were bisected down to that patch.
> > > >>
> > > >> I confirmed that reverting that commit on top of next/master gets
> > > >> sunxi booting again.
> > > >
> > > > Thanks for the report. I've removed the two clk orphan patches from
> > > > clk-next. Would it be possible to try with next-20150507 and
> > > > clk_ignore_unused on the command line?
> > >
> > > That doesn't help. I tried on cubieboard2 and bananapi.
> >
> > Thanks for trying.
> >
> > > > Also we can try to see if
> > > > critical clocks aren't being forced on by applying this patch and
> > > > looking for clk_get() failures
> > >
> > > From cubieboard2, there's a few that look rather important:
> > >
> > > [ 0.000000] Additional per-CPU info printed with stalls.
> > > [ 0.000000] Build-time adjustment of leaf fanout to 32.
> > > [ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2.
> > > [ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32,
> > > nr_cpu_ids=2
> > > [ 0.000000] NR_IRQS:16 nr_irqs:16 16
> > > [ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054
> > > [ 0.000000] Failed to enable critical clock cpu
> > > [ 0.000000] Failed to enable critical clock pll5_ddr
> > > [ 0.000000] Failed to enable critical clock ahb_sdram
> > > [ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt).
> >
> > Ok. So it seems we need to come up with some solution to the
> > "critical clocks" problem that doesn't require the individual
> > clock drivers to call clk_prepare_enable().
>
> I'm getting more and more unsure if we can really handle the complexity
> we get by allowing to register orphaned clocks. On one hand we can't
> handle the orphaned clocks properly when we do a clk_prepare/enable on
> them, on the other hand we run into trouble when we forbid to
> prepare/enable them. The fact that clocks can become orphans by
> reparenting them makes it even more complicated.
> Maybe allowing orphans is something that has to be revisited.
hmm, I don't see it this drastic. I was expecting a lot more fallout from
changing the behaviour of orphaned clocks over all arches using the CCF.
>From the kernelci-boards only the Sunxi-ones seem to have been affected at all
and also only because they need a "regulator-always-on" equivalent in the CCF,
which currently hijacks prepare/enable inside the clock driver to achive this.
On the Rockchip clocks we have something similar, with the only difference that
it is done after all clocks are registered and does not need to access the 3
orphans we have at this point due to their source clock coming from an
external i2c-connected chip that gets probed a lot later.
On Fri, May 08, 2015 at 11:30:19AM +0200, Heiko St?bner wrote:
> Am Freitag, 8. Mai 2015, 10:13:55 schrieb Sascha Hauer:
> > On Thu, May 07, 2015 at 11:53:18PM -0700, Stephen Boyd wrote:
> > > On 05/07, Kevin Hilman wrote:
> > > > On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <[email protected]>
> wrote:
> > > > > On 05/07/15 08:17, Kevin Hilman wrote:
> > > > >> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]>
> wrote:
> > > > >>> On 05/01/15 15:07, Heiko St?bner wrote:
> > > > >>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> > > > >>>>>> Instead I guess we could hook it less deep into clk_get_sys, like
> > > > >>>>>> in the
> > > > >>>>>> following patch?
> > > > >>>>>
> > > > >>>>> It looks like it will work at least, but still I'd prefer to keep
> > > > >>>>> the
> > > > >>>>> orphan check contained to clk.c. How about this compile tested
> > > > >>>>> only patch?
> > > > >>>>
> > > > >>>> I gave this a spin on my rk3288-firefly board. It still boots, the
> > > > >>>> clock tree looks the same and it also still defers nicely in the
> > > > >>>> scenario I needed it for. The implementation also looks nice - and
> > > > >>>> of course much more compact than my check in two places :-) . I
> > > > >>>> don't know if you want to put this as follow-up on top or fold it
> > > > >>>> into the original orphan-check, so in any case
> > > > >>>>
> > > > >>>> Tested-by: Heiko Stuebner <[email protected]>
> > > > >>>> Reviewed-by: Heiko Stuebner <[email protected]>
> > > > >>>
> > > > >>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it
> > > > >>> with
> > > > >>> my patch and a note that it's based on an earlier patch from you.
> > > > >>
> > > > >> It appears this has landed in linux-next in the form of 882667c1fcf1
> > > > >> clk: prevent orphan clocks from being used. A bunch of boot failures
> > > > >> for sunxi in today's linux-next[1] were bisected down to that patch.
> > > > >>
> > > > >> I confirmed that reverting that commit on top of next/master gets
> > > > >> sunxi booting again.
> > > > >
> > > > > Thanks for the report. I've removed the two clk orphan patches from
> > > > > clk-next. Would it be possible to try with next-20150507 and
> > > > > clk_ignore_unused on the command line?
> > > >
> > > > That doesn't help. I tried on cubieboard2 and bananapi.
> > >
> > > Thanks for trying.
> > >
> > > > > Also we can try to see if
> > > > > critical clocks aren't being forced on by applying this patch and
> > > > > looking for clk_get() failures
> > > >
> > > > From cubieboard2, there's a few that look rather important:
> > > >
> > > > [ 0.000000] Additional per-CPU info printed with stalls.
> > > > [ 0.000000] Build-time adjustment of leaf fanout to 32.
> > > > [ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2.
> > > > [ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32,
> > > > nr_cpu_ids=2
> > > > [ 0.000000] NR_IRQS:16 nr_irqs:16 16
> > > > [ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054
> > > > [ 0.000000] Failed to enable critical clock cpu
> > > > [ 0.000000] Failed to enable critical clock pll5_ddr
> > > > [ 0.000000] Failed to enable critical clock ahb_sdram
> > > > [ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt).
> > >
> > > Ok. So it seems we need to come up with some solution to the
> > > "critical clocks" problem that doesn't require the individual
> > > clock drivers to call clk_prepare_enable().
> >
> > I'm getting more and more unsure if we can really handle the complexity
> > we get by allowing to register orphaned clocks. On one hand we can't
> > handle the orphaned clocks properly when we do a clk_prepare/enable on
> > them, on the other hand we run into trouble when we forbid to
> > prepare/enable them. The fact that clocks can become orphans by
> > reparenting them makes it even more complicated.
> > Maybe allowing orphans is something that has to be revisited.
>
> hmm, I don't see it this drastic. I was expecting a lot more fallout from
> changing the behaviour of orphaned clocks over all arches using the CCF.
>
> From the kernelci-boards only the Sunxi-ones seem to have been affected at all
> and also only because they need a "regulator-always-on" equivalent in the CCF,
> which currently hijacks prepare/enable inside the clock driver to achive this.
Mediatek has the same case. Here we needs some clock to stay always on and
these clocks get registered before the PLLs they depend on. This means
they can't be enabled right after registration.
Keeping the prepare/enable count correct and syncing the software state
with the hardware state is made quite complicated with orphaned clocks
and I suspect more bugs here. Even now it's hard to move in the
clock framework without breaking other stuff, this won't get easier with
more bug fixes.
So I think asking what allowing orphaned clocks really buys us is valid.
Of course we would need to find a way to get the initialisation order
straight which will probably cause some pain.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote:
> On 05/07/15 08:17, Kevin Hilman wrote:
> > On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
> >> On 05/01/15 15:07, Heiko St?bner wrote:
> >>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> >>>
> >>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
> >>>>> following patch?
> >>>> It looks like it will work at least, but still I'd prefer to keep the
> >>>> orphan check contained to clk.c. How about this compile tested only patch?
> >>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
> >>> looks the same and it also still defers nicely in the scenario I needed it
> >>> for. The implementation also looks nice - and of course much more compact than
> >>> my check in two places :-) . I don't know if you want to put this as follow-up
> >>> on top or fold it into the original orphan-check, so in any case
> >>>
> >>> Tested-by: Heiko Stuebner <[email protected]>
> >>> Reviewed-by: Heiko Stuebner <[email protected]>
> >> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> >> my patch and a note that it's based on an earlier patch from you.
> > It appears this has landed in linux-next in the form of 882667c1fcf1
> > clk: prevent orphan clocks from being used. A bunch of boot failures
> > for sunxi in today's linux-next[1] were bisected down to that patch.
> >
> > I confirmed that reverting that commit on top of next/master gets
> > sunxi booting again.
> >
> >
>
> Thanks for the report. I've removed the two clk orphan patches from
> clk-next. Would it be possible to try with next-20150507 and
> clk_ignore_unused on the command line?
This makes it work, but it's not really an option.
> Also we can try to see if critical clocks aren't being forced on by
> applying this patch and looking for clk_get() failures
And that shows that the CPU and DDR clocks are not protected, which
obviously is pretty mad.
I've mass converted all our probing code to use OF_CLK_DECLARE, and
make things work again.
http://code.bulix.org/5goa5j-88345?raw
Is this an acceptable solution?
We were already moving to this, I'm not really fond of doing this like
that, but I guess this whole debacle makes it necessary.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 05/07/2015 09:18 PM, Stephen Boyd wrote:
> On 05/07/15 01:22, Tero Kristo wrote:
>> On 05/02/2015 02:40 AM, Stephen Boyd wrote:
>>> On 05/01/15 15:07, Heiko St?bner wrote:
>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>>>
>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like
>>>>>> in the
>>>>>> following patch?
>>>>> It looks like it will work at least, but still I'd prefer to keep the
>>>>> orphan check contained to clk.c. How about this compile tested only
>>>>> patch?
>>>> I gave this a spin on my rk3288-firefly board. It still boots, the
>>>> clock tree
>>>> looks the same and it also still defers nicely in the scenario I
>>>> needed it
>>>> for. The implementation also looks nice - and of course much more
>>>> compact than
>>>> my check in two places :-) . I don't know if you want to put this as
>>>> follow-up
>>>> on top or fold it into the original orphan-check, so in any case
>>>>
>>>> Tested-by: Heiko Stuebner <[email protected]>
>>>> Reviewed-by: Heiko Stuebner <[email protected]>
>>>
>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
>>> my patch and a note that it's based on an earlier patch from you.
>>
>> FWIW, just gave a try for these two patches on all TI boards I have
>> access to.
>>
>> Tested-by: Tero Kristo <[email protected]>
>>
>> I didn't try your evolved patch though, as you don't seem to have made
>> your mind yet.
>>
>
> Thanks. Can you try the evolved patch? It's in linux-next now as commit
> 882667c1fcf1, and it seems to at least break sunxi boot. I'd be
> interested if it broke TI boards.
Just tried it out, boots fine on all these:
: Board : Boot commit log
1: am335x-evm : PASS 4.1.0-rc2-next-20150507 am335x-evm.txt
2: am335x-evmsk : PASS 4.1.0-rc2-next-20150507 am335x-sk.txt
3: am3517-evm : PASS 4.1.0-rc2-next-20150507 am3517-evm.txt
4: am43x-epos-evm : PASS 4.1.0-rc2-next-20150507 am43xx-epos.txt
5: am437x-gp-evm : PASS 4.1.0-rc2-next-20150507 am43xx-gpevm.txt
6: am57xx-evm : PASS 4.1.0-rc2-next-20150507 am57xx-evm.txt
7: omap3-beagle-xm : PASS 4.1.0-rc2-next-20150507 beagleboard.txt
8: omap3-beagle : PASS 4.1.0-rc2-next-20150507
beagleboard-vanilla.txt
9: am335x-boneblack: PASS 4.1.0-rc2-next-20150507 beaglebone-black.txt
10: am335x-bone : PASS 4.1.0-rc2-next-20150507 beaglebone.txt
11: dra7xx-evm : PASS 4.1.0-rc2-next-20150507 dra7xx-evm.txt
12: omap3-n900 : PASS 4.1.0-rc2-next-20150507 n900.txt
13: omap5-uevm : PASS 4.1.0-rc2-next-20150507 omap5-evm.txt
14: omap4-panda-es : PASS 4.1.0-rc2-next-20150507 pandaboard-es.txt
15: omap4-panda : PASS 4.1.0-rc2-next-20150507 pandaboard-vanilla.txt
16: omap2430-sdp : PASS 4.1.0-rc2-next-20150507 sdp2430.txt
17: omap3430-sdp : PASS 4.1.0-rc2-next-20150507 sdp3430.txt
18: omap4-sdp-es23plus: PASS 4.1.0-rc2-next-20150507 sdp4430.txt
TOTAL = 18 boards, Booted Boards = 18, No Boot boards = 0
TI boards do not have any orphan clocks.
-Tero
On 05/08/15 03:02, Maxime Ripard wrote:
> On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote:
>> On 05/07/15 08:17, Kevin Hilman wrote:
>>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
>>>> On 05/01/15 15:07, Heiko St?bner wrote:
>>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>>>>>
>>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>>>>>>> following patch?
>>>>>> It looks like it will work at least, but still I'd prefer to keep the
>>>>>> orphan check contained to clk.c. How about this compile tested only patch?
>>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
>>>>> looks the same and it also still defers nicely in the scenario I needed it
>>>>> for. The implementation also looks nice - and of course much more compact than
>>>>> my check in two places :-) . I don't know if you want to put this as follow-up
>>>>> on top or fold it into the original orphan-check, so in any case
>>>>>
>>>>> Tested-by: Heiko Stuebner <[email protected]>
>>>>> Reviewed-by: Heiko Stuebner <[email protected]>
>>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
>>>> my patch and a note that it's based on an earlier patch from you.
>>> It appears this has landed in linux-next in the form of 882667c1fcf1
>>> clk: prevent orphan clocks from being used. A bunch of boot failures
>>> for sunxi in today's linux-next[1] were bisected down to that patch.
>>>
>>> I confirmed that reverting that commit on top of next/master gets
>>> sunxi booting again.
>>>
>>>
>> Thanks for the report. I've removed the two clk orphan patches from
>> clk-next. Would it be possible to try with next-20150507 and
>> clk_ignore_unused on the command line?
> This makes it work, but it's not really an option.
>
Hmm.. I thought it didn't fix it for Kevin. Confused.
>> Also we can try to see if critical clocks aren't being forced on by
>> applying this patch and looking for clk_get() failures
> And that shows that the CPU and DDR clocks are not protected, which
> obviously is pretty mad.
>
> I've mass converted all our probing code to use OF_CLK_DECLARE, and
> make things work again.
>
> http://code.bulix.org/5goa5j-88345?raw
>
> Is this an acceptable solution?
>
> We were already moving to this, I'm not really fond of doing this like
> that, but I guess this whole debacle makes it necessary.
>
I wonder why we can't switch out the clk_ops on the affected platforms +
clocks to be read-only (at least for the enable/disable part)? That
would fix it just the same right? I wasn't around for the original
discussion regarding this always-on stuff so perhaps I've missed something.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote:
> On 05/08/15 03:02, Maxime Ripard wrote:
> > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote:
> >> On 05/07/15 08:17, Kevin Hilman wrote:
> >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
> >>>> On 05/01/15 15:07, Heiko St?bner wrote:
> >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> >>>>>
> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
> >>>>>>> following patch?
> >>>>>> It looks like it will work at least, but still I'd prefer to keep the
> >>>>>> orphan check contained to clk.c. How about this compile tested only patch?
> >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
> >>>>> looks the same and it also still defers nicely in the scenario I needed it
> >>>>> for. The implementation also looks nice - and of course much more compact than
> >>>>> my check in two places :-) . I don't know if you want to put this as follow-up
> >>>>> on top or fold it into the original orphan-check, so in any case
> >>>>>
> >>>>> Tested-by: Heiko Stuebner <[email protected]>
> >>>>> Reviewed-by: Heiko Stuebner <[email protected]>
> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> >>>> my patch and a note that it's based on an earlier patch from you.
> >>> It appears this has landed in linux-next in the form of 882667c1fcf1
> >>> clk: prevent orphan clocks from being used. A bunch of boot failures
> >>> for sunxi in today's linux-next[1] were bisected down to that patch.
> >>>
> >>> I confirmed that reverting that commit on top of next/master gets
> >>> sunxi booting again.
> >>>
> >>>
> >> Thanks for the report. I've removed the two clk orphan patches from
> >> clk-next. Would it be possible to try with next-20150507 and
> >> clk_ignore_unused on the command line?
> > This makes it work, but it's not really an option.
> >
>
> Hmm.. I thought it didn't fix it for Kevin. Confused.
I'm too, but it does fix things here.
> >> Also we can try to see if critical clocks aren't being forced on by
> >> applying this patch and looking for clk_get() failures
> > And that shows that the CPU and DDR clocks are not protected, which
> > obviously is pretty mad.
> >
> > I've mass converted all our probing code to use OF_CLK_DECLARE, and
> > make things work again.
> >
> > http://code.bulix.org/5goa5j-88345?raw
> >
> > Is this an acceptable solution?
> >
> > We were already moving to this, I'm not really fond of doing this like
> > that, but I guess this whole debacle makes it necessary.
> >
>
> I wonder why we can't switch out the clk_ops on the affected platforms +
> clocks to be read-only (at least for the enable/disable part)? That
> would fix it just the same right? I wasn't around for the original
> discussion regarding this always-on stuff so perhaps I've missed something.
We're using clk-gate, so that would require to recode our own
driver. That change seemed less invasive, while fixing the issue and
ensuring that we wouldn't have any orphan clock, which seems to be
hunted down these days...
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Maxime Ripard <[email protected]> writes:
> On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote:
>> On 05/08/15 03:02, Maxime Ripard wrote:
>> > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote:
>> >> On 05/07/15 08:17, Kevin Hilman wrote:
>> >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
>> >>>> On 05/01/15 15:07, Heiko Stübner wrote:
>> >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>> >>>>>
>> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>> >>>>>>> following patch?
>> >>>>>> It looks like it will work at least, but still I'd prefer to keep the
>> >>>>>> orphan check contained to clk.c. How about this compile tested only patch?
>> >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
>> >>>>> looks the same and it also still defers nicely in the scenario I needed it
>> >>>>> for. The implementation also looks nice - and of course much more compact than
>> >>>>> my check in two places :-) . I don't know if you want to put this as follow-up
>> >>>>> on top or fold it into the original orphan-check, so in any case
>> >>>>>
>> >>>>> Tested-by: Heiko Stuebner <[email protected]>
>> >>>>> Reviewed-by: Heiko Stuebner <[email protected]>
>> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
>> >>>> my patch and a note that it's based on an earlier patch from you.
>> >>> It appears this has landed in linux-next in the form of 882667c1fcf1
>> >>> clk: prevent orphan clocks from being used. A bunch of boot failures
>> >>> for sunxi in today's linux-next[1] were bisected down to that patch.
>> >>>
>> >>> I confirmed that reverting that commit on top of next/master gets
>> >>> sunxi booting again.
>> >>>
>> >>>
>> >> Thanks for the report. I've removed the two clk orphan patches from
>> >> clk-next. Would it be possible to try with next-20150507 and
>> >> clk_ignore_unused on the command line?
>> > This makes it work, but it's not really an option.
>> >
>>
>> Hmm.. I thought it didn't fix it for Kevin. Confused.
>
> I'm too, but it does fix things here.
To be more precise on what I tested. I used next-20150507 and tested on
4 different sunxi platforms. First test was "normal" commandline,
second was with clk_ignore_unused appended:
- cubie: fail, fail
- cubie2: fail, fail
- bananpi: fail, pass
- cubietruck: fail, pass
So it seems to have some effect, but by itself, doesn't fix the issue.
Kevin
On Wed, May 13, 2015 at 07:33:54AM -0700, Kevin Hilman wrote:
> Maxime Ripard <[email protected]> writes:
>
> > On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote:
> >> On 05/08/15 03:02, Maxime Ripard wrote:
> >> > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote:
> >> >> On 05/07/15 08:17, Kevin Hilman wrote:
> >> >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
> >> >>>> On 05/01/15 15:07, Heiko St?bner wrote:
> >> >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> >> >>>>>
> >> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
> >> >>>>>>> following patch?
> >> >>>>>> It looks like it will work at least, but still I'd prefer to keep the
> >> >>>>>> orphan check contained to clk.c. How about this compile tested only patch?
> >> >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
> >> >>>>> looks the same and it also still defers nicely in the scenario I needed it
> >> >>>>> for. The implementation also looks nice - and of course much more compact than
> >> >>>>> my check in two places :-) . I don't know if you want to put this as follow-up
> >> >>>>> on top or fold it into the original orphan-check, so in any case
> >> >>>>>
> >> >>>>> Tested-by: Heiko Stuebner <[email protected]>
> >> >>>>> Reviewed-by: Heiko Stuebner <[email protected]>
> >> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> >> >>>> my patch and a note that it's based on an earlier patch from you.
> >> >>> It appears this has landed in linux-next in the form of 882667c1fcf1
> >> >>> clk: prevent orphan clocks from being used. A bunch of boot failures
> >> >>> for sunxi in today's linux-next[1] were bisected down to that patch.
> >> >>>
> >> >>> I confirmed that reverting that commit on top of next/master gets
> >> >>> sunxi booting again.
> >> >>>
> >> >>>
> >> >> Thanks for the report. I've removed the two clk orphan patches from
> >> >> clk-next. Would it be possible to try with next-20150507 and
> >> >> clk_ignore_unused on the command line?
> >> > This makes it work, but it's not really an option.
> >> >
> >>
> >> Hmm.. I thought it didn't fix it for Kevin. Confused.
> >
> > I'm too, but it does fix things here.
>
> To be more precise on what I tested. I used next-20150507 and tested on
> 4 different sunxi platforms. First test was "normal" commandline,
> second was with clk_ignore_unused appended:
>
> - cubie: fail, fail
> - cubie2: fail, fail
> - bananpi: fail, pass
> - cubietruck: fail, pass
>
> So it seems to have some effect, but by itself, doesn't fix the issue.
It's very odd, I actually tried with a cubie2 here...
I'm booting on an initramfs and not MMC though, but I can't see how
that can be related to our issue...
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Wed, May 13, 2015 at 1:14 PM, Maxime Ripard
<[email protected]> wrote:
> On Wed, May 13, 2015 at 07:33:54AM -0700, Kevin Hilman wrote:
>> Maxime Ripard <[email protected]> writes:
>>
>> > On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote:
>> >> On 05/08/15 03:02, Maxime Ripard wrote:
>> >> > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote:
>> >> >> On 05/07/15 08:17, Kevin Hilman wrote:
>> >> >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
>> >> >>>> On 05/01/15 15:07, Heiko Stübner wrote:
>> >> >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
>> >> >>>>>
>> >> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
>> >> >>>>>>> following patch?
>> >> >>>>>> It looks like it will work at least, but still I'd prefer to keep the
>> >> >>>>>> orphan check contained to clk.c. How about this compile tested only patch?
>> >> >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
>> >> >>>>> looks the same and it also still defers nicely in the scenario I needed it
>> >> >>>>> for. The implementation also looks nice - and of course much more compact than
>> >> >>>>> my check in two places :-) . I don't know if you want to put this as follow-up
>> >> >>>>> on top or fold it into the original orphan-check, so in any case
>> >> >>>>>
>> >> >>>>> Tested-by: Heiko Stuebner <[email protected]>
>> >> >>>>> Reviewed-by: Heiko Stuebner <[email protected]>
>> >> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
>> >> >>>> my patch and a note that it's based on an earlier patch from you.
>> >> >>> It appears this has landed in linux-next in the form of 882667c1fcf1
>> >> >>> clk: prevent orphan clocks from being used. A bunch of boot failures
>> >> >>> for sunxi in today's linux-next[1] were bisected down to that patch.
>> >> >>>
>> >> >>> I confirmed that reverting that commit on top of next/master gets
>> >> >>> sunxi booting again.
>> >> >>>
>> >> >>>
>> >> >> Thanks for the report. I've removed the two clk orphan patches from
>> >> >> clk-next. Would it be possible to try with next-20150507 and
>> >> >> clk_ignore_unused on the command line?
>> >> > This makes it work, but it's not really an option.
>> >> >
>> >>
>> >> Hmm.. I thought it didn't fix it for Kevin. Confused.
>> >
>> > I'm too, but it does fix things here.
>>
>> To be more precise on what I tested. I used next-20150507 and tested on
>> 4 different sunxi platforms. First test was "normal" commandline,
>> second was with clk_ignore_unused appended:
>>
>> - cubie: fail, fail
>> - cubie2: fail, fail
>> - bananpi: fail, pass
>> - cubietruck: fail, pass
>>
>> So it seems to have some effect, but by itself, doesn't fix the issue.
>
> It's very odd, I actually tried with a cubie2 here...
>
> I'm booting on an initramfs and not MMC though, but I can't see how
> that can be related to our issue...
I'm booting an initramfs too.
Kevin
On Wed, May 13, 2015 at 01:44:50PM -0700, Kevin Hilman wrote:
> On Wed, May 13, 2015 at 1:14 PM, Maxime Ripard
> <[email protected]> wrote:
> > On Wed, May 13, 2015 at 07:33:54AM -0700, Kevin Hilman wrote:
> >> Maxime Ripard <[email protected]> writes:
> >>
> >> > On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote:
> >> >> On 05/08/15 03:02, Maxime Ripard wrote:
> >> >> > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote:
> >> >> >> On 05/07/15 08:17, Kevin Hilman wrote:
> >> >> >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]> wrote:
> >> >> >>>> On 05/01/15 15:07, Heiko St?bner wrote:
> >> >> >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> >> >> >>>>>
> >> >> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the
> >> >> >>>>>>> following patch?
> >> >> >>>>>> It looks like it will work at least, but still I'd prefer to keep the
> >> >> >>>>>> orphan check contained to clk.c. How about this compile tested only patch?
> >> >> >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
> >> >> >>>>> looks the same and it also still defers nicely in the scenario I needed it
> >> >> >>>>> for. The implementation also looks nice - and of course much more compact than
> >> >> >>>>> my check in two places :-) . I don't know if you want to put this as follow-up
> >> >> >>>>> on top or fold it into the original orphan-check, so in any case
> >> >> >>>>>
> >> >> >>>>> Tested-by: Heiko Stuebner <[email protected]>
> >> >> >>>>> Reviewed-by: Heiko Stuebner <[email protected]>
> >> >> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with
> >> >> >>>> my patch and a note that it's based on an earlier patch from you.
> >> >> >>> It appears this has landed in linux-next in the form of 882667c1fcf1
> >> >> >>> clk: prevent orphan clocks from being used. A bunch of boot failures
> >> >> >>> for sunxi in today's linux-next[1] were bisected down to that patch.
> >> >> >>>
> >> >> >>> I confirmed that reverting that commit on top of next/master gets
> >> >> >>> sunxi booting again.
> >> >> >>>
> >> >> >>>
> >> >> >> Thanks for the report. I've removed the two clk orphan patches from
> >> >> >> clk-next. Would it be possible to try with next-20150507 and
> >> >> >> clk_ignore_unused on the command line?
> >> >> > This makes it work, but it's not really an option.
> >> >> >
> >> >>
> >> >> Hmm.. I thought it didn't fix it for Kevin. Confused.
> >> >
> >> > I'm too, but it does fix things here.
> >>
> >> To be more precise on what I tested. I used next-20150507 and tested on
> >> 4 different sunxi platforms. First test was "normal" commandline,
> >> second was with clk_ignore_unused appended:
> >>
> >> - cubie: fail, fail
> >> - cubie2: fail, fail
> >> - bananpi: fail, pass
> >> - cubietruck: fail, pass
> >>
> >> So it seems to have some effect, but by itself, doesn't fix the issue.
> >
> > It's very odd, I actually tried with a cubie2 here...
> >
> > I'm booting on an initramfs and not MMC though, but I can't see how
> > that can be related to our issue...
>
> I'm booting an initramfs too.
Then I don't know :)
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Hi Maxime, Stephen,
Am Freitag, 8. Mai 2015, 12:02:47 schrieb Maxime Ripard:
> On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote:
> > On 05/07/15 08:17, Kevin Hilman wrote:
> > > On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <[email protected]>
wrote:
> > >> On 05/01/15 15:07, Heiko St?bner wrote:
> > >>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> > >>>>> Instead I guess we could hook it less deep into clk_get_sys, like in
> > >>>>> the
> > >>>>> following patch?
> > >>>>
> > >>>> It looks like it will work at least, but still I'd prefer to keep the
> > >>>> orphan check contained to clk.c. How about this compile tested only
> > >>>> patch?
> > >>>
> > >>> I gave this a spin on my rk3288-firefly board. It still boots, the
> > >>> clock tree looks the same and it also still defers nicely in the
> > >>> scenario I needed it for. The implementation also looks nice - and of
> > >>> course much more compact than my check in two places :-) . I don't
> > >>> know if you want to put this as follow-up on top or fold it into the
> > >>> original orphan-check, so in any case
> > >>>
> > >>> Tested-by: Heiko Stuebner <[email protected]>
> > >>> Reviewed-by: Heiko Stuebner <[email protected]>
> > >>
> > >> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it
> > >> with
> > >> my patch and a note that it's based on an earlier patch from you.
> > >
> > > It appears this has landed in linux-next in the form of 882667c1fcf1
> > > clk: prevent orphan clocks from being used. A bunch of boot failures
> > > for sunxi in today's linux-next[1] were bisected down to that patch.
> > >
> > > I confirmed that reverting that commit on top of next/master gets
> > > sunxi booting again.
> >
> > Thanks for the report. I've removed the two clk orphan patches from
> > clk-next. Would it be possible to try with next-20150507 and
> > clk_ignore_unused on the command line?
>
> This makes it work, but it's not really an option.
>
> > Also we can try to see if critical clocks aren't being forced on by
> > applying this patch and looking for clk_get() failures
>
> And that shows that the CPU and DDR clocks are not protected, which
> obviously is pretty mad.
>
> I've mass converted all our probing code to use OF_CLK_DECLARE, and
> make things work again.
>
> http://code.bulix.org/5goa5j-88345?raw
>
> Is this an acceptable solution?
>
> We were already moving to this, I'm not really fond of doing this like
> that, but I guess this whole debacle makes it necessary.
did this lead anywhere meanwhile.
Last I remember the change to orphan handling made sunxi fail, but I'm still
hoping to get this usable at some point :-)
Thanks
Heiko
Hi Heiko,
On Mon, Jul 27, 2015 at 10:57:33AM +0200, Heiko St?bner wrote:
> > > Also we can try to see if critical clocks aren't being forced on by
> > > applying this patch and looking for clk_get() failures
> >
> > And that shows that the CPU and DDR clocks are not protected, which
> > obviously is pretty mad.
> >
> > I've mass converted all our probing code to use OF_CLK_DECLARE, and
> > make things work again.
> >
> > http://code.bulix.org/5goa5j-88345?raw
> >
> > Is this an acceptable solution?
> >
> > We were already moving to this, I'm not really fond of doing this like
> > that, but I guess this whole debacle makes it necessary.
>
>
> did this lead anywhere meanwhile.
>
> Last I remember the change to orphan handling made sunxi fail, but
> I'm still hoping to get this usable at some point :-)
To be honest, I don't know what the current status is. I haven't get
any news since that mail.
I started to move a significant portion of our clocks to
CLK_OF_DECLARE, but not all of them are (which probably make the
situation worse for the time being).
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 07/30, Maxime Ripard wrote:
> On Mon, Jul 27, 2015 at 10:57:33AM +0200, Heiko St?bner wrote:
> >
> > did this lead anywhere meanwhile.
> >
> > Last I remember the change to orphan handling made sunxi fail, but
> > I'm still hoping to get this usable at some point :-)
>
> To be honest, I don't know what the current status is. I haven't get
> any news since that mail.
>
> I started to move a significant portion of our clocks to
> CLK_OF_DECLARE, but not all of them are (which probably make the
> situation worse for the time being).
>
Sorry, I think we're still waiting for the critical clocks stuff
to settle down so that Sunxi can use that instead of calling
clk_prepare_enable() in init code. For now, I've put the first
patch of the two into clk-next so that we can have proper orphan
tracking. Hopefully we resolve critical clocks soon so that we
can merge the defer part.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Am Dienstag, 11. August 2015, 15:34:09 schrieb Stephen Boyd:
> On 07/30, Maxime Ripard wrote:
> > On Mon, Jul 27, 2015 at 10:57:33AM +0200, Heiko St?bner wrote:
> > > did this lead anywhere meanwhile.
> > >
> > > Last I remember the change to orphan handling made sunxi fail, but
> > > I'm still hoping to get this usable at some point :-)
> >
> > To be honest, I don't know what the current status is. I haven't get
> > any news since that mail.
> >
> > I started to move a significant portion of our clocks to
> > CLK_OF_DECLARE, but not all of them are (which probably make the
> > situation worse for the time being).
>
> Sorry, I think we're still waiting for the critical clocks stuff
> to settle down so that Sunxi can use that instead of calling
> clk_prepare_enable() in init code. For now, I've put the first
> patch of the two into clk-next so that we can have proper orphan
> tracking. Hopefully we resolve critical clocks soon so that we
> can merge the defer part.
great :-)
For the defer-part you came up with a better solution than mine anyway, if I
remember correctly.