[v4,2/2] dma/imx-sdma: convert _raw_readl/_raw_writel to readl/writel

readl/writel is more genric. And if CONFIG_ARM_DMA_MEM_BUFFERABLE,
they includes necessary memory barriers.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 drivers/dma/imx-sdma.c |   44 ++++++++++++++++++++++----------------------
 1 files changed, 22 insertions(+), 22 deletions(-)

On Sun, Jan 08, 2012 at 10:44:22PM +0800, Richard Zhao wrote:
> readl/writel is more genric. And if CONFIG_ARM_DMA_MEM_BUFFERABLE,
> they includes necessary memory barriers.
> 
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Regards,
Shawn

On Sun, Jan 08, 2012 at 10:44:22PM +0800, Richard Zhao wrote:
> readl/writel is more genric. And if CONFIG_ARM_DMA_MEM_BUFFERABLE,
> they includes necessary memory barriers.

In a DMA engine driver, you need to use the barrier accessors when:

1. You finally enable the DMA engine to perform a transfer.
   The included barrier ensures that writes to the descriptors are visible
   to the DMA engine.

2. You read from a status register before examining the descriptors.
   This ensures that the descriptor accesses won't be ordered before the
   status register read.

Provided other accesses are within the same 1K region, the remainder of
them do not have to be the strictly ordered accessors, and you can use
the _relaxed variants (but only in ARM specific drivers.)

So, if your DMA engine has a control register, and a descriptor pointer
register, you can write the descriptor pointer register with a
writel_relaxed().  When you write the control register to enable the
transfer, use writel() to ensure there's a barrier so the descriptors
are visible.

On Mon, Jan 9, 2012 at 7:51 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Jan 08, 2012 at 10:44:22PM +0800, Richard Zhao wrote:
>> readl/writel is more genric. And if CONFIG_ARM_DMA_MEM_BUFFERABLE,
>> they includes necessary memory barriers.
>
> In a DMA engine driver, you need to use the barrier accessors when:
>
> 1. You finally enable the DMA engine to perform a transfer.
>   The included barrier ensures that writes to the descriptors are visible
>   to the DMA engine.
>
> 2. You read from a status register before examining the descriptors.
>   This ensures that the descriptor accesses won't be ordered before the
>   status register read.
>
> Provided other accesses are within the same 1K region, the remainder of
> them do not have to be the strictly ordered accessors, and you can use
> the _relaxed variants (but only in ARM specific drivers.)

Russell,

Does this also mean when endian conversion is not necessary, the __raw_*
version will be better here? Or generally the _relaxed variants are more
recommended as endian conversion will be optimized away anyway with
these AMBA accesses as both sides are little-endian?

>
> So, if your DMA engine has a control register, and a descriptor pointer
> register, you can write the descriptor pointer register with a
> writel_relaxed().  When you write the control register to enable the
> transfer, use writel() to ensure there's a barrier so the descriptors
> are visible.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Jan 09, 2012 at 09:25:12PM +0800, Eric Miao wrote:
> Does this also mean when endian conversion is not necessary, the __raw_*
> version will be better here? Or generally the _relaxed variants are more
> recommended as endian conversion will be optimized away anyway with
> these AMBA accesses as both sides are little-endian?

Useless endian conversions are always optimized away.  Here's the
definitions:

If your CPU is operating in little endian mode, for 32-bit and 16-bit:

#define __cpu_to_le32(x) ((__force __le32)(__u32)(x))
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
#define __cpu_to_le16(x) ((__force __le16)(__u16)(x))
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))

So these are just casts to keep sparse happy and able to check this stuff.

#define __cpu_to_be32(x) ((__force __be32)__swab32((x)))
#define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
#define __cpu_to_be16(x) ((__force __be16)__swab16((x)))
#define __be16_to_cpu(x) __swab16((__force __u16)(__be16)(x))

These do the endian conversion.

If your CPU is running in big endian mode:

#define __cpu_to_le32(x) ((__force __le32)__swab32((x)))
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
#define __cpu_to_le16(x) ((__force __le16)__swab16((x)))
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))

So these do the endian conversion, and:

#define __cpu_to_be32(x) ((__force __be32)(__u32)(x))
#define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
#define __cpu_to_be16(x) ((__force __be16)(__u16)(x))
#define __be16_to_cpu(x) ((__force __u16)(__be16)(x))

These are just casts.

On Mon, Jan 09, 2012 at 11:51:43AM +0000, Russell King - ARM Linux wrote:
> On Sun, Jan 08, 2012 at 10:44:22PM +0800, Richard Zhao wrote:
> > readl/writel is more genric. And if CONFIG_ARM_DMA_MEM_BUFFERABLE,
> > they includes necessary memory barriers.
> 
> In a DMA engine driver, you need to use the barrier accessors when:
> 
> 1. You finally enable the DMA engine to perform a transfer.
>    The included barrier ensures that writes to the descriptors are visible
>    to the DMA engine.
> 
> 2. You read from a status register before examining the descriptors.
>    This ensures that the descriptor accesses won't be ordered before the
>    status register read.
> 
> Provided other accesses are within the same 1K region, the remainder of
> them do not have to be the strictly ordered accessors, and you can use
> the _relaxed variants (but only in ARM specific drivers.)
> 
> So, if your DMA engine has a control register, and a descriptor pointer
> register, you can write the descriptor pointer register with a
> writel_relaxed().  When you write the control register to enable the
> transfer, use writel() to ensure there's a barrier so the descriptors
> are visible.
Thanks very much for teaching. I understand now what relaxed means here.
I'll change all register access to _relaxed version except in function
sdma_enable_channel.
Freescale power pc also has sdma, I guess we can use _relaxed till they
tend to use the same driver ( I don't know when).

Thanks
Richard

On Mon, Jan 09, 2012 at 10:21:28PM +0800, Richard Zhao wrote:
> Freescale power pc also has sdma, I guess we can use _relaxed till they
> tend to use the same driver ( I don't know when).

The answer to that is to persuade powerpc to gain the _relaxed versions
(which can simply call the non-relaxed versions as a first stab.)  They
have the readx_relaxed() stuff already.

On Mon, Jan 09, 2012 at 02:46:27PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 09, 2012 at 10:21:28PM +0800, Richard Zhao wrote:
> > Freescale power pc also has sdma, I guess we can use _relaxed till they
> > tend to use the same driver ( I don't know when).
> 
> The answer to that is to persuade powerpc to gain the _relaxed versions
> (which can simply call the non-relaxed versions as a first stab.)  They
> have the readx_relaxed() stuff already.
quite right.

Thanks
Richard