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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
>>
>>
>
