Message-ID: <dbc60fcb-1759-49e8-90da-6afce5075fbf@collabora.com>
Date: Wed, 2 Apr 2025 11:58:31 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Jjian Zhou <jjian.zhou@...iatek.com>,
 Jassi Brar <jassisinghbrar@...il.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH RFC v2 0/3] add VCP mailbox and IPC driver

Il 18/03/25 08:44, Chen-Yu Tsai ha scritto:
> On Mon, Mar 17, 2025 at 6:07 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.com> wrote:
>>
>> Il 17/03/25 09:38, Jjian Zhou ha scritto:
>>> The VCP mailbox has 5 groups. Each group has corresponding interrupts,
>>> registers, and 64 slots (each slot is 4 bytes). Since different features
>>> share one of the mailbox groups, the VCP mailbox needs to establish a
>>> send table and a receive table. The send table is used to record the
>>> feature ID, mailbox ID, and the number of slots occupied. The receive table
>>> is used to record the feature ID, mailbox ID, the number of slots occupied,
>>> and the receive options. The API setup_mbox_table in mtk-vcp-ipc.c calculates
>>> the slot offset and pin index for each feature ID based on the mailbox ID and
>>> slot number in the send and receive tables (several slots form a pin, and
>>> each pin can trigger an interrupt). These descriptions are written in the
>>> mtk-vcp-ipc.c file -- we call it the IPC layer.
>>>
>>> We have two questions:
>>> How should we describe the mailbox and IPI?
>>> Can the intermediate IPC layer be rewritten as a virtual mailbox layer?
>>>
>>
>> So, for this remote processor messaging system you have:
>>    - Dynamic channel allocation
>>      - Each channel has its own endpoint
> 
> The rpmsg model has:
> 
> - device -> the remote processor
> - channel
> - endpoint
> 
> However here for the VCP and possibly all the coprocessors using the
> tinysys model, channel and endpoint are basically the same.

For now, yes. Though, I expect multiple endpoints to become a thing in future
iterations of MediaTek SoCs, and this is based off how the hardware seems to
be evolving.

> If we
> consider the "channel" to be the storage plus the interrupt vector,
> and the "endpoint" to be the function running on the remote processor
> servicing a given IPI ID, then it's always one endpoint per channel.

Like this, yes - but if you consider ipi_id as the endpoint things will change.

Alternatively, if you consider the endpoint as function running on the remote
processor as you propose, and that I think could be the better alternative,
I still expect functions to grow in future SoCs.

> 
> IMHO rpmsg gives too much latitude to make things confusing here.
> 
> rpmsg also requires the remote processor to support name service
> announcements, which really doesn't exist.

I have doubts about that: all this is not properly documented and a kind of
service announcement could actually be existing - but let's assume that it
does not as that's the right thing to do.

There's still a way around that anyway, and even though it's not the prettiest
thing on Earth, it's not a big deal imo.

> The endpoints and how
> they map to the various hardware mailbox interrupt vectors and
> storage is statically allocated, and thus needs to be described
> in the driver.
> 

I'm not sure I understand this sentence, but it feels like this can be avoided
by simply using a cell in devicetree.

rpmsg = <&something 1 0>;  or  rpmsg = <&something 0>;

>>      - Each channel has its own interrupt
>>    - Data send operation
>>      - Both with and without ACK indication from the remote processor
>>      - To channel -> endpoint
>>    - Data receive operation
>>      - From channel <- endpoint
>>      - When interrupt fires
>>      - Could use polling to avoid blocking in a few cases
>>    - A custom message structure not adhering to any standard
>>
>> Check drivers/rpmsg/ :-)
> 
> While discussing this internally, I felt like that wasn't a really
> correct model. IIUC rpmsg was first created to allow userspace to
> pass messages to the remote processor. Then somehow devices were
> being created on top of those channels.
> 

Heh, if I recall correctly, I did see some userspace messaging in one of the
downstream kernels for other chips that are heavily using the IPI - check below
for a model hint :-)

> Also, the existing mtk_rpmsg driver seemed a bit weird, like requiring
> a DT node for each rpmsg endpoint.
> 

Weird... it's weird, agreed - but I call that necessary evil.
The other way around could be worse (note: that statement is purely by heart and
general knowledge around MediaTek SoCs, not about any specific code in particular).

> That's why I thought mailboxes made more sense, as the terminology mapped
> better. As a result I never brought up rpmsg in the discussion.

I think I do understand your thinking here - and I am not *strongly* disagreeing,
but I don't really agree for the reasons that I'm explaining in this reply.

> 
> Perhaps that could be improved with better documentation for the MediaTek
> specific implementation.
> 

Now that's what I'd really like to see here, because I feel like many things around
MediaTek SoCs are suboptimally engineered (in the software sense, because in the HW
sense I really do like them) and the *primary* reason for this is exactly the lack
of documentation... -> even internally <-.

>> On MediaTek platforms, there are many IPI to handle in many subsystems for
>> all of the remote processors that are integrated in the SoC and, at this
>> point, you might as well just aggregate all of the inter processor communication
>> stuff in one place, having an API that is made just exactly for that, instead
>> of keeping to duplicate the IPI stuff over and over (and yes I know that for each
>> remote processor the TX/RX is slightly different).
>>
>> If you aggregate the IPI messaging in one place, maintenance is going to be easier,
>> and we stop getting duplication... more or less like it was done with the mtk_scp
>> IPI and mtk-vcodec .. and that's also something that is partially handled as RPMSG
>> because, well, it is a remote processor messaging driver.
>>
>> Just to make people understand *how heavily* MediaTek SoCs rely on IPI, there's
>> a *partial* list of SoC IPs that use IPI communcation:
>>
>> thermal, ccu, ccd, tinysys, vcp, atsp, sspm, slbc, mcupm, npu, mvpu, aps, mdla,
>> qos, audio, cm_mgr.... and... again, it's a partial list!
> 
> Indeed, the newest chip has become quite complicated.
> 

..and I'd like to add: for many good reasons :-)

>> That said... any other opinion from anyone else?
> 
> I tried to describe why I thought a virtual mailbox was better.
> 

The implementation issue arising with a virtual mailbox driver is that then each
driver for each IP (thermal, ccu, vcp, slbc, this and that) will contain a direct
copy of the same part-two implementation for IPI communication: this can especially
be seen on downstream kernels for MediaTek Dimensity 9xxx smartphone chips.

If done with a mailbox, there's going to be no way around it - describing it all
will be very long, so I am not doing that right now in this reply, but I invite
you to check the MT6989 kernel to understand what I'm talking about :-)

Cheers!

> 
> Thanks
> ChenYu
> 
>> Cheers,
>> Angelo
>>
>>> Example of send and recve table:
>>> Operation | mbox_id | ipi_id | msg_size | align_size | slot_ofs | pin_index |  notes
>>> send          0          0       18          18           0          0
>>> recv          0          1       18          18          18          9
>>> send          1         15        8           8           0          0
>>> send          1         16       18          18           8          4
>>> send          1          9        2           2          26         13
>>> recv          1         15        8           8          28         14       ack of send ipi_id=15
>>> recv          1         17       18          18          36         18
>>> recv          1         10        2           2          54         27       ack of send ipi_id=2
>>> send          2         11       18          18           0          0
>>> send          2          2        2           2          18          9
>>> send          2          3        3           4          20         10
>>> send          2         32        2           2          24         12
>>> recv          2         12       18          18          26         13
>>> recv          2          5        1           2          44         22
>>> recv          2          2        1           2          46         23
>>>
>>> Recv ipi_id=2 is the ack of send ipi_id=2(The ipi_id=15 is the same.)
>>>
>>> Jjian Zhou (3):
>>>     mailbox: mediatek: Add mtk-vcp-mailbox driver
>>>     firmware: mediatek: Add vcp ipc protocol interface
>>>     dt-bindings: mailbox: mtk,vcp-mbox: add mtk vcp-mbox document
>>>
>>>    .../bindings/mailbox/mtk,mt8196-vcp-mbox.yaml |  49 ++
>>>    drivers/firmware/Kconfig                      |   9 +
>>>    drivers/firmware/Makefile                     |   1 +
>>>    drivers/firmware/mtk-vcp-ipc.c                | 481 ++++++++++++++++++
>>>    drivers/mailbox/Kconfig                       |   9 +
>>>    drivers/mailbox/Makefile                      |   2 +
>>>    drivers/mailbox/mtk-vcp-mailbox.c             | 179 +++++++
>>>    include/linux/firmware/mediatek/mtk-vcp-ipc.h | 151 ++++++
>>>    include/linux/mailbox/mtk-vcp-mailbox.h       |  34 ++
>>>    9 files changed, 915 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/mailbox/mtk,mt8196-vcp-mbox.yaml
>>>    create mode 100644 drivers/firmware/mtk-vcp-ipc.c
>>>    create mode 100644 drivers/mailbox/mtk-vcp-mailbox.c
>>>    create mode 100644 include/linux/firmware/mediatek/mtk-vcp-ipc.h
>>>    create mode 100644 include/linux/mailbox/mtk-vcp-mailbox.h
>>>
>>
>>
>>

Powered by blists - more mailing lists