[CFT 07/11] spi: omap2-mcspi: add DMA engine support

[CFT 07/11] spi: omap2-mcspi: add DMA engine support

[Russell King]

Add DMA engine support to the OMAP SPI driver.  This supplements the
private DMA API implementation contained within this driver, and the
driver can be independently switched at build time between using DMA
engine and the private DMA API for the transmit and receive sides.

Tested-by: Shubhrajyoti <shubhrajyoti@ti.com>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/spi/spi-omap2-mcspi.c |  183 +++++++++++++++++++++++++++++++++-------
 1 files changed, 151 insertions(+), 32 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 46ef5fe..ca016df 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -20,6 +20,8 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
  *
  */
+#define USE_DMA_ENGINE_RX
+#define USE_DMA_ENGINE_TX
 
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -28,6 +30,8 @@
 #include <linux/device.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/omap-dma.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>
 #include <linux/clk.h>
@@ -93,6 +97,8 @@
 
 /* We have 2 DMA channels per CS, one for RX and one for TX */
 struct omap2_mcspi_dma {
+	struct dma_chan *dma_tx;
+	struct dma_chan *dma_rx;
 	int dma_tx_channel;
 	int dma_rx_channel;
 
@@ -300,6 +306,30 @@ static int mcspi_wait_for_reg_bit(void __iomem *reg, unsigned long bit)
 	return 0;
 }
 
+static void omap2_mcspi_rx_callback(void *data)
+{
+	struct spi_device *spi = data;
+	struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master);
+	struct omap2_mcspi_dma *mcspi_dma = &mcspi->dma_channels[spi->chip_select];
+
+	complete(&mcspi_dma->dma_rx_completion);
+
+	/* We must disable the DMA RX request */
+	omap2_mcspi_set_dma_req(spi, 1, 0);
+}
+
+static void omap2_mcspi_tx_callback(void *data)
+{
+	struct spi_device *spi = data;
+	struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master);
+	struct omap2_mcspi_dma *mcspi_dma = &mcspi->dma_channels[spi->chip_select];
+
+	complete(&mcspi_dma->dma_tx_completion);
+
+	/* We must disable the DMA TX request */
+	omap2_mcspi_set_dma_req(spi, 0, 0);
+}
+
 static unsigned
 omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 {
@@ -314,6 +344,9 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 	u8			* rx;
 	const u8		* tx;
 	void __iomem		*chstat_reg;
+	struct dma_slave_config	cfg;
+	enum dma_slave_buswidth width;
+	unsigned es;
 
 	mcspi = spi_master_get_devdata(spi->master);
 	mcspi_dma = &mcspi->dma_channels[spi->chip_select];
@@ -321,6 +354,71 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 
 	chstat_reg = cs->base + OMAP2_MCSPI_CHSTAT0;
 
+	if (cs->word_len <= 8) {
+		width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+		es = 1;
+	} else if (cs->word_len <= 16) {
+		width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+		es = 2;
+	} else {
+		width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		es = 4;
+	}
+
+	memset(&cfg, 0, sizeof(cfg));
+	cfg.src_addr = cs->phys + OMAP2_MCSPI_RX0;
+	cfg.dst_addr = cs->phys + OMAP2_MCSPI_TX0;
+	cfg.src_addr_width = width;
+	cfg.dst_addr_width = width;
+	cfg.src_maxburst = 1;
+	cfg.dst_maxburst = 1;
+
+	if (xfer->tx_buf && mcspi_dma->dma_tx) {
+		struct dma_async_tx_descriptor *tx;
+		struct scatterlist sg;
+
+		dmaengine_slave_config(mcspi_dma->dma_tx, &cfg);
+
+		sg_init_table(&sg, 1);
+		sg_dma_address(&sg) = xfer->tx_dma;
+		sg_dma_len(&sg) = xfer->len;
+
+		tx = dmaengine_prep_slave_sg(mcspi_dma->dma_tx, &sg, 1,
+			DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (tx) {
+			tx->callback = omap2_mcspi_tx_callback;
+			tx->callback_param = spi;
+			dmaengine_submit(tx);
+		} else {
+			/* FIXME: fall back to PIO? */
+		}
+	}
+
+	if (xfer->rx_buf && mcspi_dma->dma_rx) {
+		struct dma_async_tx_descriptor *tx;
+		struct scatterlist sg;
+		size_t len = xfer->len - es;
+
+		dmaengine_slave_config(mcspi_dma->dma_rx, &cfg);
+
+		if (l & OMAP2_MCSPI_CHCONF_TURBO)
+			len -= es;
+
+		sg_init_table(&sg, 1);
+		sg_dma_address(&sg) = xfer->rx_dma;
+		sg_dma_len(&sg) = len;
+
+		tx = dmaengine_prep_slave_sg(mcspi_dma->dma_rx, &sg, 1,
+			DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (tx) {
+			tx->callback = omap2_mcspi_rx_callback;
+			tx->callback_param = spi;
+			dmaengine_submit(tx);
+		} else {
+			/* FIXME: fall back to PIO? */
+		}
+	}
+
 	count = xfer->len;
 	c = count;
 	word_len = cs->word_len;
@@ -342,7 +440,7 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 		element_count = count >> 2;
 	}
 
-	if (tx != NULL) {
+	if (tx != NULL && mcspi_dma->dma_tx_channel != -1) {
 		omap_set_dma_transfer_params(mcspi_dma->dma_tx_channel,
 				data_type, element_count, 1,
 				OMAP_DMA_SYNC_ELEMENT,
@@ -357,7 +455,7 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 				xfer->tx_dma, 0, 0);
 	}
 
-	if (rx != NULL) {
+	if (rx != NULL && mcspi_dma->dma_rx_channel != -1) {
 		elements = element_count - 1;
 		if (l & OMAP2_MCSPI_CHCONF_TURBO)
 			elements--;
@@ -377,12 +475,18 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 	}
 
 	if (tx != NULL) {
-		omap_start_dma(mcspi_dma->dma_tx_channel);
+		if (mcspi_dma->dma_tx)
+			dma_async_issue_pending(mcspi_dma->dma_tx);
+		else
+			omap_start_dma(mcspi_dma->dma_tx_channel);
 		omap2_mcspi_set_dma_req(spi, 0, 1);
 	}
 
 	if (rx != NULL) {
-		omap_start_dma(mcspi_dma->dma_rx_channel);
+		if (mcspi_dma->dma_rx)
+			dma_async_issue_pending(mcspi_dma->dma_rx);
+		else
+			omap_start_dma(mcspi_dma->dma_rx_channel);
 		omap2_mcspi_set_dma_req(spi, 1, 1);
 	}
 
@@ -406,7 +510,10 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 		dma_unmap_single(&spi->dev, xfer->rx_dma, count, DMA_FROM_DEVICE);
 		omap2_mcspi_set_enable(spi, 0);
 
+		elements = element_count - 1;
+
 		if (l & OMAP2_MCSPI_CHCONF_TURBO) {
+			elements--;
 
 			if (likely(mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHSTAT0)
 				   & OMAP2_MCSPI_CHSTAT_RXS)) {
@@ -725,32 +832,12 @@ static int omap2_mcspi_setup_transfer(struct spi_device *spi,
 
 static void omap2_mcspi_dma_rx_callback(int lch, u16 ch_status, void *data)
 {
-	struct spi_device	*spi = data;
-	struct omap2_mcspi	*mcspi;
-	struct omap2_mcspi_dma	*mcspi_dma;
-
-	mcspi = spi_master_get_devdata(spi->master);
-	mcspi_dma = &(mcspi->dma_channels[spi->chip_select]);
-
-	complete(&mcspi_dma->dma_rx_completion);
-
-	/* We must disable the DMA RX request */
-	omap2_mcspi_set_dma_req(spi, 1, 0);
+	omap2_mcspi_rx_callback(data);
 }
 
 static void omap2_mcspi_dma_tx_callback(int lch, u16 ch_status, void *data)
 {
-	struct spi_device	*spi = data;
-	struct omap2_mcspi	*mcspi;
-	struct omap2_mcspi_dma	*mcspi_dma;
-
-	mcspi = spi_master_get_devdata(spi->master);
-	mcspi_dma = &(mcspi->dma_channels[spi->chip_select]);
-
-	complete(&mcspi_dma->dma_tx_completion);
-
-	/* We must disable the DMA TX request */
-	omap2_mcspi_set_dma_req(spi, 0, 0);
+	omap2_mcspi_tx_callback(data);
 }
 
 static int omap2_mcspi_request_dma(struct spi_device *spi)
@@ -758,17 +845,43 @@ static int omap2_mcspi_request_dma(struct spi_device *spi)
 	struct spi_master	*master = spi->master;
 	struct omap2_mcspi	*mcspi;
 	struct omap2_mcspi_dma	*mcspi_dma;
+	dma_cap_mask_t mask;
+	unsigned sig;
 
 	mcspi = spi_master_get_devdata(master);
 	mcspi_dma = mcspi->dma_channels + spi->chip_select;
 
+	init_completion(&mcspi_dma->dma_rx_completion);
+	init_completion(&mcspi_dma->dma_tx_completion);
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+#ifdef USE_DMA_ENGINE_RX
+	sig = mcspi_dma->dma_rx_sync_dev;
+	mcspi_dma->dma_rx = dma_request_channel(mask, omap_dma_filter_fn, &sig);
+	if (!mcspi_dma->dma_rx) {
+		dev_err(&spi->dev, "no RX DMA engine channel for McSPI\n");
+		return -EAGAIN;
+	}
+#else
 	if (omap_request_dma(mcspi_dma->dma_rx_sync_dev, "McSPI RX",
 			omap2_mcspi_dma_rx_callback, spi,
 			&mcspi_dma->dma_rx_channel)) {
 		dev_err(&spi->dev, "no RX DMA channel for McSPI\n");
 		return -EAGAIN;
 	}
+#endif
 
+#ifdef USE_DMA_ENGINE_TX
+	sig = mcspi_dma->dma_tx_sync_dev;
+	mcspi_dma->dma_tx = dma_request_channel(mask, omap_dma_filter_fn, &sig);
+	if (!mcspi_dma->dma_tx) {
+		dev_err(&spi->dev, "no TX DMA engine channel for McSPI\n");
+		dma_release_channel(mcspi_dma->dma_rx);
+		mcspi_dma->dma_rx = NULL;
+		return -EAGAIN;
+	}
+#else
 	if (omap_request_dma(mcspi_dma->dma_tx_sync_dev, "McSPI TX",
 			omap2_mcspi_dma_tx_callback, spi,
 			&mcspi_dma->dma_tx_channel)) {
@@ -777,9 +890,7 @@ static int omap2_mcspi_request_dma(struct spi_device *spi)
 		dev_err(&spi->dev, "no TX DMA channel for McSPI\n");
 		return -EAGAIN;
 	}
-
-	init_completion(&mcspi_dma->dma_rx_completion);
-	init_completion(&mcspi_dma->dma_tx_completion);
+#endif
 
 	return 0;
 }
@@ -812,8 +923,8 @@ static int omap2_mcspi_setup(struct spi_device *spi)
 		list_add_tail(&cs->node, &ctx->cs);
 	}
 
-	if (mcspi_dma->dma_rx_channel == -1
-			|| mcspi_dma->dma_tx_channel == -1) {
+	if ((!mcspi_dma->dma_rx && mcspi_dma->dma_rx_channel == -1) ||
+	    (!mcspi_dma->dma_tx && mcspi_dma->dma_tx_channel == -1)) {
 		ret = omap2_mcspi_request_dma(spi);
 		if (ret < 0)
 			return ret;
@@ -847,6 +958,14 @@ static void omap2_mcspi_cleanup(struct spi_device *spi)
 	if (spi->chip_select < spi->master->num_chipselect) {
 		mcspi_dma = &mcspi->dma_channels[spi->chip_select];
 
+		if (mcspi_dma->dma_rx) {
+			dma_release_channel(mcspi_dma->dma_rx);
+			mcspi_dma->dma_rx = NULL;
+		}
+		if (mcspi_dma->dma_tx) {
+			dma_release_channel(mcspi_dma->dma_tx);
+			mcspi_dma->dma_tx = NULL;
+		}
 		if (mcspi_dma->dma_rx_channel != -1) {
 			omap_free_dma(mcspi_dma->dma_rx_channel);
 			mcspi_dma->dma_rx_channel = -1;
-- 
1.7.4.4

Re: [CFT 07/11] spi: omap2-mcspi: add DMA engine support

[Linus Walleij]

On Thu, Jun 7, 2012 at 1:08 PM, Russell King
<rmk+kernel@arm.linux.org.uk> wrote:

> Add DMA engine support to the OMAP SPI driver.  This supplements the
> private DMA API implementation contained within this driver, and the
> driver can be independently switched at build time between using DMA
> engine and the private DMA API for the transmit and receive sides.
>
> Tested-by: Shubhrajyoti <shubhrajyoti@ti.com>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

This looks very good,
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [CFT 07/11] spi: omap2-mcspi: add DMA engine support

[Russell King - ARM Linux]

On Thu, Jun 07, 2012 at 12:08:35PM +0100, Russell King wrote:
> Add DMA engine support to the OMAP SPI driver.  This supplements the
> private DMA API implementation contained within this driver, and the
> driver can be independently switched at build time between using DMA
> engine and the private DMA API for the transmit and receive sides.
> 
> Tested-by: Shubhrajyoti <shubhrajyoti@ti.com>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Right, now that we have working OMAP in mainline again...

------------[ cut here ]------------
WARNING: at lib/dma-debug.c:865 check_unmap+0x1a0/0x6f8()
ks8851 spi1.0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x000000009f5c3002] [size=592 bytes]
Modules linked in:
Backtrace:
[<c0017dd0>] (dump_backtrace+0x0/0x10c) from [<c0346870>] (dump_stack+0x18/0x1c)
 r7:df79fd78 r6:c01a2200 r5:c04036bd r4:00000361
[<c0346858>] (dump_stack+0x0/0x1c) from [<c0033c48>] (warn_slowpath_common+0x58/0x70)
[<c0033bf0>] (warn_slowpath_common+0x0/0x70) from [<c0033d04>] (warn_slowpath_fmt+0x38/0x40)
 r8:df79fdf8 r7:00000000 r6:00000250 r5:00000000 r4:9f5c3002
[<c0033ccc>] (warn_slowpath_fmt+0x0/0x40) from [<c01a2200>] (check_unmap+0x1a0/0x6f8)
 r3:c040ea22 r2:c0403a0f
[<c01a2060>] (check_unmap+0x0/0x6f8) from [<c01a2978>] (debug_dma_unmap_page+0x74/0x80)
[<c01a2904>] (debug_dma_unmap_page+0x0/0x80) from [<c021bd64>] (omap2_mcspi_txrx_dma+0x33c/0x54c)
[<c021ba28>] (omap2_mcspi_txrx_dma+0x0/0x54c) from [<c021c590>] (omap2_mcspi_work+0x1b8/0x2b8)
[<c021c3d8>] (omap2_mcspi_work+0x0/0x2b8) from [<c021c974>] (omap2_mcspi_transfer_one_message+0x2e4/0x310)
[<c021c690>] (omap2_mcspi_transfer_one_message+0x0/0x310) from [<c021a9f8>] (spi_pump_messages+0x130/0x154)
[<c021a8c8>] (spi_pump_messages+0x0/0x154) from [<c00508dc>] (kthread_worker_fn+0x108/0x188)
 r7:df6a3d94 r6:df79e000 r5:df6a3d90 r4:df6a3da4
[<c00507d4>] (kthread_worker_fn+0x0/0x188) from [<c0050a60>] (kthread+0x8c/0x98)
[<c00509d4>] (kthread+0x0/0x98) from [<c0039134>] (do_exit+0x0/0x314)
 r7:00000013 r6:c0039134 r5:c00509d4 r4:df443d78
---[ end trace 1b75b31a2719ed1f ]---

So, trying to figure this out... the result is not nice.

If the spi message has is_dma_mapped = false, then we potentially map the
DMA buffers against mcspi->dev.  This struct device is the same as the
master->dev.parent.

However, when we come to complete a transfer, we unmap them against the
spi_device's struct device - in other words a different device.

That's the reason for the warning.  However, when using DMA engine, both
of these struct device's are the wrong one to be using - the right one to
use is the one assocated with the DMA engine.

However, this presents a problem with transfers with is_dma_mapped = true.
SPI device drivers appear to assume that the right struct device to use
to map for DMA is master->dev.parent.  That's fine if your SPI master
device is the struct device performing the DMA, but with DMA engine
involved, this is not true.  Not sure at the moment what to do about
that one.

Re: [CFT 07/11] spi: omap2-mcspi: add DMA engine support

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 12:53:35PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 07, 2012 at 12:08:35PM +0100, Russell King wrote:
> > Add DMA engine support to the OMAP SPI driver.  This supplements the
> > private DMA API implementation contained within this driver, and the
> > driver can be independently switched at build time between using DMA
> > engine and the private DMA API for the transmit and receive sides.
> > 
> > Tested-by: Shubhrajyoti <shubhrajyoti@ti.com>
> > Acked-by: Grant Likely <grant.likely@secretlab.ca>
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> Right, now that we have working OMAP in mainline again...

Another warning:

------------[ cut here ]------------
WARNING: at /home/rmk/git/linux-rmk/drivers/base/dd.c:257 driver_probe_device+0x78/0x21c()
Modules linked in:
Backtrace:
[<c0017d0c>] (dump_backtrace+0x0/0x10c) from [<c033e208>] (dump_stack+0x18/0x1c) r7:00000000 r6:c01ff28c r5:c040050c r4:00000101
[<c033e1f0>] (dump_stack+0x0/0x1c) from [<c00337ec>] (warn_slowpath_common+0x58/0x70)
[<c0033794>] (warn_slowpath_common+0x0/0x70) from [<c0033828>] (warn_slowpath_null+0x24/0x2c)
 r8:c04aa4d8 r7:c04aa63c r6:de70ce00 r5:de70ce34 r4:de70ce00
[<c0033804>] (warn_slowpath_null+0x0/0x2c) from [<c01ff28c>] (driver_probe_device+0x78/0x21c)
[<c01ff214>] (driver_probe_device+0x0/0x21c) from [<c01ff49c>] (__driver_attach+0x6c/0x90)
 r7:de443ec8 r6:c04aa63c r5:de70ce34 r4:de70ce00
[<c01ff430>] (__driver_attach+0x0/0x90) from [<c01fda70>] (bus_for_each_dev+0x58/0x98)
 r6:c04aa63c r5:c01ff430 r4:00000000
[<c01fda18>] (bus_for_each_dev+0x0/0x98) from [<c01ff0f4>] (driver_attach+0x20/0x28)
 r7:de6c9e80 r6:c04aa63c r5:c04aa63c r4:c0465b80
[<c01ff0d4>] (driver_attach+0x0/0x28) from [<c01fe2f4>] (bus_add_driver+0xb4/0x230)
[<c01fe240>] (bus_add_driver+0x0/0x230) from [<c01ffb24>] (driver_register+0xac/0x138)
[<c01ffa78>] (driver_register+0x0/0x138) from [<c0215d4c>] (spi_register_driver+0x4c/0x60)
 r8:0000005b r7:c045f848 r6:00000006 r5:00000018 r4:c0465b80
[<c0215d00>] (spi_register_driver+0x0/0x60) from [<c045414c>] (ks8851_init+0x14/0x1c)
[<c0454138>] (ks8851_init+0x0/0x1c) from [<c0008770>] (do_one_initcall+0x9c/0x164)
[<c00086d4>] (do_one_initcall+0x0/0x164) from [<c0436410>] (kernel_init+0x128/0x210)
[<c04362e8>] (kernel_init+0x0/0x210) from [<c0038754>] (do_exit+0x0/0x72c)
---[ end trace 4dcda79f5e89dd84 ]---
ks8851 spi1.0: message enable is 0
ks8851 spi1.0: eth0: revision 0, MAC 08:00:28:01:4d:c6, IRQ 194, has EEPROM

The relevant line:

        WARN_ON(!list_empty(&dev->devres_head));

Which suggests that someone is using devres against the struct device for
the KS8851 device before the driver is bound.  That's a bug, plain and
simple.  I've not yet been able to track down what it is or where it's
being done, but it is something that has been introduced during the last
merge window.

devm_* APIs should only be used by _drivers_ against the struct device
that they are driving - because the lifetime of these things is bounded
by the point at which the driver is bound to that struct device, to the
point that it is unbound from that struct device (and at that point,
all devm_* stuff against the struct device gets destroyed.)

Re: [CFT 07/11] spi: omap2-mcspi: add DMA engine support

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 01:08:43PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 14, 2012 at 12:53:35PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Jun 07, 2012 at 12:08:35PM +0100, Russell King wrote:
> > > Add DMA engine support to the OMAP SPI driver.  This supplements the
> > > private DMA API implementation contained within this driver, and the
> > > driver can be independently switched at build time between using DMA
> > > engine and the private DMA API for the transmit and receive sides.
> > > 
> > > Tested-by: Shubhrajyoti <shubhrajyoti@ti.com>
> > > Acked-by: Grant Likely <grant.likely@secretlab.ca>
> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > 
> > Right, now that we have working OMAP in mainline again...
> 
> Another warning:
> 
> ------------[ cut here ]------------
> WARNING: at /home/rmk/git/linux-rmk/drivers/base/dd.c:257 driver_probe_device+0x78/0x21c()
> Modules linked in:
> Backtrace:
> [<c0017d0c>] (dump_backtrace+0x0/0x10c) from [<c033e208>] (dump_stack+0x18/0x1c) r7:00000000 r6:c01ff28c r5:c040050c r4:00000101
> [<c033e1f0>] (dump_stack+0x0/0x1c) from [<c00337ec>] (warn_slowpath_common+0x58/0x70)
> [<c0033794>] (warn_slowpath_common+0x0/0x70) from [<c0033828>] (warn_slowpath_null+0x24/0x2c)
>  r8:c04aa4d8 r7:c04aa63c r6:de70ce00 r5:de70ce34 r4:de70ce00
> [<c0033804>] (warn_slowpath_null+0x0/0x2c) from [<c01ff28c>] (driver_probe_device+0x78/0x21c)
> [<c01ff214>] (driver_probe_device+0x0/0x21c) from [<c01ff49c>] (__driver_attach+0x6c/0x90)
>  r7:de443ec8 r6:c04aa63c r5:de70ce34 r4:de70ce00
> [<c01ff430>] (__driver_attach+0x0/0x90) from [<c01fda70>] (bus_for_each_dev+0x58/0x98)
>  r6:c04aa63c r5:c01ff430 r4:00000000
> [<c01fda18>] (bus_for_each_dev+0x0/0x98) from [<c01ff0f4>] (driver_attach+0x20/0x28)
>  r7:de6c9e80 r6:c04aa63c r5:c04aa63c r4:c0465b80
> [<c01ff0d4>] (driver_attach+0x0/0x28) from [<c01fe2f4>] (bus_add_driver+0xb4/0x230)
> [<c01fe240>] (bus_add_driver+0x0/0x230) from [<c01ffb24>] (driver_register+0xac/0x138)
> [<c01ffa78>] (driver_register+0x0/0x138) from [<c0215d4c>] (spi_register_driver+0x4c/0x60)
>  r8:0000005b r7:c045f848 r6:00000006 r5:00000018 r4:c0465b80
> [<c0215d00>] (spi_register_driver+0x0/0x60) from [<c045414c>] (ks8851_init+0x14/0x1c)
> [<c0454138>] (ks8851_init+0x0/0x1c) from [<c0008770>] (do_one_initcall+0x9c/0x164)
> [<c00086d4>] (do_one_initcall+0x0/0x164) from [<c0436410>] (kernel_init+0x128/0x210)
> [<c04362e8>] (kernel_init+0x0/0x210) from [<c0038754>] (do_exit+0x0/0x72c)
> ---[ end trace 4dcda79f5e89dd84 ]---
> ks8851 spi1.0: message enable is 0
> ks8851 spi1.0: eth0: revision 0, MAC 08:00:28:01:4d:c6, IRQ 194, has EEPROM
> 
> The relevant line:
> 
>         WARN_ON(!list_empty(&dev->devres_head));
> 
> Which suggests that someone is using devres against the struct device for
> the KS8851 device before the driver is bound.  That's a bug, plain and
> simple.  I've not yet been able to track down what it is or where it's
> being done, but it is something that has been introduced during the last
> merge window.
> 
> devm_* APIs should only be used by _drivers_ against the struct device
> that they are driving - because the lifetime of these things is bounded
> by the point at which the driver is bound to that struct device, to the
> point that it is unbound from that struct device (and at that point,
> all devm_* stuff against the struct device gets destroyed.)

This commit introduced the bug:

commit 1a77b127ae147f5827043a9896d7f4cb248b402e
Author: Shubhrajyoti D <shubhrajyoti@ti.com>
Date:   Sat Mar 17 12:44:01 2012 +0530

    OMAP : SPI : use devm_* functions

    The various devm_* functions allocate memory that is released when a driver
    detaches. This patch uses devm_request_and_ioremap
    to request memory in probe function. Since the freeing is not
    needed the calls are deleted from remove function.Also use
    use devm_kzalloc for the cs memory allocation.

    Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>

and sure enough, reverting this makes the warning go away.

Specifically, it is this part which is the culpret:

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 7745f91..cb2c0e3 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -789,7 +789,7 @@ static int omap2_mcspi_setup(struct spi_device *spi)
        mcspi_dma = &mcspi->dma_channels[spi->chip_select];

        if (!cs) {
-               cs = kzalloc(sizeof *cs, GFP_KERNEL);
+               cs = devm_kzalloc(&spi->dev , sizeof *cs, GFP_KERNEL);
                if (!cs)
                        return -ENOMEM;
                cs->base = mcspi->base + spi->chip_select * 0x14;
@@ -831,7 +831,6 @@ static void omap2_mcspi_cleanup(struct spi_device *spi)
                cs = spi->controller_state;
                list_del(&cs->node);

-               kfree(spi->controller_state);
        }

        if (spi->chip_select < spi->master->num_chipselect) {

because, at the time when omap2_mcspi_setup() is called, spi->dev is
not bound, and so is outside of the devres valid lifetime of that
struct device.

[PATCH] SPI: OMAP: fix over-eager devm_xxx() conversion (was: Re: [CFT 07/11] spi: omap2-mcspi: add DMA engine support)

[Russell King - ARM Linux]

From: Russell King <rmk+kernel@arm.linux.org.uk>
Subject: [PATCH] SPI: OMAP: fix over-eager devm_xxx() conversion

1a77b127ae (OMAP : SPI : use devm_* functions) converted the SPI
device controller state to use devm_kzalloc().  Unfortunately, this
is used against an unbound struct device, which results in the
following when the device is eventually bound to its driver:

------------[ cut here ]------------
WARNING: at /home/rmk/git/linux-rmk/drivers/base/dd.c:257 driver_probe_device+0x78/0x21c()
Modules linked in:
Backtrace:
[<c0017d0c>] (dump_backtrace+0x0/0x10c) from [<c033e208>] (dump_stack+0x18/0x1c) r7:00000000 r6:c01ff28c r5:c040050c r4:00000101
[<c033e1f0>] (dump_stack+0x0/0x1c) from [<c00337ec>] (warn_slowpath_common+0x58/0x70)
[<c0033794>] (warn_slowpath_common+0x0/0x70) from [<c0033828>] (warn_slowpath_null+0x24/0x2c)
[<c0033804>] (warn_slowpath_null+0x0/0x2c) from [<c01ff28c>] (driver_probe_device+0x78/0x21c)
[<c01ff214>] (driver_probe_device+0x0/0x21c) from [<c01ff49c>] (__driver_attach+0x6c/0x90)
[<c01ff430>] (__driver_attach+0x0/0x90) from [<c01fda70>] (bus_for_each_dev+0x58/0x98)
[<c01fda18>] (bus_for_each_dev+0x0/0x98) from [<c01ff0f4>] (driver_attach+0x20/0x28)
[<c01ff0d4>] (driver_attach+0x0/0x28) from [<c01fe2f4>] (bus_add_driver+0xb4/0x230)
[<c01fe240>] (bus_add_driver+0x0/0x230) from [<c01ffb24>] (driver_register+0xac/0x138)
[<c01ffa78>] (driver_register+0x0/0x138) from [<c0215d4c>] (spi_register_driver+0x4c/0x60)
[<c0215d00>] (spi_register_driver+0x0/0x60) from [<c045414c>] (ks8851_init+0x14/0x1c)
[<c0454138>] (ks8851_init+0x0/0x1c) from [<c0008770>] (do_one_initcall+0x9c/0x164)
[<c00086d4>] (do_one_initcall+0x0/0x164) from [<c0436410>] (kernel_init+0x128/0x210)
[<c04362e8>] (kernel_init+0x0/0x210) from [<c0038754>] (do_exit+0x0/0x72c)
---[ end trace 4dcda79f5e89dd84 ]---
ks8851 spi1.0: message enable is 0
ks8851 spi1.0: eth0: revision 0, MAC 08:00:28:01:4d:c6, IRQ 194, has EEPROM

Fix this by partially reverting the original commit.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/spi/spi-omap2-mcspi.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 9d3409a..6263b0f 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -829,7 +829,7 @@ static int omap2_mcspi_setup(struct spi_device *spi)
 	mcspi_dma = &mcspi->dma_channels[spi->chip_select];
 
 	if (!cs) {
-		cs = devm_kzalloc(&spi->dev , sizeof *cs, GFP_KERNEL);
+		cs = kzalloc(sizeof *cs, GFP_KERNEL);
 		if (!cs)
 			return -ENOMEM;
 		cs->base = mcspi->base + spi->chip_select * 0x14;
@@ -869,6 +869,7 @@ static void omap2_mcspi_cleanup(struct spi_device *spi)
 		cs = spi->controller_state;
 		list_del(&cs->node);
 
+		kfree(cs);
 	}
 
 	if (spi->chip_select < spi->master->num_chipselect) {

Re: [PATCH] SPI: OMAP: fix over-eager devm_xxx() conversion (was: Re: [CFT 07/11] spi: omap2-mcspi: add DMA engine support)

[Russell King - ARM Linux]

Okay, I'm going to queue this up in my tree for -rc as no one seems to
be listening to any of the emails I've sent on Thursday.

On Thu, Jun 14, 2012 at 03:07:12PM +0100, Russell King - ARM Linux wrote:
> From: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Subject: [PATCH] SPI: OMAP: fix over-eager devm_xxx() conversion
> 
> 1a77b127ae (OMAP : SPI : use devm_* functions) converted the SPI
> device controller state to use devm_kzalloc().  Unfortunately, this
> is used against an unbound struct device, which results in the
> following when the device is eventually bound to its driver:
> 
> ------------[ cut here ]------------
> WARNING: at /home/rmk/git/linux-rmk/drivers/base/dd.c:257 driver_probe_device+0x78/0x21c()
> Modules linked in:
> Backtrace:
> [<c0017d0c>] (dump_backtrace+0x0/0x10c) from [<c033e208>] (dump_stack+0x18/0x1c) r7:00000000 r6:c01ff28c r5:c040050c r4:00000101
> [<c033e1f0>] (dump_stack+0x0/0x1c) from [<c00337ec>] (warn_slowpath_common+0x58/0x70)
> [<c0033794>] (warn_slowpath_common+0x0/0x70) from [<c0033828>] (warn_slowpath_null+0x24/0x2c)
> [<c0033804>] (warn_slowpath_null+0x0/0x2c) from [<c01ff28c>] (driver_probe_device+0x78/0x21c)
> [<c01ff214>] (driver_probe_device+0x0/0x21c) from [<c01ff49c>] (__driver_attach+0x6c/0x90)
> [<c01ff430>] (__driver_attach+0x0/0x90) from [<c01fda70>] (bus_for_each_dev+0x58/0x98)
> [<c01fda18>] (bus_for_each_dev+0x0/0x98) from [<c01ff0f4>] (driver_attach+0x20/0x28)
> [<c01ff0d4>] (driver_attach+0x0/0x28) from [<c01fe2f4>] (bus_add_driver+0xb4/0x230)
> [<c01fe240>] (bus_add_driver+0x0/0x230) from [<c01ffb24>] (driver_register+0xac/0x138)
> [<c01ffa78>] (driver_register+0x0/0x138) from [<c0215d4c>] (spi_register_driver+0x4c/0x60)
> [<c0215d00>] (spi_register_driver+0x0/0x60) from [<c045414c>] (ks8851_init+0x14/0x1c)
> [<c0454138>] (ks8851_init+0x0/0x1c) from [<c0008770>] (do_one_initcall+0x9c/0x164)
> [<c00086d4>] (do_one_initcall+0x0/0x164) from [<c0436410>] (kernel_init+0x128/0x210)
> [<c04362e8>] (kernel_init+0x0/0x210) from [<c0038754>] (do_exit+0x0/0x72c)
> ---[ end trace 4dcda79f5e89dd84 ]---
> ks8851 spi1.0: message enable is 0
> ks8851 spi1.0: eth0: revision 0, MAC 08:00:28:01:4d:c6, IRQ 194, has EEPROM
> 
> Fix this by partially reverting the original commit.
> 
> Signed-off-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> ---
>  drivers/spi/spi-omap2-mcspi.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> index 9d3409a..6263b0f 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -829,7 +829,7 @@ static int omap2_mcspi_setup(struct spi_device *spi)
>  	mcspi_dma = &mcspi->dma_channels[spi->chip_select];
>  
>  	if (!cs) {
> -		cs = devm_kzalloc(&spi->dev , sizeof *cs, GFP_KERNEL);
> +		cs = kzalloc(sizeof *cs, GFP_KERNEL);
>  		if (!cs)
>  			return -ENOMEM;
>  		cs->base = mcspi->base + spi->chip_select * 0x14;
> @@ -869,6 +869,7 @@ static void omap2_mcspi_cleanup(struct spi_device *spi)
>  		cs = spi->controller_state;
>  		list_del(&cs->node);
>  
> +		kfree(cs);
>  	}
>  
>  	if (spi->chip_select < spi->master->num_chipselect) {
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [CFT 07/11] spi: omap2-mcspi: add DMA engine support

[Shubhrajyoti]

On Thursday 14 June 2012 06:20 PM, Russell King - ARM Linux wrote:
> because, at the time when omap2_mcspi_setup() is called, spi->dev is
> not bound, and so is outside of the devres valid lifetime of that
> struct device.
Agree,
Apologies for breaking in the initial commit.

[CFT 08/11] spi: omap2-mcspi: remove private DMA API implementation

[Russell King]

Remove the private DMA API implementation from spi-omap2-mcspi.c,
making it use entirely the DMA engine API.

Acked-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/spi/spi-omap2-mcspi.c |  104 ++---------------------------------------
 1 files changed, 5 insertions(+), 99 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index ca016df..9d3409a 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -20,8 +20,6 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
  *
  */
-#define USE_DMA_ENGINE_RX
-#define USE_DMA_ENGINE_TX
 
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -43,7 +41,6 @@
 
 #include <linux/spi/spi.h>
 
-#include <plat/dma.h>
 #include <plat/clock.h>
 #include <plat/mcspi.h>
 
@@ -99,8 +96,6 @@
 struct omap2_mcspi_dma {
 	struct dma_chan *dma_tx;
 	struct dma_chan *dma_rx;
-	int dma_tx_channel;
-	int dma_rx_channel;
 
 	int dma_tx_sync_dev;
 	int dma_rx_sync_dev;
@@ -336,9 +331,8 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 	struct omap2_mcspi	*mcspi;
 	struct omap2_mcspi_cs	*cs = spi->controller_state;
 	struct omap2_mcspi_dma  *mcspi_dma;
-	unsigned int		count, c;
-	unsigned long		base, tx_reg, rx_reg;
-	int			word_len, data_type, element_count;
+	unsigned int		count;
+	int			word_len, element_count;
 	int			elements = 0;
 	u32			l;
 	u8			* rx;
@@ -420,73 +414,26 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 	}
 
 	count = xfer->len;
-	c = count;
 	word_len = cs->word_len;
 
-	base = cs->phys;
-	tx_reg = base + OMAP2_MCSPI_TX0;
-	rx_reg = base + OMAP2_MCSPI_RX0;
 	rx = xfer->rx_buf;
 	tx = xfer->tx_buf;
 
 	if (word_len <= 8) {
-		data_type = OMAP_DMA_DATA_TYPE_S8;
 		element_count = count;
 	} else if (word_len <= 16) {
-		data_type = OMAP_DMA_DATA_TYPE_S16;
 		element_count = count >> 1;
 	} else /* word_len <= 32 */ {
-		data_type = OMAP_DMA_DATA_TYPE_S32;
 		element_count = count >> 2;
 	}
 
-	if (tx != NULL && mcspi_dma->dma_tx_channel != -1) {
-		omap_set_dma_transfer_params(mcspi_dma->dma_tx_channel,
-				data_type, element_count, 1,
-				OMAP_DMA_SYNC_ELEMENT,
-				mcspi_dma->dma_tx_sync_dev, 0);
-
-		omap_set_dma_dest_params(mcspi_dma->dma_tx_channel, 0,
-				OMAP_DMA_AMODE_CONSTANT,
-				tx_reg, 0, 0);
-
-		omap_set_dma_src_params(mcspi_dma->dma_tx_channel, 0,
-				OMAP_DMA_AMODE_POST_INC,
-				xfer->tx_dma, 0, 0);
-	}
-
-	if (rx != NULL && mcspi_dma->dma_rx_channel != -1) {
-		elements = element_count - 1;
-		if (l & OMAP2_MCSPI_CHCONF_TURBO)
-			elements--;
-
-		omap_set_dma_transfer_params(mcspi_dma->dma_rx_channel,
-				data_type, elements, 1,
-				OMAP_DMA_SYNC_ELEMENT,
-				mcspi_dma->dma_rx_sync_dev, 1);
-
-		omap_set_dma_src_params(mcspi_dma->dma_rx_channel, 0,
-				OMAP_DMA_AMODE_CONSTANT,
-				rx_reg, 0, 0);
-
-		omap_set_dma_dest_params(mcspi_dma->dma_rx_channel, 0,
-				OMAP_DMA_AMODE_POST_INC,
-				xfer->rx_dma, 0, 0);
-	}
-
 	if (tx != NULL) {
-		if (mcspi_dma->dma_tx)
-			dma_async_issue_pending(mcspi_dma->dma_tx);
-		else
-			omap_start_dma(mcspi_dma->dma_tx_channel);
+		dma_async_issue_pending(mcspi_dma->dma_tx);
 		omap2_mcspi_set_dma_req(spi, 0, 1);
 	}
 
 	if (rx != NULL) {
-		if (mcspi_dma->dma_rx)
-			dma_async_issue_pending(mcspi_dma->dma_rx);
-		else
-			omap_start_dma(mcspi_dma->dma_rx_channel);
+		dma_async_issue_pending(mcspi_dma->dma_rx);
 		omap2_mcspi_set_dma_req(spi, 1, 1);
 	}
 
@@ -830,16 +777,6 @@ static int omap2_mcspi_setup_transfer(struct spi_device *spi,
 	return 0;
 }
 
-static void omap2_mcspi_dma_rx_callback(int lch, u16 ch_status, void *data)
-{
-	omap2_mcspi_rx_callback(data);
-}
-
-static void omap2_mcspi_dma_tx_callback(int lch, u16 ch_status, void *data)
-{
-	omap2_mcspi_tx_callback(data);
-}
-
 static int omap2_mcspi_request_dma(struct spi_device *spi)
 {
 	struct spi_master	*master = spi->master;
@@ -856,23 +793,13 @@ static int omap2_mcspi_request_dma(struct spi_device *spi)
 
 	dma_cap_zero(mask);
 	dma_cap_set(DMA_SLAVE, mask);
-#ifdef USE_DMA_ENGINE_RX
 	sig = mcspi_dma->dma_rx_sync_dev;
 	mcspi_dma->dma_rx = dma_request_channel(mask, omap_dma_filter_fn, &sig);
 	if (!mcspi_dma->dma_rx) {
 		dev_err(&spi->dev, "no RX DMA engine channel for McSPI\n");
 		return -EAGAIN;
 	}
-#else
-	if (omap_request_dma(mcspi_dma->dma_rx_sync_dev, "McSPI RX",
-			omap2_mcspi_dma_rx_callback, spi,
-			&mcspi_dma->dma_rx_channel)) {
-		dev_err(&spi->dev, "no RX DMA channel for McSPI\n");
-		return -EAGAIN;
-	}
-#endif
 
-#ifdef USE_DMA_ENGINE_TX
 	sig = mcspi_dma->dma_tx_sync_dev;
 	mcspi_dma->dma_tx = dma_request_channel(mask, omap_dma_filter_fn, &sig);
 	if (!mcspi_dma->dma_tx) {
@@ -881,16 +808,6 @@ static int omap2_mcspi_request_dma(struct spi_device *spi)
 		mcspi_dma->dma_rx = NULL;
 		return -EAGAIN;
 	}
-#else
-	if (omap_request_dma(mcspi_dma->dma_tx_sync_dev, "McSPI TX",
-			omap2_mcspi_dma_tx_callback, spi,
-			&mcspi_dma->dma_tx_channel)) {
-		omap_free_dma(mcspi_dma->dma_rx_channel);
-		mcspi_dma->dma_rx_channel = -1;
-		dev_err(&spi->dev, "no TX DMA channel for McSPI\n");
-		return -EAGAIN;
-	}
-#endif
 
 	return 0;
 }
@@ -923,8 +840,7 @@ static int omap2_mcspi_setup(struct spi_device *spi)
 		list_add_tail(&cs->node, &ctx->cs);
 	}
 
-	if ((!mcspi_dma->dma_rx && mcspi_dma->dma_rx_channel == -1) ||
-	    (!mcspi_dma->dma_tx && mcspi_dma->dma_tx_channel == -1)) {
+	if (!mcspi_dma->dma_rx || !mcspi_dma->dma_tx) {
 		ret = omap2_mcspi_request_dma(spi);
 		if (ret < 0)
 			return ret;
@@ -966,14 +882,6 @@ static void omap2_mcspi_cleanup(struct spi_device *spi)
 			dma_release_channel(mcspi_dma->dma_tx);
 			mcspi_dma->dma_tx = NULL;
 		}
-		if (mcspi_dma->dma_rx_channel != -1) {
-			omap_free_dma(mcspi_dma->dma_rx_channel);
-			mcspi_dma->dma_rx_channel = -1;
-		}
-		if (mcspi_dma->dma_tx_channel != -1) {
-			omap_free_dma(mcspi_dma->dma_tx_channel);
-			mcspi_dma->dma_tx_channel = -1;
-		}
 	}
 }
 
@@ -1292,7 +1200,6 @@ static int __devinit omap2_mcspi_probe(struct platform_device *pdev)
 			break;
 		}
 
-		mcspi->dma_channels[i].dma_rx_channel = -1;
 		mcspi->dma_channels[i].dma_rx_sync_dev = dma_res->start;
 		sprintf(dma_ch_name, "tx%d", i);
 		dma_res = platform_get_resource_byname(pdev, IORESOURCE_DMA,
@@ -1303,7 +1210,6 @@ static int __devinit omap2_mcspi_probe(struct platform_device *pdev)
 			break;
 		}
 
-		mcspi->dma_channels[i].dma_tx_channel = -1;
 		mcspi->dma_channels[i].dma_tx_sync_dev = dma_res->start;
 	}
 
-- 
1.7.4.4