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

[Peng Fan <peng.fan@oss.nxp.com>]

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 ">="?

>+		sort(clkd->r.rates, clkd->r.num_rates,
>+		     sizeof(clkd->r.rates[0]), rate_cmp_func, NULL);
> 

Regards
Peng