diff mbox

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

Message ID 1444296229.2847.9.camel@linaro.org
State New
Headers show

Commit Message

Jon Medhurst (Tixy) Oct. 8, 2015, 9:23 a.m. UTC 
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.




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

Viresh Kumar Oct. 8, 2015, 11:24 a.m. UTC | #1 
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.
Jon Medhurst (Tixy) Oct. 8, 2015, 12:55 p.m. UTC | #2 
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.
Viresh Kumar Oct. 8, 2015, 1:52 p.m. UTC | #3 
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.
Sudeep Holla Oct. 8, 2015, 2:18 p.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Sudeep Holla Oct. 12, 2015, 1:20 p.m. UTC | #5 
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))
Jon Medhurst (Tixy) Oct. 13, 2015, 7:19 a.m. UTC | #6 
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).
diff mbox

Patch

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;
 }