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

Jie Zhan
2024-08-28T09:45:12Z

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.

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