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

[Pierre Gondois]

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()

> 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


> 4. pr_warn() in show_ospm_nominal_freq() on HW vs cache
>     mismatch: Skipping it since it relies on (2).
>
> So in v4, will pick up the rollback and leave the rest as is.
> Happy to discuss further if you think differently.