[PATCH 0/4] clk: improve handling of orphan clocks

[PATCH 0/4] clk: improve handling of orphan clocks

[Heiko Stuebner]

This resurrects a patch from Doug Anderson from november that fixes the
enable counts when previous orphan-clocks get reparented to a newly
probed parent clock.

In the ensuing discussion Russell rightfully pointed out that orphan
clocks should probably not be used at all, as things like dividers
won't be able to set meaningful / safe values at all. But Doug's reply
also rightfully mentioned the fact that we most likely cannot change
the behaviour for all orphans as maybe some arches depend on the
current behaviour and it's not really possible to find all possible
affected clocks.

So this series is two-part. Patch1 is the fix from Doug fixing the
actual enable mismatch when an enabled orphan gets reparented to
a real parent and patches 2-4 try to provide a way to let the clock
core defer clk_gets on orphan clocks.
If at some point the majority of clock drivers make use of deferrals
this could then become the default behaviour.


Doug Anderson (1):
  clk: Propagate prepare and enable when reparenting orphans

Heiko Stuebner (3):
  clk: add clk_is_orphan() to check if a clocks inherits from an orphan
    clock
  clk: add CLK_DEFER_ORPHAN flag to prevent orphans from being used
  clk: rockchip: enable CLK_DEFER_ORPHAN for all branches

 drivers/clk/clk.c            | 69 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/clk/rockchip/clk.c   |  3 ++
 include/linux/clk-provider.h |  1 +
 3 files changed, 71 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH 1/4] clk: Propagate prepare and enable when reparenting orphans

[Heiko Stuebner]

From: Doug Anderson <dianders@chromium.org>

With the existing code, if you find a parent for an orhpan that has
already been prepared / enabled, you won't enable the parent.  That
can cause later problems since the clock tree isn't in a consistent
state.  Fix by propagating the prepare and enable.

NOTE: this does bring up the question about whether the enable of the
orphan actually made sense.  If the orphan's parent wasn't enabled by
default (by the bootloader or the default state of the hardware) then
the original enable of the orphan probably didn't do what the caller
though it would.  Some users of the orphan might have preferred an
EPROBE_DEFER be returned until we had a full path to a root clock.
This patch doesn't address those concerns and really just syncs up the
state.

Tested on rk3288-evb-rk808 by temporarily considering "sclk_tsadc" as
a critical clock (to simulate a driver enabling it at bootup).

Before:

   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase
----------------------------------------------------------------------------------------
 xin32k                                   0            0       32768          0 0
    sclk_hdmi_cec                         0            0       32768          0 0
    sclk_otg_adp                          0            0       32768          0 0
    sclk_tsadc                            1            1         993          0 0

After:

   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase
----------------------------------------------------------------------------------------
 xin32k                                   1            1       32768          0 0
    sclk_hdmi_cec                         0            0       32768          0 0
    sclk_otg_adp                          0            0       32768          0 0
    sclk_tsadc                            1            1         993          0 0

Note that xin32k on rk808 is a clock that cannot be disabled in
hardware (it's an always on clock), so really all we needed to do was
to sync up the state.

Signed-off-by: Doug Anderson <dianders@chromium.org>

Adapted to recent clk->clk_core changes and move orphan handling to
a separate function as the main clk_core_reparent function is already
also used in other contexts.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f85c8e2..512323f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2011,6 +2011,27 @@ static void clk_core_reparent(struct clk_core *clk,
 	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 }
 
+static void clk_core_reparent_orphan(struct clk_core *clk,
+				     struct clk_core *new_parent)
+{
+	clk_core_reparent(clk, new_parent);
+
+	if (clk->prepare_count) {
+		unsigned long flags;
+
+		if (clk_core_prepare(new_parent)) {
+			pr_err("%s: could not prepare new parent %s\n",
+			       __func__, clk->name);
+			return;
+		}
+
+		flags = clk_enable_lock();
+		if (clk->enable_count)
+			clk_core_enable(new_parent);
+		clk_enable_unlock(flags);
+	}
+}
+
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
 {
 	if (!hw)
@@ -2404,13 +2425,13 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
 		if (orphan->num_parents && orphan->ops->get_parent) {
 			i = orphan->ops->get_parent(orphan->hw);
 			if (!strcmp(clk->name, orphan->parent_names[i]))
-				clk_core_reparent(orphan, clk);
+				clk_core_reparent_orphan(orphan, clk);
 			continue;
 		}
 
 		for (i = 0; i < orphan->num_parents; i++)
 			if (!strcmp(clk->name, orphan->parent_names[i])) {
-				clk_core_reparent(orphan, clk);
+				clk_core_reparent_orphan(orphan, clk);
 				break;
 			}
 	 }
-- 
2.1.4

Re: [PATCH 1/4] clk: Propagate prepare and enable when reparenting orphans

[Stephen Boyd]

On 04/02/15 08:34, Heiko Stuebner wrote:
> From: Doug Anderson <dianders@chromium.org>
>
> With the existing code, if you find a parent for an orhpan that has
> already been prepared / enabled, you won't enable the parent.  That
> can cause later problems since the clock tree isn't in a consistent
> state.  Fix by propagating the prepare and enable.
>
> NOTE: this does bring up the question about whether the enable of the
> orphan actually made sense.  If the orphan's parent wasn't enabled by
> default (by the bootloader or the default state of the hardware) then
> the original enable of the orphan probably didn't do what the caller
> though it would.  Some users of the orphan might have preferred an
> EPROBE_DEFER be returned until we had a full path to a root clock.
> This patch doesn't address those concerns and really just syncs up the
> state.
>
> Tested on rk3288-evb-rk808 by temporarily considering "sclk_tsadc" as
> a critical clock (to simulate a driver enabling it at bootup).
>
> Before:
>
>    clock                         enable_cnt  prepare_cnt        rate   accuracy   phase
> ----------------------------------------------------------------------------------------
>  xin32k                                   0            0       32768          0 0
>     sclk_hdmi_cec                         0            0       32768          0 0
>     sclk_otg_adp                          0            0       32768          0 0
>     sclk_tsadc                            1            1         993          0 0
>
> After:
>
>    clock                         enable_cnt  prepare_cnt        rate   accuracy   phase
> ----------------------------------------------------------------------------------------
>  xin32k                                   1            1       32768          0 0
>     sclk_hdmi_cec                         0            0       32768          0 0
>     sclk_otg_adp                          0            0       32768          0 0
>     sclk_tsadc                            1            1         993          0 0
>
> Note that xin32k on rk808 is a clock that cannot be disabled in
> hardware (it's an always on clock), so really all we needed to do was
> to sync up the state.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
>
> Adapted to recent clk->clk_core changes and move orphan handling to
> a separate function as the main clk_core_reparent function is already
> also used in other contexts.
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---

Sorry I don't get this patch at all. We shouldn't be handing out clocks
to consumers if the clock is an orphan. We also have a problem syncing
enable state in the framework with what is there in hardware when we
boot up. These aren't the same problem but they're related. Let's not
conflate the two.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/4] clk: add clk_is_orphan() to check if a clocks inherits from an orphan clock

[Heiko Stuebner]

There are cases where it is helpful to know if the full clock path can be
trusted or if there is a parent clock missing somewhere in the parent-path.

We keep it confined to the ccf area for now, if later users outside the ccf
surface it can be made more publically available.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 512323f..476f491 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2271,6 +2271,44 @@ bool clk_is_match(const struct clk *p, const struct clk *q)
 EXPORT_SYMBOL_GPL(clk_is_match);
 
 /**
+ * __clk_is_orphan - check if a clock or its parent is an orphan
+ *
+ * Walks up the clock parents until it reaches a root-clock or
+ * a clock contained in the orphan list. Returns true if there is
+ * an orphan in the parent-path otherwise false.
+ */
+static bool __clk_is_orphan(struct clk_core *clk)
+{
+	struct clk_core *orphan;
+
+	if (clk->flags & CLK_IS_ROOT)
+		return false;
+
+	hlist_for_each_entry(orphan, &clk_orphan_list, child_node) {
+		if (clk == orphan)
+			return true;
+	}
+
+	if (clk->num_parents)
+		return __clk_is_orphan(clk->parent);
+
+	return false;
+}
+
+static bool clk_is_orphan(struct clk *clk)
+{
+	bool ret;
+
+	if (!clk)
+		return false;
+
+	clk_prepare_lock();
+	ret = __clk_is_orphan(clk->core);
+	clk_prepare_unlock();
+	return ret;
+}
+
+/**
  * __clk_init - initialize the data structures in a struct clk
  * @dev:	device initializing this clk, placeholder for now
  * @clk:	clk being initialized
-- 
2.1.4

Re: [PATCH 2/4] clk: add clk_is_orphan() to check if a clocks inherits from an orphan clock

[Stephen Boyd]

On 04/02/15 08:34, Heiko Stuebner wrote:
> There are cases where it is helpful to know if the full clock path can be
> trusted or if there is a parent clock missing somewhere in the parent-path.
>
> We keep it confined to the ccf area for now, if later users outside the ccf
> surface it can be made more publically available.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/clk/clk.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 512323f..476f491 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2271,6 +2271,44 @@ bool clk_is_match(const struct clk *p, const struct clk *q)
>  EXPORT_SYMBOL_GPL(clk_is_match);
>  
>  /**
> + * __clk_is_orphan - check if a clock or its parent is an orphan

@clk?

> + *
> + * Walks up the clock parents until it reaches a root-clock or
> + * a clock contained in the orphan list. Returns true if there is
> + * an orphan in the parent-path otherwise false.
> + */
> +static bool __clk_is_orphan(struct clk_core *clk)
> +{
> +	struct clk_core *orphan;
> +
> +	if (clk->flags & CLK_IS_ROOT)
> +		return false;
> +
> +	hlist_for_each_entry(orphan, &clk_orphan_list, child_node) {
> +		if (clk == orphan)
> +			return true;
> +	}

Nitpick: Please drop braces

> +
> +	if (clk->num_parents)
> +		return __clk_is_orphan(clk->parent);

This looks inefficient. Hopefully we're not calling this all the time?
Except we call it from clk_get().

Maybe it would make sense to mark the clock as orphaned or not. When we
regsiter the clock we can see if the parent is there and if so, check
the parent's flag to see if it's orphaned or not. If it isn't orphaned
everything is good, cheap lookup to figure it out and we don't set the
flag. If it is orphaned, again cheap lookup and we set the flag. When we
adopt the clock (deorphan?) we would have to go through the clock and
all it's children to clear the flag. The cost is paid once. Then this
function becomes a cheap flag check that we can do whenever we want.

> +
> +	return false;
> +}
> +
> +static bool clk_is_orphan(struct clk *clk)
> +{
> +	bool ret;
> +
> +	if (!clk)
> +		return false;
> +
> +	clk_prepare_lock();
> +	ret = __clk_is_orphan(clk->core);
> +	clk_prepare_unlock();

Unfortunately we still have to take some sort of lock here. If we had a
new lock, orphan_lock or something, then we could lock the adoption
phase and the flag setting of all clocks under that lock. When we go to
per-clock locks we could change that to use the lock for a particular clock.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/4] clk: add CLK_DEFER_ORPHAN flag to prevent orphans from being used

[Heiko Stuebner]

The usage of clocks derived from an orphan can produce issues when trying
to set rates etc. So ideally a clk_get to such a clock should defer till
the clock hierarchy is complete.
But as some arches probably rely on such clocks we can't disable them all.
Therefore add a new clk flag where arches can enable this behaviour for
their clocks.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/clk.c            | 6 ++++++
 include/linux/clk-provider.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 476f491..8511c62 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3041,6 +3041,12 @@ 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_get_flags(clk) & CLK_DEFER_ORPHAN)
+						&& clk_is_orphan(clk)) {
+				clk = ERR_PTR(-EPROBE_DEFER);
+				break;
+			}
+
 			clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
 					       con_id);
 
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 28abf1b..ef8d669 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -31,6 +31,7 @@
 #define CLK_GET_RATE_NOCACHE	BIT(6) /* do not use the cached clk rate */
 #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
 #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
+#define CLK_DEFER_ORPHAN	BIT(9) /* defer clk_get calls for orphans */
 
 struct clk_hw;
 struct clk_core;
-- 
2.1.4

Re: [PATCH 3/4] clk: add CLK_DEFER_ORPHAN flag to prevent orphans from being used

[Stephen Boyd]

On 04/02/15 08:34, Heiko Stuebner wrote:
> The usage of clocks derived from an orphan can produce issues when trying
> to set rates etc. So ideally a clk_get to such a clock should defer till
> the clock hierarchy is complete.
> But as some arches probably rely on such clocks we can't disable them all.
> Therefore add a new clk flag where arches can enable this behaviour for
> their clocks.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/clk/clk.c            | 6 ++++++
>  include/linux/clk-provider.h | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 476f491..8511c62 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3041,6 +3041,12 @@ 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_get_flags(clk) & CLK_DEFER_ORPHAN)
> +						&& clk_is_orphan(clk)) {
> +				clk = ERR_PTR(-EPROBE_DEFER);
> +				break;
> +			}
> +
>  			clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
>  					       con_id);
>  
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 28abf1b..ef8d669 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -31,6 +31,7 @@
>  #define CLK_GET_RATE_NOCACHE	BIT(6) /* do not use the cached clk rate */
>  #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
>  #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
> +#define CLK_DEFER_ORPHAN	BIT(9) /* defer clk_get calls for orphans */
>  

I don't see why this is an opt-in feature. If we think there are arches
that are setting rates of clocks before they're ready then we have a
problem and we should fix it. These last two patches don't look necessary.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] clk: add CLK_DEFER_ORPHAN flag to prevent orphans from being used

[Heiko Stübner]

Am Freitag, 10. April 2015, 17:52:43 schrieb Stephen Boyd:
> On 04/02/15 08:34, Heiko Stuebner wrote:
> > The usage of clocks derived from an orphan can produce issues when trying
> > to set rates etc. So ideally a clk_get to such a clock should defer till
> > the clock hierarchy is complete.
> > But as some arches probably rely on such clocks we can't disable them all.
> > Therefore add a new clk flag where arches can enable this behaviour for
> > their clocks.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/clk/clk.c            | 6 ++++++
> >  include/linux/clk-provider.h | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 476f491..8511c62 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3041,6 +3041,12 @@ 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_get_flags(clk) & CLK_DEFER_ORPHAN)
> > +						&& clk_is_orphan(clk)) {
> > +				clk = ERR_PTR(-EPROBE_DEFER);
> > +				break;
> > +			}
> > +
> > 
> >  			clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
> >  			
> >  					       con_id);
> > 
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 28abf1b..ef8d669 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -31,6 +31,7 @@
> > 
> >  #define CLK_GET_RATE_NOCACHE	BIT(6) /* do not use the cached clk rate */
> >  #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change
> >  */ #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk
> >  accuracy */> 
> > +#define CLK_DEFER_ORPHAN	BIT(9) /* defer clk_get calls for orphans */
> 
> I don't see why this is an opt-in feature. If we think there are arches
> that are setting rates of clocks before they're ready then we have a
> problem and we should fix it. These last two patches don't look necessary.

yep this is some sort of policy-decision ... I'm perfectly fine with changing 
the default behaviour from my Rockchip-pov but was reluctant to do this for 
everybody.

For example in the cited example in patch1, the clock from an i2c-device 
supplies a soc-clock and the soc-clock only gets enabled/disabled at this 
point. The i2c-device is default on, so everything works till it falls over 
when going to suspend. So I guess similar corner cases might exist on other 
platforms.

It might very well be better to change the behaviour and hope for little 
fallout to fix.

So, it looks like your on favour of preventing the usage of orphans 
altogether, so I'll try to come up with a better implementation taking into 
account your comments to patch2.


Thanks
Heiko

[PATCH 4/4] clk: rockchip: enable CLK_DEFER_ORPHAN for all branches

[Heiko Stuebner]

All Rockchip drivers should handle deferentials correctly and on all boards
we have the situation that some clock sources are generated off-soc by an
external i2c component (like the xin32k feeding for example the sclk_tsadc
on rk3288). So some clocks are always orphans until the i2c drivers are
probed.
So add the new CLK_DEFER_ORPHAN flag for all our clock branches to generate
correct deferals for this timespan.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 20e05bb..5bb5fa6 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -220,6 +220,9 @@ void __init rockchip_clk_register_branches(
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		flags = list->flags;
 
+		/* defer clk_get on orphan clocks */
+		flags |= CLK_DEFER_ORPHAN;
+
 		/* catch simple muxes */
 		switch (list->branch_type) {
 		case branch_mux:
-- 
2.1.4