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

[Cristian Marussi <cristian.marussi@arm.com>]

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