Re: [PATCH V2 2/4] firmware: arm_scmi: Add perf_freq_xlate interface

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

On Wed, Jan 17, 2024 at 04:11:14PM +0530, Sibi Sankar wrote:
> Add a new perf_freq_xlate interface to the existing perf_ops to translate
> a given perf index to frequency.
> 
> This can be used by the cpufreq driver and framework to determine the
> throttled frequency from a given perf index and apply HW pressure
> accordingly.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v2:
> * Rename opp_xlate -> freq_xlate [Viresh]
> 
>  drivers/firmware/arm_scmi/perf.c | 21 +++++++++++++++++++++
>  include/linux/scmi_protocol.h    |  3 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index ae7681eda276..e286f04ee6e3 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -977,6 +977,26 @@ static int scmi_notify_support(const struct scmi_protocol_handle *ph, u32 domain
>  	return 0;
>  }
>  
> +static int scmi_perf_freq_xlate(const struct scmi_protocol_handle *ph, u32 domain,
> +				int idx, unsigned long *freq)
> +{
> +	struct perf_dom_info *dom;
> +
> +	dom = scmi_perf_domain_lookup(ph, domain);
> +	if (IS_ERR(dom))
> +		return PTR_ERR(dom);
> +
> +	if (idx >= dom->opp_count)
> +		return -ERANGE;
> +
> +	if (!dom->level_indexing_mode)
> +		*freq = dom->opp[idx].perf * dom->mult_factor;
> +	else
> +		*freq = dom->opp[idx].indicative_freq * dom->mult_factor;
> +

As said elsewhere the plan would be to change slightly the SCMI core to
avoid the need for this patch and the previous one (while NOT exposing
too much Perf info)...

... anyway just looking at the above freq calc logic in this patch, be
aware that as it stands it seems to me broken, since the idx you use to
peek into the opp array comes (in the next patch) from the range_max
carried by the notification and that can be, indeed, a perf_level OR a
perf_index BUT it is absolutely NOT guaranteed to be an index into the
opp[] array...so it may work in your case if you have a platform
defining level or indexes matching the opp[] indexes BUT it is not true
in general. (but as said, this will be handled by the core and possibly
this patch dropped...)

Thanks,
Cristian