Re: [PATCH 1/3] ACPI: CPPC: Refactor register get and set ABIs

["zhenglifeng (A)" <zhenglifeng1@huawei.com>]

Hello Pierre，

On 2024/12/6 22:23, Pierre Gondois wrote:
> Hello Lifeng,
> 
> On 11/14/24 09:48, Lifeng Zheng wrote:
>> Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().
>>
>> 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.
>>
>> Add cppc_set_reg() as a generic function for setting cppc registers. 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.
>>
>> 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..306ced9c3376 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;
>>       struct cpc_register_resource *reg;
>> +    int pcc_ss_id;
>> +    int ret = 0;
> 
> NIT: Might not be necessary if we save the value returned by cpc_read(),
> cf. other comment below.
> 
>>         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=%u) 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;
> 
> NIT: Could add here:
>   pr_debug("Invalid pcc_ss_id\n");
> just as you did in cppc_set_reg()

Will add it in next version, Thanks.

> 
>>             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);
> 
> This was not introduced by your patch, but cpc_read() return a value.
> Shouldn't we return it instead of 0 ?

Indeed. Will optimize it, Thanks.

> 
>>           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);
> 
> Same comment as above
> 
>>         return 0;
>>   }
>>   +static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)
> 
> Just to have similar functions, maybe 'cpu' should be renamed to 'cpunum' ?
> Or the other way around.

I prefer 'cpu', 'cpunum' looks like the number of cpus to me.
Will rename 'cpunum' to 'cpu' in cppc_get_reg(). Thanks.

> 
>> +{
>> +    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=%u) is not supported\n", reg_idx);
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    if (CPC_IN_PCC(reg)) {
>> +        pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> +
>> +        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);
>> +}
>> +
> 
> [snip]
>