[PATCHv3 0/6] CPPC optional registers AMD support

[PATCHv3 0/6] CPPC optional registers AMD support

[Natarajan, Janakarajan]

CPPC (Collaborative Processor Performance Control) offers optional
registers which can be used to tune the system based on energy and/or
performance requirements.

Newer AMD processors (>= Family 17h) add support for a subset of these
optional CPPC registers, based on ACPI v6.1.

The following are the supported CPPC registers for which sysfs entries
are created:
* enable                (NEW)
* max_perf              (NEW)
* min_perf              (NEW)
* energy_perf
* lowest_perf
* nominal_perf
* desired_perf          (NEW)
* feedback_ctrs
* auto_sel_enable       (NEW)
* lowest_nonlinear_perf

First, update cppc_acpi to create sysfs entries only when the optional
registers are known to be supported.

Next, a new CPUFreq driver is introduced to enable the OSPM and the userspace
to access the newly supported registers through sysfs entries found in
/sys/devices/system/cpu/cpu<num>/amd_cpufreq/.

This new CPUFreq driver can only be used by providing a module parameter,
amd_cpufreq.cppc_enable=1.

The purpose of exposing the registers via the amd-cpufreq sysfs entries is to
allow the userspace to:
* Tweak the values to fit its workload.
* Apply a profile from AMD's optimization guides.

Profiles will be documented in the performance/optimization guides.

Note:
* AMD systems will not have a policy applied in the kernel at this time.

TODO:
* Create a linux userspace tool that will help users generate a CPPC profile
  for their target workload.
* Create a general CPPC policy in the kernel.

v1->v2:
* Add macro to ensure BUFFER only registers have BUFFER type.
* Add support macro to make the right check based on register type.
* Remove support checks for registers which are mandatory.

v2->v3:
* Introduce new amd-cpufreq driver which will have priority over acpi-cpufreq.
* Move new sysfs entries creation to amd-cpufreq.

Janakarajan Natarajan (3):
  acpi/cppc: Add macros for CPPC register checks
  acpi/cppc: Ensure only supported CPPC sysfs entries are created
  drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and
    later)

Yazen Ghannam (3):
  acpi/cppc: Rework cppc_set_perf() to use cppc_regs index
  acpi/cppc: Add support for optional CPPC registers
  acpi/cppc: Add support for CPPC Enable register

 drivers/acpi/cppc_acpi.c       | 244 ++++++++++++++++++++++++++++-----
 drivers/cpufreq/Kconfig.x86    |  14 ++
 drivers/cpufreq/Makefile       |   4 +-
 drivers/cpufreq/amd-cpufreq.c  | 233 +++++++++++++++++++++++++++++++
 drivers/cpufreq/cppc_cpufreq.c |   6 +-
 include/acpi/cppc_acpi.h       |  11 +-
 6 files changed, 474 insertions(+), 38 deletions(-)
 create mode 100644 drivers/cpufreq/amd-cpufreq.c

-- 
2.17.1

[PATCHv3 1/6] acpi/cppc: Add macros for CPPC register checks

[Natarajan, Janakarajan]

Introduce two macros to help with checking the support for optional CPPC
registers.

CPC_SUP_BUFFER_ONLY ensures that an expected BUFFER only register has a
register type of ACPI_TYPE_BUFFER and is not NULL.

REG_SUPPORTED decides which check to perform based the expected type of
the CPPC register.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 drivers/acpi/cppc_acpi.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 15f103d7532b..c43de65531ae 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -111,6 +111,14 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
 #define CPC_SUPPORTED(cpc) ((cpc)->type == ACPI_TYPE_INTEGER ?		\
 				!!(cpc)->cpc_entry.int_value :		\
 				!IS_NULL_REG(&(cpc)->cpc_entry.reg))
+
+/*
+ * Evaluates to True if an optional cpc field is supported and is
+ * BUFFER only
+ */
+#define CPC_SUP_BUFFER_ONLY(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&	\
+				  !IS_NULL_REG(&(cpc)->cpc_entry.reg))
+
 /*
  * Arbitrary Retries in case the remote processor is slow to respond
  * to PCC commands. Keeping it high enough to cover emulators where
@@ -705,6 +713,26 @@ static bool is_cppc_supported(int revision, int num_ent)
  *	}
  */
 
+static bool is_buf_only(int reg_idx)
+{
+	switch (reg_idx) {
+	case HIGHEST_PERF:
+	case NOMINAL_PERF:
+	case LOW_NON_LINEAR_PERF:
+	case LOWEST_PERF:
+	case CTR_WRAP_TIME:
+	case AUTO_SEL_ENABLE:
+	case REFERENCE_PERF:
+		return false;
+	default:
+		return true;
+	}
+}
+
+#define REG_SUPPORTED(cpc, idx) (is_buf_only(idx) ?			    \
+				 CPC_SUP_BUFFER_ONLY(&cpc->cpc_regs[idx]) : \
+				 CPC_SUPPORTED(&cpc->cpc_regs[idx]))
+
 /**
  * acpi_cppc_processor_probe - Search for per CPU _CPC objects.
  * @pr: Ptr to acpi_processor containing this CPU's logical ID.
-- 
2.17.1

[PATCHv3 2/6] acpi/cppc: Ensure only supported CPPC sysfs entries are created

[Natarajan, Janakarajan]

Add attributes only for registers that are supported by the platform.
This prevents unsupported, optional registers from having sysfs entries
created.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 drivers/acpi/cppc_acpi.c | 82 +++++++++++++++++++++++++++++++++-------
 1 file changed, 68 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index c43de65531ae..53a9dc9960b6 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -183,22 +183,8 @@ static ssize_t show_feedback_ctrs(struct kobject *kobj,
 }
 define_one_cppc_ro(feedback_ctrs);
 
-static struct attribute *cppc_attrs[] = {
-	&feedback_ctrs.attr,
-	&reference_perf.attr,
-	&wraparound_time.attr,
-	&highest_perf.attr,
-	&lowest_perf.attr,
-	&lowest_nonlinear_perf.attr,
-	&nominal_perf.attr,
-	&nominal_freq.attr,
-	&lowest_freq.attr,
-	NULL
-};
-
 static struct kobj_type cppc_ktype = {
 	.sysfs_ops = &kobj_sysfs_ops,
-	.default_attrs = cppc_attrs,
 };
 
 static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
@@ -733,6 +719,69 @@ static bool is_buf_only(int reg_idx)
 				 CPC_SUP_BUFFER_ONLY(&cpc->cpc_regs[idx]) : \
 				 CPC_SUPPORTED(&cpc->cpc_regs[idx]))
 
+static int is_mandatory_reg(int reg_idx)
+{
+	switch (reg_idx) {
+	case HIGHEST_PERF:
+	case NOMINAL_PERF:
+	case LOW_NON_LINEAR_PERF:
+	case LOWEST_PERF:
+	case REFERENCE_CTR:
+	case DELIVERED_CTR:
+		return 1;
+	}
+
+	return 0;
+}
+
+#define MANDATORY_REG_CNT	6
+
+static int set_cppc_attrs(struct cpc_desc *cpc, int entries)
+{
+	int i, attr_i = 0, opt_reg_cnt;
+	static struct attribute **cppc_attrs;
+
+	cppc_attrs = kcalloc(entries, sizeof(*cppc_attrs), GFP_KERNEL);
+	if (!cppc_attrs)
+		return -ENOMEM;
+
+	/* Set optional regs */
+	opt_reg_cnt = entries - MANDATORY_REG_CNT;
+	for (i = 0; i < MAX_CPC_REG_ENT && attr_i < opt_reg_cnt; i++) {
+		if (is_mandatory_reg(i) || !REG_SUPPORTED(cpc, i))
+			continue;
+
+		switch (i) {
+		case NOMINAL_FREQ:
+			cppc_attrs[attr_i++] = &nominal_freq.attr;
+			break;
+		case LOWEST_FREQ:
+			cppc_attrs[attr_i++] = &lowest_freq.attr;
+			break;
+		case REFERENCE_PERF:
+			cppc_attrs[attr_i++] = &reference_perf.attr;
+			break;
+		case CTR_WRAP_TIME:
+			cppc_attrs[attr_i++] = &wraparound_time.attr;
+			break;
+		}
+	}
+
+	/* Set mandatory regs */
+	cppc_attrs[attr_i++] = &highest_perf.attr;
+	cppc_attrs[attr_i++] = &nominal_perf.attr;
+	cppc_attrs[attr_i++] = &lowest_nonlinear_perf.attr;
+	cppc_attrs[attr_i++] = &lowest_perf.attr;
+
+	/* Set feedback_ctr sysfs entry */
+	cppc_attrs[attr_i] = &feedback_ctrs.attr;
+
+	/* Set kobj_type member */
+	cppc_ktype.default_attrs = cppc_attrs;
+
+	return 0;
+}
+
 /**
  * acpi_cppc_processor_probe - Search for per CPU _CPC objects.
  * @pr: Ptr to acpi_processor containing this CPU's logical ID.
@@ -887,6 +936,10 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	/* Plug PSD data into this CPU's CPC descriptor. */
 	per_cpu(cpc_desc_ptr, pr->id) = cpc_ptr;
 
+	ret = set_cppc_attrs(cpc_ptr, num_ent - 2);
+	if (ret)
+		goto out_free;
+
 	ret = kobject_init_and_add(&cpc_ptr->kobj, &cppc_ktype, &cpu_dev->kobj,
 			"acpi_cppc");
 	if (ret) {
@@ -948,6 +1001,7 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr)
 			iounmap(addr);
 	}
 
+	kfree(cppc_ktype.default_attrs);
 	kobject_put(&cpc_ptr->kobj);
 	kfree(cpc_ptr);
 }
-- 
2.17.1

[PATCHv3 3/6] acpi/cppc: Rework cppc_set_perf() to use cppc_regs index

[Natarajan, Janakarajan]

From: Yazen Ghannam <Yazen.Ghannam@amd.com>

The cppc_set_perf() currently only works for DESIRED_PERF. To make it
generic, pass in the index of the register being accessed.

Also, rename cppc_set_perf() to cppc_set_reg(). This is in preparation
for it to be used for more than just the DESIRED_PERF register.

Signed-off-by: Yazen Ghannam <Yazen.Ghannam@amd.com>
[ carved out into a patch, cleaned up, productized ]
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 drivers/acpi/cppc_acpi.c       | 36 ++++++++++++++++++++++------------
 drivers/cpufreq/cppc_cpufreq.c |  6 +++---
 include/acpi/cppc_acpi.h       |  2 +-
 3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 53a9dc9960b6..c13dacea4a8b 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -56,7 +56,7 @@ struct cppc_pcc_data {
 	/*
 	 * Lock to provide controlled access to the PCC channel.
 	 *
-	 * For performance critical usecases(currently cppc_set_perf)
+	 * For performance-critical usecases(currently cppc_set_reg)
 	 *	We need to take read_lock and check if channel belongs to OSPM
 	 * before reading or writing to PCC subspace
 	 *	We need to take write_lock before transferring the channel
@@ -1341,26 +1341,38 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
 
 /**
- * cppc_set_perf - Set a CPU's performance controls.
- * @cpu: CPU for which to set performance controls.
+ * cppc_set_reg - Set the CPUs control register.
+ * @cpu: CPU for which to set the register.
  * @perf_ctrls: ptr to cppc_perf_ctrls. See cppc_acpi.h
+ * @reg_idx: Index of the register being accessed
  *
  * Return: 0 for success, -ERRNO otherwise.
  */
-int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
+		 enum cppc_regs reg_idx)
 {
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
-	struct cpc_register_resource *desired_reg;
 	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
 	struct cppc_pcc_data *pcc_ss_data = NULL;
+	struct cpc_register_resource *reg;
 	int ret = 0;
+	u32 value;
 
 	if (!cpc_desc) {
 		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
 		return -ENODEV;
 	}
 
-	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
+	switch (reg_idx) {
+	case DESIRED_PERF:
+		value = perf_ctrls->desired_perf;
+		break;
+	default:
+		pr_debug("CPC register index #%d not writeable\n", reg_idx);
+		return -EINVAL;
+	}
+
+	reg = &cpc_desc->cpc_regs[reg_idx];
 
 	/*
 	 * This is Phase-I where we want to write to CPC registers
@@ -1369,7 +1381,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 * Since read_lock can be acquired by multiple CPUs simultaneously we
 	 * achieve that goal here
 	 */
-	if (CPC_IN_PCC(desired_reg)) {
+	if (CPC_IN_PCC(reg)) {
 		if (pcc_ss_id < 0) {
 			pr_debug("Invalid pcc_ss_id\n");
 			return -ENODEV;
@@ -1396,14 +1408,14 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 * Skip writing MIN/MAX until Linux knows how to come up with
 	 * useful values.
 	 */
-	cpc_write(cpu, desired_reg, perf_ctrls->desired_perf);
+	cpc_write(cpu, reg, value);
 
-	if (CPC_IN_PCC(desired_reg))
+	if (CPC_IN_PCC(reg))
 		up_read(&pcc_ss_data->pcc_lock);	/* END Phase-I */
 	/*
 	 * This is Phase-II where we transfer the ownership of PCC to Platform
 	 *
-	 * Short Summary: Basically if we think of a group of cppc_set_perf
+	 * Short Summary: Basically if we think of a group of cppc_set_reg
 	 * requests that happened in short overlapping interval. The last CPU to
 	 * come out of Phase-I will enter Phase-II and ring the doorbell.
 	 *
@@ -1446,7 +1458,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 * case during a CMD_READ and if there are pending writes it delivers
 	 * the write command before servicing the read command
 	 */
-	if (CPC_IN_PCC(desired_reg)) {
+	if (CPC_IN_PCC(reg)) {
 		if (down_write_trylock(&pcc_ss_data->pcc_lock)) {/* BEGIN Phase-II */
 			/* Update only if there are pending write commands */
 			if (pcc_ss_data->pending_pcc_write_cmd)
@@ -1462,7 +1474,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	}
 	return ret;
 }
-EXPORT_SYMBOL_GPL(cppc_set_perf);
+EXPORT_SYMBOL_GPL(cppc_set_reg);
 
 /**
  * cppc_get_transition_latency - returns frequency transition latency in ns
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 8d8da763adc5..81e9dff03c92 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -207,7 +207,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 	freqs.new = target_freq;
 
 	cpufreq_freq_transition_begin(policy, &freqs);
-	ret = cppc_set_perf(cpu->cpu, &cpu->perf_ctrls);
+	ret = cppc_set_reg(cpu->cpu, &cpu->perf_ctrls, DESIRED_PERF);
 	cpufreq_freq_transition_end(policy, &freqs, ret != 0);
 
 	if (ret)
@@ -231,7 +231,7 @@ static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 
 	cpu->perf_ctrls.desired_perf = cpu->perf_caps.lowest_perf;
 
-	ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
+	ret = cppc_set_reg(cpu_num, &cpu->perf_ctrls, DESIRED_PERF);
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
 				cpu->perf_caps.lowest_perf, cpu_num, ret);
@@ -344,7 +344,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 					cpu->perf_caps.highest_perf);
 	cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf;
 
-	ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
+	ret = cppc_set_reg(cpu_num, &cpu->perf_ctrls, DESIRED_PERF);
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
 				cpu->perf_caps.highest_perf, cpu_num, ret);
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index a6a9373ab863..f229e903525d 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -135,7 +135,7 @@ struct cppc_cpudata {
 
 extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
 extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
-extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
+extern int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls, enum cppc_regs reg_idx);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
 extern int acpi_get_psd_map(struct cppc_cpudata **);
 extern unsigned int cppc_get_transition_latency(int cpu);
-- 
2.17.1

[PATCHv3 4/6] acpi/cppc: Add support for optional CPPC registers

[Natarajan, Janakarajan]

From: Yazen Ghannam <Yazen.Ghannam@amd.com>

Newer AMD processors support a subset of the optional CPPC registers.
Add support for these optional registers.

Signed-off-by: Yazen Ghannam <Yazen.Ghannam@amd.com>
[ carved out into a patch, cleaned up, productized ]
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 drivers/acpi/cppc_acpi.c | 88 +++++++++++++++++++++++++++++++++++++---
 include/acpi/cppc_acpi.h |  3 ++
 2 files changed, 86 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index c13dacea4a8b..b24e54263efb 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1367,6 +1367,18 @@ int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
 	case DESIRED_PERF:
 		value = perf_ctrls->desired_perf;
 		break;
+	case MAX_PERF:
+		value = perf_ctrls->max_perf;
+		break;
+	case MIN_PERF:
+		value = perf_ctrls->min_perf;
+		break;
+	case ENERGY_PERF:
+		value = perf_ctrls->energy_perf;
+		break;
+	case AUTO_SEL_ENABLE:
+		value = perf_ctrls->auto_sel_enable;
+		break;
 	default:
 		pr_debug("CPC register index #%d not writeable\n", reg_idx);
 		return -EINVAL;
@@ -1404,11 +1416,8 @@ int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
 		cpc_desc->write_cmd_status = 0;
 	}
 
-	/*
-	 * Skip writing MIN/MAX until Linux knows how to come up with
-	 * useful values.
-	 */
-	cpc_write(cpu, reg, value);
+	if (CPC_SUPPORTED(reg))
+		cpc_write(cpu, reg, value);
 
 	if (CPC_IN_PCC(reg))
 		up_read(&pcc_ss_data->pcc_lock);	/* END Phase-I */
@@ -1476,6 +1485,75 @@ int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
 }
 EXPORT_SYMBOL_GPL(cppc_set_reg);
 
+int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+{
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+	struct cpc_register_resource *desired_reg, *max_reg, *min_reg;
+	struct cpc_register_resource *energy_reg, *auto_sel_enable_reg;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	u64 desired, max, min, energy, auto_sel_enable;
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret = 0, regs_in_pcc = 0;
+
+	if (!cpc_desc) {
+		pr_debug("No CPC descriptor for CPU: %d\n", cpu);
+		return -ENODEV;
+	}
+
+	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
+	max_reg = &cpc_desc->cpc_regs[MAX_PERF];
+	min_reg = &cpc_desc->cpc_regs[MIN_PERF];
+	energy_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+	auto_sel_enable_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
+
+	/* Check if any of the perf registers are in PCC */
+	if (CPC_IN_PCC(desired_reg) || CPC_IN_PCC(max_reg) ||
+	    CPC_IN_PCC(min_reg) || CPC_IN_PCC(energy_reg) ||
+	    CPC_IN_PCC(auto_sel_enable_reg)) {
+		pcc_ss_data = pcc_data[pcc_ss_id];
+		down_write(&pcc_ss_data->pcc_lock);
+		regs_in_pcc = 1;
+
+		/*Ring doorbell once to update PCC subspace */
+		if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
+			ret = -EIO;
+			goto out_err;
+		}
+	}
+
+	/* desired_perf is the only mandatory value in perf_ctrls */
+	if (cpc_read(cpu, desired_reg, &desired))
+		ret = -EFAULT;
+
+	if (CPC_SUP_BUFFER_ONLY(max_reg) && cpc_read(cpu, max_reg, &max))
+		ret = -EFAULT;
+
+	if (CPC_SUP_BUFFER_ONLY(min_reg) && cpc_read(cpu, min_reg, &min))
+		ret = -EFAULT;
+
+	if (CPC_SUP_BUFFER_ONLY(energy_reg) &&
+	    cpc_read(cpu, energy_reg, &energy))
+		ret = -EFAULT;
+
+	if (CPC_SUPPORTED(auto_sel_enable_reg) &&
+	    cpc_read(cpu, auto_sel_enable_reg, &auto_sel_enable))
+		ret = -EFAULT;
+
+	if (!ret) {
+		perf_ctrls->desired_perf = desired;
+		perf_ctrls->max_perf = max;
+		perf_ctrls->min_perf = min;
+		perf_ctrls->energy_perf = energy;
+		perf_ctrls->auto_sel_enable = auto_sel_enable;
+	}
+
+out_err:
+	if (regs_in_pcc)
+		up_write(&pcc_ss_data->pcc_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cppc_get_perf);
+
 /**
  * cppc_get_transition_latency - returns frequency transition latency in ns
  *
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index f229e903525d..80720b246c51 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -113,6 +113,8 @@ struct cppc_perf_ctrls {
 	u32 max_perf;
 	u32 min_perf;
 	u32 desired_perf;
+	u32 auto_sel_enable;
+	u32 energy_perf;
 };
 
 struct cppc_perf_fb_ctrs {
@@ -136,6 +138,7 @@ struct cppc_cpudata {
 extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
 extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
 extern int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls, enum cppc_regs reg_idx);
+extern int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
 extern int acpi_get_psd_map(struct cppc_cpudata **);
 extern unsigned int cppc_get_transition_latency(int cpu);
-- 
2.17.1

[PATCHv3 5/6] acpi/cppc: Add support for CPPC Enable register

[Natarajan, Janakarajan]

From: Yazen Ghannam <Yazen.Ghannam@amd.com>

To enable CPPC on a processor, the OS should write a value "1" to the
CPPC Enable register. Add support for this register.

Since we have a new variable "enable" in cppc_perf_ctrls, rename it
and the associated functions i.e. cppc_perf_ctrls->cppc_ctrls and
cppc_get_perf()->cppc_get_ctrls().

Signed-off-by: Yazen Ghannam <Yazen.Ghannam@amd.com>
[ carved out into a patch, cleaned up, productized ]
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 drivers/acpi/cppc_acpi.c | 44 ++++++++++++++++++++++++----------------
 include/acpi/cppc_acpi.h | 10 +++++----
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index b24e54263efb..3199433e3f71 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1343,12 +1343,12 @@ EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
 /**
  * cppc_set_reg - Set the CPUs control register.
  * @cpu: CPU for which to set the register.
- * @perf_ctrls: ptr to cppc_perf_ctrls. See cppc_acpi.h
+ * @ctrls: ptr to cppc_ctrls. See cppc_acpi.h
  * @reg_idx: Index of the register being accessed
  *
  * Return: 0 for success, -ERRNO otherwise.
  */
-int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
+int cppc_set_reg(int cpu, struct cppc_ctrls *ctrls,
 		 enum cppc_regs reg_idx)
 {
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
@@ -1364,20 +1364,23 @@ int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
 	}
 
 	switch (reg_idx) {
+	case ENABLE:
+		value = ctrls->enable;
+		break;
 	case DESIRED_PERF:
-		value = perf_ctrls->desired_perf;
+		value = ctrls->desired_perf;
 		break;
 	case MAX_PERF:
-		value = perf_ctrls->max_perf;
+		value = ctrls->max_perf;
 		break;
 	case MIN_PERF:
-		value = perf_ctrls->min_perf;
+		value = ctrls->min_perf;
 		break;
 	case ENERGY_PERF:
-		value = perf_ctrls->energy_perf;
+		value = ctrls->energy_perf;
 		break;
 	case AUTO_SEL_ENABLE:
-		value = perf_ctrls->auto_sel_enable;
+		value = ctrls->auto_sel_enable;
 		break;
 	default:
 		pr_debug("CPC register index #%d not writeable\n", reg_idx);
@@ -1485,13 +1488,14 @@ int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
 }
 EXPORT_SYMBOL_GPL(cppc_set_reg);
 
-int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+int cppc_get_ctrls(int cpu, struct cppc_ctrls *ctrls)
 {
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
 	struct cpc_register_resource *desired_reg, *max_reg, *min_reg;
 	struct cpc_register_resource *energy_reg, *auto_sel_enable_reg;
+	struct cpc_register_resource *enable_reg;
 	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
-	u64 desired, max, min, energy, auto_sel_enable;
+	u64 desired, max, min, energy, auto_sel_enable, enable;
 	struct cppc_pcc_data *pcc_ss_data = NULL;
 	int ret = 0, regs_in_pcc = 0;
 
@@ -1500,6 +1504,7 @@ int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 		return -ENODEV;
 	}
 
+	enable_reg = &cpc_desc->cpc_regs[ENABLE];
 	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
 	max_reg = &cpc_desc->cpc_regs[MAX_PERF];
 	min_reg = &cpc_desc->cpc_regs[MIN_PERF];
@@ -1509,7 +1514,7 @@ int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	/* Check if any of the perf registers are in PCC */
 	if (CPC_IN_PCC(desired_reg) || CPC_IN_PCC(max_reg) ||
 	    CPC_IN_PCC(min_reg) || CPC_IN_PCC(energy_reg) ||
-	    CPC_IN_PCC(auto_sel_enable_reg)) {
+	    CPC_IN_PCC(auto_sel_enable_reg) || CPC_IN_PCC(enable_reg)) {
 		pcc_ss_data = pcc_data[pcc_ss_id];
 		down_write(&pcc_ss_data->pcc_lock);
 		regs_in_pcc = 1;
@@ -1521,10 +1526,14 @@ int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 		}
 	}
 
-	/* desired_perf is the only mandatory value in perf_ctrls */
+	/* desired_perf is the only mandatory value in ctrls */
 	if (cpc_read(cpu, desired_reg, &desired))
 		ret = -EFAULT;
 
+	if (CPC_SUP_BUFFER_ONLY(enable_reg) &&
+	    cpc_read(cpu, enable_reg, &enable))
+		ret = -EFAULT;
+
 	if (CPC_SUP_BUFFER_ONLY(max_reg) && cpc_read(cpu, max_reg, &max))
 		ret = -EFAULT;
 
@@ -1540,11 +1549,12 @@ int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 		ret = -EFAULT;
 
 	if (!ret) {
-		perf_ctrls->desired_perf = desired;
-		perf_ctrls->max_perf = max;
-		perf_ctrls->min_perf = min;
-		perf_ctrls->energy_perf = energy;
-		perf_ctrls->auto_sel_enable = auto_sel_enable;
+		ctrls->enable = enable;
+		ctrls->desired_perf = desired;
+		ctrls->max_perf = max;
+		ctrls->min_perf = min;
+		ctrls->energy_perf = energy;
+		ctrls->auto_sel_enable = auto_sel_enable;
 	}
 
 out_err:
@@ -1552,7 +1562,7 @@ int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 		up_write(&pcc_ss_data->pcc_lock);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(cppc_get_perf);
+EXPORT_SYMBOL_GPL(cppc_get_ctrls);
 
 /**
  * cppc_get_transition_latency - returns frequency transition latency in ns
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 80720b246c51..e6cd2a487874 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -109,7 +109,8 @@ struct cppc_perf_caps {
 	u32 nominal_freq;
 };
 
-struct cppc_perf_ctrls {
+struct cppc_ctrls {
+	bool enable;
 	u32 max_perf;
 	u32 min_perf;
 	u32 desired_perf;
@@ -128,17 +129,18 @@ struct cppc_perf_fb_ctrs {
 struct cppc_cpudata {
 	int cpu;
 	struct cppc_perf_caps perf_caps;
-	struct cppc_perf_ctrls perf_ctrls;
+	struct cppc_ctrls ctrls;
 	struct cppc_perf_fb_ctrs perf_fb_ctrs;
 	struct cpufreq_policy *cur_policy;
 	unsigned int shared_type;
 	cpumask_var_t shared_cpu_map;
 };
 
+extern int cppc_get_enable(int cpu);
 extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
 extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
-extern int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls, enum cppc_regs reg_idx);
-extern int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
+extern int cppc_set_reg(int cpu, struct cppc_ctrls *ctrls, enum cppc_regs reg_idx);
+extern int cppc_get_ctrls(int cpu, struct cppc_ctrls *ctrls);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
 extern int acpi_get_psd_map(struct cppc_cpudata **);
 extern unsigned int cppc_get_transition_latency(int cpu);
-- 
2.17.1

[PATCHv3 6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)

[Natarajan, Janakarajan]

Add a new CPUFreq driver which exposes sysfs entries to control the
platform. To make use of this driver use a kernel commandline option.

Ex: amd_cpufreq=enable	- Enable AMD CPUFreq driver for Fam17h and later

Also, place amd-cpufreq before acpi-cpufreq in the Makefile to give it
higher priority.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 drivers/cpufreq/Kconfig.x86   |  14 ++
 drivers/cpufreq/Makefile      |   4 +-
 drivers/cpufreq/amd-cpufreq.c | 233 ++++++++++++++++++++++++++++++++++
 3 files changed, 250 insertions(+), 1 deletion(-)
 create mode 100644 drivers/cpufreq/amd-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
index dfa6457deaf6..01c7c5b5486a 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -32,6 +32,20 @@ config X86_PCC_CPUFREQ
 
 	  If in doubt, say N.
 
+config X86_AMD_CPUFREQ
+	tristate "AMD CPUFreq driver"
+	depends on ACPI_PROCESSOR
+	select ACPI_CPPC_LIB
+	help
+	  This adds a CPUFreq driver which uses CPPC methods
+	  as described in the ACPI v6.1 spec for newer (>= Fam17h)
+	  AMD processors.
+
+	  When this driver is enabled it will become preferred to
+	  the acpi-cpufreq driver.
+
+	  If in doubt, say N.
+
 config X86_ACPI_CPUFREQ
 	tristate "ACPI Processor P-States driver"
 	depends on ACPI_PROCESSOR
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 689b26c6f949..b2837ed9aff2 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -22,8 +22,10 @@ obj-$(CONFIG_CPUFREQ_DT_PLATDEV)	+= cpufreq-dt-platdev.o
 # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
 # K8 systems. This is still the case but acpi-cpufreq errors out so that
 # powernow-k8 can load then. ACPI is preferred to all other hardware-specific drivers.
-# speedstep-* is preferred over p4-clockmod.
+# speedstep-* is preferred over p4-clockmod. amd-cpufreq is preferred to acpi-cpufreq
+# for Fam17h or newer AMD processors. For others, acpi-cpufreq will be used.
 
+obj-$(CONFIG_X86_AMD_CPUFREQ)		+= amd-cpufreq.o
 obj-$(CONFIG_X86_ACPI_CPUFREQ)		+= acpi-cpufreq.o
 obj-$(CONFIG_X86_POWERNOW_K8)		+= powernow-k8.o
 obj-$(CONFIG_X86_PCC_CPUFREQ)		+= pcc-cpufreq.o
diff --git a/drivers/cpufreq/amd-cpufreq.c b/drivers/cpufreq/amd-cpufreq.c
new file mode 100644
index 000000000000..262c8de3be2e
--- /dev/null
+++ b/drivers/cpufreq/amd-cpufreq.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD CPUFREQ driver for Family 17h or greater AMD processors.
+ *
+ * Copyright (C) 2019 Advanced Micro Devices, Inc.
+ *
+ * Author: Janakarajan Natarajan <janakarajan.natarajan@amd.com>
+ */
+#define pr_fmt(fmt)	"AMD Cpufreq: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/cpu.h>
+#include <linux/vmalloc.h>
+#include <linux/cpufreq.h>
+#include <linux/acpi.h>
+#include <linux/delay.h>
+
+#include <asm/unaligned.h>
+
+#include <acpi/cppc_acpi.h>
+
+struct amd_desc {
+	int cpu_id;
+	struct cppc_ctrls ctrls;
+	struct kobject kobj;
+};
+
+struct amd_desc **all_cpu_data;
+
+static unsigned int cppc_enable;
+module_param(cppc_enable, uint, 0644);
+MODULE_PARM_DESC(cppc_enable,
+		 "1 - enable AMD CpuFreq, create CPPC sysfs entries.");
+
+#define to_amd_desc(a) container_of(a, struct amd_desc, kobj)
+
+#define show_func(access_fn, struct_name, member_name)			\
+	static ssize_t show_##member_name(struct kobject *kobj,		\
+					  struct kobj_attribute *attr,	\
+					  char *buf)			\
+	{								\
+		struct amd_desc *desc = to_amd_desc(kobj);		\
+		struct struct_name st_name = {0};			\
+		int ret;						\
+									\
+		ret = access_fn(desc->cpu_id, &st_name);		\
+		if (ret)						\
+			return ret;					\
+									\
+		return scnprintf(buf, PAGE_SIZE, "%llu\n",		\
+				 (u64)st_name.member_name);		\
+	}								\
+
+#define store_func(struct_name, member_name, reg_idx)			\
+	static ssize_t store_##member_name(struct kobject *kobj,	\
+					   struct kobj_attribute *attr,	\
+					   const char *buf, size_t count)\
+	{								\
+		struct amd_desc *desc = to_amd_desc(kobj);		\
+		struct struct_name st_name = {0};			\
+		u32 val;						\
+		int ret;						\
+									\
+		ret = kstrtou32(buf, 0, &val);				\
+		if (ret)						\
+			return ret;					\
+									\
+		st_name.member_name = val;				\
+									\
+		ret = cppc_set_reg(desc->cpu_id, &st_name, reg_idx);	\
+		if (ret)						\
+			return ret;					\
+									\
+		return count;						\
+	}								\
+
+#define define_one_rw(struct_name, access_fn, member_name, reg_idx)	\
+	show_func(access_fn, struct_name, member_name)			\
+	store_func(struct_name, member_name, reg_idx)			\
+	define_one_global_rw(member_name)
+
+define_one_rw(cppc_ctrls, cppc_get_ctrls, enable, ENABLE);
+define_one_rw(cppc_ctrls, cppc_get_ctrls, max_perf, MAX_PERF);
+define_one_rw(cppc_ctrls, cppc_get_ctrls, min_perf, MIN_PERF);
+define_one_rw(cppc_ctrls, cppc_get_ctrls, desired_perf, DESIRED_PERF);
+define_one_rw(cppc_ctrls, cppc_get_ctrls, auto_sel_enable, AUTO_SEL_ENABLE);
+
+static struct attribute *amd_cpufreq_attributes[] = {
+	&enable.attr,
+	&max_perf.attr,
+	&min_perf.attr,
+	&desired_perf.attr,
+	&auto_sel_enable.attr,
+	NULL
+};
+
+static const struct attribute_group amd_cpufreq_attr_group = {
+	.attrs = amd_cpufreq_attributes,
+};
+
+static struct kobj_type amd_cpufreq_type = {
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_attrs = amd_cpufreq_attributes,
+};
+
+static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+	return 0;
+}
+
+static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+	return 0;
+}
+
+static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
+{
+	return 0;
+}
+
+static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
+					unsigned int index)
+{
+	return 0;
+}
+
+static struct cpufreq_driver amd_cpufreq_driver = {
+	.name = "amd_cpufreq",
+	.init = amd_cpufreq_cpu_init,
+	.exit = amd_cpufreq_cpu_exit,
+	.verify = amd_cpufreq_cpu_verify,
+	.target_index = amd_cpufreq_cpu_target_index,
+};
+
+static void amd_cpufreq_sysfs_delete_params(void)
+{
+	int i;
+
+	for_each_possible_cpu(i) {
+		if (all_cpu_data[i]) {
+			kobject_del(&all_cpu_data[i]->kobj);
+			kfree(all_cpu_data[i]);
+		}
+	}
+
+	kfree(all_cpu_data);
+}
+
+static int __init amd_cpufreq_sysfs_expose_params(void)
+{
+	struct device *cpu_dev;
+	int i, ret;
+
+	all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
+			       GFP_KERNEL);
+
+	if (!all_cpu_data)
+		return -ENOMEM;
+
+	for_each_possible_cpu(i) {
+		all_cpu_data[i] = kzalloc(sizeof(struct amd_desc), GFP_KERNEL);
+		if (!all_cpu_data[i]) {
+			ret = -ENOMEM;
+			goto free;
+		}
+
+		all_cpu_data[i]->cpu_id = i;
+		cpu_dev = get_cpu_device(i);
+		ret = kobject_init_and_add(&all_cpu_data[i]->kobj, &amd_cpufreq_type,
+					   &cpu_dev->kobj, "amd_cpufreq");
+		if (ret)
+			goto free;
+	}
+
+	return 0;
+free:
+	amd_cpufreq_sysfs_delete_params();
+	return ret;
+}
+
+static int __init amd_cpufreq_init(void)
+{
+	int ret = 0;
+
+	/*
+	 * Use only if:
+	 * - AMD,
+	 * - Family 17h (or) newer and,
+	 * - Explicitly enabled
+	 */
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
+	    boot_cpu_data.x86 < 0x17 || !cppc_enable)
+		return -ENODEV;
+
+	ret = cpufreq_register_driver(&amd_cpufreq_driver);
+	if (ret) {
+		pr_info("Failed to register driver\n");
+		goto out;
+	}
+
+	ret = amd_cpufreq_sysfs_expose_params();
+	if (ret) {
+		pr_info("Could not create sysfs entries\n");
+		cpufreq_unregister_driver(&amd_cpufreq_driver);
+		goto out;
+	}
+
+	pr_info("Using amd-cpufreq driver\n");
+	return ret;
+
+out:
+	return ret;
+}
+
+static void __exit amd_cpufreq_exit(void)
+{
+	amd_cpufreq_sysfs_delete_params();
+	cpufreq_unregister_driver(&amd_cpufreq_driver);
+}
+
+static const struct acpi_device_id amd_acpi_ids[] __used = {
+	{ACPI_PROCESSOR_DEVICE_HID, },
+	{}
+};
+
+device_initcall(amd_cpufreq_init);
+module_exit(amd_cpufreq_exit);
+MODULE_DEVICE_TABLE(acpi, amd_acpi_ids);
+
+MODULE_AUTHOR("Janakarajan Natarajan");
+MODULE_DESCRIPTION("AMD CPUFreq driver based on ACPI CPPC v6.1 spec");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCHv3 6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)

[Viresh Kumar]

On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
> diff --git a/drivers/cpufreq/amd-cpufreq.c b/drivers/cpufreq/amd-cpufreq.c
> +#define pr_fmt(fmt)	"AMD Cpufreq: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/vmalloc.h>
> +#include <linux/cpufreq.h>
> +#include <linux/acpi.h>
> +#include <linux/delay.h>

Please keep them in alphabetical order.

> +
> +#include <asm/unaligned.h>
> +
> +#include <acpi/cppc_acpi.h>
> +
> +struct amd_desc {
> +	int cpu_id;
> +	struct cppc_ctrls ctrls;
> +	struct kobject kobj;
> +};
> +
> +struct amd_desc **all_cpu_data;
> +
> +static unsigned int cppc_enable;
> +module_param(cppc_enable, uint, 0644);
> +MODULE_PARM_DESC(cppc_enable,
> +		 "1 - enable AMD CpuFreq, create CPPC sysfs entries.");
> +
> +#define to_amd_desc(a) container_of(a, struct amd_desc, kobj)
> +
> +#define show_func(access_fn, struct_name, member_name)			\
> +	static ssize_t show_##member_name(struct kobject *kobj,		\
> +					  struct kobj_attribute *attr,	\
> +					  char *buf)			\
> +	{								\
> +		struct amd_desc *desc = to_amd_desc(kobj);		\
> +		struct struct_name st_name = {0};			\
> +		int ret;						\
> +									\
> +		ret = access_fn(desc->cpu_id, &st_name);		\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		return scnprintf(buf, PAGE_SIZE, "%llu\n",		\
> +				 (u64)st_name.member_name);		\
> +	}								\
> +
> +#define store_func(struct_name, member_name, reg_idx)			\
> +	static ssize_t store_##member_name(struct kobject *kobj,	\
> +					   struct kobj_attribute *attr,	\
> +					   const char *buf, size_t count)\
> +	{								\
> +		struct amd_desc *desc = to_amd_desc(kobj);		\
> +		struct struct_name st_name = {0};			\
> +		u32 val;						\
> +		int ret;						\
> +									\
> +		ret = kstrtou32(buf, 0, &val);				\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		st_name.member_name = val;				\
> +									\
> +		ret = cppc_set_reg(desc->cpu_id, &st_name, reg_idx);	\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		return count;						\
> +	}								\
> +
> +#define define_one_rw(struct_name, access_fn, member_name, reg_idx)	\
> +	show_func(access_fn, struct_name, member_name)			\
> +	store_func(struct_name, member_name, reg_idx)			\
> +	define_one_global_rw(member_name)
> +
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, enable, ENABLE);
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, max_perf, MAX_PERF);
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, min_perf, MIN_PERF);
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, desired_perf, DESIRED_PERF);
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, auto_sel_enable, AUTO_SEL_ENABLE);
> +
> +static struct attribute *amd_cpufreq_attributes[] = {
> +	&enable.attr,
> +	&max_perf.attr,
> +	&min_perf.attr,
> +	&desired_perf.attr,
> +	&auto_sel_enable.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group amd_cpufreq_attr_group = {
> +	.attrs = amd_cpufreq_attributes,
> +};
> +
> +static struct kobj_type amd_cpufreq_type = {
> +	.sysfs_ops = &kobj_sysfs_ops,
> +	.default_attrs = amd_cpufreq_attributes,
> +};
> +
> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> +	return 0;
> +}
> +
> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> +{
> +	return 0;
> +}
> +
> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
> +{
> +	return 0;
> +}
> +
> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
> +					unsigned int index)
> +{
> +	return 0;
> +}

All empty helpers ? There is nothing you need to do ?

> +
> +static struct cpufreq_driver amd_cpufreq_driver = {
> +	.name = "amd_cpufreq",
> +	.init = amd_cpufreq_cpu_init,
> +	.exit = amd_cpufreq_cpu_exit,
> +	.verify = amd_cpufreq_cpu_verify,
> +	.target_index = amd_cpufreq_cpu_target_index,
> +};
> +
> +static void amd_cpufreq_sysfs_delete_params(void)
> +{
> +	int i;
> +
> +	for_each_possible_cpu(i) {
> +		if (all_cpu_data[i]) {
> +			kobject_del(&all_cpu_data[i]->kobj);

Shouldn't you use kobject_put() instead of this ?

> +			kfree(all_cpu_data[i]);
> +		}
> +	}
> +
> +	kfree(all_cpu_data);
> +}
> +
> +static int __init amd_cpufreq_sysfs_expose_params(void)
> +{
> +	struct device *cpu_dev;
> +	int i, ret;
> +
> +	all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
> +			       GFP_KERNEL);
> +
> +	if (!all_cpu_data)
> +		return -ENOMEM;
> +
> +	for_each_possible_cpu(i) {
> +		all_cpu_data[i] = kzalloc(sizeof(struct amd_desc), GFP_KERNEL);
> +		if (!all_cpu_data[i]) {
> +			ret = -ENOMEM;
> +			goto free;
> +		}
> +
> +		all_cpu_data[i]->cpu_id = i;
> +		cpu_dev = get_cpu_device(i);
> +		ret = kobject_init_and_add(&all_cpu_data[i]->kobj, &amd_cpufreq_type,
> +					   &cpu_dev->kobj, "amd_cpufreq");
> +		if (ret)
> +			goto free;
> +	}
> +
> +	return 0;
> +free:
> +	amd_cpufreq_sysfs_delete_params();
> +	return ret;
> +}

Instead of doing this once for all CPUs, it would be better to do it
every time the ->init() callback of the driver gets called. If you
have one cpufreq policy for each CPU (i.e. no CPUs share clock lines),
then the init() callback will get called once for each CPU.

> +
> +static int __init amd_cpufreq_init(void)
> +{
> +	int ret = 0;
> +
> +	/*
> +	 * Use only if:
> +	 * - AMD,
> +	 * - Family 17h (or) newer and,
> +	 * - Explicitly enabled
> +	 */
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
> +	    boot_cpu_data.x86 < 0x17 || !cppc_enable)
> +		return -ENODEV;
> +
> +	ret = cpufreq_register_driver(&amd_cpufreq_driver);
> +	if (ret) {
> +		pr_info("Failed to register driver\n");
> +		goto out;
> +	}
> +
> +	ret = amd_cpufreq_sysfs_expose_params();
> +	if (ret) {
> +		pr_info("Could not create sysfs entries\n");
> +		cpufreq_unregister_driver(&amd_cpufreq_driver);
> +		goto out;
> +	}
> +
> +	pr_info("Using amd-cpufreq driver\n");
> +	return ret;
> +
> +out:
> +	return ret;
> +}
> +
> +static void __exit amd_cpufreq_exit(void)
> +{
> +	amd_cpufreq_sysfs_delete_params();
> +	cpufreq_unregister_driver(&amd_cpufreq_driver);
> +}
> +
> +static const struct acpi_device_id amd_acpi_ids[] __used = {
> +	{ACPI_PROCESSOR_DEVICE_HID, },
> +	{}
> +};
> +
> +device_initcall(amd_cpufreq_init);
> +module_exit(amd_cpufreq_exit);
> +MODULE_DEVICE_TABLE(acpi, amd_acpi_ids);

All three should be placed directly below the struct/function they
represent without any blank lines in between. As suggested in
kernel documentation.

> +
> +MODULE_AUTHOR("Janakarajan Natarajan");
> +MODULE_DESCRIPTION("AMD CPUFreq driver based on ACPI CPPC v6.1 spec");
> +MODULE_LICENSE("GPL");

Should this be "GPL v2" ?

> -- 
> 2.17.1

-- 
viresh

Re: [PATCHv3 6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)

[Janakarajan Natarajan]

On 7/11/2019 1:12 AM, Viresh Kumar wrote:
> On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
>> diff --git a/drivers/cpufreq/amd-cpufreq.c b/drivers/cpufreq/amd-cpufreq.c
>> +#define pr_fmt(fmt)	"AMD Cpufreq: " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/cpu.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/acpi.h>
>> +#include <linux/delay.h>
> Please keep them in alphabetical order.


Ok.


>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <acpi/cppc_acpi.h>
>> +
>> +struct amd_desc {
>> +	int cpu_id;
>> +	struct cppc_ctrls ctrls;
>> +	struct kobject kobj;
>> +};
>> +
>> +struct amd_desc **all_cpu_data;
>> +
>> +static unsigned int cppc_enable;
>> +module_param(cppc_enable, uint, 0644);
>> +MODULE_PARM_DESC(cppc_enable,
>> +		 "1 - enable AMD CpuFreq, create CPPC sysfs entries.");
>> +
>> +#define to_amd_desc(a) container_of(a, struct amd_desc, kobj)
>> +
>> +#define show_func(access_fn, struct_name, member_name)			\
>> +	static ssize_t show_##member_name(struct kobject *kobj,		\
>> +					  struct kobj_attribute *attr,	\
>> +					  char *buf)			\
>> +	{								\
>> +		struct amd_desc *desc = to_amd_desc(kobj);		\
>> +		struct struct_name st_name = {0};			\
>> +		int ret;						\
>> +									\
>> +		ret = access_fn(desc->cpu_id, &st_name);		\
>> +		if (ret)						\
>> +			return ret;					\
>> +									\
>> +		return scnprintf(buf, PAGE_SIZE, "%llu\n",		\
>> +				 (u64)st_name.member_name);		\
>> +	}								\
>> +
>> +#define store_func(struct_name, member_name, reg_idx)			\
>> +	static ssize_t store_##member_name(struct kobject *kobj,	\
>> +					   struct kobj_attribute *attr,	\
>> +					   const char *buf, size_t count)\
>> +	{								\
>> +		struct amd_desc *desc = to_amd_desc(kobj);		\
>> +		struct struct_name st_name = {0};			\
>> +		u32 val;						\
>> +		int ret;						\
>> +									\
>> +		ret = kstrtou32(buf, 0, &val);				\
>> +		if (ret)						\
>> +			return ret;					\
>> +									\
>> +		st_name.member_name = val;				\
>> +									\
>> +		ret = cppc_set_reg(desc->cpu_id, &st_name, reg_idx);	\
>> +		if (ret)						\
>> +			return ret;					\
>> +									\
>> +		return count;						\
>> +	}								\
>> +
>> +#define define_one_rw(struct_name, access_fn, member_name, reg_idx)	\
>> +	show_func(access_fn, struct_name, member_name)			\
>> +	store_func(struct_name, member_name, reg_idx)			\
>> +	define_one_global_rw(member_name)
>> +
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, enable, ENABLE);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, max_perf, MAX_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, min_perf, MIN_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, desired_perf, DESIRED_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, auto_sel_enable, AUTO_SEL_ENABLE);
>> +
>> +static struct attribute *amd_cpufreq_attributes[] = {
>> +	&enable.attr,
>> +	&max_perf.attr,
>> +	&min_perf.attr,
>> +	&desired_perf.attr,
>> +	&auto_sel_enable.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group amd_cpufreq_attr_group = {
>> +	.attrs = amd_cpufreq_attributes,
>> +};
>> +
>> +static struct kobj_type amd_cpufreq_type = {
>> +	.sysfs_ops = &kobj_sysfs_ops,
>> +	.default_attrs = amd_cpufreq_attributes,
>> +};
>> +
>> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
>> +					unsigned int index)
>> +{
>> +	return 0;
>> +}
> All empty helpers ? There is nothing you need to do ?


When we posted v2 of this patchset, Rafael let us know that he was 
uncomfortable

going behind the (acpi-cpufreq) drivers back by letting the user 
communicate directly

with the platform. That's the reason we have an empty driver whose 
primary purpose

is to expose sysfs entries for the user.


>
>> +
>> +static struct cpufreq_driver amd_cpufreq_driver = {
>> +	.name = "amd_cpufreq",
>> +	.init = amd_cpufreq_cpu_init,
>> +	.exit = amd_cpufreq_cpu_exit,
>> +	.verify = amd_cpufreq_cpu_verify,
>> +	.target_index = amd_cpufreq_cpu_target_index,
>> +};
>> +
>> +static void amd_cpufreq_sysfs_delete_params(void)
>> +{
>> +	int i;
>> +
>> +	for_each_possible_cpu(i) {
>> +		if (all_cpu_data[i]) {
>> +			kobject_del(&all_cpu_data[i]->kobj);
> Shouldn't you use kobject_put() instead of this ?


Ok.


>
>> +			kfree(all_cpu_data[i]);
>> +		}
>> +	}
>> +
>> +	kfree(all_cpu_data);
>> +}
>> +
>> +static int __init amd_cpufreq_sysfs_expose_params(void)
>> +{
>> +	struct device *cpu_dev;
>> +	int i, ret;
>> +
>> +	all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
>> +			       GFP_KERNEL);
>> +
>> +	if (!all_cpu_data)
>> +		return -ENOMEM;
>> +
>> +	for_each_possible_cpu(i) {
>> +		all_cpu_data[i] = kzalloc(sizeof(struct amd_desc), GFP_KERNEL);
>> +		if (!all_cpu_data[i]) {
>> +			ret = -ENOMEM;
>> +			goto free;
>> +		}
>> +
>> +		all_cpu_data[i]->cpu_id = i;
>> +		cpu_dev = get_cpu_device(i);
>> +		ret = kobject_init_and_add(&all_cpu_data[i]->kobj, &amd_cpufreq_type,
>> +					   &cpu_dev->kobj, "amd_cpufreq");
>> +		if (ret)
>> +			goto free;
>> +	}
>> +
>> +	return 0;
>> +free:
>> +	amd_cpufreq_sysfs_delete_params();
>> +	return ret;
>> +}
> Instead of doing this once for all CPUs, it would be better to do it
> every time the ->init() callback of the driver gets called. If you
> have one cpufreq policy for each CPU (i.e. no CPUs share clock lines),
> then the init() callback will get called once for each CPU.


We are trying to avoid any use of the cpufreq mechanism.


Thanks,

Janakarajan


>
>> +
>> +static int __init amd_cpufreq_init(void)
>> +{
>> +	int ret = 0;
>> +
>> +	/*
>> +	 * Use only if:
>> +	 * - AMD,
>> +	 * - Family 17h (or) newer and,
>> +	 * - Explicitly enabled
>> +	 */
>> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
>> +	    boot_cpu_data.x86 < 0x17 || !cppc_enable)
>> +		return -ENODEV;
>> +
>> +	ret = cpufreq_register_driver(&amd_cpufreq_driver);
>> +	if (ret) {
>> +		pr_info("Failed to register driver\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = amd_cpufreq_sysfs_expose_params();
>> +	if (ret) {
>> +		pr_info("Could not create sysfs entries\n");
>> +		cpufreq_unregister_driver(&amd_cpufreq_driver);
>> +		goto out;
>> +	}
>> +
>> +	pr_info("Using amd-cpufreq driver\n");
>> +	return ret;
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>> +static void __exit amd_cpufreq_exit(void)
>> +{
>> +	amd_cpufreq_sysfs_delete_params();
>> +	cpufreq_unregister_driver(&amd_cpufreq_driver);
>> +}
>> +
>> +static const struct acpi_device_id amd_acpi_ids[] __used = {
>> +	{ACPI_PROCESSOR_DEVICE_HID, },
>> +	{}
>> +};
>> +
>> +device_initcall(amd_cpufreq_init);
>> +module_exit(amd_cpufreq_exit);
>> +MODULE_DEVICE_TABLE(acpi, amd_acpi_ids);
> All three should be placed directly below the struct/function they
> represent without any blank lines in between. As suggested in
> kernel documentation.
>
>> +
>> +MODULE_AUTHOR("Janakarajan Natarajan");
>> +MODULE_DESCRIPTION("AMD CPUFreq driver based on ACPI CPPC v6.1 spec");
>> +MODULE_LICENSE("GPL");
> Should this be "GPL v2" ?
>
>> -- 
>> 2.17.1

Re: [PATCHv3 6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)

[Viresh Kumar]

On 11-07-19, 16:58, Janakarajan Natarajan wrote:
> On 7/11/2019 1:12 AM, Viresh Kumar wrote:
> > On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
> >> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
> >> +					unsigned int index)
> >> +{
> >> +	return 0;
> >> +}
> > All empty helpers ? There is nothing you need to do ?
> 
> 
> When we posted v2 of this patchset, Rafael let us know that he was 
> uncomfortable
> 
> going behind the (acpi-cpufreq) drivers back by letting the user 
> communicate directly
> 
> with the platform. That's the reason we have an empty driver whose 
> primary purpose
> 
> is to expose sysfs entries for the user.

I read his comments now and what he suggested is:

"What about handling this like the others do, through a proper cpufreq
driver?"

I am not sure if he meant something like that you have here. Only one
cpufreq driver can be registered at any point of time with the kernel,
and so if this one is there then acpi-cpufreq or intel-pstate can't be
there. Who will do DVFS ?

-- 
viresh

Re: [PATCHv3 6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)

[Janakarajan Natarajan]

On 7/11/2019 10:10 PM, Viresh Kumar wrote:
> On 11-07-19, 16:58, Janakarajan Natarajan wrote:
>> On 7/11/2019 1:12 AM, Viresh Kumar wrote:
>>> On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
>>>> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
>>>> +					unsigned int index)
>>>> +{
>>>> +	return 0;
>>>> +}
>>> All empty helpers ? There is nothing you need to do ?
>>
>> When we posted v2 of this patchset, Rafael let us know that he was
>> uncomfortable
>>
>> going behind the (acpi-cpufreq) drivers back by letting the user
>> communicate directly
>>
>> with the platform. That's the reason we have an empty driver whose
>> primary purpose
>>
>> is to expose sysfs entries for the user.
> I read his comments now and what he suggested is:
>
> "What about handling this like the others do, through a proper cpufreq
> driver?"
>
> I am not sure if he meant something like that you have here. Only one
> cpufreq driver can be registered at any point of time with the kernel,
> and so if this one is there then acpi-cpufreq or intel-pstate can't be
> there. Who will do DVFS ?


By default, the driver is disabled and cpufreq falls back to using 
acpi-cpufreq. To enable the driver,

a parameter i.e. amd_cpufreq.cppc_enable=1 needs to be used. Then the 
user will gain access to

/sys/devices/system/cpu/cpu<num>/amd_cpufreq/{enable, max_perf, 
min_perf, desired_perf, auto_sel_enable}.


max_perf, min_perf allows the user to indicate to the platform the 
performance they expect

in an abstract scale [min_perf to max_perf] inclusive. desired_perf is 
the performance the user

would like to ideally achieve. desired_perf will fall withing max and 
min perf. Based on the value of these

registers, the platform will adjust a number of variables, voltage and 
frequency being one of them.


If the user would rather have the system handle all power/performance 
adjustments by

itself, then they can set the auto_sel_enable register.


Thanks,

Janakarajan

Re: [PATCHv3 0/6] CPPC optional registers AMD support

[Peter Zijlstra]

On Wed, Jul 10, 2019 at 06:37:09PM +0000, Natarajan, Janakarajan wrote:
> CPPC (Collaborative Processor Performance Control) offers optional
> registers which can be used to tune the system based on energy and/or
> performance requirements.
> 
> Newer AMD processors (>= Family 17h) add support for a subset of these
> optional CPPC registers, based on ACPI v6.1.
> 
> The following are the supported CPPC registers for which sysfs entries
> are created:
> * enable                (NEW)
> * max_perf              (NEW)
> * min_perf              (NEW)
> * energy_perf
> * lowest_perf
> * nominal_perf
> * desired_perf          (NEW)
> * feedback_ctrs
> * auto_sel_enable       (NEW)
> * lowest_nonlinear_perf
> 
> First, update cppc_acpi to create sysfs entries only when the optional
> registers are known to be supported.
> 
> Next, a new CPUFreq driver is introduced to enable the OSPM and the userspace
> to access the newly supported registers through sysfs entries found in
> /sys/devices/system/cpu/cpu<num>/amd_cpufreq/.
> 
> This new CPUFreq driver can only be used by providing a module parameter,
> amd_cpufreq.cppc_enable=1.
> 
> The purpose of exposing the registers via the amd-cpufreq sysfs entries is to
> allow the userspace to:
> * Tweak the values to fit its workload.
> * Apply a profile from AMD's optimization guides.

So in general I think it is a huge mistake to expose all that to
userspace. Before you know it, there's tools that actually rely on it,
and then inhibit the kernel from doing anything sane with it.

> Profiles will be documented in the performance/optimization guides.

I don't think userspace can really do anything sane with this; it lacks
much if not all useful information.

> Note:
> * AMD systems will not have a policy applied in the kernel at this time.

And why the heck not? We're trying to move all cpufreq into the
scheduler and have only a single governor, namely schedutil -- yes,
we're still stuck with legacy, and we're still working on performance
parity in some cases, but I really hope to get rid of all other cpufreq
governors eventually.

And if you look at schedutil (schedutil_cpu_util in specific) then
you'll see it is already prepared for CPPC and currently only held back
by the generic cpufreq interface.

It currently only sets desired freq, it has information for
min/guaranteed, and once we get thermal intergrated we might have
sensible data for max freq too.

> TODO:
> * Create a linux userspace tool that will help users generate a CPPC profile
>   for their target workload.

Basically a big fat NAK for this approach to cpufreq.

> * Create a general CPPC policy in the kernel.

We already have that, sorta.

RE: [PATCHv3 0/6] CPPC optional registers AMD support

[Ghannam, Yazen]

> -----Original Message-----
> From: Peter Zijlstra <peterz@infradead.org>
> Sent: Saturday, July 13, 2019 5:46 AM
> To: Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>
> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-pm@vger.kernel.org; devel@acpica.org; Rafael J . Wysocki
> <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Viresh Kumar <viresh.kumar@linaro.org>; Robert Moore
> <robert.moore@intel.com>; Erik Schmauss <erik.schmauss@intel.com>; Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Subject: Re: [PATCHv3 0/6] CPPC optional registers AMD support
> 
> On Wed, Jul 10, 2019 at 06:37:09PM +0000, Natarajan, Janakarajan wrote:
> > CPPC (Collaborative Processor Performance Control) offers optional
> > registers which can be used to tune the system based on energy and/or
> > performance requirements.
> >
> > Newer AMD processors (>= Family 17h) add support for a subset of these
> > optional CPPC registers, based on ACPI v6.1.
> >
> > The following are the supported CPPC registers for which sysfs entries
> > are created:
> > * enable                (NEW)
> > * max_perf              (NEW)
> > * min_perf              (NEW)
> > * energy_perf
> > * lowest_perf
> > * nominal_perf
> > * desired_perf          (NEW)
> > * feedback_ctrs
> > * auto_sel_enable       (NEW)
> > * lowest_nonlinear_perf
> >
> > First, update cppc_acpi to create sysfs entries only when the optional
> > registers are known to be supported.
> >
> > Next, a new CPUFreq driver is introduced to enable the OSPM and the userspace
> > to access the newly supported registers through sysfs entries found in
> > /sys/devices/system/cpu/cpu<num>/amd_cpufreq/.
> >
> > This new CPUFreq driver can only be used by providing a module parameter,
> > amd_cpufreq.cppc_enable=1.
> >
> > The purpose of exposing the registers via the amd-cpufreq sysfs entries is to
> > allow the userspace to:
> > * Tweak the values to fit its workload.
> > * Apply a profile from AMD's optimization guides.
> 
> So in general I think it is a huge mistake to expose all that to
> userspace. Before you know it, there's tools that actually rely on it,
> and then inhibit the kernel from doing anything sane with it.
> 

Okay, makes sense.

Is there any way to expose a sysfs interface and make it explicitly "experimental"? Maybe putting it in Documentation/ABI/testing/?

Or do you think it's just not worth it?

> > Profiles will be documented in the performance/optimization guides.
> 
> I don't think userspace can really do anything sane with this; it lacks
> much if not all useful information.
> 
> > Note:
> > * AMD systems will not have a policy applied in the kernel at this time.
> 
> And why the heck not? We're trying to move all cpufreq into the
> scheduler and have only a single governor, namely schedutil -- yes,
> we're still stuck with legacy, and we're still working on performance
> parity in some cases, but I really hope to get rid of all other cpufreq
> governors eventually.
> 

Because this is new to AMD systems, we didn't want to enforce a default policy.

We figured that exposing the CPPC interface would be a good way to decouple policy from the kernel and let users experiment/tune their systems, like using the userspace governor. And if some pattern emerged then we could make that a default policy in the kernel (for AMD or in general).

But you're saying we should focus more on working with the schedutil governor, correct? Do you think there's still a use for a userspace governor?

> And if you look at schedutil (schedutil_cpu_util in specific) then
> you'll see it is already prepared for CPPC and currently only held back
> by the generic cpufreq interface.
> 
> It currently only sets desired freq, it has information for
> min/guaranteed, and once we get thermal intergrated we might have
> sensible data for max freq too.
> 

Will do.

> > TODO:
> > * Create a linux userspace tool that will help users generate a CPPC profile
> >   for their target workload.
> 
> Basically a big fat NAK for this approach to cpufreq.
> 

Is that for exposing the sysfs interface, having a stub driver, or both?

Would it be better to have a cpufreq driver that implements some policy rather than just providing the sysfs interface?

> > * Create a general CPPC policy in the kernel.
> 
> We already have that, sorta.

Right, but it seems to still be focused on CPU frequency rather than abstract performance like how CPPC is defined.

This is another reason for exposing the CPPC interface directly. We'll give users the ability to interact with the platform, using CPPC, without having to follow the CPUFREQ paradigm.

Do you think this is doable? Or should we always have some kernel interaction because of the scheduler, etc.?

Thanks,
Yazen