Re: [PATCH] ACPI: CPPC: Scale EPP values to the register width

[Mario Limonciello <superm1@kernel.org>]

On 11/12/26 08:05, Christian Loehle wrote:
> On 11/12/26 14:01, Rafael J. Wysocki (Intel) wrote:
>> On Thu, Nov 12, 2026 at 2:58 PM Christian Loehle
>> <christian.loehle@arm.com> wrote:
>>>
>>> On 11/12/26 13:13, Christian Loehle wrote:
>>>> The EPP sysfs interface uses 0-255, but ACPI permits 4-8 bit fields.
>>>> Writing 128 to a 4- or 6-bit SystemMemory field currently succeeds but
>>>> stores zero, selecting maximum performance.
>>>>
>>>> Scale writes to supported narrow EPP registers and normalize reads to
>>>> 0-255. Cover both EPP setters and cppc_get_perf() so saved firmware values
>>>> round-trip correctly.
>>>>
>>>> Fixes: 922607a2b462 ("cpufreq: CPPC: Add support for autonomous selection")
>>>> Signed-off-by: Christian Loehle <christian.loehle@arm.com>
>>>> ---
>>>>   .../ABI/testing/sysfs-devices-system-cpu      |  5 ++
>>>>   drivers/acpi/cppc_acpi.c                      | 46 +++++++++++++++++--
>>>>   2 files changed, 47 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
>>>> index d219fc27a0c9..c63b5cfd993c 100644
>>>> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
>>>> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
>>>> @@ -322,6 +322,11 @@ Description:     Energy performance preference
>>>>                performance increase/decrease and the result of the hardware's
>>>>                energy efficiency and performance optimization policies.
>>>>
>>>> +             For supported EPP registers narrower than eight bits, values are
>>>> +             scaled and rounded to the nearest hardware preference.
>>>> +             Reads use the same 0-255 scale and may differ from the requested
>>>> +             value due to the reduced precision.
>>>> +
>>>>                Writing to this file only has meaning when Autonomous Selection is
>>>>                enabled.
>>>>
>>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>>> index 11a9f9eb6b4a..10b52163eb56 100644
>>>> --- a/drivers/acpi/cppc_acpi.c
>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>> @@ -38,6 +38,7 @@
>>>>   #include <linux/iopoll.h>
>>>>   #include <linux/ktime.h>
>>>>   #include <linux/list.h>
>>>> +#include <linux/math64.h>
>>>>   #include <linux/mutex.h>
>>>>   #include <linux/rbtree.h>
>>>>   #include <linux/rwsem.h>
>>>> @@ -2841,6 +2842,25 @@ int cppc_get_highest_perf(int cpunum, u64 *highest_perf)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>>>
>>>> +/* Keep the EPP interface on an 8-bit scale, including firmware readback. */
>>>> +static u64 cppc_scale_epp(const struct cpc_register_resource *reg,
>>>> +                       u64 epp, bool to_reg)
>>>> +{
>>>> +     unsigned int width = reg->cpc_entry.reg.bit_width;
>>>> +     u32 max;
>>>> +
>>>> +     if (reg->type != ACPI_TYPE_BUFFER || width < 4 || width >= 8)
>>>> +             return epp;
>>>> +
>>>> +     max = BIT(width) - 1;
>>>> +     if (to_reg)
>>>> +             return DIV_ROUND_CLOSEST_ULL(epp * max,
>>>> +                                          CPPC_EPP_ENERGY_EFFICIENCY_PREF);
>>>> +
>>>> +     return DIV_ROUND_CLOSEST_ULL(epp * CPPC_EPP_ENERGY_EFFICIENCY_PREF,
>>>> +                                  max);
>>>> +}
>>>> +
>>>>   /**
>>>>    * cppc_get_epp_perf - Get the epp register value.
>>>>    * @cpunum: CPU from which to get epp preference value.
>>>> @@ -2850,7 +2870,15 @@ EXPORT_SYMBOL_GPL(cppc_get_highest_perf);
>>>>    */
>>>>   int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>>>>   {
>>>> -     return cppc_get_reg_val(cpunum, ENERGY_PERF, epp_perf);
>>>> +     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>>>> +     int ret;
>>>> +
>>>> +     ret = cppc_get_reg_val(cpunum, ENERGY_PERF, epp_perf);
>>>> +     if (!ret)
>>>> +             *epp_perf = cppc_scale_epp(&cpc_desc->cpc_regs[ENERGY_PERF],
>>>> +                                        *epp_perf, false);
>>>> +
>>>> +     return ret;
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
>>>>
>>>> @@ -3150,6 +3178,7 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>>>>        bool auto_sel_non_pcc;
>>>>        bool epp_pcc;
>>>>        bool epp_non_pcc;
>>>> +     u64 epp;
>>>>        int ret;
>>>>
>>>>        if (!cpc_desc) {
>>>> @@ -3159,6 +3188,7 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>>>>
>>>>        auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>>>>        epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
>>>> +     epp = cppc_scale_epp(epp_set_reg, perf_ctrls->energy_perf, true);
>>>>        if (!enable && auto_sel_reg->type == ACPI_TYPE_INTEGER &&
>>>>            auto_sel_reg->cpc_entry.int_value == 1)
>>>>                return -EOPNOTSUPP;
>>>> @@ -3177,7 +3207,7 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>>>>                        return ret;
>>>>        }
>>>>        if (epp_non_pcc) {
>>>> -             ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
>>>> +             ret = cpc_write(cpu, epp_set_reg, epp);
>>>>                if (ret)
>>>>                        return ret;
>>>>        }
>>>> @@ -3205,7 +3235,7 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>>>>                }
>>>>
>>>>                if (epp_pcc) {
>>>> -                     ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
>>>> +                     ret = cpc_write(cpu, epp_set_reg, epp);
>>>>                        if (ret)
>>>>                                goto out_unlock;
>>>>                }
>>>> @@ -3235,9 +3265,17 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>>>>    */
>>>>   int cppc_set_epp(int cpu, u64 epp_val)
>>>>   {
>>>> +     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>>> +
>>>>        if (epp_val > CPPC_EPP_ENERGY_EFFICIENCY_PREF)
>>>>                return -EINVAL;
>>>>
>>>> +     if (!cpc_desc)
>>>> +             return -ENODEV;
>>>> +
>>>> +     epp_val = cppc_scale_epp(&cpc_desc->cpc_regs[ENERGY_PERF],
>>>> +                              epp_val, true);
>>>> +
>>>>        return cppc_set_reg_val(cpu, ENERGY_PERF, epp_val);
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(cppc_set_epp);
>>>> @@ -3496,7 +3534,7 @@ int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>>>>                if (ret)
>>>>                        goto out_err;
>>>>        }
>>>> -     perf_ctrls->energy_perf = energy_perf;
>>>> +     perf_ctrls->energy_perf = cppc_scale_epp(energy_perf_reg, energy_perf, false);
>>>>
>>>>        if (CPC_SUPPORTED(auto_sel_reg)) {
>>>>                ret = cpc_read(cpu, auto_sel_reg, &auto_sel);
>>>
>>> +CC Sumit's actual email, sorry about that.
>>
>> So I'm assuming that this one is for Sumit to pick up and send again
>> along with a new version of
>>
>> https://lore.kernel.org/linux-acpi/20261112080442.4007723-1-sumitg@nvidia.com/
>>
>> Or please let me know if that is not the case.
> 
> Yup, while it fixes an issue by itself technically, I don't think there
> are any affected platforms.
> I'm hoping that Sumit this (and the limit-write-at-init) patch as a series
> for all our convenience (and hopefully to get Sashiko to review them too).
> (I guess I should've mentioned that in the notes.)
> 
> And sorry about ACPI/CPPC having such high traffic lately, I'm expecting once
> we've got the issues ironed out and the new features supported it'll be a
> lot more boring :)

So the basic idea with this patch that the userspace interface is always 
8 bit so higher level software will not need to know the lower level 
implementation detail at the expense of some granularity.

Makes sense to me, but I'd like to know how realistic this hardware is 
going to be.  If it's purely hypothetical hardware; how about proposing 
to ASWG that this field must always be 8 bit?