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

[Frieder Schrempf <frieder.schrempf@kontron.de>]

On 11.07.23 00:23, 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 ?

I also stumbled upon this problem [1] and I also wonder why this is
specific to NXP or how other AMP systems like STM32MP1 solve this!?

The problem is that the CM4 firmware first needs to register some IRQs
for the mailbox before it can handle the incoming rpmsg messages. No
matter how the firmware is implemented, there will always be a delay
between starting the app and having the IRQ handlers registered.

Without looking at the details, the proper solution would probably be to
implement a way of signaling that the CM4 firmware is ready for incoming
messages and the rproc driver to wait for this signal before trying to
initialize the rpmsg.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2023-March/822285.html