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

[Marek Vasut <marex@denx.de>]

On 6/12/23 10:26, Arnaud POULIQUEN wrote:
> 
> 
> On 6/10/23 15:46, Marek Vasut wrote:
>> On 6/7/23 11:53, Arnaud POULIQUEN wrote:
>>
>> Hi,
>>
>> [...]
>>
>>>>>>>>> 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.
>>>>>>
>>>>>> Why should every single board add it separately and duplicate the
>>>>>> same stuff, if they can all start with it, and if anyone needs to
>>>>>> tweak the mailbox allocation, then they can do that in the board DT ?
>>>>>> I really don't like the duplication suggestion here.
>>>>>
>>>>> I was speaking about "detach mailbox. Here is what I would like to
>>>>> propose to you
>>>>>
>>>>> 1)  move all the mailbox declaration in the DTSI except "detach"
>>>>> 2) don't declare "detach" in boards DTS ( except ST board for legacy
>>>>> compliance)
>>>>> 3) as a next step we will have to fix the unexpected warning on the
>>>>>       "detach" mailbox.
>>>>
>>>> Why not make the mailbox available by default on all boards ?
>>>
>>> It has been introduced mainly to test the detach feature,
>>> on a second platform to help remoteproc maintainers for the review
>>> process. But the feature is not fully implemented on stm32mp1
>>> ( auto reboot of thye M4 on crash, appropriate resource table clean-up,...)
>>
>> This is a driver bug, unrelated to DT description, please just fix it.
> 
> No, it is a system usecase that depends on Hardware, M4 firmware, A7
> applications, ...
> The detach/re-attach is a quite complex feature that needs to understand
> the whole picture. As already mentioned above it as been introduced for
> test on upstream.
> 
>>
>>> I would prefer to remove it in ST board DT than add it in the DTSI.
>>> That said as mentioned for legacy support, better to keep for ST board.
>>
>> Why not remove it from ST boards if this was legacy test feature and is no
>> longer needed ?
> 
> If you can guaranty that this will not introduce regression on legacy, yes we can.

Then clearly the only way to avoid this fragmentation is to add it on 
all boards.

>>>> As far as I can tell, if the software is not using the detach mailbox, there
>>>> is no
>>>> downside, it would just be unused. User can always remove it in their DT if
>>>> really needed.
>>>
>>> The inverse it true, User can add it in their DT if really need.
>>
>> Is there a downside if this is enabled by default, yes or no ?
> 
> Yes, by doing this you impose that this channel is reserved for the detach.
> making it no more optional.

Not really, the user can still define whatever channels they need for 
their custom setup in their DT. The SoC DTSI should be just a default.

>>>> I believe once can build demos using the detach mailbox for boards with
>>>> stm32mp15xx not manufactured by ST, correct ?[]
>>>
>>> Everything could be possible...
>>> Once can want to replace the shutdown mailbox by the detach.
>>> Once can also build demo using the detach mailbox channel for another purpose.
>>>
>>> I quite confuse why you insist to declare this detach mailbox in the DTSI?
>>> Is there a strong constraint on your side?
>>
>> I just don't see any explanation why ST board(s) should be somehow special and
>> have the detach mailbox described in DT by default, while other boards would
>> require separate DT patch to use the detach mailbox first. That just reduces
>> usability of non-ST-manufactured boards and increases fragmentation. I do not
>> like that.
> 
> The "somehow special" should only be an internal M4 firmware  allowing to test
> the feature to help remoteproc maintainers to verify it on demand.
> But I can not know if someone in the community have another firmware using
> detach on the ST boards.
> Anyway what you propose here is that we impose it for all boards. Some boards
> would require separate DT patch to not use it. We just inverse the things...
> The difference is that I would not impose something optional.

I believe it is better to have single capable consistent default and let 
users remove the capabilities for specific application if needed, than 
to have fragmented inconsistent board-specific configurations where 
users have to first determine whether they need to patch them to add 
missing capabilities, and then possibly patch them, that's just a mess.

It also puts non-ST-manufactured boards into worse position.

> In term of fragmentation I can only see a specificity for the ST boards.As
> already said if you can ensure that this will not generate impact on legacy,
> it can be removed from the ST DT.
> 
> @Alex: any opinion on that?

[...]