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

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

On Wednesday 28 Aug 2024 at 17:45:09 (+0800), Jie Zhan wrote:
> 
> 
> On 28/08/2024 16:17, Ionela Voinescu wrote:
> > 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>'
> Hi Ionela,
> > 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.
> Yeah. 1 is the main thing to deal with.
> > > > 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.
> Sure, understood.
> Getting the latest desired perf would be more compatible across platforms.
> 
> Merging hisi_cppc_cpufreq_get_rate() can be risky but worth a try. The
> workaround also disables the FIE. I'll figure out whether it's feasible to
> do.

Thanks! What I was thinking was that possibly after your changes the
current cppc_cpufreq_get_rate() would be suitable for what is now the
hisilicon workaround, so there wouldn't be a need to overwrite the .get
callback with a custom one. In depends on whether on that particular
platform the unsupported counter registers read as 0 and result in the
same -EFAUT error.

As for disabling FIE, the current cppc_check_hisi_workaround() can be
called from cppc_freq_invariance_init() as an added check to the existing
ones that result in disabling FIE.

Thanks,
Ionela.

> 
> I'll send a V2 if no objection to the error handling.
> 
> Thanks,
> Jie
> > 
> > > > +	return 0;
> > > >   }
> > > >   static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> > > > -- 
> > > > 2.33.0
> > > > 
> > > -- 
> > > viresh
>