Re: [PATCH 2/3] mailbox: renesas: Support MFIS mailbox driver

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 5904 bytes --]

Hi Morimoto-san,

in addition to what the others already mentioned...

> +	help
> +	  This driver provides support for SCMI interface transport with
> +	  MFIS(Multifunctional Interface).
> +	  It is used to send short message between CPU cores and
> +	  SCP(System Control Processor).

Spaces after MFIS and SCP. But I would reword it because it can send
messages to the RT domain, too. Suggestion:

+	  This driver provides support for mailboxes of the MFIS
+	  (Multifunctional Interface) via the SCMI interface.
+	  It is used to send short message between different domains
+	  like AP, RT, and SCP.


> +
>  config RISCV_SBI_MPXY_MBOX
>  	tristate "RISC-V SBI Message Proxy (MPXY) Mailbox"
>  	depends on RISCV_SBI
> @@ -389,5 +398,4 @@ config RISCV_SBI_MPXY_MBOX
>  	  remote processor through the SBI implementation (M-mode firmware
>  	  or HS-mode hypervisor). Say Y here if you want to have this support.
>  	  If unsure say N.
> -
>  endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 81820a4f55285..060a9c7f6727b 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
>  
>  obj-$(CONFIG_OMAP2PLUS_MBOX)	+= omap-mailbox.o
>  
> +obj-$(CONFIG_RCAR_MFIS_MBOX)	+= rcar-mfis-mailbox.o
> +
>  obj-$(CONFIG_ROCKCHIP_MBOX)	+= rockchip-mailbox.o
>  
>  obj-$(CONFIG_PCC)		+= pcc.o
> diff --git a/drivers/mailbox/rcar-mfis-mailbox.c b/drivers/mailbox/rcar-mfis-mailbox.c
> new file mode 100644
> index 0000000000000..54cabfa305eb4
> --- /dev/null
> +++ b/drivers/mailbox/rcar-mfis-mailbox.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas MFIS (Multifunctional Interface) Mailbox Driver
> + *
> + * Copyright (c) 2025, Renesas Electronics Corporation. All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +static int mfis_send_data(struct mbox_chan *link, void *data)
> +{
> +	void __iomem *reg = link->con_priv;
> +
> +	/*Trigger interrupt request to firmware(SCP)*/

Spaces before and after "*"

> +	iowrite32(0x1, reg);

#define instead of hardcoded value?

But I guess the real question here is: do we know already that the SCP
firmware will just react on the interrupt? We really do not need the 15
bits of data that goes with this register? Or the message register?

> +
> +	return 0;
> +}
> +
> +static irqreturn_t mfis_rx_interrupt(int irq, void *data)
> +{
> +	struct mbox_chan *link = data;
> +	void __iomem *reg = link->con_priv;
> +
> +	mbox_chan_received_data(link, 0);
> +
> +	/* Clear interrupt register */

I would drop this comment, but maybe others won't.

> +	iowrite32(0x0, reg);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mfis_startup(struct mbox_chan *link)
> +{
> +	struct mbox_controller *mbox = link->mbox;
> +	struct device *dev = mbox->dev;
> +	int irq;
> +	int ret;
> +
> +	irq = of_irq_get(dev->of_node, 0);
> +
> +	ret = request_irq(irq, mfis_rx_interrupt,
> +			  IRQF_SHARED, "mfis-mbox", link);
> +	if (ret) {
> +		dev_err(dev,
> +			"Unable to acquire IRQ %d\n", irq);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static void mfis_shutdown(struct mbox_chan *link)
> +{
> +	struct mbox_controller *mbox = link->mbox;
> +	struct device *dev = mbox->dev;
> +	int irq;
> +
> +	irq = of_irq_get(dev->of_node, 0);
> +
> +	free_irq(irq, link);

Is it really necessary to free the irq here? We could simply disable it?

> +}
> +
> +static bool mfis_last_tx_done(struct mbox_chan *link)
> +{
> +	return true;
> +}
> +
> +static const struct mbox_chan_ops mfis_chan_ops = {
> +	.send_data	= mfis_send_data,
> +	.startup	= mfis_startup,
> +	.shutdown	= mfis_shutdown,
> +	.last_tx_done	= mfis_last_tx_done
> +};
> +
> +static int mfis_mbox_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mbox_controller *mbox;
> +	void __iomem *reg;
> +	int ret, count = 2, i;

Why is count = 2? Doesn't X5H support 64 of them?

> +
> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> +	if (!mbox)
> +		return -ENOMEM;
> +
> +	mbox->chans = devm_kcalloc(dev, count, sizeof(*mbox->chans), GFP_KERNEL);
> +	if (!mbox->chans)
> +		return -ENOMEM;
> +
> +	reg = devm_platform_ioremap_resource(pdev, i);
> +	if (IS_ERR(reg))
> +		return PTR_ERR(reg);
> +
> +	for (i = 0; i < count; i++) {
> +		mbox->chans[i].mbox	= mbox;
> +		mbox->chans[i].con_priv	= reg + ((1 - i) * 4);

Why backwards ("1 - i")?

> +	}
> +
> +	mbox->txdone_poll	= true;
> +	mbox->txdone_irq	= false;
> +	mbox->txpoll_period	= 1;
> +	mbox->num_chans		= count;
> +	mbox->ops		= &mfis_chan_ops;
> +	mbox->dev		= dev;

Only one space before '='.

> +
> +	ret = mbox_controller_register(mbox);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, mbox);
> +	dev_info(dev, "MFIS mailbox is probed\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mfis_mbox_of_match[] = {
> +	{ .compatible = "renesas,mfis-mbox", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mfis_mbox_of_match);
> +
> +static struct platform_driver mfis_mbox_driver = {
> +	.driver = {
> +		.name = "renesas-mfis-mbox",
> +		.of_match_table = mfis_mbox_of_match,
> +	},
> +	.probe	= mfis_mbox_probe,
> +};
> +module_platform_driver(mfis_mbox_driver);
> +MODULE_DESCRIPTION("Renesas MFIS mailbox driver");
> +MODULE_LICENSE("GPL v2");

"GPL" only.

But I guess we need a bit more information about the bigger picture of
this driver. Like, with which firmware was this used?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]