| Message ID | 20241203173908.3148794-2-etienne.carriere@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | firmware: arm_scmi: unbound discrete rates, support round rate | expand |
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
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 >
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
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 >
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;
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(-)