linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] CPPC optional registers AMD support
@ 2019-03-22 20:25 Natarajan, Janakarajan
  2019-03-22 20:25 ` [PATCH 1/6] acpi/cppc: Ensure only supported CPPC sysfs entries are created Natarajan, Janakarajan
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Natarajan, Janakarajan @ 2019-03-22 20:25 UTC (permalink / raw)
  To: linux-acpi, linux-kernel, linux-pm, devel
  Cc: Rafael J . Wysocki, Len Brown, Viresh Kumar, Robert Moore,
	Erik Schmauss, 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.

Janakarajan Natarajan (1):
  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       | 363 +++++++++++++++++++++++++++++----
 drivers/cpufreq/cppc_cpufreq.c |   6 +-
 include/acpi/cppc_acpi.h       |   6 +-
 3 files changed, 333 insertions(+), 42 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/6] acpi/cppc: Ensure only supported CPPC sysfs entries are created
  2019-03-22 20:25 [PATCH 0/6] CPPC optional registers AMD support Natarajan, Janakarajan
@ 2019-03-22 20:25 ` Natarajan, Janakarajan
  2019-03-22 20:26 ` [PATCH 2/6] acpi/cppc: Modify show_cppc_data macro Natarajan, Janakarajan
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Natarajan, Janakarajan @ 2019-03-22 20:25 UTC (permalink / raw)
  To: linux-acpi, linux-kernel, linux-pm, devel
  Cc: Rafael J . Wysocki, Len Brown, Viresh Kumar, Robert Moore,
	Erik Schmauss, Natarajan, Janakarajan

Add attributes 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 | 70 ++++++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 1b207fca1420..66fad270376c 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -179,22 +179,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)
@@ -709,6 +695,57 @@ static bool is_cppc_supported(int revision, int num_ent)
  *	}
  */
 
+int set_cppc_attrs(struct cpc_desc *cpc, int entries)
+{
+	static struct attribute **cppc_attrs;
+	int i, attr_i = 0;
+
+	/* Extra for feedback_ctrs to be exposed via sysfs */
+	cppc_attrs = kcalloc(entries + 1, sizeof(*cppc_attrs), GFP_KERNEL);
+	if (!cppc_attrs)
+		return -ENOMEM;
+
+	for (i = 0; i < MAX_CPC_REG_ENT && attr_i < entries; i++) {
+		if (!CPC_SUPPORTED(&cpc->cpc_regs[i]))
+			continue;
+
+		switch (i) {
+		case HIGHEST_PERF:
+			cppc_attrs[attr_i++] = &highest_perf.attr;
+			break;
+		case NOMINAL_PERF:
+			cppc_attrs[attr_i++] = &nominal_perf.attr;
+			break;
+		case LOW_NON_LINEAR_PERF:
+			cppc_attrs[attr_i++] = &lowest_nonlinear_perf.attr;
+			break;
+		case LOWEST_PERF:
+			cppc_attrs[attr_i++] = &lowest_perf.attr;
+			break;
+		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 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.
@@ -863,6 +900,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) {
@@ -924,6 +965,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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/6] acpi/cppc: Modify show_cppc_data macro
  2019-03-22 20:25 [PATCH 0/6] CPPC optional registers AMD support Natarajan, Janakarajan
  2019-03-22 20:25 ` [PATCH 1/6] acpi/cppc: Ensure only supported CPPC sysfs entries are created Natarajan, Janakarajan
@ 2019-03-22 20:26 ` Natarajan, Janakarajan
  2019-03-22 20:26 ` [PATCH 3/6] acpi/cppc: Rework cppc_set_perf() to use cppc_regs index Natarajan, Janakarajan
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Natarajan, Janakarajan @ 2019-03-22 20:26 UTC (permalink / raw)
  To: linux-acpi, linux-kernel, linux-pm, devel
  Cc: Rafael J . Wysocki, Len Brown, Viresh Kumar, Robert Moore,
	Erik Schmauss, Ghannam, Yazen, 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 66fad270376c..9daeb0b034d5 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -151,17 +151,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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/6] acpi/cppc: Rework cppc_set_perf() to use cppc_regs index
  2019-03-22 20:25 [PATCH 0/6] CPPC optional registers AMD support Natarajan, Janakarajan
  2019-03-22 20:25 ` [PATCH 1/6] acpi/cppc: Ensure only supported CPPC sysfs entries are created Natarajan, Janakarajan
  2019-03-22 20:26 ` [PATCH 2/6] acpi/cppc: Modify show_cppc_data macro Natarajan, Janakarajan
@ 2019-03-22 20:26 ` Natarajan, Janakarajan
  2019-03-22 20:26 ` [PATCH 4/6] acpi/cppc: Add macros to define a R/W sysfs entry for CPPC registers Natarajan, Janakarajan
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Natarajan, Janakarajan @ 2019-03-22 20:26 UTC (permalink / raw)
  To: linux-acpi, linux-kernel, linux-pm, devel
  Cc: Rafael J . Wysocki, Len Brown, Viresh Kumar, Robert Moore,
	Erik Schmauss, Ghannam, Yazen, 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 9daeb0b034d5..e81c19316628 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
@@ -1303,26 +1303,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
@@ -1331,7 +1343,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;
@@ -1358,14 +1370,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.
 	 *
@@ -1408,7 +1420,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)
@@ -1424,7 +1436,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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/6] acpi/cppc: Add macros to define a R/W sysfs entry for CPPC registers
  2019-03-22 20:25 [PATCH 0/6] CPPC optional registers AMD support Natarajan, Janakarajan
                   ` (2 preceding siblings ...)
  2019-03-22 20:26 ` [PATCH 3/6] acpi/cppc: Rework cppc_set_perf() to use cppc_regs index Natarajan, Janakarajan
@ 2019-03-22 20:26 ` Natarajan, Janakarajan
  2019-03-22 20:26 ` [PATCH 5/6] acpi/cppc: Add support for optional " Natarajan, Janakarajan
  2019-03-22 20:26 ` [PATCH 6/6] acpi/cppc: Add support for CPPC Enable register Natarajan, Janakarajan
  5 siblings, 0 replies; 11+ messages in thread
From: Natarajan, Janakarajan @ 2019-03-22 20:26 UTC (permalink / raw)
  To: linux-acpi, linux-kernel, linux-pm, devel
  Cc: Rafael J . Wysocki, Len Brown, Viresh Kumar, Robert Moore,
	Erik Schmauss, Ghannam, Yazen, 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 e81c19316628..7cb23b369fc7 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -134,6 +134,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)		\
@@ -156,6 +160,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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/6] acpi/cppc: Add support for optional CPPC registers
  2019-03-22 20:25 [PATCH 0/6] CPPC optional registers AMD support Natarajan, Janakarajan
                   ` (3 preceding siblings ...)
  2019-03-22 20:26 ` [PATCH 4/6] acpi/cppc: Add macros to define a R/W sysfs entry for CPPC registers Natarajan, Janakarajan
@ 2019-03-22 20:26 ` Natarajan, Janakarajan
  2019-03-27 15:47   ` Pandruvada, Srinivas
  2019-03-22 20:26 ` [PATCH 6/6] acpi/cppc: Add support for CPPC Enable register Natarajan, Janakarajan
  5 siblings, 1 reply; 11+ messages in thread
From: Natarajan, Janakarajan @ 2019-03-22 20:26 UTC (permalink / raw)
  To: linux-acpi, linux-kernel, linux-pm, devel
  Cc: Rafael J . Wysocki, Len Brown, Viresh Kumar, Robert Moore,
	Erik Schmauss, Ghannam, Yazen, 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 | 119 ++++++++++++++++++++++++++++++++++++---
 include/acpi/cppc_acpi.h |   3 +
 2 files changed, 114 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 7cb23b369fc7..f8827ba7015d 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -196,6 +196,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)
@@ -768,6 +779,21 @@ 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;
 		}
 	}
 
@@ -1348,7 +1374,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) {
@@ -1360,6 +1386,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;
@@ -1375,6 +1413,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;
@@ -1397,13 +1436,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
@@ -1451,7 +1487,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)
@@ -1469,6 +1505,73 @@ 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_SUPPORTED(max_reg) && cpc_read(cpu, max_reg, &max))
+		ret = -EFAULT;
+
+	if (CPC_SUPPORTED(min_reg) && cpc_read(cpu, min_reg, &min))
+		ret = -EFAULT;
+
+	if (CPC_SUPPORTED(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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 6/6] acpi/cppc: Add support for CPPC Enable register
  2019-03-22 20:25 [PATCH 0/6] CPPC optional registers AMD support Natarajan, Janakarajan
                   ` (4 preceding siblings ...)
  2019-03-22 20:26 ` [PATCH 5/6] acpi/cppc: Add support for optional " Natarajan, Janakarajan
@ 2019-03-22 20:26 ` Natarajan, Janakarajan
  5 siblings, 0 replies; 11+ messages in thread
From: Natarajan, Janakarajan @ 2019-03-22 20:26 UTC (permalink / raw)
  To: linux-acpi, linux-kernel, linux-pm, devel
  Cc: Rafael J . Wysocki, Len Brown, Viresh Kumar, Robert Moore,
	Erik Schmauss, Ghannam, Yazen, 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 f8827ba7015d..8c6804976bb8 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -224,6 +224,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,
 };
@@ -794,6 +831,9 @@ 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;
 		}
 	}
 
@@ -1383,6 +1423,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;
@@ -1572,6 +1615,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_SUPPORTED(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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 5/6] acpi/cppc: Add support for optional CPPC registers
  2019-03-22 20:26 ` [PATCH 5/6] acpi/cppc: Add support for optional " Natarajan, Janakarajan
@ 2019-03-27 15:47   ` Pandruvada, Srinivas
  2019-03-29 20:18     ` Ghannam, Yazen
  0 siblings, 1 reply; 11+ messages in thread
From: Pandruvada, Srinivas @ 2019-03-27 15:47 UTC (permalink / raw)
  To: linux-kernel, devel, Janakarajan.Natarajan, linux-acpi, linux-pm
  Cc: Yazen.Ghannam, lenb, viresh.kumar, Moore, Robert, Schmauss, Erik, rjw

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

On Fri, 2019-03-22 at 20:26 +0000, Natarajan, Janakarajan wrote:
> 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>
> 

[..]

> +	/* desired_perf is the only mandatory value in perf_ctrls */
> +	if (cpc_read(cpu, desired_reg, &desired))
> +		ret = -EFAULT;
> +
> +	if (CPC_SUPPORTED(max_reg) && cpc_read(cpu, max_reg, &max))
> +		ret = -EFAULT;
> +
We should create and use different macro other than CPPC_SUPPORTED.
CPC_SUPPORTED doesn't validate the correctness of object type for a
field. For example "Maximum Performance Register" can only be buffer
not integer. In this way invalid field definitions can be ignored.


> +	if (CPC_SUPPORTED(min_reg) && cpc_read(cpu, min_reg, &min))
> +		ret = -EFAULT;
> +
> +	if (CPC_SUPPORTED(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;
> +
Here it is fine to use CPC_SUPPORTED as the "Autonomous Selection
Enable" can be both integer and buffer.

Thanks,
Srinivas


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3290 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 5/6] acpi/cppc: Add support for optional CPPC registers
  2019-03-27 15:47   ` Pandruvada, Srinivas
@ 2019-03-29 20:18     ` Ghannam, Yazen
  2019-03-29 23:02       ` Rafael J. Wysocki
  2019-03-30  2:40       ` Pandruvada, Srinivas
  0 siblings, 2 replies; 11+ messages in thread
From: Ghannam, Yazen @ 2019-03-29 20:18 UTC (permalink / raw)
  To: Pandruvada, Srinivas, linux-kernel, devel, Natarajan,
	Janakarajan, linux-acpi, linux-pm
  Cc: lenb, viresh.kumar, Moore, Robert, Schmauss, Erik, rjw

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org <linux-acpi-owner@vger.kernel.org> On Behalf Of Pandruvada, Srinivas
> Sent: Wednesday, March 27, 2019 10:48 AM
> To: linux-kernel@vger.kernel.org; devel@acpica.org; Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>; linux-
> acpi@vger.kernel.org; linux-pm@vger.kernel.org
> Cc: Ghannam, Yazen <Yazen.Ghannam@amd.com>; lenb@kernel.org; viresh.kumar@linaro.org; Moore, Robert
> <robert.moore@intel.com>; Schmauss, Erik <erik.schmauss@intel.com>; rjw@rjwysocki.net
> Subject: Re: [PATCH 5/6] acpi/cppc: Add support for optional CPPC registers
> 
> On Fri, 2019-03-22 at 20:26 +0000, Natarajan, Janakarajan wrote:
> > 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>
> >
> 
> [..]
> 
> > +	/* desired_perf is the only mandatory value in perf_ctrls */
> > +	if (cpc_read(cpu, desired_reg, &desired))
> > +		ret = -EFAULT;
> > +
> > +	if (CPC_SUPPORTED(max_reg) && cpc_read(cpu, max_reg, &max))
> > +		ret = -EFAULT;
> > +
> We should create and use different macro other than CPPC_SUPPORTED.
> CPC_SUPPORTED doesn't validate the correctness of object type for a
> field. For example "Maximum Performance Register" can only be buffer
> not integer. In this way invalid field definitions can be ignored.
> 

So create something like "CPPC_SUPPORTED_BUFFER" for buffer-only registers?

And then buffer/integer registers will continue to use "CPPC_SUPPORTED".

These seem to be the only two cases at this time. Is this okay?

Thanks,
Yazen

> 
> > +	if (CPC_SUPPORTED(min_reg) && cpc_read(cpu, min_reg, &min))
> > +		ret = -EFAULT;
> > +
> > +	if (CPC_SUPPORTED(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;
> > +
> Here it is fine to use CPC_SUPPORTED as the "Autonomous Selection
> Enable" can be both integer and buffer.
> 
> Thanks,
> Srinivas


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 5/6] acpi/cppc: Add support for optional CPPC registers
  2019-03-29 20:18     ` Ghannam, Yazen
@ 2019-03-29 23:02       ` Rafael J. Wysocki
  2019-03-30  2:40       ` Pandruvada, Srinivas
  1 sibling, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2019-03-29 23:02 UTC (permalink / raw)
  To: Ghannam, Yazen
  Cc: Pandruvada, Srinivas, linux-kernel, devel, Natarajan,
	Janakarajan, linux-acpi, linux-pm, lenb, viresh.kumar, Moore,
	Robert, Schmauss, Erik

On Friday, March 29, 2019 9:18:04 PM CET Ghannam, Yazen wrote:
> > -----Original Message-----
> > From: linux-acpi-owner@vger.kernel.org <linux-acpi-owner@vger.kernel.org> On Behalf Of Pandruvada, Srinivas
> > Sent: Wednesday, March 27, 2019 10:48 AM
> > To: linux-kernel@vger.kernel.org; devel@acpica.org; Natarajan, Janakarajan <Janakarajan.Natarajan@amd.com>; linux-
> > acpi@vger.kernel.org; linux-pm@vger.kernel.org
> > Cc: Ghannam, Yazen <Yazen.Ghannam@amd.com>; lenb@kernel.org; viresh.kumar@linaro.org; Moore, Robert
> > <robert.moore@intel.com>; Schmauss, Erik <erik.schmauss@intel.com>; rjw@rjwysocki.net
> > Subject: Re: [PATCH 5/6] acpi/cppc: Add support for optional CPPC registers
> > 
> > On Fri, 2019-03-22 at 20:26 +0000, Natarajan, Janakarajan wrote:
> > > 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>
> > >
> > 
> > [..]
> > 
> > > +	/* desired_perf is the only mandatory value in perf_ctrls */
> > > +	if (cpc_read(cpu, desired_reg, &desired))
> > > +		ret = -EFAULT;
> > > +
> > > +	if (CPC_SUPPORTED(max_reg) && cpc_read(cpu, max_reg, &max))
> > > +		ret = -EFAULT;
> > > +
> > We should create and use different macro other than CPPC_SUPPORTED.
> > CPC_SUPPORTED doesn't validate the correctness of object type for a
> > field. For example "Maximum Performance Register" can only be buffer
> > not integer. In this way invalid field definitions can be ignored.
> > 
> 
> So create something like "CPPC_SUPPORTED_BUFFER" for buffer-only registers?
> 
> And then buffer/integer registers will continue to use "CPPC_SUPPORTED".
> 
> These seem to be the only two cases at this time. Is this okay?

Yes, something like that.

Thanks,
Rafael


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 5/6] acpi/cppc: Add support for optional CPPC registers
  2019-03-29 20:18     ` Ghannam, Yazen
  2019-03-29 23:02       ` Rafael J. Wysocki
@ 2019-03-30  2:40       ` Pandruvada, Srinivas
  1 sibling, 0 replies; 11+ messages in thread
From: Pandruvada, Srinivas @ 2019-03-30  2:40 UTC (permalink / raw)
  To: Yazen.Ghannam, linux-kernel, devel, Janakarajan.Natarajan,
	linux-acpi, linux-pm
  Cc: lenb, viresh.kumar, Moore, Robert, Schmauss, Erik, rjw

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

On Fri, 2019-03-29 at 20:18 +0000, Ghannam, Yazen wrote:
> > -----Original Message-----
> > From: linux-acpi-owner@vger.kernel.org <
> > linux-acpi-owner@vger.kernel.org> On Behalf Of Pandruvada, Srinivas
> > Sent: Wednesday, March 27, 2019 10:48 AM
> > To: linux-kernel@vger.kernel.org; devel@acpica.org; Natarajan,
> > Janakarajan <Janakarajan.Natarajan@amd.com>; linux-
> > acpi@vger.kernel.org; linux-pm@vger.kernel.org
> > Cc: Ghannam, Yazen <Yazen.Ghannam@amd.com>; lenb@kernel.org; 
> > viresh.kumar@linaro.org; Moore, Robert
> > <robert.moore@intel.com>; Schmauss, Erik <erik.schmauss@intel.com>;
> > rjw@rjwysocki.net
> > Subject: Re: [PATCH 5/6] acpi/cppc: Add support for optional CPPC
> > registers
> > 
> > On Fri, 2019-03-22 at 20:26 +0000, Natarajan, Janakarajan wrote:
> > > 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>
> > > 
> > 
> > [..]
> > 
> > > +	/* desired_perf is the only mandatory value in perf_ctrls */
> > > +	if (cpc_read(cpu, desired_reg, &desired))
> > > +		ret = -EFAULT;
> > > +
> > > +	if (CPC_SUPPORTED(max_reg) && cpc_read(cpu, max_reg, &max))
> > > +		ret = -EFAULT;
> > > +
> > 
> > We should create and use different macro other than CPPC_SUPPORTED.
> > CPC_SUPPORTED doesn't validate the correctness of object type for a
> > field. For example "Maximum Performance Register" can only be
> > buffer
> > not integer. In this way invalid field definitions can be ignored.
> > 
> 
> So create something like "CPPC_SUPPORTED_BUFFER" for buffer-only
> registers?
> 
> And then buffer/integer registers will continue to use
> "CPPC_SUPPORTED".
> 
> These seem to be the only two cases at this time. Is this okay?
Yes.

Thanks,
Srinivas

> 
> Thanks,
> Yazen
> 
> > 
> > > +	if (CPC_SUPPORTED(min_reg) && cpc_read(cpu, min_reg, &min))
> > > +		ret = -EFAULT;
> > > +
> > > +	if (CPC_SUPPORTED(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;
> > > +
> > 
> > Here it is fine to use CPC_SUPPORTED as the "Autonomous Selection
> > Enable" can be both integer and buffer.
> > 
> > Thanks,
> > Srinivas
> 
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3290 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-03-30  2:40 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-22 20:25 [PATCH 0/6] CPPC optional registers AMD support Natarajan, Janakarajan
2019-03-22 20:25 ` [PATCH 1/6] acpi/cppc: Ensure only supported CPPC sysfs entries are created Natarajan, Janakarajan
2019-03-22 20:26 ` [PATCH 2/6] acpi/cppc: Modify show_cppc_data macro Natarajan, Janakarajan
2019-03-22 20:26 ` [PATCH 3/6] acpi/cppc: Rework cppc_set_perf() to use cppc_regs index Natarajan, Janakarajan
2019-03-22 20:26 ` [PATCH 4/6] acpi/cppc: Add macros to define a R/W sysfs entry for CPPC registers Natarajan, Janakarajan
2019-03-22 20:26 ` [PATCH 5/6] acpi/cppc: Add support for optional " Natarajan, Janakarajan
2019-03-27 15:47   ` Pandruvada, Srinivas
2019-03-29 20:18     ` Ghannam, Yazen
2019-03-29 23:02       ` Rafael J. Wysocki
2019-03-30  2:40       ` Pandruvada, Srinivas
2019-03-22 20:26 ` [PATCH 6/6] acpi/cppc: Add support for CPPC Enable register Natarajan, Janakarajan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).