| Message ID | 561CDEA9.7020700@arm.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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>
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 */