Re: [PATCH 01/11] firmware: arm_scmi: Add clock determine_rate operation

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 27 Feb 2026 15:32:15 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Add a clock operation to help determining the effective rate, closest to
> the required one, that a specific clock can support.
> 
> Calculation is currently performed kernel side and the logic is taken
> directly from the SCMI Clock driver: embedding the determinate rate logic
> in the protocol layer enables semplifications in the SCMI Clock protocol

simplifications

> interface and  will more easily accommodate further evolutions where such
> determine_rate logic into is optionally delegated to the platform SCMI
> server.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Hi Cristian,

Drive by review follows.  It's Friday afternoon an only a few mins to beer
o'clock :)

> ---
> Spoiler alert next SCMI spec will most probably include a new
> CLOCK_DETERMINE_RATE command to delegate to the platform such calculations,
> so this clock proto_ops will be needed anyway sooner or later
> ---
>  drivers/firmware/arm_scmi/clock.c | 42 +++++++++++++++++++++++++++++++
>  include/linux/scmi_protocol.h     |  6 +++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index ab36871650a1..54e8b59c3941 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -8,6 +8,7 @@
>  #include <linux/module.h>
>  #include <linux/limits.h>
>  #include <linux/sort.h>
> +#include <asm/div64.h>
>  
>  #include "protocols.h"
>  #include "notify.h"
> @@ -624,6 +625,46 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
>  	return ret;
>  }
>  
> +static int scmi_clock_determine_rate(const struct scmi_protocol_handle *ph,
> +				     u32 clk_id, unsigned long *rate)
> +{
> +	u64 fmin, fmax, ftmp;
> +	struct scmi_clock_info *clk;
> +	struct clock_info *ci = ph->get_priv(ph);
> +
> +	if (!rate)
> +		return -EINVAL;
> +
> +	clk = scmi_clock_domain_lookup(ci, clk_id);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	/*
> +	 * If we can't figure out what rate it will be, so just return the
> +	 * rate back to the caller.
> +	 */
> +	if (clk->rate_discrete)
> +		return 0;
> +
> +	fmin = clk->range.min_rate;
> +	fmax = clk->range.max_rate;
> +	if (*rate <= fmin) {

Does the rate ever end up different by doing this than it would if you
just dropped these short cuts? If not I wonder if this code complexity
is worthwhile vs

	*rate = clamp(*rate, clk->range.min_rate, clk->range.max_rate);

then carry on with the clamping to a step.

The only case I can immediately spot where it would be different would
be if (range.max_rate - range.min_rate) % range.step_size != 0
which smells like an invalid clock and could result in an out of
range rounding up anyway.

> +		*rate = fmin;
> +		return 0;
> +	} else if (*rate >= fmax) {
> +		*rate = fmax;
> +		return 0;
> +	}
> +
> +	ftmp = *rate - fmin;
> +	ftmp += clk->range.step_size - 1; /* to round up */
> +	do_div(ftmp, clk->range.step_size);
> +
> +	*rate = ftmp * clk->range.step_size + fmin;
> +
> +	return 0;
> +}