[PATCH v18 0/7] Add io{read|write}64 to io-64-atomic headers

[PATCH v18 0/7] Add io{read|write}64 to io-64-atomic headers

[Logan Gunthorpe]

This is a resend of my cleanup series to push a number of instances of
people defining their own io{read|write}64 functions into common
headers seing they don't exist in non-64bit systems. This series adds
inline functions to the io-64-nonatomic headers and then cleans up the
drivers that defined their own copies.

This cleanup was originally requested by Greg after he reviewed my
Switchtec NTB code.

@Andrew, can you please consider merging this series as it has a
number of cross-tree pieces? It has been around for a number of
cycles, has had some reviews, and a few small pieces of it have been
already accepted through various other trees.

This marks the one year aniversary since I started this series.

Thanks,

Logan

--

Changes since v17:
- Rebased onto v4.18-rc1 (No Changes)

Changes since v16:
- Rebased onto v4.17-rc4 (No Changes)

Changes since v15:
- Rebased onto v4.17-rc1, dropping the powerpc patches which were
  picked up by Michael

Changes since v14:
- Rebased onto v4.16-rc7
- Replace the first two patches so that instead of correcting the
  endianness annotations we change to using writeX() and readX() with
  swabX() calls. This makes the big-endian functions more symmetric
  with the little-endian versions (with respect to barriers that are
  not included in the raw functions). As a side effect, it also fixes
  the kbuild warnings that the first two patches tried to address.

Changes since v13:
- Changed the subject of patch 0001 to correct a nit pointed out by Luc

Changes since v12:
- Rebased onto v4.16-rc6
- Split patch 0001 into two and reworked the commit log as requested
  by Luc Van Oostenryck

Changes since v11:
- Rebased onto v4.16-rc5
- Added a patch (0001) to fix some old and new sparse warnings
  that the kbuild robot warned about this cycle. The latest version
  of sparse was required to reproduce these.
- Added a patch (0002) to add io{read|write}64 to parisc which the kbuild
  robot also found errors for this cycle

Changes since v10:
- Rebased onto v4.16-rc4, this droped the drm/tilcdc patch which was
  picked up by that tree and is already in 4.16.

Changes since v9:
- Rebased onto v4.15-rc6
- Fixed a couple of issues in the new version of the CAAM patch as
  pointed out by Horia

Changes since v8:
- Rebased onto v4.15-rc2, as a result rewrote patch 7 seeing someone did
  some similar cleanup in that area.
- Added a patch to clean up the Switchtec NTB driver which landed in
  v4.15-rc1

Changes since v7:
- Fix minor nits from Andy Shevchenko
- Rebased onto v4.14-rc1

Changes since v6:
 ** none **

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)


Logan Gunthorpe (7):
  iomap: Use non-raw io functions for io{read|write}XXbe
  parisc: iomap: introduce io{read|write}64
  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
  crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64
  ntb: ntb_hw_switchtec: Cleanup 64bit IO defines to use the common
    header

 arch/parisc/include/asm/io.h           |   9 +++
 arch/parisc/lib/iomap.c                |  64 +++++++++++++++
 arch/powerpc/include/asm/io.h          |   2 +
 drivers/crypto/caam/regs.h             |  30 +------
 drivers/ntb/hw/intel/ntb_hw_intel.h    |  30 +------
 drivers/ntb/hw/mscc/ntb_hw_switchtec.c |  36 +--------
 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                            | 140 ++++++++++++++++++++++++++++++++-
 10 files changed, 367 insertions(+), 98 deletions(-)

--
2.11.0

[PATCH v18 1/7] iomap: Use non-raw io functions for io{read|write}XXbe

[Logan Gunthorpe]

Fix an asymmetry in the io{read|write}XXbe functions in that the
big-endian variants make use of the raw io accessors while the
little-endian variants use the regular accessors. Some architectures
implement barriers to order against both spinlocks and DMA accesses
and for these case, the big-endian variant of the API would not be
protected.

Thus, change the mmio_XXXXbe macros to use the appropriate swab() function
wrapping the regular accessor. This is similar to what was done for PIO.

When this code was originally written, barriers in the IO accessors were
not common and the accessors simply wrapped the raw functions in a
conversion to CPU endianness. Since then, barriers have been added in
some architectures and are now missing in the big endian variant of the
API.

This also manages to silence a few sparse warnings that check
for using the correct endian types which the original code did
not annotate correctly.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Philippe Ombredanne <pombredanne@nexb.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Link: http://lkml.kernel.org/r/CAK8P3a25zQDxyaY3iVv+JmSSzs7F6ssGc+HdBkGs54ZfViX+Fg@mail.gmail.com
---
 lib/iomap.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/iomap.c b/lib/iomap.c
index 541d926da95e..2c293b22569f 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -65,8 +65,8 @@ static void bad_io_access(unsigned long port, const char *access)
 #endif
 
 #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_read16be(addr) swab16(readw(addr))
+#define mmio_read32be(addr) swab32(readl(addr))
 #endif
 
 unsigned int ioread8(void __iomem *addr)
@@ -106,8 +106,8 @@ EXPORT_SYMBOL(ioread32be);
 #endif
 
 #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_write16be(val,port) writew(swab16(val),port)
+#define mmio_write32be(val,port) writel(swab32(val),port)
 #endif
 
 void iowrite8(u8 val, void __iomem *addr)
-- 
2.11.0

[PATCH v18 2/7] parisc: iomap: introduce io{read|write}64

[Logan Gunthorpe]

Add support for io{read|write}64() functions in parisc architecture.
These are pretty straightforward copies of similar functions which
make use of readq and writeq.

Also, indicate that the lo_hi and hi_lo variants of these functions
are not provided by this architecture.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Helge Deller <deller@gmx.de>
Cc: "James E.J. Bottomley" <jejb@parisc-linux.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Philippe Ombredanne <pombredanne@nexb.com>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/parisc/include/asm/io.h |  9 +++++++
 arch/parisc/lib/iomap.c      | 64 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/arch/parisc/include/asm/io.h b/arch/parisc/include/asm/io.h
index afe493b23d04..30a8315d5c07 100644
--- a/arch/parisc/include/asm/io.h
+++ b/arch/parisc/include/asm/io.h
@@ -311,6 +311,15 @@ extern void outsl (unsigned long port, const void *src, unsigned long count);
  * value for either 32 or 64 bit mode */
 #define F_EXTEND(x) ((unsigned long)((x) | (0xffffffff00000000ULL)))
 
+#define ioread64 ioread64
+#define ioread64be ioread64be
+#define iowrite64 iowrite64
+#define iowrite64be iowrite64be
+extern u64 ioread64(void __iomem *addr);
+extern u64 ioread64be(void __iomem *addr);
+extern void iowrite64(u64 val, void __iomem *addr);
+extern void iowrite64be(u64 val, void __iomem *addr);
+
 #include <asm-generic/iomap.h>
 
 /*
diff --git a/arch/parisc/lib/iomap.c b/arch/parisc/lib/iomap.c
index 4b19e6e64fb7..0195aec657e2 100644
--- a/arch/parisc/lib/iomap.c
+++ b/arch/parisc/lib/iomap.c
@@ -48,11 +48,15 @@ struct iomap_ops {
 	unsigned int (*read16be)(void __iomem *);
 	unsigned int (*read32)(void __iomem *);
 	unsigned int (*read32be)(void __iomem *);
+	u64 (*read64)(void __iomem *);
+	u64 (*read64be)(void __iomem *);
 	void (*write8)(u8, void __iomem *);
 	void (*write16)(u16, void __iomem *);
 	void (*write16be)(u16, void __iomem *);
 	void (*write32)(u32, void __iomem *);
 	void (*write32be)(u32, void __iomem *);
+	void (*write64)(u64, void __iomem *);
+	void (*write64be)(u64, void __iomem *);
 	void (*read8r)(void __iomem *, void *, unsigned long);
 	void (*read16r)(void __iomem *, void *, unsigned long);
 	void (*read32r)(void __iomem *, void *, unsigned long);
@@ -171,6 +175,16 @@ static unsigned int iomem_read32be(void __iomem *addr)
 	return __raw_readl(addr);
 }
 
+static u64 iomem_read64(void __iomem *addr)
+{
+	return readq(addr);
+}
+
+static u64 iomem_read64be(void __iomem *addr)
+{
+	return __raw_readq(addr);
+}
+
 static void iomem_write8(u8 datum, void __iomem *addr)
 {
 	writeb(datum, addr);
@@ -196,6 +210,16 @@ static void iomem_write32be(u32 datum, void __iomem *addr)
 	__raw_writel(datum, addr);
 }
 
+static void iomem_write64(u64 datum, void __iomem *addr)
+{
+	writel(datum, addr);
+}
+
+static void iomem_write64be(u64 datum, void __iomem *addr)
+{
+	__raw_writel(datum, addr);
+}
+
 static void iomem_read8r(void __iomem *addr, void *dst, unsigned long count)
 {
 	while (count--) {
@@ -250,11 +274,15 @@ static const struct iomap_ops iomem_ops = {
 	.read16be = iomem_read16be,
 	.read32 = iomem_read32,
 	.read32be = iomem_read32be,
+	.read64 = iomem_read64,
+	.read64be = iomem_read64be,
 	.write8 = iomem_write8,
 	.write16 = iomem_write16,
 	.write16be = iomem_write16be,
 	.write32 = iomem_write32,
 	.write32be = iomem_write32be,
+	.write64 = iomem_write64,
+	.write64be = iomem_write64be,
 	.read8r = iomem_read8r,
 	.read16r = iomem_read16r,
 	.read32r = iomem_read32r,
@@ -304,6 +332,20 @@ unsigned int ioread32be(void __iomem *addr)
 	return *((u32 *)addr);
 }
 
+u64 ioread64(void __iomem *addr)
+{
+	if (unlikely(INDIRECT_ADDR(addr)))
+		return iomap_ops[ADDR_TO_REGION(addr)]->read64(addr);
+	return le64_to_cpup((u64 *)addr);
+}
+
+u64 ioread64be(void __iomem *addr)
+{
+	if (unlikely(INDIRECT_ADDR(addr)))
+		return iomap_ops[ADDR_TO_REGION(addr)]->read64be(addr);
+	return *((u64 *)addr);
+}
+
 void iowrite8(u8 datum, void __iomem *addr)
 {
 	if (unlikely(INDIRECT_ADDR(addr))) {
@@ -349,6 +391,24 @@ void iowrite32be(u32 datum, void __iomem *addr)
 	}
 }
 
+void iowrite64(u64 datum, void __iomem *addr)
+{
+	if (unlikely(INDIRECT_ADDR(addr))) {
+		iomap_ops[ADDR_TO_REGION(addr)]->write64(datum, addr);
+	} else {
+		*((u64 *)addr) = cpu_to_le64(datum);
+	}
+}
+
+void iowrite64be(u64 datum, void __iomem *addr)
+{
+	if (unlikely(INDIRECT_ADDR(addr))) {
+		iomap_ops[ADDR_TO_REGION(addr)]->write64be(datum, addr);
+	} else {
+		*((u64 *)addr) = datum;
+	}
+}
+
 /* Repeating interfaces */
 
 void ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
@@ -449,11 +509,15 @@ EXPORT_SYMBOL(ioread16);
 EXPORT_SYMBOL(ioread16be);
 EXPORT_SYMBOL(ioread32);
 EXPORT_SYMBOL(ioread32be);
+EXPORT_SYMBOL(ioread64);
+EXPORT_SYMBOL(ioread64be);
 EXPORT_SYMBOL(iowrite8);
 EXPORT_SYMBOL(iowrite16);
 EXPORT_SYMBOL(iowrite16be);
 EXPORT_SYMBOL(iowrite32);
 EXPORT_SYMBOL(iowrite32be);
+EXPORT_SYMBOL(iowrite64);
+EXPORT_SYMBOL(iowrite64be);
 EXPORT_SYMBOL(ioread8_rep);
 EXPORT_SYMBOL(ioread16_rep);
 EXPORT_SYMBOL(ioread32_rep);
-- 
2.11.0

[PATCH v18 3/7] iomap: introduce io{read|write}64_{lo_hi|hi_lo}

[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 <logang@deltatee.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Suresh Warrier <warrier@linux.vnet.ibm.com>
Cc: Nicholas Piggin <npiggin@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 e0331e754568..20fe5d7515db 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -798,8 +798,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 5b63b94ef6b5..5a4af0199b32 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -31,9 +31,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 *);
@@ -41,9 +48,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
+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 2c293b22569f..e909ab71e995 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -67,6 +67,7 @@ static void bad_io_access(unsigned long port, const char *access)
 #ifndef mmio_read16be
 #define mmio_read16be(addr) swab16(readw(addr))
 #define mmio_read32be(addr) swab32(readl(addr))
+#define mmio_read64be(addr) swab64(readq(addr))
 #endif
 
 unsigned int ioread8(void __iomem *addr)
@@ -100,6 +101,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 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)
@@ -108,6 +183,7 @@ EXPORT_SYMBOL(ioread32be);
 #ifndef mmio_write16be
 #define mmio_write16be(val,port) writew(swab16(val),port)
 #define mmio_write32be(val,port) writel(swab32(val),port)
+#define mmio_write64be(val,port) writeq(swab64(val),port)
 #endif
 
 void iowrite8(u8 val, void __iomem *addr)
@@ -136,6 +212,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

Re: [v18,3/7] iomap: introduce io{read|write}64_{lo_hi|hi_lo}

[Guenter Roeck]

Hi,

On Fri, Jun 22, 2018 at 01:47:48PM -0600, Logan Gunthorpe 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.
> 

This patch causes a build failure in -next when building
ppc:corenet64_smp_defconfig.

In file included from drivers/crypto/caam/qi.c:14:0:
drivers/crypto/caam/regs.h: In function 'wr_reg64':
drivers/crypto/caam/regs.h:143:3:
	error: implicit declaration of function 'iowrite64'; did you mean 'iowrite32'?

Several files are afected; this is only the first error reported.

Bisect log is attached.

Guenter

---
# bad: [483d835c8189f0566a4cbbe47e74ffa314430e98] Add linux-next specific files for 20180713
# good: [1e4b044d22517cae7047c99038abb444423243ca] Linux 4.18-rc4
git bisect start 'HEAD' 'v4.18-rc4'
# good: [a5da8bf5df4436f40f4e28f0be51a4678830fe23] Merge remote-tracking branch 'crypto/master'
git bisect good a5da8bf5df4436f40f4e28f0be51a4678830fe23
# good: [34f19afdf1f94450c325da94559d07f746e9043d] Merge remote-tracking branch 'tip/auto-latest'
git bisect good 34f19afdf1f94450c325da94559d07f746e9043d
# good: [7aac4471307ac631685599b820616fd4c47edf29] Merge remote-tracking branch 'staging/staging-next'
git bisect good 7aac4471307ac631685599b820616fd4c47edf29
# good: [6b674574d58a738035fdf29008a308bee9235a16] Merge remote-tracking branch 'ntb/ntb-next'
git bisect good 6b674574d58a738035fdf29008a308bee9235a16
# bad: [013d0c6f50169c75eb37549f705604103cbeaa0b] module: allow symbol exports to be disabled
git bisect bad 013d0c6f50169c75eb37549f705604103cbeaa0b
# good: [205a106bac127145a4defae7d0d35945001fe924] kernel/memremap, kasan: make ZONE_DEVICE with work with KASAN
git bisect good 205a106bac127145a4defae7d0d35945001fe924
# good: [89ce5b41242240e43de867b0469b71c6b8aa9d5f] mm, swap: fix race between swapoff and some swap operations
git bisect good 89ce5b41242240e43de867b0469b71c6b8aa9d5f
# good: [9760a955e6173bd0c2f58f1507fec8c88aaffc68] proc: fix BUILD_BUG_ON breakage on powerpc64-allyesconfig
git bisect good 9760a955e6173bd0c2f58f1507fec8c88aaffc68
# good: [696b14958b4d620ae7431351b667fd79397d550d] include/asm-generic/bug.h: clarify valid uses of WARN()
git bisect good 696b14958b4d620ae7431351b667fd79397d550d
# good: [e018a02ca17fbe4c34515bec58eceb8b0be94ec6] parisc: iomap: introduce io{read|write}64
git bisect good e018a02ca17fbe4c34515bec58eceb8b0be94ec6
# bad: [e18963b65c6397fdba51416ff70c33197f6c1a84] ntb: ntb_hw_intel: use io-64-nonatomic instead of in-driver hacks
git bisect bad e18963b65c6397fdba51416ff70c33197f6c1a84
# bad: [8f6432de3dbe1d1d222d2998380a495e04ba5fef] io-64-nonatomic: add io{read|write}64[be]{_lo_hi|_hi_lo} macros
git bisect bad 8f6432de3dbe1d1d222d2998380a495e04ba5fef
# bad: [d189c7f6b4a0dec224652af6d868eebb57553345] iomap: introduce io{read|write}64_{lo_hi|hi_lo}
git bisect bad d189c7f6b4a0dec224652af6d868eebb57553345
# first bad commit: [d189c7f6b4a0dec224652af6d868eebb57553345] iomap: introduce io{read|write}64_{lo_hi|hi_lo}

Re: [v18,3/7] iomap: introduce io{read|write}64_{lo_hi|hi_lo}

[Logan Gunthorpe]

On 13/07/18 05:38 PM, Guenter Roeck wrote:
> This patch causes a build failure in -next when building
> ppc:corenet64_smp_defconfig.

Thanks for the report. This 64bit IO stuff is a bit of a mess. It looks
like your corner case arch and config was never covered by the kbuild
robot which has run extensively on this patchset.

Anyway, I think the fix is to put back the ioread64/write64 prototypes
(even though they are only used in this one corner).

Can you try it with the following patch to ensure it fixes things for you?

Logan

--

diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 5a4af0199b32..9f11a79e0d7a 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -32,6 +32,11 @@ 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 *);
+#endif
+
 #ifdef readq
 #define ioread64_lo_hi ioread64_lo_hi
 #define ioread64_hi_lo ioread64_hi_lo
@@ -49,6 +54,11 @@ 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 *);
+#endif
+
 #ifdef writeq
 #define iowrite64_lo_hi iowrite64_lo_hi
 #define iowrite64_hi_lo iowrite64_hi_lo

Re: [v18,3/7] iomap: introduce io{read|write}64_{lo_hi|hi_lo}

[Guenter Roeck]

On 07/13/2018 05:20 PM, Logan Gunthorpe wrote:
> 
> 
> On 13/07/18 05:38 PM, Guenter Roeck wrote:
>> This patch causes a build failure in -next when building
>> ppc:corenet64_smp_defconfig.
> 
> Thanks for the report. This 64bit IO stuff is a bit of a mess. It looks
> like your corner case arch and config was never covered by the kbuild
> robot which has run extensively on this patchset.
> 
> Anyway, I think the fix is to put back the ioread64/write64 prototypes
> (even though they are only used in this one corner).
> 
> Can you try it with the following patch to ensure it fixes things for you?
> 

It does.

Guenter

> Logan
> 
> --
> 
> diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
> index 5a4af0199b32..9f11a79e0d7a 100644
> --- a/include/asm-generic/iomap.h
> +++ b/include/asm-generic/iomap.h
> @@ -32,6 +32,11 @@ 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 *);
> +#endif
> +
>   #ifdef readq
>   #define ioread64_lo_hi ioread64_lo_hi
>   #define ioread64_hi_lo ioread64_hi_lo
> @@ -49,6 +54,11 @@ 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 *);
> +#endif
> +
>   #ifdef writeq
>   #define iowrite64_lo_hi iowrite64_lo_hi
>   #define iowrite64_hi_lo iowrite64_hi_lo
> 

[PATCH v18 4/7] io-64-nonatomic: add io{read|write}64[be]{_lo_hi|_hi_lo} macros

[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 <logang@deltatee.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
Cc: Greg Kroah-Hartman <gregkh@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 862d786a904f..ae21b72cce85 100644
--- a/include/linux/io-64-nonatomic-hi-lo.h
+++ b/include/linux/io-64-nonatomic-hi-lo.h
@@ -55,4 +55,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 d042e7bb5adb..faaa842dbdb9 100644
--- a/include/linux/io-64-nonatomic-lo-hi.h
+++ b/include/linux/io-64-nonatomic-lo-hi.h
@@ -55,4 +55,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
+#define iowrite64be iowrite64be_lo_hi
+#endif
+
 #endif	/* _LINUX_IO_64_NONATOMIC_LO_HI_H_ */
-- 
2.11.0

[PATCH v18 5/7] ntb: ntb_hw_intel: use io-64-nonatomic instead of in-driver hacks

[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 <logang@deltatee.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Dave Jiang <dave.jiang@intel.com>
Acked-by: Allen Hubbe <Allen.Hubbe@dell.com>
Acked-by: Jon Mason <jdmason@kudzu.us>
---
 drivers/ntb/hw/intel/ntb_hw_intel.h | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/ntb/hw/intel/ntb_hw_intel.h b/drivers/ntb/hw/intel/ntb_hw_intel.h
index c49ff8970ce3..e071e28bca3f 100644
--- a/drivers/ntb/hw/intel/ntb_hw_intel.h
+++ b/drivers/ntb/hw/intel/ntb_hw_intel.h
@@ -53,6 +53,7 @@
 
 #include <linux/ntb.h>
 #include <linux/pci.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
 
 /* PCI device IDs */
 #define PCI_DEVICE_ID_INTEL_NTB_B2B_JSF	0x3725
@@ -218,33 +219,4 @@ static inline int pdev_is_gen3(struct pci_dev *pdev)
 	return 0;
 }
 
-#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
-
 #endif
-- 
2.11.0

[PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

Clean up the extra ifdefs which defined the wr_reg64 and rd_reg64
functions in non-64bit cases in favour of the new common
io-64-nonatomic-lo-hi header.

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: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Dan Douglass <dan.douglass@nxp.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
 drivers/crypto/caam/regs.h | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 4fb91ba39c36..5826acd9194e 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -10,7 +10,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
@@ -136,10 +136,9 @@ 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)
 {
-	if (caam_little_end)
+	if (!caam_imx && caam_little_end)
 		iowrite64(data, reg);
 	else
 		iowrite64be(data, reg);
@@ -147,35 +146,12 @@ static inline void wr_reg64(void __iomem *reg, u64 data)
 
 static inline u64 rd_reg64(void __iomem *reg)
 {
-	if (caam_little_end)
+	if (!caam_imx && caam_little_end)
 		return ioread64(reg);
 	else
 		return ioread64be(reg);
 }
 
-#else /* CONFIG_64BIT */
-static inline void wr_reg64(void __iomem *reg, u64 data)
-{
-	if (!caam_imx && caam_little_end) {
-		wr_reg32((u32 __iomem *)(reg) + 1, data >> 32);
-		wr_reg32((u32 __iomem *)(reg), data);
-	} else {
-		wr_reg32((u32 __iomem *)(reg), data >> 32);
-		wr_reg32((u32 __iomem *)(reg) + 1, data);
-	}
-}
-
-static inline u64 rd_reg64(void __iomem *reg)
-{
-	if (!caam_imx && caam_little_end)
-		return ((u64)rd_reg32((u32 __iomem *)(reg) + 1) << 32 |
-			(u64)rd_reg32((u32 __iomem *)(reg)));
-
-	return ((u64)rd_reg32((u32 __iomem *)(reg)) << 32 |
-		(u64)rd_reg32((u32 __iomem *)(reg) + 1));
-}
-#endif /* CONFIG_64BIT  */
-
 static inline u64 cpu_to_caam_dma64(dma_addr_t value)
 {
 	if (caam_imx)
-- 
2.11.0

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

Hi Logan,

On Fri, Jun 22, 2018 at 4:47 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> Clean up the extra ifdefs which defined the wr_reg64 and rd_reg64
> functions in non-64bit cases in favour of the new common
> io-64-nonatomic-lo-hi header.
>
> 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: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Horia Geantă <horia.geanta@nxp.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Dan Douglass <dan.douglass@nxp.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>

This is now in linux-next as commit 46e4bf08f6388 and it breaks
booting imx6 (32-bit ARM):

[    1.872473] caam 2100000.caam: Entropy delay = 3200
[    1.938223] caam 2100000.caam: Instantiated RNG4 SH0
[    1.998983] caam 2100000.caam: Instantiated RNG4 SH1
[    2.004019] caam 2100000.caam: device ID = 0x0a16010000000000 (Era 4)
[    2.010484] caam 2100000.caam: job rings = 2, qi = 0
[    2.027389] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
[    2.028867] caam algorithms registered in /proc/crypto
[    2.035925] mmc1: queuing unknown CIS tuple 0x80 (4 bytes)
[    2.041187] caam_jr 2101000.jr0: job ring error: irqstate: 00000103
[    2.049878] Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
[    2.056591] Modules linked in:
[    2.059671] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.18.0-rc3-next-20180703 #484
[    2.067338] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[    2.073892] PC is at caam_jr_interrupt+0x120/0x12c
[    2.078702] LR is at vprintk_emit+0x228/0x43c
[    2.083069] pc : [<c075eb38>]    lr : [<c01815ec>]    psr: 60000193
[    2.089344] sp : c1001d80  ip : c1001be0  fp : c1001da4
[    2.094576] r10: c107ffe7  r9 : ec749e00  r8 : 0000012d
[    2.099810] r7 : c1001de0  r6 : c17ecc10  r5 : ec7a6010  r4 : 00000103
[    2.106346] r3 : ba36048e  r2 : ba36048e  r1 : 00000001  r0 : 00000037
[    2.112884] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Segment none
[    2.120116] Control: 10c5387d  Table: 1000404a  DAC: 00000051
[    2.125873] Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
[    2.131890] Stack: (0xc1001d80 to 0xc1002000)
[    2.136262] 1d80: ec5b5f00 ec749e64 00000000 c1001de0 0000012d
ec749e00 c1001ddc c1001da8
[    2.144454] 1da0: c0183584 c075ea24 c0176ea4 c0176850 c1001dfc
c1008908 ec749e64 ec749e00
[    2.152644] 1dc0: 00000000 00000001 ec008400 f4000100 c1001e04
c1001de0 c018364c c0183504
[    2.160834] 1de0: 00000000 ba36048e c0a87070 ec749e00 ec749e64
c1014ee4 c1001e24 c1001e08
[    2.169025] 1e00: c01836e0 c0183628 ec749e00 ec749e64 c1014ee4
00000000 c1001e44 c1001e28
[    2.177215] 1e20: c0187460 c01836ac c0f840a8 0000012d c1008a90
00000000 c1001e54 c1001e48
[    2.185406] 1e40: c018264c c01873b0 c1001e7c c1001e58 c0182cdc
c0182630 f400010c 000003eb
[    2.193597] 1e60: 000003ff 00000000 c1001eb8 c10280b0 c1001eb4
c1001e80 c047686c c0182c7c
[    2.201787] 1e80: ffffffff f4001100 00000000 c01091e8 20000013
ffffffff c1001eec 00000000
[    2.209977] 1ea0: c1000000 c1008908 c1001f14 c1001eb8 c0101a30
c0476814 00000001 00000001
[    2.218166] 1ec0: 00000000 c100bfc0 c1000000 c100892c 00000001
c100896c 00000000 c0f839b0
[    2.226356] 1ee0: c1008908 c1001f14 c1001ed8 c1001f08 c0174ddc
c01091e8 20000013 ffffffff
[    2.234545] 1f00: 00000051 00000000 c1001f24 c1001f18 c0a86e20
c01091cc c1001f64 c1001f28
[    2.242735] 1f20: c01587dc c0a86e04 00000000 00000000 c1008900
ba36048e c1008908 000000c3
[    2.250925] 1f40: 00000002 c1080480 c1008900 c1080480 c1008908
c0f66a4c c1001f74 c1001f68
[    2.259114] 1f60: c0158c88 c015862c c1001f9c c1001f78 c0a7e898
c0158c74 00000000 00000000
[    2.267304] 1f80: c0a7e78c c1001f90 c10804d8 ffffffff c1001ff4
c1001fa0 c0f00d68 c0a7e694
[    2.275493] 1fa0: ffffffff ffffffff 00000000 c0f00718 00000000
efffcb40 00000000 c0f66a4c
[    2.283683] 1fc0: ba32168e 00000000 00000000 c0f00330 00000051
10c0387d 0000113c 18000000
[    2.291873] 1fe0: 412fc09a 10c5387d 00000000 c1001ff8 00000000
c0f009e0 00000000 00000000

Any ideas on how to fix this issue?

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 6:31 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Logan,
>
> On Fri, Jun 22, 2018 at 4:47 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>> Clean up the extra ifdefs which defined the wr_reg64 and rd_reg64
>> functions in non-64bit cases in favour of the new common
>> io-64-nonatomic-lo-hi header.
>>
>> 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: Logan Gunthorpe <logang@deltatee.com>
>> Reviewed-by: Horia Geantă <horia.geanta@nxp.com>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Cc: Dan Douglass <dan.douglass@nxp.com>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: "David S. Miller" <davem@davemloft.net>
>
> This is now in linux-next as commit 46e4bf08f6388 and it breaks
> booting imx6 (32-bit ARM):


> Any ideas on how to fix this issue?

Oops, first of all the header should be hi-lo instead of lo-hi.
Does it fix it?

Otherwise I didn't (briefly) see what can be the issue.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

Hi Andy,

On Tue, Jul 3, 2018 at 2:36 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> Oops, first of all the header should be hi-lo instead of lo-hi.
> Does it fix it?

I tried as you suggested:

--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -10,7 +10,7 @@

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

 /*
  * Architecture-specific register access method

but it still crashes the kernel.

Thanks

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 03/07/18 11:36 AM, Andy Shevchenko wrote:
>> This is now in linux-next as commit 46e4bf08f6388 and it breaks
>> booting imx6 (32-bit ARM):
> 
> 
>> Any ideas on how to fix this issue?
> 
> Oops, first of all the header should be hi-lo instead of lo-hi.
> Does it fix it?
> 
> Otherwise I didn't (briefly) see what can be the issue.

I had to dig to find this: but Horia had said[1] that the HW was
specified to use lo-hi and I think we ended up changing to that for this
commit based on his feedback. This is also mentioned in the commit message.

Besides that, it looks like we are hitting an undefined instruction. So
my best guess is that we are somehow now calling a proper 64bit read
when we should be calling two 32-bit reads.

Fabio, can you send your configuration?

Thanks,

Logan

[1]
http://lkml.kernel.org/r/VI1PR0401MB259145C2DFDB5E4084EA5DFC98D20@VI1PR0401MB2591.eurprd04.prod.outlook.com

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

Hi Logan,

On Tue, Jul 3, 2018 at 3:01 PM, Logan Gunthorpe <logang@deltatee.com> wrote:

> Besides that, it looks like we are hitting an undefined instruction. So
> my best guess is that we are somehow now calling a proper 64bit read
> when we should be calling two 32-bit reads.

Yes, it seems that's the case.

> Fabio, can you send your configuration?

This is a imx6 board built with arch/arm/configs/imx_v6_v7_defconfig

Thanks

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 9:06 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Logan,
>
> On Tue, Jul 3, 2018 at 3:01 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>> Besides that, it looks like we are hitting an undefined instruction. So
>> my best guess is that we are somehow now calling a proper 64bit read
>> when we should be calling two 32-bit reads.
>
> Yes, it seems that's the case.
>
>> Fabio, can you send your configuration?
>
> This is a imx6 board built with arch/arm/configs/imx_v6_v7_defconfig

Btw, what is the environment did you use to build it?
and what is the environment / make variable you supply (like ARCH,
CROSS_COMPILE, etc)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

On Tue, Jul 3, 2018 at 3:09 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> Btw, what is the environment did you use to build it?
> and what is the environment / make variable you supply (like ARCH,
> CROSS_COMPILE, etc)?

I build it on a Dell laptop running Ubuntu 16.04 with the following
variables exported:

export ARCH=arm
export CROSS_COMPILE=/usr/bin/arm-linux-gnueabi-

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 03/07/18 12:11 PM, Fabio Estevam wrote:
> On Tue, Jul 3, 2018 at 3:09 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> 
>> Btw, what is the environment did you use to build it?
>> and what is the environment / make variable you supply (like ARCH,
>> CROSS_COMPILE, etc)?
> 
> I build it on a Dell laptop running Ubuntu 16.04 with the following
> variables exported:
> 
> export ARCH=arm
> export CROSS_COMPILE=/usr/bin/arm-linux-gnueabi-

Ok, I'm at a bit of a loss... When I look at the assembly before and
after the patch, it looks pretty much the same. Additionally, the
function where the undefined exception occurs (caam_jr_interrupt())
doesn't make any use of wr_reg64 or rd_reg64 which is the only thing the
patch changes...

Also, it looks like the nonatomic headers are doing what they are
supposed to on this arch and generating two 32-bit ios and not a 64 bit one.

So I have no idea what's going on here... Are we sure this is the patch
causing the problem? Did you bisect?

Thanks,

Logan

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

On Tue, Jul 3, 2018 at 3:47 PM, Logan Gunthorpe <logang@deltatee.com> wrote:

> Ok, I'm at a bit of a loss... When I look at the assembly before and
> after the patch, it looks pretty much the same. Additionally, the
> function where the undefined exception occurs (caam_jr_interrupt())
> doesn't make any use of wr_reg64 or rd_reg64 which is the only thing the
> patch changes...
>
> Also, it looks like the nonatomic headers are doing what they are
> supposed to on this arch and generating two 32-bit ios and not a 64 bit one.
>
> So I have no idea what's going on here... Are we sure this is the patch
> causing the problem? Did you bisect?

Yes, I am sure that 46e4bf08f6388ba748 is the one causing the kernel boot issue.

If I revert 46e4bf08f6388ba748 in linux-next then I can boot mx6 just fine.

kernelci.org reports also confirm the same:

https://storage.kernelci.org/next/master/next-20180629/arm/imx_v6_v7_defconfig/lab-baylibre-seattle/boot-imx6q-wandboard.html

Thanks

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 10:40 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Jul 3, 2018 at 3:47 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>> Ok, I'm at a bit of a loss... When I look at the assembly before and
>> after the patch, it looks pretty much the same. Additionally, the
>> function where the undefined exception occurs (caam_jr_interrupt())
>> doesn't make any use of wr_reg64 or rd_reg64 which is the only thing the
>> patch changes...
>>
>> Also, it looks like the nonatomic headers are doing what they are
>> supposed to on this arch and generating two 32-bit ios and not a 64 bit one.
>>
>> So I have no idea what's going on here... Are we sure this is the patch
>> causing the problem? Did you bisect?
>
> Yes, I am sure that 46e4bf08f6388ba748 is the one causing the kernel boot issue.
>
> If I revert 46e4bf08f6388ba748 in linux-next then I can boot mx6 just fine.
>

Interesting...

> kernelci.org reports also confirm the same:
>
> https://storage.kernelci.org/next/master/next-20180629/arm/imx_v6_v7_defconfig/lab-baylibre-seattle/boot-imx6q-wandboard.html

By the way, is there any URL which contains dmesg out of kernel with
this commit reverted?
(It would be even better if we would have 'ignore_loglevel ' there
when CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG=y)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

On Tue, Jul 3, 2018 at 4:58 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> By the way, is there any URL which contains dmesg out of kernel with
> this commit reverted?

Yes, here is the linux-next 20180626 dmesg (which is the last
linux-next that does not contain 46e4bf08f63 and it boots fine) :
https://storage.kernelci.org/next/master/next-20180626/arm/imx_v6_v7_defconfig/lab-baylibre-seattle/boot-imx6q-wandboard.html

Thanks

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 11:10 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Jul 3, 2018 at 4:58 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
>> By the way, is there any URL which contains dmesg out of kernel with
>> this commit reverted?
>
> Yes, here is the linux-next 20180626 dmesg (which is the last
> linux-next that does not contain 46e4bf08f63 and it boots fine) :
> https://storage.kernelci.org/next/master/next-20180626/arm/imx_v6_v7_defconfig/lab-baylibre-seattle/boot-imx6q-wandboard.html

Thanks.

And here just a wild guess, if you comment out a BUG() in IRQ handler,
does it boot?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 11:40 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jul 3, 2018 at 11:10 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> On Tue, Jul 3, 2018 at 4:58 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>
>>> By the way, is there any URL which contains dmesg out of kernel with
>>> this commit reverted?
>>
>> Yes, here is the linux-next 20180626 dmesg (which is the last
>> linux-next that does not contain 46e4bf08f63 and it boots fine) :
>> https://storage.kernelci.org/next/master/next-20180626/arm/imx_v6_v7_defconfig/lab-baylibre-seattle/boot-imx6q-wandboard.html
>
> Thanks.
>
> And here just a wild guess, if you comment out a BUG() in IRQ handler,
> does it boot?

One more thing, on the working version can you print the value of IRQ
status register?

Something like

pr_info_ratelimited("%s %x\n", __func__, irqstatus);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

On Tue, Jul 3, 2018 at 5:40 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> And here just a wild guess, if you comment out a BUG() in IRQ handler,
> does it boot?

Not really. It fails a bit later:

[    3.863231] caam 2100000.caam: Entropy delay = 3200
[    3.929028] caam 2100000.caam: Instantiated RNG4 SH0
[    3.989789] caam 2100000.caam: Instantiated RNG4 SH1
[    3.994809] caam 2100000.caam: device ID = 0x0a16010000000000 (Era 4)
[    4.001313] caam 2100000.caam: job rings = 2, qi = 0
[    4.019569]  mmcblk1: p1 p2
[    4.021007] caam algorithms registered in /proc/crypto
[    4.030633] caam_jr 2101000.jr0: job ring error: irqstate: 00000103
[    4.037036] Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
[    4.043751] Modules linked in:
[    4.046829] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.18.0-rc3-next-20180703-00001-g62a05d4-dirty #498
[    4.056317] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[    4.062869] PC is at caam_jr_dequeue+0x240/0x2bc
[    4.067499] LR is at 0x1
[    4.070042] pc : [<c075e6bc>]    lr : [<00000001>]    psr: 60000113
[    4.076317] sp : c1001d58  ip : ec85c000  fp : c1001da4
[    4.081549] r10: ec858010  r9 : ec85c000  r8 : 00000000
[    4.086783] r7 : 000001ff  r6 : 00000001  r5 : 00000000  r4 : 00000000
[    4.093318] r3 : ffffffff  r2 : 00000000  r1 : 00000000  r0 : 00000001
[    4.099856] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    4.106999] Control: 10c5387d  Table: 1000404a  DAC: 00000051
[    4.112754] Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
[    4.118771] Stack: (0xc1001d58 to 0xc1002000)

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 7/3/2018 2:10 PM, Fabio Estevam wrote:
> On Tue, Jul 3, 2018 at 4:58 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> 
>> By the way, is there any URL which contains dmesg out of kernel with
>> this commit reverted?
> 
> Yes, here is the linux-next 20180626 dmesg (which is the last
> linux-next that does not contain 46e4bf08f63 and it boots fine) :
> https://storage.kernelci.org/next/master/next-20180626/arm/imx_v6_v7_defconfig/lab-baylibre-seattle/boot-imx6q-wandboard.html

Is this a virtual machine? And if so, any chance we can get an image and
qemu command line to run it ourselves?

Thanks,

Logan

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

On Tue, Jul 3, 2018 at 6:16 PM, Logan Gunthorpe <logang@deltatee.com> wrote:

> Is this a virtual machine? And if so, any chance we can get an image and
> qemu command line to run it ourselves?

This is a real imx6 development board. I have never tried it on qemu.
Let me see if I can reproduce it on quemu.

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 7/3/2018 1:40 PM, Fabio Estevam wrote:
>> So I have no idea what's going on here... Are we sure this is the patch
>> causing the problem? Did you bisect?
> 
> Yes, I am sure that 46e4bf08f6388ba748 is the one causing the kernel boot issue.

Also, it could be helpful if you can compile with CONFIG_DEBUG_INFO and
use gdb to find out what line "caam_jr_interrupt+0x120" points to in
your image.

When I do it on my setup, it doesn't point to a sensible line possibly
due to different compilers.

Thanks,

Logan

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Wed, Jul 4, 2018 at 12:20 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 7/3/2018 1:40 PM, Fabio Estevam wrote:

> Also, it could be helpful if you can compile with CONFIG_DEBUG_INFO and
> use gdb to find out what line "caam_jr_interrupt+0x120" points to in
> your image.

It is an explicit call to BUG().
That's why we see wrong instruction trap.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 03/07/18 04:21 PM, Andy Shevchenko wrote:
> On Wed, Jul 4, 2018 at 12:20 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>> On 7/3/2018 1:40 PM, Fabio Estevam wrote:
> 
>> Also, it could be helpful if you can compile with CONFIG_DEBUG_INFO and
>> use gdb to find out what line "caam_jr_interrupt+0x120" points to in
>> your image.
> 
> It is an explicit call to BUG().
> That's why we see wrong instruction trap.

Oh. So some IO in some other place must have changed to trigger the BUG...

Logan

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

On Tue, Jul 3, 2018 at 8:20 PM, Logan Gunthorpe <logang@deltatee.com> wrote:

> Oh. So some IO in some other place must have changed to trigger the BUG...

Yes, there are two 64-bit write operations inside caam_jr_init().

If I use the old 64-bit write function then things works fine:

--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -384,6 +384,17 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
 }
 EXPORT_SYMBOL(caam_jr_enqueue);

+static inline void wr_reg64old(void __iomem *reg, u64 data)
+{
+       if (!caam_imx && caam_little_end) {
+               wr_reg32((u32 __iomem *)(reg) + 1, data >> 32);
+               wr_reg32((u32 __iomem *)(reg), data);
+       } else {
+               wr_reg32((u32 __iomem *)(reg), data >> 32);
+               wr_reg32((u32 __iomem *)(reg) + 1, data);
+ }
+}
+
 /*
  * Init JobR independent of platform property detection
  */
@@ -434,8 +445,8 @@ static int caam_jr_init(struct device *dev)
  jrp->head = 0;
  jrp->tail = 0;

- wr_reg64(&jrp->rregs->inpring_base, inpbusaddr);
- wr_reg64(&jrp->rregs->outring_base, outbusaddr);
+ wr_reg64old(&jrp->rregs->inpring_base, inpbusaddr);
+ wr_reg64old(&jrp->rregs->outring_base, outbusaddr);
  wr_reg32(&jrp->rregs->inpring_size, JOBR_DEPTH);
  wr_reg32(&jrp->rregs->outring_size, JOBR_DEPTH);

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 03/07/18 04:21 PM, Andy Shevchenko wrote:
> It is an explicit call to BUG().
> That's why we see wrong instruction trap.

Ok, I think I see the problem... the code is rather confusing:

Prior to the patch, IOs were BE depending on caam_little_end but if
caam_imx was set, then it wrote two LE writes with the high one first.
After the patch, it writes two BE writes with the high one first.

To confirm, can you try the patch below?

If this is the case, we can either revert the commit or fold in this
patch depending on what others think is clearer.

Thanks,

Logan

--

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 5826acd9194e..5f70c460da25 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -138,10 +138,14 @@ static inline void clrsetbits_32(void __iomem
*reg, u32 clear, u32 set)
  */
 static inline void wr_reg64(void __iomem *reg, u64 data)
 {
-       if (!caam_imx && caam_little_end)
+       if (caam_imx && caam_little_end) {
+               iowrite32(data >> 32, reg);
+               iowrite32(data, reg + sizeof(u32));
+       } else if (caam_little_end) {
                iowrite64(data, reg);
-       else
+       } else {
                iowrite64be(data, reg);
+       }
 }

 static inline u64 rd_reg64(void __iomem *reg)

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 03/07/18 05:57 PM, Logan Gunthorpe wrote:
> To confirm, can you try the patch below?

Actually, scratch that: try this patch as I forgot the read side on the
previous one.

Logan

--

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 5826acd9194e..796ff764dcbf 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -138,18 +138,26 @@ static inline void clrsetbits_32(void __iomem
*reg, u32 clear, u32 set)
  */
 static inline void wr_reg64(void __iomem *reg, u64 data)
 {
-       if (!caam_imx && caam_little_end)
+       if (caam_imx && caam_little_end) {
+               iowrite32(data >> 32, reg);
+               iowrite32(data, reg + sizeof(u32));
+       } else if (caam_little_end) {
                iowrite64(data, reg);
-       else
+       } else {
                iowrite64be(data, reg);
+       }
 }

 static inline u64 rd_reg64(void __iomem *reg)
 {
-       if (!caam_imx && caam_little_end)
+       if (caam_imx && caam_little_end) {
+               return ((u64)ioread32(reg) << 32) |
+                       (u64)ioread32(reg + sizeof(u32));
+       } else if (caam_little_end) {
                return ioread64(reg);
-       else
+       } else {
                return ioread64be(reg);
+       }
 }

 static inline u64 cpu_to_caam_dma64(dma_addr_t value)

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

On Tue, Jul 3, 2018 at 9:02 PM, Logan Gunthorpe <logang@deltatee.com> wrote:

> Actually, scratch that: try this patch as I forgot the read side on the
> previous one.

Yes, this fixes the boot regression, thanks!

You can add:

Tested-by: Fabio Estevam <fabio.estevam@nxp.com>

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Horia Geanta]

On 7/4/2018 2:58 AM, Logan Gunthorpe wrote:
> 
> 
> On 03/07/18 04:21 PM, Andy Shevchenko wrote:
>> It is an explicit call to BUG().
>> That's why we see wrong instruction trap.
> 
> Ok, I think I see the problem... the code is rather confusing:
> 
> Prior to the patch, IOs were BE depending on caam_little_end but if
> caam_imx was set, then it wrote two LE writes with the high one first.
> After the patch, it writes two BE writes with the high one first.
> 
I think there are two separate issues here:

1. Semantics of operations in io-64-nonatomic-lo-hi.h, io-64-nonatomic-hi-lo.h

Logan, you mentioned the following (which unfortunately I somehow missed):
https://lore.kernel.org/lkml/c3f2e061-5ed1-5c74-b955-3d2bfb0da074@deltatee.com
The lo_hi/hi_lo functions seem to always refer to the data being written
or read not to the address operated on.

OTOH, initial commit that added asm-generic/io-64-nonatomic-lo-hi.h and
asm-generic/io-64-nonatomic-hi-lo.h mentions:
797a796a13df6 ("asm-generic: architecture independent readq/writeq for 32bit
environment")
- <asm-generic/io-64-nonatomic-lo-hi.h> provides non-atomic readq/ writeq with
the order of lower address -> higher address
- <asm-generic/io-64-nonatomic-hi-lo.h> provides non-atomic readq/ writeq with
reversed order

I think we should keep the initial semantics when adding support for
io{read|write}64, i.e. "lo" and "hi" to refer to address and not to value.

Actually this is the semantics intended for the CAAM patch, see the note at the
end of the commit message that refers to addresses, not values:
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.


2. CAAM driver I/O accessors for i.MX case

CAAM block in some i.MX parts has broken endianness for 64b registers.
For e.g. for i.MX6S/SL/D/Q even though CAAM is little endian, BARs for I/O rings
have to be programmed as:
I/O Ring BAR+0: unused
I/O Ring BAR+4: IOVA (32-bit little endian)
when the proper layout (for a 64b register) would have been to program IOVA at
BAR+0.

This explains why I/O accessors in CAAM driver handle things differently in case
caam_imx=true.

Since this quirk cannot be accommodated in generic fashion, code dealing with
caam_imx has to stay.

Horia

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

Hi Horia,

On Wed, Jul 4, 2018 at 8:45 AM, Horia Geanta <horia.geanta@nxp.com> wrote:

> I think there are two separate issues here:
>
> 1. Semantics of operations in io-64-nonatomic-lo-hi.h, io-64-nonatomic-hi-lo.h
>
> Logan, you mentioned the following (which unfortunately I somehow missed):
> https://lore.kernel.org/lkml/c3f2e061-5ed1-5c74-b955-3d2bfb0da074@deltatee.com
> The lo_hi/hi_lo functions seem to always refer to the data being written
> or read not to the address operated on.
>
> OTOH, initial commit that added asm-generic/io-64-nonatomic-lo-hi.h and
> asm-generic/io-64-nonatomic-hi-lo.h mentions:
> 797a796a13df6 ("asm-generic: architecture independent readq/writeq for 32bit
> environment")
> - <asm-generic/io-64-nonatomic-lo-hi.h> provides non-atomic readq/ writeq with
> the order of lower address -> higher address
> - <asm-generic/io-64-nonatomic-hi-lo.h> provides non-atomic readq/ writeq with
> reversed order
>
> I think we should keep the initial semantics when adding support for
> io{read|write}64, i.e. "lo" and "hi" to refer to address and not to value.
>
> Actually this is the semantics intended for the CAAM patch, see the note at the
> end of the commit message that refers to addresses, not values:
> 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.
>
>
> 2. CAAM driver I/O accessors for i.MX case
>
> CAAM block in some i.MX parts has broken endianness for 64b registers.
> For e.g. for i.MX6S/SL/D/Q even though CAAM is little endian, BARs for I/O rings
> have to be programmed as:
> I/O Ring BAR+0: unused
> I/O Ring BAR+4: IOVA (32-bit little endian)
> when the proper layout (for a 64b register) would have been to program IOVA at
> BAR+0.
>
> This explains why I/O accessors in CAAM driver handle things differently in case
> caam_imx=true.
>
> Since this quirk cannot be accommodated in generic fashion, code dealing with
> caam_imx has to stay.

Should I sent a revert of patch 46e4bf08f6388 for the boot regression for now?

Then the two issues you pointed out could be fixed later.

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Horia Geanta]

On 7/4/2018 6:06 PM, Fabio Estevam wrote:
> Hi Horia,
> 
> On Wed, Jul 4, 2018 at 8:45 AM, Horia Geanta <horia.geanta@nxp.com> wrote:
> 
>> I think there are two separate issues here:
>>
>> 1. Semantics of operations in io-64-nonatomic-lo-hi.h, io-64-nonatomic-hi-lo.h
>>
>> Logan, you mentioned the following (which unfortunately I somehow missed):
>> https://lore.kernel.org/lkml/c3f2e061-5ed1-5c74-b955-3d2bfb0da074@deltatee.com
>> The lo_hi/hi_lo functions seem to always refer to the data being written
>> or read not to the address operated on.
>>
>> OTOH, initial commit that added asm-generic/io-64-nonatomic-lo-hi.h and
>> asm-generic/io-64-nonatomic-hi-lo.h mentions:
>> 797a796a13df6 ("asm-generic: architecture independent readq/writeq for 32bit
>> environment")
>> - <asm-generic/io-64-nonatomic-lo-hi.h> provides non-atomic readq/ writeq with
>> the order of lower address -> higher address
>> - <asm-generic/io-64-nonatomic-hi-lo.h> provides non-atomic readq/ writeq with
>> reversed order
>>
>> I think we should keep the initial semantics when adding support for
>> io{read|write}64, i.e. "lo" and "hi" to refer to address and not to value.
>>
>> Actually this is the semantics intended for the CAAM patch, see the note at the
>> end of the commit message that refers to addresses, not values:
>> 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.
>>
>>
>> 2. CAAM driver I/O accessors for i.MX case
>>
>> CAAM block in some i.MX parts has broken endianness for 64b registers.
>> For e.g. for i.MX6S/SL/D/Q even though CAAM is little endian, BARs for I/O rings
>> have to be programmed as:
>> I/O Ring BAR+0: unused
>> I/O Ring BAR+4: IOVA (32-bit little endian)
>> when the proper layout (for a 64b register) would have been to program IOVA at
>> BAR+0.
>>
>> This explains why I/O accessors in CAAM driver handle things differently in case
>> caam_imx=true.
>>
>> Since this quirk cannot be accommodated in generic fashion, code dealing with
>> caam_imx has to stay.
> 
> Should I sent a revert of patch 46e4bf08f6388 for the boot regression for now?
> 
> Then the two issues you pointed out could be fixed later.
> 
I guess it would be better this way.

Thanks,
Horia

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Fabio Estevam]

On Wed, Jul 4, 2018 at 12:59 PM, Horia Geanta <horia.geanta@nxp.com> wrote:

> I guess it would be better this way.

Sounds good. I will submit the revert patch then.

Thanks

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 7/4/2018 9:06 AM, Fabio Estevam wrote:
>> Logan, you mentioned the following (which unfortunately I somehow missed):
>> https://lore.kernel.org/lkml/c3f2e061-5ed1-5c74-b955-3d2bfb0da074@deltatee.com
>> The lo_hi/hi_lo functions seem to always refer to the data being written
>> or read not to the address operated on.

Oy, yes that was more than a year ago.

>> OTOH, initial commit that added asm-generic/io-64-nonatomic-lo-hi.h and
>> asm-generic/io-64-nonatomic-hi-lo.h mentions:
>> 797a796a13df6 ("asm-generic: architecture independent readq/writeq for 32bit
>> environment")
>> - <asm-generic/io-64-nonatomic-lo-hi.h> provides non-atomic readq/ writeq with
>> the order of lower address -> higher address
>> - <asm-generic/io-64-nonatomic-hi-lo.h> provides non-atomic readq/ writeq with
>> reversed order

Hmm, well in fairness that commit didn't add any BE operations so
lower/higher address is the same as lower/higher data being written.
hi-lo/lo-hi is a bit ambiguous in that sense and designing it to match
the semantics of the only user seemed to make sense at the time. I
didn't even check the rough comments in an old commit message which I
wouldn't really take as canonical. Also, it seems to me, most hardware
would expect you to write in order of the address (if it cares at all)
not in the order of the higher/lower data word. Though, I have no
explicit examples only a gut feeling.

>> I think we should keep the initial semantics when adding support for
>> io{read|write}64, i.e. "lo" and "hi" to refer to address and not to value.
>>
>> Actually this is the semantics intended for the CAAM patch, see the note at the
>> end of the commit message that refers to addresses, not values:
>> 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.
>>
>>
>> 2. CAAM driver I/O accessors for i.MX case
>> Since this quirk cannot be accommodated in generic fashion, code dealing with
>> caam_imx has to stay.

Yes, though IMO, I think the patch I sent earlier is clearer than the
current code with the nested ifs in different static functions. In any
case, I'll drop it and let you make any changes you see fit.

Logan

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Wed, Jul 4, 2018 at 8:01 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 7/4/2018 9:06 AM, Fabio Estevam wrote:

> Hmm, well in fairness that commit didn't add any BE operations so
> lower/higher address is the same as lower/higher data being written.
> hi-lo/lo-hi is a bit ambiguous in that sense and designing it to match
> the semantics of the only user seemed to make sense at the time. I
> didn't even check the rough comments in an old commit message which I
> wouldn't really take as canonical. Also, it seems to me, most hardware
> would expect you to write in order of the address (if it cares at all)
> not in the order of the higher/lower data word. Though, I have no
> explicit examples only a gut feeling.

We have an iDMA 32-bit hardware (see drivers/dma/dw/) which has an
extension 64-bit registers where only one of them has a specific bit
to "commit" the changes written to all of them. And by some very
unknown reason that bit is in lo part which automatically means we
must to write it last.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 7/4/2018 11:10 AM, Andy Shevchenko wrote:
> We have an iDMA 32-bit hardware (see drivers/dma/dw/) which has an
> extension 64-bit registers where only one of them has a specific bit
> to "commit" the changes written to all of them. And by some very
> unknown reason that bit is in lo part which automatically means we
> must to write it last.

And it supports both BE and LE? And in both cases it's the lo part?

Logan

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Wed, Jul 4, 2018 at 8:13 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 7/4/2018 11:10 AM, Andy Shevchenko wrote:
>> We have an iDMA 32-bit hardware (see drivers/dma/dw/) which has an
>> extension 64-bit registers where only one of them has a specific bit
>> to "commit" the changes written to all of them. And by some very
>> unknown reason that bit is in lo part which automatically means we
>> must to write it last.
>
> And it supports both BE and LE? And in both cases it's the lo part?

It's only LE for now.

P.S. If you more interested in code in kernel look for idma32_fifo_partition()
(While the bit is set in each of 32-bit part, it's actually present in
only one place)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 7/4/2018 11:16 AM, Andy Shevchenko wrote:
> On Wed, Jul 4, 2018 at 8:13 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>> On 7/4/2018 11:10 AM, Andy Shevchenko wrote:
>>> We have an iDMA 32-bit hardware (see drivers/dma/dw/) which has an
>>> extension 64-bit registers where only one of them has a specific bit
>>> to "commit" the changes written to all of them. And by some very
>>> unknown reason that bit is in lo part which automatically means we
>>> must to write it last.
>>
>> And it supports both BE and LE? And in both cases it's the lo part?
> 
> It's only LE for now.

So the main question is if they were to add BE support, would they leave
the trigger in the same address or swap it to the other address so that
it's always the LO part that triggers? Otherwise it's hard to say what
we really want for the BE variants of the non-atomic hi-lo operations.

In the end, the driver author is free to use specifically which ever
function is necessary in any given situation (lo-hi vs hi-lo) and they
can read the definitions and no one is using them yet. So either way is
probably just as valid and it's probably not really worth fussing too
much about.

Logan

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Wed, Jul 4, 2018 at 8:16 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Jul 4, 2018 at 8:13 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>> On 7/4/2018 11:10 AM, Andy Shevchenko wrote:
>>> We have an iDMA 32-bit hardware (see drivers/dma/dw/) which has an
>>> extension 64-bit registers where only one of them has a specific bit
>>> to "commit" the changes written to all of them. And by some very
>>> unknown reason that bit is in lo part which automatically means we
>>> must to write it last.
>>
>> And it supports both BE and LE? And in both cases it's the lo part?
>
> It's only LE for now.
>
> P.S. If you more interested in code in kernel look for idma32_fifo_partition()
> (While the bit is set in each of 32-bit part, it's actually present in
> only one place)

I think it doesn't contradict with what you are saying rather supports it.

I would expect to have lo-hi and hi-lo semantics done according to the
data flow, not to the address.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 7/4/2018 11:21 AM, Andy Shevchenko wrote:
> I think it doesn't contradict with what you are saying rather supports it.
> 
> I would expect to have lo-hi and hi-lo semantics done according to the
> data flow, not to the address.

Hmm, no, I expected the opposite...

Logan

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Wed, Jul 4, 2018 at 8:23 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 7/4/2018 11:21 AM, Andy Shevchenko wrote:
>> I think it doesn't contradict with what you are saying rather supports it.
>>
>> I would expect to have lo-hi and hi-lo semantics done according to the
>> data flow, not to the address.
>
> Hmm, no, I expected the opposite...

Ah, sorry, misread you. Then it means I'm for the what I said and
opposing address flow.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Wed, Jul 4, 2018 at 2:45 PM, Horia Geanta <horia.geanta@nxp.com> wrote:
> On 7/4/2018 2:58 AM, Logan Gunthorpe wrote:
>>
>>
>> On 03/07/18 04:21 PM, Andy Shevchenko wrote:
>>> It is an explicit call to BUG().
>>> That's why we see wrong instruction trap.
>>
>> Ok, I think I see the problem... the code is rather confusing:
>>
>> Prior to the patch, IOs were BE depending on caam_little_end but if
>> caam_imx was set, then it wrote two LE writes with the high one first.
>> After the patch, it writes two BE writes with the high one first.
>>
> I think there are two separate issues here:
>
> 1. Semantics of operations in io-64-nonatomic-lo-hi.h, io-64-nonatomic-hi-lo.h
>
> Logan, you mentioned the following (which unfortunately I somehow missed):
> https://lore.kernel.org/lkml/c3f2e061-5ed1-5c74-b955-3d2bfb0da074@deltatee.com
> The lo_hi/hi_lo functions seem to always refer to the data being written
> or read not to the address operated on.
>
> OTOH, initial commit that added asm-generic/io-64-nonatomic-lo-hi.h and
> asm-generic/io-64-nonatomic-hi-lo.h mentions:
> 797a796a13df6 ("asm-generic: architecture independent readq/writeq for 32bit
> environment")
> - <asm-generic/io-64-nonatomic-lo-hi.h> provides non-atomic readq/ writeq with
> the order of lower address -> higher address
> - <asm-generic/io-64-nonatomic-hi-lo.h> provides non-atomic readq/ writeq with
> reversed order
>
> I think we should keep the initial semantics when adding support for
> io{read|write}64, i.e. "lo" and "hi" to refer to address and not to value.

There is a ambiguity with the commit message.
Since it had implemented only LE IO accessors the address semantics is
the same as data flow.

As a driver writer intuitively I would expect data flow semantics.

It means it would be invariant to LE BE accessors, right?

lo-hi: LE (0x0 0x4) BE (0x4 0x0)
hi-lo: LE (0x4 0x0) BE (0x0 0x4)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 7/4/2018 11:32 AM, Andy Shevchenko wrote:
> It means it would be invariant to LE BE accessors, right?
> 
> lo-hi: LE (0x0 0x4) BE (0x4 0x0)
> hi-lo: LE (0x4 0x0) BE (0x0 0x4)


Ok, well, given that this is what I implemented originally and the
argument seems a little bike-sheddy. I vote we just leave the current
patch as is.

Logan

Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 9:01 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 03/07/18 11:36 AM, Andy Shevchenko wrote:
>>> This is now in linux-next as commit 46e4bf08f6388 and it breaks
>>> booting imx6 (32-bit ARM):

>>> Any ideas on how to fix this issue?
>>
>> Oops, first of all the header should be hi-lo instead of lo-hi.
>> Does it fix it?
>>
>> Otherwise I didn't (briefly) see what can be the issue.
>
> I had to dig to find this: but Horia had said[1] that the HW was
> specified to use lo-hi and I think we ended up changing to that for this
> commit based on his feedback. This is also mentioned in the commit message.

Apparently I missed that part of the discussion.

> Besides that, it looks like we are hitting an undefined instruction. So
> my best guess is that we are somehow now calling a proper 64bit read
> when we should be calling two 32-bit reads.

Which is a bit strange. For arm we have been using generic
definitions. Only very few architectures use own defined macros.

>
> Fabio, can you send your configuration?
>
> Thanks,
>
> Logan
>
> [1]
> http://lkml.kernel.org/r/VI1PR0401MB259145C2DFDB5E4084EA5DFC98D20@VI1PR0401MB2591.eurprd04.prod.outlook.com
>
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH v18 7/7] ntb: ntb_hw_switchtec: Cleanup 64bit IO defines to use the common header

[Logan Gunthorpe]

Clean up the ifdefs which conditionally defined the io{read|write}64
functions in favour of the new common io-64-nonatomic-lo-hi header.

Per a nit from Andy Shevchenko, the include list is also made
alphabetical.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jon Mason <jdmason@kudzu.us>
---
 drivers/ntb/hw/mscc/ntb_hw_switchtec.c | 36 ++++------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/drivers/ntb/hw/mscc/ntb_hw_switchtec.c b/drivers/ntb/hw/mscc/ntb_hw_switchtec.c
index f624ae27eabe..f403da24b833 100644
--- a/drivers/ntb/hw/mscc/ntb_hw_switchtec.c
+++ b/drivers/ntb/hw/mscc/ntb_hw_switchtec.c
@@ -13,12 +13,13 @@
  *
  */
 
-#include <linux/switchtec.h>
-#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/delay.h>
 #include <linux/kthread.h>
-#include <linux/interrupt.h>
+#include <linux/module.h>
 #include <linux/ntb.h>
+#include <linux/switchtec.h>
 
 MODULE_DESCRIPTION("Microsemi Switchtec(tm) NTB Driver");
 MODULE_VERSION("0.1");
@@ -35,35 +36,6 @@ module_param(use_lut_mws, bool, 0644);
 MODULE_PARM_DESC(use_lut_mws,
 		 "Enable the use of the LUT based memory windows");
 
-#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
-
 #define SWITCHTEC_NTB_MAGIC 0x45CC0001
 #define MAX_MWS     128
 
-- 
2.11.0