Skip to contentSkip to conversation

PL08x DMA

PATCH New Russell King - ARM Linux 3 participants 61 messages Jan 3, 2011 – Jan 17, 2011 dmaengine

lore
Share & tools
lore
#0 lore raw

cover PL08x DMA

Here's the current state of play with the PL08x driver - a massive 48 patches. The diffstat doesn't look too impresive:

 drivers/dma/amba-pl08x.c   | 1040 +++++++++++++++++++++-----------------------
 include/linux/amba/pl08x.h |   56 ++--
 2 files changed, 520 insertions(+), 576 deletions(-)

except that the two files comprise some 2300 lines, so this covers about a quarter of the driver.

The obvious fixes come first in the series, followed by the simpler changes, gradually progressing towards more and more invasive stuff.

One thing I really don't like are APIs where a lock is held across two calls - especially when it can be easily worked around. Patches 45 and 46 which sort this are intentionally towards the end.

Another thing to point out is that platforms can now control which buses the PL08x uses for transfers for memory, peripherals, etc. The logic in pl08x_select_bus() is intentionally non-symetric to prevent both source and destination transfers ending up on the same bus when both are available. However, the logic is only well defined for non-zero values of the source and destination bus masks:

src dst M1 M2

01  01  SRC+DST
01  10  SRC     DST
01  11  SRC     DST
10  01  DST     SRC
10  10          SRC+DST
10  11  DST     SRC
11  01  DST     SRC
11  10  SRC     DST
11  11  SRC     DST

(src/dst bit0 = M1 available, bit1 = M2 available)

If they happen to be zero, then the result is:

src dst M1      M2
00  00          SRC+DST
00  01  DST     SRC    
00  10          SRC+DST
00  11  DST     SRC    
01  00  SRC     DST    
10  00          SRC+DST
11  00          SRC+DST

so not quite what would be expected - so don't have zero bus masks.

#1 lore raw

PATCH 1/48 ARM: PL08x: fix spelling errors

Correct mis-spellings in comments and printk strings.

---
 drivers/dma/amba-pl08x.c |   37 +++++++++++++++++--------------------
 1 files changed, 17 insertions(+), 20 deletions(-)
drivers/dma/amba-pl08x.c +17 -20
index b605cc9..3da49ed 100644
@@ -19,7 +19,7 @@
  * this program; if not, write to the Free Software Foundation, Inc., 59
  * Temple Place - Suite 330, Boston, MA  02111-1307, USA.
  *
- * The full GNU General Public License is iin this distribution in the
+ * The full GNU General Public License is in this distribution in the
  * file called COPYING.
  *
  * Documentation: ARM DDI 0196G == PL080
@@ -82,7 +82,7 @@
 
 /**
  * struct vendor_data - vendor-specific config parameters
- * for PL08x derivates
+ * for PL08x derivatives
  * @name: the name of this specific variant
  * @channels: the number of channels available in this variant
  * @dualmaster: whether this version supports dual AHB masters
@@ -96,10 +96,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
+ * 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.
  */
 struct lli {
 	dma_addr_t src;
@@ -152,7 +150,7 @@ struct pl08x_driver_data {
 /* Size (bytes) of each LLI buffer allocated for one transfer */
 # define PL08X_LLI_TSFR_SIZE	0x2000
 
-/* Maximimum times we call dma_pool_alloc on this pool without freeing */
+/* Maximum times we call dma_pool_alloc on this pool without freeing */
 #define PL08X_MAX_ALLOCS	0x40
 #define MAX_NUM_TSFR_LLIS	(PL08X_LLI_TSFR_SIZE/sizeof(struct lli))
 #define PL08X_ALIGN		8
@@ -177,7 +175,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
+ * The next LLI pointer and the configuration interrupt bit have
  * been set when the LLIs were constructed
  */
 static void pl08x_set_cregs(struct pl08x_driver_data *pl08x,
@@ -366,8 +364,7 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
 			while (clli) {
 				bytes += get_bytes_in_cctl(llis_va[i].cctl);
 				/*
-				 * A clli of 0x00000000 will terminate the
-				 * LLI list
+				 * A LLI pointer of 0 terminates the LLI list
 				 */
 				clli = llis_va[i].next;
 				i++;
@@ -469,7 +466,7 @@ static inline u32 pl08x_cctl_bits(u32 cctl, u8 srcwidth, u8 dstwidth,
 {
 	u32 retbits = cctl;
 
-	/* Remove all src, dst and transfersize bits */
+	/* Remove all src, dst and transfer size bits */
 	retbits &= ~PL080_CONTROL_DWIDTH_MASK;
 	retbits &= ~PL080_CONTROL_SWIDTH_MASK;
 	retbits &= ~PL080_CONTROL_TRANSFER_SIZE_MASK;
@@ -701,7 +698,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	 * Choose bus to align to
 	 * - prefers destination bus if both available
 	 * - if fixed address on one bus chooses other
-	 * - modifies cctl to choose an apropriate master
+	 * - modifies cctl to choose an appropriate master
 	 */
 	pl08x_choose_master_bus(&txd->srcbus, &txd->dstbus,
 				&mbus, &sbus, cctl);
@@ -775,7 +772,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				target_len = max_bytes_per_lli;
 
 			/*
-			 * Set bus lengths for incrementing busses
+			 * Set bus lengths for incrementing buses
 			 * to number of bytes which fill to next memory
 			 * boundary
 			 */
@@ -826,7 +823,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				/*
 				 * So now we know how many bytes to transfer
 				 * to get to the nearest boundary
-				 * The next lli will past the 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
@@ -884,7 +881,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 						&& (remainder); j++) {
 					cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
 					dev_vdbg(&pl08x->adev->dev,
-						"%s align with boundardy, single byte (remain %08x)\n",
+						"%s align with boundary, single byte (remain %08x)\n",
 						__func__, remainder);
 					num_llis =
 						pl08x_fill_lli_for_desc(pl08x,
@@ -907,7 +904,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		while (remainder) {
 			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
 			dev_vdbg(&pl08x->adev->dev,
-				"%s align with boundardy, single odd byte (remain %d)\n",
+				"%s align with boundary, single odd byte (remain %d)\n",
 				__func__, remainder);
 			num_llis = pl08x_fill_lli_for_desc(pl08x, txd, num_llis,
 					1, cctl, &remainder);
@@ -1367,8 +1364,8 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 		 * available to handle it whereas slave transfers may
 		 * have been denied due to platform channel muxing restrictions
 		 * and since there is no guarantee that this will ever be
-		 * resolved, and since the signal must be aquired AFTER
-		 * aquiring the physical channel, we will let them be NACK:ed
+		 * resolved, and since the signal must be acquired AFTER
+		 * acquiring the physical channel, we will let them be NACK:ed
 		 * with -EBUSY here. The drivers can alway retry the prep()
 		 * call if they are eager on doing this using DMA.
 		 */
@@ -1620,7 +1617,7 @@ static void pl08x_ensure_on(struct pl08x_driver_data *pl08x)
 
 	val = readl(pl08x->base + PL080_CONFIG);
 	val &= ~(PL080_CONFIG_M2_BE | PL080_CONFIG_M1_BE | PL080_CONFIG_ENABLE);
-	/* We implictly clear bit 1 and that means little-endian mode */
+	/* We implicitly clear bit 1 and that means little-endian mode */
 	val |= PL080_CONFIG_ENABLE;
 	writel(val, pl08x->base + PL080_CONFIG);
 }
@@ -2160,7 +2157,7 @@ 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",
+		       "failed to register as an AMBA device (%d)\n",
 		       retval);
 	return retval;
 }
Signature · 1 line
1.6.2.5
#2 lore raw

PATCH 2/48 ARM: PL08x: fix array overflow in dma_set_runtime_config()

In reply to Russell King - ARM Linux

If maxburst was passed in as zero, we would overflow the burst_sizes[] array. Fix this by checking for this condition, and defaulting to single transfer 'bursts'.

Improve the readability of the loop using a for() loop rather than a while() loop with the iterator initialized far from the loop.

---
 drivers/dma/amba-pl08x.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
drivers/dma/amba-pl08x.c +6 -7
index 3da49ed..0809810 100644
@@ -1207,7 +1207,7 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 	u32 cctl = 0;
 	/* Mask out all except src and dst channel */
 	u32 ccfg = cd->ccfg & 0x000003DEU;
-	int i = 0;
+	int i;
 
 	/* Transfer direction */
 	plchan->runtime_direction = config->direction;
@@ -1250,18 +1250,17 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 
 	/*
 	 * Now decide on a maxburst:
-	 * If this channel will only request single transfers, set
-	 * this down to ONE element.
+	 * If this channel will only request single transfers, set this
+	 * down to ONE element.  Also select one element if no maxburst
+	 * is specified.
 	 */
-	if (plchan->cd->single) {
+	if (plchan->cd->single || maxburst == 0) {
 		cctl |= (PL080_BSIZE_1 << PL080_CONTROL_SB_SIZE_SHIFT) |
 			(PL080_BSIZE_1 << PL080_CONTROL_DB_SIZE_SHIFT);
 	} else {
-		while (i < ARRAY_SIZE(burst_sizes)) {
+		for (i = 0; i < ARRAY_SIZE(burst_sizes); i++)
 			if (burst_sizes[i].burstwords <= maxburst)
 				break;
-			i++;
-		}
 		cctl |= burst_sizes[i].reg;
 	}
 
Signature · 1 line
1.6.2.5
#3 lore raw

PATCH 3/48 ARM: PL08x: fix atomic_t usage and tx_submit() return value range

In reply to Russell King - ARM Linux

The last_issued variable uses an atomic type, which is only incremented inside a protected region, and then read. Everywhere else only reads the value, so it isn't using atomic_t correctly, and it doesn't even need to. Moreover, the DMA engine code provides us with a variable for this already - chan.cookie. Use chan.cookie instead.

Also, avoid negative dma_cookie_t values - negative returns from tx_submit() mean failure, yet in reality we always succeed. Restart from cookie 1, just like other DMA engine drivers do.

---
 drivers/dma/amba-pl08x.c   |   18 +++++++++---------
 include/linux/amba/pl08x.h |    1 -
 2 files changed, 9 insertions(+), 10 deletions(-)
drivers/dma/amba-pl08x.c +9 -9
index 0809810..5d9a156 100644
@@ -74,7 +74,6 @@
 #include <asm/hardware/pl080.h>
 #include <asm/dma.h>
 #include <asm/mach/dma.h>
-#include <asm/atomic.h>
 #include <asm/processor.h>
 #include <asm/cacheflush.h>
 
@@ -1082,8 +1081,10 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(tx->chan);
 
-	atomic_inc(&plchan->last_issued);
-	tx->cookie = atomic_read(&plchan->last_issued);
+	plchan->chan.cookie += 1;
+	if (plchan->chan.cookie < 0)
+		plchan->chan.cookie = 1;
+	tx->cookie = plchan->chan.cookie;
 	/* This unlock follows the lock in the prep() function */
 	spin_unlock_irqrestore(&plchan->lock, plchan->lockflags);
 
@@ -1115,7 +1116,7 @@ pl08x_dma_tx_status(struct dma_chan *chan,
 	enum dma_status ret;
 	u32 bytesleft = 0;
 
-	last_used = atomic_read(&plchan->last_issued);
+	last_used = plchan->chan.cookie;
 	last_complete = plchan->lc;
 
 	ret = dma_async_is_complete(cookie, last_complete, last_used);
@@ -1131,7 +1132,7 @@ pl08x_dma_tx_status(struct dma_chan *chan,
 	/*
 	 * This cookie not complete yet
 	 */
-	last_used = atomic_read(&plchan->last_issued);
+	last_used = plchan->chan.cookie;
 	last_complete = plchan->lc;
 
 	/* Get number of bytes left in the active transactions and queue */
@@ -1641,8 +1642,7 @@ static void pl08x_tasklet(unsigned long data)
 		/*
 		 * Update last completed
 		 */
-		plchan->lc =
-			(plchan->at->tx.cookie);
+		plchan->lc = plchan->at->tx.cookie;
 
 		/*
 		 * Callback to signal completion
@@ -1820,8 +1820,8 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 			 chan->name);
 
 		chan->chan.device = dmadev;
-		atomic_set(&chan->last_issued, 0);
-		chan->lc = atomic_read(&chan->last_issued);
+		chan->chan.cookie = 0;
+		chan->lc = 0;
 
 		spin_lock_init(&chan->lock);
 		INIT_LIST_HEAD(&chan->desc_list);
include/linux/amba/pl08x.h +0 -1
index 521a0f8..4ae62b4 100644
@@ -174,7 +174,6 @@ struct pl08x_dma_chan {
 	struct pl08x_channel_data *cd;
 	dma_addr_t runtime_addr;
 	enum dma_data_direction	runtime_direction;
-	atomic_t last_issued;
 	dma_cookie_t lc;
 	struct list_head desc_list;
 	struct pl08x_txd *at;
Signature · 1 line
1.6.2.5
#4 lore raw

PATCH 4/48 ARM: PL08x: fix locking in tasklet

In reply to Russell King - ARM Linux

Tasklets are run from an interruptible context. The slave DMA functions can be called from within IRQ handlers. Taking the spinlock without disabling interrupts allows an interrupt handler to run, which may try to take the spinlock again, resulting in deadlock. Fix this by using the irqsave spinlocks.

---
 drivers/dma/amba-pl08x.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/dma/amba-pl08x.c +3 -2
index 5d9a156..69cfb05 100644
@@ -1627,11 +1627,12 @@ static void pl08x_tasklet(unsigned long data)
 	struct pl08x_dma_chan *plchan = (struct pl08x_dma_chan *) data;
 	struct pl08x_phy_chan *phychan = plchan->phychan;
 	struct pl08x_driver_data *pl08x = plchan->host;
+	unsigned long flags;
 
 	if (!plchan)
 		BUG();
 
-	spin_lock(&plchan->lock);
+	spin_lock_irqsave(&plchan->lock, flags);
 
 	if (plchan->at) {
 		dma_async_tx_callback callback =
@@ -1728,7 +1729,7 @@ static void pl08x_tasklet(unsigned long data)
 		}
 	}
 
-	spin_unlock(&plchan->lock);
+	spin_unlock_irqrestore(&plchan->lock, flags);
 }
 
 static irqreturn_t pl08x_irq(int irq, void *dev)
Signature · 1 line
1.6.2.5
#5 lore raw

PATCH 5/48 ARM: PL08x: fix a leak when preparing TXDs

In reply to Russell King - ARM Linux

If we fail to allocate the LLI, the prep_* function will return NULL. However, the TXD we allocated will not be placed on any list, nor will it be freed - we'll just drop all references to it. Make sure we free it rather than leaking TXDs.

---
 drivers/dma/amba-pl08x.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/dma/amba-pl08x.c +3 -2
index 69cfb05..b3b3180 100644
@@ -1332,9 +1332,10 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 	int ret;
 
 	num_llis = pl08x_fill_llis_for_desc(pl08x, txd);
-
-	if (!num_llis)
+	if (!num_llis) {
+		kfree(txd);
 		return -EINVAL;
+	}
 
 	spin_lock_irqsave(&plchan->lock, plchan->lockflags);
 
Signature · 1 line
1.6.2.5
#6 lore raw

PATCH 6/48 ARM: PL08x: fix missed spin-unlock in pl08x_issue_pending()

In reply to Russell King - ARM Linux

pl08x_issue_pending() returns with the spinlock locked and interrupts disabled if the channel is waiting for a physical DMA to become free. This is wrong - especially as pl08x_issue_pending() is an API function as it leads to deadlocks. Fix it to always return with the spinlock unlocked.

---
 drivers/dma/amba-pl08x.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
drivers/dma/amba-pl08x.c +4 -8
index b3b3180..9a8d445 100644
@@ -1294,15 +1294,11 @@ static void pl08x_issue_pending(struct dma_chan *chan)
 	unsigned long flags;
 
 	spin_lock_irqsave(&plchan->lock, flags);
-	/* Something is already active */
-	if (plchan->at) {
-			spin_unlock_irqrestore(&plchan->lock, flags);
-			return;
-	}
-
-	/* Didn't get a physical channel so waiting for it ... */
-	if (plchan->state == PL08X_CHAN_WAITING)
+	/* Something is already active, or we're waiting for a channel... */
+	if (plchan->at || plchan->state == PL08X_CHAN_WAITING) {
+		spin_unlock_irqrestore(&plchan->lock, flags);
 		return;
+	}
 
 	/* Take the first element in the queue and execute it */
 	if (!list_empty(&plchan->desc_list)) {
Signature · 1 line
1.6.2.5
#7 lore raw

PATCH 7/48 ARM: PL08x: fix deadlock in terminate_all

In reply to Russell King - ARM Linux

Trying to disable a tasklet while holding a spinlock which the tasklet will take is a recipe for deadlock - tasklet_disable() will wait for the tasklet to finish running, which it will never do. In any case, there is not a corresponding tasklet_enable(), so once the tasklet is disabled, it will never run again until reboot.

It's safe to just remove the tasklet_disable() as we remove all current and pending descriptors before releasing this spinlock. This means that the tasklet will find no remaining work if it subsequently runs.

The only remaining issue is that the callback for an already submitted txd may be in progress, or even called after terminate_all() returns. There's not much that can be done about that as waiting for the callback to complete before returning will also lead to deadlocks.

---
 drivers/dma/amba-pl08x.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/dma/amba-pl08x.c +0 -2
index 9a8d445..4ca1eb1 100644
@@ -1560,8 +1560,6 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 			pl08x_put_phy_channel(pl08x, plchan->phychan);
 			plchan->phychan = NULL;
 		}
-		/* Stop any pending tasklet */
-		tasklet_disable(&plchan->tasklet);
 		/* Dequeue jobs and free LLIs */
 		if (plchan->at) {
 			pl08x_free_txd(pl08x, plchan->at);
Signature · 1 line
1.6.2.5
#8 lore raw

PATCH 8/48 ARM: PL08x: fix sparse warnings

In reply to Russell King - ARM Linux

drivers/dma/amba-pl08x.c:1895:40: warning: Unknown escape '%'
drivers/dma/amba-pl08x.c:1903:40: warning: Unknown escape '%'
drivers/dma/amba-pl08x.c:513:6: warning: symbol 'pl08x_choose_master_bus' was not declared. Should it be static? drivers/dma/amba-pl08x.c:604:5: warning: symbol 'pl08x_fill_llis_for_desc' was not declared. Should it be static? drivers/dma/amba-pl08x.c:1442:32: warning: symbol 'pl08x_prep_slave_sg' was not declared. Should it be static?

---
 drivers/dma/amba-pl08x.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
drivers/dma/amba-pl08x.c +5 -5
index 4ca1eb1..74fa5a0 100644
@@ -510,7 +510,7 @@ static inline u32 pl08x_cctl_bits(u32 cctl, u8 srcwidth, u8 dstwidth,
  * this prefers the destination bus if both available
  * if fixed address on one bus the other will be chosen
  */
-void pl08x_choose_master_bus(struct pl08x_bus_data *src_bus,
+static void pl08x_choose_master_bus(struct pl08x_bus_data *src_bus,
 	struct pl08x_bus_data *dst_bus, struct pl08x_bus_data **mbus,
 	struct pl08x_bus_data **sbus, u32 cctl)
 {
@@ -545,7 +545,7 @@ void pl08x_choose_master_bus(struct pl08x_bus_data *src_bus,
  * Fills in one LLI for a certain transfer descriptor
  * and advance the counter
  */
-int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
+static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 			    struct pl08x_txd *txd, int num_llis, int len,
 			    u32 cctl, u32 *remainder)
 {
@@ -1439,7 +1439,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	return &txd->tx;
 }
 
-struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
+static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		struct dma_chan *chan, struct scatterlist *sgl,
 		unsigned int sg_len, enum dma_data_direction direction,
 		unsigned long flags)
@@ -1890,7 +1890,7 @@ static int pl08x_debugfs_show(struct seq_file *s, void *data)
 	seq_printf(s, "CHANNEL:\tSTATE:\n");
 	seq_printf(s, "--------\t------\n");
 	list_for_each_entry(chan, &pl08x->memcpy.channels, chan.device_node) {
-		seq_printf(s, "%s\t\t\%s\n", chan->name,
+		seq_printf(s, "%s\t\t%s\n", chan->name,
 			   pl08x_state_str(chan->state));
 	}
 
@@ -1898,7 +1898,7 @@ static int pl08x_debugfs_show(struct seq_file *s, void *data)
 	seq_printf(s, "CHANNEL:\tSTATE:\n");
 	seq_printf(s, "--------\t------\n");
 	list_for_each_entry(chan, &pl08x->slave.channels, chan.device_node) {
-		seq_printf(s, "%s\t\t\%s\n", chan->name,
+		seq_printf(s, "%s\t\t%s\n", chan->name,
 			   pl08x_state_str(chan->state));
 	}
 
Signature · 1 line
1.6.2.5
#9 lore raw

PATCH 9/48 ARM: PL08x: add comment explaining the flow control methods

In reply to Russell King - ARM Linux

Explain the two flow control methods which the PL08x implements, along with the problem which peripheral flow control presents. This helps people understand why we are unable to use these DMA controllers with (eg) the MMCI.

---
 drivers/dma/amba-pl08x.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)
drivers/dma/amba-pl08x.c +17 -1
index 74fa5a0..4b63fc3 100644
@@ -53,7 +53,23 @@
  *
  * ASSUMES default (little) endianness for DMA transfers
  *
- * Only DMAC flow control is implemented
+ * 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
+ *    to both the BREQ and SREQ signals (contrary to documented),
+ *    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
+ *    will then move to the next LLI entry.
+ *
+ * Only the former works sanely with scatter lists, so we only implement
+ * the DMAC flow control method.  However, peripherals which use the LBREQ
+ * and LSREQ signals (eg, MMCI) are unable to use this mode, which through
+ * these hardware restrictions prevents them from using scatter DMA.
  *
  * Global TODO:
  * - Break out common code from arch/arm/mach-s3c64xx and share
Signature · 1 line
1.6.2.5
#10 lore raw

PATCH 10/48 ARM: PL08x: improve the announcement printk

In reply to Russell King - ARM Linux

Include the revision number of the PL08x primecell in the boot-time printk to allow proper identification of the peripheral. Reformat the announcement printk format reflect what we do for other primecell drivers - generally "PLXXX revX at 0xNNNNNNNN irq X".

---
 drivers/dma/amba-pl08x.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)
drivers/dma/amba-pl08x.c +4 -7
index 4b63fc3..75ab4e4 100644
@@ -98,13 +98,11 @@
 /**
  * struct vendor_data - vendor-specific config parameters
  * for PL08x derivatives
- * @name: the name of this specific variant
  * @channels: the number of channels available in this variant
  * @dualmaster: whether this version supports dual AHB masters
  * or not.
  */
 struct vendor_data {
-	char *name;
 	u8 channels;
 	bool dualmaster;
 };
@@ -2024,7 +2022,7 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 	writel(0x000000FF, pl08x->base + PL080_TC_CLEAR);
 
 	ret = request_irq(adev->irq[0], pl08x_irq, IRQF_DISABLED,
-			  vd->name, pl08x);
+			  DRIVER_NAME, pl08x);
 	if (ret) {
 		dev_err(&adev->dev, "%s failed to request interrupt %d\n",
 			__func__, adev->irq[0]);
@@ -2095,8 +2093,9 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 
 	amba_set_drvdata(adev, pl08x);
 	init_pl08x_debugfs(pl08x);
-	dev_info(&pl08x->adev->dev, "ARM(R) %s DMA block initialized @%08x\n",
-		vd->name, adev->res.start);
+	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]);
 	return 0;
 
 out_no_slave_reg:
@@ -2123,13 +2122,11 @@ out_no_pl08x:
 
 /* PL080 has 8 channels and the PL080 have just 2 */
 static struct vendor_data vendor_pl080 = {
-	.name = "PL080",
 	.channels = 8,
 	.dualmaster = true,
 };
 
 static struct vendor_data vendor_pl081 = {
-	.name = "PL081",
 	.channels = 2,
 	.dualmaster = false,
 };
Signature · 1 line
1.6.2.5
#11 lore raw

PATCH 11/48 ARM: PL08x: prefix hex numbers with 0x

In reply to Russell King - ARM Linux

A driver which emits both decimal and hex numbers in its printk creates confusion as to what is what. Prefix hex numbers with 0x.

---
 drivers/dma/amba-pl08x.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/dma/amba-pl08x.c +9 -9
index 75ab4e4..c61774a 100644
@@ -199,8 +199,8 @@ static void pl08x_set_cregs(struct pl08x_driver_data *pl08x,
 		;
 
 	dev_vdbg(&pl08x->adev->dev,
-		"WRITE channel %d: csrc=%08x, cdst=%08x, "
-		 "cctl=%08x, clli=%08x, ccfg=%08x\n",
+		"WRITE channel %d: csrc=0x%08x, cdst=0x%08x, "
+		 "cctl=0x%08x, clli=0x%08x, ccfg=0x%08x\n",
 		ch->id,
 		ch->csrc,
 		ch->cdst,
@@ -731,7 +731,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		while (remainder) {
 			dev_vdbg(&pl08x->adev->dev,
 				 "%s single byte LLIs for a transfer of "
-				 "less than a bus width (remain %08x)\n",
+				 "less than a bus width (remain 0x%08x)\n",
 				 __func__, remainder);
 			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
 			num_llis =
@@ -747,7 +747,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		while ((mbus->addr) % (mbus->buswidth)) {
 			dev_vdbg(&pl08x->adev->dev,
 				"%s adjustment lli for less than bus width "
-				 "(remain %08x)\n",
+				 "(remain 0x%08x)\n",
 				 __func__, remainder);
 			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
 			num_llis = pl08x_fill_lli_for_desc
@@ -865,7 +865,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 				if (target_len != lli_len) {
 					dev_vdbg(&pl08x->adev->dev,
-					"%s can't send what we want. Desired %08x, lli of %08x bytes in txd of %08x\n",
+					"%s can't send what we want. Desired 0x%08x, lli of 0x%08x bytes in txd of 0x%08x\n",
 					__func__, target_len, lli_len, txd->len);
 				}
 
@@ -875,7 +875,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 						       tsize);
 
 				dev_vdbg(&pl08x->adev->dev,
-					"%s fill lli with single lli chunk of size %08x (remainder %08x)\n",
+					"%s fill lli with single lli chunk of size 0x%08x (remainder 0x%08x)\n",
 					__func__, lli_len, remainder);
 				num_llis = pl08x_fill_lli_for_desc(pl08x, txd,
 						num_llis, lli_len, cctl,
@@ -894,7 +894,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 						&& (remainder); j++) {
 					cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
 					dev_vdbg(&pl08x->adev->dev,
-						"%s align with boundary, single byte (remain %08x)\n",
+						"%s align with boundary, single byte (remain 0x%08x)\n",
 						__func__, remainder);
 					num_llis =
 						pl08x_fill_lli_for_desc(pl08x,
@@ -979,7 +979,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 		for (i = 0; i < num_llis; i++) {
 			dev_vdbg(&pl08x->adev->dev,
-				 "lli %d @%p: csrc=%08x, cdst=%08x, cctl=%08x, clli=%08x\n",
+				 "lli %d @%p: csrc=0x%08x, cdst=0x%08x, cctl=0x%08x, clli=0x%08x\n",
 				 i,
 				 &llis_va[i],
 				 llis_va[i].src,
@@ -1289,7 +1289,7 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 
 	dev_dbg(&pl08x->adev->dev,
 		"configured channel %s (%s) for %s, data width %d, "
-		"maxburst %d words, LE, CCTL=%08x, CCFG=%08x\n",
+		"maxburst %d words, LE, CCTL=0x%08x, CCFG=0x%08x\n",
 		dma_chan_name(chan), plchan->name,
 		(config->direction == DMA_FROM_DEVICE) ? "RX" : "TX",
 		addr_width,
Signature · 1 line
1.6.2.5
#12 lore raw

PATCH 12/48 ARM: PL08x: remove unnecessary includes

In reply to Russell King - ARM Linux

We don't need to include linux/pci.h as we aren't a PCI driver. We aren't doing any processor specific functions, so asm/processor.h is not required. asm/cacheflush.h shouldn't be used, we have the DMA API for this. DMA interfaces aren't required as we're only implementing the dmaengine API and not a platform-private DMA API.

---
 drivers/dma/amba-pl08x.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)
drivers/dma/amba-pl08x.c +1 -6
index c61774a..d22d628 100644
@@ -77,21 +77,16 @@
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/pci.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
 #include <linux/dmapool.h>
-#include <linux/amba/bus.h>
 #include <linux/dmaengine.h>
+#include <linux/amba/bus.h>
 #include <linux/amba/pl08x.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
 
 #include <asm/hardware/pl080.h>
-#include <asm/dma.h>
-#include <asm/mach/dma.h>
-#include <asm/processor.h>
-#include <asm/cacheflush.h>
 
 #define DRIVER_NAME	"pl08xdmac"
 
Signature · 1 line
1.6.2.5
#13 lore raw

PATCH 13/48 ARM: PL08x: remove unnecessary NULL and BUG checks

In reply to Russell King - ARM Linux

The tasklet always is initialized with a non-NULL data argument. It is not possible for it to be called with a NULL data argument (unless something is very wrong in the tasklet code - in which case lots of stuff will break). Therefore, as plchan can never be NULL, remove this unnecessary BUG check.

In pl08x_tasklet(), we've already dereferenced plchan->at, so it can't be NULL here. Remove this unnecessary BUG check.

pl08x_fill_llis_for_desc() and pl08x_free_txd() are always called with a non-NULL txd argument - either as a consequence of the code paths or as a result of other checks already in place. We don't need to repeat the non-NULL check in these functions.

---
 drivers/dma/amba-pl08x.c |   21 ---------------------
 1 files changed, 0 insertions(+), 21 deletions(-)
drivers/dma/amba-pl08x.c +0 -21
index d22d628..b721ed2 100644
@@ -623,11 +623,6 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	struct lli *llis_va;
 	struct lli *llis_bus;
 
-	if (!txd) {
-		dev_err(&pl08x->adev->dev, "%s no descriptor\n", __func__);
-		return 0;
-	}
-
 	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT,
 				      &txd->llis_bus);
 	if (!txd->llis_va) {
@@ -993,11 +988,6 @@ 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)
 {
-	if (!txd)
-		dev_err(&pl08x->adev->dev,
-			"%s no descriptor to free\n",
-			__func__);
-
 	/* Free the LLI */
 	dma_pool_free(pl08x->pool, txd->llis_va,
 		      txd->llis_bus);
@@ -1633,9 +1623,6 @@ static void pl08x_tasklet(unsigned long data)
 	struct pl08x_driver_data *pl08x = plchan->host;
 	unsigned long flags;
 
-	if (!plchan)
-		BUG();
-
 	spin_lock_irqsave(&plchan->lock, flags);
 
 	if (plchan->at) {
@@ -1656,14 +1643,6 @@ static void pl08x_tasklet(unsigned long data)
 			callback(callback_param);
 
 		/*
-		 * Device callbacks should NOT clear
-		 * the current transaction on the channel
-		 * Linus: sometimes they should?
-		 */
-		if (!plchan->at)
-			BUG();
-
-		/*
 		 * Free the descriptor if it's not for a device
 		 * using a circular buffer
 		 */
Signature · 1 line
1.6.2.5
#14 lore raw

PATCH 14/48 ARM: PL08x: remove circular buffer support

In reply to Russell King - ARM Linux

The driver already won't initialize a channel with a circular buffer; the check in pl08x_prep_channel_resources() sees to that. Remove circular buffer support for the time being.

---
 drivers/dma/amba-pl08x.c |   71 +++++++++++++---------------------------------
 1 files changed, 20 insertions(+), 51 deletions(-)
drivers/dma/amba-pl08x.c +20 -51
index b721ed2..74b3591 100644
@@ -927,39 +927,21 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			__func__, (u32) MAX_NUM_TSFR_LLIS);
 		return 0;
 	}
+
+	llis_va = txd->llis_va;
 	/*
-	 * Decide whether this is a loop or a terminated transfer
+	 * The final LLI terminates the LLI.
 	 */
-	llis_va = txd->llis_va;
-	llis_bus = (struct lli *) txd->llis_bus;
-
-	if (cd->circular_buffer) {
-		/*
-		 * Loop the circular buffer so that the next element
-		 * points back to the beginning of the LLI.
-		 */
-		llis_va[num_llis - 1].next =
-			(dma_addr_t)((unsigned int)&(llis_bus[0]));
-	} else {
-		/*
-		 * On non-circular buffers, the final LLI terminates
-		 * the LLI.
-		 */
-		llis_va[num_llis - 1].next = 0;
-		/*
-		 * The final LLI element shall also fire an interrupt
-		 */
-		llis_va[num_llis - 1].cctl |= PL080_CONTROL_TC_IRQ_EN;
-	}
+	llis_va[num_llis - 1].next = 0;
+	/*
+	 * The final LLI element shall also fire an interrupt
+	 */
+	llis_va[num_llis - 1].cctl |= PL080_CONTROL_TC_IRQ_EN;
 
 	/* Now store the channel register values */
 	txd->csrc = llis_va[0].src;
 	txd->cdst = llis_va[0].dst;
-	if (num_llis > 1)
-		txd->clli = llis_va[0].next;
-	else
-		txd->clli = 0;
-
+	txd->clli = llis_va[0].next;
 	txd->cctl = llis_va[0].cctl;
 	/* ccfg will be set at physical channel allocation time */
 
@@ -1334,19 +1316,7 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 
 	spin_lock_irqsave(&plchan->lock, plchan->lockflags);
 
-	/*
-	 * If this device is not using a circular buffer then
-	 * queue this new descriptor for transfer.
-	 * The descriptor for a circular buffer continues
-	 * to be used until the channel is freed.
-	 */
-	if (txd->cd->circular_buffer)
-		dev_err(&pl08x->adev->dev,
-			"%s attempting to queue a circular buffer\n",
-			__func__);
-	else
-		list_add_tail(&txd->node,
-			      &plchan->desc_list);
+	list_add_tail(&txd->node, &plchan->desc_list);
 
 	/*
 	 * See if we already have a physical channel allocated,
@@ -1643,18 +1613,10 @@ static void pl08x_tasklet(unsigned long data)
 			callback(callback_param);
 
 		/*
-		 * Free the descriptor if it's not for a device
-		 * using a circular buffer
-		 */
-		if (!plchan->at->cd->circular_buffer) {
-			pl08x_free_txd(pl08x, plchan->at);
-			plchan->at = NULL;
-		}
-		/*
-		 * else descriptor for circular
-		 * buffers only freed when
-		 * client has disabled dma
+		 * Free the descriptor
 		 */
+		pl08x_free_txd(pl08x, plchan->at);
+		plchan->at = NULL;
 	}
 	/*
 	 * If a new descriptor is queued, set it up
@@ -1799,6 +1761,13 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 				return -ENOMEM;
 			}
 		}
+		if (chan->cd->circular_buffer) {
+			dev_err(&pl08x->adev->dev,
+				"channel %s: circular buffers not supported\n",
+				chan->name);
+			kfree(chan);
+			continue;
+		}
 		dev_info(&pl08x->adev->dev,
 			 "initialize virtual channel \"%s\"\n",
 			 chan->name);
Signature · 1 line
1.6.2.5
#15 lore raw

PATCH 15/48 ARM: PL08x: constify vendor data pointers

In reply to Russell King - ARM Linux

We should never modify the vendor data structure so make it const.

---
 drivers/dma/amba-pl08x.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/dma/amba-pl08x.c +2 -2
index 74b3591..707fa08 100644
@@ -132,7 +132,7 @@ struct pl08x_driver_data {
 	struct dma_device memcpy;
 	void __iomem *base;
 	struct amba_device *adev;
-	struct vendor_data *vd;
+	const struct vendor_data *vd;
 	struct pl08x_platform_data *pd;
 	struct pl08x_phy_chan *phy_chans;
 	struct dma_pool *pool;
@@ -1891,7 +1891,7 @@ static inline void init_pl08x_debugfs(struct pl08x_driver_data *pl08x)
 static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 {
 	struct pl08x_driver_data *pl08x;
-	struct vendor_data *vd = id->data;
+	const struct vendor_data *vd = id->data;
 	int ret = 0;
 	int i;
 
Signature · 1 line
1.6.2.5
#16 lore raw

PATCH 16/48 ARM: PL08x: avoid 'void *' struct fields when we can type them properly

In reply to Russell King - ARM Linux

Avoid using 'void *' struct fields when the structs are not defined in linux/amba/pl08x.h - instead, forward declare the struct names, and use these instead. This ensures we have proper typechecking.

---
 drivers/dma/amba-pl08x.c   |   16 ++++++++--------
 include/linux/amba/pl08x.h |    5 ++++-
 2 files changed, 12 insertions(+), 9 deletions(-)
drivers/dma/amba-pl08x.c +8 -8
index 707fa08..fada978 100644
@@ -107,7 +107,7 @@ struct vendor_data {
  * 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.
  */
-struct lli {
+struct pl08x_lli {
 	dma_addr_t src;
 	dma_addr_t dst;
 	dma_addr_t next;
@@ -160,7 +160,7 @@ struct pl08x_driver_data {
 
 /* Maximum times we call dma_pool_alloc on this pool without freeing */
 #define PL08X_MAX_ALLOCS	0x40
-#define MAX_NUM_TSFR_LLIS	(PL08X_LLI_TSFR_SIZE/sizeof(struct lli))
+#define MAX_NUM_TSFR_LLIS	(PL08X_LLI_TSFR_SIZE/sizeof(struct pl08x_lli))
 #define PL08X_ALIGN		8
 
 static inline struct pl08x_dma_chan *to_pl08x_chan(struct dma_chan *chan)
@@ -354,8 +354,8 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
 	 * currently active transaction.
 	 */
 	if (ch && txd) {
-		struct lli *llis_va = txd->llis_va;
-		struct lli *llis_bus = (struct lli *) txd->llis_bus;
+		struct pl08x_lli *llis_va = txd->llis_va;
+		struct pl08x_lli *llis_bus = (struct pl08x_lli *) txd->llis_bus;
 		u32 clli = readl(ch->base + PL080_CH_LLI);
 
 		/* First get the bytes in the current active LLI */
@@ -558,8 +558,8 @@ static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 			    struct pl08x_txd *txd, int num_llis, int len,
 			    u32 cctl, u32 *remainder)
 {
-	struct lli *llis_va = txd->llis_va;
-	struct lli *llis_bus = (struct lli *) txd->llis_bus;
+	struct pl08x_lli *llis_va = txd->llis_va;
+	struct pl08x_lli *llis_bus = (struct pl08x_lli *) txd->llis_bus;
 
 	BUG_ON(num_llis >= MAX_NUM_TSFR_LLIS);
 
@@ -620,8 +620,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	u32 cctl;
 	int max_bytes_per_lli;
 	int total_bytes = 0;
-	struct lli *llis_va;
-	struct lli *llis_bus;
+	struct pl08x_lli *llis_va;
+	struct pl08x_lli *llis_bus;
 
 	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT,
 				      &txd->llis_bus);
include/linux/amba/pl08x.h +4 -1
index 4ae62b4..3ecc20f 100644
@@ -22,6 +22,9 @@
 #include <linux/dmaengine.h>
 #include <linux/interrupt.h>
 
+struct pl08x_lli;
+struct pl08x_driver_data;
+
 /**
  * struct pl08x_channel_data - data structure to pass info between
  * platform and PL08x driver regarding channel configuration
@@ -179,7 +182,7 @@ struct pl08x_dma_chan {
 	struct pl08x_txd *at;
 	unsigned long lockflags;
 	spinlock_t lock;
-	void *host;
+	struct pl08x_driver_data *host;
 	enum pl08x_dma_chan_state state;
 	bool slave;
 	struct pl08x_txd *waiting;
Signature · 1 line
1.6.2.5
#17 lore raw

PATCH 17/48 ARM: PL08x: consolidate common txd initialization

In reply to Russell King - ARM Linux

Consolidate code which allocates and initializes txds.

---
 drivers/dma/amba-pl08x.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)
drivers/dma/amba-pl08x.c +14 -13
index fada978..d47255e 100644
@@ -1363,6 +1363,18 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 	return 0;
 }
 
+static struct pl08x_txd *pl08x_get_txd(struct pl08x_dma_chan *plchan)
+{
+	struct pl08x_txd *txd = kzalloc(sizeof(struct pl08x_txd), GFP_NOWAIT);
+
+	if (txd) {
+		dma_async_tx_descriptor_init(&txd->tx, &plchan->chan);
+		txd->tx.tx_submit = pl08x_tx_submit;
+		INIT_LIST_HEAD(&txd->node);
+	}
+	return txd;
+}
+
 /*
  * Initialize a descriptor to be used by memcpy submit
  */
@@ -1375,14 +1387,13 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	struct pl08x_txd *txd;
 	int ret;
 
-	txd = kzalloc(sizeof(struct pl08x_txd), GFP_NOWAIT);
+	txd = pl08x_get_txd(plchan);
 	if (!txd) {
 		dev_err(&pl08x->adev->dev,
 			"%s no memory for descriptor\n", __func__);
 		return NULL;
 	}
 
-	dma_async_tx_descriptor_init(&txd->tx, chan);
 	txd->direction = DMA_NONE;
 	txd->srcbus.addr = src;
 	txd->dstbus.addr = dest;
@@ -1391,12 +1402,8 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	txd->cd = &pl08x->pd->memcpy_channel;
 	/* Both to be incremented or the code will break */
 	txd->cd->cctl |= PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR;
-	txd->tx.tx_submit = pl08x_tx_submit;
-	txd->tx.callback = NULL;
-	txd->tx.callback_param = NULL;
 	txd->len = len;
 
-	INIT_LIST_HEAD(&txd->node);
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
 		return NULL;
@@ -1430,14 +1437,12 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from %s\n",
 		__func__, sgl->length, plchan->name);
 
-	txd = kzalloc(sizeof(struct pl08x_txd), GFP_NOWAIT);
+	txd = pl08x_get_txd(plchan);
 	if (!txd) {
 		dev_err(&pl08x->adev->dev, "%s no txd\n", __func__);
 		return NULL;
 	}
 
-	dma_async_tx_descriptor_init(&txd->tx, chan);
-
 	if (direction != plchan->runtime_direction)
 		dev_err(&pl08x->adev->dev, "%s DMA setup does not match "
 			"the direction configured for the PrimeCell\n",
@@ -1467,11 +1472,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 	txd->cd = plchan->cd;
-	txd->tx.tx_submit = pl08x_tx_submit;
-	txd->tx.callback = NULL;
-	txd->tx.callback_param = NULL;
 	txd->len = sgl->length;
-	INIT_LIST_HEAD(&txd->node);
 
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
Signature · 1 line
1.6.2.5
#18 lore raw

PATCH 18/48 ARM: PL08x: consolidate physical channel release code

In reply to Russell King - ARM Linux

Consolidate duplicated channel release code into release_phy_channel()

---
 drivers/dma/amba-pl08x.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)
drivers/dma/amba-pl08x.c +14 -14
index d47255e..e8967ff 100644
@@ -1058,6 +1058,18 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan,
 	return 0;
 }
 
+static void release_phy_channel(struct pl08x_dma_chan *plchan)
+{
+	struct pl08x_driver_data *pl08x = plchan->host;
+
+	if ((plchan->phychan->signal >= 0) && pl08x->pd->put_signal) {
+		pl08x->pd->put_signal(plchan);
+		plchan->phychan->signal = -1;
+	}
+	pl08x_put_phy_channel(pl08x, plchan->phychan);
+	plchan->phychan = NULL;
+}
+
 static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(tx->chan);
@@ -1522,13 +1534,7 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 			 * Mark physical channel as free and free any slave
 			 * signal
 			 */
-			if ((plchan->phychan->signal >= 0) &&
-			    pl08x->pd->put_signal) {
-				pl08x->pd->put_signal(plchan);
-				plchan->phychan->signal = -1;
-			}
-			pl08x_put_phy_channel(pl08x, plchan->phychan);
-			plchan->phychan = NULL;
+			release_phy_channel(plchan);
 		}
 		/* Dequeue jobs and free LLIs */
 		if (plchan->at) {
@@ -1590,7 +1596,6 @@ static void pl08x_ensure_on(struct pl08x_driver_data *pl08x)
 static void pl08x_tasklet(unsigned long data)
 {
 	struct pl08x_dma_chan *plchan = (struct pl08x_dma_chan *) data;
-	struct pl08x_phy_chan *phychan = plchan->phychan;
 	struct pl08x_driver_data *pl08x = plchan->host;
 	unsigned long flags;
 
@@ -1642,12 +1647,7 @@ static void pl08x_tasklet(unsigned long data)
 		 * No more jobs, so free up the physical channel
 		 * Free any allocated signal on slave transfers too
 		 */
-		if ((phychan->signal >= 0) && pl08x->pd->put_signal) {
-			pl08x->pd->put_signal(plchan);
-			phychan->signal = -1;
-		}
-		pl08x_put_phy_channel(pl08x, phychan);
-		plchan->phychan = NULL;
+		release_phy_channel(plchan);
 		plchan->state = PL08X_CHAN_IDLE;
 
 		/*
Signature · 1 line
1.6.2.5
#19 lore raw

PATCH 19/48 ARM: PL08x: ensure loops use cpu_relax()

In reply to Russell King - ARM Linux

Tight loops should use cpu_relax() to allow CPUs to reduce power consumption while waiting for events.

---
 drivers/dma/amba-pl08x.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/dma/amba-pl08x.c +3 -3
index e8967ff..78c84b3 100644
@@ -191,7 +191,7 @@ static void pl08x_set_cregs(struct pl08x_driver_data *pl08x,
 {
 	/* Wait for channel inactive */
 	while (pl08x_phy_channel_busy(ch))
-		;
+		cpu_relax();
 
 	dev_vdbg(&pl08x->adev->dev,
 		"WRITE channel %d: csrc=0x%08x, cdst=0x%08x, "
@@ -255,7 +255,7 @@ static void pl08x_enable_phy_chan(struct pl08x_driver_data *pl08x,
 	 * Do not access config register until channel shows as disabled
 	 */
 	while (readl(pl08x->base + PL080_EN_CHAN) & (1 << ch->id))
-		;
+		cpu_relax();
 
 	/*
 	 * Do not access config register until channel shows as inactive
@@ -288,7 +288,7 @@ static void pl08x_pause_phy_chan(struct pl08x_phy_chan *ch)
 
 	/* Wait for channel inactive */
 	while (pl08x_phy_channel_busy(ch))
-		;
+		cpu_relax();
 }
 
 static void pl08x_resume_phy_chan(struct pl08x_phy_chan *ch)
Signature · 1 line
1.6.2.5
#20 lore raw

PATCH 20/48 ARM: PL08x: don't assume that the LLI pointer has the bus bit clear

In reply to Russell King - ARM Linux

We only want use the address of the LLI pointer when locating the corresponding structure in memory, so clear the master bus selection bit.

---
 drivers/dma/amba-pl08x.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/dma/amba-pl08x.c +1 -1
index 78c84b3..1081165 100644
@@ -356,7 +356,7 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
 	if (ch && txd) {
 		struct pl08x_lli *llis_va = txd->llis_va;
 		struct pl08x_lli *llis_bus = (struct pl08x_lli *) txd->llis_bus;
-		u32 clli = readl(ch->base + PL080_CH_LLI);
+		u32 clli = readl(ch->base + PL080_CH_LLI) & ~PL080_LLI_LM_AHB2;
 
 		/* First get the bytes in the current active LLI */
 		bytes = get_bytes_in_cctl(readl(ch->base + PL080_CH_CONTROL));
Signature · 1 line
1.6.2.5
#21 lore raw

PATCH 21/48 ARM: PL08x: don't try to use llis_bus as a pointer

In reply to Russell King - ARM Linux

llis_bus is the DMA address of the LLI array. Casting it to be a pointer just to be able to use pointer arithmetic on it is not nice. We can trivially deal with the places where we do arithmetic on it, and it's actually cleaner this way.

---
 drivers/dma/amba-pl08x.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
drivers/dma/amba-pl08x.c +3 -6
index 1081165..760b71e 100644
@@ -559,7 +559,7 @@ static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 			    u32 cctl, u32 *remainder)
 {
 	struct pl08x_lli *llis_va = txd->llis_va;
-	struct pl08x_lli *llis_bus = (struct pl08x_lli *) txd->llis_bus;
+	dma_addr_t llis_bus = txd->llis_bus;
 
 	BUG_ON(num_llis >= MAX_NUM_TSFR_LLIS);
 
@@ -576,8 +576,7 @@ static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 	 * memory. So we don't manipulate this bit currently.
 	 */
 
-	llis_va[num_llis].next =
-		(dma_addr_t)((u32) &(llis_bus[num_llis + 1]));
+	llis_va[num_llis].next = llis_bus + (num_llis + 1) * sizeof(struct pl08x_lli);
 
 	if (cctl & PL080_CONTROL_SRC_INCR)
 		txd->srcbus.addr += len;
@@ -621,7 +620,6 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	int max_bytes_per_lli;
 	int total_bytes = 0;
 	struct pl08x_lli *llis_va;
-	struct pl08x_lli *llis_bus;
 
 	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT,
 				      &txd->llis_bus);
@@ -971,8 +969,7 @@ static void pl08x_free_txd(struct pl08x_driver_data *pl08x,
 			   struct pl08x_txd *txd)
 {
 	/* Free the LLI */
-	dma_pool_free(pl08x->pool, txd->llis_va,
-		      txd->llis_bus);
+	dma_pool_free(pl08x->pool, txd->llis_va, txd->llis_bus);
 
 	pl08x->pool_ctr--;
 
Signature · 1 line
1.6.2.5
#22 lore raw

PATCH 22/48 ARM: PL08x: use 'size_t' for lengths

In reply to Russell King - ARM Linux

Use size_t for variables denoting lengths throughout, and use the 'z' qualifier for printing the value. For safety, add a BUG_ON() in pl08x_fill_lli_for_desc() to catch the remainder potentially becoming negative.

---
 drivers/dma/amba-pl08x.c   |   40 +++++++++++++++++-----------------------
 include/linux/amba/pl08x.h |    4 ++--
 2 files changed, 19 insertions(+), 25 deletions(-)
drivers/dma/amba-pl08x.c +17 -23
index 760b71e..fa78697 100644
@@ -342,7 +342,7 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
 	struct pl08x_txd *txdi = NULL;
 	struct pl08x_txd *txd;
 	unsigned long flags;
-	u32 bytes = 0;
+	size_t bytes = 0;
 
 	spin_lock_irqsave(&plchan->lock, flags);
 
@@ -470,7 +470,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,
-				  u32 tsize)
+				  size_t tsize)
 {
 	u32 retbits = cctl;
 
@@ -583,6 +583,8 @@ static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 	if (cctl & PL080_CONTROL_DST_INCR)
 		txd->dstbus.addr += len;
 
+	BUG_ON(*remainder < len);
+
 	*remainder -= len;
 
 	return num_llis + 1;
@@ -591,7 +593,7 @@ static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 /*
  * Return number of bytes to fill to boundary, or len
  */
-static inline u32 pl08x_pre_boundary(u32 addr, u32 len)
+static inline size_t pl08x_pre_boundary(u32 addr, size_t len)
 {
 	u32 boundary;
 
@@ -614,11 +616,11 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 {
 	struct pl08x_channel_data *cd = txd->cd;
 	struct pl08x_bus_data *mbus, *sbus;
-	u32 remainder;
+	size_t remainder;
 	int num_llis = 0;
 	u32 cctl;
-	int max_bytes_per_lli;
-	int total_bytes = 0;
+	size_t max_bytes_per_lli;
+	size_t total_bytes = 0;
 	struct pl08x_lli *llis_va;
 
 	txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT,
@@ -686,13 +688,13 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	max_bytes_per_lli = min(txd->srcbus.buswidth, txd->dstbus.buswidth) *
 		PL080_CONTROL_TRANSFER_SIZE_MASK;
 	dev_vdbg(&pl08x->adev->dev,
-		 "%s max bytes per lli = %d\n",
+		 "%s max bytes per lli = %zu\n",
 		 __func__, max_bytes_per_lli);
 
 	/* We need to count this down to zero */
 	remainder = txd->len;
 	dev_vdbg(&pl08x->adev->dev,
-		 "%s remainder = %d\n",
+		 "%s remainder = %zu\n",
 		 __func__, remainder);
 
 	/*
@@ -760,9 +762,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		 * width left
 		 */
 		while (remainder > (mbus->buswidth - 1)) {
-			int lli_len, target_len;
-			int tsize;
-			int odd_bytes;
+			size_t lli_len, target_len, tsize, odd_bytes;
 
 			/*
 			 * If enough left try to send max possible,
@@ -805,7 +805,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 			if (lli_len <= 0) {
 				dev_err(&pl08x->adev->dev,
-					"%s lli_len is %d, <= 0\n",
+					"%s lli_len is %zu, <= 0\n",
 						__func__, lli_len);
 				return 0;
 			}
@@ -853,7 +853,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 				if (target_len != lli_len) {
 					dev_vdbg(&pl08x->adev->dev,
-					"%s can't send what we want. Desired 0x%08x, lli of 0x%08x bytes in txd of 0x%08x\n",
+					"%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);
 				}
 
@@ -863,7 +863,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 						       tsize);
 
 				dev_vdbg(&pl08x->adev->dev,
-					"%s fill lli with single lli chunk of size 0x%08x (remainder 0x%08x)\n",
+					"%s fill lli with single lli chunk of size 0x%08zx (remainder 0x%08zx)\n",
 					__func__, lli_len, remainder);
 				num_llis = pl08x_fill_lli_for_desc(pl08x, txd,
 						num_llis, lli_len, cctl,
@@ -882,7 +882,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 						&& (remainder); j++) {
 					cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
 					dev_vdbg(&pl08x->adev->dev,
-						"%s align with boundary, single byte (remain 0x%08x)\n",
+						"%s align with boundary, single byte (remain 0x%08zx)\n",
 						__func__, remainder);
 					num_llis =
 						pl08x_fill_lli_for_desc(pl08x,
@@ -896,16 +896,10 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		/*
 		 * Send any odd bytes
 		 */
-		if (remainder < 0) {
-			dev_err(&pl08x->adev->dev, "%s remainder not fitted 0x%08x bytes\n",
-					__func__, remainder);
-			return 0;
-		}
-
 		while (remainder) {
 			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
 			dev_vdbg(&pl08x->adev->dev,
-				"%s align with boundary, single odd byte (remain %d)\n",
+				"%s align with boundary, single odd byte (remain %zu)\n",
 				__func__, remainder);
 			num_llis = pl08x_fill_lli_for_desc(pl08x, txd, num_llis,
 					1, cctl, &remainder);
@@ -914,7 +908,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	}
 	if (total_bytes != txd->len) {
 		dev_err(&pl08x->adev->dev,
-			"%s size of encoded lli:s don't match total txd, transferred 0x%08x from size 0x%08x\n",
+			"%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;
 	}
include/linux/amba/pl08x.h +2 -2
index 3ecc20f..2c834ed 100644
@@ -77,7 +77,7 @@ struct pl08x_bus_data {
 	dma_addr_t addr;
 	u8 maxwidth;
 	u8 buswidth;
-	u32 fill_bytes;
+	size_t fill_bytes;
 };
 
 /**
@@ -113,7 +113,7 @@ struct pl08x_txd {
 	enum dma_data_direction	direction;
 	struct pl08x_bus_data srcbus;
 	struct pl08x_bus_data dstbus;
-	int len;
+	size_t len;
 	dma_addr_t llis_bus;
 	void *llis_va;
 	struct pl08x_channel_data *cd;
Signature · 1 line
1.6.2.5
#23 lore raw

PATCH 23/48 ARM: PL08x: use 'u32' for LLI structure members, not dma_addr_t

In reply to Russell King - ARM Linux

Use 'u32' for the LLI structure members, which are defined by hardware to be 32-bit. dma_addr_t is much more vague about its actual size.

---
 drivers/dma/amba-pl08x.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
drivers/dma/amba-pl08x.c +5 -4
index fa78697..a389df5 100644
@@ -105,12 +105,13 @@ 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.
+ * start & end do not - their bus bit info is in cctl.  Also note that these
+ * are fixed 32-bit quantities.
  */
 struct pl08x_lli {
-	dma_addr_t src;
-	dma_addr_t dst;
-	dma_addr_t next;
+	u32 src;
+	u32 dst;
+	u32 next;
 	u32 cctl;
 };
 
Signature · 1 line
1.6.2.5
#24 lore raw

PATCH 24/48 ARM: PL08x: rename lli.next to lli.lli

In reply to Russell King - ARM Linux

The LLI pointer in the documentation is placed into the LLI register, so name it LLI rather than 'next'.

---
 drivers/dma/amba-pl08x.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
drivers/dma/amba-pl08x.c +6 -6
index a389df5..a897315 100644
@@ -111,7 +111,7 @@ struct vendor_data {
 struct pl08x_lli {
 	u32 src;
 	u32 dst;
-	u32 next;
+	u32 lli;
 	u32 cctl;
 };
 
@@ -375,7 +375,7 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
 				/*
 				 * A LLI pointer of 0 terminates the LLI list
 				 */
-				clli = llis_va[i].next;
+				clli = llis_va[i].lli;
 				i++;
 			}
 		}
@@ -577,7 +577,7 @@ static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 	 * memory. So we don't manipulate this bit currently.
 	 */
 
-	llis_va[num_llis].next = llis_bus + (num_llis + 1) * sizeof(struct pl08x_lli);
+	llis_va[num_llis].lli = llis_bus + (num_llis + 1) * sizeof(struct pl08x_lli);
 
 	if (cctl & PL080_CONTROL_SRC_INCR)
 		txd->srcbus.addr += len;
@@ -925,7 +925,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	/*
 	 * The final LLI terminates the LLI.
 	 */
-	llis_va[num_llis - 1].next = 0;
+	llis_va[num_llis - 1].lli = 0;
 	/*
 	 * The final LLI element shall also fire an interrupt
 	 */
@@ -934,7 +934,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	/* Now store the channel register values */
 	txd->csrc = llis_va[0].src;
 	txd->cdst = llis_va[0].dst;
-	txd->clli = llis_va[0].next;
+	txd->clli = llis_va[0].lli;
 	txd->cctl = llis_va[0].cctl;
 	/* ccfg will be set at physical channel allocation time */
 
@@ -950,7 +950,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				 llis_va[i].src,
 				 llis_va[i].dst,
 				 llis_va[i].cctl,
-				 llis_va[i].next
+				 llis_va[i].lli
 				);
 		}
 	}
Signature · 1 line
1.6.2.5
#25 lore raw

PATCH 25/48 ARM: PL08x: clean up LLI lookup

In reply to Russell King - ARM Linux

As the LLI list is an array, we can use maths to locate which LLI index we're currently at, and then sum up the remaining LLI entries until we reach the end of the list.

This makes the code much easier to read, and much less susceptible to falling off the end of the array.

---
 drivers/dma/amba-pl08x.c |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)
drivers/dma/amba-pl08x.c +20 -17
index a897315..202c9e2 100644
@@ -340,53 +340,56 @@ static inline u32 get_bytes_in_cctl(u32 cctl)
 static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
 {
 	struct pl08x_phy_chan *ch;
-	struct pl08x_txd *txdi = NULL;
 	struct pl08x_txd *txd;
 	unsigned long flags;
 	size_t bytes = 0;
 
 	spin_lock_irqsave(&plchan->lock, flags);
-
 	ch = plchan->phychan;
 	txd = plchan->at;
 
 	/*
-	 * Next follow the LLIs to get the number of pending bytes in the
-	 * currently active transaction.
+	 * Follow the LLIs to get the number of remaining
+	 * bytes in the currently active transaction.
 	 */
 	if (ch && txd) {
-		struct pl08x_lli *llis_va = txd->llis_va;
-		struct pl08x_lli *llis_bus = (struct pl08x_lli *) txd->llis_bus;
 		u32 clli = readl(ch->base + PL080_CH_LLI) & ~PL080_LLI_LM_AHB2;
 
-		/* First get the bytes in the current active LLI */
+		/* First get the remaining bytes in the active transfer */
 		bytes = get_bytes_in_cctl(readl(ch->base + PL080_CH_CONTROL));
 
 		if (clli) {
-			int i = 0;
+			struct pl08x_lli *llis_va = txd->llis_va;
+			dma_addr_t llis_bus = txd->llis_bus;
+			int index;
 
-			/* Forward to the LLI pointed to by clli */
-			while ((clli != (u32) &(llis_bus[i])) &&
-			       (i < MAX_NUM_TSFR_LLIS))
-				i++;
+			BUG_ON(clli < llis_bus || clli >= llis_bus +
+				sizeof(struct pl08x_lli) * MAX_NUM_TSFR_LLIS);
+
+			/*
+			 * Locate the next LLI - as this is an array,
+			 * it's simple maths to find.
+			 */
+			index = (clli - llis_bus) / sizeof(struct pl08x_lli);
+
+			for (; index < MAX_NUM_TSFR_LLIS; index++) {
+				bytes += get_bytes_in_cctl(llis_va[index].cctl);
 
-			while (clli) {
-				bytes += get_bytes_in_cctl(llis_va[i].cctl);
 				/*
 				 * A LLI pointer of 0 terminates the LLI list
 				 */
-				clli = llis_va[i].lli;
-				i++;
+				if (!llis_va[index].lli)
+					break;
 			}
 		}
 	}
 
 	/* Sum up all queued transactions */
 	if (!list_empty(&plchan->desc_list)) {
+		struct pl08x_txd *txdi;
 		list_for_each_entry(txdi, &plchan->desc_list, node) {
 			bytes += txdi->len;
 		}
-
 	}
 
 	spin_unlock_irqrestore(&plchan->lock, flags);
Signature · 1 line
1.6.2.5
#26 lore raw

PATCH 26/48 ARM: PL08x: combine functions to start DMA into one function

In reply to Russell King - ARM Linux

There is no need for pl08x_config_phychan_for_txd(), pl08x_set_cregs() and pl08x_enable_phy_chan() to be separate - they are always called in sequence. Combine them into one function.

---
 drivers/dma/amba-pl08x.c |   98 ++++++++++++++++++----------------------------
 1 files changed, 38 insertions(+), 60 deletions(-)
drivers/dma/amba-pl08x.c +38 -60
index 202c9e2..c025a4b 100644
@@ -185,37 +185,17 @@ 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
+ * been set when the LLIs were constructed.  Poke them into the hardware
+ * and start the transfer.
  */
-static void pl08x_set_cregs(struct pl08x_driver_data *pl08x,
-			    struct pl08x_phy_chan *ch)
+static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
+	struct pl08x_txd *txd)
 {
-	/* Wait for channel inactive */
-	while (pl08x_phy_channel_busy(ch))
-		cpu_relax();
-
-	dev_vdbg(&pl08x->adev->dev,
-		"WRITE channel %d: csrc=0x%08x, cdst=0x%08x, "
-		 "cctl=0x%08x, clli=0x%08x, ccfg=0x%08x\n",
-		ch->id,
-		ch->csrc,
-		ch->cdst,
-		ch->cctl,
-		ch->clli,
-		ch->ccfg);
-
-	writel(ch->csrc, ch->base + PL080_CH_SRC_ADDR);
-	writel(ch->cdst, ch->base + PL080_CH_DST_ADDR);
-	writel(ch->clli, ch->base + PL080_CH_LLI);
-	writel(ch->cctl, ch->base + PL080_CH_CONTROL);
-	writel(ch->ccfg, ch->base + PL080_CH_CONFIG);
-}
-
-static inline void pl08x_config_phychan_for_txd(struct pl08x_dma_chan *plchan)
-{
-	struct pl08x_channel_data *cd = plchan->cd;
+	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_phy_chan *phychan = plchan->phychan;
-	struct pl08x_txd *txd = plchan->at;
+	u32 val;
+
+	plchan->at = txd;
 
 	/* Copy the basic control register calculated at transfer config */
 	phychan->csrc = txd->csrc;
@@ -224,7 +204,7 @@ static inline void pl08x_config_phychan_for_txd(struct pl08x_dma_chan *plchan)
 	phychan->cctl = txd->cctl;
 
 	/* Assign the signal to the proper control registers */
-	phychan->ccfg = cd->ccfg;
+	phychan->ccfg = plchan->cd->ccfg;
 	phychan->ccfg &= ~PL080_CONFIG_SRC_SEL_MASK;
 	phychan->ccfg &= ~PL080_CONFIG_DST_SEL_MASK;
 	/* If it wasn't set from AMBA, ignore it */
@@ -240,32 +220,38 @@ static inline void pl08x_config_phychan_for_txd(struct pl08x_dma_chan *plchan)
 	phychan->ccfg |= PL080_CONFIG_ERR_IRQ_MASK;
 	/* Always enable terminal interrupts */
 	phychan->ccfg |= PL080_CONFIG_TC_IRQ_MASK;
-}
 
-/*
- * Enable the DMA channel
- * Assumes all other configuration bits have been set
- * as desired before this code is called
- */
-static void pl08x_enable_phy_chan(struct pl08x_driver_data *pl08x,
-				  struct pl08x_phy_chan *ch)
-{
-	u32 val;
+	/* Wait for channel inactive */
+	while (pl08x_phy_channel_busy(phychan))
+		cpu_relax();
 
-	/*
-	 * Do not access config register until channel shows as disabled
-	 */
-	while (readl(pl08x->base + PL080_EN_CHAN) & (1 << ch->id))
+	dev_vdbg(&pl08x->adev->dev,
+		"WRITE channel %d: csrc=0x%08x, cdst=0x%08x, "
+		 "cctl=0x%08x, clli=0x%08x, ccfg=0x%08x\n",
+		phychan->id,
+		phychan->csrc,
+		phychan->cdst,
+		phychan->cctl,
+		phychan->clli,
+		phychan->ccfg);
+
+	writel(phychan->csrc, phychan->base + PL080_CH_SRC_ADDR);
+	writel(phychan->cdst, phychan->base + PL080_CH_DST_ADDR);
+	writel(phychan->clli, phychan->base + PL080_CH_LLI);
+	writel(phychan->cctl, phychan->base + PL080_CH_CONTROL);
+	writel(phychan->ccfg, phychan->base + PL080_CH_CONFIG);
+
+	/* Enable the DMA channel */
+	/* Do not access config register until channel shows as disabled */
+	while (readl(pl08x->base + PL080_EN_CHAN) & (1 << phychan->id))
 		cpu_relax();
 
-	/*
-	 * Do not access config register until channel shows as inactive
-	 */
-	val = readl(ch->base + PL080_CH_CONFIG);
+	/* Do not access config register until channel shows as inactive */
+	val = readl(phychan->base + PL080_CH_CONFIG);
 	while ((val & PL080_CONFIG_ACTIVE) || (val & PL080_CONFIG_ENABLE))
-		val = readl(ch->base + PL080_CH_CONFIG);
+		val = readl(phychan->base + PL080_CH_CONFIG);
 
-	writel(val | PL080_CONFIG_ENABLE, ch->base + PL080_CH_CONFIG);
+	writel(val | PL080_CONFIG_ENABLE, phychan->base + PL080_CH_CONFIG);
 }
 
 /*
@@ -1278,7 +1264,6 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 static void pl08x_issue_pending(struct dma_chan *chan)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
-	struct pl08x_driver_data *pl08x = plchan->host;
 	unsigned long flags;
 
 	spin_lock_irqsave(&plchan->lock, flags);
@@ -1296,13 +1281,9 @@ static void pl08x_issue_pending(struct dma_chan *chan)
 					struct pl08x_txd,
 					node);
 		list_del(&next->node);
-		plchan->at = next;
 		plchan->state = PL08X_CHAN_RUNNING;
 
-		/* Configure the physical channel for the active txd */
-		pl08x_config_phychan_for_txd(plchan);
-		pl08x_set_cregs(pl08x, plchan->phychan);
-		pl08x_enable_phy_chan(pl08x, plchan->phychan);
+		pl08x_start_txd(plchan, next);
 	}
 
 	spin_unlock_irqrestore(&plchan->lock, flags);
@@ -1630,11 +1611,8 @@ static void pl08x_tasklet(unsigned long data)
 					struct pl08x_txd,
 					node);
 		list_del(&next->node);
-		plchan->at = next;
-		/* Configure the physical channel for the next txd */
-		pl08x_config_phychan_for_txd(plchan);
-		pl08x_set_cregs(pl08x, plchan->phychan);
-		pl08x_enable_phy_chan(pl08x, plchan->phychan);
+
+		pl08x_start_txd(plchan, next);
 	} else {
 		struct pl08x_dma_chan *waiting = NULL;
 
Signature · 1 line
1.6.2.5
#27 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

In reply to Russell King - ARM Linux

As we now have all the code accessing the phychan {csrc,cdst,clli,cctl, ccfg} members in one function, there's no point storing the data into the struct. Get rid of the struct members. Re-order the register dump in the dev_dbg() to reflect the order we write the registers to the DMA device.

The txd {csrc,cdst,clli,cctl} values are duplicates of the lli[0] values, so there's no point duplicating these either. Program the DMAC registers directly from the lli[0] values.

---
 drivers/dma/amba-pl08x.c   |   57 ++++++++++++++-----------------------------
 include/linux/amba/pl08x.h |   13 ----------
 2 files changed, 19 insertions(+), 51 deletions(-)
drivers/dma/amba-pl08x.c +19 -38
index c025a4b..a1a18bd 100644
@@ -193,33 +193,25 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 {
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_phy_chan *phychan = plchan->phychan;
-	u32 val;
+	struct pl08x_lli *lli = &txd->llis_va[0];
+	u32 val, ccfg;
 
 	plchan->at = txd;
 
-	/* Copy the basic control register calculated at transfer config */
-	phychan->csrc = txd->csrc;
-	phychan->cdst = txd->cdst;
-	phychan->clli = txd->clli;
-	phychan->cctl = txd->cctl;
-
 	/* Assign the signal to the proper control registers */
-	phychan->ccfg = plchan->cd->ccfg;
-	phychan->ccfg &= ~PL080_CONFIG_SRC_SEL_MASK;
-	phychan->ccfg &= ~PL080_CONFIG_DST_SEL_MASK;
+	ccfg = plchan->cd->ccfg;
+	ccfg &= ~(PL080_CONFIG_SRC_SEL_MASK | PL080_CONFIG_DST_SEL_MASK);
+
 	/* If it wasn't set from AMBA, ignore it */
 	if (txd->direction == DMA_TO_DEVICE)
 		/* Select signal as destination */
-		phychan->ccfg |=
-			(phychan->signal << PL080_CONFIG_DST_SEL_SHIFT);
+		ccfg |= phychan->signal << PL080_CONFIG_DST_SEL_SHIFT;
 	else if (txd->direction == DMA_FROM_DEVICE)
 		/* Select signal as source */
-		phychan->ccfg |=
-			(phychan->signal << PL080_CONFIG_SRC_SEL_SHIFT);
-	/* Always enable error interrupts */
-	phychan->ccfg |= PL080_CONFIG_ERR_IRQ_MASK;
-	/* Always enable terminal interrupts */
-	phychan->ccfg |= PL080_CONFIG_TC_IRQ_MASK;
+		ccfg |= phychan->signal << PL080_CONFIG_SRC_SEL_SHIFT;
+
+	/* Always enable error and terminal interrupts */
+	ccfg |= PL080_CONFIG_ERR_IRQ_MASK | PL080_CONFIG_TC_IRQ_MASK;
 
 	/* Wait for channel inactive */
 	while (pl08x_phy_channel_busy(phychan))
@@ -227,19 +219,15 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 
 	dev_vdbg(&pl08x->adev->dev,
 		"WRITE channel %d: csrc=0x%08x, cdst=0x%08x, "
-		 "cctl=0x%08x, clli=0x%08x, ccfg=0x%08x\n",
-		phychan->id,
-		phychan->csrc,
-		phychan->cdst,
-		phychan->cctl,
-		phychan->clli,
-		phychan->ccfg);
-
-	writel(phychan->csrc, phychan->base + PL080_CH_SRC_ADDR);
-	writel(phychan->cdst, phychan->base + PL080_CH_DST_ADDR);
-	writel(phychan->clli, phychan->base + PL080_CH_LLI);
-	writel(phychan->cctl, phychan->base + PL080_CH_CONTROL);
-	writel(phychan->ccfg, phychan->base + PL080_CH_CONFIG);
+		"clli=0x%08x, cctl=0x%08x, ccfg=0x%08x\n",
+		phychan->id, lli->src, lli->dst, lli->lli, lli->cctl,
+		ccfg);
+
+	writel(lli->src, phychan->base + PL080_CH_SRC_ADDR);
+	writel(lli->dst, phychan->base + PL080_CH_DST_ADDR);
+	writel(lli->lli, phychan->base + PL080_CH_LLI);
+	writel(lli->cctl, phychan->base + PL080_CH_CONTROL);
+	writel(ccfg, phychan->base + PL080_CH_CONFIG);
 
 	/* Enable the DMA channel */
 	/* Do not access config register until channel shows as disabled */
@@ -920,13 +908,6 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	 */
 	llis_va[num_llis - 1].cctl |= PL080_CONTROL_TC_IRQ_EN;
 
-	/* Now store the channel register values */
-	txd->csrc = llis_va[0].src;
-	txd->cdst = llis_va[0].dst;
-	txd->clli = llis_va[0].lli;
-	txd->cctl = llis_va[0].cctl;
-	/* ccfg will be set at physical channel allocation time */
-
 #ifdef VERBOSE_DEBUG
 	{
 		int i;
include/linux/amba/pl08x.h +0 -13
index 2c834ed..29d9745 100644
@@ -95,11 +95,6 @@ struct pl08x_phy_chan {
 	spinlock_t lock;
 	int signal;
 	struct pl08x_dma_chan *serving;
-	u32 csrc;
-	u32 cdst;
-	u32 clli;
-	u32 cctl;
-	u32 ccfg;
 };
 
 /**
@@ -118,14 +113,6 @@ struct pl08x_txd {
 	void *llis_va;
 	struct pl08x_channel_data *cd;
 	bool active;
-	/*
-	 * Settings to be put into the physical channel when we
-	 * trigger this txd
-	 */
-	u32 csrc;
-	u32 cdst;
-	u32 clli;
-	u32 cctl;
 };
 
 /**
Signature · 1 line
1.6.2.5
#28 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 11 lines · Russell King - ARM Linux

As we now have all the code accessing the phychan {csrc,cdst,clli,cctl, ccfg} members in one function, there's no point storing the data into the struct. ?Get rid of the struct members. ?Re-order the register dump in the dev_dbg() to reflect the order we write the registers to the DMA device.

The txd {csrc,cdst,clli,cctl} values are duplicates of the lli[0] values, so there's no point duplicating these either. ?Program the DMAC registers directly from the lli[0] values.

This one caused:
drivers/dma/amba-pl08x.c: In function 'pl08x_start_txd':
drivers/dma/amba-pl08x.c:205: warning: dereferencing 'void *' pointer

So I appended the following:

include/linux/amba/pl08x.h +1 -1
index 933b4ed..5b87b6a 100644
@@ -118,7 +118,7 @@ struct pl08x_txd {
        dma_addr_t dst_addr;
        size_t len;
        dma_addr_t llis_bus;
-       void *llis_va;
+       struct pl08x_lli *llis_va;
        bool active;
        /* Default cctl value for LLIs */
        u32 cctl;

--
Dan

#29 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 12 lines · Dan Williams
Quoted 10 lines · Russell King - ARM Linux

As we now have all the code accessing the phychan {csrc,cdst,clli,cctl, ccfg} members in one function, there's no point storing the data into the struct. ?Get rid of the struct members. ?Re-order the register dump in the dev_dbg() to reflect the order we write the registers to the DMA device.

The txd {csrc,cdst,clli,cctl} values are duplicates of the lli[0] values, so there's no point duplicating these either. ?Program the DMAC registers directly from the lli[0] values.

Quoted 5 lines
Quoted 1 line

This one caused:
drivers/dma/amba-pl08x.c: In function 'pl08x_start_txd':
drivers/dma/amba-pl08x.c:205: warning: dereferencing 'void *' pointer

That probably means you've got something out of order. I have them ordered in my git tree, I'll recheck there.

#30 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Dan Williams
Quoted 10 lines · Russell King - ARM Linux

As we now have all the code accessing the phychan {csrc,cdst,clli,cctl, ccfg} members in one function, there's no point storing the data into the struct. ?Get rid of the struct members. ?Re-order the register dump in the dev_dbg() to reflect the order we write the registers to the DMA device.

The txd {csrc,cdst,clli,cctl} values are duplicates of the lli[0] values, so there's no point duplicating these either. ?Program the DMAC registers directly from the lli[0] values.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

This one caused:
drivers/dma/amba-pl08x.c: In function 'pl08x_start_txd':
drivers/dma/amba-pl08x.c:205: warning: dereferencing 'void *' pointer

That probably means you've got something out of order. ?I have them ordered in my git tree, I'll recheck there.

I rechecked here and I think everything is in order.

The line in question does not get touched in subsequent patches...

drivers/dma/amba-pl08x.c +2 -1
index c025a4b..a1a18bd 100644
@@ -193,33 +193,25 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 {
       struct pl08x_driver_data *pl08x = plchan->host;
       struct pl08x_phy_chan *phychan = plchan->phychan;
-       u32 val;
+       struct pl08x_lli *lli = &txd->llis_va[0];
+       u32 val, ccfg;
 

29/48 is the only other patch after this to touch pl08x_start_txd.

Double checked the sorted by subject mbox order matches the order they are applied in git.

...nothing in the series touches the definition of llis_va in include/linux/amba/pl08x.h.
e8689e63 (Linus Walleij 2010-09-28 15:57:37 +0200 113)
struct pl08x_txd {
e8689e63 (Linus Walleij 2010-09-28 15:57:37 +0200 114)
struct dma_async_tx_descriptor tx;
e8689e63 (Linus Walleij 2010-09-28 15:57:37 +0200 115)
struct list_head node;
e8689e63 (Linus Walleij 2010-09-28 15:57:37 +0200 116)
enum dma_data_direction direction;
d7244e9a (Russell King - ARM Linux 2011-01-03 22:43:35 +0000 117)
dma_addr_t src_addr;
d7244e9a (Russell King - ARM Linux 2011-01-03 22:43:35 +0000 118)
dma_addr_t dst_addr;
cace6585 (Russell King - ARM Linux 2011-01-03 22:37:31 +0000 119)
size_t len;
e8689e63 (Linus Walleij 2010-09-28 15:57:37 +0200 120)
dma_addr_t llis_bus;
e8689e63 (Linus Walleij 2010-09-28 15:57:37 +0200 121)
void *llis_va;
e8689e63 (Linus Walleij 2010-09-28 15:57:37 +0200 122)
bool active;
70b5ed6b (Russell King - ARM Linux 2011-01-03 22:40:13 +0000 123)
/* Default cctl value for LLIs */
70b5ed6b (Russell King - ARM Linux 2011-01-03 22:40:13 +0000 124)
u32 cctl;
4983a04f (Russell King - ARM Linux 2011-01-03 22:39:33 +0000 125) /*
4983a04f (Russell King - ARM Linux 2011-01-03 22:39:33 +0000 126)
* Settings to be put into the physical channe
4983a04f (Russell King - ARM Linux 2011-01-03 22:39:33 +0000 127)
* trigger this txd. Other registers are in l
4983a04f (Russell King - ARM Linux 2011-01-03 22:39:33 +0000 128) */
4983a04f (Russell King - ARM Linux 2011-01-03 22:39:33 +0000 129)
u32 ccfg;
e8689e63 (Linus Walleij 2010-09-28 15:57:37 +0200 130) };

--
Dan

#31 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines · Russell King - ARM Linux

As we now have all the code accessing the phychan {csrc,cdst,clli,cctl, ccfg} members in one function, there's no point storing the data into the struct. Get rid of the struct members. Re-order the register dump in the dev_dbg() to reflect the order we write the registers to the DMA device.

If it's revised, fix one nit, s/dev_dbg/dev_vdbg/ to be exact, too.

Quoted 20 lines
(quoted diff) +3 -13
@@ -227,19 +219,15 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 
  	dev_vdbg(&pl08x->adev->dev,
  		"WRITE channel %d: csrc=0x%08x, cdst=0x%08x, "
-		 "cctl=0x%08x, clli=0x%08x, ccfg=0x%08x\n",
-		phychan->id,
-		phychan->csrc,
-		phychan->cdst,
-		phychan->cctl,
-		phychan->clli,
-		phychan->ccfg);
-
-	writel(phychan->csrc, phychan->base + PL080_CH_SRC_ADDR);
-	writel(phychan->cdst, phychan->base + PL080_CH_DST_ADDR);
-	writel(phychan->clli, phychan->base + PL080_CH_LLI);
-	writel(phychan->cctl, phychan->base + PL080_CH_CONTROL);
-	writel(phychan->ccfg, phychan->base + PL080_CH_CONFIG);
+		"clli=0x%08x, cctl=0x%08x, ccfg=0x%08x\n",
+		phychan->id, lli->src, lli->dst, lli->lli, lli->cctl,
+		ccfg);
#32 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

In reply to Dan Williams
Quoted 17 lines · Dan Williams
Quoted 15 lines · Russell King - ARM Linux
Quoted 14 lines · Dan Williams
Quoted 11 lines · Russell King - ARM Linux

As we now have all the code accessing the phychan {csrc,cdst,clli,cctl, ccfg} members in one function, there's no point storing the data into the struct. ?Get rid of the struct members. ?Re-order the register dump in the dev_dbg() to reflect the order we write the registers to the DMA device.

The txd {csrc,cdst,clli,cctl} values are duplicates of the lli[0] values, so there's no point duplicating these either. ?Program the DMAC registers directly from the lli[0] values.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

This one caused:
drivers/dma/amba-pl08x.c: In function 'pl08x_start_txd':
drivers/dma/amba-pl08x.c:205: warning: dereferencing 'void *' pointer

That probably means you've got something out of order. ?I have them ordered in my git tree, I'll recheck there.

I rechecked here and I think everything is in order.

Okay, something's screwed in the patch set - and I think I see what it is. Patch 16 is missing a change. I'll respin.

#33 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 7 lines · Russell King - ARM Linux
Quoted 4 lines
Quoted 2 lines

That probably means you've got something out of order. ?I have them ordered in my git tree, I'll recheck there.

I rechecked here and I think everything is in order.

Okay, something's screwed in the patch set - and I think I see what it is. Patch 16 is missing a change. ?I'll respin.

Can this be incremental to the previous series instead? I'm looking
to get my 2.6.38 pull request submitted.

--
Dan

#34 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 5 lines · Dan Williams
Quoted 3 lines · Russell King - ARM Linux
Quoted 3 lines
Quoted 2 lines

That probably means you've got something out of order. ?I have them ordered in my git tree, I'll recheck there.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

I rechecked here and I think everything is in order.

Okay, something's screwed in the patch set - and I think I see what it is. Patch 16 is missing a change. ?I'll respin.

Can this be incremental to the previous series instead? I'm looking
to get my 2.6.38 pull request submitted.

In which case I don't know what state your tree in, so...

#35 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 8 lines · Russell King - ARM Linux
Quoted 7 lines · Dan Williams
Quoted 5 lines · Russell King - ARM Linux
Quoted 4 lines
Quoted 2 lines

That probably means you've got something out of order. I have them ordered in my git tree, I'll recheck there.

I rechecked here and I think everything is in order.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Okay, something's screwed in the patch set - and I think I see what it is. Patch 16 is missing a change. I'll respin.

Can this be incremental to the previous series instead? I'm looking
to get my 2.6.38 pull request submitted.

In which case I don't know what state your tree in, so...

Sorry, should have pointed that out, here it is:

git://git.kernel.org/pub/scm/linux/kernel/git/djbw/async_tx.git next

--
Dan

#36 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 12 lines · Dan Williams
Quoted 11 lines · Russell King - ARM Linux
Quoted 10 lines · Dan Williams
Quoted 7 lines · Russell King - ARM Linux
Quoted 4 lines
Quoted 2 lines

That probably means you've got something out of order. I have them ordered in my git tree, I'll recheck there.

I rechecked here and I think everything is in order.

Okay, something's screwed in the patch set - and I think I see what it is. Patch 16 is missing a change. I'll respin.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

Can this be incremental to the previous series instead? I'm looking
to get my 2.6.38 pull request submitted.

In which case I don't know what state your tree in, so...

Sorry, should have pointed that out, here it is:

git://git.kernel.org/pub/scm/linux/kernel/git/djbw/async_tx.git next

Hmm, I found the missing part in a follow-on patch. It should be safe to apply but wasn't perfect so didn't get sent. (I thought it was a little too noisy.)

Subject: [PATCH] ARM: PL08x: cleanup comments

Cleanup the formatting of comments, remove some which don't make sense anymore.

---
 drivers/dma/amba-pl08x.c   |  144 ++++++++++++++++----------------------------
 include/linux/amba/pl08x.h |   45 +++++++-------
 2 files changed, 72 insertions(+), 117 deletions(-)
drivers/dma/amba-pl08x.c +52 -94
index bebc678..7b9c1b6 100644
@@ -19,14 +19,14 @@
  * this program; if not, write to the Free Software Foundation, Inc., 59
  * Temple Place - Suite 330, Boston, MA  02111-1307, USA.
  *
- * The full GNU General Public License is in this distribution in the
- * file called COPYING.
+ * The full GNU General Public License is in this distribution in the file
+ * called COPYING.
  *
  * Documentation: ARM DDI 0196G == PL080
- * Documentation: ARM DDI 0218E	== PL081
+ * Documentation: ARM DDI 0218E == PL081
  *
- * PL080 & PL081 both have 16 sets of DMA signals that can be routed to
- * any channel.
+ * PL080 & PL081 both have 16 sets of DMA signals that can be routed to any
+ * channel.
  *
  * The PL080 has 8 channels available for simultaneous use, and the PL081
  * has only two channels. So on these DMA controllers the number of channels
@@ -91,11 +91,9 @@
 #define DRIVER_NAME	"pl08xdmac"
 
 /**
- * struct vendor_data - vendor-specific config parameters
- * for PL08x derivatives
+ * struct vendor_data - vendor-specific config parameters for PL08x derivatives
  * @channels: the number of channels available in this variant
- * @dualmaster: whether this version supports dual AHB masters
- * or not.
+ * @dualmaster: whether this version supports dual AHB masters or not.
  */
 struct vendor_data {
 	u8 channels;
@@ -241,10 +239,8 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
  *
  * Disabling individual channels could lose data.
  *
- * Disable the peripheral DMA after disabling the DMAC
- * in order to allow the DMAC FIFO to drain, and
- * hence allow the channel to show inactive
- *
+ * Disable the peripheral DMA after disabling the DMAC in order to allow
+ * the DMAC FIFO to drain, and hence allow the channel to show inactive
  */
 static void pl08x_pause_phy_chan(struct pl08x_phy_chan *ch)
 {
@@ -367,6 +363,10 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
 
 /*
  * Allocate a physical channel for a virtual channel
+ *
+ * Try to locate a physical channel to be used for this transfer. If all
+ * are taken return NULL and the requester will have to cope by using
+ * some fallback PIO mode or retrying later.
  */
 static struct pl08x_phy_chan *
 pl08x_get_phy_channel(struct pl08x_driver_data *pl08x,
@@ -376,12 +376,6 @@ pl08x_get_phy_channel(struct pl08x_driver_data *pl08x,
 	unsigned long flags;
 	int i;
 
-	/*
-	 * Try to locate a physical channel to be used for
-	 * this transfer. If all are taken return NULL and
-	 * the requester will have to cope by using some fallback
-	 * PIO mode or retrying later.
-	 */
 	for (i = 0; i < pl08x->vd->channels; i++) {
 		ch = &pl08x->phy_chans[i];
 
@@ -495,9 +489,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 this 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)
@@ -530,8 +524,7 @@ static void pl08x_choose_master_bus(struct pl08x_lli_build_data *bd,
 }
 
 /*
- * Fills in one LLI for a certain transfer descriptor
- * and advance the counter
+ * Fills in one LLI for a certain transfer descriptor and advance the counter
  */
 static void pl08x_fill_lli_for_desc(struct pl08x_lli_build_data *bd,
 	int num_llis, int len, u32 cctl)
@@ -640,15 +633,11 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	 * Choose bus to align to
 	 * - prefers destination bus if both available
 	 * - if fixed address on one bus chooses other
-	 * - modifies cctl to choose an appropriate master
 	 */
 	pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
 
 	if (txd->len < mbus->buswidth) {
-		/*
-		 * Less than a bus width available
-		 * - send as single bytes
-		 */
+		/* 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 "
@@ -659,10 +648,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			total_bytes++;
 		}
 	} else {
-		/*
-		 *  Make one byte LLIs until master bus is aligned
-		 *  - slave will then be aligned also
-		 */
+		/* 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 "
@@ -674,7 +660,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		}
 
 		/*
-		 *  Master now aligned
+		 * Master now aligned
 		 * - if slave is not then we must set its width down
 		 */
 		if (sbus->addr % sbus->buswidth) {
@@ -732,10 +718,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 			if (lli_len == target_len) {
 				/*
-				 * Can send what we wanted
-				 */
-				/*
-				 *  Maintain alignment
+				 * Can send what we wanted.
+				 * Maintain alignment
 				 */
 				lli_len	= (lli_len/mbus->buswidth) *
 							mbus->buswidth;
@@ -743,17 +727,14 @@ 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 may be working to a boundary
-				 *   on the slave bus
-				 *   We need to ensure the master stays aligned
+				 * 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;
-				/*
-				 * - and that we are working in multiples
-				 *   of the bus widths
-				 */
 				lli_len -= odd_bytes;
 
 			}
@@ -793,8 +774,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 			if (odd_bytes) {
 				/*
-				 * Creep past the boundary,
-				 * maintaining master alignment
+				 * Creep past the boundary, maintaining
+				 * master alignment
 				 */
 				int j;
 				for (j = 0; (j < mbus->buswidth)
@@ -837,13 +818,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	}
 
 	llis_va = txd->llis_va;
-	/*
-	 * The final LLI terminates the LLI.
-	 */
+	/* The final LLI terminates the LLI. */
 	llis_va[num_llis - 1].lli = 0;
-	/*
-	 * The final LLI element shall also fire an interrupt
-	 */
+	/* The final LLI element shall also fire an interrupt. */
 	llis_va[num_llis - 1].cctl |= PL080_CONTROL_TC_IRQ_EN;
 
 #ifdef VERBOSE_DEBUG
@@ -891,7 +868,6 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x,
 			list_del(&txdi->node);
 			pl08x_free_txd(pl08x, txdi);
 		}
-
 	}
 }
 
@@ -1020,10 +996,9 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_interrupt(
 }
 
 /*
- * Code accessing dma_async_is_complete() in a tight loop
- * may give problems - could schedule where indicated.
- * If slaves are relying on interrupts to signal completion this
- * function must not be called with interrupts disabled
+ * Code accessing dma_async_is_complete() in a tight loop may give problems.
+ * 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,
@@ -1046,10 +1021,6 @@ pl08x_dma_tx_status(struct dma_chan *chan,
 	}
 
 	/*
-	 * schedule(); could be inserted here
-	 */
-
-	/*
 	 * This cookie not complete yet
 	 */
 	last_used = plchan->chan.cookie;
@@ -1273,11 +1244,10 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 		}
 	} else
 		/*
-		 * Else we're all set, paused and ready to roll,
-		 * status will switch to PL08X_CHAN_RUNNING when
-		 * we call issue_pending(). If there is something
-		 * running on the channel already we don't change
-		 * its state.
+		 * Else we're all set, paused and ready to roll, status
+		 * will switch to PL08X_CHAN_RUNNING when we call
+		 * issue_pending(). If there is something running on the
+		 * channel already we don't change its state.
 		 */
 		if (plchan->state == PL08X_CHAN_IDLE)
 			plchan->state = PL08X_CHAN_PAUSED;
@@ -1528,10 +1498,9 @@ bool pl08x_filter_id(struct dma_chan *chan, void *chan_id)
 
 /*
  * Just check that the device is there and active
- * TODO: turn this bit on/off depending on the number of
- * physical channels actually used, if it is zero... well
- * shut it off. That will save some power. Cut the clock
- * at the same time.
+ * TODO: turn this bit on/off depending on the number of physical channels
+ * actually used, if it is zero... well shut it off. That will save some
+ * power. Cut the clock at the same time.
  */
 static void pl08x_ensure_on(struct pl08x_driver_data *pl08x)
 {
@@ -1579,16 +1548,11 @@ static void pl08x_tasklet(unsigned long data)
 	plchan->at = NULL;
 
 	if (txd) {
-		/*
-		 * Update last completed
-		 */
+		/* Update last completed */
 		plchan->lc = txd->tx.cookie;
 	}
 
-	/*
-	 * If a new descriptor is queued, set it up
-	 * plchan->at is NULL here
-	 */
+	/* If a new descriptor is queued, set it up plchan->at is NULL here */
 	if (!list_empty(&plchan->pend_list)) {
 		struct pl08x_txd *next;
 
@@ -1615,11 +1579,10 @@ static void pl08x_tasklet(unsigned long data)
 		plchan->state = PL08X_CHAN_IDLE;
 
 		/*
-		 * And NOW before anyone else can grab that free:d
-		 * up physical channel, see if there is some memcpy
-		 * pending that seriously needs to start because of
-		 * being stacked up while we were choking the
-		 * physical channels with data.
+		 * And NOW before anyone else can grab that free:d up
+		 * physical channel, see if there is some memcpy pending
+		 * that seriously needs to start because of being stacked
+		 * up while we were choking the physical channels with data.
 		 */
 		list_for_each_entry(waiting, &pl08x->memcpy.channels,
 				    chan.device_node) {
@@ -1670,9 +1633,7 @@ static irqreturn_t pl08x_irq(int irq, void *dev)
 
 	val = readl(pl08x->base + PL080_ERR_STATUS);
 	if (val) {
-		/*
-		 * An error interrupt (on one or more channels)
-		 */
+		/* An error interrupt (on one or more channels) */
 		dev_err(&pl08x->adev->dev,
 			"%s error interrupt, register value 0x%08x\n",
 				__func__, val);
@@ -1696,9 +1657,7 @@ static irqreturn_t pl08x_irq(int irq, void *dev)
 			mask |= (1 << i);
 		}
 	}
-	/*
-	 * Clear only the terminal interrupts on channels we processed
-	 */
+	/* Clear only the terminal interrupts on channels we processed */
 	writel(mask, pl08x->base + PL080_TC_CLEAR);
 
 	return mask ? IRQ_HANDLED : IRQ_NONE;
@@ -1717,6 +1676,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 	int i;
 
 	INIT_LIST_HEAD(&dmadev->channels);
+
 	/*
 	 * Register as many many memcpy as we have physical channels,
 	 * we won't always be able to use all but the code will have
@@ -1950,9 +1910,7 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 	/* Turn on the PL08x */
 	pl08x_ensure_on(pl08x);
 
-	/*
-	 * Attach the interrupt handler
-	 */
+	/* Attach the interrupt handler */
 	writel(0x000000FF, pl08x->base + PL080_ERR_CLEAR);
 	writel(0x000000FF, pl08x->base + PL080_TC_CLEAR);
 
include/linux/amba/pl08x.h +21 -24

101 lines not on this page · raw message

Signature · 1 line
1.6.2.5
Applied
#37 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 11 lines · Russell King - ARM Linux

Subject: [PATCH] ARM: PL08x: cleanup comments

Cleanup the formatting of comments, remove some which don't make sense anymore.

---
?drivers/dma/amba-pl08x.c ? | ?144 ++++++++++++++++---------------------------- ?include/linux/amba/pl08x.h | ? 45 +++++++-------
?2 files changed, 72 insertions(+), 117 deletions(-)

[..]

include/linux/amba/pl08x.h

Quoted 4 lines
include/linux/amba/pl08x.h +0 -0
index 933b4ed..3111385 100644

[..]

Thanks, applied after changing the following hunk...

Quoted 10 lines
include/linux/amba/pl08x.h +1 -2
@@ -118,8 +116,7 @@ struct pl08x_txd {
 ? ? ? ?dma_addr_t dst_addr;
 ? ? ? ?size_t len;
 ? ? ? ?dma_addr_t llis_bus;
- ? ? ? void *llis_va;
- ? ? ? bool active;
+ ? ? ? struct pl08x_lli *llis_va;

? ? ? ?/* Default cctl value for LLIs */
? ? ? ?u32 cctl;
? ? ? ?/*

...to:

@@ -119,7 +117,6 @@ struct pl08x_txd {
        size_t len;
        dma_addr_t llis_bus;
        struct pl08x_lli *llis_va;
-       bool active;
        /* Default cctl value for LLIs */
        u32 cctl;
        /*

--
Dan

#38 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 36 lines · Dan Williams
Quoted 11 lines · Russell King - ARM Linux

Subject: [PATCH] ARM: PL08x: cleanup comments

Cleanup the formatting of comments, remove some which don't make sense anymore.

---
?drivers/dma/amba-pl08x.c ? | ?144 ++++++++++++++++---------------------------- ?include/linux/amba/pl08x.h | ? 45 +++++++-------
?2 files changed, 72 insertions(+), 117 deletions(-)

[..]

include/linux/amba/pl08x.h

Quoted 4 lines
include/linux/amba/pl08x.h +0 -0
index 933b4ed..3111385 100644

[..]

Thanks, applied after changing the following hunk...

Quoted 10 lines
include/linux/amba/pl08x.h +1 -2
@@ -118,8 +116,7 @@ struct pl08x_txd {
 ? ? ? ?dma_addr_t dst_addr;
 ? ? ? ?size_t len;
 ? ? ? ?dma_addr_t llis_bus;
- ? ? ? void *llis_va;
- ? ? ? bool active;
+ ? ? ? struct pl08x_lli *llis_va;

? ? ? ?/* Default cctl value for LLIs */
? ? ? ?u32 cctl;
? ? ? ?/*

...to:

include/linux/amba/pl08x.h struct pl08x_txd { · line 122

Quoted 7 lines
include/linux/amba/pl08x.h +0 -1
@@ -119,7 +117,6 @@ struct pl08x_txd {
        size_t len;
        dma_addr_t llis_bus;
        struct pl08x_lli *llis_va;
-       bool active;
        /* Default cctl value for LLIs */
        u32 cctl;
        /*

Sigh - but that was the original problem which was complained about - llis_va was void pointer not a struct.

So what was the point of asking me for some random patch again?

I'm confused.

#39 lore raw

PATCH 27/48 ARM: PL08x: avoid duplicating registers in txd and phychan structures

Quoted 6 lines · Russell King - ARM Linux

Sigh - but that was the original problem which was complained about - llis_va was void pointer not a struct.

So what was the point of asking me for some random patch again?

I'm confused.

Me too... we did a fire drill to make sure the series was applied correctly and I thought the outcome was that you found one more straggling patch to include.

In any event I think we are all set now.

--
Dan

#40 lore raw

PATCH 28/48 ARM: PL08x: move ccfg into txd structure

In reply to Russell King - ARM Linux

The ccfg register is used to configure the channel parameters - the type and direction of transfer, the flow control signal and IRQ mask enables. The type and direction of transfer is known in the relevent prep_* function where a txd is created. The IRQ mask enables are always set, and the flow control signals are always set when we start processing a txd according to phychan->signal.

If we store the ccfg value in the txd structure, we can avoid modifying platform data - and even having it in platform data at all.

So, remove it from platform data too.

---
 drivers/dma/amba-pl08x.c   |   28 ++++++++++++----------------
 include/linux/amba/pl08x.h |    6 +++++-
 2 files changed, 17 insertions(+), 17 deletions(-)
drivers/dma/amba-pl08x.c +12 -16
index a1a18bd..75f9e2d 100644
@@ -194,15 +194,11 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_phy_chan *phychan = plchan->phychan;
 	struct pl08x_lli *lli = &txd->llis_va[0];
-	u32 val, ccfg;
+	u32 val, ccfg = txd->ccfg;
 
 	plchan->at = txd;
 
-	/* Assign the signal to the proper control registers */
-	ccfg = plchan->cd->ccfg;
-	ccfg &= ~(PL080_CONFIG_SRC_SEL_MASK | PL080_CONFIG_DST_SEL_MASK);
-
-	/* If it wasn't set from AMBA, ignore it */
+	/* Assign the flow control signal to this channel */
 	if (txd->direction == DMA_TO_DEVICE)
 		/* Select signal as destination */
 		ccfg |= phychan->signal << PL080_CONFIG_DST_SEL_SHIFT;
@@ -210,9 +206,6 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 		/* Select signal as source */
 		ccfg |= phychan->signal << PL080_CONFIG_SRC_SEL_SHIFT;
 
-	/* Always enable error and terminal interrupts */
-	ccfg |= PL080_CONFIG_ERR_IRQ_MASK | PL080_CONFIG_TC_IRQ_MASK;
-
 	/* Wait for channel inactive */
 	while (pl08x_phy_channel_busy(phychan))
 		cpu_relax();
@@ -1161,8 +1154,6 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 	enum dma_slave_buswidth addr_width;
 	u32 maxburst;
 	u32 cctl = 0;
-	/* Mask out all except src and dst channel */
-	u32 ccfg = cd->ccfg & 0x000003DEU;
 	int i;
 
 	/* Transfer direction */
@@ -1170,13 +1161,11 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 	if (config->direction == DMA_TO_DEVICE) {
 		plchan->runtime_addr = config->dst_addr;
 		cctl |= PL080_CONTROL_SRC_INCR;
-		ccfg |= PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		addr_width = config->dst_addr_width;
 		maxburst = config->dst_maxburst;
 	} else if (config->direction == DMA_FROM_DEVICE) {
 		plchan->runtime_addr = config->src_addr;
 		cctl |= PL080_CONTROL_DST_INCR;
-		ccfg |= PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		addr_width = config->src_addr_width;
 		maxburst = config->src_maxburst;
 	} else {
@@ -1226,16 +1215,15 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 
 	/* Modify the default channel data to fit PrimeCell request */
 	cd->cctl = cctl;
-	cd->ccfg = ccfg;
 
 	dev_dbg(&pl08x->adev->dev,
 		"configured channel %s (%s) for %s, data width %d, "
-		"maxburst %d words, LE, CCTL=0x%08x, CCFG=0x%08x\n",
+		"maxburst %d words, LE, CCTL=0x%08x\n",
 		dma_chan_name(chan), plchan->name,
 		(config->direction == DMA_FROM_DEVICE) ? "RX" : "TX",
 		addr_width,
 		maxburst,
-		cctl, ccfg);
+		cctl);
 }
 
 /*
@@ -1340,6 +1328,10 @@ static struct pl08x_txd *pl08x_get_txd(struct pl08x_dma_chan *plchan)
 		dma_async_tx_descriptor_init(&txd->tx, &plchan->chan);
 		txd->tx.tx_submit = pl08x_tx_submit;
 		INIT_LIST_HEAD(&txd->node);
+
+		/* Always enable error and terminal interrupts */
+		txd->ccfg = PL080_CONFIG_ERR_IRQ_MASK |
+			    PL080_CONFIG_TC_IRQ_MASK;
 	}
 	return txd;
 }
@@ -1369,6 +1361,8 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 
 	/* Set platform data for m2m */
 	txd->cd = &pl08x->pd->memcpy_channel;
+	txd->ccfg |= PL080_FLOW_MEM2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
+
 	/* Both to be incremented or the code will break */
 	txd->cd->cctl |= PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR;
 	txd->len = len;
@@ -1424,12 +1418,14 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	 */
 	txd->direction = direction;
 	if (direction == DMA_TO_DEVICE) {
+		txd->ccfg |= PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->srcbus.addr = sgl->dma_address;
 		if (plchan->runtime_addr)
 			txd->dstbus.addr = plchan->runtime_addr;
 		else
 			txd->dstbus.addr = plchan->cd->addr;
 	} else if (direction == DMA_FROM_DEVICE) {
+		txd->ccfg |= PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		if (plchan->runtime_addr)
 			txd->srcbus.addr = plchan->runtime_addr;
 		else
include/linux/amba/pl08x.h +5 -1
index 29d9745..8e74cb1 100644
@@ -58,7 +58,6 @@ struct pl08x_channel_data {
 	int max_signal;
 	u32 muxval;
 	u32 cctl;
-	u32 ccfg;
 	dma_addr_t addr;
 	bool circular_buffer;
 	bool single;
@@ -113,6 +112,11 @@ struct pl08x_txd {
 	void *llis_va;
 	struct pl08x_channel_data *cd;
 	bool active;
+	/*
+	 * Settings to be put into the physical channel when we
+	 * trigger this txd.  Other registers are in llis_va[0].
+	 */
+	u32 ccfg;
 };
 
 /**
Signature · 1 line
1.6.2.5
#41 lore raw

PATCH 29/48 ARM: PL08x: assign ccfg DMA request signal in prep_phy_channel()

In reply to Russell King - ARM Linux

There is no need to wait until we start processing a tx descriptor before setting up the DMA request selection in the ccfg register. We know which channel and request will be used in prep_phy_channel(), so setup the ccfg request selection at txd creation time in prep_phy_channel().

---
 drivers/dma/amba-pl08x.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)
drivers/dma/amba-pl08x.c +9 -11
index 75f9e2d..f0a2988 100644
@@ -194,18 +194,10 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_phy_chan *phychan = plchan->phychan;
 	struct pl08x_lli *lli = &txd->llis_va[0];
-	u32 val, ccfg = txd->ccfg;
+	u32 val;
 
 	plchan->at = txd;
 
-	/* Assign the flow control signal to this channel */
-	if (txd->direction == DMA_TO_DEVICE)
-		/* Select signal as destination */
-		ccfg |= phychan->signal << PL080_CONFIG_DST_SEL_SHIFT;
-	else if (txd->direction == DMA_FROM_DEVICE)
-		/* Select signal as source */
-		ccfg |= phychan->signal << PL080_CONFIG_SRC_SEL_SHIFT;
-
 	/* Wait for channel inactive */
 	while (pl08x_phy_channel_busy(phychan))
 		cpu_relax();
@@ -214,13 +206,13 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 		"WRITE channel %d: csrc=0x%08x, cdst=0x%08x, "
 		"clli=0x%08x, cctl=0x%08x, ccfg=0x%08x\n",
 		phychan->id, lli->src, lli->dst, lli->lli, lli->cctl,
-		ccfg);
+		txd->ccfg);
 
 	writel(lli->src, phychan->base + PL080_CH_SRC_ADDR);
 	writel(lli->dst, phychan->base + PL080_CH_DST_ADDR);
 	writel(lli->lli, phychan->base + PL080_CH_LLI);
 	writel(lli->cctl, phychan->base + PL080_CH_CONTROL);
-	writel(ccfg, phychan->base + PL080_CH_CONFIG);
+	writel(txd->ccfg, phychan->base + PL080_CH_CONFIG);
 
 	/* Enable the DMA channel */
 	/* Do not access config register until channel shows as disabled */
@@ -1001,6 +993,12 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan,
 			return -EBUSY;
 		}
 		ch->signal = ret;
+
+		/* Assign the flow control signal to this channel */
+		if (txd->direction == DMA_TO_DEVICE)
+			txd->ccfg |= ch->signal << PL080_CONFIG_DST_SEL_SHIFT;
+		else if (txd->direction == DMA_FROM_DEVICE)
+			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",
Signature · 1 line
1.6.2.5
#42 lore raw

PATCH 30/48 ARM: PL08x: move default cctl into txd structure

In reply to Russell King - ARM Linux

Rather than modifying platform data while preparing a transfer, copy the cctl value into the txd structure and modify the value there.

---
 drivers/dma/amba-pl08x.c   |   21 ++++++---------------
 include/linux/amba/pl08x.h |    3 ++-
 2 files changed, 8 insertions(+), 16 deletions(-)
drivers/dma/amba-pl08x.c +6 -15
index f0a2988..6d224d4 100644
@@ -577,7 +577,6 @@ static inline size_t pl08x_pre_boundary(u32 addr, size_t len)
 static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			      struct pl08x_txd *txd)
 {
-	struct pl08x_channel_data *cd = txd->cd;
 	struct pl08x_bus_data *mbus, *sbus;
 	size_t remainder;
 	int num_llis = 0;
@@ -595,17 +594,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 	pl08x->pool_ctr++;
 
-	/*
-	 * Initialize bus values for this transfer
-	 * from the passed optimal values
-	 */
-	if (!cd) {
-		dev_err(&pl08x->adev->dev, "%s no channel data\n", __func__);
-		return 0;
-	}
-
-	/* Get the default CCTL from the platform data */
-	cctl = cd->cctl;
+	/* Get the default CCTL */
+	cctl = txd->cctl;
 
 	/*
 	 * On the PL080 we have two bus masters and we
@@ -1358,11 +1348,11 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	txd->dstbus.addr = dest;
 
 	/* Set platform data for m2m */
-	txd->cd = &pl08x->pd->memcpy_channel;
 	txd->ccfg |= PL080_FLOW_MEM2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
+	txd->cctl = pl08x->pd->memcpy_channel.cctl;
 
 	/* Both to be incremented or the code will break */
-	txd->cd->cctl |= PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR;
+	txd->cctl |= PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR;
 	txd->len = len;
 
 	ret = pl08x_prep_channel_resources(plchan, txd);
@@ -1415,6 +1405,8 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	 * channel target address dynamically at runtime.
 	 */
 	txd->direction = direction;
+	txd->cctl = plchan->cd->cctl;
+
 	if (direction == DMA_TO_DEVICE) {
 		txd->ccfg |= PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->srcbus.addr = sgl->dma_address;
@@ -1434,7 +1426,6 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 			"%s direction unsupported\n", __func__);
 		return NULL;
 	}
-	txd->cd = plchan->cd;
 	txd->len = sgl->length;
 
 	ret = pl08x_prep_channel_resources(plchan, txd);
include/linux/amba/pl08x.h +2 -1
index 8e74cb1..8d90830 100644
@@ -110,8 +110,9 @@ struct pl08x_txd {
 	size_t len;
 	dma_addr_t llis_bus;
 	void *llis_va;
-	struct pl08x_channel_data *cd;
 	bool active;
+	/* Default cctl value for LLIs */
+	u32 cctl;
 	/*
 	 * Settings to be put into the physical channel when we
 	 * trigger this txd.  Other registers are in llis_va[0].
Signature · 1 line
1.6.2.5
#43 lore raw

PATCH 31/48 ARM: PL08x: move cctl increment and protection setup to prep_slave_sg

In reply to Russell King - ARM Linux

We don't need to initialize the cctl increment and protection values in the runtime_config method - we have all the inforamtion to setup these values in prep_slave_sg(). Move their initialization there.

---
 drivers/dma/amba-pl08x.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
drivers/dma/amba-pl08x.c +8 -7
index 6d224d4..72cc971 100644
@@ -1148,12 +1148,10 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 	plchan->runtime_direction = config->direction;
 	if (config->direction == DMA_TO_DEVICE) {
 		plchan->runtime_addr = config->dst_addr;
-		cctl |= PL080_CONTROL_SRC_INCR;
 		addr_width = config->dst_addr_width;
 		maxburst = config->dst_maxburst;
 	} else if (config->direction == DMA_FROM_DEVICE) {
 		plchan->runtime_addr = config->src_addr;
-		cctl |= PL080_CONTROL_DST_INCR;
 		addr_width = config->src_addr_width;
 		maxburst = config->src_maxburst;
 	} else {
@@ -1197,10 +1195,6 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 		cctl |= burst_sizes[i].reg;
 	}
 
-	/* Access the cell in privileged mode, non-bufferable, non-cacheable */
-	cctl &= ~PL080_CONTROL_PROT_MASK;
-	cctl |= PL080_CONTROL_PROT_SYS;
-
 	/* Modify the default channel data to fit PrimeCell request */
 	cd->cctl = cctl;
 
@@ -1405,10 +1399,16 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	 * channel target address dynamically at runtime.
 	 */
 	txd->direction = direction;
-	txd->cctl = plchan->cd->cctl;
+	txd->cctl = plchan->cd->cctl &
+			~(PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR |
+			  PL080_CONTROL_PROT_MASK);
+
+	/* Access the cell in privileged mode, non-bufferable, non-cacheable */
+	txd->cctl |= PL080_CONTROL_PROT_SYS;
 
 	if (direction == DMA_TO_DEVICE) {
 		txd->ccfg |= PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT;
+		txd->cctl |= PL080_CONTROL_SRC_INCR;
 		txd->srcbus.addr = sgl->dma_address;
 		if (plchan->runtime_addr)
 			txd->dstbus.addr = plchan->runtime_addr;
@@ -1416,6 +1416,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 			txd->dstbus.addr = plchan->cd->addr;
 	} else if (direction == DMA_FROM_DEVICE) {
 		txd->ccfg |= PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
+		txd->cctl |= PL080_CONTROL_DST_INCR;
 		if (plchan->runtime_addr)
 			txd->srcbus.addr = plchan->runtime_addr;
 		else
Signature · 1 line
1.6.2.5
#44 lore raw

PATCH 32/48 ARM: PL08x: move AHB master port selection into prep_* functions

In reply to Russell King - ARM Linux

As we initialize the default cctl value in the prep_* functions along with the increment settings, we don't need to repeat the selection of the AHB ports each time we create a LLI entry. Do this in the prep_* functions once per transfer.

---
 drivers/dma/amba-pl08x.c |   47 ++++++++++++++++++++++-----------------------
 1 files changed, 23 insertions(+), 24 deletions(-)
drivers/dma/amba-pl08x.c +23 -24
index 72cc971..4ee0ab1 100644
@@ -597,26 +597,6 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	/* Get the default CCTL */
 	cctl = txd->cctl;
 
-	/*
-	 * On the PL080 we have two bus masters and we
-	 * should select one for source and one for
-	 * destination. We try to use AHB2 for the
-	 * bus which does not increment (typically the
-	 * peripheral) else we just choose something.
-	 */
-	cctl &= ~(PL080_CONTROL_DST_AHB2 | PL080_CONTROL_SRC_AHB2);
-	if (pl08x->vd->dualmaster) {
-		if (cctl & PL080_CONTROL_SRC_INCR)
-			/* Source increments, use AHB2 for destination */
-			cctl |= PL080_CONTROL_DST_AHB2;
-		else if (cctl & PL080_CONTROL_DST_INCR)
-			/* Destination increments, use AHB2 for source */
-			cctl |= PL080_CONTROL_SRC_AHB2;
-		else
-			/* Just pick something, source AHB1 dest AHB2 */
-			cctl |= PL080_CONTROL_DST_AHB2;
-	}
-
 	/* Find maximum width of the source bus */
 	txd->srcbus.maxwidth =
 		pl08x_get_bytes_for_cctl((cctl & PL080_CONTROL_SWIDTH_MASK) >>
@@ -1340,14 +1320,25 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	txd->direction = DMA_NONE;
 	txd->srcbus.addr = src;
 	txd->dstbus.addr = dest;
+	txd->len = len;
 
 	/* Set platform data for m2m */
 	txd->ccfg |= PL080_FLOW_MEM2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
-	txd->cctl = pl08x->pd->memcpy_channel.cctl;
+	txd->cctl = pl08x->pd->memcpy_channel.cctl &
+			~(PL080_CONTROL_DST_AHB2 | PL080_CONTROL_SRC_AHB2);
 
 	/* Both to be incremented or the code will break */
 	txd->cctl |= PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR;
-	txd->len = len;
+
+	/*
+	 * On the PL080 we have two bus masters and we should select one for
+	 * source and one for destination. We try to use AHB2 for the bus
+	 * which does not increment (typically the peripheral) else we just
+	 * choose something.
+	 */
+	if (pl08x->vd->dualmaster)
+		/* Source increments, use AHB2 for destination */
+		txd->cctl |= PL080_CONTROL_DST_AHB2;
 
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
@@ -1399,8 +1390,11 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	 * channel target address dynamically at runtime.
 	 */
 	txd->direction = direction;
+	txd->len = sgl->length;
+
 	txd->cctl = plchan->cd->cctl &
-			~(PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR |
+			~(PL080_CONTROL_SRC_AHB2 | PL080_CONTROL_DST_AHB2 |
+			  PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR |
 			  PL080_CONTROL_PROT_MASK);
 
 	/* Access the cell in privileged mode, non-bufferable, non-cacheable */
@@ -1409,6 +1403,9 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	if (direction == DMA_TO_DEVICE) {
 		txd->ccfg |= PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->cctl |= PL080_CONTROL_SRC_INCR;
+		if (pl08x->vd->dualmaster)
+			/* Source increments, use AHB2 for destination */
+			txd->cctl |= PL080_CONTROL_DST_AHB2;
 		txd->srcbus.addr = sgl->dma_address;
 		if (plchan->runtime_addr)
 			txd->dstbus.addr = plchan->runtime_addr;
@@ -1417,6 +1414,9 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	} else if (direction == DMA_FROM_DEVICE) {
 		txd->ccfg |= PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->cctl |= PL080_CONTROL_DST_INCR;
+		if (pl08x->vd->dualmaster)
+			/* Destination increments, use AHB2 for source */
+			txd->cctl |= PL080_CONTROL_SRC_AHB2;
 		if (plchan->runtime_addr)
 			txd->srcbus.addr = plchan->runtime_addr;
 		else
@@ -1427,7 +1427,6 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 			"%s direction unsupported\n", __func__);
 		return NULL;
 	}
-	txd->len = sgl->length;
 
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
Signature · 1 line
1.6.2.5
#45 lore raw

PATCH 33/48 ARM: PL08x: allow AHB master port selection to be configured

In reply to Russell King - ARM Linux

Platforms need to be able to control which AHB master interface is used, as each AHB master interface may be asymetric. Allow the interfaces used for fetching LLIs, memory, and each peripheral to be configured individually.

---
 drivers/dma/amba-pl08x.c   |   77 ++++++++++++++++++++++++-------------------
 include/linux/amba/pl08x.h |   15 +++++++-
 2 files changed, 56 insertions(+), 36 deletions(-)
drivers/dma/amba-pl08x.c +43 -34
index 4ee0ab1..986c127 100644
@@ -126,6 +126,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
+ * @mem_buses: set to indicate memory transfers on AHB2.
  * @lock: a spinlock for this struct
  */
 struct pl08x_driver_data {
@@ -138,6 +140,8 @@ struct pl08x_driver_data {
 	struct pl08x_phy_chan *phy_chans;
 	struct dma_pool *pool;
 	int pool_ctr;
+	u8 lli_buses;
+	u8 mem_buses;
 	spinlock_t lock;
 };
 
@@ -526,20 +530,12 @@ static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 
 	BUG_ON(num_llis >= MAX_NUM_TSFR_LLIS);
 
-	llis_va[num_llis].cctl		= cctl;
-	llis_va[num_llis].src		= txd->srcbus.addr;
-	llis_va[num_llis].dst		= txd->dstbus.addr;
-
-	/*
-	 * On versions with dual masters, you can optionally AND on
-	 * PL080_LLI_LM_AHB2 to the LLI to tell the hardware to read
-	 * in new LLIs with that controller, but we always try to
-	 * choose AHB1 to point into memory. The idea is to have AHB2
-	 * fixed on the peripheral and AHB1 messing around in the
-	 * memory. So we don't manipulate this bit currently.
-	 */
-
+	llis_va[num_llis].cctl = cctl;
+	llis_va[num_llis].src = txd->srcbus.addr;
+	llis_va[num_llis].dst = txd->dstbus.addr;
 	llis_va[num_llis].lli = llis_bus + (num_llis + 1) * sizeof(struct pl08x_lli);
+	if (pl08x->lli_buses & PL08X_AHB2)
+		llis_va[num_llis].lli |= PL080_LLI_LM_AHB2;
 
 	if (cctl & PL080_CONTROL_SRC_INCR)
 		txd->srcbus.addr += len;
@@ -639,13 +635,6 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	pl08x_choose_master_bus(&txd->srcbus, &txd->dstbus,
 				&mbus, &sbus, cctl);
 
-
-	/*
-	 * The lowest bit of the LLI register
-	 * is also used to indicate which master to
-	 * use for reading the LLIs.
-	 */
-
 	if (txd->len < mbus->buswidth) {
 		/*
 		 * Less than a bus width available
@@ -1282,6 +1271,23 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 	return 0;
 }
 
+/*
+ * Given the source and destination available bus masks, select which
+ * 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(struct pl08x_driver_data *pl08x, u8 src, u8 dst)
+{
+	u32 cctl = 0;
+
+	if (!(dst & PL08X_AHB1) || ((dst & PL08X_AHB2) && (src & PL08X_AHB1)))
+		cctl |= PL080_CONTROL_DST_AHB2;
+	if (!(src & PL08X_AHB1) || ((src & PL08X_AHB2) && !(dst & PL08X_AHB2)))
+		cctl |= PL080_CONTROL_SRC_AHB2;
+
+	return cctl;
+}
+
 static struct pl08x_txd *pl08x_get_txd(struct pl08x_dma_chan *plchan)
 {
 	struct pl08x_txd *txd = kzalloc(sizeof(struct pl08x_txd), GFP_NOWAIT);
@@ -1330,15 +1336,9 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	/* Both to be incremented or the code will break */
 	txd->cctl |= PL080_CONTROL_SRC_INCR | PL080_CONTROL_DST_INCR;
 
-	/*
-	 * On the PL080 we have two bus masters and we should select one for
-	 * source and one for destination. We try to use AHB2 for the bus
-	 * which does not increment (typically the peripheral) else we just
-	 * choose something.
-	 */
 	if (pl08x->vd->dualmaster)
-		/* Source increments, use AHB2 for destination */
-		txd->cctl |= PL080_CONTROL_DST_AHB2;
+		txd->cctl |= pl08x_select_bus(pl08x,
+					pl08x->mem_buses, pl08x->mem_buses);
 
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
@@ -1359,6 +1359,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;
+	u8 src_buses, dst_buses;
 	int ret;
 
 	/*
@@ -1403,31 +1404,31 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	if (direction == DMA_TO_DEVICE) {
 		txd->ccfg |= PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->cctl |= PL080_CONTROL_SRC_INCR;
-		if (pl08x->vd->dualmaster)
-			/* Source increments, use AHB2 for destination */
-			txd->cctl |= PL080_CONTROL_DST_AHB2;
 		txd->srcbus.addr = sgl->dma_address;
 		if (plchan->runtime_addr)
 			txd->dstbus.addr = plchan->runtime_addr;
 		else
 			txd->dstbus.addr = plchan->cd->addr;
+		src_buses = pl08x->mem_buses;
+		dst_buses = plchan->cd->periph_buses;
 	} else if (direction == DMA_FROM_DEVICE) {
 		txd->ccfg |= PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->cctl |= PL080_CONTROL_DST_INCR;
-		if (pl08x->vd->dualmaster)
-			/* Destination increments, use AHB2 for source */
-			txd->cctl |= PL080_CONTROL_SRC_AHB2;
 		if (plchan->runtime_addr)
 			txd->srcbus.addr = plchan->runtime_addr;
 		else
 			txd->srcbus.addr = plchan->cd->addr;
 		txd->dstbus.addr = sgl->dma_address;
+		src_buses = plchan->cd->periph_buses;
+		dst_buses = pl08x->mem_buses;
 	} else {
 		dev_err(&pl08x->adev->dev,
 			"%s direction unsupported\n", __func__);
 		return NULL;
 	}
 
+	txd->cctl |= pl08x_select_bus(pl08x, src_buses, dst_buses);
+
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
 		return NULL;
@@ -1879,6 +1880,14 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id)
 	pl08x->adev = adev;
 	pl08x->vd = vd;
 
+	/* By default, AHB1 only.  If dualmaster, from platform */
+	pl08x->lli_buses = PL08X_AHB1;
+	pl08x->mem_buses = PL08X_AHB1;
+	if (pl08x->vd->dualmaster) {
+		pl08x->lli_buses = pl08x->pd->lli_buses;
+		pl08x->mem_buses = pl08x->pd->mem_buses;
+	}
+
 	/* A DMA memory pool for LLIs, align on 1-byte boundary */
 	pl08x->pool = dma_pool_create(DRIVER_NAME, &pl08x->adev->dev,
 			PL08X_LLI_TSFR_SIZE, PL08X_ALIGN, 0);
include/linux/amba/pl08x.h +13 -2
index 8d90830..f858651 100644
@@ -25,6 +25,12 @@
 struct pl08x_lli;
 struct pl08x_driver_data;
 
+/* Bitmasks for selecting AHB ports for DMA transfers */
+enum {
+	PL08X_AHB1 = (1 << 0),
+	PL08X_AHB2 = (1 << 1)
+};
+
 /**
  * struct pl08x_channel_data - data structure to pass info between
  * platform and PL08x driver regarding channel configuration
@@ -51,6 +57,8 @@ struct pl08x_driver_data;
  * round round round)
  * @single: the device connected to this channel will request single
  * DMA transfers, not bursts. (Bursts are default.)
+ * @periph_buses: the device connected to this channel is accessible via
+ * these buses (use PL08X_AHB1 | PL08X_AHB2).
  */
 struct pl08x_channel_data {
 	char *bus_id;
@@ -61,6 +69,7 @@ struct pl08x_channel_data {
 	dma_addr_t addr;
 	bool circular_buffer;
 	bool single;
+	u8 periph_buses;
 };
 
 /**
@@ -193,8 +202,8 @@ struct pl08x_dma_chan {
  * less than zero, else it returns the allocated signal number
  * @put_signal: indicate to the platform that this physical signal is not
  * running any DMA transfer and multiplexing can be recycled
- * @bus_bit_lli: Bit[0] of the address indicated which AHB bus master the
- * LLI addresses are on 0/1 Master 1/2.
+ * @lli_buses: buses which LLIs can be fetched from: PL08X_AHB1 | PL08X_AHB2
+ * @mem_buses: buses which memory can be accessed from: PL08X_AHB1 | PL08X_AHB2
  */
 struct pl08x_platform_data {
 	struct pl08x_channel_data *slave_channels;
@@ -202,6 +211,8 @@ struct pl08x_platform_data {
 	struct pl08x_channel_data memcpy_channel;
 	int (*get_signal)(struct pl08x_dma_chan *);
 	void (*put_signal)(struct pl08x_dma_chan *);
+	u8 lli_buses;
+	u8 mem_buses;
 };
 
 #ifdef CONFIG_AMBA_PL08X
Signature · 1 line
1.6.2.5
#46 lore raw

PATCH 34/48 ARM: PL08x: move callback outside spinlock'd region

In reply to Russell King - ARM Linux

Calling the callback handler with spinlocks in the tasklet held leads to deadlock when dmaengine functions are called:

BUG: spinlock lockup on CPU#0, sh/417, c1870a08
Backtrace:

...
[<c017b408>] (do_raw_spin_lock+0x0/0x154) from [<c02c4b98>] (_raw_spin_lock_irqsave+0x54/0x60) [<c02c4b44>] (_raw_spin_lock_irqsave+0x0/0x60) from [<c01f5828>] (pl08x_prep_channel_resources+0x718/0x8b4) [<c01f5110>] (pl08x_prep_channel_resources+0x0/0x8b4) from [<c01f5bb4>] (pl08x_prep_slave_sg+0x120/0x19c) [<c01f5a94>] (pl08x_prep_slave_sg+0x0/0x19c) from [<c01be7a0>] (pl011_dma_tx_refill+0x164/0x224) [<c01be63c>] (pl011_dma_tx_refill+0x0/0x224) from [<c01bf1c8>] (pl011_dma_tx_callback+0x7c/0xc4) [<c01bf14c>] (pl011_dma_tx_callback+0x0/0xc4) from [<c01f4d34>] (pl08x_tasklet+0x60/0x368) [<c01f4cd4>] (pl08x_tasklet+0x0/0x368) from [<c004d978>] (tasklet_action+0xa0/0x100)

Dan quoted the documentation:

Quoted 3 lines

2/ Completion callback routines cannot submit new operations. ?This ? ?results in recursion in the synchronous case and spin_locks being ? ?acquired twice in the asynchronous case.

but then followed up to say:

Quoted 4 lines

I should clarify, this is the async_memcpy() api requirement which is not used outside of md/raid5. DMA drivers can and do allow new submissions from callbacks, and the ones that do so properly move the callback outside of the driver lock.

So let's fix it by moving the callback out of the spinlocked region.

---
 drivers/dma/amba-pl08x.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)
drivers/dma/amba-pl08x.c +15 -14
index 986c127..660165d 100644
@@ -1540,32 +1540,29 @@ static void pl08x_tasklet(unsigned long data)
 {
 	struct pl08x_dma_chan *plchan = (struct pl08x_dma_chan *) data;
 	struct pl08x_driver_data *pl08x = plchan->host;
+	struct pl08x_txd *txd;
+	dma_async_tx_callback callback = NULL;
+	void *callback_param = NULL;
 	unsigned long flags;
 
 	spin_lock_irqsave(&plchan->lock, flags);
 
-	if (plchan->at) {
-		dma_async_tx_callback callback =
-			plchan->at->tx.callback;
-		void *callback_param =
-			plchan->at->tx.callback_param;
+	txd = plchan->at;
+	plchan->at = NULL;
 
-		/*
-		 * Update last completed
-		 */
-		plchan->lc = plchan->at->tx.cookie;
+	if (txd) {
+		callback = txd->tx.callback;
+		callback_param = txd->tx.callback_param;
 
 		/*
-		 * Callback to signal completion
+		 * Update last completed
 		 */
-		if (callback)
-			callback(callback_param);
+		plchan->lc = txd->tx.cookie;
 
 		/*
 		 * Free the descriptor
 		 */
-		pl08x_free_txd(pl08x, plchan->at);
-		plchan->at = NULL;
+		pl08x_free_txd(pl08x, txd);
 	}
 	/*
 	 * If a new descriptor is queued, set it up
@@ -1616,6 +1613,10 @@ static void pl08x_tasklet(unsigned long data)
 	}
 
 	spin_unlock_irqrestore(&plchan->lock, flags);
+
+	/* Callback to signal completion */
+	if (callback)
+		callback(callback_param);
 }
 
 static irqreturn_t pl08x_irq(int irq, void *dev)
Signature · 1 line
1.6.2.5
#47 lore raw

PATCH 35/48 ARM: PL08x: make pl08x_fill_lli_for_desc() return void

In reply to Russell King - ARM Linux

We don't need pl08x_fill_lli_for_desc() to return num_llis + 1 as we know that's what it always does. We can just pass in num_llis and use post-increment in the caller.

This makes the code slightly easier to read.

---
 drivers/dma/amba-pl08x.c |   30 ++++++++++++------------------
 1 files changed, 12 insertions(+), 18 deletions(-)
drivers/dma/amba-pl08x.c +12 -18
index 660165d..f619404 100644
@@ -521,9 +521,8 @@ static void pl08x_choose_master_bus(struct pl08x_bus_data *src_bus,
  * Fills in one LLI for a certain transfer descriptor
  * and advance the counter
  */
-static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
-			    struct pl08x_txd *txd, int num_llis, int len,
-			    u32 cctl, u32 *remainder)
+static void pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
+	struct pl08x_txd *txd, int num_llis, int len, u32 cctl, u32 *remainder)
 {
 	struct pl08x_lli *llis_va = txd->llis_va;
 	dma_addr_t llis_bus = txd->llis_bus;
@@ -545,8 +544,6 @@ static int pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 	BUG_ON(*remainder < len);
 
 	*remainder -= len;
-
-	return num_llis + 1;
 }
 
 /*
@@ -646,8 +643,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				 "less than a bus width (remain 0x%08x)\n",
 				 __func__, remainder);
 			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
-			num_llis =
-				pl08x_fill_lli_for_desc(pl08x, txd, num_llis, 1,
+			pl08x_fill_lli_for_desc(pl08x, txd, num_llis++, 1,
 					cctl, &remainder);
 			total_bytes++;
 		}
@@ -662,8 +658,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				 "(remain 0x%08x)\n",
 				 __func__, remainder);
 			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
-			num_llis = pl08x_fill_lli_for_desc
-				(pl08x, txd, num_llis, 1, cctl, &remainder);
+			pl08x_fill_lli_for_desc(pl08x, txd, num_llis++, 1,
+					cctl, &remainder);
 			total_bytes++;
 		}
 
@@ -787,9 +783,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				dev_vdbg(&pl08x->adev->dev,
 					"%s fill lli with single lli chunk of size 0x%08zx (remainder 0x%08zx)\n",
 					__func__, lli_len, remainder);
-				num_llis = pl08x_fill_lli_for_desc(pl08x, txd,
-						num_llis, lli_len, cctl,
-						&remainder);
+				pl08x_fill_lli_for_desc(pl08x, txd, num_llis++,
+						lli_len, cctl, &remainder);
 				total_bytes += lli_len;
 			}
 
@@ -806,10 +801,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 					dev_vdbg(&pl08x->adev->dev,
 						"%s align with boundary, single byte (remain 0x%08zx)\n",
 						__func__, remainder);
-					num_llis =
-						pl08x_fill_lli_for_desc(pl08x,
-							txd, num_llis, 1,
-							cctl, &remainder);
+					pl08x_fill_lli_for_desc(pl08x, txd,
+							num_llis++, 1, cctl,
+							&remainder);
 					total_bytes++;
 				}
 			}
@@ -823,8 +817,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			dev_vdbg(&pl08x->adev->dev,
 				"%s align with boundary, single odd byte (remain %zu)\n",
 				__func__, remainder);
-			num_llis = pl08x_fill_lli_for_desc(pl08x, txd, num_llis,
-					1, cctl, &remainder);
+			pl08x_fill_lli_for_desc(pl08x, txd, num_llis++, 1,
+					cctl, &remainder);
 			total_bytes++;
 		}
 	}
Signature · 1 line
1.6.2.5
#48 lore raw

PATCH 36/48 ARM: PL08x: ensure pl08x_pre_boundary() works for any value of addr

In reply to Russell King - ARM Linux

pl08x_pre_boundary() was unsafe with addresses towards the top of memory space:

	boundary = ((addr >> PL08X_BOUNDARY_SHIFT) + 1)
			<< PL08X_BOUNDARY_SHIFT;

This can overflow a 32-bit number, producing zero. When it does:

	if (boundary < addr + len)
		return boundary - addr;
	else
		return len;

results in (boundary - addr) returning either a large positive value. Also if addr + len overflows, this calculation also fails.

We can fix this trivially as the only thing we're actually interested in is the value of the least significant PL08X_BOUNDARY_SHIFT bits:

	boundary_len = PL08X_BOUNDARY_SIZE -
		(addr & (PL08X_BOUNDARY_SIZE - 1));

gives us the number of bytes before 'addr' becomes a multiple of PL08X_BOUNDARY_SIZE. We can then just take the min() of the two calculated lengths.

---
 drivers/dma/amba-pl08x.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)
drivers/dma/amba-pl08x.c +5 -9
index f619404..7c327c3 100644
@@ -547,19 +547,15 @@ static void pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
 }
 
 /*
- * Return number of bytes to fill to boundary, or len
+ * 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)
 {
-	u32 boundary;
+	size_t boundary_len = PL08X_BOUNDARY_SIZE -
+			(addr & (PL08X_BOUNDARY_SIZE - 1));
 
-	boundary = ((addr >> PL08X_BOUNDARY_SHIFT) + 1)
-		<< PL08X_BOUNDARY_SHIFT;
-
-	if (boundary < addr + len)
-		return boundary - addr;
-	else
-		return len;
+	return min(boundary_len, len);
 }
 
 /*
Signature · 1 line
1.6.2.5
#49 lore raw

PATCH 37/48 ARM: PL08x: use min() to calculate target_len

In reply to Russell King - ARM Linux
---
 drivers/dma/amba-pl08x.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
drivers/dma/amba-pl08x.c +1 -3
index 7c327c3..91dd6bf 100644
@@ -682,9 +682,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			 * If enough left try to send max possible,
 			 * otherwise try to send the remainder
 			 */
-			target_len = remainder;
-			if (remainder > max_bytes_per_lli)
-				target_len = max_bytes_per_lli;
+			target_len = min(remainder, max_bytes_per_lli);
 
 			/*
 			 * Set bus lengths for incrementing buses
Signature · 1 line
1.6.2.5
#50 lore raw

PATCH 38/48 ARM: PL08x: fix fill_bytes calculation

In reply to Russell King - ARM Linux

The number of bytes we want to fill into any LLI is the minimum of:
- number of bytes remaining in the transfer
- number of bytes we can transfer in a single LLI
- number of bytes we can transfer without overflowing the source boundary
- number of bytes we can transfer without overflowing the destination boundary

The minimum of the first two is already calculated (target_len). We limit the boundary calculations to this number of bytes, which will then give us the number of bytes we can place into this LLI.

---
 drivers/dma/amba-pl08x.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)
drivers/dma/amba-pl08x.c +10 -16
index 91dd6bf..be7fa17 100644
@@ -685,31 +685,25 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			target_len = min(remainder, max_bytes_per_lli);
 
 			/*
-			 * Set bus lengths for incrementing buses
-			 * to number of bytes which fill to next memory
-			 * boundary
+			 * Set bus lengths for incrementing buses to the
+			 * number of bytes which fill to next memory boundary,
+			 * limiting on the target length calculated above.
 			 */
 			if (cctl & PL080_CONTROL_SRC_INCR)
 				txd->srcbus.fill_bytes =
-					pl08x_pre_boundary(
-						txd->srcbus.addr,
-						remainder);
+					pl08x_pre_boundary(txd->srcbus.addr,
+						target_len);
 			else
-				txd->srcbus.fill_bytes =
-					max_bytes_per_lli;
+				txd->srcbus.fill_bytes = target_len;
 
 			if (cctl & PL080_CONTROL_DST_INCR)
 				txd->dstbus.fill_bytes =
-					pl08x_pre_boundary(
-						txd->dstbus.addr,
-						remainder);
+					pl08x_pre_boundary(txd->dstbus.addr,
+						target_len);
 			else
-				txd->dstbus.fill_bytes =
-						max_bytes_per_lli;
+				txd->dstbus.fill_bytes = target_len;
 
-			/*
-			 *  Find the nearest
-			 */
+			/* Find the nearest */
 			lli_len	= min(txd->srcbus.fill_bytes,
 				txd->dstbus.fill_bytes);
 
Signature · 1 line
1.6.2.5
#51 lore raw

PATCH 39/48 ARM: PL08x: don't manipulate txd->srcbus or txd->dstbus during LLI fill

In reply to Russell King - ARM Linux

Don't alter any txd->srcbus or txd->dstbus values while building the LLI list. This allows us to see the original dma_addr_t values passed in via the prep_memcpy() method.

---
 drivers/dma/amba-pl08x.c |  159 ++++++++++++++++++++++++----------------------
 1 files changed, 83 insertions(+), 76 deletions(-)
drivers/dma/amba-pl08x.c +83 -76
index be7fa17..39970c5 100644
@@ -481,38 +481,45 @@ static inline u32 pl08x_cctl_bits(u32 cctl, u8 srcwidth, u8 dstwidth,
 	return retbits;
 }
 
+struct pl08x_lli_build_data {
+	struct pl08x_txd *txd;
+	struct pl08x_driver_data *pl08x;
+	struct pl08x_bus_data srcbus;
+	struct pl08x_bus_data dstbus;
+	size_t remainder;
+};
+
 /*
  * 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
  */
-static void pl08x_choose_master_bus(struct pl08x_bus_data *src_bus,
-	struct pl08x_bus_data *dst_bus, struct pl08x_bus_data **mbus,
-	struct pl08x_bus_data **sbus, u32 cctl)
+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 = src_bus;
-		*sbus = dst_bus;
+		*mbus = &bd->srcbus;
+		*sbus = &bd->dstbus;
 	} else if (!(cctl & PL080_CONTROL_SRC_INCR)) {
-		*mbus = dst_bus;
-		*sbus = src_bus;
+		*mbus = &bd->dstbus;
+		*sbus = &bd->srcbus;
 	} else {
-		if (dst_bus->buswidth == 4) {
-			*mbus = dst_bus;
-			*sbus = src_bus;
-		} else if (src_bus->buswidth == 4) {
-			*mbus = src_bus;
-			*sbus = dst_bus;
-		} else if (dst_bus->buswidth == 2) {
-			*mbus = dst_bus;
-			*sbus = src_bus;
-		} else if (src_bus->buswidth == 2) {
-			*mbus = src_bus;
-			*sbus = dst_bus;
+		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) {
+			*mbus = &bd->dstbus;
+			*sbus = &bd->srcbus;
+		} else if (bd->srcbus.buswidth == 2) {
+			*mbus = &bd->srcbus;
+			*sbus = &bd->dstbus;
 		} else {
-			/* src_bus->buswidth == 1 */
-			*mbus = dst_bus;
-			*sbus = src_bus;
+			/* bd->srcbus.buswidth == 1 */
+			*mbus = &bd->dstbus;
+			*sbus = &bd->srcbus;
 		}
 	}
 }
@@ -521,29 +528,29 @@ static void pl08x_choose_master_bus(struct pl08x_bus_data *src_bus,
  * Fills in one LLI for a certain transfer descriptor
  * and advance the counter
  */
-static void pl08x_fill_lli_for_desc(struct pl08x_driver_data *pl08x,
-	struct pl08x_txd *txd, int num_llis, int len, u32 cctl, u32 *remainder)
+static void pl08x_fill_lli_for_desc(struct pl08x_lli_build_data *bd,
+	int num_llis, int len, u32 cctl)
 {
-	struct pl08x_lli *llis_va = txd->llis_va;
-	dma_addr_t llis_bus = txd->llis_bus;
+	struct pl08x_lli *llis_va = bd->txd->llis_va;
+	dma_addr_t llis_bus = bd->txd->llis_bus;
 
 	BUG_ON(num_llis >= MAX_NUM_TSFR_LLIS);
 
 	llis_va[num_llis].cctl = cctl;
-	llis_va[num_llis].src = txd->srcbus.addr;
-	llis_va[num_llis].dst = txd->dstbus.addr;
+	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);
-	if (pl08x->lli_buses & PL08X_AHB2)
+	if (bd->pl08x->lli_buses & PL08X_AHB2)
 		llis_va[num_llis].lli |= PL080_LLI_LM_AHB2;
 
 	if (cctl & PL080_CONTROL_SRC_INCR)
-		txd->srcbus.addr += len;
+		bd->srcbus.addr += len;
 	if (cctl & PL080_CONTROL_DST_INCR)
-		txd->dstbus.addr += len;
+		bd->dstbus.addr += len;
 
-	BUG_ON(*remainder < len);
+	BUG_ON(bd->remainder < len);
 
-	*remainder -= len;
+	bd->remainder -= len;
 }
 
 /*
@@ -567,7 +574,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			      struct pl08x_txd *txd)
 {
 	struct pl08x_bus_data *mbus, *sbus;
-	size_t remainder;
+	struct pl08x_lli_build_data bd;
 	int num_llis = 0;
 	u32 cctl;
 	size_t max_bytes_per_lli;
@@ -586,38 +593,43 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	/* Get the default CCTL */
 	cctl = txd->cctl;
 
+	bd.txd = txd;
+	bd.pl08x = pl08x;
+	bd.srcbus.addr = txd->srcbus.addr;
+	bd.dstbus.addr = txd->dstbus.addr;
+
 	/* Find maximum width of the source bus */
-	txd->srcbus.maxwidth =
+	bd.srcbus.maxwidth =
 		pl08x_get_bytes_for_cctl((cctl & PL080_CONTROL_SWIDTH_MASK) >>
 				       PL080_CONTROL_SWIDTH_SHIFT);
 
 	/* Find maximum width of the destination bus */
-	txd->dstbus.maxwidth =
+	bd.dstbus.maxwidth =
 		pl08x_get_bytes_for_cctl((cctl & PL080_CONTROL_DWIDTH_MASK) >>
 				       PL080_CONTROL_DWIDTH_SHIFT);
 
 	/* Set up the bus widths to the maximum */
-	txd->srcbus.buswidth = txd->srcbus.maxwidth;
-	txd->dstbus.buswidth = txd->dstbus.maxwidth;
+	bd.srcbus.buswidth = bd.srcbus.maxwidth;
+	bd.dstbus.buswidth = bd.dstbus.maxwidth;
 	dev_vdbg(&pl08x->adev->dev,
 		 "%s source bus is %d bytes wide, dest bus is %d bytes wide\n",
-		 __func__, txd->srcbus.buswidth, txd->dstbus.buswidth);
+		 __func__, bd.srcbus.buswidth, bd.dstbus.buswidth);
 
 
 	/*
 	 * Bytes transferred == tsize * MIN(buswidths), not max(buswidths)
 	 */
-	max_bytes_per_lli = min(txd->srcbus.buswidth, txd->dstbus.buswidth) *
+	max_bytes_per_lli = min(bd.srcbus.buswidth, bd.dstbus.buswidth) *
 		PL080_CONTROL_TRANSFER_SIZE_MASK;
 	dev_vdbg(&pl08x->adev->dev,
 		 "%s max bytes per lli = %zu\n",
 		 __func__, max_bytes_per_lli);
 
 	/* We need to count this down to zero */
-	remainder = txd->len;
+	bd.remainder = txd->len;
 	dev_vdbg(&pl08x->adev->dev,
 		 "%s remainder = %zu\n",
-		 __func__, remainder);
+		 __func__, bd.remainder);
 
 	/*
 	 * Choose bus to align to
@@ -625,22 +637,20 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 	 * - if fixed address on one bus chooses other
 	 * - modifies cctl to choose an appropriate master
 	 */
-	pl08x_choose_master_bus(&txd->srcbus, &txd->dstbus,
-				&mbus, &sbus, cctl);
+	pl08x_choose_master_bus(&bd, &mbus, &sbus, cctl);
 
 	if (txd->len < mbus->buswidth) {
 		/*
 		 * Less than a bus width available
 		 * - send as single bytes
 		 */
-		while (remainder) {
+		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__, remainder);
+				 __func__, bd.remainder);
 			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
-			pl08x_fill_lli_for_desc(pl08x, txd, num_llis++, 1,
-					cctl, &remainder);
+			pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
 			total_bytes++;
 		}
 	} else {
@@ -652,10 +662,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			dev_vdbg(&pl08x->adev->dev,
 				"%s adjustment lli for less than bus width "
 				 "(remain 0x%08x)\n",
-				 __func__, remainder);
+				 __func__, bd.remainder);
 			cctl = pl08x_cctl_bits(cctl, 1, 1, 1);
-			pl08x_fill_lli_for_desc(pl08x, txd, num_llis++, 1,
-					cctl, &remainder);
+			pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
 			total_bytes++;
 		}
 
@@ -675,14 +684,14 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		 * Make largest possible LLIs until less than one bus
 		 * width left
 		 */
-		while (remainder > (mbus->buswidth - 1)) {
+		while (bd.remainder > (mbus->buswidth - 1)) {
 			size_t lli_len, target_len, tsize, odd_bytes;
 
 			/*
 			 * If enough left try to send max possible,
 			 * otherwise try to send the remainder
 			 */
-			target_len = min(remainder, max_bytes_per_lli);
+			target_len = min(bd.remainder, max_bytes_per_lli);
 
 			/*
 			 * Set bus lengths for incrementing buses to the
@@ -690,24 +699,24 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 			 * limiting on the target length calculated above.
 			 */
 			if (cctl & PL080_CONTROL_SRC_INCR)
-				txd->srcbus.fill_bytes =
-					pl08x_pre_boundary(txd->srcbus.addr,
+				bd.srcbus.fill_bytes =
+					pl08x_pre_boundary(bd.srcbus.addr,
 						target_len);
 			else
-				txd->srcbus.fill_bytes = target_len;
+				bd.srcbus.fill_bytes = target_len;
 
 			if (cctl & PL080_CONTROL_DST_INCR)
-				txd->dstbus.fill_bytes =
-					pl08x_pre_boundary(txd->dstbus.addr,
+				bd.dstbus.fill_bytes =
+					pl08x_pre_boundary(bd.dstbus.addr,
 						target_len);
 			else
-				txd->dstbus.fill_bytes = target_len;
+				bd.dstbus.fill_bytes = target_len;
 
 			/* Find the nearest */
-			lli_len	= min(txd->srcbus.fill_bytes,
-				txd->dstbus.fill_bytes);
+			lli_len	= min(bd.srcbus.fill_bytes,
+				      bd.dstbus.fill_bytes);
 
-			BUG_ON(lli_len > remainder);
+			BUG_ON(lli_len > bd.remainder);
 
 			if (lli_len <= 0) {
 				dev_err(&pl08x->adev->dev,
@@ -764,15 +773,15 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				}
 
 				cctl = pl08x_cctl_bits(cctl,
-						       txd->srcbus.buswidth,
-						       txd->dstbus.buswidth,
+						       bd.srcbus.buswidth,
+						       bd.dstbus.buswidth,
 						       tsize);
 
 				dev_vdbg(&pl08x->adev->dev,
 					"%s fill lli with single lli chunk of size 0x%08zx (remainder 0x%08zx)\n",
-					__func__, lli_len, remainder);
-				pl08x_fill_lli_for_desc(pl08x, txd, num_llis++,
-						lli_len, cctl, &remainder);
+					__func__, lli_len, bd.remainder);
+				pl08x_fill_lli_for_desc(&bd, num_llis++,
+					lli_len, cctl);
 				total_bytes += lli_len;
 			}
 
@@ -784,14 +793,13 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 				 */
 				int j;
 				for (j = 0; (j < mbus->buswidth)
-						&& (remainder); j++) {
+						&& (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__, remainder);
-					pl08x_fill_lli_for_desc(pl08x, txd,
-							num_llis++, 1, cctl,
-							&remainder);
+						__func__, bd.remainder);
+					pl08x_fill_lli_for_desc(&bd,
+						num_llis++, 1, cctl);
 					total_bytes++;
 				}
 			}
@@ -800,13 +808,12 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 		/*
 		 * Send any odd bytes
 		 */
-		while (remainder) {
+		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__, remainder);
-			pl08x_fill_lli_for_desc(pl08x, txd, num_llis++, 1,
-					cctl, &remainder);
+				__func__, bd.remainder);
+			pl08x_fill_lli_for_desc(&bd, num_llis++, 1, cctl);
 			total_bytes++;
 		}
 	}
Signature · 1 line
1.6.2.5
#52 lore raw

PATCH 40/48 ARM: PL08x: shrink srcbus/dstbus in txd structure

In reply to Russell King - ARM Linux

We only need to store the dma address.

---
 drivers/dma/amba-pl08x.c   |   20 ++++++++++----------
 include/linux/amba/pl08x.h |    4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)
drivers/dma/amba-pl08x.c +10 -10
index 39970c5..8d8d33c 100644
@@ -595,8 +595,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
 
 	bd.txd = txd;
 	bd.pl08x = pl08x;
-	bd.srcbus.addr = txd->srcbus.addr;
-	bd.dstbus.addr = txd->dstbus.addr;
+	bd.srcbus.addr = txd->src_addr;
+	bd.dstbus.addr = txd->dst_addr;
 
 	/* Find maximum width of the source bus */
 	bd.srcbus.maxwidth =
@@ -1313,8 +1313,8 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	}
 
 	txd->direction = DMA_NONE;
-	txd->srcbus.addr = src;
-	txd->dstbus.addr = dest;
+	txd->src_addr = src;
+	txd->dst_addr = dest;
 	txd->len = len;
 
 	/* Set platform data for m2m */
@@ -1393,21 +1393,21 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	if (direction == DMA_TO_DEVICE) {
 		txd->ccfg |= PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->cctl |= PL080_CONTROL_SRC_INCR;
-		txd->srcbus.addr = sgl->dma_address;
+		txd->src_addr = sgl->dma_address;
 		if (plchan->runtime_addr)
-			txd->dstbus.addr = plchan->runtime_addr;
+			txd->dst_addr = plchan->runtime_addr;
 		else
-			txd->dstbus.addr = plchan->cd->addr;
+			txd->dst_addr = plchan->cd->addr;
 		src_buses = pl08x->mem_buses;
 		dst_buses = plchan->cd->periph_buses;
 	} else if (direction == DMA_FROM_DEVICE) {
 		txd->ccfg |= PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT;
 		txd->cctl |= PL080_CONTROL_DST_INCR;
 		if (plchan->runtime_addr)
-			txd->srcbus.addr = plchan->runtime_addr;
+			txd->src_addr = plchan->runtime_addr;
 		else
-			txd->srcbus.addr = plchan->cd->addr;
-		txd->dstbus.addr = sgl->dma_address;
+			txd->src_addr = plchan->cd->addr;
+		txd->dst_addr = sgl->dma_address;
 		src_buses = plchan->cd->periph_buses;
 		dst_buses = pl08x->mem_buses;
 	} else {
include/linux/amba/pl08x.h +2 -2
index f858651..4e9e718 100644
@@ -114,8 +114,8 @@ struct pl08x_txd {
 	struct dma_async_tx_descriptor tx;
 	struct list_head node;
 	enum dma_data_direction	direction;
-	struct pl08x_bus_data srcbus;
-	struct pl08x_bus_data dstbus;
+	dma_addr_t src_addr;
+	dma_addr_t dst_addr;
 	size_t len;
 	dma_addr_t llis_bus;
 	void *llis_va;
Signature · 1 line
1.6.2.5
#53 lore raw

PATCH 41/48 ARM: PL08x: store prep_* flags in async_tx structure

In reply to Russell King - ARM Linux

Like other DMA engine drivers do, store the passed flags into the async_tx structure, so they can be checked when the operation completes.

---
 drivers/dma/amba-pl08x.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/dma/amba-pl08x.c +5 -3
index 8d8d33c..00058e3 100644
@@ -1277,12 +1277,14 @@ static u32 pl08x_select_bus(struct pl08x_driver_data *pl08x, u8 src, u8 dst)
 	return cctl;
 }
 
-static struct pl08x_txd *pl08x_get_txd(struct pl08x_dma_chan *plchan)
+static struct pl08x_txd *pl08x_get_txd(struct pl08x_dma_chan *plchan,
+	unsigned long flags)
 {
 	struct pl08x_txd *txd = kzalloc(sizeof(struct pl08x_txd), GFP_NOWAIT);
 
 	if (txd) {
 		dma_async_tx_descriptor_init(&txd->tx, &plchan->chan);
+		txd->tx.flags = flags;
 		txd->tx.tx_submit = pl08x_tx_submit;
 		INIT_LIST_HEAD(&txd->node);
 
@@ -1305,7 +1307,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	struct pl08x_txd *txd;
 	int ret;
 
-	txd = pl08x_get_txd(plchan);
+	txd = pl08x_get_txd(plchan, flags);
 	if (!txd) {
 		dev_err(&pl08x->adev->dev,
 			"%s no memory for descriptor\n", __func__);
@@ -1363,7 +1365,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from %s\n",
 		__func__, sgl->length, plchan->name);
 
-	txd = pl08x_get_txd(plchan);
+	txd = pl08x_get_txd(plchan, flags);
 	if (!txd) {
 		dev_err(&pl08x->adev->dev, "%s no txd\n", __func__);
 		return NULL;
Signature · 1 line
1.6.2.5
#54 lore raw

PATCH 42/48 ARM: PL08x: implement unmapping of memcpy buffers

In reply to Russell King - ARM Linux

The DMA engine API requires DMA engine implementations to unmap buffers passed into the non-slave DMA methods unless the relevant completion flag is set. We aren't doing this, so implement this facility.

---
 drivers/dma/amba-pl08x.c |   52 ++++++++++++++++++++++++++++++++++-----------
 1 files changed, 39 insertions(+), 13 deletions(-)
drivers/dma/amba-pl08x.c +39 -13
index 00058e3..fb469de 100644
@@ -1527,13 +1527,33 @@ static void pl08x_ensure_on(struct pl08x_driver_data *pl08x)
 	writel(val, pl08x->base + PL080_CONFIG);
 }
 
+static void pl08x_unmap_buffers(struct pl08x_txd *txd)
+{
+	struct device *dev = txd->tx.chan->device->dev;
+
+	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);
+	}
+	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);
+		else
+			dma_unmap_page(dev, txd->dst_addr, txd->len,
+				DMA_FROM_DEVICE);
+	}
+}
+
 static void pl08x_tasklet(unsigned long data)
 {
 	struct pl08x_dma_chan *plchan = (struct pl08x_dma_chan *) data;
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_txd *txd;
-	dma_async_tx_callback callback = NULL;
-	void *callback_param = NULL;
 	unsigned long flags;
 
 	spin_lock_irqsave(&plchan->lock, flags);
@@ -1542,18 +1562,10 @@ static void pl08x_tasklet(unsigned long data)
 	plchan->at = NULL;
 
 	if (txd) {
-		callback = txd->tx.callback;
-		callback_param = txd->tx.callback_param;
-
 		/*
 		 * Update last completed
 		 */
 		plchan->lc = txd->tx.cookie;
-
-		/*
-		 * Free the descriptor
-		 */
-		pl08x_free_txd(pl08x, txd);
 	}
 	/*
 	 * If a new descriptor is queued, set it up
@@ -1605,9 +1617,23 @@ static void pl08x_tasklet(unsigned long data)
 
 	spin_unlock_irqrestore(&plchan->lock, flags);
 
-	/* Callback to signal completion */
-	if (callback)
-		callback(callback_param);
+	if (txd) {
+		dma_async_tx_callback callback = txd->tx.callback;
+		void *callback_param = txd->tx.callback_param;
+
+		/* Don't try to unmap buffers on slave channels */
+		if (!plchan->slave)
+			pl08x_unmap_buffers(txd);
+
+		/* Free the descriptor */
+		spin_lock_irqsave(&plchan->lock, flags);
+		pl08x_free_txd(pl08x, txd);
+		spin_unlock_irqrestore(&plchan->lock, flags);
+
+		/* Callback to signal completion */
+		if (callback)
+			callback(callback_param);
+	}
 }
 
 static irqreturn_t pl08x_irq(int irq, void *dev)
Signature · 1 line
1.6.2.5
#55 lore raw

PATCH 43/48 ARM: PL08x: rename 'desc_list' as 'pend_list'

In reply to Russell King - ARM Linux

This 'desc_list' is actually a list of pending descriptors, so name it after its function (pending list) rather than what it contains (descriptors).

---
 drivers/dma/amba-pl08x.c   |   20 ++++++++++----------
 include/linux/amba/pl08x.h |    4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)
drivers/dma/amba-pl08x.c +10 -10
index fb469de..433b9e7 100644
@@ -348,9 +348,9 @@ static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
 	}
 
 	/* Sum up all queued transactions */
-	if (!list_empty(&plchan->desc_list)) {
+	if (!list_empty(&plchan->pend_list)) {
 		struct pl08x_txd *txdi;
-		list_for_each_entry(txdi, &plchan->desc_list, node) {
+		list_for_each_entry(txdi, &plchan->pend_list, node) {
 			bytes += txdi->len;
 		}
 	}
@@ -880,9 +880,9 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x,
 	struct pl08x_txd *txdi = NULL;
 	struct pl08x_txd *next;
 
-	if (!list_empty(&plchan->desc_list)) {
+	if (!list_empty(&plchan->pend_list)) {
 		list_for_each_entry_safe(txdi,
-					 next, &plchan->desc_list, node) {
+					 next, &plchan->pend_list, node) {
 			list_del(&txdi->node);
 			pl08x_free_txd(pl08x, txdi);
 		}
@@ -1183,10 +1183,10 @@ static void pl08x_issue_pending(struct dma_chan *chan)
 	}
 
 	/* Take the first element in the queue and execute it */
-	if (!list_empty(&plchan->desc_list)) {
+	if (!list_empty(&plchan->pend_list)) {
 		struct pl08x_txd *next;
 
-		next = list_first_entry(&plchan->desc_list,
+		next = list_first_entry(&plchan->pend_list,
 					struct pl08x_txd,
 					node);
 		list_del(&next->node);
@@ -1213,7 +1213,7 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 
 	spin_lock_irqsave(&plchan->lock, plchan->lockflags);
 
-	list_add_tail(&txd->node, &plchan->desc_list);
+	list_add_tail(&txd->node, &plchan->pend_list);
 
 	/*
 	 * See if we already have a physical channel allocated,
@@ -1571,10 +1571,10 @@ static void pl08x_tasklet(unsigned long data)
 	 * If a new descriptor is queued, set it up
 	 * plchan->at is NULL here
 	 */
-	if (!list_empty(&plchan->desc_list)) {
+	if (!list_empty(&plchan->pend_list)) {
 		struct pl08x_txd *next;
 
-		next = list_first_entry(&plchan->desc_list,
+		next = list_first_entry(&plchan->pend_list,
 					struct pl08x_txd,
 					node);
 		list_del(&next->node);
@@ -1736,7 +1736,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 		chan->lc = 0;
 
 		spin_lock_init(&chan->lock);
-		INIT_LIST_HEAD(&chan->desc_list);
+		INIT_LIST_HEAD(&chan->pend_list);
 		tasklet_init(&chan->tasklet, pl08x_tasklet,
 			     (unsigned long) chan);
 
include/linux/amba/pl08x.h +2 -2
index 4e9e718..08a9024 100644
@@ -158,7 +158,7 @@ enum pl08x_dma_chan_state {
  * @runtime_direction: current direction of this channel according to
  * runtime config
  * @lc: last completed transaction on this channel
- * @desc_list: queued transactions pending on this channel
+ * @pend_list: queued transactions pending on this channel
  * @at: active transaction on this channel
  * @lockflags: sometimes we let a lock last between two function calls,
  * especially prep/submit, and then we need to store the IRQ flags
@@ -179,7 +179,7 @@ struct pl08x_dma_chan {
 	dma_addr_t runtime_addr;
 	enum dma_data_direction	runtime_direction;
 	dma_cookie_t lc;
-	struct list_head desc_list;
+	struct list_head pend_list;
 	struct pl08x_txd *at;
 	unsigned long lockflags;
 	spinlock_t lock;
Signature · 1 line
1.6.2.5
#56 lore raw

PATCH 44/48 ARM: PL08x: put txd's on the pending list in pl08x_tx_submit()

In reply to Russell King - ARM Linux

Don't place TXDs on the pending list when they're prepared - place them on the list when they're ready to be submitted. Also, only place memcpy requests in the wait state when they're submitted and don't have a physical channel associated.

---
 drivers/dma/amba-pl08x.c |   46 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 32 insertions(+), 14 deletions(-)
drivers/dma/amba-pl08x.c +32 -14
index 433b9e7..650e2bb 100644
@@ -173,6 +173,11 @@ static inline struct pl08x_dma_chan *to_pl08x_chan(struct dma_chan *chan)
 	return container_of(chan, struct pl08x_dma_chan, chan);
 }
 
+static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx)
+{
+	return container_of(tx, struct pl08x_txd, tx);
+}
+
 /*
  * Physical channel handling
  */
@@ -974,11 +979,27 @@ static void release_phy_channel(struct pl08x_dma_chan *plchan)
 static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(tx->chan);
+	struct pl08x_txd *txd = to_pl08x_txd(tx);
 
 	plchan->chan.cookie += 1;
 	if (plchan->chan.cookie < 0)
 		plchan->chan.cookie = 1;
 	tx->cookie = plchan->chan.cookie;
+
+	/* Put this onto the pending list */
+	list_add_tail(&txd->node, &plchan->pend_list);
+
+	/*
+	 * If there was no physical channel available for this memcpy,
+	 * stack the request up and indicate that the channel is waiting
+	 * for a free physical channel.
+	 */
+	if (!plchan->slave && !plchan->phychan) {
+		/* Do this memcpy whenever there is a channel ready */
+		plchan->state = PL08X_CHAN_WAITING;
+		plchan->waiting = txd;
+	}
+
 	/* This unlock follows the lock in the prep() function */
 	spin_unlock_irqrestore(&plchan->lock, plchan->lockflags);
 
@@ -1213,8 +1234,6 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 
 	spin_lock_irqsave(&plchan->lock, plchan->lockflags);
 
-	list_add_tail(&txd->node, &plchan->pend_list);
-
 	/*
 	 * See if we already have a physical channel allocated,
 	 * else this is the time to try to get one.
@@ -1222,24 +1241,23 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 	ret = prep_phy_channel(plchan, txd);
 	if (ret) {
 		/*
-		 * No physical channel available, we will
-		 * stack up the memcpy channels until there is a channel
-		 * available to handle it whereas slave transfers may
-		 * have been denied due to platform channel muxing restrictions
-		 * and since there is no guarantee that this will ever be
-		 * resolved, and since the signal must be acquired AFTER
-		 * acquiring the physical channel, we will let them be NACK:ed
-		 * with -EBUSY here. The drivers can alway retry the prep()
-		 * call if they are eager on doing this using DMA.
+		 * No physical channel was available.
+		 *
+		 * 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
+		 * 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
+		 * the prep() call if they are eager on doing this using DMA.
 		 */
 		if (plchan->slave) {
 			pl08x_free_txd_list(pl08x, plchan);
+			pl08x_free_txd(pl08x, txd);
 			spin_unlock_irqrestore(&plchan->lock, plchan->lockflags);
 			return -EBUSY;
 		}
-		/* Do this memcpy whenever there is a channel ready */
-		plchan->state = PL08X_CHAN_WAITING;
-		plchan->waiting = txd;
 	} else
 		/*
 		 * Else we're all set, paused and ready to roll,
Signature · 1 line
1.6.2.5
#57 lore raw

PATCH 45/48 ARM: PL08x: introduce 'phychan_hold' to hold on to physical channels

In reply to Russell King - ARM Linux

Introduce 'phychan_hold' to hold on to physical DMA channels while we're preparing a new descriptor for it. This will be incremented when we allocate a physical channel and set the MUX registers during the preparation of the TXD, and will only be decremented when the TXD is submitted.

This prevents the physical channel being given up before the new TXD is placed on the queue.

---
 drivers/dma/amba-pl08x.c   |   11 +++++++++++
 include/linux/amba/pl08x.h |    3 +++
 2 files changed, 14 insertions(+), 0 deletions(-)
drivers/dma/amba-pl08x.c +11 -0
index 650e2bb..bf6f7d0 100644
@@ -959,6 +959,7 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan,
 		 ch->signal,
 		 plchan->name);
 
+	plchan->phychan_hold++;
 	plchan->phychan = ch;
 
 	return 0;
@@ -998,6 +999,8 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
 		/* Do this memcpy whenever there is a channel ready */
 		plchan->state = PL08X_CHAN_WAITING;
 		plchan->waiting = txd;
+	} else {
+		plchan->phychan_hold--;
 	}
 
 	/* This unlock follows the lock in the prep() function */
@@ -1585,6 +1588,7 @@ static void pl08x_tasklet(unsigned long data)
 		 */
 		plchan->lc = txd->tx.cookie;
 	}
+
 	/*
 	 * If a new descriptor is queued, set it up
 	 * plchan->at is NULL here
@@ -1598,6 +1602,12 @@ static void pl08x_tasklet(unsigned long data)
 		list_del(&next->node);
 
 		pl08x_start_txd(plchan, next);
+	} 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
+		 * physical channel.
+		 */
 	} else {
 		struct pl08x_dma_chan *waiting = NULL;
 
@@ -1625,6 +1635,7 @@ static void pl08x_tasklet(unsigned long data)
 				ret = prep_phy_channel(waiting,
 						       waiting->waiting);
 				BUG_ON(ret);
+				waiting->phychan_hold--;
 				waiting->state = PL08X_CHAN_RUNNING;
 				waiting->waiting = NULL;
 				pl08x_issue_pending(&waiting->chan);
include/linux/amba/pl08x.h +3 -0
index 08a9024..95b76ea 100644
@@ -151,6 +151,8 @@ enum pl08x_dma_chan_state {
  * struct pl08x_dma_chan - this structure wraps a DMA ENGINE channel
  * @chan: wrappped abstract channel
  * @phychan: the physical channel utilized by this channel, if there is one
+ * @phychan_hold: if non-zero, hold on to the physical channel even if we
+ *  have no pending entries
  * @tasklet: tasklet scheduled by the IRQ to handle actual work etc
  * @name: name of channel
  * @cd: channel platform data
@@ -173,6 +175,7 @@ enum pl08x_dma_chan_state {
 struct pl08x_dma_chan {
 	struct dma_chan chan;
 	struct pl08x_phy_chan *phychan;
+	int phychan_hold;
 	struct tasklet_struct tasklet;
 	char *name;
 	struct pl08x_channel_data *cd;
Signature · 1 line
1.6.2.5
#58 lore raw

PATCH 46/48 ARM: PL08x: fix locking between prepare function and submit function

In reply to Russell King - ARM Linux

The PL08x driver holds on to the channel lock with interrupts disabled between the prepare and the subsequent submit API functions. This means that the locking state when the prepare function returns is dependent on whether it suceeeds or not.

It did this to ensure that the physical channel wasn't released, and as it used to add the descriptor onto the pending list at prepare time rather than submit time.

Now that we have reorganized the code to remove those reasons, we can now safely release the spinlock at the end of preparation and reacquire it in our submit function.

---
 drivers/dma/amba-pl08x.c   |   28 +++++++++-------------------
 include/linux/amba/pl08x.h |    4 ----
 2 files changed, 9 insertions(+), 23 deletions(-)
drivers/dma/amba-pl08x.c +9 -19
index bf6f7d0..1c9f712 100644
@@ -981,6 +981,9 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(tx->chan);
 	struct pl08x_txd *txd = to_pl08x_txd(tx);
+	unsigned long flags;
+
+	spin_lock_irqsave(&plchan->lock, flags);
 
 	plchan->chan.cookie += 1;
 	if (plchan->chan.cookie < 0)
@@ -1003,8 +1006,7 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
 		plchan->phychan_hold--;
 	}
 
-	/* This unlock follows the lock in the prep() function */
-	spin_unlock_irqrestore(&plchan->lock, plchan->lockflags);
+	spin_unlock_irqrestore(&plchan->lock, flags);
 
 	return tx->cookie;
 }
@@ -1225,9 +1227,9 @@ static void pl08x_issue_pending(struct dma_chan *chan)
 static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 					struct pl08x_txd *txd)
 {
-	int num_llis;
 	struct pl08x_driver_data *pl08x = plchan->host;
-	int ret;
+	unsigned long flags;
+	int num_llis, ret;
 
 	num_llis = pl08x_fill_llis_for_desc(pl08x, txd);
 	if (!num_llis) {
@@ -1235,7 +1237,7 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 		return -EINVAL;
 	}
 
-	spin_lock_irqsave(&plchan->lock, plchan->lockflags);
+	spin_lock_irqsave(&plchan->lock, flags);
 
 	/*
 	 * See if we already have a physical channel allocated,
@@ -1258,7 +1260,7 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 		if (plchan->slave) {
 			pl08x_free_txd_list(pl08x, plchan);
 			pl08x_free_txd(pl08x, txd);
-			spin_unlock_irqrestore(&plchan->lock, plchan->lockflags);
+			spin_unlock_irqrestore(&plchan->lock, flags);
 			return -EBUSY;
 		}
 	} else
@@ -1272,11 +1274,7 @@ static int pl08x_prep_channel_resources(struct pl08x_dma_chan *plchan,
 		if (plchan->state == PL08X_CHAN_IDLE)
 			plchan->state = PL08X_CHAN_PAUSED;
 
-	/*
-	 * Notice that we leave plchan->lock locked on purpose:
-	 * it will be unlocked in the subsequent tx_submit()
-	 * call. This is a consequence of the current API.
-	 */
+	spin_unlock_irqrestore(&plchan->lock, flags);
 
 	return 0;
 }
@@ -1355,10 +1353,6 @@ static struct dma_async_tx_descriptor *pl08x_prep_dma_memcpy(
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
 		return NULL;
-	/*
-	 * NB: the channel lock is held at this point so tx_submit()
-	 * must be called in direct succession.
-	 */
 
 	return &txd->tx;
 }
@@ -1444,10 +1438,6 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 	ret = pl08x_prep_channel_resources(plchan, txd);
 	if (ret)
 		return NULL;
-	/*
-	 * NB: the channel lock is held at this point so tx_submit()
-	 * must be called in direct succession.
-	 */
 
 	return &txd->tx;
 }
include/linux/amba/pl08x.h +0 -4
index 95b76ea..933b4ed 100644
@@ -162,9 +162,6 @@ enum pl08x_dma_chan_state {
  * @lc: last completed transaction on this channel
  * @pend_list: queued transactions pending on this channel
  * @at: active transaction on this channel
- * @lockflags: sometimes we let a lock last between two function calls,
- * especially prep/submit, and then we need to store the IRQ flags
- * in the channel state, here
  * @lock: a lock for this channel data
  * @host: a pointer to the host (internal use)
  * @state: whether the channel is idle, paused, running etc
@@ -184,7 +181,6 @@ struct pl08x_dma_chan {
 	dma_cookie_t lc;
 	struct list_head pend_list;
 	struct pl08x_txd *at;
-	unsigned long lockflags;
 	spinlock_t lock;
 	struct pl08x_driver_data *host;
 	enum pl08x_dma_chan_state state;
Signature · 1 line
1.6.2.5
#59 lore raw

PATCH 47/48 ARM: PL08x: allow dma_set_runtime_config() to return errors

In reply to Russell King - ARM Linux

There are cases in dma_set_runtime_config() where we fail to perform the requested action - and we just issue a KERN_ERR message in that case. We have the facility to return an error to the caller, so that is what we should do.

When we encounter an error due to invalid parameters, we should not modify driver state.

---
 drivers/dma/amba-pl08x.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)
drivers/dma/amba-pl08x.c +13 -10
index 1c9f712..c7f7b82 100644
@@ -1117,13 +1117,14 @@ static const struct burst_table burst_sizes[] = {
 	},
 };
 
-static void dma_set_runtime_config(struct dma_chan *chan,
-			       struct dma_slave_config *config)
+static int dma_set_runtime_config(struct dma_chan *chan,
+				  struct dma_slave_config *config)
 {
 	struct pl08x_dma_chan *plchan = to_pl08x_chan(chan);
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_channel_data *cd = plchan->cd;
 	enum dma_slave_buswidth addr_width;
+	dma_addr_t addr;
 	u32 maxburst;
 	u32 cctl = 0;
 	int i;
@@ -1131,17 +1132,17 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 	/* Transfer direction */
 	plchan->runtime_direction = config->direction;
 	if (config->direction == DMA_TO_DEVICE) {
-		plchan->runtime_addr = config->dst_addr;
+		addr = config->dst_addr;
 		addr_width = config->dst_addr_width;
 		maxburst = config->dst_maxburst;
 	} else if (config->direction == DMA_FROM_DEVICE) {
-		plchan->runtime_addr = config->src_addr;
+		addr = config->src_addr;
 		addr_width = config->src_addr_width;
 		maxburst = config->src_maxburst;
 	} else {
 		dev_err(&pl08x->adev->dev,
 			"bad runtime_config: alien transfer direction\n");
-		return;
+		return -EINVAL;
 	}
 
 	switch (addr_width) {
@@ -1160,7 +1161,7 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 	default:
 		dev_err(&pl08x->adev->dev,
 			"bad runtime_config: alien address width\n");
-		return;
+		return -EINVAL;
 	}
 
 	/*
@@ -1179,6 +1180,8 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 		cctl |= burst_sizes[i].reg;
 	}
 
+	plchan->runtime_addr = addr;
+
 	/* Modify the default channel data to fit PrimeCell request */
 	cd->cctl = cctl;
 
@@ -1190,6 +1193,8 @@ static void dma_set_runtime_config(struct dma_chan *chan,
 		addr_width,
 		maxburst,
 		cctl);
+
+	return 0;
 }
 
 /*
@@ -1452,10 +1457,8 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 
 	/* Controls applicable to inactive channels */
 	if (cmd == DMA_SLAVE_CONFIG) {
-		dma_set_runtime_config(chan,
-				       (struct dma_slave_config *)
-				       arg);
-		return 0;
+		return dma_set_runtime_config(chan,
+					      (struct dma_slave_config *)arg);
 	}
 
 	/*
Signature · 1 line
1.6.2.5
#60 lore raw

PATCH 48/48 ARM: PL08x: prevent dma_set_runtime_config() reconfiguring memcpy channels

In reply to Russell King - ARM Linux

Prevent dma_set_runtime_config() being used to alter the configuration supplied by the platform for memcpy channel configuration. No one should be trying to change this configuration.

---
 drivers/dma/amba-pl08x.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/dma/amba-pl08x.c +3 -0
index c7f7b82..bebc678 100644
@@ -1129,6 +1129,9 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 	u32 cctl = 0;
 	int i;
 
+	if (!plchan->slave)
+		return -EINVAL;
+
 	/* Transfer direction */
 	plchan->runtime_direction = config->direction;
 	if (config->direction == DMA_TO_DEVICE) {
Signature · 1 line
1.6.2.5