dma: edma: Provide granular residue accounting

The residue reporting in edma_tx_status() is just broken. It blindly walks the psets and recalculates the lenght of the transfer from the hardware parameters. For cyclic transfers it adds the link pset, which results in interestingly large residues. For non-cyclic it adds the dummy pset, which is stupid as well.
Aside of that it's silly to walk through the pset params when the per descriptor residue is known at the point of creating it.

Store the information in edma_desc and use it.

Signed-off-by Thomas Gleixner <[email protected]>

---
 drivers/dma/edma.c |   34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

Index: linux-2.6/drivers/dma/edma.c
===================================================================

drivers/dma/edma.c +5 -22

@@ -64,6 +64,7 @@ struct edma_desc {
 	int				absync;
 	int				pset_nr;
 	int				processed;
+	u32				residue;
 	struct edmacc_param		pset[0];
 };
 

@@ -416,6 +417,7 @@ static struct dma_async_tx_descriptor *e
 	}
 
 	edesc->pset_nr = sg_len;
+	edesc->residue = 0;
 
 	/* Allocate a PaRAM slot, if needed */
 	nslots = min_t(unsigned, MAX_NR_SG, sg_len);

@@ -450,6 +452,7 @@ static struct dma_async_tx_descriptor *e
 		}
 
 		edesc->absync = ret;
+		edesc->residue += sg_dma_len(sg);
 
 		/* If this is the last in a current SG set of transactions,
 		   enable interrupts so that next set is processed */

@@ -527,6 +530,7 @@ static struct dma_async_tx_descriptor *e
 
 	edesc->cyclic = 1;
 	edesc->pset_nr = nslots;
+	edesc->residue = buf_len;
 
 	dev_dbg(dev, "%s: nslots=%d\n", __func__, nslots);
 	dev_dbg(dev, "%s: period_len=%d\n", __func__, period_len);

@@ -622,6 +626,7 @@ static void edma_callback(unsigned ch_nu
 				vchan_cyclic_callback(&edesc->vdesc);
 			} else if (edesc->processed == edesc->pset_nr) {
 				dev_dbg(dev, "Transfer complete, stopping channel %d\n", ch_num);
+				edesc->residue = 0;
 				edma_stop(echan->ch_num);
 				vchan_cookie_complete(&edesc->vdesc);
 				edma_execute(echan);

@@ -754,25 +759,6 @@ static void edma_issue_pending(struct dm
 	spin_unlock_irqrestore(&echan->vchan.lock, flags);
 }
 
-static size_t edma_desc_size(struct edma_desc *edesc)
-{
-	int i;
-	size_t size;
-
-	if (edesc->absync)
-		for (size = i = 0; i < edesc->pset_nr; i++)
-			size += (edesc->pset[i].a_b_cnt & 0xffff) *
-				(edesc->pset[i].a_b_cnt >> 16) *
-				 edesc->pset[i].ccnt;
-	else
-		size = (edesc->pset[0].a_b_cnt & 0xffff) *
-			(edesc->pset[0].a_b_cnt >> 16) +
-			(edesc->pset[0].a_b_cnt & 0xffff) *
-			(SZ_64K - 1) * edesc->pset[0].ccnt;
-
-	return size;
-}
-
 /* Check request completion status */
 static enum dma_status edma_tx_status(struct dma_chan *chan,
 				      dma_cookie_t cookie,

@@ -789,12 +775,10 @@ static enum dma_status edma_tx_status(st
 
 	spin_lock_irqsave(&echan->vchan.lock, flags);
 	vdesc = vchan_find_desc(&echan->vchan, cookie);
-	if (vdesc) {
-		txstate->residue = edma_desc_size(to_edma_desc(&vdesc->tx));
-	} else if (echan->edesc && echan->edesc->vdesc.tx.cookie == cookie) {

Preparatory patch to support finer grained accounting.

Move the edma_params array out of edma_desc so we can add further per pset data to it.

Signed-off-by Thomas Gleixner <[email protected]>

---
 drivers/dma/edma.c |   67 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

Index: linux-2.6/drivers/dma/edma.c
===================================================================

drivers/dma/edma.c +34 -31

+};
+
 struct edma_desc {
 	struct virt_dma_desc		vdesc;
 	struct list_head		node;

@@ -65,7 +69,7 @@ struct edma_desc {
 	int				pset_nr;
 	int				processed;
 	u32				residue;
-	struct edmacc_param		pset[0];
+	struct edma_pset		pset[0];
 };
 
 struct edma_cc;

@@ -141,7 +145,7 @@ static void edma_execute(struct edma_cha
 	/* Write descriptor PaRAM set(s) */
 	for (i = 0; i < nslots; i++) {
 		j = i + edesc->processed;
-		edma_write_slot(echan->slot[i], &edesc->pset[j]);
+		edma_write_slot(echan->slot[i], &edesc->pset[j].hwpar);
 		dev_dbg(echan->vchan.chan.device->dev,
 			"\n pset[%d]:\n"
 			"  chnum\t%d\n"

@@ -155,14 +159,14 @@ static void edma_execute(struct edma_cha
 			"  cidx\t%08x\n"
 			"  lkrld\t%08x\n",
 			j, echan->ch_num, echan->slot[i],
-			edesc->pset[j].opt,
-			edesc->pset[j].src,
-			edesc->pset[j].dst,
-			edesc->pset[j].a_b_cnt,
-			edesc->pset[j].ccnt,
-			edesc->pset[j].src_dst_bidx,
-			edesc->pset[j].src_dst_cidx,
-			edesc->pset[j].link_bcntrld);
+			edesc->pset[j].hwpar.opt,
+			edesc->pset[j].hwpar.src,
+			edesc->pset[j].hwpar.dst,
+			edesc->pset[j].hwpar.a_b_cnt,
+			edesc->pset[j].hwpar.ccnt,
+			edesc->pset[j].hwpar.src_dst_bidx,
+			edesc->pset[j].hwpar.src_dst_cidx,
+			edesc->pset[j].hwpar.link_bcntrld);
 		/* Link to the previous slot if not the last set */
 		if (i != (nslots - 1))
 			edma_link(echan->slot[i], echan->slot[i+1]);

@@ -274,13 +278,14 @@ static int edma_control(struct dma_chan
  * @dma_length: Total length of the DMA transfer
  * @direction: Direction of the transfer
  */
-static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
+static int edma_config_pset(struct dma_chan *chan, struct edma_pset *epset,
 	dma_addr_t src_addr, dma_addr_t dst_addr, u32 burst,
 	enum dma_slave_buswidth dev_width, unsigned int dma_length,
 	enum dma_transfer_direction direction)
 {
 	struct edma_chan *echan = to_edma_chan(chan);
 	struct device *dev = chan->device->dev;
+	struct edmacc_param *hwpar = &epset->hwpar;
 	int acnt, bcnt, ccnt, cidx;
 	int src_bidx, dst_bidx, src_cidx, dst_cidx;
 	int absync;

@@ -351,26 +356,26 @@ static int edma_config_pset(struct dma_c
 		return -EINVAL;
 	}
 
-	pset->opt = EDMA_TCC(EDMA_CHAN_SLOT(echan->ch_num));
+	hwpar->opt = EDMA_TCC(EDMA_CHAN_SLOT(echan->ch_num));
 	/* Configure A or AB synchronized transfers */
 	if (absync)
-		pset->opt |= SYNCDIM;
+		hwpar->opt |= SYNCDIM;
 
-	pset->src = src_addr;
-	pset->dst = dst_addr;
+	hwpar->src = src_addr;
+	hwpar->dst = dst_addr;
 
-	pset->src_dst_bidx = (dst_bidx << 16) | src_bidx;
-	pset->src_dst_cidx = (dst_cidx << 16) | src_cidx;
+	hwpar->src_dst_bidx = (dst_bidx << 16) | src_bidx;
+	hwpar->src_dst_cidx = (dst_cidx << 16) | src_cidx;
 
-	pset->a_b_cnt = bcnt << 16 | acnt;
-	pset->ccnt = ccnt;
+	hwpar->a_b_cnt = bcnt << 16 | acnt;
+	hwpar->ccnt = ccnt;
 	/*
 	 * Only time when (bcntrld) auto reload is required is for
 	 * A-sync case, and in this case, a requirement of reload value
 	 * of SZ_64K-1 only is assured. 'link' is initially set to NULL
 	 * and then later will be populated by edma_execute.
 	 */
-	pset->link_bcntrld = 0xffffffff;
+	hwpar->link_bcntrld = 0xffffffff;
 	return absync;
 }
 

@@ -457,11 +462,11 @@ static struct dma_async_tx_descriptor *e
 		/* If this is the last in a current SG set of transactions,
 		   enable interrupts so that next set is processed */
 		if (!((i+1) % MAX_NR_SG))
-			edesc->pset[i].opt |= TCINTEN;
+			edesc->pset[i].hwpar.opt |= TCINTEN;
 
 		/* If this is the last set, enable completion interrupt flag */
 		if (i == sg_len - 1)
-			edesc->pset[i].opt |= TCINTEN;
+			edesc->pset[i].hwpar.opt |= TCINTEN;
 	}
 
 	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);

@@ -582,14 +587,14 @@ static struct dma_async_tx_descriptor *e
 			"  cidx\t%08x\n"
 			"  lkrld\t%08x\n",
 			i, echan->ch_num, echan->slot[i],
-			edesc->pset[i].opt,
-			edesc->pset[i].src,
-			edesc->pset[i].dst,
-			edesc->pset[i].a_b_cnt,
-			edesc->pset[i].ccnt,
-			edesc->pset[i].src_dst_bidx,
-			edesc->pset[i].src_dst_cidx,
-			edesc->pset[i].link_bcntrld);
+			edesc->pset[i].hwpar.opt,
+			edesc->pset[i].hwpar.src,
+			edesc->pset[i].hwpar.dst,
+			edesc->pset[i].hwpar.a_b_cnt,
+			edesc->pset[i].hwpar.ccnt,
+			edesc->pset[i].hwpar.src_dst_bidx,
+			edesc->pset[i].hwpar.src_dst_cidx,
+			edesc->pset[i].hwpar.link_bcntrld);
 
 		edesc->absync = ret;
 

@@ -597,7 +602,7 @@ static struct dma_async_tx_descriptor *e
 		 * Enable interrupts for every period because callback
 		 * has to be called for every period.
 		 */
-		edesc->pset[i].opt |= TCINTEN;
+		edesc->pset[i].hwpar.opt |= TCINTEN;
 	}
 
 	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);

Reading destination and source is pointless. In DEV_TO_MEM transfers we are only interested in the destination, in MEM_TO_DEV we care about the source. In MEM_TO_MEM it really does not matter which one you read.

Remove the extra pointer and select dest/source via a bool.

Reading the src/dst data from an active parameter set in the EDMA parameter RAM is not reliable, as there might be a concurrent update from the controller.

But experimentation showed, that a double readout with comparing the results and a limited loop works nicely. I've actually never found a case where the loop limit triggered, but we have it there for sanity reasons. In case it triggers we return -EBUSY and let the caller deal with it.

Remove the export of this function while at it. The only potential user is the dmaengine and that's always builtin.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/arm/common/edma.c             |   48 ++++++++++++++++++++++++++-----------
 include/linux/platform_data/edma.h |    2 -
 2 files changed, 35 insertions(+), 15 deletions(-)

Index: linux/arch/arm/common/edma.c
===================================================================

arch/arm/common/edma.c +28 -13

@@ -994,29 +994,49 @@ void edma_set_dest(unsigned slot, dma_ad
 EXPORT_SYMBOL(edma_set_dest);
 
 /**
- * edma_get_position - returns the current transfer points
+ * edma_get_position - returns the current transfer point
  * @slot: parameter RAM slot being examined
- * @src: pointer to source port position
- * @dst: pointer to destination port position
+ * @pos:  where to store the data
+ * @dst:  true selects the dest position, false the source
  *
- * Returns current source and destination addresses for a particular
- * parameter RAM slot.  Its channel should not be active when this is called.
+ * Return 0 indicates a stable readout. -EBUSY indicates that the
+ * readout failed due to concurrent updates.
+ *
+ * Call this on active channels with care. For inactive channels this
+ * never fails.
  */
-void edma_get_position(unsigned slot, dma_addr_t *src, dma_addr_t *dst)
+int edma_get_position(unsigned slot, dma_addr_t *pos, bool dst)
 {
-	struct edmacc_param temp;
-	unsigned ctlr;
+	u32 dat, ctlr, offs;
+	int i;
 
 	ctlr = EDMA_CTLR(slot);
 	slot = EDMA_CHAN_SLOT(slot);
 
-	edma_read_slot(EDMA_CTLR_CHAN(ctlr, slot), &temp);
-	if (src != NULL)
-		*src = temp.src;
-	if (dst != NULL)
-		*dst = temp.dst;
+	if (slot >= edma_cc[ctlr]->num_slots)
+		return -EINVAL;
+
+	offs = PARM_OFFSET(slot);
+	offs += dst ? PARM_DST : PARM_SRC;
+
+	/*
+	 * If the channel is active, we need to double read as we
+	 * might see half updated data. We limit this to 5
+	 * attempts. If that fails we return -EBUSY and let the caller
+	 * deal with it.
+	 */
+	dat = edma_read(ctlr, offs);
+	for (i = 0; i < 5; i++) {
+		u32 tmp = edma_read(ctlr, offs);
+
+		if (tmp == dat) {

Quoted 83 lines · Thomas Gleixner

Reading destination and source is pointless. In DEV_TO_MEM transfers we are only interested in the destination, in MEM_TO_DEV we care about the source. In MEM_TO_MEM it really does not matter which one you read.

Remove the extra pointer and select dest/source via a bool.

Reading the src/dst data from an active parameter set in the EDMA parameter RAM is not reliable, as there might be a concurrent update from the controller.

But experimentation showed, that a double readout with comparing the results and a limited loop works nicely. I've actually never found a case where the loop limit triggered, but we have it there for sanity reasons. In case it triggers we return -EBUSY and let the caller deal with it.

Remove the export of this function while at it. The only potential user is the dmaengine and that's always builtin.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/arm/common/edma.c             |   48 ++++++++++++++++++++++++++-----------
 include/linux/platform_data/edma.h |    2 -
 2 files changed, 35 insertions(+), 15 deletions(-)

Index: linux/arch/arm/common/edma.c
===================================================================

arch/arm/common/edma.c +28 -13

@@ -994,29 +994,49 @@ void edma_set_dest(unsigned slot, dma_ad
 EXPORT_SYMBOL(edma_set_dest);
 
 /**
- * edma_get_position - returns the current transfer points
+ * edma_get_position - returns the current transfer point
  * @slot: parameter RAM slot being examined
- * @src: pointer to source port position
- * @dst: pointer to destination port position
+ * @pos:  where to store the data
+ * @dst:  true selects the dest position, false the source
  *
- * Returns current source and destination addresses for a particular
- * parameter RAM slot.  Its channel should not be active when this is called.
+ * Return 0 indicates a stable readout. -EBUSY indicates that the
+ * readout failed due to concurrent updates.
+ *
+ * Call this on active channels with care. For inactive channels this
+ * never fails.
  */
-void edma_get_position(unsigned slot, dma_addr_t *src, dma_addr_t *dst)
+int edma_get_position(unsigned slot, dma_addr_t *pos, bool dst)
 {
-	struct edmacc_param temp;
-	unsigned ctlr;
+	u32 dat, ctlr, offs;
+	int i;
 
 	ctlr = EDMA_CTLR(slot);
 	slot = EDMA_CHAN_SLOT(slot);
 
-	edma_read_slot(EDMA_CTLR_CHAN(ctlr, slot), &temp);
-	if (src != NULL)
-		*src = temp.src;
-	if (dst != NULL)
-		*dst = temp.dst;
+	if (slot >= edma_cc[ctlr]->num_slots)
+		return -EINVAL;
+
+	offs = PARM_OFFSET(slot);
+	offs += dst ? PARM_DST : PARM_SRC;
+
+	/*
+	 * If the channel is active, we need to double read as we
+	 * might see half updated data. We limit this to 5
+	 * attempts. If that fails we return -EBUSY and let the caller
+	 * deal with it.
+	 */
+	dat = edma_read(ctlr, offs);
+	for (i = 0; i < 5; i++) {
+		u32 tmp = edma_read(ctlr, offs);
+
+		if (tmp == dat) {

arch/arm/common/edma.c void edma_set_dest(unsigned slot, dma_ad · line 1041

Quoted 10 lines

arch/arm/common/edma.c +6 -1

+			*pos = dat;
+			return 0;
+		}
+		dat = tmp;
+	}
+	return -EBUSY;
 }
-EXPORT_SYMBOL(edma_get_position);
 
 /**

The access is synchronized though and you shouldn't see unreliable results, unless you _are_ seeing unreliable results in which case it could be a silicon issue.

The original code also doesn't have the double read so I would just drop that, unless you are seeing reliability issues.

Here's a thread that discusses it and the end conclusion is that it should be fine (Kyle Kastile is one of the EDMA hardware designers).
http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/99/t/66011.aspx

Quoted 11 lines · Thomas Gleixner

(quoted diff) +11 -0

+	/*
+	 * If the channel is active, we need to double read as we
+	 * might see half updated data. We limit this to 5
+	 * attempts. If that fails we return -EBUSY and let the caller
+	 * deal with it.
+	 */
+	dat = edma_read(ctlr, offs);
+	for (i = 0; i < 5; i++) {
+		u32 tmp = edma_read(ctlr, offs);
+
+		if (tmp == dat) {

Review

Quoted 10 lines

(quoted diff) +6 -1

+			*pos = dat;
+			return 0;
+		}
+		dat = tmp;
+	}
+	return -EBUSY;
 }
-EXPORT_SYMBOL(edma_get_position);
 
 /**

The access is synchronized though and you shouldn't see unreliable results, unless you _are_ seeing unreliable results in which case it could be a silicon issue.

The original code also doesn't have the double read so I would just drop that, unless you are seeing reliability issues.

Here's a thread that discusses it and the end conclusion is that it should be fine (Kyle Kastile is one of the EDMA hardware designers).
http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/99/t/66011.aspx

Quoted 3 lines · Joel Fernandes

Here's a thread that discusses it and the end conclusion is that it should be fine (Kyle Kastile is one of the EDMA hardware designers).
http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/99/t/66011.aspx

Gah, right I saw the issues with the original code, which does a memcpy_fromio which copies byte by byte. Did not think about retrying with the edma_read() based one.

Quoted 5 lines · Thomas Gleixner

Quoted 3 lines · Joel Fernandes

Here's a thread that discusses it and the end conclusion is that it should be fine (Kyle Kastile is one of the EDMA hardware designers).
http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/99/t/66011.aspx

Quoted 3 lines

Gah, right I saw the issues with the original code, which does a memcpy_fromio which copies byte by byte. Did not think about retrying with the edma_read() based one.

Actually I did expect memcpy_fromio to be more clever. But you are right, using the 32bit wide read works like a charm.

The first slot in the ParamRAM of EDMA holds the current active subtransfer. Depending on the direction we read either the source or the destination address from there. In the internat psets we have the address of the buffer(s).

In the cyclic case we only use the internal pset[0] which holds the start address of the circular buffer and calculate the remaining room to the end of the buffer.

In the SG case we read the current address and compare it to the internal psets address and length. If the current address is outside of this range, the pset has been processed already and we can mark it done, update the residue value and process the next set. If its inside the range we know that we look at the current active set and stop the walk. In case of intermediate transfers we update the stats in the callback function before starting the next batch of transfers.

The tx_status callback and the callback are serialized via vchan.lock.

In the unexpected case that the read of the paramram fails due to concurrent updates by the DMA engine, we return the last good value.

Signed-off-by Thomas Gleixner <[email protected]>

---
 drivers/dma/edma.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/dma/edma.c
===================================================================

drivers/dma/edma.c +6 -1

 	struct edma_pset		pset[0];
 };
 

@@ -448,6 +451,8 @@ static struct dma_async_tx_descriptor *e
 		}
 	}
 
+	edesc->slot0 = echan->slot[0];
+
 	/* Configure PaRAM sets for each SG */
 	for_each_sg(sgl, sg, sg_len, i) {
 		/* Get address for each SG */

@@ -476,6 +481,7 @@ static struct dma_async_tx_descriptor *e
 		if (i == sg_len - 1)
 			edesc->pset[i].hwpar.opt |= TCINTEN;
 	}
+	edesc->residue_stat = edesc->residue;
 
 	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
 }

@@ -543,7 +549,7 @@ static struct dma_async_tx_descriptor *e
 
 	edesc->cyclic = 1;
 	edesc->pset_nr = nslots;
-	edesc->residue = buf_len;
+	edesc->residue = edesc->residue_stat = buf_len;
 	edesc->direction = direction;
 
 	dev_dbg(dev, "%s: nslots=%d\n", __func__, nslots);

@@ -613,6 +619,7 @@ static struct dma_async_tx_descriptor *e
 		 */
 		edesc->pset[i].hwpar.opt |= TCINTEN;
 	}
+	edesc->slot0 = echan->slot[0];
 
 	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
 }

@@ -645,7 +652,17 @@ static void edma_callback(unsigned ch_nu
 				vchan_cookie_complete(&edesc->vdesc);
 				edma_execute(echan);
 			} else {
+				int i, n;

drivers/dma/edma.c +11 -0

 				edma_execute(echan);
 			}
 		}

@@ -773,6 +790,66 @@ static void edma_issue_pending(struct dm
 	spin_unlock_irqrestore(&echan->vchan.lock, flags);
 }
 
+static u32 edma_residue(struct edma_desc *edesc)
+{
+	bool dst = edesc->direction == DMA_DEV_TO_MEM;
+	struct edma_pset *pset = edesc->pset;
+	dma_addr_t done, pos;
+	int ret, i;
+
+	/*
+	 * We always read the dst/src position from the first RamPar
+	 * pset. That's the one which is active now.
+	 */

drivers/dma/edma.c +26 -0

+	/*
+	 * Cyclic is simple. Just subtract pset[0].addr from pos.
+	 *
+	 * We never update edesc->residue in the cyclic case, so we
+	 * can tell the remaining room to the end of the circular
+	 * buffer.
+	 */
+	if (edesc->cyclic) {
+		done = pos - pset->addr;
+		edesc->residue_stat = edesc->residue - done;
+		return edesc->residue_stat;
+	}
+
+	/*
+	 * For SG operation we catch up with the last processed
+	 * status.
+	 */
+	pset += edesc->processed_stat;
+
+	for (i = edesc->processed_stat; i < edesc->processed; i++, pset++) {
+		/*
+		 * If we are inside this pset address range, we know
+		 * this is the active one. Get the current delta and
+		 * stop walking the psets.
+		 */
+		if (pos >= pset->addr && pos < pset->addr + pset->len) {

drivers/dma/edma.c +4 -1

+	return edesc->residue_stat;
+}
+
 /* Check request completion status */
 static enum dma_status edma_tx_status(struct dma_chan *chan,
 				      dma_cookie_t cookie,

@@ -789,7 +866,7 @@ static enum dma_status edma_tx_status(st
 
 	spin_lock_irqsave(&echan->vchan.lock, flags);
 	if (echan->edesc && echan->edesc->vdesc.tx.cookie == cookie)
-		txstate->residue = echan->edesc->residue;
+		txstate->residue = edma_residue(echan->edesc);
 	else if ((vdesc = vchan_find_desc(&echan->vchan, cookie)))
 		txstate->residue = to_edma_desc(&vdesc->tx)->residue;
 	spin_unlock_irqrestore(&echan->vchan.lock, flags);

Review

Quoted 2 lines · Thomas Gleixner

(quoted diff) +2 -0

+	u32				residue_stat;
+	int				slot0;

Instead of slot0, perhaps storing a pointer to the echan would be great incase in the future we need to access something else in the channel. Then you can just do edesc->echan->slot[0].

Quoted 12 lines

(quoted diff) +7 -0

@@ -645,7 +652,17 @@ static void edma_callback(unsigned ch_nu
 				vchan_cookie_complete(&edesc->vdesc);
 				edma_execute(echan);
 			} else {
+				int i, n;
+
 				dev_dbg(dev, "Intermediate transfer complete on channel %d\n", ch_num);
+
+				/* Update statistics for tx_status */
+				n = edesc->processed;
+				for (i = edesc->processed_stat; i < n; i++)
+					edesc->residue -= edesc->pset[i].len;

Review static void edma_callback(unsigned ch_nu · line 665

Quoted 3 lines

(quoted diff) +3 -0

+				edesc->processed_stat = n;
+				edesc->residue_stat = edesc->residue;
+

This is a bit of a NAK since it is O(n) on every intermediate transfer completion. I'm not sure of a better way to do it but I certainly don't want the IRQ slowed down anymore... Is there a way to pre-calculate the size of the intermediate transfer and store it somewhere for accounting?

Also another thing is I don't think processed_stat is required at all, because I think you introduced it for the possibility that there might be delays in processing interrupts for intermediate transfers. That is

actually an impossibility because such a scenario would result in catastrophic failure anyway. Because for intermediate transfer interrupts shouldn't be missed, so in this case you can just do:

for (i = 1; i <= MAX_NR_SG; i++) {
	edesc->residue -= edesc->pset[edesc->processed - i].len;
}

I think that'll work.

Review static void edma_callback(unsigned ch_nu · line 668

Quoted 3 lines

(quoted diff) +3 -0

+	if (ret)
+		return edesc->residue_stat;
+

This check could be dropped following the discussion in earlier patch and also residue_stat could be dropped if there's no other use for it.

Review static void edma_callback(unsigned ch_nu

Quoted 6 lines

(quoted diff) +6 -0

+
+		/* Otherwise mark it done and update residue[_stat]. */
+		edesc->processed_stat++;
+		edesc->residue -= pset->len;
+		edesc->residue_stat = edesc->residue;
+	}

Also, just curious - doesn't calling status too aggressively result in any slowness for you? Since vchan lock spinlock will be held during the duration of this processing, and interrupts would be disabled causing edma_callback interrupt delays possibly. I'm not saying you introduced the lock or anything because status would previously also hold the lock. I just haven't played status enough to know how it will behave when happening concurrently with DMA transfers.

Quoted 12 lines

Quoted 12 lines

(quoted diff) +7 -0

@@ -645,7 +652,17 @@ static void edma_callback(unsigned ch_nu
 				vchan_cookie_complete(&edesc->vdesc);
 				edma_execute(echan);
 			} else {
+				int i, n;
+
 				dev_dbg(dev, "Intermediate transfer complete on channel %d\n", ch_num);
+
+				/* Update statistics for tx_status */
+				n = edesc->processed;
+				for (i = edesc->processed_stat; i < n; i++)
+					edesc->residue -= edesc->pset[i].len;

Quoted 8 lines

Review static void edma_callback(unsigned ch_nu · line 665

Quoted 3 lines

(quoted diff) +3 -0

+				edesc->processed_stat = n;
+				edesc->residue_stat = edesc->residue;
+

This is a bit of a NAK since it is O(n) on every intermediate transfer completion. I'm not sure of a better way to do it but I certainly don't want the IRQ slowed down anymore... Is there a way to pre-calculate the size of the intermediate transfer and store it somewhere for accounting?

Yeah, I can do that. Adds an extra field to edma_pset, but that shouldnt hurt.

Quoted 3 lines

Also another thing is I don't think processed_stat is required at all, because I think you introduced it for the possibility that there might be delays in processing interrupts for intermediate transfers. That is

No, I introduced it to not walk the full list of psets in every tx_status() call. It's keeping track of the already finished slots.

Quoted 9 lines

actually an impossibility because such a scenario would result in catastrophic failure anyway. Because for intermediate transfer interrupts shouldn't be missed, so in this case you can just do:

for (i = 1; i <= MAX_NR_SG; i++) {
	edesc->residue -= edesc->pset[edesc->processed - i].len;
}

I think that'll work.

No. The point is:

submit()

tx_status()

We dont want to iterate through all psets when we are polling in a loop. So we progress processed_stat when we see a finished slot. And thereby we adjust edesc->residue.

Quoted 8 lines

Review static void edma_callback(unsigned ch_nu

Quoted 6 lines

(quoted diff) +6 -0

+
+		/* Otherwise mark it done and update residue[_stat]. */
+		edesc->processed_stat++;
+		edesc->residue -= pset->len;
+		edesc->residue_stat = edesc->residue;
+	}

Quoted 7 lines

Also, just curious - doesn't calling status too aggressively result in any slowness for you? Since vchan lock spinlock will be held during the duration of this processing, and interrupts would be disabled causing edma_callback interrupt delays possibly. I'm not saying you introduced the lock or anything because status would previously also hold the lock. I just haven't played status enough to know how it will behave when happening concurrently with DMA transfers.

Well, in the case I'm looking at (cyclic transfer) I do not care about the interrupt at all. I get a notification for the first incoming packet via the peripheral and go from there.

But yes, it might cause slight delays for the interrupt in the SG case. Though the irq disabled region is small if you actively poll as we keep track of the processed slots and therefor only have one or two readouts to process.

But as Russell pointed out, this should be rewritten by updating the chain on the fly and not waiting until MAX_SG has been processed.

The next obstacle was the missing per SG element reporting. We really can't wait for a full SG list for notification.

So we'd trade the CAN interrupt per packet against the EDMA interrupt per packet. And the notification which is done via a tasklet is not really helpful either.

The DCAN readout is 4 consecutive 32bit registers. The only way I got that working is by configuring the engine with:

       cfg.direction = DMA_DEV_TO_MEM;
       cfg.src_addr_width = 16;

       cfg.src_maxburst = 1;

With

       cfg.src_addr_width = 4;
       cfg.src_maxburst = 4;

it reads just 4 times the first register.

Quoted 2 lines · Thomas Gleixner

The next obstacle was the missing per SG element reporting. We really can't wait for a full SG list for notification.

Err, dmaengine doesn't have per-SG element reporting.

What it does allow is several transactions to be submitted consecutively, so that the DMA engine can move to the next transaction once the previous one has been submitted.

Where it's important that this happens with the minimum of delay, there's nothing in the API that prevents the hardware scatterlist of the previous transaction being linked directly to the following transaction, provided of course the hardware can do that.

Many DMA engine implementations are just lazy - they implement stuff as:
setup hardware, run scatter list, get to the end, raise interrupt. Fire off tasklet. Tasklet runs, calls the callback, checks to see if there's another transaction, sets up hardware for the next one. That (as you would expect) gives quite a high latency to the following transaction.

I've coded at least one DMA engine driver to start the next transaction immediately that the previous one completes, before the tasklet is run. As I say above, there's really no reason to even wait for the interrupt... if people can be bothered to think about all the implications that brings (f.e. reporting completion status, and how many bytes remaining of a transaction, etc.)

If it's just that the FIFO is spread over 4 consecutive locations (effectively due to not decoding bits 2,3 of the address bus for the register) then reading the first register four times is just as acceptable as reading them consecutively.

The reason that kind of thing was done in old days was to allow the ARM ldmia/stmia instructions to be used to access FIFOs, thereby allowing multiple words to be transferred with a single instruction. I can't believe that there's still people designing for that especially if they have a DMA engine...

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Thomas Gleixner

The next obstacle was the missing per SG element reporting. We really can't wait for a full SG list for notification.

Err, dmaengine doesn't have per-SG element reporting.

enum dma_residue_granularity {
DMA_RESIDUE_GRANULARITY_DESCRIPTOR = 0,
DMA_RESIDUE_GRANULARITY_SEGMENT = 1,
DMA_RESIDUE_GRANULARITY_BURST = 2,
};

tells a different story.

Quoted 4 lines

If it's just that the FIFO is spread over 4 consecutive locations (effectively due to not decoding bits 2,3 of the address bus for the register) then reading the first register four times is just as acceptable as reading them consecutively.

It's not a FIFO. It's four different consecutive registers, which are DMA readable. And you need to read all of them...

Quoted 5 lines

The reason that kind of thing was done in old days was to allow the ARM ldmia/stmia instructions to be used to access FIFOs, thereby allowing multiple words to be transferred with a single instruction. I can't believe that there's still people designing for that especially if they have a DMA engine...

In that case it's a magic DMA extension superglued beside the already horrible register interface of that particular IP block.

Quoted 14 lines · Thomas Gleixner

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Thomas Gleixner

The next obstacle was the missing per SG element reporting. We really can't wait for a full SG list for notification.

Err, dmaengine doesn't have per-SG element reporting.

enum dma_residue_granularity {
DMA_RESIDUE_GRANULARITY_DESCRIPTOR = 0,
DMA_RESIDUE_GRANULARITY_SEGMENT = 1,
DMA_RESIDUE_GRANULARITY_BURST = 2,
};

Quoted 1 line

tells a different story.

That's to do with the residue though, not to do with callbacks.

Quoted 14 lines

Quoted 4 lines

If it's just that the FIFO is spread over 4 consecutive locations (effectively due to not decoding bits 2,3 of the address bus for the register) then reading the first register four times is just as acceptable as reading them consecutively.

It's not a FIFO. It's four different consecutive registers, which are DMA readable. And you need to read all of them...

Quoted 5 lines

The reason that kind of thing was done in old days was to allow the ARM ldmia/stmia instructions to be used to access FIFOs, thereby allowing multiple words to be transferred with a single instruction. I can't believe that there's still people designing for that especially if they have a DMA engine...

Quoted 2 lines

In that case it's a magic DMA extension superglued beside the already horrible register interface of that particular IP block.

So it's more a copy-from-peripheral-to-memory - a DMA copy operation triggered in a similar manner to the DMA slave mode. There are a number of use cases for this, but no one has yet put their head above the parapet to spear-head that cause. :)