amd-pstate cleanups

From: Mario Limonciello <[email protected]>

Use the perf_to_freq helpers to calculate this on the fly. As the members are no longer cached add an extra check into amd_pstate_update_min_max_limit().

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
v4:
* Avoid some unnecessary changes to amd_pstate_init_freq()
* Add tag
v3:
* Fix calc error for min_freq
v2:
* Keep cached limits

---
 drivers/cpufreq/amd-pstate-ut.c | 14 +++++------
 drivers/cpufreq/amd-pstate.c    | 43 +++++++++------------------------
 drivers/cpufreq/amd-pstate.h    |  9 ++-----
 3 files changed, 20 insertions(+), 46 deletions(-)

drivers/cpufreq/amd-pstate.c +2 -1

 	struct amd_cpudata *cpudata = policy->driver_data;
 	u8 epp;
 
-	amd_pstate_update_min_max_limit(policy);
+	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
+		amd_pstate_update_min_max_limit(policy);
 
 	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
 		epp = 0;

From: Mario Limonciello <[email protected]>

By storing perf values in a union all the writes and reads can be done atomically, removing the need for some concurrency protections.

While making this change, also drop the cached frequency values, using inline helpers to calculate them on demand from perf value.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
v4:
* Rebase on earlier changes
v3:
* Pick up tag
v2:
* cache perf variable in unit tests
* Drop unnecessary check from amd_pstate_update_min_max_limit()
* Consistency with READ_ONCE()
* Drop unneeded policy checks
* add kdoc

---
 drivers/cpufreq/amd-pstate-ut.c |  18 +--
 drivers/cpufreq/amd-pstate.c    | 204 ++++++++++++++++++--------------
 drivers/cpufreq/amd-pstate.h    |  49 +++++---
 3 files changed, 155 insertions(+), 116 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +10 -8

index 445278cf40b61..ba3e06f349c6d 100644
@@ -129,6 +129,7 @@ static void amd_pstate_ut_check_perf(u32 index)
 	struct cppc_perf_caps cppc_perf;
 	struct cpufreq_policy *policy = NULL;
 	struct amd_cpudata *cpudata = NULL;
+	union perf_cached cur_perf;
 
 	for_each_possible_cpu(cpu) {
 		policy = cpufreq_cpu_get(cpu);

@@ -162,19 +163,20 @@ static void amd_pstate_ut_check_perf(u32 index)
 			lowest_perf = AMD_CPPC_LOWEST_PERF(cap1);
 		}
 
-		if (highest_perf != READ_ONCE(cpudata->highest_perf) && !cpudata->hw_prefcore) {
+		cur_perf = READ_ONCE(cpudata->perf);
+		if (highest_perf != cur_perf.highest_perf && !cpudata->hw_prefcore) {
 			pr_err("%s cpu%d highest=%d %d highest perf doesn't match\n",
-				__func__, cpu, highest_perf, cpudata->highest_perf);
+				__func__, cpu, highest_perf, cpudata->perf.highest_perf);
 			goto skip_test;
 		}
-		if ((nominal_perf != READ_ONCE(cpudata->nominal_perf)) ||
-			(lowest_nonlinear_perf != READ_ONCE(cpudata->lowest_nonlinear_perf)) ||
-			(lowest_perf != READ_ONCE(cpudata->lowest_perf))) {
+		if (nominal_perf != cur_perf.nominal_perf ||
+		   (lowest_nonlinear_perf != cur_perf.lowest_nonlinear_perf) ||
+		   (lowest_perf != cur_perf.lowest_perf)) {
 			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
 			pr_err("%s cpu%d nominal=%d %d lowest_nonlinear=%d %d lowest=%d %d, they should be equal!\n",
-				__func__, cpu, nominal_perf, cpudata->nominal_perf,
-				lowest_nonlinear_perf, cpudata->lowest_nonlinear_perf,
-				lowest_perf, cpudata->lowest_perf);
+				__func__, cpu, nominal_perf, cpudata->perf.nominal_perf,
+				lowest_nonlinear_perf, cpudata->perf.lowest_nonlinear_perf,
+				lowest_perf, cpudata->perf.lowest_perf);
 			goto skip_test;
 		}
 

drivers/cpufreq/amd-pstate.c +114 -90

index 278d909904e3b..a6066fb4ffb63 100644
@@ -142,18 +142,17 @@ static struct quirk_entry quirk_amd_7k62 = {
 	.lowest_freq = 550,
 };
 
-static inline u8 freq_to_perf(struct amd_cpudata *cpudata, unsigned int freq_val)
+static inline u8 freq_to_perf(union perf_cached perf, u32 nominal_freq, unsigned int freq_val)
 {
-	u8 perf_val = DIV_ROUND_UP_ULL((u64)freq_val * cpudata->nominal_perf,
-					cpudata->nominal_freq);
+	u8 perf_val = DIV_ROUND_UP_ULL((u64)freq_val * perf.nominal_perf, nominal_freq);
 
-	return clamp_t(u8, perf_val, cpudata->lowest_perf, cpudata->highest_perf);
+	return clamp_t(u8, perf_val, perf.lowest_perf, perf.highest_perf);
 }
 
-static inline u32 perf_to_freq(struct amd_cpudata *cpudata, u8 perf_val)
+static inline u32 perf_to_freq(union perf_cached perf, u32 nominal_freq, u8 perf_val)
 {
-	return DIV_ROUND_UP_ULL((u64)cpudata->nominal_freq * perf_val,
-				cpudata->nominal_perf);
+	return DIV_ROUND_UP_ULL((u64)nominal_freq * perf_val,
+				perf.nominal_perf);
 }
 
 static int __init dmi_matched_7k62_bios_bug(const struct dmi_system_id *dmi)

@@ -347,7 +346,9 @@ static int amd_pstate_set_energy_pref_index(struct cpufreq_policy *policy,
 	}
 
 	if (trace_amd_pstate_epp_perf_enabled()) {
-		trace_amd_pstate_epp_perf(cpudata->cpu, cpudata->highest_perf,
+		union perf_cached perf = READ_ONCE(cpudata->perf);
+
+		trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
 					  epp,
 					  FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cpudata->cppc_req_cached),
 					  FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cpudata->cppc_req_cached),

@@ -425,6 +426,7 @@ static inline int amd_pstate_cppc_enable(bool enable)
 
 static int msr_init_perf(struct amd_cpudata *cpudata)
 {
+	union perf_cached perf = READ_ONCE(cpudata->perf);
 	u64 cap1, numerator;
 
 	int ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,

@@ -436,19 +438,21 @@ static int msr_init_perf(struct amd_cpudata *cpudata)
 	if (ret)
 		return ret;
 
-	WRITE_ONCE(cpudata->highest_perf, numerator);
-	WRITE_ONCE(cpudata->max_limit_perf, numerator);
-	WRITE_ONCE(cpudata->nominal_perf, AMD_CPPC_NOMINAL_PERF(cap1));
-	WRITE_ONCE(cpudata->lowest_nonlinear_perf, AMD_CPPC_LOWNONLIN_PERF(cap1));
-	WRITE_ONCE(cpudata->lowest_perf, AMD_CPPC_LOWEST_PERF(cap1));
+	perf.highest_perf = numerator;
+	perf.max_limit_perf = numerator;
+	perf.min_limit_perf = AMD_CPPC_LOWEST_PERF(cap1);
+	perf.nominal_perf = AMD_CPPC_NOMINAL_PERF(cap1);
+	perf.lowest_nonlinear_perf = AMD_CPPC_LOWNONLIN_PERF(cap1);
+	perf.lowest_perf = AMD_CPPC_LOWEST_PERF(cap1);
+	WRITE_ONCE(cpudata->perf, perf);
 	WRITE_ONCE(cpudata->prefcore_ranking, AMD_CPPC_HIGHEST_PERF(cap1));
-	WRITE_ONCE(cpudata->min_limit_perf, AMD_CPPC_LOWEST_PERF(cap1));
 	return 0;
 }
 
 static int shmem_init_perf(struct amd_cpudata *cpudata)
 {
 	struct cppc_perf_caps cppc_perf;
+	union perf_cached perf = READ_ONCE(cpudata->perf);
 	u64 numerator;
 
 	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);

@@ -459,14 +463,14 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
 	if (ret)
 		return ret;
 
-	WRITE_ONCE(cpudata->highest_perf, numerator);
-	WRITE_ONCE(cpudata->max_limit_perf, numerator);
-	WRITE_ONCE(cpudata->nominal_perf, cppc_perf.nominal_perf);
-	WRITE_ONCE(cpudata->lowest_nonlinear_perf,
-		   cppc_perf.lowest_nonlinear_perf);
-	WRITE_ONCE(cpudata->lowest_perf, cppc_perf.lowest_perf);
+	perf.highest_perf = numerator;
+	perf.max_limit_perf = numerator;
+	perf.min_limit_perf = cppc_perf.lowest_perf;
+	perf.nominal_perf = cppc_perf.nominal_perf;
+	perf.lowest_nonlinear_perf = cppc_perf.lowest_nonlinear_perf;
+	perf.lowest_perf = cppc_perf.lowest_perf;
+	WRITE_ONCE(cpudata->perf, perf);
 	WRITE_ONCE(cpudata->prefcore_ranking, cppc_perf.highest_perf);
-	WRITE_ONCE(cpudata->min_limit_perf, cppc_perf.lowest_perf);
 
 	if (cppc_state == AMD_PSTATE_ACTIVE)
 		return 0;

@@ -549,14 +553,14 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
 			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
 {
 	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
-	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
+	union perf_cached perf = READ_ONCE(cpudata->perf);
 
 	if (!policy)
 		return;
 
 	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
 
-	policy->cur = perf_to_freq(cpudata, des_perf);
+	policy->cur = perf_to_freq(perf, cpudata->nominal_freq, des_perf);
 
 	if ((cppc_state == AMD_PSTATE_GUIDED) && (gov_flags & CPUFREQ_GOV_DYNAMIC_SWITCHING)) {
 		min_perf = des_perf;

@@ -565,7 +569,7 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
 
 	/* limit the max perf when core performance boost feature is disabled */
 	if (!cpudata->boost_supported)
-		max_perf = min_t(u8, nominal_perf, max_perf);
+		max_perf = min_t(u8, perf.nominal_perf, max_perf);
 
 	if (trace_amd_pstate_perf_enabled() && amd_pstate_sample(cpudata)) {
 		trace_amd_pstate_perf(min_perf, des_perf, max_perf, cpudata->freq,

@@ -602,39 +606,41 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
 	return 0;
 }
 
-static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
+static void amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
 {
-	u8 max_limit_perf, min_limit_perf;
 	struct amd_cpudata *cpudata = policy->driver_data;
+	union perf_cached perf = READ_ONCE(cpudata->perf);
 
-	max_limit_perf = freq_to_perf(cpudata, policy->max);
-	min_limit_perf = freq_to_perf(cpudata, policy->min);
+	perf.max_limit_perf = freq_to_perf(perf, cpudata->nominal_freq, policy->max);
+	perf.min_limit_perf = freq_to_perf(perf, cpudata->nominal_freq, policy->min);
 
 	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
-		min_limit_perf = min(cpudata->nominal_perf, max_limit_perf);
+		perf.min_limit_perf = min(perf.nominal_perf, perf.max_limit_perf);
 
-	WRITE_ONCE(cpudata->max_limit_perf, max_limit_perf);
-	WRITE_ONCE(cpudata->min_limit_perf, min_limit_perf);
 	WRITE_ONCE(cpudata->max_limit_freq, policy->max);
 	WRITE_ONCE(cpudata->min_limit_freq, policy->min);
-
-	return 0;
+	WRITE_ONCE(cpudata->perf, perf);
 }
 
 static int amd_pstate_update_freq(struct cpufreq_policy *policy,
 				  unsigned int target_freq, bool fast_switch)
 {
 	struct cpufreq_freqs freqs;
-	struct amd_cpudata *cpudata = policy->driver_data;
+	struct amd_cpudata *cpudata;
+	union perf_cached perf;
 	u8 des_perf;
 
+	cpudata = policy->driver_data;
+
 	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
 		amd_pstate_update_min_max_limit(policy);
 
+	perf = READ_ONCE(cpudata->perf);
+
 	freqs.old = policy->cur;
 	freqs.new = target_freq;
 
-	des_perf = freq_to_perf(cpudata, target_freq);
+	des_perf = freq_to_perf(perf, cpudata->nominal_freq, target_freq);
 
 	WARN_ON(fast_switch && !policy->fast_switch_enabled);
 	/*

@@ -645,8 +651,8 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
 	if (!fast_switch)
 		cpufreq_freq_transition_begin(policy, &freqs);
 
-	amd_pstate_update(cpudata, cpudata->min_limit_perf, des_perf,
-			  cpudata->max_limit_perf, fast_switch,
+	amd_pstate_update(cpudata, perf.min_limit_perf, des_perf,
+			  perf.max_limit_perf, fast_switch,
 			  policy->governor->flags);
 
 	if (!fast_switch)

@@ -675,9 +681,10 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 				   unsigned long target_perf,
 				   unsigned long capacity)
 {
-	u8 max_perf, min_perf, des_perf, cap_perf, min_limit_perf;
+	u8 max_perf, min_perf, des_perf, cap_perf;
 	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
 	struct amd_cpudata *cpudata;
+	union perf_cached perf;
 
 	if (!policy)
 		return;

@@ -687,8 +694,8 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
 		amd_pstate_update_min_max_limit(policy);
 
-	cap_perf = READ_ONCE(cpudata->highest_perf);
-	min_limit_perf = READ_ONCE(cpudata->min_limit_perf);
+	perf = READ_ONCE(cpudata->perf);
+	cap_perf = perf.highest_perf;
 
 	des_perf = cap_perf;
 	if (target_perf < capacity)

@@ -699,10 +706,10 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 	else
 		min_perf = cap_perf;
 
-	if (min_perf < min_limit_perf)
-		min_perf = min_limit_perf;
+	if (min_perf < perf.min_limit_perf)
+		min_perf = perf.min_limit_perf;
 
-	max_perf = cpudata->max_limit_perf;
+	max_perf = perf.max_limit_perf;
 	if (max_perf < min_perf)
 		max_perf = min_perf;
 

@@ -713,11 +720,12 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
+	union perf_cached perf = READ_ONCE(cpudata->perf);
 	u32 nominal_freq, max_freq;
 	int ret = 0;
 
 	nominal_freq = READ_ONCE(cpudata->nominal_freq);
-	max_freq = perf_to_freq(cpudata, READ_ONCE(cpudata->highest_perf));
+	max_freq = perf_to_freq(perf, cpudata->nominal_freq, perf.highest_perf);
 
 	if (on)
 		policy->cpuinfo.max_freq = max_freq;

@@ -888,30 +896,29 @@ static u32 amd_pstate_get_transition_latency(unsigned int cpu)
 }
 
 /*
- * amd_pstate_init_freq: Initialize the max_freq, min_freq,
- *                       nominal_freq and lowest_nonlinear_freq for
- *                       the @cpudata object.
+ * amd_pstate_init_freq: Initialize the nominal_freq and lowest_nonlinear_freq
+ *			 for the @cpudata object.
  *
- *  Requires: highest_perf, lowest_perf, nominal_perf and
- *            lowest_nonlinear_perf members of @cpudata to be
- *            initialized.
+ * Requires: all perf members of @cpudata to be initialized.
  *
- *  Returns 0 on success, non-zero value on failure.
+ * Returns 0 on success, non-zero value on failure.
  */
 static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 {
-	int ret;
-	u32 min_freq, max_freq;
-	u32 nominal_freq, lowest_nonlinear_freq;
+	u32 min_freq, max_freq, nominal_freq, lowest_nonlinear_freq;
 	struct cppc_perf_caps cppc_perf;
+	union perf_cached perf;
+	int ret;
 
 	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
 	if (ret)
 		return ret;
+	perf = READ_ONCE(cpudata->perf);
 
-	if (quirks && quirks->lowest_freq)
+	if (quirks && quirks->lowest_freq) {
 		min_freq = quirks->lowest_freq;
-	else
+		perf.lowest_perf = freq_to_perf(perf, nominal_freq, min_freq);
+	} else
 		min_freq = cppc_perf.lowest_freq;
 
 	if (quirks && quirks->nominal_freq)

@@ -924,8 +931,8 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 
 	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
 
-	max_freq = perf_to_freq(cpudata, cpudata->highest_perf);
-	lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
+	max_freq = perf_to_freq(perf, nominal_freq, perf.highest_perf);
+	lowest_nonlinear_freq = perf_to_freq(perf, nominal_freq, perf.lowest_nonlinear_perf);
 	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
 
 	/**

@@ -952,6 +959,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata;
+	union perf_cached perf;
 	struct device *dev;
 	int ret;
 

@@ -987,8 +995,14 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
 	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
 
-	policy->cpuinfo.min_freq = policy->min = perf_to_freq(cpudata, cpudata->lowest_perf);
-	policy->cpuinfo.max_freq = policy->max = perf_to_freq(cpudata, cpudata->highest_perf);
+	perf = READ_ONCE(cpudata->perf);
+
+	policy->cpuinfo.min_freq = policy->min = perf_to_freq(perf,
+							      cpudata->nominal_freq,
+							      perf.lowest_perf);
+	policy->cpuinfo.max_freq = policy->max = perf_to_freq(perf,
+							      cpudata->nominal_freq,
+							      perf.highest_perf);
 
 	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
 

@@ -1069,23 +1083,27 @@ static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
 static ssize_t show_amd_pstate_max_freq(struct cpufreq_policy *policy,
 					char *buf)
 {
-	struct amd_cpudata *cpudata = policy->driver_data;
+	struct amd_cpudata *cpudata;
+	union perf_cached perf;
 
+	cpudata = policy->driver_data;
+	perf = READ_ONCE(cpudata->perf);
 
-	return sysfs_emit(buf, "%u\n", perf_to_freq(cpudata, READ_ONCE(cpudata->highest_perf)));
+	return sysfs_emit(buf, "%u\n",
+			  perf_to_freq(perf, cpudata->nominal_freq, perf.highest_perf));
 }
 
 static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *policy,
 						     char *buf)
 {
-	int freq;
-	struct amd_cpudata *cpudata = policy->driver_data;
+	struct amd_cpudata *cpudata;
+	union perf_cached perf;
 
-	freq = READ_ONCE(cpudata->lowest_nonlinear_freq);
-	if (freq < 0)
-		return freq;
+	cpudata = policy->driver_data;
+	perf = READ_ONCE(cpudata->perf);
 
-	return sysfs_emit(buf, "%u\n", freq);
+	return sysfs_emit(buf, "%u\n",
+			  perf_to_freq(perf, cpudata->nominal_freq, perf.lowest_nonlinear_perf));
 }
 
 /*

@@ -1095,12 +1113,11 @@ static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *poli
 static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
 					    char *buf)
 {
-	u8 perf;
-	struct amd_cpudata *cpudata = policy->driver_data;
+	struct amd_cpudata *cpudata;
 
-	perf = READ_ONCE(cpudata->highest_perf);
+	cpudata = policy->driver_data;
 
-	return sysfs_emit(buf, "%u\n", perf);
+	return sysfs_emit(buf, "%u\n", cpudata->perf.highest_perf);
 }
 
 static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,

@@ -1431,6 +1448,7 @@ static bool amd_pstate_acpi_pm_profile_undefined(void)
 static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata;
+	union perf_cached perf;
 	struct device *dev;
 	u64 value;
 	int ret;

@@ -1464,8 +1482,15 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	if (ret)
 		goto free_cpudata1;
 
-	policy->cpuinfo.min_freq = policy->min = perf_to_freq(cpudata, cpudata->lowest_perf);
-	policy->cpuinfo.max_freq = policy->max = perf_to_freq(cpudata, cpudata->highest_perf);
+	perf = READ_ONCE(cpudata->perf);
+
+	policy->cpuinfo.min_freq = policy->min = perf_to_freq(perf,
+							      cpudata->nominal_freq,
+							      perf.lowest_perf);
+	policy->cpuinfo.max_freq = policy->max = perf_to_freq(perf,
+							      cpudata->nominal_freq,
+							      perf.highest_perf);
+
 	/* It will be updated by governor */
 	policy->cur = policy->cpuinfo.min_freq;
 

@@ -1526,6 +1551,7 @@ static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
 static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
+	union perf_cached perf;
 	u8 epp;
 
 	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)

@@ -1536,15 +1562,16 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 	else
 		epp = READ_ONCE(cpudata->epp_cached);
 
+	perf = READ_ONCE(cpudata->perf);
 	if (trace_amd_pstate_epp_perf_enabled()) {
-		trace_amd_pstate_epp_perf(cpudata->cpu, cpudata->highest_perf, epp,
-					  cpudata->min_limit_perf,
-					  cpudata->max_limit_perf,
+		trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf, epp,
+					  perf.min_limit_perf,
+					  perf.max_limit_perf,
 					  policy->boost_enabled);
 	}
 
-	return amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
-				      cpudata->max_limit_perf, epp, false);
+	return amd_pstate_update_perf(cpudata, perf.min_limit_perf, 0U,
+				      perf.max_limit_perf, epp, false);
 }
 
 static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)

@@ -1576,20 +1603,18 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
 static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	u8 max_perf;
+	union perf_cached perf = READ_ONCE(cpudata->perf);
 	int ret;
 
 	ret = amd_pstate_cppc_enable(true);
 	if (ret)
 		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
 
-	max_perf = READ_ONCE(cpudata->highest_perf);
-
 	if (trace_amd_pstate_epp_perf_enabled()) {
-		trace_amd_pstate_epp_perf(cpudata->cpu, cpudata->highest_perf,
+		trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
 					  cpudata->epp_cached,
 					  FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cpudata->cppc_req_cached),
-					  max_perf, policy->boost_enabled);
+					  perf.highest_perf, policy->boost_enabled);
 	}
 
 	return amd_pstate_epp_update_limit(policy);

@@ -1613,22 +1638,21 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
 static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	u8 min_perf;
+	union perf_cached perf = READ_ONCE(cpudata->perf);
 
 	if (cpudata->suspended)
 		return 0;
 
-	min_perf = READ_ONCE(cpudata->lowest_perf);
-
 	guard(mutex)(&amd_pstate_limits_lock);
 
 	if (trace_amd_pstate_epp_perf_enabled()) {
-		trace_amd_pstate_epp_perf(cpudata->cpu, cpudata->highest_perf,
+		trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
 					  AMD_CPPC_EPP_BALANCE_POWERSAVE,
-					  min_perf, min_perf, policy->boost_enabled);
+					  perf.lowest_perf, perf.lowest_perf,
+					  policy->boost_enabled);
 	}
 
-	return amd_pstate_update_perf(cpudata, min_perf, 0, min_perf,
+	return amd_pstate_update_perf(cpudata, perf.lowest_perf, 0, perf.lowest_perf,
 				      AMD_CPPC_EPP_BALANCE_POWERSAVE, false);
 }
 

drivers/cpufreq/amd-pstate.h +31 -18

index 0149933692458..8421c83c07919 100644
@@ -13,6 +13,34 @@
 /*********************************************************************
  *                        AMD P-state INTERFACE                       *
  *********************************************************************/
+
+/**
+ * union perf_cached - A union to cache performance-related data.
+ * @highest_perf: the maximum performance an individual processor may reach,
+ *		  assuming ideal conditions
+ *		  For platforms that do not support the preferred core feature, the
+ *		  highest_pef may be configured with 166 or 255, to avoid max frequency
+ *		  calculated wrongly. we take the fixed value as the highest_perf.
+ * @nominal_perf: the maximum sustained performance level of the processor,
+ *		  assuming ideal operating conditions
+ * @lowest_nonlinear_perf: the lowest performance level at which nonlinear power
+ *			   savings are achieved
+ * @lowest_perf: the absolute lowest performance level of the processor
+ * @min_limit_perf: Cached value of the performance corresponding to policy->min
+ * @max_limit_perf: Cached value of the performance corresponding to policy->max
+ */
+union perf_cached {
+	struct {
+		u8	highest_perf;
+		u8	nominal_perf;
+		u8	lowest_nonlinear_perf;
+		u8	lowest_perf;
+		u8	min_limit_perf;
+		u8	max_limit_perf;
+	};
+	u64	val;
+};
+
 /**
  * struct  amd_aperf_mperf
  * @aperf: actual performance frequency clock count

@@ -30,20 +58,9 @@ struct amd_aperf_mperf {
  * @cpu: CPU number
  * @req: constraint request to apply
  * @cppc_req_cached: cached performance request hints
- * @highest_perf: the maximum performance an individual processor may reach,
- *		  assuming ideal conditions
- *		  For platforms that do not support the preferred core feature, the
- *		  highest_pef may be configured with 166 or 255, to avoid max frequency
- *		  calculated wrongly. we take the fixed value as the highest_perf.
- * @nominal_perf: the maximum sustained performance level of the processor,
- *		  assuming ideal operating conditions
- * @lowest_nonlinear_perf: the lowest performance level at which nonlinear power
- *			   savings are achieved
- * @lowest_perf: the absolute lowest performance level of the processor
+ * @perf: cached performance-related data
  * @prefcore_ranking: the preferred core ranking, the higher value indicates a higher
  * 		  priority.
- * @min_limit_perf: Cached value of the performance corresponding to policy->min
- * @max_limit_perf: Cached value of the performance corresponding to policy->max
  * @min_limit_freq: Cached value of policy->min (in khz)
  * @max_limit_freq: Cached value of policy->max (in khz)
  * @nominal_freq: the frequency (in khz) that mapped to nominal_perf

@@ -68,13 +85,9 @@ struct amd_cpudata {
 	struct	freq_qos_request req[2];
 	u64	cppc_req_cached;
 
-	u8	highest_perf;
-	u8	nominal_perf;
-	u8	lowest_nonlinear_perf;
-	u8	lowest_perf;
+	union perf_cached perf;
+
 	u8	prefcore_ranking;
-	u8	min_limit_perf;
-	u8	max_limit_perf;
 	u32	min_limit_freq;
 	u32	max_limit_freq;
 	u32	nominal_freq;

From: Mario Limonciello <[email protected]>

amd_pstate_cpu_boost_update() and refresh_frequency_limits() both update the policy state and have nothing to do with the amd-pstate driver itself.

A global "limits" lock doesn't make sense because each CPU can have policies changed independently. Each time a CPU changes values they will atomically be written to the per-CPU perf member. Drop per CPU locking cases.

The remaining "global" driver lock is used to ensure that only one entity can change driver modes at a given time.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

drivers/cpufreq/amd-pstate.c +3 -10

index a6066fb4ffb63..85e3daddb56e0 100644
@@ -196,7 +196,6 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
 	return -EINVAL;
 }
 
-static DEFINE_MUTEX(amd_pstate_limits_lock);
 static DEFINE_MUTEX(amd_pstate_driver_lock);
 
 static u8 msr_get_epp(struct amd_cpudata *cpudata)

@@ -752,7 +751,6 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state)
 		pr_err("Boost mode is not supported by this processor or SBIOS\n");
 		return -EOPNOTSUPP;
 	}
-	guard(mutex)(&amd_pstate_driver_lock);
 
 	ret = amd_pstate_cpu_boost_update(policy, state);
 	refresh_frequency_limits(policy);

@@ -1175,8 +1173,6 @@ static ssize_t store_energy_performance_preference(
 	if (ret < 0)
 		return -EINVAL;
 
-	guard(mutex)(&amd_pstate_limits_lock);
-
 	ret = amd_pstate_set_energy_pref_index(policy, ret);
 
 	return ret ? ret : count;

@@ -1349,8 +1345,10 @@ int amd_pstate_update_status(const char *buf, size_t size)
 	if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX)
 		return -EINVAL;
 
-	if (mode_state_machine[cppc_state][mode_idx])
+	if (mode_state_machine[cppc_state][mode_idx]) {
+		guard(mutex)(&amd_pstate_driver_lock);
 		return mode_state_machine[cppc_state][mode_idx](mode_idx);
+	}
 
 	return 0;
 }

@@ -1371,7 +1369,6 @@ static ssize_t status_store(struct device *a, struct device_attribute *b,
 	char *p = memchr(buf, '\n', count);
 	int ret;
 
-	guard(mutex)(&amd_pstate_driver_lock);
 	ret = amd_pstate_update_status(buf, p ? p - buf : count);
 
 	return ret < 0 ? ret : count;

@@ -1643,8 +1640,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
 	if (cpudata->suspended)
 		return 0;
 
-	guard(mutex)(&amd_pstate_limits_lock);
-
 	if (trace_amd_pstate_epp_perf_enabled()) {
 		trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
 					  AMD_CPPC_EPP_BALANCE_POWERSAVE,

@@ -1684,8 +1679,6 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
 	struct amd_cpudata *cpudata = policy->driver_data;
 
 	if (cpudata->suspended) {
-		guard(mutex)(&amd_pstate_limits_lock);
-
 		/* enable amd pstate from suspend state*/
 		amd_pstate_epp_reenable(policy);
 

From: Mario Limonciello <[email protected]>

Several Ryzen AI processors support the exact same value for lowest nonlinear perf and lowest perf. Loosen up the unit tests to allow this scenario.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
 drivers/cpufreq/amd-pstate-ut.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +2 -2

index 9f790c7254d52..0f0b867e271cc 100644
@@ -184,7 +184,7 @@ static void amd_pstate_ut_check_perf(u32 index)
 
 		if (!((highest_perf >= nominal_perf) &&
 			(nominal_perf > lowest_nonlinear_perf) &&
-			(lowest_nonlinear_perf > lowest_perf) &&
+			(lowest_nonlinear_perf >= lowest_perf) &&
 			(lowest_perf > 0))) {
 			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
 			pr_err("%s cpu%d highest=%d >= nominal=%d > lowest_nonlinear=%d > lowest=%d > 0, the formula is incorrect!\n",

@@ -217,7 +217,7 @@ static void amd_pstate_ut_check_freq(u32 index)
 
 		if (!((policy->cpuinfo.max_freq >= cpudata->nominal_freq) &&
 			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
-			(cpudata->lowest_nonlinear_freq > policy->cpuinfo.min_freq) &&
+			(cpudata->lowest_nonlinear_freq >= policy->cpuinfo.min_freq) &&
 			(policy->cpuinfo.min_freq > 0))) {
 			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
 			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",

From: Mario Limonciello <[email protected]>

Enums are effectively used as a boolean and don't show the return value of the failing call.

Instead of using enums switch to returning the actual return code from the unit test.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
 drivers/cpufreq/amd-pstate-ut.c | 143 ++++++++++++--------------------
 1 file changed, 55 insertions(+), 88 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +12 -22

index 0f0b867e271cc..028527a0019ca 100644
@@ -32,30 +32,20 @@
 
 #include "amd-pstate.h"
 
-/*
- * Abbreviations:
- * amd_pstate_ut: used as a shortform for AMD P-State unit test.
- * It helps to keep variable names smaller, simpler
- */
-enum amd_pstate_ut_result {
-	AMD_PSTATE_UT_RESULT_PASS,
-	AMD_PSTATE_UT_RESULT_FAIL,
-};
 
 struct amd_pstate_ut_struct {
 	const char *name;
-	void (*func)(u32 index);
-	enum amd_pstate_ut_result result;
+	int (*func)(u32 index);
 };
 
 /*
  * Kernel module for testing the AMD P-State unit test
  */
-static void amd_pstate_ut_acpi_cpc_valid(u32 index);
-static void amd_pstate_ut_check_enabled(u32 index);
-static void amd_pstate_ut_check_perf(u32 index);
-static void amd_pstate_ut_check_freq(u32 index);
-static void amd_pstate_ut_check_driver(u32 index);
+static int amd_pstate_ut_acpi_cpc_valid(u32 index);
+static int amd_pstate_ut_check_enabled(u32 index);
+static int amd_pstate_ut_check_perf(u32 index);
+static int amd_pstate_ut_check_freq(u32 index);
+static int amd_pstate_ut_check_driver(u32 index);
 
 static struct amd_pstate_ut_struct amd_pstate_ut_cases[] = {
 	{"amd_pstate_ut_acpi_cpc_valid",   amd_pstate_ut_acpi_cpc_valid   },

@@ -78,51 +68,46 @@ static bool get_shared_mem(void)
 /*
  * check the _CPC object is present in SBIOS.
  */
-static void amd_pstate_ut_acpi_cpc_valid(u32 index)
+static int amd_pstate_ut_acpi_cpc_valid(u32 index)
 {
-	if (acpi_cpc_valid())
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
-	else {
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+	if (!acpi_cpc_valid()) {
 		pr_err("%s the _CPC object is not present in SBIOS!\n", __func__);
+		return -EINVAL;
 	}
+
+	return 0;
 }
 
-static void amd_pstate_ut_pstate_enable(u32 index)
+/*

drivers/cpufreq/amd-pstate-ut.c +6 -17

 	ret = rdmsrl_safe(MSR_AMD_CPPC_ENABLE, &cppc_enable);
 	if (ret) {
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
 		pr_err("%s rdmsrl_safe MSR_AMD_CPPC_ENABLE ret=%d error!\n", __func__, ret);
-		return;
+		return ret;
 	}
-	if (cppc_enable)
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
-	else {
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+
+	if (!cppc_enable) {
 		pr_err("%s amd pstate must be enabled!\n", __func__);
+		return -EINVAL;
 	}
-}
 
-/*
- * check if amd pstate is enabled
- */
-static void amd_pstate_ut_check_enabled(u32 index)
-{
-	if (get_shared_mem())
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
-	else
-		amd_pstate_ut_pstate_enable(index);
+	return 0;
 }
 
 /*
  * check if performance values are reasonable.
  * highest_perf >= nominal_perf > lowest_nonlinear_perf > lowest_perf > 0
  */
-static void amd_pstate_ut_check_perf(u32 index)
+static int amd_pstate_ut_check_perf(u32 index)
 {
 	int cpu = 0, ret = 0;
 	u32 highest_perf = 0, nominal_perf = 0, lowest_nonlinear_perf = 0, lowest_perf = 0;

Quoted 5 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

Enums are effectively used as a boolean and don't show the return value of the failing call.

Quoted 3 lines

Instead of using enums switch to returning the actual return code from the unit test.

One query below, apart from that LGTM,

Reviewed-by Dhananjay Ugwekar <[email protected]>

Quoted 67 lines

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
  drivers/cpufreq/amd-pstate-ut.c | 143 ++++++++++++--------------------
  1 file changed, 55 insertions(+), 88 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +11 -21

index 0f0b867e271cc..028527a0019ca 100644
@@ -32,30 +32,20 @@
 
  #include "amd-pstate.h"
 
-/*
- * Abbreviations:
- * amd_pstate_ut: used as a shortform for AMD P-State unit test.
- * It helps to keep variable names smaller, simpler
- */
-enum amd_pstate_ut_result {
-	AMD_PSTATE_UT_RESULT_PASS,
-	AMD_PSTATE_UT_RESULT_FAIL,
-};
 
  struct amd_pstate_ut_struct {
  	const char *name;
-	void (*func)(u32 index);
-	enum amd_pstate_ut_result result;
+	int (*func)(u32 index);
  };
 
  /*
   * Kernel module for testing the AMD P-State unit test
   */
-static void amd_pstate_ut_acpi_cpc_valid(u32 index);
-static void amd_pstate_ut_check_enabled(u32 index);
-static void amd_pstate_ut_check_perf(u32 index);
-static void amd_pstate_ut_check_freq(u32 index);
-static void amd_pstate_ut_check_driver(u32 index);
+static int amd_pstate_ut_acpi_cpc_valid(u32 index);
+static int amd_pstate_ut_check_enabled(u32 index);
+static int amd_pstate_ut_check_perf(u32 index);
+static int amd_pstate_ut_check_freq(u32 index);
+static int amd_pstate_ut_check_driver(u32 index);
 
  static struct amd_pstate_ut_struct amd_pstate_ut_cases[] = {
  	{"amd_pstate_ut_acpi_cpc_valid",   amd_pstate_ut_acpi_cpc_valid   },

@@ -78,51 +68,46 @@ static bool get_shared_mem(void)
  /*
   * check the _CPC object is present in SBIOS.
   */
-static void amd_pstate_ut_acpi_cpc_valid(u32 index)
+static int amd_pstate_ut_acpi_cpc_valid(u32 index)
  {
-	if (acpi_cpc_valid())
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
-	else {
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+	if (!acpi_cpc_valid()) {
  		pr_err("%s the _CPC object is not present in SBIOS!\n", __func__);
+		return -EINVAL;
  	}
+
+	return 0;
  }
 

drivers/cpufreq/amd-pstate-ut.c static bool get_shared_mem(void) · line 90

Quoted 12 lines

drivers/cpufreq/amd-pstate-ut.c +8 -2

-static void amd_pstate_ut_pstate_enable(u32 index)
+/*
+ * check if amd pstate is enabled
+ */
+static int amd_pstate_ut_check_enabled(u32 index)
  {
-	int ret = 0;
  	u64 cppc_enable = 0;
+	int ret;
+
+	if (get_shared_mem())
+		return 0;

What do you think about adding a "cppc_get_enable()" function in acpi_cppc.c so that we can run this check for shared mem systems as well ?

Thanks,
Dhananjay

Quoted 40 lines

drivers/cpufreq/amd-pstate-ut.c +6 -17

  	ret = rdmsrl_safe(MSR_AMD_CPPC_ENABLE, &cppc_enable);
  	if (ret) {
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
  		pr_err("%s rdmsrl_safe MSR_AMD_CPPC_ENABLE ret=%d error!\n", __func__, ret);
-		return;
+		return ret;
  	}
-	if (cppc_enable)
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
-	else {
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+
+	if (!cppc_enable) {
  		pr_err("%s amd pstate must be enabled!\n", __func__);
+		return -EINVAL;
  	}
-}
 
-/*
- * check if amd pstate is enabled
- */
-static void amd_pstate_ut_check_enabled(u32 index)
-{
-	if (get_shared_mem())
-		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
-	else
-		amd_pstate_ut_pstate_enable(index);
+	return 0;
  }
 
  /*
   * check if performance values are reasonable.
   * highest_perf >= nominal_perf > lowest_nonlinear_perf > lowest_perf > 0
   */
-static void amd_pstate_ut_check_perf(u32 index)
+static int amd_pstate_ut_check_perf(u32 index)
  {
  	int cpu = 0, ret = 0;
  	u32 highest_perf = 0, nominal_perf = 0, lowest_nonlinear_perf = 0, lowest_perf = 0;

[Snip]

Quoted 6 lines · Dhananjay Ugwekar

Quoted 5 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

Enums are effectively used as a boolean and don't show the return value of the failing call.

Quoted 7 lines

Quoted 3 lines

Instead of using enums switch to returning the actual return code from the unit test.

One query below, apart from that LGTM,

Reviewed-by Dhananjay Ugwekar <[email protected]>

Thanks!

Quoted 68 lines

Quoted 68 lines

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
  drivers/cpufreq/amd-pstate-ut.c | 143 ++++++++++++--------------------

  1 file changed, 55 insertions(+), 88 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +0 -0

index 0f0b867e271cc..028527a0019ca 100644
@@ -32,30 +32,20 @@
     #include "amd-pstate.h"

  -/*
- * Abbreviations:
- * amd_pstate_ut: used as a shortform for AMD P-State unit test.
- * It helps to keep variable names smaller, simpler
- */

drivers/cpufreq/amd-pstate-ut.c +0 -4

-enum amd_pstate_ut_result {
-    AMD_PSTATE_UT_RESULT_PASS,
-    AMD_PSTATE_UT_RESULT_FAIL,
-};

    struct amd_pstate_ut_struct {
      const char *name;
-    void (*func)(u32 index);
-    enum amd_pstate_ut_result result;
+    int (*func)(u32 index);
  };
    /*
   * Kernel module for testing the AMD P-State unit test */

drivers/cpufreq/amd-pstate-ut.c +5 -5

-static void amd_pstate_ut_acpi_cpc_valid(u32 index);
-static void amd_pstate_ut_check_enabled(u32 index);
-static void amd_pstate_ut_check_perf(u32 index);
-static void amd_pstate_ut_check_freq(u32 index);
-static void amd_pstate_ut_check_driver(u32 index);
+static int amd_pstate_ut_acpi_cpc_valid(u32 index);
+static int amd_pstate_ut_check_enabled(u32 index);
+static int amd_pstate_ut_check_perf(u32 index);
+static int amd_pstate_ut_check_freq(u32 index);
+static int amd_pstate_ut_check_driver(u32 index);

    static struct amd_pstate_ut_struct amd_pstate_ut_cases[] = { {"amd_pstate_ut_acpi_cpc_valid",   amd_pstate_ut_acpi_cpc_valid   },

drivers/cpufreq/amd-pstate-ut.c +9 -6

@@ -78,51 +68,46 @@ static bool get_shared_mem(void)
   /*
    * check the _CPC object is present in SBIOS.
    */
-static void amd_pstate_ut_acpi_cpc_valid(u32 index)
+static int amd_pstate_ut_acpi_cpc_valid(u32 index)
   {
-    if (acpi_cpc_valid())
-        amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
-    else {
-        amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+    if (!acpi_cpc_valid()) {
           pr_err("%s the _CPC object is not present in SBIOS!\n", __func__);
+        return -EINVAL;
       }
+
+    return 0;
   }
   -static void amd_pstate_ut_pstate_enable(u32 index)
+/*
+ * check if amd pstate is enabled
+ */
+static int amd_pstate_ut_check_enabled(u32 index)
   {
-    int ret = 0;

Quoted 9 lines

drivers/cpufreq/amd-pstate-ut.c static bool get_shared_mem(void) · line 90

Quoted 5 lines

drivers/cpufreq/amd-pstate-ut.c +4 -0

       u64 cppc_enable = 0;
+    int ret;
+
+    if (get_shared_mem())
+        return 0;

What do you think about adding a "cppc_get_enable()" function in acpi_cppc.c so that we can run this check for shared mem systems as well ?

I think it's a good idea.  Would you mind working that out for after this series lands?

Quoted 3 lines

Thanks,
Dhananjay

From: Mario Limonciello <[email protected]>

If a CPU is missing a policy or one has been offlined then the unit test is skipped for the rest of the CPUs on the system.

Instead; iterate online CPUs and skip any missing policies to allow continuing to test the rest of them.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
v3:
* Only check online CPUs
* Update commit message

---
 drivers/cpufreq/amd-pstate-ut.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +4 -4

index 028527a0019ca..3a541780f374f 100644
@@ -116,12 +116,12 @@ static int amd_pstate_ut_check_perf(u32 index)
 	struct amd_cpudata *cpudata = NULL;
 	union perf_cached cur_perf;
 
-	for_each_possible_cpu(cpu) {
+	for_each_online_cpu(cpu) {
 		struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
 
 		policy = cpufreq_cpu_get(cpu);
 		if (!policy)
-			break;
+			continue;
 		cpudata = policy->driver_data;
 
 		if (get_shared_mem()) {

@@ -188,12 +188,12 @@ static int amd_pstate_ut_check_freq(u32 index)
 	int cpu = 0;
 	struct amd_cpudata *cpudata = NULL;
 
-	for_each_possible_cpu(cpu) {
+	for_each_online_cpu(cpu) {
 		struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
 
 		policy = cpufreq_cpu_get(cpu);
 		if (!policy)
-			break;
+			continue;
 		cpudata = policy->driver_data;
 
 		if (!((policy->cpuinfo.max_freq >= cpudata->nominal_freq) &&

From: Mario Limonciello <[email protected]>

The CPPC enable register is configured as "write once". That is any future writes don't actually do anything.

Because of this, all the cleanup paths that currently exist for CPPC disable are non-effective.

Rework CPPC enable to only enable after all the CAP registers have been read to avoid enabling CPPC on CPUs with invalid _CPC or unpopulated MSRs.

As the register is write once, remove all cleanup paths as well.

Signed-off-by Mario Limonciello <[email protected]>

---
v4:
* Remove unnecessary amd_pstate_update_perf() call during online
* Remove unnecessary if (ret) ret.
* Drop amd_pstate_cpu_resume()
* Drop unnecessary derefs
v3:
* Fixup for suspend/resume issue

---
 drivers/cpufreq/amd-pstate.c | 187 +++++++++++------------------------
 1 file changed, 55 insertions(+), 132 deletions(-)

drivers/cpufreq/amd-pstate.c +0 -0

index fa9c581c68a39..f152636cecbeb 100644

drivers/cpufreq/amd-pstate.c +5 -44

-static inline int msr_cppc_enable(bool enable)
+static int shmem_cppc_enable(struct cpufreq_policy *policy)
 {
-	int ret, cpu;
-	unsigned long logical_proc_id_mask = 0;
-
-       /*
-        * MSR_AMD_CPPC_ENABLE is write-once, once set it cannot be cleared.
-        */
-	if (!enable)
-		return 0;
-
-	if (enable == cppc_enabled)
-		return 0;
-
-	for_each_present_cpu(cpu) {
-		unsigned long logical_id = topology_logical_package_id(cpu);
-
-		if (test_bit(logical_id, &logical_proc_id_mask))
-			continue;
-
-		set_bit(logical_id, &logical_proc_id_mask);
-
-		ret = wrmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_ENABLE,
-				enable);
-		if (ret)
-			return ret;
-	}
-
-	cppc_enabled = enable;
-	return 0;
-}
-
-static int shmem_cppc_enable(bool enable)
-{
-	int cpu, ret = 0;
 	struct cppc_perf_ctrls perf_ctrls;
+	int ret;
 
-	if (enable == cppc_enabled)
-		return 0;
-
-	for_each_present_cpu(cpu) {
-		ret = cppc_set_enable(cpu, enable);
-		if (ret)
-			return ret;
+	ret = cppc_set_enable(policy->cpu, 1);
+	if (ret)
+		return ret;
 
-		/* Enable autonomous mode for EPP */
-		if (cppc_state == AMD_PSTATE_ACTIVE) {
-			/* Set desired perf as zero to allow EPP firmware control */

drivers/cpufreq/amd-pstate.c +14 -26

 	}
 
-	cppc_enabled = enable;
 	return ret;
 }
 
 DEFINE_STATIC_CALL(amd_pstate_cppc_enable, msr_cppc_enable);
 
-static inline int amd_pstate_cppc_enable(bool enable)
+static inline int amd_pstate_cppc_enable(struct cpufreq_policy *policy)
 {
-	return static_call(amd_pstate_cppc_enable)(enable);
+	return static_call(amd_pstate_cppc_enable)(policy);
 }
 
 static int msr_init_perf(struct amd_cpudata *cpudata)

@@ -1115,28 +1060,6 @@ static void amd_pstate_cpu_exit(struct cpufreq_policy *policy)
 	kfree(cpudata);
 }
 
-static int amd_pstate_cpu_resume(struct cpufreq_policy *policy)
-{
-	int ret;
-
-	ret = amd_pstate_cppc_enable(true);
-	if (ret)
-		pr_err("failed to enable amd-pstate during resume, return %d\n", ret);
-
-	return ret;
-}
-
-static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
-{
-	int ret;
-
-	ret = amd_pstate_cppc_enable(false);
-	if (ret)
-		pr_err("failed to disable amd-pstate during suspend, return %d\n", ret);
-
-	return ret;
-}
-
 /* Sysfs attributes */
 
 /*

@@ -1228,8 +1151,10 @@ static ssize_t show_energy_performance_available_preferences(
 static ssize_t store_energy_performance_preference(
 		struct cpufreq_policy *policy, const char *buf, size_t count)
 {
+	struct amd_cpudata *cpudata = policy->driver_data;
 	char str_preference[21];
 	ssize_t ret;
+	u8 epp;
 
 	ret = sscanf(buf, "%20s", str_preference);
 	if (ret != 1)

@@ -1239,7 +1164,29 @@ static ssize_t store_energy_performance_preference(
 	if (ret < 0)
 		return -EINVAL;
 
-	ret = amd_pstate_set_energy_pref_index(policy, ret);
+	if (!ret)
+		epp = cpudata->epp_default;
+	else
+		epp = epp_values[ret];
+
+	if (epp > 0 && policy->policy == CPUFREQ_POLICY_PERFORMANCE) {
+		pr_debug("EPP cannot be set under performance policy\n");
+		return -EBUSY;
+	}
+

drivers/cpufreq/amd-pstate.c +3 -9

+	}
+
+	ret = amd_pstate_set_epp(policy, epp);
 
 	return ret ? ret : count;
 }

@@ -1272,7 +1219,6 @@ static ssize_t show_energy_performance_preference(
 
 static void amd_pstate_driver_cleanup(void)
 {
-	amd_pstate_cppc_enable(false);
 	cppc_state = AMD_PSTATE_DISABLE;
 	current_pstate_driver = NULL;
 }

@@ -1306,14 +1252,6 @@ static int amd_pstate_register_driver(int mode)
 
 	cppc_state = mode;
 
-	ret = amd_pstate_cppc_enable(true);
-	if (ret) {
-		pr_err("failed to enable cppc during amd-pstate driver registration, return %d\n",
-		       ret);
-		amd_pstate_driver_cleanup();
-		return ret;
-	}
-
 	/* at least one CPU supports CPB */
 	current_pstate_driver->boost_enabled = cpu_feature_enabled(X86_FEATURE_CPB);
 

drivers/cpufreq/amd-pstate.c +2 -13

+	if (ret)
+		goto free_cpudata1;
 
 	/* It will be updated by governor */
 	policy->cur = policy->cpuinfo.min_freq;
 
-	policy->driver_data = cpudata;
 
 	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
 

@@ -1649,31 +1591,21 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
 	return 0;
 }
 
-static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
-{
-	int ret;
-
-	ret = amd_pstate_cppc_enable(true);
-	if (ret)
-		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
-
-
-	return amd_pstate_epp_update_limit(policy);
-}
-
 static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
 	int ret;

drivers/cpufreq/amd-pstate.c +6 -13

 	return 0;
+
 }
 
 static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)

@@ -1691,11 +1623,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
 static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	int ret;
-
-	/* avoid suspending when EPP is not enabled */
-	if (cppc_state != AMD_PSTATE_ACTIVE)
-		return 0;
 
 	/* invalidate to ensure it's rewritten during resume */
 	cpudata->cppc_req_cached = 0;

@@ -1703,11 +1630,6 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
 	/* set this flag to avoid setting core offline*/
 	cpudata->suspended = true;
 
-	/* disable CPPC in lowlevel firmware */
-	ret = amd_pstate_cppc_enable(false);
-	if (ret)
-		pr_err("failed to suspend, return %d\n", ret);
-
 	return 0;
 }
 

@@ -1716,8 +1638,12 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
 	struct amd_cpudata *cpudata = policy->driver_data;
 
 	if (cpudata->suspended) {
+		int ret;
+
 		/* enable amd pstate from suspend state*/
-		amd_pstate_epp_reenable(policy);
+		ret = amd_pstate_epp_update_limit(policy);
+		if (ret)
+			return ret;
 
 		cpudata->suspended = false;
 	}

@@ -1732,8 +1658,6 @@ static struct cpufreq_driver amd_pstate_driver = {
 	.fast_switch    = amd_pstate_fast_switch,
 	.init		= amd_pstate_cpu_init,
 	.exit		= amd_pstate_cpu_exit,
-	.suspend	= amd_pstate_cpu_suspend,
-	.resume		= amd_pstate_cpu_resume,
 	.set_boost	= amd_pstate_set_boost,
 	.update_limits	= amd_pstate_update_limits,
 	.name		= "amd-pstate",

drivers/cpufreq/amd-pstate.c +0 -1

 	.update_limits	= amd_pstate_update_limits,
 	.set_boost	= amd_pstate_set_boost,
 	.name		= "amd-pstate-epp",

@@ -1900,7 +1824,6 @@ static int __init amd_pstate_init(void)
 
 global_attr_free:
 	cpufreq_unregister_driver(current_pstate_driver);
-	amd_pstate_cppc_enable(false);
 	return ret;
 }
 device_initcall(amd_pstate_init);

drivers/cpufreq/amd-pstate.c static int shmem_set_epp(struct cpufreq_policy *policy, u8 epp) · line 391

Quoted 5 lines

drivers/cpufreq/amd-pstate.c +5 -0

+	/* Enable autonomous mode for EPP */
+	if (cppc_state == AMD_PSTATE_ACTIVE) {
+		/* Set desired perf as zero to allow EPP firmware control */
+		perf_ctrls.desired_perf = 0;
+		ret = cppc_set_perf(policy->cpu, &perf_ctrls);

I'm thinking do we need this "setting of desired_perf" as a part of shmem_cppc_enable,
one thing is we're not doing it in the "msr_" counterpart
also, I guess this would be taken care as part of amd_pstate_epp_set_policy()->amd_pstate_epp_update_limit()->amd_pstate_update_perf()

Quoted 76 lines

  	}

drivers/cpufreq/amd-pstate.c +18 -26

-	cppc_enabled = enable;
  	return ret;
  }
 
  DEFINE_STATIC_CALL(amd_pstate_cppc_enable, msr_cppc_enable);
 
-static inline int amd_pstate_cppc_enable(bool enable)
+static inline int amd_pstate_cppc_enable(struct cpufreq_policy *policy)
  {
-	return static_call(amd_pstate_cppc_enable)(enable);
+	return static_call(amd_pstate_cppc_enable)(policy);
  }
 
  static int msr_init_perf(struct amd_cpudata *cpudata)

@@ -1115,28 +1060,6 @@ static void amd_pstate_cpu_exit(struct cpufreq_policy *policy)
  	kfree(cpudata);
  }
 
-static int amd_pstate_cpu_resume(struct cpufreq_policy *policy)
-{
-	int ret;
-
-	ret = amd_pstate_cppc_enable(true);
-	if (ret)
-		pr_err("failed to enable amd-pstate during resume, return %d\n", ret);
-
-	return ret;
-}
-
-static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
-{
-	int ret;
-
-	ret = amd_pstate_cppc_enable(false);
-	if (ret)
-		pr_err("failed to disable amd-pstate during suspend, return %d\n", ret);
-
-	return ret;
-}
-
  /* Sysfs attributes */
 
  /*

@@ -1228,8 +1151,10 @@ static ssize_t show_energy_performance_available_preferences(
  static ssize_t store_energy_performance_preference(
  		struct cpufreq_policy *policy, const char *buf, size_t count)
  {
+	struct amd_cpudata *cpudata = policy->driver_data;
  	char str_preference[21];
  	ssize_t ret;
+	u8 epp;
 
  	ret = sscanf(buf, "%20s", str_preference);
  	if (ret != 1)

@@ -1239,7 +1164,29 @@ static ssize_t store_energy_performance_preference(
  	if (ret < 0)
  		return -EINVAL;
 
-	ret = amd_pstate_set_energy_pref_index(policy, ret);
+	if (!ret)
+		epp = cpudata->epp_default;
+	else
+		epp = epp_values[ret];
+
+	if (epp > 0 && policy->policy == CPUFREQ_POLICY_PERFORMANCE) {
+		pr_debug("EPP cannot be set under performance policy\n");
+		return -EBUSY;
+	}
+
+	if (trace_amd_pstate_epp_perf_enabled()) {
+		union perf_cached perf = cpudata->perf;
+
+		trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,

drivers/cpufreq/amd-pstate.c static ssize_t store_energy_performance_preference( · line 1186

Quoted 6 lines

drivers/cpufreq/amd-pstate.c +6 -0

+					  epp,
+					  FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cpudata->cppc_req_cached),
+					  FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cpudata->cppc_req_cached),
+					  policy->boost_enabled,
+					  FIELD_GET(AMD_CPPC_EPP_PERF_MASK,
+						    cpudata->cppc_req_cached) != epp);

We are doing the tracing in amd_pstate_set_epp() as well right?, Isnt this one redundant?

Quoted 30 lines

drivers/cpufreq/amd-pstate.c +3 -9

+	}
+
+	ret = amd_pstate_set_epp(policy, epp);
 
  	return ret ? ret : count;
  }

@@ -1272,7 +1219,6 @@ static ssize_t show_energy_performance_preference(
 
  static void amd_pstate_driver_cleanup(void)
  {
-	amd_pstate_cppc_enable(false);
  	cppc_state = AMD_PSTATE_DISABLE;
  	current_pstate_driver = NULL;
  }

@@ -1306,14 +1252,6 @@ static int amd_pstate_register_driver(int mode)
 
  	cppc_state = mode;
 
-	ret = amd_pstate_cppc_enable(true);
-	if (ret) {
-		pr_err("failed to enable cppc during amd-pstate driver registration, return %d\n",
-		       ret);
-		amd_pstate_driver_cleanup();
-		return ret;
-	}
-
  	/* at least one CPU supports CPB */
  	current_pstate_driver->boost_enabled = cpu_feature_enabled(X86_FEATURE_CPB);
 

drivers/cpufreq/amd-pstate.c static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) · line 1496

Quoted 7 lines

drivers/cpufreq/amd-pstate.c +3 -0

@@ -1553,11 +1491,15 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
  	policy->cpuinfo.max_freq = policy->max = perf_to_freq(perf,
  							      cpudata->nominal_freq,
  							      perf.highest_perf);
+	policy->driver_data = cpudata;
+
+	ret = amd_pstate_cppc_enable(policy);

I think we missed cppc_enable in "amd_pstate_cpu_init". Confirmed this by booting with "amd_pstate=passive"

Also, one weird behavior I saw while testing this part, if we boot with "amd_pstate=passive" initially, the MSR_AMD_CPPC_ENABLE register is 0. But after I run the amd-pstate-ut (which fails the check_amd_pstate_enabled() test the first time) the MSR_AMD_CPPC_ENABLE gets set to 1. But I didnt see any code in amd-pstate-ut that sets it. We can ignore this quirk for now, just mentioned to see if you have any idea about this.

Quoted 35 lines

drivers/cpufreq/amd-pstate.c +3 -14

+	if (ret)
+		goto free_cpudata1;
 
  	/* It will be updated by governor */
  	policy->cur = policy->cpuinfo.min_freq;
 
-	policy->driver_data = cpudata;
 
  	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
 

@@ -1649,31 +1591,21 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
  	return 0;
  }
 
-static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
-{
-	int ret;
-
-	ret = amd_pstate_cppc_enable(true);
-	if (ret)
-		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
-
-
-	return amd_pstate_epp_update_limit(policy);
-}
-
  static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
  {
  	struct amd_cpudata *cpudata = policy->driver_data;
  	int ret;
 
-	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
+	pr_debug("AMD CPU Core %d going online\n", policy->cpu);
 

drivers/cpufreq/amd-pstate.c static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy) · line 1605

Quoted 6 lines

drivers/cpufreq/amd-pstate.c +2 -1

-	ret = amd_pstate_epp_reenable(policy);
+	ret = amd_pstate_cppc_enable(policy);
  	if (ret)
  		return ret;
+
  	cpudata->suspended = false;

Do we need this here?, shouldn't only resume() have this statement?

Quoted 55 lines

drivers/cpufreq/amd-pstate.c +6 -13

  	return 0;
+
  }
 
  static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)

@@ -1691,11 +1623,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
  static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
  {
  	struct amd_cpudata *cpudata = policy->driver_data;
-	int ret;
-
-	/* avoid suspending when EPP is not enabled */
-	if (cppc_state != AMD_PSTATE_ACTIVE)
-		return 0;
 
  	/* invalidate to ensure it's rewritten during resume */
  	cpudata->cppc_req_cached = 0;

@@ -1703,11 +1630,6 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
  	/* set this flag to avoid setting core offline*/
  	cpudata->suspended = true;
 
-	/* disable CPPC in lowlevel firmware */
-	ret = amd_pstate_cppc_enable(false);
-	if (ret)
-		pr_err("failed to suspend, return %d\n", ret);
-
  	return 0;
  }
 

@@ -1716,8 +1638,12 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
  	struct amd_cpudata *cpudata = policy->driver_data;
 
  	if (cpudata->suspended) {
+		int ret;
+
  		/* enable amd pstate from suspend state*/
-		amd_pstate_epp_reenable(policy);
+		ret = amd_pstate_epp_update_limit(policy);
+		if (ret)
+			return ret;
 
  		cpudata->suspended = false;
  	}

@@ -1732,8 +1658,6 @@ static struct cpufreq_driver amd_pstate_driver = {
  	.fast_switch    = amd_pstate_fast_switch,
  	.init		= amd_pstate_cpu_init,
  	.exit		= amd_pstate_cpu_exit,
-	.suspend	= amd_pstate_cpu_suspend,
-	.resume		= amd_pstate_cpu_resume,
  	.set_boost	= amd_pstate_set_boost,
  	.update_limits	= amd_pstate_update_limits,
  	.name		= "amd-pstate",

drivers/cpufreq/amd-pstate.c static struct cpufreq_driver amd_pstate_epp_driver = { · line 1676

Quoted 7 lines

drivers/cpufreq/amd-pstate.c +2 -2

@@ -1748,8 +1672,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
  	.exit		= amd_pstate_epp_cpu_exit,
  	.offline	= amd_pstate_epp_cpu_offline,
  	.online		= amd_pstate_epp_cpu_online,
-	.suspend	= amd_pstate_epp_suspend,
-	.resume		= amd_pstate_epp_resume,
+	.suspend        = amd_pstate_epp_suspend,
+	.resume         = amd_pstate_epp_resume,

Spurious whitespace change?

Quoted 11 lines

drivers/cpufreq/amd-pstate.c +0 -1

  	.update_limits	= amd_pstate_update_limits,
  	.set_boost	= amd_pstate_set_boost,
  	.name		= "amd-pstate-epp",

@@ -1900,7 +1824,6 @@ static int __init amd_pstate_init(void)
 
  global_attr_free:
  	cpufreq_unregister_driver(current_pstate_driver);
-	amd_pstate_cppc_enable(false);
  	return ret;
  }
  device_initcall(amd_pstate_init);

Quoted 9 lines

Quoted 5 lines

+    /* Enable autonomous mode for EPP */
+    if (cppc_state == AMD_PSTATE_ACTIVE) {
+        /* Set desired perf as zero to allow EPP firmware control */ +        perf_ctrls.desired_perf = 0;
+        ret = cppc_set_perf(policy->cpu, &perf_ctrls);

I'm thinking do we need this "setting of desired_perf" as a part of shmem_cppc_enable,
one thing is we're not doing it in the "msr_" counterpart
also, I guess this would be taken care as part of amd_pstate_epp_set_policy()->amd_pstate_epp_update_limit()->amd_pstate_update_perf()

Great point, agreed will drop it.

Quoted 13 lines

Quoted 13 lines

      }
  -    cppc_enabled = enable;
      return ret;
  }
    DEFINE_STATIC_CALL(amd_pstate_cppc_enable, msr_cppc_enable); -static inline int amd_pstate_cppc_enable(bool enable) +static inline int amd_pstate_cppc_enable(struct cpufreq_policy *policy) {
-    return static_call(amd_pstate_cppc_enable)(enable);
+    return static_call(amd_pstate_cppc_enable)(policy);
  }
    static int msr_init_perf(struct amd_cpudata *cpudata)

Quoted 21 lines

Quoted 21 lines

  @@ -1649,31 +1591,21 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy) return 0;
  }
  -static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)

drivers/cpufreq/amd-pstate.c +0 -11

-{
-    int ret;
-
-    ret = amd_pstate_cppc_enable(true);
-    if (ret)
-        pr_err("failed to enable amd pstate during resume, return %d\n", ret);
-
-
-    return amd_pstate_epp_update_limit(policy);
-}
-

  static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy) {
      struct amd_cpudata *cpudata = policy->driver_data; int ret;
  -    pr_debug("AMD CPU Core %d going online\n", cpudata->cpu); +    pr_debug("AMD CPU Core %d going online\n", policy->cpu);

Quoted 8 lines

Quoted 6 lines

  -    ret = amd_pstate_epp_reenable(policy);
+    ret = amd_pstate_cppc_enable(policy);
      if (ret)
          return ret;
+
      cpudata->suspended = false;

Do we need this here?, shouldn't only resume() have this statement?

The reason I had in mind for it was this sequence:
* Suspend
* CPU goes offline
* CPU goes online
* Resume

But I don't think that's realistic even with parallel boot.  I will drop this.