linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type
@ 2020-03-04  5:49 peng.fan
  2020-03-04  5:49 ` [PATCH V6 1/4] dt-bindings: mailbox: imx-mu: add SCU MU support peng.fan
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: peng.fan @ 2020-03-04  5:49 UTC (permalink / raw)
  To: shawnguo, s.hauer, jassisinghbrar, o.rempel
  Cc: kernel, festevam, linux-imx, Anson.Huang, leonard.crestez,
	aisheng.dong, linux-arm-kernel, linux-kernel, Peng Fan

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

V6:
 Add Oleksij's R-b tag
 Patch 3/4, per https://www.kernel.org/doc/Documentation/printk-formats.txt
 should use %zu for printk sizeof

V5:
 Move imx_mu_dcfg below imx_mu_priv
 Add init hooks to imx_mu_dcfg
 drop __packed __aligned
 Add more debug msg
 code style cleanup

V4:
 Drop IMX_MU_TYPE_[GENERIC, SCU]
 Pack MU chans init to separate function
 Add separate function for SCU chans init and xlate
 Add santity check to msg hdr.size
 Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3]

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.

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                      | 267 +++++++++++++++++----
 3 files changed, 260 insertions(+), 63 deletions(-)


base-commit: 770fbb32d34e5d6298cc2be590c9d2fd6069aa17
-- 
2.16.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH V6 1/4] dt-bindings: mailbox: imx-mu: add SCU MU support
  2020-03-04  5:49 [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type peng.fan
@ 2020-03-04  5:49 ` peng.fan
  2020-03-04  5:49 ` [PATCH V6 2/4] mailbox: imx: restructure code to make easy for new MU peng.fan
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: peng.fan @ 2020-03-04  5:49 UTC (permalink / raw)
  To: shawnguo, s.hauer, jassisinghbrar, o.rempel
  Cc: kernel, festevam, linux-imx, Anson.Huang, leonard.crestez,
	aisheng.dong, linux-arm-kernel, linux-kernel, 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.

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
V6:
 Add R-b tag
V5:
 None
V4:
 None
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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH V6 2/4] mailbox: imx: restructure code to make easy for new MU
  2020-03-04  5:49 [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type peng.fan
  2020-03-04  5:49 ` [PATCH V6 1/4] dt-bindings: mailbox: imx-mu: add SCU MU support peng.fan
@ 2020-03-04  5:49 ` peng.fan
  2020-03-04  5:49 ` [PATCH V6 3/4] mailbox: imx: add SCU MU support peng.fan
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: peng.fan @ 2020-03-04  5:49 UTC (permalink / raw)
  To: shawnguo, s.hauer, jassisinghbrar, o.rempel
  Cc: kernel, festevam, linux-imx, Anson.Huang, leonard.crestez,
	aisheng.dong, linux-arm-kernel, linux-kernel, 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.

Pack original mu chans related code into imx_mu_init_generic

Add tx/rx/init hooks into imx_mu_dcfg

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

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V6:
 Add R-b tag
V5:
 imx_mu_dcfg moved to below imx_mu_priv
 Add init hooks
V4:
 Pack MU chans init to imx_mu_init_generic
V3:
 New patch, restructure code.

 drivers/mailbox/imx-mailbox.c | 137 +++++++++++++++++++++++++-----------------
 1 file changed, 83 insertions(+), 54 deletions(-)

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 2cdcdc5f1119..df6c4ecd913c 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -36,13 +36,6 @@ enum imx_mu_chan_type {
 	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
 };
 
-struct imx_mu_dcfg {
-	u32	xTR[4];		/* Transmit Registers */
-	u32	xRR[4];		/* Receive Registers */
-	u32	xSR;		/* Status Register */
-	u32	xCR;		/* Control Register */
-};
-
 struct imx_mu_con_priv {
 	unsigned int		idx;
 	char			irq_desc[IMX_MU_CHAN_NAME_SIZE];
@@ -67,18 +60,14 @@ 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,
+struct imx_mu_dcfg {
+	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);
+	void (*init)(struct imx_mu_priv *priv);
+	u32	xTR[4];		/* Transmit Registers */
+	u32	xRR[4];		/* Receive Registers */
+	u32	xSR;		/* Status Register */
+	u32	xCR;		/* Control Register */
 };
 
 static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
@@ -111,6 +100,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 +146,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 +175,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 +191,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)
@@ -280,6 +287,22 @@ static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
 
 static void imx_mu_init_generic(struct imx_mu_priv *priv)
 {
+	unsigned int i;
+
+	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);
+	}
+
+	priv->mbox.num_chans = IMX_MU_CHANS;
+	priv->mbox.of_xlate = imx_mu_xlate;
+
 	if (priv->side_b)
 		return;
 
@@ -293,7 +316,6 @@ static int imx_mu_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	struct imx_mu_priv *priv;
 	const struct imx_mu_dcfg *dcfg;
-	unsigned int i;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -329,32 +351,19 @@ static int imx_mu_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	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);
-	}
-
 	priv->side_b = of_property_read_bool(np, "fsl,mu-side-b");
 
+	priv->dcfg->init(priv);
+
 	spin_lock_init(&priv->xcr_lock);
 
 	priv->mbox.dev = dev;
 	priv->mbox.ops = &imx_mu_ops;
 	priv->mbox.chans = priv->mbox_chans;
-	priv->mbox.num_chans = IMX_MU_CHANS;
-	priv->mbox.of_xlate = imx_mu_xlate;
 	priv->mbox.txdone_irq = true;
 
 	platform_set_drvdata(pdev, priv);
 
-	imx_mu_init_generic(priv);
-
 	return devm_mbox_controller_register(dev, &priv->mbox);
 }
 
@@ -367,6 +376,26 @@ static int imx_mu_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
+	.tx	= imx_mu_generic_tx,
+	.rx	= imx_mu_generic_rx,
+	.init	= imx_mu_init_generic,
+	.xTR	= {0x0, 0x4, 0x8, 0xc},
+	.xRR	= {0x10, 0x14, 0x18, 0x1c},
+	.xSR	= 0x20,
+	.xCR	= 0x24,
+};
+
+static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
+	.tx	= imx_mu_generic_tx,
+	.rx	= imx_mu_generic_rx,
+	.init	= imx_mu_init_generic,
+	.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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH V6 3/4] mailbox: imx: add SCU MU support
  2020-03-04  5:49 [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type peng.fan
  2020-03-04  5:49 ` [PATCH V6 1/4] dt-bindings: mailbox: imx-mu: add SCU MU support peng.fan
  2020-03-04  5:49 ` [PATCH V6 2/4] mailbox: imx: restructure code to make easy for new MU peng.fan
@ 2020-03-04  5:49 ` peng.fan
  2020-03-18  2:51   ` Leonard Crestez
  2020-03-04  5:49 ` [PATCH V6 4/4] firmware: imx-scu: Support one TX and one RX peng.fan
  2020-03-12 23:09 ` [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type Leonard Crestez
  4 siblings, 1 reply; 12+ messages in thread
From: peng.fan @ 2020-03-04  5:49 UTC (permalink / raw)
  To: shawnguo, s.hauer, jassisinghbrar, o.rempel
  Cc: kernel, festevam, linux-imx, Anson.Huang, leonard.crestez,
	aisheng.dong, linux-arm-kernel, linux-kernel, 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
  - For RX, support receive msg larger than 4 u32 words.
  - Support 6 channels, TX0/RX0/RXDB[0-3], not support TXDB.

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V6:
 Add R-b tag
 Use %zu for printk sizeof

V5:
 Code style cleanup
 Add more debug msg
 Drop __packed aligned
 idx santity check in scu xlate

V4:
 Added separate chans init and xlate function for SCU MU
 Limit chans to TX0/RX0/RXDB[0-3], max 6 chans.
 Santity check to msg size

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

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

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index df6c4ecd913c..0895ccd23d17 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>
@@ -27,6 +28,8 @@
 #define IMX_MU_xCR_GIRn(x)	BIT(16 + (3 - (x)))
 
 #define IMX_MU_CHANS		16
+/* TX0/RX0/RXDB[0-3] */
+#define IMX_MU_SCU_CHANS	6
 #define IMX_MU_CHAN_NAME_SIZE	20
 
 enum imx_mu_chan_type {
@@ -36,6 +39,11 @@ enum imx_mu_chan_type {
 	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
 };
 
+struct imx_sc_rpc_msg_max {
+	struct imx_sc_rpc_msg hdr;
+	u32 data[7];
+};
+
 struct imx_mu_con_priv {
 	unsigned int		idx;
 	char			irq_desc[IMX_MU_CHAN_NAME_SIZE];
@@ -134,6 +142,63 @@ 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:
+		if (msg->hdr.size > sizeof(*msg)) {
+			/*
+			 * The real message size can be different to
+			 * struct imx_sc_rpc_msg_max size
+			 */
+			dev_err(priv->dev, "Exceed max msg size (%zu) on TX, got: %i\n", sizeof(*msg), msg->hdr.size);
+			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]);
+
+	if (msg.hdr.size > sizeof(msg)) {
+		dev_err(priv->dev, "Exceed max msg size (%zu) on RX, got: %i\n",
+			sizeof(msg), msg.hdr.size);
+		return -EINVAL;
+	}
+
+	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;
@@ -263,6 +328,42 @@ static const struct mbox_chan_ops imx_mu_ops = {
 	.shutdown = imx_mu_shutdown,
 };
 
+static struct mbox_chan *imx_mu_scu_xlate(struct mbox_controller *mbox,
+					  const struct of_phandle_args *sp)
+{
+	u32 type, idx, chan;
+
+	if (sp->args_count != 2) {
+		dev_err(mbox->dev, "Invalid argument count %d\n", sp->args_count);
+		return ERR_PTR(-EINVAL);
+	}
+
+	type = sp->args[0]; /* channel type */
+	idx = sp->args[1]; /* index */
+
+	switch (type) {
+	case IMX_MU_TYPE_TX:
+	case IMX_MU_TYPE_RX:
+		if (idx != 0)
+			dev_err(mbox->dev, "Invalid chan idx: %d\n", idx);
+		chan = type;
+		break;
+	case IMX_MU_TYPE_RXDB:
+		chan = 2 + idx;
+		break;
+	default:
+		dev_err(mbox->dev, "Invalid chan type: %d\n", type);
+		return NULL;
+	}
+
+	if (chan >= mbox->num_chans) {
+		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &mbox->chans[chan];
+}
+
 static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
 				       const struct of_phandle_args *sp)
 {
@@ -310,6 +411,28 @@ static void imx_mu_init_generic(struct imx_mu_priv *priv)
 	imx_mu_write(priv, 0, priv->dcfg->xCR);
 }
 
+static void imx_mu_init_scu(struct imx_mu_priv *priv)
+{
+	unsigned int i;
+
+	for (i = 0; i < IMX_MU_SCU_CHANS; i++) {
+		struct imx_mu_con_priv *cp = &priv->con_priv[i];
+
+		cp->idx = i < 2 ? 0 : i - 2;
+		cp->type = i < 2 ? i : IMX_MU_TYPE_RXDB;
+		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);
+	}
+
+	priv->mbox.num_chans = IMX_MU_SCU_CHANS;
+	priv->mbox.of_xlate = imx_mu_scu_xlate;
+
+	/* Set default MU configuration */
+	imx_mu_write(priv, 0, priv->dcfg->xCR);
+}
+
 static int imx_mu_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -396,9 +519,20 @@ static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
 	.xCR	= 0x64,
 };
 
+static const struct imx_mu_dcfg imx_mu_cfg_imx8_scu = {
+	.tx	= imx_mu_scu_tx,
+	.rx	= imx_mu_scu_rx,
+	.init	= imx_mu_init_scu,
+	.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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH V6 4/4] firmware: imx-scu: Support one TX and one RX
  2020-03-04  5:49 [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type peng.fan
                   ` (2 preceding siblings ...)
  2020-03-04  5:49 ` [PATCH V6 3/4] mailbox: imx: add SCU MU support peng.fan
@ 2020-03-04  5:49 ` peng.fan
  2020-03-12 23:09 ` [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type Leonard Crestez
  4 siblings, 0 replies; 12+ messages in thread
From: peng.fan @ 2020-03-04  5:49 UTC (permalink / raw)
  To: shawnguo, s.hauer, jassisinghbrar, o.rempel
  Cc: kernel, festevam, linux-imx, Anson.Huang, leonard.crestez,
	aisheng.dong, linux-arm-kernel, linux-kernel, 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>
---

V6:
 None
V5:
 None
V4:
 None
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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type
  2020-03-04  5:49 [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type peng.fan
                   ` (3 preceding siblings ...)
  2020-03-04  5:49 ` [PATCH V6 4/4] firmware: imx-scu: Support one TX and one RX peng.fan
@ 2020-03-12 23:09 ` Leonard Crestez
  2020-03-13  1:07   ` Peng Fan
  4 siblings, 1 reply; 12+ messages in thread
From: Leonard Crestez @ 2020-03-12 23:09 UTC (permalink / raw)
  To: Peng Fan, jassisinghbrar, o.rempel
  Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, Anson Huang,
	Aisheng Dong, linux-arm-kernel, linux-kernel

On 2020-03-04 7:55 AM, Peng Fan wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> V6:
>   Add Oleksij's R-b tag
>   Patch 3/4, per https://www.kernel.org/doc/Documentation/printk-formats.txt
>   should use %zu for printk sizeof
> 
> V5:
>   Move imx_mu_dcfg below imx_mu_priv
>   Add init hooks to imx_mu_dcfg
>   drop __packed __aligned
>   Add more debug msg
>   code style cleanup
> 
> V4:
>   Drop IMX_MU_TYPE_[GENERIC, SCU]
>   Pack MU chans init to separate function
>   Add separate function for SCU chans init and xlate
>   Add santity check to msg hdr.size
>   Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3]
> 
> 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.
> 
> 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.

Tested-by: Leonard Crestez <leonard.crestez@nxp.com>

My stress tests pass on imx8qxp with this patcheset, however performance 
is not greatly improved. My guess is that this happens because of too 
many interrupts.

Is there really a reason to enable TIE? Spinning on TE bits without any 
interrupts should be just plain faster.

> 
> 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                      | 267 +++++++++++++++++----
>   3 files changed, 260 insertions(+), 63 deletions(-)
> 
> 
> base-commit: 770fbb32d34e5d6298cc2be590c9d2fd6069aa17
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type
  2020-03-12 23:09 ` [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type Leonard Crestez
@ 2020-03-13  1:07   ` Peng Fan
  2020-03-13  7:38     ` Peng Fan
  0 siblings, 1 reply; 12+ messages in thread
From: Peng Fan @ 2020-03-13  1:07 UTC (permalink / raw)
  To: Leonard Crestez, jassisinghbrar, o.rempel
  Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, Anson Huang,
	Aisheng Dong, linux-arm-kernel, linux-kernel

Hi Leonard,

> Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel
> type
> 
> On 2020-03-04 7:55 AM, Peng Fan wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > V6:
> >   Add Oleksij's R-b tag
> >   Patch 3/4, per
> https://www.kernel.org/doc/Documentation/printk-formats.txt
> >   should use %zu for printk sizeof
> >
> > V5:
> >   Move imx_mu_dcfg below imx_mu_priv
> >   Add init hooks to imx_mu_dcfg
> >   drop __packed __aligned
> >   Add more debug msg
> >   code style cleanup
> >
> > V4:
> >   Drop IMX_MU_TYPE_[GENERIC, SCU]
> >   Pack MU chans init to separate function
> >   Add separate function for SCU chans init and xlate
> >   Add santity check to msg hdr.size
> >   Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3]
> >
> > 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.
> >
> > 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.
> 
> Tested-by: Leonard Crestez <leonard.crestez@nxp.com>
> 

Thanks for the test.

> My stress tests pass on imx8qxp with this patcheset, however performance is
> not greatly improved. My guess is that this happens because of too many
> interrupts.

Might be. Could you share your testcase?

> 
> Is there really a reason to enable TIE? Spinning on TE bits without any
> interrupts should be just plain faster.

I could try to disable TIE and give a try. If performance improves lot, I could
change to non TX interrupt.

Oleksij, do you agree?

Thanks,
Peng.

> 
> >
> > 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                      | 267
> +++++++++++++++++----
> >   3 files changed, 260 insertions(+), 63 deletions(-)
> >
> >
> > base-commit: 770fbb32d34e5d6298cc2be590c9d2fd6069aa17
> >


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type
  2020-03-13  1:07   ` Peng Fan
@ 2020-03-13  7:38     ` Peng Fan
  2020-03-18  2:44       ` Leonard Crestez
  0 siblings, 1 reply; 12+ messages in thread
From: Peng Fan @ 2020-03-13  7:38 UTC (permalink / raw)
  To: Leonard Crestez, jassisinghbrar, o.rempel
  Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, Anson Huang,
	Aisheng Dong, linux-arm-kernel, linux-kernel

> Subject: RE: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel
> type
> 
> Hi Leonard,
> 
> > Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel
> > type
> >
> > On 2020-03-04 7:55 AM, Peng Fan wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > V6:
> > >   Add Oleksij's R-b tag
> > >   Patch 3/4, per
> > https://www.kernel.org/doc/Documentation/printk-formats.txt
> > >   should use %zu for printk sizeof
> > >
> > > V5:
> > >   Move imx_mu_dcfg below imx_mu_priv
> > >   Add init hooks to imx_mu_dcfg
> > >   drop __packed __aligned
> > >   Add more debug msg
> > >   code style cleanup
> > >
> > > V4:
> > >   Drop IMX_MU_TYPE_[GENERIC, SCU]
> > >   Pack MU chans init to separate function
> > >   Add separate function for SCU chans init and xlate
> > >   Add santity check to msg hdr.size
> > >   Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3]
> > >
> > > 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.
> > >
> > > 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.
> >
> > Tested-by: Leonard Crestez <leonard.crestez@nxp.com>
> >
> 
> Thanks for the test.
> 
> > My stress tests pass on imx8qxp with this patcheset, however
> > performance is not greatly improved. My guess is that this happens
> > because of too many interrupts.
> 
> Might be. Could you share your testcase?
> 
> >
> > Is there really a reason to enable TIE? Spinning on TE bits without
> > any interrupts should be just plain faster.
> 
> I could try to disable TIE and give a try. If performance improves lot, I could
> change to non TX interrupt.

After rethinking about this, we need TX interrupt, otherwise we have to
use TX_POLL which is slower or let the client kick the TX state machine.

Compared with original method, this already reduces to use 1 TX and 1 RX
interrupt. This already good for system.

Thanks,
Peng.

> 
> Oleksij, do you agree?
> 
> Thanks,
> Peng.
> 
> >
> > >
> > > 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                      | 267
> > +++++++++++++++++----
> > >   3 files changed, 260 insertions(+), 63 deletions(-)
> > >
> > >
> > > base-commit: 770fbb32d34e5d6298cc2be590c9d2fd6069aa17
> > >


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type
  2020-03-13  7:38     ` Peng Fan
@ 2020-03-18  2:44       ` Leonard Crestez
  2020-03-18 12:14         ` Peng Fan
  0 siblings, 1 reply; 12+ messages in thread
From: Leonard Crestez @ 2020-03-18  2:44 UTC (permalink / raw)
  To: Peng Fan, jassisinghbrar, o.rempel
  Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, Anson Huang,
	Aisheng Dong, linux-arm-kernel, linux-kernel

On 2020-03-13 9:38 AM, Peng Fan wrote:
>> Subject: RE: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel
>> type
>>
>> Hi Leonard,
>>
>>> Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel
>>> type
>>>
>>> On 2020-03-04 7:55 AM, Peng Fan wrote:
>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>
>>>> V6:
>>>>    Add Oleksij's R-b tag
>>>>    Patch 3/4, per
>>> https://www.kernel.org/doc/Documentation/printk-formats.txt
>>>>    should use %zu for printk sizeof
>>>>
>>>> V5:
>>>>    Move imx_mu_dcfg below imx_mu_priv
>>>>    Add init hooks to imx_mu_dcfg
>>>>    drop __packed __aligned
>>>>    Add more debug msg
>>>>    code style cleanup
>>>>
>>>> V4:
>>>>    Drop IMX_MU_TYPE_[GENERIC, SCU]
>>>>    Pack MU chans init to separate function
>>>>    Add separate function for SCU chans init and xlate
>>>>    Add santity check to msg hdr.size
>>>>    Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3]
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>
>>> Tested-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>
>>
>> Thanks for the test.
>>
>>> My stress tests pass on imx8qxp with this patcheset, however
>>> performance is not greatly improved. My guess is that this happens
>>> because of too many interrupts.
>>
>> Might be. Could you share your testcase?

https://github.com/cdleonard/imx-scu-test

>>> Is there really a reason to enable TIE? Spinning on TE bits without
>>> any interrupts should be just plain faster.
>>
>> I could try to disable TIE and give a try. If performance improves lot, I could
>> change to non TX interrupt.
> 
> After rethinking about this, we need TX interrupt, otherwise we have to
> use TX_POLL which is slower or let the client kick the TX state machine.
> 
> Compared with original method, this already reduces to use 1 TX and 1 RX
> interrupt. This already good for system.

Sorry, I missed that fact that your patches don't include the required 
DTS changes. Indeed that is only one TX and one RX irq per call now.

Running my test now results in RX timeout :(

-----

On an unrelated note: are you sure it is appropriate to change the 
compat string here? Another way to implement direct SCU communication 
would be as another channel type, IMX_MU_TYPE_SCUTX.

It also strange that you're adding a bool fast_ipc in imx-scu, do we 
really want to support the old path?

If SCU protocol was implemented as a channel type then maybe we could 
sidestep mbox_request_channel_by_name, parse mboxes manually and always 
request MU_TYPE_SCUTX.

--
Regards,
Leonard

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V6 3/4] mailbox: imx: add SCU MU support
  2020-03-04  5:49 ` [PATCH V6 3/4] mailbox: imx: add SCU MU support peng.fan
@ 2020-03-18  2:51   ` Leonard Crestez
  2020-03-18 12:17     ` Peng Fan
  0 siblings, 1 reply; 12+ messages in thread
From: Leonard Crestez @ 2020-03-18  2:51 UTC (permalink / raw)
  To: Peng Fan, shawnguo, o.rempel
  Cc: s.hauer, jassisinghbrar, kernel, festevam, dl-linux-imx,
	Anson Huang, Aisheng Dong, linux-arm-kernel, linux-kernel,
	Franck Lenormand

On 2020-03-04 7:56 AM, Peng Fan 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
>    - For RX, support receive msg larger than 4 u32 words.
>    - Support 6 channels, TX0/RX0/RXDB[0-3], not support TXDB.
> 
> Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V6:
>   Add R-b tag
>   Use %zu for printk sizeof
> 
> V5:
>   Code style cleanup
>   Add more debug msg
>   Drop __packed aligned
>   idx santity check in scu xlate
> 
> V4:
>   Added separate chans init and xlate function for SCU MU
>   Limit chans to TX0/RX0/RXDB[0-3], max 6 chans.
>   Santity check to msg size
> 
> V3:
>   Added scu type tx/rx and SCU MU type
> 
>   drivers/mailbox/imx-mailbox.c | 134 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 134 insertions(+)
> 
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index df6c4ecd913c..0895ccd23d17 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>
> @@ -27,6 +28,8 @@
>   #define IMX_MU_xCR_GIRn(x)	BIT(16 + (3 - (x)))
>   
>   #define IMX_MU_CHANS		16
> +/* TX0/RX0/RXDB[0-3] */
> +#define IMX_MU_SCU_CHANS	6
>   #define IMX_MU_CHAN_NAME_SIZE	20
>   
>   enum imx_mu_chan_type {
> @@ -36,6 +39,11 @@ enum imx_mu_chan_type {
>   	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
>   };
>   
> +struct imx_sc_rpc_msg_max {
> +	struct imx_sc_rpc_msg hdr;
> +	u32 data[7];
> +};
> +
>   struct imx_mu_con_priv {
>   	unsigned int		idx;
>   	char			irq_desc[IMX_MU_CHAN_NAME_SIZE];
> @@ -134,6 +142,63 @@ 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:
> +		if (msg->hdr.size > sizeof(*msg)) {
> +			/*
> +			 * The real message size can be different to
> +			 * struct imx_sc_rpc_msg_max size
> +			 */
> +			dev_err(priv->dev, "Exceed max msg size (%zu) on TX, got: %i\n", sizeof(*msg), msg->hdr.size);
> +			return -EINVAL;
> +		}
> +
> +		for (i = 0; i < msg->hdr.size; i++) > +			imx_mu_write(priv, *arg++, priv->dcfg->xTR[i % 4]);

Need to poll for TE, otherwise for long messages we could overwrite TR0 
before SCU reads it.

> +
> +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);

Shouldn't TIE be armed ahead of writing? In theory SCU could read first 
register before loop above is over.

> +		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]);
> +
> +	if (msg.hdr.size > sizeof(msg)) {
> +		dev_err(priv->dev, "Exceed max msg size (%zu) on RX, got: %i\n",
> +			sizeof(msg), msg.hdr.size);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 1; i < msg.hdr.size; i++)
> +		*data++ = imx_mu_read(priv, priv->dcfg->xRR[i % 4]);

Shouldn't you poll for !RE? It's possible to receive RX interrupt and 
start reading before other side writes the second word.

> +
> +	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;
> @@ -263,6 +328,42 @@ static const struct mbox_chan_ops imx_mu_ops = {
>   	.shutdown = imx_mu_shutdown,
>   };
>   
> +static struct mbox_chan *imx_mu_scu_xlate(struct mbox_controller *mbox,
> +					  const struct of_phandle_args *sp)
> +{
> +	u32 type, idx, chan;
> +
> +	if (sp->args_count != 2) {
> +		dev_err(mbox->dev, "Invalid argument count %d\n", sp->args_count);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	type = sp->args[0]; /* channel type */
> +	idx = sp->args[1]; /* index */
> +
> +	switch (type) {
> +	case IMX_MU_TYPE_TX:
> +	case IMX_MU_TYPE_RX:
> +		if (idx != 0)
> +			dev_err(mbox->dev, "Invalid chan idx: %d\n", idx);
> +		chan = type;
> +		break;
> +	case IMX_MU_TYPE_RXDB:
> +		chan = 2 + idx;
> +		break;
> +	default:
> +		dev_err(mbox->dev, "Invalid chan type: %d\n", type);
> +		return NULL;
> +	}
> +
> +	if (chan >= mbox->num_chans) {
> +		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return &mbox->chans[chan];
> +}
> +
>   static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
>   				       const struct of_phandle_args *sp)
>   {
> @@ -310,6 +411,28 @@ static void imx_mu_init_generic(struct imx_mu_priv *priv)
>   	imx_mu_write(priv, 0, priv->dcfg->xCR);
>   }
>   
> +static void imx_mu_init_scu(struct imx_mu_priv *priv)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < IMX_MU_SCU_CHANS; i++) {
> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> +		cp->idx = i < 2 ? 0 : i - 2;
> +		cp->type = i < 2 ? i : IMX_MU_TYPE_RXDB;
> +		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);
> +	}
> +
> +	priv->mbox.num_chans = IMX_MU_SCU_CHANS;
> +	priv->mbox.of_xlate = imx_mu_scu_xlate;
> +
> +	/* Set default MU configuration */
> +	imx_mu_write(priv, 0, priv->dcfg->xCR);
> +}
> +
>   static int imx_mu_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -396,9 +519,20 @@ static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
>   	.xCR	= 0x64,
>   };
>   
> +static const struct imx_mu_dcfg imx_mu_cfg_imx8_scu = {
> +	.tx	= imx_mu_scu_tx,
> +	.rx	= imx_mu_scu_rx,
> +	.init	= imx_mu_init_scu,
> +	.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);
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type
  2020-03-18  2:44       ` Leonard Crestez
@ 2020-03-18 12:14         ` Peng Fan
  0 siblings, 0 replies; 12+ messages in thread
From: Peng Fan @ 2020-03-18 12:14 UTC (permalink / raw)
  To: Leonard Crestez, jassisinghbrar, o.rempel
  Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, Anson Huang,
	Aisheng Dong, linux-arm-kernel, linux-kernel

> Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel
> type
> 
> On 2020-03-13 9:38 AM, Peng Fan wrote:
> >> Subject: RE: [PATCH V6 0/4] mailbox/firmware: imx: support SCU
> >> channel type
> >>
> >> Hi Leonard,
> >>
> >>> Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU
> >>> channel type
> >>>
> >>> On 2020-03-04 7:55 AM, Peng Fan wrote:
> >>>> From: Peng Fan <peng.fan@nxp.com>
> >>>>
> >>>> V6:
> >>>>    Add Oleksij's R-b tag
> >>>>    Patch 3/4, per
> >>> https://www.kernel.org/doc/Documentation/printk-formats.txt
> >>>>    should use %zu for printk sizeof
> >>>>
> >>>> V5:
> >>>>    Move imx_mu_dcfg below imx_mu_priv
> >>>>    Add init hooks to imx_mu_dcfg
> >>>>    drop __packed __aligned
> >>>>    Add more debug msg
> >>>>    code style cleanup
> >>>>
> >>>> V4:
> >>>>    Drop IMX_MU_TYPE_[GENERIC, SCU]
> >>>>    Pack MU chans init to separate function
> >>>>    Add separate function for SCU chans init and xlate
> >>>>    Add santity check to msg hdr.size
> >>>>    Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3]
> >>>>
> >>>> 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.
> >>>>
> >>>> 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.
> >>>
> >>> Tested-by: Leonard Crestez <leonard.crestez@nxp.com>
> >>>
> >>
> >> Thanks for the test.
> >>
> >>> My stress tests pass on imx8qxp with this patcheset, however
> >>> performance is not greatly improved. My guess is that this happens
> >>> because of too many interrupts.
> >>
> >> Might be. Could you share your testcase?
> 
> https://github.com/cdleonard/imx-scu-test
> 
> >>> Is there really a reason to enable TIE? Spinning on TE bits without
> >>> any interrupts should be just plain faster.
> >>
> >> I could try to disable TIE and give a try. If performance improves
> >> lot, I could change to non TX interrupt.
> >
> > After rethinking about this, we need TX interrupt, otherwise we have
> > to use TX_POLL which is slower or let the client kick the TX state machine.
> >
> > Compared with original method, this already reduces to use 1 TX and 1
> > RX interrupt. This already good for system.
> 
> Sorry, I missed that fact that your patches don't include the required DTS
> changes. Indeed that is only one TX and one RX irq per call now.
> 
> Running my test now results in RX timeout :(

Might be long that 4 word messages, because not check TX empty
and RX full in my patch.

> 
> -----
> 
> On an unrelated note: are you sure it is appropriate to change the compat
> string here? Another way to implement direct SCU communication would be
> as another channel type, IMX_MU_TYPE_SCUTX.

No. This will introduce more complexity. Per Oleksij's suggestion, I added
the compatible string.

> 
> It also strange that you're adding a bool fast_ipc in imx-scu, do we really want
> to support the old path?

It is to avoid break DT backward compatibility.

Thanks,
Peng.

> 
> If SCU protocol was implemented as a channel type then maybe we could
> sidestep mbox_request_channel_by_name, parse mboxes manually and
> always request MU_TYPE_SCUTX.
> 
> --
> Regards,
> Leonard

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH V6 3/4] mailbox: imx: add SCU MU support
  2020-03-18  2:51   ` Leonard Crestez
@ 2020-03-18 12:17     ` Peng Fan
  0 siblings, 0 replies; 12+ messages in thread
From: Peng Fan @ 2020-03-18 12:17 UTC (permalink / raw)
  To: Leonard Crestez, shawnguo, o.rempel
  Cc: s.hauer, jassisinghbrar, kernel, festevam, dl-linux-imx,
	Anson Huang, Aisheng Dong, linux-arm-kernel, linux-kernel,
	Franck Lenormand

> Subject: Re: [PATCH V6 3/4] mailbox: imx: add SCU MU support
> 
> On 2020-03-04 7:56 AM, Peng Fan 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
> >    - For RX, support receive msg larger than 4 u32 words.
> >    - Support 6 channels, TX0/RX0/RXDB[0-3], not support TXDB.
> >
> > Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V6:
> >   Add R-b tag
> >   Use %zu for printk sizeof
> >
> > V5:
> >   Code style cleanup
> >   Add more debug msg
> >   Drop __packed aligned
> >   idx santity check in scu xlate
> >
> > V4:
> >   Added separate chans init and xlate function for SCU MU
> >   Limit chans to TX0/RX0/RXDB[0-3], max 6 chans.
> >   Santity check to msg size
> >
> > V3:
> >   Added scu type tx/rx and SCU MU type
> >
> >   drivers/mailbox/imx-mailbox.c | 134
> ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 134 insertions(+)
> >
> > diff --git a/drivers/mailbox/imx-mailbox.c
> > b/drivers/mailbox/imx-mailbox.c index df6c4ecd913c..0895ccd23d17
> > 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>
> > @@ -27,6 +28,8 @@
> >   #define IMX_MU_xCR_GIRn(x)	BIT(16 + (3 - (x)))
> >
> >   #define IMX_MU_CHANS		16
> > +/* TX0/RX0/RXDB[0-3] */
> > +#define IMX_MU_SCU_CHANS	6
> >   #define IMX_MU_CHAN_NAME_SIZE	20
> >
> >   enum imx_mu_chan_type {
> > @@ -36,6 +39,11 @@ enum imx_mu_chan_type {
> >   	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
> >   };
> >
> > +struct imx_sc_rpc_msg_max {
> > +	struct imx_sc_rpc_msg hdr;
> > +	u32 data[7];
> > +};
> > +
> >   struct imx_mu_con_priv {
> >   	unsigned int		idx;
> >   	char			irq_desc[IMX_MU_CHAN_NAME_SIZE];
> > @@ -134,6 +142,63 @@ 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:
> > +		if (msg->hdr.size > sizeof(*msg)) {
> > +			/*
> > +			 * The real message size can be different to
> > +			 * struct imx_sc_rpc_msg_max size
> > +			 */
> > +			dev_err(priv->dev, "Exceed max msg size (%zu) on TX,
> got: %i\n", sizeof(*msg), msg->hdr.size);
> > +			return -EINVAL;
> > +		}
> > +
> > +		for (i = 0; i < msg->hdr.size; i++) > +			imx_mu_write(priv,
> *arg++, priv->dcfg->xTR[i % 4]);
> 
> Need to poll for TE, otherwise for long messages we could overwrite TR0
> before SCU reads it.

I planned to add a follow up patch for long word messages.
Since you raised it, I'll add check for TX for send, RX for recv.

> 
> > +
> > +		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
> 
> Shouldn't TIE be armed ahead of writing? In theory SCU could read first
> register before loop above is over.

This is to avoid TX interrupt be triggered early.

> 
> > +		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]);
> > +
> > +	if (msg.hdr.size > sizeof(msg)) {
> > +		dev_err(priv->dev, "Exceed max msg size (%zu) on RX, got: %i\n",
> > +			sizeof(msg), msg.hdr.size);
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (i = 1; i < msg.hdr.size; i++)
> > +		*data++ = imx_mu_read(priv, priv->dcfg->xRR[i % 4]);
> 
> Shouldn't you poll for !RE? It's possible to receive RX interrupt and start
> reading before other side writes the second word.

Will add RE check.


Thanks,
Peng.

> 
> > +
> > +	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; @@
> > -263,6 +328,42 @@ static const struct mbox_chan_ops imx_mu_ops = {
> >   	.shutdown = imx_mu_shutdown,
> >   };
> >
> > +static struct mbox_chan *imx_mu_scu_xlate(struct mbox_controller
> *mbox,
> > +					  const struct of_phandle_args *sp) {
> > +	u32 type, idx, chan;
> > +
> > +	if (sp->args_count != 2) {
> > +		dev_err(mbox->dev, "Invalid argument count %d\n",
> sp->args_count);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	type = sp->args[0]; /* channel type */
> > +	idx = sp->args[1]; /* index */
> > +
> > +	switch (type) {
> > +	case IMX_MU_TYPE_TX:
> > +	case IMX_MU_TYPE_RX:
> > +		if (idx != 0)
> > +			dev_err(mbox->dev, "Invalid chan idx: %d\n", idx);
> > +		chan = type;
> > +		break;
> > +	case IMX_MU_TYPE_RXDB:
> > +		chan = 2 + idx;
> > +		break;
> > +	default:
> > +		dev_err(mbox->dev, "Invalid chan type: %d\n", type);
> > +		return NULL;
> > +	}
> > +
> > +	if (chan >= mbox->num_chans) {
> > +		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d,
> idx: %d)\n", chan, type, idx);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	return &mbox->chans[chan];
> > +}
> > +
> >   static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
> >   				       const struct of_phandle_args *sp)
> >   {
> > @@ -310,6 +411,28 @@ static void imx_mu_init_generic(struct
> imx_mu_priv *priv)
> >   	imx_mu_write(priv, 0, priv->dcfg->xCR);
> >   }
> >
> > +static void imx_mu_init_scu(struct imx_mu_priv *priv) {
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < IMX_MU_SCU_CHANS; i++) {
> > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > +
> > +		cp->idx = i < 2 ? 0 : i - 2;
> > +		cp->type = i < 2 ? i : IMX_MU_TYPE_RXDB;
> > +		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);
> > +	}
> > +
> > +	priv->mbox.num_chans = IMX_MU_SCU_CHANS;
> > +	priv->mbox.of_xlate = imx_mu_scu_xlate;
> > +
> > +	/* Set default MU configuration */
> > +	imx_mu_write(priv, 0, priv->dcfg->xCR); }
> > +
> >   static int imx_mu_probe(struct platform_device *pdev)
> >   {
> >   	struct device *dev = &pdev->dev;
> > @@ -396,9 +519,20 @@ static const struct imx_mu_dcfg
> imx_mu_cfg_imx7ulp = {
> >   	.xCR	= 0x64,
> >   };
> >
> > +static const struct imx_mu_dcfg imx_mu_cfg_imx8_scu = {
> > +	.tx	= imx_mu_scu_tx,
> > +	.rx	= imx_mu_scu_rx,
> > +	.init	= imx_mu_init_scu,
> > +	.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);
> >


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-03-18 12:17 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-04  5:49 [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type peng.fan
2020-03-04  5:49 ` [PATCH V6 1/4] dt-bindings: mailbox: imx-mu: add SCU MU support peng.fan
2020-03-04  5:49 ` [PATCH V6 2/4] mailbox: imx: restructure code to make easy for new MU peng.fan
2020-03-04  5:49 ` [PATCH V6 3/4] mailbox: imx: add SCU MU support peng.fan
2020-03-18  2:51   ` Leonard Crestez
2020-03-18 12:17     ` Peng Fan
2020-03-04  5:49 ` [PATCH V6 4/4] firmware: imx-scu: Support one TX and one RX peng.fan
2020-03-12 23:09 ` [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel type Leonard Crestez
2020-03-13  1:07   ` Peng Fan
2020-03-13  7:38     ` Peng Fan
2020-03-18  2:44       ` Leonard Crestez
2020-03-18 12:14         ` Peng Fan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).