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

[Etienne CARRIERE - foss <etienne.carriere@foss.st.com>]

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
>