Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1241713 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-07 19:40 +0200
    Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2015-10-08 11:30 +0200
      Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-08 13:30 +0200
        Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2015-10-08 15:00 +0200
          Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-08 16:00 +0200
          Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active Sudeep Holla <sudeep.holla@foss.arm.com> - 2015-10-08 16:20 +0200
      Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active Sudeep Holla <sudeep.holla@arm.com> - 2015-10-12 15:30 +0200
        Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2015-10-13 09:20 +0200
          Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active Sudeep Holla <sudeep.holla@arm.com> - 2015-10-13 12:40 +0200
            Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2015-10-14 09:20 +0200
              Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL  switcher is active Sudeep Holla <sudeep.holla@arm.com> - 2015-10-14 10:50 +0200

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

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

[toc] | [next] | [standalone]

#1242110

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



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

[toc] | [prev] | [next] | [standalone]

#1242218

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

[toc] | [prev] | [next] | [standalone]

#1242317

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

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

[toc] | [prev] | [next] | [standalone]

#1242385

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

[toc] | [prev] | [next] | [standalone]

#1242398

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/

[toc] | [prev] | [next] | [standalone]

#1244688

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

[toc] | [prev] | [next] | [standalone]

#1245403

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

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

[toc] | [prev] | [next] | [standalone]

#1245559

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

[toc] | [prev] | [next] | [standalone]

#1246354

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

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

[toc] | [prev] | [next] | [standalone]

#1246422

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web