[PATCH 7/7] mailbox: goog-mba: Introduce the goog-mba mailbox driver

[Doug Anderson]

Hi,

On Mon, Sep 21, 2026 at 5:11 PM Jassi Brar <jassisinghbrar at gmail.com> wrote:
>
> > > If you do have some real numbers and think it is worth it on your
> > > platform, then maybe expose each doorbell/shm-slot as a generic
> > > channel. clk_mailbox will request a generic channel, do the request
> > > and free it. The same effect but without inventing a new api. I can
> > > share a draft if you want, but I suggest let's not make things
> > > complicated without proven benefit.
> >
> > FWIW, the downstream code in Pixel does what I think you're
> > suggesting. I can confidently say that, while it doesn't require
> > changes to the mailbox core, it is much more convoluted and
> > complicated. It also bleeds into the device-tree representation, which
> > doesn't feel great.
> >
> > Just to be concrete, I'll document how the downstream driver works. If
> > this isn't what you were thinking, please correct me.
> >
> > Back to our simplified "clk_mailbox" driver. We'll say that our
> > "clk_mailbox" driver talks over a single mailbox to the remote
> > processor. Let's say each message is 4 words big. The message space is
> > 32-words big. 32 / 4 = 8 which means this space is divided into 8
> > queue slots. Downstream represents each of these queue slots as a
> > generic channel. That means that, in the device tree, our
> > "clk_mailbox" driver looks looks like this:
> >
> > mboxes = <&cpm_tx_mba 0>,
> >          <&cpm_tx_mba 1>,
> >          <&cpm_tx_mba 2>,
> >          <&cpm_tx_mba 3>,
> >          <&cpm_tx_mba 4>,
> >          <&cpm_tx_mba 5>,
> >          <&cpm_tx_mba 6>,
> >          <&cpm_tx_mba 7>;
> >
> > Then the "clk_mailbox" driver is in charge of rotating through each of
> > the channels. First it writes to channel 0, then it writes to channel
> > 1, etc. This works with no changes to the core, but...
> >
> I was thinking something like   mboxes = <&cpm_tx_mba>
> clk_mailbox simply asks for some channel, gets allocated the next free
> slot. The mailbox controller driver keeps track of the channel-slot
> map locally and is responsible for managing contiguous slots and
> completing the tx upon receiving ack.

Hmmm. OK, so I guess you're saying that every time "clk_mailbox" wants
to send a message, it calls mbox_request_channel(). It always requests
the same channel over and over again, but underneath the mailbox's
fw_xlate() function returns the next distinct channel? Then once
"clk_mailbox" sees the tx_done then it calls mbox_free_channel()? I
guess it would need to fork the mbox_free_channel() into a delayed
work function since mbox_free_channel() requires grabbing a mutex and
tx_done() is called from atomic context. It would be up to
"clk_mailbox" to ensure that it used the interface properly: always
send the message on the next allocated mailbox, always free the
channels in order, etc. Also, "clk_mailbox" would still need to
implement its own queue to handle the case where there were no more
free channels.

One thing that is important is that "clk_mailbox" needs to know when
"tx_done". If we didn't need that, everything would be vastly simpler:
just allocate the channel, send the message, and free it right away.

Did I understand your suggestion correctly this time, or am I still
not getting it?

Assuming I understood correctly, my thoughts would be:
1. At least this doesn't bleed into the device tree, which is great.
2. I'm a little worried about all the overhead involved in constantly
requesting / freeing channels. Maybe it's not as bad as I fear, but
those functions don't seem intended for constant calls. Even if the
overhead isn't that bad, the extra overhead needed to fork the free
call to delayed work doesn't seem great.
3. It feels like trying to manage this from "clk_mailbox" is going to
be a bunch of complicated code, including managing our own queue since
we still can't use the mailbox core's queue.

Overall, it feels like an bunch of awkward code. It feels like a hack
that a downstream kernel module would do simply because they didn't
want to improve the mailbox core...


> > I'm also a little confused about the resistance. I don't feel like the
> > mailbox core change is that complicated. The diffstat shows 58
> > insertions and 17 deletions. 14 of those added lines are comments.
> > While we certainly don't want to add useless APIs, to me this truly
> > seems like the correct way to add the functionality. It also doesn't
> > seem absurd to me that some future mailbox controller out there will
> > also support queuing like this.
> >
> It is not the diff stat but about inserting a flag in the api to
> introduce special case behavior. It is like adding one person to the
> party introduces N-1 handshakes -

Sure, it's new core code for a single client. ...but some client has
to be the first. The idea of a mailbox controller being able to queue
data doesn't feel like an absurd feature that nobody would ever need
again. Heck, once the core supports the feature it seems like someone
out there will figure out how to make their existing hardware work in
"queue-mode" and improve its performance.


> the has_queue flag doesn't play well
> with other configurations and may allow future platforms to abuse
> has_queue to implement hacks.

I don't really understand this part. Can you give any examples? How
does "has_queue" not play well with other configurations? You mean the
fact that my code right now only works with "MBOX_TXDONE_BY_IRQ"? I
don't think relaxing that would be very hard. ...and sure, people will
implement hacks no matter what API you give them (the "allocate a new
channel for each message" might qualify as one such hack?), but it
doesn't feel like "has_queue" is especially prone to abuse, is it?

-Doug