Re: [PATCH] cpufreq: CPPC: Return desired perf in ->get() if feedback counters are 0

[Ionela Voinescu <ionela.voinescu@arm.com>]

Hi,

On Wednesday 28 Aug 2024 at 12:20:41 (+0530), Viresh Kumar wrote:
> Cc'd few developers.
> 
> On 19-08-24, 11:51, Jie Zhan wrote:
> > The CPPC performance feedback counters could return 0 when the target cpu
> > is in a deep idle state (e.g. powered off) and those counters are not
> > powered.  cppc_cpufreq_get_rate() returns 0 in this case, triggering two
> > problems:
> > 
> > 1. cpufreq_online() gets a false error and doesn't generate a cpufreq
> > policy, which happens in cpufreq_add_dev() when a new cpu device is added.
> > 2. 'cpuinfo_cur_freq' shows '<unknown>'

I suppose 2. is not necessarily a problem as the current (hardware)
frequency is indeed unknown.

But there's not clean way to fix 1. while keeping 2. as is, or at least
not one I could identify.

> > 
> > Don't take it as an error and return the frequency corresponding to the
> > desired perf when the feedback counters are 0.
> > 
> > Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.")
> > Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> > ---
> >  drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > index bafa32dd375d..1c5eb12c1a5a 100644
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -748,18 +748,25 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
> >  
> >  	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
> >  	if (ret)
> > -		return 0;
> > +		goto out_err;
> >  
> >  	udelay(2); /* 2usec delay between sampling */
> >  
> >  	ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
> >  	if (ret)
> > -		return 0;
> > +		goto out_err;
> >  
> >  	delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
> >  					       &fb_ctrs_t1);
> >  
> >  	return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> > +
> > +out_err:
> > +	if (ret == -EFAULT)
> > +		return cppc_perf_to_khz(&cpu_data->perf_caps,
> > +					cpu_data->perf_ctrls.desired_perf);
> > +

A better way might be to cppc_get_desired_perf(cpu, &desired_perf) first
and return the khz equivalent of the result, as currently done in
hisi_cppc_cpufreq_get_rate(). Even a merge of the two functions might be
suitable, but I'm not familiar with the specifics of the hisilicon platforms
involved. This might be better as some platforms can provide performance
feedback through the desired performance register so a read of it would
be better than using the cached desired_perf value.

Hope it helps,
Ionela.

> > +	return 0;
> >  }
> >  
> >  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> > -- 
> > 2.33.0
> > 
> 
> -- 
> viresh