[RESEND PATCH V2 0/9] Implement AMD Pstate EPP Driver

[RESEND PATCH V2 0/9] Implement AMD Pstate EPP Driver

[Perry Yuan]

Hi all,

This patchset implements one new AMD CPU frequency driver
"amd-pstate-epp” instance for better performance and power control.
CPPC has a parameter called energy preference performance (EPP).
The EPP is used in the CCLK 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).

AMD Energy Performance Preference (EPP) provides a hint to the hardware
if software wants to bias toward performance (0x0) or energy efficiency (0xff)
The lowlevel power firmware will calculate the runtime frequency according to the EPP preference 
value. So the EPP hint will impact the CPU cores frequency responsiveness.

We use the RAPL interface with "perf" tool to get the energy data of the package power.
Performance Per Watt (PPW) Calculation:

The PPW calculation is referred by below paper:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsoftware.intel.com%2Fcontent%2Fdam%2Fdevelop%2Fexternal%2Fus%2Fen%2Fdocuments%2Fperformance-per-what-paper.pdf&data=04%7C01%7CPerry.Yuan%40amd.com%7Cac66e8ce98044e9b062708d9ab47c8d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637729147708574423%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=TPOvCE%2Frbb0ptBreWNxHqOi9YnVhcHGKG88vviDLb00%3D&reserved=0

Below formula is referred from below spec to measure the PPW:

(F / t) / P = F * t / (t * E) = F / E,

"F" is the number of frames per second.
"P" is power measured in watts.
"E" is energy measured in joules.

Gitsouce Benchmark Data on ROME Server CPU
+------------------------------+------------------------------+------------+------------------+
| Kernel Module                | PPW (1 / s * J)              |Energy(J) | PPW Improvement (%)|
+==============================+==============================+============+==================+
| acpi-cpufreq:schedutil       | 5.85658E-05                  | 17074.8    | base             |
+------------------------------+------------------------------+------------+------------------+
| acpi-cpufreq:ondemand        | 5.03079E-05                  | 19877.6    | -14.10%          |
+------------------------------+------------------------------+------------+------------------+
| acpi-cpufreq:performance     | 5.88132E-05                  | 17003      | 0.42%            |
+------------------------------+------------------------------+------------+------------------+
| amd-pstate:ondemand          | 4.60295E-05                  | 21725.2    | -21.41%          |
+------------------------------+------------------------------+------------+------------------+
| amd-pstate:schedutil         | 4.70026E-05                  | 21275.4    | -19.7%           |
+------------------------------+------------------------------+------------+------------------+
| amd-pstate:performance       | 5.80094E-05                  | 17238.6    | -0.95%           |
+------------------------------+------------------------------+------------+------------------+
| EPP:performance              | 5.8292E-05                   | 17155      | -0.47%           |
+------------------------------+------------------------------+------------+------------------+
| EPP: balance performance:    | 6.71709E-05                  | 14887.4    | 14.69%           |
+------------------------------+------------------------------+------------+------------------+
| EPP:power                    | 6.66951E-05                  | 4993.6     | 13.88%           |
+------------------------------+------------------------------+------------+------------------+

Tbench Benchmark Data on ROME Server CPU
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| Kernel Module                               | PPW MB / (s * J)  |Throughput(MB/s)| Energy (J)|PPW Improvement(%)|
+=============================================+===================+==============+=============+==================+
| acpi_cpufreq: schedutil                     | 46.39             | 17191        | 37057.3     | base             |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| acpi_cpufreq: ondemand                      | 51.51             | 19269.5      | 37406.5     | 11.04 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| acpi_cpufreq: performance                   | 45.96             | 17063.7      | 37123.7     | -0.74 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| EPP:powersave: performance(0)               | 54.46             | 20263.1      | 37205       | 17.87 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| EPP:powersave: balance performance          | 55.03             | 20481.9      | 37221.5     | 19.14 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| EPP:powersave: balance_power                | 54.43             | 20245.9      | 37194.2     | 17.77 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| EPP:powersave: power(255)                   | 54.26             | 20181.7      | 37197.4     | 17.40 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| amd-pstate: schedutil                       | 48.22             | 17844.9      | 37006.6     | 3.80 %           |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| amd-pstate: ondemand                        | 61.30             | 22988        | 37503.4     | 33.72 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| amd-pstate: performance                     | 54.52             | 20252.6      | 37147.8     | 17.81 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+

changes from v1:
 * rebased to v6.0
 * drive feedbacks from Mario for the suspend/resume patch
 * drive feedbacks from Nathan for the EPP support on msr type
 * fix some typos and code style indent problems
 * update commit comments for patch 4/7
 * change the `epp_enabled` module param name to `epp`
 * set the default epp mode to be false
 * add testing for the x86_energy_perf_policy utility patchset(will
   send that utility patchset with another thread)

Perry Yuan (9):
  ACPI: CPPC: Add AMD pstate energy performance preference cppc control
  cpufreq: amd_pstate: add module parameter to load amd pstate EPP
    driver
  cpufreq: cpufreq: export cpufreq cpu release and acquire
  x86/msr: Add the MSR definition for AMD CPPC boost state
  Documentation: amd-pstate: add EPP profiles introduction
  cpufreq: amd_pstate: add AMD pstate EPP support for shared memory type
    processor
  cpufreq: amd_pstate: add AMD Pstate EPP support for the MSR based
    processors
  cpufreq: amd_pstate: implement amd pstate cpu online and offline
    callback
  cpufreq: amd-pstate: implement suspend and resume callbacks

 Documentation/admin-guide/pm/amd-pstate.rst |  19 +
 arch/x86/include/asm/msr-index.h            |   7 +
 drivers/acpi/cppc_acpi.c                    | 128 ++-
 drivers/cpufreq/amd-pstate.c                | 949 +++++++++++++++++++-
 drivers/cpufreq/cpufreq.c                   |   2 +
 include/acpi/cppc_acpi.h                    |  17 +
 6 files changed, 1115 insertions(+), 7 deletions(-)

-- 
2.34.1

[RESEND PATCH V2 1/9] ACPI: CPPC: Add AMD pstate energy performance preference cppc control

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

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 <Perry.Yuan@amd.com>
---
 drivers/acpi/cppc_acpi.c | 128 ++++++++++++++++++++++++++++++++++++++-
 include/acpi/cppc_acpi.h |  17 ++++++
 2 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 093675b1a1ff..b0e7817cb97f 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -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;
+
+	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");
+
+	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 -EIO;
+
+		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);
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret = -EINVAL;
+
+	if (!cpc_desc) {
+		pr_warn("No CPC descriptor for CPU:%d\n", cpu);
+		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 -EIO;
+
+		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
+ */
+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);
+		return -EINVAL;
+	}
+
+	epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+	if (CPC_IN_PCC(epp_set_reg)) {
+		if (pcc_ss_id < 0)
+			return -EIO;
+
+		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);
+	}
+
+	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.
@@ -1400,7 +1526,7 @@ int cppc_set_enable(int cpu, bool enable)
 		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 */
+		/* 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;
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index c5614444031f..10d91aeedaca 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -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*/
-- 
2.34.1

[RESEND PATCH V2 2/9] cpufreq: amd_pstate: add module parameter to load amd pstate EPP driver

[Perry Yuan]

The amd_pstate mode parameter will allow user to select which amd pstate
working mode as booting mode, amd_pstate instance or amd_pstate_epp instance.

1) amd_pstate instance is depending on the target operation mode.
2) amd_pstate_epp instance is depending on the set_policy operation mode.It
   is also called active mode that AMD SMU has EPP algorithm to control the
   CPU runtime frequency according to the EPP set value and workload.

Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index f52b8f2fe529..2d28f458589c 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -63,6 +63,10 @@ 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 epp = false;
+module_param(epp, bool, 0444);
+MODULE_PARM_DESC(epp, "Enable energy performance preference (EPP) control");
+
 static struct cpufreq_driver amd_pstate_driver;
 
 /**
-- 
2.34.1

[RESEND PATCH V2 3/9] cpufreq: cpufreq: export cpufreq cpu release and acquire

[Perry Yuan]

cpufreq_cpu_release" and "cpufreq_cpu_acquire" are only used internally by
drivers/cpufreq/cpufreq.c currently.
Export them so that other drivers such as the AMD P-state driver can use them as well.

Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
 drivers/cpufreq/cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 69b3d61852ac..a491fea4985e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -262,6 +262,7 @@ void cpufreq_cpu_release(struct cpufreq_policy *policy)
 
 	cpufreq_cpu_put(policy);
 }
+EXPORT_SYMBOL_GPL(cpufreq_cpu_release);
 
 /**
  * cpufreq_cpu_acquire - Find policy for a CPU, mark it as busy and lock it.
@@ -291,6 +292,7 @@ struct cpufreq_policy *cpufreq_cpu_acquire(unsigned int cpu)
 
 	return policy;
 }
+EXPORT_SYMBOL_GPL(cpufreq_cpu_acquire);
 
 /*********************************************************************
  *            EXTERNALLY AFFECTING FREQUENCY CHANGES                 *
-- 
2.34.1

[RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Perry Yuan]

This MSR can be used to check whether the CPU frequency boost state
is enabled in the hardware control. User can change the boost state in
the BIOS setting,amd_pstate driver will update the boost state according
to this msr value.

AMD Processor Programming Reference (PPR)
Link: https://www.amd.com/system/files/TechDocs/40332.pdf [p1095]
Link: https://www.amd.com/system/files/TechDocs/56569-A1-PUB.zip [p162]

Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
 arch/x86/include/asm/msr-index.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 6674bdb096f3..e5ea1c9f747b 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -569,6 +569,7 @@
 #define MSR_AMD_CPPC_CAP2		0xc00102b2
 #define MSR_AMD_CPPC_REQ		0xc00102b3
 #define MSR_AMD_CPPC_STATUS		0xc00102b4
+#define MSR_AMD_CPPC_HW_CTL		0xc0010015
 
 #define AMD_CPPC_LOWEST_PERF(x)		(((x) >> 0) & 0xff)
 #define AMD_CPPC_LOWNONLIN_PERF(x)	(((x) >> 8) & 0xff)
@@ -579,6 +580,8 @@
 #define AMD_CPPC_MIN_PERF(x)		(((x) & 0xff) << 8)
 #define AMD_CPPC_DES_PERF(x)		(((x) & 0xff) << 16)
 #define AMD_CPPC_ENERGY_PERF_PREF(x)	(((x) & 0xff) << 24)
+#define AMD_CPPC_PRECISION_BOOST_BIT   25
+#define AMD_CPPC_PRECISION_BOOST_ENABLED       BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT)
 
 /* AMD Performance Counter Global Status and Control MSRs */
 #define MSR_AMD64_PERF_CNTR_GLOBAL_STATUS	0xc0000300
-- 
2.34.1

[RESEND PATCH V2 5/9] Documentation: amd-pstate: add EPP profiles introduction

[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_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 <Perry.Yuan@amd.com>
---
 Documentation/admin-guide/pm/amd-pstate.rst | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
index 83b58eb4ab4d..d0f0e115013b 100644
--- a/Documentation/admin-guide/pm/amd-pstate.rst
+++ b/Documentation/admin-guide/pm/amd-pstate.rst
@@ -261,6 +261,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
+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`.
 
-- 
2.34.1

[RESEND PATCH V2 6/9] cpufreq: amd_pstate: add AMD pstate EPP support for shared memory type processor

[Perry Yuan]

Add Energy Performance Preference support for AMD SOCs which do not
contain a designated MSR for CPPC support. A shared memory interface
is used for CPPC on these SOCs and the ACPI PCC channel is used to
enable EPP and reset the desired performance.

Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 2d28f458589c..08f9e335f97c 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -135,12 +135,25 @@ static inline int pstate_enable(bool enable)
 
 static int cppc_enable(bool enable)
 {
+	struct cppc_perf_ctrls perf_ctrls;
 	int cpu, ret = 0;
 
 	for_each_present_cpu(cpu) {
 		ret = cppc_set_enable(cpu, enable);
 		if (ret)
 			return ret;
+		if (epp) {
+			/* Enable autonomous mode for EPP */
+			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;
-- 
2.34.1

[RESEND PATCH V2 7/9] cpufreq: amd_pstate: add AMD Pstate EPP support for the MSR based processors

[Perry Yuan]

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 <Perry.Yuan@amd.com>
---
 arch/x86/include/asm/msr-index.h |   4 +
 drivers/cpufreq/amd-pstate.c     | 795 ++++++++++++++++++++++++++++++-
 2 files changed, 792 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index e5ea1c9f747b..53cbdb0c522b 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -588,6 +588,10 @@
 #define MSR_AMD64_PERF_CNTR_GLOBAL_CTL		0xc0000301
 #define MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR	0xc0000302
 
+#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
 /* Fam 17h MSRs */
 #define MSR_F17H_IRPERF			0xc00000e9
 
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 08f9e335f97c..58418808aadf 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -67,7 +67,8 @@ static bool epp = false;
 module_param(epp, bool, 0444);
 MODULE_PARM_DESC(epp, "Enable energy performance preference (EPP) control");
 
-static struct cpufreq_driver amd_pstate_driver;
+static struct cpufreq_driver *default_pstate_driver;
+static struct amd_cpudata **all_cpu_data;
 
 /**
  * struct  amd_aperf_mperf
@@ -79,6 +80,7 @@ struct amd_aperf_mperf {
 	u64 aperf;
 	u64 mperf;
 	u64 tsc;
+	u64 time;
 };
 
 /**
@@ -101,7 +103,19 @@ struct amd_aperf_mperf {
  * @prev: Last Aperf/Mperf/tsc count value read from register
  * @freq: current cpu frequency value
  * @boost_supported: check whether the Processor or SBIOS supports boost mode
- *
+ * @epp_powersave: Last saved CPPC energy performance preference
+				when policy switched to performance
+ * @epp_policy: Last saved policy used to set energy-performance preference
+ * @epp_cached: Cached CPPC energy-performance preference value
+ * @policy: Cpufreq policy value
+ * @sched_flags: Store scheduler flags for possible cross CPU update
+ * @update_util_set: CPUFreq utility callback is set
+ * @last_update: Time stamp of the last performance state update
+ * @cppc_boost_min: Last CPPC boosted min performance state
+ * @cppc_cap1_cached: Cached value of the last CPPC Capabilities MSR
+ * @update_util: Cpufreq utility callback information
+ * @sample: the stored performance sample
+
  * The amd_cpudata is key private data for each CPU thread in AMD P-State, and
  * represents all the attributes and goals that AMD P-State requests at runtime.
  */
@@ -126,8 +140,197 @@ struct amd_cpudata {
 
 	u64 	freq;
 	bool	boost_supported;
+	u64	cppc_hw_conf_cached;
+
+	/* EPP feature related attributes*/
+	s16	epp_powersave;
+	s16	epp_policy;
+	s16	epp_cached;
+	u32	policy;
+	u32	sched_flags;
+	bool	update_util_set;
+	u64	last_update;
+	u64	last_io_update;
+	u32	cppc_boost_min;
+	u64	cppc_cap1_cached;
+	struct	update_util_data update_util;
+	struct	amd_aperf_mperf sample;
+};
+
+/**
+ * struct amd_pstate_params - global parameters for the performance control
+ * @ cppc_boost_disabled wheher the core performance boost disabled
+ */
+struct amd_pstate_params {
+	bool cppc_boost_disabled;
+};
+
+/*
+ * AMD Energy Preference Performance (EPP)
+ * The EPP is used in the CCLK 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)
+ * display strings corresponding to EPP index in the
+ * energy_perf_strings[]
+ *	index		String
+ *-------------------------------------
+ *	0		default
+ *	1		performance
+ *	2		balance_performance
+ *	3		balance_power
+ *	4		power
+ */
+enum energy_perf_value_index {
+	EPP_INDEX_DEFAULT = 0,
+	EPP_INDEX_PERFORMANCE,
+	EPP_INDEX_BALANCE_PERFORMANCE,
+	EPP_INDEX_BALANCE_POWERSAVE,
+	EPP_INDEX_POWERSAVE,
+};
+
+static const char * const energy_perf_strings[] = {
+	[EPP_INDEX_DEFAULT] = "default",
+	[EPP_INDEX_PERFORMANCE] = "performance",
+	[EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
+	[EPP_INDEX_BALANCE_POWERSAVE] = "balance_power",
+	[EPP_INDEX_POWERSAVE] = "power",
+	NULL
+};
+
+static unsigned int epp_values[] = {
+	[EPP_INDEX_DEFAULT] = 0,
+	[EPP_INDEX_PERFORMANCE] = AMD_CPPC_EPP_PERFORMANCE,
+	[EPP_INDEX_BALANCE_PERFORMANCE] = AMD_CPPC_EPP_BALANCE_PERFORMANCE,
+	[EPP_INDEX_BALANCE_POWERSAVE] = AMD_CPPC_EPP_BALANCE_POWERSAVE,
+	[EPP_INDEX_POWERSAVE] = AMD_CPPC_EPP_POWERSAVE,
 };
 
+static struct amd_pstate_params global_params;
+
+static DEFINE_MUTEX(amd_pstate_limits_lock);
+static DEFINE_MUTEX(amd_pstate_driver_lock);
+static DEFINE_SPINLOCK(amd_pstate_cpu_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)
 {
 	return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable);
@@ -496,7 +699,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)
@@ -660,10 +863,108 @@ 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);
+	if (ret < 0) {
+		ret = kstrtouint(buf, 10, &epp);
+		if (ret)
+			return ret;
+
+		if ((epp > 255) || (epp < 0))
+			return -EINVAL;
+
+		raw = true;
+	}
+
+	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]);
+}
+
+static void amd_pstate_update_policies(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		cpufreq_update_policy(cpu);
+}
+
+static ssize_t show_pstate_dynamic_boost(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%u\n", cppc_boost);
+}
+
+static ssize_t store_pstate_dynamic_boost(struct kobject *a,
+				       struct kobj_attribute *b,
+				       const char *buf, size_t count)
+{
+	unsigned int input;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &input);
+	if (ret)
+		return ret;
+
+	mutex_lock(&amd_pstate_driver_lock);
+	cppc_boost = !!input;
+	amd_pstate_update_policies();
+	mutex_unlock(&amd_pstate_driver_lock);
+
+	return count;
+}
+
 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);
+define_one_global_rw(pstate_dynamic_boost);
 
 static struct freq_attr *amd_pstate_attr[] = {
 	&amd_pstate_max_freq,
@@ -672,6 +973,424 @@ 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 struct attribute *pstate_global_attributes[] = {
+	&pstate_dynamic_boost.attr,
+	NULL
+};
+
+static const struct attribute_group amd_pstate_global_attr_group = {
+	.attrs = pstate_global_attributes,
+};
+
+static inline void update_boost_state(void)
+{
+	u64 misc_en;
+	struct amd_cpudata *cpudata;
+
+	cpudata = all_cpu_data[0];
+	rdmsrl(MSR_AMD_CPPC_HW_CTL, misc_en);
+	global_params.cppc_boost_disabled = misc_en & AMD_CPPC_PRECISION_BOOST_ENABLED;
+}
+
+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
+	 * the default cpufreq governor is neither powersave nor performance.
+	 */
+	policy->policy = CPUFREQ_POLICY_POWERSAVE;
+
+	return 0;
+}
+
+static int amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
+{
+	pr_debug("amd-pstate: CPU %d exiting\n", policy->cpu);
+	policy->fast_switch_possible = false;
+	return 0;
+}
+
+static void amd_pstate_update_max_freq(unsigned int cpu)
+{
+	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
+
+	if (!policy)
+		return;
+
+	refresh_frequency_limits(policy);
+	cpufreq_cpu_release(policy);
+}
+
+static void amd_pstate_epp_update_limits(unsigned int cpu)
+{
+	mutex_lock(&amd_pstate_driver_lock);
+	update_boost_state();
+	if (global_params.cppc_boost_disabled) {
+		for_each_possible_cpu(cpu)
+			amd_pstate_update_max_freq(cpu);
+	} else {
+		cpufreq_update_policy(cpu);
+	}
+	mutex_unlock(&amd_pstate_driver_lock);
+}
+
+static int cppc_boost_hold_time_ns = 3 * NSEC_PER_MSEC;
+
+static inline void amd_pstate_boost_up(struct amd_cpudata *cpudata)
+{
+	u64 hwp_req = READ_ONCE(cpudata->cppc_req_cached);
+	u64 hwp_cap = READ_ONCE(cpudata->cppc_cap1_cached);
+	u32 max_limit = (hwp_req & 0xff);
+	u32 min_limit = (hwp_req & 0xff00) >> 8;
+	u32 boost_level1;
+
+	/* If max and min are equal or already at max, nothing to boost */
+	if (max_limit == min_limit)
+		return;
+
+	/* Set boost max and min to initial value */
+	if (!cpudata->cppc_boost_min)
+		cpudata->cppc_boost_min = min_limit;
+
+	boost_level1 = ((AMD_CPPC_NOMINAL_PERF(hwp_cap) + min_limit) >> 1);
+
+	if (cpudata->cppc_boost_min < boost_level1)
+		cpudata->cppc_boost_min = boost_level1;
+	else if (cpudata->cppc_boost_min < AMD_CPPC_NOMINAL_PERF(hwp_cap))
+		cpudata->cppc_boost_min = AMD_CPPC_NOMINAL_PERF(hwp_cap);
+	else if (cpudata->cppc_boost_min == AMD_CPPC_NOMINAL_PERF(hwp_cap))
+		cpudata->cppc_boost_min = max_limit;
+	else
+		return;
+
+	hwp_req &= ~AMD_CPPC_MIN_PERF(~0L);
+	hwp_req |= AMD_CPPC_MIN_PERF(cpudata->cppc_boost_min);
+	wrmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, hwp_req);
+	cpudata->last_update = cpudata->sample.time;
+}
+
+static inline void amd_pstate_boost_down(struct amd_cpudata *cpudata)
+{
+	bool expired;
+
+	if (cpudata->cppc_boost_min) {
+		expired = time_after64(cpudata->sample.time, cpudata->last_update +
+					cppc_boost_hold_time_ns);
+
+		if (expired) {
+			wrmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
+						cpudata->cppc_req_cached);
+			cpudata->cppc_boost_min = 0;
+		}
+	}
+
+	cpudata->last_update = cpudata->sample.time;
+}
+
+static inline void amd_pstate_boost_update_util(struct amd_cpudata *cpudata,
+						      u64 time)
+{
+	cpudata->sample.time = time;
+	if (smp_processor_id() != cpudata->cpu)
+		return;
+
+	if (cpudata->sched_flags & SCHED_CPUFREQ_IOWAIT) {
+		bool do_io = false;
+
+		cpudata->sched_flags = 0;
+		/*
+		 * Set iowait_boost flag and update time. Since IO WAIT flag
+		 * is set all the time, we can't just conclude that there is
+		 * some IO bound activity is scheduled on this CPU with just
+		 * one occurrence. If we receive at least two in two
+		 * consecutive ticks, then we treat as boost candidate.
+		 * This is leveraged from Intel Pstate driver.
+		 */
+		if (time_before64(time, cpudata->last_io_update + 2 * TICK_NSEC))
+			do_io = true;
+
+		cpudata->last_io_update = time;
+
+		if (do_io)
+			amd_pstate_boost_up(cpudata);
+
+	} else {
+		amd_pstate_boost_down(cpudata);
+	}
+}
+
+static inline void amd_pstate_cppc_update_hook(struct update_util_data *data,
+						u64 time, unsigned int flags)
+{
+	struct amd_cpudata *cpudata = container_of(data,
+				struct amd_cpudata, update_util);
+
+	cpudata->sched_flags |= flags;
+
+	if (smp_processor_id() == cpudata->cpu)
+		amd_pstate_boost_update_util(cpudata, time);
+}
+
+static void amd_pstate_clear_update_util_hook(unsigned int cpu)
+{
+	struct amd_cpudata *cpudata = all_cpu_data[cpu];
+
+	if (!cpudata->update_util_set)
+		return;
+
+	cpufreq_remove_update_util_hook(cpu);
+	cpudata->update_util_set = false;
+	synchronize_rcu();
+}
+
+static void amd_pstate_set_update_util_hook(unsigned int cpu_num)
+{
+	struct amd_cpudata *cpudata = all_cpu_data[cpu_num];
+
+	if (!cppc_boost) {
+		if (cpudata->update_util_set)
+			amd_pstate_clear_update_util_hook(cpudata->cpu);
+		return;
+	}
+
+	if (cpudata->update_util_set)
+		return;
+
+	cpudata->sample.time = 0;
+	cpufreq_add_update_util_hook(cpu_num, &cpudata->update_util,
+						amd_pstate_cppc_update_hook);
+	cpudata->update_util_set = true;
+}
+
+static void amd_pstate_epp_init(unsigned int cpu)
+{
+	struct amd_cpudata *cpudata = all_cpu_data[cpu];
+	u32 max_perf, min_perf;
+	u64 value;
+	s16 epp;
+	int ret;
+
+	max_perf = READ_ONCE(cpudata->highest_perf);
+	min_perf = READ_ONCE(cpudata->lowest_perf);
+
+	value = READ_ONCE(cpudata->cppc_req_cached);
+
+	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
+		min_perf = max_perf;
+
+	/* Initial min/max values for CPPC Performance Controls Register */
+	value &= ~AMD_CPPC_MIN_PERF(~0L);
+	value |= AMD_CPPC_MIN_PERF(min_perf);
+
+	value &= ~AMD_CPPC_MAX_PERF(~0L);
+	value |= AMD_CPPC_MAX_PERF(max_perf);
+
+	/* CPPC EPP feature require to set zero to the desire perf bit */
+	value &= ~AMD_CPPC_DES_PERF(~0L);
+	value |= AMD_CPPC_DES_PERF(0);
+
+	if (cpudata->epp_policy == cpudata->policy)
+		goto skip_epp;
+
+	cpudata->epp_policy = cpudata->policy;
+
+	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
+		epp = amd_pstate_get_epp(cpudata, value);
+		cpudata->epp_powersave = epp;
+		if (epp < 0)
+			goto skip_epp;
+		/* force the epp value to be zero for performance policy */
+		epp = 0;
+	} else {
+		if (cpudata->epp_powersave < 0)
+			goto skip_epp;
+		/* Get BIOS pre-defined epp value */
+		epp = amd_pstate_get_epp(cpudata, value);
+		if (epp)
+			goto skip_epp;
+		epp = cpudata->epp_powersave;
+	}
+	/* Set initial EPP value */
+	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+		value &= ~GENMASK_ULL(31, 24);
+		value |= (u64)epp << 24;
+	}
+
+skip_epp:
+	WRITE_ONCE(cpudata->cppc_req_cached, value);
+	ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+	if (!ret)
+		cpudata->epp_cached = epp;
+}
+
+static void amd_pstate_set_max_limits(struct amd_cpudata *cpudata)
+{
+	u64 hwp_cap = READ_ONCE(cpudata->cppc_cap1_cached);
+	u64 hwp_req = READ_ONCE(cpudata->cppc_req_cached);
+	u32 max_limit = (hwp_cap >> 24) & 0xff;
+
+	hwp_req &= ~AMD_CPPC_MIN_PERF(~0L);
+	hwp_req |= AMD_CPPC_MIN_PERF(max_limit);
+	wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, hwp_req);
+}
+
+static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
+{
+	struct amd_cpudata *cpudata;
+
+	if (!policy->cpuinfo.max_freq)
+		return -ENODEV;
+
+	pr_debug("set_policy: cpuinfo.max %u policy->max %u\n",
+				policy->cpuinfo.max_freq, policy->max);
+
+	cpudata = all_cpu_data[policy->cpu];
+	cpudata->policy = policy->policy;
+
+	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+		mutex_lock(&amd_pstate_limits_lock);
+
+		if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
+			amd_pstate_clear_update_util_hook(policy->cpu);
+			amd_pstate_set_max_limits(cpudata);
+		} else {
+			amd_pstate_set_update_util_hook(policy->cpu);
+		}
+
+		if (boot_cpu_has(X86_FEATURE_CPPC))
+			amd_pstate_epp_init(policy->cpu);
+
+		mutex_unlock(&amd_pstate_limits_lock);
+	}
+
+	return 0;
+}
+
+static void amd_pstate_verify_cpu_policy(struct amd_cpudata *cpudata,
+					   struct cpufreq_policy_data *policy)
+{
+	update_boost_state();
+	cpufreq_verify_within_cpu_limits(policy);
+}
+
+static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
+{
+	amd_pstate_verify_cpu_policy(all_cpu_data[policy->cpu], policy);
+	pr_debug("policy_max =%d, policy_min=%d\n", policy->max, policy->min);
+	return 0;
+}
+
 static struct cpufreq_driver amd_pstate_driver = {
 	.flags		= CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
 	.verify		= amd_pstate_verify,
@@ -685,8 +1404,20 @@ static struct cpufreq_driver amd_pstate_driver = {
 	.attr		= amd_pstate_attr,
 };
 
+static struct cpufreq_driver amd_pstate_epp_driver = {
+	.flags		= CPUFREQ_CONST_LOOPS,
+	.verify		= amd_pstate_epp_verify_policy,
+	.setpolicy	= amd_pstate_epp_set_policy,
+	.init		= amd_pstate_epp_cpu_init,
+	.exit		= amd_pstate_epp_cpu_exit,
+	.update_limits	= amd_pstate_epp_update_limits,
+	.name		= "amd_pstate_epp",
+	.attr		= amd_pstate_epp_attr,
+};
+
 static int __init amd_pstate_init(void)
 {
+	static struct amd_cpudata **cpudata;
 	int ret;
 
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
@@ -701,10 +1432,24 @@ static int __init amd_pstate_init(void)
 	if (cpufreq_get_current_driver())
 		return -EEXIST;
 
+	cpudata = vzalloc(array_size(sizeof(void *), num_possible_cpus()));
+	if (!cpudata)
+		return -ENOMEM;
+	WRITE_ONCE(all_cpu_data, cpudata);
+
+	if (epp) {
+		pr_info("AMD CPPC loading with amd_pstate_epp driver instance.\n");
+		default_pstate_driver = &amd_pstate_epp_driver;
+	} else {
+		pr_info("AMD CPPC loading with amd_pstate driver instance.\n");
+		default_pstate_driver = &amd_pstate_driver;
+	}
+
 	/* capability check */
 	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+		if (!epp)
+			default_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
 		pr_debug("AMD CPPC MSR based functionality is supported\n");
-		amd_pstate_driver.adjust_perf = amd_pstate_adjust_perf;
 	} else if (shared_mem) {
 		static_call_update(amd_pstate_enable, cppc_enable);
 		static_call_update(amd_pstate_init_perf, cppc_init_perf);
@@ -721,19 +1466,55 @@ static int __init amd_pstate_init(void)
 		return ret;
 	}
 
-	ret = cpufreq_register_driver(&amd_pstate_driver);
+	ret = cpufreq_register_driver(default_pstate_driver);
 	if (ret)
-		pr_err("failed to register amd_pstate_driver with return %d\n",
+		pr_err("failed to register amd pstate driver with return %d\n",
 		       ret);
 
+	amd_pstate_kobj = kobject_create_and_add("amd-pstate", &cpu_subsys.dev_root->kobj);
+	if (!amd_pstate_kobj)
+		pr_err("amd-pstate: Global sysfs registration failed.\n");
+
+	ret = sysfs_create_group(amd_pstate_kobj, &amd_pstate_global_attr_group);
+	if (ret) {
+		pr_err("amd-pstate: Sysfs attribute export failed with error %d.\n",
+		       ret);
+	}
+
 	return ret;
 }
 
+static inline void amd_pstate_kobj_cleanup(struct kobject *kobj)
+{
+	kobject_del(kobj);
+	kobject_put(kobj);
+}
+
 static void __exit amd_pstate_exit(void)
 {
-	cpufreq_unregister_driver(&amd_pstate_driver);
+	unsigned int cpu;
+
+	cpufreq_unregister_driver(default_pstate_driver);
 
 	amd_pstate_enable(false);
+
+	sysfs_remove_group(amd_pstate_kobj, &amd_pstate_global_attr_group);
+	amd_pstate_kobj_cleanup(amd_pstate_kobj);
+
+	cpus_read_lock();
+	for_each_online_cpu(cpu) {
+		if (all_cpu_data[cpu]) {
+			if (default_pstate_driver == &amd_pstate_epp_driver)
+				amd_pstate_clear_update_util_hook(cpu);
+
+			spin_lock(&amd_pstate_cpu_lock);
+			kfree(all_cpu_data[cpu]);
+			WRITE_ONCE(all_cpu_data[cpu], NULL);
+			spin_unlock(&amd_pstate_cpu_lock);
+		}
+	}
+	cpus_read_unlock();
+
 }
 
 module_init(amd_pstate_init);
-- 
2.34.1

[RESEND PATCH V2 8/9] cpufreq: amd_pstate: implement amd pstate cpu online and offline callback

[Perry Yuan]

The patch adds online and offline driver callback support to allow cpu cores go
offline and help to restore the previous working states when core goes
back online later for EPP driver mode.

Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 101 ++++++++++++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 58418808aadf..8d99714022e3 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -115,7 +115,8 @@ struct amd_aperf_mperf {
  * @cppc_cap1_cached: Cached value of the last CPPC Capabilities MSR
  * @update_util: Cpufreq utility callback information
  * @sample: the stored performance sample
-
+ * @suspended:	Whether or not the driver has been suspended.
+ *
  * The amd_cpudata is key private data for each CPU thread in AMD P-State, and
  * represents all the attributes and goals that AMD P-State requests at runtime.
  */
@@ -155,6 +156,7 @@ struct amd_cpudata {
 	u64	cppc_cap1_cached;
 	struct	update_util_data update_util;
 	struct	amd_aperf_mperf sample;
+	bool suspended;
 };
 
 /**
@@ -215,6 +217,9 @@ static DEFINE_SPINLOCK(amd_pstate_cpu_lock);
 static bool cppc_boost __read_mostly;
 struct kobject *amd_pstate_kobj;
 
+/* the flag whether the pstate driver is exiting */
+static bool pstate_exiting;
+
 #ifdef CONFIG_ACPI_CPPC_LIB
 static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
 {
@@ -1377,6 +1382,96 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
 	return 0;
 }
 
+static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
+{
+	struct cppc_perf_ctrls perf_ctrls;
+	u64 value, max_perf;
+	int ret;
+
+	ret = amd_pstate_enable(true);
+	if (ret)
+		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
+
+	value = READ_ONCE(cpudata->cppc_req_cached);
+	max_perf = READ_ONCE(cpudata->highest_perf);
+
+	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+	} else {
+		perf_ctrls.max_perf = max_perf;
+		perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(cpudata->epp_cached);
+		cppc_set_perf(cpudata->cpu, &perf_ctrls);
+	}
+}
+
+static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
+{
+	struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+
+	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
+
+	if (epp) {
+		amd_pstate_epp_reenable(cpudata);
+		cpudata->suspended = false;
+	}
+
+	return 0;
+}
+
+static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
+{
+	struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+	struct cppc_perf_ctrls perf_ctrls;
+	int min_perf;
+	u64 value;
+
+	min_perf = READ_ONCE(cpudata->lowest_perf);
+	value = READ_ONCE(cpudata->cppc_req_cached);
+
+	mutex_lock(&amd_pstate_limits_lock);
+	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+		cpudata->epp_policy = CPUFREQ_POLICY_UNKNOWN;
+
+		/* Set max perf same as min perf */
+		value &= ~AMD_CPPC_MAX_PERF(~0L);
+		value |= AMD_CPPC_MAX_PERF(min_perf);
+		value &= ~AMD_CPPC_MIN_PERF(~0L);
+		value |= AMD_CPPC_MIN_PERF(min_perf);
+		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+	} else {
+		perf_ctrls.desired_perf = 0;
+		perf_ctrls.max_perf = min_perf;
+		perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(AMD_CPPC_EPP_POWERSAVE);
+		cppc_set_perf(cpudata->cpu, &perf_ctrls);
+	}
+	mutex_unlock(&amd_pstate_limits_lock);
+}
+
+static int amd_pstate_cpu_offline(struct cpufreq_policy *policy)
+{
+	struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+
+	pr_debug("AMD CPU Core %d going offline\n", cpudata->cpu);
+
+	if (cpudata->suspended)
+		return 0;
+
+	if (pstate_exiting)
+		return 0;
+
+	if (epp)
+		amd_pstate_epp_offline(policy);
+
+	return 0;
+}
+
+static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
+{
+	amd_pstate_clear_update_util_hook(policy->cpu);
+
+	return amd_pstate_cpu_offline(policy);
+}
+
 static void amd_pstate_verify_cpu_policy(struct amd_cpudata *cpudata,
 					   struct cpufreq_policy_data *policy)
 {
@@ -1411,6 +1506,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
 	.init		= amd_pstate_epp_cpu_init,
 	.exit		= amd_pstate_epp_cpu_exit,
 	.update_limits	= amd_pstate_epp_update_limits,
+	.offline	= amd_pstate_epp_cpu_offline,
+	.online		= amd_pstate_epp_cpu_online,
 	.name		= "amd_pstate_epp",
 	.attr		= amd_pstate_epp_attr,
 };
@@ -1480,6 +1577,7 @@ static int __init amd_pstate_init(void)
 		pr_err("amd-pstate: Sysfs attribute export failed with error %d.\n",
 		       ret);
 	}
+	pstate_exiting = false;
 
 	return ret;
 }
@@ -1494,6 +1592,7 @@ static void __exit amd_pstate_exit(void)
 {
 	unsigned int cpu;
 
+	pstate_exiting = true;
 	cpufreq_unregister_driver(default_pstate_driver);
 
 	amd_pstate_enable(false);
-- 
2.34.1

[RESEND PATCH V2 9/9] cpufreq: amd-pstate: implement suspend and resume callbacks

[Perry Yuan]

add suspend and resume support for the AMD processors by amd_pstate_epp
driver instance.

When the CPPC is suspended, EPP driver will set EPP profile to 'power'
profile and set max/min perf to lowest perf value.
When resume happens, it will restore the MSR registers with
previous cached value.

Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 40 ++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 8d99714022e3..a4683fdddde6 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1472,6 +1472,44 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
 	return amd_pstate_cpu_offline(policy);
 }
 
+static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
+{
+	struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+	int ret;
+
+	/* avoid suspending when EPP is not enabled */
+	if (!epp)
+		return 0;
+
+	/* set this flag to avoid setting core offline*/
+	cpudata->suspended = true;
+
+	/* disable CPPC in lowlevel firmware */
+	ret = amd_pstate_enable(false);
+	if (ret)
+		pr_err("failed to suspend, return %d\n", ret);
+
+	return 0;
+}
+
+static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
+{
+	struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+
+	if (cpudata->suspended) {
+		mutex_lock(&amd_pstate_limits_lock);
+
+		/* enable amd pstate from suspend state*/
+		amd_pstate_epp_reenable(cpudata);
+
+		mutex_unlock(&amd_pstate_limits_lock);
+
+		cpudata->suspended = false;
+	}
+
+	return 0;
+}
+
 static void amd_pstate_verify_cpu_policy(struct amd_cpudata *cpudata,
 					   struct cpufreq_policy_data *policy)
 {
@@ -1508,6 +1546,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
 	.update_limits	= amd_pstate_epp_update_limits,
 	.offline	= amd_pstate_epp_cpu_offline,
 	.online		= amd_pstate_epp_cpu_online,
+	.suspend	= amd_pstate_epp_suspend,
+	.resume		= amd_pstate_epp_resume,
 	.name		= "amd_pstate_epp",
 	.attr		= amd_pstate_epp_attr,
 };
-- 
2.34.1

Re: [RESEND PATCH V2 7/9] cpufreq: amd_pstate: add AMD Pstate EPP support for the MSR based processors

[Viresh Kumar]

On 11-10-22, 00:22, Perry Yuan wrote:
> +static void amd_pstate_update_max_freq(unsigned int cpu)
> +{
> +	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);

Why isn't cpufreq_cpu_get() enough here ? cpufreq_cpu_acquire() is
special as it takes policy->rwsem as well and you shouldn't need it.

> +
> +	if (!policy)
> +		return;
> +
> +	refresh_frequency_limits(policy);
> +	cpufreq_cpu_release(policy);
> +}

-- 
viresh

RE: [RESEND PATCH V2 7/9] cpufreq: amd_pstate: add AMD Pstate EPP support for the MSR based processors

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Viresh. 

> -----Original Message-----
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Sent: Tuesday, October 11, 2022 10:54 AM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: rafael.j.wysocki@intel.com; Huang, Ray <Ray.Huang@amd.com>; Sharma,
> Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, Li
> (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [RESEND PATCH V2 7/9] cpufreq: amd_pstate: add AMD Pstate EPP
> support for the MSR based processors
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> On 11-10-22, 00:22, Perry Yuan wrote:
> > +static void amd_pstate_update_max_freq(unsigned int cpu) {
> > +     struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
> 
> Why isn't cpufreq_cpu_get() enough here ? cpufreq_cpu_acquire() is special as it
> takes policy->rwsem as well and you shouldn't need it.
> 
> > +
> > +     if (!policy)
> > +             return;
> > +
> > +     refresh_frequency_limits(policy);
> > +     cpufreq_cpu_release(policy);
> > +}
> 
> --
> Viresh

Thanks for your suggestion. I will try the cpufreq_cpu_get() and get this fixed in v3.

Perry .

Re: [RESEND PATCH V2 0/9] Implement AMD Pstate EPP Driver

[Russell Haley]

Although I am very much in favor of having some kernel interface to the
EPP MSR for AMD CPUs just as for Intel, I have some reservations about
the units in the tables, and whether performance per watt, measured in
this way by these benchmarks, is an appropriate figure of merit for
cpufreq governors.

On 10/10/22 11:22, Perry Yuan wrote:

> The PPW calculation is referred by below paper:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsoftware.intel.com%2Fcontent%2Fdam%2Fdevelop%2Fexternal%2Fus%2Fen%2Fdocuments%2Fperformance-per-what-paper.pdf&data=04%7C01%7CPerry.Yuan%40amd.com%7Cac66e8ce98044e9b062708d9ab47c8d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637729147708574423%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=TPOvCE%2Frbb0ptBreWNxHqOi9YnVhcHGKG88vviDLb00%3D&reserved=0
> 
> Below formula is referred from below spec to measure the PPW:
> 
> (F / t) / P = F * t / (t * E) = F / E,
> 
> "F" is the number of frames per second.
> "P" is power measured in watts.
> "E" is energy measured in joules.

In the whitepaper, "F" is not the number of frames per second.  It is
the number of frames.  The number of frames per second is "F/t", where
"t" is the number of seconds. Following the dimensional analysis:

    Frames
   --------- / Watts
    seconds

    Frames      Joules
 = --------- / ---------
    seconds     seconds

    Frames      seconds
 = --------- * ---------
    seconds     Joules

    Frames
 = ---------
    Joules

All the seconds cancel, and performance per watt reduces to completed
work divided by energy, as you would expect. However, in the benchmark
tables, seconds always appear in the PPW unit.

Furthermore...

> Gitsouce Benchmark Data on ROME Server CPU
> +------------------------------+------------------------------+------------+------------------+
> | Kernel Module                | PPW (1 / s * J)              |Energy(J) | PPW Improvement (%)|
> +==============================+==============================+============+==================+
> | acpi-cpufreq:schedutil       | 5.85658E-05                  | 17074.8    | base             |
> +------------------------------+------------------------------+------------+------------------+
> | acpi-cpufreq:ondemand        | 5.03079E-05                  | 19877.6    | -14.10%          |
> +------------------------------+------------------------------+------------+------------------+
> | acpi-cpufreq:performance     | 5.88132E-05                  | 17003      | 0.42%            |
> +------------------------------+------------------------------+------------+------------------+
> | amd-pstate:ondemand          | 4.60295E-05                  | 21725.2    | -21.41%          |
> +------------------------------+------------------------------+------------+------------------+
> | amd-pstate:schedutil         | 4.70026E-05                  | 21275.4    | -19.7%           |
> +------------------------------+------------------------------+------------+------------------+
> | amd-pstate:performance       | 5.80094E-05                  | 17238.6    | -0.95%           |
> +------------------------------+------------------------------+------------+------------------+
> | EPP:performance              | 5.8292E-05                   | 17155      | -0.47%           |
> +------------------------------+------------------------------+------------+------------------+
> | EPP: balance performance:    | 6.71709E-05                  | 14887.4    | 14.69%           |
> +------------------------------+------------------------------+------------+------------------+
> | EPP:power                    | 6.66951E-05                  | 4993.6     | 13.88%           |
> +------------------------------+------------------------------+------------+------------------+

The numbers in the PPW column are equal to 1/Energy, so the math works
out even if the units are mislabeled. But neither the actual performance
nor anything that can be used to derive it appear in the table.

As far as I can tell, this benchmark, which compiles git from source,
should be entirely CPU bound.  That is, it is occupying at least one CPU
core for the entire runtime. [1] For such tasks, to a first order
approximation you can run the CPU at 1/2 frequency and finish the task
with 1/4 the energy in 2x the time. Since the time units vanish,
"performance per watt" can look good when performance and watts are both
low. So you very much need to have performance in the table.

I can think of a couple ways to handle this problem. The empirical
approach would be to use the userspace governor and scaling_setspeed to
iteratively find a fixed frequency with similar benchmark performance to
each driver/governor, and then report the energy usage. The "benchmark"
should probably be a sum of multiple runtime benchmarks, or a harmonic
mean of multiple rate benchmarks, because the advantage a governor is
supposed to have is the ability to adapt to different workloads and/or
different phases of computation.

Alternately, one might use or perf^3/watt as the figure of merit. That's
an ED2P metric [2], and you'd be comparing governors on their ability to
make the CPU look like a "better" CPU by identifying tasks that waste a
lot of available cycles stalled on things outside the CPU core clock
domain (DRAM, I/O) and running them at lower frequency and higher
instructions per available cycle.

I've heard about perf^2/watt being used, but I don't know what, if any,
theoretical basis it has.

On another note, If PPW of CPU-bound tasks is maximized based on energy
counted with the CPU package energy MSR only (assuming it's even
calibrated), without including DRAM and baseline consumers like fans,
HDDs, southbridge, displays, NICs, radios, ect., then the PPW of the
system as a whole is certain to be worse. This is the idea behind
race-to-idle. On the other hand, CPU package power can be the correct
measure for deadline-type workloads where finishing the task sooner
doesn't allow powering down the machine. That's stuff like
line-speed-limited network servers and scrolling in web browsers. In
that case, the only thing that goes to sleep when the task is done is
the CPU, so the only energy that counts is the energy burnt in the CPU.

> Tbench Benchmark Data on ROME Server CPU
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | Kernel Module                               | PPW MB / (s * J)  |Throughput(MB/s)| Energy (J)|PPW Improvement(%)|
> +=============================================+===================+==============+=============+==================+
> | acpi_cpufreq: schedutil                     | 46.39             | 17191        | 37057.3     | base             |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | acpi_cpufreq: ondemand                      | 51.51             | 19269.5      | 37406.5     | 11.04 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | acpi_cpufreq: performance                   | 45.96             | 17063.7      | 37123.7     | -0.74 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | EPP:powersave: performance(0)               | 54.46             | 20263.1      | 37205       | 17.87 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | EPP:powersave: balance performance          | 55.03             | 20481.9      | 37221.5     | 19.14 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | EPP:powersave: balance_power                | 54.43             | 20245.9      | 37194.2     | 17.77 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | EPP:powersave: power(255)                   | 54.26             | 20181.7      | 37197.4     | 17.40 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | amd-pstate: schedutil                       | 48.22             | 17844.9      | 37006.6     | 3.80 %           |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | amd-pstate: ondemand                        | 61.30             | 22988        | 37503.4     | 33.72 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | amd-pstate: performance                     | 54.52             | 20252.6      | 37147.8     | 17.81 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
For this one it seems like PPW is calculated as Throughput/Energy * 100?
The benchmark looks a lot like the result of running the script at [3].
It looks like the script would multiply by 99 though?  And also the
bogus time units do not appear in the script, so if that's a newer
version I'm glad it's fixed.

But I ran tbench on my own machine, single-thread to reduce the impact
of background activity, and got this:

+---------------+---------------------+--------+
| CPU Frequency | Throughput ( MB/s ) | Perf % |
+===============+=====================+========+
| 1 GHz         |  85.78              | Base   |
| 2 GHz         | 174.35              | 203 %  |
| 3 GHz         | 264.04              | 308 %  |
| 4 GHz         | 352.86              | 411 %  |
+---------------+---------------------+--------+

Which implies tbench is 100% clock-frequency-bound [1, 4], and so this
benchmark is equivalent to measuring the average clock frequency over
the runtime. I think that means the most interesting number in your
table is the throughput.

Somehow, amd-pstate:ondemand is running the CPU faster on average than
even amd-pstate:performance and EPP:powersave:performance, which
*should* be choosing the highest possible frequency at all times.

1. As I understand it, the intent in the schedutil governor is to run
CPU-bound tasks at maximum performance, and if you want to trade energy
for time userspace should set cpu.uclamp.max in the cgroup.  Any
CPU-bound benchmark that runs slower under the schedutil governor than
under the performance governor can then be considered a bug. There are
many such bugs, and tbench is one of them.  But I agree with the
philosphy: 1:1 scaling with CPU frequency is the best possible, and no
governor should be running such a workload below scaling_max_frequency.

2. http://www.eecs.umich.edu/courses/eecs470/OLD/w14/lectures/470L14W14.pdf

3.
https://patchwork.kernel.org/project/linux-pm/patch/20220914061105.1982477-3-li.meng@amd.com/

4. I suspect the >100% scaling is due to the relative overhead of
background tasks and scheduling being less at higher clock frequency.

Re: [RESEND PATCH V2 0/9] Implement AMD Pstate EPP Driver

[Huang Rui]

On Tue, Oct 11, 2022 at 12:22:39AM +0800, Yuan, Perry wrote:
> Hi all,
> 
> This patchset implements one new AMD CPU frequency driver
> "amd-pstate-epp” instance for better performance and power control.
> CPPC has a parameter called energy preference performance (EPP).
> The EPP is used in the CCLK 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).
> 
> AMD Energy Performance Preference (EPP) provides a hint to the hardware
> if software wants to bias toward performance (0x0) or energy efficiency (0xff)
> The lowlevel power firmware will calculate the runtime frequency according to the EPP preference 
> value. So the EPP hint will impact the CPU cores frequency responsiveness.
> 
> We use the RAPL interface with "perf" tool to get the energy data of the package power.
> Performance Per Watt (PPW) Calculation:
> 
> The PPW calculation is referred by below paper:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsoftware.intel.com%2Fcontent%2Fdam%2Fdevelop%2Fexternal%2Fus%2Fen%2Fdocuments%2Fperformance-per-what-paper.pdf&data=04%7C01%7CPerry.Yuan%40amd.com%7Cac66e8ce98044e9b062708d9ab47c8d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637729147708574423%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=TPOvCE%2Frbb0ptBreWNxHqOi9YnVhcHGKG88vviDLb00%3D&reserved=0
> 
> Below formula is referred from below spec to measure the PPW:
> 
> (F / t) / P = F * t / (t * E) = F / E,
> 
> "F" is the number of frames per second.
> "P" is power measured in watts.
> "E" is energy measured in joules.
> 
> Gitsouce Benchmark Data on ROME Server CPU
> +------------------------------+------------------------------+------------+------------------+
> | Kernel Module                | PPW (1 / s * J)              |Energy(J) | PPW Improvement (%)|
> +==============================+==============================+============+==================+
> | acpi-cpufreq:schedutil       | 5.85658E-05                  | 17074.8    | base             |
> +------------------------------+------------------------------+------------+------------------+
> | acpi-cpufreq:ondemand        | 5.03079E-05                  | 19877.6    | -14.10%          |
> +------------------------------+------------------------------+------------+------------------+
> | acpi-cpufreq:performance     | 5.88132E-05                  | 17003      | 0.42%            |
> +------------------------------+------------------------------+------------+------------------+
> | amd-pstate:ondemand          | 4.60295E-05                  | 21725.2    | -21.41%          |
> +------------------------------+------------------------------+------------+------------------+
> | amd-pstate:schedutil         | 4.70026E-05                  | 21275.4    | -19.7%           |
> +------------------------------+------------------------------+------------+------------------+
> | amd-pstate:performance       | 5.80094E-05                  | 17238.6    | -0.95%           |
> +------------------------------+------------------------------+------------+------------------+
> | EPP:performance              | 5.8292E-05                   | 17155      | -0.47%           |
> +------------------------------+------------------------------+------------+------------------+
> | EPP: balance performance:    | 6.71709E-05                  | 14887.4    | 14.69%           |
> +------------------------------+------------------------------+------------+------------------+
> | EPP:power                    | 6.66951E-05                  | 4993.6     | 13.88%           |
> +------------------------------+------------------------------+------------+------------------+
> 
> Tbench Benchmark Data on ROME Server CPU
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | Kernel Module                               | PPW MB / (s * J)  |Throughput(MB/s)| Energy (J)|PPW Improvement(%)|
> +=============================================+===================+==============+=============+==================+
> | acpi_cpufreq: schedutil                     | 46.39             | 17191        | 37057.3     | base             |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | acpi_cpufreq: ondemand                      | 51.51             | 19269.5      | 37406.5     | 11.04 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | acpi_cpufreq: performance                   | 45.96             | 17063.7      | 37123.7     | -0.74 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | EPP:powersave: performance(0)               | 54.46             | 20263.1      | 37205       | 17.87 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | EPP:powersave: balance performance          | 55.03             | 20481.9      | 37221.5     | 19.14 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | EPP:powersave: balance_power                | 54.43             | 20245.9      | 37194.2     | 17.77 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | EPP:powersave: power(255)                   | 54.26             | 20181.7      | 37197.4     | 17.40 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | amd-pstate: schedutil                       | 48.22             | 17844.9      | 37006.6     | 3.80 %           |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | amd-pstate: ondemand                        | 61.30             | 22988        | 37503.4     | 33.72 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> | amd-pstate: performance                     | 54.52             | 20252.6      | 37147.8     | 17.81 %          |
> +---------------------------------------------+-------------------+--------------+-------------+------------------+
> 

Please add the test cycle number in the average test result.

Since Jasmine has submitted the TBench and Gitsource benchmark test suite
into amd-pstate-ut test module, I suggest to align the test script and make
sure everybody can use that script to get the similar accurate performance
per watt data even on the EPP mode.

https://lore.kernel.org/lkml/20220914061105.1982477-1-li.meng@amd.com/

Thanks,
Ray

> changes from v1:
>  * rebased to v6.0
>  * drive feedbacks from Mario for the suspend/resume patch
>  * drive feedbacks from Nathan for the EPP support on msr type
>  * fix some typos and code style indent problems
>  * update commit comments for patch 4/7
>  * change the `epp_enabled` module param name to `epp`
>  * set the default epp mode to be false
>  * add testing for the x86_energy_perf_policy utility patchset(will
>    send that utility patchset with another thread)
> 
> Perry Yuan (9):
>   ACPI: CPPC: Add AMD pstate energy performance preference cppc control
>   cpufreq: amd_pstate: add module parameter to load amd pstate EPP
>     driver
>   cpufreq: cpufreq: export cpufreq cpu release and acquire
>   x86/msr: Add the MSR definition for AMD CPPC boost state
>   Documentation: amd-pstate: add EPP profiles introduction
>   cpufreq: amd_pstate: add AMD pstate EPP support for shared memory type
>     processor
>   cpufreq: amd_pstate: add AMD Pstate EPP support for the MSR based
>     processors
>   cpufreq: amd_pstate: implement amd pstate cpu online and offline
>     callback
>   cpufreq: amd-pstate: implement suspend and resume callbacks
> 
>  Documentation/admin-guide/pm/amd-pstate.rst |  19 +
>  arch/x86/include/asm/msr-index.h            |   7 +
>  drivers/acpi/cppc_acpi.c                    | 128 ++-
>  drivers/cpufreq/amd-pstate.c                | 949 +++++++++++++++++++-
>  drivers/cpufreq/cpufreq.c                   |   2 +
>  include/acpi/cppc_acpi.h                    |  17 +
>  6 files changed, 1115 insertions(+), 7 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [RESEND PATCH V2 1/9] ACPI: CPPC: Add AMD pstate energy performance preference cppc control

[Huang Rui]

On Tue, Oct 11, 2022 at 12:22:40AM +0800, Yuan, Perry wrote:
> 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.
> 

The energy performance preference register is actually defined by ACPI, so
it's not the AMD specific stuff. It's correct to define a general function
in cppc_acpi lib not only for AMD processors. We won't need to mention "AMD
P-State" energy performance preference whatever it is implemented with MSR
or SystemMemory in the subject and commit message here.

> 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 <Perry.Yuan@amd.com>
> ---
>  drivers/acpi/cppc_acpi.c | 128 ++++++++++++++++++++++++++++++++++++++-
>  include/acpi/cppc_acpi.h |  17 ++++++
>  2 files changed, 144 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 093675b1a1ff..b0e7817cb97f 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -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;
> +
> +	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");
> +
> +	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 -EIO;

Should be -ENODEV. That means no subspace.

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

The same comment with above, should be -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
> + */
> +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);
> +		return -EINVAL;
> +	}
> +
> +	epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
> +
> +	if (CPC_IN_PCC(epp_set_reg)) {
> +		if (pcc_ss_id < 0)
> +			return -EIO;

The same comment with above, should be -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 */
> +		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> +		up_write(&pcc_ss_data->pcc_lock);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
> +

Most of cppc_set_auto_epp and cppc_set_epp_perf implementations are
duplicated. Could you use one general function?

>  /**
>   * cppc_set_enable - Set to enable CPPC on the processor by writing the
>   * Continuous Performance Control package EnableRegister field.
> @@ -1400,7 +1526,7 @@ int cppc_set_enable(int cpu, bool enable)
>  		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 */
> +		/* after writing CPC, transfer the ownership of PCC to platform */

There is no change with this line.

Thanks,
Ray

>  		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>  		up_write(&pcc_ss_data->pcc_lock);
>  		return ret;
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index c5614444031f..10d91aeedaca 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -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*/
> -- 
> 2.34.1
> 

Re: [RESEND PATCH V2 2/9] cpufreq: amd_pstate: add module parameter to load amd pstate EPP driver

[Huang Rui]

On Tue, Oct 11, 2022 at 12:22:41AM +0800, Yuan, Perry wrote:
> The amd_pstate mode parameter will allow user to select which amd pstate
> working mode as booting mode, amd_pstate instance or amd_pstate_epp instance.
> 
> 1) amd_pstate instance is depending on the target operation mode.
> 2) amd_pstate_epp instance is depending on the set_policy operation mode.It
>    is also called active mode that AMD SMU has EPP algorithm to control the
>    CPU runtime frequency according to the EPP set value and workload.
> 

There is one concern here: how can the user know which kind of processor
supports the EPP function in hardware?

Thanks,
Ray

> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index f52b8f2fe529..2d28f458589c 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -63,6 +63,10 @@ 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 epp = false;
> +module_param(epp, bool, 0444);
> +MODULE_PARM_DESC(epp, "Enable energy performance preference (EPP) control");
> +
>  static struct cpufreq_driver amd_pstate_driver;
>  
>  /**
> -- 
> 2.34.1
> 

Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Huang Rui]

On Tue, Oct 11, 2022 at 12:22:43AM +0800, Yuan, Perry wrote:
> This MSR can be used to check whether the CPU frequency boost state
> is enabled in the hardware control. User can change the boost state in
> the BIOS setting,amd_pstate driver will update the boost state according
> to this msr value.
> 
> AMD Processor Programming Reference (PPR)
> Link: https://www.amd.com/system/files/TechDocs/40332.pdf [p1095]
> Link: https://www.amd.com/system/files/TechDocs/56569-A1-PUB.zip [p162]
> 
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
>  arch/x86/include/asm/msr-index.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 6674bdb096f3..e5ea1c9f747b 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -569,6 +569,7 @@
>  #define MSR_AMD_CPPC_CAP2		0xc00102b2
>  #define MSR_AMD_CPPC_REQ		0xc00102b3
>  #define MSR_AMD_CPPC_STATUS		0xc00102b4
> +#define MSR_AMD_CPPC_HW_CTL		0xc0010015
>  
>  #define AMD_CPPC_LOWEST_PERF(x)		(((x) >> 0) & 0xff)
>  #define AMD_CPPC_LOWNONLIN_PERF(x)	(((x) >> 8) & 0xff)
> @@ -579,6 +580,8 @@
>  #define AMD_CPPC_MIN_PERF(x)		(((x) & 0xff) << 8)
>  #define AMD_CPPC_DES_PERF(x)		(((x) & 0xff) << 16)
>  #define AMD_CPPC_ENERGY_PERF_PREF(x)	(((x) & 0xff) << 24)
> +#define AMD_CPPC_PRECISION_BOOST_BIT   25
> +#define AMD_CPPC_PRECISION_BOOST_ENABLED       BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT)

I had commented the MSR_AMD_CPPC_HW_CTL is duplicated with MSR_K7_HWCR

https://lore.kernel.org/lkml/YtX+uF/nAIG0ykHN@amd.com/
https://lore.kernel.org/lkml/YtX586RDd9Xw44IO@amd.com/

Could you please make sure address the commments?

Thanks,
Ray

Re: [RESEND PATCH V2 6/9] cpufreq: amd_pstate: add AMD pstate EPP support for shared memory type processor

[Huang Rui]

On Tue, Oct 11, 2022 at 12:22:45AM +0800, Yuan, Perry wrote:
> Add Energy Performance Preference support for AMD SOCs which do not
> contain a designated MSR for CPPC support. A shared memory interface
> is used for CPPC on these SOCs and the ACPI PCC channel is used to
> enable EPP and reset the desired performance.
> 
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 2d28f458589c..08f9e335f97c 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -135,12 +135,25 @@ static inline int pstate_enable(bool enable)
>  
>  static int cppc_enable(bool enable)
>  {
> +	struct cppc_perf_ctrls perf_ctrls;
>  	int cpu, ret = 0;
>  
>  	for_each_present_cpu(cpu) {
>  		ret = cppc_set_enable(cpu, enable);
>  		if (ret)
>  			return ret;
> +		if (epp) {
> +			/* Enable autonomous mode for EPP */
> +			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;
> +		}

This patch only writes the desired_perf as 0 to enable the EPP function,
but it cannot be an independent function or patch without the dependency of
the next one (patch 7).

Thanks,
Ray

Re: [RESEND PATCH V2 7/9] cpufreq: amd_pstate: add AMD Pstate EPP support for the MSR based processors

[Huang Rui]

On Tue, Oct 11, 2022 at 12:22:46AM +0800, Yuan, Perry wrote:
> 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 <Perry.Yuan@amd.com>
> ---
>  arch/x86/include/asm/msr-index.h |   4 +
>  drivers/cpufreq/amd-pstate.c     | 795 ++++++++++++++++++++++++++++++-
>  2 files changed, 792 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index e5ea1c9f747b..53cbdb0c522b 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -588,6 +588,10 @@
>  #define MSR_AMD64_PERF_CNTR_GLOBAL_CTL		0xc0000301
>  #define MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR	0xc0000302
>  
> +#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

Please move this definitions into amd-pstate.c. Even on shared memory
processors, we still need this macros as well. They are not MSR specific.

>  /* Fam 17h MSRs */
>  #define MSR_F17H_IRPERF			0xc00000e9
>  
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 08f9e335f97c..58418808aadf 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -67,7 +67,8 @@ static bool epp = false;
>  module_param(epp, bool, 0444);
>  MODULE_PARM_DESC(epp, "Enable energy performance preference (EPP) control");
>  
> -static struct cpufreq_driver amd_pstate_driver;
> +static struct cpufreq_driver *default_pstate_driver;
> +static struct amd_cpudata **all_cpu_data;
>  

Why do we need a global all_cpu_data here? We can get the cpudata from the
private data in cpufreq_policy.

>  /**
>   * struct  amd_aperf_mperf
> @@ -79,6 +80,7 @@ struct amd_aperf_mperf {
>  	u64 aperf;
>  	u64 mperf;
>  	u64 tsc;
> +	u64 time;
>  };
>  
>  /**
> @@ -101,7 +103,19 @@ struct amd_aperf_mperf {
>   * @prev: Last Aperf/Mperf/tsc count value read from register
>   * @freq: current cpu frequency value
>   * @boost_supported: check whether the Processor or SBIOS supports boost mode
> - *
> + * @epp_powersave: Last saved CPPC energy performance preference
> +				when policy switched to performance
> + * @epp_policy: Last saved policy used to set energy-performance preference
> + * @epp_cached: Cached CPPC energy-performance preference value
> + * @policy: Cpufreq policy value
> + * @sched_flags: Store scheduler flags for possible cross CPU update
> + * @update_util_set: CPUFreq utility callback is set
> + * @last_update: Time stamp of the last performance state update
> + * @cppc_boost_min: Last CPPC boosted min performance state
> + * @cppc_cap1_cached: Cached value of the last CPPC Capabilities MSR
> + * @update_util: Cpufreq utility callback information
> + * @sample: the stored performance sample
> +
>   * The amd_cpudata is key private data for each CPU thread in AMD P-State, and
>   * represents all the attributes and goals that AMD P-State requests at runtime.
>   */
> @@ -126,8 +140,197 @@ struct amd_cpudata {
>  
>  	u64 	freq;
>  	bool	boost_supported;
> +	u64	cppc_hw_conf_cached;
> +
> +	/* EPP feature related attributes*/
> +	s16	epp_powersave;
> +	s16	epp_policy;
> +	s16	epp_cached;
> +	u32	policy;
> +	u32	sched_flags;
> +	bool	update_util_set;
> +	u64	last_update;
> +	u64	last_io_update;
> +	u32	cppc_boost_min;
> +	u64	cppc_cap1_cached;
> +	struct	update_util_data update_util;
> +	struct	amd_aperf_mperf sample;
> +};
> +
> +/**
> + * struct amd_pstate_params - global parameters for the performance control
> + * @ cppc_boost_disabled wheher the core performance boost disabled
> + */
> +struct amd_pstate_params {
> +	bool cppc_boost_disabled;
> +};
> +
> +/*
> + * AMD Energy Preference Performance (EPP)
> + * The EPP is used in the CCLK 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)
> + * display strings corresponding to EPP index in the
> + * energy_perf_strings[]
> + *	index		String
> + *-------------------------------------
> + *	0		default
> + *	1		performance
> + *	2		balance_performance
> + *	3		balance_power
> + *	4		power
> + */
> +enum energy_perf_value_index {
> +	EPP_INDEX_DEFAULT = 0,
> +	EPP_INDEX_PERFORMANCE,
> +	EPP_INDEX_BALANCE_PERFORMANCE,
> +	EPP_INDEX_BALANCE_POWERSAVE,
> +	EPP_INDEX_POWERSAVE,
> +};
> +
> +static const char * const energy_perf_strings[] = {
> +	[EPP_INDEX_DEFAULT] = "default",
> +	[EPP_INDEX_PERFORMANCE] = "performance",
> +	[EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
> +	[EPP_INDEX_BALANCE_POWERSAVE] = "balance_power",
> +	[EPP_INDEX_POWERSAVE] = "power",
> +	NULL
> +};
> +
> +static unsigned int epp_values[] = {
> +	[EPP_INDEX_DEFAULT] = 0,
> +	[EPP_INDEX_PERFORMANCE] = AMD_CPPC_EPP_PERFORMANCE,
> +	[EPP_INDEX_BALANCE_PERFORMANCE] = AMD_CPPC_EPP_BALANCE_PERFORMANCE,
> +	[EPP_INDEX_BALANCE_POWERSAVE] = AMD_CPPC_EPP_BALANCE_POWERSAVE,
> +	[EPP_INDEX_POWERSAVE] = AMD_CPPC_EPP_POWERSAVE,
>  };
>  
> +static struct amd_pstate_params global_params;
> +
> +static DEFINE_MUTEX(amd_pstate_limits_lock);
> +static DEFINE_MUTEX(amd_pstate_driver_lock);
> +static DEFINE_SPINLOCK(amd_pstate_cpu_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)
>  {
>  	return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable);
> @@ -496,7 +699,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)
> @@ -660,10 +863,108 @@ 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);
> +	if (ret < 0) {
> +		ret = kstrtouint(buf, 10, &epp);
> +		if (ret)
> +			return ret;
> +
> +		if ((epp > 255) || (epp < 0))
> +			return -EINVAL;
> +
> +		raw = true;
> +	}
> +
> +	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]);
> +}
> +
> +static void amd_pstate_update_policies(void)
> +{
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu)
> +		cpufreq_update_policy(cpu);
> +}
> +
> +static ssize_t show_pstate_dynamic_boost(struct kobject *kobj,
> +				struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%u\n", cppc_boost);
> +}
> +
> +static ssize_t store_pstate_dynamic_boost(struct kobject *a,
> +				       struct kobj_attribute *b,
> +				       const char *buf, size_t count)
> +{
> +	unsigned int input;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &input);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&amd_pstate_driver_lock);
> +	cppc_boost = !!input;
> +	amd_pstate_update_policies();
> +	mutex_unlock(&amd_pstate_driver_lock);
> +
> +	return count;
> +}
> +
>  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);
> +define_one_global_rw(pstate_dynamic_boost);
>  
>  static struct freq_attr *amd_pstate_attr[] = {
>  	&amd_pstate_max_freq,
> @@ -672,6 +973,424 @@ 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 struct attribute *pstate_global_attributes[] = {
> +	&pstate_dynamic_boost.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group amd_pstate_global_attr_group = {
> +	.attrs = pstate_global_attributes,
> +};
> +
> +static inline void update_boost_state(void)
> +{
> +	u64 misc_en;
> +	struct amd_cpudata *cpudata;
> +
> +	cpudata = all_cpu_data[0];
> +	rdmsrl(MSR_AMD_CPPC_HW_CTL, misc_en);
> +	global_params.cppc_boost_disabled = misc_en & AMD_CPPC_PRECISION_BOOST_ENABLED;
> +}

The legacy HW_CTL register is used for legacy ACPI P-State that I mentioned
before. We cannot mix them up with the CPPC driver.

If you encountere any problem, we should handle this in the firmware.

> +
> +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
> +	 * the default cpufreq governor is neither powersave nor performance.
> +	 */
> +	policy->policy = CPUFREQ_POLICY_POWERSAVE;
> +
> +	return 0;
> +}
> +
> +static int amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
> +{
> +	pr_debug("amd-pstate: CPU %d exiting\n", policy->cpu);
> +	policy->fast_switch_possible = false;
> +	return 0;
> +}
> +
> +static void amd_pstate_update_max_freq(unsigned int cpu)
> +{
> +	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
> +
> +	if (!policy)
> +		return;
> +
> +	refresh_frequency_limits(policy);
> +	cpufreq_cpu_release(policy);
> +}
> +
> +static void amd_pstate_epp_update_limits(unsigned int cpu)
> +{
> +	mutex_lock(&amd_pstate_driver_lock);
> +	update_boost_state();
> +	if (global_params.cppc_boost_disabled) {
> +		for_each_possible_cpu(cpu)
> +			amd_pstate_update_max_freq(cpu);
> +	} else {
> +		cpufreq_update_policy(cpu);
> +	}
> +	mutex_unlock(&amd_pstate_driver_lock);
> +}
> +
> +static int cppc_boost_hold_time_ns = 3 * NSEC_PER_MSEC;
> +
> +static inline void amd_pstate_boost_up(struct amd_cpudata *cpudata)
> +{
> +	u64 hwp_req = READ_ONCE(cpudata->cppc_req_cached);
> +	u64 hwp_cap = READ_ONCE(cpudata->cppc_cap1_cached);
> +	u32 max_limit = (hwp_req & 0xff);
> +	u32 min_limit = (hwp_req & 0xff00) >> 8;
> +	u32 boost_level1;
> +
> +	/* If max and min are equal or already at max, nothing to boost */
> +	if (max_limit == min_limit)
> +		return;
> +
> +	/* Set boost max and min to initial value */
> +	if (!cpudata->cppc_boost_min)
> +		cpudata->cppc_boost_min = min_limit;
> +
> +	boost_level1 = ((AMD_CPPC_NOMINAL_PERF(hwp_cap) + min_limit) >> 1);
> +
> +	if (cpudata->cppc_boost_min < boost_level1)
> +		cpudata->cppc_boost_min = boost_level1;
> +	else if (cpudata->cppc_boost_min < AMD_CPPC_NOMINAL_PERF(hwp_cap))
> +		cpudata->cppc_boost_min = AMD_CPPC_NOMINAL_PERF(hwp_cap);
> +	else if (cpudata->cppc_boost_min == AMD_CPPC_NOMINAL_PERF(hwp_cap))
> +		cpudata->cppc_boost_min = max_limit;
> +	else
> +		return;
> +
> +	hwp_req &= ~AMD_CPPC_MIN_PERF(~0L);
> +	hwp_req |= AMD_CPPC_MIN_PERF(cpudata->cppc_boost_min);
> +	wrmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, hwp_req);
> +	cpudata->last_update = cpudata->sample.time;
> +}
> +
> +static inline void amd_pstate_boost_down(struct amd_cpudata *cpudata)
> +{
> +	bool expired;
> +
> +	if (cpudata->cppc_boost_min) {
> +		expired = time_after64(cpudata->sample.time, cpudata->last_update +
> +					cppc_boost_hold_time_ns);
> +
> +		if (expired) {
> +			wrmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
> +						cpudata->cppc_req_cached);
> +			cpudata->cppc_boost_min = 0;
> +		}
> +	}
> +
> +	cpudata->last_update = cpudata->sample.time;
> +}
> +
> +static inline void amd_pstate_boost_update_util(struct amd_cpudata *cpudata,
> +						      u64 time)
> +{
> +	cpudata->sample.time = time;
> +	if (smp_processor_id() != cpudata->cpu)
> +		return;
> +
> +	if (cpudata->sched_flags & SCHED_CPUFREQ_IOWAIT) {
> +		bool do_io = false;
> +
> +		cpudata->sched_flags = 0;
> +		/*
> +		 * Set iowait_boost flag and update time. Since IO WAIT flag
> +		 * is set all the time, we can't just conclude that there is
> +		 * some IO bound activity is scheduled on this CPU with just
> +		 * one occurrence. If we receive at least two in two
> +		 * consecutive ticks, then we treat as boost candidate.
> +		 * This is leveraged from Intel Pstate driver.
> +		 */
> +		if (time_before64(time, cpudata->last_io_update + 2 * TICK_NSEC))
> +			do_io = true;
> +
> +		cpudata->last_io_update = time;
> +
> +		if (do_io)
> +			amd_pstate_boost_up(cpudata);
> +
> +	} else {
> +		amd_pstate_boost_down(cpudata);
> +	}
> +}
> +
> +static inline void amd_pstate_cppc_update_hook(struct update_util_data *data,
> +						u64 time, unsigned int flags)
> +{
> +	struct amd_cpudata *cpudata = container_of(data,
> +				struct amd_cpudata, update_util);
> +
> +	cpudata->sched_flags |= flags;
> +
> +	if (smp_processor_id() == cpudata->cpu)
> +		amd_pstate_boost_update_util(cpudata, time);
> +}
> +
> +static void amd_pstate_clear_update_util_hook(unsigned int cpu)
> +{
> +	struct amd_cpudata *cpudata = all_cpu_data[cpu];
> +
> +	if (!cpudata->update_util_set)
> +		return;
> +
> +	cpufreq_remove_update_util_hook(cpu);
> +	cpudata->update_util_set = false;
> +	synchronize_rcu();
> +}
> +
> +static void amd_pstate_set_update_util_hook(unsigned int cpu_num)
> +{
> +	struct amd_cpudata *cpudata = all_cpu_data[cpu_num];
> +
> +	if (!cppc_boost) {
> +		if (cpudata->update_util_set)
> +			amd_pstate_clear_update_util_hook(cpudata->cpu);
> +		return;
> +	}
> +
> +	if (cpudata->update_util_set)
> +		return;
> +
> +	cpudata->sample.time = 0;
> +	cpufreq_add_update_util_hook(cpu_num, &cpudata->update_util,
> +						amd_pstate_cppc_update_hook);
> +	cpudata->update_util_set = true;
> +}
> +
> +static void amd_pstate_epp_init(unsigned int cpu)
> +{
> +	struct amd_cpudata *cpudata = all_cpu_data[cpu];
> +	u32 max_perf, min_perf;
> +	u64 value;
> +	s16 epp;
> +	int ret;
> +
> +	max_perf = READ_ONCE(cpudata->highest_perf);
> +	min_perf = READ_ONCE(cpudata->lowest_perf);
> +
> +	value = READ_ONCE(cpudata->cppc_req_cached);
> +
> +	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
> +		min_perf = max_perf;
> +
> +	/* Initial min/max values for CPPC Performance Controls Register */
> +	value &= ~AMD_CPPC_MIN_PERF(~0L);
> +	value |= AMD_CPPC_MIN_PERF(min_perf);
> +
> +	value &= ~AMD_CPPC_MAX_PERF(~0L);
> +	value |= AMD_CPPC_MAX_PERF(max_perf);
> +
> +	/* CPPC EPP feature require to set zero to the desire perf bit */
> +	value &= ~AMD_CPPC_DES_PERF(~0L);
> +	value |= AMD_CPPC_DES_PERF(0);
> +
> +	if (cpudata->epp_policy == cpudata->policy)
> +		goto skip_epp;
> +
> +	cpudata->epp_policy = cpudata->policy;
> +
> +	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
> +		epp = amd_pstate_get_epp(cpudata, value);
> +		cpudata->epp_powersave = epp;
> +		if (epp < 0)
> +			goto skip_epp;
> +		/* force the epp value to be zero for performance policy */
> +		epp = 0;
> +	} else {
> +		if (cpudata->epp_powersave < 0)
> +			goto skip_epp;
> +		/* Get BIOS pre-defined epp value */
> +		epp = amd_pstate_get_epp(cpudata, value);
> +		if (epp)
> +			goto skip_epp;
> +		epp = cpudata->epp_powersave;
> +	}
> +	/* Set initial EPP value */
> +	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +		value &= ~GENMASK_ULL(31, 24);
> +		value |= (u64)epp << 24;
> +	}
> +
> +skip_epp:
> +	WRITE_ONCE(cpudata->cppc_req_cached, value);
> +	ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> +	if (!ret)
> +		cpudata->epp_cached = epp;
> +}
> +
> +static void amd_pstate_set_max_limits(struct amd_cpudata *cpudata)
> +{
> +	u64 hwp_cap = READ_ONCE(cpudata->cppc_cap1_cached);
> +	u64 hwp_req = READ_ONCE(cpudata->cppc_req_cached);
> +	u32 max_limit = (hwp_cap >> 24) & 0xff;
> +
> +	hwp_req &= ~AMD_CPPC_MIN_PERF(~0L);
> +	hwp_req |= AMD_CPPC_MIN_PERF(max_limit);
> +	wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, hwp_req);
> +}
> +
> +static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
> +{
> +	struct amd_cpudata *cpudata;
> +
> +	if (!policy->cpuinfo.max_freq)
> +		return -ENODEV;
> +
> +	pr_debug("set_policy: cpuinfo.max %u policy->max %u\n",
> +				policy->cpuinfo.max_freq, policy->max);
> +
> +	cpudata = all_cpu_data[policy->cpu];
> +	cpudata->policy = policy->policy;
> +
> +	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +		mutex_lock(&amd_pstate_limits_lock);
> +
> +		if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
> +			amd_pstate_clear_update_util_hook(policy->cpu);
> +			amd_pstate_set_max_limits(cpudata);
> +		} else {
> +			amd_pstate_set_update_util_hook(policy->cpu);
> +		}
> +
> +		if (boot_cpu_has(X86_FEATURE_CPPC))
> +			amd_pstate_epp_init(policy->cpu);
> +
> +		mutex_unlock(&amd_pstate_limits_lock);

Could you please explain why do we need the update CPU utilization here? I
know Intel has the similar implementation, but why do we need them?

> +	}
> +
> +	return 0;
> +}
> +
> +static void amd_pstate_verify_cpu_policy(struct amd_cpudata *cpudata,
> +					   struct cpufreq_policy_data *policy)
> +{
> +	update_boost_state();
> +	cpufreq_verify_within_cpu_limits(policy);
> +}
> +
> +static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
> +{
> +	amd_pstate_verify_cpu_policy(all_cpu_data[policy->cpu], policy);
> +	pr_debug("policy_max =%d, policy_min=%d\n", policy->max, policy->min);
> +	return 0;
> +}
> +
>  static struct cpufreq_driver amd_pstate_driver = {
>  	.flags		= CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
>  	.verify		= amd_pstate_verify,
> @@ -685,8 +1404,20 @@ static struct cpufreq_driver amd_pstate_driver = {
>  	.attr		= amd_pstate_attr,
>  };
>  
> +static struct cpufreq_driver amd_pstate_epp_driver = {
> +	.flags		= CPUFREQ_CONST_LOOPS,
> +	.verify		= amd_pstate_epp_verify_policy,
> +	.setpolicy	= amd_pstate_epp_set_policy,
> +	.init		= amd_pstate_epp_cpu_init,
> +	.exit		= amd_pstate_epp_cpu_exit,
> +	.update_limits	= amd_pstate_epp_update_limits,
> +	.name		= "amd_pstate_epp",
> +	.attr		= amd_pstate_epp_attr,
> +};
> +
>  static int __init amd_pstate_init(void)
>  {
> +	static struct amd_cpudata **cpudata;
>  	int ret;
>  
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> @@ -701,10 +1432,24 @@ static int __init amd_pstate_init(void)
>  	if (cpufreq_get_current_driver())
>  		return -EEXIST;
>  
> +	cpudata = vzalloc(array_size(sizeof(void *), num_possible_cpus()));
> +	if (!cpudata)
> +		return -ENOMEM;
> +	WRITE_ONCE(all_cpu_data, cpudata);
> +
> +	if (epp) {
> +		pr_info("AMD CPPC loading with amd_pstate_epp driver instance.\n");
> +		default_pstate_driver = &amd_pstate_epp_driver;
> +	} else {
> +		pr_info("AMD CPPC loading with amd_pstate driver instance.\n");
> +		default_pstate_driver = &amd_pstate_driver;
> +	}
> +

The pr_info can be handle with one line like here:

pr_info("AMD CPPC loading with %s driver instance.\n", default_pstate_driver->name); 

>  	/* capability check */
>  	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +		if (!epp)
> +			default_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
>  		pr_debug("AMD CPPC MSR based functionality is supported\n");
> -		amd_pstate_driver.adjust_perf = amd_pstate_adjust_perf;
>  	} else if (shared_mem) {
>  		static_call_update(amd_pstate_enable, cppc_enable);
>  		static_call_update(amd_pstate_init_perf, cppc_init_perf);
> @@ -721,19 +1466,55 @@ static int __init amd_pstate_init(void)
>  		return ret;
>  	}
>  
> -	ret = cpufreq_register_driver(&amd_pstate_driver);
> +	ret = cpufreq_register_driver(default_pstate_driver);
>  	if (ret)
> -		pr_err("failed to register amd_pstate_driver with return %d\n",
> +		pr_err("failed to register amd pstate driver with return %d\n",
>  		       ret);
>  
> +	amd_pstate_kobj = kobject_create_and_add("amd-pstate", &cpu_subsys.dev_root->kobj);
> +	if (!amd_pstate_kobj)
> +		pr_err("amd-pstate: Global sysfs registration failed.\n");
> +
> +	ret = sysfs_create_group(amd_pstate_kobj, &amd_pstate_global_attr_group);
> +	if (ret) {
> +		pr_err("amd-pstate: Sysfs attribute export failed with error %d.\n",
> +		       ret);
> +	}
> +
>  	return ret;
>  }
>  
> +static inline void amd_pstate_kobj_cleanup(struct kobject *kobj)
> +{
> +	kobject_del(kobj);
> +	kobject_put(kobj);
> +}
> +
>  static void __exit amd_pstate_exit(void)
>  {
> -	cpufreq_unregister_driver(&amd_pstate_driver);
> +	unsigned int cpu;
> +
> +	cpufreq_unregister_driver(default_pstate_driver);
>  
>  	amd_pstate_enable(false);
> +
> +	sysfs_remove_group(amd_pstate_kobj, &amd_pstate_global_attr_group);
> +	amd_pstate_kobj_cleanup(amd_pstate_kobj);
> +
> +	cpus_read_lock();
> +	for_each_online_cpu(cpu) {
> +		if (all_cpu_data[cpu]) {
> +			if (default_pstate_driver == &amd_pstate_epp_driver)
> +				amd_pstate_clear_update_util_hook(cpu);
> +
> +			spin_lock(&amd_pstate_cpu_lock);
> +			kfree(all_cpu_data[cpu]);
> +			WRITE_ONCE(all_cpu_data[cpu], NULL);
> +			spin_unlock(&amd_pstate_cpu_lock);
> +		}
> +	}
> +	cpus_read_unlock();
> +
>  }
>  
>  module_init(amd_pstate_init);
> -- 
> 2.34.1
> 

RE: [RESEND PATCH V2 2/9] cpufreq: amd_pstate: add module parameter to load amd pstate EPP driver

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Ray. 

> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Monday, October 17, 2022 5:47 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> <Deepak.Sharma@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [RESEND PATCH V2 2/9] cpufreq: amd_pstate: add module
> parameter to load amd pstate EPP driver
> 
> On Tue, Oct 11, 2022 at 12:22:41AM +0800, Yuan, Perry wrote:
> > The amd_pstate mode parameter will allow user to select which amd
> > pstate working mode as booting mode, amd_pstate instance or
> amd_pstate_epp instance.
> >
> > 1) amd_pstate instance is depending on the target operation mode.
> > 2) amd_pstate_epp instance is depending on the set_policy operation
> mode.It
> >    is also called active mode that AMD SMU has EPP algorithm to control the
> >    CPU runtime frequency according to the EPP set value and workload.
> >
> 
> There is one concern here: how can the user know which kind of processor
> supports the EPP function in hardware?
> 
> Thanks,
> Ray

Checked with internal team, I was told that EPP is part of the CPPC implementation, 
The Pstate driver already check if the cppc valid when driver loading, we do not need to check the epp again. 

Perry.

> 
> > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index f52b8f2fe529..2d28f458589c 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -63,6 +63,10 @@ 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 epp = false;
> > +module_param(epp, bool, 0444);
> > +MODULE_PARM_DESC(epp, "Enable energy performance preference (EPP)
> > +control");
> > +
> >  static struct cpufreq_driver amd_pstate_driver;
> >
> >  /**
> > --
> > 2.34.1
> >

RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Ray. 

> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Monday, October 17, 2022 5:57 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> <Deepak.Sharma@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> AMD CPPC boost state
> 
> On Tue, Oct 11, 2022 at 12:22:43AM +0800, Yuan, Perry wrote:
> > This MSR can be used to check whether the CPU frequency boost state is
> > enabled in the hardware control. User can change the boost state in
> > the BIOS setting,amd_pstate driver will update the boost state
> > according to this msr value.
> >
> > AMD Processor Programming Reference (PPR)
> > Link: https://www.amd.com/system/files/TechDocs/40332.pdf [p1095]
> > Link: https://www.amd.com/system/files/TechDocs/56569-A1-PUB.zip
> > [p162]
> >
> > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > ---
> >  arch/x86/include/asm/msr-index.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/msr-index.h
> > b/arch/x86/include/asm/msr-index.h
> > index 6674bdb096f3..e5ea1c9f747b 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -569,6 +569,7 @@
> >  #define MSR_AMD_CPPC_CAP2		0xc00102b2
> >  #define MSR_AMD_CPPC_REQ		0xc00102b3
> >  #define MSR_AMD_CPPC_STATUS		0xc00102b4
> > +#define MSR_AMD_CPPC_HW_CTL		0xc0010015
> >
> >  #define AMD_CPPC_LOWEST_PERF(x)		(((x) >> 0) & 0xff)
> >  #define AMD_CPPC_LOWNONLIN_PERF(x)	(((x) >> 8) & 0xff)
> > @@ -579,6 +580,8 @@
> >  #define AMD_CPPC_MIN_PERF(x)		(((x) & 0xff) << 8)
> >  #define AMD_CPPC_DES_PERF(x)		(((x) & 0xff) << 16)
> >  #define AMD_CPPC_ENERGY_PERF_PREF(x)	(((x) & 0xff) << 24)
> > +#define AMD_CPPC_PRECISION_BOOST_BIT   25
> > +#define AMD_CPPC_PRECISION_BOOST_ENABLED
> BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT)
> 
> I had commented the MSR_AMD_CPPC_HW_CTL is duplicated with
> MSR_K7_HWCR
> 
> https://lore.kernel.org/lkml/YtX+uF/nAIG0ykHN@amd.com/
> https://lore.kernel.org/lkml/YtX586RDd9Xw44IO@amd.com/
> 
> Could you please make sure address the commments?
> 
> Thanks,
> Ray

If I rename that the MSR definition string, that will cause lots of driver file change. 
So I suggest to add one new MSR macro for the CPPC, the MSR_K7_HWCR is  mismatching in the CPPC Pstate driver.
If you refuse to use this new one, I will reuse that old one. 

Perry. 

RE: [RESEND PATCH V2 6/9] cpufreq: amd_pstate: add AMD pstate EPP support for shared memory type processor

[Yuan, Perry]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Monday, October 17, 2022 6:55 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> <Deepak.Sharma@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [RESEND PATCH V2 6/9] cpufreq: amd_pstate: add AMD pstate
> EPP support for shared memory type processor
> 
> On Tue, Oct 11, 2022 at 12:22:45AM +0800, Yuan, Perry wrote:
> > Add Energy Performance Preference support for AMD SOCs which do not
> > contain a designated MSR for CPPC support. A shared memory interface
> > is used for CPPC on these SOCs and the ACPI PCC channel is used to
> > enable EPP and reset the desired performance.
> >
> > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index 2d28f458589c..08f9e335f97c 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -135,12 +135,25 @@ static inline int pstate_enable(bool enable)
> >
> >  static int cppc_enable(bool enable)
> >  {
> > +	struct cppc_perf_ctrls perf_ctrls;
> >  	int cpu, ret = 0;
> >
> >  	for_each_present_cpu(cpu) {
> >  		ret = cppc_set_enable(cpu, enable);
> >  		if (ret)
> >  			return ret;
> > +		if (epp) {
> > +			/* Enable autonomous mode for EPP */
> > +			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;
> > +		}
> 
> This patch only writes the desired_perf as 0 to enable the EPP function, but it
> cannot be an independent function or patch without the dependency of the
> next one (patch 7).
> 
> Thanks,
> Ray

Do you mean that I could squash this patch into Patch 7 ?
If so , I will get this into V3. 

Perry.

RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Yuan, Perry <Perry.Yuan@amd.com>
> Sent: Thursday, October 20, 2022 11:01
> To: Huang, Ray <Ray.Huang@amd.com>
> Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> <Deepak.Sharma@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> AMD CPPC boost state
> 
> [AMD Official Use Only - General]
> 
> Hi Ray.
> 
> > -----Original Message-----
> > From: Huang, Ray <Ray.Huang@amd.com>
> > Sent: Monday, October 17, 2022 5:57 PM
> > To: Yuan, Perry <Perry.Yuan@amd.com>
> > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> > <Deepak.Sharma@amd.com>; Limonciello, Mario
> > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>;
> Meng,
> > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> > AMD CPPC boost state
> >
> > On Tue, Oct 11, 2022 at 12:22:43AM +0800, Yuan, Perry wrote:
> > > This MSR can be used to check whether the CPU frequency boost state is
> > > enabled in the hardware control. User can change the boost state in
> > > the BIOS setting,amd_pstate driver will update the boost state
> > > according to this msr value.
> > >
> > > AMD Processor Programming Reference (PPR)
> > > Link: https://www.amd.com/system/files/TechDocs/40332.pdf [p1095]
> > > Link: https://www.amd.com/system/files/TechDocs/56569-A1-PUB.zip
> > > [p162]
> > >
> > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > ---
> > >  arch/x86/include/asm/msr-index.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/x86/include/asm/msr-index.h
> > > b/arch/x86/include/asm/msr-index.h
> > > index 6674bdb096f3..e5ea1c9f747b 100644
> > > --- a/arch/x86/include/asm/msr-index.h
> > > +++ b/arch/x86/include/asm/msr-index.h
> > > @@ -569,6 +569,7 @@
> > >  #define MSR_AMD_CPPC_CAP2		0xc00102b2
> > >  #define MSR_AMD_CPPC_REQ		0xc00102b3
> > >  #define MSR_AMD_CPPC_STATUS		0xc00102b4
> > > +#define MSR_AMD_CPPC_HW_CTL		0xc0010015
> > >
> > >  #define AMD_CPPC_LOWEST_PERF(x)		(((x) >> 0) & 0xff)
> > >  #define AMD_CPPC_LOWNONLIN_PERF(x)	(((x) >> 8) & 0xff)
> > > @@ -579,6 +580,8 @@
> > >  #define AMD_CPPC_MIN_PERF(x)		(((x) & 0xff) << 8)
> > >  #define AMD_CPPC_DES_PERF(x)		(((x) & 0xff) << 16)
> > >  #define AMD_CPPC_ENERGY_PERF_PREF(x)	(((x) & 0xff) << 24)
> > > +#define AMD_CPPC_PRECISION_BOOST_BIT   25
> > > +#define AMD_CPPC_PRECISION_BOOST_ENABLED
> > BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT)
> >
> > I had commented the MSR_AMD_CPPC_HW_CTL is duplicated with
> > MSR_K7_HWCR
> >
> > https://lore.kernel.org/lkml/YtX+uF/nAIG0ykHN@amd.com/
> > https://lore.kernel.org/lkml/YtX586RDd9Xw44IO@amd.com/
> >
> > Could you please make sure address the commments?
> >
> > Thanks,
> > Ray
> 
> If I rename that the MSR definition string, that will cause lots of driver file
> change.
> So I suggest to add one new MSR macro for the CPPC, the MSR_K7_HWCR is
> mismatching in the CPPC Pstate driver.
> If you refuse to use this new one, I will reuse that old one.

To avoid changing too much stuff at once how about if you give an alias?
IE something like:

#define MSR_AMD_CPPC_HW_CTL MSR_K7_HWCR

> 
> Perry.

RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Yuan, Perry]

[Public]



> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Friday, October 21, 2022 12:05 AM
> To: Yuan, Perry <Perry.Yuan@amd.com>; Huang, Ray
> <Ray.Huang@amd.com>
> Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> <Deepak.Sharma@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> AMD CPPC boost state
> 
> [Public]
> 
> 
> 
> > -----Original Message-----
> > From: Yuan, Perry <Perry.Yuan@amd.com>
> > Sent: Thursday, October 20, 2022 11:01
> > To: Huang, Ray <Ray.Huang@amd.com>
> > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma,
> > Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario
> > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>;
> > Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, Li (Jassmine)
> > <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> > AMD CPPC boost state
> >
> > [AMD Official Use Only - General]
> >
> > Hi Ray.
> >
> > > -----Original Message-----
> > > From: Huang, Ray <Ray.Huang@amd.com>
> > > Sent: Monday, October 17, 2022 5:57 PM
> > > To: Yuan, Perry <Perry.Yuan@amd.com>
> > > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma,
> > > Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario
> > > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>;
> > > Du, Xiaojian <Xiaojian.Du@amd.com>;
> > Meng,
> > > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition
> > > for AMD CPPC boost state
> > >
> > > On Tue, Oct 11, 2022 at 12:22:43AM +0800, Yuan, Perry wrote:
> > > > This MSR can be used to check whether the CPU frequency boost
> > > > state is enabled in the hardware control. User can change the
> > > > boost state in the BIOS setting,amd_pstate driver will update the
> > > > boost state according to this msr value.
> > > >
> > > > AMD Processor Programming Reference (PPR)
> > > > Link: https://www.amd.com/system/files/TechDocs/40332.pdf [p1095]
> > > > Link: https://www.amd.com/system/files/TechDocs/56569-A1-PUB.zip
> > > > [p162]
> > > >
> > > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > > ---
> > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/arch/x86/include/asm/msr-index.h
> > > > b/arch/x86/include/asm/msr-index.h
> > > > index 6674bdb096f3..e5ea1c9f747b 100644
> > > > --- a/arch/x86/include/asm/msr-index.h
> > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > @@ -569,6 +569,7 @@
> > > >  #define MSR_AMD_CPPC_CAP2		0xc00102b2
> > > >  #define MSR_AMD_CPPC_REQ		0xc00102b3
> > > >  #define MSR_AMD_CPPC_STATUS		0xc00102b4
> > > > +#define MSR_AMD_CPPC_HW_CTL		0xc0010015
> > > >
> > > >  #define AMD_CPPC_LOWEST_PERF(x)		(((x) >> 0) & 0xff)
> > > >  #define AMD_CPPC_LOWNONLIN_PERF(x)	(((x) >> 8) & 0xff)
> > > > @@ -579,6 +580,8 @@
> > > >  #define AMD_CPPC_MIN_PERF(x)		(((x) & 0xff) << 8)
> > > >  #define AMD_CPPC_DES_PERF(x)		(((x) & 0xff) << 16)
> > > >  #define AMD_CPPC_ENERGY_PERF_PREF(x)	(((x) & 0xff) << 24)
> > > > +#define AMD_CPPC_PRECISION_BOOST_BIT   25
> > > > +#define AMD_CPPC_PRECISION_BOOST_ENABLED
> > > BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT)
> > >
> > > I had commented the MSR_AMD_CPPC_HW_CTL is duplicated with
> > > MSR_K7_HWCR
> > >
> > > https://lore.kernel.org/lkml/YtX+uF/nAIG0ykHN@amd.com/
> > > https://lore.kernel.org/lkml/YtX586RDd9Xw44IO@amd.com/
> > >
> > > Could you please make sure address the commments?
> > >
> > > Thanks,
> > > Ray
> >
> > If I rename that the MSR definition string, that will cause lots of
> > driver file change.
> > So I suggest to add one new MSR macro for the CPPC, the MSR_K7_HWCR is
> > mismatching in the CPPC Pstate driver.
> > If you refuse to use this new one, I will reuse that old one.
> 
> To avoid changing too much stuff at once how about if you give an alias?
> IE something like:
> 
> #define MSR_AMD_CPPC_HW_CTL MSR_K7_HWCR

I prefer this code suggestion. 
Then the new name string can be matched with CPPC/Pstate driver. 


> 
> >
> > Perry.

Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Huang Rui]

+ Boris,

On Fri, Oct 21, 2022 at 12:05:21AM +0800, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
> > -----Original Message-----
> > From: Yuan, Perry <Perry.Yuan@amd.com>
> > Sent: Thursday, October 20, 2022 11:01
> > To: Huang, Ray <Ray.Huang@amd.com>
> > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> > <Deepak.Sharma@amd.com>; Limonciello, Mario
> > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> > AMD CPPC boost state
> > 
> > [AMD Official Use Only - General]
> > 
> > Hi Ray.
> > 
> > > -----Original Message-----
> > > From: Huang, Ray <Ray.Huang@amd.com>
> > > Sent: Monday, October 17, 2022 5:57 PM
> > > To: Yuan, Perry <Perry.Yuan@amd.com>
> > > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> > > <Deepak.Sharma@amd.com>; Limonciello, Mario
> > > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>;
> > Meng,
> > > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> > > AMD CPPC boost state
> > >
> > > On Tue, Oct 11, 2022 at 12:22:43AM +0800, Yuan, Perry wrote:
> > > > This MSR can be used to check whether the CPU frequency boost state is
> > > > enabled in the hardware control. User can change the boost state in
> > > > the BIOS setting,amd_pstate driver will update the boost state
> > > > according to this msr value.
> > > >
> > > > AMD Processor Programming Reference (PPR)
> > > > Link: https://www.amd.com/system/files/TechDocs/40332.pdf [p1095]
> > > > Link: https://www.amd.com/system/files/TechDocs/56569-A1-PUB.zip
> > > > [p162]
> > > >
> > > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > > ---
> > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/arch/x86/include/asm/msr-index.h
> > > > b/arch/x86/include/asm/msr-index.h
> > > > index 6674bdb096f3..e5ea1c9f747b 100644
> > > > --- a/arch/x86/include/asm/msr-index.h
> > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > @@ -569,6 +569,7 @@
> > > >  #define MSR_AMD_CPPC_CAP2		0xc00102b2
> > > >  #define MSR_AMD_CPPC_REQ		0xc00102b3
> > > >  #define MSR_AMD_CPPC_STATUS		0xc00102b4
> > > > +#define MSR_AMD_CPPC_HW_CTL		0xc0010015
> > > >
> > > >  #define AMD_CPPC_LOWEST_PERF(x)		(((x) >> 0) & 0xff)
> > > >  #define AMD_CPPC_LOWNONLIN_PERF(x)	(((x) >> 8) & 0xff)
> > > > @@ -579,6 +580,8 @@
> > > >  #define AMD_CPPC_MIN_PERF(x)		(((x) & 0xff) << 8)
> > > >  #define AMD_CPPC_DES_PERF(x)		(((x) & 0xff) << 16)
> > > >  #define AMD_CPPC_ENERGY_PERF_PREF(x)	(((x) & 0xff) << 24)
> > > > +#define AMD_CPPC_PRECISION_BOOST_BIT   25
> > > > +#define AMD_CPPC_PRECISION_BOOST_ENABLED
> > > BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT)
> > >
> > > I had commented the MSR_AMD_CPPC_HW_CTL is duplicated with
> > > MSR_K7_HWCR
> > >
> > > https://lore.kernel.org/lkml/YtX+uF/nAIG0ykHN@amd.com/
> > > https://lore.kernel.org/lkml/YtX586RDd9Xw44IO@amd.com/
> > >
> > > Could you please make sure address the commments?
> > >
> > > Thanks,
> > > Ray
> > 
> > If I rename that the MSR definition string, that will cause lots of driver file
> > change.
> > So I suggest to add one new MSR macro for the CPPC, the MSR_K7_HWCR is
> > mismatching in the CPPC Pstate driver.
> > If you refuse to use this new one, I will reuse that old one.
> 
> To avoid changing too much stuff at once how about if you give an alias?
> IE something like:
> 
> #define MSR_AMD_CPPC_HW_CTL MSR_K7_HWCR
> 

The mainly concern is that HWCR is for legacy ACPI P-State control not for
CPPC. I talked with hardware guys before, it's not suggested to mix them up
together. This register has been defined for a long time even before Zen
processor.

Thanks,
Ray

Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Borislav Petkov]

On Fri, Oct 21, 2022 at 01:22:11PM +0800, Huang Rui wrote:
> > > If I rename that the MSR definition string, that will cause lots of driver file
> > > change.
> > > So I suggest to add one new MSR macro for the CPPC, the MSR_K7_HWCR is
> > > mismatching in the CPPC Pstate driver.
> > > If you refuse to use this new one, I will reuse that old one.
> > 
> > To avoid changing too much stuff at once how about if you give an alias?
> > IE something like:
> > 
> > #define MSR_AMD_CPPC_HW_CTL MSR_K7_HWCR

Why would you all even think about adding a new name and not use
MSR_K7_HWCR?

The other code uses it just fine, do git grep MSR_K7_HWCR.

We have waaay too many MSRs, no need to unnecessarily confuse people
with an alias or rename stuff. Just use MSR_K7_HWCR like everything else
does.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Ray.

> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Friday, October 21, 2022 1:22 PM
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> viresh.kumar@linaro.org; Sharma, Deepak <Deepak.Sharma@amd.com>;
> Fontenot, Nathan <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Borislav Petkov <bp@alien8.de>
> Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> AMD CPPC boost state
> 
> + Boris,
> 
> On Fri, Oct 21, 2022 at 12:05:21AM +0800, Limonciello, Mario wrote:
> > [Public]
> >
> >
> >
> > > -----Original Message-----
> > > From: Yuan, Perry <Perry.Yuan@amd.com>
> > > Sent: Thursday, October 20, 2022 11:01
> > > To: Huang, Ray <Ray.Huang@amd.com>
> > > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma,
> > > Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario
> > > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>;
> > > Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, Li (Jassmine)
> > > <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition
> > > for AMD CPPC boost state
> > >
> > > [AMD Official Use Only - General]
> > >
> > > Hi Ray.
> > >
> > > > -----Original Message-----
> > > > From: Huang, Ray <Ray.Huang@amd.com>
> > > > Sent: Monday, October 17, 2022 5:57 PM
> > > > To: Yuan, Perry <Perry.Yuan@amd.com>
> > > > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma,
> > > > Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario
> > > > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > > > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > > > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > > > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>;
> > > Meng,
> > > > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org
> > > > Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition
> > > > for AMD CPPC boost state
> > > >
> > > > On Tue, Oct 11, 2022 at 12:22:43AM +0800, Yuan, Perry wrote:
> > > > > This MSR can be used to check whether the CPU frequency boost
> > > > > state is enabled in the hardware control. User can change the
> > > > > boost state in the BIOS setting,amd_pstate driver will update
> > > > > the boost state according to this msr value.
> > > > >
> > > > > AMD Processor Programming Reference (PPR)
> > > > > Link: https://www.amd.com/system/files/TechDocs/40332.pdf
> > > > > [p1095]
> > > > > Link: https://www.amd.com/system/files/TechDocs/56569-A1-
> PUB.zip
> > > > > [p162]
> > > > >
> > > > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > > > ---
> > > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/arch/x86/include/asm/msr-index.h
> > > > > b/arch/x86/include/asm/msr-index.h
> > > > > index 6674bdb096f3..e5ea1c9f747b 100644
> > > > > --- a/arch/x86/include/asm/msr-index.h
> > > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > > @@ -569,6 +569,7 @@
> > > > >  #define MSR_AMD_CPPC_CAP2		0xc00102b2
> > > > >  #define MSR_AMD_CPPC_REQ		0xc00102b3
> > > > >  #define MSR_AMD_CPPC_STATUS		0xc00102b4
> > > > > +#define MSR_AMD_CPPC_HW_CTL		0xc0010015
> > > > >
> > > > >  #define AMD_CPPC_LOWEST_PERF(x)		(((x) >> 0) & 0xff)
> > > > >  #define AMD_CPPC_LOWNONLIN_PERF(x)	(((x) >> 8) & 0xff)
> > > > > @@ -579,6 +580,8 @@
> > > > >  #define AMD_CPPC_MIN_PERF(x)		(((x) & 0xff) << 8)
> > > > >  #define AMD_CPPC_DES_PERF(x)		(((x) & 0xff) << 16)
> > > > >  #define AMD_CPPC_ENERGY_PERF_PREF(x)	(((x) & 0xff) << 24)
> > > > > +#define AMD_CPPC_PRECISION_BOOST_BIT   25
> > > > > +#define AMD_CPPC_PRECISION_BOOST_ENABLED
> > > > BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT)
> > > >
> > > > I had commented the MSR_AMD_CPPC_HW_CTL is duplicated with
> > > > MSR_K7_HWCR
> > > >
> > > > https://lore.kernel.org/lkml/YtX+uF/nAIG0ykHN@amd.com/
> > > > https://lore.kernel.org/lkml/YtX586RDd9Xw44IO@amd.com/
> > > >
> > > > Could you please make sure address the commments?
> > > >
> > > > Thanks,
> > > > Ray
> > >
> > > If I rename that the MSR definition string, that will cause lots of
> > > driver file change.
> > > So I suggest to add one new MSR macro for the CPPC, the MSR_K7_HWCR
> > > is mismatching in the CPPC Pstate driver.
> > > If you refuse to use this new one, I will reuse that old one.
> >
> > To avoid changing too much stuff at once how about if you give an alias?
> > IE something like:
> >
> > #define MSR_AMD_CPPC_HW_CTL MSR_K7_HWCR
> >
> 
> The mainly concern is that HWCR is for legacy ACPI P-State control not for
> CPPC. I talked with hardware guys before, it's not suggested to mix them up
> together. This register has been defined for a long time even before Zen
> processor.
> 
> Thanks,
> Ray

I have removed the code not to write boost state to that MSR, just check the boost state from the MSR bit value.
It will not cause any problems, I have tested and confirmed that the BIT value will be changed after BOOST ON/OFF switched in BIOS setting. 
So we can just check the boost state here for pstate driver notification. 

Perry. 

RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Boris.

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Friday, October 21, 2022 5:38 PM
> To: Huang, Ray <Ray.Huang@amd.com>
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Yuan, Perry
> <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> viresh.kumar@linaro.org; Sharma, Deepak <Deepak.Sharma@amd.com>;
> Fontenot, Nathan <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> AMD CPPC boost state
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> On Fri, Oct 21, 2022 at 01:22:11PM +0800, Huang Rui wrote:
> > > > If I rename that the MSR definition string, that will cause lots
> > > > of driver file change.
> > > > So I suggest to add one new MSR macro for the CPPC, the
> > > > MSR_K7_HWCR is mismatching in the CPPC Pstate driver.
> > > > If you refuse to use this new one, I will reuse that old one.
> > >
> > > To avoid changing too much stuff at once how about if you give an alias?
> > > IE something like:
> > >
> > > #define MSR_AMD_CPPC_HW_CTL MSR_K7_HWCR
> 
> Why would you all even think about adding a new name and not use
> MSR_K7_HWCR?
> 
> The other code uses it just fine, do git grep MSR_K7_HWCR.
> 
> We have waaay too many MSRs, no need to unnecessarily confuse people
> with an alias or rename stuff. Just use MSR_K7_HWCR like everything else
> does.
> 
> --
> Regards/Gruss,
>     Boris.
> 

Sure, we can continue to use that MSR definition as you suggested.
Thank you comment on this.

Perry.

> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeop
> le.kernel.org%2Ftglx%2Fnotes-about-
> netiquette&amp;data=05%7C01%7CPerry.Yuan%40amd.com%7Ce3d156c553
> 2a490a2f2d08dab347f1e8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> C0%7C638019418849896672%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&amp;sdata=wVEYx3BNksqXxaycPl81BjVotrWMlcJnVNdbFO7bQto%
> 3D&amp;reserved=0

Re: [RESEND PATCH V2 6/9] cpufreq: amd_pstate: add AMD pstate EPP support for shared memory type processor

[Huang Rui]

On Fri, Oct 21, 2022 at 12:04:26AM +0800, Yuan, Perry wrote:
> [AMD Official Use Only - General]
> 
> 
> 
> > -----Original Message-----
> > From: Huang, Ray <Ray.Huang@amd.com>
> > Sent: Monday, October 17, 2022 6:55 PM
> > To: Yuan, Perry <Perry.Yuan@amd.com>
> > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> > <Deepak.Sharma@amd.com>; Limonciello, Mario
> > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [RESEND PATCH V2 6/9] cpufreq: amd_pstate: add AMD pstate
> > EPP support for shared memory type processor
> > 
> > On Tue, Oct 11, 2022 at 12:22:45AM +0800, Yuan, Perry wrote:
> > > Add Energy Performance Preference support for AMD SOCs which do not
> > > contain a designated MSR for CPPC support. A shared memory interface
> > > is used for CPPC on these SOCs and the ACPI PCC channel is used to
> > > enable EPP and reset the desired performance.
> > >
> > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > ---
> > >  drivers/cpufreq/amd-pstate.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/cpufreq/amd-pstate.c
> > > b/drivers/cpufreq/amd-pstate.c index 2d28f458589c..08f9e335f97c 100644
> > > --- a/drivers/cpufreq/amd-pstate.c
> > > +++ b/drivers/cpufreq/amd-pstate.c
> > > @@ -135,12 +135,25 @@ static inline int pstate_enable(bool enable)
> > >
> > >  static int cppc_enable(bool enable)
> > >  {
> > > +	struct cppc_perf_ctrls perf_ctrls;
> > >  	int cpu, ret = 0;
> > >
> > >  	for_each_present_cpu(cpu) {
> > >  		ret = cppc_set_enable(cpu, enable);
> > >  		if (ret)
> > >  			return ret;
> > > +		if (epp) {
> > > +			/* Enable autonomous mode for EPP */
> > > +			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;
> > > +		}
> > 
> > This patch only writes the desired_perf as 0 to enable the EPP function, but it
> > cannot be an independent function or patch without the dependency of the
> > next one (patch 7).
> > 
> > Thanks,
> > Ray
> 
> Do you mean that I could squash this patch into Patch 7 ?
> If so , I will get this into V3. 
> 

Yes, thanks.

Ray

Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Huang Rui]

On Mon, Oct 24, 2022 at 10:56:50AM +0800, Yuan, Perry wrote:
> [AMD Official Use Only - General]
> 
> Hi Ray.
> 
> > -----Original Message-----
> > From: Huang, Ray <Ray.Huang@amd.com>
> > Sent: Friday, October 21, 2022 1:22 PM
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> > viresh.kumar@linaro.org; Sharma, Deepak <Deepak.Sharma@amd.com>;
> > Fontenot, Nathan <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Borislav Petkov <bp@alien8.de>
> > Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> > AMD CPPC boost state
> > 
> > + Boris,
> > 
> > On Fri, Oct 21, 2022 at 12:05:21AM +0800, Limonciello, Mario wrote:
> > > [Public]
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Yuan, Perry <Perry.Yuan@amd.com>
> > > > Sent: Thursday, October 20, 2022 11:01
> > > > To: Huang, Ray <Ray.Huang@amd.com>
> > > > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma,
> > > > Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario
> > > > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > > > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > > > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > <Shimmer.Huang@amd.com>;
> > > > Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, Li (Jassmine)
> > > > <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org
> > > > Subject: RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition
> > > > for AMD CPPC boost state
> > > >
> > > > [AMD Official Use Only - General]
> > > >
> > > > Hi Ray.
> > > >
> > > > > -----Original Message-----
> > > > > From: Huang, Ray <Ray.Huang@amd.com>
> > > > > Sent: Monday, October 17, 2022 5:57 PM
> > > > > To: Yuan, Perry <Perry.Yuan@amd.com>
> > > > > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma,
> > > > > Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario
> > > > > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > > > > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > > > > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > > > > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>;
> > > > Meng,
> > > > > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > > > > kernel@vger.kernel.org
> > > > > Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition
> > > > > for AMD CPPC boost state
> > > > >
> > > > > On Tue, Oct 11, 2022 at 12:22:43AM +0800, Yuan, Perry wrote:
> > > > > > This MSR can be used to check whether the CPU frequency boost
> > > > > > state is enabled in the hardware control. User can change the
> > > > > > boost state in the BIOS setting,amd_pstate driver will update
> > > > > > the boost state according to this msr value.
> > > > > >
> > > > > > AMD Processor Programming Reference (PPR)
> > > > > > Link: https://www.amd.com/system/files/TechDocs/40332.pdf
> > > > > > [p1095]
> > > > > > Link: https://www.amd.com/system/files/TechDocs/56569-A1-
> > PUB.zip
> > > > > > [p162]
> > > > > >
> > > > > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > > > > ---
> > > > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/x86/include/asm/msr-index.h
> > > > > > b/arch/x86/include/asm/msr-index.h
> > > > > > index 6674bdb096f3..e5ea1c9f747b 100644
> > > > > > --- a/arch/x86/include/asm/msr-index.h
> > > > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > > > @@ -569,6 +569,7 @@
> > > > > >  #define MSR_AMD_CPPC_CAP2		0xc00102b2
> > > > > >  #define MSR_AMD_CPPC_REQ		0xc00102b3
> > > > > >  #define MSR_AMD_CPPC_STATUS		0xc00102b4
> > > > > > +#define MSR_AMD_CPPC_HW_CTL		0xc0010015
> > > > > >
> > > > > >  #define AMD_CPPC_LOWEST_PERF(x)		(((x) >> 0) & 0xff)
> > > > > >  #define AMD_CPPC_LOWNONLIN_PERF(x)	(((x) >> 8) & 0xff)
> > > > > > @@ -579,6 +580,8 @@
> > > > > >  #define AMD_CPPC_MIN_PERF(x)		(((x) & 0xff) << 8)
> > > > > >  #define AMD_CPPC_DES_PERF(x)		(((x) & 0xff) << 16)
> > > > > >  #define AMD_CPPC_ENERGY_PERF_PREF(x)	(((x) & 0xff) << 24)
> > > > > > +#define AMD_CPPC_PRECISION_BOOST_BIT   25
> > > > > > +#define AMD_CPPC_PRECISION_BOOST_ENABLED
> > > > > BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT)
> > > > >
> > > > > I had commented the MSR_AMD_CPPC_HW_CTL is duplicated with
> > > > > MSR_K7_HWCR
> > > > >
> > > > > https://lore.kernel.org/lkml/YtX+uF/nAIG0ykHN@amd.com/
> > > > > https://lore.kernel.org/lkml/YtX586RDd9Xw44IO@amd.com/
> > > > >
> > > > > Could you please make sure address the commments?
> > > > >
> > > > > Thanks,
> > > > > Ray
> > > >
> > > > If I rename that the MSR definition string, that will cause lots of
> > > > driver file change.
> > > > So I suggest to add one new MSR macro for the CPPC, the MSR_K7_HWCR
> > > > is mismatching in the CPPC Pstate driver.
> > > > If you refuse to use this new one, I will reuse that old one.
> > >
> > > To avoid changing too much stuff at once how about if you give an alias?
> > > IE something like:
> > >
> > > #define MSR_AMD_CPPC_HW_CTL MSR_K7_HWCR
> > >
> > 
> > The mainly concern is that HWCR is for legacy ACPI P-State control not for
> > CPPC. I talked with hardware guys before, it's not suggested to mix them up
> > together. This register has been defined for a long time even before Zen
> > processor.
> > 
> > Thanks,
> > Ray
> 
> I have removed the code not to write boost state to that MSR, just check the boost state from the MSR bit value.
> It will not cause any problems, I have tested and confirmed that the BIT value will be changed after BOOST ON/OFF switched in BIOS setting. 
> So we can just check the boost state here for pstate driver notification. 
> 

If we found MSR_K7_HWCR would impact the max frequency in CPPC, we should
report a defect or issue to firmware team. (Then we can add a quirk
function to workaround this in amd-pstate)

Thanks,
Ray

RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for AMD CPPC boost state

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Ray. 

> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Tuesday, October 25, 2022 8:32 AM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>;
> rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma, Deepak
> <Deepak.Sharma@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Borislav Petkov <bp@alien8.de>
> Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition for
> AMD CPPC boost state
> 
> On Mon, Oct 24, 2022 at 10:56:50AM +0800, Yuan, Perry wrote:
> > [AMD Official Use Only - General]
> >
> > Hi Ray.
> >
> > > -----Original Message-----
> > > From: Huang, Ray <Ray.Huang@amd.com>
> > > Sent: Friday, October 21, 2022 1:22 PM
> > > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > > Cc: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> > > viresh.kumar@linaro.org; Sharma, Deepak <Deepak.Sharma@amd.com>;
> > > Fontenot, Nathan <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>;
> > > Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, Li (Jassmine)
> > > <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Borislav Petkov <bp@alien8.de>
> > > Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR definition
> > > for AMD CPPC boost state
> > >
> > > + Boris,
> > >
> > > On Fri, Oct 21, 2022 at 12:05:21AM +0800, Limonciello, Mario wrote:
> > > > [Public]
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Yuan, Perry <Perry.Yuan@amd.com>
> > > > > Sent: Thursday, October 20, 2022 11:01
> > > > > To: Huang, Ray <Ray.Huang@amd.com>
> > > > > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Sharma,
> > > > > Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario
> > > > > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > > > > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > > > > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > > <Shimmer.Huang@amd.com>;
> > > > > Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, Li (Jassmine)
> > > > > <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > > > > kernel@vger.kernel.org
> > > > > Subject: RE: [RESEND PATCH V2 4/9] x86/msr: Add the MSR
> > > > > definition for AMD CPPC boost state
> > > > >
> > > > > [AMD Official Use Only - General]
> > > > >
> > > > > Hi Ray.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Huang, Ray <Ray.Huang@amd.com>
> > > > > > Sent: Monday, October 17, 2022 5:57 PM
> > > > > > To: Yuan, Perry <Perry.Yuan@amd.com>
> > > > > > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org;
> > > > > > Sharma, Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario
> > > > > > <Mario.Limonciello@amd.com>; Fontenot, Nathan
> > > > > > <Nathan.Fontenot@amd.com>; Deucher, Alexander
> > > > > > <Alexander.Deucher@amd.com>; Huang, Shimmer
> > > > > > <Shimmer.Huang@amd.com>; Du, Xiaojian
> <Xiaojian.Du@amd.com>;
> > > > > Meng,
> > > > > > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org;
> > > > > > linux- kernel@vger.kernel.org
> > > > > > Subject: Re: [RESEND PATCH V2 4/9] x86/msr: Add the MSR
> > > > > > definition for AMD CPPC boost state
> > > > > >
> > > > > > On Tue, Oct 11, 2022 at 12:22:43AM +0800, Yuan, Perry wrote:
> > > > > > > This MSR can be used to check whether the CPU frequency
> > > > > > > boost state is enabled in the hardware control. User can
> > > > > > > change the boost state in the BIOS setting,amd_pstate driver
> > > > > > > will update the boost state according to this msr value.
> > > > > > >
> > > > > > > AMD Processor Programming Reference (PPR)
> > > > > > > Link: https://www.amd.com/system/files/TechDocs/40332.pdf
> > > > > > > [p1095]
> > > > > > > Link: https://www.amd.com/system/files/TechDocs/56569-A1-
> > > PUB.zip
> > > > > > > [p162]
> > > > > > >
> > > > > > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > > > > > ---
> > > > > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > > > > >  1 file changed, 3 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/x86/include/asm/msr-index.h
> > > > > > > b/arch/x86/include/asm/msr-index.h
> > > > > > > index 6674bdb096f3..e5ea1c9f747b 100644
> > > > > > > --- a/arch/x86/include/asm/msr-index.h
> > > > > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > > > > @@ -569,6 +569,7 @@
> > > > > > >  #define MSR_AMD_CPPC_CAP2		0xc00102b2
> > > > > > >  #define MSR_AMD_CPPC_REQ		0xc00102b3
> > > > > > >  #define MSR_AMD_CPPC_STATUS		0xc00102b4
> > > > > > > +#define MSR_AMD_CPPC_HW_CTL		0xc0010015
> > > > > > >
> > > > > > >  #define AMD_CPPC_LOWEST_PERF(x)		(((x) >> 0) &
> 0xff)
> > > > > > >  #define AMD_CPPC_LOWNONLIN_PERF(x)	(((x) >> 8) & 0xff)
> > > > > > > @@ -579,6 +580,8 @@
> > > > > > >  #define AMD_CPPC_MIN_PERF(x)		(((x) & 0xff) << 8)
> > > > > > >  #define AMD_CPPC_DES_PERF(x)		(((x) & 0xff) << 16)
> > > > > > >  #define AMD_CPPC_ENERGY_PERF_PREF(x)	(((x) & 0xff)
> << 24)
> > > > > > > +#define AMD_CPPC_PRECISION_BOOST_BIT   25
> > > > > > > +#define AMD_CPPC_PRECISION_BOOST_ENABLED
> > > > > > BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT)
> > > > > >
> > > > > > I had commented the MSR_AMD_CPPC_HW_CTL is duplicated with
> > > > > > MSR_K7_HWCR
> > > > > >
> > > > > > https://lore.kernel.org/lkml/YtX+uF/nAIG0ykHN@amd.com/
> > > > > > https://lore.kernel.org/lkml/YtX586RDd9Xw44IO@amd.com/
> > > > > >
> > > > > > Could you please make sure address the commments?
> > > > > >
> > > > > > Thanks,
> > > > > > Ray
> > > > >
> > > > > If I rename that the MSR definition string, that will cause lots
> > > > > of driver file change.
> > > > > So I suggest to add one new MSR macro for the CPPC, the
> > > > > MSR_K7_HWCR is mismatching in the CPPC Pstate driver.
> > > > > If you refuse to use this new one, I will reuse that old one.
> > > >
> > > > To avoid changing too much stuff at once how about if you give an alias?
> > > > IE something like:
> > > >
> > > > #define MSR_AMD_CPPC_HW_CTL MSR_K7_HWCR
> > > >
> > >
> > > The mainly concern is that HWCR is for legacy ACPI P-State control
> > > not for CPPC. I talked with hardware guys before, it's not suggested
> > > to mix them up together. This register has been defined for a long
> > > time even before Zen processor.
> > >
> > > Thanks,
> > > Ray
> >
> > I have removed the code not to write boost state to that MSR, just check
> the boost state from the MSR bit value.
> > It will not cause any problems, I have tested and confirmed that the BIT
> value will be changed after BOOST ON/OFF switched in BIOS setting.
> > So we can just check the boost state here for pstate driver notification.
> >
> 
> If we found MSR_K7_HWCR would impact the max frequency in CPPC, we
> should report a defect or issue to firmware team. (Then we can add a quirk
> function to workaround this in amd-pstate)
> 
> Thanks,
> Ray

Sure , I will revise the patch and still using the MSR_K7_HWCR to check freq boost state in pstate driver. 
Thanks for the feedback.

Perry.