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

[K Prateek Nayak <kprateek.nayak@amd.com>]

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] = {
            [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?

> +

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

> +}
-- 
Thanks and Regards,
Prateek