Re: [PATCH 11/11] firmware: arm_scmi: Introduce all_rates_get clock operation

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

On Sat, Feb 28, 2026 at 10:49:47AM +0800, Peng Fan wrote:
> On Fri, Feb 27, 2026 at 03:32:25PM +0000, Cristian Marussi wrote:
> >Add a clock operation to get the whole set of rates available to a specific
> >clock: when needed this request could transparently trigger a full rate
> >discovery enumeration if this specific clock-rates were previously only
> >lazily enumerated.
> >
> >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >---
> > drivers/firmware/arm_scmi/clock.c | 85 +++++++++++++++++++++----------
> > include/linux/scmi_protocol.h     |  9 ++++
> > 2 files changed, 67 insertions(+), 27 deletions(-)
> >
> >diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> >index a0de10652abe..c2fd9a1c3316 100644
> >--- a/drivers/firmware/arm_scmi/clock.c
> >+++ b/drivers/firmware/arm_scmi/clock.c
> >@@ -159,10 +159,8 @@ struct scmi_clock_rate_notify_payld {
> > 
> > struct scmi_clock_desc {
> > 	u32 id;
> >-	bool rate_discrete;
> > 	unsigned int tot_rates;
> >-	unsigned int num_rates;
> >-	u64 *rates;
> >+	struct scmi_clock_rates r;
> > #define	RATE_MIN	0
> > #define	RATE_MAX	1
> > #define	RATE_STEP	2
> >@@ -469,10 +467,10 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,
> > 	flags = le32_to_cpu(r->num_rates_flags);
> > 	st->num_remaining = NUM_REMAINING(flags);
> > 	st->num_returned = NUM_RETURNED(flags);
> >-	p->clkd->rate_discrete = RATE_DISCRETE(flags);
> >+	p->clkd->r.rate_discrete = RATE_DISCRETE(flags);
> > 
> > 	/* Warn about out of spec replies ... */
> >-	if (!p->clkd->rate_discrete &&
> >+	if (!p->clkd->r.rate_discrete &&
> > 	    (st->num_returned != 3 || st->num_remaining != 0)) {
> > 		dev_warn(p->dev,
> > 			 "Out-of-spec CLOCK_DESCRIBE_RATES reply for %s - returned:%d remaining:%d rx_len:%zd\n",
> >@@ -486,9 +484,9 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,
> > 	if (!st->max_resources) {
> > 		unsigned int tot_rates = st->num_returned + st->num_remaining;
> > 
> >-		p->clkd->rates = devm_kcalloc(p->dev, tot_rates,
> >-					      sizeof(*p->clkd->rates), GFP_KERNEL);
> >-		if (!p->clkd->rates)
> >+		p->clkd->r.rates = devm_kcalloc(p->dev, tot_rates,
> >+						sizeof(*p->clkd->r.rates), GFP_KERNEL);
> >+		if (!p->clkd->r.rates)
> > 			return -ENOMEM;
> > 
> > 		/* max_resources is used by the iterators to control bounds */
> >@@ -507,10 +505,10 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
> > 	struct scmi_clk_ipriv *p = priv;
> > 	const struct scmi_msg_resp_clock_describe_rates *r = response;
> > 
> >-	p->clkd->rates[p->clkd->num_rates] = RATE_TO_U64(r->rate[st->loop_idx]);
> >+	p->clkd->r.rates[p->clkd->r.num_rates] = RATE_TO_U64(r->rate[st->loop_idx]);
> > 
> > 	/* Count only effectively discovered rates */
> >-	p->clkd->num_rates++;
> >+	p->clkd->r.num_rates++;
> > 
> > 	return 0;
> > }
> >@@ -531,7 +529,13 @@ scmi_clock_describe_rates_get_full(const struct scmi_protocol_handle *ph,
> > 		.dev = ph->dev,
> > 	};
> > 
> >-	iter = ph->hops->iter_response_init(ph, &ops, 0, CLOCK_DESCRIBE_RATES,
> >+	/*
> >+	 * Using tot_rates as max_resources parameter here so as to trigger
> >+	 * the dynamic allocation only when strictly needed: when trying a
> >+	 * full enumeration after a lazy one tot_rates will be non-zero.
> >+	 */
> >+	iter = ph->hops->iter_response_init(ph, &ops, clkd->tot_rates,
> >+					    CLOCK_DESCRIBE_RATES,
> > 					    sizeof(struct scmi_msg_clock_describe_rates),
> > 					    &cpriv);
> > 	if (IS_ERR(iter))
> >@@ -542,12 +546,12 @@ scmi_clock_describe_rates_get_full(const struct scmi_protocol_handle *ph,
> > 		return ret;
> > 
> > 	/* empty set ? */
> >-	if (!clkd->num_rates)
> >+	if (!clkd->r.num_rates)
> > 		return 0;
> > 
> >-	if (clkd->rate_discrete)
> >-		sort(clkd->rates, clkd->num_rates,
> >-		     sizeof(clkd->rates[0]), rate_cmp_func, NULL);
> >+	if (clkd->r.rate_discrete && PROTOCOL_REV_MAJOR(ph->version) == 0x1)
> 
> Not understand well "PROTOCOL_REV_MAJOR(ph->version) == 0x1", I may
> get something wrong, should use ">="?

I have NOT double checked BUT I think fro Etienne original patch, you
can assume that clock rates are returned by the platform in ascending
order (already sorted) only after Clock protocol version 0x01, the first
ever, so ONLY when teh used version is 0x1 we must perform a full scan
(no lazy optimization since we cannot assume that rates[last-1] is max
AND we must sort the obtained list of clocks...

Thanks,
Cristian