| Message ID | 20240730144707.1647025-4-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add SCMI transport descriptors | expand |
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); I am bit on the fence on that change, it is useful, and we have done similar things before using a command line parameter. 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.
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
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); + return trans->desc; }
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(+)