[PATCH] dmaengine: rcar-dmac: enable descriptor mode on 40bit

[PATCH] dmaengine: rcar-dmac: enable descriptor mode on 40bit

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

SYS-DMAC can use 40bit address transfer, and it supports Descriptor
Mode too. Current SYS-DMAC driver disables Descriptor Mode if it was
40bit address today. But it can use Descriptor Mode with 40bit if
transfer Source/Destination address are located in same 4GiB region
in the 40 bit address space.
This patch enables it if all condition was clear

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/dma/sh/rcar-dmac.c | 65 +++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 48b22d5..2ebd2be 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -344,13 +344,19 @@ static void rcar_dmac_chan_start_xfer(struct rcar_dmac_chan *chan)
 		rcar_dmac_chan_write(chan, RCAR_DMARS, chan->mid_rid);
 
 	if (desc->hwdescs.use) {
-		struct rcar_dmac_xfer_chunk *chunk;
+		struct rcar_dmac_xfer_chunk *chunk =
+			list_first_entry(&desc->chunks,
+					 struct rcar_dmac_xfer_chunk, node);
 
 		dev_dbg(chan->chan.device->dev,
 			"chan%u: queue desc %p: %u@%pad\n",
 			chan->index, desc, desc->nchunks, &desc->hwdescs.dma);
 
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+		rcar_dmac_chan_write(chan, RCAR_DMAFIXSAR,
+				     chunk->src_addr >> 32);
+		rcar_dmac_chan_write(chan, RCAR_DMAFIXDAR,
+				     chunk->dst_addr >> 32);
 		rcar_dmac_chan_write(chan, RCAR_DMAFIXDPBASE,
 				     desc->hwdescs.dma >> 32);
 #endif
@@ -368,8 +374,6 @@ static void rcar_dmac_chan_start_xfer(struct rcar_dmac_chan *chan)
 		 * should. Initialize it manually with the destination address
 		 * of the first chunk.
 		 */
-		chunk = list_first_entry(&desc->chunks,
-					 struct rcar_dmac_xfer_chunk, node);
 		rcar_dmac_chan_write(chan, RCAR_DMADAR,
 				     chunk->dst_addr & 0xffffffff);
 
@@ -855,8 +859,12 @@ static void rcar_dmac_chan_configure_desc(struct rcar_dmac_chan *chan,
 	unsigned int nchunks = 0;
 	unsigned int max_chunk_size;
 	unsigned int full_size = 0;
-	bool highmem = false;
+	bool cross_boundary = false;
 	unsigned int i;
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+	u32 high_src_addr = 0;
+	u32 high_dst_addr = 0;
+#endif
 
 	desc = rcar_dmac_desc_get(chan);
 	if (!desc)
@@ -885,25 +893,6 @@ static void rcar_dmac_chan_configure_desc(struct rcar_dmac_chan *chan,
 		while (len) {
 			unsigned int size = min(len, max_chunk_size);
 
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-			/*
-			 * Prevent individual transfers from crossing 4GB
-			 * boundaries.
-			 */
-			if (dev_addr >> 32 != (dev_addr + size - 1) >> 32)
-				size = ALIGN(dev_addr, 1ULL << 32) - dev_addr;
-			if (mem_addr >> 32 != (mem_addr + size - 1) >> 32)
-				size = ALIGN(mem_addr, 1ULL << 32) - mem_addr;
-
-			/*
-			 * Check if either of the source or destination address
-			 * can't be expressed in 32 bits. If so we can't use
-			 * hardware descriptor lists.
-			 */
-			if (dev_addr >> 32 || mem_addr >> 32)
-				highmem = true;
-#endif
-
 			chunk = rcar_dmac_xfer_chunk_get(chan);
 			if (!chunk) {
 				rcar_dmac_desc_put(chan, desc);
@@ -918,6 +907,26 @@ static void rcar_dmac_chan_configure_desc(struct rcar_dmac_chan *chan,
 				chunk->dst_addr = dev_addr;
 			}
 
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+			if (i == 0) {
+				high_src_addr = chunk->src_addr >> 32;
+				high_dst_addr = chunk->dst_addr >> 32;
+			}
+
+			/*
+			 * Prevent individual transfers from crossing 4GB
+			 * boundaries.
+			 */
+			if (dev_addr >> 32 != (dev_addr + size - 1) >> 32)
+				size = ALIGN(dev_addr, 1ULL << 32) - dev_addr;
+			if (mem_addr >> 32 != (mem_addr + size - 1) >> 32)
+				size = ALIGN(mem_addr, 1ULL << 32) - mem_addr;
+
+			if ((chunk->src_addr >> 32 != high_src_addr) ||
+			    (chunk->dst_addr >> 32 != high_dst_addr))
+				cross_boundary = true;
+#endif
+
 			chunk->size = size;
 
 			dev_dbg(chan->chan.device->dev,
@@ -943,13 +952,11 @@ static void rcar_dmac_chan_configure_desc(struct rcar_dmac_chan *chan,
 	 * Use hardware descriptor lists if possible when more than one chunk
 	 * needs to be transferred (otherwise they don't make much sense).
 	 *
-	 * The highmem check currently covers the whole transfer. As an
-	 * optimization we could use descriptor lists for consecutive lowmem
-	 * chunks and direct manual mode for highmem chunks. Whether the
-	 * performance improvement would be significant enough compared to the
-	 * additional complexity remains to be investigated.
+	 * Source/Destination address should be located in same 4GiB region
+	 * in the 40bit address space when it uses Hardware descriptor,
+	 * and cross_boundary is checking it.
 	 */
-	desc->hwdescs.use = !highmem && nchunks > 1;
+	desc->hwdescs.use = !cross_boundary && nchunks > 1;
 	if (desc->hwdescs.use) {
 		if (rcar_dmac_fill_hwdesc(chan, desc) < 0)
 			desc->hwdescs.use = false;
-- 
1.9.1

Re: [PATCH] dmaengine: rcar-dmac: enable descriptor mode on 40bit

[Laurent Pinchart]

Hello Morimoto-san,

Thank you for the patch.

On Tuesday 07 Mar 2017 01:10:32 Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> SYS-DMAC can use 40bit address transfer, and it supports Descriptor
> Mode too. Current SYS-DMAC driver disables Descriptor Mode if it was
> 40bit address today. But it can use Descriptor Mode with 40bit if
> transfer Source/Destination address are located in same 4GiB region
> in the 40 bit address space.
> This patch enables it if all condition was clear
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/dma/sh/rcar-dmac.c | 65 +++++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 48b22d5..2ebd2be 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c

[snip]

> @@ -855,8 +859,12 @@ static void rcar_dmac_chan_configure_desc(struct
> rcar_dmac_chan *chan, unsigned int nchunks = 0;
>  	unsigned int max_chunk_size;
>  	unsigned int full_size = 0;
> -	bool highmem = false;
> +	bool cross_boundary = false;
>  	unsigned int i;
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> +	u32 high_src_addr = 0;
> +	u32 high_dst_addr = 0;
> +#endif
> 
>  	desc = rcar_dmac_desc_get(chan);
>  	if (!desc)
> @@ -885,25 +893,6 @@ static void rcar_dmac_chan_configure_desc(struct
> rcar_dmac_chan *chan, while (len) {
>  			unsigned int size = min(len, max_chunk_size);
> 
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -			/*
> -			 * Prevent individual transfers from crossing 4GB
> -			 * boundaries.
> -			 */
> -			if (dev_addr >> 32 != (dev_addr + size - 1) >> 32)
> -				size = ALIGN(dev_addr, 1ULL << 32) - dev_addr;
> -			if (mem_addr >> 32 != (mem_addr + size - 1) >> 32)
> -				size = ALIGN(mem_addr, 1ULL << 32) - mem_addr;
> -
> -			/*
> -			 * Check if either of the source or destination 
address
> -			 * can't be expressed in 32 bits. If so we can't use
> -			 * hardware descriptor lists.
> -			 */
> -			if (dev_addr >> 32 || mem_addr >> 32)
> -				highmem = true;
> -#endif
> -
>  			chunk = rcar_dmac_xfer_chunk_get(chan);
>  			if (!chunk) {
>  				rcar_dmac_desc_put(chan, desc);
> @@ -918,6 +907,26 @@ static void rcar_dmac_chan_configure_desc(struct
> rcar_dmac_chan *chan, chunk->dst_addr = dev_addr;
>  			}
> 
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> +			if (i == 0) {
> +				high_src_addr = chunk->src_addr >> 32;
> +				high_dst_addr = chunk->dst_addr >> 32;
> +			}

This will be triggered for every iteration of the while loop, so if one sg 
entry crosses a 32-bit boundary, you won't detect it.

I know it will mean one additional #ifdef, but I think you should move that 
code out of the while loop as I previously proposed.

> +			/*
> +			 * Prevent individual transfers from crossing 4GB
> +			 * boundaries.
> +			 */
> +			if (dev_addr >> 32 != (dev_addr + size - 1) >> 32)
> +				size = ALIGN(dev_addr, 1ULL << 32) - dev_addr;
> +			if (mem_addr >> 32 != (mem_addr + size - 1) >> 32)
> +				size = ALIGN(mem_addr, 1ULL << 32) - mem_addr;

This would then become

			if (dev_addr >> 32 != (dev_addr + size - 1) >> 32) {
				size = ALIGN(dev_addr, 1ULL << 32) - dev_addr;
				cross_boundary = true;
			}
			if (mem_addr >> 32 != (mem_addr + size - 1) >> 32) {
				size = ALIGN(mem_addr, 1ULL << 32) - mem_addr;
				cross_boundary = true;
			}


> +			if ((chunk->src_addr >> 32 != high_src_addr) ||
> +			    (chunk->dst_addr >> 32 != high_dst_addr))
> +				cross_boundary = true;

And this check can also be moved just before the while loop.

> +#endif
> +
>  			chunk->size = size;
> 
>  			dev_dbg(chan->chan.device->dev,
> @@ -943,13 +952,11 @@ static void rcar_dmac_chan_configure_desc(struct
> rcar_dmac_chan *chan, * Use hardware descriptor lists if possible when more
> than one chunk * needs to be transferred (otherwise they don't make much
> sense). *
> -	 * The highmem check currently covers the whole transfer. As an
> -	 * optimization we could use descriptor lists for consecutive lowmem
> -	 * chunks and direct manual mode for highmem chunks. Whether the
> -	 * performance improvement would be significant enough compared to the
> -	 * additional complexity remains to be investigated.
> +	 * Source/Destination address should be located in same 4GiB region
> +	 * in the 40bit address space when it uses Hardware descriptor,
> +	 * and cross_boundary is checking it.
>  	 */
> -	desc->hwdescs.use = !highmem && nchunks > 1;
> +	desc->hwdescs.use = !cross_boundary && nchunks > 1;
>  	if (desc->hwdescs.use) {
>  		if (rcar_dmac_fill_hwdesc(chan, desc) < 0)
>  			desc->hwdescs.use = false;

-- 
Regards,

Laurent Pinchart

Re: [PATCH] dmaengine: rcar-dmac: enable descriptor mode on 40bit

[Kuninori Morimoto]

Hi Laurent

Thank you for your review

> > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> > +			if (i == 0) {
> > +				high_src_addr = chunk->src_addr >> 32;
> > +				high_dst_addr = chunk->dst_addr >> 32;
> > +			}
> 
> This will be triggered for every iteration of the while loop, so if one sg 
> entry crosses a 32-bit boundary, you won't detect it.
> 
> I know it will mean one additional #ifdef, but I think you should move that 
> code out of the while loop as I previously proposed.

Grr indeed. Thank you for pointing it.
Hmm.. OK, I will add +1 #ifdef

Best regards
---
Kuninori Morimoto