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

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

[Heiko Stübner]

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 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 orphan 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.


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

Heiko Stuebner (2):
  clk: track the orphan status of clocks and their children
  clk: prevent orphan clocks from being used

 drivers/clk/clk.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 3 deletions(-)

-- 
2.1.4

[PATCH v2 1/2] clk: track the orphan status of clocks and their children

[Heiko Stübner]

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 <sboyd@codeaurora.org>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 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 f85c8e2..a9fa5ab 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;
@@ -1433,18 +1434,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_update_orphan_status(struct clk_core *clk, int is_orphan)
+{
+	struct clk_core *child;
+
+	clk->orphan = is_orphan;
+
+	hlist_for_each_entry(child, &clk->children, child_node)
+		clk_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_update_orphan_status(clk, becomes_orphan);
 	} else {
 		hlist_add_head(&clk->child_node, &clk_orphan_list);
+		if (!was_orphan)
+			clk_update_orphan_status(clk, 1);
 	}
 
 	clk->parent = new_parent;
@@ -2348,13 +2371,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 = 0;
+	} else {
 		hlist_add_head(&clk->child_node, &clk_orphan_list);
+		clk->orphan = 1;
+	}
 
 	/*
 	 * Set clk's accuracy.  The preferred method is to use
-- 
2.1.4

[PATCH v2 2/2] clk: prevent orphan clocks from being used

[Heiko Stübner]

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 <heiko@sntech.de>
---
 drivers/clk/clk.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a9fa5ab..b977701 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2272,6 +2272,17 @@ 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;
+
+	if (!clk->core)
+		return false;
+
+	return clk->core->orphan;
+}
+
 /**
  * __clk_init - initialize the data structures in a struct clk
  * @dev:	device initializing this clk, placeholder for now
@@ -3009,6 +3020,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

Re: [PATCH v2 1/2] clk: track the orphan status of clocks and their children

[Stephen Boyd]

On 04/12/15 04:37, Heiko Stübner wrote:
> 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 <sboyd@codeaurora.org>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Just nitpicks. Otherwise it looks good to me.

> ---
>  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 f85c8e2..a9fa5ab 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;
> @@ -1433,18 +1434,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_update_orphan_status(struct clk_core *clk, int is_orphan)

Please s/clk/core/ here because it's a new function (yeah we've been
slow and haven't updated the whole file to do this consistently). Also
use bool for is_orphan.

> +{
> +	struct clk_core *child;
> +
> +	clk->orphan = is_orphan;
> +
> +	hlist_for_each_entry(child, &clk->children, child_node)
> +		clk_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_update_orphan_status(clk, becomes_orphan);
>  	} else {
>  		hlist_add_head(&clk->child_node, &clk_orphan_list);
> +		if (!was_orphan)
> +			clk_update_orphan_status(clk, 1);

Please use true here.

>  	}
>  
>  	clk->parent = new_parent;
> @@ -2348,13 +2371,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 = 0;
> +	} else {
>  		hlist_add_head(&clk->child_node, &clk_orphan_list);
> +		clk->orphan = 1;

Please use true/false here.

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

Re: [PATCH v2 2/2] clk: prevent orphan clocks from being used

[Stephen Boyd]

On 04/12/15 04:38, Heiko Stübner 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 <heiko@sntech.de>
> ---
>  drivers/clk/clk.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a9fa5ab..b977701 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2272,6 +2272,17 @@ 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;
> +
> +	if (!clk->core)
> +		return false;

Looks like a useless check. clk->core better be there.

> +
> +	return clk->core->orphan;
> +}
> +
>  /**
>   * __clk_init - initialize the data structures in a struct clk
>   * @dev:	device initializing this clk, placeholder for now
> @@ -3009,6 +3020,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);
>  

Otherwise looks good. We really need to test it on lots of platforms though.

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