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

[Mario Limonciello]

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.

> > +}