Skip to contentSkip to conversation

dmaengine: pl330: flush before wait, and add dev burst support.

PATCH Superseded Frank Mori Hess 2 participants 10 messages Feb 6, 2018 – Apr 16, 2018 dmaengine

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 dmaengine: pl330: flush before wait, and add dev burst support.

Resending same patch as before, except not corrupted by word-wrap this time.

Do DMAFLUSHP _before_ the first DMAWFP to insure controller
and peripheral are in agreement about dma request state before first transfer. Add support for burst transfers to/from peripherals. In the new scheme, the controller does as many burst transfers as it can then transfers the remaining dregs with either single transfers for peripherals, or with a reduced size burst for memory-to-memory transfers.

I tested dma transfers to peripherals with designware serial port (drivers/tty/serial/8250/8250_dw.c) and a GPIB interface
(https://github.com/fmhess/fmh_gpib_core), with a 4.1 kernel. The port of my changes to HEAD has only been tested to compile. I'm willing to try to test on my hardware with a HEAD kernel and try using the dmatest module to test memory-to-memory transfers if this patch might be accepted.

---
 drivers/dma/pl330.c | 131 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 87 insertions(+), 44 deletions(-)
drivers/dma/pl330.c +87 -44
index d7327fd5f445..5a3e80ec4b0b 100644
@@ -1094,26 +1094,27 @@ static inline int _ldst_memtomem(unsigned dry_run, u8 buf[],
 	return off;
 }
 
-static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
-				 u8 buf[], const struct _xfer_spec *pxs,
-				 int cyc)
+static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run, 
+				u8 buf[], const struct _xfer_spec *pxs, 
+				int cyc, enum pl330_cond cond)
 {
 	int off = 0;
-	enum pl330_cond cond;
 
 	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
 		cond = BURST;
-	else
-		cond = SINGLE;
 
+	/* do FLUSHP at beginning to clear any stale dma requests before first WFP. */
+	if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
-		off += _emit_LDP(dry_run, &buf[off], cond, pxs->desc->peri);
+		if(cond == ALWAYS) {
+			off += _emit_LDP(dry_run, &buf[off], SINGLE, pxs->desc->peri);
+			off += _emit_LDP(dry_run, &buf[off], BURST, pxs->desc->peri);
+		}else {
+			off += _emit_LDP(dry_run, &buf[off], cond, pxs->desc->peri);
+		}
 		off += _emit_ST(dry_run, &buf[off], ALWAYS);
-
-		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-			off += _emit_FLUSHP(dry_run, &buf[off],
-					    pxs->desc->peri);
 	}
 
 	return off;
@@ -1121,24 +1122,26 @@ static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
 
 static inline int _ldst_memtodev(struct pl330_dmac *pl330,
 				 unsigned dry_run, u8 buf[],
-				 const struct _xfer_spec *pxs, int cyc)
+				 const struct _xfer_spec *pxs, int cyc, 
+				 enum pl330_cond cond)
 {
 	int off = 0;
-	enum pl330_cond cond;
 
 	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
 		cond = BURST;
-	else
-		cond = SINGLE;
 
+	/* do FLUSHP at beginning to clear any stale dma requests before first WFP. */
+	if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_LD(dry_run, &buf[off], ALWAYS);
-		off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
-
-		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-			off += _emit_FLUSHP(dry_run, &buf[off],
-					    pxs->desc->peri);
+		if(cond == ALWAYS) {
+			off += _emit_STP(dry_run, &buf[off], SINGLE, pxs->desc->peri);
+			off += _emit_STP(dry_run, &buf[off], BURST, pxs->desc->peri);
+		}else {
+			off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
+		}
 	}
 
 	return off;
@@ -1148,13 +1151,14 @@ static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
-
+	enum pl330_cond cond = BRST_LEN(pxs->ccr) > 1 ? BURST : SINGLE;
+	
 	switch (pxs->desc->rqtype) {
 	case DMA_MEM_TO_DEV:
-		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, cyc);
+		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, cyc, cond);
 		break;
 	case DMA_DEV_TO_MEM:
-		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, cyc);
+		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, cyc, cond);
 		break;
 	case DMA_MEM_TO_MEM:
 		off += _ldst_memtomem(dry_run, &buf[off], pxs, cyc);
@@ -1167,6 +1171,39 @@ static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 	return off;
 }
 
+/* transfer dregs with single transfers to peripheral, or a reduced size burst
+ * for mem-to-mem. */
+static int _dregs(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
+		const struct _xfer_spec *pxs, int transfer_length)
+{
+	int off = 0;
+	int dregs_ccr;
+	
+	if(transfer_length == 0) return off;
+	
+	switch (pxs->desc->rqtype) {
+	case DMA_MEM_TO_DEV:
+		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, transfer_length, SINGLE);
+		break;
+	case DMA_DEV_TO_MEM:
+		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, transfer_length, SINGLE);
+		break;
+	case DMA_MEM_TO_MEM:
+		dregs_ccr = pxs->ccr;
+		dregs_ccr &= ~((0xf << CC_SRCBRSTLEN_SHFT) | (0xf << CC_DSTBRSTLEN_SHFT)) ;
+		dregs_ccr |= (((transfer_length - 1) & 0xf) << CC_SRCBRSTLEN_SHFT);
+		dregs_ccr |= (((transfer_length - 1) & 0xf) << CC_DSTBRSTLEN_SHFT);
+		off += _emit_MOV(dry_run, &buf[off], CCR, dregs_ccr);
+		off += _ldst_memtomem(dry_run, &buf[off], pxs, 1);
+		break;
+	default:
+		off += 0x40000000; /* Scare off the Client */
+		break;
+	}
+
+	return off;
+}
+
 /* Returns bytes consumed and updates bursts */
 static inline int _loop(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		unsigned long *bursts, const struct _xfer_spec *pxs)
@@ -1256,6 +1293,7 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 	struct pl330_xfer *x = &pxs->desc->px;
 	u32 ccr = pxs->ccr;
 	unsigned long c, bursts = BYTE_TO_BURST(x->bytes, ccr);
+	int num_dregs = (x->bytes - BURST_TO_BYTE(bursts, ccr)) / BRST_SIZE(ccr);
 	int off = 0;
 
 	while (bursts) {
@@ -1263,7 +1301,8 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 		off += _loop(pl330, dry_run, &buf[off], &c, pxs);
 		bursts -= c;
 	}
-
+	off += _dregs(pl330, dry_run, &buf[off], pxs, num_dregs);
+	
 	return off;
 }
 
@@ -1294,7 +1333,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 		      struct _xfer_spec *pxs)
 {
 	struct _pl330_req *req = &thrd->req[index];
-	struct pl330_xfer *x;
 	u8 *buf = req->mc_cpu;
 	int off = 0;
 
@@ -1303,11 +1341,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 	/* DMAMOV CCR, ccr */
 	off += _emit_MOV(dry_run, &buf[off], CCR, pxs->ccr);
 
-	x = &pxs->desc->px;
-	/* Error if xfer length is not aligned at burst size */
-	if (x->bytes % (BRST_SIZE(pxs->ccr) * BRST_LEN(pxs->ccr)))
-		return -EINVAL;
-
 	off += _setup_xfer(pl330, dry_run, &buf[off], pxs);
 
 	/* DMASEV peripheral/event */
@@ -2115,15 +2148,31 @@ static int pl330_config(struct dma_chan *chan,
 			pch->fifo_addr = slave_config->dst_addr;
 		if (slave_config->dst_addr_width)
 			pch->burst_sz = __ffs(slave_config->dst_addr_width);
-		if (slave_config->dst_maxburst)
-			pch->burst_len = slave_config->dst_maxburst;
+		if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+			pch->burst_len = 1;
+		else if (slave_config->dst_maxburst)
+		{
+			if(slave_config->dst_maxburst > PL330_MAX_BURST)
+				pch->burst_len = PL330_MAX_BURST;
+			else
+				pch->burst_len = slave_config->dst_maxburst;
+		}else
+			pch->burst_len = 1;
 	} else if (slave_config->direction == DMA_DEV_TO_MEM) {
 		if (slave_config->src_addr)
 			pch->fifo_addr = slave_config->src_addr;
 		if (slave_config->src_addr_width)
 			pch->burst_sz = __ffs(slave_config->src_addr_width);
-		if (slave_config->src_maxburst)
-			pch->burst_len = slave_config->src_maxburst;
+		if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+			pch->burst_len = 1;
+		else if (slave_config->src_maxburst)
+		{
+			if(slave_config->src_maxburst > PL330_MAX_BURST)
+				pch->burst_len = PL330_MAX_BURST;
+			else
+				pch->burst_len = slave_config->src_maxburst;
+		}else
+			pch->burst_len = 1;
 	}
 
 	return 0;
@@ -2517,14 +2566,8 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
 	burst_len >>= desc->rqcfg.brst_size;
 
 	/* src/dst_burst_len can't be more than 16 */
-	if (burst_len > 16)
-		burst_len = 16;
-
-	while (burst_len > 1) {
-		if (!(len % (burst_len << desc->rqcfg.brst_size)))
-			break;
-		burst_len--;
-	}
+	if (burst_len > PL330_MAX_BURST)
+		burst_len = PL330_MAX_BURST;
 
 	return burst_len;
 }
@@ -2596,7 +2639,7 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 
 		desc->rqtype = direction;
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2741,7 +2784,7 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		}
 
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->rqtype = direction;
 		desc->bytes_requested = sg_dma_len(sg);
 	}
#1 lore raw

PATCH v2 1/1 dmaengine: pl330: flush before wait, and add dev burst support.

Quoted 2 lines · Frank Mori Hess

Resending same patch as before, except not corrupted by word-wrap this time.

This is useless for changelog..

Quoted 2 lines

Do DMAFLUSHP _before_ the first DMAWFP to insure controller

do you mean ensure..?

Quoted 6 lines

and peripheral are in agreement about dma request state before first transfer. Add support for burst transfers to/from peripherals. In the new scheme, the controller does as many burst transfers as it can then transfers the remaining dregs with either single transfers for peripherals, or with a reduced size burst for memory-to-memory transfers.

Quoted 7 lines

I tested dma transfers to peripherals with designware serial port (drivers/tty/serial/8250/8250_dw.c) and a GPIB interface
(https://github.com/fmhess/fmh_gpib_core), with a 4.1 kernel. The port of my changes to HEAD has only been tested to compile. I'm willing to try to test on my hardware with a HEAD kernel and try using the dmatest module to test memory-to-memory transfers if this patch might be accepted.

This one as well, pls move this after s-o-b line, which give reviewer and maintainer context but don't get applied..

Quoted 5 lines
---
 drivers/dma/pl330.c | 131 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 87 insertions(+), 44 deletions(-)

This has too many style issues, please run ./scripts/checkpatch.pl on the patch. It should have 0 errors, pls use your discretion for warnings

Right now it shows:
total: 19 errors, 21 warnings

*Also* CC Maintainers, ./scripts/get_maintainer.pl will tell you whom to CC. Maintainers might miss the patch if not CCed...

Quoted 256 lines
drivers/dma/pl330.c +87 -44
index d7327fd5f445..5a3e80ec4b0b 100644
@@ -1094,26 +1094,27 @@ static inline int _ldst_memtomem(unsigned dry_run, u8 buf[],
 	return off;
 }
 
-static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
-				 u8 buf[], const struct _xfer_spec *pxs,
-				 int cyc)
+static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run, 
+				u8 buf[], const struct _xfer_spec *pxs, 
+				int cyc, enum pl330_cond cond)
 {
 	int off = 0;
-	enum pl330_cond cond;
 
 	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
 		cond = BURST;
-	else
-		cond = SINGLE;
 
+	/* do FLUSHP at beginning to clear any stale dma requests before first WFP. */
+	if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
-		off += _emit_LDP(dry_run, &buf[off], cond, pxs->desc->peri);
+		if(cond == ALWAYS) {
+			off += _emit_LDP(dry_run, &buf[off], SINGLE, pxs->desc->peri);
+			off += _emit_LDP(dry_run, &buf[off], BURST, pxs->desc->peri);
+		}else {
+			off += _emit_LDP(dry_run, &buf[off], cond, pxs->desc->peri);
+		}
 		off += _emit_ST(dry_run, &buf[off], ALWAYS);
-
-		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-			off += _emit_FLUSHP(dry_run, &buf[off],
-					    pxs->desc->peri);
 	}
 
 	return off;
@@ -1121,24 +1122,26 @@ static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
 
 static inline int _ldst_memtodev(struct pl330_dmac *pl330,
 				 unsigned dry_run, u8 buf[],
-				 const struct _xfer_spec *pxs, int cyc)
+				 const struct _xfer_spec *pxs, int cyc, 
+				 enum pl330_cond cond)
 {
 	int off = 0;
-	enum pl330_cond cond;
 
 	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
 		cond = BURST;
-	else
-		cond = SINGLE;
 
+	/* do FLUSHP at beginning to clear any stale dma requests before first WFP. */
+	if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_LD(dry_run, &buf[off], ALWAYS);
-		off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
-
-		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-			off += _emit_FLUSHP(dry_run, &buf[off],
-					    pxs->desc->peri);
+		if(cond == ALWAYS) {
+			off += _emit_STP(dry_run, &buf[off], SINGLE, pxs->desc->peri);
+			off += _emit_STP(dry_run, &buf[off], BURST, pxs->desc->peri);
+		}else {
+			off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
+		}
 	}
 
 	return off;
@@ -1148,13 +1151,14 @@ static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
-
+	enum pl330_cond cond = BRST_LEN(pxs->ccr) > 1 ? BURST : SINGLE;
+	
 	switch (pxs->desc->rqtype) {
 	case DMA_MEM_TO_DEV:
-		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, cyc);
+		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, cyc, cond);
 		break;
 	case DMA_DEV_TO_MEM:
-		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, cyc);
+		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, cyc, cond);
 		break;
 	case DMA_MEM_TO_MEM:
 		off += _ldst_memtomem(dry_run, &buf[off], pxs, cyc);
@@ -1167,6 +1171,39 @@ static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 	return off;
 }
 
+/* transfer dregs with single transfers to peripheral, or a reduced size burst
+ * for mem-to-mem. */
+static int _dregs(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
+		const struct _xfer_spec *pxs, int transfer_length)
+{
+	int off = 0;
+	int dregs_ccr;
+	
+	if(transfer_length == 0) return off;
+	
+	switch (pxs->desc->rqtype) {
+	case DMA_MEM_TO_DEV:
+		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, transfer_length, SINGLE);
+		break;
+	case DMA_DEV_TO_MEM:
+		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, transfer_length, SINGLE);
+		break;
+	case DMA_MEM_TO_MEM:
+		dregs_ccr = pxs->ccr;
+		dregs_ccr &= ~((0xf << CC_SRCBRSTLEN_SHFT) | (0xf << CC_DSTBRSTLEN_SHFT)) ;
+		dregs_ccr |= (((transfer_length - 1) & 0xf) << CC_SRCBRSTLEN_SHFT);
+		dregs_ccr |= (((transfer_length - 1) & 0xf) << CC_DSTBRSTLEN_SHFT);
+		off += _emit_MOV(dry_run, &buf[off], CCR, dregs_ccr);
+		off += _ldst_memtomem(dry_run, &buf[off], pxs, 1);
+		break;
+	default:
+		off += 0x40000000; /* Scare off the Client */
+		break;
+	}
+
+	return off;
+}
+
 /* Returns bytes consumed and updates bursts */
 static inline int _loop(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		unsigned long *bursts, const struct _xfer_spec *pxs)
@@ -1256,6 +1293,7 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 	struct pl330_xfer *x = &pxs->desc->px;
 	u32 ccr = pxs->ccr;
 	unsigned long c, bursts = BYTE_TO_BURST(x->bytes, ccr);
+	int num_dregs = (x->bytes - BURST_TO_BYTE(bursts, ccr)) / BRST_SIZE(ccr);
 	int off = 0;
 
 	while (bursts) {
@@ -1263,7 +1301,8 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 		off += _loop(pl330, dry_run, &buf[off], &c, pxs);
 		bursts -= c;
 	}
-
+	off += _dregs(pl330, dry_run, &buf[off], pxs, num_dregs);
+	
 	return off;
 }
 
@@ -1294,7 +1333,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 		      struct _xfer_spec *pxs)
 {
 	struct _pl330_req *req = &thrd->req[index];
-	struct pl330_xfer *x;
 	u8 *buf = req->mc_cpu;
 	int off = 0;
 
@@ -1303,11 +1341,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 	/* DMAMOV CCR, ccr */
 	off += _emit_MOV(dry_run, &buf[off], CCR, pxs->ccr);
 
-	x = &pxs->desc->px;
-	/* Error if xfer length is not aligned at burst size */
-	if (x->bytes % (BRST_SIZE(pxs->ccr) * BRST_LEN(pxs->ccr)))
-		return -EINVAL;
-
 	off += _setup_xfer(pl330, dry_run, &buf[off], pxs);
 
 	/* DMASEV peripheral/event */
@@ -2115,15 +2148,31 @@ static int pl330_config(struct dma_chan *chan,
 			pch->fifo_addr = slave_config->dst_addr;
 		if (slave_config->dst_addr_width)
 			pch->burst_sz = __ffs(slave_config->dst_addr_width);
-		if (slave_config->dst_maxburst)
-			pch->burst_len = slave_config->dst_maxburst;
+		if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+			pch->burst_len = 1;
+		else if (slave_config->dst_maxburst)
+		{
+			if(slave_config->dst_maxburst > PL330_MAX_BURST)
+				pch->burst_len = PL330_MAX_BURST;
+			else
+				pch->burst_len = slave_config->dst_maxburst;
+		}else
+			pch->burst_len = 1;
 	} else if (slave_config->direction == DMA_DEV_TO_MEM) {
 		if (slave_config->src_addr)
 			pch->fifo_addr = slave_config->src_addr;
 		if (slave_config->src_addr_width)
 			pch->burst_sz = __ffs(slave_config->src_addr_width);
-		if (slave_config->src_maxburst)
-			pch->burst_len = slave_config->src_maxburst;
+		if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+			pch->burst_len = 1;
+		else if (slave_config->src_maxburst)
+		{
+			if(slave_config->src_maxburst > PL330_MAX_BURST)
+				pch->burst_len = PL330_MAX_BURST;
+			else
+				pch->burst_len = slave_config->src_maxburst;
+		}else
+			pch->burst_len = 1;
 	}
 
 	return 0;
@@ -2517,14 +2566,8 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
 	burst_len >>= desc->rqcfg.brst_size;
 
 	/* src/dst_burst_len can't be more than 16 */
-	if (burst_len > 16)
-		burst_len = 16;
-
-	while (burst_len > 1) {
-		if (!(len % (burst_len << desc->rqcfg.brst_size)))
-			break;
-		burst_len--;
-	}
+	if (burst_len > PL330_MAX_BURST)
+		burst_len = PL330_MAX_BURST;
 
 	return burst_len;
 }
@@ -2596,7 +2639,7 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 
 		desc->rqtype = direction;
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2741,7 +2784,7 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		}
 
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->rqtype = direction;
 		desc->bytes_requested = sg_dma_len(sg);
 	}
Signature · 8 lines
2.11.0



--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#2 lore raw

PATCH v3 1/1 dmaengine: pl330: flush before wait, and add dev burst support.

Do DMAFLUSHP _before_ the first DMAWFP to ensure controller and peripheral are in agreement about dma request state before first transfer. Add support for burst transfers to/from peripherals. In the new scheme, the controller does as many burst transfers as it can then transfers the remaining dregs with either single transfers for peripherals, or with a reduced size burst for memory-to-memory transfers.

I tested dma transfers to peripherals with designware serial port (drivers/tty/serial/8250/8250_dw.c) and a GPIB interface
(https://github.com/fmhess/fmh_gpib_core). I tested memory-to-memory transfers using the dmatest module.

v3 of this patch should be the same as v2 except with checkpatch.pl warnings and errors cleaned up.

---
 drivers/dma/pl330.c | 146 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 104 insertions(+), 42 deletions(-)
drivers/dma/pl330.c +104 -42
index d7327fd5f445..cc2e4456bec1 100644
@@ -1094,26 +1094,33 @@ static inline int _ldst_memtomem(unsigned dry_run, u8 buf[],
 	return off;
 }
 
-static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
-				 u8 buf[], const struct _xfer_spec *pxs,
-				 int cyc)
+static inline int _ldst_devtomem(struct pl330_dmac *pl330,
+				unsigned int dry_run, u8 buf[],
+				const struct _xfer_spec *pxs,
+				int cyc, enum pl330_cond cond)
 {
 	int off = 0;
-	enum pl330_cond cond;
 
 	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
 		cond = BURST;
-	else
-		cond = SINGLE;
 
+	/* do FLUSHP at beginning to clear any stale dma requests before the
+	 * first WFP.
+	 */
+	if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
-		off += _emit_LDP(dry_run, &buf[off], cond, pxs->desc->peri);
+		if (cond == ALWAYS) {
+			off += _emit_LDP(dry_run, &buf[off], SINGLE,
+				pxs->desc->peri);
+			off += _emit_LDP(dry_run, &buf[off], BURST,
+				pxs->desc->peri);
+		} else {
+			off += _emit_LDP(dry_run, &buf[off], cond,
+				pxs->desc->peri);
+		}
 		off += _emit_ST(dry_run, &buf[off], ALWAYS);
-
-		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-			off += _emit_FLUSHP(dry_run, &buf[off],
-					    pxs->desc->peri);
 	}
 
 	return off;
@@ -1121,24 +1128,31 @@ static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
 
 static inline int _ldst_memtodev(struct pl330_dmac *pl330,
 				 unsigned dry_run, u8 buf[],
-				 const struct _xfer_spec *pxs, int cyc)
+				 const struct _xfer_spec *pxs, int cyc,
+				 enum pl330_cond cond)
 {
 	int off = 0;
-	enum pl330_cond cond;
 
 	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
 		cond = BURST;
-	else
-		cond = SINGLE;
 
+	/* do FLUSHP at beginning to clear any stale dma requests before the
+	 * first WFP.
+	 */
+	if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_LD(dry_run, &buf[off], ALWAYS);
-		off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
-
-		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-			off += _emit_FLUSHP(dry_run, &buf[off],
-					    pxs->desc->peri);
+		if (cond == ALWAYS) {
+			off += _emit_STP(dry_run, &buf[off], SINGLE,
+				pxs->desc->peri);
+			off += _emit_STP(dry_run, &buf[off], BURST,
+				pxs->desc->peri);
+		} else {
+			off += _emit_STP(dry_run, &buf[off], cond,
+				pxs->desc->peri);
+		}
 	}
 
 	return off;
@@ -1148,13 +1162,16 @@ static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
+	enum pl330_cond cond = BRST_LEN(pxs->ccr) > 1 ? BURST : SINGLE;
 
 	switch (pxs->desc->rqtype) {
 	case DMA_MEM_TO_DEV:
-		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, cyc);
+		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, cyc,
+			cond);
 		break;
 	case DMA_DEV_TO_MEM:
-		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, cyc);
+		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, cyc,
+			cond);
 		break;
 	case DMA_MEM_TO_MEM:
 		off += _ldst_memtomem(dry_run, &buf[off], pxs, cyc);
@@ -1167,6 +1184,46 @@ static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 	return off;
 }
 
+/* transfer dregs with single transfers to peripheral, or a reduced size burst
+ * for mem-to-mem.
+ */
+static int _dregs(struct pl330_dmac *pl330, unsigned int dry_run, u8 buf[],
+		const struct _xfer_spec *pxs, int transfer_length)
+{
+	int off = 0;
+	int dregs_ccr;
+
+	if (transfer_length == 0)
+		return off;
+
+	switch (pxs->desc->rqtype) {
+	case DMA_MEM_TO_DEV:
+		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs,
+			transfer_length, SINGLE);
+		break;
+	case DMA_DEV_TO_MEM:
+		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs,
+			transfer_length, SINGLE);
+		break;
+	case DMA_MEM_TO_MEM:
+		dregs_ccr = pxs->ccr;
+		dregs_ccr &= ~((0xf << CC_SRCBRSTLEN_SHFT) |
+			(0xf << CC_DSTBRSTLEN_SHFT));
+		dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+			CC_SRCBRSTLEN_SHFT);
+		dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+			CC_DSTBRSTLEN_SHFT);
+		off += _emit_MOV(dry_run, &buf[off], CCR, dregs_ccr);
+		off += _ldst_memtomem(dry_run, &buf[off], pxs, 1);
+		break;
+	default:
+		off += 0x40000000; /* Scare off the Client */
+		break;
+	}
+
+	return off;
+}
+
 /* Returns bytes consumed and updates bursts */
 static inline int _loop(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		unsigned long *bursts, const struct _xfer_spec *pxs)
@@ -1256,6 +1313,8 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 	struct pl330_xfer *x = &pxs->desc->px;
 	u32 ccr = pxs->ccr;
 	unsigned long c, bursts = BYTE_TO_BURST(x->bytes, ccr);
+	int num_dregs = (x->bytes - BURST_TO_BYTE(bursts, ccr)) /
+		BRST_SIZE(ccr);
 	int off = 0;
 
 	while (bursts) {
@@ -1263,6 +1322,7 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 		off += _loop(pl330, dry_run, &buf[off], &c, pxs);
 		bursts -= c;
 	}
+	off += _dregs(pl330, dry_run, &buf[off], pxs, num_dregs);
 
 	return off;
 }
@@ -1294,7 +1354,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 		      struct _xfer_spec *pxs)
 {
 	struct _pl330_req *req = &thrd->req[index];
-	struct pl330_xfer *x;
 	u8 *buf = req->mc_cpu;
 	int off = 0;
 
@@ -1303,11 +1362,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 	/* DMAMOV CCR, ccr */
 	off += _emit_MOV(dry_run, &buf[off], CCR, pxs->ccr);
 
-	x = &pxs->desc->px;
-	/* Error if xfer length is not aligned at burst size */
-	if (x->bytes % (BRST_SIZE(pxs->ccr) * BRST_LEN(pxs->ccr)))
-		return -EINVAL;
-
 	off += _setup_xfer(pl330, dry_run, &buf[off], pxs);
 
 	/* DMASEV peripheral/event */
@@ -2115,15 +2169,29 @@ static int pl330_config(struct dma_chan *chan,
 			pch->fifo_addr = slave_config->dst_addr;
 		if (slave_config->dst_addr_width)
 			pch->burst_sz = __ffs(slave_config->dst_addr_width);
-		if (slave_config->dst_maxburst)
-			pch->burst_len = slave_config->dst_maxburst;
+		if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+			pch->burst_len = 1;
+		else if (slave_config->dst_maxburst) {
+			if (slave_config->dst_maxburst > PL330_MAX_BURST)
+				pch->burst_len = PL330_MAX_BURST;
+			else
+				pch->burst_len = slave_config->dst_maxburst;
+		} else
+			pch->burst_len = 1;
 	} else if (slave_config->direction == DMA_DEV_TO_MEM) {
 		if (slave_config->src_addr)
 			pch->fifo_addr = slave_config->src_addr;
 		if (slave_config->src_addr_width)
 			pch->burst_sz = __ffs(slave_config->src_addr_width);
-		if (slave_config->src_maxburst)
-			pch->burst_len = slave_config->src_maxburst;
+		if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+			pch->burst_len = 1;
+		else if (slave_config->src_maxburst) {
+			if (slave_config->src_maxburst > PL330_MAX_BURST)
+				pch->burst_len = PL330_MAX_BURST;
+			else
+				pch->burst_len = slave_config->src_maxburst;
+		} else
+			pch->burst_len = 1;
 	}
 
 	return 0;
@@ -2517,14 +2585,8 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
 	burst_len >>= desc->rqcfg.brst_size;
 
 	/* src/dst_burst_len can't be more than 16 */
-	if (burst_len > 16)
-		burst_len = 16;
-
-	while (burst_len > 1) {
-		if (!(len % (burst_len << desc->rqcfg.brst_size)))
-			break;
-		burst_len--;
-	}
+	if (burst_len > PL330_MAX_BURST)
+		burst_len = PL330_MAX_BURST;
 
 	return burst_len;
 }
@@ -2596,7 +2658,7 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 
 		desc->rqtype = direction;
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2741,7 +2803,7 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		}
 
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->rqtype = direction;
 		desc->bytes_requested = sg_dma_len(sg);
 	}
#3 lore raw
Quoted 9 lines · Frank Mori Hess

Do DMAFLUSHP _before_ the first DMAWFP to ensure controller and peripheral are in agreement about dma request state before first transfer. Add support for burst transfers to/from peripherals. In the new scheme, the controller does as many burst transfers as it can then transfers the remaining dregs with either single transfers for peripherals, or with a reduced size burst for memory-to-memory transfers.

sorry if it wasnt clear earlier, the lines below should come after the --- line. git-am skips that part while applying..

Quoted 30 lines

I tested dma transfers to peripherals with designware serial port (drivers/tty/serial/8250/8250_dw.c) and a GPIB interface
(https://github.com/fmhess/fmh_gpib_core). I tested memory-to-memory transfers using the dmatest module.

v3 of this patch should be the same as v2 except with checkpatch.pl warnings and errors cleaned up.

---
 drivers/dma/pl330.c | 146 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 104 insertions(+), 42 deletions(-)
drivers/dma/pl330.c +4 -3
index d7327fd5f445..cc2e4456bec1 100644
@@ -1094,26 +1094,33 @@ static inline int _ldst_memtomem(unsigned dry_run, u8 buf[],
 	return off;
 }
 
-static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
-				 u8 buf[], const struct _xfer_spec *pxs,
-				 int cyc)
+static inline int _ldst_devtomem(struct pl330_dmac *pl330,
+				unsigned int dry_run, u8 buf[],
+				const struct _xfer_spec *pxs,
+				int cyc, enum pl330_cond cond)
 {
 	int off = 0;

drivers/dma/pl330.c static inline int _ldst_memtomem(unsigned dry_run, u8 buf[], · line 1109

Quoted 10 lines
drivers/dma/pl330.c +3 -3
-	enum pl330_cond cond;
 
 	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
 		cond = BURST;
-	else
-		cond = SINGLE;
 
+	/* do FLUSHP at beginning to clear any stale dma requests before the
+	 * first WFP.
+	 */

multiline comments should be:

/*
* this is an example of multi-line
* comment
*/

Pls fix at this and other places...

Quoted 26 lines
drivers/dma/pl330.c +7 -8
 static inline int _ldst_memtodev(struct pl330_dmac *pl330,
 				 unsigned dry_run, u8 buf[],
-				 const struct _xfer_spec *pxs, int cyc)
+				 const struct _xfer_spec *pxs, int cyc,
+				 enum pl330_cond cond)
 {
 	int off = 0;
-	enum pl330_cond cond;
 
 	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
 		cond = BURST;
-	else
-		cond = SINGLE;
 
+	/* do FLUSHP at beginning to clear any stale dma requests before the
+	 * first WFP.
+	 */
+	if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_LD(dry_run, &buf[off], ALWAYS);
-		off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
-
-		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-			off += _emit_FLUSHP(dry_run, &buf[off],

drivers/dma/pl330.c static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run, · line 1155

Quoted 10 lines
drivers/dma/pl330.c +9 -1
-					    pxs->desc->peri);
+		if (cond == ALWAYS) {
+			off += _emit_STP(dry_run, &buf[off], SINGLE,
+				pxs->desc->peri);
+			off += _emit_STP(dry_run, &buf[off], BURST,
+				pxs->desc->peri);
+		} else {
+			off += _emit_STP(dry_run, &buf[off], cond,
+				pxs->desc->peri);
+		}

this looks quite similar to previous routine above, if so can we please make it common function and invoke from both of these...

Quoted 4 lines
drivers/dma/pl330.c +4 -0
+static int _dregs(struct pl330_dmac *pl330, unsigned int dry_run, u8 buf[],
+		const struct _xfer_spec *pxs, int transfer_length)
+{
+	int off = 0;

drivers/dma/pl330.c static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[], · line 1203

Quoted 10 lines
drivers/dma/pl330.c +10 -0
+	int dregs_ccr;
+
+	if (transfer_length == 0)
+		return off;
+
+	switch (pxs->desc->rqtype) {
+	case DMA_MEM_TO_DEV:
+		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs,
+			transfer_length, SINGLE);
+		break;

empty line after each case pls

Quoted 7 lines
drivers/dma/pl330.c +7 -0
+	case DMA_DEV_TO_MEM:
+		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs,
+			transfer_length, SINGLE);
+		break;
+	case DMA_MEM_TO_MEM:
+		dregs_ccr = pxs->ccr;
+		dregs_ccr &= ~((0xf << CC_SRCBRSTLEN_SHFT) |

drivers/dma/pl330.c static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[], · line 1220

Quoted 10 lines
drivers/dma/pl330.c +10 -0
+			(0xf << CC_DSTBRSTLEN_SHFT));
+		dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+			CC_SRCBRSTLEN_SHFT);
+		dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+			CC_DSTBRSTLEN_SHFT);
+		off += _emit_MOV(dry_run, &buf[off], CCR, dregs_ccr);
+		off += _ldst_memtomem(dry_run, &buf[off], pxs, 1);
+		break;
+	default:
+		off += 0x40000000; /* Scare off the Client */

Can you explain this bit, shouldnt this be err?

Quoted 13 lines
drivers/dma/pl330.c +0 -5
@@ -1303,11 +1362,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 	/* DMAMOV CCR, ccr */
 	off += _emit_MOV(dry_run, &buf[off], CCR, pxs->ccr);
 
-	x = &pxs->desc->px;
-	/* Error if xfer length is not aligned at burst size */
-	if (x->bytes % (BRST_SIZE(pxs->ccr) * BRST_LEN(pxs->ccr)))
-		return -EINVAL;
-
 	off += _setup_xfer(pl330, dry_run, &buf[off], pxs);
 
 	/* DMASEV peripheral/event */

drivers/dma/pl330.c static int pl330_config(struct dma_chan *chan, · line 2173

Quoted 7 lines
drivers/dma/pl330.c +2 -2
@@ -2115,15 +2169,29 @@ static int pl330_config(struct dma_chan *chan,
 			pch->fifo_addr = slave_config->dst_addr;
 		if (slave_config->dst_addr_width)
 			pch->burst_sz = __ffs(slave_config->dst_addr_width);
-		if (slave_config->dst_maxburst)
-			pch->burst_len = slave_config->dst_maxburst;
+		if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+			pch->burst_len = 1;

so in this case we don't honour the requested burst length?

Quoted 13 lines
drivers/dma/pl330.c +7 -1
+		else if (slave_config->dst_maxburst) {
+			if (slave_config->dst_maxburst > PL330_MAX_BURST)
+				pch->burst_len = PL330_MAX_BURST;
+			else
+				pch->burst_len = slave_config->dst_maxburst;
+		} else
+			pch->burst_len = 1;
 	} else if (slave_config->direction == DMA_DEV_TO_MEM) {
 		if (slave_config->src_addr)
 			pch->fifo_addr = slave_config->src_addr;
 		if (slave_config->src_addr_width)
 			pch->burst_sz = __ffs(slave_config->src_addr_width);
-		if (slave_config->src_maxburst)

drivers/dma/pl330.c static int pl330_config(struct dma_chan *chan, · line 2194

Quoted 10 lines
drivers/dma/pl330.c +9 -1
-			pch->burst_len = slave_config->src_maxburst;
+		if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+			pch->burst_len = 1;
+		else if (slave_config->src_maxburst) {
+			if (slave_config->src_maxburst > PL330_MAX_BURST)
+				pch->burst_len = PL330_MAX_BURST;
+			else
+				pch->burst_len = slave_config->src_maxburst;
+		} else
+			pch->burst_len = 1;

again this looks duplicate..

Signature · 1 line
~Vinod
#4 lore raw
Quoted 3 lines · Vinod Koul

sorry if it wasnt clear earlier, the lines below should come after the --- line. git-am skips that part while applying..

ok

Quoted 5 lines

drivers/dma/pl330.c static inline int _ldst_memtomem(unsigned dry_run, u8 buf[], · line 1109

Quoted 4 lines
drivers/dma/pl330.c +3 -0
+     /* do FLUSHP at beginning to clear any stale dma requests before the
+      * first WFP.
+      */
Quoted 8 lines

multiline comments should be:

/*
* this is an example of multi-line
* comment
*/

Pls fix at this and other places...

ok

Quoted 38 lines
Quoted 27 lines

static inline int _ldst_memtodev(struct pl330_dmac *pl330,

                               unsigned dry_run, u8 buf[],
drivers/dma/pl330.c +7 -8
-                              const struct _xfer_spec *pxs, int cyc)
+                              const struct _xfer_spec *pxs, int cyc,
+                              enum pl330_cond cond)
 {
      int off = 0;
-     enum pl330_cond cond;
 
      if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
              cond = BURST;
-     else
-             cond = SINGLE;
 
+     /* do FLUSHP at beginning to clear any stale dma requests before the
+      * first WFP.
+      */
+     if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+             off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
      while (cyc--) {
              off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
              off += _emit_LD(dry_run, &buf[off], ALWAYS);
-             off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
-
-             if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-                     off += _emit_FLUSHP(dry_run, &buf[off],

drivers/dma/pl330.c static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run, · line 1155

Quoted 10 lines
drivers/dma/pl330.c +9 -1
-                                         pxs->desc->peri);
+             if (cond == ALWAYS) {
+                     off += _emit_STP(dry_run, &buf[off], SINGLE,
+                             pxs->desc->peri);
+                     off += _emit_STP(dry_run, &buf[off], BURST,
+                             pxs->desc->peri);
+             } else {
+                     off += _emit_STP(dry_run, &buf[off], cond,
+                             pxs->desc->peri);
+             }
Quoted 2 lines

this looks quite similar to previous routine above, if so can we please make it common function and invoke from both of these...

_ldst_memtodev and _ldst_devtomem are similar, but they were even more similar before my patch. Do I really have to refactor existing code to get my patch applied? I'm not trying to take over maintainership of the pl330.c driver.

Quoted 17 lines
Quoted 5 lines
drivers/dma/pl330.c +4 -0
+static int _dregs(struct pl330_dmac *pl330, unsigned int dry_run, u8 buf[],
+             const struct _xfer_spec *pxs, int transfer_length)
+{
+     int off = 0;

drivers/dma/pl330.c static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[], · line 1203

Quoted 10 lines
drivers/dma/pl330.c +10 -0
+     int dregs_ccr;
+
+     if (transfer_length == 0)
+             return off;
+
+     switch (pxs->desc->rqtype) {
+     case DMA_MEM_TO_DEV:
+             off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs,
+                     transfer_length, SINGLE);
+             break;

empty line after each case pls

ok

Quoted 19 lines
Quoted 8 lines
drivers/dma/pl330.c +7 -0
+     case DMA_DEV_TO_MEM:
+             off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs,
+                     transfer_length, SINGLE);
+             break;
+     case DMA_MEM_TO_MEM:
+             dregs_ccr = pxs->ccr;
+             dregs_ccr &= ~((0xf << CC_SRCBRSTLEN_SHFT) |

drivers/dma/pl330.c static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[], · line 1220

Quoted 10 lines
drivers/dma/pl330.c +10 -0
+                     (0xf << CC_DSTBRSTLEN_SHFT));
+             dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+                     CC_SRCBRSTLEN_SHFT);
+             dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+                     CC_DSTBRSTLEN_SHFT);
+             off += _emit_MOV(dry_run, &buf[off], CCR, dregs_ccr);
+             off += _ldst_memtomem(dry_run, &buf[off], pxs, 1);
+             break;
+     default:
+             off += 0x40000000; /* Scare off the Client */
Quoted 2 lines

Can you explain this bit, shouldnt this be err?

I just copied that behavior from the existing _bursts() function. I guess the original author's idea was returning a big offset would result in a dry run failure due to exceeding the maximum buffer size. I do agree an error would be better, although it would require refactoring since the return values from _bursts and _dregs are not checked for errors, they just blindly add the return value to the offset.

Quoted 11 lines
Quoted 11 lines
drivers/dma/pl330.c +0 -5
@@ -1303,11 +1362,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
      /* DMAMOV CCR, ccr */
      off += _emit_MOV(dry_run, &buf[off], CCR, pxs->ccr);
 
-     x = &pxs->desc->px;
-     /* Error if xfer length is not aligned at burst size */
-     if (x->bytes % (BRST_SIZE(pxs->ccr) * BRST_LEN(pxs->ccr)))
-             return -EINVAL;
-
      off += _setup_xfer(pl330, dry_run, &buf[off], pxs);
 
Quoted 12 lines

drivers/dma/pl330.c static int pl330_config(struct dma_chan *chan, · line 2173

Quoted 9 lines
drivers/dma/pl330.c +2 -2
      /* DMASEV peripheral/event */
@@ -2115,15 +2169,29 @@ static int pl330_config(struct dma_chan *chan,
                      pch->fifo_addr = slave_config->dst_addr;
              if (slave_config->dst_addr_width)
                      pch->burst_sz = __ffs(slave_config->dst_addr_width);
-             if (slave_config->dst_maxburst)
-                     pch->burst_len = slave_config->dst_maxburst;
+             if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+                     pch->burst_len = 1;

so in this case we don't honour the requested burst length?

I don't have any experience with the PL330_QUIRK_BROKEN_NO_FLUSHP hardware, or knowledge of how it (mis)behaves. So I just tried to maintain the existing behavior as much as possible. The old code advertised the max burst length as 1 with PL330_QUIRK_BROKEN_NO_FLUSHP hardware, and programmed the pl330 to respond only to burst requests (and of course the old code never did any peripheral bursts longer than 1). I actually don't mind changing the behavior of PL330_QUIRK_BROKEN_NO_FLUSHP but it seems like someone with more knowledge of that hardware should be involved, like the rock-chips.com guys who introduced it in commit 271e1b86e69140fe65718ae8a264284c46d3129d ,

Quoted 24 lines
Quoted 13 lines
drivers/dma/pl330.c +7 -1
+             else if (slave_config->dst_maxburst) {
+                     if (slave_config->dst_maxburst > PL330_MAX_BURST)
+                             pch->burst_len = PL330_MAX_BURST;
+                     else
+                             pch->burst_len = slave_config->dst_maxburst;
+             } else
+                     pch->burst_len = 1;
      } else if (slave_config->direction == DMA_DEV_TO_MEM) {
              if (slave_config->src_addr)
                      pch->fifo_addr = slave_config->src_addr;
              if (slave_config->src_addr_width)
                      pch->burst_sz = __ffs(slave_config->src_addr_width);
-             if (slave_config->src_maxburst)

drivers/dma/pl330.c static int pl330_config(struct dma_chan *chan, · line 2194

Quoted 10 lines
drivers/dma/pl330.c +9 -1
-                     pch->burst_len = slave_config->src_maxburst;
+             if (pch->dmac->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+                     pch->burst_len = 1;
+             else if (slave_config->src_maxburst) {
+                     if (slave_config->src_maxburst > PL330_MAX_BURST)
+                             pch->burst_len = PL330_MAX_BURST;
+                     else
+                             pch->burst_len = slave_config->src_maxburst;
+             } else
+                     pch->burst_len = 1;
Quoted 3 lines

again this looks duplicate..
--
~Vinod

Ok, I'll make a little helper function.

#5 lore raw

PATCH v4 1/1 dmaengine: pl330: flush before wait, and add dev burst support.

In reply to Vinod Koul

Do DMAFLUSHP _before_ the first DMAWFP to ensure controller and peripheral are in agreement about dma request state before first transfer. Add support for burst transfers to/from peripherals. In the new scheme, the controller does as many burst transfers as it can then transfers the remaining dregs with either single transfers for peripherals, or with a reduced size burst for memory-to-memory transfers.

---

I tested dma transfers to peripherals with v3 patch and designware serial port (drivers/tty/serial/8250/8250_dw.c) and a GPIB interface
(https://github.com/fmhess/fmh_gpib_core). I tested memory-to-memory transfers using the dmatest module.

v3 of this patch should be the same as v2 except with checkpatch.pl warnings and errors cleaned up.

v4 addresses cosmetic complaints about v3, should be functionally unchanged.

 drivers/dma/pl330.c | 209 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 159 insertions(+), 50 deletions(-)
drivers/dma/pl330.c +159 -50
index d7327fd5f445..819a578e317f 100644
@@ -27,6 +27,7 @@
 #include <linux/of_dma.h>
 #include <linux/err.h>
 #include <linux/pm_runtime.h>
+#include <linux/bug.h>
 
 #include "dmaengine.h"
 #define PL330_MAX_CHAN		8
@@ -1094,51 +1095,96 @@ static inline int _ldst_memtomem(unsigned dry_run, u8 buf[],
 	return off;
 }
 
-static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
-				 u8 buf[], const struct _xfer_spec *pxs,
-				 int cyc)
+static u32 _emit_load(unsigned int dry_run, u8 buf[],
+	enum pl330_cond cond, enum dma_transfer_direction direction,
+	u8 peri)
 {
 	int off = 0;
-	enum pl330_cond cond;
 
-	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
-		cond = BURST;
-	else
-		cond = SINGLE;
+	switch (direction) {
+	case DMA_MEM_TO_MEM:
+		/* fall through */
+	case DMA_MEM_TO_DEV:
+		off += _emit_LD(dry_run, &buf[off], cond);
+		break;
 
-	while (cyc--) {
-		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
-		off += _emit_LDP(dry_run, &buf[off], cond, pxs->desc->peri);
-		off += _emit_ST(dry_run, &buf[off], ALWAYS);
+	case DMA_DEV_TO_MEM:
+		if (cond == ALWAYS) {
+			off += _emit_LDP(dry_run, &buf[off], SINGLE,
+				peri);
+			off += _emit_LDP(dry_run, &buf[off], BURST,
+				peri);
+		} else {
+			off += _emit_LDP(dry_run, &buf[off], cond,
+				peri);
+		}
+		break;
 
-		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-			off += _emit_FLUSHP(dry_run, &buf[off],
-					    pxs->desc->peri);
+	default:
+		/* this code should be unreachable */
+		WARN_ON(1);
+		break;
 	}
 
 	return off;
 }
 
-static inline int _ldst_memtodev(struct pl330_dmac *pl330,
+static inline u32 _emit_store(unsigned int dry_run, u8 buf[],
+	enum pl330_cond cond, enum dma_transfer_direction direction,
+	u8 peri)
+{
+	int off = 0;
+
+	switch (direction) {
+	case DMA_MEM_TO_MEM:
+		/* fall through */
+	case DMA_DEV_TO_MEM:
+		off += _emit_ST(dry_run, &buf[off], cond);
+		break;
+
+	case DMA_MEM_TO_DEV:
+		if (cond == ALWAYS) {
+			off += _emit_STP(dry_run, &buf[off], SINGLE,
+				peri);
+			off += _emit_STP(dry_run, &buf[off], BURST,
+				peri);
+		} else {
+			off += _emit_STP(dry_run, &buf[off], cond,
+				peri);
+		}
+		break;
+
+	default:
+		/* this code should be unreachable */
+		WARN_ON(1);
+		break;
+	}
+
+	return off;
+}
+
+static inline int _ldst_peripheral(struct pl330_dmac *pl330,
 				 unsigned dry_run, u8 buf[],
-				 const struct _xfer_spec *pxs, int cyc)
+				 const struct _xfer_spec *pxs, int cyc,
+				 enum pl330_cond cond)
 {
 	int off = 0;
-	enum pl330_cond cond;
 
 	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
 		cond = BURST;
-	else
-		cond = SINGLE;
 
+	/*
+	 * do FLUSHP at beginning to clear any stale dma requests before the
+	 * first WFP.
+	 */
+	if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
-		off += _emit_LD(dry_run, &buf[off], ALWAYS);
-		off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
-
-		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
-			off += _emit_FLUSHP(dry_run, &buf[off],
-					    pxs->desc->peri);
+		off += _emit_load(dry_run, &buf[off], cond, pxs->desc->rqtype,
+			pxs->desc->peri);
+		off += _emit_store(dry_run, &buf[off], cond, pxs->desc->rqtype,
+			pxs->desc->peri);
 	}
 
 	return off;
@@ -1148,19 +1194,65 @@ static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
+	enum pl330_cond cond = BRST_LEN(pxs->ccr) > 1 ? BURST : SINGLE;
 
 	switch (pxs->desc->rqtype) {
 	case DMA_MEM_TO_DEV:
-		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, cyc);
-		break;
+		/* fall through */
 	case DMA_DEV_TO_MEM:
-		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, cyc);
+		off += _ldst_peripheral(pl330, dry_run, &buf[off], pxs, cyc,
+			cond);
 		break;
+
 	case DMA_MEM_TO_MEM:
 		off += _ldst_memtomem(dry_run, &buf[off], pxs, cyc);
 		break;
+
+	default:
+		/* this code should be unreachable */
+		WARN_ON(1);
+		break;
+	}
+
+	return off;
+}
+
+/*
+ * transfer dregs with single transfers to peripheral, or a reduced size burst
+ * for mem-to-mem.
+ */
+static int _dregs(struct pl330_dmac *pl330, unsigned int dry_run, u8 buf[],
+		const struct _xfer_spec *pxs, int transfer_length)
+{
+	int off = 0;
+	int dregs_ccr;
+
+	if (transfer_length == 0)
+		return off;
+
+	switch (pxs->desc->rqtype) {
+	case DMA_MEM_TO_DEV:
+		/* fall through */
+	case DMA_DEV_TO_MEM:
+		off += _ldst_peripheral(pl330, dry_run, &buf[off], pxs,
+			transfer_length, SINGLE);
+		break;
+
+	case DMA_MEM_TO_MEM:
+		dregs_ccr = pxs->ccr;
+		dregs_ccr &= ~((0xf << CC_SRCBRSTLEN_SHFT) |
+			(0xf << CC_DSTBRSTLEN_SHFT));
+		dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+			CC_SRCBRSTLEN_SHFT);
+		dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+			CC_DSTBRSTLEN_SHFT);
+		off += _emit_MOV(dry_run, &buf[off], CCR, dregs_ccr);
+		off += _ldst_memtomem(dry_run, &buf[off], pxs, 1);
+		break;
+
 	default:
-		off += 0x40000000; /* Scare off the Client */
+		/* this code should be unreachable */
+		WARN_ON(1);
 		break;
 	}
 
@@ -1256,6 +1348,8 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 	struct pl330_xfer *x = &pxs->desc->px;
 	u32 ccr = pxs->ccr;
 	unsigned long c, bursts = BYTE_TO_BURST(x->bytes, ccr);
+	int num_dregs = (x->bytes - BURST_TO_BYTE(bursts, ccr)) /
+		BRST_SIZE(ccr);
 	int off = 0;
 
 	while (bursts) {
@@ -1263,6 +1357,7 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 		off += _loop(pl330, dry_run, &buf[off], &c, pxs);
 		bursts -= c;
 	}
+	off += _dregs(pl330, dry_run, &buf[off], pxs, num_dregs);
 
 	return off;
 }
@@ -1294,7 +1389,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 		      struct _xfer_spec *pxs)
 {
 	struct _pl330_req *req = &thrd->req[index];
-	struct pl330_xfer *x;
 	u8 *buf = req->mc_cpu;
 	int off = 0;
 
@@ -1303,11 +1397,6 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 	/* DMAMOV CCR, ccr */
 	off += _emit_MOV(dry_run, &buf[off], CCR, pxs->ccr);
 
-	x = &pxs->desc->px;
-	/* Error if xfer length is not aligned at burst size */
-	if (x->bytes % (BRST_SIZE(pxs->ccr) * BRST_LEN(pxs->ccr)))
-		return -EINVAL;
-
 	off += _setup_xfer(pl330, dry_run, &buf[off], pxs);
 
 	/* DMASEV peripheral/event */
@@ -1365,6 +1454,20 @@ static int pl330_submit_req(struct pl330_thread *thrd,
 	u32 ccr;
 	int ret = 0;
 
+	switch (desc->rqtype) {
+	case DMA_MEM_TO_DEV:
+		break;
+
+	case DMA_DEV_TO_MEM:
+		break;
+
+	case DMA_MEM_TO_MEM:
+		break;
+
+	default:
+		return -ENOTSUPP;
+	}
+
 	if (pl330->state == DYING
 		|| pl330->dmac_tbd.reset_chan & (1 << thrd->id)) {
 		dev_info(thrd->dmac->ddma.dev, "%s:%d\n",
@@ -2104,6 +2207,18 @@ static bool pl330_prep_slave_fifo(struct dma_pl330_chan *pch,
 	return true;
 }
 
+static int fixup_burst_len(int max_burst_len, int quirks)
+{
+	if (quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+		return 1;
+	else if (max_burst_len > PL330_MAX_BURST)
+		return PL330_MAX_BURST;
+	else if (max_burst_len < 1)
+		return 1;
+	else
+		return max_burst_len;
+}
+
 static int pl330_config(struct dma_chan *chan,
 			struct dma_slave_config *slave_config)
 {
@@ -2115,15 +2230,15 @@ static int pl330_config(struct dma_chan *chan,
 			pch->fifo_addr = slave_config->dst_addr;
 		if (slave_config->dst_addr_width)
 			pch->burst_sz = __ffs(slave_config->dst_addr_width);
-		if (slave_config->dst_maxburst)
-			pch->burst_len = slave_config->dst_maxburst;
+		pch->burst_len = fixup_burst_len(slave_config->dst_maxburst,
+			pch->dmac->quirks);
 	} else if (slave_config->direction == DMA_DEV_TO_MEM) {
 		if (slave_config->src_addr)
 			pch->fifo_addr = slave_config->src_addr;
 		if (slave_config->src_addr_width)
 			pch->burst_sz = __ffs(slave_config->src_addr_width);
-		if (slave_config->src_maxburst)
-			pch->burst_len = slave_config->src_maxburst;
+		pch->burst_len = fixup_burst_len(slave_config->src_maxburst,
+			pch->dmac->quirks);
 	}
 
 	return 0;
@@ -2517,14 +2632,8 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
 	burst_len >>= desc->rqcfg.brst_size;
 
 	/* src/dst_burst_len can't be more than 16 */
-	if (burst_len > 16)
-		burst_len = 16;
-
-	while (burst_len > 1) {
-		if (!(len % (burst_len << desc->rqcfg.brst_size)))
-			break;
-		burst_len--;
-	}
+	if (burst_len > PL330_MAX_BURST)
+		burst_len = PL330_MAX_BURST;
 
 	return burst_len;
 }
@@ -2596,7 +2705,7 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 
 		desc->rqtype = direction;
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2741,7 +2850,7 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		}
 
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->rqtype = direction;
 		desc->bytes_requested = sg_dma_len(sg);
 	}
#6 lore raw
Quoted 6 lines · Frank Mori Hess

Do DMAFLUSHP _before_ the first DMAWFP to ensure controller and peripheral are in agreement about dma request state before first transfer. Add support for burst transfers to/from peripherals. In the new scheme, the controller does as many burst transfers as it can then transfers the remaining dregs with either single transfers for peripherals, or with a reduced size burst for memory-to-memory transfers.

Hi, what is the state of this patch? I just noticed in patchwork it is now listed as "Not applicable"? The original broken-by-wordwrap patch is listed as "Accepted"?

https://patchwork.kernel.org/project/linux-dmaengine/list/?submitter=178687&state=*

I found it has a bug handling dregs btw, I'll include a fix for that in the next version or as a follow-on patch as appropriate.
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw
Quoted 8 lines · Frank Mori Hess
Quoted 6 lines · Frank Mori Hess

Do DMAFLUSHP _before_ the first DMAWFP to ensure controller and peripheral are in agreement about dma request state before first transfer. Add support for burst transfers to/from peripherals. In the new scheme, the controller does as many burst transfers as it can then transfers the remaining dregs with either single transfers for peripherals, or with a reduced size burst for memory-to-memory transfers.

Quoted 5 lines

Hi, what is the state of this patch? I just noticed in patchwork it is now listed as "Not applicable"? The original broken-by-wordwrap patch is listed as "Accepted"?

https://patchwork.kernel.org/project/linux-dmaengine/list/?submitter=178687&state=*

That is not correct state, my script would update as not applicable when it doesn't find patch in my queue..

Quoted 2 lines

I found it has a bug handling dregs btw, I'll include a fix for that in the next version or as a follow-on patch as appropriate.

Looks like I have missed this one somehow, so please update the patch with fix and I shall look into it.

Thanks

Signature · 1 line
~Vinod
#8 lore raw
Quoted 7 lines · Vinod Koul
Quoted 6 lines

Hi, what is the state of this patch? I just noticed in patchwork it is now listed as "Not applicable"? The original broken-by-wordwrap patch is listed as "Accepted"?

https://patchwork.kernel.org/project/linux-dmaengine/list/?submitter=178687&state=*

Quoted 9 lines

That is not correct state, my script would update as not applicable when it doesn't find patch in my queue..

Quoted 2 lines

I found it has a bug handling dregs btw, I'll include a fix for that in the next version or as a follow-on patch as appropriate.

Looks like I have missed this one somehow, so please update the patch with fix and I shall look into it.

Ok, I tested the v4 patch and it turns out it was a false alarm about the bug in handling dregs. It works fine. I've registered in the dmaengine patchwork, and changed the state of the v4 patch back to "new".

#9 lore raw
Quoted 14 lines · Frank Mori Hess
Quoted 13 lines · Vinod Koul
Quoted 6 lines

Hi, what is the state of this patch? I just noticed in patchwork it is now listed as "Not applicable"? The original broken-by-wordwrap patch is listed as "Accepted"?

https://patchwork.kernel.org/project/linux-dmaengine/list/?submitter=178687&state=*

That is not correct state, my script would update as not applicable when it doesn't find patch in my queue..

Quoted 2 lines

I found it has a bug handling dregs btw, I'll include a fix for that in the next version or as a follow-on patch as appropriate.

Quoted 8 lines
Quoted 3 lines

Looks like I have missed this one somehow, so please update the patch with fix and I shall look into it.

Ok, I tested the v4 patch and it turns out it was a false alarm about the bug in handling dregs. It works fine. I've registered in the dmaengine patchwork, and changed the state of the v4 patch back to "new".

Can you please repost this one after rebasing to dmaengine -next

Signature · 1 line
~Vinod