Messages in this thread | | | Date | Thu, 26 Oct 2023 09:53:21 +0100 | From | Sudeep Holla <> | Subject | Re: [PATCH 2/3] cpufreq: CPPC: Keep the target core awake when reading its cpufreq rate |
| |
On Thu, Oct 26, 2023 at 10:24:54AM +0800, Zeng Heng wrote: > > 在 2023/10/25 19:13, Sudeep Holla 写道: > > On Wed, Oct 25, 2023 at 05:38:46PM +0800, Zeng Heng wrote: > > > As ARM AMU's document says, all counters are subject to any changes > > > in clock frequency, including clock stopping caused by the WFI and WFE > > > instructions. > > > > > > Therefore, using smp_call_on_cpu() to trigger target CPU to > > > read self's AMU counters, which ensures the counters are working > > > properly while cstate feature is enabled. > > > > > > Reported-by: Sumit Gupta <sumitg@nvidia.com> > > > Link: https://lore.kernel.org/all/20230418113459.12860-7-sumitg@nvidia.com/ > > > Signed-off-by: Zeng Heng <zengheng4@huawei.com> > > > --- > > > drivers/cpufreq/cppc_cpufreq.c | 39 ++++++++++++++++++++++++++-------- > > > 1 file changed, 30 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > > > index fe08ca419b3d..321a9dc9484d 100644 > > > --- a/drivers/cpufreq/cppc_cpufreq.c > > > +++ b/drivers/cpufreq/cppc_cpufreq.c > > [...] > > > > > @@ -850,18 +871,18 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > > > cpufreq_cpu_put(policy); > > > - ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); > > > - if (ret) > > > - return 0; > > > - > > > - udelay(2); /* 2usec delay between sampling */ > > > + if (cpu_has_amu_feat(cpu)) > > Have you compiled this on x86 ? Even if you have somehow managed to, > > this is not the right place to check the presence of AMU feature on > > the CPU. > > If AMU registers are used in CPPC, they must be using FFH GAS, in which > > case the interpretation of FFH is architecture dependent code. > > According to drivers/cpufreq/Makefile, cppc_cpufreq.c is only compiled with > ARM architecture. >
Well that's true but this change doesn't belong to cppc_cpufreq.c, it must be part of drivers/acpi/cppc_acpi.c IMO and sorry I assumed that without explicitly mentioning that here.
> But here, I would change cpu_has_amu_feat() with cpc_ffh_supported(), which > belongs to FFH APIs. >
It is not like that. cppc_acpi.c will know the GAS is FFH based so no need to check anything there. I see counters_read_on_cpu() called from cpc_ffh_read() already takes care of reading the AMUs on the right CPU. What exactly is the issue you are seeing ? I don't if this change is needed at all.
-- Regards, Sudeep
| |