Thread (10 messages) 10 messages, 2 authors, 2020-03-03

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

From: Peng Fan <peng.fan@nxp.com>
Date: 2020-03-03 06:54:06
Also in: lkml 

Subject: Re: [PATCH V4 3/4] mailbox: imx: add SCU MU support



On 03.03.20 02:52, peng.fan@nxp.com wrote:
quoted
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.
quoted
Per i.MX8/8X Reference mannual, Chapter "12.9.2.3.2 Messaging
Examples",
quoted
  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.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
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 | 128
+++++++++++++++++++++++++++++++++++++++++-
quoted
  1 file changed, 127 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/imx-mailbox.c
b/drivers/mailbox/imx-mailbox.c index e98f3550f995..fbdcd68d8490
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 {
@@ -39,6 +42,11 @@ enum imx_mu_chan_type {
  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);
__packed __aligned(4) looks like some sort of superstition. hdr is 4 byte and
u32 is 4 byte as well. What kind of problems should it solve?
Please see
https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/
commit/?h=for-next&id=2a182f27ff5d56d3af99b986b7321fc81aa80d65

Seems not needed. I'll drop it in V5.
quoted
+
  struct imx_mu_dcfg {
  	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, void
*data);
quoted
  	int (*rx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp);
@@ -136,6 +144,56 @@ static int imx_mu_generic_rx(struct imx_mu_priv
*priv,
quoted
  	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(struct imx_sc_rpc_msg_max)) {
please use, sizeof(*msg)

and we need here a comment:
/* the real message size can be different to struct imx_sc_rpc_msg_max size
*/
quoted
+			dev_err(priv->dev, "Exceed max msg size\n");
please,
dev_err(priv->dev, "Exceed max msg size (%i) on TX, got: %i\n", sizeof(msg),
msg.hdr.size);

or some thing like this.
ok, fix in v5.

quoted
+			return -EINVAL;
+		}
please add space
Fix in v5.

quoted
+		for (i = 0; i < msg->hdr.size; i++) {
+			imx_mu_write(priv, *arg++,
+				     priv->dcfg->xTR[i % 4]);
+		}
please remove {}
Fix in V5.

quoted
+		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);
quoted
+		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 space
Fix in v5.

quoted
+	if (msg.hdr.size > sizeof(struct imx_sc_rpc_msg_max)) {
please use sizeof(msg) instead.

quoted
+		dev_err(priv->dev, "Exceed max msg size\n");
please,
dev_err(priv->dev, "Exceed max msg size (%i) on RX, got: %i\n", sizeof(msg),
msg.hdr.size);

or some thing like this.
Fix in v5.

quoted
+		return -EINVAL;
+	}
please add space
Fix in v5.

quoted
+	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;
@@ -265,6 +323,39 @@ static const struct mbox_chan_ops imx_mu_ops =
{
quoted
  	.shutdown = imx_mu_shutdown,
  };

+static struct mbox_chan *imx_mu_scu_xlate(struct mbox_controller
*mbox,
quoted
+					  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);
quoted
+		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:
please add sanity check for idx and print some error.
Will add something as below:
 if (idx != 0)
	dev_err(mbox->dev, "Invalid channel index: %d\n", idx)

quoted
+		chan = type;
+		break;
+	case IMX_MU_TYPE_RXDB:
+		chan = 2 + idx;
+		break;
+	default:
please print here some useful error.
Ok.

quoted
+		return NULL;
+	}
+
+	if (chan >= mbox->num_chans) {
+		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d,
idx: %d)\n", chan, type, idx);
quoted
+		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)
  {
@@ -312,6 +403,28 @@ static void imx_mu_init_generic(struct
imx_mu_priv *priv)
quoted
  	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;
@@ -355,7 +468,10 @@ static int imx_mu_probe(struct platform_device
*pdev)
quoted
  	priv->side_b = of_property_read_bool(np, "fsl,mu-side-b");

-	imx_mu_init_generic(priv);
+	if (of_device_is_compatible(np, "fsl,imx8-mu-scu"))
+		imx_mu_init_scu(priv);
+	else
+		imx_mu_init_generic(priv);
we already have a way to execute device specific functions, please be
consequent.
will use priv->dcfg->init

Thanks,
Peng.

quoted
  	spin_lock_init(&priv->xcr_lock);

@@ -396,9 +512,19 @@ static const struct imx_mu_dcfg
imx_mu_cfg_imx7ulp = {
quoted
  	.xCR	= 0x64,
  };

+static const struct imx_mu_dcfg imx_mu_cfg_imx8_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%7C211195
82757947bfa7e708d7bf3e9301%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
C0%7C0%7C637188147787211737&amp;sdata=qQaXzBkxr7KB%2F%2BW3Tq
PiZH3TX%2FeZ5Cu6wl8O889XiU8%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 |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
  



    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help