[PATCH v2] clk: Do not complain about correctly set read-only muxes when assigning clock parents from device tree

[PATCH v2] clk: Do not complain about correctly set read-only muxes when assigning clock parents from device tree

[Philipp Zabel]

Assigning a clock parent to a mux with the CLK_MUX_READ_ONLY flag causes an
error "clk: failed to reparent read_only_mux to already_set_parent: -38"
even if the hardware is already set to the correct parent clock.

This patch avoids the error message by checking whether the correct parent
is already set before calling clk_set_parent. This allows to use the
assigned-clock-parents device tree binding for clock muxes that are not
allowed to be changed anymore at the time of_clk_set_defaults is called.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes since v1:
 - Added Shawn's ack.
 - Moved the (rc < 0) check into the body of the if statement, directly after
   the rc = clk_set_parent(...) call.
---
 drivers/clk/clk-conf.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index aad4796..bb4dc02 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -62,10 +62,13 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 			goto err;
 		}
 
-		rc = clk_set_parent(clk, pclk);
-		if (rc < 0)
-			pr_err("clk: failed to reparent %s to %s: %d\n",
-			       __clk_get_name(clk), __clk_get_name(pclk), rc);
+		if (pclk != __clk_get_parent(clk)) {
+			rc = clk_set_parent(clk, pclk);
+			if (rc < 0)
+				pr_err("clk: failed to reparent %s to %s: %d\n",
+				       __clk_get_name(clk),
+				       __clk_get_name(pclk), rc);
+		}
 		clk_put(clk);
 		clk_put(pclk);
 	}
-- 
2.1.4

Re: [PATCH v2] clk: Do not complain about correctly set read-only muxes when assigning clock parents from device tree

[Stephen Boyd]

On 01/06/2015 07:45 AM, Philipp Zabel wrote:
> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> index aad4796..bb4dc02 100644
> --- a/drivers/clk/clk-conf.c
> +++ b/drivers/clk/clk-conf.c
> @@ -62,10 +62,13 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
>  			goto err;
>  		}
>  
> -		rc = clk_set_parent(clk, pclk);
> -		if (rc < 0)
> -			pr_err("clk: failed to reparent %s to %s: %d\n",
> -			       __clk_get_name(clk), __clk_get_name(pclk), rc);
> +		if (pclk != __clk_get_parent(clk)) {
> +			rc = clk_set_parent(clk, pclk);
> +			if (rc < 0)
> +				pr_err("clk: failed to reparent %s to %s: %d\n",
> +				       __clk_get_name(clk),
> +				       __clk_get_name(pclk), rc);
> +		}
>  		clk_put(clk);
>  		clk_put(pclk);
>  	}

Why not do this in the core? As far as I can tell other drivers could
run into the same problem, no? Does this work?

-----8<-------

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f4963b7d4e17..3278645f4729 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1677,16 +1677,18 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 	if (!clk)
 		return 0;
 
-	/* verify ops for for multi-parent clks */
-	if ((clk->num_parents > 1) && (!clk->ops->set_parent))
-		return -ENOSYS;
-
 	/* prevent racing with updates to the clock topology */
 	clk_prepare_lock();
 
 	if (clk->parent == parent)
 		goto out;
 
+	/* verify ops for for multi-parent clks */
+	if ((clk->num_parents > 1) && (!clk->ops->set_parent)) {
+		ret = -ENOSYS;
+		goto out;
+	}
+
 	/* check that we are allowed to re-parent if the clock is in use */
 	if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) {
 		ret = -EBUSY;

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

Re: [PATCH v2] clk: Do not complain about correctly set read-only muxes when assigning clock parents from device tree

[Philipp Zabel]

Hi Stephen,

Am Donnerstag, den 08.01.2015, 13:32 -0800 schrieb Stephen Boyd:
[...]
> Why not do this in the core? As far as I can tell other drivers could
> run into the same problem, no? Does this work?
> 
> -----8<-------
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f4963b7d4e17..3278645f4729 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1677,16 +1677,18 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>  	if (!clk)
>  		return 0;
>  
> -	/* verify ops for for multi-parent clks */
> -	if ((clk->num_parents > 1) && (!clk->ops->set_parent))
> -		return -ENOSYS;
> -
>  	/* prevent racing with updates to the clock topology */
>  	clk_prepare_lock();
>  
>  	if (clk->parent == parent)
>  		goto out;
>  
> +	/* verify ops for for multi-parent clks */
> +	if ((clk->num_parents > 1) && (!clk->ops->set_parent)) {
> +		ret = -ENOSYS;
> +		goto out;
> +	}
> +
>  	/* check that we are allowed to re-parent if the clock is in use */
>  	if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) {
>  		ret = -EBUSY;

"[PATCH] clk: make set_parent succeed for any clock if the parent to be
set is the same as the current parent" ?

It works, but it also changes the API, as it makes
   clk_set_parent(some_non_mux, its_current_parent)
succeed instead of return -ENOSYS.

regards
Philipp

Re: [PATCH v2] clk: Do not complain about correctly set read-only muxes when assigning clock parents from device tree

[Stephen Boyd]

On 01/09/2015 06:50 AM, Philipp Zabel wrote:
> Hi Stephen,
>
> Am Donnerstag, den 08.01.2015, 13:32 -0800 schrieb Stephen Boyd:
> [...]
>> Why not do this in the core? As far as I can tell other drivers could
>> run into the same problem, no? Does this work?
>>
>> -----8<-------
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index f4963b7d4e17..3278645f4729 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1677,16 +1677,18 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>>  	if (!clk)
>>  		return 0;
>>  
>> -	/* verify ops for for multi-parent clks */
>> -	if ((clk->num_parents > 1) && (!clk->ops->set_parent))
>> -		return -ENOSYS;
>> -
>>  	/* prevent racing with updates to the clock topology */
>>  	clk_prepare_lock();
>>  
>>  	if (clk->parent == parent)
>>  		goto out;
>>  
>> +	/* verify ops for for multi-parent clks */
>> +	if ((clk->num_parents > 1) && (!clk->ops->set_parent)) {
>> +		ret = -ENOSYS;
>> +		goto out;
>> +	}
>> +
>>  	/* check that we are allowed to re-parent if the clock is in use */
>>  	if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) {
>>  		ret = -EBUSY;
> "[PATCH] clk: make set_parent succeed for any clock if the parent to be
> set is the same as the current parent" ?
>
> It works, but it also changes the API, as it makes
>    clk_set_parent(some_non_mux, its_current_parent)
> succeed instead of return -ENOSYS.

I would think a non_mux clk would have clk->num_parents == 1, so I don't
see how it would return -ENOSYS here. What you mention should succeed
today. Otherwise we have a bug.

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

Re: [PATCH v2] clk: Do not complain about correctly set read-only muxes when assigning clock parents from device tree

[Philipp Zabel]

Am Freitag, den 09.01.2015, 10:51 -0800 schrieb Stephen Boyd:
> On 01/09/2015 06:50 AM, Philipp Zabel wrote:
> > Hi Stephen,
> >
> > Am Donnerstag, den 08.01.2015, 13:32 -0800 schrieb Stephen Boyd:
> > [...]
> >> Why not do this in the core? As far as I can tell other drivers could
> >> run into the same problem, no? Does this work?
> >>
> >> -----8<-------
> >>
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> index f4963b7d4e17..3278645f4729 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -1677,16 +1677,18 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
> >>  	if (!clk)
> >>  		return 0;
> >>  
> >> -	/* verify ops for for multi-parent clks */
> >> -	if ((clk->num_parents > 1) && (!clk->ops->set_parent))
> >> -		return -ENOSYS;
> >> -
> >>  	/* prevent racing with updates to the clock topology */
> >>  	clk_prepare_lock();
> >>  
> >>  	if (clk->parent == parent)
> >>  		goto out;
> >>  
> >> +	/* verify ops for for multi-parent clks */
> >> +	if ((clk->num_parents > 1) && (!clk->ops->set_parent)) {
> >> +		ret = -ENOSYS;
> >> +		goto out;
> >> +	}
> >> +
> >>  	/* check that we are allowed to re-parent if the clock is in use */
> >>  	if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) {
> >>  		ret = -EBUSY;
> > "[PATCH] clk: make set_parent succeed for any clock if the parent to be
> > set is the same as the current parent" ?
> >
> > It works, but it also changes the API, as it makes
> >    clk_set_parent(some_non_mux, its_current_parent)
> > succeed instead of return -ENOSYS.
> 
> I would think a non_mux clk would have clk->num_parents == 1, so I don't
> see how it would return -ENOSYS here. What you mention should succeed
> today. Otherwise we have a bug.

Right. In that case, I'm all for it.

regards
Philipp

[PATCH v2] clk: Do not complain about correctly set read-only muxes when assigning clock parents from device tree

[Philipp Zabel]

Assigning a clock parent to a mux with the CLK_MUX_READ_ONLY flag causes an
error "clk: failed to reparent read_only_mux to already_set_parent: -38"
even if the hardware is already set to the correct parent clock.

This patch avoids the error message by checking whether the correct parent
is already set before calling clk_set_parent. This allows to use the
assigned-clock-parents device tree binding for clock muxes that are not
allowed to be changed anymore at the time of_clk_set_defaults is called.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes since v1:
 - Move the (rc < 0) check into the body of if (pclk != __clk_get_parent(clk)),
   as suggested by Uwe Kleine-König. No functional change.
---
 drivers/clk/clk-conf.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index aad4796..bb4dc02 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -62,10 +62,13 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 			goto err;
 		}
 
-		rc = clk_set_parent(clk, pclk);
-		if (rc < 0)
-			pr_err("clk: failed to reparent %s to %s: %d\n",
-			       __clk_get_name(clk), __clk_get_name(pclk), rc);
+		if (pclk != __clk_get_parent(clk)) {
+			rc = clk_set_parent(clk, pclk);
+			if (rc < 0)
+				pr_err("clk: failed to reparent %s to %s: %d\n",
+				       __clk_get_name(clk),
+				       __clk_get_name(pclk), rc);
+		}
 		clk_put(clk);
 		clk_put(pclk);
 	}
-- 
2.1.3