Re: [PATCH v9 5/9] mailbox: add CIX mailbox driver

["Arnd Bergmann" <arnd@arndb.de>]

On Mon, Jun 9, 2025, at 05:16, Peter Chen wrote:
> From: Guomin Chen <Guomin.Chen@cixtech.com>
>
> The CIX mailbox controller, used in the Cix SoCs, like sky1.
> facilitates message transmission between multiple processors
> within the SoC, such as the AP, PM, audio DSP, SensorHub MCU,
> and others.
>
> Reviewed-by: Peter Chen <peter.chen@cixtech.com>
> Signed-off-by: Guomin Chen <Guomin.Chen@cixtech.com>
> Signed-off-by: Gary Yang <gary.yang@cixtech.com>
> Signed-off-by: Lihua Liu <Lihua.Liu@cixtech.com>
> Signed-off-by: Peter Chen <peter.chen@cixtech.com>

This is the only driver holding up the merge of the CIX
platform, so I had a closer look myself. The driver looks
well written overall, and I see a lot of details that have
come up in previous versions are addressed.

The one thing that stuck out to me is the design of
having multiple types of mailbox in one driver, which
feels out of scope for a simple mailbox.

> +static int cix_mbox_send_data_reg(struct mbox_chan *chan, void *data)
> +{
> +       struct cix_mbox_priv *priv = to_cix_mbox_priv(chan->mbox);
> +       union cix_mbox_msg_reg_fifo *msg = data;
> +       u32 len, i;
> +
> +       if (!data)
> +               return -EINVAL;
> +
> +       len = mbox_get_msg_size(data);
> +       for (i = 0; i < len; i++)
> +               cix_mbox_write(priv, msg->buf[i], REG_MSG(i));

In particular, this bit seems to do more than just what I think
of as a simple mailbox that should have fixed-length messages,
it feels more like a generic message passing interface between
device drivers and firmware or some microcontroller.

What is the purpose here?

> +static int cix_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
...
> +       switch (cp->type) {
> +       case CIX_MBOX_TYPE_DB:
> +               cix_mbox_send_data_db(chan, data);
> +               break;
> +       case CIX_MBOX_TYPE_REG:
> +               cix_mbox_send_data_reg(chan, data);
> +               break;
> +       case CIX_MBOX_TYPE_FIFO:
> +               cix_mbox_send_data_fifo(chan, data);
> +               break;
> +       case CIX_MBOX_TYPE_FAST:
> +               cix_mbox_send_data_fast(chan, data);
> +               break;

Similarly, this also exceeds the complexity I would expect
in a simple controller, it feels like there should either
be four separate drivers that implement one type of interface,
or a much higher-level abstraction.

Is there a document that details how the messages are
structured and what the users are? How does a user of
the mailbox know the size of a message to pass down?

     Arnd