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

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

On Fri, Feb 27, 2026 at 04:50:09PM +0000, Jonathan Cameron wrote:
> 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 :)

Thanks for having a look :P

> 
> > ---
> > 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.

Yes indeed, but this patch aimed really ONLY at moving this logic from the
CLK SCMI driver into the SCMI Clock protocol layer since it then enables
more simplications...I have NOT really look into the logic...I suppose I
could optimize this in a following distinct patch...

> 
> > +		*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;
> > +}
> 

Thanks,
Cristian