[PATCH 0/5] cpufreq/amd-pstate: Reuse and refactor code

[PATCH 0/5] cpufreq/amd-pstate: Reuse and refactor code

[Dhananjay Ugwekar]

Use static calls to avoid frequent MSR/shared memory system checks.

Reuse existing functions amd_pstate_update_perf() and
amd_pstate_set_epp() instead of duplicating code.

Remove an unnecessary check for active mode in online and offline
functions.

Eliminate a redundant function amd_pstate_epp_offline().

Dhananjay Ugwekar (5):
  cpufreq/amd-pstate: Convert the amd_pstate_get/set_epp() to static
    calls
  cpufreq/amd-pstate: Move the invocation of amd_pstate_update_perf()
  cpufreq/amd-pstate: Refactor amd_pstate_epp_reenable() and
    amd_pstate_epp_offline()
  cpufreq/amd-pstate: Remove the cppc_state check in offline/online
    functions
  cpufreq/amd-pstate: Merge amd_pstate_epp_cpu_offline() and
    amd_pstate_epp_offline()

 drivers/cpufreq/amd-pstate.c | 151 +++++++++++++++++------------------
 1 file changed, 73 insertions(+), 78 deletions(-)

-- 
2.34.1

[PATCH 1/5] cpufreq/amd-pstate: Convert the amd_pstate_get/set_epp() to static calls

[Dhananjay Ugwekar]

MSR and shared memory based systems have different mechanisms to get and
set the epp value. Split those mechanisms into different functions and
assign them appropriately to the static calls at boot time. This eliminates
the need for the "if(cpu_feature_enabled(X86_FEATURE_CPPC))" checks at
runtime.

Also, propagate the error code from rdmsrl_on_cpu() and cppc_get_epp_perf()
to *_get_epp()'s caller, instead of returning -EIO unconditionally.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 92 +++++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 32 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index d7630bab2516..d391e8cafeca 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -180,26 +180,40 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
 static DEFINE_MUTEX(amd_pstate_limits_lock);
 static DEFINE_MUTEX(amd_pstate_driver_lock);
 
-static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
+static s16 msr_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
 {
 	u64 epp;
 	int ret;
 
-	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
-		if (!cppc_req_cached) {
-			epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
-					&cppc_req_cached);
-			if (epp)
-				return epp;
-		}
-		epp = (cppc_req_cached >> 24) & 0xFF;
-	} else {
-		ret = cppc_get_epp_perf(cpudata->cpu, &epp);
+	if (!cppc_req_cached) {
+		ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &cppc_req_cached);
 		if (ret < 0) {
 			pr_debug("Could not retrieve energy perf value (%d)\n", ret);
-			return -EIO;
+			return ret;
 		}
 	}
+	epp = (cppc_req_cached >> 24) & 0xFF;
+
+	return (s16)epp;
+}
+
+DEFINE_STATIC_CALL(amd_pstate_get_epp, msr_get_epp);
+
+static inline s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
+{
+	return static_call(amd_pstate_get_epp)(cpudata, cppc_req_cached);
+}
+
+static s16 shmem_get_epp(struct amd_cpudata *cpudata, u64 dummy)
+{
+	u64 epp;
+	int ret;
+
+	ret = cppc_get_epp_perf(cpudata->cpu, &epp);
+	if (ret < 0) {
+		pr_debug("Could not retrieve energy perf value (%d)\n", ret);
+		return ret;
+	}
 
 	return (s16)(epp & 0xff);
 }
@@ -253,33 +267,45 @@ static inline void amd_pstate_update_perf(struct amd_cpudata *cpudata,
 					    max_perf, fast_switch);
 }
 
-static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
+static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
 {
 	int ret;
-	struct cppc_perf_ctrls perf_ctrls;
-
-	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
-		u64 value = READ_ONCE(cpudata->cppc_req_cached);
 
-		value &= ~GENMASK_ULL(31, 24);
-		value |= (u64)epp << 24;
-		WRITE_ONCE(cpudata->cppc_req_cached, value);
+	u64 value = READ_ONCE(cpudata->cppc_req_cached);
 
-		ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
-		if (!ret)
-			cpudata->epp_cached = epp;
-	} else {
-		amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
-					     cpudata->max_limit_perf, false);
+	value &= ~GENMASK_ULL(31, 24);
+	value |= (u64)epp << 24;
+	WRITE_ONCE(cpudata->cppc_req_cached, value);
 
-		perf_ctrls.energy_perf = epp;
-		ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
-		if (ret) {
-			pr_debug("failed to set energy perf value (%d)\n", ret);
-			return ret;
-		}
+	ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+	if (!ret)
 		cpudata->epp_cached = epp;
+
+	return ret;
+}
+
+DEFINE_STATIC_CALL(amd_pstate_set_epp, msr_set_epp);
+
+static inline int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
+{
+	return static_call(amd_pstate_set_epp)(cpudata, epp);
+}
+
+static int shmem_set_epp(struct amd_cpudata *cpudata, u32 epp)
+{
+	int ret;
+	struct cppc_perf_ctrls perf_ctrls;
+
+	amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
+				     cpudata->max_limit_perf, false);
+
+	perf_ctrls.energy_perf = epp;
+	ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
+	if (ret) {
+		pr_debug("failed to set energy perf value (%d)\n", ret);
+		return ret;
 	}
+	cpudata->epp_cached = epp;
 
 	return ret;
 }
@@ -1867,6 +1893,8 @@ static int __init amd_pstate_init(void)
 		static_call_update(amd_pstate_cppc_enable, shmem_cppc_enable);
 		static_call_update(amd_pstate_init_perf, shmem_init_perf);
 		static_call_update(amd_pstate_update_perf, shmem_update_perf);
+		static_call_update(amd_pstate_get_epp, shmem_get_epp);
+		static_call_update(amd_pstate_set_epp, shmem_set_epp);
 	}
 
 	ret = amd_pstate_register_driver(cppc_state);
-- 
2.34.1

[PATCH 2/5] cpufreq/amd-pstate: Move the invocation of amd_pstate_update_perf()

[Dhananjay Ugwekar]

amd_pstate_update_perf() should not be a part of shmem_set_epp() function,
so move it to the amd_pstate_epp_update_limit() function, where it is needed.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index d391e8cafeca..a1b2393cef22 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -296,9 +296,6 @@ static int shmem_set_epp(struct amd_cpudata *cpudata, u32 epp)
 	int ret;
 	struct cppc_perf_ctrls perf_ctrls;
 
-	amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
-				     cpudata->max_limit_perf, false);
-
 	perf_ctrls.energy_perf = epp;
 	ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
 	if (ret) {
@@ -1598,6 +1595,10 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 		epp = 0;
 
 	WRITE_ONCE(cpudata->cppc_req_cached, value);
+
+	amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
+			       cpudata->max_limit_perf, false);
+
 	return amd_pstate_set_epp(cpudata, epp);
 }
 
-- 
2.34.1

[PATCH 3/5] cpufreq/amd-pstate: Refactor amd_pstate_epp_reenable() and amd_pstate_epp_offline()

[Dhananjay Ugwekar]

Replace similar code chunks with amd_pstate_update_perf() and
amd_pstate_set_epp() function calls.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 36 +++++++-----------------------------
 1 file changed, 7 insertions(+), 29 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index a1b2393cef22..a38be7727c9d 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1630,25 +1630,17 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
 
 static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
 {
-	struct cppc_perf_ctrls perf_ctrls;
-	u64 value, max_perf;
+	u64 max_perf;
 	int ret;
 
 	ret = amd_pstate_cppc_enable(true);
 	if (ret)
 		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
 
-	value = READ_ONCE(cpudata->cppc_req_cached);
 	max_perf = READ_ONCE(cpudata->highest_perf);
 
-	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
-		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
-	} else {
-		perf_ctrls.max_perf = max_perf;
-		cppc_set_perf(cpudata->cpu, &perf_ctrls);
-		perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(cpudata->epp_cached);
-		cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
-	}
+	amd_pstate_update_perf(cpudata, 0, 0, max_perf, false);
+	amd_pstate_set_epp(cpudata, cpudata->epp_cached);
 }
 
 static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
@@ -1668,7 +1660,6 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
 static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	struct cppc_perf_ctrls perf_ctrls;
 	int min_perf;
 	u64 value;
 
@@ -1676,23 +1667,10 @@ static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
 	value = READ_ONCE(cpudata->cppc_req_cached);
 
 	mutex_lock(&amd_pstate_limits_lock);
-	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
-		cpudata->epp_policy = CPUFREQ_POLICY_UNKNOWN;
-
-		/* Set max perf same as min perf */
-		value &= ~AMD_CPPC_MAX_PERF(~0L);
-		value |= AMD_CPPC_MAX_PERF(min_perf);
-		value &= ~AMD_CPPC_MIN_PERF(~0L);
-		value |= AMD_CPPC_MIN_PERF(min_perf);
-		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
-	} else {
-		perf_ctrls.desired_perf = 0;
-		perf_ctrls.min_perf = min_perf;
-		perf_ctrls.max_perf = min_perf;
-		cppc_set_perf(cpudata->cpu, &perf_ctrls);
-		perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(HWP_EPP_BALANCE_POWERSAVE);
-		cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
-	}
+
+	amd_pstate_update_perf(cpudata, min_perf, 0, min_perf, false);
+	amd_pstate_set_epp(cpudata, AMD_CPPC_EPP_BALANCE_POWERSAVE);
+
 	mutex_unlock(&amd_pstate_limits_lock);
 }
 
-- 
2.34.1

[PATCH 4/5] cpufreq/amd-pstate: Remove the cppc_state check in offline/online functions

[Dhananjay Ugwekar]

Only amd_pstate_epp driver (i.e. cppc_state = ACTIVE) enters the
amd_pstate_epp_offline() and amd_pstate_epp_cpu_online() functions,
so remove the unnecessary if condition checking if cppc_state is
equal to AMD_PSTATE_ACTIVE.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index a38be7727c9d..08a10ef7aa5b 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1649,10 +1649,8 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
 
 	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
 
-	if (cppc_state == AMD_PSTATE_ACTIVE) {
-		amd_pstate_epp_reenable(cpudata);
-		cpudata->suspended = false;
-	}
+	amd_pstate_epp_reenable(cpudata);
+	cpudata->suspended = false;
 
 	return 0;
 }
@@ -1683,8 +1681,7 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
 	if (cpudata->suspended)
 		return 0;
 
-	if (cppc_state == AMD_PSTATE_ACTIVE)
-		amd_pstate_epp_offline(policy);
+	amd_pstate_epp_offline(policy);
 
 	return 0;
 }
-- 
2.34.1

[PATCH 5/5] cpufreq/amd-pstate: Merge amd_pstate_epp_cpu_offline() and amd_pstate_epp_offline()

[Dhananjay Ugwekar]

amd_pstate_epp_offline() is only called from within
amd_pstate_epp_cpu_offline() and doesn't make much sense to have it at all.
Hence, remove it.

Also remove the unncessary debug print in the offline path while at it.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 08a10ef7aa5b..298065912976 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1655,12 +1655,15 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
 	return 0;
 }
 
-static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
+static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
 	int min_perf;
 	u64 value;
 
+	if (cpudata->suspended)
+		return 0;
+
 	min_perf = READ_ONCE(cpudata->lowest_perf);
 	value = READ_ONCE(cpudata->cppc_req_cached);
 
@@ -1670,18 +1673,6 @@ static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
 	amd_pstate_set_epp(cpudata, AMD_CPPC_EPP_BALANCE_POWERSAVE);
 
 	mutex_unlock(&amd_pstate_limits_lock);
-}
-
-static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
-{
-	struct amd_cpudata *cpudata = policy->driver_data;
-
-	pr_debug("AMD CPU Core %d going offline\n", cpudata->cpu);
-
-	if (cpudata->suspended)
-		return 0;
-
-	amd_pstate_epp_offline(policy);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 0/5] cpufreq/amd-pstate: Reuse and refactor code

[Mario Limonciello]

On 12/4/2024 08:48, Dhananjay Ugwekar wrote:
> Use static calls to avoid frequent MSR/shared memory system checks.
> 
> Reuse existing functions amd_pstate_update_perf() and
> amd_pstate_set_epp() instead of duplicating code.
> 
> Remove an unnecessary check for active mode in online and offline
> functions.
> 
> Eliminate a redundant function amd_pstate_epp_offline().
> 
> Dhananjay Ugwekar (5):
>    cpufreq/amd-pstate: Convert the amd_pstate_get/set_epp() to static
>      calls
>    cpufreq/amd-pstate: Move the invocation of amd_pstate_update_perf()
>    cpufreq/amd-pstate: Refactor amd_pstate_epp_reenable() and
>      amd_pstate_epp_offline()
>    cpufreq/amd-pstate: Remove the cppc_state check in offline/online
>      functions
>    cpufreq/amd-pstate: Merge amd_pstate_epp_cpu_offline() and
>      amd_pstate_epp_offline()
> 
>   drivers/cpufreq/amd-pstate.c | 151 +++++++++++++++++------------------
>   1 file changed, 73 insertions(+), 78 deletions(-)
> 

For the patches not already reviewed:

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll add this to superm1/bleeding-edge.

I have another series that I will build on top of it as well.

Re: [PATCH 3/5] cpufreq/amd-pstate: Refactor amd_pstate_epp_reenable() and amd_pstate_epp_offline()

[kernel test robot]

Hi Dhananjay,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge linus/master v6.13-rc1 next-20241204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dhananjay-Ugwekar/cpufreq-amd-pstate-Convert-the-amd_pstate_get-set_epp-to-static-calls/20241204-225537
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20241204144842.164178-4-Dhananjay.Ugwekar%40amd.com
patch subject: [PATCH 3/5] cpufreq/amd-pstate: Refactor amd_pstate_epp_reenable() and amd_pstate_epp_offline()
config: x86_64-buildonly-randconfig-006-20241205 (https://download.01.org/0day-ci/archive/20241205/202412050615.ObPzrf34-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241205/202412050615.ObPzrf34-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412050615.ObPzrf34-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/cpufreq/amd-pstate.c: In function 'amd_pstate_epp_offline':
>> drivers/cpufreq/amd-pstate.c:1664:13: warning: variable 'value' set but not used [-Wunused-but-set-variable]
    1664 |         u64 value;
         |             ^~~~~


vim +/value +1664 drivers/cpufreq/amd-pstate.c

d4da12f8033a123 Perry Yuan        2023-01-31  1659  
d4da12f8033a123 Perry Yuan        2023-01-31  1660  static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
d4da12f8033a123 Perry Yuan        2023-01-31  1661  {
d4da12f8033a123 Perry Yuan        2023-01-31  1662  	struct amd_cpudata *cpudata = policy->driver_data;
d4da12f8033a123 Perry Yuan        2023-01-31  1663  	int min_perf;
d4da12f8033a123 Perry Yuan        2023-01-31 @1664  	u64 value;
d4da12f8033a123 Perry Yuan        2023-01-31  1665  
d4da12f8033a123 Perry Yuan        2023-01-31  1666  	min_perf = READ_ONCE(cpudata->lowest_perf);
d4da12f8033a123 Perry Yuan        2023-01-31  1667  	value = READ_ONCE(cpudata->cppc_req_cached);
d4da12f8033a123 Perry Yuan        2023-01-31  1668  
d4da12f8033a123 Perry Yuan        2023-01-31  1669  	mutex_lock(&amd_pstate_limits_lock);
d4da12f8033a123 Perry Yuan        2023-01-31  1670  
33cc0b550fa3510 Dhananjay Ugwekar 2024-12-04  1671  	amd_pstate_update_perf(cpudata, min_perf, 0, min_perf, false);
33cc0b550fa3510 Dhananjay Ugwekar 2024-12-04  1672  	amd_pstate_set_epp(cpudata, AMD_CPPC_EPP_BALANCE_POWERSAVE);
33cc0b550fa3510 Dhananjay Ugwekar 2024-12-04  1673  
d4da12f8033a123 Perry Yuan        2023-01-31  1674  	mutex_unlock(&amd_pstate_limits_lock);
d4da12f8033a123 Perry Yuan        2023-01-31  1675  }
d4da12f8033a123 Perry Yuan        2023-01-31  1676  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/5] cpufreq/amd-pstate: Reuse and refactor code

[Dhananjay Ugwekar]

Hello Mario,

On 12/4/2024 10:37 PM, Mario Limonciello wrote:
> On 12/4/2024 08:48, Dhananjay Ugwekar wrote:
>> Use static calls to avoid frequent MSR/shared memory system checks.
>>
>> Reuse existing functions amd_pstate_update_perf() and
>> amd_pstate_set_epp() instead of duplicating code.
>>
>> Remove an unnecessary check for active mode in online and offline
>> functions.
>>
>> Eliminate a redundant function amd_pstate_epp_offline().
>>
>> Dhananjay Ugwekar (5):
>>    cpufreq/amd-pstate: Convert the amd_pstate_get/set_epp() to static
>>      calls
>>    cpufreq/amd-pstate: Move the invocation of amd_pstate_update_perf()
>>    cpufreq/amd-pstate: Refactor amd_pstate_epp_reenable() and
>>      amd_pstate_epp_offline()
>>    cpufreq/amd-pstate: Remove the cppc_state check in offline/online
>>      functions
>>    cpufreq/amd-pstate: Merge amd_pstate_epp_cpu_offline() and
>>      amd_pstate_epp_offline()
>>
>>   drivers/cpufreq/amd-pstate.c | 151 +++++++++++++++++------------------
>>   1 file changed, 73 insertions(+), 78 deletions(-)
>>
> 
> For the patches not already reviewed:
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Thanks! 

> 
> I'll add this to superm1/bleeding-edge.

There is a small warning in the 3rd patch, how do you suggest we fix that, 
should I post a new version or you'll modify the patch at your end?

   drivers/cpufreq/amd-pstate.c: In function 'amd_pstate_epp_offline':
>> drivers/cpufreq/amd-pstate.c:1664:13: warning: variable 'value' set but not used [-Wunused-but-set-variable]
    1664 |         u64 value;
         |             ^~~~~

Regards,
Dhananjay

> 
> I have another series that I will build on top of it as well.

Re: [PATCH 0/5] cpufreq/amd-pstate: Reuse and refactor code

[Mario Limonciello]

On 12/4/2024 22:29, Dhananjay Ugwekar wrote:
> Hello Mario,
> 
> On 12/4/2024 10:37 PM, Mario Limonciello wrote:
>> On 12/4/2024 08:48, Dhananjay Ugwekar wrote:
>>> Use static calls to avoid frequent MSR/shared memory system checks.
>>>
>>> Reuse existing functions amd_pstate_update_perf() and
>>> amd_pstate_set_epp() instead of duplicating code.
>>>
>>> Remove an unnecessary check for active mode in online and offline
>>> functions.
>>>
>>> Eliminate a redundant function amd_pstate_epp_offline().
>>>
>>> Dhananjay Ugwekar (5):
>>>     cpufreq/amd-pstate: Convert the amd_pstate_get/set_epp() to static
>>>       calls
>>>     cpufreq/amd-pstate: Move the invocation of amd_pstate_update_perf()
>>>     cpufreq/amd-pstate: Refactor amd_pstate_epp_reenable() and
>>>       amd_pstate_epp_offline()
>>>     cpufreq/amd-pstate: Remove the cppc_state check in offline/online
>>>       functions
>>>     cpufreq/amd-pstate: Merge amd_pstate_epp_cpu_offline() and
>>>       amd_pstate_epp_offline()
>>>
>>>    drivers/cpufreq/amd-pstate.c | 151 +++++++++++++++++------------------
>>>    1 file changed, 73 insertions(+), 78 deletions(-)
>>>
>>
>> For the patches not already reviewed:
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> Thanks!
> 
>>
>> I'll add this to superm1/bleeding-edge.
> 
> There is a small warning in the 3rd patch, how do you suggest we fix that,
> should I post a new version or you'll modify the patch at your end?
> 
>     drivers/cpufreq/amd-pstate.c: In function 'amd_pstate_epp_offline':
>>> drivers/cpufreq/amd-pstate.c:1664:13: warning: variable 'value' set but not used [-Wunused-but-set-variable]
>      1664 |         u64 value;
>           |             ^~~~~
> 
> Regards,
> Dhananjay
> 

Don't worry, it's a one line change.  I'll take care of it when I 
migrate it to superm1/linux-next.

Re: [PATCH 1/5] cpufreq/amd-pstate: Convert the amd_pstate_get/set_epp() to static calls

[Gautham R. Shenoy]

Hello Dhananjay,

On Wed, Dec 04, 2024 at 02:48:38PM +0000, Dhananjay Ugwekar wrote:
> MSR and shared memory based systems have different mechanisms to get and
> set the epp value. Split those mechanisms into different functions and
> assign them appropriately to the static calls at boot time. This eliminates
> the need for the "if(cpu_feature_enabled(X86_FEATURE_CPPC))" checks at
> runtime.
> 
> Also, propagate the error code from rdmsrl_on_cpu() and cppc_get_epp_perf()
> to *_get_epp()'s caller, instead of returning -EIO unconditionally.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>

Thanks for taking this up.
This patch looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  drivers/cpufreq/amd-pstate.c | 92 +++++++++++++++++++++++-------------
>  1 file changed, 60 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index d7630bab2516..d391e8cafeca 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -180,26 +180,40 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
>  static DEFINE_MUTEX(amd_pstate_limits_lock);
>  static DEFINE_MUTEX(amd_pstate_driver_lock);
>  
> -static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
> +static s16 msr_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
>  {
>  	u64 epp;
>  	int ret;
>  
> -	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -		if (!cppc_req_cached) {
> -			epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
> -					&cppc_req_cached);
> -			if (epp)
> -				return epp;
> -		}
> -		epp = (cppc_req_cached >> 24) & 0xFF;
> -	} else {
> -		ret = cppc_get_epp_perf(cpudata->cpu, &epp);
> +	if (!cppc_req_cached) {
> +		ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &cppc_req_cached);
>  		if (ret < 0) {
>  			pr_debug("Could not retrieve energy perf value (%d)\n", ret);
> -			return -EIO;
> +			return ret;
>  		}
>  	}
> +	epp = (cppc_req_cached >> 24) & 0xFF;
> +
> +	return (s16)epp;
> +}
> +
> +DEFINE_STATIC_CALL(amd_pstate_get_epp, msr_get_epp);
> +
> +static inline s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
> +{
> +	return static_call(amd_pstate_get_epp)(cpudata, cppc_req_cached);
> +}
> +
> +static s16 shmem_get_epp(struct amd_cpudata *cpudata, u64 dummy)
> +{
> +	u64 epp;
> +	int ret;
> +
> +	ret = cppc_get_epp_perf(cpudata->cpu, &epp);
> +	if (ret < 0) {
> +		pr_debug("Could not retrieve energy perf value (%d)\n", ret);
> +		return ret;
> +	}
>  
>  	return (s16)(epp & 0xff);
>  }
> @@ -253,33 +267,45 @@ static inline void amd_pstate_update_perf(struct amd_cpudata *cpudata,
>  					    max_perf, fast_switch);
>  }
>  
> -static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
>  {
>  	int ret;
> -	struct cppc_perf_ctrls perf_ctrls;
> -
> -	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -		u64 value = READ_ONCE(cpudata->cppc_req_cached);
>  
> -		value &= ~GENMASK_ULL(31, 24);
> -		value |= (u64)epp << 24;
> -		WRITE_ONCE(cpudata->cppc_req_cached, value);
> +	u64 value = READ_ONCE(cpudata->cppc_req_cached);
>  
> -		ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> -		if (!ret)
> -			cpudata->epp_cached = epp;
> -	} else {
> -		amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
> -					     cpudata->max_limit_perf, false);
> +	value &= ~GENMASK_ULL(31, 24);
> +	value |= (u64)epp << 24;
> +	WRITE_ONCE(cpudata->cppc_req_cached, value);
>  
> -		perf_ctrls.energy_perf = epp;
> -		ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
> -		if (ret) {
> -			pr_debug("failed to set energy perf value (%d)\n", ret);
> -			return ret;
> -		}
> +	ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> +	if (!ret)
>  		cpudata->epp_cached = epp;
> +
> +	return ret;
> +}
> +
> +DEFINE_STATIC_CALL(amd_pstate_set_epp, msr_set_epp);
> +
> +static inline int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +{
> +	return static_call(amd_pstate_set_epp)(cpudata, epp);
> +}
> +
> +static int shmem_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +{
> +	int ret;
> +	struct cppc_perf_ctrls perf_ctrls;
> +
> +	amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
> +				     cpudata->max_limit_perf, false);
> +
> +	perf_ctrls.energy_perf = epp;
> +	ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
> +	if (ret) {
> +		pr_debug("failed to set energy perf value (%d)\n", ret);
> +		return ret;
>  	}
> +	cpudata->epp_cached = epp;
>  
>  	return ret;
>  }
> @@ -1867,6 +1893,8 @@ static int __init amd_pstate_init(void)
>  		static_call_update(amd_pstate_cppc_enable, shmem_cppc_enable);
>  		static_call_update(amd_pstate_init_perf, shmem_init_perf);
>  		static_call_update(amd_pstate_update_perf, shmem_update_perf);
> +		static_call_update(amd_pstate_get_epp, shmem_get_epp);
> +		static_call_update(amd_pstate_set_epp, shmem_set_epp);
>  	}
>  
>  	ret = amd_pstate_register_driver(cppc_state);
> -- 
> 2.34.1
> 

Re: [PATCH 2/5] cpufreq/amd-pstate: Move the invocation of amd_pstate_update_perf()

[Gautham R. Shenoy]

On Wed, Dec 04, 2024 at 02:48:39PM +0000, Dhananjay Ugwekar wrote:
> amd_pstate_update_perf() should not be a part of shmem_set_epp() function,
> so move it to the amd_pstate_epp_update_limit() function, where it is needed.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>


Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  drivers/cpufreq/amd-pstate.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index d391e8cafeca..a1b2393cef22 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -296,9 +296,6 @@ static int shmem_set_epp(struct amd_cpudata *cpudata, u32 epp)
>  	int ret;
>  	struct cppc_perf_ctrls perf_ctrls;
>  
> -	amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
> -				     cpudata->max_limit_perf, false);
> -
>  	perf_ctrls.energy_perf = epp;
>  	ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
>  	if (ret) {
> @@ -1598,6 +1595,10 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>  		epp = 0;
>  
>  	WRITE_ONCE(cpudata->cppc_req_cached, value);
> +
> +	amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
> +			       cpudata->max_limit_perf, false);
> +
>  	return amd_pstate_set_epp(cpudata, epp);
>  }
>  
> -- 
> 2.34.1
> 

Re: [PATCH 3/5] cpufreq/amd-pstate: Refactor amd_pstate_epp_reenable() and amd_pstate_epp_offline()

[Gautham R. Shenoy]

On Wed, Dec 04, 2024 at 02:48:40PM +0000, Dhananjay Ugwekar wrote:
> Replace similar code chunks with amd_pstate_update_perf() and
> amd_pstate_set_epp() function calls.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 36 +++++++-----------------------------
>  1 file changed, 7 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index a1b2393cef22..a38be7727c9d 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1630,25 +1630,17 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>  
>  static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
>  {
> -	struct cppc_perf_ctrls perf_ctrls;
> -	u64 value, max_perf;
> +	u64 max_perf;
>  	int ret;
>  
>  	ret = amd_pstate_cppc_enable(true);
>  	if (ret)
>  		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
>  
> -	value = READ_ONCE(cpudata->cppc_req_cached);
>  	max_perf = READ_ONCE(cpudata->highest_perf);
>  
> -	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> -	} else {
> -		perf_ctrls.max_perf = max_perf;
> -		cppc_set_perf(cpudata->cpu, &perf_ctrls);
> -		perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(cpudata->epp_cached);
> -		cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
> -	}
> +	amd_pstate_update_perf(cpudata, 0, 0, max_perf, false);
> +	amd_pstate_set_epp(cpudata, cpudata->epp_cached);

This will cause two MSR writes on MSR based systems.

But then, we don't expect the amd_pstate_epp_reenable() and
amd_pstate_epp_offline() to be called often. Hence it should be ok.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.




>  }
>  
>  static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
> @@ -1668,7 +1660,6 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
>  static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	struct cppc_perf_ctrls perf_ctrls;
>  	int min_perf;
>  	u64 value;
>  
> @@ -1676,23 +1667,10 @@ static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
>  	value = READ_ONCE(cpudata->cppc_req_cached);
>  
>  	mutex_lock(&amd_pstate_limits_lock);
> -	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -		cpudata->epp_policy = CPUFREQ_POLICY_UNKNOWN;
> -
> -		/* Set max perf same as min perf */
> -		value &= ~AMD_CPPC_MAX_PERF(~0L);
> -		value |= AMD_CPPC_MAX_PERF(min_perf);
> -		value &= ~AMD_CPPC_MIN_PERF(~0L);
> -		value |= AMD_CPPC_MIN_PERF(min_perf);
> -		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> -	} else {
> -		perf_ctrls.desired_perf = 0;
> -		perf_ctrls.min_perf = min_perf;
> -		perf_ctrls.max_perf = min_perf;
> -		cppc_set_perf(cpudata->cpu, &perf_ctrls);
> -		perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(HWP_EPP_BALANCE_POWERSAVE);
> -		cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
> -	}
> +
> +	amd_pstate_update_perf(cpudata, min_perf, 0, min_perf, false);
> +	amd_pstate_set_epp(cpudata, AMD_CPPC_EPP_BALANCE_POWERSAVE);
> +
>  	mutex_unlock(&amd_pstate_limits_lock);
>  }
>  
> -- 
> 2.34.1
>