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

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.

Thank you,
Sumit Gupta