cpufreq: arm_big_little: fix frequency check when bL switcher is active

Message ID 561CDEA9.7020700@arm.com
State New
Headers show

Commit Message

Sudeep Holla Oct. 13, 2015, 10:36 a.m.
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

         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;
  }
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jon Medhurst (Tixy) Oct. 14, 2015, 7:12 a.m. | #1
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.
Sudeep Holla Oct. 14, 2015, 8:48 a.m. | #2
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.
Jon Medhurst (Tixy) Oct. 19, 2015, 8:33 a.m. | #3
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.
Sudeep Holla Oct. 19, 2015, 8:44 a.m. | #4
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>

Patch

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 */