Re: [PATCH 04/19] cpufreq: amd: introduce a new amd pstate driver to support future processors

[Fontenot, Nathan]

On 9/8/2021 9:59 AM, Huang Rui wrote:
> amd-pstate is the AMD CPU performance scaling driver that introduces a
> new CPU frequency control mechanism on AMD Zen based CPU series in Linux
> kernel. The new mechanism is based on Collaborative processor
> performance control (CPPC) which is finer grain frequency management
> than legacy ACPI hardware P-States. Current AMD CPU platforms are using
> the ACPI P-states driver to manage CPU frequency and clocks with
> switching only in 3 P-states. AMD P-States is to replace the ACPI
> P-states controls, allows a flexible, low-latency interface for the
> Linux kernel to directly communicate the performance hints to hardware.
>

This patch seems like it is just enabling CPPC on AMD and not a new mechanism
based on CPPC. Can you clarify?

Also, if this is just enabling CPPC, shouldn't the driver be named something
like amd_cppc and not amd_pstate? This isn't using P-states.
 
> "amd-pstate" leverages the Linux kernel governors such as *schedutil*,
> *ondemand*, etc. to manage the performance hints which are provided by CPPC
> hardware functionality. The first version for amd-pstate is to support one
> of the Zen3 processors, and we will support more in future after we verify
> the hardware and SBIOS functionalities.
> 
> There are two types of hardware implementations for amd-pstate: one is full
> MSR support and another is shared memory support. It can use
> X86_FEATURE_AMD_CPPC_EXT feature flag to distinguish the different types.
>

Looking at the drivers/acpi code for CPPC I don't think this distinction
between MSRs and shared memory requires a feature flag. Shouldn't this be
handled properly in cpc_read|write if the ACPI tables are set up correctly?
Please correct me if I'm wrong.

This would also remove the need for the additional indirection pointed
out by Peter.

Could you also provide an explanation as to why a new CPPC driver is need
instead of updating the existing cppc_cpufreq driver.

-Nathan