Fixes for dw_dmac and atmel-mci for AP700x

Hi Hans-Christian,

Do you know to which list I could send the patch(es) below?

Viresh Kumar: did you have an urgent reason to replace __raw_readl and __raw_writel with readl/writel in the dw_dmac module?
(see http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/044799.html)

Here's a draft:

Because of the changes since 2.6.38 to dw_dmac.c and atmel-mci.c, the MCI driver on the AP700x platforms didn't work anymore using DMA. The PIO method still worked well.

I found several problems. The patch below makes it work again. It was tested on an atngw100 board.

atmel-mci:
- The AP700x has an MCI controller with version 0x210 Therefor it was supposed to work the PDC (peripheral DMA controller). However, in this CPU the PDC is not connected to the MCI.
- When using DMA, the driver was accessing a register ATMCI_DMA at offset 0x50, which is undefined for the AP700x. But it probably doesn't hurt either...

dw_dmac:
- After 2.6.39, the registers were accessed using readl/writel in stead of the __raw_readl and __raw_writel causing a 16-bit swap of all values (little endian access)

- Access to memory was sometimes done in chunks of 64-bits, which gives an undefined value of 0x03 for SRC/DST_TR_WIDTH field in the CTLxL register

- The SMS field in the CTLxL register received the wrong value:
0 in stead of 1

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

---
 arch/avr32/mach-at32ap/at32ap700x.c |    6 ++++++
 drivers/dma/dw_dmac.c               |    8 ++++++++
 drivers/dma/dw_dmac_regs.h          |    8 ++++----
 drivers/mmc/host/atmel-mci.c        |   19 ++++++++++++++++---
 4 files changed, 34 insertions(+), 7 deletions(-)

arch/avr32/mach-at32ap/at32ap700x.c +6 -0

index 0445c4f..06b6eff 100644
@@ -1355,6 +1355,12 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
                                | DWC_CFGH_DST_PER(1));
        slave->sdata.cfg_lo &= ~(DWC_CFGL_HS_DST_POL
                                | DWC_CFGL_HS_SRC_POL);
+       /* value for SMS: Source Master Select */
+       slave->sdata.src_master = 1;
+       /* value for DMS: Destination Master Select */
+       slave->sdata.dst_master = 0;
+       /* SRC/DST_TR_WIDTH register only accepts 0,1,2 */
+       slave->sdata.max_mem_width = 2;
        data->dma_slave = slave;
 diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

index 7212961..a4bdf1d 100644

drivers/dma/dw_dmac.c +8 -0

@@ -754,6 +754,10 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
                                mem_width = 1;
                        else
                                mem_width = 0;
+                       /* Some controllers don't support 64-bits mem access */
+                       if (dws->max_mem_width &&
+                               mem_width > dws->max_mem_width)
+                               mem_width = dws->max_mem_width;
  slave_sg_todev_fill_desc:
                        desc = dwc_desc_get(dwc);

@@ -821,6 +825,10 @@ slave_sg_todev_fill_desc:
                                mem_width = 1;
                        else
                                mem_width = 0;
+                       /* Some controllers don't support 64-bits mem access */
+                       if (dws->max_mem_width &&
+                               mem_width > dws->max_mem_width)
+                               mem_width = dws->max_mem_width;
  slave_sg_fromdev_fill_desc:
                        desc = dwc_desc_get(dwc);

drivers/dma/dw_dmac_regs.h +4 -4

index f298f69..b380d5f 100644
@@ -176,9 +176,9 @@ __dwc_regs(struct dw_dma_chan *dwc)
 }
  #define channel_readl(dwc, name) \
-       readl(&(__dwc_regs(dwc)->name))
+       __raw_readl(&(__dwc_regs(dwc)->name))
 #define channel_writel(dwc, name, val) \
-       writel((val), &(__dwc_regs(dwc)->name))
+       __raw_writel((val), &(__dwc_regs(dwc)->name))
  static inline struct dw_dma_chan *to_dw_dma_chan(struct dma_chan *chan)
 {

@@ -202,9 +202,9 @@ static inline struct dw_dma_regs __iomem *__dw_regs(struct dw_dma *dw)
 }
  #define dma_readl(dw, name) \
-       readl(&(__dw_regs(dw)->name))
+       __raw_readl(&(__dw_regs(dw)->name))
 #define dma_writel(dw, name, val) \
-       writel((val), &(__dw_regs(dw)->name))
+       __raw_writel((val), &(__dw_regs(dw)->name))
  #define channel_set_bit(dw, reg, mask) \
        dma_writel(dw, reg, ((mask) << 8) | (mask))

drivers/mmc/host/atmel-mci.c +16 -1

index f2c115e..b78a674 100644
@@ -75,6 +75,7 @@ struct atmel_mci_caps {
        bool    has_pdc;
        bool    has_cfg_reg;
        bool    has_cstor_reg;
+       bool    has_dma_reg;
        bool    has_highspeed;
        bool    has_rwproof;
        bool    has_odd_clk_div;

@@ -411,7 +412,7 @@ static int atmci_regs_show(struct seq_file *s, void *v)
        atmci_show_status_reg(s, "SR", buf[ATMCI_SR / 4]);
        atmci_show_status_reg(s, "IMR", buf[ATMCI_IMR / 4]);
 -      if (host->caps.has_dma) {
+       if (host->caps.has_dma_reg) {
                u32 val;
                val = buf[ATMCI_DMA / 4];

@@ -767,7 +768,7 @@ static void atmci_dma_complete(void *arg)
        dev_vdbg(&host->pdev->dev, "DMA complete\n");
 -      if (host->caps.has_dma)
+       if (host->caps.has_dma_reg)
                /* Disable DMA hardware handshaking on MCI */
                atmci_writel(host, ATMCI_DMA, atmci_readl(host, ATMCI_DMA) & ~ATMCI_DMAEN);
 @@ -954,7 +955,9 @@ atmci_prepare_data_dma(struct atmel_mci *host, struct mmc_data *data)
                maxburst = atmci_convert_chksize(host->dma_conf.dst_maxburst);
        }
 -      atmci_writel(host, ATMCI_DMA, ATMCI_DMA_CHKSIZE(maxburst) | ATMCI_DMAEN);
+       if (host->caps.has_dma_reg)
+               atmci_writel(host, ATMCI_DMA,
+                       ATMCI_DMA_CHKSIZE(maxburst) | ATMCI_DMAEN);
        sglen = dma_map_sg(chan->device->dev, data->sg,
                        data->sg_len, direction);

@@ -2203,7 +2206,11 @@ static void __init atmci_get_cap(struct atmel_mci *host)
                        "version: 0x%x\n", version);
        host->caps.has_dma = 0;
+#if defined(CONFIG_CPU_AT32AP700X)
+       host->caps.has_pdc = 0;
+#else
        host->caps.has_pdc = 1;
+#endif
        host->caps.has_cfg_reg = 0;
        host->caps.has_cstor_reg = 0;
        host->caps.has_highspeed = 0;

@@ -2221,6 +2228,7 @@ static void __init atmci_get_cap(struct atmel_mci *host)
        case 0x300:
 #ifdef CONFIG_AT_HDMAC
                host->caps.has_dma = 1;
+               host->caps.has_dma_reg = 1;
 #else
                dev_info(&host->pdev->dev,
                        "has dma capability but dma engine is not selected, then use pio\n");

@@ -2230,6 +2238,11 @@ static void __init atmci_get_cap(struct atmel_mci *host)
                host->caps.has_cstor_reg = 1;
                host->caps.has_highspeed = 1;
        case 0x200:
+#if defined(CONFIG_DW_DMAC) && defined(CONFIG_MMC_ATMELMCI_DMA)
+               host->caps.has_dma = 1;
+               dev_info(&host->pdev->dev,
+                       "using dma (AP7000)\n");
+#endif
                host->caps.has_rwproof = 1;
                host->caps.need_blksz_mul_4 = 0;
        case 0x100:
--

1.7.8.0

I have added linux-kernel list in cc as there might be other users of this patch.
Also, please try to keep spear-devel list in cc for dw_dmac as we are using this driver for SPEAr.

Quoted 4 lines

dw_dmac:
- After 2.6.39, the registers were accessed using readl/writel in stead of the __raw_readl and __raw_writel causing a 16-bit swap of all values (little endian access)

Ahhhh!! Firstly we can't use __raw* for architectures >= ARMv6. It is not only for endianess but for memory barriers. Why are they getting swapped for your case? Does your processor and dw_dmac have different endianess?

And if i am not wrong, we should always try not to use __raw* variants just due to endianess things... instead use either readl/writel OR readl_/writel_ relaxed.
I am not sure if relaxed versions are available for architectures other than ARM.

Quoted 3 lines

- Access to memory was sometimes done in chunks of 64-bits, which gives an undefined value of 0x03 for SRC/DST_TR_WIDTH field in the CTLxL register

Looks fine. But there should be a separate patch for this.

Quoted 2 lines

- The SMS field in the CTLxL register received the wrong value:
0 in stead of 1

I believe it is not for dw_dmac?

On 21 August 2012 11:42, Hein Tibosch <[email protected] <mailto:[email protected]>> wrote:

On 8/21/2012 12:42 PM, viresh kumar wrote:
> Ahhhh!! Firstly we can't use __raw* for architectures >= ARMv6. It is not > only for endianess but for memory barriers. Why are they getting swapped > for your case? Does your processor and dw_dmac have different endianess?

If I'm not wrong: the __raw_* functions will access the i/o memory in native endianess. As far as I know, all AVR32 drivers are currently using the __raw* functions. I never encountered a problem with that.

I don't have the best knowledge in this area :( and so cc-ing Arnd who can help us here. So my perception of these routines is:

readl is defined generically as:

#define readl(addr) __le32_to_cpu(__raw_readl(addr))

Which converts to a simple __raw_readl() for little endian systems and swapped bytes for a big endian system.

You wrote in the beginning

Quoted 3 lines

Quoted 3 lines

- After 2.6.39, the registers were accessed using readl/writel in stead of the __raw_readl and __raw_writel causing a 16-bit swap of all values (little endian access)

What's this 16-bit swap here? It should be a 8 bit swap by __swab32() routine.

Is AVR32 a big-endian system? Probably big-endian, that's why values are getting swapped. And dw_dmac is the standard one, can call it little endian for the time being.

@Arnd: What should we do here?

Quoted 11 lines · Hein Tibosch

Quoted 3 lines · viresh kumar

Ahhhh!! Firstly we can't use __raw* for architectures >= ARMv6. It is not only for endianess but for memory barriers. Why are they getting swapped for your case? Does your processor and dw_dmac have different endianess?

If I'm not wrong: the __raw_* functions will access the i/o memory in native endianess.
As far as I know, all AVR32 drivers are currently using the __raw* functions. I never
encountered a problem with that.

I don't have the best knowledge in this area :( and so cc-ing Arnd who can help us here.
So my perception of these routines is:

readl is defined generically as:

#define readl(addr) __le32_to_cpu(__raw_readl(addr))

Which converts to a simple __raw_readl() for little endian systems and swapped bytes for a big endian system.

You wrote in the beginning

Quoted 3 lines

Quoted 3 lines

- After 2.6.39, the registers were accessed using readl/writel in stead of the __raw_readl and __raw_writel causing a 16-bit swap of all values (little endian access)

What's this 16-bit swap here? It should be a 8 bit swap by __swab32() routine.

Is AVR32 a big-endian system? Probably big-endian, that's why values are getting
swapped. And dw_dmac is the standard one, can call it little endian for the time being.

@Arnd: What should we do here?

Quoted 12 lines

Is AVR32 a big-endian system? Probably big-endian, that's why values are

Quoted 2 lines

getting
swapped. And dw_dmac is the standard one, can call it little endian for

the

Quoted 3 lines

time being.

@Arnd: What should we do here?

Yes, AVR32 is big-endian. I assume that dw_dmac can be either configured as little-endian or big-endian and that it is configured as big-endian on AVR32.

Just to understand a bit more on this always confusing endianess concept:
- For AVR32, readl is calling swab everytime. So whatever we write will get swapped.
- What are the implications of dw_dmac configured in little/big endian?

When we write something to register of a peripheral, whose endianess property decides how it will get written. Processor or Peripheral?

Quoted 4 lines · Hein Tibosch

On 8/21/2012 2:35 PM, Viresh Kumar wrote:

It got swapped as 0xAABB.CCDD => 0xCCDD.AABB

@Arnd: How do we explain this? shouldn't it be DD CC BB AA??

Quoted 5 lines

Memory barriers: within the AVR32 code, one often sees explicit ways to introduce memory barriers, e.g.:

    hsmc_readl(hsmc, MODE0); /* I/O barrier */

For ARM it has become a bit complex about using barriers. That's why they are added in readl/writel to remove any confusion.

Quoted 2 lines · Arnd Bergmann

Yes, this is very strange. Maybe the compiler already splits the access into two 16-byte loads and that confuses the device?

@Arnd: Is compiler allowed to do that even when we have volatile specified for the access? It shouldn't optimize the access at all i believe.

Quoted 7 lines · Arnd Bergmann

Quoted 6 lines · Viresh Kumar

Quoted 2 lines · Arnd Bergmann

Yes, this is very strange. Maybe the compiler already splits the access into two 16-byte loads and that confuses the device?

@Arnd: Is compiler allowed to do that even when we have volatile

Quoted 8 lines

specified

Quoted 1 line

for the access? It shouldn't optimize the access at all i believe.

Yes. The "volatile" keyword implies that the compiler has to do the access exactly once and that it cannot reorder the access with others on the same address, or read more data than is specified, but it can (and does) split the access if there is a reason for that, e.g. when the address might be misaligned and the architecture cannot do misaligned loads.

But that can't be the case here. Isn't it? So we shouldn't have got such results.

Quoted 13 lines · Viresh Kumar

Quoted 12 lines

Is AVR32 a big-endian system? Probably big-endian, that's why values are

Quoted 2 lines

getting
swapped. And dw_dmac is the standard one, can call it little endian for

the

Quoted 3 lines

time being.

@Arnd: What should we do here?

Yes, AVR32 is big-endian. I assume that dw_dmac can be either configured as little-endian or big-endian and that it is configured as big-endian on AVR32.

Quoted 7 lines

Just to understand a bit more on this always confusing endianess concept:
- For AVR32, readl is calling swab everytime. So whatever we write will get swapped.
- What are the implications of dw_dmac configured in little/big endian?

When we write something to register of a peripheral, whose endianess property decides how it will get written. Processor or Peripheral?

The device decides which accessor we need to use (readl, ioread, ioread_be, in_be, in_le, ...). The architecture code must ensure that this is implemented properly based on the CPU endianess. We don't have a proper accessor function that implements "device has same endianess as CPU". Using __raw_* is not a replacement for that.

I don't mind adding such an accessor at all, and a number people have complained about the lack of this for some time, but you should be aware that a lot of peripherals that are intended to be used in "CPU-endian" mode eventually end up getting used in "wrong-endian" mode, e.g. when someone decides to put that peripheral on a PCI card and someone else sticks it into a machine that has a CPU with the opposed (or configurable) endianess. It would be nice if the likes of designware could at least pick one endianess per device they do, but the reality is that we have to deal with both variants and only the device driver can find out what it is.

It should be easy to tell from the object code whether this happened or not. If it did, then we can investigate why gcc did that, otherwise something else caused the strange byte swap.

The safe way to define the readl() function in asm/io.h is to use an inline assembly that prevents the access from getting split, but avr32 just uses a pointer dereference here.

I think I just found the answer elsewhere in arch/avr32/mach-at32ap/include/mach/io.h, which defines

# define __mem_ioswabl(a, x)    swahb32(x)

and that apparently does the halfword swap when CONFIG_AP700X_16_BIT_SMC is set. This explains why Havard said it's wrong to use readl on internal deviceson avr32, but unfortunately that rule conflicts with how we define the accessors on ARM.

Quoted 25 lines · Arnd Bergmann

Quoted 11 lines · Viresh Kumar

Quoted 11 lines

Is AVR32 a big-endian system? Probably big-endian, that's why values are

Quoted 2 lines

getting
swapped. And dw_dmac is the standard one, can call it little endian for

the

Quoted 3 lines

time being.

@Arnd: What should we do here?

Yes, AVR32 is big-endian. I assume that dw_dmac can be either configured as little-endian or big-endian and that it is configured as big-endian on AVR32.

Quoted 7 lines

Just to understand a bit more on this always confusing endianess concept:
- For AVR32, readl is calling swab everytime. So whatever we write will get swapped.
- What are the implications of dw_dmac configured in little/big endian?

When we write something to register of a peripheral, whose endianess property decides how it will get written. Processor or Peripheral?

The device decides which accessor we need to use (readl, ioread, ioread_be, in_be, in_le, ...). The architecture code must ensure that this is implemented properly based on the CPU endianess. We don't have a proper accessor function that implements "device has same endianess as CPU". Using __raw_* is not a replacement for that.

Quoted 10 lines

I don't mind adding such an accessor at all, and a number people have complained about the lack of this for some time, but you should be aware that a lot of peripherals that are intended to be used in "CPU-endian" mode eventually end up getting used in "wrong-endian" mode, e.g. when someone decides to put that peripheral on a PCI card and someone else sticks it into a machine that has a CPU with the opposed (or configurable) endianess. It would be nice if the likes of designware could at least pick one endianess per device they do, but the reality is that we have to deal with both variants and only the device driver can find out what it is.

A native-endian accessor would really help in this case. Back in 2006 when I did the AVR32 port, that's what I thought __raw_ was, but I suspect I was both wrong, and the semantics of the I/O accessors have changed slightly over time. But since __raw_ did exactly what was needed on both AVR32 (big endian) and AT91 ARM (little endian) devices, I ended up using it for all on-chip devices.

read[bwl] on the other hand was implemented for external device access. Since the SMC controller on AP7000 is really bizarre (basically big endian data with little endian addressing), these accessors became pretty weird too.

Anyway, if we want to purge all those inappropriate __raw accesses from the Atmel drivers, we're going to need a replacement for internal native-endian access. Right now, we don't have any.

<cut> I don't think adding barriers is the
right thing to do because they won't do anything useful in practice, so it's hard to tell whether they are used "correctly". And it will hurt performance at least on AVR32 since wmb() evaluates to a "sync" instruction, which flushes the write buffer to RAM. Since MMIO writes are unbuffered, that's pure overhead.