ST Restricted
> -----Original Message-----
> From: Marek Vasut <marex@xxxxxxx>
> Sent: Tuesday, May 30, 2023 1:51 PM
> To: Arnaud POULIQUEN <arnaud.pouliquen@xxxxxx>; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Cc: devicetree@xxxxxxxxxxxxxxx; Conor Dooley <conor+dt@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Richard Cochran
> <richardcochran@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Maxime
> Coquelin <mcoquelin.stm32@xxxxxxxxx>; linux-stm32@st-md-
> mailman.stormreply.com; kernel@xxxxxxxxxxxxxxxxxx
> 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@st-md-
> mailman.stormreply.com>
> >> On Behalf Of Marek Vasut
> >> Sent: Thursday, May 18, 2023 3:13 AM
> >> To: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >> Cc: Marek Vasut <marex@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; Conor
> >> Dooley <conor+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Richard Cochran
> >> <richardcochran@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> Maxime
> >> Coquelin <mcoquelin.stm32@xxxxxxxxx>; linux-stm32@st-md-
> >> mailman.stormreply.com; kernel@xxxxxxxxxxxxxxxxxx
> >> 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@xxxxxxx>
> >> ---
> >> Cc: Alexandre Torgue <alexandre.torgue@xxxxxxxxxxx>
> >> Cc: Conor Dooley <conor+dt@xxxxxxxxxx>
> >> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>
> >> Cc: Maxime Coquelin <mcoquelin.stm32@xxxxxxxxx>
> >> Cc: Richard Cochran <richardcochran@xxxxxxxxx>
> >> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> >> Cc: devicetree@xxxxxxxxxxxxxxx
> >> Cc: kernel@xxxxxxxxxxxxxxxxxx
> >> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >> Cc: linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx
> >> ---
> >> 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
