[PATCH v5 0/8] Support for autonomous selection in cppc_cpufreq

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

 - patches 6-8 expose sysfs files for users to control CPPC autonomous
   selection when supported

Changelog:

v5:

 - add more explanation to the commit logs and comments
 - change REG_OPTIONAL from bin to hex
 - split patch 2 into 3 smaller patches
 - remove CPPC_REG_VAL_READ() and CPPC_REG_VAL_WRITE() macros
 - move the modification part in patch 5 into a separate patch
 - rename the sysfs file from "energy_perf" to
   energy_performance_preference_val

v4:

 - add REG_OPTIONAL and IS_OPTIONAL_CPC_REG to judge if a cpc register is
   an optional one
 - check whether the register is optional before CPC_SUPPORTED check in
   cppc_get_reg_val() and cppc_set_reg_val()
 - check the register's type in cppc_set_reg_val()
 - add macros to generally implement registers getting and setting
   functions
 - move some logic codes from cppc_cpufreq.c to cppc_acpi.c
 - replace cppc_get_auto_sel_caps() by cppc_get_auto_sel()

v3:

 - 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

v2:

 - fix some incorrect placeholder
 - change kstrtoul to kstrtobool in store_auto_select

Lifeng Zheng (8):
  ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is
    optional
  ACPI: CPPC: Optimize cppc_get_perf()
  ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()
  ACPI: CPPC: Add cppc_set_reg_val()
  ACPI: CPPC: Refactor register value get and set ABIs
  ACPI: CPPC: Modify cppc_get_auto_sel_caps() to cppc_get_auto_sel()
  ACPI: CPPC: Add three functions related to autonomous selection
  cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

 .../ABI/testing/sysfs-devices-system-cpu      |  54 ++++
 drivers/acpi/cppc_acpi.c                      | 303 +++++++++++-------
 drivers/cpufreq/amd-pstate.c                  |   3 +-
 drivers/cpufreq/cppc_cpufreq.c                | 109 +++++++
 include/acpi/cppc_acpi.h                      |  30 +-
 5 files changed, 372 insertions(+), 127 deletions(-)

-- 
2.33.0

[PATCH v5 1/8] ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is optional

[Lifeng Zheng]

In ACPI 6.5, s8.4.6.1 _CPC (Continuous Performance Control), whether each
of the per-cpu cpc_regs[] is mendatory or optional is defined. Since the
CPC_SUPPORTED() check is only for optional cpc field, another macro to
check if the field is optional is needed.

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index f193e713825a..39f019e265da 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -129,6 +129,20 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
 #define CPC_SUPPORTED(cpc) ((cpc)->type == ACPI_TYPE_INTEGER ?		\
 				!!(cpc)->cpc_entry.int_value :		\
 				!IS_NULL_REG(&(cpc)->cpc_entry.reg))
+
+/*
+ * Each bit indicates the optionality of the register in per-cpu
+ * cpc_regs[] with the corresponding index. 0 means mandatory and 1
+ * means optional.
+ */
+#define REG_OPTIONAL (0x1FC7D0)
+
+/*
+ * Use the index of the register in per-cpu cpc_regs[] to check if
+ * it's an optional one.
+ */
+#define IS_OPTIONAL_CPC_REG(reg_idx) (REG_OPTIONAL & (1U << (reg_idx)))
+
 /*
  * Arbitrary Retries in case the remote processor is slow to respond
  * to PCC commands. Keeping it high enough to cover emulators where
-- 
2.33.0

[PATCH v5 2/8] ACPI: CPPC: Optimize cppc_get_perf()

[Lifeng Zheng]

Optimize cppc_get_perf() with three changes:

1. Change the error kind to "no such device" when pcc_ss_id < 0, as other
register value getting functions.

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

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

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 39f019e265da..db22f8f107db 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1201,20 +1201,27 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
 
 	reg = &cpc_desc->cpc_regs[reg_idx];
 
+	if (IS_OPTIONAL_CPC_REG(reg_idx) && !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;
+		int ret;
 
-		if (pcc_ss_id < 0)
-			return -EIO;
+		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)
-			cpc_read(cpunum, reg, perf);
+			ret = cpc_read(cpunum, reg, perf);
 		else
 			ret = -EIO;
 
@@ -1223,9 +1230,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
 		return ret;
 	}
 
-	cpc_read(cpunum, reg, perf);
-
-	return 0;
+	return cpc_read(cpunum, reg, perf);
 }
 
 /**
-- 
2.33.0

[PATCH v5 3/8] ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()

[Lifeng Zheng]

Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
cppc registers. And extract the operations if register is in pcc out as
cppc_get_reg_val_in_pcc(). Without functional change.

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index db22f8f107db..3c9c4ce2a0b0 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1189,48 +1189,52 @@ 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);
-	struct cpc_register_resource *reg;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret;
 
-	if (!cpc_desc) {
-		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
+	if (pcc_ss_id < 0) {
+		pr_debug("Invalid pcc_ss_id\n");
 		return -ENODEV;
 	}
 
-	reg = &cpc_desc->cpc_regs[reg_idx];
+	pcc_ss_data = pcc_data[pcc_ss_id];
 
-	if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
-		pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
-		return -EOPNOTSUPP;
-	}
+	down_write(&pcc_ss_data->pcc_lock);
 
-	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;
+	if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
+		ret = cpc_read(cpu, reg, val);
+	else
+		ret = -EIO;
 
-		if (pcc_ss_id < 0) {
-			pr_debug("Invalid pcc_ss_id\n");
-			return -ENODEV;
-		}
+	up_write(&pcc_ss_data->pcc_lock);
 
-		pcc_ss_data = pcc_data[pcc_ss_id];
+	return ret;
+}
 
-		down_write(&pcc_ss_data->pcc_lock);
+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 (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
-			ret = cpc_read(cpunum, reg, perf);
-		else
-			ret = -EIO;
+	if (!cpc_desc) {
+		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
+		return -ENODEV;
+	}
 
-		up_write(&pcc_ss_data->pcc_lock);
+	reg = &cpc_desc->cpc_regs[reg_idx];
 
-		return ret;
+	if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
+		pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
+		return -EOPNOTSUPP;
 	}
 
-	return cpc_read(cpunum, reg, perf);
+	if (CPC_IN_PCC(reg))
+		return cppc_get_reg_val_in_pcc(cpu, reg, val);
+
+	return cpc_read(cpu, reg, val);
 }
 
 /**
@@ -1242,7 +1246,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
  */
 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);
 
@@ -1255,7 +1259,7 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
  */
 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);
 }
 
 /**
@@ -1267,7 +1271,7 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
  */
 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);
 
@@ -1280,7 +1284,7 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
  */
 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 v5 4/8] ACPI: CPPC: Add cppc_set_reg_val()

[Lifeng Zheng]

Add cppc_set_reg_val() as a generic function for setting cppc registers
value, with this features:

1. Check register type. If a register is writeable, it must be a buffer.

2. Check if the register is a optional and null one right after getting the
register.  Because if so, the rest of the operations are meaningless.

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

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

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 3c9c4ce2a0b0..17558d2b5ae5 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1237,6 +1237,56 @@ static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val)
 	return cpc_read(cpu, reg, val);
 }
 
+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;
+
+	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;
+	}
+
+	reg = &cpc_desc->cpc_regs[reg_idx];
+
+	/* if a register is writeable, it must be a buffer */
+	if ((reg->type != ACPI_TYPE_BUFFER) ||
+	    (IS_OPTIONAL_CPC_REG(reg_idx) && IS_NULL_REG(&reg->cpc_entry.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);
+}
+
 /**
  * cppc_get_desired_perf - Get the desired performance register value.
  * @cpunum: CPU from which to get desired performance.
-- 
2.33.0

[PATCH v5 5/8] ACPI: CPPC: Refactor register value get and set ABIs

[Lifeng Zheng]

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

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 17558d2b5ae5..c9fa5fdde7eb 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1614,44 +1614,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);
@@ -1663,43 +1633,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);
 
@@ -1713,38 +1647,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 v5 6/8] ACPI: CPPC: Modify cppc_get_auto_sel_caps() to cppc_get_auto_sel()

[Lifeng Zheng]

Modify cppc_get_auto_sel_caps() to cppc_get_auto_sel(). Using a
cppc_perf_caps to carry the value is unnecessary.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/acpi/cppc_acpi.c     | 15 ++++++++-------
 drivers/cpufreq/amd-pstate.c |  3 ++-
 include/acpi/cppc_acpi.h     |  6 +++---
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index c9fa5fdde7eb..e4c663000e40 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1608,23 +1608,24 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
 EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
 
 /**
- * cppc_get_auto_sel_caps - Read autonomous selection register.
- * @cpunum : CPU from which to read register.
- * @perf_caps : struct where autonomous selection register value is updated.
+ * cppc_get_auto_sel() - Read autonomous selection register.
+ * @cpu: CPU from which to read register.
+ * @enable: Return address.
  */
-int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+int cppc_get_auto_sel(int cpu, bool *enable)
 {
 	u64 auto_sel;
 	int ret;
 
-	ret = cppc_get_reg_val(cpunum, AUTO_SEL_ENABLE, &auto_sel);
+	ret = cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, &auto_sel);
 	if (ret)
 		return ret;
 
-	perf_caps->auto_sel = (bool)auto_sel;
+	*enable = (bool)auto_sel;
+
 	return 0;
 }
-EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
+EXPORT_SYMBOL_GPL(cppc_get_auto_sel);
 
 /**
  * cppc_set_auto_sel - Write autonomous selection register.
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index dd9b8d6993d6..d289edc851c0 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -436,6 +436,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
 {
 	struct cppc_perf_caps cppc_perf;
 	u64 numerator;
+	bool auto_sel;
 
 	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
 	if (ret)
@@ -457,7 +458,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
 	if (cppc_state == AMD_PSTATE_ACTIVE)
 		return 0;
 
-	ret = cppc_get_auto_sel_caps(cpudata->cpu, &cppc_perf);
+	ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel);
 	if (ret) {
 		pr_warn("failed to get auto_sel, ret: %d\n", ret);
 		return 0;
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 62d368bcd9ec..31767c65be20 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -159,7 +159,7 @@ 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_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
+extern int cppc_get_auto_sel(int cpu, bool *enable);
 extern int cppc_set_auto_sel(int cpu, bool enable);
 extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
 extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
@@ -229,11 +229,11 @@ static inline int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
 {
 	return -EOPNOTSUPP;
 }
-static inline int cppc_set_auto_sel(int cpu, bool enable)
+static inline int cppc_get_auto_sel(int cpu, bool *enable)
 {
 	return -EOPNOTSUPP;
 }
-static inline int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+static inline int cppc_set_auto_sel(int cpu, bool enable)
 {
 	return -EOPNOTSUPP;
 }
-- 
2.33.0

[PATCH v5 7/8] ACPI: CPPC: Add three functions related to autonomous selection

[Lifeng Zheng]

cppc_set_epp - write energy performance preference register value, based on
ACPI 6.5, s8.4.6.1.7

cppc_get_auto_act_window - read autonomous activity window register value,
based on ACPI 6.5, s8.4.6.1.6

cppc_set_auto_act_window - write autonomous activity window register value,
based on ACPI 6.5, s8.4.6.1.6

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index e4c663000e40..a075eaa83d6c 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1607,6 +1607,86 @@ 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)
+{
+	if (epp_val > CPPC_ENERGY_PERF_MAX)
+		return -EINVAL;
+
+	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.
+ *
+ * According to ACPI 6.5, s8.4.6.1.6, the value read from the autonomous
+ * activity window register consists of two parts: a 7 bits value indicate
+ * significand and a 3 bits value indicate exponent.
+ */
+int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
+{
+	unsigned int exp;
+	u64 val, sig;
+	int ret;
+
+	ret = cppc_get_reg_val(cpu, AUTO_ACT_WINDOW, &val);
+	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;
+	*auto_act_window = sig * int_pow(10, exp);
+
+	return 0;
+}
+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: usec value to write to the autonomous activity window register.
+ *
+ * According to ACPI 6.5, s8.4.6.1.6, the value to write to the autonomous
+ * activity window register consists of two parts: a 7 bits value indicate
+ * significand and a 3 bits value indicate exponent.
+ */
+int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
+{
+	/* The max value to stroe is 1270000000 */
+	u64 max_val = CPPC_AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, CPPC_AUTO_ACT_WINDOW_MAX_EXP);
+	int exp = 0;
+	u64 val;
+
+	if (auto_act_window > max_val)
+		return -EINVAL;
+
+	/*
+	 * The max significand is 127, when auto_act_window is larger than
+	 * 129, discard the precision of the last digit and increase the
+	 * exponent by 1.
+	 */
+	while (auto_act_window > CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH) {
+		auto_act_window /= 10;
+		exp += 1;
+	}
+
+	/* For 128 and 129, cut it to 127. */
+	if (auto_act_window > CPPC_AUTO_ACT_WINDOW_MAX_SIG)
+		auto_act_window = CPPC_AUTO_ACT_WINDOW_MAX_SIG;
+
+	val = (exp << CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) + auto_act_window;
+
+	return cppc_set_reg_val(cpu, AUTO_ACT_WINDOW, val);
+}
+EXPORT_SYMBOL_GPL(cppc_set_auto_act_window);
+
 /**
  * cppc_get_auto_sel() - Read autonomous selection register.
  * @cpu: CPU from which to read register.
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 31767c65be20..325e9543e08f 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;
@@ -159,6 +168,9 @@ 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, bool *enable);
 extern int cppc_set_auto_sel(int cpu, bool enable);
 extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
@@ -229,6 +241,18 @@ static inline int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
 {
 	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, bool *enable)
 {
 	return -EOPNOTSUPP;
-- 
2.33.0

[PATCH v5 8/8] 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                | 109 ++++++++++++++++++
 2 files changed, 163 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index 206079d3bd5b..eaaa553a1819 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:		February 2025
+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 is only present if the cppc-cpufreq driver is in use.
+
+What:		/sys/devices/system/cpu/cpuX/cpufreq/auto_act_window
+Date:		February 2025
+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 is only present if the cppc-cpufreq driver is in use.
+
+What:		/sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference_val
+Date:		February 2025
+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 is only present 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 8f512448382f..16a41fb19c92 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -815,10 +815,119 @@ 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)
+{
+	bool 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, "%d\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)
+{
+	u64 val;
+	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;
+
+	return sysfs_emit(buf, "%llu\n", val);
+}
+
+static ssize_t store_auto_act_window(struct cpufreq_policy *policy,
+				     const char *buf, size_t count)
+{
+	u64 usec;
+	int ret;
+
+	ret = kstrtou64(buf, 0, &usec);
+	if (ret)
+		return ret;
+
+	ret = cppc_set_auto_act_window(policy->cpu, usec);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t show_energy_performance_preference_val(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_performance_preference_val(struct cpufreq_policy *policy,
+						       const char *buf, size_t count)
+{
+	u64 val;
+	int ret;
+
+	ret = kstrtou64(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	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_performance_preference_val);
 
 static struct freq_attr *cppc_cpufreq_attr[] = {
 	&freqdomain_cpus,
+	&auto_select,
+	&auto_act_window,
+	&energy_performance_preference_val,
 	NULL,
 };
 
-- 
2.33.0

Re: [PATCH v5 0/8] Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

Hello Rafael & Viresh,

There are two things about this patchset I would like your advice on:

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

2. In intel_pstate driver, there is a file named
"energy_performance_preference", which accepts not only raw value but also
a few string arguments and converts them to integers. But I think this
implementation is not really nice and prefer not to follow it [2]. To
distinguish it, I named the file in cppc_cpufreq
"energy_performance_preference_val" instead of
"energy_performance_preference". Should I follow the implementation of
intel_pstate?     

Looking forward to your reply!

Relevant discussion:
[1] https://lore.kernel.org/all/522721da-1a5c-439c-96a8-d0300dd0f906@huawei.com/
[2] https://lore.kernel.org/all/0c511da2-6a4a-4fa2-9d82-da45d1afe346@huawei.com/

Re: [PATCH v5 0/8] Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

On 2025/2/6 21:14, 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-5 refactor out the general CPPC register get and set functions
>    in cppc_acpi.c
> 
>  - patches 6-8 expose sysfs files for users to control CPPC autonomous
>    selection when supported
> 
> Changelog:
> 
> v5:
> 
>  - add more explanation to the commit logs and comments
>  - change REG_OPTIONAL from bin to hex
>  - split patch 2 into 3 smaller patches
>  - remove CPPC_REG_VAL_READ() and CPPC_REG_VAL_WRITE() macros
>  - move the modification part in patch 5 into a separate patch
>  - rename the sysfs file from "energy_perf" to
>    energy_performance_preference_val
> 
> v4:
> 
>  - add REG_OPTIONAL and IS_OPTIONAL_CPC_REG to judge if a cpc register is
>    an optional one
>  - check whether the register is optional before CPC_SUPPORTED check in
>    cppc_get_reg_val() and cppc_set_reg_val()
>  - check the register's type in cppc_set_reg_val()
>  - add macros to generally implement registers getting and setting
>    functions
>  - move some logic codes from cppc_cpufreq.c to cppc_acpi.c
>  - replace cppc_get_auto_sel_caps() by cppc_get_auto_sel()
> 
> v3:
> 
>  - 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
> 
> v2:
> 
>  - fix some incorrect placeholder
>  - change kstrtoul to kstrtobool in store_auto_select
> 
> Lifeng Zheng (8):
>   ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is
>     optional
>   ACPI: CPPC: Optimize cppc_get_perf()
>   ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()
>   ACPI: CPPC: Add cppc_set_reg_val()
>   ACPI: CPPC: Refactor register value get and set ABIs
>   ACPI: CPPC: Modify cppc_get_auto_sel_caps() to cppc_get_auto_sel()
>   ACPI: CPPC: Add three functions related to autonomous selection
>   cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq
> 
>  .../ABI/testing/sysfs-devices-system-cpu      |  54 ++++
>  drivers/acpi/cppc_acpi.c                      | 303 +++++++++++-------
>  drivers/cpufreq/amd-pstate.c                  |   3 +-
>  drivers/cpufreq/cppc_cpufreq.c                | 109 +++++++
>  include/acpi/cppc_acpi.h                      |  30 +-
>  5 files changed, 372 insertions(+), 127 deletions(-)
> 

Gentle ping.

Attach discussions of previous versions:
v1: https://lore.kernel.org/all/20241114084816.1128647-1-zhenglifeng1@huawei.com/
v2: https://lore.kernel.org/all/20241122062051.3658577-1-zhenglifeng1@huawei.com/
v3: https://lore.kernel.org/all/20241216091603.1247644-1-zhenglifeng1@huawei.com/
v4: https://lore.kernel.org/all/20250113122104.3870673-1-zhenglifeng1@huawei.com/

Re: [PATCH v5 0/8] Support for autonomous selection in cppc_cpufreq

[Rafael J. Wysocki]

On Thu, Feb 13, 2025 at 2:55 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote:
>
> On 2025/2/6 21:14, 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-5 refactor out the general CPPC register get and set functions
> >    in cppc_acpi.c
> >
> >  - patches 6-8 expose sysfs files for users to control CPPC autonomous
> >    selection when supported
> >
> > Changelog:
> >
> > v5:
> >
> >  - add more explanation to the commit logs and comments
> >  - change REG_OPTIONAL from bin to hex
> >  - split patch 2 into 3 smaller patches
> >  - remove CPPC_REG_VAL_READ() and CPPC_REG_VAL_WRITE() macros
> >  - move the modification part in patch 5 into a separate patch
> >  - rename the sysfs file from "energy_perf" to
> >    energy_performance_preference_val
> >
> > v4:
> >
> >  - add REG_OPTIONAL and IS_OPTIONAL_CPC_REG to judge if a cpc register is
> >    an optional one
> >  - check whether the register is optional before CPC_SUPPORTED check in
> >    cppc_get_reg_val() and cppc_set_reg_val()
> >  - check the register's type in cppc_set_reg_val()
> >  - add macros to generally implement registers getting and setting
> >    functions
> >  - move some logic codes from cppc_cpufreq.c to cppc_acpi.c
> >  - replace cppc_get_auto_sel_caps() by cppc_get_auto_sel()
> >
> > v3:
> >
> >  - 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
> >
> > v2:
> >
> >  - fix some incorrect placeholder
> >  - change kstrtoul to kstrtobool in store_auto_select
> >
> > Lifeng Zheng (8):
> >   ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is
> >     optional
> >   ACPI: CPPC: Optimize cppc_get_perf()
> >   ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()
> >   ACPI: CPPC: Add cppc_set_reg_val()
> >   ACPI: CPPC: Refactor register value get and set ABIs
> >   ACPI: CPPC: Modify cppc_get_auto_sel_caps() to cppc_get_auto_sel()
> >   ACPI: CPPC: Add three functions related to autonomous selection
> >   cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq
> >
> >  .../ABI/testing/sysfs-devices-system-cpu      |  54 ++++
> >  drivers/acpi/cppc_acpi.c                      | 303 +++++++++++-------
> >  drivers/cpufreq/amd-pstate.c                  |   3 +-
> >  drivers/cpufreq/cppc_cpufreq.c                | 109 +++++++
> >  include/acpi/cppc_acpi.h                      |  30 +-
> >  5 files changed, 372 insertions(+), 127 deletions(-)
> >
>
> Gentle ping.

OK, so I'm wondering how this is related to the patch series at

https://lore.kernel.org/linux-acpi/20250211103737.447704-1-sumitg@nvidia.com/

> Attach discussions of previous versions:
> v1: https://lore.kernel.org/all/20241114084816.1128647-1-zhenglifeng1@huawei.com/
> v2: https://lore.kernel.org/all/20241122062051.3658577-1-zhenglifeng1@huawei.com/
> v3: https://lore.kernel.org/all/20241216091603.1247644-1-zhenglifeng1@huawei.com/
> v4: https://lore.kernel.org/all/20250113122104.3870673-1-zhenglifeng1@huawei.com/
>

Re: [PATCH v5 0/8] Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

On 2025/2/19 3:17, Rafael J. Wysocki wrote:
> On Thu, Feb 13, 2025 at 2:55 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote:
>>
>> On 2025/2/6 21:14, 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-5 refactor out the general CPPC register get and set functions
>>>    in cppc_acpi.c
>>>
>>>  - patches 6-8 expose sysfs files for users to control CPPC autonomous
>>>    selection when supported
>>>
>>> Changelog:
>>>
>>> v5:
>>>
>>>  - add more explanation to the commit logs and comments
>>>  - change REG_OPTIONAL from bin to hex
>>>  - split patch 2 into 3 smaller patches
>>>  - remove CPPC_REG_VAL_READ() and CPPC_REG_VAL_WRITE() macros
>>>  - move the modification part in patch 5 into a separate patch
>>>  - rename the sysfs file from "energy_perf" to
>>>    energy_performance_preference_val
>>>
>>> v4:
>>>
>>>  - add REG_OPTIONAL and IS_OPTIONAL_CPC_REG to judge if a cpc register is
>>>    an optional one
>>>  - check whether the register is optional before CPC_SUPPORTED check in
>>>    cppc_get_reg_val() and cppc_set_reg_val()
>>>  - check the register's type in cppc_set_reg_val()
>>>  - add macros to generally implement registers getting and setting
>>>    functions
>>>  - move some logic codes from cppc_cpufreq.c to cppc_acpi.c
>>>  - replace cppc_get_auto_sel_caps() by cppc_get_auto_sel()
>>>
>>> v3:
>>>
>>>  - 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
>>>
>>> v2:
>>>
>>>  - fix some incorrect placeholder
>>>  - change kstrtoul to kstrtobool in store_auto_select
>>>
>>> Lifeng Zheng (8):
>>>   ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is
>>>     optional
>>>   ACPI: CPPC: Optimize cppc_get_perf()
>>>   ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()
>>>   ACPI: CPPC: Add cppc_set_reg_val()
>>>   ACPI: CPPC: Refactor register value get and set ABIs
>>>   ACPI: CPPC: Modify cppc_get_auto_sel_caps() to cppc_get_auto_sel()
>>>   ACPI: CPPC: Add three functions related to autonomous selection
>>>   cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq
>>>
>>>  .../ABI/testing/sysfs-devices-system-cpu      |  54 ++++
>>>  drivers/acpi/cppc_acpi.c                      | 303 +++++++++++-------
>>>  drivers/cpufreq/amd-pstate.c                  |   3 +-
>>>  drivers/cpufreq/cppc_cpufreq.c                | 109 +++++++
>>>  include/acpi/cppc_acpi.h                      |  30 +-
>>>  5 files changed, 372 insertions(+), 127 deletions(-)
>>>
>>
>> Gentle ping.
> 
> OK, so I'm wondering how this is related to the patch series at
> 
> https://lore.kernel.org/linux-acpi/20250211103737.447704-1-sumitg@nvidia.com/

This series refactors some cppc_acpi ABIs and supports cppc autonomous
selection with sysfs files in cpufreq policy.  Later, [1] proposed another
design with different user interfaces.We will discuss and reach a consensus
with regard to this.

However, as mentioned in [1], patch 1-7 in this series (the cppc_acpi part)
are not related to user interfaces, so can be reviewed and applied
separately.  I can also send patch 1-7 as a new thread if preferred.

[1] https://lore.kernel.org/linux-acpi/20250211103737.447704-1-sumitg@nvidia.com/

> 
>> Attach discussions of previous versions:
>> v1: https://lore.kernel.org/all/20241114084816.1128647-1-zhenglifeng1@huawei.com/
>> v2: https://lore.kernel.org/all/20241122062051.3658577-1-zhenglifeng1@huawei.com/
>> v3: https://lore.kernel.org/all/20241216091603.1247644-1-zhenglifeng1@huawei.com/
>> v4: https://lore.kernel.org/all/20250113122104.3870673-1-zhenglifeng1@huawei.com/
>>
> 

Re: [PATCH v5 0/8] Support for autonomous selection in cppc_cpufreq

[Pierre Gondois]

On 2/22/25 11:07, zhenglifeng (A) wrote:
> On 2025/2/19 3:17, Rafael J. Wysocki wrote:
>> On Thu, Feb 13, 2025 at 2:55 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote:
>>>
>>> On 2025/2/6 21:14, 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-5 refactor out the general CPPC register get and set functions
>>>>     in cppc_acpi.c
>>>>
>>>>   - patches 6-8 expose sysfs files for users to control CPPC autonomous
>>>>     selection when supported
>>>>
>>>> Changelog:
>>>>
>>>> v5:
>>>>
>>>>   - add more explanation to the commit logs and comments
>>>>   - change REG_OPTIONAL from bin to hex
>>>>   - split patch 2 into 3 smaller patches
>>>>   - remove CPPC_REG_VAL_READ() and CPPC_REG_VAL_WRITE() macros
>>>>   - move the modification part in patch 5 into a separate patch
>>>>   - rename the sysfs file from "energy_perf" to
>>>>     energy_performance_preference_val
>>>>
>>>> v4:
>>>>
>>>>   - add REG_OPTIONAL and IS_OPTIONAL_CPC_REG to judge if a cpc register is
>>>>     an optional one
>>>>   - check whether the register is optional before CPC_SUPPORTED check in
>>>>     cppc_get_reg_val() and cppc_set_reg_val()
>>>>   - check the register's type in cppc_set_reg_val()
>>>>   - add macros to generally implement registers getting and setting
>>>>     functions
>>>>   - move some logic codes from cppc_cpufreq.c to cppc_acpi.c
>>>>   - replace cppc_get_auto_sel_caps() by cppc_get_auto_sel()
>>>>
>>>> v3:
>>>>
>>>>   - 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
>>>>
>>>> v2:
>>>>
>>>>   - fix some incorrect placeholder
>>>>   - change kstrtoul to kstrtobool in store_auto_select
>>>>
>>>> Lifeng Zheng (8):
>>>>    ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is
>>>>      optional
>>>>    ACPI: CPPC: Optimize cppc_get_perf()
>>>>    ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()
>>>>    ACPI: CPPC: Add cppc_set_reg_val()
>>>>    ACPI: CPPC: Refactor register value get and set ABIs
>>>>    ACPI: CPPC: Modify cppc_get_auto_sel_caps() to cppc_get_auto_sel()
>>>>    ACPI: CPPC: Add three functions related to autonomous selection
>>>>    cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq
>>>>
>>>>   .../ABI/testing/sysfs-devices-system-cpu      |  54 ++++
>>>>   drivers/acpi/cppc_acpi.c                      | 303 +++++++++++-------
>>>>   drivers/cpufreq/amd-pstate.c                  |   3 +-
>>>>   drivers/cpufreq/cppc_cpufreq.c                | 109 +++++++
>>>>   include/acpi/cppc_acpi.h                      |  30 +-
>>>>   5 files changed, 372 insertions(+), 127 deletions(-)
>>>>
>>>
>>> Gentle ping.
>>
>> OK, so I'm wondering how this is related to the patch series at
>>
>> https://lore.kernel.org/linux-acpi/20250211103737.447704-1-sumitg@nvidia.com/
> 
> This series refactors some cppc_acpi ABIs and supports cppc autonomous
> selection with sysfs files in cpufreq policy.  Later, [1] proposed another
> design with different user interfaces.We will discuss and reach a consensus
> with regard to this.
> 
> However, as mentioned in [1], patch 1-7 in this series (the cppc_acpi part)
> are not related to user interfaces, so can be reviewed and applied
> separately.  I can also send patch 1-7 as a new thread if preferred.
> 
> [1] https://lore.kernel.org/linux-acpi/20250211103737.447704-1-sumitg@nvidia.com/

I tried the patchset on a platform which doesn't implement CPC and everything worked well.
As Lifeng said,
   PATCH v5 8/8] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq
seems to be still in discussion, but for patches 1-7 FWIW:

Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

Regards,
Pierre

> 
>>
>>> Attach discussions of previous versions:
>>> v1: https://lore.kernel.org/all/20241114084816.1128647-1-zhenglifeng1@huawei.com/
>>> v2: https://lore.kernel.org/all/20241122062051.3658577-1-zhenglifeng1@huawei.com/
>>> v3: https://lore.kernel.org/all/20241216091603.1247644-1-zhenglifeng1@huawei.com/
>>> v4: https://lore.kernel.org/all/20250113122104.3870673-1-zhenglifeng1@huawei.com/
>>>
>>
> 
> 

Re: [PATCH v5 0/8] Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

On 2025/2/24 18:31, Pierre Gondois wrote:
> 
> 
> On 2/22/25 11:07, zhenglifeng (A) wrote:
>> On 2025/2/19 3:17, Rafael J. Wysocki wrote:
>>> On Thu, Feb 13, 2025 at 2:55 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote:
>>>>
>>>> On 2025/2/6 21:14, 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-5 refactor out the general CPPC register get and set functions
>>>>>     in cppc_acpi.c
>>>>>
>>>>>   - patches 6-8 expose sysfs files for users to control CPPC autonomous
>>>>>     selection when supported
>>>>>
>>>>> Changelog:
>>>>>
>>>>> v5:
>>>>>
>>>>>   - add more explanation to the commit logs and comments
>>>>>   - change REG_OPTIONAL from bin to hex
>>>>>   - split patch 2 into 3 smaller patches
>>>>>   - remove CPPC_REG_VAL_READ() and CPPC_REG_VAL_WRITE() macros
>>>>>   - move the modification part in patch 5 into a separate patch
>>>>>   - rename the sysfs file from "energy_perf" to
>>>>>     energy_performance_preference_val
>>>>>
>>>>> v4:
>>>>>
>>>>>   - add REG_OPTIONAL and IS_OPTIONAL_CPC_REG to judge if a cpc register is
>>>>>     an optional one
>>>>>   - check whether the register is optional before CPC_SUPPORTED check in
>>>>>     cppc_get_reg_val() and cppc_set_reg_val()
>>>>>   - check the register's type in cppc_set_reg_val()
>>>>>   - add macros to generally implement registers getting and setting
>>>>>     functions
>>>>>   - move some logic codes from cppc_cpufreq.c to cppc_acpi.c
>>>>>   - replace cppc_get_auto_sel_caps() by cppc_get_auto_sel()
>>>>>
>>>>> v3:
>>>>>
>>>>>   - 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
>>>>>
>>>>> v2:
>>>>>
>>>>>   - fix some incorrect placeholder
>>>>>   - change kstrtoul to kstrtobool in store_auto_select
>>>>>
>>>>> Lifeng Zheng (8):
>>>>>    ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is
>>>>>      optional
>>>>>    ACPI: CPPC: Optimize cppc_get_perf()
>>>>>    ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()
>>>>>    ACPI: CPPC: Add cppc_set_reg_val()
>>>>>    ACPI: CPPC: Refactor register value get and set ABIs
>>>>>    ACPI: CPPC: Modify cppc_get_auto_sel_caps() to cppc_get_auto_sel()
>>>>>    ACPI: CPPC: Add three functions related to autonomous selection
>>>>>    cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq
>>>>>
>>>>>   .../ABI/testing/sysfs-devices-system-cpu      |  54 ++++
>>>>>   drivers/acpi/cppc_acpi.c                      | 303 +++++++++++-------
>>>>>   drivers/cpufreq/amd-pstate.c                  |   3 +-
>>>>>   drivers/cpufreq/cppc_cpufreq.c                | 109 +++++++
>>>>>   include/acpi/cppc_acpi.h                      |  30 +-
>>>>>   5 files changed, 372 insertions(+), 127 deletions(-)
>>>>>
>>>>
>>>> Gentle ping.
>>>
>>> OK, so I'm wondering how this is related to the patch series at
>>>
>>> https://lore.kernel.org/linux-acpi/20250211103737.447704-1-sumitg@nvidia.com/
>>
>> This series refactors some cppc_acpi ABIs and supports cppc autonomous
>> selection with sysfs files in cpufreq policy.  Later, [1] proposed another
>> design with different user interfaces.We will discuss and reach a consensus
>> with regard to this.
>>
>> However, as mentioned in [1], patch 1-7 in this series (the cppc_acpi part)
>> are not related to user interfaces, so can be reviewed and applied
>> separately.  I can also send patch 1-7 as a new thread if preferred.
>>
>> [1] https://lore.kernel.org/linux-acpi/20250211103737.447704-1-sumitg@nvidia.com/
> 
> I tried the patchset on a platform which doesn't implement CPC and everything worked well.
> As Lifeng said,
>   PATCH v5 8/8] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq
> seems to be still in discussion, but for patches 1-7 FWIW:
> 
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

Thank you!

Regards,
Lifeng

> 
> Regards,
> Pierre
> 
>>
>>>
>>>> Attach discussions of previous versions:
>>>> v1: https://lore.kernel.org/all/20241114084816.1128647-1-zhenglifeng1@huawei.com/
>>>> v2: https://lore.kernel.org/all/20241122062051.3658577-1-zhenglifeng1@huawei.com/
>>>> v3: https://lore.kernel.org/all/20241216091603.1247644-1-zhenglifeng1@huawei.com/
>>>> v4: https://lore.kernel.org/all/20250113122104.3870673-1-zhenglifeng1@huawei.com/
>>>>
>>>
>>
>>
> 

Re: [PATCH v5 3/8] ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()

[Rafael J. Wysocki]

On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
> cppc registers. And extract the operations if register is in pcc out as
> cppc_get_reg_val_in_pcc(). Without functional change.

This should be split into two patches IMV.

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index db22f8f107db..3c9c4ce2a0b0 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1189,48 +1189,52 @@ 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);
> -       struct cpc_register_resource *reg;
> +       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> +       struct cppc_pcc_data *pcc_ss_data = NULL;
> +       int ret;
>
> -       if (!cpc_desc) {
> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> +       if (pcc_ss_id < 0) {
> +               pr_debug("Invalid pcc_ss_id\n");
>                 return -ENODEV;
>         }
>
> -       reg = &cpc_desc->cpc_regs[reg_idx];
> +       pcc_ss_data = pcc_data[pcc_ss_id];
>
> -       if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
> -               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> -               return -EOPNOTSUPP;
> -       }

I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro.  I'm not
convinced at all that it adds any value above (and in the next patch
for that matter) and the message printing the register index is just
plain unuseful to anyone who doesn't know how to decode it.

If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS
regardless of what IS_OPTIONAL_CPC_REG() has to say about it.

> +       down_write(&pcc_ss_data->pcc_lock);
>
> -       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;
> +       if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
> +               ret = cpc_read(cpu, reg, val);
> +       else
> +               ret = -EIO;
>
> -               if (pcc_ss_id < 0) {
> -                       pr_debug("Invalid pcc_ss_id\n");
> -                       return -ENODEV;
> -               }
> +       up_write(&pcc_ss_data->pcc_lock);
>
> -               pcc_ss_data = pcc_data[pcc_ss_id];
> +       return ret;
> +}
>
> -               down_write(&pcc_ss_data->pcc_lock);
> +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 (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
> -                       ret = cpc_read(cpunum, reg, perf);
> -               else
> -                       ret = -EIO;
> +       if (!cpc_desc) {
> +               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> +               return -ENODEV;
> +       }
>
> -               up_write(&pcc_ss_data->pcc_lock);
> +       reg = &cpc_desc->cpc_regs[reg_idx];
>
> -               return ret;
> +       if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
> +               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> +               return -EOPNOTSUPP;
>         }
>
> -       return cpc_read(cpunum, reg, perf);
> +       if (CPC_IN_PCC(reg))
> +               return cppc_get_reg_val_in_pcc(cpu, reg, val);
> +
> +       return cpc_read(cpu, reg, val);
>  }
>
>  /**
> @@ -1242,7 +1246,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>   */
>  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);
>
> @@ -1255,7 +1259,7 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>   */
>  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);
>  }
>
>  /**
> @@ -1267,7 +1271,7 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>   */
>  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);
>
> @@ -1280,7 +1284,7 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>   */
>  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
>
>

Re: [PATCH v5 3/8] ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()

[zhenglifeng (A)]

On 2025/3/13 3:54, Rafael J. Wysocki wrote:

> On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
>> cppc registers. And extract the operations if register is in pcc out as
>> cppc_get_reg_val_in_pcc(). Without functional change.
> 
> This should be split into two patches IMV.

Yes. That makes sense. Thanks.

> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
>>  1 file changed, 35 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index db22f8f107db..3c9c4ce2a0b0 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1189,48 +1189,52 @@ 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);
>> -       struct cpc_register_resource *reg;
>> +       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> +       struct cppc_pcc_data *pcc_ss_data = NULL;
>> +       int ret;
>>
>> -       if (!cpc_desc) {
>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>> +       if (pcc_ss_id < 0) {
>> +               pr_debug("Invalid pcc_ss_id\n");
>>                 return -ENODEV;
>>         }
>>
>> -       reg = &cpc_desc->cpc_regs[reg_idx];
>> +       pcc_ss_data = pcc_data[pcc_ss_id];
>>
>> -       if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
>> -               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> -               return -EOPNOTSUPP;
>> -       }
> 
> I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro.  I'm not
> convinced at all that it adds any value above (and in the next patch
> for that matter) and the message printing the register index is just
> plain unuseful to anyone who doesn't know how to decode it.

With this index, it is easier to locate problems. This is what a "pr_debug"
for, isn't it?

> 
> If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS
> regardless of what IS_OPTIONAL_CPC_REG() has to say about it.

The name "CPC_SUPPORTED" may be a little confused. Actually, in ACPI 6.5,
only optional _CPC package fields that are not supported by the platform
should be encoded as 0 intergers or NULL registers. A mandatory field as a
0 interger is valid. So If I wanted to make this function as a generic one
to read cppc registers, it would have been more reasonable to do this
IS_OPTIONAL_CPC_REG() check before CPC_SUPPORTED().

> 
>> +       down_write(&pcc_ss_data->pcc_lock);
>>
>> -       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;
>> +       if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>> +               ret = cpc_read(cpu, reg, val);
>> +       else
>> +               ret = -EIO;
>>
>> -               if (pcc_ss_id < 0) {
>> -                       pr_debug("Invalid pcc_ss_id\n");
>> -                       return -ENODEV;
>> -               }
>> +       up_write(&pcc_ss_data->pcc_lock);
>>
>> -               pcc_ss_data = pcc_data[pcc_ss_id];
>> +       return ret;
>> +}
>>
>> -               down_write(&pcc_ss_data->pcc_lock);
>> +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 (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>> -                       ret = cpc_read(cpunum, reg, perf);
>> -               else
>> -                       ret = -EIO;
>> +       if (!cpc_desc) {
>> +               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> +               return -ENODEV;
>> +       }
>>
>> -               up_write(&pcc_ss_data->pcc_lock);
>> +       reg = &cpc_desc->cpc_regs[reg_idx];
>>
>> -               return ret;
>> +       if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
>> +               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>> +               return -EOPNOTSUPP;
>>         }
>>
>> -       return cpc_read(cpunum, reg, perf);
>> +       if (CPC_IN_PCC(reg))
>> +               return cppc_get_reg_val_in_pcc(cpu, reg, val);
>> +
>> +       return cpc_read(cpu, reg, val);
>>  }
>>
>>  /**
>> @@ -1242,7 +1246,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>>   */
>>  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);
>>
>> @@ -1255,7 +1259,7 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
>>   */
>>  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);
>>  }
>>
>>  /**
>> @@ -1267,7 +1271,7 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
>>   */
>>  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);
>>
>> @@ -1280,7 +1284,7 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>   */
>>  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
>>
>>
> 

Re: [PATCH v5 3/8] ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()

[Rafael J. Wysocki]

On Fri, Mar 14, 2025 at 10:25 AM zhenglifeng (A)
<zhenglifeng1@huawei.com> wrote:
>
> On 2025/3/13 3:54, Rafael J. Wysocki wrote:
>
> > On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
> >>
> >> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
> >> cppc registers. And extract the operations if register is in pcc out as
> >> cppc_get_reg_val_in_pcc(). Without functional change.
> >
> > This should be split into two patches IMV.
>
> Yes. That makes sense. Thanks.
>
> >
> >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> >> ---
> >>  drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
> >>  1 file changed, 35 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> >> index db22f8f107db..3c9c4ce2a0b0 100644
> >> --- a/drivers/acpi/cppc_acpi.c
> >> +++ b/drivers/acpi/cppc_acpi.c
> >> @@ -1189,48 +1189,52 @@ 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);
> >> -       struct cpc_register_resource *reg;
> >> +       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> >> +       struct cppc_pcc_data *pcc_ss_data = NULL;
> >> +       int ret;
> >>
> >> -       if (!cpc_desc) {
> >> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> >> +       if (pcc_ss_id < 0) {
> >> +               pr_debug("Invalid pcc_ss_id\n");
> >>                 return -ENODEV;
> >>         }
> >>
> >> -       reg = &cpc_desc->cpc_regs[reg_idx];
> >> +       pcc_ss_data = pcc_data[pcc_ss_id];
> >>
> >> -       if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
> >> -               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
> >> -               return -EOPNOTSUPP;
> >> -       }
> >
> > I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro.  I'm not
> > convinced at all that it adds any value above (and in the next patch
> > for that matter) and the message printing the register index is just
> > plain unuseful to anyone who doesn't know how to decode it.
>
> With this index, it is easier to locate problems. This is what a "pr_debug"
> for, isn't it?

For those who know how to decode it, yes.  For others, not really.

> >
> > If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS
> > regardless of what IS_OPTIONAL_CPC_REG() has to say about it.
>
> The name "CPC_SUPPORTED" may be a little confused. Actually, in ACPI 6.5,
> only optional _CPC package fields that are not supported by the platform
> should be encoded as 0 intergers or NULL registers. A mandatory field as a
> 0 interger is valid. So If I wanted to make this function as a generic one
> to read cppc registers, it would have been more reasonable to do this
> IS_OPTIONAL_CPC_REG() check before CPC_SUPPORTED().

I see, so you need to explain this in the changelog.

And IMV the code logic should be:

(1) If this is a NULL register, don't use it.
(2) If it is integer 0, check if it is optional.
    (a) If it is optional, don't use it.
    (b) Otherwise, use 0 as the value.

Of course, there is a problem for platforms that may want to pass 0 as
an optional field value, but this is a spec issue.

Re: [PATCH v5 3/8] ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()

[zhenglifeng (A)]

On 2025/3/14 18:32, Rafael J. Wysocki wrote:

> On Fri, Mar 14, 2025 at 10:25 AM zhenglifeng (A)
> <zhenglifeng1@huawei.com> wrote:
>>
>> On 2025/3/13 3:54, Rafael J. Wysocki wrote:
>>
>>> On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>>>
>>>> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
>>>> cppc registers. And extract the operations if register is in pcc out as
>>>> cppc_get_reg_val_in_pcc(). Without functional change.
>>>
>>> This should be split into two patches IMV.
>>
>> Yes. That makes sense. Thanks.
>>
>>>
>>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>>>> ---
>>>>  drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
>>>>  1 file changed, 35 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>>> index db22f8f107db..3c9c4ce2a0b0 100644
>>>> --- a/drivers/acpi/cppc_acpi.c
>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>> @@ -1189,48 +1189,52 @@ 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);
>>>> -       struct cpc_register_resource *reg;
>>>> +       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>>> +       struct cppc_pcc_data *pcc_ss_data = NULL;
>>>> +       int ret;
>>>>
>>>> -       if (!cpc_desc) {
>>>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>>>> +       if (pcc_ss_id < 0) {
>>>> +               pr_debug("Invalid pcc_ss_id\n");
>>>>                 return -ENODEV;
>>>>         }
>>>>
>>>> -       reg = &cpc_desc->cpc_regs[reg_idx];
>>>> +       pcc_ss_data = pcc_data[pcc_ss_id];
>>>>
>>>> -       if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) {
>>>> -               pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx);
>>>> -               return -EOPNOTSUPP;
>>>> -       }
>>>
>>> I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro.  I'm not
>>> convinced at all that it adds any value above (and in the next patch
>>> for that matter) and the message printing the register index is just
>>> plain unuseful to anyone who doesn't know how to decode it.
>>
>> With this index, it is easier to locate problems. This is what a "pr_debug"
>> for, isn't it?
> 
> For those who know how to decode it, yes.  For others, not really.

At least it means something. But if you think this index is confusing for
those who don't know how to decode it, I'll remove it in the next version.

> 
>>>
>>> If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS
>>> regardless of what IS_OPTIONAL_CPC_REG() has to say about it.
>>
>> The name "CPC_SUPPORTED" may be a little confused. Actually, in ACPI 6.5,
>> only optional _CPC package fields that are not supported by the platform
>> should be encoded as 0 intergers or NULL registers. A mandatory field as a
>> 0 interger is valid. So If I wanted to make this function as a generic one
>> to read cppc registers, it would have been more reasonable to do this
>> IS_OPTIONAL_CPC_REG() check before CPC_SUPPORTED().
> 
> I see, so you need to explain this in the changelog.

OK.

> 
> And IMV the code logic should be:
> 
> (1) If this is a NULL register, don't use it.
> (2) If it is integer 0, check if it is optional.
>     (a) If it is optional, don't use it.
>     (b) Otherwise, use 0 as the value.
> 
> Of course, there is a problem for platforms that may want to pass 0 as
> an optional field value, but this is a spec issue.

I'll change the logic in next version. Thanks!