[00/12] DMAENGINE: PL08X driver updates

[00/12] DMAENGINE: PL08X driver updates

[Linus Walleij]

Hi Dan,

This updates the PL08X driver in the async_tx next branch with
a number of fixes and features, each one pretty self-describing.
This has been tested with the PrimeCell DMA series on the
RealView PB11MPCore and works flawlessly, multiplexing
several virtual clients, both devices (slaves) and memcpy
channels, across only two (!) available physical channels.

If you prefer you can squash these into the initial driver
submission, I don't mind.

Yours,
Linus Walleij

[PATCH 01/12] DMAENGINE: remove clock code from PL08X driver

[Linus Walleij]

With the introduction of the pclk infrastructure the block clock
is handled by the AMBA PrimeCell bus core, and we can remove the
local references to any clocks.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index fc5aaeb..2afb4f6 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -68,7 +68,6 @@
 #include <linux/amba/bus.h>
 #include <linux/dmaengine.h>
 #include <linux/amba/pl08x.h>
-#include <linux/clk.h>
 
 #include <asm/hardware/pl080.h>
 #include <asm/dma.h>
@@ -113,7 +112,6 @@ struct lli {
  * @adev: the corresponding AMBA (PrimeCell) bus entry
  * @vd: vendor data for this PL08x variant
  * @pd: platform data passed in from the platform/machine
- * @clk: a clock to be enabled for this device to run
  * @phy_chans: array of data for the physical channels
  * @pool: a pool for the LLI descriptors
  * @pool_ctr: counter of LLIs in the pool
@@ -124,7 +122,6 @@ struct pl08x_driver_data {
 	struct amba_device *adev;
 	struct vendor_data *vd;
 	struct pl08x_platform_data *pd;
-	struct clk *clk;
 	struct pl08x_phy_chan *phy_chans;
 	struct dma_pool *pool;
 	int pool_ctr;
@@ -1826,19 +1823,6 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 	pl08x->adev = adev;
 	pl08x->vd = vd;
 
-	pl08x->clk = clk_get(&adev->dev, NULL);
-	if (IS_ERR(pl08x->clk)) {
-		ret = PTR_ERR(pl08x->clk);
-		if (ret == -ENOENT) {
-			dev_info(&adev->dev, "no clock supplied, assume always on\n");
-			pl08x->clk = NULL;
-		} else
-			goto out_no_clk;
-	}
-	/* Constantly enabled for now, we can control this dynamically */
-	if (pl08x->clk)
-		clk_enable(pl08x->clk);
-
 	/* A DMA memory pool for LLIs, align on 1-byte boundary */
 	pl08x->pool = dma_pool_create(DRIVER_NAME, &pl08x->adev->dev,
 			PL08X_LLI_TSFR_SIZE, PL08X_ALIGN, 0);
@@ -1963,8 +1947,6 @@ out_no_irq:
 out_no_ioremap:
 	dma_pool_destroy(pl08x->pool);
 out_no_lli_pool:
-	clk_put(pl08x->clk);
-out_no_clk:
 	kfree(pl08x);
 out_no_pl08x:
 	amba_release_regions(adev);
-- 
1.6.3.3

[PATCH 02/12] DMAENGINE: safeguard PL08X from missing platform data

[Linus Walleij]

The PL08X needs some platform data with channel assignments etc to
work properly, bail out of this is not supplied.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 2afb4f6..506749b 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1819,6 +1819,13 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 		goto out_no_pl08x;
 	}
 
+	/* Get the platform data */
+	pl08x->pd = dev_get_platdata(&adev->dev);
+	if (!pl08x->pd) {
+		dev_err(&adev->dev, "no platform data supplied\n");
+		goto out_no_platdata;
+	}
+
 	/* Assign useful pointers to the driver state */
 	pl08x->adev = adev;
 	pl08x->vd = vd;
@@ -1879,9 +1886,6 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 			 pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE");
 	}
 
-	/* Get the platform data */
-	pl08x->pd = dev_get_platdata(&adev->dev);
-
 	/* Set caps */
 	dma_cap_set(DMA_MEMCPY, dmac_memcpy.cap_mask);
 	dma_cap_set(DMA_SLAVE, dmac_slave.cap_mask);
@@ -1947,6 +1951,7 @@ out_no_irq:
 out_no_ioremap:
 	dma_pool_destroy(pl08x->pool);
 out_no_lli_pool:
+out_no_platdata:
 	kfree(pl08x);
 out_no_pl08x:
 	amba_release_regions(adev);
-- 
1.6.3.3

[PATCH 03/12] DMAENGINE: fetch tasklet struct for PL08X include file

[Linus Walleij]

The PL08X include file is using struct tasklet so let us bring this
in apropriately.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 include/linux/amba/pl08x.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index 8435f4a..2bb7ac4 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -20,6 +20,7 @@
 
 /* We need sizes of structs from this header */
 #include <linux/dmaengine.h>
+#include <linux/interrupt.h>
 
 /**
  * struct pl08x_channel_data - data structure to pass info between
-- 
1.6.3.3

[PATCH 04/12] DMAENGINE: use GFP_NOWAIT for PL08X allocations

[Linus Walleij]

Since DMA jobs can be prepared and fired in IRQ context, request
GFP_NOWAIT memory for the structs.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 506749b..d1a4813 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -628,7 +628,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		return 0;
 	}
 
-	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_KERNEL,
+	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT,
 				      &txd->llis_bus);
 	if (!txd->llis_va) {
 		dev_err(&pl08x->adev->dev, "%s no memory for llis\n", __func__);
@@ -1564,7 +1564,7 @@ struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from %s\n",
 		__func__, sgl->length, plchan->name);
 
-	txd = kzalloc(sizeof(struct pl08x_txd), GFP_KERNEL);
+	txd = kzalloc(sizeof(struct pl08x_txd), GFP_NOWAIT);
 	if (!txd) {
 		dev_err(&pl08x->adev->dev, "%s no txd\n", __func__);
 		return NULL;
-- 
1.6.3.3

[PATCH 05/12] DMAENGINE: terminate transfers on PL08X mere throughly

[Linus Walleij]

We need to terminate pending tasklets and be sure to disable not
only the channel but it's IRQ:s when we terminate all work on
a PL08X channel.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index d1a4813..65f595c 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -312,6 +312,8 @@ static void pl08x_stop_phy_chan(struct pl08x_phy_chan *ch)
 	/* Disable channel */
 	val = readl(ch->base + PL080_CH_CONFIG);
 	val &= ~PL080_CONFIG_ENABLE;
+	val &= ~PL080_CONFIG_ERR_IRQ_MASK;
+	val &= ~PL080_CONFIG_TC_IRQ_MASK;
 	writel(val, ch->base + PL080_CH_CONFIG);
 }
 
@@ -1653,13 +1655,15 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 			pl08x_put_phy_channel(pl08x, plchan->phychan);
 			plchan->phychan = NULL;
 		}
-
+		/* Stop any pending tasklet */
+		tasklet_disable(&plchan->tasklet);
 		/* Dequeue jobs and free LLIs */
 		if (plchan->at) {
 			pl08x_free_txd(pl08x, plchan->at);
-			pl08x_free_txd_list(pl08x, plchan);
 			plchan->at = NULL;
 		}
+		/* Dequeue jobs not yet fired as well */
+		pl08x_free_txd_list(pl08x, plchan);
 		break;
 	case DMA_PAUSE:
 		pl08x_pause_phy_chan(plchan->phychan);
-- 
1.6.3.3

[PATCH 06/12] DMAENGINE: free txd list if submission fails on PL08X

[Linus Walleij]

Sometimes oversubscribed channels fail to mux in a signal line for
the transmission, and we bail out. Make sure we also clean out any
pending txd lists when this happens.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 65f595c..65892d5 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1270,6 +1270,7 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
 	ret = prep_phy_channel(plchan, txd);
 	if (ret) {
 		/* No physical channel available, cope with it */
+		pl08x_free_txd_list(pl08x, plchan);
 		spin_unlock_irqrestore(&plchan->lock, flags);
 		return -EBUSY;
 	}
-- 
1.6.3.3

[PATCH 07/12] DMAENGINE: add debugfs file for PL08X

[Linus Walleij]

This adds a debugfs file for the PL08X showing the current allocation
of physical to virtual channels.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 65892d5..a99ad19 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -68,6 +68,8 @@
 #include <linux/amba/bus.h>
 #include <linux/dmaengine.h>
 #include <linux/amba/pl08x.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #include <asm/hardware/pl080.h>
 #include <asm/dma.h>
@@ -1806,6 +1808,60 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 	return i;
 }
 
+#ifdef CONFIG_DEBUG_FS
+static int pl08x_debugfs_show(struct seq_file *s, void *data)
+{
+	struct pl08x_driver_data *pl08x = s->private;
+	struct pl08x_phy_chan *ch;
+	unsigned long flags;
+	int i;
+
+	seq_printf(s, "PL08x physical channels:\n");
+	seq_printf(s, "CHANNEL:\tUSER:\n");
+	seq_printf(s, "--------\t-----\n");
+	for (i = 0; i < pl08x->vd->channels; i++) {
+		struct pl08x_dma_chan *virt_chan;
+
+		ch = &pl08x->phy_chans[i];
+
+		spin_lock_irqsave(&ch->lock, flags);
+		virt_chan = ch->serving;
+
+		seq_printf(s, "%d\t\t%s\n",
+			   ch->id, virt_chan ? virt_chan->name : "(none)");
+
+		spin_unlock_irqrestore(&ch->lock, flags);
+	}
+	return 0;
+}
+
+static int pl08x_debugfs_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pl08x_debugfs_show, inode->i_private);
+}
+
+static const struct file_operations pl08x_debugfs_operations = {
+	.open		= pl08x_debugfs_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static void init_pl08x_debugfs(struct pl08x_driver_data *pl08x)
+{
+	/* Expose a simple debugfs interface to view all clocks */
+	(void) debugfs_create_file(dev_name(&pl08x->adev->dev), S_IFREG | S_IRUGO,
+				   NULL, pl08x,
+				   &pl08x_debugfs_operations);
+	return 0;
+}
+
+#else
+static inline void init_pl08x_debugfs(struct pl08x_driver_data *pl08x)
+{
+}
+#endif
+
 static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 {
 	struct pl08x_driver_data *pl08x;
@@ -1937,6 +1993,7 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 	}
 
 	amba_set_drvdata(adev, pl08x);
+	init_pl08x_debugfs(pl08x);
 	dev_info(&pl08x->adev->dev, "ARM(R) %s DMA block initialized @%08x\n",
 		vd->name, adev->res.start);
 	return 0;
-- 
1.6.3.3

[PATCH 08/12] DMAENGINE: be more explicit when freeing PL08X signals

[Linus Walleij]

The signals connected to the PL08X physical channels shall not
be tangled up with the physical channels themselves. This gets
some reference counting right.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index a99ad19..a7ce08e 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -449,7 +449,6 @@ static inline void pl08x_put_phy_channel(struct pl08x_driver_data *pl08x,
 	/* Mark it as free */
 	spin_lock_irqsave(&ch->lock, flags);
 	ch->serving = NULL;
-	ch->signal = -1;
 	spin_unlock_irqrestore(&ch->lock, flags);
 }
 
@@ -1100,8 +1099,10 @@ static void pl08x_tasklet(unsigned long data)
 		 * No more jobs, so free up the physical channel
 		 * Free any allocated signal on slave transfers too
 		 */
-		if ((phychan->signal >= 0) && pl08x->pd->put_signal)
+		if ((phychan->signal >= 0) && pl08x->pd->put_signal) {
 			pl08x->pd->put_signal(plchan);
+			phychan->signal = -1;
+		}
 		pl08x_put_phy_channel(pl08x, phychan);
 		plchan->phychan = NULL;
 	}
@@ -1193,6 +1194,7 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan,
 	 */
 	if ((txd->direction == DMA_FROM_DEVICE ||
 	     txd->direction == DMA_TO_DEVICE) &&
+	    ch->signal < 0 &&
 	    pl08x->pd->get_signal) {
 		ret = pl08x->pd->get_signal(plchan);
 		if (ret < 0) {
@@ -1653,8 +1655,10 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 			 * signal
 			 */
 			if ((plchan->phychan->signal >= 0) &&
-			    pl08x->pd->put_signal)
+			    pl08x->pd->put_signal) {
 				pl08x->pd->put_signal(plchan);
+				plchan->phychan->signal = -1;
+			}
 			pl08x_put_phy_channel(pl08x, plchan->phychan);
 			plchan->phychan = NULL;
 		}
-- 
1.6.3.3

[PATCH 09/12] DMAENGINE: remove pointless module safeguard on PL08X

[Linus Walleij]

The PL08X driver safeguards itself from being compiled as a module
for no reason: the Kconfig option is a bool anyway.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index a7ce08e..f80fc4b 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -130,18 +130,6 @@ struct pl08x_driver_data {
 	spinlock_t lock;
 };
 
-#ifdef MODULE
-
-# error "AMBA PL08X DMA CANNOT BE COMPILED AS A LOADABLE MODULE AT PRESENT"
-
-/*
-	a) Some devices might make use of DMA during boot
-	   (esp true for DMAENGINE implementation)
-	b) Memory allocation will need much more attention
-	   before load/unload can be supported
- */
-#endif
-
 /*
  * PL08X specific defines
  */
-- 
1.6.3.3

[PATCH 10/12] DMAENGINE: move the PL08X to full runtime allocation

[Linus Walleij]

This change makes the PL08X not being a singleton anymore, this
piece was missing earlier: we move the struct dma_device entries
into the driver state holder. Also flag channels as slaves and
fix up the channel freeing code.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c   |  112 +++++++++++++++++++++++++-------------------
 include/linux/amba/pl08x.h |    2 +
 2 files changed, 66 insertions(+), 48 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index f80fc4b..4573189 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -110,6 +110,8 @@ struct lli {
 
 /**
  * struct pl08x_driver_data - the local state holder for the PL08x
+ * @slave: slave engine for this instance
+ * @memcpy: memcpy engine for this instance
  * @base: virtual memory base (remapped) for the PL08x
  * @adev: the corresponding AMBA (PrimeCell) bus entry
  * @vd: vendor data for this PL08x variant
@@ -120,6 +122,8 @@ struct lli {
  * @lock: a spinlock for this struct
  */
 struct pl08x_driver_data {
+	struct dma_device slave;
+	struct dma_device memcpy;
 	void __iomem *base;
 	struct amba_device *adev;
 	struct vendor_data *vd;
@@ -1691,38 +1695,6 @@ bool pl08x_filter_id(struct dma_chan *chan, void *chan_id)
 	return false;
 }
 
-static struct dma_device dmac_memcpy = {
-	.device_alloc_chan_resources	= pl08x_alloc_chan_resources,
-	.device_free_chan_resources	= pl08x_free_chan_resources,
-	.device_prep_dma_memcpy		= pl08x_prep_dma_memcpy,
-	.device_prep_dma_xor		= NULL,
-	.device_prep_dma_memset		= NULL,
-	.device_prep_dma_interrupt	= pl08x_prep_dma_interrupt,
-	.device_tx_status		= pl08x_dma_tx_status,
-	.device_issue_pending		= pl08x_issue_pending,
-	.device_control			= pl08x_control,
-	/*
-	 * Align to 4-byte boundary
-	 * This makes the DMAtests fail with grace on PB1176
-	 * broken DMA hardware instead of locking everything
-	 * up.
-	 */
-	/* .copy_align			= 2, */
-};
-
-static struct dma_device dmac_slave = {
-	.device_alloc_chan_resources	= pl08x_alloc_chan_resources,
-	.device_free_chan_resources	= pl08x_free_chan_resources,
-	.device_prep_dma_xor		= NULL,
-	.device_prep_dma_memset		= NULL,
-	.device_prep_dma_interrupt	= pl08x_prep_dma_interrupt,
-	.device_tx_status		= pl08x_dma_tx_status,
-	.device_issue_pending		= pl08x_issue_pending,
-	.device_prep_slave_sg		= pl08x_prep_slave_sg,
-	.device_control			= pl08x_control,
-};
-
-
 /*
  * Just check that the device is there and active
  * TODO: turn this bit on/off depending on the number of
@@ -1770,6 +1742,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 		chan->host = pl08x;
 
 		if (slave) {
+			chan->slave = true;
 			chan->name = pl08x->pd->slave_channels[i].bus_id;
 			chan->cd = &pl08x->pd->slave_channels[i];
 		} else {
@@ -1800,10 +1773,23 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 	return i;
 }
 
+static void pl08x_free_virtual_channels(struct dma_device *dmadev)
+{
+	struct pl08x_dma_chan *chan = NULL;
+	struct pl08x_dma_chan *next;
+
+	list_for_each_entry_safe(chan,
+				 next, &dmadev->channels, chan.device_node) {
+		list_del(&chan->chan.device_node);
+		kfree(chan);
+	}
+}
+
 #ifdef CONFIG_DEBUG_FS
 static int pl08x_debugfs_show(struct seq_file *s, void *data)
 {
 	struct pl08x_driver_data *pl08x = s->private;
+	struct pl08x_dma_chan *chan;
 	struct pl08x_phy_chan *ch;
 	unsigned long flags;
 	int i;
@@ -1824,6 +1810,21 @@ static int pl08x_debugfs_show(struct seq_file *s, void *data)
 
 		spin_unlock_irqrestore(&ch->lock, flags);
 	}
+
+	seq_printf(s, "\nPL08x virtual memcpy channels:\n");
+	seq_printf(s, "CHANNEL:\n");
+	seq_printf(s, "--------\n");
+	list_for_each_entry(chan, &pl08x->memcpy.channels, chan.device_node) {
+		seq_printf(s, "%s\n", chan->name);
+	}
+
+	seq_printf(s, "\nPL08x virtual slave channels:\n");
+	seq_printf(s, "CHANNEL:\n");
+	seq_printf(s, "--------\n");
+	list_for_each_entry(chan, &pl08x->slave.channels, chan.device_node) {
+		seq_printf(s, "%s\n", chan->name);
+	}
+
 	return 0;
 }
 
@@ -1845,7 +1846,6 @@ static void init_pl08x_debugfs(struct pl08x_driver_data *pl08x)
 	(void) debugfs_create_file(dev_name(&pl08x->adev->dev), S_IFREG | S_IRUGO,
 				   NULL, pl08x,
 				   &pl08x_debugfs_operations);
-	return 0;
 }
 
 #else
@@ -1872,6 +1872,28 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 		goto out_no_pl08x;
 	}
 
+	/* Initialize memcpy engine */
+	dma_cap_set(DMA_MEMCPY, pl08x->memcpy.cap_mask);
+	pl08x->memcpy.dev = &adev->dev;
+	pl08x->memcpy.device_alloc_chan_resources = pl08x_alloc_chan_resources;
+	pl08x->memcpy.device_free_chan_resources = pl08x_free_chan_resources;
+	pl08x->memcpy.device_prep_dma_memcpy = pl08x_prep_dma_memcpy;
+	pl08x->memcpy.device_prep_dma_interrupt = pl08x_prep_dma_interrupt;
+	pl08x->memcpy.device_tx_status = pl08x_dma_tx_status;
+	pl08x->memcpy.device_issue_pending = pl08x_issue_pending;
+	pl08x->memcpy.device_control = pl08x_control;
+
+	/* Initialize slave engine */
+	dma_cap_set(DMA_SLAVE, pl08x->slave.cap_mask);
+	pl08x->slave.dev = &adev->dev;
+	pl08x->slave.device_alloc_chan_resources = pl08x_alloc_chan_resources;
+	pl08x->slave.device_free_chan_resources = pl08x_free_chan_resources;
+	pl08x->slave.device_prep_dma_interrupt = pl08x_prep_dma_interrupt;
+	pl08x->slave.device_tx_status = pl08x_dma_tx_status;
+	pl08x->slave.device_issue_pending = pl08x_issue_pending;
+	pl08x->slave.device_prep_slave_sg = pl08x_prep_slave_sg;
+	pl08x->slave.device_control = pl08x_control;
+
 	/* Get the platform data */
 	pl08x->pd = dev_get_platdata(&adev->dev);
 	if (!pl08x->pd) {
@@ -1939,14 +1961,8 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 			 pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE");
 	}
 
-	/* Set caps */
-	dma_cap_set(DMA_MEMCPY, dmac_memcpy.cap_mask);
-	dma_cap_set(DMA_SLAVE, dmac_slave.cap_mask);
-	dmac_memcpy.dev = &adev->dev;
-	dmac_slave.dev = &adev->dev;
-
 	/* Register as many memcpy channels as there are physical channels */
-	ret = pl08x_dma_init_virtual_channels(pl08x, &dmac_memcpy,
+	ret = pl08x_dma_init_virtual_channels(pl08x, &pl08x->memcpy,
 					      pl08x->vd->channels, false);
 	if (ret <= 0) {
 		dev_warn(&pl08x->adev->dev,
@@ -1954,10 +1970,10 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 			 __func__, ret);
 		goto out_no_memcpy;
 	}
-	dmac_memcpy.chancnt = ret;
+	pl08x->memcpy.chancnt = ret;
 
 	/* Register slave channels */
-	ret = pl08x_dma_init_virtual_channels(pl08x, &dmac_slave,
+	ret = pl08x_dma_init_virtual_channels(pl08x, &pl08x->slave,
 					      pl08x->pd->num_slave_channels,
 					      true);
 	if (ret <= 0) {
@@ -1966,9 +1982,9 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 				__func__, ret);
 		goto out_no_slave;
 	}
-	dmac_slave.chancnt = ret;
+	pl08x->slave.chancnt = ret;
 
-	ret = dma_async_device_register(&dmac_memcpy);
+	ret = dma_async_device_register(&pl08x->memcpy);
 	if (ret) {
 		dev_warn(&pl08x->adev->dev,
 			"%s failed to register memcpy as an async device - %d\n",
@@ -1976,7 +1992,7 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 		goto out_no_memcpy_reg;
 	}
 
-	ret = dma_async_device_register(&dmac_slave);
+	ret = dma_async_device_register(&pl08x->slave);
 	if (ret) {
 		dev_warn(&pl08x->adev->dev,
 			"%s failed to register slave as an async device - %d\n",
@@ -1991,11 +2007,11 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 	return 0;
 
 out_no_slave_reg:
-	dma_async_device_unregister(&dmac_memcpy);
+	dma_async_device_unregister(&pl08x->memcpy);
 out_no_memcpy_reg:
-	/* FIXME: free slave channels */
+	pl08x_free_virtual_channels(&pl08x->slave);
 out_no_slave:
-	/* FIXME: free memcpy channels */
+	pl08x_free_virtual_channels(&pl08x->memcpy);
 out_no_memcpy:
 	kfree(pl08x->phy_chans);
 out_no_phychans:
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index 2bb7ac4..6db44f9 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -138,6 +138,7 @@ struct pl08x_txd {
  * @lock: a lock for this channel data
  * @host: a pointer to the host (internal use)
  * @paused: whether the channel is paused
+ * @slave: whether this channel is a device (slave) or for memcpy
  */
 struct pl08x_dma_chan {
 	struct dma_chan chan;
@@ -154,6 +155,7 @@ struct pl08x_dma_chan {
 	spinlock_t lock;
 	void *host;
 	bool paused;
+	bool slave;
 };
 
 /**
-- 
1.6.3.3

[PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait

[Linus Walleij]

This change makes the memcpy transfers wait for a physical channel
to become available if no free channel is available when the job
is submitted. When the first physical channel fires its tasklet,
it will spin over the memcpy channels to see if one of these is
waiting.

This is necessary to get the memcpy semantics right: the generic
memcpy API assume transfers never fail, and with our oversubscribed
physical channels this becomes a problem: sometimes submit would
fail. This fixes it by letting the memcpy channels pull a free
channel ASAP.

The slave channels shall however *fail* if no channel is available
since the device will then either fall back to some PIO mode or
retry.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c   |  299 ++++++++++++++++++++++++--------------------
 include/linux/amba/pl08x.h |    3 +
 2 files changed, 168 insertions(+), 134 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 4573189..49fb19d 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1018,134 +1018,6 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x,
 	}
 }
 
-static void pl08x_tasklet(unsigned long data)
-{
-	struct pl08x_dma_chan *plchan = (struct pl08x_dma_chan *) data;
-	struct pl08x_phy_chan *phychan = plchan->phychan;
-	struct pl08x_driver_data *pl08x = plchan->host;
-	unsigned long flags;
-
-	if (!plchan)
-		BUG();
-
-	spin_lock_irqsave(&plchan->lock, flags);
-
-	if (plchan->at) {
-		dma_async_tx_callback callback =
-			plchan->at->tx.callback;
-		void *callback_param =
-			plchan->at->tx.callback_param;
-
-		/*
-		 * Update last completed
-		 */
-		plchan->lc =
-			(plchan->at->tx.cookie);
-
-		/*
-		 * Callback to signal completion
-		 */
-		if (callback)
-			callback(callback_param);
-
-		/*
-		 * Device callbacks should NOT clear
-		 * the current transaction on the channel
-		 * Linus: sometimes they should?
-		 */
-		if (!plchan->at)
-			BUG();
-
-		/*
-		 * Free the descriptor if it's not for a device
-		 * using a circular buffer
-		 */
-		if (!plchan->at->cd->circular_buffer) {
-			pl08x_free_txd(pl08x, plchan->at);
-			plchan->at = NULL;
-		}
-		/*
-		 * else descriptor for circular
-		 * buffers only freed when
-		 * client has disabled dma
-		 */
-	}
-	/*
-	 * If a new descriptor is queued, set it up
-	 * plchan->at is NULL here
-	 */
-	if (!list_empty(&plchan->desc_list)) {
-		struct pl08x_txd *next;
-
-		next = list_first_entry(&plchan->desc_list,
-					struct pl08x_txd,
-					node);
-		list_del(&next->node);
-		plchan->at = next;
-		/* Configure the physical channel for the next txd */
-		pl08x_config_phychan_for_txd(plchan);
-		pl08x_set_cregs(pl08x, plchan->phychan);
-		pl08x_enable_phy_chan(pl08x, plchan->phychan);
-	} else {
-		/*
-		 * No more jobs, so free up the physical channel
-		 * Free any allocated signal on slave transfers too
-		 */
-		if ((phychan->signal >= 0) && pl08x->pd->put_signal) {
-			pl08x->pd->put_signal(plchan);
-			phychan->signal = -1;
-		}
-		pl08x_put_phy_channel(pl08x, phychan);
-		plchan->phychan = NULL;
-	}
-
-	spin_unlock_irqrestore(&plchan->lock, flags);
-}
-
-static irqreturn_t pl08x_irq(int irq, void *dev)
-{
-	struct pl08x_driver_data *pl08x = dev;
-	u32 mask = 0;
-	u32 val;
-	int i;
-
-	val = readl(pl08x->base + PL080_ERR_STATUS);
-	if (val) {
-		/*
-		 * An error interrupt (on one or more channels)
-		 */
-		dev_err(&pl08x->adev->dev,
-			"%s error interrupt, register value 0x%08x\n",
-				__func__, val);
-		/*
-		 * Simply clear ALL PL08X error interrupts,
-		 * regardless of channel and cause
-		 * FIXME: should be 0x00000003 on PL081 really.
-		 */
-		writel(0x000000FF, pl08x->base + PL080_ERR_CLEAR);
-	}
-	val = readl(pl08x->base + PL080_INT_STATUS);
-	for (i = 0; i < pl08x->vd->channels; i++) {
-		if ((1 << i) & val) {
-			/* Locate physical channel */
-			struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i];
-			struct pl08x_dma_chan *plchan = phychan->serving;
-
-			/* Schedule tasklet on this channel */
-			tasklet_schedule(&plchan->tasklet);
-
-			mask |= (1 << i);
-		}
-	}
-	/*
-	 * Clear only the terminal interrupts on channels we processed
-	 */
-	writel(mask, pl08x->base + PL080_TC_CLEAR);
-
-	return mask ? IRQ_HANDLED : IRQ_NONE;
-}
-
-
 /*
  * The DMA ENGINE API
  */
@@ -1265,12 +1137,20 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
 	 */
 	ret = prep_phy_channel(plchan, txd);
 	if (ret) {
-		/* No physical channel available, cope with it */
-		pl08x_free_txd_list(pl08x, plchan);
-		spin_unlock_irqrestore(&plchan->lock, flags);
-		return -EBUSY;
+		/*
+		 * No physical channel available, we will
+		 * stack up the memcpy channels until there is a channel
+		 * available to handle it whereas slave transfers cannot
+		 * wait and will simply be NACK:ed with -EBUSY.
+		 */
+		if (plchan->slave) {
+			/* No physical channel available, cope with it */
+			pl08x_free_txd_list(pl08x, plchan);
+			spin_unlock_irqrestore(&plchan->lock, flags);
+			return -EBUSY;
+		} else
+			plchan->waiting = txd;
 	}
-
 	spin_unlock_irqrestore(&plchan->lock, flags);
 
 	return tx->cookie;
@@ -1478,7 +1358,6 @@ static void pl08x_issue_pending(struct dma_chan *chan)
 	struct pl08x_driver_data *pl08x = plchan->host;
 	unsigned long flags;
 
-
 	spin_lock_irqsave(&plchan->lock, flags);
 	/* Something is already active */
 	if (plchan->at) {
@@ -1486,6 +1365,10 @@ static void pl08x_issue_pending(struct dma_chan *chan)
 			return;
 	}
 
+	/* Didn't get a physical channel so waiting for it ... */
+	if (plchan->waiting)
+		return;
+
 	/* Take the first element in the queue and execute it */
 	if (!list_empty(&plchan->desc_list)) {
 		struct pl08x_txd *next;
@@ -1713,6 +1596,154 @@ static void pl08x_ensure_on(struct pl08x_driver_data *pl08x)
 	writel(val, pl08x->base + PL080_CONFIG);
 }
 
+static void pl08x_tasklet(unsigned long data)
+{
+	struct pl08x_dma_chan *plchan = (struct pl08x_dma_chan *) data;
+	struct pl08x_phy_chan *phychan = plchan->phychan;
+	struct pl08x_driver_data *pl08x = plchan->host;
+
+	if (!plchan)
+		BUG();
+
+	spin_lock(&plchan->lock);
+
+	if (plchan->at) {
+		dma_async_tx_callback callback =
+			plchan->at->tx.callback;
+		void *callback_param =
+			plchan->at->tx.callback_param;
+
+		/*
+		 * Update last completed
+		 */
+		plchan->lc =
+			(plchan->at->tx.cookie);
+
+		/*
+		 * Callback to signal completion
+		 */
+		if (callback)
+			callback(callback_param);
+
+		/*
+		 * Device callbacks should NOT clear
+		 * the current transaction on the channel
+		 * Linus: sometimes they should?
+		 */
+		if (!plchan->at)
+			BUG();
+
+		/*
+		 * Free the descriptor if it's not for a device
+		 * using a circular buffer
+		 */
+		if (!plchan->at->cd->circular_buffer) {
+			pl08x_free_txd(pl08x, plchan->at);
+			plchan->at = NULL;
+		}
+		/*
+		 * else descriptor for circular
+		 * buffers only freed when
+		 * client has disabled dma
+		 */
+	}
+	/*
+	 * If a new descriptor is queued, set it up
+	 * plchan->at is NULL here
+	 */
+	if (!list_empty(&plchan->desc_list)) {
+		struct pl08x_txd *next;
+
+		next = list_first_entry(&plchan->desc_list,
+					struct pl08x_txd,
+					node);
+		list_del(&next->node);
+		plchan->at = next;
+		/* Configure the physical channel for the next txd */
+		pl08x_config_phychan_for_txd(plchan);
+		pl08x_set_cregs(pl08x, plchan->phychan);
+		pl08x_enable_phy_chan(pl08x, plchan->phychan);
+	} else {
+		struct pl08x_dma_chan *waiting = NULL;
+
+		/*
+		 * No more jobs, so free up the physical channel
+		 * Free any allocated signal on slave transfers too
+		 */
+		if ((phychan->signal >= 0) && pl08x->pd->put_signal) {
+			pl08x->pd->put_signal(plchan);
+			phychan->signal = -1;
+		}
+		pl08x_put_phy_channel(pl08x, phychan);
+		plchan->phychan = NULL;
+
+		/*
+		 * And NOW before anyone else can grab that free:d
+		 * up physical channel, see if there is some memcpy
+		 * pending that seriously needs to start because of
+		 * being stacked up while we were choking the
+		 * physical channels with data.
+		 */
+		list_for_each_entry(waiting, &pl08x->memcpy.channels, chan.device_node) {
+			if (waiting->waiting) {
+				int ret;
+
+				/* This should REALLY not fail now */
+				ret = prep_phy_channel(waiting, waiting->waiting);
+				BUG_ON(ret);
+				waiting->waiting = NULL;
+				pl08x_issue_pending(&waiting->chan);
+				break;
+			}
+		}
+	}
+
+	spin_unlock(&plchan->lock);
+}
+
+static irqreturn_t pl08x_irq(int irq, void *dev)
+{
+	struct pl08x_driver_data *pl08x = dev;
+	u32 mask = 0;
+	u32 val;
+	int i;
+
+	val = readl(pl08x->base + PL080_ERR_STATUS);
+	if (val) {
+		/*
+		 * An error interrupt (on one or more channels)
+		 */
+		dev_err(&pl08x->adev->dev,
+			"%s error interrupt, register value 0x%08x\n",
+				__func__, val);
+		/*
+		 * Simply clear ALL PL08X error interrupts,
+		 * regardless of channel and cause
+		 * FIXME: should be 0x00000003 on PL081 really.
+		 */
+		writel(0x000000FF, pl08x->base + PL080_ERR_CLEAR);
+	}
+	val = readl(pl08x->base + PL080_INT_STATUS);
+	for (i = 0; i < pl08x->vd->channels; i++) {
+		if ((1 << i) & val) {
+			/* Locate physical channel */
+			struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i];
+			struct pl08x_dma_chan *plchan = phychan->serving;
+
+			/* Schedule tasklet on this channel */
+			tasklet_schedule(&plchan->tasklet);
+
+			mask |= (1 << i);
+		}
+	}
+	/*
+	 * Clear only the terminal interrupts on channels we processed
+	 */
+	writel(mask, pl08x->base + PL080_TC_CLEAR);
+
+	return mask ? IRQ_HANDLED : IRQ_NONE;
+}
+
 /*
  * Initialise the DMAC memcpy/slave channels.
  * Make a local wrapper to hold required data
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index 6db44f9..f461648 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -139,6 +139,8 @@ struct pl08x_txd {
  * @host: a pointer to the host (internal use)
  * @paused: whether the channel is paused
  * @slave: whether this channel is a device (slave) or for memcpy
+ * @waiting: a TX descriptor on this channel which is waiting for
+ * a physical channel to become available
  */
 struct pl08x_dma_chan {
 	struct dma_chan chan;
@@ -156,6 +158,7 @@ struct pl08x_dma_chan {
 	void *host;
 	bool paused;
 	bool slave;
+	struct pl08x_txd *waiting;
 };
 
 /**
-- 
1.6.3.3

[PATCH 12/12] DMAENGINE: define channel states for the PL08X

[Linus Walleij]

Instead of strange things like a bool indicating paused state for
channels, let's define a proper channel state and use that. Also
print it out in the debugfs so we get a nice overview of the
channels and states.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c   |   70 ++++++++++++++++++++++++++++++++++----------
 include/linux/amba/pl08x.h |   22 ++++++++++++-
 2 files changed, 74 insertions(+), 18 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 49fb19d..1718b2e 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1148,9 +1148,20 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
 			pl08x_free_txd_list(pl08x, plchan);
 			spin_unlock_irqrestore(&plchan->lock, flags);
 			return -EBUSY;
-		} else
+		} else {
+			plchan->state = PL08X_CHAN_WAITING;
 			plchan->waiting = txd;
-	}
+		}
+	} else
+		/*
+		 * Else we're all set, paused and ready to roll,
+		 * status will switch to PL08X_CHAN_RUNNING when
+		 * we call issue_pending(). If there is something
+		 * running on the channel already we don't change
+		 * its state.
+		 */
+		if (plchan->state == PL08X_CHAN_IDLE)
+			plchan->state = PL08X_CHAN_PAUSED;
 	spin_unlock_irqrestore(&plchan->lock, flags);
 
 	return tx->cookie;
@@ -1206,9 +1217,10 @@ pl08x_dma_tx_status(struct dma_chan *chan,
 	dma_set_tx_state(txstate, last_complete, last_used,
 			 bytesleft);
 
-	if (plchan->paused)
+	if (plchan->state == PL08X_CHAN_PAUSED)
 		return DMA_PAUSED;
 
+	/* Whether waiting or running, we're in progress */
 	return DMA_IN_PROGRESS;
 }
 
@@ -1366,7 +1378,7 @@ static void pl08x_issue_pending(struct dma_chan *chan)
 	}
 
 	/* Didn't get a physical channel so waiting for it ... */
-	if (plchan->waiting)
+	if (plchan->state == PL08X_CHAN_WAITING)
 		return;
 
 	/* Take the first element in the queue and execute it */
@@ -1378,6 +1390,7 @@ static void pl08x_issue_pending(struct dma_chan *chan)
 					node);
 		list_del(&next->node);
 		plchan->at = next;
+		plchan->state = PL08X_CHAN_RUNNING;
 
 		/* Configure the physical channel for the active txd */
 		pl08x_config_phychan_for_txd(plchan);
@@ -1520,7 +1533,7 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 
 	switch (cmd) {
 	case DMA_TERMINATE_ALL:
-		plchan->paused = false;
+		plchan->state = PL08X_CHAN_IDLE;
 
 		if (plchan->phychan) {
 			pl08x_stop_phy_chan(plchan->phychan);
@@ -1549,11 +1562,11 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 		break;
 	case DMA_PAUSE:
 		pl08x_pause_phy_chan(plchan->phychan);
-		plchan->paused = true;
+		plchan->state = PL08X_CHAN_PAUSED;
 		break;
 	case DMA_RESUME:
 		pl08x_resume_phy_chan(plchan->phychan);
-		plchan->paused = false;
+		plchan->state = PL08X_CHAN_RUNNING;
 		break;
 	default:
 		/* Unknown command */
@@ -1676,6 +1689,7 @@ static void pl08x_tasklet(unsigned long data)
 		}
 		pl08x_put_phy_channel(pl08x, phychan);
 		plchan->phychan = NULL;
+		plchan->state = PL08X_CHAN_IDLE;
 
 		/*
 		 * And NOW before anyone else can grab that free:d
@@ -1684,13 +1698,17 @@ static void pl08x_tasklet(unsigned long data)
 		 * being stacked up while we were choking the
 		 * physical channels with data.
 		 */
-		list_for_each_entry(waiting, &pl08x->memcpy.channels, chan.device_node) {
-			if (waiting->waiting) {
+		list_for_each_entry(waiting, &pl08x->memcpy.channels,
+				    chan.device_node) {
+		  if (waiting->state == PL08X_CHAN_WAITING &&
+			    waiting->waiting != NULL) {
 				int ret;
 
 				/* This should REALLY not fail now */
-				ret = prep_phy_channel(waiting, waiting->waiting);
+				ret = prep_phy_channel(waiting,
+						       waiting->waiting);
 				BUG_ON(ret);
+				waiting->state = PL08X_CHAN_RUNNING;
 				waiting->waiting = NULL;
 				pl08x_issue_pending(&waiting->chan);
 				break;
@@ -1771,6 +1789,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 		}
 
 		chan->host = pl08x;
+		chan->state = PL08X_CHAN_IDLE;
 
 		if (slave) {
 			chan->slave = true;
@@ -1817,6 +1836,23 @@ static void pl08x_free_virtual_channels(struct dma_device *dmadev)
 }
 
 #ifdef CONFIG_DEBUG_FS
+static const char *pl08x_state_str(enum pl08x_dma_chan_state state)
+{
+	switch(state) {
+	case PL08X_CHAN_IDLE:
+		return "idle";
+	case PL08X_CHAN_RUNNING:
+		return "running";
+	case PL08X_CHAN_PAUSED:
+		return "paused";
+	case PL08X_CHAN_WAITING:
+		return "waiting";
+	default:
+		break;
+	}
+	return "UNKNOWN STATE";
+}
+
 static int pl08x_debugfs_show(struct seq_file *s, void *data)
 {
 	struct pl08x_driver_data *pl08x = s->private;
@@ -1843,17 +1879,19 @@ static int pl08x_debugfs_show(struct seq_file *s, void *data)
 	}
 
 	seq_printf(s, "\nPL08x virtual memcpy channels:\n");
-	seq_printf(s, "CHANNEL:\n");
-	seq_printf(s, "--------\n");
+	seq_printf(s, "CHANNEL:\tSTATE:\n");
+	seq_printf(s, "--------\t------\n");
 	list_for_each_entry(chan, &pl08x->memcpy.channels, chan.device_node) {
-		seq_printf(s, "%s\n", chan->name);
+		seq_printf(s, "%s\t\t\%s\n", chan->name,
+			   pl08x_state_str(chan->state));
 	}
 
 	seq_printf(s, "\nPL08x virtual slave channels:\n");
-	seq_printf(s, "CHANNEL:\n");
-	seq_printf(s, "--------\n");
+	seq_printf(s, "CHANNEL:\tSTATE:\n");
+	seq_printf(s, "--------\t------\n");
 	list_for_each_entry(chan, &pl08x->slave.channels, chan.device_node) {
-		seq_printf(s, "%s\n", chan->name);
+		seq_printf(s, "%s\t\t\%s\n", chan->name,
+			   pl08x_state_str(chan->state));
 	}
 
 	return 0;
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index f461648..d54fbff 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -123,6 +123,24 @@ struct pl08x_txd {
 };
 
 /**
+ * struct pl08x_dma_chan_state - holds the PL08x specific virtual
+ * channel states
+ * @PL08X_CHAN_IDLE: the channel is idle
+ * @PL08X_CHAN_RUNNING: the channel has allocated a physical transport
+ * channel and is running a transfer on it
+ * @PL08X_CHAN_PAUSED: the channel has allocated a physical transport
+ * channel, but the transfer is currently paused
+ * @PL08X_CHAN_WAITING: the channel is waiting for a physical transport
+ * channel to become available (only pertains to memcpy channels)
+ */
+enum pl08x_dma_chan_state {
+	PL08X_CHAN_IDLE,
+	PL08X_CHAN_RUNNING,
+	PL08X_CHAN_PAUSED,
+	PL08X_CHAN_WAITING,
+};
+
+/**
  * struct pl08x_dma_chan - this structure wraps a DMA ENGINE channel
  * @chan: wrappped abstract channel
  * @phychan: the physical channel utilized by this channel, if there is one
@@ -137,7 +155,7 @@ struct pl08x_txd {
  * @at: active transaction on this channel
  * @lock: a lock for this channel data
  * @host: a pointer to the host (internal use)
- * @paused: whether the channel is paused
+ * @state: whether the channel is idle, paused, running etc
  * @slave: whether this channel is a device (slave) or for memcpy
  * @waiting: a TX descriptor on this channel which is waiting for
  * a physical channel to become available
@@ -156,7 +174,7 @@ struct pl08x_dma_chan {
 	struct pl08x_txd *at;
 	spinlock_t lock;
 	void *host;
-	bool paused;
+	enum pl08x_dma_chan_state state;
 	bool slave;
 	struct pl08x_txd *waiting;
 };
-- 
1.6.3.3

Re: [00/12] DMAENGINE: PL08X driver updates

[Linus Walleij]

2010/8/31 Linus Walleij <linus.walleij@stericsson.com>:

> This updates the PL08X driver in the async_tx next branch with
> a number of fixes and features, each one pretty self-describing.
> This has been tested with the PrimeCell DMA series on the
> RealView PB11MPCore and works flawlessly, multiplexing
> several virtual clients, both devices (slaves) and memcpy
> channels, across only two (!) available physical channels.

Dan can you ACK and queue this for next?

I know you're busy, just a reminder...

Yours,
Linus Walleij

Re: [00/12] DMAENGINE: PL08X driver updates

[Dan Williams]

On Wed, Sep 8, 2010 at 7:26 AM, Linus Walleij
<linus.ml.walleij@gmail.com> wrote:
> 2010/8/31 Linus Walleij <linus.walleij@stericsson.com>:
>
>> This updates the PL08X driver in the async_tx next branch with
>> a number of fixes and features, each one pretty self-describing.
>> This has been tested with the PrimeCell DMA series on the
>> RealView PB11MPCore and works flawlessly, multiplexing
>> several virtual clients, both devices (slaves) and memcpy
>> channels, across only two (!) available physical channels.
>
> Dan can you ACK and queue this for next?
>
> I know you're busy, just a reminder...
>

I'm back at work this coming week and will get all this queued up.

--
Dan

Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait

[Dan Williams]

On Tue, Aug 31, 2010 at 5:12 AM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> This change makes the memcpy transfers wait for a physical channel
> to become available if no free channel is available when the job
> is submitted. When the first physical channel fires its tasklet,
> it will spin over the memcpy channels to see if one of these is
> waiting.
>
> This is necessary to get the memcpy semantics right: the generic
> memcpy API assume transfers never fail, and with our oversubscribed
> physical channels this becomes a problem: sometimes submit would
> fail. This fixes it by letting the memcpy channels pull a free
> channel ASAP.
>
> The slave channels shall however *fail* if no channel is available
> since the device will then either fall back to some PIO mode or
> retry.
>

This patch does not sit right with me.  It seems a bit arbitrary that
memcpy operations will be queued while slave operations are failed.
Is there anyway to know at prep time whether a subsequent submit will
fail?  Are there any cases where a slave might want its operation
queued?

The prep routine is meant to guarantee that all the resources for a
transaction have been acquired.  The only reason ->tx_submit() has a
return value is to support the net_dma usage model that uses opaque
cookies for tracking transactions.

If we make tx_submit() fallable we should go back and ensure that all
usages are prepared to handle failure.

--
Dan

sidenote: this driver needs to be converted to a dma descriptor pool
(allocated at device_alloc_chan_resources time), or at a minimum use
GFP_NOWAIT in device_prep_dma_memcpy because that routine may be
called from atomic contexts.

Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait

[Linus Walleij]

2010/9/23 Dan Williams <dan.j.williams@intel.com>:
> On Tue, Aug 31, 2010 at 5:12 AM, Linus Walleij
> <linus.walleij@stericsson.com> wrote:
>> This change makes the memcpy transfers wait for a physical channel
>> to become available if no free channel is available when the job
>> is submitted. When the first physical channel fires its tasklet,
>> it will spin over the memcpy channels to see if one of these is
>> waiting.
>>
>> This is necessary to get the memcpy semantics right: the generic
>> memcpy API assume transfers never fail, and with our oversubscribed
>> physical channels this becomes a problem: sometimes submit would
>> fail. This fixes it by letting the memcpy channels pull a free
>> channel ASAP.
>>
>> The slave channels shall however *fail* if no channel is available
>> since the device will then either fall back to some PIO mode or
>> retry.
>>
>
> This patch does not sit right with me.  It seems a bit arbitrary that
> memcpy operations will be queued while slave operations are failed.

I was sort of suspecting a discussion around this...

Background: the PL081 that I'm testing on only has *two* physical
channels, they can be used for memcpy, slave transfers (RX/TX).

My first option was to set one channel aside for memcpy and
one for dev xfers and be done with it.

But that would be devastating if
I was only using them for memcpy or only devxfer, since it would
slash performance in half. So I decided to let memcpy and
dev xfers battle for the channels with oversubscription and exposing
two virtual memcpy channels.

Whereas the slave drivers I've written are prepared to handle the
case where a transfer fails gracefully (and not treat it as an error)
and fall back to IRQ/PIO mode, the memcpy tests in
drivers/dma/dmatest.c does treat a failure to prep() or submit()
as an error.

So for this reason I'm queueing memcpy requests until
there is a channel ready.

Are you suggesting I should rewrite the memcpy tests instead
so they gracefully handle a failed prep() call, not treat it as an
error and either:

- retry or
- copy with a regular klibc memcpy() call?

> Is there anyway to know at prep time whether a subsequent submit will
> fail?  Are there any cases where a slave might want its operation
> queued?
>
> The prep routine is meant to guarantee that all the resources for a
> transaction have been acquired.  The only reason ->tx_submit() has a
> return value is to support the net_dma usage model that uses opaque
> cookies for tracking transactions.

It is possible to do this at prep() time. However that assumes that every
device transfer has code that executes this sequence:

.device_prep_slave_sg()
.tx_submit()
.device_issue_pending()

In direct succession. If there is an arbitrary delay between prep()
and submit() (where I currently fail xfers) the physical channels
may starve if prep() allocates them.

If I can safely assume that prep() and .tx_submit() are in quick
succession, I can reserve the physical channel at prep() time.

This seems to be the case in current code where only quick
things like setting a callback etc is done between prep() and
.tx_submit().

So I'll spin the PL08x around to reserve channels on
prep() instead.

(By the way: we should rename .tx_submit() to just .submit()
since in device context this can just as well be RX!)

> If we make tx_submit() fallable we should go back and ensure that all
> usages are prepared to handle failure.

OK I've implemented failure path for tx_submit() in all my
drivers but I also have it for prep() of course. I hope I can keep
that code... I'll take out the comments about allocation failure
and move them to the prep() calls though.

Yours,
Linus Walleij

Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait

[Linus Walleij]

2010/9/23 Dan Williams <dan.j.williams@intel.com>:

> The prep routine is meant to guarantee that all the resources for a
> transaction have been acquired.

A practicality here:

The assumption to fail for physical channel allocation at .tx_submit()
is already in the first patch adding the driver.

Should I squash all the icremental improvements into the original
driver, convert to fail at .prep() and resubmit to make all of this
easier, or do you want me to refactor the patch stack, or refactor
from patch 11/12 or...?

Yours,
Linus Walleij

Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait

[Dan Williams]

On Thu, Sep 23, 2010 at 12:39 AM, Linus Walleij
<linus.ml.walleij@gmail.com> wrote:
> 2010/9/23 Dan Williams <dan.j.williams@intel.com>:
>> On Tue, Aug 31, 2010 at 5:12 AM, Linus Walleij
>> <linus.walleij@stericsson.com> wrote:
>>> This change makes the memcpy transfers wait for a physical channel
>>> to become available if no free channel is available when the job
>>> is submitted. When the first physical channel fires its tasklet,
>>> it will spin over the memcpy channels to see if one of these is
>>> waiting.
>>>
>>> This is necessary to get the memcpy semantics right: the generic
>>> memcpy API assume transfers never fail, and with our oversubscribed
>>> physical channels this becomes a problem: sometimes submit would
>>> fail. This fixes it by letting the memcpy channels pull a free
>>> channel ASAP.
>>>
>>> The slave channels shall however *fail* if no channel is available
>>> since the device will then either fall back to some PIO mode or
>>> retry.
>>>
>>
>> This patch does not sit right with me.  It seems a bit arbitrary that
>> memcpy operations will be queued while slave operations are failed.
>
> I was sort of suspecting a discussion around this...
>
> Background: the PL081 that I'm testing on only has *two* physical
> channels, they can be used for memcpy, slave transfers (RX/TX).
>
> My first option was to set one channel aside for memcpy and
> one for dev xfers and be done with it.
>
> But that would be devastating if
> I was only using them for memcpy or only devxfer, since it would
> slash performance in half. So I decided to let memcpy and
> dev xfers battle for the channels with oversubscription and exposing
> two virtual memcpy channels.
>
> Whereas the slave drivers I've written are prepared to handle the
> case where a transfer fails gracefully (and not treat it as an error)
> and fall back to IRQ/PIO mode, the memcpy tests in
> drivers/dma/dmatest.c does treat a failure to prep() or submit()
> as an error.

Yeah, that is a hold over from when the original ioatdma driver
performed memory allocations at submit time (fixed here a0587bcf).

>
> So for this reason I'm queueing memcpy requests until
> there is a channel ready.
>
> Are you suggesting I should rewrite the memcpy tests instead
> so they gracefully handle a failed prep() call, not treat it as an
> error and either:
>
> - retry or
> - copy with a regular klibc memcpy() call?

I was more thinking that at a minimum all slave drivers would follow
the same semantic and expect their submit requests to be queued or
fail, as it stands we have a mix of both across all slave
implementations.

>
>> Is there anyway to know at prep time whether a subsequent submit will
>> fail?  Are there any cases where a slave might want its operation
>> queued?
>>
>> The prep routine is meant to guarantee that all the resources for a
>> transaction have been acquired.  The only reason ->tx_submit() has a
>> return value is to support the net_dma usage model that uses opaque
>> cookies for tracking transactions.
>
> It is possible to do this at prep() time. However that assumes that every
> device transfer has code that executes this sequence:
>
> .device_prep_slave_sg()
> .tx_submit()
> .device_issue_pending()
>
> In direct succession. If there is an arbitrary delay between prep()
> and submit() (where I currently fail xfers) the physical channels
> may starve if prep() allocates them.

There is precedent, albeit a bit inelegant, for taking a lock in
prep() and dropping it in submit().  The ioatdma driver does this to
ensure in-order submission because the hardware caches the address of
the next descriptor in the ring.  Out-of-order submission would
require a hardware reset to clear that cache prep() and submit() are
not paired.

> If I can safely assume that prep() and .tx_submit() are in quick
> succession, I can reserve the physical channel at prep() time.
>

It is safe to actively enforce this as long as you never plan to
support the async_tx channel switching capability (only a few raid
drivers use this presently).  I'm converting to be an explicit opt-in
because most drivers don't need this and can use the cut down version
of dma_async_tx_descriptor.

> This seems to be the case in current code where only quick
> things like setting a callback etc is done between prep() and
> .tx_submit().
>
> So I'll spin the PL08x around to reserve channels on
> prep() instead.
>
> (By the way: we should rename .tx_submit() to just .submit()
> since in device context this can just as well be RX!)

The 'tx' in async_tx came "asynchronous transfer/transform"  where the
'x' is transfer or transform.  I'd also like to change drop the
'device_' from the method names and rename dma_async_tx_descriptor to
something shorter, but I'm not sure it is worth the thrash.

>> If we make tx_submit() fallable we should go back and ensure that all
>> usages are prepared to handle failure.
>
> OK I've implemented failure path for tx_submit() in all my
> drivers but I also have it for prep() of course. I hope I can keep
> that code... I'll take out the comments about allocation failure
> and move them to the prep() calls though.
>

Sounds good.

--
Dan

Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait

[Dan Williams]

On Thu, Sep 23, 2010 at 12:46 AM, Linus Walleij
<linus.ml.walleij@gmail.com> wrote:
> 2010/9/23 Dan Williams <dan.j.williams@intel.com>:
>
>> The prep routine is meant to guarantee that all the resources for a
>> transaction have been acquired.
>
> A practicality here:
>
> The assumption to fail for physical channel allocation at .tx_submit()
> is already in the first patch adding the driver.
>
> Should I squash all the icremental improvements into the original
> driver, convert to fail at .prep() and resubmit to make all of this
> easier, or do you want me to refactor the patch stack, or refactor
> from patch 11/12 or...?
>

At this point I am already rebasing my next branch to catch up with
the backlog, so a new v5 of the driver with all the roll-ups works for
me.

Also not sure if you caught this comment from earlier:
> sidenote: this driver needs to be converted to a dma descriptor pool
> (allocated at device_alloc_chan_resources time), or at a minimum use
> GFP_NOWAIT in device_prep_dma_memcpy because that routine may be
> called from atomic contexts.

Thanks,
Dan