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

["Jon Medhurst (Tixy)" <tixy@linaro.org>]

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.

-- 
Tixy