Hello Lifeng,
On 11/14/24 09:48, Lifeng Zheng wrote:
Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().
Rename cppc_get_perf() to cppc_get_reg() as a generic function to read cppc
registers, with two changes:
1. Change the error kind to "no such device" when pcc_ss_id < 0, which
means that this cpu cannot get a valid pcc_ss_id.
2. Add a check to verify if the register is a cpc supported one before
using it.
Add cppc_set_reg() as a generic function for setting cppc registers. Unlike
other set reg ABIs, this function checks CPC_SUPPORTED right after getting
the register, because the rest of the operations are meaningless if this
register is not a cpc supported one.
Signed-off-by: Lifeng Zheng <zhenglifeng1@xxxxxxxxxx>
---
drivers/acpi/cppc_acpi.c | 191 +++++++++++++++------------------------
1 file changed, 72 insertions(+), 119 deletions(-)
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index c1f3568d0c50..306ced9c3376 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1179,10 +1179,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
return ret_val;
}
-static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
+static int cppc_get_reg(int cpunum, enum cppc_regs reg_idx, u64 *val)
{
struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+ struct cppc_pcc_data *pcc_ss_data = NULL;
struct cpc_register_resource *reg;
+ int pcc_ss_id;
+ int ret = 0;
NIT: Might not be necessary if we save the value returned by cpc_read(),
cf. other comment below.
if (!cpc_desc) {
pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
@@ -1191,20 +1194,23 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
reg = &cpc_desc->cpc_regs[reg_idx];
+ if (!CPC_SUPPORTED(reg)) {
+ pr_debug("CPC register (reg_idx=%u) is not supported\n", reg_idx);
+ return -EOPNOTSUPP;
+ }
+
if (CPC_IN_PCC(reg)) {
- int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
- struct cppc_pcc_data *pcc_ss_data = NULL;
- int ret = 0;
+ pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
if (pcc_ss_id < 0)
- return -EIO;
+ return -ENODEV;
NIT: Could add here:
pr_debug("Invalid pcc_ss_id\n");
just as you did in cppc_set_reg()
pcc_ss_data = pcc_data[pcc_ss_id];
down_write(&pcc_ss_data->pcc_lock);
if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
- cpc_read(cpunum, reg, perf);
+ cpc_read(cpunum, reg, val);
This was not introduced by your patch, but cpc_read() return a value.
Shouldn't we return it instead of 0 ?
else
ret = -EIO;
@@ -1213,21 +1219,65 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
return ret;
}
- cpc_read(cpunum, reg, perf);
+ cpc_read(cpunum, reg, val);
Same comment as above
return 0;
}
+static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)
Just to have similar functions, maybe 'cpu' should be renamed to 'cpunum' ?
Or the other way around.
+{
+ struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+ struct cppc_pcc_data *pcc_ss_data = NULL;
+ struct cpc_register_resource *reg;
+ int pcc_ss_id;
+ int ret;
+
+ if (!cpc_desc) {
+ pr_debug("No CPC descriptor for CPU:%d\n", cpu);
+ return -ENODEV;
+ }
+
+ reg = &cpc_desc->cpc_regs[reg_idx];
+
+ if (!CPC_SUPPORTED(reg)) {
+ pr_debug("CPC register (reg_idx=%u) is not supported\n", reg_idx);
+ return -EOPNOTSUPP;
+ }
+
+ if (CPC_IN_PCC(reg)) {
+ pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+
+ if (pcc_ss_id < 0) {
+ pr_debug("Invalid pcc_ss_id\n");
+ return -ENODEV;
+ }
+
+ ret = cpc_write(cpu, reg, val);
+ if (ret)
+ return ret;
+
+ pcc_ss_data = pcc_data[pcc_ss_id];
+
+ down_write(&pcc_ss_data->pcc_lock);
+ /* after writing CPC, transfer the ownership of PCC to platform */
+ ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
+ up_write(&pcc_ss_data->pcc_lock);
+ return ret;
+ }
+
+ return cpc_write(cpu, reg, val);
+}
+
[snip]
