Re: [PATCH v2 1/2] cpufreq: CPPC: keep target core awake when reading its cpufreq rate

[Yang Shi <yang@xxxxxxxxxxxxxxxxxxxxxx>]

On 5/17/23 8:03 AM, Sumit Gupta wrote:
> On 17/05/23 13:47, Pierre Gondois wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > +Ionela, Sumit, Yang,
> >
> > Hello Zeng,
> >
> > I think solutions around related issues were suggested at:
> >
> > [1] 
> > https://lore.kernel.org/all/20230418113459.12860-7-sumitg@xxxxxxxxxx/
> > [2] 
> > https://lore.kernel.org/all/20230328193846.8757-1-yang@xxxxxxxxxxxxxxxxxxxxxx/
> > [3] https://lore.kernel.org/all/ZEl1Fms%2FJmdEZsVn@xxxxxxx/
> >
> > About this patch, it seems to mean that CPPC counters of CPUx are always
> > accessed from CPUx, even when they are not AMUs. For instance CPPC
> > counters could be memory mapped and accessible from any CPU.
> > cpu_has_amu_feat() should allow to probe if a CPU uses AMUs or not,
> > and [2] had an implementation using it.
> >
> > Another comment about PATCH 2/2 is that if the counters are accessed
> > through FFH, arm64 version of cpc_read_ffh() is calling
> > counters_read_on_cpu(), and a comment in counters_read_on_cpu() seems
> > to specify the function must be called with interrupt enabled.
> >
> > I think the best solution so far was the one at [3], suggested by 
> > Ionela,
> > but it doesn't seem to solve your issue. Indeed, it is not checked 
> > whether
> > the counters are AMU counters and that they must be remotely read (to
> > have the CPU awake),
> >
> > Regards,
> > Pierre
>
> I think the solution in [1] is simple and solves all the three cases.
> Also, it provides better accuracy between the set and get frequency as 
> compared to [3].

I don't think [1] patches work for our case. We use mmio instead of AMU. 
Increasing delay could help to mitigate it somehow, buyt 25us is not 
good enough for our case. IIRC the fix proposed by Ionela works for both 
yours and mine.

> This can be merged and can later still be improved in Upstream.
>
> If OK, I can send new version by changing the patch to apply for all 
> ARM SoC's with AMU and not specific to Tegra.
>
> Thank you,
> Sumit Gupta
>
> > On 5/16/23 15:32, 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 during calculation.
> > >
> > > Signed-off-by: Zeng Heng <zengheng4@xxxxxxxxxx>
> > > ---
> > >   drivers/cpufreq/cppc_cpufreq.c | 30 +++++++++++++++++++-----------
> > >   1 file changed, 19 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/cppc_cpufreq.c 
> > > b/drivers/cpufreq/cppc_cpufreq.c
> > > index 022e3555407c..910167f58bb3 100644
> > > --- a/drivers/cpufreq/cppc_cpufreq.c
> > > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > > @@ -837,9 +837,24 @@ static int cppc_perf_from_fbctrs(struct 
> > > cppc_cpudata *cpu_data,
> > >       return (reference_perf * delta_delivered) / delta_reference;
> > >   }
> > >
> > > +static int cppc_get_perf_ctrs_smp(void *val)
> > > +{
> > > +     int cpu = smp_processor_id();
> > > +     struct cppc_perf_fb_ctrs *fb_ctrs = val;
> > > +     int ret;
> > > +
> > > +     ret = cppc_get_perf_ctrs(cpu, fb_ctrs);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     udelay(2); /* 2usec delay between sampling */
> > > +
> > > +     return cppc_get_perf_ctrs(cpu, fb_ctrs + 1);
> > > +}
> > > +
> > >   static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
> > >   {
> > > -     struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> > > +     struct cppc_perf_fb_ctrs fb_ctrs[2] = {0};
> > >       struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> > >       struct cppc_cpudata *cpu_data = policy->driver_data;
> > >       u64 delivered_perf;
> > > @@ -847,19 +862,12 @@ 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 ret;
> > > -
> > > -     udelay(2); /* 2usec delay between sampling */
> > > -
> > > -     ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
> > > +     ret = smp_call_on_cpu(cpu, cppc_get_perf_ctrs_smp, fb_ctrs, 1);
> > >       if (ret)
> > >               return ret;
> > >
> > > -     delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
> > > -                                            &fb_ctrs_t1);
> > > -
> > > +     delivered_perf = cppc_perf_from_fbctrs(cpu_data, fb_ctrs,
> > > +                                            fb_ctrs + 1);
> > >       return cppc_cpufreq_perf_to_khz(cpu_data, delivered_perf);
> > >   }