[PATCH V2 0/2] mailbox/firmware: imx: support SCU channel type

[PATCH V2 0/2] mailbox/firmware: imx: support SCU channel type

[peng.fan]

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

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.

Pachset based on i.MX Shawn's for-next branch, commit fd7eba9fa1f534b710.

To test this patchset, I applied the below diff:
 diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
 index fb5f752b15fe..c5636624726e 100644
 --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
 +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
 @@ -140,17 +140,11 @@
 
         scu {
                 compatible = "fsl,imx-scu";
 -               mbox-names = "tx0", "tx1", "tx2", "tx3",
 -                            "rx0", "rx1", "rx2", "rx3",
 +               mbox-names = "tx0",
 +                            "rx0",
                              "gip3";
                 mboxes = <&lsio_mu1 0 0
 -                         &lsio_mu1 0 1
 -                         &lsio_mu1 0 2
 -                         &lsio_mu1 0 3
                           &lsio_mu1 1 0
 -                         &lsio_mu1 1 1
 -                         &lsio_mu1 1 2
 -                         &lsio_mu1 1 3
                           &lsio_mu1 3 3>;
 
                 clk: clock-controller {

Peng Fan (2):
  mailbox: imx: support SCU channel type
  firmware: imx-scu: Support one TX and one RX

 drivers/firmware/imx/imx-scu.c | 48 ++++++++++++++++++++++++++++++++----------
 drivers/mailbox/imx-mailbox.c  | 46 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 79 insertions(+), 15 deletions(-)


base-commit: fd7eba9fa1f534b7102f4762e25c991f78ec283d
-- 
2.16.4

[PATCH V2 1/2] mailbox: imx: support SCU channel type

[peng.fan]

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

Per i.MX8QXP 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 channel type work,
  - parse the size of msg.
  - Only enable TR0/RR0 interrupt for transmit/receive message.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/mailbox/imx-mailbox.c | 46 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 2cdcdc5f1119..0b4a33254114 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>
@@ -65,8 +66,14 @@ struct imx_mu_priv {
 	int			irq;
 
 	bool			side_b;
+	bool			scu;
 };
 
+struct imx_sc_rpc_msg_max {
+	struct imx_sc_rpc_msg hdr;
+	u32 data[7];
+} __packed __aligned(4);;
+
 static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
 	.xTR	= {0x0, 0x4, 0x8, 0xc},
 	.xRR	= {0x10, 0x14, 0x18, 0x1c},
@@ -123,7 +130,10 @@ 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;
+	struct imx_sc_rpc_msg_max msg;
+	u32 *p_msg = (u32 *)&msg;
 	u32 val, ctrl, dat;
+	int i;
 
 	ctrl = imx_mu_read(priv, priv->dcfg->xCR);
 	val = imx_mu_read(priv, priv->dcfg->xSR);
@@ -152,8 +162,19 @@ 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);
+		if (!priv->scu) {
+			dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
+			mbox_chan_received_data(chan, (void *)&dat);
+		} else {
+			imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0));
+			*p_msg++ = imx_mu_read(priv, priv->dcfg->xRR[0]);
+			for (i = 1; i < msg.hdr.size; i++) {
+				*p_msg++ = imx_mu_read(priv,
+						       priv->dcfg->xRR[i % 4]);
+			}
+			imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(0), 0);
+			mbox_chan_received_data(chan, (void *)&msg);
+		}
 	} 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,11 +190,20 @@ 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;
+	struct imx_sc_rpc_msg_max *msg = data;
 	u32 *arg = data;
+	int i;
 
 	switch (cp->type) {
 	case IMX_MU_TYPE_TX:
-		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
+		if (priv->scu) {
+			for (i = 0; i < msg->hdr.size; i++) {
+				imx_mu_write(priv, *arg++,
+					     priv->dcfg->xTR[i % 4]);
+			}
+		} else {
+			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:
@@ -259,6 +289,7 @@ static const struct mbox_chan_ops imx_mu_ops = {
 static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
 				       const struct of_phandle_args *sp)
 {
+	struct imx_mu_priv *priv = to_imx_mu_priv(mbox);
 	u32 type, idx, chan;
 
 	if (sp->args_count != 2) {
@@ -270,7 +301,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) {
+	/* For TX/RX SCU, only one channel supported */
+	if ((chan >= mbox->num_chans) ||
+	    (priv->scu && type < 1 && idx >= 1)) {
 		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx);
 		return ERR_PTR(-EINVAL);
 	}
@@ -341,6 +374,11 @@ static int imx_mu_probe(struct platform_device *pdev)
 	}
 
 	priv->side_b = of_property_read_bool(np, "fsl,mu-side-b");
+	np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu");
+	if (np) {
+		priv->scu = true;
+		of_node_put(np);
+	}
 
 	spin_lock_init(&priv->xcr_lock);
 
-- 
2.16.4

Re: [PATCH V2 1/2] mailbox: imx: support SCU channel type

[Oleksij Rempel]

Hi,

On 25.02.20 04:17, peng.fan@nxp.com wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Per i.MX8QXP 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 channel type work,
>    - parse the size of msg.
>    - Only enable TR0/RR0 interrupt for transmit/receive message.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   drivers/mailbox/imx-mailbox.c | 46 +++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 2cdcdc5f1119..0b4a33254114 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>
> @@ -65,8 +66,14 @@ struct imx_mu_priv {
>   	int			irq;
>   
>   	bool			side_b;
> +	bool			scu;
>   };
>   
> +struct imx_sc_rpc_msg_max {
> +	struct imx_sc_rpc_msg hdr;
> +	u32 data[7];
> +} __packed __aligned(4);;
> +
>   static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
>   	.xTR	= {0x0, 0x4, 0x8, 0xc},
>   	.xRR	= {0x10, 0x14, 0x18, 0x1c},
> @@ -123,7 +130,10 @@ 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;
> +	struct imx_sc_rpc_msg_max msg;
> +	u32 *p_msg = (u32 *)&msg;
>   	u32 val, ctrl, dat;
> +	int i;
>   
>   	ctrl = imx_mu_read(priv, priv->dcfg->xCR);
>   	val = imx_mu_read(priv, priv->dcfg->xSR);
> @@ -152,8 +162,19 @@ 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);
> +		if (!priv->scu) {
> +			dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
> +			mbox_chan_received_data(chan, (void *)&dat);
> +		} else {
> +			imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0));
> +			*p_msg++ = imx_mu_read(priv, priv->dcfg->xRR[0]);
> +			for (i = 1; i < msg.hdr.size; i++) {
> +				*p_msg++ = imx_mu_read(priv,
> +						       priv->dcfg->xRR[i % 4]);
> +			}
> +			imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(0), 0);
> +			mbox_chan_received_data(chan, (void *)&msg);
> +		}
>   	} 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,11 +190,20 @@ 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;
> +	struct imx_sc_rpc_msg_max *msg = data;
>   	u32 *arg = data;
> +	int i;
>   
>   	switch (cp->type) {
>   	case IMX_MU_TYPE_TX:
> -		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
> +		if (priv->scu) {
> +			for (i = 0; i < msg->hdr.size; i++) {
> +				imx_mu_write(priv, *arg++,
> +					     priv->dcfg->xTR[i % 4]);
> +			}
> +		} else {
> +			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:
> @@ -259,6 +289,7 @@ static const struct mbox_chan_ops imx_mu_ops = {
>   static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
>   				       const struct of_phandle_args *sp)
>   {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(mbox);
>   	u32 type, idx, chan;
>   
>   	if (sp->args_count != 2) {
> @@ -270,7 +301,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) {
> +	/* For TX/RX SCU, only one channel supported */
> +	if ((chan >= mbox->num_chans) ||
> +	    (priv->scu && type < 1 && idx >= 1)) {
>   		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx);
>   		return ERR_PTR(-EINVAL);
>   	}
> @@ -341,6 +374,11 @@ static int imx_mu_probe(struct platform_device *pdev)
>   	}
>   
>   	priv->side_b = of_property_read_bool(np, "fsl,mu-side-b");
> +	np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu");

This will configure every MU on the SoC as SCU, even it is not communicating with it.

> +	if (np) {
> +		priv->scu = true;
> +		of_node_put(np);
> +	}
>   

Please take a look at drivers/remoteproc/imx_rproc.c
especially the use of struct imx_rproc_dcfg.
Introduce simialr struct in the imx-mailbox.c

It will be some thing like:
struct imx_mu_dcfg {
	unsigned int chans; /* number of supported channels */
	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp);
	int (*rx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp);
}

Define functions for generic tx/rx. And for SCU specific tx/rx. Please, add sanity check 
in to SCU specific functions. In current implementation an error on SCU side will couse 
stack corruption on Linux side.

Use compatible string (some thing like fsl,imx8-mu-scu) to detect proper metrics of new 
device.

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 V2 1/2] mailbox: imx: support SCU channel type

[Peng Fan]

Hi Oleksij,

> Subject: Re: [PATCH V2 1/2] mailbox: imx: support SCU channel type
> 
> Hi,
> 
> On 25.02.20 04:17, peng.fan@nxp.com wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Per i.MX8QXP 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 channel type work,
> >    - parse the size of msg.
> >    - Only enable TR0/RR0 interrupt for transmit/receive message.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >   drivers/mailbox/imx-mailbox.c | 46
> +++++++++++++++++++++++++++++++++++++++----
> >   1 file changed, 42 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mailbox/imx-mailbox.c
> > b/drivers/mailbox/imx-mailbox.c index 2cdcdc5f1119..0b4a33254114
> > 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>
> > @@ -65,8 +66,14 @@ struct imx_mu_priv {
> >   	int			irq;
> >
> >   	bool			side_b;
> > +	bool			scu;
> >   };
> >
> > +struct imx_sc_rpc_msg_max {
> > +	struct imx_sc_rpc_msg hdr;
> > +	u32 data[7];
> > +} __packed __aligned(4);;
> > +
> >   static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> >   	.xTR	= {0x0, 0x4, 0x8, 0xc},
> >   	.xRR	= {0x10, 0x14, 0x18, 0x1c},
> > @@ -123,7 +130,10 @@ 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;
> > +	struct imx_sc_rpc_msg_max msg;
> > +	u32 *p_msg = (u32 *)&msg;
> >   	u32 val, ctrl, dat;
> > +	int i;
> >
> >   	ctrl = imx_mu_read(priv, priv->dcfg->xCR);
> >   	val = imx_mu_read(priv, priv->dcfg->xSR); @@ -152,8 +162,19 @@
> > 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);
> > +		if (!priv->scu) {
> > +			dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
> > +			mbox_chan_received_data(chan, (void *)&dat);
> > +		} else {
> > +			imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0));
> > +			*p_msg++ = imx_mu_read(priv, priv->dcfg->xRR[0]);
> > +			for (i = 1; i < msg.hdr.size; i++) {
> > +				*p_msg++ = imx_mu_read(priv,
> > +						       priv->dcfg->xRR[i % 4]);
> > +			}
> > +			imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(0), 0);
> > +			mbox_chan_received_data(chan, (void *)&msg);
> > +		}
> >   	} 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,11 +190,20
> @@ 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;
> > +	struct imx_sc_rpc_msg_max *msg = data;
> >   	u32 *arg = data;
> > +	int i;
> >
> >   	switch (cp->type) {
> >   	case IMX_MU_TYPE_TX:
> > -		imx_mu_write(priv, *arg, priv->dcfg->xTR[cp->idx]);
> > +		if (priv->scu) {
> > +			for (i = 0; i < msg->hdr.size; i++) {
> > +				imx_mu_write(priv, *arg++,
> > +					     priv->dcfg->xTR[i % 4]);
> > +			}
> > +		} else {
> > +			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:
> > @@ -259,6 +289,7 @@ static const struct mbox_chan_ops imx_mu_ops = {
> >   static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
> >   				       const struct of_phandle_args *sp)
> >   {
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(mbox);
> >   	u32 type, idx, chan;
> >
> >   	if (sp->args_count != 2) {
> > @@ -270,7 +301,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) {
> > +	/* For TX/RX SCU, only one channel supported */
> > +	if ((chan >= mbox->num_chans) ||
> > +	    (priv->scu && type < 1 && idx >= 1)) {
> >   		dev_err(mbox->dev, "Not supported channel number: %d.
> (type: %d, idx: %d)\n", chan, type, idx);
> >   		return ERR_PTR(-EINVAL);
> >   	}
> > @@ -341,6 +374,11 @@ static int imx_mu_probe(struct platform_device
> *pdev)
> >   	}
> >
> >   	priv->side_b = of_property_read_bool(np, "fsl,mu-side-b");
> > +	np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu");
> 
> This will configure every MU on the SoC as SCU, even it is not communicating
> with it.
> 
> > +	if (np) {
> > +		priv->scu = true;
> > +		of_node_put(np);
> > +	}
> >
> 
> Please take a look at drivers/remoteproc/imx_rproc.c especially the use of
> struct imx_rproc_dcfg.
> Introduce simialr struct in the imx-mailbox.c

ok, I'll introduce something like that.

> 
> It will be some thing like:
> struct imx_mu_dcfg {
> 	unsigned int chans; /* number of supported channels */
> 	int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp);
> 	int (*rx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp); }
> 
> Define functions for generic tx/rx. And for SCU specific tx/rx. Please, add
> sanity check in to SCU specific functions. In current implementation an error
> on SCU side will couse stack corruption on Linux side.

We expect SCU not crash or have error behavior. Anyway I'll add check in Linux
side.

> 
> Use compatible string (some thing like fsl,imx8-mu-scu) to detect proper
> metrics of new device.

ok. Thanks for your suggestions.

Thanks,
Peng.

> 
> 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%7C96e4b5
> fa939d47929c4208d7b9bc26b4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637182090067313481&amp;sdata=y%2BI7j05Cxsz2e67aNiWD3
> Tg3%2FCY1JNIIBIfGm4j1bB4%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 |

[PATCH V2 2/2] 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>
---
 drivers/firmware/imx/imx-scu.c | 48 ++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index f71eaa5bf52d..c839dbaaf3da 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,7 @@ static int imx_scu_probe(struct platform_device *pdev)
 	struct imx_sc_chan *sc_chan;
 	struct mbox_client *cl;
 	char *chan_name;
+	int num_channel;
 	int ret;
 	int i;
 
@@ -253,11 +273,15 @@ 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)
+	sc_ipc->fast_ipc = true;
+
+	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 +293,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