diff mbox series

[v5,3/8] ACPI: CPPC: Rename cppc_get_perf() to cppc_get_reg_val()

Message ID 20250206131428.3261578-4-zhenglifeng1@huawei.com
State New
Headers show
Series Support for autonomous selection in cppc_cpufreq | expand

Commit Message

zhenglifeng (A) Feb. 6, 2025, 1:14 p.m. UTC
Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
cppc registers. And extract the operations if register is in pcc out as
cppc_get_reg_val_in_pcc(). Without functional change.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

Comments

Rafael J. Wysocki March 12, 2025, 7:54 p.m. UTC | #1
On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
> cppc registers. And extract the operations if register is in pcc out as
> cppc_get_reg_val_in_pcc(). Without functional change.

This should be split into two patches IMV.

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index db22f8f107db..3c9c4ce2a0b0 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1189,48 +1189,52 @@ 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);
> -       struct cpc_register_resource *reg;
> +       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> +       struct cppc_pcc_data *pcc_ss_data = NULL;
> +       int ret;
>
> -       if (!cpc_desc) {
> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> +       if (pcc_ss_id < 0) {
> +               pr_debug("Invalid pcc_ss_id\n");
>                 return -ENODEV;
>         }
>
> -       reg = &cpc_desc->cpc_regs[reg_idx];
> +       pcc_ss_data = pcc_data[pcc_ss_id];
>
> -       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;
> -       }

I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro.  I'm not
convinced at all that it adds any value above (and in the next patch
for that matter) and the message printing the register index is just
plain unuseful to anyone who doesn't know how to decode it.

If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS
regardless of what IS_OPTIONAL_CPC_REG() has to say about it.

> +       down_write(&pcc_ss_data->pcc_lock);
>
> -       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;
> +       if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
> +               ret = cpc_read(cpu, reg, val);
> +       else
> +               ret = -EIO;
>
> -               if (pcc_ss_id < 0) {
> -                       pr_debug("Invalid pcc_ss_id\n");
> -                       return -ENODEV;
> -               }
> +       up_write(&pcc_ss_data->pcc_lock);
>
> -               pcc_ss_data = pcc_data[pcc_ss_id];
> +       return ret;
> +}
>
> -               down_write(&pcc_ss_data->pcc_lock);
> +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 (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
> -                       ret = cpc_read(cpunum, reg, perf);
> -               else
> -                       ret = -EIO;
> +       if (!cpc_desc) {
> +               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> +               return -ENODEV;
> +       }
>
> -               up_write(&pcc_ss_data->pcc_lock);
> +       reg = &cpc_desc->cpc_regs[reg_idx];
>
> -               return ret;
> +       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;
>         }
>
> -       return cpc_read(cpunum, reg, perf);
> +       if (CPC_IN_PCC(reg))
> +               return cppc_get_reg_val_in_pcc(cpu, reg, val);
> +
> +       return cpc_read(cpu, reg, val);
>  }
>
>  /**
> @@ -1242,7 +1246,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);
>
> @@ -1255,7 +1259,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);
>  }
>
>  /**
> @@ -1267,7 +1271,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);
>
> @@ -1280,7 +1284,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
>
>
zhenglifeng (A) March 14, 2025, 9:24 a.m. UTC | #2
On 2025/3/13 3:54, Rafael J. Wysocki wrote:

> On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
>> cppc registers. And extract the operations if register is in pcc out as
>> cppc_get_reg_val_in_pcc(). Without functional change.
> 
> This should be split into two patches IMV.

Yes. That makes sense. Thanks.

> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
>>  1 file changed, 35 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index db22f8f107db..3c9c4ce2a0b0 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1189,48 +1189,52 @@ 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);
>> -       struct cpc_register_resource *reg;
>> +       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>> +       struct cppc_pcc_data *pcc_ss_data = NULL;
>> +       int ret;
>>
>> -       if (!cpc_desc) {
>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>> +       if (pcc_ss_id < 0) {
>> +               pr_debug("Invalid pcc_ss_id\n");
>>                 return -ENODEV;
>>         }
>>
>> -       reg = &cpc_desc->cpc_regs[reg_idx];
>> +       pcc_ss_data = pcc_data[pcc_ss_id];
>>
>> -       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;
>> -       }
> 
> I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro.  I'm not
> convinced at all that it adds any value above (and in the next patch
> for that matter) and the message printing the register index is just
> plain unuseful to anyone who doesn't know how to decode it.

With this index, it is easier to locate problems. This is what a "pr_debug"
for, isn't it?

> 
> If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS
> regardless of what IS_OPTIONAL_CPC_REG() has to say about it.

The name "CPC_SUPPORTED" may be a little confused. Actually, in ACPI 6.5,
only optional _CPC package fields that are not supported by the platform
should be encoded as 0 intergers or NULL registers. A mandatory field as a
0 interger is valid. So If I wanted to make this function as a generic one
to read cppc registers, it would have been more reasonable to do this
IS_OPTIONAL_CPC_REG() check before CPC_SUPPORTED().

> 
>> +       down_write(&pcc_ss_data->pcc_lock);
>>
>> -       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;
>> +       if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>> +               ret = cpc_read(cpu, reg, val);
>> +       else
>> +               ret = -EIO;
>>
>> -               if (pcc_ss_id < 0) {
>> -                       pr_debug("Invalid pcc_ss_id\n");
>> -                       return -ENODEV;
>> -               }
>> +       up_write(&pcc_ss_data->pcc_lock);
>>
>> -               pcc_ss_data = pcc_data[pcc_ss_id];
>> +       return ret;
>> +}
>>
>> -               down_write(&pcc_ss_data->pcc_lock);
>> +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 (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
>> -                       ret = cpc_read(cpunum, reg, perf);
>> -               else
>> -                       ret = -EIO;
>> +       if (!cpc_desc) {
>> +               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> +               return -ENODEV;
>> +       }
>>
>> -               up_write(&pcc_ss_data->pcc_lock);
>> +       reg = &cpc_desc->cpc_regs[reg_idx];
>>
>> -               return ret;
>> +       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;
>>         }
>>
>> -       return cpc_read(cpunum, reg, perf);
>> +       if (CPC_IN_PCC(reg))
>> +               return cppc_get_reg_val_in_pcc(cpu, reg, val);
>> +
>> +       return cpc_read(cpu, reg, val);
>>  }
>>
>>  /**
>> @@ -1242,7 +1246,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);
>>
>> @@ -1255,7 +1259,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);
>>  }
>>
>>  /**
>> @@ -1267,7 +1271,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);
>>
>> @@ -1280,7 +1284,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
>>
>>
>
Rafael J. Wysocki March 14, 2025, 10:32 a.m. UTC | #3
On Fri, Mar 14, 2025 at 10:25 AM zhenglifeng (A)
<zhenglifeng1@huawei.com> wrote:
>
> On 2025/3/13 3:54, Rafael J. Wysocki wrote:
>
> > On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
> >>
> >> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
> >> cppc registers. And extract the operations if register is in pcc out as
> >> cppc_get_reg_val_in_pcc(). Without functional change.
> >
> > This should be split into two patches IMV.
>
> Yes. That makes sense. Thanks.
>
> >
> >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> >> ---
> >>  drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
> >>  1 file changed, 35 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> >> index db22f8f107db..3c9c4ce2a0b0 100644
> >> --- a/drivers/acpi/cppc_acpi.c
> >> +++ b/drivers/acpi/cppc_acpi.c
> >> @@ -1189,48 +1189,52 @@ 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);
> >> -       struct cpc_register_resource *reg;
> >> +       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> >> +       struct cppc_pcc_data *pcc_ss_data = NULL;
> >> +       int ret;
> >>
> >> -       if (!cpc_desc) {
> >> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> >> +       if (pcc_ss_id < 0) {
> >> +               pr_debug("Invalid pcc_ss_id\n");
> >>                 return -ENODEV;
> >>         }
> >>
> >> -       reg = &cpc_desc->cpc_regs[reg_idx];
> >> +       pcc_ss_data = pcc_data[pcc_ss_id];
> >>
> >> -       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;
> >> -       }
> >
> > I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro.  I'm not
> > convinced at all that it adds any value above (and in the next patch
> > for that matter) and the message printing the register index is just
> > plain unuseful to anyone who doesn't know how to decode it.
>
> With this index, it is easier to locate problems. This is what a "pr_debug"
> for, isn't it?

For those who know how to decode it, yes.  For others, not really.

> >
> > If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS
> > regardless of what IS_OPTIONAL_CPC_REG() has to say about it.
>
> The name "CPC_SUPPORTED" may be a little confused. Actually, in ACPI 6.5,
> only optional _CPC package fields that are not supported by the platform
> should be encoded as 0 intergers or NULL registers. A mandatory field as a
> 0 interger is valid. So If I wanted to make this function as a generic one
> to read cppc registers, it would have been more reasonable to do this
> IS_OPTIONAL_CPC_REG() check before CPC_SUPPORTED().

I see, so you need to explain this in the changelog.

And IMV the code logic should be:

(1) If this is a NULL register, don't use it.
(2) If it is integer 0, check if it is optional.
    (a) If it is optional, don't use it.
    (b) Otherwise, use 0 as the value.

Of course, there is a problem for platforms that may want to pass 0 as
an optional field value, but this is a spec issue.
zhenglifeng (A) March 21, 2025, 1:45 a.m. UTC | #4
On 2025/3/14 18:32, Rafael J. Wysocki wrote:

> On Fri, Mar 14, 2025 at 10:25 AM zhenglifeng (A)
> <zhenglifeng1@huawei.com> wrote:
>>
>> On 2025/3/13 3:54, Rafael J. Wysocki wrote:
>>
>>> On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>>>
>>>> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read
>>>> cppc registers. And extract the operations if register is in pcc out as
>>>> cppc_get_reg_val_in_pcc(). Without functional change.
>>>
>>> This should be split into two patches IMV.
>>
>> Yes. That makes sense. Thanks.
>>
>>>
>>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>>>> ---
>>>>  drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++-------------------
>>>>  1 file changed, 35 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>>> index db22f8f107db..3c9c4ce2a0b0 100644
>>>> --- a/drivers/acpi/cppc_acpi.c
>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>> @@ -1189,48 +1189,52 @@ 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);
>>>> -       struct cpc_register_resource *reg;
>>>> +       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>>> +       struct cppc_pcc_data *pcc_ss_data = NULL;
>>>> +       int ret;
>>>>
>>>> -       if (!cpc_desc) {
>>>> -               pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
>>>> +       if (pcc_ss_id < 0) {
>>>> +               pr_debug("Invalid pcc_ss_id\n");
>>>>                 return -ENODEV;
>>>>         }
>>>>
>>>> -       reg = &cpc_desc->cpc_regs[reg_idx];
>>>> +       pcc_ss_data = pcc_data[pcc_ss_id];
>>>>
>>>> -       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;
>>>> -       }
>>>
>>> I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro.  I'm not
>>> convinced at all that it adds any value above (and in the next patch
>>> for that matter) and the message printing the register index is just
>>> plain unuseful to anyone who doesn't know how to decode it.
>>
>> With this index, it is easier to locate problems. This is what a "pr_debug"
>> for, isn't it?
> 
> For those who know how to decode it, yes.  For others, not really.

At least it means something. But if you think this index is confusing for
those who don't know how to decode it, I'll remove it in the next version.

> 
>>>
>>> If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS
>>> regardless of what IS_OPTIONAL_CPC_REG() has to say about it.
>>
>> The name "CPC_SUPPORTED" may be a little confused. Actually, in ACPI 6.5,
>> only optional _CPC package fields that are not supported by the platform
>> should be encoded as 0 intergers or NULL registers. A mandatory field as a
>> 0 interger is valid. So If I wanted to make this function as a generic one
>> to read cppc registers, it would have been more reasonable to do this
>> IS_OPTIONAL_CPC_REG() check before CPC_SUPPORTED().
> 
> I see, so you need to explain this in the changelog.

OK.

> 
> And IMV the code logic should be:
> 
> (1) If this is a NULL register, don't use it.
> (2) If it is integer 0, check if it is optional.
>     (a) If it is optional, don't use it.
>     (b) Otherwise, use 0 as the value.
> 
> Of course, there is a problem for platforms that may want to pass 0 as
> an optional field value, but this is a spec issue.

I'll change the logic in next version. Thanks!
diff mbox series

Patch

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index db22f8f107db..3c9c4ce2a0b0 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1189,48 +1189,52 @@  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);
-	struct cpc_register_resource *reg;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret;
 
-	if (!cpc_desc) {
-		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
+	if (pcc_ss_id < 0) {
+		pr_debug("Invalid pcc_ss_id\n");
 		return -ENODEV;
 	}
 
-	reg = &cpc_desc->cpc_regs[reg_idx];
+	pcc_ss_data = pcc_data[pcc_ss_id];
 
-	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;
-	}
+	down_write(&pcc_ss_data->pcc_lock);
 
-	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;
+	if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
+		ret = cpc_read(cpu, reg, val);
+	else
+		ret = -EIO;
 
-		if (pcc_ss_id < 0) {
-			pr_debug("Invalid pcc_ss_id\n");
-			return -ENODEV;
-		}
+	up_write(&pcc_ss_data->pcc_lock);
 
-		pcc_ss_data = pcc_data[pcc_ss_id];
+	return ret;
+}
 
-		down_write(&pcc_ss_data->pcc_lock);
+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 (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
-			ret = cpc_read(cpunum, reg, perf);
-		else
-			ret = -EIO;
+	if (!cpc_desc) {
+		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
+		return -ENODEV;
+	}
 
-		up_write(&pcc_ss_data->pcc_lock);
+	reg = &cpc_desc->cpc_regs[reg_idx];
 
-		return ret;
+	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;
 	}
 
-	return cpc_read(cpunum, reg, perf);
+	if (CPC_IN_PCC(reg))
+		return cppc_get_reg_val_in_pcc(cpu, reg, val);
+
+	return cpc_read(cpu, reg, val);
 }
 
 /**
@@ -1242,7 +1246,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);
 
@@ -1255,7 +1259,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);
 }
 
 /**
@@ -1267,7 +1271,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);
 
@@ -1280,7 +1284,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);