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

[Marek Vasut]

On 7/11/23 18:21, Mathieu Poirier wrote:
> 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 at 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 at denx.de>
>>>>>>>>>> ---
>>>>>>>>>> Cc: Bjorn Andersson <andersson at kernel.org>
>>>>>>>>>> Cc: Conor Dooley <conor+dt at kernel.org>
>>>>>>>>>> Cc: Fabio Estevam <festevam at gmail.com>
>>>>>>>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt at linaro.org>
>>>>>>>>>> Cc: Mathieu Poirier <mathieu.poirier at linaro.org>
>>>>>>>>>> Cc: NXP Linux Team <linux-imx at nxp.com>
>>>>>>>>>> Cc: Peng Fan <peng.fan at nxp.com>
>>>>>>>>>> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>
>>>>>>>>>> Cc: Rob Herring <robh+dt at kernel.org>
>>>>>>>>>> Cc: Sascha Hauer <s.hauer at pengutronix.de>
>>>>>>>>>> Cc: Shawn Guo <shawnguo at kernel.org>
>>>>>>>>>> Cc: devicetree at vger.kernel.org
>>>>>>>>>> Cc: linux-arm-kernel at lists.infradead.org
>>>>>>>>>> Cc: linux-remoteproc at 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().

I added the following to the CM7 BSP from NXP, which removes the need 
for the extra delay. I believe that is also the proper fix. Whether NXP 
will pick it up in some form, is up to NXP.

This whole startup-delay patch is now unnecessary for me, i.e. I stop here.

"
Run RPMSG init with IRQs globally disabled

The rpmsg_lite_remote_init() function runs in parallel with Linux side
rpmsg_probe()->virtqueue_notify()->rproc_virtio_notify() which raises an
IRQ on CM7 side. Unless IRQs are disabled during rpmsg_lite_remote_init()
time, it is possible the kick from CA53 side would be received and end up
in MU_M7_IRQHandler()->env_isr()->virtqueue_notification() for virtqueue
which is not yet fully initialized. Such IRQ would then be discarded or
mishandled, and rpmsg_lite_wait_for_link_up() would never complete. The
firmware would then fail to communicate with CA53 side.

Fix this by running the RPMSG initialization with global IRQs off, which
delays the reception of IRQ from CA53 side until after the virtqueues are
fully and properly initialized, and the IRQ can be properly handled.

diff --git 
a/boards/evkmimx8mn/multicore_examples/rpmsg_lite_str_echo_rtos/main_remote.c 
b/boards/evkmimx8mn/multicore_examples/rpmsg_lite_str_echo_rtos/main_remote.c
index 655287c..936822e 100644
--- 
a/boards/evkmimx8mn/multicore_examples/rpmsg_lite_str_echo_rtos/main_remote.c
+++ 
b/boards/evkmimx8mn/multicore_examples/rpmsg_lite_str_echo_rtos/main_remote.c
@@ -87,6 +87,7 @@ void app_task(void *param)
      /* Print the initial banner */
      PRINTF("\r\nRPMSG String Echo FreeRTOS RTOS API Demo...\r\n");

+    __disable_irq();
  #ifdef MCMGR_USED
      uint32_t startupData;

@@ -100,6 +101,7 @@ void app_task(void *param)
  #else
      my_rpmsg = rpmsg_lite_remote_init((void *)RPMSG_LITE_SHMEM_BASE, 
RPMSG_LITE_LINK_ID, RL_NO_FLAGS);
  #endif /* MCMGR_USED */
+    __enable_irq();

      rpmsg_lite_wait_for_link_up(my_rpmsg, RL_BLOCK);
"