Re: [PATCH 2/2] cpufreq: scmi: Update Energy Model with allowed performance limits

From: Cristian Marussi
Date: Wed May 01 2024 - 05:26:57 EST


On Wed, Apr 03, 2024 at 05:23:15PM +0100, Lukasz Luba wrote:
> The Energy Model (EM) supports performance limits updates. Use the SCMI
> notifications to get information from FW about allowed frequency scope for
> the CPUs.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> ---
> drivers/cpufreq/scmi-cpufreq.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index d946b7a082584..90c8448578cb1 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -185,12 +185,25 @@ static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned long event,
> {
> struct scmi_data *priv = container_of(nb, struct scmi_data, limit_notify_nb);
> struct scmi_perf_limits_report *limit_notify = data;
> + unsigned int limit_freq_max_khz, limit_freq_min_khz;
> struct cpufreq_policy *policy = priv->policy;
> - unsigned int limit_freq_khz;
> + struct em_perf_domain *pd;
> + int ret;
> +
> + limit_freq_max_khz = limit_notify->range_max_freq / HZ_PER_KHZ;
> + limit_freq_min_khz = limit_notify->range_min_freq / HZ_PER_KHZ;

Note that these values could be zeroed if the notification is good but
the range_min/range_max values could NOT be mapped to a frequency
equivalent (due to some FW errors).

I would probably have to add a warn about this error in the core SCMI
notification path (or drop the notif as a whole); if not here you could
end-up just setting max/min to 0 if the fw has messed up the
notification range_min/range_max.

Or is it just that, especially max_feq = 0 is NOT plausible value and you
will need anyway to check it here ?

>
> - limit_freq_khz = limit_notify->range_max_freq / HZ_PER_KHZ;
> + pd = em_cpu_get(policy->cpu);
> + if (pd) {
> + ret = em_update_performance_limits(pd, limit_freq_min_khz,
> + limit_freq_max_khz);
> + if (ret)
> + dev_warn(priv->cpu_dev,
> + "EM perf limits update failed\n");
> + }
>
> - policy->max = clamp(limit_freq_khz, policy->cpuinfo.min_freq, policy->cpuinfo.max_freq);
> + policy->max = clamp(limit_freq_max_khz, policy->cpuinfo.min_freq,
> + policy->cpuinfo.max_freq);

FWIW, regarding the SCMI bits.

LGTM.
Reviewed-by: Cristian Marussi <cristian.marussi@xxxxxxx>

Thanks,
Cristian