[PATCH RFC v2 0/3] add VCP mailbox and IPC driver
Chen-Yu Tsai
wenst at chromium.org
Thu Apr 17 00:16:34 PDT 2025
Hi,
Sorry for the late reply.
On Wed, Apr 2, 2025 at 5:58 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno at collabora.com> wrote:
>
> 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 at collabora.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.
I don't see it happening yet. What I think will happen is they will add
more coprocessors and corresponding mailbox controllers. And I think we
should focus on what the current firmware implements and try to model
that.
I think we should just ask MediaTek if their firmware interface is fixed
and what they would do if they ran out of IPI space.
> > 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.
Sure, but it's more than likely they will add more IPIs to handle that.
> >
> > 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>;
That could work. Then again rpmsg is some software construct. Also not
all coprocessors implement the same thing. See below.
> >> - 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 :-)
Is that direct messaging or going through some ioctl interface? I think
direct messaging to some system critical firmware without any sanity
checks is potentially dangerous.
> > 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 <-.
That I agree with.
> >> 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 :-)
It also creates new problems, such as resource handover. There are
internal regulators or clock controls that are controlled by the
firmware after the coprocessor is brought up. But until then, in
some cases those resources are still under the control of the kernel;
in other cases they are left turned on by the bootloader so as not
to block consumers. Some delicate (read: fragile) handover was done,
which I'm not very fond of.
> >> 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.
For the code that MediaTek has provided for MT8196, I can say that there
are at least three types of firmware interfaces:
- ADSP
We're running SOF, so that might be different from the Android stuff that
you saw for 9300. The ADSP mailboxes don't have any shared storage or
channels. They are simply doorbells. The shared storage interface is
likely common to SOF implementations
- Tinysys over SCMI
This seems to be the main coprocessor, which seems to be the SSPM.
Judging by the associated drivers, the firmware implements a vendor
extension protocol (0x80) over SCMI, over which it provides various
commands.
>From the code we have it looks like this part manages the QOS, SLBC, and
CM_MGR parts.
- Tinysys over mailbox controllers (with scratch registers)
This covers the other coprocessors such as VCP, GPUEB, MCUPM and
probably others that we don't currently have included in our tree.
This is the part we are discussing.
Given how the communication channels are crammed into the mailboxes,
it's unlikely you can get rid of said part-two implementation. There
needs to be something that maps individual channels onto the mailbox
channels and combined storage. The question is simply how we want to
model this. And regardless of whether it be rpmsg or virtual mailbox,
we will end up with some shared library code to implement this
translation. This is what currently exists as mtk-mbox.c and
mtk_tinysys_ipi.c under drivers/soc/mediaktek/ in our tree. I assume
there is something similar in the code that you have, given that
most of the code we have for MT8196 was ported over from their
internal trees without much modification.
mtk-mbox.c handles the grouped mailboxes, while mtk_tinysys_ipi.c provides
the IPI interface. I assume the latter is what you are referring to as
"part-two IPI". mtk_tinysys_ipi.c goes even further as it builds the
IPI interface on top of rpmsg (implemented in drivers/rpmsg/mtk_rpmsg_mbox.c).
What I would like to see is to drop the low level mailbox stuff from
mtk-mbox.c, move the mailbox combining and repartitioning bits to a
library that implements the IPI interface, using rpmsg or virtual
mailboxes or whatever. I don't see why this would end up with multiple
copies. The only thing that coprocessor drivers need to have is a
table on how the IPIs map to the mailboxes.
> 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 :-)
You'll have to provide a link to that. However I think a lot of the code
for MT8196 is derived from the same source.
Thanks
ChenYu
> 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
> >>>
> >>
> >>
> >>
More information about the linux-arm-kernel
mailing list