[PATCH v4 0/6] Support for autonomous selection in cppc_cpufreq

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

 - patches 5-6 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/

Changelog:

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 (6):
  ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro
  ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function
  ACPI: CPPC: Add macros to generally implement registers getting and
    setting functions
  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                      | 332 +++++++++---------
 drivers/cpufreq/amd-pstate.c                  |   3 +-
 drivers/cpufreq/cppc_cpufreq.c                | 109 ++++++
 include/acpi/cppc_acpi.h                      |  30 +-
 5 files changed, 359 insertions(+), 169 deletions(-)

-- 
2.33.0

[PATCH v4 1/6] ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro

[Lifeng Zheng]

Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is an optional one.

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index f193e713825a..6454b469338f 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -129,6 +129,12 @@ 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))
+
+/* These indicate optional of the per-cpu cpc_regs[]. */
+#define REG_OPTIONAL (0b111111100011111010000)
+
+#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

Re: [PATCH v4 1/6] ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro

[Rafael J. Wysocki]

On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is an optional one.

This requires a bit more explanation, especially what's the purpose of
it (ie. the "why").

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/acpi/cppc_acpi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index f193e713825a..6454b469338f 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -129,6 +129,12 @@ 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))
> +
> +/* These indicate optional of the per-cpu cpc_regs[]. */

Again, you need to say more here, like how this is supposed to work.

> +#define REG_OPTIONAL (0b111111100011111010000)

A hex literal would work too AFAICS.

> +
> +#define IS_OPTIONAL_CPC_REG(reg_idx) (REG_OPTIONAL & (1U << (reg_idx)))

You need to explain what reg_idx is.

> +
>  /*
>   * Arbitrary Retries in case the remote processor is slow to respond
>   * to PCC commands. Keeping it high enough to cover emulators where
> --

Re: [PATCH v4 1/6] ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro

[zhenglifeng (A)]

On 2025/1/14 21:27, Rafael J. Wysocki wrote:

> On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> Add IS_OPTIONAL_CPC_REG macro to judge if a cpc_reg is an optional one.
> 
> This requires a bit more explanation, especially what's the purpose of
> it (ie. the "why").

Will add more explanation. Thanks.

> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/acpi/cppc_acpi.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index f193e713825a..6454b469338f 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -129,6 +129,12 @@ 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))
>> +
>> +/* These indicate optional of the per-cpu cpc_regs[]. */
> 
> Again, you need to say more here, like how this is supposed to work.

Will add it. Thanks.

> 
>> +#define REG_OPTIONAL (0b111111100011111010000)
> 
> A hex literal would work too AFAICS.

Will change it. Thanks.

> 
>> +
>> +#define IS_OPTIONAL_CPC_REG(reg_idx) (REG_OPTIONAL & (1U << (reg_idx)))
> 
> You need to explain what reg_idx is.

Will add more annotations. Thanks.

> 
>> +
>>  /*
>>   * Arbitrary Retries in case the remote processor is slow to respond
>>   * to PCC commands. Keeping it high enough to cover emulators where
>> --

[PATCH v4 2/6] 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 mandatory or 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() 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 | 105 ++++++++++++++++++++++++++++++---------
 1 file changed, 82 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 6454b469338f..571f94855dce 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1181,43 +1181,102 @@ 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 (IS_OPTIONAL_CPC_REG(reg_idx) && !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 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);
 }
 
 /**
@@ -1229,7 +1288,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);
 
@@ -1242,7 +1301,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);
 }
 
 /**
@@ -1254,7 +1313,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);
 
@@ -1267,7 +1326,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 v4 2/6] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[Rafael J. Wysocki]

The word "function" at the end of the subject is redundant IMV.

On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> 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 mandatory or 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() 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.

This mixes functional changes with function renames and code
refactoring while it is better to do all of these things separately.

Why don't you split the patch into a few smaller patches doing each
one thing at a time?  Like rename the existing function and refactor
it in one patch (if this makes sense), make functional changes to it
in another patch, then add new functions in a third one?

This would help to review the changes and explain why each of them is made.

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/acpi/cppc_acpi.c | 105 ++++++++++++++++++++++++++++++---------
>  1 file changed, 82 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 6454b469338f..571f94855dce 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1181,43 +1181,102 @@ 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 (IS_OPTIONAL_CPC_REG(reg_idx) && !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 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);
>  }
>
>  /**
> @@ -1229,7 +1288,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);
>
> @@ -1242,7 +1301,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);
>  }
>
>  /**
> @@ -1254,7 +1313,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);
>
> @@ -1267,7 +1326,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 v4 2/6] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[zhenglifeng (A)]

On 2025/1/15 1:41, Rafael J. Wysocki wrote:

> The word "function" at the end of the subject is redundant IMV.

Yes, you are right. Will delete it. Thanks.

> 
> On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> 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 mandatory or 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() 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.
> 
> This mixes functional changes with function renames and code
> refactoring while it is better to do all of these things separately.
> 
> Why don't you split the patch into a few smaller patches doing each
> one thing at a time?  Like rename the existing function and refactor
> it in one patch (if this makes sense), make functional changes to it
> in another patch, then add new functions in a third one?
> 
> This would help to review the changes and explain why each of them is made.

It does make more sense. Will split it. Thanks.

> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/acpi/cppc_acpi.c | 105 ++++++++++++++++++++++++++++++---------
>>  1 file changed, 82 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 6454b469338f..571f94855dce 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1181,43 +1181,102 @@ 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 (IS_OPTIONAL_CPC_REG(reg_idx) && !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 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);
>>  }
>>
>>  /**
>> @@ -1229,7 +1288,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);
>>
>> @@ -1242,7 +1301,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);
>>  }
>>
>>  /**
>> @@ -1254,7 +1313,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);
>>
>> @@ -1267,7 +1326,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 v4 3/6] ACPI: CPPC: Add macros to generally implement registers getting and setting functions

[Lifeng Zheng]

Add CPPC_REG_VAL_READ() to implement registers getting functions.

Add CPPC_REG_VAL_WRITE() to implement registers setting functions.

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 571f94855dce..6326a1536cda 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
 	return cpc_write(cpu, reg, val);
 }
 
+#define CPPC_REG_VAL_READ(reg_name, reg_idx)		\
+int cppc_get_##reg_name(int cpu, u64 *val)		\
+{							\
+	return cppc_get_reg_val(cpu, reg_idx, val);	\
+}							\
+EXPORT_SYMBOL_GPL(cppc_get_##reg_name)
+
+#define CPPC_REG_VAL_WRITE(reg_name, reg_idx)		\
+int cppc_set_##reg_name(int cpu, u64 val)		\
+{							\
+	return cppc_set_reg_val(cpu, reg_idx, val);	\
+}							\
+EXPORT_SYMBOL_GPL(cppc_set_##reg_name)
+
 /**
  * cppc_get_desired_perf - Get the desired performance register value.
  * @cpunum: CPU from which to get desired performance.
-- 
2.33.0

Re: [PATCH v4 3/6] ACPI: CPPC: Add macros to generally implement registers getting and setting functions

[Rafael J. Wysocki]

On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Add CPPC_REG_VAL_READ() to implement registers getting functions.
>
> Add CPPC_REG_VAL_WRITE() to implement registers setting functions.
>
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>

I don't particularly like these macros as they will generally make it
harder to follow the code.

> ---
>  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 571f94855dce..6326a1536cda 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
>         return cpc_write(cpu, reg, val);
>  }
>
> +#define CPPC_REG_VAL_READ(reg_name, reg_idx)           \
> +int cppc_get_##reg_name(int cpu, u64 *val)             \
> +{                                                      \
> +       return cppc_get_reg_val(cpu, reg_idx, val);     \
> +}                                                      \
> +EXPORT_SYMBOL_GPL(cppc_get_##reg_name)

What about if defining something like

#define CPPC_READ_REG_VAL(cpu, reg_name, val)
cppc_get_reg_val((cpu), CPPC_REG_IDX(reg_name), (val))

(and analogously for the WRITE_ part), where CPPC_REG_IDX(reg_name) is

#define CPPC_REG_IDX(reg_name)    CPPC_REG_##reg_name_IDX

and there are CPPC_REG_##reg_name_IDX macros defined for all register
names in use?

For example

#define CPPC_REG_desired_perf_IDX   DESIRED_PERF

Re: [PATCH v4 3/6] ACPI: CPPC: Add macros to generally implement registers getting and setting functions

[zhenglifeng (A)]

On 2025/1/15 1:58, Rafael J. Wysocki wrote:

> On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> Add CPPC_REG_VAL_READ() to implement registers getting functions.
>>
>> Add CPPC_REG_VAL_WRITE() to implement registers setting functions.
>>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> 
> I don't particularly like these macros as they will generally make it
> harder to follow the code.
> 
>> ---
>>  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 571f94855dce..6326a1536cda 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
>>         return cpc_write(cpu, reg, val);
>>  }
>>
>> +#define CPPC_REG_VAL_READ(reg_name, reg_idx)           \
>> +int cppc_get_##reg_name(int cpu, u64 *val)             \
>> +{                                                      \
>> +       return cppc_get_reg_val(cpu, reg_idx, val);     \
>> +}                                                      \
>> +EXPORT_SYMBOL_GPL(cppc_get_##reg_name)
> 
> What about if defining something like
> 
> #define CPPC_READ_REG_VAL(cpu, reg_name, val)
> cppc_get_reg_val((cpu), CPPC_REG_IDX(reg_name), (val))
> 
> (and analogously for the WRITE_ part), where CPPC_REG_IDX(reg_name) is
> 
> #define CPPC_REG_IDX(reg_name)    CPPC_REG_##reg_name_IDX
> 
> and there are CPPC_REG_##reg_name_IDX macros defined for all register
> names in use?
> 
> For example
> 
> #define CPPC_REG_desired_perf_IDX   DESIRED_PERF

What about keeping these two macros but replace reg_idx with
CPPC_REG_IDX(reg_name)? With this, the only needed parameter for these two
macros is reg_name.

Re: [PATCH v4 3/6] ACPI: CPPC: Add macros to generally implement registers getting and setting functions

[Rafael J. Wysocki]

On Wed, Jan 15, 2025 at 9:59 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote:
>
> On 2025/1/15 1:58, Rafael J. Wysocki wrote:
>
> > On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
> >>
> >> Add CPPC_REG_VAL_READ() to implement registers getting functions.
> >>
> >> Add CPPC_REG_VAL_WRITE() to implement registers setting functions.
> >>
> >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> >
> > I don't particularly like these macros as they will generally make it
> > harder to follow the code.
> >
> >> ---
> >>  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 571f94855dce..6326a1536cda 100644
> >> --- a/drivers/acpi/cppc_acpi.c
> >> +++ b/drivers/acpi/cppc_acpi.c
> >> @@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
> >>         return cpc_write(cpu, reg, val);
> >>  }
> >>
> >> +#define CPPC_REG_VAL_READ(reg_name, reg_idx)           \
> >> +int cppc_get_##reg_name(int cpu, u64 *val)             \
> >> +{                                                      \
> >> +       return cppc_get_reg_val(cpu, reg_idx, val);     \
> >> +}                                                      \
> >> +EXPORT_SYMBOL_GPL(cppc_get_##reg_name)
> >
> > What about if defining something like
> >
> > #define CPPC_READ_REG_VAL(cpu, reg_name, val)
> > cppc_get_reg_val((cpu), CPPC_REG_IDX(reg_name), (val))
> >
> > (and analogously for the WRITE_ part), where CPPC_REG_IDX(reg_name) is
> >
> > #define CPPC_REG_IDX(reg_name)    CPPC_REG_##reg_name_IDX
> >
> > and there are CPPC_REG_##reg_name_IDX macros defined for all register
> > names in use?
> >
> > For example
> >
> > #define CPPC_REG_desired_perf_IDX   DESIRED_PERF
>
> What about keeping these two macros but replace reg_idx with
> CPPC_REG_IDX(reg_name)? With this, the only needed parameter for these two
> macros is reg_name.

The problem is that looking up functions defined through macros is
hard when somebody wants to know what they do, so I'd prefer to avoid
doing that.

Re: [PATCH v4 3/6] ACPI: CPPC: Add macros to generally implement registers getting and setting functions

[zhenglifeng (A)]

On 2025/1/15 19:12, Rafael J. Wysocki wrote:

> On Wed, Jan 15, 2025 at 9:59 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote:
>>
>> On 2025/1/15 1:58, Rafael J. Wysocki wrote:
>>
>>> On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>>>
>>>> Add CPPC_REG_VAL_READ() to implement registers getting functions.
>>>>
>>>> Add CPPC_REG_VAL_WRITE() to implement registers setting functions.
>>>>
>>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>>>
>>> I don't particularly like these macros as they will generally make it
>>> harder to follow the code.
>>>
>>>> ---
>>>>  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 571f94855dce..6326a1536cda 100644
>>>> --- a/drivers/acpi/cppc_acpi.c
>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>> @@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val)
>>>>         return cpc_write(cpu, reg, val);
>>>>  }
>>>>
>>>> +#define CPPC_REG_VAL_READ(reg_name, reg_idx)           \
>>>> +int cppc_get_##reg_name(int cpu, u64 *val)             \
>>>> +{                                                      \
>>>> +       return cppc_get_reg_val(cpu, reg_idx, val);     \
>>>> +}                                                      \
>>>> +EXPORT_SYMBOL_GPL(cppc_get_##reg_name)
>>>
>>> What about if defining something like
>>>
>>> #define CPPC_READ_REG_VAL(cpu, reg_name, val)
>>> cppc_get_reg_val((cpu), CPPC_REG_IDX(reg_name), (val))
>>>
>>> (and analogously for the WRITE_ part), where CPPC_REG_IDX(reg_name) is
>>>
>>> #define CPPC_REG_IDX(reg_name)    CPPC_REG_##reg_name_IDX
>>>
>>> and there are CPPC_REG_##reg_name_IDX macros defined for all register
>>> names in use?
>>>
>>> For example
>>>
>>> #define CPPC_REG_desired_perf_IDX   DESIRED_PERF
>>
>> What about keeping these two macros but replace reg_idx with
>> CPPC_REG_IDX(reg_name)? With this, the only needed parameter for these two
>> macros is reg_name.
> 
> The problem is that looking up functions defined through macros is
> hard when somebody wants to know what they do, so I'd prefer to avoid
> doing that.

I see your point. Let's just remove these.

> 
> 

[PATCH v4 4/6] 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 | 165 +++------------------------------------
 1 file changed, 11 insertions(+), 154 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 6326a1536cda..03134613311d 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1293,56 +1293,10 @@ int cppc_set_##reg_name(int cpu, u64 val)		\
 }							\
 EXPORT_SYMBOL_GPL(cppc_set_##reg_name)
 
-/**
- * cppc_get_desired_perf - Get the desired performance register value.
- * @cpunum: CPU from which to get desired performance.
- * @desired_perf: Return address.
- *
- * Return: 0 for success, -EIO otherwise.
- */
-int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
-{
-	return cppc_get_reg_val(cpunum, DESIRED_PERF, desired_perf);
-}
-EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
-
-/**
- * cppc_get_nominal_perf - Get the nominal performance register value.
- * @cpunum: CPU from which to get nominal performance.
- * @nominal_perf: Return address.
- *
- * Return: 0 for success, -EIO otherwise.
- */
-int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
-{
-	return cppc_get_reg_val(cpunum, NOMINAL_PERF, nominal_perf);
-}
-
-/**
- * cppc_get_highest_perf - Get the highest performance register value.
- * @cpunum: CPU from which to get highest performance.
- * @highest_perf: Return address.
- *
- * Return: 0 for success, -EIO otherwise.
- */
-int cppc_get_highest_perf(int cpunum, u64 *highest_perf)
-{
-	return cppc_get_reg_val(cpunum, HIGHEST_PERF, highest_perf);
-}
-EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
-
-/**
- * cppc_get_epp_perf - Get the epp register value.
- * @cpunum: CPU from which to get epp preference value.
- * @epp_perf: Return address.
- *
- * Return: 0 for success, -EIO otherwise.
- */
-int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
-{
-	return cppc_get_reg_val(cpunum, ENERGY_PERF, epp_perf);
-}
-EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
+CPPC_REG_VAL_READ(desired_perf, DESIRED_PERF);
+CPPC_REG_VAL_READ(nominal_perf, NOMINAL_PERF);
+CPPC_REG_VAL_READ(highest_perf, HIGHEST_PERF);
+CPPC_REG_VAL_READ(epp_perf, ENERGY_PERF);
 
 /**
  * cppc_get_perf_caps - Get a CPU's performance capabilities.
@@ -1620,44 +1574,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);
@@ -1669,43 +1593,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);
 
@@ -1719,38 +1607,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 v4 5/6] ACPI: CPPC: Add autonomous selection ABIs

[Lifeng Zheng]

cppc_set_epp - write energy performance preference register value

cppc_get_auto_act_window - read autonomous activity window register value

cppc_set_auto_act_window - write autonomous activity window register value

cppc_get_auto_sel - read autonomous selection enable register value,
modified from cppc_get_auto_sel_caps()

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 03134613311d..7bfe30f7b40f 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1568,23 +1568,89 @@ 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_set_epp() - Write the EPP register.
+ * @cpu: CPU on which to write register.
+ * @epp_val: Value to write to the EPP register.
  */
-int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+int cppc_set_epp(int cpu, u64 epp_val)
 {
-	u64 auto_sel;
+	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.
+ */
+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.
+ */
+int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
+{
+	u64 max_val = CPPC_AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, CPPC_AUTO_ACT_WINDOW_MAX_EXP);
+	int digits = 0;
+	u64 val;
+
+	if (auto_act_window > max_val)
+		return -EINVAL;
+
+	while (auto_act_window > CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH) {
+		auto_act_window /= 10;
+		digits += 1;
+	}
+
+	if (auto_act_window > CPPC_AUTO_ACT_WINDOW_MAX_SIG)
+		auto_act_window = CPPC_AUTO_ACT_WINDOW_MAX_SIG;
+
+	val = (digits << 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.
+ * @enable: Return address.
+ */
+int cppc_get_auto_sel(int cpu, bool *enable)
+{
+	u64 val;
 	int ret;
 
-	ret = cppc_get_reg_val(cpunum, AUTO_SEL_ENABLE, &auto_sel);
+	ret = cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, &val);
 	if (ret)
 		return ret;
 
-	perf_caps->auto_sel = (bool)auto_sel;
+	*enable = (bool)val;
+
 	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 66e5dfc711c0..8bc11d0618f8 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -399,6 +399,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)
@@ -420,7 +421,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..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,7 +168,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_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
+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);
 extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
@@ -229,11 +241,23 @@ 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_set_epp(int cpu, u64 epp_val)
 {
 	return -EOPNOTSUPP;
 }
-static inline int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+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;
+}
+static inline int cppc_set_auto_sel(int cpu, bool enable)
 {
 	return -EOPNOTSUPP;
 }
-- 
2.33.0

Re: [PATCH v4 5/6] ACPI: CPPC: Add autonomous selection ABIs

[Rafael J. Wysocki]

On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:

This should mention the specification revision and section number(s)
for the specification material the code is based on.

> cppc_set_epp - write energy performance preference register value
>
> cppc_get_auto_act_window - read autonomous activity window register value
>
> cppc_set_auto_act_window - write autonomous activity window register value
>
> cppc_get_auto_sel - read autonomous selection enable register value,
> modified from cppc_get_auto_sel_caps()

It would be better to move the modification part into a separate patch.

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/acpi/cppc_acpi.c     | 82 ++++++++++++++++++++++++++++++++----
>  drivers/cpufreq/amd-pstate.c |  3 +-
>  include/acpi/cppc_acpi.h     | 30 +++++++++++--
>  3 files changed, 103 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 03134613311d..7bfe30f7b40f 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1568,23 +1568,89 @@ 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_set_epp() - Write the EPP register.
> + * @cpu: CPU on which to write register.
> + * @epp_val: Value to write to the EPP register.
>   */
> -int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> +int cppc_set_epp(int cpu, u64 epp_val)
>  {
> -       u64 auto_sel;
> +       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.

It would be good to describe the autonomous activity window encoding.

> + */
> +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.
> + */
> +int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
> +{
> +       u64 max_val = CPPC_AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, CPPC_AUTO_ACT_WINDOW_MAX_EXP);
> +       int digits = 0;
> +       u64 val;
> +
> +       if (auto_act_window > max_val)
> +               return -EINVAL;
> +
> +       while (auto_act_window > CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH) {
> +               auto_act_window /= 10;
> +               digits += 1;
> +       }
> +
> +       if (auto_act_window > CPPC_AUTO_ACT_WINDOW_MAX_SIG)
> +               auto_act_window = CPPC_AUTO_ACT_WINDOW_MAX_SIG;

It looks like this may clobber the most significant bit, or am I mistaken?

> +
> +       val = (digits << 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.
> + * @enable: Return address.
> + */
> +int cppc_get_auto_sel(int cpu, bool *enable)
> +{
> +       u64 val;
>         int ret;
>
> -       ret = cppc_get_reg_val(cpunum, AUTO_SEL_ENABLE, &auto_sel);
> +       ret = cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, &val);
>         if (ret)
>                 return ret;
>
> -       perf_caps->auto_sel = (bool)auto_sel;
> +       *enable = (bool)val;
> +
>         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 66e5dfc711c0..8bc11d0618f8 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -399,6 +399,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)
> @@ -420,7 +421,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..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,7 +168,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_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
> +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);
>  extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
> @@ -229,11 +241,23 @@ 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_set_epp(int cpu, u64 epp_val)
>  {
>         return -EOPNOTSUPP;
>  }
> -static inline int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> +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;
> +}
> +static inline int cppc_set_auto_sel(int cpu, bool enable)
>  {
>         return -EOPNOTSUPP;
>  }
> --
> 2.33.0
>
>

Re: [PATCH v4 5/6] ACPI: CPPC: Add autonomous selection ABIs

[zhenglifeng (A)]

On 2025/1/15 2:24, Rafael J. Wysocki wrote:

> On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
> 
> This should mention the specification revision and section number(s)
> for the specification material the code is based on.

Will mention it. Thanks.

> 
>> cppc_set_epp - write energy performance preference register value
>>
>> cppc_get_auto_act_window - read autonomous activity window register value
>>
>> cppc_set_auto_act_window - write autonomous activity window register value
>>
>> cppc_get_auto_sel - read autonomous selection enable register value,
>> modified from cppc_get_auto_sel_caps()
> 
> It would be better to move the modification part into a separate patch.

Yes, good point. Thanks.

> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/acpi/cppc_acpi.c     | 82 ++++++++++++++++++++++++++++++++----
>>  drivers/cpufreq/amd-pstate.c |  3 +-
>>  include/acpi/cppc_acpi.h     | 30 +++++++++++--
>>  3 files changed, 103 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 03134613311d..7bfe30f7b40f 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1568,23 +1568,89 @@ 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_set_epp() - Write the EPP register.
>> + * @cpu: CPU on which to write register.
>> + * @epp_val: Value to write to the EPP register.
>>   */
>> -int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>> +int cppc_set_epp(int cpu, u64 epp_val)
>>  {
>> -       u64 auto_sel;
>> +       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.
> 
> It would be good to describe the autonomous activity window encoding.

Will add. Thanks.

> 
>> + */
>> +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.
>> + */
>> +int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
>> +{
>> +       u64 max_val = CPPC_AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, CPPC_AUTO_ACT_WINDOW_MAX_EXP);
>> +       int digits = 0;
>> +       u64 val;
>> +
>> +       if (auto_act_window > max_val)
>> +               return -EINVAL;
>> +
>> +       while (auto_act_window > CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH) {
>> +               auto_act_window /= 10;
>> +               digits += 1;
>> +       }
>> +
>> +       if (auto_act_window > CPPC_AUTO_ACT_WINDOW_MAX_SIG)
>> +               auto_act_window = CPPC_AUTO_ACT_WINDOW_MAX_SIG;
> 
> It looks like this may clobber the most significant bit, or am I mistaken?

Actually, after the while loop above, auto_act_window is not larger than
CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH, which is 129. So this if condition
is valid only when auto_act_window is 128 or 129. Since we only have 7 bits
to store this value, 128 and 129 can only be cut to 127.

> 
>> +
>> +       val = (digits << 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.
>> + * @enable: Return address.
>> + */
>> +int cppc_get_auto_sel(int cpu, bool *enable)
>> +{
>> +       u64 val;
>>         int ret;
>>
>> -       ret = cppc_get_reg_val(cpunum, AUTO_SEL_ENABLE, &auto_sel);
>> +       ret = cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, &val);
>>         if (ret)
>>                 return ret;
>>
>> -       perf_caps->auto_sel = (bool)auto_sel;
>> +       *enable = (bool)val;
>> +
>>         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 66e5dfc711c0..8bc11d0618f8 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -399,6 +399,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)
>> @@ -420,7 +421,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..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,7 +168,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_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
>> +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);
>>  extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>> @@ -229,11 +241,23 @@ 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_set_epp(int cpu, u64 epp_val)
>>  {
>>         return -EOPNOTSUPP;
>>  }
>> -static inline int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>> +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;
>> +}
>> +static inline int cppc_set_auto_sel(int cpu, bool enable)
>>  {
>>         return -EOPNOTSUPP;
>>  }
>> --
>> 2.33.0
>>
>>
> 

[PATCH v4 6/6] 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..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 bd8f75accfa0..ea6c6a5bbd8c 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -814,10 +814,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_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)
+{
+	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_perf);
 
 static struct freq_attr *cppc_cpufreq_attr[] = {
 	&freqdomain_cpus,
+	&auto_select,
+	&auto_act_window,
+	&energy_perf,
 	NULL,
 };
 
-- 
2.33.0

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Gautham R. Shenoy]

Hello Lifeng,


On Mon, Jan 13, 2025 at 08:21:04PM +0800, Lifeng Zheng wrote:
> Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq
> driver.
> 

[..snip..]

> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bd8f75accfa0..ea6c6a5bbd8c 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -814,10 +814,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);

When the auto_select register is not supported, since
cppc_set_reg_val() doesn't have the !CPC_SUPPORTED(reg) check, that
function won't return an error, and thus this store function won't
return an error either. Should there be a !CPC_SUPPORTED(reg) check in
cppc_set_reg_val() as well? Or should the store function call
cppc_get_auto_sel() to figure out if the register is supported or not?

--
Thanks and Regards
gautham.

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

On 2025/1/15 22:51, Gautham R. Shenoy wrote:

> Hello Lifeng,
> 
> 
> On Mon, Jan 13, 2025 at 08:21:04PM +0800, Lifeng Zheng wrote:
>> Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq
>> driver.
>>
> 
> [..snip..]
> 
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index bd8f75accfa0..ea6c6a5bbd8c 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -814,10 +814,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);
> 
> When the auto_select register is not supported, since
> cppc_set_reg_val() doesn't have the !CPC_SUPPORTED(reg) check, that
> function won't return an error, and thus this store function won't
> return an error either. Should there be a !CPC_SUPPORTED(reg) check in
> cppc_set_reg_val() as well? Or should the store function call
> cppc_get_auto_sel() to figure out if the register is supported or not?

In patch 2, I have this check in cppc_set_reg_val():

+	/* 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 a register is not a cpc supported one, it must be either an integer type
or a null one. So it won't pass this check I think.

> 
> --
> Thanks and Regards
> gautham.

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Gautham R. Shenoy]

On Thu, Jan 16, 2025 at 09:26:37AM +0800, zhenglifeng (A) wrote:
> On 2025/1/15 22:51, Gautham R. Shenoy wrote:
> 
> > Hello Lifeng,
> > 
> > 
> > On Mon, Jan 13, 2025 at 08:21:04PM +0800, Lifeng Zheng wrote:
> >> Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq
> >> driver.
> >>
> > 
> > [..snip..]
> > 
> >> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> >> index bd8f75accfa0..ea6c6a5bbd8c 100644
> >> --- a/drivers/cpufreq/cppc_cpufreq.c
> >> +++ b/drivers/cpufreq/cppc_cpufreq.c
> >> @@ -814,10 +814,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);
> > 
> > When the auto_select register is not supported, since
> > cppc_set_reg_val() doesn't have the !CPC_SUPPORTED(reg) check, that
> > function won't return an error, and thus this store function won't
> > return an error either. Should there be a !CPC_SUPPORTED(reg) check in
> > cppc_set_reg_val() as well? Or should the store function call
> > cppc_get_auto_sel() to figure out if the register is supported or not?
> 
> In patch 2, I have this check in cppc_set_reg_val():
> 
> +	/* 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 a register is not a cpc supported one, it must be either an integer type
> or a null one. So it won't pass this check I think.

Ah, I see. In that case, you can remove the cppc_get_auto_sel() in
shmem_init_perf() function in amd_pstate.c (in Patch 5/6) from the
following snippet. The auto_sel value is nowhere used in the rest of
the code.

@@ -399,6 +399,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
 {
 	struct cppc_perf_caps cppc_perf;
 	u64 numerator;
+	bool auto_sel; <--- Not needed.
 
 	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
 	if (ret)
@@ -420,7 +421,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);   <--- Not needed.
+	ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel);         <--- Not needed.
 	if (ret) {                                                <--- Not needed.
 		pr_warn("failed to get auto_sel, ret: %d\n", ret); <--- Not needed.


--
Thanks and Regards
gautham.

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

On 2025/1/16 14:13, Gautham R. Shenoy wrote:

> On Thu, Jan 16, 2025 at 09:26:37AM +0800, zhenglifeng (A) wrote:
>> On 2025/1/15 22:51, Gautham R. Shenoy wrote:
>>
>>> Hello Lifeng,
>>>
>>>
>>> On Mon, Jan 13, 2025 at 08:21:04PM +0800, Lifeng Zheng wrote:
>>>> Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq
>>>> driver.
>>>>
>>>
>>> [..snip..]
>>>
>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>>> index bd8f75accfa0..ea6c6a5bbd8c 100644
>>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>>> @@ -814,10 +814,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);
>>>
>>> When the auto_select register is not supported, since
>>> cppc_set_reg_val() doesn't have the !CPC_SUPPORTED(reg) check, that
>>> function won't return an error, and thus this store function won't
>>> return an error either. Should there be a !CPC_SUPPORTED(reg) check in
>>> cppc_set_reg_val() as well? Or should the store function call
>>> cppc_get_auto_sel() to figure out if the register is supported or not?
>>
>> In patch 2, I have this check in cppc_set_reg_val():
>>
>> +	/* 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 a register is not a cpc supported one, it must be either an integer type
>> or a null one. So it won't pass this check I think.
> 
> Ah, I see. In that case, you can remove the cppc_get_auto_sel() in
> shmem_init_perf() function in amd_pstate.c (in Patch 5/6) from the
> following snippet. The auto_sel value is nowhere used in the rest of
> the code.
> 
> @@ -399,6 +399,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
>  {
>  	struct cppc_perf_caps cppc_perf;
>  	u64 numerator;
> +	bool auto_sel; <--- Not needed.
>  
>  	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
>  	if (ret)
> @@ -420,7 +421,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);   <--- Not needed.
> +	ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel);         <--- Not needed.
>  	if (ret) {                                                <--- Not needed.
>  		pr_warn("failed to get auto_sel, ret: %d\n", ret); <--- Not needed.
> 

If auto_sel is not supported, this function will return 0 after getting
fail. But after removing cppc_get_auto_sel(), this function will return
-EOPNOTSUPP by setting. Is this alright?

> 
> --
> Thanks and Regards
> gautham.

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Gautham R. Shenoy]

On Thu, Jan 16, 2025 at 04:01:08PM +0800, zhenglifeng (A) wrote:

> > @@ -399,6 +399,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
> >  {
> >  	struct cppc_perf_caps cppc_perf;
> >  	u64 numerator;
> > +	bool auto_sel; <--- Not needed.
> >  
> >  	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> >  	if (ret)
> > @@ -420,7 +421,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);   <--- Not needed.
> > +	ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel);         <--- Not needed.
> >  	if (ret) {                                                <--- Not needed.
> >  		pr_warn("failed to get auto_sel, ret: %d\n", ret); <--- Not needed.
> > 
> 
> If auto_sel is not supported, this function will return 0 after getting
> fail. But after removing cppc_get_auto_sel(), this function will return
> -EOPNOTSUPP by setting. Is this alright?

This is not ok. The shmem_init_perf() function shouldn't error out if
the auto-selection register is not supported.

Also, looking at the function, there may be a few additional cleanups
required in that one. For now, let us just retain the
cppc_get_auto_sel() and cppc_set_auto_sel() functions as you have done
in this patchset.

--
Thanks and Regards
gautham.

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Russell Haley]

Hello,

I noticed something here just as a user casually browsing the mailing list.

On 1/13/25 6:21 AM, Lifeng Zheng wrote:
> 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..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.
>  

[...snip...]

> +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.

In intel_pstate driver, there is file with near-identical semantics:

/sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference

It also accepts a few string arguments and converts them to integers.

Perhaps the same name should be used, and the semantics made exactly
identical, and then it could be documented as present for either
cppc_cpufreq OR intel_pstate?

I think would be more elegant if userspace tooling could Just Work with
either driver.

One might object that the frequency selection behavior that results from
any particular value of the register itself might be different, but they
are *already* different between Intel's P and E-cores in the same CPU
package. (Ugh.)

--
Thanks,
Russell

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

On 2025/1/16 19:39, Russell Haley wrote:

> Hello,
> 
> I noticed something here just as a user casually browsing the mailing list.
> 
> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>> 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..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.
>>  
> 
> [...snip...]
> 
>> +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.
> 
> In intel_pstate driver, there is file with near-identical semantics:
> 
> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
> 
> It also accepts a few string arguments and converts them to integers.
> 
> Perhaps the same name should be used, and the semantics made exactly
> identical, and then it could be documented as present for either
> cppc_cpufreq OR intel_pstate?
> 
> I think would be more elegant if userspace tooling could Just Work with
> either driver.
> 
> One might object that the frequency selection behavior that results from
> any particular value of the register itself might be different, but they
> are *already* different between Intel's P and E-cores in the same CPU
> package. (Ugh.)

Yes, I should use the same name. Thanks.

As for accepting string arguments and converting them to integers, I don't
think it is necessary. It'll be a litte confused if someone writes a raw
value and reads a string I think. I prefer to let users freely set this
value.

In addition, there are many differences between the implementations of
energy_performance_preference in intel_pstate and cppc_cpufreq (and
amd-pstate...). It is really difficult to explain all this differences in
this document. So I'll leave it to be documented as present for
cppc_cpufreq only.

> 
> --
> Thanks,
> Russell
> 
> 
> 

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Mario Limonciello]

On 1/16/2025 21:11, zhenglifeng (A) wrote:
> On 2025/1/16 19:39, Russell Haley wrote:
> 
>> Hello,
>>
>> I noticed something here just as a user casually browsing the mailing list.
>>
>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>> 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..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.
>>>   
>>
>> [...snip...]
>>
>>> +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.
>>
>> In intel_pstate driver, there is file with near-identical semantics:
>>
>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>
>> It also accepts a few string arguments and converts them to integers.
>>
>> Perhaps the same name should be used, and the semantics made exactly
>> identical, and then it could be documented as present for either
>> cppc_cpufreq OR intel_pstate?
>>
>> I think would be more elegant if userspace tooling could Just Work with
>> either driver.
>>
>> One might object that the frequency selection behavior that results from
>> any particular value of the register itself might be different, but they
>> are *already* different between Intel's P and E-cores in the same CPU
>> package. (Ugh.)
> 
> Yes, I should use the same name. Thanks.
> 
> As for accepting string arguments and converting them to integers, I don't
> think it is necessary. It'll be a litte confused if someone writes a raw
> value and reads a string I think. I prefer to let users freely set this
> value.
> 
> In addition, there are many differences between the implementations of
> energy_performance_preference in intel_pstate and cppc_cpufreq (and
> amd-pstate...). It is really difficult to explain all this differences in
> this document. So I'll leave it to be documented as present for
> cppc_cpufreq only.

At least the interface to userspace I think we should do the best we can 
to be the same between all the drivers if possible.

For example; I've got a patch that I may bring up in a future kernel 
cycle that adds raw integer writes to amd-pstates 
energy_performance_profile to behave the same way intel-pstate does.

> 
>>
>> --
>> Thanks,
>> Russell
>>
>>
>>
> 

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

On 2025/1/17 22:30, Mario Limonciello wrote:

> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>> On 2025/1/16 19:39, Russell Haley wrote:
>>
>>> Hello,
>>>
>>> I noticed something here just as a user casually browsing the mailing list.
>>>
>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>> 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..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.
>>>>   
>>>
>>> [...snip...]
>>>
>>>> +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.
>>>
>>> In intel_pstate driver, there is file with near-identical semantics:
>>>
>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>>
>>> It also accepts a few string arguments and converts them to integers.
>>>
>>> Perhaps the same name should be used, and the semantics made exactly
>>> identical, and then it could be documented as present for either
>>> cppc_cpufreq OR intel_pstate?
>>>
>>> I think would be more elegant if userspace tooling could Just Work with
>>> either driver.
>>>
>>> One might object that the frequency selection behavior that results from
>>> any particular value of the register itself might be different, but they
>>> are *already* different between Intel's P and E-cores in the same CPU
>>> package. (Ugh.)
>>
>> Yes, I should use the same name. Thanks.
>>
>> As for accepting string arguments and converting them to integers, I don't
>> think it is necessary. It'll be a litte confused if someone writes a raw
>> value and reads a string I think. I prefer to let users freely set this
>> value.
>>
>> In addition, there are many differences between the implementations of
>> energy_performance_preference in intel_pstate and cppc_cpufreq (and
>> amd-pstate...). It is really difficult to explain all this differences in
>> this document. So I'll leave it to be documented as present for
>> cppc_cpufreq only.
> 
> At least the interface to userspace I think we should do the best we can to be the same between all the drivers if possible.
> 
> For example; I've got a patch that I may bring up in a future kernel cycle that adds raw integer writes to amd-pstates energy_performance_profile to behave the same way intel-pstate does.

I agree that it's better to keep this interface consistent across different
drivers. But in my opinion, the implementation of intel_pstate
energy_performance_preference is not really nice. Someone may write a raw
value but read a string, or read strings for some values and read raw
values for some other values. It is inconsistent. It may be better to use
some other implementation, such as seperating the operations of r/w strings
and raw values into two files.

I think it's better to consult Rafael and Viresh about how this should
evolve.

> 
>>
>>>
>>> -- 
>>> Thanks,
>>> Russell
>>>
>>>
>>>
>>
> 

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Pierre Gondois]

On 1/20/25 04:15, zhenglifeng (A) wrote:
> On 2025/1/17 22:30, Mario Limonciello wrote:
> 
>> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>>> On 2025/1/16 19:39, Russell Haley wrote:
>>>
>>>> Hello,
>>>>
>>>> I noticed something here just as a user casually browsing the mailing list.
>>>>
>>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>>> 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..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.
>>>>>    
>>>>
>>>> [...snip...]
>>>>
>>>>> +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.
>>>>
>>>> In intel_pstate driver, there is file with near-identical semantics:
>>>>
>>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>>>
>>>> It also accepts a few string arguments and converts them to integers.
>>>>
>>>> Perhaps the same name should be used, and the semantics made exactly
>>>> identical, and then it could be documented as present for either
>>>> cppc_cpufreq OR intel_pstate?
>>>>
>>>> I think would be more elegant if userspace tooling could Just Work with
>>>> either driver.
>>>>
>>>> One might object that the frequency selection behavior that results from
>>>> any particular value of the register itself might be different, but they
>>>> are *already* different between Intel's P and E-cores in the same CPU
>>>> package. (Ugh.)
>>>
>>> Yes, I should use the same name. Thanks.
>>>
>>> As for accepting string arguments and converting them to integers, I don't
>>> think it is necessary. It'll be a litte confused if someone writes a raw
>>> value and reads a string I think. I prefer to let users freely set this
>>> value.
>>>
>>> In addition, there are many differences between the implementations of
>>> energy_performance_preference in intel_pstate and cppc_cpufreq (and
>>> amd-pstate...). It is really difficult to explain all this differences in
>>> this document. So I'll leave it to be documented as present for
>>> cppc_cpufreq only.
>>
>> At least the interface to userspace I think we should do the best we can to be the same between all the drivers if possible.
>>
>> For example; I've got a patch that I may bring up in a future kernel cycle that adds raw integer writes to amd-pstates energy_performance_profile to behave the same way intel-pstate does.
> 
> I agree that it's better to keep this interface consistent across different
> drivers. But in my opinion, the implementation of intel_pstate
> energy_performance_preference is not really nice. Someone may write a raw
> value but read a string, or read strings for some values and read raw
> values for some other values. It is inconsistent. It may be better to use
> some other implementation, such as seperating the operations of r/w strings
> and raw values into two files.

I agree it would be better to be sure of the type to expect when reading the
energy_performance_preference file. The epp values in the range 0-255 with 0
being the performance value for all interfaces.

In the current epp strings, it seems there is a big gap between the PERFORMANCE
and the BALANCE_PERFORMANCE strings. Maybe it would be good to complete it:
EPP_PERFORMANCE		0x00
EPP_BALANCE_PERFORMANCE	0x40      // state value changed
EPP_BALANCE		0x80      // new state
EPP_BALANCE_POWERSAVE	0xC0
EPP_POWERSAVE		0xFF

NIT: The mapping seems to be slightly different for intel_pstate and amd-pstate
currently:
drivers/cpufreq/amd-pstate.c
#define AMD_CPPC_EPP_PERFORMANCE		0x00
#define AMD_CPPC_EPP_BALANCE_PERFORMANCE	0x80
#define AMD_CPPC_EPP_BALANCE_POWERSAVE		0xBF
#define AMD_CPPC_EPP_POWERSAVE			0xFF

arch/x86/include/asm/msr-index.h
#define HWP_EPP_PERFORMANCE		0x00
#define HWP_EPP_BALANCE_PERFORMANCE	0x80
#define HWP_EPP_BALANCE_POWERSAVE	0xC0   <------ Different from AMD_CPPC_EPP_BALANCE_POWERSAVE
#define HWP_EPP_POWERSAVE		0xFF

> 
> I think it's better to consult Rafael and Viresh about how this should
> evolve.

Yes indeed

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Mario Limonciello]

On 1/20/2025 08:49, Pierre Gondois wrote:
> 
> 
> On 1/20/25 04:15, zhenglifeng (A) wrote:
>> On 2025/1/17 22:30, Mario Limonciello wrote:
>>
>>> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>>>> On 2025/1/16 19:39, Russell Haley wrote:
>>>>
>>>>> Hello,
>>>>>
>>>>> I noticed something here just as a user casually browsing the 
>>>>> mailing list.
>>>>>
>>>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>>>> 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..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.
>>>>>
>>>>> [...snip...]
>>>>>
>>>>>> +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.
>>>>>
>>>>> In intel_pstate driver, there is file with near-identical semantics:
>>>>>
>>>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>>>>
>>>>> It also accepts a few string arguments and converts them to integers.
>>>>>
>>>>> Perhaps the same name should be used, and the semantics made exactly
>>>>> identical, and then it could be documented as present for either
>>>>> cppc_cpufreq OR intel_pstate?
>>>>>
>>>>> I think would be more elegant if userspace tooling could Just Work 
>>>>> with
>>>>> either driver.
>>>>>
>>>>> One might object that the frequency selection behavior that results 
>>>>> from
>>>>> any particular value of the register itself might be different, but 
>>>>> they
>>>>> are *already* different between Intel's P and E-cores in the same CPU
>>>>> package. (Ugh.)
>>>>
>>>> Yes, I should use the same name. Thanks.
>>>>
>>>> As for accepting string arguments and converting them to integers, I 
>>>> don't
>>>> think it is necessary. It'll be a litte confused if someone writes a 
>>>> raw
>>>> value and reads a string I think. I prefer to let users freely set this
>>>> value.
>>>>
>>>> In addition, there are many differences between the implementations of
>>>> energy_performance_preference in intel_pstate and cppc_cpufreq (and
>>>> amd-pstate...). It is really difficult to explain all this 
>>>> differences in
>>>> this document. So I'll leave it to be documented as present for
>>>> cppc_cpufreq only.
>>>
>>> At least the interface to userspace I think we should do the best we 
>>> can to be the same between all the drivers if possible.
>>>
>>> For example; I've got a patch that I may bring up in a future kernel 
>>> cycle that adds raw integer writes to amd-pstates 
>>> energy_performance_profile to behave the same way intel-pstate does.
>>
>> I agree that it's better to keep this interface consistent across 
>> different
>> drivers. But in my opinion, the implementation of intel_pstate
>> energy_performance_preference is not really nice. Someone may write a raw
>> value but read a string, or read strings for some values and read raw
>> values for some other values. It is inconsistent. It may be better to use
>> some other implementation, such as seperating the operations of r/w 
>> strings
>> and raw values into two files.
> 
> I agree it would be better to be sure of the type to expect when reading 
> the
> energy_performance_preference file. The epp values in the range 0-255 
> with 0
> being the performance value for all interfaces.
> 
> In the current epp strings, it seems there is a big gap between the 
> PERFORMANCE
> and the BALANCE_PERFORMANCE strings. Maybe it would be good to complete it:
> EPP_PERFORMANCE        0x00
> EPP_BALANCE_PERFORMANCE    0x40      // state value changed
> EPP_BALANCE        0x80      // new state
> EPP_BALANCE_POWERSAVE    0xC0
> EPP_POWERSAVE        0xFF
> 
> NIT: The mapping seems to be slightly different for intel_pstate and 
> amd-pstate
> currently:
> drivers/cpufreq/amd-pstate.c
> #define AMD_CPPC_EPP_PERFORMANCE        0x00
> #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
> #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
> #define AMD_CPPC_EPP_POWERSAVE            0xFF
> 
> arch/x86/include/asm/msr-index.h
> #define HWP_EPP_PERFORMANCE        0x00
> #define HWP_EPP_BALANCE_PERFORMANCE    0x80
> #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------ Different from 
> AMD_CPPC_EPP_BALANCE_POWERSAVE
> #define HWP_EPP_POWERSAVE        0xFF
> 
>>
>> I think it's better to consult Rafael and Viresh about how this should
>> evolve.
> 
> Yes indeed

Maybe it's best to discuss what the goal of raw EPP number writes is to 
decide what to do with it.

IE in intel-pstate is it for userspace to be able to actually utilize 
something besides the strings all the time?  Or is it just for debugging 
to find better values for strings in the future?

If the former maybe we're better off splitting to 
'energy_performance_preference' and 'energy_performance_preference_int'.

If the latter maybe we're better off putting the integer writes and 
reads into debugfs instead and making 'energy_performance_preference' 
return -EINVAL while a non-predefined value is in use.

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

On 2025/1/21 1:44, Mario Limonciello wrote:

> On 1/20/2025 08:49, Pierre Gondois wrote:
>>
>>
>> On 1/20/25 04:15, zhenglifeng (A) wrote:
>>> On 2025/1/17 22:30, Mario Limonciello wrote:
>>>
>>>> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>>>>> On 2025/1/16 19:39, Russell Haley wrote:
>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> I noticed something here just as a user casually browsing the mailing list.
>>>>>>
>>>>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>>>>> 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..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.
>>>>>>
>>>>>> [...snip...]
>>>>>>
>>>>>>> +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.
>>>>>>
>>>>>> In intel_pstate driver, there is file with near-identical semantics:
>>>>>>
>>>>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>>>>>
>>>>>> It also accepts a few string arguments and converts them to integers.
>>>>>>
>>>>>> Perhaps the same name should be used, and the semantics made exactly
>>>>>> identical, and then it could be documented as present for either
>>>>>> cppc_cpufreq OR intel_pstate?
>>>>>>
>>>>>> I think would be more elegant if userspace tooling could Just Work with
>>>>>> either driver.
>>>>>>
>>>>>> One might object that the frequency selection behavior that results from
>>>>>> any particular value of the register itself might be different, but they
>>>>>> are *already* different between Intel's P and E-cores in the same CPU
>>>>>> package. (Ugh.)
>>>>>
>>>>> Yes, I should use the same name. Thanks.
>>>>>
>>>>> As for accepting string arguments and converting them to integers, I don't
>>>>> think it is necessary. It'll be a litte confused if someone writes a raw
>>>>> value and reads a string I think. I prefer to let users freely set this
>>>>> value.
>>>>>
>>>>> In addition, there are many differences between the implementations of
>>>>> energy_performance_preference in intel_pstate and cppc_cpufreq (and
>>>>> amd-pstate...). It is really difficult to explain all this differences in
>>>>> this document. So I'll leave it to be documented as present for
>>>>> cppc_cpufreq only.
>>>>
>>>> At least the interface to userspace I think we should do the best we can to be the same between all the drivers if possible.
>>>>
>>>> For example; I've got a patch that I may bring up in a future kernel cycle that adds raw integer writes to amd-pstates energy_performance_profile to behave the same way intel-pstate does.
>>>
>>> I agree that it's better to keep this interface consistent across different
>>> drivers. But in my opinion, the implementation of intel_pstate
>>> energy_performance_preference is not really nice. Someone may write a raw
>>> value but read a string, or read strings for some values and read raw
>>> values for some other values. It is inconsistent. It may be better to use
>>> some other implementation, such as seperating the operations of r/w strings
>>> and raw values into two files.
>>
>> I agree it would be better to be sure of the type to expect when reading the
>> energy_performance_preference file. The epp values in the range 0-255 with 0
>> being the performance value for all interfaces.
>>
>> In the current epp strings, it seems there is a big gap between the PERFORMANCE
>> and the BALANCE_PERFORMANCE strings. Maybe it would be good to complete it:
>> EPP_PERFORMANCE        0x00
>> EPP_BALANCE_PERFORMANCE    0x40      // state value changed
>> EPP_BALANCE        0x80      // new state
>> EPP_BALANCE_POWERSAVE    0xC0
>> EPP_POWERSAVE        0xFF
>>
>> NIT: The mapping seems to be slightly different for intel_pstate and amd-pstate
>> currently:
>> drivers/cpufreq/amd-pstate.c
>> #define AMD_CPPC_EPP_PERFORMANCE        0x00
>> #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
>> #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
>> #define AMD_CPPC_EPP_POWERSAVE            0xFF
>>
>> arch/x86/include/asm/msr-index.h
>> #define HWP_EPP_PERFORMANCE        0x00
>> #define HWP_EPP_BALANCE_PERFORMANCE    0x80
>> #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------ Different from AMD_CPPC_EPP_BALANCE_POWERSAVE
>> #define HWP_EPP_POWERSAVE        0xFF
>>
>>>
>>> I think it's better to consult Rafael and Viresh about how this should
>>> evolve.
>>
>> Yes indeed
> 
> Maybe it's best to discuss what the goal of raw EPP number writes is to decide what to do with it.
> 
> IE in intel-pstate is it for userspace to be able to actually utilize something besides the strings all the time?  Or is it just for debugging to find better values for strings in the future?
> 
> If the former maybe we're better off splitting to 'energy_performance_preference' and 'energy_performance_preference_int'.
> 
> If the latter maybe we're better off putting the integer writes and reads into debugfs instead and making 'energy_performance_preference' return -EINVAL while a non-predefined value is in use.

I think it's the former. 

I added the author of the patch that allows raw energy performance
preference value in intel_pstate to ask about what the goal is and if this
would be ok to do the modification mentioned above.

To see the patch from https://lore.kernel.org/all/20200626183401.1495090-3-srinivas.pandruvada@linux.intel.com/

Anyway, the purpose of this patch is to allow users write and read raw EPP
number. So maybe I can just rename the file to
'energy_performance_preference_int'?

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Srinivas Pandruvada]

On 1/20/25 18:42, zhenglifeng (A) wrote:
> On 2025/1/21 1:44, Mario Limonciello wrote:
>
>> On 1/20/2025 08:49, Pierre Gondois wrote:
>>>
>>> On 1/20/25 04:15, zhenglifeng (A) wrote:
>>>> On 2025/1/17 22:30, Mario Limonciello wrote:
>>>>
>>>>> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>>>>>> On 2025/1/16 19:39, Russell Haley wrote:
>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> I noticed something here just as a user casually browsing the mailing list.
>>>>>>>
>>>>>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>>>>>> 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..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.
>>>>>>> [...snip...]
>>>>>>>
>>>>>>>> +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.
>>>>>>> In intel_pstate driver, there is file with near-identical semantics:
>>>>>>>
>>>>>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>>>>>>
>>>>>>> It also accepts a few string arguments and converts them to integers.
>>>>>>>
>>>>>>> Perhaps the same name should be used, and the semantics made exactly
>>>>>>> identical, and then it could be documented as present for either
>>>>>>> cppc_cpufreq OR intel_pstate?
>>>>>>>
>>>>>>> I think would be more elegant if userspace tooling could Just Work with
>>>>>>> either driver.
>>>>>>>
>>>>>>> One might object that the frequency selection behavior that results from
>>>>>>> any particular value of the register itself might be different, but they
>>>>>>> are *already* different between Intel's P and E-cores in the same CPU
>>>>>>> package. (Ugh.)
>>>>>> Yes, I should use the same name. Thanks.
>>>>>>
>>>>>> As for accepting string arguments and converting them to integers, I don't
>>>>>> think it is necessary. It'll be a litte confused if someone writes a raw
>>>>>> value and reads a string I think. I prefer to let users freely set this
>>>>>> value.
>>>>>>
>>>>>> In addition, there are many differences between the implementations of
>>>>>> energy_performance_preference in intel_pstate and cppc_cpufreq (and
>>>>>> amd-pstate...). It is really difficult to explain all this differences in
>>>>>> this document. So I'll leave it to be documented as present for
>>>>>> cppc_cpufreq only.
>>>>> At least the interface to userspace I think we should do the best we can to be the same between all the drivers if possible.
>>>>>
>>>>> For example; I've got a patch that I may bring up in a future kernel cycle that adds raw integer writes to amd-pstates energy_performance_profile to behave the same way intel-pstate does.
>>>> I agree that it's better to keep this interface consistent across different
>>>> drivers. But in my opinion, the implementation of intel_pstate
>>>> energy_performance_preference is not really nice. Someone may write a raw
>>>> value but read a string, or read strings for some values and read raw
>>>> values for some other values. It is inconsistent. It may be better to use
>>>> some other implementation, such as seperating the operations of r/w strings
>>>> and raw values into two files.
>>> I agree it would be better to be sure of the type to expect when reading the
>>> energy_performance_preference file. The epp values in the range 0-255 with 0
>>> being the performance value for all interfaces.
>>>
>>> In the current epp strings, it seems there is a big gap between the PERFORMANCE
>>> and the BALANCE_PERFORMANCE strings. Maybe it would be good to complete it:
>>> EPP_PERFORMANCE        0x00
>>> EPP_BALANCE_PERFORMANCE    0x40      // state value changed
>>> EPP_BALANCE        0x80      // new state
>>> EPP_BALANCE_POWERSAVE    0xC0
>>> EPP_POWERSAVE        0xFF
>>>
>>> NIT: The mapping seems to be slightly different for intel_pstate and amd-pstate
>>> currently:
>>> drivers/cpufreq/amd-pstate.c
>>> #define AMD_CPPC_EPP_PERFORMANCE        0x00
>>> #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
>>> #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
>>> #define AMD_CPPC_EPP_POWERSAVE            0xFF
>>>
>>> arch/x86/include/asm/msr-index.h
>>> #define HWP_EPP_PERFORMANCE        0x00
>>> #define HWP_EPP_BALANCE_PERFORMANCE    0x80
>>> #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------ Different from AMD_CPPC_EPP_BALANCE_POWERSAVE
>>> #define HWP_EPP_POWERSAVE        0xFF
>>>
>>>> I think it's better to consult Rafael and Viresh about how this should
>>>> evolve.
>>> Yes indeed
>> Maybe it's best to discuss what the goal of raw EPP number writes is to decide what to do with it.
>>
>> IE in intel-pstate is it for userspace to be able to actually utilize something besides the strings all the time?  Or is it just for debugging to find better values for strings in the future?
>>
>> If the former maybe we're better off splitting to 'energy_performance_preference' and 'energy_performance_preference_int'.
>>
>> If the latter maybe we're better off putting the integer writes and reads into debugfs instead and making 'energy_performance_preference' return -EINVAL while a non-predefined value is in use.

In Intel case EPP values can be different based on processor. In some 
case they they end up sharing the same CPU model. So strings are not 
suitable for all cases. Also there is different preference of EPP 
between Chrome systems and non chrome distro. For example Chrome has 
some resource manager which can change and same on Intel distros with LPMD.


Thanks,

Srinivas


> I think it's the former.
>
> I added the author of the patch that allows raw energy performance
> preference value in intel_pstate to ask about what the goal is and if this
> would be ok to do the modification mentioned above.
>
> To see the patch from https://lore.kernel.org/all/20200626183401.1495090-3-srinivas.pandruvada@linux.intel.com/
>
> Anyway, the purpose of this patch is to allow users write and read raw EPP
> number. So maybe I can just rename the file to
> 'energy_performance_preference_int'?
>

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Mario Limonciello]

On 1/23/2025 10:46, Srinivas Pandruvada wrote:
> 
> On 1/20/25 18:42, zhenglifeng (A) wrote:
>> On 2025/1/21 1:44, Mario Limonciello wrote:
>>
>>> On 1/20/2025 08:49, Pierre Gondois wrote:
>>>>
>>>> On 1/20/25 04:15, zhenglifeng (A) wrote:
>>>>> On 2025/1/17 22:30, Mario Limonciello wrote:
>>>>>
>>>>>> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>>>>>>> On 2025/1/16 19:39, Russell Haley wrote:
>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> I noticed something here just as a user casually browsing the 
>>>>>>>> mailing list.
>>>>>>>>
>>>>>>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>>>>>>> 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..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.
>>>>>>>> [...snip...]
>>>>>>>>
>>>>>>>>> +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.
>>>>>>>> In intel_pstate driver, there is file with near-identical 
>>>>>>>> semantics:
>>>>>>>>
>>>>>>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>>>>>>>
>>>>>>>> It also accepts a few string arguments and converts them to 
>>>>>>>> integers.
>>>>>>>>
>>>>>>>> Perhaps the same name should be used, and the semantics made 
>>>>>>>> exactly
>>>>>>>> identical, and then it could be documented as present for either
>>>>>>>> cppc_cpufreq OR intel_pstate?
>>>>>>>>
>>>>>>>> I think would be more elegant if userspace tooling could Just 
>>>>>>>> Work with
>>>>>>>> either driver.
>>>>>>>>
>>>>>>>> One might object that the frequency selection behavior that 
>>>>>>>> results from
>>>>>>>> any particular value of the register itself might be different, 
>>>>>>>> but they
>>>>>>>> are *already* different between Intel's P and E-cores in the 
>>>>>>>> same CPU
>>>>>>>> package. (Ugh.)
>>>>>>> Yes, I should use the same name. Thanks.
>>>>>>>
>>>>>>> As for accepting string arguments and converting them to 
>>>>>>> integers, I don't
>>>>>>> think it is necessary. It'll be a litte confused if someone 
>>>>>>> writes a raw
>>>>>>> value and reads a string I think. I prefer to let users freely 
>>>>>>> set this
>>>>>>> value.
>>>>>>>
>>>>>>> In addition, there are many differences between the 
>>>>>>> implementations of
>>>>>>> energy_performance_preference in intel_pstate and cppc_cpufreq (and
>>>>>>> amd-pstate...). It is really difficult to explain all this 
>>>>>>> differences in
>>>>>>> this document. So I'll leave it to be documented as present for
>>>>>>> cppc_cpufreq only.
>>>>>> At least the interface to userspace I think we should do the best 
>>>>>> we can to be the same between all the drivers if possible.
>>>>>>
>>>>>> For example; I've got a patch that I may bring up in a future 
>>>>>> kernel cycle that adds raw integer writes to amd-pstates 
>>>>>> energy_performance_profile to behave the same way intel-pstate does.
>>>>> I agree that it's better to keep this interface consistent across 
>>>>> different
>>>>> drivers. But in my opinion, the implementation of intel_pstate
>>>>> energy_performance_preference is not really nice. Someone may write 
>>>>> a raw
>>>>> value but read a string, or read strings for some values and read raw
>>>>> values for some other values. It is inconsistent. It may be better 
>>>>> to use
>>>>> some other implementation, such as seperating the operations of r/w 
>>>>> strings
>>>>> and raw values into two files.
>>>> I agree it would be better to be sure of the type to expect when 
>>>> reading the
>>>> energy_performance_preference file. The epp values in the range 
>>>> 0-255 with 0
>>>> being the performance value for all interfaces.
>>>>
>>>> In the current epp strings, it seems there is a big gap between the 
>>>> PERFORMANCE
>>>> and the BALANCE_PERFORMANCE strings. Maybe it would be good to 
>>>> complete it:
>>>> EPP_PERFORMANCE        0x00
>>>> EPP_BALANCE_PERFORMANCE    0x40      // state value changed
>>>> EPP_BALANCE        0x80      // new state
>>>> EPP_BALANCE_POWERSAVE    0xC0
>>>> EPP_POWERSAVE        0xFF
>>>>
>>>> NIT: The mapping seems to be slightly different for intel_pstate and 
>>>> amd-pstate
>>>> currently:
>>>> drivers/cpufreq/amd-pstate.c
>>>> #define AMD_CPPC_EPP_PERFORMANCE        0x00
>>>> #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
>>>> #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
>>>> #define AMD_CPPC_EPP_POWERSAVE            0xFF
>>>>
>>>> arch/x86/include/asm/msr-index.h
>>>> #define HWP_EPP_PERFORMANCE        0x00
>>>> #define HWP_EPP_BALANCE_PERFORMANCE    0x80
>>>> #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------ Different from 
>>>> AMD_CPPC_EPP_BALANCE_POWERSAVE
>>>> #define HWP_EPP_POWERSAVE        0xFF
>>>>
>>>>> I think it's better to consult Rafael and Viresh about how this should
>>>>> evolve.
>>>> Yes indeed
>>> Maybe it's best to discuss what the goal of raw EPP number writes is 
>>> to decide what to do with it.
>>>
>>> IE in intel-pstate is it for userspace to be able to actually utilize 
>>> something besides the strings all the time?  Or is it just for 
>>> debugging to find better values for strings in the future?
>>>
>>> If the former maybe we're better off splitting to 
>>> 'energy_performance_preference' and 'energy_performance_preference_int'.
>>>
>>> If the latter maybe we're better off putting the integer writes and 
>>> reads into debugfs instead and making 'energy_performance_preference' 
>>> return -EINVAL while a non-predefined value is in use.
> 
> In Intel case EPP values can be different based on processor. In some 
> case they they end up sharing the same CPU model. So strings are not 
> suitable for all cases. Also there is different preference of EPP 
> between Chrome systems and non chrome distro. For example Chrome has 
> some resource manager which can change and same on Intel distros with LPMD.
> 

Thanks for confirming it is intentional and changing it would break 
existing userspace.

And FWIW even in Windows there are more than 4 situational values used 
like we have in Linux today.

As the status quo is there I personally feel that we should do the exact 
same for other implementation of energy_performance_preference.

> 
> Thanks,
> 
> Srinivas
> 
> 
>> I think it's the former.
>>
>> I added the author of the patch that allows raw energy performance
>> preference value in intel_pstate to ask about what the goal is and if 
>> this
>> would be ok to do the modification mentioned above.
>>
>> To see the patch from https://lore.kernel.org/ 
>> all/20200626183401.1495090-3-srinivas.pandruvada@linux.intel.com/
>>
>> Anyway, the purpose of this patch is to allow users write and read raw 
>> EPP
>> number. So maybe I can just rename the file to
>> 'energy_performance_preference_int'?
>>

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

On 2025/1/24 1:05, Mario Limonciello wrote:

> On 1/23/2025 10:46, Srinivas Pandruvada wrote:
>>
>> On 1/20/25 18:42, zhenglifeng (A) wrote:
>>> On 2025/1/21 1:44, Mario Limonciello wrote:
>>>
>>>> On 1/20/2025 08:49, Pierre Gondois wrote:
>>>>>
>>>>> On 1/20/25 04:15, zhenglifeng (A) wrote:
>>>>>> On 2025/1/17 22:30, Mario Limonciello wrote:
>>>>>>
>>>>>>> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>>>>>>>> On 2025/1/16 19:39, Russell Haley wrote:
>>>>>>>>
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> I noticed something here just as a user casually browsing the mailing list.
>>>>>>>>>
>>>>>>>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>>>>>>>> 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..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.
>>>>>>>>> [...snip...]
>>>>>>>>>
>>>>>>>>>> +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.
>>>>>>>>> In intel_pstate driver, there is file with near-identical semantics:
>>>>>>>>>
>>>>>>>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>>>>>>>>
>>>>>>>>> It also accepts a few string arguments and converts them to integers.
>>>>>>>>>
>>>>>>>>> Perhaps the same name should be used, and the semantics made exactly
>>>>>>>>> identical, and then it could be documented as present for either
>>>>>>>>> cppc_cpufreq OR intel_pstate?
>>>>>>>>>
>>>>>>>>> I think would be more elegant if userspace tooling could Just Work with
>>>>>>>>> either driver.
>>>>>>>>>
>>>>>>>>> One might object that the frequency selection behavior that results from
>>>>>>>>> any particular value of the register itself might be different, but they
>>>>>>>>> are *already* different between Intel's P and E-cores in the same CPU
>>>>>>>>> package. (Ugh.)
>>>>>>>> Yes, I should use the same name. Thanks.
>>>>>>>>
>>>>>>>> As for accepting string arguments and converting them to integers, I don't
>>>>>>>> think it is necessary. It'll be a litte confused if someone writes a raw
>>>>>>>> value and reads a string I think. I prefer to let users freely set this
>>>>>>>> value.
>>>>>>>>
>>>>>>>> In addition, there are many differences between the implementations of
>>>>>>>> energy_performance_preference in intel_pstate and cppc_cpufreq (and
>>>>>>>> amd-pstate...). It is really difficult to explain all this differences in
>>>>>>>> this document. So I'll leave it to be documented as present for
>>>>>>>> cppc_cpufreq only.
>>>>>>> At least the interface to userspace I think we should do the best we can to be the same between all the drivers if possible.
>>>>>>>
>>>>>>> For example; I've got a patch that I may bring up in a future kernel cycle that adds raw integer writes to amd-pstates energy_performance_profile to behave the same way intel-pstate does.
>>>>>> I agree that it's better to keep this interface consistent across different
>>>>>> drivers. But in my opinion, the implementation of intel_pstate
>>>>>> energy_performance_preference is not really nice. Someone may write a raw
>>>>>> value but read a string, or read strings for some values and read raw
>>>>>> values for some other values. It is inconsistent. It may be better to use
>>>>>> some other implementation, such as seperating the operations of r/w strings
>>>>>> and raw values into two files.
>>>>> I agree it would be better to be sure of the type to expect when reading the
>>>>> energy_performance_preference file. The epp values in the range 0-255 with 0
>>>>> being the performance value for all interfaces.
>>>>>
>>>>> In the current epp strings, it seems there is a big gap between the PERFORMANCE
>>>>> and the BALANCE_PERFORMANCE strings. Maybe it would be good to complete it:
>>>>> EPP_PERFORMANCE        0x00
>>>>> EPP_BALANCE_PERFORMANCE    0x40      // state value changed
>>>>> EPP_BALANCE        0x80      // new state
>>>>> EPP_BALANCE_POWERSAVE    0xC0
>>>>> EPP_POWERSAVE        0xFF
>>>>>
>>>>> NIT: The mapping seems to be slightly different for intel_pstate and amd-pstate
>>>>> currently:
>>>>> drivers/cpufreq/amd-pstate.c
>>>>> #define AMD_CPPC_EPP_PERFORMANCE        0x00
>>>>> #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
>>>>> #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
>>>>> #define AMD_CPPC_EPP_POWERSAVE            0xFF
>>>>>
>>>>> arch/x86/include/asm/msr-index.h
>>>>> #define HWP_EPP_PERFORMANCE        0x00
>>>>> #define HWP_EPP_BALANCE_PERFORMANCE    0x80
>>>>> #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------ Different from AMD_CPPC_EPP_BALANCE_POWERSAVE
>>>>> #define HWP_EPP_POWERSAVE        0xFF
>>>>>
>>>>>> I think it's better to consult Rafael and Viresh about how this should
>>>>>> evolve.
>>>>> Yes indeed
>>>> Maybe it's best to discuss what the goal of raw EPP number writes is to decide what to do with it.
>>>>
>>>> IE in intel-pstate is it for userspace to be able to actually utilize something besides the strings all the time?  Or is it just for debugging to find better values for strings in the future?
>>>>
>>>> If the former maybe we're better off splitting to 'energy_performance_preference' and 'energy_performance_preference_int'.
>>>>
>>>> If the latter maybe we're better off putting the integer writes and reads into debugfs instead and making 'energy_performance_preference' return -EINVAL while a non-predefined value is in use.
>>
>> In Intel case EPP values can be different based on processor. In some case they they end up sharing the same CPU model. So strings are not suitable for all cases. Also there is different preference of EPP between Chrome systems and non chrome distro. For example Chrome has some resource manager which can change and same on Intel distros with LPMD.
>>
> 
> Thanks for confirming it is intentional and changing it would break existing userspace.
> 
> And FWIW even in Windows there are more than 4 situational values used like we have in Linux today.
> 
> As the status quo is there I personally feel that we should do the exact same for other implementation of energy_performance_preference.

I still don't think this implementation is nice, for the following reasons:

1. Users may write raw value but read string. It's odd.

2. Sometimes a raw value is read and sometimes a character string is read.
The userspace tool needs to adapt this.

3. Reading and writing EPP strings is not really general in driver. It is
more reasonable to use the userspace tool to implement it.

In order not to break existing userspace, I'll rename the file to
'energy_performance_preference_int' or 'energy_performance_preference_val'
in cppc_cpufreq and only support reading and writing raw value. As for
accepting string arguments, it's not necessary for cppc_cpufreq for now.
It's easy to add this feature but hard to remove, so I'll leave it to the
future if it is really needed.

As for amd-pstate and intel_pstate, you can decide how
energy_performance_preference should evolve. But I strongly recommend
splitting it.

Regards,

Lifeng

> 
>>
>> Thanks,
>>
>> Srinivas
>>
>>
>>> I think it's the former.
>>>
>>> I added the author of the patch that allows raw energy performance
>>> preference value in intel_pstate to ask about what the goal is and if this
>>> would be ok to do the modification mentioned above.
>>>
>>> To see the patch from https://lore.kernel.org/ all/20200626183401.1495090-3-srinivas.pandruvada@linux.intel.com/
>>>
>>> Anyway, the purpose of this patch is to allow users write and read raw EPP
>>> number. So maybe I can just rename the file to
>>> 'energy_performance_preference_int'?
>>>
> 

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[srinivas pandruvada]

On Fri, 2025-01-24 at 11:53 +0800, zhenglifeng (A) wrote:
> On 2025/1/24 1:05, Mario Limonciello wrote:
> 
> > On 1/23/2025 10:46, Srinivas Pandruvada wrote:
> > > 
> > > On 1/20/25 18:42, zhenglifeng (A) wrote:
> > > > On 2025/1/21 1:44, Mario Limonciello wrote:
> > > > 
> > > > > On 1/20/2025 08:49, Pierre Gondois wrote:
> > > > > > 
> > > > > > On 1/20/25 04:15, zhenglifeng (A) wrote:
> > > > > > > On 2025/1/17 22:30, Mario Limonciello wrote:
> > > > > > > 
> > > > > > > > On 1/16/2025 21:11, zhenglifeng (A) wrote:
> > > > > > > > > On 2025/1/16 19:39, Russell Haley wrote:
> > > > > > > > > 
> > > > > > > > > > Hello,
> > > > > > > > > > 
> > > > > > > > > > I noticed something here just as a user casually
> > > > > > > > > > browsing the mailing list.
> > > > > > > > > > 
> > > > > > > > > > On 1/13/25 6:21 AM, Lifeng Zheng wrote:
> > > > > > > > > > > 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..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.
> > > > > > > > > > [...snip...]
> > > > > > > > > > 
> > > > > > > > > > > +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.
> > > > > > > > > > In intel_pstate driver, there is file with near-
> > > > > > > > > > identical semantics:
> > > > > > > > > > 
> > > > > > > > > > /sys/devices/system/cpu/cpuX/cpufreq/energy_perform
> > > > > > > > > > ance_preference
> > > > > > > > > > 
> > > > > > > > > > It also accepts a few string arguments and converts
> > > > > > > > > > them to integers.
> > > > > > > > > > 
> > > > > > > > > > Perhaps the same name should be used, and the
> > > > > > > > > > semantics made exactly
> > > > > > > > > > identical, and then it could be documented as
> > > > > > > > > > present for either
> > > > > > > > > > cppc_cpufreq OR intel_pstate?
> > > > > > > > > > 
> > > > > > > > > > I think would be more elegant if userspace tooling
> > > > > > > > > > could Just Work with
> > > > > > > > > > either driver.
> > > > > > > > > > 
> > > > > > > > > > One might object that the frequency selection
> > > > > > > > > > behavior that results from
> > > > > > > > > > any particular value of the register itself might
> > > > > > > > > > be different, but they
> > > > > > > > > > are *already* different between Intel's P and E-
> > > > > > > > > > cores in the same CPU
> > > > > > > > > > package. (Ugh.)
> > > > > > > > > Yes, I should use the same name. Thanks.
> > > > > > > > > 
> > > > > > > > > As for accepting string arguments and converting them
> > > > > > > > > to integers, I don't
> > > > > > > > > think it is necessary. It'll be a litte confused if
> > > > > > > > > someone writes a raw
> > > > > > > > > value and reads a string I think. I prefer to let
> > > > > > > > > users freely set this
> > > > > > > > > value.
> > > > > > > > > 
> > > > > > > > > In addition, there are many differences between the
> > > > > > > > > implementations of
> > > > > > > > > energy_performance_preference in intel_pstate and
> > > > > > > > > cppc_cpufreq (and
> > > > > > > > > amd-pstate...). It is really difficult to explain all
> > > > > > > > > this differences in
> > > > > > > > > this document. So I'll leave it to be documented as
> > > > > > > > > present for
> > > > > > > > > cppc_cpufreq only.
> > > > > > > > At least the interface to userspace I think we should
> > > > > > > > do the best we can to be the same between all the
> > > > > > > > drivers if possible.
> > > > > > > > 
> > > > > > > > For example; I've got a patch that I may bring up in a
> > > > > > > > future kernel cycle that adds raw integer writes to
> > > > > > > > amd-pstates energy_performance_profile to behave the
> > > > > > > > same way intel-pstate does.
> > > > > > > I agree that it's better to keep this interface
> > > > > > > consistent across different
> > > > > > > drivers. But in my opinion, the implementation of
> > > > > > > intel_pstate
> > > > > > > energy_performance_preference is not really nice. Someone
> > > > > > > may write a raw
> > > > > > > value but read a string, or read strings for some values
> > > > > > > and read raw
> > > > > > > values for some other values. It is inconsistent. It may
> > > > > > > be better to use
> > > > > > > some other implementation, such as seperating the
> > > > > > > operations of r/w strings
> > > > > > > and raw values into two files.
> > > > > > I agree it would be better to be sure of the type to expect
> > > > > > when reading the
> > > > > > energy_performance_preference file. The epp values in the
> > > > > > range 0-255 with 0
> > > > > > being the performance value for all interfaces.
> > > > > > 
> > > > > > In the current epp strings, it seems there is a big gap
> > > > > > between the PERFORMANCE
> > > > > > and the BALANCE_PERFORMANCE strings. Maybe it would be good
> > > > > > to complete it:
> > > > > > EPP_PERFORMANCE        0x00
> > > > > > EPP_BALANCE_PERFORMANCE    0x40      // state value changed
> > > > > > EPP_BALANCE        0x80      // new state
> > > > > > EPP_BALANCE_POWERSAVE    0xC0
> > > > > > EPP_POWERSAVE        0xFF
> > > > > > 
> > > > > > NIT: The mapping seems to be slightly different for
> > > > > > intel_pstate and amd-pstate
> > > > > > currently:
> > > > > > drivers/cpufreq/amd-pstate.c
> > > > > > #define AMD_CPPC_EPP_PERFORMANCE        0x00
> > > > > > #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
> > > > > > #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
> > > > > > #define AMD_CPPC_EPP_POWERSAVE            0xFF
> > > > > > 
> > > > > > arch/x86/include/asm/msr-index.h
> > > > > > #define HWP_EPP_PERFORMANCE        0x00
> > > > > > #define HWP_EPP_BALANCE_PERFORMANCE    0x80
> > > > > > #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------
> > > > > > Different from AMD_CPPC_EPP_BALANCE_POWERSAVE
> > > > > > #define HWP_EPP_POWERSAVE        0xFF
> > > > > > 
> > > > > > > I think it's better to consult Rafael and Viresh about
> > > > > > > how this should
> > > > > > > evolve.
> > > > > > Yes indeed
> > > > > Maybe it's best to discuss what the goal of raw EPP number
> > > > > writes is to decide what to do with it.
> > > > > 
> > > > > IE in intel-pstate is it for userspace to be able to actually
> > > > > utilize something besides the strings all the time?  Or is it
> > > > > just for debugging to find better values for strings in the
> > > > > future?
> > > > > 
> > > > > If the former maybe we're better off splitting to
> > > > > 'energy_performance_preference' and
> > > > > 'energy_performance_preference_int'.
> > > > > 
> > > > > If the latter maybe we're better off putting the integer
> > > > > writes and reads into debugfs instead and making
> > > > > 'energy_performance_preference' return -EINVAL while a non-
> > > > > predefined value is in use.
> > > 
> > > In Intel case EPP values can be different based on processor. In
> > > some case they they end up sharing the same CPU model. So strings
> > > are not suitable for all cases. Also there is different
> > > preference of EPP between Chrome systems and non chrome distro.
> > > For example Chrome has some resource manager which can change and
> > > same on Intel distros with LPMD.
> > > 
> > 
> > Thanks for confirming it is intentional and changing it would break
> > existing userspace.
> > 
> > And FWIW even in Windows there are more than 4 situational values
> > used like we have in Linux today.
> > 
> > As the status quo is there I personally feel that we should do the
> > exact same for other implementation of
> > energy_performance_preference.
> 
> I still don't think this implementation is nice, for the following
> reasons:
> 
> 1. Users may write raw value but read string. It's odd.
> 
> 2. Sometimes a raw value is read and sometimes a character string is
> read.
> The userspace tool needs to adapt this.
> 
> 3. Reading and writing EPP strings is not really general in driver.
> It is
> more reasonable to use the userspace tool to implement it.
> 
> In order not to break existing userspace, I'll rename the file to
> 'energy_performance_preference_int' or
> 'energy_performance_preference_val'
> in cppc_cpufreq and only support reading and writing raw value. As
> for
> accepting string arguments, it's not necessary for cppc_cpufreq for
> now.
> It's easy to add this feature but hard to remove, so I'll leave it to
> the
> future if it is really needed.
> 
> As for amd-pstate and intel_pstate, you can decide how
> energy_performance_preference should evolve. But I strongly recommend
> splitting it.

No. User space can deal with this already. Atleast this has one
interface. If you split you need to keep them consistent. You can write
a raw value  to new attribute and then can read a string from the other
attribute, which means different.

This is not the only place where strings and raw values can be written
in sysfs. Also true for energy_perf_bias.


Thanks,
Srinivas



> 
> Regards,
> 
> Lifeng
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > Srinivas
> > > 
> > > 
> > > > I think it's the former.
> > > > 
> > > > I added the author of the patch that allows raw energy
> > > > performance
> > > > preference value in intel_pstate to ask about what the goal is
> > > > and if this
> > > > would be ok to do the modification mentioned above.
> > > > 
> > > > To see the patch from
> > > > https://lore.kernel.org/ all/20200626183401.1495090-3-srinivas.pandruvada@linux.intel.c
> > > > om/
> > > > 
> > > > Anyway, the purpose of this patch is to allow users write and
> > > > read raw EPP
> > > > number. So maybe I can just rename the file to
> > > > 'energy_performance_preference_int'?
> > > > 
> > 
> 

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

Hello,

Sorry for my delay due to the recent holiday.

On 2025/1/24 22:18, srinivas pandruvada wrote:

> On Fri, 2025-01-24 at 11:53 +0800, zhenglifeng (A) wrote:
>> On 2025/1/24 1:05, Mario Limonciello wrote:
>>
>>> On 1/23/2025 10:46, Srinivas Pandruvada wrote:
>>>>
>>>> On 1/20/25 18:42, zhenglifeng (A) wrote:
>>>>> On 2025/1/21 1:44, Mario Limonciello wrote:
>>>>>
>>>>>> On 1/20/2025 08:49, Pierre Gondois wrote:
>>>>>>>
>>>>>>> On 1/20/25 04:15, zhenglifeng (A) wrote:
>>>>>>>> On 2025/1/17 22:30, Mario Limonciello wrote:
>>>>>>>>
>>>>>>>>> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>>>>>>>>>> On 2025/1/16 19:39, Russell Haley wrote:
>>>>>>>>>>
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> I noticed something here just as a user casually
>>>>>>>>>>> browsing the mailing list.
>>>>>>>>>>>
>>>>>>>>>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>>>>>>>>>> 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..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.
>>>>>>>>>>> [...snip...]
>>>>>>>>>>>
>>>>>>>>>>>> +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.
>>>>>>>>>>> In intel_pstate driver, there is file with near-
>>>>>>>>>>> identical semantics:
>>>>>>>>>>>
>>>>>>>>>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_perform
>>>>>>>>>>> ance_preference
>>>>>>>>>>>
>>>>>>>>>>> It also accepts a few string arguments and converts
>>>>>>>>>>> them to integers.
>>>>>>>>>>>
>>>>>>>>>>> Perhaps the same name should be used, and the
>>>>>>>>>>> semantics made exactly
>>>>>>>>>>> identical, and then it could be documented as
>>>>>>>>>>> present for either
>>>>>>>>>>> cppc_cpufreq OR intel_pstate?
>>>>>>>>>>>
>>>>>>>>>>> I think would be more elegant if userspace tooling
>>>>>>>>>>> could Just Work with
>>>>>>>>>>> either driver.
>>>>>>>>>>>
>>>>>>>>>>> One might object that the frequency selection
>>>>>>>>>>> behavior that results from
>>>>>>>>>>> any particular value of the register itself might
>>>>>>>>>>> be different, but they
>>>>>>>>>>> are *already* different between Intel's P and E-
>>>>>>>>>>> cores in the same CPU
>>>>>>>>>>> package. (Ugh.)
>>>>>>>>>> Yes, I should use the same name. Thanks.
>>>>>>>>>>
>>>>>>>>>> As for accepting string arguments and converting them
>>>>>>>>>> to integers, I don't
>>>>>>>>>> think it is necessary. It'll be a litte confused if
>>>>>>>>>> someone writes a raw
>>>>>>>>>> value and reads a string I think. I prefer to let
>>>>>>>>>> users freely set this
>>>>>>>>>> value.
>>>>>>>>>>
>>>>>>>>>> In addition, there are many differences between the
>>>>>>>>>> implementations of
>>>>>>>>>> energy_performance_preference in intel_pstate and
>>>>>>>>>> cppc_cpufreq (and
>>>>>>>>>> amd-pstate...). It is really difficult to explain all
>>>>>>>>>> this differences in
>>>>>>>>>> this document. So I'll leave it to be documented as
>>>>>>>>>> present for
>>>>>>>>>> cppc_cpufreq only.
>>>>>>>>> At least the interface to userspace I think we should
>>>>>>>>> do the best we can to be the same between all the
>>>>>>>>> drivers if possible.
>>>>>>>>>
>>>>>>>>> For example; I've got a patch that I may bring up in a
>>>>>>>>> future kernel cycle that adds raw integer writes to
>>>>>>>>> amd-pstates energy_performance_profile to behave the
>>>>>>>>> same way intel-pstate does.
>>>>>>>> I agree that it's better to keep this interface
>>>>>>>> consistent across different
>>>>>>>> drivers. But in my opinion, the implementation of
>>>>>>>> intel_pstate
>>>>>>>> energy_performance_preference is not really nice. Someone
>>>>>>>> may write a raw
>>>>>>>> value but read a string, or read strings for some values
>>>>>>>> and read raw
>>>>>>>> values for some other values. It is inconsistent. It may
>>>>>>>> be better to use
>>>>>>>> some other implementation, such as seperating the
>>>>>>>> operations of r/w strings
>>>>>>>> and raw values into two files.
>>>>>>> I agree it would be better to be sure of the type to expect
>>>>>>> when reading the
>>>>>>> energy_performance_preference file. The epp values in the
>>>>>>> range 0-255 with 0
>>>>>>> being the performance value for all interfaces.
>>>>>>>
>>>>>>> In the current epp strings, it seems there is a big gap
>>>>>>> between the PERFORMANCE
>>>>>>> and the BALANCE_PERFORMANCE strings. Maybe it would be good
>>>>>>> to complete it:
>>>>>>> EPP_PERFORMANCE        0x00
>>>>>>> EPP_BALANCE_PERFORMANCE    0x40      // state value changed
>>>>>>> EPP_BALANCE        0x80      // new state
>>>>>>> EPP_BALANCE_POWERSAVE    0xC0
>>>>>>> EPP_POWERSAVE        0xFF
>>>>>>>
>>>>>>> NIT: The mapping seems to be slightly different for
>>>>>>> intel_pstate and amd-pstate
>>>>>>> currently:
>>>>>>> drivers/cpufreq/amd-pstate.c
>>>>>>> #define AMD_CPPC_EPP_PERFORMANCE        0x00
>>>>>>> #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
>>>>>>> #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
>>>>>>> #define AMD_CPPC_EPP_POWERSAVE            0xFF
>>>>>>>
>>>>>>> arch/x86/include/asm/msr-index.h
>>>>>>> #define HWP_EPP_PERFORMANCE        0x00
>>>>>>> #define HWP_EPP_BALANCE_PERFORMANCE    0x80
>>>>>>> #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------
>>>>>>> Different from AMD_CPPC_EPP_BALANCE_POWERSAVE
>>>>>>> #define HWP_EPP_POWERSAVE        0xFF
>>>>>>>
>>>>>>>> I think it's better to consult Rafael and Viresh about
>>>>>>>> how this should
>>>>>>>> evolve.
>>>>>>> Yes indeed
>>>>>> Maybe it's best to discuss what the goal of raw EPP number
>>>>>> writes is to decide what to do with it.
>>>>>>
>>>>>> IE in intel-pstate is it for userspace to be able to actually
>>>>>> utilize something besides the strings all the time?  Or is it
>>>>>> just for debugging to find better values for strings in the
>>>>>> future?
>>>>>>
>>>>>> If the former maybe we're better off splitting to
>>>>>> 'energy_performance_preference' and
>>>>>> 'energy_performance_preference_int'.
>>>>>>
>>>>>> If the latter maybe we're better off putting the integer
>>>>>> writes and reads into debugfs instead and making
>>>>>> 'energy_performance_preference' return -EINVAL while a non-
>>>>>> predefined value is in use.
>>>>
>>>> In Intel case EPP values can be different based on processor. In
>>>> some case they they end up sharing the same CPU model. So strings
>>>> are not suitable for all cases. Also there is different
>>>> preference of EPP between Chrome systems and non chrome distro.
>>>> For example Chrome has some resource manager which can change and
>>>> same on Intel distros with LPMD.
>>>>
>>>
>>> Thanks for confirming it is intentional and changing it would break
>>> existing userspace.
>>>
>>> And FWIW even in Windows there are more than 4 situational values
>>> used like we have in Linux today.
>>>
>>> As the status quo is there I personally feel that we should do the
>>> exact same for other implementation of
>>> energy_performance_preference.
>>
>> I still don't think this implementation is nice, for the following
>> reasons:
>>
>> 1. Users may write raw value but read string. It's odd.
>>
>> 2. Sometimes a raw value is read and sometimes a character string is
>> read.
>> The userspace tool needs to adapt this.
>>
>> 3. Reading and writing EPP strings is not really general in driver.
>> It is
>> more reasonable to use the userspace tool to implement it.
>>
>> In order not to break existing userspace, I'll rename the file to
>> 'energy_performance_preference_int' or
>> 'energy_performance_preference_val'
>> in cppc_cpufreq and only support reading and writing raw value. As
>> for
>> accepting string arguments, it's not necessary for cppc_cpufreq for
>> now.
>> It's easy to add this feature but hard to remove, so I'll leave it to
>> the
>> future if it is really needed.
>>
>> As for amd-pstate and intel_pstate, you can decide how
>> energy_performance_preference should evolve. But I strongly recommend
>> splitting it.
> 
> No. User space can deal with this already. 

I know that. What I mean is that user space had to do more to adapt this
"sometims value sometimes string" situation. This could have been avoided.

> Atleast this has one
> interface. If you split you need to keep them consistent. You can write
> a raw value  to new attribute and then can read a string from the other
> attribute, which means different.

I won't do any change in amd-pstate and intel_pstate because it involves
too much. I'll only add a new file in cppc_cpufreq for reading and writing
EPP raw value.

> 
> This is not the only place where strings and raw values can be written
> in sysfs. Also true for energy_perf_bias.
> 
> 
> Thanks,
> Srinivas
> 
> 
> 
>>
>> Regards,
>>
>> Lifeng
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Srinivas
>>>>
>>>>
>>>>> I think it's the former.
>>>>>
>>>>> I added the author of the patch that allows raw energy
>>>>> performance
>>>>> preference value in intel_pstate to ask about what the goal is
>>>>> and if this
>>>>> would be ok to do the modification mentioned above.
>>>>>
>>>>> To see the patch from
>>>>> https://lore.kernel.org/ all/20200626183401.1495090-3-srinivas.pandruvada@linux.intel.c
>>>>> om/
>>>>>
>>>>> Anyway, the purpose of this patch is to allow users write and
>>>>> read raw EPP
>>>>> number. So maybe I can just rename the file to
>>>>> 'energy_performance_preference_int'?
>>>>>
>>>
>>
> 
> 

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[Russell Haley]

On 1/23/25 9:53 PM, zhenglifeng (A) wrote:
> On 2025/1/24 1:05, Mario Limonciello wrote:
> 
>> On 1/23/2025 10:46, Srinivas Pandruvada wrote:
>>>
>>> On 1/20/25 18:42, zhenglifeng (A) wrote:
>>>> On 2025/1/21 1:44, Mario Limonciello wrote:
>>>>
>>>>> On 1/20/2025 08:49, Pierre Gondois wrote:
>>>>>>
>>>>>> On 1/20/25 04:15, zhenglifeng (A) wrote:
>>>>>>> On 2025/1/17 22:30, Mario Limonciello wrote:
>>>>>>>
>>>>>>>> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>>>>>>>>> On 2025/1/16 19:39, Russell Haley wrote:
>>>>>>>>>
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> I noticed something here just as a user casually browsing the mailing list.
>>>>>>>>>>
>>>>>>>>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>>>>>>>>> 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..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.
>>>>>>>>>> [...snip...]
>>>>>>>>>>
>>>>>>>>>>> +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.
>>>>>>>>>> In intel_pstate driver, there is file with near-identical semantics:
>>>>>>>>>>
>>>>>>>>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>>>>>>>>>
>>>>>>>>>> It also accepts a few string arguments and converts them to integers.
>>>>>>>>>>
>>>>>>>>>> Perhaps the same name should be used, and the semantics made exactly
>>>>>>>>>> identical, and then it could be documented as present for either
>>>>>>>>>> cppc_cpufreq OR intel_pstate?
>>>>>>>>>>
>>>>>>>>>> I think would be more elegant if userspace tooling could Just Work with
>>>>>>>>>> either driver.
>>>>>>>>>>
>>>>>>>>>> One might object that the frequency selection behavior that results from
>>>>>>>>>> any particular value of the register itself might be different, but they
>>>>>>>>>> are *already* different between Intel's P and E-cores in the same CPU
>>>>>>>>>> package. (Ugh.)
>>>>>>>>> Yes, I should use the same name. Thanks.
>>>>>>>>>
>>>>>>>>> As for accepting string arguments and converting them to integers, I don't
>>>>>>>>> think it is necessary. It'll be a litte confused if someone writes a raw
>>>>>>>>> value and reads a string I think. I prefer to let users freely set this
>>>>>>>>> value.
>>>>>>>>>
>>>>>>>>> In addition, there are many differences between the implementations of
>>>>>>>>> energy_performance_preference in intel_pstate and cppc_cpufreq (and
>>>>>>>>> amd-pstate...). It is really difficult to explain all this differences in
>>>>>>>>> this document. So I'll leave it to be documented as present for
>>>>>>>>> cppc_cpufreq only.
>>>>>>>> At least the interface to userspace I think we should do the best we can to be the same between all the drivers if possible.
>>>>>>>>
>>>>>>>> For example; I've got a patch that I may bring up in a future kernel cycle that adds raw integer writes to amd-pstates energy_performance_profile to behave the same way intel-pstate does.
>>>>>>> I agree that it's better to keep this interface consistent across different
>>>>>>> drivers. But in my opinion, the implementation of intel_pstate
>>>>>>> energy_performance_preference is not really nice. Someone may write a raw
>>>>>>> value but read a string, or read strings for some values and read raw
>>>>>>> values for some other values. It is inconsistent. It may be better to use
>>>>>>> some other implementation, such as seperating the operations of r/w strings
>>>>>>> and raw values into two files.
>>>>>> I agree it would be better to be sure of the type to expect when reading the
>>>>>> energy_performance_preference file. The epp values in the range 0-255 with 0
>>>>>> being the performance value for all interfaces.
>>>>>>
>>>>>> In the current epp strings, it seems there is a big gap between the PERFORMANCE
>>>>>> and the BALANCE_PERFORMANCE strings. Maybe it would be good to complete it:
>>>>>> EPP_PERFORMANCE        0x00
>>>>>> EPP_BALANCE_PERFORMANCE    0x40      // state value changed
>>>>>> EPP_BALANCE        0x80      // new state
>>>>>> EPP_BALANCE_POWERSAVE    0xC0
>>>>>> EPP_POWERSAVE        0xFF
>>>>>>
>>>>>> NIT: The mapping seems to be slightly different for intel_pstate and amd-pstate
>>>>>> currently:
>>>>>> drivers/cpufreq/amd-pstate.c
>>>>>> #define AMD_CPPC_EPP_PERFORMANCE        0x00
>>>>>> #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
>>>>>> #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
>>>>>> #define AMD_CPPC_EPP_POWERSAVE            0xFF
>>>>>>
>>>>>> arch/x86/include/asm/msr-index.h
>>>>>> #define HWP_EPP_PERFORMANCE        0x00
>>>>>> #define HWP_EPP_BALANCE_PERFORMANCE    0x80
>>>>>> #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------ Different from AMD_CPPC_EPP_BALANCE_POWERSAVE
>>>>>> #define HWP_EPP_POWERSAVE        0xFF
>>>>>>
>>>>>>> I think it's better to consult Rafael and Viresh about how this should
>>>>>>> evolve.
>>>>>> Yes indeed
>>>>> Maybe it's best to discuss what the goal of raw EPP number writes is to decide what to do with it.
>>>>>
>>>>> IE in intel-pstate is it for userspace to be able to actually utilize something besides the strings all the time?  Or is it just for debugging to find better values for strings in the future?
>>>>>
>>>>> If the former maybe we're better off splitting to 'energy_performance_preference' and 'energy_performance_preference_int'.
>>>>>
>>>>> If the latter maybe we're better off putting the integer writes and reads into debugfs instead and making 'energy_performance_preference' return -EINVAL while a non-predefined value is in use.
>>>
>>> In Intel case EPP values can be different based on processor. In some case they they end up sharing the same CPU model. So strings are not suitable for all cases. Also there is different preference of EPP between Chrome systems and non chrome distro. For example Chrome has some resource manager which can change and same on Intel distros with LPMD.
>>>
>>
>> Thanks for confirming it is intentional and changing it would break existing userspace.
>>
>> And FWIW even in Windows there are more than 4 situational values used like we have in Linux today.
>>
>> As the status quo is there I personally feel that we should do the exact same for other implementation of energy_performance_preference.
> 
> I still don't think this implementation is nice, for the following reasons:
> 
> 1. Users may write raw value but read string. It's odd.
> 
> 2. Sometimes a raw value is read and sometimes a character string is read.
> The userspace tool needs to adapt this.
> 
> 3. Reading and writing EPP strings is not really general in driver. It is
> more reasonable to use the userspace tool to implement it.
> 
> In order not to break existing userspace, I'll rename the file to
> 'energy_performance_preference_int' or 'energy_performance_preference_val'
> in cppc_cpufreq and only support reading and writing raw value. As for
> accepting string arguments, it's not necessary for cppc_cpufreq for now.
> It's easy to add this feature but hard to remove, so I'll leave it to the
> future if it is really needed.
> 
> As for amd-pstate and intel_pstate, you can decide how
> energy_performance_preference should evolve. But I strongly recommend
> splitting it.
> 
> Regards,
> 
> Lifeng

I agree that not being able to write-read-confirm a numeric value that
happens to match one of the strings is... ugly. I have seen cases of
userspace fighting with firmware for control of the EPP, and detecting
that's happening is difficult if there are *other* reasons you might not
get back what you just wrote.

However, the desktop userspace pretty much only uses the strings anyway,
and they serve to translate arbitrary non-linear hardware-specific
scales into something userspace can build policy on. They are somewhat
less magic than the raw values, although still, IMO, pretty magic.

The raw values don't have consistent interpretation other than that
higher numbers give monotonically increasing efficiency for that
specific core, and I use that wording particularly. Lower numbers may
not increase performance because of long-term power/thermal limits, and
on asymmetric-CPU machines, the same number may not mean the same thing
for different cores in the same CPU package.

Leaving it up to userspace means either you need machine-model-specific
golden images, manually tuned by a skilled administrator, or CPU model
checks and an out-of-tree hardware database that somehow everyone
collaborates on. That database would likely miss a lot of things that
aren't popular X86 CPUs less than 5 years old.

Thanks,

Russell
>>
> 

Re: [PATCH v4 6/6] cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq

[zhenglifeng (A)]

Hello,

Sorry for my delay due to the recent holiday.

On 2025/1/24 22:32, Russell Haley wrote:

> On 1/23/25 9:53 PM, zhenglifeng (A) wrote:
>> On 2025/1/24 1:05, Mario Limonciello wrote:
>>
>>> On 1/23/2025 10:46, Srinivas Pandruvada wrote:
>>>>
>>>> On 1/20/25 18:42, zhenglifeng (A) wrote:
>>>>> On 2025/1/21 1:44, Mario Limonciello wrote:
>>>>>
>>>>>> On 1/20/2025 08:49, Pierre Gondois wrote:
>>>>>>>
>>>>>>> On 1/20/25 04:15, zhenglifeng (A) wrote:
>>>>>>>> On 2025/1/17 22:30, Mario Limonciello wrote:
>>>>>>>>
>>>>>>>>> On 1/16/2025 21:11, zhenglifeng (A) wrote:
>>>>>>>>>> On 2025/1/16 19:39, Russell Haley wrote:
>>>>>>>>>>
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> I noticed something here just as a user casually browsing the mailing list.
>>>>>>>>>>>
>>>>>>>>>>> On 1/13/25 6:21 AM, Lifeng Zheng wrote:
>>>>>>>>>>>> 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..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.
>>>>>>>>>>> [...snip...]
>>>>>>>>>>>
>>>>>>>>>>>> +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.
>>>>>>>>>>> In intel_pstate driver, there is file with near-identical semantics:
>>>>>>>>>>>
>>>>>>>>>>> /sys/devices/system/cpu/cpuX/cpufreq/energy_performance_preference
>>>>>>>>>>>
>>>>>>>>>>> It also accepts a few string arguments and converts them to integers.
>>>>>>>>>>>
>>>>>>>>>>> Perhaps the same name should be used, and the semantics made exactly
>>>>>>>>>>> identical, and then it could be documented as present for either
>>>>>>>>>>> cppc_cpufreq OR intel_pstate?
>>>>>>>>>>>
>>>>>>>>>>> I think would be more elegant if userspace tooling could Just Work with
>>>>>>>>>>> either driver.
>>>>>>>>>>>
>>>>>>>>>>> One might object that the frequency selection behavior that results from
>>>>>>>>>>> any particular value of the register itself might be different, but they
>>>>>>>>>>> are *already* different between Intel's P and E-cores in the same CPU
>>>>>>>>>>> package. (Ugh.)
>>>>>>>>>> Yes, I should use the same name. Thanks.
>>>>>>>>>>
>>>>>>>>>> As for accepting string arguments and converting them to integers, I don't
>>>>>>>>>> think it is necessary. It'll be a litte confused if someone writes a raw
>>>>>>>>>> value and reads a string I think. I prefer to let users freely set this
>>>>>>>>>> value.
>>>>>>>>>>
>>>>>>>>>> In addition, there are many differences between the implementations of
>>>>>>>>>> energy_performance_preference in intel_pstate and cppc_cpufreq (and
>>>>>>>>>> amd-pstate...). It is really difficult to explain all this differences in
>>>>>>>>>> this document. So I'll leave it to be documented as present for
>>>>>>>>>> cppc_cpufreq only.
>>>>>>>>> At least the interface to userspace I think we should do the best we can to be the same between all the drivers if possible.
>>>>>>>>>
>>>>>>>>> For example; I've got a patch that I may bring up in a future kernel cycle that adds raw integer writes to amd-pstates energy_performance_profile to behave the same way intel-pstate does.
>>>>>>>> I agree that it's better to keep this interface consistent across different
>>>>>>>> drivers. But in my opinion, the implementation of intel_pstate
>>>>>>>> energy_performance_preference is not really nice. Someone may write a raw
>>>>>>>> value but read a string, or read strings for some values and read raw
>>>>>>>> values for some other values. It is inconsistent. It may be better to use
>>>>>>>> some other implementation, such as seperating the operations of r/w strings
>>>>>>>> and raw values into two files.
>>>>>>> I agree it would be better to be sure of the type to expect when reading the
>>>>>>> energy_performance_preference file. The epp values in the range 0-255 with 0
>>>>>>> being the performance value for all interfaces.
>>>>>>>
>>>>>>> In the current epp strings, it seems there is a big gap between the PERFORMANCE
>>>>>>> and the BALANCE_PERFORMANCE strings. Maybe it would be good to complete it:
>>>>>>> EPP_PERFORMANCE        0x00
>>>>>>> EPP_BALANCE_PERFORMANCE    0x40      // state value changed
>>>>>>> EPP_BALANCE        0x80      // new state
>>>>>>> EPP_BALANCE_POWERSAVE    0xC0
>>>>>>> EPP_POWERSAVE        0xFF
>>>>>>>
>>>>>>> NIT: The mapping seems to be slightly different for intel_pstate and amd-pstate
>>>>>>> currently:
>>>>>>> drivers/cpufreq/amd-pstate.c
>>>>>>> #define AMD_CPPC_EPP_PERFORMANCE        0x00
>>>>>>> #define AMD_CPPC_EPP_BALANCE_PERFORMANCE    0x80
>>>>>>> #define AMD_CPPC_EPP_BALANCE_POWERSAVE        0xBF
>>>>>>> #define AMD_CPPC_EPP_POWERSAVE            0xFF
>>>>>>>
>>>>>>> arch/x86/include/asm/msr-index.h
>>>>>>> #define HWP_EPP_PERFORMANCE        0x00
>>>>>>> #define HWP_EPP_BALANCE_PERFORMANCE    0x80
>>>>>>> #define HWP_EPP_BALANCE_POWERSAVE    0xC0   <------ Different from AMD_CPPC_EPP_BALANCE_POWERSAVE
>>>>>>> #define HWP_EPP_POWERSAVE        0xFF
>>>>>>>
>>>>>>>> I think it's better to consult Rafael and Viresh about how this should
>>>>>>>> evolve.
>>>>>>> Yes indeed
>>>>>> Maybe it's best to discuss what the goal of raw EPP number writes is to decide what to do with it.
>>>>>>
>>>>>> IE in intel-pstate is it for userspace to be able to actually utilize something besides the strings all the time?  Or is it just for debugging to find better values for strings in the future?
>>>>>>
>>>>>> If the former maybe we're better off splitting to 'energy_performance_preference' and 'energy_performance_preference_int'.
>>>>>>
>>>>>> If the latter maybe we're better off putting the integer writes and reads into debugfs instead and making 'energy_performance_preference' return -EINVAL while a non-predefined value is in use.
>>>>
>>>> In Intel case EPP values can be different based on processor. In some case they they end up sharing the same CPU model. So strings are not suitable for all cases. Also there is different preference of EPP between Chrome systems and non chrome distro. For example Chrome has some resource manager which can change and same on Intel distros with LPMD.
>>>>
>>>
>>> Thanks for confirming it is intentional and changing it would break existing userspace.
>>>
>>> And FWIW even in Windows there are more than 4 situational values used like we have in Linux today.
>>>
>>> As the status quo is there I personally feel that we should do the exact same for other implementation of energy_performance_preference.
>>
>> I still don't think this implementation is nice, for the following reasons:
>>
>> 1. Users may write raw value but read string. It's odd.
>>
>> 2. Sometimes a raw value is read and sometimes a character string is read.
>> The userspace tool needs to adapt this.
>>
>> 3. Reading and writing EPP strings is not really general in driver. It is
>> more reasonable to use the userspace tool to implement it.
>>
>> In order not to break existing userspace, I'll rename the file to
>> 'energy_performance_preference_int' or 'energy_performance_preference_val'
>> in cppc_cpufreq and only support reading and writing raw value. As for
>> accepting string arguments, it's not necessary for cppc_cpufreq for now.
>> It's easy to add this feature but hard to remove, so I'll leave it to the
>> future if it is really needed.
>>
>> As for amd-pstate and intel_pstate, you can decide how
>> energy_performance_preference should evolve. But I strongly recommend
>> splitting it.
>>
>> Regards,
>>
>> Lifeng
> 
> I agree that not being able to write-read-confirm a numeric value that
> happens to match one of the strings is... ugly. I have seen cases of
> userspace fighting with firmware for control of the EPP, and detecting
> that's happening is difficult if there are *other* reasons you might not
> get back what you just wrote.
> 
> However, the desktop userspace pretty much only uses the strings anyway,
> and they serve to translate arbitrary non-linear hardware-specific
> scales into something userspace can build policy on. They are somewhat
> less magic than the raw values, although still, IMO, pretty magic.
> 
> The raw values don't have consistent interpretation other than that
> higher numbers give monotonically increasing efficiency for that
> specific core, and I use that wording particularly. Lower numbers may
> not increase performance because of long-term power/thermal limits, and
> on asymmetric-CPU machines, the same number may not mean the same thing
> for different cores in the same CPU package.
> 
> Leaving it up to userspace means either you need machine-model-specific
> golden images, manually tuned by a skilled administrator, or CPU model
> checks and an out-of-tree hardware database that somehow everyone
> collaborates on. That database would likely miss a lot of things that
> aren't popular X86 CPUs less than 5 years old.
> 
> Thanks,
> 
> Russell

If userspace only uses the strings, then it makes more sense to split it I
think.

>>>
>>
>