Re: [PATCH v4 2/8] ACPI: CPPC: Add cppc_get_perf() API to read performance controls

[Sumit Gupta <sumitg@nvidia.com>]

Hi Pierre,


On 27/11/25 20:23, Pierre Gondois wrote:
> External email: Use caution opening links or attachments
>
>
> Hello Sumit,
>
> Sorry for the late review.
> I think it would be nice to split the patchset in 2 as handling the
> auto_sel and
> min/max_perf values in the cppc_cpufreq driver might lead to more
> discussion.
> I.e. the ACPI: CPPC: XXX patches should be straightforward to upstream.
> This is just a personal opinion, feel free to ignore it.
>

I think its better to keep the changes in one patchset for better context.
I agree that the 'ACPI: CPPC: XX' patches can be applied first if ACK'ed.
Will update the cover letter in v5 to request for taking those patches
first if no new issues and send v6 with changes on the rest of patches
if any new comments on them.


>
> On 11/5/25 12:38, Sumit Gupta wrote:
>> Add cppc_get_perf() function to read values of performance control
>> registers including desired_perf, min_perf, max_perf, and energy_perf.
>>
>> This provides a read interface to complement the existing 
>> cppc_set_perf()
>> write interface for performance control registers.
>>
>> Signed-off-by: Sumit Gupta<sumitg@nvidia.com>
>> ---
>>   drivers/acpi/cppc_acpi.c | 73 ++++++++++++++++++++++++++++++++++++++++
>>   include/acpi/cppc_acpi.h |  5 +++
>>   2 files changed, 78 insertions(+)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index ab4651205e8a..05672c30187c 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1731,6 +1731,79 @@ int cppc_set_enable(int cpu, bool enable)
>>       return cppc_set_reg_val(cpu, ENABLE, enable);
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_set_enable);
>> +/**
>> + * cppc_get_perf - Get a CPU's performance controls.
>> + * @cpu: CPU for which to get performance controls.
>> + * @perf_ctrls: ptr to cppc_perf_ctrls. See cppc_acpi.h
>> + *
>> + * Return: 0 for success with perf_ctrls, -ERRNO otherwise.
>> + */
>> +int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>> +{
>> +     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> +     struct cpc_register_resource *desired_perf_reg, *min_perf_reg, 
>> *max_perf_reg,
>> +                                  *energy_perf_reg;
>> +     u64 desired_perf = 0, min = 0, max = 0, energy_perf = 0;
>> +     int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> +     struct cppc_pcc_data *pcc_ss_data = NULL;
>> +     int ret = 0, regs_in_pcc = 0;
>> +
>> +     if (!cpc_desc) {
>> +             pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> +             return -ENODEV;
>> +     }
>> +
>> +     if (!perf_ctrls) {
>> +             pr_debug("Invalid perf_ctrls pointer\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     desired_perf_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
>> +     min_perf_reg = &cpc_desc->cpc_regs[MIN_PERF];
>> +     max_perf_reg = &cpc_desc->cpc_regs[MAX_PERF];
>> +     energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
>> +
>> +     /* Are any of the regs PCC ?*/
>> +     if (CPC_IN_PCC(desired_perf_reg) || CPC_IN_PCC(min_perf_reg) ||
>> +         CPC_IN_PCC(max_perf_reg) || CPC_IN_PCC(energy_perf_reg)) {
>> +             if (pcc_ss_id < 0) {
>> +                     pr_debug("Invalid pcc_ss_id forCPU:%d\n", cpu);
>> +                     return -ENODEV;
>> +             }
>> +             pcc_ss_data = pcc_data[pcc_ss_id];
>> +             regs_in_pcc = 1;
>> +             down_write(&pcc_ss_data->pcc_lock);
>> +             /* Ring doorbell once to update PCC subspace */
>> +             if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
>> +                     pr_debug("Failed to send PCC command for 
>> CPU:%d, ret:%d\n", cpu, ret);
>> +                     ret = -EIO;
>> +                     goto out_err;
>> +             }
>> +     }
>> +
>> +     /* Read optional elements if present */
>> +     if (CPC_SUPPORTED(max_perf_reg))
>> +             cpc_read(cpu, max_perf_reg, &max);
>> +     perf_ctrls->max_perf = max;
>> +
>> +     if (CPC_SUPPORTED(min_perf_reg))
>> +             cpc_read(cpu, min_perf_reg, &min);
>> +     perf_ctrls->min_perf = min;
>> +
>
> NIT: I think the 'desired_perf_reg' register is mandatory, so the check
> could be removed.
>
>

The register is optional when Autonomous mode is enabled.
As per CPPC spec:
"This register is optional when OSPM indicates support for CPPC2 in the
platform-wide _OSC capabilities and the Autonomous Selection Enable
register is Integer 1"

Thank you,
Sumit Gupta


>> +     if (CPC_SUPPORTED(desired_perf_reg))
>> +             cpc_read(cpu, desired_perf_reg, &desired_perf);
>> +     perf_ctrls->desired_perf = desired_perf;
>> +
>> +     if (CPC_SUPPORTED(energy_perf_reg))
>> +             cpc_read(cpu, energy_perf_reg, &energy_perf);
>> +     perf_ctrls->energy_perf = energy_perf;
>> +
>> +out_err:
>> +     if (regs_in_pcc)
>> +             up_write(&pcc_ss_data->pcc_lock);
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(cppc_get_perf);

....