Re: [PATCH 1/2] cpufreq: arm_big_little: check if the frequency is set correctly

[Michael Turquette <mike.turquette@linaro.org>]

Quoting Sudeep Holla (2015-03-30 06:39:00)
> 
> 
> On 30/03/15 14:27, Viresh Kumar wrote:
> > On 30 March 2015 at 17:57, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >> The actual frequency is set through "clk_change_rate" which is void
> >> function. If the underlying hardware fails and returns error, the error
> >> is lost in the clk layer. In order to track such failures, we need to
> >> read back the frequency(just the cached value as clk_recalc called after
> >> clk->ops->set_rate gets the frequency)
> >>
> >> This patch adds check to see if the frequency is set correctly or if
> >> they were any hardware failures and sends the appropriate errors to the
> >> cpufreq core.
> >>
> >> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >> ---
> >>   drivers/cpufreq/arm_big_little.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> >> index e1a6ba66a7f5..3fc676c63f91 100644
> >> --- a/drivers/cpufreq/arm_big_little.c
> >> +++ b/drivers/cpufreq/arm_big_little.c
> >> @@ -186,6 +186,8 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> >>                  mutex_unlock(&cluster_lock[old_cluster]);
> >>          }
> >>
> >> +       if (bL_cpufreq_get_rate(cpu) != new_rate)
> >> +               return -EIO;
> >>          return 0;
> >>   }
> >
> > This doesn't look to me the right place for fixing this.
> >
> 
> Yes I agree, after going through clk.c, I thought pre-/post- notifiers
> are designed for such purpose. I tried using them but found it
> unnecessary when it can be as simple as in this patch. However it's good
> to hear from Mike as I seem to have assumed a lot here.

Viresh & Sudeep,

clk_set_rate returns an error (and always has), so it seems to me that
this patch is unnecessary. bL_cpufreq_set_rate checks for an error from
clk_set_rate and handles it.

clk_change_rate is static and not exposed outside of drivers/clk/clk.c.

This patch gets a NAK from me.

Regards,
Mike

> 
> Regards,
> Sudeep