dw_dmac: max_mem_width limits value for SRC/DST_TR_WID register

PATCH In review Hein Tibosch 3 participants 7 messages Sep 2, 2012 – Sep 4, 2012 dmaengine

lore

Share & tools

lore

Hein Tibosch

author

2012-09-02 18:53 UTC #0 lore raw

PATCH v4 2/3 dw_dmac: max_mem_width limits value for SRC/DST_TR_WID register

From: Hein Tibosch <[email protected]>

v4: now based and tested on 3.6-rc4

The dw_dmac driver was earlier adapted to do 64-bit transfers on the memory side (see https://lkml.org/lkml/2012/1/18/52)
This works on ARM platforms but for AVR32 (AP700x) the maximum allowed transfer size is 32-bits.
This patch allows the arch code to set a new slave property max_mem_width to limit the size.

Allowable values are:

#define DW_MEM_WIDTH_64 0 /* default */
#define DW_MEM_WIDTH_32 1 /* e.g. for avr32 */

Signed-off-by Hein Tibosch <[email protected]>

Acked-by Viresh Kumar <[email protected]>

---
 drivers/dma/dw_dmac.c   |   13 +++++++------
 include/linux/dw_dmac.h |    3 +++
 2 files changed, 10 insertions(+), 6 deletions(-)

drivers/dma/dw_dmac.c +7 -6

index d3c5a5a..311953c 100644
@@ -190,14 +190,14 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 }
 
 /*----------------------------------------------------------------------*/
-
-static inline unsigned int dwc_fast_fls(unsigned long long v)
+static inline unsigned int dwc_fast_fls(unsigned long long v,
+	struct dw_dma_slave *dws)
 {
 	/*
 	 * We can be a lot more clever here, but this should take care
 	 * of the most common optimization.
 	 */
-	if (!(v & 7))
+	if (dws->max_mem_width == DW_MEM_WIDTH_64 && !(v & 7))
 		return 3;
 	else if (!(v & 3))
 		return 2;

@@ -636,6 +636,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 		size_t len, unsigned long flags)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
+	struct dw_dma_slave	*dws = chan->private;
 	struct dw_desc		*desc;
 	struct dw_desc		*first;
 	struct dw_desc		*prev;

@@ -655,7 +656,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 		return NULL;
 	}
 
-	src_width = dst_width = dwc_fast_fls(src | dest | len);
+	src_width = dst_width = dwc_fast_fls(src | dest | len, dws);
 
 	ctllo = DWC_DEFAULT_CTLLO(chan)
 			| DWC_CTLL_DST_WIDTH(dst_width)

@@ -755,7 +756,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 			mem = sg_dma_address(sg);
 			len = sg_dma_len(sg);
 
-			mem_width = dwc_fast_fls(mem | len);
+			mem_width = dwc_fast_fls(mem | len, dws);
 
 slave_sg_todev_fill_desc:
 			desc = dwc_desc_get(dwc);

@@ -815,7 +816,7 @@ slave_sg_todev_fill_desc:
 			mem = sg_dma_address(sg);
 			len = sg_dma_len(sg);
 
-			mem_width = dwc_fast_fls(mem | len);
+			mem_width = dwc_fast_fls(mem | len, dws);
 
 slave_sg_fromdev_fill_desc:
 			desc = dwc_desc_get(dwc);

include/linux/dw_dmac.h +3 -0

index 2412e02..330afb2 100644
@@ -58,6 +58,9 @@ struct dw_dma_slave {
 	u32			cfg_lo;
 	u8			src_master;
 	u8			dst_master;
+#define	DW_MEM_WIDTH_64		0
+#define	DW_MEM_WIDTH_32		1	/* e.g. for avr32 */
+	u8			max_mem_width;
 };
 
 /* Platform-configurable bits in CFG_HI */

Signature · 1 line

1.7.8.0

Andy Shevchenko

2012-09-03 08:25 UTC #1 lore raw

Quoted 11 lines · Hein Tibosch

From: Hein Tibosch <[email protected]>
v4: now based and tested on 3.6-rc4
The dw_dmac driver was earlier adapted to do 64-bit transfers on the memory side (see https://lkml.org/lkml/2012/1/18/52)
This works on ARM platforms but for AVR32 (AP700x) the maximum allowed transfer size is 32-bits.
This patch allows the arch code to set a new slave property max_mem_width to limit the size.

Quoted 4 lines

Allowable values are:

#define DW_MEM_WIDTH_64         0       /* default */
#define DW_MEM_WIDTH_32         1       /* e.g. for avr32 */

There are 4 options: 32, 64, 128, and 256 bits. I would prefer to see the value in conjunction with
real value in the register, namely 2 for 32, 3 - 64, 4 - 128, 5 - 256.

Quoted 24 lines

drivers/dma/dw_dmac.c +3 -3

index d3c5a5a..311953c 100644
@@ -190,14 +190,14 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 }
 
 /*----------------------------------------------------------------------*/
-
-static inline unsigned int dwc_fast_fls(unsigned long long v)
+static inline unsigned int dwc_fast_fls(unsigned long long v,
+       struct dw_dma_slave *dws)
 {
        /*
         * We can be a lot more clever here, but this should take care
         * of the most common optimization.
         */
-       if (!(v & 7))
+       if (dws->max_mem_width == DW_MEM_WIDTH_64 && !(v & 7))
                return 3;
        else if (!(v & 3))
                return 2;

drivers/dma/dw_dmac.c dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, · line 659

Quoted 5 lines

drivers/dma/dw_dmac.c +1 -1

@@ -655,7 +656,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
                return NULL;
        }
 
-       src_width = dst_width = dwc_fast_fls(src | dest | len);
+       src_width = dst_width = dwc_fast_fls(src | dest | len, dws);

And taking into consideration my upper comment, it could be easier to use min_t(unsigned int, max_mem_width, dwc_fast_fls(...)) here and there.

drivers/dma/dw_dmac.c struct dw_dma_slave { · line 63

Quoted 7 lines

drivers/dma/dw_dmac.c +3 -0

@@ -58,6 +58,9 @@ struct dw_dma_slave {
        u32                     cfg_lo;
        u8                      src_master;
        u8                      dst_master;
+#define        DW_MEM_WIDTH_64         0
+#define        DW_MEM_WIDTH_32         1       /* e.g. for avr32 */
+       u8                      max_mem_width;

Might be I missed something, but why is it slave configuration? I think the controller (actually channel) structure is more suitable to keep that field inside.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Viresh Kumar

2012-09-03 08:30 UTC #2 lore raw

Quoted 5 lines · Andy Shevchenko

Quoted 2 lines
#define DW_MEM_WIDTH_64         0       /* default */
#define DW_MEM_WIDTH_32         1       /* e.g. for avr32 */
There are 4 options: 32, 64, 128, and 256 bits. I would prefer to see the value in conjunction with
real value in the register, namely 2 for 32, 3 - 64, 4 - 128, 5 - 256.

Which register are you talking about? This configuration is outside of DMAC controller and i am not sure if dw DMAC controller can do 128 or 256 bit transfers.

Quoted 10 lines

include/linux/dw_dmac.h struct dw_dma_slave { · line 63

Quoted 7 lines

include/linux/dw_dmac.h +3 -0

@@ -58,6 +58,9 @@ struct dw_dma_slave {
        u32                     cfg_lo;
        u8                      src_master;
        u8                      dst_master;
+#define        DW_MEM_WIDTH_64         0
+#define        DW_MEM_WIDTH_32         1       /* e.g. for avr32 */
+       u8                      max_mem_width;

Might be I missed something, but why is it slave configuration? I think the controller (actually channel) structure is more suitable to keep that field inside.

@Hein: Even i missed it. How will you do memcpy transfers as we don't have this structure there. Probably you need to move this to DMA controller platform data filed.

viresh

Andy Shevchenko

2012-09-03 08:49 UTC #3 lore raw

Quoted 10 lines · Viresh Kumar

Quoted 5 lines · Andy Shevchenko
Quoted 2 lines
#define DW_MEM_WIDTH_64         0       /* default */
#define DW_MEM_WIDTH_32         1       /* e.g. for avr32 */
There are 4 options: 32, 64, 128, and 256 bits. I would prefer to see the value in conjunction with
real value in the register, namely 2 for 32, 3 - 64, 4 - 128, 5 - 256.
Which register are you talking about? This configuration is outside of DMAC controller and i am not sure if dw DMAC controller can do 128 or 256 bit transfers.

SRC_WIDTH & DST_WIDTH in CTLx. The field are 3 bit long. Acceptable values from 0 to 5.
2 corresponds to 32 bit transfers.

Quoted 16 lines

Quoted 10 lines
include/linux/dw_dmac.h struct dw_dma_slave { · line 63
Quoted 7 lines
include/linux/dw_dmac.h +3 -0
@@ -58,6 +58,9 @@ struct dw_dma_slave {
        u32                     cfg_lo;
        u8                      src_master;
        u8                      dst_master;
+#define        DW_MEM_WIDTH_64         0
+#define        DW_MEM_WIDTH_32         1       /* e.g. for avr32 */
+       u8                      max_mem_width;
Might be I missed something, but why is it slave configuration? I think the controller (actually channel) structure is more suitable to keep that field inside.
@Hein: Even i missed it. How will you do memcpy transfers as we don't have this structure there. Probably you need to move this to DMA controller platform data filed.
viresh

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Viresh Kumar

2012-09-03 08:59 UTC #4 lore raw

Quoted 7 lines · Andy Shevchenko

Quoted 3 lines · Viresh Kumar
Which register are you talking about? This configuration is outside of DMAC controller and i am not sure if dw DMAC controller can do 128 or 256 bit transfers.
SRC_WIDTH & DST_WIDTH in CTLx. The field are 3 bit long. Acceptable values from 0 to 5.
2 corresponds to 32 bit transfers.

The field is 3 bit long but only allowable values are 0,1,2 & 3... This is what i can check in my copy of dw_dmac manual.

4 and 5 aren't valid values.

Hein Tibosch

author

2012-09-03 13:49 UTC #5 lore raw

Quoted 5 lines · Viresh Kumar

Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Viresh Kumar
Which register are you talking about? This configuration is outside of DMAC controller and i am not sure if dw DMAC controller can do 128 or 256 bit transfers.

Quoted 7 lines

Quoted 3 lines
SRC_WIDTH & DST_WIDTH in CTLx. The field are 3 bit long. Acceptable values from 0 to 5.
2 corresponds to 32 bit transfers.
The field is 3 bit long but only allowable values are 0,1,2 & 3... This is what i can check in my copy of dw_dmac manual.
4 and 5 aren't valid values.

About today's remarks about the patch series:

1. The first draft of the patches worked with the max allowable value for the SRC_WIDTH & DST_WIDTH fields: 0,1,2,3... Viresh thought it was not transparent enough, he suggested to make it simpler with a binary choice of 32- or 64-bits, defaulting to 64-bits.
But Andy is right: there are versions supporting 256-bit wide memory transfers. I'd also go for this previous solution and use: "min(max_mem_width, width)"

The only problem is that one doesn't want to change arch code for other platforms (ARM) so I proposed: let "max_mem_width=0" mean: leave it up to the driver, for now 3 : 64-bits.

2. In another version I made 'max_mem_width' a member of 'dw_dma_platform_data' because I also see it as 'constant' for all dma slaves.
But the dw_dmac controller can be used for multiple (types of) memories and in that case, maybe a limit per slave might be desirable? My knowledge of DMA-hardware doesn't reach far enough to judge that.

I'd say: for now let it become a member of 'dw_dma_platform_data' because it's the max value of a register field.

3. Felipe Balbi: why don't we ask the DW IP for its maximum allowed value of SRC_WIDTH & DST_WIDTH (on the memory side)? Sure, would be elegant! I contacted Synopsys this week but as my company doesn't have a contract with them, they won't share any secrets. And before sharing secrets, I'd need to sign a non-disclosure contract with them...
I was particularly interested in the register called 'DMA ID Register' at offset 0x3a8, which has no description in the Atmel sheets.

Alternatively, we could do a small dma-memcpy-test at start-up and try all values from 5 (or 7) down to 2. The first value that works correctly will be used as the maximum.
But that'll take some CPU time, because with invalid settings the memcpy will timeout. I wouldn't mind to try this to see if it is a solution.

Hein

Andy Shevchenko

2012-09-04 06:38 UTC #6 lore raw

Quoted 10 lines · Hein Tibosch

1. The first draft of the patches worked with the max allowable value for the SRC_WIDTH & DST_WIDTH fields: 0,1,2,3... Viresh thought it was not transparent enough, he suggested to make it simpler with a binary choice of 32- or 64-bits, defaulting to 64-bits.
But Andy is right: there are versions supporting 256-bit wide memory transfers. I'd also go for this previous solution and use: "min(max_mem_width, width)"
The only problem is that one doesn't want to change arch code for other platforms (ARM) so I proposed: let "max_mem_width=0" mean: leave it up to the driver, for now 3 : 64-bits.

Sounds better to support all possible options without any additional layer of conversion, isn't it?

Quoted 5 lines

2. In another version I made 'max_mem_width' a member of 'dw_dma_platform_data' because I also see it as 'constant' for all dma slaves.
But the dw_dmac controller can be used for multiple (types of) memories and in that case, maybe a limit per slave might be desirable? My knowledge of DMA-hardware doesn't reach far enough to judge that.

As Viresh told early that will not cover memory-to-memory transfers.

Quoted 2 lines

I'd say: for now let it become a member of 'dw_dma_platform_data' because it's the max value of a register field.

I support such choice.

Quoted 2 lines

3. Felipe Balbi: why don't we ask the DW IP for its maximum allowed value of SRC_WIDTH & DST_WIDTH (on the memory side)? Sure, would be elegant!

It's not so simple, unfortunately.

Quoted 3 lines

Alternatively, we could do a small dma-memcpy-test at start-up and try all values from 5 (or 7) down to 2. The first value that works correctly will be used as the maximum.

Oh, it might be good idea to get this value in case neither IP nor platform data provides it.
I'm pretty sure the platform device driver has to know this beforehand.

Signature · 2 lines

With Best Regards,
Andy Shevchenko