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:

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

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

++++++++++++++++++++++++++++++++++++++++++-

quoted

  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,

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:

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;

quoted

+		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);

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 sanity check. the message size should not be higher then
sizeof(msg)

Yes.

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

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.

quoted

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

quoted

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

quoted

@@ -284,7 +334,9 @@ static struct mbox_chan * imx_mu_xlate(struct

mbox_controller *mbox,

quoted

  	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.

quoted

  		dev_err(mbox->dev, "Not supported channel number: %d.

(type: %d, idx: %d)\n", chan, type, idx);

quoted

  		return ERR_PTR(-EINVAL);
  	}

@@ -401,9 +453,20 @@ static const struct imx_mu_dcfg

imx_mu_cfg_imx7ulp = {

quoted

  	.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&data=02%7C01%7Cpeng.fan%40nxp.com%7C47558a
c6c6bd4ba38c2708d7be8eb946%7C686ea1d3bc2b4c6fa92cd99c5c301635%
7C0%7C0%7C637187392512520476&sdata=N0WbBREAFegjX0p%2FSv3
cuLpb8FvnCePU%2Bz6VkhC%2FU4c%3D&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