Re: [PATCH v2 1/3] cpufreq/amd-pstate: Add per SoC and per core type EPP tuning values

[Mario Limonciello <superm1@kernel.org>]

On 9/8/26 01:19, K Prateek Nayak wrote:
> Hello Mario,
> 
> On 9/8/2026 9:49 AM, Mario Limonciello (AMD) wrote:
>> +/*
>> + * The numeric EPP value programmed for each named preference. First dimension
>> + * is CPU type (TOPO_CPU_TYPE_ANY for non-hybrid, TOPO_CPU_TYPE_PERFORMANCE/
>> + * EFFICIENCY/LOW_POWER for hybrid). The initializer holds the legacy values
>> + * used as the fallback on any platform not listed in amd_pstate_epp_soc_ids[];
>> + * amd_pstate_init_epp_values() overwrites slots at boot when the running SoC
>> + * has a per-SoC (and potentially per-CPU-type) override.
>> + */
>> +static u8 epp_values[][EPP_INDEX_MAX] = {
>> +	[TOPO_CPU_TYPE_ANY] = {
>> +		[EPP_INDEX_DEFAULT] = 0,
>> +		[EPP_INDEX_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_PERFORMANCE,
>> +		[EPP_INDEX_BALANCE_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_BALANCE_PERFORMANCE,
>> +		[EPP_INDEX_BALANCE_POWERSAVE] = AMD_CPPC_EPP_LEGACY_BALANCE_POWERSAVE,
>> +		[EPP_INDEX_POWERSAVE] = AMD_CPPC_EPP_LEGACY_POWERSAVE,
>> +	},
>> +	[TOPO_CPU_TYPE_PERFORMANCE] = {
>> +		[EPP_INDEX_DEFAULT] = 0,
>> +		[EPP_INDEX_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_PERFORMANCE,
>> +		[EPP_INDEX_BALANCE_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_BALANCE_PERFORMANCE,
>> +		[EPP_INDEX_BALANCE_POWERSAVE] = AMD_CPPC_EPP_LEGACY_BALANCE_POWERSAVE,
>> +		[EPP_INDEX_POWERSAVE] = AMD_CPPC_EPP_LEGACY_POWERSAVE,
>> +	},
>> +	[TOPO_CPU_TYPE_EFFICIENCY] = {
>> +		[EPP_INDEX_DEFAULT] = 0,
>> +		[EPP_INDEX_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_PERFORMANCE,
>> +		[EPP_INDEX_BALANCE_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_BALANCE_PERFORMANCE,
>> +		[EPP_INDEX_BALANCE_POWERSAVE] = AMD_CPPC_EPP_LEGACY_BALANCE_POWERSAVE,
>> +		[EPP_INDEX_POWERSAVE] = AMD_CPPC_EPP_LEGACY_POWERSAVE,
>> +	},
>> +	[TOPO_CPU_TYPE_LOW_POWER] = {
>> +		[EPP_INDEX_DEFAULT] = 0,
>> +		[EPP_INDEX_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_PERFORMANCE,
>> +		[EPP_INDEX_BALANCE_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_BALANCE_PERFORMANCE,
>> +		[EPP_INDEX_BALANCE_POWERSAVE] = AMD_CPPC_EPP_LEGACY_BALANCE_POWERSAVE,
>> +		[EPP_INDEX_POWERSAVE] = AMD_CPPC_EPP_LEGACY_POWERSAVE,
>> +	},
>> +};
> 
> If my eyes aren't decieveing me, all the indices are initialized to same
> value so we can perhaps do:
> 
>      static u8 epp_values[][EPP_INDEX_MAX] = {
>          /*
>           * Initialize all CPU types to legacy defaults.
>           * amd_pstate_init_epp_values() will fix these up
>           * based on the platform during boot.
> 	 */
>          [TOPO_CPU_TYPE_ANY ... TOPO_CPU_TYPE_LOW_POWER] = {

Woah!  I didn't realize that kind of initalizer was valid!

Yeah will adjust to this.

>              [EPP_INDEX_DEFAULT] = 0,
>              [EPP_INDEX_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_PERFORMANCE,
>              [EPP_INDEX_BALANCE_PERFORMANCE] = AMD_CPPC_EPP_LEGACY_BALANCE_PERFORMANCE,
>              [EPP_INDEX_BALANCE_POWERSAVE] = AMD_CPPC_EPP_LEGACY_BALANCE_POWERSAVE,
>              [EPP_INDEX_POWERSAVE] = AMD_CPPC_EPP_LEGACY_POWERSAVE,
>          },
>      };
> 
>> +static_assert(ARRAY_SIZE(epp_values) == TOPO_CPU_TYPE_LOW_POWER + 1,
>> +	      "epp_values must have entries for all CPU types up to TOPO_CPU_TYPE_LOW_POWER");
>> +
>> +/*
>> + * Get the EPP value row for a given CPU, accounting for hybrid CPU types.
>> + * Non-hybrid systems use TOPO_CPU_TYPE_ANY; hybrid systems use the CPU's
>> + * actual type (PERFORMANCE/EFFICIENCY/LOW_POWER).
>> + */
>> +static inline u8 *amd_pstate_cpu_epp_values(int cpu)
>> +{
>> +	enum x86_topology_cpu_type type = cpu_data(cpu).topo.cpu_type;
> 
> Can we cache this in cpudata to avoid an additional dereference?

OK.

> 
>> +
> 
> I would prefer a:
> 
>      if (!cpu_feature_enabled(X86_FEATURE_AMD_HTR_CORES))
>          return epp_values[TOPO_CPU_TYPE_ANY];
> 
> here to avoid the branches for non-heterogenoeus platforms.
> 
>> +	if (type == TOPO_CPU_TYPE_PERFORMANCE ||
>> +	    type == TOPO_CPU_TYPE_EFFICIENCY ||
>> +	    type == TOPO_CPU_TYPE_LOW_POWER)
>> +		return epp_values[type];
>> +
>> +	return epp_values[TOPO_CPU_TYPE_ANY];
> 
> A switch case might look prettier ;-)

Sure will change it.

> 
>> +}