Re: [PATCH v4 7/8] cpufreq: CPPC: update policy min/max when toggling auto_select

[Pierre Gondois <pierre.gondois@arm.com>]

On 11/5/25 12:38, Sumit Gupta wrote:
> When CPPC autonomous selection (auto_select) is enabled or disabled,
> the policy min/max frequency limits should be updated appropriately to
> reflect the new operating mode.
>
> Currently, toggling auto_select only changes the hardware register but
> doesn't update the cpufreq policy constraints, which can lead to
> inconsistent behavior between the hardware state and the policy limits
> visible to userspace and other kernel components.
>
> When auto_select is enabled, preserve the current min/max performance
> values to maintain user-configured limits. When disabled, the hardware
> operates in a default mode where the OS directly controls performance,
> so update the policy limits accordingly.
>
> Signed-off-by: Sumit Gupta<sumitg@nvidia.com>
> ---
>   drivers/cpufreq/cppc_cpufreq.c | 67 ++++++++++++++++++++++++++++++++--
>   1 file changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index a425ad575aa6..d1b44beaddda 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -646,6 +646,26 @@ static int cppc_cpufreq_set_mperf_limit(struct cpufreq_policy *policy, u64 val,
>   #define cppc_cpufreq_set_max_perf(policy, val, update_reg, update_policy) \
>   	cppc_cpufreq_set_mperf_limit(policy, val, update_reg, update_policy, false)
>   
> +static int cppc_cpufreq_update_autosel_val(struct cpufreq_policy *policy, bool auto_sel)
> +{
> +	struct cppc_cpudata *cpu_data = policy->driver_data;
> +	unsigned int cpu = policy->cpu;
> +	int ret;
> +
> +	pr_debug("cpu%d, auto_selcurr:%u,new:%d\n", cpu, cpu_data->perf_caps.auto_sel, auto_sel);
> +
> +	guard(mutex)(&cppc_cpufreq_update_autosel_config_lock);
Would it be possible to explain why we need this mutex specifically for 
auto_sel ?
> +
> +	ret = cppc_set_auto_sel(cpu, auto_sel);
> +	if (ret) {
> +		pr_warn("Failed to set auto_sel=%d for CPU%d (%d)\n", auto_sel, cpu, ret);
> +		return ret;
> +	}
> +	cpu_data->perf_caps.auto_sel = auto_sel;
> +
> +	return 0;
> +}
> +
>   static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>   {
>   	unsigned int cpu = policy->cpu;
> @@ -879,8 +899,49 @@ static ssize_t show_auto_select(struct cpufreq_policy *policy, char *buf)
>   	return sysfs_emit(buf, "%d\n", val);
>   }
>   
> -static ssize_t store_auto_select(struct cpufreq_policy *policy,
> -				 const char *buf, size_t count)
> +/**
> + * cppc_cpufreq_update_auto_select - Update autonomous selection config for policy->cpu
> + * @policy: cpufreq policy
> + * @enable: enable/disable autonomous selection
> + */
> +static int cppc_cpufreq_update_auto_select(struct cpufreq_policy *policy, bool enable)
> +{
> +	struct cppc_cpudata *cpu_data = policy->driver_data;
> +	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
> +	u64 min_perf = caps->lowest_nonlinear_perf;
> +	u64 max_perf = caps->nominal_perf;
> +	int ret;
> +
> +	if (enable) {
> +		if (cpu_data->perf_ctrls.min_perf)
> +			min_perf = cpu_data->perf_ctrls.min_perf;
> +		if (cpu_data->perf_ctrls.max_perf)
> +			max_perf = cpu_data->perf_ctrls.max_perf;
> +	}

I think the min/max performance values are still relevant when auto_sel is
disabled/absent. So:
- enabling/disabling autonomous selection
- setting min/max perf values
should not have any dependency I think.


> +
> +	/*
> +	 * Set min/max performance registers and update policy constraints.
> +	 *   When enabling: update both registers and policy.
> +	 *   When disabling: update policy only.
> +	 * Continue even if min/max are not supported, as EPP and autosel
> +	 * might still be supported.
> +	 */
> +	ret = cppc_cpufreq_set_min_perf(policy, min_perf, enable, true);
> +	if (ret && ret != -EOPNOTSUPP)
> +		return ret;
> +
> +	ret = cppc_cpufreq_set_max_perf(policy, max_perf, enable, true);
> +	if (ret && ret != -EOPNOTSUPP)
> +		return ret;
> +
> +	ret = cppc_cpufreq_update_autosel_val(policy, enable);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static ssize_t store_auto_select(struct cpufreq_policy *policy, const char *buf, size_t count)
>   {
>   	bool val;
>   	int ret;
> @@ -889,7 +950,7 @@ static ssize_t store_auto_select(struct cpufreq_policy *policy,
>   	if (ret)
>   		return ret;
>   
> -	ret = cppc_set_auto_sel(policy->cpu, val);
> +	ret = cppc_cpufreq_update_auto_select(policy, val);
>   	if (ret)
>   		return ret;
>