Re: [PATCH v1 3/6] firmware: arm_scmi: Use max-rx-timeout-ms from devicetree

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

On Thu, Sep 12, 2024 at 02:05:53PM -0700, Florian Fainelli wrote:
> On 7/30/24 07:47, Cristian Marussi wrote:
> > Override default maximum RX timeout with the value picked from the
> > devicetree, when provided.
> > 
> > Suggested-by: Peng Fan <peng.fan@nxp.com>
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >   drivers/firmware/arm_scmi/driver.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 332cd5207bbc..e7dab0eea540 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -2964,6 +2964,7 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info)
> >   static const struct scmi_desc *scmi_transport_setup(struct device *dev)
> >   {
> >   	struct scmi_transport *trans;
> > +	int ret;
> >   	trans = dev_get_platdata(dev);
> >   	if (!trans || !trans->desc || !trans->supplier || !trans->core_ops)
> > @@ -2980,6 +2981,14 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
> >   	dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier));
> > +	ret = of_property_read_u32(dev->of_node, "max-rx-timeout-ms",
> > +				   &trans->desc->max_rx_timeout_ms);
> > +	if (ret && ret != -EINVAL)
> > +		dev_err(dev, "Malformed max-rx-timeout-ms DT property.\n");
> > +
> > +	dev_info(dev, "SCMI max-rx-timeout: %dms\n",
> > +		 trans->desc->max_rx_timeout_ms);
>

Hi Florian,
 
> I am bit on the fence on that change, it is useful, and we have done similar
> things before using a command line parameter.
> 

I think the requirement around this as it came from from NXP/Peng was that,
depending on the design (HW/FW) of the SCMI platform at hand it can be that
the same default transport timeout (that is by itself much larger than
usually needed) could still be not enough for the transport: I think the
example was mentioning a system with many agents, so that your request
could end-up been "bootle-necked" in the execution queue of the server (AFAIU)

In such a case (if you cannot avoid the issue at first by reviewing your
design...) you have the effective need of describing that specific
platform/transport timeout characteristic and override the built-in default....

...it was not meant to be usead as a plain configuration....even though I
suppose this is sort of a matter of interepretation (and that can be
abused downstream...)

> This is definitively useful when bringing up new systems where you might be
> sprinkling enough debugging messages that this pushes your message
> processing logic too close to the default 30ms timeout. For normal use
> cases, we really want the message timeout to be as small as possible for
> most SCMI traffic but if we want the timeout to be configurable, that might
> have have to be on a per-message basis.

A per-message timeout would need a lot of re-design, but how would you tune
then all the possibly different timeouts across different platforms and/or
transports combinations...this seems something that could need a lot of
runtime calibration and re-configuration on a live system....

...have you specifically seen the need for such varying per-message
timeout in some specific case ?

Thanks,
Cristian