[PATCH 00/18] dmaengine/amba-pl08x updates

[PATCH 00/18] 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
- 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


Viresh Kumar (18):
  ARM: asm/pl080.h: Protect against multiple inclusion of header file
  dmaengine/amba-pl08x: Resolve formatting issues
  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: Simplify pl08x_ensure_on()
  dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests
  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              |  671 ++++++++++++++++-----------------
 include/linux/amba/pl08x.h            |   33 ++-
 3 files changed, 347 insertions(+), 361 deletions(-)

-- 
1.7.2.2

[PATCH 01/18] 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>
---
 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 e4a04e4..33c78d7 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 01/18] ARM: asm/pl080.h: Protect against multiple inclusion of header file

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> <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>

Good idea.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

[PATCH 02/18] 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 and spaces
- align code to 80 cols
- Don't mix spaces and tabs (use tabs instead)
- combine several lines to one line

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

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 196a737..1f7c510 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -12,12 +12,12 @@
  *
  * This program is distributed in the hope that it will be useful, but WITHOUT
  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
  * more details.
  *
  * You should have received a copy of the GNU General Public License along with
  * this program; if not, write to the Free Software Foundation, Inc., 59
- * Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+ * Temple Place - Suite 330, Boston, MA 02111-1307, USA.
  *
  * The full GNU General Public License is in this distribution in the file
  * called COPYING.
@@ -56,18 +56,18 @@
  * The PL08x has two flow control settings:
  *  - DMAC flow control: the transfer size defines the number of transfers
  *    which occur for the current LLI entry, and the DMAC raises TC at the
- *    end of every LLI entry.  Observed behaviour shows the DMAC listening
+ *    end of every LLI entry. Observed behaviour shows the DMAC listening
  *    to both the BREQ and SREQ signals (contrary to documented),
- *    transferring data if either is active.  The LBREQ and LSREQ signals
+ *    transferring data if either is active. The LBREQ and LSREQ signals
  *    are ignored.
  *
  *  - Peripheral flow control: the transfer size is ignored (and should be
- *    zero).  The data is transferred from the current LLI entry, until
- *    after the final transfer signalled by LBREQ or LSREQ.  The DMAC
+ *    zero). The data is transferred from the current LLI entry, until
+ *    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
+ * 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.
  *
@@ -103,8 +103,8 @@ struct vendor_data {
 
 /*
  * PL08X private data structures
- * An LLI struct - see PL08x TRM.  Note that next uses bit[0] as a bus bit,
- * start & end do not - their bus bit info is in cctl.  Also note that these
+ * An LLI struct - see PL08x TRM. Note that next uses bit[0] as a bus bit,
+ * start & end do not - their bus bit info is in cctl. Also note that these
  * are fixed 32-bit quantities.
  */
 struct pl08x_lli {
@@ -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
  */
@@ -189,7 +190,7 @@ static int pl08x_phy_channel_busy(struct pl08x_phy_chan *ch)
 /*
  * Set the initial DMA register values i.e. those for the first LLI
  * The next LLI pointer and the configuration interrupt bit have
- * been set when the LLIs were constructed.  Poke them into the hardware
+ * been set when the LLIs were constructed. Poke them into the hardware
  * and start the transfer.
  */
 static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
@@ -271,10 +272,9 @@ 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
+ * clears any pending interrupt status. This should not be used for
  * an on-going transfer, but as a method of shutting down a channel
  * (eg, when it's no longer used) or terminating a transfer.
  */
@@ -284,7 +284,7 @@ static void pl08x_terminate_phy_chan(struct pl08x_driver_data *pl08x,
 	u32 val = readl(ch->base + PL080_CH_CONFIG);
 
 	val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
-	         PL080_CONFIG_TC_IRQ_MASK);
+			PL080_CONFIG_TC_IRQ_MASK);
 
 	writel(val, ch->base + PL080_CH_CONFIG);
 
@@ -380,7 +380,7 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
  */
 static struct pl08x_phy_chan *
 pl08x_get_phy_channel(struct pl08x_driver_data *pl08x,
-		      struct pl08x_dma_chan *virt_chan)
+		struct pl08x_dma_chan *virt_chan)
 {
 	struct pl08x_phy_chan *ch = NULL;
 	unsigned long flags;
@@ -445,7 +445,7 @@ static inline unsigned int pl08x_get_bytes_for_cctl(unsigned int coded)
 }
 
 static inline u32 pl08x_cctl_bits(u32 cctl, u8 srcwidth, u8 dstwidth,
-				  size_t tsize)
+		size_t tsize)
 {
 	u32 retbits = cctl;
 
@@ -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)
@@ -577,18 +578,16 @@ static inline size_t pl08x_pre_boundary(u32 addr, size_t len)
  * Return 0 for error
  */
 static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
-			      struct pl08x_txd *txd)
+		struct pl08x_txd *txd)
 {
 	struct pl08x_bus_data *mbus, *sbus;
 	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;
@@ -607,12 +606,12 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	/* Find maximum width of the source bus */
 	bd.srcbus.maxwidth =
 		pl08x_get_bytes_for_cctl((cctl & PL080_CONTROL_SWIDTH_MASK) >>
-				       PL080_CONTROL_SWIDTH_SHIFT);
+				PL080_CONTROL_SWIDTH_SHIFT);
 
 	/* Find maximum width of the destination bus */
 	bd.dstbus.maxwidth =
 		pl08x_get_bytes_for_cctl((cctl & PL080_CONTROL_DWIDTH_MASK) >>
-				       PL080_CONTROL_DWIDTH_SHIFT);
+				PL080_CONTROL_DWIDTH_SHIFT);
 
 	/* Set up the bus widths to the maximum */
 	bd.srcbus.buswidth = bd.srcbus.maxwidth;
@@ -634,7 +633,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	 */
 	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 "
+			"llimax=%zu\n",
 		 bd.srcbus.addr, cctl & PL080_CONTROL_SRC_INCR ? "+" : "",
 		 bd.srcbus.buswidth,
 		 bd.dstbus.addr, cctl & PL080_CONTROL_DST_INCR ? "+" : "",
@@ -713,7 +713,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 			/* Find the nearest */
 			lli_len	= min(bd.srcbus.fill_bytes,
-				      bd.dstbus.fill_bytes);
+					bd.dstbus.fill_bytes);
 
 			BUG_ON(lli_len > bd.remainder);
 
@@ -735,16 +735,15 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			} 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
+				 * 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
+				 * 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) {
@@ -756,30 +755,32 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				 */
 				/* FIXME: use round_down()? */
 				tsize = lli_len / min(mbus->buswidth,
-						      sbus->buswidth);
+						sbus->buswidth);
 				lli_len	= tsize * min(mbus->buswidth,
-						      sbus->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);
+					"%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);
+						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",
+					"%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;
 			}
 
-
 			if (odd_bytes) {
 				/*
 				 * Creep past the boundary, maintaining
@@ -790,7 +791,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 						&& (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",
+						"%s align with boundary, single"
+						" byte (remain 0x%08zx)\n",
 						__func__, bd.remainder);
 					pl08x_fill_lli_for_desc(&bd,
 						num_llis++, 1, cctl);
@@ -805,16 +807,17 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		while (bd.remainder) {
 			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
 			dev_vdbg(&pl08x->adev->dev,
-				"%s align with boundary, single odd byte (remain %zu)\n",
-				__func__, bd.remainder);
+				"%s align with boundary, single odd byte "
+				"(remain %zu)\n", __func__, bd.remainder);
 			pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
 			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);
+			"%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;
 	}
 
@@ -853,7 +856,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 /* You should call this with the struct pl08x lock held */
 static void pl08x_free_txd(struct pl08x_driver_data *pl08x,
-			   struct pl08x_txd *txd)
+		struct pl08x_txd *txd)
 {
 	/* Free the LLI */
 	dma_pool_free(pl08x->pool, txd->llis_va, txd->llis_bus);
@@ -894,7 +897,7 @@ static void pl08x_free_chan_resources(struct dma_chan *chan)
  * This should be called with the channel plchan->lock held
  */
 static int prep_phy_channel(struct pl08x_dma_chan *plchan,
-			    struct pl08x_txd *txd)
+		struct pl08x_txd *txd)
 {
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_phy_chan *ch;
@@ -907,7 +910,8 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan,
 	ch = pl08x_get_phy_channel(pl08x, plchan);
 	if (!ch) {
 		/* No physical channel available, cope with it */
-		dev_dbg(&pl08x->adev->dev, "no physical channel available for xfer on %s\n", plchan->name);
+		dev_dbg(&pl08x->adev->dev, "no physical channel available for "
+				"xfer on %s\n", plchan->name);
 		return -EBUSY;
 	}
 
@@ -916,13 +920,12 @@ 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,
-				"unable to use physical channel %d for transfer on %s due to platform restrictions\n",
+				"unable to use physical channel %d for transfer"
+				" on %s due to platform restrictions\n",
 				ch->id, plchan->name);
 			/* Release physical channel & return */
 			pl08x_put_phy_channel(pl08x, ch);
@@ -937,7 +940,8 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan,
 			txd->ccfg |= ch->signal << PL080_CONFIG_SRC_SEL_SHIFT;
 	}
 
-	dev_dbg(&pl08x->adev->dev, "allocated physical channel %d and signal %d for xfer on %s\n",
+	dev_dbg(&pl08x->adev->dev, "allocated physical channel %d and signal %d"
+			" for xfer on %s\n",
 		 ch->id,
 		 ch->signal,
 		 plchan->name);
@@ -1007,10 +1011,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;
@@ -1089,7 +1091,7 @@ static const struct burst_table burst_sizes[] = {
 
 /*
  * Given the source and destination available bus masks, select which
- * will be routed to each port.  We try to have source and destination
+ * will be routed to each port. We try to have source and destination
  * on separate ports, but always respect the allowable settings.
  */
 static u32 pl08x_select_bus(u8 src, u8 dst)
@@ -1107,8 +1109,8 @@ static u32 pl08x_select_bus(u8 src, u8 dst)
 static u32 pl08x_cctl(u32 cctl)
 {
 	cctl &= ~(PL080_CONTROL_SRC_AHB2 | PL080_CONTROL_DST_AHB2 |
-		  PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR |
-		  PL080_CONTROL_PROT_MASK);
+			PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR |
+			PL080_CONTROL_PROT_MASK);
 
 	/* Access the cell in privileged mode, non-bufferable, non-cacheable */
 	return cctl | PL080_CONTROL_PROT_SYS;
@@ -1140,7 +1142,7 @@ static u32 pl08x_burst(u32 maxburst)
 }
 
 static int dma_set_runtime_config(struct dma_chan *chan,
-				  struct dma_slave_config *config)
+		struct dma_slave_config *config)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
 	struct pl08x_driver_data *pl08x = plchan->host;
@@ -1177,7 +1179,7 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 
 	/*
 	 * If this channel will only request single transfers, set this
-	 * down to ONE element.  Also select one element if no maxburst
+	 * down to ONE element. Also select one element if no maxburst
 	 * is specified.
 	 */
 	if (plchan->cd->single)
@@ -1270,7 +1272,7 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 		 * memcpy transfers can be sorted out at submission time.
 		 *
 		 * Slave transfers may have been denied due to platform
-		 * channel muxing restrictions.  Since there is no guarantee
+		 * channel muxing restrictions. Since there is no guarantee
 		 * that this will ever be resolved, and the signal must be
 		 * acquired AFTER acquiring the physical channel, we will let
 		 * them be NACK:ed with -EBUSY here. The drivers can retry
@@ -1310,7 +1312,7 @@ static struct pl08x_txd *pl08x_get_txd(struct pl08x_dma_chan *plchan,
 
 		/* Always enable error and terminal interrupts */
 		txd->ccfg = PL080_CONFIG_ERR_IRQ_MASK |
-			    PL080_CONFIG_TC_IRQ_MASK;
+			PL080_CONFIG_TC_IRQ_MASK;
 	}
 	return txd;
 }
@@ -1349,7 +1351,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 
 	if (pl08x->vd->dualmaster)
 		txd->cctl |= pl08x_select_bus(pl08x->mem_buses,
-					      pl08x->mem_buses);
+				pl08x->mem_buses);
 
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
@@ -1377,8 +1379,8 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		BUG();
 	}
 
-	dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from %s\n",
-		__func__, sgl->length, plchan->name);
+	dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from"
+			" %s\n", __func__, sgl->length, plchan->name);
 
 	txd = pl08x_get_txd(plchan, flags);
 	if (!txd) {
@@ -1400,12 +1402,14 @@ 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->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->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;
@@ -1433,7 +1437,7 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 	/* Controls applicable to inactive channels */
 	if (cmd == DMA_SLAVE_CONFIG) {
 		return dma_set_runtime_config(chan,
-					      (struct dma_slave_config *)arg);
+				(struct dma_slave_config *)arg);
 	}
 
 	/*
@@ -1567,7 +1571,7 @@ static void pl08x_tasklet(unsigned long data)
 	} else if (plchan->phychan_hold) {
 		/*
 		 * This channel is still in use - we have a new txd being
-		 * prepared and will soon be queued.  Don't give up the
+		 * prepared and will soon be queued. Don't give up the
 		 * physical channel.
 		 */
 	} else {
@@ -1587,14 +1591,14 @@ static void pl08x_tasklet(unsigned long data)
 		 * up while we were choking the physical channels with data.
 		 */
 		list_for_each_entry(waiting, &pl08x->memcpy.channels,
-				    chan.device_node) {
-		  if (waiting->state == PL08X_CHAN_WAITING &&
-			    waiting->waiting != NULL) {
+				chan.device_node) {
+			if (waiting->state == PL08X_CHAN_WAITING &&
+					waiting->waiting != NULL) {
 				int ret;
 
 				/* This should REALLY not fail now */
 				ret = prep_phy_channel(waiting,
-						       waiting->waiting);
+						waiting->waiting);
 				BUG_ON(ret);
 				waiting->phychan_hold--;
 				waiting->state = PL08X_CHAN_RUNNING;
@@ -1684,9 +1688,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;
@@ -1738,7 +1740,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 		spin_lock_init(&chan->lock);
 		INIT_LIST_HEAD(&chan->pend_list);
 		tasklet_init(&chan->tasklet, pl08x_tasklet,
-			     (unsigned long) chan);
+				(unsigned long) chan);
 
 		list_add_tail(&chan->chan.device_node, &dmadev->channels);
 	}
@@ -1797,7 +1799,7 @@ static int pl08x_debugfs_show(struct seq_file *s, void *data)
 		virt_chan = ch->serving;
 
 		seq_printf(s, "%d\t\t%s\n",
-			   ch->id, virt_chan ? virt_chan->name : "(none)");
+				ch->id, virt_chan ? virt_chan->name : "(none)");
 
 		spin_unlock_irqrestore(&ch->lock, flags);
 	}
@@ -1807,7 +1809,7 @@ static int pl08x_debugfs_show(struct seq_file *s, void *data)
 	seq_printf(s, "--------\t------\n");
 	list_for_each_entry(chan, &pl08x->memcpy.channels, chan.device_node) {
 		seq_printf(s, "%s\t\t%s\n", chan->name,
-			   pl08x_state_str(chan->state));
+				pl08x_state_str(chan->state));
 	}
 
 	seq_printf(s, "\nPL08x virtual slave channels:\n");
@@ -1815,7 +1817,7 @@ static int pl08x_debugfs_show(struct seq_file *s, void *data)
 	seq_printf(s, "--------\t------\n");
 	list_for_each_entry(chan, &pl08x->slave.channels, chan.device_node) {
 		seq_printf(s, "%s\t\t%s\n", chan->name,
-			   pl08x_state_str(chan->state));
+				pl08x_state_str(chan->state));
 	}
 
 	return 0;
@@ -1836,9 +1838,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
@@ -1898,7 +1900,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	pl08x->adev = adev;
 	pl08x->vd = vd;
 
-	/* By default, AHB1 only.  If dualmaster, from platform */
+	/* By default, AHB1 only. If dualmaster, from platform */
 	pl08x->lli_buses = PL08X_AHB1;
 	pl08x->mem_buses = PL08X_AHB1;
 	if (pl08x->vd->dualmaster) {
@@ -1930,7 +1932,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	writel(0x000000FF, pl08x->base + PL080_TC_CLEAR);
 
 	ret = request_irq(adev->irq[0], pl08x_irq, IRQF_DISABLED,
-			  DRIVER_NAME, pl08x);
+			DRIVER_NAME, pl08x);
 	if (ret) {
 		dev_err(&adev->dev, "%s failed to request interrupt %d\n",
 			__func__, adev->irq[0]);
@@ -1938,8 +1940,8 @@ 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)),
-			GFP_KERNEL);
+	pl08x->phy_chans = kmalloc((vd->channels *
+				sizeof(struct pl08x_phy_chan)), GFP_KERNEL);
 	if (!pl08x->phy_chans) {
 		dev_err(&adev->dev, "%s failed to allocate "
 			"physical channel holders\n",
@@ -1962,7 +1964,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 
 	/* Register as many memcpy channels as there are physical channels */
 	ret = pl08x_dma_init_virtual_channels(pl08x, &pl08x->memcpy,
-					      pl08x->vd->channels, false);
+			pl08x->vd->channels, false);
 	if (ret <= 0) {
 		dev_warn(&pl08x->adev->dev,
 			 "%s failed to enumerate memcpy channels - %d\n",
@@ -1973,8 +1975,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",
@@ -1985,9 +1986,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 
 	ret = dma_async_device_register(&pl08x->memcpy);
 	if (ret) {
-		dev_warn(&pl08x->adev->dev,
-			"%s failed to register memcpy as an async device - %d\n",
-			__func__, ret);
+		dev_warn(&pl08x->adev->dev, "%s failed to register memcpy as an"
+				" async device - %d\n", __func__, ret);
 		goto out_no_memcpy_reg;
 	}
 
@@ -2073,8 +2073,8 @@ static int __init pl08x_init(void)
 	retval = amba_driver_register(&pl08x_amba_driver);
 	if (retval)
 		printk(KERN_WARNING DRIVER_NAME
-		       "failed to register as an AMBA device (%d)\n",
-		       retval);
+			"failed to register as an AMBA device (%d)\n",
+			retval);
 	return retval;
 }
 subsys_initcall(pl08x_init);
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index e6e28f3..3e25825 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -121,7 +121,7 @@ struct pl08x_txd {
 	u32 cctl;
 	/*
 	 * Settings to be put into the physical channel when we
-	 * trigger this txd.  Other registers are in llis_va[0].
+	 * trigger this txd. Other registers are in llis_va[0].
 	 */
 	u32 ccfg;
 };
-- 
1.7.2.2

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

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 04:19:12PM +0530, Viresh Kumar wrote:
> There were few formatting related issues in code. This patch fixes them.
> Fixes include:
> - Remove extra blank lines and spaces

A double space after a full-stop is standard practice for many people.
Please don't reformat that.

> @@ -284,7 +284,7 @@ static void pl08x_terminate_phy_chan(struct pl08x_driver_data *pl08x,
>  	u32 val = readl(ch->base + PL080_CH_CONFIG);
>  
>  	val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
> -	         PL080_CONFIG_TC_IRQ_MASK);
> +			PL080_CONFIG_TC_IRQ_MASK);

This looks worse than the original.

>  
>  	writel(val, ch->base + PL080_CH_CONFIG);
>  
> @@ -380,7 +380,7 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
>   */
>  static struct pl08x_phy_chan *
>  pl08x_get_phy_channel(struct pl08x_driver_data *pl08x,
> -		      struct pl08x_dma_chan *virt_chan)
> +		struct pl08x_dma_chan *virt_chan)

Ditto.
>  {
>  	struct pl08x_phy_chan *ch = NULL;
>  	unsigned long flags;
> @@ -445,7 +445,7 @@ static inline unsigned int pl08x_get_bytes_for_cctl(unsigned int coded)
>  }
>  
>  static inline u32 pl08x_cctl_bits(u32 cctl, u8 srcwidth, u8 dstwidth,
> -				  size_t tsize)
> +		size_t tsize)

Ditto.

And many more of that.

>  	ret = dma_async_device_register(&pl08x->memcpy);
>  	if (ret) {
> -		dev_warn(&pl08x->adev->dev,
> -			"%s failed to register memcpy as an async device - %d\n",
> -			__func__, ret);
> +		dev_warn(&pl08x->adev->dev, "%s failed to register memcpy as an"
> +				" async device - %d\n", __func__, ret);

Definitely not acceptable.  Ignore warnings about wrapping 80 columns for
kernel messages.

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

[viresh kumar]

On 07/29/2011 04:30 PM, Russell King - ARM Linux wrote:
> On Fri, Jul 29, 2011 at 04:19:12PM +0530, Viresh Kumar wrote:
>> - Remove extra blank lines and spaces
> 
> A double space after a full-stop is standard practice for many people.
> Please don't reformat that.
> 

Ok. I didn't knew it.

>> @@ -284,7 +284,7 @@ static void pl08x_terminate_phy_chan(struct pl08x_driver_data *pl08x,
>>  	u32 val = readl(ch->base + PL080_CH_CONFIG);
>>  
>>  	val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
>> -	         PL080_CONFIG_TC_IRQ_MASK);
>> +			PL080_CONFIG_TC_IRQ_MASK);
> 
> This looks worse than the original.
> 

> 
> And many more of that.
> 

Ok. Should i leave tabs and spaces mixed or just delete one tab in my change.

>>  	ret = dma_async_device_register(&pl08x->memcpy);
>>  	if (ret) {
>> -		dev_warn(&pl08x->adev->dev,
>> -			"%s failed to register memcpy as an async device - %d\n",
>> -			__func__, ret);
>> +		dev_warn(&pl08x->adev->dev, "%s failed to register memcpy as an"
>> +				" async device - %d\n", __func__, ret);
> 
> Definitely not acceptable.  Ignore warnings about wrapping 80 columns for
> kernel messages.

Ok.

-- 
viresh

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

[Koul, Vinod]

On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> There were few formatting related issues in code. This patch fixes them.
> Fixes include:
> - Remove extra blank lines and spaces
> - align code to 80 cols
> - Don't mix spaces and tabs (use tabs instead)
> - combine several lines to one line
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/dma/amba-pl08x.c   |  192 ++++++++++++++++++++++----------------------
>  include/linux/amba/pl08x.h |    2 +-
>  2 files changed, 97 insertions(+), 97 deletions(-)
> 
>  
> @@ -735,16 +735,15 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  			} 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
> +				 * 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
> +				 * bus. We need to ensure the master stays
>  				 * aligned, and that we are working in
>  				 * multiples of the bus widths.
>  				 */
This looks v ugly, care to fix this.
80chars should be followed with a little common sense, if it sacrifices
code readability then pls ignore it.
In places where you can fix it w/o sacrificing code quality pls do it :)

>  				odd_bytes = lli_len % mbus->buswidth;
>  				lli_len -= odd_bytes;
> -
>  			}
>  
>  			if (lli_len) {
> @@ -756,30 +755,32 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>  				 */
>  				/* FIXME: use round_down()? */
while you are fixing the driver, care to fix the FIXME as well?
>  				tsize = lli_len / min(mbus->buswidth,
> -						      sbus->buswidth);
> +						sbus->buswidth);
>  				lli_len	= tsize * min(mbus->buswidth,
> -						      sbus->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);
> +					"%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);
>  				}
Oh no, you don't want to do this, see above...

Okay, looking below mostly it would be comments like these, please go
thru above and apply fixes only where they make sense


-- 
~Vinod

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

[viresh kumar]

On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
>>  				/*
>>  				 * So now we know how many bytes to transfer
>> -				 * to get to the nearest boundary.  The next
>> -				 * LLI will past the boundary.  However, we
>> +				 * 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
>> +				 * bus. We need to ensure the master stays
>>  				 * aligned, and that we are working in
>>  				 * multiples of the bus widths.
>>  				 */
> This looks v ugly, care to fix this.
> 80chars should be followed with a little common sense, if it sacrifices
> code readability then pls ignore it.
> In places where you can fix it w/o sacrificing code quality pls do it :)

Ok.

>>  				/* FIXME: use round_down()? */
> while you are fixing the driver, care to fix the FIXME as well?

Already removed this. :)

--
viresh

[PATCH 03/18] 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 3e25825..5509a50 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 04/18] 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 |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 1f7c510..4a64fdf 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -498,9 +498,9 @@ 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
+ * - 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)
@@ -626,11 +626,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 "
-- 
1.7.2.2

[PATCH 04/18] dmaengine/amba-pl08x: Remove redundant comment and rewrite original

[Koul, Vinod]

On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> 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 |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 1f7c510..4a64fdf 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -498,9 +498,9 @@ 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
> + * - prefers the destination bus if both available
> + * - if fixed address on one bus the other will be chosen
Not sure I get it, English is not my first language :)
>   */
>  static void pl08x_choose_master_bus(struct pl08x_lli_build_data *bd,
>  	struct pl08x_bus_data **mbus, struct pl08x_bus_data **sbus, u32 cctl)
> @@ -626,11 +626,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 "


-- 
~Vinod

[PATCH 04/18] dmaengine/amba-pl08x: Remove redundant comment and rewrite original

[viresh kumar]

On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
>>  /*
>> - * 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
>> + * - prefers the destination bus if both available
>> + * - if fixed address on one bus the other will be chosen
> Not sure I get it, English is not my first language :)

Nor mine either. :)

Will give some more explanation.

But before that i wanted to know the exact purpose of this
master-slave concept here.
Probably it is for choosing the victim for reduction of width, in case
src and dest addresses
are misaligned to each other.
@Russell and Linus: Is this correct?

--
viresh

[PATCH 04/18] dmaengine/amba-pl08x: Remove redundant comment and rewrite original

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 08:47:06AM -0700, viresh kumar wrote:
> On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> > On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> >>  /*
> >> - * 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
> >> + * - prefers the destination bus if both available
> >> + * - if fixed address on one bus the other will be chosen
> > Not sure I get it, English is not my first language :)
> 
> Nor mine either. :)
> 
> Will give some more explanation.
> 
> But before that i wanted to know the exact purpose of this
> master-slave concept here.
> Probably it is for choosing the victim for reduction of width, in case
> src and dest addresses
> are misaligned to each other.
> @Russell and Linus: Is this correct?

No idea - most of the LLI code that's in this driver came from within
ARM Ltd, and I've dared not change its behaviour in case the original
author knew something that isn't public.

In addition, as the ARM platforms I have with a PL080 on seem to have
their DMA support buggered beyond belief (that's putting it mildly),
I've not really been able to test anything more complex than single
byte DMA to a UART.  Linus W has one of the rare ARM platforms which
has a working implementation.

I'm not sure why the function is named in terms of master/slave because
those terms don't seem to have much to do with what its actually doing
(which is selecting either the source or destination to handle alignment
issues with.)  And it's not really about busses either.

If the LLI creation code can be greatly simplified and that function
killed off, that'd certainly be a great step forward.  In the mean time
I'd suggest leaving that comment as-is as long as the function exists.

[PATCH 04/18] dmaengine/amba-pl08x: Remove redundant comment and rewrite original

[Linus Walleij]

2011/7/29 viresh kumar <viresh.linux@gmail.com>:
> On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
>> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
>>> ?/*
>>> - * 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
>>> + * - prefers the destination bus if both available
>>> + * - if fixed address on one bus the other will be chosen
>> Not sure I get it, English is not my first language :)
>
> Nor mine either. :)
>
> Will give some more explanation.
>
> But before that i wanted to know the exact purpose of this
> master-slave concept here.

The PL08x has two bus mastering interfaces. They can be
connected to two separate busses and two adress spaces
even, usually that is not the case though.

> Probably it is for choosing the victim for reduction of width, in case
> src and dest addresses
> are misaligned to each other.
> @Russell and Linus: Is this correct?

You can see from the function that it is only about choosing the
master and slave. One of them is reading from some memory
and one of them is writing. These are called source and
destination respectively.

What it then does is:

- If one of the buses will be targeting a fixed
  address, choose the other one, so the one that increase address
  become master and the fixed address slave
.
- If both buses increas address (usually memory-to-memory) the
  third case is activated and deals with trying to set the widest
  bus as master.

Crystal clear?

Please copyedit/author something like the above...

Thanks,
Linus Walleij

[PATCH 04/18] dmaengine/amba-pl08x: Remove redundant comment and rewrite original

[Russell King - ARM Linux]

On Sun, Jul 31, 2011 at 12:32:55AM +0200, Linus Walleij wrote:
> 2011/7/29 viresh kumar <viresh.linux@gmail.com>:
> > Nor mine either. :)
> >
> > Will give some more explanation.
> >
> > But before that i wanted to know the exact purpose of this
> > master-slave concept here.
> 
> The PL08x has two bus mastering interfaces. They can be
> connected to two separate busses and two adress spaces
> even, usually that is not the case though.

This is not what the function is about... that's done by my
pl08x_select_bus() function.

This function (pl08x_choose_master_bus) does something completely
different, and has to do with how the LLIs are laid out.  I think
it's to do with whether we use the source or destination for
alignment purposes.

It really has nothing to do with choosing busses.

[PATCH 04/18] dmaengine/amba-pl08x: Remove redundant comment and rewrite original

[Linus Walleij]

2011/7/31 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sun, Jul 31, 2011 at 12:32:55AM +0200, Linus Walleij wrote:
>> The PL08x has two bus mastering interfaces. They can be
>> connected to two separate busses and two adress spaces
>> even, usually that is not the case though.
>
> This is not what the function is about... that's done by my
> pl08x_select_bus() function.

To be clear, I was explaining what I percieved as a
question from Viresh about what the comment in this hunk of
the patch was about:

@@ -498,9 +498,9 @@ 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
+ * - 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)

This is the comment just above pl08x_choose_master_bus(),
the pl08x_lli_build_data() just has a danglig semicolon in the hunk.

As for the LLI code, all of it's weird complexity comes from the fact
that the DMAC cannot let a single element pass across 1KB (0x400)
boundaries, which is one more than a little insane hardware
restriction, and I have no clue where that limitation actually comes
from, likely strange VHDL code.

Then for this hunk:

@@ -626,11 +626,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);

This is I think just redundant commenting of what the called function does,
so yes should be deleted.

Thanks,
Linus Walleij

[PATCH 04/18] dmaengine/amba-pl08x: Remove redundant comment and rewrite original

[viresh kumar]

On 7/30/11, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/7/31 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> On Sun, Jul 31, 2011 at 12:32:55AM +0200, Linus Walleij wrote:
>>> The PL08x has two bus mastering interfaces. They can be
>>> connected to two separate busses and two adress spaces
>>> even, usually that is not the case though.
>>
>> This is not what the function is about... that's done by my
>> pl08x_select_bus() function.
>
> To be clear, I was explaining what I percieved as a
> question from Viresh about what the comment in this hunk of
> the patch was about:
>

Just to clear my question again:
- It wasn't about the comment but concept behind Master/Slave terminology.
- Why are we referring src and dest as Master and Slave. As there is
nothing like that.
- As pointed by Russell and me earlier: Probably it is for choosing
the victim for
reduction of width, in case src and dest addresses are misaligned to each other.

If what i understood is correct, then we should get rid of this
routine or this master/slave
terminology, as this is causing confusion.

--
viresh

[PATCH 04/18] dmaengine/amba-pl08x: Remove redundant comment and rewrite original

[Russell King - ARM Linux]

On Sun, Jul 31, 2011 at 01:37:46AM +0200, Linus Walleij wrote:
> 2011/7/31 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Sun, Jul 31, 2011 at 12:32:55AM +0200, Linus Walleij wrote:
> >> The PL08x has two bus mastering interfaces. They can be
> >> connected to two separate busses and two adress spaces
> >> even, usually that is not the case though.
> >
> > This is not what the function is about... that's done by my
> > pl08x_select_bus() function.
> 
> To be clear, I was explaining what I percieved as a
> question from Viresh about what the comment in this hunk of
> the patch was about:

Yes, and the comment and function name are wrong for what it's doing.
I repeat: it has nothing to do with selecting an AHB bus.

> As for the LLI code, all of it's weird complexity comes from the fact
> that the DMAC cannot let a single element pass across 1KB (0x400)
> boundaries, which is one more than a little insane hardware
> restriction, and I have no clue where that limitation actually comes
> from, likely strange VHDL code.

Viresh's discussion with ARM Ltd indicates that this is not the case,
and that restriction comes from misunderstanding of the documentation.

[PATCH 05/18] 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>
---
 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 4a64fdf..56a9670 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1724,7 +1724,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);
 
@@ -1952,7 +1952,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 05/18] dmaengine/amba-pl08x: Changing few prints to dev_dbg from dev_info

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> 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>

Oh I really liked these prints but whatever.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

[PATCH 06/18] 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>
---
 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 56a9670..53d722e 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1505,13 +1505,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 06/18] dmaengine/amba-pl08x: Simplify pl08x_ensure_on()

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> 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>

Obviously correct,
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

[PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests

[Viresh Kumar]

Amba devices have interface clocks attached as adev->pclk. This is the only
clock associated with DMAC and it can be disabled when DMAC is not in use. This
patch adds support to enable/disable this clk as and when channels are requested
and freed. Also it disables this clock at the end of probe.

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

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 53d722e..f1a25cf 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -881,11 +881,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;
+
+	amba_pclk_enable(pl08x->adev);
 	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;
+
+	amba_pclk_disable(pl08x->adev);
 }
 
 /*
@@ -1993,6 +2001,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]);
+
+	amba_pclk_disable(adev);
 	return 0;
 
 out_no_slave_reg:
-- 
1.7.2.2

[PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 04:19:17PM +0530, Viresh Kumar wrote:
> Amba devices have interface clocks attached as adev->pclk. This is the only
> clock associated with DMAC and it can be disabled when DMAC is not in use. This
> patch adds support to enable/disable this clk as and when channels are requested
> and freed. Also it disables this clock at the end of probe.

It may make better sense to convert this to runtime PM.  I suspect
that there's core support which the amba/bus.c can do to help in that
respect (eg, managing the apb pclk itself) so that we don't have to
add the same code to every primecell driver.

[PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests

[Russell King - ARM Linux]

On Sat, Jul 30, 2011 at 01:07:40PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 29, 2011 at 04:19:17PM +0530, Viresh Kumar wrote:
> > Amba devices have interface clocks attached as adev->pclk. This is the only
> > clock associated with DMAC and it can be disabled when DMAC is not in use. This
> > patch adds support to enable/disable this clk as and when channels are requested
> > and freed. Also it disables this clock at the end of probe.
> 
> It may make better sense to convert this to runtime PM.  I suspect
> that there's core support which the amba/bus.c can do to help in that
> respect (eg, managing the apb pclk itself) so that we don't have to
> add the same code to every primecell driver.

Something like this for the bus driver (untested):

 drivers/amba/bus.c |   38 ++++++++++++++++++++++++++++++++++++--
 1 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index d74926e..9e93f34 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -365,6 +365,40 @@ static int amba_pm_restore_noirq(struct device *dev)
 
 #endif /* !CONFIG_HIBERNATE_CALLBACKS */
 
+#ifdef CONFIG_PM_RUNTIME
+/*
+ * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
+ * enable/disable the bus clock at runtime PM suspend/resume as this
+ * does not result in loss of context.  However, disabling vcore power
+ * would do, so we leave that to the driver.
+ */
+static int amba_pm_runtime_suspend(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	int ret = pm_generic_runtime_suspend(dev);
+
+	if (ret == 0 && dev->driver)
+		clk_disable(pcdev->pclk);
+
+	return ret;
+}
+
+static int amba_pm_runtime_resume(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	int ret;
+
+	if (dev->driver) {
+		ret = clk_enable(pcdev->pclk);
+		/* Failure is probably fatal to the system, but... */
+		if (ret)
+			return ret;
+	}
+
+	return pm_generic_runtime_resume(dev);
+}
+#endif
+
 #ifdef CONFIG_PM
 
 static const struct dev_pm_ops amba_pm = {
@@ -383,8 +417,8 @@ static const struct dev_pm_ops amba_pm = {
 	.poweroff_noirq	= amba_pm_poweroff_noirq,
 	.restore_noirq	= amba_pm_restore_noirq,
 	SET_RUNTIME_PM_OPS(
-		pm_generic_runtime_suspend,
-		pm_generic_runtime_resume,
+		amba_pm_runtime_suspend,
+		amba_pm_runtime_resume,
 		pm_generic_runtime_idle
 	)
 };

[PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests

[Linus Walleij]

2011/7/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sat, Jul 30, 2011 at 01:07:40PM +0100, Russell King - ARM Linux wrote:
>> It may make better sense to convert this to runtime PM. ?I suspect
>> that there's core support which the amba/bus.c can do to help in that
>> respect (eg, managing the apb pclk itself) so that we don't have to
>> add the same code to every primecell driver.
>
> Something like this for the bus driver (untested):
>
> ?drivers/amba/bus.c | ? 38 ++++++++++++++++++++++++++++++++++++--
> ?1 files changed, 36 insertions(+), 2 deletions(-)

I think the pm_runtime_* code Rabin put in place inside
drivers/spi/spi-pl022.c would play really well with this approach, and
just work, so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

[PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests

[Linus Walleij]

2011/7/31 Linus Walleij <linus.walleij@linaro.org>:
> 2011/7/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> On Sat, Jul 30, 2011 at 01:07:40PM +0100, Russell King - ARM Linux wrote:
>>> It may make better sense to convert this to runtime PM. ?I suspect
>>> that there's core support which the amba/bus.c can do to help in that
>>> respect (eg, managing the apb pclk itself) so that we don't have to
>>> add the same code to every primecell driver.
>>
>> Something like this for the bus driver (untested):
>>
>> ?drivers/amba/bus.c | ? 38 ++++++++++++++++++++++++++++++++++++--
>> ?1 files changed, 36 insertions(+), 2 deletions(-)
>
> I think the pm_runtime_* code Rabin put in place inside
> drivers/spi/spi-pl022.c would play really well with this approach, and
> just work, so:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

..and while it will just cause some double refcounts on the clock,
it makes sense to delete the pclk manipulation from the PL022
driver code as part of the patch, like this:

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index eba88c7..4c4c02a 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -516,7 +516,6 @@ static void giveback(struct pl022 *pl022)
                msg->complete(msg->context);
        /* This message is completed, so let's turn off the clocks & power */
        clk_disable(pl022->clk);
-       amba_pclk_disable(pl022->adev);
        amba_vcore_disable(pl022->adev);
        pm_runtime_put(&pl022->adev->dev);
 }
@@ -1546,7 +1545,6 @@ static void pump_messages(struct work_struct *work)
         */
        pm_runtime_get_sync(&pl022->adev->dev);
        amba_vcore_enable(pl022->adev);
-       amba_pclk_enable(pl022->adev);
        clk_enable(pl022->clk);
        restore_state(pl022);
        flush(pl022);
@@ -2236,10 +2234,10 @@ pl022_probe(struct amba_device *adev, const
struct amba_id *id)
        }
        dev_dbg(dev, "probe succeeded\n");
        /*
-        * Disable the silicon block pclk and any voltage domain and just
-        * power it up and clock it when it's needed
+        * Disable the silicon block voltage domain and just
+        * power it up when it's needed. The AMBA core code will
+        * enable the pclk.
         */
-       amba_pclk_disable(adev);
        amba_vcore_disable(adev);
        return 0;

Yours,
Linus Walleij

[PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests

[Russell King - ARM Linux]

On Sun, Jul 31, 2011 at 02:04:47AM +0200, Linus Walleij wrote:
> 2011/7/31 Linus Walleij <linus.walleij@linaro.org>:
> > 2011/7/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> >> On Sat, Jul 30, 2011 at 01:07:40PM +0100, Russell King - ARM Linux wrote:
> >>> It may make better sense to convert this to runtime PM. ?I suspect
> >>> that there's core support which the amba/bus.c can do to help in that
> >>> respect (eg, managing the apb pclk itself) so that we don't have to
> >>> add the same code to every primecell driver.
> >>
> >> Something like this for the bus driver (untested):
> >>
> >> ?drivers/amba/bus.c | ? 38 ++++++++++++++++++++++++++++++++++++--
> >> ?1 files changed, 36 insertions(+), 2 deletions(-)
> >
> > I think the pm_runtime_* code Rabin put in place inside
> > drivers/spi/spi-pl022.c would play really well with this approach, and
> > just work, so:
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> ..and while it will just cause some double refcounts on the clock,
> it makes sense to delete the pclk manipulation from the PL022
> driver code as part of the patch, like this:

Yes, this looks fine.  Shall I wrap it up as part of my patch?

Two other things I've spotted in this driver are:

1. The remove function doesn't undo what the probe function did to
the pclk and vcore.  It needs to keep things balanced.  For a driver
which doesn't manage its pclk, this is what happens:
	- core gets pclk
	- core enables pclk
	- core calls driver's probe
		- driver sets stuff up
...
	- core calls driver's remove
		- driver tidies up
	- core disables pclk
	- core puts pclk

And PL022 does this:
	- core gets pclk
	- core enables pclk
	- core calls driver's probe
		- driver sets stuff up
		- driver disables pclk
...
	- core calls driver's remove
		- driver tidies up
	- core disables pclk
	- core puts pclk

Notice the double-disable of pclk in that sequence.  If ->probe disables
pclk, ->remove needs to return with that disable balanced with an enable.

2. It thinks it can refuse 'remove' by returning an error code.  This
is false.  removes can't be aborted - here's the code from drivers/base/dd.c:

static void __device_release_driver(struct device *dev)
{
...
                if (dev->bus && dev->bus->remove)
                        dev->bus->remove(dev);
                else if (drv->remove)
                        drv->remove(dev);
...
}

Notice how return codes go nowhere.  remove should _really_ be a void
function to stop people thinking that it can be aborted.  It can't.

[PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests

[Linus Walleij]

On Sun, Jul 31, 2011 at 7:04 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Jul 31, 2011 at 02:04:47AM +0200, Linus Walleij wrote:
>>
>> ..and while it will just cause some double refcounts on the clock,
>> it makes sense to delete the pclk manipulation from the PL022
>> driver code as part of the patch, like this:
>
> Yes, this looks fine. ?Shall I wrap it up as part of my patch?

Yes please.
Acked-by.

> Two other things I've spotted in this driver are:
>
> 1. The remove function doesn't undo what the probe function did to
> the pclk and vcore. ?It needs to keep things balanced. ?For a driver
> which doesn't manage its pclk, this is what happens:
> ? ? ? ?- core gets pclk
> ? ? ? ?- core enables pclk
> ? ? ? ?- core calls driver's probe
> ? ? ? ? ? ? ? ?- driver sets stuff up
> ...
> ? ? ? ?- core calls driver's remove
> ? ? ? ? ? ? ? ?- driver tidies up
> ? ? ? ?- core disables pclk
> ? ? ? ?- core puts pclk
>
> And PL022 does this:
> ? ? ? ?- core gets pclk
> ? ? ? ?- core enables pclk
> ? ? ? ?- core calls driver's probe
> ? ? ? ? ? ? ? ?- driver sets stuff up
> ? ? ? ? ? ? ? ?- driver disables pclk
> ...
> ? ? ? ?- core calls driver's remove
> ? ? ? ? ? ? ? ?- driver tidies up
> ? ? ? ?- core disables pclk
> ? ? ? ?- core puts pclk
>
> Notice the double-disable of pclk in that sequence. ?If ->probe disables
> pclk, ->remove needs to return with that disable balanced with an enable.

Ah yes. Can you fix this as part of the patch since it's pretty related?
Much appreciated if you do.

> 2. It thinks it can refuse 'remove' by returning an error code. ?This
> is false. ?removes can't be aborted - here's the code from drivers/base/dd.c:
>
> static void __device_release_driver(struct device *dev)
> {
> ...
> ? ? ? ? ? ? ? ?if (dev->bus && dev->bus->remove)
> ? ? ? ? ? ? ? ? ? ? ? ?dev->bus->remove(dev);
> ? ? ? ? ? ? ? ?else if (drv->remove)
> ? ? ? ? ? ? ? ? ? ? ? ?drv->remove(dev);
> ...
> }
>
> Notice how return codes go nowhere. ?remove should _really_ be a void
> function to stop people thinking that it can be aborted. ?It can't.

Correct, thanks for spotting this. I'll send a separate patch to Grant
fixing this up.

Yours,
Linus Walleij

[PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests

[Russell King - ARM Linux]

On Tue, Aug 02, 2011 at 11:05:38AM +0200, Linus Walleij wrote:
> On Sun, Jul 31, 2011 at 7:04 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Jul 31, 2011 at 02:04:47AM +0200, Linus Walleij wrote:
> >>
> >> ..and while it will just cause some double refcounts on the clock,
> >> it makes sense to delete the pclk manipulation from the PL022
> >> driver code as part of the patch, like this:
> >
> > Yes, this looks fine. ?Shall I wrap it up as part of my patch?
> 
> Yes please.
> Acked-by.

Can you give this a try - this adds runtime PM to the primecell core,
updates the SPI driver for those changes, and adds runtime PM to the
MMCI driver.  I've briefly tested this on Versatile PB926, though
obviously not the SPI stuff very much because Versatile has no SPI
peripherals.

I chose the PCI methodology to this - rather than having every driver
fiddle about with enabling runtime PM, that's dealt with in the core
and instead, drivers just do a put() in their probe and a balancing
get() in their remove function to activate runtime PM for the device.

Thanks.

 drivers/amba/bus.c      |   57 +++++++++++++++++++++++++++++++--
 drivers/mmc/host/mmci.c |   12 +++++++
 drivers/spi/spi-pl022.c |   80 ++++++++++++++++++++++++++++-------------------
 3 files changed, 114 insertions(+), 35 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index d74926e..84bdaac 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -365,6 +365,40 @@ static int amba_pm_restore_noirq(struct device *dev)
 
 #endif /* !CONFIG_HIBERNATE_CALLBACKS */
 
+#ifdef CONFIG_PM_RUNTIME
+/*
+ * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
+ * enable/disable the bus clock at runtime PM suspend/resume as this
+ * does not result in loss of context.  However, disabling vcore power
+ * would do, so we leave that to the driver.
+ */
+static int amba_pm_runtime_suspend(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	int ret = pm_generic_runtime_suspend(dev);
+
+	if (ret == 0 && dev->driver)
+		clk_disable(pcdev->pclk);
+
+	return ret;
+}
+
+static int amba_pm_runtime_resume(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	int ret;
+
+	if (dev->driver) {
+		ret = clk_enable(pcdev->pclk);
+		/* Failure is probably fatal to the system, but... */
+		if (ret)
+			return ret;
+	}
+
+	return pm_generic_runtime_resume(dev);
+}
+#endif
+
 #ifdef CONFIG_PM
 
 static const struct dev_pm_ops amba_pm = {
@@ -383,8 +417,8 @@ static const struct dev_pm_ops amba_pm = {
 	.poweroff_noirq	= amba_pm_poweroff_noirq,
 	.restore_noirq	= amba_pm_restore_noirq,
 	SET_RUNTIME_PM_OPS(
-		pm_generic_runtime_suspend,
-		pm_generic_runtime_resume,
+		amba_pm_runtime_suspend,
+		amba_pm_runtime_resume,
 		pm_generic_runtime_idle
 	)
 };
@@ -494,10 +528,18 @@ static int amba_probe(struct device *dev)
 		if (ret)
 			break;
 
+		pm_runtime_get_noresume(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+
 		ret = pcdrv->probe(pcdev, id);
 		if (ret == 0)
 			break;
 
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_put_noidle(dev);
+
 		amba_put_disable_pclk(pcdev);
 		amba_put_disable_vcore(pcdev);
 	} while (0);
@@ -509,7 +551,16 @@ static int amba_remove(struct device *dev)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *drv = to_amba_driver(dev->driver);
-	int ret = drv->remove(pcdev);
+	int ret;
+
+	pm_runtime_get_sync(dev);
+	ret = drv->remove(pcdev);
+	pm_runtime_put_noidle(dev);
+
+	/* Undo the runtime PM settings in amba_probe() */
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_put_noidle(dev);
 
 	amba_put_disable_pclk(pcdev);
 	amba_put_disable_vcore(pcdev);
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 56e9a41..5e142b7 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -29,6 +29,7 @@
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
 #include <linux/amba/mmci.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/div64.h>
 #include <asm/io.h>
@@ -170,6 +171,7 @@ mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 	 * back into the driver...
 	 */
 	spin_unlock(&host->lock);
+	pm_runtime_put(mmc_dev(host->mmc));
 	mmc_request_done(host->mmc, mrq);
 	spin_lock(&host->lock);
 }
@@ -984,6 +986,8 @@ static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		return;
 	}
 
+	pm_runtime_get_sync(mmc_dev(mmc));
+
 	spin_lock_irqsave(&host->lock, flags);
 
 	host->mrq = mrq;
@@ -1327,6 +1331,8 @@ static int __devinit mmci_probe(struct amba_device *dev,
 
 	mmci_dma_setup(host);
 
+	pm_runtime_put(&dev->dev);
+
 	mmc_add_host(mmc);
 
 	return 0;
@@ -1364,6 +1370,12 @@ static int __devexit mmci_remove(struct amba_device *dev)
 	if (mmc) {
 		struct mmci_host *host = mmc_priv(mmc);
 
+		/*
+		 * Undo pm_runtime_put() in probe.  We use the _sync
+		 * version here so that we can access the primecell.
+		 */
+		pm_runtime_get_sync(&dev->dev);
+
 		mmc_remove_host(mmc);
 
 		writel(0, host->base + MMCIMASK0);
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index eba88c7..9f1e927 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -515,9 +515,6 @@ static void giveback(struct pl022 *pl022)
 	if (msg->complete)
 		msg->complete(msg->context);
 	/* This message is completed, so let's turn off the clocks & power */
-	clk_disable(pl022->clk);
-	amba_pclk_disable(pl022->adev);
-	amba_vcore_disable(pl022->adev);
 	pm_runtime_put(&pl022->adev->dev);
 }
 
@@ -1545,9 +1542,6 @@ static void pump_messages(struct work_struct *work)
 	 * (poll/interrupt/DMA)
 	 */
 	pm_runtime_get_sync(&pl022->adev->dev);
-	amba_vcore_enable(pl022->adev);
-	amba_pclk_enable(pl022->adev);
-	clk_enable(pl022->clk);
 	restore_state(pl022);
 	flush(pl022);
 
@@ -2186,8 +2180,6 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	}
 	printk(KERN_INFO "pl022: mapped registers from 0x%08x to %p\n",
 	       adev->res.start, pl022->virtbase);
-	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
 
 	pl022->clk = clk_get(&adev->dev, NULL);
 	if (IS_ERR(pl022->clk)) {
@@ -2195,7 +2187,6 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 		dev_err(&adev->dev, "could not retrieve SSP/SPI bus clock\n");
 		goto err_no_clk;
 	}
-
 	/* Disable SSP */
 	writew((readw(SSP_CR1(pl022->virtbase)) & (~SSP_CR1_MASK_SSE)),
 	       SSP_CR1(pl022->virtbase));
@@ -2235,12 +2226,9 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 		goto err_spi_register;
 	}
 	dev_dbg(dev, "probe succeeded\n");
-	/*
-	 * Disable the silicon block pclk and any voltage domain and just
-	 * power it up and clock it when it's needed
-	 */
-	amba_pclk_disable(adev);
-	amba_vcore_disable(adev);
+
+	/* let runtime pm put suspend */
+	pm_runtime_put(dev);
 	return 0;
 
  err_spi_register:
@@ -2249,7 +2237,6 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	destroy_queue(pl022);
 	pl022_dma_remove(pl022);
 	free_irq(adev->irq[0], pl022);
-	pm_runtime_disable(&adev->dev);
  err_no_irq:
 	clk_put(pl022->clk);
  err_no_clk:
@@ -2271,6 +2258,12 @@ pl022_remove(struct amba_device *adev)
 	if (!pl022)
 		return 0;
 
+	/*
+	 * undo pm_runtime_put() in probe.  I assume that we're not
+	 * accessing the primecell here.
+	 */
+	pm_runtime_get_noresume(&adev->dev);
+
 	/* Remove the queue */
 	status = destroy_queue(pl022);
 	if (status != 0) {
@@ -2293,10 +2286,10 @@ pl022_remove(struct amba_device *adev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int pl022_suspend(struct amba_device *adev, pm_message_t state)
+#ifdef CONFIG_SUSPEND
+static int pl011_suspend(struct device *dev)
 {
-	struct pl022 *pl022 = amba_get_drvdata(adev);
+	struct pl022 *pl022 = dev_get_drvdata(dev);
 	int status = 0;
 
 	status = stop_queue(pl022);
@@ -2305,34 +2298,58 @@ static int pl022_suspend(struct amba_device *adev, pm_message_t state)
 		return status;
 	}
 
-	amba_vcore_enable(adev);
-	amba_pclk_enable(adev);
+	amba_vcore_enable(pl022->adev);
+	amba_pclk_enable(pl022->adev);
 	load_ssp_default_config(pl022);
-	amba_pclk_disable(adev);
-	amba_vcore_disable(adev);
+	amba_pclk_disable(pl022->adev);
+	amba_vcore_disable(pl022->adev);
 	dev_dbg(&adev->dev, "suspended\n");
 	return 0;
 }
 
-static int pl022_resume(struct amba_device *adev)
+static int pl022_resume(struct device *dev)
 {
-	struct pl022 *pl022 = amba_get_drvdata(adev);
+	struct pl022 *pl022 = dev_get_drvdata(dev);
 	int status = 0;
 
 	/* Start the queue running */
 	status = start_queue(pl022);
 	if (status)
-		dev_err(&adev->dev, "problem starting queue (%d)\n", status);
+		dev_err(dev, "problem starting queue (%d)\n", status);
 	else
-		dev_dbg(&adev->dev, "resumed\n");
+		dev_dbg(dev, "resumed\n");
 
 	return status;
 }
-#else
-#define pl022_suspend NULL
-#define pl022_resume NULL
 #endif	/* CONFIG_PM */
 
+#ifdef CONFIG_PM_RUNTIME
+static int pl022_runtime_suspend(struct device *dev)
+{
+	struct pl022 *pl022 = dev_get_drvdata(dev);
+
+	clk_disable(pl022->clk);
+	amba_vcore_disable(pl022->adev);
+
+	return 0;
+}
+
+static int pl022_runtime_resume(struct device *dev)
+{
+	struct pl022 *pl022 = dev_get_drvdata(dev);
+
+	amba_vcore_enable(pl022->adev);
+	clk_enable(pl022->clk);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops pl022_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
+	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
+};
+
 static struct vendor_data vendor_arm = {
 	.fifodepth = 8,
 	.max_bpw = 16,
@@ -2412,12 +2429,11 @@ static struct amba_id pl022_ids[] = {
 static struct amba_driver pl022_driver = {
 	.drv = {
 		.name	= "ssp-pl022",
+		.pm	= &pl022_dev_pm_ops,
 	},
 	.id_table	= pl022_ids,
 	.probe		= pl022_probe,
 	.remove		= __devexit_p(pl022_remove),
-	.suspend        = pl022_suspend,
-	.resume         = pl022_resume,
 };
 
 

[PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with channel requests

[Linus Walleij]

On Wed, Aug 3, 2011 at 2:55 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Aug 02, 2011 at 11:05:38AM +0200, Linus Walleij wrote:
>> On Sun, Jul 31, 2011 at 7:04 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Sun, Jul 31, 2011 at 02:04:47AM +0200, Linus Walleij wrote:
>> >>
>> >> ..and while it will just cause some double refcounts on the clock,
>> >> it makes sense to delete the pclk manipulation from the PL022
>> >> driver code as part of the patch, like this:
>> >
>> > Yes, this looks fine. ?Shall I wrap it up as part of my patch?
>>
>> Yes please.
>> Acked-by.
>
> Can you give this a try - this adds runtime PM to the primecell core,
> updates the SPI driver for those changes, and adds runtime PM to the
> MMCI driver. ?I've briefly tested this on Versatile PB926, though
> obviously not the SPI stuff very much because Versatile has no SPI
> peripherals.
>
> I chose the PCI methodology to this - rather than having every driver
> fiddle about with enabling runtime PM, that's dealt with in the core
> and instead, drivers just do a put() in their probe and a balancing
> get() in their remove function to activate runtime PM for the device.

Sorry for the delay.

It works like a charm. I've tested it on U300 with a loopback test
and MMC and for both it works perfectly and the reference
counter goes to zero when unused.

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

Yours,
Linus Walleij

[PATCH 08/18] 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>
---
 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 f1a25cf..c4ce1a2 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -923,7 +923,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 08/18] dmaengine/amba-pl08x: No need to check "ch->signal < 0"

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> 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>

Thanks,
Linus Walleij

[PATCH 09/18] 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 occured on which channels.

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

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index c4ce1a2..b9137bc 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1630,40 +1630,33 @@ 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 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);
-	for (i = 0; i < pl08x->vd->channels; i++) {
-		if ((1 << i) & val) {
+	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) & err) || ((1 << i) & tc)) {
 			/* Locate physical channel */
 			struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i];
 			struct pl08x_dma_chan *plchan = phychan->serving;
 
 			/* Schedule tasklet on this channel */
 			tasklet_schedule(&plchan->tasklet);
-
-			mask |= (1 << i);
 		}
-	}
-	/* Clear only the terminal interrupts on channels we processed */
-	writel(mask, pl08x->base + PL080_TC_CLEAR);
 
-	return mask ? IRQ_HANDLED : IRQ_NONE;
+	return IRQ_HANDLED;
 }
 
 static void pl08x_dma_slave_init(struct pl08x_dma_chan *chan)
-- 
1.7.2.2

[PATCH 09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[Koul, Vinod]

On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> 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 occured on which channels.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/dma/amba-pl08x.c |   43 ++++++++++++++++++-------------------------
>  1 files changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index c4ce1a2..b9137bc 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -1630,40 +1630,33 @@ 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 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);
again this looks convoluted, and the stuff is quotes can be in single
line :)
> +		writel(err, pl08x->base + PL080_ERR_CLEAR);
>  	}
> -	val = readl(pl08x->base + PL080_INT_STATUS);
> -	for (i = 0; i < pl08x->vd->channels; i++) {
> -		if ((1 << i) & val) {
> +	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) & err) || ((1 << i) & tc)) {
>  			/* Locate physical channel */
>  			struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i];
>  			struct pl08x_dma_chan *plchan = phychan->serving;
>  
>  			/* Schedule tasklet on this channel */
>  			tasklet_schedule(&plchan->tasklet);
but in tasklet you will call the client callback, so how does the client
know if this was success or error? Did I miss anything?
> -
> -			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;
> +	return IRQ_HANDLED;
>  }
>  
>  static void pl08x_dma_slave_init(struct pl08x_dma_chan *chan)


-- 
~Vinod

[PATCH 09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 05:46:39PM +0530, Koul, Vinod wrote:
> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> > +	/* 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);
> again this looks convoluted, and the stuff is quotes can be in single
> line :)

And it shows the danger of wrapping quoted strings.  This will print
the following message:

...: pl08x_irq error interrupt, register value0xNNNNNNNN

So it may want a space between 'value' and '0x%08x' - and it may want a
':' after '%s' too.

[PATCH 09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[viresh kumar]

On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
>> +		dev_err(&pl08x->adev->dev, "%s error interrupt, register value"
>> +				"0x%08x\n", __func__, err);
> again this looks convoluted, and the stuff is quotes can be in single
> line :)

Will take care of this in all patches.

> but in tasklet you will call the client callback, so how does the client
> know if this was success or error? Did I miss anything?

No you didn't. I couldn't find a way in framework to report back to
client success or failure.
Is there any way? For now i have only kept error prints.

--
viresh

[PATCH 09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[Koul, Vinod]

On Fri, 2011-07-29 at 08:30 -0700, viresh kumar wrote:
> On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> > On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> >> +		dev_err(&pl08x->adev->dev, "%s error interrupt, register value"
> >> +				"0x%08x\n", __func__, err);
> > again this looks convoluted, and the stuff is quotes can be in single
> > line :)
> 
> Will take care of this in all patches.
> 
> > but in tasklet you will call the client callback, so how does the client
> > know if this was success or error? Did I miss anything?
> 
> No you didn't. I couldn't find a way in framework to report back to
> client success or failure.
> Is there any way? For now i have only kept error prints.
Sorry for not responding earlier.

Yes that's the whole point, today callback mechanism doesn't tell the
_status_ of the transfer (which if we need change can be discussed as
well), but to counter argue I have never been able to generate the error
interrupt, are you able to do on your controller?

One more point wrt this patch, what do we gain here from calling
tasklet, are you cleaning up your queue for the error descriptor or
something, I think not. so just calling to print doesn't make sense to
me, comments?


-- 
~Vinod

[PATCH 09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[viresh kumar]

On 08/04/2011 09:55 AM, Koul, Vinod wrote:
> Yes that's the whole point, today callback mechanism doesn't tell the
> _status_ of the transfer (which if we need change can be discussed as
> well), but to counter argue I have never been able to generate the error
> interrupt, are you able to do on your controller?

Yes, if we supply unaligned address, with access width as word, then it gives
error interrupt.

Regarding your point of updating callback for reporting errors,
I think it is required and should be a common issue.

> 
> One more point wrt this patch, what do we gain here from calling
> tasklet, are you cleaning up your queue for the error descriptor or
> something, I think not. so just calling to print doesn't make sense to
> me, comments?

Tasklet does following:
- If any other transfer request is queued up, then it is started
- else, it releases physical channel, by clearing it completely.

Finally it unmap buffers, frees txd, and call callback.

This probably is enough to end transfer for which user was waiting.
If user hasn't used any timeout mechanisms then he will never come
to know that an error occurred. So its better to call its callback,
to tell him, transfer has completed, successfully or unsuccessfully.

-- 
viresh

[PATCH 09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[Koul, Vinod]

On Thu, 2011-08-04 at 11:24 +0530, viresh kumar wrote:
> On 08/04/2011 09:55 AM, Koul, Vinod wrote:
> > Yes that's the whole point, today callback mechanism doesn't tell the
> > _status_ of the transfer (which if we need change can be discussed as
> > well), but to counter argue I have never been able to generate the error
> > interrupt, are you able to do on your controller?
> 
> Yes, if we supply unaligned address, with access width as word, then it gives
> error interrupt.
> 
> Regarding your point of updating callback for reporting errors,
> I think it is required and should be a common issue.
Okay till now no one has asked for it. We can add to callback arguments
either a bool to indicate what is the transfer status or usual error
code to also indicate what type of failure.
I would prefer latter, if there is consensus on this, we can go thru
with this approach, Dan your comments?

-- 
~Vinod

[PATCH 09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[Russell King - ARM Linux]

On Thu, Aug 04, 2011 at 11:02:42AM +0530, Koul, Vinod wrote:
> On Thu, 2011-08-04 at 11:24 +0530, viresh kumar wrote:
> > On 08/04/2011 09:55 AM, Koul, Vinod wrote:
> > > Yes that's the whole point, today callback mechanism doesn't tell the
> > > _status_ of the transfer (which if we need change can be discussed as
> > > well), but to counter argue I have never been able to generate the error
> > > interrupt, are you able to do on your controller?
> > 
> > Yes, if we supply unaligned address, with access width as word, then it gives
> > error interrupt.
> > 
> > Regarding your point of updating callback for reporting errors,
> > I think it is required and should be a common issue.
> Okay till now no one has asked for it. We can add to callback arguments
> either a bool to indicate what is the transfer status or usual error
> code to also indicate what type of failure.
> I would prefer latter, if there is consensus on this, we can go thru
> with this approach, Dan your comments?

If we are concerned about unaligned addresses, then that's something which
should be checked for at prepare time, to avoid failing DMA transfers.  A
failed DMA transfer is potentially a data loss situation, one that we want
to avoid.

Consider for instance a UART driver using DMA for TX/RX.  We want to know
when we prepare the RX DMA whether the DMA engine is going to be able to
perform the requested transfer.  We don't want to know when characters
start coming in, because not only would we have to undo the DMA setup,
but we'd also need to ensure that we start reading the characters via PIO
as quickly as possible to avoid overruns.

So, what I'd say is if you receive an error interrupt, either the prep
code isn't robust enough - it's like a BUG() telling us that something
needs fixing.  So, I think printing a warning and stalling makes sense -
and hopefully we get a bug report to investigate the causes.

[PATCH 09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

(...)
> - ? ? ? u32 mask = 0;
(...)
> + ? ? ? if (!err && !tc)
> + ? ? ? ? ? ? ? return IRQ_NONE;
(...)
> -
> - ? ? ? ? ? ? ? ? ? ? ? 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;
> + ? ? ? return IRQ_HANDLED;

NAK.

These snippets illustrate what is not good about this patch, you're making
the driver fragile by removing checks for spurious IRQ.

So for example if there is an error or TC IRQ on a channel that is not
in use, what happens now?

It used to result in IRQ_NONE, now all of a sudden we start handling
spurious IRQs and claim IRQ_HANDLED with totally unpredictable
results, whereas they would previously gather error metrics for
spurious IRQs.

Yours,
Linus Walleij

[PATCH 09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt

[viresh kumar]

On 7/30/11, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/7/29 Viresh Kumar <viresh.kumar@st.com>:
> NAK.
>
> These snippets illustrate what is not good about this patch, you're making
> the driver fragile by removing checks for spurious IRQ.
>
> So for example if there is an error or TC IRQ on a channel that is not
> in use, what happens now?
>
> It used to result in IRQ_NONE, now all of a sudden we start handling
> spurious IRQs and claim IRQ_HANDLED with totally unpredictable
> results, whereas they would previously gather error metrics for
> spurious IRQs.
>

I got your point. Will resend this patch with better handling for
spurious interrupts.

--
viresh

[PATCH 10/18] 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 o f 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>
---
 drivers/dma/amba-pl08x.c   |  136 ++++---------------------------------------
 include/linux/amba/pl08x.h |    2 -
 2 files changed, 13 insertions(+), 125 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index b9137bc..04f7889 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
 
@@ -561,18 +553,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
@@ -679,121 +659,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;
-			}
+			tsize = lli_len / min(mbus->buswidth, sbus->buswidth);
+			lli_len	= tsize * min(mbus->buswidth, sbus->buswidth);
 
-			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,
+			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;
-			}
 
-			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++;
-				}
-			}
+			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 +697,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, "
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index 5509a50..98628a8 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 10/18] dmaengine/amba-pl08x: Get rid of pl08x_pre_boundary()

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 04:19:20PM +0530, Viresh Kumar wrote:
> 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 o f 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.

Good.  I'm glad someone's finally getting to the bottom of what's
actually required and what isn't...  This driver has been a mess for
quite a long time and is in desperate need of these kinds of cleanups.

[PATCH 10/18] dmaengine/amba-pl08x: Get rid of pl08x_pre_boundary()

[Koul, Vinod]

On Fri, 2011-07-29 at 12:05 +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 29, 2011 at 04:19:20PM +0530, Viresh Kumar wrote:
> > 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 o f 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.
> 
> Good.  I'm glad someone's finally getting to the bottom of what's
> actually required and what isn't...  This driver has been a mess for
> quite a long time and is in desperate need of these kinds of cleanups.
Typically in few dmac IPs i know of:
There is max limitation of what a burst length can be for dma, and what
a max transfer length can be.
The dma will push/pull data into peripheral FIFO based on
max_burst_length parameter (note length in dma terminology is always
items not bytes, and one item is usually defined as
transfer_length/src_width). For a transfer (single block or in
multi-block) the dmac will break the transaction into multiple bursts
and based on max_burst_length it will push/pull data to/from FIFO. Also
matching FIFO thresholds with max_burst_length is also important and can
be tuned based on perf/power requirements. 

Sharing my knowledge, hopefully it helps :)

-- 
~Vinod

[PATCH 10/18] dmaengine/amba-pl08x: Get rid of pl08x_pre_boundary()

[viresh kumar]

On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Fri, 2011-07-29 at 12:05 +0100, Russell King - ARM Linux wrote:
>> On Fri, Jul 29, 2011 at 04:19:20PM +0530, Viresh Kumar wrote:
> Typically in few dmac IPs i know of:
> There is max limitation of what a burst length can be for dma, and what
> a max transfer length can be.
> The dma will push/pull data into peripheral FIFO based on
> max_burst_length parameter (note length in dma terminology is always
> items not bytes, and one item is usually defined as
> transfer_length/src_width). For a transfer (single block or in
> multi-block) the dmac will break the transaction into multiple bursts
> and based on max_burst_length it will push/pull data to/from FIFO. Also
> matching FIFO thresholds with max_burst_length is also important and can
> be tuned based on perf/power requirements.
>
> Sharing my knowledge, hopefully it helps :)
>

Thanks. It was indeed helpful. :)

--
viresh

[PATCH 10/18] dmaengine/amba-pl08x: Get rid of pl08x_pre_boundary()

[Koul, Vinod]

On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> 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 o f 1KB. The
								^^^^^^
of

> 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>
> ---
>  drivers/dma/amba-pl08x.c   |  136 ++++---------------------------------------
>  include/linux/amba/pl08x.h |    2 -
>  2 files changed, 13 insertions(+), 125 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index b9137bc..04f7889 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
>  
> @@ -561,18 +553,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
> @@ -679,121 +659,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;
> -			}
> +			tsize = lli_len / min(mbus->buswidth, sbus->buswidth);
> +			lli_len	= tsize * min(mbus->buswidth, sbus->buswidth);
>  
> -			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,
> +			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;
> -			}
>  
> -			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++;
> -				}
> -			}
> +			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 +697,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, "
> diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
> index 5509a50..98628a8 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;
>  };
>  
>  /**


-- 
~Vinod

[PATCH 10/18] dmaengine/amba-pl08x: Get rid of pl08x_pre_boundary()

[viresh kumar]

On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
>> You can still issue bursts within the PL080 that are in excess o f 1KB.
>> The
> 								^^^^^^
> of

Ok.

[PATCH 10/18] dmaengine/amba-pl08x: Get rid of pl08x_pre_boundary()

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> 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>

OMG yes that makes perfect sense. I questioned this in the past and was
pointed to the same manual page.

What is fun is that the code was written by ARM.

Let's merge this and I'll test it on the PB11MPCore, I think Mem2Mem
with the DMA engine compile-in tests may work on a few other boards as
well so maybe Russell can test it too.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 11/18] 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>
---
 drivers/dma/amba-pl08x.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 04f7889..3b7357e 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -597,24 +597,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");
@@ -654,6 +647,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 11/18] dmaengine/amba-pl08x: max_bytes_per_lli is TRANSFER_SIZE * src_width (not MIN(width))

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

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

Great and THANKS for your tiresome research in ironing out the ambiguities
in the manuals.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 11/18] dmaengine/amba-pl08x: max_bytes_per_lli is TRANSFER_SIZE * src_width (not MIN(width))

[Russell King - ARM Linux]

On Sun, Jul 31, 2011 at 02:27:38AM +0200, Linus Walleij wrote:
> 2011/7/29 Viresh Kumar <viresh.kumar@st.com>:
> 
> > max_bytes_per_lli = bd.srcbus.buswidth * PL080_CONTROL_TRANSFER_SIZE_MASK;
> > This is confirmed by ARM support guys.
> 
> Great and THANKS for your tiresome research in ironing out the ambiguities
> in the manuals.

The manual isn't that bad.  The code from ARM though is, and raises lots
of "why are they doing that?  oh, they must know better than the docs"
type questions.  Obviously they don't!

[PATCH 12/18] 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 3b7357e..590397d 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -552,6 +552,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
@@ -563,7 +571,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;
 
@@ -612,29 +620,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
@@ -685,13 +691,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 "
+				"%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 12/18] dmaengine/amba-pl08x: Add prep_single_byte_llis() routine

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 04:19:22PM +0530, Viresh Kumar wrote:
> 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 3b7357e..590397d 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -552,6 +552,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;

Seeing as your patch series started with a cleanup of spacing/tabbing,
it's surprising to find a patch introducing new code which has broken
tabbing.

[PATCH 12/18] dmaengine/amba-pl08x: Add prep_single_byte_llis() routine

[viresh kumar]

On 07/29/2011 04:36 PM, Russell King - ARM Linux wrote:
>> > +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;
> Seeing as your patch series started with a cleanup of spacing/tabbing,
> it's surprising to find a patch introducing new code which has broken
> tabbing.
> 

Haah.... How can i do this? Sorry will fix this.

-- 
viresh

[PATCH 13/18] 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>
---
 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 590397d..44f317a 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -662,20 +662,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 13/18] dmaengine/amba-pl08x: Align lli_len to max(src.width, dst.width)

[Koul, Vinod]

On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> 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.
hmmm, not sure I follow you...
In previous patch you said

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

So why should lli_len be related to max of both widths?

-- 
~Vinod

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

[viresh kumar]

On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
>> 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.
> hmmm, not sure I follow you...
> In previous patch you said
>
> "max_bytes_per_lli = bd.srcbus.buswidth *
> PL080_CONTROL_TRANSFER_SIZE_MASK;
> This is confirmed by ARM support guys."
>
> So why should lli_len be related to max of both widths?
>

Sorry if my commit log wasn't clear enough.
Actually there is a field in ctrl reg of channel: transfer_size (it is
max 4095),

total data to be transferred = srcwidth * value programmed in transfer_size.

Now, suppose we have to transfer 98 bytes with src width = HALF WORD
and dest width = WORD

then value to be programmed in transfer_size is 98 / 2 = 49

Now on the destination side, transfers will be word by word and so
after 24 transfers on dest and 48 on src, 96 bytes would be
transferred.

But for the last two bytes, we can read two bytes from src, but can't
transfer them to dest. As its width was 4.

That's why we need to have lli_len to be related to max of both widths?

--
viresh

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

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> 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.

Again, a great and well researched patch.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 14/18] 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>
---
 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 44f317a..6b0ca26 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -492,34 +492,24 @@ struct pl08x_lli_build_data {
 /*
  * Autoselect a master bus to use for the transfer
  * - 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 14/18] dmaengine/amba-pl08x: Choose peripheral bus as master bus

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> 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>

Looks correct to me!
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

[PATCH 15/18] 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 fc field in struct pl08x_channel_data, which will be used only
for slave transfers and is not used in case of mem2mem transfers. In case this
is passed as 0, then flow controller is decided based on DMA_DIRECTION.

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

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 6b0ca26..9eea0d1 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -632,6 +632,38 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 	if (bd.remainder) {
 		/*
+		 * 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
+		 */
+		if (!bd.remainder) {
+			u32 fc = (txd->cctl & 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);
+			continue;
+		}
+
+		/*
 		 * Master now aligned
 		 * - if slave is not then we must set its width down
 		 */
@@ -1255,7 +1287,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,14 +1321,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;
@@ -1306,6 +1334,14 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
+	if (plchan->cd->fc)
+		tmp = plchan->cd->fc;
+	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 98628a8..febb0da 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -47,6 +47,10 @@ 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
+ * @fc: Flow Controller Settings for ccfg register. Only valid for slave
+ * channels. Fill with any PL080_FLOW_* macro's present in
+ * <asm/hardware/pl080.h>. If passed as 0, then Flow control will be decided
+ * according to DMA direction.
  * @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 +69,7 @@ struct pl08x_channel_data {
 	int max_signal;
 	u32 muxval;
 	u32 cctl;
+	u32 fc;
 	dma_addr_t addr;
 	bool circular_buffer;
 	bool single;
-- 
1.7.2.2

[PATCH 15/18] dmaengine/amba-pl08x: Pass flow controller information with slave channel data

[Russell King - ARM Linux]

Apart from my initial question in this thread...

On Fri, Jul 29, 2011 at 04:19:25PM +0530, Viresh Kumar wrote:
> +	if (plchan->cd->fc)
> +		tmp = plchan->cd->fc;
> +	else
> +		tmp = (direction == DMA_TO_DEVICE) ? PL080_FLOW_MEM2PER :
> +			PL080_FLOW_PER2MEM;
> +
> +	txd->ccfg |= tmp << PL080_CONFIG_FLOW_CONTROL_SHIFT;
> +

Please, no, don't go back to encoding the flow mask into the platform
data.  Instead use 'fc' as an indication of who should be the flow
controller, and hav ethis select the flow control based on both
'fc' and the direction.

[PATCH 15/18] dmaengine/amba-pl08x: Pass flow controller information with slave channel data

[viresh kumar]

On 07/29/2011 04:40 PM, Russell King - ARM Linux wrote:
> Please, no, don't go back to encoding the flow mask into the platform
> data.  Instead use 'fc' as an indication of who should be the flow
> controller, and hav ethis select the flow control based on both
> 'fc' and the direction.

Ok.

-- 
viresh

[PATCH 15/18] dmaengine/amba-pl08x: Pass flow controller information with slave channel data

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

I have the same comment as Russell basically:

> @@ -65,6 +69,7 @@ struct pl08x_channel_data {
> ? ? ? ?int max_signal;
> ? ? ? ?u32 muxval;
> ? ? ? ?u32 cctl;
> + ? ? ? u32 fc;

bool device_fc;

or similar. Then encode the necessary bits if the device controls
flow, and fall back to default (DMAC control) otherwise.

> ? ? ? ?dma_addr_t addr;
> ? ? ? ?bool circular_buffer;
> ? ? ? ?bool single;
> --
> 1.7.2.2

Thanks,
Linus Walleij

[PATCH 15/18] dmaengine/amba-pl08x: Pass flow controller information with slave channel data

[viresh kumar]

On 7/30/11, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/7/29 Viresh Kumar <viresh.kumar@st.com>:
>
> I have the same comment as Russell basically:
>
>> @@ -65,6 +69,7 @@ struct pl08x_channel_data {
>>        int max_signal;
>>        u32 muxval;
>>        u32 cctl;
>> +       u32 fc;
>
> bool device_fc;
>
> or similar. Then encode the necessary bits if the device controls
> flow, and fall back to default (DMAC control) otherwise.
>

Yes. I thought of exactly the same code.

--
viresh

[PATCH 16/18] 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.

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

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 9eea0d1..ddc06bd 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -354,7 +354,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;
 		}
 	}
 
@@ -564,6 +566,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) {
@@ -573,13 +576,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 =
@@ -591,46 +590,27 @@ 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");
-
-	/*
-	 * 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;
-	}
+		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");
 
-	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) {
 		/*
 		 * Zero length is only allowed if all these requirements are
 		 * met:
@@ -664,79 +644,111 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		}
 
 		/*
-		 * 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,
+				/*
+				 * 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 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,
+				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;
@@ -769,11 +781,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);
 }
 
@@ -1228,6 +1247,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 |
@@ -1246,6 +1266,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);
@@ -1255,10 +1276,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;
@@ -1287,17 +1317,11 @@ 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);
 
@@ -1318,17 +1342,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;
@@ -1342,6 +1364,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;
@@ -1439,22 +1481,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 febb0da..9090342 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -107,12 +107,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
@@ -122,10 +134,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 16/18] dmaengine/amba-pl08x: Add support for sg len greater than one for slave transfers

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> Untill now, sg_len greater than one is not supported. This patch adds support to
> do that.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>

Thanks, was on my TODO once.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 04:19:26PM +0530, Viresh Kumar wrote:
> Untill now, sg_len greater than one is not supported. This patch adds support to
> do that.

I'm not sure that this is the correct approach.  memcpy()s can only be
used with one single buffer, so the sg stuff for that (and the unmapping
support) doesn't make sense at all.

The only place where this makes sense is the slave sg stuff.  I wonder
whether we can better deal with that by having the LLI setup code deal
with one SG list entry at a time, and chain each together.

Something I've also been pondering which is related to this is linking
together DMA operations using the LLI chaining when interrupts and
callbacks aren't required.  It's a very similar problem.  Other DMA
engine drivers do this.

Finally, note that some of the PL08x code assumes that for any TXD,
the LLI list is a contiguous array.  See the first part of
pl08x_getbytes_chan(). That needs fixing as soon as we start going to
more than one SG list entry.

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

[Linus Walleij]

On Sun, Aug 14, 2011 at 10:36 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jul 29, 2011 at 04:19:26PM +0530, Viresh Kumar wrote:
>> Untill now, sg_len greater than one is not supported. This patch adds support to
>> do that.
>
> I'm not sure that this is the correct approach. ?memcpy()s can only be
> used with one single buffer, so the sg stuff for that (and the unmapping
> support) doesn't make sense at all.

I agree, that part is fishy.

> The only place where this makes sense is the slave sg stuff. ?I wonder
> whether we can better deal with that by having the LLI setup code deal
> with one SG list entry at a time, and chain each together.
>
> Something I've also been pondering which is related to this is linking
> together DMA operations using the LLI chaining when interrupts and
> callbacks aren't required. ?It's a very similar problem. ?Other DMA
> engine drivers do this.

Yeah. Viresh can you hold this patch off for the moment, move it to
the end of the series and see if we can find some more optimal
approach for this?

> Finally, note that some of the PL08x code assumes that for any TXD,
> the LLI list is a contiguous array. ?See the first part of
> pl08x_getbytes_chan(). That needs fixing as soon as we start going to
> more than one SG list entry.

Actually, after the removal of the phantom boundary in the PL08x
driver, the LLI handling code starts to resemble the stuff inside
the coh901318.c driver an *awful lot*, so I'm now ever more
convinced that that thing is a PL08x derivate. (I have asked the
hardware designers but the people who made it are unreachable.)

And the coh901318 code handles this by properly walking the LLIs
in memory IIRC, so could be a good inspiration.

I am aware that we need to consolidate that LLI handling so
the coh901318 and PL08x share the same LLI code atleast, or I
may even be able to merge all into pl08x.c. I have a mental note
to get to that once we've got PL08x into shape, until then it
can atleast be used for inspiration.

Thanks,
Linus Walleij

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

[Viresh Kumar]

Hi Russell & Linus,

Sorry for being late to reply.

On 8/14/2011 2:06 PM, Russell King - ARM Linux wrote:
> On Fri, Jul 29, 2011 at 04:19:26PM +0530, Viresh Kumar wrote:
>> Untill now, sg_len greater than one is not supported. This patch adds support to
>> do that.
> 
> I'm not sure that this is the correct approach.  memcpy()s can only be
> used with one single buffer, so the sg stuff for that (and the unmapping
> support) doesn't make sense at all.
> 

I am not sure if i get this completely. In memcpy, we still don't support
more than one sg. We have created a new member in txd, which keeps track of
data (addresses, len).

While unmapping, we are unmapping only single dsg. I think i don't need to
use list_for_each_entry() there, as there is only one element present in list.
But can still keep that.

> The only place where this makes sense is the slave sg stuff.  I wonder
> whether we can better deal with that by having the LLI setup code deal
> with one SG list entry at a time, and chain each together.
> 
> Something I've also been pondering which is related to this is linking
> together DMA operations using the LLI chaining when interrupts and
> callbacks aren't required.  It's a very similar problem.  Other DMA
> engine drivers do this.
> 

Are you talking about linking all sg's together or linking multiple calls to
prep_dma_memcpy? I am handling the first.

> Finally, note that some of the PL08x code assumes that for any TXD,
> the LLI list is a contiguous array.  See the first part of
> pl08x_getbytes_chan(). That needs fixing as soon as we start going to
> more than one SG list entry.

We still have one contiguous array for LLI list. In pl08x_fill_llis_for_desc()
i am using same txd->llis_va for all sg's.

Sorry if i misunderstood your concerns.

-- 
viresh

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

[Russell King - ARM Linux]

On Thu, Aug 18, 2011 at 02:08:19PM +0530, Viresh Kumar wrote:
> On 8/14/2011 2:06 PM, Russell King - ARM Linux wrote:
> > On Fri, Jul 29, 2011 at 04:19:26PM +0530, Viresh Kumar wrote:
> >> Untill now, sg_len greater than one is not supported. This patch adds support to
> >> do that.
> > 
> > I'm not sure that this is the correct approach.  memcpy()s can only be
> > used with one single buffer, so the sg stuff for that (and the unmapping
> > support) doesn't make sense at all.
> > 
> 
> I am not sure if i get this completely. In memcpy, we still don't support
> more than one sg. We have created a new member in txd, which keeps track of
> data (addresses, len).

Yes, but we shouldn't need to translate it into any kind of scatterlist.

> While unmapping, we are unmapping only single dsg. I think i don't need to
> use list_for_each_entry() there, as there is only one element present in list.
> But can still keep that.

Correct.

> > The only place where this makes sense is the slave sg stuff.  I wonder
> > whether we can better deal with that by having the LLI setup code deal
> > with one SG list entry at a time, and chain each together.
> > 
> > Something I've also been pondering which is related to this is linking
> > together DMA operations using the LLI chaining when interrupts and
> > callbacks aren't required.  It's a very similar problem.  Other DMA
> > engine drivers do this.
> 
> Are you talking about linking all sg's together or linking multiple calls to
> prep_dma_memcpy? I am handling the first.

I'm talking about letting the hardware process as many txds as possible
without calling back into the driver to have it setup the next txd.

> > Finally, note that some of the PL08x code assumes that for any TXD,
> > the LLI list is a contiguous array.  See the first part of
> > pl08x_getbytes_chan(). That needs fixing as soon as we start going to
> > more than one SG list entry.
> 
> We still have one contiguous array for LLI list. In pl08x_fill_llis_for_desc()
> i am using same txd->llis_va for all sg's.

That's ok then.

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

[Viresh Kumar]

On 8/21/2011 2:03 PM, Russell King - ARM Linux wrote:
>> > I am not sure if i get this completely. In memcpy, we still don't support
>> > more than one sg. We have created a new member in txd, which keeps track of
>> > data (addresses, len).
> Yes, but we shouldn't need to translate it into any kind of scatterlist.
> 

Ok. I way out is keeping two separate variables in txd, list for slave transfers,
and pointer to single element for memcpy. And that looks to be even bad to me.
Why waste memory, for second variable. Or create union of both.

One more thing, we can actually have scatter gather in memcpy too in future. This will be
helpful then also. But surely that's something not implemented currently.

-- 
viresh

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

[Koul, Vinod]

On Tue, 2011-08-23 at 09:52 +0530, Viresh Kumar wrote:
> On 8/21/2011 2:03 PM, Russell King - ARM Linux wrote:
> >> > I am not sure if i get this completely. In memcpy, we still don't support
> >> > more than one sg. We have created a new member in txd, which keeps track of
> >> > data (addresses, len).
> > Yes, but we shouldn't need to translate it into any kind of scatterlist.
> > 
> 
> Ok. I way out is keeping two separate variables in txd, list for slave transfers,
> and pointer to single element for memcpy. And that looks to be even bad to me.
> Why waste memory, for second variable. Or create union of both.
> 
> One more thing, we can actually have scatter gather in memcpy too in future. This will be
> helpful then also. But surely that's something not implemented currently.
> 
There are few points for consideration:
1) Clients will calls tx_submit() and submit the descriptors, this
should push your descriptor to queued list
2) On issue_pending() you need to push the queued descriptors to dmac.
If your hardware can transfer multiple descriptors, then you should use
this and push all available descriptors to hardware.

This way you ensure optimal performance and give LLI for memcpy even
when you think API is not available.

If there are some use cases where we need scatterlist support for
memcpy, and if above doesn't satisfy, should be okay to add it.


-- 
~Vinod

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

[Viresh Kumar]

On 8/23/2011 9:52 AM, Viresh Kumar wrote:
> On 8/21/2011 2:03 PM, Russell King - ARM Linux wrote:
>>>> I am not sure if i get this completely. In memcpy, we still don't support
>>>> more than one sg. We have created a new member in txd, which keeps track of
>>>> data (addresses, len).
>> Yes, but we shouldn't need to translate it into any kind of scatterlist.
>>
> 
> Ok. I way out is keeping two separate variables in txd, list for slave transfers,
> and pointer to single element for memcpy. And that looks to be even bad to me.
> Why waste memory, for second variable. Or create union of both.
> 
> One more thing, we can actually have scatter gather in memcpy too in future. This will be
> helpful then also. But surely that's something not implemented currently.
> 

Russell/Linus,

Probably this is the only issue left in this patch. (Sorry if i missed some other points)
Please suggest what should i modify here, so that Vinod can go ahead and push pending
patches too.

-- 
viresh

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

[Linus Walleij]

On Fri, Aug 26, 2011 at 5:41 AM, Viresh Kumar <viresh.kumar@st.com> wrote:

> Probably this is the only issue left in this patch. (Sorry if i missed some other points)
> Please suggest what should i modify here, so that Vinod can go ahead and push pending
> patches too.

Patches are individual for that very reason - so they can be applied one by one.

I already suggested to Vinod to merge the other patches as far as possible,
for certain 1 thru 15 could be merged right off with some minor
manual fixup. Then we can fix this one patch.

Yours,
Linus Walleij

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

[Viresh Kumar]

On 8/26/2011 1:33 PM, Linus Walleij wrote:
> Patches are individual for that very reason - so they can be applied one by one.
> 
> I already suggested to Vinod to merge the other patches as far as possible,
> for certain 1 thru 15 could be merged right off with some minor
> manual fixup. Then we can fix this one patch.

Actually, Vinod has already applied earlier 17 patches. Now only 3 are left, this one
and two after it.

I just wanted to know, whether i need to do any other modification in this patch or not?

-- 
viresh

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

[Viresh Kumar]

On 8/26/2011 2:21 PM, Viresh Kumar wrote:
> I just wanted to know, whether i need to do any other modification in this patch or not?

Hello,

Do i need to update anything in this patch? Or can this be pushed as it is?

-- 
viresh

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

[Koul, Vinod]

On Thu, 2011-09-01 at 15:37 +0530, Viresh Kumar wrote:
> On 8/26/2011 2:21 PM, Viresh Kumar wrote:
> > I just wanted to know, whether i need to do any other modification in this patch or not?
> 
> Hello,
> 
> Do i need to update anything in this patch? Or can this be pushed as it is?
> 
I thought Linus W, and Russell had some comments on this. 

--
~Vinod

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

[Linus Walleij]

2011/9/7 Koul, Vinod <vinod.koul@intel.com>:
> On Thu, 2011-09-01 at 15:37 +0530, Viresh Kumar wrote:
>> On 8/26/2011 2:21 PM, Viresh Kumar wrote:
>> > I just wanted to know, whether i need to do any other modification in this patch or not?
>>
>> Hello,
>>
>> Do i need to update anything in this patch? Or can this be pushed as it is?
>>
> I thought Linus W, and Russell had some comments on this.

I think the patch brings valuable functionality we don't want to loose when
there is a solution. Basically the dmaengine has a contract to handle
sglists of any lengths and it's a pity that we don't, and I suspect Viresh
cannot use the driver for MMC unless something like this is added, so
Acked-by: Linus Walleij <linus.walleij@linaro.org>

BUT I think it is possible to rewrite it a bit later so as to get a better
handling of this. Isn't Russells initial remark that the LLI:s can simply just
take in the entire sglist at once true?

Check for example in drivers/dma/coh901318.c which evidently has
a LLI format similar or identical to what pl08x use (which is another
reason to refactor this later):

It will just fill in LLIs for all the elements of the sglist, and off you go.

Like this:

       /* The dma only supports transmitting packages up to
         * MAX_DMA_PACKET_SIZE. Calculate to total number of
         * dma elemts required to send the entire sg list
         */
        for_each_sg(sgl, sg, sg_len, i) {
                unsigned int factor;
                size = sg_dma_len(sg);

                if (size <= MAX_DMA_PACKET_SIZE) {
                        len++;
                        continue;
                }

                factor = size >> MAX_DMA_PACKET_SIZE_SHIFT;
                if ((factor << MAX_DMA_PACKET_SIZE_SHIFT) < size)
                        factor++;

                len += factor;
        }

        pr_debug("Allocate %d lli:s for this transfer\n", len);
        lli = coh901318_lli_alloc(&cohc->base->pool, len);

        if (lli == NULL)
                goto err_dma_alloc;

        /* initiate allocated lli list */
        ret = coh901318_lli_fill_sg(&cohc->base->pool, lli, sgl, sg_len,
                                    cohc_dev_addr(cohc),
                                    ctrl_chained,
                                    ctrl,
                                    ctrl_last,
                                    direction, COH901318_CX_CTRL_TC_IRQ_ENABLE);
        if (ret)
                goto err_lli_fill;


The rest of interesting code is in coh901318_lli_fill_sg() as you can
see.

Evidently this need to be fixed as part of factoring out the LLI handling
from PL08x and coh901318 into a single one. If we ever get to it.

Yours,
Linus Walleij

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

[Viresh Kumar]

On 9/8/2011 4:31 AM, Linus Walleij wrote:
> I think the patch brings valuable functionality we don't want to loose when
> there is a solution. Basically the dmaengine has a contract to handle
> sglists of any lengths and it's a pity that we don't, and I suspect Viresh
> cannot use the driver for MMC unless something like this is added, so
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 

Thanks Again.

> BUT I think it is possible to rewrite it a bit later so as to get a better
> handling of this. Isn't Russells initial remark that the LLI:s can simply just
> take in the entire sglist at once true?

If i am getting this clearly, the concern is "why to queue separate transfers for
individual sg's? Better would be to prepare the complete list at once and
start the transfer, so that DMA stops only after finishing all sg's
passed from user." Is this what you are pointing at?

If yes, then the same is done in this patch too. An array for llis is allocated at 
the start, then for each sg i prepare lli list from this array. Last lli from one sg
is followed by first lli from next sg. And so i get a continuous chain of llis.

-- 
viresh

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

[Linus Walleij]

On Thu, Sep 8, 2011 at 5:50 AM, Viresh Kumar <viresh.kumar@st.com> wrote:

> If i am getting this clearly, the concern is "why to queue separate transfers for
> individual sg's? Better would be to prepare the complete list at once and
> start the transfer, so that DMA stops only after finishing all sg's
> passed from user." Is this what you are pointing at?

Yes.

> If yes, then the same is done in this patch too. An array for llis is allocated at
> the start, then for each sg i prepare lli list from this array. Last lli from one sg
> is followed by first lli from next sg. And so i get a continuous chain of llis.

OK so I guess I was lost in the code ...

So this is mainy cached as txd->dsg_list so you can quickly retrieve the
number of bytes pending in the LLI by traversing that sglist.

This is better than what the coh901318 does, because that driver
resorts to going into the physical LLIs themselves to retrieve this
info.

It also seems like this will play nice with Per Forlin's MMC
speed-up patches, so that will become effective for your MMC
usecases.

Now I really like this patch.

Sorry for being such a slow learner!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 16/18] dmaengine/ambhe rest oa-pl08x: Add support for sg len greater than one for slave transfers

[Vinod Koul]

On Thu, 2011-09-08 at 12:29 +0200, Linus Walleij wrote:
> On Thu, Sep 8, 2011 at 5:50 AM, Viresh Kumar <viresh.kumar@st.com> wrote:
> 
> > If i am getting this clearly, the concern is "why to queue separate transfers for
> > individual sg's? Better would be to prepare the complete list at once and
> > start the transfer, so that DMA stops only after finishing all sg's
> > passed from user." Is this what you are pointing at?
> 
> Yes.
> 
> > If yes, then the same is done in this patch too. An array for llis is allocated at
> > the start, then for each sg i prepare lli list from this array. Last lli from one sg
> > is followed by first lli from next sg. And so i get a continuous chain of llis.
> 
> OK so I guess I was lost in the code ...
> 
> So this is mainy cached as txd->dsg_list so you can quickly retrieve the
> number of bytes pending in the LLI by traversing that sglist.
> 
> This is better than what the coh901318 does, because that driver
> resorts to going into the physical LLIs themselves to retrieve this
> info.
> 
> It also seems like this will play nice with Per Forlin's MMC
> speed-up patches, so that will become effective for your MMC
> usecases.
> 
> Now I really like this patch.
> 
> Sorry for being such a slow learner!
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Okay, great. I will check the rest of the series (i think 3 patches) and
apply then next week.

-- 
~Vinod Koul
Intel Corp.

[PATCH 16/18] dmaengine/ambhe rest oa-pl08x: Add support for sg len greater than one for slave transfers

[Vinod Koul]

On Fri, 2011-09-09 at 03:24 +0530, Vinod Koul wrote:
> On Thu, 2011-09-08 at 12:29 +0200, Linus Walleij wrote:
> > On Thu, Sep 8, 2011 at 5:50 AM, Viresh Kumar <viresh.kumar@st.com> wrote:
> > 
> > > If i am getting this clearly, the concern is "why to queue separate transfers for
> > > individual sg's? Better would be to prepare the complete list at once and
> > > start the transfer, so that DMA stops only after finishing all sg's
> > > passed from user." Is this what you are pointing at?
> > 
> > Yes.
> > 
> > > If yes, then the same is done in this patch too. An array for llis is allocated at
> > > the start, then for each sg i prepare lli list from this array. Last lli from one sg
> > > is followed by first lli from next sg. And so i get a continuous chain of llis.
> > 
> > OK so I guess I was lost in the code ...
> > 
> > So this is mainy cached as txd->dsg_list so you can quickly retrieve the
> > number of bytes pending in the LLI by traversing that sglist.
> > 
> > This is better than what the coh901318 does, because that driver
> > resorts to going into the physical LLIs themselves to retrieve this
> > info.
> > 
> > It also seems like this will play nice with Per Forlin's MMC
> > speed-up patches, so that will become effective for your MMC
> > usecases.
> > 
> > Now I really like this patch.
> > 
> > Sorry for being such a slow learner!
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Okay, great. I will check the rest of the series (i think 3 patches) and
> apply then next week.
Applied the remaining two patches for the series to my next

-- 
~Vinod

[PATCH 16/18] dmaengine/ambhe rest oa-pl08x: Add support for sg len greater than one for slave transfers

[Viresh Kumar]

On 9/20/2011 11:49 AM, Vinod Koul wrote:
> Applied the remaining two patches for the series to my next

Probably there were three patches left, 18/20, 19/20, 20/20 in V3 of my patchset.

-- 
viresh

[PATCH 16/18] dmaengine/ambhe rest oa-pl08x: Add support for sg len greater than one for slave transfers

[Vinod Koul]

On Tue, 2011-09-20 at 11:57 +0530, Viresh Kumar wrote:
> On 9/20/2011 11:49 AM, Vinod Koul wrote:
> > Applied the remaining two patches for the series to my next
> 
> Probably there were three patches left, 18/20, 19/20, 20/20 in V3 of my patchset.
Looks like I had applied patch 20 earlier...


-- 
~Vinod

[PATCH 17/18] 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>
---
 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 ddc06bd..a72255c 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -784,7 +784,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 17/18] dmaengine/amba-pl08x: Check txd->llis_va before freeing dma_pool

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> In pl08x_free_txd(), check if pool is allocated successfully before freeing it.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>

Looks like a good idea.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 18/18] 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>
---
 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 a72255c..b2a95ce 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1193,7 +1193,7 @@ 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);
+		pl08x_free_txd(pl08x, txd);
 		return -EINVAL;
 	}
 
-- 
1.7.2.2

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

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 04:19:28PM +0530, Viresh Kumar wrote:
> 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>
> ---
>  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 a72255c..b2a95ce 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -1193,7 +1193,7 @@ 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);
> +		pl08x_free_txd(pl08x, txd);

pl08x_free_txd() is supposed to be called under the channel spinlock,
which is why it isn't used here.  We don't want to hold the spinlock
throughout the LLI filling because that could cause some problems,
and potentially worsen IRQ latency.

It's something which needs more work...

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

[viresh kumar]

On 07/29/2011 04:45 PM, Russell King - ARM Linux wrote:
>> > @@ -1193,7 +1193,7 @@ 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);
>> > +		pl08x_free_txd(pl08x, txd);
> pl08x_free_txd() is supposed to be called under the channel spinlock,
> which is why it isn't used here.  We don't want to hold the spinlock
> throughout the LLI filling because that could cause some problems,
> and potentially worsen IRQ latency.
> 
> It's something which needs more work...

Maybe we can place lock-unlock around this call to pl08x_free_txd()?

-- 
viresh

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

[Linus Walleij]

2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

> 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>

Good idea,
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[viresh kumar]

On 7/30/11, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/7/29 Viresh Kumar <viresh.kumar@st.com>:
>
>> 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>
>
> Good idea,
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yet there is a problem with this patch, as pointed by Russell.
pl08x_free_txd() must be called with channel lock taken. Will
check what can be done here.

--
viresh

[PATCH 00/18] dmaengine/amba-pl08x updates

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 04:19:10PM +0530, Viresh Kumar wrote:
> 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

Peripheral flow is broken beyond belief.  It doesn't work for at all
for scatterlists.  The way peripheral flow control works on PL08x is
as follows:

1. You build a scatterlist which is a list of [address, length] pairs
2. You pass that to the DMA controller.  The DMA controller ignores the
   length field.
3. The DMA controller reads/writes to [address] and increments it.
4. The DMA controller moves to the next SG entry when the peripheral
   signals an end-of-transfer.

Please explain how you're getting around the issue where the scatterlist
length is not respected by the DMA controller.

[PATCH 00/18] dmaengine/amba-pl08x updates

[viresh kumar]

Hi Russell,

On 07/29/2011 04:27 PM, Russell King - ARM Linux wrote:
> Peripheral flow is broken beyond belief.  It doesn't work for at all
> for scatterlists.  The way peripheral flow control works on PL08x is
> as follows:
> 
> 1. You build a scatterlist which is a list of [address, length] pairs
> 2. You pass that to the DMA controller.  The DMA controller ignores the
>    length field.
> 3. The DMA controller reads/writes to [address] and increments it.
> 4. The DMA controller moves to the next SG entry when the peripheral
>    signals an end-of-transfer.
> 
> Please explain how you're getting around the issue where the scatterlist
> length is not respected by the DMA controller.

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.

I haven't added any comment for second case anywhere in the code. Will do that
after i get first level of review comments.

-- 
viresh

[PATCH 00/18] dmaengine/amba-pl08x updates

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 04:44:30PM +0530, viresh kumar wrote:
> 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.

Ok, so lets have the driver explicitly enforce one SG entry if peripheral
flow is set.  It would also be a good idea to pass not zero length, but
the real transfer size in the SG entry for the sake of having the API
know the full information about the transfer.

(Ok, it already only supports one SG entry, but that's something else
which needs fixing - which may be possible now that you've cleaned up
the LLI creation code.)

[PATCH 00/18] dmaengine/amba-pl08x updates

[viresh kumar]

On 07/29/2011 04:49 PM, Russell King - ARM Linux wrote:
> Ok, so lets have the driver explicitly enforce one SG entry if peripheral
> flow is set.  It would also be a good idea to pass not zero length, but
> the real transfer size in the SG entry for the sake of having the API
> know the full information about the transfer.

In my case, JPEG driver doesn't know how much data to transfer. So i have to
send zero from there. Also, i am not sure why would peripheral needs to be
flow controller if the peripherals driver already know how much to transfer?

> 
> (Ok, it already only supports one SG entry, but that's something else
> which needs fixing - which may be possible now that you've cleaned up
> the LLI creation code.)
> .

Yes. I cleaned it up.

-- 
viresh

[PATCH 00/18] dmaengine/amba-pl08x updates

[Russell King - ARM Linux]

On Fri, Jul 29, 2011 at 04:53:52PM +0530, viresh kumar wrote:
> On 07/29/2011 04:49 PM, Russell King - ARM Linux wrote:
> > Ok, so lets have the driver explicitly enforce one SG entry if peripheral
> > flow is set.  It would also be a good idea to pass not zero length, but
> > the real transfer size in the SG entry for the sake of having the API
> > know the full information about the transfer.
> 
> In my case, JPEG driver doesn't know how much data to transfer. So i have to
> send zero from there. Also, i am not sure why would peripheral needs to be
> flow controller if the peripherals driver already know how much to transfer?

If the jpeg driver doesn't know how much data will be transferred, how
can it allocate a buffer for the device to DMA into?

[PATCH 00/18] dmaengine/amba-pl08x updates

[viresh kumar]

On 7/29/11, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Fri, Jul 29, 2011 at 04:53:52PM +0530, viresh kumar wrote:
>> On 07/29/2011 04:49 PM, Russell King - ARM Linux wrote:
>> In my case, JPEG driver doesn't know how much data to transfer. So i have
>> to
>> send zero from there. Also, i am not sure why would peripheral needs to be
>> flow controller if the peripherals driver already know how much to
>> transfer?
>
> If the jpeg driver doesn't know how much data will be transferred, how
> can it allocate a buffer for the device to DMA into?
>

We can specify the upper limit to jpeg after which it stops the
conversion and DMA transfer
inbetween. At this time, we need to start a new DMA transfer with a
separate buffer.

Obviously, for all packets leaving the last, this upper limit is the
size of data transferred before
stopping DMA. But for the last packet we don't how much data will be
there (<= upper limit)

[PATCH 00/18] dmaengine/amba-pl08x updates

[Koul, Vinod]

On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> 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
> - etc
Viresh,

There were some fixes required in your driver for changing submit and
issue_pending behavior, was it this one?

-- 
~Vinod

[PATCH 00/18] dmaengine/amba-pl08x updates

[viresh kumar]

On 07/29/2011 05:28 PM, Koul, Vinod wrote:
> There were some fixes required in your driver for changing submit and
> issue_pending behavior, was it this one?

Actually not. Also, i just wanted to understand their purpose
(submit & issue_pending) and there were no issues as such in
this driver.

-- 
viresh