firmware: arm_scmi: get only min/max clock rates

Commit Message

Etienne CARRIERE - foss July 29, 2024, 6:53 a.m. UTC

Remove limitation of 16 clock rates max for discrete clock rates
description.

Driver clk-scmi.c in only interested in the min and max clock rates.
Get these by querying the first and last discrete rates with SCMI
clock protocol message ID CLK_DESCRIBE_RATES since the SMCI clock
protocol specification states that rates enumerated by this
command are to be enumerated in "numeric ascending order" [1].

Link: https://developer.arm.com/documentation/den0056 [1]
Signed-off-by: Etienne Carriere <[e-mail]>
---
 drivers/clk/clk-scmi.c            |   4 +-
 drivers/firmware/arm_scmi/clock.c | 184 +++++++++++-------------------
 include/linux/scmi_protocol.h     |   4 +-
 3 files changed, 68 insertions(+), 124 deletions(-)

Comments

Cristian Marussi July 30, 2024, 3:12 p.m. UTC | #1

On Mon, Jul 29, 2024 at 08:53:06AM +0200, Etienne Carriere wrote:
> Remove limitation of 16 clock rates max for discrete clock rates
> description.
> 

Hi Etienne,

> Driver clk-scmi.c in only interested in the min and max clock rates.
> Get these by querying the first and last discrete rates with SCMI
> clock protocol message ID CLK_DESCRIBE_RATES since the SMCI clock
> protocol specification states that rates enumerated by this
> command are to be enumerated in "numeric ascending order" [1].
> 

While I understand the positive optinization of not having to query the
full list of clock domains in order to retrieve just min/max (that are
the only interesting clocks for CLK framework), and the removal of the
artificial 16 clocks limit, I would NOT drop in its entirety the original
well tested code used to query the full list (based on iterators), because
it will be most probably needed anyway in the future to implement
determine_rate via bisection (if I udnerstood correctly what you meant
in our offline chat) and because it would be needed anyway for testing
purposes.

IOW, why dont you do exactly what you did BUT within a new distinct
scmi_describe_min_max_rates() helper, WHILE keeping the original code
scmi_clock_describe_rates_get() unchanged, and start calling your new
method at init to retrieve only max/min ?

So that we can attach anyway (now or then) a new 'lazy' ops to the old
method and use it to retrieve the full list of clocks when needed (for
testing or whatever).

Thanks,
Cristian

Patch

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index d86a02563f6c..abe59da06324 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -234,8 +234,8 @@  static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
 		if (num_rates <= 0)
 			return -EINVAL;
 
-		min_rate = sclk->info->list.rates[0];
-		max_rate = sclk->info->list.rates[num_rates - 1];
+		min_rate = sclk->info->list.min_rate;
+		max_rate = sclk->info->list.max_rate;
 	} else {
 		min_rate = sclk->info->range.min_rate;
 		max_rate = sclk->info->range.max_rate;
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 134019297d08..88fd240052c0 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -404,140 +404,84 @@  static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
-static int rate_cmp_func(const void *_r1, const void *_r2)
+static int
+scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
+			      struct scmi_clock_info *clk)
 {
-	const u64 *r1 = _r1, *r2 = _r2;
-
-	if (*r1 < *r2)
-		return -1;
-	else if (*r1 == *r2)
-		return 0;
-	else
-		return 1;
-}
+	struct scmi_msg_clock_describe_rates *msg;
+	const struct scmi_msg_resp_clock_describe_rates *resp;
+	unsigned int num_returned, num_remaining;
+	struct scmi_xfer *t;
+	int ret;
 
-static void iter_clk_describe_prepare_message(void *message,
-					      const unsigned int desc_index,
-					      const void *priv)
-{
-	struct scmi_msg_clock_describe_rates *msg = message;
-	const struct scmi_clk_ipriv *p = priv;
+	/* Get either the range triplet or the min rate & rates count */
+	ret = ph->xops->xfer_get_init(ph, CLOCK_DESCRIBE_RATES, sizeof(*msg), 0,
+				      &t);
+	if (ret)
+		return ret;
 
-	msg->id = cpu_to_le32(p->clk_id);
-	/* Set the number of rates to be skipped/already read */
-	msg->rate_index = cpu_to_le32(desc_index);
-}
+	msg = t->tx.buf;
+	msg->id = cpu_to_le32(clk_id);
+	msg->rate_index = 0;
 
-static int
-iter_clk_describe_update_state(struct scmi_iterator_state *st,
-			       const void *response, void *priv)
-{
-	u32 flags;
-	struct scmi_clk_ipriv *p = priv;
-	const struct scmi_msg_resp_clock_describe_rates *r = response;
-
-	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);
-
-	/* Warn about out of spec replies ... */
-	if (!p->clk->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,
-			 st->rx_len);
-
-		/*
-		 * A known quirk: a triplet is returned but num_returned != 3
-		 * Check for a safe payload size and fix.
-		 */
-		if (st->num_returned != 3 && st->num_remaining == 0 &&
-		    st->rx_len == sizeof(*r) + sizeof(__le32) * 2 * 3) {
-			st->num_returned = 3;
-			st->num_remaining = 0;
-		} else {
-			dev_err(p->dev,
-				"Cannot fix out-of-spec reply !\n");
-			return -EPROTO;
-		}
-	}
+	resp = t->rx.buf;
 
-	return 0;
-}
+	ret = ph->xops->do_xfer(ph, t);
+	if (ret)
+		goto out;
 
-static int
-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];
+	clk->rate_discrete = RATE_DISCRETE(resp->num_rates_flags);
+	num_returned = NUM_RETURNED(resp->num_rates_flags);
+	num_remaining = NUM_REMAINING(resp->num_rates_flags);
 
-		*rate = RATE_TO_U64(r->rate[st->loop_idx]);
-		p->clk->list.num_rates++;
-	}
+	if (clk->rate_discrete) {
+		clk->list.num_rates = num_returned + num_remaining;
+		clk->list.min_rate = RATE_TO_U64(resp->rate[0]);
 
-	return ret;
-}
+		if (num_remaining) {
+			msg->rate_index = clk->list.num_rates - 1;
 
-static int
-scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
-			      struct scmi_clock_info *clk)
-{
-	int ret;
-	void *iter;
-	struct scmi_iterator_ops ops = {
-		.prepare_message = iter_clk_describe_prepare_message,
-		.update_state = iter_clk_describe_update_state,
-		.process_response = iter_clk_describe_process_response,
-	};
-	struct scmi_clk_ipriv cpriv = {
-		.clk_id = clk_id,
-		.clk = clk,
-		.dev = ph->dev,
-	};
+			ret = ph->xops->do_xfer(ph, t);
+			if (ret)
+				goto out;
 
-	iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_RATES,
-					    CLOCK_DESCRIBE_RATES,
-					    sizeof(struct scmi_msg_clock_describe_rates),
-					    &cpriv);
-	if (IS_ERR(iter))
-		return PTR_ERR(iter);
+			clk->list.max_rate = RATE_TO_U64(resp->rate[0]);
+		} else {
+			u64 max = RATE_TO_U64(resp->rate[num_returned - 1]);
 
-	ret = ph->hops->iter_response_run(iter);
-	if (ret)
-		return ret;
+			clk->list.max_rate = max;
+		}
+	} else {
+		/* Warn about out of spec replies ... */
+		if (num_returned != 3 || num_remaining != 0) {
+			dev_warn(ph->dev,
+				 "Out-of-spec CLOCK_DESCRIBE_RATES reply for %s - returned:%d remaining:%d rx_len:%zd\n",
+				 clk->name, num_returned, num_remaining,
+				 t->rx.len);
+
+			/*
+			 * A known quirk: a triplet is returned but
+			 * num_returned != 3, check for a safe payload
+			 * size to use returned info.
+			 */
+			if (num_remaining != 0 ||
+			    t->rx.len != sizeof(*resp) +
+					 sizeof(__le32) * 2 * 3) {
+				dev_err(ph->dev,
+					"Cannot fix out-of-spec reply !\n");
+				ret = -EPROTO;
+				goto out;
+			}
+		}
 
-	if (!clk->rate_discrete) {
-		dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
-			clk->range.min_rate, clk->range.max_rate,
-			clk->range.step_size);
-	} else if (clk->list.num_rates) {
-		sort(clk->list.rates, clk->list.num_rates,
-		     sizeof(clk->list.rates[0]), rate_cmp_func, NULL);
+		clk->range.min_rate = RATE_TO_U64(resp->rate[0]);
+		clk->range.max_rate = RATE_TO_U64(resp->rate[1]);
+		clk->range.step_size = RATE_TO_U64(resp->rate[2]);
 	}
 
+out:
+	ph->xops->xfer_put(ph, t);
+
 	return ret;
 }
 
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 3a9bb5b9a9e8..fe4b464419a0 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -15,7 +15,6 @@ 
 
 #define SCMI_MAX_STR_SIZE		64
 #define SCMI_SHORT_NAME_MAX_SIZE	16
-#define SCMI_MAX_NUM_RATES		16
 
 /**
  * struct scmi_revision_info - version information structure
@@ -54,7 +53,8 @@  struct scmi_clock_info {
 	union {
 		struct {
 			int num_rates;
-			u64 rates[SCMI_MAX_NUM_RATES];
+			u64 min_rate;
+			u64 max_rate;
 		} list;
 		struct {
 			u64 min_rate;