[PATCH 0/3] Support burst request by peripherals

[PATCH 0/3] Support burst request by peripherals

[Shawn Lin]

Burst request is supported by pl330 but the original code
only support single mode as some Socs didn't implement it.
So this feature has been missing for a long time. But it's
very important for efficiency.

This patchset is gonna support it without the probability of
breaking old(other) platforms, so the new optional property
is introduced.

Also when supporting burst request type, we could be able
to deal with unaligned case internally.

After applying this patchset, we could see significant improvement
when doing mem-2-dev/dev-2-mem/mem-2-mem which I mentioned in the
commit msg.

I would appreciate it if folks could help review and test it.:)

Thanks for any feedback.



Shawn Lin (3):
  dt/bindings: arm-pl330: add description of arm,pl330-periph-burst
  dmaengine: pl330: enable burst mode by parsing dt
  dmaengine: pl330: support transfer unaligned with (burst len * burst
    size)

 .../devicetree/bindings/dma/arm-pl330.txt          |  1 +
 drivers/dma/pl330.c                                | 58 +++++++++++++++-------
 2 files changed, 42 insertions(+), 17 deletions(-)

-- 
2.3.7

[PATCH 0/3] Support burst request by peripherals

[Shawn Lin]

Burst request is supported by pl330 but the original code
only support single mode as some Socs didn't implement it.
So this feature has been missing for a long time. But it's
very important for efficiency.

This patchset is gonna support it without the probability of
breaking old(other) platforms, so the new optional property
is introduced.

Also when supporting burst request type, we could be able
to deal with unaligned case internally.

After applying this patchset, we could see significant improvement
when doing mem-2-dev/dev-2-mem/mem-2-mem which I mentioned in the
commit msg.

I would appreciate it if folks could help review and test it.:)

Thanks for any feedback.



Shawn Lin (3):
  dt/bindings: arm-pl330: add description of arm,pl330-periph-burst
  dmaengine: pl330: enable burst mode by parsing dt
  dmaengine: pl330: support transfer unaligned with (burst len * burst
    size)

 .../devicetree/bindings/dma/arm-pl330.txt          |  1 +
 drivers/dma/pl330.c                                | 58 +++++++++++++++-------
 2 files changed, 42 insertions(+), 17 deletions(-)

-- 
2.3.7


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] Support burst request by peripherals

[Shawn Lin]

Burst request is supported by pl330 but the original code
only support single mode as some Socs didn't implement it.
So this feature has been missing for a long time. But it's
very important for efficiency.

This patchset is gonna support it without the probability of
breaking old(other) platforms, so the new optional property
is introduced.

Also when supporting burst request type, we could be able
to deal with unaligned case internally.

After applying this patchset, we could see significant improvement
when doing mem-2-dev/dev-2-mem/mem-2-mem which I mentioned in the
commit msg.

I would appreciate it if folks could help review and test it.:)

Thanks for any feedback.



Shawn Lin (3):
  dt/bindings: arm-pl330: add description of arm,pl330-periph-burst
  dmaengine: pl330: enable burst mode by parsing dt
  dmaengine: pl330: support transfer unaligned with (burst len * burst
    size)

 .../devicetree/bindings/dma/arm-pl330.txt          |  1 +
 drivers/dma/pl330.c                                | 58 +++++++++++++++-------
 2 files changed, 42 insertions(+), 17 deletions(-)

-- 
2.3.7

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
support busrt mode.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

 Documentation/devicetree/bindings/dma/arm-pl330.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt b/Documentation/devicetree/bindings/dma/arm-pl330.txt
index db7e226..262e97a 100644
--- a/Documentation/devicetree/bindings/dma/arm-pl330.txt
+++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
@@ -16,6 +16,7 @@ Optional properties:
   - dma-channels: contains the total number of DMA channels supported by the DMAC
   - dma-requests: contains the total number of DMA requests supported by the DMAC
   - arm,pl330-broken-no-flushp: quirk for avoiding to execute DMAFLUSHP
+  - arm,pl330-periph-burst: set peripheral dma request type as burst mode
 
 Example:
 
-- 
2.3.7

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
support busrt mode.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

---

 Documentation/devicetree/bindings/dma/arm-pl330.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt b/Documentation/devicetree/bindings/dma/arm-pl330.txt
index db7e226..262e97a 100644
--- a/Documentation/devicetree/bindings/dma/arm-pl330.txt
+++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
@@ -16,6 +16,7 @@ Optional properties:
   - dma-channels: contains the total number of DMA channels supported by the DMAC
   - dma-requests: contains the total number of DMA requests supported by the DMAC
   - arm,pl330-broken-no-flushp: quirk for avoiding to execute DMAFLUSHP
+  - arm,pl330-periph-burst: set peripheral dma request type as burst mode
 
 Example:
 
-- 
2.3.7


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm, pl330-periph-burst

[Shawn Lin]

This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
support busrt mode.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

 Documentation/devicetree/bindings/dma/arm-pl330.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt b/Documentation/devicetree/bindings/dma/arm-pl330.txt
index db7e226..262e97a 100644
--- a/Documentation/devicetree/bindings/dma/arm-pl330.txt
+++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
@@ -16,6 +16,7 @@ Optional properties:
   - dma-channels: contains the total number of DMA channels supported by the DMAC
   - dma-requests: contains the total number of DMA requests supported by the DMAC
   - arm,pl330-broken-no-flushp: quirk for avoiding to execute DMAFLUSHP
+  - arm,pl330-periph-burst: set peripheral dma request type as burst mode
 
 Example:
 
-- 
2.3.7

[PATCH 2/3] dmaengine: pl330: enable burst mode by parsing dt

[Shawn Lin]

Currently pl330 use single mode defaultly. But burst
mode can improve efficiency of memory accessing. We
couldn't enable it by defalut in case of breaking any
Socs which don't support it.

With burst mode supported, we could see the improvement
significantly when tesing SPI transfer etc.

default single mode
[   88.292550] spi write 65536*1 cost 32402us speed:2022KB/S

After applied with burst mode(len 16)
[   17.625296] spi write 65536*1 cost 17830us speed:3675KB/S

Cc: Huibin Hong <huibin.hong@rock-chips.com>
Cc: Xing Zheng <zhengxing@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/dma/pl330.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4fc3ffb..a09bf22 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -491,6 +491,8 @@ struct pl330_dmac {
 	/* Peripheral channels connected to this DMAC */
 	unsigned int num_peripherals;
 	struct dma_pl330_chan *peripherals; /* keep at end */
+	/* set peripherals request type according to soc config */
+	enum pl330_cond peripherals_req_type;
 	int quirks;
 };
 
@@ -1156,12 +1158,7 @@ static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
 				 int cyc)
 {
 	int off = 0;
-	enum pl330_cond cond;
-
-	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
-		cond = BURST;
-	else
-		cond = SINGLE;
+	enum pl330_cond cond = pl330->peripherals_req_type;
 
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
@@ -1181,12 +1178,7 @@ static inline int _ldst_memtodev(struct pl330_dmac *pl330,
 				 const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
-	enum pl330_cond cond;
-
-	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
-		cond = BURST;
-	else
-		cond = SINGLE;
+	enum pl330_cond cond = pl330->peripherals_req_type;
 
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
@@ -2597,7 +2589,12 @@ 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;
+
+		if (pl330->peripherals_req_type == BURST)
+			desc->rqcfg.brst_len = pch->burst_len;
+		else
+			desc->rqcfg.brst_len = 1;
+
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2742,7 +2739,12 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		}
 
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+
+		if (pch->dmac->peripherals_req_type == BURST)
+			desc->rqcfg.brst_len = pch->burst_len;
+		else
+			desc->rqcfg.brst_len = 1;
+
 		desc->rqtype = direction;
 		desc->bytes_requested = sg_dma_len(sg);
 	}
@@ -2836,6 +2838,11 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	pl330->mcbufsz = pdat ? pdat->mcbuf_sz : 0;
 
+	if (of_find_property(np, "arm,pl330-periph-burst", NULL))
+		pl330->peripherals_req_type = BURST;
+	else
+		pl330->peripherals_req_type = SINGLE;
+
 	/* get quirk */
 	for (i = 0; i < ARRAY_SIZE(of_quirks); i++)
 		if (of_property_read_bool(np, of_quirks[i].quirk))
-- 
2.3.7

[PATCH 2/3] dmaengine: pl330: enable burst mode by parsing dt

[Shawn Lin]

Currently pl330 use single mode defaultly. But burst
mode can improve efficiency of memory accessing. We
couldn't enable it by defalut in case of breaking any
Socs which don't support it.

With burst mode supported, we could see the improvement
significantly when tesing SPI transfer etc.

default single mode
[   88.292550] spi write 65536*1 cost 32402us speed:2022KB/S

After applied with burst mode(len 16)
[   17.625296] spi write 65536*1 cost 17830us speed:3675KB/S

Cc: Huibin Hong <huibin.hong-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Cc: Xing Zheng <zhengxing-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 drivers/dma/pl330.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4fc3ffb..a09bf22 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -491,6 +491,8 @@ struct pl330_dmac {
 	/* Peripheral channels connected to this DMAC */
 	unsigned int num_peripherals;
 	struct dma_pl330_chan *peripherals; /* keep at end */
+	/* set peripherals request type according to soc config */
+	enum pl330_cond peripherals_req_type;
 	int quirks;
 };
 
@@ -1156,12 +1158,7 @@ static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
 				 int cyc)
 {
 	int off = 0;
-	enum pl330_cond cond;
-
-	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
-		cond = BURST;
-	else
-		cond = SINGLE;
+	enum pl330_cond cond = pl330->peripherals_req_type;
 
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
@@ -1181,12 +1178,7 @@ static inline int _ldst_memtodev(struct pl330_dmac *pl330,
 				 const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
-	enum pl330_cond cond;
-
-	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
-		cond = BURST;
-	else
-		cond = SINGLE;
+	enum pl330_cond cond = pl330->peripherals_req_type;
 
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
@@ -2597,7 +2589,12 @@ 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;
+
+		if (pl330->peripherals_req_type == BURST)
+			desc->rqcfg.brst_len = pch->burst_len;
+		else
+			desc->rqcfg.brst_len = 1;
+
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2742,7 +2739,12 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		}
 
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+
+		if (pch->dmac->peripherals_req_type == BURST)
+			desc->rqcfg.brst_len = pch->burst_len;
+		else
+			desc->rqcfg.brst_len = 1;
+
 		desc->rqtype = direction;
 		desc->bytes_requested = sg_dma_len(sg);
 	}
@@ -2836,6 +2838,11 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	pl330->mcbufsz = pdat ? pdat->mcbuf_sz : 0;
 
+	if (of_find_property(np, "arm,pl330-periph-burst", NULL))
+		pl330->peripherals_req_type = BURST;
+	else
+		pl330->peripherals_req_type = SINGLE;
+
 	/* get quirk */
 	for (i = 0; i < ARRAY_SIZE(of_quirks); i++)
 		if (of_property_read_bool(np, of_quirks[i].quirk))
-- 
2.3.7


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] dmaengine: pl330: enable burst mode by parsing dt

[Shawn Lin]

Currently pl330 use single mode defaultly. But burst
mode can improve efficiency of memory accessing. We
couldn't enable it by defalut in case of breaking any
Socs which don't support it.

With burst mode supported, we could see the improvement
significantly when tesing SPI transfer etc.

default single mode
[   88.292550] spi write 65536*1 cost 32402us speed:2022KB/S

After applied with burst mode(len 16)
[   17.625296] spi write 65536*1 cost 17830us speed:3675KB/S

Cc: Huibin Hong <huibin.hong@rock-chips.com>
Cc: Xing Zheng <zhengxing@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/dma/pl330.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4fc3ffb..a09bf22 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -491,6 +491,8 @@ struct pl330_dmac {
 	/* Peripheral channels connected to this DMAC */
 	unsigned int num_peripherals;
 	struct dma_pl330_chan *peripherals; /* keep at end */
+	/* set peripherals request type according to soc config */
+	enum pl330_cond peripherals_req_type;
 	int quirks;
 };
 
@@ -1156,12 +1158,7 @@ static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
 				 int cyc)
 {
 	int off = 0;
-	enum pl330_cond cond;
-
-	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
-		cond = BURST;
-	else
-		cond = SINGLE;
+	enum pl330_cond cond = pl330->peripherals_req_type;
 
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
@@ -1181,12 +1178,7 @@ static inline int _ldst_memtodev(struct pl330_dmac *pl330,
 				 const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
-	enum pl330_cond cond;
-
-	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
-		cond = BURST;
-	else
-		cond = SINGLE;
+	enum pl330_cond cond = pl330->peripherals_req_type;
 
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
@@ -2597,7 +2589,12 @@ 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;
+
+		if (pl330->peripherals_req_type == BURST)
+			desc->rqcfg.brst_len = pch->burst_len;
+		else
+			desc->rqcfg.brst_len = 1;
+
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2742,7 +2739,12 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		}
 
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+
+		if (pch->dmac->peripherals_req_type == BURST)
+			desc->rqcfg.brst_len = pch->burst_len;
+		else
+			desc->rqcfg.brst_len = 1;
+
 		desc->rqtype = direction;
 		desc->bytes_requested = sg_dma_len(sg);
 	}
@@ -2836,6 +2838,11 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	pl330->mcbufsz = pdat ? pdat->mcbuf_sz : 0;
 
+	if (of_find_property(np, "arm,pl330-periph-burst", NULL))
+		pl330->peripherals_req_type = BURST;
+	else
+		pl330->peripherals_req_type = SINGLE;
+
 	/* get quirk */
 	for (i = 0; i < ARRAY_SIZE(of_quirks); i++)
 		if (of_property_read_bool(np, of_quirks[i].quirk))
-- 
2.3.7

[PATCH 3/3] dmaengine: pl330: support transfer unaligned with (burst len * burst size)

[Shawn Lin]

Currently pl330 doesn't support transfer which doesn't
align with burst len * burst size. This should be only
for single mode. Let's allow it for busrt mode if available.

e.g. transfers 0x10002 bytes:
First loop 256*16*16=0x10000, burst size is 1, burst length is 16.
Then the second loop 2 bytes, burst size is 1, burst length is 1.

f0041000:        DMAMOV CCR 0xbc02f1
f0041006:        DMAMOV SAR 0xdd6c0000
f004100c:        DMAMOV DAR 0xff1d0400
f0041012:        DMALP_0 15
f0041014:        DMALP_1 255
f0041016:        DMAWFPB 12
f0041018:        DMALDA
f0041019:        DMASTPB 12
f004101b:        DMAFLUSHP 12
f004101d:        DMALPENDA_1 bjmpto_7
f004101f:        DMALPENDA_0 bjmpto_b
f0041021:        DMAMOV CCR 0x800201
f0041027:        DMALP_1 1
f0041029:        DMAWFPB 12
f004102b:        DMALDA
f004102c:        DMASTPB 12
f004102e:        DMAFLUSHP 12
f0041030:        DMALPENDA_1 bjmpto_7
f0041032:        DMASEV 0
f0041034:        DMAEND

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/dma/pl330.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index a09bf22..cdb4afc 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -240,6 +240,7 @@ 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 BYTE_MOD_BURST_LEN(b, ccr)	(((b) / BRST_SIZE(ccr)) % BRST_LEN(ccr))
 
 /*
  * With 256 bytes, we can do more than 2.5MB and 5MB xfers per req
@@ -1331,6 +1332,19 @@ static inline int _setup_xfer(struct pl330_dmac *pl330,
 	/* Setup Loop(s) */
 	off += _setup_loops(pl330, dry_run, &buf[off], pxs);
 
+	if (pl330->peripherals_req_type == BURST) {
+		unsigned int ccr = pxs->ccr;
+		unsigned long c = 0;
+
+		c = BYTE_MOD_BURST_LEN(x->bytes, pxs->ccr);
+		if (c) {
+			ccr &= ~(0xf << CC_SRCBRSTLEN_SHFT);
+			ccr &= ~(0xf << CC_DSTBRSTLEN_SHFT);
+			off += _emit_MOV(dry_run, &buf[off], CCR, ccr);
+			off += _loop(pl330, dry_run, &buf[off], &c, pxs);
+		}
+	}
+
 	return off;
 }
 
@@ -1353,9 +1367,12 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 	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;
+
+	if (pl330->peripherals_req_type != BURST) {
+		/* 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);
 
-- 
2.3.7

[PATCH 3/3] dmaengine: pl330: support transfer unaligned with (burst len * burst size)

[Shawn Lin]

Currently pl330 doesn't support transfer which doesn't
align with burst len * burst size. This should be only
for single mode. Let's allow it for busrt mode if available.

e.g. transfers 0x10002 bytes:
First loop 256*16*16=0x10000, burst size is 1, burst length is 16.
Then the second loop 2 bytes, burst size is 1, burst length is 1.

f0041000:        DMAMOV CCR 0xbc02f1
f0041006:        DMAMOV SAR 0xdd6c0000
f004100c:        DMAMOV DAR 0xff1d0400
f0041012:        DMALP_0 15
f0041014:        DMALP_1 255
f0041016:        DMAWFPB 12
f0041018:        DMALDA
f0041019:        DMASTPB 12
f004101b:        DMAFLUSHP 12
f004101d:        DMALPENDA_1 bjmpto_7
f004101f:        DMALPENDA_0 bjmpto_b
f0041021:        DMAMOV CCR 0x800201
f0041027:        DMALP_1 1
f0041029:        DMAWFPB 12
f004102b:        DMALDA
f004102c:        DMASTPB 12
f004102e:        DMAFLUSHP 12
f0041030:        DMALPENDA_1 bjmpto_7
f0041032:        DMASEV 0
f0041034:        DMAEND

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 drivers/dma/pl330.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index a09bf22..cdb4afc 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -240,6 +240,7 @@ 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 BYTE_MOD_BURST_LEN(b, ccr)	(((b) / BRST_SIZE(ccr)) % BRST_LEN(ccr))
 
 /*
  * With 256 bytes, we can do more than 2.5MB and 5MB xfers per req
@@ -1331,6 +1332,19 @@ static inline int _setup_xfer(struct pl330_dmac *pl330,
 	/* Setup Loop(s) */
 	off += _setup_loops(pl330, dry_run, &buf[off], pxs);
 
+	if (pl330->peripherals_req_type == BURST) {
+		unsigned int ccr = pxs->ccr;
+		unsigned long c = 0;
+
+		c = BYTE_MOD_BURST_LEN(x->bytes, pxs->ccr);
+		if (c) {
+			ccr &= ~(0xf << CC_SRCBRSTLEN_SHFT);
+			ccr &= ~(0xf << CC_DSTBRSTLEN_SHFT);
+			off += _emit_MOV(dry_run, &buf[off], CCR, ccr);
+			off += _loop(pl330, dry_run, &buf[off], &c, pxs);
+		}
+	}
+
 	return off;
 }
 
@@ -1353,9 +1367,12 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 	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;
+
+	if (pl330->peripherals_req_type != BURST) {
+		/* 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);
 
-- 
2.3.7

[PATCH 3/3] dmaengine: pl330: support transfer unaligned with (burst len * burst size)

[Shawn Lin]

Currently pl330 doesn't support transfer which doesn't
align with burst len * burst size. This should be only
for single mode. Let's allow it for busrt mode if available.

e.g. transfers 0x10002 bytes:
First loop 256*16*16=0x10000, burst size is 1, burst length is 16.
Then the second loop 2 bytes, burst size is 1, burst length is 1.

f0041000:        DMAMOV CCR 0xbc02f1
f0041006:        DMAMOV SAR 0xdd6c0000
f004100c:        DMAMOV DAR 0xff1d0400
f0041012:        DMALP_0 15
f0041014:        DMALP_1 255
f0041016:        DMAWFPB 12
f0041018:        DMALDA
f0041019:        DMASTPB 12
f004101b:        DMAFLUSHP 12
f004101d:        DMALPENDA_1 bjmpto_7
f004101f:        DMALPENDA_0 bjmpto_b
f0041021:        DMAMOV CCR 0x800201
f0041027:        DMALP_1 1
f0041029:        DMAWFPB 12
f004102b:        DMALDA
f004102c:        DMASTPB 12
f004102e:        DMAFLUSHP 12
f0041030:        DMALPENDA_1 bjmpto_7
f0041032:        DMASEV 0
f0041034:        DMAEND

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/dma/pl330.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index a09bf22..cdb4afc 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -240,6 +240,7 @@ 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 BYTE_MOD_BURST_LEN(b, ccr)	(((b) / BRST_SIZE(ccr)) % BRST_LEN(ccr))
 
 /*
  * With 256 bytes, we can do more than 2.5MB and 5MB xfers per req
@@ -1331,6 +1332,19 @@ static inline int _setup_xfer(struct pl330_dmac *pl330,
 	/* Setup Loop(s) */
 	off += _setup_loops(pl330, dry_run, &buf[off], pxs);
 
+	if (pl330->peripherals_req_type == BURST) {
+		unsigned int ccr = pxs->ccr;
+		unsigned long c = 0;
+
+		c = BYTE_MOD_BURST_LEN(x->bytes, pxs->ccr);
+		if (c) {
+			ccr &= ~(0xf << CC_SRCBRSTLEN_SHFT);
+			ccr &= ~(0xf << CC_DSTBRSTLEN_SHFT);
+			off += _emit_MOV(dry_run, &buf[off], CCR, ccr);
+			off += _loop(pl330, dry_run, &buf[off], &c, pxs);
+		}
+	}
+
 	return off;
 }
 
@@ -1353,9 +1367,12 @@ static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
 	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;
+
+	if (pl330->peripherals_req_type != BURST) {
+		/* 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);
 
-- 
2.3.7

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Vinod Koul]

On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
> support busrt mode.

why should this be DT property. Only reason I can think of if some hw
versions support this and some won't.

If all are supporting, please enable it everywhere for obvious reasons.

-- 
~Vinod

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Vinod Koul]

On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
> support busrt mode.

why should this be DT property. Only reason I can think of if some hw
versions support this and some won't.

If all are supporting, please enable it everywhere for obvious reasons.

-- 
~Vinod

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi Vinod,

在 2016/8/5 11:34, Vinod Koul 写道:
> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>> support busrt mode.
>
> why should this be DT property. Only reason I can think of if some hw
> versions support this and some won't.

yes, if we want to support burst mode, both of the master(pl330) and
client(several peripherals) should implement it, otherwise it will
be broken when enabling.

So I mentioned it on my cover letter as the reason to introduce this
optional property. If people add this property and find it's ok for
their platform, they could land new dt-patch to add it. But we could
*not* presume that we could get all users involved in testing this
patchet. I don't wanna to break any other platforms, so it's needed.


Thanks.

>
> If all are supporting, please enable it everywhere for obvious reasons.
>


-- 
Best Regards
Shawn Lin

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi Vinod,

? 2016/8/5 11:34, Vinod Koul ??:
> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>> support busrt mode.
>
> why should this be DT property. Only reason I can think of if some hw
> versions support this and some won't.

yes, if we want to support burst mode, both of the master(pl330) and
client(several peripherals) should implement it, otherwise it will
be broken when enabling.

So I mentioned it on my cover letter as the reason to introduce this
optional property. If people add this property and find it's ok for
their platform, they could land new dt-patch to add it. But we could
*not* presume that we could get all users involved in testing this
patchet. I don't wanna to break any other platforms, so it's needed.


Thanks.

>
> If all are supporting, please enable it everywhere for obvious reasons.
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 2/3] dmaengine: pl330: enable burst mode by parsing dt

[Xing Zheng]

Hi Shawn,

On 2016年08月05日 10:53, Shawn Lin wrote:
> Currently pl330 use single mode defaultly. But burst
> mode can improve efficiency of memory accessing. We
> couldn't enable it by defalut in case of breaking any
> Socs which don't support it.
>
> With burst mode supported, we could see the improvement
> significantly when tesing SPI transfer etc.
>
> default single mode
> [   88.292550] spi write 65536*1 cost 32402us speed:2022KB/S
>
> After applied with burst mode(len 16)
> [   17.625296] spi write 65536*1 cost 17830us speed:3675KB/S
>
> Cc: Huibin Hong <huibin.hong@rock-chips.com>
> Cc: Xing Zheng <zhengxing@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
Tested-by: Xing Zheng <zhengxing@rock-chips.com>

Thanks

-- 
- Xing Zheng

[PATCH 2/3] dmaengine: pl330: enable burst mode by parsing dt

[Xing Zheng]

Hi Shawn,

On 2016?08?05? 10:53, Shawn Lin wrote:
> Currently pl330 use single mode defaultly. But burst
> mode can improve efficiency of memory accessing. We
> couldn't enable it by defalut in case of breaking any
> Socs which don't support it.
>
> With burst mode supported, we could see the improvement
> significantly when tesing SPI transfer etc.
>
> default single mode
> [   88.292550] spi write 65536*1 cost 32402us speed:2022KB/S
>
> After applied with burst mode(len 16)
> [   17.625296] spi write 65536*1 cost 17830us speed:3675KB/S
>
> Cc: Huibin Hong <huibin.hong@rock-chips.com>
> Cc: Xing Zheng <zhengxing@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
Tested-by: Xing Zheng <zhengxing@rock-chips.com>

Thanks

-- 
- Xing Zheng

Re: [PATCH 3/3] dmaengine: pl330: support transfer unaligned with (burst len * burst size)

[Xing Zheng]

Hi Shawn

On 2016年08月05日 10:53, Shawn Lin wrote:
> Currently pl330 doesn't support transfer which doesn't
> align with burst len * burst size. This should be only
> for single mode. Let's allow it for busrt mode if available.
>
> e.g. transfers 0x10002 bytes:
> First loop 256*16*16=0x10000, burst size is 1, burst length is 16.
> Then the second loop 2 bytes, burst size is 1, burst length is 1.
>
> f0041000:        DMAMOV CCR 0xbc02f1
> f0041006:        DMAMOV SAR 0xdd6c0000
> f004100c:        DMAMOV DAR 0xff1d0400
> f0041012:        DMALP_0 15
> f0041014:        DMALP_1 255
> f0041016:        DMAWFPB 12
> f0041018:        DMALDA
> f0041019:        DMASTPB 12
> f004101b:        DMAFLUSHP 12
> f004101d:        DMALPENDA_1 bjmpto_7
> f004101f:        DMALPENDA_0 bjmpto_b
> f0041021:        DMAMOV CCR 0x800201
> f0041027:        DMALP_1 1
> f0041029:        DMAWFPB 12
> f004102b:        DMALDA
> f004102c:        DMASTPB 12
> f004102e:        DMAFLUSHP 12
> f0041030:        DMALPENDA_1 bjmpto_7
> f0041032:        DMASEV 0
> f0041034:        DMAEND
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
Tested-by: Xing Zheng <zhengxing@rock-chips.com>

Thanks.

-- 
- Xing Zheng

Re: [PATCH 3/3] dmaengine: pl330: support transfer unaligned with (burst len * burst size)

[Xing Zheng]

Hi Shawn

On 2016年08月05日 10:53, Shawn Lin wrote:
> Currently pl330 doesn't support transfer which doesn't
> align with burst len * burst size. This should be only
> for single mode. Let's allow it for busrt mode if available.
>
> e.g. transfers 0x10002 bytes:
> First loop 256*16*16=0x10000, burst size is 1, burst length is 16.
> Then the second loop 2 bytes, burst size is 1, burst length is 1.
>
> f0041000:        DMAMOV CCR 0xbc02f1
> f0041006:        DMAMOV SAR 0xdd6c0000
> f004100c:        DMAMOV DAR 0xff1d0400
> f0041012:        DMALP_0 15
> f0041014:        DMALP_1 255
> f0041016:        DMAWFPB 12
> f0041018:        DMALDA
> f0041019:        DMASTPB 12
> f004101b:        DMAFLUSHP 12
> f004101d:        DMALPENDA_1 bjmpto_7
> f004101f:        DMALPENDA_0 bjmpto_b
> f0041021:        DMAMOV CCR 0x800201
> f0041027:        DMALP_1 1
> f0041029:        DMAWFPB 12
> f004102b:        DMALDA
> f004102c:        DMASTPB 12
> f004102e:        DMAFLUSHP 12
> f0041030:        DMALPENDA_1 bjmpto_7
> f0041032:        DMASEV 0
> f0041034:        DMAEND
>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
Tested-by: Xing Zheng <zhengxing-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

Thanks.

-- 
- Xing Zheng


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] dmaengine: pl330: support transfer unaligned with (burst len * burst size)

[Xing Zheng]

Hi Shawn

On 2016?08?05? 10:53, Shawn Lin wrote:
> Currently pl330 doesn't support transfer which doesn't
> align with burst len * burst size. This should be only
> for single mode. Let's allow it for busrt mode if available.
>
> e.g. transfers 0x10002 bytes:
> First loop 256*16*16=0x10000, burst size is 1, burst length is 16.
> Then the second loop 2 bytes, burst size is 1, burst length is 1.
>
> f0041000:        DMAMOV CCR 0xbc02f1
> f0041006:        DMAMOV SAR 0xdd6c0000
> f004100c:        DMAMOV DAR 0xff1d0400
> f0041012:        DMALP_0 15
> f0041014:        DMALP_1 255
> f0041016:        DMAWFPB 12
> f0041018:        DMALDA
> f0041019:        DMASTPB 12
> f004101b:        DMAFLUSHP 12
> f004101d:        DMALPENDA_1 bjmpto_7
> f004101f:        DMALPENDA_0 bjmpto_b
> f0041021:        DMAMOV CCR 0x800201
> f0041027:        DMALP_1 1
> f0041029:        DMAWFPB 12
> f004102b:        DMALDA
> f004102c:        DMASTPB 12
> f004102e:        DMAFLUSHP 12
> f0041030:        DMALPENDA_1 bjmpto_7
> f0041032:        DMASEV 0
> f0041034:        DMAEND
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
Tested-by: Xing Zheng <zhengxing@rock-chips.com>

Thanks.

-- 
- Xing Zheng

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Lars-Peter Clausen]

On 08/05/2016 09:25 AM, Shawn Lin wrote:
> Hi Vinod,
> 
> 在 2016/8/5 11:34, Vinod Koul 写道:
>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>> support busrt mode.
>>
>> why should this be DT property. Only reason I can think of if some hw
>> versions support this and some won't.
> 
> yes, if we want to support burst mode, both of the master(pl330) and
> client(several peripherals) should implement it, otherwise it will
> be broken when enabling.

As you said, it is up to the consumer peripheral whether it supports BURST,
SINGLE or both. So this is a per client property, but you specify this as a
a global property on the producer side.

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Lars-Peter Clausen]

On 08/05/2016 09:25 AM, Shawn Lin wrote:
> Hi Vinod,
> 
> 在 2016/8/5 11:34, Vinod Koul 写道:
>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>> support busrt mode.
>>
>> why should this be DT property. Only reason I can think of if some hw
>> versions support this and some won't.
> 
> yes, if we want to support burst mode, both of the master(pl330) and
> client(several peripherals) should implement it, otherwise it will
> be broken when enabling.

As you said, it is up to the consumer peripheral whether it supports BURST,
SINGLE or both. So this is a per client property, but you specify this as a
a global property on the producer side.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Lars-Peter Clausen]

On 08/05/2016 09:25 AM, Shawn Lin wrote:
> Hi Vinod,
> 
> ? 2016/8/5 11:34, Vinod Koul ??:
>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>> support busrt mode.
>>
>> why should this be DT property. Only reason I can think of if some hw
>> versions support this and some won't.
> 
> yes, if we want to support burst mode, both of the master(pl330) and
> client(several peripherals) should implement it, otherwise it will
> be broken when enabling.

As you said, it is up to the consumer peripheral whether it supports BURST,
SINGLE or both. So this is a per client property, but you specify this as a
a global property on the producer side.

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi Lars-Peter,

在 2016/8/9 16:39, Lars-Peter Clausen 写道:
> On 08/05/2016 09:25 AM, Shawn Lin wrote:
>> Hi Vinod,
>>
>> 在 2016/8/5 11:34, Vinod Koul 写道:
>>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>>> support busrt mode.
>>>
>>> why should this be DT property. Only reason I can think of if some hw
>>> versions support this and some won't.
>>
>> yes, if we want to support burst mode, both of the master(pl330) and
>> client(several peripherals) should implement it, otherwise it will
>> be broken when enabling.
>
> As you said, it is up to the consumer peripheral whether it supports BURST,
> SINGLE or both. So this is a per client property, but you specify this as a
> a global property on the producer side.

Thanks for comment.

yup, but what is the proper way to add it ? :)


a) If pl330 support BURST as well as all the peripherals, we could
enable it.

b) If pl300 support BURST, but all the peripherals don't support it,
we could not enable it.

c) If pl300 support BURST, but not all the peripherals support it,
we also could not enable it.

the burst feature of peripheral IP may be vendor-specific, but the
common driver for this peripheral are used for many many vendors which
means we could not check all of this info. It's very likely to break
them... I couldn't figure out how many upstreamed peripheral drivers
who are using pl300 either.

So this check should be done by all this vendors but we could make
sure we don't break them before they check a), b), c), right?


>
>
>
>


-- 
Best Regards
Shawn Lin

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi Lars-Peter,

? 2016/8/9 16:39, Lars-Peter Clausen ??:
> On 08/05/2016 09:25 AM, Shawn Lin wrote:
>> Hi Vinod,
>>
>> ? 2016/8/5 11:34, Vinod Koul ??:
>>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>>> support busrt mode.
>>>
>>> why should this be DT property. Only reason I can think of if some hw
>>> versions support this and some won't.
>>
>> yes, if we want to support burst mode, both of the master(pl330) and
>> client(several peripherals) should implement it, otherwise it will
>> be broken when enabling.
>
> As you said, it is up to the consumer peripheral whether it supports BURST,
> SINGLE or both. So this is a per client property, but you specify this as a
> a global property on the producer side.

Thanks for comment.

yup, but what is the proper way to add it ? :)


a) If pl330 support BURST as well as all the peripherals, we could
enable it.

b) If pl300 support BURST, but all the peripherals don't support it,
we could not enable it.

c) If pl300 support BURST, but not all the peripherals support it,
we also could not enable it.

the burst feature of peripheral IP may be vendor-specific, but the
common driver for this peripheral are used for many many vendors which
means we could not check all of this info. It's very likely to break
them... I couldn't figure out how many upstreamed peripheral drivers
who are using pl300 either.

So this check should be done by all this vendors but we could make
sure we don't break them before they check a), b), c), right?


>
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi, Vinod and Lars-Peter

Ping.. Any better idea to share :)

On 2016/8/9 17:12, Shawn Lin wrote:
> Hi Lars-Peter,
>
> 在 2016/8/9 16:39, Lars-Peter Clausen 写道:
>> On 08/05/2016 09:25 AM, Shawn Lin wrote:
>>> Hi Vinod,
>>>
>>> 在 2016/8/5 11:34, Vinod Koul 写道:
>>>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>>>> support busrt mode.
>>>>
>>>> why should this be DT property. Only reason I can think of if some hw
>>>> versions support this and some won't.
>>>
>>> yes, if we want to support burst mode, both of the master(pl330) and
>>> client(several peripherals) should implement it, otherwise it will
>>> be broken when enabling.
>>
>> As you said, it is up to the consumer peripheral whether it supports
>> BURST,
>> SINGLE or both. So this is a per client property, but you specify this
>> as a
>> a global property on the producer side.
>
> Thanks for comment.
>
> yup, but what is the proper way to add it ? :)
>
>
> a) If pl330 support BURST as well as all the peripherals, we could
> enable it.
>
> b) If pl300 support BURST, but all the peripherals don't support it,
> we could not enable it.
>
> c) If pl300 support BURST, but not all the peripherals support it,
> we also could not enable it.
>
> the burst feature of peripheral IP may be vendor-specific, but the
> common driver for this peripheral are used for many many vendors which
> means we could not check all of this info. It's very likely to break
> them... I couldn't figure out how many upstreamed peripheral drivers
> who are using pl300 either.
>
> So this check should be done by all this vendors but we could make
> sure we don't break them before they check a), b), c), right?
>
>
>>
>>
>>
>>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi, Vinod and Lars-Peter

Ping.. Any better idea to share :)

On 2016/8/9 17:12, Shawn Lin wrote:
> Hi Lars-Peter,
>
> 在 2016/8/9 16:39, Lars-Peter Clausen 写道:
>> On 08/05/2016 09:25 AM, Shawn Lin wrote:
>>> Hi Vinod,
>>>
>>> 在 2016/8/5 11:34, Vinod Koul 写道:
>>>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>>>> support busrt mode.
>>>>
>>>> why should this be DT property. Only reason I can think of if some hw
>>>> versions support this and some won't.
>>>
>>> yes, if we want to support burst mode, both of the master(pl330) and
>>> client(several peripherals) should implement it, otherwise it will
>>> be broken when enabling.
>>
>> As you said, it is up to the consumer peripheral whether it supports
>> BURST,
>> SINGLE or both. So this is a per client property, but you specify this
>> as a
>> a global property on the producer side.
>
> Thanks for comment.
>
> yup, but what is the proper way to add it ? :)
>
>
> a) If pl330 support BURST as well as all the peripherals, we could
> enable it.
>
> b) If pl300 support BURST, but all the peripherals don't support it,
> we could not enable it.
>
> c) If pl300 support BURST, but not all the peripherals support it,
> we also could not enable it.
>
> the burst feature of peripheral IP may be vendor-specific, but the
> common driver for this peripheral are used for many many vendors which
> means we could not check all of this info. It's very likely to break
> them... I couldn't figure out how many upstreamed peripheral drivers
> who are using pl300 either.
>
> So this check should be done by all this vendors but we could make
> sure we don't break them before they check a), b), c), right?
>
>
>>
>>
>>
>>
>
>


-- 
Best Regards
Shawn Lin

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi, Vinod and Lars-Peter

Ping.. Any better idea to share :)

On 2016/8/9 17:12, Shawn Lin wrote:
> Hi Lars-Peter,
>
> ? 2016/8/9 16:39, Lars-Peter Clausen ??:
>> On 08/05/2016 09:25 AM, Shawn Lin wrote:
>>> Hi Vinod,
>>>
>>> ? 2016/8/5 11:34, Vinod Koul ??:
>>>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>>>> support busrt mode.
>>>>
>>>> why should this be DT property. Only reason I can think of if some hw
>>>> versions support this and some won't.
>>>
>>> yes, if we want to support burst mode, both of the master(pl330) and
>>> client(several peripherals) should implement it, otherwise it will
>>> be broken when enabling.
>>
>> As you said, it is up to the consumer peripheral whether it supports
>> BURST,
>> SINGLE or both. So this is a per client property, but you specify this
>> as a
>> a global property on the producer side.
>
> Thanks for comment.
>
> yup, but what is the proper way to add it ? :)
>
>
> a) If pl330 support BURST as well as all the peripherals, we could
> enable it.
>
> b) If pl300 support BURST, but all the peripherals don't support it,
> we could not enable it.
>
> c) If pl300 support BURST, but not all the peripherals support it,
> we also could not enable it.
>
> the burst feature of peripheral IP may be vendor-specific, but the
> common driver for this peripheral are used for many many vendors which
> means we could not check all of this info. It's very likely to break
> them... I couldn't figure out how many upstreamed peripheral drivers
> who are using pl300 either.
>
> So this check should be done by all this vendors but we could make
> sure we don't break them before they check a), b), c), right?
>
>
>>
>>
>>
>>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Vinod Koul]

On Wed, Aug 17, 2016 at 04:11:03PM +0800, Shawn Lin wrote:
> Hi, Vinod and Lars-Peter
> 
> Ping.. Any better idea to share :)
> 
> On 2016/8/9 17:12, Shawn Lin wrote:
> >Hi Lars-Peter,
> >
> >在 2016/8/9 16:39, Lars-Peter Clausen 写道:
> >>On 08/05/2016 09:25 AM, Shawn Lin wrote:
> >>>Hi Vinod,
> >>>
> >>>在 2016/8/5 11:34, Vinod Koul 写道:
> >>>>On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
> >>>>>This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
> >>>>>support busrt mode.
> >>>>
> >>>>why should this be DT property. Only reason I can think of if some hw
> >>>>versions support this and some won't.
> >>>
> >>>yes, if we want to support burst mode, both of the master(pl330) and
> >>>client(several peripherals) should implement it, otherwise it will
> >>>be broken when enabling.
> >>
> >>As you said, it is up to the consumer peripheral whether it supports
> >>BURST,
> >>SINGLE or both. So this is a per client property, but you specify this
> >>as a
> >>a global property on the producer side.
> >
> >Thanks for comment.
> >
> >yup, but what is the proper way to add it ? :)
> >
> >
> >a) If pl330 support BURST as well as all the peripherals, we could
> >enable it.
> >
> >b) If pl300 support BURST, but all the peripherals don't support it,
> >we could not enable it.
> >
> >c) If pl300 support BURST, but not all the peripherals support it,
> >we also could not enable it.
> >
> >the burst feature of peripheral IP may be vendor-specific, but the
> >common driver for this peripheral are used for many many vendors which
> >means we could not check all of this info. It's very likely to break
> >them... I couldn't figure out how many upstreamed peripheral drivers
> >who are using pl300 either.
> >
> >So this check should be done by all this vendors but we could make
> >sure we don't break them before they check a), b), c), right?

Since support for BURST needs to be from peripheral too, we should have
that as a property for peripheral not for controller.

The peripheral drivers can communicate the burst to be used to pl330
using src_maxburst/dst_maxburst in dma_slave_config. We can use this
value to indicate the DMA should be single (a value of 0) or burst with
"burst" value.

-- 
~Vinod

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Vinod Koul]

On Wed, Aug 17, 2016 at 04:11:03PM +0800, Shawn Lin wrote:
> Hi, Vinod and Lars-Peter
> 
> Ping.. Any better idea to share :)
> 
> On 2016/8/9 17:12, Shawn Lin wrote:
> >Hi Lars-Peter,
> >
> >? 2016/8/9 16:39, Lars-Peter Clausen ??:
> >>On 08/05/2016 09:25 AM, Shawn Lin wrote:
> >>>Hi Vinod,
> >>>
> >>>? 2016/8/5 11:34, Vinod Koul ??:
> >>>>On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
> >>>>>This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
> >>>>>support busrt mode.
> >>>>
> >>>>why should this be DT property. Only reason I can think of if some hw
> >>>>versions support this and some won't.
> >>>
> >>>yes, if we want to support burst mode, both of the master(pl330) and
> >>>client(several peripherals) should implement it, otherwise it will
> >>>be broken when enabling.
> >>
> >>As you said, it is up to the consumer peripheral whether it supports
> >>BURST,
> >>SINGLE or both. So this is a per client property, but you specify this
> >>as a
> >>a global property on the producer side.
> >
> >Thanks for comment.
> >
> >yup, but what is the proper way to add it ? :)
> >
> >
> >a) If pl330 support BURST as well as all the peripherals, we could
> >enable it.
> >
> >b) If pl300 support BURST, but all the peripherals don't support it,
> >we could not enable it.
> >
> >c) If pl300 support BURST, but not all the peripherals support it,
> >we also could not enable it.
> >
> >the burst feature of peripheral IP may be vendor-specific, but the
> >common driver for this peripheral are used for many many vendors which
> >means we could not check all of this info. It's very likely to break
> >them... I couldn't figure out how many upstreamed peripheral drivers
> >who are using pl300 either.
> >
> >So this check should be done by all this vendors but we could make
> >sure we don't break them before they check a), b), c), right?

Since support for BURST needs to be from peripheral too, we should have
that as a property for peripheral not for controller.

The peripheral drivers can communicate the burst to be used to pl330
using src_maxburst/dst_maxburst in dma_slave_config. We can use this
value to indicate the DMA should be single (a value of 0) or burst with
"burst" value.

-- 
~Vinod

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi Vinod,

在 2016/8/19 10:45, Vinod Koul 写道:
> On Wed, Aug 17, 2016 at 04:11:03PM +0800, Shawn Lin wrote:
>> Hi, Vinod and Lars-Peter
>>
>> Ping.. Any better idea to share :)
>>
>> On 2016/8/9 17:12, Shawn Lin wrote:
>>> Hi Lars-Peter,
>>>
>>> 在 2016/8/9 16:39, Lars-Peter Clausen 写道:
>>>> On 08/05/2016 09:25 AM, Shawn Lin wrote:
>>>>> Hi Vinod,
>>>>>
>>>>> 在 2016/8/5 11:34, Vinod Koul 写道:
>>>>>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>>>>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>>>>>> support busrt mode.
>>>>>>
>>>>>> why should this be DT property. Only reason I can think of if some hw
>>>>>> versions support this and some won't.
>>>>>
>>>>> yes, if we want to support burst mode, both of the master(pl330) and
>>>>> client(several peripherals) should implement it, otherwise it will
>>>>> be broken when enabling.
>>>>
>>>> As you said, it is up to the consumer peripheral whether it supports
>>>> BURST,
>>>> SINGLE or both. So this is a per client property, but you specify this
>>>> as a
>>>> a global property on the producer side.
>>>
>>> Thanks for comment.
>>>
>>> yup, but what is the proper way to add it ? :)
>>>
>>>
>>> a) If pl330 support BURST as well as all the peripherals, we could
>>> enable it.
>>>
>>> b) If pl300 support BURST, but all the peripherals don't support it,
>>> we could not enable it.
>>>
>>> c) If pl300 support BURST, but not all the peripherals support it,
>>> we also could not enable it.
>>>
>>> the burst feature of peripheral IP may be vendor-specific, but the
>>> common driver for this peripheral are used for many many vendors which
>>> means we could not check all of this info. It's very likely to break
>>> them... I couldn't figure out how many upstreamed peripheral drivers
>>> who are using pl300 either.
>>>
>>> So this check should be done by all this vendors but we could make
>>> sure we don't break them before they check a), b), c), right?
>
> Since support for BURST needs to be from peripheral too, we should have
> that as a property for peripheral not for controller.
>
> The peripheral drivers can communicate the burst to be used to pl330
> using src_maxburst/dst_maxburst in dma_slave_config. We can use this
> value to indicate the DMA should be single (a value of 0) or burst with
> "burst" value.

Thanks for sharing this.

But this is really a difficult trade-off decision to add this new
property for pl330 only.

Burst mode was supported by Boojin Kim's patch by default(commit
848e9776fee42 ("dmaengine: pl330: support burst mode for dev-to-mem and
mem-to-dev transmit")).  But we found it will break SoCFPGA or
Exynos4412 reported by Dinh Nguyen and Bartlomiej Zolnierkiewicz[0].
So finally Caesar Wang contributed a patch, commit 0a18f9b268 ("
dmaengine: pl330: fix to support the burst mode") to fix it, but what
it actually did is to use single burst for any case, namely some kind
of regression for Boojin Kim's improvement.

So we can see these drivers which was broken by enabling burst mode
had already set src_maxburst/dst_maxburst. It looks to me so unfortunate
that the driver like 8250_dw.c was using so widely that we couldn't
set scr/dst_maxburst as this is really vendor specific for whether it
supports burst for 8250_dw or not..

So it is quite painful that we probably will get dozens of regression
reports when enabling burst mode by default. But without this, we have
been suffering from low performance quite a long time due to this
roadblock.

Two possible paths to land this patch are:
(1) Keep this property for pl330 only, so we have no chance to
break others and we could make the platforms enjoy it if adding this
property for their own dts.

(2) Figuer out all the broken platfroms if enabling burst and fix them
one by one for the src/dst_maxburst(maybe by enabling burst mode and
get regression reports). If we could not solve any one of them, then we
have to give up all the effort we do, and let this pain keep on
stalling people's expectation of better performance.


I would appreciate it if you could share your thought more, as I really 
want more platforms benefit from it(at least don't break them)  :)


[0] http://www.gossamer-threads.com/lists/linux/kernel/2374171

>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi Vinod,

在 2016/8/19 10:45, Vinod Koul 写道:
> On Wed, Aug 17, 2016 at 04:11:03PM +0800, Shawn Lin wrote:
>> Hi, Vinod and Lars-Peter
>>
>> Ping.. Any better idea to share :)
>>
>> On 2016/8/9 17:12, Shawn Lin wrote:
>>> Hi Lars-Peter,
>>>
>>> 在 2016/8/9 16:39, Lars-Peter Clausen 写道:
>>>> On 08/05/2016 09:25 AM, Shawn Lin wrote:
>>>>> Hi Vinod,
>>>>>
>>>>> 在 2016/8/5 11:34, Vinod Koul 写道:
>>>>>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>>>>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>>>>>> support busrt mode.
>>>>>>
>>>>>> why should this be DT property. Only reason I can think of if some hw
>>>>>> versions support this and some won't.
>>>>>
>>>>> yes, if we want to support burst mode, both of the master(pl330) and
>>>>> client(several peripherals) should implement it, otherwise it will
>>>>> be broken when enabling.
>>>>
>>>> As you said, it is up to the consumer peripheral whether it supports
>>>> BURST,
>>>> SINGLE or both. So this is a per client property, but you specify this
>>>> as a
>>>> a global property on the producer side.
>>>
>>> Thanks for comment.
>>>
>>> yup, but what is the proper way to add it ? :)
>>>
>>>
>>> a) If pl330 support BURST as well as all the peripherals, we could
>>> enable it.
>>>
>>> b) If pl300 support BURST, but all the peripherals don't support it,
>>> we could not enable it.
>>>
>>> c) If pl300 support BURST, but not all the peripherals support it,
>>> we also could not enable it.
>>>
>>> the burst feature of peripheral IP may be vendor-specific, but the
>>> common driver for this peripheral are used for many many vendors which
>>> means we could not check all of this info. It's very likely to break
>>> them... I couldn't figure out how many upstreamed peripheral drivers
>>> who are using pl300 either.
>>>
>>> So this check should be done by all this vendors but we could make
>>> sure we don't break them before they check a), b), c), right?
>
> Since support for BURST needs to be from peripheral too, we should have
> that as a property for peripheral not for controller.
>
> The peripheral drivers can communicate the burst to be used to pl330
> using src_maxburst/dst_maxburst in dma_slave_config. We can use this
> value to indicate the DMA should be single (a value of 0) or burst with
> "burst" value.

Thanks for sharing this.

But this is really a difficult trade-off decision to add this new
property for pl330 only.

Burst mode was supported by Boojin Kim's patch by default(commit
848e9776fee42 ("dmaengine: pl330: support burst mode for dev-to-mem and
mem-to-dev transmit")).  But we found it will break SoCFPGA or
Exynos4412 reported by Dinh Nguyen and Bartlomiej Zolnierkiewicz[0].
So finally Caesar Wang contributed a patch, commit 0a18f9b268 ("
dmaengine: pl330: fix to support the burst mode") to fix it, but what
it actually did is to use single burst for any case, namely some kind
of regression for Boojin Kim's improvement.

So we can see these drivers which was broken by enabling burst mode
had already set src_maxburst/dst_maxburst. It looks to me so unfortunate
that the driver like 8250_dw.c was using so widely that we couldn't
set scr/dst_maxburst as this is really vendor specific for whether it
supports burst for 8250_dw or not..

So it is quite painful that we probably will get dozens of regression
reports when enabling burst mode by default. But without this, we have
been suffering from low performance quite a long time due to this
roadblock.

Two possible paths to land this patch are:
(1) Keep this property for pl330 only, so we have no chance to
break others and we could make the platforms enjoy it if adding this
property for their own dts.

(2) Figuer out all the broken platfroms if enabling burst and fix them
one by one for the src/dst_maxburst(maybe by enabling burst mode and
get regression reports). If we could not solve any one of them, then we
have to give up all the effort we do, and let this pain keep on
stalling people's expectation of better performance.


I would appreciate it if you could share your thought more, as I really 
want more platforms benefit from it(at least don't break them)  :)


[0] http://www.gossamer-threads.com/lists/linux/kernel/2374171

>


-- 
Best Regards
Shawn Lin

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Shawn Lin]

Hi Vinod,

? 2016/8/19 10:45, Vinod Koul ??:
> On Wed, Aug 17, 2016 at 04:11:03PM +0800, Shawn Lin wrote:
>> Hi, Vinod and Lars-Peter
>>
>> Ping.. Any better idea to share :)
>>
>> On 2016/8/9 17:12, Shawn Lin wrote:
>>> Hi Lars-Peter,
>>>
>>> ? 2016/8/9 16:39, Lars-Peter Clausen ??:
>>>> On 08/05/2016 09:25 AM, Shawn Lin wrote:
>>>>> Hi Vinod,
>>>>>
>>>>> ? 2016/8/5 11:34, Vinod Koul ??:
>>>>>> On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
>>>>>>> This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
>>>>>>> support busrt mode.
>>>>>>
>>>>>> why should this be DT property. Only reason I can think of if some hw
>>>>>> versions support this and some won't.
>>>>>
>>>>> yes, if we want to support burst mode, both of the master(pl330) and
>>>>> client(several peripherals) should implement it, otherwise it will
>>>>> be broken when enabling.
>>>>
>>>> As you said, it is up to the consumer peripheral whether it supports
>>>> BURST,
>>>> SINGLE or both. So this is a per client property, but you specify this
>>>> as a
>>>> a global property on the producer side.
>>>
>>> Thanks for comment.
>>>
>>> yup, but what is the proper way to add it ? :)
>>>
>>>
>>> a) If pl330 support BURST as well as all the peripherals, we could
>>> enable it.
>>>
>>> b) If pl300 support BURST, but all the peripherals don't support it,
>>> we could not enable it.
>>>
>>> c) If pl300 support BURST, but not all the peripherals support it,
>>> we also could not enable it.
>>>
>>> the burst feature of peripheral IP may be vendor-specific, but the
>>> common driver for this peripheral are used for many many vendors which
>>> means we could not check all of this info. It's very likely to break
>>> them... I couldn't figure out how many upstreamed peripheral drivers
>>> who are using pl300 either.
>>>
>>> So this check should be done by all this vendors but we could make
>>> sure we don't break them before they check a), b), c), right?
>
> Since support for BURST needs to be from peripheral too, we should have
> that as a property for peripheral not for controller.
>
> The peripheral drivers can communicate the burst to be used to pl330
> using src_maxburst/dst_maxburst in dma_slave_config. We can use this
> value to indicate the DMA should be single (a value of 0) or burst with
> "burst" value.

Thanks for sharing this.

But this is really a difficult trade-off decision to add this new
property for pl330 only.

Burst mode was supported by Boojin Kim's patch by default(commit
848e9776fee42 ("dmaengine: pl330: support burst mode for dev-to-mem and
mem-to-dev transmit")).  But we found it will break SoCFPGA or
Exynos4412 reported by Dinh Nguyen and Bartlomiej Zolnierkiewicz[0].
So finally Caesar Wang contributed a patch, commit 0a18f9b268 ("
dmaengine: pl330: fix to support the burst mode") to fix it, but what
it actually did is to use single burst for any case, namely some kind
of regression for Boojin Kim's improvement.

So we can see these drivers which was broken by enabling burst mode
had already set src_maxburst/dst_maxburst. It looks to me so unfortunate
that the driver like 8250_dw.c was using so widely that we couldn't
set scr/dst_maxburst as this is really vendor specific for whether it
supports burst for 8250_dw or not..

So it is quite painful that we probably will get dozens of regression
reports when enabling burst mode by default. But without this, we have
been suffering from low performance quite a long time due to this
roadblock.

Two possible paths to land this patch are:
(1) Keep this property for pl330 only, so we have no chance to
break others and we could make the platforms enjoy it if adding this
property for their own dts.

(2) Figuer out all the broken platfroms if enabling burst and fix them
one by one for the src/dst_maxburst(maybe by enabling burst mode and
get regression reports). If we could not solve any one of them, then we
have to give up all the effort we do, and let this pain keep on
stalling people's expectation of better performance.


I would appreciate it if you could share your thought more, as I really 
want more platforms benefit from it(at least don't break them)  :)


[0] http://www.gossamer-threads.com/lists/linux/kernel/2374171

>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Vinod Koul]

On Sun, Aug 21, 2016 at 09:00:58AM +0800, Shawn Lin wrote:
> Hi Vinod,
> 
> 在 2016/8/19 10:45, Vinod Koul 写道:
> >On Wed, Aug 17, 2016 at 04:11:03PM +0800, Shawn Lin wrote:
> >>Hi, Vinod and Lars-Peter
> >>
> >>Ping.. Any better idea to share :)
> >>
> >>On 2016/8/9 17:12, Shawn Lin wrote:
> >>>Hi Lars-Peter,
> >>>
> >>>在 2016/8/9 16:39, Lars-Peter Clausen 写道:
> >>>>On 08/05/2016 09:25 AM, Shawn Lin wrote:
> >>>>>Hi Vinod,
> >>>>>
> >>>>>在 2016/8/5 11:34, Vinod Koul 写道:
> >>>>>>On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
> >>>>>>>This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
> >>>>>>>support busrt mode.
> >>>>>>
> >>>>>>why should this be DT property. Only reason I can think of if some hw
> >>>>>>versions support this and some won't.
> >>>>>
> >>>>>yes, if we want to support burst mode, both of the master(pl330) and
> >>>>>client(several peripherals) should implement it, otherwise it will
> >>>>>be broken when enabling.
> >>>>
> >>>>As you said, it is up to the consumer peripheral whether it supports
> >>>>BURST,
> >>>>SINGLE or both. So this is a per client property, but you specify this
> >>>>as a
> >>>>a global property on the producer side.
> >>>
> >>>Thanks for comment.
> >>>
> >>>yup, but what is the proper way to add it ? :)
> >>>
> >>>
> >>>a) If pl330 support BURST as well as all the peripherals, we could
> >>>enable it.
> >>>
> >>>b) If pl300 support BURST, but all the peripherals don't support it,
> >>>we could not enable it.
> >>>
> >>>c) If pl300 support BURST, but not all the peripherals support it,
> >>>we also could not enable it.
> >>>
> >>>the burst feature of peripheral IP may be vendor-specific, but the
> >>>common driver for this peripheral are used for many many vendors which
> >>>means we could not check all of this info. It's very likely to break
> >>>them... I couldn't figure out how many upstreamed peripheral drivers
> >>>who are using pl300 either.
> >>>
> >>>So this check should be done by all this vendors but we could make
> >>>sure we don't break them before they check a), b), c), right?
> >
> >Since support for BURST needs to be from peripheral too, we should have
> >that as a property for peripheral not for controller.
> >
> >The peripheral drivers can communicate the burst to be used to pl330
> >using src_maxburst/dst_maxburst in dma_slave_config. We can use this
> >value to indicate the DMA should be single (a value of 0) or burst with
> >"burst" value.
> 
> Thanks for sharing this.
> 
> But this is really a difficult trade-off decision to add this new
> property for pl330 only.
> 
> Burst mode was supported by Boojin Kim's patch by default(commit
> 848e9776fee42 ("dmaengine: pl330: support burst mode for dev-to-mem and
> mem-to-dev transmit")).  But we found it will break SoCFPGA or
> Exynos4412 reported by Dinh Nguyen and Bartlomiej Zolnierkiewicz[0].
> So finally Caesar Wang contributed a patch, commit 0a18f9b268 ("
> dmaengine: pl330: fix to support the burst mode") to fix it, but what
> it actually did is to use single burst for any case, namely some kind
> of regression for Boojin Kim's improvement.
> 
> So we can see these drivers which was broken by enabling burst mode
> had already set src_maxburst/dst_maxburst. It looks to me so unfortunate
> that the driver like 8250_dw.c was using so widely that we couldn't
> set scr/dst_maxburst as this is really vendor specific for whether it
> supports burst for 8250_dw or not..
> 
> So it is quite painful that we probably will get dozens of regression
> reports when enabling burst mode by default. But without this, we have
> been suffering from low performance quite a long time due to this
> roadblock.

well in that case I would suggest fixing client first. Make all users of
pl330 not to use burst mode and then enable them one by one after testing

> 
> Two possible paths to land this patch are:
> (1) Keep this property for pl330 only, so we have no chance to
> break others and we could make the platforms enjoy it if adding this
> property for their own dts.
> 
> (2) Figuer out all the broken platfroms if enabling burst and fix them
> one by one for the src/dst_maxburst(maybe by enabling burst mode and
> get regression reports). If we could not solve any one of them, then we
> have to give up all the effort we do, and let this pain keep on
> stalling people's expectation of better performance.
> 
> 
> I would appreciate it if you could share your thought more, as I
> really want more platforms benefit from it(at least don't break
> them)  :)
> 
> 
> [0] http://www.gossamer-threads.com/lists/linux/kernel/2374171
> 
> >
> 
> 
> -- 
> Best Regards
> Shawn Lin
> 

-- 
~Vinod

Re: [PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Vinod Koul]

On Sun, Aug 21, 2016 at 09:00:58AM +0800, Shawn Lin wrote:
> Hi Vinod,
> 
> 在 2016/8/19 10:45, Vinod Koul 写道:
> >On Wed, Aug 17, 2016 at 04:11:03PM +0800, Shawn Lin wrote:
> >>Hi, Vinod and Lars-Peter
> >>
> >>Ping.. Any better idea to share :)
> >>
> >>On 2016/8/9 17:12, Shawn Lin wrote:
> >>>Hi Lars-Peter,
> >>>
> >>>在 2016/8/9 16:39, Lars-Peter Clausen 写道:
> >>>>On 08/05/2016 09:25 AM, Shawn Lin wrote:
> >>>>>Hi Vinod,
> >>>>>
> >>>>>在 2016/8/5 11:34, Vinod Koul 写道:
> >>>>>>On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
> >>>>>>>This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
> >>>>>>>support busrt mode.
> >>>>>>
> >>>>>>why should this be DT property. Only reason I can think of if some hw
> >>>>>>versions support this and some won't.
> >>>>>
> >>>>>yes, if we want to support burst mode, both of the master(pl330) and
> >>>>>client(several peripherals) should implement it, otherwise it will
> >>>>>be broken when enabling.
> >>>>
> >>>>As you said, it is up to the consumer peripheral whether it supports
> >>>>BURST,
> >>>>SINGLE or both. So this is a per client property, but you specify this
> >>>>as a
> >>>>a global property on the producer side.
> >>>
> >>>Thanks for comment.
> >>>
> >>>yup, but what is the proper way to add it ? :)
> >>>
> >>>
> >>>a) If pl330 support BURST as well as all the peripherals, we could
> >>>enable it.
> >>>
> >>>b) If pl300 support BURST, but all the peripherals don't support it,
> >>>we could not enable it.
> >>>
> >>>c) If pl300 support BURST, but not all the peripherals support it,
> >>>we also could not enable it.
> >>>
> >>>the burst feature of peripheral IP may be vendor-specific, but the
> >>>common driver for this peripheral are used for many many vendors which
> >>>means we could not check all of this info. It's very likely to break
> >>>them... I couldn't figure out how many upstreamed peripheral drivers
> >>>who are using pl300 either.
> >>>
> >>>So this check should be done by all this vendors but we could make
> >>>sure we don't break them before they check a), b), c), right?
> >
> >Since support for BURST needs to be from peripheral too, we should have
> >that as a property for peripheral not for controller.
> >
> >The peripheral drivers can communicate the burst to be used to pl330
> >using src_maxburst/dst_maxburst in dma_slave_config. We can use this
> >value to indicate the DMA should be single (a value of 0) or burst with
> >"burst" value.
> 
> Thanks for sharing this.
> 
> But this is really a difficult trade-off decision to add this new
> property for pl330 only.
> 
> Burst mode was supported by Boojin Kim's patch by default(commit
> 848e9776fee42 ("dmaengine: pl330: support burst mode for dev-to-mem and
> mem-to-dev transmit")).  But we found it will break SoCFPGA or
> Exynos4412 reported by Dinh Nguyen and Bartlomiej Zolnierkiewicz[0].
> So finally Caesar Wang contributed a patch, commit 0a18f9b268 ("
> dmaengine: pl330: fix to support the burst mode") to fix it, but what
> it actually did is to use single burst for any case, namely some kind
> of regression for Boojin Kim's improvement.
> 
> So we can see these drivers which was broken by enabling burst mode
> had already set src_maxburst/dst_maxburst. It looks to me so unfortunate
> that the driver like 8250_dw.c was using so widely that we couldn't
> set scr/dst_maxburst as this is really vendor specific for whether it
> supports burst for 8250_dw or not..
> 
> So it is quite painful that we probably will get dozens of regression
> reports when enabling burst mode by default. But without this, we have
> been suffering from low performance quite a long time due to this
> roadblock.

well in that case I would suggest fixing client first. Make all users of
pl330 not to use burst mode and then enable them one by one after testing

> 
> Two possible paths to land this patch are:
> (1) Keep this property for pl330 only, so we have no chance to
> break others and we could make the platforms enjoy it if adding this
> property for their own dts.
> 
> (2) Figuer out all the broken platfroms if enabling burst and fix them
> one by one for the src/dst_maxburst(maybe by enabling burst mode and
> get regression reports). If we could not solve any one of them, then we
> have to give up all the effort we do, and let this pain keep on
> stalling people's expectation of better performance.
> 
> 
> I would appreciate it if you could share your thought more, as I
> really want more platforms benefit from it(at least don't break
> them)  :)
> 
> 
> [0] http://www.gossamer-threads.com/lists/linux/kernel/2374171
> 
> >
> 
> 
> -- 
> Best Regards
> Shawn Lin
> 

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] dt/bindings: arm-pl330: add description of arm,pl330-periph-burst

[Vinod Koul]

On Sun, Aug 21, 2016 at 09:00:58AM +0800, Shawn Lin wrote:
> Hi Vinod,
> 
> ? 2016/8/19 10:45, Vinod Koul ??:
> >On Wed, Aug 17, 2016 at 04:11:03PM +0800, Shawn Lin wrote:
> >>Hi, Vinod and Lars-Peter
> >>
> >>Ping.. Any better idea to share :)
> >>
> >>On 2016/8/9 17:12, Shawn Lin wrote:
> >>>Hi Lars-Peter,
> >>>
> >>>? 2016/8/9 16:39, Lars-Peter Clausen ??:
> >>>>On 08/05/2016 09:25 AM, Shawn Lin wrote:
> >>>>>Hi Vinod,
> >>>>>
> >>>>>? 2016/8/5 11:34, Vinod Koul ??:
> >>>>>>On Fri, Aug 05, 2016 at 10:53:20AM +0800, Shawn Lin wrote:
> >>>>>>>This patch adds the "arm,pl330-periph-burst" for arm-pl330 to
> >>>>>>>support busrt mode.
> >>>>>>
> >>>>>>why should this be DT property. Only reason I can think of if some hw
> >>>>>>versions support this and some won't.
> >>>>>
> >>>>>yes, if we want to support burst mode, both of the master(pl330) and
> >>>>>client(several peripherals) should implement it, otherwise it will
> >>>>>be broken when enabling.
> >>>>
> >>>>As you said, it is up to the consumer peripheral whether it supports
> >>>>BURST,
> >>>>SINGLE or both. So this is a per client property, but you specify this
> >>>>as a
> >>>>a global property on the producer side.
> >>>
> >>>Thanks for comment.
> >>>
> >>>yup, but what is the proper way to add it ? :)
> >>>
> >>>
> >>>a) If pl330 support BURST as well as all the peripherals, we could
> >>>enable it.
> >>>
> >>>b) If pl300 support BURST, but all the peripherals don't support it,
> >>>we could not enable it.
> >>>
> >>>c) If pl300 support BURST, but not all the peripherals support it,
> >>>we also could not enable it.
> >>>
> >>>the burst feature of peripheral IP may be vendor-specific, but the
> >>>common driver for this peripheral are used for many many vendors which
> >>>means we could not check all of this info. It's very likely to break
> >>>them... I couldn't figure out how many upstreamed peripheral drivers
> >>>who are using pl300 either.
> >>>
> >>>So this check should be done by all this vendors but we could make
> >>>sure we don't break them before they check a), b), c), right?
> >
> >Since support for BURST needs to be from peripheral too, we should have
> >that as a property for peripheral not for controller.
> >
> >The peripheral drivers can communicate the burst to be used to pl330
> >using src_maxburst/dst_maxburst in dma_slave_config. We can use this
> >value to indicate the DMA should be single (a value of 0) or burst with
> >"burst" value.
> 
> Thanks for sharing this.
> 
> But this is really a difficult trade-off decision to add this new
> property for pl330 only.
> 
> Burst mode was supported by Boojin Kim's patch by default(commit
> 848e9776fee42 ("dmaengine: pl330: support burst mode for dev-to-mem and
> mem-to-dev transmit")).  But we found it will break SoCFPGA or
> Exynos4412 reported by Dinh Nguyen and Bartlomiej Zolnierkiewicz[0].
> So finally Caesar Wang contributed a patch, commit 0a18f9b268 ("
> dmaengine: pl330: fix to support the burst mode") to fix it, but what
> it actually did is to use single burst for any case, namely some kind
> of regression for Boojin Kim's improvement.
> 
> So we can see these drivers which was broken by enabling burst mode
> had already set src_maxburst/dst_maxburst. It looks to me so unfortunate
> that the driver like 8250_dw.c was using so widely that we couldn't
> set scr/dst_maxburst as this is really vendor specific for whether it
> supports burst for 8250_dw or not..
> 
> So it is quite painful that we probably will get dozens of regression
> reports when enabling burst mode by default. But without this, we have
> been suffering from low performance quite a long time due to this
> roadblock.

well in that case I would suggest fixing client first. Make all users of
pl330 not to use burst mode and then enable them one by one after testing

> 
> Two possible paths to land this patch are:
> (1) Keep this property for pl330 only, so we have no chance to
> break others and we could make the platforms enjoy it if adding this
> property for their own dts.
> 
> (2) Figuer out all the broken platfroms if enabling burst and fix them
> one by one for the src/dst_maxburst(maybe by enabling burst mode and
> get regression reports). If we could not solve any one of them, then we
> have to give up all the effort we do, and let this pain keep on
> stalling people's expectation of better performance.
> 
> 
> I would appreciate it if you could share your thought more, as I
> really want more platforms benefit from it(at least don't break
> them)  :)
> 
> 
> [0] http://www.gossamer-threads.com/lists/linux/kernel/2374171
> 
> >
> 
> 
> -- 
> Best Regards
> Shawn Lin
> 

-- 
~Vinod