Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1516758 > unrolled thread

[PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-11-08 04:30 +0100
    Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Mark Rutland <mark.rutland@arm.com> - 2016-11-08 13:10 +0100
      Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Arnd Bergmann <arnd@arndb.de> - 2016-11-08 17:20 +0100
        Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Arnd Bergmann <arnd@arndb.de> - 2016-11-08 17:20 +0100
      Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-11-09 00:20 +0100
        Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-11-10 09:40 +0100
        Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Mark Rutland <mark.rutland@arm.com> - 2016-11-10 12:30 +0100
          Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-11-10 20:40 +0100
            Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-11-11 11:10 +0100
    Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Will Deacon <will.deacon@arm.com> - 2016-11-08 17:20 +0100
      Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced John Garry <john.garry@huawei.com> - 2016-11-08 17:50 +0100
        Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Will Deacon <will.deacon@arm.com> - 2016-11-08 17:50 +0100
          Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced John Garry <john.garry@huawei.com> - 2016-11-08 18:10 +0100
          Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Arnd Bergmann <arnd@arndb.de> - 2016-11-08 23:40 +0100
            Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced John Garry <john.garry@huawei.com> - 2016-11-09 12:40 +0100
              Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced Arnd Bergmann <arnd@arndb.de> - 2016-11-09 22:40 +0100

#1516758 — [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced

For arm64, there is no I/O space as other architectural platforms, such as
X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
such as Hip06, when accessing some legacy ISA devices connected to LPC, those
known port addresses are used to control the corresponding target devices, for
example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
normal MMIO mode in using.

To drive these devices, this patch introduces a method named indirect-IO.
In this method the in/out pair in arch/arm64/include/asm/io.h will be
redefined. When upper layer drivers call in/out with those known legacy port
addresses to access the peripherals, the hooking functions corrresponding to
those target peripherals will be called. Through this way, those upper layer
drivers which depend on in/out can run on Hip06 without any changes.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 arch/arm64/Kconfig             |  6 +++
 arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/io.h    | 29 +++++++++++++
 arch/arm64/kernel/Makefile     |  1 +
 arch/arm64/kernel/extio.c      | 27 ++++++++++++
 5 files changed, 157 insertions(+)
 create mode 100644 arch/arm64/include/asm/extio.h
 create mode 100644 arch/arm64/kernel/extio.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 969ef88..b44070b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -163,6 +163,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
 config ARCH_MMAP_RND_COMPAT_BITS_MAX
        default 16
 
+config ARM64_INDIRECT_PIO
+	bool "access peripherals with legacy I/O port"
+	help
+	  Support special accessors for ISA I/O devices. This is needed for
+	  SoCs that do not support standard read/write for the ISA range.
+
 config NO_IOPORT_MAP
 	def_bool y if !PCI
 
diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
new file mode 100644
index 0000000..6ae0787
--- /dev/null
+++ b/arch/arm64/include/asm/extio.h
@@ -0,0 +1,94 @@
+/*
+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __LINUX_EXTIO_H
+#define __LINUX_EXTIO_H
+
+struct extio_ops {
+	unsigned long start;/* inclusive, sys io addr */
+	unsigned long end;/* inclusive, sys io addr */
+
+	u64 (*pfin)(void *devobj, unsigned long ptaddr,	size_t dlen);
+	void (*pfout)(void *devobj, unsigned long ptaddr, u32 outval,
+					size_t dlen);
+	u64 (*pfins)(void *devobj, unsigned long ptaddr, void *inbuf,
+				size_t dlen, unsigned int count);
+	void (*pfouts)(void *devobj, unsigned long ptaddr,
+				const void *outbuf, size_t dlen,
+				unsigned int count);
+	void *devpara;
+};
+
+extern struct extio_ops *arm64_extio_ops;
+
+#define DECLARE_EXTIO(bw, type)						\
+extern type in##bw(unsigned long addr);					\
+extern void out##bw(type value, unsigned long addr);			\
+extern void ins##bw(unsigned long addr, void *buffer, unsigned int count);\
+extern void outs##bw(unsigned long addr, const void *buffer, unsigned int count);
+
+#define BUILD_EXTIO(bw, type)						\
+type in##bw(unsigned long addr)						\
+{									\
+	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
+			arm64_extio_ops->end < addr)			\
+		return read##bw(PCI_IOBASE + addr);			\
+	return arm64_extio_ops->pfin ?					\
+		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
+			addr, sizeof(type)) : -1;			\
+}									\
+									\
+void out##bw(type value, unsigned long addr)				\
+{									\
+	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
+			arm64_extio_ops->end < addr)			\
+		write##bw(value, PCI_IOBASE + addr);			\
+	else								\
+		if (arm64_extio_ops->pfout)				\
+			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
+				addr, value, sizeof(type));		\
+}									\
+									\
+void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
+{									\
+	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
+			arm64_extio_ops->end < addr)			\
+		reads##bw(PCI_IOBASE + addr, buffer, count);		\
+	else								\
+		if (arm64_extio_ops->pfins)				\
+			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
+				addr, buffer, sizeof(type), count);	\
+}									\
+									\
+void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
+{									\
+	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
+			arm64_extio_ops->end < addr)			\
+		writes##bw(PCI_IOBASE + addr, buffer, count);		\
+	else								\
+		if (arm64_extio_ops->pfouts)				\
+			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
+				addr, buffer, sizeof(type), count);	\
+}
+
+static inline void arm64_set_extops(struct extio_ops *ops)
+{
+	if (ops)
+		WRITE_ONCE(arm64_extio_ops, ops);
+}
+
+#endif /* __LINUX_EXTIO_H*/
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 0bba427..136735d 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -31,6 +31,7 @@
 #include <asm/early_ioremap.h>
 #include <asm/alternative.h>
 #include <asm/cpufeature.h>
+#include <asm/extio.h>
 
 #include <xen/xen.h>
 
@@ -149,6 +150,34 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
 #define IO_SPACE_LIMIT		(PCI_IO_SIZE - 1)
 #define PCI_IOBASE		((void __iomem *)PCI_IO_START)
 
+
+/*
+ * redefine the in(s)b/out(s)b for indirect-IO.
+ */
+#ifdef CONFIG_ARM64_INDIRECT_PIO
+#define inb inb
+#define outb outb
+#define insb insb
+#define outsb outsb
+/* external declaration */
+DECLARE_EXTIO(b, u8)
+
+#define inw inw
+#define outw outw
+#define insw insw
+#define outsw outsw
+
+DECLARE_EXTIO(w, u16)
+
+#define inl inl
+#define outl outl
+#define insl insl
+#define outsl outsl
+
+DECLARE_EXTIO(l, u32)
+#endif
+
+
 /*
  * String version of I/O memory access operations.
  */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 7d66bba..60e0482 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -31,6 +31,7 @@ arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
 					   sys_compat.o entry32.o
 arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
 arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
+arm64-obj-$(CONFIG_ARM64_INDIRECT_PIO)	+= extio.o
 arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)	+= module-plts.o
 arm64-obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o perf_callchain.o
 arm64-obj-$(CONFIG_HW_PERF_EVENTS)	+= perf_event.o
diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
new file mode 100644
index 0000000..647b3fa
--- /dev/null
+++ b/arch/arm64/kernel/extio.c
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/io.h>
+
+struct extio_ops *arm64_extio_ops;
+
+
+BUILD_EXTIO(b, u8)
+
+BUILD_EXTIO(w, u16)
+
+BUILD_EXTIO(l, u32)
-- 
1.9.1

[toc] | [next] | [standalone]

#1517118

On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> For arm64, there is no I/O space as other architectural platforms, such as
> X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
> such as Hip06, when accessing some legacy ISA devices connected to LPC, those
> known port addresses are used to control the corresponding target devices, for
> example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
> normal MMIO mode in using.

This has nothing to do with arm64. Hardware with this kind of indirect
bus access could be integrated with a variety of CPU architectures. It
simply hasn't been, yet.

> To drive these devices, this patch introduces a method named indirect-IO.
> In this method the in/out pair in arch/arm64/include/asm/io.h will be
> redefined. When upper layer drivers call in/out with those known legacy port
> addresses to access the peripherals, the hooking functions corrresponding to
> those target peripherals will be called. Through this way, those upper layer
> drivers which depend on in/out can run on Hip06 without any changes.

As above, this has nothing to do with arm64, and as such, should live in
generic code, exactly as we would do if we had higher-level ISA
accessor ops.

Regardless, given the multi-instance case, I don't think this is
sufficient in general (and I think we need higher-level ISA accessors
to handle the indirection).

[...]

> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
> new file mode 100644
> index 0000000..6ae0787
> --- /dev/null
> +++ b/arch/arm64/include/asm/extio.h

> +#ifndef __LINUX_EXTIO_H
> +#define __LINUX_EXTIO_H

This doesn't match the file naming, __ASM_EXTIO_H would be consistent
with other arm64 headers.

> +
> +struct extio_ops {
> +	unsigned long start;/* inclusive, sys io addr */
> +	unsigned long end;/* inclusive, sys io addr */

Please put whitespace before inline comments.

[...]

> +type in##bw(unsigned long addr)						\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		return read##bw(PCI_IOBASE + addr);			\
> +	return arm64_extio_ops->pfin ?					\
> +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
> +			addr, sizeof(type)) : -1;			\
> +}									\
> +									\
> +void out##bw(type value, unsigned long addr)				\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		write##bw(value, PCI_IOBASE + addr);			\
> +	else								\
> +		if (arm64_extio_ops->pfout)				\
> +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> +				addr, value, sizeof(type));		\
> +}									\
> +									\
> +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
> +	else								\
> +		if (arm64_extio_ops->pfins)				\
> +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
> +				addr, buffer, sizeof(type), count);	\
> +}									\
> +									\
> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
> +	else								\
> +		if (arm64_extio_ops->pfouts)				\
> +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
> +				addr, buffer, sizeof(type), count);	\
> +}
> +

So all PCI I/O will be slowed down by irrelevant checks when this is
enabled?

[...]

> +static inline void arm64_set_extops(struct extio_ops *ops)
> +{
> +	if (ops)
> +		WRITE_ONCE(arm64_extio_ops, ops);
> +}

Why WRITE_ONCE()?

Is this not protected/propagated by some synchronisation mechanism?

WRITE_ONCE() is not sufficient to ensure that this is consistently
observed by readers, and regardless, I don't see READ_ONCE() anywhere in
this patch.

This looks very suspicious.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1517334

On Tuesday, November 8, 2016 12:03:23 PM CET Mark Rutland wrote:
> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> > For arm64, there is no I/O space as other architectural platforms, such as
> > X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
> > such as Hip06, when accessing some legacy ISA devices connected to LPC, those
> > known port addresses are used to control the corresponding target devices, for
> > example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
> > normal MMIO mode in using.
> 
> This has nothing to do with arm64. Hardware with this kind of indirect
> bus access could be integrated with a variety of CPU architectures. It
> simply hasn't been, yet.

Actually PowerPC has a vaguely similar mechanism.

> > To drive these devices, this patch introduces a method named indirect-IO.
> > In this method the in/out pair in arch/arm64/include/asm/io.h will be
> > redefined. When upper layer drivers call in/out with those known legacy port
> > addresses to access the peripherals, the hooking functions corrresponding to
> > those target peripherals will be called. Through this way, those upper layer
> > drivers which depend on in/out can run on Hip06 without any changes.
> 
> As above, this has nothing to do with arm64, and as such, should live in
> generic code, exactly as we would do if we had higher-level ISA
> accessor ops.
> 
> Regardless, given the multi-instance case, I don't think this is
> sufficient in general (and I think we need higher-level ISA accessors
> to handle the indirection).

I think it is rather unlikely that we have to deal with multiple
instances in the future, it's more likely that future platforms
won't have any I/O ports at all, which is why I was advocating for
simplicity here.

> > +type in##bw(unsigned long addr)						\
> > +{									\
> > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > +			arm64_extio_ops->end < addr)			\
> > +		return read##bw(PCI_IOBASE + addr);			\
> > +	return arm64_extio_ops->pfin ?					\
> > +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
> > +			addr, sizeof(type)) : -1;			\
> > +}									\
> > +									\
> > +void out##bw(type value, unsigned long addr)				\
> > +{									\
> > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > +			arm64_extio_ops->end < addr)			\
> > +		write##bw(value, PCI_IOBASE + addr);			\
> > +	else								\
> > +		if (arm64_extio_ops->pfout)				\
> > +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > +				addr, value, sizeof(type));		\
> > +}									\
> > +									\
> > +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
> > +{									\
> > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > +			arm64_extio_ops->end < addr)			\
> > +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
> > +	else								\
> > +		if (arm64_extio_ops->pfins)				\
> > +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
> > +				addr, buffer, sizeof(type), count);	\
> > +}									\
> > +									\
> > +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
> > +{									\
> > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > +			arm64_extio_ops->end < addr)			\
> > +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
> > +	else								\
> > +		if (arm64_extio_ops->pfouts)				\
> > +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
> > +				addr, buffer, sizeof(type), count);	\
> > +}
> > +
> 
> So all PCI I/O will be slowed down by irrelevant checks when this is
> enabled?

I don't see a better alternative. I earlier suggested having these
out of line so we don't grow the object code too much when it is
enabled.

Performance of PIO accessors is not an issue here though, any bus
access will by definition be orders of magnitude slower than the
added branches and dereferences here.

> [...]
> 
> > +static inline void arm64_set_extops(struct extio_ops *ops)
> > +{
> > +	if (ops)
> > +		WRITE_ONCE(arm64_extio_ops, ops);
> > +}
> 
> Why WRITE_ONCE()?
> 
> Is this not protected/propagated by some synchronisation mechanism?
> 
> WRITE_ONCE() is not sufficient to ensure that this is consistently
> observed by readers, and regardless, I don't see READ_ONCE() anywhere in
> this patch.
> 
> This looks very suspicious.

Agreed, this looks wrong.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1517344

On Tuesday, November 8, 2016 5:09:59 PM CET Arnd Bergmann wrote:
> 
> I don't see a better alternative. I earlier suggested having these
> out of line so we don't grow the object code too much when it is
> enabled.
> 

On second look, I see that they are all done out of line, I would
just move around the BUILD_EXTIO macro to the file that uses it
and remove and open-code the DECLARE_EXTIO() as that makes it easier
to grep.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1517640

On Tue, 2016-11-08 at 12:03 +0000, Mark Rutland wrote:
> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> > 
> > For arm64, there is no I/O space as other architectural platforms, such as
> > X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
> > such as Hip06, when accessing some legacy ISA devices connected to LPC, those
> > known port addresses are used to control the corresponding target devices, for
> > example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
> > normal MMIO mode in using.
> 
> This has nothing to do with arm64. Hardware with this kind of indirect
> bus access could be integrated with a variety of CPU architectures. It
> simply hasn't been, yet.

On some ppc's we also use similar indirect access methods for IOs. We
have a generic infrastructure for re-routing some memory or IO regions
to hooks.

On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
firmware calls ;-) We use that infrastructure to plumb in the LPC bus.

> > To drive these devices, this patch introduces a method named indirect-IO.
> > In this method the in/out pair in arch/arm64/include/asm/io.h will be
> > redefined. When upper layer drivers call in/out with those known legacy port
> > addresses to access the peripherals, the hooking functions corrresponding to
> > those target peripherals will be called. Through this way, those upper layer
> > drivers which depend on in/out can run on Hip06 without any changes.
> 
> As above, this has nothing to do with arm64, and as such, should live in
> generic code, exactly as we would do if we had higher-level ISA
> accessor ops.
> 
> Regardless, given the multi-instance case, I don't think this is
> sufficient in general (and I think we need higher-level ISA accessors
> to handle the indirection).

Multi-instance with IO is tricky to do generically because archs already
have all sort of hacks to deal with the fact that inb/outb don't require
an explicit ioremap, so an IO resource can take all sort of shape depending
on the arch.

Overall it boils down to applying some kind of per-instance "offset" to
the IO port number though.

> [...]
> 
> > 
> > diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
> > new file mode 100644
> > index 0000000..6ae0787
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/extio.h
> 
> > 
> > +#ifndef __LINUX_EXTIO_H
> > +#define __LINUX_EXTIO_H
> 
> This doesn't match the file naming, __ASM_EXTIO_H would be consistent
> with other arm64 headers.
> 
> > 
> > +
> > +struct extio_ops {
> > > > +	unsigned long start;/* inclusive, sys io addr */
> > > > +	unsigned long end;/* inclusive, sys io addr */
> 
> Please put whitespace before inline comments.
> 
> [...]
> 
> > 
> > > > +type in##bw(unsigned long addr)						\
> > > > +{									\
> > > > > > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > > > > > +			arm64_extio_ops->end < addr)			\
> > > > > > +		return read##bw(PCI_IOBASE + addr);			\
> > > > > > +	return arm64_extio_ops->pfin ?					\
> > > > > > +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
> > > > > > +			addr, sizeof(type)) : -1;			\
> > > > +}									\
> > > > +									\
> > > > +void out##bw(type value, unsigned long addr)				\
> > > > +{									\
> > > > > > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > > > > > +			arm64_extio_ops->end < addr)			\
> > > > > > +		write##bw(value, PCI_IOBASE + addr);			\
> > > > > > +	else								\
> > > > > > +		if (arm64_extio_ops->pfout)				\
> > > > +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > > > > > +				addr, value, sizeof(type));		\
> > > > +}									\
> > > > +									\
> > > > +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
> > > > +{									\
> > > > > > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > > > > > +			arm64_extio_ops->end < addr)			\
> > > > > > +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
> > > > > > +	else								\
> > > > > > +		if (arm64_extio_ops->pfins)				\
> > > > +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
> > > > > > +				addr, buffer, sizeof(type), count);	\
> > > > +}									\
> > > > +									\
> > > > +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
> > > > +{									\
> > > > > > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > > > > > +			arm64_extio_ops->end < addr)			\
> > > > > > +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
> > > > > > +	else								\
> > > > > > +		if (arm64_extio_ops->pfouts)				\
> > > > +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
> > > > > > +				addr, buffer, sizeof(type), count);	\
> > +}
> > +
> 
> So all PCI I/O will be slowed down by irrelevant checks when this is
> enabled?
> 
> [...]
> 
> > 
> > +static inline void arm64_set_extops(struct extio_ops *ops)
> > +{
> > > > +	if (ops)
> > > > +		WRITE_ONCE(arm64_extio_ops, ops);
> > +}
> 
> Why WRITE_ONCE()?
> 
> Is this not protected/propagated by some synchronisation mechanism?
> 
> WRITE_ONCE() is not sufficient to ensure that this is consistently
> observed by readers, and regardless, I don't see READ_ONCE() anywhere in
> this patch.
> 
> This looks very suspicious.
> 
> Thanks,
> Mark.

[toc] | [prev] | [next] | [standalone]

#1518739

Hi, Ben,


On 2016/11/9 7:16, Benjamin Herrenschmidt wrote:
> On Tue, 2016-11-08 at 12:03 +0000, Mark Rutland wrote:
>> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
>>>
>>> For arm64, there is no I/O space as other architectural platforms, such as
>>> X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
>>> such as Hip06, when accessing some legacy ISA devices connected to LPC, those
>>> known port addresses are used to control the corresponding target devices, for
>>> example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
>>> normal MMIO mode in using.
>>
>> This has nothing to do with arm64. Hardware with this kind of indirect
>> bus access could be integrated with a variety of CPU architectures. It
>> simply hasn't been, yet.
> 
> On some ppc's we also use similar indirect access methods for IOs. We
> have a generic infrastructure for re-routing some memory or IO regions
> to hooks.
> 
I am interested on the generic infrastructure on PPC.
Could you point out where those drivers are?
want to take a look..

Thanks,
Zhichang

> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
> firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
> 
>>> To drive these devices, this patch introduces a method named indirect-IO.
>>> In this method the in/out pair in arch/arm64/include/asm/io.h will be
>>> redefined. When upper layer drivers call in/out with those known legacy port
>>> addresses to access the peripherals, the hooking functions corrresponding to
>>> those target peripherals will be called. Through this way, those upper layer
>>> drivers which depend on in/out can run on Hip06 without any changes.
>>
>> As above, this has nothing to do with arm64, and as such, should live in
>> generic code, exactly as we would do if we had higher-level ISA
>> accessor ops.
>>
>> Regardless, given the multi-instance case, I don't think this is
>> sufficient in general (and I think we need higher-level ISA accessors
>> to handle the indirection).
> 
> Multi-instance with IO is tricky to do generically because archs already
> have all sort of hacks to deal with the fact that inb/outb don't require
> an explicit ioremap, so an IO resource can take all sort of shape depending
> on the arch.
> 
> Overall it boils down to applying some kind of per-instance "offset" to
> the IO port number though.
> 

[toc] | [prev] | [next] | [standalone]

#1518878

On Wed, Nov 09, 2016 at 10:16:42AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2016-11-08 at 12:03 +0000, Mark Rutland wrote:
> > On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> > > 
> > > For arm64, there is no I/O space as other architectural platforms, such as
> > > X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
> > > such as Hip06, when accessing some legacy ISA devices connected to LPC, those
> > > known port addresses are used to control the corresponding target devices, for
> > > example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
> > > normal MMIO mode in using.
> > 
> > This has nothing to do with arm64. Hardware with this kind of indirect
> > bus access could be integrated with a variety of CPU architectures. It
> > simply hasn't been, yet.
> 
> On some ppc's we also use similar indirect access methods for IOs. We
> have a generic infrastructure for re-routing some memory or IO regions
> to hooks.
> 
> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
> firmware calls ;-) We use that infrastructure to plumb in the LPC bus.

Just to check, do you hook that in your inb/outb/etc?

Generally, it would seem nicer if we could have higher-level
isa_{inb,outb,whatever} accessors that we could hook separately from
other IO.

We don't necessarily have to move all ISA drivers over to that if we had
a separate symbol for that interface.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1519316

On Thu, 2016-11-10 at 11:22 +0000, Mark Rutland wrote:
> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
> > firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
> 
> Just to check, do you hook that in your inb/outb/etc?

Yes.

> Generally, it would seem nicer if we could have higher-level
> isa_{inb,outb,whatever} accessors that we could hook separately from
> other IO.

Maybe but generally speaking, we don't discriminate accessors per bus,
ie, readl etc... work on all memory mapped busses, inb... works on all
busses with an "IO space", at least that's been the idea. It probably
all comes from the fact that PCI IO and ISA are the same space on
x86 and most other platforms (not all).

> We don't necessarily have to move all ISA drivers over to that if we had
> a separate symbol for that interface.

What I do on ppc today is that I have a chunk of virtual address space
that is reserved for "IO space". The first 64k are "reserved" in that
they route to "the primary" ISA bus (for legacy crap that uses hard
coded addresses, though I use that for my LPC bus too). I "allocate"
space for the PCI IO spaces higher in that space. Was I to support more
LPC busses I could allocate them up there too.

The IO resource of a given device thus becomes the actual IO port plus
the offset of the base of the segment it's in.

For memory mapped IO, inb/outb will just add the virtual address of
the base of all IO space to that. The hooking mechanism will pickup
the stuff that isn't memory mapped.

It's a bit messy but then IO space performance has never been a huge
worry since IO cycles tend to be very slow to begin with.

Note: We also have the ISA memory and ISA FW spaces that we don't have
good accessors for. They somewhat exist (I think the fbdev layer uses
some for vga) but it's messy.

Cheers,
Ben.

[toc] | [prev] | [next] | [standalone]

#1519659

Hi, Ben, Mark,

Thanks for your comments! These are helpful!


On 2016/11/11 3:32, Benjamin Herrenschmidt wrote:
> On Thu, 2016-11-10 at 11:22 +0000, Mark Rutland wrote:
>> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
>>> firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
>>
>> Just to check, do you hook that in your inb/outb/etc?
> 
> Yes.
> 
>> Generally, it would seem nicer if we could have higher-level
>> isa_{inb,outb,whatever} accessors that we could hook separately from
>> other IO.
> 
> Maybe but generally speaking, we don't discriminate accessors per bus,
> ie, readl etc... work on all memory mapped busses, inb... works on all
> busses with an "IO space", at least that's been the idea. It probably
> all comes from the fact that PCI IO and ISA are the same space on
> x86 and most other platforms (not all).
> 
>> We don't necessarily have to move all ISA drivers over to that if we had
>> a separate symbol for that interface.
> 
> What I do on ppc today is that I have a chunk of virtual address space
> that is reserved for "IO space". The first 64k are "reserved" in that
> they route to "the primary" ISA bus (for legacy crap that uses hard
> coded addresses, though I use that for my LPC bus too). I "allocate"
> space for the PCI IO spaces higher in that space. Was I to support more
> LPC busses I could allocate them up there too.
>

I have similar idea as your PPC MMIO.

We notice the prototype of {in/out()} is something like that:

static inline u8 inb(unsigned long addr)
static inline void outb(u8 value, unsigned long addr)

The type of parameter 'addr' is unsigned long. For I/O space, it is big enough.
So, could you divide this 'addr' into several bit segments? The top 8 bits is
defined as bus index. For normal direct IO, the bus index is 0. For those bus
device which need indirectIO or some special I/O accessors, when these devices
are initializing, can request to allocate an unique ID to them, and register
their own accessors to the entry which is corresponding to the ID.

In this way, we can support multiple domains, I think.
But I am not sure whether it is feasible, for example, are there some
architectures/platforms had populated the top 8 bits? Do we need to request IO
region from ioport_resource for those devices?  etc...


Thanks,
Zhichang


> The IO resource of a given device thus becomes the actual IO port plus
> the offset of the base of the segment it's in.
> 
> For memory mapped IO, inb/outb will just add the virtual address of
> the base of all IO space to that. The hooking mechanism will pickup
> the stuff that isn't memory mapped.
> 
> It's a bit messy but then IO space performance has never been a huge
> worry since IO cycles tend to be very slow to begin with.
> 
> Note: We also have the ISA memory and ISA FW spaces that we don't have
> good accessors for. They somewhat exist (I think the fbdev layer uses
> some for vga) but it's messy.
> 
> Cheers,
> Ben.
> 
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1517337

On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> For arm64, there is no I/O space as other architectural platforms, such as
> X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
> such as Hip06, when accessing some legacy ISA devices connected to LPC, those
> known port addresses are used to control the corresponding target devices, for
> example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
> normal MMIO mode in using.
> 
> To drive these devices, this patch introduces a method named indirect-IO.
> In this method the in/out pair in arch/arm64/include/asm/io.h will be
> redefined. When upper layer drivers call in/out with those known legacy port
> addresses to access the peripherals, the hooking functions corrresponding to
> those target peripherals will be called. Through this way, those upper layer
> drivers which depend on in/out can run on Hip06 without any changes.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  arch/arm64/Kconfig             |  6 +++
>  arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/include/asm/io.h    | 29 +++++++++++++
>  arch/arm64/kernel/Makefile     |  1 +
>  arch/arm64/kernel/extio.c      | 27 ++++++++++++
>  5 files changed, 157 insertions(+)
>  create mode 100644 arch/arm64/include/asm/extio.h
>  create mode 100644 arch/arm64/kernel/extio.c
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 969ef88..b44070b 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -163,6 +163,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
>         default 16
>  
> +config ARM64_INDIRECT_PIO
> +	bool "access peripherals with legacy I/O port"
> +	help
> +	  Support special accessors for ISA I/O devices. This is needed for
> +	  SoCs that do not support standard read/write for the ISA range.
> +
>  config NO_IOPORT_MAP
>  	def_bool y if !PCI
>  
> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
> new file mode 100644
> index 0000000..6ae0787
> --- /dev/null
> +++ b/arch/arm64/include/asm/extio.h
> @@ -0,0 +1,94 @@
> +/*
> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __LINUX_EXTIO_H
> +#define __LINUX_EXTIO_H
> +
> +struct extio_ops {
> +	unsigned long start;/* inclusive, sys io addr */
> +	unsigned long end;/* inclusive, sys io addr */
> +
> +	u64 (*pfin)(void *devobj, unsigned long ptaddr,	size_t dlen);
> +	void (*pfout)(void *devobj, unsigned long ptaddr, u32 outval,
> +					size_t dlen);
> +	u64 (*pfins)(void *devobj, unsigned long ptaddr, void *inbuf,
> +				size_t dlen, unsigned int count);
> +	void (*pfouts)(void *devobj, unsigned long ptaddr,
> +				const void *outbuf, size_t dlen,
> +				unsigned int count);
> +	void *devpara;
> +};
> +
> +extern struct extio_ops *arm64_extio_ops;
> +
> +#define DECLARE_EXTIO(bw, type)						\
> +extern type in##bw(unsigned long addr);					\
> +extern void out##bw(type value, unsigned long addr);			\
> +extern void ins##bw(unsigned long addr, void *buffer, unsigned int count);\
> +extern void outs##bw(unsigned long addr, const void *buffer, unsigned int count);
> +
> +#define BUILD_EXTIO(bw, type)						\
> +type in##bw(unsigned long addr)						\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		return read##bw(PCI_IOBASE + addr);			\
> +	return arm64_extio_ops->pfin ?					\
> +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
> +			addr, sizeof(type)) : -1;			\
> +}									\
> +									\
> +void out##bw(type value, unsigned long addr)				\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		write##bw(value, PCI_IOBASE + addr);			\
> +	else								\
> +		if (arm64_extio_ops->pfout)				\
> +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> +				addr, value, sizeof(type));		\
> +}									\
> +									\
> +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
> +	else								\
> +		if (arm64_extio_ops->pfins)				\
> +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
> +				addr, buffer, sizeof(type), count);	\
> +}									\
> +									\
> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
> +	else								\
> +		if (arm64_extio_ops->pfouts)				\
> +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
> +				addr, buffer, sizeof(type), count);	\
> +}
> +
> +static inline void arm64_set_extops(struct extio_ops *ops)
> +{
> +	if (ops)
> +		WRITE_ONCE(arm64_extio_ops, ops);

Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
side. Also, what if multiple drivers want to set different ops for distinct
address ranges?

> +}
> +
> +#endif /* __LINUX_EXTIO_H*/
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 0bba427..136735d 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -31,6 +31,7 @@
>  #include <asm/early_ioremap.h>
>  #include <asm/alternative.h>
>  #include <asm/cpufeature.h>
> +#include <asm/extio.h>
>  
>  #include <xen/xen.h>
>  
> @@ -149,6 +150,34 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>  #define IO_SPACE_LIMIT		(PCI_IO_SIZE - 1)
>  #define PCI_IOBASE		((void __iomem *)PCI_IO_START)
>  
> +
> +/*
> + * redefine the in(s)b/out(s)b for indirect-IO.
> + */
> +#ifdef CONFIG_ARM64_INDIRECT_PIO
> +#define inb inb
> +#define outb outb
> +#define insb insb
> +#define outsb outsb
> +/* external declaration */
> +DECLARE_EXTIO(b, u8)
> +
> +#define inw inw
> +#define outw outw
> +#define insw insw
> +#define outsw outsw
> +
> +DECLARE_EXTIO(w, u16)
> +
> +#define inl inl
> +#define outl outl
> +#define insl insl
> +#define outsl outsl
> +
> +DECLARE_EXTIO(l, u32)
> +#endif
> +
> +
>  /*
>   * String version of I/O memory access operations.
>   */
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 7d66bba..60e0482 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -31,6 +31,7 @@ arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
>  					   sys_compat.o entry32.o
>  arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
>  arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
> +arm64-obj-$(CONFIG_ARM64_INDIRECT_PIO)	+= extio.o
>  arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)	+= module-plts.o
>  arm64-obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o perf_callchain.o
>  arm64-obj-$(CONFIG_HW_PERF_EVENTS)	+= perf_event.o
> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> new file mode 100644
> index 0000000..647b3fa
> --- /dev/null
> +++ b/arch/arm64/kernel/extio.c
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/io.h>
> +
> +struct extio_ops *arm64_extio_ops;
> +
> +
> +BUILD_EXTIO(b, u8)
> +
> +BUILD_EXTIO(w, u16)
> +
> +BUILD_EXTIO(l, u32)

Is there no way to make this slightly more generic, so that it can be
re-used elsewhere? For example, if struct extio_ops was common, then
you could have the singleton (which maybe should be an interval tree?),
type definition, setter function and the BUILD_EXTIO invocations
somewhere generic, rather than squirelled away in the arch backend.

Will

[toc] | [prev] | [next] | [standalone]

#1517376

On 08/11/2016 16:12, Will Deacon wrote:
> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
>> For arm64, there is no I/O space as other architectural platforms, such as
>> X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
>> such as Hip06, when accessing some legacy ISA devices connected to LPC, those
>> known port addresses are used to control the corresponding target devices, for
>> example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
>> normal MMIO mode in using.
>>
>> To drive these devices, this patch introduces a method named indirect-IO.
>> In this method the in/out pair in arch/arm64/include/asm/io.h will be
>> redefined. When upper layer drivers call in/out with those known legacy port
>> addresses to access the peripherals, the hooking functions corrresponding to
>> those target peripherals will be called. Through this way, those upper layer
>> drivers which depend on in/out can run on Hip06 without any changes.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>> ---
>>  arch/arm64/Kconfig             |  6 +++
>>  arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
>>  arch/arm64/include/asm/io.h    | 29 +++++++++++++
>>  arch/arm64/kernel/Makefile     |  1 +
>>  arch/arm64/kernel/extio.c      | 27 ++++++++++++
>>  5 files changed, 157 insertions(+)
>>  create mode 100644 arch/arm64/include/asm/extio.h
>>  create mode 100644 arch/arm64/kernel/extio.c
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 969ef88..b44070b 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -163,6 +163,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
>>         default 16
>>
>> +config ARM64_INDIRECT_PIO
>> +	bool "access peripherals with legacy I/O port"
>> +	help
>> +	  Support special accessors for ISA I/O devices. This is needed for
>> +	  SoCs that do not support standard read/write for the ISA range.
>> +
>>  config NO_IOPORT_MAP
>>  	def_bool y if !PCI
>>
>> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
>> new file mode 100644
>> index 0000000..6ae0787
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/extio.h
>> @@ -0,0 +1,94 @@
>> +/*
>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __LINUX_EXTIO_H
>> +#define __LINUX_EXTIO_H
>> +
>> +struct extio_ops {
>> +	unsigned long start;/* inclusive, sys io addr */
>> +	unsigned long end;/* inclusive, sys io addr */
>> +
>> +	u64 (*pfin)(void *devobj, unsigned long ptaddr,	size_t dlen);
>> +	void (*pfout)(void *devobj, unsigned long ptaddr, u32 outval,
>> +					size_t dlen);
>> +	u64 (*pfins)(void *devobj, unsigned long ptaddr, void *inbuf,
>> +				size_t dlen, unsigned int count);
>> +	void (*pfouts)(void *devobj, unsigned long ptaddr,
>> +				const void *outbuf, size_t dlen,
>> +				unsigned int count);
>> +	void *devpara;
>> +};
>> +
>> +extern struct extio_ops *arm64_extio_ops;
>> +
>> +#define DECLARE_EXTIO(bw, type)						\
>> +extern type in##bw(unsigned long addr);					\
>> +extern void out##bw(type value, unsigned long addr);			\
>> +extern void ins##bw(unsigned long addr, void *buffer, unsigned int count);\
>> +extern void outs##bw(unsigned long addr, const void *buffer, unsigned int count);
>> +
>> +#define BUILD_EXTIO(bw, type)						\
>> +type in##bw(unsigned long addr)						\
>> +{									\
>> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
>> +			arm64_extio_ops->end < addr)			\
>> +		return read##bw(PCI_IOBASE + addr);			\
>> +	return arm64_extio_ops->pfin ?					\
>> +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
>> +			addr, sizeof(type)) : -1;			\
>> +}									\
>> +									\
>> +void out##bw(type value, unsigned long addr)				\
>> +{									\
>> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
>> +			arm64_extio_ops->end < addr)			\
>> +		write##bw(value, PCI_IOBASE + addr);			\
>> +	else								\
>> +		if (arm64_extio_ops->pfout)				\
>> +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>> +				addr, value, sizeof(type));		\
>> +}									\
>> +									\
>> +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
>> +{									\
>> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
>> +			arm64_extio_ops->end < addr)			\
>> +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
>> +	else								\
>> +		if (arm64_extio_ops->pfins)				\
>> +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
>> +				addr, buffer, sizeof(type), count);	\
>> +}									\
>> +									\
>> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
>> +{									\
>> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
>> +			arm64_extio_ops->end < addr)			\
>> +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
>> +	else								\
>> +		if (arm64_extio_ops->pfouts)				\
>> +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
>> +				addr, buffer, sizeof(type), count);	\
>> +}
>> +
>> +static inline void arm64_set_extops(struct extio_ops *ops)
>> +{
>> +	if (ops)
>> +		WRITE_ONCE(arm64_extio_ops, ops);
>
> Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
> side. Also, what if multiple drivers want to set different ops for distinct
> address ranges?

I think that the idea here is that we only have possibly one master in 
the system which offers indirectIO backend, so another one could not 
possibly re-set this value.

>
>> +}
>> +
>> +#endif /* __LINUX_EXTIO_H*/
>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>> index 0bba427..136735d 100644
>> --- a/arch/arm64/include/asm/io.h
>> +++ b/arch/arm64/include/asm/io.h
>> @@ -31,6 +31,7 @@
>>  #include <asm/early_ioremap.h>
>>  #include <asm/alternative.h>
>>  #include <asm/cpufeature.h>
>> +#include <asm/extio.h>
>>
>>  #include <xen/xen.h>
>>
>> @@ -149,6 +150,34 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>>  #define IO_SPACE_LIMIT		(PCI_IO_SIZE - 1)
>>  #define PCI_IOBASE		((void __iomem *)PCI_IO_START)
>>
>> +
>> +/*
>> + * redefine the in(s)b/out(s)b for indirect-IO.
>> + */
>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>> +#define inb inb
>> +#define outb outb
>> +#define insb insb
>> +#define outsb outsb
>> +/* external declaration */
>> +DECLARE_EXTIO(b, u8)
>> +
>> +#define inw inw
>> +#define outw outw
>> +#define insw insw
>> +#define outsw outsw
>> +
>> +DECLARE_EXTIO(w, u16)
>> +
>> +#define inl inl
>> +#define outl outl
>> +#define insl insl
>> +#define outsl outsl
>> +
>> +DECLARE_EXTIO(l, u32)
>> +#endif
>> +
>> +
>>  /*
>>   * String version of I/O memory access operations.
>>   */
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index 7d66bba..60e0482 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -31,6 +31,7 @@ arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
>>  					   sys_compat.o entry32.o
>>  arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
>>  arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
>> +arm64-obj-$(CONFIG_ARM64_INDIRECT_PIO)	+= extio.o
>>  arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)	+= module-plts.o
>>  arm64-obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o perf_callchain.o
>>  arm64-obj-$(CONFIG_HW_PERF_EVENTS)	+= perf_event.o
>> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
>> new file mode 100644
>> index 0000000..647b3fa
>> --- /dev/null
>> +++ b/arch/arm64/kernel/extio.c
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/io.h>
>> +
>> +struct extio_ops *arm64_extio_ops;
>> +
>> +
>> +BUILD_EXTIO(b, u8)
>> +
>> +BUILD_EXTIO(w, u16)
>> +
>> +BUILD_EXTIO(l, u32)
>
> Is there no way to make this slightly more generic, so that it can be
> re-used elsewhere? For example, if struct extio_ops was common, then
> you could have the singleton (which maybe should be an interval tree?),
> type definition, setter function and the BUILD_EXTIO invocations
> somewhere generic, rather than squirelled away in the arch backend.
>
> Will

The concern would be that some architecture which uses generic 
higher-level ISA accessor ops, but have IO space, could be affected.

John

>
> .
>

[toc] | [prev] | [next] | [standalone]

#1517377

On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
> On 08/11/2016 16:12, Will Deacon wrote:
> >On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> >>+static inline void arm64_set_extops(struct extio_ops *ops)
> >>+{
> >>+	if (ops)
> >>+		WRITE_ONCE(arm64_extio_ops, ops);
> >
> >Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
> >side. Also, what if multiple drivers want to set different ops for distinct
> >address ranges?
> 
> I think that the idea here is that we only have possibly one master in the
> system which offers indirectIO backend, so another one could not possibly
> re-set this value.

Why is that assumption valid, and why does WRITE_ONCE help there? It's not
ONCE as in WARN_ONCE, more ONCE as in exactly-once-per-invocation.

> >>diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> >>new file mode 100644
> >>index 0000000..647b3fa
> >>--- /dev/null
> >>+++ b/arch/arm64/kernel/extio.c
> >>@@ -0,0 +1,27 @@
> >>+/*
> >>+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> >>+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify
> >>+ * it under the terms of the GNU General Public License version 2 as
> >>+ * published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>+ * GNU General Public License for more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License
> >>+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >>+ */
> >>+
> >>+#include <linux/io.h>
> >>+
> >>+struct extio_ops *arm64_extio_ops;
> >>+
> >>+
> >>+BUILD_EXTIO(b, u8)
> >>+
> >>+BUILD_EXTIO(w, u16)
> >>+
> >>+BUILD_EXTIO(l, u32)
> >
> >Is there no way to make this slightly more generic, so that it can be
> >re-used elsewhere? For example, if struct extio_ops was common, then
> >you could have the singleton (which maybe should be an interval tree?),
> >type definition, setter function and the BUILD_EXTIO invocations
> >somewhere generic, rather than squirelled away in the arch backend.
> >
> The concern would be that some architecture which uses generic higher-level
> ISA accessor ops, but have IO space, could be affected.

You're already adding a Kconfig symbol for this stuff, so you can keep
that if you don't want it on other architectures. I'm just arguing that
plumbing drivers directly into arch code via arm64_set_extops is not
something I'm particularly fond of, especially when it looks like it
could be avoided with a small amount of effort.

Will

[toc] | [prev] | [next] | [standalone]

#1517392

On 08/11/2016 16:49, Will Deacon wrote:
> On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
>> On 08/11/2016 16:12, Will Deacon wrote:
>>> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
>>>> +static inline void arm64_set_extops(struct extio_ops *ops)
>>>> +{
>>>> +	if (ops)
>>>> +		WRITE_ONCE(arm64_extio_ops, ops);
>>>
>>> Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
>>> side. Also, what if multiple drivers want to set different ops for distinct
>>> address ranges?
>>
>> I think that the idea here is that we only have possibly one master in the
>> system which offers indirectIO backend, so another one could not possibly
>> re-set this value.
>
> Why is that assumption valid, and why does WRITE_ONCE help there? It's not
> ONCE as in WARN_ONCE, more ONCE as in exactly-once-per-invocation.

It's only valid based on the inherent assumption that all indirectIO is 
redirected to one backend master, i.e. LPC driver.

Anyway, right, I don't think that WRITE_ONCE is correct. Zhichang was 
looking for something which would only allow the pointer to be written 
once ever.

>
>>>> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
>>>> new file mode 100644
>>>> index 0000000..647b3fa
>>>> --- /dev/null
>>>> +++ b/arch/arm64/kernel/extio.c
>>>> @@ -0,0 +1,27 @@
>>>> +/*
>>>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>>>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License
>>>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#include <linux/io.h>
>>>> +
>>>> +struct extio_ops *arm64_extio_ops;
>>>> +
>>>> +
>>>> +BUILD_EXTIO(b, u8)
>>>> +
>>>> +BUILD_EXTIO(w, u16)
>>>> +
>>>> +BUILD_EXTIO(l, u32)
>>>
>>> Is there no way to make this slightly more generic, so that it can be
>>> re-used elsewhere? For example, if struct extio_ops was common, then
>>> you could have the singleton (which maybe should be an interval tree?),
>>> type definition, setter function and the BUILD_EXTIO invocations
>>> somewhere generic, rather than squirelled away in the arch backend.
>>>
>> The concern would be that some architecture which uses generic higher-level
>> ISA accessor ops, but have IO space, could be affected.
>
> You're already adding a Kconfig symbol for this stuff, so you can keep
> that if you don't want it on other architectures. I'm just arguing that
> plumbing drivers directly into arch code via arm64_set_extops is not
> something I'm particularly fond of, especially when it looks like it
> could be avoided with a small amount of effort.

We'll check this.

Cheers,
John

>
> Will
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1517618

On Tuesday, November 8, 2016 4:49:49 PM CET Will Deacon wrote:
> On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
> > On 08/11/2016 16:12, Will Deacon wrote:
> > >On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:

> > >Is there no way to make this slightly more generic, so that it can be
> > >re-used elsewhere? For example, if struct extio_ops was common, then
> > >you could have the singleton (which maybe should be an interval tree?),
> > >type definition, setter function and the BUILD_EXTIO invocations
> > >somewhere generic, rather than squirelled away in the arch backend.
> > >
> > The concern would be that some architecture which uses generic higher-level
> > ISA accessor ops, but have IO space, could be affected.
> 
> You're already adding a Kconfig symbol for this stuff, so you can keep
> that if you don't want it on other architectures. I'm just arguing that
> plumbing drivers directly into arch code via arm64_set_extops is not
> something I'm particularly fond of, especially when it looks like it
> could be avoided with a small amount of effort.

Agreed, I initially suggested putting this into arch/arm64/, but there isn't
really a reason why it couldn't just live in lib/ with the header file
bits moved to include/asm-generic/io.h which we already use.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1518072

On 08/11/2016 22:35, Arnd Bergmann wrote:
> On Tuesday, November 8, 2016 4:49:49 PM CET Will Deacon wrote:
>> On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
>>> On 08/11/2016 16:12, Will Deacon wrote:
>>>> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
>
>>>> Is there no way to make this slightly more generic, so that it can be
>>>> re-used elsewhere? For example, if struct extio_ops was common, then
>>>> you could have the singleton (which maybe should be an interval tree?),
>>>> type definition, setter function and the BUILD_EXTIO invocations
>>>> somewhere generic, rather than squirelled away in the arch backend.
>>>>
>>> The concern would be that some architecture which uses generic higher-level
>>> ISA accessor ops, but have IO space, could be affected.
>>
>> You're already adding a Kconfig symbol for this stuff, so you can keep
>> that if you don't want it on other architectures. I'm just arguing that
>> plumbing drivers directly into arch code via arm64_set_extops is not
>> something I'm particularly fond of, especially when it looks like it
>> could be avoided with a small amount of effort.
>
> Agreed, I initially suggested putting this into arch/arm64/, but there isn't
> really a reason why it couldn't just live in lib/ with the header file
> bits moved to include/asm-generic/io.h which we already use.
>

Right, Zhichang will check the logistics of this. The generic io.h is 
quite clean, so as long as you don't mind new build switches of this 
nature being added, it should be ok; and we'll plan on moving extio.h 
into include/asm-generic as well.

Cheers,
John

> 	Arnd
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1518506

On Wednesday, November 9, 2016 11:29:46 AM CET John Garry wrote:
> On 08/11/2016 22:35, Arnd Bergmann wrote:
> > On Tuesday, November 8, 2016 4:49:49 PM CET Will Deacon wrote:
> >> On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
> >>> On 08/11/2016 16:12, Will Deacon wrote:
> >>>> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> >
> >>>> Is there no way to make this slightly more generic, so that it can be
> >>>> re-used elsewhere? For example, if struct extio_ops was common, then
> >>>> you could have the singleton (which maybe should be an interval tree?),
> >>>> type definition, setter function and the BUILD_EXTIO invocations
> >>>> somewhere generic, rather than squirelled away in the arch backend.
> >>>>
> >>> The concern would be that some architecture which uses generic higher-level
> >>> ISA accessor ops, but have IO space, could be affected.
> >>
> >> You're already adding a Kconfig symbol for this stuff, so you can keep
> >> that if you don't want it on other architectures. I'm just arguing that
> >> plumbing drivers directly into arch code via arm64_set_extops is not
> >> something I'm particularly fond of, especially when it looks like it
> >> could be avoided with a small amount of effort.
> >
> > Agreed, I initially suggested putting this into arch/arm64/, but there isn't
> > really a reason why it couldn't just live in lib/ with the header file
> > bits moved to include/asm-generic/io.h which we already use.
> >
> 
> Right, Zhichang will check the logistics of this. The generic io.h is 
> quite clean, so as long as you don't mind new build switches of this 
> nature being added, it should be ok; and we'll plan on moving extio.h 
> into include/asm-generic as well.

I think all we need is an #ifdef CONFIG_something around the existing
defintion, with the alternative being "extern" declarations, after that
all the interesting logic can sit in a file in lib/.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web