| Message ID | 20250113122104.3870673-1-zhenglifeng1@huawei.com |
|---|---|
| Headers | show |
| Series | Support for autonomous selection in cppc_cpufreq | expand |
The word "function" at the end of the subject is redundant IMV. On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> 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 mandatory or 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() as a generic function for setting cppc registers > value, with this features: > > 1. Check register type. If a register is writeable, it must be a buffer. > > 2. Check if the register is a optional and null one right after getting the > register. Because if so, the rest of the operations are meaningless. > > 3. Extract the operations if register is in pcc out as > cppc_set_reg_val_in_pcc(). > > These functions can be used to reduce some existing code duplication. This mixes functional changes with function renames and code refactoring while it is better to do all of these things separately. Why don't you split the patch into a few smaller patches doing each one thing at a time? Like rename the existing function and refactor it in one patch (if this makes sense), make functional changes to it in another patch, then add new functions in a third one? This would help to review the changes and explain why each of them is made. > Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> > --- > drivers/acpi/cppc_acpi.c | 105 ++++++++++++++++++++++++++++++--------- > 1 file changed, 82 insertions(+), 23 deletions(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 6454b469338f..571f94855dce 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -1181,43 +1181,102 @@ 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 (IS_OPTIONAL_CPC_REG(reg_idx) && !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 a register is writeable, it must be a buffer */ > + if ((reg->type != ACPI_TYPE_BUFFER) || > + (IS_OPTIONAL_CPC_REG(reg_idx) && IS_NULL_REG(®->cpc_entry.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); > } > > /** > @@ -1229,7 +1288,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf) > */ > 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); > > @@ -1242,7 +1301,7 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf); > */ > 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); > } > > /** > @@ -1254,7 +1313,7 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf) > */ > 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); > > @@ -1267,7 +1326,7 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf); > */ > 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); > > -- > 2.33.0 > >
On 2025/1/15 1:41, Rafael J. Wysocki wrote: > The word "function" at the end of the subject is redundant IMV. Yes, you are right. Will delete it. Thanks. > > On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> 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 mandatory or 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() as a generic function for setting cppc registers >> value, with this features: >> >> 1. Check register type. If a register is writeable, it must be a buffer. >> >> 2. Check if the register is a optional and null one right after getting the >> register. Because if so, the rest of the operations are meaningless. >> >> 3. Extract the operations if register is in pcc out as >> cppc_set_reg_val_in_pcc(). >> >> These functions can be used to reduce some existing code duplication. > > This mixes functional changes with function renames and code > refactoring while it is better to do all of these things separately. > > Why don't you split the patch into a few smaller patches doing each > one thing at a time? Like rename the existing function and refactor > it in one patch (if this makes sense), make functional changes to it > in another patch, then add new functions in a third one? > > This would help to review the changes and explain why each of them is made. It does make more sense. Will split it. Thanks. > >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> >> --- >> drivers/acpi/cppc_acpi.c | 105 ++++++++++++++++++++++++++++++--------- >> 1 file changed, 82 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c >> index 6454b469338f..571f94855dce 100644 >> --- a/drivers/acpi/cppc_acpi.c >> +++ b/drivers/acpi/cppc_acpi.c >> @@ -1181,43 +1181,102 @@ 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 (IS_OPTIONAL_CPC_REG(reg_idx) && !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 a register is writeable, it must be a buffer */ >> + if ((reg->type != ACPI_TYPE_BUFFER) || >> + (IS_OPTIONAL_CPC_REG(reg_idx) && IS_NULL_REG(®->cpc_entry.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); >> } >> >> /** >> @@ -1229,7 +1288,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf) >> */ >> 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); >> >> @@ -1242,7 +1301,7 @@ EXPORT_SYMBOL_GPL(cppc_get_desired_perf); >> */ >> 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); >> } >> >> /** >> @@ -1254,7 +1313,7 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf) >> */ >> 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); >> >> @@ -1267,7 +1326,7 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf); >> */ >> 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); >> >> -- >> 2.33.0 >> >> >
On 2025/1/15 2:24, Rafael J. Wysocki wrote: > On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote: > > This should mention the specification revision and section number(s) > for the specification material the code is based on. Will mention it. Thanks. > >> cppc_set_epp - write energy performance preference register value >> >> cppc_get_auto_act_window - read autonomous activity window register value >> >> cppc_set_auto_act_window - write autonomous activity window register value >> >> cppc_get_auto_sel - read autonomous selection enable register value, >> modified from cppc_get_auto_sel_caps() > > It would be better to move the modification part into a separate patch. Yes, good point. Thanks. > >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> >> --- >> drivers/acpi/cppc_acpi.c | 82 ++++++++++++++++++++++++++++++++---- >> drivers/cpufreq/amd-pstate.c | 3 +- >> include/acpi/cppc_acpi.h | 30 +++++++++++-- >> 3 files changed, 103 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c >> index 03134613311d..7bfe30f7b40f 100644 >> --- a/drivers/acpi/cppc_acpi.c >> +++ b/drivers/acpi/cppc_acpi.c >> @@ -1568,23 +1568,89 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable) >> EXPORT_SYMBOL_GPL(cppc_set_epp_perf); >> >> /** >> - * cppc_get_auto_sel_caps - Read autonomous selection register. >> - * @cpunum : CPU from which to read register. >> - * @perf_caps : struct where autonomous selection register value is updated. >> + * cppc_set_epp() - Write the EPP register. >> + * @cpu: CPU on which to write register. >> + * @epp_val: Value to write to the EPP register. >> */ >> -int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps) >> +int cppc_set_epp(int cpu, u64 epp_val) >> { >> - u64 auto_sel; >> + if (epp_val > CPPC_ENERGY_PERF_MAX) >> + return -EINVAL; >> + >> + return cppc_set_reg_val(cpu, ENERGY_PERF, epp_val); >> +} >> +EXPORT_SYMBOL_GPL(cppc_set_epp); >> + >> +/** >> + * cppc_get_auto_act_window() - Read autonomous activity window register. >> + * @cpu: CPU from which to read register. >> + * @auto_act_window: Return address. > > It would be good to describe the autonomous activity window encoding. Will add. Thanks. > >> + */ >> +int cppc_get_auto_act_window(int cpu, u64 *auto_act_window) >> +{ >> + unsigned int exp; >> + u64 val, sig; >> + int ret; >> + >> + ret = cppc_get_reg_val(cpu, AUTO_ACT_WINDOW, &val); >> + if (ret) >> + return ret; >> + >> + sig = val & CPPC_AUTO_ACT_WINDOW_MAX_SIG; >> + exp = (val >> CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) & CPPC_AUTO_ACT_WINDOW_MAX_EXP; >> + *auto_act_window = sig * int_pow(10, exp); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(cppc_get_auto_act_window); >> + >> +/** >> + * cppc_set_auto_act_window() - Write autonomous activity window register. >> + * @cpu: CPU on which to write register. >> + * @auto_act_window: usec value to write to the autonomous activity window register. >> + */ >> +int cppc_set_auto_act_window(int cpu, u64 auto_act_window) >> +{ >> + u64 max_val = CPPC_AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, CPPC_AUTO_ACT_WINDOW_MAX_EXP); >> + int digits = 0; >> + u64 val; >> + >> + if (auto_act_window > max_val) >> + return -EINVAL; >> + >> + while (auto_act_window > CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH) { >> + auto_act_window /= 10; >> + digits += 1; >> + } >> + >> + if (auto_act_window > CPPC_AUTO_ACT_WINDOW_MAX_SIG) >> + auto_act_window = CPPC_AUTO_ACT_WINDOW_MAX_SIG; > > It looks like this may clobber the most significant bit, or am I mistaken? Actually, after the while loop above, auto_act_window is not larger than CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH, which is 129. So this if condition is valid only when auto_act_window is 128 or 129. Since we only have 7 bits to store this value, 128 and 129 can only be cut to 127. > >> + >> + val = (digits << CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) + auto_act_window; >> + >> + return cppc_set_reg_val(cpu, AUTO_ACT_WINDOW, val); >> +} >> +EXPORT_SYMBOL_GPL(cppc_set_auto_act_window); >> + >> +/** >> + * cppc_get_auto_sel() - Read autonomous selection register. >> + * @cpu: CPU from which to read register. >> + * @enable: Return address. >> + */ >> +int cppc_get_auto_sel(int cpu, bool *enable) >> +{ >> + u64 val; >> int ret; >> >> - ret = cppc_get_reg_val(cpunum, AUTO_SEL_ENABLE, &auto_sel); >> + ret = cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, &val); >> if (ret) >> return ret; >> >> - perf_caps->auto_sel = (bool)auto_sel; >> + *enable = (bool)val; >> + >> return 0; >> } >> -EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps); >> +EXPORT_SYMBOL_GPL(cppc_get_auto_sel); >> >> /** >> * cppc_set_auto_sel - Write autonomous selection register. >> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c >> index 66e5dfc711c0..8bc11d0618f8 100644 >> --- a/drivers/cpufreq/amd-pstate.c >> +++ b/drivers/cpufreq/amd-pstate.c >> @@ -399,6 +399,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata) >> { >> struct cppc_perf_caps cppc_perf; >> u64 numerator; >> + bool auto_sel; >> >> int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf); >> if (ret) >> @@ -420,7 +421,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata) >> if (cppc_state == AMD_PSTATE_ACTIVE) >> return 0; >> >> - ret = cppc_get_auto_sel_caps(cpudata->cpu, &cppc_perf); >> + ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel); >> if (ret) { >> pr_warn("failed to get auto_sel, ret: %d\n", ret); >> return 0; >> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h >> index 62d368bcd9ec..325e9543e08f 100644 >> --- a/include/acpi/cppc_acpi.h >> +++ b/include/acpi/cppc_acpi.h >> @@ -32,6 +32,15 @@ >> #define CMD_READ 0 >> #define CMD_WRITE 1 >> >> +#define CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE (7) >> +#define CPPC_AUTO_ACT_WINDOW_EXP_BIT_SIZE (3) >> +#define CPPC_AUTO_ACT_WINDOW_MAX_SIG ((1 << CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) - 1) >> +#define CPPC_AUTO_ACT_WINDOW_MAX_EXP ((1 << CPPC_AUTO_ACT_WINDOW_EXP_BIT_SIZE) - 1) >> +/* CPPC_AUTO_ACT_WINDOW_MAX_SIG is 127, so 128 and 129 will decay to 127 when writing */ >> +#define CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH 129 >> + >> +#define CPPC_ENERGY_PERF_MAX (0xFF) >> + >> /* Each register has the folowing format. */ >> struct cpc_reg { >> u8 descriptor; >> @@ -159,7 +168,10 @@ extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val); >> extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val); >> extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf); >> extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable); >> -extern int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps); >> +extern int cppc_set_epp(int cpu, u64 epp_val); >> +extern int cppc_get_auto_act_window(int cpu, u64 *auto_act_window); >> +extern int cppc_set_auto_act_window(int cpu, u64 auto_act_window); >> +extern int cppc_get_auto_sel(int cpu, bool *enable); >> extern int cppc_set_auto_sel(int cpu, bool enable); >> extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf); >> extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator); >> @@ -229,11 +241,23 @@ static inline int cppc_get_epp_perf(int cpunum, u64 *epp_perf) >> { >> return -EOPNOTSUPP; >> } >> -static inline int cppc_set_auto_sel(int cpu, bool enable) >> +static inline int cppc_set_epp(int cpu, u64 epp_val) >> { >> return -EOPNOTSUPP; >> } >> -static inline int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps) >> +static inline int cppc_get_auto_act_window(int cpu, u64 *auto_act_window) >> +{ >> + return -EOPNOTSUPP; >> +} >> +static inline int cppc_set_auto_act_window(int cpu, u64 auto_act_window) >> +{ >> + return -EOPNOTSUPP; >> +} >> +static inline int cppc_get_auto_sel(int cpu, bool *enable) >> +{ >> + return -EOPNOTSUPP; >> +} >> +static inline int cppc_set_auto_sel(int cpu, bool enable) >> { >> return -EOPNOTSUPP; >> } >> -- >> 2.33.0 >> >> >
Add sysfs interfaces for CPPC autonomous selection in the cppc_cpufreq driver. The patch series is organized in two parts: - patch 1-4 refactor out the general CPPC register get and set functions in cppc_acpi.c - patches 5-6 expose sysfs files for users to control CPPC autonomous selection when supported --- Since Pierre and me have discussed about whether or not to show auto_act_window and energy_perf when auto_select is disabled. It seems like whether to show these two files has their own points. We'd like to ask for some advice. Relevant discussion: [1] https://lore.kernel.org/all/522721da-1a5c-439c-96a8-d0300dd0f906@huawei.com/ Changelog: v4: - add REG_OPTIONAL and IS_OPTIONAL_CPC_REG to judge if a cpc register is an optional one - check whether the register is optional before CPC_SUPPORTED check in cppc_get_reg_val() and cppc_set_reg_val() - check the register's type in cppc_set_reg_val() - add macros to generally implement registers getting and setting functions - move some logic codes from cppc_cpufreq.c to cppc_acpi.c - replace cppc_get_auto_sel_caps() by cppc_get_auto_sel() v3: - change cppc_get_reg() and cppc_set_reg() name to cppc_get_reg_val() and cppc_set_reg_val() - extract cppc_get_reg_val_in_pcc() and cppc_set_reg_val_in_pcc() - return the result of cpc_read() in cppc_get_reg_val() - add pr_debug() in cppc_get_reg_val_in_pcc() when pcc_ss_id < 0 - rename 'cpunum' to 'cpu' in cppc_get_reg_val() - move some macros from drivers/cpufreq/cppc_cpufreq.c to include/acpi/cppc_acpi.h with a CPPC_XXX prefix v2: - fix some incorrect placeholder - change kstrtoul to kstrtobool in store_auto_select Lifeng Zheng (6): ACPI: CPPC: Add IS_OPTIONAL_CPC_REG macro ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function ACPI: CPPC: Add macros to generally implement registers getting and setting functions ACPI: CPPC: Refactor register value 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 | 332 +++++++++--------- drivers/cpufreq/amd-pstate.c | 3 +- drivers/cpufreq/cppc_cpufreq.c | 109 ++++++ include/acpi/cppc_acpi.h | 30 +- 5 files changed, 359 insertions(+), 169 deletions(-)