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

[Jon Medhurst (Tixy)]

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).

-- 
Tixy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/