[PATCH v3 0/4] Support for autonomous selection in cppc_cpufreq

[PATCH v3 0/4] 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-2 refactor out the general CPPC register get and set functions
   in cppc_acpi.c

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

Since Pierre and me have discussed about whether or not to show
auto_act_window and energy_perf when auto_select is disabled. It seems
like whether to show these two files has their own points. We'd like to
ask for some advice.

Relevant discussion:
[1] https://lore.kernel.org/all/522721da-1a5c-439c-96a8-d0300dd0f906@huawei.com/

Change since v2:
 - change cppc_get_reg() and cppc_set_reg() name to cppc_get_reg_val() and
   cppc_set_reg_val()
 - extract cppc_get_reg_val_in_pcc() and cppc_set_reg_val_in_pcc()
 - return the result of cpc_read() in cppc_get_reg_val()
 - add pr_debug() in cppc_get_reg_val_in_pcc() when pcc_ss_id < 0
 - rename 'cpunum' to 'cpu' in cppc_get_reg_val()
 - move some macros from drivers/cpufreq/cppc_cpufreq.c to
   include/acpi/cppc_acpi.h with a CPPC_XXX prefix

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

Lifeng Zheng (4):
  ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function
  ACPI: CPPC: Refactor register value 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                      | 266 +++++++++---------
 drivers/cpufreq/cppc_cpufreq.c                | 129 +++++++++
 include/acpi/cppc_acpi.h                      |  29 ++
 4 files changed, 347 insertions(+), 131 deletions(-)

-- 
2.33.0

[PATCH v3 1/4] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[Lifeng Zheng]

Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
cppc registers, with four 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.

3. Extract the operations if register is in pcc out as
cppc_get_reg_val_in_pcc().

4. Return the result of cpc_read() instead of 0.

Add cppc_set_reg_val_in_pcc() and cppc_set_reg_val() as generic functions
for setting cppc registers value. Unlike other set reg ABIs,
cppc_set_reg_val() 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.

These functions can be used to reduce some existing code duplication.

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index c1f3568d0c50..bb5333a503a2 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1179,43 +1179,100 @@ 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_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val)
 {
-	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret;
+
+	if (pcc_ss_id < 0) {
+		pr_debug("Invalid pcc_ss_id\n");
+		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)
+		ret = cpc_read(cpu, reg, val);
+	else
+		ret = -EIO;
+
+	up_write(&pcc_ss_data->pcc_lock);
+
+	return ret;
+}
+
+static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val)
+{
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
 	struct cpc_register_resource *reg;
 
 	if (!cpc_desc) {
-		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
+		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
 		return -ENODEV;
 	}
 
 	reg = &cpc_desc->cpc_regs[reg_idx];
 
-	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;
-
-		if (pcc_ss_id < 0)
-			return -EIO;
+	if (!CPC_SUPPORTED(reg)) {
+		pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
+		return -EOPNOTSUPP;
+	}
 
-		pcc_ss_data = pcc_data[pcc_ss_id];
+	if (CPC_IN_PCC(reg))
+		return cppc_get_reg_val_in_pcc(cpu, reg, val);
 
-		down_write(&pcc_ss_data->pcc_lock);
+	return cpc_read(cpu, reg, val);
+}
 
-		if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
-			cpc_read(cpunum, reg, perf);
-		else
-			ret = -EIO;
+static int cppc_set_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 val)
+{
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret;
 
-		up_write(&pcc_ss_data->pcc_lock);
+	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;
+}
+
+static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
+{
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+	struct cpc_register_resource *reg;
+
+	if (!cpc_desc) {
+		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
+		return -ENODEV;
 	}
 
-	cpc_read(cpunum, reg, perf);
+	reg = &cpc_desc->cpc_regs[reg_idx];
 
-	return 0;
+	if (!CPC_SUPPORTED(reg)) {
+		pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
+		return -EOPNOTSUPP;
+	}
+
+	if (CPC_IN_PCC(reg))
+		return cppc_set_reg_val_in_pcc(cpu, reg, val);
+
+	return cpc_write(cpu, reg, val);
 }
 
 /**
@@ -1223,11 +1280,11 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
  * @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_val(cpunum, DESIRED_PERF, desired_perf);
 }
 EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
 
@@ -1236,11 +1293,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_val(cpunum, NOMINAL_PERF, nominal_perf);
 }
 
 /**
@@ -1248,11 +1305,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_val(cpunum, HIGHEST_PERF, highest_perf);
 }
 EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
 
@@ -1261,11 +1318,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_val(cpunum, ENERGY_PERF, epp_perf);
 }
 EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
 
-- 
2.33.0

[PATCH v3 2/4] ACPI: CPPC: Refactor register value get and set ABIs

[Lifeng Zheng]

Refactor register value get and set ABIs by using cppc_get_reg_val() and
cppc_set_reg_val().

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index bb5333a503a2..83c7fcad74ad 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1602,44 +1602,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_val(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);
@@ -1651,43 +1621,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_val(cpu, AUTO_SEL_ENABLE, enable);
 }
 EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
 
@@ -1701,38 +1635,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_val(cpu, ENABLE, enable);
 }
 EXPORT_SYMBOL_GPL(cppc_set_enable);
 
-- 
2.33.0

[PATCH v3 3/4] 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 83c7fcad74ad..645f2366c888 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1595,6 +1595,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_val(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_val(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_val(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_val(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 v3 4/4] 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                | 129 ++++++++++++++++++
 include/acpi/cppc_acpi.h                      |   9 ++
 3 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..111fab50b7a0 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -792,10 +792,139 @@ 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;
+}
+
+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 & CPPC_AUTO_ACT_WINDOW_MAX_SIG;
+	exp = (val >> CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) & CPPC_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 > CPPC_AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, CPPC_AUTO_ACT_WINDOW_MAX_EXP))
+		return -EINVAL;
+
+	while (usec > CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH) {
+		usec /= 10;
+		digits += 1;
+	}
+
+	if (usec > CPPC_AUTO_ACT_WINDOW_MAX_SIG)
+		usec = CPPC_AUTO_ACT_WINDOW_MAX_SIG;
+
+	ret = cppc_set_auto_act_window(policy->cpu,
+				       (digits << CPPC_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);
+}
+
+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 > CPPC_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,
 };
 
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 134931b081a0..8176fca4c86b 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -32,6 +32,15 @@
 #define	CMD_READ 0
 #define	CMD_WRITE 1
 
+#define CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE	(7)
+#define CPPC_AUTO_ACT_WINDOW_EXP_BIT_SIZE	(3)
+#define CPPC_AUTO_ACT_WINDOW_MAX_SIG	((1 << CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) - 1)
+#define CPPC_AUTO_ACT_WINDOW_MAX_EXP	((1 << CPPC_AUTO_ACT_WINDOW_EXP_BIT_SIZE) - 1)
+/* CPPC_AUTO_ACT_WINDOW_MAX_SIG is 127, so 128 and 129 will decay to 127 when writing */
+#define CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH 129
+
+#define CPPC_ENERGY_PERF_MAX	(0xFF)
+
 /* Each register has the folowing format. */
 struct cpc_reg {
 	u8 descriptor;
-- 
2.33.0

Re: [PATCH v3 1/4] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[Pierre Gondois]

Hello Lifeng,

On 12/16/24 10:16, Lifeng Zheng wrote:
> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
> cppc registers, with four 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.
> 
> 3. Extract the operations if register is in pcc out as
> cppc_get_reg_val_in_pcc().
> 
> 4. Return the result of cpc_read() instead of 0.
> 
> Add cppc_set_reg_val_in_pcc() and cppc_set_reg_val() as generic functions
> for setting cppc registers value. Unlike other set reg ABIs,
> cppc_set_reg_val() 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.
> 
> These functions can be used to reduce some existing code duplication.
> 
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>   drivers/acpi/cppc_acpi.c | 111 +++++++++++++++++++++++++++++----------
>   1 file changed, 84 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index c1f3568d0c50..bb5333a503a2 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1179,43 +1179,100 @@ 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_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val)
>   {
> -	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> +	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> +	struct cppc_pcc_data *pcc_ss_data = NULL;
> +	int ret;
> +
> +	if (pcc_ss_id < 0) {
> +		pr_debug("Invalid pcc_ss_id\n");
> +		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)
> +		ret = cpc_read(cpu, reg, val);
> +	else
> +		ret = -EIO;
> +
> +	up_write(&pcc_ss_data->pcc_lock);
> +
> +	return ret;
> +}
> +
> +static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val)
> +{
> +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>   	struct cpc_register_resource *reg;
>   
>   	if (!cpc_desc) {
> -		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> +		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>   		return -ENODEV;
>   	}
>   
>   	reg = &cpc_desc->cpc_regs[reg_idx];
>   
> -	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;
> -
> -		if (pcc_ss_id < 0)
> -			return -EIO;
> +	if (!CPC_SUPPORTED(reg)) {
> +		pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> +		return -EOPNOTSUPP;
> +	}

I think this is only valid for optional fields. Meaning that:
- if the function is used one day for the mandatory 'Lowest Performance'
field, an integer value of 0 would be valid.
- if the function is used for a mandatory field containing a NULL Buffer,
it seems we would return -EFAULT currently, through cpc_read(). -EOPNOTSUPP
doesn't seem appropriate as the field would be mandatory.

Maybe the function needs an additional 'bool optional' input parameter
to do these check conditionally.

>   
> -		pcc_ss_data = pcc_data[pcc_ss_id];
> +	if (CPC_IN_PCC(reg))
> +		return cppc_get_reg_val_in_pcc(cpu, reg, val);
>   
> -		down_write(&pcc_ss_data->pcc_lock);
> +	return cpc_read(cpu, reg, val);
> +}
>   
> -		if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
> -			cpc_read(cpunum, reg, perf);
> -		else
> -			ret = -EIO;
> +static int cppc_set_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 val)
> +{
> +	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> +	struct cppc_pcc_data *pcc_ss_data = NULL;
> +	int ret;
>   
> -		up_write(&pcc_ss_data->pcc_lock);
> +	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;
> +}
> +
> +static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
> +{
> +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> +	struct cpc_register_resource *reg;
> +
> +	if (!cpc_desc) {
> +		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> +		return -ENODEV;
>   	}
>   
> -	cpc_read(cpunum, reg, perf);
> +	reg = &cpc_desc->cpc_regs[reg_idx];
>   
> -	return 0;
> +	if (!CPC_SUPPORTED(reg)) {
> +		pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> +		return -EOPNOTSUPP;
> +	}

Similarly to cppc_get_reg_val(), if a field is:
- mandatory + integer: currently doesn't exist. Not sure we should
try to detect that, but might be safer.
- mandatory + buffer: should not return -EOPNOTSUPP I think
- optional + integer: e.g.: 'Autonomous Selection Enable Register',
we should return -EOPNOTSUPP. It seems that currently, if the integer
value is 1, I get a 'write error: Bad address'
- optional + buffer:
should effectively return -EOPNOTSUPP if the buffer is NULL.

> +
> +	if (CPC_IN_PCC(reg))
> +		return cppc_set_reg_val_in_pcc(cpu, reg, val);
> +
> +	return cpc_write(cpu, reg, val);
>   }
>   
>   /**
> @@ -1223,11 +1280,11 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>    * @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_val(cpunum, DESIRED_PERF, desired_perf);
>   }
>   EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>   
> @@ -1236,11 +1293,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_val(cpunum, NOMINAL_PERF, nominal_perf);
>   }
>   
>   /**
> @@ -1248,11 +1305,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_val(cpunum, HIGHEST_PERF, highest_perf);
>   }
>   EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>   
> @@ -1261,11 +1318,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_val(cpunum, ENERGY_PERF, epp_perf);
>   }
>   EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
>   

Re: [PATCH v3 3/4] ACPI: CPPC: Add autonomous selection ABIs

[Pierre Gondois]

Hello Lifeng, Huang, Gautham, Mario,

On 12/16/24 10:16, Lifeng Zheng wrote:
> 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 83c7fcad74ad..645f2366c888 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1595,6 +1595,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_val(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)

As there is only one way to interpret the value of the
'Autonomous Activity Window Register', maybe the logic to convert
from/to the register value to a value in us should be placed here
rather than in the cppc_cpufreq driver.
Meaning, maybe the prototype should be:

int cppc_get_auto_act_window(int cpu, unsigned int *auto_act_window);

Similar remark for cppc_set_epp() and other functions.

> +{
> +	return cppc_get_reg_val(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_val(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)

Similarly, maybe it would be better to use:
int cppc_get_auto_sel(int cpu, bool *auto_sel);

> +{
> +	return cppc_get_reg_val(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);

This is a bit annoying, but maybe only one function between:
- cppc_get_auto_sel_caps()
- cppc_get_auto_sel()
is necessary.

I added the owners of the amd-pstate driver to ask if this would
be ok to replace cppc_get_auto_sel_caps() by cppc_get_auto_sel().

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

Re: [PATCH v3 3/4] ACPI: CPPC: Add autonomous selection ABIs

[Mario Limonciello]

On 12/17/2024 07:48, Pierre Gondois wrote:
> Hello Lifeng, Huang, Gautham, Mario,
> 
> On 12/16/24 10:16, Lifeng Zheng wrote:
>> 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 83c7fcad74ad..645f2366c888 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1595,6 +1595,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_val(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)
> 
> As there is only one way to interpret the value of the
> 'Autonomous Activity Window Register', maybe the logic to convert
> from/to the register value to a value in us should be placed here
> rather than in the cppc_cpufreq driver.
> Meaning, maybe the prototype should be:
> 
> int cppc_get_auto_act_window(int cpu, unsigned int *auto_act_window);
> 
> Similar remark for cppc_set_epp() and other functions.
> 
>> +{
>> +    return cppc_get_reg_val(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_val(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)
> 
> Similarly, maybe it would be better to use:
> int cppc_get_auto_sel(int cpu, bool *auto_sel);
> 
>> +{
>> +    return cppc_get_reg_val(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);
> 
> This is a bit annoying, but maybe only one function between:
> - cppc_get_auto_sel_caps()
> - cppc_get_auto_sel()
> is necessary.
> 
> I added the owners of the amd-pstate driver to ask if this would
> be ok to replace cppc_get_auto_sel_caps() by cppc_get_auto_sel().

Yeah I have no concerns with this if that's the direction this patch 
series goes.  Feel free to change amd-pstate in the patch that 
introduces cppc_get_auto_sel().

I'll be out around the US holiday, so I might not be able to review it 
for a while, but CC Gautham on the series and he may be able to.

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

Re: [PATCH v3 1/4] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[zhenglifeng (A)]

On 2024/12/17 21:48, Pierre Gondois wrote:
> Hello Lifeng,
> 
> On 12/16/24 10:16, Lifeng Zheng wrote:
>> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
>> cppc registers, with four 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.
>>
>> 3. Extract the operations if register is in pcc out as
>> cppc_get_reg_val_in_pcc().
>>
>> 4. Return the result of cpc_read() instead of 0.
>>
>> Add cppc_set_reg_val_in_pcc() and cppc_set_reg_val() as generic functions
>> for setting cppc registers value. Unlike other set reg ABIs,
>> cppc_set_reg_val() 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.
>>
>> These functions can be used to reduce some existing code duplication.
>>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>   drivers/acpi/cppc_acpi.c | 111 +++++++++++++++++++++++++++++----------
>>   1 file changed, 84 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index c1f3568d0c50..bb5333a503a2 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1179,43 +1179,100 @@ 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_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val)
>>   {
>> -    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> +    int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> +    struct cppc_pcc_data *pcc_ss_data = NULL;
>> +    int ret;
>> +
>> +    if (pcc_ss_id < 0) {
>> +        pr_debug("Invalid pcc_ss_id\n");
>> +        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)
>> +        ret = cpc_read(cpu, reg, val);
>> +    else
>> +        ret = -EIO;
>> +
>> +    up_write(&pcc_ss_data->pcc_lock);
>> +
>> +    return ret;
>> +}
>> +
>> +static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val)
>> +{
>> +    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>       struct cpc_register_resource *reg;
>>         if (!cpc_desc) {
>> -        pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>> +        pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>>           return -ENODEV;
>>       }
>>         reg = &cpc_desc->cpc_regs[reg_idx];
>>   -    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;
>> -
>> -        if (pcc_ss_id < 0)
>> -            return -EIO;
>> +    if (!CPC_SUPPORTED(reg)) {
>> +        pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> +        return -EOPNOTSUPP;
>> +    }
> 
> I think this is only valid for optional fields. Meaning that:
> - if the function is used one day for the mandatory 'Lowest Performance'
> field, an integer value of 0 would be valid.
> - if the function is used for a mandatory field containing a NULL Buffer,
> it seems we would return -EFAULT currently, through cpc_read(). -EOPNOTSUPP
> doesn't seem appropriate as the field would be mandatory.
> 
> Maybe the function needs an additional 'bool optional' input parameter
> to do these check conditionally.

Indeed, I should have judged the type before doing this check. But adding a
input parameter is not a really nice way to me. How about adding a bool
list of length MAX_CPC_REG_ENT in cppc_acpi.h to indicate wheter it is
optional?

> 
>>   -        pcc_ss_data = pcc_data[pcc_ss_id];
>> +    if (CPC_IN_PCC(reg))
>> +        return cppc_get_reg_val_in_pcc(cpu, reg, val);
>>   -        down_write(&pcc_ss_data->pcc_lock);
>> +    return cpc_read(cpu, reg, val);
>> +}
>>   -        if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>> -            cpc_read(cpunum, reg, perf);
>> -        else
>> -            ret = -EIO;
>> +static int cppc_set_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 val)
>> +{
>> +    int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> +    struct cppc_pcc_data *pcc_ss_data = NULL;
>> +    int ret;
>>   -        up_write(&pcc_ss_data->pcc_lock);
>> +    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;
>> +}
>> +
>> +static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
>> +{
>> +    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> +    struct cpc_register_resource *reg;
>> +
>> +    if (!cpc_desc) {
>> +        pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> +        return -ENODEV;
>>       }
>>   -    cpc_read(cpunum, reg, perf);
>> +    reg = &cpc_desc->cpc_regs[reg_idx];
>>   -    return 0;
>> +    if (!CPC_SUPPORTED(reg)) {
>> +        pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> +        return -EOPNOTSUPP;
>> +    }
> 
> Similarly to cppc_get_reg_val(), if a field is:
> - mandatory + integer: currently doesn't exist. Not sure we should
> try to detect that, but might be safer.
> - mandatory + buffer: should not return -EOPNOTSUPP I think
> - optional + integer: e.g.: 'Autonomous Selection Enable Register',
> we should return -EOPNOTSUPP. It seems that currently, if the integer
> value is 1, I get a 'write error: Bad address'
> - optional + buffer:
> should effectively return -EOPNOTSUPP if the buffer is NULL.

Actually, cpc_write() doesn't check field type and treats the field as a
buffer. That's why you get 'Bad address' error when the integer value is 1.
I think the existing code needs to be improved, otherwise there may be
unexpected problems.

Do you mean we should return -EOPNOTSUPP no matter what to be written if
this field is a optional + integer one? And what about a mandatory +
integer one. Should we directly write the int_value?

Looking forward to your opinion.

> 
>> +
>> +    if (CPC_IN_PCC(reg))
>> +        return cppc_set_reg_val_in_pcc(cpu, reg, val);
>> +
>> +    return cpc_write(cpu, reg, val);
>>   }
>>     /**
>> @@ -1223,11 +1280,11 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>    * @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_val(cpunum, DESIRED_PERF, desired_perf);
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>>   @@ -1236,11 +1293,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_val(cpunum, NOMINAL_PERF, nominal_perf);
>>   }
>>     /**
>> @@ -1248,11 +1305,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_val(cpunum, HIGHEST_PERF, highest_perf);
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>   @@ -1261,11 +1318,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_val(cpunum, ENERGY_PERF, epp_perf);
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
>>   
> 

Re: [PATCH v3 3/4] ACPI: CPPC: Add autonomous selection ABIs

[zhenglifeng (A)]

Hello Pierre,

On 2024/12/17 21:48, Pierre Gondois wrote:
> Hello Lifeng, Huang, Gautham, Mario,
> 
> On 12/16/24 10:16, Lifeng Zheng wrote:
>> 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 83c7fcad74ad..645f2366c888 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1595,6 +1595,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_val(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)
> 
> As there is only one way to interpret the value of the
> 'Autonomous Activity Window Register', maybe the logic to convert
> from/to the register value to a value in us should be placed here
> rather than in the cppc_cpufreq driver.
> Meaning, maybe the prototype should be:
> 
> int cppc_get_auto_act_window(int cpu, unsigned int *auto_act_window);
> 
> Similar remark for cppc_set_epp() and other functions.

Good point. Will improve it. Thanks.

> 
>> +{
>> +    return cppc_get_reg_val(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_val(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)
> 
> Similarly, maybe it would be better to use:
> int cppc_get_auto_sel(int cpu, bool *auto_sel);

Good point the same. Thanks.

> 
>> +{
>> +    return cppc_get_reg_val(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);
> 
> This is a bit annoying, but maybe only one function between:
> - cppc_get_auto_sel_caps()
> - cppc_get_auto_sel()
> is necessary.
> 
> I added the owners of the amd-pstate driver to ask if this would
> be ok to replace cppc_get_auto_sel_caps() by cppc_get_auto_sel().

Really nice. Thanks.

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

Re: [PATCH v3 3/4] ACPI: CPPC: Add autonomous selection ABIs

[zhenglifeng (A)]

Hello Pierre, Mario, Gautham, Huang

On 2024/12/18 4:38, Mario Limonciello wrote:
> On 12/17/2024 07:48, Pierre Gondois wrote:
>> Hello Lifeng, Huang, Gautham, Mario,
>>
>> On 12/16/24 10:16, Lifeng Zheng wrote:
>>> 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 83c7fcad74ad..645f2366c888 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -1595,6 +1595,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_val(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)
>>
>> As there is only one way to interpret the value of the
>> 'Autonomous Activity Window Register', maybe the logic to convert
>> from/to the register value to a value in us should be placed here
>> rather than in the cppc_cpufreq driver.
>> Meaning, maybe the prototype should be:
>>
>> int cppc_get_auto_act_window(int cpu, unsigned int *auto_act_window);
>>
>> Similar remark for cppc_set_epp() and other functions.
>>
>>> +{
>>> +    return cppc_get_reg_val(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_val(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)
>>
>> Similarly, maybe it would be better to use:
>> int cppc_get_auto_sel(int cpu, bool *auto_sel);
>>
>>> +{
>>> +    return cppc_get_reg_val(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);
>>
>> This is a bit annoying, but maybe only one function between:
>> - cppc_get_auto_sel_caps()
>> - cppc_get_auto_sel()
>> is necessary.
>>
>> I added the owners of the amd-pstate driver to ask if this would
>> be ok to replace cppc_get_auto_sel_caps() by cppc_get_auto_sel().
> 
> Yeah I have no concerns with this if that's the direction this patch series goes.  Feel free to change amd-pstate in the patch that introduces cppc_get_auto_sel().
> 
> I'll be out around the US holiday, so I might not be able to review it for a while, but CC Gautham on the series and he may be able to.

After checking, it turns out that the only place uses
cppc_get_auto_sel_caps() only check the ret but never uses the value of
perf_caps. I believe cppc_get_auto_sel() will meet the requirements.

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

Re: [PATCH v3 1/4] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[Pierre Gondois]

Hello Lifeng,

On 12/20/24 09:30, zhenglifeng (A) wrote:
> On 2024/12/17 21:48, Pierre Gondois wrote:
>> Hello Lifeng,
>>
>> On 12/16/24 10:16, Lifeng Zheng wrote:
>>> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
>>> cppc registers, with four 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.
>>>
>>> 3. Extract the operations if register is in pcc out as
>>> cppc_get_reg_val_in_pcc().
>>>
>>> 4. Return the result of cpc_read() instead of 0.
>>>
>>> Add cppc_set_reg_val_in_pcc() and cppc_set_reg_val() as generic functions
>>> for setting cppc registers value. Unlike other set reg ABIs,
>>> cppc_set_reg_val() 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.
>>>
>>> These functions can be used to reduce some existing code duplication.
>>>
>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>>> ---
>>>    drivers/acpi/cppc_acpi.c | 111 +++++++++++++++++++++++++++++----------
>>>    1 file changed, 84 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>> index c1f3568d0c50..bb5333a503a2 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -1179,43 +1179,100 @@ 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_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val)
>>>    {
>>> -    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>>> +    int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>> +    struct cppc_pcc_data *pcc_ss_data = NULL;
>>> +    int ret;
>>> +
>>> +    if (pcc_ss_id < 0) {
>>> +        pr_debug("Invalid pcc_ss_id\n");
>>> +        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)
>>> +        ret = cpc_read(cpu, reg, val);
>>> +    else
>>> +        ret = -EIO;
>>> +
>>> +    up_write(&pcc_ss_data->pcc_lock);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val)
>>> +{
>>> +    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>>        struct cpc_register_resource *reg;
>>>          if (!cpc_desc) {
>>> -        pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>>> +        pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>>>            return -ENODEV;
>>>        }
>>>          reg = &cpc_desc->cpc_regs[reg_idx];
>>>    -    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;
>>> -
>>> -        if (pcc_ss_id < 0)
>>> -            return -EIO;
>>> +    if (!CPC_SUPPORTED(reg)) {
>>> +        pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>>> +        return -EOPNOTSUPP;
>>> +    }
>>
>> I think this is only valid for optional fields. Meaning that:
>> - if the function is used one day for the mandatory 'Lowest Performance'
>> field, an integer value of 0 would be valid.
>> - if the function is used for a mandatory field containing a NULL Buffer,
>> it seems we would return -EFAULT currently, through cpc_read(). -EOPNOTSUPP
>> doesn't seem appropriate as the field would be mandatory.
>>
>> Maybe the function needs an additional 'bool optional' input parameter
>> to do these check conditionally.
> 
> Indeed, I should have judged the type before doing this check. But adding a
> input parameter is not a really nice way to me. How about adding a bool
> list of length MAX_CPC_REG_ENT in cppc_acpi.h to indicate wheter it is
> optional?

Actually all these functions:
- cppc_get_desired_perf
- cppc_get_highest_perf
- cppc_get_epp_perf
- cppc_set_epp
- cppc_get_auto_act_window
- cppc_set_auto_act_window
- cppc_get_auto_sel
- cppc_get_nominal_perf

and in general all the functions getting / setting one value at a time could
be implemented by macros similars to show_cppc_data(). From what I see the
input parameters required are:
- name of the field
- if the field is mandatory to have or not
- if the field is writeable
- if the field is implemented as an integer, register, or can be both

This would avoid having numerous function definitions doing approximately the
same thing.

> 
>>
>>>    -        pcc_ss_data = pcc_data[pcc_ss_id];
>>> +    if (CPC_IN_PCC(reg))
>>> +        return cppc_get_reg_val_in_pcc(cpu, reg, val);
>>>    -        down_write(&pcc_ss_data->pcc_lock);
>>> +    return cpc_read(cpu, reg, val);
>>> +}
>>>    -        if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>>> -            cpc_read(cpunum, reg, perf);
>>> -        else
>>> -            ret = -EIO;
>>> +static int cppc_set_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 val)
>>> +{
>>> +    int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>> +    struct cppc_pcc_data *pcc_ss_data = NULL;
>>> +    int ret;
>>>    -        up_write(&pcc_ss_data->pcc_lock);
>>> +    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;
>>> +}
>>> +
>>> +static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
>>> +{
>>> +    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>> +    struct cpc_register_resource *reg;
>>> +
>>> +    if (!cpc_desc) {
>>> +        pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>>> +        return -ENODEV;
>>>        }
>>>    -    cpc_read(cpunum, reg, perf);
>>> +    reg = &cpc_desc->cpc_regs[reg_idx];
>>>    -    return 0;
>>> +    if (!CPC_SUPPORTED(reg)) {
>>> +        pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>>> +        return -EOPNOTSUPP;
>>> +    }
>>
>> Similarly to cppc_get_reg_val(), if a field is:
>> - mandatory + integer: currently doesn't exist. Not sure we should
>> try to detect that, but might be safer.
>> - mandatory + buffer: should not return -EOPNOTSUPP I think
>> - optional + integer: e.g.: 'Autonomous Selection Enable Register',
>> we should return -EOPNOTSUPP. It seems that currently, if the integer
>> value is 1, I get a 'write error: Bad address'
>> - optional + buffer:
>> should effectively return -EOPNOTSUPP if the buffer is NULL.
> 
> Actually, cpc_write() doesn't check field type and treats the field as a
> buffer. That's why you get 'Bad address' error when the integer value is 1.
> I think the existing code needs to be improved, otherwise there may be
> unexpected problems.
> 
> Do you mean we should return -EOPNOTSUPP no matter what to be written if
> this field is a optional + integer one?

Yes exact

  And what about a mandatory +
> integer one. Should we directly write the int_value?

I don't think it is possible to have this. Indeed, if a value is writeable,
it must be a register, so mandatory + integer should not exist. I suggested
a check in case someone made a mistake, but it is not sure the check is actually
necessary.

Regards,
Pierre

Re: [PATCH v3 1/4] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[zhenglifeng (A)]

Hello Pierre,

On 2025/1/8 0:54, Pierre Gondois wrote:
> Hello Lifeng,
> 
> On 12/20/24 09:30, zhenglifeng (A) wrote:
>> On 2024/12/17 21:48, Pierre Gondois wrote:
>>> Hello Lifeng,
>>>
>>> On 12/16/24 10:16, Lifeng Zheng wrote:
>>>> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
>>>> cppc registers, with four 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.
>>>>
>>>> 3. Extract the operations if register is in pcc out as
>>>> cppc_get_reg_val_in_pcc().
>>>>
>>>> 4. Return the result of cpc_read() instead of 0.
>>>>
>>>> Add cppc_set_reg_val_in_pcc() and cppc_set_reg_val() as generic functions
>>>> for setting cppc registers value. Unlike other set reg ABIs,
>>>> cppc_set_reg_val() 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.
>>>>
>>>> These functions can be used to reduce some existing code duplication.
>>>>
>>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>>>> ---
>>>>    drivers/acpi/cppc_acpi.c | 111 +++++++++++++++++++++++++++++----------
>>>>    1 file changed, 84 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>>> index c1f3568d0c50..bb5333a503a2 100644
>>>> --- a/drivers/acpi/cppc_acpi.c
>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>> @@ -1179,43 +1179,100 @@ 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_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val)
>>>>    {
>>>> -    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>>>> +    int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>>> +    struct cppc_pcc_data *pcc_ss_data = NULL;
>>>> +    int ret;
>>>> +
>>>> +    if (pcc_ss_id < 0) {
>>>> +        pr_debug("Invalid pcc_ss_id\n");
>>>> +        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)
>>>> +        ret = cpc_read(cpu, reg, val);
>>>> +    else
>>>> +        ret = -EIO;
>>>> +
>>>> +    up_write(&pcc_ss_data->pcc_lock);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val)
>>>> +{
>>>> +    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>>>        struct cpc_register_resource *reg;
>>>>          if (!cpc_desc) {
>>>> -        pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>>>> +        pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>>>>            return -ENODEV;
>>>>        }
>>>>          reg = &cpc_desc->cpc_regs[reg_idx];
>>>>    -    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;
>>>> -
>>>> -        if (pcc_ss_id < 0)
>>>> -            return -EIO;
>>>> +    if (!CPC_SUPPORTED(reg)) {
>>>> +        pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>>>> +        return -EOPNOTSUPP;
>>>> +    }
>>>
>>> I think this is only valid for optional fields. Meaning that:
>>> - if the function is used one day for the mandatory 'Lowest Performance'
>>> field, an integer value of 0 would be valid.
>>> - if the function is used for a mandatory field containing a NULL Buffer,
>>> it seems we would return -EFAULT currently, through cpc_read(). -EOPNOTSUPP
>>> doesn't seem appropriate as the field would be mandatory.
>>>
>>> Maybe the function needs an additional 'bool optional' input parameter
>>> to do these check conditionally.
>>
>> Indeed, I should have judged the type before doing this check. But adding a
>> input parameter is not a really nice way to me. How about adding a bool
>> list of length MAX_CPC_REG_ENT in cppc_acpi.h to indicate wheter it is
>> optional?
> 
> Actually all these functions:
> - cppc_get_desired_perf
> - cppc_get_highest_perf
> - cppc_get_epp_perf
> - cppc_set_epp
> - cppc_get_auto_act_window
> - cppc_set_auto_act_window

As you suggest in another patch, the logic should be placed in
cppc_get_auto_act_window() and some other functions. I'm afraid these
functions couldn't be implemented with the macros you suggest.

> - cppc_get_auto_sel
> - cppc_get_nominal_perf
> 
> and in general all the functions getting / setting one value at a time could
> be implemented by macros similars to show_cppc_data(). From what I see the
> input parameters required are:
> - name of the field
> - if the field is mandatory to have or not

If with this parameter, we should put all the cppc_get_reg_val() and
cppc_set_reg_val() in the macro. This wouldn't look really nice. I
prefer to use a macro to judge mandatory / optional. I'll show you in
v4.

> - if the field is writeable

I think we can define a READ macro, a WRITE macro and a RW macro. For
the registers which are not writeable, only use the READ macro to
implement getting function.

> - if the field is implemented as an integer, register, or can be both

I don't think this parameter is necessary. The field type can be got
from cpc_desc->cpc_regs[reg_idx].type.

> 
> This would avoid having numerous function definitions doing approximately the
> same thing.

So from what I see the input parameters required are name of the field
and reg_idx. Thanks for your advice!

> 
>>
>>>
>>>>    -        pcc_ss_data = pcc_data[pcc_ss_id];
>>>> +    if (CPC_IN_PCC(reg))
>>>> +        return cppc_get_reg_val_in_pcc(cpu, reg, val);
>>>>    -        down_write(&pcc_ss_data->pcc_lock);
>>>> +    return cpc_read(cpu, reg, val);
>>>> +}
>>>>    -        if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>>>> -            cpc_read(cpunum, reg, perf);
>>>> -        else
>>>> -            ret = -EIO;
>>>> +static int cppc_set_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 val)
>>>> +{
>>>> +    int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>>> +    struct cppc_pcc_data *pcc_ss_data = NULL;
>>>> +    int ret;
>>>>    -        up_write(&pcc_ss_data->pcc_lock);
>>>> +    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;
>>>> +}
>>>> +
>>>> +static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
>>>> +{
>>>> +    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>>> +    struct cpc_register_resource *reg;
>>>> +
>>>> +    if (!cpc_desc) {
>>>> +        pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>>>> +        return -ENODEV;
>>>>        }
>>>>    -    cpc_read(cpunum, reg, perf);
>>>> +    reg = &cpc_desc->cpc_regs[reg_idx];
>>>>    -    return 0;
>>>> +    if (!CPC_SUPPORTED(reg)) {
>>>> +        pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>>>> +        return -EOPNOTSUPP;
>>>> +    }
>>>
>>> Similarly to cppc_get_reg_val(), if a field is:
>>> - mandatory + integer: currently doesn't exist. Not sure we should
>>> try to detect that, but might be safer.
>>> - mandatory + buffer: should not return -EOPNOTSUPP I think
>>> - optional + integer: e.g.: 'Autonomous Selection Enable Register',
>>> we should return -EOPNOTSUPP. It seems that currently, if the integer
>>> value is 1, I get a 'write error: Bad address'
>>> - optional + buffer:
>>> should effectively return -EOPNOTSUPP if the buffer is NULL.
>>
>> Actually, cpc_write() doesn't check field type and treats the field as a
>> buffer. That's why you get 'Bad address' error when the integer value is 1.
>> I think the existing code needs to be improved, otherwise there may be
>> unexpected problems.
>>
>> Do you mean we should return -EOPNOTSUPP no matter what to be written if
>> this field is a optional + integer one?
> 
> Yes exact
> 
>> And what about a mandatory +
>> integer one. Should we directly write the int_value?
> 
> I don't think it is possible to have this. Indeed, if a value is writeable,
> it must be a register, so mandatory + integer should not exist. I suggested
> a check in case someone made a mistake, but it is not sure the check is actually
> necessary.

Yeah, I think it's better to have this check, too.

Regards,
Lifeng

> 
> Regards,
> Pierre
> 

Re: [PATCH v3 1/4] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[Pierre Gondois]

Hello Lifeng,

On 1/10/25 03:23, zhenglifeng (A) wrote:
> Hello Pierre,
> 
> On 2025/1/8 0:54, Pierre Gondois wrote:
>> Hello Lifeng,
>>
>> On 12/20/24 09:30, zhenglifeng (A) wrote:
>>> On 2024/12/17 21:48, Pierre Gondois wrote:
>>>> Hello Lifeng,
>>>>
>>>> On 12/16/24 10:16, Lifeng Zheng wrote:
>>>>> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
>>>>> cppc registers, with four 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.
>>>>>
>>>>> 3. Extract the operations if register is in pcc out as
>>>>> cppc_get_reg_val_in_pcc().
>>>>>
>>>>> 4. Return the result of cpc_read() instead of 0.
>>>>>
>>>>> Add cppc_set_reg_val_in_pcc() and cppc_set_reg_val() as generic functions
>>>>> for setting cppc registers value. Unlike other set reg ABIs,
>>>>> cppc_set_reg_val() 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.
>>>>>
>>>>> These functions can be used to reduce some existing code duplication.
>>>>>
>>>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>>>>> ---
>>>>>     drivers/acpi/cppc_acpi.c | 111 +++++++++++++++++++++++++++++----------
>>>>>     1 file changed, 84 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>>>> index c1f3568d0c50..bb5333a503a2 100644
>>>>> --- a/drivers/acpi/cppc_acpi.c
>>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>>> @@ -1179,43 +1179,100 @@ 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_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val)
>>>>>     {
>>>>> -    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>>>>> +    int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>>>> +    struct cppc_pcc_data *pcc_ss_data = NULL;
>>>>> +    int ret;
>>>>> +
>>>>> +    if (pcc_ss_id < 0) {
>>>>> +        pr_debug("Invalid pcc_ss_id\n");
>>>>> +        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)
>>>>> +        ret = cpc_read(cpu, reg, val);
>>>>> +    else
>>>>> +        ret = -EIO;
>>>>> +
>>>>> +    up_write(&pcc_ss_data->pcc_lock);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val)
>>>>> +{
>>>>> +    struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>>>>         struct cpc_register_resource *reg;
>>>>>           if (!cpc_desc) {
>>>>> -        pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>>>>> +        pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>>>>>             return -ENODEV;
>>>>>         }
>>>>>           reg = &cpc_desc->cpc_regs[reg_idx];
>>>>>     -    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;
>>>>> -
>>>>> -        if (pcc_ss_id < 0)
>>>>> -            return -EIO;
>>>>> +    if (!CPC_SUPPORTED(reg)) {
>>>>> +        pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>>>>> +        return -EOPNOTSUPP;
>>>>> +    }
>>>>
>>>> I think this is only valid for optional fields. Meaning that:
>>>> - if the function is used one day for the mandatory 'Lowest Performance'
>>>> field, an integer value of 0 would be valid.
>>>> - if the function is used for a mandatory field containing a NULL Buffer,
>>>> it seems we would return -EFAULT currently, through cpc_read(). -EOPNOTSUPP
>>>> doesn't seem appropriate as the field would be mandatory.
>>>>
>>>> Maybe the function needs an additional 'bool optional' input parameter
>>>> to do these check conditionally.
>>>
>>> Indeed, I should have judged the type before doing this check. But adding a
>>> input parameter is not a really nice way to me. How about adding a bool
>>> list of length MAX_CPC_REG_ENT in cppc_acpi.h to indicate wheter it is
>>> optional?
>>
>> Actually all these functions:
>> - cppc_get_desired_perf
>> - cppc_get_highest_perf
>> - cppc_get_epp_perf
>> - cppc_set_epp
>> - cppc_get_auto_act_window
>> - cppc_set_auto_act_window
> 
> As you suggest in another patch, the logic should be placed in
> cppc_get_auto_act_window() and some other functions. I'm afraid these
> functions couldn't be implemented with the macros you suggest.

If you're referring to the [get|set]_auto_act_window() functions, I guess
it should be ok to have the getter/setter functions implemented as a macros,
and then have a wrapper to do the conversion of the value.

> 
>> - cppc_get_auto_sel
>> - cppc_get_nominal_perf
>>
>> and in general all the functions getting / setting one value at a time could
>> be implemented by macros similars to show_cppc_data(). From what I see the
>> input parameters required are:
>> - name of the field
>> - if the field is mandatory to have or not
> 
> If with this parameter, we should put all the cppc_get_reg_val() and
> cppc_set_reg_val() in the macro. This wouldn't look really nice. I
> prefer to use a macro to judge mandatory / optional. I'll show you in
> v4.
> 

If you prefer to have specific macro names to distinguish optional/mandatory
fields, it also seems a good solution.

>> - if the field is writeable
> 
> I think we can define a READ macro, a WRITE macro and a RW macro. For
> the registers which are not writeable, only use the READ macro to
> implement getting function.

Yes right, same comment as above.

> 
>> - if the field is implemented as an integer, register, or can be both
> 
> I don't think this parameter is necessary. The field type can be got
> from cpc_desc->cpc_regs[reg_idx].type.

Yes indeed.

> 
>>
>> This would avoid having numerous function definitions doing approximately the
>> same thing.
> 
> So from what I see the input parameters required are name of the field
> and reg_idx. Thanks for your advice!
>