| Message ID | 20241122062051.3658577-1-zhenglifeng1@huawei.com |
|---|---|
| Headers | show |
| Series | Support for autonomous selection in cppc_cpufreq | expand |
Gentle ping. +cc a few developers On 2024/11/22 14:20, Lifeng Zheng wrote: > Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq > driver. > > The patch series is organized in two parts: > > - patch 1 refactor out the general CPPC register get and set functions > in cppc_acpi.c > > - patches 2-3 expose sysfs files for users to control CPPC autonomous > selection when supported > > Change since v1: > - fix some incorrect placeholder > - change kstrtoul to kstrtobool in store_auto_select > > Lifeng Zheng (3): > ACPI: CPPC: Refactor register get and set ABIs > ACPI: CPPC: Add autonomous selection ABIs > cpufreq: CPPC: Support for autonomous selection in cppc_cpufreq > > .../ABI/testing/sysfs-devices-system-cpu | 54 ++++ > drivers/acpi/cppc_acpi.c | 235 +++++++++--------- > drivers/cpufreq/cppc_cpufreq.c | 138 ++++++++++ > include/acpi/cppc_acpi.h | 20 ++ > 4 files changed, 328 insertions(+), 119 deletions(-) >
On Fri, Nov 22, 2024 at 7:21 AM Lifeng Zheng <zhenglifeng1@huawei.com> wrote: > > Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg(). I don't quite like the cppc_get_reg() name. I think that cppc_get_reg_val() would be better. > 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. So it's not just a rename, but also a change in behavior. Can this change in behavior become user-visible? > Add cppc_set_reg() as a generic function for setting cppc registers. Again, I would prefer cppc_set_reg_val(). > 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. And the new function is used to reduce some existing code duplication, isn't it? Which would be good to mention here. > Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> > --- > 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..9aab22d8136a 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; Why are you moving this here? This change is not related to the rest of the patch, is it? > struct cpc_register_resource *reg; > + int pcc_ss_id; > + int ret = 0; And here? > > 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=%d) 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; > > 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); > 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); > > return 0; > } > > +static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val) > +{ > + 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=%d) is not supported\n", reg_idx); > + return -EOPNOTSUPP; > + } > + > + if (CPC_IN_PCC(reg)) { > + pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); Please declare the variables that are only needed in the PCC case here. Also, I think it would be better to define a new function, say cppc_set_reg_val_in_pcc() for this code and then have if (CPC_IN_PCC(reg)) return cppc_set_reg_val_in_pcc(reg, val); > + > + 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); > +} > + > /** > * cppc_get_desired_perf - Get the desired performance register value. > * @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(cpunum, DESIRED_PERF, desired_perf); > } > EXPORT_SYMBOL_GPL(cppc_get_desired_perf); > > @@ -1236,11 +1286,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. What do you mean by ERRNO? > */ > int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf) > { > - return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf); > + return cppc_get_reg(cpunum, NOMINAL_PERF, nominal_perf); > } > > /** > @@ -1248,11 +1298,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(cpunum, HIGHEST_PERF, highest_perf); > } > EXPORT_SYMBOL_GPL(cppc_get_highest_perf); > > @@ -1261,11 +1311,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. Same here? > */ > int cppc_get_epp_perf(int cpunum, u64 *epp_perf) > { > - return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf); > + return cppc_get_reg(cpunum, ENERGY_PERF, epp_perf); > } > EXPORT_SYMBOL_GPL(cppc_get_epp_perf); It would be cleaner to do the changes below in a separate patch IMV. > @@ -1545,44 +1595,14 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf); > */ > int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps) > { > - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum); > - struct cpc_register_resource *auto_sel_reg; > - u64 auto_sel; > - > - if (!cpc_desc) { > - pr_debug("No CPC descriptor for CPU:%d\n", cpunum); > - return -ENODEV; > - } > - > - auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE]; > - > - if (!CPC_SUPPORTED(auto_sel_reg)) > - pr_warn_once("Autonomous mode is not unsupported!\n"); > - > - if (CPC_IN_PCC(auto_sel_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 -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) { > - cpc_read(cpunum, auto_sel_reg, &auto_sel); > - perf_caps->auto_sel = (bool)auto_sel; > - } else { > - ret = -EIO; > - } > - > - up_write(&pcc_ss_data->pcc_lock); > + u64 auto_sel; > + int ret; > > + ret = cppc_get_reg(cpunum, AUTO_SEL_ENABLE, &auto_sel); > + if (ret) > return ret; > - } > > + perf_caps->auto_sel = (bool)auto_sel; > return 0; > } > EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps); > @@ -1594,43 +1614,7 @@ EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps); > */ > int cppc_set_auto_sel(int cpu, bool enable) > { > - int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); > - struct cpc_register_resource *auto_sel_reg; > - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu); > - struct cppc_pcc_data *pcc_ss_data = NULL; > - int ret = -EINVAL; > - > - if (!cpc_desc) { > - pr_debug("No CPC descriptor for CPU:%d\n", cpu); > - return -ENODEV; > - } > - > - auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE]; > - > - if (CPC_IN_PCC(auto_sel_reg)) { > - if (pcc_ss_id < 0) { > - pr_debug("Invalid pcc_ss_id\n"); > - return -ENODEV; > - } > - > - if (CPC_SUPPORTED(auto_sel_reg)) { > - ret = cpc_write(cpu, auto_sel_reg, enable); > - 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); > - } else { > - ret = -ENOTSUPP; > - pr_debug("_CPC in PCC is not supported\n"); > - } > - > - return ret; > + return cppc_set_reg(cpu, AUTO_SEL_ENABLE, enable); > } > EXPORT_SYMBOL_GPL(cppc_set_auto_sel); > > @@ -1644,38 +1628,7 @@ EXPORT_SYMBOL_GPL(cppc_set_auto_sel); > */ > int cppc_set_enable(int cpu, bool 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 = -EINVAL; > - > - if (!cpc_desc) { > - pr_debug("No CPC descriptor for CPU:%d\n", cpu); > - return -EINVAL; > - } > - > - 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); > - /* after writing CPC, transfer the ownership of PCC to platfrom */ > - ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE); > - up_write(&pcc_ss_data->pcc_lock); > - return ret; > - } > - > - return cpc_write(cpu, enable_reg, enable); > + return cppc_set_reg(cpu, ENABLE, enable); > } > EXPORT_SYMBOL_GPL(cppc_set_enable); > > --
On 2024/12/11 2:14, Rafael J. Wysocki wrote: > On Fri, Nov 22, 2024 at 7:21 AM Lifeng Zheng <zhenglifeng1@huawei.com> wrote: >> >> Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg(). > > I don't quite like the cppc_get_reg() name. I think that > cppc_get_reg_val() would be better. Indeed, it is better. Will change. Thanks. > >> 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. > > So it's not just a rename, but also a change in behavior. Can this > change in behavior become user-visible? The register value get ABIs in this file returned different error numbers when pcc_ss_id < 0, but should be the same one. So I chose a most suitable one I thought to be returned here when doing refactoring. This change is not user-visible as I know. It is necessary to do the CPC_SUPPORTED() check before using the register. If it is not a cpc supported one, the rest of the operation is pointless and may be dangerous. This change might be user-visible but is still necessary. > >> Add cppc_set_reg() as a generic function for setting cppc registers. > > Again, I would prefer cppc_set_reg_val(). > >> 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. > > And the new function is used to reduce some existing code duplication, > isn't it? Which would be good to mention here. Yes, Will mention it in next version. Thanks. > >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> >> --- >> 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..9aab22d8136a 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; > > Why are you moving this here? This change is not related to the rest > of the patch, is it? > >> struct cpc_register_resource *reg; >> + int pcc_ss_id; >> + int ret = 0; > > And here? Moving these because I'm used to declare variables at the beginning of a function. It's really unnecessary. After defining new functions as cppc_get_reg_val_in_pcc() and cppc_set_reg_val_in_pcc() as you suggest below, these variables will be moved to the new functions. > >> >> 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=%d) 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; >> >> 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); >> 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); >> >> return 0; >> } >> >> +static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val) >> +{ >> + 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=%d) is not supported\n", reg_idx); >> + return -EOPNOTSUPP; >> + } >> + >> + if (CPC_IN_PCC(reg)) { >> + pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); > > Please declare the variables that are only needed in the PCC case here. > > Also, I think it would be better to define a new function, say > cppc_set_reg_val_in_pcc() for this code and then have > > if (CPC_IN_PCC(reg)) > return cppc_set_reg_val_in_pcc(reg, val); Will define new functions as cppc_get_reg_val_in_pcc() and cppc_set_reg_val_in_pcc(). Thanks. > >> + >> + 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); >> +} >> + >> /** >> * cppc_get_desired_perf - Get the desired performance register value. >> * @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(cpunum, DESIRED_PERF, desired_perf); >> } >> EXPORT_SYMBOL_GPL(cppc_get_desired_perf); >> >> @@ -1236,11 +1286,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. > > What do you mean by ERRNO? Error number. I see this expression elsewhere in this file so I use it too. > >> */ >> int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf) >> { >> - return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf); >> + return cppc_get_reg(cpunum, NOMINAL_PERF, nominal_perf); >> } >> >> /** >> @@ -1248,11 +1298,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(cpunum, HIGHEST_PERF, highest_perf); >> } >> EXPORT_SYMBOL_GPL(cppc_get_highest_perf); >> >> @@ -1261,11 +1311,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. > > Same here? > >> */ >> int cppc_get_epp_perf(int cpunum, u64 *epp_perf) >> { >> - return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf); >> + return cppc_get_reg(cpunum, ENERGY_PERF, epp_perf); >> } >> EXPORT_SYMBOL_GPL(cppc_get_epp_perf); > > It would be cleaner to do the changes below in a separate patch IMV. Will separate it. Thanks. > >> @@ -1545,44 +1595,14 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf); >> */ >> int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps) >> { >> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum); >> - struct cpc_register_resource *auto_sel_reg; >> - u64 auto_sel; >> - >> - if (!cpc_desc) { >> - pr_debug("No CPC descriptor for CPU:%d\n", cpunum); >> - return -ENODEV; >> - } >> - >> - auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE]; >> - >> - if (!CPC_SUPPORTED(auto_sel_reg)) >> - pr_warn_once("Autonomous mode is not unsupported!\n"); >> - >> - if (CPC_IN_PCC(auto_sel_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 -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) { >> - cpc_read(cpunum, auto_sel_reg, &auto_sel); >> - perf_caps->auto_sel = (bool)auto_sel; >> - } else { >> - ret = -EIO; >> - } >> - >> - up_write(&pcc_ss_data->pcc_lock); >> + u64 auto_sel; >> + int ret; >> >> + ret = cppc_get_reg(cpunum, AUTO_SEL_ENABLE, &auto_sel); >> + if (ret) >> return ret; >> - } >> >> + perf_caps->auto_sel = (bool)auto_sel; >> return 0; >> } >> EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps); >> @@ -1594,43 +1614,7 @@ EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps); >> */ >> int cppc_set_auto_sel(int cpu, bool enable) >> { >> - int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); >> - struct cpc_register_resource *auto_sel_reg; >> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu); >> - struct cppc_pcc_data *pcc_ss_data = NULL; >> - int ret = -EINVAL; >> - >> - if (!cpc_desc) { >> - pr_debug("No CPC descriptor for CPU:%d\n", cpu); >> - return -ENODEV; >> - } >> - >> - auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE]; >> - >> - if (CPC_IN_PCC(auto_sel_reg)) { >> - if (pcc_ss_id < 0) { >> - pr_debug("Invalid pcc_ss_id\n"); >> - return -ENODEV; >> - } >> - >> - if (CPC_SUPPORTED(auto_sel_reg)) { >> - ret = cpc_write(cpu, auto_sel_reg, enable); >> - 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); >> - } else { >> - ret = -ENOTSUPP; >> - pr_debug("_CPC in PCC is not supported\n"); >> - } >> - >> - return ret; >> + return cppc_set_reg(cpu, AUTO_SEL_ENABLE, enable); >> } >> EXPORT_SYMBOL_GPL(cppc_set_auto_sel); >> >> @@ -1644,38 +1628,7 @@ EXPORT_SYMBOL_GPL(cppc_set_auto_sel); >> */ >> int cppc_set_enable(int cpu, bool 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 = -EINVAL; >> - >> - if (!cpc_desc) { >> - pr_debug("No CPC descriptor for CPU:%d\n", cpu); >> - return -EINVAL; >> - } >> - >> - 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); >> - /* after writing CPC, transfer the ownership of PCC to platfrom */ >> - ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE); >> - up_write(&pcc_ss_data->pcc_lock); >> - return ret; >> - } >> - >> - return cpc_write(cpu, enable_reg, enable); >> + return cppc_set_reg(cpu, ENABLE, enable); >> } >> EXPORT_SYMBOL_GPL(cppc_set_enable); >> >> -- >