RE: [PATCH] firmware: imx: scu: Ensure sequential TX

From: Peng Fan <peng.fan@nxp.com>
To: Leonard Crestez <leonard.crestez@nxp.com>,
	Shawn Guo <shawnguo@kernel.org>
Cc: Aisheng Dong <aisheng.dong@nxp.com>,
	Richard Zhu <hongxing.zhu@nxp.com>,
	Jassi Brar <jassisinghbrar@gmail.com>,
	Oleksij Rempel <o.rempel@pengutronix.de>,
	dl-linux-imx <linux-imx@nxp.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	Fabio Estevam <fabio.estevam@nxp.com>,
	Daniel Baluta <daniel.baluta@nxp.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH] firmware: imx: scu: Ensure sequential TX
Date: Fri, 21 Feb 2020 01:26:40 +0000	[thread overview]
Message-ID: <AM0PR04MB4481B07AB544FF95729429C888120@AM0PR04MB4481.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <ae051784024f8fcc458437e278c27b4e79c6fe7d.1582214881.git.leonard.crestez@nxp.com>

> Subject: [PATCH] firmware: imx: scu: Ensure sequential TX
> 
> SCU requires that all messages words are written sequentially but linux MU
> driver implements multiple independent channels for each register so
> ordering between different channels must be ensured by SCU API interface.
> 
> Wait for tx_done before every send to ensure that no queueing happens at
> the mailbox channel level.
> 
> Fixes: edbee095fafb ("firmware: imx: add SCU firmware driver support")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: <stable@vger.kernel.org>

I am working on binding 4 registers in one channel per MU chapter using
example. But since this is a critical fix,

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> ---
>  drivers/firmware/imx/imx-scu.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> This manifests as "SCU timeout" message followed by system hang.
> 
> This is not a very pretty fix but avoids inserting additional waits except in
> extremely rare circumstances.
> 
> An alternative would be to implement a new type of mailbox channel which
> handles all 4 registers together. Exposing the MU as 4 independent channels
> is very awkward.
> 
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
> index 03b43b7a6d1d..f71eaa5bf52d 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -27,10 +27,11 @@ struct imx_sc_chan {
>  	struct imx_sc_ipc *sc_ipc;
> 
>  	struct mbox_client cl;
>  	struct mbox_chan *ch;
>  	int idx;
> +	struct completion tx_done;
>  };
> 
>  struct imx_sc_ipc {
>  	/* SCU uses 4 Tx and 4 Rx channels */
>  	struct imx_sc_chan chans[SCU_MU_CHAN_NUM]; @@ -98,10 +99,18
> @@ int imx_scu_get_handle(struct imx_sc_ipc **ipc)
>  	*ipc = imx_sc_ipc_handle;
>  	return 0;
>  }
>  EXPORT_SYMBOL(imx_scu_get_handle);
> 
> +/* Callback called when the word of a message is ack-ed, eg read by SCU
> +*/ static void imx_scu_tx_done(struct mbox_client *cl, void *mssg, int
> +r) {
> +	struct imx_sc_chan *sc_chan = container_of(cl, struct imx_sc_chan,
> +cl);
> +
> +	complete(&sc_chan->tx_done);
> +}
> +
>  static void imx_scu_rx_callback(struct mbox_client *c, void *msg)  {
>  	struct imx_sc_chan *sc_chan = container_of(c, struct imx_sc_chan, cl);
>  	struct imx_sc_ipc *sc_ipc = sc_chan->sc_ipc;
>  	struct imx_sc_rpc_msg *hdr;
> @@ -147,10 +156,23 @@ 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++) {
>  		sc_chan = &sc_ipc->chans[i % 4];
> +
> +		/*
> +		 * SCU requires that all messages words are written
> +		 * sequentially but linux MU driver implements multiple
> +		 * independent channels for each register so ordering between
> +		 * different channels must be ensured by SCU API interface.
> +		 *
> +		 * 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);
> +
>  		ret = mbox_send_message(sc_chan->ch, &data[i]);
>  		if (ret < 0)
>  			return ret;
>  	}
> 
> @@ -245,10 +267,15 @@ static int imx_scu_probe(struct platform_device
> *pdev)
>  		cl->dev = dev;
>  		cl->tx_block = false;
>  		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);
> +
>  		sc_chan->sc_ipc = sc_ipc;
>  		sc_chan->idx = i % 4;
>  		sc_chan->ch = mbox_request_channel_byname(cl, chan_name);
>  		if (IS_ERR(sc_chan->ch)) {
>  			ret = PTR_ERR(sc_chan->ch);
> --
> 2.17.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel