[PATCH 0/7] dmaengine: pl330: Updates and new quirks for peripheral usecases

[PATCH 0/7] dmaengine: pl330: Updates and new quirks for peripheral usecases

[Joy Chakraborty]

This patch series makes some initial minor and cosmetic changes:
    -Add variables and logic to handle separate source and destination
    AxSize and AxLen.
    -Use __ffs to calculate AxSize for consistency in the driver
    -Use switch-case in prep_slave_sg() for consistency
    -Change args get_burst_len() to remove redundant "len" and add
    burst_size so that it can be used in multiple places.

to majorly enable addition of 2 quirks in the last 2 patches:
    -Addition of a quirk to allow transactions towards memory to use the
    maximum possible bus width (AxSize) during a memory to peripheral
    dma usage or vise-versa.
    -Addition of a quirk which makes PL330 copy left over data after
    executing bursts to the peripheral in singles instead of bursts.
    -Update dt-bindings to represent the quirks
Quirks are explained further in the commit text.
---

Joy Chakraborty (7):
  dmaengine: pl330: Separate SRC and DST burst size and len
  dmaengine: pl330: Use FFS to calculate burst size
  dmaengine: pl330: Change if-else to switch-case for consistency
  dmaengine: pl330: Change unused arg "len" from get_burst_len()
  dmaengine: pl330: Quirk to optimize AxSize for peripheral usecases
  dmaengine: pl330: Quirk to use DMA singles for peripheral _dregs
  dt-bindings: dmaengine: pl330: Add new quirks

 .../devicetree/bindings/dma/arm,pl330.yaml    |   8 +
 drivers/dma/pl330.c                           | 245 +++++++++++++++---
 2 files changed, 213 insertions(+), 40 deletions(-)

-- 
2.40.1.495.gc816e09b53d-goog

[PATCH 1/7] dmaengine: pl330: Separate SRC and DST burst size and len

[Joy Chakraborty]

Add new variables in request configuration to handle source and
destination AxSize and AxLen separately and allow them to have different
values.

This allows further patches to configure different AxSize and AxLen for
optimum bus utilisation.

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/dma/pl330.c | 71 +++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 0d9257fbdfb0..c006e481b4c5 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -240,6 +240,12 @@ enum pl330_byteswap {
 #define BYTE_TO_BURST(b, ccr)	((b) / BRST_SIZE(ccr) / BRST_LEN(ccr))
 #define BURST_TO_BYTE(c, ccr)	((c) * BRST_SIZE(ccr) * BRST_LEN(ccr))
 
+#define SRC_BRST_SIZE(ccr)	BRST_SIZE(ccr)
+#define DST_BRST_SIZE(ccr)	(1 << (((ccr) >> CC_DSTBRSTSIZE_SHFT) & 0x7))
+
+#define SRC_BRST_LEN(ccr)	BRST_LEN(ccr)
+#define DST_BRST_LEN(ccr)	((((ccr) >> CC_DSTBRSTLEN_SHFT) & 0xf) + 1)
+
 /*
  * With 256 bytes, we can do more than 2.5MB and 5MB xfers per req
  * at 1byte/burst for P<->M and M<->M respectively.
@@ -305,8 +311,10 @@ struct pl330_reqcfg {
 	bool nonsecure;
 	bool privileged;
 	bool insnaccess;
-	unsigned brst_len:5;
-	unsigned brst_size:3; /* in power of 2 */
+	unsigned src_brst_size : 3; /* in power of 2 */
+	unsigned src_brst_len:5;
+	unsigned dst_brst_size : 3; /* in power of 2 */
+	unsigned dst_brst_len:5;
 
 	enum pl330_cachectrl dcctl;
 	enum pl330_cachectrl scctl;
@@ -1204,7 +1212,10 @@ 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;
+	enum pl330_cond cond = SINGLE;
+
+	if (SRC_BRST_LEN(pxs->ccr) > 1 || DST_BRST_LEN(pxs->ccr) > 1)
+		cond = BURST;
 
 	if (pl330->quirks & PL330_QUIRK_PERIPH_BURST)
 		cond = BURST;
@@ -1235,12 +1246,12 @@ static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
  * for mem-to-mem, mem-to-dev or dev-to-mem.
  */
 static int _dregs(struct pl330_dmac *pl330, unsigned int dry_run, u8 buf[],
-		const struct _xfer_spec *pxs, int transfer_length)
+		const struct _xfer_spec *pxs, int src_length, int dst_length)
 {
 	int off = 0;
 	int dregs_ccr;
 
-	if (transfer_length == 0)
+	if (src_length == 0 || dst_length == 0)
 		return off;
 
 	/*
@@ -1253,9 +1264,9 @@ static int _dregs(struct pl330_dmac *pl330, unsigned int dry_run, u8 buf[],
 	dregs_ccr = pxs->ccr;
 	dregs_ccr &= ~((0xf << CC_SRCBRSTLEN_SHFT) |
 		(0xf << CC_DSTBRSTLEN_SHFT));
-	dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+	dregs_ccr |= (((src_length - 1) & 0xf) <<
 		CC_SRCBRSTLEN_SHFT);
-	dregs_ccr |= (((transfer_length - 1) & 0xf) <<
+	dregs_ccr |= (((dst_length - 1) & 0xf) <<
 		CC_DSTBRSTLEN_SHFT);
 
 	switch (pxs->desc->rqtype) {
@@ -1369,16 +1380,18 @@ 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;
+	int num_dreg_bytes = x->bytes - BURST_TO_BYTE(bursts, ccr);
+	int num_src_dregs, num_dst_dregs, off = 0;
+
+	num_src_dregs = num_dreg_bytes / SRC_BRST_SIZE(ccr);
+	num_dst_dregs = num_dreg_bytes / DST_BRST_SIZE(ccr);
 
 	while (bursts) {
 		c = bursts;
 		off += _loop(pl330, dry_run, &buf[off], &c, pxs);
 		bursts -= c;
 	}
-	off += _dregs(pl330, dry_run, &buf[off], pxs, num_dregs);
+	off += _dregs(pl330, dry_run, &buf[off], pxs, num_src_dregs, num_dst_dregs);
 
 	return off;
 }
@@ -1446,11 +1459,11 @@ static inline u32 _prepare_ccr(const struct pl330_reqcfg *rqc)
 	if (rqc->insnaccess)
 		ccr |= CC_SRCIA | CC_DSTIA;
 
-	ccr |= (((rqc->brst_len - 1) & 0xf) << CC_SRCBRSTLEN_SHFT);
-	ccr |= (((rqc->brst_len - 1) & 0xf) << CC_DSTBRSTLEN_SHFT);
+	ccr |= (((rqc->src_brst_len - 1) & 0xf) << CC_SRCBRSTLEN_SHFT);
+	ccr |= (((rqc->dst_brst_len - 1) & 0xf) << CC_DSTBRSTLEN_SHFT);
 
-	ccr |= (rqc->brst_size << CC_SRCBRSTSIZE_SHFT);
-	ccr |= (rqc->brst_size << CC_DSTBRSTSIZE_SHFT);
+	ccr |= (rqc->src_brst_size << CC_SRCBRSTSIZE_SHFT);
+	ccr |= (rqc->dst_brst_size << CC_DSTBRSTSIZE_SHFT);
 
 	ccr |= (rqc->scctl << CC_SRCCCTRL_SHFT);
 	ccr |= (rqc->dcctl << CC_DSTCCTRL_SHFT);
@@ -2656,7 +2669,7 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
 
 	burst_len = pl330->pcfg.data_bus_width / 8;
 	burst_len *= pl330->pcfg.data_buf_dep / pl330->pcfg.num_chan;
-	burst_len >>= desc->rqcfg.brst_size;
+	burst_len >>= desc->rqcfg.src_brst_size;
 
 	/* src/dst_burst_len can't be more than 16 */
 	if (burst_len > PL330_MAX_BURST)
@@ -2735,8 +2748,10 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 		}
 
 		desc->rqtype = direction;
-		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = pch->burst_len;
+		desc->rqcfg.src_brst_size = pch->burst_sz;
+		desc->rqcfg.src_brst_len = pch->burst_len;
+		desc->rqcfg.dst_brst_size = pch->burst_sz;
+		desc->rqcfg.dst_brst_len = pch->burst_len;
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2789,17 +2804,21 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 	while ((src | dst | len) & (burst - 1))
 		burst /= 2;
 
-	desc->rqcfg.brst_size = 0;
-	while (burst != (1 << desc->rqcfg.brst_size))
-		desc->rqcfg.brst_size++;
+	desc->rqcfg.src_brst_size = 0;
+	while (burst != (1 << desc->rqcfg.src_brst_size))
+		desc->rqcfg.src_brst_size++;
 
-	desc->rqcfg.brst_len = get_burst_len(desc, len);
+	desc->rqcfg.src_brst_len = get_burst_len(desc, len);
 	/*
 	 * If burst size is smaller than bus width then make sure we only
 	 * transfer one at a time to avoid a burst stradling an MFIFO entry.
 	 */
 	if (burst * 8 < pl330->pcfg.data_bus_width)
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.src_brst_len = 1;
+
+	/* For Mem2Mem, set destination AxSize and AxLen same as source*/
+	desc->rqcfg.dst_brst_len = desc->rqcfg.src_brst_len;
+	desc->rqcfg.dst_brst_size = desc->rqcfg.src_brst_size;
 
 	desc->bytes_requested = len;
 
@@ -2879,8 +2898,10 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 				sg_dma_len(sg));
 		}
 
-		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = pch->burst_len;
+		desc->rqcfg.src_brst_size = pch->burst_sz;
+		desc->rqcfg.src_brst_len = pch->burst_len;
+		desc->rqcfg.dst_brst_size = pch->burst_sz;
+		desc->rqcfg.dst_brst_len = pch->burst_len;
 		desc->rqtype = direction;
 		desc->bytes_requested = sg_dma_len(sg);
 	}
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH 2/7] dmaengine: pl330: Use FFS to calculate burst size

[Joy Chakraborty]

Use __ffs to calculate burst size in pl330_prep_dma_memcpy() for
consistency across the driver as other functions already use __ffs for
the same functionality.

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/dma/pl330.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index c006e481b4c5..39a66ff29e27 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2804,10 +2804,7 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 	while ((src | dst | len) & (burst - 1))
 		burst /= 2;
 
-	desc->rqcfg.src_brst_size = 0;
-	while (burst != (1 << desc->rqcfg.src_brst_size))
-		desc->rqcfg.src_brst_size++;
-
+	desc->rqcfg.src_brst_size = __ffs(burst);
 	desc->rqcfg.src_brst_len = get_burst_len(desc, len);
 	/*
 	 * If burst size is smaller than bus width then make sure we only
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH 3/7] dmaengine: pl330: Change if-else to switch-case for consistency

[Joy Chakraborty]

Change if-else to switch-case in pl330_prep_slave_sg() function for
consistency with other peripheral transfer functions in the driver.

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/dma/pl330.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 39a66ff29e27..746da0bbea92 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2883,16 +2883,21 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		else
 			list_add_tail(&desc->node, &first->node);
 
-		if (direction == DMA_MEM_TO_DEV) {
+		switch (direction) {
+		case DMA_MEM_TO_DEV:
 			desc->rqcfg.src_inc = 1;
 			desc->rqcfg.dst_inc = 0;
 			fill_px(&desc->px, pch->fifo_dma, sg_dma_address(sg),
 				sg_dma_len(sg));
-		} else {
+			break;
+		case DMA_DEV_TO_MEM:
 			desc->rqcfg.src_inc = 0;
 			desc->rqcfg.dst_inc = 1;
 			fill_px(&desc->px, sg_dma_address(sg), pch->fifo_dma,
 				sg_dma_len(sg));
+			break;
+		default:
+			break;
 		}
 
 		desc->rqcfg.src_brst_size = pch->burst_sz;
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH 4/7] dmaengine: pl330: Change unused arg "len" from get_burst_len()

[Joy Chakraborty]

Remove unused length argument from get_burst_len() and add burst size as
an argument to allow usage of this function in other places as source and
destination burst sizes are handled separately.

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/dma/pl330.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 746da0bbea92..e5e610c91f18 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2660,8 +2660,7 @@ __pl330_prep_dma_memcpy(struct dma_pl330_chan *pch, dma_addr_t dst,
 	return desc;
 }
 
-/* Call after fixing burst size */
-static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
+static inline int get_burst_len(struct dma_pl330_desc *desc, unsigned int brst_size)
 {
 	struct dma_pl330_chan *pch = desc->pchan;
 	struct pl330_dmac *pl330 = pch->dmac;
@@ -2669,7 +2668,7 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
 
 	burst_len = pl330->pcfg.data_bus_width / 8;
 	burst_len *= pl330->pcfg.data_buf_dep / pl330->pcfg.num_chan;
-	burst_len >>= desc->rqcfg.src_brst_size;
+	burst_len >>= brst_size;
 
 	/* src/dst_burst_len can't be more than 16 */
 	if (burst_len > PL330_MAX_BURST)
@@ -2805,7 +2804,7 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 		burst /= 2;
 
 	desc->rqcfg.src_brst_size = __ffs(burst);
-	desc->rqcfg.src_brst_len = get_burst_len(desc, len);
+	desc->rqcfg.src_brst_len = get_burst_len(desc, desc->rqcfg.src_brst_size);
 	/*
 	 * If burst size is smaller than bus width then make sure we only
 	 * transfer one at a time to avoid a burst stradling an MFIFO entry.
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH 5/7] dmaengine: pl330: Quirk to optimize AxSize for peripheral usecases

[Joy Chakraborty]

Add quirk "arm,pl330-optimize-dev2mem-axsize" to choose maximum possible
AxSize for transactions towards memory during usecases which copy data
between memory and peripherals.

Currently PL330 driver chooses equal AxLen and AxSize for both loads and
stores to/from memory and peripherals which is inefficient towards
memory as the whole bus width is not used for transfers as a peripheral
might be limited to use only a narrow size of the buswidth available.

Example scenario:
    A peripheral might require data byte by byte which would  make AxSize
    = 1 byte and AxLen = 16 for both load from memory and store to
    Peripheral.
    This can be optimized for memory by using maximum AxSize (say
    16bytes) then load from memory can be done with AxSize = 16byte,
    AxLen = 1 and store to peripheral with AxSize = 1byte, AxLen =
    16 beats.

Instruction setup with quirk :
    512bytes copy from Memory(16bytes * 4beats) to Peripheral(4bytes *
    16 beats)
    ---
    DMAMOV CCR 0xbd0239
    DMAMOV SAR 0xffffe000
    DMAMOV DAR 0xffffc860
    DMALP_1 7
    DMAFLUSHP 0
    DMAWFPB 0
    DMALDB
    DMASTPB 0
    DMALPENDA_1 bjmpto_7
    DMASEV 3
    DMAEND
    ---

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/dma/pl330.c | 105 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 89 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index e5e610c91f18..b4933fab8a62 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -35,6 +35,7 @@
 
 #define PL330_QUIRK_BROKEN_NO_FLUSHP	BIT(0)
 #define PL330_QUIRK_PERIPH_BURST	BIT(1)
+#define PL330_QUIRK_OPTIMIZE_DEV2MEM_AXSIZE	BIT(2)
 
 enum pl330_cachectrl {
 	CCTRL0,		/* Noncacheable and nonbufferable */
@@ -519,6 +520,10 @@ static struct pl330_of_quirks {
 	{
 		.quirk = "arm,pl330-periph-burst",
 		.id = PL330_QUIRK_PERIPH_BURST,
+	},
+	{
+		.quirk = "arm,pl330-optimize-dev2mem-axsize",
+		.id = PL330_QUIRK_OPTIMIZE_DEV2MEM_AXSIZE,
 	}
 };
 
@@ -2677,6 +2682,56 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, unsigned int brst_s
 	return burst_len;
 }
 
+/*
+ * Returns burst size to be used to copy data from/to memory during a
+ * peripheral transfer
+ */
+static unsigned int get_periph_mem_brst_sz(dma_addr_t addr, size_t len,
+					   struct dma_pl330_chan *pch, int quirks)
+{
+	unsigned int burst, burst_size = pch->burst_sz;
+
+	if (quirks & PL330_QUIRK_OPTIMIZE_DEV2MEM_AXSIZE) {
+		/* Select max possible burst size */
+		burst = pch->dmac->pcfg.data_bus_width / 8;
+
+		/*
+		 * Make sure we use a burst size that aligns with the memory and length.
+		 */
+		while ((addr | len) & (burst - 1))
+			burst /= 2;
+
+		burst_size = __ffs(burst);
+	}
+	return burst_size;
+}
+
+/*
+ * Returns burst length to be used to copy data from/to memory during a
+ * peripheral transfer
+ */
+static unsigned int get_periph_mem_brst_len(struct dma_pl330_desc *desc,
+					    struct dma_pl330_chan *pch,
+					    unsigned int burst_size, int quirks)
+{
+	unsigned int burst_len = pch->burst_len;
+
+	if (quirks & PL330_QUIRK_OPTIMIZE_DEV2MEM_AXSIZE &&
+	    burst_size != pch->burst_sz) {
+		/* Select max possible burst len */
+		burst_len = get_burst_len(desc, burst_size);
+
+		/*
+		 * Adjust AxLen to keep number of bytes same in Load/Store
+		 */
+		if (burst_size > pch->burst_sz)
+			burst_len = pch->burst_len >> (burst_size - pch->burst_sz);
+		else
+			pch->burst_len = burst_len >> (pch->burst_sz - burst_size);
+	}
+	return burst_len;
+}
+
 static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 		struct dma_chan *chan, dma_addr_t dma_addr, size_t len,
 		size_t period_len, enum dma_transfer_direction direction,
@@ -2684,8 +2739,8 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 {
 	struct dma_pl330_desc *desc = NULL, *first = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
+	unsigned int i, burst_size, burst_len;
 	struct pl330_dmac *pl330 = pch->dmac;
-	unsigned int i;
 	dma_addr_t dst;
 	dma_addr_t src;
 
@@ -2729,28 +2784,35 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 			return NULL;
 		}
 
+		burst_size = get_periph_mem_brst_sz(dma_addr, period_len, pch, pl330->quirks);
+		burst_len = get_periph_mem_brst_len(desc, pch, burst_size, pl330->quirks);
+
 		switch (direction) {
 		case DMA_MEM_TO_DEV:
 			desc->rqcfg.src_inc = 1;
 			desc->rqcfg.dst_inc = 0;
 			src = dma_addr;
 			dst = pch->fifo_dma;
+			desc->rqcfg.src_brst_size = burst_size;
+			desc->rqcfg.src_brst_len = burst_len;
+			desc->rqcfg.dst_brst_size = pch->burst_sz;
+			desc->rqcfg.dst_brst_len = pch->burst_len;
 			break;
 		case DMA_DEV_TO_MEM:
 			desc->rqcfg.src_inc = 0;
 			desc->rqcfg.dst_inc = 1;
 			src = pch->fifo_dma;
 			dst = dma_addr;
+			desc->rqcfg.src_brst_size = pch->burst_sz;
+			desc->rqcfg.src_brst_len = pch->burst_len;
+			desc->rqcfg.dst_brst_size = burst_size;
+			desc->rqcfg.dst_brst_len = burst_len;
 			break;
 		default:
 			break;
 		}
 
 		desc->rqtype = direction;
-		desc->rqcfg.src_brst_size = pch->burst_sz;
-		desc->rqcfg.src_brst_len = pch->burst_len;
-		desc->rqcfg.dst_brst_size = pch->burst_sz;
-		desc->rqcfg.dst_brst_len = pch->burst_len;
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2850,7 +2912,11 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 {
 	struct dma_pl330_desc *first, *desc = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
+	unsigned int burst_size, burst_len;
+	struct pl330_dmac *pl330;
 	struct scatterlist *sg;
+	dma_addr_t mem_addr;
+	size_t len;
 	int i;
 
 	if (unlikely(!pch || !sgl || !sg_len))
@@ -2862,13 +2928,12 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		return NULL;
 
 	first = NULL;
+	pl330 = pch->dmac;
 
 	for_each_sg(sgl, sg, sg_len, i) {
 
 		desc = pl330_get_desc(pch);
 		if (!desc) {
-			struct pl330_dmac *pl330 = pch->dmac;
-
 			dev_err(pch->dmac->ddma.dev,
 				"%s:%d Unable to fetch desc\n",
 				__func__, __LINE__);
@@ -2882,29 +2947,37 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		else
 			list_add_tail(&desc->node, &first->node);
 
+		mem_addr = sg_dma_address(sg);
+		len = sg_dma_len(sg);
+
+		burst_size = get_periph_mem_brst_sz(mem_addr, len, pch, pl330->quirks);
+		burst_len = get_periph_mem_brst_len(desc, pch, burst_size, pl330->quirks);
+
 		switch (direction) {
 		case DMA_MEM_TO_DEV:
 			desc->rqcfg.src_inc = 1;
 			desc->rqcfg.dst_inc = 0;
-			fill_px(&desc->px, pch->fifo_dma, sg_dma_address(sg),
-				sg_dma_len(sg));
+			desc->rqcfg.src_brst_size = burst_size;
+			desc->rqcfg.src_brst_len = burst_len;
+			desc->rqcfg.dst_brst_size = pch->burst_sz;
+			desc->rqcfg.dst_brst_len = pch->burst_len;
+			fill_px(&desc->px, pch->fifo_dma, mem_addr, len);
 			break;
 		case DMA_DEV_TO_MEM:
 			desc->rqcfg.src_inc = 0;
 			desc->rqcfg.dst_inc = 1;
-			fill_px(&desc->px, sg_dma_address(sg), pch->fifo_dma,
-				sg_dma_len(sg));
+			desc->rqcfg.src_brst_size = pch->burst_sz;
+			desc->rqcfg.src_brst_len = pch->burst_len;
+			desc->rqcfg.dst_brst_size = burst_size;
+			desc->rqcfg.dst_brst_len = burst_len;
+			fill_px(&desc->px, mem_addr, pch->fifo_dma, len);
 			break;
 		default:
 			break;
 		}
 
-		desc->rqcfg.src_brst_size = pch->burst_sz;
-		desc->rqcfg.src_brst_len = pch->burst_len;
-		desc->rqcfg.dst_brst_size = pch->burst_sz;
-		desc->rqcfg.dst_brst_len = pch->burst_len;
 		desc->rqtype = direction;
-		desc->bytes_requested = sg_dma_len(sg);
+		desc->bytes_requested = len;
 	}
 
 	/* Return the last desc in the chain */
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH 6/7] dmaengine: pl330: Quirk to use DMA singles for peripheral _dregs

[Joy Chakraborty]

Add quirk "arm,pl330-periph-single-dregs" to use DMA singles in a loop
to load/store the remaining bytes left after chucks of bursts are done
which is handled by the _dregs() function.

If the transfer length is not a multiple of (AxLen*AxSize) then the
_dregs function takes care of setting up CCR with residual burst
required to complete the transaction. It does so by changing the
AxLen in CCR and 1 burst of Load and Store.
But some peripherals might not set the burst request signal to the DMA
controller since the number of bytes to transfer is less then the
initial size of burst requested i.e. AxLen*AxSize leading to a forever
wait.

Example of such a case :
    Considering a peripheral having an RX FIFO of n bytes and a sw
    configurable threshold level logic which drives the RX burst req
    signal to PL330 i.e. when data in the RX fifo crosses the threshold
    value the peripheral asserts the RX burst request to PL330 to copy
    data from the fifo in bursts.
    Taking an example say the Rx Fifo is 256 bytes in depth, the max
    AxLen is 16, max AxSize is 4bytes and 304 bytes had to copied from
    peripheral to memory.
    In this case the peripheral SW driver would configure the threshold
    to the maximum possible burst size (AxLen*AxSize) i.e. 64 bytes and
    pass the same to pl330 driver using src/dst_maxburst variable.
    PL330 would copy the first 256 bytes with 4 burst transactions and
    the 48 remaining bytes would be handled by _dregs().
    Currently _dregs() would setup a burst for AxLen=3 and AxSize=16 to
    copy the 48bytes but since 48bytes is below the threshold configured
    at the peripheral the Rx burst request signal would not get set
    leading to a forever wait and timeout.
    This quirk will copy the remaining 48bytes using single transactions
    of 4bytes each which would not depend on the burst req signal from
    the peripheral.

Instructions generated for above example with quirk set:
    DMAMOV CCR 0xbd0239
    DMAMOV SAR 0xffffe000
    DMAMOV DAR 0xffffc860
    DMALP_1 3
    DMAFLUSHP 0
    DMAWFPB 0
    DMALDB
    DMASTPB 0
    DMALPENDA_1 bjmpto_7
    DMAMOV CCR 0xad0229
    DMALDA
    DMALP_0 11
    DMAFLUSHP 0
    DMAWFPS 0
    DMASTPS 0
    DMALPENDA_0 bjmpto_6
    DMASEV 3
    DMAEND

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/dma/pl330.c | 74 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b4933fab8a62..0b4e5390bace 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -36,6 +36,7 @@
 #define PL330_QUIRK_BROKEN_NO_FLUSHP	BIT(0)
 #define PL330_QUIRK_PERIPH_BURST	BIT(1)
 #define PL330_QUIRK_OPTIMIZE_DEV2MEM_AXSIZE	BIT(2)
+#define PL330_QUIRK_PERIPH_SINGLE_DREGS	BIT(3)
 
 enum pl330_cachectrl {
 	CCTRL0,		/* Noncacheable and nonbufferable */
@@ -524,6 +525,10 @@ static struct pl330_of_quirks {
 	{
 		.quirk = "arm,pl330-optimize-dev2mem-axsize",
 		.id = PL330_QUIRK_OPTIMIZE_DEV2MEM_AXSIZE,
+	},
+	{
+		.quirk = "arm,pl330-periph-single-dregs",
+		.id = PL330_QUIRK_PERIPH_SINGLE_DREGS,
 	}
 };
 
@@ -1213,6 +1218,67 @@ static inline int _ldst_peripheral(struct pl330_dmac *pl330,
 	return off;
 }
 
+/*
+ * Sets up transfers to peripheral using DMA Singles instead of Bursts.
+ * Data is moved between fifo and memory in bursts following which it is
+ * loaded/stored to peripheral using Loops of DMA singles based on
+ * transfer direction.
+ */
+static inline int _ldst_periph_single_dregs(struct pl330_dmac *pl330,
+					    unsigned int dry_run, u8 buf[],
+					    const struct _xfer_spec *pxs,
+					    int src_length, int dst_length)
+{
+	int off = 0;
+	unsigned int ljmp, lpcnt;
+	struct _arg_LPEND lpend;
+	enum dma_transfer_direction direction = pxs->desc->rqtype;
+
+	if (direction == DMA_MEM_TO_DEV) {
+		off += _emit_load(dry_run, &buf[off], ALWAYS, direction,
+				  pxs->desc->peri);
+		lpcnt = dst_length;
+	} else {
+		lpcnt = src_length;
+	}
+
+	/*
+	 * Use Loop Cnt 0 to load/store from/to peripheral in single transactions
+	 * since Burst Req might not be set as pending transfer length maybe less
+	 * size of bytes to burst (AxSize * AxLen).
+	 */
+	off += _emit_LP(dry_run, &buf[off], 0, lpcnt);
+	ljmp = off;
+
+	/*
+	 * 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);
+
+	off += _emit_WFP(dry_run, &buf[off], SINGLE, pxs->desc->peri);
+
+	if (direction == DMA_MEM_TO_DEV)
+		off += _emit_store(dry_run, &buf[off], SINGLE, direction,
+				   pxs->desc->peri);
+	else
+		off += _emit_load(dry_run, &buf[off], SINGLE, direction,
+				  pxs->desc->peri);
+
+	lpend.cond = ALWAYS;
+	lpend.forever = false;
+	lpend.loop = 0;
+	lpend.bjump = off - ljmp;
+	off += _emit_LPEND(dry_run, &buf[off], &lpend);
+
+	if (direction == DMA_DEV_TO_MEM)
+		off += _emit_store(dry_run, &buf[off], ALWAYS, direction,
+				   pxs->desc->peri);
+
+	return off;
+}
+
 static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		const struct _xfer_spec *pxs, int cyc)
 {
@@ -1278,8 +1344,12 @@ static int _dregs(struct pl330_dmac *pl330, unsigned int dry_run, u8 buf[],
 	case DMA_MEM_TO_DEV:
 	case DMA_DEV_TO_MEM:
 		off += _emit_MOV(dry_run, &buf[off], CCR, dregs_ccr);
-		off += _ldst_peripheral(pl330, dry_run, &buf[off], pxs, 1,
-					BURST);
+		if (pl330->quirks & PL330_QUIRK_PERIPH_SINGLE_DREGS)
+			off += _ldst_periph_single_dregs(pl330, dry_run, &buf[off],
+							 pxs, src_length, dst_length);
+		else
+			off += _ldst_peripheral(pl330, dry_run, &buf[off], pxs, 1,
+						BURST);
 		break;
 
 	case DMA_MEM_TO_MEM:
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH 7/7] dt-bindings: dmaengine: pl330: Add new quirks

[Joy Chakraborty]

Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
and "arm,pl330-periph-single-dregs"

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
index 4a3dd6f5309b..0499a7fba88d 100644
--- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
+++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
@@ -53,6 +53,14 @@ properties:
     type: boolean
     description: quirk for performing burst transfer only
 
+  arm,pl330-optimize-dev2mem-axsize:
+    type: boolean
+    description: quirk for optimizing AxSize used between dev<->mem
+
+  arm,pl330-periph-single-dregs:
+    type: boolean
+    description: quirk for using dma-singles for peripherals in _dregs()
+
   dma-coherent: true
 
   iommus:
-- 
2.40.1.495.gc816e09b53d-goog

Re: [PATCH 7/7] dt-bindings: dmaengine: pl330: Add new quirks

[Krzysztof Kozlowski]

On 04/05/2023 16:57, Joy Chakraborty wrote:
> Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
> and "arm,pl330-periph-single-dregs"

This we can see from the diff. You need to answer why?

> 
> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> ---
>  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> index 4a3dd6f5309b..0499a7fba88d 100644
> --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> @@ -53,6 +53,14 @@ properties:
>      type: boolean
>      description: quirk for performing burst transfer only
>  
> +  arm,pl330-optimize-dev2mem-axsize:
> +    type: boolean
> +    description: quirk for optimizing AxSize used between dev<->mem

This tells me nothing... Neither what it is about nor why this is
property of a board or PL330 hardware implementation. Please describe
hardware, not drivers.

> +
> +  arm,pl330-periph-single-dregs:
> +    type: boolean
> +    description: quirk for using dma-singles for peripherals in _dregs()

Same concerns.


Best regards,
Krzysztof

Re: [PATCH 7/7] dt-bindings: dmaengine: pl330: Add new quirks

[Joy Chakraborty]

On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 04/05/2023 16:57, Joy Chakraborty wrote:
> > Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
> > and "arm,pl330-periph-single-dregs"
>
> This we can see from the diff. You need to answer why?
>

Sure will change it to:
"
Addition of following quirks :
- "arm,pl330-periph-use-diff-axsize"
   AxSize of transactions to peripherals are limited by the peripheral
address width which inturn limits the AxSize used for transactions
towards memory.
   This quirk will make transactions to memory use the maximum
possible bus width(AxSize), store data in MFIFO and use narrow
multi-beat transactions to move data to peripherals.
   This only applies to transfers between memory and peripherals where
bus widths available are different for memory and the peripheral.
- "arm,pl330-periph-complete-with-singles" :
   When transfer sizes are not a multiple of a block of burst
transfers (AxLen * AxSize configured at the peripheral), certain
peripherals might choose not to set the burst request at the
peripheral request interface of the DMA.
   This quirk moves the remaining bytes to the peripheral using single
transactions.
"

> >
> > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > ---
> >  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> > index 4a3dd6f5309b..0499a7fba88d 100644
> > --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> > +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> > @@ -53,6 +53,14 @@ properties:
> >      type: boolean
> >      description: quirk for performing burst transfer only
> >
> > +  arm,pl330-optimize-dev2mem-axsize:
> > +    type: boolean
> > +    description: quirk for optimizing AxSize used between dev<->mem
>
> This tells me nothing... Neither what it is about nor why this is
> property of a board or PL330 hardware implementation. Please describe
> hardware, not drivers.
>

Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
"
Quirk to use different AxSize for bursts while accessing source and
destination when moving data between memory and peripheral.
Maximum possible bus width is used as AxSize for transactions towards
memory and transactions towards peripherals use AxSize as per
peripheral address width.
"

> > +
> > +  arm,pl330-periph-single-dregs:
> > +    type: boolean
> > +    description: quirk for using dma-singles for peripherals in _dregs()
>
> Same concerns.
>

Will change the name to  "arm,pl330-periph-complete-with-singles" and
add description:
"
Quirk to use dma singles n times instead of an n beat burst to
complete a transfer when the transfer size is not a multiple of the
burst size and burst length configured at the peripheral.
n being bytes left after the major chunk is transferred with
peripheral configured burst transactions.
"

>
> Best regards,
> Krzysztof
>

Will update the patch series if this is satisfactory.

Thanks
Joy

Re: [PATCH 7/7] dt-bindings: dmaengine: pl330: Add new quirks

[Krzysztof Kozlowski]

On 05/05/2023 11:44, Joy Chakraborty wrote:
> On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 04/05/2023 16:57, Joy Chakraborty wrote:
>>> Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
>>> and "arm,pl330-periph-single-dregs"
>>
>> This we can see from the diff. You need to answer why?
>>
> 
> Sure will change it to:
> "
> Addition of following quirks :
> - "arm,pl330-periph-use-diff-axsize"
>    AxSize of transactions to peripherals are limited by the peripheral
> address width which inturn limits the AxSize used for transactions
> towards memory.
>    This quirk will make transactions to memory use the maximum
> possible bus width(AxSize), store data in MFIFO and use narrow
> multi-beat transactions to move data to peripherals.
>    This only applies to transfers between memory and peripherals where
> bus widths available are different for memory and the peripheral.
> - "arm,pl330-periph-complete-with-singles" :
>    When transfer sizes are not a multiple of a block of burst
> transfers (AxLen * AxSize configured at the peripheral), certain
> peripherals might choose not to set the burst request at the
> peripheral request interface of the DMA.
>    This quirk moves the remaining bytes to the peripheral using single
> transactions.
> "

This does not answer why. You just copied again the patch contents.

> 
>>>
>>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
>>> ---
>>>  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>> index 4a3dd6f5309b..0499a7fba88d 100644
>>> --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>> +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>> @@ -53,6 +53,14 @@ properties:
>>>      type: boolean
>>>      description: quirk for performing burst transfer only
>>>
>>> +  arm,pl330-optimize-dev2mem-axsize:
>>> +    type: boolean
>>> +    description: quirk for optimizing AxSize used between dev<->mem
>>
>> This tells me nothing... Neither what it is about nor why this is
>> property of a board or PL330 hardware implementation. Please describe
>> hardware, not drivers.
>>
> 
> Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
> "
> Quirk to use different AxSize for bursts while accessing source and
> destination when moving data between memory and peripheral.
> Maximum possible bus width is used as AxSize for transactions towards
> memory and transactions towards peripherals use AxSize as per
> peripheral address width.
> "

Still no answer. Why this is property of a board or PL330 hardware
implementation?

I also asked to describe hardware but I still see "quirk to ...". We use
"quirk" as concept in Linux driver. Describe the hardware, not Linux driver.


> 
>>> +
>>> +  arm,pl330-periph-single-dregs:
>>> +    type: boolean
>>> +    description: quirk for using dma-singles for peripherals in _dregs()
>>
>> Same concerns.
>>
> 
> Will change the name to  "arm,pl330-periph-complete-with-singles" and
> add description:
> "
> Quirk to use dma singles n times instead of an n beat burst to
> complete a transfer when the transfer size is not a multiple of the

No, how you wrote it sounds like driver. Don't add driver quirks to DT.

Best regards,
Krzysztof

Re: [PATCH 7/7] dt-bindings: dmaengine: pl330: Add new quirks

[Joy Chakraborty]

On Fri, May 5, 2023 at 5:53 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/05/2023 11:44, Joy Chakraborty wrote:
> > On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 04/05/2023 16:57, Joy Chakraborty wrote:
> >>> Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
> >>> and "arm,pl330-periph-single-dregs"
> >>
> >> This we can see from the diff. You need to answer why?
> >>
> >
> > Sure will change it to:
> > "
> > Addition of following quirks :
> > - "arm,pl330-periph-use-diff-axsize"
> >    AxSize of transactions to peripherals are limited by the peripheral
> > address width which inturn limits the AxSize used for transactions
> > towards memory.
> >    This quirk will make transactions to memory use the maximum
> > possible bus width(AxSize), store data in MFIFO and use narrow
> > multi-beat transactions to move data to peripherals.
> >    This only applies to transfers between memory and peripherals where
> > bus widths available are different for memory and the peripheral.
> > - "arm,pl330-periph-complete-with-singles" :
> >    When transfer sizes are not a multiple of a block of burst
> > transfers (AxLen * AxSize configured at the peripheral), certain
> > peripherals might choose not to set the burst request at the
> > peripheral request interface of the DMA.
> >    This quirk moves the remaining bytes to the peripheral using single
> > transactions.
> > "
>
> This does not answer why. You just copied again the patch contents.
>
Hi Krzysztof,
Both the changes could be useful for SOC's which have PL330 integrated
with a peripheral but I am not sure if all SOC's need/want this change
hence wanted to keep it as a DT knob to avoid any regressions.
But like you say it might not be the right thing to do.

> >
> >>>
> >>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
> >>>  1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>> index 4a3dd6f5309b..0499a7fba88d 100644
> >>> --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>> +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>> @@ -53,6 +53,14 @@ properties:
> >>>      type: boolean
> >>>      description: quirk for performing burst transfer only
> >>>
> >>> +  arm,pl330-optimize-dev2mem-axsize:
> >>> +    type: boolean
> >>> +    description: quirk for optimizing AxSize used between dev<->mem
> >>
> >> This tells me nothing... Neither what it is about nor why this is
> >> property of a board or PL330 hardware implementation. Please describe
> >> hardware, not drivers.
> >>
> >
> > Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
> > "
> > Quirk to use different AxSize for bursts while accessing source and
> > destination when moving data between memory and peripheral.
> > Maximum possible bus width is used as AxSize for transactions towards
> > memory and transactions towards peripherals use AxSize as per
> > peripheral address width.
> > "
>
> Still no answer. Why this is property of a board or PL330 hardware
> implementation?
> I also asked to describe hardware but I still see "quirk to ...". We use
> "quirk" as concept in Linux driver. Describe the hardware, not Linux driver.
>

This comes to use when the bus width requirement between peripheral
and memory is different, but buswidth is something we read from HW
registers so this can be enabled by default.

>
> >
> >>> +
> >>> +  arm,pl330-periph-single-dregs:
> >>> +    type: boolean
> >>> +    description: quirk for using dma-singles for peripherals in _dregs()
> >>
> >> Same concerns.
> >>

An example of such a case is given in the ARM TRM for PL330, so maybe
we can have this by default as well.
Link : https://developer.arm.com/documentation/ddi0424/d/functional-overview/peripheral-request-interface/dmac-length-management#:~:text=DMAC%20length%20management-,Example%202.3,-shows%20a%20DMAC

> >
> > Will change the name to  "arm,pl330-periph-complete-with-singles" and
> > add description:
> > "
> > Quirk to use dma singles n times instead of an n beat burst to
> > complete a transfer when the transfer size is not a multiple of the
>
> No, how you wrote it sounds like driver. Don't add driver quirks to DT.
>
> Best regards,
> Krzysztof
>

Hi Vinod Kaul,

Do you think it is feasible to enable these changes by default instead
of a DT property ?

Thanks
Joy

Re: [PATCH 7/7] dt-bindings: dmaengine: pl330: Add new quirks

[Krzysztof Kozlowski]

On 08/05/2023 13:58, Joy Chakraborty wrote:
> On Fri, May 5, 2023 at 5:53 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 05/05/2023 11:44, Joy Chakraborty wrote:
>>> On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 04/05/2023 16:57, Joy Chakraborty wrote:
>>>>> Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
>>>>> and "arm,pl330-periph-single-dregs"
>>>>
>>>> This we can see from the diff. You need to answer why?
>>>>
>>>
>>> Sure will change it to:
>>> "
>>> Addition of following quirks :
>>> - "arm,pl330-periph-use-diff-axsize"
>>>    AxSize of transactions to peripherals are limited by the peripheral
>>> address width which inturn limits the AxSize used for transactions
>>> towards memory.
>>>    This quirk will make transactions to memory use the maximum
>>> possible bus width(AxSize), store data in MFIFO and use narrow
>>> multi-beat transactions to move data to peripherals.
>>>    This only applies to transfers between memory and peripherals where
>>> bus widths available are different for memory and the peripheral.
>>> - "arm,pl330-periph-complete-with-singles" :
>>>    When transfer sizes are not a multiple of a block of burst
>>> transfers (AxLen * AxSize configured at the peripheral), certain
>>> peripherals might choose not to set the burst request at the
>>> peripheral request interface of the DMA.
>>>    This quirk moves the remaining bytes to the peripheral using single
>>> transactions.
>>> "
>>
>> This does not answer why. You just copied again the patch contents.
>>
> Hi Krzysztof,
> Both the changes could be useful for SOC's which have PL330 integrated
> with a peripheral 

What do you mean here by "PL330 integrated with a peripheral"?

> but I am not sure if all SOC's need/want this change
> hence wanted to keep it as a DT knob to avoid any regressions.
> But like you say it might not be the right thing to do.

Devicetree is for describing hardware, not the contents of registers of
a device. Your changes might fit or might not, I don't know this good
enough, so I wait for your justification. Without justification this
looks like controlling driver from DT...

> 
>>>
>>>>>
>>>>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
>>>>>  1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>>>> index 4a3dd6f5309b..0499a7fba88d 100644
>>>>> --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>>>> +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>>>> @@ -53,6 +53,14 @@ properties:
>>>>>      type: boolean
>>>>>      description: quirk for performing burst transfer only
>>>>>
>>>>> +  arm,pl330-optimize-dev2mem-axsize:
>>>>> +    type: boolean
>>>>> +    description: quirk for optimizing AxSize used between dev<->mem
>>>>
>>>> This tells me nothing... Neither what it is about nor why this is
>>>> property of a board or PL330 hardware implementation. Please describe
>>>> hardware, not drivers.
>>>>
>>>
>>> Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
>>> "
>>> Quirk to use different AxSize for bursts while accessing source and
>>> destination when moving data between memory and peripheral.
>>> Maximum possible bus width is used as AxSize for transactions towards
>>> memory and transactions towards peripherals use AxSize as per
>>> peripheral address width.
>>> "
>>
>> Still no answer. Why this is property of a board or PL330 hardware
>> implementation?
>> I also asked to describe hardware but I still see "quirk to ...". We use
>> "quirk" as concept in Linux driver. Describe the hardware, not Linux driver.
>>
> 
> This comes to use when the bus width requirement between peripheral
> and memory is different, but buswidth is something we read from HW
> registers so this can be enabled by default.

Don't add discoverable stuff to DT.

> 
>>
>>>
>>>>> +
>>>>> +  arm,pl330-periph-single-dregs:
>>>>> +    type: boolean
>>>>> +    description: quirk for using dma-singles for peripherals in _dregs()
>>>>
>>>> Same concerns.
>>>>
> 
> An example of such a case is given in the ARM TRM for PL330, so maybe
> we can have this by default as well.
> Link : https://developer.arm.com/documentation/ddi0424/d/functional-overview/peripheral-request-interface/dmac-length-management#:~:text=DMAC%20length%20management-,Example%202.3,-shows%20a%20DMAC

I could not find here a case describing hardware. You pointed out some
code. What does the code have anything to do with DT?


Best regards,
Krzysztof

Re: [PATCH 7/7] dt-bindings: dmaengine: pl330: Add new quirks

[Joy Chakraborty]

On Mon, May 8, 2023 at 10:13 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/05/2023 13:58, Joy Chakraborty wrote:
> > On Fri, May 5, 2023 at 5:53 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 05/05/2023 11:44, Joy Chakraborty wrote:
> >>> On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> On 04/05/2023 16:57, Joy Chakraborty wrote:
> >>>>> Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
> >>>>> and "arm,pl330-periph-single-dregs"
> >>>>
> >>>> This we can see from the diff. You need to answer why?
> >>>>
> >>>
> >>> Sure will change it to:
> >>> "
> >>> Addition of following quirks :
> >>> - "arm,pl330-periph-use-diff-axsize"
> >>>    AxSize of transactions to peripherals are limited by the peripheral
> >>> address width which inturn limits the AxSize used for transactions
> >>> towards memory.
> >>>    This quirk will make transactions to memory use the maximum
> >>> possible bus width(AxSize), store data in MFIFO and use narrow
> >>> multi-beat transactions to move data to peripherals.
> >>>    This only applies to transfers between memory and peripherals where
> >>> bus widths available are different for memory and the peripheral.
> >>> - "arm,pl330-periph-complete-with-singles" :
> >>>    When transfer sizes are not a multiple of a block of burst
> >>> transfers (AxLen * AxSize configured at the peripheral), certain
> >>> peripherals might choose not to set the burst request at the
> >>> peripheral request interface of the DMA.
> >>>    This quirk moves the remaining bytes to the peripheral using single
> >>> transactions.
> >>> "
> >>
> >> This does not answer why. You just copied again the patch contents.
> >>
> > Hi Krzysztof,
> > Both the changes could be useful for SOC's which have PL330 integrated
> > with a peripheral
>
> What do you mean here by "PL330 integrated with a peripheral"?

Hi Krzysztof,

By integration with peripheral I mean when the PL330 DMA is used to
copy data to/from memory to a peripheral hardware (e.g. FIFO of a SPI
master) where flow control of data is managed by the peripheral
request interface exposed by PL330 :
https://developer.arm.com/documentation/ddi0424/a/functional-overview/peripheral-request-interface

>
> > but I am not sure if all SOC's need/want this change
> > hence wanted to keep it as a DT knob to avoid any regressions.
> > But like you say it might not be the right thing to do.
>
> Devicetree is for describing hardware, not the contents of registers of
> a device. Your changes might fit or might not, I don't know this good
> enough, so I wait for your justification. Without justification this
> looks like controlling driver from DT...
>

Yes this does control the driver behaviour on how the PL330 DMA
hardware is programmed but it also is a function of
   - The bus width available in the soc towards memory and peripheral
to be different.
   - The requirement of peripherals interfaced with PL330 on how odd
transfer sizes are to be copied from memory to peripheral.

But, both changes IMO can be enabled by default as well in the driver
without devicetree knobs but it carries the risk of regression on
SOC's which do not have such a requirement.

Hence I was looking for some insight from Vinod Koul to see if it is
okay to go ahead with the changes without device tree knobs.

> >
> >>>
> >>>>>
> >>>>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
> >>>>>  1 file changed, 8 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>>>> index 4a3dd6f5309b..0499a7fba88d 100644
> >>>>> --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>>>> @@ -53,6 +53,14 @@ properties:
> >>>>>      type: boolean
> >>>>>      description: quirk for performing burst transfer only
> >>>>>
> >>>>> +  arm,pl330-optimize-dev2mem-axsize:
> >>>>> +    type: boolean
> >>>>> +    description: quirk for optimizing AxSize used between dev<->mem
> >>>>
> >>>> This tells me nothing... Neither what it is about nor why this is
> >>>> property of a board or PL330 hardware implementation. Please describe
> >>>> hardware, not drivers.
> >>>>
> >>>
> >>> Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
> >>> "
> >>> Quirk to use different AxSize for bursts while accessing source and
> >>> destination when moving data between memory and peripheral.
> >>> Maximum possible bus width is used as AxSize for transactions towards
> >>> memory and transactions towards peripherals use AxSize as per
> >>> peripheral address width.
> >>> "
> >>
> >> Still no answer. Why this is property of a board or PL330 hardware
> >> implementation?
> >> I also asked to describe hardware but I still see "quirk to ...". We use
> >> "quirk" as concept in Linux driver. Describe the hardware, not Linux driver.
> >>
> >
> > This comes to use when the bus width requirement between peripheral
> > and memory is different, but buswidth is something we read from HW
> > registers so this can be enabled by default.
>
> Don't add discoverable stuff to DT.

Sure, will not add this to DT.

>
> >
> >>
> >>>
> >>>>> +
> >>>>> +  arm,pl330-periph-single-dregs:
> >>>>> +    type: boolean
> >>>>> +    description: quirk for using dma-singles for peripherals in _dregs()
> >>>>
> >>>> Same concerns.
> >>>>
> >
> > An example of such a case is given in the ARM TRM for PL330, so maybe
> > we can have this by default as well.
> > Link : https://developer.arm.com/documentation/ddi0424/d/functional-overview/peripheral-request-interface/dmac-length-management#:~:text=DMAC%20length%20management-,Example%202.3,-shows%20a%20DMAC
>
> I could not find here a case describing hardware. You pointed out some
> code. What does the code have anything to do with DT?
>

The instructions mentioned here are consumed by the PL330 Hardware to
generate AXI transactions on the system bus. The example mentioned in
the link is similar to how the driver would behave when this is
enabled.

I shall remove this as well and create a new patch without any DT
depency for the changes.

Thanks
Joy
>
> Best regards,
> Krzysztof
>