Re: [PATCH v2 2/2] ACPI: CPPC: Add ospm_nominal_perf support

Pierre Gondois
2026-04-30T16:26:49Z

Hello Sumit,

On 4/30/26 16:24, Sumit Gupta wrote:
> Add acpi_cppc/ospm_nominal_perf sysfs attribute (read-write) and
> cppc_set_ospm_nominal_perf() API for the OSPM Nominal Performance
> register (ACPI 6.6, Section 8.4.6.1.2.6).
>
> The register conveys the desired nominal performance level at which
> the platform may run. OSPM can request a lower level than platform
> nominal. Valid range is [Lowest Performance, Nominal Performance].
> The value tells the platform what OSPM considers nominal. The
> platform classifies performance above this as boosted and below as
> throttled. It uses that for its power/thermal decisions.
>
> Although the register is write-only per spec, cache the OSPM-written
> value in cpc_desc so userspace can observe it via sysfs, and to
> skip redundant writes.
>
> Initialize to platform nominal at policy init. Override via sysfs
> if needed.
>
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> ---
> drivers/acpi/cppc_acpi.c | 69 ++++++++++++++++++++++++++++++++++
> drivers/cpufreq/cppc_cpufreq.c | 10 +++++
> include/acpi/cppc_acpi.h | 6 +++
> 3 files changed, 85 insertions(+)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index a1c91ce20cc8..fbc620adafad 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -155,6 +155,10 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
> static struct kobj_attribute _name = \
> __ATTR(_name, 0444, show_##_name, NULL)
>
> +#define define_one_cppc_rw(_name) \
> +static struct kobj_attribute _name = \
> +__ATTR(_name, 0644, show_##_name, store_##_name)
> +
> #define to_cpc_desc(a) container_of(a, struct cpc_desc, kobj)
>
> #define show_cppc_data(access_fn, struct_name, member_name) \
> @@ -211,6 +215,38 @@ static ssize_t show_feedback_ctrs(struct kobject *kobj,
> }
> define_one_cppc_ro(feedback_ctrs);
>
> +static ssize_t show_ospm_nominal_perf(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
> + u64 val = READ_ONCE(cpc_ptr->ospm_nominal_perf);
> +
> + if (!val)
> + return -ENODATA;
> +
> + return sysfs_emit(buf, "%llu\n", val);
> +}
> +
> +static ssize_t store_ospm_nominal_perf(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
> + u64 val;
> + int ret;
> +
> + ret = kstrtou64(buf, 0, &val);
> + if (ret)
> + return ret;
> +
> + ret = cppc_set_ospm_nominal_perf(cpc_ptr->cpu_id, val);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +define_one_cppc_rw(ospm_nominal_perf);
> +
> static struct attribute *cppc_attrs[] = {
> &feedback_ctrs.attr,
> &reference_perf.attr,
> @@ -222,6 +258,7 @@ static struct attribute *cppc_attrs[] = {
> &nominal_perf.attr,
> &nominal_freq.attr,
> &lowest_freq.attr,
> + &ospm_nominal_perf.attr,
> NULL
> };
> ATTRIBUTE_GROUPS(cppc);
> @@ -1683,6 +1720,38 @@ int cppc_set_epp(int cpu, u64 epp_val)
> }
> EXPORT_SYMBOL_GPL(cppc_set_epp);
>
> +/**
> + * cppc_set_ospm_nominal_perf() - Write OSPM Nominal Performance register.
> + * @cpu: CPU on which to write register.
> + * @ospm_nominal_perf: Value to write to the OSPM Nominal Performance register.
> + *
> + * OSPM Nominal Performance allows OSPM to inform the platform of the nominal
> + * performance level it intends to maintain.
> + *
> + * Return: 0 for success, -EINVAL on invalid input, -EOPNOTSUPP if not
> + * supported, -EIO otherwise.
> + */
> +int cppc_set_ospm_nominal_perf(int cpu, u64 ospm_nominal_perf)
> +{
> + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> + int ret;
> +
> + if (!ospm_nominal_perf || ospm_nominal_perf > U32_MAX)
> + return -EINVAL;
I think the spec also requests to have a value in the range

[lowest:nominal]. As these registers are read-only it should

be ok to read the values here ?

> +
> + if (cpc_desc &&
> + READ_ONCE(cpc_desc->ospm_nominal_perf) == ospm_nominal_perf)
> + return 0;
> +
> + ret = cppc_set_reg_val(cpu, OSPM_NOMINAL_PERF, ospm_nominal_perf);
> + if (ret)
> + return ret;
> +

Shouldn't we have some protection against concurrent accesses ?

> + WRITE_ONCE(cpc_desc->ospm_nominal_perf, ospm_nominal_perf);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cppc_set_ospm_nominal_perf);
> +
> /**
> * cppc_get_auto_act_window() - Read autonomous activity window register.
> * @cpu: CPU from which to read register.
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 7e7f9dfb7a24..d06cba963550 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -715,6 +715,16 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> goto out;
> }
>
> + /*
> + * Initialize OSPM Nominal Performance to inform firmware of
> + * OSPM's nominal level. Performance above this value = boost;
> + * below = throttle. Uses platform nominal by default.
> + */
> + ret = cppc_set_ospm_nominal_perf(cpu, caps->nominal_perf);
> + if (ret && ret != -EOPNOTSUPP)
> + pr_debug("Failed to set ospm_nominal_perf for CPU%d: %d\n",
> + cpu, ret);
> +

IIUC, if (ospm_nominal_perf == nominal_perf), the firmware should
not behave differently. Is this really useful ?

------------

Also this seems like there will need some synchronization
mechanism to keep-up with the boost state.

If the ospm_nominal_perf is lowered and boost is disabled,
a freq. update should happen. IMO it looks like this could
be handled with (another) freq_qos_request.

This new freq_qos_request, if we name it ospm_nominal_freq_req,
should only be taken into account if boost is disabled.
Otherwise, if boost is enabled, ospm_nominal_freq_req
should be ignored.

------------

Also, the function seems to set the ospm_nominal_freq for
a single CPU when the policy might be common for multiple
CPUs right ?

The issues this field raises seems similar to the auto_sel
ones. I.e. :

- concurrency accesses + need for a scratch value

- what should happen when unloading the driver

- the value can be set for single CPUs but we might
want to have the same value for the whole policy

Maybe a common solution should be found.
(I m not suggesting anything right now unfortunately).

> cppc_cpufreq_cpu_fie_init(policy);
> return 0;
>
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 8693890a7275..3771e2ed507d 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -86,6 +86,7 @@ struct cpc_desc {
> struct cpc_register_resource cpc_regs[MAX_CPC_REG_ENT];
> struct acpi_psd_package domain_info;
> struct kobject kobj;
> + u32 ospm_nominal_perf;
> };
>
> /* These are indexes into the per-cpu cpc_regs[]. Order is important. */
> @@ -180,6 +181,7 @@ extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
> extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf);
> extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
> extern int cppc_set_epp(int cpu, u64 epp_val);
> +extern int cppc_set_ospm_nominal_perf(int cpu, u64 ospm_nominal_perf);
> extern int cppc_get_auto_act_window(int cpu, u64 *auto_act_window);
> extern int cppc_set_auto_act_window(int cpu, u64 auto_act_window);
> extern int cppc_get_auto_sel(int cpu, bool *enable);
> @@ -266,6 +268,10 @@ static inline int cppc_set_epp(int cpu, u64 epp_val)
> {
> return -EOPNOTSUPP;
> }
> +static inline int cppc_set_ospm_nominal_perf(int cpu, u64 ospm_nominal_perf)
> +{
> + return -EOPNOTSUPP;
> +}
> static inline int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
> {
> return -EOPNOTSUPP;