[PATCH v2 0/3] Support for autonomous selection in cppc_cpufreq

[PATCH v2 0/3] Support for autonomous selection in cppc_cpufreq

[Lifeng Zheng]

Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq
driver.

The patch series is organized in two parts:

 - patch 1 refactor out the general CPPC register get and set functions
   in cppc_acpi.c

 - patches 2-3 expose sysfs files for users to control CPPC autonomous
   selection when supported

Change since v1:
 - fix some incorrect placeholder
 - change kstrtoul to kstrtobool in store_auto_select

Lifeng Zheng (3):
  ACPI: CPPC: Refactor register get and set ABIs
  ACPI: CPPC: Add autonomous selection ABIs
  cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

 .../ABI/testing/sysfs-devices-system-cpu      |  54 ++++
 drivers/acpi/cppc_acpi.c                      | 235 +++++++++---------
 drivers/cpufreq/cppc_cpufreq.c                | 138 ++++++++++
 include/acpi/cppc_acpi.h                      |  20 ++
 4 files changed, 328 insertions(+), 119 deletions(-)

-- 
2.33.0

[PATCH v2 1/3] ACPI: CPPC: Refactor register get and set ABIs

[Lifeng Zheng]

Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().

Rename cppc_get_perf() to cppc_get_reg() as a generic function to read cppc
registers, with two changes:

1. Change the error kind to "no such device" when pcc_ss_id < 0, which
means that this cpu cannot get a valid pcc_ss_id.

2. Add a check to verify if the register is a cpc supported one before
using it.

Add cppc_set_reg() as a generic function for setting cppc registers. Unlike
other set reg ABIs, this function checks CPC_SUPPORTED right after getting
the register, because the rest of the operations are meaningless if this
register is not a cpc supported one.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/acpi/cppc_acpi.c | 191 +++++++++++++++------------------------
 1 file changed, 72 insertions(+), 119 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index c1f3568d0c50..9aab22d8136a 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1179,10 +1179,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
 	return ret_val;
 }
 
-static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
+static int cppc_get_reg(int cpunum, enum cppc_regs reg_idx, u64 *val)
 {
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+	struct cppc_pcc_data *pcc_ss_data = NULL;
 	struct cpc_register_resource *reg;
+	int pcc_ss_id;
+	int ret = 0;
 
 	if (!cpc_desc) {
 		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
@@ -1191,20 +1194,23 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
 
 	reg = &cpc_desc->cpc_regs[reg_idx];
 
+	if (!CPC_SUPPORTED(reg)) {
+		pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
+		return -EOPNOTSUPP;
+	}
+
 	if (CPC_IN_PCC(reg)) {
-		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
-		struct cppc_pcc_data *pcc_ss_data = NULL;
-		int ret = 0;
+		pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
 
 		if (pcc_ss_id < 0)
-			return -EIO;
+			return -ENODEV;
 
 		pcc_ss_data = pcc_data[pcc_ss_id];
 
 		down_write(&pcc_ss_data->pcc_lock);
 
 		if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
-			cpc_read(cpunum, reg, perf);
+			cpc_read(cpunum, reg, val);
 		else
 			ret = -EIO;
 
@@ -1213,21 +1219,65 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
 		return ret;
 	}
 
-	cpc_read(cpunum, reg, perf);
+	cpc_read(cpunum, reg, val);
 
 	return 0;
 }
 
+static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)
+{
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	struct cpc_register_resource *reg;
+	int pcc_ss_id;
+	int ret;
+
+	if (!cpc_desc) {
+		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
+		return -ENODEV;
+	}
+
+	reg = &cpc_desc->cpc_regs[reg_idx];
+
+	if (!CPC_SUPPORTED(reg)) {
+		pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
+		return -EOPNOTSUPP;
+	}
+
+	if (CPC_IN_PCC(reg)) {
+		pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+
+		if (pcc_ss_id < 0) {
+			pr_debug("Invalid pcc_ss_id\n");
+			return -ENODEV;
+		}
+
+		ret = cpc_write(cpu, reg, val);
+		if (ret)
+			return ret;
+
+		pcc_ss_data = pcc_data[pcc_ss_id];
+
+		down_write(&pcc_ss_data->pcc_lock);
+		/* after writing CPC, transfer the ownership of PCC to platform */
+		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
+		up_write(&pcc_ss_data->pcc_lock);
+		return ret;
+	}
+
+	return cpc_write(cpu, reg, val);
+}
+
 /**
  * cppc_get_desired_perf - Get the desired performance register value.
  * @cpunum: CPU from which to get desired performance.
  * @desired_perf: Return address.
  *
- * Return: 0 for success, -EIO otherwise.
+ * Return: 0 for success, -ERRNO otherwise.
  */
 int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
 {
-	return cppc_get_perf(cpunum, DESIRED_PERF, desired_perf);
+	return cppc_get_reg(cpunum, DESIRED_PERF, desired_perf);
 }
 EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
 
@@ -1236,11 +1286,11 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
  * @cpunum: CPU from which to get nominal performance.
  * @nominal_perf: Return address.
  *
- * Return: 0 for success, -EIO otherwise.
+ * Return: 0 for success, -ERRNO otherwise.
  */
 int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
 {
-	return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
+	return cppc_get_reg(cpunum, NOMINAL_PERF, nominal_perf);
 }
 
 /**
@@ -1248,11 +1298,11 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
  * @cpunum: CPU from which to get highest performance.
  * @highest_perf: Return address.
  *
- * Return: 0 for success, -EIO otherwise.
+ * Return: 0 for success, -ERRNO otherwise.
  */
 int cppc_get_highest_perf(int cpunum, u64 *highest_perf)
 {
-	return cppc_get_perf(cpunum, HIGHEST_PERF, highest_perf);
+	return cppc_get_reg(cpunum, HIGHEST_PERF, highest_perf);
 }
 EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
 
@@ -1261,11 +1311,11 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
  * @cpunum: CPU from which to get epp preference value.
  * @epp_perf: Return address.
  *
- * Return: 0 for success, -EIO otherwise.
+ * Return: 0 for success, -ERRNO otherwise.
  */
 int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
 {
-	return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
+	return cppc_get_reg(cpunum, ENERGY_PERF, epp_perf);
 }
 EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
 
@@ -1545,44 +1595,14 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
  */
 int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 {
-	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
-	struct cpc_register_resource *auto_sel_reg;
-	u64  auto_sel;
-
-	if (!cpc_desc) {
-		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
-		return -ENODEV;
-	}
-
-	auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
-
-	if (!CPC_SUPPORTED(auto_sel_reg))
-		pr_warn_once("Autonomous mode is not unsupported!\n");
-
-	if (CPC_IN_PCC(auto_sel_reg)) {
-		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
-		struct cppc_pcc_data *pcc_ss_data = NULL;
-		int ret = 0;
-
-		if (pcc_ss_id < 0)
-			return -ENODEV;
-
-		pcc_ss_data = pcc_data[pcc_ss_id];
-
-		down_write(&pcc_ss_data->pcc_lock);
-
-		if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0) {
-			cpc_read(cpunum, auto_sel_reg, &auto_sel);
-			perf_caps->auto_sel = (bool)auto_sel;
-		} else {
-			ret = -EIO;
-		}
-
-		up_write(&pcc_ss_data->pcc_lock);
+	u64 auto_sel;
+	int ret;
 
+	ret = cppc_get_reg(cpunum, AUTO_SEL_ENABLE, &auto_sel);
+	if (ret)
 		return ret;
-	}
 
+	perf_caps->auto_sel = (bool)auto_sel;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
@@ -1594,43 +1614,7 @@ EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
  */
 int cppc_set_auto_sel(int cpu, bool enable)
 {
-	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
-	struct cpc_register_resource *auto_sel_reg;
-	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
-	struct cppc_pcc_data *pcc_ss_data = NULL;
-	int ret = -EINVAL;
-
-	if (!cpc_desc) {
-		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
-		return -ENODEV;
-	}
-
-	auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
-
-	if (CPC_IN_PCC(auto_sel_reg)) {
-		if (pcc_ss_id < 0) {
-			pr_debug("Invalid pcc_ss_id\n");
-			return -ENODEV;
-		}
-
-		if (CPC_SUPPORTED(auto_sel_reg)) {
-			ret = cpc_write(cpu, auto_sel_reg, enable);
-			if (ret)
-				return ret;
-		}
-
-		pcc_ss_data = pcc_data[pcc_ss_id];
-
-		down_write(&pcc_ss_data->pcc_lock);
-		/* after writing CPC, transfer the ownership of PCC to platform */
-		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
-		up_write(&pcc_ss_data->pcc_lock);
-	} else {
-		ret = -ENOTSUPP;
-		pr_debug("_CPC in PCC is not supported\n");
-	}
-
-	return ret;
+	return cppc_set_reg(cpu, AUTO_SEL_ENABLE, enable);
 }
 EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
 
@@ -1644,38 +1628,7 @@ EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
  */
 int cppc_set_enable(int cpu, bool enable)
 {
-	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
-	struct cpc_register_resource *enable_reg;
-	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
-	struct cppc_pcc_data *pcc_ss_data = NULL;
-	int ret = -EINVAL;
-
-	if (!cpc_desc) {
-		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
-		return -EINVAL;
-	}
-
-	enable_reg = &cpc_desc->cpc_regs[ENABLE];
-
-	if (CPC_IN_PCC(enable_reg)) {
-
-		if (pcc_ss_id < 0)
-			return -EIO;
-
-		ret = cpc_write(cpu, enable_reg, enable);
-		if (ret)
-			return ret;
-
-		pcc_ss_data = pcc_data[pcc_ss_id];
-
-		down_write(&pcc_ss_data->pcc_lock);
-		/* after writing CPC, transfer the ownership of PCC to platfrom */
-		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
-		up_write(&pcc_ss_data->pcc_lock);
-		return ret;
-	}
-
-	return cpc_write(cpu, enable_reg, enable);
+	return cppc_set_reg(cpu, ENABLE, enable);
 }
 EXPORT_SYMBOL_GPL(cppc_set_enable);
 
-- 
2.33.0

[PATCH v2 2/3] ACPI: CPPC: Add autonomous selection ABIs

[Lifeng Zheng]

cppc_set_epp - write energy performance preference register

cppc_get_auto_act_window - read autonomous activity window register

cppc_set_auto_act_window - write autonomous activity window register

cppc_get_auto_sel - read autonomous selection enable register

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/acpi/cppc_acpi.c | 44 ++++++++++++++++++++++++++++++++++++++++
 include/acpi/cppc_acpi.h | 20 ++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 9aab22d8136a..48354d7d04bb 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1588,6 +1588,50 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
 }
 EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
 
+/**
+ * cppc_set_epp() - Write the EPP register.
+ * @cpu: CPU on which to write register.
+ * @epp_val: Value to write to the EPP register.
+ */
+int cppc_set_epp(int cpu, u64 epp_val)
+{
+	return cppc_set_reg(cpu, ENERGY_PERF, epp_val);
+}
+EXPORT_SYMBOL_GPL(cppc_set_epp);
+
+/**
+ * cppc_get_auto_act_window() - Read autonomous activity window register.
+ * @cpu: CPU from which to read register.
+ * @auto_act_window: Return address.
+ */
+int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
+{
+	return cppc_get_reg(cpu, AUTO_ACT_WINDOW, auto_act_window);
+}
+EXPORT_SYMBOL_GPL(cppc_get_auto_act_window);
+
+/**
+ * cppc_set_auto_act_window() - Write autonomous activity window register.
+ * @cpu: CPU on which to write register.
+ * @auto_act_window: Value to write to the autonomous activity window register.
+ */
+int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
+{
+	return cppc_set_reg(cpu, AUTO_ACT_WINDOW, auto_act_window);
+}
+EXPORT_SYMBOL_GPL(cppc_set_auto_act_window);
+
+/**
+ * cppc_get_auto_sel() - Read autonomous selection register.
+ * @cpu: CPU from which to read register.
+ * @auto_sel: Return address.
+ */
+int cppc_get_auto_sel(int cpu, u64 *auto_sel)
+{
+	return cppc_get_reg(cpu, AUTO_SEL_ENABLE, auto_sel);
+}
+EXPORT_SYMBOL_GPL(cppc_get_auto_sel);
+
 /**
  * cppc_get_auto_sel_caps - Read autonomous selection register.
  * @cpunum : CPU from which to read register.
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 62d368bcd9ec..134931b081a0 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -159,6 +159,10 @@ extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
 extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
 extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf);
 extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
+extern int cppc_set_epp(int cpu, u64 epp_val);
+extern int cppc_get_auto_act_window(int cpu, u64 *auto_act_window);
+extern int cppc_set_auto_act_window(int cpu, u64 auto_act_window);
+extern int cppc_get_auto_sel(int cpu, u64 *auto_sel);
 extern int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
 extern int cppc_set_auto_sel(int cpu, bool enable);
 extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
@@ -225,6 +229,22 @@ static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls,
 {
 	return -EOPNOTSUPP;
 }
+static inline int cppc_set_epp(int cpu, u64 epp_val)
+{
+	return -EOPNOTSUPP;
+}
+static inline int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
+{
+	return -EOPNOTSUPP;
+}
+static inline int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
+{
+	return -EOPNOTSUPP;
+}
+static inline int cppc_get_auto_sel(int cpu, u64 *auto_sel)
+{
+	return -EOPNOTSUPP;
+}
 static inline int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
 {
 	return -EOPNOTSUPP;
-- 
2.33.0

[PATCH v2 3/3] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Lifeng Zheng]

Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq
driver.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 .../ABI/testing/sysfs-devices-system-cpu      |  54 +++++++
 drivers/cpufreq/cppc_cpufreq.c                | 138 ++++++++++++++++++
 2 files changed, 192 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index 206079d3bd5b..3d87c3bb3fe2 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
@@ -268,6 +268,60 @@ Description:	Discover CPUs in the same CPU frequency coordination domain
 		This file is only present if the acpi-cpufreq or the cppc-cpufreq
 		drivers are in use.
 
+What:		/sys/devices/system/cpu/cpuX/cpufreq/auto_select
+Date:		October 2024
+Contact:	linux-pm@vger.kernel.org
+Description:	Autonomous selection enable
+
+		Read/write interface to control autonomous selection enable
+			Read returns autonomous selection status:
+				0: autonomous selection is disabled
+				1: autonomous selection is enabled
+
+			Write 'y' or '1' or 'on' to enable autonomous selection.
+			Write 'n' or '0' or 'off' to disable autonomous selection.
+
+		This file only presents if the cppc-cpufreq driver is in use.
+
+What:		/sys/devices/system/cpu/cpuX/cpufreq/auto_act_window
+Date:		October 2024
+Contact:	linux-pm@vger.kernel.org
+Description:	Autonomous activity window
+
+		This file indicates a moving utilization sensitivity window to
+		the platform's autonomous selection policy.
+
+		Read/write an integer represents autonomous activity window (in
+		microseconds) from/to this file. The max value to write is
+		1270000000 but the max significand is 127. This means that if 128
+		is written to this file, 127 will be stored. If the value is
+		greater than 130, only the first two digits will be saved as
+		significand.
+
+		Writing a zero value to this file enable the platform to
+		determine an appropriate Activity Window depending on the workload.
+
+		Writing to this file only has meaning when Autonomous Selection is
+		enabled.
+
+		This file only presents if the cppc-cpufreq driver is in use.
+
+What:		/sys/devices/system/cpu/cpuX/cpufreq/energy_perf
+Date:		October 2024
+Contact:	linux-pm@vger.kernel.org
+Description:	Energy performance preference
+
+		Read/write an 8-bit integer from/to this file. This file
+		represents a range of values from 0 (performance preference) to
+		0xFF (energy efficiency preference) that influences the rate of
+		performance increase/decrease and the result of the hardware's
+		energy efficiency and performance optimization policies.
+
+		Writing to this file only has meaning when Autonomous Selection is
+		enabled.
+
+		This file only presents if the cppc-cpufreq driver is in use.
+
 
 What:		/sys/devices/system/cpu/cpu*/cache/index3/cache_disable_{0,1}
 Date:		August 2008
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 2b8708475ac7..351ecdd49ad4 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -792,10 +792,148 @@ static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
 
 	return cpufreq_show_cpus(cpu_data->shared_cpu_map, buf);
 }
+
+static ssize_t show_auto_select(struct cpufreq_policy *policy, char *buf)
+{
+	u64 val;
+	int ret;
+
+	ret = cppc_get_auto_sel(policy->cpu, &val);
+
+	/* show "<unsupported>" when this register is not supported by cpc */
+	if (ret == -EOPNOTSUPP)
+		return sysfs_emit(buf, "%s\n", "<unsupported>");
+
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%llu\n", val);
+}
+
+static ssize_t store_auto_select(struct cpufreq_policy *policy,
+				 const char *buf, size_t count)
+{
+	bool val;
+	int ret;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	ret = cppc_set_auto_sel(policy->cpu, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+#define AUTO_ACT_WINDOW_SIG_BIT_SIZE	(7)
+#define AUTO_ACT_WINDOW_EXP_BIT_SIZE	(3)
+#define AUTO_ACT_WINDOW_MAX_SIG	((1 << AUTO_ACT_WINDOW_SIG_BIT_SIZE) - 1)
+#define AUTO_ACT_WINDOW_MAX_EXP	((1 << AUTO_ACT_WINDOW_EXP_BIT_SIZE) - 1)
+/* AUTO_ACT_WINDOW_MAX_SIG is 127, so 128 and 129 will decay to 127 when writing */
+#define AUTO_ACT_WINDOW_SIG_CARRY_THRESH 129
+
+static ssize_t show_auto_act_window(struct cpufreq_policy *policy, char *buf)
+{
+	unsigned int exp;
+	u64 val, sig;
+	int ret;
+
+	ret = cppc_get_auto_act_window(policy->cpu, &val);
+
+	/* show "<unsupported>" when this register is not supported by cpc */
+	if (ret == -EOPNOTSUPP)
+		return sysfs_emit(buf, "%s\n", "<unsupported>");
+
+	if (ret)
+		return ret;
+
+	sig = val & AUTO_ACT_WINDOW_MAX_SIG;
+	exp = (val >> AUTO_ACT_WINDOW_SIG_BIT_SIZE) & AUTO_ACT_WINDOW_MAX_EXP;
+
+	return sysfs_emit(buf, "%llu\n", sig * int_pow(10, exp));
+}
+
+static ssize_t store_auto_act_window(struct cpufreq_policy *policy,
+				     const char *buf, size_t count)
+{
+	unsigned long usec;
+	int digits = 0;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &usec);
+	if (ret)
+		return ret;
+
+	if (usec > AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, AUTO_ACT_WINDOW_MAX_EXP))
+		return -EINVAL;
+
+	while (usec > AUTO_ACT_WINDOW_SIG_CARRY_THRESH) {
+		usec /= 10;
+		digits += 1;
+	}
+
+	if (usec > AUTO_ACT_WINDOW_MAX_SIG)
+		usec = AUTO_ACT_WINDOW_MAX_SIG;
+
+	ret = cppc_set_auto_act_window(policy->cpu,
+				       (digits << AUTO_ACT_WINDOW_SIG_BIT_SIZE) + usec);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t show_energy_perf(struct cpufreq_policy *policy, char *buf)
+{
+	u64 val;
+	int ret;
+
+	ret = cppc_get_epp_perf(policy->cpu, &val);
+
+	/* show "<unsupported>" when this register is not supported by cpc */
+	if (ret == -EOPNOTSUPP)
+		return sysfs_emit(buf, "%s\n", "<unsupported>");
+
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%llu\n", val);
+}
+
+#define ENERGY_PERF_MAX	(0xFF)
+
+static ssize_t store_energy_perf(struct cpufreq_policy *policy,
+				 const char *buf, size_t count)
+{
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val > ENERGY_PERF_MAX)
+		return -EINVAL;
+
+	ret = cppc_set_epp(policy->cpu, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
 cpufreq_freq_attr_ro(freqdomain_cpus);
+cpufreq_freq_attr_rw(auto_select);
+cpufreq_freq_attr_rw(auto_act_window);
+cpufreq_freq_attr_rw(energy_perf);
 
 static struct freq_attr *cppc_cpufreq_attr[] = {
 	&freqdomain_cpus,
+	&auto_select,
+	&auto_act_window,
+	&energy_perf,
 	NULL,
 };
 
-- 
2.33.0

Re: [PATCH v2 0/3] Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

Gentle ping.
+cc a few developers

On 2024/11/22 14:20, Lifeng Zheng wrote:
> Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq
> driver.
> 
> The patch series is organized in two parts:
> 
>  - patch 1 refactor out the general CPPC register get and set functions
>    in cppc_acpi.c
> 
>  - patches 2-3 expose sysfs files for users to control CPPC autonomous
>    selection when supported
> 
> Change since v1:
>  - fix some incorrect placeholder
>  - change kstrtoul to kstrtobool in store_auto_select
> 
> Lifeng Zheng (3):
>   ACPI: CPPC: Refactor register get and set ABIs
>   ACPI: CPPC: Add autonomous selection ABIs
>   cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq
> 
>  .../ABI/testing/sysfs-devices-system-cpu      |  54 ++++
>  drivers/acpi/cppc_acpi.c                      | 235 +++++++++---------
>  drivers/cpufreq/cppc_cpufreq.c                | 138 ++++++++++
>  include/acpi/cppc_acpi.h                      |  20 ++
>  4 files changed, 328 insertions(+), 119 deletions(-)
> 

Re: [PATCH v2 1/3] ACPI: CPPC: Refactor register get and set ABIs

[Rafael J. Wysocki]

On Fri, Nov 22, 2024 at 7:21 AM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().

I don't quite like the cppc_get_reg() name.  I think that
cppc_get_reg_val() would be better.

> Rename cppc_get_perf() to cppc_get_reg() as a generic function to read cppc
> registers, with two changes:
>
> 1. Change the error kind to "no such device" when pcc_ss_id < 0, which
> means that this cpu cannot get a valid pcc_ss_id.
>
> 2. Add a check to verify if the register is a cpc supported one before
> using it.

So it's not just a rename, but also a change in behavior.  Can this
change in behavior become user-visible?

> Add cppc_set_reg() as a generic function for setting cppc registers.

Again, I would prefer cppc_set_reg_val().

> Unlike other set reg ABIs, this function checks CPC_SUPPORTED right after getting
> the register, because the rest of the operations are meaningless if this
> register is not a cpc supported one.

And the new function is used to reduce some existing code duplication,
isn't it?  Which would be good to mention here.

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/acpi/cppc_acpi.c | 191 +++++++++++++++------------------------
>  1 file changed, 72 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index c1f3568d0c50..9aab22d8136a 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1179,10 +1179,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
>         return ret_val;
>  }
>
> -static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
> +static int cppc_get_reg(int cpunum, enum cppc_regs reg_idx, u64 *val)
>  {
>         struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> +       struct cppc_pcc_data *pcc_ss_data = NULL;

Why are you moving this here?  This change is not related to the rest
of the patch, is it?

>         struct cpc_register_resource *reg;
> +       int pcc_ss_id;
> +       int ret = 0;

And here?

>
>         if (!cpc_desc) {
>                 pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> @@ -1191,20 +1194,23 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>
>         reg = &cpc_desc->cpc_regs[reg_idx];
>
> +       if (!CPC_SUPPORTED(reg)) {
> +               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> +               return -EOPNOTSUPP;
> +       }
> +
>         if (CPC_IN_PCC(reg)) {
> -               int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> -               struct cppc_pcc_data *pcc_ss_data = NULL;
> -               int ret = 0;
> +               pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>
>                 if (pcc_ss_id < 0)
> -                       return -EIO;
> +                       return -ENODEV;
>
>                 pcc_ss_data = pcc_data[pcc_ss_id];
>
>                 down_write(&pcc_ss_data->pcc_lock);
>
>                 if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
> -                       cpc_read(cpunum, reg, perf);
> +                       cpc_read(cpunum, reg, val);
>                 else
>                         ret = -EIO;
>
> @@ -1213,21 +1219,65 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>                 return ret;
>         }
>
> -       cpc_read(cpunum, reg, perf);
> +       cpc_read(cpunum, reg, val);
>
>         return 0;
>  }
>
> +static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)
> +{
> +       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> +       struct cppc_pcc_data *pcc_ss_data = NULL;
> +       struct cpc_register_resource *reg;
> +       int pcc_ss_id;
> +       int ret;
> +
> +       if (!cpc_desc) {
> +               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> +               return -ENODEV;
> +       }
> +
> +       reg = &cpc_desc->cpc_regs[reg_idx];
> +
> +       if (!CPC_SUPPORTED(reg)) {
> +               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> +               return -EOPNOTSUPP;
> +       }
> +
> +       if (CPC_IN_PCC(reg)) {
> +               pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);

Please declare the variables that are only needed in the PCC case here.

Also, I think it would be better to define a new function, say
cppc_set_reg_val_in_pcc() for this code and then have

if (CPC_IN_PCC(reg))
        return cppc_set_reg_val_in_pcc(reg, val);

> +
> +               if (pcc_ss_id < 0) {
> +                       pr_debug("Invalid pcc_ss_id\n");
> +                       return -ENODEV;
> +               }
> +
> +               ret = cpc_write(cpu, reg, val);
> +               if (ret)
> +                       return ret;
> +
> +               pcc_ss_data = pcc_data[pcc_ss_id];
> +
> +               down_write(&pcc_ss_data->pcc_lock);
> +               /* after writing CPC, transfer the ownership of PCC to platform */
> +               ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> +               up_write(&pcc_ss_data->pcc_lock);
> +               return ret;
> +       }
> +
> +       return cpc_write(cpu, reg, val);
> +}
> +
>  /**
>   * cppc_get_desired_perf - Get the desired performance register value.
>   * @cpunum: CPU from which to get desired performance.
>   * @desired_perf: Return address.
>   *
> - * Return: 0 for success, -EIO otherwise.
> + * Return: 0 for success, -ERRNO otherwise.
>   */
>  int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
>  {
> -       return cppc_get_perf(cpunum, DESIRED_PERF, desired_perf);
> +       return cppc_get_reg(cpunum, DESIRED_PERF, desired_perf);
>  }
>  EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>
> @@ -1236,11 +1286,11 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>   * @cpunum: CPU from which to get nominal performance.
>   * @nominal_perf: Return address.
>   *
> - * Return: 0 for success, -EIO otherwise.
> + * Return: 0 for success, -ERRNO otherwise.

What do you mean by ERRNO?

>   */
>  int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>  {
> -       return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
> +       return cppc_get_reg(cpunum, NOMINAL_PERF, nominal_perf);
>  }
>
>  /**
> @@ -1248,11 +1298,11 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>   * @cpunum: CPU from which to get highest performance.
>   * @highest_perf: Return address.
>   *
> - * Return: 0 for success, -EIO otherwise.
> + * Return: 0 for success, -ERRNO otherwise.
>   */
>  int cppc_get_highest_perf(int cpunum, u64 *highest_perf)
>  {
> -       return cppc_get_perf(cpunum, HIGHEST_PERF, highest_perf);
> +       return cppc_get_reg(cpunum, HIGHEST_PERF, highest_perf);
>  }
>  EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>
> @@ -1261,11 +1311,11 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>   * @cpunum: CPU from which to get epp preference value.
>   * @epp_perf: Return address.
>   *
> - * Return: 0 for success, -EIO otherwise.
> + * Return: 0 for success, -ERRNO otherwise.

Same here?

>   */
>  int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>  {
> -       return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
> +       return cppc_get_reg(cpunum, ENERGY_PERF, epp_perf);
>  }
>  EXPORT_SYMBOL_GPL(cppc_get_epp_perf);

It would be cleaner to do the changes below in a separate patch IMV.

> @@ -1545,44 +1595,14 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>   */
>  int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>  {
> -       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> -       struct cpc_register_resource *auto_sel_reg;
> -       u64  auto_sel;
> -
> -       if (!cpc_desc) {
> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> -               return -ENODEV;
> -       }
> -
> -       auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
> -
> -       if (!CPC_SUPPORTED(auto_sel_reg))
> -               pr_warn_once("Autonomous mode is not unsupported!\n");
> -
> -       if (CPC_IN_PCC(auto_sel_reg)) {
> -               int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> -               struct cppc_pcc_data *pcc_ss_data = NULL;
> -               int ret = 0;
> -
> -               if (pcc_ss_id < 0)
> -                       return -ENODEV;
> -
> -               pcc_ss_data = pcc_data[pcc_ss_id];
> -
> -               down_write(&pcc_ss_data->pcc_lock);
> -
> -               if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0) {
> -                       cpc_read(cpunum, auto_sel_reg, &auto_sel);
> -                       perf_caps->auto_sel = (bool)auto_sel;
> -               } else {
> -                       ret = -EIO;
> -               }
> -
> -               up_write(&pcc_ss_data->pcc_lock);
> +       u64 auto_sel;
> +       int ret;
>
> +       ret = cppc_get_reg(cpunum, AUTO_SEL_ENABLE, &auto_sel);
> +       if (ret)
>                 return ret;
> -       }
>
> +       perf_caps->auto_sel = (bool)auto_sel;
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
> @@ -1594,43 +1614,7 @@ EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
>   */
>  int cppc_set_auto_sel(int cpu, bool enable)
>  {
> -       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> -       struct cpc_register_resource *auto_sel_reg;
> -       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> -       struct cppc_pcc_data *pcc_ss_data = NULL;
> -       int ret = -EINVAL;
> -
> -       if (!cpc_desc) {
> -               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> -               return -ENODEV;
> -       }
> -
> -       auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
> -
> -       if (CPC_IN_PCC(auto_sel_reg)) {
> -               if (pcc_ss_id < 0) {
> -                       pr_debug("Invalid pcc_ss_id\n");
> -                       return -ENODEV;
> -               }
> -
> -               if (CPC_SUPPORTED(auto_sel_reg)) {
> -                       ret = cpc_write(cpu, auto_sel_reg, enable);
> -                       if (ret)
> -                               return ret;
> -               }
> -
> -               pcc_ss_data = pcc_data[pcc_ss_id];
> -
> -               down_write(&pcc_ss_data->pcc_lock);
> -               /* after writing CPC, transfer the ownership of PCC to platform */
> -               ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> -               up_write(&pcc_ss_data->pcc_lock);
> -       } else {
> -               ret = -ENOTSUPP;
> -               pr_debug("_CPC in PCC is not supported\n");
> -       }
> -
> -       return ret;
> +       return cppc_set_reg(cpu, AUTO_SEL_ENABLE, enable);
>  }
>  EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>
> @@ -1644,38 +1628,7 @@ EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>   */
>  int cppc_set_enable(int cpu, bool enable)
>  {
> -       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> -       struct cpc_register_resource *enable_reg;
> -       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> -       struct cppc_pcc_data *pcc_ss_data = NULL;
> -       int ret = -EINVAL;
> -
> -       if (!cpc_desc) {
> -               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> -               return -EINVAL;
> -       }
> -
> -       enable_reg = &cpc_desc->cpc_regs[ENABLE];
> -
> -       if (CPC_IN_PCC(enable_reg)) {
> -
> -               if (pcc_ss_id < 0)
> -                       return -EIO;
> -
> -               ret = cpc_write(cpu, enable_reg, enable);
> -               if (ret)
> -                       return ret;
> -
> -               pcc_ss_data = pcc_data[pcc_ss_id];
> -
> -               down_write(&pcc_ss_data->pcc_lock);
> -               /* after writing CPC, transfer the ownership of PCC to platfrom */
> -               ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> -               up_write(&pcc_ss_data->pcc_lock);
> -               return ret;
> -       }
> -
> -       return cpc_write(cpu, enable_reg, enable);
> +       return cppc_set_reg(cpu, ENABLE, enable);
>  }
>  EXPORT_SYMBOL_GPL(cppc_set_enable);
>
> --

Re: [PATCH v2 1/3] ACPI: CPPC: Refactor register get and set ABIs

[zhenglifeng (A)]

On 2024/12/11 2:14, Rafael J. Wysocki wrote:
> On Fri, Nov 22, 2024 at 7:21 AM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().
> 
> I don't quite like the cppc_get_reg() name.  I think that
> cppc_get_reg_val() would be better.

Indeed, it is better. Will change. Thanks.

> 
>> Rename cppc_get_perf() to cppc_get_reg() as a generic function to read cppc
>> registers, with two changes:
>>
>> 1. Change the error kind to "no such device" when pcc_ss_id < 0, which
>> means that this cpu cannot get a valid pcc_ss_id.
>>
>> 2. Add a check to verify if the register is a cpc supported one before
>> using it.
> 
> So it's not just a rename, but also a change in behavior.  Can this
> change in behavior become user-visible?

The register value get ABIs in this file returned different error numbers
when pcc_ss_id < 0, but should be the same one. So I chose a most suitable
one I thought to be returned here when doing refactoring. This change is
not user-visible as I know.

It is necessary to do the CPC_SUPPORTED() check before using the register.
If it is not a cpc supported one, the rest of the operation is pointless
and may be dangerous. This change might be user-visible but is still
necessary.

> 
>> Add cppc_set_reg() as a generic function for setting cppc registers.
> 
> Again, I would prefer cppc_set_reg_val().
> 
>> Unlike other set reg ABIs, this function checks CPC_SUPPORTED right after getting
>> the register, because the rest of the operations are meaningless if this
>> register is not a cpc supported one.
> 
> And the new function is used to reduce some existing code duplication,
> isn't it?  Which would be good to mention here.

Yes, Will mention it in next version. Thanks.

> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/acpi/cppc_acpi.c | 191 +++++++++++++++------------------------
>>  1 file changed, 72 insertions(+), 119 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index c1f3568d0c50..9aab22d8136a 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1179,10 +1179,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
>>         return ret_val;
>>  }
>>
>> -static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>> +static int cppc_get_reg(int cpunum, enum cppc_regs reg_idx, u64 *val)
>>  {
>>         struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> +       struct cppc_pcc_data *pcc_ss_data = NULL;
> 
> Why are you moving this here?  This change is not related to the rest
> of the patch, is it?
> 
>>         struct cpc_register_resource *reg;
>> +       int pcc_ss_id;
>> +       int ret = 0;
> 
> And here?

Moving these because I'm used to declare variables at the beginning of a
function. It's really unnecessary. After defining new functions as
cppc_get_reg_val_in_pcc() and cppc_set_reg_val_in_pcc() as you suggest
below, these variables will be moved to the new functions.

> 
>>
>>         if (!cpc_desc) {
>>                 pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>> @@ -1191,20 +1194,23 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>
>>         reg = &cpc_desc->cpc_regs[reg_idx];
>>
>> +       if (!CPC_SUPPORTED(reg)) {
>> +               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>>         if (CPC_IN_PCC(reg)) {
>> -               int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>> -               struct cppc_pcc_data *pcc_ss_data = NULL;
>> -               int ret = 0;
>> +               pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>>
>>                 if (pcc_ss_id < 0)
>> -                       return -EIO;
>> +                       return -ENODEV;
>>
>>                 pcc_ss_data = pcc_data[pcc_ss_id];
>>
>>                 down_write(&pcc_ss_data->pcc_lock);
>>
>>                 if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>> -                       cpc_read(cpunum, reg, perf);
>> +                       cpc_read(cpunum, reg, val);
>>                 else
>>                         ret = -EIO;
>>
>> @@ -1213,21 +1219,65 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>                 return ret;
>>         }
>>
>> -       cpc_read(cpunum, reg, perf);
>> +       cpc_read(cpunum, reg, val);
>>
>>         return 0;
>>  }
>>
>> +static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)
>> +{
>> +       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> +       struct cppc_pcc_data *pcc_ss_data = NULL;
>> +       struct cpc_register_resource *reg;
>> +       int pcc_ss_id;
>> +       int ret;
>> +
>> +       if (!cpc_desc) {
>> +               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> +               return -ENODEV;
>> +       }
>> +
>> +       reg = &cpc_desc->cpc_regs[reg_idx];
>> +
>> +       if (!CPC_SUPPORTED(reg)) {
>> +               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>> +       if (CPC_IN_PCC(reg)) {
>> +               pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> 
> Please declare the variables that are only needed in the PCC case here.
> 
> Also, I think it would be better to define a new function, say
> cppc_set_reg_val_in_pcc() for this code and then have
> 
> if (CPC_IN_PCC(reg))
>         return cppc_set_reg_val_in_pcc(reg, val);

Will define new functions as cppc_get_reg_val_in_pcc() and
cppc_set_reg_val_in_pcc(). Thanks.

> 
>> +
>> +               if (pcc_ss_id < 0) {
>> +                       pr_debug("Invalid pcc_ss_id\n");
>> +                       return -ENODEV;
>> +               }
>> +
>> +               ret = cpc_write(cpu, reg, val);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               pcc_ss_data = pcc_data[pcc_ss_id];
>> +
>> +               down_write(&pcc_ss_data->pcc_lock);
>> +               /* after writing CPC, transfer the ownership of PCC to platform */
>> +               ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>> +               up_write(&pcc_ss_data->pcc_lock);
>> +               return ret;
>> +       }
>> +
>> +       return cpc_write(cpu, reg, val);
>> +}
>> +
>>  /**
>>   * cppc_get_desired_perf - Get the desired performance register value.
>>   * @cpunum: CPU from which to get desired performance.
>>   * @desired_perf: Return address.
>>   *
>> - * Return: 0 for success, -EIO otherwise.
>> + * Return: 0 for success, -ERRNO otherwise.
>>   */
>>  int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
>>  {
>> -       return cppc_get_perf(cpunum, DESIRED_PERF, desired_perf);
>> +       return cppc_get_reg(cpunum, DESIRED_PERF, desired_perf);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>>
>> @@ -1236,11 +1286,11 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>>   * @cpunum: CPU from which to get nominal performance.
>>   * @nominal_perf: Return address.
>>   *
>> - * Return: 0 for success, -EIO otherwise.
>> + * Return: 0 for success, -ERRNO otherwise.
> 
> What do you mean by ERRNO?

Error number. I see this expression elsewhere in this file so I use it too.

> 
>>   */
>>  int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>>  {
>> -       return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
>> +       return cppc_get_reg(cpunum, NOMINAL_PERF, nominal_perf);
>>  }
>>
>>  /**
>> @@ -1248,11 +1298,11 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>>   * @cpunum: CPU from which to get highest performance.
>>   * @highest_perf: Return address.
>>   *
>> - * Return: 0 for success, -EIO otherwise.
>> + * Return: 0 for success, -ERRNO otherwise.
>>   */
>>  int cppc_get_highest_perf(int cpunum, u64 *highest_perf)
>>  {
>> -       return cppc_get_perf(cpunum, HIGHEST_PERF, highest_perf);
>> +       return cppc_get_reg(cpunum, HIGHEST_PERF, highest_perf);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>
>> @@ -1261,11 +1311,11 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>   * @cpunum: CPU from which to get epp preference value.
>>   * @epp_perf: Return address.
>>   *
>> - * Return: 0 for success, -EIO otherwise.
>> + * Return: 0 for success, -ERRNO otherwise.
> 
> Same here?
> 
>>   */
>>  int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>>  {
>> -       return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
>> +       return cppc_get_reg(cpunum, ENERGY_PERF, epp_perf);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
> 
> It would be cleaner to do the changes below in a separate patch IMV.

Will separate it. Thanks.

> 
>> @@ -1545,44 +1595,14 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>>   */
>>  int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>>  {
>> -       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> -       struct cpc_register_resource *auto_sel_reg;
>> -       u64  auto_sel;
>> -
>> -       if (!cpc_desc) {
>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>> -               return -ENODEV;
>> -       }
>> -
>> -       auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>> -
>> -       if (!CPC_SUPPORTED(auto_sel_reg))
>> -               pr_warn_once("Autonomous mode is not unsupported!\n");
>> -
>> -       if (CPC_IN_PCC(auto_sel_reg)) {
>> -               int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>> -               struct cppc_pcc_data *pcc_ss_data = NULL;
>> -               int ret = 0;
>> -
>> -               if (pcc_ss_id < 0)
>> -                       return -ENODEV;
>> -
>> -               pcc_ss_data = pcc_data[pcc_ss_id];
>> -
>> -               down_write(&pcc_ss_data->pcc_lock);
>> -
>> -               if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0) {
>> -                       cpc_read(cpunum, auto_sel_reg, &auto_sel);
>> -                       perf_caps->auto_sel = (bool)auto_sel;
>> -               } else {
>> -                       ret = -EIO;
>> -               }
>> -
>> -               up_write(&pcc_ss_data->pcc_lock);
>> +       u64 auto_sel;
>> +       int ret;
>>
>> +       ret = cppc_get_reg(cpunum, AUTO_SEL_ENABLE, &auto_sel);
>> +       if (ret)
>>                 return ret;
>> -       }
>>
>> +       perf_caps->auto_sel = (bool)auto_sel;
>>         return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
>> @@ -1594,43 +1614,7 @@ EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
>>   */
>>  int cppc_set_auto_sel(int cpu, bool enable)
>>  {
>> -       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> -       struct cpc_register_resource *auto_sel_reg;
>> -       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> -       struct cppc_pcc_data *pcc_ss_data = NULL;
>> -       int ret = -EINVAL;
>> -
>> -       if (!cpc_desc) {
>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> -               return -ENODEV;
>> -       }
>> -
>> -       auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>> -
>> -       if (CPC_IN_PCC(auto_sel_reg)) {
>> -               if (pcc_ss_id < 0) {
>> -                       pr_debug("Invalid pcc_ss_id\n");
>> -                       return -ENODEV;
>> -               }
>> -
>> -               if (CPC_SUPPORTED(auto_sel_reg)) {
>> -                       ret = cpc_write(cpu, auto_sel_reg, enable);
>> -                       if (ret)
>> -                               return ret;
>> -               }
>> -
>> -               pcc_ss_data = pcc_data[pcc_ss_id];
>> -
>> -               down_write(&pcc_ss_data->pcc_lock);
>> -               /* after writing CPC, transfer the ownership of PCC to platform */
>> -               ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>> -               up_write(&pcc_ss_data->pcc_lock);
>> -       } else {
>> -               ret = -ENOTSUPP;
>> -               pr_debug("_CPC in PCC is not supported\n");
>> -       }
>> -
>> -       return ret;
>> +       return cppc_set_reg(cpu, AUTO_SEL_ENABLE, enable);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>>
>> @@ -1644,38 +1628,7 @@ EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>>   */
>>  int cppc_set_enable(int cpu, bool enable)
>>  {
>> -       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> -       struct cpc_register_resource *enable_reg;
>> -       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> -       struct cppc_pcc_data *pcc_ss_data = NULL;
>> -       int ret = -EINVAL;
>> -
>> -       if (!cpc_desc) {
>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> -               return -EINVAL;
>> -       }
>> -
>> -       enable_reg = &cpc_desc->cpc_regs[ENABLE];
>> -
>> -       if (CPC_IN_PCC(enable_reg)) {
>> -
>> -               if (pcc_ss_id < 0)
>> -                       return -EIO;
>> -
>> -               ret = cpc_write(cpu, enable_reg, enable);
>> -               if (ret)
>> -                       return ret;
>> -
>> -               pcc_ss_data = pcc_data[pcc_ss_id];
>> -
>> -               down_write(&pcc_ss_data->pcc_lock);
>> -               /* after writing CPC, transfer the ownership of PCC to platfrom */
>> -               ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>> -               up_write(&pcc_ss_data->pcc_lock);
>> -               return ret;
>> -       }
>> -
>> -       return cpc_write(cpu, enable_reg, enable);
>> +       return cppc_set_reg(cpu, ENABLE, enable);
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_set_enable);
>>
>> --
>