[PATCH 00/11] SCMI Clock rates discovery rework

[PATCH 00/11] SCMI Clock rates discovery rework

[Cristian Marussi]

Hi,

it was a known limitation, in the SCMI Clock protocol support, the lack of
dynamic allocation around per-clock rates discovery: fixed size statically
per-clock rates arrays did not scale and was increasingly a waste of memory
(see [1]).

This series aim at solving this in successive steps:

 - simplify and reduce to the minimum possible the rates data info exposed
   to the SCMI driver by scmi_clock_info
 - move away from static fixed allocation of per-clock rates arrays in
   favour of a completely dynamic runtime allocation: just allocate what
   is needed based on the effectively discovered

This is done in patches 1-6.

A further bigger optimization suggested in a past series [1] by Etienne
would be, whenever allowed by the spec, to limit upfront the number of
queries in order to simply retrieve min and max rate, that are indeed the
only rates needed by the CLK SCMI driver.

The approach proposed in [1] was open coding and duplicating some of the
functionalities already provided by SCMI iterators, though.

Patch 7-10 implement such optimization instead by:

 - reworking core SCMI iterators to support bound enumerations
 - use such new bound iterators to perform the minimum number of queries
   in order to ony retrieve min an max rate

As a final result now the rates enumeration triggered by the CLK SCMI
driver, while still allocating for all the existent rates, miminize the
number of SCMI CLK_DESCRIBE_RATE messages needed to obtain min and max.

Finally, patch 11 introduces a new clock protocol operation to be able to
trigger anytime on demand a full enumeration and obtain the full list of
rates when needed, not only min/max: this latter method is really only used
currently by some dowstream SCMI Test driver of mine.

Based on v7.0-rc1. 

Tested on JUNO and an emulated environment.

Any feeback welcome.

Thanks,
Cristian

[1]: https://lore.kernel.org/arm-scmi/aZsX-oplR6fiLBBN@pluto/T/#t
[2]: https://lore.kernel.org/20241203173908.3148794-2-etienne.carriere@foss.st.com
---

Cristian Marussi (11):
  firmware: arm_scmi: Add clock determine_rate operation
  clk: scmi: Use new determine_rate clock operation
  firmware: arm_scmi: Simplify clock rates exposed interface
  clk: scmi: Use new simplified per-clock rate properties
  firmware: arm_scmi: Drop unused clock rate interfaces
  firmware: arm_scmi: Make clock rates allocation dynamic
  firmware: arm_scmi: Harden clock parents discovery
  firmware: arm_scmi: Refactor iterators internal allocation
  firmware: arm_scmi: Add bound iterators support
  firmware: arm_scmi: Use bound iterators to minimize discovered rates
  firmware: arm_scmi: Introduce all_rates_get clock operation

 drivers/clk/clk-scmi.c                |  48 +----
 drivers/firmware/arm_scmi/clock.c     | 296 ++++++++++++++++++++------
 drivers/firmware/arm_scmi/driver.c    |  73 +++++--
 drivers/firmware/arm_scmi/protocols.h |  13 +-
 include/linux/scmi_protocol.h         |  29 +--
 5 files changed, 314 insertions(+), 145 deletions(-)

-- 
2.53.0

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

[Cristian Marussi]

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>
---
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) {
+		*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;
+}
+
 static int
 scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id,
 		      enum clk_state state,
@@ -936,6 +977,7 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
 	.info_get = scmi_clock_info_get,
 	.rate_get = scmi_clock_rate_get,
 	.rate_set = scmi_clock_rate_set,
+	.determine_rate = scmi_clock_determine_rate,
 	.enable = scmi_clock_enable,
 	.disable = scmi_clock_disable,
 	.state_get = scmi_clock_state_get,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index aafaac1496b0..28579c145045 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -91,6 +91,10 @@ enum scmi_clock_oem_config {
  * @info_get: get the information of the specified clock
  * @rate_get: request the current clock rate of a clock
  * @rate_set: set the clock rate of a clock
+ * @determine_rate: determine the effective rate that can be supported by a
+ *		    clock calculating the closest allowed rate.
+ *		    Note that @rate is an input/output parameter used both to
+ *		    describe the requested rate and report the closest match
  * @enable: enables the specified clock
  * @disable: disables the specified clock
  * @state_get: get the status of the specified clock
@@ -108,6 +112,8 @@ struct scmi_clk_proto_ops {
 			u64 *rate);
 	int (*rate_set)(const struct scmi_protocol_handle *ph, u32 clk_id,
 			u64 rate);
+	int (*determine_rate)(const struct scmi_protocol_handle *ph, u32 clk_id,
+			      unsigned long *rate);
 	int (*enable)(const struct scmi_protocol_handle *ph, u32 clk_id,
 		      bool atomic);
 	int (*disable)(const struct scmi_protocol_handle *ph, u32 clk_id,
-- 
2.53.0

[PATCH 02/11] clk: scmi: Use new determine_rate clock operation

[Cristian Marussi]

Use the Clock protocol layer determine_rate logic to calculate the closest
rate that can be supported by a specific clock.

No functional change.

Cc: Brian Masney <bmasney@redhat.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Note that the calculation logic in the protocol layer is exactly the same
as it wes here.

@Brian I suppose once your CLK_ROUNDING_FW_MANAGED sereis is merged I can flag
such SCMI clocks.
---
 drivers/clk/clk-scmi.c | 31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 6b286ea6f121..c223e4ef1dd1 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -12,7 +12,6 @@
 #include <linux/of.h>
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
-#include <asm/div64.h>
 
 #define NOT_ATOMIC	false
 #define ATOMIC		true
@@ -57,35 +56,17 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
 static int scmi_clk_determine_rate(struct clk_hw *hw,
 				   struct clk_rate_request *req)
 {
-	u64 fmin, fmax, ftmp;
+	int ret;
 	struct scmi_clk *clk = to_scmi_clk(hw);
 
 	/*
-	 * We can't figure out what rate it will be, so just return the
-	 * rate back to the caller. scmi_clk_recalc_rate() will be called
-	 * after the rate is set and we'll know what rate the clock is
+	 * If we could not get a better rate scmi_clk_recalc_rate() will be
+	 * called after the rate is set and we'll know what rate the clock is
 	 * running at then.
 	 */
-	if (clk->info->rate_discrete)
-		return 0;
-
-	fmin = clk->info->range.min_rate;
-	fmax = clk->info->range.max_rate;
-	if (req->rate <= fmin) {
-		req->rate = fmin;
-
-		return 0;
-	} else if (req->rate >= fmax) {
-		req->rate = fmax;
-
-		return 0;
-	}
-
-	ftmp = req->rate - fmin;
-	ftmp += clk->info->range.step_size - 1; /* to round up */
-	do_div(ftmp, clk->info->range.step_size);
-
-	req->rate = ftmp * clk->info->range.step_size + fmin;
+	ret = scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);
+	if (ret)
+		return ret;
 
 	return 0;
 }
-- 
2.53.0

[PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface

[Cristian Marussi]

Move needlessly exposed fields away from scmi_clock_info into the new
internal struct scmi_clock_desc while keeping exposed only the two new
min_rate and max_rate fields for each clock.

No functional change.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 145 +++++++++++++++---------------
 include/linux/scmi_protocol.h     |   2 +
 2 files changed, 74 insertions(+), 73 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 54e8b59c3941..f5d1c608f85a 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -157,13 +157,27 @@ struct scmi_clock_rate_notify_payld {
 	__le32 rate_high;
 };
 
+struct scmi_clock_desc {
+	u32 id;
+	bool rate_discrete;
+	unsigned int num_rates;
+	u64 rates[SCMI_MAX_NUM_RATES];
+#define	RATE_MIN	0
+#define	RATE_MAX	1
+#define	RATE_STEP	2
+	struct scmi_clock_info info;
+};
+
+#define to_desc(p)	(container_of((p), struct scmi_clock_desc, info))
+
 struct clock_info {
 	int num_clocks;
 	int max_async_req;
 	bool notify_rate_changed_cmd;
 	bool notify_rate_change_requested_cmd;
 	atomic_t cur_async_req;
-	struct scmi_clock_info *clk;
+	struct scmi_clock_desc *clkds;
+#define CLOCK_INFO(c, i)	(&(((c)->clkds + (i))->info))
 	int (*clock_config_set)(const struct scmi_protocol_handle *ph,
 				u32 clk_id, enum clk_state state,
 				enum scmi_clock_oem_config oem_type,
@@ -185,7 +199,7 @@ scmi_clock_domain_lookup(struct clock_info *ci, u32 clk_id)
 	if (clk_id >= ci->num_clocks)
 		return ERR_PTR(-EINVAL);
 
-	return ci->clk + clk_id;
+	return CLOCK_INFO(ci, clk_id);
 }
 
 static int
@@ -226,8 +240,7 @@ scmi_clock_protocol_attributes_get(const struct scmi_protocol_handle *ph,
 
 struct scmi_clk_ipriv {
 	struct device *dev;
-	u32 clk_id;
-	struct scmi_clock_info *clk;
+	struct scmi_clock_desc *clkd;
 };
 
 static void iter_clk_possible_parents_prepare_message(void *message, unsigned int desc_index,
@@ -236,7 +249,7 @@ static void iter_clk_possible_parents_prepare_message(void *message, unsigned in
 	struct scmi_msg_clock_possible_parents *msg = message;
 	const struct scmi_clk_ipriv *p = priv;
 
-	msg->id = cpu_to_le32(p->clk_id);
+	msg->id = cpu_to_le32(p->clkd->id);
 	/* Set the number of OPPs to be skipped/already read */
 	msg->skip_parents = cpu_to_le32(desc_index);
 }
@@ -246,7 +259,6 @@ static int iter_clk_possible_parents_update_state(struct scmi_iterator_state *st
 {
 	const struct scmi_msg_resp_clock_possible_parents *r = response;
 	struct scmi_clk_ipriv *p = priv;
-	struct device *dev = ((struct scmi_clk_ipriv *)p)->dev;
 	u32 flags;
 
 	flags = le32_to_cpu(r->num_parent_flags);
@@ -258,12 +270,13 @@ static int iter_clk_possible_parents_update_state(struct scmi_iterator_state *st
 	 * assume it's returned+remaining on first call.
 	 */
 	if (!st->max_resources) {
-		p->clk->num_parents = st->num_returned + st->num_remaining;
-		p->clk->parents = devm_kcalloc(dev, p->clk->num_parents,
-					       sizeof(*p->clk->parents),
-					       GFP_KERNEL);
-		if (!p->clk->parents) {
-			p->clk->num_parents = 0;
+		p->clkd->info.num_parents = st->num_returned + st->num_remaining;
+		p->clkd->info.parents = devm_kcalloc(p->dev,
+						     p->clkd->info.num_parents,
+						     sizeof(*p->clkd->info.parents),
+						     GFP_KERNEL);
+		if (!p->clkd->info.parents) {
+			p->clkd->info.num_parents = 0;
 			return -ENOMEM;
 		}
 		st->max_resources = st->num_returned + st->num_remaining;
@@ -280,29 +293,27 @@ static int iter_clk_possible_parents_process_response(const struct scmi_protocol
 	const struct scmi_msg_resp_clock_possible_parents *r = response;
 	struct scmi_clk_ipriv *p = priv;
 
-	u32 *parent = &p->clk->parents[st->desc_index + st->loop_idx];
+	u32 *parent = &p->clkd->info.parents[st->desc_index + st->loop_idx];
 
 	*parent = le32_to_cpu(r->possible_parents[st->loop_idx]);
 
 	return 0;
 }
 
-static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph, u32 clk_id,
-				       struct scmi_clock_info *clk)
+static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph,
+				       u32 clk_id, struct clock_info *cinfo)
 {
 	struct scmi_iterator_ops ops = {
 		.prepare_message = iter_clk_possible_parents_prepare_message,
 		.update_state = iter_clk_possible_parents_update_state,
 		.process_response = iter_clk_possible_parents_process_response,
 	};
-
+	struct scmi_clock_desc *clkd = &cinfo->clkds[clk_id];
 	struct scmi_clk_ipriv ppriv = {
-		.clk_id = clk_id,
-		.clk = clk,
+		.clkd = clkd,
 		.dev = ph->dev,
 	};
 	void *iter;
-	int ret;
 
 	iter = ph->hops->iter_response_init(ph, &ops, 0,
 					    CLOCK_POSSIBLE_PARENTS_GET,
@@ -311,9 +322,7 @@ static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph, u3
 	if (IS_ERR(iter))
 		return PTR_ERR(iter);
 
-	ret = ph->hops->iter_response_run(iter);
-
-	return ret;
+	return ph->hops->iter_response_run(iter);
 }
 
 static int
@@ -352,7 +361,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 	u32 attributes;
 	struct scmi_xfer *t;
 	struct scmi_msg_resp_clock_attributes *attr;
-	struct scmi_clock_info *clk = cinfo->clk + clk_id;
+	struct scmi_clock_info *clk = CLOCK_INFO(cinfo, clk_id);
 
 	ret = ph->xops->xfer_get_init(ph, CLOCK_ATTRIBUTES,
 				      sizeof(clk_id), sizeof(*attr), &t);
@@ -394,7 +403,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 			clk->rate_change_requested_notifications = true;
 		if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
 			if (SUPPORTS_PARENT_CLOCK(attributes))
-				scmi_clock_possible_parents(ph, clk_id, clk);
+				scmi_clock_possible_parents(ph, clk_id, cinfo);
 			if (SUPPORTS_GET_PERMISSIONS(attributes))
 				scmi_clock_get_permissions(ph, clk_id, clk);
 			if (SUPPORTS_EXTENDED_CONFIG(attributes))
@@ -424,7 +433,7 @@ static void iter_clk_describe_prepare_message(void *message,
 	struct scmi_msg_clock_describe_rates *msg = message;
 	const struct scmi_clk_ipriv *p = priv;
 
-	msg->id = cpu_to_le32(p->clk_id);
+	msg->id = cpu_to_le32(p->clkd->id);
 	/* Set the number of rates to be skipped/already read */
 	msg->rate_index = cpu_to_le32(desc_index);
 }
@@ -457,14 +466,14 @@ 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->clk->rate_discrete = RATE_DISCRETE(flags);
+	p->clkd->rate_discrete = RATE_DISCRETE(flags);
 
 	/* Warn about out of spec replies ... */
-	if (!p->clk->rate_discrete &&
+	if (!p->clkd->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",
-			 p->clk->name, st->num_returned, st->num_remaining,
+			 p->clkd->info.name, st->num_returned, st->num_remaining,
 			 st->rx_len);
 
 		SCMI_QUIRK(clock_rates_triplet_out_of_spec,
@@ -479,38 +488,19 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
 				   const void *response,
 				   struct scmi_iterator_state *st, void *priv)
 {
-	int ret = 0;
 	struct scmi_clk_ipriv *p = priv;
 	const struct scmi_msg_resp_clock_describe_rates *r = response;
 
-	if (!p->clk->rate_discrete) {
-		switch (st->desc_index + st->loop_idx) {
-		case 0:
-			p->clk->range.min_rate = RATE_TO_U64(r->rate[0]);
-			break;
-		case 1:
-			p->clk->range.max_rate = RATE_TO_U64(r->rate[1]);
-			break;
-		case 2:
-			p->clk->range.step_size = RATE_TO_U64(r->rate[2]);
-			break;
-		default:
-			ret = -EINVAL;
-			break;
-		}
-	} else {
-		u64 *rate = &p->clk->list.rates[st->desc_index + st->loop_idx];
+	p->clkd->rates[st->desc_index + st->loop_idx] =
+		RATE_TO_U64(r->rate[st->loop_idx]);
+	p->clkd->num_rates++;
 
-		*rate = RATE_TO_U64(r->rate[st->loop_idx]);
-		p->clk->list.num_rates++;
-	}
-
-	return ret;
+	return 0;
 }
 
 static int
 scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
-			      struct scmi_clock_info *clk)
+			      struct clock_info *cinfo)
 {
 	int ret;
 	void *iter;
@@ -519,9 +509,9 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 		.update_state = iter_clk_describe_update_state,
 		.process_response = iter_clk_describe_process_response,
 	};
+	struct scmi_clock_desc *clkd = &cinfo->clkds[clk_id];
 	struct scmi_clk_ipriv cpriv = {
-		.clk_id = clk_id,
-		.clk = clk,
+		.clkd = clkd,
 		.dev = ph->dev,
 	};
 
@@ -536,16 +526,23 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 	if (ret)
 		return ret;
 
-	if (!clk->rate_discrete) {
+	/* empty set ? */
+	if (!clkd->num_rates)
+		return 0;
+
+	if (!clkd->rate_discrete) {
+		clkd->info.max_rate = clkd->rates[RATE_MAX];
 		dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
-			clk->range.min_rate, clk->range.max_rate,
-			clk->range.step_size);
-	} else if (clk->list.num_rates) {
-		sort(clk->list.rates, clk->list.num_rates,
-		     sizeof(clk->list.rates[0]), rate_cmp_func, NULL);
+			clkd->rates[RATE_MIN], clkd->rates[RATE_MAX],
+			clkd->rates[RATE_STEP]);
+	} else {
+		sort(clkd->rates, clkd->num_rates,
+		     sizeof(clkd->rates[0]), rate_cmp_func, NULL);
+		clkd->info.max_rate = clkd->rates[clkd->num_rates - 1];
 	}
+	clkd->info.min_rate = clkd->rates[RATE_MIN];
 
-	return ret;
+	return 0;
 }
 
 static int
@@ -630,6 +627,7 @@ static int scmi_clock_determine_rate(const struct scmi_protocol_handle *ph,
 {
 	u64 fmin, fmax, ftmp;
 	struct scmi_clock_info *clk;
+	struct scmi_clock_desc *clkd;
 	struct clock_info *ci = ph->get_priv(ph);
 
 	if (!rate)
@@ -639,15 +637,17 @@ static int scmi_clock_determine_rate(const struct scmi_protocol_handle *ph,
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
+	clkd = to_desc(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)
+	if (clkd->rate_discrete)
 		return 0;
 
-	fmin = clk->range.min_rate;
-	fmax = clk->range.max_rate;
+	fmin = clk->min_rate;
+	fmax = clk->max_rate;
 	if (*rate <= fmin) {
 		*rate = fmin;
 		return 0;
@@ -657,10 +657,10 @@ static int scmi_clock_determine_rate(const struct scmi_protocol_handle *ph,
 	}
 
 	ftmp = *rate - fmin;
-	ftmp += clk->range.step_size - 1; /* to round up */
-	do_div(ftmp, clk->range.step_size);
+	ftmp += clkd->rates[RATE_STEP] - 1; /* to round up */
+	do_div(ftmp, clkd->rates[RATE_STEP]);
 
-	*rate = ftmp * clk->range.step_size + fmin;
+	*rate = ftmp * clkd->rates[RATE_STEP] + fmin;
 
 	return 0;
 }
@@ -1122,17 +1122,16 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
 	if (ret)
 		return ret;
 
-	cinfo->clk = devm_kcalloc(ph->dev, cinfo->num_clocks,
-				  sizeof(*cinfo->clk), GFP_KERNEL);
-	if (!cinfo->clk)
+	cinfo->clkds = devm_kcalloc(ph->dev, cinfo->num_clocks,
+				    sizeof(*cinfo->clkds), GFP_KERNEL);
+	if (!cinfo->clkds)
 		return -ENOMEM;
 
 	for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
-		struct scmi_clock_info *clk = cinfo->clk + clkid;
-
+		cinfo->clkds[clkid].id = clkid;
 		ret = scmi_clock_attributes_get(ph, clkid, cinfo);
 		if (!ret)
-			scmi_clock_describe_rates_get(ph, clkid, clk);
+			scmi_clock_describe_rates_get(ph, clkid, cinfo);
 	}
 
 	if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 28579c145045..7283302b0c85 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -51,6 +51,8 @@ struct scmi_clock_info {
 	bool rate_ctrl_forbidden;
 	bool parent_ctrl_forbidden;
 	bool extended_config;
+	u64 min_rate;
+	u64 max_rate;
 	union {
 		struct {
 			int num_rates;
-- 
2.53.0

[PATCH 04/11] clk: scmi: Use new simplified per-clock rate properties

[Cristian Marussi]

Use the new min_rate and max_rate unified properties that provide the
proper values without having to consider the clock type.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/clk/clk-scmi.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index c223e4ef1dd1..7c562559ad8b 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -202,7 +202,6 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
 			     const struct clk_ops *scmi_ops)
 {
 	int ret;
-	unsigned long min_rate, max_rate;
 
 	struct clk_init_data init = {
 		.flags = CLK_GET_RATE_NOCACHE,
@@ -217,20 +216,8 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
 	if (ret)
 		return ret;
 
-	if (sclk->info->rate_discrete) {
-		int num_rates = sclk->info->list.num_rates;
-
-		if (num_rates <= 0)
-			return -EINVAL;
-
-		min_rate = sclk->info->list.rates[0];
-		max_rate = sclk->info->list.rates[num_rates - 1];
-	} else {
-		min_rate = sclk->info->range.min_rate;
-		max_rate = sclk->info->range.max_rate;
-	}
-
-	clk_hw_set_rate_range(&sclk->hw, min_rate, max_rate);
+	clk_hw_set_rate_range(&sclk->hw, sclk->info->min_rate,
+			      sclk->info->max_rate);
 	return ret;
 }
 
-- 
2.53.0

[PATCH 05/11] firmware: arm_scmi: Drop unused clock rate interfaces

[Cristian Marussi]

Only the unified interface exposing min_rate/max_rate is now used.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 include/linux/scmi_protocol.h | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 7283302b0c85..d97b4e734744 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -53,17 +53,6 @@ struct scmi_clock_info {
 	bool extended_config;
 	u64 min_rate;
 	u64 max_rate;
-	union {
-		struct {
-			int num_rates;
-			u64 rates[SCMI_MAX_NUM_RATES];
-		} list;
-		struct {
-			u64 min_rate;
-			u64 max_rate;
-			u64 step_size;
-		} range;
-	};
 	int num_parents;
 	u32 *parents;
 };
-- 
2.53.0

[PATCH 06/11] firmware: arm_scmi: Make clock rates allocation dynamic

[Cristian Marussi]

Leveraging SCMI Clock protocol dynamic discovery capabilities, move away
from the static per-clock rates allocation model in favour of a dynamic
runtime allocation based on effectively discovered resources.

No functional change.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 19 ++++++++++++++++---
 include/linux/scmi_protocol.h     |  1 -
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index f5d1c608f85a..d0fb5affb5cf 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -161,7 +161,7 @@ struct scmi_clock_desc {
 	u32 id;
 	bool rate_discrete;
 	unsigned int num_rates;
-	u64 rates[SCMI_MAX_NUM_RATES];
+	u64 *rates;
 #define	RATE_MIN	0
 #define	RATE_MAX	1
 #define	RATE_STEP	2
@@ -480,6 +480,18 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,
 			   QUIRK_OUT_OF_SPEC_TRIPLET);
 	}
 
+	if (!st->max_resources) {
+		int num_rates = st->num_returned + st->num_remaining;
+
+		p->clkd->rates = devm_kcalloc(p->dev, num_rates,
+					      sizeof(*p->clkd->rates), GFP_KERNEL);
+		if (!p->clkd->rates)
+			return -ENOMEM;
+
+		/* max_resources is used by the iterators to control bounds */
+		st->max_resources = st->num_returned + st->num_remaining;
+	}
+
 	return 0;
 }
 
@@ -493,6 +505,8 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
 
 	p->clkd->rates[st->desc_index + st->loop_idx] =
 		RATE_TO_U64(r->rate[st->loop_idx]);
+
+	/* Count only effectively discovered rates */
 	p->clkd->num_rates++;
 
 	return 0;
@@ -515,8 +529,7 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 		.dev = ph->dev,
 	};
 
-	iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_RATES,
-					    CLOCK_DESCRIBE_RATES,
+	iter = ph->hops->iter_response_init(ph, &ops, 0, CLOCK_DESCRIBE_RATES,
 					    sizeof(struct scmi_msg_clock_describe_rates),
 					    &cpriv);
 	if (IS_ERR(iter))
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index d97b4e734744..5552ac04c820 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -15,7 +15,6 @@
 
 #define SCMI_MAX_STR_SIZE		64
 #define SCMI_SHORT_NAME_MAX_SIZE	16
-#define SCMI_MAX_NUM_RATES		16
 
 /**
  * struct scmi_revision_info - version information structure
-- 
2.53.0

[PATCH 07/11] firmware: arm_scmi: Harden clock parents discovery

[Cristian Marussi]

Fix clock parents enumeration to account only for effectively discovered
parents during enumeration, avoiding to trust the total number of parents
declared upfront by the platform.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index d0fb5affb5cf..15faa79abed4 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -270,15 +270,15 @@ static int iter_clk_possible_parents_update_state(struct scmi_iterator_state *st
 	 * assume it's returned+remaining on first call.
 	 */
 	if (!st->max_resources) {
-		p->clkd->info.num_parents = st->num_returned + st->num_remaining;
-		p->clkd->info.parents = devm_kcalloc(p->dev,
-						     p->clkd->info.num_parents,
+		int num_parents = st->num_returned + st->num_remaining;
+
+		p->clkd->info.parents = devm_kcalloc(p->dev, num_parents,
 						     sizeof(*p->clkd->info.parents),
 						     GFP_KERNEL);
-		if (!p->clkd->info.parents) {
-			p->clkd->info.num_parents = 0;
+		if (!p->clkd->info.parents)
 			return -ENOMEM;
-		}
+
+		/* max_resources is used by the iterators to control bounds */
 		st->max_resources = st->num_returned + st->num_remaining;
 	}
 
@@ -293,9 +293,11 @@ static int iter_clk_possible_parents_process_response(const struct scmi_protocol
 	const struct scmi_msg_resp_clock_possible_parents *r = response;
 	struct scmi_clk_ipriv *p = priv;
 
-	u32 *parent = &p->clkd->info.parents[st->desc_index + st->loop_idx];
+	p->clkd->info.parents[st->desc_index + st->loop_idx] =
+		le32_to_cpu(r->possible_parents[st->loop_idx]);
 
-	*parent = le32_to_cpu(r->possible_parents[st->loop_idx]);
+	/* Count only effectively discovered parents */
+	p->clkd->info.num_parents++;
 
 	return 0;
 }
-- 
2.53.0

[PATCH 08/11] firmware: arm_scmi: Refactor iterators internal allocation

[Cristian Marussi]

Use cleanup handlers to manage iterator data structures.

No functional change.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 35 +++++++++++++++---------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index baefa7d43c00..d9d6edbc1275 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -17,6 +17,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/bitmap.h>
+#include <linux/cleanup.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/export.h>
@@ -1789,39 +1790,41 @@ static void *scmi_iterator_init(const struct scmi_protocol_handle *ph,
 				size_t tx_size, void *priv)
 {
 	int ret;
-	struct scmi_iterator *i;
 
-	i = devm_kzalloc(ph->dev, sizeof(*i), GFP_KERNEL);
+	struct scmi_iterator *i __free(kfree) = kzalloc(sizeof(*i), GFP_KERNEL);
 	if (!i)
 		return ERR_PTR(-ENOMEM);
 
+	if (!ops || !ph)
+		return ERR_PTR(-EINVAL);
+
 	i->ph = ph;
 	i->ops = ops;
 	i->priv = priv;
 
 	ret = ph->xops->xfer_get_init(ph, msg_id, tx_size, 0, &i->t);
-	if (ret) {
-		devm_kfree(ph->dev, i);
+	if (ret)
 		return ERR_PTR(ret);
-	}
 
 	i->state.max_resources = max_resources;
 	i->msg = i->t->tx.buf;
 	i->resp = i->t->rx.buf;
 
-	return i;
+	return no_free_ptr(i);
 }
 
 static int scmi_iterator_run(void *iter)
 {
-	int ret = -EINVAL;
+	int ret;
 	struct scmi_iterator_ops *iops;
 	const struct scmi_protocol_handle *ph;
 	struct scmi_iterator_state *st;
-	struct scmi_iterator *i = iter;
 
-	if (!i || !i->ops || !i->ph)
-		return ret;
+	if (!iter)
+		return -EINVAL;
+
+	/* Take ownership of the iterator */
+	struct scmi_iterator *i __free(kfree) = iter;
 
 	iops = i->ops;
 	ph = i->ph;
@@ -1846,12 +1849,12 @@ static int scmi_iterator_run(void *iter)
 			break;
 		}
 
-		for (st->loop_idx = 0; st->loop_idx < st->num_returned;
-		     st->loop_idx++) {
+		for (st->loop_idx = 0; !ret && st->loop_idx < st->num_returned;
+		     st->loop_idx++)
 			ret = iops->process_response(ph, i->resp, st, i->priv);
-			if (ret)
-				goto out;
-		}
+
+		if (ret)
+			break;
 
 		st->desc_index += st->num_returned;
 		ph->xops->reset_rx_to_maxsz(ph, i->t);
@@ -1861,10 +1864,8 @@ static int scmi_iterator_run(void *iter)
 		 */
 	} while (st->num_returned && st->num_remaining);
 
-out:
 	/* Finalize and destroy iterator */
 	ph->xops->xfer_put(ph, i->t);
-	devm_kfree(ph->dev, i);
 
 	return ret;
 }
-- 
2.53.0

[PATCH 09/11] firmware: arm_scmi: Add bound iterators support

[Cristian Marussi]

SCMI core stack provides some common helpers to handle in a unified way
multipart message replies: such iterator-helpers, when run, currently
process by default the whole set of discovered resources.

Introduce an alternative way to run the initialized iterator on a limited
range of resources.

Note that the subset of resources that can be chosen is anyway limited by
the SCMI protocol specification, since you are only allowed to choose the
startindex on a multi-part enumeration NOT the end index, so that the
effective number of returned items by a bound iterators depends really
on platform side decisions.

Suggested-by: Etienne Carriere <etienne.carriere@foss.st.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c     |  3 +-
 drivers/firmware/arm_scmi/driver.c    | 58 +++++++++++++++++++--------
 drivers/firmware/arm_scmi/protocols.h | 13 +++++-
 3 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 15faa79abed4..d7df5c45836e 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -505,8 +505,7 @@ 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[st->desc_index + st->loop_idx] =
-		RATE_TO_U64(r->rate[st->loop_idx]);
+	p->clkd->rates[p->clkd->num_rates] = RATE_TO_U64(r->rate[st->loop_idx]);
 
 	/* Count only effectively discovered rates */
 	p->clkd->num_rates++;
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index d9d6edbc1275..299265d05f62 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1813,48 +1813,50 @@ static void *scmi_iterator_init(const struct scmi_protocol_handle *ph,
 	return no_free_ptr(i);
 }
 
-static int scmi_iterator_run(void *iter)
+static int __scmi_iterator_run(void *iter, unsigned int *start, unsigned int *end)
 {
 	int ret;
 	struct scmi_iterator_ops *iops;
 	const struct scmi_protocol_handle *ph;
 	struct scmi_iterator_state *st;
+	struct scmi_iterator *i;
 
 	if (!iter)
 		return -EINVAL;
 
-	/* Take ownership of the iterator */
-	struct scmi_iterator *i __free(kfree) = iter;
-
+	i = iter;
 	iops = i->ops;
 	ph = i->ph;
 	st = &i->state;
 
+	/* Reinitialize state for next run */
+	st->num_returned = 0;
+	st->num_remaining = 0;
+	st->desc_index = start ? *start : 0;
+
 	do {
 		iops->prepare_message(i->msg, st->desc_index, i->priv);
 		ret = ph->xops->do_xfer(ph, i->t);
 		if (ret)
-			break;
+			return ret;
 
 		st->rx_len = i->t->rx.len;
 		ret = iops->update_state(st, i->resp, i->priv);
 		if (ret)
-			break;
+			return ret;
 
 		if (st->num_returned > st->max_resources - st->desc_index) {
 			dev_err(ph->dev,
 				"No. of resources can't exceed %d\n",
 				st->max_resources);
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 
-		for (st->loop_idx = 0; !ret && st->loop_idx < st->num_returned;
-		     st->loop_idx++)
+		for (st->loop_idx = 0; st->loop_idx < st->num_returned; st->loop_idx++) {
 			ret = iops->process_response(ph, i->resp, st, i->priv);
-
-		if (ret)
-			break;
+			if (ret)
+				return ret;
+		}
 
 		st->desc_index += st->num_returned;
 		ph->xops->reset_rx_to_maxsz(ph, i->t);
@@ -1862,14 +1864,36 @@ static int scmi_iterator_run(void *iter)
 		 * check for both returned and remaining to avoid infinite
 		 * loop due to buggy firmware
 		 */
-	} while (st->num_returned && st->num_remaining);
+	} while (st->num_returned && st->num_remaining &&
+		 (!end || (st->desc_index <= min(*end, st->max_resources - 1))));
 
-	/* Finalize and destroy iterator */
-	ph->xops->xfer_put(ph, i->t);
+	return 0;
+}
+
+static void scmi_iterator_cleanup(void *iter)
+{
+	struct scmi_iterator *i = iter;
+
+	i->ph->xops->xfer_put(i->ph, i->t);
+	kfree(i);
+}
+
+static int scmi_iterator_run(void *iter)
+{
+	int ret;
+
+	ret = __scmi_iterator_run(iter, NULL, NULL);
+	scmi_iterator_cleanup(iter);
 
 	return ret;
 }
 
+static int scmi_iterator_run_bound(void *iter, unsigned int *start,
+				   unsigned int *end)
+{
+	return __scmi_iterator_run(iter, start, end);
+}
+
 struct scmi_msg_get_fc_info {
 	__le32 domain;
 	__le32 message_id;
@@ -2048,6 +2072,8 @@ static const struct scmi_proto_helpers_ops helpers_ops = {
 	.get_max_msg_size = scmi_common_get_max_msg_size,
 	.iter_response_init = scmi_iterator_init,
 	.iter_response_run = scmi_iterator_run,
+	.iter_response_run_bound = scmi_iterator_run_bound,
+	.iter_response_cleanup = scmi_iterator_cleanup,
 	.protocol_msg_check = scmi_protocol_msg_check,
 	.fastchannel_init = scmi_common_fastchannel_init,
 	.fastchannel_db_ring = scmi_common_fastchannel_db_ring,
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 4c75970326e6..487f84239385 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -259,7 +259,15 @@ struct scmi_fc_info {
  *			multi-part responses using the custom operations
  *			provided in @ops.
  * @iter_response_run: A common helper to trigger the run of a previously
- *		       initialized iterator.
+ *		       initialized iterator. Note that unbound iterators are
+ *		       automatically cleaned up.
+ * @iter_response_run_bound: A common helper to trigger the run of a previously
+ *			     initialized iterator, but only within the
+ *			     specified, optional, @start and @end resource
+ *			     indexes. Note that these bound-iterators need
+ *			     explicit cleanup via @iter_response_bound_cleanup.
+ * @iter_response_bound_cleanup: A common helper to finally release the iterator
+ *				 for bound iterators.
  * @protocol_msg_check: A common helper to check is a specific protocol message
  *			is supported.
  * @fastchannel_init: A common helper used to initialize FC descriptors by
@@ -276,6 +284,9 @@ struct scmi_proto_helpers_ops {
 				    unsigned int max_resources, u8 msg_id,
 				    size_t tx_size, void *priv);
 	int (*iter_response_run)(void *iter);
+	int (*iter_response_run_bound)(void *iter,
+				       unsigned int *start, unsigned int *end);
+	void (*iter_response_cleanup)(void *iter);
 	int (*protocol_msg_check)(const struct scmi_protocol_handle *ph,
 				  u32 message_id, u32 *attributes);
 	void (*fastchannel_init)(const struct scmi_protocol_handle *ph,
-- 
2.53.0

[PATCH 10/11] firmware: arm_scmi: Use bound iterators to minimize discovered rates

[Cristian Marussi]

Clock rates are guaranteed to be returned in ascending order for SCMI clock
protocol versions greater than 1.0: in such a case, use bounded iterators
to minimize the number of message exchanges needed to discover min and max
rate.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 92 +++++++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index d7df5c45836e..a0de10652abe 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -160,6 +160,7 @@ 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;
 #define	RATE_MIN	0
@@ -483,15 +484,16 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,
 	}
 
 	if (!st->max_resources) {
-		int num_rates = st->num_returned + st->num_remaining;
+		unsigned int tot_rates = st->num_returned + st->num_remaining;
 
-		p->clkd->rates = devm_kcalloc(p->dev, num_rates,
+		p->clkd->rates = devm_kcalloc(p->dev, tot_rates,
 					      sizeof(*p->clkd->rates), GFP_KERNEL);
 		if (!p->clkd->rates)
 			return -ENOMEM;
 
 		/* max_resources is used by the iterators to control bounds */
-		st->max_resources = st->num_returned + st->num_remaining;
+		p->clkd->tot_rates = tot_rates;
+		st->max_resources = tot_rates;
 	}
 
 	return 0;
@@ -514,8 +516,8 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
 }
 
 static int
-scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
-			      struct clock_info *cinfo)
+scmi_clock_describe_rates_get_full(const struct scmi_protocol_handle *ph,
+				   struct scmi_clock_desc *clkd)
 {
 	int ret;
 	void *iter;
@@ -524,7 +526,6 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 		.update_state = iter_clk_describe_update_state,
 		.process_response = iter_clk_describe_process_response,
 	};
-	struct scmi_clock_desc *clkd = &cinfo->clkds[clk_id];
 	struct scmi_clk_ipriv cpriv = {
 		.clkd = clkd,
 		.dev = ph->dev,
@@ -544,19 +545,90 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 	if (!clkd->num_rates)
 		return 0;
 
+	if (clkd->rate_discrete)
+		sort(clkd->rates, clkd->num_rates,
+		     sizeof(clkd->rates[0]), rate_cmp_func, NULL);
+
+	return 0;
+}
+
+static int
+scmi_clock_describe_rates_get_lazy(const struct scmi_protocol_handle *ph,
+				   struct scmi_clock_desc *clkd)
+{
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_clk_describe_prepare_message,
+		.update_state = iter_clk_describe_update_state,
+		.process_response = iter_clk_describe_process_response,
+	};
+	struct scmi_clk_ipriv cpriv = {
+		.clkd = clkd,
+		.dev = ph->dev,
+	};
+	unsigned int first, last;
+	void *iter;
+	int ret;
+
+	iter = ph->hops->iter_response_init(ph, &ops, 0, CLOCK_DESCRIBE_RATES,
+					    sizeof(struct scmi_msg_clock_describe_rates),
+					    &cpriv);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	/* Try to grab a triplet, so that in case is NON-discrete we are done */
+	first = 0;
+	last = 2;
+	ret = ph->hops->iter_response_run_bound(iter, &first, &last);
+	if (ret)
+		goto out;
+
+	/* If discrete grab the last value, which should be the max */
+	if (clkd->rate_discrete && clkd->tot_rates > 3) {
+		first = clkd->tot_rates - 1;
+		last = clkd->tot_rates - 1;
+		ret = ph->hops->iter_response_run_bound(iter, &first, &last);
+	}
+
+out:
+	ph->hops->iter_response_cleanup(iter);
+
+	return ret;
+}
+
+static int
+scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph,
+			      u32 clk_id, struct clock_info *cinfo)
+{
+	struct scmi_clock_desc *clkd = &cinfo->clkds[clk_id];
+	int ret;
+
+	/*
+	 * Since only after SCMI Clock v1.0 the returned rates are guaranteed to
+	 * be discovered in ascending order, lazy enumeration cannot be use for
+	 * SCMI Clock v1.0 protocol.
+	 */
+	if (PROTOCOL_REV_MAJOR(ph->version) > 0x1)
+		ret = scmi_clock_describe_rates_get_lazy(ph, clkd);
+	else
+		ret = scmi_clock_describe_rates_get_full(ph, clkd);
+
+	if (ret)
+		return ret;
+
+	clkd->info.min_rate = clkd->rates[RATE_MIN];
 	if (!clkd->rate_discrete) {
 		clkd->info.max_rate = clkd->rates[RATE_MAX];
 		dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
 			clkd->rates[RATE_MIN], clkd->rates[RATE_MAX],
 			clkd->rates[RATE_STEP]);
 	} else {
-		sort(clkd->rates, clkd->num_rates,
-		     sizeof(clkd->rates[0]), rate_cmp_func, NULL);
 		clkd->info.max_rate = clkd->rates[clkd->num_rates - 1];
+		dev_dbg(ph->dev, "Clock:%s DISCRETE:%d -> Min %llu Max %llu\n",
+			clkd->info.name, clkd->rate_discrete,
+			clkd->info.min_rate, clkd->info.max_rate);
 	}
-	clkd->info.min_rate = clkd->rates[RATE_MIN];
 
-	return 0;
+	return ret;
 }
 
 static int
-- 
2.53.0

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

[Cristian Marussi]

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)
+		sort(clkd->r.rates, clkd->r.num_rates,
+		     sizeof(clkd->r.rates[0]), rate_cmp_func, NULL);
 
 	return 0;
 }
@@ -583,7 +587,7 @@ scmi_clock_describe_rates_get_lazy(const struct scmi_protocol_handle *ph,
 		goto out;
 
 	/* If discrete grab the last value, which should be the max */
-	if (clkd->rate_discrete && clkd->tot_rates > 3) {
+	if (clkd->r.rate_discrete && clkd->tot_rates > 3) {
 		first = clkd->tot_rates - 1;
 		last = clkd->tot_rates - 1;
 		ret = ph->hops->iter_response_run_bound(iter, &first, &last);
@@ -615,16 +619,16 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph,
 	if (ret)
 		return ret;
 
-	clkd->info.min_rate = clkd->rates[RATE_MIN];
-	if (!clkd->rate_discrete) {
-		clkd->info.max_rate = clkd->rates[RATE_MAX];
+	clkd->info.min_rate = clkd->r.rates[RATE_MIN];
+	if (!clkd->r.rate_discrete) {
+		clkd->info.max_rate = clkd->r.rates[RATE_MAX];
 		dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
-			clkd->rates[RATE_MIN], clkd->rates[RATE_MAX],
-			clkd->rates[RATE_STEP]);
+			clkd->r.rates[RATE_MIN], clkd->r.rates[RATE_MAX],
+			clkd->r.rates[RATE_STEP]);
 	} else {
-		clkd->info.max_rate = clkd->rates[clkd->num_rates - 1];
+		clkd->info.max_rate = clkd->r.rates[clkd->r.num_rates - 1];
 		dev_dbg(ph->dev, "Clock:%s DISCRETE:%d -> Min %llu Max %llu\n",
-			clkd->info.name, clkd->rate_discrete,
+			clkd->info.name, clkd->r.rate_discrete,
 			clkd->info.min_rate, clkd->info.max_rate);
 	}
 
@@ -729,7 +733,7 @@ static int scmi_clock_determine_rate(const struct scmi_protocol_handle *ph,
 	 * If we can't figure out what rate it will be, so just return the
 	 * rate back to the caller.
 	 */
-	if (clkd->rate_discrete)
+	if (clkd->r.rate_discrete)
 		return 0;
 
 	fmin = clk->min_rate;
@@ -743,14 +747,40 @@ static int scmi_clock_determine_rate(const struct scmi_protocol_handle *ph,
 	}
 
 	ftmp = *rate - fmin;
-	ftmp += clkd->rates[RATE_STEP] - 1; /* to round up */
-	do_div(ftmp, clkd->rates[RATE_STEP]);
+	ftmp += clkd->r.rates[RATE_STEP] - 1; /* to round up */
+	do_div(ftmp, clkd->r.rates[RATE_STEP]);
 
-	*rate = ftmp * clkd->rates[RATE_STEP] + fmin;
+	*rate = ftmp * clkd->r.rates[RATE_STEP] + fmin;
 
 	return 0;
 }
 
+static const struct scmi_clock_rates *
+scmi_clock_all_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id)
+{
+	struct clock_info *ci = ph->get_priv(ph);
+	struct scmi_clock_desc *clkd;
+	struct scmi_clock_info *clk;
+
+	clk = scmi_clock_domain_lookup(ci, clk_id);
+	if (IS_ERR(clk) || !clk->name[0])
+		return NULL;
+
+	clkd = to_desc(clk);
+	/* Needs full enumeration ? */
+	if (clkd->r.rate_discrete && clkd->tot_rates != clkd->r.num_rates) {
+		int ret;
+
+		/* rates[] is already allocated BUT we need to re-enumerate */
+		clkd->r.num_rates = 0;
+		ret = scmi_clock_describe_rates_get_full(ph, clkd);
+		if (ret)
+			return NULL;
+	}
+
+	return &clkd->r;
+}
+
 static int
 scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id,
 		      enum clk_state state,
@@ -1064,6 +1094,7 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
 	.rate_get = scmi_clock_rate_get,
 	.rate_set = scmi_clock_rate_set,
 	.determine_rate = scmi_clock_determine_rate,
+	.all_rates_get = scmi_clock_all_rates_get,
 	.enable = scmi_clock_enable,
 	.disable = scmi_clock_disable,
 	.state_get = scmi_clock_state_get,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 5552ac04c820..c710107c2120 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -40,6 +40,12 @@ struct scmi_revision_info {
 	char sub_vendor_id[SCMI_SHORT_NAME_MAX_SIZE];
 };
 
+struct scmi_clock_rates {
+	bool rate_discrete;
+	unsigned int num_rates;
+	u64 *rates;
+};
+
 struct scmi_clock_info {
 	char name[SCMI_MAX_STR_SIZE];
 	unsigned int enable_latency;
@@ -85,6 +91,7 @@ enum scmi_clock_oem_config {
  *		    clock calculating the closest allowed rate.
  *		    Note that @rate is an input/output parameter used both to
  *		    describe the requested rate and report the closest match
+ * @all_rates_get: get the list of all available rates for the specified clock.
  * @enable: enables the specified clock
  * @disable: disables the specified clock
  * @state_get: get the status of the specified clock
@@ -104,6 +111,8 @@ struct scmi_clk_proto_ops {
 			u64 rate);
 	int (*determine_rate)(const struct scmi_protocol_handle *ph, u32 clk_id,
 			      unsigned long *rate);
+	const struct scmi_clock_rates __must_check *(*all_rates_get)
+		(const struct scmi_protocol_handle *ph, u32 clk_id);
 	int (*enable)(const struct scmi_protocol_handle *ph, u32 clk_id,
 		      bool atomic);
 	int (*disable)(const struct scmi_protocol_handle *ph, u32 clk_id,
-- 
2.53.0

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

[Jonathan Cameron]

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

Re: [PATCH 10/11] firmware: arm_scmi: Use bound iterators to minimize discovered rates

[Jonathan Cameron]

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

> Clock rates are guaranteed to be returned in ascending order for SCMI clock
> protocol versions greater than 1.0: in such a case, use bounded iterators
> to minimize the number of message exchanges needed to discover min and max
> rate.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

> +
> +static int
> +scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph,
> +			      u32 clk_id, struct clock_info *cinfo)
> +{
> +	struct scmi_clock_desc *clkd = &cinfo->clkds[clk_id];
> +	int ret;
> +
> +	/*
> +	 * Since only after SCMI Clock v1.0 the returned rates are guaranteed to
> +	 * be discovered in ascending order, lazy enumeration cannot be use for
> +	 * SCMI Clock v1.0 protocol.
> +	 */
> +	if (PROTOCOL_REV_MAJOR(ph->version) > 0x1)
> +		ret = scmi_clock_describe_rates_get_lazy(ph, clkd);
> +	else
> +		ret = scmi_clock_describe_rates_get_full(ph, clkd);
> +
> +	if (ret)
> +		return ret;
> +
> +	clkd->info.min_rate = clkd->rates[RATE_MIN];
>  	if (!clkd->rate_discrete) {
>  		clkd->info.max_rate = clkd->rates[RATE_MAX];
>  		dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
>  			clkd->rates[RATE_MIN], clkd->rates[RATE_MAX],
>  			clkd->rates[RATE_STEP]);
>  	} else {
> -		sort(clkd->rates, clkd->num_rates,
> -		     sizeof(clkd->rates[0]), rate_cmp_func, NULL);
>  		clkd->info.max_rate = clkd->rates[clkd->num_rates - 1];
> +		dev_dbg(ph->dev, "Clock:%s DISCRETE:%d -> Min %llu Max %llu\n",
> +			clkd->info.name, clkd->rate_discrete,
> +			clkd->info.min_rate, clkd->info.max_rate);
>  	}
> -	clkd->info.min_rate = clkd->rates[RATE_MIN];
>  
> -	return 0;
> +	return ret;
Why?  Far as I can see it's still always zero if you get here.

>  }
>  
>  static int

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

[Peng Fan]

Hi Cristian,

On Fri, Feb 27, 2026 at 03:32:15PM +0000, Cristian Marussi 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>
>---
>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

Is there any early reviewing version available?

Thanks,
Peng

>---
> 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) {
>+		*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;
>+}
>+
> static int
> scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id,
> 		      enum clk_state state,
>@@ -936,6 +977,7 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
> 	.info_get = scmi_clock_info_get,
> 	.rate_get = scmi_clock_rate_get,
> 	.rate_set = scmi_clock_rate_set,
>+	.determine_rate = scmi_clock_determine_rate,
> 	.enable = scmi_clock_enable,
> 	.disable = scmi_clock_disable,
> 	.state_get = scmi_clock_state_get,
>diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
>index aafaac1496b0..28579c145045 100644
>--- a/include/linux/scmi_protocol.h
>+++ b/include/linux/scmi_protocol.h
>@@ -91,6 +91,10 @@ enum scmi_clock_oem_config {
>  * @info_get: get the information of the specified clock
>  * @rate_get: request the current clock rate of a clock
>  * @rate_set: set the clock rate of a clock
>+ * @determine_rate: determine the effective rate that can be supported by a
>+ *		    clock calculating the closest allowed rate.
>+ *		    Note that @rate is an input/output parameter used both to
>+ *		    describe the requested rate and report the closest match
>  * @enable: enables the specified clock
>  * @disable: disables the specified clock
>  * @state_get: get the status of the specified clock
>@@ -108,6 +112,8 @@ struct scmi_clk_proto_ops {
> 			u64 *rate);
> 	int (*rate_set)(const struct scmi_protocol_handle *ph, u32 clk_id,
> 			u64 rate);
>+	int (*determine_rate)(const struct scmi_protocol_handle *ph, u32 clk_id,
>+			      unsigned long *rate);
> 	int (*enable)(const struct scmi_protocol_handle *ph, u32 clk_id,
> 		      bool atomic);
> 	int (*disable)(const struct scmi_protocol_handle *ph, u32 clk_id,
>-- 
>2.53.0
>

Re: [PATCH 02/11] clk: scmi: Use new determine_rate clock operation

[Peng Fan]

On Fri, Feb 27, 2026 at 03:32:16PM +0000, Cristian Marussi wrote:
>Use the Clock protocol layer determine_rate logic to calculate the closest
>rate that can be supported by a specific clock.
>
>No functional change.
>
>Cc: Brian Masney <bmasney@redhat.com>
>Cc: Michael Turquette <mturquette@baylibre.com>
>Cc: Stephen Boyd <sboyd@kernel.org>
>Cc: linux-clk@vger.kernel.org
>Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>---
>Note that the calculation logic in the protocol layer is exactly the same
>as it wes here.
>
>@Brian I suppose once your CLK_ROUNDING_FW_MANAGED sereis is merged I can flag
>such SCMI clocks.

Per my reading of Brain's thread, if ->determine_rate exists,
->determine_rate() will be used.

 	} else if (core->ops->determine_rate) {
 		return core->ops->determine_rate(core->hw, req);
+	} else if (clk_is_rounding_fw_managed(core)) {
+		return 0;

So unless update scmi_clk_determine_rate() to something:
--------
if (clk & CLK_ROUNDING_FW_MANAGED)
	return 0;

return scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);
--------

It maybe better to update Brain's patch to move clk_is_rounding_fw_managed()
above the check of core->ops->determine_rate().

>---
> drivers/clk/clk-scmi.c | 31 ++++++-------------------------
> 1 file changed, 6 insertions(+), 25 deletions(-)
>
>diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
>index 6b286ea6f121..c223e4ef1dd1 100644
>--- a/drivers/clk/clk-scmi.c
>+++ b/drivers/clk/clk-scmi.c
>@@ -12,7 +12,6 @@
> #include <linux/of.h>
> #include <linux/module.h>
> #include <linux/scmi_protocol.h>
>-#include <asm/div64.h>
> 
> #define NOT_ATOMIC	false
> #define ATOMIC		true
>@@ -57,35 +56,17 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
> static int scmi_clk_determine_rate(struct clk_hw *hw,
> 				   struct clk_rate_request *req)
> {
>-	u64 fmin, fmax, ftmp;
>+	int ret;
> 	struct scmi_clk *clk = to_scmi_clk(hw);
> 
> 	/*
>-	 * We can't figure out what rate it will be, so just return the
>-	 * rate back to the caller. scmi_clk_recalc_rate() will be called
>-	 * after the rate is set and we'll know what rate the clock is
>+	 * If we could not get a better rate scmi_clk_recalc_rate() will be
>+	 * called after the rate is set and we'll know what rate the clock is
> 	 * running at then.
> 	 */
>-	if (clk->info->rate_discrete)
>-		return 0;
>-
>-	fmin = clk->info->range.min_rate;
>-	fmax = clk->info->range.max_rate;
>-	if (req->rate <= fmin) {
>-		req->rate = fmin;
>-
>-		return 0;
>-	} else if (req->rate >= fmax) {
>-		req->rate = fmax;
>-
>-		return 0;
>-	}
>-
>-	ftmp = req->rate - fmin;
>-	ftmp += clk->info->range.step_size - 1; /* to round up */
>-	do_div(ftmp, clk->info->range.step_size);
>-
>-	req->rate = ftmp * clk->info->range.step_size + fmin;
>+	ret = scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);
>+	if (ret)
>+		return ret;

nit:
"return scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);"

Otherwise:
Reviewed-by: Peng Fan <peng.fan@nxp.com>

> 
> 	return 0;
> }
>-- 
>2.53.0
>

Re: [PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface

[Peng Fan]

On Fri, Feb 27, 2026 at 03:32:17PM +0000, Cristian Marussi wrote:
>Move needlessly exposed fields away from scmi_clock_info into the new
>internal struct scmi_clock_desc while keeping exposed only the two new
>min_rate and max_rate fields for each clock.
>
>No functional change.
>
>Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>---
> drivers/firmware/arm_scmi/clock.c | 145 +++++++++++++++---------------
> include/linux/scmi_protocol.h     |   2 +
> 2 files changed, 74 insertions(+), 73 deletions(-)
>
>diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
>index 54e8b59c3941..f5d1c608f85a 100644
>--- a/drivers/firmware/arm_scmi/clock.c
>+++ b/drivers/firmware/arm_scmi/clock.c
>@@ -157,13 +157,27 @@ struct scmi_clock_rate_notify_payld {
> 	__le32 rate_high;
> };
> 
>+struct scmi_clock_desc {
>+	u32 id;
>+	bool rate_discrete;
>+	unsigned int num_rates;
>+	u64 rates[SCMI_MAX_NUM_RATES];
>+#define	RATE_MIN	0
>+#define	RATE_MAX	1
>+#define	RATE_STEP	2
>+	struct scmi_clock_info info;
>+};
>+
>+#define to_desc(p)	(container_of((p), struct scmi_clock_desc, info))

Nit:
no need parentheses

>+
> struct clock_info {
> 	int num_clocks;
> 	int max_async_req;
> 	bool notify_rate_changed_cmd;
> 	bool notify_rate_change_requested_cmd;
> 	atomic_t cur_async_req;
>-	struct scmi_clock_info *clk;
>+	struct scmi_clock_desc *clkds;
>+#define CLOCK_INFO(c, i)	(&(((c)->clkds + (i))->info))

Ditto.

> 	int (*clock_config_set)(const struct scmi_protocol_handle *ph,
> 				u32 clk_id, enum clk_state state,
> 				enum scmi_clock_oem_config oem_type,
>@@ -185,7 +199,7 @@ scmi_clock_domain_lookup(struct clock_info *ci, u32 clk_id)
> 	if (clk_id >= ci->num_clocks)
> 		return ERR_PTR(-EINVAL);
> 
>-	return ci->clk + clk_id;
>+	return CLOCK_INFO(ci, clk_id);
> }
> 
> static int
>@@ -226,8 +240,7 @@ scmi_clock_protocol_attributes_get(const struct scmi_protocol_handle *ph,
> 
> struct scmi_clk_ipriv {
> 	struct device *dev;
>-	u32 clk_id;
>-	struct scmi_clock_info *clk;
>+	struct scmi_clock_desc *clkd;
> };
> 
> static void iter_clk_possible_parents_prepare_message(void *message, unsigned int desc_index,
>@@ -236,7 +249,7 @@ static void iter_clk_possible_parents_prepare_message(void *message, unsigned in
> 	struct scmi_msg_clock_possible_parents *msg = message;
> 	const struct scmi_clk_ipriv *p = priv;
> 
>-	msg->id = cpu_to_le32(p->clk_id);
>+	msg->id = cpu_to_le32(p->clkd->id);
> 	/* Set the number of OPPs to be skipped/already read */
> 	msg->skip_parents = cpu_to_le32(desc_index);
> }
>@@ -246,7 +259,6 @@ static int iter_clk_possible_parents_update_state(struct scmi_iterator_state *st
> {
> 	const struct scmi_msg_resp_clock_possible_parents *r = response;
> 	struct scmi_clk_ipriv *p = priv;
>-	struct device *dev = ((struct scmi_clk_ipriv *)p)->dev;
> 	u32 flags;
> 
> 	flags = le32_to_cpu(r->num_parent_flags);
>@@ -258,12 +270,13 @@ static int iter_clk_possible_parents_update_state(struct scmi_iterator_state *st
> 	 * assume it's returned+remaining on first call.
> 	 */
> 	if (!st->max_resources) {
>-		p->clk->num_parents = st->num_returned + st->num_remaining;
>-		p->clk->parents = devm_kcalloc(dev, p->clk->num_parents,
>-					       sizeof(*p->clk->parents),
>-					       GFP_KERNEL);
>-		if (!p->clk->parents) {
>-			p->clk->num_parents = 0;
>+		p->clkd->info.num_parents = st->num_returned + st->num_remaining;
>+		p->clkd->info.parents = devm_kcalloc(p->dev,
>+						     p->clkd->info.num_parents,
>+						     sizeof(*p->clkd->info.parents),
>+						     GFP_KERNEL);
>+		if (!p->clkd->info.parents) {
>+			p->clkd->info.num_parents = 0;
> 			return -ENOMEM;
> 		}
> 		st->max_resources = st->num_returned + st->num_remaining;
>@@ -280,29 +293,27 @@ static int iter_clk_possible_parents_process_response(const struct scmi_protocol
> 	const struct scmi_msg_resp_clock_possible_parents *r = response;
> 	struct scmi_clk_ipriv *p = priv;
> 
>-	u32 *parent = &p->clk->parents[st->desc_index + st->loop_idx];
>+	u32 *parent = &p->clkd->info.parents[st->desc_index + st->loop_idx];
> 
> 	*parent = le32_to_cpu(r->possible_parents[st->loop_idx]);
> 
> 	return 0;
> }
> 
>-static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph, u32 clk_id,
>-				       struct scmi_clock_info *clk)
>+static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph,
>+				       u32 clk_id, struct clock_info *cinfo)
> {
> 	struct scmi_iterator_ops ops = {
> 		.prepare_message = iter_clk_possible_parents_prepare_message,
> 		.update_state = iter_clk_possible_parents_update_state,
> 		.process_response = iter_clk_possible_parents_process_response,
> 	};
>-
>+	struct scmi_clock_desc *clkd = &cinfo->clkds[clk_id];
> 	struct scmi_clk_ipriv ppriv = {
>-		.clk_id = clk_id,
>-		.clk = clk,
>+		.clkd = clkd,
> 		.dev = ph->dev,
> 	};
> 	void *iter;
>-	int ret;
> 
> 	iter = ph->hops->iter_response_init(ph, &ops, 0,
> 					    CLOCK_POSSIBLE_PARENTS_GET,
>@@ -311,9 +322,7 @@ static int scmi_clock_possible_parents(const struct scmi_protocol_handle *ph, u3
> 	if (IS_ERR(iter))
> 		return PTR_ERR(iter);
> 
>-	ret = ph->hops->iter_response_run(iter);
>-
>-	return ret;
>+	return ph->hops->iter_response_run(iter);
> }
> 
> static int
>@@ -352,7 +361,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> 	u32 attributes;
> 	struct scmi_xfer *t;
> 	struct scmi_msg_resp_clock_attributes *attr;
>-	struct scmi_clock_info *clk = cinfo->clk + clk_id;
>+	struct scmi_clock_info *clk = CLOCK_INFO(cinfo, clk_id);
> 
> 	ret = ph->xops->xfer_get_init(ph, CLOCK_ATTRIBUTES,
> 				      sizeof(clk_id), sizeof(*attr), &t);
>@@ -394,7 +403,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> 			clk->rate_change_requested_notifications = true;
> 		if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
> 			if (SUPPORTS_PARENT_CLOCK(attributes))
>-				scmi_clock_possible_parents(ph, clk_id, clk);
>+				scmi_clock_possible_parents(ph, clk_id, cinfo);
> 			if (SUPPORTS_GET_PERMISSIONS(attributes))
> 				scmi_clock_get_permissions(ph, clk_id, clk);
> 			if (SUPPORTS_EXTENDED_CONFIG(attributes))
>@@ -424,7 +433,7 @@ static void iter_clk_describe_prepare_message(void *message,
> 	struct scmi_msg_clock_describe_rates *msg = message;
> 	const struct scmi_clk_ipriv *p = priv;
> 
>-	msg->id = cpu_to_le32(p->clk_id);
>+	msg->id = cpu_to_le32(p->clkd->id);
> 	/* Set the number of rates to be skipped/already read */
> 	msg->rate_index = cpu_to_le32(desc_index);
> }
>@@ -457,14 +466,14 @@ 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->clk->rate_discrete = RATE_DISCRETE(flags);
>+	p->clkd->rate_discrete = RATE_DISCRETE(flags);
> 
> 	/* Warn about out of spec replies ... */
>-	if (!p->clk->rate_discrete &&
>+	if (!p->clkd->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",
>-			 p->clk->name, st->num_returned, st->num_remaining,
>+			 p->clkd->info.name, st->num_returned, st->num_remaining,
> 			 st->rx_len);
> 
> 		SCMI_QUIRK(clock_rates_triplet_out_of_spec,
>@@ -479,38 +488,19 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
> 				   const void *response,
> 				   struct scmi_iterator_state *st, void *priv)
> {
>-	int ret = 0;
> 	struct scmi_clk_ipriv *p = priv;
> 	const struct scmi_msg_resp_clock_describe_rates *r = response;
> 
>-	if (!p->clk->rate_discrete) {
>-		switch (st->desc_index + st->loop_idx) {
>-		case 0:
>-			p->clk->range.min_rate = RATE_TO_U64(r->rate[0]);
>-			break;
>-		case 1:
>-			p->clk->range.max_rate = RATE_TO_U64(r->rate[1]);
>-			break;
>-		case 2:
>-			p->clk->range.step_size = RATE_TO_U64(r->rate[2]);
>-			break;
>-		default:
>-			ret = -EINVAL;
>-			break;
>-		}
>-	} else {
>-		u64 *rate = &p->clk->list.rates[st->desc_index + st->loop_idx];
>+	p->clkd->rates[st->desc_index + st->loop_idx] =
>+		RATE_TO_U64(r->rate[st->loop_idx]);
>+	p->clkd->num_rates++;
> 
>-		*rate = RATE_TO_U64(r->rate[st->loop_idx]);
>-		p->clk->list.num_rates++;
>-	}
>-
>-	return ret;
>+	return 0;
> }
> 
> static int
> scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
>-			      struct scmi_clock_info *clk)
>+			      struct clock_info *cinfo)
> {
> 	int ret;
> 	void *iter;
>@@ -519,9 +509,9 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
> 		.update_state = iter_clk_describe_update_state,
> 		.process_response = iter_clk_describe_process_response,
> 	};
>+	struct scmi_clock_desc *clkd = &cinfo->clkds[clk_id];
> 	struct scmi_clk_ipriv cpriv = {
>-		.clk_id = clk_id,
>-		.clk = clk,
>+		.clkd = clkd,
> 		.dev = ph->dev,
> 	};
> 
>@@ -536,16 +526,23 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
> 	if (ret)
> 		return ret;
> 
>-	if (!clk->rate_discrete) {
>+	/* empty set ? */
>+	if (!clkd->num_rates)
>+		return 0;
>+
>+	if (!clkd->rate_discrete) {
>+		clkd->info.max_rate = clkd->rates[RATE_MAX];

Not related to this patch. Just have a question,
if a broken firmware returns RATE_MIN, but no RATE_MAX and RATE_STEP,
should some sanity checking being added?

Regards,
Peng

Re: [PATCH 04/11] clk: scmi: Use new simplified per-clock rate properties

[Peng Fan]

On Fri, Feb 27, 2026 at 03:32:18PM +0000, Cristian Marussi wrote:
>Use the new min_rate and max_rate unified properties that provide the
>proper values without having to consider the clock type.
>
>Cc: Michael Turquette <mturquette@baylibre.com>
>Cc: Stephen Boyd <sboyd@kernel.org>
>Cc: linux-clk@vger.kernel.org
>Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH 05/11] firmware: arm_scmi: Drop unused clock rate interfaces

[Peng Fan]

On Fri, Feb 27, 2026 at 03:32:19PM +0000, Cristian Marussi wrote:
>Only the unified interface exposing min_rate/max_rate is now used.
>
>Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH 06/11] firmware: arm_scmi: Make clock rates allocation dynamic

[Peng Fan]

On Fri, Feb 27, 2026 at 03:32:20PM +0000, Cristian Marussi wrote:
>Leveraging SCMI Clock protocol dynamic discovery capabilities, move away
>from the static per-clock rates allocation model in favour of a dynamic
>runtime allocation based on effectively discovered resources.
>
>No functional change.
>
>Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>---
> drivers/firmware/arm_scmi/clock.c | 19 ++++++++++++++++---
> include/linux/scmi_protocol.h     |  1 -
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
>index f5d1c608f85a..d0fb5affb5cf 100644
>--- a/drivers/firmware/arm_scmi/clock.c
>+++ b/drivers/firmware/arm_scmi/clock.c
>@@ -161,7 +161,7 @@ struct scmi_clock_desc {
> 	u32 id;
> 	bool rate_discrete;
> 	unsigned int num_rates;
>-	u64 rates[SCMI_MAX_NUM_RATES];
>+	u64 *rates;
> #define	RATE_MIN	0
> #define	RATE_MAX	1
> #define	RATE_STEP	2
>@@ -480,6 +480,18 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,
> 			   QUIRK_OUT_OF_SPEC_TRIPLET);
> 	}
> 
>+	if (!st->max_resources) {
>+		int num_rates = st->num_returned + st->num_remaining;
>+
>+		p->clkd->rates = devm_kcalloc(p->dev, num_rates,
>+					      sizeof(*p->clkd->rates), GFP_KERNEL);
>+		if (!p->clkd->rates)
>+			return -ENOMEM;

It may be not related to this patch,
I see scmi_clock_describe_rates_get() does not have return value check
when being called in scmi_clock_protocol_init().

So if devm_kcalloc() fails, there maybe issue without a sanity check
to return value of scmi_clock_describe_rates_get().

Regards
Peng

>+
>+		/* max_resources is used by the iterators to control bounds */
>+		st->max_resources = st->num_returned + st->num_remaining;
>+	}
>+
> 	return 0;
> }
> 
>@@ -493,6 +505,8 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
> 
> 	p->clkd->rates[st->desc_index + st->loop_idx] =
> 		RATE_TO_U64(r->rate[st->loop_idx]);
>+
>+	/* Count only effectively discovered rates */
> 	p->clkd->num_rates++;
> 
> 	return 0;
>@@ -515,8 +529,7 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
> 		.dev = ph->dev,
> 	};
> 
>-	iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_RATES,
>-					    CLOCK_DESCRIBE_RATES,
>+	iter = ph->hops->iter_response_init(ph, &ops, 0, CLOCK_DESCRIBE_RATES,
> 					    sizeof(struct scmi_msg_clock_describe_rates),
> 					    &cpriv);
> 	if (IS_ERR(iter))
>diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
>index d97b4e734744..5552ac04c820 100644
>--- a/include/linux/scmi_protocol.h
>+++ b/include/linux/scmi_protocol.h
>@@ -15,7 +15,6 @@
> 
> #define SCMI_MAX_STR_SIZE		64
> #define SCMI_SHORT_NAME_MAX_SIZE	16
>-#define SCMI_MAX_NUM_RATES		16
> 
> /**
>  * struct scmi_revision_info - version information structure
>-- 
>2.53.0
>

Re: [PATCH 07/11] firmware: arm_scmi: Harden clock parents discovery

[Peng Fan]

On Fri, Feb 27, 2026 at 03:32:21PM +0000, Cristian Marussi wrote:
>Fix clock parents enumeration to account only for effectively discovered
>parents during enumeration, avoiding to trust the total number of parents
>declared upfront by the platform.
>
>Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>---
> drivers/firmware/arm_scmi/clock.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
>index d0fb5affb5cf..15faa79abed4 100644
>--- a/drivers/firmware/arm_scmi/clock.c
>+++ b/drivers/firmware/arm_scmi/clock.c
>@@ -270,15 +270,15 @@ static int iter_clk_possible_parents_update_state(struct scmi_iterator_state *st
> 	 * assume it's returned+remaining on first call.
> 	 */
> 	if (!st->max_resources) {
>-		p->clkd->info.num_parents = st->num_returned + st->num_remaining;
>-		p->clkd->info.parents = devm_kcalloc(p->dev,
>-						     p->clkd->info.num_parents,
>+		int num_parents = st->num_returned + st->num_remaining;
>+
>+		p->clkd->info.parents = devm_kcalloc(p->dev, num_parents,
> 						     sizeof(*p->clkd->info.parents),
> 						     GFP_KERNEL);
>-		if (!p->clkd->info.parents) {
>-			p->clkd->info.num_parents = 0;
>+		if (!p->clkd->info.parents)
> 			return -ENOMEM;
>-		}
>+
>+		/* max_resources is used by the iterators to control bounds */
> 		st->max_resources = st->num_returned + st->num_remaining;
> 	}
> 
>@@ -293,9 +293,11 @@ static int iter_clk_possible_parents_process_response(const struct scmi_protocol
> 	const struct scmi_msg_resp_clock_possible_parents *r = response;
> 	struct scmi_clk_ipriv *p = priv;
> 
>-	u32 *parent = &p->clkd->info.parents[st->desc_index + st->loop_idx];
>+	p->clkd->info.parents[st->desc_index + st->loop_idx] =
>+		le32_to_cpu(r->possible_parents[st->loop_idx]);
> 
>-	*parent = le32_to_cpu(r->possible_parents[st->loop_idx]);
>+	/* Count only effectively discovered parents */
>+	p->clkd->info.num_parents++;

It maybe good to give a warning, if mismatch between 
number of effectively discovered parents and "st->num_returned + st->num_remaining"

Anyway this patch LGTM:

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> 
> 	return 0;
> }
>-- 
>2.53.0
>

RE: [PATCH 09/11] firmware: arm_scmi: Add bound iterators support

[Peng Fan (OSS)]

> Subject: Re: [PATCH 09/11] firmware: arm_scmi: Add bound iterators
> support
> 
> On Fri, Feb 27, 2026 at 03:32:23PM +0000, Cristian Marussi wrote:
> >SCMI core stack provides some common helpers to handle in a unified
> way
> >multipart message replies: such iterator-helpers, when run, currently
> >process by default the whole set of discovered resources.
> >
> >Introduce an alternative way to run the initialized iterator on a
> >limited range of resources.
> >
> >Note that the subset of resources that can be chosen is anyway
> limited
> >by the SCMI protocol specification, since you are only allowed to
> >choose the startindex on a multi-part enumeration NOT the end index,
> so
> >that the effective number of returned items by a bound iterators
> >depends really on platform side decisions.
> >
> >Suggested-by: Etienne Carriere <etienne.carriere@foss.st.com>
> >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >---
> > drivers/firmware/arm_scmi/clock.c     |  3 +-
> > drivers/firmware/arm_scmi/driver.c    | 58 +++++++++++++++++++------
> --
> > drivers/firmware/arm_scmi/protocols.h | 13 +++++-
> > 3 files changed, 55 insertions(+), 19 deletions(-)
> >
> >diff --git a/drivers/firmware/arm_scmi/clock.c
> >b/drivers/firmware/arm_scmi/clock.c
> >index 15faa79abed4..d7df5c45836e 100644
> >--- a/drivers/firmware/arm_scmi/clock.c
> >+++ b/drivers/firmware/arm_scmi/clock.c
> >@@ -505,8 +505,7 @@ 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[st->desc_index + st->loop_idx] =
> >-		RATE_TO_U64(r->rate[st->loop_idx]);
> >+	p->clkd->rates[p->clkd->num_rates] =
> >+RATE_TO_U64(r->rate[st->loop_idx]);
> 
> Seems irrelevant
> 
Ignore this. I understand wrong.

Thanks,
Peng.

Re: [PATCH 09/11] firmware: arm_scmi: Add bound iterators support

[Peng Fan]

On Fri, Feb 27, 2026 at 03:32:23PM +0000, Cristian Marussi wrote:
>SCMI core stack provides some common helpers to handle in a unified way
>multipart message replies: such iterator-helpers, when run, currently
>process by default the whole set of discovered resources.
>
>Introduce an alternative way to run the initialized iterator on a limited
>range of resources.
>
>Note that the subset of resources that can be chosen is anyway limited by
>the SCMI protocol specification, since you are only allowed to choose the
>startindex on a multi-part enumeration NOT the end index, so that the
>effective number of returned items by a bound iterators depends really
>on platform side decisions.
>
>Suggested-by: Etienne Carriere <etienne.carriere@foss.st.com>
>Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>---
> drivers/firmware/arm_scmi/clock.c     |  3 +-
> drivers/firmware/arm_scmi/driver.c    | 58 +++++++++++++++++++--------
> drivers/firmware/arm_scmi/protocols.h | 13 +++++-
> 3 files changed, 55 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
>index 15faa79abed4..d7df5c45836e 100644
>--- a/drivers/firmware/arm_scmi/clock.c
>+++ b/drivers/firmware/arm_scmi/clock.c
>@@ -505,8 +505,7 @@ 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[st->desc_index + st->loop_idx] =
>-		RATE_TO_U64(r->rate[st->loop_idx]);
>+	p->clkd->rates[p->clkd->num_rates] = RATE_TO_U64(r->rate[st->loop_idx]);

Seems irrelevant

Regards
Peng

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

[Peng Fan]

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

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

[Cristian Marussi]

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

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

[Cristian Marussi]

On Sat, Feb 28, 2026 at 08:27:11AM +0800, Peng Fan wrote:
> Hi Cristian,
> 
> On Fri, Feb 27, 2026 at 03:32:15PM +0000, Cristian Marussi 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>
> >---
> >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
> 

Hi Peng, 

thanks for having a look...

> Is there any early reviewing version available?

No I dont think there is anything shareable...just some preliminary
exploratory work following your and other vendor reaquest to have a way
to properly determine upfront what will be the final rate starting from
the requested one, because delegating all to the fw-side round-up leads
to issues in some cases when the final rate is different from teh
requested one...well...you know better than me why, being one of the
guys that pointed out the issues... :D (if I am not mistaken)

It is anyway material for v4.1...which has still to be started...so this
was just a reminder that a dedicated protocol version would be most
probably needed soon-ish..

Thanks,
Cristian

Re: [PATCH 02/11] clk: scmi: Use new determine_rate clock operation

[Cristian Marussi]

On Sat, Feb 28, 2026 at 08:56:04AM +0800, Peng Fan wrote:
> On Fri, Feb 27, 2026 at 03:32:16PM +0000, Cristian Marussi wrote:
> >Use the Clock protocol layer determine_rate logic to calculate the closest
> >rate that can be supported by a specific clock.
> >
> >No functional change.
> >
> >Cc: Brian Masney <bmasney@redhat.com>
> >Cc: Michael Turquette <mturquette@baylibre.com>
> >Cc: Stephen Boyd <sboyd@kernel.org>
> >Cc: linux-clk@vger.kernel.org
> >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >---
> >Note that the calculation logic in the protocol layer is exactly the same
> >as it wes here.
> >
> >@Brian I suppose once your CLK_ROUNDING_FW_MANAGED sereis is merged I can flag
> >such SCMI clocks.
> 
> Per my reading of Brain's thread, if ->determine_rate exists,
> ->determine_rate() will be used.
> 
>  	} else if (core->ops->determine_rate) {
>  		return core->ops->determine_rate(core->hw, req);
> +	} else if (clk_is_rounding_fw_managed(core)) {
> +		return 0;
> 
> So unless update scmi_clk_determine_rate() to something:
> --------
> if (clk & CLK_ROUNDING_FW_MANAGED)
> 	return 0;
> 
> return scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);
> --------
> 
> It maybe better to update Brain's patch to move clk_is_rounding_fw_managed()
> above the check of core->ops->determine_rate().

Indeed, I may have not fully understood Brian patch, since it appeared
while I was already reworking this...

I suppose I could also refrain from registering a determine_rate and
use the new flag when I know the rate will be rounded by FW...in the
future simply there will be the possibility to ask the firmware first
for a final 'clock rate determination' upfront in some of the cases in
which now we rely on FW rounding..

> 
> >---
> > drivers/clk/clk-scmi.c | 31 ++++++-------------------------
> > 1 file changed, 6 insertions(+), 25 deletions(-)
> >
> >diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> >index 6b286ea6f121..c223e4ef1dd1 100644
> >--- a/drivers/clk/clk-scmi.c
> >+++ b/drivers/clk/clk-scmi.c
> >@@ -12,7 +12,6 @@
> > #include <linux/of.h>
> > #include <linux/module.h>
> > #include <linux/scmi_protocol.h>
> >-#include <asm/div64.h>
> > 
> > #define NOT_ATOMIC	false
> > #define ATOMIC		true
> >@@ -57,35 +56,17 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
> > static int scmi_clk_determine_rate(struct clk_hw *hw,
> > 				   struct clk_rate_request *req)
> > {
> >-	u64 fmin, fmax, ftmp;
> >+	int ret;
> > 	struct scmi_clk *clk = to_scmi_clk(hw);
> > 
> > 	/*
> >-	 * We can't figure out what rate it will be, so just return the
> >-	 * rate back to the caller. scmi_clk_recalc_rate() will be called
> >-	 * after the rate is set and we'll know what rate the clock is
> >+	 * If we could not get a better rate scmi_clk_recalc_rate() will be
> >+	 * called after the rate is set and we'll know what rate the clock is
> > 	 * running at then.
> > 	 */
> >-	if (clk->info->rate_discrete)
> >-		return 0;
> >-
> >-	fmin = clk->info->range.min_rate;
> >-	fmax = clk->info->range.max_rate;
> >-	if (req->rate <= fmin) {
> >-		req->rate = fmin;
> >-
> >-		return 0;
> >-	} else if (req->rate >= fmax) {
> >-		req->rate = fmax;
> >-
> >-		return 0;
> >-	}
> >-
> >-	ftmp = req->rate - fmin;
> >-	ftmp += clk->info->range.step_size - 1; /* to round up */
> >-	do_div(ftmp, clk->info->range.step_size);
> >-
> >-	req->rate = ftmp * clk->info->range.step_size + fmin;
> >+	ret = scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);
> >+	if (ret)
> >+		return ret;
> 
> nit:
> "return scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);"

..oh yes...
> 
> Otherwise:
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> 

Thanks,
Cristian

Re: [PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface

[Cristian Marussi]

On Sat, Feb 28, 2026 at 10:07:28AM +0800, Peng Fan wrote:
> On Fri, Feb 27, 2026 at 03:32:17PM +0000, Cristian Marussi wrote:
> >Move needlessly exposed fields away from scmi_clock_info into the new
> >internal struct scmi_clock_desc while keeping exposed only the two new
> >min_rate and max_rate fields for each clock.
> >
> >No functional change.
> >
> >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >---
> > drivers/firmware/arm_scmi/clock.c | 145 +++++++++++++++---------------
> > include/linux/scmi_protocol.h     |   2 +
> > 2 files changed, 74 insertions(+), 73 deletions(-)
> >
> >diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> >index 54e8b59c3941..f5d1c608f85a 100644
> >--- a/drivers/firmware/arm_scmi/clock.c
> >+++ b/drivers/firmware/arm_scmi/clock.c
> >@@ -157,13 +157,27 @@ struct scmi_clock_rate_notify_payld {
> > 	__le32 rate_high;
> > };
> > 
> >+struct scmi_clock_desc {
> >+	u32 id;
> >+	bool rate_discrete;
> >+	unsigned int num_rates;
> >+	u64 rates[SCMI_MAX_NUM_RATES];
> >+#define	RATE_MIN	0
> >+#define	RATE_MAX	1
> >+#define	RATE_STEP	2
> >+	struct scmi_clock_info info;
> >+};
> >+
> >+#define to_desc(p)	(container_of((p), struct scmi_clock_desc, info))
> 
> Nit:
> no need parentheses
> 

ok...

> >+
> > struct clock_info {
> > 	int num_clocks;
> > 	int max_async_req;
> > 	bool notify_rate_changed_cmd;
> > 	bool notify_rate_change_requested_cmd;
> > 	atomic_t cur_async_req;
> >-	struct scmi_clock_info *clk;
> >+	struct scmi_clock_desc *clkds;
> >+#define CLOCK_INFO(c, i)	(&(((c)->clkds + (i))->info))
> 
> Ditto.

ok

> 
> > 	int (*clock_config_set)(const struct scmi_protocol_handle *ph,
> > 				u32 clk_id, enum clk_state state,
> > 				enum scmi_clock_oem_config oem_type,
> >@@ -185,7 +199,7 @@ scmi_clock_domain_lookup(struct clock_info *ci, u32 clk_id)
> > 	if (clk_id >= ci->num_clocks)
> > 		return ERR_PTR(-EINVAL);
> > 
> >-	return ci->clk + clk_id;
> >+	return CLOCK_INFO(ci, clk_id);
> > }

[snip] 

> > 
> >@@ -536,16 +526,23 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
> > 	if (ret)
> > 		return ret;
> > 
> >-	if (!clk->rate_discrete) {
> >+	/* empty set ? */
> >+	if (!clkd->num_rates)
> >+		return 0;
> >+
> >+	if (!clkd->rate_discrete) {
> >+		clkd->info.max_rate = clkd->rates[RATE_MAX];
> 
> Not related to this patch. Just have a question,
> if a broken firmware returns RATE_MIN, but no RATE_MAX and RATE_STEP,
> should some sanity checking being added?
>

I think we already have consistency check around this, while parsing
with the iterators

	https://elixir.bootlin.com/linux/v6.19.3/source/drivers/firmware/arm_scmi/clock.c#L464

...and a related quirk because some platform really reported the right
number of rates BUT failed to express that in the message AND the
introduced additional checks broke some platforms in the wild..

	https://elixir.bootlin.com/linux/v6.19.3/source/drivers/firmware/arm_scmi/clock.c#L433

Do you see still some opportunity for misbehave with the above ?

Thanks,
Cristian

Re: [PATCH 06/11] firmware: arm_scmi: Make clock rates allocation dynamic

[Cristian Marussi]

On Sat, Feb 28, 2026 at 10:29:11AM +0800, Peng Fan wrote:
> On Fri, Feb 27, 2026 at 03:32:20PM +0000, Cristian Marussi wrote:
> >Leveraging SCMI Clock protocol dynamic discovery capabilities, move away
> >from the static per-clock rates allocation model in favour of a dynamic
> >runtime allocation based on effectively discovered resources.
> >
> >No functional change.
> >
> >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >---
> > drivers/firmware/arm_scmi/clock.c | 19 ++++++++++++++++---
> > include/linux/scmi_protocol.h     |  1 -
> > 2 files changed, 16 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> >index f5d1c608f85a..d0fb5affb5cf 100644
> >--- a/drivers/firmware/arm_scmi/clock.c
> >+++ b/drivers/firmware/arm_scmi/clock.c
> >@@ -161,7 +161,7 @@ struct scmi_clock_desc {
> > 	u32 id;
> > 	bool rate_discrete;
> > 	unsigned int num_rates;
> >-	u64 rates[SCMI_MAX_NUM_RATES];
> >+	u64 *rates;
> > #define	RATE_MIN	0
> > #define	RATE_MAX	1
> > #define	RATE_STEP	2
> >@@ -480,6 +480,18 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,
> > 			   QUIRK_OUT_OF_SPEC_TRIPLET);
> > 	}
> > 
> >+	if (!st->max_resources) {
> >+		int num_rates = st->num_returned + st->num_remaining;
> >+
> >+		p->clkd->rates = devm_kcalloc(p->dev, num_rates,
> >+					      sizeof(*p->clkd->rates), GFP_KERNEL);
> >+		if (!p->clkd->rates)
> >+			return -ENOMEM;
> 
> It may be not related to this patch,
> I see scmi_clock_describe_rates_get() does not have return value check
> when being called in scmi_clock_protocol_init().
> 
> So if devm_kcalloc() fails, there maybe issue without a sanity check
> to return value of scmi_clock_describe_rates_get().
> 

Yes I think I saw that...wanted to fix too..and forgot :D 

I will add a fix in V2.

Thanks,
Cristian

Re: [PATCH 07/11] firmware: arm_scmi: Harden clock parents discovery

[Cristian Marussi]

On Sat, Feb 28, 2026 at 10:39:59AM +0800, Peng Fan wrote:
> On Fri, Feb 27, 2026 at 03:32:21PM +0000, Cristian Marussi wrote:
> >Fix clock parents enumeration to account only for effectively discovered
> >parents during enumeration, avoiding to trust the total number of parents
> >declared upfront by the platform.
> >
> >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >---
> > drivers/firmware/arm_scmi/clock.c | 18 ++++++++++--------
> > 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> >diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> >index d0fb5affb5cf..15faa79abed4 100644
> >--- a/drivers/firmware/arm_scmi/clock.c
> >+++ b/drivers/firmware/arm_scmi/clock.c
> >@@ -270,15 +270,15 @@ static int iter_clk_possible_parents_update_state(struct scmi_iterator_state *st
> > 	 * assume it's returned+remaining on first call.
> > 	 */
> > 	if (!st->max_resources) {
> >-		p->clkd->info.num_parents = st->num_returned + st->num_remaining;
> >-		p->clkd->info.parents = devm_kcalloc(p->dev,
> >-						     p->clkd->info.num_parents,
> >+		int num_parents = st->num_returned + st->num_remaining;
> >+
> >+		p->clkd->info.parents = devm_kcalloc(p->dev, num_parents,
> > 						     sizeof(*p->clkd->info.parents),
> > 						     GFP_KERNEL);
> >-		if (!p->clkd->info.parents) {
> >-			p->clkd->info.num_parents = 0;
> >+		if (!p->clkd->info.parents)
> > 			return -ENOMEM;
> >-		}
> >+
> >+		/* max_resources is used by the iterators to control bounds */
> > 		st->max_resources = st->num_returned + st->num_remaining;
> > 	}
> > 
> >@@ -293,9 +293,11 @@ static int iter_clk_possible_parents_process_response(const struct scmi_protocol
> > 	const struct scmi_msg_resp_clock_possible_parents *r = response;
> > 	struct scmi_clk_ipriv *p = priv;
> > 
> >-	u32 *parent = &p->clkd->info.parents[st->desc_index + st->loop_idx];
> >+	p->clkd->info.parents[st->desc_index + st->loop_idx] =
> >+		le32_to_cpu(r->possible_parents[st->loop_idx]);
> > 
> >-	*parent = le32_to_cpu(r->possible_parents[st->loop_idx]);
> >+	/* Count only effectively discovered parents */
> >+	p->clkd->info.num_parents++;
> 
> It maybe good to give a warning, if mismatch between 
> number of effectively discovered parents and "st->num_returned + st->num_remaining"
> 

Indeed there could be sign of something off fw-side...

> Anyway this patch LGTM:
> 
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
>

Thanks,
Cristian

Re: [PATCH 09/11] firmware: arm_scmi: Add bound iterators support

[Cristian Marussi]

On Sat, Feb 28, 2026 at 02:43:47AM +0000, Peng Fan (OSS) wrote:
> > Subject: Re: [PATCH 09/11] firmware: arm_scmi: Add bound iterators
> > support
> > 
> > On Fri, Feb 27, 2026 at 03:32:23PM +0000, Cristian Marussi wrote:
> > >SCMI core stack provides some common helpers to handle in a unified
> > way
> > >multipart message replies: such iterator-helpers, when run, currently
> > >process by default the whole set of discovered resources.
> > >
> > >Introduce an alternative way to run the initialized iterator on a
> > >limited range of resources.
> > >
> > >Note that the subset of resources that can be chosen is anyway
> > limited
> > >by the SCMI protocol specification, since you are only allowed to
> > >choose the startindex on a multi-part enumeration NOT the end index,
> > so
> > >that the effective number of returned items by a bound iterators
> > >depends really on platform side decisions.
> > >
> > >Suggested-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > >---
> > > drivers/firmware/arm_scmi/clock.c     |  3 +-
> > > drivers/firmware/arm_scmi/driver.c    | 58 +++++++++++++++++++------
> > --
> > > drivers/firmware/arm_scmi/protocols.h | 13 +++++-
> > > 3 files changed, 55 insertions(+), 19 deletions(-)
> > >
> > >diff --git a/drivers/firmware/arm_scmi/clock.c
> > >b/drivers/firmware/arm_scmi/clock.c
> > >index 15faa79abed4..d7df5c45836e 100644
> > >--- a/drivers/firmware/arm_scmi/clock.c
> > >+++ b/drivers/firmware/arm_scmi/clock.c
> > >@@ -505,8 +505,7 @@ 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[st->desc_index + st->loop_idx] =
> > >-		RATE_TO_U64(r->rate[st->loop_idx]);
> > >+	p->clkd->rates[p->clkd->num_rates] =
> > >+RATE_TO_U64(r->rate[st->loop_idx]);
> > 
> > Seems irrelevant
> > 
> Ignore this. I understand wrong.

Yeah...I will double check BUT the aim is to proper behaving while
filling up the vector when doing only a partial discovery with bound
iterators...in that case you want to fill probably only a subset of the
available slots in order like [0]:min, [1]:max...

Thanks,
Cristian

Re: [PATCH 10/11] firmware: arm_scmi: Use bound iterators to minimize discovered rates

[Cristian Marussi]

On Fri, Feb 27, 2026 at 04:53:39PM +0000, Jonathan Cameron wrote:
> On Fri, 27 Feb 2026 15:32:24 +0000
> Cristian Marussi <cristian.marussi@arm.com> wrote:
> 
> > Clock rates are guaranteed to be returned in ascending order for SCMI clock
> > protocol versions greater than 1.0: in such a case, use bounded iterators
> > to minimize the number of message exchanges needed to discover min and max
> > rate.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> > +
> > +static int
> > +scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph,
> > +			      u32 clk_id, struct clock_info *cinfo)
> > +{
> > +	struct scmi_clock_desc *clkd = &cinfo->clkds[clk_id];
> > +	int ret;
> > +
> > +	/*
> > +	 * Since only after SCMI Clock v1.0 the returned rates are guaranteed to
> > +	 * be discovered in ascending order, lazy enumeration cannot be use for
> > +	 * SCMI Clock v1.0 protocol.
> > +	 */
> > +	if (PROTOCOL_REV_MAJOR(ph->version) > 0x1)
> > +		ret = scmi_clock_describe_rates_get_lazy(ph, clkd);
> > +	else
> > +		ret = scmi_clock_describe_rates_get_full(ph, clkd);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	clkd->info.min_rate = clkd->rates[RATE_MIN];
> >  	if (!clkd->rate_discrete) {
> >  		clkd->info.max_rate = clkd->rates[RATE_MAX];
> >  		dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
> >  			clkd->rates[RATE_MIN], clkd->rates[RATE_MAX],
> >  			clkd->rates[RATE_STEP]);
> >  	} else {
> > -		sort(clkd->rates, clkd->num_rates,
> > -		     sizeof(clkd->rates[0]), rate_cmp_func, NULL);
> >  		clkd->info.max_rate = clkd->rates[clkd->num_rates - 1];
> > +		dev_dbg(ph->dev, "Clock:%s DISCRETE:%d -> Min %llu Max %llu\n",
> > +			clkd->info.name, clkd->rate_discrete,
> > +			clkd->info.min_rate, clkd->info.max_rate);
> >  	}
> > -	clkd->info.min_rate = clkd->rates[RATE_MIN];
> >  
> > -	return 0;
> > +	return ret;
> Why?  Far as I can see it's still always zero if you get here.

...well...simply bad refactoring late evening near beer o'clock time :P

I will fix.

Thanks,
Cristian

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

[Cristian Marussi]

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

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

[Peng Fan]

On Sat, Feb 28, 2026 at 10:47:20AM +0000, Cristian Marussi wrote:
>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...

Per https://developer.arm.com/documentation/den0056/c/?lang=en, SCMI version
3.0, CLOCK protocol version is 1.0, I see:
"The clock rates returned by this call should be in numeric ascending order".

SCMI 2.0 also has it. But SCMI 1.0 does not have above line.

All the above specs are using CLOCK protocol version 1.0.

It might be overshoot, should the "PROTOCOL_REV_MAJOR(ph->version) == 0x1"
be dropped, so always sort the array?

Regards
Peng

>
>Thanks,
>Cristian

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

[Cristian Marussi]

On Mon, Mar 02, 2026 at 03:18:30PM +0800, Peng Fan wrote:
> On Sat, Feb 28, 2026 at 10:47:20AM +0000, Cristian Marussi wrote:
> >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.
> >> >

Hi,

> >> >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(-)

[snip]

> >> > 
> >> >-	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...
> 
> Per https://developer.arm.com/documentation/den0056/c/?lang=en, SCMI version
> 3.0, CLOCK protocol version is 1.0, I see:
> "The clock rates returned by this call should be in numeric ascending order".
> 
> SCMI 2.0 also has it. But SCMI 1.0 does not have above line.
> 
> All the above specs are using CLOCK protocol version 1.0.
> 
> It might be overshoot, should the "PROTOCOL_REV_MAJOR(ph->version) == 0x1"
> be dropped, so always sort the array?

Yes there is a 'glitch' in the specs v1.0/v2.0/v3.0 since all reports
Clock proto version 0x10000 BUT the clarification around the ordering of
the returned rates is NOT always there, so, since we must be absolutely
pedantic to guarantee compatibility with any possible deployed SCMI/FW,
we cannot assume that any 0x10000 protocol return ordered rates: this
means that if proto==0x10000 we MUST sort AND we cannot play smart with
the new bound iterators since we are NOT assure that rates[0] AND
rates[last -1] contain respectively min and max rates...

On the other side for any protocol version != 0x10000 (i.e. v3.1/v3.2/v4.0)
we can assume those rates as ordered and so, on one side, we can run 'lazy'
partial enumerations AND, on the other side, we can avoid the overhead of
sorting with full scans enumerations.

Thanks,
Cristian

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

[Geert Uytterhoeven]

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!

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

> +
> +       *rate = ftmp * clk->range.step_size + fmin;
> +
> +       return 0;
> +}
> +
>  static int
>  scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id,
>                       enum clk_state state,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 02/11] clk: scmi: Use new determine_rate clock operation

[Geert Uytterhoeven]

Hi Cristian,

On Fri, 27 Feb 2026 at 16:33, Cristian Marussi <cristian.marussi@arm.com> wrote:
> Use the Clock protocol layer determine_rate logic to calculate the closest
> rate that can be supported by a specific clock.
>
> No functional change.
>
> Cc: Brian Masney <bmasney@redhat.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -57,35 +56,17 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
>  static int scmi_clk_determine_rate(struct clk_hw *hw,
>                                    struct clk_rate_request *req)
>  {
> -       u64 fmin, fmax, ftmp;
> +       int ret;
>         struct scmi_clk *clk = to_scmi_clk(hw);
>
>         /*
> -        * We can't figure out what rate it will be, so just return the
> -        * rate back to the caller. scmi_clk_recalc_rate() will be called
> -        * after the rate is set and we'll know what rate the clock is
> +        * If we could not get a better rate scmi_clk_recalc_rate() will be
> +        * called after the rate is set and we'll know what rate the clock is
>          * running at then.
>          */
> -       if (clk->info->rate_discrete)
> -               return 0;
> -
> -       fmin = clk->info->range.min_rate;
> -       fmax = clk->info->range.max_rate;
> -       if (req->rate <= fmin) {
> -               req->rate = fmin;
> -
> -               return 0;
> -       } else if (req->rate >= fmax) {
> -               req->rate = fmax;
> -
> -               return 0;
> -       }
> -
> -       ftmp = req->rate - fmin;
> -       ftmp += clk->info->range.step_size - 1; /* to round up */
> -       do_div(ftmp, clk->info->range.step_size);

Oh, so the truncation bug exists in the original code, too.

> -
> -       req->rate = ftmp * clk->info->range.step_size + fmin;
> +       ret = scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);
> +       if (ret)
> +               return ret;
>
>         return 0;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface

[Geert Uytterhoeven]

Hi Cristian,

Thanks for your patch!

On Fri, 27 Feb 2026 at 16:33, Cristian Marussi <cristian.marussi@arm.com> wrote:
> Move needlessly exposed fields away from scmi_clock_info into the new
> internal struct scmi_clock_desc while keeping exposed only the two new
> min_rate and max_rate fields for each clock.
>
> No functional change.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -157,13 +157,27 @@ struct scmi_clock_rate_notify_payld {
>         __le32 rate_high;
>  };
>
> +struct scmi_clock_desc {
> +       u32 id;
> +       bool rate_discrete;
> +       unsigned int num_rates;
> +       u64 rates[SCMI_MAX_NUM_RATES];
> +#define        RATE_MIN        0
> +#define        RATE_MAX        1
> +#define        RATE_STEP       2

Any specific reason you are not using a union here, like in
scmi_clock_info?

> +       struct scmi_clock_info info;
> +};

> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -51,6 +51,8 @@ struct scmi_clock_info {
>         bool rate_ctrl_forbidden;
>         bool parent_ctrl_forbidden;
>         bool extended_config;
> +       u64 min_rate;
> +       u64 max_rate;
>         union {
>                 struct {
>                         int num_rates;

You patch description read like the actual rates would be moved
from scmi_clock_info to scmi_clock_desc, i.e. _removed_ here?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface

[Geert Uytterhoeven]

Hi Cristian,

On Mon, 2 Mar 2026 at 13:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, 27 Feb 2026 at 16:33, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > Move needlessly exposed fields away from scmi_clock_info into the new
> > internal struct scmi_clock_desc while keeping exposed only the two new
> > min_rate and max_rate fields for each clock.
> >
> > No functional change.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -157,13 +157,27 @@ struct scmi_clock_rate_notify_payld {
> >         __le32 rate_high;
> >  };
> >
> > +struct scmi_clock_desc {
> > +       u32 id;
> > +       bool rate_discrete;
> > +       unsigned int num_rates;
> > +       u64 rates[SCMI_MAX_NUM_RATES];
> > +#define        RATE_MIN        0
> > +#define        RATE_MAX        1
> > +#define        RATE_STEP       2
>
> Any specific reason you are not using a union here, like in
> scmi_clock_info?

Ah, "[PATCH 06/11] firmware: arm_scmi: Make clock rates allocation
dynamic" answers that.

>
> > +       struct scmi_clock_info info;
> > +};
>
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -51,6 +51,8 @@ struct scmi_clock_info {
> >         bool rate_ctrl_forbidden;
> >         bool parent_ctrl_forbidden;
> >         bool extended_config;
> > +       u64 min_rate;
> > +       u64 max_rate;
> >         union {
> >                 struct {
> >                         int num_rates;
>
> You patch description read like the actual rates would be moved
> from scmi_clock_info to scmi_clock_desc, i.e. _removed_ here?

And these members are actually removed in "[PATCH 05/11] firmware:
arm_scmi: Drop unused clock rate interfaces".  Please reflect that in
this patch description.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 00/11] SCMI Clock rates discovery rework

[Geert Uytterhoeven]

Hi Cristian,

Thanks for your series!

On Fri, 27 Feb 2026 at 16:33, Cristian Marussi <cristian.marussi@arm.com> wrote:
> it was a known limitation, in the SCMI Clock protocol support, the lack of
> dynamic allocation around per-clock rates discovery: fixed size statically
> per-clock rates arrays did not scale and was increasingly a waste of memory
> (see [1]).
>
> This series aim at solving this in successive steps:
>
>  - simplify and reduce to the minimum possible the rates data info exposed
>    to the SCMI driver by scmi_clock_info
>  - move away from static fixed allocation of per-clock rates arrays in
>    favour of a completely dynamic runtime allocation: just allocate what
>    is needed based on the effectively discovered
>
> This is done in patches 1-6.
>
> A further bigger optimization suggested in a past series [1] by Etienne

s/[1]/[2]/

> would be, whenever allowed by the spec, to limit upfront the number of
> queries in order to simply retrieve min and max rate, that are indeed the
> only rates needed by the CLK SCMI driver.
>
> The approach proposed in [1] was open coding and duplicating some of the

What does [1] refer to?

> functionalities already provided by SCMI iterators, though.
>
> Patch 7-10 implement such optimization instead by:
>
>  - reworking core SCMI iterators to support bound enumerations
>  - use such new bound iterators to perform the minimum number of queries
>    in order to ony retrieve min an max rate
>
> As a final result now the rates enumeration triggered by the CLK SCMI
> driver, while still allocating for all the existent rates, miminize the
> number of SCMI CLK_DESCRIBE_RATE messages needed to obtain min and max.
>
> Finally, patch 11 introduces a new clock protocol operation to be able to
> trigger anytime on demand a full enumeration and obtain the full list of
> rates when needed, not only min/max: this latter method is really only used
> currently by some dowstream SCMI Test driver of mine.
>
> Based on v7.0-rc1.
>
> Tested on JUNO and an emulated environment.

Thank you, this removes the need for increasing SCMI_MAX_NUM_RATES on
R-Car X5H, while decreasing memory usage.
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

> [1]: https://lore.kernel.org/arm-scmi/aZsX-oplR6fiLBBN@pluto/T/#t
> [2]: https://lore.kernel.org/20241203173908.3148794-2-etienne.carriere@foss.st.com

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 02/11] clk: scmi: Use new determine_rate clock operation

[Brian Masney]

On Sat, Feb 28, 2026 at 08:56:04AM +0800, Peng Fan wrote:
> On Fri, Feb 27, 2026 at 03:32:16PM +0000, Cristian Marussi wrote:
> >Use the Clock protocol layer determine_rate logic to calculate the closest
> >rate that can be supported by a specific clock.
> >
> >No functional change.
> >
> >Cc: Brian Masney <bmasney@redhat.com>
> >Cc: Michael Turquette <mturquette@baylibre.com>
> >Cc: Stephen Boyd <sboyd@kernel.org>
> >Cc: linux-clk@vger.kernel.org
> >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >---
> >Note that the calculation logic in the protocol layer is exactly the same
> >as it wes here.
> >
> >@Brian I suppose once your CLK_ROUNDING_FW_MANAGED sereis is merged I can flag
> >such SCMI clocks.
> 
> Per my reading of Brain's thread, if ->determine_rate exists,
> ->determine_rate() will be used.
> 
>  	} else if (core->ops->determine_rate) {
>  		return core->ops->determine_rate(core->hw, req);
> +	} else if (clk_is_rounding_fw_managed(core)) {
> +		return 0;
> 
> So unless update scmi_clk_determine_rate() to something:
> --------
> if (clk & CLK_ROUNDING_FW_MANAGED)
> 	return 0;
> 
> return scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);
> --------
> 
> It maybe better to update Brain's patch to move clk_is_rounding_fw_managed()
> above the check of core->ops->determine_rate().

The clk framework has some basic sanity checks in place that are called
during device probe to ensure that various ops are configured properly. I
could add a check that if CLK_ROUNDING_FW_MANAGED [*] is set, and a
determine_rate() op is set, then it gives an error.

[*] Note: I am tentatively planning to rename that to CLK_ROUNDING_NOOP
in v2 in about a week.

Brian

RE: [PATCH 02/11] clk: scmi: Use new determine_rate clock operation

[Peng Fan]

> Subject: Re: [PATCH 02/11] clk: scmi: Use new determine_rate clock
> operation
> 
> On Sat, Feb 28, 2026 at 08:56:04AM +0800, Peng Fan wrote:
> > On Fri, Feb 27, 2026 at 03:32:16PM +0000, Cristian Marussi wrote:
> > >Use the Clock protocol layer determine_rate logic to calculate the
> > >closest rate that can be supported by a specific clock.
> > >
> > >No functional change.
> > >
> > >Cc: Brian Masney <bmasney@redhat.com>
> > >Cc: Michael Turquette <mturquette@baylibre.com>
> > >Cc: Stephen Boyd <sboyd@kernel.org>
> > >Cc: linux-clk@vger.kernel.org
> > >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > >---
> > >Note that the calculation logic in the protocol layer is exactly the
> > >same as it wes here.
> > >
> > >@Brian I suppose once your CLK_ROUNDING_FW_MANAGED sereis
> is merged I
> > >can flag such SCMI clocks.
> >
> > Per my reading of Brain's thread, if ->determine_rate exists,
> > ->determine_rate() will be used.
> >
> >  	} else if (core->ops->determine_rate) {
> >  		return core->ops->determine_rate(core->hw, req);
> > +	} else if (clk_is_rounding_fw_managed(core)) {
> > +		return 0;
> >
> > So unless update scmi_clk_determine_rate() to something:
> > --------
> > if (clk & CLK_ROUNDING_FW_MANAGED)
> > 	return 0;
> >
> > return scmi_proto_clk_ops->determine_rate(clk->ph, clk->id,
> > &req->rate);
> > --------
> >
> > It maybe better to update Brain's patch to move
> > clk_is_rounding_fw_managed() above the check of core->ops-
> >determine_rate().
> 
> The clk framework has some basic sanity checks in place that are called
> during device probe to ensure that various ops are configured properly.
> I could add a check that if CLK_ROUNDING_FW_MANAGED [*] is set,
> and a
> determine_rate() op is set, then it gives an error.

Sounds good to me.

Thanks
Peng.

> 
> [*] Note: I am tentatively planning to rename that to
> CLK_ROUNDING_NOOP in v2 in about a week.
> 
> Brian
> 

Re: [PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface

[Cristian Marussi]

On Mon, Mar 02, 2026 at 01:48:00PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> Thanks for your patch!

Hi,

> 
> On Fri, 27 Feb 2026 at 16:33, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > Move needlessly exposed fields away from scmi_clock_info into the new
> > internal struct scmi_clock_desc while keeping exposed only the two new
> > min_rate and max_rate fields for each clock.
> >
> > No functional change.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -157,13 +157,27 @@ struct scmi_clock_rate_notify_payld {
> >         __le32 rate_high;
> >  };
> >
> > +struct scmi_clock_desc {
> > +       u32 id;
> > +       bool rate_discrete;
> > +       unsigned int num_rates;
> > +       u64 rates[SCMI_MAX_NUM_RATES];
> > +#define        RATE_MIN        0
> > +#define        RATE_MAX        1
> > +#define        RATE_STEP       2
> 
> Any specific reason you are not using a union here, like in
> scmi_clock_info?
>

No, I will fix in v2.
 
> > +       struct scmi_clock_info info;
> > +};
> 
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -51,6 +51,8 @@ struct scmi_clock_info {
> >         bool rate_ctrl_forbidden;
> >         bool parent_ctrl_forbidden;
> >         bool extended_config;
> > +       u64 min_rate;
> > +       u64 max_rate;
> >         union {
> >                 struct {
> >                         int num_rates;
> 
> You patch description read like the actual rates would be moved
> from scmi_clock_info to scmi_clock_desc, i.e. _removed_ here?
> 

Yes they will be removed later in the series to avoid breaking the
build...I will add a _removed_ tag for clarity (that TIL that exist :D)

Thanks,
Cristian

Re: [PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface

[Cristian Marussi]

On Mon, Mar 02, 2026 at 02:09:14PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Mon, 2 Mar 2026 at 13:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, 27 Feb 2026 at 16:33, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > Move needlessly exposed fields away from scmi_clock_info into the new
> > > internal struct scmi_clock_desc while keeping exposed only the two new
> > > min_rate and max_rate fields for each clock.
> > >
> > > No functional change.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -157,13 +157,27 @@ struct scmi_clock_rate_notify_payld {
> > >         __le32 rate_high;
> > >  };
> > >
> > > +struct scmi_clock_desc {
> > > +       u32 id;
> > > +       bool rate_discrete;
> > > +       unsigned int num_rates;
> > > +       u64 rates[SCMI_MAX_NUM_RATES];
> > > +#define        RATE_MIN        0
> > > +#define        RATE_MAX        1
> > > +#define        RATE_STEP       2
> >
> > Any specific reason you are not using a union here, like in
> > scmi_clock_info?
> 
> Ah, "[PATCH 06/11] firmware: arm_scmi: Make clock rates allocation
> dynamic" answers that.
> 
> >
> > > +       struct scmi_clock_info info;
> > > +};
> >
> > > --- a/include/linux/scmi_protocol.h
> > > +++ b/include/linux/scmi_protocol.h
> > > @@ -51,6 +51,8 @@ struct scmi_clock_info {
> > >         bool rate_ctrl_forbidden;
> > >         bool parent_ctrl_forbidden;
> > >         bool extended_config;
> > > +       u64 min_rate;
> > > +       u64 max_rate;
> > >         union {
> > >                 struct {
> > >                         int num_rates;
> >
> > You patch description read like the actual rates would be moved
> > from scmi_clock_info to scmi_clock_desc, i.e. _removed_ here?
> 
> And these members are actually removed in "[PATCH 05/11] firmware:
> arm_scmi: Drop unused clock rate interfaces".  Please reflect that in
> this patch description.
> 

Ok I will better explain.

Thanks,
Cristian

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

[Cristian Marussi]

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

Re: [PATCH 02/11] clk: scmi: Use new determine_rate clock operation

[Cristian Marussi]

On Mon, Mar 02, 2026 at 12:11:16PM -0500, Brian Masney wrote:
> On Sat, Feb 28, 2026 at 08:56:04AM +0800, Peng Fan wrote:
> > On Fri, Feb 27, 2026 at 03:32:16PM +0000, Cristian Marussi wrote:
> > >Use the Clock protocol layer determine_rate logic to calculate the closest
> > >rate that can be supported by a specific clock.
> > >
> > >No functional change.
> > >
> > >Cc: Brian Masney <bmasney@redhat.com>
> > >Cc: Michael Turquette <mturquette@baylibre.com>
> > >Cc: Stephen Boyd <sboyd@kernel.org>
> > >Cc: linux-clk@vger.kernel.org
> > >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > >---
> > >Note that the calculation logic in the protocol layer is exactly the same
> > >as it wes here.
> > >
> > >@Brian I suppose once your CLK_ROUNDING_FW_MANAGED sereis is merged I can flag
> > >such SCMI clocks.
> > 
> > Per my reading of Brain's thread, if ->determine_rate exists,
> > ->determine_rate() will be used.
> > 
> >  	} else if (core->ops->determine_rate) {
> >  		return core->ops->determine_rate(core->hw, req);
> > +	} else if (clk_is_rounding_fw_managed(core)) {
> > +		return 0;
> > 
> > So unless update scmi_clk_determine_rate() to something:
> > --------
> > if (clk & CLK_ROUNDING_FW_MANAGED)
> > 	return 0;
> > 
> > return scmi_proto_clk_ops->determine_rate(clk->ph, clk->id, &req->rate);
> > --------
> > 
> > It maybe better to update Brain's patch to move clk_is_rounding_fw_managed()
> > above the check of core->ops->determine_rate().
> 
> The clk framework has some basic sanity checks in place that are called
> during device probe to ensure that various ops are configured properly. I
> could add a check that if CLK_ROUNDING_FW_MANAGED [*] is set, and a
> determine_rate() op is set, then it gives an error.
> 
> [*] Note: I am tentatively planning to rename that to CLK_ROUNDING_NOOP
> in v2 in about a week.

Ok, I will track yor series to decide how to better handle this on my
side.

Thanks,
Cristian

Re: [PATCH 02/11] clk: scmi: Use new determine_rate clock operation

[Cristian Marussi]

On Mon, Mar 02, 2026 at 01:39:51PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Fri, 27 Feb 2026 at 16:33, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > Use the Clock protocol layer determine_rate logic to calculate the closest
> > rate that can be supported by a specific clock.
> >
> > No functional change.
> >
> > Cc: Brian Masney <bmasney@redhat.com>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: linux-clk@vger.kernel.org
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -57,35 +56,17 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
> >  static int scmi_clk_determine_rate(struct clk_hw *hw,
> >                                    struct clk_rate_request *req)
> >  {
> > -       u64 fmin, fmax, ftmp;
> > +       int ret;
> >         struct scmi_clk *clk = to_scmi_clk(hw);
> >
> >         /*
> > -        * We can't figure out what rate it will be, so just return the
> > -        * rate back to the caller. scmi_clk_recalc_rate() will be called
> > -        * after the rate is set and we'll know what rate the clock is
> > +        * If we could not get a better rate scmi_clk_recalc_rate() will be
> > +        * called after the rate is set and we'll know what rate the clock is
> >          * running at then.
> >          */
> > -       if (clk->info->rate_discrete)
> > -               return 0;
> > -
> > -       fmin = clk->info->range.min_rate;
> > -       fmax = clk->info->range.max_rate;
> > -       if (req->rate <= fmin) {
> > -               req->rate = fmin;
> > -
> > -               return 0;
> > -       } else if (req->rate >= fmax) {
> > -               req->rate = fmax;
> > -
> > -               return 0;
> > -       }
> > -
> > -       ftmp = req->rate - fmin;
> > -       ftmp += clk->info->range.step_size - 1; /* to round up */
> > -       do_div(ftmp, clk->info->range.step_size);
> 
> Oh, so the truncation bug exists in the original code, too.

Yep...as said I will fix the original code and then move the fixed
code.

Thanks,
Cristian

Re: [PATCH 00/11] SCMI Clock rates discovery rework

[Cristian Marussi]

On Mon, Mar 02, 2026 at 02:25:00PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> Thanks for your series!
> 
> On Fri, 27 Feb 2026 at 16:33, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > it was a known limitation, in the SCMI Clock protocol support, the lack of
> > dynamic allocation around per-clock rates discovery: fixed size statically
> > per-clock rates arrays did not scale and was increasingly a waste of memory
> > (see [1]).
> >
> > This series aim at solving this in successive steps:
> >
> >  - simplify and reduce to the minimum possible the rates data info exposed
> >    to the SCMI driver by scmi_clock_info
> >  - move away from static fixed allocation of per-clock rates arrays in
> >    favour of a completely dynamic runtime allocation: just allocate what
> >    is needed based on the effectively discovered
> >
> > This is done in patches 1-6.
> >
> > A further bigger optimization suggested in a past series [1] by Etienne
> 
> s/[1]/[2]/
> 
> > would be, whenever allowed by the spec, to limit upfront the number of
> > queries in order to simply retrieve min and max rate, that are indeed the
> > only rates needed by the CLK SCMI driver.
> >
> > The approach proposed in [1] was open coding and duplicating some of the
> 
> What does [1] refer to?

I messed up the refs..of course...it was just a reference to your thread
where the number of rates where staticallty raised to 64.

> 
> > functionalities already provided by SCMI iterators, though.
> >
> > Patch 7-10 implement such optimization instead by:
> >
> >  - reworking core SCMI iterators to support bound enumerations
> >  - use such new bound iterators to perform the minimum number of queries
> >    in order to ony retrieve min an max rate
> >
> > As a final result now the rates enumeration triggered by the CLK SCMI
> > driver, while still allocating for all the existent rates, miminize the
> > number of SCMI CLK_DESCRIBE_RATE messages needed to obtain min and max.
> >
> > Finally, patch 11 introduces a new clock protocol operation to be able to
> > trigger anytime on demand a full enumeration and obtain the full list of
> > rates when needed, not only min/max: this latter method is really only used
> > currently by some dowstream SCMI Test driver of mine.
> >
> > Based on v7.0-rc1.
> >
> > Tested on JUNO and an emulated environment.
> 
> Thank you, this removes the need for increasing SCMI_MAX_NUM_RATES on
> R-Car X5H, while decreasing memory usage.
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 

Thanks for testing in the real world !
Cristian