Re: [PATCH] Fixes for dw_dmac and atmel-mci for AP700x

[Hein Tibosch <hein_tibosch@yahoo.es>]

On 8/21/2012 10:15 PM, Havard Skinnemoen wrote:
> On Tue, Aug 21, 2012 at 1:31 AM, Arnd Bergmann <arnd.bergmann@linaro.org> wrote:
>> On Tuesday 21 August 2012, Viresh Kumar wrote:
>>>> 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?
>>>> 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?
>> 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.
> 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.

If I may summarize: Viresh replaced the __raw* accessor functions with
readl/writel, because they will enforce a correct ordering of instructions
exchanged with the peripheral.

On the ARM configurations this worked ok, because both the device and the
CPU are little endian.

However, for AVR32 the driver got broken: readl/writel are little endian
accessors.

I looked up an earlier thread "MMIO and gcc re-ordering issue"*, in which
Havard wrote:

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

Four years later, we still don't have a generic native-endian MMIO-access
API which also introduce proper memory barriers.

So for the moment, do we have a better solution than this:

#ifdef CONFIG_AVR32
+#define dma_readl(dw, name) \
+	__raw_readl(&(__dw_regs(dw)->name))
+#define dma_writel(dw, name, val) \
+	__raw_writel((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

and do the same for channel_readl/writel ?

Hein

*http://linux.derkeiler.com/Mailing-Lists/Kernel/2008-05/msg13776.html