| Message ID | 20241216091603.1247644-2-zhenglifeng1@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Support for autonomous selection in cppc_cpufreq | expand |
Hello Lifeng, On 12/16/24 10:16, Lifeng Zheng wrote: > Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read > cppc registers, with four 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. > > 3. Extract the operations if register is in pcc out as > cppc_get_reg_val_in_pcc(). > > 4. Return the result of cpc_read() instead of 0. > > Add cppc_set_reg_val_in_pcc() and cppc_set_reg_val() as generic functions > for setting cppc registers value. Unlike other set reg ABIs, > cppc_set_reg_val() 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. > > These functions can be used to reduce some existing code duplication. > > Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> > --- > drivers/acpi/cppc_acpi.c | 111 +++++++++++++++++++++++++++++---------- > 1 file changed, 84 insertions(+), 27 deletions(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index c1f3568d0c50..bb5333a503a2 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -1179,43 +1179,100 @@ 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_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val) > { > - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum); > + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); > + struct cppc_pcc_data *pcc_ss_data = NULL; > + int ret; > + > + if (pcc_ss_id < 0) { > + pr_debug("Invalid pcc_ss_id\n"); > + return -ENODEV; > + } > + > + 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) > + ret = cpc_read(cpu, reg, val); > + else > + ret = -EIO; > + > + up_write(&pcc_ss_data->pcc_lock); > + > + return ret; > +} > + > +static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val) > +{ > + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu); > struct cpc_register_resource *reg; > > if (!cpc_desc) { > - pr_debug("No CPC descriptor for CPU:%d\n", cpunum); > + pr_debug("No CPC descriptor for CPU:%d\n", cpu); > return -ENODEV; > } > > reg = &cpc_desc->cpc_regs[reg_idx]; > > - 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; > - > - if (pcc_ss_id < 0) > - return -EIO; > + if (!CPC_SUPPORTED(reg)) { > + pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx); > + return -EOPNOTSUPP; > + } I think this is only valid for optional fields. Meaning that: - if the function is used one day for the mandatory 'Lowest Performance' field, an integer value of 0 would be valid. - if the function is used for a mandatory field containing a NULL Buffer, it seems we would return -EFAULT currently, through cpc_read(). -EOPNOTSUPP doesn't seem appropriate as the field would be mandatory. Maybe the function needs an additional 'bool optional' input parameter to do these check conditionally. > > - pcc_ss_data = pcc_data[pcc_ss_id]; > + if (CPC_IN_PCC(reg)) > + return cppc_get_reg_val_in_pcc(cpu, reg, val); > > - down_write(&pcc_ss_data->pcc_lock); > + return cpc_read(cpu, reg, val); > +} > > - if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0) > - cpc_read(cpunum, reg, perf); > - else > - ret = -EIO; > +static int cppc_set_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 val) > +{ > + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); > + struct cppc_pcc_data *pcc_ss_data = NULL; > + int ret; > > - up_write(&pcc_ss_data->pcc_lock); > + 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; > +} > + > +static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val) > +{ > + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu); > + struct cpc_register_resource *reg; > + > + if (!cpc_desc) { > + pr_debug("No CPC descriptor for CPU:%d\n", cpu); > + return -ENODEV; > } > > - cpc_read(cpunum, reg, perf); > + reg = &cpc_desc->cpc_regs[reg_idx]; > > - return 0; > + if (!CPC_SUPPORTED(reg)) { > + pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx); > + return -EOPNOTSUPP; > + } Similarly to cppc_get_reg_val(), if a field is: - mandatory + integer: currently doesn't exist. Not sure we should try to detect that, but might be safer. - mandatory + buffer: should not return -EOPNOTSUPP I think - optional + integer: e.g.: 'Autonomous Selection Enable Register', we should return -EOPNOTSUPP. It seems that currently, if the integer value is 1, I get a 'write error: Bad address' - optional + buffer: should effectively return -EOPNOTSUPP if the buffer is NULL. > + > + if (CPC_IN_PCC(reg)) > + return cppc_set_reg_val_in_pcc(cpu, reg, val); > + > + return cpc_write(cpu, reg, val); > } > > /** > @@ -1223,11 +1280,11 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf) > * @cpunum: CPU from which to get desired performance. > * @desired_perf: Return address. > * > - * Return: 0 for success, -EIO otherwise. > + * Return: 0 for success, -ERRNO otherwise. > */ > int cppc_get_desired_perf(int cpunum, u64 *desired_perf) > { > - return cppc_get_perf(cpunum, DESIRED_PERF, desired_perf); > + return cppc_get_reg_val(cpunum, DESIRED_PERF, desired_perf); > } > EXPORT_SYMBOL_GPL(cppc_get_desired_perf); > > @@ -1236,11 +1293,11 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf); > * @cpunum: CPU from which to get nominal performance. > * @nominal_perf: Return address. > * > - * Return: 0 for success, -EIO otherwise. > + * Return: 0 for success, -ERRNO otherwise. > */ > int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf) > { > - return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf); > + return cppc_get_reg_val(cpunum, NOMINAL_PERF, nominal_perf); > } > > /** > @@ -1248,11 +1305,11 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf) > * @cpunum: CPU from which to get highest performance. > * @highest_perf: Return address. > * > - * Return: 0 for success, -EIO otherwise. > + * Return: 0 for success, -ERRNO otherwise. > */ > int cppc_get_highest_perf(int cpunum, u64 *highest_perf) > { > - return cppc_get_perf(cpunum, HIGHEST_PERF, highest_perf); > + return cppc_get_reg_val(cpunum, HIGHEST_PERF, highest_perf); > } > EXPORT_SYMBOL_GPL(cppc_get_highest_perf); > > @@ -1261,11 +1318,11 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf); > * @cpunum: CPU from which to get epp preference value. > * @epp_perf: Return address. > * > - * Return: 0 for success, -EIO otherwise. > + * Return: 0 for success, -ERRNO otherwise. > */ > int cppc_get_epp_perf(int cpunum, u64 *epp_perf) > { > - return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf); > + return cppc_get_reg_val(cpunum, ENERGY_PERF, epp_perf); > } > EXPORT_SYMBOL_GPL(cppc_get_epp_perf); >
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index c1f3568d0c50..bb5333a503a2 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -1179,43 +1179,100 @@ 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_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val) { - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum); + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); + struct cppc_pcc_data *pcc_ss_data = NULL; + int ret; + + if (pcc_ss_id < 0) { + pr_debug("Invalid pcc_ss_id\n"); + return -ENODEV; + } + + 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) + ret = cpc_read(cpu, reg, val); + else + ret = -EIO; + + up_write(&pcc_ss_data->pcc_lock); + + return ret; +} + +static int cppc_get_reg_val(int cpu, enum cppc_regs reg_idx, u64 *val) +{ + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu); struct cpc_register_resource *reg; if (!cpc_desc) { - pr_debug("No CPC descriptor for CPU:%d\n", cpunum); + pr_debug("No CPC descriptor for CPU:%d\n", cpu); return -ENODEV; } reg = &cpc_desc->cpc_regs[reg_idx]; - 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; - - if (pcc_ss_id < 0) - return -EIO; + if (!CPC_SUPPORTED(reg)) { + pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx); + return -EOPNOTSUPP; + } - pcc_ss_data = pcc_data[pcc_ss_id]; + if (CPC_IN_PCC(reg)) + return cppc_get_reg_val_in_pcc(cpu, reg, val); - down_write(&pcc_ss_data->pcc_lock); + return cpc_read(cpu, reg, val); +} - if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0) - cpc_read(cpunum, reg, perf); - else - ret = -EIO; +static int cppc_set_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 val) +{ + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); + struct cppc_pcc_data *pcc_ss_data = NULL; + int ret; - up_write(&pcc_ss_data->pcc_lock); + 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; +} + +static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val) +{ + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu); + struct cpc_register_resource *reg; + + if (!cpc_desc) { + pr_debug("No CPC descriptor for CPU:%d\n", cpu); + return -ENODEV; } - cpc_read(cpunum, reg, perf); + reg = &cpc_desc->cpc_regs[reg_idx]; - return 0; + if (!CPC_SUPPORTED(reg)) { + pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx); + return -EOPNOTSUPP; + } + + if (CPC_IN_PCC(reg)) + return cppc_set_reg_val_in_pcc(cpu, reg, val); + + return cpc_write(cpu, reg, val); } /** @@ -1223,11 +1280,11 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf) * @cpunum: CPU from which to get desired performance. * @desired_perf: Return address. * - * Return: 0 for success, -EIO otherwise. + * Return: 0 for success, -ERRNO otherwise. */ int cppc_get_desired_perf(int cpunum, u64 *desired_perf) { - return cppc_get_perf(cpunum, DESIRED_PERF, desired_perf); + return cppc_get_reg_val(cpunum, DESIRED_PERF, desired_perf); } EXPORT_SYMBOL_GPL(cppc_get_desired_perf); @@ -1236,11 +1293,11 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf); * @cpunum: CPU from which to get nominal performance. * @nominal_perf: Return address. * - * Return: 0 for success, -EIO otherwise. + * Return: 0 for success, -ERRNO otherwise. */ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf) { - return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf); + return cppc_get_reg_val(cpunum, NOMINAL_PERF, nominal_perf); } /** @@ -1248,11 +1305,11 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf) * @cpunum: CPU from which to get highest performance. * @highest_perf: Return address. * - * Return: 0 for success, -EIO otherwise. + * Return: 0 for success, -ERRNO otherwise. */ int cppc_get_highest_perf(int cpunum, u64 *highest_perf) { - return cppc_get_perf(cpunum, HIGHEST_PERF, highest_perf); + return cppc_get_reg_val(cpunum, HIGHEST_PERF, highest_perf); } EXPORT_SYMBOL_GPL(cppc_get_highest_perf); @@ -1261,11 +1318,11 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf); * @cpunum: CPU from which to get epp preference value. * @epp_perf: Return address. * - * Return: 0 for success, -EIO otherwise. + * Return: 0 for success, -ERRNO otherwise. */ int cppc_get_epp_perf(int cpunum, u64 *epp_perf) { - return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf); + return cppc_get_reg_val(cpunum, ENERGY_PERF, epp_perf); } EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read cppc registers, with four 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. 3. Extract the operations if register is in pcc out as cppc_get_reg_val_in_pcc(). 4. Return the result of cpc_read() instead of 0. Add cppc_set_reg_val_in_pcc() and cppc_set_reg_val() as generic functions for setting cppc registers value. Unlike other set reg ABIs, cppc_set_reg_val() 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. These functions can be used to reduce some existing code duplication. Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> --- drivers/acpi/cppc_acpi.c | 111 +++++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 27 deletions(-)