Implement AMD Pstate EPP Driver

Add the EPP(Energy Performance Preference) support for the AMD SoCs without the dedicated CPPC MSR, those SoCs need to add this cppc acpi functions to update EPP values and desired perf value.

In order to get EPP worked, cppc_get_epp_caps() will query EPP preference value and cppc_set_epp_perf() will set EPP new value.
Before the EPP works, pstate driver will use cppc_set_auto_epp() to enable EPP function from firmware firstly.

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/acpi/cppc_acpi.c | 126 +++++++++++++++++++++++++++++++++++++++
  include/acpi/cppc_acpi.h |  17 ++++++
  2 files changed, 143 insertions(+)

drivers/acpi/cppc_acpi.c +12 -0

index 093675b1a1ff..d9c38dee1f48 100644
@@ -1365,6 +1365,132 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
  }
  EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
 
+/**
+ * cppc_get_epp_caps - Get the energy preference register value.
+ * @cpunum: CPU from which to get epp preference level.
+ * @perf_caps: Return address.
+ *
+ * Return: 0 for success, -EIO otherwise.
+ */
+int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+{
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+	struct cpc_register_resource *energy_perf_reg;
+	u64 energy_perf;

drivers/acpi/cppc_acpi.c +29 -0

+
+	if (CPC_IN_PCC(energy_perf_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, energy_perf_reg, &energy_perf);
+			perf_caps->energy_perf = energy_perf;
+		} else {
+			ret = -EIO;
+		}
+
+		up_write(&pcc_ss_data->pcc_lock);
+
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cppc_get_epp_caps);
+

drivers/acpi/cppc_acpi.c +30 -0

+		return -EINVAL;
+	}
+
+	auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
+
+	if (CPC_IN_PCC(auto_sel_reg)) {
+		if (pcc_ss_id < 0)
+			return -ENODEV;
+
+		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);
+		return ret;
+	}
+
+	return cpc_write(cpu, auto_sel_reg, enable);
+}
+EXPORT_SYMBOL_GPL(cppc_set_auto_epp);
+
+/*
+ * Set Energy Performance Preference Register value through
+ * Performance Controls Interface
+ */

drivers/acpi/cppc_acpi.c +9 -0

+		return -EINVAL;
+	}
+
+	epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+	if (CPC_IN_PCC(epp_set_reg)) {
+		if (pcc_ss_id < 0)
+			return -ENODEV;
+

drivers/acpi/cppc_acpi.c +6 -0

+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
+
  /**
   * cppc_set_enable - Set to enable CPPC on the processor by writing the
   * Continuous Performance Control package EnableRegister field.

include/acpi/cppc_acpi.h +17 -0

index c5614444031f..10d91aeedaca 100644
@@ -108,12 +108,14 @@ struct cppc_perf_caps {
  	u32 lowest_nonlinear_perf;
  	u32 lowest_freq;
  	u32 nominal_freq;
+	u32 energy_perf;
  };
 
  struct cppc_perf_ctrls {
  	u32 max_perf;
  	u32 min_perf;
  	u32 desired_perf;
+	u32 energy_perf;
  };
 
  struct cppc_perf_fb_ctrs {

@@ -149,6 +151,9 @@ extern bool cpc_ffh_supported(void);
  extern bool cpc_supported_by_cpu(void);
  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_set_auto_epp(int cpu, bool enable);
+extern int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps);
+extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
  #else /* !CONFIG_ACPI_CPPC_LIB */
  static inline int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
  {

@@ -202,6 +207,18 @@ static inline int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
  {
  	return -ENOTSUPP;
  }
+static inline int cppc_set_auto_epp(int cpu, bool enable)
+{
+	return -ENOTSUPP;
+}
+static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+{
+	return -ENOTSUPP;
+}
+static inline int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+{
+	return -ENOTSUPP;
+}
  #endif /* !CONFIG_ACPI_CPPC_LIB */
 
  #endif /* _CPPC_ACPI_H*/

Quoted 3 lines · Perry Yuan

Add the EPP(Energy Performance Preference) support for the AMD SoCs without the dedicated CPPC MSR, those SoCs need to add this cppc acpi functions to update EPP values and desired perf value.

As far as I can tell this is generic code. Although the reason you're submitting it is for enabling AMD SoCs, the commit message should be worded as such.

Quoted 5 lines

In order to get EPP worked, cppc_get_epp_caps() will query EPP preference value and cppc_set_epp_perf() will set EPP new value.
Before the EPP works, pstate driver will use cppc_set_auto_epp() to enable EPP function from firmware firstly.

This could more succinctly say:

"Add support for setting and querying EPP preferences to the generic CPPC driver. This enables downstream drivers such as amd-pstate to discover and use these values."

Quoted 27 lines

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/acpi/cppc_acpi.c | 126 +++++++++++++++++++++++++++++++++++++++
  include/acpi/cppc_acpi.h |  17 ++++++
  2 files changed, 143 insertions(+)

drivers/acpi/cppc_acpi.c +12 -0

index 093675b1a1ff..d9c38dee1f48 100644
@@ -1365,6 +1365,132 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
  }
  EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
 
+/**
+ * cppc_get_epp_caps - Get the energy preference register value.
+ * @cpunum: CPU from which to get epp preference level.
+ * @perf_caps: Return address.
+ *
+ * Return: 0 for success, -EIO otherwise.
+ */
+int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+{
+     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+     struct cpc_register_resource *energy_perf_reg;
+     u64 energy_perf;

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1389

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+
+     if (!cpc_desc) {
+             pr_warn("No CPC descriptor for CPU:%d\n", cpunum);
+             return -ENODEV;
+     }
+
+     energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+     if (!CPC_SUPPORTED(energy_perf_reg))
+             pr_warn("energy perf reg update is unsupported!\n");

No need to add a explanation point at the end.

As this is a per-CPU message I wonder if this would be better as pr_warn_once()? Othewrise some systems with large numbers of cores might potentially show this message quite a few times.

Quoted 16 lines

drivers/acpi/cppc_acpi.c +16 -0

+
+     if (CPC_IN_PCC(energy_perf_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, energy_perf_reg, &energy_perf);
+                     perf_caps->energy_perf = energy_perf;

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1413

Quoted 8 lines

drivers/acpi/cppc_acpi.c +8 -0

+             } else {
+                     ret = -EIO;
+             }
+
+             up_write(&pcc_ss_data->pcc_lock);
+
+             return ret;
+     }

Quoted 5 lines

drivers/acpi/cppc_acpi.c +5 -0

+
+     return 0;
+}
+EXPORT_SYMBOL_GPL(cppc_get_epp_caps);
+

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1428

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+int cppc_set_auto_epp(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_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 30 lines

drivers/acpi/cppc_acpi.c +30 -0

+             return -EINVAL;
+     }
+
+     auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
+
+     if (CPC_IN_PCC(auto_sel_reg)) {
+             if (pcc_ss_id < 0)
+                     return -ENODEV;
+
+             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);
+             return ret;
+     }
+
+     return cpc_write(cpu, auto_sel_reg, enable);
+}
+EXPORT_SYMBOL_GPL(cppc_set_auto_epp);
+
+/*
+ * Set Energy Performance Preference Register value through
+ * Performance Controls Interface
+ */

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1468

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+{
+     int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+     struct cpc_register_resource *epp_set_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_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 15 lines

drivers/acpi/cppc_acpi.c +15 -0

+             return -EINVAL;
+     }
+
+     epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+     if (CPC_IN_PCC(epp_set_reg)) {
+             if (pcc_ss_id < 0)
+                     return -ENODEV;
+
+             ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
+             if (ret)
+                     return ret;
+
+             pcc_ss_data = pcc_data[pcc_ss_id];
+

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1447

Quoted 4 lines

drivers/acpi/cppc_acpi.c +4 -0

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

cppc_set_auto_epp and cppc_set_epp_perf have nearly the same code in the if block. I wonder if it's worth having a static helper function for this purpose that takes "reg" and "value" as arguments?

Quoted 1 line

drivers/acpi/cppc_acpi.c +1 -0

+     }

Quoted 56 lines

drivers/acpi/cppc_acpi.c +5 -0

+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
+
  /**
   * cppc_set_enable - Set to enable CPPC on the processor by writing the
   * Continuous Performance Control package EnableRegister field.

include/acpi/cppc_acpi.h +17 -0

index c5614444031f..10d91aeedaca 100644
@@ -108,12 +108,14 @@ struct cppc_perf_caps {
      u32 lowest_nonlinear_perf;
      u32 lowest_freq;
      u32 nominal_freq;
+     u32 energy_perf;
  };
 
  struct cppc_perf_ctrls {
      u32 max_perf;
      u32 min_perf;
      u32 desired_perf;
+     u32 energy_perf;
  };
 
  struct cppc_perf_fb_ctrs {

@@ -149,6 +151,9 @@ extern bool cpc_ffh_supported(void);
  extern bool cpc_supported_by_cpu(void);
  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_set_auto_epp(int cpu, bool enable);
+extern int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps);
+extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
  #else /* !CONFIG_ACPI_CPPC_LIB */
  static inline int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
  {

@@ -202,6 +207,18 @@ static inline int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
  {
      return -ENOTSUPP;
  }
+static inline int cppc_set_auto_epp(int cpu, bool enable)
+{
+     return -ENOTSUPP;
+}
+static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+{
+     return -ENOTSUPP;
+}
+static inline int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+{
+     return -ENOTSUPP;
+}
  #endif /* !CONFIG_ACPI_CPPC_LIB */
 
  #endif /* _CPPC_ACPI_H*/

-----Original Message-----
From: Rafael J. Wysocki <[email protected]>
Sent: Thursday, November 10, 2022 10:50 PM
To: Limonciello, Mario <[email protected]>; Yuan, Perry <[email protected]>
Cc: [email protected]; Huang, Ray <[email protected]>; [email protected]; Sharma, Deepak <[email protected]>; Fontenot, Nathan <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 1/8] ACPI: CPPC: Add AMD pstate energy performance preference cppc control

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

Quoted 16 lines · Mario

Quoted 3 lines · Perry Yuan

Add the EPP(Energy Performance Preference) support for the AMD SoCs without the dedicated CPPC MSR, those SoCs need to add this cppc acpi functions to update EPP values and desired perf value.

As far as I can tell this is generic code. Although the reason you're submitting it is for enabling AMD SoCs, the commit message should be worded as such.

Quoted 5 lines

In order to get EPP worked, cppc_get_epp_caps() will query EPP preference value and cppc_set_epp_perf() will set EPP new value. Before the EPP works, pstate driver will use cppc_set_auto_epp() to enable EPP function from firmware firstly.

Quoted 10 lines

This could more succinctly say:

"Add support for setting and querying EPP preferences to the generic CPPC driver. This enables downstream drivers such as amd-pstate to discover and use these values."

Quoted 4 lines

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/acpi/cppc_acpi.c | 126

+++++++++++++++++++++++++++++++++++++++

Quoted 8 lines

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct

Quoted 8 lines

  include/acpi/cppc_acpi.h |  17 ++++++
  2 files changed, 143 insertions(+)

drivers/acpi/cppc_acpi.c +0 -0

index 093675b1a1ff..d9c38dee1f48 100644
@@ -1365,6 +1365,132 @@ int cppc_get_perf_ctrs(int cpunum, struct

cppc_perf_fb_ctrs *perf_fb_ctrs)

Quoted 28 lines

Quoted 15 lines

drivers/acpi/cppc_acpi.c +12 -0

  }
  EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
 
+/**
+ * cppc_get_epp_caps - Get the energy preference register value.
+ * @cpunum: CPU from which to get epp preference level.
+ * @perf_caps: Return address.
+ *
+ * Return: 0 for success, -EIO otherwise.
+ */
+int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+{
+     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+     struct cpc_register_resource *energy_perf_reg;
+     u64 energy_perf;

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1389

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+
+     if (!cpc_desc) {
+             pr_warn("No CPC descriptor for CPU:%d\n", cpunum);
+             return -ENODEV;
+     }
+
+     energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+     if (!CPC_SUPPORTED(energy_perf_reg))
+             pr_warn("energy perf reg update is unsupported!\n");

No need to add a explanation point at the end.

Quoted 3 lines

As this is a per-CPU message I wonder if this would be better as pr_warn_once()? Othewrise some systems with large numbers of cores might potentially show this message quite a few times.

pr_info_once() would suffice IMO.

Quoted 17 lines

Quoted 17 lines

drivers/acpi/cppc_acpi.c +16 -0

+
+     if (CPC_IN_PCC(energy_perf_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, energy_perf_reg, &energy_perf);
+                     perf_caps->energy_perf = energy_perf;

Quoted 8 lines

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1413

Quoted 8 lines

drivers/acpi/cppc_acpi.c +8 -0

+             } else {
+                     ret = -EIO;
+             }
+
+             up_write(&pcc_ss_data->pcc_lock);
+
+             return ret;
+     }

What if CPC is not in PCC?

Would returning 0 then work for all users?

Quoted 86 lines

Quoted 5 lines

drivers/acpi/cppc_acpi.c +4 -0

+
+     return 0;
+}
+EXPORT_SYMBOL_GPL(cppc_get_epp_caps);

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1428

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+
+int cppc_set_auto_epp(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_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 29 lines

drivers/acpi/cppc_acpi.c +29 -0

+             return -EINVAL;
+     }
+
+     auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
+
+     if (CPC_IN_PCC(auto_sel_reg)) {
+             if (pcc_ss_id < 0)
+                     return -ENODEV;
+
+             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);
+             return ret;
+     }
+
+     return cpc_write(cpu, auto_sel_reg, enable); }
+EXPORT_SYMBOL_GPL(cppc_set_auto_epp);
+
+/*
+ * Set Energy Performance Preference Register value through
+ * Performance Controls Interface
+ */

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1468

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+{
+     int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+     struct cpc_register_resource *epp_set_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_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 9 lines

drivers/acpi/cppc_acpi.c +9 -0

+             return -EINVAL;
+     }
+
+     epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+     if (CPC_IN_PCC(epp_set_reg)) {
+             if (pcc_ss_id < 0)
+                     return -ENODEV;
+

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1487

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+             ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
+             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);

cppc_set_auto_epp and cppc_set_epp_perf have nearly the same code in the if block. I wonder if it's worth having a static helper function for this purpose that takes "reg" and "value" as arguments?

Quoted 1 line

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct · line 1486

Quoted 1 line

drivers/acpi/cppc_acpi.c +1 -0

+     }

And what about the non-PCC case here?

Quoted 27 lines

Quoted 27 lines

drivers/acpi/cppc_acpi.c +5 -0

+
+     return ret;
+}
+EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
+
  /**
   * cppc_set_enable - Set to enable CPPC on the processor by writing the
   * Continuous Performance Control package EnableRegister field.

include/acpi/cppc_acpi.h +2 -0

index c5614444031f..10d91aeedaca 100644
@@ -108,12 +108,14 @@ struct cppc_perf_caps {
      u32 lowest_nonlinear_perf;
      u32 lowest_freq;
      u32 nominal_freq;
+     u32 energy_perf;
  };
 
  struct cppc_perf_ctrls {
      u32 max_perf;
      u32 min_perf;
      u32 desired_perf;
+     u32 energy_perf;
  };
 

Quoted 14 lines

include/acpi/cppc_acpi.h static inline int cpc_write_ffh(int cpunum, struct

Quoted 14 lines

include/acpi/cppc_acpi.h +4 -0

  struct cppc_perf_fb_ctrs {

@@ -149,6 +151,9 @@ extern bool cpc_ffh_supported(void);
  extern bool cpc_supported_by_cpu(void);
  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_set_auto_epp(int cpu, bool enable); extern int
+cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps);
+extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls
+*perf_ctrls);
  #else /* !CONFIG_ACPI_CPPC_LIB */
  static inline int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
  {

@@ -202,6 +207,18 @@ static inline int cpc_write_ffh(int cpunum, struct

cpc_reg *reg, u64 val)

Quoted 18 lines

Quoted 17 lines

include/acpi/cppc_acpi.h +11 -0

  {
      return -ENOTSUPP;
  }
+static inline int cppc_set_auto_epp(int cpu, bool enable) {
+     return -ENOTSUPP;
+}
+static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls
+*perf_ctrls) {
+     return -ENOTSUPP;
+}
+static inline int cppc_get_epp_caps(int cpunum, struct
+cppc_perf_caps *perf_caps) {
+     return -ENOTSUPP;
+}
  #endif /* !CONFIG_ACPI_CPPC_LIB */
 
  #endif /* _CPPC_ACPI_H*/

[AMD Official Use Only - General]

Quoted 85 lines

-----Original Message-----
From: Rafael J. Wysocki <[email protected]>
Sent: Thursday, November 10, 2022 10:50 PM
To: Limonciello, Mario <[email protected]>; Yuan, Perry <[email protected]>
Cc: [email protected]; Huang, Ray <[email protected]>; [email protected]; Sharma, Deepak <[email protected]>; Fontenot, Nathan <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 1/8] ACPI: CPPC: Add AMD pstate energy performance preference cppc control

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

Quoted 16 lines · Mario

Quoted 3 lines · Perry Yuan

Add the EPP(Energy Performance Preference) support for the AMD SoCs without the dedicated CPPC MSR, those SoCs need to add this cppc acpi functions to update EPP values and desired perf value.

As far as I can tell this is generic code. Although the reason you're submitting it is for enabling AMD SoCs, the commit message should be worded as such.

Quoted 5 lines

In order to get EPP worked, cppc_get_epp_caps() will query EPP preference value and cppc_set_epp_perf() will set EPP new value. Before the EPP works, pstate driver will use cppc_set_auto_epp() to enable EPP function from firmware firstly.

Quoted 10 lines

This could more succinctly say:

"Add support for setting and querying EPP preferences to the generic CPPC driver. This enables downstream drivers such as amd-pstate to discover and use these values."

Quoted 4 lines

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/acpi/cppc_acpi.c | 126

+++++++++++++++++++++++++++++++++++++++

Quoted 8 lines

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct

Quoted 8 lines

  include/acpi/cppc_acpi.h |  17 ++++++
  2 files changed, 143 insertions(+)

drivers/acpi/cppc_acpi.c +0 -0

index 093675b1a1ff..d9c38dee1f48 100644
@@ -1365,6 +1365,132 @@ int cppc_get_perf_ctrs(int cpunum, struct

cppc_perf_fb_ctrs *perf_fb_ctrs)

Quoted 28 lines

Quoted 15 lines

drivers/acpi/cppc_acpi.c +12 -0

  }
  EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
 
+/**
+ * cppc_get_epp_caps - Get the energy preference register value.
+ * @cpunum: CPU from which to get epp preference level.
+ * @perf_caps: Return address.
+ *
+ * Return: 0 for success, -EIO otherwise.
+ */
+int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
+{
+     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+     struct cpc_register_resource *energy_perf_reg;
+     u64 energy_perf;

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1389

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+
+     if (!cpc_desc) {
+             pr_warn("No CPC descriptor for CPU:%d\n", cpunum);
+             return -ENODEV;
+     }
+
+     energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+     if (!CPC_SUPPORTED(energy_perf_reg))
+             pr_warn("energy perf reg update is unsupported!\n");

No need to add a explanation point at the end.

Quoted 5 lines

Quoted 3 lines

As this is a per-CPU message I wonder if this would be better as pr_warn_once()? Othewrise some systems with large numbers of cores might potentially show this message quite a few times.

pr_info_once() would suffice IMO.

Fixed in V4.

Quoted 28 lines

Quoted 17 lines

Quoted 17 lines

drivers/acpi/cppc_acpi.c +16 -0

+
+     if (CPC_IN_PCC(energy_perf_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, energy_perf_reg, &energy_perf);
+                     perf_caps->energy_perf = energy_perf;

Quoted 8 lines

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1413

Quoted 8 lines

drivers/acpi/cppc_acpi.c +8 -0

+             } else {
+                     ret = -EIO;
+             }
+
+             up_write(&pcc_ss_data->pcc_lock);
+
+             return ret;
+     }

What if CPC is not in PCC?

Quoted 1 line

Would returning 0 then work for all users?

Fixed in V4

Quoted 86 lines

Quoted 86 lines

Quoted 5 lines

drivers/acpi/cppc_acpi.c +4 -0

+
+     return 0;
+}
+EXPORT_SYMBOL_GPL(cppc_get_epp_caps);

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1428

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+
+int cppc_set_auto_epp(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_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 29 lines

drivers/acpi/cppc_acpi.c +29 -0

+             return -EINVAL;
+     }
+
+     auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
+
+     if (CPC_IN_PCC(auto_sel_reg)) {
+             if (pcc_ss_id < 0)
+                     return -ENODEV;
+
+             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);
+             return ret;
+     }
+
+     return cpc_write(cpu, auto_sel_reg, enable); }
+EXPORT_SYMBOL_GPL(cppc_set_auto_epp);
+
+/*
+ * Set Energy Performance Preference Register value through
+ * Performance Controls Interface
+ */

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1468

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+{
+     int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+     struct cpc_register_resource *epp_set_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_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 9 lines

drivers/acpi/cppc_acpi.c +9 -0

+             return -EINVAL;
+     }
+
+     epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+     if (CPC_IN_PCC(epp_set_reg)) {
+             if (pcc_ss_id < 0)
+                     return -ENODEV;
+

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1487

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+             ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
+             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);

cppc_set_auto_epp and cppc_set_epp_perf have nearly the same code in the if block. I wonder if it's worth having a static helper function for this purpose that takes "reg" and "value" as arguments?

Quoted 3 lines

Quoted 1 line

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct · line 1486

Quoted 1 line

drivers/acpi/cppc_acpi.c +1 -0

+     }

And what about the non-PCC case here?

I merge the cppc_set_auto_epp and cppc_set_epp_perf in V4. For the non-PCC case, we canno set the EPP value to FW, then just returned Error code. Is it Ok ?

-----Original Message-----
From: Rafael J. Wysocki <[email protected]>
Sent: Thursday, November 10, 2022 11:56 PM
To: Yuan, Perry <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>; Limonciello, Mario <[email protected]>; [email protected]; Huang, Ray <[email protected]>; [email protected]; Sharma, Deepak <[email protected]>; Fontenot, Nathan <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 1/8] ACPI: CPPC: Add AMD pstate energy performance preference cppc control

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

Quoted 14 lines · Perry

[AMD Official Use Only - General]

Quoted 9 lines

-----Original Message-----
From: Rafael J. Wysocki <[email protected]>
Sent: Thursday, November 10, 2022 10:50 PM
To: Limonciello, Mario <[email protected]>; Yuan, Perry <[email protected]>
Cc: [email protected]; Huang, Ray <[email protected]>; [email protected]; Sharma, Deepak <[email protected]>; Fontenot, Nathan <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer

<[email protected]>;

Quoted 146 lines

Quoted 78 lines

Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 1/8] ACPI: CPPC: Add AMD pstate energy performance preference cppc control

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

Quoted 16 lines · Mario

Quoted 3 lines · Perry Yuan

Add the EPP(Energy Performance Preference) support for the AMD SoCs without the dedicated CPPC MSR, those SoCs need to add this cppc acpi functions to update EPP values and desired perf value.

As far as I can tell this is generic code. Although the reason you're submitting it is for enabling AMD SoCs, the commit message should be worded as such.

Quoted 5 lines

In order to get EPP worked, cppc_get_epp_caps() will query EPP preference value and cppc_set_epp_perf() will set EPP new value. Before the EPP works, pstate driver will use cppc_set_auto_epp() to enable EPP function from firmware firstly.

Quoted 10 lines

This could more succinctly say:

"Add support for setting and querying EPP preferences to the generic CPPC driver. This enables downstream drivers such as amd-pstate to discover and use these values."

Quoted 4 lines

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/acpi/cppc_acpi.c | 126

+++++++++++++++++++++++++++++++++++++++

Quoted 9 lines

Quoted 9 lines

  include/acpi/cppc_acpi.h |  17 ++++++
  2 files changed, 143 insertions(+)

drivers/acpi/cppc_acpi.c +0 -0

index 093675b1a1ff..d9c38dee1f48 100644
@@ -1365,6 +1365,132 @@ int cppc_get_perf_ctrs(int cpunum,

struct

cppc_perf_fb_ctrs *perf_fb_ctrs)

Quoted 29 lines

Quoted 16 lines

  }
  EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);

drivers/acpi/cppc_acpi.c +13 -0

+/**
+ * cppc_get_epp_caps - Get the energy preference register value.
+ * @cpunum: CPU from which to get epp preference level.
+ * @perf_caps: Return address.
+ *
+ * Return: 0 for success, -EIO otherwise.
+ */
+int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps
+*perf_caps) {
+     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+     struct cpc_register_resource *energy_perf_reg;
+     u64 energy_perf;
+

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+     if (!cpc_desc) {
+             pr_warn("No CPC descriptor for CPU:%d\n", cpunum);
+             return -ENODEV;
+     }
+
+     energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+     if (!CPC_SUPPORTED(energy_perf_reg))
+             pr_warn("energy perf reg update is
+ unsupported!\n");

No need to add a explanation point at the end.

Quoted 5 lines

Quoted 3 lines

As this is a per-CPU message I wonder if this would be better as pr_warn_once()? Othewrise some systems with large numbers of cores might potentially show this message quite a few times.

pr_info_once() would suffice IMO.

Fixed in V4.

Quoted 28 lines

Quoted 17 lines

Quoted 17 lines

drivers/acpi/cppc_acpi.c +16 -0

+
+     if (CPC_IN_PCC(energy_perf_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, energy_perf_reg, &energy_perf);
+                     perf_caps->energy_perf = energy_perf;

Quoted 8 lines

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1413

Quoted 8 lines

drivers/acpi/cppc_acpi.c +8 -0

+             } else {
+                     ret = -EIO;
+             }
+
+             up_write(&pcc_ss_data->pcc_lock);
+
+             return ret;
+     }

What if CPC is not in PCC?

Quoted 1 line

Would returning 0 then work for all users?

Fixed in V4

Quoted 28 lines

Quoted 28 lines

Quoted 5 lines

drivers/acpi/cppc_acpi.c +4 -0

+
+     return 0;
+}
+EXPORT_SYMBOL_GPL(cppc_get_epp_caps);

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1428

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+
+int cppc_set_auto_epp(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_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 9 lines

drivers/acpi/cppc_acpi.c +9 -0

+             return -EINVAL;
+     }
+
+     auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
+
+     if (CPC_IN_PCC(auto_sel_reg)) {
+             if (pcc_ss_id < 0)
+                     return -ENODEV;
+

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)

Quoted 8 lines

drivers/acpi/cppc_acpi.c +8 -0

+             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 */

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+             ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
+             up_write(&pcc_ss_data->pcc_lock);
+             return ret;
+     }
+
+     return cpc_write(cpu, auto_sel_reg, enable); }
+EXPORT_SYMBOL_GPL(cppc_set_auto_epp);
+
+/*
+ * Set Energy Performance Preference Register value through

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1468

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+ * Performance Controls Interface  */ int cppc_set_epp_perf(int
+cpu, struct cppc_perf_ctrls *perf_ctrls) {
+     int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+     struct cpc_register_resource *epp_set_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_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 9 lines

drivers/acpi/cppc_acpi.c +9 -0

+             return -EINVAL;
+     }
+
+     epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+     if (CPC_IN_PCC(epp_set_reg)) {
+             if (pcc_ss_id < 0)
+                     return -ENODEV;
+

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)

Quoted 8 lines

drivers/acpi/cppc_acpi.c +8 -0

+             ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
+             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 */

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/acpi/cppc_acpi.c

Quoted 2 lines

drivers/acpi/cppc_acpi.c +2 -0

+             ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
+             up_write(&pcc_ss_data->pcc_lock);

cppc_set_auto_epp and cppc_set_epp_perf have nearly the same

code

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

in the if block. I wonder if it's worth having a static helper function for this purpose that takes "reg" and "value" as arguments?

Quoted 1 line

+ }

Quoted 5 lines

Quoted 1 line

And what about the non-PCC case here?

I merge the cppc_set_auto_epp and cppc_set_epp_perf in V4. For the non-PCC case, we canno set the EPP value to FW, then just returned Error code. Is it Ok ?

Yes, if it cannot be updated, it should be treated the same way as unsupported IMV.

-----Original Message-----
From: Limonciello, Mario <[email protected]>
Sent: Tuesday, November 8, 2022 2:45 AM
To: Yuan, Perry <[email protected]>; [email protected]; Huang, Ray <[email protected]>; [email protected]
Cc: Sharma, Deepak <[email protected]>; Fontenot, Nathan <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 1/8] ACPI: CPPC: Add AMD pstate energy performance preference cppc control

Quoted 3 lines · Perry Yuan

Add the EPP(Energy Performance Preference) support for the AMD SoCs without the dedicated CPPC MSR, those SoCs need to add this cppc acpi functions to update EPP values and desired perf value.

As far as I can tell this is generic code. Although the reason you're submitting it is for enabling AMD SoCs, the commit message should be worded as such.

Quoted 5 lines

In order to get EPP worked, cppc_get_epp_caps() will query EPP preference value and cppc_set_epp_perf() will set EPP new value. Before the EPP works, pstate driver will use cppc_set_auto_epp() to enable EPP function from firmware firstly.

This could more succinctly say:

"Add support for setting and querying EPP preferences to the generic CPPC driver. This enables downstream drivers such as amd-pstate to discover and use these values."

Quoted 4 lines

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/acpi/cppc_acpi.c | 126

+++++++++++++++++++++++++++++++++++++++

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct

Quoted 8 lines

  include/acpi/cppc_acpi.h |  17 ++++++
  2 files changed, 143 insertions(+)

drivers/acpi/cppc_acpi.c index renamed from drivers/acpi/cppc_acpi.c +0 -0

093675b1a1ff..d9c38dee1f48 100644

drivers/acpi/cppc_acpi.c +0 -0

@@ -1365,6 +1365,132 @@ int cppc_get_perf_ctrs(int cpunum, struct

cppc_perf_fb_ctrs *perf_fb_ctrs)

Quoted 14 lines

drivers/acpi/cppc_acpi.c +11 -0

  }
  EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
 
+/**
+ * cppc_get_epp_caps - Get the energy preference register value.
+ * @cpunum: CPU from which to get epp preference level.
+ * @perf_caps: Return address.
+ *
+ * Return: 0 for success, -EIO otherwise.
+ */
+int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps) {
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+	struct cpc_register_resource *energy_perf_reg;
+	u64 energy_perf;

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1389

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+
+	if (!cpc_desc) {
+		pr_warn("No CPC descriptor for CPU:%d\n", cpunum);
+		return -ENODEV;
+	}
+
+	energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+	if (!CPC_SUPPORTED(energy_perf_reg))
+		pr_warn("energy perf reg update is unsupported!\n");

No need to add a explanation point at the end.

As this is a per-CPU message I wonder if this would be better as pr_warn_once()? Othewrise some systems with large numbers of cores might potentially show this message quite a few times.

Quoted 34 lines

drivers/acpi/cppc_acpi.c +33 -0

+
+	if (CPC_IN_PCC(energy_perf_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, energy_perf_reg, &energy_perf);
+			perf_caps->energy_perf = energy_perf;
+		} else {
+			ret = -EIO;
+		}
+
+		up_write(&pcc_ss_data->pcc_lock);
+
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cppc_get_epp_caps);
+
+int cppc_set_auto_epp(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);

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1428

Quoted 5 lines

drivers/acpi/cppc_acpi.c +5 -0

+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret = -EINVAL;
+
+	if (!cpc_desc) {
+		pr_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 7 lines

drivers/acpi/cppc_acpi.c +7 -0

+		return -EINVAL;
+	}
+
+	auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
+
+	if (CPC_IN_PCC(auto_sel_reg)) {
+		if (pcc_ss_id < 0)

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+			return -ENODEV;
+
+		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 */

Quoted 16 lines

drivers/acpi/cppc_acpi.c +16 -0

+		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
+		up_write(&pcc_ss_data->pcc_lock);
+		return ret;
+	}
+
+	return cpc_write(cpu, auto_sel_reg, enable); }
+EXPORT_SYMBOL_GPL(cppc_set_auto_epp);
+
+/*
+ * Set Energy Performance Preference Register value through
+ * Performance Controls Interface
+ */
+int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls) {
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cpc_register_resource *epp_set_reg;
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) · line 1428

Quoted 5 lines

drivers/acpi/cppc_acpi.c +5 -0

+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret = -EINVAL;
+
+	if (!cpc_desc) {
+		pr_warn("No CPC descriptor for CPU:%d\n", cpu);

Is this actually warn worthy? I would think it's fine a debug like we have for the other _CPC missing messages.

Quoted 8 lines

drivers/acpi/cppc_acpi.c +7 -0

+		return -EINVAL;
+	}
+
+	epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+	if (CPC_IN_PCC(epp_set_reg)) {
+		if (pcc_ss_id < 0)

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)

Quoted 10 lines

drivers/acpi/cppc_acpi.c +10 -0

+			return -ENODEV;
+
+		ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
+		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 */

drivers/acpi/cppc_acpi.c int cppc_get_perf_ctrs(int cpunum, struct · line 1483

Quoted 2 lines

drivers/acpi/cppc_acpi.c +2 -0

+		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
+		up_write(&pcc_ss_data->pcc_lock);

cppc_set_auto_epp and cppc_set_epp_perf have nearly the same code in the if block. I wonder if it's worth having a static helper function for this purpose that takes "reg" and "value" as arguments?

include/acpi/cppc_acpi.h static inline int cpc_write_ffh(int cpunum, struct

Quoted 41 lines

drivers/acpi/cppc_acpi.c +6 -0

+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
+
  /**
   * cppc_set_enable - Set to enable CPPC on the processor by writing the
   * Continuous Performance Control package EnableRegister field.

include/acpi/cppc_acpi.h index renamed from include/acpi/cppc_acpi.h +0 -0

c5614444031f..10d91aeedaca 100644

include/acpi/cppc_acpi.h +6 -0

@@ -108,12 +108,14 @@ struct cppc_perf_caps {
  	u32 lowest_nonlinear_perf;
  	u32 lowest_freq;
  	u32 nominal_freq;
+	u32 energy_perf;
  };
 
  struct cppc_perf_ctrls {
  	u32 max_perf;
  	u32 min_perf;
  	u32 desired_perf;
+	u32 energy_perf;
  };
 
  struct cppc_perf_fb_ctrs {

@@ -149,6 +151,9 @@ extern bool cpc_ffh_supported(void);
  extern bool cpc_supported_by_cpu(void);
  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_set_auto_epp(int cpu, bool enable); extern int
+cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps);
+extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls
+*perf_ctrls);
  #else /* !CONFIG_ACPI_CPPC_LIB */
  static inline int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
  {

@@ -202,6 +207,18 @@ static inline int cpc_write_ffh(int cpunum, struct

cpc_reg *reg, u64 val)

Quoted 17 lines

include/acpi/cppc_acpi.h +11 -0

  {
  	return -ENOTSUPP;
  }
+static inline int cppc_set_auto_epp(int cpu, bool enable) {
+	return -ENOTSUPP;
+}
+static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls
+*perf_ctrls) {
+	return -ENOTSUPP;
+}
+static inline int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps
+*perf_caps) {
+	return -ENOTSUPP;
+}
  #endif /* !CONFIG_ACPI_CPPC_LIB */
 
  #endif /* _CPPC_ACPI_H*/

The patch add AMD pstate EPP feature introduction and what EPP preference supported for AMD processors.

User can get supported list from energy_performance_available_preferences attribute file, or update current profile to energy_performance_preference file

1) See all EPP profiles

$ sudo cat /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_available_preferences
default performance balance_performance balance_power power

2) Check current EPP profile

$ sudo cat /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference
performance

3) Set new EPP profile

$ sudo bash -c "echo power > /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference"

Signed-off-by Perry Yuan <[email protected]>

---
  Documentation/admin-guide/pm/amd-pstate.rst | 19 +++++++++++++++++++
  1 file changed, 19 insertions(+)

Documentation/admin-guide/pm/amd-pstate.rst +0 -0

index 8f3d30c5a0d8..78c6525d5a49 100644

Documentation/admin-guide/pm/amd-pstate.rst +11 -0

+
+``energy_performance_preference``
+
+The current energy performance preference can be read from this attribute.
+and user can change current preference according to energy or performance needs
+Please get all support profiles list from
+``energy_performance_available_preferences`` attribute, all the profiles are
+integer values defined between 0 to 255 when EPP feature is enabled by platform
+firmware, if EPP feature is disabled, driver will ignore the written value
+This attribute is read-write.
+
  Other performance and frequency values can be read back from
  ``/sys/devices/system/cpu/cpuX/acpi_cppc/``, see :ref:`cppc_sysfs`.
 

-----Original Message-----
From: Limonciello, Mario <[email protected]>
Sent: Tuesday, November 8, 2022 2:34 AM
To: Yuan, Perry <[email protected]>; [email protected]; Huang, Ray <[email protected]>; [email protected]
Cc: Sharma, Deepak <[email protected]>; Fontenot, Nathan <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 2/8] Documentation: amd-pstate: add EPP profiles introduction

Quoted 10 lines · Perry Yuan

The patch add AMD pstate EPP feature introduction and what EPP preference supported for AMD processors.

User can get supported list from energy_performance_available_preferences attribute file, or update current profile to energy_performance_preference file

1) See all EPP profiles

$ sudo cat

/sys/devices/system/cpu/cpu0/cpufreq/energy_performance_available_pref

Quoted 9 lines

erences default performance balance_performance balance_power power

2) Check current EPP profile

$ sudo cat
/sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference
performance

3) Set new EPP profile

$ sudo bash -c "echo power >

/sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference"

Quoted 4 lines

Signed-off-by Perry Yuan <[email protected]>

---
  Documentation/admin-guide/pm/amd-pstate.rst | 19

+++++++++++++++++++

Documentation/admin-guide/pm/amd-pstate.rst lowest non-linear performance in `AMD CPPC

Quoted 8 lines

  1 file changed, 19 insertions(+)

Documentation/admin-guide/pm/amd-pstate.rst +0 -0

index 8f3d30c5a0d8..78c6525d5a49 100644
@@ -262,6 +262,25 @@ lowest non-linear performance in `AMD CPPC

Performance Capability

Documentation/admin-guide/pm/amd-pstate.rst lowest non-linear performance in `AMD CPPC · line 271

Quoted 10 lines

Documentation/admin-guide/pm/amd-pstate.rst +7 -0

  <perf_cap_>`_.)
  This attribute is read-only.
  > +``energy_performance_available_preferences``
+
+All the supported EPP preference could be selected, List of the
+strings that can be set to the ``energy_performance_preference``
+attribute those different profiles represent different energy vs
+efficiency hints provided to low-level firmware however, the
+``default`` represents the epp value is set by platform firmware This
+attribute is read-only.

My proposal at rewording it.

A list of all the supported EPP preferences that could be used for ``energy_performance_preference` on this system.
These profiles represent different hints that are provided to the low-level firmware about the user's desired energy vs efficiency tradeoff. ``default`` represents the epp value is set by platform firmware. This attribute is read- only.

Documentation/admin-guide/pm/amd-pstate.rst lowest non-linear performance in `AMD CPPC Performance Capability

Quoted 6 lines

Documentation/admin-guide/pm/amd-pstate.rst +4 -0

+
+``energy_performance_preference``
+
+The current energy performance preference can be read from this

attribute.

Quoted 10 lines

Documentation/admin-guide/pm/amd-pstate.rst +7 -0

+and user can change current preference according to energy or
+performance needs Please get all support profiles list from
+``energy_performance_available_preferences`` attribute, all the
+profiles are integer values defined between 0 to 255 when EPP feature
+is enabled by platform firmware, if EPP feature is disabled, driver
+will ignore the written value This attribute is read-write.
+
  Other performance and frequency values can be read back from
  ``/sys/devices/system/cpu/cpuX/acpi_cppc/``, see :ref:`cppc_sysfs`.
 

The patch add AMD pstate EPP feature introduction and what EPP preference supported for AMD processors.

User can get supported list from energy_performance_available_preferences attribute file, or update current profile to energy_performance_preference file

1) See all EPP profiles

$ sudo cat /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_available_preferences
default performance balance_performance balance_power power

2) Check current EPP profile

$ sudo cat /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference
performance

3) Set new EPP profile

$ sudo bash -c "echo power > /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference"

Signed-off-by Perry Yuan <[email protected]>

---
 Documentation/admin-guide/pm/amd-pstate.rst | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Documentation/admin-guide/pm/amd-pstate.rst +13 -0

index 8f3d30c5a0d8..78c6525d5a49 100644
@@ -262,6 +262,25 @@ lowest non-linear performance in `AMD CPPC Performance Capability
 <perf_cap_>`_.)
 This attribute is read-only.
 
+``energy_performance_available_preferences``
+
+All the supported EPP preference could be selected, List of the strings that
+can be set to the ``energy_performance_preference`` attribute
+those different profiles represent different energy vs efficiency hints provided
+to low-level firmware
+however, the ``default`` represents the epp value is set by platform firmware
+This attribute is read-only.
+
+``energy_performance_preference``
+
+The current energy performance preference can be read from this attribute.
+and user can change current preference according to energy or performance needs

-----Original Message-----
From: Rafael J. Wysocki <[email protected]>
Sent: Thursday, November 10, 2022 10:58 PM
To: Yuan, Perry <[email protected]>
Cc: [email protected]; Huang, Ray <[email protected]>; [email protected]; Sharma, Deepak <[email protected]>; Limonciello, Mario <[email protected]>; Fontenot, Nathan <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 2/8] Documentation: amd-pstate: add EPP profiles introduction

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

Quoted 11 lines · Perry Yuan

The patch add AMD pstate EPP feature introduction and what EPP preference supported for AMD processors.

User can get supported list from energy_performance_available_preferences attribute file, or update current profile to energy_performance_preference file

1) See all EPP profiles

$ sudo cat

/sys/devices/system/cpu/cpu0/cpufreq/energy_performance_available_pr ef

Quoted 9 lines

erences default performance balance_performance balance_power power

2) Check current EPP profile

$ sudo cat
/sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference
performance

3) Set new EPP profile

$ sudo bash -c "echo power >

/sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference"

Quoted 4 lines

Signed-off-by Perry Yuan <[email protected]>

---
 Documentation/admin-guide/pm/amd-pstate.rst | 19

+++++++++++++++++++

Quoted 14 lines

 1 file changed, 19 insertions(+)

Documentation/admin-guide/pm/amd-pstate.rst +0 -0

index 8f3d30c5a0d8..78c6525d5a49 100644
@@ -262,6 +262,25 @@ lowest non-linear performance in `AMD CPPC

Performance Capability
<perf_cap_>`_.)
This attribute is read-only.

+``energy_performance_available_preferences``

Documentation/admin-guide/pm/amd-pstate.rst +1 -0

+

Documentation/admin-guide/pm/amd-pstate.rst

Quoted 10 lines

Documentation/admin-guide/pm/amd-pstate.rst +10 -0

+All the supported EPP preference could be selected, List of the
+strings that can be set to the ``energy_performance_preference``
+attribute those different profiles represent different energy vs
+efficiency hints provided to low-level firmware however, the
+``default`` represents the epp value is set by platform firmware This
+attribute is read-only.
+
+``energy_performance_preference``
+
+The current energy performance preference can be read from this

attribute.

Quoted 11 lines

Documentation/admin-guide/pm/amd-pstate.rst +7 -0

+and user can change current preference according to energy or
+performance needs Please get all support profiles list from
+``energy_performance_available_preferences`` attribute, all the
+profiles are integer values defined between 0 to 255 when EPP feature
+is enabled by platform firmware, if EPP feature is disabled, driver
+will ignore the written value This attribute is read-write.
+
 Other performance and frequency values can be read back from

``/sys/devices/system/cpu/cpuX/acpi_cppc/``, see :ref:cppc_sysfs.

--

So the interface is the same as for intel_pstate AFAICS, which is good, because it is all consistent.

However, there is some code duplication introduced in the subsequent patches and that may be a problem from the perspective of maintaining that consistency in the future.

Have you at least considered introducing a common EPP support header file that could be used by both intel_pstate and amd_pstate?

Change the amd-pstate driver as the built-in type which can help to load the driver before the acpi_cpufreq driver as the default pstate driver for the AMD processors.

for the processors do not have the dedicated MSR functions, add amd-pstate=legacy_cppc to grub which enable shared memmory interface to communicate with cppc_acpi module to control pstate hints.

Signed-off-by Perry Yuan <[email protected]>

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

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

index ace7d50cf2ac..14906431dc15 100644
@@ -59,10 +59,7 @@
   * we disable it by default to go acpi-cpufreq on these processors and add a
   * module parameter to be able to enable it manually for debugging.
   */
-static bool shared_mem = false;
-module_param(shared_mem, bool, 0444);
-MODULE_PARM_DESC(shared_mem,
-		 "enable amd-pstate on processors with shared memory solution (false = disabled (default), true = enabled)");
+static bool shared_mem __read_mostly;
 
  static struct cpufreq_driver amd_pstate_driver;
 

@@ -653,16 +650,22 @@ static int __init amd_pstate_init(void)
 
  	return ret;
  }
+device_initcall(amd_pstate_init);
 
-static void __exit amd_pstate_exit(void)

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

-module_init(amd_pstate_init);
-module_exit(amd_pstate_exit);
+	return 0;
+}
+early_param("amd-pstate", amd_pstate_param);
 
  MODULE_AUTHOR("Huang Rui <[email protected]>");
  MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");

-----Original Message-----
From: Limonciello, Mario <[email protected]>
Sent: Tuesday, November 8, 2022 2:30 AM
To: Yuan, Perry <[email protected]>; [email protected]; Huang, Ray <[email protected]>; [email protected]; Karny, Wyes <[email protected]>
Cc: Sharma, Deepak <[email protected]>; Fontenot, Nathan <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v3 3/8] cpufreq: amd-pstate: change driver to be built-in type

+ [email protected]

You should sync with Wyes Karny on this patch, I think he had some different ideas that you guys should fold together for v4 of this series. I'll leave some direct comments on your implementation below.

Also, include him in on CC for your v4.

Quoted 4 lines · Perry Yuan

Change the amd-pstate driver as the built-in type which can help to load the driver before the acpi_cpufreq driver as the default pstate driver for the AMD processors.

Quoted 3 lines

for the processors do not have the dedicated MSR functions, add amd-pstate=legacy_cppc to grub which enable shared memmory interface to communicate with cppc_acpi module to control pstate hints.

1) s/memmory/memory/
2) Although many users will use GRUB to configure their kernel command line you should not assume it in the commit message.

Quoted 6 lines

Signed-off-by Perry Yuan <[email protected]>

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

You need to document the new early parameter support in kernel- parameters.txt, and should also put it in amd-pstate.rst.

Quoted 3 lines

(quoted diff) +0 -0

b/drivers/cpufreq/amd-pstate.c index ace7d50cf2ac..14906431dc15

100644

drivers/cpufreq/amd-pstate.c line 59

Quoted 4 lines

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

@@ -59,10 +59,7 @@
   * we disable it by default to go acpi-cpufreq on these processors and add

a

drivers/cpufreq/amd-pstate.c

Quoted 4 lines

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

   * module parameter to be able to enable it manually for debugging.
   */
-static bool shared_mem = false;
-module_param(shared_mem, bool, 0444); -

MODULE_PARM_DESC(shared_mem,

drivers/cpufreq/amd-pstate.c line 64

Quoted 1 line

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

-		 "enable amd-pstate on processors with shared memory

solution (false = disabled (default), true = enabled)");

Quoted 10 lines

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

+static bool shared_mem __read_mostly;
 
  static struct cpufreq_driver amd_pstate_driver;
 

@@ -653,16 +650,22 @@ static int __init amd_pstate_init(void)
 
  	return ret;
  }
+device_initcall(amd_pstate_init);
 

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void) · line 660

Quoted 10 lines

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

-static void __exit amd_pstate_exit(void)
+static int __init amd_pstate_param(char *str)
  {
-	cpufreq_unregister_driver(&amd_pstate_driver);
+	if (!str)
+		return -EINVAL;
 
-	amd_pstate_enable(false);
-}
+	/* enable shared memory type CPPC ,if you processor has no MSR,

you have to add this

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void) · line 661

Quoted 1 line

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

+	 * to your grub to make cppc driver loaded successfully.

Don't reference GRUB here, it should be referenced from the kernel command line.

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void) · line 664

Quoted 3 lines

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

+	 */
+	if (!strcmp(str, "legacy_cppc"))
+		shared_mem = true;

Sync with Wyes about this. He had some different strings and flow in mind which I think would be more preferable.

Quoted 10 lines

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

-module_init(amd_pstate_init);
-module_exit(amd_pstate_exit);
+	return 0;
+}
+early_param("amd-pstate", amd_pstate_param);
 
  MODULE_AUTHOR("Huang Rui <[email protected]>");
  MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");

Add EPP driver support for those AMD CPUs which has full MSR feature enabled, The EPP is used in the DPM controller to drive the frequency that a core is going to operate during short periods of activity.

EPP values will be utilized for different OS profiles (balanced, performance, power savings). cppc performance can be controlled by the user space interface sys attributes for min and max frequency limits, when pstate driver is working under power save policy.

EPP scale is 0 - 255, 0 is the max performance and 255 is min level. balance_performance (0x80) can provide best balance performance and watt for most of system, meanwhile user can choose performance policy on needs.

$ cat /sys/devices/system/cpu/cpufreq/policy0/energy_performance_available_preferences
default performance balance_performance balance_power power

$ cat /sys/devices/system/cpu/cpufreq/policy0/energy_performance_preference
balance_performance

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/cpufreq/amd-pstate.c | 658 ++++++++++++++++++++++++++++++++++-
  include/linux/amd-pstate.h   |  81 +++++
  2 files changed, 734 insertions(+), 5 deletions(-)

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

index 14906431dc15..eb82bc6a7f66 100644
@@ -60,8 +60,136 @@
   * module parameter to be able to enable it manually for debugging.
   */
  static bool shared_mem __read_mostly;
+static int cppc_active __read_mostly;
+static int disable_pstate_load __initdata;
+static int epp_off __initdata;
 
-static struct cpufreq_driver amd_pstate_driver;
+static struct cpufreq_driver *default_pstate_driver;
+static struct amd_cpudata **all_cpu_data;
+
+static struct amd_pstate_params global_params;
+
+static DEFINE_MUTEX(amd_pstate_limits_lock);
+static DEFINE_MUTEX(amd_pstate_driver_lock);
+
+static bool cppc_boost __read_mostly;
+struct kobject *amd_pstate_kobj;
+
+#ifdef CONFIG_ACPI_CPPC_LIB
+static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
+{
+	s16 epp;
+	struct cppc_perf_caps perf_caps;
+	int ret;
+
+	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+		if (!cppc_req_cached) {
+			epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
+					    &cppc_req_cached);
+			if (epp)
+				return epp;
+		}
+		epp = (cppc_req_cached >> 24) & 0xFF;
+	} else {
+		ret = cppc_get_epp_caps(cpudata->cpu, &perf_caps);
+		if (ret < 0) {
+			pr_debug("Could not retrieve energy perf value (%d)\n", ret);
+			return -EIO;
+		}
+		epp = (s16) perf_caps.energy_perf;
+	}
+
+	return epp;
+}
+#endif
+
+static int amd_pstate_get_energy_pref_index(struct amd_cpudata *cpudata, int *raw_epp)
+{
+	s16 epp;
+	int index = -EINVAL;
+
+	*raw_epp = 0;
+	epp = amd_pstate_get_epp(cpudata, 0);
+	if (epp < 0)
+		return epp;
+
+	switch (epp) {
+	case AMD_CPPC_EPP_PERFORMANCE:
+		index = EPP_INDEX_PERFORMANCE;
+		break;
+	case AMD_CPPC_EPP_BALANCE_PERFORMANCE:
+		index = EPP_INDEX_BALANCE_PERFORMANCE;
+		break;
+	case AMD_CPPC_EPP_BALANCE_POWERSAVE:
+		index = EPP_INDEX_BALANCE_POWERSAVE;
+		break;
+	case AMD_CPPC_EPP_POWERSAVE:
+		index = EPP_INDEX_POWERSAVE;
+		break;
+	default:
+		*raw_epp = epp;
+		index = 0;
+	}
+
+	return index;
+}
+
+#ifdef CONFIG_ACPI_CPPC_LIB
+static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
+{
+	int ret;
+	struct cppc_perf_ctrls perf_ctrls;
+
+	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+		u64 value = READ_ONCE(cpudata->cppc_req_cached);
+
+		value &= ~GENMASK_ULL(31, 24);
+		value |= (u64)epp << 24;
+		WRITE_ONCE(cpudata->cppc_req_cached, value);
+
+		ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+		if (!ret)
+			cpudata->epp_cached = epp;
+	} else {
+		perf_ctrls.energy_perf = epp;
+		ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls);
+		if (ret) {
+			pr_debug("failed to set energy perf value (%d)\n", ret);
+			return ret;
+		}
+		cpudata->epp_cached = epp;
+	}
+
+	return ret;
+}
+
+static int amd_pstate_set_energy_pref_index(struct amd_cpudata *cpudata,
+					      int pref_index, bool use_raw,
+					      u32 raw_epp)
+{
+	int epp = -EINVAL;
+	int ret;
+
+	if (!pref_index) {
+		pr_debug("EPP pref_index is invalid\n");
+		return -EINVAL;
+	}
+
+	if (use_raw)
+		epp = raw_epp;
+	else if (epp == -EINVAL)
+		epp = epp_values[pref_index];
+
+	if (epp > 0 && cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
+		pr_debug("EPP cannot be set under performance policy\n");
+		return -EBUSY;
+	}
+
+	ret = amd_pstate_set_epp(cpudata, epp);
+
+	return ret;
+}
+#endif
 
  static inline int pstate_enable(bool enable)
  {

@@ -71,11 +199,25 @@ static inline int pstate_enable(bool enable)
  static int cppc_enable(bool enable)
  {
  	int cpu, ret = 0;
+	struct cppc_perf_ctrls perf_ctrls;
 
  	for_each_present_cpu(cpu) {
  		ret = cppc_set_enable(cpu, enable);
  		if (ret)
  			return ret;
+
+		/* Enable autonomous mode for EPP */
+		if (!cppc_active) {
+			ret = cppc_set_auto_epp(cpu, enable);
+			if (ret)
+				return ret;
+
+			/* Set desired perf as zero to allow EPP firmware control */
+			perf_ctrls.desired_perf = 0;
+			ret = cppc_set_perf(cpu, &perf_ctrls);
+			if (ret)
+				return ret;
+		}
  	}
 
  	return ret;

@@ -418,7 +560,7 @@ static void amd_pstate_boost_init(struct amd_cpudata *cpudata)
  		return;
 
  	cpudata->boost_supported = true;
-	amd_pstate_driver.boost_enabled = true;
+	default_pstate_driver->boost_enabled = true;
  }
 
  static int amd_pstate_cpu_init(struct cpufreq_policy *policy)

@@ -582,10 +724,74 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
  	return sprintf(&buf[0], "%u\n", perf);
  }
 
+static ssize_t show_energy_performance_available_preferences(
+				struct cpufreq_policy *policy, char *buf)
+{
+	int i = 0;
+	int ret = 0;
+
+	while (energy_perf_strings[i] != NULL)
+		ret += sprintf(&buf[ret], "%s ", energy_perf_strings[i++]);
+
+	ret += sprintf(&buf[ret], "\n");
+
+	return ret;
+}
+
+static ssize_t store_energy_performance_preference(
+		struct cpufreq_policy *policy, const char *buf, size_t count)
+{
+	struct amd_cpudata *cpudata = policy->driver_data;
+	char str_preference[21];
+	bool raw = false;
+	ssize_t ret;
+	u32 epp = 0;
+
+	ret = sscanf(buf, "%20s", str_preference);
+	if (ret != 1)
+		return -EINVAL;
+
+	ret = match_string(energy_perf_strings, -1, str_preference);

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

+
+	mutex_lock(&amd_pstate_limits_lock);
+	ret = amd_pstate_set_energy_pref_index(cpudata, ret, raw, epp);
+	mutex_unlock(&amd_pstate_limits_lock);
+
+	return ret ?: count;
+}
+
+static ssize_t show_energy_performance_preference(
+				struct cpufreq_policy *policy, char *buf)
+{
+	struct amd_cpudata *cpudata = policy->driver_data;
+	int preference, raw_epp;
+
+	preference = amd_pstate_get_energy_pref_index(cpudata, &raw_epp);
+	if (preference < 0)
+		return preference;
+
+	if (raw_epp)
+		return  sprintf(buf, "%d\n", raw_epp);
+	else
+		return  sprintf(buf, "%s\n", energy_perf_strings[preference]);
+}
+
  cpufreq_freq_attr_ro(amd_pstate_max_freq);
  cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
 
  cpufreq_freq_attr_ro(amd_pstate_highest_perf);
+cpufreq_freq_attr_rw(energy_performance_preference);
+cpufreq_freq_attr_ro(energy_performance_available_preferences);
 
  static struct freq_attr *amd_pstate_attr[] = {
  	&amd_pstate_max_freq,

@@ -594,6 +800,415 @@ static struct freq_attr *amd_pstate_attr[] = {
  	NULL,
  };
 
+static struct freq_attr *amd_pstate_epp_attr[] = {
+	&amd_pstate_max_freq,
+	&amd_pstate_lowest_nonlinear_freq,
+	&amd_pstate_highest_perf,
+	&energy_performance_preference,
+	&energy_performance_available_preferences,
+	NULL,
+};
+
+static inline void update_boost_state(void)
+{
+	u64 misc_en;
+	struct amd_cpudata *cpudata;
+
+	cpudata = all_cpu_data[0];
+	rdmsrl(MSR_K7_HWCR, misc_en);
+	global_params.cppc_boost_disabled = misc_en & BIT_ULL(25);
+}
+
+static int amd_pstate_init_cpu(unsigned int cpunum)
+{
+	struct amd_cpudata *cpudata;
+
+	cpudata = all_cpu_data[cpunum];
+	if (!cpudata) {
+		cpudata = kzalloc(sizeof(*cpudata), GFP_KERNEL);
+		if (!cpudata)
+			return -ENOMEM;
+		WRITE_ONCE(all_cpu_data[cpunum], cpudata);
+
+		cpudata->cpu = cpunum;
+	}
+	cpudata->epp_powersave = -EINVAL;
+	cpudata->epp_policy = 0;
+	pr_debug("controlling: cpu %d\n", cpunum);
+	return 0;
+}
+
+static int __amd_pstate_cpu_init(struct cpufreq_policy *policy)
+{
+	int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret;
+	struct amd_cpudata *cpudata;
+	struct device *dev;
+	int rc;
+	u64 value;
+
+	rc = amd_pstate_init_cpu(policy->cpu);
+	if (rc)
+		return rc;
+
+	cpudata = all_cpu_data[policy->cpu];
+
+	dev = get_cpu_device(policy->cpu);
+	if (!dev)
+		goto free_cpudata1;
+
+	rc = amd_pstate_init_perf(cpudata);
+	if (rc)
+		goto free_cpudata1;
+
+	min_freq = amd_get_min_freq(cpudata);
+	max_freq = amd_get_max_freq(cpudata);
+	nominal_freq = amd_get_nominal_freq(cpudata);
+	lowest_nonlinear_freq = amd_get_lowest_nonlinear_freq(cpudata);
+	if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) {
+		dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n",
+				min_freq, max_freq);
+		ret = -EINVAL;
+		goto free_cpudata1;
+	}
+
+	policy->min = min_freq;
+	policy->max = max_freq;
+
+	policy->cpuinfo.min_freq = min_freq;
+	policy->cpuinfo.max_freq = max_freq;
+	/* It will be updated by governor */
+	policy->cur = policy->cpuinfo.min_freq;
+
+	/* Initial processor data capability frequencies */
+	cpudata->max_freq = max_freq;
+	cpudata->min_freq = min_freq;
+	cpudata->nominal_freq = nominal_freq;
+	cpudata->lowest_nonlinear_freq = lowest_nonlinear_freq;
+
+	policy->driver_data = cpudata;
+
+	update_boost_state();
+	cpudata->epp_cached = amd_pstate_get_epp(cpudata, value);
+
+	policy->min = policy->cpuinfo.min_freq;
+	policy->max = policy->cpuinfo.max_freq;
+
+	if (boot_cpu_has(X86_FEATURE_CPPC))
+		policy->fast_switch_possible = true;
+
+	if (!shared_mem && boot_cpu_has(X86_FEATURE_CPPC)) {
+		ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value);
+		if (ret)
+			return ret;
+		WRITE_ONCE(cpudata->cppc_req_cached, value);
+
+		ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1, &value);
+		if (ret)
+			return ret;
+		WRITE_ONCE(cpudata->cppc_cap1_cached, value);
+	}
+	amd_pstate_boost_init(cpudata);
+
+	return 0;
+
+free_cpudata1:
+	kfree(cpudata);
+	return ret;
+}
+
+static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
+{
+	int ret;
+
+	ret = __amd_pstate_cpu_init(policy);
+	if (ret)
+		return ret;
+	/*
+	 * Set the policy to powersave to provide a valid fallback value in case