Re: [PATCH v4 1/1] cpufreq/amd-pstate: Fix EPP initialization for shared memory systems

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

Hello Marco,

I don't mind the changes, except that it can be broken down into
two patches as Mario suggested for easier backporting :-)

Some notes below ...

On 6/6/2026 12:27 PM, Marco Scardovi wrote:
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 8d55e2be825b..a35cf126e335 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -199,7 +199,7 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
>  
>  static DEFINE_MUTEX(amd_pstate_driver_lock);
>  
> -static u8 msr_get_epp(struct amd_cpudata *cpudata)
> +static int msr_get_epp(struct amd_cpudata *cpudata)
>  {
>  	u64 value;
>  	int ret;
> @@ -215,12 +215,12 @@ static u8 msr_get_epp(struct amd_cpudata *cpudata)
>  
>  DEFINE_STATIC_CALL(amd_pstate_get_epp, msr_get_epp);
>  
> -static inline s16 amd_pstate_get_epp(struct amd_cpudata *cpudata)
> +static inline int amd_pstate_get_epp(struct amd_cpudata *cpudata)

The change to return type and the amd_pstate_epp_cpu_init() bits
should be Patch 1/2 with:

Fixes: 555bbe67a622 ("cpufreq/amd-pstate: Convert all perf values to u8")

and commit message stating the necessary error handling that is
required if amd_pstate_get_epp() fails.

>  {
>  	return static_call(amd_pstate_get_epp)(cpudata);
>  }
>  
> -static u8 shmem_get_epp(struct amd_cpudata *cpudata)
> +static int shmem_get_epp(struct amd_cpudata *cpudata)
>  {
>  	u64 epp;
>  	int ret;
> @@ -525,10 +525,6 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
>  	perf.lowest_perf = cppc_perf.lowest_perf;
>  	WRITE_ONCE(cpudata->perf, perf);
>  	WRITE_ONCE(cpudata->prefcore_ranking, cppc_perf.highest_perf);
> -
> -	if (cppc_state == AMD_PSTATE_ACTIVE)
> -		return 0;
> -

And this should be Patch 2/2 with:

Fixes: 2dd6d0ebf740 ("cpufreq: amd-pstate: Add guided autonomous mode")

and a commit message that reads it is necessary for AMD_PSTATE_ACTIVE mode
on shared memory system to toggle on AUTO_SEL_ENABLE based on the
ACPI spec for CPPC v2 and below.

>  	ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel);
>  	if (ret) {
>  		pr_warn("failed to get auto_sel, ret: %d\n", ret);
> @@ -1877,6 +1873,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	struct amd_cpudata *cpudata;
>  	union perf_cached perf;
>  	struct device *dev;
> +	int default_epp;
>  	int ret;
>  
>  	/*
> @@ -1926,6 +1923,14 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  
>  	policy->boost_supported = READ_ONCE(cpudata->boost_supported);
>  
> +	/* Cache the firmware programmed EPP */
> +	default_epp = amd_pstate_get_epp(cpudata);
> +	if (default_epp < 0) {
> +		ret = default_epp;
> +		goto free_cpudata1;
> +	}
> +	FIELD_MODIFY(AMD_CPPC_EPP_PERF_MASK, &cpudata->cppc_req_cached, default_epp);

I would actually prefer this "cppc_req_cached" change to be a third
patch that says caching the initial EPP saves on an unnecessary
reprogramming later when the EPP is first set but I don't mind it
being part of Patch 1 with a small note in the commit message.

> +
>  	/*
>  	 * Set the policy to provide a valid fallback value in case
>  	 * the default cpufreq governor is neither powersave nor performance.
> @@ -1933,7 +1938,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	if (amd_pstate_acpi_pm_profile_server() ||
>  	    amd_pstate_acpi_pm_profile_undefined()) {
>  		policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> -		cpudata->epp_default_ac = cpudata->epp_default_dc = amd_pstate_get_epp(cpudata);
> +		cpudata->epp_default_ac = cpudata->epp_default_dc = default_epp;
>  		cpudata->current_profile = PLATFORM_PROFILE_PERFORMANCE;
>  	} else {
>  		policy->policy = CPUFREQ_POLICY_POWERSAVE;

-- 
Thanks and Regards,
Prateek