Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce property mbox-rx-timeout-ms

[Sudeep Holla <sudeep.holla@arm.com>]

On Thu, Jun 27, 2024 at 11:17:49PM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: introduce
> > property mbox-rx-timeout-ms
> >
> > On Fri, Jun 21, 2024 at 08:46:57PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > System Controller Management Interface(SCMI) firmwares might
> > have
> > > different designs by SCMI firmware developers. So the maximum
> > receive
> > > channel timeout value might also varies in the various designs.
> > >
> > > So introduce property mbox-rx-timeout-ms to let each platform could
> > > set its own timeout value in device tree.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 6
> > ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git
> > a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > index 4d823f3b1f0e..d6cc2bf4c819 100644
> > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > @@ -121,6 +121,12 @@ properties:
> > >        atomic mode of operation, even if requested.
> > >      default: 0
> > >
> > > +  max-rx-timeout-ms:
> > > +    description:
> > > +      An optional time value, expressed in milliseconds, representing,
> > on this
> > > +      platform, the mailbox maximum timeout value for receive
> > channel.
> >
> > "on this platform"? Doesn't every property apply to the given platform?
>
> Yeah, apply to all the use mailbox.
>
> >
> > > +    default: 0
> >
> > 0 means no timeout or response is instant?
>
> I should use 30ms same as what the driver currently use.
>

That sounds very wrong to me. The binding is independent of current driver
behaviour. How the driver handles the case of default 0 value is different
from what the default value in the DT means IMO. You can't just set a default
value in the DT binding based on the current driver setting.

We can always say since it is optional, absence of it is what driver handles
as 30ms. 0ms is impossible or incorrect value as transport involves some
delay even if it is in terms of uS. So I prefer to set a value of > 0 in DT
and make that a requirement.

--
Regards,
Sudeep