2017-06-22 16:49:51

by Logan Gunthorpe

[permalink] [raw]
Subject: [PATCH 0/7] cleanup issues with io{read|write}64

Hi,

Presently, the 64bit IO functions are not very usable in drivers because
they are not universally available in all architectures. This leads to
a bunch of hacks in the kernel to work around this. (See the last 3
patches in this series.) As part of my switchtec_ntb submission which
added another one of these warts, Greg asked me to look into fixing
it[1].

So this patchset attempts to solve this issue by filling in the missing
implementations in iomap.c and io.h. After that, the alpha architecture is
the only one I found that also needed a fix for this. Finally, this
patchset removes the hacks that have accumulated in the kernel,
thus far, for working around this.

This set is based off of v4.12-rc6.

Thanks,

Logan

[1] https://marc.info/?l=linux-kernel&m=149774601910663&w=2

Logan Gunthorpe (7):
drm/tilcdc: don't use volatile with iowrite64
iomap: implement ioread64 and iowrite64
asm-generic/io.h: make ioread64 and iowrite64 universally available
alpha: provide ioread64 and iowrite64 implementations
ntb: ntb_hw_intel: remove ioread64 and iowrite64 hacks
drm/tilcdc: clean up ifdef hacks around iowrite64
crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

arch/alpha/include/asm/io.h | 2 ++
arch/alpha/kernel/io.c | 18 +++++++++++
arch/powerpc/include/asm/io.h | 2 ++
drivers/crypto/caam/regs.h | 29 -----------------
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 8 +----
drivers/ntb/hw/intel/ntb_hw_intel.c | 30 -----------------
include/asm-generic/io.h | 54 ++++++++++++++++++++++++-------
include/asm-generic/iomap.h | 4 ---
lib/iomap.c | 62 ++++++++++++++++++++++++++++++++++++
9 files changed, 127 insertions(+), 82 deletions(-)

--
2.11.0


2017-06-22 16:49:26

by Logan Gunthorpe

[permalink] [raw]
Subject: [PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

Currently, ioread64 and iowrite64 are only available io CONFIG_64BIT=y
and CONFIG_GENERIC_IOMAP=n. Thus, seeing the functions are not
universally available, it makes them unusable for driver developers.
This leads to ugly hacks such as those at the top of

drivers/ntb/hw/intel/ntb_hw_intel.c

This patch adds fallback implementations for when CONFIG_64BIT and
CONFIG_GENERIC_IOMAP are not set. These functions use two io32 based
calls to complete the operation.

Note, we do not use the volatile keyword in these functions like the
others in the same file. It is necessary to avoid a compiler warning
on arm.

Signed-off-by: Logan Gunthorpe <[email protected]>
Cc: Arnd Bergmann <[email protected]>
---
include/asm-generic/io.h | 54 +++++++++++++++++++++++++++++++++++++-----------
1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ef015eb3403..817edaa3da78 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -585,15 +585,24 @@ static inline u32 ioread32(const volatile void __iomem *addr)
}
#endif

-#ifdef CONFIG_64BIT
#ifndef ioread64
#define ioread64 ioread64
-static inline u64 ioread64(const volatile void __iomem *addr)
+#ifdef readq
+static inline u64 ioread64(const void __iomem *addr)
{
return readq(addr);
}
+#else
+static inline u64 ioread64(const void __iomem *addr)
+{
+ u64 low, high;
+
+ low = ioread32(addr);
+ high = ioread32(addr + sizeof(u32));
+ return low | (high << 32);
+}
+#endif
#endif
-#endif /* CONFIG_64BIT */

#ifndef iowrite8
#define iowrite8 iowrite8
@@ -619,15 +628,21 @@ static inline void iowrite32(u32 value, volatile void __iomem *addr)
}
#endif

-#ifdef CONFIG_64BIT
#ifndef iowrite64
#define iowrite64 iowrite64
-static inline void iowrite64(u64 value, volatile void __iomem *addr)
+#ifdef writeq
+static inline void iowrite64(u64 value, void __iomem *addr)
{
writeq(value, addr);
}
+#else
+static inline void iowrite64(u64 value, void __iomem *addr)
+{
+ iowrite32(value, addr);
+ iowrite32(value >> 32, addr + sizeof(u32));
+}
+#endif
#endif
-#endif /* CONFIG_64BIT */

#ifndef ioread16be
#define ioread16be ioread16be
@@ -645,15 +660,24 @@ static inline u32 ioread32be(const volatile void __iomem *addr)
}
#endif

-#ifdef CONFIG_64BIT
#ifndef ioread64be
#define ioread64be ioread64be
-static inline u64 ioread64be(const volatile void __iomem *addr)
+#ifdef readq
+static inline u64 ioread64be(const void __iomem *addr)
{
return swab64(readq(addr));
}
+#else
+static inline u64 ioread64be(const void __iomem *addr)
+{
+ u64 low, high;
+
+ low = ioread32be(addr + sizeof(u32));
+ high = ioread32be(addr);
+ return low | (high << 32);
+}
+#endif
#endif
-#endif /* CONFIG_64BIT */

#ifndef iowrite16be
#define iowrite16be iowrite16be
@@ -671,15 +695,21 @@ static inline void iowrite32be(u32 value, volatile void __iomem *addr)
}
#endif

-#ifdef CONFIG_64BIT
#ifndef iowrite64be
#define iowrite64be iowrite64be
-static inline void iowrite64be(u64 value, volatile void __iomem *addr)
+#ifdef writeq
+static inline void iowrite64be(u64 value, void __iomem *addr)
{
writeq(swab64(value), addr);
}
+#else
+static inline void iowrite64be(u64 value, void __iomem *addr)
+{
+ iowrite32be(value >> 32, addr);
+ iowrite32be(value, addr + sizeof(u32));
+}
+#endif
#endif
-#endif /* CONFIG_64BIT */

#ifndef ioread8_rep
#define ioread8_rep ioread8_rep
--
2.11.0

2017-06-22 16:49:28

by Logan Gunthorpe

[permalink] [raw]
Subject: [PATCH 1/7] drm/tilcdc: don't use volatile with iowrite64

This is a prep patch for adding a universal iowrite64.

The patch is to prevent compiler warnings when we add iowrite64 that
would occur because there is an unnecessary volatile in this driver.

Signed-off-by: Logan Gunthorpe <[email protected]>
Cc: Jyri Sarha <[email protected]>
Cc: Tomi Valkeinen <[email protected]>
Cc: David Airlie <[email protected]>
---
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
index 9d528c0a67a4..e9ce725698a9 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
@@ -131,14 +131,14 @@ static inline void tilcdc_write(struct drm_device *dev, u32 reg, u32 data)
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;
+ void __iomem *addr = priv->mmio + reg;

#ifdef iowrite64
iowrite64(data, addr);
#else
__iowmb();
/* This compiles to strd (=64-bit write) on ARM7 */
- *(volatile u64 __force *)addr = __cpu_to_le64(data);
+ *(u64 __force *)addr = __cpu_to_le64(data);
#endif
}

--
2.11.0

2017-06-22 16:49:24

by Logan Gunthorpe

[permalink] [raw]
Subject: [PATCH 5/7] ntb: ntb_hw_intel: remove ioread64 and iowrite64 hacks

Now that ioread64 and iowrite64 are available generically we can
remove the hack at the top of ntb_hw_intel.c that patches them in
when they are not available.

Signed-off-by: Logan Gunthorpe <[email protected]>
Cc: Jon Mason <[email protected]>
Cc: Dave Jiang <[email protected]>
Cc: Allen Hubbe <[email protected]>
---
drivers/ntb/hw/intel/ntb_hw_intel.c | 30 ------------------------------
1 file changed, 30 deletions(-)

diff --git a/drivers/ntb/hw/intel/ntb_hw_intel.c b/drivers/ntb/hw/intel/ntb_hw_intel.c
index c00238491673..56221d540c2b 100644
--- a/drivers/ntb/hw/intel/ntb_hw_intel.c
+++ b/drivers/ntb/hw/intel/ntb_hw_intel.c
@@ -153,35 +153,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) {
@@ -3008,4 +2979,3 @@ static void __exit intel_ntb_pci_driver_exit(void)
debugfs_remove_recursive(debugfs_dir);
}
module_exit(intel_ntb_pci_driver_exit);
-
--
2.11.0

2017-06-22 16:50:10

by Logan Gunthorpe

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

Now that ioread64 and iowrite64 are always available we don't
need the ugly ifdefs to change their implementation when they
are not.

Signed-off-by: Logan Gunthorpe <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Dan Douglass <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>
---
drivers/crypto/caam/regs.h | 29 -----------------------------
1 file changed, 29 deletions(-)

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 84d2f838a063..26fc19dd0c39 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -134,7 +134,6 @@ 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)
@@ -151,34 +150,6 @@ static inline u64 rd_reg64(void __iomem *reg)
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);
- }
-}
-
-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)));
- else
-#endif
- return ((u64)rd_reg32((u32 __iomem *)(reg)) << 32 |
- (u64)rd_reg32((u32 __iomem *)(reg) + 1));
-}
-#endif /* CONFIG_64BIT */
-
#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
#ifdef CONFIG_SOC_IMX7D
#define cpu_to_caam_dma(value) \
--
2.11.0

2017-06-22 16:50:39

by Logan Gunthorpe

[permalink] [raw]
Subject: [PATCH 2/7] iomap: implement ioread64 and iowrite64

Currently, ioread64 and iowrite64 are not impleminted in the generic
iomap implementation. The prototypes are defined if CONFIG_64BIT is set
but there is no actual implementation.

Seeing the functions are not universally available, they are unusable
for driver developers. This leads to ugly hacks such as those at
the top of

drivers/ntb/hw/intel/ntb_hw_intel.c

This patch adds generic implementations for these functions. We add
the obvious version if readq/writeq are implemented and fall back
to using two io32 calls in cases that don't provide direct 64bit
accesses. Thus making the functions universally available to
configurations with CONFIG_GENERIC_IOMAP=y.

For any pio accesses, the 64bit operations remain unsupported and
simply call bad_io_access in cases readq would be called.

Signed-off-by: Logan Gunthorpe <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Suresh Warrier <[email protected]>
Cc: Nicholas Piggin <[email protected]>
---
arch/powerpc/include/asm/io.h | 2 ++
include/asm-generic/iomap.h | 4 ---
lib/iomap.c | 62 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 422f99cf9924..11a83667d2c3 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..43ec4ea9f6f9 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -30,20 +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 *);
-#endif

extern void iowrite8(u8, void __iomem *);
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 *);
-#endif

/*
* "string" versions of the above. Note that they
diff --git a/lib/iomap.c b/lib/iomap.c
index fc3dcb4b238e..e38e036cb52f 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)
@@ -93,11 +94,45 @@ unsigned int ioread32be(void __iomem *addr)
IO_COND(addr, return pio_read32be(port), return mmio_read32be(addr));
return 0xffffffff;
}
+
+#ifdef readq
+u64 ioread64(void __iomem *addr)
+{
+ IO_COND(addr, bad_io_access(port, "ioread64"), return readq(addr));
+ return 0xffffffffffffffffLL;
+}
+u64 ioread64be(void __iomem *addr)
+{
+ IO_COND(addr, bad_io_access(port, "ioread64be"),
+ return mmio_read64be(addr));
+ return 0xffffffffffffffffLL;
+}
+#else
+u64 ioread64(void __iomem *addr)
+{
+ u64 low, high;
+
+ low = ioread32(addr);
+ high = ioread32(addr + sizeof(u32));
+ return low | (high << 32);
+}
+u64 ioread64be(void __iomem *addr)
+{
+ u64 low, high;
+
+ low = ioread32be(addr + sizeof(u32));
+ high = ioread32be(addr);
+ return low | (high << 32);
+}
+#endif
+
EXPORT_SYMBOL(ioread8);
EXPORT_SYMBOL(ioread16);
EXPORT_SYMBOL(ioread16be);
EXPORT_SYMBOL(ioread32);
EXPORT_SYMBOL(ioread32be);
+EXPORT_SYMBOL(ioread64);
+EXPORT_SYMBOL(ioread64be);

#ifndef pio_write16be
#define pio_write16be(val,port) outw(swab16(val),port)
@@ -107,6 +142,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)
@@ -129,11 +165,37 @@ void iowrite32be(u32 val, void __iomem *addr)
{
IO_COND(addr, pio_write32be(val,port), mmio_write32be(val, addr));
}
+
+#ifdef writeq
+void iowrite64(u64 val, void __iomem *addr)
+{
+ IO_COND(addr, bad_io_access(port, "iowrite64"), writeq(val, addr));
+}
+void iowrite64be(u64 val, void __iomem *addr)
+{
+ IO_COND(addr, bad_io_access(port, "iowrite64be"),
+ mmio_write64be(val, addr));
+}
+#else
+void iowrite64(u64 val, void __iomem *addr)
+{
+ iowrite32(val, addr);
+ iowrite32(val >> 32, addr + sizeof(u32));
+}
+void iowrite64be(u64 val, void __iomem *addr)
+{
+ iowrite32be(val >> 32, addr);
+ iowrite32be(val, addr + sizeof(u32));
+}
+#endif
+
EXPORT_SYMBOL(iowrite8);
EXPORT_SYMBOL(iowrite16);
EXPORT_SYMBOL(iowrite16be);
EXPORT_SYMBOL(iowrite32);
EXPORT_SYMBOL(iowrite32be);
+EXPORT_SYMBOL(iowrite64);
+EXPORT_SYMBOL(iowrite64be);

/*
* These are the "repeat MMIO read/write" functions.
--
2.11.0

2017-06-22 16:51:01

by Logan Gunthorpe

[permalink] [raw]
Subject: [PATCH 6/7] drm/tilcdc: clean up ifdef hacks around iowrite64

Now that we can expect iowrite64 to always exist the hack is no longer
necessary so we just call iowrite64 directly.

Signed-off-by: Logan Gunthorpe <[email protected]>
Cc: Jyri Sarha <[email protected]>
Cc: Tomi Valkeinen <[email protected]>
Cc: David Airlie <[email protected]>
---
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 6 ------
1 file changed, 6 deletions(-)

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

-#ifdef iowrite64
iowrite64(data, addr);
-#else
- __iowmb();
- /* This compiles to strd (=64-bit write) on ARM7 */
- *(u64 __force *)addr = __cpu_to_le64(data);
-#endif
}

static inline u32 tilcdc_read(struct drm_device *dev, u32 reg)
--
2.11.0

2017-06-22 16:51:16

by Logan Gunthorpe

[permalink] [raw]
Subject: [PATCH 4/7] alpha: provide ioread64 and iowrite64 implementations

Alpha implements its own io operation and doesn't use the
common library. Thus to make ioread64 and iowrite64 globally
available we need to add implementations for alpha.

For this, we simply use calls that chain two 32-bit operations.
(mostly because I don't really understand the alpha architecture.)

Signed-off-by: Logan Gunthorpe <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
---
arch/alpha/include/asm/io.h | 2 ++
arch/alpha/kernel/io.c | 18 ++++++++++++++++++
2 files changed, 20 insertions(+)

diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
index ff4049155c84..15588092c062 100644
--- a/arch/alpha/include/asm/io.h
+++ b/arch/alpha/include/asm/io.h
@@ -493,8 +493,10 @@ extern inline void writeq(u64 b, volatile void __iomem *addr)

#define ioread16be(p) be16_to_cpu(ioread16(p))
#define ioread32be(p) be32_to_cpu(ioread32(p))
+#define ioread64be(p) be64_to_cpu(ioread64(p))
#define iowrite16be(v,p) iowrite16(cpu_to_be16(v), (p))
#define iowrite32be(v,p) iowrite32(cpu_to_be32(v), (p))
+#define iowrite64be(v,p) iowrite32(cpu_to_be64(v), (p))

#define inb_p inb
#define inw_p inw
diff --git a/arch/alpha/kernel/io.c b/arch/alpha/kernel/io.c
index 19c5875ab398..8c28026f7849 100644
--- a/arch/alpha/kernel/io.c
+++ b/arch/alpha/kernel/io.c
@@ -59,6 +59,24 @@ EXPORT_SYMBOL(iowrite8);
EXPORT_SYMBOL(iowrite16);
EXPORT_SYMBOL(iowrite32);

+u64 ioread64(void __iomem *addr)
+{
+ u64 low, high;
+
+ low = ioread32(addr);
+ high = ioread32(addr + sizeof(u32));
+ return low | (high << 32);
+}
+
+void iowrite64(u64 val, void __iomem *addr)
+{
+ iowrite32(val, addr);
+ iowrite32(val >> 32, addr + sizeof(u32));
+}
+
+EXPORT_SYMBOL(ioread64);
+EXPORT_SYMBOL(iowrite64);
+
u8 inb(unsigned long port)
{
return ioread8(ioport_map(port, 1));
--
2.11.0

2017-06-22 17:17:56

by Dave Jiang

[permalink] [raw]
Subject: RE: [PATCH 5/7] ntb: ntb_hw_intel: remove ioread64 and iowrite64 hacks



> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Logan Gunthorpe
> Sent: Thursday, June 22, 2017 9:48 AM
> To: [email protected]; [email protected]; [email protected]; [email protected]; linuxppc-
> [email protected]; [email protected]; [email protected]
> Cc: Arnd Bergmann <[email protected]>; Greg Kroah-Hartman <[email protected]>; Stephen Bates <[email protected]>;
> Logan Gunthorpe <[email protected]>; Jon Mason <[email protected]>; Jiang, Dave <[email protected]>; Allen Hubbe
> <[email protected]>
> Subject: [PATCH 5/7] ntb: ntb_hw_intel: remove ioread64 and iowrite64 hacks
>
> Now that ioread64 and iowrite64 are available generically we can
> remove the hack at the top of ntb_hw_intel.c that patches them in
> when they are not available.
>
> Signed-off-by: Logan Gunthorpe <[email protected]>
> Cc: Jon Mason <[email protected]>
> Cc: Dave Jiang <[email protected]>
> Cc: Allen Hubbe <[email protected]>

Thanks for doing this Logan.

Acked-by: Dave Jiang <[email protected]>

> ---
> drivers/ntb/hw/intel/ntb_hw_intel.c | 30 ------------------------------
> 1 file changed, 30 deletions(-)
>
> diff --git a/drivers/ntb/hw/intel/ntb_hw_intel.c b/drivers/ntb/hw/intel/ntb_hw_intel.c
> index c00238491673..56221d540c2b 100644
> --- a/drivers/ntb/hw/intel/ntb_hw_intel.c
> +++ b/drivers/ntb/hw/intel/ntb_hw_intel.c
> @@ -153,35 +153,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) {
> @@ -3008,4 +2979,3 @@ static void __exit intel_ntb_pci_driver_exit(void)
> debugfs_remove_recursive(debugfs_dir);
> }
> module_exit(intel_ntb_pci_driver_exit);
> -
> --
> 2.11.0
>
> --
> 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 [email protected].
> To post to this group, send email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20170622164817.25515-6-
> logang%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

2017-06-22 17:29:33

by Stephen Bates

[permalink] [raw]
Subject: Re: [PATCH 4/7] alpha: provide ioread64 and iowrite64 implementations

> +#define iowrite64be(v,p) iowrite32(cpu_to_be64(v), (p))

Logan, thanks for taking this cleanup on. I think this should be iowrite64 not iowrite32?

Stephen



2017-06-22 17:31:05

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 4/7] alpha: provide ioread64 and iowrite64 implementations

On 6/22/2017 11:29 AM, Stephen Bates wrote:
>> +#define iowrite64be(v,p) iowrite32(cpu_to_be64(v), (p))
>
> Logan, thanks for taking this cleanup on. I think this should be iowrite64 not iowrite32?

Yup, good catch. Thanks. I'll fix it in a v2 of this series.

Logan

2017-06-22 20:09:05

by Alan Cox

[permalink] [raw]
Subject: Re: [PATCH 4/7] alpha: provide ioread64 and iowrite64 implementations

On Thu, 22 Jun 2017 10:48:14 -0600
Logan Gunthorpe <[email protected]> wrote:

> Alpha implements its own io operation and doesn't use the
> common library. Thus to make ioread64 and iowrite64 globally
> available we need to add implementations for alpha.
>
> For this, we simply use calls that chain two 32-bit operations.
> (mostly because I don't really understand the alpha architecture.)

But this does not do the same thing as an ioread64 with regards to
atomicity or side effects on the device. The same is true of the other
hacks. You either have a real 64bit single read/write from MMIO space or
you don't. You can't fake it.


Alan

2017-06-22 20:10:15

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 4/7] alpha: provide ioread64 and iowrite64 implementations

On 6/22/2017 2:08 PM, Alan Cox wrote:
> But this does not do the same thing as an ioread64 with regards to
> atomicity or side effects on the device. The same is true of the other
> hacks. You either have a real 64bit single read/write from MMIO space or
> you don't. You can't fake it.

Yes, I know. But is it not better than having every driver that wants to
use these functions fake it themselves?

Logan

2017-06-22 20:14:52

by Alan Cox

[permalink] [raw]
Subject: Re: [PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

On Thu, 22 Jun 2017 10:48:13 -0600
Logan Gunthorpe <[email protected]> wrote:

> Currently, ioread64 and iowrite64 are only available io CONFIG_64BIT=y
> and CONFIG_GENERIC_IOMAP=n. Thus, seeing the functions are not
> universally available, it makes them unusable for driver developers.
> This leads to ugly hacks such as those at the top of
>
> drivers/ntb/hw/intel/ntb_hw_intel.c
>
> This patch adds fallback implementations for when CONFIG_64BIT and
> CONFIG_GENERIC_IOMAP are not set. These functions use two io32 based
> calls to complete the operation.
>
> Note, we do not use the volatile keyword in these functions like the
> others in the same file. It is necessary to avoid a compiler warning
> on arm.

This is a really really bad idea as per the Alpha comment.

ioread64 and iowrite64 generate a single 64bit bus transaction. There is
hardware where mmio operations have side effects so simply using a pair
of 32bit operations blindly does not work (consider something as trivial
as reading a 64bit performance counter or incrementing pointer).

If a platform doesn't support 64bit I/O operations from the CPU then you
either need to use some kind of platform/architecture specific interface
if present or accept you don't have one.

It's not safe to split it. Possibly for some use cases you could add an

ioread64_maysplit()

but you cannot blindly break ioread64/write64() and expect it to
magically allow you to use drivers that depend upon it.

What btw is the actual ARM compiler warning ? Is the compiler also trying
to tell you it's a bad idea ?

Alan

2017-06-22 20:25:14

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

On 6/22/2017 2:14 PM, Alan Cox wrote:
> If a platform doesn't support 64bit I/O operations from the CPU then you
> either need to use some kind of platform/architecture specific interface
> if present or accept you don't have one.

Yes, I understand that.

The thing is that every user that's currently using it right now is
patching in their own version that splits it on non-64bit systems.

> It's not safe to split it. Possibly for some use cases you could add an
> ioread64_maysplit()

I'm open to doing something like that.

> What btw is the actual ARM compiler warning ? Is the compiler also trying
> to tell you it's a bad idea ?

It's just the compiler noting that you are mixing volatile and
non-volatile pointers. Strangely some io{read|write}XX use volatile but
most do not. But it's nothing crazy.

Logan


2017-06-22 20:37:16

by Alan Cox

[permalink] [raw]
Subject: Re: [PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

On Thu, 22 Jun 2017 14:24:58 -0600
Logan Gunthorpe <[email protected]> wrote:

> On 6/22/2017 2:14 PM, Alan Cox wrote:
> > If a platform doesn't support 64bit I/O operations from the CPU then you
> > either need to use some kind of platform/architecture specific interface
> > if present or accept you don't have one.
>
> Yes, I understand that.
>
> The thing is that every user that's currently using it right now is
> patching in their own version that splits it on non-64bit systems.
>
> > It's not safe to split it. Possibly for some use cases you could add an
> > ioread64_maysplit()
>
> I'm open to doing something like that.

I think that makes sense for the platforms with that problem. I'm not
sure there are many that can't do it for mmio at least. 486SX can't do it
and I guess some ARM32 but I think almost everyone else can including
most 32bit x86.

What's more of a problem is a lot of platforms can do 64bit MMIO via
ioread/write64 but not 64bit port I/O, and it's not clear how you
represent that via an ioread/write API that abstracts it away.

Alan

2017-06-22 20:38:46

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

On 6/22/2017 2:36 PM, Alan Cox wrote:
> I think that makes sense for the platforms with that problem. I'm not
> sure there are many that can't do it for mmio at least. 486SX can't do it
> and I guess some ARM32 but I think almost everyone else can including
> most 32bit x86.
>
> What's more of a problem is a lot of platforms can do 64bit MMIO via
> ioread/write64 but not 64bit port I/O, and it's not clear how you
> represent that via an ioread/write API that abstracts it away.

In Patch 2, we call bad_io_access for anyone trying to do 64bit accesses
on port I/O.

Logan

2017-06-22 21:03:22

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 4/7] alpha: provide ioread64 and iowrite64 implementations

On Thu, Jun 22, 2017 at 10:09 PM, Logan Gunthorpe <[email protected]> wrote:
> On 6/22/2017 2:08 PM, Alan Cox wrote:
>>
>> But this does not do the same thing as an ioread64 with regards to
>> atomicity or side effects on the device. The same is true of the other
>> hacks. You either have a real 64bit single read/write from MMIO space or
>> you don't. You can't fake it.
>
>
> Yes, I know. But is it not better than having every driver that wants to use
> these functions fake it themselves?

Drivers that want a non-atomic variant should include either
include/linux/io-64-nonatomic-hi-lo.h or include/linux/io-64-nonatomic-lo-hi.h
depending on what they need. Drivers that require 64-bit I/O should
probably just depend on CONFIG_64BIT and maybe use readq/writeq.

I see that there are exactly two drivers calling ioread64/iowrite64:
drivers/crypto/caam/ is architecture specific and
drivers/ntb/hw/intel/ntb_hw_intel.c already has a workaround that should
make it build on alpha.

Arnd

2017-06-22 21:10:39

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 4/7] alpha: provide ioread64 and iowrite64 implementations

On 6/22/2017 3:03 PM, Arnd Bergmann wrote:
> Drivers that want a non-atomic variant should include either
> include/linux/io-64-nonatomic-hi-lo.h or include/linux/io-64-nonatomic-lo-hi.h
> depending on what they need. Drivers that require 64-bit I/O should
> probably just depend on CONFIG_64BIT and maybe use readq/writeq.

Ok, I will work something like that up.

We'll still need a patch similar to patch 2 (less the non-atomic
versions) seeing even CONFIG_GENERIC_IOMAP arches don't actually have a
working ioread64/iowrite64 implementation.

Thanks,

Logan

2017-06-22 21:20:54

by Richard Henderson

[permalink] [raw]
Subject: Re: [PATCH 4/7] alpha: provide ioread64 and iowrite64 implementations

On 06/22/2017 09:48 AM, Logan Gunthorpe wrote:
> Alpha implements its own io operation and doesn't use the
> common library. Thus to make ioread64 and iowrite64 globally
> available we need to add implementations for alpha.
>
> For this, we simply use calls that chain two 32-bit operations.
> (mostly because I don't really understand the alpha architecture.)

It's not difficult to provide this interface[*]. I believe the only reason I
didn't do so from the beginning is that it wasn't used.



r~


* At least for systems other than Jensen, which cannot generate 64-bit I/O. On
the other hand, Jensen doesn't have PCI (EISA only), and so won't have any
devices that care.

2017-06-23 06:51:22

by Horia Geanta

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

On 6/22/2017 7:49 PM, Logan Gunthorpe wrote:
> Now that ioread64 and iowrite64 are always available we don't
> need the ugly ifdefs to change their implementation when they
> are not.
>
Thanks Logan.

Note however this is not equivalent - it changes the behaviour, since
CAAM engine on i.MX6S/SL/D/Q platforms is broken in terms of 64-bit
register endianness - see CONFIG_CRYPTO_DEV_FSL_CAAM_IMX usage in code
you are removing.

[Yes, current code has its problems, as it does not differentiate b/w
i.MX platforms with and without the (unofficial) erratum, but this
should be fixed separately.]

Below is the change that would keep current logic - still forcing i.MX
to write CAAM 64-bit registers in BE even if the engine is LE (yes, diff
is doing a poor job).

Horia

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 84d2f838a063..b893ebb24e65 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -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


> Signed-off-by: Logan Gunthorpe <[email protected]>
> Cc: "Horia Geant?" <[email protected]>
> Cc: Dan Douglass <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> ---
> drivers/crypto/caam/regs.h | 29 -----------------------------
> 1 file changed, 29 deletions(-)
>
> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> index 84d2f838a063..26fc19dd0c39 100644
> --- a/drivers/crypto/caam/regs.h
> +++ b/drivers/crypto/caam/regs.h
> @@ -134,7 +134,6 @@ 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)
> @@ -151,34 +150,6 @@ static inline u64 rd_reg64(void __iomem *reg)
> 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);
> - }
> -}
> -
> -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)));
> - else
> -#endif
> - return ((u64)rd_reg32((u32 __iomem *)(reg)) << 32 |
> - (u64)rd_reg32((u32 __iomem *)(reg) + 1));
> -}
> -#endif /* CONFIG_64BIT */
> -
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> #ifdef CONFIG_SOC_IMX7D
> #define cpu_to_caam_dma(value) \
>

2017-06-23 17:59:24

by Logan Gunthorpe

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

Thanks Horia.

I'm inclined to just use your patch verbatim. I can set you as author,
but no matter how I do it, I'll need your Signed-off-by.

Logan

On 23/06/17 12:51 AM, Horia Geant? wrote:
> On 6/22/2017 7:49 PM, Logan Gunthorpe wrote:
>> Now that ioread64 and iowrite64 are always available we don't
>> need the ugly ifdefs to change their implementation when they
>> are not.
>>
> Thanks Logan.
>
> Note however this is not equivalent - it changes the behaviour, since
> CAAM engine on i.MX6S/SL/D/Q platforms is broken in terms of 64-bit
> register endianness - see CONFIG_CRYPTO_DEV_FSL_CAAM_IMX usage in code
> you are removing.
>
> [Yes, current code has its problems, as it does not differentiate b/w
> i.MX platforms with and without the (unofficial) erratum, but this
> should be fixed separately.]
>
> Below is the change that would keep current logic - still forcing i.MX
> to write CAAM 64-bit registers in BE even if the engine is LE (yes, diff
> is doing a poor job).
>
> Horia
>
> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> index 84d2f838a063..b893ebb24e65 100644
> --- a/drivers/crypto/caam/regs.h
> +++ b/drivers/crypto/caam/regs.h
> @@ -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
>
>
>> Signed-off-by: Logan Gunthorpe <[email protected]>
>> Cc: "Horia Geant?" <[email protected]>
>> Cc: Dan Douglass <[email protected]>
>> Cc: Herbert Xu <[email protected]>
>> Cc: "David S. Miller" <[email protected]>
>> ---
>> drivers/crypto/caam/regs.h | 29 -----------------------------
>> 1 file changed, 29 deletions(-)
>>
>> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
>> index 84d2f838a063..26fc19dd0c39 100644
>> --- a/drivers/crypto/caam/regs.h
>> +++ b/drivers/crypto/caam/regs.h
>> @@ -134,7 +134,6 @@ 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)
>> @@ -151,34 +150,6 @@ static inline u64 rd_reg64(void __iomem *reg)
>> 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);
>> - }
>> -}
>> -
>> -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)));
>> - else
>> -#endif
>> - return ((u64)rd_reg32((u32 __iomem *)(reg)) << 32 |
>> - (u64)rd_reg32((u32 __iomem *)(reg) + 1));
>> -}
>> -#endif /* CONFIG_64BIT */
>> -
>> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>> #ifdef CONFIG_SOC_IMX7D
>> #define cpu_to_caam_dma(value) \
>>

2017-06-24 11:57:53

by Horia Geanta

[permalink] [raw]
Subject: [PATCH v2 7/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

Now that ioread64 and iowrite64 are always available we don't
need the ugly ifdefs to change their implementation when they
are not.

Signed-off-by: Logan Gunthorpe <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Dan Douglass <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>

Updated patch such that behaviour does not change
from i.MX workaround point of view.

Signed-off-by: Horia Geantă <[email protected]>
---
drivers/crypto/caam/regs.h | 33 ++++-----------------------------
1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 84d2f838a063..b893ebb24e65 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -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.12.0.264.gd6db3f216544

2017-06-24 15:13:13

by Richard Henderson

[permalink] [raw]
Subject: [PATCH] alpha: provide ioread64 and iowrite64 implementations

All Alpha hosts except for Jensen provide 64-bit I/O operations.

Jensen is EISA only, so there ought not be any devices that even
attempt such operations. But just in case, use 2 32-bit operations.

Cc: Logan Gunthorpe <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
Signed-off-by: Richard Henderson <[email protected]>
---
arch/alpha/include/asm/core_apecs.h | 16 ++++++++++++++++
arch/alpha/include/asm/core_cia.h | 16 ++++++++++++++++
arch/alpha/include/asm/core_lca.h | 16 ++++++++++++++++
arch/alpha/include/asm/core_mcpcia.h | 21 +++++++++++++++++++++
arch/alpha/include/asm/core_t2.h | 24 +++++++++++++++++++-----
arch/alpha/include/asm/io.h | 27 +++++++++++++++++++++++++++
arch/alpha/include/asm/io_trivial.h | 12 ++++++++++++
arch/alpha/include/asm/jensen.h | 32 +++++++++++++++++++++++++++-----
arch/alpha/include/asm/machvec.h | 2 ++
arch/alpha/kernel/io.c | 27 +++++++++++++++++++++++++++
arch/alpha/kernel/machvec_impl.h | 2 ++
11 files changed, 185 insertions(+), 10 deletions(-)

diff --git a/arch/alpha/include/asm/core_apecs.h b/arch/alpha/include/asm/core_apecs.h
index 6785ff7..7e2ff13 100644
--- a/arch/alpha/include/asm/core_apecs.h
+++ b/arch/alpha/include/asm/core_apecs.h
@@ -471,6 +471,22 @@ __EXTERN_INLINE void apecs_iowrite32(u32 b, void __iomem *xaddr)
*(vuip)addr = b;
}

+__EXTERN_INLINE u64 apecs_ioread64(void __iomem *xaddr)
+{
+ unsigned long addr = (unsigned long) xaddr;
+ if (addr < APECS_DENSE_MEM)
+ addr = ((addr - APECS_IO) << 5) + APECS_IO + 0x78;
+ return *(vulp)addr;
+}
+
+__EXTERN_INLINE void apecs_iowrite64(u64 b, void __iomem *xaddr)
+{
+ unsigned long addr = (unsigned long) xaddr;
+ if (addr < APECS_DENSE_MEM)
+ addr = ((addr - APECS_IO) << 5) + APECS_IO + 0x78;
+ *(vulp)addr = b;
+}
+
__EXTERN_INLINE void __iomem *apecs_ioportmap(unsigned long addr)
{
return (void __iomem *)(addr + APECS_IO);
diff --git a/arch/alpha/include/asm/core_cia.h b/arch/alpha/include/asm/core_cia.h
index 9e0516c..54792b3 100644
--- a/arch/alpha/include/asm/core_cia.h
+++ b/arch/alpha/include/asm/core_cia.h
@@ -419,6 +419,22 @@ __EXTERN_INLINE void cia_iowrite32(u32 b, void __iomem *xaddr)
*(vuip)addr = b;
}

+__EXTERN_INLINE u64 cia_ioread64(void __iomem *xaddr)
+{
+ unsigned long addr = (unsigned long) xaddr;
+ if (addr < CIA_DENSE_MEM)
+ addr = ((addr - CIA_IO) << 5) + CIA_IO + 0x78;
+ return *(vulp)addr;
+}
+
+__EXTERN_INLINE void cia_iowrite64(u64 b, void __iomem *xaddr)
+{
+ unsigned long addr = (unsigned long) xaddr;
+ if (addr < CIA_DENSE_MEM)
+ addr = ((addr - CIA_IO) << 5) + CIA_IO + 0x78;
+ *(vulp)addr = b;
+}
+
__EXTERN_INLINE void __iomem *cia_ioportmap(unsigned long addr)
{
return (void __iomem *)(addr + CIA_IO);
diff --git a/arch/alpha/include/asm/core_lca.h b/arch/alpha/include/asm/core_lca.h
index 8ee6c51..b8e43cd 100644
--- a/arch/alpha/include/asm/core_lca.h
+++ b/arch/alpha/include/asm/core_lca.h
@@ -317,6 +317,22 @@ __EXTERN_INLINE void lca_iowrite32(u32 b, void __iomem *xaddr)
*(vuip)addr = b;
}

+__EXTERN_INLINE u64 lca_ioread64(void __iomem *xaddr)
+{
+ unsigned long addr = (unsigned long) xaddr;
+ if (addr < LCA_DENSE_MEM)
+ addr = ((addr - LCA_IO) << 5) + LCA_IO + 0x78;
+ return *(vulp)addr;
+}
+
+__EXTERN_INLINE void lca_iowrite64(u64 b, void __iomem *xaddr)
+{
+ unsigned long addr = (unsigned long) xaddr;
+ if (addr < LCA_DENSE_MEM)
+ addr = ((addr - LCA_IO) << 5) + LCA_IO + 0x78;
+ *(vulp)addr = b;
+}
+
__EXTERN_INLINE void __iomem *lca_ioportmap(unsigned long addr)
{
return (void __iomem *)(addr + LCA_IO);
diff --git a/arch/alpha/include/asm/core_mcpcia.h b/arch/alpha/include/asm/core_mcpcia.h
index ad44bef..e4d9139 100644
--- a/arch/alpha/include/asm/core_mcpcia.h
+++ b/arch/alpha/include/asm/core_mcpcia.h
@@ -247,6 +247,7 @@ struct el_MCPCIA_uncorrected_frame_mcheck {

#define vip volatile int __force *
#define vuip volatile unsigned int __force *
+#define vulp volatile unsigned long __force *

#ifndef MCPCIA_ONE_HAE_WINDOW
#define MCPCIA_FROB_MMIO \
@@ -334,6 +335,25 @@ __EXTERN_INLINE void mcpcia_iowrite32(u32 b, void __iomem *xaddr)
*(vuip)addr = b;
}

+__EXTERN_INLINE u64 mcpcia_ioread64(void __iomem *xaddr)
+{
+ unsigned long addr = (unsigned long)xaddr;
+
+ if (!__mcpcia_is_mmio(addr))
+ addr = ((addr & 0xffff) << 5) + (addr & ~0xfffful) + 0x78;
+
+ return *(vulp)addr;
+}
+
+__EXTERN_INLINE void mcpcia_iowrite64(u64 b, void __iomem *xaddr)
+{
+ unsigned long addr = (unsigned long)xaddr;
+
+ if (!__mcpcia_is_mmio(addr))
+ addr = ((addr & 0xffff) << 5) + (addr & ~0xfffful) + 0x78;
+
+ *(vulp)addr = b;
+}

__EXTERN_INLINE void __iomem *mcpcia_ioportmap(unsigned long addr)
{
@@ -361,6 +381,7 @@ __EXTERN_INLINE int mcpcia_is_mmio(const volatile void __iomem *xaddr)

#undef vip
#undef vuip
+#undef vulp

#undef __IO_PREFIX
#define __IO_PREFIX mcpcia
diff --git a/arch/alpha/include/asm/core_t2.h b/arch/alpha/include/asm/core_t2.h
index ade9d92..cb3467d 100644
--- a/arch/alpha/include/asm/core_t2.h
+++ b/arch/alpha/include/asm/core_t2.h
@@ -359,6 +359,7 @@ struct el_t2_frame_corrected {

#define vip volatile int *
#define vuip volatile unsigned int *
+#define vulp volatile unsigned long *

extern inline u8 t2_inb(unsigned long addr)
{
@@ -401,6 +402,17 @@ extern inline void t2_outl(u32 b, unsigned long addr)
mb();
}

+extern inline u64 t2_inq(unsigned long addr)
+{
+ return *(vulp) ((addr << 5) + T2_IO + 0x78);
+}
+
+extern inline void t2_outq(u64 b, unsigned long addr)
+{
+ *(vulp) ((addr << 5) + T2_IO + 0x78) = b;
+ mb();
+}
+

/*
* Memory functions.
@@ -570,8 +582,8 @@ __EXTERN_INLINE int t2_is_mmio(const volatile void __iomem *addr)
/* New-style ioread interface. The mmio routines are so ugly for T2 that
it doesn't make sense to merge the pio and mmio routines. */

-#define IOPORT(OS, NS) \
-__EXTERN_INLINE unsigned int t2_ioread##NS(void __iomem *xaddr) \
+#define IOPORT(OS, NS, RT) \
+__EXTERN_INLINE RT t2_ioread##NS(void __iomem *xaddr) \
{ \
if (t2_is_mmio(xaddr)) \
return t2_read##OS(xaddr); \
@@ -586,14 +598,16 @@ __EXTERN_INLINE void t2_iowrite##NS(u##NS b, void __iomem *xaddr) \
t2_out##OS(b, (unsigned long)xaddr - T2_IO); \
}

-IOPORT(b, 8)
-IOPORT(w, 16)
-IOPORT(l, 32)
+IOPORT(b, 8, unsigned int)
+IOPORT(w, 16, unsigned int)
+IOPORT(l, 32, unsigned int)
+IOPORT(q, 64, u64)

#undef IOPORT

#undef vip
#undef vuip
+#undef vulp

#undef __IO_PREFIX
#define __IO_PREFIX t2
diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
index ff40491..16426e8 100644
--- a/arch/alpha/include/asm/io.h
+++ b/arch/alpha/include/asm/io.h
@@ -158,6 +158,7 @@ static inline void generic_##NAME(TYPE b, QUAL void __iomem *addr) \
REMAP1(unsigned int, ioread8, /**/)
REMAP1(unsigned int, ioread16, /**/)
REMAP1(unsigned int, ioread32, /**/)
+REMAP1(u64, ioread64, /**/)
REMAP1(u8, readb, const volatile)
REMAP1(u16, readw, const volatile)
REMAP1(u32, readl, const volatile)
@@ -166,6 +167,7 @@ REMAP1(u64, readq, const volatile)
REMAP2(u8, iowrite8, /**/)
REMAP2(u16, iowrite16, /**/)
REMAP2(u32, iowrite32, /**/)
+REMAP2(u64, iowrite64, /**/)
REMAP2(u8, writeb, volatile)
REMAP2(u16, writew, volatile)
REMAP2(u32, writel, volatile)
@@ -384,6 +386,19 @@ extern inline void iowrite32(u32 b, void __iomem *addr)
mb();
}

+extern inline u64 ioread64(void __iomem *addr)
+{
+ unsigned int ret = IO_CONCAT(__IO_PREFIX,ioread64)(addr);
+ mb();
+ return ret;
+}
+
+extern inline void iowrite64(u64 b, void __iomem *addr)
+{
+ IO_CONCAT(__IO_PREFIX,iowrite64)(b, addr);
+ mb();
+}
+
extern inline u32 inl(unsigned long port)
{
return ioread32(ioport_map(port, 4));
@@ -393,6 +408,16 @@ extern inline void outl(u32 b, unsigned long port)
{
iowrite32(b, ioport_map(port, 4));
}
+
+extern inline u64 inq(unsigned long port)
+{
+ return ioread64(ioport_map(port, 8));
+}
+
+extern inline void outq(u64 b, unsigned long port)
+{
+ iowrite64(b, ioport_map(port, 8));
+}
#endif

#if IO_CONCAT(__IO_PREFIX,trivial_rw_bw) == 1
@@ -493,8 +518,10 @@ extern inline void writeq(u64 b, volatile void __iomem *addr)

#define ioread16be(p) be16_to_cpu(ioread16(p))
#define ioread32be(p) be32_to_cpu(ioread32(p))
+#define ioread64be(p) be64_to_cpu(ioread64(p))
#define iowrite16be(v,p) iowrite16(cpu_to_be16(v), (p))
#define iowrite32be(v,p) iowrite32(cpu_to_be32(v), (p))
+#define iowrite64be(v,p) iowrite64(cpu_to_be64(v), (p))

#define inb_p inb
#define inw_p inw
diff --git a/arch/alpha/include/asm/io_trivial.h b/arch/alpha/include/asm/io_trivial.h
index 1c77f10..ae97730 100644
--- a/arch/alpha/include/asm/io_trivial.h
+++ b/arch/alpha/include/asm/io_trivial.h
@@ -37,11 +37,23 @@ IO_CONCAT(__IO_PREFIX,ioread32)(void __iomem *a)
return *(volatile u32 __force *)a;
}

+__EXTERN_INLINE u64
+IO_CONCAT(__IO_PREFIX,ioread64)(void __iomem *a)
+{
+ return *(volatile u64 __force *)a;
+}
+
__EXTERN_INLINE void
IO_CONCAT(__IO_PREFIX,iowrite32)(u32 b, void __iomem *a)
{
*(volatile u32 __force *)a = b;
}
+
+__EXTERN_INLINE void
+IO_CONCAT(__IO_PREFIX,iowrite64)(u64 b, void __iomem *a)
+{
+ *(volatile u64 __force *)a = b;
+}
#endif

#if IO_CONCAT(__IO_PREFIX,trivial_rw_bw) == 1
diff --git a/arch/alpha/include/asm/jensen.h b/arch/alpha/include/asm/jensen.h
index 964b06e..7e945cf 100644
--- a/arch/alpha/include/asm/jensen.h
+++ b/arch/alpha/include/asm/jensen.h
@@ -182,6 +182,17 @@ __EXTERN_INLINE u32 jensen_inl(unsigned long addr)
return *(vuip) ((addr << 7) + EISA_IO + 0x60);
}

+__EXTERN_INLINE u64 jensen_inq(unsigned long addr)
+{
+ unsigned long ioaddr = (addr << 7) + EISA_IO + 0x60;
+ unsigned long l, h;
+
+ jensen_set_hae(0);
+ l = *(vuip)ioaddr;
+ h = *(vuip)(ioaddr + (4 << 7));
+ return h << 32 | l;
+}
+
__EXTERN_INLINE void jensen_outw(u16 b, unsigned long addr)
{
jensen_set_hae(0);
@@ -196,6 +207,16 @@ __EXTERN_INLINE void jensen_outl(u32 b, unsigned long addr)
mb();
}

+__EXTERN_INLINE void jensen_outq(u64 b, unsigned long addr)
+{
+ unsigned long ioaddr = (addr << 7) + EISA_IO + 0x60;
+
+ jensen_set_hae(0);
+ *(vuip)ioaddr = b;
+ *(vuip)(ioaddr + (4 << 7)) = b >> 32;
+ mb();
+}
+
/*
* Memory functions.
*/
@@ -303,8 +324,8 @@ __EXTERN_INLINE int jensen_is_mmio(const volatile void __iomem *addr)
/* New-style ioread interface. All the routines are so ugly for Jensen
that it doesn't make sense to merge them. */

-#define IOPORT(OS, NS) \
-__EXTERN_INLINE unsigned int jensen_ioread##NS(void __iomem *xaddr) \
+#define IOPORT(OS, NS, RT) \
+__EXTERN_INLINE RT jensen_ioread##NS(void __iomem *xaddr) \
{ \
if (jensen_is_mmio(xaddr)) \
return jensen_read##OS(xaddr - 0x100000000ul); \
@@ -319,9 +340,10 @@ __EXTERN_INLINE void jensen_iowrite##NS(u##NS b, void __iomem *xaddr) \
jensen_out##OS(b, (unsigned long)xaddr); \
}

-IOPORT(b, 8)
-IOPORT(w, 16)
-IOPORT(l, 32)
+IOPORT(b, 8, unsigned int)
+IOPORT(w, 16, unsigned int)
+IOPORT(l, 32, unsigned int)
+IOPORT(q, 64, u64)

#undef IOPORT

diff --git a/arch/alpha/include/asm/machvec.h b/arch/alpha/include/asm/machvec.h
index 75cb364..c037c7b 100644
--- a/arch/alpha/include/asm/machvec.h
+++ b/arch/alpha/include/asm/machvec.h
@@ -48,10 +48,12 @@ struct alpha_machine_vector
unsigned int (*mv_ioread8)(void __iomem *);
unsigned int (*mv_ioread16)(void __iomem *);
unsigned int (*mv_ioread32)(void __iomem *);
+ u64 (*mv_ioread64)(void __iomem *);

void (*mv_iowrite8)(u8, void __iomem *);
void (*mv_iowrite16)(u16, void __iomem *);
void (*mv_iowrite32)(u32, void __iomem *);
+ void (*mv_iowrite64)(u64, void __iomem *);

u8 (*mv_readb)(const volatile void __iomem *);
u16 (*mv_readw)(const volatile void __iomem *);
diff --git a/arch/alpha/kernel/io.c b/arch/alpha/kernel/io.c
index 19c5875..d36146f 100644
--- a/arch/alpha/kernel/io.c
+++ b/arch/alpha/kernel/io.c
@@ -34,6 +34,13 @@ unsigned int ioread32(void __iomem *addr)
return ret;
}

+u64 ioread64(void __iomem *addr)
+{
+ u64 ret = IO_CONCAT(__IO_PREFIX,ioread64)(addr);
+ mb();
+ return ret;
+}
+
void iowrite8(u8 b, void __iomem *addr)
{
IO_CONCAT(__IO_PREFIX,iowrite8)(b, addr);
@@ -52,12 +59,20 @@ void iowrite32(u32 b, void __iomem *addr)
mb();
}

+void iowrite64(u64 b, void __iomem *addr)
+{
+ IO_CONCAT(__IO_PREFIX,iowrite64)(b, addr);
+ mb();
+}
+
EXPORT_SYMBOL(ioread8);
EXPORT_SYMBOL(ioread16);
EXPORT_SYMBOL(ioread32);
+EXPORT_SYMBOL(ioread64);
EXPORT_SYMBOL(iowrite8);
EXPORT_SYMBOL(iowrite16);
EXPORT_SYMBOL(iowrite32);
+EXPORT_SYMBOL(iowrite64);

u8 inb(unsigned long port)
{
@@ -74,6 +89,11 @@ u32 inl(unsigned long port)
return ioread32(ioport_map(port, 4));
}

+u64 inq(unsigned long port)
+{
+ return ioread64(ioport_map(port, 8));
+}
+
void outb(u8 b, unsigned long port)
{
iowrite8(b, ioport_map(port, 1));
@@ -89,12 +109,19 @@ void outl(u32 b, unsigned long port)
iowrite32(b, ioport_map(port, 4));
}

+void outq(u64 b, unsigned long port)
+{
+ iowrite64(b, ioport_map(port, 8));
+}
+
EXPORT_SYMBOL(inb);
EXPORT_SYMBOL(inw);
EXPORT_SYMBOL(inl);
+EXPORT_SYMBOL(inq);
EXPORT_SYMBOL(outb);
EXPORT_SYMBOL(outw);
EXPORT_SYMBOL(outl);
+EXPORT_SYMBOL(outq);

u8 __raw_readb(const volatile void __iomem *addr)
{
diff --git a/arch/alpha/kernel/machvec_impl.h b/arch/alpha/kernel/machvec_impl.h
index b7d6960..9b37e19 100644
--- a/arch/alpha/kernel/machvec_impl.h
+++ b/arch/alpha/kernel/machvec_impl.h
@@ -79,9 +79,11 @@
.mv_ioread8 = CAT(low,_ioread8), \
.mv_ioread16 = CAT(low,_ioread16), \
.mv_ioread32 = CAT(low,_ioread32), \
+ .mv_ioread64 = CAT(low,_ioread64), \
.mv_iowrite8 = CAT(low,_iowrite8), \
.mv_iowrite16 = CAT(low,_iowrite16), \
.mv_iowrite32 = CAT(low,_iowrite32), \
+ .mv_iowrite64 = CAT(low,_iowrite64), \
.mv_readb = CAT(low,_readb), \
.mv_readw = CAT(low,_readw), \
.mv_readl = CAT(low,_readl), \
--
2.9.4

2017-06-24 15:19:56

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH] alpha: provide ioread64 and iowrite64 implementations

Hey,

On 24/06/17 09:13 AM, Richard Henderson wrote:
> All Alpha hosts except for Jensen provide 64-bit I/O operations.
>
> Jensen is EISA only, so there ought not be any devices that even
> attempt such operations. But just in case, use 2 32-bit operations.

Thanks for this, Richard.

However, I was recently enlightened by the existence of the
linux/io-64-nonatomic* headers. This is where the 2 32-bit operation
functions belong. So you should probably remove them from this patch and
let drivers that need them just include those headers.

Logan

> Cc: Logan Gunthorpe <[email protected]>
> Cc: Ivan Kokshaysky <[email protected]>
> Cc: Matt Turner <[email protected]>
> Signed-off-by: Richard Henderson <[email protected]>
> ---
> arch/alpha/include/asm/core_apecs.h | 16 ++++++++++++++++
> arch/alpha/include/asm/core_cia.h | 16 ++++++++++++++++
> arch/alpha/include/asm/core_lca.h | 16 ++++++++++++++++
> arch/alpha/include/asm/core_mcpcia.h | 21 +++++++++++++++++++++
> arch/alpha/include/asm/core_t2.h | 24 +++++++++++++++++++-----
> arch/alpha/include/asm/io.h | 27 +++++++++++++++++++++++++++
> arch/alpha/include/asm/io_trivial.h | 12 ++++++++++++
> arch/alpha/include/asm/jensen.h | 32 +++++++++++++++++++++++++++-----
> arch/alpha/include/asm/machvec.h | 2 ++
> arch/alpha/kernel/io.c | 27 +++++++++++++++++++++++++++
> arch/alpha/kernel/machvec_impl.h | 2 ++
> 11 files changed, 185 insertions(+), 10 deletions(-)
>
> diff --git a/arch/alpha/include/asm/core_apecs.h b/arch/alpha/include/asm/core_apecs.h
> index 6785ff7..7e2ff13 100644
> --- a/arch/alpha/include/asm/core_apecs.h
> +++ b/arch/alpha/include/asm/core_apecs.h
> @@ -471,6 +471,22 @@ __EXTERN_INLINE void apecs_iowrite32(u32 b, void __iomem *xaddr)
> *(vuip)addr = b;
> }
>
> +__EXTERN_INLINE u64 apecs_ioread64(void __iomem *xaddr)
> +{
> + unsigned long addr = (unsigned long) xaddr;
> + if (addr < APECS_DENSE_MEM)
> + addr = ((addr - APECS_IO) << 5) + APECS_IO + 0x78;
> + return *(vulp)addr;
> +}
> +
> +__EXTERN_INLINE void apecs_iowrite64(u64 b, void __iomem *xaddr)
> +{
> + unsigned long addr = (unsigned long) xaddr;
> + if (addr < APECS_DENSE_MEM)
> + addr = ((addr - APECS_IO) << 5) + APECS_IO + 0x78;
> + *(vulp)addr = b;
> +}
> +
> __EXTERN_INLINE void __iomem *apecs_ioportmap(unsigned long addr)
> {
> return (void __iomem *)(addr + APECS_IO);
> diff --git a/arch/alpha/include/asm/core_cia.h b/arch/alpha/include/asm/core_cia.h
> index 9e0516c..54792b3 100644
> --- a/arch/alpha/include/asm/core_cia.h
> +++ b/arch/alpha/include/asm/core_cia.h
> @@ -419,6 +419,22 @@ __EXTERN_INLINE void cia_iowrite32(u32 b, void __iomem *xaddr)
> *(vuip)addr = b;
> }
>
> +__EXTERN_INLINE u64 cia_ioread64(void __iomem *xaddr)
> +{
> + unsigned long addr = (unsigned long) xaddr;
> + if (addr < CIA_DENSE_MEM)
> + addr = ((addr - CIA_IO) << 5) + CIA_IO + 0x78;
> + return *(vulp)addr;
> +}
> +
> +__EXTERN_INLINE void cia_iowrite64(u64 b, void __iomem *xaddr)
> +{
> + unsigned long addr = (unsigned long) xaddr;
> + if (addr < CIA_DENSE_MEM)
> + addr = ((addr - CIA_IO) << 5) + CIA_IO + 0x78;
> + *(vulp)addr = b;
> +}
> +
> __EXTERN_INLINE void __iomem *cia_ioportmap(unsigned long addr)
> {
> return (void __iomem *)(addr + CIA_IO);
> diff --git a/arch/alpha/include/asm/core_lca.h b/arch/alpha/include/asm/core_lca.h
> index 8ee6c51..b8e43cd 100644
> --- a/arch/alpha/include/asm/core_lca.h
> +++ b/arch/alpha/include/asm/core_lca.h
> @@ -317,6 +317,22 @@ __EXTERN_INLINE void lca_iowrite32(u32 b, void __iomem *xaddr)
> *(vuip)addr = b;
> }
>
> +__EXTERN_INLINE u64 lca_ioread64(void __iomem *xaddr)
> +{
> + unsigned long addr = (unsigned long) xaddr;
> + if (addr < LCA_DENSE_MEM)
> + addr = ((addr - LCA_IO) << 5) + LCA_IO + 0x78;
> + return *(vulp)addr;
> +}
> +
> +__EXTERN_INLINE void lca_iowrite64(u64 b, void __iomem *xaddr)
> +{
> + unsigned long addr = (unsigned long) xaddr;
> + if (addr < LCA_DENSE_MEM)
> + addr = ((addr - LCA_IO) << 5) + LCA_IO + 0x78;
> + *(vulp)addr = b;
> +}
> +
> __EXTERN_INLINE void __iomem *lca_ioportmap(unsigned long addr)
> {
> return (void __iomem *)(addr + LCA_IO);
> diff --git a/arch/alpha/include/asm/core_mcpcia.h b/arch/alpha/include/asm/core_mcpcia.h
> index ad44bef..e4d9139 100644
> --- a/arch/alpha/include/asm/core_mcpcia.h
> +++ b/arch/alpha/include/asm/core_mcpcia.h
> @@ -247,6 +247,7 @@ struct el_MCPCIA_uncorrected_frame_mcheck {
>
> #define vip volatile int __force *
> #define vuip volatile unsigned int __force *
> +#define vulp volatile unsigned long __force *
>
> #ifndef MCPCIA_ONE_HAE_WINDOW
> #define MCPCIA_FROB_MMIO \
> @@ -334,6 +335,25 @@ __EXTERN_INLINE void mcpcia_iowrite32(u32 b, void __iomem *xaddr)
> *(vuip)addr = b;
> }
>
> +__EXTERN_INLINE u64 mcpcia_ioread64(void __iomem *xaddr)
> +{
> + unsigned long addr = (unsigned long)xaddr;
> +
> + if (!__mcpcia_is_mmio(addr))
> + addr = ((addr & 0xffff) << 5) + (addr & ~0xfffful) + 0x78;
> +
> + return *(vulp)addr;
> +}
> +
> +__EXTERN_INLINE void mcpcia_iowrite64(u64 b, void __iomem *xaddr)
> +{
> + unsigned long addr = (unsigned long)xaddr;
> +
> + if (!__mcpcia_is_mmio(addr))
> + addr = ((addr & 0xffff) << 5) + (addr & ~0xfffful) + 0x78;
> +
> + *(vulp)addr = b;
> +}
>
> __EXTERN_INLINE void __iomem *mcpcia_ioportmap(unsigned long addr)
> {
> @@ -361,6 +381,7 @@ __EXTERN_INLINE int mcpcia_is_mmio(const volatile void __iomem *xaddr)
>
> #undef vip
> #undef vuip
> +#undef vulp
>
> #undef __IO_PREFIX
> #define __IO_PREFIX mcpcia
> diff --git a/arch/alpha/include/asm/core_t2.h b/arch/alpha/include/asm/core_t2.h
> index ade9d92..cb3467d 100644
> --- a/arch/alpha/include/asm/core_t2.h
> +++ b/arch/alpha/include/asm/core_t2.h
> @@ -359,6 +359,7 @@ struct el_t2_frame_corrected {
>
> #define vip volatile int *
> #define vuip volatile unsigned int *
> +#define vulp volatile unsigned long *
>
> extern inline u8 t2_inb(unsigned long addr)
> {
> @@ -401,6 +402,17 @@ extern inline void t2_outl(u32 b, unsigned long addr)
> mb();
> }
>
> +extern inline u64 t2_inq(unsigned long addr)
> +{
> + return *(vulp) ((addr << 5) + T2_IO + 0x78);
> +}
> +
> +extern inline void t2_outq(u64 b, unsigned long addr)
> +{
> + *(vulp) ((addr << 5) + T2_IO + 0x78) = b;
> + mb();
> +}
> +
>
> /*
> * Memory functions.
> @@ -570,8 +582,8 @@ __EXTERN_INLINE int t2_is_mmio(const volatile void __iomem *addr)
> /* New-style ioread interface. The mmio routines are so ugly for T2 that
> it doesn't make sense to merge the pio and mmio routines. */
>
> -#define IOPORT(OS, NS) \
> -__EXTERN_INLINE unsigned int t2_ioread##NS(void __iomem *xaddr) \
> +#define IOPORT(OS, NS, RT) \
> +__EXTERN_INLINE RT t2_ioread##NS(void __iomem *xaddr) \
> { \
> if (t2_is_mmio(xaddr)) \
> return t2_read##OS(xaddr); \
> @@ -586,14 +598,16 @@ __EXTERN_INLINE void t2_iowrite##NS(u##NS b, void __iomem *xaddr) \
> t2_out##OS(b, (unsigned long)xaddr - T2_IO); \
> }
>
> -IOPORT(b, 8)
> -IOPORT(w, 16)
> -IOPORT(l, 32)
> +IOPORT(b, 8, unsigned int)
> +IOPORT(w, 16, unsigned int)
> +IOPORT(l, 32, unsigned int)
> +IOPORT(q, 64, u64)
>
> #undef IOPORT
>
> #undef vip
> #undef vuip
> +#undef vulp
>
> #undef __IO_PREFIX
> #define __IO_PREFIX t2
> diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
> index ff40491..16426e8 100644
> --- a/arch/alpha/include/asm/io.h
> +++ b/arch/alpha/include/asm/io.h
> @@ -158,6 +158,7 @@ static inline void generic_##NAME(TYPE b, QUAL void __iomem *addr) \
> REMAP1(unsigned int, ioread8, /**/)
> REMAP1(unsigned int, ioread16, /**/)
> REMAP1(unsigned int, ioread32, /**/)
> +REMAP1(u64, ioread64, /**/)
> REMAP1(u8, readb, const volatile)
> REMAP1(u16, readw, const volatile)
> REMAP1(u32, readl, const volatile)
> @@ -166,6 +167,7 @@ REMAP1(u64, readq, const volatile)
> REMAP2(u8, iowrite8, /**/)
> REMAP2(u16, iowrite16, /**/)
> REMAP2(u32, iowrite32, /**/)
> +REMAP2(u64, iowrite64, /**/)
> REMAP2(u8, writeb, volatile)
> REMAP2(u16, writew, volatile)
> REMAP2(u32, writel, volatile)
> @@ -384,6 +386,19 @@ extern inline void iowrite32(u32 b, void __iomem *addr)
> mb();
> }
>
> +extern inline u64 ioread64(void __iomem *addr)
> +{
> + unsigned int ret = IO_CONCAT(__IO_PREFIX,ioread64)(addr);
> + mb();
> + return ret;
> +}
> +
> +extern inline void iowrite64(u64 b, void __iomem *addr)
> +{
> + IO_CONCAT(__IO_PREFIX,iowrite64)(b, addr);
> + mb();
> +}
> +
> extern inline u32 inl(unsigned long port)
> {
> return ioread32(ioport_map(port, 4));
> @@ -393,6 +408,16 @@ extern inline void outl(u32 b, unsigned long port)
> {
> iowrite32(b, ioport_map(port, 4));
> }
> +
> +extern inline u64 inq(unsigned long port)
> +{
> + return ioread64(ioport_map(port, 8));
> +}
> +
> +extern inline void outq(u64 b, unsigned long port)
> +{
> + iowrite64(b, ioport_map(port, 8));
> +}
> #endif
>
> #if IO_CONCAT(__IO_PREFIX,trivial_rw_bw) == 1
> @@ -493,8 +518,10 @@ extern inline void writeq(u64 b, volatile void __iomem *addr)
>
> #define ioread16be(p) be16_to_cpu(ioread16(p))
> #define ioread32be(p) be32_to_cpu(ioread32(p))
> +#define ioread64be(p) be64_to_cpu(ioread64(p))
> #define iowrite16be(v,p) iowrite16(cpu_to_be16(v), (p))
> #define iowrite32be(v,p) iowrite32(cpu_to_be32(v), (p))
> +#define iowrite64be(v,p) iowrite64(cpu_to_be64(v), (p))
>
> #define inb_p inb
> #define inw_p inw
> diff --git a/arch/alpha/include/asm/io_trivial.h b/arch/alpha/include/asm/io_trivial.h
> index 1c77f10..ae97730 100644
> --- a/arch/alpha/include/asm/io_trivial.h
> +++ b/arch/alpha/include/asm/io_trivial.h
> @@ -37,11 +37,23 @@ IO_CONCAT(__IO_PREFIX,ioread32)(void __iomem *a)
> return *(volatile u32 __force *)a;
> }
>
> +__EXTERN_INLINE u64
> +IO_CONCAT(__IO_PREFIX,ioread64)(void __iomem *a)
> +{
> + return *(volatile u64 __force *)a;
> +}
> +
> __EXTERN_INLINE void
> IO_CONCAT(__IO_PREFIX,iowrite32)(u32 b, void __iomem *a)
> {
> *(volatile u32 __force *)a = b;
> }
> +
> +__EXTERN_INLINE void
> +IO_CONCAT(__IO_PREFIX,iowrite64)(u64 b, void __iomem *a)
> +{
> + *(volatile u64 __force *)a = b;
> +}
> #endif
>
> #if IO_CONCAT(__IO_PREFIX,trivial_rw_bw) == 1
> diff --git a/arch/alpha/include/asm/jensen.h b/arch/alpha/include/asm/jensen.h
> index 964b06e..7e945cf 100644
> --- a/arch/alpha/include/asm/jensen.h
> +++ b/arch/alpha/include/asm/jensen.h
> @@ -182,6 +182,17 @@ __EXTERN_INLINE u32 jensen_inl(unsigned long addr)
> return *(vuip) ((addr << 7) + EISA_IO + 0x60);
> }
>
> +__EXTERN_INLINE u64 jensen_inq(unsigned long addr)
> +{
> + unsigned long ioaddr = (addr << 7) + EISA_IO + 0x60;
> + unsigned long l, h;
> +
> + jensen_set_hae(0);
> + l = *(vuip)ioaddr;
> + h = *(vuip)(ioaddr + (4 << 7));
> + return h << 32 | l;
> +}
> +
> __EXTERN_INLINE void jensen_outw(u16 b, unsigned long addr)
> {
> jensen_set_hae(0);
> @@ -196,6 +207,16 @@ __EXTERN_INLINE void jensen_outl(u32 b, unsigned long addr)
> mb();
> }
>
> +__EXTERN_INLINE void jensen_outq(u64 b, unsigned long addr)
> +{
> + unsigned long ioaddr = (addr << 7) + EISA_IO + 0x60;
> +
> + jensen_set_hae(0);
> + *(vuip)ioaddr = b;
> + *(vuip)(ioaddr + (4 << 7)) = b >> 32;
> + mb();
> +}
> +
> /*
> * Memory functions.
> */
> @@ -303,8 +324,8 @@ __EXTERN_INLINE int jensen_is_mmio(const volatile void __iomem *addr)
> /* New-style ioread interface. All the routines are so ugly for Jensen
> that it doesn't make sense to merge them. */
>
> -#define IOPORT(OS, NS) \
> -__EXTERN_INLINE unsigned int jensen_ioread##NS(void __iomem *xaddr) \
> +#define IOPORT(OS, NS, RT) \
> +__EXTERN_INLINE RT jensen_ioread##NS(void __iomem *xaddr) \
> { \
> if (jensen_is_mmio(xaddr)) \
> return jensen_read##OS(xaddr - 0x100000000ul); \
> @@ -319,9 +340,10 @@ __EXTERN_INLINE void jensen_iowrite##NS(u##NS b, void __iomem *xaddr) \
> jensen_out##OS(b, (unsigned long)xaddr); \
> }
>
> -IOPORT(b, 8)
> -IOPORT(w, 16)
> -IOPORT(l, 32)
> +IOPORT(b, 8, unsigned int)
> +IOPORT(w, 16, unsigned int)
> +IOPORT(l, 32, unsigned int)
> +IOPORT(q, 64, u64)
>
> #undef IOPORT
>
> diff --git a/arch/alpha/include/asm/machvec.h b/arch/alpha/include/asm/machvec.h
> index 75cb364..c037c7b 100644
> --- a/arch/alpha/include/asm/machvec.h
> +++ b/arch/alpha/include/asm/machvec.h
> @@ -48,10 +48,12 @@ struct alpha_machine_vector
> unsigned int (*mv_ioread8)(void __iomem *);
> unsigned int (*mv_ioread16)(void __iomem *);
> unsigned int (*mv_ioread32)(void __iomem *);
> + u64 (*mv_ioread64)(void __iomem *);
>
> void (*mv_iowrite8)(u8, void __iomem *);
> void (*mv_iowrite16)(u16, void __iomem *);
> void (*mv_iowrite32)(u32, void __iomem *);
> + void (*mv_iowrite64)(u64, void __iomem *);
>
> u8 (*mv_readb)(const volatile void __iomem *);
> u16 (*mv_readw)(const volatile void __iomem *);
> diff --git a/arch/alpha/kernel/io.c b/arch/alpha/kernel/io.c
> index 19c5875..d36146f 100644
> --- a/arch/alpha/kernel/io.c
> +++ b/arch/alpha/kernel/io.c
> @@ -34,6 +34,13 @@ unsigned int ioread32(void __iomem *addr)
> return ret;
> }
>
> +u64 ioread64(void __iomem *addr)
> +{
> + u64 ret = IO_CONCAT(__IO_PREFIX,ioread64)(addr);
> + mb();
> + return ret;
> +}
> +
> void iowrite8(u8 b, void __iomem *addr)
> {
> IO_CONCAT(__IO_PREFIX,iowrite8)(b, addr);
> @@ -52,12 +59,20 @@ void iowrite32(u32 b, void __iomem *addr)
> mb();
> }
>
> +void iowrite64(u64 b, void __iomem *addr)
> +{
> + IO_CONCAT(__IO_PREFIX,iowrite64)(b, addr);
> + mb();
> +}
> +
> EXPORT_SYMBOL(ioread8);
> EXPORT_SYMBOL(ioread16);
> EXPORT_SYMBOL(ioread32);
> +EXPORT_SYMBOL(ioread64);
> EXPORT_SYMBOL(iowrite8);
> EXPORT_SYMBOL(iowrite16);
> EXPORT_SYMBOL(iowrite32);
> +EXPORT_SYMBOL(iowrite64);
>
> u8 inb(unsigned long port)
> {
> @@ -74,6 +89,11 @@ u32 inl(unsigned long port)
> return ioread32(ioport_map(port, 4));
> }
>
> +u64 inq(unsigned long port)
> +{
> + return ioread64(ioport_map(port, 8));
> +}
> +
> void outb(u8 b, unsigned long port)
> {
> iowrite8(b, ioport_map(port, 1));
> @@ -89,12 +109,19 @@ void outl(u32 b, unsigned long port)
> iowrite32(b, ioport_map(port, 4));
> }
>
> +void outq(u64 b, unsigned long port)
> +{
> + iowrite64(b, ioport_map(port, 8));
> +}
> +
> EXPORT_SYMBOL(inb);
> EXPORT_SYMBOL(inw);
> EXPORT_SYMBOL(inl);
> +EXPORT_SYMBOL(inq);
> EXPORT_SYMBOL(outb);
> EXPORT_SYMBOL(outw);
> EXPORT_SYMBOL(outl);
> +EXPORT_SYMBOL(outq);
>
> u8 __raw_readb(const volatile void __iomem *addr)
> {
> diff --git a/arch/alpha/kernel/machvec_impl.h b/arch/alpha/kernel/machvec_impl.h
> index b7d6960..9b37e19 100644
> --- a/arch/alpha/kernel/machvec_impl.h
> +++ b/arch/alpha/kernel/machvec_impl.h
> @@ -79,9 +79,11 @@
> .mv_ioread8 = CAT(low,_ioread8), \
> .mv_ioread16 = CAT(low,_ioread16), \
> .mv_ioread32 = CAT(low,_ioread32), \
> + .mv_ioread64 = CAT(low,_ioread64), \
> .mv_iowrite8 = CAT(low,_iowrite8), \
> .mv_iowrite16 = CAT(low,_iowrite16), \
> .mv_iowrite32 = CAT(low,_iowrite32), \
> + .mv_iowrite64 = CAT(low,_iowrite64), \
> .mv_readb = CAT(low,_readb), \
> .mv_readw = CAT(low,_readw), \
> .mv_readl = CAT(low,_readl), \
>

2017-06-24 15:25:35

by Richard Henderson

[permalink] [raw]
Subject: Re: [PATCH] alpha: provide ioread64 and iowrite64 implementations

On 06/24/2017 08:19 AM, Logan Gunthorpe wrote:
> Hey,
>
> On 24/06/17 09:13 AM, Richard Henderson wrote:
>> All Alpha hosts except for Jensen provide 64-bit I/O operations.
>>
>> Jensen is EISA only, so there ought not be any devices that even
>> attempt such operations. But just in case, use 2 32-bit operations.
>
> Thanks for this, Richard.
>
> However, I was recently enlightened by the existence of the
> linux/io-64-nonatomic* headers. This is where the 2 32-bit operation
> functions belong. So you should probably remove them from this patch and
> let drivers that need them just include those headers.

This has nothing to do with drivers.

This is all about providing 64-bit operations for the 99.999% of alphas that do
have them.


r~

2017-06-24 15:32:11

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH] alpha: provide ioread64 and iowrite64 implementations



On 24/06/17 09:25 AM, Richard Henderson wrote:
> On 06/24/2017 08:19 AM, Logan Gunthorpe wrote:
>> Hey,
>>
>> On 24/06/17 09:13 AM, Richard Henderson wrote:
>>> All Alpha hosts except for Jensen provide 64-bit I/O operations.
>>>
>>> Jensen is EISA only, so there ought not be any devices that even
>>> attempt such operations. But just in case, use 2 32-bit operations.
>>
>> Thanks for this, Richard.
>>
>> However, I was recently enlightened by the existence of the
>> linux/io-64-nonatomic* headers. This is where the 2 32-bit operation
>> functions belong. So you should probably remove them from this patch and
>> let drivers that need them just include those headers.
>
> This has nothing to do with drivers.
>
> This is all about providing 64-bit operations for the 99.999% of alphas
> that do have them.

As per the discussion with Arnd and Alan, that's not what they want.
Arches that have native 64bit operations should supply them. For drivers
that want to use them in the presence of arches that don't supply them,
they simply include one of the linux/io-64-nonatomic helper headers.

So, IMO, the Jensen inq and outq funtions in your patch, which I did
read, should probably just not be provided.

Logan

2017-06-24 16:14:21

by Richard Henderson

[permalink] [raw]
Subject: Re: [PATCH] alpha: provide ioread64 and iowrite64 implementations

On 06/24/2017 08:32 AM, Logan Gunthorpe wrote:
> So, IMO, the Jensen inq and outq funtions in your patch, which I did
> read, should probably just not be provided.

So you would prefer a function containing BUG?
Because I have to have *something* to put into the machvec function pointer.


r~

2017-06-24 17:17:54

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH] alpha: provide ioread64 and iowrite64 implementations



On 24/06/17 10:14 AM, Richard Henderson wrote:
> So you would prefer a function containing BUG?

Don't be so obtuse. Of course not.

> Because I have to have *something* to put into the machvec function
> pointer.

Well I can't say I understand the point of the machvec code, but if
you're saying it's impossible to have the Jensen sub-arch not supply a
64bit io functions then, fine, leave it as is.

Logan

2017-06-26 08:54:55

by Jyri Sarha

[permalink] [raw]
Subject: Re: [PATCH 1/7] drm/tilcdc: don't use volatile with iowrite64

On 06/22/17 19:48, Logan Gunthorpe wrote:
> This is a prep patch for adding a universal iowrite64.
>
> The patch is to prevent compiler warnings when we add iowrite64 that
> would occur because there is an unnecessary volatile in this driver.
>
> Signed-off-by: Logan Gunthorpe <[email protected]>
> Cc: Jyri Sarha <[email protected]>
> Cc: Tomi Valkeinen <[email protected]>
> Cc: David Airlie <[email protected]>

Acked-by: Jyri Sarha <[email protected]>

> ---
> drivers/gpu/drm/tilcdc/tilcdc_regs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
> index 9d528c0a67a4..e9ce725698a9 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
> @@ -131,14 +131,14 @@ static inline void tilcdc_write(struct drm_device *dev, u32 reg, u32 data)
> 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;
> + void __iomem *addr = priv->mmio + reg;
>
> #ifdef iowrite64
> iowrite64(data, addr);
> #else
> __iowmb();
> /* This compiles to strd (=64-bit write) on ARM7 */
> - *(volatile u64 __force *)addr = __cpu_to_le64(data);
> + *(u64 __force *)addr = __cpu_to_le64(data);
> #endif
> }
>
>

2017-06-26 08:55:36

by Jyri Sarha

[permalink] [raw]
Subject: Re: [PATCH 6/7] drm/tilcdc: clean up ifdef hacks around iowrite64

On 06/22/17 19:48, Logan Gunthorpe wrote:
> Now that we can expect iowrite64 to always exist the hack is no longer
> necessary so we just call iowrite64 directly.
>
> Signed-off-by: Logan Gunthorpe <[email protected]>
> Cc: Jyri Sarha <[email protected]>
> Cc: Tomi Valkeinen <[email protected]>
> Cc: David Airlie <[email protected]>

Acked-by: Jyri Sarha <[email protected]>

And thanks!

> ---
> drivers/gpu/drm/tilcdc/tilcdc_regs.h | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
> index e9ce725698a9..0b901405f30a 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
> @@ -133,13 +133,7 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data)
> struct tilcdc_drm_private *priv = dev->dev_private;
> void __iomem *addr = priv->mmio + reg;
>
> -#ifdef iowrite64
> iowrite64(data, addr);
> -#else
> - __iowmb();
> - /* This compiles to strd (=64-bit write) on ARM7 */
> - *(u64 __force *)addr = __cpu_to_le64(data);
> -#endif
> }
>
> static inline u32 tilcdc_read(struct drm_device *dev, u32 reg)
>

2017-06-26 16:26:57

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 6/7] drm/tilcdc: clean up ifdef hacks around iowrite64

Hi Jyri,

Thanks for the ack. However, I'm reworking this patch set to use the
include/linux/io-64-nonatomic* headers which will explicitly devolve
into two 32-bit transfers. It's not clear whether this is appropriate
for the tilcdc driver as it was never setup to use 32-bit transfers
(unlike the others I had patched).

If you think it's ok, I can still patch this driver to use the
non-atomic headers. Otherwise I can leave it out. Please let me know.

Thanks,

Logan


On 26/06/17 02:55 AM, Jyri Sarha wrote:
> Acked-by: Jyri Sarha <[email protected]>
>
> And thanks!
>
>> ---
>> drivers/gpu/drm/tilcdc/tilcdc_regs.h | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
>> index e9ce725698a9..0b901405f30a 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h
>> @@ -133,13 +133,7 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data)
>> struct tilcdc_drm_private *priv = dev->dev_private;
>> void __iomem *addr = priv->mmio + reg;
>>
>> -#ifdef iowrite64
>> iowrite64(data, addr);
>> -#else
>> - __iowmb();
>> - /* This compiles to strd (=64-bit write) on ARM7 */
>> - *(u64 __force *)addr = __cpu_to_le64(data);
>> -#endif
>> }
>>
>> static inline u32 tilcdc_read(struct drm_device *dev, u32 reg)
>>
>

2017-06-26 20:43:48

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 2/7] iomap: implement ioread64 and iowrite64

> +u64 ioread64(void __iomem *addr)
> +{
> + u64 low, high;
> +
> + low = ioread32(addr);
> + high = ioread32(addr + sizeof(u32));
> + return low | (high << 32);
> +}
> +u64 ioread64be(void __iomem *addr)
> +{
> + u64 low, high;
> +
> + low = ioread32be(addr + sizeof(u32));
> + high = ioread32be(addr);
> + return low | (high << 32);
> +}
> +#endif

This hardcodes the behavior of include/linux/io-64-nonatomic-hi-lo.h, which
I find rather confusing, as only about one in five drivers wants this
behavior.

I'd suggest you don't add it in lib/iomap.c at all for 32-bit architectures,
but rather use the same logic that we have for readq/writeq in
io-64-nonatomic-hi-lo.h and io-64-nonatomic-lo-hi.h, adding
{lo_hi,hi_lo}_{ioread,iowrite}{,be} to the same files, and provide
the {ioread,iowrite}{,be} macros only if they have not been defined
at that point.

Arnd

2017-06-26 21:26:08

by Logan Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH 2/7] iomap: implement ioread64 and iowrite64

On 6/26/2017 2:43 PM, Arnd Bergmann wrote:
> This hardcodes the behavior of include/linux/io-64-nonatomic-hi-lo.h, which
> I find rather confusing, as only about one in five drivers wants this
> behavior.
>
> I'd suggest you don't add it in lib/iomap.c at all for 32-bit architectures,
> but rather use the same logic that we have for readq/writeq in
> io-64-nonatomic-hi-lo.h and io-64-nonatomic-lo-hi.h, adding
> {lo_hi,hi_lo}_{ioread,iowrite}{,be} to the same files, and provide
> the {ioread,iowrite}{,be} macros only if they have not been defined
> at that point.

Thanks Arnd. Yes, I'm already reworking this patchset to do exactly that.

Logan

2017-06-27 20:40:25

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 6/7] drm/tilcdc: clean up ifdef hacks around iowrite64

On Mon, Jun 26, 2017 at 6:26 PM, Logan Gunthorpe <[email protected]> wrote:
> Hi Jyri,
>
> Thanks for the ack. However, I'm reworking this patch set to use the
> include/linux/io-64-nonatomic* headers which will explicitly devolve
> into two 32-bit transfers. It's not clear whether this is appropriate
> for the tilcdc driver as it was never setup to use 32-bit transfers
> (unlike the others I had patched).
>
> If you think it's ok, I can still patch this driver to use the
> non-atomic headers. Otherwise I can leave it out. Please let me know.

You'd have to first figure out whether this device is of the lo-hi
or the hi-lo variant, or doesn't allow the I/O to be split at all.

Note that we could theoretically define ARM to use strd/ldrd
for writeq/readq, but I would expect that to be wrong with many
other devices that can use the existing io-64-nonatomic headers.

The comment in set_scanout() suggests that we actually do rely
on the write64 to be atomic, so we probably don't want to change
this driver.

Arnd