Re: [PATCH RFC v2 0/3] add VCP mailbox and IPC driver

[AngeloGioacchino Del Regno]

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@xxxxxxxxxxxxx> 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