2014-04-17 13:47:45

by Will Deacon

[permalink] [raw]
Subject: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

Hello,

This RFC series attempts to define a portable (i.e. cross-architecture)
definition of the {readX,writeX}_relaxed MMIO accessor functions. These
functions are already in widespread use amongst drivers (mainly those supporting
devices embedded in ARM SoCs), but lack any well-defined semantics and,
subsequently, any portable definitions to allow these drivers to be compiled for
other architectures.

The two main motivations for this series are:

(1) To promote use of the _relaxed MMIO accessors on weakly-ordered
architectures, where they can bring significant performance improvements
over their non-relaxed counterparts.

(2) To allow COMPILE_TEST to build drivers using the relaxed accessors across
all architectures.

The proposed semantics largely match exactly those provided by the ARM
implementation (i.e. no weaker), with one exception (see below).

Informally:

- Relaxed accesses to the same device are ordered with respect to each other.

- Relaxed accesses are *not* guaranteed to be ordered with respect to normal
memory accesses (e.g. DMA buffers -- this is what gives us the performance
boost over the non-relaxed versions).

- Relaxed accesses are not guaranteed to be ordered with respect to
LOCK/UNLOCK operations.

In actual fact, the relaxed accessors *are* ordered with respect to LOCK/UNLOCK
operations on ARM[64], but I have added this constraint for the benefit of
PowerPC, which has expensive I/O barriers in the spin_unlock path for the
non-relaxed accessors.

A corollary to this is that mmiowb() probably needs rethinking. As it currently
stands, an mmiowb() is required to order MMIO writes to a device from multiple
CPUs, even if that device is protected by a lock. However, this isn't often used
in practice, leading to PowerPC implementing both mmiowb() *and* synchronising
I/O in spin_unlock.

I would propose making the non-relaxed I/O accessors ordered with respect to
LOCK/UNLOCK, leaving mmiowb() to be used with the relaxed accessors, if
required, but would welcome thoughts/suggestions on this topic.

All feedback welcome,

Will


Will Deacon (18):
asm-generic: io: implement relaxed accessor macros as conditional
wrappers
microblaze: io: remove dummy relaxed accessor macros
s390: io: remove dummy relaxed accessor macros for reads
xtensa: io: remove dummy relaxed accessor macros for reads
alpha: io: implement relaxed accessor macros for writes
frv: io: implement dummy relaxed accessor macros for writes
cris: io: implement dummy relaxed accessor macros for writes
ia64: io: implement dummy relaxed accessor macros for writes
m32r: io: implement dummy relaxed accessor macros for writes
m68k: io: implement dummy relaxed accessor macros for writes
mn10300: io: implement dummy relaxed accessor macros for writes
parisc: io: implement dummy relaxed accessor macros for writes
powerpc: io: implement dummy relaxed accessor macros for writes
sparc: io: implement dummy relaxed accessor macros for writes
tile: io: implement dummy relaxed accessor macros for writes
x86: io: implement dummy relaxed accessor macros for writes
documentation: memory-barriers: clarify relaxed io accessor semantics
asm-generic: io: define relaxed accessor macros unconditionally

Documentation/memory-barriers.txt | 13 +++++++++----
arch/alpha/include/asm/io.h | 12 ++++++++----
arch/cris/include/asm/io.h | 3 +++
arch/frv/include/asm/io.h | 3 +++
arch/ia64/include/asm/io.h | 4 ++++
arch/m32r/include/asm/io.h | 3 +++
arch/m68k/include/asm/io.h | 8 ++++++++
arch/m68k/include/asm/io_no.h | 4 ----
arch/microblaze/include/asm/io.h | 8 --------
arch/mn10300/include/asm/io.h | 4 ++++
arch/parisc/include/asm/io.h | 12 ++++++++----
arch/powerpc/include/asm/io.h | 12 ++++++++----
arch/s390/include/asm/io.h | 5 -----
arch/sparc/include/asm/io.h | 9 +++++++++
arch/sparc/include/asm/io_32.h | 3 ---
arch/sparc/include/asm/io_64.h | 22 ++++++++++------------
arch/tile/include/asm/io.h | 4 ++++
arch/x86/include/asm/io.h | 4 ++++
arch/xtensa/include/asm/io.h | 7 -------
include/asm-generic/io.h | 23 ++++++++++++++++-------
20 files changed, 101 insertions(+), 62 deletions(-)

--
1.9.1


2014-04-17 13:45:12

by Will Deacon

[permalink] [raw]
Subject: [PATCH 08/18] ia64: io: implement dummy relaxed accessor macros for writes

write{b,w,l,q}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to ia64, which may
be able to be optimised in a similar manner to the relaxed read
accessors at a later date.

Cc: Tony Luck <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/ia64/include/asm/io.h | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h
index 0d2bcb37ec35..379165154c27 100644
--- a/arch/ia64/include/asm/io.h
+++ b/arch/ia64/include/asm/io.h
@@ -393,6 +393,10 @@ __writeq (unsigned long val, volatile void __iomem *addr)
#define writew(v,a) __writew((v), (a))
#define writel(v,a) __writel((v), (a))
#define writeq(v,a) __writeq((v), (a))
+#define writeb_relaxed(v,a) __writeb((v), (a))
+#define writew_relaxed(v,a) __writew((v), (a))
+#define writel_relaxed(v,a) __writel((v), (a))
+#define writeq_relaxed(v,a) __writeq((v), (a))
#define __raw_writeb writeb
#define __raw_writew writew
#define __raw_writel writel
--
1.9.1

2014-04-17 13:45:29

by Will Deacon

[permalink] [raw]
Subject: [PATCH 14/18] sparc: io: implement dummy relaxed accessor macros for writes

write{b,w,l,q}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to sparc, in the
same vein as the dummy definitions for the relaxed read accessors. The
existing relaxed read{b,w,l} accessors are moved into asm/io.h, since
they are identical between 32-bit and 64-bit machines.

Cc: "David S. Miller" <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/sparc/include/asm/io.h | 9 +++++++++
arch/sparc/include/asm/io_32.h | 3 ---
arch/sparc/include/asm/io_64.h | 22 ++++++++++------------
3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/arch/sparc/include/asm/io.h b/arch/sparc/include/asm/io.h
index f6902cf3cbe9..493f22c4684f 100644
--- a/arch/sparc/include/asm/io.h
+++ b/arch/sparc/include/asm/io.h
@@ -10,6 +10,15 @@
* Defines used for both SPARC32 and SPARC64
*/

+/* Relaxed accessors for MMIO */
+#define readb_relaxed(__addr) readb(__addr)
+#define readw_relaxed(__addr) readw(__addr)
+#define readl_relaxed(__addr) readl(__addr)
+
+#define writeb_relaxed(__b, __addr) writeb(__b, __addr)
+#define writew_relaxed(__w, __addr) writew(__w, __addr)
+#define writel_relaxed(__l, __addr) writel(__l, __addr)
+
/* Big endian versions of memory read/write routines */
#define readb_be(__addr) __raw_readb(__addr)
#define readw_be(__addr) __raw_readw(__addr)
diff --git a/arch/sparc/include/asm/io_32.h b/arch/sparc/include/asm/io_32.h
index c1acbd891cbc..41d33e567f1d 100644
--- a/arch/sparc/include/asm/io_32.h
+++ b/arch/sparc/include/asm/io_32.h
@@ -89,9 +89,6 @@ static inline void __writel(u32 l, volatile void __iomem *addr)
#define readb(__addr) __readb(__addr)
#define readw(__addr) __readw(__addr)
#define readl(__addr) __readl(__addr)
-#define readb_relaxed(__addr) readb(__addr)
-#define readw_relaxed(__addr) readw(__addr)
-#define readl_relaxed(__addr) readl(__addr)

#define writeb(__b, __addr) __writeb((__b),(__addr))
#define writew(__w, __addr) __writew((__w),(__addr))
diff --git a/arch/sparc/include/asm/io_64.h b/arch/sparc/include/asm/io_64.h
index 09b0b88aeb2a..2b4cd52831d0 100644
--- a/arch/sparc/include/asm/io_64.h
+++ b/arch/sparc/include/asm/io_64.h
@@ -203,18 +203,16 @@ static inline void _writeq(u64 q, volatile void __iomem *addr)
: "memory");
}

-#define readb(__addr) _readb(__addr)
-#define readw(__addr) _readw(__addr)
-#define readl(__addr) _readl(__addr)
-#define readq(__addr) _readq(__addr)
-#define readb_relaxed(__addr) _readb(__addr)
-#define readw_relaxed(__addr) _readw(__addr)
-#define readl_relaxed(__addr) _readl(__addr)
-#define readq_relaxed(__addr) _readq(__addr)
-#define writeb(__b, __addr) _writeb(__b, __addr)
-#define writew(__w, __addr) _writew(__w, __addr)
-#define writel(__l, __addr) _writel(__l, __addr)
-#define writeq(__q, __addr) _writeq(__q, __addr)
+#define readb(__addr) _readb(__addr)
+#define readw(__addr) _readw(__addr)
+#define readl(__addr) _readl(__addr)
+#define readq(__addr) _readq(__addr)
+#define readq_relaxed(__addr) _readq(__addr)
+#define writeb(__b, __addr) _writeb(__b, __addr)
+#define writew(__w, __addr) _writew(__w, __addr)
+#define writel(__l, __addr) _writel(__l, __addr)
+#define writeq(__q, __addr) _writeq(__q, __addr)
+#define writeq_relaxed(__q, __addr) _writeq(__q, __addr)

/* Now versions without byte-swapping. */
static inline u8 _raw_readb(unsigned long addr)
--
1.9.1

2014-04-17 13:45:43

by Will Deacon

[permalink] [raw]
Subject: [PATCH 09/18] m32r: io: implement dummy relaxed accessor macros for writes

write{b,w,l}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to m32r, in the
same vein as the dummy definitions for the relaxed read accessors.

Cc: Hirokazu Takata <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/m32r/include/asm/io.h | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/m32r/include/asm/io.h b/arch/m32r/include/asm/io.h
index 4010f1fc5b65..6e7787f3dac7 100644
--- a/arch/m32r/include/asm/io.h
+++ b/arch/m32r/include/asm/io.h
@@ -161,6 +161,9 @@ static inline void _writel(unsigned long l, unsigned long addr)
#define __raw_writeb writeb
#define __raw_writew writew
#define __raw_writel writel
+#define writeb_relaxed writeb
+#define writew_relaxed writew
+#define writel_relaxed writel

#define ioread8 read
#define ioread16 readw
--
1.9.1

2014-04-17 13:45:10

by Will Deacon

[permalink] [raw]
Subject: [PATCH 17/18] documentation: memory-barriers: clarify relaxed io accessor semantics

This patch extends the paragraph describing the relaxed read io accessors
so that the relaxed accessors are defined to be:

- Ordered with respect to each other if accessing the same peripheral

- Unordered with respect to normal memory accesses

- Unordered with respect to LOCK/UNLOCK operations

Whilst many architectures will provide stricter semantics, ARM, Alpha and
PPC can achieve significant performance gains by taking advantage of some
or all of the above relaxations.

Cc: Randy Dunlap <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul E. McKenney <[email protected]>
Cc: David Howells <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
Documentation/memory-barriers.txt | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 556f951f8626..f31c88691ee9 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -2462,10 +2462,15 @@ functions:
Please refer to the PCI specification for more information on interactions
between PCI transactions.

- (*) readX_relaxed()
-
- These are similar to readX(), but are not guaranteed to be ordered in any
- way. Be aware that there is no I/O read barrier available.
+ (*) readX_relaxed(), writeX_relaxed()
+
+ These are similar to readX() and writeX(), but provide weaker memory
+ ordering guarantees. Specifically, they do not guarantee ordering with
+ respect to normal memory accesses (e.g. DMA buffers) nor do they guarantee
+ ordering with respect to LOCK or UNLOCK operations. If the latter is
+ required, an mmiowb() barrier can be used. Note that relaxed accesses to
+ the same peripheral are guaranteed to be ordered with respect to each
+ other.

(*) ioreadX(), iowriteX()

--
1.9.1

2014-04-17 13:45:08

by Will Deacon

[permalink] [raw]
Subject: [PATCH 01/18] asm-generic: io: implement relaxed accessor macros as conditional wrappers

{read,write}{b,w,l,q}_relaxed are implemented by some architectures in
order to permit memory-mapped I/O accesses with weaker barrier semantics
than the non-relaxed variants.

This patch adds wrappers to asm-generic so that drivers can rely on the
relaxed accessors being available, even if they don't always provide
weaker ordering guarantees. Since some architectures both include
asm-generic/io.h and define some relaxed accessors, the definitions here
are conditional for the time being.

Cc: Arnd Bergmann <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
include/asm-generic/io.h | 41 ++++++++++++++++++++++++++++++++++-------
1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 975e1cc75edb..6a93889aeb0d 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -52,15 +52,24 @@ static inline u32 __raw_readl(const volatile void __iomem *addr)
}
#endif

-#define readb __raw_readb
+#define readb __raw_readb
+#ifndef readb_relaxed
+#define readb_relaxed readb
+#endif

-#define readw readw
+#define readw readw
+#ifndef readw_relaxed
+#define readw_relaxed readw
+#endif
static inline u16 readw(const volatile void __iomem *addr)
{
return __le16_to_cpu(__raw_readw(addr));
}

-#define readl readl
+#define readl readl
+#ifndef readl_relaxed
+#define readl_relaxed readl
+#endif
static inline u32 readl(const volatile void __iomem *addr)
{
return __le32_to_cpu(__raw_readl(addr));
@@ -87,9 +96,21 @@ static inline void __raw_writel(u32 b, volatile void __iomem *addr)
}
#endif

-#define writeb __raw_writeb
-#define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
-#define writel(b,addr) __raw_writel(__cpu_to_le32(b),addr)
+#define writeb __raw_writeb
+#define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
+#define writel(b,addr) __raw_writel(__cpu_to_le32(b),addr)
+
+#ifndef writeb_relaxed
+#define writeb_relaxed writeb
+#endif
+
+#ifndef writew_relaxed
+#define writew_relaxed writew
+#endif
+
+#ifndef writel_relaxed
+#define writel_relaxed writel
+#endif

#ifdef CONFIG_64BIT
#ifndef __raw_readq
@@ -99,7 +120,10 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
}
#endif

-#define readq readq
+#define readq readq
+#ifndef readq_relaxed
+#define readq_relaxed readq
+#endif
static inline u64 readq(const volatile void __iomem *addr)
{
return __le64_to_cpu(__raw_readq(addr));
@@ -113,6 +137,9 @@ static inline void __raw_writeq(u64 b, volatile void __iomem *addr)
#endif

#define writeq(b, addr) __raw_writeq(__cpu_to_le64(b), addr)
+#ifndef writeq_relaxed
+#define writeq_relaxed writeq
+#endif
#endif /* CONFIG_64BIT */

#ifndef PCI_IOBASE
--
1.9.1

2014-04-17 13:45:05

by Will Deacon

[permalink] [raw]
Subject: [PATCH 16/18] x86: io: implement dummy relaxed accessor macros for writes

write{b,w,l,q}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to x86, in the
same vein as the dummy definitions for the relaxed read accessors.

Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/x86/include/asm/io.h | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8a1e0c..2ea07f5ec7b7 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -74,6 +74,9 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
#define __raw_readw __readw
#define __raw_readl __readl

+#define writeb_relaxed(v, a) __writeb(v, a)
+#define writew_relaxed(v, a) __writew(v, a)
+#define writel_relaxed(v, a) __writel(v, a)
#define __raw_writeb __writeb
#define __raw_writew __writew
#define __raw_writel __writel
@@ -86,6 +89,7 @@ build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
build_mmio_write(writeq, "q", unsigned long, "r", :"memory")

#define readq_relaxed(a) readq(a)
+#define writeq_relaxed(v, a) writeq(v, a)

#define __raw_readq(a) readq(a)
#define __raw_writeq(val, addr) writeq(val, addr)
--
1.9.1

2014-04-17 13:47:43

by Will Deacon

[permalink] [raw]
Subject: [PATCH 15/18] tile: io: implement dummy relaxed accessor macros for writes

write{b,w,l,q}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to tile, in the
same vein as the dummy definitions for the relaxed read accessors.

Cc: Chris Metcalf <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/tile/include/asm/io.h | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/tile/include/asm/io.h b/arch/tile/include/asm/io.h
index 9fe434969fab..d372641054d9 100644
--- a/arch/tile/include/asm/io.h
+++ b/arch/tile/include/asm/io.h
@@ -241,6 +241,10 @@ static inline void writeq(u64 val, unsigned long addr)
#define readw_relaxed readw
#define readl_relaxed readl
#define readq_relaxed readq
+#define writeb_relaxed writeb
+#define writew_relaxed writew
+#define writel_relaxed writel
+#define writeq_relaxed writeq

#define ioread8 readb
#define ioread16 readw
--
1.9.1

2014-04-17 13:45:00

by Will Deacon

[permalink] [raw]
Subject: [PATCH 04/18] xtensa: io: remove dummy relaxed accessor macros for reads

These are now defined by asm-generic/io.h, so we don't need the private
definitions anymore.

Cc: Chris Zankel <[email protected]>
Cc: Max Filippov <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/xtensa/include/asm/io.h | 7 -------
1 file changed, 7 deletions(-)

diff --git a/arch/xtensa/include/asm/io.h b/arch/xtensa/include/asm/io.h
index 74944207167e..fe1600a09438 100644
--- a/arch/xtensa/include/asm/io.h
+++ b/arch/xtensa/include/asm/io.h
@@ -74,13 +74,6 @@ static inline void iounmap(volatile void __iomem *addr)

#endif /* CONFIG_MMU */

-/*
- * Generic I/O
- */
-#define readb_relaxed readb
-#define readw_relaxed readw
-#define readl_relaxed readl
-
#endif /* __KERNEL__ */

#include <asm-generic/io.h>
--
1.9.1

2014-04-17 13:49:31

by Will Deacon

[permalink] [raw]
Subject: [PATCH 02/18] microblaze: io: remove dummy relaxed accessor macros

These are now defined by asm-generic/io.h, so we don't need the private
definitions anymore.

Cc: Michal Simek <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/microblaze/include/asm/io.h | 8 --------
1 file changed, 8 deletions(-)

diff --git a/arch/microblaze/include/asm/io.h b/arch/microblaze/include/asm/io.h
index 1e4c3329f62e..2987fd4c73d6 100644
--- a/arch/microblaze/include/asm/io.h
+++ b/arch/microblaze/include/asm/io.h
@@ -72,12 +72,4 @@ extern void __iomem *ioremap(phys_addr_t address, unsigned long size);

#include <asm-generic/io.h>

-#define readb_relaxed readb
-#define readw_relaxed readw
-#define readl_relaxed readl
-
-#define writeb_relaxed writeb
-#define writew_relaxed writew
-#define writel_relaxed writel
-
#endif /* _ASM_MICROBLAZE_IO_H */
--
1.9.1

2014-04-17 13:49:29

by Will Deacon

[permalink] [raw]
Subject: [PATCH 11/18] mn10300: io: implement dummy relaxed accessor macros for writes

write{b,w,l}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to mn10300, in the
same vein as the dummy definitions for the relaxed read accessors.

Cc: David Howells <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/mn10300/include/asm/io.h | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/mn10300/include/asm/io.h b/arch/mn10300/include/asm/io.h
index e6ed0d897ccc..897ba3c12b32 100644
--- a/arch/mn10300/include/asm/io.h
+++ b/arch/mn10300/include/asm/io.h
@@ -67,6 +67,10 @@ static inline void writel(u32 b, volatile void __iomem *addr)
#define __raw_writew writew
#define __raw_writel writel

+#define writeb_relaxed writeb
+#define writew_relaxed writew
+#define writel_relaxed writel
+
/*****************************************************************************/
/*
* traditional input/output functions
--
1.9.1

2014-04-17 13:49:27

by Will Deacon

[permalink] [raw]
Subject: [PATCH 12/18] parisc: io: implement dummy relaxed accessor macros for writes

write{b,w,l,q}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to parisc, in the
same vein as the dummy definitions for the relaxed read accessors.

Cc: Helge Deller <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/parisc/include/asm/io.h | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/parisc/include/asm/io.h b/arch/parisc/include/asm/io.h
index 1f6d2ae7aba5..8cd0abf28ffb 100644
--- a/arch/parisc/include/asm/io.h
+++ b/arch/parisc/include/asm/io.h
@@ -217,10 +217,14 @@ static inline void writeq(unsigned long long q, volatile void __iomem *addr)
#define writel writel
#define writeq writeq

-#define readb_relaxed(addr) readb(addr)
-#define readw_relaxed(addr) readw(addr)
-#define readl_relaxed(addr) readl(addr)
-#define readq_relaxed(addr) readq(addr)
+#define readb_relaxed(addr) readb(addr)
+#define readw_relaxed(addr) readw(addr)
+#define readl_relaxed(addr) readl(addr)
+#define readq_relaxed(addr) readq(addr)
+#define writeb_relaxed(b, addr) writeb(b, addr)
+#define writew_relaxed(w, addr) writew(w, addr)
+#define writel_relaxed(l, addr) writel(l, addr)
+#define writeq_relaxed(q, addr) writeq(q, addr)

#define mmiowb() do { } while (0)

--
1.9.1

2014-04-17 13:49:26

by Will Deacon

[permalink] [raw]
Subject: [PATCH 03/18] s390: io: remove dummy relaxed accessor macros for reads

These are now defined by asm-generic/io.h, so we don't need the private
definitions anymore.

Cc: Heiko Carstens <[email protected]>
Cc: Martin Schwidefsky <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/s390/include/asm/io.h | 5 -----
1 file changed, 5 deletions(-)

diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
index cd6b9ee7b69c..722befdf8332 100644
--- a/arch/s390/include/asm/io.h
+++ b/arch/s390/include/asm/io.h
@@ -60,11 +60,6 @@ static inline void iounmap(volatile void __iomem *addr)
#define __raw_writel zpci_write_u32
#define __raw_writeq zpci_write_u64

-#define readb_relaxed readb
-#define readw_relaxed readw
-#define readl_relaxed readl
-#define readq_relaxed readq
-
#endif /* CONFIG_PCI */

#include <asm-generic/io.h>
--
1.9.1

2014-04-17 13:49:24

by Will Deacon

[permalink] [raw]
Subject: [PATCH 18/18] asm-generic: io: define relaxed accessor macros unconditionally

Now that no architectures using asm-generic/io.h define their own relaxed
accessors, the dummy definitions can be used unconditionally.

Cc: Arnd Bergmann <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
include/asm-generic/io.h | 18 ------------------
1 file changed, 18 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 6a93889aeb0d..ad80cab2a758 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -53,23 +53,17 @@ static inline u32 __raw_readl(const volatile void __iomem *addr)
#endif

#define readb __raw_readb
-#ifndef readb_relaxed
#define readb_relaxed readb
-#endif

#define readw readw
-#ifndef readw_relaxed
#define readw_relaxed readw
-#endif
static inline u16 readw(const volatile void __iomem *addr)
{
return __le16_to_cpu(__raw_readw(addr));
}

#define readl readl
-#ifndef readl_relaxed
#define readl_relaxed readl
-#endif
static inline u32 readl(const volatile void __iomem *addr)
{
return __le32_to_cpu(__raw_readl(addr));
@@ -100,17 +94,9 @@ static inline void __raw_writel(u32 b, volatile void __iomem *addr)
#define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
#define writel(b,addr) __raw_writel(__cpu_to_le32(b),addr)

-#ifndef writeb_relaxed
#define writeb_relaxed writeb
-#endif
-
-#ifndef writew_relaxed
#define writew_relaxed writew
-#endif
-
-#ifndef writel_relaxed
#define writel_relaxed writel
-#endif

#ifdef CONFIG_64BIT
#ifndef __raw_readq
@@ -121,9 +107,7 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
#endif

#define readq readq
-#ifndef readq_relaxed
#define readq_relaxed readq
-#endif
static inline u64 readq(const volatile void __iomem *addr)
{
return __le64_to_cpu(__raw_readq(addr));
@@ -137,9 +121,7 @@ static inline void __raw_writeq(u64 b, volatile void __iomem *addr)
#endif

#define writeq(b, addr) __raw_writeq(__cpu_to_le64(b), addr)
-#ifndef writeq_relaxed
#define writeq_relaxed writeq
-#endif
#endif /* CONFIG_64BIT */

#ifndef PCI_IOBASE
--
1.9.1

2014-04-17 13:52:04

by Will Deacon

[permalink] [raw]
Subject: [PATCH 06/18] frv: io: implement dummy relaxed accessor macros for writes

write{b,w,l}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to frv, in the same
vein as the dummy definitions for the relaxed read accessors.

Cc: David Howells <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/frv/include/asm/io.h | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h
index 8cb50a2fbcb2..99bb7efaf9b7 100644
--- a/arch/frv/include/asm/io.h
+++ b/arch/frv/include/asm/io.h
@@ -243,6 +243,9 @@ static inline void writel(uint32_t datum, volatile void __iomem *addr)
__flush_PCI_writes();
}

+#define writeb_relaxed writeb
+#define writew_relaxed writew
+#define writel_relaxed writel

/* Values for nocacheflag and cmode */
#define IOMAP_FULL_CACHING 0
--
1.9.1

2014-04-17 13:52:02

by Will Deacon

[permalink] [raw]
Subject: [PATCH 07/18] cris: io: implement dummy relaxed accessor macros for writes

write{b,w,l}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to Cris, in the same
vein as the dummy definitions for the relaxed read accessors.

Cc: Mikael Starvik <[email protected]>
Cc: Jesper Nilsson <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/cris/include/asm/io.h | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/cris/include/asm/io.h b/arch/cris/include/asm/io.h
index e59dba12ce94..752a3f45df60 100644
--- a/arch/cris/include/asm/io.h
+++ b/arch/cris/include/asm/io.h
@@ -112,6 +112,9 @@ static inline void writel(unsigned int b, volatile void __iomem *addr)
else
*(volatile unsigned int __force *) addr = b;
}
+#define writeb_relaxed(b, addr) writeb(b, addr)
+#define writew_relaxed(b, addr) writew(b, addr)
+#define writel_relaxed(b, addr) writel(b, addr)
#define __raw_writeb writeb
#define __raw_writew writew
#define __raw_writel writel
--
1.9.1

2014-04-17 13:52:00

by Will Deacon

[permalink] [raw]
Subject: [PATCH 10/18] m68k: io: implement dummy relaxed accessor macros for writes

write{b,w,l}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to m68k, in the
same vein as the dummy definitions for the relaxed read accessors.
Additionally, the existing relaxed read accessors are moved into
asm/io.h, so that they can be used by m68k targets with an MMU.

Cc: Geert Uytterhoeven <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/m68k/include/asm/io.h | 8 ++++++++
arch/m68k/include/asm/io_no.h | 4 ----
2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/m68k/include/asm/io.h b/arch/m68k/include/asm/io.h
index c70cc9155003..bccd5a914eb6 100644
--- a/arch/m68k/include/asm/io.h
+++ b/arch/m68k/include/asm/io.h
@@ -3,3 +3,11 @@
#else
#include <asm/io_mm.h>
#endif
+
+#define readb_relaxed(addr) readb(addr)
+#define readw_relaxed(addr) readw(addr)
+#define readl_relaxed(addr) readl(addr)
+
+#define writeb_relaxed(b, addr) writeb(b, addr)
+#define writew_relaxed(b, addr) writew(b, addr)
+#define writel_relaxed(b, addr) writel(b, addr)
diff --git a/arch/m68k/include/asm/io_no.h b/arch/m68k/include/asm/io_no.h
index 52f7e8499172..19c237c63dc2 100644
--- a/arch/m68k/include/asm/io_no.h
+++ b/arch/m68k/include/asm/io_no.h
@@ -40,10 +40,6 @@ static inline unsigned int _swapl(volatile unsigned long v)
#define readl(addr) \
({ unsigned int __v = (*(volatile unsigned int *) (addr)); __v; })

-#define readb_relaxed(addr) readb(addr)
-#define readw_relaxed(addr) readw(addr)
-#define readl_relaxed(addr) readl(addr)
-
#define writeb(b,addr) (void)((*(volatile unsigned char *) (addr)) = (b))
#define writew(b,addr) (void)((*(volatile unsigned short *) (addr)) = (b))
#define writel(b,addr) (void)((*(volatile unsigned int *) (addr)) = (b))
--
1.9.1

2014-04-17 13:51:57

by Will Deacon

[permalink] [raw]
Subject: [PATCH 13/18] powerpc: io: implement dummy relaxed accessor macros for writes

write{b,w,l,q}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O accesses with weaker barrier semantics than the
non-relaxed variants.

This patch adds dummy macros for the write accessors to powerpc, in the
same vein as the dummy definitions for the relaxed read accessors.

Cc: Benjamin Herrenschmidt <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/powerpc/include/asm/io.h | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 97d3869991ca..9eaf301ac952 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -617,10 +617,14 @@ static inline void name at \
/*
* We don't do relaxed operations yet, at least not with this semantic
*/
-#define readb_relaxed(addr) readb(addr)
-#define readw_relaxed(addr) readw(addr)
-#define readl_relaxed(addr) readl(addr)
-#define readq_relaxed(addr) readq(addr)
+#define readb_relaxed(addr) readb(addr)
+#define readw_relaxed(addr) readw(addr)
+#define readl_relaxed(addr) readl(addr)
+#define readq_relaxed(addr) readq(addr)
+#define writeb_relaxed(v, addr) writeb(v, addr)
+#define writew_relaxed(v, addr) writew(v, addr)
+#define writel_relaxed(v, addr) writel(v, addr)
+#define writeq_relaxed(v, addr) writeq(v, addr)

#ifdef CONFIG_PPC32
#define mmiowb()
--
1.9.1

2014-04-17 14:01:25

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

On Thu, Apr 17, 2014 at 02:44:03PM +0100, Will Deacon wrote:
> Hello,
>
> This RFC series attempts to define a portable (i.e. cross-architecture)
> definition of the {readX,writeX}_relaxed MMIO accessor functions. These
> functions are already in widespread use amongst drivers (mainly those supporting
> devices embedded in ARM SoCs), but lack any well-defined semantics and,
> subsequently, any portable definitions to allow these drivers to be compiled for
> other architectures.
>
> The two main motivations for this series are:
>
> (1) To promote use of the _relaxed MMIO accessors on weakly-ordered
> architectures, where they can bring significant performance improvements
> over their non-relaxed counterparts.
>
> (2) To allow COMPILE_TEST to build drivers using the relaxed accessors across
> all architectures.
>
> The proposed semantics largely match exactly those provided by the ARM
> implementation (i.e. no weaker), with one exception (see below).
>
> Informally:
>
> - Relaxed accesses to the same device are ordered with respect to each other.
>
> - Relaxed accesses are *not* guaranteed to be ordered with respect to normal
> memory accesses (e.g. DMA buffers -- this is what gives us the performance
> boost over the non-relaxed versions).
>
> - Relaxed accesses are not guaranteed to be ordered with respect to
> LOCK/UNLOCK operations.
>
> In actual fact, the relaxed accessors *are* ordered with respect to LOCK/UNLOCK
> operations on ARM[64], but I have added this constraint for the benefit of
> PowerPC, which has expensive I/O barriers in the spin_unlock path for the
> non-relaxed accessors.
>
> A corollary to this is that mmiowb() probably needs rethinking. As it currently
> stands, an mmiowb() is required to order MMIO writes to a device from multiple
> CPUs, even if that device is protected by a lock. However, this isn't often used
> in practice, leading to PowerPC implementing both mmiowb() *and* synchronising
> I/O in spin_unlock.
>
> I would propose making the non-relaxed I/O accessors ordered with respect to
> LOCK/UNLOCK, leaving mmiowb() to be used with the relaxed accessors, if
> required, but would welcome thoughts/suggestions on this topic.

So the non-relaxed ops already imply the expensive I/O barrier (mmiowb?)
and therefore, PPC can drop it from spin_unlock()?

Also, I read mmiowb() as MMIO-write-barrier(), what do we have to
order/contain mmio-reads?

I have _0_ experience with MMIO, so I've no idea if ordering/containing
reads is silly or not.

2014-04-17 14:17:24

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

Hi Peter,

On Thu, Apr 17, 2014 at 03:00:36PM +0100, Peter Zijlstra wrote:
> On Thu, Apr 17, 2014 at 02:44:03PM +0100, Will Deacon wrote:
> > In actual fact, the relaxed accessors *are* ordered with respect to LOCK/UNLOCK
> > operations on ARM[64], but I have added this constraint for the benefit of
> > PowerPC, which has expensive I/O barriers in the spin_unlock path for the
> > non-relaxed accessors.
> >
> > A corollary to this is that mmiowb() probably needs rethinking. As it currently
> > stands, an mmiowb() is required to order MMIO writes to a device from multiple
> > CPUs, even if that device is protected by a lock. However, this isn't often used
> > in practice, leading to PowerPC implementing both mmiowb() *and* synchronising
> > I/O in spin_unlock.
> >
> > I would propose making the non-relaxed I/O accessors ordered with respect to
> > LOCK/UNLOCK, leaving mmiowb() to be used with the relaxed accessors, if
> > required, but would welcome thoughts/suggestions on this topic.
>
> So the non-relaxed ops already imply the expensive I/O barrier (mmiowb?)
> and therefore, PPC can drop it from spin_unlock()?

Ben can probably help out here, but if my proposal went ahead (that is,
only the non-relaxed ops would imply mmiowb()), then it would actually
be implemented on PPC by having only those accessors call IO_SET_SYNC_FLAG,
which is checked during unlock (in SYNC_IO).

> Also, I read mmiowb() as MMIO-write-barrier(), what do we have to
> order/contain mmio-reads?

My understanding is that this is related to posted stores from different
CPUs being re-ordered on the bus, so I wouldn't expect reads to suffer
(although, since this isn't permitted on ARM, I'm guessing here).

Will

2014-04-17 14:45:36

by Will Deacon

[permalink] [raw]
Subject: [PATCH 05/18] alpha: io: implement relaxed accessor macros for writes

write{b,w,l,q}_relaxed are implemented by some architectures in order to
permit memory-mapped I/O writes with weaker barrier semantics than the
non-relaxed variants.

This patch implements these write macros for Alpha, in the same vein as
the relaxed read macros, which are already implemented.

Cc: Richard Henderson <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
arch/alpha/include/asm/io.h | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
index 5ebab5895edb..f05bdb4b1cb9 100644
--- a/arch/alpha/include/asm/io.h
+++ b/arch/alpha/include/asm/io.h
@@ -500,10 +500,14 @@ extern inline void writeq(u64 b, volatile void __iomem *addr)
#define outb_p outb
#define outw_p outw
#define outl_p outl
-#define readb_relaxed(addr) __raw_readb(addr)
-#define readw_relaxed(addr) __raw_readw(addr)
-#define readl_relaxed(addr) __raw_readl(addr)
-#define readq_relaxed(addr) __raw_readq(addr)
+#define readb_relaxed(addr) __raw_readb(addr)
+#define readw_relaxed(addr) __raw_readw(addr)
+#define readl_relaxed(addr) __raw_readl(addr)
+#define readq_relaxed(addr) __raw_readq(addr)
+#define writeb_relaxed(b, addr) __raw_writeb(b, addr)
+#define writew_relaxed(b, addr) __raw_writew(b, addr)
+#define writel_relaxed(b, addr) __raw_writel(b, addr)
+#define writeq_relaxed(b, addr) __raw_writeq(b, addr)

#define mmiowb()

--
1.9.1

2014-04-17 14:53:00

by Chris Metcalf

[permalink] [raw]
Subject: Re: [PATCH 15/18] tile: io: implement dummy relaxed accessor macros for writes

On 4/17/2014 9:44 AM, Will Deacon wrote:
> write{b,w,l,q}_relaxed are implemented by some architectures in order to
> permit memory-mapped I/O accesses with weaker barrier semantics than the
> non-relaxed variants.
>
> This patch adds dummy macros for the write accessors to tile, in the
> same vein as the dummy definitions for the relaxed read accessors.
>
> Cc: Chris Metcalf <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> ---
> arch/tile/include/asm/io.h | 4 ++++
> 1 file changed, 4 insertions(+)

Acked-by: Chris Metcalf <[email protected]>

--
Chris Metcalf, Tilera Corp.
http://www.tilera.com

2014-04-17 15:36:51

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

On Thu, Apr 17, 2014 at 02:44:03PM +0100, Will Deacon wrote:
> Hello,
>
> This RFC series attempts to define a portable (i.e. cross-architecture)
> definition of the {readX,writeX}_relaxed MMIO accessor functions. These
> functions are already in widespread use amongst drivers (mainly those supporting
> devices embedded in ARM SoCs), but lack any well-defined semantics and,
> subsequently, any portable definitions to allow these drivers to be compiled for
> other architectures.

Could this be made in such a way that only architectures that need
to provide their own versions actually have to add them?

The current patch-set adds the same dummy defines all over,
and will put this burden also on new architectures.

Sam

2014-04-17 15:48:15

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

Hi Sam,

On Thu, Apr 17, 2014 at 04:36:38PM +0100, Sam Ravnborg wrote:
> On Thu, Apr 17, 2014 at 02:44:03PM +0100, Will Deacon wrote:
> > Hello,
> >
> > This RFC series attempts to define a portable (i.e. cross-architecture)
> > definition of the {readX,writeX}_relaxed MMIO accessor functions. These
> > functions are already in widespread use amongst drivers (mainly those supporting
> > devices embedded in ARM SoCs), but lack any well-defined semantics and,
> > subsequently, any portable definitions to allow these drivers to be compiled for
> > other architectures.
>
> Could this be made in such a way that only architectures that need
> to provide their own versions actually have to add them?
>
> The current patch-set adds the same dummy defines all over,
> and will put this burden also on new architectures.

It shouldn't be a burden for new architectures, as they will use
asm-generic/io.h and get the definitions from there.

Will

2014-04-17 16:07:37

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 10/18] m68k: io: implement dummy relaxed accessor macros for writes

On Thu, Apr 17, 2014 at 3:44 PM, Will Deacon <[email protected]> wrote:
> write{b,w,l}_relaxed are implemented by some architectures in order to
> permit memory-mapped I/O accesses with weaker barrier semantics than the
> non-relaxed variants.
>
> This patch adds dummy macros for the write accessors to m68k, in the
> same vein as the dummy definitions for the relaxed read accessors.
> Additionally, the existing relaxed read accessors are moved into
> asm/io.h, so that they can be used by m68k targets with an MMU.
>
> Cc: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>

Acked-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2014-04-17 19:16:22

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

On Thu, Apr 17, 2014 at 04:47:15PM +0100, Will Deacon wrote:
> Hi Sam,
>
> On Thu, Apr 17, 2014 at 04:36:38PM +0100, Sam Ravnborg wrote:
> > On Thu, Apr 17, 2014 at 02:44:03PM +0100, Will Deacon wrote:
> > > Hello,
> > >
> > > This RFC series attempts to define a portable (i.e. cross-architecture)
> > > definition of the {readX,writeX}_relaxed MMIO accessor functions. These
> > > functions are already in widespread use amongst drivers (mainly those supporting
> > > devices embedded in ARM SoCs), but lack any well-defined semantics and,
> > > subsequently, any portable definitions to allow these drivers to be compiled for
> > > other architectures.
> >
> > Could this be made in such a way that only architectures that need
> > to provide their own versions actually have to add them?
> >
> > The current patch-set adds the same dummy defines all over,
> > and will put this burden also on new architectures.
>
> It shouldn't be a burden for new architectures, as they will use
> asm-generic/io.h and get the definitions from there.

Why is it then necesary to do this for sparc:
diff --git a/arch/sparc/include/asm/io.h b/arch/sparc/include/asm/io.h
index f6902cf3cbe9..493f22c4684f 100644
--- a/arch/sparc/include/asm/io.h
+++ b/arch/sparc/include/asm/io.h
@@ -10,6 +10,15 @@
* Defines used for both SPARC32 and SPARC64
*/

+/* Relaxed accessors for MMIO */
+#define readb_relaxed(__addr) readb(__addr)
+#define readw_relaxed(__addr) readw(__addr)
+#define readl_relaxed(__addr) readl(__addr)
+
+#define writeb_relaxed(__b, __addr) writeb(__b, __addr)
+#define writew_relaxed(__w, __addr) writew(__w, __addr)
+#define writel_relaxed(__l, __addr) writel(__l, __addr)

And similar for several other architectures.

For asm-generic/io.h:
+#ifndef readb_relaxed
+#define readb_relaxed readb
+#endif

This has same effect as the above.
Only difference is that the implementation in asm-generic lacks the arguments.

The patch also breaks the pattern that the #define foobar foobar is
on the line just above the static inline that implements the function.

-#define readw readw
+#define readw readw

+#ifndef readw_relaxed
+#define readw_relaxed readw
+#endif
Move this blow below the static inline would make this easier to understand.

static inline u16 readw(const volatile void __iomem *addr)
{
return __le16_to_cpu(__raw_readw(addr));
}


Sam

2014-04-17 21:38:08

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

On Thu, 2014-04-17 at 16:00 +0200, Peter Zijlstra wrote:

> So the non-relaxed ops already imply the expensive I/O barrier (mmiowb?)
> and therefore, PPC can drop it from spin_unlock()?

We play a trick. We set a per-cpu flag in writeX and test it in unlock
before doing the barrier. Still better than having the barrier in every
MMIO at this stage for us.

Whether we want to change that with then new scheme ... we'll see.

> Also, I read mmiowb() as MMIO-write-barrier(), what do we have to
> order/contain mmio-reads?
>
> I have _0_ experience with MMIO, so I've no idea if ordering/containing
> reads is silly or not.

I will review the rest when I'm back from vacation (or maybe this
week-end).

Thanks Will for picking that up, it's long overdue :)

Cheers,
Ben.

2014-04-22 13:44:47

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

Hello Sam,

On Thu, Apr 17, 2014 at 08:15:55PM +0100, Sam Ravnborg wrote:
> On Thu, Apr 17, 2014 at 04:47:15PM +0100, Will Deacon wrote:
> > On Thu, Apr 17, 2014 at 04:36:38PM +0100, Sam Ravnborg wrote:
> > > On Thu, Apr 17, 2014 at 02:44:03PM +0100, Will Deacon wrote:
> > > > This RFC series attempts to define a portable (i.e. cross-architecture)
> > > > definition of the {readX,writeX}_relaxed MMIO accessor functions. These
> > > > functions are already in widespread use amongst drivers (mainly those supporting
> > > > devices embedded in ARM SoCs), but lack any well-defined semantics and,
> > > > subsequently, any portable definitions to allow these drivers to be compiled for
> > > > other architectures.
> > >
> > > Could this be made in such a way that only architectures that need
> > > to provide their own versions actually have to add them?
> > >
> > > The current patch-set adds the same dummy defines all over,
> > > and will put this burden also on new architectures.
> >
> > It shouldn't be a burden for new architectures, as they will use
> > asm-generic/io.h and get the definitions from there.
>
> Why is it then necesary to do this for sparc:
> diff --git a/arch/sparc/include/asm/io.h b/arch/sparc/include/asm/io.h
> index f6902cf3cbe9..493f22c4684f 100644
> --- a/arch/sparc/include/asm/io.h
> +++ b/arch/sparc/include/asm/io.h
> @@ -10,6 +10,15 @@
> * Defines used for both SPARC32 and SPARC64
> */
>
> +/* Relaxed accessors for MMIO */
> +#define readb_relaxed(__addr) readb(__addr)
> +#define readw_relaxed(__addr) readw(__addr)
> +#define readl_relaxed(__addr) readl(__addr)
> +
> +#define writeb_relaxed(__b, __addr) writeb(__b, __addr)
> +#define writew_relaxed(__w, __addr) writew(__w, __addr)
> +#define writel_relaxed(__l, __addr) writel(__l, __addr)
>
> And similar for several other architectures.

This is because Sparc (and the other architectures I had to modify) don't
make use of asm-generic/io.h. Furthermore, it's not as simple as adding an
include, since you'll pull in the generic definitions of things like readw
and inb as it stands.

We could make a new asm-generic file purely for the relaxed accessors, but
I really don't think it's worth the hassle.

> For asm-generic/io.h:
> +#ifndef readb_relaxed
> +#define readb_relaxed readb
> +#endif
>
> This has same effect as the above.
> Only difference is that the implementation in asm-generic lacks the arguments.

I just followed the existing style in asm-generic/io.h.

> The patch also breaks the pattern that the #define foobar foobar is
> on the line just above the static inline that implements the function.
>
> -#define readw readw
> +#define readw readw
>
> +#ifndef readw_relaxed
> +#define readw_relaxed readw
> +#endif
> Move this blow below the static inline would make this easier to understand.
>
> static inline u16 readw(const volatile void __iomem *addr)
> {
> return __le16_to_cpu(__raw_readw(addr));
> }

Sure, I can fix that.

Will

2014-04-22 13:47:40

by Jesper Nilsson

[permalink] [raw]
Subject: Re: [PATCH 07/18] cris: io: implement dummy relaxed accessor macros for writes

On Thu, Apr 17, 2014 at 02:44:10PM +0100, Will Deacon wrote:
> write{b,w,l}_relaxed are implemented by some architectures in order to
> permit memory-mapped I/O accesses with weaker barrier semantics than the
> non-relaxed variants.
>
> This patch adds dummy macros for the write accessors to Cris, in the same
> vein as the dummy definitions for the relaxed read accessors.
>
> Cc: Mikael Starvik <[email protected]>

Acked-by: Jesper Nilsson <[email protected]>

> Signed-off-by: Will Deacon <[email protected]>

/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]

2014-04-22 13:53:24

by Michal Simek

[permalink] [raw]
Subject: Re: [PATCH 02/18] microblaze: io: remove dummy relaxed accessor macros

On 04/17/2014 03:44 PM, Will Deacon wrote:
> These are now defined by asm-generic/io.h, so we don't need the private
> definitions anymore.
>
> Cc: Michal Simek <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> ---
> arch/microblaze/include/asm/io.h | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/arch/microblaze/include/asm/io.h b/arch/microblaze/include/asm/io.h
> index 1e4c3329f62e..2987fd4c73d6 100644
> --- a/arch/microblaze/include/asm/io.h
> +++ b/arch/microblaze/include/asm/io.h
> @@ -72,12 +72,4 @@ extern void __iomem *ioremap(phys_addr_t address, unsigned long size);
>
> #include <asm-generic/io.h>
>
> -#define readb_relaxed readb
> -#define readw_relaxed readw
> -#define readl_relaxed readl
> -
> -#define writeb_relaxed writeb
> -#define writew_relaxed writew
> -#define writel_relaxed writel
> -
> #endif /* _ASM_MICROBLAZE_IO_H */
>

Acked-by: Michal Simek <[email protected]>

Thanks,
Michal

--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



Attachments:
signature.asc (263.00 B)
OpenPGP digital signature

2014-04-22 14:10:30

by Michal Simek

[permalink] [raw]
Subject: Re: [PATCH 18/18] asm-generic: io: define relaxed accessor macros unconditionally

On 04/17/2014 03:44 PM, Will Deacon wrote:
> Now that no architectures using asm-generic/io.h define their own relaxed
> accessors, the dummy definitions can be used unconditionally.
>
> Cc: Arnd Bergmann <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> ---
> include/asm-generic/io.h | 18 ------------------
> 1 file changed, 18 deletions(-)

Do we need this? I think there could be a need to overwrite them.
Currently we are just lucky that architecture which uses asm-generic/io.h
don't need to overwrite it.
But I expect that all archs should use asm-generic/io.h to clean
architecture io.h exactly I have done it for Microblaze.

Thanks,
Michal

--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



Attachments:
signature.asc (263.00 B)
OpenPGP digital signature

2014-04-22 14:31:06

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

>
> This is because Sparc (and the other architectures I had to modify) don't
> make use of asm-generic/io.h.
Somehow I missed this.

Added to my todo list to look into sparc in this respect.
A quick lokk reveal that sparc32 can use most/all of the generic io.h

Sam

2014-04-22 15:19:01

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH 18/18] asm-generic: io: define relaxed accessor macros unconditionally

Hi Michal,

On Tue, Apr 22, 2014 at 03:09:46PM +0100, Michal Simek wrote:
> On 04/17/2014 03:44 PM, Will Deacon wrote:
> > Now that no architectures using asm-generic/io.h define their own relaxed
> > accessors, the dummy definitions can be used unconditionally.
> >
> > Cc: Arnd Bergmann <[email protected]>
> > Signed-off-by: Will Deacon <[email protected]>
> > ---
> > include/asm-generic/io.h | 18 ------------------
> > 1 file changed, 18 deletions(-)
>
> Do we need this? I think there could be a need to overwrite them.
> Currently we are just lucky that architecture which uses asm-generic/io.h
> don't need to overwrite it.
> But I expect that all archs should use asm-generic/io.h to clean
> architecture io.h exactly I have done it for Microblaze.

I'm open to keeping the conditional definitions, but it introduces a
discrepancy with the non-relaxed versions, which are defined unconditionally
(although the underlying __raw_* accessors can be overridden).

Will

2014-04-22 16:09:09

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH 16/18] x86: io: implement dummy relaxed accessor macros for writes

On Thu, Apr 17, 2014 at 02:44:19PM +0100, Will Deacon wrote:
> write{b,w,l,q}_relaxed are implemented by some architectures in order to
> permit memory-mapped I/O accesses with weaker barrier semantics than the
> non-relaxed variants.
>
> This patch adds dummy macros for the write accessors to x86, in the
> same vein as the dummy definitions for the relaxed read accessors.
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> ---
> arch/x86/include/asm/io.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index b8237d8a1e0c..2ea07f5ec7b7 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -74,6 +74,9 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
> #define __raw_readw __readw
> #define __raw_readl __readl
>
> +#define writeb_relaxed(v, a) __writeb(v, a)
> +#define writew_relaxed(v, a) __writew(v, a)
> +#define writel_relaxed(v, a) __writel(v, a)
> #define __raw_writeb __writeb
> #define __raw_writew __writew
> #define __raw_writel __writel
> @@ -86,6 +89,7 @@ build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
> build_mmio_write(writeq, "q", unsigned long, "r", :"memory")
>
> #define readq_relaxed(a) readq(a)
> +#define writeq_relaxed(v, a) writeq(v, a)

Actually, I should be using the regular (i.e. without the double underscore
prefix) accessors for the relaxed variants, including the existing read
flavours here. The proposed semantics are that the accessors are ordered
with respect to each other, which necessitates a compiler barrier.

Will

2014-04-23 07:12:41

by Michal Simek

[permalink] [raw]
Subject: Re: [PATCH 18/18] asm-generic: io: define relaxed accessor macros unconditionally

Hi Will,

On 04/22/2014 05:18 PM, Will Deacon wrote:
> Hi Michal,
>
> On Tue, Apr 22, 2014 at 03:09:46PM +0100, Michal Simek wrote:
>> On 04/17/2014 03:44 PM, Will Deacon wrote:
>>> Now that no architectures using asm-generic/io.h define their own relaxed
>>> accessors, the dummy definitions can be used unconditionally.
>>>
>>> Cc: Arnd Bergmann <[email protected]>
>>> Signed-off-by: Will Deacon <[email protected]>
>>> ---
>>> include/asm-generic/io.h | 18 ------------------
>>> 1 file changed, 18 deletions(-)
>>
>> Do we need this? I think there could be a need to overwrite them.
>> Currently we are just lucky that architecture which uses asm-generic/io.h
>> don't need to overwrite it.
>> But I expect that all archs should use asm-generic/io.h to clean
>> architecture io.h exactly I have done it for Microblaze.
>
> I'm open to keeping the conditional definitions, but it introduces a
> discrepancy with the non-relaxed versions, which are defined unconditionally
> (although the underlying __raw_* accessors can be overridden).

up to you.

Thanks,
Michal


--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



Attachments:
signature.asc (263.00 B)
OpenPGP digital signature

2014-04-23 07:23:36

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH 18/18] asm-generic: io: define relaxed accessor macros unconditionally

> But I expect that all archs should use asm-generic/io.h to clean
> architecture io.h exactly I have done it for Microblaze.
I took a quick look at sparc32 - doing this is a nice cleanup.
Thanks for the hint.

Sam

2014-04-23 07:36:23

by Michal Simek

[permalink] [raw]
Subject: Re: [PATCH 18/18] asm-generic: io: define relaxed accessor macros unconditionally

On 04/23/2014 09:23 AM, Sam Ravnborg wrote:
>> But I expect that all archs should use asm-generic/io.h to clean
>> architecture io.h exactly I have done it for Microblaze.
> I took a quick look at sparc32 - doing this is a nice cleanup.
> Thanks for the hint.

Nice. I had to fix some sparse warning which was introduce
by this change. You should check them too if you haven't done it.

As follow up we should move these to asm-generic/io.h too
ioremap_writethrough
ioremap_nocache
ioremap_fullcache - used nowhere
ioremap_wc

Thanks,
Michal

--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



Attachments:
signature.asc (263.00 B)
OpenPGP digital signature