[PATCH v2 0/7] CPPC optional registers AMD support

[PATCH v2 0/7] 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 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

The CPPC driver is updated to enable the OSPM and the userspace to
access
the newly supported registers.

The purpose of exposing the registers via the 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.
* By default, acpi_cpufreq will still be used.

TODO:
* Create a linux userspace tool that will help users generate a CPPC
* profile
  for their target workload.
* Create or update a driver to apply 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.

Janakarajan Natarajan (2):
  acpi/cppc: Add macro for CPPC register BUFFER only check
  acpi/cppc: Ensure only supported CPPC sysfs entries are created

Yazen Ghannam (5):
  acpi/cppc: Modify show_cppc_data macro
  acpi/cppc: Rework cppc_set_perf() to use cppc_regs index
  acpi/cppc: Add macros to define a R/W sysfs entry for CPPC registers
  acpi/cppc: Add support for optional CPPC registers
  acpi/cppc: Add support for CPPC Enable register

 drivers/acpi/cppc_acpi.c       | 402 +++++++++++++++++++++++++++++----
 drivers/cpufreq/cppc_cpufreq.c |   6 +-
 include/acpi/cppc_acpi.h       |   6 +-
 3 files changed, 372 insertions(+), 42 deletions(-)

-- 
2.17.1

[PATCH v2 1/7] acpi/cppc: Add macro for CPPC register BUFFER only check

[Natarajan, Janakarajan]

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

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index d4244e7d0e38..56a09e057c4f 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -115,6 +115,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
-- 
2.17.1

[PATCH v2 2/7] acpi/cppc: Ensure only supported CPPC sysfs entries are created

[Natarajan, Janakarajan]

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

Also, add a macro REG_SUPPORTED which will decide on the check to perform
based on the type of register.

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

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 56a09e057c4f..37157d19d2df 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -187,22 +187,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)
@@ -717,6 +703,87 @@ 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]))
+
+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 = entries - MANDATORY_REG_CNT;
+	static struct attribute **cppc_attrs;
+
+	cppc_attrs = kcalloc(entries, sizeof(*cppc_attrs), GFP_KERNEL);
+	if (!cppc_attrs)
+		return -ENOMEM;
+
+	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 CPUs logical Id.
@@ -871,6 +938,10 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	/* Plug PSD data into this CPUs 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) {
@@ -932,6 +1003,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

[PATCH v2 3/7] acpi/cppc: Modify show_cppc_data macro

[Natarajan, Janakarajan]

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

The show_cppc_data macro implicity uses define_one_cppc_ro. This will
prevent the creation of an attribute with read and write permissions.

Create a separate macro that defines a show attribute and creates
a read-only sysfs entry. This is in preparation for adding a macro
to create sysfs entries with read+write permission.

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 | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 37157d19d2df..10ae5a5818e6 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -159,17 +159,20 @@ __ATTR(_name, 0444, show_##_name, NULL)
 		return scnprintf(buf, PAGE_SIZE, "%llu\n",		\
 				(u64)st_name.member_name);		\
 	}								\
+
+#define show_cppc_data_ro(access_fn, struct_name, member_name)		\
+	show_cppc_data(access_fn, struct_name, member_name)		\
 	define_one_cppc_ro(member_name)
 
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, highest_perf);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_perf);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_perf);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_nonlinear_perf);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_freq);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq);
+show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, highest_perf);
+show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, lowest_perf);
+show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, nominal_perf);
+show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, lowest_nonlinear_perf);
+show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, lowest_freq);
+show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, nominal_freq);
 
-show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf);
-show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
+show_cppc_data_ro(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf);
+show_cppc_data_ro(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
 
 static ssize_t show_feedback_ctrs(struct kobject *kobj,
 		struct attribute *attr, char *buf)
-- 
2.17.1

[PATCH v2 4/7] 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 10ae5a5818e6..1cce231b8501 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -60,7 +60,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
@@ -1346,26 +1346,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 CPUs 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
@@ -1374,7 +1386,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;
@@ -1401,14 +1413,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.
 	 *
@@ -1451,7 +1463,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)
@@ -1467,7 +1479,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 2ae978d27e61..420bd44f6958 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -211,7 +211,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)
@@ -235,7 +235,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);
@@ -348,7 +348,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 ba6fd7202775..ba3b3fb64572 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -139,7 +139,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

[PATCH v2 5/7] acpi/cppc: Add macros to define a R/W sysfs entry for CPPC registers

[Natarajan, Janakarajan]

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

Some CPPC registers can be used to configure the platform. To enable this,
create macros to define the show, store routines and create sysfs entries
with R/W permission.

Signed-off-by: Yazen Ghannam <Yazen.Ghannam@amd.com>
[ carved into a patch, cleaned up, productized ]
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 drivers/acpi/cppc_acpi.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 1cce231b8501..1e862415faf0 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -142,6 +142,10 @@ struct cppc_attr {
 static struct cppc_attr _name =			\
 __ATTR(_name, 0444, show_##_name, NULL)
 
+#define define_one_cppc_rw(_name)		\
+static struct cppc_attr _name =			\
+__ATTR(_name, 0644, show_##_name, store_##_name)
+
 #define to_cpc_desc(a) container_of(a, struct cpc_desc, kobj)
 
 #define show_cppc_data(access_fn, struct_name, member_name)		\
@@ -164,6 +168,33 @@ __ATTR(_name, 0444, show_##_name, NULL)
 	show_cppc_data(access_fn, struct_name, member_name)		\
 	define_one_cppc_ro(member_name)
 
+#define store_cppc_data(struct_name, member_name, reg_idx)		\
+	static ssize_t store_##member_name(struct kobject *kobj,	\
+					   struct attribute *attr,	\
+					   const char *c, ssize_t count)\
+	{								\
+		struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);		\
+		struct struct_name st_name = {0};			\
+		u32 val;						\
+		int ret;						\
+									\
+		ret = kstrtou32(c, 0, &val);				\
+		if (ret)						\
+			return ret;					\
+									\
+		st_name.member_name = val;				\
+									\
+		ret = cppc_set_reg(cpc_ptr->cpu_id, &st_name, reg_idx);	\
+		if (ret)						\
+			return ret;					\
+									\
+		return count;						\
+	}								\
+
+#define store_cppc_data_rw(struct_name, member_name, reg_idx)		\
+	store_cppc_data(struct_name, member_name, reg_idx)		\
+	define_one_cppc_rw(member_name)
+
 show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, highest_perf);
 show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, lowest_perf);
 show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, nominal_perf);
-- 
2.17.1

[PATCH v2 6/7] 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.
Create show, store and helper routines for supported CPPC 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 | 120 ++++++++++++++++++++++++++++++++++++---
 include/acpi/cppc_acpi.h |   3 +
 2 files changed, 115 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 1e862415faf0..bb57d526e54e 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -204,6 +204,17 @@ show_cppc_data_ro(cppc_get_perf_caps, cppc_perf_caps, nominal_freq);
 
 show_cppc_data_ro(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf);
 show_cppc_data_ro(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
+show_cppc_data(cppc_get_perf, cppc_perf_ctrls, desired_perf);
+show_cppc_data(cppc_get_perf, cppc_perf_ctrls, max_perf);
+show_cppc_data(cppc_get_perf, cppc_perf_ctrls, min_perf);
+show_cppc_data(cppc_get_perf, cppc_perf_ctrls, energy_perf);
+show_cppc_data(cppc_get_perf, cppc_perf_ctrls, auto_sel_enable);
+
+store_cppc_data_rw(cppc_perf_ctrls, desired_perf, DESIRED_PERF);
+store_cppc_data_rw(cppc_perf_ctrls, max_perf, MAX_PERF);
+store_cppc_data_rw(cppc_perf_ctrls, min_perf, MIN_PERF);
+store_cppc_data_rw(cppc_perf_ctrls, energy_perf, ENERGY_PERF);
+store_cppc_data_rw(cppc_perf_ctrls, auto_sel_enable, AUTO_SEL_ENABLE);
 
 static ssize_t show_feedback_ctrs(struct kobject *kobj,
 		struct attribute *attr, char *buf)
@@ -800,6 +811,21 @@ static int set_cppc_attrs(struct cpc_desc *cpc, int entries)
 		case CTR_WRAP_TIME:
 			cppc_attrs[attr_i++] = &wraparound_time.attr;
 			break;
+		case MAX_PERF:
+			cppc_attrs[attr_i++] = &max_perf.attr;
+			break;
+		case MIN_PERF:
+			cppc_attrs[attr_i++] = &min_perf.attr;
+			break;
+		case ENERGY_PERF:
+			cppc_attrs[attr_i++] = &energy_perf.attr;
+			break;
+		case AUTO_SEL_ENABLE:
+			cppc_attrs[attr_i++] = &auto_sel_enable.attr;
+			break;
+		case DESIRED_PERF:
+			cppc_attrs[attr_i++] = &desired_perf.attr;
+			break;
 		}
 	}
 
@@ -1391,7 +1417,7 @@ int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
 	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;
+	int ret = 0, regs_in_pcc = 0;
 	u32 value;
 
 	if (!cpc_desc) {
@@ -1403,6 +1429,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;
@@ -1418,6 +1456,7 @@ int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
 	 * achieve that goal here
 	 */
 	if (CPC_IN_PCC(reg)) {
+		regs_in_pcc = 1;
 		if (pcc_ss_id < 0) {
 			pr_debug("Invalid pcc_ss_id\n");
 			return -ENODEV;
@@ -1440,13 +1479,10 @@ 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))
+	if (regs_in_pcc)
 		up_read(&pcc_ss_data->pcc_lock);	/* END Phase-I */
 	/*
 	 * This is Phase-II where we transfer the ownership of PCC to Platform
@@ -1494,7 +1530,7 @@ int cppc_set_reg(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(reg)) {
+	if (regs_in_pcc) {
 		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)
@@ -1512,6 +1548,74 @@ 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];
+
+	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 ba3b3fb64572..6f651235933c 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -117,6 +117,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 {
@@ -140,6 +142,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

[PATCH v2 7/7] 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.

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 | 96 ++++++++++++++++++++++++++++++++++++++++
 include/acpi/cppc_acpi.h |  1 +
 2 files changed, 97 insertions(+)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index bb57d526e54e..b74c489a3e8d 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -232,6 +232,43 @@ static ssize_t show_feedback_ctrs(struct kobject *kobj,
 }
 define_one_cppc_ro(feedback_ctrs);
 
+/* Used to move ENABLE register value between userspace and platform */
+static bool cppc_cpu_enable;
+
+static ssize_t show_enable(struct kobject *kobj,
+			   struct attribute *attr,
+			   char *buf)
+{
+	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
+	int ret;
+
+	ret = cppc_get_enable(cpc_ptr->cpu_id);
+	if (ret)
+		return ret;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", cppc_cpu_enable);
+}
+
+static ssize_t store_enable(struct kobject *kobj,
+			    struct attribute *attr,
+			    const char *c, ssize_t count)
+{
+	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
+	int ret;
+
+	ret = kstrtobool(c, &cppc_cpu_enable);
+	if (ret)
+		return ret;
+
+	ret = cppc_set_reg(cpc_ptr->cpu_id, NULL, ENABLE);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+define_one_cppc_rw(enable);
+
 static struct kobj_type cppc_ktype = {
 	.sysfs_ops = &kobj_sysfs_ops,
 };
@@ -826,6 +863,9 @@ static int set_cppc_attrs(struct cpc_desc *cpc, int entries)
 		case DESIRED_PERF:
 			cppc_attrs[attr_i++] = &desired_perf.attr;
 			break;
+		case ENABLE:
+			cppc_attrs[attr_i++] = &enable.attr;
+			break;
 		}
 	}
 
@@ -1426,6 +1466,9 @@ int cppc_set_reg(int cpu, struct cppc_perf_ctrls *perf_ctrls,
 	}
 
 	switch (reg_idx) {
+	case ENABLE:
+		value = cppc_cpu_enable;
+		break;
 	case DESIRED_PERF:
 		value = perf_ctrls->desired_perf;
 		break;
@@ -1616,6 +1659,59 @@ int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf);
 
+/**
+ * cppc_get_enable - Read a CPUs enable register.
+ * @cpu: CPU from which to read control values.
+ *
+ * Return: 0 for success.
+ */
+int cppc_get_enable(int cpu)
+{
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cpc_register_resource *enable_reg;
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret = 0, regs_in_pcc = 0;
+	u64 enable;
+
+	if (!cpc_desc) {
+		pr_debug("No CPC descriptor for CPU: %d\n", cpu);
+		return -ENODEV;
+	}
+
+	enable_reg = &cpc_desc->cpc_regs[ENABLE];
+
+	if (!CPC_SUP_BUFFER_ONLY(enable_reg)) {
+		pr_warn("CPC ENABLE register not supported.\n");
+		return -ENOTSUPP;
+	}
+
+	if (CPC_IN_PCC(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;
+		}
+	}
+
+	if (cpc_read(cpu, enable_reg, &enable)) {
+		ret = -EFAULT;
+		goto out_err;
+	}
+
+	cppc_cpu_enable = enable;
+
+out_err:
+	if (regs_in_pcc)
+		up_write(&pcc_ss_data->pcc_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cppc_get_enable);
+
 /**
  * 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 6f651235933c..fcdedff8e6bd 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -139,6 +139,7 @@ struct cppc_cpudata {
 	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);
-- 
2.17.1

Re: [PATCH v2 0/7] CPPC optional registers AMD support

[Janakarajan Natarajan]

On 4/4/19 4:25 PM, 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 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
>
> The CPPC driver is updated to enable the OSPM and the userspace to
> access
> the newly supported registers.
>
> The purpose of exposing the registers via the 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.
> * By default, acpi_cpufreq will still be used.
>
> TODO:
> * Create a linux userspace tool that will help users generate a CPPC
> * profile
>    for their target workload.
> * Create or update a driver to apply 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.


Are there any concerns regarding this patchset?


Thanks.


>
> Janakarajan Natarajan (2):
>    acpi/cppc: Add macro for CPPC register BUFFER only check
>    acpi/cppc: Ensure only supported CPPC sysfs entries are created
>
> Yazen Ghannam (5):
>    acpi/cppc: Modify show_cppc_data macro
>    acpi/cppc: Rework cppc_set_perf() to use cppc_regs index
>    acpi/cppc: Add macros to define a R/W sysfs entry for CPPC registers
>    acpi/cppc: Add support for optional CPPC registers
>    acpi/cppc: Add support for CPPC Enable register
>
>   drivers/acpi/cppc_acpi.c       | 402 +++++++++++++++++++++++++++++----
>   drivers/cpufreq/cppc_cpufreq.c |   6 +-
>   include/acpi/cppc_acpi.h       |   6 +-
>   3 files changed, 372 insertions(+), 42 deletions(-)
>

Re: [PATCH v2 0/7] CPPC optional registers AMD support

[Rafael J. Wysocki]

On Tue, Apr 16, 2019 at 12:35 AM Janakarajan Natarajan <jnataraj@amd.com> wrote:
>
> On 4/4/19 4:25 PM, 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 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
> >
> > The CPPC driver is updated to enable the OSPM and the userspace to
> > access
> > the newly supported registers.
> >
> > The purpose of exposing the registers via the 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.
> > * By default, acpi_cpufreq will still be used.
> >
> > TODO:
> > * Create a linux userspace tool that will help users generate a CPPC
> > * profile
> >    for their target workload.
> > * Create or update a driver to apply 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.
>
>
> Are there any concerns regarding this patchset?

Yes, there are.

Unfortunately, it is generally problematic.

First off, the behavior of existing sysfs files cannot be changed at
will, as there may be users of them out there already depending on the
current behavior.

Second, at least some CPPC control registers are used by cpufreq
drivers (cppc_cpufreq and intel_pstate), so modifying them behind the
drivers' backs is not a good idea in general.  For this reason, adding
new sysfs attributes to allow user space to do that is quite
questionable.

Thanks,
Rafael

Re: [Devel] [PATCH v2 0/7] CPPC optional registers AMD support

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2409 bytes --]

On Tue, Apr 16, 2019 at 12:35 AM Janakarajan Natarajan <jnataraj(a)amd.com> wrote:
>
> On 4/4/19 4:25 PM, 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 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
> >
> > The CPPC driver is updated to enable the OSPM and the userspace to
> > access
> > the newly supported registers.
> >
> > The purpose of exposing the registers via the 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.
> > * By default, acpi_cpufreq will still be used.
> >
> > TODO:
> > * Create a linux userspace tool that will help users generate a CPPC
> > * profile
> >    for their target workload.
> > * Create or update a driver to apply 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.
>
>
> Are there any concerns regarding this patchset?

Yes, there are.

Unfortunately, it is generally problematic.

First off, the behavior of existing sysfs files cannot be changed at
will, as there may be users of them out there already depending on the
current behavior.

Second, at least some CPPC control registers are used by cpufreq
drivers (cppc_cpufreq and intel_pstate), so modifying them behind the
drivers' backs is not a good idea in general.  For this reason, adding
new sysfs attributes to allow user space to do that is quite
questionable.

Thanks,
Rafael

RE: [PATCH v2 0/7] CPPC optional registers AMD support

[Ghannam, Yazen]

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Tuesday, April 16, 2019 4:34 AM
> To: Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>
> Cc: Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>; 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: [PATCH v2 0/7] CPPC optional registers AMD support
> 
> On Tue, Apr 16, 2019 at 12:35 AM Janakarajan Natarajan <jnataraj@amd.com> wrote:
> >
> > On 4/4/19 4:25 PM, 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 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
> > >
> > > The CPPC driver is updated to enable the OSPM and the userspace to
> > > access
> > > the newly supported registers.
> > >
> > > The purpose of exposing the registers via the 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.
> > > * By default, acpi_cpufreq will still be used.
> > >
> > > TODO:
> > > * Create a linux userspace tool that will help users generate a CPPC
> > > * profile
> > >    for their target workload.
> > > * Create or update a driver to apply 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.
> >
> >
> > Are there any concerns regarding this patchset?
> 
> Yes, there are.
> 
> Unfortunately, it is generally problematic.
> 
> First off, the behavior of existing sysfs files cannot be changed at
> will, as there may be users of them out there already depending on the
> current behavior.
> 

The intent is to add new sysfs files without changing the existing files. Is that okay?

Or is the addition of new files also not acceptable?

> Second, at least some CPPC control registers are used by cpufreq
> drivers (cppc_cpufreq and intel_pstate), so modifying them behind the
> drivers' backs is not a good idea in general.  For this reason, adding
> new sysfs attributes to allow user space to do that is quite
> questionable.
> 

Yes, good point.

What if a check is added so that writes only succeed if the CPUFREQ governor is set to userspace?

Thanks,
Yazen

RE: [PATCH v2 0/7] CPPC optional registers AMD support

[Ghannam, Yazen]

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Tuesday, April 16, 2019 4:34 AM
> To: Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>
> Cc: Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>; 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: [PATCH v2 0/7] CPPC optional registers AMD support
> 
> On Tue, Apr 16, 2019 at 12:35 AM Janakarajan Natarajan <jnataraj@amd.com> wrote:
> >
> > On 4/4/19 4:25 PM, 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 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
> > >
> > > The CPPC driver is updated to enable the OSPM and the userspace to
> > > access
> > > the newly supported registers.
> > >
> > > The purpose of exposing the registers via the 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.
> > > * By default, acpi_cpufreq will still be used.
> > >
> > > TODO:
> > > * Create a linux userspace tool that will help users generate a CPPC
> > > * profile
> > >    for their target workload.
> > > * Create or update a driver to apply 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.
> >
> >
> > Are there any concerns regarding this patchset?
> 
> Yes, there are.
> 
> Unfortunately, it is generally problematic.
> 
> First off, the behavior of existing sysfs files cannot be changed at
> will, as there may be users of them out there already depending on the
> current behavior.
> 

The intent is to add new sysfs files without changing the existing files. Is that okay?

Or is the addition of new files also not acceptable?

> Second, at least some CPPC control registers are used by cpufreq
> drivers (cppc_cpufreq and intel_pstate), so modifying them behind the
> drivers' backs is not a good idea in general.  For this reason, adding
> new sysfs attributes to allow user space to do that is quite
> questionable.
> 

Yes, good point.

What if a check is added so that writes only succeed if the CPUFREQ governor is set to userspace?

Thanks,
Yazen

Re: [PATCH v2 0/7] CPPC optional registers AMD support

[Rafael J. Wysocki]

On Wed, Apr 17, 2019 at 7:28 PM Ghannam, Yazen <Yazen.Ghannam@amd.com> wrote:
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <rafael@kernel.org>
> > Sent: Tuesday, April 16, 2019 4:34 AM
> > To: Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>
> > Cc: Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>; 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: [PATCH v2 0/7] CPPC optional registers AMD support
> >
> > On Tue, Apr 16, 2019 at 12:35 AM Janakarajan Natarajan <jnataraj@amd.com> wrote:
> > >
> > > On 4/4/19 4:25 PM, 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 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
> > > >
> > > > The CPPC driver is updated to enable the OSPM and the userspace to
> > > > access
> > > > the newly supported registers.
> > > >
> > > > The purpose of exposing the registers via the 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.
> > > > * By default, acpi_cpufreq will still be used.
> > > >
> > > > TODO:
> > > > * Create a linux userspace tool that will help users generate a CPPC
> > > > * profile
> > > >    for their target workload.
> > > > * Create or update a driver to apply 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.
> > >
> > >
> > > Are there any concerns regarding this patchset?
> >
> > Yes, there are.
> >
> > Unfortunately, it is generally problematic.
> >
> > First off, the behavior of existing sysfs files cannot be changed at
> > will, as there may be users of them out there already depending on the
> > current behavior.
> >
>
> The intent is to add new sysfs files without changing the existing files. Is that okay?
>
> Or is the addition of new files also not acceptable?
>
> > Second, at least some CPPC control registers are used by cpufreq
> > drivers (cppc_cpufreq and intel_pstate), so modifying them behind the
> > drivers' backs is not a good idea in general.  For this reason, adding
> > new sysfs attributes to allow user space to do that is quite
> > questionable.
> >
>
> Yes, good point.
>
> What if a check is added so that writes only succeed if the CPUFREQ governor is set to userspace?

That wouldn't be particularly straightforward IMO.

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

Re: [Devel] [PATCH v2 0/7] CPPC optional registers AMD support

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 3856 bytes --]

On Wed, Apr 17, 2019 at 7:28 PM Ghannam, Yazen <Yazen.Ghannam(a)amd.com> wrote:
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <rafael(a)kernel.org>
> > Sent: Tuesday, April 16, 2019 4:34 AM
> > To: Natarajan, Janakarajan <Janakarajan.Natarajan(a)amd.com>
> > Cc: Natarajan, Janakarajan <Janakarajan.Natarajan(a)amd.com>; linux-acpi(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; linux-
> > pm(a)vger.kernel.org; devel(a)acpica.org; Rafael J . Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; Viresh Kumar
> > <viresh.kumar(a)linaro.org>; Robert Moore <robert.moore(a)intel.com>; Erik Schmauss <erik.schmauss(a)intel.com>; Ghannam, Yazen
> > <Yazen.Ghannam(a)amd.com>
> > Subject: Re: [PATCH v2 0/7] CPPC optional registers AMD support
> >
> > On Tue, Apr 16, 2019 at 12:35 AM Janakarajan Natarajan <jnataraj(a)amd.com> wrote:
> > >
> > > On 4/4/19 4:25 PM, 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 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
> > > >
> > > > The CPPC driver is updated to enable the OSPM and the userspace to
> > > > access
> > > > the newly supported registers.
> > > >
> > > > The purpose of exposing the registers via the 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.
> > > > * By default, acpi_cpufreq will still be used.
> > > >
> > > > TODO:
> > > > * Create a linux userspace tool that will help users generate a CPPC
> > > > * profile
> > > >    for their target workload.
> > > > * Create or update a driver to apply 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.
> > >
> > >
> > > Are there any concerns regarding this patchset?
> >
> > Yes, there are.
> >
> > Unfortunately, it is generally problematic.
> >
> > First off, the behavior of existing sysfs files cannot be changed at
> > will, as there may be users of them out there already depending on the
> > current behavior.
> >
>
> The intent is to add new sysfs files without changing the existing files. Is that okay?
>
> Or is the addition of new files also not acceptable?
>
> > Second, at least some CPPC control registers are used by cpufreq
> > drivers (cppc_cpufreq and intel_pstate), so modifying them behind the
> > drivers' backs is not a good idea in general.  For this reason, adding
> > new sysfs attributes to allow user space to do that is quite
> > questionable.
> >
>
> Yes, good point.
>
> What if a check is added so that writes only succeed if the CPUFREQ governor is set to userspace?

That wouldn't be particularly straightforward IMO.

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

RE: [PATCH v2 0/7] CPPC optional registers AMD support

[Ghannam, Yazen]

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Wednesday, April 17, 2019 5:11 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>; Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>; 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>
> Subject: Re: [PATCH v2 0/7] CPPC optional registers AMD support
> 
> On Wed, Apr 17, 2019 at 7:28 PM Ghannam, Yazen <Yazen.Ghannam@amd.com> wrote:
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki <rafael@kernel.org>
> > > Sent: Tuesday, April 16, 2019 4:34 AM
> > > To: Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>
> > > Cc: Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>; 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: [PATCH v2 0/7] CPPC optional registers AMD support
> > >
> > > On Tue, Apr 16, 2019 at 12:35 AM Janakarajan Natarajan <jnataraj@amd.com> wrote:
> > > >
> > > > On 4/4/19 4:25 PM, 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 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
> > > > >
> > > > > The CPPC driver is updated to enable the OSPM and the userspace to
> > > > > access
> > > > > the newly supported registers.
> > > > >
> > > > > The purpose of exposing the registers via the 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.
> > > > > * By default, acpi_cpufreq will still be used.
> > > > >
> > > > > TODO:
> > > > > * Create a linux userspace tool that will help users generate a CPPC
> > > > > * profile
> > > > >    for their target workload.
> > > > > * Create or update a driver to apply 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.
> > > >
> > > >
> > > > Are there any concerns regarding this patchset?
> > >
> > > Yes, there are.
> > >
> > > Unfortunately, it is generally problematic.
> > >
> > > First off, the behavior of existing sysfs files cannot be changed at
> > > will, as there may be users of them out there already depending on the
> > > current behavior.
> > >
> >
> > The intent is to add new sysfs files without changing the existing files. Is that okay?
> >
> > Or is the addition of new files also not acceptable?
> >
> > > Second, at least some CPPC control registers are used by cpufreq
> > > drivers (cppc_cpufreq and intel_pstate), so modifying them behind the
> > > drivers' backs is not a good idea in general.  For this reason, adding
> > > new sysfs attributes to allow user space to do that is quite
> > > questionable.
> > >
> >
> > Yes, good point.
> >
> > What if a check is added so that writes only succeed if the CPUFREQ governor is set to userspace?
> 
> That wouldn't be particularly straightforward IMO.
> 
> What about handling this like the others do, through a proper cpufreq driver?

That seemed unnecessary. We want to expose the CPPC interface directly so that advanced users can interact with the Platform without CPUFREQ. It seemed like the simplest way to do this is to extend the existing CPPC sysfs interface. Also, doing it this way would keep the interface driver/vendor-neutral, so users on any CPPC-enabled platform could use it.

However, I understand if that's too big of a change.

In this case, would you prefer something like the following?
1) Move the new interface somewhere else (another path, debugfs, etc.?) and mark it as "testing". This could reinforce that the new interface is for advanced users, testing, debugging, etc. However, the interface would still be populated by cppc_lib and would remain driver/vendor-neutral. And we could restrict the interface to prevent interactions with the CPUFREQ drivers.
2) Or, create a new CPUFREQ driver. The cppc_lib code could still be updated to support the new CPPC registers, but the driver would enable the sysfs interface. This would prevent any interactions with existing CPUFREQ drivers, but users will need to do more to enable the interface. We can still keep this driver vendor-neutral (cppc_userspace_cpufreq?).

Thanks,
Yazen