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@xxxxxxxxxxxxx>
---
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
