On Thu, Sep 09, 2021 at 08:21:48AM +0800, Shuah Khan wrote:

On 9/8/21 8:59 AM, Huang Rui wrote:

quoted

From: Jinzhou Su <redacted>

Export the cppc enable register function for future use.

This patch also adds a new function. How about saying something about
adding a new function.

quoted

Signed-off-by: Jinzhou Su <redacted>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---
  drivers/acpi/cppc_acpi.c | 42 ++++++++++++++++++++++++++++++++++++++++
  include/acpi/cppc_acpi.h |  5 +++++
  2 files changed, 47 insertions(+)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index a4d4eebba1da..de4b30545215 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c

@@ -1220,6 +1220,48 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
  }
  EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
  
+/**
+ * cppc_set_enable - Set to enable CPPC register.

Please make this more descriptive - does it write to register
What is the behavior in error paths etc.

quoted

+ * @cpu: CPU for which to enable CPPC register.
+ * @enable: enable field to write into share memory.

What should this be? What are the valid values to write?
Also aren't we adding this to header file where prtotype
is defined these days?

Thank you for the suggestions, I will refine the comments and commmit log
in V2.

quoted

+ *
+ * Return: 0 for success, -ERRNO otherwise.
+ */
+int cppc_set_enable(int cpu, u32 enable)
+{
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cpc_register_resource *enable_reg;
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret = -1;
+
+	if (!cpc_desc) {
+		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
+		return -ENODEV;
+	}
+

Don't we need to do some error checking on input args? What is the
valid range for cpu and enbale?

Good point.

quoted

+	enable_reg = &cpc_desc->cpc_regs[ENABLE];
+
+	if (CPC_IN_PCC(enable_reg)) {
+
+		if (pcc_ss_id < 0)
+			return -EIO;
+
+		ret = cpc_write(cpu, enable_reg, enable);
+		if (ret)
+			return ret;
+
+		pcc_ss_data = pcc_data[pcc_ss_id];
+
+		down_write(&pcc_ss_data->pcc_lock);
+		send_pcc_cmd(pcc_ss_id, CMD_WRITE);

Could this fail?

Will add error handling in V2.

Thanks,
Ray