[PATCH v3 0/3] Ordered I/O accessors

[PATCH v3 0/3] Ordered I/O accessors

[Catalin Marinas]

Hi,

This version replaces the do { ... } while (0) constructs in the final
patch with ({ ... }) to avoid errors in drivers putting brackets around
the write*() accessors. To avoid the write*() macro having a non-void
type (and possibly the compiler generating code to read a register), the
first patch explicitly casts the __raw_write*() accessors to (void).


Catalin Marinas (3):
      ARM: Introduce *_relaxed() I/O accessors
      ARM: Convert L2x0 to use the IO relaxed operations
      ARM: Add barriers to the I/O accessors if ARM_DMA_MEM_BUFFERABLE


 arch/arm/include/asm/io.h |   40 ++++++++++++++++++++++++++++------------
 arch/arm/mm/cache-l2x0.c  |   26 +++++++++++++-------------
 2 files changed, 41 insertions(+), 25 deletions(-)

-- 
Catalin

[PATCH v3 1/3] ARM: Introduce *_relaxed() I/O accessors

[Catalin Marinas]

This patch introduces readl*_relaxed()/write*_relaxed() as the main I/O
accessors (when __mem_pci is defined). The standard read*()/write*()
macros are now based on the relaxed accessors.

This patch is in preparation for a subsequent patch which adds barriers
to the I/O accessors.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/include/asm/io.h |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index c980156..9db072d 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -179,25 +179,30 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
  * IO port primitives for more information.
  */
 #ifdef __mem_pci
-#define readb(c) ({ __u8  __v = __raw_readb(__mem_pci(c)); __v; })
-#define readw(c) ({ __u16 __v = le16_to_cpu((__force __le16) \
+#define readb_relaxed(c) ({ u8  __v = __raw_readb(__mem_pci(c)); __v; })
+#define readw_relaxed(c) ({ u16 __v = le16_to_cpu((__force __le16) \
 					__raw_readw(__mem_pci(c))); __v; })
-#define readl(c) ({ __u32 __v = le32_to_cpu((__force __le32) \
+#define readl_relaxed(c) ({ u32 __v = le32_to_cpu((__force __le32) \
 					__raw_readl(__mem_pci(c))); __v; })
-#define readb_relaxed(addr) readb(addr)
-#define readw_relaxed(addr) readw(addr)
-#define readl_relaxed(addr) readl(addr)
+
+#define writeb_relaxed(v,c)	((void)__raw_writeb(v,__mem_pci(c)))
+#define writew_relaxed(v,c)	((void)__raw_writew((__force u16) \
+					cpu_to_le16(v),__mem_pci(c)))
+#define writel_relaxed(v,c)	((void)__raw_writel((__force u32) \
+					cpu_to_le32(v),__mem_pci(c)))
+
+#define readb(c)		readb_relaxed(c)
+#define readw(c)		readw_relaxed(c)
+#define readl(c)		readl_relaxed(c)
+
+#define writeb(v,c)		writeb_relaxed(v,c)
+#define writew(v,c)		writew_relaxed(v,c)
+#define writel(v,c)		writel_relaxed(v,c)
 
 #define readsb(p,d,l)		__raw_readsb(__mem_pci(p),d,l)
 #define readsw(p,d,l)		__raw_readsw(__mem_pci(p),d,l)
 #define readsl(p,d,l)		__raw_readsl(__mem_pci(p),d,l)
 
-#define writeb(v,c)		__raw_writeb(v,__mem_pci(c))
-#define writew(v,c)		__raw_writew((__force __u16) \
-					cpu_to_le16(v),__mem_pci(c))
-#define writel(v,c)		__raw_writel((__force __u32) \
-					cpu_to_le32(v),__mem_pci(c))
-
 #define writesb(p,d,l)		__raw_writesb(__mem_pci(p),d,l)
 #define writesw(p,d,l)		__raw_writesw(__mem_pci(p),d,l)
 #define writesl(p,d,l)		__raw_writesl(__mem_pci(p),d,l)

[PATCH v3 1/3] ARM: Introduce *_relaxed() I/O accessors

[Giuseppe Calderaro]

+#define writeb_relaxed(v,c)	((void)__raw_writeb(v,__mem_pci(c)))
+#define writew_relaxed(v,c)	((void)__raw_writew((__force u16) \
+					cpu_to_le16(v),__mem_pci(c)))
+#define writel_relaxed(v,c)	((void)__raw_writel((__force u32) \

Adding the cast here means that someone could use the __raw_writeb/w/l
macros and still get the side effect.
What about adding the same cast to the __raw_* defines as well?

Giuseppe

[PATCH v3 1/3] ARM: Introduce *_relaxed() I/O accessors

[Catalin Marinas]

On Wed, 2010-07-14 at 16:15 +0100, Giuseppe Calderaro wrote:
> +#define writeb_relaxed(v,c)    ((void)__raw_writeb(v,__mem_pci(c)))
> +#define writew_relaxed(v,c)    ((void)__raw_writew((__force u16) \
> +                                       cpu_to_le16(v),__mem_pci(c)))
> +#define writel_relaxed(v,c)    ((void)__raw_writel((__force u32) \
> 
> Adding the cast here means that someone could use the __raw_writeb/w/l
> macros and still get the side effect.
> What about adding the same cast to the __raw_* defines as well?

Those macros were too long already :). Those macros have a different
construct with a comma inside the brackets. I'm not sure about the
resulting type.

But anyway, we've had the __raw_ accessors for a long time and haven't
seen any problem.

-- 
Catalin

[PATCH v3 2/3] ARM: Convert L2x0 to use the IO relaxed operations

[Catalin Marinas]

This patch is in preparation for a subsequent patch which adds barriers
to the I/O accessors. Since the mandatory barriers may do an L2 cache
sync, this patch avoids a recursive call into l2x0_cache_sync() via the
write*() accessors and wmb() and a call into l2x0_cache_sync() with the
l2x0_lock held.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/mm/cache-l2x0.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 9819869..532c35c 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -32,14 +32,14 @@ static uint32_t l2x0_way_mask;	/* Bitmask of active ways */
 static inline void cache_wait(void __iomem *reg, unsigned long mask)
 {
 	/* wait for the operation to complete */
-	while (readl(reg) & mask)
+	while (readl_relaxed(reg) & mask)
 		;
 }
 
 static inline void cache_sync(void)
 {
 	void __iomem *base = l2x0_base;
-	writel(0, base + L2X0_CACHE_SYNC);
+	writel_relaxed(0, base + L2X0_CACHE_SYNC);
 	cache_wait(base + L2X0_CACHE_SYNC, 1);
 }
 
@@ -47,14 +47,14 @@ static inline void l2x0_clean_line(unsigned long addr)
 {
 	void __iomem *base = l2x0_base;
 	cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
-	writel(addr, base + L2X0_CLEAN_LINE_PA);
+	writel_relaxed(addr, base + L2X0_CLEAN_LINE_PA);
 }
 
 static inline void l2x0_inv_line(unsigned long addr)
 {
 	void __iomem *base = l2x0_base;
 	cache_wait(base + L2X0_INV_LINE_PA, 1);
-	writel(addr, base + L2X0_INV_LINE_PA);
+	writel_relaxed(addr, base + L2X0_INV_LINE_PA);
 }
 
 #ifdef CONFIG_PL310_ERRATA_588369
@@ -75,9 +75,9 @@ static inline void l2x0_flush_line(unsigned long addr)
 
 	/* Clean by PA followed by Invalidate by PA */
 	cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
-	writel(addr, base + L2X0_CLEAN_LINE_PA);
+	writel_relaxed(addr, base + L2X0_CLEAN_LINE_PA);
 	cache_wait(base + L2X0_INV_LINE_PA, 1);
-	writel(addr, base + L2X0_INV_LINE_PA);
+	writel_relaxed(addr, base + L2X0_INV_LINE_PA);
 }
 #else
 
@@ -90,7 +90,7 @@ static inline void l2x0_flush_line(unsigned long addr)
 {
 	void __iomem *base = l2x0_base;
 	cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
-	writel(addr, base + L2X0_CLEAN_INV_LINE_PA);
+	writel_relaxed(addr, base + L2X0_CLEAN_INV_LINE_PA);
 }
 #endif
 
@@ -109,7 +109,7 @@ static inline void l2x0_inv_all(void)
 
 	/* invalidate all ways */
 	spin_lock_irqsave(&l2x0_lock, flags);
-	writel(l2x0_way_mask, l2x0_base + L2X0_INV_WAY);
+	writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_INV_WAY);
 	cache_wait(l2x0_base + L2X0_INV_WAY, l2x0_way_mask);
 	cache_sync();
 	spin_unlock_irqrestore(&l2x0_lock, flags);
@@ -215,8 +215,8 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 
 	l2x0_base = base;
 
-	cache_id = readl(l2x0_base + L2X0_CACHE_ID);
-	aux = readl(l2x0_base + L2X0_AUX_CTRL);
+	cache_id = readl_relaxed(l2x0_base + L2X0_CACHE_ID);
+	aux = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);
 
 	/* Determine the number of ways */
 	switch (cache_id & L2X0_CACHE_ID_PART_MASK) {
@@ -245,17 +245,17 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 	 * If you are booting from non-secure mode
 	 * accessing the below registers will fault.
 	 */
-	if (!(readl(l2x0_base + L2X0_CTRL) & 1)) {
+	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & 1)) {
 
 		/* l2x0 controller is disabled */
 		aux &= aux_mask;
 		aux |= aux_val;
-		writel(aux, l2x0_base + L2X0_AUX_CTRL);
+		writel_relaxed(aux, l2x0_base + L2X0_AUX_CTRL);
 
 		l2x0_inv_all();
 
 		/* enable L2X0 */
-		writel(1, l2x0_base + L2X0_CTRL);
+		writel_relaxed(1, l2x0_base + L2X0_CTRL);
 	}
 
 	outer_cache.inv_range = l2x0_inv_range;

[PATCH v3 3/3] ARM: Add barriers to the I/O accessors if ARM_DMA_MEM_BUFFERABLE

[Catalin Marinas]

When the coherent DMA buffers are mapped as Normal Non-cacheable
(ARM_DMA_MEM_BUFFERABLE enabled), buffer accesses are no longer ordered
with Device memory accesses causing failures in device drivers that do
not use the mandatory memory barriers before starting a DMA transfer.
LKML discussions led to the conclusion that such barriers have to be
added to the I/O accessors:

http://thread.gmane.org/gmane.linux.kernel/683509/focus=686153
http://thread.gmane.org/gmane.linux.ide/46414
http://thread.gmane.org/gmane.linux.kernel.cross-arch/5250

This patch introduces a wmb() barrier to the write*() I/O accessors to
handle the situations where Normal Non-cacheable writes are still in the
processor (or L2 cache controller) write buffer before a DMA transfer
command is issued. For the read*() accessors, a rmb() is introduced
after the I/O to avoid speculative loads where the driver polls for a
DMA transfer ready bit.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/include/asm/io.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 9db072d..3c91e7c 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -26,6 +26,7 @@
 #include <linux/types.h>
 #include <asm/byteorder.h>
 #include <asm/memory.h>
+#include <asm/system.h>
 
 /*
  * ISA I/O bus memory addresses are 1:1 with the physical address.
@@ -191,6 +192,15 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
 #define writel_relaxed(v,c)	((void)__raw_writel((__force u32) \
 					cpu_to_le32(v),__mem_pci(c)))
 
+#ifdef CONFIG_ARM_DMA_MEM_BUFFERABLE
+#define readb(c)		({ u8  __v = readb_relaxed(c); rmb(); __v; })
+#define readw(c)		({ u16 __v = readw_relaxed(c); rmb(); __v; })
+#define readl(c)		({ u32 __v = readl_relaxed(c); rmb(); __v; })
+
+#define writeb(v,c)		({ wmb(); writeb_relaxed(v,c); })
+#define writew(v,c)		({ wmb(); writew_relaxed(v,c); })
+#define writel(v,c)		({ wmb(); writel_relaxed(v,c); })
+#else
 #define readb(c)		readb_relaxed(c)
 #define readw(c)		readw_relaxed(c)
 #define readl(c)		readl_relaxed(c)
@@ -198,6 +208,7 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
 #define writeb(v,c)		writeb_relaxed(v,c)
 #define writew(v,c)		writew_relaxed(v,c)
 #define writel(v,c)		writel_relaxed(v,c)
+#endif
 
 #define readsb(p,d,l)		__raw_readsb(__mem_pci(p),d,l)
 #define readsw(p,d,l)		__raw_readsw(__mem_pci(p),d,l)

[PATCH v3 0/3] Ordered I/O accessors

[Russell King - ARM Linux]

On Wed, Jul 14, 2010 at 04:01:12PM +0100, Catalin Marinas wrote:
> This version replaces the do { ... } while (0) constructs in the final
> patch with ({ ... }) to avoid errors in drivers putting brackets around
> the write*() accessors. To avoid the write*() macro having a non-void
> type (and possibly the compiler generating code to read a register), the
> first patch explicitly casts the __raw_write*() accessors to (void).

Ok.  Can you please put this in the patch system?  I think we need to
get this in prior to 2.6.35 being released.

[PATCH v3 0/3] Ordered I/O accessors

[Rabin VINCENT]

On Tue, Jul 27, 2010 at 13:03:10 +0200, Russell King - ARM Linux wrote:
> On Wed, Jul 14, 2010 at 04:01:12PM +0100, Catalin Marinas wrote:
> > This version replaces the do { ... } while (0) constructs in the final
> > patch with ({ ... }) to avoid errors in drivers putting brackets around
> > the write*() accessors. To avoid the write*() macro having a non-void
> > type (and possibly the compiler generating code to read a register), the
> > first patch explicitly casts the __raw_write*() accessors to (void).
> 
> Ok.  Can you please put this in the patch system?  I think we need to
> get this in prior to 2.6.35 being released.

ux500 is using writel() inside uncompress.h, so this series triggers a
build failure there:

  arch/arm/boot/compressed/misc.o: In function `putc':
  arch/arm/mach-ux500/include/mach/uncompress.h:41:
  undefined reference to `outer_cache'

Something like the following needs to be applied to fix it.  I'll submit
this as a separate patch if it can't be folded in.

diff --git a/arch/arm/mach-ux500/include/mach/uncompress.h b/arch/arm/mach-ux500/include/mach/uncompress.h
index 8552eb1..0271ca0 100644
--- a/arch/arm/mach-ux500/include/mach/uncompress.h
+++ b/arch/arm/mach-ux500/include/mach/uncompress.h
@@ -30,22 +30,22 @@
 static void putc(const char c)
 {
 	/* Do nothing if the UART is not enabled. */
-	if (!(readb(U8500_UART_CR) & 0x1))
+	if (!(__raw_readb(U8500_UART_CR) & 0x1))
 		return;
 
 	if (c == '\n')
 		putc('\r');
 
-	while (readb(U8500_UART_FR) & (1 << 5))
+	while (__raw_readb(U8500_UART_FR) & (1 << 5))
 		barrier();
-	writeb(c, U8500_UART_DR);
+	__raw_writeb(c, U8500_UART_DR);
 }
 
 static void flush(void)
 {
-	if (!(readb(U8500_UART_CR) & 0x1))
+	if (!(__raw_readb(U8500_UART_CR) & 0x1))
 		return;
-	while (readb(U8500_UART_FR) & (1 << 3))
+	while (__raw_readb(U8500_UART_FR) & (1 << 3))
 		barrier();
 }
 

[PATCH v3 0/3] Ordered I/O accessors

[Will Deacon]

Hi Russell, Rabin,

> On Tue, Jul 27, 2010 at 13:03:10 +0200, Russell King - ARM Linux wrote:
> > On Wed, Jul 14, 2010 at 04:01:12PM +0100, Catalin Marinas wrote:
> > > This version replaces the do { ... } while (0) constructs in the final
> > > patch with ({ ... }) to avoid errors in drivers putting brackets around
> > > the write*() accessors. To avoid the write*() macro having a non-void
> > > type (and possibly the compiler generating code to read a register), the
> > > first patch explicitly casts the __raw_write*() accessors to (void).
> >
> > Ok.  Can you please put this in the patch system?  I think we need to
> > get this in prior to 2.6.35 being released.

Catalin's away at the moment and I'm not sure if he's picking up mail.

> ux500 is using writel() inside uncompress.h, so this series triggers a
> build failure there:
> 
>   arch/arm/boot/compressed/misc.o: In function `putc':
>   arch/arm/mach-ux500/include/mach/uncompress.h:41:
>   undefined reference to `outer_cache'

There's also an issue with the iowrite* macros, which need a (void)
cast in the same way as the write* guys.

Will

[PATCH v3 0/3] Ordered I/O accessors

[Russell King - ARM Linux]

On Wed, Jul 28, 2010 at 02:06:52PM +0100, Will Deacon wrote:
> Hi Russell, Rabin,
> 
> > On Tue, Jul 27, 2010 at 13:03:10 +0200, Russell King - ARM Linux wrote:
> > > On Wed, Jul 14, 2010 at 04:01:12PM +0100, Catalin Marinas wrote:
> > > > This version replaces the do { ... } while (0) constructs in the final
> > > > patch with ({ ... }) to avoid errors in drivers putting brackets around
> > > > the write*() accessors. To avoid the write*() macro having a non-void
> > > > type (and possibly the compiler generating code to read a register), the
> > > > first patch explicitly casts the __raw_write*() accessors to (void).
> > >
> > > Ok.  Can you please put this in the patch system?  I think we need to
> > > get this in prior to 2.6.35 being released.
> 
> Catalin's away at the moment and I'm not sure if he's picking up mail.
> 
> > ux500 is using writel() inside uncompress.h, so this series triggers a
> > build failure there:
> > 
> >   arch/arm/boot/compressed/misc.o: In function `putc':
> >   arch/arm/mach-ux500/include/mach/uncompress.h:41:
> >   undefined reference to `outer_cache'
> 
> There's also an issue with the iowrite* macros, which need a (void)
> cast in the same way as the write* guys.

I've fixed the io* macros up too.  We need a patch to fix ux500 ASAP
though.

[PATCH v3 0/3] Ordered I/O accessors

[Russell King - ARM Linux]

On Wed, Jul 28, 2010 at 06:18:47PM +0530, Rabin VINCENT wrote:
> Something like the following needs to be applied to fix it.  I'll submit
> this as a separate patch if it can't be folded in.

Please submit it as a separate patch to the patch system.  Thanks.

> diff --git a/arch/arm/mach-ux500/include/mach/uncompress.h b/arch/arm/mach-ux500/include/mach/uncompress.h
> index 8552eb1..0271ca0 100644
> --- a/arch/arm/mach-ux500/include/mach/uncompress.h
> +++ b/arch/arm/mach-ux500/include/mach/uncompress.h
> @@ -30,22 +30,22 @@
>  static void putc(const char c)
>  {
>  	/* Do nothing if the UART is not enabled. */
> -	if (!(readb(U8500_UART_CR) & 0x1))
> +	if (!(__raw_readb(U8500_UART_CR) & 0x1))
>  		return;
>  
>  	if (c == '\n')
>  		putc('\r');
>  
> -	while (readb(U8500_UART_FR) & (1 << 5))
> +	while (__raw_readb(U8500_UART_FR) & (1 << 5))
>  		barrier();
> -	writeb(c, U8500_UART_DR);
> +	__raw_writeb(c, U8500_UART_DR);
>  }
>  
>  static void flush(void)
>  {
> -	if (!(readb(U8500_UART_CR) & 0x1))
> +	if (!(__raw_readb(U8500_UART_CR) & 0x1))
>  		return;
> -	while (readb(U8500_UART_FR) & (1 << 3))
> +	while (__raw_readb(U8500_UART_FR) & (1 << 3))
>  		barrier();
>  }
>  

[PATCH v3 0/3] Ordered I/O accessors

[Catalin Marinas]

On Tue, 2010-07-27 at 12:03 +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 14, 2010 at 04:01:12PM +0100, Catalin Marinas wrote:
> > This version replaces the do { ... } while (0) constructs in the final
> > patch with ({ ... }) to avoid errors in drivers putting brackets around
> > the write*() accessors. To avoid the write*() macro having a non-void
> > type (and possibly the compiler generating code to read a register), the
> > first patch explicitly casts the __raw_write*() accessors to (void).
> 
> Ok.  Can you please put this in the patch system?  I think we need to
> get this in prior to 2.6.35 being released.

I pushed the three patches to the patch system (I'm still around
tomorrow and then off-line for a week, so please let me know if there
are any issues).

Thanks.

-- 
Catalin