asm-generic/io.h: Implement generic {read,write}s*()

PATCH In review Thierry Reding 6 participants 23 messages Jul 16, 2014 – Aug 5, 2014

Subsystems (3)

Arm (32-bit) arm64 Generic include/asm header files

lore

Share & tools

lore

Thierry Reding

author

2014-07-16 11:01 UTC #0 lore raw

PATCH v3 1/3 asm-generic/io.h: Implement generic {read,write}s*()

From: Thierry Reding <[email protected]>

Currently driver writers need to use io{read,write}{8,16,32}_rep() when accessing FIFO registers portably. This is bad for two reasons: it is inconsistent with how other registers are accessed using the standard {read,write}{b,w,l}() functions, which can lead to confusion. On some architectures the io{read,write}*() functions also need to perform some extra checks to determine whether an address is memory-mapped or refers to I/O space. Drivers which can be expected to never use I/O can safely use the {read,write}s{b,w,l,q}(), just like they use their non-string variants and there's no need for these extra checks.

This patch implements generic versions of readsb(), readsw(), readsl(), readsq(), writesb(), writesw(), writesl() and writesq(). Variants of these string functions for I/O accesses (ins*() and outs*() as well as ioread*_rep() and iowrite*_rep()) are now implemented in terms of the new functions.

Going forward, {read,write}{,s}{b,w,l,q}() should be used consistently by drivers for devices that will only ever be memory-mapped and hence don't need to access I/O space, whereas io{read,write}{8,16,32}_rep() should be used by drivers for devices that can be either memory-mapped or I/O-mapped.

While at it, also make sure that any of the functions provided as fallback for architectures that don't override them can't be overridden subsequently.

This is compile- and runtime-tested on 32-bit and 64-bit ARM and compile tested on Microblaze, s390, SPARC and Xtensa. For ARC, Blackfin, Metag, OpenRISC, Score and Unicore32 which also use asm-generic/io.h I couldn't find or build a cross-compiler that would run on my system. But by code inspection they shouldn't break with this patch.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- allow architectures to override io{read,write}{16,32}be()
- explain the reasons for this change in the commit message

Changes in v2:
- respect IO_SPACE_LIMIT in ioport_map()

 include/asm-generic/io.h | 294 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 211 insertions(+), 83 deletions(-)

include/asm-generic/io.h +211 ~~-83~~

index 975e1cc75edb..b2ea16b4264b 100644
@@ -32,6 +32,7 @@
  * memory location directly.
  */
 #ifndef __raw_readb
+#define __raw_readb __raw_readb
 static inline u8 __raw_readb(const volatile void __iomem *addr)
 {
 	return *(const volatile u8 __force *) addr;

@@ -39,6 +40,7 @@ static inline u8 __raw_readb(const volatile void __iomem *addr)
 #endif
 
 #ifndef __raw_readw
+#define __raw_readw __raw_readw
 static inline u16 __raw_readw(const volatile void __iomem *addr)
 {
 	return *(const volatile u16 __force *) addr;

@@ -46,27 +48,35 @@ static inline u16 __raw_readw(const volatile void __iomem *addr)
 #endif
 
 #ifndef __raw_readl
+#define __raw_readl __raw_readl
 static inline u32 __raw_readl(const volatile void __iomem *addr)
 {
 	return *(const volatile u32 __force *) addr;
 }
 #endif
 
+#ifndef readb
 #define readb __raw_readb
+#endif
 
+#ifndef readw
 #define readw readw
 static inline u16 readw(const volatile void __iomem *addr)
 {
 	return __le16_to_cpu(__raw_readw(addr));
 }
+#endif
 
+#ifndef readl
 #define readl readl
 static inline u32 readl(const volatile void __iomem *addr)
 {
 	return __le32_to_cpu(__raw_readl(addr));
 }
+#endif
 
 #ifndef __raw_writeb
+#define __raw_writeb __raw_writeb
 static inline void __raw_writeb(u8 b, volatile void __iomem *addr)
 {
 	*(volatile u8 __force *) addr = b;

@@ -74,6 +84,7 @@ static inline void __raw_writeb(u8 b, volatile void __iomem *addr)
 #endif
 
 #ifndef __raw_writew
+#define __raw_writew __raw_writew
 static inline void __raw_writew(u16 b, volatile void __iomem *addr)
 {
 	*(volatile u16 __force *) addr = b;

@@ -81,78 +92,225 @@ static inline void __raw_writew(u16 b, volatile void __iomem *addr)
 #endif
 
 #ifndef __raw_writel
+#define __raw_writel __raw_writel
 static inline void __raw_writel(u32 b, volatile void __iomem *addr)
 {
 	*(volatile u32 __force *) addr = b;
 }
 #endif
 
+#ifndef writeb
 #define writeb __raw_writeb
+#endif
+
+#ifndef writew
 #define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
+#endif
+
+#ifndef writel
 #define writel(b,addr) __raw_writel(__cpu_to_le32(b),addr)
+#endif
 
 #ifdef CONFIG_64BIT
 #ifndef __raw_readq
+#define __raw_readq __raw_readq
 static inline u64 __raw_readq(const volatile void __iomem *addr)
 {
 	return *(const volatile u64 __force *) addr;
 }
 #endif
 
+#ifndef readq
 #define readq readq
 static inline u64 readq(const volatile void __iomem *addr)
 {
 	return __le64_to_cpu(__raw_readq(addr));
 }
+#endif
 
 #ifndef __raw_writeq
+#define __raw_writeq __raw_writeq
 static inline void __raw_writeq(u64 b, volatile void __iomem *addr)
 {
 	*(volatile u64 __force *) addr = b;
 }
 #endif
 
+#ifndef writeq
 #define writeq(b, addr) __raw_writeq(__cpu_to_le64(b), addr)
+#endif
+#endif /* CONFIG_64BIT */
+
+#ifndef readsb
+#define readsb readsb
+static inline void readsb(const void __iomem *addr, void *buffer, int count)
+{
+	if (count) {
+		u8 *buf = buffer;
+		do {
+			u8 x = __raw_readb(addr);
+			*buf++ = x;
+		} while (--count);
+	}
+}
+#endif
+
+#ifndef readsw
+#define readsw readsw
+static inline void readsw(const void __iomem *addr, void *buffer, int count)
+{
+	if (count) {
+		u16 *buf = buffer;
+		do {
+			u16 x = __raw_readw(addr);
+			*buf++ = x;
+		} while (--count);
+	}
+}
+#endif
+
+#ifndef readsl
+#define readsl readsl
+static inline void readsl(const void __iomem *addr, void *buffer, int count)
+{
+	if (count) {
+		u32 *buf = buffer;
+		do {
+			u32 x = __raw_readl(addr);
+			*buf++ = x;
+		} while (--count);
+	}
+}
+#endif
+
+#ifndef writesb
+#define writesb writesb
+static inline void writesb(void __iomem *addr, const void *buffer, int count)
+{
+	if (count) {
+		const u8 *buf = buffer;
+		do {
+			__raw_writeb(*buf++, addr);
+		} while (--count);
+	}
+}
+#endif
+
+#ifndef writesw
+#define writesw writesw
+static inline void writesw(void __iomem *addr, const void *buffer, int count)
+{
+	if (count) {
+		const u16 *buf = buffer;
+		do {
+			__raw_writew(*buf++, addr);
+		} while (--count);
+	}
+}
+#endif
+
+#ifndef writesl
+#define writesl writesl
+static inline void writesl(void __iomem *addr, const void *buffer, int count)
+{
+	if (count) {
+		const u32 *buf = buffer;
+		do {
+			__raw_writel(*buf++, addr);
+		} while (--count);
+	}
+}
+#endif
+
+#ifdef CONFIG_64BIT
+#ifndef readsq
+#define readsq readsq
+static inline void readsq(const void __iomem *addr, void *buffer, int count)
+{
+	if (count) {
+		u64 *buf = buffer;
+		do {
+			u64 x = __raw_readq(addr);
+			*buf++ = x;
+		} while (--count);
+	}
+}
+#endif
+
+#ifndef writesq
+#define writesq writesq
+static inline void writesq(void __iomem *addr, const void *buffer, int count)
+{
+	if (count) {
+		const u64 *buf = buffer;
+		do {
+			__raw_writeq(*buf++, addr);
+		} while (--count);
+	}
+}
+#endif
 #endif /* CONFIG_64BIT */
 
 #ifndef PCI_IOBASE
 #define PCI_IOBASE ((void __iomem *) 0)
 #endif
 
+#ifndef IO_SPACE_LIMIT
+#define IO_SPACE_LIMIT 0xffff
+#endif
+
 /*****************************************************************************/
 /*
  * traditional input/output functions
  */
 
+#ifndef inb
+#define inb inb
 static inline u8 inb(unsigned long addr)
 {
-	return readb(addr + PCI_IOBASE);
+	return readb(PCI_IOBASE + addr);
 }
+#endif
 
+#ifndef inw
+#define inw inw
 static inline u16 inw(unsigned long addr)
 {
-	return readw(addr + PCI_IOBASE);
+	return readw(PCI_IOBASE + addr);
 }
+#endif
 
+#ifndef inl
+#define inl inl
 static inline u32 inl(unsigned long addr)
 {
-	return readl(addr + PCI_IOBASE);
+	return readl(PCI_IOBASE + addr);
 }
+#endif
 
+#ifndef outb
+#define outb outb
 static inline void outb(u8 b, unsigned long addr)
 {
-	writeb(b, addr + PCI_IOBASE);
+	writeb(b, PCI_IOBASE + addr);
 }
+#endif
 
+#ifndef outw
+#define outw outw
 static inline void outw(u16 b, unsigned long addr)
 {
-	writew(b, addr + PCI_IOBASE);
+	writew(b, PCI_IOBASE + addr);
 }
+#endif
 
+#ifndef outl
+#define outl outl
 static inline void outl(u32 b, unsigned long addr)
 {
-	writel(b, addr + PCI_IOBASE);
+	writel(b, PCI_IOBASE + addr);
 }
+#endif
 
 #define inb_p(addr)	inb(addr)
 #define inw_p(addr)	inw(addr)

@@ -162,112 +320,71 @@ static inline void outl(u32 b, unsigned long addr)
 #define outl_p(x, addr)	outl((x), (addr))
 
 #ifndef insb
-static inline void insb(unsigned long addr, void *buffer, int count)
-{
-	if (count) {
-		u8 *buf = buffer;
-		do {
-			u8 x = __raw_readb(addr + PCI_IOBASE);
-			*buf++ = x;
-		} while (--count);
-	}
-}
+#define insb(addr, buffer, count) readsb(PCI_IOBASE + addr, buffer, count)
 #endif
 
 #ifndef insw
-static inline void insw(unsigned long addr, void *buffer, int count)
-{
-	if (count) {
-		u16 *buf = buffer;
-		do {
-			u16 x = __raw_readw(addr + PCI_IOBASE);
-			*buf++ = x;
-		} while (--count);
-	}
-}
+#define insw(addr, buffer, count) readsw(PCI_IOBASE + addr, buffer, count)
 #endif
 
 #ifndef insl
-static inline void insl(unsigned long addr, void *buffer, int count)
-{
-	if (count) {
-		u32 *buf = buffer;
-		do {
-			u32 x = __raw_readl(addr + PCI_IOBASE);
-			*buf++ = x;
-		} while (--count);
-	}
-}
+#define insl(addr, buffer, count) readsl(PCI_IOBASE + addr, buffer, count)
 #endif
 
 #ifndef outsb
-static inline void outsb(unsigned long addr, const void *buffer, int count)
-{
-	if (count) {
-		const u8 *buf = buffer;
-		do {
-			__raw_writeb(*buf++, addr + PCI_IOBASE);
-		} while (--count);
-	}
-}
+#define outsb(addr, buffer, count) writesb(PCI_IOBASE + addr, buffer, count)
 #endif
 
 #ifndef outsw
-static inline void outsw(unsigned long addr, const void *buffer, int count)
-{
-	if (count) {
-		const u16 *buf = buffer;
-		do {
-			__raw_writew(*buf++, addr + PCI_IOBASE);
-		} while (--count);
-	}
-}
+#define outsw(addr, buffer, count) writesw(PCI_IOBASE + addr, buffer, count)
 #endif
 
 #ifndef outsl
-static inline void outsl(unsigned long addr, const void *buffer, int count)
-{
-	if (count) {
-		const u32 *buf = buffer;
-		do {
-			__raw_writel(*buf++, addr + PCI_IOBASE);
-		} while (--count);
-	}
-}
+#define outsl(addr, buffer, count) writesl(PCI_IOBASE + addr, buffer, count)
 #endif
 
+#define insb_p(port,to,len)	insb(port,to,len)
+#define insw_p(port,to,len)	insw(port,to,len)
+#define insl_p(port,to,len)	insl(port,to,len)
+
+#define outsb_p(port,from,len)	outsb(port,from,len)
+#define outsw_p(port,from,len)	outsw(port,from,len)
+#define outsl_p(port,from,len)	outsl(port,from,len)
+
 #ifndef CONFIG_GENERIC_IOMAP
 #define ioread8(addr)		readb(addr)
 #define ioread16(addr)		readw(addr)
-#define ioread16be(addr)	__be16_to_cpu(__raw_readw(addr))
 #define ioread32(addr)		readl(addr)
+
+#ifndef ioread16be
+#define ioread16be(addr)	__be16_to_cpu(__raw_readw(addr))
+#endif
+
+#ifndef ioread32be
 #define ioread32be(addr)	__be32_to_cpu(__raw_readl(addr))
+#endif
 
 #define iowrite8(v, addr)	writeb((v), (addr))
 #define iowrite16(v, addr)	writew((v), (addr))
-#define iowrite16be(v, addr)	__raw_writew(__cpu_to_be16(v), addr)
 #define iowrite32(v, addr)	writel((v), (addr))
-#define iowrite32be(v, addr)	__raw_writel(__cpu_to_be32(v), addr)
 
-#define ioread8_rep(p, dst, count) \
-	insb((unsigned long) (p), (dst), (count))
-#define ioread16_rep(p, dst, count) \
-	insw((unsigned long) (p), (dst), (count))
-#define ioread32_rep(p, dst, count) \
-	insl((unsigned long) (p), (dst), (count))
-
-#define iowrite8_rep(p, src, count) \
-	outsb((unsigned long) (p), (src), (count))
-#define iowrite16_rep(p, src, count) \
-	outsw((unsigned long) (p), (src), (count))
-#define iowrite32_rep(p, src, count) \
-	outsl((unsigned long) (p), (src), (count))
-#endif /* CONFIG_GENERIC_IOMAP */
+#ifndef iowrite16be
+#define iowrite16be(v, addr)	__raw_writew(__cpu_to_be16(v), addr)
+#endif
 
-#ifndef IO_SPACE_LIMIT
-#define IO_SPACE_LIMIT 0xffff
+#ifndef iowrite32be
+#define iowrite32be(v, addr)	__raw_writel(__cpu_to_be32(v), addr)
 #endif
 
+#define ioread8_rep(p, dst, count) readsb(p, dst, count)
+#define ioread16_rep(p, dst, count) readsw(p, dst, count)
+#define ioread32_rep(p, dst, count) readsl(p, dst, count)
+
+#define iowrite8_rep(p, src, count) writesb(p, src, count)
+#define iowrite16_rep(p, src, count) writesw(p, src, count)
+#define iowrite32_rep(p, src, count) writesl(p, src, count)
+#endif /* CONFIG_GENERIC_IOMAP */
+
 #ifdef __KERNEL__
 
 #include <linux/vmalloc.h>

@@ -278,6 +395,7 @@ struct pci_dev;
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
 
 #ifndef pci_iounmap
+#define pci_iounmap pci_iounmap
 static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
 {
 }

@@ -289,11 +407,15 @@ static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
  * These are pretty trivial
  */
 #ifndef virt_to_phys
+#define virt_to_phys virt_to_phys
 static inline unsigned long virt_to_phys(volatile void *address)
 {
 	return __pa((unsigned long)address);
 }
+#endif
 
+#ifndef phys_to_virt
+#define phys_to_virt phys_to_virt
 static inline void *phys_to_virt(unsigned long address)
 {
 	return __va(address);

@@ -329,14 +451,20 @@ static inline void iounmap(void __iomem *addr)
 
 #ifdef CONFIG_HAS_IOPORT_MAP
 #ifndef CONFIG_GENERIC_IOMAP
+#ifndef ioport_map
+#define ioport_map ioport_map
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
-	return (void __iomem *) port;
+	return PCI_IOBASE + (port & IO_SPACE_LIMIT);
 }
+#endif
 
+#ifndef ioport_unmap
+#define ioport_unmap ioport_unmap
 static inline void ioport_unmap(void __iomem *p)
 {
 }
+#endif
 #else /* CONFIG_GENERIC_IOMAP */
 extern void __iomem *ioport_map(unsigned long port, unsigned int nr);
 extern void ioport_unmap(void __iomem *p);

Signature · 1 line

2.0.1

Thierry Reding

author

2014-07-16 11:01 UTC #1 lore raw

PATCH v3 2/3 ARM: Use include/asm-generic/io.h

From: Thierry Reding <[email protected]>

Include the generic I/O header file so that duplicate implementations can be removed. This will also help to establish consistency across more architectures regarding which accessors they support.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- override io{read,write}{16,32}be() since the generic ones don't have the required memory barriers

 arch/arm/include/asm/io.h     | 53 +++++++++++++------------------------------
 arch/arm/include/asm/memory.h |  2 ++
 2 files changed, 18 insertions(+), 37 deletions(-)

arch/arm/include/asm/io.h +16 ~~-37~~

index a78562f21aab..72d87e788cfd 100644
@@ -69,6 +69,7 @@ extern void __raw_readsl(const void __iomem *addr, void *data, int longlen);
  * writeback addressing modes as these incur a significant performance
  * overhead (the address generation must be emulated in software).
  */
+#define __raw_writew __raw_writew
 static inline void __raw_writew(u16 val, volatile void __iomem *addr)
 {
 	asm volatile("strh %1, %0"

@@ -76,6 +77,7 @@ static inline void __raw_writew(u16 val, volatile void __iomem *addr)
 		     : "r" (val));
 }
 
+#define __raw_readw __raw_readw
 static inline u16 __raw_readw(const volatile void __iomem *addr)
 {
 	u16 val;

@@ -86,6 +88,7 @@ static inline u16 __raw_readw(const volatile void __iomem *addr)
 }
 #endif
 
+#define __raw_writeb __raw_writeb
 static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
 {
 	asm volatile("strb %1, %0"

@@ -93,6 +96,7 @@ static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
 		     : "r" (val));
 }
 
+#define __raw_writel __raw_writel
 static inline void __raw_writel(u32 val, volatile void __iomem *addr)
 {
 	asm volatile("str %1, %0"

@@ -100,6 +104,7 @@ static inline void __raw_writel(u32 val, volatile void __iomem *addr)
 		     : "r" (val));
 }
 
+#define __raw_readb __raw_readb
 static inline u8 __raw_readb(const volatile void __iomem *addr)
 {
 	u8 val;

@@ -109,6 +114,7 @@ static inline u8 __raw_readb(const volatile void __iomem *addr)
 	return val;
 }
 
+#define __raw_readl __raw_readl
 static inline u32 __raw_readl(const volatile void __iomem *addr)
 {
 	u32 val;

@@ -267,20 +273,6 @@ extern void pci_iounmap_io(unsigned int offset);
 #define insl(p,d,l)		__raw_readsl(__io(p),d,l)
 #endif
 
-#define outb_p(val,port)	outb((val),(port))
-#define outw_p(val,port)	outw((val),(port))
-#define outl_p(val,port)	outl((val),(port))
-#define inb_p(port)		inb((port))
-#define inw_p(port)		inw((port))
-#define inl_p(port)		inl((port))
-
-#define outsb_p(port,from,len)	outsb(port,from,len)
-#define outsw_p(port,from,len)	outsw(port,from,len)
-#define outsl_p(port,from,len)	outsl(port,from,len)
-#define insb_p(port,to,len)	insb(port,to,len)
-#define insw_p(port,to,len)	insw(port,to,len)
-#define insl_p(port,to,len)	insl(port,to,len)
-
 /*
  * String version of IO memory access ops:
  */

@@ -347,39 +339,26 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
 #define iounmap				__arm_iounmap
 
 /*
- * io{read,write}{8,16,32} macros
+ * io{read,write}{16,32}be() macros
  */
-#ifndef ioread8
-#define ioread8(p)	({ unsigned int __v = __raw_readb(p); __iormb(); __v; })
-#define ioread16(p)	({ unsigned int __v = le16_to_cpu((__force __le16)__raw_readw(p)); __iormb(); __v; })
-#define ioread32(p)	({ unsigned int __v = le32_to_cpu((__force __le32)__raw_readl(p)); __iormb(); __v; })
-
-#define ioread16be(p)	({ unsigned int __v = be16_to_cpu((__force __be16)__raw_readw(p)); __iormb(); __v; })
-#define ioread32be(p)	({ unsigned int __v = be32_to_cpu((__force __be32)__raw_readl(p)); __iormb(); __v; })
-
-#define iowrite8(v,p)	({ __iowmb(); __raw_writeb(v, p); })
-#define iowrite16(v,p)	({ __iowmb(); __raw_writew((__force __u16)cpu_to_le16(v), p); })
-#define iowrite32(v,p)	({ __iowmb(); __raw_writel((__force __u32)cpu_to_le32(v), p); })
+#define ioread16be(p)		({ __u16 __v = be16_to_cpu((__force __be16)__raw_readw(p)); __iormb(); __v; })
+#define ioread32be(p)		({ __u32 __v = be32_to_cpu((__force __be32)__raw_readl(p)); __iormb(); __v; })
 
-#define iowrite16be(v,p) ({ __iowmb(); __raw_writew((__force __u16)cpu_to_be16(v), p); })
-#define iowrite32be(v,p) ({ __iowmb(); __raw_writel((__force __u32)cpu_to_be32(v), p); })
-
-#define ioread8_rep(p,d,c)	__raw_readsb(p,d,c)
-#define ioread16_rep(p,d,c)	__raw_readsw(p,d,c)
-#define ioread32_rep(p,d,c)	__raw_readsl(p,d,c)
-
-#define iowrite8_rep(p,s,c)	__raw_writesb(p,s,c)
-#define iowrite16_rep(p,s,c)	__raw_writesw(p,s,c)
-#define iowrite32_rep(p,s,c)	__raw_writesl(p,s,c)
+#define iowrite16be(v,p)	({ __iowmb(); __raw_writew((__force __u16)cpu_to_be16(v), p); })
+#define iowrite32be(v,p)	({ __iowmb(); __raw_writew((__force __u32)cpu_to_be32(v), p); })
 
+#define ioport_map ioport_map
 extern void __iomem *ioport_map(unsigned long port, unsigned int nr);
+#define ioport_unmap ioport_unmap
 extern void ioport_unmap(void __iomem *addr);
-#endif
 
 struct pci_dev;
 
+#define pci_iounmap pci_iounmap
 extern void pci_iounmap(struct pci_dev *dev, void __iomem *addr);
 
+#include <asm-generic/io.h>
+
 /*
  * can the hardware map this into one segment or not, given no other
  * constraints.

arch/arm/include/asm/memory.h +2 -0

index 2b751464d6ff..35ae6eaf33b2 100644
@@ -278,11 +278,13 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
  * translation for translating DMA addresses.  Use the driver
  * DMA support - see dma-mapping.h.
  */
+#define virt_to_phys virt_to_phys
 static inline phys_addr_t virt_to_phys(const volatile void *x)
 {
 	return __virt_to_phys((unsigned long)(x));
 }
 
+#define phys_to_virt phys_to_virt
 static inline void *phys_to_virt(phys_addr_t x)
 {
 	return (void *)__phys_to_virt(x);

Signature · 1 line

2.0.1

Catalin Marinas

maintainer Acked-by on v3 2/3

2014-07-17 12:03 UTC #2 lore raw

Re: [PATCH v3 2/3] ARM: Use include/asm-generic/io.h

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>
Include the generic I/O header file so that duplicate implementations can be removed. This will also help to establish consistency across more architectures regarding which accessors they support.
Signed-off-by Thierry Reding <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Thierry Reding

author

2014-07-16 11:01 UTC #3 lore raw

PATCH v3 3/3 arm64: Use include/asm-generic/io.h

In reply to Thierry Reding

From: Thierry Reding <[email protected]>

Include the generic I/O header file so that duplicate implementations can be removed. This will also help to establish consistency across more architectures regarding which accessors they support.

Acked-by Catalin Marinas <[email protected]>

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v3:
- add io{read,write}{16,32}be() to match what 32-bit ARM does

 arch/arm64/Kconfig              |   1 -
 arch/arm64/include/asm/io.h     | 108 +++++++---------------------------------
 arch/arm64/include/asm/memory.h |   2 +
 3 files changed, 20 insertions(+), 91 deletions(-)

arch/arm64/Kconfig +0 -1

index 02cd76f6d7b4..3fa250534e16 100644
@@ -21,7 +21,6 @@ config ARM64
 	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
 	select GENERIC_CPU_AUTOPROBE
 	select GENERIC_EARLY_IOREMAP
-	select GENERIC_IOMAP
 	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_SHOW
 	select GENERIC_SCHED_CLOCK

arch/arm64/include/asm/io.h +18 ~~-90~~

164 lines not on this page · raw message

arch/arm64/include/asm/memory.h +2 -0

index 993bce527b85..ca6116de3155 100644
@@ -122,11 +122,13 @@ extern phys_addr_t		memstart_addr;
  * translation for translating DMA addresses.  Use the driver
  * DMA support - see dma-mapping.h.
  */
+#define virt_to_phys virt_to_phys
 static inline phys_addr_t virt_to_phys(const volatile void *x)
 {
 	return __virt_to_phys((unsigned long)(x));
 }
 
+#define phys_to_virt phys_to_virt
 static inline void *phys_to_virt(phys_addr_t x)
 {
 	return (void *)(__phys_to_virt(x));

Signature · 1 line

2.0.1

Catalin Marinas

maintainer

2014-07-17 12:04 UTC #4 lore raw

Re: [PATCH v3 3/3] arm64: Use include/asm-generic/io.h

Quoted 11 lines · Thierry Reding

From: Thierry Reding <[email protected]>
Include the generic I/O header file so that duplicate implementations can be removed. This will also help to establish consistency across more architectures regarding which accessors they support.
Acked-by Catalin Marinas <[email protected]>
Signed-off-by Thierry Reding <[email protected]>
---
Changes in v3:
- add io{read,write}{16,32}be() to match what 32-bit ARM does

It looks fine. Thanks.

Signature · 1 line

Catalin

Thierry Reding

author

2014-07-17 12:26 UTC #5 lore raw

Re: [PATCH v3 3/3] arm64: Use include/asm-generic/io.h

Quoted 7 lines · Catalin Marinas

Quoted 6 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Include the generic I/O header file so that duplicate implementations can be removed. This will also help to establish consistency across more architectures regarding which accessors they support.

Quoted 7 lines

Quoted 5 lines
Acked-by Catalin Marinas <[email protected]>
Signed-off-by Thierry Reding <[email protected]>
---
Changes in v3:
- add io{read,write}{16,32}be() to match what 32-bit ARM does
It looks fine. Thanks.

Great. Russell hasn't commented on this yet, anyone know how to get his attention?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140717/903ffc89/attachment.sig>

Catalin Marinas

maintainer

2014-07-18 15:50 UTC #6 lore raw

Re: [PATCH v3 3/3] arm64: Use include/asm-generic/io.h

Quoted 14 lines · Thierry Reding

Quoted 13 lines · Catalin Marinas
Quoted 11 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Include the generic I/O header file so that duplicate implementations can be removed. This will also help to establish consistency across more architectures regarding which accessors they support.
Acked-by Catalin Marinas <[email protected]>
Signed-off-by Thierry Reding <[email protected]>
---
Changes in v3:
- add io{read,write}{16,32}be() to match what 32-bit ARM does

Quoted 4 lines

Quoted 1 line
It looks fine. Thanks.
Great. Russell hasn't commented on this yet, anyone know how to get his attention?

Given he dependency on the first patch, they should either go in together or first patch in 3.17 and the rest later (but I would prefer that they all go in at the same time).

I don't mind how they are merged (the arm32 tree, arm-soc or arm64). If Russell doesn't have an opinion, I can take the 1st and 3rd patches via the arm64 tree.

Signature · 1 line

Catalin

Catalin Marinas

maintainer Acked-by on v3 1/3

2014-07-17 12:01 UTC #7 lore raw

In reply to Thierry Reding

Quoted 28 lines · Thierry Reding

From: Thierry Reding <[email protected]>
Currently driver writers need to use io{read,write}{8,16,32}_rep() when accessing FIFO registers portably. This is bad for two reasons: it is inconsistent with how other registers are accessed using the standard {read,write}{b,w,l}() functions, which can lead to confusion. On some architectures the io{read,write}*() functions also need to perform some extra checks to determine whether an address is memory-mapped or refers to I/O space. Drivers which can be expected to never use I/O can safely use the {read,write}s{b,w,l,q}(), just like they use their non-string variants and there's no need for these extra checks.
This patch implements generic versions of readsb(), readsw(), readsl(), readsq(), writesb(), writesw(), writesl() and writesq(). Variants of these string functions for I/O accesses (ins*() and outs*() as well as ioread*_rep() and iowrite*_rep()) are now implemented in terms of the new functions.
Going forward, {read,write}{,s}{b,w,l,q}() should be used consistently by drivers for devices that will only ever be memory-mapped and hence don't need to access I/O space, whereas io{read,write}{8,16,32}_rep() should be used by drivers for devices that can be either memory-mapped or I/O-mapped.
While at it, also make sure that any of the functions provided as fallback for architectures that don't override them can't be overridden subsequently.

Quoted 7 lines

This is compile- and runtime-tested on 32-bit and 64-bit ARM and compile tested on Microblaze, s390, SPARC and Xtensa. For ARC, Blackfin, Metag, OpenRISC, Score and Unicore32 which also use asm-generic/io.h I couldn't find or build a cross-compiler that would run on my system. But by code inspection they shouldn't break with this patch.
Signed-off-by Thierry Reding <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Sam Ravnborg

2014-07-18 20:59 UTC #8 lore raw

In reply to Thierry Reding

Quoted 28 lines · Thierry Reding

From: Thierry Reding <[email protected]>
Currently driver writers need to use io{read,write}{8,16,32}_rep() when accessing FIFO registers portably. This is bad for two reasons: it is inconsistent with how other registers are accessed using the standard {read,write}{b,w,l}() functions, which can lead to confusion. On some architectures the io{read,write}*() functions also need to perform some extra checks to determine whether an address is memory-mapped or refers to I/O space. Drivers which can be expected to never use I/O can safely use the {read,write}s{b,w,l,q}(), just like they use their non-string variants and there's no need for these extra checks.
This patch implements generic versions of readsb(), readsw(), readsl(), readsq(), writesb(), writesw(), writesl() and writesq(). Variants of these string functions for I/O accesses (ins*() and outs*() as well as ioread*_rep() and iowrite*_rep()) are now implemented in terms of the new functions.
Going forward, {read,write}{,s}{b,w,l,q}() should be used consistently by drivers for devices that will only ever be memory-mapped and hence don't need to access I/O space, whereas io{read,write}{8,16,32}_rep() should be used by drivers for devices that can be either memory-mapped or I/O-mapped.
While at it, also make sure that any of the functions provided as fallback for architectures that don't override them can't be overridden subsequently.

Quoted 7 lines

This is compile- and runtime-tested on 32-bit and 64-bit ARM and compile tested on Microblaze, s390, SPARC and Xtensa. For ARC, Blackfin, Metag, OpenRISC, Score and Unicore32 which also use asm-generic/io.h I couldn't find or build a cross-compiler that would run on my system. But by code inspection they shouldn't break with this patch.
Signed-off-by Thierry Reding <[email protected]>

Hi Thierry.

While browsing the resulting asm-generic/io.h it is irritating that the functions are messed up like they are.

From the start of the file the IO accessors are defined in following order:
__raw_readb
__raw_readw
__raw_readl

readb
readw
readl

__raw_writeb
__raw_writew
__raw_writel

writeb
writew
writel

__raw_readq

readq

__raw_writeq

writeq

See how strange it looks?

A semi related question.
Why do we play all these macro tricks in io.h?

Example:

    #define writeb __raw_writeb

The consensus these days is to use static inline to have the correct prototype but this file is contains a lot of these macro conversions.

All these things are not introduced by your patch but now that you show some love and care for this file maybe we could take the next step and bring more order to the current semi chaos?

Sam

Sam Ravnborg

2014-07-18 21:06 UTC #9 lore raw

Quoted 4 lines

All these things are not introduced by your patch but now that you show some love and care for this file maybe we could take the next step and bring more order to the current semi chaos?

And I just noticed...

These are never used - and only defined a few places:

#define inb_p(addr)     inb(addr)
#define inw_p(addr)     inw(addr)
#define inl_p(addr)     inl(addr)

#define outb_p(x, addr) outb((x), (addr))
#define outw_p(x, addr) outw((x), (addr))
#define outl_p(x, addr) outl((x), (addr))

Likewise for these:

#define insb_p(port,to,len)     insb(port,to,len)
#define insw_p(port,to,len)     insw(port,to,len)
#define insl_p(port,to,len)     insl(port,to,len)

#define outsb_p(port,from,len)  outsb(port,from,len)
#define outsw_p(port,from,len)  outsw(port,from,len)
#define outsl_p(port,from,len)  outsl(port,from,len)

This set:

#define inb_p(addr)     inb(addr)
#define inw_p(addr)     inw(addr)
#define inl_p(addr)     inl(addr)

#define outb_p(x, addr) outb((x), (addr))
#define outw_p(x, addr) outw((x), (addr))
#define outl_p(x, addr) outl((x), (addr))

Should have a comment that say they are deprecated. Especially the "b" variants still have many users.

Sam

Arnd Bergmann

maintainer

2014-07-19 07:38 UTC #10 lore raw

Quoted 30 lines · On Friday 18 July 2014 23:06:31 Sam Ravnborg wrote:

Quoted 4 lines
All these things are not introduced by your patch but now that you show some love and care for this file maybe we could take the next step and bring more order to the current semi chaos?
And I just noticed...
These are never used - and only defined a few places:
#define inb_p(addr)     inb(addr)
#define inw_p(addr)     inw(addr)
#define inl_p(addr)     inl(addr)
#define outb_p(x, addr) outb((x), (addr))
#define outw_p(x, addr) outw((x), (addr))
#define outl_p(x, addr) outl((x), (addr))
Likewise for these:
#define insb_p(port,to,len)     insb(port,to,len)
#define insw_p(port,to,len)     insw(port,to,len)
#define insl_p(port,to,len)     insl(port,to,len)
#define outsb_p(port,from,len)  outsb(port,from,len)
#define outsw_p(port,from,len)  outsw(port,from,len)
#define outsl_p(port,from,len)  outsl(port,from,len)
This set:
#define inb_p(addr)     inb(addr)
#define inw_p(addr)     inw(addr)
#define inl_p(addr)     inl(addr)

Quoted 6 lines

#define outb_p(x, addr) outb((x), (addr))
#define outw_p(x, addr) outw((x), (addr))
#define outl_p(x, addr) outl((x), (addr))
Should have a comment that say they are deprecated. Especially the "b" variants still have many users.

Are they? I don't remember ever seeing a reason to deprecate them. We could perhaps enclose them in #ifdef CONFIG_ISA, but there may also be some drivers that use the same code for ISA and PCI, and it doesn't really hurt on PCI.

Arnd

Sam Ravnborg

2014-07-19 08:41 UTC #11 lore raw

Quoted 9 lines

Quoted 9 lines
This set:
#define inb_p(addr)     inb(addr)
#define inw_p(addr)     inw(addr)
#define inl_p(addr)     inl(addr)
#define outb_p(x, addr) outb((x), (addr))
#define outw_p(x, addr) outw((x), (addr))
#define outl_p(x, addr) outl((x), (addr))

Quoted 7 lines

Quoted 2 lines
Should have a comment that say they are deprecated. Especially the "b" variants still have many users.
Are they? I don't remember ever seeing a reason to deprecate them. We could perhaps enclose them in #ifdef CONFIG_ISA, but there may also be some drivers that use the same code for ISA and PCI, and it doesn't really hurt on PCI.

It is my understanding that inl and inl_p are the same these days. A quick grep indicate that only m68k define the
_p variant different from the other.
But I failed to find and description of the difference between the two which is why I assumed they were identical and thus no need for both.

Sam

Arnd Bergmann

maintainer

2014-07-19 09:05 UTC #12 lore raw

Quoted 17 lines · On Saturday 19 July 2014 10:41:52 Sam Ravnborg wrote:

Quoted 9 lines
Quoted 9 lines
This set:
#define inb_p(addr)     inb(addr)
#define inw_p(addr)     inw(addr)
#define inl_p(addr)     inl(addr)
#define outb_p(x, addr) outb((x), (addr))
#define outw_p(x, addr) outw((x), (addr))
#define outl_p(x, addr) outl((x), (addr))
Quoted 7 lines
Quoted 2 lines
Should have a comment that say they are deprecated. Especially the "b" variants still have many users.
Are they? I don't remember ever seeing a reason to deprecate them. We could perhaps enclose them in #ifdef CONFIG_ISA, but there may also be some drivers that use the same code for ISA and PCI, and it doesn't really hurt on PCI.

Quoted 5 lines

It is my understanding that inl and inl_p are the same these days. A quick grep indicate that only m68k define the
_p variant different from the other.
But I failed to find and description of the difference between the two which is why I assumed they were identical and thus no need for both.

I don't know why m68k needs it, it's really an x86-specific thing, see slow_down_io() in arch/x86/include/asm/io.h.

Arnd

Sam Ravnborg

2014-07-19 09:11 UTC #13 lore raw

Quoted 18 lines · Arnd Bergmann

Quoted 17 lines · On Saturday 19 July 2014 10:41:52 Sam Ravnborg wrote:
Quoted 16 lines
Quoted 11 lines
This set:
#define inb_p(addr)     inb(addr)
#define inw_p(addr)     inw(addr)
#define inl_p(addr)     inl(addr)
#define outb_p(x, addr) outb((x), (addr))
#define outw_p(x, addr) outw((x), (addr))
#define outl_p(x, addr) outl((x), (addr))
Should have a comment that say they are deprecated. Especially the "b" variants still have many users.
Are they? I don't remember ever seeing a reason to deprecate them. We could perhaps enclose them in #ifdef CONFIG_ISA, but there may also be some drivers that use the same code for ISA and PCI, and it doesn't really hurt on PCI.

Quoted 8 lines

Quoted 5 lines
It is my understanding that inl and inl_p are the same these days. A quick grep indicate that only m68k define the
_p variant different from the other.
But I failed to find and description of the difference between the two which is why I assumed they were identical and thus no need for both.
I don't know why m68k needs it, it's really an x86-specific thing, see slow_down_io() in arch/x86/include/asm/io.h.

I had missed the x86 versions when grepping. Hmm, and with the macro tricks they play in asm/io.h this file is not at all grep friendly.

So xxx_p is for pause (or something like that).
This also matches that m68k do some tricks with delay() in the _p variants. Thanks for the explanation.

Sam

James Bottomley

2014-07-19 17:21 UTC #14 lore raw

Quoted 28 lines · Sam Ravnborg

Quoted 18 lines · Arnd Bergmann
Quoted 17 lines · On Saturday 19 July 2014 10:41:52 Sam Ravnborg wrote:
Quoted 16 lines
Quoted 11 lines
This set:
#define inb_p(addr)     inb(addr)
#define inw_p(addr)     inw(addr)
#define inl_p(addr)     inl(addr)
#define outb_p(x, addr) outb((x), (addr))
#define outw_p(x, addr) outw((x), (addr))
#define outl_p(x, addr) outl((x), (addr))
Should have a comment that say they are deprecated. Especially the "b" variants still have many users.
Are they? I don't remember ever seeing a reason to deprecate them. We could perhaps enclose them in #ifdef CONFIG_ISA, but there may also be some drivers that use the same code for ISA and PCI, and it doesn't really hurt on PCI.
Quoted 8 lines
Quoted 5 lines
It is my understanding that inl and inl_p are the same these days. A quick grep indicate that only m68k define the
_p variant different from the other.
But I failed to find and description of the difference between the two which is why I assumed they were identical and thus no need for both.
I don't know why m68k needs it, it's really an x86-specific thing, see slow_down_io() in arch/x86/include/asm/io.h.

Quoted 7 lines

I had missed the x86 versions when grepping. Hmm, and with the macro tricks they play in asm/io.h this file is not at all grep friendly.
So xxx_p is for pause (or something like that).
This also matches that m68k do some tricks with delay() in the _p variants. Thanks for the explanation.

Yes, the original x86 problem is that the CPU has a direct IO output bus which you can connect to. The in/out instructions activate the I/O pin of the CPU indicating the address should be decoded by the device space. Some devices are two slow to latch at the CPU speed, so if you do a succession of in's or out's, particularly to mailbox locations, the device misses some of the I/O transactions. Adding the delay slows the sequence down to the point where the device can react to it.

I/O cycle addressing is also problematic for architectures which don't have an in/out instruction or the concept of I/O access (parisc uses a special bridge to translate memory access to I/O cycles).

In theory, modern devices should be memory mapped (so capable of reacting at CPU bus speed) but the PCI spec keeps the concept of I/O mapping (triggering an I/O cycle instead of a memory one). Most modern PCI devices offer both memory and I/O mapped access and we always choose the memory mapped one in that case. Any device supporting both would never need the delay. However, the PCI spec still preserves the concept of a legacy device that only has I/O ports, which would possibly need the delay ... the PCI SIG keeps threatening to deprecate the I/O BAR, but haven't yet done so yes (as of PCIe version 3.0).

James

Geert Uytterhoeven

2014-08-05 09:07 UTC #15 lore raw

In reply to Sam Ravnborg

Quoted 28 lines · Sam Ravnborg

Quoted 18 lines · Arnd Bergmann
Quoted 17 lines · On Saturday 19 July 2014 10:41:52 Sam Ravnborg wrote:
Quoted 16 lines
Quoted 11 lines
This set:
#define inb_p(addr)     inb(addr)
#define inw_p(addr)     inw(addr)
#define inl_p(addr)     inl(addr)
#define outb_p(x, addr) outb((x), (addr))
#define outw_p(x, addr) outw((x), (addr))
#define outl_p(x, addr) outl((x), (addr))
Should have a comment that say they are deprecated. Especially the "b" variants still have many users.
Are they? I don't remember ever seeing a reason to deprecate them. We could perhaps enclose them in #ifdef CONFIG_ISA, but there may also be some drivers that use the same code for ISA and PCI, and it doesn't really hurt on PCI.
Quoted 8 lines
Quoted 5 lines
It is my understanding that inl and inl_p are the same these days. A quick grep indicate that only m68k define the
_p variant different from the other.
But I failed to find and description of the difference between the two which is why I assumed they were identical and thus no need for both.
I don't know why m68k needs it, it's really an x86-specific thing, see slow_down_io() in arch/x86/include/asm/io.h.

Quoted 7 lines

I had missed the x86 versions when grepping. Hmm, and with the macro tricks they play in asm/io.h this file is not at all grep friendly.
So xxx_p is for pause (or something like that).
This also matches that m68k do some tricks with delay() in the _p variants. Thanks for the explanation.

m68k's isa_delay() uses the same approach as x86's slow_down_io(), but only for Q40/Q60, which has a "real" ISA bus that accepts legacy ISA expansion cards (http://www.q40.de/).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Sam Ravnborg

2014-08-05 09:14 UTC #16 lore raw

Quoted 8 lines

Quoted 4 lines
So xxx_p is for pause (or something like that).
This also matches that m68k do some tricks with delay() in the _p variants. Thanks for the explanation.
m68k's isa_delay() uses the same approach as x86's slow_down_io(), but only for Q40/Q60, which has a "real" ISA bus that accepts legacy ISA expansion cards (http://www.q40.de/).

Based on the nice description from James I have on my todo list to cook up a patch to io.h to document this.
Had just not got around to do it yet.

Sam

Arnd Bergmann

maintainer

2014-07-19 07:44 UTC #17 lore raw

In reply to Sam Ravnborg

Quoted 61 lines · On Friday 18 July 2014 22:59:53 Sam Ravnborg wrote:

Quoted 28 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Currently driver writers need to use io{read,write}{8,16,32}_rep() when accessing FIFO registers portably. This is bad for two reasons: it is inconsistent with how other registers are accessed using the standard {read,write}{b,w,l}() functions, which can lead to confusion. On some architectures the io{read,write}*() functions also need to perform some extra checks to determine whether an address is memory-mapped or refers to I/O space. Drivers which can be expected to never use I/O can safely use the {read,write}s{b,w,l,q}(), just like they use their non-string variants and there's no need for these extra checks.
This patch implements generic versions of readsb(), readsw(), readsl(), readsq(), writesb(), writesw(), writesl() and writesq(). Variants of these string functions for I/O accesses (ins*() and outs*() as well as ioread*_rep() and iowrite*_rep()) are now implemented in terms of the new functions.
Going forward, {read,write}{,s}{b,w,l,q}() should be used consistently by drivers for devices that will only ever be memory-mapped and hence don't need to access I/O space, whereas io{read,write}{8,16,32}_rep() should be used by drivers for devices that can be either memory-mapped or I/O-mapped.
While at it, also make sure that any of the functions provided as fallback for architectures that don't override them can't be overridden subsequently.
Quoted 7 lines
This is compile- and runtime-tested on 32-bit and 64-bit ARM and compile tested on Microblaze, s390, SPARC and Xtensa. For ARC, Blackfin, Metag, OpenRISC, Score and Unicore32 which also use asm-generic/io.h I couldn't find or build a cross-compiler that would run on my system. But by code inspection they shouldn't break with this patch.
Signed-off-by Thierry Reding <[email protected]>
Hi Thierry.
While browsing the resulting asm-generic/io.h it is irritating that the functions are messed up like they are.
From the start of the file the IO accessors are defined in following order:
__raw_readb
__raw_readw
__raw_readl
readb
readw
readl
__raw_writeb
__raw_writew
__raw_writel
writeb
writew
writel
__raw_readq

Quoted 8 lines

readq
__raw_writeq
writeq
See how strange it looks?

It saves one #ifdef CONFIG_64BIT to do it like this, but I guess you are right that reordering them slightly would be nice here.

Quoted 7 lines

A semi related question.
Why do we play all these macro tricks in io.h?
Example:
    #define writeb __raw_writeb

Quoted 7 lines

The consensus these days is to use static inline to have the correct prototype but this file is contains a lot of these macro conversions.
All these things are not introduced by your patch but now that you show some love and care for this file maybe we could take the next step and bring more order to the current semi chaos?

The macros are mainly there so we can test their presence with #ifdef. The interface is complex enough that there is probably an architecture for any combination of overrides: most should override the __raw_*() functions with inline assembly, but a lot don't do that and it works because of implementation details of the compiler. Some may need to override either readl() or readl_relaxed() but not the other one.

For this reason, we want architecture-level files that include the asm-generic version to use macros (or macro + inline function) rather than a plain inline.

I was arguing earlier that we don't need the extra macros in the asm-generic version, but it also doesn't hurt and it can make it slightly easier for new architectures to copy the bits from asm-generic they want to override.

Arnd

Sam Ravnborg

2014-07-19 08:53 UTC #18 lore raw

Quoted 11 lines

Quoted 11 lines
A semi related question.
Why do we play all these macro tricks in io.h?
Example:
    #define writeb __raw_writeb
The consensus these days is to use static inline to have the correct prototype but this file is contains a lot of these macro conversions.

Quoted 6 lines

Quoted 3 lines
All these things are not introduced by your patch but now that you show some love and care for this file maybe we could take the next step and bring more order to the current semi chaos?
The macros are mainly there so we can test their presence with #ifdef.

There are two types of macros in io.h:

    #define __raw_writeb __raw_writeb
    #ifndef __raw_writeb

This is the one you talk about which allows an architecture to provide a local implementation of a function.
These are prefectly OK and are required.

Then there are the other type where one IO access function may re-use the implementation of another IO access function:

    #ifndef writeb
    #define writeb __raw_writeb
    #endif

This could have been implmented like this:

#ifndef writeb
#define writeb writeb
static inline void writeb(u8 b, volatile void __iomem *addr) {
__raw_writeb(b, addr);
}
#endif

In this way the prototype of the function is easy to understand and we avoid the macro tricks were we blindly replace one function name, with another function name.
And we also use the same pattarn all over for the various functions.

Concerning the efficiency the compiler should be smart enough to do the same independent on the two implmentations.

The only drawback I see is that the above is 6 lines of codes, where the macro expansion is 3 lines of code.

And when we talk about simpler expansion like ioread8() then we replace one line with 4 lines.

Sam

Arnd Bergmann

maintainer

2014-07-19 09:10 UTC #19 lore raw

Quoted 18 lines · On Saturday 19 July 2014 10:53:38 Sam Ravnborg wrote:

Then there are the other type where one IO access function may re-use the implementation of another IO access function:
    #ifndef writeb
    #define writeb __raw_writeb
    #endif
This could have been implmented like this:
#ifndef writeb
#define writeb writeb
static inline void writeb(u8 b, volatile void __iomem *addr)
{
   __raw_writeb(b, addr);
}
#endif

Quoted 7 lines

In this way the prototype of the function is easy to understand and we avoid the macro tricks were we blindly replace one function name, with another function name.
And we also use the same pattarn all over for the various functions.
Concerning the efficiency the compiler should be smart enough to do the same independent on the two implmentations.

I really don't have a strong opinion on those, as you say one is a little shorter and the other is a little more readable, so my preference in a case like this is to leave it up to the person who last touches the code and let them decide.

Arnd

Sam Ravnborg

2014-07-19 12:59 UTC #20 lore raw

PATCH 1/1 asm-generic/io.h: reorder funtions to form logical groups

In reply to Sam Ravnborg

Quoted 1 line

From 929c64c1aaf378b767e0ed89826b6bb12449df15 Mon Sep 17 00:00:00 2001

From: Sam Ravnborg <[email protected]>
Date: Sat, 19 Jul 2014 14:47:43 +0200
Subject: [PATCH] asm-generic/io.h: reorder funtions to form logical groups

Reoder the functions so the various functions are grouped according to how they access memoy.
For example __raw_{read,write}* are now all grouped.

The benefit of this grouping is that one can easier find all IO accessors of one type.
To do so a few more #ifdef CONFIG_64BIT had to be used.

Add a small boiler plate comment for some of the groups to better let them stand out.

Signed-off-by Sam Ravnborg <[email protected]>

---

Hi Thierry.

This is my attempt to bring some order into io.h with respect to the order the functions are defined in.

In a follow-up mail I also said we should delete the _p variants of some methods but I then learned they are for slow IO access. So these I have left as is.

And introducing static inline for all functions that are pure macro substitution is also left out for now.

Please consider if you will take this as a follow-on patch.

Sam

 include/asm-generic/io.h | 126 +++++++++++++++++++++++++++--------------------
 1 file changed, 73 insertions(+), 53 deletions(-)

include/asm-generic/io.h +73 ~~-53~~

index b2ea16b..5c84db4 100644
@@ -24,10 +24,10 @@
 #define mmiowb() do {} while (0)
 #endif
 
-/*****************************************************************************/
 /*
- * readX/writeX() are used to access memory mapped devices. On some
- * architectures the memory mapped IO stuff needs to be accessed
+ * raw_{read,write}{b,w,l,q} access memory in native endian.
+ *
+ * On some architectures the memory mapped IO stuff needs to be accessed
  * differently. On the simple architectures, we just read/write the
  * memory location directly.
  */

@@ -55,25 +55,16 @@ static inline u32 __raw_readl(const volatile void __iomem *addr)
 }
 #endif
 
-#ifndef readb
-#define readb __raw_readb
-#endif
-
-#ifndef readw
-#define readw readw
-static inline u16 readw(const volatile void __iomem *addr)
+#ifdef CONFIG_64BIT
+#ifndef __raw_readq
+#define __raw_readq __raw_readq
+static inline u64 __raw_readq(const volatile void __iomem *addr)
 {
-	return __le16_to_cpu(__raw_readw(addr));
+	return *(const volatile u64 __force *) addr;
 }
 #endif
+#endif /* CONFIG_64BIT */
 
-#ifndef readl
-#define readl readl
-static inline u32 readl(const volatile void __iomem *addr)
-{
-	return __le32_to_cpu(__raw_readl(addr));
-}
-#endif
 
 #ifndef __raw_writeb
 #define __raw_writeb __raw_writeb

@@ -99,27 +90,42 @@ static inline void __raw_writel(u32 b, volatile void __iomem *addr)
 }
 #endif
 
-#ifndef writeb
-#define writeb __raw_writeb
+#ifdef CONFIG_64BIT
+#ifndef __raw_writeq
+#define __raw_writeq __raw_writeq
+static inline void __raw_writeq(u64 b, volatile void __iomem *addr)
+{
+	*(volatile u64 __force *) addr = b;
+}
 #endif
+#endif /* CONFIG_64BIT */
 
-#ifndef writew
-#define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
+
+/*
+ * {read,write}{b,w,l,q} access little endian memory
+ * and return result in native endian
+ */
+#ifndef readb
+#define readb __raw_readb
 #endif
 
-#ifndef writel
-#define writel(b,addr) __raw_writel(__cpu_to_le32(b),addr)
+#ifndef readw
+#define readw readw
+static inline u16 readw(const volatile void __iomem *addr)
+{
+	return __le16_to_cpu(__raw_readw(addr));
+}
 #endif
 
-#ifdef CONFIG_64BIT
-#ifndef __raw_readq
-#define __raw_readq __raw_readq
-static inline u64 __raw_readq(const volatile void __iomem *addr)
+#ifndef readl
+#define readl readl
+static inline u32 readl(const volatile void __iomem *addr)
 {
-	return *(const volatile u64 __force *) addr;
+	return __le32_to_cpu(__raw_readl(addr));
 }
 #endif
 
+#ifdef CONFIG_64BIT
 #ifndef readq
 #define readq readq
 static inline u64 readq(const volatile void __iomem *addr)

@@ -127,20 +133,31 @@ static inline u64 readq(const volatile void __iomem *addr)
 	return __le64_to_cpu(__raw_readq(addr));
 }
 #endif
+#endif /* CONFIG_64BIT */
 
-#ifndef __raw_writeq
-#define __raw_writeq __raw_writeq
-static inline void __raw_writeq(u64 b, volatile void __iomem *addr)
-{
-	*(volatile u64 __force *) addr = b;
-}
+
+#ifndef writeb
+#define writeb __raw_writeb
+#endif
+
+#ifndef writew
+#define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
+#endif
+
+#ifndef writel
+#define writel(b,addr) __raw_writel(__cpu_to_le32(b),addr)
 #endif
 
+#ifdef CONFIG_64BIT
 #ifndef writeq
 #define writeq(b, addr) __raw_writeq(__cpu_to_le64(b), addr)
 #endif
 #endif /* CONFIG_64BIT */
 
+
+/*
+ * {read,write}s{b,w,l.q}b access native memory in chunks specified by count
+ */
 #ifndef readsb
 #define readsb readsb
 static inline void readsb(const void __iomem *addr, void *buffer, int count)

@@ -183,6 +200,23 @@ static inline void readsl(const void __iomem *addr, void *buffer, int count)
 }
 #endif
 
+#ifdef CONFIG_64BIT
+#ifndef readsq
+#define readsq readsq
+static inline void readsq(const void __iomem *addr, void *buffer, int count)
+{
+	if (count) {
+		u64 *buf = buffer;
+		do {
+			u64 x = __raw_readq(addr);
+			*buf++ = x;
+		} while (--count);
+	}
+}
+#endif
+#endif /* CONFIG_64BIT */
+
+
 #ifndef writesb
 #define writesb writesb
 static inline void writesb(void __iomem *addr, const void *buffer, int count)

@@ -223,20 +257,6 @@ static inline void writesl(void __iomem *addr, const void *buffer, int count)
 #endif
 
 #ifdef CONFIG_64BIT
-#ifndef readsq
-#define readsq readsq
-static inline void readsq(const void __iomem *addr, void *buffer, int count)
-{
-	if (count) {
-		u64 *buf = buffer;
-		do {
-			u64 x = __raw_readq(addr);
-			*buf++ = x;
-		} while (--count);
-	}
-}
-#endif
-
 #ifndef writesq
 #define writesq writesq
 static inline void writesq(void __iomem *addr, const void *buffer, int count)

@@ -356,6 +376,10 @@ static inline void outl(u32 b, unsigned long addr)
 #define ioread16(addr)		readw(addr)
 #define ioread32(addr)		readl(addr)
 
+#define iowrite8(v, addr)	writeb((v), (addr))
+#define iowrite16(v, addr)	writew((v), (addr))
+#define iowrite32(v, addr)	writel((v), (addr))
+
 #ifndef ioread16be
 #define ioread16be(addr)	__be16_to_cpu(__raw_readw(addr))
 #endif

@@ -364,10 +388,6 @@ static inline void outl(u32 b, unsigned long addr)
 #define ioread32be(addr)	__be32_to_cpu(__raw_readl(addr))
 #endif
 
-#define iowrite8(v, addr)	writeb((v), (addr))
-#define iowrite16(v, addr)	writew((v), (addr))
-#define iowrite32(v, addr)	writel((v), (addr))
-
 #ifndef iowrite16be
 #define iowrite16be(v, addr)	__raw_writew(__cpu_to_be16(v), addr)
 #endif

Signature · 1 line

1.9.3

Thierry Reding

author

2014-08-01 14:09 UTC #21 lore raw

Re: [PATCH] asm-generic/io.h: reorder funtions to form logical groups

Quoted 24 lines · Sam Ravnborg

From 929c64c1aaf378b767e0ed89826b6bb12449df15 Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <[email protected]>
Date: Sat, 19 Jul 2014 14:47:43 +0200
Subject: [PATCH] asm-generic/io.h: reorder funtions to form logical groups
Reoder the functions so the various functions are grouped according to how they access memoy.
For example __raw_{read,write}* are now all grouped.
The benefit of this grouping is that one can easier find all IO accessors of one type.
To do so a few more #ifdef CONFIG_64BIT had to be used.
Add a small boiler plate comment for some of the groups to better let them stand out.
Signed-off-by Sam Ravnborg <[email protected]>
---
Hi Thierry.
This is my attempt to bring some order into io.h with respect to the order the functions are defined in.

Quoted 8 lines

In a follow-up mail I also said we should delete the _p variants of some methods but I then learned they are for slow IO access. So these I have left as is.
And introducing static inline for all functions that are pure macro substitution is also left out for now.
Please consider if you will take this as a follow-on patch.

Just a short update on this: I've pulled your patch into my series and then thought I'd make a last pass over these and send out a new version but then I started noticing a few more things that I thought I could quickly clean up as well. One thing led to another and I'm now up to double the number of patches. I still think it's worth doing, but unfortunately it's become somewhat more complicated since it now touches a couple more architectures and even some drivers (/dev/mem (!) and sunzilog).

I'm almost through sorting out the remaining issues I think, but it'll take me another couple of days to rebase and squash patches and run some build tests to verify that things don't break somewhere in the middle.

Oh, by the way: would you prefer that I keep your patch separate or shall I just squash it into the one that has the major asm-generic/io.h cleanup?

Thierry

Sam Ravnborg

2014-08-01 22:42 UTC #22 lore raw

Re: [PATCH] asm-generic/io.h: reorder funtions to form logical groups

Quoted 4 lines

Oh, by the way: would you prefer that I keep your patch separate or shall I just squash it into the one that has the major asm-generic/io.h cleanup?

Do whatever is the most convinient for you.
And do not bother with any attribution and such for such a simple patch.

Sam