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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 10/07/2023 23:52, Marek Vasut 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 ...
> 
>> , 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 ?

Put some working value in the driver. If this does not help, complain to
NXP about their SDK firmware. I know how that would work - NXP does not
really care about customers and open-source - but that should not be
really an excuse for us.

Best regards,
Krzysztof