[PATCH V4 1/5] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[PATCH V4 1/5] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[Dong Aisheng]

Mailbox devices may have only one channel which means the mbox-cells
at least 1 does not make sense for this type devices. Let's remove
that limitation to allow the mbox-cells to be equal to 0.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
 * New patch introduced in v3 series
 * Actually there're already users in kernel with mbox-cells set to 0.
   See:
   arch/arm/boot/dts/bcm283x.dtsi:145: #mbox-cells = <0>;
---
 Documentation/devicetree/bindings/mailbox/mailbox.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt
index af8ecee..c2fcd05 100644
--- a/Documentation/devicetree/bindings/mailbox/mailbox.txt
+++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
@@ -6,8 +6,7 @@ assign appropriate mailbox channel to client drivers.
 * Mailbox Controller
 
 Required property:
-- #mbox-cells: Must be at least 1. Number of cells in a mailbox
-		specifier.
+- #mbox-cells: Number of cells in a mailbox specifier.
 
 Example:
 	mailbox: mailbox {
-- 
2.7.4

[PATCH V4 3/5] mailbox: imx: add imx mu support

[Dong Aisheng]

This is used for i.MX multi core communication.
e.g. A core to SCU firmware(M core) on MX8.

Tx is using polling mode while Rx is interrupt driven and
schedule a hrtimer to receive remain words if have more than
4 words.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: linux-kernel@vger.kernel.org
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * New patch
   Old MU library dropped
---
 drivers/mailbox/Kconfig  |   8 ++
 drivers/mailbox/Makefile |   2 +
 drivers/mailbox/imx-mu.c | 341 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 351 insertions(+)
 create mode 100644 drivers/mailbox/imx-mu.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index a2bb274..4dd4823 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -124,6 +124,14 @@ config HI6220_MBOX
 	  between application processors and MCU. Say Y here if you want to
 	  build Hi6220 mailbox controller driver.
 
+config IMX_MU
+	tristate "IMX MU Mailbox"
+	depends on ARCH_MXC && OF
+	help
+	  An implementation of the i.MX MU Mailbox. It is used to send message
+	  between application processors and other processors/MCU/DSP. Select
+	  Y here if you want to use i.MX MU Mailbox controller.
+
 config MAILBOX_TEST
 	tristate "Mailbox Test Client"
 	depends on OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index cc23c3a..fa62f07 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -31,6 +31,8 @@ obj-$(CONFIG_HI3660_MBOX)	+= hi3660-mailbox.o
 
 obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 
+obj-$(CONFIG_IMX_MU)		+= imx-mu.o
+
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
 
 obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
diff --git a/drivers/mailbox/imx-mu.c b/drivers/mailbox/imx-mu.c
new file mode 100644
index 0000000..339dfa9
--- /dev/null
+++ b/drivers/mailbox/imx-mu.c
@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP.
+ *
+ *  Author: Dong Aisheng <aisheng.dong@nxp.com>
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/hrtimer.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define MU_xTR0		0x0
+#define MU_xRR0		0x10
+#define MU_xSR		0x20
+#define MU_xCR		0x24
+
+#define MU_TR_COUNT		4
+#define MU_RR_COUNT		4
+
+#define MU_CR_GIEn_MASK		GENMASK(37, 28)
+#define MU_CR_RIEn_MASK		GENMASK(27, 24)
+#define MU_CR_TIEn_MASK		GENMASK(23, 20)
+#define MU_CR_GIRn_MASK		GENMASK(19, 16)
+#define MU_CR_NMI_MASK		BIT(3)
+#define MU_CR_Fn_MASK		GENMASK(2, 0)
+#define MU_CR_RIE0_SHIFT	27
+#define MU_CR_TIE0_SHIFT	23
+#define MU_CR_RIE0_MASK		BIT(27)
+#define MU_CR_GIE0_MASK		BIT(31)
+
+#define MU_SR_TE0_MASK		BIT(23)
+#define MU_SR_RF0_MASK		BIT(27)
+
+#define MU_SCU_MAX_MSG		8
+#define MU_DATA_TIME_OUT_US	(100 * USEC_PER_MSEC)
+
+struct imx_mu {
+	void __iomem *regs;
+	struct mbox_controller mbox;
+	struct mbox_chan chans;
+	struct hrtimer poll_hrt;
+	/* for runtime scu msg store */
+	u32 *msg;
+};
+
+/*
+ * Wait to receive message from the other core.
+ */
+static int imx_mu_receive_msg(struct mbox_chan *chan, u32 index, u32 *msg)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 mask, sr;
+	int ret;
+
+	mask = MU_SR_RF0_MASK >> index;
+
+	/* Wait RX register to be full. */
+	ret = readl_poll_timeout_atomic(mu->regs + MU_xSR, sr, sr & mask,
+					0, MU_DATA_TIME_OUT_US);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Waiting MU receive register (%u) full timeout!\n",
+			index);
+		return ret;
+	}
+
+	*msg = readl(mu->regs + MU_xRR0 + (index * 4));
+
+	return 0;
+}
+
+/*
+ * Wait and send message to the other core.
+ */
+static int imx_mu_send_msg(struct mbox_chan *chan, u32 index, u32 msg)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 mask, sr;
+	int ret;
+
+	mask = MU_SR_TE0_MASK >> index;
+
+	/* Wait TX register to be empty. */
+	ret = readl_poll_timeout_atomic(mu->regs + MU_xSR, sr, sr & mask,
+					0, MU_DATA_TIME_OUT_US);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Waiting MU transmit register (%u) empty timeout!\n",
+			index);
+		return ret;
+	}
+
+	writel(msg, mu->regs + MU_xTR0  + (index * 4));
+
+	return 0;
+}
+
+static enum hrtimer_restart imx_chan_recv_hrtimer(struct hrtimer *hrtimer)
+{
+	struct imx_mu *mu = container_of(hrtimer, struct imx_mu, poll_hrt);
+	u8 *raw_data = (u8 *)mu->msg;
+	int size;
+	int ret;
+	int i;
+
+	/* check msg size */
+	raw_data = (u8 *)mu->msg;
+	size = raw_data[1];
+
+	dev_dbg(mu->mbox.dev, "receive the remain %d words\n", size - 1);
+
+	for (i = 1; i < size; i++) {
+		ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
+		if (ret)
+			break;
+	}
+
+	mbox_chan_received_data(&mu->chans, (void *)mu->msg);
+
+	return HRTIMER_NORESTART;
+}
+
+static irqreturn_t imx_channel_irq(int irq, void *data)
+{
+	struct imx_mu *mu = data;
+	u32 status, mask;
+	u8 *raw_data;
+	int size;
+	int i;
+
+	/* Only enabled RIE0 interrupt */
+	status = readl(mu->regs + MU_xSR);
+	if (!(status & MU_SR_RF0_MASK))
+		return IRQ_NONE;
+
+	/* Get the first data */
+	*mu->msg = readl(mu->regs + MU_xRR0);
+
+	/* check msg size */
+	raw_data = (u8 *)mu->msg;
+	size = raw_data[1];
+
+	dev_dbg(mu->mbox.dev, "receive data: head 0x%x\n", *mu->msg);
+
+	if (size > MU_SCU_MAX_MSG) {
+		dev_err(mu->mbox.dev,
+			"exceed the maximum scu msg size : %d\n", size);
+		return IRQ_HANDLED;
+
+	} else if (size > 1 && size <= 4) {
+		/* check remain data mask */
+		mask = GENMASK(MU_CR_RIE0_SHIFT	- 1, MU_CR_RIE0_SHIFT - size + 1);
+		status = readl(mu->regs + MU_xSR);
+
+		if ((status & mask) != mask) {
+			dev_dbg(mu->mbox.dev,
+				"start hrtimer to receive the remain %d words\n", size - 1);
+			hrtimer_start(&mu->poll_hrt, ktime_set(0, 0),
+				      HRTIMER_MODE_REL);
+			return IRQ_HANDLED;
+		}
+
+		dev_dbg(mu->mbox.dev,
+			"receive data: size %d mask 0x%x status 0x%x\n",
+			size, mask, status);
+
+		for (i = 1; i < size; i++)
+			mu->msg[i] = readl(mu->regs + MU_xRR0 + i * 4);
+
+	} else if (size > 4) {
+		dev_dbg(mu->mbox.dev,
+			"start hrtimer to receive the remain %d words\n", size - 1);
+		hrtimer_start(&mu->poll_hrt, ktime_set(0, 0),
+			      HRTIMER_MODE_REL);
+
+		return IRQ_HANDLED;
+	}
+
+	mbox_chan_received_data(&mu->chans, (void *)mu->msg);
+
+	return IRQ_HANDLED;
+}
+
+static int imx_mu_chan_send_data(struct mbox_chan *chan, void *data)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u8 *raw_data = data;
+	int i, ret;
+	int size;
+
+	if (!data)
+		return -EINVAL;
+
+	mu->msg = data;
+
+	/* SCU protocol size position is at the second u8 */
+	size = raw_data[1];
+
+	dev_dbg(mu->mbox.dev, "send data (size %d)\n", size);
+
+	for (i = 0; i < size; i++) {
+		ret = imx_mu_send_msg(chan, i % 4, *(mu->msg + i));
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int imx_mu_chan_startup(struct mbox_chan *chan)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 val;
+
+	/* Enable RIE0 interrupt */
+	val = readl(mu->regs + MU_xCR);
+	val |= MU_CR_RIE0_MASK;
+	writel(val, mu->regs + MU_xCR);
+
+	return 0;
+}
+
+static void imx_mu_chan_shutdown(struct mbox_chan *chan)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 val;
+
+	/* Clear RIEn, TIEn, GIRn and ABFn. */
+	val = readl(mu->regs + MU_xCR);
+	val &= ~(MU_CR_RIEn_MASK | MU_CR_TIEn_MASK |
+		 MU_CR_GIRn_MASK | MU_CR_NMI_MASK | MU_CR_Fn_MASK);
+	writel(val, mu->regs + MU_xCR);
+}
+
+static struct mbox_chan_ops imx_mu_chan_ops = {
+	.send_data = imx_mu_chan_send_data,
+	.startup = imx_mu_chan_startup,
+	.shutdown = imx_mu_chan_shutdown,
+};
+
+static struct mbox_chan *imx_mu_index_xlate(struct mbox_controller *mbox,
+					    const struct of_phandle_args *sp)
+{
+	if (sp->args_count != 0) {
+		dev_err(mbox->dev,
+			"incorrect mu channel specified in devicetree\n");
+		return NULL;
+	}
+
+	return &mbox->chans[0];
+}
+
+static int imx_mu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mbox_controller *mbox;
+	struct resource *res;
+	struct imx_mu *mu;
+	int irq, err;
+	u32 val;
+
+	mu = devm_kzalloc(&pdev->dev, sizeof(*mu), GFP_KERNEL);
+	if (!mu)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mu->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mu->regs))
+		return PTR_ERR(mu->regs);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "can't get irq number\n");
+		return irq;
+	}
+
+	err = devm_request_irq(dev, irq, imx_channel_irq, 0,
+			       dev_name(dev), mu);
+	if (err < 0) {
+		dev_err(dev, "Failed to request IRQ#%u: %d\n", irq, err);
+		return err;
+	}
+
+	hrtimer_init(&mu->poll_hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	mu->poll_hrt.function = imx_chan_recv_hrtimer;
+
+	platform_set_drvdata(pdev, mu);
+
+	mbox = &mu->mbox;
+	mbox->dev = dev;
+	mbox->num_chans = 1;
+	mbox->txdone_irq = false;
+	mbox->txdone_poll = false;
+	mbox->chans = &mu->chans;
+	mbox->ops = &imx_mu_chan_ops;
+	mbox->of_xlate = &imx_mu_index_xlate;
+	mu->chans.con_priv = mu;
+
+	/* Init MU */
+	val = readl(mu->regs + MU_xCR);
+	/* Clear GIEn, RIEn, TIEn, GIRn and ABFn. */
+	val &= ~(MU_CR_GIEn_MASK | MU_CR_RIEn_MASK | MU_CR_TIEn_MASK |
+		 MU_CR_GIRn_MASK | MU_CR_NMI_MASK | MU_CR_Fn_MASK);
+	writel(val, mu->regs + MU_xCR);
+
+	err = mbox_controller_register(mbox);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register mailbox: %d\n", err);
+		return err;
+	}
+
+	dev_info(dev, "registered mailbox\n");
+
+	return 0;
+}
+
+static const struct of_device_id imx_mu_match[] = {
+	{ .compatible = "fsl,imx8qxp-mu", },
+	{ /* Sentinel */ }
+};
+
+static struct platform_driver imx_mu_driver = {
+	.driver = {
+		.name = "imx-mu",
+		.of_match_table = imx_mu_match,
+	},
+	.probe = imx_mu_probe,
+};
+
+static int __init imx_mu_init(void)
+{
+	return platform_driver_register(&imx_mu_driver);
+}
+core_initcall(imx_mu_init);
-- 
2.7.4

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Sascha Hauer]

Hi,

On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> This is used for i.MX multi core communication.
> e.g. A core to SCU firmware(M core) on MX8.
> 
> Tx is using polling mode while Rx is interrupt driven and
> schedule a hrtimer to receive remain words if have more than
> 4 words.

You told us that using interrupts is not possible due to miserable
performance, we then provided you a way with which you could poll.  Why
are you using interrupts now?

We also suggested a way how the SCU mode could be integrated into the
generic MU support driver Oleksij posted and now you send a driver which
uses the same name as Oleksijs driver, but it only and exclusively works
in SCU mode. This doesn't bring us forward.

We suggested a binding that allows coexisting of the SCU mode and the
generic mode of the MU by putting the mode information into the second
mbox-cell. Why don't you use this?

I don't think it's necessary to rewrite Oleksijs driver, instead it
should rather be extended with the code I already provided as an
example. With that we could make both of us happy since we can both
have a suitable driver and even share most of the MU code.

Regards,
Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> Sent: Tuesday, July 10, 2018 10:20 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> Hi,
> 
> On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > This is used for i.MX multi core communication.
> > e.g. A core to SCU firmware(M core) on MX8.
> >
> > Tx is using polling mode while Rx is interrupt driven and schedule a
> > hrtimer to receive remain words if have more than
> > 4 words.
> 
> You told us that using interrupts is not possible due to miserable
> performance, we then provided you a way with which you could poll.  Why
> are you using interrupts now?
> 

Because mailbox framework does not support sync rx now, I think we do not
need to wait for that feature done first as it's independent and  separate
features of framework. 

So for now, we're just using the common way in kernel as arm scpi and ti sci. 
When framework supports it, we can easily switch to it.

I optimized the performance a bit by removing the unnecessary memcopy
between tx/tx messages. The test result of booting time shows there's no
obvious regressions. I'm not sure whether it's due to we're booting a minimum
system or the extra cost is very minor to be noticed due to not too much cmds
sent during booting.
(Copy Peng to comments more as he tried and reported that performance drop
with vendor tree)

From the time measurement of sc_call_rpc, we can see that most rpc command
In polling mode can finish within 10us and very rare ones over 20us.
If switched to irq mode, those 10us cmds will change to about 20us. 

But the overall booting time did not increase much. Maybe the irq mode
also saves some CPU MIPS to execute other works in parallel?

> We also suggested a way how the SCU mode could be integrated into the
> generic MU support driver Oleksij posted and now you send a driver which
> uses the same name as Oleksijs driver, but it only and exclusively works in
> SCU mode. This doesn't bring us forward.
> 

Can Oleksij's patch be implemented against this one?
As I remember you said we've still not determined whether Oleksij's approach
is the most suitable way and it's still under discussion.
(Actually TI's approach looks better which is more simiar as SCU way?)

Furthermore, from this patch, you will notice that Oleksij's patch almost
did not work for SCU at all. I have to totally rewrite one for SCU.
So I did not write against his patch as it does not help.
And Oleksij's patch is quite simple while the SCU one is much complicated
than his one. So we probably better get SCU done first.

> We suggested a binding that allows coexisting of the SCU mode and the
> generic mode of the MU by putting the mode information into the second
> mbox-cell. Why don't you use this?
> 

You mean this?
+#define IMX_MU_CHANNEL0		0
+#define IMX_MU_CHANNEL1		1
+#define IMX_MU_CHANNEL2		2
+#define IMX_MU_CHANNEL3		3
+#define IMX_MU_CHANNEL_IMX8_SCU 4

It's hard for me to believe it's correct and it's over abstract to HW.
So I thought using mbox-cells to distinguish seems to be better.

> I don't think it's necessary to rewrite Oleksijs driver, instead it should rather
> be extended with the code I already provided as an example. With that we
> could make both of us happy since we can both have a suitable driver and
> even share most of the MU code.

As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I can't
see the meaning to demo the code on top of this driver. We can review
the SCU implementation directly with this driver which is more easy.
Then we can decide how to merge them together.

Regards
Dong Aisheng

> 
> Regards,
> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> Cb359a3eddee54bf1b40a08d5e6702f22%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C636668291863846639&amp;sdata=hSucaLRfCB1j1McwlfO%
> 2FL0921QXiHg68sl%2B23CvEp4Q%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Sascha Hauer]

On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> Hi Sascha,
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > Sent: Tuesday, July 10, 2018 10:20 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> > <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > shawnguo@kernel.org
> > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > 
> > Hi,
> > 
> > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > This is used for i.MX multi core communication.
> > > e.g. A core to SCU firmware(M core) on MX8.
> > >
> > > Tx is using polling mode while Rx is interrupt driven and schedule a
> > > hrtimer to receive remain words if have more than
> > > 4 words.
> > 
> > You told us that using interrupts is not possible due to miserable
> > performance, we then provided you a way with which you could poll.  Why
> > are you using interrupts now?
> > 
> 
> Because mailbox framework does not support sync rx now, I think we do not
> need to wait for that feature done first as it's independent and  separate
> features of framework.

You can wait forever for this feature, nobody will add it for you. It's
up to you to add support for that feature. Who else should add this
feature if not you? And when will you add that feature if not now when
you actually need it?
It is common practice that you adjust the frameworks to your needs
rather than working around them.

> 
> So for now, we're just using the common way in kernel as arm scpi and ti sci. 
> When framework supports it, we can easily switch to it.
> 
> I optimized the performance a bit by removing the unnecessary memcopy
> between tx/tx messages. The test result of booting time shows there's no
> obvious regressions. I'm not sure whether it's due to we're booting a minimum
> system or the extra cost is very minor to be noticed due to not too much cmds
> sent during booting.

Your boot time argument seems to be a very weak one. unnecessary memcpys
of a few bytes only take a fraction of the time interrupt latencies
introduce.

> (Copy Peng to comments more as he tried and reported that performance drop
> with vendor tree)
> 
> From the time measurement of sc_call_rpc, we can see that most rpc command
> In polling mode can finish within 10us and very rare ones over 20us.
> If switched to irq mode, those 10us cmds will change to about 20us. 
> 
> But the overall booting time did not increase much. Maybe the irq mode
> also saves some CPU MIPS to execute other works in parallel?
> 
> > We also suggested a way how the SCU mode could be integrated into the
> > generic MU support driver Oleksij posted and now you send a driver which
> > uses the same name as Oleksijs driver, but it only and exclusively works in
> > SCU mode. This doesn't bring us forward.
> > 
> 
> Can Oleksij's patch be implemented against this one?
> As I remember you said we've still not determined whether Oleksij's approach
> is the most suitable way and it's still under discussion.
> (Actually TI's approach looks better which is more simiar as SCU way?)
> 
> Furthermore, from this patch, you will notice that Oleksij's patch almost
> did not work for SCU at all. I have to totally rewrite one for SCU.

You are kidding. You have to rewrite it because you didn't follow my
suggestion to implement polling in the mailbox framework.

> So I did not write against his patch as it does not help.
> And Oleksij's patch is quite simple while the SCU one is much complicated
> than his one. So we probably better get SCU done first.
> 
> > We suggested a binding that allows coexisting of the SCU mode and the
> > generic mode of the MU by putting the mode information into the second
> > mbox-cell. Why don't you use this?
> > 
> 
> You mean this?
> +#define IMX_MU_CHANNEL0		0
> +#define IMX_MU_CHANNEL1		1
> +#define IMX_MU_CHANNEL2		2
> +#define IMX_MU_CHANNEL3		3
> +#define IMX_MU_CHANNEL_IMX8_SCU 4

Yes.

> 
> It's hard for me to believe it's correct and it's over abstract to HW.
> So I thought using mbox-cells to distinguish seems to be better.

The MU has four rx registers, four tx registers and four interrupts.
What else should this be than four channels? The number of channels the
MU has is obviously defined by the software running at the other end
of the MU. Yes, the SCU makes one channel from it, in other cases it's
four channels.

> 
> > I don't think it's necessary to rewrite Oleksijs driver, instead it should rather
> > be extended with the code I already provided as an example. With that we
> > could make both of us happy since we can both have a suitable driver and
> > even share most of the MU code.
> 
> As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I can't
> see the meaning to demo the code on top of this driver. We can review
> the SCU implementation directly with this driver which is more easy.
> Then we can decide how to merge them together.

You can only not share the code because the mailbox framework does not
support polling. And yes, I even provided some code which implements SCU
mode ontop of Oleksijs driver.

Honestly, what I expect from you is no rocket science and we would
already be there if we didn't spend our time discussing this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> Sent: Wednesday, July 11, 2018 3:55 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> > Hi Sascha,
> >
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > Sent: Tuesday, July 10, 2018 10:20 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
> > > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org;
> > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
> > > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> > > <fabio.estevam@nxp.com>; shawnguo@kernel.org
> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > >
> > > Hi,
> > >
> > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > > This is used for i.MX multi core communication.
> > > > e.g. A core to SCU firmware(M core) on MX8.
> > > >
> > > > Tx is using polling mode while Rx is interrupt driven and schedule
> > > > a hrtimer to receive remain words if have more than
> > > > 4 words.
> > >
> > > You told us that using interrupts is not possible due to miserable
> > > performance, we then provided you a way with which you could poll.
> > > Why are you using interrupts now?
> > >
> >
> > Because mailbox framework does not support sync rx now, I think we do
> > not need to wait for that feature done first as it's independent and
> > separate features of framework.
> 
> You can wait forever for this feature, nobody will add it for you. It's up to you
> to add support for that feature. Who else should add this feature if not you?
> And when will you add that feature if not now when you actually need it?
> It is common practice that you adjust the frameworks to your needs rather
> than working around them.
> 

I'm willing to add it. Just because you said Jassi already had the idea on how to
Implement it and does not add much complexity. So I just want to see his patches.
But if he did not work on it, I can also help on it.

Another reason is i did not observe regression as before, so it looks like the irq
is an applicable way currently and not must use polling at this point as that's
an independent new feature of framework. So irq mode is the quickest way
to demo the mbox using  as we already had experience before.

> >
> > So for now, we're just using the common way in kernel as arm scpi and ti sci.
> > When framework supports it, we can easily switch to it.
> >
> > I optimized the performance a bit by removing the unnecessary memcopy
> > between tx/tx messages. The test result of booting time shows there's
> > no obvious regressions. I'm not sure whether it's due to we're booting
> > a minimum system or the extra cost is very minor to be noticed due to
> > not too much cmds sent during booting.
> 
> Your boot time argument seems to be a very weak one. unnecessary
> memcpys of a few bytes only take a fraction of the time interrupt latencies
> introduce.
> 

Yes, interrupt latency may be the major part.

> > (Copy Peng to comments more as he tried and reported that performance
> > drop with vendor tree)
> >
> > From the time measurement of sc_call_rpc, we can see that most rpc
> > command In polling mode can finish within 10us and very rare ones over
> 20us.
> > If switched to irq mode, those 10us cmds will change to about 20us.
> >
> > But the overall booting time did not increase much. Maybe the irq mode
> > also saves some CPU MIPS to execute other works in parallel?
> >
> > > We also suggested a way how the SCU mode could be integrated into
> > > the generic MU support driver Oleksij posted and now you send a
> > > driver which uses the same name as Oleksijs driver, but it only and
> > > exclusively works in SCU mode. This doesn't bring us forward.
> > >
> >
> > Can Oleksij's patch be implemented against this one?
> > As I remember you said we've still not determined whether Oleksij's
> > approach is the most suitable way and it's still under discussion.
> > (Actually TI's approach looks better which is more simiar as SCU way?)
> >
> > Furthermore, from this patch, you will notice that Oleksij's patch
> > almost did not work for SCU at all. I have to totally rewrite one for SCU.
> 
> You are kidding. You have to rewrite it because you didn't follow my
> suggestion to implement polling in the mailbox framework.
> 

As I said, I did not observe regression when switch to irq way.
And irq way is the common mbox approach in kernel. So i just use
it to quickly demo the mbox using which we already implemented.
It's simple!

But I got your meaning that we must switch to polling mode
now, because we want to sync with M4, right?

> > So I did not write against his patch as it does not help.
> > And Oleksij's patch is quite simple while the SCU one is much
> > complicated than his one. So we probably better get SCU done first.
> >
> > > We suggested a binding that allows coexisting of the SCU mode and
> > > the generic mode of the MU by putting the mode information into the
> > > second mbox-cell. Why don't you use this?
> > >
> >
> > You mean this?
> > +#define IMX_MU_CHANNEL0		0
> > +#define IMX_MU_CHANNEL1		1
> > +#define IMX_MU_CHANNEL2		2
> > +#define IMX_MU_CHANNEL3		3
> > +#define IMX_MU_CHANNEL_IMX8_SCU 4
> 
> Yes.
> 
> >
> > It's hard for me to believe it's correct and it's over abstract to HW.
> > So I thought using mbox-cells to distinguish seems to be better.
> 
> The MU has four rx registers, four tx registers and four interrupts.
> What else should this be than four channels? The number of channels the
> MU has is obviously defined by the software running at the other end of the
> MU. Yes, the SCU makes one channel from it, in other cases it's four channels.
>

I'm not against 4 four channels. Just feel SCU using is strange because
SCU is using it as one channel from HW point of view. Not four.
And make them into four channels for M4 also lose the HW capability to send
Up to 4 words at one time. That's also an issue. 
(I guess we're assuming no actual users of multi words sending, right?)

So using mbox-cells to distinguish seems more reasonable.
e.g.
For one channel case: (communicate with SCU)
Mbox-cells = <0>;

Client_node {
	....
	Mbox =<&lsio_mu0>;
}

For 4 channel case: (communicate with M4)
Mbox-cells = <1>;

Client_node {
	....
	mbox =<&mu0 1>;
}

Anything wrong with it?

It seems more reflect the real HW and will not introduce confusion.

> >
> > > I don't think it's necessary to rewrite Oleksijs driver, instead it
> > > should rather be extended with the code I already provided as an
> > > example. With that we could make both of us happy since we can both
> > > have a suitable driver and even share most of the MU code.
> >
> > As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I
> > can't see the meaning to demo the code on top of this driver. We can
> > review the SCU implementation directly with this driver which is more easy.
> > Then we can decide how to merge them together.
> 
> You can only not share the code because the mailbox framework does not
> support polling. And yes, I even provided some code which implements SCU
> mode ontop of Oleksijs driver.
> 
> Honestly, what I expect from you is no rocket science and we would already
> be there if we didn't spend our time discussing this.
> 

I actually spent a lot time on it to switch to mbox to demo the using step by step,
and try to merge them with M4 users later. Although I don't think it's quite worthy
to do that, but I still do it Just because I respect you guys.

From our point of view, we just tried to be more careful about the change request
as we have more information than outside.

I also don't want to stop at this point. So I will try to add sync polling in mailbox
Framework and switch to it now.

Regards
Dong Aisheng

> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> Cb49570c607cf4057563d08d5e7039a70%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636668925017826212&amp;sdata=UT5AhUX%2FV7mCtUit0
> 1KSgJtf%2FdmOmmDMVz8EVebBaVk%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > Sent: Wednesday, July 11, 2018 3:55 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> > <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > shawnguo@kernel.org
> > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >
> > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> > > Hi Sascha,
> > >
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > > Sent: Tuesday, July 10, 2018 10:20 PM
> > > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
> > > > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org;
> > > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
> > > > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> > > > <fabio.estevam@nxp.com>; shawnguo@kernel.org
> > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > > >
> > > > Hi,
> > > >
> > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > > > This is used for i.MX multi core communication.
> > > > > e.g. A core to SCU firmware(M core) on MX8.
> > > > >
> > > > > Tx is using polling mode while Rx is interrupt driven and schedule
> > > > > a hrtimer to receive remain words if have more than
> > > > > 4 words.
> > > >
> > > > You told us that using interrupts is not possible due to miserable
> > > > performance, we then provided you a way with which you could poll.
> > > > Why are you using interrupts now?
> > > >
> > >
> > > Because mailbox framework does not support sync rx now, I think we do
> > > not need to wait for that feature done first as it's independent and
> > > separate features of framework.
> >
> > You can wait forever for this feature, nobody will add it for you. It's up to you
> > to add support for that feature. Who else should add this feature if not you?
> > And when will you add that feature if not now when you actually need it?
> > It is common practice that you adjust the frameworks to your needs rather
> > than working around them.
> >
>
> I'm willing to add it. Just because you said Jassi already had the idea on how to
> Implement it and does not add much complexity. So I just want to see his patches.
> But if he did not work on it, I can also help on it.
>
I am not much aware of the history of this conversation... but it
seems you need to make use of mbox_chan_ops.peek_data().

If not that, please let me know the requirement.

Cheers!

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Wednesday, July 11, 2018 6:44 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> kernel@lists.infradead.org; dongas86@gmail.com; linux-
> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > Sent: Wednesday, July 11, 2018 3:55 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
> > > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org;
> > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
> > > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> > > <fabio.estevam@nxp.com>; shawnguo@kernel.org
> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > >
> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> > > > Hi Sascha,
> > > >
> > > > > -----Original Message-----
> > > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
> > > > > linux-kernel@vger.kernel.org; Oleksij Rempel
> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > > > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > > > > shawnguo@kernel.org
> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > > > > This is used for i.MX multi core communication.
> > > > > > e.g. A core to SCU firmware(M core) on MX8.
> > > > > >
> > > > > > Tx is using polling mode while Rx is interrupt driven and
> > > > > > schedule a hrtimer to receive remain words if have more than
> > > > > > 4 words.
> > > > >
> > > > > You told us that using interrupts is not possible due to
> > > > > miserable performance, we then provided you a way with which you
> could poll.
> > > > > Why are you using interrupts now?
> > > > >
> > > >
> > > > Because mailbox framework does not support sync rx now, I think we
> > > > do not need to wait for that feature done first as it's
> > > > independent and separate features of framework.
> > >
> > > You can wait forever for this feature, nobody will add it for you.
> > > It's up to you to add support for that feature. Who else should add this
> feature if not you?
> > > And when will you add that feature if not now when you actually need it?
> > > It is common practice that you adjust the frameworks to your needs
> > > rather than working around them.
> > >
> >
> > I'm willing to add it. Just because you said Jassi already had the
> > idea on how to Implement it and does not add much complexity. So I just
> want to see his patches.
> > But if he did not work on it, I can also help on it.
> >
> I am not much aware of the history of this conversation... but it seems you
> need to make use of mbox_chan_ops.peek_data().
> 
> If not that, please let me know the requirement.
> 

Thanks for the suggestion.
It looks to me may work.

From the definition, it seems it's used to pull data from remote side.
/**
 * mbox_client_peek_data - A way for client driver to pull data
 *                      received from remote by the controller.
 * @chan: Mailbox channel assigned to this client.
 *          
 * A poke to controller driver for any received data.
 * The data is actually passed onto client via the
 * mbox_chan_received_data()
 * The call can be made from atomic context, so the controller's
 * implementation of peek_data() must not sleep.
 *
 * Return: True, if controller has, and is going to push after this,
 *          some data.
 *         False, if controller doesn't have any data to be read.
 */     
bool mbox_client_peek_data(struct mbox_chan *chan)
{
        if (chan->mbox->ops->peek_data)
                return chan->mbox->ops->peek_data(chan);
        
        return false;
}       
EXPORT_SYMBOL_GPL(mbox_client_peek_data);
But it seems most users in kernel simply implement it as a data available
Checking rather than receiving it.
See:
drivers/mailbox/ti-msgmgr.c
drivers/mailbox/mailbox-altera.c

Only bcm uses it to receive data.
drivers/mailbox/bcm-flexrm-mailbox.c

For our requirement, we want to implement sync receiving protocol like:
Sc_call_rpc()
{
	mbox_send_message(chan, msg)
	If (!no_resp)
		// rx also stored in msg
		mbox_receive_msg_in_polling(chan, msg);
	mbox_client_txdone();
}

If using peek_data, it can be:
Sc_call_rpc()
{
	mbox_send_message(chan, msg)
	If (!no_resp)
		// rx also stored in msg
		Mbox_client_peek_data(chan);
	mbox_client_txdone();
}

And for mu controller driver .peek_data():
imx_mu_peek_data(chan)
{
        // get first word and parse data size
        imx_mu_receive_msg(&mu->chans, 0, mu->msg);

        raw_data = (u8 *)mu->msg;
        size = raw_data[1];

        // receive rest of them 
        for (i = 1; i < size; i++) {
                ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
                if (ret)
                        return false;
        }

        mbox_chan_received_data(&mu->chans, (void *)mu->msg);

        return true;
}
It is much specific to SCU.
Do you think such peek_data using for sync receiving mode is ok?

Regards
Dong Aisheng

> Cheers!

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> Sent: Wednesday, July 11, 2018 6:44 PM
>> To: A.s. Dong <aisheng.dong@nxp.com>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
>> kernel@lists.infradead.org; dongas86@gmail.com; linux-
>> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
>> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>; shawnguo@kernel.org
>> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>>
>> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> >
>> > > -----Original Message-----
>> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
>> > > Sent: Wednesday, July 11, 2018 3:55 PM
>> > > To: A.s. Dong <aisheng.dong@nxp.com>
>> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
>> > > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org;
>> > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
>> > > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
>> > > <fabio.estevam@nxp.com>; shawnguo@kernel.org
>> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> > >
>> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
>> > > > Hi Sascha,
>> > > >
>> > > > > -----Original Message-----
>> > > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
>> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
>> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
>> > > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
>> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
>> > > > > linux-kernel@vger.kernel.org; Oleksij Rempel
>> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
>> > > > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
>> > > > > shawnguo@kernel.org
>> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> > > > >
>> > > > > Hi,
>> > > > >
>> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
>> > > > > > This is used for i.MX multi core communication.
>> > > > > > e.g. A core to SCU firmware(M core) on MX8.
>> > > > > >
>> > > > > > Tx is using polling mode while Rx is interrupt driven and
>> > > > > > schedule a hrtimer to receive remain words if have more than
>> > > > > > 4 words.
>> > > > >
>> > > > > You told us that using interrupts is not possible due to
>> > > > > miserable performance, we then provided you a way with which you
>> could poll.
>> > > > > Why are you using interrupts now?
>> > > > >
>> > > >
>> > > > Because mailbox framework does not support sync rx now, I think we
>> > > > do not need to wait for that feature done first as it's
>> > > > independent and separate features of framework.
>> > >
>> > > You can wait forever for this feature, nobody will add it for you.
>> > > It's up to you to add support for that feature. Who else should add this
>> feature if not you?
>> > > And when will you add that feature if not now when you actually need it?
>> > > It is common practice that you adjust the frameworks to your needs
>> > > rather than working around them.
>> > >
>> >
>> > I'm willing to add it. Just because you said Jassi already had the
>> > idea on how to Implement it and does not add much complexity. So I just
>> want to see his patches.
>> > But if he did not work on it, I can also help on it.
>> >
>> I am not much aware of the history of this conversation... but it seems you
>> need to make use of mbox_chan_ops.peek_data().
>>
>> If not that, please let me know the requirement.
>>
>
> Thanks for the suggestion.
> It looks to me may work.
>
> From the definition, it seems it's used to pull data from remote side.
> /**
>  * mbox_client_peek_data - A way for client driver to pull data
>  *                      received from remote by the controller.
>  * @chan: Mailbox channel assigned to this client.
>  *
>  * A poke to controller driver for any received data.
>  * The data is actually passed onto client via the
>  * mbox_chan_received_data()
>  * The call can be made from atomic context, so the controller's
>  * implementation of peek_data() must not sleep.
>  *
>  * Return: True, if controller has, and is going to push after this,
>  *          some data.
>  *         False, if controller doesn't have any data to be read.
>  */
> bool mbox_client_peek_data(struct mbox_chan *chan)
> {
>         if (chan->mbox->ops->peek_data)
>                 return chan->mbox->ops->peek_data(chan);
>
>         return false;
> }
> EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> But it seems most users in kernel simply implement it as a data available
> Checking rather than receiving it.
> See:
> drivers/mailbox/ti-msgmgr.c
> drivers/mailbox/mailbox-altera.c
>
> Only bcm uses it to receive data.
> drivers/mailbox/bcm-flexrm-mailbox.c
>
> For our requirement, we want to implement sync receiving protocol like:
> Sc_call_rpc()
> {
>         mbox_send_message(chan, msg)
>         If (!no_resp)
>                 // rx also stored in msg
>                 mbox_receive_msg_in_polling(chan, msg);
>         mbox_client_txdone();
> }
>
> If using peek_data, it can be:
> Sc_call_rpc()
> {
>         mbox_send_message(chan, msg)
>         If (!no_resp)
>                 // rx also stored in msg
>                 Mbox_client_peek_data(chan);
>
Yes, and you may want to loop for a certain amount of time if peek
keeps returning false.

>         mbox_client_txdone();
> }
>
> And for mu controller driver .peek_data():
> imx_mu_peek_data(chan)
> {
>         // get first word and parse data size
>         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
>
>         raw_data = (u8 *)mu->msg;
>         size = raw_data[1];
>
>         // receive rest of them
>         for (i = 1; i < size; i++) {
>                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
>                 if (ret)
>                         return false;
>         }
>
>         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
>
Not sure how your controller works. But the peek() callback only
_checks_ if there is some data available to be read. Please note that
peek() can not sleep.
So if the data fetching doesn't sleep you can do that here, otherwise
peek has to schedule the actual fetching of data from remote and
providing to the client via mbox_chan_received_data.

-jassi

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 12, 2018 12:32 AM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> kernel@lists.infradead.org; dongas86@gmail.com; linux-
> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> > Hi Jassi,
> >
> >> -----Original Message-----
> >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> >> Sent: Wednesday, July 11, 2018 6:44 PM
> >> To: A.s. Dong <aisheng.dong@nxp.com>
> >> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> >> kernel@lists.infradead.org; dongas86@gmail.com; linux-
> >> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> >> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> >> <fabio.estevam@nxp.com>; shawnguo@kernel.org
> >> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >>
> >> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com>
> wrote:
> >> >
> >> > > -----Original Message-----
> >> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> >> > > Sent: Wednesday, July 11, 2018 3:55 PM
> >> > > To: A.s. Dong <aisheng.dong@nxp.com>
> >> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
> >> > > Jassi Brar <jassisinghbrar@gmail.com>;
> >> > > linux-kernel@vger.kernel.org; Oleksij Rempel
> >> > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> >> > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> >> > > shawnguo@kernel.org
> >> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >> > >
> >> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> >> > > > Hi Sascha,
> >> > > >
> >> > > > > -----Original Message-----
> >> > > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> >> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
> >> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
> >> > > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
> >> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
> >> > > > > linux-kernel@vger.kernel.org; Oleksij Rempel
> >> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> >> > > > > kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>;
> >> > > > > shawnguo@kernel.org
> >> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >> > > > >
> >> > > > > Hi,
> >> > > > >
> >> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> >> > > > > > This is used for i.MX multi core communication.
> >> > > > > > e.g. A core to SCU firmware(M core) on MX8.
> >> > > > > >
> >> > > > > > Tx is using polling mode while Rx is interrupt driven and
> >> > > > > > schedule a hrtimer to receive remain words if have more
> >> > > > > > than
> >> > > > > > 4 words.
> >> > > > >
> >> > > > > You told us that using interrupts is not possible due to
> >> > > > > miserable performance, we then provided you a way with which
> >> > > > > you
> >> could poll.
> >> > > > > Why are you using interrupts now?
> >> > > > >
> >> > > >
> >> > > > Because mailbox framework does not support sync rx now, I think
> >> > > > we do not need to wait for that feature done first as it's
> >> > > > independent and separate features of framework.
> >> > >
> >> > > You can wait forever for this feature, nobody will add it for you.
> >> > > It's up to you to add support for that feature. Who else should
> >> > > add this
> >> feature if not you?
> >> > > And when will you add that feature if not now when you actually need
> it?
> >> > > It is common practice that you adjust the frameworks to your
> >> > > needs rather than working around them.
> >> > >
> >> >
> >> > I'm willing to add it. Just because you said Jassi already had the
> >> > idea on how to Implement it and does not add much complexity. So I
> >> > just
> >> want to see his patches.
> >> > But if he did not work on it, I can also help on it.
> >> >
> >> I am not much aware of the history of this conversation... but it
> >> seems you need to make use of mbox_chan_ops.peek_data().
> >>
> >> If not that, please let me know the requirement.
> >>
> >
> > Thanks for the suggestion.
> > It looks to me may work.
> >
> > From the definition, it seems it's used to pull data from remote side.
> > /**
> >  * mbox_client_peek_data - A way for client driver to pull data
> >  *                      received from remote by the controller.
> >  * @chan: Mailbox channel assigned to this client.
> >  *
> >  * A poke to controller driver for any received data.
> >  * The data is actually passed onto client via the
> >  * mbox_chan_received_data()
> >  * The call can be made from atomic context, so the controller's
> >  * implementation of peek_data() must not sleep.
> >  *
> >  * Return: True, if controller has, and is going to push after this,
> >  *          some data.
> >  *         False, if controller doesn't have any data to be read.
> >  */
> > bool mbox_client_peek_data(struct mbox_chan *chan) {
> >         if (chan->mbox->ops->peek_data)
> >                 return chan->mbox->ops->peek_data(chan);
> >
> >         return false;
> > }
> > EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> > But it seems most users in kernel simply implement it as a data
> > available Checking rather than receiving it.
> > See:
> > drivers/mailbox/ti-msgmgr.c
> > drivers/mailbox/mailbox-altera.c
> >
> > Only bcm uses it to receive data.
> > drivers/mailbox/bcm-flexrm-mailbox.c
> >
> > For our requirement, we want to implement sync receiving protocol like:
> > Sc_call_rpc()
> > {
> >         mbox_send_message(chan, msg)
> >         If (!no_resp)
> >                 // rx also stored in msg
> >                 mbox_receive_msg_in_polling(chan, msg);
> >         mbox_client_txdone();
> > }
> >
> > If using peek_data, it can be:
> > Sc_call_rpc()
> > {
> >         mbox_send_message(chan, msg)
> >         If (!no_resp)
> >                 // rx also stored in msg
> >                 Mbox_client_peek_data(chan);
> >
> Yes, and you may want to loop for a certain amount of time if peek keeps
> returning false.
> 
> >         mbox_client_txdone();
> > }
> >
> > And for mu controller driver .peek_data():
> > imx_mu_peek_data(chan)
> > {
> >         // get first word and parse data size
> >         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
> >
> >         raw_data = (u8 *)mu->msg;
> >         size = raw_data[1];
> >
> >         // receive rest of them
> >         for (i = 1; i < size; i++) {
> >                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
> >                 if (ret)
> >                         return false;
> >         }
> >
> >         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
> >
> Not sure how your controller works. But the peek() callback only _checks_ if
> there is some data available to be read. Please note that
> peek() can not sleep.
> So if the data fetching doesn't sleep you can do that here, otherwise peek
> has to schedule the actual fetching of data from remote and providing to the
> client via mbox_chan_received_data.
> 

bcm seems is using peek to receive data, not only checking the data availability,
right?
drivers/mailbox/bcm-flexrm-mailbox.c

So I did the similar way for i.MX. I just sent out that new patch series.
Please help review if any problem of it.
BTW i.MX peek is using busy polling, so won't sleep.

Regards
Dong Aisheng

> -jassi

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 10:11 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> Sent: Thursday, July 12, 2018 12:32 AM
>> To: A.s. Dong <aisheng.dong@nxp.com>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
>> kernel@lists.infradead.org; dongas86@gmail.com; linux-
>> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
>> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>; shawnguo@kernel.org
>> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>>
>> On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> > Hi Jassi,
>> >
>> >> -----Original Message-----
>> >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> >> Sent: Wednesday, July 11, 2018 6:44 PM
>> >> To: A.s. Dong <aisheng.dong@nxp.com>
>> >> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
>> >> kernel@lists.infradead.org; dongas86@gmail.com; linux-
>> >> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
>> >> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
>> >> <fabio.estevam@nxp.com>; shawnguo@kernel.org
>> >> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> >>
>> >> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com>
>> wrote:
>> >> >
>> >> > > -----Original Message-----
>> >> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
>> >> > > Sent: Wednesday, July 11, 2018 3:55 PM
>> >> > > To: A.s. Dong <aisheng.dong@nxp.com>
>> >> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
>> >> > > Jassi Brar <jassisinghbrar@gmail.com>;
>> >> > > linux-kernel@vger.kernel.org; Oleksij Rempel
>> >> > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
>> >> > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
>> >> > > shawnguo@kernel.org
>> >> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> >> > >
>> >> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
>> >> > > > Hi Sascha,
>> >> > > >
>> >> > > > > -----Original Message-----
>> >> > > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
>> >> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
>> >> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
>> >> > > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
>> >> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
>> >> > > > > linux-kernel@vger.kernel.org; Oleksij Rempel
>> >> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
>> >> > > > > kernel@pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>;
>> >> > > > > shawnguo@kernel.org
>> >> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> >> > > > >
>> >> > > > > Hi,
>> >> > > > >
>> >> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
>> >> > > > > > This is used for i.MX multi core communication.
>> >> > > > > > e.g. A core to SCU firmware(M core) on MX8.
>> >> > > > > >
>> >> > > > > > Tx is using polling mode while Rx is interrupt driven and
>> >> > > > > > schedule a hrtimer to receive remain words if have more
>> >> > > > > > than
>> >> > > > > > 4 words.
>> >> > > > >
>> >> > > > > You told us that using interrupts is not possible due to
>> >> > > > > miserable performance, we then provided you a way with which
>> >> > > > > you
>> >> could poll.
>> >> > > > > Why are you using interrupts now?
>> >> > > > >
>> >> > > >
>> >> > > > Because mailbox framework does not support sync rx now, I think
>> >> > > > we do not need to wait for that feature done first as it's
>> >> > > > independent and separate features of framework.
>> >> > >
>> >> > > You can wait forever for this feature, nobody will add it for you.
>> >> > > It's up to you to add support for that feature. Who else should
>> >> > > add this
>> >> feature if not you?
>> >> > > And when will you add that feature if not now when you actually need
>> it?
>> >> > > It is common practice that you adjust the frameworks to your
>> >> > > needs rather than working around them.
>> >> > >
>> >> >
>> >> > I'm willing to add it. Just because you said Jassi already had the
>> >> > idea on how to Implement it and does not add much complexity. So I
>> >> > just
>> >> want to see his patches.
>> >> > But if he did not work on it, I can also help on it.
>> >> >
>> >> I am not much aware of the history of this conversation... but it
>> >> seems you need to make use of mbox_chan_ops.peek_data().
>> >>
>> >> If not that, please let me know the requirement.
>> >>
>> >
>> > Thanks for the suggestion.
>> > It looks to me may work.
>> >
>> > From the definition, it seems it's used to pull data from remote side.
>> > /**
>> >  * mbox_client_peek_data - A way for client driver to pull data
>> >  *                      received from remote by the controller.
>> >  * @chan: Mailbox channel assigned to this client.
>> >  *
>> >  * A poke to controller driver for any received data.
>> >  * The data is actually passed onto client via the
>> >  * mbox_chan_received_data()
>> >  * The call can be made from atomic context, so the controller's
>> >  * implementation of peek_data() must not sleep.
>> >  *
>> >  * Return: True, if controller has, and is going to push after this,
>> >  *          some data.
>> >  *         False, if controller doesn't have any data to be read.
>> >  */
>> > bool mbox_client_peek_data(struct mbox_chan *chan) {
>> >         if (chan->mbox->ops->peek_data)
>> >                 return chan->mbox->ops->peek_data(chan);
>> >
>> >         return false;
>> > }
>> > EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>> > But it seems most users in kernel simply implement it as a data
>> > available Checking rather than receiving it.
>> > See:
>> > drivers/mailbox/ti-msgmgr.c
>> > drivers/mailbox/mailbox-altera.c
>> >
>> > Only bcm uses it to receive data.
>> > drivers/mailbox/bcm-flexrm-mailbox.c
>> >
>> > For our requirement, we want to implement sync receiving protocol like:
>> > Sc_call_rpc()
>> > {
>> >         mbox_send_message(chan, msg)
>> >         If (!no_resp)
>> >                 // rx also stored in msg
>> >                 mbox_receive_msg_in_polling(chan, msg);
>> >         mbox_client_txdone();
>> > }
>> >
>> > If using peek_data, it can be:
>> > Sc_call_rpc()
>> > {
>> >         mbox_send_message(chan, msg)
>> >         If (!no_resp)
>> >                 // rx also stored in msg
>> >                 Mbox_client_peek_data(chan);
>> >
>> Yes, and you may want to loop for a certain amount of time if peek keeps
>> returning false.
>>
>> >         mbox_client_txdone();
>> > }
>> >
>> > And for mu controller driver .peek_data():
>> > imx_mu_peek_data(chan)
>> > {
>> >         // get first word and parse data size
>> >         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
>> >
>> >         raw_data = (u8 *)mu->msg;
>> >         size = raw_data[1];
>> >
>> >         // receive rest of them
>> >         for (i = 1; i < size; i++) {
>> >                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
>> >                 if (ret)
>> >                         return false;
>> >         }
>> >
>> >         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
>> >
>> Not sure how your controller works. But the peek() callback only _checks_ if
>> there is some data available to be read. Please note that
>> peek() can not sleep.
>> So if the data fetching doesn't sleep you can do that here, otherwise peek
>> has to schedule the actual fetching of data from remote and providing to the
>> client via mbox_chan_received_data.
>>
>
> bcm seems is using peek to receive data, not only checking the data availability,
> right?
> drivers/mailbox/bcm-flexrm-mailbox.c
>
As I said, if fetching data from remote don't need to sleep, you can
call mbox_chan_received_data() from peek(). Otherwise not.

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 12, 2018 1:01 AM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> kernel@lists.infradead.org; dongas86@gmail.com; linux-
> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

....

> >
> > bcm seems is using peek to receive data, not only checking the data
> > availability, right?
> > drivers/mailbox/bcm-flexrm-mailbox.c
> >
> As I said, if fetching data from remote don't need to sleep, you can call
> mbox_chan_received_data() from peek(). Otherwise not.

Got it. So it seems work for i.MX.
Thanks.

Regards
Dong Aisheng

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Peng Fan]

Hi A.S

> -----Original Message-----
> From: A.s. Dong
> Sent: 2018年7月11日 15:30
> To: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo@kernel.org
> Subject: RE: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> Hi Sascha,
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > Sent: Tuesday, July 10, 2018 10:20 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
> > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij
> > Rempel <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > shawnguo@kernel.org
> > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >
> > Hi,
> >
> > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > This is used for i.MX multi core communication.
> > > e.g. A core to SCU firmware(M core) on MX8.
> > >
> > > Tx is using polling mode while Rx is interrupt driven and schedule a
> > > hrtimer to receive remain words if have more than
> > > 4 words.
> >
> > You told us that using interrupts is not possible due to miserable
> > performance, we then provided you a way with which you could poll.
> > Why are you using interrupts now?
> >
> 
> Because mailbox framework does not support sync rx now, I think we do not
> need to wait for that feature done first as it's independent and  separate
> features of framework.
> 
> So for now, we're just using the common way in kernel as arm scpi and ti sci.
> When framework supports it, we can easily switch to it.
> 
> I optimized the performance a bit by removing the unnecessary memcopy
> between tx/tx messages. The test result of booting time shows there's no
> obvious regressions. I'm not sure whether it's due to we're booting a minimum
> system or the extra cost is very minor to be noticed due to not too much cmds
> sent during booting.
> (Copy Peng to comments more as he tried and reported that performance drop
> with vendor tree)

The txpoll_period is set 1, the minimum is 1ms. So it introduces latency in the initial mailbox
for SCU communication.

Regards,
Peng

> 
> From the time measurement of sc_call_rpc, we can see that most rpc command
> In polling mode can finish within 10us and very rare ones over 20us.
> If switched to irq mode, those 10us cmds will change to about 20us.
> 
> But the overall booting time did not increase much. Maybe the irq mode also
> saves some CPU MIPS to execute other works in parallel?
> 
> > We also suggested a way how the SCU mode could be integrated into the
> > generic MU support driver Oleksij posted and now you send a driver
> > which uses the same name as Oleksijs driver, but it only and
> > exclusively works in SCU mode. This doesn't bring us forward.
> >
> 
> Can Oleksij's patch be implemented against this one?
> As I remember you said we've still not determined whether Oleksij's approach is
> the most suitable way and it's still under discussion.
> (Actually TI's approach looks better which is more simiar as SCU way?)
> 
> Furthermore, from this patch, you will notice that Oleksij's patch almost did not
> work for SCU at all. I have to totally rewrite one for SCU.
> So I did not write against his patch as it does not help.
> And Oleksij's patch is quite simple while the SCU one is much complicated than
> his one. So we probably better get SCU done first.
> 
> > We suggested a binding that allows coexisting of the SCU mode and the
> > generic mode of the MU by putting the mode information into the second
> > mbox-cell. Why don't you use this?
> >
> 
> You mean this?
> +#define IMX_MU_CHANNEL0		0
> +#define IMX_MU_CHANNEL1		1
> +#define IMX_MU_CHANNEL2		2
> +#define IMX_MU_CHANNEL3		3
> +#define IMX_MU_CHANNEL_IMX8_SCU 4
> 
> It's hard for me to believe it's correct and it's over abstract to HW.
> So I thought using mbox-cells to distinguish seems to be better.
> 
> > I don't think it's necessary to rewrite Oleksijs driver, instead it
> > should rather be extended with the code I already provided as an
> > example. With that we could make both of us happy since we can both
> > have a suitable driver and even share most of the MU code.
> 
> As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I can't see the
> meaning to demo the code on top of this driver. We can review the SCU
> implementation directly with this driver which is more easy.
> Then we can decide how to merge them together.
> 
> Regards
> Dong Aisheng
> 
> >
> > Regards,
> > Sascha
> >
> > --
> > Pengutronix e.K.                           |
> |
> > Industrial Linux Solutions                 |
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> > w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> >
> Cb359a3eddee54bf1b40a08d5e6702f22%7C686ea1d3bc2b4c6fa92cd99c5c301
> >
> 635%7C0%7C0%7C636668291863846639&amp;sdata=hSucaLRfCB1j1McwlfO%
> > 2FL0921QXiHg68sl%2B23CvEp4Q%3D&amp;reserved=0  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |