[PATCH v2 0/2] firmware: arm_scmi: unbound discrete rates, support round rate

[PATCH v2 0/2] firmware: arm_scmi: unbound discrete rates, support round rate

[Etienne Carriere]

These 2 patches propose to remove the limitation of 16 discrete rate max on
SCMI clocks and implements an effective round_rate operation on SCMI clocks
that provides non-linear possible rates.

The 1st patch removes a limitation on SCMI clocks that is not really needed
since the SCMI clock driver does not need to store the full list of
supported discrete rates but only require to store the min and max rate
values. This change was initially proposed in:
https://lore.kernel.org/lkml/20240729065306.1210733-1-etienne.carriere@foss.st.com/

The second patch implements a real round_rate operation that is needed for
example on STM32MP25 platforms for the video and the sound drivers that needs
to know the effective possible clock rates in order to select a compliant sample
clock frequency regarding the audio quality constraints.

STM32MP25 platforms also need the 1st patch of this series since many of the
audio clocks (SAIx and MDFx interfaces) and the LTDC video clock are provided
by SCMI clocks (CK_SCMI_FLEXGEN_x).

Etienne Carriere (2):
  firmware: arm_scmi: get only min/max clock rates
  firmware: arm_scmi: round rate bisecting in discrete rates

 drivers/clk/clk-scmi.c            |  21 ++-
 drivers/firmware/arm_scmi/clock.c | 205 +++++++++++++++++++++++++++++-
 include/linux/scmi_protocol.h     |   7 +-
 3 files changed, 220 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] firmware: arm_scmi: get only min/max clock rates

[Etienne Carriere]

Remove limitation of 16 clock rates max for discrete clock rates
description when the SCMI firmware supports SCMI Clock protocol v2.0
or later.

Driver clk-scmi.c is 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 CLOCK_DESCRIBE_RATES since the SCMI
specification v2.0 and later states that rates enumerated by this
command are to be enumerated in "numeric ascending order" [1].

Preserve the implementation that queries all discrete rates (16 rates
max) to support SCMI firmware built on SCMI specification v1.0 [2]
where SCMI Clock protocol v1.0 does not explicitly require rates
described with CLOCK_DESCRIBE_RATES to be in ascending order.

Link: https://developer.arm.com/documentation/den0056 [1]
Link: https://developer.arm.com/documentation/den0056/a [2]
Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
Changes since patch series v1:
- Preserve the original implementation and keep using it for SCMI
  Clock protocol v1.0.

---
 drivers/clk/clk-scmi.c            |   4 +-
 drivers/firmware/arm_scmi/clock.c | 112 ++++++++++++++++++++++++++++--
 include/linux/scmi_protocol.h     |   4 +-
 3 files changed, 110 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 15510c2ff21c..09ccd6cea7f2 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -244,8 +244,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 2ed2279388f0..34fde0b88098 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -223,10 +223,21 @@ scmi_clock_protocol_attributes_get(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
+/*
+ * Support SCMI_CLOCK protocol v1.0 as described in SCMI specification v1.0
+ * that do not explicitly require clock rates described with command
+ * CLOCK_DESCRIBE_RATES to be in ascending order. The Linux legacy
+ * implementation for these clock supports a max of 16 rates.
+ * In SCMI specification v2.0 and later, rates must be in ascending order
+ * to we query only to min and max rates values.
+ */
+#define SCMI_MAX_NUM_RATES_V1		16
+
 struct scmi_clk_ipriv {
 	struct device *dev;
 	u32 clk_id;
 	struct scmi_clock_info *clk;
+	u64 rates[SCMI_MAX_NUM_RATES_V1];
 };
 
 static void iter_clk_possible_parents_prepare_message(void *message, unsigned int desc_index,
@@ -493,7 +504,7 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
 			break;
 		}
 	} else {
-		u64 *rate = &p->clk->list.rates[st->desc_index + st->loop_idx];
+		u64 *rate = &p->rates[st->desc_index + st->loop_idx];
 
 		*rate = RATE_TO_U64(r->rate[st->loop_idx]);
 		p->clk->list.num_rates++;
@@ -519,7 +530,7 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 		.dev = ph->dev,
 	};
 
-	iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_RATES,
+	iter = ph->hops->iter_response_init(ph, &ops, ARRAY_SIZE(cpriv.rates),
 					    CLOCK_DESCRIBE_RATES,
 					    sizeof(struct scmi_msg_clock_describe_rates),
 					    &cpriv);
@@ -535,10 +546,95 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 			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);
+		sort(cpriv.rates, clk->list.num_rates,
+		     sizeof(cpriv.rates[0]), rate_cmp_func, NULL);
+		clk->list.min_rate = cpriv.rates[0];
+		clk->list.max_rate = cpriv.rates[clk->list.num_rates - 1];
+	}
+
+	return ret;
+}
+
+static int scmi_clock_get_rates_bound(const struct scmi_protocol_handle *ph,
+				      u32 clk_id, struct scmi_clock_info *clk)
+{
+	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;
+
+	/* 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 = t->tx.buf;
+	msg->id = cpu_to_le32(clk_id);
+	msg->rate_index = 0;
+
+	resp = t->rx.buf;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (ret)
+		goto out;
+
+	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);
+
+	if (clk->rate_discrete) {
+		clk->list.num_rates = num_returned + num_remaining;
+		clk->list.min_rate = RATE_TO_U64(resp->rate[0]);
+
+		if (num_remaining) {
+			ph->xops->reset_rx_to_maxsz(ph, t);
+			msg->id = cpu_to_le32(clk_id);
+			msg->rate_index = cpu_to_le32(clk->list.num_rates - 1);
+			ret = ph->xops->do_xfer(ph, t);
+			if (!ret)
+				clk->list.max_rate = RATE_TO_U64(resp->rate[0]);
+		} else {
+			u64 max = RATE_TO_U64(resp->rate[num_returned - 1]);
+
+			clk->list.max_rate = max;
+		}
+	} else {
+		/* We expect a triplet, 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;
+			}
+		}
+
+		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]);
+
+		dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
+			clk->range.min_rate, clk->range.max_rate,
+			clk->range.step_size);
 	}
 
+out:
+	ph->xops->xfer_put(ph, t);
+
 	return ret;
 }
 
@@ -1089,8 +1185,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
 		struct scmi_clock_info *clk = cinfo->clk + clkid;
 
 		ret = scmi_clock_attributes_get(ph, clkid, cinfo, version);
-		if (!ret)
-			scmi_clock_describe_rates_get(ph, clkid, clk);
+		if (!ret) {
+			if (PROTOCOL_REV_MAJOR(version) >= 0x2)
+				scmi_clock_get_rates_bound(ph, clkid, clk);
+			else
+				scmi_clock_describe_rates_get(ph, clkid, clk);
+		}
 	}
 
 	if (PROTOCOL_REV_MAJOR(version) >= 0x3) {
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 688466a0e816..240478bb8476 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;
-- 
2.25.1

Re: [PATCH v2 1/2] firmware: arm_scmi: get only min/max clock rates

[Sudeep Holla]

On Tue, Dec 03, 2024 at 06:39:07PM +0100, Etienne Carriere wrote:
> Remove limitation of 16 clock rates max for discrete clock rates
> description when the SCMI firmware supports SCMI Clock protocol v2.0
> or later.
>
> Driver clk-scmi.c is 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 CLOCK_DESCRIBE_RATES since the SCMI
> specification v2.0 and later states that rates enumerated by this
> command are to be enumerated in "numeric ascending order" [1].
>
> Preserve the implementation that queries all discrete rates (16 rates
> max) to support SCMI firmware built on SCMI specification v1.0 [2]
> where SCMI Clock protocol v1.0 does not explicitly require rates
> described with CLOCK_DESCRIBE_RATES to be in ascending order.
>
> Link: https://developer.arm.com/documentation/den0056 [1]
> Link: https://developer.arm.com/documentation/den0056/a [2]
> Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> ---

[...]

> +
> +static int scmi_clock_get_rates_bound(const struct scmi_protocol_handle *ph,
> +				      u32 clk_id, struct scmi_clock_info *clk)
> +{

This new function seem to have unwraped the scmi_iterator_ops(namely
prepare_message, update_state and process_response instead of reusing them.
Can you please explain why it wasn't possible to reuse them ?

--
Regards,
Sudeep

Re: [PATCH v2 1/2] firmware: arm_scmi: get only min/max clock rates

[Etienne CARRIERE - foss]

On Monday, December 9, 2024, Sudeep Holla wrote:
> On Tue, Dec 03, 2024 at 06:39:07PM +0100, Etienne Carriere wrote:
> > Remove limitation of 16 clock rates max for discrete clock rates
> > description when the SCMI firmware supports SCMI Clock protocol v2.0
> > or later.
> >
> > Driver clk-scmi.c is 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 CLOCK_DESCRIBE_RATES since the SCMI
> > specification v2.0 and later states that rates enumerated by this
> > command are to be enumerated in "numeric ascending order" [1].
> >
> > Preserve the implementation that queries all discrete rates (16 rates
> > max) to support SCMI firmware built on SCMI specification v1.0 [2]
> > where SCMI Clock protocol v1.0 does not explicitly require rates
> > described with CLOCK_DESCRIBE_RATES to be in ascending order.
> >
> > Link: https://developer.arm.com/documentation/den0056 [1]
> > Link: https://developer.arm.com/documentation/den0056/a [2]
> > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > ---
> 
> [...]
> 
> > +
> > +static int scmi_clock_get_rates_bound(const struct scmi_protocol_handle *ph,
> > +                                   u32 clk_id, struct scmi_clock_info *clk)
> > +{
> 
> This new function seem to have unwraped the scmi_iterator_ops(namely
> prepare_message, update_state and process_response instead of reusing them.
> Can you please explain why it wasn't possible to reuse them ?

Since we're interested here only in min and max rates, let's query the
first and last rates only. This can save a bit of useless transactions between
agent and firmware in case there are many clocks with somewhat large
the discrete rate lists.

I though using the iterator for this specific case would add a bit more
complexity: it's expected to iterate (st->desc_index incremented from the
common scmi_iterator_run() function) whereas here I propose to send
only 2 messages.

BR,
Etienne

> 
> --
> Regards,
> Sudeep
>

Re: [PATCH v2 1/2] firmware: arm_scmi: get only min/max clock rates

[Cristian Marussi]

On Mon, Dec 09, 2024 at 01:48:48PM +0000, Etienne CARRIERE - foss wrote:
> On Monday, December 9, 2024, Sudeep Holla wrote:
> > On Tue, Dec 03, 2024 at 06:39:07PM +0100, Etienne Carriere wrote:
> > > Remove limitation of 16 clock rates max for discrete clock rates
> > > description when the SCMI firmware supports SCMI Clock protocol v2.0
> > > or later.
> > >
> > > Driver clk-scmi.c is 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 CLOCK_DESCRIBE_RATES since the SCMI
> > > specification v2.0 and later states that rates enumerated by this
> > > command are to be enumerated in "numeric ascending order" [1].
> > >
> > > Preserve the implementation that queries all discrete rates (16 rates
> > > max) to support SCMI firmware built on SCMI specification v1.0 [2]
> > > where SCMI Clock protocol v1.0 does not explicitly require rates
> > > described with CLOCK_DESCRIBE_RATES to be in ascending order.
> > >
> > > Link: https://developer.arm.com/documentation/den0056 [1]
> > > Link: https://developer.arm.com/documentation/den0056/a [2]
> > > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > > ---

Hi,

> > 
> > [...]
> > 
> > > +
> > > +static int scmi_clock_get_rates_bound(const struct scmi_protocol_handle *ph,
> > > +                                   u32 clk_id, struct scmi_clock_info *clk)
> > > +{
> > 
> > This new function seem to have unwraped the scmi_iterator_ops(namely
> > prepare_message, update_state and process_response instead of reusing them.
> > Can you please explain why it wasn't possible to reuse them ?
> 
> Since we're interested here only in min and max rates, let's query the
> first and last rates only. This can save a bit of useless transactions between
> agent and firmware in case there are many clocks with somewhat large
> the discrete rate lists.
> 
> I though using the iterator for this specific case would add a bit more
> complexity: it's expected to iterate (st->desc_index incremented from the
> common scmi_iterator_run() function) whereas here I propose to send
> only 2 messages.

Yes, indeed the core iterator helpers are meant to issue a 'full scan'
retrievieng all the resources that are returned while handling in a
common way the underlying machinery common to all messages that, like
DESCRIBE_RATES, could possibly return their results in chunks as a
multi-part reply...

...having said that I can certainly extend the iterators to be configurable
enough to fit this new usecase and retrieve only the desired part of the
'scan' so that can be used for this kind of max/min query or for the
bisection case.

I would avoid to re-introduce ad-hoc code to handle these new usecases
that do not fit into the existing iterator logic, since iterators
were introduced to remove duplication and unify under common
methods...and this new iterator scenario seems to me that has already 2
usecases and certainly more protocol could want to perform similar 'lazy
partial queries' in the future, so I'd prefer to address this in a more
general way upfront if possible...I will think about it and post something
next week in the form of some new iterator extensions, if it's fine for you.

Thanks,
Cristian

Re: [PATCH v2 1/2] firmware: arm_scmi: get only min/max clock rates

[Etienne CARRIERE - foss]

On Monday, December 9, 2024, Cristian Marussi wrote:
> On Mon, Dec 09, 2024 at 01:48:48PM +0000, Etienne CARRIERE - foss wrote:
> > On Monday, December 9, 2024, Sudeep Holla wrote:
> > > On Tue, Dec 03, 2024 at 06:39:07PM +0100, Etienne Carriere wrote:
> > > > Remove limitation of 16 clock rates max for discrete clock rates
> > > > description when the SCMI firmware supports SCMI Clock protocol v2.0
> > > > or later.
> > > >
> > > > Driver clk-scmi.c is 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 CLOCK_DESCRIBE_RATES since the SCMI
> > > > specification v2.0 and later states that rates enumerated by this
> > > > command are to be enumerated in "numeric ascending order" [1].
> > > >
> > > > Preserve the implementation that queries all discrete rates (16 rates
> > > > max) to support SCMI firmware built on SCMI specification v1.0 [2]
> > > > where SCMI Clock protocol v1.0 does not explicitly require rates
> > > > described with CLOCK_DESCRIBE_RATES to be in ascending order.
> > > >
> > > > Link: https://developer.arm.com/documentation/den0056 [1]
> > > > Link: https://developer.arm.com/documentation/den0056/a [2]
> > > > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > > > ---
> 
> Hi,
> 
> > >
> > > [...]
> > >
> > > > +
> > > > +static int scmi_clock_get_rates_bound(const struct scmi_protocol_handle *ph,
> > > > +                                   u32 clk_id, struct scmi_clock_info *clk)
> > > > +{
> > >
> > > This new function seem to have unwraped the scmi_iterator_ops(namely
> > > prepare_message, update_state and process_response instead of reusing them.
> > > Can you please explain why it wasn't possible to reuse them ?
> >
> > Since we're interested here only in min and max rates, let's query the
> > first and last rates only. This can save a bit of useless transactions between
> > agent and firmware in case there are many clocks with somewhat large
> > the discrete rate lists.
> >
> > I though using the iterator for this specific case would add a bit more
> > complexity: it's expected to iterate (st->desc_index incremented from the
> > common scmi_iterator_run() function) whereas here I propose to send
> > only 2 messages.
> 
> Yes, indeed the core iterator helpers are meant to issue a 'full scan'
> retrievieng all the resources that are returned while handling in a
> common way the underlying machinery common to all messages that, like
> DESCRIBE_RATES, could possibly return their results in chunks as a
> multi-part reply...
> 
> ...having said that I can certainly extend the iterators to be configurable
> enough to fit this new usecase and retrieve only the desired part of the
> 'scan' so that can be used for this kind of max/min query or for the
> bisection case.
> 
> I would avoid to re-introduce ad-hoc code to handle these new usecases
> that do not fit into the existing iterator logic, since iterators
> were introduced to remove duplication and unify under common
> methods...and this new iterator scenario seems to me that has already 2
> usecases and certainly more protocol could want to perform similar 'lazy
> partial queries' in the future, so I'd prefer to address this in a more
> general way upfront if possible...I will think about it and post something
> next week in the form of some new iterator extensions, if it's fine for you.
> 

Hi Cristian,
Thanks for looking at this. Any help here is very welcome.

BR,
Etienne


> Thanks,
> Cristian
> 

[PATCH v2 2/2] firmware: arm_scmi: round rate bisecting in discrete rates

[Etienne Carriere]

Implement clock round_rate operation for SCMI clocks that describe a
discrete rates list. Bisect into the supported rates when using SCMI
message CLOCK_DESCRIBE_RATES to optimize SCMI communication transfers.
Parse the rate list array when the target rate fit in the bounds
of the command response for simplicity.

If so some reason the sequence fails or if the SCMI driver has no
round_rate SCMI clock handler, then fallback to the legacy strategy that
returned the target rate value.

Operation handle scmi_clk_determine_rate() is change to get the effective
supported rounded rate when there is no clock re-parenting operation
supported. Otherwise, preserve the implementation that assumed any
clock rate could be obtained.

Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
Changes since patch series v1:
- New patch introduced in this v2 series.

---
 drivers/clk/clk-scmi.c            | 17 +++++-
 drivers/firmware/arm_scmi/clock.c | 93 +++++++++++++++++++++++++++++++
 include/linux/scmi_protocol.h     |  3 +
 3 files changed, 110 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 09ccd6cea7f2..7bbb2ee55f4f 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -61,13 +61,20 @@ static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
 	struct scmi_clk *clk = to_scmi_clk(hw);
 
 	/*
-	 * We can't figure out what rate it will be, so just return the
+	 * In case we can't figure out what rate it will be when the clock
+	 * describes a list of discrete rates, then just return the
 	 * rate back to the caller. scmi_clk_recalc_rate() will be called
 	 * after the rate is set and we'll know what rate the clock is
 	 * running at then.
 	 */
-	if (clk->info->rate_discrete)
+	if (clk->info->rate_discrete) {
+		ftmp = rate;
+		if (scmi_proto_clk_ops->round_rate &&
+		    !scmi_proto_clk_ops->round_rate(clk->ph, clk->id, &ftmp))
+			return ftmp;
+
 		return rate;
+	}
 
 	fmin = clk->info->range.min_rate;
 	fmax = clk->info->range.max_rate;
@@ -122,9 +129,13 @@ static u8 scmi_clk_get_parent(struct clk_hw *hw)
 static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
 {
 	/*
-	 * Suppose all the requested rates are supported, and let firmware
+	 * If not several parents look into supported rates. Otherwise
+	 * suppose all the requested rates are supported, and let firmware
 	 * to handle the left work.
 	 */
+	if (to_scmi_clk(hw)->info->num_parents < 2)
+		req->rate = scmi_clk_round_rate(hw, req->rate, NULL);
+
 	return 0;
 }
 
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 34fde0b88098..e416476dd336 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -999,6 +999,98 @@ static int scmi_clock_config_oem_get(const struct scmi_protocol_handle *ph,
 				    NULL, oem_val, atomic);
 }
 
+static int scmi_clock_round_rate(const struct scmi_protocol_handle *ph,
+				 u32 clk_id, u64 *rate)
+
+{
+	const struct scmi_msg_resp_clock_describe_rates *resp;
+	size_t index_low, index_high, index_tmp, count, i;
+	struct scmi_msg_clock_describe_rates *msg;
+	u64 rate_low, rate_high, target_rate;
+	struct scmi_xfer *xfer;
+	int ret;
+	struct clock_info *ci = ph->get_priv(ph);
+	struct scmi_clock_info *clk = ci->clk + clk_id;
+
+	if (clk_id >= ci->num_clocks ||
+	    WARN_ONCE(!clk->rate_discrete, "Unexpected linear rates"))
+		return -EINVAL;
+
+	target_rate = *rate;
+	index_low = 0;
+	index_high = clk->list.num_rates - 1;
+	rate_low = clk->list.min_rate;
+	rate_high = clk->list.max_rate;
+
+	if (target_rate <= rate_low) {
+		*rate = rate_low;
+		return 0;
+	}
+	if (target_rate >= rate_high) {
+		*rate = rate_high;
+		return 0;
+	}
+
+	ret = ph->xops->xfer_get_init(ph, CLOCK_DESCRIBE_RATES, sizeof(*msg), 0,
+				      &xfer);
+	if (ret)
+		return ret;
+
+	resp = xfer->rx.buf;
+	msg = xfer->tx.buf;
+	msg->id = cpu_to_le32(clk_id);
+
+	while (true) {
+		index_tmp = (index_low + index_high) / 2;
+
+		ph->xops->reset_rx_to_maxsz(ph, xfer);
+		msg->id = cpu_to_le32(clk_id);
+		msg->rate_index = cpu_to_le32(index_tmp);
+
+		ret = ph->xops->do_xfer(ph, xfer);
+		if (!ret && (!RATE_DISCRETE(resp->num_rates_flags) ||
+			     !NUM_RETURNED(resp->num_rates_flags)))
+			ret = -EPROTO;
+		if (ret)
+			break;
+
+		count = NUM_RETURNED(resp->num_rates_flags);
+
+		if (target_rate < RATE_TO_U64(resp->rate[0])) {
+			index_high = index_tmp;
+			rate_high = RATE_TO_U64(resp->rate[0]);
+		} else if (target_rate > RATE_TO_U64(resp->rate[count - 1])) {
+			index_low = index_tmp + count - 1;
+			rate_low = RATE_TO_U64(resp->rate[count - 1]);
+		} else {
+			for (i = 1; i < count; i++)
+				if (target_rate <= RATE_TO_U64(resp->rate[i]))
+					break;
+
+			index_low = index_tmp + i - 1;
+			rate_low = RATE_TO_U64(resp->rate[i - 1]);
+
+			if (i < count) {
+				index_high = index_tmp + i;
+				rate_high = RATE_TO_U64(resp->rate[i]);
+			}
+		}
+
+		if (index_high <= index_low + 1) {
+			if (target_rate - rate_low > rate_high - target_rate)
+				*rate = rate_high;
+			else
+				*rate = rate_low;
+
+			break;
+		}
+	}
+
+	ph->xops->xfer_put(ph, xfer);
+
+	return ret;
+}
+
 static int scmi_clock_count_get(const struct scmi_protocol_handle *ph)
 {
 	struct clock_info *ci = ph->get_priv(ph);
@@ -1027,6 +1119,7 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
 	.info_get = scmi_clock_info_get,
 	.rate_get = scmi_clock_rate_get,
 	.rate_set = scmi_clock_rate_set,
+	.round_rate = scmi_clock_round_rate,
 	.enable = scmi_clock_enable,
 	.disable = scmi_clock_disable,
 	.state_get = scmi_clock_state_get,
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 240478bb8476..30cf373c3f8b 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -91,6 +91,7 @@ enum scmi_clock_oem_config {
  * @info_get: get the information of the specified clock
  * @rate_get: request the current clock rate of a clock
  * @rate_set: set the clock rate of a clock
+ * @round_rate: tell which is the nearest rate a clock supports (w/o setting it)
  * @enable: enables the specified clock
  * @disable: disables the specified clock
  * @state_get: get the status of the specified clock
@@ -108,6 +109,8 @@ struct scmi_clk_proto_ops {
 			u64 *rate);
 	int (*rate_set)(const struct scmi_protocol_handle *ph, u32 clk_id,
 			u64 rate);
+	int (*round_rate)(const struct scmi_protocol_handle *ph, u32 clk_id,
+			  u64 *rate);
 	int (*enable)(const struct scmi_protocol_handle *ph, u32 clk_id,
 		      bool atomic);
 	int (*disable)(const struct scmi_protocol_handle *ph, u32 clk_id,
-- 
2.25.1

Re: [PATCH v2 2/2] firmware: arm_scmi: round rate bisecting in discrete rates

[Dan Carpenter]

On Tue, Dec 03, 2024 at 06:39:08PM +0100, Etienne Carriere wrote:
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 09ccd6cea7f2..7bbb2ee55f4f 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -61,13 +61,20 @@ static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
>  	struct scmi_clk *clk = to_scmi_clk(hw);
>  
>  	/*
> -	 * We can't figure out what rate it will be, so just return the
> +	 * In case we can't figure out what rate it will be when the clock
> +	 * describes a list of discrete rates, then just return the
>  	 * rate back to the caller. scmi_clk_recalc_rate() will be called
>  	 * after the rate is set and we'll know what rate the clock is
>  	 * running at then.
>  	 */
> -	if (clk->info->rate_discrete)
> +	if (clk->info->rate_discrete) {
> +		ftmp = rate;

No need for this assignment.

> +		if (scmi_proto_clk_ops->round_rate &&
> +		    !scmi_proto_clk_ops->round_rate(clk->ph, clk->id, &ftmp))
> +			return ftmp;
> +
>  		return rate;
> +	}
>  
>  	fmin = clk->info->range.min_rate;
>  	fmax = clk->info->range.max_rate;

regards,
dan carpenter

Re: [PATCH v2 2/2] firmware: arm_scmi: round rate bisecting in discrete rates

[Etienne CARRIERE - foss]

Hello Dan,

Thanks for looking at this change.

On Friday, December 6, 2024, Dan Carpenter wrote:
> 
> On Tue, Dec 03, 2024 at 06:39:08PM +0100, Etienne Carriere wrote:
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > index 09ccd6cea7f2..7bbb2ee55f4f 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -61,13 +61,20 @@ static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> >       struct scmi_clk *clk = to_scmi_clk(hw);
> >
> >       /*
> > -      * We can't figure out what rate it will be, so just return the
> > +      * In case we can't figure out what rate it will be when the clock
> > +      * describes a list of discrete rates, then just return the
> >        * rate back to the caller. scmi_clk_recalc_rate() will be called
> >        * after the rate is set and we'll know what rate the clock is
> >        * running at then.
> >        */
> > -     if (clk->info->rate_discrete)
> > +     if (clk->info->rate_discrete) {
> > +             ftmp = rate;
> 
> No need for this assignment.

It is needed. The round_rate handler in scmi clock protocol driver 
I added in drivers/firmware/arm_scmi/clock.c expects the argument
to carry the target rate as input value and provide the closest reachable
rate as output value, hence initializing @ftmp above. I needed to preserve
the value @rate since used if this round_rate handle is not available
or returns with an error.

Note that I could have changed scmi_proto_clk_ops->round_rate() 
function ABI to split input target rate and output rounded rate into
2 separated explicit arguments.

Regards,
Etienne

> 
> > +             if (scmi_proto_clk_ops->round_rate &&
> > +                 !scmi_proto_clk_ops->round_rate(clk->ph, clk->id, &ftmp))
> > +                     return ftmp;
> > +
> >               return rate;
> > +     }
> >
> >       fmin = clk->info->range.min_rate;
> >       fmax = clk->info->range.max_rate;
> 
> regards,
> dan carpenter
> 

Re: [PATCH v2 2/2] firmware: arm_scmi: round rate bisecting in discrete rates

[Dan Carpenter]

On Mon, Dec 09, 2024 at 08:16:24AM +0000, Etienne CARRIERE - foss wrote:
> Hello Dan,
> 
> Thanks for looking at this change.
> 
> On Friday, December 6, 2024, Dan Carpenter wrote:
> > 
> > On Tue, Dec 03, 2024 at 06:39:08PM +0100, Etienne Carriere wrote:
> > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > > index 09ccd6cea7f2..7bbb2ee55f4f 100644
> > > --- a/drivers/clk/clk-scmi.c
> > > +++ b/drivers/clk/clk-scmi.c
> > > @@ -61,13 +61,20 @@ static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> > >       struct scmi_clk *clk = to_scmi_clk(hw);
> > >
> > >       /*
> > > -      * We can't figure out what rate it will be, so just return the
> > > +      * In case we can't figure out what rate it will be when the clock
> > > +      * describes a list of discrete rates, then just return the
> > >        * rate back to the caller. scmi_clk_recalc_rate() will be called
> > >        * after the rate is set and we'll know what rate the clock is
> > >        * running at then.
> > >        */
> > > -     if (clk->info->rate_discrete)
> > > +     if (clk->info->rate_discrete) {
> > > +             ftmp = rate;
> > 
> > No need for this assignment.
> 
> It is needed. The round_rate handler in scmi clock protocol driver 
> I added in drivers/firmware/arm_scmi/clock.c expects the argument
> to carry the target rate as input value and provide the closest reachable
> rate as output value, hence initializing @ftmp above. I needed to preserve
> the value @rate since used if this round_rate handle is not available
> or returns with an error.
> 
> Note that I could have changed scmi_proto_clk_ops->round_rate() 
> function ABI to split input target rate and output rounded rate into
> 2 separated explicit arguments.

No, no.  Leave it.  Sorry.  I wasn't paying attention.

regards,
dan carpenter

Re: [PATCH v2 2/2] firmware: arm_scmi: round rate bisecting in discrete rates

[Sudeep Holla]

On Tue, Dec 03, 2024 at 06:39:08PM +0100, Etienne Carriere wrote:
> Implement clock round_rate operation for SCMI clocks that describe a
> discrete rates list. Bisect into the supported rates when using SCMI
> message CLOCK_DESCRIBE_RATES to optimize SCMI communication transfers.

Let me stop here and try to understand the requirement here. So you do
communicate with the firmware to arrive at this round_rate ? Does the
list of discreet clock rates changes at the run-time that enables the
need for it. Or will the initial list just include max and min ?

> Parse the rate list array when the target rate fit in the bounds
> of the command response for simplicity.
>

I don't understand what you mean by this.

> If so some reason the sequence fails or if the SCMI driver has no
> round_rate SCMI clock handler, then fallback to the legacy strategy that
> returned the target rate value.
>

Hmm, so we perform some extra dance but we are okay to fallback to default.
I am more confused.

> Operation handle scmi_clk_determine_rate() is change to get the effective
> supported rounded rate when there is no clock re-parenting operation
> supported. Otherwise, preserve the implementation that assumed any
> clock rate could be obtained.
>

OK, no I think I am getting some idea. Is this case where the parent has
changed and the describe rates can give a different result at run-time.

I need to re-read the part of the spec, but we may need some clarity so
that this implementation is not vendor specific. I am yet to understand this
fully. I just need to make sure spec covers this aspect and anything we
add here is generic solution.

I would like to avoid this extra query if not required which you seem to
have made an attempt but I just want to be thorough and make sure that's
what we need w.r.t the specification.

-- 
Regards,
Sudeep

Re: [PATCH v2 2/2] firmware: arm_scmi: round rate bisecting in discrete rates

[Etienne CARRIERE - foss]

Hello Sudeep,

 On Monday, December 9, 2024 11:46 AM, Sudeep Holla wrote: 
> On Tue, Dec 03, 2024 at 06:39:08PM +0100, Etienne Carriere wrote:
> > Implement clock round_rate operation for SCMI clocks that describe a
> > discrete rates list. Bisect into the supported rates when using SCMI
> > message CLOCK_DESCRIBE_RATES to optimize SCMI communication transfers.
> 
> Let me stop here and try to understand the requirement here. So you do
> communicate with the firmware to arrive at this round_rate ? Does the
> list of discreet clock rates changes at the run-time that enables the
> need for it. Or will the initial list just include max and min ?

I don't expect the list to change at run-time. The initial list is
expected to describe all supported rates. But because this list may
be big, I don't think arm_scmi/clock.c driver should store the full list
of all supported rates for each of the SCMI clocks. It would cost to
much memory. Therefore I propose to query it at runtime, when
needed, and bisect to lower the number of required transactions
between the agent and the firmware.

> 
> > Parse the rate list array when the target rate fit in the bounds
> > of the command response for simplicity.
> >
> 
> I don't understand what you mean by this.

I meant here that we bisect into supported rates when communicating
with the firmware but once the firmware response provides list portion
when target rate fits into, we just scan into that array instead of bisecting
into. We could also bisect into that array but it is likely quite small
(<128 byte in existing SCMI transport drivers) and that would add a bit
more code for no much gain IMHO.


> 
> > If so some reason the sequence fails or if the SCMI driver has no
> > round_rate SCMI clock handler, then fallback to the legacy strategy that
> > returned the target rate value.
> >
> 
> Hmm, so we perform some extra dance but we are okay to fallback to default.
> I am more confused.

Here, I propose to preserve the exiting sequence in clk/clk-scmi.c in case
arm_scmi/clock.c does not implement this new round_rate SCMI clock 
operation (it can be the case if these 2 drivers are .ko modules, not
well known built-in drivers).

> 
> > Operation handle scmi_clk_determine_rate() is change to get the effective
> > supported rounded rate when there is no clock re-parenting operation
> > supported. Otherwise, preserve the implementation that assumed any
> > clock rate could be obtained.
> >
> 
> OK, no I think I am getting some idea. Is this case where the parent has
> changed and the describe rates can give a different result at run-time.

This does not deal with whether parent has changed or not. I would expect
the same request sent multiple times to provide the very same result. But
as I said above, I don't think arm_scmi/clock.c should consume a possibly
large array of memory to store all supported rate each of the SCMI clocks
(that describe discrete rates).

An alternate way could be to add an SCMI Clock protocol command in the
spec allowing agent to query a closest supported rate, in 1 shot. Maybe
this new command could return both rounded rate and the SCMI parent
clock needed to reach that rounded rate, better fitting clk_determine_rate()
expectations.

> 
> I need to re-read the part of the spec, but we may need some clarity so
> that this implementation is not vendor specific. I am yet to understand this
> fully. I just need to make sure spec covers this aspect and anything we
> add here is generic solution.
> 
> I would like to avoid this extra query if not required which you seem to
> have made an attempt but I just want to be thorough and make sure that's
> what we need w.r.t the specification.

Sure, I indeed prefer clear and robust implementation in the long term,
being the one I propose here or another one.

Regards,
Etienne

> 
> --
> Regards,
> Sudeep
> 

Re: [PATCH v2 2/2] firmware: arm_scmi: round rate bisecting in discrete rates

[Sudeep Holla]

On Mon, Dec 09, 2024 at 12:59:58PM +0000, Etienne CARRIERE - foss wrote:
> Hello Sudeep,
> 
>  On Monday, December 9, 2024 11:46 AM, Sudeep Holla wrote: 
> > On Tue, Dec 03, 2024 at 06:39:08PM +0100, Etienne Carriere wrote:
> > > Implement clock round_rate operation for SCMI clocks that describe a
> > > discrete rates list. Bisect into the supported rates when using SCMI
> > > message CLOCK_DESCRIBE_RATES to optimize SCMI communication transfers.
> > 
> > Let me stop here and try to understand the requirement here. So you do
> > communicate with the firmware to arrive at this round_rate ? Does the
> > list of discreet clock rates changes at the run-time that enables the
> > need for it. Or will the initial list just include max and min ?
> 
> I don't expect the list to change at run-time. The initial list is
> expected to describe all supported rates. But because this list may
> be big, I don't think arm_scmi/clock.c driver should store the full list
> of all supported rates for each of the SCMI clocks. It would cost to
> much memory. Therefore I propose to query it at runtime, when
> needed, and bisect to lower the number of required transactions
> between the agent and the firmware.
>

Ah so, this is nothing to do with set_parent, but just an optimisation.
This change optimises for space but some other platform may have all the
space but the communication with SCMI platform is not good enough to make
runtime calls like this change. How do we cater that then ?

We need some spec-ed way or a unique way to identify what is best for
the platform IMO. We can change the way you have done in this change set
as someone else may complain in the future that it is costly to send
such command every time a clock needs to be set. I am just guessing here
may not be true.

> > 
> > > Parse the rate list array when the target rate fit in the bounds
> > > of the command response for simplicity.
> > >
> > 
> > I don't understand what you mean by this.
> 
> I meant here that we bisect into supported rates when communicating
> with the firmware but once the firmware response provides list portion
> when target rate fits into, we just scan into that array instead of bisecting
> into. We could also bisect into that array but it is likely quite small
> (<128 byte in existing SCMI transport drivers) and that would add a bit
> more code for no much gain IMHO.
> 
> 
> > 
> > > If so some reason the sequence fails or if the SCMI driver has no
> > > round_rate SCMI clock handler, then fallback to the legacy strategy that
> > > returned the target rate value.
> > >
> > 
> > Hmm, so we perform some extra dance but we are okay to fallback to default.
> > I am more confused.
> 
> Here, I propose to preserve the exiting sequence in clk/clk-scmi.c in case
> arm_scmi/clock.c does not implement this new round_rate SCMI clock 
> operation (it can be the case if these 2 drivers are .ko modules, not
> well known built-in drivers).
>

I don't think it would work if it is not built on the same kernel anyways.
I don't work much about this use-case.

> > 
> > > Operation handle scmi_clk_determine_rate() is change to get the effective
> > > supported rounded rate when there is no clock re-parenting operation
> > > supported. Otherwise, preserve the implementation that assumed any
> > > clock rate could be obtained.
> > >
> > 
> > OK, no I think I am getting some idea. Is this case where the parent has
> > changed and the describe rates can give a different result at run-time.
> 
> This does not deal with whether parent has changed or not. I would expect
> the same request sent multiple times to provide the very same result. But
> as I said above, I don't think arm_scmi/clock.c should consume a possibly
> large array of memory to store all supported rate each of the SCMI clocks
> (that describe discrete rates).
>

Right, my assumption was totally wrong. Thanks for confirming.

> An alternate way could be to add an SCMI Clock protocol command in the
> spec allowing agent to query a closest supported rate, in 1 shot. Maybe
> this new command could return both rounded rate and the SCMI parent
> clock needed to reach that rounded rate, better fitting clk_determine_rate()
> expectations.
>

May be that would be ideal but you need to make a case for such a spec change.

> > 
> > I need to re-read the part of the spec, but we may need some clarity so
> > that this implementation is not vendor specific. I am yet to understand this
> > fully. I just need to make sure spec covers this aspect and anything we
> > add here is generic solution.
> > 
> > I would like to avoid this extra query if not required which you seem to
> > have made an attempt but I just want to be thorough and make sure that's
> > what we need w.r.t the specification.
> 
> Sure, I indeed prefer clear and robust implementation in the long term,
> being the one I propose here or another one.
> 

Good then, we can work towards achieving that. If you can specify how slow
or memory hungry is it without these changes and how much this change helps
your platform, we can take it up with spec authors and see if they are happy
to provide some alternative to deal with this in a generic way.

--
Regards,
Sudeep

Re: [PATCH v2 2/2] firmware: arm_scmi: round rate bisecting in discrete rates

[Etienne CARRIERE - foss]

On Monday, December 9, 2024, Sudeep Holla wrote:
> On Mon, Dec 09, 2024 at 12:59:58PM +0000, Etienne CARRIERE - foss wrote:
> > Hello Sudeep,
> >
> >  On Monday, December 9, 2024 11:46 AM, Sudeep Holla wrote:
> > > On Tue, Dec 03, 2024 at 06:39:08PM +0100, Etienne Carriere wrote:
> > > > Implement clock round_rate operation for SCMI clocks that describe a
> > > > discrete rates list. Bisect into the supported rates when using SCMI
> > > > message CLOCK_DESCRIBE_RATES to optimize SCMI communication transfers.
> > >
> > > Let me stop here and try to understand the requirement here. So you do
> > > communicate with the firmware to arrive at this round_rate ? Does the
> > > list of discreet clock rates changes at the run-time that enables the
> > > need for it. Or will the initial list just include max and min ?
> >
> > I don't expect the list to change at run-time. The initial list is
> > expected to describe all supported rates. But because this list may
> > be big, I don't think arm_scmi/clock.c driver should store the full list
> > of all supported rates for each of the SCMI clocks. It would cost to
> > much memory. Therefore I propose to query it at runtime, when
> > needed, and bisect to lower the number of required transactions
> > between the agent and the firmware.
> >
> 
> Ah so, this is nothing to do with set_parent, but just an optimisation.
> This change optimises for space but some other platform may have all the
> space but the communication with SCMI platform is not good enough to make
> runtime calls like this change. How do we cater that then ?

This change does not optimize memory. It implements a real clk_round_rate()
operation for SCMI clocks that have a discrete supported rates list. The
existing implementation does not support it, it behaves as if the
requested clock is supported and let caller change the clock rate to
find out which rounded rate it effectively gets. This does not suit 
audio and video clock constraints.

How to deal between platforms with large memory/slow SCMI 
communication and those with the opposite? I think the easiest way
would be to have a dedicated SCMI Clock protocol command.

> 
> We need some spec-ed way or a unique way to identify what is best for
> the platform IMO. We can change the way you have done in this change set
> as someone else may complain in the future that it is costly to send
> such command every time a clock needs to be set. I am just guessing here
> may not be true.
> 
> > >
> > > > Parse the rate list array when the target rate fit in the bounds
> > > > of the command response for simplicity.
> > > >
> > >
> > > I don't understand what you mean by this.
> >
> > I meant here that we bisect into supported rates when communicating
> > with the firmware but once the firmware response provides list portion
> > when target rate fits into, we just scan into that array instead of bisecting
> > into. We could also bisect into that array but it is likely quite small
> > (<128 byte in existing SCMI transport drivers) and that would add a bit
> > more code for no much gain IMHO.
> >
> >
> > >
> > > > If so some reason the sequence fails or if the SCMI driver has no
> > > > round_rate SCMI clock handler, then fallback to the legacy strategy that
> > > > returned the target rate value.
> > > >
> > >
> > > Hmm, so we perform some extra dance but we are okay to fallback to default.
> > > I am more confused.
> >
> > Here, I propose to preserve the exiting sequence in clk/clk-scmi.c in case
> > arm_scmi/clock.c does not implement this new round_rate SCMI clock
> > operation (it can be the case if these 2 drivers are .ko modules, not
> > well known built-in drivers).
> >
> 
> I don't think it would work if it is not built on the same kernel anyways.
> I don't work much about this use-case.

Using the same kernel will not enforce the driver was not modified regarding
the vanilla upstream version. This may be also true for built-in modules
I guess.

> 
> > >
> > > > Operation handle scmi_clk_determine_rate() is change to get the effective
> > > > supported rounded rate when there is no clock re-parenting operation
> > > > supported. Otherwise, preserve the implementation that assumed any
> > > > clock rate could be obtained.
> > > >
> > >
> > > OK, no I think I am getting some idea. Is this case where the parent has
> > > changed and the describe rates can give a different result at run-time.
> >
> > This does not deal with whether parent has changed or not. I would expect
> > the same request sent multiple times to provide the very same result. But
> > as I said above, I don't think arm_scmi/clock.c should consume a possibly
> > large array of memory to store all supported rate each of the SCMI clocks
> > (that describe discrete rates).
> >
> 
> Right, my assumption was totally wrong. Thanks for confirming.
> 
> > An alternate way could be to add an SCMI Clock protocol command in the
> > spec allowing agent to query a closest supported rate, in 1 shot. Maybe
> > this new command could return both rounded rate and the SCMI parent
> > clock needed to reach that rounded rate, better fitting clk_determine_rate()
> > expectations.
> >
> 
> May be that would be ideal but you need to make a case for such a spec change.

We need effective round_rate support for STM32MP2 platforms where audio
and video clocks are provided by a clock exposed by the SCMI server. These
drivers detect the (possibly external) device needs at runtime and need
to select an input clock that fits some constraints for quality reason.
Audio quality is the most sensible to clock rate inaccuracy.

> 
> > >
> > > I need to re-read the part of the spec, but we may need some clarity so
> > > that this implementation is not vendor specific. I am yet to understand this
> > > fully. I just need to make sure spec covers this aspect and anything we
> > > add here is generic solution.
> > >
> > > I would like to avoid this extra query if not required which you seem to
> > > have made an attempt but I just want to be thorough and make sure that's
> > > what we need w.r.t the specification.
> >
> > Sure, I indeed prefer clear and robust implementation in the long term,
> > being the one I propose here or another one.
> >
> 
> Good then, we can work towards achieving that. If you can specify how slow
> or memory hungry is it without these changes and how much this change helps
> your platform, we can take it up with spec authors and see if they are happy
> to provide some alternative to deal with this in a generic way.

The platforms we target usually have plenty of RAM, lets say hundreds of MBytes.
Not that much for some system but enough I guess to store a few hundreds of
clock rates for a few dozen of clocks (few kByte of RAM).

That said, thinking more and more about this, I really belive a dedicate SCMI
clock protocol command would better fit platform needs in the long term.

BR,
Etienne

> 
> --
> Regards,
> Sudeep
> 

Re: [PATCH v2 2/2] firmware: arm_scmi: round rate bisecting in discrete rates

[Etienne CARRIERE - foss]

On Tuesday, December 10, 2024, Etienne CARRIERE - foss wrote:
> On Monday, December 9, 2024, Sudeep Holla wrote:
> > On Mon, Dec 09, 2024 at 12:59:58PM +0000, Etienne CARRIERE - foss wrote:
> (...)
> > > > I would like to avoid this extra query if not required which you seem to
> > > > have made an attempt but I just want to be thorough and make sure that's
> > > > what we need w.r.t the specification.
> > >
> > > Sure, I indeed prefer clear and robust implementation in the long term,
> > > being the one I propose here or another one.
> > >
> >
> > Good then, we can work towards achieving that. If you can specify how slow
> > or memory hungry is it without these changes and how much this change helps
> > your platform, we can take it up with spec authors and see if they are happy
> > to provide some alternative to deal with this in a generic way.
> 
> The platforms we target usually have plenty of RAM, lets say hundreds of MBytes.
> Not that much for some system but enough I guess to store a few hundreds of
> clock rates for a few dozen of clocks (few kByte of RAM).

During last SCMI monthly meeting [1], there was a suggestion to use a lazy
query of the full clock discrete rates list: the first time clk_round_rate()
is requested on an SCMI clock with discrete rates, we can allocated and get
the full rate list once in struct scmi_clock_info. It will be ease to locally bisect
or scan into these supported rates for clk_round_rate() operations.

It would prevent to store the full discrete rates list for all SCMI clocks when
clk_round_rate() is queried only for a very few of them.

Do you think it would be a good compromise? 

[1] https://linaro.atlassian.net/wiki/spaces/SCMI/overview#Meetings

BR,
Etienne

> 
> That said, thinking more and more about this, I really belive a dedicate SCMI
> clock protocol command would better fit platform needs in the long term.
> 
> BR,
> Etienne
> 
> >
> > --
> > Regards,
> > Sudeep
> >
>