linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] dw_dmac: make driver endianness configurable
@ 2012-08-26 20:53 Hein Tibosch
  2012-08-27  7:03 ` Hans-Christian Egtvedt
  0 siblings, 1 reply; 11+ messages in thread
From: Hein Tibosch @ 2012-08-26 20:53 UTC (permalink / raw)
  To: viresh kumar
  Cc: spear-devel, Linux Kernel Mailing List, ludovic.desroches,
	Havard Skinnemoen, Nicolas Ferre, egtvedt, Andrew Morton,
	Arnd Bergmann

The dw_dmac was originally developed for avr32 to be used with the
Synopsys DesignWare AHB DMA controller. After 2.6.38, device access was done
with the little-endian readl/writel functions. This didn't work on the
avr32 platform, because it needs native-endian (i.e. big-endian) accessors.
This patch makes the endianness configurable using 'DW_DMAC_BE',
which will default be true for AVR32


Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
---
 drivers/dma/Kconfig        |    8 ++++++++
 drivers/dma/dw_dmac_regs.h |   23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index aadeb5b..3635daf 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -89,6 +89,14 @@ config DW_DMAC
 	  Support the Synopsys DesignWare AHB DMA controller.  This
 	  can be integrated in chips such as the Atmel AT32ap7000.
 
+config DW_DMAC_BE
+	bool "Synopsys DesignWare AHB DMA needs big endian access"
+	default y if AVR32
+	depends on DW_DMAC
+	help
+	  Say yes if access to the Synopsys DesignWare AHB DMA controller
+	  should be big endian, such as for Atmel AT32ap7000
+
 config AT_HDMAC
 	tristate "Atmel AHB DMA support"
 	depends on ARCH_AT91
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index f298f69..cf048c3 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -175,11 +175,22 @@ __dwc_regs(struct dw_dma_chan *dwc)
 	return dwc->ch_regs;
 }
 
+#ifdef CONFIG_DW_DMAC_BE
+
+#define channel_readl(dwc, name) \
+	ioread32be(&(__dwc_regs(dwc)->name))
+#define channel_writel(dwc, name, val) \
+	iowrite32be((val), &(__dwc_regs(dwc)->name))
+
+#else
+
 #define channel_readl(dwc, name) \
 	readl(&(__dwc_regs(dwc)->name))
 #define channel_writel(dwc, name, val) \
 	writel((val), &(__dwc_regs(dwc)->name))
 
+#endif
+
 static inline struct dw_dma_chan *to_dw_dma_chan(struct dma_chan *chan)
 {
 	return container_of(chan, struct dw_dma_chan, chan);
@@ -201,11 +212,23 @@ static inline struct dw_dma_regs __iomem *__dw_regs(struct dw_dma *dw)
 	return dw->regs;
 }
 
+#ifdef CONFIG_DW_DMAC_BE
+
+#define dma_readl(dwc, name) \
+	ioread32be(&(__dw_regs(dw)->name))
+#define dma_writel(dwc, name, val) \
+	iowrite32be((val), &(__dw_regs(dw)->name))
+
+#else
+
 #define dma_readl(dw, name) \
 	readl(&(__dw_regs(dw)->name))
 #define dma_writel(dw, name, val) \
 	writel((val), &(__dw_regs(dw)->name))
 
+#endif
+
 #define channel_set_bit(dw, reg, mask) \
 	dma_writel(dw, reg, ((mask) << 8) | (mask))
 #define channel_clear_bit(dw, reg, mask) \
-- 
1.7.8.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-08-26 20:53 [PATCH 1/2] dw_dmac: make driver endianness configurable Hein Tibosch
@ 2012-08-27  7:03 ` Hans-Christian Egtvedt
  2012-08-27  8:47   ` Hein Tibosch
  0 siblings, 1 reply; 11+ messages in thread
From: Hans-Christian Egtvedt @ 2012-08-27  7:03 UTC (permalink / raw)
  To: Hein Tibosch
  Cc: viresh kumar, spear-devel, Linux Kernel Mailing List,
	ludovic.desroches, Havard Skinnemoen, Nicolas Ferre,
	Andrew Morton, Arnd Bergmann

Around Mon 27 Aug 2012 04:53:02 +0800 or thereabout, Hein Tibosch wrote:
> The dw_dmac was originally developed for avr32 to be used with the
> Synopsys DesignWare AHB DMA controller. After 2.6.38, device access was done
> with the little-endian readl/writel functions. This didn't work on the
> avr32 platform, because it needs native-endian (i.e. big-endian) accessors.
> This patch makes the endianness configurable using 'DW_DMAC_BE',
> which will default be true for AVR32

I think the English in kconfig could use some brushing up.

> Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
> ---
>  drivers/dma/Kconfig        |    8 ++++++++
>  drivers/dma/dw_dmac_regs.h |   23 +++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index aadeb5b..3635daf 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -89,6 +89,14 @@ config DW_DMAC
>  	  Support the Synopsys DesignWare AHB DMA controller.  This
>  	  can be integrated in chips such as the Atmel AT32ap7000.
>  
> +config DW_DMAC_BE
>

This name isn't that long, so we could skip the abbreviation of big endian;
DW_DMAC_BIG_ENDIAN_IO or something similar?

> +	bool "Synopsys DesignWare AHB DMA needs big endian access"
>

bool "Use big endian I/O register access"

> +	default y if AVR32
> +	depends on DW_DMAC
> +	help
> +	  Say yes if access to the Synopsys DesignWare AHB DMA controller
> +	  should be big endian, such as for Atmel AT32ap7000
> +

"Say yes here to use big endian I/O access when reading and writing to the
DMA controller registers. This is needed on some platforms, like the Atmel
AVR32 architecture.

If unsure, use the default setting."

<snipp patch diff>

-- 
mvh
Hans-Christian Egtvedt

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-08-27  7:03 ` Hans-Christian Egtvedt
@ 2012-08-27  8:47   ` Hein Tibosch
  2012-08-27 11:14     ` Hans-Christian Egtvedt
  0 siblings, 1 reply; 11+ messages in thread
From: Hein Tibosch @ 2012-08-27  8:47 UTC (permalink / raw)
  To: Hans-Christian Egtvedt
  Cc: viresh kumar, spear-devel, Linux Kernel Mailing List,
	ludovic.desroches, Havard Skinnemoen, Nicolas Ferre,
	Andrew Morton, Arnd Bergmann

On 8/27/2012 3:03 PM, Hans-Christian Egtvedt wrote:
> I think the English in kconfig could use some brushing up.
>  
>> +config DW_DMAC_BE
>
>
> This name isn't that long, so we could skip the abbreviation of big endian;
> DW_DMAC_BIG_ENDIAN_IO or something similar?
>
>> +	bool "Synopsys DesignWare AHB DMA needs big endian access"
>>
> bool "Use big endian I/O register access"

Brushing up the config items:

+config DW_DMAC_BIG_ENDIAN_IO
+	bool "Use big endian I/O register access"
+	default y if AVR32
+	depends on DW_DMAC
+	help
+	  Say yes here to use big endian I/O access when reading and writing
+	  to the DMA controller registers. This is needed on some platforms,
+	  like the Atmel AVR32 architecture.
+
+	  If unsure, use the default setting.

And as I'd like to define the maximum memory transfer width in the same
Kconfig:

+config DW_DMAC_MEM_64_BIT
+	bool "Allow 64-bit memory transfers"
+	default y if !AVR32
+	depends on DW_DMAC
+	help
+	  Say yes if the DMA controller may do 64-bit memory transfers
+	  For AVR32, say no because only up to 32-bit transfers are
+	  defined

Would this be better?

Thanks, Hein

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-08-27  8:47   ` Hein Tibosch
@ 2012-08-27 11:14     ` Hans-Christian Egtvedt
  2012-08-27 14:58       ` Hein Tibosch
  0 siblings, 1 reply; 11+ messages in thread
From: Hans-Christian Egtvedt @ 2012-08-27 11:14 UTC (permalink / raw)
  To: Hein Tibosch
  Cc: viresh kumar, spear-devel, Linux Kernel Mailing List,
	ludovic.desroches, Havard Skinnemoen, Nicolas Ferre,
	Andrew Morton, Arnd Bergmann

Around Mon 27 Aug 2012 16:47:40 +0800 or thereabout, Hein Tibosch wrote:
> On 8/27/2012 3:03 PM, Hans-Christian Egtvedt wrote:
>> I think the English in kconfig could use some brushing up.
>>  
>>> +config DW_DMAC_BE
>>
>>
>> This name isn't that long, so we could skip the abbreviation of big endian;
>> DW_DMAC_BIG_ENDIAN_IO or something similar?
>>
>>> +	bool "Synopsys DesignWare AHB DMA needs big endian access"
>>>
>> bool "Use big endian I/O register access"
> 
> Brushing up the config items:
> 
> +config DW_DMAC_BIG_ENDIAN_IO
> +	bool "Use big endian I/O register access"
> +	default y if AVR32
> +	depends on DW_DMAC
> +	help
> +	  Say yes here to use big endian I/O access when reading and writing
> +	  to the DMA controller registers. This is needed on some platforms,
> +	  like the Atmel AVR32 architecture.
> +
> +	  If unsure, use the default setting.

This sounds good in my ears, but I don't speak English natively.

> And as I'd like to define the maximum memory transfer width in the same
> Kconfig:
> 
> +config DW_DMAC_MEM_64_BIT
> +	bool "Allow 64-bit memory transfers"
> +	default y if !AVR32
> +	depends on DW_DMAC
> +	help
> +	  Say yes if the DMA controller may do 64-bit memory transfers
> +	  For AVR32, say no because only up to 32-bit transfers are
> +	  defined

Is this sane to add? Could some non-AVR32 platforms use 64-bit and 32-bit
depending on runtime configuration? E.g. if you build a kernel with support
for multiple boards/processors, and there is a mix of 32-bit and 64-bit wide
DMA support.

I think it is better to select 32/64-bit at runtime.

-- 
mvh
Hans-Christian Egtvedt

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-08-27 11:14     ` Hans-Christian Egtvedt
@ 2012-08-27 14:58       ` Hein Tibosch
  2012-08-28  3:23         ` Viresh Kumar
  0 siblings, 1 reply; 11+ messages in thread
From: Hein Tibosch @ 2012-08-27 14:58 UTC (permalink / raw)
  To: Hans-Christian Egtvedt
  Cc: viresh kumar, spear-devel, Linux Kernel Mailing List,
	ludovic.desroches, Havard Skinnemoen, Nicolas Ferre,
	Andrew Morton, Arnd Bergmann

On 8/27/2012 7:14 PM, Hans-Christian Egtvedt wrote:
> Around Mon 27 Aug 2012 16:47:40 +0800 or thereabout, Hein Tibosch wrote:
>> On 8/27/2012 3:03 PM, Hans-Christian Egtvedt wrote:
>>> Brushing up the config items:
>>>
>>> +config DW_DMAC_BIG_ENDIAN_IO
>>> +	bool "Use big endian I/O register access"
>>> +	default y if AVR32
>>> +	depends on DW_DMAC
>>> +	help
>>> +	  Say yes here to use big endian I/O access when reading and writing
>>> +	  to the DMA controller registers. This is needed on some platforms,
>>> +	  like the Atmel AVR32 architecture.
>>> +
>>> +	  If unsure, use the default setting.
> This sounds good in my ears, but I don't speak English natively.
I think you Norwegians are doing very well in English
And btw, I'm not from .es but from .nl, which is very close to England

>> And as I'd like to define the maximum memory transfer width in the same
>> Kconfig:
>>
>> +config DW_DMAC_MEM_64_BIT
>> +	bool "Allow 64-bit memory transfers"
>> +	default y if !AVR32
>> +	depends on DW_DMAC
>> +	help
>> +	  Say yes if the DMA controller may do 64-bit memory transfers
>> +	  For AVR32, say no because only up to 32-bit transfers are
>> +	  defined
> Is this sane to add? Could some non-AVR32 platforms use 64-bit and 32-bit
> depending on runtime configuration? E.g. if you build a kernel with support
> for multiple boards/processors, and there is a mix of 32-bit and 64-bit wide
> DMA support.
>
> I think it is better to select 32/64-bit at runtime.

I did that in the first patch, adding a new property to the dw_dma_slave
structure. It had the small disadvantage that some arch code had to be
adapted (at32ap700x.c).

Viresh, what do you think? Add a property called "mem_64_bit_access" or so?

Or should it be passed as a member of 'dw_dma_platform_data', because it
is a property of the (entire) DMA controller?

Hein

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-08-27 14:58       ` Hein Tibosch
@ 2012-08-28  3:23         ` Viresh Kumar
  2012-08-28  6:55           ` Hein Tibosch
  0 siblings, 1 reply; 11+ messages in thread
From: Viresh Kumar @ 2012-08-28  3:23 UTC (permalink / raw)
  To: Hein Tibosch
  Cc: Hans-Christian Egtvedt, spear-devel, Linux Kernel Mailing List,
	ludovic.desroches, Havard Skinnemoen, Nicolas Ferre,
	Andrew Morton, Arnd Bergmann

On 27 August 2012 20:28, Hein Tibosch <hein_tibosch@yahoo.es> wrote:
>>> +config DW_DMAC_MEM_64_BIT
>>> +    bool "Allow 64-bit memory transfers"
>>> +    default y if !AVR32
>>> +    depends on DW_DMAC
>>> +    help
>>> +      Say yes if the DMA controller may do 64-bit memory transfers
>>> +      For AVR32, say no because only up to 32-bit transfers are
>>> +      defined
>> Is this sane to add? Could some non-AVR32 platforms use 64-bit and 32-bit
>> depending on runtime configuration? E.g. if you build a kernel with support
>> for multiple boards/processors, and there is a mix of 32-bit and 64-bit wide
>> DMA support.
>>
>> I think it is better to select 32/64-bit at runtime.
>
> I did that in the first patch, adding a new property to the dw_dma_slave
> structure. It had the small disadvantage that some arch code had to be
> adapted (at32ap700x.c).
>
> Viresh, what do you think? Add a property called "mem_64_bit_access" or so?
>
> Or should it be passed as a member of 'dw_dma_platform_data', because it
> is a property of the (entire) DMA controller?

I think second option is better. But can there be some supportive scenarios of
first option?

We have a system, with two different memory controllers, one supporting 32 bit
and other 64 bit?

Or what we can do now is: go with option 2, i.e. update dw_dma_platform_data
and if some platform like what i mentioned above comes, then we can move it
to slave data.

viresh

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-08-28  3:23         ` Viresh Kumar
@ 2012-08-28  6:55           ` Hein Tibosch
  2012-08-28  7:05             ` Viresh Kumar
  2012-08-28  7:39             ` Felipe Balbi
  0 siblings, 2 replies; 11+ messages in thread
From: Hein Tibosch @ 2012-08-28  6:55 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Hans-Christian Egtvedt, spear-devel, Linux Kernel Mailing List,
	ludovic.desroches, Havard Skinnemoen, Nicolas Ferre,
	Andrew Morton, Arnd Bergmann

On 8/28/2012 11:23 AM, Viresh Kumar wrote:
> On 27 August 2012 20:28, Hein Tibosch <hein_tibosch@yahoo.es> wrote:
>>>> +config DW_DMAC_MEM_64_BIT
>>>> +    bool "Allow 64-bit memory transfers"
>>>> +    default y if !AVR32
>>>> +    depends on DW_DMAC
>>>> +    help
>>>> +      Say yes if the DMA controller may do 64-bit memory transfers
>>>> +      For AVR32, say no because only up to 32-bit transfers are
>>>> +      defined
>>> Is this sane to add? Could some non-AVR32 platforms use 64-bit and 32-bit
>>> depending on runtime configuration? E.g. if you build a kernel with support
>>> for multiple boards/processors, and there is a mix of 32-bit and 64-bit wide
>>> DMA support.
>>>
>>> I think it is better to select 32/64-bit at runtime.
>> I did that in the first patch, adding a new property to the dw_dma_slave
>> structure. It had the small disadvantage that some arch code had to be
>> adapted (at32ap700x.c).
>>
>> Viresh, what do you think? Add a property called "mem_64_bit_access" or so?
>>
>> Or should it be passed as a member of 'dw_dma_platform_data', because it
>> is a property of the (entire) DMA controller?
> I think second option is better. But can there be some supportive scenarios of
> first option?
>
> We have a system, with two different memory controllers, one supporting 32 bit
> and other 64 bit?
>
> Or what we can do now is: go with option 2, i.e. update dw_dma_platform_data
> and if some platform like what i mentioned above comes, then we can move it
> to slave data.
What about this:

In case of AVR32, the arch code will indicate:

static struct dw_dma_platform_data dw_dmac0_data = {
	.nr_channels    = 3,
	/* DMAC supports up to 32-bit memory access */
	.mem_access_32_bit_only = true,
};

ARM users won't have to change anything because mem_access_32_bit_only
will be false and therefor 'dw->mem_64_bit' will become true

Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>

---
 drivers/dma/dw_dmac.c      |   11 ++++++++---
 drivers/dma/dw_dmac_regs.h |    2 ++
 include/linux/dw_dmac.h    |    2 ++
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 7212961..a37053c 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -618,6 +618,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		*dw = to_dw_dma(dwc->chan.device);
 	struct dw_desc		*desc;
 	struct dw_desc		*first;
 	struct dw_desc		*prev;
@@ -639,7 +640,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 	 * We can be a lot more clever here, but this should take care
 	 * of the most common optimization.
 	 */
-	if (!((src | dest  | len) & 7))
+	if (dw->mem_64_bit && !((src | dest  | len) & 7))
 		src_width = dst_width = 3;
 	else if (!((src | dest  | len) & 3))
 		src_width = dst_width = 2;
@@ -710,6 +711,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
 	struct dw_dma_slave	*dws = chan->private;
 	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
+	struct dw_dma		*dw = to_dw_dma(dwc->chan.device);
 	struct dw_desc		*prev;
 	struct dw_desc		*first;
 	u32			ctllo;
@@ -746,7 +748,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 			mem = sg_dma_address(sg);
 			len = sg_dma_len(sg);
 
-			if (!((mem | len) & 7))
+			if (dw->mem_64_bit && !((mem | len) & 7))
 				mem_width = 3;
 			else if (!((mem | len) & 3))
 				mem_width = 2;
@@ -813,7 +815,7 @@ slave_sg_todev_fill_desc:
 			mem = sg_dma_address(sg);
 			len = sg_dma_len(sg);
 
-			if (!((mem | len) & 7))
+			if (dw->mem_64_bit && !((mem | len) & 7))
 				mem_width = 3;
 			else if (!((mem | len) & 3))
 				mem_width = 2;
@@ -1419,6 +1421,9 @@ static int __init dw_probe(struct platform_device *pdev)
 		goto err_kfree;
 	}
 
+	/* Remember if 64-bit access to memory is allowed */
+	dw->mem_64_bit = !pdata->mem_access_32_bit_only;
+
 	dw->regs = ioremap(io->start, DW_REGLEN);
 	if (!dw->regs) {
 		err = -ENOMEM;
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 9758651..e24562e 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -199,6 +199,8 @@ struct dw_dma {
 	struct clk		*clk;
 
 	u8			all_chan_mask;
+	/* 64-bit access to memory is allowed */
+	bool			mem_64_bit;
 
 	struct dw_dma_chan	chan[0];
 };
diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
index 2412e02..d01d63f 100644
--- a/include/linux/dw_dmac.h
+++ b/include/linux/dw_dmac.h
@@ -29,6 +29,8 @@ struct dw_dma_platform_data {
 #define CHAN_PRIORITY_ASCENDING		0	/* chan0 highest */
 #define CHAN_PRIORITY_DESCENDING	1	/* chan7 highest */
 	unsigned char	chan_priority;
+	/* Make true if 64-bit access to memory is not implemented */
+	bool		mem_access_32_bit_only;
 };
 
 /* bursts size */
-- 
1.7.8.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-08-28  6:55           ` Hein Tibosch
@ 2012-08-28  7:05             ` Viresh Kumar
  2012-08-28  7:39             ` Felipe Balbi
  1 sibling, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2012-08-28  7:05 UTC (permalink / raw)
  To: Hein Tibosch
  Cc: Hans-Christian Egtvedt, spear-devel, Linux Kernel Mailing List,
	ludovic.desroches, Havard Skinnemoen, Nicolas Ferre,
	Andrew Morton, Arnd Bergmann

On 28 August 2012 12:25, Hein Tibosch <hein_tibosch@yahoo.es> wrote:
> What about this:
>
> In case of AVR32, the arch code will indicate:
>
> static struct dw_dma_platform_data dw_dmac0_data = {
>         .nr_channels    = 3,
>         /* DMAC supports up to 32-bit memory access */
>         .mem_access_32_bit_only = true,
> };
>
> ARM users won't have to change anything because mem_access_32_bit_only
> will be false and therefor 'dw->mem_64_bit' will become true

I will go for the earlier solution that you sent: with max-mem-width.
0 = 64 bit, so nothing to be changed for ARM as global structures would be
getting initialized to zero
1=32 bit, pass this for AVR32.

--
viresh

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-08-28  6:55           ` Hein Tibosch
  2012-08-28  7:05             ` Viresh Kumar
@ 2012-08-28  7:39             ` Felipe Balbi
  2012-09-03  7:50               ` Andy Shevchenko
  1 sibling, 1 reply; 11+ messages in thread
From: Felipe Balbi @ 2012-08-28  7:39 UTC (permalink / raw)
  To: Hein Tibosch
  Cc: Viresh Kumar, Hans-Christian Egtvedt, spear-devel,
	Linux Kernel Mailing List, ludovic.desroches, Havard Skinnemoen,
	Nicolas Ferre, Andrew Morton, Arnd Bergmann

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

On Tue, Aug 28, 2012 at 02:55:35PM +0800, Hein Tibosch wrote:
> On 8/28/2012 11:23 AM, Viresh Kumar wrote:
> > On 27 August 2012 20:28, Hein Tibosch <hein_tibosch@yahoo.es> wrote:
> >>>> +config DW_DMAC_MEM_64_BIT
> >>>> +    bool "Allow 64-bit memory transfers"
> >>>> +    default y if !AVR32
> >>>> +    depends on DW_DMAC
> >>>> +    help
> >>>> +      Say yes if the DMA controller may do 64-bit memory transfers
> >>>> +      For AVR32, say no because only up to 32-bit transfers are
> >>>> +      defined
> >>> Is this sane to add? Could some non-AVR32 platforms use 64-bit and 32-bit
> >>> depending on runtime configuration? E.g. if you build a kernel with support
> >>> for multiple boards/processors, and there is a mix of 32-bit and 64-bit wide
> >>> DMA support.
> >>>
> >>> I think it is better to select 32/64-bit at runtime.
> >> I did that in the first patch, adding a new property to the dw_dma_slave
> >> structure. It had the small disadvantage that some arch code had to be
> >> adapted (at32ap700x.c).
> >>
> >> Viresh, what do you think? Add a property called "mem_64_bit_access" or so?
> >>
> >> Or should it be passed as a member of 'dw_dma_platform_data', because it
> >> is a property of the (entire) DMA controller?
> > I think second option is better. But can there be some supportive scenarios of
> > first option?
> >
> > We have a system, with two different memory controllers, one supporting 32 bit
> > and other 64 bit?
> >
> > Or what we can do now is: go with option 2, i.e. update dw_dma_platform_data
> > and if some platform like what i mentioned above comes, then we can move it
> > to slave data.
> What about this:
> 
> In case of AVR32, the arch code will indicate:
> 
> static struct dw_dma_platform_data dw_dmac0_data = {
> 	.nr_channels    = 3,
> 	/* DMAC supports up to 32-bit memory access */
> 	.mem_access_32_bit_only = true,
> };
> 
> ARM users won't have to change anything because mem_access_32_bit_only
> will be false and therefor 'dw->mem_64_bit' will become true
> 
> Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
> 
> ---
>  drivers/dma/dw_dmac.c      |   11 ++++++++---
>  drivers/dma/dw_dmac_regs.h |    2 ++
>  include/linux/dw_dmac.h    |    2 ++
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index 7212961..a37053c 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -618,6 +618,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		*dw = to_dw_dma(dwc->chan.device);
>  	struct dw_desc		*desc;
>  	struct dw_desc		*first;
>  	struct dw_desc		*prev;
> @@ -639,7 +640,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
>  	 * We can be a lot more clever here, but this should take care
>  	 * of the most common optimization.
>  	 */
> -	if (!((src | dest  | len) & 7))
> +	if (dw->mem_64_bit && !((src | dest  | len) & 7))
>  		src_width = dst_width = 3;
>  	else if (!((src | dest  | len) & 3))
>  		src_width = dst_width = 2;
> @@ -710,6 +711,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
>  	struct dw_dma_slave	*dws = chan->private;
>  	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
> +	struct dw_dma		*dw = to_dw_dma(dwc->chan.device);
>  	struct dw_desc		*prev;
>  	struct dw_desc		*first;
>  	u32			ctllo;
> @@ -746,7 +748,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  			mem = sg_dma_address(sg);
>  			len = sg_dma_len(sg);
>  
> -			if (!((mem | len) & 7))
> +			if (dw->mem_64_bit && !((mem | len) & 7))
>  				mem_width = 3;
>  			else if (!((mem | len) & 3))
>  				mem_width = 2;
> @@ -813,7 +815,7 @@ slave_sg_todev_fill_desc:
>  			mem = sg_dma_address(sg);
>  			len = sg_dma_len(sg);
>  
> -			if (!((mem | len) & 7))
> +			if (dw->mem_64_bit && !((mem | len) & 7))
>  				mem_width = 3;
>  			else if (!((mem | len) & 3))
>  				mem_width = 2;
> @@ -1419,6 +1421,9 @@ static int __init dw_probe(struct platform_device *pdev)
>  		goto err_kfree;
>  	}
>  
> +	/* Remember if 64-bit access to memory is allowed */
> +	dw->mem_64_bit = !pdata->mem_access_32_bit_only;
> +
>  	dw->regs = ioremap(io->start, DW_REGLEN);
>  	if (!dw->regs) {
>  		err = -ENOMEM;
> diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
> index 9758651..e24562e 100644
> --- a/drivers/dma/dw_dmac_regs.h
> +++ b/drivers/dma/dw_dmac_regs.h
> @@ -199,6 +199,8 @@ struct dw_dma {
>  	struct clk		*clk;
>  
>  	u8			all_chan_mask;
> +	/* 64-bit access to memory is allowed */
> +	bool			mem_64_bit;
>  
>  	struct dw_dma_chan	chan[0];
>  };
> diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
> index 2412e02..d01d63f 100644
> --- a/include/linux/dw_dmac.h
> +++ b/include/linux/dw_dmac.h
> @@ -29,6 +29,8 @@ struct dw_dma_platform_data {
>  #define CHAN_PRIORITY_ASCENDING		0	/* chan0 highest */
>  #define CHAN_PRIORITY_DESCENDING	1	/* chan7 highest */
>  	unsigned char	chan_priority;
> +	/* Make true if 64-bit access to memory is not implemented */
> +	bool		mem_access_32_bit_only;
>  };

Can you not read this from any internal register ? Synopsys generally
adds a set of read only registers which we can use to guess which
features where enabled in the IP when configuring it with their IP
configuration tool.

-- 
balbi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-08-28  7:39             ` Felipe Balbi
@ 2012-09-03  7:50               ` Andy Shevchenko
  2012-09-03 14:16                 ` Felipe Balbi
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2012-09-03  7:50 UTC (permalink / raw)
  To: balbi
  Cc: Hein Tibosch, Viresh Kumar, Hans-Christian Egtvedt, spear-devel,
	Linux Kernel Mailing List, ludovic.desroches, Havard Skinnemoen,
	Nicolas Ferre, Arnd Bergmann

On Tue, Aug 28, 2012 at 10:39 AM, Felipe Balbi <balbi@ti.com> wrote:
> Can you not read this from any internal register ? Synopsys generally
> adds a set of read only registers which we can use to guess which
> features where enabled in the IP when configuring it with their IP
> configuration tool.
You right. I have a patch to support few options like that already.
However, I can't share
it yet by some legal reasons.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
  2012-09-03  7:50               ` Andy Shevchenko
@ 2012-09-03 14:16                 ` Felipe Balbi
  0 siblings, 0 replies; 11+ messages in thread
From: Felipe Balbi @ 2012-09-03 14:16 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: balbi, Hein Tibosch, Viresh Kumar, Hans-Christian Egtvedt,
	spear-devel, Linux Kernel Mailing List, ludovic.desroches,
	Havard Skinnemoen, Nicolas Ferre, Arnd Bergmann

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

On Mon, Sep 03, 2012 at 10:50:44AM +0300, Andy Shevchenko wrote:
> On Tue, Aug 28, 2012 at 10:39 AM, Felipe Balbi <balbi@ti.com> wrote:
> > Can you not read this from any internal register ? Synopsys generally
> > adds a set of read only registers which we can use to guess which
> > features where enabled in the IP when configuring it with their IP
> > configuration tool.
> You right. I have a patch to support few options like that already.
> However, I can't share
> it yet by some legal reasons.

fair enough, in that case it'd be best to hold this patch and provide
something better later ;-)

-- 
balbi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-09-03 14:20 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-26 20:53 [PATCH 1/2] dw_dmac: make driver endianness configurable Hein Tibosch
2012-08-27  7:03 ` Hans-Christian Egtvedt
2012-08-27  8:47   ` Hein Tibosch
2012-08-27 11:14     ` Hans-Christian Egtvedt
2012-08-27 14:58       ` Hein Tibosch
2012-08-28  3:23         ` Viresh Kumar
2012-08-28  6:55           ` Hein Tibosch
2012-08-28  7:05             ` Viresh Kumar
2012-08-28  7:39             ` Felipe Balbi
2012-09-03  7:50               ` Andy Shevchenko
2012-09-03 14:16                 ` Felipe Balbi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).