[Linux-stm32] [PATCH 1/5] ARM: dts: stm32: Add missing detach mailbox for emtrion emSBC-Argon

Thu Jun 1 05:56:16 PDT 2023

ST Restricted

> -----Original Message-----
> From: Marek Vasut <marex at denx.de>
> Sent: Tuesday, May 30, 2023 1:51 PM
> To: Arnaud POULIQUEN <arnaud.pouliquen at st.com>; linux-arm-
> kernel at lists.infradead.org
> Cc: devicetree at vger.kernel.org; Conor Dooley <conor+dt at kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt at linaro.org>; Richard Cochran
> <richardcochran at gmail.com>; Rob Herring <robh+dt at kernel.org>; Maxime
> Coquelin <mcoquelin.stm32 at gmail.com>; linux-stm32 at st-md-
> mailman.stormreply.com; kernel at dh-electronics.com
> Subject: Re: [Linux-stm32] [PATCH 1/5] ARM: dts: stm32: Add missing detach
> mailbox for emtrion emSBC-Argon
> 
> On 5/30/23 10:43, Arnaud POULIQUEN wrote:
> > Hello Marek,
> 
> Hi,
> 
> > ST Restricted
> >
> >> -----Original Message-----
> >> From: Linux-stm32 <linux-stm32-bounces at st-md-
> mailman.stormreply.com>
> >> On Behalf Of Marek Vasut
> >> Sent: Thursday, May 18, 2023 3:13 AM
> >> To: linux-arm-kernel at lists.infradead.org
> >> Cc: Marek Vasut <marex at denx.de>; devicetree at vger.kernel.org; Conor
> >> Dooley <conor+dt at kernel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt at linaro.org>; Richard Cochran
> >> <richardcochran at gmail.com>; Rob Herring <robh+dt at kernel.org>;
> Maxime
> >> Coquelin <mcoquelin.stm32 at gmail.com>; linux-stm32 at st-md-
> >> mailman.stormreply.com; kernel at dh-electronics.com
> >> Subject: [Linux-stm32] [PATCH 1/5] ARM: dts: stm32: Add missing
> >> detach mailbox for emtrion emSBC-Argon
> >>
> >> Add missing "detach" mailbox to this board to permit the CPU to
> >> inform the remote processor on a detach. This signal allows the
> >> remote processor firmware to stop IPC communication and to
> >> reinitialize the resources for a re-attach.
> >>
> >> Without this mailbox, detach is not possible and kernel log contains
> >> the following warning to, so make sure all the STM32MP15xx platform
> >> DTs are in sync regarding the mailboxes to fix the detach issue and the
> warning:
> >> "
> >> stm32-rproc 10000000.m4: mbox_request_channel_byname() could not
> >> locate channel named "detach"
> >> "
> >>
> >> Fixes: 6257dfc1c412 ("ARM: dts: stm32: Add coprocessor detach mbox on
> >> stm32mp15x-dkx boards")
> >> Signed-off-by: Marek Vasut <marex at denx.de>
> >> ---
> >> Cc: Alexandre Torgue <alexandre.torgue at foss.st.com>
> >> Cc: Conor Dooley <conor+dt at kernel.org>
> >> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt at linaro.org>
> >> Cc: Maxime Coquelin <mcoquelin.stm32 at gmail.com>
> >> Cc: Richard Cochran <richardcochran at gmail.com>
> >> Cc: Rob Herring <robh+dt at kernel.org>
> >> Cc: devicetree at vger.kernel.org
> >> Cc: kernel at dh-electronics.com
> >> Cc: linux-arm-kernel at lists.infradead.org
> >> Cc: linux-stm32 at st-md-mailman.stormreply.com
> >> ---
> >>   arch/arm/boot/dts/stm32mp157c-emstamp-argon.dtsi | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/stm32mp157c-emstamp-argon.dtsi
> >> b/arch/arm/boot/dts/stm32mp157c-emstamp-argon.dtsi
> >> index b01470a9a3d53..82061c9186338 100644
> >> --- a/arch/arm/boot/dts/stm32mp157c-emstamp-argon.dtsi
> >> +++ b/arch/arm/boot/dts/stm32mp157c-emstamp-argon.dtsi
> >> @@ -366,8 +366,8 @@ &iwdg2 {
> >>   &m4_rproc {
> >>   	memory-region = <&retram>, <&mcuram>, <&mcuram2>,
> <&vdev0vring0>,
> >>   			<&vdev0vring1>, <&vdev0buffer>;
> >> -	mboxes = <&ipcc 0>, <&ipcc 1>, <&ipcc 2>;
> >> -	mbox-names = "vq0", "vq1", "shutdown";
> >> +	mboxes = <&ipcc 0>, <&ipcc 1>, <&ipcc 2>, <&ipcc 3>;
> >> +	mbox-names = "vq0", "vq1", "shutdown", "detach";
> >
> > Why do you want to add the detach mailbox?
> > It looks to me here that you want to clean the warning message, right?
> 
> Yes
> 
> > The detach is used in a particular usecase where the main processor is
> > shutdown while the coprocessor is still running.
> > I would prefer to not enable it by default as it need a specific
> > coprocessor Firmware.
> 
> Why is it enabled by default on ST boards and left out on all other boards ?

Mainly to be able to test the feature on the K2 board . And ensure that the generic
Implementation proposed was compatible. We do not provide demo in the ST
Distribution. Remove it could be an option, but this could ipact legacy some M4 firmware.

> 
> Surely the ST evaluation boards can load and run both types of firmware,
> ones which do use the detach mailbox and ones which do not use the detach
> mailbox , right ?

Yes,

> 
> I assume that if the firmware does not use the detach mailbox, then the
> detach mailbox is just ignored and unused, so there is no problem with
> having it described in the DT in any case ?

Yes, The aim of the ST evaluation board is to provide a DT  to a support
different firmwares for demo and tests.  But it is not the case of all boards...
If your boards provide demo using the "detach" it is justified.
If you just add it as a workaround to mask the warnings, you just mask the issue.

> 
> And if that's the case, then I would much rather prefer to have all the boards
> describe the same set of mailboxes, so they don't diverge . What do you
> think ?
> 

I would avoid this.  It is only a configuration by default for current demo.
The allocation depends on the firmware loaded on M4, so depend on the project.
For instance, a work has started in OpenAMP community to implement the vIrtio Services
For the IPC.  Each virtio services would be associated to one or several mailbox
Channels.  In this case we would need to arbitrate allocations.
The result could be that we propose a virtio channel for rpmsg + some other virtio.
More than that we probably manage the mailboxes in sub node
Here is an RFC on the topic (https://lore.kernel.org/lkml/20220920202201.GB1042164@p14s/)

That said, fixing rpmsg virqueue and the shutdown mailboxes in the  SoC dtsi, seems reasonable as it
provides the default expected implementation.
Do the same for the detach that is optional and mainly unused, I'm not fan.
Adding the detach mailbox in the DT to mask a warning issue, I'm rather against it

> > Rather than adding unused optional mailbox, I will more in favor of
> > having a mbox_request_channel_byname_optional helper or something
> > similar
> 
> See above, I think it is better to have the mailbox described in DT always and
> not use it (the user can always remove it), than to not have it described on
> some boards and have it described on other boards (inconsistency).

Adding it in the DT ( and especially in the Soc DTSI) can also be interpreted as
"it is defined so you must use it". I would expect that the Bindings already provide 
the information to help user to add it on need.

Regards,
Arnaud