Re: [PATCH v8 3/3] cpufreq: CPPC: Reflect the OSPM nominal in boost and limits

[Sumit Gupta <sumitg@nvidia.com>]

Hi Christian,


On 03/11/26 22:42, Christian Loehle wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 10/30/26 17:27, Sumit Gupta wrote:
>> Boost is the performance range above nominal, so lowering the OSPM
>> Nominal Performance enlarges the boost range and drops the non-boost
>> ceiling. Keep the policy limits consistent with the register.
>>
>> Add cppc_cpufreq_effective_nominal(), which returns the OSPM Nominal
>> Performance when set and the platform nominal otherwise. Use it for the
>> non-boost ceiling in set_boost(), and to update the policy limits when
>> ospm_nominal_freq is written.
>>
>> The cpufreq core caps scaling_max_freq with a per-policy QoS request,
>> boost_freq_req, that it sets to cpuinfo.max_freq and refreshes only when
>> boost is toggled. A write to ospm_nominal_freq changes cpuinfo.max_freq
>> without toggling boost, so cppc_cpufreq_update_nominal_limits() updates
>> the request itself, rather than leaving scaling_max_freq at the old
>> nominal.
>>
>> When boost is enabled the ceiling is highest_perf, not the nominal, so
>> the update is skipped. set_boost() applies the new nominal when boost is
>> turned off.
>>
>> A platform with highest_perf == nominal_perf has no boost range at boot.
>> Lowering the OSPM nominal from sysfs can create one. The core, however,
>> adds its boost FREQ_QOS_MAX request only at policy setup, and only if
>> boost_supported is already set. Set boost_supported in init() when the
>> OSPM register is supported and highest_perf > lowest_perf. Boost can
>> then be enabled after the nominal is lowered.
>>
>> Suggested-by: Pierre Gondois <pierre.gondois@arm.com>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> 
> Thinking out loud here:
> I'm curious if there's actually a use-case for this, presumably once we
> let userspace effectively dictate boost allocation, we practically expect
> userspace to sort out scheduling, too (e.g. by pinning?) and therefore this
> becomes largely irrelevant?
> 

I think it is still needed. Lowering the OSPM nominal puts the levels
between it and the platform nominal into the boost range.
Without this patch, boost=0 still allows those levels. Userspace would
then need an extra frequency cap to stay out of boost. This patch makes
boost=0 cap the policy at the OSPM nominal.

>> ---
>>   .../ABI/testing/sysfs-devices-system-cpu      |  4 ++
>>   drivers/cpufreq/cppc_cpufreq.c                | 72 ++++++++++++++++++-
>>   2 files changed, 73 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
>> index 6af7fa86e44c..d219fc27a0c9 100644
>> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
>> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
>> @@ -362,6 +362,10 @@ Description:     OSPM Nominal Performance (kHz)
>>                Write a value in kHz, between the frequencies corresponding to
>>                Lowest Performance and Nominal Performance.
>>
>> +             While boost is disabled, cpuinfo_max_freq is set to this value
>> +             and scaling_max_freq cannot go above it. While boost is
>> +             enabled, cpuinfo_max_freq is the Highest Performance frequency.
>> +
>>                The register cannot be read back, so a read returns the last
>>                value written, rounded to a performance level. It returns
>>                "<unset>" before the first write. Reads and writes fail with
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 662397526433..7cac31de7325 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -915,6 +915,29 @@ static void cppc_cpufreq_put_cpu_data(struct cpufreq_policy *policy)
>>        policy->driver_data = NULL;
>>   }
>>
>> +/*
>> + * Return the non-boost performance ceiling. This is the OSPM Nominal
>> + * Performance the driver last requested, or the platform-reported Nominal
>> + * Performance if none was requested.
>> + *
>> + * The register is write-only, so the requested value comes from the
>> + * software-tracked state rather than from hardware.
>> + */
>> +static u32 cppc_cpufreq_effective_nominal(struct cpufreq_policy *policy)
>> +{
>> +     const struct cppc_policy_state *st = cppc_cpufreq_policy_state(policy);
>> +     struct cppc_cpudata *cpu_data = policy->driver_data;
>> +     u64 ospm_nominal;
>> +
>> +     ospm_nominal = st->regs[CPPC_SAVED_OSPM_NOMINAL_PERF][CPPC_SAVED_REQUESTED];
>> +
>> +     /* U64_MAX means OSPM has not selected a nominal level. */
>> +     if (ospm_nominal == U64_MAX)
>> +             return cpu_data->perf_caps.nominal_perf;
>> +
>> +     return (u32)ospm_nominal;
>> +}
>> +
>>   static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>   {
>>        unsigned int cpu = policy->cpu;
>> @@ -974,9 +997,14 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>
>>        /*
>>         * If 'highest_perf' is greater than 'nominal_perf', we assume CPU Boost
>> -      * is supported.
>> +      * is supported. A writable OSPM Nominal Performance register can also
>> +      * open a boost range at runtime by lowering the nominal, so assume
>> +      * boost is supported in that case too, letting the core register its
>> +      * QoS request up front.
>>         */
>> -     if (caps->highest_perf > caps->nominal_perf)
>> +     if (caps->highest_perf > caps->nominal_perf ||
>> +         (caps->highest_perf > caps->lowest_perf &&
>> +          cppc_ospm_nominal_perf_supported(cpu)))
>>                policy->boost_supported = true;
>>
>>        /*
>> @@ -1280,11 +1308,12 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>>   {
>>        struct cppc_cpudata *cpu_data = policy->driver_data;
>>        struct cppc_perf_caps *caps = &cpu_data->perf_caps;
>> +     u32 nominal = cppc_cpufreq_effective_nominal(policy);
>>
>>        if (state)
>>                policy->cpuinfo.max_freq = cppc_perf_to_khz(caps, caps->highest_perf);
>>        else
>> -             policy->cpuinfo.max_freq = cppc_perf_to_khz(caps, caps->nominal_perf);
>> +             policy->cpuinfo.max_freq = cppc_perf_to_khz(caps, nominal);
>>
>>        return 0;
>>   }
>> @@ -1494,6 +1523,41 @@ static int cppc_get_perf_limited_filtered(int cpu, u64 *perf_limited)
>>   CPPC_CPUFREQ_ATTR_RW_U64(perf_limited, cppc_get_perf_limited_filtered,
>>                         cppc_set_perf_limited)
>>
>> +/*
>> + * While boost is disabled, the nominal is the ceiling. Set cpuinfo.max_freq to
>> + * it and update the core's boost_freq_req to match. The core only syncs
>> + * boost_freq_req when boost is enabled or disabled, so a plain nominal change
>> + * must update it here. Without that, raising the nominal again would leave the
>> + * stale request capping the policy.
>> + *
>> + * Refresh the limits directly rather than leave it to the QoS notifier, which
>> + * only schedules the update, so the new ceiling is in place once the write
>> + * returns. The sysfs store path holds policy->rwsem, which the refresh needs.
> 
> Interestingly the global boost toggle doesn't seem to hold policy->rwsem when iterating
> over the policies. That seems to be an oversight, I'll send a patch.
> 
> The rest looks okay to me FWIW.
> 

Thanks for the fix.

Regards,
Sumit

....