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

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

On Tue, Jul 11, 2023 at 12:23:02AM +0200, Marek Vasut wrote:
> On 7/11/23 00:01, Mathieu Poirier wrote:
> > 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?
> 
> I'd argue because of legacy firmware that is already deployed.
> New firmware builds can, old ones probably cannot be fixed.
> 
> Do you have a suggestion how such a mechanism should look like?
> As far as I can tell, the MX8M SDK stuff looks very similar to the STM32
> Cube stuff, so maybe the mechanism is already there ?

Either with a flag in the config space of the resource table or implicit
synchronization using the mailbox.  I suggest to have a look at struct
mbox_client where tx_block, knows_txdone and tx_done should be useful.  I'd use
those with a completion in rproc::ops::prepare() or rproc_ops::start().