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

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

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