[v3,8/9] spi: rockchip: modify DMA max burst to 1
diff mbox

Message ID 1441101524-1700-1-git-send-email-shawn.lin@rock-chips.com
State New
Headers show

Commit Message

Shawn Lin Sept. 1, 2015, 9:58 a.m. UTC
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>
---

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

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

Comments

Mark Brown Sept. 3, 2015, 12:54 p.m. UTC | #1
On Tue, Sep 01, 2015 at 05:58:44PM +0800, Shawn Lin wrote:

> 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.

If this is a quirk of the DMA controller why is it being fixed with a
property specific to the SPI controller?  Can't the DMA controller just
override the burst size?

> +#define ROCKCHIP_SPI_BORKEN_BURST_LEN (1<<0) /* broken burst len*/

This is misspelt.
Shawn Lin Sept. 3, 2015, 1:41 p.m. UTC | #2
On 2015/9/3 20:54, Mark Brown wrote:
> On Tue, Sep 01, 2015 at 05:58:44PM +0800, Shawn Lin wrote:
>
>> 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.
>
> If this is a quirk of the DMA controller why is it being fixed with a
> property specific to the SPI controller?  Can't the DMA controller just
> override the burst size?
>

Cool...that's a good question. Actually DMA controller can't do that 
since block peripheral controller(e.g sd/emmc, they always access data 
by block-512Bytes.) can't meet the case. So It's hard for dma controller 
know who is the caller, and what should be done.

The only thing dma controller can do is avoid to execute DMAFLUSHP on 
its own. But how about peripheral controller trigger non-multiple 
access? So that's what I mentioned on covery letter that we should do 
"broken things" for both dma controller and some peripherals.

>> +#define ROCKCHIP_SPI_BORKEN_BURST_LEN (1<<0) /* broken burst len*/
>
> This is misspelt.

Woops, sorry for the bit. Thanks, Mark.

>
Mark Brown Sept. 3, 2015, 5:45 p.m. UTC | #3
On Thu, Sep 03, 2015 at 09:41:43PM +0800, Shawn Lin wrote:
> On 2015/9/3 20:54, Mark Brown wrote:

> >If this is a quirk of the DMA controller why is it being fixed with a
> >property specific to the SPI controller?  Can't the DMA controller just
> >override the burst size?

> Cool...that's a good question. Actually DMA controller can't do that since
> block peripheral controller(e.g sd/emmc, they always access data by
> block-512Bytes.) can't meet the case. So It's hard for dma controller know
> who is the caller, and what should be done.

You could take a guess based on request size, but that's going to be a
bit error prone I imagine.

> The only thing dma controller can do is avoid to execute DMAFLUSHP on its
> own. But how about peripheral controller trigger non-multiple access? So
> that's what I mentioned on covery letter that we should do "broken things"
> for both dma controller and some peripherals.

The other thing it could do is provide the information out via some
internal Linux API which the drivers could query instead of directly
having a DT property in each client driver.  That way at least the DT
only needs the quirk in one place even if the clients need to handle it
individually.
Shawn Lin Sept. 4, 2015, 12:03 a.m. UTC | #4
On 2015/9/4 1:45, Mark Brown wrote:
> On Thu, Sep 03, 2015 at 09:41:43PM +0800, Shawn Lin wrote:
>> On 2015/9/3 20:54, Mark Brown wrote:
>
>>> If this is a quirk of the DMA controller why is it being fixed with a
>>> property specific to the SPI controller?  Can't the DMA controller just
>>> override the burst size?
>
>> Cool...that's a good question. Actually DMA controller can't do that since
>> block peripheral controller(e.g sd/emmc, they always access data by
>> block-512Bytes.) can't meet the case. So It's hard for dma controller know
>> who is the caller, and what should be done.
>
> You could take a guess based on request size, but that's going to be a
> bit error prone I imagine.
>

yes, not always cover all cases.

>> The only thing dma controller can do is avoid to execute DMAFLUSHP on its
>> own. But how about peripheral controller trigger non-multiple access? So
>> that's what I mentioned on covery letter that we should do "broken things"
>> for both dma controller and some peripherals.
>
> The other thing it could do is provide the information out via some
> internal Linux API which the drivers could query instead of directly
> having a DT property in each client driver.  That way at least the DT
> only needs the quirk in one place even if the clients need to handle it
> individually.

Good idea. Thanks, Mark.

>

Patch
diff mbox

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 68e7efe..3748ca5 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 quirk;
+#define ROCKCHIP_SPI_BORKEN_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->quirk & ROCKCHIP_SPI_BORKEN_BURST_LEN)
+			rxconf.src_maxburst = 1;
+		else
+			rxconf.src_maxburst = rs->n_bytes;
 		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->quirk & ROCKCHIP_SPI_BORKEN_BURST_LEN)
+			txconf.dst_maxburst = 1;
+		else
+			txconf.dst_maxburst = rs->n_bytes;
 		dmaengine_slave_config(rs->dma_tx.ch, &txconf);
 
 		txdesc = dmaengine_prep_slave_sg(
@@ -699,6 +707,10 @@  static int rockchip_spi_probe(struct platform_device *pdev)
 		goto err_get_fifo_len;
 	}
 
+	if (of_property_read_bool(pdev->dev.of_node,
+				  "rockchip,spi-broken-burst-len"))
+		rs->quirk |= ROCKCHIP_SPI_BORKEN_BURST_LEN;
+
 	spin_lock_init(&rs->lock);
 
 	pm_runtime_set_active(&pdev->dev);