[PATCH v5 0/10] Fix broken DMAFLUSHP on Rockchips platform

[PATCH v5 0/10] Fix broken DMAFLUSHP on Rockchips platform

[Shawn Lin]

The purpose of the DMAFLUSHP instruction:
- Tell the peripheral to clear its status and control registers.
- Send a message to the peripheral to resend its level status.

There are 3 timings described in PL330 Technical Reference Manual:
- Timing 1: Burst request, can work well without DMAFLUSHP.
- Timing 2: Single and burst request, DMAC will ignore the single
	    transfer request. This timing happens if there are single
	    and burst request.
- Timing 3: Single transfers for a burst request, DMAC should signals
	    datype to request the peripheral to flush the contents of
	    any control registers. This timing happens if there is
	    not enough MFIFO to places the burst data.

A peripheral may signal a DMA request during the execution of
DMAFLUSHP instruction, that cause DMA request being ignored by DMAC.

But DMAC and all peripherals on RK3X SoCs DO NOT support DMAFLUSHP.
It can't send a message to the peripheral to resend DMA request,
and the peripheral can't acknowledge a flush request from DMAC.
So all DMA requests should NOT be ignored by DMAC, and DMAC will not
notify the peripheral to flush.

To fix this problem, we need:
- Do NOT execute DMAFLUSHP instruction.
- Timing 2 and timing 3 should not happen.

Because on RK3X SoCs, there are 6 or below  channels and 32 MFIFO depth
for DMAC_BUS, and 8 channels and 64 MFIFO depth for DMAC_PERI, it is
impossible to hit the timing 3 if burst length is equal or less than 4.

Since the request type signal by the peripheral can only be set by
software. We can set Rockchip Soc's GRF_PERIDMAC_CON0[2:1] to select single
or burst request, if it is set b01,  all of the peripharals will signal a brust
request. So the timing 2 will not happen, too.

So DMAC on RK3X can support single or burst transfer, but can't support
mixed transfer.

Because burst transfer is more efficient than single transfer, this is
confirmed by our ASIC team, who strongly suggest to use burst transfer.
And this is confirmed by Addy's test on RK3288-Pink2 board, the speed of
spi flash burst transfer will increase about two times than single transfer.
Also, I have tested dw_mmc with pl330 on RK3188 platform to double confirm
the result. That means burst transfer is reansonable.

So we need a quirk not to execute DMAFLUSHP instruction and to use burst
transfer.

Note:
- The Rockchip Soc default value of GRF_PERIDMAC_CON0[2:1] is b01. To
  support brust transfer, these bits should not be changed in bootloader.


Changes in v5:
- add Mark's tag for spi changes
- remove unnecessary whitespace change
- use switch statement for I2S dma_quirk's manipulation

Changes in v4:
- remove spi & i2s dts changes and query quirk from dmaengine API
  suggeseted by Mark.
- fix typo
- Add dmaengine_get_quirk hook and implement it for pl330

Changes in v3:
- add Sunny's tag
- add more rockchip drivers' changes in this patchset
- add Reviewed-by: Sonny Rao <sonnyrao@chromium.org>

Changes in v2:
- amend the author
- reorder the patches suggested by Doug
- add Reviewed-by: Doug Anderson <dianders@chromium.org> for
  rk3288.dtsi patch and arm-pl330.txt patch
- amend Olof's mail address

Changes in v1:
- rename broken-no-flushp to "arm,pl330-broken-no-flushp" suggested
  by Krzysztof.
- add From original author.
- remove Sunny's tag

Addy Ke (3):
  DMA: pl330: add quirk for broken no flushp
  ARM: dts: Add arm,pl330-broken-no-flushp quirk for rk3288 platform
  spi: rockchip: modify DMA max burst to 1

Boojin Kim (1):
  DMA: pl330: support burst mode for dev-to-mem and mem-to-dev transmit

Shawn Lin (5):
  Documentation: arm-pl330: add description of
    arm,pl330-broken-no-flushp
  ARM: dts: Add arm,pl330-broken-no-flushp quirk for rk3xxx platform
  dmaengine: add API for getting dma controller's quirk
  DMA: pl330: implement dmaengine_get_quirks hook
  snd: dmaengine-pcm: add snd_dmaengine_pcm_get_quirks interface

Yiwei Cai (1):
  ASoC: rockchip_i2s: modify DMA max burst to 1

 .../devicetree/bindings/dma/arm-pl330.txt          |   1 +
 arch/arm/boot/dts/rk3288.dtsi                      |   3 +
 arch/arm/boot/dts/rk3xxx.dtsi                      |   3 +
 drivers/dma/pl330.c                                | 110 +++++++++++++++------
 drivers/spi/spi-rockchip.c                         |  13 ++-
 include/linux/dmaengine.h                          |   9 ++
 sound/soc/rockchip/rockchip_i2s.c                  |  21 ++++
 sound/soc/soc-generic-dmaengine-pcm.c              |  24 +++++
 8 files changed, 153 insertions(+), 31 deletions(-)

-- 
2.3.7

[PATCH v5 01/10] DMA: pl330: support burst mode for dev-to-mem and mem-to-dev transmit

[Shawn Lin]

From: Boojin Kim <boojin.kim@samsung.com>

This patch adds to support burst mode for dev-to-mem and
mem-to-dev transmit.

Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
cc: Heiko Stuebner <heiko@sntech.de>
cc: Doug Anderson <dianders@chromium.org>
cc: Olof Johansson <olof@lixom.net>

Reviewed-by: Sonny Rao <sonnyrao@chromium.org>
---

Changes in v5:
- add Mark's tag for spi changes
- remove unnecessary whitespace change
- use switch statement for i2s quirk

Changes in v4:
- remove spi & i2s dts changes and query quirk from dmaengine API
  suggeseted by Mark.
- fix typo
- Add dmaengine_get_quirk hook and implement it for pl330

Changes in v3:
- add Sunny's tag
- add more rockchip drivers' changes in this patchset

Changes in v2:
- amend the author
- reorder the patches suggested by Doug
- add Reviewed-by: Doug Anderson <dianders@chromium.org> for
  rk3288.dtsi patch and arm-pl330.txt patch

Changes in v1:
- rename broken-no-flushp to "arm,pl330-broken-no-flushp" suggested
  by Krzysztof.
- add From original author.
- remove Sunny's tag

 drivers/dma/pl330.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index ecab4ea0..0d544d2 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1141,10 +1141,13 @@ static inline int _ldst_devtomem(unsigned dry_run, u8 buf[],
 		const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
+	enum pl330_cond cond;
+
+	cond = (pxs->desc->rqcfg.brst_len == 1) ? SINGLE : BURST;
 
 	while (cyc--) {
-		off += _emit_WFP(dry_run, &buf[off], SINGLE, pxs->desc->peri);
-		off += _emit_LDP(dry_run, &buf[off], SINGLE, pxs->desc->peri);
+		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
+		off += _emit_LDP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_ST(dry_run, &buf[off], ALWAYS);
 		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	}
@@ -1156,11 +1159,14 @@ static inline int _ldst_memtodev(unsigned dry_run, u8 buf[],
 		const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
+	enum pl330_cond cond;
+
+	cond = (pxs->desc->rqcfg.brst_len == 1) ? SINGLE : BURST;
 
 	while (cyc--) {
-		off += _emit_WFP(dry_run, &buf[off], SINGLE, pxs->desc->peri);
+		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_LD(dry_run, &buf[off], ALWAYS);
-		off += _emit_STP(dry_run, &buf[off], SINGLE, pxs->desc->peri);
+		off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
 	}
 
@@ -2557,7 +2563,7 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 
 		desc->rqtype = direction;
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->bytes_requested = period_len;
 		fill_px(&desc->px, dst, src, period_len);
 
@@ -2702,7 +2708,7 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		}
 
 		desc->rqcfg.brst_size = pch->burst_sz;
-		desc->rqcfg.brst_len = 1;
+		desc->rqcfg.brst_len = pch->burst_len;
 		desc->rqtype = direction;
 		desc->bytes_requested = sg_dma_len(sg);
 	}
-- 
2.3.7

[PATCH v5 02/10] Documentation: arm-pl330: add description of arm,pl330-broken-no-flushp

[Shawn Lin]

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

Reviewed-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Sonny Rao <sonnyrao@chromium.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
- add Reviewed-by: Sonny Rao <sonnyrao@chromium.org>

Changes in v2:
- add Reviewed-by: Doug Anderson <dianders@chromium.org>

Changes in v1:
- rename broken-no-flushp to "arm,pl330-broken-no-flushp" suggested
  by Krzysztof.

 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 2675658..db7e226 100644
--- a/Documentation/devicetree/bindings/dma/arm-pl330.txt
+++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
@@ -15,6 +15,7 @@ Optional properties:
     cells in the dmas property of client device.
   - 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
 
 Example:
 
-- 
2.3.7

[PATCH v5 03/10] DMA: pl330: add quirk for broken no flushp

[Shawn Lin]

From: Addy Ke <addy.ke@rock-chips.com>

This patch add "arm,pl330-broken-no-flushp" quirk to avoid execute
DMAFLUSHP if Soc doesn't support it.

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
cc: Doug Anderson <dianders@chromium.org>
cc: Heiko Stuebner <heiko@sntech.de>
cc: Olof Johansson <olof@lixom.net>

Reviewed-by: Sonny Rao <sonnyrao@chromium.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
- add Reviewed-by: Sonny Rao <sonnyrao@chromium.org>

Changes in v2:
- amend the author
- fix Olof's mail address

Changes in v1:
- rename broken-no-flushp to "arm,pl330-broken-no-flushp" suggested
  by Krzysztof.
- remove Sunny's tag

 drivers/dma/pl330.c | 87 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 25 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 0d544d2..3b9b426 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -34,6 +34,8 @@
 #define PL330_MAX_IRQS		32
 #define PL330_MAX_PERI		32
 
+#define PL330_QUIRK_BROKEN_NO_FLUSHP BIT(0)
+
 enum pl330_cachectrl {
 	CCTRL0,		/* Noncacheable and nonbufferable */
 	CCTRL1,		/* Bufferable only */
@@ -488,6 +490,17 @@ struct pl330_dmac {
 	/* Peripheral channels connected to this DMAC */
 	unsigned int num_peripherals;
 	struct dma_pl330_chan *peripherals; /* keep at end */
+	int quirks;
+};
+
+static struct pl330_of_quirks {
+	char *quirk;
+	int id;
+} of_quirks[] = {
+	{
+		.quirk = "arm,pl330-broken-no-flushp",
+		.id = PL330_QUIRK_BROKEN_NO_FLUSHP,
+	}
 };
 
 struct dma_pl330_desc {
@@ -1137,53 +1150,68 @@ static inline int _ldst_memtomem(unsigned dry_run, u8 buf[],
 	return off;
 }
 
-static inline int _ldst_devtomem(unsigned dry_run, u8 buf[],
-		const struct _xfer_spec *pxs, int cyc)
+static inline int _ldst_devtomem(struct pl330_dmac *pl330, unsigned dry_run,
+				 u8 buf[], const struct _xfer_spec *pxs,
+				 int cyc)
 {
 	int off = 0;
 	enum pl330_cond cond;
 
-	cond = (pxs->desc->rqcfg.brst_len == 1) ? SINGLE : BURST;
+	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+		cond = BURST;
+	else
+		cond = (pxs->desc->rqcfg.brst_len == 1) ? SINGLE : BURST;
 
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_LDP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_ST(dry_run, &buf[off], ALWAYS);
-		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
+
+		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+			off += _emit_FLUSHP(dry_run, &buf[off],
+					    pxs->desc->peri);
 	}
 
 	return off;
 }
 
-static inline int _ldst_memtodev(unsigned dry_run, u8 buf[],
-		const struct _xfer_spec *pxs, int cyc)
+static inline int _ldst_memtodev(struct pl330_dmac *pl330,
+				 unsigned dry_run, u8 buf[],
+				 const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
 	enum pl330_cond cond;
 
-	cond = (pxs->desc->rqcfg.brst_len == 1) ? SINGLE : BURST;
+	if (pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP)
+		cond = BURST;
+	else
+		cond = (pxs->desc->rqcfg.brst_len == 1) ? SINGLE : BURST;
+
 
 	while (cyc--) {
 		off += _emit_WFP(dry_run, &buf[off], cond, pxs->desc->peri);
 		off += _emit_LD(dry_run, &buf[off], ALWAYS);
 		off += _emit_STP(dry_run, &buf[off], cond, pxs->desc->peri);
-		off += _emit_FLUSHP(dry_run, &buf[off], pxs->desc->peri);
+
+		if (!(pl330->quirks & PL330_QUIRK_BROKEN_NO_FLUSHP))
+			off += _emit_FLUSHP(dry_run, &buf[off],
+					    pxs->desc->peri);
 	}
 
 	return off;
 }
 
-static int _bursts(unsigned dry_run, u8 buf[],
+static int _bursts(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		const struct _xfer_spec *pxs, int cyc)
 {
 	int off = 0;
 
 	switch (pxs->desc->rqtype) {
 	case DMA_MEM_TO_DEV:
-		off += _ldst_memtodev(dry_run, &buf[off], pxs, cyc);
+		off += _ldst_memtodev(pl330, dry_run, &buf[off], pxs, cyc);
 		break;
 	case DMA_DEV_TO_MEM:
-		off += _ldst_devtomem(dry_run, &buf[off], pxs, cyc);
+		off += _ldst_devtomem(pl330, dry_run, &buf[off], pxs, cyc);
 		break;
 	case DMA_MEM_TO_MEM:
 		off += _ldst_memtomem(dry_run, &buf[off], pxs, cyc);
@@ -1197,7 +1225,7 @@ static int _bursts(unsigned dry_run, u8 buf[],
 }
 
 /* Returns bytes consumed and updates bursts */
-static inline int _loop(unsigned dry_run, u8 buf[],
+static inline int _loop(struct pl330_dmac *pl330, unsigned dry_run, u8 buf[],
 		unsigned long *bursts, const struct _xfer_spec *pxs)
 {
 	int cyc, cycmax, szlp, szlpend, szbrst, off;
@@ -1220,7 +1248,7 @@ static inline int _loop(unsigned dry_run, u8 buf[],
 	}
 
 	szlp = _emit_LP(1, buf, 0, 0);
-	szbrst = _bursts(1, buf, pxs, 1);
+	szbrst = _bursts(pl330, 1, buf, pxs, 1);
 
 	lpend.cond = ALWAYS;
 	lpend.forever = false;
@@ -1252,7 +1280,7 @@ static inline int _loop(unsigned dry_run, u8 buf[],
 	off += _emit_LP(dry_run, &buf[off], 1, lcnt1);
 	ljmp1 = off;
 
-	off += _bursts(dry_run, &buf[off], pxs, cyc);
+	off += _bursts(pl330, dry_run, &buf[off], pxs, cyc);
 
 	lpend.cond = ALWAYS;
 	lpend.forever = false;
@@ -1275,8 +1303,9 @@ static inline int _loop(unsigned dry_run, u8 buf[],
 	return off;
 }
 
-static inline int _setup_loops(unsigned dry_run, u8 buf[],
-		const struct _xfer_spec *pxs)
+static inline int _setup_loops(struct pl330_dmac *pl330,
+			       unsigned dry_run, u8 buf[],
+			       const struct _xfer_spec *pxs)
 {
 	struct pl330_xfer *x = &pxs->desc->px;
 	u32 ccr = pxs->ccr;
@@ -1285,15 +1314,16 @@ static inline int _setup_loops(unsigned dry_run, u8 buf[],
 
 	while (bursts) {
 		c = bursts;
-		off += _loop(dry_run, &buf[off], &c, pxs);
+		off += _loop(pl330, dry_run, &buf[off], &c, pxs);
 		bursts -= c;
 	}
 
 	return off;
 }
 
-static inline int _setup_xfer(unsigned dry_run, u8 buf[],
-		const struct _xfer_spec *pxs)
+static inline int _setup_xfer(struct pl330_dmac *pl330,
+			      unsigned dry_run, u8 buf[],
+			      const struct _xfer_spec *pxs)
 {
 	struct pl330_xfer *x = &pxs->desc->px;
 	int off = 0;
@@ -1304,7 +1334,7 @@ static inline int _setup_xfer(unsigned dry_run, u8 buf[],
 	off += _emit_MOV(dry_run, &buf[off], DAR, x->dst_addr);
 
 	/* Setup Loop(s) */
-	off += _setup_loops(dry_run, &buf[off], pxs);
+	off += _setup_loops(pl330, dry_run, &buf[off], pxs);
 
 	return off;
 }
@@ -1313,8 +1343,9 @@ static inline int _setup_xfer(unsigned dry_run, u8 buf[],
  * A req is a sequence of one or more xfer units.
  * Returns the number of bytes taken to setup the MC for the req.
  */
-static int _setup_req(unsigned dry_run, struct pl330_thread *thrd,
-		unsigned index, struct _xfer_spec *pxs)
+static int _setup_req(struct pl330_dmac *pl330, unsigned dry_run,
+		      struct pl330_thread *thrd, unsigned index,
+		      struct _xfer_spec *pxs)
 {
 	struct _pl330_req *req = &thrd->req[index];
 	struct pl330_xfer *x;
@@ -1331,7 +1362,7 @@ static int _setup_req(unsigned dry_run, struct pl330_thread *thrd,
 	if (x->bytes % (BRST_SIZE(pxs->ccr) * BRST_LEN(pxs->ccr)))
 		return -EINVAL;
 
-	off += _setup_xfer(dry_run, &buf[off], pxs);
+	off += _setup_xfer(pl330, dry_run, &buf[off], pxs);
 
 	/* DMASEV peripheral/event */
 	off += _emit_SEV(dry_run, &buf[off], thrd->ev);
@@ -1425,7 +1456,7 @@ static int pl330_submit_req(struct pl330_thread *thrd,
 	xs.desc = desc;
 
 	/* First dry run to check if req is acceptable */
-	ret = _setup_req(1, thrd, idx, &xs);
+	ret = _setup_req(pl330, 1, thrd, idx, &xs);
 	if (ret < 0)
 		goto xfer_exit;
 
@@ -1439,7 +1470,7 @@ static int pl330_submit_req(struct pl330_thread *thrd,
 	/* Hook the request */
 	thrd->lstenq = idx;
 	thrd->req[idx].desc = desc;
-	_setup_req(0, thrd, idx, &xs);
+	_setup_req(pl330, 0, thrd, idx, &xs);
 
 	ret = 0;
 
@@ -2784,6 +2815,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	struct resource *res;
 	int i, ret, irq;
 	int num_chan;
+	struct device_node *np = adev->dev.of_node;
 
 	pdat = dev_get_platdata(&adev->dev);
 
@@ -2803,6 +2835,11 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	pl330->mcbufsz = pdat ? pdat->mcbuf_sz : 0;
 
+	/* get quirk */
+	for (i = 0; i < ARRAY_SIZE(of_quirks); i++)
+		if (of_property_read_bool(np, of_quirks[i].quirk))
+			pl330->quirks |= of_quirks[i].id;
+
 	res = &adev->res;
 	pl330->base = devm_ioremap_resource(&adev->dev, res);
 	if (IS_ERR(pl330->base))
-- 
2.3.7

[PATCH v5 04/10] ARM: dts: Add arm,pl330-broken-no-flushp quirk for rk3288 platform

[Shawn Lin]

From: Addy Ke <addy.ke@rock-chips.com>

Pl330 integrated in rk3288 platform doesn't support
DMAFLUSHP function. So we add arm,pl330-broken-no-flushp quirk
for it.

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
cc: Heiko Stuebner <heiko@sntech.de>
cc: Olof Johansson <olof@lixom.net>
cc: Sonny Rao <sonnyrao@chromium.org>

Reviewed-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Sonny Rao <sonnyrao@chromium.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
- add Reviewed-by: Sonny Rao <sonnyrao@chromium.org>

Changes in v2:
- amend the author
- add Reviewed-by: Doug Anderson <dianders@chromium.org>
- amend Olof's mail address

Changes in v1:
- rename broken-no-flushp to "arm,pl330-broken-no-flushp" suggested
  by Krzysztof.
- remove Sunny's tag

 arch/arm/boot/dts/rk3288.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 22316d0..106adf7 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -144,6 +144,7 @@
 			#dma-cells = <1>;
 			clocks = <&cru ACLK_DMAC2>;
 			clock-names = "apb_pclk";
+			arm,pl330-broken-no-flushp;
 		};
 
 		dmac_bus_ns: dma-controller@ff600000 {
@@ -155,6 +156,7 @@
 			clocks = <&cru ACLK_DMAC1>;
 			clock-names = "apb_pclk";
 			status = "disabled";
+			arm,pl330-broken-no-flushp;
 		};
 
 		dmac_bus_s: dma-controller@ffb20000 {
@@ -165,6 +167,7 @@
 			#dma-cells = <1>;
 			clocks = <&cru ACLK_DMAC1>;
 			clock-names = "apb_pclk";
+			arm,pl330-broken-no-flushp;
 		};
 	};
 
-- 
2.3.7

[PATCH v5 05/10] ARM: dts: Add arm,pl330-broken-no-flushp quirk for rk3xxx platform

[Shawn Lin]

Pl330 integrated in rk3xxx platform doesn't support
DMAFLUSHP function. So we add arm,pl330-broken-no-flushp quirk
for it.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
cc: Heiko Stuebner <heiko@sntech.de>
cc: Doug Anderson <dianders@chromium.org>
cc: Olof Johansson <olof@lixom.net>

---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1:
- rename broken-no-flushp to "arm,pl330-broken-no-flushp" suggested
  by Krzysztof.

 arch/arm/boot/dts/rk3xxx.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/rk3xxx.dtsi b/arch/arm/boot/dts/rk3xxx.dtsi
index a2ae9f3..a8ca4b3 100644
--- a/arch/arm/boot/dts/rk3xxx.dtsi
+++ b/arch/arm/boot/dts/rk3xxx.dtsi
@@ -79,6 +79,7 @@
 			#dma-cells = <1>;
 			clocks = <&cru ACLK_DMA1>;
 			clock-names = "apb_pclk";
+			arm,pl330-broken-no-flushp;
 		};
 
 		dmac1_ns: dma-controller@2001c000 {
@@ -89,6 +90,7 @@
 			#dma-cells = <1>;
 			clocks = <&cru ACLK_DMA1>;
 			clock-names = "apb_pclk";
+			arm,pl330-broken-no-flushp;
 			status = "disabled";
 		};
 
@@ -100,6 +102,7 @@
 			#dma-cells = <1>;
 			clocks = <&cru ACLK_DMA2>;
 			clock-names = "apb_pclk";
+			arm,pl330-broken-no-flushp;
 		};
 	};
 
-- 
2.3.7

[PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Shawn Lin]

Add dmaengine_get_quirks API for peripheral devices to query
quirks if they need it to make special workaround due to broken
dma controller design.

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

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1: None

 include/linux/dmaengine.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index e2f5eb4..5174ca4 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -704,6 +704,7 @@ struct dma_device {
 
 	int (*device_config)(struct dma_chan *chan,
 			     struct dma_slave_config *config);
+	int (*device_get_quirks)(struct dma_chan *chan);
 	int (*device_pause)(struct dma_chan *chan);
 	int (*device_resume)(struct dma_chan *chan);
 	int (*device_terminate_all)(struct dma_chan *chan);
@@ -723,6 +724,14 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
 	return -ENOSYS;
 }
 
+static inline int dmaengine_get_quirks(struct dma_chan *chan)
+{
+	if (chan->device->device_get_quirks)
+		return chan->device->device_get_quirks(chan);
+
+	return -ENOSYS;
+}
+
 static inline bool is_slave_direction(enum dma_transfer_direction direction)
 {
 	return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
-- 
2.3.7

[PATCH v5 07/10] DMA: pl330: implement dmaengine_get_quirks hook

[Shawn Lin]

By adding this function, slave device can query quirks
from pl330 if they need special settings for dmaengine.

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

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1: None

 drivers/dma/pl330.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 3b9b426..bf01d24 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2156,6 +2156,14 @@ static int pl330_config(struct dma_chan *chan,
 	return 0;
 }
 
+static int pl330_quirks(struct dma_chan *chan)
+{
+	struct dma_pl330_chan *pch = to_pchan(chan);
+	struct pl330_dmac *pl330 = pch->dmac;
+
+	return pl330->quirks;
+}
+
 static int pl330_terminate_all(struct dma_chan *chan)
 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
@@ -2928,6 +2936,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	pd->device_tx_status = pl330_tx_status;
 	pd->device_prep_slave_sg = pl330_prep_slave_sg;
 	pd->device_config = pl330_config;
+	pd->device_get_quirks = pl330_quirks;
 	pd->device_pause = pl330_pause;
 	pd->device_terminate_all = pl330_terminate_all;
 	pd->device_issue_pending = pl330_issue_pending;
-- 
2.3.7

[PATCH v5 08/10] spi: rockchip: modify DMA max burst to 1

[Shawn Lin]

From: Addy Ke <addy.ke@rock-chips.com>

Generic dma controller on Rockchips' platform cannot support
DMAFLUSHP instruction which make dma to flush the req of non-aligned
or non-multiple of what we need. That will cause an unrecoverable
dma bus error. The saftest way is to set dma max burst to 1.

Signed-off-by: Addy ke <addy.ke@rock-chips.com>
Fixes: 64e36824b32b06 ("spi/rockchip: add driver for Rockchip...")
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
cc: Heiko Stuebner <heiko@sntech.de>
cc: Olof Johansson <olof@lixom.net>
cc: Doug Anderson <dianders@chromium.org>
cc: Sonny Rao <sonnyrao@chromium.org>
Acked-by: Mark Brown <broonie@kernel.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1: None

 drivers/spi/spi-rockchip.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 68e7efe..89dd3d8 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -199,6 +199,8 @@ struct rockchip_spi {
 	struct sg_table rx_sg;
 	struct rockchip_spi_dma_data dma_rx;
 	struct rockchip_spi_dma_data dma_tx;
+	int dma_quirk;
+#define ROCKCHIP_SPI_BROKEN_BURST_LEN (1<<0) /* broken burst len*/
 };
 
 static inline void spi_enable_chip(struct rockchip_spi *rs, int enable)
@@ -449,7 +451,10 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs)
 		rxconf.direction = rs->dma_rx.direction;
 		rxconf.src_addr = rs->dma_rx.addr;
 		rxconf.src_addr_width = rs->n_bytes;
-		rxconf.src_maxburst = rs->n_bytes;
+		if (rs->dma_quirk == ROCKCHIP_SPI_BROKEN_BURST_LEN)
+			rxconf.src_maxburst = 1;
+		else
+			rxconf.src_maxburst = 4;
 		dmaengine_slave_config(rs->dma_rx.ch, &rxconf);
 
 		rxdesc = dmaengine_prep_slave_sg(
@@ -466,7 +471,10 @@ static void rockchip_spi_prepare_dma(struct rockchip_spi *rs)
 		txconf.direction = rs->dma_tx.direction;
 		txconf.dst_addr = rs->dma_tx.addr;
 		txconf.dst_addr_width = rs->n_bytes;
-		txconf.dst_maxburst = rs->n_bytes;
+		if (rs->dma_quirk == ROCKCHIP_SPI_BROKEN_BURST_LEN)
+			txconf.dst_maxburst = 1;
+		else
+			txconf.dst_maxburst = 4;
 		dmaengine_slave_config(rs->dma_tx.ch, &txconf);
 
 		txdesc = dmaengine_prep_slave_sg(
@@ -731,6 +739,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	}
 
 	if (rs->dma_tx.ch && rs->dma_rx.ch) {
+		rs->dma_quirk = dmaengine_get_quirks(rs->dma_rx.ch);
 		rs->dma_tx.addr = (dma_addr_t)(mem->start + ROCKCHIP_SPI_TXDR);
 		rs->dma_rx.addr = (dma_addr_t)(mem->start + ROCKCHIP_SPI_RXDR);
 		rs->dma_tx.direction = DMA_MEM_TO_DEV;
-- 
2.3.7

[PATCH v5 09/10] snd: dmaengine-pcm: add snd_dmaengine_pcm_get_quirks interface

[Shawn Lin]

Add snd_dmaengine_pcm_get_quirks for I2S devices to query
dma controller's quirks if they need it to make special workaround
due to broken dma controller design

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

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1: None

 sound/soc/soc-generic-dmaengine-pcm.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index 6fd1906..42136005 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -466,4 +466,28 @@ void snd_dmaengine_pcm_unregister(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_unregister);
 
+
+/**
+ * snd_dmaengine_pcm_get_quirks - Get dmaengine quirks based PCM device
+ * @dev: Parent device the PCM was register with
+ */
+int snd_dmaengine_pcm_get_quirks(struct device *dev)
+{
+	struct snd_soc_platform *platform;
+	struct dmaengine_pcm *pcm;
+	int ret = -ENODEV;
+
+	platform = snd_soc_lookup_platform(dev);
+	if (!platform)
+		return ret;
+
+	pcm = soc_platform_to_pcm(platform);
+
+	if (pcm->chan)
+		ret = dmaengine_get_quirks(pcm->chan[0]);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_get_quirks);
+
 MODULE_LICENSE("GPL");
-- 
2.3.7

[PATCH v5 10/10] ASoC: rockchip_i2s: modify DMA max burst to 1

[Shawn Lin]

From: Yiwei Cai <cain.cai@rock-chips.com>

Test with command -
arecord -D hw:0,0 /tmp/a.wav, there are the error dump:
dma-pl330 ffb20000.dma-controller: fill_queue:2251 Bad Desc(7)

This error is happening when no a multiple of burst size * burst
length are coming in. The root cause is pl330 dma controller on
Rockchips' platform cannot support DMAFLUSHP instruction which make
dma to flush the req of non-aligned or non-multiple of what we set
before. The saftest way is to set dma max burst to 1.

Signed-off-by: Yiwei Cai <cain.cai@rock-chips.com>
Fixes: 4495c89fc ("ASoC: add driver for Rockchip RK3xxx I2S")
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
cc: Addy Ke <addy.ke@rock-chips.com>
cc: Jianqun Xu <xjq@rock-chips.com>
cc: Heiko Stuebner <heiko@sntech.de>
cc: Olof Johansson <olof@lixom.net>
cc: Doug Anderson <dianders@chromium.org>
cc: Sonny Rao <sonnyrao@chromium.org>
cc: Mark Brown <broonie@kernel.org>

---

Changes in v5:
- use switch statement for dma_quirk's manipulation

Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1: None

 sound/soc/rockchip/rockchip_i2s.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index acb5be5..543d0c0 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -23,6 +23,8 @@
 
 #define DRV_NAME "rockchip-i2s"
 
+#define ROCKCHIP_I2S_BROKEN_BURST_LEN (1<<0) /* broken burst len */
+
 struct rk_i2s_dev {
 	struct device *dev;
 
@@ -418,6 +420,7 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
 	struct rk_i2s_dev *i2s;
 	struct resource *res;
 	void __iomem *regs;
+	int dma_quirk;
 	int ret;
 
 	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
@@ -489,6 +492,24 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
 		goto err_pcm_register;
 	}
 
+	dma_quirk = snd_dmaengine_pcm_get_quirks(&pdev->dev);
+	switch (dma_quirk) {
+	case ROCKCHIP_I2S_BROKEN_BURST_LEN:
+		/*
+		 * Unfortunately, we find broken burst len here,
+		 * just have to limit maxburst to ONE in order to avoid
+		 * non-multiple burst len access fail the dmaengine if
+		 * it can't support flush peripheral function.
+		 */
+		i2s->playback_dma_data.maxburst = 1;
+		i2s->capture_dma_data.maxburst = 1;
+		break;
+	default:
+		dev_info(&pdev->dev, "Default no dma_quirk!\n");
+		break;
+	}
+
 	return 0;
 
 err_pcm_register:
-- 
2.3.7

Re: [PATCH v5 10/10] ASoC: rockchip_i2s: modify DMA max burst to 1

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

On Mon, Sep 14, 2015 at 07:49:42AM +0800, Shawn Lin wrote:

> +	default:
> +		dev_info(&pdev->dev, "Default no dma_quirk!\n");
> +		break;
> +	}

Probably printing a message when not using the quirk is a bit noisy
(hopefully future devices will fix this!).  Otherwise

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 0/10] Fix broken DMAFLUSHP on Rockchips platform

[Shawn Lin]

Hi Vinod,

	Is there any chance for this patchset to be applied? :)

On 2015/9/14 7:45, Shawn Lin wrote:
> The purpose of the DMAFLUSHP instruction:
> - Tell the peripheral to clear its status and control registers.
> - Send a message to the peripheral to resend its level status.
>
> There are 3 timings described in PL330 Technical Reference Manual:
> - Timing 1: Burst request, can work well without DMAFLUSHP.
> - Timing 2: Single and burst request, DMAC will ignore the single
> 	    transfer request. This timing happens if there are single
> 	    and burst request.
> - Timing 3: Single transfers for a burst request, DMAC should signals
> 	    datype to request the peripheral to flush the contents of
> 	    any control registers. This timing happens if there is
> 	    not enough MFIFO to places the burst data.
>
> A peripheral may signal a DMA request during the execution of
> DMAFLUSHP instruction, that cause DMA request being ignored by DMAC.
>
> But DMAC and all peripherals on RK3X SoCs DO NOT support DMAFLUSHP.
> It can't send a message to the peripheral to resend DMA request,
> and the peripheral can't acknowledge a flush request from DMAC.
> So all DMA requests should NOT be ignored by DMAC, and DMAC will not
> notify the peripheral to flush.
>
> To fix this problem, we need:
> - Do NOT execute DMAFLUSHP instruction.
> - Timing 2 and timing 3 should not happen.
>
> Because on RK3X SoCs, there are 6 or below  channels and 32 MFIFO depth
> for DMAC_BUS, and 8 channels and 64 MFIFO depth for DMAC_PERI, it is
> impossible to hit the timing 3 if burst length is equal or less than 4.
>
> Since the request type signal by the peripheral can only be set by
> software. We can set Rockchip Soc's GRF_PERIDMAC_CON0[2:1] to select single
> or burst request, if it is set b01,  all of the peripharals will signal a brust
> request. So the timing 2 will not happen, too.
>
> So DMAC on RK3X can support single or burst transfer, but can't support
> mixed transfer.
>
> Because burst transfer is more efficient than single transfer, this is
> confirmed by our ASIC team, who strongly suggest to use burst transfer.
> And this is confirmed by Addy's test on RK3288-Pink2 board, the speed of
> spi flash burst transfer will increase about two times than single transfer.
> Also, I have tested dw_mmc with pl330 on RK3188 platform to double confirm
> the result. That means burst transfer is reansonable.
>
> So we need a quirk not to execute DMAFLUSHP instruction and to use burst
> transfer.
>
> Note:
> - The Rockchip Soc default value of GRF_PERIDMAC_CON0[2:1] is b01. To
>    support brust transfer, these bits should not be changed in bootloader.
>
>
> Changes in v5:
> - add Mark's tag for spi changes
> - remove unnecessary whitespace change
> - use switch statement for I2S dma_quirk's manipulation
>
> Changes in v4:
> - remove spi & i2s dts changes and query quirk from dmaengine API
>    suggeseted by Mark.
> - fix typo
> - Add dmaengine_get_quirk hook and implement it for pl330
>
> Changes in v3:
> - add Sunny's tag
> - add more rockchip drivers' changes in this patchset
> - add Reviewed-by: Sonny Rao <sonnyrao@chromium.org>
>
> Changes in v2:
> - amend the author
> - reorder the patches suggested by Doug
> - add Reviewed-by: Doug Anderson <dianders@chromium.org> for
>    rk3288.dtsi patch and arm-pl330.txt patch
> - amend Olof's mail address
>
> Changes in v1:
> - rename broken-no-flushp to "arm,pl330-broken-no-flushp" suggested
>    by Krzysztof.
> - add From original author.
> - remove Sunny's tag
>
> Addy Ke (3):
>    DMA: pl330: add quirk for broken no flushp
>    ARM: dts: Add arm,pl330-broken-no-flushp quirk for rk3288 platform
>    spi: rockchip: modify DMA max burst to 1
>
> Boojin Kim (1):
>    DMA: pl330: support burst mode for dev-to-mem and mem-to-dev transmit
>
> Shawn Lin (5):
>    Documentation: arm-pl330: add description of
>      arm,pl330-broken-no-flushp
>    ARM: dts: Add arm,pl330-broken-no-flushp quirk for rk3xxx platform
>    dmaengine: add API for getting dma controller's quirk
>    DMA: pl330: implement dmaengine_get_quirks hook
>    snd: dmaengine-pcm: add snd_dmaengine_pcm_get_quirks interface
>
> Yiwei Cai (1):
>    ASoC: rockchip_i2s: modify DMA max burst to 1
>
>   .../devicetree/bindings/dma/arm-pl330.txt          |   1 +
>   arch/arm/boot/dts/rk3288.dtsi                      |   3 +
>   arch/arm/boot/dts/rk3xxx.dtsi                      |   3 +
>   drivers/dma/pl330.c                                | 110 +++++++++++++++------
>   drivers/spi/spi-rockchip.c                         |  13 ++-
>   include/linux/dmaengine.h                          |   9 ++
>   sound/soc/rockchip/rockchip_i2s.c                  |  21 ++++
>   sound/soc/soc-generic-dmaengine-pcm.c              |  24 +++++
>   8 files changed, 153 insertions(+), 31 deletions(-)
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Vinod Koul]

On Mon, Sep 14, 2015 at 07:48:59AM +0800, Shawn Lin wrote:
> Add dmaengine_get_quirks API for peripheral devices to query
> quirks if they need it to make special workaround due to broken
> dma controller design.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> Changes in v1: None
> 
>  include/linux/dmaengine.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index e2f5eb4..5174ca4 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -704,6 +704,7 @@ struct dma_device {
>  
>  	int (*device_config)(struct dma_chan *chan,
>  			     struct dma_slave_config *config);
> +	int (*device_get_quirks)(struct dma_chan *chan);

And why do we want to expose this to users? THis doesnt seem right!

A quirk may exists but should be handled inside the controller driver and do
appropriate action. You don't tell users or expect them to handle these

-- 
~Vinod

Re: [PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Shawn Lin]

Hi Vinod,

On 2015/10/5 23:37, Vinod Koul wrote:
> On Mon, Sep 14, 2015 at 07:48:59AM +0800, Shawn Lin wrote:
>> Add dmaengine_get_quirks API for peripheral devices to query
>> quirks if they need it to make special workaround due to broken
>> dma controller design.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>> Changes in v1: None
>>
>>   include/linux/dmaengine.h | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index e2f5eb4..5174ca4 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -704,6 +704,7 @@ struct dma_device {
>>
>>   	int (*device_config)(struct dma_chan *chan,
>>   			     struct dma_slave_config *config);
>> +	int (*device_get_quirks)(struct dma_chan *chan);
>
> And why do we want to expose this to users? THis doesnt seem right!
>

Basically I agree not to expose dma's quirk to slave controllers...But, 
the fact I mentioned on cover letter explain the reasons why I have to 
let slave controllers know that they are working with a broken dma. It's 
a dilemma that if we don't want that to be exposed(let slave 
controllers' driver get the info via a API), we have to add broken quirk 
for all of them ,here and there, which seems to be a disaster:(

I would appreciate it if you could give me some suggestions at your 
earliest convenience. :)

> A quirk may exists but should be handled inside the controller driver and do
> appropriate action. You don't tell users or expect them to handle these
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Vinod Koul]

On Tue, Oct 06, 2015 at 05:21:13PM +0800, Shawn Lin wrote:

> >>+	int (*device_get_quirks)(struct dma_chan *chan);
> >
> >And why do we want to expose this to users? THis doesnt seem right!
> >
> 
> Basically I agree not to expose dma's quirk to slave controllers...But, the
> fact I mentioned on cover letter explain the reasons why I have to let slave
> controllers know that they are working with a broken dma. It's a dilemma
> that if we don't want that to be exposed(let slave controllers' driver get
> the info via a API), we have to add broken quirk for all of them ,here and
> there, which seems to be a disaster:(
> 
> I would appreciate it if you could give me some suggestions at your earliest
> convenience. :)
> 
> >A quirk may exists but should be handled inside the controller driver and do
> >appropriate action. You don't tell users or expect them to handle these
> >
I laready gave one re-read the above lines.

Anyway I went ahead and read the usage. You are setting the slave parameters
for this. I can see two ways:
1. Have the quirk to driver and based on quirk reset the slave settings when
they are set by client.
2. Put this in DT and set the dma properties based on these quirks and let
driver and cleint be agnostic to it

-- 
~Vinod

Re: [alsa-devel] [PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Lars-Peter Clausen]

On 10/06/2015 11:21 AM, Shawn Lin wrote:
> Hi Vinod,
> 
> On 2015/10/5 23:37, Vinod Koul wrote:
>> On Mon, Sep 14, 2015 at 07:48:59AM +0800, Shawn Lin wrote:
>>> Add dmaengine_get_quirks API for peripheral devices to query
>>> quirks if they need it to make special workaround due to broken
>>> dma controller design.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>> ---
>>>
>>> Changes in v5: None
>>> Changes in v4: None
>>> Changes in v3: None
>>> Changes in v2: None
>>> Changes in v1: None
>>>
>>>   include/linux/dmaengine.h | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>> index e2f5eb4..5174ca4 100644
>>> --- a/include/linux/dmaengine.h
>>> +++ b/include/linux/dmaengine.h
>>> @@ -704,6 +704,7 @@ struct dma_device {
>>>
>>>       int (*device_config)(struct dma_chan *chan,
>>>                    struct dma_slave_config *config);
>>> +    int (*device_get_quirks)(struct dma_chan *chan);
>>
>> And why do we want to expose this to users? THis doesnt seem right!
>>
> 
> Basically I agree not to expose dma's quirk to slave controllers...But, the
> fact I mentioned on cover letter explain the reasons why I have to let slave
> controllers know that they are working with a broken dma. It's a dilemma
> that if we don't want that to be exposed(let slave controllers' driver get
> the info via a API), we have t add broken quirk for all of them ,here and
> there, which seems to be a disaster:(

The problem with this API is that it transports values with device specific
meanings over a generic API. Which is generally speaking not a good idea
because the consumer witch is supposed to be generic suddenly needs to know
which provider it is talking to.

A better solution in this case typically is either introduce a generic API
with generic values or a custom API with custom values, but don't mix the two.

> 
> I would appreciate it if you could give me some suggestions at your earliest
> convenience. :)

In this case I think the best way to handle this is not quirks, but rather
expose the actual maximum burst size using the DMA capabilities API. Since
supporting only a certain burst depth is not really a quirk. All hardware
has a limit for this and for some it might be larger or smaller than for
others and it might be the same IP core but the maximum size depends on some
IP core parameters. So this should be discoverable.

- Lars

Re: [PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Shawn Lin]

On 2015/10/7 22:32, Vinod Koul wrote:
> On Tue, Oct 06, 2015 at 05:21:13PM +0800, Shawn Lin wrote:
>
>>>> +	int (*device_get_quirks)(struct dma_chan *chan);
>>>
>>> And why do we want to expose this to users? THis doesnt seem right!
>>>
>>
>> Basically I agree not to expose dma's quirk to slave controllers...But, the
>> fact I mentioned on cover letter explain the reasons why I have to let slave
>> controllers know that they are working with a broken dma. It's a dilemma
>> that if we don't want that to be exposed(let slave controllers' driver get
>> the info via a API), we have to add broken quirk for all of them ,here and
>> there, which seems to be a disaster:(
>>
>> I would appreciate it if you could give me some suggestions at your earliest
>> convenience. :)
>>
>>> A quirk may exists but should be handled inside the controller driver and do
>>> appropriate action. You don't tell users or expect them to handle these
>>>
> I laready gave one re-read the above lines.
>
> Anyway I went ahead and read the usage. You are setting the slave parameters
> for this. I can see two ways:
> 1. Have the quirk to driver and based on quirk reset the slave settings when
> they are set by client.
> 2. Put this in DT and set the dma properties based on these quirks and let
> driver and cleint be agnostic to it

+Mark Brown

Thanks for these.

The first one is hard for dma to distinguish "broken 
slave"(I2S/SPI/UART..) from the unbroken ones(block devices like 
mmc/SFC...). If all clients are broken for that, it's easy to reset the 
slave parameters in dma driver. Unfortunately, not always, at least for 
rockchips' Socs.

And before V4, I did pass quirks to clents from DT, but Mark thought we 
should avoid to get it from dt. Anyway, now I think about it again,

(1) dma drivers doesn't know the users who should use limited burst and 
who won't. That make it's impossible for dma drivers to handle it alone 
or expose new API of burst capabilities to clients.
(2) It's inappropriate to expose quirks to clients.

so the only way I can see is to make some tricks from DT.
I would prefer to add "burst limit" property for the broken slaves 
instead of quirks for them which I did before V4.

Something like:

&spi{
max-burst-len=<1>;
}

How about?
:)

>


-- 
Best Regards
Shawn Lin

Re: [alsa-devel] [PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Shawn Lin]

在 2015/10/8 16:31, Lars-Peter Clausen 写道:
> On 10/06/2015 11:21 AM, Shawn Lin wrote:
>> Hi Vinod,
>>
>> On 2015/10/5 23:37, Vinod Koul wrote:
>>> On Mon, Sep 14, 2015 at 07:48:59AM +0800, Shawn Lin wrote:
>>>> Add dmaengine_get_quirks API for peripheral devices to query
>>>> quirks if they need it to make special workaround due to broken
>>>> dma controller design.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v5: None
>>>> Changes in v4: None
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>> Changes in v1: None
>>>>
>>>>    include/linux/dmaengine.h | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>>> index e2f5eb4..5174ca4 100644
>>>> --- a/include/linux/dmaengine.h
>>>> +++ b/include/linux/dmaengine.h
>>>> @@ -704,6 +704,7 @@ struct dma_device {
>>>>
>>>>        int (*device_config)(struct dma_chan *chan,
>>>>                     struct dma_slave_config *config);
>>>> +    int (*device_get_quirks)(struct dma_chan *chan);
>>>
>>> And why do we want to expose this to users? THis doesnt seem right!
>>>
>>
>> Basically I agree not to expose dma's quirk to slave controllers...But, the
>> fact I mentioned on cover letter explain the reasons why I have to let slave
>> controllers know that they are working with a broken dma. It's a dilemma
>> that if we don't want that to be exposed(let slave controllers' driver get
>> the info via a API), we have t add broken quirk for all of them ,here and
>> there, which seems to be a disaster:(
>
> The problem with this API is that it transports values with device specific
> meanings over a generic API. Which is generally speaking not a good idea
> because the consumer witch is supposed to be generic suddenly needs to know
> which provider it is talking to.
>
> A better solution in this case typically is either introduce a generic API
> with generic values or a custom API with custom values, but don't mix the two.
>
>>
>> I would appreciate it if you could give me some suggestions at your earliest
>> convenience. :)
>
> In this case I think the best way to handle this is not quirks, but rather
> expose the actual maximum burst size using the DMA capabilities API. Since
> supporting only a certain burst depth is not really a quirk. All hardware
> has a limit for this and for some it might be larger or smaller than for
> others and it might be the same IP core but the maximum size depends on some
> IP core parameters. So this should be discoverable.
>

Hi Lars,

Thanks for looking for that.

It's a good idea if all clients of the Soc are broken, but unfortunately 
some of them work. So... max burst shoule be different for individuals.

> - Lars
>
>
>
>


-- 
Best Regards
Shawn Lin

Re: [alsa-devel] [PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Lars-Peter Clausen]

On 10/09/2015 01:31 PM, Shawn Lin wrote:
> 在 2015/10/8 16:31, Lars-Peter Clausen 写道:
>> On 10/06/2015 11:21 AM, Shawn Lin wrote:
>>> Hi Vinod,
>>>
>>> On 2015/10/5 23:37, Vinod Koul wrote:
>>>> On Mon, Sep 14, 2015 at 07:48:59AM +0800, Shawn Lin wrote:
>>>>> Add dmaengine_get_quirks API for peripheral devices to query
>>>>> quirks if they need it to make special workaround due to broken
>>>>> dma controller design.
>>>>>
>>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>> ---
>>>>>
>>>>> Changes in v5: None
>>>>> Changes in v4: None
>>>>> Changes in v3: None
>>>>> Changes in v2: None
>>>>> Changes in v1: None
>>>>>
>>>>>    include/linux/dmaengine.h | 9 +++++++++
>>>>>    1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>>>> index e2f5eb4..5174ca4 100644
>>>>> --- a/include/linux/dmaengine.h
>>>>> +++ b/include/linux/dmaengine.h
>>>>> @@ -704,6 +704,7 @@ struct dma_device {
>>>>>
>>>>>        int (*device_config)(struct dma_chan *chan,
>>>>>                     struct dma_slave_config *config);
>>>>> +    int (*device_get_quirks)(struct dma_chan *chan);
>>>>
>>>> And why do we want to expose this to users? THis doesnt seem right!
>>>>
>>>
>>> Basically I agree not to expose dma's quirk to slave controllers...But, the
>>> fact I mentioned on cover letter explain the reasons why I have to let slave
>>> controllers know that they are working with a broken dma. It's a dilemma
>>> that if we don't want that to be exposed(let slave controllers' driver get
>>> the info via a API), we have t add broken quirk for all of them ,here and
>>> there, which seems to be a disaster:(
>>
>> The problem with this API is that it transports values with device specific
>> meanings over a generic API. Which is generally speaking not a good idea
>> because the consumer witch is supposed to be generic suddenly needs to know
>> which provider it is talking to.
>>
>> A better solution in this case typically is either introduce a generic API
>> with generic values or a custom API with custom values, but don't mix the
>> two.
>>
>>>
>>> I would appreciate it if you could give me some suggestions at your earliest
>>> convenience. :)
>>
>> In this case I think the best way to handle this is not quirks, but rather
>> expose the actual maximum burst size using the DMA capabilities API. Since
>> supporting only a certain burst depth is not really a quirk. All hardware
>> has a limit for this and for some it might be larger or smaller than for
>> others and it might be the same IP core but the maximum size depends on some
>> IP core parameters. So this should be discoverable.
>>
> 
> Hi Lars,
> 
> Thanks for looking for that.
> 
> It's a good idea if all clients of the Soc are broken, but unfortunately
> some of them work. So... max burst shoule be different for individuals.

Well, the dma_get_slave_caps() API works on a DMA channel, so I don't think
this will be a problem.

- Lars

Re: [alsa-devel] [PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Vinod Koul]

On Thu, Oct 08, 2015 at 10:31:18AM +0200, Lars-Peter Clausen wrote:
> > Basically I agree not to expose dma's quirk to slave controllers...But, the
> > fact I mentioned on cover letter explain the reasons why I have to let slave
> > controllers know that they are working with a broken dma. It's a dilemma
> > that if we don't want that to be exposed(let slave controllers' driver get
> > the info via a API), we have t add broken quirk for all of them ,here and
> > there, which seems to be a disaster:(
> 
> The problem with this API is that it transports values with device specific
> meanings over a generic API. Which is generally speaking not a good idea
> because the consumer witch is supposed to be generic suddenly needs to know
> which provider it is talking to.
> 
> A better solution in this case typically is either introduce a generic API
> with generic values or a custom API with custom values, but don't mix the two.
> 
> > 
> > I would appreciate it if you could give me some suggestions at your earliest
> > convenience. :)
> 
> In this case I think the best way to handle this is not quirks, but rather
> expose the actual maximum burst size using the DMA capabilities API. Since
> supporting only a certain burst depth is not really a quirk. All hardware
> has a limit for this and for some it might be larger or smaller than for
> others and it might be the same IP core but the maximum size depends on some
> IP core parameters. So this should be discoverable.

yes that makes more sense than adding quirks, exposing the right values
which should be a readable property for driver will ensure it works on
system with/without quirks

-- 
~Vinod

Re: [alsa-devel] [PATCH v5 06/10] dmaengine: add API for getting dma controller's quirk

[Shawn Lin]

On 2015/10/14 18:53, Vinod Koul wrote:
> On Thu, Oct 08, 2015 at 10:31:18AM +0200, Lars-Peter Clausen wrote:
>>> Basically I agree not to expose dma's quirk to slave controllers...But, the
>>> fact I mentioned on cover letter explain the reasons why I have to let slave
>>> controllers know that they are working with a broken dma. It's a dilemma
>>> that if we don't want that to be exposed(let slave controllers' driver get
>>> the info via a API), we have t add broken quirk for all of them ,here and
>>> there, which seems to be a disaster:(
>>
>> The problem with this API is that it transports values with device specific
>> meanings over a generic API. Which is generally speaking not a good idea
>> because the consumer witch is supposed to be generic suddenly needs to know
>> which provider it is talking to.
>>
>> A better solution in this case typically is either introduce a generic API
>> with generic values or a custom API with custom values, but don't mix the two.
>>
>>>
>>> I would appreciate it if you could give me some suggestions at your earliest
>>> convenience. :)
>>
>> In this case I think the best way to handle this is not quirks, but rather
>> expose the actual maximum burst size using the DMA capabilities API. Since
>> supporting only a certain burst depth is not really a quirk. All hardware
>> has a limit for this and for some it might be larger or smaller than for
>> others and it might be the same IP core but the maximum size depends on some
>> IP core parameters. So this should be discoverable.
>
> yes that makes more sense than adding quirks, exposing the right values
> which should be a readable property for driver will ensure it works on
> system with/without quirks

Sorry for late response in this thread.

Right, we can expose max-burst to clients by dma_slave_caps instead of 
quirks. I will try it and send v6 ASAP.

Thanks Lars and Vinod.

>


-- 
Best Regards
Shawn Lin