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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
