Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Mon, 10 Jul 2023 at 15:53, Marek Vasut <marex@denx.de> wrote:
>
> On 7/10/23 22:00, Krzysztof Kozlowski wrote:
> > On 10/07/2023 15:46, Marek Vasut wrote:
> >> On 7/10/23 14:52, Krzysztof Kozlowski wrote:
> >>> On 10/07/2023 11:18, Marek Vasut wrote:
> >>>> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
> >>>>> On 08/07/2023 01:24, Marek Vasut wrote:
> >>>>>> Document fsl,startup-delay-ms property which indicates how long
> >>>>>> the system software should wait until attempting to communicate
> >>>>>> with the CM firmware. This gives the CM firmware a bit of time
> >>>>>> to boot and get ready for communication.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>> ---
> >>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
> >>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
> >>>>>> Cc: Fabio Estevam <festevam@gmail.com>
> >>>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> >>>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
> >>>>>> Cc: Peng Fan <peng.fan@nxp.com>
> >>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> >>>>>> Cc: Rob Herring <robh+dt@kernel.org>
> >>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> >>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
> >>>>>> Cc: devicetree@vger.kernel.org
> >>>>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>>>> Cc: linux-remoteproc@vger.kernel.org
> >>>>>> ---
> >>>>>>     .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
> >>>>>>     1 file changed, 5 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> >>>>>> index 0c3910f152d1d..c940199ce89df 100644
> >>>>>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> >>>>>> @@ -76,6 +76,11 @@ properties:
> >>>>>>           This property is to specify the resource id of the remote processor in SoC
> >>>>>>           which supports SCFW
> >>>>>>
> >>>>>> +  fsl,startup-delay-ms:
> >>>>>> +    default: 0
> >>>>>> +    description:
> >>>>>> +      CM firmware start up delay.
> >>>>>
> >>>>> I don't see particular improvements from v2 and no responses addressing
> >>>>> my comment:
> >>>>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
> >>>>
> >>>> I wasn't aware of this being submitted before, esp. since I wrote the
> >>>> binding document from scratch. Which comment is not addressed, the type
> >>>> ref is not present and the sentence starts with caps, so what is missing ?
> >>>
> >>>
> >>> That the property looks like a hacky solution to some SW problem. Why
> >>> this delay should be different on different boards?
> >>
> >> It probably depends more on the CM4 firmware that is being launched. The
> >> ones I tested were fine with 50..500ms delay, but the delay was always
> >> needed.
> >
> > If this is for some official remoteproc FW running on M4
>
> It is not, it is some SDK which can be downloaded from NXP website,
> which can then be used to compile the firmware blob. The license is
> BSD-3 however, so it is conductive to producing binaries without
> matching sources ...
>

Why can't the SDK be upgraded to provide some kind of hand-shake
mechanism, as suggested when I first reviewed this patchset?

> >, then probably
> > this could be implied by compatible. Otherwise, if this depends on
> > actual M4 firmware which can totally vary between each board of the same
> > type (I can run my own FW on M4, right?
>
> Yeah
>
> > ), then it is not suitable DT
> > property. How it would even look like? You add here 500 ms for all known
> > firmwares and then someone comes with FW requiring delay of 600 ms.
>
> I would say, some sane default and if some firmware is specifically
> weird, just up the delay. It is better than no firmware working at all.
>
> Do you have a better hint how to deal with this ?