Re: [PATCH 3/3] cpufreq: scmi: Set transition_delay_us

[Cristian Marussi <cristian.marussi@arm.com>]

On Thu, Feb 22, 2024 at 02:57:01PM +0100, Pierre Gondois wrote:
> Make use of the newly added callbacks:
> - rate_limit_get()
> - fast_switch_rate_limit()
> to populate policies's `transition_delay_us`, defined as the
> 'Preferred average time interval between consecutive
> invocations of the driver to set the frequency for this policy.'
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  drivers/cpufreq/scmi-cpufreq.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 

Hi,

> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index 4ee23f4ebf4a..0b483bd0d3ca 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -144,6 +144,29 @@ scmi_get_cpu_power(struct device *cpu_dev, unsigned long *power,
>  	return 0;
>  }
>  
> +static int
> +scmi_get_rate_limit(u32 domain, bool has_fast_switch)
> +{
> +	int ret, rate_limit;
> +
> +	if (has_fast_switch) {
> +		/*
> +		 * Fast channels are used whenever available,
> +		 * so use their rate_limit value if populated.
> +		 */
> +		ret = perf_ops->fast_switch_rate_limit(ph, domain,
> +						       &rate_limit);
> +		if (!ret && rate_limit)
> +			return rate_limit;
> +	}
> +
> +	ret = perf_ops->rate_limit_get(ph, domain, &rate_limit);
> +	if (ret)
> +		return 0;
> +
> +	return rate_limit;
> +}
> +
>  static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  {
>  	int ret, nr_opp, domain;
> @@ -250,6 +273,9 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  	policy->fast_switch_possible =
>  		perf_ops->fast_switch_possible(ph, domain);
>  
> +	policy->transition_delay_us =
> +		scmi_get_rate_limit(domain, policy->fast_switch_possible);
> +
>  	return 0;
>  

As a second thought, I have just realized that now we have 2 ops to get the
rate_limit for a domain, one used in case of FCs and another in case of std
messaging w/out FCs, BUT given that we always use FCs when available, AND we
do not indeed have any way from perf_ops to explicitly request a set/get
ops NOT to use FCs when available, does it even make sense to expose such
2 functions ? Do we need such flexibility ?

Shouldn't we just expose one single rate_limit perf_ops and let the SCMI core
decide what to return depending on the presence or not of the FCs for that
domain ?

Maybe @Sudeep thinks differently.

Thanks,
Cristian