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

[Marek Vasut <marex@denx.de>]

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@lists.infradead.org
>> Cc: Marek Vasut <marex@denx.de>; devicetree@vger.kernel.org; Conor
>> Dooley <conor+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Richard Cochran
>> <richardcochran@gmail.com>; Rob Herring <robh+dt@kernel.org>; Maxime
>> Coquelin <mcoquelin.stm32@gmail.com>; linux-stm32@st-md-
>> mailman.stormreply.com; kernel@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@denx.de>
>> ---
>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> Cc: Richard Cochran <richardcochran@gmail.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Cc: kernel@dh-electronics.com
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-stm32@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 ?

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 ?

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 ?

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 ?

> 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).