[PATCH v2 00/13] SCMI Clock rates discovery rework

[PATCH v2 00/13] 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 2-6.

A further bigger optimization suggested in a past series [2] 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-12 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 only 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 13 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-rc3. 
Tested on JUNO and an emulated environment.

Beside addressing a few review comments, in V2:

 - patch [1/13] introduces a fix for the pre-existing rounding algorithm,
   before relocating the algorithm logic as alreday done in V1.
 - patch [8/13] hardens clock protocol initialization by adding some
   missing retval checks

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
---
v1 --> v2
 - Rebaed on v7.0-rc3
 - Added a Fixes patch to rectify bug in rounding algo
 - Removed useless parenthesis in macros
 - Collected a few Reviewed-by tags
 - Clarified commit message

Cristian Marussi (13):
  clk: scmi: Fix clock rate rounding
  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 protocol initialization
  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     | 316 ++++++++++++++++++++------
 drivers/firmware/arm_scmi/driver.c    |  73 ++++--
 drivers/firmware/arm_scmi/protocols.h |  13 +-
 include/linux/scmi_protocol.h         |  29 ++-
 5 files changed, 329 insertions(+), 150 deletions(-)

-- 
2.53.0

[PATCH v2 01/13] clk: scmi: Fix clock rate rounding

[Cristian Marussi]

While the do_div() helper used for rounding expects its divisor argument
to be a 32bits quantity, the currently provided divisor parameter is a
64bit value that, as a consequence, is silently truncated and a possible
source of bugs.

Fix by using the proper div64_ul helper.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Fixes: 7a8655e19bdb ("clk: scmi: Fix the rounding of clock rate")
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/clk/clk-scmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 6b286ea6f121..b6a12f3bc123 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -10,9 +10,9 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
-#include <asm/div64.h>
 
 #define NOT_ATOMIC	false
 #define ATOMIC		true
@@ -83,7 +83,7 @@ static int scmi_clk_determine_rate(struct clk_hw *hw,
 
 	ftmp = req->rate - fmin;
 	ftmp += clk->info->range.step_size - 1; /* to round up */
-	do_div(ftmp, clk->info->range.step_size);
+	ftmp = div64_ul(ftmp, clk->info->range.step_size);
 
 	req->rate = ftmp * clk->info->range.step_size + fmin;
 
-- 
2.53.0

Re: [PATCH v2 01/13] clk: scmi: Fix clock rate rounding

[Geert Uytterhoeven]

Hi Cristian,

On Tue, 10 Mar 2026 at 19:40, Cristian Marussi <cristian.marussi@arm.com> wrote:
> While the do_div() helper used for rounding expects its divisor argument
> to be a 32bits quantity, the currently provided divisor parameter is a
> 64bit value that, as a consequence, is silently truncated and a possible
> source of bugs.
>
> Fix by using the proper div64_ul helper.
>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Fixes: 7a8655e19bdb ("clk: scmi: Fix the rounding of clock rate")
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks for your patch!

> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c

> @@ -83,7 +83,7 @@ static int scmi_clk_determine_rate(struct clk_hw *hw,
>
>         ftmp = req->rate - fmin;
>         ftmp += clk->info->range.step_size - 1; /* to round up */
> -       do_div(ftmp, clk->info->range.step_size);
> +       ftmp = div64_ul(ftmp, clk->info->range.step_size);

include/linux/math64.h has:

    #if BITS_PER_LONG == 64
    #define div64_ul(x, y)   div64_u64((x), (y))
    #elif BITS_PER_LONG == 32
    #define div64_ul(x, y)   div_u64((x), (y))
    #endif

I.e. div64_ul() is meant for the case where the divisor is unsigned
long.  Hence div64_ul() may still truncate step_size on 32-bit
platforms, and thus should use div64_u64() unconditionally.

I am aware clock rates are "unsigned long" on 32-bit platforms, and
thus cannot support rates that do not fit in a 32-bit value.
If that is the reason you are using div64_ul(), it should be documented
properly.  And probably the SCMI core code should reject any rate values
(incl. min, max, step) that do not fit in unsigned long, as such clocks
cannot be used on 32-bit platforms.

>
>         req->rate = ftmp * clk->info->range.step_size + fmin;
>

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 v2 01/13] clk: scmi: Fix clock rate rounding

[Cristian Marussi]

On Wed, Mar 11, 2026 at 12:30:34PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Tue, 10 Mar 2026 at 19:40, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > While the do_div() helper used for rounding expects its divisor argument
> > to be a 32bits quantity, the currently provided divisor parameter is a
> > 64bit value that, as a consequence, is silently truncated and a possible
> > source of bugs.
> >
> > Fix by using the proper div64_ul helper.
> >
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: linux-clk@vger.kernel.org
> > Fixes: 7a8655e19bdb ("clk: scmi: Fix the rounding of clock rate")
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> 
> > @@ -83,7 +83,7 @@ static int scmi_clk_determine_rate(struct clk_hw *hw,
> >
> >         ftmp = req->rate - fmin;
> >         ftmp += clk->info->range.step_size - 1; /* to round up */
> > -       do_div(ftmp, clk->info->range.step_size);
> > +       ftmp = div64_ul(ftmp, clk->info->range.step_size);
> 
> include/linux/math64.h has:
> 
>     #if BITS_PER_LONG == 64
>     #define div64_ul(x, y)   div64_u64((x), (y))
>     #elif BITS_PER_LONG == 32
>     #define div64_ul(x, y)   div_u64((x), (y))
>     #endif
> 
> I.e. div64_ul() is meant for the case where the divisor is unsigned
> long.  Hence div64_ul() may still truncate step_size on 32-bit
> platforms, and thus should use div64_u64() unconditionally.
> 

Ah...my bad, I missed that...

> I am aware clock rates are "unsigned long" on 32-bit platforms, and
> thus cannot support rates that do not fit in a 32-bit value.
> If that is the reason you are using div64_ul(), it should be documented
> properly.  And probably the SCMI core code should reject any rate values
> (incl. min, max, step) that do not fit in unsigned long, as such clocks
> cannot be used on 32-bit platforms.

As per the SCMI spec Clock rates are reported as 64bit, so I suppose
that, yes I will have to add the checks you mentioned to avoid trying to
fit a reported clock rate where the upper 32bits are not zero into an
unsigned long on a 32bit machine...

Seems like there will be a V3 (together with your other reports..)

Thanks for your reviews !

Cristian

[PATCH v2 02/13] 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 simplifications 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>
---
v1 --> v2
 - use the fixed rounding algo using div64_ul

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..54b55517b759 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2018-2022 ARM Ltd.
  */
 
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/limits.h>
 #include <linux/sort.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 */
+	ftmp = div64_ul(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 v2 03/13] 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>
---
@brian: I'd modify further this clk-scmi driver, with a patch on top of
this series, to properly use your new CLK_ROUNDING_NOOP flag once your
series AND another (already reviewed) series on clk-scmi from Peng are in.
---
 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 b6a12f3bc123..c223e4ef1dd1 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -10,7 +10,6 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/of.h>
-#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
 
@@ -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 */
-	ftmp = div64_ul(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 v2 04/13] firmware: arm_scmi: Simplify clock rates exposed interface

[Cristian Marussi]

Introduce a new internal struct scmi_clock_desc so as to be able to hide,
in the future, some of the needlessly public fields currently kept inside
scmi_clock_info, 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>
---
v1 --> v2
 - removed useless parenthesis
 - reworded comit message
---
 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 54b55517b759..467b13a3a18f 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 */
-	ftmp = div64_ul(ftmp, clk->range.step_size);
+	ftmp += clkd->rates[RATE_STEP] - 1; /* to round up */
+	ftmp = div64_ul(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

Re: [PATCH v2 04/13] firmware: arm_scmi: Simplify clock rates exposed interface

[Peng Fan]

On Tue, Mar 10, 2026 at 06:40:21PM +0000, Cristian Marussi wrote:
>Introduce a new internal struct scmi_clock_desc so as to be able to hide,
>in the future, some of the needlessly public fields currently kept inside
>scmi_clock_info, 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>

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

[PATCH v2 05/13] 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
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v1 --> v2
 - Collected Peng Reviewed-by tag
---
 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

Re: [PATCH v2 05/13] clk: scmi: Use new simplified per-clock rate properties

[Sudeep Holla]

On Tue, Mar 10, 2026 at 06:40:22PM +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.
> 

Hi Stephen/Mike,

If you are satisfied with the clk-scmi changes, I can take this series via the
Arm SoC tree. Please provide your Ack for patches 1, 3, and 5/13 if you are
happy with the changes and with me taking them.

-- 
Regards,
Sudeep

[PATCH v2 06/13] firmware: arm_scmi: Drop unused clock rate interfaces

[Cristian Marussi]

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

Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v1 --> v2
 - Collected Peng Reviewed-by tag
---
 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 v2 07/13] 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 467b13a3a18f..c9b62edce4fd 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

Re: [PATCH v2 07/13] firmware: arm_scmi: Make clock rates allocation dynamic

[Peng Fan]

On Tue, Mar 10, 2026 at 06:40:24PM +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>

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

[PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Cristian Marussi]

Add proper error handling on failure to enumerate clocks features or
rates.

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

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index c9b62edce4fd..bf956305a8fe 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -402,10 +402,16 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 		    SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
 			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, cinfo);
-			if (SUPPORTS_GET_PERMISSIONS(attributes))
-				scmi_clock_get_permissions(ph, clk_id, clk);
+			if (SUPPORTS_PARENT_CLOCK(attributes)) {
+				ret = scmi_clock_possible_parents(ph, clk_id, cinfo);
+				if (ret)
+					return ret;
+			}
+			if (SUPPORTS_GET_PERMISSIONS(attributes)) {
+				ret = scmi_clock_get_permissions(ph, clk_id, clk);
+				if (ret)
+					return ret;
+			}
 			if (SUPPORTS_EXTENDED_CONFIG(attributes))
 				clk->extended_config = true;
 		}
@@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
 	for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
 		cinfo->clkds[clkid].id = clkid;
 		ret = scmi_clock_attributes_get(ph, clkid, cinfo);
-		if (!ret)
-			scmi_clock_describe_rates_get(ph, clkid, cinfo);
+		if (ret)
+			return ret;
+
+		ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
+		if (ret)
+			return ret;
 	}
 
 	if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {
-- 
2.53.0

Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Geert Uytterhoeven]

Hi Cristian,

On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> Add proper error handling on failure to enumerate clocks features or
> rates.
>
> 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

> @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
>         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
>                 cinfo->clkds[clkid].id = clkid;
>                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> -               if (!ret)
> -                       scmi_clock_describe_rates_get(ph, clkid, cinfo);
> +               if (ret)
> +                       return ret;

This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
do not support the SCMI CLOCK_ATTRIBUTES command.
Before, these clocks were still instantiated, but were further unusable.
After, the whole clock driver fails to initialize, and no SCMI clocks
are available at all.

> +
> +               ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> +               if (ret)
> +                       return ret;
>         }
>
>         if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {

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 v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Cristian Marussi]

On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > Add proper error handling on failure to enumerate clocks features or
> > rates.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Hi,

> 
> Thanks for your patch!
> 
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> 
> > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> >                 cinfo->clkds[clkid].id = clkid;
> >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > -               if (!ret)
> > -                       scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > +               if (ret)
> > +                       return ret;
> 
> This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> do not support the SCMI CLOCK_ATTRIBUTES command.
> Before, these clocks were still instantiated, but were further unusable.
> After, the whole clock driver fails to initialize, and no SCMI clocks
> are available at all.

...and this is exactly what I feared while doing this sort of hardening :P

So there are a few possible solutions (beside reverting this straight away)

The easy fix would be instead change the above in a

	if (ret)
		continue;

...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
FW releases to fix this :D

Another option could be leave it as it is, since indeed it is the correct enforced
behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
quirk targeting the affected FW releases...

This latter option, though, while enforcing the correct behaviour AND
fixing your R-Car issue, leaves open the door for a number of possible
failures of other unknowingly buggy Vendors similarly deployed firmwares...

...that could be solved with more quirks of course...but...worth it ?

Thoughts ?

Let's see also what @Sudeep thinks about this...

Thanks for testing !

Cristian

Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Sudeep Holla]

On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > Hi Cristian,
> > 
> > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > Add proper error handling on failure to enumerate clocks features or
> > > rates.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> Hi,
> 
> > 
> > Thanks for your patch!
> > 
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > 
> > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > >                 cinfo->clkds[clkid].id = clkid;
> > >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > -               if (!ret)
> > > -                       scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > +               if (ret)
> > > +                       return ret;
> > 
> > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > do not support the SCMI CLOCK_ATTRIBUTES command.
> > Before, these clocks were still instantiated, but were further unusable.
> > After, the whole clock driver fails to initialize, and no SCMI clocks
> > are available at all.
> 
> ...and this is exactly what I feared while doing this sort of hardening :P
> 
> So there are a few possible solutions (beside reverting this straight away)
> 
> The easy fix would be instead change the above in a
> 
> 	if (ret)
> 		continue;
> 
> ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> FW releases to fix this :D
> 
> Another option could be leave it as it is, since indeed it is the correct enforced
> behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> quirk targeting the affected FW releases...
> 
> This latter option, though, while enforcing the correct behaviour AND
> fixing your R-Car issue, leaves open the door for a number of possible
> failures of other unknowingly buggy Vendors similarly deployed firmwares...
> 
> ...that could be solved with more quirks of course...but...worth it ?
> 
> Thoughts ?
> 
> Let's see also what @Sudeep thinks about this...
> 

I prefer to fix it as a quirk to prevent similar issues on newer platforms if
the firmware baselines are derived from it. In the worst case, we can relax
the hardening until we figure out a proper quirk-based solution.

-- 
Regards,
Sudeep

Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Cristian Marussi]

On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > Hi Cristian,
> > > 
> > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > Add proper error handling on failure to enumerate clocks features or
> > > > rates.
> > > >
> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > 
> > Hi,
> > 
> > > 
> > > Thanks for your patch!
> > > 
> > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > 
> > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > >                 cinfo->clkds[clkid].id = clkid;
> > > >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > -               if (!ret)
> > > > -                       scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > +               if (ret)
> > > > +                       return ret;
> > > 
> > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > do not support the SCMI CLOCK_ATTRIBUTES command.
> > > Before, these clocks were still instantiated, but were further unusable.
> > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > are available at all.
> > 
> > ...and this is exactly what I feared while doing this sort of hardening :P
> > 
> > So there are a few possible solutions (beside reverting this straight away)
> > 
> > The easy fix would be instead change the above in a
> > 
> > 	if (ret)
> > 		continue;
> > 
> > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > FW releases to fix this :D
> > 
> > Another option could be leave it as it is, since indeed it is the correct enforced
> > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > quirk targeting the affected FW releases...
> > 
> > This latter option, though, while enforcing the correct behaviour AND
> > fixing your R-Car issue, leaves open the door for a number of possible
> > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > 
> > ...that could be solved with more quirks of course...but...worth it ?
> > 
> > Thoughts ?
> > 
> > Let's see also what @Sudeep thinks about this...
> > 
> 
> I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> the firmware baselines are derived from it. In the worst case, we can relax
> the hardening until we figure out a proper quirk-based solution.

Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
Geert with proper versioning....so I can check that there are no
surprises round the (quirked) corner...

Thanks,
Cristian

Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Geert Uytterhoeven]

Hi Cristian,

On Thu, 12 Mar 2026 at 17:36, Cristian Marussi <cristian.marussi@arm.com> wrote:
> On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> > On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > > Hi Cristian,
> > > >
> > > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > > Add proper error handling on failure to enumerate clocks features or
> > > > > rates.
> > > > >
> > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > >
> > > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > >
> > > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > > >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > > >                 cinfo->clkds[clkid].id = clkid;
> > > > >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > > -               if (!ret)
> > > > > -                       scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > >
> > > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > > do not support the SCMI CLOCK_ATTRIBUTES command.

Apparently it is not just CLOCK_ATTRIBUTES, but also
CLOCK_DESCRIBE_RATES.

> > > > Before, these clocks were still instantiated, but were further unusable.
> > > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > > are available at all.
> > >
> > > ...and this is exactly what I feared while doing this sort of hardening :P
> > >
> > > So there are a few possible solutions (beside reverting this straight away)
> > >
> > > The easy fix would be instead change the above in a
> > >
> > >     if (ret)
> > >             continue;
> > >
> > > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > > FW releases to fix this :D
> > >
> > > Another option could be leave it as it is, since indeed it is the correct enforced
> > > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > > quirk targeting the affected FW releases...
> > >
> > > This latter option, though, while enforcing the correct behaviour AND
> > > fixing your R-Car issue, leaves open the door for a number of possible
> > > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > >
> > > ...that could be solved with more quirks of course...but...worth it ?
> > >
> > > Thoughts ?
> > >
> > > Let's see also what @Sudeep thinks about this...
> >
> > I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> > the firmware baselines are derived from it. In the worst case, we can relax
> > the hardening until we figure out a proper quirk-based solution.
>
> Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
> Geert with proper versioning....so I can check that there are no
> surprises round the (quirked) corner...

Unfortunately you cannot "continue" from a quirk, without resorting
to a goto, so I sent a fix: "[PATCH] firmware: arm_scmi: Support loop
control in quirk code snippets"[1].

Then I came up with the following preliminary (have to check more
firmware versions) quirk (Gmail whitespace-damaged):

diff --git a/drivers/firmware/arm_scmi/clock.c
b/drivers/firmware/arm_scmi/clock.c
index f62f9492bd42afbc..6f2af6e9084836c6 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
clk_protocol_events = {
        .num_events = ARRAY_SIZE(clk_events),
 };

+#define QUIRK_RCAR_X5H_NO_ATTRIBUTES                                   \
+       ({                                                              \
+               if (ret == -EREMOTEIO || ret == -EOPNOTSUPP)            \
+                       continue;                                       \
+       })
+
+#define QUIRK_RCAR_X5H_NO_RATES
         \
+       ({                                                              \
+               if (ret == -EOPNOTSUPP)                                 \
+                       ret = 0;                                        \
+       })
+
 static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
 {
        int clkid, ret;
@@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
struct scmi_protocol_handle *ph)
        for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
                cinfo->clkds[clkid].id = clkid;
                ret = scmi_clock_attributes_get(ph, clkid, cinfo);
+               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
QUIRK_RCAR_X5H_NO_ATTRIBUTES);
                if (ret)
                        return ret;

                ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
+               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
QUIRK_RCAR_X5H_NO_RATES);
                if (ret)
                        return ret;
        }
diff --git a/drivers/firmware/arm_scmi/quirks.c
b/drivers/firmware/arm_scmi/quirks.c
index 3772139a758c8a78..5a69f119e1b6c806 100644
--- a/drivers/firmware/arm_scmi/quirks.c
+++ b/drivers/firmware/arm_scmi/quirks.c
@@ -172,6 +172,8 @@ struct scmi_quirk {
 /* Global Quirks Definitions */
 DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
 DEFINE_SCMI_QUIRK(perf_level_get_fc_force, "Qualcomm", NULL, "0x20000-");
+DEFINE_SCMI_QUIRK(clock_rcar_x5h_no_attributes, "Renesas", NULL, "0x10a0000",
+                 "renesas,r8a78000");

 /*
  * Quirks Pointers Array
@@ -182,6 +184,7 @@ DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
"Qualcomm", NULL, "0x20000-");
 static struct scmi_quirk *scmi_quirks_table[] = {
        __DECLARE_SCMI_QUIRK_ENTRY(clock_rates_triplet_out_of_spec),
        __DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
+       __DECLARE_SCMI_QUIRK_ENTRY(clock_rcar_x5h_no_attributes),
        NULL
 };

diff --git a/drivers/firmware/arm_scmi/quirks.h
b/drivers/firmware/arm_scmi/quirks.h
index 74bf6406dd043049..13f28d13bbd74d4c 100644
--- a/drivers/firmware/arm_scmi/quirks.h
+++ b/drivers/firmware/arm_scmi/quirks.h
@@ -48,5 +48,6 @@ static inline void scmi_quirks_enable(struct device
*dev, const char *vend,
 /* Quirk delarations */
 DECLARE_SCMI_QUIRK(clock_rates_triplet_out_of_spec);
 DECLARE_SCMI_QUIRK(perf_level_get_fc_force);
+DECLARE_SCMI_QUIRK(clock_rcar_x5h_no_attributes);

 #endif /* _SCMI_QUIRKS_H */

Does that look like what you have in mind?
Thanks!

[1] https://lore.kernel.org/51de914cddef8fa86c2e7dd5397e5df759c45464.1773675224.git.geert+renesas@glider.be/

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 v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Cristian Marussi]

On Mon, Mar 16, 2026 at 04:50:17PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Thu, 12 Mar 2026 at 17:36, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> > > On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > > > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > > > Hi Cristian,
> > > > >
> > > > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > > > Add proper error handling on failure to enumerate clocks features or
> > > > > > rates.
> > > > > >
> > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Hi Geert,

> > > >
> > > > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > > >
> > > > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > > > >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > > > >                 cinfo->clkds[clkid].id = clkid;
> > > > > >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > > > -               if (!ret)
> > > > > > -                       scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > > > +               if (ret)
> > > > > > +                       return ret;
> > > > >
> > > > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > > > do not support the SCMI CLOCK_ATTRIBUTES command.
> 
> Apparently it is not just CLOCK_ATTRIBUTES, but also
> CLOCK_DESCRIBE_RATES.
> 

I was indeed suspicious that I could have opened a can of worms by
more strictly enfrocing these...

> > > > > Before, these clocks were still instantiated, but were further unusable.
> > > > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > > > are available at all.
> > > >
> > > > ...and this is exactly what I feared while doing this sort of hardening :P
> > > >
> > > > So there are a few possible solutions (beside reverting this straight away)
> > > >
> > > > The easy fix would be instead change the above in a
> > > >
> > > >     if (ret)
> > > >             continue;
> > > >
> > > > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > > > FW releases to fix this :D
> > > >
> > > > Another option could be leave it as it is, since indeed it is the correct enforced
> > > > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > > > quirk targeting the affected FW releases...
> > > >
> > > > This latter option, though, while enforcing the correct behaviour AND
> > > > fixing your R-Car issue, leaves open the door for a number of possible
> > > > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > > >
> > > > ...that could be solved with more quirks of course...but...worth it ?
> > > >
> > > > Thoughts ?
> > > >
> > > > Let's see also what @Sudeep thinks about this...
> > >
> > > I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> > > the firmware baselines are derived from it. In the worst case, we can relax
> > > the hardening until we figure out a proper quirk-based solution.
> >
> > Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
> > Geert with proper versioning....so I can check that there are no
> > surprises round the (quirked) corner...
> 
> Unfortunately you cannot "continue" from a quirk, without resorting
> to a goto, so I sent a fix: "[PATCH] firmware: arm_scmi: Support loop
> control in quirk code snippets"[1].
> 

Yes ... just realized that this afternoon when trying to draft a
quirk... (see other thread)

> Then I came up with the following preliminary (have to check more
> firmware versions) quirk (Gmail whitespace-damaged):
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c
> b/drivers/firmware/arm_scmi/clock.c
> index f62f9492bd42afbc..6f2af6e9084836c6 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
> clk_protocol_events = {
>         .num_events = ARRAY_SIZE(clk_events),
>  };
> 
> +#define QUIRK_RCAR_X5H_NO_ATTRIBUTES                                   \
> +       ({                                                              \
> +               if (ret == -EREMOTEIO || ret == -EOPNOTSUPP)            \
> +                       continue;                                       \
> +       })
> +
> +#define QUIRK_RCAR_X5H_NO_RATES
>          \
> +       ({                                                              \
> +               if (ret == -EOPNOTSUPP)                                 \
> +                       ret = 0;                                        \
> +       })
> +
>  static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
>  {
>         int clkid, ret;
> @@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
> struct scmi_protocol_handle *ph)
>         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
>                 cinfo->clkds[clkid].id = clkid;
>                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> QUIRK_RCAR_X5H_NO_ATTRIBUTES);
>                 if (ret)
>                         return ret;
> 
>                 ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> QUIRK_RCAR_X5H_NO_RATES);
>                 if (ret)
>                         return ret;
>         }
> diff --git a/drivers/firmware/arm_scmi/quirks.c
> b/drivers/firmware/arm_scmi/quirks.c
> index 3772139a758c8a78..5a69f119e1b6c806 100644
> --- a/drivers/firmware/arm_scmi/quirks.c
> +++ b/drivers/firmware/arm_scmi/quirks.c
> @@ -172,6 +172,8 @@ struct scmi_quirk {
>  /* Global Quirks Definitions */
>  DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
>  DEFINE_SCMI_QUIRK(perf_level_get_fc_force, "Qualcomm", NULL, "0x20000-");
> +DEFINE_SCMI_QUIRK(clock_rcar_x5h_no_attributes, "Renesas", NULL, "0x10a0000",
> +                 "renesas,r8a78000");
> 
>  /*
>   * Quirks Pointers Array
> @@ -182,6 +184,7 @@ DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> "Qualcomm", NULL, "0x20000-");
>  static struct scmi_quirk *scmi_quirks_table[] = {
>         __DECLARE_SCMI_QUIRK_ENTRY(clock_rates_triplet_out_of_spec),
>         __DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
> +       __DECLARE_SCMI_QUIRK_ENTRY(clock_rcar_x5h_no_attributes),
>         NULL
>  };
> 
> diff --git a/drivers/firmware/arm_scmi/quirks.h
> b/drivers/firmware/arm_scmi/quirks.h
> index 74bf6406dd043049..13f28d13bbd74d4c 100644
> --- a/drivers/firmware/arm_scmi/quirks.h
> +++ b/drivers/firmware/arm_scmi/quirks.h
> @@ -48,5 +48,6 @@ static inline void scmi_quirks_enable(struct device
> *dev, const char *vend,
>  /* Quirk delarations */
>  DECLARE_SCMI_QUIRK(clock_rates_triplet_out_of_spec);
>  DECLARE_SCMI_QUIRK(perf_level_get_fc_force);
> +DECLARE_SCMI_QUIRK(clock_rcar_x5h_no_attributes);
> 
>  #endif /* _SCMI_QUIRKS_H */
> 
> Does that look like what you have in mind?
> Thanks!

Yes in quirk I was only addressing NOT_ATTRIBUTES and mimicing the old
behaviour with continue, BUT if the set of clocks not supporting attributes
and the set of clocks not suppporting rates is disjoint, I feel we need your
double quirks :P

If you are still finding out the exact FW versions that are failing maybe
it is better if you carry on and test the quirk-framework fix above together
with your quirks and we can make sure to pick all up together...

...OR maybe better I can also drop for now my offending patch that breaks
your FW from my V3 series and you can pick it up and post it later with
your quirks and the Quirk framework fix you propsoed so that we are sure
that we dont break anything while fixing all of this...

Also because we are already in V4 and I dont want to risk to post the
breaking fix (that was at the end broke since forever) BUT not the quirks...

Let's see what @Sudeep thinks

Thanks,
Cristian

Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Geert Uytterhoeven]

Hi Cristian,

On Mon, 16 Mar 2026 at 17:14, Cristian Marussi <cristian.marussi@arm.com> wrote:
> On Mon, Mar 16, 2026 at 04:50:17PM +0100, Geert Uytterhoeven wrote:
> > On Thu, 12 Mar 2026 at 17:36, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> > > > On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > > > > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > > > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > > > > Add proper error handling on failure to enumerate clocks features or
> > > > > > > rates.
> > > > > > >
> > > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

> > > > > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > > > >
> > > > > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > > > > >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > > > > >                 cinfo->clkds[clkid].id = clkid;
> > > > > > >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > > > > -               if (!ret)
> > > > > > > -                       scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > > > > +               if (ret)
> > > > > > > +                       return ret;
> > > > > >
> > > > > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > > > > do not support the SCMI CLOCK_ATTRIBUTES command.
> >
> > Apparently it is not just CLOCK_ATTRIBUTES, but also
> > CLOCK_DESCRIBE_RATES.
>
> I was indeed suspicious that I could have opened a can of worms by
> more strictly enfrocing these...
>
> > > > > > Before, these clocks were still instantiated, but were further unusable.
> > > > > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > > > > are available at all.
> > > > >
> > > > > ...and this is exactly what I feared while doing this sort of hardening :P
> > > > >
> > > > > So there are a few possible solutions (beside reverting this straight away)
> > > > >
> > > > > The easy fix would be instead change the above in a
> > > > >
> > > > >     if (ret)
> > > > >             continue;
> > > > >
> > > > > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > > > > FW releases to fix this :D
> > > > >
> > > > > Another option could be leave it as it is, since indeed it is the correct enforced
> > > > > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > > > > quirk targeting the affected FW releases...
> > > > >
> > > > > This latter option, though, while enforcing the correct behaviour AND
> > > > > fixing your R-Car issue, leaves open the door for a number of possible
> > > > > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > > > >
> > > > > ...that could be solved with more quirks of course...but...worth it ?
> > > > >
> > > > > Thoughts ?
> > > > >
> > > > > Let's see also what @Sudeep thinks about this...
> > > >
> > > > I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> > > > the firmware baselines are derived from it. In the worst case, we can relax
> > > > the hardening until we figure out a proper quirk-based solution.
> > >
> > > Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
> > > Geert with proper versioning....so I can check that there are no
> > > surprises round the (quirked) corner...
> >
> > Unfortunately you cannot "continue" from a quirk, without resorting
> > to a goto, so I sent a fix: "[PATCH] firmware: arm_scmi: Support loop
> > control in quirk code snippets"[1].
>
> Yes ... just realized that this afternoon when trying to draft a
> quirk... (see other thread)
>
> > Then I came up with the following preliminary (have to check more
> > firmware versions) quirk (Gmail whitespace-damaged):
> >
> > diff --git a/drivers/firmware/arm_scmi/clock.c
> > b/drivers/firmware/arm_scmi/clock.c
> > index f62f9492bd42afbc..6f2af6e9084836c6 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
> > clk_protocol_events = {
> >         .num_events = ARRAY_SIZE(clk_events),
> >  };
> >
> > +#define QUIRK_RCAR_X5H_NO_ATTRIBUTES                                   \
> > +       ({                                                              \
> > +               if (ret == -EREMOTEIO || ret == -EOPNOTSUPP)            \
> > +                       continue;                                       \
> > +       })
> > +
> > +#define QUIRK_RCAR_X5H_NO_RATES
> >          \
> > +       ({                                                              \
> > +               if (ret == -EOPNOTSUPP)                                 \
> > +                       ret = 0;                                        \
> > +       })
> > +
> >  static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> >  {
> >         int clkid, ret;
> > @@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
> > struct scmi_protocol_handle *ph)
> >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> >                 cinfo->clkds[clkid].id = clkid;
> >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> >                 if (ret)
> >                         return ret;
> >
> >                 ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > QUIRK_RCAR_X5H_NO_RATES);
> >                 if (ret)
> >                         return ret;
> >         }

> > Does that look like what you have in mind?
> > Thanks!
>
> Yes in quirk I was only addressing NOT_ATTRIBUTES and mimicing the old
> behaviour with continue, BUT if the set of clocks not supporting attributes
> and the set of clocks not suppporting rates is disjoint, I feel we need your
> double quirks :P

I could have used

    SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_ATTRIBUTES);

after both scmi_clock_attributes_get() and
scmi_clock_describe_rates_get(), but I wanted to keep the check as
strict as possible: the former returns two error codes to ignore,
the latter only one.

> If you are still finding out the exact FW versions that are failing maybe
> it is better if you carry on and test the quirk-framework fix above together
> with your quirks and we can make sure to pick all up together...

It is not urgent, as R-Car X5H SCMI support is not yet upstream.

> ...OR maybe better I can also drop for now my offending patch that breaks
> your FW from my V3 series and you can pick it up and post it later with
> your quirks and the Quirk framework fix you propsoed so that we are sure
> that we dont break anything while fixing all of this...

While there is indeed a chance that this hardening regresses on
platforms that are already upstream...

> Also because we are already in V4 and I dont want to risk to post the
> breaking fix (that was at the end broke since forever) BUT not the quirks...

s/in V4/at rc4/?

> Let's see what @Sudeep thinks

OK.

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 v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Cristian Marussi]

On Mon, Mar 16, 2026 at 05:35:26PM +0100, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Mon, 16 Mar 2026 at 17:14, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > On Mon, Mar 16, 2026 at 04:50:17PM +0100, Geert Uytterhoeven wrote:
> > > On Thu, 12 Mar 2026 at 17:36, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:
> > > > > On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:
> > > > > > On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:
> > > > > > > On Tue, 10 Mar 2026 at 19:56, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > > > > > Add proper error handling on failure to enumerate clocks features or
> > > > > > > > rates.
> > > > > > > >
> > > > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> > > > > > > > --- a/drivers/firmware/arm_scmi/clock.c
> > > > > > > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > > > > >
> > > > > > > > @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > > > > > >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > > > > > > >                 cinfo->clkds[clkid].id = clkid;
> > > > > > > >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > > > > > > -               if (!ret)
> > > > > > > > -                       scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > > > > > > +               if (ret)
> > > > > > > > +                       return ret;
> > > > > > >
> > > > > > > This change breaks R-Car X5H with SCP FW SDKv4.28.0, as some clocks
> > > > > > > do not support the SCMI CLOCK_ATTRIBUTES command.
> > >
> > > Apparently it is not just CLOCK_ATTRIBUTES, but also
> > > CLOCK_DESCRIBE_RATES.
> >
> > I was indeed suspicious that I could have opened a can of worms by
> > more strictly enfrocing these...
> >
> > > > > > > Before, these clocks were still instantiated, but were further unusable.
> > > > > > > After, the whole clock driver fails to initialize, and no SCMI clocks
> > > > > > > are available at all.
> > > > > >
> > > > > > ...and this is exactly what I feared while doing this sort of hardening :P
> > > > > >
> > > > > > So there are a few possible solutions (beside reverting this straight away)
> > > > > >
> > > > > > The easy fix would be instead change the above in a
> > > > > >
> > > > > >     if (ret)
> > > > > >             continue;
> > > > > >
> > > > > > ...with a bit of annoying accompanying FW_BUG logs, of course, to cause future
> > > > > > FW releases to fix this :D
> > > > > >
> > > > > > Another option could be leave it as it is, since indeed it is the correct enforced
> > > > > > behaviour, being CLOCK_ATTRIBUTES a mandatory command, BUT add on top an ad-hoc SCMI
> > > > > > quirk targeting the affected FW releases...
> > > > > >
> > > > > > This latter option, though, while enforcing the correct behaviour AND
> > > > > > fixing your R-Car issue, leaves open the door for a number of possible
> > > > > > failures of other unknowingly buggy Vendors similarly deployed firmwares...
> > > > > >
> > > > > > ...that could be solved with more quirks of course...but...worth it ?
> > > > > >
> > > > > > Thoughts ?
> > > > > >
> > > > > > Let's see also what @Sudeep thinks about this...
> > > > >
> > > > > I prefer to fix it as a quirk to prevent similar issues on newer platforms if
> > > > > the firmware baselines are derived from it. In the worst case, we can relax
> > > > > the hardening until we figure out a proper quirk-based solution.
> > > >
> > > > Ok, I can post a V3 with a dummy quirk 'template' RFC to be filled by
> > > > Geert with proper versioning....so I can check that there are no
> > > > surprises round the (quirked) corner...
> > >
> > > Unfortunately you cannot "continue" from a quirk, without resorting
> > > to a goto, so I sent a fix: "[PATCH] firmware: arm_scmi: Support loop
> > > control in quirk code snippets"[1].
> >
> > Yes ... just realized that this afternoon when trying to draft a
> > quirk... (see other thread)
> >
> > > Then I came up with the following preliminary (have to check more
> > > firmware versions) quirk (Gmail whitespace-damaged):
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c
> > > b/drivers/firmware/arm_scmi/clock.c
> > > index f62f9492bd42afbc..6f2af6e9084836c6 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
> > > clk_protocol_events = {
> > >         .num_events = ARRAY_SIZE(clk_events),
> > >  };
> > >
> > > +#define QUIRK_RCAR_X5H_NO_ATTRIBUTES                                   \
> > > +       ({                                                              \
> > > +               if (ret == -EREMOTEIO || ret == -EOPNOTSUPP)            \
> > > +                       continue;                                       \
> > > +       })
> > > +
> > > +#define QUIRK_RCAR_X5H_NO_RATES
> > >          \
> > > +       ({                                                              \
> > > +               if (ret == -EOPNOTSUPP)                                 \
> > > +                       ret = 0;                                        \
> > > +       })
> > > +
> > >  static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > >  {
> > >         int clkid, ret;
> > > @@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
> > > struct scmi_protocol_handle *ph)
> > >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > >                 cinfo->clkds[clkid].id = clkid;
> > >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> > >                 if (ret)
> > >                         return ret;
> > >
> > >                 ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_RATES);
> > >                 if (ret)
> > >                         return ret;
> > >         }
> 
> > > Does that look like what you have in mind?
> > > Thanks!
> >
> > Yes in quirk I was only addressing NOT_ATTRIBUTES and mimicing the old
> > behaviour with continue, BUT if the set of clocks not supporting attributes
> > and the set of clocks not suppporting rates is disjoint, I feel we need your
> > double quirks :P
> 
> I could have used
> 
>     SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> 
> after both scmi_clock_attributes_get() and
> scmi_clock_describe_rates_get(), but I wanted to keep the check as
> strict as possible: the former returns two error codes to ignore,
> the latter only one.
> 
> > If you are still finding out the exact FW versions that are failing maybe
> > it is better if you carry on and test the quirk-framework fix above together
> > with your quirks and we can make sure to pick all up together...
> 
> It is not urgent, as R-Car X5H SCMI support is not yet upstream.

Ah ok.

> 
> > ...OR maybe better I can also drop for now my offending patch that breaks
> > your FW from my V3 series and you can pick it up and post it later with
> > your quirks and the Quirk framework fix you propsoed so that we are sure
> > that we dont break anything while fixing all of this...
> 
> While there is indeed a chance that this hardening regresses on
> platforms that are already upstream...

Yes indeed...

> 
> > Also because we are already in V4 and I dont want to risk to post the
> > breaking fix (that was at the end broke since forever) BUT not the quirks...
> 
> s/in V4/at rc4/?

yep... -rc4 I meant..

> 
> > Let's see what @Sudeep thinks
> 
> OK.
>

Thanks,
Cristian

Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Geert Uytterhoeven]

Hi Cristian,

On Mon, 16 Mar 2026 at 17:35, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, 16 Mar 2026 at 17:14, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > On Mon, Mar 16, 2026 at 04:50:17PM +0100, Geert Uytterhoeven wrote:
> > > Then I came up with the following preliminary (have to check more
> > > firmware versions) quirk (Gmail whitespace-damaged):
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c
> > > b/drivers/firmware/arm_scmi/clock.c
> > > index f62f9492bd42afbc..6f2af6e9084836c6 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -1230,6 +1230,18 @@ static const struct scmi_protocol_events
> > > clk_protocol_events = {
> > >         .num_events = ARRAY_SIZE(clk_events),
> > >  };
> > >
> > > +#define QUIRK_RCAR_X5H_NO_ATTRIBUTES                                   \
> > > +       ({                                                              \
> > > +               if (ret == -EREMOTEIO || ret == -EOPNOTSUPP)            \
> > > +                       continue;                                       \
> > > +       })
> > > +
> > > +#define QUIRK_RCAR_X5H_NO_RATES
> > >          \
> > > +       ({                                                              \
> > > +               if (ret == -EOPNOTSUPP)                                 \
> > > +                       ret = 0;                                        \
> > > +       })
> > > +
> > >  static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > >  {
> > >         int clkid, ret;
> > > @@ -1254,10 +1266,12 @@ static int scmi_clock_protocol_init(const
> > > struct scmi_protocol_handle *ph)
> > >         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
> > >                 cinfo->clkds[clkid].id = clkid;
> > >                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> > > +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_ATTRIBUTES);
> > >                 if (ret)
> > >                         return ret;
> > >
> > >                 ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> > > +               SCMI_QUIRK(clock_rcar_x5h_no_attributes,
> > > QUIRK_RCAR_X5H_NO_RATES);
> > >                 if (ret)
> > >                         return ret;
> > >         }
>
> > > Does that look like what you have in mind?
> > > Thanks!
> >
> > Yes in quirk I was only addressing NOT_ATTRIBUTES and mimicing the old
> > behaviour with continue, BUT if the set of clocks not supporting attributes
> > and the set of clocks not suppporting rates is disjoint, I feel we need your
> > double quirks :P
>
> I could have used
>
>     SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_ATTRIBUTES);
>
> after both scmi_clock_attributes_get() and
> scmi_clock_describe_rates_get(), but I wanted to keep the check as
> strict as possible: the former returns two error codes to ignore,
> the latter only one.

So these are two mitigations:

    #define QUIRK_RCAR_X5H_NO_ATTRIBUTES    ({ ... })
    SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_ATTRIBUTES);

and

    #define QUIRK_RCAR_X5H_NO_RATES         ({ ... })
    SCMI_QUIRK(clock_rcar_x5h_no_attributes, QUIRK_RCAR_X5H_NO_RATES);

gated by a single quirk entry clock_rcar_x5h_no_attributes:

    DECLARE_SCMI_QUIRK(clock_rcar_x5h_no_attributes);
    DEFINE_SCMI_QUIRK(clock_rcar_x5h_no_attributes, "Renesas", NULL,
                      "0x10a0000", "renesas,r8a78000");
    __DECLARE_SCMI_QUIRK_ENTRY(clock_rcar_x5h_no_attributes),

In general, when a specific SCMI implementation has multiple quirks
and needs multiple mitigations, do you prefer to have individual
entries for each quirk plus mitigation, or just a single entry with
multiple mitigations (which may not be limited to a single protocol,
unlike my example above)?

Thanks!


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 v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Marek Szyprowski]

On 10.03.2026 19:40, Cristian Marussi wrote:
> Add proper error handling on failure to enumerate clocks features or
> rates.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

This patch landed yesterday in linux-next as commit 0d8b0c8068a8 
("firmware: arm_scmi: Harden clock protocol initialization"). In my 
tests I found that it causes a regression on RK3568 Odroid-M1 board 
(arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts), cpufreq and GPU 
device are not probed properly:

# dmesg | grep scmi
scmi_core: SCMI protocol bus registered
arm-scmi arm-scmi.0.auto: Using scmi_smc_transport
arm-scmi arm-scmi.0.auto: SCMI max-rx-timeout: 30ms / max-msg-size: 
104bytes / max-msg: 20
scmi_protocol scmi_dev.1: Enabled polling mode TX channel - prot_id:16
arm-scmi arm-scmi.0.auto: SCMI Notifications - Core Enabled.
arm-scmi arm-scmi.0.auto: Malformed reply - real_sz:8 calc_sz:4  
(loop_num_ret:1)
arm-scmi arm-scmi.0.auto: SCMI Protocol v2.0 'rockchip:' Firmware 
version 0x0
arm-scmi arm-scmi.0.auto: Enabling SCMI Quirk 
[quirk_clock_rates_triplet_out_of_spec]
scmi-clocks scmi_dev.3: probe with driver scmi-clocks failed with error -22

# cat /sys/kernel/debug/devices_deferred
fde60000.gpu
cpufreq-dt

# dmesg | grep fde60000.gpu
rockchip-pm-domain fdd90000.power-management:power-controller: 
sync_state() pending due to fde60000.gpu
panfrost fde60000.gpu: get clock failed -517
panfrost fde60000.gpu: clk init failed -517
panfrost fde60000.gpu: get clock failed -517
panfrost fde60000.gpu: clk init failed -517

...

> ---
>   drivers/firmware/arm_scmi/clock.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index c9b62edce4fd..bf956305a8fe 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -402,10 +402,16 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
>   		    SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
>   			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, cinfo);
> -			if (SUPPORTS_GET_PERMISSIONS(attributes))
> -				scmi_clock_get_permissions(ph, clk_id, clk);
> +			if (SUPPORTS_PARENT_CLOCK(attributes)) {
> +				ret = scmi_clock_possible_parents(ph, clk_id, cinfo);
> +				if (ret)
> +					return ret;
> +			}
> +			if (SUPPORTS_GET_PERMISSIONS(attributes)) {
> +				ret = scmi_clock_get_permissions(ph, clk_id, clk);
> +				if (ret)
> +					return ret;
> +			}
>   			if (SUPPORTS_EXTENDED_CONFIG(attributes))
>   				clk->extended_config = true;
>   		}
> @@ -1143,8 +1149,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
>   	for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
>   		cinfo->clkds[clkid].id = clkid;
>   		ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> -		if (!ret)
> -			scmi_clock_describe_rates_get(ph, clkid, cinfo);
> +		if (ret)
> +			return ret;
> +
> +		ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
> +		if (ret)
> +			return ret;
>   	}
>   
>   	if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) {

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Cristian Marussi]

On Wed, Mar 25, 2026 at 12:02:41PM +0100, Marek Szyprowski wrote:
> On 10.03.2026 19:40, Cristian Marussi wrote:
> > Add proper error handling on failure to enumerate clocks features or
> > rates.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 

Hi Marek,

> This patch landed yesterday in linux-next as commit 0d8b0c8068a8 
> ("firmware: arm_scmi: Harden clock protocol initialization"). In my 
> tests I found that it causes a regression on RK3568 Odroid-M1 board 
> (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts), cpufreq and GPU 
> device are not probed properly:
> 
> # dmesg | grep scmi
> scmi_core: SCMI protocol bus registered
> arm-scmi arm-scmi.0.auto: Using scmi_smc_transport
> arm-scmi arm-scmi.0.auto: SCMI max-rx-timeout: 30ms / max-msg-size: 
> 104bytes / max-msg: 20
> scmi_protocol scmi_dev.1: Enabled polling mode TX channel - prot_id:16
> arm-scmi arm-scmi.0.auto: SCMI Notifications - Core Enabled.
> arm-scmi arm-scmi.0.auto: Malformed reply - real_sz:8 calc_sz:4  
> (loop_num_ret:1)
> arm-scmi arm-scmi.0.auto: SCMI Protocol v2.0 'rockchip:' Firmware 
> version 0x0
> arm-scmi arm-scmi.0.auto: Enabling SCMI Quirk 
> [quirk_clock_rates_triplet_out_of_spec]
> scmi-clocks scmi_dev.3: probe with driver scmi-clocks failed with error -22
> 

Yes there are multiple reports of issues on this hardening, the series
is on hold and wont go into v7.1 as of now...it needs some basic fixes
and various quirks probably to address non-compliant firmwares...

It will be pushed to next again with a few more fixes in the coming
days and then we'll need to figure out how many quirks will be needed on
top of that and if it is acceptable at all...

Thanks for the report,
Cristian

Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Alexander Stein]

Hi,

Am Mittwoch, 25. März 2026, 13:27:48 CET schrieb Cristian Marussi:
> On Wed, Mar 25, 2026 at 12:02:41PM +0100, Marek Szyprowski wrote:
> > On 10.03.2026 19:40, Cristian Marussi wrote:
> > > Add proper error handling on failure to enumerate clocks features or
> > > rates.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > 
> 
> Hi Marek,
> 
> > This patch landed yesterday in linux-next as commit 0d8b0c8068a8 
> > ("firmware: arm_scmi: Harden clock protocol initialization"). In my 
> > tests I found that it causes a regression on RK3568 Odroid-M1 board 
> > (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts), cpufreq and GPU 
> > device are not probed properly:
> > 
> > # dmesg | grep scmi
> > scmi_core: SCMI protocol bus registered
> > arm-scmi arm-scmi.0.auto: Using scmi_smc_transport
> > arm-scmi arm-scmi.0.auto: SCMI max-rx-timeout: 30ms / max-msg-size: 
> > 104bytes / max-msg: 20
> > scmi_protocol scmi_dev.1: Enabled polling mode TX channel - prot_id:16
> > arm-scmi arm-scmi.0.auto: SCMI Notifications - Core Enabled.
> > arm-scmi arm-scmi.0.auto: Malformed reply - real_sz:8 calc_sz:4  
> > (loop_num_ret:1)
> > arm-scmi arm-scmi.0.auto: SCMI Protocol v2.0 'rockchip:' Firmware 
> > version 0x0
> > arm-scmi arm-scmi.0.auto: Enabling SCMI Quirk 
> > [quirk_clock_rates_triplet_out_of_spec]
> > scmi-clocks scmi_dev.3: probe with driver scmi-clocks failed with error -22
> > 
> 
> Yes there are multiple reports of issues on this hardening, the series
> is on hold and wont go into v7.1 as of now...it needs some basic fixes
> and various quirks probably to address non-compliant firmwares...
> 
> It will be pushed to next again with a few more fixes in the coming
> days and then we'll need to figure out how many quirks will be needed on
> top of that and if it is acceptable at all...

Just for the records: imx95 (maybe imx94 as well) is also affected by this.
My board doesn't boot at all, because all the clocks are provided by SCMI.

With this diff I can see it's the 'ext' clock
-->8---
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -1253,8 +1253,11 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
        for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
                cinfo->clkds[clkid].id = clkid;
                ret = scmi_clock_attributes_get(ph, clkid, cinfo);
-               if (ret)
+               if (ret) {
+                       dev_warn(ph->dev, "scmi_clock_attributes_get failed for '%s': %d\n",
+                                cinfo->clkds->info.name, ret);
                        return ret;
+               }
 
                ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
                if (ret)
-->8---
> arm-scmi arm-scmi.0.auto: scmi_clock_attributes_get failed for 'ext': -2
> scmi-clocks scmi_dev.6: probe with driver scmi-clocks failed with error -2

What's the idea of how to proceeed as apparently several platforms are
affected?

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 08/13] firmware: arm_scmi: Harden clock protocol initialization

[Sudeep Holla]

On Thu, Mar 26, 2026 at 09:55:18AM +0100, Alexander Stein wrote:
> Hi,
> 
> Am Mittwoch, 25. März 2026, 13:27:48 CET schrieb Cristian Marussi:
> > On Wed, Mar 25, 2026 at 12:02:41PM +0100, Marek Szyprowski wrote:
> > > On 10.03.2026 19:40, Cristian Marussi wrote:
> > > > Add proper error handling on failure to enumerate clocks features or
> > > > rates.
> > > >
> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > 
> > 
> > Hi Marek,
> > 
> > > This patch landed yesterday in linux-next as commit 0d8b0c8068a8 
> > > ("firmware: arm_scmi: Harden clock protocol initialization"). In my 
> > > tests I found that it causes a regression on RK3568 Odroid-M1 board 
> > > (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts), cpufreq and GPU 
> > > device are not probed properly:
> > > 
> > > # dmesg | grep scmi
> > > scmi_core: SCMI protocol bus registered
> > > arm-scmi arm-scmi.0.auto: Using scmi_smc_transport
> > > arm-scmi arm-scmi.0.auto: SCMI max-rx-timeout: 30ms / max-msg-size: 
> > > 104bytes / max-msg: 20
> > > scmi_protocol scmi_dev.1: Enabled polling mode TX channel - prot_id:16
> > > arm-scmi arm-scmi.0.auto: SCMI Notifications - Core Enabled.
> > > arm-scmi arm-scmi.0.auto: Malformed reply - real_sz:8 calc_sz:4  
> > > (loop_num_ret:1)
> > > arm-scmi arm-scmi.0.auto: SCMI Protocol v2.0 'rockchip:' Firmware 
> > > version 0x0
> > > arm-scmi arm-scmi.0.auto: Enabling SCMI Quirk 
> > > [quirk_clock_rates_triplet_out_of_spec]
> > > scmi-clocks scmi_dev.3: probe with driver scmi-clocks failed with error -22
> > > 
> > 
> > Yes there are multiple reports of issues on this hardening, the series
> > is on hold and wont go into v7.1 as of now...it needs some basic fixes
> > and various quirks probably to address non-compliant firmwares...
> > 
> > It will be pushed to next again with a few more fixes in the coming
> > days and then we'll need to figure out how many quirks will be needed on
> > top of that and if it is acceptable at all...
> 
> Just for the records: imx95 (maybe imx94 as well) is also affected by this.
> My board doesn't boot at all, because all the clocks are provided by SCMI.
> 
> With this diff I can see it's the 'ext' clock
> -->8---
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -1253,8 +1253,11 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
>         for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
>                 cinfo->clkds[clkid].id = clkid;
>                 ret = scmi_clock_attributes_get(ph, clkid, cinfo);
> -               if (ret)
> +               if (ret) {
> +                       dev_warn(ph->dev, "scmi_clock_attributes_get failed for '%s': %d\n",
> +                                cinfo->clkds->info.name, ret);
>                         return ret;
> +               }
>  
>                 ret = scmi_clock_describe_rates_get(ph, clkid, cinfo);
>                 if (ret)
> -->8---
> > arm-scmi arm-scmi.0.auto: scmi_clock_attributes_get failed for 'ext': -2
> > scmi-clocks scmi_dev.6: probe with driver scmi-clocks failed with error -2
> 
> What's the idea of how to proceeed as apparently several platforms are
> affected?
> 

Not exactly answer to the above question, but more discussion here:

https://lore.kernel.org/all/20260324-scmi-clock-fix-v1-v1-1-65c21935824b@nxp.com

-- 
Regards,
Sudeep

[PATCH v2 09/13] 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 bf956305a8fe..5f6b25c7b523 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

Re: [PATCH v2 09/13] firmware: arm_scmi: Harden clock parents discovery

[Peng Fan]

On Tue, Mar 10, 2026 at 06:40:26PM +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>

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

[PATCH v2 10/13] 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 3e76a3204ba4..4bd069da879e 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

Re: [PATCH v2 10/13] firmware: arm_scmi: Refactor iterators internal allocation

[Peng Fan]

On Tue, Mar 10, 2026 at 06:40:27PM +0000, Cristian Marussi wrote:
>Use cleanup handlers to manage iterator data structures.
>
>No functional change.
>
>Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

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

[PATCH v2 11/13] 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 5f6b25c7b523..a7dbb0c4f31c 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -511,8 +511,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 4bd069da879e..3c393a071750 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

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

[Peng Fan]

On Tue, Mar 10, 2026 at 06:40:28PM +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>
>---
>+static void scmi_iterator_cleanup(void *iter)
>+{
>+	struct scmi_iterator *i = iter;

I see you use no_free_ptr for allocation,
Do we need to use __free for i or drop the __free usage in allocation? 

Regards
Peng

>+
>+	i->ph->xops->xfer_put(i->ph, i->t);
>+	kfree(i);
>+}
>+

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

[Cristian Marussi]

On Tue, Mar 17, 2026 at 03:44:02PM +0800, Peng Fan wrote:
> On Tue, Mar 10, 2026 at 06:40:28PM +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>
> >---
> >+static void scmi_iterator_cleanup(void *iter)
> >+{
> >+	struct scmi_iterator *i = iter;
> 
> I see you use no_free_ptr for allocation,
> Do we need to use __free for i or drop the __free usage in allocation? 

Neither I think...iter is allocated by iterator_init() which do use
__free cleanups to simplify error paths, BUT if everything goes fine
iter is returned with no_free_ptr() as a normal pointer and all the
compiler-attached cleanup helpers are removed...

Then you can use in an iterator_run() or iterator_run_bound() and in
borth case it HAS to be manually cleanup by calling this scmi_iterator_cleanup
helper where...

> 
> Regards
> Peng
> 
> >+
> >+	i->ph->xops->xfer_put(i->ph, i->t);
> >+	kfree(i);

..it is finally freed explicitly...

...or I am getting something else wrong around the cleanup helpers :P ?

Thanks,
Cristian

[PATCH v2 12/13] 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>
---
v1 --> v2
 - fixed final ret value in scmi_clock_describe_get
---
 drivers/firmware/arm_scmi/clock.c | 90 +++++++++++++++++++++++++++----
 1 file changed, 81 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index a7dbb0c4f31c..f50689bf5414 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
@@ -489,15 +490,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;
@@ -520,8 +522,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;
@@ -530,7 +532,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,
@@ -550,17 +551,88 @@ 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;
 }
-- 
2.53.0

[PATCH v2 13/13] 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 f50689bf5414..082fb0db8681 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
@@ -475,10 +473,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",
@@ -492,9 +490,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 */
@@ -513,10 +511,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;
 }
@@ -537,7 +535,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))
@@ -548,12 +552,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;
 }
@@ -589,7 +593,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);
@@ -621,16 +625,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);
 	}
 
@@ -735,7 +739,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;
@@ -749,14 +753,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 */
-	ftmp = div64_ul(ftmp, clkd->rates[RATE_STEP]);
+	ftmp += clkd->r.rates[RATE_STEP] - 1; /* to round up */
+	ftmp = div64_ul(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,
@@ -1070,6 +1100,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 v2 13/13] firmware: arm_scmi: Introduce all_rates_get clock operation

[Peng Fan]

On Tue, Mar 10, 2026 at 06:40:30PM +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>

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

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

[Geert Uytterhoeven]

Hi Cristian,

On Tue, 10 Mar 2026 at 19:40, 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 2-6.
>
> A further bigger optimization suggested in a past series [2] 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-12 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 only 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 13 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-rc3.
> Tested on JUNO and an emulated environment.
>
> Beside addressing a few review comments, in V2:
>
>  - patch [1/13] introduces a fix for the pre-existing rounding algorithm,
>    before relocating the algorithm logic as alreday done in V1.
>  - patch [8/13] hardens clock protocol initialization by adding some
>    missing retval checks

Thank you, this removes the need for increasing SCMI_MAX_NUM_RATES on
R-Car X5H, while decreasing memory usage.

As upstream does not support SCMI on R-Car X5H yet, I am ignoring
the hardening for now (it may be fixed by a quirk), so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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