Re: [PATCH v2 2/2] cpufreq: CPPC: Dont read counters for idle CPUs

[Beata Michalska <beata.michalska@arm.com>]

On Wed, Jul 09, 2025 at 03:49:03PM -0700, Prashant Malani wrote:
> On Wed, 9 Jul 2025 at 10:25, Prashant Malani <pmalani@google.com> wrote:
> >
> > Hi Beata,
> >
> > Thanks for taking a look.
> >
> > On Mon, 7 Jul 2025 at 01:33, Beata Michalska <beata.michalska@arm.com> wrote:
> > >
> > > Hi Prashant,
> > >
> > > On Wed, Jul 02, 2025 at 11:38:11AM -0700, Prashant Malani wrote:
> > > > Hi All,
> > > >
> > > > Ionela, Beata, could you kindly review ?
> > > >
> > > > On Fri, 27 Jun 2025 at 10:07, Prashant Malani <pmalani@google.com> wrote:
> > > > >
> > > >
> > > > I think it is pertinent to note: the actual act of reading the CPPC counters
> > > > will (at least for ACPI_ADR_SPACE_FIXED_HARDWARE counters)
> > > > wake the CPU up, so even if a CPU *was* idle, the reading of the counters
> > > > calls cpc_read_ffh() [1] which does an IPI on the target CPU [2] thus waking
> > > > it up from WFI.
> > > >
> > > > And that brings us back to the original assertion made in this patch:
> > > > the counter values are quite unreliable when the CPU is in this
> > > > idle (or rather I should correct that to, waking from WFI) state.
> > > >
> > > I'd say that's very platform specific, and as such playing with the delay makes
> > > little sense. I'd need to do more deliberate testing, but I haven't noticed
> > > (yet) any discrepancies in AMU counters on waking up.
> > > Aside, you have mentioned that you've observed the frequency reported to be
> > > above max one (4GHz vs 3.5HZ if I recall correctly) - shouldn't that be clamped
> > > by the driver if the values are outside of supported range ?
> > >
> > > Verifying whether the CPU is idle before poking it just to get a counters
> > > reading to derive current frequency from those does feel rather like an
> > > appealing idea.
> >
> > That's good to hear. What can we do to refine this series further?
So I believe this should be handled in CPUFreq core, if at all.
Would be good to get an input/opinion from the maintainers: Viresh and Rafael.
> >
> > > Narrowing the scope for ACPI_ADR_SPACE_FIXED_HARDWARE cases
> > > could be solved by providing a query for the address space. Though I am not an
> > > expert here so would be good to get some input from someone who is
> > > (on both).
> >
> > Who would be the expert here (are they on this mailing list)?
Probably as above + Sudeep Holla
> >
> > Could you point me to an example for the query for the address space? Then
> > I can respin this series to use that query and narrow the scope.
>
Actually was suggesting adding one.
> Actually, if the idea of this optimization (the idle_cpu check) sounds
> good to you,
> I don't see why we should limit it to ACPI_ADR_SPACE_FIXED_HARDWARE.
> IOW, the patch can remain in its current form.
> 
Right, that does need though verifying against all users.

In the meantime ....
It seems that the issue of getting counters on a CPU that is idle is not
in the counters themselves, but in the way how they are being read - at least
from what I can observe.
The first read experience longer delay between reading core and const counters,
and as const one is read as a second one, it misses some increments (within
calculated delta). So, what we could do within the driver is either:
- Add a way to request reading both counters in a single cpc_read (preferable
  I guess, though I would have to have a closer look at that)
- Add some logic that would make sure the reads are not far apart

Would you be able to verify that on your end?

---
BR
Beata
> Best regards,
> 
> -- 
> -Prashant