Re: [PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface
From: Peng Fan <hidden>
Date: 2026-02-28 02:05:43
Also in:
arm-scmi, linux-clk, linux-renesas-soc, lkml
On Fri, Feb 27, 2026 at 03:32:17PM +0000, Cristian Marussi wrote:
quoted hunk ↗ jump to hunk
Move needlessly exposed fields away from scmi_clock_info into the new internal struct scmi_clock_desc while keeping exposed only the two new min_rate and max_rate fields for each clock. No functional change. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/firmware/arm_scmi/clock.c | 145 +++++++++++++++--------------- include/linux/scmi_protocol.h | 2 + 2 files changed, 74 insertions(+), 73 deletions(-)diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c index 54e8b59c3941..f5d1c608f85a 100644 --- a/drivers/firmware/arm_scmi/clock.c +++ b/drivers/firmware/arm_scmi/clock.c@@ -157,13 +157,27 @@ struct scmi_clock_rate_notify_payld {__le32 rate_high; }; +struct scmi_clock_desc { + u32 id; + bool rate_discrete; + unsigned int num_rates; + u64 rates[SCMI_MAX_NUM_RATES]; +#define RATE_MIN 0 +#define RATE_MAX 1 +#define RATE_STEP 2 + struct scmi_clock_info info; +}; + +#define to_desc(p) (container_of((p), struct scmi_clock_desc, info))
Nit: no need parentheses
+
struct clock_info {
int num_clocks;
int max_async_req;
bool notify_rate_changed_cmd;
bool notify_rate_change_requested_cmd;
atomic_t cur_async_req;
- struct scmi_clock_info *clk;
+ struct scmi_clock_desc *clkds;
+#define CLOCK_INFO(c, i) (&(((c)->clkds + (i))->info))Ditto.
quoted hunk ↗ jump to hunk
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 instruct 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_protocolconst 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, u3if (IS_ERR(iter)) return PTR_ERR(iter); - ret = ph->hops->iter_response_run(iter); - - return ret; + return ph->hops->iter_response_run(iter); } static int@@ -352,7 +361,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,u32 attributes; struct scmi_xfer *t; struct scmi_msg_resp_clock_attributes *attr; - struct scmi_clock_info *clk = cinfo->clk + clk_id; + struct scmi_clock_info *clk = CLOCK_INFO(cinfo, clk_id); ret = ph->xops->xfer_get_init(ph, CLOCK_ATTRIBUTES, sizeof(clk_id), sizeof(*attr), &t);@@ -394,7 +403,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,clk->rate_change_requested_notifications = true; if (PROTOCOL_REV_MAJOR(ph->version) >= 0x3) { if (SUPPORTS_PARENT_CLOCK(attributes)) - scmi_clock_possible_parents(ph, clk_id, clk); + scmi_clock_possible_parents(ph, clk_id, cinfo); if (SUPPORTS_GET_PERMISSIONS(attributes)) scmi_clock_get_permissions(ph, clk_id, clk); if (SUPPORTS_EXTENDED_CONFIG(attributes))@@ -424,7 +433,7 @@ static void iter_clk_describe_prepare_message(void *message,struct scmi_msg_clock_describe_rates *msg = message; const struct scmi_clk_ipriv *p = priv; - msg->id = cpu_to_le32(p->clk_id); + msg->id = cpu_to_le32(p->clkd->id); /* Set the number of rates to be skipped/already read */ msg->rate_index = cpu_to_le32(desc_index); }@@ -457,14 +466,14 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,flags = le32_to_cpu(r->num_rates_flags); st->num_remaining = NUM_REMAINING(flags); st->num_returned = NUM_RETURNED(flags); - p->clk->rate_discrete = RATE_DISCRETE(flags); + p->clkd->rate_discrete = RATE_DISCRETE(flags); /* Warn about out of spec replies ... */ - if (!p->clk->rate_discrete && + if (!p->clkd->rate_discrete && (st->num_returned != 3 || st->num_remaining != 0)) { dev_warn(p->dev, "Out-of-spec CLOCK_DESCRIBE_RATES reply for %s - returned:%d remaining:%d rx_len:%zd\n", - p->clk->name, st->num_returned, st->num_remaining, + p->clkd->info.name, st->num_returned, st->num_remaining, st->rx_len); SCMI_QUIRK(clock_rates_triplet_out_of_spec,@@ -479,38 +488,19 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,const void *response, struct scmi_iterator_state *st, void *priv) { - int ret = 0; struct scmi_clk_ipriv *p = priv; const struct scmi_msg_resp_clock_describe_rates *r = response; - if (!p->clk->rate_discrete) { - switch (st->desc_index + st->loop_idx) { - case 0: - p->clk->range.min_rate = RATE_TO_U64(r->rate[0]); - break; - case 1: - p->clk->range.max_rate = RATE_TO_U64(r->rate[1]); - break; - case 2: - p->clk->range.step_size = RATE_TO_U64(r->rate[2]); - break; - default: - ret = -EINVAL; - break; - } - } else { - u64 *rate = &p->clk->list.rates[st->desc_index + st->loop_idx]; + p->clkd->rates[st->desc_index + st->loop_idx] = + RATE_TO_U64(r->rate[st->loop_idx]); + p->clkd->num_rates++; - *rate = RATE_TO_U64(r->rate[st->loop_idx]); - p->clk->list.num_rates++; - } - - return ret; + return 0; } static int scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id, - struct scmi_clock_info *clk) + struct clock_info *cinfo) { int ret; void *iter;@@ -519,9 +509,9 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,.update_state = iter_clk_describe_update_state, .process_response = iter_clk_describe_process_response, }; + struct scmi_clock_desc *clkd = &cinfo->clkds[clk_id]; struct scmi_clk_ipriv cpriv = { - .clk_id = clk_id, - .clk = clk, + .clkd = clkd, .dev = ph->dev, };@@ -536,16 +526,23 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,if (ret) return ret; - if (!clk->rate_discrete) { + /* empty set ? */ + if (!clkd->num_rates) + return 0; + + if (!clkd->rate_discrete) { + clkd->info.max_rate = clkd->rates[RATE_MAX];
Not related to this patch. Just have a question, if a broken firmware returns RATE_MIN, but no RATE_MAX and RATE_STEP, should some sanity checking being added? Regards, Peng