[PATCH 03/11] firmware: arm_scmi: Simplify clock rates exposed interface
Peng Fan
peng.fan at oss.nxp.com
Fri Feb 27 18:07:28 PST 2026
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 at 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.
> 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];
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
More information about the linux-arm-kernel
mailing list