Re: [PATCH v3 2/2] ACPI: CPPC: Add ospm_nominal_perf support

[Sumit Gupta]

Hi Pierre,


On 11/06/26 20:01, Pierre Gondois wrote:
> External email: Use caution opening links or attachments
>
>
> On 6/9/26 10:53, Sumit Gupta wrote:
> > On 28/05/26 17:37, Pierre Gondois wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > Hello Sumit,
> > > > Hi Pierre,
> > > >
> > > > Thanks for the review and the complementary patch.
> > > > Going point by point:
> > > >
> > > > 1. Rollback for a partially applied multiple CPU write in
> > > >     store_ospm_nominal_freq(): Agreed, will add into v4.
> > > >
> > > > 2. cppc_get_ospm_nominal_perf() and the show/init/exit coherence
> > > >     checks that rely on it: I'd skip these as the register is
> > > > write-only
> > > >     as per spec.
> > > NIT:
> > > IIUC having a write-only register doesn't mean we cannot read it.
> > > Cf. cppc_get_desired_perf()
> >
> >
> > Good point.
> > v5 reads the register via a new cppc_get_ospm_nominal_perf().
> > So, show() returns the register value or "<unsupported>",
> > and dropped the cache/bool.
> >
> >
> > > > 3. Initializing the register at startup and restoring at exit: In
> > > > v3, we
> > > >     dropped the unconditional cpu_init write so user values would
> > > >     survive CPU hotplug. The spec also makes the explicit init
> > > >     unnecessary: "If this register is not provided, then OSPM must
> > > >     assume that the OSPM Nominal Performance value is equal to
> > > >     the Nominal Performance value.". The unwritten default already
> > > >     looks well defined.
> > >
> > > The concern I had was for the scenario where:
> > >
> > > - the driver is loaded
> > >
> > > - the user sets an ospm_nominal_freq value
> > >
> > > - the driver is unloaded
> > >
> > > In such case, the ospm_nominal_freq value will still be set to a
> > > non-default value. The modifications suggested previously would
> > > allow to handle that case to come back to the default value.
> > >
> > > FWIU, we have:
> > >
> > > +------+     +---------+     +-----------+     +------+
> > > | User | <-> | CPPC    | <-> | CPPC      | <-> | CPPC |
> > > +------+     | driver  |     | reg       |     | HW   |
> > >              +---------+     | interface |     | reg  |
> > >                              +-----------+     +------+
> > >
> > > So if we want to handle:
> > >
> > > - the case described above
> > >
> > > - the case you mentioned, i.e. hot-plugging CPUs
> > >
> > > maybe the scratch values should be stored along the CPPC register
> > > interface. This would allow to handle complex cases where CPUs
> > > are hotplugged and the driver is loaded/unloaded ?
> > >
> > > Note: the same kind of scenario should apply to the auto_sel register
> >
> > Right.
> > After unload, the register keeps the user set value instead of the
> > firmware value. In a follow-up, I will restore the firmware value on
> > unload and reapply the user value across hotplug, grouping the
> > OSPM-set registers together (ospm_nominal_perf, auto_sel and EPP).
> > On my test platforms the registers survive hotplug, but that isn't
> > guaranteed in general.
> >
> > I think it's better to keep the saved state in the cppc_cpufreq driver
> > rather than the CPPC register interface. intel_pstate and amd-pstate
> > do the same.
> > For reapply, will use a CPU hotplug callback rather than ->online/
> > ->offline hooks. Those are only called when a policy gains its first
> > online CPU or loses its last one. cppc_cpufreq also has shared
> > (SHARED_TYPE_ANY) policy, offlining and onlining a single CPU
> > keeps the policy active, so neither hook is called for it. A per-CPU
> > hotplug callback is needed to cover that case.
> >
> > Let me know if you have other thoughts.
> Is it necessary to have cpuhp callbacks ?
>
> The cppc_cpufreq driver should only support:
> - SHARED_TYPE_HW/NONE: in such case, the polity should
> only have one CPU
> - SHARED_TYPE_ANY: in such case, configuring any CPU should
> configure all the CPUs of the policy. The only issue is that
> we currently don't know whether the CPUs in the policy
> share the same CPPC registers or have individual registers.
> I think it is currently strongly assumed there is only one
> registers for all the CPUs.
>
> For instance, if:
> - CPU0/1 are in the same perf. domain
> - don't have the same CPPC registers
> - have a SHARED_TYPE_ANY policy (i.e. writing to any
> of CPU0/1's CPPC registers triggers an update of the
> hardware for the 2 CPUs)
> - policy->cpu = 0
> then:
> - enabling auto_sel for the policy means configuring
> CPU0's CPPC register
> - if CPU0 is offlined, policy->cpu is updated to 1 (pointing to CPU1),
> - then reading the policy's auto_sel register means reading
> CPU1's CPPC register, which was not updated
>
> So using the already present online()/offline() callbacks should be
> enough. It would also be good to check that, for a policy, all the
> CPPC registers are identical.
>
> Note: having per-CPU CPPC registers + SHARED_TYPE_ANY
> policy is theoretically valid, but unsupported FWIU.
>
> Let me know if it makes sense or not,
>
> Regards,
>
> Pierre


Agreed, the cpuhp callback isn't needed.
For a SHARED_TYPE_ANY policy cppc_cpufreq treats the control registers
as equivalent across the policy's CPUs. A single CPU offline/online
within the policy therefore doesn't lose the value. Only a full teardown
and bring-up can lose value on the platforms that reset the register on
hotplug. That path already goes through ->exit/->init, so reapplying the
saved value from ->init() covers it without adding callbacks.

We can add a check that guards this assumption in cppc_cpufreq_cpu_init,
for the CPUFREQ_SHARED_TYPE_ANY case. Compare the control registers each
CPU in policy->cpus exposes (excluding per-CPU feedback counters) and
warn on a mismatch rather than failing init. Since it concerns existing
behavior, the change can be a separate independent patch. It flags the
per CPU registers case you described. Does that match what you had in mind?

Thank you,
Sumit Gupta