| 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); >
On 2024/12/17 21:48, Pierre Gondois wrote: > 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. Indeed, I should have judged the type before doing this check. But adding a input parameter is not a really nice way to me. How about adding a bool list of length MAX_CPC_REG_ENT in cppc_acpi.h to indicate wheter it is optional? > >> - 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. Actually, cpc_write() doesn't check field type and treats the field as a buffer. That's why you get 'Bad address' error when the integer value is 1. I think the existing code needs to be improved, otherwise there may be unexpected problems. Do you mean we should return -EOPNOTSUPP no matter what to be written if this field is a optional + integer one? And what about a mandatory + integer one. Should we directly write the int_value? Looking forward to your opinion. > >> + >> + 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); >> >
Hello Lifeng, On 12/20/24 09:30, zhenglifeng (A) wrote: > On 2024/12/17 21:48, Pierre Gondois wrote: >> 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. > > Indeed, I should have judged the type before doing this check. But adding a > input parameter is not a really nice way to me. How about adding a bool > list of length MAX_CPC_REG_ENT in cppc_acpi.h to indicate wheter it is > optional? Actually all these functions: - cppc_get_desired_perf - cppc_get_highest_perf - cppc_get_epp_perf - cppc_set_epp - cppc_get_auto_act_window - cppc_set_auto_act_window - cppc_get_auto_sel - cppc_get_nominal_perf and in general all the functions getting / setting one value at a time could be implemented by macros similars to show_cppc_data(). From what I see the input parameters required are: - name of the field - if the field is mandatory to have or not - if the field is writeable - if the field is implemented as an integer, register, or can be both This would avoid having numerous function definitions doing approximately the same thing. > >> >>> - 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. > > Actually, cpc_write() doesn't check field type and treats the field as a > buffer. That's why you get 'Bad address' error when the integer value is 1. > I think the existing code needs to be improved, otherwise there may be > unexpected problems. > > Do you mean we should return -EOPNOTSUPP no matter what to be written if > this field is a optional + integer one? Yes exact And what about a mandatory + > integer one. Should we directly write the int_value? I don't think it is possible to have this. Indeed, if a value is writeable, it must be a register, so mandatory + integer should not exist. I suggested a check in case someone made a mistake, but it is not sure the check is actually necessary. Regards, Pierre
Hello Pierre, On 2025/1/8 0:54, Pierre Gondois wrote: > Hello Lifeng, > > On 12/20/24 09:30, zhenglifeng (A) wrote: >> On 2024/12/17 21:48, Pierre Gondois wrote: >>> 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. >> >> Indeed, I should have judged the type before doing this check. But adding a >> input parameter is not a really nice way to me. How about adding a bool >> list of length MAX_CPC_REG_ENT in cppc_acpi.h to indicate wheter it is >> optional? > > Actually all these functions: > - cppc_get_desired_perf > - cppc_get_highest_perf > - cppc_get_epp_perf > - cppc_set_epp > - cppc_get_auto_act_window > - cppc_set_auto_act_window As you suggest in another patch, the logic should be placed in cppc_get_auto_act_window() and some other functions. I'm afraid these functions couldn't be implemented with the macros you suggest. > - cppc_get_auto_sel > - cppc_get_nominal_perf > > and in general all the functions getting / setting one value at a time could > be implemented by macros similars to show_cppc_data(). From what I see the > input parameters required are: > - name of the field > - if the field is mandatory to have or not If with this parameter, we should put all the cppc_get_reg_val() and cppc_set_reg_val() in the macro. This wouldn't look really nice. I prefer to use a macro to judge mandatory / optional. I'll show you in v4. > - if the field is writeable I think we can define a READ macro, a WRITE macro and a RW macro. For the registers which are not writeable, only use the READ macro to implement getting function. > - if the field is implemented as an integer, register, or can be both I don't think this parameter is necessary. The field type can be got from cpc_desc->cpc_regs[reg_idx].type. > > This would avoid having numerous function definitions doing approximately the > same thing. So from what I see the input parameters required are name of the field and reg_idx. Thanks for your advice! > >> >>> >>>> - 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. >> >> Actually, cpc_write() doesn't check field type and treats the field as a >> buffer. That's why you get 'Bad address' error when the integer value is 1. >> I think the existing code needs to be improved, otherwise there may be >> unexpected problems. >> >> Do you mean we should return -EOPNOTSUPP no matter what to be written if >> this field is a optional + integer one? > > Yes exact > >> And what about a mandatory + >> integer one. Should we directly write the int_value? > > I don't think it is possible to have this. Indeed, if a value is writeable, > it must be a register, so mandatory + integer should not exist. I suggested > a check in case someone made a mistake, but it is not sure the check is actually > necessary. Yeah, I think it's better to have this check, too. Regards, Lifeng > > Regards, > Pierre >
Hello Lifeng, On 1/10/25 03:23, zhenglifeng (A) wrote: > Hello Pierre, > > On 2025/1/8 0:54, Pierre Gondois wrote: >> Hello Lifeng, >> >> On 12/20/24 09:30, zhenglifeng (A) wrote: >>> On 2024/12/17 21:48, Pierre Gondois wrote: >>>> 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. >>> >>> Indeed, I should have judged the type before doing this check. But adding a >>> input parameter is not a really nice way to me. How about adding a bool >>> list of length MAX_CPC_REG_ENT in cppc_acpi.h to indicate wheter it is >>> optional? >> >> Actually all these functions: >> - cppc_get_desired_perf >> - cppc_get_highest_perf >> - cppc_get_epp_perf >> - cppc_set_epp >> - cppc_get_auto_act_window >> - cppc_set_auto_act_window > > As you suggest in another patch, the logic should be placed in > cppc_get_auto_act_window() and some other functions. I'm afraid these > functions couldn't be implemented with the macros you suggest. If you're referring to the [get|set]_auto_act_window() functions, I guess it should be ok to have the getter/setter functions implemented as a macros, and then have a wrapper to do the conversion of the value. > >> - cppc_get_auto_sel >> - cppc_get_nominal_perf >> >> and in general all the functions getting / setting one value at a time could >> be implemented by macros similars to show_cppc_data(). From what I see the >> input parameters required are: >> - name of the field >> - if the field is mandatory to have or not > > If with this parameter, we should put all the cppc_get_reg_val() and > cppc_set_reg_val() in the macro. This wouldn't look really nice. I > prefer to use a macro to judge mandatory / optional. I'll show you in > v4. > If you prefer to have specific macro names to distinguish optional/mandatory fields, it also seems a good solution. >> - if the field is writeable > > I think we can define a READ macro, a WRITE macro and a RW macro. For > the registers which are not writeable, only use the READ macro to > implement getting function. Yes right, same comment as above. > >> - if the field is implemented as an integer, register, or can be both > > I don't think this parameter is necessary. The field type can be got > from cpc_desc->cpc_regs[reg_idx].type. Yes indeed. > >> >> This would avoid having numerous function definitions doing approximately the >> same thing. > > So from what I see the input parameters required are name of the field > and reg_idx. Thanks for your advice! >
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(-)