[PATCH V2 00/20] dmaengine/amba-pl08x updates

[PATCH V2 00/20] dmaengine/amba-pl08x updates

[Viresh Kumar]

Hi Linus,

This patchset contains patches for ARM's pl08x DMA controller driver.
Main changes introduced by this patchset are:
- Formatting fixes
- Logical Fixes
- Support for Peripheral as flow controller
- Support for sg_len to be greater than 1 for slave transfers
- Schedule tasklet for error interrupts
- remove pl08x_pre_boundary() routine
- support for runtime PM
- etc

This patchset is tested on SPEAr600 for following tests:
- memory to memory copy (using drivers/dma/dmatest, upto 16 threads per channel)
- peripheral to memory and vice versa (Peripheral: JPEG)

This was rebased on Vinod's slave-dma.git/next, over following patch:

commit 1ae105aa7416087f2920c35c3cd16831d0d09c9c
Merge: 02f8c6a 5a42fb9
Author: Vinod Koul <vinod.koul@intel.com>
Date:   Wed Jul 27 20:43:21 2011 +0530

    Merge branch 'next' into for-linus-3.0

Changes Since V1:
- Dropped many formatting fixes which were added earlier, as they made things
  look even bad.
- Fixed tabbing issues in PATCH "Add prep_single_byte_llis() routine"
- Many comments are updated throughout the patchset.
- Fixed interrupt handler to report spurious interrupt for channels which are
  not used.
- Clk enabling/disabling done via runtime PM
- Complete mask of flow controller is not passed from platform data now,
  direction is decided at runtime.
- pl08x_free_txd() called with locks.
- Two new patches are added:
  - Rearrange inclusion of header files in ascending order
  - pass (*ptr) to sizeof() instead of (struct xyz)


Hope i haven't missed any review comments.

Viresh Kumar (20):
  ARM: asm/pl080.h: Protect against multiple inclusion of header file
  dmaengine/amba-pl08x: Resolve formatting issues
  dmaengine/amba-pl08x: Rearrange inclusion of header files in
    ascending order
  dmaengine/amba-pl08x: pass (*ptr) to sizeof() instead of (struct xyz)
  dmaengine/amba-pl08x: Complete doc comment for struct pl08x_txd
  dmaengine/amba-pl08x: Remove redundant comment and rewrite original
  dmaengine/amba-pl08x: Changing few prints to dev_dbg from dev_info
  dmaengine/amba-pl08x: support runtime PM
  dmaengine/amba-pl08x: Simplify pl08x_ensure_on()
  dmaengine/amba-pl08x: No need to check "ch->signal < 0"
  dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt
  dmaengine/amba-pl08x: Get rid of pl08x_pre_boundary()
  dmaengine/amba-pl08x: max_bytes_per_lli is TRANSFER_SIZE * src_width
    (not MIN(width))
  dmaengine/amba-pl08x: Add prep_single_byte_llis() routine
  dmaengine/amba-pl08x: Align lli_len to max(src.width, dst.width)
  dmaengine/amba-pl08x: Choose peripheral bus as master bus
  dmaengine/amba-pl08x: Pass flow controller information with slave
    channel data
  dmaengine/amba-pl08x: Add support for sg len greater than one for
    slave transfers
  dmaengine/amba-pl08x: Check txd->llis_va before freeing dma_pool
  dmaengine/amba-pl08x: Call pl08x_free_txd() instead of calling
    kfree() directly

 arch/arm/include/asm/hardware/pl080.h |    4 +
 drivers/dma/amba-pl08x.c              |  623 ++++++++++++++++-----------------
 include/linux/amba/pl08x.h            |   30 ++-
 3 files changed, 332 insertions(+), 325 deletions(-)

-- 
1.7.2.2

[PATCH V2 01/20] ARM: asm/pl080.h: Protect against multiple inclusion of header file

[Viresh Kumar]

<asm/hardware/pl080.h> doesn't have protection to deal with multiple inclusion.
And so we get compilation errors in cases where this file is included more than
once. This patch adds #ifdefs at the top of file to protect it against multiple
inclusions.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/include/asm/hardware/pl080.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/hardware/pl080.h b/arch/arm/include/asm/hardware/pl080.h
index f35b86e..fc68cc9 100644
--- a/arch/arm/include/asm/hardware/pl080.h
+++ b/arch/arm/include/asm/hardware/pl080.h
@@ -21,6 +21,9 @@
  * OneNAND features.
 */
 
+#ifndef ASM_PL080_H
+#define ASM_PL080_H
+
 #define PL080_INT_STATUS			(0x00)
 #define PL080_TC_STATUS				(0x04)
 #define PL080_TC_CLEAR				(0x08)
@@ -138,3 +141,4 @@ struct pl080s_lli {
 	u32	control1;
 };
 
+#endif /* ASM_PL080_H */
-- 
1.7.2.2

[PATCH V2 02/20] dmaengine/amba-pl08x: Resolve formatting issues

[Viresh Kumar]

There were few formatting related issues in code. This patch fixes them.
Fixes include:
- Remove extra blank lines
- align code to 80 cols
- combine several lines to one line

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   41 ++++++++++++++++-------------------------
 1 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 196a737..4c4a309 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -125,7 +125,8 @@ struct pl08x_lli {
  * @phy_chans: array of data for the physical channels
  * @pool: a pool for the LLI descriptors
  * @pool_ctr: counter of LLIs in the pool
- * @lli_buses: bitmask to or in to LLI pointer selecting AHB port for LLI fetches
+ * @lli_buses: bitmask to or in to LLI pointer selecting AHB port for LLI
+ * fetches
  * @mem_buses: set to indicate memory transfers on AHB2.
  * @lock: a spinlock for this struct
  */
@@ -271,7 +272,6 @@ static void pl08x_resume_phy_chan(struct pl08x_phy_chan *ch)
 	writel(val, ch->base + PL080_CH_CONFIG);
 }
 
-
 /*
  * pl08x_terminate_phy_chan() stops the channel, clears the FIFO and
  * clears any pending interrupt status.  This should not be used for
@@ -546,7 +546,8 @@ static void pl08x_fill_lli_for_desc(struct pl08x_lli_build_data *bd,
 	llis_va[num_llis].cctl = cctl;
 	llis_va[num_llis].src = bd->srcbus.addr;
 	llis_va[num_llis].dst = bd->dstbus.addr;
-	llis_va[num_llis].lli = llis_bus + (num_llis + 1) * sizeof(struct pl08x_lli);
+	llis_va[num_llis].lli = llis_bus + (num_llis + 1) *
+		sizeof(struct pl08x_lli);
 	llis_va[num_llis].lli |= bd->lli_bus;
 
 	if (cctl & PL080_CONTROL_SRC_INCR)
@@ -583,12 +584,10 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	struct pl08x_lli_build_data bd;
 	int num_llis = 0;
 	u32 cctl;
-	size_t max_bytes_per_lli;
-	size_t total_bytes = 0;
+	size_t max_bytes_per_lli, total_bytes = 0;
 	struct pl08x_lli *llis_va;
 
-	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT,
-				      &txd->llis_bus);
+	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__);
 		return 0;
@@ -779,7 +778,6 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				total_bytes += lli_len;
 			}
 
-
 			if (odd_bytes) {
 				/*
 				 * Creep past the boundary, maintaining
@@ -916,9 +914,7 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan,
 	 * need, but for slaves the physical signals may be muxed!
 	 * Can the platform allow us to use this channel?
 	 */
-	if (plchan->slave &&
-	    ch->signal < 0 &&
-	    pl08x->pd->get_signal) {
+	if (plchan->slave && ch->signal < 0 && pl08x->pd->get_signal) {
 		ret = pl08x->pd->get_signal(plchan);
 		if (ret < 0) {
 			dev_dbg(&pl08x->adev->dev,
@@ -1007,10 +1003,8 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_interrupt(
  * If slaves are relying on interrupts to signal completion this function
  * must not be called with interrupts disabled.
  */
-static enum dma_status
-pl08x_dma_tx_status(struct dma_chan *chan,
-		    dma_cookie_t cookie,
-		    struct dma_tx_state *txstate)
+static enum dma_status pl08x_dma_tx_status(struct dma_chan *chan,
+		dma_cookie_t cookie, struct dma_tx_state *txstate)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
 	dma_cookie_t last_used;
@@ -1588,8 +1582,8 @@ static void pl08x_tasklet(unsigned long data)
 		 */
 		list_for_each_entry(waiting, &pl08x->memcpy.channels,
 				    chan.device_node) {
-		  if (waiting->state == PL08X_CHAN_WAITING &&
-			    waiting->waiting != NULL) {
+			if (waiting->state == PL08X_CHAN_WAITING &&
+				waiting->waiting != NULL) {
 				int ret;
 
 				/* This should REALLY not fail now */
@@ -1684,9 +1678,7 @@ static void pl08x_dma_slave_init(struct pl08x_dma_chan *chan)
  * Make a local wrapper to hold required data
  */
 static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
-					   struct dma_device *dmadev,
-					   unsigned int channels,
-					   bool slave)
+		struct dma_device *dmadev, unsigned int channels, bool slave)
 {
 	struct pl08x_dma_chan *chan;
 	int i;
@@ -1836,9 +1828,9 @@ static const struct file_operations pl08x_debugfs_operations = {
 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);
+	(void) debugfs_create_file(dev_name(&pl08x->adev->dev),
+			S_IFREG | S_IRUGO, NULL, pl08x,
+			&pl08x_debugfs_operations);
 }
 
 #else
@@ -1973,8 +1965,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 
 	/* Register slave channels */
 	ret = pl08x_dma_init_virtual_channels(pl08x, &pl08x->slave,
-					      pl08x->pd->num_slave_channels,
-					      true);
+			pl08x->pd->num_slave_channels, true);
 	if (ret <= 0) {
 		dev_warn(&pl08x->adev->dev,
 			"%s failed to enumerate slave channels - %d\n",
-- 
1.7.2.2

[PATCH V2 03/20] dmaengine/amba-pl08x: Rearrange inclusion of header files in ascending order

[Viresh Kumar]

Header files included in driver are not present in alphabetical order. Rearrange
them in alphabetical order.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 4c4a309..8e2056b 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -74,19 +74,18 @@
  * Global TODO:
  * - Break out common code from arch/arm/mach-s3c64xx and share
  */
-#include <linux/device.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/interrupt.h>
-#include <linux/slab.h>
-#include <linux/delay.h>
-#include <linux/dmapool.h>
-#include <linux/dmaengine.h>
 #include <linux/amba/bus.h>
 #include <linux/amba/pl08x.h>
 #include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dmaengine.h>
+#include <linux/dmapool.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
 #include <linux/seq_file.h>
-
+#include <linux/slab.h>
 #include <asm/hardware/pl080.h>
 
 #define DRIVER_NAME	"pl08xdmac"
-- 
1.7.2.2

[PATCH V2 04/20] dmaengine/amba-pl08x: pass (*ptr) to sizeof() instead of (struct xyz)

[Viresh Kumar]

As mentioned in Documentation/CodingStyle,

The preferred form for passing a size of a struct is the following:
   p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.

This patch replaces (struct xyz) with *ptr at several occurrences in driver.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 8e2056b..01c2f50 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1293,7 +1293,7 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 static struct pl08x_txd *pl08x_get_txd(struct pl08x_dma_chan *plchan,
 	unsigned long flags)
 {
-	struct pl08x_txd *txd = kzalloc(sizeof(struct pl08x_txd), GFP_NOWAIT);
+	struct pl08x_txd *txd = kzalloc(sizeof(*txd), GFP_NOWAIT);
 
 	if (txd) {
 		dma_async_tx_descriptor_init(&txd->tx, &plchan->chan);
@@ -1690,7 +1690,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 	 * to cope with that situation.
 	 */
 	for (i = 0; i < channels; i++) {
-		chan = kzalloc(sizeof(struct pl08x_dma_chan), GFP_KERNEL);
+		chan = kzalloc(sizeof(*chan), GFP_KERNEL);
 		if (!chan) {
 			dev_err(&pl08x->adev->dev,
 				"%s no memory for channel\n", __func__);
@@ -1850,7 +1850,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 		return ret;
 
 	/* Create the driver state holder */
-	pl08x = kzalloc(sizeof(struct pl08x_driver_data), GFP_KERNEL);
+	pl08x = kzalloc(sizeof(*pl08x), GFP_KERNEL);
 	if (!pl08x) {
 		ret = -ENOMEM;
 		goto out_no_pl08x;
@@ -1929,7 +1929,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	}
 
 	/* Initialize physical channels */
-	pl08x->phy_chans = kmalloc((vd->channels * sizeof(struct pl08x_phy_chan)),
+	pl08x->phy_chans = kmalloc((vd->channels * sizeof(*pl08x->phy_chans)),
 			GFP_KERNEL);
 	if (!pl08x->phy_chans) {
 		dev_err(&adev->dev, "%s failed to allocate "
-- 
1.7.2.2

[PATCH V2 05/20] dmaengine/amba-pl08x: Complete doc comment for struct pl08x_txd

[Viresh Kumar]

Doc comment for struct pl08x_txd was incomplete. Complete that.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 include/linux/amba/pl08x.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index e6e28f3..cd8f629 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -105,8 +105,16 @@ struct pl08x_phy_chan {
 
 /**
  * struct pl08x_txd - wrapper for struct dma_async_tx_descriptor
+ * @tx: async tx descriptor
+ * @node: node for txd list for channels
+ * @src_addr: src address of txd
+ * @dst_addr: dst address of txd
+ * @len: transfer len in bytes
+ * @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;
-- 
1.7.2.2

[PATCH V2 06/20] dmaengine/amba-pl08x: Remove redundant comment and rewrite original

[Viresh Kumar]

Similar comment is present over routine also pl08x_choose_master_bus(). Keeping
one of them. Also rewrite that comment to convey message clearly.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 01c2f50..6c52959 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -497,9 +497,13 @@ struct pl08x_lli_build_data {
 };
 
 /*
- * Autoselect a master bus to use for the transfer this prefers the
- * destination bus if both available if fixed address on one bus the
- * other will be chosen
+ * Autoselect a master bus to use for the transfer. Slave will be the chosen as
+ * victim in case src & dest are not similarly aligned. i.e. If after aligning
+ * masters address with width requirements of transfer (by sending few byte by
+ * byte data), slave is still not aligned, then its width will be reduced to
+ * BYTE.
+ * - prefers the destination bus if both available
+ * - if fixed address on one bus the other will be chosen
  */
 static void pl08x_choose_master_bus(struct pl08x_lli_build_data *bd,
 	struct pl08x_bus_data **mbus, struct pl08x_bus_data **sbus, u32 cctl)
@@ -625,11 +629,6 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	/* We need to count this down to zero */
 	bd.remainder = txd->len;
 
-	/*
-	 * Choose bus to align to
-	 * - prefers destination bus if both available
-	 * - if fixed address on one bus chooses other
-	 */
 	pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
 
 	dev_vdbg(&pl08x->adev->dev, "src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu llimax=%zu\n",
-- 
1.7.2.2

[PATCH V2 07/20] dmaengine/amba-pl08x: Changing few prints to dev_dbg from dev_info

[Viresh Kumar]

For 8 memory and 16 slave channels 35 boot print lines are printed. And that is
too much. Most of this would be more useful for debugging. So moving few of them
to dev_dbg instead of dev_info. Now only 3 prints will be printed.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 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 6c52959..3fbaf0e 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1717,7 +1717,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 			kfree(chan);
 			continue;
 		}
-		dev_info(&pl08x->adev->dev,
+		dev_dbg(&pl08x->adev->dev,
 			 "initialize virtual channel \"%s\"\n",
 			 chan->name);
 
@@ -1945,7 +1945,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 		spin_lock_init(&ch->lock);
 		ch->serving = NULL;
 		ch->signal = -1;
-		dev_info(&adev->dev,
+		dev_dbg(&adev->dev,
 			 "physical channel %d is %s\n", i,
 			 pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE");
 	}
-- 
1.7.2.2

[PATCH V2 08/20] dmaengine/amba-pl08x: support runtime PM

[Viresh Kumar]

Insert notifiers for the runtime PM API. With this the runtime PM layer kicks in
to action where used. This will also handle enabling/disabling of interface
clock (Code will be added in amba/bus.c by Russell King).

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 3fbaf0e..428a67b 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -84,6 +84,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <asm/hardware/pl080.h>
@@ -879,11 +880,19 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x,
  */
 static int pl08x_alloc_chan_resources(struct dma_chan *chan)
 {
+	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
+	struct pl08x_driver_data *pl08x = plchan->host;
+
+	pm_runtime_resume(&pl08x->adev->dev);
 	return 0;
 }
 
 static void pl08x_free_chan_resources(struct dma_chan *chan)
 {
+	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
+	struct pl08x_driver_data *pl08x = plchan->host;
+
+	pm_runtime_suspend(&pl08x->adev->dev);
 }
 
 /*
@@ -1993,6 +2002,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n",
 		 amba_part(adev), amba_rev(adev),
 		 (unsigned long long)adev->res.start, adev->irq[0]);
+
+	pm_runtime_suspend(&adev->dev);
 	return 0;
 
 out_no_slave_reg:
-- 
1.7.2.2

[PATCH V2 09/20] dmaengine/amba-pl08x: Simplify pl08x_ensure_on()

[Viresh Kumar]

Simply writing 1 on bit 0 is sufficient instead of reading and clearing bits.
Also as per manual, for bit 3-31 of DMACConfiguration register:
"read undefined, write as 0"

So, we must not rely on values read from this registers bit 3-31.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/amba-pl08x.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 428a67b..da17249 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1507,13 +1507,7 @@ bool pl08x_filter_id(struct dma_chan *chan, void *chan_id)
  */
 static void pl08x_ensure_on(struct pl08x_driver_data *pl08x)
 {
-	u32 val;
-
-	val = readl(pl08x->base + PL080_CONFIG);
-	val &= ~(PL080_CONFIG_M2_BE | PL080_CONFIG_M1_BE | PL080_CONFIG_ENABLE);
-	/* We implicitly clear bit 1 and that means little-endian mode */
-	val |= PL080_CONFIG_ENABLE;
-	writel(val, pl08x->base + PL080_CONFIG);
+	writel(PL080_CONFIG_ENABLE, pl08x->base + PL080_CONFIG);
 }
 
 static void pl08x_unmap_buffers(struct pl08x_txd *txd)
-- 
1.7.2.2

[PATCH V2 10/20] dmaengine/amba-pl08x: No need to check "ch->signal < 0"

[Viresh Kumar]

We have just executed following in pl08x_get_phy_channel():
	ch->signal = -1;

We don't have to compare "ch->signal < 0", as this will always be true.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/amba-pl08x.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index da17249..f463841 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -921,7 +921,7 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan,
 	 * need, but for slaves the physical signals may be muxed!
 	 * Can the platform allow us to use this channel?
 	 */
-	if (plchan->slave && ch->signal < 0 && pl08x->pd->get_signal) {
+	if (plchan->slave && pl08x->pd->get_signal) {
 		ret = pl08x->pd->get_signal(plchan);
 		if (ret < 0) {
 			dev_dbg(&pl08x->adev->dev,
-- 
1.7.2.2

[PATCH V2 11/20] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[Viresh Kumar]

Currently, if error interrupt occurs, nothing is done in interrupt handler (just
clearing the interrupts). We must somehow indicate this to the user that DMA is
over, due to ERR interrupt or TC interrupt.

So, this patch just schedules existing tasklet, with a print showing error
interrupt has occurred on which channels.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   44 +++++++++++++++++++++++---------------------
 1 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index f463841..1b7a60f 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1624,38 +1624,40 @@ static void pl08x_tasklet(unsigned long data)
 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);
+	u32 mask = 0, err, tc, i;
+
+	/* check & clear - ERR & TC interrupts */
+	err = readl(pl08x->base + PL080_ERR_STATUS);
+	if (err) {
+		dev_err(&pl08x->adev->dev, "%s error interrupt, register value 0x%08x\n",
+			__func__, err);
+		writel(err, pl08x->base + PL080_ERR_CLEAR);
 	}
-	val = readl(pl08x->base + PL080_INT_STATUS);
+	tc = readl(pl08x->base + PL080_INT_STATUS);
+	if (tc)
+		writel(tc, pl08x->base + PL080_TC_CLEAR);
+
+	if (!err && !tc)
+		return IRQ_NONE;
+
 	for (i = 0; i < pl08x->vd->channels; i++) {
-		if ((1 << i) & val) {
+		if (((1 << i) & err) || ((1 << i) & tc)) {
 			/* Locate physical channel */
 			struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i];
 			struct pl08x_dma_chan *plchan = phychan->serving;
 
+			if (!plchan) {
+				dev_err(&pl08x->adev->dev,
+					"%s Error TC interrupt on unused channel: 0x%08x\n",
+					__func__, i);
+				continue;
+			}
+
 			/* 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;
 }
-- 
1.7.2.2

[PATCH V2 12/20] dmaengine/amba-pl08x: Get rid of pl08x_pre_boundary()

[Viresh Kumar]

Pl080 Manual says: "Bursts do not cross the 1KB address boundary"

We can program the controller to cross 1 KB boundary on a burst and controller
can take care of this boundary condition by itself.

Following is the discussion with ARM Technical Support Guys (David):
[Viresh] Manual says: "Bursts do not cross the 1KB address boundary"

What does that actually mean? As, Maximum size transferable with a single LLI is
4095 * 4 =16380 ~ 16KB. So, if we don't have src/dest address aligned to burst
size, we can't use this big of an LLI.

[David] There is a difference between bursts describing the total data
transferred by the DMA controller and AHB bursts. Bursts described by the
programmable parameters in the PL080 have no direct connection with the bursts
that are seen on the AHB bus.

The statement that "Bursts do not cross the 1KB address boundary" in the TRM is
referring to AHB bursts, where this limitation is a requirement of the AHB spec.
You can still issue bursts within the PL080 that are in excess of 1KB. The
PL080 will make sure that its bursts are broken down into legal AHB bursts which
will be formatted to ensure that no AHB burst crosses a 1KB boundary.

Based on above discussion, this patch removes all code related to 1 KB boundary
as we are not required to handle this in driver.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/amba-pl08x.c   |  141 +++++--------------------------------------
 include/linux/amba/pl08x.h |    2 -
 2 files changed, 17 insertions(+), 126 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 1b7a60f..e00a9b5 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -149,14 +149,6 @@ struct pl08x_driver_data {
  * PL08X specific defines
  */
 
-/*
- * Memory boundaries: the manual for PL08x says that the controller
- * cannot read past a 1KiB boundary, so these defines are used to
- * create transfer LLIs that do not cross such boundaries.
- */
-#define PL08X_BOUNDARY_SHIFT		(10)	/* 1KB 0x400 */
-#define PL08X_BOUNDARY_SIZE		(1 << PL08X_BOUNDARY_SHIFT)
-
 /* Size (bytes) of each LLI buffer allocated for one transfer */
 # define PL08X_LLI_TSFR_SIZE	0x2000
 
@@ -565,18 +557,6 @@ static void pl08x_fill_lli_for_desc(struct pl08x_lli_build_data *bd,
 }
 
 /*
- * Return number of bytes to fill to boundary, or len.
- * This calculation works for any value of addr.
- */
-static inline size_t pl08x_pre_boundary(u32 addr, size_t len)
-{
-	size_t boundary_len = PL08X_BOUNDARY_SIZE -
-			(addr & (PL08X_BOUNDARY_SIZE - 1));
-
-	return min(boundary_len, len);
-}
-
-/*
  * This fills in the table of LLIs for the transfer descriptor
  * Note that we assume we never have to change the burst sizes
  * Return 0 for error
@@ -682,118 +662,30 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		 * width left
 		 */
 		while (bd.remainder > (mbus->buswidth - 1)) {
-			size_t lli_len, target_len, tsize, odd_bytes;
+			size_t lli_len, tsize;
 
 			/*
 			 * If enough left try to send max possible,
 			 * otherwise try to send the remainder
 			 */
-			target_len = min(bd.remainder, max_bytes_per_lli);
-
+			lli_len = min(bd.remainder, max_bytes_per_lli);
 			/*
-			 * Set bus lengths for incrementing buses to the
-			 * number of bytes which fill to next memory boundary,
-			 * limiting on the target length calculated above.
+			 * Check against minimum bus alignment: Calculate actual
+			 * transfer size in relation to bus width and get a
+			 * maximum remainder of the smallest bus width - 1
 			 */
-			if (cctl & PL080_CONTROL_SRC_INCR)
-				bd.srcbus.fill_bytes =
-					pl08x_pre_boundary(bd.srcbus.addr,
-						target_len);
-			else
-				bd.srcbus.fill_bytes = target_len;
-
-			if (cctl & PL080_CONTROL_DST_INCR)
-				bd.dstbus.fill_bytes =
-					pl08x_pre_boundary(bd.dstbus.addr,
-						target_len);
-			else
-				bd.dstbus.fill_bytes = target_len;
-
-			/* Find the nearest */
-			lli_len	= min(bd.srcbus.fill_bytes,
-				      bd.dstbus.fill_bytes);
-
-			BUG_ON(lli_len > bd.remainder);
-
-			if (lli_len <= 0) {
-				dev_err(&pl08x->adev->dev,
-					"%s lli_len is %zu, <= 0\n",
-						__func__, lli_len);
-				return 0;
-			}
-
-			if (lli_len == target_len) {
-				/*
-				 * Can send what we wanted.
-				 * Maintain alignment
-				 */
-				lli_len	= (lli_len/mbus->buswidth) *
-							mbus->buswidth;
-				odd_bytes = 0;
-			} else {
-				/*
-				 * So now we know how many bytes to transfer
-				 * to get to the nearest boundary.  The next
-				 * LLI will past the boundary.  However, we
-				 * may be working to a boundary on the slave
-				 * bus.  We need to ensure the master stays
-				 * aligned, and that we are working in
-				 * multiples of the bus widths.
-				 */
-				odd_bytes = lli_len % mbus->buswidth;
-				lli_len -= odd_bytes;
-
-			}
-
-			if (lli_len) {
-				/*
-				 * Check against minimum bus alignment:
-				 * Calculate actual transfer size in relation
-				 * to bus width an get a maximum remainder of
-				 * the smallest bus width - 1
-				 */
-				/* FIXME: use round_down()? */
-				tsize = lli_len / min(mbus->buswidth,
-						      sbus->buswidth);
-				lli_len	= tsize * min(mbus->buswidth,
-						      sbus->buswidth);
-
-				if (target_len != lli_len) {
-					dev_vdbg(&pl08x->adev->dev,
-					"%s can't send what we want. Desired 0x%08zx, lli of 0x%08zx bytes in txd of 0x%08zx\n",
-					__func__, target_len, lli_len, txd->len);
-				}
-
-				cctl = pl08x_cctl_bits(cctl,
-						       bd.srcbus.buswidth,
-						       bd.dstbus.buswidth,
-						       tsize);
-
-				dev_vdbg(&pl08x->adev->dev,
-					"%s fill lli with single lli chunk of size 0x%08zx (remainder 0x%08zx)\n",
-					__func__, lli_len, bd.remainder);
-				pl08x_fill_lli_for_desc(&bd, num_llis++,
-					lli_len, cctl);
-				total_bytes += lli_len;
-			}
+			tsize = lli_len / min(mbus->buswidth, sbus->buswidth);
+			lli_len	= tsize * min(mbus->buswidth, sbus->buswidth);
 
-			if (odd_bytes) {
-				/*
-				 * Creep past the boundary, maintaining
-				 * master alignment
-				 */
-				int j;
-				for (j = 0; (j < mbus->buswidth)
-						&& (bd.remainder); j++) {
-					cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
-					dev_vdbg(&pl08x->adev->dev,
-						"%s align with boundary, single byte (remain 0x%08zx)\n",
-						__func__, bd.remainder);
-					pl08x_fill_lli_for_desc(&bd,
-						num_llis++, 1, cctl);
-					total_bytes++;
-				}
-			}
+			dev_vdbg(&pl08x->adev->dev,
+				"%s fill lli with single lli chunk of "
+				"size 0x%08zx (remainder 0x%08zx)\n",
+				__func__, lli_len, bd.remainder);
+
+			cctl = pl08x_cctl_bits(cctl, bd.srcbus.buswidth,
+					bd.dstbus.buswidth, tsize);
+			pl08x_fill_lli_for_desc(&bd, num_llis++, lli_len, cctl);
+			total_bytes += lli_len;
 		}
 
 		/*
@@ -808,6 +700,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			total_bytes++;
 		}
 	}
+
 	if (total_bytes != txd->len) {
 		dev_err(&pl08x->adev->dev,
 			"%s size of encoded lli:s don't match total txd, transferred 0x%08zx from size 0x%08zx\n",
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index cd8f629..ecd17f5 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -77,13 +77,11 @@ struct pl08x_channel_data {
  * @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
- * @fill_bytes: bytes required to fill to the next bus memory boundary
  */
 struct pl08x_bus_data {
 	dma_addr_t addr;
 	u8 maxwidth;
 	u8 buswidth;
-	size_t fill_bytes;
 };
 
 /**
-- 
1.7.2.2

[PATCH V2 13/20] dmaengine/amba-pl08x: max_bytes_per_lli is TRANSFER_SIZE * src_width (not MIN(width))

[Viresh Kumar]

max_bytes_per_lli = bd.srcbus.buswidth * PL080_CONTROL_TRANSFER_SIZE_MASK;
This is confirmed by ARM support guys.

Below is summary of mail exchange with them:

[Viresh] What is the total data to be transferred in case source and destination
bus widths are different. Suppose, source bus width is 2 bytes and destination
is 4 bytes. Now in order to transfer 80 bytes, what should be value of
TransferSize field in control reg: 40? or 20?.

[David from ARM] The value that is programmed into the TransferSize field should
be the number of <SourceWidth> transfers needed to achieve the required data
transfer.

So, to transfer 80 bytes, with a Source Width of 2, the TransferSize field =
should be programmed with:

        Total transfer size
        ------------------- = 40
          <source width>

[Viresh] Will this change if source is 4 bytes and dest is 2?

[David] Yes - the calculation then becomes:

        Total transfer size
        ------------------- =20
          <source width>

Also, max_bytes_per_lli must be calculated after fixing src and dest widths not
before that. So move this code to the correct place.

This patch also removes max_bytes_per_lli from earlier print message, as till
that point max_bytes_per_lli is unknown.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/amba-pl08x.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index e00a9b5..c29e55c 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -601,23 +601,17 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	bd.srcbus.buswidth = bd.srcbus.maxwidth;
 	bd.dstbus.buswidth = bd.dstbus.maxwidth;
 
-	/*
-	 * Bytes transferred == tsize * MIN(buswidths), not max(buswidths)
-	 */
-	max_bytes_per_lli = min(bd.srcbus.buswidth, bd.dstbus.buswidth) *
-		PL080_CONTROL_TRANSFER_SIZE_MASK;
-
 	/* We need to count this down to zero */
 	bd.remainder = txd->len;
 
 	pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
 
-	dev_vdbg(&pl08x->adev->dev, "src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu llimax=%zu\n",
+	dev_vdbg(&pl08x->adev->dev, "src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu\n",
 		 bd.srcbus.addr, cctl & PL080_CONTROL_SRC_INCR ? "+" : "",
 		 bd.srcbus.buswidth,
 		 bd.dstbus.addr, cctl & PL080_CONTROL_DST_INCR ? "+" : "",
 		 bd.dstbus.buswidth,
-		 bd.remainder, max_bytes_per_lli);
+		 bd.remainder);
 	dev_vdbg(&pl08x->adev->dev, "mbus=%s sbus=%s\n",
 		 mbus == &bd.srcbus ? "src" : "dst",
 		 sbus == &bd.srcbus ? "src" : "dst");
@@ -657,6 +651,10 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			sbus->buswidth = 1;
 		}
 
+		/* Bytes transferred = tsize * src width, not MIN(buswidths) */
+		max_bytes_per_lli = bd.srcbus.buswidth *
+			PL080_CONTROL_TRANSFER_SIZE_MASK;
+
 		/*
 		 * Make largest possible LLIs until less than one bus
 		 * width left
-- 
1.7.2.2

[PATCH V2 14/20] dmaengine/amba-pl08x: Add prep_single_byte_llis() routine

[Viresh Kumar]

Code for creating single byte llis is present at several places. Create a
routine to avoid code redundancy.

Also, we don't need one lli per single byte transfer, we can have single lli to
do all single byte transfer.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   61 +++++++++++++++++++++++++---------------------
 1 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index c29e55c..0bdf3b8 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -556,6 +556,14 @@ static void pl08x_fill_lli_for_desc(struct pl08x_lli_build_data *bd,
 	bd->remainder -= len;
 }
 
+static inline void prep_byte_width_lli(struct pl08x_lli_build_data *bd,
+		u32 *cctl, u32 len, int num_llis, size_t *total_bytes)
+{
+	*cctl = pl08x_cctl_bits(*cctl, 1, 1, len);
+	pl08x_fill_lli_for_desc(bd, num_llis, len, *cctl);
+	(*total_bytes) += len;
+}
+
 /*
  * This fills in the table of LLIs for the transfer descriptor
  * Note that we assume we never have to change the burst sizes
@@ -567,7 +575,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	struct pl08x_bus_data *mbus, *sbus;
 	struct pl08x_lli_build_data bd;
 	int num_llis = 0;
-	u32 cctl;
+	u32 cctl, early_bytes = 0;
 	size_t max_bytes_per_lli, total_bytes = 0;
 	struct pl08x_lli *llis_va;
 
@@ -616,29 +624,27 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		 mbus == &bd.srcbus ? "src" : "dst",
 		 sbus == &bd.srcbus ? "src" : "dst");
 
-	if (txd->len < mbus->buswidth) {
-		/* Less than a bus width available - send as single bytes */
-		while (bd.remainder) {
-			dev_vdbg(&pl08x->adev->dev,
-				 "%s single byte LLIs for a transfer of "
-				 "less than a bus width (remain 0x%08x)\n",
-				 __func__, bd.remainder);
-			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
-			pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
-			total_bytes++;
-		}
-	} else {
-		/* Make one byte LLIs until master bus is aligned */
-		while ((mbus->addr) % (mbus->buswidth)) {
-			dev_vdbg(&pl08x->adev->dev,
-				"%s adjustment lli for less than bus width "
-				 "(remain 0x%08x)\n",
-				 __func__, bd.remainder);
-			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
-			pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
-			total_bytes++;
-		}
+	/*
+	 * Send byte by byte for following cases
+	 * - Less than a bus width available
+	 * - until master bus is aligned
+	 */
+	if (bd.remainder < mbus->buswidth)
+		early_bytes = bd.remainder;
+	else if ((mbus->addr) % (mbus->buswidth)) {
+		early_bytes = mbus->buswidth - (mbus->addr) % (mbus->buswidth);
+		if ((bd.remainder - early_bytes) < mbus->buswidth)
+			early_bytes = bd.remainder;
+	}
+
+	if (early_bytes) {
+		dev_vdbg(&pl08x->adev->dev, "%s byte width LLIs "
+				"(remain 0x%08x)\n", __func__, bd.remainder);
+		prep_byte_width_lli(&bd, &cctl, early_bytes, num_llis++,
+				&total_bytes);
+	}
 
+	if (bd.remainder) {
 		/*
 		 * Master now aligned
 		 * - if slave is not then we must set its width down
@@ -689,13 +695,12 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		/*
 		 * Send any odd bytes
 		 */
-		while (bd.remainder) {
-			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
+		if (bd.remainder) {
 			dev_vdbg(&pl08x->adev->dev,
-				"%s align with boundary, single odd byte (remain %zu)\n",
+				"%s align with boundary, send odd bytes (remain %zu)\n",
 				__func__, bd.remainder);
-			pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
-			total_bytes++;
+			prep_byte_width_lli(&bd, &cctl, bd.remainder,
+					num_llis++, &total_bytes);
 		}
 	}
 
-- 
1.7.2.2

[PATCH V2 15/20] dmaengine/amba-pl08x: Align lli_len to max(src.width, dst.width)

[Viresh Kumar]

Currently lli_len is aligned to min of two widths, which looks to be incorrect.
Instead it should be aligned to max of both widths.

Lets say, total_size = 441 bytes

MIN: lets check if min() suits or not:

CASE 1: srcwidth = 1, dstwidth = 4
min(src, dst) = 1

i.e. We program transfer size in control reg to 441.
Now, till 440 bytes everything is fine, but on the last byte DMAC can't transfer
1 byte to dst, as its width is 4.

CASE 2: srcwidth = 4, dstwidth = 1
min(src, dst) = 1

i.e. we program transfer size in control reg to 110 (data transferred = 110 * srcwidth).
So, here too 1 byte is left, but on the source side.

MAX: Lets check if max() suits or not:

CASE 3: srcwidth = 1, dstwidth = 4
max(src, dst) = 4

Aligned size is 440

i.e. We program transfer size in control reg to 440.
Now, all 440 bytes will be transferred without any issues.

CASE 4: srcwidth = 4, dstwidth = 1
max(src, dst) = 4

Aligned size is 440

i.e. We program transfer size in control reg to 110 (data transferred = 110 * srcwidth).
Now, also all 440 bytes will be transferred without any issues.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/amba-pl08x.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 0bdf3b8..ef02fb9 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -666,20 +666,22 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		 * width left
 		 */
 		while (bd.remainder > (mbus->buswidth - 1)) {
-			size_t lli_len, tsize;
+			size_t lli_len, tsize, width;
 
 			/*
 			 * If enough left try to send max possible,
 			 * otherwise try to send the remainder
 			 */
 			lli_len = min(bd.remainder, max_bytes_per_lli);
+
 			/*
-			 * Check against minimum bus alignment: Calculate actual
+			 * Check against maximum bus alignment: Calculate actual
 			 * transfer size in relation to bus width and get a
-			 * maximum remainder of the smallest bus width - 1
+			 * maximum remainder of the highest bus width - 1
 			 */
-			tsize = lli_len / min(mbus->buswidth, sbus->buswidth);
-			lli_len	= tsize * min(mbus->buswidth, sbus->buswidth);
+			width = max(mbus->buswidth, sbus->buswidth);
+			lli_len = (lli_len / width) * width;
+			tsize = lli_len / bd.srcbus.buswidth;
 
 			dev_vdbg(&pl08x->adev->dev,
 				"%s fill lli with single lli chunk of "
-- 
1.7.2.2

[PATCH V2 16/20] dmaengine/amba-pl08x: Choose peripheral bus as master bus

[Viresh Kumar]

When we have DMA transfers between peripheral and memory, then we shouldn't
reduce width of peripheral at all, as that may be a strict requirement. But we
can always reduce width of memory access, with some compromise in performance.
Thus, we must select peripheral as master and not memory.

Also this rearranges code to make it shorter.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/amba-pl08x.c |   22 ++++++----------------
 1 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index ef02fb9..c969309 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -496,34 +496,24 @@ struct pl08x_lli_build_data {
  * byte data), slave is still not aligned, then its width will be reduced to
  * BYTE.
  * - prefers the destination bus if both available
- * - if fixed address on one bus the other will be chosen
+ * - prefers bus with fixed address (i.e. peripheral)
  */
 static void pl08x_choose_master_bus(struct pl08x_lli_build_data *bd,
 	struct pl08x_bus_data **mbus, struct pl08x_bus_data **sbus, u32 cctl)
 {
 	if (!(cctl & PL080_CONTROL_DST_INCR)) {
-		*mbus = &bd->srcbus;
-		*sbus = &bd->dstbus;
-	} else if (!(cctl & PL080_CONTROL_SRC_INCR)) {
 		*mbus = &bd->dstbus;
 		*sbus = &bd->srcbus;
+	} else if (!(cctl & PL080_CONTROL_SRC_INCR)) {
+		*mbus = &bd->srcbus;
+		*sbus = &bd->dstbus;
 	} else {
-		if (bd->dstbus.buswidth == 4) {
-			*mbus = &bd->dstbus;
-			*sbus = &bd->srcbus;
-		} else if (bd->srcbus.buswidth == 4) {
-			*mbus = &bd->srcbus;
-			*sbus = &bd->dstbus;
-		} else if (bd->dstbus.buswidth == 2) {
+		if (bd->dstbus.buswidth >= bd->srcbus.buswidth) {
 			*mbus = &bd->dstbus;
 			*sbus = &bd->srcbus;
-		} else if (bd->srcbus.buswidth == 2) {
+		} else {
 			*mbus = &bd->srcbus;
 			*sbus = &bd->dstbus;
-		} else {
-			/* bd->srcbus.buswidth == 1 */
-			*mbus = &bd->dstbus;
-			*sbus = &bd->srcbus;
 		}
 	}
 }
-- 
1.7.2.2

[PATCH V2 17/20] dmaengine/amba-pl08x: Pass flow controller information with slave channel data

[Viresh Kumar]

At least, on SPEAr platforms there is one peripheral, JPEG, which can be flow
controller for DMA transfer. Currently DMA controller driver didn't support
peripheral flow controller configurations.

This patch adds device_fc field in struct pl08x_channel_data, which will be used
only for slave transfers and is not used in case of mem2mem transfers.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c   |   61 ++++++++++++++++++++++++++++++++++++++------
 include/linux/amba/pl08x.h |    4 +++
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index c969309..d573dc2 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -66,11 +66,6 @@
  *    after the final transfer signalled by LBREQ or LSREQ.  The DMAC
  *    will then move to the next LLI entry.
  *
- * Only the former works sanely with scatter lists, so we only implement
- * the DMAC flow control method.  However, peripherals which use the LBREQ
- * and LSREQ signals (eg, MMCI) are unable to use this mode, which through
- * these hardware restrictions prevents them from using scatter DMA.
- *
  * Global TODO:
  * - Break out common code from arch/arm/mach-s3c64xx and share
  */
@@ -615,6 +610,49 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		 sbus == &bd.srcbus ? "src" : "dst");
 
 	/*
+	 * Zero length is only allowed if all these requirements are met:
+	 * - flow controller is peripheral.
+	 * - src.addr is aligned to src.width
+	 * - dst.addr is aligned to dst.width
+	 *
+	 * sg_len == 1 should be true, as there can be two cases here:
+	 * - Memory addresses are contiguous and are not scattered. Here, Only
+	 * one sg will be passed by user driver, with memory address and zero
+	 * length. We pass this to controller and after the transfer it will
+	 * receive the last burst request from peripheral and so transfer
+	 * finishes.
+	 *
+	 * - Memory addresses are scattered and are not contiguous. Here,
+	 * Obviously as DMA controller doesn't know when a lli's transfer gets
+	 * over, it can't load next lli. So in this case, there has to be an
+	 * assumption that only one lli is supported. Thus, we can't have
+	 * scattered addresses.
+	 */
+	if (!bd.remainder) {
+		u32 fc = (txd->ccfg & PL080_CONFIG_FLOW_CONTROL_MASK) >>
+			PL080_CONFIG_FLOW_CONTROL_SHIFT;
+		if (!((fc >= PL080_FLOW_SRC2DST_DST) &&
+					(fc <= PL080_FLOW_SRC2DST_SRC))) {
+			dev_err(&pl08x->adev->dev, "%s sg len can't be zero",
+				__func__);
+			return 0;
+		}
+
+		if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
+				(bd.srcbus.addr % bd.srcbus.buswidth)) {
+			dev_err(&pl08x->adev->dev,
+				"%s src & dst address must be aligned to src"
+				" & dst width if peripheral is flow controller",
+				__func__);
+			return 0;
+		}
+
+		cctl = pl08x_cctl_bits(cctl, bd.srcbus.buswidth,
+				bd.dstbus.buswidth, 0);
+		pl08x_fill_lli_for_desc(&bd, num_llis++, 0, cctl);
+	}
+
+	/*
 	 * Send byte by byte for following cases
 	 * - Less than a bus width available
 	 * - until master bus is aligned
@@ -1255,7 +1293,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_txd *txd;
-	int ret;
+	int ret, tmp;
 
 	/*
 	 * Current implementation ASSUMES only one sg
@@ -1289,12 +1327,10 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	txd->len = sgl->length;
 
 	if (direction == DMA_TO_DEVICE) {
-		txd->ccfg |= PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->cctl = plchan->dst_cctl;
 		txd->src_addr = sgl->dma_address;
 		txd->dst_addr = plchan->dst_addr;
 	} else if (direction == DMA_FROM_DEVICE) {
-		txd->ccfg |= PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->cctl = plchan->src_cctl;
 		txd->src_addr = plchan->src_addr;
 		txd->dst_addr = sgl->dma_address;
@@ -1304,6 +1340,15 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
+	if (plchan->cd->device_fc)
+		tmp = (direction == DMA_TO_DEVICE) ? PL080_FLOW_MEM2PER_PER :
+			PL080_FLOW_PER2MEM_PER;
+	else
+		tmp = (direction == DMA_TO_DEVICE) ? PL080_FLOW_MEM2PER :
+			PL080_FLOW_PER2MEM;
+
+	txd->ccfg |= tmp << PL080_CONFIG_FLOW_CONTROL_SHIFT;
+
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
 		return NULL;
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index ecd17f5..a22662c 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -47,6 +47,9 @@ enum {
  * @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
+ * @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@Runtime.
  * @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,6 +68,7 @@ struct pl08x_channel_data {
 	int max_signal;
 	u32 muxval;
 	u32 cctl;
+	bool device_fc;
 	dma_addr_t addr;
 	bool circular_buffer;
 	bool single;
-- 
1.7.2.2

[PATCH V2 18/20] dmaengine/amba-pl08x: Add support for sg len greater than one for slave transfers

[Viresh Kumar]

Untill now, sg_len greater than one is not supported. This patch adds support to
do that.

Note: Still, if peripheral is flow controller, sg_len can't be greater that one.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/amba-pl08x.c   |  375 +++++++++++++++++++++++++-------------------
 include/linux/amba/pl08x.h |   22 ++-
 2 files changed, 229 insertions(+), 168 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index d573dc2..adc8a36 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -349,7 +349,9 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
 	if (!list_empty(&plchan->pend_list)) {
 		struct pl08x_txd *txdi;
 		list_for_each_entry(txdi, &plchan->pend_list, node) {
-			bytes += txdi->len;
+			struct pl08x_sg *dsg;
+			list_for_each_entry(dsg, &txd->dsg_list, node)
+				bytes += dsg->len;
 		}
 	}
 
@@ -563,6 +565,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	u32 cctl, early_bytes = 0;
 	size_t max_bytes_per_lli, total_bytes = 0;
 	struct pl08x_lli *llis_va;
+	struct pl08x_sg *dsg;
 
 	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT, &txd->llis_bus);
 	if (!txd->llis_va) {
@@ -572,13 +575,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 	pl08x->pool_ctr++;
 
-	/* Get the default CCTL */
-	cctl = txd->cctl;
-
 	bd.txd = txd;
-	bd.srcbus.addr = txd->src_addr;
-	bd.dstbus.addr = txd->dst_addr;
 	bd.lli_bus = (pl08x->lli_buses & PL08X_AHB2) ? PL080_LLI_LM_AHB2 : 0;
+	cctl = txd->cctl;
 
 	/* Find maximum width of the source bus */
 	bd.srcbus.maxwidth =
@@ -590,162 +589,178 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		pl08x_get_bytes_for_cctl((cctl & PL080_CONTROL_DWIDTH_MASK) >>
 				       PL080_CONTROL_DWIDTH_SHIFT);
 
-	/* Set up the bus widths to the maximum */
-	bd.srcbus.buswidth = bd.srcbus.maxwidth;
-	bd.dstbus.buswidth = bd.dstbus.maxwidth;
+	list_for_each_entry(dsg, &txd->dsg_list, node) {
+		cctl = txd->cctl;
 
-	/* We need to count this down to zero */
-	bd.remainder = txd->len;
+		bd.srcbus.addr = dsg->src_addr;
+		bd.dstbus.addr = dsg->dst_addr;
+		bd.remainder = dsg->len;
+		bd.srcbus.buswidth = bd.srcbus.maxwidth;
+		bd.dstbus.buswidth = bd.dstbus.maxwidth;
 
-	pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
+		pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
 
-	dev_vdbg(&pl08x->adev->dev, "src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu\n",
-		 bd.srcbus.addr, cctl & PL080_CONTROL_SRC_INCR ? "+" : "",
-		 bd.srcbus.buswidth,
-		 bd.dstbus.addr, cctl & PL080_CONTROL_DST_INCR ? "+" : "",
-		 bd.dstbus.buswidth,
-		 bd.remainder);
-	dev_vdbg(&pl08x->adev->dev, "mbus=%s sbus=%s\n",
-		 mbus == &bd.srcbus ? "src" : "dst",
-		 sbus == &bd.srcbus ? "src" : "dst");
+		dev_vdbg(&pl08x->adev->dev, "src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu\n",
+			bd.srcbus.addr, cctl & PL080_CONTROL_SRC_INCR ? "+" : "",
+			bd.srcbus.buswidth,
+			bd.dstbus.addr, cctl & PL080_CONTROL_DST_INCR ? "+" : "",
+			bd.dstbus.buswidth,
+			bd.remainder);
+		dev_vdbg(&pl08x->adev->dev, "mbus=%s sbus=%s\n",
+			mbus == &bd.srcbus ? "src" : "dst",
+			sbus == &bd.srcbus ? "src" : "dst");
 
-	/*
-	 * Zero length is only allowed if all these requirements are met:
-	 * - flow controller is peripheral.
-	 * - src.addr is aligned to src.width
-	 * - dst.addr is aligned to dst.width
-	 *
-	 * sg_len == 1 should be true, as there can be two cases here:
-	 * - Memory addresses are contiguous and are not scattered. Here, Only
-	 * one sg will be passed by user driver, with memory address and zero
-	 * length. We pass this to controller and after the transfer it will
-	 * receive the last burst request from peripheral and so transfer
-	 * finishes.
-	 *
-	 * - Memory addresses are scattered and are not contiguous. Here,
-	 * Obviously as DMA controller doesn't know when a lli's transfer gets
-	 * over, it can't load next lli. So in this case, there has to be an
-	 * assumption that only one lli is supported. Thus, we can't have
-	 * scattered addresses.
-	 */
-	if (!bd.remainder) {
-		u32 fc = (txd->ccfg & PL080_CONFIG_FLOW_CONTROL_MASK) >>
-			PL080_CONFIG_FLOW_CONTROL_SHIFT;
-		if (!((fc >= PL080_FLOW_SRC2DST_DST) &&
+		/*
+		 * Zero length is only allowed if all these requirements are
+		 * met:
+		 * - flow controller is peripheral.
+		 * - src.addr is aligned to src.width
+		 * - dst.addr is aligned to dst.width
+		 *
+		 * sg_len == 1 should be true, as there can be two cases here:
+		 *
+		 * - Memory addresses are contiguous and are not scattered.
+		 *   Here, Only one sg will be passed by user driver, with
+		 *   memory address and zero length. We pass this to controller
+		 *   and after the transfer it will receive the last burst
+		 *   request from peripheral and so transfer finishes.
+		 *
+		 * - Memory addresses are scattered and are not contiguous.
+		 *   Here, Obviously as DMA controller doesn't know when a lli's
+		 *   transfer gets over, it can't load next lli. So in this
+		 *   case, there has to be an assumption that only one lli is
+		 *   supported. Thus, we can't have scattered addresses.
+		 */
+		if (!bd.remainder) {
+			u32 fc = (txd->ccfg & PL080_CONFIG_FLOW_CONTROL_MASK) >>
+				PL080_CONFIG_FLOW_CONTROL_SHIFT;
+			if (!((fc >= PL080_FLOW_SRC2DST_DST) &&
 					(fc <= PL080_FLOW_SRC2DST_SRC))) {
-			dev_err(&pl08x->adev->dev, "%s sg len can't be zero",
-				__func__);
-			return 0;
-		}
-
-		if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
-				(bd.srcbus.addr % bd.srcbus.buswidth)) {
-			dev_err(&pl08x->adev->dev,
-				"%s src & dst address must be aligned to src"
-				" & dst width if peripheral is flow controller",
-				__func__);
-			return 0;
-		}
-
-		cctl = pl08x_cctl_bits(cctl, bd.srcbus.buswidth,
-				bd.dstbus.buswidth, 0);
-		pl08x_fill_lli_for_desc(&bd, num_llis++, 0, cctl);
-	}
+				dev_err(&pl08x->adev->dev, "%s sg len can't be zero",
+					__func__);
+				return 0;
+			}
 
-	/*
-	 * Send byte by byte for following cases
-	 * - Less than a bus width available
-	 * - until master bus is aligned
-	 */
-	if (bd.remainder < mbus->buswidth)
-		early_bytes = bd.remainder;
-	else if ((mbus->addr) % (mbus->buswidth)) {
-		early_bytes = mbus->buswidth - (mbus->addr) % (mbus->buswidth);
-		if ((bd.remainder - early_bytes) < mbus->buswidth)
-			early_bytes = bd.remainder;
-	}
+			if ((bd.srcbus.addr % bd.srcbus.buswidth) ||
+					(bd.srcbus.addr % bd.srcbus.buswidth)) {
+				dev_err(&pl08x->adev->dev,
+					"%s src & dst address must be aligned to src"
+					" & dst width if peripheral is flow controller",
+					__func__);
+				return 0;
+			}
 
-	if (early_bytes) {
-		dev_vdbg(&pl08x->adev->dev, "%s byte width LLIs "
-				"(remain 0x%08x)\n", __func__, bd.remainder);
-		prep_byte_width_lli(&bd, &cctl, early_bytes, num_llis++,
-				&total_bytes);
-	}
+			cctl = pl08x_cctl_bits(cctl, bd.srcbus.buswidth,
+					bd.dstbus.buswidth, 0);
+			pl08x_fill_lli_for_desc(&bd, num_llis++, 0, cctl);
+			break;
+		}
 
-	if (bd.remainder) {
 		/*
-		 * Master now aligned
-		 * - if slave is not then we must set its width down
+		 * Send byte by byte for following cases
+		 * - Less than a bus width available
+		 * - until master bus is aligned
 		 */
-		if (sbus->addr % sbus->buswidth) {
-			dev_dbg(&pl08x->adev->dev,
-				"%s set down bus width to one byte\n",
-				 __func__);
+		if (bd.remainder < mbus->buswidth)
+			early_bytes = bd.remainder;
+		else if ((mbus->addr) % (mbus->buswidth)) {
+			early_bytes = mbus->buswidth - (mbus->addr) %
+				(mbus->buswidth);
+			if ((bd.remainder - early_bytes) < mbus->buswidth)
+				early_bytes = bd.remainder;
+		}
 
-			sbus->buswidth = 1;
+		if (early_bytes) {
+			dev_vdbg(&pl08x->adev->dev,
+				"%s byte width LLIs (remain 0x%08x)\n",
+				__func__, bd.remainder);
+			prep_byte_width_lli(&bd, &cctl, early_bytes, num_llis++,
+				&total_bytes);
 		}
 
-		/* Bytes transferred = tsize * src width, not MIN(buswidths) */
-		max_bytes_per_lli = bd.srcbus.buswidth *
-			PL080_CONTROL_TRANSFER_SIZE_MASK;
+		if (bd.remainder) {
+			/*
+			 * Master now aligned
+			 * - if slave is not then we must set its width down
+			 */
+			if (sbus->addr % sbus->buswidth) {
+				dev_dbg(&pl08x->adev->dev,
+					"%s set down bus width to one byte\n",
+					__func__);
 
-		/*
-		 * Make largest possible LLIs until less than one bus
-		 * width left
-		 */
-		while (bd.remainder > (mbus->buswidth - 1)) {
-			size_t lli_len, tsize, width;
+				sbus->buswidth = 1;
+			}
 
 			/*
-			 * If enough left try to send max possible,
-			 * otherwise try to send the remainder
+			 * Bytes transferred = tsize * src width, not
+			 * MIN(buswidths)
 			 */
-			lli_len = min(bd.remainder, max_bytes_per_lli);
+			max_bytes_per_lli = bd.srcbus.buswidth *
+				PL080_CONTROL_TRANSFER_SIZE_MASK;
+			dev_vdbg(&pl08x->adev->dev,
+				"%s max bytes per lli = %zu\n",
+				__func__, max_bytes_per_lli);
 
 			/*
-			 * Check against maximum bus alignment: Calculate actual
-			 * transfer size in relation to bus width and get a
-			 * maximum remainder of the highest bus width - 1
+			 * Make largest possible LLIs until less than one bus
+			 * width left
 			 */
-			width = max(mbus->buswidth, sbus->buswidth);
-			lli_len = (lli_len / width) * width;
-			tsize = lli_len / bd.srcbus.buswidth;
+			while (bd.remainder > (mbus->buswidth - 1)) {
+				size_t lli_len, tsize, width;
 
-			dev_vdbg(&pl08x->adev->dev,
-				"%s fill lli with single lli chunk of "
-				"size 0x%08zx (remainder 0x%08zx)\n",
-				__func__, lli_len, bd.remainder);
+				/*
+				 * If enough left try to send max possible,
+				 * otherwise try to send the remainder
+				 */
+				lli_len = min(bd.remainder, max_bytes_per_lli);
 
-			cctl = pl08x_cctl_bits(cctl, bd.srcbus.buswidth,
+				/*
+				 * Check against maximum bus alignment:
+				 * Calculate actual transfer size in relation to
+				 * bus width an get a maximum remainder of the
+				 * highest bus width - 1
+				 */
+				width = max(mbus->buswidth, sbus->buswidth);
+				lli_len = (lli_len / width) * width;
+				tsize = lli_len / bd.srcbus.buswidth;
+
+				dev_vdbg(&pl08x->adev->dev,
+					"%s fill lli with single lli chunk of "
+					"size 0x%08zx (remainder 0x%08zx)\n",
+					__func__, lli_len, bd.remainder);
+
+				cctl = pl08x_cctl_bits(cctl, bd.srcbus.buswidth,
 					bd.dstbus.buswidth, tsize);
-			pl08x_fill_lli_for_desc(&bd, num_llis++, lli_len, cctl);
-			total_bytes += lli_len;
-		}
+				pl08x_fill_lli_for_desc(&bd, num_llis++,
+						lli_len, cctl);
+				total_bytes += lli_len;
+			}
 
-		/*
-		 * Send any odd bytes
-		 */
-		if (bd.remainder) {
-			dev_vdbg(&pl08x->adev->dev,
-				"%s align with boundary, send odd bytes (remain %zu)\n",
-				__func__, bd.remainder);
-			prep_byte_width_lli(&bd, &cctl, bd.remainder,
-					num_llis++, &total_bytes);
+			/*
+			 * Send any odd bytes
+			 */
+			if (bd.remainder) {
+				dev_vdbg(&pl08x->adev->dev,
+					"%s align with boundary, send odd bytes (remain %zu)\n",
+					__func__, bd.remainder);
+				prep_byte_width_lli(&bd, &cctl, bd.remainder,
+						num_llis++, &total_bytes);
+			}
 		}
-	}
 
-	if (total_bytes != txd->len) {
-		dev_err(&pl08x->adev->dev,
-			"%s size of encoded lli:s don't match total txd, transferred 0x%08zx from size 0x%08zx\n",
-			__func__, total_bytes, txd->len);
-		return 0;
-	}
+		if (total_bytes != dsg->len) {
+			dev_err(&pl08x->adev->dev,
+				"%s size of encoded lli:s don't match total txd, transferred 0x%08zx from size 0x%08zx\n",
+				__func__, total_bytes, dsg->len);
+			return 0;
+		}
 
-	if (num_llis >= MAX_NUM_TSFR_LLIS) {
-		dev_err(&pl08x->adev->dev,
-			"%s need to increase MAX_NUM_TSFR_LLIS from 0x%08x\n",
-			__func__, (u32) MAX_NUM_TSFR_LLIS);
-		return 0;
+		if (num_llis >= MAX_NUM_TSFR_LLIS) {
+			dev_err(&pl08x->adev->dev,
+				"%s need to increase MAX_NUM_TSFR_LLIS from 0x%08x\n",
+				__func__, (u32) MAX_NUM_TSFR_LLIS);
+			return 0;
+		}
 	}
 
 	llis_va = txd->llis_va;
@@ -778,11 +793,18 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 static void pl08x_free_txd(struct pl08x_driver_data *pl08x,
 			   struct pl08x_txd *txd)
 {
+	struct pl08x_sg *dsg, *_dsg;
+
 	/* Free the LLI */
 	dma_pool_free(pl08x->pool, txd->llis_va, txd->llis_bus);
 
 	pl08x->pool_ctr--;
 
+	list_for_each_entry_safe(dsg, _dsg, &txd->dsg_list, node) {
+		list_del(&dsg->node);
+		kfree(dsg);
+	}
+
 	kfree(txd);
 }
 
@@ -1234,6 +1256,7 @@ static struct pl08x_txd *pl08x_get_txd(struct pl08x_dma_chan *plchan,
 		txd->tx.flags = flags;
 		txd->tx.tx_submit = pl08x_tx_submit;
 		INIT_LIST_HEAD(&txd->node);
+		INIT_LIST_HEAD(&txd->dsg_list);
 
 		/* Always enable error and terminal interrupts */
 		txd->ccfg = PL080_CONFIG_ERR_IRQ_MASK |
@@ -1252,6 +1275,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_txd *txd;
+	struct pl08x_sg *dsg;
 	int ret;
 
 	txd = pl08x_get_txd(plchan, flags);
@@ -1261,10 +1285,19 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 		return NULL;
 	}
 
+	dsg = kzalloc(sizeof(struct pl08x_sg), GFP_NOWAIT);
+	if (!dsg) {
+		pl08x_free_txd(pl08x, txd);
+		dev_err(&pl08x->adev->dev, "%s no memory for pl080 sg\n",
+				__func__);
+		return NULL;
+	}
+	list_add_tail(&dsg->node, &txd->dsg_list);
+
 	txd->direction = DMA_NONE;
-	txd->src_addr = src;
-	txd->dst_addr = dest;
-	txd->len = len;
+	dsg->src_addr = src;
+	dsg->dst_addr = dest;
+	dsg->len = len;
 
 	/* Set platform data for m2m */
 	txd->ccfg |= PL080_FLOW_MEM2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
@@ -1293,19 +1326,13 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_txd *txd;
+	struct pl08x_sg *dsg;
+	struct scatterlist *sg;
+	dma_addr_t slave_addr;
 	int ret, tmp;
 
-	/*
-	 * Current implementation ASSUMES only one sg
-	 */
-	if (sg_len != 1) {
-		dev_err(&pl08x->adev->dev, "%s prepared too long sglist\n",
-			__func__);
-		BUG();
-	}
-
 	dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from %s\n",
-		__func__, sgl->length, plchan->name);
+			__func__, sgl->length, plchan->name);
 
 	txd = pl08x_get_txd(plchan, flags);
 	if (!txd) {
@@ -1324,17 +1351,15 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	 * channel target address dynamically at runtime.
 	 */
 	txd->direction = direction;
-	txd->len = sgl->length;
 
 	if (direction == DMA_TO_DEVICE) {
 		txd->cctl = plchan->dst_cctl;
-		txd->src_addr = sgl->dma_address;
-		txd->dst_addr = plchan->dst_addr;
+		slave_addr = plchan->dst_addr;
 	} else if (direction == DMA_FROM_DEVICE) {
 		txd->cctl = plchan->src_cctl;
-		txd->src_addr = plchan->src_addr;
-		txd->dst_addr = sgl->dma_address;
+		slave_addr = plchan->src_addr;
 	} else {
+		pl08x_free_txd(pl08x, txd);
 		dev_err(&pl08x->adev->dev,
 			"%s direction unsupported\n", __func__);
 		return NULL;
@@ -1349,6 +1374,26 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 
 	txd->ccfg |= tmp << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 
+	for_each_sg(sgl, sg, sg_len, tmp) {
+		dsg = kzalloc(sizeof(struct pl08x_sg), GFP_NOWAIT);
+		if (!dsg) {
+			pl08x_free_txd(pl08x, txd);
+			dev_err(&pl08x->adev->dev, "%s no mem for pl080 sg\n",
+					__func__);
+			return NULL;
+		}
+		list_add_tail(&dsg->node, &txd->dsg_list);
+
+		dsg->len = sg_dma_len(sg);
+		if (direction == DMA_TO_DEVICE) {
+			dsg->src_addr = sg_phys(sg);
+			dsg->dst_addr = slave_addr;
+		} else {
+			dsg->src_addr = slave_addr;
+			dsg->dst_addr = sg_phys(sg);
+		}
+	}
+
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
 		return NULL;
@@ -1446,22 +1491,28 @@ static void pl08x_ensure_on(struct pl08x_driver_data *pl08x)
 static void pl08x_unmap_buffers(struct pl08x_txd *txd)
 {
 	struct device *dev = txd->tx.chan->device->dev;
+	struct pl08x_sg *dsg;
 
 	if (!(txd->tx.flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
 		if (txd->tx.flags & DMA_COMPL_SRC_UNMAP_SINGLE)
-			dma_unmap_single(dev, txd->src_addr, txd->len,
-				DMA_TO_DEVICE);
-		else
-			dma_unmap_page(dev, txd->src_addr, txd->len,
-				DMA_TO_DEVICE);
+			list_for_each_entry(dsg, &txd->dsg_list, node)
+				dma_unmap_single(dev, dsg->src_addr, dsg->len,
+						DMA_TO_DEVICE);
+		else {
+			list_for_each_entry(dsg, &txd->dsg_list, node)
+				dma_unmap_page(dev, dsg->src_addr, dsg->len,
+						DMA_TO_DEVICE);
+		}
 	}
 	if (!(txd->tx.flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
 		if (txd->tx.flags & DMA_COMPL_DEST_UNMAP_SINGLE)
-			dma_unmap_single(dev, txd->dst_addr, txd->len,
-				DMA_FROM_DEVICE);
+			list_for_each_entry(dsg, &txd->dsg_list, node)
+				dma_unmap_single(dev, dsg->dst_addr, dsg->len,
+						DMA_FROM_DEVICE);
 		else
-			dma_unmap_page(dev, txd->dst_addr, txd->len,
-				DMA_FROM_DEVICE);
+			list_for_each_entry(dsg, &txd->dsg_list, node)
+				dma_unmap_page(dev, dsg->dst_addr, dsg->len,
+						DMA_FROM_DEVICE);
 	}
 }
 
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index a22662c..9eabffb 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -106,12 +106,24 @@ struct pl08x_phy_chan {
 };
 
 /**
+ * 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
- * @src_addr: src address of txd
- * @dst_addr: dst address of txd
- * @len: transfer len in bytes
+ * @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
@@ -121,10 +133,8 @@ struct pl08x_phy_chan {
 struct pl08x_txd {
 	struct dma_async_tx_descriptor tx;
 	struct list_head node;
+	struct list_head dsg_list;
 	enum dma_data_direction	direction;
-	dma_addr_t src_addr;
-	dma_addr_t dst_addr;
-	size_t len;
 	dma_addr_t llis_bus;
 	struct pl08x_lli *llis_va;
 	/* Default cctl value for LLIs */
-- 
1.7.2.2

[PATCH V2 19/20] dmaengine/amba-pl08x: Check txd->llis_va before freeing dma_pool

[Viresh Kumar]

In pl08x_free_txd(), check if pool is allocated successfully before freeing it.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/amba-pl08x.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index adc8a36..2667e0d 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -796,7 +796,8 @@ static void pl08x_free_txd(struct pl08x_driver_data *pl08x,
 	struct pl08x_sg *dsg, *_dsg;
 
 	/* Free the LLI */
-	dma_pool_free(pl08x->pool, txd->llis_va, txd->llis_bus);
+	if (txd->llis_va)
+		dma_pool_free(pl08x->pool, txd->llis_va, txd->llis_bus);
 
 	pl08x->pool_ctr--;
 
-- 
1.7.2.2

[PATCH V2 20/20] dmaengine/amba-pl08x: Call pl08x_free_txd() instead of calling kfree() directly

[Viresh Kumar]

pl08x_prep_channel_resources() is calling kfree() directly for txd(). To
maintain consistency in code call pl08x_free_txd() instead.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/dma/amba-pl08x.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 2667e0d..063970f 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1202,7 +1202,9 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 
 	num_llis = pl08x_fill_llis_for_desc(pl08x, txd);
 	if (!num_llis) {
-		kfree(txd);
+		spin_lock_irqsave(&plchan->lock, flags);
+		pl08x_free_txd(pl08x, txd);
+		spin_unlock_irqrestore(&plchan->lock, flags);
 		return -EINVAL;
 	}
 
-- 
1.7.2.2

[PATCH V2 03/20] dmaengine/amba-pl08x: Rearrange inclusion of header files in ascending order

[Sergei Shtylyov]

Hello.

On 01-08-2011 13:37, Viresh Kumar wrote:

> Header files included in driver are not present in alphabetical order. Rearrange
> them in alphabetical order.

> Signed-off-by: Viresh Kumar<viresh.kumar@st.com>
> ---
>   drivers/dma/amba-pl08x.c |   17 ++++++++---------
>   1 files changed, 8 insertions(+), 9 deletions(-)

> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 4c4a309..8e2056b 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -74,19 +74,18 @@
>    * Global TODO:
>    * - Break out common code from arch/arm/mach-s3c64xx and share
>    */
> -#include<linux/device.h>
> -#include<linux/init.h>
> -#include<linux/module.h>
> -#include<linux/interrupt.h>
> -#include<linux/slab.h>
> -#include<linux/delay.h>
> -#include<linux/dmapool.h>
> -#include<linux/dmaengine.h>
>   #include<linux/amba/bus.h>
>   #include<linux/amba/pl08x.h>
>   #include<linux/debugfs.h>
> +#include<linux/delay.h>
> +#include<linux/device.h>
> +#include<linux/dmaengine.h>
> +#include<linux/dmapool.h>
> +#include<linux/init.h>
> +#include<linux/interrupt.h>
> +#include<linux/module.h>
>   #include<linux/seq_file.h>
> -
> +#include<linux/slab.h>

    Shouldn't there be emoty line here?

>   #include<asm/hardware/pl080.h>

WBR, Sergei

[PATCH V2 08/20] dmaengine/amba-pl08x: support runtime PM

[Russell King - ARM Linux]

On Mon, Aug 01, 2011 at 03:07:18PM +0530, Viresh Kumar wrote:
> @@ -1993,6 +2002,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  	dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n",
>  		 amba_part(adev), amba_rev(adev),
>  		 (unsigned long long)adev->res.start, adev->irq[0]);
> +
> +	pm_runtime_suspend(&adev->dev);

Having read the runtime pm documentation, devices are assumed to be
suspended at probe time, and there should be a call to pm_runtime_enable()
in here.  See Documentation/power/runtime_pm.txt chapter 5.

However, this is complicated by the core managing the peripheral clock,
which starts off in the enabled state.  So there's only one sane solution,
which is to tell the runtime PM that the device is already active.  So
I think a primecell's probe function should look like this:

primecell_probe()
{
	ret = amba_request_regions(adev, NULL);
	if (ret)
		return ret;

	... allocate stuff, don't access primecell though ...

	pm_runtime_set_active(&adev->dev);
	pm_runtime_enable(&adev->dev);

	... get clocks and enable them, do rest of init ...

	pm_runtime_put_sync(&adev->dev);
	return 0;
}

[PATCH V2 03/20] dmaengine/amba-pl08x: Rearrange inclusion of header files in ascending order

[viresh kumar]

On 08/01/2011 03:22 PM, Sergei Shtylyov wrote:
>> > +#include<linux/slab.h>
>     Shouldn't there be emoty line here?
> 
>> >   #include<asm/hardware/pl080.h>


Ya, can be, but i thought it is also fine. Don't know.
If you want i can do that.

-- 
viresh

[PATCH V2 08/20] dmaengine/amba-pl08x: support runtime PM

[viresh kumar]

On 08/01/2011 03:56 PM, Russell King - ARM Linux wrote:
> I think a primecell's probe function should look like this:
> 
> primecell_probe()
> {
> 	ret = amba_request_regions(adev, NULL);
> 	if (ret)
> 		return ret;
> 
> 	... allocate stuff, don't access primecell though ...
> 
> 	pm_runtime_set_active(&adev->dev);
> 	pm_runtime_enable(&adev->dev);
> 
> 	... get clocks and enable them, do rest of init ...
> 
> 	pm_runtime_put_sync(&adev->dev);
> 	return 0;
> }

Ok. I didn't had much knowhow of pm_runtime* framework. I will
send V3 for this patch alone, please see if that one is fine or still
needs some updation.

Thanks for your help.

-- 
viresh

[PATCH V3 08/20] dmaengine/amba-pl08x: support runtime PM

[Viresh Kumar]

Insert notifiers for the runtime PM API. With this the runtime PM layer kicks in
to action where used. This will also handle enabling/disabling of interface
clock (Code will be added in amba/bus.c by Russell King).

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 3fbaf0e..a829f0e 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -84,6 +84,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <asm/hardware/pl080.h>
@@ -879,11 +880,19 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x,
  */
 static int pl08x_alloc_chan_resources(struct dma_chan *chan)
 {
+	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
+	struct pl08x_driver_data *pl08x = plchan->host;
+
+	pm_runtime_get_sync(&pl08x->adev->dev);
 	return 0;
 }
 
 static void pl08x_free_chan_resources(struct dma_chan *chan)
 {
+	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
+	struct pl08x_driver_data *pl08x = plchan->host;
+
+	pm_runtime_put_sync(&pl08x->adev->dev);
 }
 
 /*
@@ -1855,6 +1864,9 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 		goto out_no_pl08x;
 	}
 
+	pm_runtime_set_active(&adev->dev);
+	pm_runtime_enable(&adev->dev);
+
 	/* Initialize memcpy engine */
 	dma_cap_set(DMA_MEMCPY, pl08x->memcpy.cap_mask);
 	pl08x->memcpy.dev = &adev->dev;
@@ -1993,6 +2005,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n",
 		 amba_part(adev), amba_rev(adev),
 		 (unsigned long long)adev->res.start, adev->irq[0]);
+
+	pm_runtime_put_sync(&adev->dev);
 	return 0;
 
 out_no_slave_reg:
@@ -2011,6 +2025,9 @@ out_no_ioremap:
 	dma_pool_destroy(pl08x->pool);
 out_no_lli_pool:
 out_no_platdata:
+	pm_runtime_put_sync(&adev->dev);
+	pm_runtime_disable(&adev->dev);
+
 	kfree(pl08x);
 out_no_pl08x:
 	amba_release_regions(adev);
-- 
1.7.2.2

[PATCH V2 08/20] dmaengine/amba-pl08x: support runtime PM

[Russell King - ARM Linux]

On Mon, Aug 01, 2011 at 03:07:18PM +0530, Viresh Kumar wrote:
> Insert notifiers for the runtime PM API. With this the runtime PM layer kicks in
> to action where used. This will also handle enabling/disabling of interface
> clock (Code will be added in amba/bus.c by Russell King).

I don't think this is correct...

> @@ -879,11 +880,19 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x,
>   */
>  static int pl08x_alloc_chan_resources(struct dma_chan *chan)
>  {
> +	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
> +	struct pl08x_driver_data *pl08x = plchan->host;
> +
> +	pm_runtime_resume(&pl08x->adev->dev);

Shouldn't this be pm_runtime_get_sync() ?

>  	return 0;
>  }
>  
>  static void pl08x_free_chan_resources(struct dma_chan *chan)
>  {
> +	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
> +	struct pl08x_driver_data *pl08x = plchan->host;
> +
> +	pm_runtime_suspend(&pl08x->adev->dev);

And pm_runtime_put() ?

>  }
>  
>  /*
> @@ -1993,6 +2002,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  	dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n",
>  		 amba_part(adev), amba_rev(adev),
>  		 (unsigned long long)adev->res.start, adev->irq[0]);
> +
> +	pm_runtime_suspend(&adev->dev);

And pm_runtime_put() ?

We don't want to call pm_runtime_suspend/resume directly because that
could have bad consequences if more than one channel is in use.

The enabling and disabling of runtime PM for AMBA devices will be handled
in the core code...

Lastly, we may want to make this even tighter to the actual period that
the DMA is being used, rather than just the period that a driver has
been allocated a channel.  I'd rather have it done that way now (and
tested) so that we achieve maximal effect from runtime PM, rather than
having something which needs to be revisited again.

IOW, if this is worth doing, then its worth doing properly first time.

[PATCH V2 08/20] dmaengine/amba-pl08x: support runtime PM

[viresh kumar]

On 08/03/2011 06:09 PM, Russell King - ARM Linux wrote:
> On Mon, Aug 01, 2011 at 03:07:18PM +0530, Viresh Kumar wrote:
>> Insert notifiers for the runtime PM API. With this the runtime PM layer kicks in
>> to action where used. This will also handle enabling/disabling of interface
>> clock (Code will be added in amba/bus.c by Russell King).
> 
> I don't think this is correct...

Sorry, but i couldn't get this comment completely. Mentioning clock
stuff here is incorrect or mentioning about your code? Or both?
Or something else?

>> +	pm_runtime_resume(&pl08x->adev->dev);
> 
> Shouldn't this be pm_runtime_get_sync() ?

>> +	pm_runtime_suspend(&pl08x->adev->dev);
> 
> And pm_runtime_put() ?

>> +	pm_runtime_suspend(&adev->dev);
> 
> And pm_runtime_put() ?

I have already fixed above in V3.

> Lastly, we may want to make this even tighter to the actual period that
> the DMA is being used, rather than just the period that a driver has
> been allocated a channel.  I'd rather have it done that way now (and
> tested) so that we achieve maximal effect from runtime PM, rather than
> having something which needs to be revisited again.
> 
> IOW, if this is worth doing, then its worth doing properly first time.

Correct. What about this one:

---
 drivers/dma/amba-pl08x.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 3fbaf0e..24353df 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -84,6 +84,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <asm/hardware/pl080.h>
@@ -405,6 +406,7 @@ pl08x_get_phy_channel(struct pl08x_driver_data *pl08x,
 		return NULL;
 	}
 
+	pm_runtime_get_sync(&pl08x->adev->dev);
 	return ch;
 }
 
@@ -418,6 +420,8 @@ static inline void pl08x_put_phy_channel(struct pl08x_driver_data *pl08x,
 	/* Stop the channel and clear its interrupts */
 	pl08x_terminate_phy_chan(pl08x, ch);
 
+	pm_runtime_put(&pl08x->adev->dev);
+
 	/* Mark it as free */
 	ch->serving = NULL;
 	spin_unlock_irqrestore(&ch->lock, flags);
@@ -1855,6 +1859,9 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 		goto out_no_pl08x;
 	}
 
+	pm_runtime_set_active(&adev->dev);
+	pm_runtime_enable(&adev->dev);
+
 	/* Initialize memcpy engine */
 	dma_cap_set(DMA_MEMCPY, pl08x->memcpy.cap_mask);
 	pl08x->memcpy.dev = &adev->dev;
@@ -1993,6 +2000,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n",
 		 amba_part(adev), amba_rev(adev),
 		 (unsigned long long)adev->res.start, adev->irq[0]);
+
+	pm_runtime_put(&adev->dev);
 	return 0;
 
 out_no_slave_reg:
@@ -2011,6 +2020,9 @@ out_no_ioremap:
 	dma_pool_destroy(pl08x->pool);
 out_no_lli_pool:
 out_no_platdata:
+	pm_runtime_put(&adev->dev);
+	pm_runtime_disable(&adev->dev);
+
 	kfree(pl08x);
 out_no_pl08x:
 	amba_release_regions(adev);


-- 
viresh

[PATCH V2 08/20] dmaengine/amba-pl08x: support runtime PM

[Koul, Vinod]

On Thu, 2011-08-04 at 10:49 +0530, viresh kumar wrote:
> On 08/03/2011 06:09 PM, Russell King - ARM Linux wrote:
> > On Mon, Aug 01, 2011 at 03:07:18PM +0530, Viresh Kumar wrote:
> >> Insert notifiers for the runtime PM API. With this the runtime PM layer kicks in
> >> to action where used. This will also handle enabling/disabling of interface
> >> clock (Code will be added in amba/bus.c by Russell King).
> > 
> > I don't think this is correct...
> 
> Sorry, but i couldn't get this comment completely. Mentioning clock
> stuff here is incorrect or mentioning about your code? Or both?
> Or something else?
> 
> >> +	pm_runtime_resume(&pl08x->adev->dev);
> > 
> > Shouldn't this be pm_runtime_get_sync() ?
> 
> >> +	pm_runtime_suspend(&pl08x->adev->dev);
> > 
> > And pm_runtime_put() ?
> 
> >> +	pm_runtime_suspend(&adev->dev);
> > 
> > And pm_runtime_put() ?
> 
> I have already fixed above in V3.
> 
> > Lastly, we may want to make this even tighter to the actual period that
> > the DMA is being used, rather than just the period that a driver has
> > been allocated a channel.  I'd rather have it done that way now (and
> > tested) so that we achieve maximal effect from runtime PM, rather than
> > having something which needs to be revisited again.
> > 
> > IOW, if this is worth doing, then its worth doing properly first time.
> 
> Correct. What about this one:
> 
> ---
>  drivers/dma/amba-pl08x.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 3fbaf0e..24353df 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -84,6 +84,7 @@
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <asm/hardware/pl080.h>
> @@ -405,6 +406,7 @@ pl08x_get_phy_channel(struct pl08x_driver_data *pl08x,
>  		return NULL;
>  	}
>  
> +	pm_runtime_get_sync(&pl08x->adev->dev);
this should be ideally one of the first things you would do not last.
get_sync will ensure your .runtime_resume callback is called before it
returns (if its suspended)
>  	return ch;
>  }
>  
> @@ -418,6 +420,8 @@ static inline void pl08x_put_phy_channel(struct pl08x_driver_data *pl08x,
>  	/* Stop the channel and clear its interrupts */
>  	pl08x_terminate_phy_chan(pl08x, ch);
>  
> +	pm_runtime_put(&pl08x->adev->dev);
> +
>  	/* Mark it as free */
>  	ch->serving = NULL;
>  	spin_unlock_irqrestore(&ch->lock, flags);
> @@ -1855,6 +1859,9 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto out_no_pl08x;
>  	}
>  
> +	pm_runtime_set_active(&adev->dev);
> +	pm_runtime_enable(&adev->dev);
> +
>  	/* Initialize memcpy engine */
>  	dma_cap_set(DMA_MEMCPY, pl08x->memcpy.cap_mask);
>  	pl08x->memcpy.dev = &adev->dev;
> @@ -1993,6 +2000,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  	dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n",
>  		 amba_part(adev), amba_rev(adev),
>  		 (unsigned long long)adev->res.start, adev->irq[0]);
> +
> +	pm_runtime_put(&adev->dev);
_put is probe looks suspect, why do you need this here as you are
already setting the status as active, this _put will decrement your
usage count and possibly call your runtime_suspend
>  	return 0;
>  
>  out_no_slave_reg:
> @@ -2011,6 +2020,9 @@ out_no_ioremap:
>  	dma_pool_destroy(pl08x->pool);
>  out_no_lli_pool:
>  out_no_platdata:
> +	pm_runtime_put(&adev->dev);
> +	pm_runtime_disable(&adev->dev);
> +
>  	kfree(pl08x);
>  out_no_pl08x:
>  	amba_release_regions(adev);
> 
> 


-- 
~Vinod

[PATCH V2 08/20] dmaengine/amba-pl08x: support runtime PM

[viresh kumar]

On 08/04/2011 11:06 AM, Koul, Vinod wrote:
> On Thu, 2011-08-04 at 10:49 +0530, viresh kumar wrote:
>> On 08/03/2011 06:09 PM, Russell King - ARM Linux wrote:
>>> On Mon, Aug 01, 2011 at 03:07:18PM +0530, Viresh Kumar wrote:
>> @@ -405,6 +406,7 @@ pl08x_get_phy_channel(struct pl08x_driver_data *pl08x,
>>  		return NULL;
>>  	}
>>  
>> +	pm_runtime_get_sync(&pl08x->adev->dev);
> this should be ideally one of the first things you would do not last.
> get_sync will ensure your .runtime_resume callback is called before it
> returns (if its suspended)
>>  	return ch;
>>  }

Until this point we are not touching the registers at all. And they will
accessed after this point only.

>> +	pm_runtime_put(&adev->dev);
> _put is probe looks suspect, why do you need this here 

To save power.

> as you are already setting the status as active, this _put will decrement your
> usage count and possibly call your runtime_suspend

We set status as active, as amba/bus layer has enabled it before calling
probe and it doesn't put it.

As DMA will not be used until get_phy_channel() is called, so we can save
some energy here too. So i put it here.

-- 
viresh

[PATCH V2 08/20] dmaengine/amba-pl08x: support runtime PM

[Koul, Vinod]

On Thu, 2011-08-04 at 12:01 +0530, viresh kumar wrote:
> On 08/04/2011 11:06 AM, Koul, Vinod wrote:
> > On Thu, 2011-08-04 at 10:49 +0530, viresh kumar wrote:
> >> On 08/03/2011 06:09 PM, Russell King - ARM Linux wrote:
> >>> On Mon, Aug 01, 2011 at 03:07:18PM +0530, Viresh Kumar wrote:
> >> @@ -405,6 +406,7 @@ pl08x_get_phy_channel(struct pl08x_driver_data *pl08x,
> >>  		return NULL;
> >>  	}
> >>  
> >> +	pm_runtime_get_sync(&pl08x->adev->dev);
> > this should be ideally one of the first things you would do not last.
> > get_sync will ensure your .runtime_resume callback is called before it
> > returns (if its suspended)
> >>  	return ch;
> >>  }
> 
> Until this point we are not touching the registers at all. And they will
> accessed after this point only.
But from maintainability POV it should be at the start.

> 
> >> +	pm_runtime_put(&adev->dev);
> > _put is probe looks suspect, why do you need this here 
> 
> To save power.
> 
> > as you are already setting the status as active, this _put will decrement your
> > usage count and possibly call your runtime_suspend
> 
> We set status as active, as amba/bus layer has enabled it before calling
> probe and it doesn't put it.
> 
> As DMA will not be used until get_phy_channel() is called, so we can save
> some energy here too. So i put it here.
> 


-- 
~Vinod

[PATCH V2 07/20] dmaengine/amba-pl08x: Changing few prints to dev_dbg from dev_info

[Koul, Vinod]

On Mon, 2011-08-01 at 15:07 +0530, Viresh Kumar wrote:
> For 8 memory and 16 slave channels 35 boot print lines are printed. And that is
> too much. Most of this would be more useful for debugging. So moving few of them
> to dev_dbg instead of dev_info. Now only 3 prints will be printed.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  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 6c52959..3fbaf0e 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -1717,7 +1717,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
>  			kfree(chan);
>  			continue;
>  		}
> -		dev_info(&pl08x->adev->dev,
> +		dev_dbg(&pl08x->adev->dev,
>  			 "initialize virtual channel \"%s\"\n",
>  			 chan->name);
>  
> @@ -1945,7 +1945,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  		spin_lock_init(&ch->lock);
>  		ch->serving = NULL;
>  		ch->signal = -1;
> -		dev_info(&adev->dev,
> +		dev_dbg(&adev->dev,
>  			 "physical channel %d is %s\n", i,
>  			 pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE");
>  	}
ahh, this looks ugly, can you pls fix the print to make them look
better. Single line or fmt string is one line should be ok, arg can be
moved to second.

-- 
~Vinod

[PATCH V2 08/20] dmaengine/amba-pl08x: support runtime PM

[viresh kumar]

On 08/04/2011 12:58 PM, Koul, Vinod wrote:
>> > 
>> > Until this point we are not touching the registers at all. And they will
>> > accessed after this point only.
> But from maintainability POV it should be at the start.
> 

Sorry for missing this earlier, I looked at the code again and
realized why i put it at the end of the routine.

The routine looks like this

        for (all channels)
                if (!ch->serving)
                        break;

        if (i == pl08x->vd->channels) {
                /* No physical channel available, cope with it */
                return NULL;
        }

        pm_runtime_get_sync(&pl08x->adev->dev);

So, this has to be put at end only. We don't want to call this if no physical
channel is free.

-- 
viresh

[PATCH V3 08/20] dmaengine/amba-pl08x: support runtime PM

[Koul, Vinod]

On Mon, 2011-08-01 at 17:26 +0530, Viresh Kumar wrote:
> Insert notifiers for the runtime PM API. With this the runtime PM layer kicks in
> to action where used. This will also handle enabling/disabling of interface
> clock (Code will be added in amba/bus.c by Russell King).
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/dma/amba-pl08x.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 3fbaf0e..a829f0e 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -84,6 +84,7 @@
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <asm/hardware/pl080.h>
> @@ -879,11 +880,19 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x,
>   */
>  static int pl08x_alloc_chan_resources(struct dma_chan *chan)
>  {
> +	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
> +	struct pl08x_driver_data *pl08x = plchan->host;
> +
> +	pm_runtime_get_sync(&pl08x->adev->dev);
>  	return 0;
>  }
>  
>  static void pl08x_free_chan_resources(struct dma_chan *chan)
>  {
> +	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
> +	struct pl08x_driver_data *pl08x = plchan->host;
> +
> +	pm_runtime_put_sync(&pl08x->adev->dev);
>  }
>  
>  /*
> @@ -1855,6 +1864,9 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto out_no_pl08x;
>  	}
>  
> +	pm_runtime_set_active(&adev->dev);
> +	pm_runtime_enable(&adev->dev);
> +
>  	/* Initialize memcpy engine */
>  	dma_cap_set(DMA_MEMCPY, pl08x->memcpy.cap_mask);
>  	pl08x->memcpy.dev = &adev->dev;
> @@ -1993,6 +2005,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  	dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n",
>  		 amba_part(adev), amba_rev(adev),
>  		 (unsigned long long)adev->res.start, adev->irq[0]);
> +
> +	pm_runtime_put_sync(&adev->dev);
i dont think pm_runtime_put_sync() is the right thing to do here. This
will call your suspend and return after suspend has been called. This
will delay your probe completion and have impact on system boot time,
this is a very wrong approach.
Please move this to _put, so that suspend will be called asynchronously

Also while at it why should you require all the _put calls to be _sync
type?
>  	return 0;
>  
>  out_no_slave_reg:
> @@ -2011,6 +2025,9 @@ out_no_ioremap:
>  	dma_pool_destroy(pl08x->pool);
>  out_no_lli_pool:
>  out_no_platdata:
> +	pm_runtime_put_sync(&adev->dev);
> +	pm_runtime_disable(&adev->dev);
> +
>  	kfree(pl08x);
>  out_no_pl08x:
>  	amba_release_regions(adev);


-- 
~Vinod

[PATCH V2 07/20] dmaengine/amba-pl08x: Changing few prints to dev_dbg from dev_info

[Koul, Vinod]

On Thu, 2011-08-04 at 14:55 +0530, viresh kumar wrote:
> On 08/04/2011 01:58 PM, Koul, Vinod wrote:
> >> > +		dev_dbg(&adev->dev,
> >> >  			 "physical channel %d is %s\n", i,
> >> >  			 pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE");
> >> >  	}
> > ahh, this looks ugly, can you pls fix the print to make them look
> > better. Single line or fmt string is one line should be ok, arg can be
> > moved to second.
> 
> Just to check if I understood your suggestion correctly, you want this:
> 
> 		dev_dbg(&adev->dev, "physical channel %d is %s\n",
> 			i, pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE");
> 
> Surely that will be done.
> 

Much better :)
-- 
~Vinod

[PATCH V2 07/20] dmaengine/amba-pl08x: Changing few prints to dev_dbg from dev_info

[viresh kumar]

On 08/04/2011 01:58 PM, Koul, Vinod wrote:
>> > +		dev_dbg(&adev->dev,
>> >  			 "physical channel %d is %s\n", i,
>> >  			 pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE");
>> >  	}
> ahh, this looks ugly, can you pls fix the print to make them look
> better. Single line or fmt string is one line should be ok, arg can be
> moved to second.

Just to check if I understood your suggestion correctly, you want this:

		dev_dbg(&adev->dev, "physical channel %d is %s\n",
			i, pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE");

Surely that will be done.

-- 
viresh

[PATCH V3 08/20] dmaengine/amba-pl08x: support runtime PM

[viresh kumar]

On 08/04/2011 02:22 PM, Koul, Vinod wrote:
> i dont think pm_runtime_put_sync() is the right thing to do here. This
> will call your suspend and return after suspend has been called. This
> will delay your probe completion and have impact on system boot time,
> this is a very wrong approach.
> Please move this to _put, so that suspend will be called asynchronously
> 
> Also while at it why should you require all the _put calls to be _sync
> type?

That was a mistake, and i already fixed this in a reply to Russell's mail
today.

-- 
viresh

[PATCH V2 18/20] dmaengine/amba-pl08x: Add support for sg len greater than one for slave transfers

[viresh kumar]

On 08/01/2011 03:07 PM, Viresh KUMAR wrote:
> @@ -563,6 +565,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>         u32 cctl, early_bytes = 0;
>         size_t max_bytes_per_lli, total_bytes = 0;

> +       list_for_each_entry(dsg, &txd->dsg_list, node) {
> +               cctl = txd->cctl;
> 
> -       /* We need to count this down to zero */
> -       bd.remainder = txd->len;
> +               bd.srcbus.addr = dsg->src_addr;
> +               bd.dstbus.addr = dsg->dst_addr;
> +               bd.remainder = dsg->len;
> +               bd.srcbus.buswidth = bd.srcbus.maxwidth;
> +               bd.dstbus.buswidth = bd.dstbus.maxwidth;
> 

> +               if (total_bytes != dsg->len) {
> +                       dev_err(&pl08x->adev->dev,
> +                               "%s size of encoded lli:s don't match total txd, transferred 0x%08zx from size 0x%08zx\n",
> +                               __func__, total_bytes, dsg->len);
> +                       return 0;
> +               }
>         }

Correction:

total_bytes must be reinitialized to zero inside list_for_each_entry loop.
As we are comparing it to dsg->len for every dsg.

Will fix this and send with V3.

-- 
viresh

[PATCH V3 08/20] dmaengine/amba-pl08x: support runtime PM

[Viresh Kumar]

Insert notifiers for the runtime PM API. With this the runtime PM layer kicks in
to action where used.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index ead88c9..5dd97f4 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -84,6 +84,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <asm/hardware/pl080.h>
@@ -405,6 +406,7 @@ pl08x_get_phy_channel(struct pl08x_driver_data *pl08x,
 		return NULL;
 	}
 
+	pm_runtime_get_sync(&pl08x->adev->dev);
 	return ch;
 }
 
@@ -418,6 +420,8 @@ static inline void pl08x_put_phy_channel(struct pl08x_driver_data *pl08x,
 	/* Stop the channel and clear its interrupts */
 	pl08x_terminate_phy_chan(pl08x, ch);
 
+	pm_runtime_put(&pl08x->adev->dev);
+
 	/* Mark it as free */
 	ch->serving = NULL;
 	spin_unlock_irqrestore(&ch->lock, flags);
@@ -1855,6 +1859,9 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 		goto out_no_pl08x;
 	}
 
+	pm_runtime_set_active(&adev->dev);
+	pm_runtime_enable(&adev->dev);
+
 	/* Initialize memcpy engine */
 	dma_cap_set(DMA_MEMCPY, pl08x->memcpy.cap_mask);
 	pl08x->memcpy.dev = &adev->dev;
@@ -1992,6 +1999,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n",
 		 amba_part(adev), amba_rev(adev),
 		 (unsigned long long)adev->res.start, adev->irq[0]);
+
+	pm_runtime_put(&adev->dev);
 	return 0;
 
 out_no_slave_reg:
@@ -2010,6 +2019,9 @@ out_no_ioremap:
 	dma_pool_destroy(pl08x->pool);
 out_no_lli_pool:
 out_no_platdata:
+	pm_runtime_put(&adev->dev);
+	pm_runtime_disable(&adev->dev);
+
 	kfree(pl08x);
 out_no_pl08x:
 	amba_release_regions(adev);
-- 
1.7.2.2