Re: [PATCH v3 1/4] ACPI: CPPC: Add cppc_get_reg_val and cppc_set_reg_val function

[Pierre Gondois <pierre.gondois@arm.com>]

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!
>