| Message ID | 1444296229.2847.9.camel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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/
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))
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 --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; }