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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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
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
On 27 August 2012 20:28, Hein Tibosch <[email protected]> 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
On 28 August 2012 12:25, Hein Tibosch <[email protected]> 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
On 8/28/2012 11:23 AM, Viresh Kumar wrote:
> On 27 August 2012 20:28, Hein Tibosch <[email protected]> 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 <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
>
> ---
> 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