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

Hi Cristian,

On Thu, 12 Mar 2026 at 17:36, Cristian Marussi [off-list ref] wrote:

On Thu, Mar 12, 2026 at 03:33:52PM +0000, Sudeep Holla wrote:

quoted

On Wed, Mar 11, 2026 at 06:45:41PM +0000, Cristian Marussi wrote:

quoted

On Wed, Mar 11, 2026 at 05:59:43PM +0100, Geert Uytterhoeven wrote:

quoted

Hi Cristian,

On Tue, 10 Mar 2026 at 19:56, Cristian Marussi [off-list ref] wrote:

quoted

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

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

quoted

quoted

--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c

quoted

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

quoted

quoted

quoted

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

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