[PATCH V3 0/4] mailbox/firmware: imx: support SCU channel type

[PATCH V3 0/4] mailbox/firmware: imx: support SCU channel type

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>

V3:
 Rebase to Shawn's for-next
 Include fsl,imx8-mu-scu compatible
 Per Oleksij's comments, introduce generic tx/rx and added scu mu type
 Check fsl,imx8-mu-scu in firmware driver for fast_ipc

V2:
 Drop patch 1/3 which added fsl,scu property
 Force to use scu channel type when machine has node compatible "fsl,imx-scu"
 Force imx-scu to use fast_ipc

 I not found a generic method to make SCFW message generic enough, SCFW
 message is not fixed length including TX and RX. And it use TR0/RR0
 interrupt.

 It might need https://patchwork.kernel.org/patch/11395247/ to avoid
 build break.

V1:
Sorry to bind the mailbox/firmware patch together. This is make it
to understand what changed to support using 1 TX and 1 RX channel
for SCFW message.

Per i.MX8QXP Reference mannual, there are several message using
examples. One of them is:
Passing short messages: Transmit register(s) can be used to pass
short messages from one to four words in length. For example,
when a four-word message is desired, only one of the registers
needs to have its corresponding interrupt enable bit set at the
receiver side.

This patchset is to using this for SCFW message to replace four TX
and four RX method.


Peng Fan (4):
  dt-bindings: mailbox: imx-mu: add SCU MU support
  mailbox: imx: restructure code to make easy for new MU
  mailbox: imx: add SCU MU support
  firmware: imx-scu: Support one TX and one RX

 .../devicetree/bindings/mailbox/fsl,mu.txt         |   2 +
 drivers/firmware/imx/imx-scu.c                     |  54 +++++--
 drivers/mailbox/imx-mailbox.c                      | 165 ++++++++++++++++-----
 3 files changed, 176 insertions(+), 45 deletions(-)


base-commit: 48b4bfe7105f646e270596bf9d22df0e8a4ae217
-- 
2.16.4

[PATCH V3 1/4] dt-bindings: mailbox: imx-mu: add SCU MU support

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>

i.MX8/8X SCU MU is dedicated for communication between SCU
and Cortex-A cores from hardware design, it could not be reused
for other purpose. To use SCU MU more effectivly, add "fsl,imx8-scu-mu"
compatile to support fast IPC.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V3:
 New patch

 Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
index 9c43357c5924..31486c9f6443 100644
--- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -23,6 +23,8 @@ Required properties:
 		be included together with SoC specific compatible.
 		There is a version 1.0 MU on imx7ulp, use "fsl,imx7ulp-mu"
 		compatible to support it.
+		To communicate with i.MX8 SCU, "fsl,imx8-mu-scu" could be
+		used for fast IPC
 - reg :		Should contain the registers location and length
 - interrupts :	Interrupt number. The interrupt specifier format depends
 		on the interrupt controller parent.
-- 
2.16.4

[PATCH V3 2/4] mailbox: imx: restructure code to make easy for new MU

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>

Add imx_mu_generic_tx for data send and imx_mu_generic_rx for interrupt
data receive.
Add 'type' for MU type.

With these, it will be a bit easy to introduce i.MX8/8X SCU type
MU dedicated to communicate with SCU.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V3:
 New patch, restructure code.

 drivers/mailbox/imx-mailbox.c | 100 ++++++++++++++++++++++++++++--------------
 1 file changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 2cdcdc5f1119..901a3431fdb5 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -36,7 +36,17 @@ enum imx_mu_chan_type {
 	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
 };
 
+enum imx_mu_type {
+	IMX_MU_TYPE_GENERIC,
+};
+
+struct imx_mu_priv;
+struct imx_mu_con_priv;
+
 struct imx_mu_dcfg {
+	enum imx_mu_type type;
+	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, void *data);
+	int (*rx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp);
 	u32	xTR[4];		/* Transmit Registers */
 	u32	xRR[4];		/* Receive Registers */
 	u32	xSR;		/* Status Register */
@@ -67,20 +77,6 @@ struct imx_mu_priv {
 	bool			side_b;
 };
 
-static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
-	.xTR	= {0x0, 0x4, 0x8, 0xc},
-	.xRR	= {0x10, 0x14, 0x18, 0x1c},
-	.xSR	= 0x20,
-	.xCR	= 0x24,
-};
-
-static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
-	.xTR	= {0x20, 0x24, 0x28, 0x2c},
-	.xRR	= {0x40, 0x44, 0x48, 0x4c},
-	.xSR	= 0x60,
-	.xCR	= 0x64,
-};
-
 static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
 {
 	return container_of(mbox, struct imx_mu_priv, mbox);
@@ -111,6 +107,40 @@ static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, u32 set, u32 clr)
 	return val;
 }
 
+static int imx_mu_generic_tx(struct imx_mu_priv *priv,
+			     struct imx_mu_con_priv *cp,
+			     void *data)
+{
+	u32 *arg = data;
+
+	switch (cp->type) {
+	case IMX_MU_TYPE_TX:
+		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
+		break;
+	case IMX_MU_TYPE_TXDB:
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
+		tasklet_schedule(&cp->txdb_tasklet);
+		break;
+	default:
+		dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int imx_mu_generic_rx(struct imx_mu_priv *priv,
+			     struct imx_mu_con_priv *cp)
+{
+	u32 dat;
+
+	dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
+	mbox_chan_received_data(cp->chan, (void *)&dat);
+
+	return 0;
+}
+
 static void imx_mu_txdb_tasklet(unsigned long data)
 {
 	struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data;
@@ -123,7 +153,7 @@ 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, ctrl, dat;
+	u32 val, ctrl;
 
 	ctrl = imx_mu_read(priv, priv->dcfg->xCR);
 	val = imx_mu_read(priv, priv->dcfg->xSR);
@@ -152,8 +182,7 @@ static irqreturn_t imx_mu_isr(int irq, void *p)
 		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
 		mbox_chan_txdone(chan, 0);
 	} else if (val == IMX_MU_xSR_RFn(cp->idx)) {
-		dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
-		mbox_chan_received_data(chan, (void *)&dat);
+		priv->dcfg->rx(priv, cp);
 	} else if (val == IMX_MU_xSR_GIPn(cp->idx)) {
 		imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx), priv->dcfg->xSR);
 		mbox_chan_received_data(chan, NULL);
@@ -169,23 +198,8 @@ 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;
-
-	switch (cp->type) {
-	case IMX_MU_TYPE_TX:
-		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
-		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
-		break;
-	case IMX_MU_TYPE_TXDB:
-		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
-		tasklet_schedule(&cp->txdb_tasklet);
-		break;
-	default:
-		dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type);
-		return -EINVAL;
-	}
 
-	return 0;
+	return priv->dcfg->tx(priv, cp, data);
 }
 
 static int imx_mu_startup(struct mbox_chan *chan)
@@ -367,6 +381,26 @@ static int imx_mu_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
+	.type	= IMX_MU_TYPE_GENERIC,
+	.tx	= imx_mu_generic_tx,
+	.rx	= imx_mu_generic_rx,
+	.xTR	= {0x0, 0x4, 0x8, 0xc},
+	.xRR	= {0x10, 0x14, 0x18, 0x1c},
+	.xSR	= 0x20,
+	.xCR	= 0x24,
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
+	.type	= IMX_MU_TYPE_GENERIC,
+	.tx	= imx_mu_generic_tx,
+	.rx	= imx_mu_generic_rx,
+	.xTR	= {0x20, 0x24, 0x28, 0x2c},
+	.xRR	= {0x40, 0x44, 0x48, 0x4c},
+	.xSR	= 0x60,
+	.xCR	= 0x64,
+};
+
 static const struct of_device_id imx_mu_dt_ids[] = {
 	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp },
 	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx },
-- 
2.16.4

[PATCH V3 3/4] mailbox: imx: add SCU MU support

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>

i.MX8/8X SCU MU is dedicated for communication between SCU and Cortex-A
cores from hardware design, and could not be reused for other purpose.

Per i.MX8/8X Reference mannual, Chapter "12.9.2.3.2 Messaging Examples",
 Passing short messages: Transmit register(s) can be used to pass
 short messages from one to four words in length. For example, when
 a four-word message is desired, only one of the registers needs to
 have its corresponding interrupt enable bit set at the receiver side;
 the message’s first three words are written to the registers whose
 interrupt is masked, and the fourth word is written to the other
 register (which triggers an interrupt at the receiver side).

i.MX8/8X SCU firmware IPC is an implementation of passing short
messages. But current imx-mailbox driver only support one word
message, i.MX8/8X linux side firmware has to request four TX
and four RX to support IPC to SCU firmware. This is low efficent
and more interrupts triggered compared with one TX and
one RX.

To make SCU MU work,
  - parse the size of msg.
  - Only enable TR0/RR0 interrupt for transmit/receive message.
  - For TX/RX, only support one TX channel and one RX channel

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V3:
 Added scu type tx/rx and SCU MU type

 drivers/mailbox/imx-mailbox.c | 65 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 901a3431fdb5..41664a64c5fd 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/firmware/imx/ipc.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -38,11 +39,17 @@ enum imx_mu_chan_type {
 
 enum imx_mu_type {
 	IMX_MU_TYPE_GENERIC,
+	IMX_MU_TYPE_SCU,
 };
 
 struct imx_mu_priv;
 struct imx_mu_con_priv;
 
+struct imx_sc_rpc_msg_max {
+	struct imx_sc_rpc_msg hdr;
+	u32 data[7];
+} __packed __aligned(4);;
+
 struct imx_mu_dcfg {
 	enum imx_mu_type type;
 	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, void *data);
@@ -141,6 +148,48 @@ static int imx_mu_generic_rx(struct imx_mu_priv *priv,
 	return 0;
 }
 
+static int imx_mu_scu_tx(struct imx_mu_priv *priv,
+			 struct imx_mu_con_priv *cp,
+			 void *data)
+{
+	struct imx_sc_rpc_msg_max *msg = data;
+	u32 *arg = data;
+	int i;
+
+	switch (cp->type) {
+	case IMX_MU_TYPE_TX:
+		for (i = 0; i < msg->hdr.size; i++) {
+			imx_mu_write(priv, *arg++,
+				     priv->dcfg->xTR[i % 4]);
+		}
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
+		break;
+	default:
+		dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int imx_mu_scu_rx(struct imx_mu_priv *priv,
+			 struct imx_mu_con_priv *cp)
+{
+	struct imx_sc_rpc_msg_max msg;
+	u32 *data = (u32 *)&msg;
+	int i;
+
+	imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0));
+	*data++ = imx_mu_read(priv, priv->dcfg->xRR[0]);
+	for (i = 1; i < msg.hdr.size; i++)
+		*data++ = imx_mu_read(priv, priv->dcfg->xRR[i % 4]);
+
+	imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(0), 0);
+	mbox_chan_received_data(cp->chan, (void *)&msg);
+
+	return 0;
+}
+
 static void imx_mu_txdb_tasklet(unsigned long data)
 {
 	struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data;
@@ -274,6 +323,7 @@ static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
 				       const struct of_phandle_args *sp)
 {
 	u32 type, idx, chan;
+	struct imx_mu_priv *priv = to_imx_mu_priv(mbox);
 
 	if (sp->args_count != 2) {
 		dev_err(mbox->dev, "Invalid argument count %d\n", sp->args_count);
@@ -284,7 +334,9 @@ static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
 	idx = sp->args[1]; /* index */
 	chan = type * 4 + idx;
 
-	if (chan >= mbox->num_chans) {
+	if (chan >= mbox->num_chans ||
+	    (priv->dcfg->type == IMX_MU_TYPE_SCU &&
+	     type < IMX_MU_TYPE_TXDB && idx > 0)) {
 		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx);
 		return ERR_PTR(-EINVAL);
 	}
@@ -401,9 +453,20 @@ static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
 	.xCR	= 0x64,
 };
 
+static const struct imx_mu_dcfg imx_mu_cfg_imx8_scu = {
+	.type	= IMX_MU_TYPE_SCU,
+	.tx	= imx_mu_scu_tx,
+	.rx	= imx_mu_scu_rx,
+	.xTR	= {0x0, 0x4, 0x8, 0xc},
+	.xRR	= {0x10, 0x14, 0x18, 0x1c},
+	.xSR	= 0x20,
+	.xCR	= 0x24,
+};
+
 static const struct of_device_id imx_mu_dt_ids[] = {
 	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp },
 	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx },
+	{ .compatible = "fsl,imx8-mu-scu", .data = &imx_mu_cfg_imx8_scu },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
-- 
2.16.4

[PATCH V3 4/4] firmware: imx-scu: Support one TX and one RX

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>

Current imx-scu requires four TX and four RX to communicate with
SCU. This is low efficient and causes lots of mailbox interrupts.

With imx-mailbox driver could support one TX to use all four transmit
registers and one RX to use all four receive registers, imx-scu
could use one TX and one RX.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V3:
 Check mbox fsl,imx8-mu-scu for fast_ipc

 drivers/firmware/imx/imx-scu.c | 54 +++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index f71eaa5bf52d..e94a5585b698 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -38,6 +38,7 @@ struct imx_sc_ipc {
 	struct device *dev;
 	struct mutex lock;
 	struct completion done;
+	bool fast_ipc;
 
 	/* temporarily store the SCU msg */
 	u32 *msg;
@@ -115,6 +116,7 @@ static void imx_scu_rx_callback(struct mbox_client *c, void *msg)
 	struct imx_sc_ipc *sc_ipc = sc_chan->sc_ipc;
 	struct imx_sc_rpc_msg *hdr;
 	u32 *data = msg;
+	int i;
 
 	if (!sc_ipc->msg) {
 		dev_warn(sc_ipc->dev, "unexpected rx idx %d 0x%08x, ignore!\n",
@@ -122,6 +124,19 @@ static void imx_scu_rx_callback(struct mbox_client *c, void *msg)
 		return;
 	}
 
+	if (sc_ipc->fast_ipc) {
+		hdr = msg;
+		sc_ipc->rx_size = hdr->size;
+		sc_ipc->msg[0] = *data++;
+
+		for (i = 1; i < sc_ipc->rx_size; i++)
+			sc_ipc->msg[i] = *data++;
+
+		complete(&sc_ipc->done);
+
+		return;
+	}
+
 	if (sc_chan->idx == 0) {
 		hdr = msg;
 		sc_ipc->rx_size = hdr->size;
@@ -147,6 +162,7 @@ static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc, void *msg)
 	struct imx_sc_chan *sc_chan;
 	u32 *data = msg;
 	int ret;
+	int size;
 	int i;
 
 	/* Check size */
@@ -156,7 +172,8 @@ static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc, void *msg)
 	dev_dbg(sc_ipc->dev, "RPC SVC %u FUNC %u SIZE %u\n", hdr->svc,
 		hdr->func, hdr->size);
 
-	for (i = 0; i < hdr->size; i++) {
+	size = sc_ipc->fast_ipc ? 1 : hdr->size;
+	for (i = 0; i < size; i++) {
 		sc_chan = &sc_ipc->chans[i % 4];
 
 		/*
@@ -168,8 +185,10 @@ static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc, void *msg)
 		 * Wait for tx_done before every send to ensure that no
 		 * queueing happens at the mailbox channel level.
 		 */
-		wait_for_completion(&sc_chan->tx_done);
-		reinit_completion(&sc_chan->tx_done);
+		if (!sc_ipc->fast_ipc) {
+			wait_for_completion(&sc_chan->tx_done);
+			reinit_completion(&sc_chan->tx_done);
+		}
 
 		ret = mbox_send_message(sc_chan->ch, &data[i]);
 		if (ret < 0)
@@ -246,6 +265,8 @@ static int imx_scu_probe(struct platform_device *pdev)
 	struct imx_sc_chan *sc_chan;
 	struct mbox_client *cl;
 	char *chan_name;
+	struct of_phandle_args args;
+	int num_channel;
 	int ret;
 	int i;
 
@@ -253,11 +274,20 @@ static int imx_scu_probe(struct platform_device *pdev)
 	if (!sc_ipc)
 		return -ENOMEM;
 
-	for (i = 0; i < SCU_MU_CHAN_NUM; i++) {
-		if (i < 4)
+	ret = of_parse_phandle_with_args(pdev->dev.of_node, "mboxes",
+					 "#mbox-cells", 0, &args);
+	if (ret)
+		return ret;
+
+	sc_ipc->fast_ipc = of_device_is_compatible(args.np, "fsl,imx8-mu-scu");
+
+	num_channel = sc_ipc->fast_ipc ? 2 : SCU_MU_CHAN_NUM;
+	for (i = 0; i < num_channel; i++) {
+		if (i < num_channel / 2)
 			chan_name = kasprintf(GFP_KERNEL, "tx%d", i);
 		else
-			chan_name = kasprintf(GFP_KERNEL, "rx%d", i - 4);
+			chan_name = kasprintf(GFP_KERNEL, "rx%d",
+					      i - num_channel / 2);
 
 		if (!chan_name)
 			return -ENOMEM;
@@ -269,13 +299,15 @@ static int imx_scu_probe(struct platform_device *pdev)
 		cl->knows_txdone = true;
 		cl->rx_callback = imx_scu_rx_callback;
 
-		/* Initial tx_done completion as "done" */
-		cl->tx_done = imx_scu_tx_done;
-		init_completion(&sc_chan->tx_done);
-		complete(&sc_chan->tx_done);
+		if (!sc_ipc->fast_ipc) {
+			/* Initial tx_done completion as "done" */
+			cl->tx_done = imx_scu_tx_done;
+			init_completion(&sc_chan->tx_done);
+			complete(&sc_chan->tx_done);
+		}
 
 		sc_chan->sc_ipc = sc_ipc;
-		sc_chan->idx = i % 4;
+		sc_chan->idx = i % (num_channel / 2);
 		sc_chan->ch = mbox_request_channel_byname(cl, chan_name);
 		if (IS_ERR(sc_chan->ch)) {
 			ret = PTR_ERR(sc_chan->ch);
-- 
2.16.4

Re: [PATCH V3 2/4] mailbox: imx: restructure code to make easy for new MU

[Oleksij Rempel]

Hi Peng,

On 26.02.20 05:40, peng.fan@nxp.com wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add imx_mu_generic_tx for data send and imx_mu_generic_rx for interrupt
> data receive.
> Add 'type' for MU type.
> 
> With these, it will be a bit easy to introduce i.MX8/8X SCU type
> MU dedicated to communicate with SCU.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V3:
>   New patch, restructure code.
> 
>   drivers/mailbox/imx-mailbox.c | 100 ++++++++++++++++++++++++++++--------------
>   1 file changed, 67 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 2cdcdc5f1119..901a3431fdb5 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -36,7 +36,17 @@ enum imx_mu_chan_type {
>   	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
>   };
>   
> +enum imx_mu_type {
> +	IMX_MU_TYPE_GENERIC,
> +};

I assume this enum is not needed, see my next email
> +struct imx_mu_priv;
> +struct imx_mu_con_priv;
> +
>   struct imx_mu_dcfg {
> +	enum imx_mu_type type;
> +	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, void *data);
> +	int (*rx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp);
>   	u32	xTR[4];		/* Transmit Registers */
>   	u32	xRR[4];		/* Receive Registers */
>   	u32	xSR;		/* Status Register */
> @@ -67,20 +77,6 @@ struct imx_mu_priv {
>   	bool			side_b;
>   };
>   
> -static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> -	.xTR	= {0x0, 0x4, 0x8, 0xc},
> -	.xRR	= {0x10, 0x14, 0x18, 0x1c},
> -	.xSR	= 0x20,
> -	.xCR	= 0x24,
> -};
> -
> -static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> -	.xTR	= {0x20, 0x24, 0x28, 0x2c},
> -	.xRR	= {0x40, 0x44, 0x48, 0x4c},
> -	.xSR	= 0x60,
> -	.xCR	= 0x64,
> -};

Please, do not move it.

>   static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
>   {
>   	return container_of(mbox, struct imx_mu_priv, mbox);
> @@ -111,6 +107,40 @@ static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, u32 set, u32 clr)
>   	return val;
>   }
>   
> +static int imx_mu_generic_tx(struct imx_mu_priv *priv,
> +			     struct imx_mu_con_priv *cp,
> +			     void *data)
> +{
> +	u32 *arg = data;
> +
> +	switch (cp->type) {
> +	case IMX_MU_TYPE_TX:
> +		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
> +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
> +		break;
> +	case IMX_MU_TYPE_TXDB:
> +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
> +		tasklet_schedule(&cp->txdb_tasklet);
> +		break;
> +	default:
> +		dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int imx_mu_generic_rx(struct imx_mu_priv *priv,
> +			     struct imx_mu_con_priv *cp)
> +{
> +	u32 dat;
> +
> +	dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
> +	mbox_chan_received_data(cp->chan, (void *)&dat);
> +
> +	return 0;
> +}
> +
>   static void imx_mu_txdb_tasklet(unsigned long data)
>   {
>   	struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data;
> @@ -123,7 +153,7 @@ 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, ctrl, dat;
> +	u32 val, ctrl;
>   
>   	ctrl = imx_mu_read(priv, priv->dcfg->xCR);
>   	val = imx_mu_read(priv, priv->dcfg->xSR);
> @@ -152,8 +182,7 @@ static irqreturn_t imx_mu_isr(int irq, void *p)
>   		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
>   		mbox_chan_txdone(chan, 0);
>   	} else if (val == IMX_MU_xSR_RFn(cp->idx)) {
> -		dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
> -		mbox_chan_received_data(chan, (void *)&dat);
> +		priv->dcfg->rx(priv, cp);
>   	} else if (val == IMX_MU_xSR_GIPn(cp->idx)) {
>   		imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx), priv->dcfg->xSR);
>   		mbox_chan_received_data(chan, NULL);
> @@ -169,23 +198,8 @@ 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;
> -
> -	switch (cp->type) {
> -	case IMX_MU_TYPE_TX:
> -		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
> -		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
> -		break;
> -	case IMX_MU_TYPE_TXDB:
> -		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
> -		tasklet_schedule(&cp->txdb_tasklet);
> -		break;
> -	default:
> -		dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type);
> -		return -EINVAL;
> -	}
>   
> -	return 0;
> +	return priv->dcfg->tx(priv, cp, data);
>   }
>   
>   static int imx_mu_startup(struct mbox_chan *chan)
> @@ -367,6 +381,26 @@ static int imx_mu_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> +	.type	= IMX_MU_TYPE_GENERIC,
> +	.tx	= imx_mu_generic_tx,
> +	.rx	= imx_mu_generic_rx,
> +	.xTR	= {0x0, 0x4, 0x8, 0xc},
> +	.xRR	= {0x10, 0x14, 0x18, 0x1c},
> +	.xSR	= 0x20,
> +	.xCR	= 0x24,
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> +	.type	= IMX_MU_TYPE_GENERIC,
> +	.tx	= imx_mu_generic_tx,
> +	.rx	= imx_mu_generic_rx,
> +	.xTR	= {0x20, 0x24, 0x28, 0x2c},
> +	.xRR	= {0x40, 0x44, 0x48, 0x4c},
> +	.xSR	= 0x60,
> +	.xCR	= 0x64,
> +};
> +
>   static const struct of_device_id imx_mu_dt_ids[] = {
>   	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp },
>   	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx },
> 

Kind regards,
Oleksij Rempel

-- 
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 V3 3/4] mailbox: imx: add SCU MU support

[Oleksij Rempel]

Hi,

On 26.02.20 05:40, peng.fan@nxp.com wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX8/8X SCU MU is dedicated for communication between SCU and Cortex-A
> cores from hardware design, and could not be reused for other purpose.
> 
> Per i.MX8/8X Reference mannual, Chapter "12.9.2.3.2 Messaging Examples",
>   Passing short messages: Transmit register(s) can be used to pass
>   short messages from one to four words in length. For example, when
>   a four-word message is desired, only one of the registers needs to
>   have its corresponding interrupt enable bit set at the receiver side;
>   the message’s first three words are written to the registers whose
>   interrupt is masked, and the fourth word is written to the other
>   register (which triggers an interrupt at the receiver side).
> 
> i.MX8/8X SCU firmware IPC is an implementation of passing short
> messages. But current imx-mailbox driver only support one word
> message, i.MX8/8X linux side firmware has to request four TX
> and four RX to support IPC to SCU firmware. This is low efficent
> and more interrupts triggered compared with one TX and
> one RX.
> 
> To make SCU MU work,
>    - parse the size of msg.
>    - Only enable TR0/RR0 interrupt for transmit/receive message.
>    - For TX/RX, only support one TX channel and one RX channel
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V3:
>   Added scu type tx/rx and SCU MU type
> 
>   drivers/mailbox/imx-mailbox.c | 65 ++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 901a3431fdb5..41664a64c5fd 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -4,6 +4,7 @@
>    */
>   
>   #include <linux/clk.h>
> +#include <linux/firmware/imx/ipc.h>
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
>   #include <linux/kernel.h>
> @@ -38,11 +39,17 @@ enum imx_mu_chan_type {
>   
>   enum imx_mu_type {
>   	IMX_MU_TYPE_GENERIC,
> +	IMX_MU_TYPE_SCU,
>   };
>   
>   struct imx_mu_priv;
>   struct imx_mu_con_priv;
>   
> +struct imx_sc_rpc_msg_max {
> +	struct imx_sc_rpc_msg hdr;
> +	u32 data[7];
> +} __packed __aligned(4);;
> +
>   struct imx_mu_dcfg {
>   	enum imx_mu_type type;
>   	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, void *data);
> @@ -141,6 +148,48 @@ static int imx_mu_generic_rx(struct imx_mu_priv *priv,
>   	return 0;
>   }
>   
> +static int imx_mu_scu_tx(struct imx_mu_priv *priv,
> +			 struct imx_mu_con_priv *cp,
> +			 void *data)
> +{
> +	struct imx_sc_rpc_msg_max *msg = data;
> +	u32 *arg = data;
> +	int i;
> +
> +	switch (cp->type) {
> +	case IMX_MU_TYPE_TX:

please add sanity check if msg->hdr.size can be handled by this driver version.

> +		for (i = 0; i < msg->hdr.size; i++) {
> +			imx_mu_write(priv, *arg++,
> +				     priv->dcfg->xTR[i % 4]);
> +		}
> +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
> +		break;
> +	default:
> +		dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int imx_mu_scu_rx(struct imx_mu_priv *priv,
> +			 struct imx_mu_con_priv *cp)
> +{
> +	struct imx_sc_rpc_msg_max msg;
> +	u32 *data = (u32 *)&msg;
> +	int i;
> +
> +	imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0));
> +	*data++ = imx_mu_read(priv, priv->dcfg->xRR[0]);

please add sanity check. the message size should not be higher then sizeof(msg)

> +	for (i = 1; i < msg.hdr.size; i++)
> +		*data++ = imx_mu_read(priv, priv->dcfg->xRR[i % 4]);
> +
> +	imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(0), 0);

Please do not forget to handle properly new msg size in your rx_callback. In previous 
implementation the message size was 4byte.

> +	mbox_chan_received_data(cp->chan, (void *)&msg);
> +
> +	return 0;
> +}
> +
>   static void imx_mu_txdb_tasklet(unsigned long data)
>   {
>   	struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data;
> @@ -274,6 +323,7 @@ static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
>   				       const struct of_phandle_args *sp)
>   {
>   	u32 type, idx, chan;
> +	struct imx_mu_priv *priv = to_imx_mu_priv(mbox);
>   
>   	if (sp->args_count != 2) {
>   		dev_err(mbox->dev, "Invalid argument count %d\n", sp->args_count);
> @@ -284,7 +334,9 @@ static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
>   	idx = sp->args[1]; /* index */
>   	chan = type * 4 + idx;
>   
> -	if (chan >= mbox->num_chans) {
> +	if (chan >= mbox->num_chans ||
> +	    (priv->dcfg->type == IMX_MU_TYPE_SCU &&
> +	     type < IMX_MU_TYPE_TXDB && idx > 0)) {

We need this check since mbox->num_chans do not reflects new reality. Now we have only 2 
channels. One RX and one TX. No idea if we need doorbell channels for SCU. If doorbells 
are not supported, it is better to add a separate imx_mu_xlate for SCU

and add SCU specific channel init in probe in addition to:
         for (i = 0; i < IMX_MU_CHANS; i++) { 

                 struct imx_mu_con_priv *cp = &priv->con_priv[i]; 

 

                 cp->idx = i % 4; 

                 cp->type = i >> 2; 

                 cp->chan = &priv->mbox_chans[i]; 

                 priv->mbox_chans[i].con_priv = cp; 

                 snprintf(cp->irq_desc, sizeof(cp->irq_desc), 

                          "imx_mu_chan[%i-%i]", cp->type, cp->idx); 

         }

There is no need to init unsupported channels. Please pack it in separate function

>   		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx);
>   		return ERR_PTR(-EINVAL);
>   	}
> @@ -401,9 +453,20 @@ static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
>   	.xCR	= 0x64,
>   };
>   
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8_scu = {
> +	.type	= IMX_MU_TYPE_SCU,
> +	.tx	= imx_mu_scu_tx,
> +	.rx	= imx_mu_scu_rx,
> +	.xTR	= {0x0, 0x4, 0x8, 0xc},
> +	.xRR	= {0x10, 0x14, 0x18, 0x1c},
> +	.xSR	= 0x20,
> +	.xCR	= 0x24,
> +};
> +
>   static const struct of_device_id imx_mu_dt_ids[] = {
>   	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp },
>   	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx },
> +	{ .compatible = "fsl,imx8-mu-scu", .data = &imx_mu_cfg_imx8_scu },
>   	{ },
>   };
>   MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> 

Kind regards,
Oleksij Rempel

-- 
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 V3 2/4] mailbox: imx: restructure code to make easy for new MU

[Peng Fan]

Hi Oleksij,

> Subject: Re: [PATCH V3 2/4] mailbox: imx: restructure code to make easy for
> new MU
> 
> Hi Peng,
> 
> On 26.02.20 05:40, peng.fan@nxp.com wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add imx_mu_generic_tx for data send and imx_mu_generic_rx for
> > interrupt data receive.
> > Add 'type' for MU type.
> >
> > With these, it will be a bit easy to introduce i.MX8/8X SCU type MU
> > dedicated to communicate with SCU.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V3:
> >   New patch, restructure code.
> >
> >   drivers/mailbox/imx-mailbox.c | 100
> ++++++++++++++++++++++++++++--------------
> >   1 file changed, 67 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/mailbox/imx-mailbox.c
> > b/drivers/mailbox/imx-mailbox.c index 2cdcdc5f1119..901a3431fdb5
> > 100644
> > --- a/drivers/mailbox/imx-mailbox.c
> > +++ b/drivers/mailbox/imx-mailbox.c
> > @@ -36,7 +36,17 @@ enum imx_mu_chan_type {
> >   	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
> >   };
> >
> > +enum imx_mu_type {
> > +	IMX_MU_TYPE_GENERIC,
> > +};
> 
> I assume this enum is not needed, see my next email

This is to just differentiate with SCU MU, ok, will check your next email.
Then back if anything I not follow up.

> > +struct imx_mu_priv;
> > +struct imx_mu_con_priv;
> > +
> >   struct imx_mu_dcfg {
> > +	enum imx_mu_type type;
> > +	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, void
> *data);
> > +	int (*rx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp);
> >   	u32	xTR[4];		/* Transmit Registers */
> >   	u32	xRR[4];		/* Receive Registers */
> >   	u32	xSR;		/* Status Register */
> > @@ -67,20 +77,6 @@ struct imx_mu_priv {
> >   	bool			side_b;
> >   };
> >
> > -static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > -	.xTR	= {0x0, 0x4, 0x8, 0xc},
> > -	.xRR	= {0x10, 0x14, 0x18, 0x1c},
> > -	.xSR	= 0x20,
> > -	.xCR	= 0x24,
> > -};
> > -
> > -static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > -	.xTR	= {0x20, 0x24, 0x28, 0x2c},
> > -	.xRR	= {0x40, 0x44, 0x48, 0x4c},
> > -	.xSR	= 0x60,
> > -	.xCR	= 0x64,
> > -};
> 
> Please, do not move it.

Ok. Then I need to add imx_mu_generic_tx prototype above the
upper two.

Thanks,
Peng.

> 
> >   static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller
> *mbox)
> >   {
> >   	return container_of(mbox, struct imx_mu_priv, mbox); @@ -111,6
> > +107,40 @@ static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, u32 set,
> u32 clr)
> >   	return val;
> >   }
> >
> > +static int imx_mu_generic_tx(struct imx_mu_priv *priv,
> > +			     struct imx_mu_con_priv *cp,
> > +			     void *data)
> > +{
> > +	u32 *arg = data;
> > +
> > +	switch (cp->type) {
> > +	case IMX_MU_TYPE_TX:
> > +		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
> > +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
> > +		break;
> > +	case IMX_MU_TYPE_TXDB:
> > +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
> > +		tasklet_schedule(&cp->txdb_tasklet);
> > +		break;
> > +	default:
> > +		dev_warn_ratelimited(priv->dev, "Send data on wrong channel
> type: %d\n", cp->type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_mu_generic_rx(struct imx_mu_priv *priv,
> > +			     struct imx_mu_con_priv *cp)
> > +{
> > +	u32 dat;
> > +
> > +	dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
> > +	mbox_chan_received_data(cp->chan, (void *)&dat);
> > +
> > +	return 0;
> > +}
> > +
> >   static void imx_mu_txdb_tasklet(unsigned long data)
> >   {
> >   	struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data; @@
> > -123,7 +153,7 @@ 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, ctrl, dat;
> > +	u32 val, ctrl;
> >
> >   	ctrl = imx_mu_read(priv, priv->dcfg->xCR);
> >   	val = imx_mu_read(priv, priv->dcfg->xSR); @@ -152,8 +182,7 @@
> > static irqreturn_t imx_mu_isr(int irq, void *p)
> >   		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
> >   		mbox_chan_txdone(chan, 0);
> >   	} else if (val == IMX_MU_xSR_RFn(cp->idx)) {
> > -		dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
> > -		mbox_chan_received_data(chan, (void *)&dat);
> > +		priv->dcfg->rx(priv, cp);
> >   	} else if (val == IMX_MU_xSR_GIPn(cp->idx)) {
> >   		imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx),
> priv->dcfg->xSR);
> >   		mbox_chan_received_data(chan, NULL); @@ -169,23 +198,8
> @@ 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;
> > -
> > -	switch (cp->type) {
> > -	case IMX_MU_TYPE_TX:
> > -		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
> > -		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
> > -		break;
> > -	case IMX_MU_TYPE_TXDB:
> > -		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
> > -		tasklet_schedule(&cp->txdb_tasklet);
> > -		break;
> > -	default:
> > -		dev_warn_ratelimited(priv->dev, "Send data on wrong channel
> type: %d\n", cp->type);
> > -		return -EINVAL;
> > -	}
> >
> > -	return 0;
> > +	return priv->dcfg->tx(priv, cp, data);
> >   }
> >
> >   static int imx_mu_startup(struct mbox_chan *chan) @@ -367,6 +381,26
> > @@ static int imx_mu_remove(struct platform_device *pdev)
> >   	return 0;
> >   }
> >
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > +	.type	= IMX_MU_TYPE_GENERIC,
> > +	.tx	= imx_mu_generic_tx,
> > +	.rx	= imx_mu_generic_rx,
> > +	.xTR	= {0x0, 0x4, 0x8, 0xc},
> > +	.xRR	= {0x10, 0x14, 0x18, 0x1c},
> > +	.xSR	= 0x20,
> > +	.xCR	= 0x24,
> > +};
> > +
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> > +	.type	= IMX_MU_TYPE_GENERIC,
> > +	.tx	= imx_mu_generic_tx,
> > +	.rx	= imx_mu_generic_rx,
> > +	.xTR	= {0x20, 0x24, 0x28, 0x2c},
> > +	.xRR	= {0x40, 0x44, 0x48, 0x4c},
> > +	.xSR	= 0x60,
> > +	.xCR	= 0x64,
> > +};
> > +
> >   static const struct of_device_id imx_mu_dt_ids[] = {
> >   	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp },
> >   	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx },
> >
> 
> Kind regards,
> Oleksij Rempel
> 
> --
> Pengutronix e.K.                           |
> |
> Industrial Linux Solutions                 |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7Ccf660c
> 3909e14128e65f08d7be898905%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C637187370224967554&amp;sdata=ZdgIGuzpnhX1liQO4KmCtXx
> jjg%2BnDJoPze4pfVXIAig%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 V3 3/4] mailbox: imx: add SCU MU support

[Peng Fan]

Hi Oleksij,

> Subject: Re: [PATCH V3 3/4] mailbox: imx: add SCU MU support
> 
> Hi,
> 
> On 26.02.20 05:40, peng.fan@nxp.com wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > i.MX8/8X SCU MU is dedicated for communication between SCU and
> > Cortex-A cores from hardware design, and could not be reused for other
> purpose.
> >
> > Per i.MX8/8X Reference mannual, Chapter "12.9.2.3.2 Messaging
> Examples",
> >   Passing short messages: Transmit register(s) can be used to pass
> >   short messages from one to four words in length. For example, when
> >   a four-word message is desired, only one of the registers needs to
> >   have its corresponding interrupt enable bit set at the receiver side;
> >   the message’s first three words are written to the registers whose
> >   interrupt is masked, and the fourth word is written to the other
> >   register (which triggers an interrupt at the receiver side).
> >
> > i.MX8/8X SCU firmware IPC is an implementation of passing short
> > messages. But current imx-mailbox driver only support one word
> > message, i.MX8/8X linux side firmware has to request four TX and four
> > RX to support IPC to SCU firmware. This is low efficent and more
> > interrupts triggered compared with one TX and one RX.
> >
> > To make SCU MU work,
> >    - parse the size of msg.
> >    - Only enable TR0/RR0 interrupt for transmit/receive message.
> >    - For TX/RX, only support one TX channel and one RX channel
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V3:
> >   Added scu type tx/rx and SCU MU type
> >
> >   drivers/mailbox/imx-mailbox.c | 65
> ++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mailbox/imx-mailbox.c
> > b/drivers/mailbox/imx-mailbox.c index 901a3431fdb5..41664a64c5fd
> > 100644
> > --- a/drivers/mailbox/imx-mailbox.c
> > +++ b/drivers/mailbox/imx-mailbox.c
> > @@ -4,6 +4,7 @@
> >    */
> >
> >   #include <linux/clk.h>
> > +#include <linux/firmware/imx/ipc.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/io.h>
> >   #include <linux/kernel.h>
> > @@ -38,11 +39,17 @@ enum imx_mu_chan_type {
> >
> >   enum imx_mu_type {
> >   	IMX_MU_TYPE_GENERIC,
> > +	IMX_MU_TYPE_SCU,
> >   };
> >
> >   struct imx_mu_priv;
> >   struct imx_mu_con_priv;
> >
> > +struct imx_sc_rpc_msg_max {
> > +	struct imx_sc_rpc_msg hdr;
> > +	u32 data[7];
> > +} __packed __aligned(4);;
> > +
> >   struct imx_mu_dcfg {
> >   	enum imx_mu_type type;
> >   	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp,
> > void *data); @@ -141,6 +148,48 @@ static int imx_mu_generic_rx(struct
> imx_mu_priv *priv,
> >   	return 0;
> >   }
> >
> > +static int imx_mu_scu_tx(struct imx_mu_priv *priv,
> > +			 struct imx_mu_con_priv *cp,
> > +			 void *data)
> > +{
> > +	struct imx_sc_rpc_msg_max *msg = data;
> > +	u32 *arg = data;
> > +	int i;
> > +
> > +	switch (cp->type) {
> > +	case IMX_MU_TYPE_TX:
> 
> please add sanity check if msg->hdr.size can be handled by this driver version.

Yes, I forgot to add that. Should as below:

if (msg->hdr.size > sizeof(struct imx_sc_rpc_msg_max))
	return -EINVAL;

> 
> > +		for (i = 0; i < msg->hdr.size; i++) {
> > +			imx_mu_write(priv, *arg++,
> > +				     priv->dcfg->xTR[i % 4]);
> > +		}
> > +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
> > +		break;
> > +	default:
> > +		dev_warn_ratelimited(priv->dev, "Send data on wrong channel
> type: %d\n", cp->type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_mu_scu_rx(struct imx_mu_priv *priv,
> > +			 struct imx_mu_con_priv *cp)
> > +{
> > +	struct imx_sc_rpc_msg_max msg;
> > +	u32 *data = (u32 *)&msg;
> > +	int i;
> > +
> > +	imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0));
> > +	*data++ = imx_mu_read(priv, priv->dcfg->xRR[0]);
> 
> please add sanity check. the message size should not be higher then
> sizeof(msg)

Yes.

> 
> > +	for (i = 1; i < msg.hdr.size; i++)
> > +		*data++ = imx_mu_read(priv, priv->dcfg->xRR[i % 4]);
> > +
> > +	imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(0), 0);
> 
> Please do not forget to handle properly new msg size in your rx_callback. In
> previous implementation the message size was 4byte.

My patch could handle size larger than 4 words. The previous implementation
to limit 4 words is not good.

> 
> > +	mbox_chan_received_data(cp->chan, (void *)&msg);
> > +
> > +	return 0;
> > +}
> > +
> >   static void imx_mu_txdb_tasklet(unsigned long data)
> >   {
> >   	struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data; @@
> > -274,6 +323,7 @@ static struct mbox_chan * imx_mu_xlate(struct
> mbox_controller *mbox,
> >   				       const struct of_phandle_args *sp)
> >   {
> >   	u32 type, idx, chan;
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(mbox);
> >
> >   	if (sp->args_count != 2) {
> >   		dev_err(mbox->dev, "Invalid argument count %d\n",
> sp->args_count);
> > @@ -284,7 +334,9 @@ static struct mbox_chan * imx_mu_xlate(struct
> mbox_controller *mbox,
> >   	idx = sp->args[1]; /* index */
> >   	chan = type * 4 + idx;
> >
> > -	if (chan >= mbox->num_chans) {
> > +	if (chan >= mbox->num_chans ||
> > +	    (priv->dcfg->type == IMX_MU_TYPE_SCU &&
> > +	     type < IMX_MU_TYPE_TXDB && idx > 0)) {
> 
> We need this check since mbox->num_chans do not reflects new reality. Now
> we have only 2 channels. One RX and one TX. No idea if we need doorbell
> channels for SCU. If doorbells are not supported, it is better to add a separate
> imx_mu_xlate for SCU

TXDB currently not supported, but I am not sure whether it will be supported
in future. I could add a separate xlate as you suggested, since TXDB not support
now.

> 
> and add SCU specific channel init in probe in addition to:
>          for (i = 0; i < IMX_MU_CHANS; i++) {
> 
>                  struct imx_mu_con_priv *cp = &priv->con_priv[i];
> 
> 
> 
>                  cp->idx = i % 4;
> 
>                  cp->type = i >> 2;
> 
>                  cp->chan = &priv->mbox_chans[i];
> 
>                  priv->mbox_chans[i].con_priv = cp;
> 
>                  snprintf(cp->irq_desc, sizeof(cp->irq_desc),
> 
>                           "imx_mu_chan[%i-%i]", cp->type, cp->idx);
> 
>          }
> 
> There is no need to init unsupported channels. Please pack it in separate
> function

ok. I just want to reuse current code as more as possible and avoid
adding too much new code.

Thanks,
Peng.

> 
> >   		dev_err(mbox->dev, "Not supported channel number: %d.
> (type: %d, idx: %d)\n", chan, type, idx);
> >   		return ERR_PTR(-EINVAL);
> >   	}
> > @@ -401,9 +453,20 @@ static const struct imx_mu_dcfg
> imx_mu_cfg_imx7ulp = {
> >   	.xCR	= 0x64,
> >   };
> >
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8_scu = {
> > +	.type	= IMX_MU_TYPE_SCU,
> > +	.tx	= imx_mu_scu_tx,
> > +	.rx	= imx_mu_scu_rx,
> > +	.xTR	= {0x0, 0x4, 0x8, 0xc},
> > +	.xRR	= {0x10, 0x14, 0x18, 0x1c},
> > +	.xSR	= 0x20,
> > +	.xCR	= 0x24,
> > +};
> > +
> >   static const struct of_device_id imx_mu_dt_ids[] = {
> >   	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp },
> >   	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx },
> > +	{ .compatible = "fsl,imx8-mu-scu", .data = &imx_mu_cfg_imx8_scu },
> >   	{ },
> >   };
> >   MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> >
> 
> Kind regards,
> Oleksij Rempel
> 
> --
> Pengutronix e.K.                           |
> |
> Industrial Linux Solutions                 |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7C47558a
> c6c6bd4ba38c2708d7be8eb946%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C637187392512520476&amp;sdata=N0WbBREAFegjX0p%2FSv3
> cuLpb8FvnCePU%2Bz6VkhC%2FU4c%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 V3 2/4] mailbox: imx: restructure code to make easy for new MU

[Oleksij Rempel]

On 02.03.20 10:58, Peng Fan wrote:
> Hi Oleksij,
> 
>> Subject: Re: [PATCH V3 2/4] mailbox: imx: restructure code to make easy for
>> new MU
>>
>> Hi Peng,
>>
>> On 26.02.20 05:40, peng.fan@nxp.com wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> Add imx_mu_generic_tx for data send and imx_mu_generic_rx for
>>> interrupt data receive.
>>> Add 'type' for MU type.
>>>
>>> With these, it will be a bit easy to introduce i.MX8/8X SCU type MU
>>> dedicated to communicate with SCU.
>>>
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>
>>> V3:
>>>    New patch, restructure code.
>>>
>>>    drivers/mailbox/imx-mailbox.c | 100
>> ++++++++++++++++++++++++++++--------------
>>>    1 file changed, 67 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/imx-mailbox.c
>>> b/drivers/mailbox/imx-mailbox.c index 2cdcdc5f1119..901a3431fdb5
>>> 100644
>>> --- a/drivers/mailbox/imx-mailbox.c
>>> +++ b/drivers/mailbox/imx-mailbox.c
>>> @@ -36,7 +36,17 @@ enum imx_mu_chan_type {
>>>    	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
>>>    };
>>>
>>> +enum imx_mu_type {
>>> +	IMX_MU_TYPE_GENERIC,
>>> +};
>>
>> I assume this enum is not needed, see my next email
> 
> This is to just differentiate with SCU MU, ok, will check your next email.
> Then back if anything I not follow up.
> 
>>> +struct imx_mu_priv;
>>> +struct imx_mu_con_priv;
>>> +
>>>    struct imx_mu_dcfg {
>>> +	enum imx_mu_type type;
>>> +	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, void
>> *data);
>>> +	int (*rx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp);
>>>    	u32	xTR[4];		/* Transmit Registers */
>>>    	u32	xRR[4];		/* Receive Registers */
>>>    	u32	xSR;		/* Status Register */
>>> @@ -67,20 +77,6 @@ struct imx_mu_priv {
>>>    	bool			side_b;
>>>    };
>>>
>>> -static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
>>> -	.xTR	= {0x0, 0x4, 0x8, 0xc},
>>> -	.xRR	= {0x10, 0x14, 0x18, 0x1c},
>>> -	.xSR	= 0x20,
>>> -	.xCR	= 0x24,
>>> -};
>>> -
>>> -static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
>>> -	.xTR	= {0x20, 0x24, 0x28, 0x2c},
>>> -	.xRR	= {0x40, 0x44, 0x48, 0x4c},
>>> -	.xSR	= 0x60,
>>> -	.xCR	= 0x64,
>>> -};
>>
>> Please, do not move it.
> 
> Ok. Then I need to add imx_mu_generic_tx prototype above the
> upper two.

Ah... this was the reason. Then please ignore my comment :)

> Thanks,
> Peng.
> 
>>
>>>    static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller
>> *mbox)
>>>    {
>>>    	return container_of(mbox, struct imx_mu_priv, mbox); @@ -111,6
>>> +107,40 @@ static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, u32 set,
>> u32 clr)
>>>    	return val;
>>>    }
>>>
>>> +static int imx_mu_generic_tx(struct imx_mu_priv *priv,
>>> +			     struct imx_mu_con_priv *cp,
>>> +			     void *data)
>>> +{
>>> +	u32 *arg = data;
>>> +
>>> +	switch (cp->type) {
>>> +	case IMX_MU_TYPE_TX:
>>> +		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
>>> +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
>>> +		break;
>>> +	case IMX_MU_TYPE_TXDB:
>>> +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
>>> +		tasklet_schedule(&cp->txdb_tasklet);
>>> +		break;
>>> +	default:
>>> +		dev_warn_ratelimited(priv->dev, "Send data on wrong channel
>> type: %d\n", cp->type);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int imx_mu_generic_rx(struct imx_mu_priv *priv,
>>> +			     struct imx_mu_con_priv *cp)
>>> +{
>>> +	u32 dat;
>>> +
>>> +	dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
>>> +	mbox_chan_received_data(cp->chan, (void *)&dat);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    static void imx_mu_txdb_tasklet(unsigned long data)
>>>    {
>>>    	struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data; @@
>>> -123,7 +153,7 @@ 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, ctrl, dat;
>>> +	u32 val, ctrl;
>>>
>>>    	ctrl = imx_mu_read(priv, priv->dcfg->xCR);
>>>    	val = imx_mu_read(priv, priv->dcfg->xSR); @@ -152,8 +182,7 @@
>>> static irqreturn_t imx_mu_isr(int irq, void *p)
>>>    		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
>>>    		mbox_chan_txdone(chan, 0);
>>>    	} else if (val == IMX_MU_xSR_RFn(cp->idx)) {
>>> -		dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
>>> -		mbox_chan_received_data(chan, (void *)&dat);
>>> +		priv->dcfg->rx(priv, cp);
>>>    	} else if (val == IMX_MU_xSR_GIPn(cp->idx)) {
>>>    		imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx),
>> priv->dcfg->xSR);
>>>    		mbox_chan_received_data(chan, NULL); @@ -169,23 +198,8
>> @@ 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;
>>> -
>>> -	switch (cp->type) {
>>> -	case IMX_MU_TYPE_TX:
>>> -		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
>>> -		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
>>> -		break;
>>> -	case IMX_MU_TYPE_TXDB:
>>> -		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
>>> -		tasklet_schedule(&cp->txdb_tasklet);
>>> -		break;
>>> -	default:
>>> -		dev_warn_ratelimited(priv->dev, "Send data on wrong channel
>> type: %d\n", cp->type);
>>> -		return -EINVAL;
>>> -	}
>>>
>>> -	return 0;
>>> +	return priv->dcfg->tx(priv, cp, data);
>>>    }
>>>
>>>    static int imx_mu_startup(struct mbox_chan *chan) @@ -367,6 +381,26
>>> @@ static int imx_mu_remove(struct platform_device *pdev)
>>>    	return 0;
>>>    }
>>>
>>> +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
>>> +	.type	= IMX_MU_TYPE_GENERIC,
>>> +	.tx	= imx_mu_generic_tx,
>>> +	.rx	= imx_mu_generic_rx,
>>> +	.xTR	= {0x0, 0x4, 0x8, 0xc},
>>> +	.xRR	= {0x10, 0x14, 0x18, 0x1c},
>>> +	.xSR	= 0x20,
>>> +	.xCR	= 0x24,
>>> +};
>>> +
>>> +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
>>> +	.type	= IMX_MU_TYPE_GENERIC,
>>> +	.tx	= imx_mu_generic_tx,
>>> +	.rx	= imx_mu_generic_rx,
>>> +	.xTR	= {0x20, 0x24, 0x28, 0x2c},
>>> +	.xRR	= {0x40, 0x44, 0x48, 0x4c},
>>> +	.xSR	= 0x60,
>>> +	.xCR	= 0x64,
>>> +};
>>> +
>>>    static const struct of_device_id imx_mu_dt_ids[] = {
>>>    	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp },
>>>    	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx },
>>>
>>
>> Kind regards,
>> Oleksij Rempel
>>
>> --
>> Pengutronix e.K.                           |
>> |
>> Industrial Linux Solutions                 |
>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
>> engutronix.de%2F&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7Ccf660c
>> 3909e14128e65f08d7be898905%7C686ea1d3bc2b4c6fa92cd99c5c301635%
>> 7C0%7C0%7C637187370224967554&amp;sdata=ZdgIGuzpnhX1liQO4KmCtXx
>> jjg%2BnDJoPze4pfVXIAig%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 |

Kind regards,
Oleksij Rempel

-- 
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 |