Re: [PATCH 2/3] soc: renesas: Add Renesas R-Car MFIS driver

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, 17 Mar 2026 at 14:06, Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> Renesas R-Car MFIS offers multiple features but most importantly
> mailboxes and hwspinlocks. Because they share a common register space
> and a common register unprotection mechanism, a single driver was chosen
> to handle all dependencies. (MFD and auxiliary bus have been tried as
> well, but they failed because of circular dependencies.)
>
> In this first step, the driver implements common register access and a
> mailbox controller. hwspinlock support will be added incrementally, once
> the subsystem allows out-of-directory drivers.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/soc/renesas/rcar-mfis.c

> +struct mfis_info {
> +       u32 unprotect_mask;
> +       unsigned int mb_num_channels;
> +       unsigned int mb_reg_comes_from_dt:1;
> +       unsigned int mb_tx_uses_eicr:1;
> +       unsigned int mb_channels_are_unidir:1;
> +};
> +
> +struct mfis_per_chan_priv {

I would drop the "per_".

> +       u32 reg;
> +       int irq;
> +};
> +
> +struct mfis_priv {
> +       struct device *dev;
> +       struct mfis_reg common_reg;
> +       struct mfis_reg mbox_reg;
> +       const struct mfis_info *info;
> +
> +       /* mailbox private data */
> +       struct mbox_controller mbox;
> +       struct mfis_per_chan_priv *per_chan_privs;

Likewise.
This could be a flexible array, if it weren't for the hwspinlock array
you will have to add later, right?

> +};
> +
> +static u32 mfis_read(struct mfis_reg *mreg, unsigned int reg)
> +{
> +       return ioread32(mreg->base + reg);
> +}
> +
> +static void mfis_write(struct mfis_reg *mreg, u32 reg, u32 val)

Both writel() and iowrite32() use the inverse order of "reg" and "val".
But I can understand you want to keep "mreg" and "reg" together.

> +{
> +       struct mfis_priv *priv = mreg->priv;
> +       u32 unprotect_mask = priv->info->unprotect_mask;
> +       u32 unprotect_code;
> +
> +       /*
> +        * [Gen4] key: 0xACCE0000, mask: 0x0000FFFF
> +        * [Gen5] key: 0xACC00000, mask: 0x000FFFFF
> +        */
> +       unprotect_code = (MFIS_UNPROTECT_KEY & ~unprotect_mask) |
> +                        ((mreg->start | reg) & unprotect_mask);
> +
> +       iowrite32(unprotect_code, priv->common_reg.base + MFISWACNTR);
> +       iowrite32(val, mreg->base + reg);
> +}
> +
> +/****************************************************
> + *                     Mailbox

Missing closing asterisk ;-)

> + ****************************************************/
> +
> +#define mfis_mb_mbox_to_priv(_m) container_of((_m), struct mfis_priv, mbox)

Both "mb" and "mbox", so perhaps mfis_mbox_to_priv()?
And perhaps s/mb_/mbox_/ everywhere?

> +static int mfis_mb_startup(struct mbox_chan *chan)
> +{
> +       struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> +       int ret = 0;
> +
> +       if (per_chan_priv->irq)
> +               ret = request_irq(per_chan_priv->irq, mfis_mb_iicr_interrupt, 0,
> +                                 dev_name(chan->mbox->dev), chan);
> +
> +       return ret;

You can reduce indentation, and get rid of ret, by inverting the
conditional.

> +}
> +
> +static void mfis_mb_shutdown(struct mbox_chan *chan)
> +{
> +       struct mfis_per_chan_priv *per_chan_priv = chan->con_priv;
> +
> +       free_irq(per_chan_priv->irq, chan);

if (per_chan_priv->irq) ...

free_irq() seems to support zero, but irq_to_desc() still has to
traverse the mtree.

> +}

> +static struct mbox_chan *mfis_mb_of_xlate(struct mbox_controller *mbox,
> +                                         const struct of_phandle_args *sp)
> +{
> +       struct mfis_priv *priv = mfis_mb_mbox_to_priv(mbox);
> +       struct mfis_per_chan_priv *per_chan_priv;
> +       struct mbox_chan *chan;
> +       u32 chan_num, chan_flags;
> +       bool tx_uses_eicr, is_only_rx;
> +
> +       if (sp->args_count != 2)
> +               return ERR_PTR(-EINVAL);
> +
> +       chan_num = sp->args[0];

"chan_num" is the hardware channel number, and should be validated
against mpriv->info->mb_num_channels.

> +       chan_flags = sp->args[1];
> +
> +       /* Channel layout is described in mfis_mb_probe() */

Given you already use "chan += ..." below, perhaps:

    chan = mbox->chans + chan_num;

> +       if (priv->info->mb_channels_are_unidir) {
> +               is_only_rx = chan_flags & MFIS_CHANNEL_RX;
> +               chan = mbox->chans + 2 * chan_num + is_only_rx;

...and:

    chan += chan_num + is_only_rx;

> +       } else {
> +               is_only_rx = false;
> +               chan = mbox->chans + chan_num;

... and drop this line?
With a proper preinitalization of is_only_rx, the whole "else" branch
can be dropped.

> +       }
> +
> +       if (priv->info->mb_reg_comes_from_dt) {
> +               tx_uses_eicr = chan_flags & MFIS_CHANNEL_EICR;
> +               if (tx_uses_eicr)
> +                       chan += mbox->num_chans / 2;
> +       } else {
> +               tx_uses_eicr = priv->info->mb_tx_uses_eicr;
> +       }

"chan - mbox->chans" is the logical channel number, and should be
validated against mbox_num_chans, to avoid out-of-bound accesses.

> +
> +       per_chan_priv = chan->con_priv;
> +       per_chan_priv->reg = chan_num * 0x1000 + (tx_uses_eicr ^ is_only_rx) * 4;

I think it would be good to document this register is either an IICR
or EICR register offset, through:
  1. A comment, or
  2. Definitions and code, e.g

         #define MFISIICR 0x00
         #define MFISEICR 0x04

         if (tx_uses_eicr ^ is_only_rx)
             per_chan_priv->reg = chan_num * 0x1000 + MFISEICR;
         else
             per_chan_priv->reg = chan_num * 0x1000 + MFISIICR;

     Or:

         #define MFISIICR(i) ((i) * 0x1000 + 0x00)
         #define MFISEICR(i) ((i) * 0x1000 + 0x04)

         per_chan_priv->reg = (tx_uses_eicr ^ is_only_rx) ? MFISEICR(chan_num)
                                                          : MFISIICR(chan_num);

> +
> +       if (!priv->info->mb_channels_are_unidir || is_only_rx) {
> +               char irqname[8];
> +               char suffix = tx_uses_eicr ? 'i' : 'e';
> +
> +               /* "ch0i" or "ch0e" */
> +               scnprintf(irqname, sizeof(irqname), "ch%d%c", chan_num, suffix);

"%u" for unsigned chan_num.

> +
> +               per_chan_priv->irq = of_irq_get_byname(mbox->dev->of_node, irqname);
> +               if (per_chan_priv->irq < 0)
> +                       return ERR_PTR(per_chan_priv->irq);
> +               else if (per_chan_priv->irq == 0)

No need for "else" after "return".

> +                       return ERR_PTR(-ENOENT);
> +       }
> +
> +       return chan;
> +}
> +
> +static int mfis_mb_probe(struct mfis_priv *priv)
> +{
> +       struct device *dev = priv->dev;
> +       struct mbox_chan *chan;
> +       struct mbox_controller *mbox;
> +       unsigned int i, num_chan = priv->info->mb_num_channels;

"i" is only used in the for-loop below, so you could declare it in the
for-statement. As a bonus, that would avoid mixing the declaration of
uninitialized and initialized variables.

> +
> +       if (priv->info->mb_channels_are_unidir)
> +               /* Channel layout: Ch0-TX, Ch0-RX, Ch1-TX... */
> +               num_chan *= 2;
> +
> +       if (priv->info->mb_reg_comes_from_dt)
> +               /* Channel layout: <n> IICR channels, <n> EICR channels */
> +               num_chan *= 2;

Curly braces around multi-line if-branches (even if they are comments)?

> +
> +       chan  = devm_kcalloc(dev, num_chan, sizeof(*chan), GFP_KERNEL);
> +       if (!chan)
> +               return -ENOMEM;
> +
> +       priv->per_chan_privs = devm_kcalloc(dev, num_chan, sizeof(*priv->per_chan_privs),
> +                                           GFP_KERNEL);
> +       if (!priv->per_chan_privs)
> +               return -ENOMEM;
> +
> +       mbox = &priv->mbox;
> +
> +       for (i = 0; i < num_chan; i++)
> +               chan[i].con_priv = &priv->per_chan_privs[i];
> +
> +       mbox->chans = chan;
> +       mbox->num_chans = num_chan;
> +       mbox->txdone_poll = true;
> +       mbox->ops = &mfis_iicr_ops;
> +       mbox->dev = dev;
> +       mbox->of_xlate = mfis_mb_of_xlate;
> +
> +       return mbox_controller_register(mbox);
> +}
> +
> +/****************************************************
> + *             Common

Missing closing asterisk.

> + ****************************************************/
>
> +static int mfis_reg_probe(struct platform_device *pdev, struct mfis_priv *priv,
> +                         struct mfis_reg *mreg, const char *name, bool required)
> +{
> +       struct resource *res;
> +       void __iomem *base;
> +
> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> +
> +       /* If there is no mailbox resource, registers are in the common space */
> +       if (!res && !required) {

Given you only test for the negated "!required", perhaps invert the
logic, and replace "required" by "optional"?

> +               priv->mbox_reg = priv->common_reg;

This left me looking for an assignment to priv->mbox_reg in the "else"
branch ;-) Then I realized "&priv->mbox_reg" is passed as the "mreg"
parameter.  Hence perhaps:

    *mreg = priv->common_reg;

?

> +       } else {
> +               base = devm_ioremap_resource(&pdev->dev, res);
> +               if (IS_ERR(base))
> +                       return PTR_ERR(base);
> +
> +               mreg->base = base;
> +               mreg->start = res->start;
> +               mreg->priv = priv;
> +       }
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds