[PATCH V5 0/3] ARM64 LPC: legacy ISA I/O support

[PATCH V5 0/3] ARM64 LPC: legacy ISA I/O support

[zhichang.yuan]

This patch supports the IPMI-bt device attached to the Low-Pin-Count interface
implemented on Hisilicon Hip06 SoC.
	                -----------
			| LPC host|
	                |         |
	                -----------
	                     |
 	        _____________V_______________LPC
                  |			  |
                  V	                  V
			             ------------
			             |  BT(ipmi)|
			             ------------

When master accesses those periperals beneath the Hip06 LPC, a specific LPC
driver is needed to make LPC host generate the standard LPC I/O cycles with
the target periperals'I/O port addresses. But on curent arm64 world, there is
no real I/O accesses. All the I/O operations through in/out pair are based on
MMIO which is not satisfied the I/O mechanism on Hip06 LPC.
To solve this issue and keep the relevant existing peripherals' driver
unchanged, this patch set redefines the in/out pair to support both the IO
operations for Hip06 LPC and the original MMIO. The way specific to Hip06 is
named as indirect-IO in this patchset.

Changes from V4:
  - Some revises based on the comments from Bjorn, Rob on V4;
  - Fixed the compile error on some platforms, such as openrisc;

Changes from V3:
  - UART support deferred to a separate patchset; This patchset only support
  ipmi device under LPC;
  - LPC bus I/O range is fixed to 0 ~ (PCIBIOS_MIN_IO - 1), which is separeted
  from PCI/PCIE PIO space;
  - Based on Arnd's remarks, removed the ranges property from Hip06 lpc dts and
  added a new fixup function, of_isa_indirect_io(), to get the I/O address
  directly from LPC dts configurations;
  - Support in(w,l)/out(w,l) for Hip06 lpc I/O;
  - Decouple the header file dependency on the gerenic io.h by defining in/out
  as normal functions in c file;
  - removed unused macro definitions in the LPC driver;

Changes from V2:
  - Support the PIO retrieval from the linux PIO generated by
  pci_address_to_pio. This method replace the 4K PIO reservation in V2;
  - Support the flat-tree earlycon;
  - Some revises based on Arnd's remarks;
  - Make sure the linux PIO range allocated to Hip06 LPC peripherals starts
  from non-ZERO;

Changes from V1:
  - Support the ACPI LPC device;
  - Optimize the dts LPC driver in ISA compatible mode;
  - Reserve the IO range below 4K in avoid the possible conflict with PCI host
  IO ranges;
  - Support the LPC uart and relevant earlycon;

Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>

zhichang.yuan (3):
  ARM64 LPC: Indirect ISA port IO introduced
  ARM64 LPC: Add missing range exception for special ISA
  ARM64 LPC: LPC driver implementation on Hip06

 .../arm/hisilicon/hisilicon-low-pin-count.txt      |  31 ++
 MAINTAINERS                                        |   8 +
 arch/arm64/Kconfig                                 |   6 +
 arch/arm64/include/asm/extio.h                     |  94 ++++
 arch/arm64/include/asm/io.h                        |  35 ++
 arch/arm64/kernel/Makefile                         |   1 +
 arch/arm64/kernel/extio.c                          |  52 +++
 drivers/bus/Kconfig                                |   8 +
 drivers/bus/Makefile                               |   1 +
 drivers/bus/hisi_lpc.c                             | 501 +++++++++++++++++++++
 drivers/of/address.c                               |  56 ++-
 drivers/pci/pci.c                                  |   6 +-
 include/linux/of_address.h                         |  17 +
 include/linux/pci.h                                |   8 +
 14 files changed, 820 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
 create mode 100644 arch/arm64/include/asm/extio.h
 create mode 100644 arch/arm64/kernel/extio.c
 create mode 100644 drivers/bus/hisi_lpc.c

-- 
1.9.1

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

[zhichang.yuan]

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-5wv7dgnIgG8@public.gmane.org>
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: zhichang.yuan <yuanzhichang-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 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-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
+ *
+ * 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-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
+ *
+ * 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Rutland]

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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

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

[Arnd Bergmann]

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

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

[Benjamin Herrenschmidt]

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.

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

[zhichang.yuan]

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.
> 

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

[Mark Rutland]

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.

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

[Benjamin Herrenschmidt]

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.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[zhichang.yuan]

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.
> 
> 
> .
> 

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

[Arnd Bergmann]

On Friday, November 11, 2016 6:07:07 PM CET zhichang.yuan wrote:
> 
> 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.

Ah, have you looked at the IA64 code? It does exactly this.
For ARM64 we decided to use the same basic approach as powerpc with
a single range of virtual memory for mapping it as that somewhat
simplified all cases we knew about at the time.

> 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...

On a 64-bit architecture, the top 32 bits of the port number are
definitely free to use for this, and 8 bits are probably sufficient.

Even on 32 bit architectures, I can't see why we'd ever need more than
16 bits worth of addressing within a domain, so using 8 bit domain
and 16 bit address leaves 8 or 40 unused bits.

	Arnd

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

[zhichang.yuan]

Hi, Arnd,


On 2016/11/18 17:20, Arnd Bergmann wrote:
> On Friday, November 11, 2016 6:07:07 PM CET zhichang.yuan wrote:
>>
>> 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.
> 
> Ah, have you looked at the IA64 code? It does exactly this.
> For ARM64 we decided to use the same basic approach as powerpc with
> a single range of virtual memory for mapping it as that somewhat
> simplified all cases we knew about at the time.

Yes. I spent some time to trace how to work on PPC. But the code is a bit long,
I am not clear on how the indirectIO there was supported.

I noticed there are CONFIG_PPC_INDIRECT_PIO and CONFIG_PPC_INDIRECT_MMIO on PPC.
It seems that only CONFIG_PPC_INDIRECT_MMIO applied some MSB to store the bus
tokens which are used to get iowa_busses[] for specific operation helpers.
I can not find how CONFIG_PPC_INDIRECT_PIO support multiple ISA domains. It
seems only Opal-lpc.c adopt this INDIRECT_PIO method.

Although CONFIG_PPC_INDIRECT_MMIO is for MMIO, seems not suitable for ISA/LPC
I/O. But this idea is helpful.

what else did I miss??

> 
>> 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...
> 
> On a 64-bit architecture, the top 32 bits of the port number are
> definitely free to use for this, and 8 bits are probably sufficient.
> 
> Even on 32 bit architectures, I can't see why we'd ever need more than
> 16 bits worth of addressing within a domain, so using 8 bit domain
> and 16 bit address leaves 8 or 40 unused bits.

Yes. 8 bits are enough.
But the maximal PIO on some architectures are defined as ~0 or -1. There is no
any bare space left. Probably we can not ensure the upper 8 bits available.


Thanks,
Zhichang


> 
> 	Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

On Friday, November 18, 2016 7:12:35 PM CET zhichang.yuan wrote:
> Hi, Arnd,
> 
> 
> On 2016/11/18 17:20, Arnd Bergmann wrote:
> > On Friday, November 11, 2016 6:07:07 PM CET zhichang.yuan wrote:
> >>
> >> 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.
> > 
> > Ah, have you looked at the IA64 code? It does exactly this.
> > For ARM64 we decided to use the same basic approach as powerpc with
> > a single range of virtual memory for mapping it as that somewhat
> > simplified all cases we knew about at the time.
> 
> Yes. I spent some time to trace how to work on PPC. But the code is a bit long,
> I am not clear on how the indirectIO there was supported.
> 
> I noticed there are CONFIG_PPC_INDIRECT_PIO and CONFIG_PPC_INDIRECT_MMIO on PPC.
> It seems that only CONFIG_PPC_INDIRECT_MMIO applied some MSB to store the bus
> tokens which are used to get iowa_busses[] for specific operation helpers.
> I can not find how CONFIG_PPC_INDIRECT_PIO support multiple ISA domains. It
> seems only Opal-lpc.c adopt this INDIRECT_PIO method.
> 
> Although CONFIG_PPC_INDIRECT_MMIO is for MMIO, seems not suitable for ISA/LPC
> I/O. But this idea is helpful.
> 
> what else did I miss??

I mentioned two different things here: ia64 IIRC uses some bits of the
port number to look up the domain, while powerpc traditionally had no
support for any such lookup, it did the same thing as ARM64 with
virtual remapping of MMIO ranges into an address range starting at
a fixed virtual address.

CONFIG_PPC_INDIRECT_PIO is a fairly recent addition, I was not thinking
of that.

> >> 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...
> > 
> > On a 64-bit architecture, the top 32 bits of the port number are
> > definitely free to use for this, and 8 bits are probably sufficient.
> > 
> > Even on 32 bit architectures, I can't see why we'd ever need more than
> > 16 bits worth of addressing within a domain, so using 8 bit domain
> > and 16 bit address leaves 8 or 40 unused bits.
> 
> Yes. 8 bits are enough.
> But the maximal PIO on some architectures are defined as ~0 or -1. There is no
> any bare space left. Probably we can not ensure the upper 8 bits available.

Right, we clearly can't use it across all architectures. The trick with
architectures using ULONG_MAX as the limit for port numbers is that they
treat it as a 1:1 mapping between port numbers and virtual addresses,
which is yet another way to handle the MMIO-based devices, but that has
a number of downsides we don't need to get into now.

What I think the code should do is a generic workaround handling that
architectures can opt-in to. We'd start doing this on ARM64 only,
and can then decide whether to change ARM or PowerPC over to use
that as well.

	Arnd

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

[John Garry]

On 08/11/2016 23: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.
>
> 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.
>
Hi,

I would like to mention another topic on supporting LPC, and this is 
regard to eSPI support.
eSPI is seen as the successor for LPC, and some BMCs already support it.

I had a chat with Arnd on this, and the idea to model LPC as a SPI bus 
adpater (and also eSPI).

However it seems to me that most platforms will/should support eSPI as a 
transparent bridge, same as LPC on x86. So I don't think that this is 
much point in modelling LPC/eSPI as a bus.

So we shall continue with indriect-IO support...

Thanks,
John

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

[Will Deacon]

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

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

[John Garry]

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

>
> .
>

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

[Will Deacon]

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

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

[John Garry]

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
>
> .
>

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

[Arnd Bergmann]

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

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

[John Garry]

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
>
> .
>


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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

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

[Ming Lei]

Hi Guys,

On Tue, Nov 8, 2016 at 11:47 AM, zhichang.yuan
<yuanzhichang@hisilicon.com> 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(+)

When I applied these three patches against current linus tree and
enable CONFIG_HISILICON_LPC, the following build failure[1] is
triggered when running 'make modules'.


Thanks,
Ming

[1] 'make modules' failure log

  Building modules, stage 2.
  MODPOST 2260 modules
ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
ERROR: "outw" [drivers/video/vgastate.ko] undefined!
ERROR: "outb" [drivers/video/vgastate.ko] undefined!
ERROR: "inb" [drivers/video/vgastate.ko] undefined!
ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
.....
ERROR: "inb" [drivers/ata/pata_cmd64x.ko] undefined!
ERROR: "inb" [drivers/ata/pata_artop.ko] undefined!
scripts/Makefile.modpost:91: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1196: recipe for target 'modules' failed
make: *** [modules] Error 2


>  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
>



-- 
Ming Lei

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

[zhichang.yuan]

Hi，Ming,


On 2016/12/22 16:15, Ming Lei wrote:
> Hi Guys,
> 
> On Tue, Nov 8, 2016 at 11:47 AM, zhichang.yuan
> <yuanzhichang@hisilicon.com> 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(+)
> 
> When I applied these three patches against current linus tree and
> enable CONFIG_HISILICON_LPC, the following build failure[1] is
> triggered when running 'make modules'.
> 

Thanks for your report!

This patch has compilation issue on some architectures, sorry for the inconvenience caused by this!
The ongoing v6 will solve these issues.
I will trace this failure and provide a fix if you can not wait for the next version.

Could you send me your .config in private? I don't want to bother all the hacker in the mail-list.


Thanks,
Zhichang

> 
> Thanks,
> Ming
> 
> [1] 'make modules' failure log
> 
>   Building modules, stage 2.
>   MODPOST 2260 modules
> ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "outw" [drivers/video/vgastate.ko] undefined!
> ERROR: "outb" [drivers/video/vgastate.ko] undefined!
> ERROR: "inb" [drivers/video/vgastate.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
> .....
> ERROR: "inb" [drivers/ata/pata_cmd64x.ko] undefined!
> ERROR: "inb" [drivers/ata/pata_artop.ko] undefined!
> scripts/Makefile.modpost:91: recipe for target '__modpost' failed
> make[1]: *** [__modpost] Error 1
> Makefile:1196: recipe for target 'modules' failed
> make: *** [modules] Error 2
> 
> 
>>  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
>>
> 
> 
> 

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

[Ming Lei]

[-- Attachment #1: Type: text/plain, Size: 13571 bytes --]

On Fri, Dec 23, 2016 at 9:43 AM, zhichang.yuan
<yuanzhichang@hisilicon.com> wrote:
> Hi，Ming,
>
>
> On 2016/12/22 16:15, Ming Lei wrote:
>> Hi Guys,
>>
>> On Tue, Nov 8, 2016 at 11:47 AM, zhichang.yuan
>> <yuanzhichang@hisilicon.com> 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(+)
>>
>> When I applied these three patches against current linus tree and
>> enable CONFIG_HISILICON_LPC, the following build failure[1] is
>> triggered when running 'make modules'.
>>
>
> Thanks for your report!
>
> This patch has compilation issue on some architectures, sorry for the inconvenience caused by this!
> The ongoing v6 will solve these issues.
> I will trace this failure and provide a fix if you can not wait for the next version.
>
> Could you send me your .config in private? I don't want to bother all the hacker in the mail-list.
>

Sure, please see the config in attachment.

>
> Thanks,
> Zhichang
>
>>
>> Thanks,
>> Ming
>>
>> [1] 'make modules' failure log
>>
>>   Building modules, stage 2.
>>   MODPOST 2260 modules
>> ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
>> ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
>> ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
>> ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
>> ERROR: "outw" [drivers/video/vgastate.ko] undefined!
>> ERROR: "outb" [drivers/video/vgastate.ko] undefined!
>> ERROR: "inb" [drivers/video/vgastate.ko] undefined!
>> ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
>> ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
>> ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
>> ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
>> ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
>> ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
>> ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
>> .....
>> ERROR: "inb" [drivers/ata/pata_cmd64x.ko] undefined!
>> ERROR: "inb" [drivers/ata/pata_artop.ko] undefined!
>> scripts/Makefile.modpost:91: recipe for target '__modpost' failed
>> make[1]: *** [__modpost] Error 1
>> Makefile:1196: recipe for target 'modules' failed
>> make: *** [modules] Error 2
>>
>>
>>>  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
>>>
>>
>>
>>
>



Thanks,
Ming Lei

[-- Attachment #2: config.tar.gz --]
[-- Type: application/x-gzip, Size: 37193 bytes --]

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

[Arnd Bergmann]

On Thursday, December 22, 2016 4:15:57 PM CET Ming Lei wrote:
> ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "outw" [drivers/video/vgastate.ko] undefined!
> ERROR: "outb" [drivers/video/vgastate.ko] undefined!
> ERROR: "inb" [drivers/video/vgastate.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
> 

In case you haven't figured it out by now, the new code is simply
missing a few "EXPORT_SYMBOL" lines.

	Arnd

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

[Yuanzhichang]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Hi, Arnd,
Thanks for your points!
l had sent a private fix patch to Ming before, he tested and said it was OK.
All these will be covered in the coming V6.

Ming, thanks for your work too!

Best,
Zhichang

Sent from HUAWEI AnyOffice
发件人：Arnd Bergmann
收件人：linux-arm-kernel@lists.infradead.org
抄送：Ming Lei,袁志昌,Mark Rutland,Gabriele Paoloni,Benjamin Herrenschmidt,Will Deacon,Linuxarm,Lorenzo Pieralisi,徐威,linux-serial@vger.kernel.org,Catalin Marinas,devicetree@vger.kernel.org,minyard@acm.org,Liviu Dudau,John Garry,zourongrong@gmail.com,Rob Herring,Bjorn Helgaas,kantyzc@163.com,zhichang.yuan02@gmail.com,linux-pci@vger.kernel.org,Linux Kernel Mailing List,Olof Johansson
时间：2017-01-06 19:43:58
主题:Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced

On Thursday, December 22, 2016 4:15:57 PM CET Ming Lei wrote:
> ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "outw" [drivers/video/vgastate.ko] undefined!
> ERROR: "outb" [drivers/video/vgastate.ko] undefined!
> ERROR: "inb" [drivers/video/vgastate.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
>

In case you haven't figured it out by now, the new code is simply
missing a few "EXPORT_SYMBOL" lines.

        Arnd


[-- Attachment #2: Type: text/html, Size: 3013 bytes --]

[PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[zhichang.yuan]

This patch solves two issues:
1) parse and get the right I/O range from DTS node whose parent does not
define the corresponding ranges property;

There are some special ISA/LPC devices that work on a specific I/O range where
it is not correct to specify a ranges property in DTS parent node as cpu
addresses translated from DTS node are only for memory space on some
architectures, such as Arm64. Without the parent 'ranges' property, current
of_translate_address() return an error.
Here we add a fixup function, of_get_isa_indirect_io(). During the OF address
translation, this fixup will be called to check the 'reg' address to be
translating is for those sepcial ISA/LPC devices and get the I/O range
directly from the 'reg' property.

2) eliminate the I/O range conflict risk with PCI/PCIE leagecy I/O device;

The current __of_address_to_resource() always translates the I/O range to PIO.
But this processing is not suitable for our ISA/LPC devices whose I/O range is
not cpu address(Arnd had stressed this in his comments on V2,V3 patch-set).
Here, we bypass the mapping between cpu address and PIO for the special
ISA/LPC devices. But to drive these ISA/LPC devices, a I/O port address below
PCIBIOS_MIN_IO is needed by in*/out*(). Which means there is conflict risk
between I/O range of [0, PCIBIOS_MIN_IO) and PCI/PCIE legacy I/O range of [0,
IO_SPACE_LIMIT).
To avoid the I/O conflict, this patch reserve the I/O range below
PCIBIOS_MIN_IO.

Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 .../arm/hisilicon/hisilicon-low-pin-count.txt      | 31 ++++++++++++
 arch/arm64/include/asm/io.h                        |  6 +++
 arch/arm64/kernel/extio.c                          | 25 ++++++++++
 drivers/of/address.c                               | 56 +++++++++++++++++++++-
 drivers/pci/pci.c                                  |  6 +--
 include/linux/of_address.h                         | 17 +++++++
 include/linux/pci.h                                |  8 ++++
 7 files changed, 145 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt

diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
new file mode 100644
index 0000000..13c8ddd
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
@@ -0,0 +1,31 @@
+Hisilicon Hip06 low-pin-count device
+  Usually LPC controller is part of PCI host bridge, so the legacy ISA ports
+  locate on LPC bus can be accessed direclty. But some SoCs have independent
+  LPC controller, and access the legacy ports by triggering LPC I/O cycles.
+  Hisilicon Hip06 implements this LPC device.
+
+Required properties:
+- compatible: should be "hisilicon,low-pin-count"
+- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
+- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
+- reg: base memory range where the register set of this device is mapped.
+
+Note:
+  The node name before '@' must be "isa" to represent the binding stick to the
+  ISA/EISA binding specification.
+
+Example:
+
+isa@a01b0000 {
+	compatible = "hisilicom,low-pin-count";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	reg = <0x0 0xa01b0000 0x0 0x1000>;
+
+	ipmi0: bt@e4 {
+		compatible = "ipmi-bt";
+		device_type = "ipmi";
+		reg = <0x01 0xe4 0x04>;
+		status = "disabled";
+	};
+};
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 136735d..c26b7cc 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -175,6 +175,12 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
 #define outsl outsl
 
 DECLARE_EXTIO(l, u32)
+
+#define indirect_io_enabled indirect_io_enabled
+extern bool indirect_io_enabled(void);
+
+#define addr_is_indirect_io addr_is_indirect_io
+extern int addr_is_indirect_io(u64 taddr);
 #endif
 
 
diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
index 647b3fa..3d45fa8 100644
--- a/arch/arm64/kernel/extio.c
+++ b/arch/arm64/kernel/extio.c
@@ -19,6 +19,31 @@
 
 struct extio_ops *arm64_extio_ops;
 
+/**
+ * indirect_io_enabled - check whether indirectIO is enabled.
+ *	arm64_extio_ops will be set only when indirectIO mechanism had been
+ *	initialized.
+ *
+ * Returns true when indirectIO is enabled.
+ */
+bool indirect_io_enabled(void)
+{
+	return arm64_extio_ops ? true : false;
+}
+
+/**
+ * addr_is_indirect_io - check whether the input taddr is for indirectIO.
+ * @taddr: the io address to be checked.
+ *
+ * Returns 1 when taddr is in the range; otherwise return 0.
+ */
+int addr_is_indirect_io(u64 taddr)
+{
+	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end < taddr)
+		return 0;
+
+	return 1;
+}
 
 BUILD_EXTIO(b, u8)
 
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903..cc2a05d 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct device_node *np)
 	return false;
 }
 
+
+/*
+ * of_isa_indirect_io - get the IO address from some isa reg property value.
+ *	For some isa/lpc devices, no ranges property in ancestor node.
+ *	The device addresses are described directly in their regs property.
+ *	This fixup function will be called to get the IO address of isa/lpc
+ *	devices when the normal of_translation failed.
+ *
+ * @parent:	points to the parent dts node;
+ * @bus:		points to the of_bus which can be used to parse address;
+ * @addr:	the address from reg property;
+ * @na:		the address cell counter of @addr;
+ * @presult:	store the address paresed from @addr;
+ *
+ * return 1 when successfully get the I/O address;
+ * 0 will return for some failures.
+ */
+static int of_get_isa_indirect_io(struct device_node *parent,
+				struct of_bus *bus, __be32 *addr,
+				int na, u64 *presult)
+{
+	unsigned int flags;
+	unsigned int rlen;
+
+	/* whether support indirectIO */
+	if (!indirect_io_enabled())
+		return 0;
+
+	if (!of_bus_isa_match(parent))
+		return 0;
+
+	flags = bus->get_flags(addr);
+	if (!(flags & IORESOURCE_IO))
+		return 0;
+
+	/* there is ranges property, apply the normal translation directly. */
+	if (of_get_property(parent, "ranges", &rlen))
+		return 0;
+
+	*presult = of_read_number(addr + 1, na - 1);
+	/* this fixup is only valid for specific I/O range. */
+	return addr_is_indirect_io(*presult);
+}
+
 static int of_translate_one(struct device_node *parent, struct of_bus *bus,
 			    struct of_bus *pbus, __be32 *addr,
 			    int na, int ns, int pna, const char *rprop)
@@ -595,6 +639,15 @@ static u64 __of_translate_address(struct device_node *dev,
 			result = of_read_number(addr, na);
 			break;
 		}
+		/*
+		 * For indirectIO device which has no ranges property, get
+		 * the address from reg directly.
+		 */
+		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
+			pr_debug("isa indirectIO matched(%s)..addr = 0x%llx\n",
+				of_node_full_name(dev), result);
+			break;
+		}
 
 		/* Get new parent bus and counts */
 		pbus = of_match_bus(parent);
@@ -688,8 +741,9 @@ static int __of_address_to_resource(struct device_node *dev,
 	if (taddr == OF_BAD_ADDR)
 		return -EINVAL;
 	memset(r, 0, sizeof(struct resource));
-	if (flags & IORESOURCE_IO) {
+	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
 		unsigned long port;
+
 		port = pci_address_to_pio(taddr);
 		if (port == (unsigned long)-1)
 			return -EINVAL;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ba34907..1a08511 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
 
 #ifdef PCI_IOBASE
 	struct io_range *range;
-	resource_size_t allocated_size = 0;
+	resource_size_t allocated_size = PCIBIOS_MIN_IO;
 
 	/* check if the range hasn't been previously recorded */
 	spin_lock(&io_range_lock);
@@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
 
 #ifdef PCI_IOBASE
 	struct io_range *range;
-	resource_size_t allocated_size = 0;
+	resource_size_t allocated_size = PCIBIOS_MIN_IO;
 
 	if (pio > IO_SPACE_LIMIT)
 		return address;
@@ -3335,7 +3335,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
 {
 #ifdef PCI_IOBASE
 	struct io_range *res;
-	resource_size_t offset = 0;
+	resource_size_t offset = PCIBIOS_MIN_IO;
 	unsigned long addr = -1;
 
 	spin_lock(&io_range_lock);
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 3786473..deec469 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -24,6 +24,23 @@ struct of_pci_range {
 #define for_each_of_pci_range(parser, range) \
 	for (; of_pci_range_parser_one(parser, range);)
 
+
+#ifndef indirect_io_enabled
+#define indirect_io_enabled indirect_io_enabled
+static inline bool indirect_io_enabled(void)
+{
+	return false;
+}
+#endif
+
+#ifndef addr_is_indirect_io
+#define addr_is_indirect_io addr_is_indirect_io
+static inline int addr_is_indirect_io(u64 taddr)
+{
+	return 0;
+}
+#endif
+
 /* Translate a DMA address from device space to CPU space */
 extern u64 of_translate_dma_address(struct device_node *dev,
 				    const __be32 *in_addr);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0e49f70..7f6bbb6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
 /* provide the legacy pci_dma_* API */
 #include <linux/pci-dma-compat.h>
 
+/*
+ * define this macro here to refrain from compilation error for some
+ * platforms. Please keep this macro at the end of this header file.
+ */
+#ifndef PCIBIOS_MIN_IO
+#define PCIBIOS_MIN_IO		0
+#endif
+
 #endif /* LINUX_PCI_H */
-- 
1.9.1

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]

Hi zhichang.yuan,

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.9-rc4 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/zhichang-yuan/ARM64-LPC-legacy-ISA-I-O-support/20161108-114742
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: mips-ath25_defconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All error/warnings (new ones prefixed by >>):

   In file included from arch/mips/pci/pci.c:18:0:
>> include/linux/pci.h:2113:25: error: expected identifier or '(' before numeric constant
    #define PCIBIOS_MIN_IO  0
                            ^
>> arch/mips/pci/pci.c:34:15: note: in expansion of macro 'PCIBIOS_MIN_IO'
    unsigned long PCIBIOS_MIN_IO;
                  ^~~~~~~~~~~~~~
   In file included from include/linux/linkage.h:6:0,
                    from include/linux/kernel.h:6,
                    from include/asm-generic/bug.h:13,
                    from arch/mips/include/asm/bug.h:41,
                    from include/linux/bug.h:4,
                    from arch/mips/pci/pci.c:11:
>> include/linux/pci.h:2113:25: error: expected identifier or '(' before numeric constant
    #define PCIBIOS_MIN_IO  0
                            ^
   include/linux/export.h:57:21: note: in definition of macro '___EXPORT_SYMBOL'
     extern typeof(sym) sym;     \
                        ^~~
>> arch/mips/pci/pci.c:326:1: note: in expansion of macro 'EXPORT_SYMBOL'
    EXPORT_SYMBOL(PCIBIOS_MIN_IO);
    ^~~~~~~~~~~~~
   arch/mips/pci/pci.c:326:15: note: in expansion of macro 'PCIBIOS_MIN_IO'
    EXPORT_SYMBOL(PCIBIOS_MIN_IO);
                  ^~~~~~~~~~~~~~
>> include/linux/export.h:66:21: error: lvalue required as unary '&' operand
     = { (unsigned long)&sym, __kstrtab_##sym }
                        ^
>> include/linux/export.h:94:25: note: in expansion of macro '___EXPORT_SYMBOL'
    #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
                            ^~~~~~~~~~~~~~~~
   include/linux/export.h:98:2: note: in expansion of macro '__EXPORT_SYMBOL'
     __EXPORT_SYMBOL(sym, "")
     ^~~~~~~~~~~~~~~
>> arch/mips/pci/pci.c:326:1: note: in expansion of macro 'EXPORT_SYMBOL'
    EXPORT_SYMBOL(PCIBIOS_MIN_IO);
    ^~~~~~~~~~~~~

vim +2113 include/linux/pci.h

  2107	
  2108	/*
  2109	 * define this macro here to refrain from compilation error for some
  2110	 * platforms. Please keep this macro at the end of this header file.
  2111	 */
  2112	#ifndef PCIBIOS_MIN_IO
> 2113	#define PCIBIOS_MIN_IO		0
  2114	#endif
  2115	
  2116	#endif /* LINUX_PCI_H */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 11431 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

Hi zhichang.yuan,

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.9-rc4 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/zhichang-yuan/ARM64-LPC-legacy-ISA-I-O-support/20161108-114742
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm-sunxi_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `of_address_to_resource':
>> sunxi_sid.c:(.text+0x18af5c): undefined reference to `pcibios_min_io'
   sunxi_sid.c:(.text+0x18af60): undefined reference to `pcibios_min_io'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20012 bytes --]

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[liviu.dudau-5wv7dgnIgG8]

On Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:
> This patch solves two issues:
> 1) parse and get the right I/O range from DTS node whose parent does not
> define the corresponding ranges property;
> 
> There are some special ISA/LPC devices that work on a specific I/O range where
> it is not correct to specify a ranges property in DTS parent node as cpu
> addresses translated from DTS node are only for memory space on some
> architectures, such as Arm64. Without the parent 'ranges' property, current
> of_translate_address() return an error.
> Here we add a fixup function, of_get_isa_indirect_io(). During the OF address
> translation, this fixup will be called to check the 'reg' address to be
> translating is for those sepcial ISA/LPC devices and get the I/O range
> directly from the 'reg' property.
> 
> 2) eliminate the I/O range conflict risk with PCI/PCIE leagecy I/O device;
> 
> The current __of_address_to_resource() always translates the I/O range to PIO.
> But this processing is not suitable for our ISA/LPC devices whose I/O range is
> not cpu address(Arnd had stressed this in his comments on V2,V3 patch-set).
> Here, we bypass the mapping between cpu address and PIO for the special
> ISA/LPC devices. But to drive these ISA/LPC devices, a I/O port address below
> PCIBIOS_MIN_IO is needed by in*/out*(). Which means there is conflict risk
> between I/O range of [0, PCIBIOS_MIN_IO) and PCI/PCIE legacy I/O range of [0,
> IO_SPACE_LIMIT).
> To avoid the I/O conflict, this patch reserve the I/O range below
> PCIBIOS_MIN_IO.
> 
> Signed-off-by: zhichang.yuan <yuanzhichang-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  .../arm/hisilicon/hisilicon-low-pin-count.txt      | 31 ++++++++++++
>  arch/arm64/include/asm/io.h                        |  6 +++
>  arch/arm64/kernel/extio.c                          | 25 ++++++++++
>  drivers/of/address.c                               | 56 +++++++++++++++++++++-
>  drivers/pci/pci.c                                  |  6 +--
>  include/linux/of_address.h                         | 17 +++++++
>  include/linux/pci.h                                |  8 ++++
>  7 files changed, 145 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> new file mode 100644
> index 0000000..13c8ddd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> @@ -0,0 +1,31 @@
> +Hisilicon Hip06 low-pin-count device
> +  Usually LPC controller is part of PCI host bridge, so the legacy ISA ports
> +  locate on LPC bus can be accessed direclty. But some SoCs have independent
> +  LPC controller, and access the legacy ports by triggering LPC I/O cycles.
> +  Hisilicon Hip06 implements this LPC device.
> +
> +Required properties:
> +- compatible: should be "hisilicon,low-pin-count"
> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> +- reg: base memory range where the register set of this device is mapped.
> +
> +Note:
> +  The node name before '@' must be "isa" to represent the binding stick to the
> +  ISA/EISA binding specification.
> +
> +Example:
> +
> +isa@a01b0000 {
> +	compatible = "hisilicom,low-pin-count";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	reg = <0x0 0xa01b0000 0x0 0x1000>;
> +
> +	ipmi0: bt@e4 {
> +		compatible = "ipmi-bt";
> +		device_type = "ipmi";
> +		reg = <0x01 0xe4 0x04>;
> +		status = "disabled";
> +	};
> +};


This documentation file needs to be part of the next patch. It has nothing to do with
what you are trying to fix here.


> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 136735d..c26b7cc 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -175,6 +175,12 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>  #define outsl outsl
>  
>  DECLARE_EXTIO(l, u32)
> +
> +#define indirect_io_enabled indirect_io_enabled
> +extern bool indirect_io_enabled(void);
> +
> +#define addr_is_indirect_io addr_is_indirect_io
> +extern int addr_is_indirect_io(u64 taddr);
>  #endif
>  
>  
> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> index 647b3fa..3d45fa8 100644
> --- a/arch/arm64/kernel/extio.c
> +++ b/arch/arm64/kernel/extio.c
> @@ -19,6 +19,31 @@
>  
>  struct extio_ops *arm64_extio_ops;
>  
> +/**
> + * indirect_io_enabled - check whether indirectIO is enabled.
> + *	arm64_extio_ops will be set only when indirectIO mechanism had been
> + *	initialized.
> + *
> + * Returns true when indirectIO is enabled.
> + */
> +bool indirect_io_enabled(void)
> +{
> +	return arm64_extio_ops ? true : false;
> +}
> +
> +/**
> + * addr_is_indirect_io - check whether the input taddr is for indirectIO.
> + * @taddr: the io address to be checked.
> + *
> + * Returns 1 when taddr is in the range; otherwise return 0.
> + */
> +int addr_is_indirect_io(u64 taddr)
> +{
> +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end < taddr)

start >= taddr ?

> +		return 0;
> +
> +	return 1;
> +}
>  
>  BUILD_EXTIO(b, u8)
>  
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 02b2903..cc2a05d 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct device_node *np)
>  	return false;
>  }
>  
> +
> +/*
> + * of_isa_indirect_io - get the IO address from some isa reg property value.
> + *	For some isa/lpc devices, no ranges property in ancestor node.
> + *	The device addresses are described directly in their regs property.
> + *	This fixup function will be called to get the IO address of isa/lpc
> + *	devices when the normal of_translation failed.
> + *
> + * @parent:	points to the parent dts node;
> + * @bus:		points to the of_bus which can be used to parse address;
> + * @addr:	the address from reg property;
> + * @na:		the address cell counter of @addr;
> + * @presult:	store the address paresed from @addr;
> + *
> + * return 1 when successfully get the I/O address;
> + * 0 will return for some failures.

Bah, you are returning a signed int, why 0 for failure? Return a negative value with
error codes. Otherwise change the return value into a bool.

> + */
> +static int of_get_isa_indirect_io(struct device_node *parent,
> +				struct of_bus *bus, __be32 *addr,
> +				int na, u64 *presult)
> +{
> +	unsigned int flags;
> +	unsigned int rlen;
> +
> +	/* whether support indirectIO */
> +	if (!indirect_io_enabled())
> +		return 0;
> +
> +	if (!of_bus_isa_match(parent))
> +		return 0;
> +
> +	flags = bus->get_flags(addr);
> +	if (!(flags & IORESOURCE_IO))
> +		return 0;
> +
> +	/* there is ranges property, apply the normal translation directly. */

s/there is ranges/if we have a 'ranges'/

> +	if (of_get_property(parent, "ranges", &rlen))
> +		return 0;
> +
> +	*presult = of_read_number(addr + 1, na - 1);
> +	/* this fixup is only valid for specific I/O range. */
> +	return addr_is_indirect_io(*presult);
> +}
> +
>  static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>  			    struct of_bus *pbus, __be32 *addr,
>  			    int na, int ns, int pna, const char *rprop)
> @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct device_node *dev,
>  			result = of_read_number(addr, na);
>  			break;
>  		}
> +		/*
> +		 * For indirectIO device which has no ranges property, get
> +		 * the address from reg directly.
> +		 */
> +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> +			pr_debug("isa indirectIO matched(%s)..addr = 0x%llx\n",
> +				of_node_full_name(dev), result);
> +			break;
> +		}
>  
>  		/* Get new parent bus and counts */
>  		pbus = of_match_bus(parent);
> @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct device_node *dev,
>  	if (taddr == OF_BAD_ADDR)
>  		return -EINVAL;
>  	memset(r, 0, sizeof(struct resource));
> -	if (flags & IORESOURCE_IO) {
> +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
>  		unsigned long port;
> +
>  		port = pci_address_to_pio(taddr);
>  		if (port == (unsigned long)-1)
>  			return -EINVAL;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ba34907..1a08511 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
>  
>  #ifdef PCI_IOBASE
>  	struct io_range *range;
> -	resource_size_t allocated_size = 0;
> +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
>  
>  	/* check if the range hasn't been previously recorded */
>  	spin_lock(&io_range_lock);
> @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
>  
>  #ifdef PCI_IOBASE
>  	struct io_range *range;
> -	resource_size_t allocated_size = 0;
> +	resource_size_t allocated_size = PCIBIOS_MIN_IO;

Have you checked that pci_pio_to_address still returns valid values after this? I know that
you are trying to take into account PCIBIOS_MIN_IO limit when allocating reserving the IO ranges,
but the values added in the io_range_list are still starting from zero, no from PCIBIOS_MIN_IO,
so the calculation of the address in this function could return negative values casted to pci_addr_t.

Maybe you want to adjust the range->start value in pci_register_io_range() as well to have it
offset by PCIBIOS_MIN_IO as well.

Best regards,
Liviu

>  
>  	if (pio > IO_SPACE_LIMIT)
>  		return address;
> @@ -3335,7 +3335,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
>  {
>  #ifdef PCI_IOBASE
>  	struct io_range *res;
> -	resource_size_t offset = 0;
> +	resource_size_t offset = PCIBIOS_MIN_IO;
>  	unsigned long addr = -1;
>  
>  	spin_lock(&io_range_lock);
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 3786473..deec469 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -24,6 +24,23 @@ struct of_pci_range {
>  #define for_each_of_pci_range(parser, range) \
>  	for (; of_pci_range_parser_one(parser, range);)
>  
> +
> +#ifndef indirect_io_enabled
> +#define indirect_io_enabled indirect_io_enabled
> +static inline bool indirect_io_enabled(void)
> +{
> +	return false;
> +}
> +#endif
> +
> +#ifndef addr_is_indirect_io
> +#define addr_is_indirect_io addr_is_indirect_io
> +static inline int addr_is_indirect_io(u64 taddr)
> +{
> +	return 0;
> +}
> +#endif
> +
>  /* Translate a DMA address from device space to CPU space */
>  extern u64 of_translate_dma_address(struct device_node *dev,
>  				    const __be32 *in_addr);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0e49f70..7f6bbb6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
>  /* provide the legacy pci_dma_* API */
>  #include <linux/pci-dma-compat.h>
>  
> +/*
> + * define this macro here to refrain from compilation error for some
> + * platforms. Please keep this macro at the end of this header file.
> + */
> +#ifndef PCIBIOS_MIN_IO
> +#define PCIBIOS_MIN_IO		0
> +#endif
> +
>  #endif /* LINUX_PCI_H */
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[Gabriele Paoloni]

Hi Liviu

Thanks for reviewing

> -----Original Message-----
> From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> Sent: 09 November 2016 11:40
> To: Yuanzhichang
> Cc: catalin.marinas@arm.com; will.deacon@arm.com; robh+dt@kernel.org;
> bhelgaas@google.com; mark.rutland@arm.com; olof@lixom.net;
> arnd@arndb.de; linux-arm-kernel@lists.infradead.org;
> lorenzo.pieralisi@arm.com; linux-kernel@vger.kernel.org; Linuxarm;
> devicetree@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> serial@vger.kernel.org; minyard@acm.org; benh@kernel.crashing.org;
> zourongrong@gmail.com; John Garry; Gabriele Paoloni;
> zhichang.yuan02@gmail.com; kantyzc@163.com; xuwei (O)
> Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> special ISA
> 
> On Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:
> > This patch solves two issues:
> > 1) parse and get the right I/O range from DTS node whose parent does
> not
> > define the corresponding ranges property;
> >
> > There are some special ISA/LPC devices that work on a specific I/O
> range where
> > it is not correct to specify a ranges property in DTS parent node as
> cpu
> > addresses translated from DTS node are only for memory space on some
> > architectures, such as Arm64. Without the parent 'ranges' property,
> current
> > of_translate_address() return an error.
> > Here we add a fixup function, of_get_isa_indirect_io(). During the OF
> address
> > translation, this fixup will be called to check the 'reg' address to
> be
> > translating is for those sepcial ISA/LPC devices and get the I/O
> range
> > directly from the 'reg' property.
> >
> > 2) eliminate the I/O range conflict risk with PCI/PCIE leagecy I/O
> device;
> >
> > The current __of_address_to_resource() always translates the I/O
> range to PIO.
> > But this processing is not suitable for our ISA/LPC devices whose I/O
> range is
> > not cpu address(Arnd had stressed this in his comments on V2,V3
> patch-set).
> > Here, we bypass the mapping between cpu address and PIO for the
> special
> > ISA/LPC devices. But to drive these ISA/LPC devices, a I/O port
> address below
> > PCIBIOS_MIN_IO is needed by in*/out*(). Which means there is conflict
> risk
> > between I/O range of [0, PCIBIOS_MIN_IO) and PCI/PCIE legacy I/O
> range of [0,
> > IO_SPACE_LIMIT).
> > To avoid the I/O conflict, this patch reserve the I/O range below
> > PCIBIOS_MIN_IO.
> >
> > Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> > ---
> >  .../arm/hisilicon/hisilicon-low-pin-count.txt      | 31 ++++++++++++
> >  arch/arm64/include/asm/io.h                        |  6 +++
> >  arch/arm64/kernel/extio.c                          | 25 ++++++++++
> >  drivers/of/address.c                               | 56
> +++++++++++++++++++++-
> >  drivers/pci/pci.c                                  |  6 +--
> >  include/linux/of_address.h                         | 17 +++++++
> >  include/linux/pci.h                                |  8 ++++
> >  7 files changed, 145 insertions(+), 4 deletions(-)
> >  create mode 100644
> Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-
> count.txt
> >
> > diff --git
> a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-
> count.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-
> low-pin-count.txt
> > new file mode 100644
> > index 0000000..13c8ddd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-
> pin-count.txt
> > @@ -0,0 +1,31 @@
> > +Hisilicon Hip06 low-pin-count device
> > +  Usually LPC controller is part of PCI host bridge, so the legacy
> ISA ports
> > +  locate on LPC bus can be accessed direclty. But some SoCs have
> independent
> > +  LPC controller, and access the legacy ports by triggering LPC I/O
> cycles.
> > +  Hisilicon Hip06 implements this LPC device.
> > +
> > +Required properties:
> > +- compatible: should be "hisilicon,low-pin-count"
> > +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> > +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> > +- reg: base memory range where the register set of this device is
> mapped.
> > +
> > +Note:
> > +  The node name before '@' must be "isa" to represent the binding
> stick to the
> > +  ISA/EISA binding specification.
> > +
> > +Example:
> > +
> > +isa@a01b0000 {
> > +	compatible = "hisilicom,low-pin-count";
> > +	#address-cells = <2>;
> > +	#size-cells = <1>;
> > +	reg = <0x0 0xa01b0000 0x0 0x1000>;
> > +
> > +	ipmi0: bt@e4 {
> > +		compatible = "ipmi-bt";
> > +		device_type = "ipmi";
> > +		reg = <0x01 0xe4 0x04>;
> > +		status = "disabled";
> > +	};
> > +};
> 
> 
> This documentation file needs to be part of the next patch. It has
> nothing to do with
> what you are trying to fix here.

Yes you're right...we'll move it to next one

> 
> 
> > diff --git a/arch/arm64/include/asm/io.h
> b/arch/arm64/include/asm/io.h
> > index 136735d..c26b7cc 100644
> > --- a/arch/arm64/include/asm/io.h
> > +++ b/arch/arm64/include/asm/io.h
> > @@ -175,6 +175,12 @@ static inline u64 __raw_readq(const volatile
> void __iomem *addr)
> >  #define outsl outsl
> >
> >  DECLARE_EXTIO(l, u32)
> > +
> > +#define indirect_io_enabled indirect_io_enabled
> > +extern bool indirect_io_enabled(void);
> > +
> > +#define addr_is_indirect_io addr_is_indirect_io
> > +extern int addr_is_indirect_io(u64 taddr);
> >  #endif
> >
> >
> > diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> > index 647b3fa..3d45fa8 100644
> > --- a/arch/arm64/kernel/extio.c
> > +++ b/arch/arm64/kernel/extio.c
> > @@ -19,6 +19,31 @@
> >
> >  struct extio_ops *arm64_extio_ops;
> >
> > +/**
> > + * indirect_io_enabled - check whether indirectIO is enabled.
> > + *	arm64_extio_ops will be set only when indirectIO mechanism had
> been
> > + *	initialized.
> > + *
> > + * Returns true when indirectIO is enabled.
> > + */
> > +bool indirect_io_enabled(void)
> > +{
> > +	return arm64_extio_ops ? true : false;
> > +}
> > +
> > +/**
> > + * addr_is_indirect_io - check whether the input taddr is for
> indirectIO.
> > + * @taddr: the io address to be checked.
> > + *
> > + * Returns 1 when taddr is in the range; otherwise return 0.
> > + */
> > +int addr_is_indirect_io(u64 taddr)
> > +{
> > +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
> taddr)
> 
> start >= taddr ?

Nope... if  (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
then taddr is outside the range [start; end] and will return 0; otherwise
it will return 1...

> 
> > +		return 0;
> > +
> > +	return 1;
> > +}
> >
> >  BUILD_EXTIO(b, u8)
> >
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 02b2903..cc2a05d 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> device_node *np)
> >  	return false;
> >  }
> >
> > +
> > +/*
> > + * of_isa_indirect_io - get the IO address from some isa reg
> property value.
> > + *	For some isa/lpc devices, no ranges property in ancestor node.
> > + *	The device addresses are described directly in their regs
> property.
> > + *	This fixup function will be called to get the IO address of
> isa/lpc
> > + *	devices when the normal of_translation failed.
> > + *
> > + * @parent:	points to the parent dts node;
> > + * @bus:		points to the of_bus which can be used to parse
> address;
> > + * @addr:	the address from reg property;
> > + * @na:		the address cell counter of @addr;
> > + * @presult:	store the address paresed from @addr;
> > + *
> > + * return 1 when successfully get the I/O address;
> > + * 0 will return for some failures.
> 
> Bah, you are returning a signed int, why 0 for failure? Return a
> negative value with
> error codes. Otherwise change the return value into a bool.

Yes we'll move to bool

> 
> > + */
> > +static int of_get_isa_indirect_io(struct device_node *parent,
> > +				struct of_bus *bus, __be32 *addr,
> > +				int na, u64 *presult)
> > +{
> > +	unsigned int flags;
> > +	unsigned int rlen;
> > +
> > +	/* whether support indirectIO */
> > +	if (!indirect_io_enabled())
> > +		return 0;
> > +
> > +	if (!of_bus_isa_match(parent))
> > +		return 0;
> > +
> > +	flags = bus->get_flags(addr);
> > +	if (!(flags & IORESOURCE_IO))
> > +		return 0;
> > +
> > +	/* there is ranges property, apply the normal translation
> directly. */
> 
> s/there is ranges/if we have a 'ranges'/

Thanks for spotting this

> 
> > +	if (of_get_property(parent, "ranges", &rlen))
> > +		return 0;
> > +
> > +	*presult = of_read_number(addr + 1, na - 1);
> > +	/* this fixup is only valid for specific I/O range. */
> > +	return addr_is_indirect_io(*presult);
> > +}
> > +
> >  static int of_translate_one(struct device_node *parent, struct
> of_bus *bus,
> >  			    struct of_bus *pbus, __be32 *addr,
> >  			    int na, int ns, int pna, const char *rprop)
> > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> device_node *dev,
> >  			result = of_read_number(addr, na);
> >  			break;
> >  		}
> > +		/*
> > +		 * For indirectIO device which has no ranges property, get
> > +		 * the address from reg directly.
> > +		 */
> > +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> > +			pr_debug("isa indirectIO matched(%s)..addr =
> 0x%llx\n",
> > +				of_node_full_name(dev), result);
> > +			break;
> > +		}
> >
> >  		/* Get new parent bus and counts */
> >  		pbus = of_match_bus(parent);
> > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> device_node *dev,
> >  	if (taddr == OF_BAD_ADDR)
> >  		return -EINVAL;
> >  	memset(r, 0, sizeof(struct resource));
> > -	if (flags & IORESOURCE_IO) {
> > +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> >  		unsigned long port;
> > +
> >  		port = pci_address_to_pio(taddr);
> >  		if (port == (unsigned long)-1)
> >  			return -EINVAL;
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index ba34907..1a08511 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
> addr, resource_size_t size)
> >
> >  #ifdef PCI_IOBASE
> >  	struct io_range *range;
> > -	resource_size_t allocated_size = 0;
> > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> >
> >  	/* check if the range hasn't been previously recorded */
> >  	spin_lock(&io_range_lock);
> > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long
> pio)
> >
> >  #ifdef PCI_IOBASE
> >  	struct io_range *range;
> > -	resource_size_t allocated_size = 0;
> > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> 
> Have you checked that pci_pio_to_address still returns valid values
> after this? I know that
> you are trying to take into account PCIBIOS_MIN_IO limit when
> allocating reserving the IO ranges,
> but the values added in the io_range_list are still starting from zero,
> no from PCIBIOS_MIN_IO,

I think you're wrong here as in pci_address_to_pio we have:
+	resource_size_t offset = PCIBIOS_MIN_IO;

This should be enough to guarantee that the PIOs start at
PCIBIOS_MIN_IO...right?


> so the calculation of the address in this function could return
> negative values casted to pci_addr_t.
> 
> Maybe you want to adjust the range->start value in
> pci_register_io_range() as well to have it
> offset by PCIBIOS_MIN_IO as well.
> 
> Best regards,
> Liviu
> 
> >
> >  	if (pio > IO_SPACE_LIMIT)
> >  		return address;
> > @@ -3335,7 +3335,7 @@ unsigned long __weak
> pci_address_to_pio(phys_addr_t address)
> >  {
> >  #ifdef PCI_IOBASE
> >  	struct io_range *res;
> > -	resource_size_t offset = 0;
> > +	resource_size_t offset = PCIBIOS_MIN_IO;
> >  	unsigned long addr = -1;
> >
> >  	spin_lock(&io_range_lock);
> > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > index 3786473..deec469 100644
> > --- a/include/linux/of_address.h
> > +++ b/include/linux/of_address.h
> > @@ -24,6 +24,23 @@ struct of_pci_range {
> >  #define for_each_of_pci_range(parser, range) \
> >  	for (; of_pci_range_parser_one(parser, range);)
> >
> > +
> > +#ifndef indirect_io_enabled
> > +#define indirect_io_enabled indirect_io_enabled
> > +static inline bool indirect_io_enabled(void)
> > +{
> > +	return false;
> > +}
> > +#endif
> > +
> > +#ifndef addr_is_indirect_io
> > +#define addr_is_indirect_io addr_is_indirect_io
> > +static inline int addr_is_indirect_io(u64 taddr)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> >  /* Translate a DMA address from device space to CPU space */
> >  extern u64 of_translate_dma_address(struct device_node *dev,
> >  				    const __be32 *in_addr);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 0e49f70..7f6bbb6 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> pci_bus *bus)
> >  /* provide the legacy pci_dma_* API */
> >  #include <linux/pci-dma-compat.h>
> >
> > +/*
> > + * define this macro here to refrain from compilation error for some
> > + * platforms. Please keep this macro at the end of this header file.
> > + */
> > +#ifndef PCIBIOS_MIN_IO
> > +#define PCIBIOS_MIN_IO		0
> > +#endif
> > +
> >  #endif /* LINUX_PCI_H */
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[liviu.dudau-5wv7dgnIgG8]

On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
> Hi Liviu
> 
> Thanks for reviewing
> 

[removed some irrelevant part of discussion, avoid crazy formatting]

> > > +/**
> > > + * addr_is_indirect_io - check whether the input taddr is for
> > indirectIO.
> > > + * @taddr: the io address to be checked.
> > > + *
> > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > + */
> > > +int addr_is_indirect_io(u64 taddr)
> > > +{
> > > +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
> > taddr)
> > 
> > start >= taddr ?
> 
> Nope... if  (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
> then taddr is outside the range [start; end] and will return 0; otherwise
> it will return 1...

Oops, sorry, did not pay attention to the returned value. The check is
correct as it is, no need to change then.

> 
> > 
> > > +		return 0;
> > > +
> > > +	return 1;
> > > +}
> > >
> > >  BUILD_EXTIO(b, u8)
> > >
> > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > index 02b2903..cc2a05d 100644
> > > --- a/drivers/of/address.c
> > > +++ b/drivers/of/address.c
> > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > device_node *np)
> > >  	return false;
> > >  }
> > >
> > > +
> > > +/*
> > > + * of_isa_indirect_io - get the IO address from some isa reg
> > property value.
> > > + *	For some isa/lpc devices, no ranges property in ancestor node.
> > > + *	The device addresses are described directly in their regs
> > property.
> > > + *	This fixup function will be called to get the IO address of
> > isa/lpc
> > > + *	devices when the normal of_translation failed.
> > > + *
> > > + * @parent:	points to the parent dts node;
> > > + * @bus:		points to the of_bus which can be used to parse
> > address;
> > > + * @addr:	the address from reg property;
> > > + * @na:		the address cell counter of @addr;
> > > + * @presult:	store the address paresed from @addr;
> > > + *
> > > + * return 1 when successfully get the I/O address;
> > > + * 0 will return for some failures.
> > 
> > Bah, you are returning a signed int, why 0 for failure? Return a
> > negative value with
> > error codes. Otherwise change the return value into a bool.
> 
> Yes we'll move to bool
> 
> > 
> > > + */
> > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > +				struct of_bus *bus, __be32 *addr,
> > > +				int na, u64 *presult)
> > > +{
> > > +	unsigned int flags;
> > > +	unsigned int rlen;
> > > +
> > > +	/* whether support indirectIO */
> > > +	if (!indirect_io_enabled())
> > > +		return 0;
> > > +
> > > +	if (!of_bus_isa_match(parent))
> > > +		return 0;
> > > +
> > > +	flags = bus->get_flags(addr);
> > > +	if (!(flags & IORESOURCE_IO))
> > > +		return 0;
> > > +
> > > +	/* there is ranges property, apply the normal translation
> > directly. */
> > 
> > s/there is ranges/if we have a 'ranges'/
> 
> Thanks for spotting this
> 
> > 
> > > +	if (of_get_property(parent, "ranges", &rlen))
> > > +		return 0;
> > > +
> > > +	*presult = of_read_number(addr + 1, na - 1);
> > > +	/* this fixup is only valid for specific I/O range. */
> > > +	return addr_is_indirect_io(*presult);
> > > +}
> > > +
> > >  static int of_translate_one(struct device_node *parent, struct
> > of_bus *bus,
> > >  			    struct of_bus *pbus, __be32 *addr,
> > >  			    int na, int ns, int pna, const char *rprop)
> > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > device_node *dev,
> > >  			result = of_read_number(addr, na);
> > >  			break;
> > >  		}
> > > +		/*
> > > +		 * For indirectIO device which has no ranges property, get
> > > +		 * the address from reg directly.
> > > +		 */
> > > +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> > > +			pr_debug("isa indirectIO matched(%s)..addr =
> > 0x%llx\n",
> > > +				of_node_full_name(dev), result);
> > > +			break;
> > > +		}
> > >
> > >  		/* Get new parent bus and counts */
> > >  		pbus = of_match_bus(parent);
> > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > device_node *dev,
> > >  	if (taddr == OF_BAD_ADDR)
> > >  		return -EINVAL;
> > >  	memset(r, 0, sizeof(struct resource));
> > > -	if (flags & IORESOURCE_IO) {
> > > +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > >  		unsigned long port;
> > > +
> > >  		port = pci_address_to_pio(taddr);
> > >  		if (port == (unsigned long)-1)
> > >  			return -EINVAL;
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index ba34907..1a08511 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
> > addr, resource_size_t size)
> > >
> > >  #ifdef PCI_IOBASE
> > >  	struct io_range *range;
> > > -	resource_size_t allocated_size = 0;
> > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > >
> > >  	/* check if the range hasn't been previously recorded */
> > >  	spin_lock(&io_range_lock);
> > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long
> > pio)
> > >
> > >  #ifdef PCI_IOBASE
> > >  	struct io_range *range;
> > > -	resource_size_t allocated_size = 0;
> > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > 
> > Have you checked that pci_pio_to_address still returns valid values
> > after this? I know that
> > you are trying to take into account PCIBIOS_MIN_IO limit when
> > allocating reserving the IO ranges,
> > but the values added in the io_range_list are still starting from zero,
> > no from PCIBIOS_MIN_IO,
> 
> I think you're wrong here as in pci_address_to_pio we have:
> +	resource_size_t offset = PCIBIOS_MIN_IO;
> 
> This should be enough to guarantee that the PIOs start at
> PCIBIOS_MIN_IO...right?

I don't think you can guarantee that the pio value that gets passed into
pci_pio_to_address() always comes from a previously returned value by
pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()

	if (pio < PCIBIOS_MIN_IO)
		return address;

to avoid adding more checks in the list_for_each_entry() loop.

Best regards,
Liviu

> 
> 
> > so the calculation of the address in this function could return
> > negative values casted to pci_addr_t.
> > 
> > Maybe you want to adjust the range->start value in
> > pci_register_io_range() as well to have it
> > offset by PCIBIOS_MIN_IO as well.
> > 
> > Best regards,
> > Liviu
> > 
> > >
> > >  	if (pio > IO_SPACE_LIMIT)
> > >  		return address;
> > > @@ -3335,7 +3335,7 @@ unsigned long __weak
> > pci_address_to_pio(phys_addr_t address)
> > >  {
> > >  #ifdef PCI_IOBASE
> > >  	struct io_range *res;
> > > -	resource_size_t offset = 0;
> > > +	resource_size_t offset = PCIBIOS_MIN_IO;
> > >  	unsigned long addr = -1;
> > >
> > >  	spin_lock(&io_range_lock);
> > > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > > index 3786473..deec469 100644
> > > --- a/include/linux/of_address.h
> > > +++ b/include/linux/of_address.h
> > > @@ -24,6 +24,23 @@ struct of_pci_range {
> > >  #define for_each_of_pci_range(parser, range) \
> > >  	for (; of_pci_range_parser_one(parser, range);)
> > >
> > > +
> > > +#ifndef indirect_io_enabled
> > > +#define indirect_io_enabled indirect_io_enabled
> > > +static inline bool indirect_io_enabled(void)
> > > +{
> > > +	return false;
> > > +}
> > > +#endif
> > > +
> > > +#ifndef addr_is_indirect_io
> > > +#define addr_is_indirect_io addr_is_indirect_io
> > > +static inline int addr_is_indirect_io(u64 taddr)
> > > +{
> > > +	return 0;
> > > +}
> > > +#endif
> > > +
> > >  /* Translate a DMA address from device space to CPU space */
> > >  extern u64 of_translate_dma_address(struct device_node *dev,
> > >  				    const __be32 *in_addr);
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 0e49f70..7f6bbb6 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> > pci_bus *bus)
> > >  /* provide the legacy pci_dma_* API */
> > >  #include <linux/pci-dma-compat.h>
> > >
> > > +/*
> > > + * define this macro here to refrain from compilation error for some
> > > + * platforms. Please keep this macro at the end of this header file.
> > > + */
> > > +#ifndef PCIBIOS_MIN_IO
> > > +#define PCIBIOS_MIN_IO		0
> > > +#endif
> > > +
> > >  #endif /* LINUX_PCI_H */
> > > --
> > > 1.9.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-pci"
> > in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[zhichang.yuan]

Hi,Liviu,

Thanks for your comments!


On 2016/11/10 0:50, liviu.dudau@arm.com wrote:
> On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
>> Hi Liviu
>>
>> Thanks for reviewing
>>
> 
> [removed some irrelevant part of discussion, avoid crazy formatting]
> 
>>>> +/**
>>>> + * addr_is_indirect_io - check whether the input taddr is for
>>> indirectIO.
>>>> + * @taddr: the io address to be checked.
>>>> + *
>>>> + * Returns 1 when taddr is in the range; otherwise return 0.
>>>> + */
>>>> +int addr_is_indirect_io(u64 taddr)
>>>> +{
>>>> +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
>>> taddr)
>>>
>>> start >= taddr ?
>>
>> Nope... if  (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
>> then taddr is outside the range [start; end] and will return 0; otherwise
>> it will return 1...
> 
> Oops, sorry, did not pay attention to the returned value. The check is
> correct as it is, no need to change then.
> 
>>
>>>
>>>> +		return 0;
>>>> +
>>>> +	return 1;
>>>> +}
>>>>
>>>>  BUILD_EXTIO(b, u8)
>>>>
>>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>>>> index 02b2903..cc2a05d 100644
>>>> --- a/drivers/of/address.c
>>>> +++ b/drivers/of/address.c
>>>> @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
>>> device_node *np)
>>>>  	return false;
>>>>  }
>>>>
>>>> +
>>>> +/*
>>>> + * of_isa_indirect_io - get the IO address from some isa reg
>>> property value.
>>>> + *	For some isa/lpc devices, no ranges property in ancestor node.
>>>> + *	The device addresses are described directly in their regs
>>> property.
>>>> + *	This fixup function will be called to get the IO address of
>>> isa/lpc
>>>> + *	devices when the normal of_translation failed.
>>>> + *
>>>> + * @parent:	points to the parent dts node;
>>>> + * @bus:		points to the of_bus which can be used to parse
>>> address;
>>>> + * @addr:	the address from reg property;
>>>> + * @na:		the address cell counter of @addr;
>>>> + * @presult:	store the address paresed from @addr;
>>>> + *
>>>> + * return 1 when successfully get the I/O address;
>>>> + * 0 will return for some failures.
>>>
>>> Bah, you are returning a signed int, why 0 for failure? Return a
>>> negative value with
>>> error codes. Otherwise change the return value into a bool.
>>
>> Yes we'll move to bool
>>
>>>
>>>> + */
>>>> +static int of_get_isa_indirect_io(struct device_node *parent,
>>>> +				struct of_bus *bus, __be32 *addr,
>>>> +				int na, u64 *presult)
>>>> +{
>>>> +	unsigned int flags;
>>>> +	unsigned int rlen;
>>>> +
>>>> +	/* whether support indirectIO */
>>>> +	if (!indirect_io_enabled())
>>>> +		return 0;
>>>> +
>>>> +	if (!of_bus_isa_match(parent))
>>>> +		return 0;
>>>> +
>>>> +	flags = bus->get_flags(addr);
>>>> +	if (!(flags & IORESOURCE_IO))
>>>> +		return 0;
>>>> +
>>>> +	/* there is ranges property, apply the normal translation
>>> directly. */
>>>
>>> s/there is ranges/if we have a 'ranges'/
>>
>> Thanks for spotting this
>>
>>>
>>>> +	if (of_get_property(parent, "ranges", &rlen))
>>>> +		return 0;
>>>> +
>>>> +	*presult = of_read_number(addr + 1, na - 1);
>>>> +	/* this fixup is only valid for specific I/O range. */
>>>> +	return addr_is_indirect_io(*presult);
>>>> +}
>>>> +
>>>>  static int of_translate_one(struct device_node *parent, struct
>>> of_bus *bus,
>>>>  			    struct of_bus *pbus, __be32 *addr,
>>>>  			    int na, int ns, int pna, const char *rprop)
>>>> @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
>>> device_node *dev,
>>>>  			result = of_read_number(addr, na);
>>>>  			break;
>>>>  		}
>>>> +		/*
>>>> +		 * For indirectIO device which has no ranges property, get
>>>> +		 * the address from reg directly.
>>>> +		 */
>>>> +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
>>>> +			pr_debug("isa indirectIO matched(%s)..addr =
>>> 0x%llx\n",
>>>> +				of_node_full_name(dev), result);
>>>> +			break;
>>>> +		}
>>>>
>>>>  		/* Get new parent bus and counts */
>>>>  		pbus = of_match_bus(parent);
>>>> @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
>>> device_node *dev,
>>>>  	if (taddr == OF_BAD_ADDR)
>>>>  		return -EINVAL;
>>>>  	memset(r, 0, sizeof(struct resource));
>>>> -	if (flags & IORESOURCE_IO) {
>>>> +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
>>>>  		unsigned long port;
>>>> +
>>>>  		port = pci_address_to_pio(taddr);
>>>>  		if (port == (unsigned long)-1)
>>>>  			return -EINVAL;
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index ba34907..1a08511 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
>>> addr, resource_size_t size)
>>>>
>>>>  #ifdef PCI_IOBASE
>>>>  	struct io_range *range;
>>>> -	resource_size_t allocated_size = 0;
>>>> +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
>>>>
>>>>  	/* check if the range hasn't been previously recorded */
>>>>  	spin_lock(&io_range_lock);
>>>> @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long
>>> pio)
>>>>
>>>>  #ifdef PCI_IOBASE
>>>>  	struct io_range *range;
>>>> -	resource_size_t allocated_size = 0;
>>>> +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
>>>
>>> Have you checked that pci_pio_to_address still returns valid values
>>> after this? I know that
>>> you are trying to take into account PCIBIOS_MIN_IO limit when
>>> allocating reserving the IO ranges,
>>> but the values added in the io_range_list are still starting from zero,
>>> no from PCIBIOS_MIN_IO,
>>
>> I think you're wrong here as in pci_address_to_pio we have:
>> +	resource_size_t offset = PCIBIOS_MIN_IO;
>>
>> This should be enough to guarantee that the PIOs start at
>> PCIBIOS_MIN_IO...right?
> 
> I don't think you can guarantee that the pio value that gets passed into
> pci_pio_to_address() always comes from a previously returned value by
> pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()
> 
> 	if (pio < PCIBIOS_MIN_IO)
> 		return address;
> 
> to avoid adding more checks in the list_for_each_entry() loop.
> 

I will register some ranges to the list and test it later.

But from my understanding, pci_pio_to_address() should can return the right
original physical address.


According to the algorithm, the output PIO ranges are consecutive， just like this:


					input pio of pci_pio_to_address()
						|
						V
|----------------|--------------------------|------|-----------|
					    ^
					    |
					allocated_size is here


The change of this patch just make the start PIO from ZERO to PCIBIOS_MIN_IO.

in pci_pio_to_address(), for one input pio which fall into any PIO segment, the
return address will be:

address = range->start + pio - allocated_size;

Since allocated_size is the total range size of all IO ranges before the one
where pio belong, then (pio - allocated_size) is the offset to the range start,
So....


Thanks!
Zhichang

> Best regards,
> Liviu
> 
>>
>>
>>> so the calculation of the address in this function could return
>>> negative values casted to pci_addr_t.
>>>
>>> Maybe you want to adjust the range->start value in
>>> pci_register_io_range() as well to have it
>>> offset by PCIBIOS_MIN_IO as well.
>>>
>>> Best regards,
>>> Liviu
>>>
>>>>
>>>>  	if (pio > IO_SPACE_LIMIT)
>>>>  		return address;
>>>> @@ -3335,7 +3335,7 @@ unsigned long __weak
>>> pci_address_to_pio(phys_addr_t address)
>>>>  {
>>>>  #ifdef PCI_IOBASE
>>>>  	struct io_range *res;
>>>> -	resource_size_t offset = 0;
>>>> +	resource_size_t offset = PCIBIOS_MIN_IO;
>>>>  	unsigned long addr = -1;
>>>>
>>>>  	spin_lock(&io_range_lock);
>>>> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
>>>> index 3786473..deec469 100644
>>>> --- a/include/linux/of_address.h
>>>> +++ b/include/linux/of_address.h
>>>> @@ -24,6 +24,23 @@ struct of_pci_range {
>>>>  #define for_each_of_pci_range(parser, range) \
>>>>  	for (; of_pci_range_parser_one(parser, range);)
>>>>
>>>> +
>>>> +#ifndef indirect_io_enabled
>>>> +#define indirect_io_enabled indirect_io_enabled
>>>> +static inline bool indirect_io_enabled(void)
>>>> +{
>>>> +	return false;
>>>> +}
>>>> +#endif
>>>> +
>>>> +#ifndef addr_is_indirect_io
>>>> +#define addr_is_indirect_io addr_is_indirect_io
>>>> +static inline int addr_is_indirect_io(u64 taddr)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>>  /* Translate a DMA address from device space to CPU space */
>>>>  extern u64 of_translate_dma_address(struct device_node *dev,
>>>>  				    const __be32 *in_addr);
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 0e49f70..7f6bbb6 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
>>> pci_bus *bus)
>>>>  /* provide the legacy pci_dma_* API */
>>>>  #include <linux/pci-dma-compat.h>
>>>>
>>>> +/*
>>>> + * define this macro here to refrain from compilation error for some
>>>> + * platforms. Please keep this macro at the end of this header file.
>>>> + */
>>>> +#ifndef PCIBIOS_MIN_IO
>>>> +#define PCIBIOS_MIN_IO		0
>>>> +#endif
>>>> +
>>>>  #endif /* LINUX_PCI_H */
>>>> --
>>>> 1.9.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci"
>>> in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[Gabriele Paoloni]

Hi Liviu

> -----Original Message-----
> From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> Sent: 09 November 2016 16:51
> To: Gabriele Paoloni
> Cc: Yuanzhichang; catalin.marinas@arm.com; will.deacon@arm.com;
> robh+dt@kernel.org; bhelgaas@google.com; mark.rutland@arm.com;
> olof@lixom.net; arnd@arndb.de; linux-arm-kernel@lists.infradead.org;
> lorenzo.pieralisi@arm.com; linux-kernel@vger.kernel.org; Linuxarm;
> devicetree@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> serial@vger.kernel.org; minyard@acm.org; benh@kernel.crashing.org;
> zourongrong@gmail.com; John Garry; zhichang.yuan02@gmail.com;
> kantyzc@163.com; xuwei (O)
> Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> special ISA
> 
> On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
> > Hi Liviu
> >
> > Thanks for reviewing
> >
> 
> [removed some irrelevant part of discussion, avoid crazy formatting]
> 
> > > > +/**
> > > > + * addr_is_indirect_io - check whether the input taddr is for
> > > indirectIO.
> > > > + * @taddr: the io address to be checked.
> > > > + *
> > > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > > + */
> > > > +int addr_is_indirect_io(u64 taddr)
> > > > +{
> > > > +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end
> <
> > > taddr)
> > >
> > > start >= taddr ?
> >
> > Nope... if  (taddr < arm64_extio_ops->start || taddr >
> arm64_extio_ops->end)
> > then taddr is outside the range [start; end] and will return 0;
> otherwise
> > it will return 1...
> 
> Oops, sorry, did not pay attention to the returned value. The check is
> correct as it is, no need to change then.
> 
> >
> > >
> > > > +		return 0;
> > > > +
> > > > +	return 1;
> > > > +}
> > > >
> > > >  BUILD_EXTIO(b, u8)
> > > >
> > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > > index 02b2903..cc2a05d 100644
> > > > --- a/drivers/of/address.c
> > > > +++ b/drivers/of/address.c
> > > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > > device_node *np)
> > > >  	return false;
> > > >  }
> > > >
> > > > +
> > > > +/*
> > > > + * of_isa_indirect_io - get the IO address from some isa reg
> > > property value.
> > > > + *	For some isa/lpc devices, no ranges property in ancestor
> node.
> > > > + *	The device addresses are described directly in their regs
> > > property.
> > > > + *	This fixup function will be called to get the IO address of
> > > isa/lpc
> > > > + *	devices when the normal of_translation failed.
> > > > + *
> > > > + * @parent:	points to the parent dts node;
> > > > + * @bus:		points to the of_bus which can be used to parse
> > > address;
> > > > + * @addr:	the address from reg property;
> > > > + * @na:		the address cell counter of @addr;
> > > > + * @presult:	store the address paresed from @addr;
> > > > + *
> > > > + * return 1 when successfully get the I/O address;
> > > > + * 0 will return for some failures.
> > >
> > > Bah, you are returning a signed int, why 0 for failure? Return a
> > > negative value with
> > > error codes. Otherwise change the return value into a bool.
> >
> > Yes we'll move to bool
> >
> > >
> > > > + */
> > > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > > +				struct of_bus *bus, __be32 *addr,
> > > > +				int na, u64 *presult)
> > > > +{
> > > > +	unsigned int flags;
> > > > +	unsigned int rlen;
> > > > +
> > > > +	/* whether support indirectIO */
> > > > +	if (!indirect_io_enabled())
> > > > +		return 0;
> > > > +
> > > > +	if (!of_bus_isa_match(parent))
> > > > +		return 0;
> > > > +
> > > > +	flags = bus->get_flags(addr);
> > > > +	if (!(flags & IORESOURCE_IO))
> > > > +		return 0;
> > > > +
> > > > +	/* there is ranges property, apply the normal translation
> > > directly. */
> > >
> > > s/there is ranges/if we have a 'ranges'/
> >
> > Thanks for spotting this
> >
> > >
> > > > +	if (of_get_property(parent, "ranges", &rlen))
> > > > +		return 0;
> > > > +
> > > > +	*presult = of_read_number(addr + 1, na - 1);
> > > > +	/* this fixup is only valid for specific I/O range. */
> > > > +	return addr_is_indirect_io(*presult);
> > > > +}
> > > > +
> > > >  static int of_translate_one(struct device_node *parent, struct
> > > of_bus *bus,
> > > >  			    struct of_bus *pbus, __be32 *addr,
> > > >  			    int na, int ns, int pna, const char *rprop)
> > > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > > device_node *dev,
> > > >  			result = of_read_number(addr, na);
> > > >  			break;
> > > >  		}
> > > > +		/*
> > > > +		 * For indirectIO device which has no ranges
> property, get
> > > > +		 * the address from reg directly.
> > > > +		 */
> > > > +		if (of_get_isa_indirect_io(dev, bus, addr, na,
> &result)) {
> > > > +			pr_debug("isa indirectIO matched(%s)..addr =
> > > 0x%llx\n",
> > > > +				of_node_full_name(dev), result);
> > > > +			break;
> > > > +		}
> > > >
> > > >  		/* Get new parent bus and counts */
> > > >  		pbus = of_match_bus(parent);
> > > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > > device_node *dev,
> > > >  	if (taddr == OF_BAD_ADDR)
> > > >  		return -EINVAL;
> > > >  	memset(r, 0, sizeof(struct resource));
> > > > -	if (flags & IORESOURCE_IO) {
> > > > +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > > >  		unsigned long port;
> > > > +
> > > >  		port = pci_address_to_pio(taddr);
> > > >  		if (port == (unsigned long)-1)
> > > >  			return -EINVAL;
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index ba34907..1a08511 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -3263,7 +3263,7 @@ int __weak
> pci_register_io_range(phys_addr_t
> > > addr, resource_size_t size)
> > > >
> > > >  #ifdef PCI_IOBASE
> > > >  	struct io_range *range;
> > > > -	resource_size_t allocated_size = 0;
> > > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > >
> > > >  	/* check if the range hasn't been previously recorded */
> > > >  	spin_lock(&io_range_lock);
> > > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned
> long
> > > pio)
> > > >
> > > >  #ifdef PCI_IOBASE
> > > >  	struct io_range *range;
> > > > -	resource_size_t allocated_size = 0;
> > > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > >
> > > Have you checked that pci_pio_to_address still returns valid values
> > > after this? I know that
> > > you are trying to take into account PCIBIOS_MIN_IO limit when
> > > allocating reserving the IO ranges,
> > > but the values added in the io_range_list are still starting from
> zero,
> > > no from PCIBIOS_MIN_IO,
> >
> > I think you're wrong here as in pci_address_to_pio we have:
> > +	resource_size_t offset = PCIBIOS_MIN_IO;
> >
> > This should be enough to guarantee that the PIOs start at
> > PCIBIOS_MIN_IO...right?
> 
> I don't think you can guarantee that the pio value that gets passed
> into
> pci_pio_to_address() always comes from a previously returned value by
> pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()

Maybe I am missing something...could you make an exampleof a case
where an IO toke doesn’t come from pci_address_to_pio() ?

Thanks

Gab


> 
> 	if (pio < PCIBIOS_MIN_IO)
> 		return address;
> 
> to avoid adding more checks in the list_for_each_entry() loop.
> 
> Best regards,
> Liviu
> 
> >
> >
> > > so the calculation of the address in this function could return
> > > negative values casted to pci_addr_t.
> > >
> > > Maybe you want to adjust the range->start value in
> > > pci_register_io_range() as well to have it
> > > offset by PCIBIOS_MIN_IO as well.
> > >
> > > Best regards,
> > > Liviu
> > >
> > > >
> > > >  	if (pio > IO_SPACE_LIMIT)
> > > >  		return address;
> > > > @@ -3335,7 +3335,7 @@ unsigned long __weak
> > > pci_address_to_pio(phys_addr_t address)
> > > >  {
> > > >  #ifdef PCI_IOBASE
> > > >  	struct io_range *res;
> > > > -	resource_size_t offset = 0;
> > > > +	resource_size_t offset = PCIBIOS_MIN_IO;
> > > >  	unsigned long addr = -1;
> > > >
> > > >  	spin_lock(&io_range_lock);
> > > > diff --git a/include/linux/of_address.h
> b/include/linux/of_address.h
> > > > index 3786473..deec469 100644
> > > > --- a/include/linux/of_address.h
> > > > +++ b/include/linux/of_address.h
> > > > @@ -24,6 +24,23 @@ struct of_pci_range {
> > > >  #define for_each_of_pci_range(parser, range) \
> > > >  	for (; of_pci_range_parser_one(parser, range);)
> > > >
> > > > +
> > > > +#ifndef indirect_io_enabled
> > > > +#define indirect_io_enabled indirect_io_enabled
> > > > +static inline bool indirect_io_enabled(void)
> > > > +{
> > > > +	return false;
> > > > +}
> > > > +#endif
> > > > +
> > > > +#ifndef addr_is_indirect_io
> > > > +#define addr_is_indirect_io addr_is_indirect_io
> > > > +static inline int addr_is_indirect_io(u64 taddr)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > > +#endif
> > > > +
> > > >  /* Translate a DMA address from device space to CPU space */
> > > >  extern u64 of_translate_dma_address(struct device_node *dev,
> > > >  				    const __be32 *in_addr);
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index 0e49f70..7f6bbb6 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> > > pci_bus *bus)
> > > >  /* provide the legacy pci_dma_* API */
> > > >  #include <linux/pci-dma-compat.h>
> > > >
> > > > +/*
> > > > + * define this macro here to refrain from compilation error for
> some
> > > > + * platforms. Please keep this macro at the end of this header
> file.
> > > > + */
> > > > +#ifndef PCIBIOS_MIN_IO
> > > > +#define PCIBIOS_MIN_IO		0
> > > > +#endif
> > > > +
> > > >  #endif /* LINUX_PCI_H */
> > > > --
> > > > 1.9.1
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-
> pci"
> > > in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-
> info.html
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[liviu.dudau]

On Thu, Nov 10, 2016 at 04:06:40PM +0000, Gabriele Paoloni wrote:
> Hi Liviu
> 
> > -----Original Message-----
> > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> > Sent: 09 November 2016 16:51
> > To: Gabriele Paoloni
> > Cc: Yuanzhichang; catalin.marinas@arm.com; will.deacon@arm.com;
> > robh+dt@kernel.org; bhelgaas@google.com; mark.rutland@arm.com;
> > olof@lixom.net; arnd@arndb.de; linux-arm-kernel@lists.infradead.org;
> > lorenzo.pieralisi@arm.com; linux-kernel@vger.kernel.org; Linuxarm;
> > devicetree@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> > serial@vger.kernel.org; minyard@acm.org; benh@kernel.crashing.org;
> > zourongrong@gmail.com; John Garry; zhichang.yuan02@gmail.com;
> > kantyzc@163.com; xuwei (O)
> > Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> > special ISA
> > 
> > On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
> > > Hi Liviu
> > >
> > > Thanks for reviewing
> > >
> > 
> > [removed some irrelevant part of discussion, avoid crazy formatting]
> > 
> > > > > +/**
> > > > > + * addr_is_indirect_io - check whether the input taddr is for
> > > > indirectIO.
> > > > > + * @taddr: the io address to be checked.
> > > > > + *
> > > > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > > > + */
> > > > > +int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end
> > <
> > > > taddr)
> > > >
> > > > start >= taddr ?
> > >
> > > Nope... if  (taddr < arm64_extio_ops->start || taddr >
> > arm64_extio_ops->end)
> > > then taddr is outside the range [start; end] and will return 0;
> > otherwise
> > > it will return 1...
> > 
> > Oops, sorry, did not pay attention to the returned value. The check is
> > correct as it is, no need to change then.
> > 
> > >
> > > >
> > > > > +		return 0;
> > > > > +
> > > > > +	return 1;
> > > > > +}
> > > > >
> > > > >  BUILD_EXTIO(b, u8)
> > > > >
> > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > > > index 02b2903..cc2a05d 100644
> > > > > --- a/drivers/of/address.c
> > > > > +++ b/drivers/of/address.c
> > > > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > > > device_node *np)
> > > > >  	return false;
> > > > >  }
> > > > >
> > > > > +
> > > > > +/*
> > > > > + * of_isa_indirect_io - get the IO address from some isa reg
> > > > property value.
> > > > > + *	For some isa/lpc devices, no ranges property in ancestor
> > node.
> > > > > + *	The device addresses are described directly in their regs
> > > > property.
> > > > > + *	This fixup function will be called to get the IO address of
> > > > isa/lpc
> > > > > + *	devices when the normal of_translation failed.
> > > > > + *
> > > > > + * @parent:	points to the parent dts node;
> > > > > + * @bus:		points to the of_bus which can be used to parse
> > > > address;
> > > > > + * @addr:	the address from reg property;
> > > > > + * @na:		the address cell counter of @addr;
> > > > > + * @presult:	store the address paresed from @addr;
> > > > > + *
> > > > > + * return 1 when successfully get the I/O address;
> > > > > + * 0 will return for some failures.
> > > >
> > > > Bah, you are returning a signed int, why 0 for failure? Return a
> > > > negative value with
> > > > error codes. Otherwise change the return value into a bool.
> > >
> > > Yes we'll move to bool
> > >
> > > >
> > > > > + */
> > > > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > > > +				struct of_bus *bus, __be32 *addr,
> > > > > +				int na, u64 *presult)
> > > > > +{
> > > > > +	unsigned int flags;
> > > > > +	unsigned int rlen;
> > > > > +
> > > > > +	/* whether support indirectIO */
> > > > > +	if (!indirect_io_enabled())
> > > > > +		return 0;
> > > > > +
> > > > > +	if (!of_bus_isa_match(parent))
> > > > > +		return 0;
> > > > > +
> > > > > +	flags = bus->get_flags(addr);
> > > > > +	if (!(flags & IORESOURCE_IO))
> > > > > +		return 0;
> > > > > +
> > > > > +	/* there is ranges property, apply the normal translation
> > > > directly. */
> > > >
> > > > s/there is ranges/if we have a 'ranges'/
> > >
> > > Thanks for spotting this
> > >
> > > >
> > > > > +	if (of_get_property(parent, "ranges", &rlen))
> > > > > +		return 0;
> > > > > +
> > > > > +	*presult = of_read_number(addr + 1, na - 1);
> > > > > +	/* this fixup is only valid for specific I/O range. */
> > > > > +	return addr_is_indirect_io(*presult);
> > > > > +}
> > > > > +
> > > > >  static int of_translate_one(struct device_node *parent, struct
> > > > of_bus *bus,
> > > > >  			    struct of_bus *pbus, __be32 *addr,
> > > > >  			    int na, int ns, int pna, const char *rprop)
> > > > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > > > device_node *dev,
> > > > >  			result = of_read_number(addr, na);
> > > > >  			break;
> > > > >  		}
> > > > > +		/*
> > > > > +		 * For indirectIO device which has no ranges
> > property, get
> > > > > +		 * the address from reg directly.
> > > > > +		 */
> > > > > +		if (of_get_isa_indirect_io(dev, bus, addr, na,
> > &result)) {
> > > > > +			pr_debug("isa indirectIO matched(%s)..addr =
> > > > 0x%llx\n",
> > > > > +				of_node_full_name(dev), result);
> > > > > +			break;
> > > > > +		}
> > > > >
> > > > >  		/* Get new parent bus and counts */
> > > > >  		pbus = of_match_bus(parent);
> > > > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > > > device_node *dev,
> > > > >  	if (taddr == OF_BAD_ADDR)
> > > > >  		return -EINVAL;
> > > > >  	memset(r, 0, sizeof(struct resource));
> > > > > -	if (flags & IORESOURCE_IO) {
> > > > > +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > > > >  		unsigned long port;
> > > > > +
> > > > >  		port = pci_address_to_pio(taddr);
> > > > >  		if (port == (unsigned long)-1)
> > > > >  			return -EINVAL;
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index ba34907..1a08511 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -3263,7 +3263,7 @@ int __weak
> > pci_register_io_range(phys_addr_t
> > > > addr, resource_size_t size)
> > > > >
> > > > >  #ifdef PCI_IOBASE
> > > > >  	struct io_range *range;
> > > > > -	resource_size_t allocated_size = 0;
> > > > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > > >
> > > > >  	/* check if the range hasn't been previously recorded */
> > > > >  	spin_lock(&io_range_lock);
> > > > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned
> > long
> > > > pio)
> > > > >
> > > > >  #ifdef PCI_IOBASE
> > > > >  	struct io_range *range;
> > > > > -	resource_size_t allocated_size = 0;
> > > > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > >
> > > > Have you checked that pci_pio_to_address still returns valid values
> > > > after this? I know that
> > > > you are trying to take into account PCIBIOS_MIN_IO limit when
> > > > allocating reserving the IO ranges,
> > > > but the values added in the io_range_list are still starting from
> > zero,
> > > > no from PCIBIOS_MIN_IO,
> > >
> > > I think you're wrong here as in pci_address_to_pio we have:
> > > +	resource_size_t offset = PCIBIOS_MIN_IO;
> > >
> > > This should be enough to guarantee that the PIOs start at
> > > PCIBIOS_MIN_IO...right?
> > 
> > I don't think you can guarantee that the pio value that gets passed
> > into
> > pci_pio_to_address() always comes from a previously returned value by
> > pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()
> 
> Maybe I am missing something...could you make an exampleof a case
> where an IO toke doesn’t come from pci_address_to_pio() ?

Don't know, maybe it is coming from some DT or platform data? I was just
asking for confirmation that no one has seen issues there, I'm not saying
I've seen it happening.

Best regards,
Liviu

> 
> Thanks
> 
> Gab
> 
> 
> > 
> > 	if (pio < PCIBIOS_MIN_IO)
> > 		return address;
> > 
> > to avoid adding more checks in the list_for_each_entry() loop.
> > 
> > Best regards,
> > Liviu
> > 
> > >
> > >
> > > > so the calculation of the address in this function could return
> > > > negative values casted to pci_addr_t.
> > > >
> > > > Maybe you want to adjust the range->start value in
> > > > pci_register_io_range() as well to have it
> > > > offset by PCIBIOS_MIN_IO as well.
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > >
> > > > >  	if (pio > IO_SPACE_LIMIT)
> > > > >  		return address;
> > > > > @@ -3335,7 +3335,7 @@ unsigned long __weak
> > > > pci_address_to_pio(phys_addr_t address)
> > > > >  {
> > > > >  #ifdef PCI_IOBASE
> > > > >  	struct io_range *res;
> > > > > -	resource_size_t offset = 0;
> > > > > +	resource_size_t offset = PCIBIOS_MIN_IO;
> > > > >  	unsigned long addr = -1;
> > > > >
> > > > >  	spin_lock(&io_range_lock);
> > > > > diff --git a/include/linux/of_address.h
> > b/include/linux/of_address.h
> > > > > index 3786473..deec469 100644
> > > > > --- a/include/linux/of_address.h
> > > > > +++ b/include/linux/of_address.h
> > > > > @@ -24,6 +24,23 @@ struct of_pci_range {
> > > > >  #define for_each_of_pci_range(parser, range) \
> > > > >  	for (; of_pci_range_parser_one(parser, range);)
> > > > >
> > > > > +
> > > > > +#ifndef indirect_io_enabled
> > > > > +#define indirect_io_enabled indirect_io_enabled
> > > > > +static inline bool indirect_io_enabled(void)
> > > > > +{
> > > > > +	return false;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > +#ifndef addr_is_indirect_io
> > > > > +#define addr_is_indirect_io addr_is_indirect_io
> > > > > +static inline int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > +	return 0;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > >  /* Translate a DMA address from device space to CPU space */
> > > > >  extern u64 of_translate_dma_address(struct device_node *dev,
> > > > >  				    const __be32 *in_addr);
> > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > > index 0e49f70..7f6bbb6 100644
> > > > > --- a/include/linux/pci.h
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> > > > pci_bus *bus)
> > > > >  /* provide the legacy pci_dma_* API */
> > > > >  #include <linux/pci-dma-compat.h>
> > > > >
> > > > > +/*
> > > > > + * define this macro here to refrain from compilation error for
> > some
> > > > > + * platforms. Please keep this macro at the end of this header
> > file.
> > > > > + */
> > > > > +#ifndef PCIBIOS_MIN_IO
> > > > > +#define PCIBIOS_MIN_IO		0
> > > > > +#endif
> > > > > +
> > > > >  #endif /* LINUX_PCI_H */
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-
> > pci"
> > > > in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-
> > info.html
> > 
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[Mark Rutland]

On Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:
> +Hisilicon Hip06 low-pin-count device
> +  Usually LPC controller is part of PCI host bridge, so the legacy ISA ports
> +  locate on LPC bus can be accessed direclty. But some SoCs have independent
> +  LPC controller, and access the legacy ports by triggering LPC I/O cycles.
> +  Hisilicon Hip06 implements this LPC device.

s/direclty/directly/

My understanding of ISA (which may be flawed) is that it's not part of
the PCI host bridge, but rather on x86 it happens to share the IO space
with PCI.

So, how about this becomes:

  Hisilicon Hip06 SoCs implement a Low Pin Count (LPC) controller, which
  provides access to some legacy ISA devices.

I believe that we could theoretically have multiple independent LPC/ISA
busses, as is possible with PCI on !x86 systems. If the current ISA code
assumes a singleton bus, I think that's something that needs to be fixed
up more generically.

I don't see why we should need any architecture-specific code here. Why
can we not fix up the ISA bus code in drivers/of/address.c such that it
handles multiple ISA bus instances, and translates all sub-device
addresses relative to the specific bus instance?

> +Required properties:
> +- compatible: should be "hisilicon,low-pin-count"

This would be better as something like:

	"hisilicon,hip06-lpc-controller"

If it's reused in other SoCs, we can add more strings as we usually do.

> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> +- reg: base memory range where the register set of this device is mapped.
> +
> +Note:
> +  The node name before '@' must be "isa" to represent the binding stick to the
> +  ISA/EISA binding specification.
> +
> +Example:
> +
> +isa@a01b0000 {
> +	compatible = "hisilicom,low-pin-count";

s/hisilicom/hisilicon/

My comment above on the compatible string also applies.

> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	reg = <0x0 0xa01b0000 0x0 0x1000>;
> +
> +	ipmi0: bt@e4 {
> +		compatible = "ipmi-bt";
> +		device_type = "ipmi";
> +		reg = <0x01 0xe4 0x04>;
> +		status = "disabled";
> +	};
> +};

Please remove the status property; it's irrelevant to the example.

[...]

> +/**
> + * indirect_io_enabled - check whether indirectIO is enabled.
> + *	arm64_extio_ops will be set only when indirectIO mechanism had been
> + *	initialized.
> + *
> + * Returns true when indirectIO is enabled.
> + */
> +bool indirect_io_enabled(void)
> +{
> +	return arm64_extio_ops ? true : false;
> +}

	return !!arm64_extio_ops;

> +/**
> + * addr_is_indirect_io - check whether the input taddr is for indirectIO.
> + * @taddr: the io address to be checked.
> + *
> + * Returns 1 when taddr is in the range; otherwise return 0.
> + */
> +int addr_is_indirect_io(u64 taddr)
> +{
> +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end < taddr)
> +		return 0;
> +
> +	return 1;
> +}

Why not bool?

I don't think this is the right thing to do, regardless.

> + * of_isa_indirect_io - get the IO address from some isa reg property value.
> + *	For some isa/lpc devices, no ranges property in ancestor node.
> + *	The device addresses are described directly in their regs property.
> + *	This fixup function will be called to get the IO address of isa/lpc
> + *	devices when the normal of_translation failed.
> + *
> + * @parent:	points to the parent dts node;
> + * @bus:		points to the of_bus which can be used to parse address;
> + * @addr:	the address from reg property;
> + * @na:		the address cell counter of @addr;
> + * @presult:	store the address paresed from @addr;
> + *
> + * return 1 when successfully get the I/O address;
> + * 0 will return for some failures.
> + */
> +static int of_get_isa_indirect_io(struct device_node *parent,
> +				struct of_bus *bus, __be32 *addr,
> +				int na, u64 *presult)
> +{
> +	unsigned int flags;
> +	unsigned int rlen;
> +
> +	/* whether support indirectIO */
> +	if (!indirect_io_enabled())
> +		return 0;
> +
> +	if (!of_bus_isa_match(parent))
> +		return 0;
> +
> +	flags = bus->get_flags(addr);
> +	if (!(flags & IORESOURCE_IO))
> +		return 0;
> +
> +	/* there is ranges property, apply the normal translation directly. */
> +	if (of_get_property(parent, "ranges", &rlen))
> +		return 0;
> +
> +	*presult = of_read_number(addr + 1, na - 1);
> +	/* this fixup is only valid for specific I/O range. */
> +	return addr_is_indirect_io(*presult);
> +}
> +
>  static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>  			    struct of_bus *pbus, __be32 *addr,
>  			    int na, int ns, int pna, const char *rprop)
> @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct device_node *dev,
>  			result = of_read_number(addr, na);
>  			break;
>  		}
> +		/*
> +		 * For indirectIO device which has no ranges property, get
> +		 * the address from reg directly.
> +		 */
> +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> +			pr_debug("isa indirectIO matched(%s)..addr = 0x%llx\n",
> +				of_node_full_name(dev), result);
> +			break;
> +		}

I don't believe this is the right place for this to live. This should
live in the isa of_bus, matched in the usual way with of_match_bus(),
and used in pbus->translate() in of_translate_one().

If we need to extend the prototypes of those functions, we should do so.

If we need to be able to register instance-specific translations or
indirection, we should build the infrastructure for that rather than
forcing a singleton translation in here.

Thanks,
Mark.

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[Arnd Bergmann]

On Tuesday, November 8, 2016 11:49:53 AM CET Mark Rutland wrote:
> On Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:
> > +Hisilicon Hip06 low-pin-count device
> > +  Usually LPC controller is part of PCI host bridge, so the legacy ISA ports
> > +  locate on LPC bus can be accessed direclty. But some SoCs have independent
> > +  LPC controller, and access the legacy ports by triggering LPC I/O cycles.
> > +  Hisilicon Hip06 implements this LPC device.
> 
> s/direclty/directly/
> 
> My understanding of ISA (which may be flawed) is that it's not part of