Re: [PATCH RFC 05/10] mailbox: add MediaTek GPUEB IPI mailbox

Chia-I Wu
2025-09-12T17:30:52Z

On Fri, Sep 12, 2025 at 3:59 AM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> On Friday, 12 September 2025 06:48:17 Central European Summer Time Chia-I Wu wrote:
> > On Fri, Sep 5, 2025 at 3:24 AM Nicolas Frattaroli
> > <nicolas.frattaroli@collabora.com> wrote:
> > >
> > > The MT8196 SoC uses an embedded MCU to control frequencies and power of
> > > the GPU. This controller is referred to as "GPUEB".
> > >
> > > It communicates to the application processor, among other ways, through
> > > a mailbox.
> > >
> > > The mailbox exposes one interrupt, which appears to only be fired when a
> > > response is received, rather than a transaction is completed. For us,
> > > this means we unfortunately need to poll for txdone.
> > >
> > > The mailbox also requires the EB clock to be on when touching any of the
> > > mailbox registers.
> > >
> > > Add a simple driver for it based on the common mailbox framework.
> > >
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > > drivers/mailbox/Kconfig | 10 ++
> > > drivers/mailbox/Makefile | 2 +
> > > drivers/mailbox/mtk-gpueb-mailbox.c | 330 ++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 342 insertions(+)
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > > index 02432d4a5ccd46a16156a09c7f277fb03e4013f5..2016defda1fabb5c0fcc8078f84a52d4e4e00167 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -294,6 +294,16 @@ config MTK_CMDQ_MBOX
> > > critical time limitation, such as updating display configuration
> > > during the vblank.
> > >
> > > +config MTK_GPUEB_MBOX
> > > + tristate "MediaTek GPUEB Mailbox Support"
> > > + depends on ARCH_MEDIATEK || COMPILE_TEST
> > > + help
> > > + The MediaTek GPUEB mailbox is used to communicate with the embedded
> > > + controller in charge of GPU frequency and power management on some
> > > + MediaTek SoCs, such as the MT8196.
> > > + Say Y or m here if you want to support the MT8196 SoC in your kernel
> > > + build.
> > > +
> > > config ZYNQMP_IPI_MBOX
> > > tristate "Xilinx ZynqMP IPI Mailbox"
> > > depends on ARCH_ZYNQMP && OF
> > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > index 98a68f838486eed117d24296138bf59fda3f92e4..564d06e71313e6d1972e4a6036e1e78c8c7ec450 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -63,6 +63,8 @@ obj-$(CONFIG_MTK_ADSP_MBOX) += mtk-adsp-mailbox.o
> > >
> > > obj-$(CONFIG_MTK_CMDQ_MBOX) += mtk-cmdq-mailbox.o
> > >
> > > +obj-$(CONFIG_MTK_GPUEB_MBOX) += mtk-gpueb-mailbox.o
> > > +
> > > obj-$(CONFIG_ZYNQMP_IPI_MBOX) += zynqmp-ipi-mailbox.o
> > >
> > > obj-$(CONFIG_SUN6I_MSGBOX) += sun6i-msgbox.o
> > > diff --git a/drivers/mailbox/mtk-gpueb-mailbox.c b/drivers/mailbox/mtk-gpueb-mailbox.c
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..0236fb358136e434a09a21ef293fe949ced94123
> > > --- /dev/null
> > > +++ b/drivers/mailbox/mtk-gpueb-mailbox.c
> > > @@ -0,0 +1,330 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * MediaTek GPUEB mailbox driver for SoCs such as the MT8196
> > > + *
> > > + * Copyright (C) 2025, Collabora Ltd.
> > > + *
> > > + * Developers harmed in the making of this driver:
> > > + * - Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > + */
> > > +
> > > +#include <linux/atomic.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/device.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/mailbox_controller.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define MBOX_CTL_TX_STS 0x0000
> > > +#define MBOX_CTL_IRQ_SET 0x0004
> > > +#define MBOX_CTL_IRQ_CLR 0x0074
> > > +#define MBOX_CTL_RX_STS 0x0078
> > > +
> > > +#define MBOX_FULL BIT(0) /* i.e. we've received data */
> > > +#define MBOX_CLOGGED BIT(1) /* i.e. the channel is shutdown */
> > > +
> > > +struct mtk_gpueb_mbox {
> > > + struct device *dev;
> > > + struct clk *clk;
> > > + void __iomem *mbox_mmio;
> > > + void __iomem *mbox_ctl;
> > > + void **rx_buf;
> > > + atomic_t *rx_status;
> > > + struct mbox_controller mbox;
> > > + unsigned int *chn;
> > > + int irq;
> > > + const struct mtk_gpueb_mbox_variant *v;
> > > +};
> > > +
> > > +struct mtk_gpueb_mbox_ch {
> > > + const char *name;
> > > + const int num;
> > > + const unsigned int tx_offset;
> > > + const unsigned int tx_len;
> > > + const unsigned int rx_offset;
> > > + const unsigned int rx_len;
> > > + const bool no_response;
> > > +};
> > > +
> > > +struct mtk_gpueb_mbox_variant {
> > > + unsigned int num_channels;
> > > + const struct mtk_gpueb_mbox_ch channels[] __counted_by(num_channels);
> > > +};
> > > +
> > > +/**
> > > + * mtk_gpueb_mbox_read_rx - read RX buffer from MMIO into ebm's RX buffer
> > > + * @ebm: pointer to &struct mtk_gpueb_mbox instance
> > > + * @channel: number of channel to read
> > > + */
> > > +static void mtk_gpueb_mbox_read_rx(struct mtk_gpueb_mbox *ebm,
> > > + unsigned int channel)
> > > +{
> > > + const struct mtk_gpueb_mbox_ch *ch;
> > > +
> > > + ch = &ebm->v->channels[channel];
> > > +
> > > + memcpy_fromio(ebm->rx_buf[channel], ebm->mbox_mmio + ch->rx_offset,
> > > + ch->rx_len);
> > > +
> > > +}
> > > +
> > > +static irqreturn_t mtk_gpueb_mbox_isr(int irq, void *data)
> > > +{
> > > + struct mtk_gpueb_mbox *ebm = data;
> > > + u32 rx_handled = 0;
> > > + u32 rx_sts;
> > > + int i;
> > > +
> > > + rx_sts = readl(ebm->mbox_ctl + MBOX_CTL_RX_STS);
> > > +
> > > + for (i = 0; i < ebm->v->num_channels; i++) {
> > > + if (rx_sts & BIT(i)) {
> > > + if (!atomic_cmpxchg(&ebm->rx_status[i], 0,
> > > + MBOX_FULL | MBOX_CLOGGED))
> > > + rx_handled |= BIT(i);
> > > + }
> > > + }
> > We can loop over bits that are set in rx_sts, if we expect that only a
> > few bits are set most of the time.
>
> Could you elaborate on your preferred approach? num_channels will be
> smaller than the bit width of rx_sts. I could loop from __fls down to
> 0 checking for (!rx_sts ^ rx_handled) for an early exit, but I'm not
> sure if this microoptimisation is what you meant or if that even makes
> much sense instruction cost wise.
>
> Alternatively, I could loop from __fss to num_channels.
>
> Is there some commonly used hyper-optimised "loop over set bits" macro
> that I'm unfamiliar with?
Not sure if there is a macro, but panthor open-codes something like

while (rx_sts) {
int chan = ffs(rx_sts) - 1;
rx_sts &= ~BIT(chan);
// do away with chan
}

>
> > > +
> > > + writel(rx_handled, ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > > +
> > > + if (!(rx_sts ^ rx_handled))
> > "rx_sts == rx_handled" should be more direct.
>
> Good point, will change.
>
> > > + return IRQ_WAKE_THREAD;
> > > +
> > > + dev_warn_ratelimited(ebm->dev, "spurious interrupts on 0x%04X\n",
> > > + rx_sts ^ rx_handled);
> > > + return IRQ_NONE;
> > It seems a bit too punishing when there are spurious interrupts. I
> > wonder if we should warn, but return IRQ_WAKE_THREAD as long as
> > rx_handled != 0.
> >
> > Also, if another interrupt can fire before mtk_gpueb_mbox_thread runs,
> > that's data dropping rather than spurious interrupts.
> >
>
> Yeah, I agree that this is bad. As wens pointed out, my IRQ clearing
> earlier would most certainly open us up to losing data. I'll
> definitely look into changing this; iirc the current code resulted
> from me being unsure whether partially handled IRQs should still
> be counted as a handled IRQ or not.
If we only get rx data (response) as a result of tx data (request),
this would be fine. But if there can be unsolicited tx data (looking
at fast_dvfs_event), yeah, we are clearing irq too early.

>
> > > +}
> > > +
> > > +static irqreturn_t mtk_gpueb_mbox_thread(int irq, void *data)
> > > +{
> > > + struct mtk_gpueb_mbox *ebm = data;
> > > + irqreturn_t ret = IRQ_NONE;
> > > + int status;
> > > + int i;
> > > +
> > > + for (i = 0; i < ebm->v->num_channels; i++) {
> > > + status = atomic_cmpxchg(&ebm->rx_status[i],
> > > + MBOX_FULL | MBOX_CLOGGED, MBOX_FULL);
> > > + if (status == (MBOX_FULL | MBOX_CLOGGED)) {
> > We could also save rx_handled from mtk_gpueb_mbox_isr and loop over
> > bits that are set. If we do that, ebm->rx_status[i] is guaranteed to
> > be MBOX_FULL | MBOX_CLOGGED.
> >
>
> Wouldn't storing rx_handled open us up to data races with unpleasant
> code to try and prevent them? As far as I understand, the ISR is allowed
> to fire again before the thread has completed. Having the rx_status bits
> as individual atomics gives us finer granularity than needing to protect
> the entire set of bits.
>
> If there is some way to pass data from the ISR to the awoken thread
> directly as a per-invocation parameter rather than a squirreled away
> member of the driver private struct, then I agree doing it your way
> seems better.
>
> > > + mtk_gpueb_mbox_read_rx(ebm, i);
> > > + mbox_chan_received_data(&ebm->mbox.chans[i],
> > > + ebm->rx_buf[i]);
> > It looks like we read the data and pass it on to the client
> > immediately. Does each channel need its own rx_buf?
> >
>
> Yeah, common mailbox framework does not do a copy here, it just passes
> the pointer. Which is good, because nobody likes copies, and it
> prevents the mailbox core from needing to know the size of received
> data.
>
> So the individual rx_bufs are needed to prevent channel cross-talk.
I checked a few other mailbox drivers. The data pointer could even
point to local variables in some cases. It seems, in general,
mbox_client cannot expect the data to be valid after
mbox_chan_received_data returns.

>
> > > + /* FIXME: When does MBOX_FULL get cleared? Here? */
> > > + atomic_set(&ebm->rx_status[i], 0);
> > > + ret = IRQ_HANDLED;
> > > + }
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int mtk_gpueb_mbox_send_data(struct mbox_chan *chan, void *data)
> > > +{
> > > + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(chan->mbox->dev);
> > > + unsigned int *num = chan->con_priv;
> > > + int i;
> > > + u32 *values = data;
> > > +
> > > + if (*num >= ebm->v->num_channels)
> > > + return -ECHRNG;
> > Can this ever happen? (I am not familiar with the mbox subsystem)
>
> Only if someone with a pointer to chan modifies con_priv outside of
> what the mailbox driver set. It's an unlikely scenario, so I do think
> I can remove this check, but the downside is that if someone ever does
> manage to do this then the next line is a convenient read primitive.
That should be an invalid usage that we hopefully don't need to worry
about. Otherwise, as things are, *num can also be modified after the
check so the check is not sufficient.

>
> > > +
> > > + if (!ebm->v->channels[*num].no_response &&
> > > + atomic_read(&ebm->rx_status[*num]))
> > > + return -EBUSY;
> > When no_response is true, rx_status is 0. We probably don't need to
> > check no_response.
>
> Now that I'm thinking about it, I can probably get rid of no_response
> altogether. It was originally added because I conflated txdone with
> response received, and one particular command on the sleep channel,
> namely powering off the MFG, is unable to ever cause a response
> interrupt to fire.
>
> Since we now poll for txdone anyway using the TX status register, I
> don't think this is needed at all anymore, so I'll try and get
> rid of it.
>
> > > +
> > > + writel(BIT(*num), ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > > +
> > > + /*
> > > + * We don't want any fancy nonsense, just write the 32-bit values in
> > > + * order. memcpy_toio/__iowrite32_copy don't work here, because fancy.
> > > + */
> > > + for (i = 0; i < ebm->v->channels[*num].tx_len; i += 4) {
> > > + writel(values[i / 4],
> > > + ebm->mbox_mmio + ebm->v->channels[*num].tx_offset + i);
> > > + }
> > > +
> > > + writel(BIT(*num), ebm->mbox_ctl + MBOX_CTL_IRQ_SET);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_gpueb_mbox_startup(struct mbox_chan *chan)
> > > +{
> > > + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(chan->mbox->dev);
> > > + unsigned int *num = chan->con_priv;
> > > +
> > > + if (*num >= ebm->v->num_channels)
> > > + return -ECHRNG;
> > > +
> > > + atomic_set(&ebm->rx_status[*num], 0);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void mtk_gpueb_mbox_shutdown(struct mbox_chan *chan)
> > > +{
> > > + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(chan->mbox->dev);
> > > + unsigned int *num = chan->con_priv;
> > > +
> > > + atomic_set(&ebm->rx_status[*num], MBOX_CLOGGED);
> > > +}
> > > +
> > > +static bool mtk_gpueb_mbox_last_tx_done(struct mbox_chan *chan)
> > > +{
> > > + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(chan->mbox->dev);
> > > + unsigned int *num = chan->con_priv;
> > > +
> > > + return !(readl(ebm->mbox_ctl + MBOX_CTL_TX_STS) & BIT(*num));
> > > +}
> > > +
> > > +const struct mbox_chan_ops mtk_gpueb_mbox_ops = {
> > > + .send_data = mtk_gpueb_mbox_send_data,
> > > + .startup = mtk_gpueb_mbox_startup,
> > > + .shutdown = mtk_gpueb_mbox_shutdown,
> > > + .last_tx_done = mtk_gpueb_mbox_last_tx_done,
> > > +};
> > > +
> > > +static struct mbox_chan *
> > > +mtk_gpueb_mbox_of_xlate(struct mbox_controller *mbox,
> > > + const struct of_phandle_args *sp)
> > > +{
> > > + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(mbox->dev);
> > > +
> > > + if (!sp->args_count)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + if (sp->args[0] >= ebm->v->num_channels)
> > > + return ERR_PTR(-ECHRNG);
> > > +
> > > + return &mbox->chans[sp->args[0]];
> > > +}
> > > +
> > > +static int mtk_gpueb_mbox_probe(struct platform_device *pdev)
> > > +{
> > > + struct mtk_gpueb_mbox *ebm;
> > > + unsigned int rx_buf_sz;
> > > + void *buf;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + ebm = devm_kzalloc(&pdev->dev, sizeof(*ebm), GFP_KERNEL);
> > > + if (!ebm)
> > > + return -ENOMEM;
> > > +
> > > + ebm->dev = &pdev->dev;
> > > + ebm->v = of_device_get_match_data(ebm->dev);
> > > +
> > > + dev_set_drvdata(ebm->dev, ebm);
> > > +
> > > + ebm->clk = devm_clk_get_enabled(ebm->dev, NULL);
> > > + if (IS_ERR(ebm->clk))
> > > + return dev_err_probe(ebm->dev, PTR_ERR(ebm->clk),
> > > + "Failed to get 'eb' clock\n");
> > > +
> > > + ebm->mbox_mmio = devm_platform_ioremap_resource_byname(pdev, "mbox");
> > > + if (IS_ERR(ebm->mbox_mmio))
> > > + return dev_err_probe(ebm->dev, PTR_ERR(ebm->mbox_mmio),
> > > + "Couldn't map mailbox registers\n");
> > > +
> > > + ebm->mbox_ctl = devm_platform_ioremap_resource_byname(pdev, "mbox_ctl");
> > > + if (IS_ERR(ebm->mbox_ctl))
> > > + return dev_err_probe(
> > > + ebm->dev, PTR_ERR(ebm->mbox_ctl),
> > > + "Couldn't map mailbox control registers\n");
> > > +
> > > + rx_buf_sz = (ebm->v->channels[ebm->v->num_channels - 1].rx_offset +
> > > + ebm->v->channels[ebm->v->num_channels - 1].rx_len);
> > rx is after tx in mmio. The first half of the space is wasted.
> >
> > We follow mtk_gpueb_mbox_read_rx by mbox_chan_received_data. It seems
> > we only need max of rx_len's.
>
> We do need the sum of all rx_len's but you're correct that the current code
> allocates too much memory. What I think it should do is
>
> last rx_offset + last rx_len - first rx_offset
>
> to get the total amount of MMIO occupied by RX.
>
> > > +
> > > + buf = devm_kzalloc(ebm->dev, rx_buf_sz, GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > +
> > > + ebm->rx_buf = devm_kmalloc_array(ebm->dev, ebm->v->num_channels,
> > > + sizeof(*ebm->rx_buf), GFP_KERNEL);
> > > + if (!ebm->rx_buf)
> > > + return -ENOMEM;
> > > +
> > > + ebm->mbox.chans = devm_kcalloc(ebm->dev, ebm->v->num_channels,
> > > + sizeof(struct mbox_chan), GFP_KERNEL);
> > > + if (!ebm->mbox.chans)
> > > + return -ENOMEM;
> > > +
> > > + ebm->rx_status = devm_kcalloc(ebm->dev, ebm->v->num_channels,
> > > + sizeof(atomic_t), GFP_KERNEL);
> > > + if (!ebm->rx_status)
> > > + return -ENOMEM;
> > > +
> > > + ebm->chn = devm_kcalloc(ebm->dev, ebm->v->num_channels,
> > > + sizeof(*ebm->chn), GFP_KERNEL);
> > > +
> > > + for (i = 0; i < ebm->v->num_channels; i++) {
> > > + ebm->rx_buf[i] = buf + ebm->v->channels[i].rx_offset;
> > > + spin_lock_init(&ebm->mbox.chans[i].lock);
> > > + /* the things you do to avoid explicit casting void* */
> > I actually prefer an inline helper that casts chan->con_priv to the
> > channel number. Another option is "chan - ebm->mox.chans".
>
> Hmmm, yeah. Though, I don't enjoy chan - ebm->mbox.chans because now we're
> in the woods regarding pointer arithmetics.
>
> I think the inline helper makes more sense, even if explicitly using a
> pointer as an integer gives me the heebie-jeebies.
>
> > > + ebm->chn[i] = i;
> > > + ebm->mbox.chans[i].con_priv = &ebm->chn[i];
> > > + atomic_set(&ebm->rx_status[i], MBOX_CLOGGED);
> > > + }
> > > +
> > > + ebm->mbox.dev = ebm->dev;
> > > + ebm->mbox.num_chans = ebm->v->num_channels;
> > > + ebm->mbox.txdone_poll = true;
> > > + ebm->mbox.txpoll_period = 0; /* minimum hrtimer interval */
> > > + ebm->mbox.of_xlate = mtk_gpueb_mbox_of_xlate;
> > > + ebm->mbox.ops = &mtk_gpueb_mbox_ops;
> > > +
> > > + ebm->irq = platform_get_irq(pdev, 0);
> > > + if (ebm->irq < 0)
> > > + return ebm->irq;
> > > +
> > > + ret = devm_request_threaded_irq(ebm->dev, ebm->irq, mtk_gpueb_mbox_isr,
> > > + mtk_gpueb_mbox_thread, 0, NULL, ebm);
> > > + if (ret)
> > > + return dev_err_probe(ebm->dev, ret, "failed to request IRQ\n");
> > > +
> > > + ret = devm_mbox_controller_register(ebm->dev, &ebm->mbox);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct mtk_gpueb_mbox_variant mtk_gpueb_mbox_mt8196 = {
> > > + .num_channels = 12,
> > > + .channels = {
> > > + { "fast_dvfs_event", 0, 0x0000, 16, 0x00e0, 16, false },
> > > + { "gpufreq", 1, 0x0010, 32, 0x00f0, 32, false },
> > > + { "sleep", 2, 0x0030, 12, 0x0110, 4, true },
> > > + { "timer", 3, 0x003c, 24, 0x0114, 4, false },
> > > + { "fhctl", 4, 0x0054, 36, 0x0118, 4, false },
> > > + { "ccf", 5, 0x0078, 16, 0x011c, 16, false },
> > > + { "gpumpu", 6, 0x0088, 24, 0x012c, 4, false },
> > > + { "fast_dvfs", 7, 0x00a0, 24, 0x0130, 24, false },
> > > + { "ipir_c_met", 8, 0x00b8, 4, 0x0148, 16, false },
> > > + { "ipis_c_met", 9, 0x00bc, 16, 0x0158, 4, false },
> > > + { "brisket", 10, 0x00cc, 16, 0x015c, 16, false },
> > > + { "ppb", 11, 0x00dc, 4, 0x016c, 4, false },
> > > + },
> > > +};
> > > +
> > > +static const struct of_device_id mtk_gpueb_mbox_of_ids[] = {
> > > + { .compatible = "mediatek,mt8196-gpueb-mbox",
> > > + .data = &mtk_gpueb_mbox_mt8196 },
> > > + { /* Sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_gpueb_mbox_of_ids);
> > > +
> > > +static struct platform_driver mtk_gpueb_mbox_drv = {
> > > + .probe = mtk_gpueb_mbox_probe,
> > > + .driver = {
> > > + .name = "mtk-gpueb-mbox",
> > > + .of_match_table = mtk_gpueb_mbox_of_ids,
> > > + }
> > > +};
> > > +module_platform_driver(mtk_gpueb_mbox_drv);
> > > +
> > > +MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
> > > +MODULE_DESCRIPTION("MediaTek GPUEB mailbox driver for SoCs such as the MT8196");
> > > +MODULE_LICENSE("GPL");
> > >
> > > --
> > > 2.51.0
> > >
> >
>
> Thank you for your review!
>
> Kind regards,
> Nicolas Frattaroli
>
>