[v2,4/4] mailbox: Add support for i.MX7D messaging unit

Commit Message

Oleksij Rempel June 15, 2018, 9:51 a.m. UTC

The Mailbox controller is able to send messages (up to 4 32 bit words)
between the endpoints.

This driver was tested using the mailbox-test driver sending messages
between the Cortex-A7 and the Cortex-M4.

Signed-off-by: Oleksij Rempel <[e-mail]>
---
 drivers/mailbox/Kconfig       |   6 +
 drivers/mailbox/Makefile      |   2 +
 drivers/mailbox/imx-mailbox.c | 288 ++++++++++++++++++++++++++++++++++
 3 files changed, 296 insertions(+)
 create mode 100644 drivers/mailbox/imx-mailbox.c

Comments

Aisheng Dong June 26, 2018, 10:09 a.m. UTC | #1

> -----Original Message-----
> From: Oleksij Rempel [mailto:[e-mail]]
> Sent: Friday, June 15, 2018 5:51 PM
> To: Shawn Guo <[e-mail]>; Fabio Estevam
> <[e-mail]>; Rob Herring <[e-mail]>; Mark
> Rutland <[e-mail]>; A.s. Dong <[e-mail]>
> Cc: Oleksij Rempel <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; dl-linux-imx <[e-mail]>
> Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
> 
> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.
> 
> Signed-off-by: Oleksij Rempel <[e-mail]>
> ---
>  drivers/mailbox/Kconfig       |   6 +
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/imx-mailbox.c | 288
> ++++++++++++++++++++++++++++++++++
>  3 files changed, 296 insertions(+)
>  create mode 100644 drivers/mailbox/imx-mailbox.c
> 
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> a2bb27446dce..e1d2738a2e4c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,12 @@ config ARM_MHU
>  	  The controller has 3 mailbox channels, the last of which can be
>  	  used in Secure mode only.
> 
> +config IMX_MBOX
> +	tristate "iMX Mailbox"
> +	depends on SOC_IMX7D || COMPILE_TEST
> +	help
> +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> +
>  config PLATFORM_MHU
>  	tristate "Platform MHU Mailbox"
>  	depends on OF
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> cc23c3a43fcd..ba2fe1b6dd62 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> 
>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> 
> +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> +
>  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> 
>  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> new file mode 100644 index 000000000000..e3f621cb1d30
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> +<[e-mail]>  */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +/* Transmit Register */
> +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> +/* Receive Register */
> +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> +/* Status Register */
> +#define IMX_MU_xSR		0x20
> +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> +#define IMX_MU_xSR_BRDIP	BIT(9)
> +
> +/* Control Register */
> +#define IMX_MU_xCR		0x24
> +/* Transmit Interrupt Enable */
> +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> +
> +#define IMX_MU_MAX_CHANS	4u
> +
> +struct imx_mu_priv;
> +
> +struct imx_mu_cfg {
> +	unsigned int		chans;
> +	void (*init_hw)(struct imx_mu_priv *priv); };
> +
> +struct imx_mu_con_priv {
> +	int			irq;
> +	unsigned int		bidx;
> +	unsigned int		idx;
> +};
> +
> +struct imx_mu_priv {
> +	struct device		*dev;
> +	const struct imx_mu_cfg	*dcfg;
> +	void __iomem		*base;
> +
> +	struct mbox_controller	mbox;
> +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> +
> +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> +	struct clk		*clk;
> +};
> +
> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
> +{
> +	return container_of(mbox, struct imx_mu_priv, mbox); }
> +
> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> +	iowrite32(val, priv->base + offs);
> +}
> +
> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> +	return ioread32(priv->base + offs);
> +}
> +
> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32
> +clr) {
> +	u32 val;
> +
> +	val = imx_mu_read(priv, offs);
> +	val &= ~clr;
> +	val |= set;
> +	imx_mu_write(priv, val, offs);
> +
> +	return val;
> +}
> +
> +static irqreturn_t imx_mu_isr(int irq, void *p) {
> +	struct mbox_chan *chan = p;
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	u32 val, dat;
> +
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {

I'm wondering whether this can work properly for multi consumers
at the same time. 
Because xSR_TEn is 1 by default and four virtual channels actually
are using the same interrupt. That means channel 1 interrupt may
cause channel 2 believe it's txdone.
Have we tested such using?

> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> >bidx));
> +		mbox_chan_txdone(chan, 0);
> +	}
> +
> +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> +		mbox_chan_received_data(chan, (void *)&dat);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	u32 val;
> +
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +	/* test if transmit register is empty */
> +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> +
> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	u32 *arg = data;
> +
> +	if (!imx_mu_last_tx_done(chan))
> +		return -EBUSY;
> +
> +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> +
> +	return 0;
> +}

Since Sascha is requesting to write a generic MU mailbox driver for both
SCU MU and M4 case, the current way using virtual channels in this patch
only send one word a time obviously can't fit for SCU MU clients well.
Do you think if we can refer to TI case to design a generic data transfer
protocol to allow send multi words which is more close to SCU?
include/linux/soc/ti/ti-msgmgr.h
struct ti_msgmgr_message {
        size_t len;
        u8 *buf;
};  

Or we try to support both type transfer protocols in this driver?
That may introduce much complexities, personally I'm not quite
like that.

Regards
Dong Aisheng

> +
> +static int imx_mu_startup(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	int ret;
> +
> +	ret = request_irq(cp->irq, imx_mu_isr,
> +			  IRQF_SHARED, "imx_mu_chan", chan);
> +	if (ret) {
> +		dev_err(chan->mbox->dev,
> +			"Unable to acquire IRQ %d\n", cp->irq);
> +		return ret;
> +	}
> +
> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> +
> +	return 0;
> +}
> +
> +static void imx_mu_shutdown(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> >bidx));
> +
> +	free_irq(cp->irq, chan);
> +}
> +
> +static const struct mbox_chan_ops imx_mu_ops = {
> +	.send_data = imx_mu_send_data,
> +	.startup = imx_mu_startup,
> +	.shutdown = imx_mu_shutdown,
> +};
> +
> +static int imx_mu_probe(struct platform_device *pdev) {
> +	struct device *dev = &pdev->dev;
> +	struct resource *iomem;
> +	struct imx_mu_priv *priv;
> +	const struct imx_mu_cfg *dcfg;
> +	unsigned int i, chans;
> +	int irq, ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dcfg = of_device_get_match_data(dev);
> +	if (!dcfg)
> +		return -EINVAL;
> +
> +	priv->dcfg = dcfg;
> +	priv->dev = dev;
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq <= 0)
> +		return irq < 0 ? irq : -EINVAL;
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		if (PTR_ERR(priv->clk) == -ENOENT) {
> +			priv->clk = NULL;
> +		} else {
> +			dev_err(dev, "Failed to get clock\n");
> +			return PTR_ERR(priv->clk);
> +		}
> +	}
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> +	/* Initialize channel identifiers */
> +	for (i = 0; i < chans; i++) {
> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> +		cp->bidx = 3 - i;
> +		cp->idx = i;
> +		cp->irq = irq;
> +		priv->mbox_chans[i].con_priv = cp;
> +	}
> +
> +	priv->mbox.dev = dev;
> +	priv->mbox.ops = &imx_mu_ops;
> +	priv->mbox.chans = priv->mbox_chans;
> +	priv->mbox.num_chans = chans;
> +	priv->mbox.txdone_irq = true;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	if (priv->dcfg->init_hw)
> +		priv->dcfg->init_hw(priv);
> +
> +	return mbox_controller_register(&priv->mbox);
> +}
> +
> +static int imx_mu_remove(struct platform_device *pdev) {
> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> +
> +	mbox_controller_unregister(&priv->mbox);
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +
> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> +	/* Set default config */
> +	imx_mu_write(priv, 0, IMX_MU_xCR);
> +}
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> +	.chans = IMX_MU_MAX_CHANS,
> +	.init_hw = imx_mu_init_imx7d_a,
> +};
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> +	.chans = IMX_MU_MAX_CHANS,
> +};
> +
> +static const struct of_device_id imx_mu_dt_ids[] = {
> +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> +
> +static struct platform_driver imx_mu_driver = {
> +	.probe		= imx_mu_probe,
> +	.remove		= imx_mu_remove,
> +	.driver = {
> +		.name	= "imx_mu",
> +		.of_match_table = imx_mu_dt_ids,
> +	},
> +};
> +module_platform_driver(imx_mu_driver);
> +
> +MODULE_AUTHOR("Oleksij Rempel <[e-mail]>");
> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
> MODULE_LICENSE("GPL
> +v2");
> --
> 2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Oleksij Rempel June 26, 2018, 10:56 a.m. UTC | #2

On 26.06.2018 12:09, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:[e-mail]]
>> Sent: Friday, June 15, 2018 5:51 PM
>> To: Shawn Guo <[e-mail]>; Fabio Estevam
>> <[e-mail]>; Rob Herring <[e-mail]>; Mark
>> Rutland <[e-mail]>; A.s. Dong <[e-mail]>
>> Cc: Oleksij Rempel <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; dl-linux-imx <[e-mail]>
>> Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
>>
>> The Mailbox controller is able to send messages (up to 4 32 bit words)
>> between the endpoints.
>>
>> This driver was tested using the mailbox-test driver sending messages
>> between the Cortex-A7 and the Cortex-M4.
>>
>> Signed-off-by: Oleksij Rempel <[e-mail]>
>> ---
>>  drivers/mailbox/Kconfig       |   6 +
>>  drivers/mailbox/Makefile      |   2 +
>>  drivers/mailbox/imx-mailbox.c | 288
>> ++++++++++++++++++++++++++++++++++
>>  3 files changed, 296 insertions(+)
>>  create mode 100644 drivers/mailbox/imx-mailbox.c
>>
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
>> a2bb27446dce..e1d2738a2e4c 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -15,6 +15,12 @@ config ARM_MHU
>>  	  The controller has 3 mailbox channels, the last of which can be
>>  	  used in Secure mode only.
>>
>> +config IMX_MBOX
>> +	tristate "iMX Mailbox"
>> +	depends on SOC_IMX7D || COMPILE_TEST
>> +	help
>> +	  Mailbox implementation for iMX7D Messaging Unit (MU).
>> +
>>  config PLATFORM_MHU
>>  	tristate "Platform MHU Mailbox"
>>  	depends on OF
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
>> cc23c3a43fcd..ba2fe1b6dd62 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
>>
>>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
>>
>> +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
>> +
>>  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
>>
>>  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
>> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
>> new file mode 100644 index 000000000000..e3f621cb1d30
>> --- /dev/null
>> +++ b/drivers/mailbox/imx-mailbox.c
>> @@ -0,0 +1,288 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
>> +<[e-mail]>  */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +
>> +/* Transmit Register */
>> +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
>> +/* Receive Register */
>> +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
>> +/* Status Register */
>> +#define IMX_MU_xSR		0x20
>> +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
>> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
>> +#define IMX_MU_xSR_BRDIP	BIT(9)
>> +
>> +/* Control Register */
>> +#define IMX_MU_xCR		0x24
>> +/* Transmit Interrupt Enable */
>> +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
>> +/* Receive Interrupt Enable */
>> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
>> +
>> +#define IMX_MU_MAX_CHANS	4u
>> +
>> +struct imx_mu_priv;
>> +
>> +struct imx_mu_cfg {
>> +	unsigned int		chans;
>> +	void (*init_hw)(struct imx_mu_priv *priv); };
>> +
>> +struct imx_mu_con_priv {
>> +	int			irq;
>> +	unsigned int		bidx;
>> +	unsigned int		idx;
>> +};
>> +
>> +struct imx_mu_priv {
>> +	struct device		*dev;
>> +	const struct imx_mu_cfg	*dcfg;
>> +	void __iomem		*base;
>> +
>> +	struct mbox_controller	mbox;
>> +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
>> +
>> +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
>> +	struct clk		*clk;
>> +};
>> +
>> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
>> +{
>> +	return container_of(mbox, struct imx_mu_priv, mbox); }
>> +
>> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
>> +	iowrite32(val, priv->base + offs);
>> +}
>> +
>> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
>> +	return ioread32(priv->base + offs);
>> +}
>> +
>> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32
>> +clr) {
>> +	u32 val;
>> +
>> +	val = imx_mu_read(priv, offs);
>> +	val &= ~clr;
>> +	val |= set;
>> +	imx_mu_write(priv, val, offs);
>> +
>> +	return val;
>> +}
>> +
>> +static irqreturn_t imx_mu_isr(int irq, void *p) {
>> +	struct mbox_chan *chan = p;
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +
>> +	u32 val, dat;
>> +
>> +	val = imx_mu_read(priv, IMX_MU_xSR);
>> +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
>> +	if (!val)
>> +		return IRQ_NONE;
>> +
>> +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> 
> I'm wondering whether this can work properly for multi consumers
> at the same time. 
> Because xSR_TEn is 1 by default and four virtual channels actually
> are using the same interrupt. That means channel 1 interrupt may
> cause channel 2 believe it's txdone.
> Have we tested such using?

see imx_mu_send_data()
..... imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);

> 
>> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
>>> bidx));

and here ^^^
TX status is enabled only for send and disabled as soon at transfer is done.

>> +		mbox_chan_txdone(chan, 0);
>> +	}
>> +
>> +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
>> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
>> +		mbox_chan_received_data(chan, (void *)&dat);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +	u32 val;
>> +
>> +	val = imx_mu_read(priv, IMX_MU_xSR);
>> +	/* test if transmit register is empty */
>> +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
>> +
>> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +	u32 *arg = data;
>> +
>> +	if (!imx_mu_last_tx_done(chan))
>> +		return -EBUSY;
>> +
>> +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
>> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
>> +
>> +	return 0;
>> +}
> 
> Since Sascha is requesting to write a generic MU mailbox driver for both
> SCU MU and M4 case, the current way using virtual channels in this patch
> only send one word a time obviously can't fit for SCU MU clients well.
> Do you think if we can refer to TI case to design a generic data transfer
> protocol to allow send multi words which is more close to SCU?

According to your code, you are able to send 1 word message. It means,
your SCU is configured to trigger an interrupt or status update if REG0
was written. The same is true for 2, 3, 4 and 5 word messages.

If the MU configuration would look like you it described, you would be
forced to write always 4 words, even for 1 word message.

> include/linux/soc/ti/ti-msgmgr.h
> struct ti_msgmgr_message {
>         size_t len;
>         u8 *buf;
> };  
> 
> Or we try to support both type transfer protocols in this driver?

Sure. ti-msgmgr.c is a good example. You will probably need reduced
variant of it. It is generic enough to make it useful not only for SCU.

> That may introduce much complexities, personally I'm not quite
> like that.

I expect 50-150 lines of extra code.

> Regards
> Dong Aisheng
> 
>> +
>> +static int imx_mu_startup(struct mbox_chan *chan) {
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +	int ret;
>> +
>> +	ret = request_irq(cp->irq, imx_mu_isr,
>> +			  IRQF_SHARED, "imx_mu_chan", chan);
>> +	if (ret) {
>> +		dev_err(chan->mbox->dev,
>> +			"Unable to acquire IRQ %d\n", cp->irq);
>> +		return ret;
>> +	}
>> +
>> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static void imx_mu_shutdown(struct mbox_chan *chan) {
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +
>> +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
>> +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
>>> bidx));
>> +
>> +	free_irq(cp->irq, chan);
>> +}
>> +
>> +static const struct mbox_chan_ops imx_mu_ops = {
>> +	.send_data = imx_mu_send_data,
>> +	.startup = imx_mu_startup,
>> +	.shutdown = imx_mu_shutdown,
>> +};
>> +
>> +static int imx_mu_probe(struct platform_device *pdev) {
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *iomem;
>> +	struct imx_mu_priv *priv;
>> +	const struct imx_mu_cfg *dcfg;
>> +	unsigned int i, chans;
>> +	int irq, ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	dcfg = of_device_get_match_data(dev);
>> +	if (!dcfg)
>> +		return -EINVAL;
>> +
>> +	priv->dcfg = dcfg;
>> +	priv->dev = dev;
>> +
>> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
>> +	if (IS_ERR(priv->base))
>> +		return PTR_ERR(priv->base);
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq <= 0)
>> +		return irq < 0 ? irq : -EINVAL;
>> +
>> +	priv->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(priv->clk)) {
>> +		if (PTR_ERR(priv->clk) == -ENOENT) {
>> +			priv->clk = NULL;
>> +		} else {
>> +			dev_err(dev, "Failed to get clock\n");
>> +			return PTR_ERR(priv->clk);
>> +		}
>> +	}
>> +
>> +	ret = clk_prepare_enable(priv->clk);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable clock\n");
>> +		return ret;
>> +	}
>> +
>> +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
>> +	/* Initialize channel identifiers */
>> +	for (i = 0; i < chans; i++) {
>> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
>> +
>> +		cp->bidx = 3 - i;
>> +		cp->idx = i;
>> +		cp->irq = irq;
>> +		priv->mbox_chans[i].con_priv = cp;
>> +	}
>> +
>> +	priv->mbox.dev = dev;
>> +	priv->mbox.ops = &imx_mu_ops;
>> +	priv->mbox.chans = priv->mbox_chans;
>> +	priv->mbox.num_chans = chans;
>> +	priv->mbox.txdone_irq = true;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	if (priv->dcfg->init_hw)
>> +		priv->dcfg->init_hw(priv);
>> +
>> +	return mbox_controller_register(&priv->mbox);
>> +}
>> +
>> +static int imx_mu_remove(struct platform_device *pdev) {
>> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
>> +
>> +	mbox_controller_unregister(&priv->mbox);
>> +	clk_disable_unprepare(priv->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
>> +	/* Set default config */
>> +	imx_mu_write(priv, 0, IMX_MU_xCR);
>> +}
>> +
>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
>> +	.chans = IMX_MU_MAX_CHANS,
>> +	.init_hw = imx_mu_init_imx7d_a,
>> +};
>> +
>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
>> +	.chans = IMX_MU_MAX_CHANS,
>> +};
>> +
>> +static const struct of_device_id imx_mu_dt_ids[] = {
>> +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
>> +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
>> +
>> +static struct platform_driver imx_mu_driver = {
>> +	.probe		= imx_mu_probe,
>> +	.remove		= imx_mu_remove,
>> +	.driver = {
>> +		.name	= "imx_mu",
>> +		.of_match_table = imx_mu_dt_ids,
>> +	},
>> +};
>> +module_platform_driver(imx_mu_driver);
>> +
>> +MODULE_AUTHOR("Oleksij Rempel <[e-mail]>");
>> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
>> MODULE_LICENSE("GPL
>> +v2");
>> --
>> 2.17.1
> 
> 
>

Aisheng Dong June 26, 2018, 12:07 p.m. UTC | #3

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBPbGVrc2lqIFJlbXBlbCBbbWFp
bHRvOm8ucmVtcGVsQHBlbmd1dHJvbml4LmRlXQ0KPiBTZW50OiBUdWVzZGF5LCBKdW5lIDI2LCAy
MDE4IDY6NTYgUE0NCj4gVG86IEEucy4gRG9uZyA8YWlzaGVuZy5kb25nQG54cC5jb20+OyBTaGF3
biBHdW8NCj4gPHNoYXduZ3VvQGtlcm5lbC5vcmc+OyBGYWJpbyBFc3RldmFtIDxmYWJpby5lc3Rl
dmFtQG54cC5jb20+OyBSb2INCj4gSGVycmluZyA8cm9iaCtkdEBrZXJuZWwub3JnPjsgTWFyayBS
dXRsYW5kIDxtYXJrLnJ1dGxhbmRAYXJtLmNvbT4NCj4gQ2M6IGRldmljZXRyZWVAdmdlci5rZXJu
ZWwub3JnOyBkbC1saW51eC1pbXggPGxpbnV4LWlteEBueHAuY29tPjsgbGludXgtDQo+IGFybS1r
ZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZzsga2VybmVsQHBlbmd1dHJvbml4LmRlOyBsaW51eC0N
Cj4gY2xrQHZnZXIua2VybmVsLm9yZw0KPiBTdWJqZWN0OiBSZTogW1BBVENIIHYyIDQvNF0gbWFp
bGJveDogQWRkIHN1cHBvcnQgZm9yIGkuTVg3RCBtZXNzYWdpbmcgdW5pdA0KPiANCj4gDQo+IA0K
PiBPbiAyNi4wNi4yMDE4IDEyOjA5LCBBLnMuIERvbmcgd3JvdGU6DQo+ID4+IC0tLS0tT3JpZ2lu
YWwgTWVzc2FnZS0tLS0tDQo+ID4+IEZyb206IE9sZWtzaWogUmVtcGVsIFttYWlsdG86by5yZW1w
ZWxAcGVuZ3V0cm9uaXguZGVdDQo+ID4+IFNlbnQ6IEZyaWRheSwgSnVuZSAxNSwgMjAxOCA1OjUx
IFBNDQo+ID4+IFRvOiBTaGF3biBHdW8gPHNoYXduZ3VvQGtlcm5lbC5vcmc+OyBGYWJpbyBFc3Rl
dmFtDQo+ID4+IDxmYWJpby5lc3RldmFtQG54cC5jb20+OyBSb2IgSGVycmluZyA8cm9iaCtkdEBr
ZXJuZWwub3JnPjsgTWFyaw0KPiA+PiBSdXRsYW5kIDxtYXJrLnJ1dGxhbmRAYXJtLmNvbT47IEEu
cy4gRG9uZyA8YWlzaGVuZy5kb25nQG54cC5jb20+DQo+ID4+IENjOiBPbGVrc2lqIFJlbXBlbCA8
by5yZW1wZWxAcGVuZ3V0cm9uaXguZGU+OyBrZXJuZWxAcGVuZ3V0cm9uaXguZGU7DQo+ID4+IGxp
bnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZzsgZGV2aWNldHJlZUB2Z2VyLmtlcm5l
bC5vcmc7DQo+ID4+IGxpbnV4LSBjbGtAdmdlci5rZXJuZWwub3JnOyBkbC1saW51eC1pbXggPGxp
bnV4LWlteEBueHAuY29tPg0KPiA+PiBTdWJqZWN0OiBbUEFUQ0ggdjIgNC80XSBtYWlsYm94OiBB
ZGQgc3VwcG9ydCBmb3IgaS5NWDdEIG1lc3NhZ2luZw0KPiA+PiB1bml0DQo+ID4+DQo+ID4+IFRo
ZSBNYWlsYm94IGNvbnRyb2xsZXIgaXMgYWJsZSB0byBzZW5kIG1lc3NhZ2VzICh1cCB0byA0IDMy
IGJpdA0KPiA+PiB3b3JkcykgYmV0d2VlbiB0aGUgZW5kcG9pbnRzLg0KPiA+Pg0KPiA+PiBUaGlz
IGRyaXZlciB3YXMgdGVzdGVkIHVzaW5nIHRoZSBtYWlsYm94LXRlc3QgZHJpdmVyIHNlbmRpbmcg
bWVzc2FnZXMNCj4gPj4gYmV0d2VlbiB0aGUgQ29ydGV4LUE3IGFuZCB0aGUgQ29ydGV4LU00Lg0K
PiA+Pg0KPiA+PiBTaWduZWQtb2ZmLWJ5OiBPbGVrc2lqIFJlbXBlbCA8by5yZW1wZWxAcGVuZ3V0
cm9uaXguZGU+DQo+ID4+IC0tLQ0KPiA+PiAgZHJpdmVycy9tYWlsYm94L0tjb25maWcgICAgICAg
fCAgIDYgKw0KPiA+PiAgZHJpdmVycy9tYWlsYm94L01ha2VmaWxlICAgICAgfCAgIDIgKw0KPiA+
PiAgZHJpdmVycy9tYWlsYm94L2lteC1tYWlsYm94LmMgfCAyODgNCj4gPj4gKysrKysrKysrKysr
KysrKysrKysrKysrKysrKysrKysrKw0KPiA+PiAgMyBmaWxlcyBjaGFuZ2VkLCAyOTYgaW5zZXJ0
aW9ucygrKQ0KPiA+PiAgY3JlYXRlIG1vZGUgMTAwNjQ0IGRyaXZlcnMvbWFpbGJveC9pbXgtbWFp
bGJveC5jDQo+ID4+DQo+ID4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL21haWxib3gvS2NvbmZpZyBi
L2RyaXZlcnMvbWFpbGJveC9LY29uZmlnIGluZGV4DQo+ID4+IGEyYmIyNzQ0NmRjZS4uZTFkMjcz
OGEyZTRjIDEwMDY0NA0KPiA+PiAtLS0gYS9kcml2ZXJzL21haWxib3gvS2NvbmZpZw0KPiA+PiAr
KysgYi9kcml2ZXJzL21haWxib3gvS2NvbmZpZw0KPiA+PiBAQCAtMTUsNiArMTUsMTIgQEAgY29u
ZmlnIEFSTV9NSFUNCj4gPj4gIAkgIFRoZSBjb250cm9sbGVyIGhhcyAzIG1haWxib3ggY2hhbm5l
bHMsIHRoZSBsYXN0IG9mIHdoaWNoIGNhbiBiZQ0KPiA+PiAgCSAgdXNlZCBpbiBTZWN1cmUgbW9k
ZSBvbmx5Lg0KPiA+Pg0KPiA+PiArY29uZmlnIElNWF9NQk9YDQo+ID4+ICsJdHJpc3RhdGUgImlN
WCBNYWlsYm94Ig0KPiA+PiArCWRlcGVuZHMgb24gU09DX0lNWDdEIHx8IENPTVBJTEVfVEVTVA0K
PiA+PiArCWhlbHANCj4gPj4gKwkgIE1haWxib3ggaW1wbGVtZW50YXRpb24gZm9yIGlNWDdEIE1l
c3NhZ2luZyBVbml0IChNVSkuDQo+ID4+ICsNCj4gPj4gIGNvbmZpZyBQTEFURk9STV9NSFUNCj4g
Pj4gIAl0cmlzdGF0ZSAiUGxhdGZvcm0gTUhVIE1haWxib3giDQo+ID4+ICAJZGVwZW5kcyBvbiBP
Rg0KPiA+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9tYWlsYm94L01ha2VmaWxlIGIvZHJpdmVycy9t
YWlsYm94L01ha2VmaWxlDQo+ID4+IGluZGV4DQo+ID4+IGNjMjNjM2E0M2ZjZC4uYmEyZmUxYjZk
ZDYyIDEwMDY0NA0KPiA+PiAtLS0gYS9kcml2ZXJzL21haWxib3gvTWFrZWZpbGUNCj4gPj4gKysr
IGIvZHJpdmVycy9tYWlsYm94L01ha2VmaWxlDQo+ID4+IEBAIC03LDYgKzcsOCBAQCBvYmotJChD
T05GSUdfTUFJTEJPWF9URVNUKQkrPSBtYWlsYm94LXRlc3Qubw0KPiA+Pg0KPiA+PiAgb2JqLSQo
Q09ORklHX0FSTV9NSFUpCSs9IGFybV9taHUubw0KPiA+Pg0KPiA+PiArb2JqLSQoQ09ORklHX0lN
WF9NQk9YKQkrPSBpbXgtbWFpbGJveC5vDQo+ID4+ICsNCj4gPj4gIG9iai0kKENPTkZJR19QTEFU
Rk9STV9NSFUpCSs9IHBsYXRmb3JtX21odS5vDQo+ID4+DQo+ID4+ICBvYmotJChDT05GSUdfUEwz
MjBfTUJPWCkJKz0gcGwzMjAtaXBjLm8NCj4gPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvbWFpbGJv
eC9pbXgtbWFpbGJveC5jDQo+ID4+IGIvZHJpdmVycy9tYWlsYm94L2lteC1tYWlsYm94LmMgbmV3
IGZpbGUgbW9kZSAxMDA2NDQgaW5kZXgNCj4gPj4gMDAwMDAwMDAwMDAwLi5lM2Y2MjFjYjFkMzAN
Cj4gPj4gLS0tIC9kZXYvbnVsbA0KPiA+PiArKysgYi9kcml2ZXJzL21haWxib3gvaW14LW1haWxi
b3guYw0KPiA+PiBAQCAtMCwwICsxLDI4OCBAQA0KPiA+PiArLy8gU1BEWC1MaWNlbnNlLUlkZW50
aWZpZXI6IEdQTC0yLjANCj4gPj4gKy8qDQo+ID4+ICsgKiBDb3B5cmlnaHQgKGMpIDIwMTggUGVu
Z3V0cm9uaXgsIE9sZWtzaWogUmVtcGVsDQo+ID4+ICs8by5yZW1wZWxAcGVuZ3V0cm9uaXguZGU+
ICAqLw0KPiA+PiArDQo+ID4+ICsjaW5jbHVkZSA8bGludXgvY2xrLmg+DQo+ID4+ICsjaW5jbHVk
ZSA8bGludXgvaW50ZXJydXB0Lmg+DQo+ID4+ICsjaW5jbHVkZSA8bGludXgvaW8uaD4NCj4gPj4g
KyNpbmNsdWRlIDxsaW51eC9rZXJuZWwuaD4NCj4gPj4gKyNpbmNsdWRlIDxsaW51eC9tYWlsYm94
X2NvbnRyb2xsZXIuaD4gI2luY2x1ZGUgPGxpbnV4L21vZHVsZS5oPg0KPiA+PiArI2luY2x1ZGUg
PGxpbnV4L29mX2RldmljZS5oPg0KPiA+PiArDQo+ID4+ICsvKiBUcmFuc21pdCBSZWdpc3RlciAq
Lw0KPiA+PiArI2RlZmluZSBJTVhfTVVfeFRSbih4KQkJKDB4MDAgKyA0ICogKHgpKQ0KPiA+PiAr
LyogUmVjZWl2ZSBSZWdpc3RlciAqLw0KPiA+PiArI2RlZmluZSBJTVhfTVVfeFJSbih4KQkJKDB4
MTAgKyA0ICogKHgpKQ0KPiA+PiArLyogU3RhdHVzIFJlZ2lzdGVyICovDQo+ID4+ICsjZGVmaW5l
IElNWF9NVV94U1IJCTB4MjANCj4gPj4gKyNkZWZpbmUgSU1YX01VX3hTUl9URW4oeCkJQklUKDIw
ICsgKHgpKQ0KPiA+PiArI2RlZmluZSBJTVhfTVVfeFNSX1JGbih4KQlCSVQoMjQgKyAoeCkpDQo+
ID4+ICsjZGVmaW5lIElNWF9NVV94U1JfQlJESVAJQklUKDkpDQo+ID4+ICsNCj4gPj4gKy8qIENv
bnRyb2wgUmVnaXN0ZXIgKi8NCj4gPj4gKyNkZWZpbmUgSU1YX01VX3hDUgkJMHgyNA0KPiA+PiAr
LyogVHJhbnNtaXQgSW50ZXJydXB0IEVuYWJsZSAqLw0KPiA+PiArI2RlZmluZSBJTVhfTVVfeENS
X1RJRW4oeCkJQklUKDIwICsgKHgpKQ0KPiA+PiArLyogUmVjZWl2ZSBJbnRlcnJ1cHQgRW5hYmxl
ICovDQo+ID4+ICsjZGVmaW5lIElNWF9NVV94Q1JfUklFbih4KQlCSVQoMjQgKyAoeCkpDQo+ID4+
ICsNCj4gPj4gKyNkZWZpbmUgSU1YX01VX01BWF9DSEFOUwk0dQ0KPiA+PiArDQo+ID4+ICtzdHJ1
Y3QgaW14X211X3ByaXY7DQo+ID4+ICsNCj4gPj4gK3N0cnVjdCBpbXhfbXVfY2ZnIHsNCj4gPj4g
Kwl1bnNpZ25lZCBpbnQJCWNoYW5zOw0KPiA+PiArCXZvaWQgKCppbml0X2h3KShzdHJ1Y3QgaW14
X211X3ByaXYgKnByaXYpOyB9Ow0KPiA+PiArDQo+ID4+ICtzdHJ1Y3QgaW14X211X2Nvbl9wcml2
IHsNCj4gPj4gKwlpbnQJCQlpcnE7DQo+ID4+ICsJdW5zaWduZWQgaW50CQliaWR4Ow0KPiA+PiAr
CXVuc2lnbmVkIGludAkJaWR4Ow0KPiA+PiArfTsNCj4gPj4gKw0KPiA+PiArc3RydWN0IGlteF9t
dV9wcml2IHsNCj4gPj4gKwlzdHJ1Y3QgZGV2aWNlCQkqZGV2Ow0KPiA+PiArCWNvbnN0IHN0cnVj
dCBpbXhfbXVfY2ZnCSpkY2ZnOw0KPiA+PiArCXZvaWQgX19pb21lbQkJKmJhc2U7DQo+ID4+ICsN
Cj4gPj4gKwlzdHJ1Y3QgbWJveF9jb250cm9sbGVyCW1ib3g7DQo+ID4+ICsJc3RydWN0IG1ib3hf
Y2hhbgltYm94X2NoYW5zW0lNWF9NVV9NQVhfQ0hBTlNdOw0KPiA+PiArDQo+ID4+ICsJc3RydWN0
IGlteF9tdV9jb25fcHJpdiAgY29uX3ByaXZbSU1YX01VX01BWF9DSEFOU107DQo+ID4+ICsJc3Ry
dWN0IGNsawkJKmNsazsNCj4gPj4gK307DQo+ID4+ICsNCj4gPj4gK3N0YXRpYyBzdHJ1Y3QgaW14
X211X3ByaXYgKnRvX2lteF9tdV9wcml2KHN0cnVjdCBtYm94X2NvbnRyb2xsZXINCj4gPj4gKypt
Ym94KSB7DQo+ID4+ICsJcmV0dXJuIGNvbnRhaW5lcl9vZihtYm94LCBzdHJ1Y3QgaW14X211X3By
aXYsIG1ib3gpOyB9DQo+ID4+ICsNCj4gPj4gK3N0YXRpYyB2b2lkIGlteF9tdV93cml0ZShzdHJ1
Y3QgaW14X211X3ByaXYgKnByaXYsIHUzMiB2YWwsIHUzMiBvZmZzKSB7DQo+ID4+ICsJaW93cml0
ZTMyKHZhbCwgcHJpdi0+YmFzZSArIG9mZnMpOw0KPiA+PiArfQ0KPiA+PiArDQo+ID4+ICtzdGF0
aWMgdTMyIGlteF9tdV9yZWFkKHN0cnVjdCBpbXhfbXVfcHJpdiAqcHJpdiwgdTMyIG9mZnMpIHsN
Cj4gPj4gKwlyZXR1cm4gaW9yZWFkMzIocHJpdi0+YmFzZSArIG9mZnMpOyB9DQo+ID4+ICsNCj4g
Pj4gK3N0YXRpYyB1MzIgaW14X211X3JtdyhzdHJ1Y3QgaW14X211X3ByaXYgKnByaXYsIHUzMiBv
ZmZzLCB1MzIgc2V0LA0KPiA+PiArdTMyDQo+ID4+ICtjbHIpIHsNCj4gPj4gKwl1MzIgdmFsOw0K
PiA+PiArDQo+ID4+ICsJdmFsID0gaW14X211X3JlYWQocHJpdiwgb2Zmcyk7DQo+ID4+ICsJdmFs
ICY9IH5jbHI7DQo+ID4+ICsJdmFsIHw9IHNldDsNCj4gPj4gKwlpbXhfbXVfd3JpdGUocHJpdiwg
dmFsLCBvZmZzKTsNCj4gPj4gKw0KPiA+PiArCXJldHVybiB2YWw7DQo+ID4+ICt9DQo+ID4+ICsN
Cj4gPj4gK3N0YXRpYyBpcnFyZXR1cm5fdCBpbXhfbXVfaXNyKGludCBpcnEsIHZvaWQgKnApIHsN
Cj4gPj4gKwlzdHJ1Y3QgbWJveF9jaGFuICpjaGFuID0gcDsNCj4gPj4gKwlzdHJ1Y3QgaW14X211
X3ByaXYgKnByaXYgPSB0b19pbXhfbXVfcHJpdihjaGFuLT5tYm94KTsNCj4gPj4gKwlzdHJ1Y3Qg
aW14X211X2Nvbl9wcml2ICpjcCA9IGNoYW4tPmNvbl9wcml2Ow0KPiA+PiArDQo+ID4+ICsJdTMy
IHZhbCwgZGF0Ow0KPiA+PiArDQo+ID4+ICsJdmFsID0gaW14X211X3JlYWQocHJpdiwgSU1YX01V
X3hTUik7DQo+ID4+ICsJdmFsICY9IElNWF9NVV94U1JfVEVuKGNwLT5iaWR4KSB8IElNWF9NVV94
U1JfUkZuKGNwLT5iaWR4KTsNCj4gPj4gKwlpZiAoIXZhbCkNCj4gPj4gKwkJcmV0dXJuIElSUV9O
T05FOw0KPiA+PiArDQo+ID4+ICsJaWYgKHZhbCAmIElNWF9NVV94U1JfVEVuKGNwLT5iaWR4KSkg
ew0KPiA+DQo+ID4gSSdtIHdvbmRlcmluZyB3aGV0aGVyIHRoaXMgY2FuIHdvcmsgcHJvcGVybHkg
Zm9yIG11bHRpIGNvbnN1bWVycyBhdA0KPiA+IHRoZSBzYW1lIHRpbWUuDQo+ID4gQmVjYXVzZSB4
U1JfVEVuIGlzIDEgYnkgZGVmYXVsdCBhbmQgZm91ciB2aXJ0dWFsIGNoYW5uZWxzIGFjdHVhbGx5
IGFyZQ0KPiA+IHVzaW5nIHRoZSBzYW1lIGludGVycnVwdC4gVGhhdCBtZWFucyBjaGFubmVsIDEg
aW50ZXJydXB0IG1heSBjYXVzZQ0KPiA+IGNoYW5uZWwgMiBiZWxpZXZlIGl0J3MgdHhkb25lLg0K
PiA+IEhhdmUgd2UgdGVzdGVkIHN1Y2ggdXNpbmc/DQo+IA0KPiBzZWUgaW14X211X3NlbmRfZGF0
YSgpDQo+IC4uLi4uIGlteF9tdV9ybXcocHJpdiwgSU1YX01VX3hDUiwgSU1YX01VX3hTUl9URW4o
Y3AtPmJpZHgpLCAwKTsNCj4gDQo+ID4NCj4gPj4gKwkJaW14X211X3Jtdyhwcml2LCBJTVhfTVVf
eENSLCAwLCBJTVhfTVVfeENSX1RJRW4oY3AtDQo+ID4+PiBiaWR4KSk7DQo+IA0KPiBhbmQgaGVy
ZSBeXl4NCj4gVFggc3RhdHVzIGlzIGVuYWJsZWQgb25seSBmb3Igc2VuZCBhbmQgZGlzYWJsZWQg
YXMgc29vbiBhdCB0cmFuc2ZlciBpcyBkb25lLg0KPiANCg0KVGhhdCBjb250cm9scyB0aGUgaW50
ZXJydXB0IGVuYWJsZSBzaWduYWwsIEknbSBub3Qgc3VyZSBpZiB0aGUgc3RhdHVzIGJpdA0KV29u
J3QgYmUgc2V0IGlmIG5vdCBlbmFibGUgaW50ZXJydXB0LiBJJ3ZlIG5vdCB0ZXN0ZWQgaXQuDQoN
CkhhdmUgeW91IGRvdWJsZSBjaGVja2VkIGl0Pw0KDQpGb3IgbiA9IHswLCAxLCAyLCAzfSBQcm9j
ZXNzb3IgQSBUcmFuc21pdCBSZWdpc3RlciBuIEVtcHR5LiAoUmVhZC1vbmx5KQ0K4oCiIFRoZSBU
RW4gYml0IGlzIHNldCB0byDigJwx4oCdIGFmdGVyIHRoZSBCUlJuIHJlZ2lzdGVyIGlzIHJlYWQg
b24gdGhlIFByb2Nlc3NvciBCLXNpZGUuDQrigKIgQWZ0ZXIgdGhlIFRFbiBiaXQgaXMgc2V0IHRv
IOKAnDHigJ0sIHRoZSBURW4gYml0IHNpZ25hbHMgdGhlIFByb2Nlc3NvciBBLXNpZGUgdGhhdCB0
aGUgQVRSbiByZWdpc3RlciBpcw0KcmVhZHkgdG8gYmUgd3JpdHRlbiBvbiB0aGUgUHJvY2Vzc29y
IEEtc2lkZSwgYW5kIGEgVHJhbnNtaXQgbiBpbnRlcnJ1cHQgaXMgaXNzdWVkIG9uIHRoZSBQcm9j
ZXNzb3INCkEtc2lkZSAoaWYgdGhlIFRFbiBiaXQgaW4gdGhlIEFDUiByZWdpc3RlciBpcyBzZXQg
dG8g4oCcMeKAnSkuDQouLi4NCg0KPiA+PiArCQltYm94X2NoYW5fdHhkb25lKGNoYW4sIDApOw0K
PiA+PiArCX0NCj4gPj4gKw0KPiA+PiArCWlmICh2YWwgJiBJTVhfTVVfeFNSX1JGbihjcC0+Ymlk
eCkpIHsNCj4gPj4gKwkJZGF0ID0gaW14X211X3JlYWQocHJpdiwgSU1YX01VX3hSUm4oY3AtPmlk
eCkpOw0KPiA+PiArCQltYm94X2NoYW5fcmVjZWl2ZWRfZGF0YShjaGFuLCAodm9pZCAqKSZkYXQp
Ow0KPiA+PiArCX0NCj4gPj4gKw0KPiA+PiArCXJldHVybiBJUlFfSEFORExFRDsNCj4gPj4gK30N
Cj4gPj4gKw0KPiA+PiArc3RhdGljIGJvb2wgaW14X211X2xhc3RfdHhfZG9uZShzdHJ1Y3QgbWJv
eF9jaGFuICpjaGFuKSB7DQo+ID4+ICsJc3RydWN0IGlteF9tdV9wcml2ICpwcml2ID0gdG9faW14
X211X3ByaXYoY2hhbi0+bWJveCk7DQo+ID4+ICsJc3RydWN0IGlteF9tdV9jb25fcHJpdiAqY3Ag
PSBjaGFuLT5jb25fcHJpdjsNCj4gPj4gKwl1MzIgdmFsOw0KPiA+PiArDQo+ID4+ICsJdmFsID0g
aW14X211X3JlYWQocHJpdiwgSU1YX01VX3hTUik7DQo+ID4+ICsJLyogdGVzdCBpZiB0cmFuc21p
dCByZWdpc3RlciBpcyBlbXB0eSAqLw0KPiA+PiArCXJldHVybiAoISEodmFsICYgSU1YX01VX3hT
Ul9URW4oY3AtPmJpZHgpKSk7IH0NCj4gPj4gKw0KPiA+PiArc3RhdGljIGludCBpbXhfbXVfc2Vu
ZF9kYXRhKHN0cnVjdCBtYm94X2NoYW4gKmNoYW4sIHZvaWQgKmRhdGEpIHsNCj4gPj4gKwlzdHJ1
Y3QgaW14X211X3ByaXYgKnByaXYgPSB0b19pbXhfbXVfcHJpdihjaGFuLT5tYm94KTsNCj4gPj4g
KwlzdHJ1Y3QgaW14X211X2Nvbl9wcml2ICpjcCA9IGNoYW4tPmNvbl9wcml2Ow0KPiA+PiArCXUz
MiAqYXJnID0gZGF0YTsNCj4gPj4gKw0KPiA+PiArCWlmICghaW14X211X2xhc3RfdHhfZG9uZShj
aGFuKSkNCj4gPj4gKwkJcmV0dXJuIC1FQlVTWTsNCj4gPj4gKw0KPiA+PiArCWlteF9tdV93cml0
ZShwcml2LCAqYXJnLCBJTVhfTVVfeFRSbihjcC0+aWR4KSk7DQo+ID4+ICsJaW14X211X3Jtdyhw
cml2LCBJTVhfTVVfeENSLCBJTVhfTVVfeFNSX1RFbihjcC0+YmlkeCksIDApOw0KPiA+PiArDQo+
ID4+ICsJcmV0dXJuIDA7DQo+ID4+ICt9DQo+ID4NCj4gPiBTaW5jZSBTYXNjaGEgaXMgcmVxdWVz
dGluZyB0byB3cml0ZSBhIGdlbmVyaWMgTVUgbWFpbGJveCBkcml2ZXIgZm9yDQo+ID4gYm90aCBT
Q1UgTVUgYW5kIE00IGNhc2UsIHRoZSBjdXJyZW50IHdheSB1c2luZyB2aXJ0dWFsIGNoYW5uZWxz
IGluDQo+ID4gdGhpcyBwYXRjaCBvbmx5IHNlbmQgb25lIHdvcmQgYSB0aW1lIG9idmlvdXNseSBj
YW4ndCBmaXQgZm9yIFNDVSBNVSBjbGllbnRzDQo+IHdlbGwuDQo+ID4gRG8geW91IHRoaW5rIGlm
IHdlIGNhbiByZWZlciB0byBUSSBjYXNlIHRvIGRlc2lnbiBhIGdlbmVyaWMgZGF0YQ0KPiA+IHRy
YW5zZmVyIHByb3RvY29sIHRvIGFsbG93IHNlbmQgbXVsdGkgd29yZHMgd2hpY2ggaXMgbW9yZSBj
bG9zZSB0byBTQ1U/DQo+IA0KPiBBY2NvcmRpbmcgdG8geW91ciBjb2RlLCB5b3UgYXJlIGFibGUg
dG8gc2VuZCAxIHdvcmQgbWVzc2FnZS4gSXQgbWVhbnMsIHlvdXINCj4gU0NVIGlzIGNvbmZpZ3Vy
ZWQgdG8gdHJpZ2dlciBhbiBpbnRlcnJ1cHQgb3Igc3RhdHVzIHVwZGF0ZSBpZiBSRUcwIHdhcyB3
cml0dGVuLg0KPiBUaGUgc2FtZSBpcyB0cnVlIGZvciAyLCAzLCA0IGFuZCA1IHdvcmQgbWVzc2Fn
ZXMuDQo+IA0KDQpTQ1UgaXMgaW50ZXJydXB0IGRyaXZlbiBhbHJlYWR5IGZvciB0aGUgZmlyc3Qg
d29yZC4NCldlIGRvIGNhbiBzZW5kIHdvcmQgb25lIGJ5IG9uZSBidXQgdGhlIHBlcmZvcm1hbmNl
IHdvdWxkIGJlIHRlcnJpYmxlDQpjb21wYXJpbmcgdG8gd3JpdGUgNCBhIHRpbWUuDQoNCj4gSWYg
dGhlIE1VIGNvbmZpZ3VyYXRpb24gd291bGQgbG9vayBsaWtlIHlvdSBpdCBkZXNjcmliZWQsIHlv
dSB3b3VsZCBiZSBmb3JjZWQNCj4gdG8gd3JpdGUgYWx3YXlzIDQgd29yZHMsIGV2ZW4gZm9yIDEg
d29yZCBtZXNzYWdlLg0KPiANCj4gPiBpbmNsdWRlL2xpbnV4L3NvYy90aS90aS1tc2dtZ3IuaA0K
PiA+IHN0cnVjdCB0aV9tc2dtZ3JfbWVzc2FnZSB7DQo+ID4gICAgICAgICBzaXplX3QgbGVuOw0K
PiA+ICAgICAgICAgdTggKmJ1ZjsNCj4gPiB9Ow0KPiA+DQo+ID4gT3Igd2UgdHJ5IHRvIHN1cHBv
cnQgYm90aCB0eXBlIHRyYW5zZmVyIHByb3RvY29scyBpbiB0aGlzIGRyaXZlcj8NCj4gDQo+IFN1
cmUuIHRpLW1zZ21nci5jIGlzIGEgZ29vZCBleGFtcGxlLiBZb3Ugd2lsbCBwcm9iYWJseSBuZWVk
IHJlZHVjZWQgdmFyaWFudA0KPiBvZiBpdC4gSXQgaXMgZ2VuZXJpYyBlbm91Z2ggdG8gbWFrZSBp
dCB1c2VmdWwgbm90IG9ubHkgZm9yIFNDVS4NCj4gDQoNClNhc2NoYSBuZWVkcyBhIGNvbW1vbiBk
ZXNpZ24gZm9yIGJvdGggTTQgYW5kIFNDVS5JZiBkZWNpZGUgdG8gZG8gdGhhdCwNCnlvdSBwcm9i
YWJseSBuZWVkIHVwZGF0ZSB0aGlzIHBhdGNoIGFzIHdlbGwuDQoNCkJ1dCBldmVuIGRvaW5nIGxp
a2UgVEkgc3R5bGUsIGl0IHN0aWxsIG5lZWQgaGFjayBmb3IgU0NVIGFzIHRoZSBkYXRhIHNpemUg
b2Zmc2V0DQpJcyBkaWZmZXJlbnQuIEhvd2V2ZXIsIHRoYXQgd291bGQgYmUgYSBtdWNoIHNtYWxs
ZXIgaGFjayB0aGFuIGRvaW5nIGJhc2VkDQpPbiB0aGlzIGRyaXZlci4NCg0KPiA+IFRoYXQgbWF5
IGludHJvZHVjZSBtdWNoIGNvbXBsZXhpdGllcywgcGVyc29uYWxseSBJJ20gbm90IHF1aXRlIGxp
a2UNCj4gPiB0aGF0Lg0KPiANCj4gSSBleHBlY3QgNTAtMTUwIGxpbmVzIG9mIGV4dHJhIGNvZGUu
DQo+IA0KDQpIb3BlIHRoYXQgY291bGQgYmUgdHJ1ZS4NCkRvIHlvdSBoYXZlIHN1Z2dlc3Rpb24g
b24gaG93IHRvIGtlZXAgdHdvIHR5cGUgcHJvdG9jb2wgY28tZXhpc3QNCklmIHlvdSB0aG91Z2h0
IHRoYXQgd291bGQgd29yayB3aXRob3V0IHR3byBtdWNoIGV4dHJhIGNvbXBsZXhpdHk/DQpIYXZl
IHlvdSB0cmllZCBpdCBhbHJlYWR5IGJhc2VkIHRoaXMgZHJpdmVyPw0KDQpSZWdhcmRzDQpEb25n
IEFpc2hlbmcNCg0KPiA+IFJlZ2FyZHMNCj4gPiBEb25nIEFpc2hlbmcNCj4gPg0KPiA+PiArDQo+
ID4+ICtzdGF0aWMgaW50IGlteF9tdV9zdGFydHVwKHN0cnVjdCBtYm94X2NoYW4gKmNoYW4pIHsN
Cj4gPj4gKwlzdHJ1Y3QgaW14X211X3ByaXYgKnByaXYgPSB0b19pbXhfbXVfcHJpdihjaGFuLT5t
Ym94KTsNCj4gPj4gKwlzdHJ1Y3QgaW14X211X2Nvbl9wcml2ICpjcCA9IGNoYW4tPmNvbl9wcml2
Ow0KPiA+PiArCWludCByZXQ7DQo+ID4+ICsNCj4gPj4gKwlyZXQgPSByZXF1ZXN0X2lycShjcC0+
aXJxLCBpbXhfbXVfaXNyLA0KPiA+PiArCQkJICBJUlFGX1NIQVJFRCwgImlteF9tdV9jaGFuIiwg
Y2hhbik7DQo+ID4+ICsJaWYgKHJldCkgew0KPiA+PiArCQlkZXZfZXJyKGNoYW4tPm1ib3gtPmRl
diwNCj4gPj4gKwkJCSJVbmFibGUgdG8gYWNxdWlyZSBJUlEgJWRcbiIsIGNwLT5pcnEpOw0KPiA+
PiArCQlyZXR1cm4gcmV0Ow0KPiA+PiArCX0NCj4gPj4gKw0KPiA+PiArCWlteF9tdV9ybXcocHJp
diwgSU1YX01VX3hDUiwgSU1YX01VX3hDUl9SSUVuKGNwLT5iaWR4KSwgMCk7DQo+ID4+ICsNCj4g
Pj4gKwlyZXR1cm4gMDsNCj4gPj4gK30NCj4gPj4gKw0KPiA+PiArc3RhdGljIHZvaWQgaW14X211
X3NodXRkb3duKHN0cnVjdCBtYm94X2NoYW4gKmNoYW4pIHsNCj4gPj4gKwlzdHJ1Y3QgaW14X211
X3ByaXYgKnByaXYgPSB0b19pbXhfbXVfcHJpdihjaGFuLT5tYm94KTsNCj4gPj4gKwlzdHJ1Y3Qg
aW14X211X2Nvbl9wcml2ICpjcCA9IGNoYW4tPmNvbl9wcml2Ow0KPiA+PiArDQo+ID4+ICsJaW14
X211X3Jtdyhwcml2LCBJTVhfTVVfeENSLCAwLA0KPiA+PiArCQkgICBJTVhfTVVfeENSX1RJRW4o
Y3AtPmJpZHgpIHwgSU1YX01VX3hDUl9SSUVuKGNwLQ0KPiA+Pj4gYmlkeCkpOw0KPiA+PiArDQo+
ID4+ICsJZnJlZV9pcnEoY3AtPmlycSwgY2hhbik7DQo+ID4+ICt9DQo+ID4+ICsNCj4gPj4gK3N0
YXRpYyBjb25zdCBzdHJ1Y3QgbWJveF9jaGFuX29wcyBpbXhfbXVfb3BzID0gew0KPiA+PiArCS5z
ZW5kX2RhdGEgPSBpbXhfbXVfc2VuZF9kYXRhLA0KPiA+PiArCS5zdGFydHVwID0gaW14X211X3N0
YXJ0dXAsDQo+ID4+ICsJLnNodXRkb3duID0gaW14X211X3NodXRkb3duLA0KPiA+PiArfTsNCj4g
Pj4gKw0KPiA+PiArc3RhdGljIGludCBpbXhfbXVfcHJvYmUoc3RydWN0IHBsYXRmb3JtX2Rldmlj
ZSAqcGRldikgew0KPiA+PiArCXN0cnVjdCBkZXZpY2UgKmRldiA9ICZwZGV2LT5kZXY7DQo+ID4+
ICsJc3RydWN0IHJlc291cmNlICppb21lbTsNCj4gPj4gKwlzdHJ1Y3QgaW14X211X3ByaXYgKnBy
aXY7DQo+ID4+ICsJY29uc3Qgc3RydWN0IGlteF9tdV9jZmcgKmRjZmc7DQo+ID4+ICsJdW5zaWdu
ZWQgaW50IGksIGNoYW5zOw0KPiA+PiArCWludCBpcnEsIHJldDsNCj4gPj4gKw0KPiA+PiArCXBy
aXYgPSBkZXZtX2t6YWxsb2MoZGV2LCBzaXplb2YoKnByaXYpLCBHRlBfS0VSTkVMKTsNCj4gPj4g
KwlpZiAoIXByaXYpDQo+ID4+ICsJCXJldHVybiAtRU5PTUVNOw0KPiA+PiArDQo+ID4+ICsJZGNm
ZyA9IG9mX2RldmljZV9nZXRfbWF0Y2hfZGF0YShkZXYpOw0KPiA+PiArCWlmICghZGNmZykNCj4g
Pj4gKwkJcmV0dXJuIC1FSU5WQUw7DQo+ID4+ICsNCj4gPj4gKwlwcml2LT5kY2ZnID0gZGNmZzsN
Cj4gPj4gKwlwcml2LT5kZXYgPSBkZXY7DQo+ID4+ICsNCj4gPj4gKwlpb21lbSA9IHBsYXRmb3Jt
X2dldF9yZXNvdXJjZShwZGV2LCBJT1JFU09VUkNFX01FTSwgMCk7DQo+ID4+ICsJcHJpdi0+YmFz
ZSA9IGRldm1faW9yZW1hcF9yZXNvdXJjZSgmcGRldi0+ZGV2LCBpb21lbSk7DQo+ID4+ICsJaWYg
KElTX0VSUihwcml2LT5iYXNlKSkNCj4gPj4gKwkJcmV0dXJuIFBUUl9FUlIocHJpdi0+YmFzZSk7
DQo+ID4+ICsNCj4gPj4gKwlpcnEgPSBwbGF0Zm9ybV9nZXRfaXJxKHBkZXYsIDApOw0KPiA+PiAr
CWlmIChpcnEgPD0gMCkNCj4gPj4gKwkJcmV0dXJuIGlycSA8IDAgPyBpcnEgOiAtRUlOVkFMOw0K
PiA+PiArDQo+ID4+ICsJcHJpdi0+Y2xrID0gZGV2bV9jbGtfZ2V0KGRldiwgTlVMTCk7DQo+ID4+
ICsJaWYgKElTX0VSUihwcml2LT5jbGspKSB7DQo+ID4+ICsJCWlmIChQVFJfRVJSKHByaXYtPmNs
aykgPT0gLUVOT0VOVCkgew0KPiA+PiArCQkJcHJpdi0+Y2xrID0gTlVMTDsNCj4gPj4gKwkJfSBl
bHNlIHsNCj4gPj4gKwkJCWRldl9lcnIoZGV2LCAiRmFpbGVkIHRvIGdldCBjbG9ja1xuIik7DQo+
ID4+ICsJCQlyZXR1cm4gUFRSX0VSUihwcml2LT5jbGspOw0KPiA+PiArCQl9DQo+ID4+ICsJfQ0K
PiA+PiArDQo+ID4+ICsJcmV0ID0gY2xrX3ByZXBhcmVfZW5hYmxlKHByaXYtPmNsayk7DQo+ID4+
ICsJaWYgKHJldCkgew0KPiA+PiArCQlkZXZfZXJyKGRldiwgIkZhaWxlZCB0byBlbmFibGUgY2xv
Y2tcbiIpOw0KPiA+PiArCQlyZXR1cm4gcmV0Ow0KPiA+PiArCX0NCj4gPj4gKw0KPiA+PiArCWNo
YW5zID0gbWluKGRjZmctPmNoYW5zLCBJTVhfTVVfTUFYX0NIQU5TKTsNCj4gPj4gKwkvKiBJbml0
aWFsaXplIGNoYW5uZWwgaWRlbnRpZmllcnMgKi8NCj4gPj4gKwlmb3IgKGkgPSAwOyBpIDwgY2hh
bnM7IGkrKykgew0KPiA+PiArCQlzdHJ1Y3QgaW14X211X2Nvbl9wcml2ICpjcCA9ICZwcml2LT5j
b25fcHJpdltpXTsNCj4gPj4gKw0KPiA+PiArCQljcC0+YmlkeCA9IDMgLSBpOw0KPiA+PiArCQlj
cC0+aWR4ID0gaTsNCj4gPj4gKwkJY3AtPmlycSA9IGlycTsNCj4gPj4gKwkJcHJpdi0+bWJveF9j
aGFuc1tpXS5jb25fcHJpdiA9IGNwOw0KPiA+PiArCX0NCj4gPj4gKw0KPiA+PiArCXByaXYtPm1i
b3guZGV2ID0gZGV2Ow0KPiA+PiArCXByaXYtPm1ib3gub3BzID0gJmlteF9tdV9vcHM7DQo+ID4+
ICsJcHJpdi0+bWJveC5jaGFucyA9IHByaXYtPm1ib3hfY2hhbnM7DQo+ID4+ICsJcHJpdi0+bWJv
eC5udW1fY2hhbnMgPSBjaGFuczsNCj4gPj4gKwlwcml2LT5tYm94LnR4ZG9uZV9pcnEgPSB0cnVl
Ow0KPiA+PiArDQo+ID4+ICsJcGxhdGZvcm1fc2V0X2RydmRhdGEocGRldiwgcHJpdik7DQo+ID4+
ICsNCj4gPj4gKwlpZiAocHJpdi0+ZGNmZy0+aW5pdF9odykNCj4gPj4gKwkJcHJpdi0+ZGNmZy0+
aW5pdF9odyhwcml2KTsNCj4gPj4gKw0KPiA+PiArCXJldHVybiBtYm94X2NvbnRyb2xsZXJfcmVn
aXN0ZXIoJnByaXYtPm1ib3gpOw0KPiA+PiArfQ0KPiA+PiArDQo+ID4+ICtzdGF0aWMgaW50IGlt
eF9tdV9yZW1vdmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikgew0KPiA+PiArCXN0cnVj
dCBpbXhfbXVfcHJpdiAqcHJpdiA9IHBsYXRmb3JtX2dldF9kcnZkYXRhKHBkZXYpOw0KPiA+PiAr
DQo+ID4+ICsJbWJveF9jb250cm9sbGVyX3VucmVnaXN0ZXIoJnByaXYtPm1ib3gpOw0KPiA+PiAr
CWNsa19kaXNhYmxlX3VucHJlcGFyZShwcml2LT5jbGspOw0KPiA+PiArDQo+ID4+ICsJcmV0dXJu
IDA7DQo+ID4+ICt9DQo+ID4+ICsNCj4gPj4gKw0KPiA+PiArc3RhdGljIHZvaWQgaW14X211X2lu
aXRfaW14N2RfYShzdHJ1Y3QgaW14X211X3ByaXYgKnByaXYpIHsNCj4gPj4gKwkvKiBTZXQgZGVm
YXVsdCBjb25maWcgKi8NCj4gPj4gKwlpbXhfbXVfd3JpdGUocHJpdiwgMCwgSU1YX01VX3hDUik7
DQo+ID4+ICt9DQo+ID4+ICsNCj4gPj4gK3N0YXRpYyBjb25zdCBzdHJ1Y3QgaW14X211X2NmZyBp
bXhfbXVfY2ZnX2lteDdkX2EgPSB7DQo+ID4+ICsJLmNoYW5zID0gSU1YX01VX01BWF9DSEFOUywN
Cj4gPj4gKwkuaW5pdF9odyA9IGlteF9tdV9pbml0X2lteDdkX2EsDQo+ID4+ICt9Ow0KPiA+PiAr
DQo+ID4+ICtzdGF0aWMgY29uc3Qgc3RydWN0IGlteF9tdV9jZmcgaW14X211X2NmZ19pbXg3ZF9i
ID0gew0KPiA+PiArCS5jaGFucyA9IElNWF9NVV9NQVhfQ0hBTlMsDQo+ID4+ICt9Ow0KPiA+PiAr
DQo+ID4+ICtzdGF0aWMgY29uc3Qgc3RydWN0IG9mX2RldmljZV9pZCBpbXhfbXVfZHRfaWRzW10g
PSB7DQo+ID4+ICsJeyAuY29tcGF0aWJsZSA9ICJmc2wsaW14N3MtbXUtYSIsIC5kYXRhID0gJmlt
eF9tdV9jZmdfaW14N2RfYSB9LA0KPiA+PiArCXsgLmNvbXBhdGlibGUgPSAiZnNsLGlteDdzLW11
LWIiLCAuZGF0YSA9ICZpbXhfbXVfY2ZnX2lteDdkX2IgfSwNCj4gPj4gKwl7IH0sDQo+ID4+ICt9
Ow0KPiA+PiArTU9EVUxFX0RFVklDRV9UQUJMRShvZiwgaW14X211X2R0X2lkcyk7DQo+ID4+ICsN
Cj4gPj4gK3N0YXRpYyBzdHJ1Y3QgcGxhdGZvcm1fZHJpdmVyIGlteF9tdV9kcml2ZXIgPSB7DQo+
ID4+ICsJLnByb2JlCQk9IGlteF9tdV9wcm9iZSwNCj4gPj4gKwkucmVtb3ZlCQk9IGlteF9tdV9y
ZW1vdmUsDQo+ID4+ICsJLmRyaXZlciA9IHsNCj4gPj4gKwkJLm5hbWUJPSAiaW14X211IiwNCj4g
Pj4gKwkJLm9mX21hdGNoX3RhYmxlID0gaW14X211X2R0X2lkcywNCj4gPj4gKwl9LA0KPiA+PiAr
fTsNCj4gPj4gK21vZHVsZV9wbGF0Zm9ybV9kcml2ZXIoaW14X211X2RyaXZlcik7DQo+ID4+ICsN
Cj4gPj4gK01PRFVMRV9BVVRIT1IoIk9sZWtzaWogUmVtcGVsIDxvLnJlbXBlbEBwZW5ndXRyb25p
eC5kZT4iKTsNCj4gPj4gK01PRFVMRV9ERVNDUklQVElPTigiTWVzc2FnZSBVbml0IGRyaXZlciBm
b3IgaS5NWCIpOw0KPiA+PiBNT0RVTEVfTElDRU5TRSgiR1BMDQo+ID4+ICt2MiIpOw0KPiA+PiAt
LQ0KPiA+PiAyLjE3LjENCj4gPg0KPiA+DQo+ID4NCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Oleksij Rempel July 2, 2018, 7:35 a.m. UTC | #4

On 26.06.2018 14:07, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:[e-mail]]
>> Sent: Tuesday, June 26, 2018 6:56 PM
>> To: A.s. Dong <[e-mail]>; Shawn Guo
>> <[e-mail]>; Fabio Estevam <[e-mail]>; Rob
>> Herring <[e-mail]>; Mark Rutland <[e-mail]>
>> Cc: [e-mail]; dl-linux-imx <[e-mail]>; linux-
>> [e-mail]; [e-mail]; linux-
>> [e-mail]
>> Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
>>
>>
>>
>> On 26.06.2018 12:09, A.s. Dong wrote:
>>>> -----Original Message-----
>>>> From: Oleksij Rempel [mailto:[e-mail]]
>>>> Sent: Friday, June 15, 2018 5:51 PM
>>>> To: Shawn Guo <[e-mail]>; Fabio Estevam
>>>> <[e-mail]>; Rob Herring <[e-mail]>; Mark
>>>> Rutland <[e-mail]>; A.s. Dong <[e-mail]>
>>>> Cc: Oleksij Rempel <[e-mail]>; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> linux- [e-mail]; dl-linux-imx <[e-mail]>
>>>> Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging
>>>> unit
>>>>
>>>> The Mailbox controller is able to send messages (up to 4 32 bit
>>>> words) between the endpoints.
>>>>
>>>> This driver was tested using the mailbox-test driver sending messages
>>>> between the Cortex-A7 and the Cortex-M4.
>>>>
>>>> Signed-off-by: Oleksij Rempel <[e-mail]>
>>>> ---
>>>>  drivers/mailbox/Kconfig       |   6 +
>>>>  drivers/mailbox/Makefile      |   2 +
>>>>  drivers/mailbox/imx-mailbox.c | 288
>>>> ++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 296 insertions(+)
>>>>  create mode 100644 drivers/mailbox/imx-mailbox.c
>>>>
>>>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
>>>> a2bb27446dce..e1d2738a2e4c 100644
>>>> --- a/drivers/mailbox/Kconfig
>>>> +++ b/drivers/mailbox/Kconfig
>>>> @@ -15,6 +15,12 @@ config ARM_MHU
>>>>  	  The controller has 3 mailbox channels, the last of which can be
>>>>  	  used in Secure mode only.
>>>>
>>>> +config IMX_MBOX
>>>> +	tristate "iMX Mailbox"
>>>> +	depends on SOC_IMX7D || COMPILE_TEST
>>>> +	help
>>>> +	  Mailbox implementation for iMX7D Messaging Unit (MU).
>>>> +
>>>>  config PLATFORM_MHU
>>>>  	tristate "Platform MHU Mailbox"
>>>>  	depends on OF
>>>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>>>> index
>>>> cc23c3a43fcd..ba2fe1b6dd62 100644
>>>> --- a/drivers/mailbox/Makefile
>>>> +++ b/drivers/mailbox/Makefile
>>>> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
>>>>
>>>>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
>>>>
>>>> +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
>>>> +
>>>>  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
>>>>
>>>>  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
>>>> diff --git a/drivers/mailbox/imx-mailbox.c
>>>> b/drivers/mailbox/imx-mailbox.c new file mode 100644 index
>>>> 000000000000..e3f621cb1d30
>>>> --- /dev/null
>>>> +++ b/drivers/mailbox/imx-mailbox.c
>>>> @@ -0,0 +1,288 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
>>>> +<[e-mail]>  */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/mailbox_controller.h> #include <linux/module.h>
>>>> +#include <linux/of_device.h>
>>>> +
>>>> +/* Transmit Register */
>>>> +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
>>>> +/* Receive Register */
>>>> +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
>>>> +/* Status Register */
>>>> +#define IMX_MU_xSR		0x20
>>>> +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
>>>> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
>>>> +#define IMX_MU_xSR_BRDIP	BIT(9)
>>>> +
>>>> +/* Control Register */
>>>> +#define IMX_MU_xCR		0x24
>>>> +/* Transmit Interrupt Enable */
>>>> +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
>>>> +/* Receive Interrupt Enable */
>>>> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
>>>> +
>>>> +#define IMX_MU_MAX_CHANS	4u
>>>> +
>>>> +struct imx_mu_priv;
>>>> +
>>>> +struct imx_mu_cfg {
>>>> +	unsigned int		chans;
>>>> +	void (*init_hw)(struct imx_mu_priv *priv); };
>>>> +
>>>> +struct imx_mu_con_priv {
>>>> +	int			irq;
>>>> +	unsigned int		bidx;
>>>> +	unsigned int		idx;
>>>> +};
>>>> +
>>>> +struct imx_mu_priv {
>>>> +	struct device		*dev;
>>>> +	const struct imx_mu_cfg	*dcfg;
>>>> +	void __iomem		*base;
>>>> +
>>>> +	struct mbox_controller	mbox;
>>>> +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
>>>> +
>>>> +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
>>>> +	struct clk		*clk;
>>>> +};
>>>> +
>>>> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller
>>>> +*mbox) {
>>>> +	return container_of(mbox, struct imx_mu_priv, mbox); }
>>>> +
>>>> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
>>>> +	iowrite32(val, priv->base + offs);
>>>> +}
>>>> +
>>>> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
>>>> +	return ioread32(priv->base + offs); }
>>>> +
>>>> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set,
>>>> +u32
>>>> +clr) {
>>>> +	u32 val;
>>>> +
>>>> +	val = imx_mu_read(priv, offs);
>>>> +	val &= ~clr;
>>>> +	val |= set;
>>>> +	imx_mu_write(priv, val, offs);
>>>> +
>>>> +	return val;
>>>> +}
>>>> +
>>>> +static irqreturn_t imx_mu_isr(int irq, void *p) {
>>>> +	struct mbox_chan *chan = p;
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +
>>>> +	u32 val, dat;
>>>> +
>>>> +	val = imx_mu_read(priv, IMX_MU_xSR);
>>>> +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
>>>> +	if (!val)
>>>> +		return IRQ_NONE;
>>>> +
>>>> +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
>>>
>>> I'm wondering whether this can work properly for multi consumers at
>>> the same time.
>>> Because xSR_TEn is 1 by default and four virtual channels actually are
>>> using the same interrupt. That means channel 1 interrupt may cause
>>> channel 2 believe it's txdone.
>>> Have we tested such using?
>>
>> see imx_mu_send_data()
>> ..... imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
>>
>>>
>>>> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
>>>>> bidx));
>>
>> and here ^^^
>> TX status is enabled only for send and disabled as soon at transfer is done.
>>
> 
> That controls the interrupt enable signal, I'm not sure if the status bit
> Won't be set if not enable interrupt. I've not tested it.
> 
> Have you double checked it?
> 
> For n = {0, 1, 2, 3} Processor A Transmit Register n Empty. (Read-only)
> • The TEn bit is set to “1” after the BRRn register is read on the Processor B-side.
> • After the TEn bit is set to “1”, the TEn bit signals the Processor A-side that the ATRn register is
> ready to be written on the Processor A-side, and a Transmit n interrupt is issued on the Processor
> A-side (if the TEn bit in the ACR register is set to “1”).

You are right.
> mdw phys 0x30aa0024 # read control register
0x30aa0024: 08000000  # onyl RIE for channel 0 is enabled
> mdw phys 0x30aa0020 # read status register
0x30aa0020: 00f00200  # all TE are 1

So i need to compare Control with Status regs to see if we expect an
Interrupt on for the TX.

> 
>>>> +		mbox_chan_txdone(chan, 0);
>>>> +	}
>>>> +
>>>> +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
>>>> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
>>>> +		mbox_chan_received_data(chan, (void *)&dat);
>>>> +	}
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +	u32 val;
>>>> +
>>>> +	val = imx_mu_read(priv, IMX_MU_xSR);
>>>> +	/* test if transmit register is empty */
>>>> +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
>>>> +
>>>> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +	u32 *arg = data;
>>>> +
>>>> +	if (!imx_mu_last_tx_done(chan))
>>>> +		return -EBUSY;
>>>> +
>>>> +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
>>>> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> Since Sascha is requesting to write a generic MU mailbox driver for
>>> both SCU MU and M4 case, the current way using virtual channels in
>>> this patch only send one word a time obviously can't fit for SCU MU clients
>> well.
>>> Do you think if we can refer to TI case to design a generic data
>>> transfer protocol to allow send multi words which is more close to SCU?
>>
>> According to your code, you are able to send 1 word message. It means, your
>> SCU is configured to trigger an interrupt or status update if REG0 was written.
>> The same is true for 2, 3, 4 and 5 word messages.
>>
> 
> SCU is interrupt driven already for the first word.
> We do can send word one by one but the performance would be terrible
> comparing to write 4 a time.
> 
>> If the MU configuration would look like you it described, you would be forced
>> to write always 4 words, even for 1 word message.
>>
>>> include/linux/soc/ti/ti-msgmgr.h
>>> struct ti_msgmgr_message {
>>>         size_t len;
>>>         u8 *buf;
>>> };
>>>
>>> Or we try to support both type transfer protocols in this driver?
>>
>> Sure. ti-msgmgr.c is a good example. You will probably need reduced variant
>> of it. It is generic enough to make it useful not only for SCU.
>>
> 
> Sascha needs a common design for both M4 and SCU.If decide to do that,
> you probably need update this patch as well.
> 
> But even doing like TI style, it still need hack for SCU as the data size offset
> Is different. However, that would be a much smaller hack than doing based
> On this driver.
> 
>>> That may introduce much complexities, personally I'm not quite like
>>> that.
>>
>> I expect 50-150 lines of extra code.
>>
> 
> Hope that could be true.
> Do you have suggestion on how to keep two type protocol co-exist
> If you thought that would work without two much extra complexity?
> Have you tried it already based this driver?
> 
> Regards
> Dong Aisheng
> 
>>> Regards
>>> Dong Aisheng
>>>
>>>> +
>>>> +static int imx_mu_startup(struct mbox_chan *chan) {
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +	int ret;
>>>> +
>>>> +	ret = request_irq(cp->irq, imx_mu_isr,
>>>> +			  IRQF_SHARED, "imx_mu_chan", chan);
>>>> +	if (ret) {
>>>> +		dev_err(chan->mbox->dev,
>>>> +			"Unable to acquire IRQ %d\n", cp->irq);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void imx_mu_shutdown(struct mbox_chan *chan) {
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +
>>>> +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
>>>> +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
>>>>> bidx));
>>>> +
>>>> +	free_irq(cp->irq, chan);
>>>> +}
>>>> +
>>>> +static const struct mbox_chan_ops imx_mu_ops = {
>>>> +	.send_data = imx_mu_send_data,
>>>> +	.startup = imx_mu_startup,
>>>> +	.shutdown = imx_mu_shutdown,
>>>> +};
>>>> +
>>>> +static int imx_mu_probe(struct platform_device *pdev) {
>>>> +	struct device *dev = &pdev->dev;
>>>> +	struct resource *iomem;
>>>> +	struct imx_mu_priv *priv;
>>>> +	const struct imx_mu_cfg *dcfg;
>>>> +	unsigned int i, chans;
>>>> +	int irq, ret;
>>>> +
>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	dcfg = of_device_get_match_data(dev);
>>>> +	if (!dcfg)
>>>> +		return -EINVAL;
>>>> +
>>>> +	priv->dcfg = dcfg;
>>>> +	priv->dev = dev;
>>>> +
>>>> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
>>>> +	if (IS_ERR(priv->base))
>>>> +		return PTR_ERR(priv->base);
>>>> +
>>>> +	irq = platform_get_irq(pdev, 0);
>>>> +	if (irq <= 0)
>>>> +		return irq < 0 ? irq : -EINVAL;
>>>> +
>>>> +	priv->clk = devm_clk_get(dev, NULL);
>>>> +	if (IS_ERR(priv->clk)) {
>>>> +		if (PTR_ERR(priv->clk) == -ENOENT) {
>>>> +			priv->clk = NULL;
>>>> +		} else {
>>>> +			dev_err(dev, "Failed to get clock\n");
>>>> +			return PTR_ERR(priv->clk);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	ret = clk_prepare_enable(priv->clk);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Failed to enable clock\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
>>>> +	/* Initialize channel identifiers */
>>>> +	for (i = 0; i < chans; i++) {
>>>> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
>>>> +
>>>> +		cp->bidx = 3 - i;
>>>> +		cp->idx = i;
>>>> +		cp->irq = irq;
>>>> +		priv->mbox_chans[i].con_priv = cp;
>>>> +	}
>>>> +
>>>> +	priv->mbox.dev = dev;
>>>> +	priv->mbox.ops = &imx_mu_ops;
>>>> +	priv->mbox.chans = priv->mbox_chans;
>>>> +	priv->mbox.num_chans = chans;
>>>> +	priv->mbox.txdone_irq = true;
>>>> +
>>>> +	platform_set_drvdata(pdev, priv);
>>>> +
>>>> +	if (priv->dcfg->init_hw)
>>>> +		priv->dcfg->init_hw(priv);
>>>> +
>>>> +	return mbox_controller_register(&priv->mbox);
>>>> +}
>>>> +
>>>> +static int imx_mu_remove(struct platform_device *pdev) {
>>>> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
>>>> +
>>>> +	mbox_controller_unregister(&priv->mbox);
>>>> +	clk_disable_unprepare(priv->clk);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +
>>>> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
>>>> +	/* Set default config */
>>>> +	imx_mu_write(priv, 0, IMX_MU_xCR);
>>>> +}
>>>> +
>>>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
>>>> +	.chans = IMX_MU_MAX_CHANS,
>>>> +	.init_hw = imx_mu_init_imx7d_a,
>>>> +};
>>>> +
>>>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
>>>> +	.chans = IMX_MU_MAX_CHANS,
>>>> +};
>>>> +
>>>> +static const struct of_device_id imx_mu_dt_ids[] = {
>>>> +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
>>>> +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
>>>> +	{ },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
>>>> +
>>>> +static struct platform_driver imx_mu_driver = {
>>>> +	.probe		= imx_mu_probe,
>>>> +	.remove		= imx_mu_remove,
>>>> +	.driver = {
>>>> +		.name	= "imx_mu",
>>>> +		.of_match_table = imx_mu_dt_ids,
>>>> +	},
>>>> +};
>>>> +module_platform_driver(imx_mu_driver);
>>>> +
>>>> +MODULE_AUTHOR("Oleksij Rempel <[e-mail]>");
>>>> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
>>>> MODULE_LICENSE("GPL
>>>> +v2");
>>>> --
>>>> 2.17.1
>>>
>>>
>>>
>

Aisheng Dong July 12, 2018, 11:28 a.m. UTC | #5

Hi Oleksij,

> -----Original Message-----
> From: Oleksij Rempel [mailto:[e-mail]]
> Sent: Friday, June 15, 2018 5:51 PM
> To: Shawn Guo <[e-mail]>; Fabio Estevam
> <[e-mail]>; Rob Herring <[e-mail]>; Mark
> Rutland <[e-mail]>; A.s. Dong <[e-mail]>
> Cc: Oleksij Rempel <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; dl-linux-imx <[e-mail]>
> Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
> 

This is not correct according to current implementation as we abstract them
into 4 virtual channels while each 'channel' can send only one word one time.
We probably need explain such limitation in commit message as well.

I'm not strongly against this way. But it makes the controller lose the HW
capability to send up to 4 words. I'd just like to know a bit history or reason
why we decided to do that. Do we design it for specific users case for M4?
And are we assuming there will be no real users of multi words send requirement?

> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.
> 
> Signed-off-by: Oleksij Rempel <[e-mail]>
> ---
>  drivers/mailbox/Kconfig       |   6 +
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/imx-mailbox.c | 288
> ++++++++++++++++++++++++++++++++++
>  3 files changed, 296 insertions(+)
>  create mode 100644 drivers/mailbox/imx-mailbox.c
> 
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> a2bb27446dce..e1d2738a2e4c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,12 @@ config ARM_MHU
>  	  The controller has 3 mailbox channels, the last of which can be
>  	  used in Secure mode only.
> 
> +config IMX_MBOX
> +	tristate "iMX Mailbox"
> +	depends on SOC_IMX7D || COMPILE_TEST

Better change to ARCH_MXC as other platform does.

> +	help
> +	  Mailbox implementation for iMX7D Messaging Unit (MU).

Ditto

> +
>  config PLATFORM_MHU
>  	tristate "Platform MHU Mailbox"
>  	depends on OF
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> cc23c3a43fcd..ba2fe1b6dd62 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> 
>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> 
> +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> +
>  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> 
>  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> new file mode 100644 index 000000000000..e3f621cb1d30
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> +<[e-mail]>  */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +/* Transmit Register */
> +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> +/* Receive Register */
> +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> +/* Status Register */
> +#define IMX_MU_xSR		0x20
> +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> +#define IMX_MU_xSR_BRDIP	BIT(9)
> +
> +/* Control Register */
> +#define IMX_MU_xCR		0x24
> +/* Transmit Interrupt Enable */
> +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> +
> +#define IMX_MU_MAX_CHANS	4u
> +
> +struct imx_mu_priv;
> +
> +struct imx_mu_cfg {
> +	unsigned int		chans;
> +	void (*init_hw)(struct imx_mu_priv *priv); };
> +
> +struct imx_mu_con_priv {
> +	int			irq;
> +	unsigned int		bidx;
> +	unsigned int		idx;
> +};
> +
> +struct imx_mu_priv {
> +	struct device		*dev;
> +	const struct imx_mu_cfg	*dcfg;
> +	void __iomem		*base;
> +
> +	struct mbox_controller	mbox;
> +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> +
> +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> +	struct clk		*clk;
> +};
> +
> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
> +{
> +	return container_of(mbox, struct imx_mu_priv, mbox); }
> +
> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> +	iowrite32(val, priv->base + offs);
> +}
> +
> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> +	return ioread32(priv->base + offs);
> +}
> +
> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32
> +clr) {
> +	u32 val;
> +
> +	val = imx_mu_read(priv, offs);
> +	val &= ~clr;
> +	val |= set;
> +	imx_mu_write(priv, val, offs);
> +
> +	return val;
> +}
> +
> +static irqreturn_t imx_mu_isr(int irq, void *p) {
> +	struct mbox_chan *chan = p;
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	u32 val, dat;
> +
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> >bidx));
> +		mbox_chan_txdone(chan, 0);
> +	}
> +
> +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> +		mbox_chan_received_data(chan, (void *)&dat);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	u32 val;
> +
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +	/* test if transmit register is empty */
> +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> +
> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	u32 *arg = data;
> +
> +	if (!imx_mu_last_tx_done(chan))
> +		return -EBUSY;
> +
> +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> +
> +	return 0;
> +}
> +
> +static int imx_mu_startup(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	int ret;
> +
> +	ret = request_irq(cp->irq, imx_mu_isr,
> +			  IRQF_SHARED, "imx_mu_chan", chan);

I guess no need to assign the irq for each cp as we have only one irq.

> +	if (ret) {
> +		dev_err(chan->mbox->dev,
> +			"Unable to acquire IRQ %d\n", cp->irq);
> +		return ret;
> +	}
> +
> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> +
> +	return 0;
> +}
> +
> +static void imx_mu_shutdown(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> >bidx));
> +
> +	free_irq(cp->irq, chan);
> +}
> +
> +static const struct mbox_chan_ops imx_mu_ops = {
> +	.send_data = imx_mu_send_data,
> +	.startup = imx_mu_startup,
> +	.shutdown = imx_mu_shutdown,
> +};
> +
> +static int imx_mu_probe(struct platform_device *pdev) {
> +	struct device *dev = &pdev->dev;
> +	struct resource *iomem;
> +	struct imx_mu_priv *priv;
> +	const struct imx_mu_cfg *dcfg;
> +	unsigned int i, chans;
> +	int irq, ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dcfg = of_device_get_match_data(dev);
> +	if (!dcfg)
> +		return -EINVAL;
> +
> +	priv->dcfg = dcfg;
> +	priv->dev = dev;
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq <= 0)
> +		return irq < 0 ? irq : -EINVAL;
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		if (PTR_ERR(priv->clk) == -ENOENT) {
> +			priv->clk = NULL;
> +		} else {
> +			dev_err(dev, "Failed to get clock\n");

I guess we may not need print it for DEFER_PROBE error case.

> +			return PTR_ERR(priv->clk);
> +		}
> +	}
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> +	/* Initialize channel identifiers */
> +	for (i = 0; i < chans; i++) {
> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> +		cp->bidx = 3 - i;

We may not need it if we improve the macro to calculate bidx by idx?

> +		cp->idx = i;
> +		cp->irq = irq;
> +		priv->mbox_chans[i].con_priv = cp;
> +	}
> +
> +	priv->mbox.dev = dev;
> +	priv->mbox.ops = &imx_mu_ops;
> +	priv->mbox.chans = priv->mbox_chans;
> +	priv->mbox.num_chans = chans;
> +	priv->mbox.txdone_irq = true;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	if (priv->dcfg->init_hw)
> +		priv->dcfg->init_hw(priv);
> +
> +	return mbox_controller_register(&priv->mbox);
> +}
> +
> +static int imx_mu_remove(struct platform_device *pdev) {
> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> +
> +	mbox_controller_unregister(&priv->mbox);
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +
> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> +	/* Set default config */
> +	imx_mu_write(priv, 0, IMX_MU_xCR);

This will reset both MU Side A and B.
So we may need make sure Side B is initialized after A?

> +}
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> +	.chans = IMX_MU_MAX_CHANS,
> +	.init_hw = imx_mu_init_imx7d_a,
> +};
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> +	.chans = IMX_MU_MAX_CHANS,
> +};
> +
> +static const struct of_device_id imx_mu_dt_ids[] = {
> +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },

I'm not sure whether we already have the decision to use fsl,<soc>-mu compatible
String and use property to specify the mu side.
Can you double check if we can switch to that way?

And would you update the binding doc for M4 support according to the qxp mu one
Which Is already signed by Rob's tag?

Regards
Dong Aisheng

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> +
> +static struct platform_driver imx_mu_driver = {
> +	.probe		= imx_mu_probe,
> +	.remove		= imx_mu_remove,
> +	.driver = {
> +		.name	= "imx_mu",
> +		.of_match_table = imx_mu_dt_ids,
> +	},
> +};
> +module_platform_driver(imx_mu_driver);
> +
> +MODULE_AUTHOR("Oleksij Rempel <[e-mail]>");
> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
> MODULE_LICENSE("GPL
> +v2");
> --
> 2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Oleksij Rempel July 14, 2018, 7:02 a.m. UTC | #6

Hi,

Beside, what is equivalent of name and family name is in your name?
I know it is different in China, so I won't to avoid confusion with:
"Hi $name," format :)

On Thu, Jul 12, 2018 at 11:28:16AM +0000, A.s. Dong wrote:
> Hi Oleksij,
> 
> > -----Original Message-----
> > From: Oleksij Rempel [mailto:[e-mail]]
> > Sent: Friday, June 15, 2018 5:51 PM
> > To: Shawn Guo <[e-mail]>; Fabio Estevam
> > <[e-mail]>; Rob Herring <[e-mail]>; Mark
> > Rutland <[e-mail]>; A.s. Dong <[e-mail]>
> > Cc: Oleksij Rempel <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; dl-linux-imx <[e-mail]>
> > Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> > 
> > The Mailbox controller is able to send messages (up to 4 32 bit words)
> > between the endpoints.
> > 
> 
> This is not correct according to current implementation as we abstract them
> into 4 virtual channels while each 'channel' can send only one word one time.
> We probably need explain such limitation in commit message as well.
> 
> I'm not strongly against this way. But it makes the controller lose the HW
> capability to send up to 4 words. I'd just like to know a bit history or reason
> why we decided to do that. Do we design it for specific users case for M4?

no, it is R&D.

> And are we assuming there will be no real users of multi words send requirement?

no. In my experience, each imaginable Brainfuck configuration will
actually happen some day in some design for $reasons.
So, no assumptions, just currently working configuration of my R&D
project.

> > This driver was tested using the mailbox-test driver sending messages
> > between the Cortex-A7 and the Cortex-M4.
> > 
> > Signed-off-by: Oleksij Rempel <[e-mail]>
> > ---
> >  drivers/mailbox/Kconfig       |   6 +
> >  drivers/mailbox/Makefile      |   2 +
> >  drivers/mailbox/imx-mailbox.c | 288
> > ++++++++++++++++++++++++++++++++++
> >  3 files changed, 296 insertions(+)
> >  create mode 100644 drivers/mailbox/imx-mailbox.c
> > 
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > a2bb27446dce..e1d2738a2e4c 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -15,6 +15,12 @@ config ARM_MHU
> >  	  The controller has 3 mailbox channels, the last of which can be
> >  	  used in Secure mode only.
> > 
> > +config IMX_MBOX
> > +	tristate "iMX Mailbox"
> > +	depends on SOC_IMX7D || COMPILE_TEST
> 
> Better change to ARCH_MXC as other platform does.

ok

> > +	help
> > +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> 
> Ditto

ok

> > +
> >  config PLATFORM_MHU
> >  	tristate "Platform MHU Mailbox"
> >  	depends on OF
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > cc23c3a43fcd..ba2fe1b6dd62 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> > 
> >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > 
> > +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > +
> >  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> > 
> >  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> > diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> > new file mode 100644 index 000000000000..e3f621cb1d30
> > --- /dev/null
> > +++ b/drivers/mailbox/imx-mailbox.c
> > @@ -0,0 +1,288 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> > +<[e-mail]>  */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +
> > +/* Transmit Register */
> > +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> > +/* Receive Register */
> > +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> > +/* Status Register */
> > +#define IMX_MU_xSR		0x20
> > +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> > +#define IMX_MU_xSR_BRDIP	BIT(9)
> > +
> > +/* Control Register */
> > +#define IMX_MU_xCR		0x24
> > +/* Transmit Interrupt Enable */
> > +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> > +
> > +#define IMX_MU_MAX_CHANS	4u
> > +
> > +struct imx_mu_priv;
> > +
> > +struct imx_mu_cfg {
> > +	unsigned int		chans;
> > +	void (*init_hw)(struct imx_mu_priv *priv); };
> > +
> > +struct imx_mu_con_priv {
> > +	int			irq;
> > +	unsigned int		bidx;
> > +	unsigned int		idx;
> > +};
> > +
> > +struct imx_mu_priv {
> > +	struct device		*dev;
> > +	const struct imx_mu_cfg	*dcfg;
> > +	void __iomem		*base;
> > +
> > +	struct mbox_controller	mbox;
> > +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> > +
> > +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> > +	struct clk		*clk;
> > +};
> > +
> > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
> > +{
> > +	return container_of(mbox, struct imx_mu_priv, mbox); }
> > +
> > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> > +	iowrite32(val, priv->base + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> > +	return ioread32(priv->base + offs);
> > +}
> > +
> > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32
> > +clr) {
> > +	u32 val;
> > +
> > +	val = imx_mu_read(priv, offs);
> > +	val &= ~clr;
> > +	val |= set;
> > +	imx_mu_write(priv, val, offs);
> > +
> > +	return val;
> > +}
> > +
> > +static irqreturn_t imx_mu_isr(int irq, void *p) {
> > +	struct mbox_chan *chan = p;
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +
> > +	u32 val, dat;
> > +
> > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> > +	if (!val)
> > +		return IRQ_NONE;
> > +
> > +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> > +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> > >bidx));
> > +		mbox_chan_txdone(chan, 0);
> > +	}
> > +
> > +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> > +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> > +		mbox_chan_received_data(chan, (void *)&dat);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +	u32 val;
> > +
> > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > +	/* test if transmit register is empty */
> > +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> > +
> > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +	u32 *arg = data;
> > +
> > +	if (!imx_mu_last_tx_done(chan))
> > +		return -EBUSY;
> > +
> > +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_mu_startup(struct mbox_chan *chan) {
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +	int ret;
> > +
> > +	ret = request_irq(cp->irq, imx_mu_isr,
> > +			  IRQF_SHARED, "imx_mu_chan", chan);
> 
> I guess no need to assign the irq for each cp as we have only one irq.

all irq chip controller have one sink and number of source limited to
imagination or amount of bits in a register. May be we will need some day to
write a irqchip driver to make it work as chained irq controller.

So, I don't see any technical reason to not do it. Are you?

> > +	if (ret) {
> > +		dev_err(chan->mbox->dev,
> > +			"Unable to acquire IRQ %d\n", cp->irq);
> > +		return ret;
> > +	}
> > +
> > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static void imx_mu_shutdown(struct mbox_chan *chan) {
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +
> > +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> > +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> > >bidx));
> > +
> > +	free_irq(cp->irq, chan);
> > +}
> > +
> > +static const struct mbox_chan_ops imx_mu_ops = {
> > +	.send_data = imx_mu_send_data,
> > +	.startup = imx_mu_startup,
> > +	.shutdown = imx_mu_shutdown,
> > +};
> > +
> > +static int imx_mu_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct resource *iomem;
> > +	struct imx_mu_priv *priv;
> > +	const struct imx_mu_cfg *dcfg;
> > +	unsigned int i, chans;
> > +	int irq, ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	dcfg = of_device_get_match_data(dev);
> > +	if (!dcfg)
> > +		return -EINVAL;
> > +
> > +	priv->dcfg = dcfg;
> > +	priv->dev = dev;
> > +
> > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq <= 0)
> > +		return irq < 0 ? irq : -EINVAL;
> > +
> > +	priv->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(priv->clk)) {
> > +		if (PTR_ERR(priv->clk) == -ENOENT) {
> > +			priv->clk = NULL;
> > +		} else {
> > +			dev_err(dev, "Failed to get clock\n");
> 
> I guess we may not need print it for DEFER_PROBE error case.

ok.

> > +			return PTR_ERR(priv->clk);
> > +		}
> > +	}
> > +
> > +	ret = clk_prepare_enable(priv->clk);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable clock\n");
> > +		return ret;
> > +	}
> > +
> > +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> > +	/* Initialize channel identifiers */
> > +	for (i = 0; i < chans; i++) {
> > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > +
> > +		cp->bidx = 3 - i;
> 
> We may not need it if we improve the macro to calculate bidx by idx?

Are all implementation of NXP MU have reversed bit order?
Will it fit good for one channel implementation?

> > +		cp->idx = i;
> > +		cp->irq = irq;
> > +		priv->mbox_chans[i].con_priv = cp;
> > +	}
> > +
> > +	priv->mbox.dev = dev;
> > +	priv->mbox.ops = &imx_mu_ops;
> > +	priv->mbox.chans = priv->mbox_chans;
> > +	priv->mbox.num_chans = chans;
> > +	priv->mbox.txdone_irq = true;
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	if (priv->dcfg->init_hw)
> > +		priv->dcfg->init_hw(priv);
> > +
> > +	return mbox_controller_register(&priv->mbox);
> > +}
> > +
> > +static int imx_mu_remove(struct platform_device *pdev) {
> > +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> > +
> > +	mbox_controller_unregister(&priv->mbox);
> > +	clk_disable_unprepare(priv->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> > +	/* Set default config */
> > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> 
> This will reset both MU Side A and B.
> So we may need make sure Side B is initialized after A?

I assume it is implementation specific, as soon as it will be needed,
we may introduce extra DT flag. No need to cover all possible cases if
we don't have to.

> > +}
> > +
> > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> > +	.chans = IMX_MU_MAX_CHANS,
> > +	.init_hw = imx_mu_init_imx7d_a,
> > +};
> > +
> > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> > +	.chans = IMX_MU_MAX_CHANS,
> > +};
> > +
> > +static const struct of_device_id imx_mu_dt_ids[] = {
> > +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> > +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> 
> I'm not sure whether we already have the decision to use fsl,<soc>-mu compatible
> String and use property to specify the mu side.
> Can you double check if we can switch to that way?

ok.

> And would you update the binding doc for M4 support according to the qxp mu one
> Which Is already signed by Rob's tag?

ok.

So, should I update my patch set including DT binding documentation
prior to yours?

If yes, can you please contact Rob to avoid confusions.

Aisheng Dong July 14, 2018, 8:49 a.m. UTC | #7

> -----Original Message-----
> From: Oleksij Rempel [mailto:[e-mail]]
> Sent: Saturday, July 14, 2018 3:02 PM
> To: A.s. Dong <[e-mail]>
> Cc: Shawn Guo <[e-mail]>; Fabio Estevam
> <[e-mail]>; Rob Herring <[e-mail]>; Mark
> Rutland <[e-mail]>; [e-mail]; dl-linux-
> imx <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> Hi,
> 
> Beside, what is equivalent of name and family name is in your name?
> I know it is different in China, so I won't to avoid confusion with:
> "Hi $name," format :)
> 

Dong is my family name. Either Hi A.S or Hi Dong is fine to me. :)

> On Thu, Jul 12, 2018 at 11:28:16AM +0000, A.s. Dong wrote:
> > Hi Oleksij,
> >
> > > -----Original Message-----
> > > From: Oleksij Rempel [mailto:[e-mail]]
> > > Sent: Friday, June 15, 2018 5:51 PM
> > > To: Shawn Guo <[e-mail]>; Fabio Estevam
> > > <[e-mail]>; Rob Herring <[e-mail]>; Mark
> > > Rutland <[e-mail]>; A.s. Dong <[e-mail]>
> > > Cc: Oleksij Rempel <[e-mail]>;
> [e-mail];
> > > [e-mail]; [e-mail];
> > > linux- [e-mail]; dl-linux-imx <[e-mail]>
> > > Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging
> > > unit
> > >
> > > The Mailbox controller is able to send messages (up to 4 32 bit
> > > words) between the endpoints.
> > >
> >
> > This is not correct according to current implementation as we abstract
> > them into 4 virtual channels while each 'channel' can send only one word
> one time.
> > We probably need explain such limitation in commit message as well.
> >
> > I'm not strongly against this way. But it makes the controller lose
> > the HW capability to send up to 4 words. I'd just like to know a bit
> > history or reason why we decided to do that. Do we design it for specific
> users case for M4?
> 
> no, it is R&D.
> 

Got it

> > And are we assuming there will be no real users of multi words send
> requirement?
> 
> no. In my experience, each imaginable Brainfuck configuration will actually
> happen some day in some design for $reasons.
> So, no assumptions, just currently working configuration of my R&D project.
> 

I'm fine with as it is currently. We don't have to address all possible
Issues in one time.

> > > This driver was tested using the mailbox-test driver sending
> > > messages between the Cortex-A7 and the Cortex-M4.
> > >
> > > Signed-off-by: Oleksij Rempel <[e-mail]>
> > > ---
> > >  drivers/mailbox/Kconfig       |   6 +
> > >  drivers/mailbox/Makefile      |   2 +
> > >  drivers/mailbox/imx-mailbox.c | 288
> > > ++++++++++++++++++++++++++++++++++
> > >  3 files changed, 296 insertions(+)
> > >  create mode 100644 drivers/mailbox/imx-mailbox.c
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > a2bb27446dce..e1d2738a2e4c 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -15,6 +15,12 @@ config ARM_MHU
> > >  	  The controller has 3 mailbox channels, the last of which can be
> > >  	  used in Secure mode only.
> > >
> > > +config IMX_MBOX
> > > +	tristate "iMX Mailbox"
> > > +	depends on SOC_IMX7D || COMPILE_TEST
> >
> > Better change to ARCH_MXC as other platform does.
> 
> ok
> 
> > > +	help
> > > +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> >
> > Ditto
> 
> ok
> 
> > > +
> > >  config PLATFORM_MHU
> > >  	tristate "Platform MHU Mailbox"
> > >  	depends on OF
> > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > index
> > > cc23c3a43fcd..ba2fe1b6dd62 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> > >
> > >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > >
> > > +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > > +
> > >  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> > >
> > >  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > b/drivers/mailbox/imx-mailbox.c new file mode 100644 index
> > > 000000000000..e3f621cb1d30
> > > --- /dev/null
> > > +++ b/drivers/mailbox/imx-mailbox.c
> > > @@ -0,0 +1,288 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> > > +<[e-mail]>  */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +
> > > +/* Transmit Register */
> > > +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> > > +/* Receive Register */
> > > +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> > > +/* Status Register */
> > > +#define IMX_MU_xSR		0x20
> > > +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> > > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> > > +#define IMX_MU_xSR_BRDIP	BIT(9)
> > > +
> > > +/* Control Register */
> > > +#define IMX_MU_xCR		0x24
> > > +/* Transmit Interrupt Enable */
> > > +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> > > +/* Receive Interrupt Enable */
> > > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> > > +
> > > +#define IMX_MU_MAX_CHANS	4u
> > > +
> > > +struct imx_mu_priv;
> > > +
> > > +struct imx_mu_cfg {
> > > +	unsigned int		chans;
> > > +	void (*init_hw)(struct imx_mu_priv *priv); };
> > > +
> > > +struct imx_mu_con_priv {
> > > +	int			irq;
> > > +	unsigned int		bidx;
> > > +	unsigned int		idx;
> > > +};
> > > +
> > > +struct imx_mu_priv {
> > > +	struct device		*dev;
> > > +	const struct imx_mu_cfg	*dcfg;
> > > +	void __iomem		*base;
> > > +
> > > +	struct mbox_controller	mbox;
> > > +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> > > +
> > > +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> > > +	struct clk		*clk;
> > > +};
> > > +
> > > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller
> > > +*mbox) {
> > > +	return container_of(mbox, struct imx_mu_priv, mbox); }
> > > +
> > > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> > > +	iowrite32(val, priv->base + offs); }
> > > +
> > > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> > > +	return ioread32(priv->base + offs); }
> > > +
> > > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set,
> > > +u32
> > > +clr) {
> > > +	u32 val;
> > > +
> > > +	val = imx_mu_read(priv, offs);
> > > +	val &= ~clr;
> > > +	val |= set;
> > > +	imx_mu_write(priv, val, offs);
> > > +
> > > +	return val;
> > > +}
> > > +
> > > +static irqreturn_t imx_mu_isr(int irq, void *p) {
> > > +	struct mbox_chan *chan = p;
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +
> > > +	u32 val, dat;
> > > +
> > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> > > +	if (!val)
> > > +		return IRQ_NONE;
> > > +
> > > +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> > > +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> > > >bidx));
> > > +		mbox_chan_txdone(chan, 0);
> > > +	}
> > > +
> > > +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> > > +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> > > +		mbox_chan_received_data(chan, (void *)&dat);
> > > +	}
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +	u32 val;
> > > +
> > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > +	/* test if transmit register is empty */
> > > +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> > > +
> > > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +	u32 *arg = data;
> > > +
> > > +	if (!imx_mu_last_tx_done(chan))
> > > +		return -EBUSY;
> > > +
> > > +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int imx_mu_startup(struct mbox_chan *chan) {
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +	int ret;
> > > +
> > > +	ret = request_irq(cp->irq, imx_mu_isr,
> > > +			  IRQF_SHARED, "imx_mu_chan", chan);
> >
> > I guess no need to assign the irq for each cp as we have only one irq.
> 
> all irq chip controller have one sink and number of source limited to
> imagination or amount of bits in a register. May be we will need some day to
> write a irqchip driver to make it work as chained irq controller.
> 

We do need write an irqchip driver later because MU still supports another
four general purpose interrupts which will be used by SCU firmware.
But I don't think it's necessary to abstract them for TX/RX interrutps.
This makes thing simple.

> So, I don't see any technical reason to not do it. Are you?
> 
> > > +	if (ret) {
> > > +		dev_err(chan->mbox->dev,
> > > +			"Unable to acquire IRQ %d\n", cp->irq);
> > > +		return ret;
> > > +	}
> > > +
> > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void imx_mu_shutdown(struct mbox_chan *chan) {
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +
> > > +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> > > +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> > > >bidx));
> > > +
> > > +	free_irq(cp->irq, chan);
> > > +}
> > > +
> > > +static const struct mbox_chan_ops imx_mu_ops = {
> > > +	.send_data = imx_mu_send_data,
> > > +	.startup = imx_mu_startup,
> > > +	.shutdown = imx_mu_shutdown,
> > > +};
> > > +
> > > +static int imx_mu_probe(struct platform_device *pdev) {
> > > +	struct device *dev = &pdev->dev;
> > > +	struct resource *iomem;
> > > +	struct imx_mu_priv *priv;
> > > +	const struct imx_mu_cfg *dcfg;
> > > +	unsigned int i, chans;
> > > +	int irq, ret;
> > > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	dcfg = of_device_get_match_data(dev);
> > > +	if (!dcfg)
> > > +		return -EINVAL;
> > > +
> > > +	priv->dcfg = dcfg;
> > > +	priv->dev = dev;
> > > +
> > > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> > > +	if (IS_ERR(priv->base))
> > > +		return PTR_ERR(priv->base);
> > > +
> > > +	irq = platform_get_irq(pdev, 0);
> > > +	if (irq <= 0)
> > > +		return irq < 0 ? irq : -EINVAL;
> > > +
> > > +	priv->clk = devm_clk_get(dev, NULL);
> > > +	if (IS_ERR(priv->clk)) {
> > > +		if (PTR_ERR(priv->clk) == -ENOENT) {
> > > +			priv->clk = NULL;
> > > +		} else {
> > > +			dev_err(dev, "Failed to get clock\n");
> >
> > I guess we may not need print it for DEFER_PROBE error case.
> 
> ok.
> 
> > > +			return PTR_ERR(priv->clk);
> > > +		}
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(priv->clk);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to enable clock\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> > > +	/* Initialize channel identifiers */
> > > +	for (i = 0; i < chans; i++) {
> > > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > > +
> > > +		cp->bidx = 3 - i;
> >
> > We may not need it if we improve the macro to calculate bidx by idx?
> 
> Are all implementation of NXP MU have reversed bit order?

AFAIK NXP MU library is used for all known platforms with MUs.
So I guess yes.

> Will it fit good for one channel implementation?

If you see the SCU MU patches I sent, you will see I also need
convert the channel index to channel mask. But here you're
using bidx where SCU MU does have. So have a common
channel index to mask macro may be good for both using.

> 
> > > +		cp->idx = i;
> > > +		cp->irq = irq;
> > > +		priv->mbox_chans[i].con_priv = cp;
> > > +	}
> > > +
> > > +	priv->mbox.dev = dev;
> > > +	priv->mbox.ops = &imx_mu_ops;
> > > +	priv->mbox.chans = priv->mbox_chans;
> > > +	priv->mbox.num_chans = chans;
> > > +	priv->mbox.txdone_irq = true;
> > > +
> > > +	platform_set_drvdata(pdev, priv);
> > > +
> > > +	if (priv->dcfg->init_hw)
> > > +		priv->dcfg->init_hw(priv);
> > > +
> > > +	return mbox_controller_register(&priv->mbox);
> > > +}
> > > +
> > > +static int imx_mu_remove(struct platform_device *pdev) {
> > > +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> > > +
> > > +	mbox_controller_unregister(&priv->mbox);
> > > +	clk_disable_unprepare(priv->clk);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +
> > > +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> > > +	/* Set default config */
> > > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> >
> > This will reset both MU Side A and B.
> > So we may need make sure Side B is initialized after A?
> 
> I assume it is implementation specific, as soon as it will be needed, we may
> introduce extra DT flag. No need to cover all possible cases if we don't have
> to.
> 

We shouldn't reset SCU side, but SCU MU is not using it. So I'm okay.
We just need to know the limitation later. Probably a note added here
is better.

> > > +}
> > > +
> > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> > > +	.chans = IMX_MU_MAX_CHANS,
> > > +	.init_hw = imx_mu_init_imx7d_a,
> > > +};
> > > +
> > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> > > +	.chans = IMX_MU_MAX_CHANS,
> > > +};
> > > +
> > > +static const struct of_device_id imx_mu_dt_ids[] = {
> > > +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> > > +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> >
> > I'm not sure whether we already have the decision to use fsl,<soc>-mu
> > compatible String and use property to specify the mu side.
> > Can you double check if we can switch to that way?
> 
> ok.
> 
> > And would you update the binding doc for M4 support according to the
> > qxp mu one Which Is already signed by Rob's tag?
> 
> ok.
> 
> So, should I update my patch set including DT binding documentation prior to
> yours?
> 

I guess you can pick that patch and send with yours. Once your part is
reviewed ok (should be quick) then I can send the SCU part based on your
Patch series.

Finally, I'm glad that we meet an agreement now. As we're trying to
Speed up the mx8qxp support and targets to hit v4.19 kernel.
So hopefully you could help send the updated patch series soon.
Then I can follow up with my work. :)

Regards
Dong Aisheng

> If yes, can you please contact Rob to avoid confusions.
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Oleksij Rempel July 14, 2018, 9:41 a.m. UTC | #8

On Sat, Jul 14, 2018 at 08:49:28AM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Oleksij Rempel [mailto:[e-mail]]
> > Sent: Saturday, July 14, 2018 3:02 PM
> > To: A.s. Dong <[e-mail]>
> > Cc: Shawn Guo <[e-mail]>; Fabio Estevam
> > <[e-mail]>; Rob Herring <[e-mail]>; Mark
> > Rutland <[e-mail]>; [e-mail]; dl-linux-
> > imx <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]
> > Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> > 
> > Hi,
> > 
> > Beside, what is equivalent of name and family name is in your name?
> > I know it is different in China, so I won't to avoid confusion with:
> > "Hi $name," format :)
> > 
> 
> Dong is my family name. Either Hi A.S or Hi Dong is fine to me. :)
> 
> > On Thu, Jul 12, 2018 at 11:28:16AM +0000, A.s. Dong wrote:
> > > Hi Oleksij,
> > >
> > > > -----Original Message-----
> > > > From: Oleksij Rempel [mailto:[e-mail]]
> > > > Sent: Friday, June 15, 2018 5:51 PM
> > > > To: Shawn Guo <[e-mail]>; Fabio Estevam
> > > > <[e-mail]>; Rob Herring <[e-mail]>; Mark
> > > > Rutland <[e-mail]>; A.s. Dong <[e-mail]>
> > > > Cc: Oleksij Rempel <[e-mail]>;
> > [e-mail];
> > > > [e-mail]; [e-mail];
> > > > linux- [e-mail]; dl-linux-imx <[e-mail]>
> > > > Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging
> > > > unit
> > > >
> > > > The Mailbox controller is able to send messages (up to 4 32 bit
> > > > words) between the endpoints.
> > > >
> > >
> > > This is not correct according to current implementation as we abstract
> > > them into 4 virtual channels while each 'channel' can send only one word
> > one time.
> > > We probably need explain such limitation in commit message as well.
> > >
> > > I'm not strongly against this way. But it makes the controller lose
> > > the HW capability to send up to 4 words. I'd just like to know a bit
> > > history or reason why we decided to do that. Do we design it for specific
> > users case for M4?
> > 
> > no, it is R&D.
> > 
> 
> Got it
> 
> > > And are we assuming there will be no real users of multi words send
> > requirement?
> > 
> > no. In my experience, each imaginable Brainfuck configuration will actually
> > happen some day in some design for $reasons.
> > So, no assumptions, just currently working configuration of my R&D project.
> > 
> 
> I'm fine with as it is currently. We don't have to address all possible
> Issues in one time.
> 
> > > > This driver was tested using the mailbox-test driver sending
> > > > messages between the Cortex-A7 and the Cortex-M4.
> > > >
> > > > Signed-off-by: Oleksij Rempel <[e-mail]>
> > > > ---
> > > >  drivers/mailbox/Kconfig       |   6 +
> > > >  drivers/mailbox/Makefile      |   2 +
> > > >  drivers/mailbox/imx-mailbox.c | 288
> > > > ++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 296 insertions(+)
> > > >  create mode 100644 drivers/mailbox/imx-mailbox.c
> > > >
> > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > a2bb27446dce..e1d2738a2e4c 100644
> > > > --- a/drivers/mailbox/Kconfig
> > > > +++ b/drivers/mailbox/Kconfig
> > > > @@ -15,6 +15,12 @@ config ARM_MHU
> > > >  	  The controller has 3 mailbox channels, the last of which can be
> > > >  	  used in Secure mode only.
> > > >
> > > > +config IMX_MBOX
> > > > +	tristate "iMX Mailbox"
> > > > +	depends on SOC_IMX7D || COMPILE_TEST
> > >
> > > Better change to ARCH_MXC as other platform does.
> > 
> > ok
> > 
> > > > +	help
> > > > +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> > >
> > > Ditto
> > 
> > ok
> > 
> > > > +
> > > >  config PLATFORM_MHU
> > > >  	tristate "Platform MHU Mailbox"
> > > >  	depends on OF
> > > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > > index
> > > > cc23c3a43fcd..ba2fe1b6dd62 100644
> > > > --- a/drivers/mailbox/Makefile
> > > > +++ b/drivers/mailbox/Makefile
> > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> > > >
> > > >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > > >
> > > > +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > > > +
> > > >  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> > > >
> > > >  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> > > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > > b/drivers/mailbox/imx-mailbox.c new file mode 100644 index
> > > > 000000000000..e3f621cb1d30
> > > > --- /dev/null
> > > > +++ b/drivers/mailbox/imx-mailbox.c
> > > > @@ -0,0 +1,288 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> > > > +<[e-mail]>  */
> > > > +
> > > > +#include <linux/clk.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > > > +#include <linux/of_device.h>
> > > > +
> > > > +/* Transmit Register */
> > > > +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> > > > +/* Receive Register */
> > > > +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> > > > +/* Status Register */
> > > > +#define IMX_MU_xSR		0x20
> > > > +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> > > > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> > > > +#define IMX_MU_xSR_BRDIP	BIT(9)
> > > > +
> > > > +/* Control Register */
> > > > +#define IMX_MU_xCR		0x24
> > > > +/* Transmit Interrupt Enable */
> > > > +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> > > > +/* Receive Interrupt Enable */
> > > > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> > > > +
> > > > +#define IMX_MU_MAX_CHANS	4u
> > > > +
> > > > +struct imx_mu_priv;
> > > > +
> > > > +struct imx_mu_cfg {
> > > > +	unsigned int		chans;
> > > > +	void (*init_hw)(struct imx_mu_priv *priv); };
> > > > +
> > > > +struct imx_mu_con_priv {
> > > > +	int			irq;
> > > > +	unsigned int		bidx;
> > > > +	unsigned int		idx;
> > > > +};
> > > > +
> > > > +struct imx_mu_priv {
> > > > +	struct device		*dev;
> > > > +	const struct imx_mu_cfg	*dcfg;
> > > > +	void __iomem		*base;
> > > > +
> > > > +	struct mbox_controller	mbox;
> > > > +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> > > > +
> > > > +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> > > > +	struct clk		*clk;
> > > > +};
> > > > +
> > > > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller
> > > > +*mbox) {
> > > > +	return container_of(mbox, struct imx_mu_priv, mbox); }
> > > > +
> > > > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> > > > +	iowrite32(val, priv->base + offs); }
> > > > +
> > > > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> > > > +	return ioread32(priv->base + offs); }
> > > > +
> > > > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set,
> > > > +u32
> > > > +clr) {
> > > > +	u32 val;
> > > > +
> > > > +	val = imx_mu_read(priv, offs);
> > > > +	val &= ~clr;
> > > > +	val |= set;
> > > > +	imx_mu_write(priv, val, offs);
> > > > +
> > > > +	return val;
> > > > +}
> > > > +
> > > > +static irqreturn_t imx_mu_isr(int irq, void *p) {
> > > > +	struct mbox_chan *chan = p;
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +
> > > > +	u32 val, dat;
> > > > +
> > > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > > +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> > > > +	if (!val)
> > > > +		return IRQ_NONE;
> > > > +
> > > > +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> > > > +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> > > > >bidx));
> > > > +		mbox_chan_txdone(chan, 0);
> > > > +	}
> > > > +
> > > > +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> > > > +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> > > > +		mbox_chan_received_data(chan, (void *)&dat);
> > > > +	}
> > > > +
> > > > +	return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +	u32 val;
> > > > +
> > > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > > +	/* test if transmit register is empty */
> > > > +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> > > > +
> > > > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +	u32 *arg = data;
> > > > +
> > > > +	if (!imx_mu_last_tx_done(chan))
> > > > +		return -EBUSY;
> > > > +
> > > > +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> > > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int imx_mu_startup(struct mbox_chan *chan) {
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +	int ret;
> > > > +
> > > > +	ret = request_irq(cp->irq, imx_mu_isr,
> > > > +			  IRQF_SHARED, "imx_mu_chan", chan);
> > >
> > > I guess no need to assign the irq for each cp as we have only one irq.
> > 
> > all irq chip controller have one sink and number of source limited to
> > imagination or amount of bits in a register. May be we will need some day to
> > write a irqchip driver to make it work as chained irq controller.
> > 
> 
> We do need write an irqchip driver later because MU still supports another
> four general purpose interrupts which will be used by SCU firmware.
> But I don't think it's necessary to abstract them for TX/RX interrutps.
> This makes thing simple.

You just gave me one more reason to keep current version :)

> > So, I don't see any technical reason to not do it. Are you?
> > 
> > > > +	if (ret) {
> > > > +		dev_err(chan->mbox->dev,
> > > > +			"Unable to acquire IRQ %d\n", cp->irq);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void imx_mu_shutdown(struct mbox_chan *chan) {
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +
> > > > +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> > > > +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> > > > >bidx));
> > > > +
> > > > +	free_irq(cp->irq, chan);
> > > > +}
> > > > +
> > > > +static const struct mbox_chan_ops imx_mu_ops = {
> > > > +	.send_data = imx_mu_send_data,
> > > > +	.startup = imx_mu_startup,
> > > > +	.shutdown = imx_mu_shutdown,
> > > > +};
> > > > +
> > > > +static int imx_mu_probe(struct platform_device *pdev) {
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct resource *iomem;
> > > > +	struct imx_mu_priv *priv;
> > > > +	const struct imx_mu_cfg *dcfg;
> > > > +	unsigned int i, chans;
> > > > +	int irq, ret;
> > > > +
> > > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > +	if (!priv)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	dcfg = of_device_get_match_data(dev);
> > > > +	if (!dcfg)
> > > > +		return -EINVAL;
> > > > +
> > > > +	priv->dcfg = dcfg;
> > > > +	priv->dev = dev;
> > > > +
> > > > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> > > > +	if (IS_ERR(priv->base))
> > > > +		return PTR_ERR(priv->base);
> > > > +
> > > > +	irq = platform_get_irq(pdev, 0);
> > > > +	if (irq <= 0)
> > > > +		return irq < 0 ? irq : -EINVAL;
> > > > +
> > > > +	priv->clk = devm_clk_get(dev, NULL);
> > > > +	if (IS_ERR(priv->clk)) {
> > > > +		if (PTR_ERR(priv->clk) == -ENOENT) {
> > > > +			priv->clk = NULL;
> > > > +		} else {
> > > > +			dev_err(dev, "Failed to get clock\n");
> > >
> > > I guess we may not need print it for DEFER_PROBE error case.
> > 
> > ok.
> > 
> > > > +			return PTR_ERR(priv->clk);
> > > > +		}
> > > > +	}
> > > > +
> > > > +	ret = clk_prepare_enable(priv->clk);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to enable clock\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> > > > +	/* Initialize channel identifiers */
> > > > +	for (i = 0; i < chans; i++) {
> > > > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > > > +
> > > > +		cp->bidx = 3 - i;
> > >
> > > We may not need it if we improve the macro to calculate bidx by idx?
> > 
> > Are all implementation of NXP MU have reversed bit order?
> 
> AFAIK NXP MU library is used for all known platforms with MUs.
> So I guess yes.
> 
> > Will it fit good for one channel implementation?
> 
> If you see the SCU MU patches I sent, you will see I also need
> convert the channel index to channel mask. But here you're
> using bidx where SCU MU does have. So have a common
> channel index to mask macro may be good for both using.

ok

> > 
> > > > +		cp->idx = i;
> > > > +		cp->irq = irq;
> > > > +		priv->mbox_chans[i].con_priv = cp;
> > > > +	}
> > > > +
> > > > +	priv->mbox.dev = dev;
> > > > +	priv->mbox.ops = &imx_mu_ops;
> > > > +	priv->mbox.chans = priv->mbox_chans;
> > > > +	priv->mbox.num_chans = chans;
> > > > +	priv->mbox.txdone_irq = true;
> > > > +
> > > > +	platform_set_drvdata(pdev, priv);
> > > > +
> > > > +	if (priv->dcfg->init_hw)
> > > > +		priv->dcfg->init_hw(priv);
> > > > +
> > > > +	return mbox_controller_register(&priv->mbox);
> > > > +}
> > > > +
> > > > +static int imx_mu_remove(struct platform_device *pdev) {
> > > > +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> > > > +
> > > > +	mbox_controller_unregister(&priv->mbox);
> > > > +	clk_disable_unprepare(priv->clk);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +
> > > > +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> > > > +	/* Set default config */
> > > > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> > >
> > > This will reset both MU Side A and B.
> > > So we may need make sure Side B is initialized after A?
> > 
> > I assume it is implementation specific, as soon as it will be needed, we may
> > introduce extra DT flag. No need to cover all possible cases if we don't have
> > to.
> > 
> 
> We shouldn't reset SCU side, but SCU MU is not using it. So I'm okay.
> We just need to know the limitation later. Probably a note added here
> is better.

ok

> > > > +}
> > > > +
> > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> > > > +	.chans = IMX_MU_MAX_CHANS,
> > > > +	.init_hw = imx_mu_init_imx7d_a,
> > > > +};
> > > > +
> > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> > > > +	.chans = IMX_MU_MAX_CHANS,
> > > > +};
> > > > +
> > > > +static const struct of_device_id imx_mu_dt_ids[] = {
> > > > +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> > > > +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> > >
> > > I'm not sure whether we already have the decision to use fsl,<soc>-mu
> > > compatible String and use property to specify the mu side.
> > > Can you double check if we can switch to that way?
> > 
> > ok.
> > 
> > > And would you update the binding doc for M4 support according to the
> > > qxp mu one Which Is already signed by Rob's tag?
> > 
> > ok.
> > 
> > So, should I update my patch set including DT binding documentation prior to
> > yours?
> > 
> 
> I guess you can pick that patch and send with yours. Once your part is
> reviewed ok (should be quick) then I can send the SCU part based on your
> Patch series.

Normally it is preferred to squash all history for newly created files.
I'll take your patch as base with minimal changes and send some comments
to Rob.

> Finally, I'm glad that we meet an agreement now. As we're trying to
> Speed up the mx8qxp support and targets to hit v4.19 kernel.
> So hopefully you could help send the updated patch series soon.
> Then I can follow up with my work. :)

I'll try to finish it and resend new version at Monday. Ok?

> Regards
> Dong Aisheng
> 
> > If yes, can you please contact Rob to avoid confusions.
> > 
> > --
> > Pengutronix e.K.                           |                             |
> > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
>

Aisheng Dong July 14, 2018, 11:41 a.m. UTC | #9

> -----Original Message-----
> From: Oleksij Rempel [mailto:[e-mail]]
> Sent: Saturday, July 14, 2018 5:42 PM
> To: A.s. Dong <[e-mail]>
> Cc: Shawn Guo <[e-mail]>; Fabio Estevam
> <[e-mail]>; Rob Herring <[e-mail]>; Mark
> Rutland <[e-mail]>; [e-mail]; dl-linux-
> imx <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> On Sat, Jul 14, 2018 at 08:49:28AM +0000, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Oleksij Rempel [mailto:[e-mail]]
> > > Sent: Saturday, July 14, 2018 3:02 PM
> > > To: A.s. Dong <[e-mail]>
> > > Cc: Shawn Guo <[e-mail]>; Fabio Estevam
> > > <[e-mail]>; Rob Herring <[e-mail]>; Mark
> > > Rutland <[e-mail]>; [e-mail];
> > > dl-linux- imx <[e-mail]>;
> > > [e-mail];
> > > [e-mail]; [e-mail]
> > > Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D
> > > messaging unit
> > >
> > > Hi,
> > >
> > > Beside, what is equivalent of name and family name is in your name?
> > > I know it is different in China, so I won't to avoid confusion with:
> > > "Hi $name," format :)
> > >
> >
> > Dong is my family name. Either Hi A.S or Hi Dong is fine to me. :)
> >
> > > On Thu, Jul 12, 2018 at 11:28:16AM +0000, A.s. Dong wrote:
> > > > Hi Oleksij,
> > > >
> > > > > -----Original Message-----
> > > > > From: Oleksij Rempel [mailto:[e-mail]]
> > > > > Sent: Friday, June 15, 2018 5:51 PM
> > > > > To: Shawn Guo <[e-mail]>; Fabio Estevam
> > > > > <[e-mail]>; Rob Herring <[e-mail]>;
> Mark
> > > > > Rutland <[e-mail]>; A.s. Dong
> <[e-mail]>
> > > > > Cc: Oleksij Rempel <[e-mail]>;
> > > [e-mail];
> > > > > [e-mail];
> > > > > [e-mail];
> > > > > linux- [e-mail]; dl-linux-imx <[e-mail]>
> > > > > Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D
> > > > > messaging unit
> > > > >
> > > > > The Mailbox controller is able to send messages (up to 4 32 bit
> > > > > words) between the endpoints.
> > > > >
> > > >
> > > > This is not correct according to current implementation as we
> > > > abstract them into 4 virtual channels while each 'channel' can
> > > > send only one word
> > > one time.
> > > > We probably need explain such limitation in commit message as well.
> > > >
> > > > I'm not strongly against this way. But it makes the controller
> > > > lose the HW capability to send up to 4 words. I'd just like to
> > > > know a bit history or reason why we decided to do that. Do we
> > > > design it for specific
> > > users case for M4?
> > >
> > > no, it is R&D.
> > >
> >
> > Got it
> >
> > > > And are we assuming there will be no real users of multi words
> > > > send
> > > requirement?
> > >
> > > no. In my experience, each imaginable Brainfuck configuration will
> > > actually happen some day in some design for $reasons.
> > > So, no assumptions, just currently working configuration of my R&D
> project.
> > >
> >
> > I'm fine with as it is currently. We don't have to address all
> > possible Issues in one time.
> >
> > > > > This driver was tested using the mailbox-test driver sending
> > > > > messages between the Cortex-A7 and the Cortex-M4.
> > > > >
> > > > > Signed-off-by: Oleksij Rempel <[e-mail]>
> > > > > ---
> > > > >  drivers/mailbox/Kconfig       |   6 +
> > > > >  drivers/mailbox/Makefile      |   2 +
> > > > >  drivers/mailbox/imx-mailbox.c | 288
> > > > > ++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 296 insertions(+)  create mode 100644
> > > > > drivers/mailbox/imx-mailbox.c
> > > > >
> > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > > > > index a2bb27446dce..e1d2738a2e4c 100644
> > > > > --- a/drivers/mailbox/Kconfig
> > > > > +++ b/drivers/mailbox/Kconfig
> > > > > @@ -15,6 +15,12 @@ config ARM_MHU
> > > > >  	  The controller has 3 mailbox channels, the last of which can
> be
> > > > >  	  used in Secure mode only.
> > > > >
> > > > > +config IMX_MBOX
> > > > > +	tristate "iMX Mailbox"
> > > > > +	depends on SOC_IMX7D || COMPILE_TEST
> > > >
> > > > Better change to ARCH_MXC as other platform does.
> > >
> > > ok
> > >
> > > > > +	help
> > > > > +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> > > >
> > > > Ditto
> > >
> > > ok
> > >
> > > > > +
> > > > >  config PLATFORM_MHU
> > > > >  	tristate "Platform MHU Mailbox"
> > > > >  	depends on OF
> > > > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > > > index
> > > > > cc23c3a43fcd..ba2fe1b6dd62 100644
> > > > > --- a/drivers/mailbox/Makefile
> > > > > +++ b/drivers/mailbox/Makefile
> > > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-
> test.o
> > > > >
> > > > >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > > > >
> > > > > +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > > > > +
> > > > >  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> > > > >
> > > > >  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> > > > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > > > b/drivers/mailbox/imx-mailbox.c new file mode 100644 index
> > > > > 000000000000..e3f621cb1d30
> > > > > --- /dev/null
> > > > > +++ b/drivers/mailbox/imx-mailbox.c
> > > > > @@ -0,0 +1,288 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> > > > > +<[e-mail]>  */
> > > > > +
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/io.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > > > > +#include <linux/of_device.h>
> > > > > +
> > > > > +/* Transmit Register */
> > > > > +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> > > > > +/* Receive Register */
> > > > > +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> > > > > +/* Status Register */
> > > > > +#define IMX_MU_xSR		0x20
> > > > > +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> > > > > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> > > > > +#define IMX_MU_xSR_BRDIP	BIT(9)
> > > > > +
> > > > > +/* Control Register */
> > > > > +#define IMX_MU_xCR		0x24
> > > > > +/* Transmit Interrupt Enable */
> > > > > +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> > > > > +/* Receive Interrupt Enable */
> > > > > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> > > > > +
> > > > > +#define IMX_MU_MAX_CHANS	4u
> > > > > +
> > > > > +struct imx_mu_priv;
> > > > > +
> > > > > +struct imx_mu_cfg {
> > > > > +	unsigned int		chans;
> > > > > +	void (*init_hw)(struct imx_mu_priv *priv); };
> > > > > +
> > > > > +struct imx_mu_con_priv {
> > > > > +	int			irq;
> > > > > +	unsigned int		bidx;
> > > > > +	unsigned int		idx;
> > > > > +};
> > > > > +
> > > > > +struct imx_mu_priv {
> > > > > +	struct device		*dev;
> > > > > +	const struct imx_mu_cfg	*dcfg;
> > > > > +	void __iomem		*base;
> > > > > +
> > > > > +	struct mbox_controller	mbox;
> > > > > +	struct mbox_chan
> 	mbox_chans[IMX_MU_MAX_CHANS];
> > > > > +
> > > > > +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> > > > > +	struct clk		*clk;
> > > > > +};
> > > > > +
> > > > > +static struct imx_mu_priv *to_imx_mu_priv(struct
> > > > > +mbox_controller
> > > > > +*mbox) {
> > > > > +	return container_of(mbox, struct imx_mu_priv, mbox); }
> > > > > +
> > > > > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
> {
> > > > > +	iowrite32(val, priv->base + offs); }
> > > > > +
> > > > > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> > > > > +	return ioread32(priv->base + offs); }
> > > > > +
> > > > > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32
> > > > > +set,
> > > > > +u32
> > > > > +clr) {
> > > > > +	u32 val;
> > > > > +
> > > > > +	val = imx_mu_read(priv, offs);
> > > > > +	val &= ~clr;
> > > > > +	val |= set;
> > > > > +	imx_mu_write(priv, val, offs);
> > > > > +
> > > > > +	return val;
> > > > > +}
> > > > > +
> > > > > +static irqreturn_t imx_mu_isr(int irq, void *p) {
> > > > > +	struct mbox_chan *chan = p;
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +
> > > > > +	u32 val, dat;
> > > > > +
> > > > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > > > +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp-
> >bidx);
> > > > > +	if (!val)
> > > > > +		return IRQ_NONE;
> > > > > +
> > > > > +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> > > > > +		imx_mu_rmw(priv, IMX_MU_xCR, 0,
> IMX_MU_xCR_TIEn(cp-
> > > > > >bidx));
> > > > > +		mbox_chan_txdone(chan, 0);
> > > > > +	}
> > > > > +
> > > > > +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> > > > > +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> > > > > +		mbox_chan_received_data(chan, (void *)&dat);
> > > > > +	}
> > > > > +
> > > > > +	return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +	u32 val;
> > > > > +
> > > > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > > > +	/* test if transmit register is empty */
> > > > > +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> > > > > +
> > > > > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +	u32 *arg = data;
> > > > > +
> > > > > +	if (!imx_mu_last_tx_done(chan))
> > > > > +		return -EBUSY;
> > > > > +
> > > > > +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> > > > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp-
> >bidx), 0);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int imx_mu_startup(struct mbox_chan *chan) {
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = request_irq(cp->irq, imx_mu_isr,
> > > > > +			  IRQF_SHARED, "imx_mu_chan", chan);
> > > >
> > > > I guess no need to assign the irq for each cp as we have only one irq.
> > >
> > > all irq chip controller have one sink and number of source limited
> > > to imagination or amount of bits in a register. May be we will need
> > > some day to write a irqchip driver to make it work as chained irq
> controller.
> > >
> >
> > We do need write an irqchip driver later because MU still supports
> > another four general purpose interrupts which will be used by SCU
> firmware.
> > But I don't think it's necessary to abstract them for TX/RX interrutps.
> > This makes thing simple.
> 
> You just gave me one more reason to keep current version :)
> 

Anyway, I don't think it's a must change.
You can keep it.

> > > So, I don't see any technical reason to not do it. Are you?
> > >
> > > > > +	if (ret) {
> > > > > +		dev_err(chan->mbox->dev,
> > > > > +			"Unable to acquire IRQ %d\n", cp->irq);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp-
> >bidx), 0);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static void imx_mu_shutdown(struct mbox_chan *chan) {
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +
> > > > > +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> > > > > +		   IMX_MU_xCR_TIEn(cp->bidx) |
> IMX_MU_xCR_RIEn(cp-
> > > > > >bidx));
> > > > > +
> > > > > +	free_irq(cp->irq, chan);
> > > > > +}
> > > > > +
> > > > > +static const struct mbox_chan_ops imx_mu_ops = {
> > > > > +	.send_data = imx_mu_send_data,
> > > > > +	.startup = imx_mu_startup,
> > > > > +	.shutdown = imx_mu_shutdown,
> > > > > +};
> > > > > +
> > > > > +static int imx_mu_probe(struct platform_device *pdev) {
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	struct resource *iomem;
> > > > > +	struct imx_mu_priv *priv;
> > > > > +	const struct imx_mu_cfg *dcfg;
> > > > > +	unsigned int i, chans;
> > > > > +	int irq, ret;
> > > > > +
> > > > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > > +	if (!priv)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	dcfg = of_device_get_match_data(dev);
> > > > > +	if (!dcfg)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	priv->dcfg = dcfg;
> > > > > +	priv->dev = dev;
> > > > > +
> > > > > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM,
> 0);
> > > > > +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> > > > > +	if (IS_ERR(priv->base))
> > > > > +		return PTR_ERR(priv->base);
> > > > > +
> > > > > +	irq = platform_get_irq(pdev, 0);
> > > > > +	if (irq <= 0)
> > > > > +		return irq < 0 ? irq : -EINVAL;
> > > > > +
> > > > > +	priv->clk = devm_clk_get(dev, NULL);
> > > > > +	if (IS_ERR(priv->clk)) {
> > > > > +		if (PTR_ERR(priv->clk) == -ENOENT) {
> > > > > +			priv->clk = NULL;
> > > > > +		} else {
> > > > > +			dev_err(dev, "Failed to get clock\n");
> > > >
> > > > I guess we may not need print it for DEFER_PROBE error case.
> > >
> > > ok.
> > >
> > > > > +			return PTR_ERR(priv->clk);
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	ret = clk_prepare_enable(priv->clk);
> > > > > +	if (ret) {
> > > > > +		dev_err(dev, "Failed to enable clock\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> > > > > +	/* Initialize channel identifiers */
> > > > > +	for (i = 0; i < chans; i++) {
> > > > > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > > > > +
> > > > > +		cp->bidx = 3 - i;
> > > >
> > > > We may not need it if we improve the macro to calculate bidx by idx?
> > >
> > > Are all implementation of NXP MU have reversed bit order?
> >
> > AFAIK NXP MU library is used for all known platforms with MUs.
> > So I guess yes.
> >
> > > Will it fit good for one channel implementation?
> >
> > If you see the SCU MU patches I sent, you will see I also need convert
> > the channel index to channel mask. But here you're using bidx where
> > SCU MU does have. So have a common channel index to mask macro may
> be
> > good for both using.
> 
> ok
> 
> > >
> > > > > +		cp->idx = i;
> > > > > +		cp->irq = irq;
> > > > > +		priv->mbox_chans[i].con_priv = cp;
> > > > > +	}
> > > > > +
> > > > > +	priv->mbox.dev = dev;
> > > > > +	priv->mbox.ops = &imx_mu_ops;
> > > > > +	priv->mbox.chans = priv->mbox_chans;
> > > > > +	priv->mbox.num_chans = chans;
> > > > > +	priv->mbox.txdone_irq = true;
> > > > > +
> > > > > +	platform_set_drvdata(pdev, priv);
> > > > > +
> > > > > +	if (priv->dcfg->init_hw)
> > > > > +		priv->dcfg->init_hw(priv);
> > > > > +
> > > > > +	return mbox_controller_register(&priv->mbox);
> > > > > +}
> > > > > +
> > > > > +static int imx_mu_remove(struct platform_device *pdev) {
> > > > > +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> > > > > +
> > > > > +	mbox_controller_unregister(&priv->mbox);
> > > > > +	clk_disable_unprepare(priv->clk);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +
> > > > > +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> > > > > +	/* Set default config */
> > > > > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> > > >
> > > > This will reset both MU Side A and B.
> > > > So we may need make sure Side B is initialized after A?
> > >
> > > I assume it is implementation specific, as soon as it will be
> > > needed, we may introduce extra DT flag. No need to cover all
> > > possible cases if we don't have to.
> > >
> >
> > We shouldn't reset SCU side, but SCU MU is not using it. So I'm okay.
> > We just need to know the limitation later. Probably a note added here
> > is better.
> 
> ok
> 
> > > > > +}
> > > > > +
> > > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> > > > > +	.chans = IMX_MU_MAX_CHANS,
> > > > > +	.init_hw = imx_mu_init_imx7d_a, };
> > > > > +
> > > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> > > > > +	.chans = IMX_MU_MAX_CHANS,
> > > > > +};
> > > > > +
> > > > > +static const struct of_device_id imx_mu_dt_ids[] = {
> > > > > +	{ .compatible = "fsl,imx7s-mu-a", .data =
> &imx_mu_cfg_imx7d_a },
> > > > > +	{ .compatible = "fsl,imx7s-mu-b", .data =
> &imx_mu_cfg_imx7d_b
> > > > > +},
> > > >
> > > > I'm not sure whether we already have the decision to use
> > > > fsl,<soc>-mu compatible String and use property to specify the mu side.
> > > > Can you double check if we can switch to that way?
> > >
> > > ok.
> > >
> > > > And would you update the binding doc for M4 support according to
> > > > the qxp mu one Which Is already signed by Rob's tag?
> > >
> > > ok.
> > >
> > > So, should I update my patch set including DT binding documentation
> > > prior to yours?
> > >
> >
> > I guess you can pick that patch and send with yours. Once your part is
> > reviewed ok (should be quick) then I can send the SCU part based on
> > your Patch series.
> 
> Normally it is preferred to squash all history for newly created files.
> I'll take your patch as base with minimal changes and send some comments
> to Rob.
> 
> > Finally, I'm glad that we meet an agreement now. As we're trying to
> > Speed up the mx8qxp support and targets to hit v4.19 kernel.
> > So hopefully you could help send the updated patch series soon.
> > Then I can follow up with my work. :)
> 
> I'll try to finish it and resend new version at Monday. Ok?
> 

Good to me.
Thanks

Regards
Dong Aisheng

> > Regards
> > Dong Aisheng
> >
> > > If yes, can you please contact Rob to avoid confusions.
> > >
> > > --
> > > Pengutronix e.K.                           |                             |
> > > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]
> |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
> >
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index a2bb27446dce..e1d2738a2e4c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -15,6 +15,12 @@  config ARM_MHU
 	  The controller has 3 mailbox channels, the last of which can be
 	  used in Secure mode only.
 
+config IMX_MBOX
+	tristate "iMX Mailbox"
+	depends on SOC_IMX7D || COMPILE_TEST
+	help
+	  Mailbox implementation for iMX7D Messaging Unit (MU).
+
 config PLATFORM_MHU
 	tristate "Platform MHU Mailbox"
 	depends on OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index cc23c3a43fcd..ba2fe1b6dd62 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -7,6 +7,8 @@  obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
 
 obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
 
+obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
+
 obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
 
 obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
new file mode 100644
index 000000000000..e3f621cb1d30
--- /dev/null
+++ b/drivers/mailbox/imx-mailbox.c
@@ -0,0 +1,288 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Pengutronix, Oleksij Rempel <[e-mail]>
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+/* Transmit Register */
+#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
+/* Receive Register */
+#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
+/* Status Register */
+#define IMX_MU_xSR		0x20
+#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
+#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
+#define IMX_MU_xSR_BRDIP	BIT(9)
+
+/* Control Register */
+#define IMX_MU_xCR		0x24
+/* Transmit Interrupt Enable */
+#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
+
+#define IMX_MU_MAX_CHANS	4u
+
+struct imx_mu_priv;
+
+struct imx_mu_cfg {
+	unsigned int		chans;
+	void (*init_hw)(struct imx_mu_priv *priv);
+};
+
+struct imx_mu_con_priv {
+	int			irq;
+	unsigned int		bidx;
+	unsigned int		idx;
+};
+
+struct imx_mu_priv {
+	struct device		*dev;
+	const struct imx_mu_cfg	*dcfg;
+	void __iomem		*base;
+
+	struct mbox_controller	mbox;
+	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
+
+	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
+	struct clk		*clk;
+};
+
+static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct imx_mu_priv, mbox);
+}
+
+static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
+{
+	iowrite32(val, priv->base + offs);
+}
+
+static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs)
+{
+	return ioread32(priv->base + offs);
+}
+
+static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr)
+{
+	u32 val;
+
+	val = imx_mu_read(priv, offs);
+	val &= ~clr;
+	val |= set;
+	imx_mu_write(priv, val, offs);
+
+	return val;
+}
+
+static irqreturn_t imx_mu_isr(int irq, void *p)
+{
+	struct mbox_chan *chan = p;
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+
+	u32 val, dat;
+
+	val = imx_mu_read(priv, IMX_MU_xSR);
+	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
+		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->bidx));
+		mbox_chan_txdone(chan, 0);
+	}
+
+	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
+		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
+		mbox_chan_received_data(chan, (void *)&dat);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static bool imx_mu_last_tx_done(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	u32 val;
+
+	val = imx_mu_read(priv, IMX_MU_xSR);
+	/* test if transmit register is empty */
+	return (!!(val & IMX_MU_xSR_TEn(cp->bidx)));
+}
+
+static int imx_mu_send_data(struct mbox_chan *chan, void *data)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	u32 *arg = data;
+
+	if (!imx_mu_last_tx_done(chan))
+		return -EBUSY;
+
+	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
+	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
+
+	return 0;
+}
+
+static int imx_mu_startup(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	int ret;
+
+	ret = request_irq(cp->irq, imx_mu_isr,
+			  IRQF_SHARED, "imx_mu_chan", chan);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Unable to acquire IRQ %d\n", cp->irq);
+		return ret;
+	}
+
+	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
+
+	return 0;
+}
+
+static void imx_mu_shutdown(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+
+	imx_mu_rmw(priv, IMX_MU_xCR, 0,
+		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp->bidx));
+
+	free_irq(cp->irq, chan);
+}
+
+static const struct mbox_chan_ops imx_mu_ops = {
+	.send_data = imx_mu_send_data,
+	.startup = imx_mu_startup,
+	.shutdown = imx_mu_shutdown,
+};
+
+static int imx_mu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *iomem;
+	struct imx_mu_priv *priv;
+	const struct imx_mu_cfg *dcfg;
+	unsigned int i, chans;
+	int irq, ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dcfg = of_device_get_match_data(dev);
+	if (!dcfg)
+		return -EINVAL;
+
+	priv->dcfg = dcfg;
+	priv->dev = dev;
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return irq < 0 ? irq : -EINVAL;
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		if (PTR_ERR(priv->clk) == -ENOENT) {
+			priv->clk = NULL;
+		} else {
+			dev_err(dev, "Failed to get clock\n");
+			return PTR_ERR(priv->clk);
+		}
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
+	/* Initialize channel identifiers */
+	for (i = 0; i < chans; i++) {
+		struct imx_mu_con_priv *cp = &priv->con_priv[i];
+
+		cp->bidx = 3 - i;
+		cp->idx = i;
+		cp->irq = irq;
+		priv->mbox_chans[i].con_priv = cp;
+	}
+
+	priv->mbox.dev = dev;
+	priv->mbox.ops = &imx_mu_ops;
+	priv->mbox.chans = priv->mbox_chans;
+	priv->mbox.num_chans = chans;
+	priv->mbox.txdone_irq = true;
+
+	platform_set_drvdata(pdev, priv);
+
+	if (priv->dcfg->init_hw)
+		priv->dcfg->init_hw(priv);
+
+	return mbox_controller_register(&priv->mbox);
+}
+
+static int imx_mu_remove(struct platform_device *pdev)
+{
+	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&priv->mbox);
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+
+static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv)
+{
+	/* Set default config */
+	imx_mu_write(priv, 0, IMX_MU_xCR);
+}
+
+static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
+	.chans = IMX_MU_MAX_CHANS,
+	.init_hw = imx_mu_init_imx7d_a,
+};
+
+static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
+	.chans = IMX_MU_MAX_CHANS,
+};
+
+static const struct of_device_id imx_mu_dt_ids[] = {
+	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
+	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
+
+static struct platform_driver imx_mu_driver = {
+	.probe		= imx_mu_probe,
+	.remove		= imx_mu_remove,
+	.driver = {
+		.name	= "imx_mu",
+		.of_match_table = imx_mu_dt_ids,
+	},
+};
+module_platform_driver(imx_mu_driver);
+
+MODULE_AUTHOR("Oleksij Rempel <[e-mail]>");
+MODULE_DESCRIPTION("Message Unit driver for i.MX");
+MODULE_LICENSE("GPL v2");