[PATCH v1 0/2] spi: dw-mid: fix simplex DMA transfers

[PATCH v1 0/2] spi: dw-mid: fix simplex DMA transfers

[Andy Shevchenko]

In current shape the driver relies on both TX and RX transfers when DMA is
enabled. In case of simplex transfer the driver will stuck, since call back
function will never achieve the proper condition to finish the transfer.

This patchset fixes the potential issue.

Andy Shevchenko (2):
  spi: dw-mid: refactor to use helpers
  spi: dw-mid: split rx and tx callbacks when DMA

 drivers/spi/spi-dw-mid.c | 114 ++++++++++++++++++++++++++++++++++-------------
 drivers/spi/spi-dw.h     |   2 +-
 2 files changed, 84 insertions(+), 32 deletions(-)

-- 
2.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 1/2] spi: dw-mid: refactor to use helpers

[Andy Shevchenko]

This patch splits few helpers, namely dw_spi_dma_prepare_rx(),
dw_spi_dma_prepare_tx(), and dw_spi_dma_setup() which will be useful for the
consequent improvements.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/spi/spi-dw-mid.c | 69 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
index 46c6d58..c8319ab 100644
--- a/drivers/spi/spi-dw-mid.c
+++ b/drivers/spi/spi-dw-mid.c
@@ -111,28 +111,11 @@ static void dw_spi_dma_done(void *arg)
 	dw_spi_xfer_done(dws);
 }
 
-static int mid_spi_dma_transfer(struct dw_spi *dws, int cs_change)
+static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws)
 {
-	struct dma_async_tx_descriptor *txdesc, *rxdesc;
-	struct dma_slave_config txconf, rxconf;
-	u16 dma_ctrl = 0;
-
-	/* 1. setup DMA related registers */
-	if (cs_change) {
-		spi_enable_chip(dws, 0);
-		dw_writew(dws, DW_SPI_DMARDLR, 0xf);
-		dw_writew(dws, DW_SPI_DMATDLR, 0x10);
-		if (dws->tx_dma)
-			dma_ctrl |= SPI_DMA_TDMAE;
-		if (dws->rx_dma)
-			dma_ctrl |= SPI_DMA_RDMAE;
-		dw_writew(dws, DW_SPI_DMACR, dma_ctrl);
-		spi_enable_chip(dws, 1);
-	}
+	struct dma_slave_config txconf;
+	struct dma_async_tx_descriptor *txdesc;
 
-	dws->dma_chan_done = 0;
-
-	/* 2. Prepare the TX dma transfer */
 	txconf.direction = DMA_MEM_TO_DEV;
 	txconf.dst_addr = dws->dma_addr;
 	txconf.dst_maxburst = LNW_DMA_MSIZE_16;
@@ -154,7 +137,14 @@ static int mid_spi_dma_transfer(struct dw_spi *dws, int cs_change)
 	txdesc->callback = dw_spi_dma_done;
 	txdesc->callback_param = dws;
 
-	/* 3. Prepare the RX dma transfer */
+	return txdesc;
+}
+
+static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws)
+{
+	struct dma_slave_config rxconf;
+	struct dma_async_tx_descriptor *rxdesc;
+
 	rxconf.direction = DMA_DEV_TO_MEM;
 	rxconf.src_addr = dws->dma_addr;
 	rxconf.src_maxburst = LNW_DMA_MSIZE_16;
@@ -176,6 +166,43 @@ static int mid_spi_dma_transfer(struct dw_spi *dws, int cs_change)
 	rxdesc->callback = dw_spi_dma_done;
 	rxdesc->callback_param = dws;
 
+	return rxdesc;
+}
+
+static void dw_spi_dma_setup(struct dw_spi *dws)
+{
+	u16 dma_ctrl = 0;
+
+	spi_enable_chip(dws, 0);
+
+	dw_writew(dws, DW_SPI_DMARDLR, 0xf);
+	dw_writew(dws, DW_SPI_DMATDLR, 0x10);
+
+	if (dws->tx_dma)
+		dma_ctrl |= SPI_DMA_TDMAE;
+	if (dws->rx_dma)
+		dma_ctrl |= SPI_DMA_RDMAE;
+	dw_writew(dws, DW_SPI_DMACR, dma_ctrl);
+
+	spi_enable_chip(dws, 1);
+}
+
+static int mid_spi_dma_transfer(struct dw_spi *dws, int cs_change)
+{
+	struct dma_async_tx_descriptor *txdesc, *rxdesc;
+
+	/* 1. setup DMA related registers */
+	if (cs_change)
+		dw_spi_dma_setup(dws);
+
+	dws->dma_chan_done = 0;
+
+	/* 2. Prepare the TX dma transfer */
+	txdesc = dw_spi_dma_prepare_tx(dws);
+
+	/* 3. Prepare the RX dma transfer */
+	rxdesc = dw_spi_dma_prepare_rx(dws);
+
 	/* rx must be started before tx due to spi instinct */
 	dmaengine_submit(rxdesc);
 	dma_async_issue_pending(dws->rxchan);
-- 
2.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/2] spi: dw-mid: split rx and tx callbacks when DMA

[Andy Shevchenko]

Currently driver wouldn't work properly if user asked for simplex transfer. The
patch separates DMA rx and tx callbacks and finishes transfer correctly in any
case.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/spi/spi-dw-mid.c | 53 +++++++++++++++++++++++++++++++++++-------------
 drivers/spi/spi-dw.h     |  2 +-
 2 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
index c8319ab..7281316 100644
--- a/drivers/spi/spi-dw-mid.c
+++ b/drivers/spi/spi-dw-mid.c
@@ -26,6 +26,9 @@
 #include <linux/intel_mid_dma.h>
 #include <linux/pci.h>
 
+#define RX_BUSY		0
+#define TX_BUSY		1
+
 struct mid_dma {
 	struct intel_mid_dma_slave	dmas_tx;
 	struct intel_mid_dma_slave	dmas_rx;
@@ -98,15 +101,14 @@ static void mid_spi_dma_exit(struct dw_spi *dws)
 }
 
 /*
- * dws->dma_chan_done is cleared before the dma transfer starts,
- * callback for rx/tx channel will each increment it by 1.
- * Reaching 2 means the whole spi transaction is done.
+ * dws->dma_chan_busy is set before the dma transfer starts, callback for tx
+ * channel will clear a corresponding bit.
  */
-static void dw_spi_dma_done(void *arg)
+static void dw_spi_dma_tx_done(void *arg)
 {
 	struct dw_spi *dws = arg;
 
-	if (++dws->dma_chan_done != 2)
+	if (test_and_clear_bit(TX_BUSY, &dws->dma_chan_busy) & BIT(RX_BUSY))
 		return;
 	dw_spi_xfer_done(dws);
 }
@@ -116,6 +118,9 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws)
 	struct dma_slave_config txconf;
 	struct dma_async_tx_descriptor *txdesc;
 
+	if (!dws->tx_dma)
+		return NULL;
+
 	txconf.direction = DMA_MEM_TO_DEV;
 	txconf.dst_addr = dws->dma_addr;
 	txconf.dst_maxburst = LNW_DMA_MSIZE_16;
@@ -134,17 +139,33 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws)
 				1,
 				DMA_MEM_TO_DEV,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
-	txdesc->callback = dw_spi_dma_done;
+	txdesc->callback = dw_spi_dma_tx_done;
 	txdesc->callback_param = dws;
 
 	return txdesc;
 }
 
+/*
+ * dws->dma_chan_busy is set before the dma transfer starts, callback for rx
+ * channel will clear a corresponding bit.
+ */
+static void dw_spi_dma_rx_done(void *arg)
+{
+	struct dw_spi *dws = arg;
+
+	if (test_and_clear_bit(RX_BUSY, &dws->dma_chan_busy) & BIT(TX_BUSY))
+		return;
+	dw_spi_xfer_done(dws);
+}
+
 static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws)
 {
 	struct dma_slave_config rxconf;
 	struct dma_async_tx_descriptor *rxdesc;
 
+	if (!dws->rx_dma)
+		return NULL;
+
 	rxconf.direction = DMA_DEV_TO_MEM;
 	rxconf.src_addr = dws->dma_addr;
 	rxconf.src_maxburst = LNW_DMA_MSIZE_16;
@@ -163,7 +184,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws)
 				1,
 				DMA_DEV_TO_MEM,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
-	rxdesc->callback = dw_spi_dma_done;
+	rxdesc->callback = dw_spi_dma_rx_done;
 	rxdesc->callback_param = dws;
 
 	return rxdesc;
@@ -195,8 +216,6 @@ static int mid_spi_dma_transfer(struct dw_spi *dws, int cs_change)
 	if (cs_change)
 		dw_spi_dma_setup(dws);
 
-	dws->dma_chan_done = 0;
-
 	/* 2. Prepare the TX dma transfer */
 	txdesc = dw_spi_dma_prepare_tx(dws);
 
@@ -204,11 +223,17 @@ static int mid_spi_dma_transfer(struct dw_spi *dws, int cs_change)
 	rxdesc = dw_spi_dma_prepare_rx(dws);
 
 	/* rx must be started before tx due to spi instinct */
-	dmaengine_submit(rxdesc);
-	dma_async_issue_pending(dws->rxchan);
-
-	dmaengine_submit(txdesc);
-	dma_async_issue_pending(dws->txchan);
+	if (rxdesc) {
+		set_bit(RX_BUSY, &dws->dma_chan_busy);
+		dmaengine_submit(rxdesc);
+		dma_async_issue_pending(dws->rxchan);
+	}
+
+	if (txdesc) {
+		set_bit(TX_BUSY, &dws->dma_chan_busy);
+		dmaengine_submit(txdesc);
+		dma_async_issue_pending(dws->txchan);
+	}
 
 	return 0;
 }
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 83a103a..3d32be6 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -139,7 +139,7 @@ struct dw_spi {
 	struct scatterlist	tx_sgl;
 	struct dma_chan		*rxchan;
 	struct scatterlist	rx_sgl;
-	int			dma_chan_done;
+	unsigned long		dma_chan_busy;
 	struct device		*dma_dev;
 	dma_addr_t		dma_addr; /* phy address of the Data register */
 	struct dw_spi_dma_ops	*dma_ops;
-- 
2.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 0/2] spi: dw-mid: fix simplex DMA transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On Tue, Oct 28, 2014 at 06:25:00PM +0200, Andy Shevchenko wrote:
> In current shape the driver relies on both TX and RX transfers when DMA is
> enabled. In case of simplex transfer the driver will stuck, since call back
> function will never achieve the proper condition to finish the transfer.

Applied both, thanks.  Can you please also send a patch for Linus
setting SPI_MASTER_MUST_RX and SPI_MASTER_MUST_TX - this is a bug which
should be fixed in mainline but these seem a bit big for merge right
now, those flags will cause the core to always ensure that both
directions have data avoiding the issue?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 0/2] spi: dw-mid: fix simplex DMA transfers

[Andy Shevchenko]

On Tue, 2014-10-28 at 22:41 +0000, Mark Brown wrote:
> On Tue, Oct 28, 2014 at 06:25:00PM +0200, Andy Shevchenko wrote:
> > In current shape the driver relies on both TX and RX transfers when DMA is
> > enabled. In case of simplex transfer the driver will stuck, since call back
> > function will never achieve the proper condition to finish the transfer.
> 
> Applied both, thanks.

Thanks!

>   Can you please also send a patch for Linus
> setting SPI_MASTER_MUST_RX and SPI_MASTER_MUST_TX - this is a bug which
> should be fixed in mainline but these seem a bit big for merge right
> now, those flags will cause the core to always ensure that both
> directions have data avoiding the issue?

I'm sorry I didn't quite understand that bug.

When those bits are set the core will always provide valid tx_buf and
rx_buf pointers. How would it help us? Currently driver relies on tx_dma
and rx_dma which are provided by clients if I understood correctly.

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 0/2] spi: dw-mid: fix simplex DMA transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

On Wed, Oct 29, 2014 at 12:37:56PM +0200, Andy Shevchenko wrote:
> On Tue, 2014-10-28 at 22:41 +0000, Mark Brown wrote:

> >   Can you please also send a patch for Linus
> > setting SPI_MASTER_MUST_RX and SPI_MASTER_MUST_TX - this is a bug which
> > should be fixed in mainline but these seem a bit big for merge right
> > now, those flags will cause the core to always ensure that both
> > directions have data avoiding the issue?

> I'm sorry I didn't quite understand that bug.

> When those bits are set the core will always provide valid tx_buf and
> rx_buf pointers. How would it help us? Currently driver relies on tx_dma
> and rx_dma which are provided by clients if I understood correctly.

Uh, are you sure - how exactly has this been tested?  tx_dma and rx_dma
*can* be provided by users but vanishingly few actually do so.  Drivers
must not rely on the caller doing this, the feature will be going away
apart from anything else.  If the driver is relying on this it probably
needs to have the DMA support that's there removed as a fix and should
be converted to use the core DMA mapping functionality in -next.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 0/2] spi: dw-mid: fix simplex DMA transfers

[Andy Shevchenko]

On Wed, 2014-10-29 at 11:17 +0000, Mark Brown wrote:
> On Wed, Oct 29, 2014 at 12:37:56PM +0200, Andy Shevchenko wrote:
> > On Tue, 2014-10-28 at 22:41 +0000, Mark Brown wrote:
> 
> > >   Can you please also send a patch for Linus
> > > setting SPI_MASTER_MUST_RX and SPI_MASTER_MUST_TX - this is a bug which
> > > should be fixed in mainline but these seem a bit big for merge right
> > > now, those flags will cause the core to always ensure that both
> > > directions have data avoiding the issue?
> 
> > I'm sorry I didn't quite understand that bug.
> 
> > When those bits are set the core will always provide valid tx_buf and
> > rx_buf pointers. How would it help us? Currently driver relies on tx_dma
> > and rx_dma which are provided by clients if I understood correctly.
> 
> Uh, are you sure

There is a function in spi-dw.c called map_dma_buffers() which uses
tx_dma and rx_dma pointers. I don't see where they are initialized.

>  - how exactly has this been tested?  

I don't know how it was tested before, but, besides intel-mid-dma is
somehow broken (I won't spend time to patch it), I'm using patch I
already published which converts driver to use dw_dmac driver. With it I
run tests on a loop back.

> tx_dma and rx_dma
> *can* be provided by users but vanishingly few actually do so.  Drivers
> must not rely on the caller doing this, the feature will be going away
> apart from anything else.  If the driver is relying on this it probably
> needs to have the DMA support that's there removed as a fix and should
> be converted to use the core DMA mapping functionality in -next.

So, regarding to above what would you like me to do as next step?

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 0/2] spi: dw-mid: fix simplex DMA transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

On Wed, Oct 29, 2014 at 03:20:48PM +0200, Andy Shevchenko wrote:
> On Wed, 2014-10-29 at 11:17 +0000, Mark Brown wrote:

> >  - how exactly has this been tested?  

> I don't know how it was tested before, but, besides intel-mid-dma is
> somehow broken (I won't spend time to patch it), I'm using patch I
> already published which converts driver to use dw_dmac driver. With it I
> run tests on a loop back.

How exactly do you run those tests though, what is the client driver?

> > tx_dma and rx_dma
> > *can* be provided by users but vanishingly few actually do so.  Drivers
> > must not rely on the caller doing this, the feature will be going away
> > apart from anything else.  If the driver is relying on this it probably
> > needs to have the DMA support that's there removed as a fix and should
> > be converted to use the core DMA mapping functionality in -next.

> So, regarding to above what would you like me to do as next step?

Looking at map_dma_buffers() it's probably actually safe since it checks
that the message has is_dma_mapped set which virtually no clients will
do.  However this means that we're not bothering to DMA almost all the
time which is silly - I'd like to see the driver converted to provide a
can_dma() operation and let the core worry about mapping, that will mean
a substantial performance win for most clients.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 0/2] spi: dw-mid: fix simplex DMA transfers

[Andy Shevchenko]

On Wed, 2014-10-29 at 22:15 +0000, Mark Brown wrote:
> On Wed, Oct 29, 2014 at 03:20:48PM +0200, Andy Shevchenko wrote:
> > On Wed, 2014-10-29 at 11:17 +0000, Mark Brown wrote:
> 
> > >  - how exactly has this been tested?  
> 
> > I don't know how it was tested before, but, besides intel-mid-dma is
> > somehow broken (I won't spend time to patch it), I'm using patch I
> > already published which converts driver to use dw_dmac driver. With it I
> > run tests on a loop back.
> 
> How exactly do you run those tests though, what is the client driver?

I have a hack patch to register the spidev and I run internal test suite
written by Mika Westerberg. It uses a loop back mode to send and check
data.

> 
> > > tx_dma and rx_dma
> > > *can* be provided by users but vanishingly few actually do so.  Drivers
> > > must not rely on the caller doing this, the feature will be going away
> > > apart from anything else.  If the driver is relying on this it probably
> > > needs to have the DMA support that's there removed as a fix and should
> > > be converted to use the core DMA mapping functionality in -next.
> 
> > So, regarding to above what would you like me to do as next step?
> 
> Looking at map_dma_buffers() it's probably actually safe since it checks
> that the message has is_dma_mapped set which virtually no clients will
> do.  However this means that we're not bothering to DMA almost all the
> time which is silly - I'd like to see the driver converted to provide a
> can_dma() operation and let the core worry about mapping, that will mean
> a substantial performance win for most clients.

That is exactly my intention with the additional thing — to use dw_dmac
instead of intel-mid-dma.

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html