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

[Mario Limonciello <mario.limonciello@amd.com>]

On 6/7/26 23:21, K Prateek Nayak wrote:
> Hello Marco,
> 
> I don't mind the changes, except that it can be broken down into
> two patches as Mario suggested for easier backporting :-)

Totally agree. All the technical stuff I'm happy with too, just split it up.

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