[RFC] pl08x: don't use dma_slave_config direction argument

[RFC] pl08x: don't use dma_slave_config direction argument

[Russell King - ARM Linux]

This series removes the dependence on the dma_slave_config direction
argument for the PL08x DMA engine driver, and in doing so, we end up
with less code in the driver.

We now compute the cctl values for both directions, and continue to
select the appropriate one at prepare time.  If this is found to be
invalid, the prepare function will return NULL.

However, we still use the direction argument in the slave configuration
call to determine whether we should report and fail an invalid
configuration.  Eventually this will be removed.

* This has only been compile tested.  Test feedback welcomed. *

 drivers/dma/amba-pl08x.c   |  196 +++++++++++++++++++++++++++++---------------
 include/linux/amba/pl08x.h |   84 +------------------
 2 files changed, 134 insertions(+), 146 deletions(-)

[PATCH 1/6] DMA: PL08x: move private data structures into amba-pl08x.c

[Russell King]

Move the driver private data structures into the driver itself, rather
than having them exposed to everyone in a header file.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/dma/amba-pl08x.c   |   72 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/pl08x.h |   74 +------------------------------------------
 2 files changed, 74 insertions(+), 72 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 3d704ab..492160b 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -90,6 +90,7 @@
 #define DRIVER_NAME	"pl08xdmac"
 
 static struct amba_driver pl08x_amba_driver;
+struct pl08x_driver_data;
 
 /**
  * struct vendor_data - vendor-specific config parameters for PL08x derivatives
@@ -115,6 +116,77 @@ struct pl08x_lli {
 };
 
 /**
+ * struct pl08x_bus_data - information of source or destination
+ * busses for a transfer
+ * @addr: current address
+ * @maxwidth: the maximum width of a transfer on this bus
+ * @buswidth: the width of this bus in bytes: 1, 2 or 4
+ */
+struct pl08x_bus_data {
+	dma_addr_t addr;
+	u8 maxwidth;
+	u8 buswidth;
+};
+
+/**
+ * struct pl08x_phy_chan - holder for the physical channels
+ * @id: physical index to this channel
+ * @lock: a lock to use when altering an instance of this struct
+ * @signal: the physical signal (aka channel) serving this physical channel
+ * right now
+ * @serving: the virtual channel currently being served by this physical
+ * channel
+ */
+struct pl08x_phy_chan {
+	unsigned int id;
+	void __iomem *base;
+	spinlock_t lock;
+	int signal;
+	struct pl08x_dma_chan *serving;
+};
+
+/**
+ * struct pl08x_sg - structure containing data per sg
+ * @src_addr: src address of sg
+ * @dst_addr: dst address of sg
+ * @len: transfer len in bytes
+ * @node: node for txd's dsg_list
+ */
+struct pl08x_sg {
+	dma_addr_t src_addr;
+	dma_addr_t dst_addr;
+	size_t len;
+	struct list_head node;
+};
+
+/**
+ * struct pl08x_txd - wrapper for struct dma_async_tx_descriptor
+ * @tx: async tx descriptor
+ * @node: node for txd list for channels
+ * @dsg_list: list of children sg's
+ * @direction: direction of transfer
+ * @llis_bus: DMA memory address (physical) start for the LLIs
+ * @llis_va: virtual memory address start for the LLIs
+ * @cctl: control reg values for current txd
+ * @ccfg: config reg values for current txd
+ */
+struct pl08x_txd {
+	struct dma_async_tx_descriptor tx;
+	struct list_head node;
+	struct list_head dsg_list;
+	enum dma_transfer_direction direction;
+	dma_addr_t llis_bus;
+	struct pl08x_lli *llis_va;
+	/* Default cctl value for LLIs */
+	u32 cctl;
+	/*
+	 * Settings to be put into the physical channel when we
+	 * trigger this txd.  Other registers are in llis_va[0].
+	 */
+	u32 ccfg;
+};
+
+/**
  * struct pl08x_driver_data - the local state holder for the PL08x
  * @slave: slave engine for this instance
  * @memcpy: memcpy engine for this instance
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index e64ce2c..ab3d846 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -21,8 +21,9 @@
 #include <linux/dmaengine.h>
 #include <linux/interrupt.h>
 
-struct pl08x_lli;
 struct pl08x_driver_data;
+struct pl08x_phy_chan;
+struct pl08x_txd;
 
 /* Bitmasks for selecting AHB ports for DMA transfers */
 enum {
@@ -72,77 +73,6 @@ struct pl08x_channel_data {
 };
 
 /**
- * Struct pl08x_bus_data - information of source or destination
- * busses for a transfer
- * @addr: current address
- * @maxwidth: the maximum width of a transfer on this bus
- * @buswidth: the width of this bus in bytes: 1, 2 or 4
- */
-struct pl08x_bus_data {
-	dma_addr_t addr;
-	u8 maxwidth;
-	u8 buswidth;
-};
-
-/**
- * struct pl08x_phy_chan - holder for the physical channels
- * @id: physical index to this channel
- * @lock: a lock to use when altering an instance of this struct
- * @signal: the physical signal (aka channel) serving this physical channel
- * right now
- * @serving: the virtual channel currently being served by this physical
- * channel
- */
-struct pl08x_phy_chan {
-	unsigned int id;
-	void __iomem *base;
-	spinlock_t lock;
-	int signal;
-	struct pl08x_dma_chan *serving;
-};
-
-/**
- * struct pl08x_sg - structure containing data per sg
- * @src_addr: src address of sg
- * @dst_addr: dst address of sg
- * @len: transfer len in bytes
- * @node: node for txd's dsg_list
- */
-struct pl08x_sg {
-	dma_addr_t src_addr;
-	dma_addr_t dst_addr;
-	size_t len;
-	struct list_head node;
-};
-
-/**
- * struct pl08x_txd - wrapper for struct dma_async_tx_descriptor
- * @tx: async tx descriptor
- * @node: node for txd list for channels
- * @dsg_list: list of children sg's
- * @direction: direction of transfer
- * @llis_bus: DMA memory address (physical) start for the LLIs
- * @llis_va: virtual memory address start for the LLIs
- * @cctl: control reg values for current txd
- * @ccfg: config reg values for current txd
- */
-struct pl08x_txd {
-	struct dma_async_tx_descriptor tx;
-	struct list_head node;
-	struct list_head dsg_list;
-	enum dma_transfer_direction direction;
-	dma_addr_t llis_bus;
-	struct pl08x_lli *llis_va;
-	/* Default cctl value for LLIs */
-	u32 cctl;
-	/*
-	 * Settings to be put into the physical channel when we
-	 * trigger this txd.  Other registers are in llis_va[0].
-	 */
-	u32 ccfg;
-};
-
-/**
  * struct pl08x_dma_chan_state - holds the PL08x specific virtual channel
  * states
  * @PL08X_CHAN_IDLE: the channel is idle
-- 
1.7.4.4

[PATCH 1/6] DMA: PL08x: move private data structures into amba-pl08x.c

[Linus Walleij]

On Wed, May 16, 2012 at 1:05 PM, Russell King
<rmk+kernel@arm.linux.org.uk> wrote:

> Move the driver private data structures into the driver itself, rather
> than having them exposed to everyone in a header file.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

I applied this series (it needed a trvial fix for the Nomadik variant support
which is in Vinod's tree), then ran memcpy tests on the NHK S8815,
and it works like a charm.

So FWIW:
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 2/6] DMA: PL08x: get src/dst addr direct from dma_slave_config struct

[Russell King]

Add a dma_slave_config struct to struct pl08x_dma_chan, and move the
src_addr/dst_addr arguments into this struct.  This is a step away
from using the dma_slave_config's direction member.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/dma/amba-pl08x.c   |   12 ++++++------
 include/linux/amba/pl08x.h |    3 +--
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 492160b..81f47cfa 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1181,6 +1181,8 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 		return -EINVAL;
 	}
 
+	plchan->cfg = *config;
+
 	cctl |= width << PL080_CONTROL_SWIDTH_SHIFT;
 	cctl |= width << PL080_CONTROL_DWIDTH_SHIFT;
 
@@ -1199,12 +1201,10 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 	plchan->device_fc = config->device_fc;
 
 	if (plchan->runtime_direction == DMA_DEV_TO_MEM) {
-		plchan->src_addr = config->src_addr;
 		plchan->src_cctl = pl08x_cctl(cctl) | PL080_CONTROL_DST_INCR |
 			pl08x_select_bus(plchan->cd->periph_buses,
 					 pl08x->mem_buses);
 	} else {
-		plchan->dst_addr = config->dst_addr;
 		plchan->dst_cctl = pl08x_cctl(cctl) | PL080_CONTROL_SRC_INCR |
 			pl08x_select_bus(pl08x->mem_buses,
 					 plchan->cd->periph_buses);
@@ -1418,10 +1418,10 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 
 	if (direction == DMA_MEM_TO_DEV) {
 		txd->cctl = plchan->dst_cctl;
-		slave_addr = plchan->dst_addr;
+		slave_addr = plchan->cfg.dst_addr;
 	} else if (direction == DMA_DEV_TO_MEM) {
 		txd->cctl = plchan->src_cctl;
-		slave_addr = plchan->src_addr;
+		slave_addr = plchan->cfg.src_addr;
 	} else {
 		pl08x_free_txd(pl08x, txd);
 		dev_err(&pl08x->adev->dev,
@@ -1723,8 +1723,8 @@ static void pl08x_dma_slave_init(struct pl08x_dma_chan *chan)
 
 	chan->slave = true;
 	chan->name = chan->cd->bus_id;
-	chan->src_addr = chan->cd->addr;
-	chan->dst_addr = chan->cd->addr;
+	chan->cfg.src_addr = chan->cd->addr;
+	chan->cfg.dst_addr = chan->cd->addr;
 	chan->src_cctl = cctl | PL080_CONTROL_DST_INCR |
 		pl08x_select_bus(chan->cd->periph_buses, chan->host->mem_buses);
 	chan->dst_cctl = cctl | PL080_CONTROL_SRC_INCR |
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index ab3d846..b47b3c2 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -121,8 +121,7 @@ struct pl08x_dma_chan {
 	struct tasklet_struct tasklet;
 	char *name;
 	const struct pl08x_channel_data *cd;
-	dma_addr_t src_addr;
-	dma_addr_t dst_addr;
+	struct dma_slave_config cfg;
 	u32 src_cctl;
 	u32 dst_cctl;
 	enum dma_transfer_direction runtime_direction;
-- 
1.7.4.4

[PATCH 3/6] DMA: PL08x: get rid of device_fc in struct pl08x_dma_chan

[Russell King]

As we now store the dma_slave_config in pl08x_dma_chan, we don't need
to store this separately.  Use the one in dma_slave_config directly.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/dma/amba-pl08x.c   |    4 +---
 include/linux/amba/pl08x.h |    4 ----
 2 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 81f47cfa..c2e510a 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1198,8 +1198,6 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 	cctl |= burst << PL080_CONTROL_SB_SIZE_SHIFT;
 	cctl |= burst << PL080_CONTROL_DB_SIZE_SHIFT;
 
-	plchan->device_fc = config->device_fc;
-
 	if (plchan->runtime_direction == DMA_DEV_TO_MEM) {
 		plchan->src_cctl = pl08x_cctl(cctl) | PL080_CONTROL_DST_INCR |
 			pl08x_select_bus(plchan->cd->periph_buses,
@@ -1429,7 +1427,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
-	if (plchan->device_fc)
+	if (plchan->cfg.device_fc)
 		tmp = (direction == DMA_MEM_TO_DEV) ? PL080_FLOW_MEM2PER_PER :
 			PL080_FLOW_PER2MEM_PER;
 	else
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index b47b3c2..199ea4b 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -108,9 +108,6 @@ enum pl08x_dma_chan_state {
  * @host: a pointer to the host (internal use)
  * @state: whether the channel is idle, paused, running etc
  * @slave: whether this channel is a device (slave) or for memcpy
- * @device_fc: Flow Controller Settings for ccfg register. Only valid for slave
- * channels. Fill with 'true' if peripheral should be flow controller. Direction
- * will be selected at Runtime.
  * @waiting: a TX descriptor on this channel which is waiting for a physical
  * channel to become available
  */
@@ -131,7 +128,6 @@ struct pl08x_dma_chan {
 	struct pl08x_driver_data *host;
 	enum pl08x_dma_chan_state state;
 	bool slave;
-	bool device_fc;
 	struct pl08x_txd *waiting;
 };
 
-- 
1.7.4.4

[PATCH 4/6] DMA: PL08x: move the bus and increment selection to dma prepare function

[Russell King]

Move the bus and transfer increment selection to the DMA prepare
function rather than the slave configuration function.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/dma/amba-pl08x.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index c2e510a..af62c35 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1199,13 +1199,9 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 	cctl |= burst << PL080_CONTROL_DB_SIZE_SHIFT;
 
 	if (plchan->runtime_direction == DMA_DEV_TO_MEM) {
-		plchan->src_cctl = pl08x_cctl(cctl) | PL080_CONTROL_DST_INCR |
-			pl08x_select_bus(plchan->cd->periph_buses,
-					 pl08x->mem_buses);
+		plchan->src_cctl = pl08x_cctl(cctl);
 	} else {
-		plchan->dst_cctl = pl08x_cctl(cctl) | PL080_CONTROL_SRC_INCR |
-			pl08x_select_bus(pl08x->mem_buses,
-					 plchan->cd->periph_buses);
+		plchan->dst_cctl = pl08x_cctl(cctl);
 	}
 
 	dev_dbg(&pl08x->adev->dev,
@@ -1392,6 +1388,8 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	struct scatterlist *sg;
 	dma_addr_t slave_addr;
 	int ret, tmp;
+	u8 src_buses, dst_buses;
+	u32 cctl;
 
 	dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from %s\n",
 			__func__, sgl->length, plchan->name);
@@ -1415,11 +1413,15 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	txd->direction = direction;
 
 	if (direction == DMA_MEM_TO_DEV) {
-		txd->cctl = plchan->dst_cctl;
+		cctl = plchan->dst_cctl | PL080_CONTROL_SRC_INCR;
 		slave_addr = plchan->cfg.dst_addr;
+		src_buses = pl08x->mem_buses;
+		dst_buses = plchan->cd->periph_buses;
 	} else if (direction == DMA_DEV_TO_MEM) {
-		txd->cctl = plchan->src_cctl;
+		cctl = plchan->src_cctl | PL080_CONTROL_DST_INCR;
 		slave_addr = plchan->cfg.src_addr;
+		src_buses = plchan->cd->periph_buses;
+		dst_buses = pl08x->mem_buses;
 	} else {
 		pl08x_free_txd(pl08x, txd);
 		dev_err(&pl08x->adev->dev,
@@ -1427,6 +1429,8 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
+	cctl |= pl08x_select_bus(src_buses, dst_buses);
+
 	if (plchan->cfg.device_fc)
 		tmp = (direction == DMA_MEM_TO_DEV) ? PL080_FLOW_MEM2PER_PER :
 			PL080_FLOW_PER2MEM_PER;
@@ -1434,7 +1438,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		tmp = (direction == DMA_MEM_TO_DEV) ? PL080_FLOW_MEM2PER :
 			PL080_FLOW_PER2MEM;
 
-	txd->ccfg |= tmp << PL080_CONFIG_FLOW_CONTROL_SHIFT;
+	txd->ccfg = cctl | (tmp << PL080_CONFIG_FLOW_CONTROL_SHIFT);
 
 	for_each_sg(sgl, sg, sg_len, tmp) {
 		dsg = kzalloc(sizeof(struct pl08x_sg), GFP_NOWAIT);
@@ -1723,10 +1727,8 @@ static void pl08x_dma_slave_init(struct pl08x_dma_chan *chan)
 	chan->name = chan->cd->bus_id;
 	chan->cfg.src_addr = chan->cd->addr;
 	chan->cfg.dst_addr = chan->cd->addr;
-	chan->src_cctl = cctl | PL080_CONTROL_DST_INCR |
-		pl08x_select_bus(chan->cd->periph_buses, chan->host->mem_buses);
-	chan->dst_cctl = cctl | PL080_CONTROL_SRC_INCR |
-		pl08x_select_bus(chan->host->mem_buses, chan->cd->periph_buses);
+	chan->src_cctl = cctl;
+	chan->dst_cctl = cctl;
 }
 
 /*
-- 
1.7.4.4

[PATCH 5/6] DMA: PL08x: extract function to to generate cctl values

[Russell King]

Extract the functionality from dma_slave_config to generate the cctl
values for a given bus width and burst size.  This allows us to use
this elsewhere in the driver, namely the prepare functions.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/dma/amba-pl08x.c |   53 +++++++++++++++++++++++++++------------------
 1 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index af62c35..bf94156 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1148,14 +1148,40 @@ static u32 pl08x_burst(u32 maxburst)
 	return burst_sizes[i].reg;
 }
 
+static u32 pl08x_get_cctl(struct pl08x_dma_chan *plchan,
+	enum dma_slave_buswidth addr_width, u32 maxburst)
+{
+	u32 width, burst, cctl = 0;
+
+	width = pl08x_width(addr_width);
+	if (width == ~0)
+		return ~0;
+
+	cctl |= width << PL080_CONTROL_SWIDTH_SHIFT;
+	cctl |= width << PL080_CONTROL_DWIDTH_SHIFT;
+
+	/*
+	 * If this channel will only request single transfers, set this
+	 * down to ONE element.  Also select one element if no maxburst
+	 * is specified.
+	 */
+	if (plchan->cd->single)
+		maxburst = 1;
+
+	burst = pl08x_burst(maxburst);
+	cctl |= burst << PL080_CONTROL_SB_SIZE_SHIFT;
+	cctl |= burst << PL080_CONTROL_DB_SIZE_SHIFT;
+
+	return pl08x_cctl(cctl);
+}
+
 static int dma_set_runtime_config(struct dma_chan *chan,
 				  struct dma_slave_config *config)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
 	struct pl08x_driver_data *pl08x = plchan->host;
 	enum dma_slave_buswidth addr_width;
-	u32 width, burst, maxburst;
-	u32 cctl = 0;
+	u32 maxburst, cctl = 0;
 
 	if (!plchan->slave)
 		return -EINVAL;
@@ -1174,8 +1200,8 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 		return -EINVAL;
 	}
 
-	width = pl08x_width(addr_width);
-	if (width == ~0) {
+	cctl = pl08x_get_cctl(plchan, addr_width, maxburst);
+	if (cctl == ~0) {
 		dev_err(&pl08x->adev->dev,
 			"bad runtime_config: alien address width\n");
 		return -EINVAL;
@@ -1183,25 +1209,10 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 
 	plchan->cfg = *config;
 
-	cctl |= width << PL080_CONTROL_SWIDTH_SHIFT;
-	cctl |= width << PL080_CONTROL_DWIDTH_SHIFT;
-
-	/*
-	 * If this channel will only request single transfers, set this
-	 * down to ONE element.  Also select one element if no maxburst
-	 * is specified.
-	 */
-	if (plchan->cd->single)
-		maxburst = 1;
-
-	burst = pl08x_burst(maxburst);
-	cctl |= burst << PL080_CONTROL_SB_SIZE_SHIFT;
-	cctl |= burst << PL080_CONTROL_DB_SIZE_SHIFT;
-
 	if (plchan->runtime_direction == DMA_DEV_TO_MEM) {
-		plchan->src_cctl = pl08x_cctl(cctl);
+		plchan->src_cctl = cctl;
 	} else {
-		plchan->dst_cctl = pl08x_cctl(cctl);
+		plchan->dst_cctl = cctl;
 	}
 
 	dev_dbg(&pl08x->adev->dev,
-- 
1.7.4.4

[PATCH 6/6] DMA: PL08x: ignore 'direction' argument in dma_slave_config

[Russell King]

Ignore the direction argument in dma_slave_config, and configure both
directions independently.  We still check that the configuration for
the intended direction is valid; this check will eventually be dropped.
This check is just for debugging at present.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/dma/amba-pl08x.c   |   53 +++++++++++++++-----------------------------
 include/linux/amba/pl08x.h |    3 --
 2 files changed, 18 insertions(+), 38 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index bf94156..97f5df4 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1180,50 +1180,31 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
 	struct pl08x_driver_data *pl08x = plchan->host;
-	enum dma_slave_buswidth addr_width;
-	u32 maxburst, cctl = 0;
+	u32 src_cctl, dst_cctl;
 
 	if (!plchan->slave)
 		return -EINVAL;
 
-	/* Transfer direction */
-	plchan->runtime_direction = config->direction;
-	if (config->direction == DMA_MEM_TO_DEV) {
-		addr_width = config->dst_addr_width;
-		maxburst = config->dst_maxburst;
-	} else if (config->direction == DMA_DEV_TO_MEM) {
-		addr_width = config->src_addr_width;
-		maxburst = config->src_maxburst;
-	} else {
+	dst_cctl = pl08x_get_cctl(plchan, config->dst_addr_width,
+				  config->dst_maxburst);
+	if (dst_cctl == ~0 && config->direction == DMA_MEM_TO_DEV) {
 		dev_err(&pl08x->adev->dev,
-			"bad runtime_config: alien transfer direction\n");
+			"bad runtime_config: alien address width (M2D)\n");
 		return -EINVAL;
 	}
 
-	cctl = pl08x_get_cctl(plchan, addr_width, maxburst);
-	if (cctl == ~0) {
+	src_cctl = pl08x_get_cctl(plchan, config->src_addr_width,
+				  config->src_maxburst);
+	if (src_cctl == ~0 && config->direction == DMA_DEV_TO_MEM) {
 		dev_err(&pl08x->adev->dev,
-			"bad runtime_config: alien address width\n");
+			"bad runtime_config: alien address width (D2M)\n");
 		return -EINVAL;
 	}
 
+	plchan->dst_cctl = dst_cctl;
+	plchan->src_cctl = src_cctl;
 	plchan->cfg = *config;
 
-	if (plchan->runtime_direction == DMA_DEV_TO_MEM) {
-		plchan->src_cctl = cctl;
-	} else {
-		plchan->dst_cctl = cctl;
-	}
-
-	dev_dbg(&pl08x->adev->dev,
-		"configured channel %s (%s) for %s, data width %d, "
-		"maxburst %d words, LE, CCTL=0x%08x\n",
-		dma_chan_name(chan), plchan->name,
-		(config->direction == DMA_DEV_TO_MEM) ? "RX" : "TX",
-		addr_width,
-		maxburst,
-		cctl);
-
 	return 0;
 }
 
@@ -1411,11 +1392,6 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
-	if (direction != plchan->runtime_direction)
-		dev_err(&pl08x->adev->dev, "%s DMA setup does not match "
-			"the direction configured for the PrimeCell\n",
-			__func__);
-
 	/*
 	 * Set up addresses, the PrimeCell configured address
 	 * will take precedence since this may configure the
@@ -1440,6 +1416,13 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
+	if (cctl == ~0) {
+		pl08x_free_txd(pl08x, txd);
+		dev_err(&pl08x->adev->dev,
+			"DMA slave configuration botched?\n");
+		return NULL;
+	}
+
 	cctl |= pl08x_select_bus(src_buses, dst_buses);
 
 	if (plchan->cfg.device_fc)
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index 199ea4b..5f8e6d7 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -100,8 +100,6 @@ enum pl08x_dma_chan_state {
  * @name: name of channel
  * @cd: channel platform data
  * @runtime_addr: address for RX/TX according to the runtime config
- * @runtime_direction: current direction of this channel according to
- * runtime config
  * @pend_list: queued transactions pending on this channel
  * @at: active transaction on this channel
  * @lock: a lock for this channel data
@@ -121,7 +119,6 @@ struct pl08x_dma_chan {
 	struct dma_slave_config cfg;
 	u32 src_cctl;
 	u32 dst_cctl;
-	enum dma_transfer_direction runtime_direction;
 	struct list_head pend_list;
 	struct pl08x_txd *at;
 	spinlock_t lock;
-- 
1.7.4.4

[RFC] pl08x: don't use dma_slave_config direction argument

[Russell King - ARM Linux]

On Wed, May 16, 2012 at 12:04:51PM +0100, Russell King - ARM Linux wrote:
> This series removes the dependence on the dma_slave_config direction
> argument for the PL08x DMA engine driver, and in doing so, we end up
> with less code in the driver.
> 
> We now compute the cctl values for both directions, and continue to
> select the appropriate one at prepare time.  If this is found to be
> invalid, the prepare function will return NULL.
> 
> However, we still use the direction argument in the slave configuration
> call to determine whether we should report and fail an invalid
> configuration.  Eventually this will be removed.

This is the final patch, which is not part of this RFC, which illustrates
where we should be with this driver.  Note that this will require all
slave users to issue a DMA slave configuration call before they use the
channel; the "defaults" are no longer used.  I think this is the case
today anyway.  Note - even more code reduction...

 drivers/dma/amba-pl08x.c   |   36 +++++++++---------------------------
 include/linux/amba/pl08x.h |    5 ++---
 2 files changed, 11 insertions(+), 30 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 7d92510..d662c22 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1180,30 +1180,10 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 				  struct dma_slave_config *config)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
-	struct pl08x_driver_data *pl08x = plchan->host;
-	u32 src_cctl, dst_cctl;
 
 	if (!plchan->slave)
 		return -EINVAL;
 
-	dst_cctl = pl08x_get_cctl(plchan, config->dst_addr_width,
-				  config->dst_maxburst);
-	if (dst_cctl == ~0 && config->direction == DMA_MEM_TO_DEV) {
-		dev_err(&pl08x->adev->dev,
-			"bad runtime_config: alien address width (M2D)\n");
-		return -EINVAL;
-	}
-
-	src_cctl = pl08x_get_cctl(plchan, config->src_addr_width,
-				  config->src_maxburst);
-	if (src_cctl == ~0 && config->direction == DMA_DEV_TO_MEM) {
-		dev_err(&pl08x->adev->dev,
-			"bad runtime_config: alien address width (D2M)\n");
-		return -EINVAL;
-	}
-
-	plchan->dst_cctl = dst_cctl;
-	plchan->src_cctl = src_cctl;
 	plchan->cfg = *config;
 
 	return 0;
@@ -1379,10 +1359,11 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	struct pl08x_txd *txd;
 	struct pl08x_sg *dsg;
 	struct scatterlist *sg;
+	enum dma_slave_buswidth addr_width;
 	dma_addr_t slave_addr;
 	int ret, tmp;
 	u8 src_buses, dst_buses;
-	u32 cctl;
+	u32 maxburst, cctl;
 
 	dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from %s\n",
 			__func__, sgl->length, plchan->name);
@@ -1401,13 +1382,17 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	txd->direction = direction;
 
 	if (direction == DMA_MEM_TO_DEV) {
-		cctl = plchan->dst_cctl | PL080_CONTROL_SRC_INCR;
+		cctl = PL080_CONTROL_SRC_INCR;
 		slave_addr = plchan->cfg.dst_addr;
+		addr_width = plchan->cfg.dst_addr_width;
+		maxburst = plchan->cfg.dst_maxburst;
 		src_buses = pl08x->mem_buses;
 		dst_buses = plchan->cd->periph_buses;
 	} else if (direction == DMA_DEV_TO_MEM) {
-		cctl = plchan->src_cctl | PL080_CONTROL_DST_INCR;
+		cctl = PL080_CONTROL_DST_INCR;
 		slave_addr = plchan->cfg.src_addr;
+		addr_width = plchan->cfg.src_addr_width;
+		maxburst = plchan->cfg.src_maxburst;
 		src_buses = plchan->cd->periph_buses;
 		dst_buses = pl08x->mem_buses;
 	} else {
@@ -1417,6 +1402,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
+	cctl |= pl08x_get_cctl(plchan, addr_width, maxburst);
 	if (cctl == ~0) {
 		pl08x_free_txd(pl08x, txd);
 		dev_err(&pl08x->adev->dev,
@@ -1723,14 +1709,10 @@ static void pl08x_timer(unsigned long data)
 
 static void pl08x_dma_slave_init(struct pl08x_dma_chan *chan)
 {
-	u32 cctl = pl08x_cctl(chan->cd->cctl);
-
 	chan->slave = true;
 	chan->name = chan->cd->bus_id;
 	chan->cfg.src_addr = chan->cd->addr;
 	chan->cfg.dst_addr = chan->cd->addr;
-	chan->src_cctl = cctl;
-	chan->dst_cctl = cctl;
 }
 
 /*
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index 5f8e6d7..5d0ad6b 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -47,7 +47,8 @@ enum {
  * devices with static assignments
  * @muxval: a number usually used to poke into some mux regiser to
  * mux in the signal to this channel
- * @cctl_opt: default options for the channel control register
+ * @cctl: default options for the channel control register
+ *  *** not used for slave channels ***
  * @addr: source/target address in physical memory for this DMA channel,
  * can be the address of a FIFO register for burst requests for example.
  * This can be left undefined if the PrimeCell API is used for configuring
@@ -117,8 +118,6 @@ struct pl08x_dma_chan {
 	char *name;
 	const struct pl08x_channel_data *cd;
 	struct dma_slave_config cfg;
-	u32 src_cctl;
-	u32 dst_cctl;
 	struct list_head pend_list;
 	struct pl08x_txd *at;
 	spinlock_t lock;

[RFC] pl08x: don't use dma_slave_config direction argument

[Linus Walleij]

On Wed, May 16, 2012 at 1:17 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

>> This series removes the dependence on the dma_slave_config direction
>> argument for the PL08x DMA engine driver, and in doing so, we end up
>> with less code in the driver.
>>
>> We now compute the cctl values for both directions, and continue to
>> select the appropriate one at prepare time.  If this is found to be
>> invalid, the prepare function will return NULL.
>>
>> However, we still use the direction argument in the slave configuration
>> call to determine whether we should report and fail an invalid
>> configuration.  Eventually this will be removed.

The direction of this is sound.

I think one thing that needs fixing along with this is the PL180/MMCI
driver since it currently accepts a single channel and then switch
direction of that channel depending on whether we read or write
from/to the device.

All channel providers and machines need to be checked to see
they use the parameter from prep_slave_sg() instead. The pair
I can think of is the COH901318 DMAengine driver and the U300
platform data (the ux500 has two channels, TX and RX).

Don't let this hold you back though, I'm happy to fix up any breakage
to get this moving.

> This is the final patch, which is not part of this RFC, which illustrates
> where we should be with this driver. ?Note that this will require all
> slave users to issue a DMA slave configuration call before they use the
> channel; the "defaults" are no longer used. ?I think this is the case
> today anyway. ?Note - even more code reduction...

This looks good. I tried to apply the patch to test it but failed so I
guess there is some more development underneath it.

> @@ -47,7 +47,8 @@ enum {
> ?* devices with static assignments
> ?* @muxval: a number usually used to poke into some mux regiser to
> ?* mux in the signal to this channel
> - * @cctl_opt: default options for the channel control register
> + * @cctl: default options for the channel control register
> + * ?*** not used for slave channels ***

Maybe just rename this thing cctl_memcpy then?

Yours,
Linus Walleij

[RFC] pl08x: don't use dma_slave_config direction argument

[Russell King - ARM Linux]

On Wed, May 16, 2012 at 01:59:42PM +0200, Linus Walleij wrote:
> I think one thing that needs fixing along with this is the PL180/MMCI
> driver since it currently accepts a single channel and then switch
> direction of that channel depending on whether we read or write
> from/to the device.

That's an interesting problem if the DMA engine coupling is different
from how PL080/PL180 are coupled together.

> > This is the final patch, which is not part of this RFC, which illustrates
> > where we should be with this driver. ?Note that this will require all
> > slave users to issue a DMA slave configuration call before they use the
> > channel; the "defaults" are no longer used. ?I think this is the case
> > today anyway. ?Note - even more code reduction...
> 
> This looks good. I tried to apply the patch to test it but failed so I
> guess there is some more development underneath it.

Yes, I've reshuffled the patches so the one below should apply.

> > @@ -47,7 +47,8 @@ enum {
> > ?* devices with static assignments
> > ?* @muxval: a number usually used to poke into some mux regiser to
> > ?* mux in the signal to this channel
> > - * @cctl_opt: default options for the channel control register
> > + * @cctl: default options for the channel control register
> > + * ?*** not used for slave channels ***
> 
> Maybe just rename this thing cctl_memcpy then?

And I've changed this; I couldn't find any in-tree users of it though.
I've also added back the obvious check for invalid widths into the
dma_slave_config function.

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 97f5df4..5e4dc16 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1179,30 +1179,15 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 				  struct dma_slave_config *config)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
-	struct pl08x_driver_data *pl08x = plchan->host;
-	u32 src_cctl, dst_cctl;
 
 	if (!plchan->slave)
 		return -EINVAL;
 
-	dst_cctl = pl08x_get_cctl(plchan, config->dst_addr_width,
-				  config->dst_maxburst);
-	if (dst_cctl == ~0 && config->direction == DMA_MEM_TO_DEV) {
-		dev_err(&pl08x->adev->dev,
-			"bad runtime_config: alien address width (M2D)\n");
+	/* Reject definitely invalid configurations */
+	if (config->src_addr_width == DMA_SLAVE_BUSWIDTH_8_BYTES ||
+	    config->dst_addr_width == DMA_SLAVE_BUSWIDTH_4_BYTES)
 		return -EINVAL;
-	}
 
-	src_cctl = pl08x_get_cctl(plchan, config->src_addr_width,
-				  config->src_maxburst);
-	if (src_cctl == ~0 && config->direction == DMA_DEV_TO_MEM) {
-		dev_err(&pl08x->adev->dev,
-			"bad runtime_config: alien address width (D2M)\n");
-		return -EINVAL;
-	}
-
-	plchan->dst_cctl = dst_cctl;
-	plchan->src_cctl = src_cctl;
 	plchan->cfg = *config;
 
 	return 0;
@@ -1351,7 +1336,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 
 	/* Set platform data for m2m */
 	txd->ccfg |= PL080_FLOW_MEM2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
-	txd->cctl = pl08x->pd->memcpy_channel.cctl &
+	txd->cctl = pl08x->pd->memcpy_channel.cctl_memcpy &
 			~(PL080_CONTROL_DST_AHB2 | PL080_CONTROL_SRC_AHB2);
 
 	/* Both to be incremented or the code will break */
@@ -1378,10 +1363,11 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	struct pl08x_txd *txd;
 	struct pl08x_sg *dsg;
 	struct scatterlist *sg;
+	enum dma_slave_buswidth addr_width;
 	dma_addr_t slave_addr;
 	int ret, tmp;
 	u8 src_buses, dst_buses;
-	u32 cctl;
+	u32 maxburst, cctl;
 
 	dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from %s\n",
 			__func__, sgl->length, plchan->name);
@@ -1400,13 +1386,17 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	txd->direction = direction;
 
 	if (direction == DMA_MEM_TO_DEV) {
-		cctl = plchan->dst_cctl | PL080_CONTROL_SRC_INCR;
+		cctl = PL080_CONTROL_SRC_INCR;
 		slave_addr = plchan->cfg.dst_addr;
+		addr_width = plchan->cfg.dst_addr_width;
+		maxburst = plchan->cfg.dst_maxburst;
 		src_buses = pl08x->mem_buses;
 		dst_buses = plchan->cd->periph_buses;
 	} else if (direction == DMA_DEV_TO_MEM) {
-		cctl = plchan->src_cctl | PL080_CONTROL_DST_INCR;
+		cctl = PL080_CONTROL_DST_INCR;
 		slave_addr = plchan->cfg.src_addr;
+		addr_width = plchan->cfg.src_addr_width;
+		maxburst = plchan->cfg.src_maxburst;
 		src_buses = plchan->cd->periph_buses;
 		dst_buses = pl08x->mem_buses;
 	} else {
@@ -1416,6 +1406,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
+	cctl |= pl08x_get_cctl(plchan, addr_width, maxburst);
 	if (cctl == ~0) {
 		pl08x_free_txd(pl08x, txd);
 		dev_err(&pl08x->adev->dev,
@@ -1715,14 +1706,10 @@ static irqreturn_t pl08x_irq(int irq, void *dev)
 
 static void pl08x_dma_slave_init(struct pl08x_dma_chan *chan)
 {
-	u32 cctl = pl08x_cctl(chan->cd->cctl);
-
 	chan->slave = true;
 	chan->name = chan->cd->bus_id;
 	chan->cfg.src_addr = chan->cd->addr;
 	chan->cfg.dst_addr = chan->cd->addr;
-	chan->src_cctl = cctl;
-	chan->dst_cctl = cctl;
 }
 
 /*
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index 5f8e6d7..9331b68 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -47,7 +47,8 @@ enum {
  * devices with static assignments
  * @muxval: a number usually used to poke into some mux regiser to
  * mux in the signal to this channel
- * @cctl_opt: default options for the channel control register
+ * @cctl_memcpy: options for the channel control register for memcpy
+ *  *** not used for slave channels ***
  * @addr: source/target address in physical memory for this DMA channel,
  * can be the address of a FIFO register for burst requests for example.
  * This can be left undefined if the PrimeCell API is used for configuring
@@ -65,7 +66,7 @@ struct pl08x_channel_data {
 	int min_signal;
 	int max_signal;
 	u32 muxval;
-	u32 cctl;
+	u32 cctl_memcpy;
 	dma_addr_t addr;
 	bool circular_buffer;
 	bool single;
@@ -117,8 +118,6 @@ struct pl08x_dma_chan {
 	char *name;
 	const struct pl08x_channel_data *cd;
 	struct dma_slave_config cfg;
-	u32 src_cctl;
-	u32 dst_cctl;
 	struct list_head pend_list;
 	struct pl08x_txd *at;
 	spinlock_t lock;

[RFC] pl08x: don't use dma_slave_config direction argument

[Russell King - ARM Linux]

On Wed, May 16, 2012 at 01:59:42PM +0200, Linus Walleij wrote:
> Maybe just rename this thing cctl_memcpy then?

I've just noticed that arm-soc contains this (eg):

struct pl08x_channel_data spear310_dma_info[] = {
        {
                .bus_id = "uart0_rx",
                .min_signal = 2,
                .max_signal = 2,
                .muxval = 0,
                .cctl = 0,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                .periph_buses = PL08X_AHB1,

So, changing .cctl to .cctl_memcpy breaks the compilation for all the spear
platforms.  Moreover, these explicit-zero initializers should not be here.
Even with these gone, one still remains in the spear6xx code for their
memcpy support.

Can we get these zero initializers killed for this merge window please.

[RFC] pl08x: don't use dma_slave_config direction argument

[Olof Johansson]

On Thu, May 17, 2012 at 11:42:50AM +0100, Russell King - ARM Linux wrote:
> On Wed, May 16, 2012 at 01:59:42PM +0200, Linus Walleij wrote:
> > Maybe just rename this thing cctl_memcpy then?
> 
> I've just noticed that arm-soc contains this (eg):
> 
> struct pl08x_channel_data spear310_dma_info[] = {
>         {
>                 .bus_id = "uart0_rx",
>                 .min_signal = 2,
>                 .max_signal = 2,
>                 .muxval = 0,
>                 .cctl = 0,
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                 .periph_buses = PL08X_AHB1,
> 
> So, changing .cctl to .cctl_memcpy breaks the compilation for all the spear
> platforms.  Moreover, these explicit-zero initializers should not be here.
> Even with these gone, one still remains in the spear6xx code for their
> memcpy support.
> 
> Can we get these zero initializers killed for this merge window please.

Introduced by "SPEAr: Add PL080 DMA support for 3xx and 6xx". 

There are some muxval implicit 0 initalizers too, removed them while I was at
it.

Viresh, can I get an acked-by?


>From cda9fbb62852a1b1fc0b1e77eec8b916d0f4d762 Mon Sep 17 00:00:00 2001
From: Olof Johansson <olof@lixom.net>
Date: Fri, 18 May 2012 08:43:39 -0700
Subject: [PATCH] ARM: spear3xx: remove some zero initalizers

Removing some unnecessary 0 initializers from platform pl08x platform
data structures.

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 arch/arm/mach-spear3xx/spear300.c | 38 --------------------------------------
 arch/arm/mach-spear3xx/spear310.c | 38 --------------------------------------
 arch/arm/mach-spear3xx/spear320.c | 38 --------------------------------------
 3 files changed, 114 deletions(-)

diff --git a/arch/arm/mach-spear3xx/spear300.c b/arch/arm/mach-spear3xx/spear300.c
index f74a05b..e3d6933 100644
--- a/arch/arm/mach-spear3xx/spear300.c
+++ b/arch/arm/mach-spear3xx/spear300.c
@@ -119,183 +119,145 @@ struct pl08x_channel_data spear300_dma_info[] = {
 		.bus_id = "uart0_rx",
 		.min_signal = 2,
 		.max_signal = 2,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart0_tx",
 		.min_signal = 3,
 		.max_signal = 3,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ssp0_rx",
 		.min_signal = 8,
 		.max_signal = 8,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ssp0_tx",
 		.min_signal = 9,
 		.max_signal = 9,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "i2c_rx",
 		.min_signal = 10,
 		.max_signal = 10,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "i2c_tx",
 		.min_signal = 11,
 		.max_signal = 11,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "irda",
 		.min_signal = 12,
 		.max_signal = 12,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "adc",
 		.min_signal = 13,
 		.max_signal = 13,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "to_jpeg",
 		.min_signal = 14,
 		.max_signal = 14,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "from_jpeg",
 		.min_signal = 15,
 		.max_signal = 15,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras0_rx",
-		.min_signal = 0,
-		.max_signal = 0,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras0_tx",
 		.min_signal = 1,
 		.max_signal = 1,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras1_rx",
 		.min_signal = 2,
 		.max_signal = 2,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras1_tx",
 		.min_signal = 3,
 		.max_signal = 3,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras2_rx",
 		.min_signal = 4,
 		.max_signal = 4,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras2_tx",
 		.min_signal = 5,
 		.max_signal = 5,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras3_rx",
 		.min_signal = 6,
 		.max_signal = 6,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras3_tx",
 		.min_signal = 7,
 		.max_signal = 7,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras4_rx",
 		.min_signal = 8,
 		.max_signal = 8,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras4_tx",
 		.min_signal = 9,
 		.max_signal = 9,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras5_rx",
 		.min_signal = 10,
 		.max_signal = 10,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras5_tx",
 		.min_signal = 11,
 		.max_signal = 11,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras6_rx",
 		.min_signal = 12,
 		.max_signal = 12,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras6_tx",
 		.min_signal = 13,
 		.max_signal = 13,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras7_rx",
 		.min_signal = 14,
 		.max_signal = 14,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras7_tx",
 		.min_signal = 15,
 		.max_signal = 15,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	},
 };
diff --git a/arch/arm/mach-spear3xx/spear310.c b/arch/arm/mach-spear3xx/spear310.c
index 84dfb09..13ee447 100644
--- a/arch/arm/mach-spear3xx/spear310.c
+++ b/arch/arm/mach-spear3xx/spear310.c
@@ -204,183 +204,145 @@ struct pl08x_channel_data spear310_dma_info[] = {
 		.bus_id = "uart0_rx",
 		.min_signal = 2,
 		.max_signal = 2,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart0_tx",
 		.min_signal = 3,
 		.max_signal = 3,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ssp0_rx",
 		.min_signal = 8,
 		.max_signal = 8,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ssp0_tx",
 		.min_signal = 9,
 		.max_signal = 9,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "i2c_rx",
 		.min_signal = 10,
 		.max_signal = 10,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "i2c_tx",
 		.min_signal = 11,
 		.max_signal = 11,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "irda",
 		.min_signal = 12,
 		.max_signal = 12,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "adc",
 		.min_signal = 13,
 		.max_signal = 13,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "to_jpeg",
 		.min_signal = 14,
 		.max_signal = 14,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "from_jpeg",
 		.min_signal = 15,
 		.max_signal = 15,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart1_rx",
-		.min_signal = 0,
-		.max_signal = 0,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart1_tx",
 		.min_signal = 1,
 		.max_signal = 1,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart2_rx",
 		.min_signal = 2,
 		.max_signal = 2,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart2_tx",
 		.min_signal = 3,
 		.max_signal = 3,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart3_rx",
 		.min_signal = 4,
 		.max_signal = 4,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart3_tx",
 		.min_signal = 5,
 		.max_signal = 5,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart4_rx",
 		.min_signal = 6,
 		.max_signal = 6,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart4_tx",
 		.min_signal = 7,
 		.max_signal = 7,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart5_rx",
 		.min_signal = 8,
 		.max_signal = 8,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart5_tx",
 		.min_signal = 9,
 		.max_signal = 9,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras5_rx",
 		.min_signal = 10,
 		.max_signal = 10,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras5_tx",
 		.min_signal = 11,
 		.max_signal = 11,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras6_rx",
 		.min_signal = 12,
 		.max_signal = 12,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras6_tx",
 		.min_signal = 13,
 		.max_signal = 13,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras7_rx",
 		.min_signal = 14,
 		.max_signal = 14,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ras7_tx",
 		.min_signal = 15,
 		.max_signal = 15,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	},
 };
diff --git a/arch/arm/mach-spear3xx/spear320.c b/arch/arm/mach-spear3xx/spear320.c
index a88fa84..8ceb0c5 100644
--- a/arch/arm/mach-spear3xx/spear320.c
+++ b/arch/arm/mach-spear3xx/spear320.c
@@ -212,183 +212,145 @@ struct pl08x_channel_data spear320_dma_info[] = {
 		.bus_id = "uart0_rx",
 		.min_signal = 2,
 		.max_signal = 2,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "uart0_tx",
 		.min_signal = 3,
 		.max_signal = 3,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ssp0_rx",
 		.min_signal = 8,
 		.max_signal = 8,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ssp0_tx",
 		.min_signal = 9,
 		.max_signal = 9,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "i2c0_rx",
 		.min_signal = 10,
 		.max_signal = 10,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "i2c0_tx",
 		.min_signal = 11,
 		.max_signal = 11,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "irda",
 		.min_signal = 12,
 		.max_signal = 12,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "adc",
 		.min_signal = 13,
 		.max_signal = 13,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "to_jpeg",
 		.min_signal = 14,
 		.max_signal = 14,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "from_jpeg",
 		.min_signal = 15,
 		.max_signal = 15,
-		.muxval = 0,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB1,
 	}, {
 		.bus_id = "ssp1_rx",
-		.min_signal = 0,
-		.max_signal = 0,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "ssp1_tx",
 		.min_signal = 1,
 		.max_signal = 1,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "ssp2_rx",
 		.min_signal = 2,
 		.max_signal = 2,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "ssp2_tx",
 		.min_signal = 3,
 		.max_signal = 3,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "uart1_rx",
 		.min_signal = 4,
 		.max_signal = 4,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "uart1_tx",
 		.min_signal = 5,
 		.max_signal = 5,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "uart2_rx",
 		.min_signal = 6,
 		.max_signal = 6,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "uart2_tx",
 		.min_signal = 7,
 		.max_signal = 7,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "i2c1_rx",
 		.min_signal = 8,
 		.max_signal = 8,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "i2c1_tx",
 		.min_signal = 9,
 		.max_signal = 9,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "i2c2_rx",
 		.min_signal = 10,
 		.max_signal = 10,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "i2c2_tx",
 		.min_signal = 11,
 		.max_signal = 11,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "i2s_rx",
 		.min_signal = 12,
 		.max_signal = 12,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "i2s_tx",
 		.min_signal = 13,
 		.max_signal = 13,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "rs485_rx",
 		.min_signal = 14,
 		.max_signal = 14,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	}, {
 		.bus_id = "rs485_tx",
 		.min_signal = 15,
 		.max_signal = 15,
 		.muxval = 1,
-		.cctl = 0,
 		.periph_buses = PL08X_AHB2,
 	},
 };
-- 
1.7.10.1.488.g05fbf7a

[RFC] pl08x: don't use dma_slave_config direction argument

[Linus Walleij]

On Fri, May 18, 2012 at 5:47 PM, Olof Johansson <olof@lixom.net> wrote:

> From cda9fbb62852a1b1fc0b1e77eec8b916d0f4d762 Mon Sep 17 00:00:00 2001
> From: Olof Johansson <olof@lixom.net>
> Date: Fri, 18 May 2012 08:43:39 -0700
> Subject: [PATCH] ARM: spear3xx: remove some zero initalizers
>
> Removing some unnecessary 0 initializers from platform pl08x platform
> data structures.
>
> Signed-off-by: Olof Johansson <olof@lixom.net>

Oh Olof beats Viresh to the show, thanks!
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFC] pl08x: don't use dma_slave_config direction argument

[Linus Walleij]

On Thu, May 17, 2012 at 12:42 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> On Wed, May 16, 2012 at 01:59:42PM +0200, Linus Walleij wrote:
>> Maybe just rename this thing cctl_memcpy then?
>
> I've just noticed that arm-soc contains this (eg):
>
> struct pl08x_channel_data spear310_dma_info[] = {
> ? ? ? ?{
> ? ? ? ? ? ? ? ?.bus_id = "uart0_rx",
> ? ? ? ? ? ? ? ?.min_signal = 2,
> ? ? ? ? ? ? ? ?.max_signal = 2,
> ? ? ? ? ? ? ? ?.muxval = 0,
> ? ? ? ? ? ? ? ?.cctl = 0,
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ? ? ? ? ? ? ? ?.periph_buses = PL08X_AHB1,
>
> So, changing .cctl to .cctl_memcpy breaks the compilation for all the spear
> platforms. ?Moreover, these explicit-zero initializers should not be here.
> Even with these gone, one still remains in the spear6xx code for their
> memcpy support.
>
> Can we get these zero initializers killed for this merge window please.

That'd be Viresh's patch set I think ... Viresh can you post a patch on
top of the stuff in ARM SoC that kills off the zero intializers for cctl?

Yours,
Linus Walleij

[RFC] pl08x: don't use dma_slave_config direction argument

[Roland Stigge]

Hi,

On 05/16/2012 01:04 PM, Russell King - ARM Linux wrote:
> This series removes the dependence on the dma_slave_config direction
> argument for the PL08x DMA engine driver, and in doing so, we end up
> with less code in the driver.
> 
> We now compute the cctl values for both directions, and continue to
> select the appropriate one at prepare time.  If this is found to be
> invalid, the prepare function will return NULL.
> 
> However, we still use the direction argument in the slave configuration
> call to determine whether we should report and fail an invalid
> configuration.  Eventually this will be removed.
> 
> * This has only been compile tested.  Test feedback welcomed. *
> 
>  drivers/dma/amba-pl08x.c   |  196 +++++++++++++++++++++++++++++---------------
>  include/linux/amba/pl08x.h |   84 +------------------
>  2 files changed, 134 insertions(+), 146 deletions(-)
> 

After applying series 1-6 (as well as with 1-7), my just posted LPC32xx
NAND drivers have problems mounting the MTD (basically, NAND page reads,
DMA dev to mem from NAND controller internal buffers).

* SLC driver says:

lpc32xx-nand 20020000.flash: FIFO not empty!
lpc32xx-nand 20020000.flash: FIFO held data too long
lpc32xx-nand 20020000.flash: DMA FIFO failure

* MLC driver just gets bad data resulting in detected bad blocks.

Without the patches, they work as tested.

Any hints, before I find the time to sort out all the details with
amba-pl08x?

Thanks,

Roland

[RFC] pl08x: don't use dma_slave_config direction argument

[Russell King - ARM Linux]

On Wed, May 16, 2012 at 02:24:02PM +0200, Roland Stigge wrote:
> After applying series 1-6 (as well as with 1-7), my just posted LPC32xx
> NAND drivers have problems mounting the MTD (basically, NAND page reads,
> DMA dev to mem from NAND controller internal buffers).
> 
> * SLC driver says:
> 
> lpc32xx-nand 20020000.flash: FIFO not empty!
> lpc32xx-nand 20020000.flash: FIFO held data too long
> lpc32xx-nand 20020000.flash: DMA FIFO failure
> 
> * MLC driver just gets bad data resulting in detected bad blocks.
> 
> Without the patches, they work as tested.
> 
> Any hints, before I find the time to sort out all the details with
> amba-pl08x?

Nothing jumps out; it would be useful if you could bisect the small
series - I did the conversion in small steps so it should be easy to
track down.  And it shouldn't take too long to rebuild between each
iteration.  Whatever it is, it's probably going to be a stupid mistake
somewhere.

[RFC] pl08x: don't use dma_slave_config direction argument

[Roland Stigge]

On 05/16/2012 02:52 PM, Russell King - ARM Linux wrote:
>> * SLC driver says:
>>
>> lpc32xx-nand 20020000.flash: FIFO not empty!
>> lpc32xx-nand 20020000.flash: FIFO held data too long
>> lpc32xx-nand 20020000.flash: DMA FIFO failure
>>
>> Any hints, before I find the time to sort out all the details with
>> amba-pl08x?
> 
> Nothing jumps out; it would be useful if you could bisect the small
> series - I did the conversion in small steps so it should be easy to
> track down.  And it shouldn't take too long to rebuild between each
> iteration.  Whatever it is, it's probably going to be a stupid mistake
> somewhere.

The problem first appears with patch #4.

[RFC] pl08x: don't use dma_slave_config direction argument

[Roland Stigge]

On 05/16/2012 03:04 PM, Roland Stigge wrote:
> On 05/16/2012 02:52 PM, Russell King - ARM Linux wrote:
>>> * SLC driver says:
>>>
>>> lpc32xx-nand 20020000.flash: FIFO not empty!
>>> lpc32xx-nand 20020000.flash: FIFO held data too long
>>> lpc32xx-nand 20020000.flash: DMA FIFO failure
>>>
>>> Any hints, before I find the time to sort out all the details with
>>> amba-pl08x?
>>
>> Nothing jumps out; it would be useful if you could bisect the small
>> series - I did the conversion in small steps so it should be easy to
>> track down.  And it shouldn't take too long to rebuild between each
>> iteration.  Whatever it is, it's probably going to be a stupid mistake
>> somewhere.
> 
> The problem first appears with patch #4.

FYI: My pl08x is configured via platform data like this:

static struct pl08x_channel_data pl08x_slave_channels[] = {
        {
                .bus_id = "nand-slc",
                .min_signal = 1, /* SLC NAND Flash */
                .max_signal = 1,
                .periph_buses = PL08X_AHB1,
        },
        {

                .bus_id = "nand-mlc",
                .min_signal = 12, /* MLC NAND Flash */
                .max_signal = 12,
                .periph_buses = PL08X_AHB1,
        },
};

static int pl08x_get_signal(struct pl08x_dma_chan *ch) {
        return ch->cd->min_signal;
}

static void pl08x_put_signal(struct pl08x_dma_chan *ch) {
}

static struct pl08x_platform_data pl08x_pd = {
        .slave_channels = &pl08x_slave_channels[0],
        .num_slave_channels = 2,
        .get_signal = &pl08x_get_signal,
        .put_signal = &pl08x_put_signal,
        .lli_buses = PL08X_AHB1,
        .mem_buses = PL08X_AHB1,
};

[RFC] pl08x: don't use dma_slave_config direction argument

[Russell King - ARM Linux]

On Wed, May 16, 2012 at 03:04:39PM +0200, Roland Stigge wrote:
> On 05/16/2012 02:52 PM, Russell King - ARM Linux wrote:
> >> * SLC driver says:
> >>
> >> lpc32xx-nand 20020000.flash: FIFO not empty!
> >> lpc32xx-nand 20020000.flash: FIFO held data too long
> >> lpc32xx-nand 20020000.flash: DMA FIFO failure
> >>
> >> Any hints, before I find the time to sort out all the details with
> >> amba-pl08x?
> > 
> > Nothing jumps out; it would be useful if you could bisect the small
> > series - I did the conversion in small steps so it should be easy to
> > track down.  And it shouldn't take too long to rebuild between each
> > iteration.  Whatever it is, it's probably going to be a stupid mistake
> > somewhere.
> 
> The problem first appears with patch #4.

I said it would be something silly...

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 940f762..e62dd87 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1415,7 +1415,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
-	cctl |= pl08x_select_bus(src_buses, dst_buses);
+	txd->cctl = cctl | pl08x_select_bus(src_buses, dst_buses);
 
 	if (plchan->cfg.device_fc)
 		tmp = (direction == DMA_MEM_TO_DEV) ? PL080_FLOW_MEM2PER_PER :
@@ -1424,7 +1424,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		tmp = (direction == DMA_MEM_TO_DEV) ? PL080_FLOW_MEM2PER :
 			PL080_FLOW_PER2MEM;
 
-	txd->ccfg = cctl | (tmp << PL080_CONFIG_FLOW_CONTROL_SHIFT);
+	txd->ccfg |= tmp << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 
 	for_each_sg(sgl, sg, sg_len, tmp) {
 		dsg = kzalloc(sizeof(struct pl08x_sg), GFP_NOWAIT);

[RFC] pl08x: don't use dma_slave_config direction argument

[Roland Stigge]

On 05/16/2012 03:17 PM, Russell King - ARM Linux wrote:
>>>> * SLC driver says:
>>>>
>>>> lpc32xx-nand 20020000.flash: FIFO not empty!
>>>> lpc32xx-nand 20020000.flash: FIFO held data too long
>>>> lpc32xx-nand 20020000.flash: DMA FIFO failure
>>>>
>>>> Any hints, before I find the time to sort out all the details with
>>>> amba-pl08x?
>>>
>>> Nothing jumps out; it would be useful if you could bisect the small
>>> series - I did the conversion in small steps so it should be easy to
>>> track down.  And it shouldn't take too long to rebuild between each
>>> iteration.  Whatever it is, it's probably going to be a stupid mistake
>>> somewhere.
>>
>> The problem first appears with patch #4.
> 
> I said it would be something silly...

Works fine now, including series 1-7.

Thanks,

Roland

[RFC] pl08x: don't use dma_slave_config direction argument

[Russell King - ARM Linux]

On Wed, May 16, 2012 at 03:36:50PM +0200, Roland Stigge wrote:
> Works fine now, including series 1-7.

Great, I'll re-post the series, and then maybe we can get some tested-bys
on it.

I think it's too close to the merge window to consider queuing it up for
this window - so I think it'll have to wait until the following window.
That will give it a reasonable amount of time to sit in linux-next and
hopefully any users of it will report any problems.

[RFC] pl08x: don't use dma_slave_config direction argument

[Russell King - ARM Linux]

Linus,

Is there any reason you can see not to do this?

I ultimately need to kill off the passing of struct pl08x_dma_chan to
these functions, because that's preventing me moving the struct into
drivers/dma - which I need to in order to convert amba-pl08x.c to use
virt-dma.c.

The only use of pl08x_dma_chan in the functions below is for the debug
prints, which I suggest we just kill off - or we could change them to
be cd->bus_id, which is the same thing anyway.

diff --git a/arch/arm/mach-realview/core.c b/arch/arm/mach-realview/core.c
index 45868bb..30efe77 100644
--- a/arch/arm/mach-realview/core.c
+++ b/arch/arm/mach-realview/core.c
...
+/* State of the big DMA mux */
+static u32 current_mux;
+static u32 mux_users;
+static DEFINE_SPINLOCK(current_mux_lock);
+
+static int pl081_get_signal(struct pl08x_dma_chan *ch, const struct pl08x_channel_data *cd)
+{
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&current_mux_lock, flags);
+	/*
+	 * We're on the same mux so fine, go ahead!
+	 */
+	if (cd->muxval == current_mux) {
+		mux_users ++;
+		spin_unlock_irqrestore(&current_mux_lock, flags);
+		/* We still have to write it since it may be OFF by default */
+		val = readl(__io_address(REALVIEW_SYS_DMAPSR));
+		val &= 0xFFFFFFFCU;
+		val |= current_mux;
+		writel(val, __io_address(REALVIEW_SYS_DMAPSR));
+		return cd->min_signal;
+	}
+	/*
+	 * If we're not on the same mux and there are already
+	 * users on the other mux setting, tough luck, the client
+	 * can come back and retry or give up and fall back to
+	 * PIO mode.
+	 */
+	if (mux_users) {
+		spin_unlock_irqrestore(&current_mux_lock, flags);
+		return -EBUSY;
+	}
+
+	/* Switch mux setting */
+	current_mux = cd->muxval;
+
+	val = readl(__io_address(REALVIEW_SYS_DMAPSR));
+	val &= 0xFFFFFFFCU;
+	val |= cd->muxval;
+	writel(val, __io_address(REALVIEW_SYS_DMAPSR));
+
+	pr_info("%s: muxing in %s in bank %d writing value "
+		"%08x to register %08x\n",
+		__func__, ch->name, cd->muxval,
+		val, REALVIEW_SYS_DMAPSR);
+
+	spin_unlock_irqrestore(&current_mux_lock, flags);
+
+	return cd->min_signal;
+}
+
+static void pl081_put_signal(struct pl08x_dma_chan *ch, int signal)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&current_mux_lock, flags);
+	mux_users--;
+	spin_unlock_irqrestore(&current_mux_lock, flags);
+}
...
diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index 6bbd74e..5570894 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
...
+/* This is a lock for the above muxing array */
+static DEFINE_SPINLOCK(muxlock);
+
+static int pl080_get_signal(struct pl08x_dma_chan *ch, const struct pl08x_channel_data *cd)
+{
+	pr_debug("requesting DMA signal on channel %s\n", ch->name);
+
+	/*
+	 * The AB926EJ-S is simple - only static assignments
+	 * so the channel is already muxed in and ready to go.
+	 */
+	if (machine_is_versatile_ab())
+		return cd->min_signal;
+
+	/* The PB926EJ-S is hairier */
+	if (machine_is_versatile_pb()) {
+		unsigned long flags;
+		int i;
+
+		if (cd->min_signal > ARRAY_SIZE(pl080_muxtab) ||
+		    cd->max_signal > ARRAY_SIZE(pl080_muxtab) ||
+		    (cd->max_signal < cd->min_signal)) {
+			pr_err("%s: illegal muxing constraints for %s\n",
+			       __func__, ch->name);
+			return -EINVAL;
+		}
+		/* Try to find a signal to use */
+		spin_lock_irqsave(&muxlock, flags);
+		for (i = cd->min_signal; i <= cd->max_signal; i++) {
+			if (!pl080_muxtab[i].user) {
+				u32 val;
+
+				pl080_muxtab[i].user = ch;
+				/* If the channels need to be muxed in, mux them! */
+				if (pl080_muxtab[i].ctrlreg) {
+					val = readl(__io_address(pl080_muxtab[i].ctrlreg));
+					val &= 0xFFFFFF60U;
+					val |= 0x80; /* Turn on muxing */
+					val |= cd->muxval; /* Mux in the right peripheral */
+					writel(val, __io_address(pl080_muxtab[i].ctrlreg));
+					pr_debug("%s: muxing in %s at channel %d writing value "
+						 "%08x to register %08x\n",
+						 __func__, ch->name, i,
+						 val, pl080_muxtab[i].ctrlreg);
+				}
+				spin_unlock_irqrestore(&muxlock, flags);
+				return pl080_muxtab[i].id;
+			}
+		}
+		spin_unlock_irqrestore(&muxlock, flags);
+	}
+
+	return -EBUSY;
+}
+
+static void pl080_put_signal(struct pl08x_dma_chan *ch, int signal)
+{
+	unsigned long flags;
+	int i;
+
+	pr_debug("releasing DMA signal on channel %s\n", ch->name);
+
+	spin_lock_irqsave(&muxlock, flags);
+	for (i = 0; i < ARRAY_SIZE(pl080_muxtab); i++) {
+		if (pl080_muxtab[i].id == signal) {
+			WARN_ON(pl080_muxtab[i].user != ch);
+			pl080_muxtab[i].user = NULL;
+
+			if (pl080_muxtab[i].ctrlreg) {
+				u32 val;
+
+				val = readl(__io_address(pl080_muxtab[i].ctrlreg));
+				val &= 0xFFFFFF60U; /* Disable, select no channel */
+				writel(val, __io_address(pl080_muxtab[i].ctrlreg));
+				pr_debug("%s: muxing out %s@channel %d writing value "
+				       "%08x to register %08x\n",
+				       __func__, ch->name, i,
+				       val, pl080_muxtab[i].ctrlreg);
+			}
+			spin_unlock_irqrestore(&muxlock, flags);
+			return;
+		}
+	}
+	spin_unlock_irqrestore(&muxlock, flags);
+	pr_debug("%s: unable to release muxing on channel %s\n",
+	       __func__, ch->name);
+}
...
diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index e62dd87..afe4640 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -947,7 +947,7 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan,
 	 * Can the platform allow us to use this channel?
 	 */
 	if (plchan->slave && pl08x->pd->get_signal) {
-		ret = pl08x->pd->get_signal(plchan);
+		ret = pl08x->pd->get_signal(plchan, plchan->cd);
 		if (ret < 0) {
 			dev_dbg(&pl08x->adev->dev,
 				"unable to use physical channel %d for transfer on %s due to platform restrictions\n",
@@ -982,7 +982,7 @@ static void release_phy_channel(struct pl08x_dma_chan *plchan)
 	struct pl08x_driver_data *pl08x = plchan->host;
 
 	if ((plchan->phychan->signal >= 0) && pl08x->pd->put_signal) {
-		pl08x->pd->put_signal(plchan);
+		pl08x->pd->put_signal(plchan, plchan->phychan->signal);
 		plchan->phychan->signal = -1;
 	}
 	pl08x_put_phy_channel(pl08x, plchan->phychan);
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index 9331b68..f7312f8 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -147,8 +147,8 @@ struct pl08x_platform_data {
 	const struct pl08x_channel_data *slave_channels;
 	unsigned int num_slave_channels;
 	struct pl08x_channel_data memcpy_channel;
-	int (*get_signal)(struct pl08x_dma_chan *);
-	void (*put_signal)(struct pl08x_dma_chan *);
+	int (*get_signal)(struct pl08x_dma_chan *, const struct pl08x_channel_data *);
+	void (*put_signal)(struct pl08x_dma_chan *, int);
 	u8 lli_buses;
 	u8 mem_buses;
 };

[RFC] pl08x: don't use dma_slave_config direction argument

[Linus Walleij]

On Thu, May 17, 2012 at 4:30 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> Is there any reason you can see not to do this?

Nope, I like the patches for Versatile and RealView, and should have
worked on them myself if I could have free:ed up some time. They
are much needed in-tree since I hear there are more PL08x-based platforms
out there that need signal muxing and the reference designs should provide a
good example of how to do that.

It also provides me a good reason to go and actually test this on the
PB11MPcore as it lands in the next kernel.

> I ultimately need to kill off the passing of struct pl08x_dma_chan to
> these functions, because that's preventing me moving the struct into
> drivers/dma - which I need to in order to convert amba-pl08x.c to use
> virt-dma.c.

OK, no big deal.

> The only use of pl08x_dma_chan in the functions below is for the debug
> prints, which I suggest we just kill off - or we could change them to
> be cd->bus_id, which is the same thing anyway.

Any way is fine with me.

Yours,
Linus Walleij

[RFC] pl08x: don't use dma_slave_config direction argument

[Russell King - ARM Linux]

On Wed, May 16, 2012 at 12:04:51PM +0100, Russell King - ARM Linux wrote:
> This series removes the dependence on the dma_slave_config direction
> argument for the PL08x DMA engine driver, and in doing so, we end up
> with less code in the driver.

I've just been testing this on the Versatile PB926 with my only working
peripheral (PL011) and the runtime PM support which was added to the PL08x
driver is totally breaking this setup.

It's causing lockdep to spit out:

[ INFO: inconsistent lock state ]
inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
init/1 [HC0[0]:SC0[0]:HE1:SE1] takes:
 (&port_lock_key){?.....}, at: [<c01b59ac>] uart_start+0x24/0x74
{IN-HARDIRQ-W} state was registered at:
...
  [<c01b97f8>] pl011_int+0x1c/0x4a8
...
stack backtrace:
[<c00561b4>] (trace_hardirqs_on+0x0/0x18) from [<c02dfc6c>] (_raw_spin_unlock_irq+0x30/0x64)
[<c02dfc3c>] (_raw_spin_unlock_irq+0x0/0x64) from [<c01c6214>] (__rpm_callback+0x30/0x60)
[<c01c61e4>] (__rpm_callback+0x0/0x60) from [<c01c733c>] (rpm_resume+0x3e0/0x568)
...
[<c019b7a8>] (pl08x_prep_slave_sg+0x0/0x270) from [<c01b953c>] (pl011_dma_tx_refill+0x104/0x1c8)
[<c01b9438>] (pl011_dma_tx_refill+0x0/0x1c8) from [<c01b9704>] (pl011_start_tx+0x38/0x110)

Basically, uart_start() has taken the port spinlock with IRQs disabled.
We then go through the PL011 start_tx function, eventually into the
PL08x driver to start the transmit DMA.

The PL08x driver then calls pm_runtime_get_sync(), when then invokes
__rpm_callback.  This unconditionally enables IRQs underneath the
per-port spinlock, allowing potential deadlock with PL011's interrupt
handler.

So, the only solution I can see is to strip out all the runtime PM stuff
from PL08x, which is buggy anyway.

And it's taken all afternoon to work out what's going on here... because
it can't spit that debug out because it deadlocks in the PL011 console
write function on the very same lock...  This is extremely nasty.

[RFC] pl08x: don't use dma_slave_config direction argument

[Linus Walleij]

On Thu, May 17, 2012 at 7:03 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> I've just been testing this on the Versatile PB926 with my only working
> peripheral (PL011) and the runtime PM support which was added to the PL08x
> driver is totally breaking this setup.

Ouch! That was a nasty bug.

> So, the only solution I can see is to strip out all the runtime PM stuff
> from PL08x, which is buggy anyway.

I'd just kill it FTM, maybe the patch can simply be reverted?

What worries me is that this is such a straight-forward use of the runtime PM
API, the semantic difficulties with this API has caused me a lot of headache
recently. It just looks simple but isn't.

*_get_sync() should never be called in interrupt context, OK, but this is even
nastier, no calling under any spinlock ... how can we ever guarantee that?

Yours,
Linus Walleij