2012-10-14 15:26:16

by Hein_Tibosch

[permalink] [raw]
Subject: [PATCH REGRESSION FIX] dw_dmac: make driver's endianness configurable

From: Hein Tibosch <[email protected]>

The dw_dmac was originally developed for avr32 to be used with the Synopsys
DesignWare AHB DMA controller. Starting from 2.6.38, access to the device's i/o
memory was done with the little-endian readl/writel functions(1)

This broke the driver for the avr32 platform, because it needs big (native)
endian accessors.
This patch makes the endianness configurable using 'DW_DMAC_BIG_ENDIAN_IO',
which will default be true for AVR32

I submitted this patch before(2) but then waited for Andy to finish other
changes to the same module(3).

(1) https://patchwork.kernel.org/patch/608211
(2) https://lkml.org/lkml/2012/8/26/148
(3) https://lkml.org/lkml/2012/9/21/173

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

---
drivers/dma/Kconfig | 11 +++++++++++
drivers/dma/dw_dmac_regs.h | 18 +++++++++++++-----
2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 677cd6e..d4c1218 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -90,6 +90,17 @@ config DW_DMAC
Support the Synopsys DesignWare AHB DMA controller. This
can be integrated in chips such as the Atmel AT32ap7000.

+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.
+
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 ff39fa6..8896559 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -98,9 +98,17 @@ struct dw_dma_regs {
u32 DW_PARAMS;
};

+#ifdef CONFIG_DW_DMAC_BIG_ENDIAN_IO
+#define dma_readl_native ioread32be
+#define dma_writel_native iowrite32be
+#else
+#define dma_readl_native readl
+#define dma_writel_native writel
+#endif
+
/* To access the registers in early stage of probe */
#define dma_read_byaddr(addr, name) \
- readl((addr) + offsetof(struct dw_dma_regs, name))
+ dma_readl_native((addr) + offsetof(struct dw_dma_regs, name))

/* Bitfields in DW_PARAMS */
#define DW_PARAMS_NR_CHAN 8 /* number of channels */
@@ -216,9 +224,9 @@ __dwc_regs(struct dw_dma_chan *dwc)
}

#define channel_readl(dwc, name) \
- readl(&(__dwc_regs(dwc)->name))
+ dma_readl_native(&(__dwc_regs(dwc)->name))
#define channel_writel(dwc, name, val) \
- writel((val), &(__dwc_regs(dwc)->name))
+ dma_writel_native((val), &(__dwc_regs(dwc)->name))

static inline struct dw_dma_chan *to_dw_dma_chan(struct dma_chan *chan)
{
@@ -246,9 +254,9 @@ static inline struct dw_dma_regs __iomem *__dw_regs(struct dw_dma *dw)
}

#define dma_readl(dw, name) \
- readl(&(__dw_regs(dw)->name))
+ dma_readl_native(&(__dw_regs(dw)->name))
#define dma_writel(dw, name, val) \
- writel((val), &(__dw_regs(dw)->name))
+ dma_writel_native((val), &(__dw_regs(dw)->name))

#define channel_set_bit(dw, reg, mask) \
dma_writel(dw, reg, ((mask) << 8) | (mask))
--
1.7.8.0



2012-10-14 19:09:57

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH REGRESSION FIX] dw_dmac: make driver's endianness configurable

On Sunday 14 October 2012, Hein Tibosch wrote:
> From: Hein Tibosch <[email protected]>
>
> The dw_dmac was originally developed for avr32 to be used with the Synopsys
> DesignWare AHB DMA controller. Starting from 2.6.38, access to the device's i/o
> memory was done with the little-endian readl/writel functions(1)
>
> This broke the driver for the avr32 platform, because it needs big (native)
> endian accessors.
> This patch makes the endianness configurable using 'DW_DMAC_BIG_ENDIAN_IO',
> which will default be true for AVR32
>
> I submitted this patch before(2) but then waited for Andy to finish other
> changes to the same module(3).
>
> (1) https://patchwork.kernel.org/patch/608211
> (2) https://lkml.org/lkml/2012/8/26/148
> (3) https://lkml.org/lkml/2012/9/21/173
>
> Signed-off-by: Hein Tibosch <[email protected]>

Acked-by: Arnd Bergmann <[email protected]>

2012-10-14 20:08:32

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH REGRESSION FIX] dw_dmac: make driver's endianness configurable

On Sun, Oct 14, 2012 at 10:54 AM, Hein Tibosch <[email protected]> wrote:
> From: Hein Tibosch <[email protected]>
>
> The dw_dmac was originally developed for avr32 to be used with the Synopsys
> DesignWare AHB DMA controller. Starting from 2.6.38, access to the device's i/o
> memory was done with the little-endian readl/writel functions(1)
>
> This broke the driver for the avr32 platform, because it needs big (native)
> endian accessors.
> This patch makes the endianness configurable using 'DW_DMAC_BIG_ENDIAN_IO',
> which will default be true for AVR32
>
> I submitted this patch before(2) but then waited for Andy to finish other
> changes to the same module(3).
>
> (1) https://patchwork.kernel.org/patch/608211
> (2) https://lkml.org/lkml/2012/8/26/148
> (3) https://lkml.org/lkml/2012/9/21/173
>
> Signed-off-by: Hein Tibosch <[email protected]>
>
> ---
> drivers/dma/Kconfig | 11 +++++++++++
> drivers/dma/dw_dmac_regs.h | 18 +++++++++++++-----
> 2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 677cd6e..d4c1218 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -90,6 +90,17 @@ config DW_DMAC
> Support the Synopsys DesignWare AHB DMA controller. This
> can be integrated in chips such as the Atmel AT32ap7000.
>
> +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.
> +
> 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 ff39fa6..8896559 100644
> --- a/drivers/dma/dw_dmac_regs.h
> +++ b/drivers/dma/dw_dmac_regs.h
> @@ -98,9 +98,17 @@ struct dw_dma_regs {
> u32 DW_PARAMS;
> };
>
> +#ifdef CONFIG_DW_DMAC_BIG_ENDIAN_IO
> +#define dma_readl_native ioread32be
> +#define dma_writel_native iowrite32be
> +#else
> +#define dma_readl_native readl
> +#define dma_writel_native writel
> +#endif
> +
> /* To access the registers in early stage of probe */
> #define dma_read_byaddr(addr, name) \
> - readl((addr) + offsetof(struct dw_dma_regs, name))
> + dma_readl_native((addr) + offsetof(struct dw_dma_regs, name))
>
> /* Bitfields in DW_PARAMS */
> #define DW_PARAMS_NR_CHAN 8 /* number of channels */
> @@ -216,9 +224,9 @@ __dwc_regs(struct dw_dma_chan *dwc)
> }
>
> #define channel_readl(dwc, name) \
> - readl(&(__dwc_regs(dwc)->name))
> + dma_readl_native(&(__dwc_regs(dwc)->name))
> #define channel_writel(dwc, name, val) \
> - writel((val), &(__dwc_regs(dwc)->name))
> + dma_writel_native((val), &(__dwc_regs(dwc)->name))
>
> static inline struct dw_dma_chan *to_dw_dma_chan(struct dma_chan *chan)
> {
> @@ -246,9 +254,9 @@ static inline struct dw_dma_regs __iomem *__dw_regs(struct dw_dma *dw)
> }
>
> #define dma_readl(dw, name) \
> - readl(&(__dw_regs(dw)->name))
> + dma_readl_native(&(__dw_regs(dw)->name))
> #define dma_writel(dw, name, val) \
> - writel((val), &(__dw_regs(dw)->name))
> + dma_writel_native((val), &(__dw_regs(dw)->name))
>
> #define channel_set_bit(dw, reg, mask) \
> dma_writel(dw, reg, ((mask) << 8) | (mask))
Why did you not change this one?


--
With Best Regards,
Andy Shevchenko

2012-10-15 01:28:26

by Hein_Tibosch

[permalink] [raw]
Subject: Re: [PATCH REGRESSION FIX] dw_dmac: make driver's endianness configurable

Hi Andy,
On 10/15/2012 4:08 AM, Andy Shevchenko wrote:
> On Sun, Oct 14, 2012 at 10:54 AM, Hein Tibosch <[email protected]> wrote:
>> From: Hein Tibosch <[email protected]>
>>
>> The dw_dmac was originally developed for avr32 to be used with the Synopsys
>> DesignWare AHB DMA controller. Starting from 2.6.38, access to the device's i/o
>> <snip>
>> #define dma_readl(dw, name) \
>> - readl(&(__dw_regs(dw)->name))
>> + dma_readl_native(&(__dw_regs(dw)->name))
>> #define dma_writel(dw, name, val) \
>> - writel((val), &(__dw_regs(dw)->name))
>> + dma_writel_native((val), &(__dw_regs(dw)->name))
>>
>> #define channel_set_bit(dw, reg, mask) \
>> dma_writel(dw, reg, ((mask) << 8) | (mask))
> Why did you not change this one?

Because "dma_writel" already calls "dma_writel_native"

2012-10-15 03:19:49

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH REGRESSION FIX] dw_dmac: make driver's endianness configurable

On Sun, Oct 14, 2012 at 1:24 PM, Hein Tibosch <[email protected]> wrote:
> From: Hein Tibosch <[email protected]>
>
> The dw_dmac was originally developed for avr32 to be used with the Synopsys
> DesignWare AHB DMA controller. Starting from 2.6.38, access to the device's i/o
> memory was done with the little-endian readl/writel functions(1)
>
> This broke the driver for the avr32 platform, because it needs big (native)
> endian accessors.
> This patch makes the endianness configurable using 'DW_DMAC_BIG_ENDIAN_IO',
> which will default be true for AVR32
>
> I submitted this patch before(2) but then waited for Andy to finish other
> changes to the same module(3).
>
> (1) https://patchwork.kernel.org/patch/608211
> (2) https://lkml.org/lkml/2012/8/26/148
> (3) https://lkml.org/lkml/2012/9/21/173
>
> Signed-off-by: Hein Tibosch <[email protected]>

Good.

Acked-by: Viresh Kumar <[email protected]>

2012-10-15 07:32:01

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH REGRESSION FIX] dw_dmac: make driver's endianness configurable

On Mon, 2012-10-15 at 08:39 +0800, Hein Tibosch wrote:
> Hi Andy,
> On 10/15/2012 4:08 AM, Andy Shevchenko wrote:
> > On Sun, Oct 14, 2012 at 10:54 AM, Hein Tibosch <[email protected]> wrote:
> >> From: Hein Tibosch <[email protected]>
> >>
> >> The dw_dmac was originally developed for avr32 to be used with the Synopsys
> >> DesignWare AHB DMA controller. Starting from 2.6.38, access to the device's i/o
> >> <snip>
> >> #define dma_readl(dw, name) \
> >> - readl(&(__dw_regs(dw)->name))
> >> + dma_readl_native(&(__dw_regs(dw)->name))
> >> #define dma_writel(dw, name, val) \
> >> - writel((val), &(__dw_regs(dw)->name))
> >> + dma_writel_native((val), &(__dw_regs(dw)->name))
> >>
> >> #define channel_set_bit(dw, reg, mask) \
> >> dma_writel(dw, reg, ((mask) << 8) | (mask))
> > Why did you not change this one?
>
> Because "dma_writel" already calls "dma_writel_native"
Sure,
looks good for me.

--
Andy Shevchenko <[email protected]>
Intel Finland Oy

2012-10-23 23:12:31

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH REGRESSION FIX] dw_dmac: make driver's endianness configurable

On Sun, 14 Oct 2012 15:54:13 +0800
Hein Tibosch <[email protected]> wrote:

> The dw_dmac was originally developed for avr32 to be used with the Synopsys
> DesignWare AHB DMA controller. Starting from 2.6.38, access to the device's i/o
> memory was done with the little-endian readl/writel functions(1)
>
> This broke the driver for the avr32 platform, because it needs big (native)
> endian accessors.
> This patch makes the endianness configurable using 'DW_DMAC_BIG_ENDIAN_IO',
> which will default be true for AVR32

Do we think this bug should be fixed in earlier kernel versions?

If so, the patch might need to be tweaked for 3.6 and earlier, which
don't have the dma_read_byaddr() definition.

2012-10-24 01:36:21

by Hein_Tibosch

[permalink] [raw]
Subject: Re: [PATCH REGRESSION FIX] dw_dmac: make driver's endianness configurable

Hi Andrew,

On 10/24/2012 7:12 AM, Andrew Morton wrote:
> On Sun, 14 Oct 2012 15:54:13 +0800
> Hein Tibosch <[email protected]> wrote:
>
>> The dw_dmac was originally developed for avr32 to be used with the Synopsys
>> DesignWare AHB DMA controller. Starting from 2.6.38, access to the device's i/o
>> memory was done with the little-endian readl/writel functions(1)
>>
>> This broke the driver for the avr32 platform, because it needs big (native)
>> endian accessors.
>> This patch makes the endianness configurable using 'DW_DMAC_BIG_ENDIAN_IO',
>> which will default be true for AVR32
> Do we think this bug should be fixed in earlier kernel versions?
>
> If so, the patch might need to be tweaked for 3.6 and earlier, which
> don't have the dma_read_byaddr() definition.

The 'bug' only affected avr32 (AP700x) users. I think there won't be much demand
for it. Beside that, there were more breakages after 2.6.38, which have only
recently been fixed in 3.7-rc1

These patches should then be back-ported as well:

http://www.spinics.net/lists/linux-mmc/msg16104.html
http://www.gossamer-threads.com/lists/linux/kernel/1603537

Thanks, Hein