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

Geert Uytterhoeven
2026-03-19T12:59:01Z

Hi Wolfram,

On Tue, 17 Mar 2026 at 14:06, Wolfram Sang
<wsa+renesas@sang-engineering.com> 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@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

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@linux-m68k.org

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