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

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

On Mon, Mar 02, 2026 at 01:37:00PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Fri, 27 Feb 2026 at 16:33, 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
> > 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>
> 
> Thanks for your patch!

Hi,

thanks for having a look.

> 
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -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) {
> > +               *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);
> 
> step_size is u64, while do_div() truncates it to 32-bit.

Yes, as pointed out also by other reviewers, there are pre-existent bugs
probably in this rounding...this patch was meant only to move the logic
away from the CLK SCMI driver into the SCMI Clock protocol layer since
it enables a few simplification...

In the next V2, I will fix rounding errors by adding a dedicated Fix on
top of the original code, before this 'relocation', so as to make the
backport easier and move the fixed code.

Thanks,
Cristian