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

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

[Hein Tibosch]

From: Hein Tibosch <hein_tibosch@yahoo.es>

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 <hein_tibosch@yahoo.es>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/dma/dw_dmac.c   |   13 +++++++------
 include/linux/dw_dmac.h |    3 +++
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index d3c5a5a..311953c 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -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);
diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
index 2412e02..330afb2 100644
--- a/include/linux/dw_dmac.h
+++ b/include/linux/dw_dmac.h
@@ -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 */
-- 
1.7.8.0

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

[Andy Shevchenko]

On Sun, Sep 2, 2012 at 8:54 PM, Hein Tibosch <hein_tibosch@yahoo.es> wrote:
> From: Hein Tibosch <hein_tibosch@yahoo.es>
>
> 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 */
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.

> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index d3c5a5a..311953c 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -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;

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Viresh Kumar]

On 3 September 2012 13:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> #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.

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

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

[Andy Shevchenko]

On Mon, Sep 3, 2012 at 11:30 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 3 September 2012 13:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> #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.

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



-- 
With Best Regards,
Andy Shevchenko

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

[Viresh Kumar]

On 3 September 2012 14:19, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Sep 3, 2012 at 11:30 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> 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.

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

[Hein Tibosch]

On 9/3/2012 4:59 PM, Viresh Kumar wrote:
> On 3 September 2012 14:19, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Mon, Sep 3, 2012 at 11:30 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> 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.

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

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

[Andy Shevchenko]

On Mon, Sep 3, 2012 at 4:06 PM, Hein Tibosch <hein_tibosch@yahoo.es> wrote:
> 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?

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

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

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

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

-- 
With Best Regards,
Andy Shevchenko