[PATCH v4 0/5] make io{read|write}64 globally usable

[Logan Gunthorpe]

This is version four of my patchset to enable drivers to use
io{read|write}64 on all arches.

Changes since v3:

- I noticed powerpc didn't use the appropriate functions seeing
readq/writeq were not defined when iomap.h was included. Thus I've
included a patch to adjust this
- Fixed some mistakes with a couple of the defines in io-64-nonatomic*
headers
- Fixed a typo noticed by Horia.


Horia Geantă (1):
crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

Logan Gunthorpe (4):
powerpc: io.h: move iomap.h include so that it can use readq/writeq
defs
iomap: introduce io{read|write}64_{lo_hi|hi_lo}
io-64-nonatomic: add io{read|write}64[be]{_lo_hi|_hi_lo} macros
ntb: ntb_hw_intel: use io-64-nonatomic instead of in-driver hacks

arch/powerpc/include/asm/io.h | 6 +-
drivers/crypto/caam/regs.h | 35 ++-------
drivers/ntb/hw/intel/ntb_hw_intel.c | 30 +-------
include/asm-generic/iomap.h | 26 +++++--
include/linux/io-64-nonatomic-hi-lo.h | 60 ++++++++++++++++
include/linux/io-64-nonatomic-lo-hi.h | 60 ++++++++++++++++
lib/iomap.c | 132 ++++++++++++++++++++++++++++++++++
7 files changed, 282 insertions(+), 67 deletions(-)

--
2.11.0

[Logan Gunthorpe]

Now that ioread64 and iowrite64 are available in io-64-nonatomic,
we can remove the hack at the top of ntb_hw_intel.c and replace it
with an include.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
Cc: Jon Mason <jdm...@kudzu.us>
Cc: Allen Hubbe <Allen...@emc.com>
Acked-by: Dave Jiang <dave....@intel.com>
---
drivers/ntb/hw/intel/ntb_hw_intel.c | 30 +-----------------------------
1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/ntb/hw/intel/ntb_hw_intel.c b/drivers/ntb/hw/intel/ntb_hw_intel.c
index 2557e2c05b90..606c90f59d4b 100644
--- a/drivers/ntb/hw/intel/ntb_hw_intel.c
+++ b/drivers/ntb/hw/intel/ntb_hw_intel.c
@@ -59,6 +59,7 @@
#include <linux/random.h>
#include <linux/slab.h>
#include <linux/ntb.h>
+#include <linux/io-64-nonatomic-lo-hi.h>

#include "ntb_hw_intel.h"

@@ -155,35 +156,6 @@ MODULE_PARM_DESC(xeon_b2b_dsd_bar5_addr32,
static inline enum ntb_topo xeon_ppd_topo(struct intel_ntb_dev *ndev, u8 ppd);
static int xeon_init_isr(struct intel_ntb_dev *ndev);

-#ifndef ioread64
-#ifdef readq
-#define ioread64 readq
-#else
-#define ioread64 _ioread64
-static inline u64 _ioread64(void __iomem *mmio)
-{
- u64 low, high;
-
- low = ioread32(mmio);
- high = ioread32(mmio + sizeof(u32));
- return low | (high << 32);
-}
-#endif
-#endif
-
-#ifndef iowrite64
-#ifdef writeq
-#define iowrite64 writeq
-#else
-#define iowrite64 _iowrite64
-static inline void _iowrite64(u64 val, void __iomem *mmio)
-{
- iowrite32(val, mmio);
- iowrite32(val >> 32, mmio + sizeof(u32));
-}
-#endif
-#endif
-
static inline int pdev_is_atom(struct pci_dev *pdev)
{
switch (pdev->device) {
--
2.11.0

[Logan Gunthorpe]

This patch adds generic io{read|write}64[be]{_lo_hi|_hi_lo} macros if
they are not already defined by the architecture. (As they are provided
by the generic iomap library).

The patch also points io{read|write}64[be] to the variant specified by the
header name.

This is because new drivers are encouraged to use ioreadXX, et al instead
of readX[1], et al -- and mixing ioreadXX with readq is pretty ugly.

[1] ldd3: section 9.4.2

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
cc: Christoph Hellwig <h...@lst.de>
cc: Arnd Bergmann <ar...@arndb.de>
cc: Alan Cox <gno...@lxorguk.ukuu.org.uk>
cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
include/linux/io-64-nonatomic-hi-lo.h | 60 +++++++++++++++++++++++++++++++++++
include/linux/io-64-nonatomic-lo-hi.h | 60 +++++++++++++++++++++++++++++++++++
2 files changed, 120 insertions(+)

diff --git a/include/linux/io-64-nonatomic-hi-lo.h b/include/linux/io-64-nonatomic-hi-lo.h
index defcc4644ce3..31d28e981299 100644
--- a/include/linux/io-64-nonatomic-hi-lo.h
+++ b/include/linux/io-64-nonatomic-hi-lo.h
@@ -54,4 +54,64 @@ static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr)
#define writeq_relaxed hi_lo_writeq_relaxed
#endif

+#ifndef ioread64_hi_lo
+#define ioread64_hi_lo ioread64_hi_lo
+static inline u64 ioread64_hi_lo(void __iomem *addr)
+{
+ u32 low, high;
+
+ high = ioread32(addr + sizeof(u32));
+ low = ioread32(addr);
+
+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64_hi_lo
+#define iowrite64_hi_lo iowrite64_hi_lo
+static inline void iowrite64_hi_lo(u64 val, void __iomem *addr)
+{
+ iowrite32(val >> 32, addr + sizeof(u32));
+ iowrite32(val, addr);
+}
+#endif
+
+#ifndef ioread64be_hi_lo
+#define ioread64be_hi_lo ioread64be_hi_lo
+static inline u64 ioread64be_hi_lo(void __iomem *addr)
+{
+ u32 low, high;
+
+ high = ioread32be(addr);
+ low = ioread32be(addr + sizeof(u32));
+
+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64be_hi_lo
+#define iowrite64be_hi_lo iowrite64be_hi_lo
+static inline void iowrite64be_hi_lo(u64 val, void __iomem *addr)
+{
+ iowrite32be(val >> 32, addr);
+ iowrite32be(val, addr + sizeof(u32));
+}
+#endif
+
+#ifndef ioread64
+#define ioread64 ioread64_hi_lo
+#endif
+
+#ifndef iowrite64
+#define iowrite64 iowrite64_hi_lo
+#endif
+
+#ifndef ioread64be
+#define ioread64be ioread64be_hi_lo
+#endif
+
+#ifndef iowrite64be
+#define iowrite64be iowrite64be_hi_lo
+#endif
+
#endif /* _LINUX_IO_64_NONATOMIC_HI_LO_H_ */
diff --git a/include/linux/io-64-nonatomic-lo-hi.h b/include/linux/io-64-nonatomic-lo-hi.h
index 084461a4e5ab..437a34f20f5a 100644
--- a/include/linux/io-64-nonatomic-lo-hi.h
+++ b/include/linux/io-64-nonatomic-lo-hi.h
@@ -54,4 +54,64 @@ static inline void lo_hi_writeq_relaxed(__u64 val, volatile void __iomem *addr)
#define writeq_relaxed lo_hi_writeq_relaxed
#endif

+#ifndef ioread64_lo_hi
+#define ioread64_lo_hi ioread64_lo_hi
+static inline u64 ioread64_lo_hi(void __iomem *addr)
+{
+ u32 low, high;
+
+ low = ioread32(addr);
+ high = ioread32(addr + sizeof(u32));
+
+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64_lo_hi
+#define iowrite64_lo_hi iowrite64_lo_hi
+static inline void iowrite64_lo_hi(u64 val, void __iomem *addr)
+{
+ iowrite32(val, addr);
+ iowrite32(val >> 32, addr + sizeof(u32));
+}
+#endif
+
+#ifndef ioread64be_lo_hi
+#define ioread64be_lo_hi ioread64be_lo_hi
+static inline u64 ioread64be_lo_hi(void __iomem *addr)
+{
+ u32 low, high;
+
+ low = ioread32be(addr + sizeof(u32));
+ high = ioread32be(addr);
+
+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64be_lo_hi
+#define iowrite64be_lo_hi iowrite64be_lo_hi
+static inline void iowrite64be_lo_hi(u64 val, void __iomem *addr)
+{
+ iowrite32be(val, addr + sizeof(u32));
+ iowrite32be(val >> 32, addr);
+}
+#endif
+
+#ifndef ioread64
+#define ioread64 ioread64_lo_hi
+#endif
+
+#ifndef iowrite64
+#define iowrite64 iowrite64_lo_hi
+#endif
+
+#ifndef ioread64be
+#define ioread64be ioread64be_lo_hi
+#endif
+
+#ifndef iowrite64be
+#define iowrite64be iowrite64be_lo_hi
+#endif
+
#endif /* _LINUX_IO_64_NONATOMIC_LO_HI_H_ */
--
2.11.0

[Logan Gunthorpe]

From: Horia Geantă <horia....@nxp.com>

We can now make use of the io-64-nonatomic-lo-hi header to always
provide 64 bit IO operations. So this patch cleans up the extra
CONFIG_64BIT ifdefs.

To be consistent with CAAM engine HW spec: in case of 64-bit registers,
irrespective of device endianness, the lower address should be read from
/ written to first, followed by the upper address. Indeed the I/O
accessors in CAAM driver currently don't follow the spec, however this
is a good opportunity to fix the code.

Signed-off-by: Horia Geantă <horia....@nxp.com>
Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
Cc: Horia Geantă <horia....@nxp.com>
Cc: Dan Douglass <dan.do...@nxp.com>
Cc: Herbert Xu <her...@gondor.apana.org.au>
Cc: "David S. Miller" <da...@davemloft.net>
---
drivers/crypto/caam/regs.h | 35 +++++------------------------------
1 file changed, 5 insertions(+), 30 deletions(-)

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 84d2f838a063..0c45505458e7 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -9,7 +9,7 @@

#include <linux/types.h>
#include <linux/bitops.h>
-#include <linux/io.h>
+#include <linux/io-64-nonatomic-lo-hi.h>

/*
* Architecture-specific register access methods
@@ -134,50 +134,25 @@ static inline void clrsetbits_32(void __iomem *reg, u32 clear, u32 set)
* base + 0x0000 : least-significant 32 bits
* base + 0x0004 : most-significant 32 bits
*/
-#ifdef CONFIG_64BIT
static inline void wr_reg64(void __iomem *reg, u64 data)
{
+#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
if (caam_little_end)
iowrite64(data, reg);
else
- iowrite64be(data, reg);
-}
-
-static inline u64 rd_reg64(void __iomem *reg)
-{
- if (caam_little_end)
- return ioread64(reg);
- else
- return ioread64be(reg);
-}
-
-#else /* CONFIG_64BIT */
-static inline void wr_reg64(void __iomem *reg, u64 data)
-{
-#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
- if (caam_little_end) {
- wr_reg32((u32 __iomem *)(reg) + 1, data >> 32);
- wr_reg32((u32 __iomem *)(reg), data);
- } else
#endif
- {
- wr_reg32((u32 __iomem *)(reg), data >> 32);
- wr_reg32((u32 __iomem *)(reg) + 1, data);
- }
+ iowrite64be(data, reg);
}

static inline u64 rd_reg64(void __iomem *reg)
{
#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
if (caam_little_end)
- return ((u64)rd_reg32((u32 __iomem *)(reg) + 1) << 32 |
- (u64)rd_reg32((u32 __iomem *)(reg)));
+ return ioread64(reg);
else
#endif
- return ((u64)rd_reg32((u32 __iomem *)(reg)) << 32 |
- (u64)rd_reg32((u32 __iomem *)(reg) + 1));
+ return ioread64be(reg);
}
-#endif /* CONFIG_64BIT */

#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
#ifdef CONFIG_SOC_IMX7D
--
2.11.0

[Logan Gunthorpe]

Subsequent patches in this series makes use of the readq and writeq
defines in iomap.h. However, as is, they get missed on the powerpc
platform seeing the include comes before the define. This patch
moves the include down to fix this.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
Cc: Benjamin Herrenschmidt <be...@kernel.crashing.org>
Cc: Paul Mackerras <pau...@samba.org>
Cc: Michael Ellerman <m...@ellerman.id.au>
Cc: Nicholas Piggin <npi...@gmail.com>
Cc: Suresh Warrier <war...@linux.vnet.ibm.com>
Cc: "Oliver O'Halloran" <ooh...@gmail.com>
---
arch/powerpc/include/asm/io.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 422f99cf9924..af074923d598 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -33,8 +33,6 @@ extern struct pci_dev *isa_bridge_pcidev;
#include <asm/mmu.h>
#include <asm/ppc_asm.h>

-#include <asm-generic/iomap.h>
-
#ifdef CONFIG_PPC64
#include <asm/paca.h>
#endif
@@ -663,6 +661,8 @@ static inline void name at \
#define writel_relaxed(v, addr) writel(v, addr)
#define writeq_relaxed(v, addr) writeq(v, addr)

+#include <asm-generic/iomap.h>
+
#ifdef CONFIG_PPC32
#define mmiowb()
#else
--
2.11.0

[Logan Gunthorpe]

In order to provide non-atomic functions for io{read|write}64 that will
use readq and writeq when appropriate. We define a number of variants
of these functions in the generic iomap that will do non-atomic
operations on pio but atomic operations on mmio.

These functions are only defined if readq and writeq are defined. If
they are not, then the wrappers that always use non-atomic operations
from include/linux/io-64-nonatomic*.h will be used.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
Cc: Benjamin Herrenschmidt <be...@kernel.crashing.org>
Cc: Paul Mackerras <pau...@samba.org>
Cc: Michael Ellerman <m...@ellerman.id.au>

Cc: Arnd Bergmann <ar...@arndb.de>
Cc: Suresh Warrier <war...@linux.vnet.ibm.com>
Cc: Nicholas Piggin <npi...@gmail.com>
---
arch/powerpc/include/asm/io.h | 2 +
include/asm-generic/iomap.h | 26 +++++++--
lib/iomap.c | 132 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index af074923d598..4cc420cfaa78 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -788,8 +788,10 @@ extern void __iounmap_at(void *ea, unsigned long size);

#define mmio_read16be(addr) readw_be(addr)
#define mmio_read32be(addr) readl_be(addr)
+#define mmio_read64be(addr) readq_be(addr)
#define mmio_write16be(val, addr) writew_be(val, addr)
#define mmio_write32be(val, addr) writel_be(val, addr)
+#define mmio_write64be(val, addr) writeq_be(val, addr)
#define mmio_insb(addr, dst, count) readsb(addr, dst, count)
#define mmio_insw(addr, dst, count) readsw(addr, dst, count)
#define mmio_insl(addr, dst, count) readsl(addr, dst, count)
diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 650fede33c25..e4601455ac4a 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -30,9 +30,16 @@ extern unsigned int ioread16(void __iomem *);
extern unsigned int ioread16be(void __iomem *);
extern unsigned int ioread32(void __iomem *);
extern unsigned int ioread32be(void __iomem *);
-#ifdef CONFIG_64BIT
-extern u64 ioread64(void __iomem *);
-extern u64 ioread64be(void __iomem *);
+
+#ifdef readq
+#define ioread64_lo_hi ioread64_lo_hi
+#define ioread64_hi_lo ioread64_hi_lo
+#define ioread64be_lo_hi ioread64be_lo_hi
+#define ioread64be_hi_lo ioread64be_hi_lo
+extern u64 ioread64_lo_hi(void __iomem *addr);
+extern u64 ioread64_hi_lo(void __iomem *addr);
+extern u64 ioread64be_lo_hi(void __iomem *addr);
+extern u64 ioread64be_hi_lo(void __iomem *addr);
#endif

extern void iowrite8(u8, void __iomem *);
@@ -40,9 +47,16 @@ extern void iowrite16(u16, void __iomem *);
extern void iowrite16be(u16, void __iomem *);
extern void iowrite32(u32, void __iomem *);
extern void iowrite32be(u32, void __iomem *);
-#ifdef CONFIG_64BIT
-extern void iowrite64(u64, void __iomem *);
-extern void iowrite64be(u64, void __iomem *);
+
+#ifdef writeq
+#define iowrite64_lo_hi iowrite64_lo_hi
+#define iowrite64_hi_lo iowrite64_hi_lo
+#define iowrite64be_lo_hi iowrite64be_lo_hi
+#define iowrite64be_hi_lo iowrite64be_hi_lo
+void iowrite64_lo_hi(u64 val, void __iomem *addr);
+void iowrite64_hi_lo(u64 val, void __iomem *addr);
+void iowrite64be_lo_hi(u64 val, void __iomem *addr);
+void iowrite64be_hi_lo(u64 val, void __iomem *addr);
#endif

/*
diff --git a/lib/iomap.c b/lib/iomap.c
index fc3dcb4b238e..b993400d60bd 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -66,6 +66,7 @@ static void bad_io_access(unsigned long port, const char *access)
#ifndef mmio_read16be
#define mmio_read16be(addr) be16_to_cpu(__raw_readw(addr))
#define mmio_read32be(addr) be32_to_cpu(__raw_readl(addr))
+#define mmio_read64be(addr) be64_to_cpu(__raw_readq(addr))
#endif

unsigned int ioread8(void __iomem *addr)
@@ -99,6 +100,80 @@ EXPORT_SYMBOL(ioread16be);
EXPORT_SYMBOL(ioread32);
EXPORT_SYMBOL(ioread32be);

+#ifdef readq
+static u64 pio_read64_lo_hi(unsigned long port)
+{
+ u64 lo, hi;
+
+ lo = inl(port);
+ hi = inl(port + sizeof(u32));
+
+ return lo | (hi << 32);
+}
+
+static u64 pio_read64_hi_lo(unsigned long port)
+{
+ u64 lo, hi;
+
+ hi = inl(port + sizeof(u32));
+ lo = inl(port);
+
+ return lo | (hi << 32);
+}
+
+static u64 pio_read64be_lo_hi(unsigned long port)
+{
+ u64 lo, hi;
+
+ lo = pio_read32be(port + sizeof(u32));
+ hi = pio_read32be(port);
+
+ return lo | (hi << 32);
+}
+
+static u64 pio_read64be_hi_lo(unsigned long port)
+{
+ u64 lo, hi;
+
+ hi = pio_read32be(port);
+ lo = pio_read32be(port + sizeof(u32));
+
+ return lo | (hi << 32);
+}
+
+u64 ioread64_lo_hi(void __iomem *addr)
+{
+ IO_COND(addr, return pio_read64_lo_hi(port), return readq(addr));
+ return 0xffffffffffffffffLL;
+}
+
+u64 ioread64_hi_lo(void __iomem *addr)
+{
+ IO_COND(addr, return pio_read64_hi_lo(port), return readq(addr));
+ return 0xffffffffffffffffLL;
+}
+
+u64 ioread64be_lo_hi(void __iomem *addr)
+{
+ IO_COND(addr, return pio_read64be_lo_hi(port),
+ return mmio_read64be(addr));
+ return 0xffffffffffffffffLL;
+}
+
+u64 ioread64be_hi_lo(void __iomem *addr)
+{
+ IO_COND(addr, return pio_read64be_hi_lo(port),
+ return mmio_read64be(addr));
+ return 0xffffffffffffffffLL;
+}
+
+EXPORT_SYMBOL(ioread64_lo_hi);
+EXPORT_SYMBOL(ioread64_hi_lo);
+EXPORT_SYMBOL(ioread64be_lo_hi);
+EXPORT_SYMBOL(ioread64be_hi_lo);
+
+#endif /* readq */
+
#ifndef pio_write16be
#define pio_write16be(val,port) outw(swab16(val),port)
#define pio_write32be(val,port) outl(swab32(val),port)
@@ -107,6 +182,7 @@ EXPORT_SYMBOL(ioread32be);
#ifndef mmio_write16be
#define mmio_write16be(val,port) __raw_writew(be16_to_cpu(val),port)
#define mmio_write32be(val,port) __raw_writel(be32_to_cpu(val),port)
+#define mmio_write64be(val,port) __raw_writeq(be64_to_cpu(val),port)
#endif

void iowrite8(u8 val, void __iomem *addr)
@@ -135,6 +211,62 @@ EXPORT_SYMBOL(iowrite16be);
EXPORT_SYMBOL(iowrite32);
EXPORT_SYMBOL(iowrite32be);

+#ifdef writeq
+static void pio_write64_lo_hi(u64 val, unsigned long port)
+{
+ outl(val, port);
+ outl(val >> 32, port + sizeof(u32));
+}
+
+static void pio_write64_hi_lo(u64 val, unsigned long port)
+{
+ outl(val >> 32, port + sizeof(u32));
+ outl(val, port);
+}
+
+static void pio_write64be_lo_hi(u64 val, unsigned long port)
+{
+ pio_write32be(val, port + sizeof(u32));
+ pio_write32be(val >> 32, port);
+}
+
+static void pio_write64be_hi_lo(u64 val, unsigned long port)
+{
+ pio_write32be(val >> 32, port);
+ pio_write32be(val, port + sizeof(u32));
+}
+
+void iowrite64_lo_hi(u64 val, void __iomem *addr)
+{
+ IO_COND(addr, pio_write64_lo_hi(val, port),
+ writeq(val, addr));
+}
+
+void iowrite64_hi_lo(u64 val, void __iomem *addr)
+{
+ IO_COND(addr, pio_write64_hi_lo(val, port),
+ writeq(val, addr));
+}
+
+void iowrite64be_lo_hi(u64 val, void __iomem *addr)
+{
+ IO_COND(addr, pio_write64be_lo_hi(val, port),
+ mmio_write64be(val, addr));
+}
+
+void iowrite64be_hi_lo(u64 val, void __iomem *addr)
+{
+ IO_COND(addr, pio_write64be_hi_lo(val, port),
+ mmio_write64be(val, addr));
+}
+
+EXPORT_SYMBOL(iowrite64_lo_hi);
+EXPORT_SYMBOL(iowrite64_hi_lo);
+EXPORT_SYMBOL(iowrite64be_lo_hi);
+EXPORT_SYMBOL(iowrite64be_hi_lo);
+
+#endif /* readq */
+
/*
* These are the "repeat MMIO read/write" functions.
* Note the "__raw" accesses, since we don't want to
--
2.11.0

[Allen Hubbe]

From: Logan Gunthorpe

> Now that ioread64 and iowrite64 are available in io-64-nonatomic,
> we can remove the hack at the top of ntb_hw_intel.c and replace it
> with an include.
>
> Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
> Cc: Jon Mason <jdm...@kudzu.us>
> Cc: Allen Hubbe <Allen...@emc.com>
> Acked-by: Dave Jiang <dave....@intel.com>

Acked-by: Allen Hubbe <Allen...@dell.com>

[Michael Ellerman]

Logan Gunthorpe <log...@deltatee.com> writes:

> Subsequent patches in this series makes use of the readq and writeq
> defines in iomap.h. However, as is, they get missed on the powerpc
> platform seeing the include comes before the define. This patch
> moves the include down to fix this.
>
> Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
> Cc: Benjamin Herrenschmidt <be...@kernel.crashing.org>
> Cc: Paul Mackerras <pau...@samba.org>
> Cc: Michael Ellerman <m...@ellerman.id.au>
> Cc: Nicholas Piggin <npi...@gmail.com>
> Cc: Suresh Warrier <war...@linux.vnet.ibm.com>
> Cc: "Oliver O'Halloran" <ooh...@gmail.com>
> ---
> arch/powerpc/include/asm/io.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Seems fair enough, have you tested it at all?

cheers

[Logan Gunthorpe]

On 18/07/17 11:57 PM, Michael Ellerman wrote:
> Seems fair enough, have you tested it at all?

It's only been compile tested and the kbuild robot has beat up on it a bit.

Thanks,

Logan

[Horia Geantă]

Looks like the patch set does not compile on PPC (.config generated
using make corenet64_smp_defconfig):

[...]
LD vmlinux.o
MODPOST vmlinux.o
drivers/crypto/caam/jr.o: In function `rd_reg64':
/home/b05471/repos/cryptodev-2.6/drivers/crypto/caam/regs.h:154:
undefined reference to `.ioread64be_lo_hi'
/home/b05471/repos/cryptodev-2.6/drivers/crypto/caam/regs.h:151:
undefined reference to `.ioread64_lo_hi'
/home/b05471/repos/cryptodev-2.6/drivers/crypto/caam/regs.h:151:
undefined reference to `.ioread64_lo_hi'
/home/b05471/repos/cryptodev-2.6/drivers/crypto/caam/regs.h:154:
undefined reference to `.ioread64be_lo_hi'
drivers/crypto/caam/jr.o: In function `wr_reg64':
/home/b05471/repos/cryptodev-2.6/drivers/crypto/caam/regs.h:141:
undefined reference to `.iowrite64_lo_hi'
/home/b05471/repos/cryptodev-2.6/drivers/crypto/caam/regs.h:141:
undefined reference to `.iowrite64_lo_hi'
/home/b05471/repos/cryptodev-2.6/drivers/crypto/caam/regs.h:144:
undefined reference to `.iowrite64be_lo_hi'
/home/b05471/repos/cryptodev-2.6/drivers/crypto/caam/regs.h:144:
undefined reference to `.iowrite64be_lo_hi'
make: *** [vmlinux] Error 1

Regards,
Horia

[Michael Ellerman]

OK. I don't think I see any way it can break anything, so feel free to
merge it, it'll get boot testing on powerpc once it's in linux-next.

Acked-by: Michael Ellerman <m...@ellerman.id.au>

cheers

[Horia Geantă]

include/asm-generic/iomap.h is included before
include/linux/io-64-nonatomic-lo-hi.h:

include/linux/io.h -> arch/powerpc/include/asm/io.h ->
include/asm-generic/iomap.h

Thus, the "extern" version of ioread64_lo_hi and friends will be used
(and not the inline version from io-64-nonatomic-lo-hi.h).

But for this kernel .config:
-CONFIG_GENERIC_IOMAP=n and
-there is no implementation of io{read|write}64[be]{_lo_hi|_hi_lo} in
arch/powerpc/kernel/iomap.c

Regards,
Horia

[Logan Gunthorpe]

On 20/07/17 06:36 AM, Horia Geantă wrote:
> include/asm-generic/iomap.h is included before
> include/linux/io-64-nonatomic-lo-hi.h:
>
> include/linux/io.h -> arch/powerpc/include/asm/io.h ->
> include/asm-generic/iomap.h
>
> Thus, the "extern" version of ioread64_lo_hi and friends will be used
> (and not the inline version from io-64-nonatomic-lo-hi.h).
>
> But for this kernel .config:
> -CONFIG_GENERIC_IOMAP=n and
> -there is no implementation of io{read|write}64[be]{_lo_hi|_hi_lo} in
> arch/powerpc/kernel/iomap.c

Ah, thanks for the triage. I'll take a look at fixing this for a v5.

Logan

[Logan Gunthorpe]

Changes since v4:
- Add functions so the powerpc implementation of iomap.c compiles. (As
noticed by Horia)

Changes since v3:

- I noticed powerpc didn't use the appropriate functions seeing
readq/writeq were not defined when iomap.h was included. Thus I've
included a patch to adjust this
- Fixed some mistakes with a couple of the defines in io-64-nonatomic*
headers
- Fixed a typo noticed by Horia.

(earlier versions were drastically different)

Horia Geantă (1):
crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

Logan Gunthorpe (5):

powerpc: io.h: move iomap.h include so that it can use readq/writeq
defs

powerpc: iomap.c: introduce io{read|write}64_{lo_hi|hi_lo}

iomap: introduce io{read|write}64_{lo_hi|hi_lo}
io-64-nonatomic: add io{read|write}64[be]{_lo_hi|_hi_lo} macros
ntb: ntb_hw_intel: use io-64-nonatomic instead of in-driver hacks

arch/powerpc/include/asm/io.h | 6 +-

arch/powerpc/kernel/iomap.c | 40 +++++++++++

drivers/crypto/caam/regs.h | 35 ++-------
drivers/ntb/hw/intel/ntb_hw_intel.c | 30 +-------
include/asm-generic/iomap.h | 26 +++++--
include/linux/io-64-nonatomic-hi-lo.h | 60 ++++++++++++++++
include/linux/io-64-nonatomic-lo-hi.h | 60 ++++++++++++++++
lib/iomap.c | 132 ++++++++++++++++++++++++++++++++++

8 files changed, 322 insertions(+), 67 deletions(-)

--
2.11.0

[Logan Gunthorpe]

These functions will be introduced into the generic iomap.c so
they can deal with PIO accesses in hi-lo/lo-hi variants. Thus,
the powerpc version of iomap.c will need to provide the same
functions even though, in this arch, they are identical to the
regular io{read|write}64 functions.
---
arch/powerpc/kernel/iomap.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)

diff --git a/arch/powerpc/kernel/iomap.c b/arch/powerpc/kernel/iomap.c
index a1854d1ded8b..b43dbadfd24f 100644
--- a/arch/powerpc/kernel/iomap.c
+++ b/arch/powerpc/kernel/iomap.c
@@ -44,12 +44,32 @@ u64 ioread64(void __iomem *addr)
{
return readq(addr);

}
+u64 ioread64_lo_hi(void __iomem *addr)
+{

+ return readq(addr);

+}
+u64 ioread64_hi_lo(void __iomem *addr)
+{

+ return readq(addr);
+}
u64 ioread64be(void __iomem *addr)
{
return readq_be(addr);

}
+u64 ioread64be_lo_hi(void __iomem *addr)
+{

+ return readq_be(addr);

+}
+u64 ioread64be_hi_lo(void __iomem *addr)
+{

+ return readq_be(addr);
+}
EXPORT_SYMBOL(ioread64);
+EXPORT_SYMBOL(ioread64_lo_hi);
+EXPORT_SYMBOL(ioread64_hi_lo);
EXPORT_SYMBOL(ioread64be);
+EXPORT_SYMBOL(ioread64be_lo_hi);
+EXPORT_SYMBOL(ioread64be_hi_lo);
#endif /* __powerpc64__ */

void iowrite8(u8 val, void __iomem *addr)

@@ -82,12 +102,32 @@ void iowrite64(u64 val, void __iomem *addr)
{
writeq(val, addr);

}
+void iowrite64_lo_hi(u64 val, void __iomem *addr)
+{

+ writeq(val, addr);

+}
+void iowrite64_hi_lo(u64 val, void __iomem *addr)
+{

+ writeq(val, addr);
+}
void iowrite64be(u64 val, void __iomem *addr)
{
writeq_be(val, addr);

}
+void iowrite64be_lo_hi(u64 val, void __iomem *addr)
+{

+ writeq_be(val, addr);

+}
+void iowrite64be_hi_lo(u64 val, void __iomem *addr)
+{

+ writeq_be(val, addr);
+}
EXPORT_SYMBOL(iowrite64);
+EXPORT_SYMBOL(iowrite64_lo_hi);
+EXPORT_SYMBOL(iowrite64_hi_lo);
EXPORT_SYMBOL(iowrite64be);
+EXPORT_SYMBOL(iowrite64be_lo_hi);
+EXPORT_SYMBOL(iowrite64be_hi_lo);
#endif /* __powerpc64__ */

/*
--
2.11.0

[Logan Gunthorpe]

Now that ioread64 and iowrite64 are available in io-64-nonatomic,
we can remove the hack at the top of ntb_hw_intel.c and replace it
with an include.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>

[Logan Gunthorpe]

From: Horia Geantă <horia....@nxp.com>

We can now make use of the io-64-nonatomic-lo-hi header to always
provide 64 bit IO operations. So this patch cleans up the extra
CONFIG_64BIT ifdefs.

To be consistent with CAAM engine HW spec: in case of 64-bit registers,
irrespective of device endianness, the lower address should be read from
/ written to first, followed by the upper address. Indeed the I/O
accessors in CAAM driver currently don't follow the spec, however this
is a good opportunity to fix the code.

Signed-off-by: Horia Geantă <horia....@nxp.com>

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>

[Logan Gunthorpe]

+static inline u64 ioread64_hi_lo(void __iomem *addr)
+{

+ u32 low, high;
+
+ high = ioread32(addr + sizeof(u32));
+ low = ioread32(addr);
+
+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64_hi_lo
+#define iowrite64_hi_lo iowrite64_hi_lo

+static inline void iowrite64_hi_lo(u64 val, void __iomem *addr)
+{

+ iowrite32(val >> 32, addr + sizeof(u32));
+ iowrite32(val, addr);
+}
+#endif
+
+#ifndef ioread64be_hi_lo
+#define ioread64be_hi_lo ioread64be_hi_lo

+static inline u64 ioread64be_hi_lo(void __iomem *addr)
+{

+ u32 low, high;
+
+ high = ioread32be(addr);

+ low = ioread32be(addr + sizeof(u32));
+

+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64be_hi_lo
+#define iowrite64be_hi_lo iowrite64be_hi_lo

+static inline void iowrite64be_hi_lo(u64 val, void __iomem *addr)
+{

+static inline u64 ioread64_lo_hi(void __iomem *addr)
+{

+ u32 low, high;
+
+ low = ioread32(addr);

+ high = ioread32(addr + sizeof(u32));
+

+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64_lo_hi
+#define iowrite64_lo_hi iowrite64_lo_hi

+static inline void iowrite64_lo_hi(u64 val, void __iomem *addr)
+{

+ iowrite32(val, addr);
+ iowrite32(val >> 32, addr + sizeof(u32));
+}
+#endif
+
+#ifndef ioread64be_lo_hi
+#define ioread64be_lo_hi ioread64be_lo_hi

+static inline u64 ioread64be_lo_hi(void __iomem *addr)
+{

+ u32 low, high;
+
+ low = ioread32be(addr + sizeof(u32));
+ high = ioread32be(addr);
+
+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64be_lo_hi
+#define iowrite64be_lo_hi iowrite64be_lo_hi

+static inline void iowrite64be_lo_hi(u64 val, void __iomem *addr)
+{

[Logan Gunthorpe]

Subsequent patches in this series makes use of the readq and writeq
defines in iomap.h. However, as is, they get missed on the powerpc
platform seeing the include comes before the define. This patch
moves the include down to fix this.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>

Acked-By: Michael Ellerman <m...@ellerman.id.au>

Cc: Benjamin Herrenschmidt <be...@kernel.crashing.org>
Cc: Paul Mackerras <pau...@samba.org>
Cc: Michael Ellerman <m...@ellerman.id.au>
Cc: Nicholas Piggin <npi...@gmail.com>
Cc: Suresh Warrier <war...@linux.vnet.ibm.com>
Cc: "Oliver O'Halloran" <ooh...@gmail.com>
---
arch/powerpc/include/asm/io.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 422f99cf9924..af074923d598 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h

[Logan Gunthorpe]

In order to provide non-atomic functions for io{read|write}64 that will
use readq and writeq when appropriate. We define a number of variants
of these functions in the generic iomap that will do non-atomic
operations on pio but atomic operations on mmio.

These functions are only defined if readq and writeq are defined. If
they are not, then the wrappers that always use non-atomic operations
from include/linux/io-64-nonatomic*.h will be used.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>

Cc: Benjamin Herrenschmidt <be...@kernel.crashing.org>
Cc: Paul Mackerras <pau...@samba.org>
Cc: Michael Ellerman <m...@ellerman.id.au>

Cc: Arnd Bergmann <ar...@arndb.de>
Cc: Suresh Warrier <war...@linux.vnet.ibm.com>

Cc: Nicholas Piggin <npi...@gmail.com>
---
arch/powerpc/include/asm/io.h | 2 +
include/asm-generic/iomap.h | 26 +++++++--
lib/iomap.c | 132 ++++++++++++++++++++++++++++++++++++++++++

3 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index af074923d598..4cc420cfaa78 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h

@@ -788,8 +788,10 @@ extern void __iounmap_at(void *ea, unsigned long size);

#define mmio_read16be(addr) readw_be(addr)
#define mmio_read32be(addr) readl_be(addr)
+#define mmio_read64be(addr) readq_be(addr)
#define mmio_write16be(val, addr) writew_be(val, addr)
#define mmio_write32be(val, addr) writel_be(val, addr)
+#define mmio_write64be(val, addr) writeq_be(val, addr)
#define mmio_insb(addr, dst, count) readsb(addr, dst, count)
#define mmio_insw(addr, dst, count) readsw(addr, dst, count)
#define mmio_insl(addr, dst, count) readsl(addr, dst, count)
diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h

index 650fede33c25..30edebf627fe 100644

+extern void iowrite64_lo_hi(u64 val, void __iomem *addr);
+extern void iowrite64_hi_lo(u64 val, void __iomem *addr);
+extern void iowrite64be_lo_hi(u64 val, void __iomem *addr);
+extern void iowrite64be_hi_lo(u64 val, void __iomem *addr);

#endif

/*
diff --git a/lib/iomap.c b/lib/iomap.c
index fc3dcb4b238e..b993400d60bd 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -66,6 +66,7 @@ static void bad_io_access(unsigned long port, const char *access)
#ifndef mmio_read16be
#define mmio_read16be(addr) be16_to_cpu(__raw_readw(addr))
#define mmio_read32be(addr) be32_to_cpu(__raw_readl(addr))
+#define mmio_read64be(addr) be64_to_cpu(__raw_readq(addr))
#endif

unsigned int ioread8(void __iomem *addr)
@@ -99,6 +100,80 @@ EXPORT_SYMBOL(ioread16be);
EXPORT_SYMBOL(ioread32);
EXPORT_SYMBOL(ioread32be);

+#ifdef readq
+static u64 pio_read64_lo_hi(unsigned long port)
+{
+ u64 lo, hi;
+
+ lo = inl(port);

+ hi = inl(port + sizeof(u32));
+

+ lo = pio_read32be(port + sizeof(u32));
+

+ return lo | (hi << 32);
+}
+

+u64 ioread64_lo_hi(void __iomem *addr)
+{

+ IO_COND(addr, return pio_read64_lo_hi(port), return readq(addr));
+ return 0xffffffffffffffffLL;
+}
+

+u64 ioread64_hi_lo(void __iomem *addr)
+{

+ IO_COND(addr, return pio_read64_hi_lo(port), return readq(addr));
+ return 0xffffffffffffffffLL;
+}
+

+u64 ioread64be_lo_hi(void __iomem *addr)
+{

+ IO_COND(addr, return pio_read64be_lo_hi(port),
+ return mmio_read64be(addr));
+ return 0xffffffffffffffffLL;
+}
+

+u64 ioread64be_hi_lo(void __iomem *addr)
+{

+ IO_COND(addr, return pio_read64be_hi_lo(port),
+ return mmio_read64be(addr));
+ return 0xffffffffffffffffLL;
+}
+
+EXPORT_SYMBOL(ioread64_lo_hi);
+EXPORT_SYMBOL(ioread64_hi_lo);
+EXPORT_SYMBOL(ioread64be_lo_hi);
+EXPORT_SYMBOL(ioread64be_hi_lo);
+
+#endif /* readq */
+
#ifndef pio_write16be
#define pio_write16be(val,port) outw(swab16(val),port)
#define pio_write32be(val,port) outl(swab32(val),port)
@@ -107,6 +182,7 @@ EXPORT_SYMBOL(ioread32be);
#ifndef mmio_write16be
#define mmio_write16be(val,port) __raw_writew(be16_to_cpu(val),port)
#define mmio_write32be(val,port) __raw_writel(be32_to_cpu(val),port)
+#define mmio_write64be(val,port) __raw_writeq(be64_to_cpu(val),port)
#endif

void iowrite8(u8 val, void __iomem *addr)

+void iowrite64_lo_hi(u64 val, void __iomem *addr)
+{

+ IO_COND(addr, pio_write64_lo_hi(val, port),
+ writeq(val, addr));
+}
+

+void iowrite64_hi_lo(u64 val, void __iomem *addr)
+{

+ IO_COND(addr, pio_write64_hi_lo(val, port),
+ writeq(val, addr));
+}
+

+void iowrite64be_lo_hi(u64 val, void __iomem *addr)
+{

+ IO_COND(addr, pio_write64be_lo_hi(val, port),
+ mmio_write64be(val, addr));
+}
+

+void iowrite64be_hi_lo(u64 val, void __iomem *addr)
+{

[Andy Shevchenko]

On Thu, Jul 27, 2017 at 2:19 AM, Logan Gunthorpe <log...@deltatee.com> wrote:
> In order to provide non-atomic functions for io{read|write}64 that will
> use readq and writeq when appropriate. We define a number of variants
> of these functions in the generic iomap that will do non-atomic
> operations on pio but atomic operations on mmio.
>
> These functions are only defined if readq and writeq are defined. If
> they are not, then the wrappers that always use non-atomic operations
> from include/linux/io-64-nonatomic*.h will be used.

Don't you see here a slight problem?

In some cases we want to substitute atomic in favour of non-atomic
when both are defined.
So, please don't do this "smartly".

> +u64 ioread64_lo_hi(void __iomem *addr)
> +{
> + IO_COND(addr, return pio_read64_lo_hi(port), return readq(addr));
> + return 0xffffffffffffffffLL;
> +}

U missed u.

--
With Best Regards,
Andy Shevchenko

[Horia Geantă]

On 7/27/2017 2:19 AM, Logan Gunthorpe wrote:
> Changes since v4:
> - Add functions so the powerpc implementation of iomap.c compiles. (As
> noticed by Horia)

Tested-by: Horia Geantă <horia....@nxp.com>

more exactly: crypto self-tests pass on CAAM crypto engine
on NXP platforms LS1046A (ARMv8 A53), T1040 (PPC64 e5500), P4080 (PPC
e500mc).

[Logan Gunthorpe]

On 30/07/17 10:03 AM, Andy Shevchenko wrote:
> On Thu, Jul 27, 2017 at 2:19 AM, Logan Gunthorpe <log...@deltatee.com> wrote:
>> In order to provide non-atomic functions for io{read|write}64 that will
>> use readq and writeq when appropriate. We define a number of variants
>> of these functions in the generic iomap that will do non-atomic
>> operations on pio but atomic operations on mmio.
>>
>> These functions are only defined if readq and writeq are defined. If
>> they are not, then the wrappers that always use non-atomic operations
>> from include/linux/io-64-nonatomic*.h will be used.
>
> Don't you see here a slight problem?
>
> In some cases we want to substitute atomic in favour of non-atomic
> when both are defined.
> So, please don't do this "smartly".

I'm not sure what you mean here. The driver should use ioread64 and
include an io-64-nonatomic header. Then there are three cases:

1) The arch has no atomic 64 bit io operations defined. In this case it
uses the non-atomic inline function in the io-64-nonatomic header.

2) The arch uses CONFIG_GENERIC_IOMAP and has readq defined, but not
ioread64 defined (likely because pio can't do atomic 64 bit operations
but mmio can). In this case we need to use the ioread64_xx functions
defined in iomap.c which do atomic mmio and non-atomic pio.

3) The arch has ioread64 defined so the atomic operation is used.

>> +u64 ioread64_lo_hi(void __iomem *addr)
>> +{
>> + IO_COND(addr, return pio_read64_lo_hi(port), return readq(addr));
>> + return 0xffffffffffffffffLL;
>> +}
>
> U missed u.

I'll fix this in the next revision.

Thanks,

Logan

[Andy Shevchenko]

On Mon, Jul 31, 2017 at 6:55 PM, Logan Gunthorpe <log...@deltatee.com> wrote:
> On 30/07/17 10:03 AM, Andy Shevchenko wrote:
>> On Thu, Jul 27, 2017 at 2:19 AM, Logan Gunthorpe <log...@deltatee.com> wrote:
>>> In order to provide non-atomic functions for io{read|write}64 that will
>>> use readq and writeq when appropriate. We define a number of variants
>>> of these functions in the generic iomap that will do non-atomic
>>> operations on pio but atomic operations on mmio.
>>>
>>> These functions are only defined if readq and writeq are defined. If
>>> they are not, then the wrappers that always use non-atomic operations
>>> from include/linux/io-64-nonatomic*.h will be used.
>>
>> Don't you see here a slight problem?
>>
>> In some cases we want to substitute atomic in favour of non-atomic
>> when both are defined.
>> So, please don't do this "smartly".
>
> I'm not sure what you mean here. The driver should use ioread64 and
> include an io-64-nonatomic header. Then there are three cases:
>
> 1) The arch has no atomic 64 bit io operations defined. In this case it
> uses the non-atomic inline function in the io-64-nonatomic header.

Okay

> 2) The arch uses CONFIG_GENERIC_IOMAP and has readq defined, but not
> ioread64 defined (likely because pio can't do atomic 64 bit operations
> but mmio can). In this case we need to use the ioread64_xx functions
> defined in iomap.c which do atomic mmio and non-atomic pio.

Not okay.

Some drivers (hardware) would like to have non-atomic MMIO accesses
when readq() defined

> 3) The arch has ioread64 defined so the atomic operation is used.

Not okay. Same reason as above.

In case of readq() / writeq() it's defined by the order of inclusion:

1)
include <...non-atomic...>
include <linux/io.h>

Always non-atomic will be used.

2)
include <linux/io.h>
include <...non-atomic...>

Auto switch like you described.

I don't like above solution, since it's fragile, but few drivers depend on that.

If you wish to do it always like 2) perhaps we need to split accessors
to ones for fixed bus width and ones for atomic/non-atomic cases.
OTOH, it would be done by introducing
memcpyXX_fromio()
memcpyXX_toio()
memsetXX_io()

Where XX = 64, 32, 16, 8.

Note, that ioreadXX_rep() is not the same as above.

P.S. I have done a table of comparison between IO accessors in Linux
kernel and it looks hell out of being consistent.

[Logan Gunthorpe]

On 31/07/17 10:10 AM, Andy Shevchenko wrote:
> Some drivers (hardware) would like to have non-atomic MMIO accesses
> when readq() defined

Huh? But that's the whole point of the io64-nonatomic header. If a
driver wants a specific non-atomic access they should just code two 32
bit accesses.

> In case of readq() / writeq() it's defined by the order of inclusion:
>
> 1)
> include <...non-atomic...>
> include <linux/io.h>
>
> Always non-atomic will be used.

I'm afraid you're wrong about this. The io-64-nonatomic-xx header
includes linux/io.h. Thus the order of the includes doesn't matter and
it will always auto switch. In any case, making an interface do
different things depending on the order of include files is *completely*
insane.

> P.S. I have done a table of comparison between IO accessors in Linux
> kernel and it looks hell out of being consistent.

There are a few corner oddities but it's really not that bad. Most
things are done for a reason if you dig into them.

Logan

[Andy Shevchenko]

On Mon, Jul 31, 2017 at 7:31 PM, Logan Gunthorpe <log...@deltatee.com> wrote:
> On 31/07/17 10:10 AM, Andy Shevchenko wrote:
>> Some drivers (hardware) would like to have non-atomic MMIO accesses
>> when readq() defined
>
> Huh? But that's the whole point of the io64-nonatomic header. If a
> driver wants a specific non-atomic access they should just code two 32
> bit accesses.

You mean to call them directly as lo_hi_XXX() or hi_lo_XXX() ?
Yes it would work.

>> In case of readq() / writeq() it's defined by the order of inclusion:
>>
>> 1)
>> include <...non-atomic...>
>> include <linux/io.h>
>>
>> Always non-atomic will be used.
>
> I'm afraid you're wrong about this. The io-64-nonatomic-xx header
> includes linux/io.h. Thus the order of the includes doesn't matter and
> it will always auto switch. In any case, making an interface do
> different things depending on the order of include files is *completely*
> insane.

Yes, you are right. I was thinking about something unrelated.

[Logan Gunthorpe]

On 31/07/17 11:58 AM, Andy Shevchenko wrote:
> On Mon, Jul 31, 2017 at 7:31 PM, Logan Gunthorpe <log...@deltatee.com> wrote:
>> On 31/07/17 10:10 AM, Andy Shevchenko wrote:
>>> Some drivers (hardware) would like to have non-atomic MMIO accesses
>>> when readq() defined
>>
>> Huh? But that's the whole point of the io64-nonatomic header. If a
>> driver wants a specific non-atomic access they should just code two 32
>> bit accesses.

> You mean to call them directly as lo_hi_XXX() or hi_lo_XXX() ?
> Yes it would work.

I suppose you could do that too but I really meant just using two io32
calls. That's the most explicit way to indicate you want a non-atomic
access.

Logan

[Andy Shevchenko]

Per commit 3a044178cccf they are exactly created for this kind of cases.

[Logan Gunthorpe]

On 31/07/17 12:03 PM, Andy Shevchenko wrote:
>
> Per commit 3a044178cccf they are exactly created for this kind of cases.
>

Sure, ok, and my patchset provides the same set of functions to satisfy
such a use.

Logan

[Andy Shevchenko]

Okay, please, Cc me for next version, I think I need fresh view on it.
Thanks!

[Jon Mason]

On Wed, Jul 26, 2017 at 05:19:16PM -0600, Logan Gunthorpe wrote:
> Now that ioread64 and iowrite64 are available in io-64-nonatomic,
> we can remove the hack at the top of ntb_hw_intel.c and replace it
> with an include.
>
> Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
> Cc: Jon Mason <jdm...@kudzu.us>

This is okay by me, but I'm assuming that this patch will go through
as part of the series (and not via my tree). If this changes, please
let me know.

Acked-by: Jon Mason <jdm...@kudzu.us>

> Cc: Allen Hubbe <Allen...@emc.com>

You already have Allen's Ack below. So, you can remove this :)

> --
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+...@googlegroups.com.
> To post to this group, send email to linu...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20170726231917.6073-6-logang%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

[Logan Gunthorpe]

Changes since v5:
- Added a fix to the tilcdc driver to ensure it doesn't use the
non-atomic operation. (This includes adding io{read|write}64[be]_is_nonatomic
defines).

Changes since v4:
- Add functions so the powerpc implementation of iomap.c compiles. (As
noticed by Horia)

Changes since v3:

- I noticed powerpc didn't use the appropriate functions seeing
readq/writeq were not defined when iomap.h was included. Thus I've
included a patch to adjust this
- Fixed some mistakes with a couple of the defines in io-64-nonatomic*
headers
- Fixed a typo noticed by Horia.

(earlier versions were drastically different)

Horia Geantă (1):
crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

Logan Gunthorpe (6):
drm/tilcdc: ensure nonatomic iowrite64 is not used

powerpc: io.h: move iomap.h include so that it can use readq/writeq
defs
powerpc: iomap.c: introduce io{read|write}64_{lo_hi|hi_lo}
iomap: introduce io{read|write}64_{lo_hi|hi_lo}
io-64-nonatomic: add io{read|write}64[be]{_lo_hi|_hi_lo} macros
ntb: ntb_hw_intel: use io-64-nonatomic instead of in-driver hacks

arch/powerpc/include/asm/io.h | 6 +-
arch/powerpc/kernel/iomap.c | 40 +++++++++++
drivers/crypto/caam/regs.h | 35 ++-------

drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 +-

drivers/ntb/hw/intel/ntb_hw_intel.c | 30 +-------
include/asm-generic/iomap.h | 26 +++++--

include/linux/io-64-nonatomic-hi-lo.h | 64 +++++++++++++++++
include/linux/io-64-nonatomic-lo-hi.h | 64 +++++++++++++++++
lib/iomap.c | 132 ++++++++++++++++++++++++++++++++++
9 files changed, 331 insertions(+), 68 deletions(-)

--
2.11.0

[Logan Gunthorpe]

Now that ioread64 and iowrite64 are available in io-64-nonatomic,
we can remove the hack at the top of ntb_hw_intel.c and replace it
with an include.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>

Acked-by: Dave Jiang <dave....@intel.com>
Acked-by: Allen Hubbe <Allen...@dell.com>

Acked-by: Jon Mason <jdm...@kudzu.us>

[Logan Gunthorpe]

Add a check to ensure iowrite64 is only used if it is atomic.

It was decided in [1] that the tilcdc driver should not be using an
atomic operation (so it was left out of this patchset). However, it turns
out that through the drm code, a nonatomic header is actually included:

include/linux/io-64-nonatomic-lo-hi.h
is included from include/drm/drm_os_linux.h:9:0,
from include/drm/drmP.h:74,
from include/drm/drm_modeset_helper.h:26,
from include/drm/drm_atomic_helper.h:33,
from drivers/gpu/drm/tilcdc/tilcdc_crtc.c:19:

And thus, without this change, this patchset would inadvertantly
change the behaviour of the tilcdc driver.

[1] lkml.kernel.org/r/CAK8P3a2HhO_zCnsTzq7hmWSz...@mail.gmail.com

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
Cc: Jyri Sarha <jsa...@ti.com>
Cc: Arnd Bergmann <ar...@arndb.de>
Cc: Tomi Valkeinen <tomi.va...@ti.com>
Cc: David Airlie <air...@linux.ie>
---
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
index 9d528c0a67a4..5048ebb86835 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
@@ -133,7 +133,7 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data)
struct tilcdc_drm_private *priv = dev->dev_private;
volatile void __iomem *addr = priv->mmio + reg;

-#ifdef iowrite64
+#if defined(iowrite64) && !defined(iowrite64_is_nonatomic)
iowrite64(data, addr);
#else
__iowmb();
--
2.11.0

[Logan Gunthorpe]

This patch adds generic io{read|write}64[be]{_lo_hi|_hi_lo} macros if
they are not already defined by the architecture. (As they are provided
by the generic iomap library).

The patch also points io{read|write}64[be] to the variant specified by the
header name.

This is because new drivers are encouraged to use ioreadXX, et al instead
of readX[1], et al -- and mixing ioreadXX with readq is pretty ugly.

[1] ldd3: section 9.4.2

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
cc: Christoph Hellwig <h...@lst.de>
cc: Arnd Bergmann <ar...@arndb.de>
cc: Alan Cox <gno...@lxorguk.ukuu.org.uk>
cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---

include/linux/io-64-nonatomic-hi-lo.h | 64 +++++++++++++++++++++++++++++++++++
include/linux/io-64-nonatomic-lo-hi.h | 64 +++++++++++++++++++++++++++++++++++
2 files changed, 128 insertions(+)

diff --git a/include/linux/io-64-nonatomic-hi-lo.h b/include/linux/io-64-nonatomic-hi-lo.h
index defcc4644ce3..410c8b177080 100644
--- a/include/linux/io-64-nonatomic-hi-lo.h
+++ b/include/linux/io-64-nonatomic-hi-lo.h
@@ -54,4 +54,68 @@ static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr)

#define writeq_relaxed hi_lo_writeq_relaxed
#endif

+#ifndef ioread64_hi_lo
+#define ioread64_hi_lo ioread64_hi_lo

+static inline u64 ioread64_hi_lo(void __iomem *addr)
+{

+ u32 low, high;
+
+ high = ioread32(addr + sizeof(u32));
+ low = ioread32(addr);
+
+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64_hi_lo
+#define iowrite64_hi_lo iowrite64_hi_lo
+static inline void iowrite64_hi_lo(u64 val, void __iomem *addr)
+{
+ iowrite32(val >> 32, addr + sizeof(u32));
+ iowrite32(val, addr);
+}
+#endif
+
+#ifndef ioread64be_hi_lo
+#define ioread64be_hi_lo ioread64be_hi_lo

+static inline u64 ioread64be_hi_lo(void __iomem *addr)
+{

+ u32 low, high;
+
+ high = ioread32be(addr);

+ low = ioread32be(addr + sizeof(u32));
+

+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64be_hi_lo
+#define iowrite64be_hi_lo iowrite64be_hi_lo
+static inline void iowrite64be_hi_lo(u64 val, void __iomem *addr)
+{
+ iowrite32be(val >> 32, addr);
+ iowrite32be(val, addr + sizeof(u32));
+}
+#endif
+
+#ifndef ioread64

+#define ioread64_is_nonatomic

+#define ioread64 ioread64_hi_lo
+#endif
+
+#ifndef iowrite64

+#define iowrite64_is_nonatomic

+#define iowrite64 iowrite64_hi_lo
+#endif
+
+#ifndef ioread64be

+#define ioread64be_is_nonatomic

+#define ioread64be ioread64be_hi_lo
+#endif
+
+#ifndef iowrite64be

+#define iowrite64be_is_nonatomic

+#define iowrite64be iowrite64be_hi_lo
+#endif
+
#endif /* _LINUX_IO_64_NONATOMIC_HI_LO_H_ */
diff --git a/include/linux/io-64-nonatomic-lo-hi.h b/include/linux/io-64-nonatomic-lo-hi.h

index 084461a4e5ab..acba36812be8 100644
--- a/include/linux/io-64-nonatomic-lo-hi.h
+++ b/include/linux/io-64-nonatomic-lo-hi.h
@@ -54,4 +54,68 @@ static inline void lo_hi_writeq_relaxed(__u64 val, volatile void __iomem *addr)

#define writeq_relaxed lo_hi_writeq_relaxed
#endif

+#ifndef ioread64_lo_hi
+#define ioread64_lo_hi ioread64_lo_hi

+static inline u64 ioread64_lo_hi(void __iomem *addr)
+{

+ u32 low, high;
+
+ low = ioread32(addr);

+ high = ioread32(addr + sizeof(u32));
+

+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64_lo_hi
+#define iowrite64_lo_hi iowrite64_lo_hi
+static inline void iowrite64_lo_hi(u64 val, void __iomem *addr)
+{
+ iowrite32(val, addr);
+ iowrite32(val >> 32, addr + sizeof(u32));
+}
+#endif
+
+#ifndef ioread64be_lo_hi
+#define ioread64be_lo_hi ioread64be_lo_hi

+static inline u64 ioread64be_lo_hi(void __iomem *addr)
+{

+ u32 low, high;
+
+ low = ioread32be(addr + sizeof(u32));
+ high = ioread32be(addr);
+
+ return low + ((u64)high << 32);
+}
+#endif
+
+#ifndef iowrite64be_lo_hi
+#define iowrite64be_lo_hi iowrite64be_lo_hi
+static inline void iowrite64be_lo_hi(u64 val, void __iomem *addr)
+{
+ iowrite32be(val, addr + sizeof(u32));
+ iowrite32be(val >> 32, addr);
+}
+#endif
+
+#ifndef ioread64

+#define ioread64_is_nonatomic

+#define ioread64 ioread64_lo_hi
+#endif
+
+#ifndef iowrite64

+#define iowrite64_is_nonatomic

+#define iowrite64 iowrite64_lo_hi
+#endif
+
+#ifndef ioread64be

+#define ioread64be_is_nonatomic

+#define ioread64be ioread64be_lo_hi
+#endif
+
+#ifndef iowrite64be

+#define iowrite64be_is_nonatomic

[Logan Gunthorpe]

From: Horia Geantă <horia....@nxp.com>

We can now make use of the io-64-nonatomic-lo-hi header to always
provide 64 bit IO operations. So this patch cleans up the extra
CONFIG_64BIT ifdefs.

To be consistent with CAAM engine HW spec: in case of 64-bit registers,
irrespective of device endianness, the lower address should be read from
/ written to first, followed by the upper address. Indeed the I/O
accessors in CAAM driver currently don't follow the spec, however this
is a good opportunity to fix the code.

Signed-off-by: Horia Geantă <horia....@nxp.com>

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>

[Logan Gunthorpe]

These functions will be introduced into the generic iomap.c so
they can deal with PIO accesses in hi-lo/lo-hi variants. Thus,
the powerpc version of iomap.c will need to provide the same
functions even though, in this arch, they are identical to the
regular io{read|write}64 functions.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
Tested-by: Horia Geantă <horia....@nxp.com>

Cc: Benjamin Herrenschmidt <be...@kernel.crashing.org>
Cc: Paul Mackerras <pau...@samba.org>
Cc: Michael Ellerman <m...@ellerman.id.au>

---
arch/powerpc/kernel/iomap.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)

diff --git a/arch/powerpc/kernel/iomap.c b/arch/powerpc/kernel/iomap.c
index a1854d1ded8b..b43dbadfd24f 100644
--- a/arch/powerpc/kernel/iomap.c
+++ b/arch/powerpc/kernel/iomap.c
@@ -44,12 +44,32 @@ u64 ioread64(void __iomem *addr)
{
return readq(addr);
}

+u64 ioread64_lo_hi(void __iomem *addr)
+{
+ return readq(addr);
+}
+u64 ioread64_hi_lo(void __iomem *addr)
+{

+ return readq(addr);
+}
u64 ioread64be(void __iomem *addr)
{
return readq_be(addr);
}

+u64 ioread64be_lo_hi(void __iomem *addr)
+{
+ return readq_be(addr);
+}
+u64 ioread64be_hi_lo(void __iomem *addr)
+{

+ return readq_be(addr);
+}
EXPORT_SYMBOL(ioread64);
+EXPORT_SYMBOL(ioread64_lo_hi);
+EXPORT_SYMBOL(ioread64_hi_lo);
EXPORT_SYMBOL(ioread64be);
+EXPORT_SYMBOL(ioread64be_lo_hi);
+EXPORT_SYMBOL(ioread64be_hi_lo);
#endif /* __powerpc64__ */

void iowrite8(u8 val, void __iomem *addr)

@@ -82,12 +102,32 @@ void iowrite64(u64 val, void __iomem *addr)
{
writeq(val, addr);
}

+void iowrite64_lo_hi(u64 val, void __iomem *addr)
+{
+ writeq(val, addr);
+}
+void iowrite64_hi_lo(u64 val, void __iomem *addr)
+{

+ writeq(val, addr);
+}
void iowrite64be(u64 val, void __iomem *addr)
{
writeq_be(val, addr);
}

+void iowrite64be_lo_hi(u64 val, void __iomem *addr)
+{
+ writeq_be(val, addr);
+}
+void iowrite64be_hi_lo(u64 val, void __iomem *addr)
+{

[Logan Gunthorpe]

Subsequent patches in this series makes use of the readq and writeq
defines in iomap.h. However, as is, they get missed on the powerpc
platform seeing the include comes before the define. This patch
moves the include down to fix this.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
Acked-By: Michael Ellerman <m...@ellerman.id.au>

Cc: Benjamin Herrenschmidt <be...@kernel.crashing.org>
Cc: Paul Mackerras <pau...@samba.org>
Cc: Michael Ellerman <m...@ellerman.id.au>

Cc: Nicholas Piggin <npi...@gmail.com>
Cc: Suresh Warrier <war...@linux.vnet.ibm.com>

Cc: "Oliver O'Halloran" <ooh...@gmail.com>
---
arch/powerpc/include/asm/io.h | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 422f99cf9924..af074923d598 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h

[Logan Gunthorpe]

In order to provide non-atomic functions for io{read|write}64 that will
use readq and writeq when appropriate. We define a number of variants
of these functions in the generic iomap that will do non-atomic
operations on pio but atomic operations on mmio.

These functions are only defined if readq and writeq are defined. If
they are not, then the wrappers that always use non-atomic operations
from include/linux/io-64-nonatomic*.h will be used.

Signed-off-by: Logan Gunthorpe <log...@deltatee.com>

Cc: Benjamin Herrenschmidt <be...@kernel.crashing.org>
Cc: Paul Mackerras <pau...@samba.org>
Cc: Michael Ellerman <m...@ellerman.id.au>

Cc: Arnd Bergmann <ar...@arndb.de>
Cc: Suresh Warrier <war...@linux.vnet.ibm.com>

Cc: Nicholas Piggin <npi...@gmail.com>
---
arch/powerpc/include/asm/io.h | 2 +
include/asm-generic/iomap.h | 26 +++++++--
lib/iomap.c | 132 ++++++++++++++++++++++++++++++++++++++++++

3 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index af074923d598..4cc420cfaa78 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h

index fc3dcb4b238e..845b9c41082c 100644

--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -66,6 +66,7 @@ static void bad_io_access(unsigned long port, const char *access)
#ifndef mmio_read16be
#define mmio_read16be(addr) be16_to_cpu(__raw_readw(addr))
#define mmio_read32be(addr) be32_to_cpu(__raw_readl(addr))
+#define mmio_read64be(addr) be64_to_cpu(__raw_readq(addr))
#endif

unsigned int ioread8(void __iomem *addr)
@@ -99,6 +100,80 @@ EXPORT_SYMBOL(ioread16be);
EXPORT_SYMBOL(ioread32);
EXPORT_SYMBOL(ioread32be);

+#ifdef readq
+static u64 pio_read64_lo_hi(unsigned long port)
+{
+ u64 lo, hi;
+
+ lo = inl(port);

+ hi = inl(port + sizeof(u32));
+

+ lo = pio_read32be(port + sizeof(u32));
+

+ return lo | (hi << 32);
+}
+

+u64 ioread64_lo_hi(void __iomem *addr)
+{

+ IO_COND(addr, return pio_read64_lo_hi(port), return readq(addr));

+ return 0xffffffffffffffffULL;
+}

+
+u64 ioread64_hi_lo(void __iomem *addr)
+{

+ IO_COND(addr, return pio_read64_hi_lo(port), return readq(addr));

+ return 0xffffffffffffffffULL;
+}
+

+u64 ioread64be_lo_hi(void __iomem *addr)
+{

+ IO_COND(addr, return pio_read64be_lo_hi(port),
+ return mmio_read64be(addr));

+ return 0xffffffffffffffffULL;
+}

+
+u64 ioread64be_hi_lo(void __iomem *addr)
+{

+ IO_COND(addr, return pio_read64be_hi_lo(port),
+ return mmio_read64be(addr));

+ return 0xffffffffffffffffULL;

+}
+
+EXPORT_SYMBOL(ioread64_lo_hi);
+EXPORT_SYMBOL(ioread64_hi_lo);
+EXPORT_SYMBOL(ioread64be_lo_hi);
+EXPORT_SYMBOL(ioread64be_hi_lo);
+
+#endif /* readq */
+
#ifndef pio_write16be
#define pio_write16be(val,port) outw(swab16(val),port)
#define pio_write32be(val,port) outl(swab32(val),port)
@@ -107,6 +182,7 @@ EXPORT_SYMBOL(ioread32be);
#ifndef mmio_write16be
#define mmio_write16be(val,port) __raw_writew(be16_to_cpu(val),port)
#define mmio_write32be(val,port) __raw_writel(be32_to_cpu(val),port)
+#define mmio_write64be(val,port) __raw_writeq(be64_to_cpu(val),port)
#endif

void iowrite8(u8 val, void __iomem *addr)

+void iowrite64_lo_hi(u64 val, void __iomem *addr)
+{

+ IO_COND(addr, pio_write64_lo_hi(val, port),
+ writeq(val, addr));
+}

+
+void iowrite64_hi_lo(u64 val, void __iomem *addr)
+{

+ IO_COND(addr, pio_write64_hi_lo(val, port),
+ writeq(val, addr));
+}
+

+void iowrite64be_lo_hi(u64 val, void __iomem *addr)
+{

+ IO_COND(addr, pio_write64be_lo_hi(val, port),
+ mmio_write64be(val, addr));
+}

+
+void iowrite64be_hi_lo(u64 val, void __iomem *addr)
+{

[Tomi Valkeinen]

On 03/08/17 21:30, Logan Gunthorpe wrote:
> Add a check to ensure iowrite64 is only used if it is atomic.
>
> It was decided in [1] that the tilcdc driver should not be using an
> atomic operation (so it was left out of this patchset). However, it turns
> out that through the drm code, a nonatomic header is actually included:
>
> include/linux/io-64-nonatomic-lo-hi.h
> is included from include/drm/drm_os_linux.h:9:0,
> from include/drm/drmP.h:74,
> from include/drm/drm_modeset_helper.h:26,
> from include/drm/drm_atomic_helper.h:33,
> from drivers/gpu/drm/tilcdc/tilcdc_crtc.c:19:
>
> And thus, without this change, this patchset would inadvertantly
> change the behaviour of the tilcdc driver.

I haven't really followed the discussion on this, but if the tilcdc's
use of iowrite64 causes (real) problems/complications elsewhere, I think
we could drop it.

The problem is that the HW has a race issue, and the two registers in
question should be written as close to each other as possible. We
thought a single 64bit write, writing to both registers in one go, would
improve that slightly, compared to two 32 bit writes.

Jyri, correct me if I'm wrong, but we have no proof that it actually
helps, and it might be that even if it helps, the difference is
theoretical. Probably if we ensure the irqs are off when we do two 32
bit writes, we're already close enough to the optimal case.

Tomi

[Logan Gunthorpe]

On 04/08/17 07:03 AM, Tomi Valkeinen wrote:
> I haven't really followed the discussion on this, but if the tilcdc's
> use of iowrite64 causes (real) problems/complications elsewhere, I think
> we could drop it.

Well, that's up to you. The patch I submitted should still be correct
though, and if arm ever gets a proper atomic iowrite64 implementation it
would be good to use it. So in an annotative sense it's nice to keep the
function call in there.

Thanks,

Logan

[Jyri Sarha]

For the sake of this particular case you are right, the atomicity is
probably not that important here. But in general ARM7 has an atomic
64bit write and I think it is a shame if it can not be easily used in
linux kernel.

Best regards,
Jyri