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

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

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

ok...

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

ok

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

[snip] 

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

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

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

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

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

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

Thanks,
Cristian