[PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Jon Medhurst (Tixy)]

The check for correct frequency being set in bL_cpufreq_set_rate is
broken when the big.LITTLE switcher is active, for two reasons.

1. The 'new_rate' variable gets overwritten before the test by the
code calculating the frequency of the old cluster.

2. The frequency returned by bL_cpufreq_get_rate will be the virtual
frequency, not the actual one the intended version of new_rate contains.

This means the function always returns an error causing an endless
stream of: "cpufreq: __target_index: Failed to change cpu frequency: -5"

As the comment for the test indicates that it is temporary and limited
lets not bother fixing it to work when the bL switcher is active and
instead just bypass the test in that case.

Fixes: 0a95e630b49a ("cpufreq: arm_big_little: check if the frequency is set correctly")

Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
 drivers/cpufreq/arm_big_little.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index f1e42f8..71deddb 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -196,7 +196,7 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
 	 * be reading only the cached value anyway. This needs to  be removed
 	 * once clk core is fixed.
 	 */
-	if (bL_cpufreq_get_rate(cpu) != new_rate)
+	if (!bLs && bL_cpufreq_get_rate(cpu) != new_rate)
 		return -EIO;
 	return 0;
 }
-- 
2.1.4

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Viresh Kumar]

On 02-10-15, 18:38, Jon Medhurst (Tixy) wrote:
> The check for correct frequency being set in bL_cpufreq_set_rate is
> broken when the big.LITTLE switcher is active, for two reasons.
> 
> 1. The 'new_rate' variable gets overwritten before the test by the
> code calculating the frequency of the old cluster.
> 
> 2. The frequency returned by bL_cpufreq_get_rate will be the virtual
> frequency, not the actual one the intended version of new_rate contains.
> 
> This means the function always returns an error causing an endless
> stream of: "cpufreq: __target_index: Failed to change cpu frequency: -5"
> 
> As the comment for the test indicates that it is temporary and limited
> lets not bother fixing it to work when the bL switcher is active and
> instead just bypass the test in that case.
> 
> Fixes: 0a95e630b49a ("cpufreq: arm_big_little: check if the frequency is set correctly")
> 
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> ---
>  drivers/cpufreq/arm_big_little.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

And why not fix it properly by doing this:

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index f1e42f8ce0fc..5b36657a76d6 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -128,7 +128,7 @@ static unsigned int bL_cpufreq_get_rate(unsigned int cpu)
 static unsigned int
 bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
 {
-	u32 new_rate, prev_rate;
+	u32 new_rate, prev_rate, target_rate;
 	int ret;
 	bool bLs = is_bL_switching_enabled();
 
@@ -140,9 +140,11 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
 		per_cpu(physical_cluster, cpu) = new_cluster;
 
 		new_rate = find_cluster_maxfreq(new_cluster);
+		target_rate = new_rate;
 		new_rate = ACTUAL_FREQ(new_cluster, new_rate);
 	} else {
 		new_rate = rate;
+		target_rate = new_rate;
 	}
 
 	pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d, freq: %d\n",
@@ -196,7 +198,7 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
 	 * be reading only the cached value anyway. This needs to  be removed
 	 * once clk core is fixed.
 	 */
-	if (bL_cpufreq_get_rate(cpu) != new_rate)
+	if (bL_cpufreq_get_rate(cpu) != target_rate)
 		return -EIO;
 	return 0;
 }

-- 
viresh

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Jon Medhurst (Tixy)]

On Wed, 2015-10-07 at 23:09 +0530, Viresh Kumar wrote:
[...]
> And why not fix it properly by doing this:
> 
> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> index f1e42f8ce0fc..5b36657a76d6 100644
> --- a/drivers/cpufreq/arm_big_little.c
> +++ b/drivers/cpufreq/arm_big_little.c
> @@ -128,7 +128,7 @@ static unsigned int bL_cpufreq_get_rate(unsigned int cpu)
>  static unsigned int
>  bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>  {
> -	u32 new_rate, prev_rate;
> +	u32 new_rate, prev_rate, target_rate;
>  	int ret;
>  	bool bLs = is_bL_switching_enabled();
>  
> @@ -140,9 +140,11 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>  		per_cpu(physical_cluster, cpu) = new_cluster;
>  
>  		new_rate = find_cluster_maxfreq(new_cluster);
> +		target_rate = new_rate;
>  		new_rate = ACTUAL_FREQ(new_cluster, new_rate);
>  	} else {
>  		new_rate = rate;
> +		target_rate = new_rate;
>  	}
>  
>  	pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d, freq: %d\n",
> @@ -196,7 +198,7 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>  	 * be reading only the cached value anyway. This needs to  be removed
>  	 * once clk core is fixed.
>  	 */
> -	if (bL_cpufreq_get_rate(cpu) != new_rate)
> +	if (bL_cpufreq_get_rate(cpu) != target_rate)
>  		return -EIO;
>  	return 0;
>  }

You call that a proper fix? ;-) It's comparing an 'virtual' frequency to
an 'actual' frequency.

If the real intent is to check that clk_set_rate works I would have
thought the patch below would be correct. But I didn't propose that as
it's the obvious implementation and I assumed the original patch didn't
do it that way for a reason.

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index f1e42f8..59115a4 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -149,6 +149,18 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
 			__func__, cpu, old_cluster, new_cluster, new_rate);
 
 	ret = clk_set_rate(clk[new_cluster], new_rate * 1000);
+	if (!ret) {
+		/*
+		 * FIXME: clk_set_rate has to handle the case where clk_change_rate
+		 * can fail due to hardware or firmware issues. Until the clk core
+		 * layer is fixed, we can check here. In most of the cases we will
+		 * be reading only the cached value anyway. This needs to  be removed
+		 * once clk core is fixed.
+		 */
+		if (clk_get_rate(clk[new_cluster]) != new_rate * 1000)
+			ret = -EIO;
+	}
+
 	if (WARN_ON(ret)) {
 		pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret,
 				new_cluster);
@@ -189,15 +201,6 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
 		mutex_unlock(&cluster_lock[old_cluster]);
 	}
 
-	/*
-	 * FIXME: clk_set_rate has to handle the case where clk_change_rate
-	 * can fail due to hardware or firmware issues. Until the clk core
-	 * layer is fixed, we can check here. In most of the cases we will
-	 * be reading only the cached value anyway. This needs to  be removed
-	 * once clk core is fixed.
-	 */
-	if (bL_cpufreq_get_rate(cpu) != new_rate)
-		return -EIO;
 	return 0;
 }
 

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Viresh Kumar]

On 08-10-15, 10:23, Jon Medhurst (Tixy) wrote:
> On Wed, 2015-10-07 at 23:09 +0530, Viresh Kumar wrote:
> [...]
> > And why not fix it properly by doing this:
> > 
> > diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> > index f1e42f8ce0fc..5b36657a76d6 100644
> > --- a/drivers/cpufreq/arm_big_little.c
> > +++ b/drivers/cpufreq/arm_big_little.c
> > @@ -128,7 +128,7 @@ static unsigned int bL_cpufreq_get_rate(unsigned int cpu)
> >  static unsigned int
> >  bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> >  {
> > -	u32 new_rate, prev_rate;
> > +	u32 new_rate, prev_rate, target_rate;
> >  	int ret;
> >  	bool bLs = is_bL_switching_enabled();
> >  
> > @@ -140,9 +140,11 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> >  		per_cpu(physical_cluster, cpu) = new_cluster;
> >  
> >  		new_rate = find_cluster_maxfreq(new_cluster);
> > +		target_rate = new_rate;

This is still a virtual freq ...

> >  		new_rate = ACTUAL_FREQ(new_cluster, new_rate);

And new_rate is the actual freq..

> >  	} else {
> >  		new_rate = rate;
> > +		target_rate = new_rate;

Here both are actual freqs, and no virtual freq.

> >  	}
> >  
> >  	pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d, freq: %d\n",
> > @@ -196,7 +198,7 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> >  	 * be reading only the cached value anyway. This needs to  be removed
> >  	 * once clk core is fixed.
> >  	 */
> > -	if (bL_cpufreq_get_rate(cpu) != new_rate)
> > +	if (bL_cpufreq_get_rate(cpu) != target_rate)
> >  		return -EIO;
> >  	return 0;
> >  }
> 
> You call that a proper fix? ;-) It's comparing an 'virtual' frequency to
> an 'actual' frequency.

So, why do you say so? I thought both are virtual freqs only.

> If the real intent is to check that clk_set_rate works I would have
> thought the patch below would be correct. But I didn't propose that as
> it's the obvious implementation and I assumed the original patch didn't
> do it that way for a reason.

Maybe yes. Only Sudeep can tell why he didn't do it that way. But
yeah, the intent was only what the comment says.

-- 
viresh

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Jon Medhurst (Tixy)]

On Thu, 2015-10-08 at 16:54 +0530, Viresh Kumar wrote:
> On 08-10-15, 10:23, Jon Medhurst (Tixy) wrote:
> > On Wed, 2015-10-07 at 23:09 +0530, Viresh Kumar wrote:
[...]
> > > @@ -140,9 +140,11 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> > >  		per_cpu(physical_cluster, cpu) = new_cluster;
> > >  
> > >  		new_rate = find_cluster_maxfreq(new_cluster);
> > > +		target_rate = new_rate;
> 
> This is still a virtual freq ...
> 
> > >  		new_rate = ACTUAL_FREQ(new_cluster, new_rate);
> 
> And new_rate is the actual freq..
> 
> > >  	} else {
> > >  		new_rate = rate;
> > > +		target_rate = new_rate;
> 
> Here both are actual freqs, and no virtual freq.
> 
> > >  	}
> > >  
> > >  	pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d, freq: %d\n",
> > > @@ -196,7 +198,7 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> > >  	 * be reading only the cached value anyway. This needs to  be removed
> > >  	 * once clk core is fixed.
> > >  	 */
> > > -	if (bL_cpufreq_get_rate(cpu) != new_rate)
> > > +	if (bL_cpufreq_get_rate(cpu) != target_rate)
> > >  		return -EIO;
> > >  	return 0;
> > >  }
> > 
> > You call that a proper fix? ;-) It's comparing an 'virtual' frequency to
> > an 'actual' frequency.
> 
> So, why do you say so? I thought both are virtual freqs only.

You are right, I had misread the code. I guess my problem is that I'm
actually running the code then when it doesn't work (which it doesn't)
going back to try and work out why not.

Looking a bit more carefully, the reason your fix doesn't work is that
bL_cpufreq_get_rate returns the last requested rate for this CPU,
whereas target_rate/new_rate is the maximum rate requested by any CPU on
the cluster (which is what we want the hardware set to).
> 
> > If the real intent is to check that clk_set_rate works I would have
> > thought the patch below would be correct. But I didn't propose that as
> > it's the obvious implementation and I assumed the original patch didn't
> > do it that way for a reason.
> 
> Maybe yes. Only Sudeep can tell why he didn't do it that way. But
> yeah, the intent was only what the comment says.

So sounds like my alternative fix of checking the 'actual' frequency
immediately after setting it is probably the way forward, unless Sudeep
chimes in with additional info about the issue he was trying to address.

-- 
Tixy

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Viresh Kumar]

On 08-10-15, 13:55, Jon Medhurst (Tixy) wrote:
> Looking a bit more carefully, the reason your fix doesn't work is that
> bL_cpufreq_get_rate returns the last requested rate for this CPU,
> whereas target_rate/new_rate is the maximum rate requested by any CPU on
> the cluster (which is what we want the hardware set to).

Sigh..

> So sounds like my alternative fix of checking the 'actual' frequency
> immediately after setting it is probably the way forward, unless Sudeep
> chimes in with additional info about the issue he was trying to address.

Right.

-- 
viresh

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Sudeep Holla]

On 08/10/15 13:55, Jon Medhurst (Tixy) wrote:
> On Thu, 2015-10-08 at 16:54 +0530, Viresh Kumar wrote:
>> On 08-10-15, 10:23, Jon Medhurst (Tixy) wrote:
>>> On Wed, 2015-10-07 at 23:09 +0530, Viresh Kumar wrote:

[...]

>
> You are right, I had misread the code. I guess my problem is that I'm
> actually running the code then when it doesn't work (which it doesn't)
> going back to try and work out why not.
>
> Looking a bit more carefully, the reason your fix doesn't work is that
> bL_cpufreq_get_rate returns the last requested rate for this CPU,
> whereas target_rate/new_rate is the maximum rate requested by any CPU on
> the cluster (which is what we want the hardware set to).
>>
>>> If the real intent is to check that clk_set_rate works I would have
>>> thought the patch below would be correct. But I didn't propose that as
>>> it's the obvious implementation and I assumed the original patch didn't
>>> do it that way for a reason.
>>
>> Maybe yes. Only Sudeep can tell why he didn't do it that way. But
>> yeah, the intent was only what the comment says.
>
> So sounds like my alternative fix of checking the 'actual' frequency
> immediately after setting it is probably the way forward, unless Sudeep
> chimes in with additional info about the issue he was trying to address.
>

(Sorry was delayed response, was traveling last 3 days)

Honestly, I forgot to take into about the difference between virtual and
actual frequency in bL switcher context when I made this change. Sorry
for that. I have not looked at this patch yet, need to recall bLS
understanding first for that.

It needs to be removed once the clk core propagates the hardware error
to the user of clk_set correctly. Mike had mentioned that clk layer
needs some surgery to fix that :)

Regards,
Sudeep

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Sudeep Holla]

On 08/10/15 10:23, Jon Medhurst (Tixy) wrote:
[...]

> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> index f1e42f8..59115a4 100644
> --- a/drivers/cpufreq/arm_big_little.c
> +++ b/drivers/cpufreq/arm_big_little.c
> @@ -149,6 +149,18 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>   			__func__, cpu, old_cluster, new_cluster, new_rate);
>
>   	ret = clk_set_rate(clk[new_cluster], new_rate * 1000);
> +	if (!ret) {
> +		/*
> +		 * FIXME: clk_set_rate has to handle the case where clk_change_rate
> +		 * can fail due to hardware or firmware issues. Until the clk core
> +		 * layer is fixed, we can check here. In most of the cases we will
> +		 * be reading only the cached value anyway. This needs to  be removed
> +		 * once clk core is fixed.
> +		 */
> +		if (clk_get_rate(clk[new_cluster]) != new_rate * 1000)
> +			ret = -EIO;
> +	}
> +
>   	if (WARN_ON(ret)) {
>   		pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret,
>   				new_cluster);
> @@ -189,15 +201,6 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>   		mutex_unlock(&cluster_lock[old_cluster]);
>   	}
>
> -	/*
> -	 * FIXME: clk_set_rate has to handle the case where clk_change_rate
> -	 * can fail due to hardware or firmware issues. Until the clk core
> -	 * layer is fixed, we can check here. In most of the cases we will
> -	 * be reading only the cached value anyway. This needs to  be removed
> -	 * once clk core is fixed.
> -	 */
> -	if (bL_cpufreq_get_rate(cpu) != new_rate)
> -		return -EIO;
>   	return 0;
>   }
>
>
>
>

The above change looks good to me but with minor nit. You can get rid of
if(!ret) check if you move the hunk after if (WARN_ON(ret))

-- 
Regards, Sudeep

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Jon Medhurst (Tixy)]

On Mon, 2015-10-12 at 14:20 +0100, Sudeep Holla wrote:
> 
> On 08/10/15 10:23, Jon Medhurst (Tixy) wrote:
> [...]
> 
> > diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> > index f1e42f8..59115a4 100644
> > --- a/drivers/cpufreq/arm_big_little.c
> > +++ b/drivers/cpufreq/arm_big_little.c
> > @@ -149,6 +149,18 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> >   			__func__, cpu, old_cluster, new_cluster, new_rate);
> >
> >   	ret = clk_set_rate(clk[new_cluster], new_rate * 1000);
> > +	if (!ret) {
> > +		/*
> > +		 * FIXME: clk_set_rate has to handle the case where clk_change_rate
> > +		 * can fail due to hardware or firmware issues. Until the clk core
> > +		 * layer is fixed, we can check here. In most of the cases we will
> > +		 * be reading only the cached value anyway. This needs to  be removed
> > +		 * once clk core is fixed.
> > +		 */
> > +		if (clk_get_rate(clk[new_cluster]) != new_rate * 1000)
> > +			ret = -EIO;
> > +	}
> > +
> >   	if (WARN_ON(ret)) {
> >   		pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret,
> >   				new_cluster);
> > @@ -189,15 +201,6 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> >   		mutex_unlock(&cluster_lock[old_cluster]);
> >   	}
> >
> > -	/*
> > -	 * FIXME: clk_set_rate has to handle the case where clk_change_rate
> > -	 * can fail due to hardware or firmware issues. Until the clk core
> > -	 * layer is fixed, we can check here. In most of the cases we will
> > -	 * be reading only the cached value anyway. This needs to  be removed
> > -	 * once clk core is fixed.
> > -	 */
> > -	if (bL_cpufreq_get_rate(cpu) != new_rate)
> > -		return -EIO;
> >   	return 0;
> >   }
> >
> >
> >
> >
> 
> The above change looks good to me but with minor nit. You can get rid of
> if(!ret) check if you move the hunk after if (WARN_ON(ret))

But then we wouldn't get the WARN_ON and pr_err triggered when we detect
the clock rate isn't set, which surely is half the reason for the check
in the first place?

(P.S. I'm on holiday this week without access to my main computer, dev
boards or decent internet connectivity).

-- 
Tixy

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Sudeep Holla]

On 13/10/15 08:19, Jon Medhurst (Tixy) wrote:
> On Mon, 2015-10-12 at 14:20 +0100, Sudeep Holla wrote:
>>
>> On 08/10/15 10:23, Jon Medhurst (Tixy) wrote:
>> [...]
>>
>>> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
>>> index f1e42f8..59115a4 100644
>>> --- a/drivers/cpufreq/arm_big_little.c
>>> +++ b/drivers/cpufreq/arm_big_little.c
>>> @@ -149,6 +149,18 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>>>    			__func__, cpu, old_cluster, new_cluster, new_rate);
>>>
>>>    	ret = clk_set_rate(clk[new_cluster], new_rate * 1000);
>>> +	if (!ret) {
>>> +		/*
>>> +		 * FIXME: clk_set_rate has to handle the case where clk_change_rate
>>> +		 * can fail due to hardware or firmware issues. Until the clk core
>>> +		 * layer is fixed, we can check here. In most of the cases we will
>>> +		 * be reading only the cached value anyway. This needs to  be removed
>>> +		 * once clk core is fixed.
>>> +		 */
>>> +		if (clk_get_rate(clk[new_cluster]) != new_rate * 1000)
>>> +			ret = -EIO;
>>> +	}
>>> +
>>>    	if (WARN_ON(ret)) {
>>>    		pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret,
>>>    				new_cluster);
>>> @@ -189,15 +201,6 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>>>    		mutex_unlock(&cluster_lock[old_cluster]);
>>>    	}
>>>
>>> -	/*
>>> -	 * FIXME: clk_set_rate has to handle the case where clk_change_rate
>>> -	 * can fail due to hardware or firmware issues. Until the clk core
>>> -	 * layer is fixed, we can check here. In most of the cases we will
>>> -	 * be reading only the cached value anyway. This needs to  be removed
>>> -	 * once clk core is fixed.
>>> -	 */
>>> -	if (bL_cpufreq_get_rate(cpu) != new_rate)
>>> -		return -EIO;
>>>    	return 0;
>>>    }
>>>
>>>
>>>
>>>
>>
>> The above change looks good to me but with minor nit. You can get rid of
>> if(!ret) check if you move the hunk after if (WARN_ON(ret))
>
> But then we wouldn't get the WARN_ON and pr_err triggered when we detect
> the clock rate isn't set, which surely is half the reason for the check
> in the first place?
>

Not sure if I understand what you mean or may be I was not clear, so
thought I will put the delta here. Let me know if and how its still a 
problem.

Regards,
Sudeep

-->8

diff --git i/drivers/cpufreq/arm_big_little.c 
w/drivers/cpufreq/arm_big_little.c
index f1e42f8ce0fc..05e850f80f39 100644
--- i/drivers/cpufreq/arm_big_little.c
+++ w/drivers/cpufreq/arm_big_little.c
@@ -164,6 +164,16 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 
new_cluster, u32 rate)

         mutex_unlock(&cluster_lock[new_cluster]);

+       /*
+        * FIXME: clk_set_rate has to handle the case where clk_change_rate
+        * can fail due to hardware or firmware issues. Until the clk core
+        * layer is fixed, we can check here. In most of the cases we will
+        * be reading only the cached value anyway. This needs to  be 
removed
+        * once clk core is fixed.
+        */
+       if (bL_cpufreq_get_rate(cpu) != new_rate)
+               return -EIO;
+
         /* Recalc freq for old cluster when switching clusters */
         if (old_cluster != new_cluster) {
                 pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d\n",
@@ -189,15 +199,6 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 
new_cluster, u32 rate)
                 mutex_unlock(&cluster_lock[old_cluster]);
         }

-       /*
-        * FIXME: clk_set_rate has to handle the case where clk_change_rate
-        * can fail due to hardware or firmware issues. Until the clk core
-        * layer is fixed, we can check here. In most of the cases we will
-        * be reading only the cached value anyway. This needs to  be 
removed
-        * once clk core is fixed.
-        */
-       if (bL_cpufreq_get_rate(cpu) != new_rate)
-               return -EIO;
         return 0;
  }

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Jon Medhurst (Tixy)]

On Tue, 2015-10-13 at 11:36 +0100, Sudeep Holla wrote:
> 
> On 13/10/15 08:19, Jon Medhurst (Tixy) wrote:
[...]
> > But then we wouldn't get the WARN_ON and pr_err triggered when we detect
> > the clock rate isn't set, which surely is half the reason for the check
> > in the first place?
> >
> 
> Not sure if I understand what you mean or may be I was not clear, so
> thought I will put the delta here. Let me know if and how its still a 
> problem.
>
> diff --git i/drivers/cpufreq/arm_big_little.c 
> w/drivers/cpufreq/arm_big_little.c
> index f1e42f8ce0fc..05e850f80f39 100644
> --- i/drivers/cpufreq/arm_big_little.c
> +++ w/drivers/cpufreq/arm_big_little.c
> @@ -164,6 +164,16 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 
> new_cluster, u32 rate)
> 
>          mutex_unlock(&cluster_lock[new_cluster]);
> 
> +       /*
> +        * FIXME: clk_set_rate has to handle the case where clk_change_rate
> +        * can fail due to hardware or firmware issues. Until the clk core
> +        * layer is fixed, we can check here. In most of the cases we will
> +        * be reading only the cached value anyway. This needs to  be 
> removed
> +        * once clk core is fixed.
> +        */
> +       if (bL_cpufreq_get_rate(cpu) != new_rate)
> +               return -EIO;
> +
>          /* Recalc freq for old cluster when switching clusters */
>          if (old_cluster != new_cluster) {
>                  pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d\n",

That's what I though you meant, and I can't see why you would want to do
that and bypass the error reporting for clk_get_rate failing. After all,
the code we're moving around is explicitly there to workaround the fact
that clk_set_rate doesn't actually pass through all errors, so it's
doing additional error checking. (At least, that's what the comment
says). So this looks more logical to me.

ret = clk_set_rate()
if(!ret)                         /* if no error from clk_set_rate   */
    if(clk_get_rate()!=correct)  /* but some additional checks fail */
        ret = -EIO;              /* then indicate an error anyway   */

if (WARN_ON(ret))                /* Warn if error setting rate and */
    pr_err("clk_set_rate failed")/* print and error too */


But if people want the if(clk_get_rate()!=correct) after the WARN_ON
then lets do that, the important thing is to get the code fixed.

-- 
Tixy

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Sudeep Holla]

On 14/10/15 08:12, Jon Medhurst (Tixy) wrote:
> On Tue, 2015-10-13 at 11:36 +0100, Sudeep Holla wrote:
>>
>> On 13/10/15 08:19, Jon Medhurst (Tixy) wrote:
> [...]
>>> But then we wouldn't get the WARN_ON and pr_err triggered when we detect
>>> the clock rate isn't set, which surely is half the reason for the check
>>> in the first place?
>>>
>>
>> Not sure if I understand what you mean or may be I was not clear, so
>> thought I will put the delta here. Let me know if and how its still a
>> problem.
>>
>> diff --git i/drivers/cpufreq/arm_big_little.c
>> w/drivers/cpufreq/arm_big_little.c
>> index f1e42f8ce0fc..05e850f80f39 100644
>> --- i/drivers/cpufreq/arm_big_little.c
>> +++ w/drivers/cpufreq/arm_big_little.c
>> @@ -164,6 +164,16 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32
>> new_cluster, u32 rate)
>>
>>           mutex_unlock(&cluster_lock[new_cluster]);
>>
>> +       /*
>> +        * FIXME: clk_set_rate has to handle the case where clk_change_rate
>> +        * can fail due to hardware or firmware issues. Until the clk core
>> +        * layer is fixed, we can check here. In most of the cases we will
>> +        * be reading only the cached value anyway. This needs to  be
>> removed
>> +        * once clk core is fixed.
>> +        */
>> +       if (bL_cpufreq_get_rate(cpu) != new_rate)
>> +               return -EIO;
>> +
>>           /* Recalc freq for old cluster when switching clusters */
>>           if (old_cluster != new_cluster) {
>>                   pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d\n",
>
> That's what I though you meant, and I can't see why you would want to do
> that and bypass the error reporting for clk_get_rate failing. After all,
> the code we're moving around is explicitly there to workaround the fact
> that clk_set_rate doesn't actually pass through all errors, so it's
> doing additional error checking. (At least, that's what the comment
> says). So this looks more logical to me.
>

OK, I understand what you mean now. I don't have a strong opinion, but
here is the reason why I prefer the approach I said earlier:
clk_set_rate doesn't return error if the h/w or f/w return error which
is usually the last step. So calling clk_get_rate when clk_set_rate
return error quite early makes no sense to me.

-- 
Regards,
Sudeep

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Jon Medhurst (Tixy)]

On Wed, 2015-10-14 at 09:48 +0100, Sudeep Holla wrote:
> 
> On 14/10/15 08:12, Jon Medhurst (Tixy) wrote:
> > On Tue, 2015-10-13 at 11:36 +0100, Sudeep Holla wrote:
> >>
> >> On 13/10/15 08:19, Jon Medhurst (Tixy) wrote:
> > [...]
> >>> But then we wouldn't get the WARN_ON and pr_err triggered when we detect
> >>> the clock rate isn't set, which surely is half the reason for the check
> >>> in the first place?
> >>>
> >>
> >> Not sure if I understand what you mean or may be I was not clear, so
> >> thought I will put the delta here. Let me know if and how its still a
> >> problem.
> >>
> >> diff --git i/drivers/cpufreq/arm_big_little.c
> >> w/drivers/cpufreq/arm_big_little.c
> >> index f1e42f8ce0fc..05e850f80f39 100644
> >> --- i/drivers/cpufreq/arm_big_little.c
> >> +++ w/drivers/cpufreq/arm_big_little.c
> >> @@ -164,6 +164,16 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32
> >> new_cluster, u32 rate)
> >>
> >>           mutex_unlock(&cluster_lock[new_cluster]);
> >>
> >> +       /*
> >> +        * FIXME: clk_set_rate has to handle the case where clk_change_rate
> >> +        * can fail due to hardware or firmware issues. Until the clk core
> >> +        * layer is fixed, we can check here. In most of the cases we will
> >> +        * be reading only the cached value anyway. This needs to  be
> >> removed
> >> +        * once clk core is fixed.
> >> +        */
> >> +       if (bL_cpufreq_get_rate(cpu) != new_rate)
> >> +               return -EIO;
> >> +
> >>           /* Recalc freq for old cluster when switching clusters */
> >>           if (old_cluster != new_cluster) {
> >>                   pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d\n",
> >
> > That's what I though you meant, and I can't see why you would want to do
> > that and bypass the error reporting for clk_get_rate failing. After all,
> > the code we're moving around is explicitly there to workaround the fact
> > that clk_set_rate doesn't actually pass through all errors, so it's
> > doing additional error checking. (At least, that's what the comment
> > says). So this looks more logical to me.
> >
> 
> OK, I understand what you mean now. I don't have a strong opinion, but
> here is the reason why I prefer the approach I said earlier:
> clk_set_rate doesn't return error if the h/w or f/w return error which
> is usually the last step. So calling clk_get_rate when clk_set_rate
> return error quite early makes no sense to me.

It doesn't to me either, but my suggested code doesn't do that, it only
calls clk_get_rate if the is _no_ error from clk_set_rate, the pseudo
code again...

ret = clk_set_rate()
if(!ret)                         /* if no error from clk_set_rate   */
    if(clk_get_rate()!=correct)  /* but some additional checks fail */
        ret = -EIO;              /* then indicate an error anyway   */

!ret is ret==0 is 'no error' as the comment says. So the clock framework
thinks the rate was set OK and we then use clk_get_rate to see if those
unreported h/w or f/w errors mean that it actually wasn't set OK.

-- 
Tixy

Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

[Sudeep Holla]

On 19/10/15 09:33, Jon Medhurst (Tixy) wrote:
> On Wed, 2015-10-14 at 09:48 +0100, Sudeep Holla wrote:
>>

[...]

>>
>> OK, I understand what you mean now. I don't have a strong opinion, but
>> here is the reason why I prefer the approach I said earlier:
>> clk_set_rate doesn't return error if the h/w or f/w return error which
>> is usually the last step. So calling clk_get_rate when clk_set_rate
>> return error quite early makes no sense to me.
>
> It doesn't to me either, but my suggested code doesn't do that, it only
> calls clk_get_rate if the is _no_ error from clk_set_rate, the pseudo
> code again...
>
> ret = clk_set_rate()
> if(!ret)                         /* if no error from clk_set_rate   */
>      if(clk_get_rate()!=correct)  /* but some additional checks fail */
>          ret = -EIO;              /* then indicate an error anyway   */
>
> !ret is ret==0 is 'no error' as the comment says. So the clock framework
> thinks the rate was set OK and we then use clk_get_rate to see if those
> unreported h/w or f/w errors mean that it actually wasn't set OK.
>

Ah sorry, my mistake. May be I got carried away by that extra if(!ret).
I am fine with the patch.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep