| Message ID | 1582692043-683-4-git-send-email-peng.fan@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mailbox/firmware: imx: support SCU channel type | expand |
Hi, On 26.02.20 05:40, peng.fan@nxp.com wrote: > From: Peng Fan <peng.fan@nxp.com> > > i.MX8/8X SCU MU is dedicated for communication between SCU and Cortex-A > cores from hardware design, and could not be reused for other purpose. > > Per i.MX8/8X Reference mannual, Chapter "12.9.2.3.2 Messaging Examples", > Passing short messages: Transmit register(s) can be used to pass > short messages from one to four words in length. For example, when > a four-word message is desired, only one of the registers needs to > have its corresponding interrupt enable bit set at the receiver side; > the message’s first three words are written to the registers whose > interrupt is masked, and the fourth word is written to the other > register (which triggers an interrupt at the receiver side). > > i.MX8/8X SCU firmware IPC is an implementation of passing short > messages. But current imx-mailbox driver only support one word > message, i.MX8/8X linux side firmware has to request four TX > and four RX to support IPC to SCU firmware. This is low efficent > and more interrupts triggered compared with one TX and > one RX. > > To make SCU MU work, > - parse the size of msg. > - Only enable TR0/RR0 interrupt for transmit/receive message. > - For TX/RX, only support one TX channel and one RX channel > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > > V3: > Added scu type tx/rx and SCU MU type > > drivers/mailbox/imx-mailbox.c | 65 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 64 insertions(+), 1 deletion(-) > > diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c > index 901a3431fdb5..41664a64c5fd 100644 > --- a/drivers/mailbox/imx-mailbox.c > +++ b/drivers/mailbox/imx-mailbox.c > @@ -4,6 +4,7 @@ > */ > > #include <linux/clk.h> > +#include <linux/firmware/imx/ipc.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/kernel.h> > @@ -38,11 +39,17 @@ enum imx_mu_chan_type { > > enum imx_mu_type { > IMX_MU_TYPE_GENERIC, > + IMX_MU_TYPE_SCU, > }; > > struct imx_mu_priv; > struct imx_mu_con_priv; > > +struct imx_sc_rpc_msg_max { > + struct imx_sc_rpc_msg hdr; > + u32 data[7]; > +} __packed __aligned(4);; > + > struct imx_mu_dcfg { > enum imx_mu_type type; > int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, void *data); > @@ -141,6 +148,48 @@ static int imx_mu_generic_rx(struct imx_mu_priv *priv, > return 0; > } > > +static int imx_mu_scu_tx(struct imx_mu_priv *priv, > + struct imx_mu_con_priv *cp, > + void *data) > +{ > + struct imx_sc_rpc_msg_max *msg = data; > + u32 *arg = data; > + int i; > + > + switch (cp->type) { > + case IMX_MU_TYPE_TX: please add sanity check if msg->hdr.size can be handled by this driver version. > + for (i = 0; i < msg->hdr.size; i++) { > + imx_mu_write(priv, *arg++, > + priv->dcfg->xTR[i % 4]); > + } > + imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0); > + break; > + default: > + dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int imx_mu_scu_rx(struct imx_mu_priv *priv, > + struct imx_mu_con_priv *cp) > +{ > + struct imx_sc_rpc_msg_max msg; > + u32 *data = (u32 *)&msg; > + int i; > + > + imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0)); > + *data++ = imx_mu_read(priv, priv->dcfg->xRR[0]); please add sanity check. the message size should not be higher then sizeof(msg) > + for (i = 1; i < msg.hdr.size; i++) > + *data++ = imx_mu_read(priv, priv->dcfg->xRR[i % 4]); > + > + imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(0), 0); Please do not forget to handle properly new msg size in your rx_callback. In previous implementation the message size was 4byte. > + mbox_chan_received_data(cp->chan, (void *)&msg); > + > + return 0; > +} > + > static void imx_mu_txdb_tasklet(unsigned long data) > { > struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data; > @@ -274,6 +323,7 @@ static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox, > const struct of_phandle_args *sp) > { > u32 type, idx, chan; > + struct imx_mu_priv *priv = to_imx_mu_priv(mbox); > > if (sp->args_count != 2) { > dev_err(mbox->dev, "Invalid argument count %d\n", sp->args_count); > @@ -284,7 +334,9 @@ static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox, > idx = sp->args[1]; /* index */ > chan = type * 4 + idx; > > - if (chan >= mbox->num_chans) { > + if (chan >= mbox->num_chans || > + (priv->dcfg->type == IMX_MU_TYPE_SCU && > + type < IMX_MU_TYPE_TXDB && idx > 0)) { We need this check since mbox->num_chans do not reflects new reality. Now we have only 2 channels. One RX and one TX. No idea if we need doorbell channels for SCU. If doorbells are not supported, it is better to add a separate imx_mu_xlate for SCU and add SCU specific channel init in probe in addition to: for (i = 0; i < IMX_MU_CHANS; i++) { struct imx_mu_con_priv *cp = &priv->con_priv[i]; cp->idx = i % 4; cp->type = i >> 2; cp->chan = &priv->mbox_chans[i]; priv->mbox_chans[i].con_priv = cp; snprintf(cp->irq_desc, sizeof(cp->irq_desc), "imx_mu_chan[%i-%i]", cp->type, cp->idx); } There is no need to init unsupported channels. Please pack it in separate function > dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx); > return ERR_PTR(-EINVAL); > } > @@ -401,9 +453,20 @@ static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = { > .xCR = 0x64, > }; > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8_scu = { > + .type = IMX_MU_TYPE_SCU, > + .tx = imx_mu_scu_tx, > + .rx = imx_mu_scu_rx, > + .xTR = {0x0, 0x4, 0x8, 0xc}, > + .xRR = {0x10, 0x14, 0x18, 0x1c}, > + .xSR = 0x20, > + .xCR = 0x24, > +}; > + > static const struct of_device_id imx_mu_dt_ids[] = { > { .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp }, > { .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx }, > + { .compatible = "fsl,imx8-mu-scu", .data = &imx_mu_cfg_imx8_scu }, > { }, > }; > MODULE_DEVICE_TABLE(of, imx_mu_dt_ids); > Kind regards, Oleksij Rempel
Hi Oleksij, > Subject: Re: [PATCH V3 3/4] mailbox: imx: add SCU MU support > > Hi, > > On 26.02.20 05:40, peng.fan@nxp.com wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > i.MX8/8X SCU MU is dedicated for communication between SCU and > > Cortex-A cores from hardware design, and could not be reused for other > purpose. > > > > Per i.MX8/8X Reference mannual, Chapter "12.9.2.3.2 Messaging > Examples", > > Passing short messages: Transmit register(s) can be used to pass > > short messages from one to four words in length. For example, when > > a four-word message is desired, only one of the registers needs to > > have its corresponding interrupt enable bit set at the receiver side; > > the message’s first three words are written to the registers whose > > interrupt is masked, and the fourth word is written to the other > > register (which triggers an interrupt at the receiver side). > > > > i.MX8/8X SCU firmware IPC is an implementation of passing short > > messages. But current imx-mailbox driver only support one word > > message, i.MX8/8X linux side firmware has to request four TX and four > > RX to support IPC to SCU firmware. This is low efficent and more > > interrupts triggered compared with one TX and one RX. > > > > To make SCU MU work, > > - parse the size of msg. > > - Only enable TR0/RR0 interrupt for transmit/receive message. > > - For TX/RX, only support one TX channel and one RX channel > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > > > V3: > > Added scu type tx/rx and SCU MU type > > > > drivers/mailbox/imx-mailbox.c | 65 > ++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 64 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mailbox/imx-mailbox.c > > b/drivers/mailbox/imx-mailbox.c index 901a3431fdb5..41664a64c5fd > > 100644 > > --- a/drivers/mailbox/imx-mailbox.c > > +++ b/drivers/mailbox/imx-mailbox.c > > @@ -4,6 +4,7 @@ > > */ > > > > #include <linux/clk.h> > > +#include <linux/firmware/imx/ipc.h> > > #include <linux/interrupt.h> > > #include <linux/io.h> > > #include <linux/kernel.h> > > @@ -38,11 +39,17 @@ enum imx_mu_chan_type { > > > > enum imx_mu_type { > > IMX_MU_TYPE_GENERIC, > > + IMX_MU_TYPE_SCU, > > }; > > > > struct imx_mu_priv; > > struct imx_mu_con_priv; > > > > +struct imx_sc_rpc_msg_max { > > + struct imx_sc_rpc_msg hdr; > > + u32 data[7]; > > +} __packed __aligned(4);; > > + > > struct imx_mu_dcfg { > > enum imx_mu_type type; > > int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, > > void *data); @@ -141,6 +148,48 @@ static int imx_mu_generic_rx(struct > imx_mu_priv *priv, > > return 0; > > } > > > > +static int imx_mu_scu_tx(struct imx_mu_priv *priv, > > + struct imx_mu_con_priv *cp, > > + void *data) > > +{ > > + struct imx_sc_rpc_msg_max *msg = data; > > + u32 *arg = data; > > + int i; > > + > > + switch (cp->type) { > > + case IMX_MU_TYPE_TX: > > please add sanity check if msg->hdr.size can be handled by this driver version. Yes, I forgot to add that. Should as below: if (msg->hdr.size > sizeof(struct imx_sc_rpc_msg_max)) return -EINVAL; > > > + for (i = 0; i < msg->hdr.size; i++) { > > + imx_mu_write(priv, *arg++, > > + priv->dcfg->xTR[i % 4]); > > + } > > + imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0); > > + break; > > + default: > > + dev_warn_ratelimited(priv->dev, "Send data on wrong channel > type: %d\n", cp->type); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static int imx_mu_scu_rx(struct imx_mu_priv *priv, > > + struct imx_mu_con_priv *cp) > > +{ > > + struct imx_sc_rpc_msg_max msg; > > + u32 *data = (u32 *)&msg; > > + int i; > > + > > + imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0)); > > + *data++ = imx_mu_read(priv, priv->dcfg->xRR[0]); > > please add sanity check. the message size should not be higher then > sizeof(msg) Yes. > > > + for (i = 1; i < msg.hdr.size; i++) > > + *data++ = imx_mu_read(priv, priv->dcfg->xRR[i % 4]); > > + > > + imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(0), 0); > > Please do not forget to handle properly new msg size in your rx_callback. In > previous implementation the message size was 4byte. My patch could handle size larger than 4 words. The previous implementation to limit 4 words is not good. > > > + mbox_chan_received_data(cp->chan, (void *)&msg); > > + > > + return 0; > > +} > > + > > static void imx_mu_txdb_tasklet(unsigned long data) > > { > > struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data; @@ > > -274,6 +323,7 @@ static struct mbox_chan * imx_mu_xlate(struct > mbox_controller *mbox, > > const struct of_phandle_args *sp) > > { > > u32 type, idx, chan; > > + struct imx_mu_priv *priv = to_imx_mu_priv(mbox); > > > > if (sp->args_count != 2) { > > dev_err(mbox->dev, "Invalid argument count %d\n", > sp->args_count); > > @@ -284,7 +334,9 @@ static struct mbox_chan * imx_mu_xlate(struct > mbox_controller *mbox, > > idx = sp->args[1]; /* index */ > > chan = type * 4 + idx; > > > > - if (chan >= mbox->num_chans) { > > + if (chan >= mbox->num_chans || > > + (priv->dcfg->type == IMX_MU_TYPE_SCU && > > + type < IMX_MU_TYPE_TXDB && idx > 0)) { > > We need this check since mbox->num_chans do not reflects new reality. Now > we have only 2 channels. One RX and one TX. No idea if we need doorbell > channels for SCU. If doorbells are not supported, it is better to add a separate > imx_mu_xlate for SCU TXDB currently not supported, but I am not sure whether it will be supported in future. I could add a separate xlate as you suggested, since TXDB not support now. > > and add SCU specific channel init in probe in addition to: > for (i = 0; i < IMX_MU_CHANS; i++) { > > struct imx_mu_con_priv *cp = &priv->con_priv[i]; > > > > cp->idx = i % 4; > > cp->type = i >> 2; > > cp->chan = &priv->mbox_chans[i]; > > priv->mbox_chans[i].con_priv = cp; > > snprintf(cp->irq_desc, sizeof(cp->irq_desc), > > "imx_mu_chan[%i-%i]", cp->type, cp->idx); > > } > > There is no need to init unsupported channels. Please pack it in separate > function ok. I just want to reuse current code as more as possible and avoid adding too much new code. Thanks, Peng. > > > dev_err(mbox->dev, "Not supported channel number: %d. > (type: %d, idx: %d)\n", chan, type, idx); > > return ERR_PTR(-EINVAL); > > } > > @@ -401,9 +453,20 @@ static const struct imx_mu_dcfg > imx_mu_cfg_imx7ulp = { > > .xCR = 0x64, > > }; > > > > +static const struct imx_mu_dcfg imx_mu_cfg_imx8_scu = { > > + .type = IMX_MU_TYPE_SCU, > > + .tx = imx_mu_scu_tx, > > + .rx = imx_mu_scu_rx, > > + .xTR = {0x0, 0x4, 0x8, 0xc}, > > + .xRR = {0x10, 0x14, 0x18, 0x1c}, > > + .xSR = 0x20, > > + .xCR = 0x24, > > +}; > > + > > static const struct of_device_id imx_mu_dt_ids[] = { > > { .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp }, > > { .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx }, > > + { .compatible = "fsl,imx8-mu-scu", .data = &imx_mu_cfg_imx8_scu }, > > { }, > > }; > > MODULE_DEVICE_TABLE(of, imx_mu_dt_ids); > > > > Kind regards, > Oleksij Rempel > > -- > Pengutronix e.K. | > | > Industrial Linux Solutions | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p > engutronix.de%2F&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 |
diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c index 901a3431fdb5..41664a64c5fd 100644 --- a/drivers/mailbox/imx-mailbox.c +++ b/drivers/mailbox/imx-mailbox.c @@ -4,6 +4,7 @@ */ #include <linux/clk.h> +#include <linux/firmware/imx/ipc.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/kernel.h> @@ -38,11 +39,17 @@ enum imx_mu_chan_type { enum imx_mu_type { IMX_MU_TYPE_GENERIC, + IMX_MU_TYPE_SCU, }; struct imx_mu_priv; struct imx_mu_con_priv; +struct imx_sc_rpc_msg_max { + struct imx_sc_rpc_msg hdr; + u32 data[7]; +} __packed __aligned(4);; + struct imx_mu_dcfg { enum imx_mu_type type; int (*tx)(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp, void *data); @@ -141,6 +148,48 @@ static int imx_mu_generic_rx(struct imx_mu_priv *priv, return 0; } +static int imx_mu_scu_tx(struct imx_mu_priv *priv, + struct imx_mu_con_priv *cp, + void *data) +{ + struct imx_sc_rpc_msg_max *msg = data; + u32 *arg = data; + int i; + + switch (cp->type) { + case IMX_MU_TYPE_TX: + for (i = 0; i < msg->hdr.size; i++) { + imx_mu_write(priv, *arg++, + priv->dcfg->xTR[i % 4]); + } + imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0); + break; + default: + dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type); + return -EINVAL; + } + + return 0; +} + +static int imx_mu_scu_rx(struct imx_mu_priv *priv, + struct imx_mu_con_priv *cp) +{ + struct imx_sc_rpc_msg_max msg; + u32 *data = (u32 *)&msg; + int i; + + imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_RIEn(0)); + *data++ = imx_mu_read(priv, priv->dcfg->xRR[0]); + for (i = 1; i < msg.hdr.size; i++) + *data++ = imx_mu_read(priv, priv->dcfg->xRR[i % 4]); + + imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(0), 0); + mbox_chan_received_data(cp->chan, (void *)&msg); + + return 0; +} + static void imx_mu_txdb_tasklet(unsigned long data) { struct imx_mu_con_priv *cp = (struct imx_mu_con_priv *)data; @@ -274,6 +323,7 @@ static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox, const struct of_phandle_args *sp) { u32 type, idx, chan; + struct imx_mu_priv *priv = to_imx_mu_priv(mbox); if (sp->args_count != 2) { dev_err(mbox->dev, "Invalid argument count %d\n", sp->args_count); @@ -284,7 +334,9 @@ static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox, idx = sp->args[1]; /* index */ chan = type * 4 + idx; - if (chan >= mbox->num_chans) { + if (chan >= mbox->num_chans || + (priv->dcfg->type == IMX_MU_TYPE_SCU && + type < IMX_MU_TYPE_TXDB && idx > 0)) { dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx); return ERR_PTR(-EINVAL); } @@ -401,9 +453,20 @@ static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = { .xCR = 0x64, }; +static const struct imx_mu_dcfg imx_mu_cfg_imx8_scu = { + .type = IMX_MU_TYPE_SCU, + .tx = imx_mu_scu_tx, + .rx = imx_mu_scu_rx, + .xTR = {0x0, 0x4, 0x8, 0xc}, + .xRR = {0x10, 0x14, 0x18, 0x1c}, + .xSR = 0x20, + .xCR = 0x24, +}; + static const struct of_device_id imx_mu_dt_ids[] = { { .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_imx7ulp }, { .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_imx6sx }, + { .compatible = "fsl,imx8-mu-scu", .data = &imx_mu_cfg_imx8_scu }, { }, }; MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);