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

[Pierre Gondois <pierre.gondois@arm.com>]

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


> Thank you,
> Sumit Gupta
>