Ordered I/O accessors
Subsystems (3)
v3 cover Ordered I/O accessors
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(-)
Signature · 1 line
Catalin
PATCH v3 1/3 ARM: Introduce *_relaxed() I/O accessors
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.
--- arch/arm/include/asm/io.h | 29 +++++++++++++++++------------ 1 files changed, 17 insertions(+), 12 deletions(-)
arch/arm/include/asm/io.h +17 -12
@@ -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
+#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
Quoted 8 lines · Giuseppe Calderaro
arch/arm/include/asm/io.h +4
-0+#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.
Signature · 1 line
Catalin
PATCH v3 2/3 ARM: Convert L2x0 to use the IO relaxed operations
In reply to Catalin MarinasThis 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.
--- arch/arm/mm/cache-l2x0.c | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-)
arch/arm/mm/cache-l2x0.c +13 -13
@@ -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
In reply to Catalin MarinasWhen 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.
--- arch/arm/include/asm/io.h | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
arch/arm/include/asm/io.h +11 -0
@@ -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)
v3 cover Ordered I/O accessors
In reply to Catalin MarinasQuoted 5 lines · Catalin Marinas
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.
v3 cover Ordered I/O accessors
Quoted 9 lines · Russell King - ARM Linux
Quoted 5 lines · Catalin Marinas
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.
arch/arm/mach-ux500/include/mach/uncompress.h +5 -5
@@ -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 (); }
v3 cover Ordered I/O accessors
Hi Russell, Rabin,
Quoted 10 lines · Russell King - ARM Linux
Quoted 9 lines · Catalin Marinas
Quoted 5 lines
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.
Quoted 6 lines
ux500 is using
writel()insideuncompress.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
v3 cover Ordered I/O accessors
Quoted 18 lines · Will Deacon
Hi Russell, Rabin,
Quoted 10 lines · Russell King - ARM Linux
Quoted 9 lines · Catalin Marinas
Quoted 5 lines
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.
Quoted 3 lines
ux500 is using
writel()insideuncompress.h, so this series triggers a build failure there:
Quoted 6 lines
Quoted 3 lines
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.
v3 cover Ordered I/O accessors
In reply to Rabin VINCENTQuoted 2 lines · Rabin VINCENT
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.
Quoted 32 lines
arch/arm/mach-ux500/include/mach/uncompress.h +5
-5@@ -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 (); }
v3 cover Ordered I/O accessors
In reply to Russell King - ARM LinuxQuoted 9 lines · Russell King - ARM Linux
Quoted 5 lines · Catalin Marinas
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.
Signature · 1 line
Catalin