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

[Sumit Gupta <sumitg@nvidia.com>]

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