[PATCH v17 00/10] LPC: legacy ISA I/O support

[PATCH v17 00/10] LPC: legacy ISA I/O support

[John Garry]

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

When master accesses those peripherals beneath the Hip06/Hip07 LPC, a specific
LPC driver is needed to make LPC host generate the standard LPC I/O cycles with
the target peripherals'I/O port addresses. But on curent arm64 world, there is
no real I/O accesses. All the I/O operations through in/out accessors are based
on MMIO ranges; on Hip06/Hip07 LPC the I/O accesses are performed through driver
specific accessors rather than MMIO.
To solve this issue and keep the relevant existing peripherals' drivers untouched,
this patchset:
   - introduces a generic I/O space management framework, logical PIO, to support
      I/O operations on host controllers operating either on MMIO buses or on buses
     requiring specific driver I/O accessors;
   - redefines the in/out accessors to provide a unified interface for both MMIO
     and driver specific I/O operations. Using logical PIO, th call of in/out() from
     the host children drivers, such as ipmi-si, will be redirected to the
     corresponding device-specific I/O hooks to perform the I/O accesses.

Based on this patch-set, all the I/O accesses to Hip06/Hip07 LPC peripherals can
be supported without any changes on the existing ipmi-si driver.

The whole patchset has been tested on Hip07 D05 board both using DTB and ACPI.

Differences to v16:
- Add patch to rename acpi_is_serial_bus_slave()
Note: Could not rebase on recent linux-next as either build broken or fails
to boot.

Differences to v15:
- addressed further coding style comments in LLDD and logic PIO code
  from Andy
- rebased to linux next 20180302

Differences to v14:
- addressed coding style comments from Andy
- applied tags from Rafael, Andy, and Dann
- rebased to linux next 20180223 (had to fix locally arm64 build issue)

Differences to v13:
- dropped ACPI scan handler and added patch to not enumerate children
  of indirect IO hosts in ACPI code
- tidied up logic_pio.c a bit for kerneldoc and made some APIs clearer
  to understand
- tided (and simplified) hisi_lpc.c and added new ACPI probe code
  (same as previous ACPI scan handler code, so comments from Rafael
  and Andy included)
- reinstated PCI range upper limit check in pci_pio_to_address()
- dropped Dann Frazier's "tested-by" tag in light of changes
- rebase to linuxnext 20180219 (had to fix locally arm64 build issue)

Differences to v12:
    - Addressed ACPI comments from Rafael and Andy, including:
     - added SPDX license identifiers (other new files in the series got this also)
     - fixed style issues, like superflous newlines and symbol naming
     - add fuller acpi_indirectio.c patch commit message
     - dropped acpi_indirectio_host_data (author's decision) to simplify
    - added Rob Herring's tag
    - rebase to linux-next 20180212
    
Differences to v11:
    - fixed build errors for i386, m68k, and tile
    - added a comment in LPC driver commit log why we set
       the kernel config as bool
    - some tidying logic_pio code

Differences to v10:
    - dropped CONFIG_LOGIC_PIO. Reason is that CONFIG_PCI
      depends on this, and CONFIG_PCI is a per-arch CONFIG.
      So we would require all arch's kconfig to select this.
    - Addressed Dann Frazier's comments on LPC driver, and
      sopme other cleanup
    - Moved logic_pio.h to be included in generic asm io.h
    - Fixed ACPI indirect IO host setup to handle >1 child
    - Relocated ACPI indirect IO host setup code to
      drivers/acpi
    - Rebased to linux next-20180118

Changes from v9:
  - patch 2 has been split into 3 patches according to Bjorn comments on
    v9 thread
  - patch 1 has been reworked accordign to Bjorn comments on v9
  - now logic_pio_trans_hwaddr() has a sanity check to make sure the resource
    size fits into the assigned range
  - in patch 5 the MFD framework has been used to probe the LPC children
    according to the suggestion from Mika Westerberg
  - Maintaner has changed to Huawei Linuxarm mailing list

Changes from v8:
  - Simplified LIB IO framewrok
  - Moved INDIRECT PIO ACPI framework under acpi/arm64
  - Renamed occurrences of "lib io" and "indirect io" to "lib pio" and
    "indirect pio" to keep the patchset nomenclature consistent
  - Removed Alignment reuqirements
  - Moved LPC specific code out of ACPI common framework
  - Now PIO indirect HW ranges can overlap
  - Changed HiSilicon LPC driver maintainer (Gabriele Paoloni now) and split
    maintaner file modifications in a separate commit
  - Removed the commit with the DT nodes support for hip06 and hip07 (to be
    pushed separately)
  - Added a checking on ioport_map() not to break that function as Arnd points
    out in V7 review thread;
  - fixed the compile issues on alpha, m68k;

Changes from V7:
  - Based on Arnd's comment, rename the LIBIO as LOGIC_PIO;
  - Improved the mapping process in LOGIC_PIO to gain better efficiency when
    redirecting the I/O accesses to right device driver;
  - To reduce the impact on PCI MMIO to a minimum, add a new
    CONFIG_INDIRECT_PIO for indirect-IO hosts/devices;
  - Added a new ACPI handler for indirect-IO hosts/devices;
  - Fixed the compile issues on V6;

Changes from V6:
  - According to the comments from Bjorn and Alex, merge PCI IO and indirect-IO
    into a generic I/O space management, LIBIO;
  - Adopted the '_DEP' to replace the platform bus notifier. In this way, we can
    ensure the LPC peripherals' I/O resources had been translated to logical IO
    before the LPC peripheral enumeration;
  - Replaced the rwlock with rcu list based on Alex's suggestion;
  - Applied relaxed write/read to LPC driver;
  - Some bugs fixing and some optimazations based on the comments of V6;

Changes from V5:
  - Made the extio driver more generic and locate in lib/;
  - Supported multiple indirect-IO bus instances;
  - Extended the pci_register_io_range() to support indirect-IO, then dropped
  the I/O reservation used in previous patchset;
  - Reimplemented the ACPI LPC support;
  - Fixed some bugs, including the compile error on other archs, the module
  building failure found by Ming Lei, etc;

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;

V16 thread here: https://lkml.org/lkml/2018/2/26/584
V15 thread here: https://lkml.org/lkml/2018/2/26/584
V14 thread here: https://lkml.org/lkml/2018/2/19/460
V13 thread here: https://lkml.org/lkml/2018/2/13/744
V12 thread here: https://lkml.org/lkml/2018/1/23/508
V11 thread here: https://lkml.org/lkml/2018/1/21/38
V10 thread here: https://lkml.org/lkml/2017/10/27/465
V9 thread here: https://lkml.org/lkml/2017/5/25/263
V8 thread here: https://lkml.org/lkml/2017/3/30/619
V7 thread here: https://lkml.org/lkml/2017/3/12/279
v6 thread here: https://lkml.org/lkml/2017/1/24/25
v5 thread here: https://lkml.org/lkml/2016/11/7/955
v4 thread here: https://lkml.org/lkml/2016/10/20/149
v3 thread here: https://lkml.org/lkml/2016/9/14/326
v2 thread here: https://lkml.org/lkml/2016/9/7/356
v1 thread here: https://lkml.org/lkml/2015/12/29/154

Gabriele Paoloni (2):
  PCI: Remove unused __weak attribute in pci_register_io_range()
  PCI: Add fwnode handler as input param of pci_register_io_range()

John Garry (4):
  ACPI / scan: rename acpi_is_serial_bus_slave() to widen use
  ACPI / scan: do not enumerate Indirect IO host children
  HISI LPC: Add ACPI support
  MAINTAINERS: Add maintainer for HiSilicon LPC driver

Zhichang Yuan (4):
  LIB: Introduce a generic PIO mapping method
  PCI: Apply the new generic I/O management on PCI IO hosts
  OF: Add missing I/O range exception for indirect-IO devices
  HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings

 .../arm/hisilicon/hisilicon-low-pin-count.txt      |  33 ++
 MAINTAINERS                                        |   7 +
 drivers/acpi/pci_root.c                            |   8 +-
 drivers/acpi/scan.c                                |  33 +-
 drivers/bus/Kconfig                                |   8 +
 drivers/bus/Makefile                               |   2 +
 drivers/bus/hisi_lpc.c                             | 623 +++++++++++++++++++++
 drivers/of/address.c                               |  96 +++-
 drivers/pci/pci.c                                  |  95 +---
 include/acpi/acpi_bus.h                            |   2 +-
 include/asm-generic/io.h                           |   4 +-
 include/linux/logic_pio.h                          | 124 ++++
 include/linux/pci.h                                |   3 +-
 lib/Kconfig                                        |  15 +
 lib/Makefile                                       |   2 +
 lib/logic_pio.c                                    | 282 ++++++++++
 16 files changed, 1229 insertions(+), 108 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
 create mode 100644 drivers/bus/hisi_lpc.c
 create mode 100644 include/linux/logic_pio.h
 create mode 100644 lib/logic_pio.c

-- 
1.9.1

[PATCH v17 01/10] LIB: Introduce a generic PIO mapping method

[John Garry]

From: Zhichang Yuan <yuanzhichang@hisilicon.com>

In commit 41f8bba7f555 ("of/pci: Add pci_register_io_range() and
pci_pio_to_address()"), a new I/O space management was supported. With
that driver, the I/O ranges configured for PCI/PCIe hosts on some
architectures can be mapped to logical PIO, converted easily between
CPU address and the corresponding logicial PIO. Based on this, PCI
I/O devices can be accessed in a memory read/write way through the
unified in/out accessors.

But on some archs/platforms, there are bus hosts which access I/O
peripherals with host-local I/O port addresses rather than memory
addresses after memory-mapped.

To support those devices, a more generic I/O mapping method is introduced
here. Through this patch, both the CPU addresses and the host-local port
can be mapped into the logical PIO space with different logical/fake PIOs.
After this, all the I/O accesses to either PCI MMIO devices or host-local
I/O peripherals can be unified into the existing I/O accessors defined in
asm-generic/io.h and be redirected to the right device-specific hooks
based on the input logical PIO.

Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Tested-by: dann frazier <dann.frazier@canonical.com>
---
 include/asm-generic/io.h  |   2 +
 include/linux/logic_pio.h | 124 ++++++++++++++++++++
 lib/Kconfig               |  15 +++
 lib/Makefile              |   2 +
 lib/logic_pio.c           | 282 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 425 insertions(+)
 create mode 100644 include/linux/logic_pio.h
 create mode 100644 lib/logic_pio.c

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7c6a39e..f76fbd6 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -351,6 +351,8 @@ static inline void writesq(volatile void __iomem *addr, const void *buffer,
 #define IO_SPACE_LIMIT 0xffff
 #endif
 
+#include <linux/logic_pio.h>
+
 /*
  * {in,out}{b,w,l}() access little endian I/O. {in,out}{b,w,l}_p() can be
  * implemented on hardware that needs an additional delay for I/O accesses to
diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
new file mode 100644
index 0000000..8c78ff4
--- /dev/null
+++ b/include/linux/logic_pio.h
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
+ * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com>
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ *
+ */
+
+#ifndef __LINUX_LOGIC_PIO_H
+#define __LINUX_LOGIC_PIO_H
+
+#include <linux/fwnode.h>
+
+enum {
+	LOGIC_PIO_INDIRECT,		/* Indirect IO flag */
+	LOGIC_PIO_CPU_MMIO,		/* Memory mapped IO flag */
+};
+
+struct logic_pio_hwaddr {
+	struct list_head list;
+	struct fwnode_handle *fwnode;
+	resource_size_t hw_start;
+	resource_size_t io_start;
+	resource_size_t size; /* range size populated */
+	unsigned long flags;
+
+	void *hostdata;
+	const struct logic_pio_host_ops *ops;
+};
+
+struct logic_pio_host_ops {
+	u32 (*in)(void *hostdata, unsigned long addr, size_t dwidth);
+	void (*out)(void *hostdata, unsigned long addr, u32 val,
+		    size_t dwidth);
+	u32 (*ins)(void *hostdata, unsigned long addr, void *buffer,
+		   size_t dwidth, unsigned int count);
+	void (*outs)(void *hostdata, unsigned long addr, const void *buffer,
+		     size_t dwidth, unsigned int count);
+};
+
+#ifdef CONFIG_INDIRECT_PIO
+u8 logic_inb(unsigned long addr);
+void logic_outb(u8 value, unsigned long addr);
+void logic_outw(u16 value, unsigned long addr);
+void logic_outl(u32 value, unsigned long addr);
+u16 logic_inw(unsigned long addr);
+u32 logic_inl(unsigned long addr);
+void logic_outb(u8 value, unsigned long addr);
+void logic_outw(u16 value, unsigned long addr);
+void logic_outl(u32 value, unsigned long addr);
+void logic_insb(unsigned long addr, void *buffer, unsigned int count);
+void logic_insl(unsigned long addr, void *buffer, unsigned int count);
+void logic_insw(unsigned long addr, void *buffer, unsigned int count);
+void logic_outsb(unsigned long addr, const void *buffer, unsigned int count);
+void logic_outsw(unsigned long addr, const void *buffer, unsigned int count);
+void logic_outsl(unsigned long addr, const void *buffer, unsigned int count);
+
+#ifndef inb
+#define inb logic_inb
+#endif
+
+#ifndef inw
+#define inw logic_inw
+#endif
+
+#ifndef inl
+#define inl logic_inl
+#endif
+
+#ifndef outb
+#define outb logic_outb
+#endif
+
+#ifndef outw
+#define outw logic_outw
+#endif
+
+#ifndef outl
+#define outl logic_outl
+#endif
+
+#ifndef insb
+#define insb logic_insb
+#endif
+
+#ifndef insw
+#define insw logic_insw
+#endif
+
+#ifndef insl
+#define insl logic_insl
+#endif
+
+#ifndef outsb
+#define outsb logic_outsb
+#endif
+
+#ifndef outsw
+#define outsw logic_outsw
+#endif
+
+#ifndef outsl
+#define outsl logic_outsl
+#endif
+
+/*
+ * Below we reserve 0x4000 bytes for Indirect IO as so far this library is only
+ * used by Hisilicon LPC Host. If needed in future we may reserve a wider IO
+ * area by redefining the macro below.
+ */
+#define PIO_INDIRECT_SIZE 0x4000
+#define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE)
+#else
+#define MMIO_UPPER_LIMIT IO_SPACE_LIMIT
+#endif /* CONFIG_INDIRECT_PIO */
+
+struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode);
+unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode,
+			resource_size_t hw_addr, resource_size_t size);
+int logic_pio_register_range(struct logic_pio_hwaddr *newrange);
+extern resource_size_t logic_pio_to_hwaddr(unsigned long pio);
+extern unsigned long logic_pio_trans_cpuaddr(resource_size_t hw_addr);
+
+#endif /* __LINUX_LOGIC_PIO_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index e960894..d9dd02c 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -55,6 +55,21 @@ config ARCH_USE_CMPXCHG_LOCKREF
 config ARCH_HAS_FAST_MULTIPLIER
 	bool
 
+config INDIRECT_PIO
+	bool "Access I/O in non-MMIO mode"
+	depends on ARM64
+	help
+	  On some platforms where no separate I/O space exists, there are I/O
+	  hosts which can not be accessed in MMIO mode. Using the logical PIO
+	  mechanism, the host-local I/O resource can be mapped into system
+	  logic PIO space shared with MMIO hosts, such as PCI/PCIE, then the
+	  system can access the I/O devices with the mapped logic PIO through
+	  I/O accessors.
+	  This way has relatively little I/O performance cost. Please make
+	  sure your devices really need this configure item enabled.
+
+	  When in doubt, say N.
+
 config CRC_CCITT
 	tristate "CRC-CCITT functions"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index 0bd50d71f..8fc0d3a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -82,6 +82,8 @@ obj-$(CONFIG_HAS_IOMEM) += iomap_copy.o devres.o
 obj-$(CONFIG_CHECK_SIGNATURE) += check_signature.o
 obj-$(CONFIG_DEBUG_LOCKING_API_SELFTESTS) += locking-selftest.o
 
+obj-y += logic_pio.o
+
 obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
 
 obj-$(CONFIG_BTREE) += btree.o
diff --git a/lib/logic_pio.c b/lib/logic_pio.c
new file mode 100644
index 0000000..8394c2d
--- /dev/null
+++ b/lib/logic_pio.c
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
+ * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com>
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ *
+ */
+
+#define pr_fmt(fmt)	"LOGIC PIO: " fmt
+
+#include <linux/of.h>
+#include <linux/io.h>
+#include <linux/logic_pio.h>
+#include <linux/mm.h>
+#include <linux/rculist.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+
+/* The unique hardware address list. */
+static LIST_HEAD(io_range_list);
+static DEFINE_MUTEX(io_range_mutex);
+
+/* Consider a kernel general helper for this */
+#define in_range(b, first, len)        ((b) >= (first) && (b) < (first) + (len))
+
+/**
+ * logic_pio_register_range - register logical PIO range for a host
+ * @new_range: pointer to the io range to be registered.
+ *
+ * returns 0 on success, the error code in case of failure
+ *
+ * Register a new io range node in the io range list.
+ */
+int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
+{
+	struct logic_pio_hwaddr *range;
+	resource_size_t start = new_range->hw_start;
+	resource_size_t end = new_range->hw_start + new_range->size;
+	resource_size_t mmio_sz = 0;
+	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
+	int ret = 0;
+
+	if (!new_range || !new_range->fwnode || !new_range->size)
+		return -EINVAL;
+
+	mutex_lock(&io_range_mutex);
+	list_for_each_entry_rcu(range, &io_range_list, list) {
+		if (range->fwnode == new_range->fwnode) {
+			/* range already there */
+			ret = -EFAULT;
+			goto end_register;
+		}
+		if (range->flags == LOGIC_PIO_CPU_MMIO &&
+		    new_range->flags == LOGIC_PIO_CPU_MMIO) {
+			/* for MMIO ranges we need to check for overlap */
+			if (start >= range->hw_start + range->size ||
+			    end < range->hw_start) {
+				mmio_sz += range->size;
+			} else {
+				ret = -EFAULT;
+				goto end_register;
+			}
+		} else if (range->flags == LOGIC_PIO_INDIRECT &&
+			   new_range->flags == LOGIC_PIO_INDIRECT) {
+			iio_sz += range->size;
+		}
+	}
+
+	/* range not registered yet, check for available space */
+	if (new_range->flags == LOGIC_PIO_CPU_MMIO) {
+		if (mmio_sz + new_range->size - 1 > MMIO_UPPER_LIMIT) {
+			/* if it's too big check if 64K space can be reserved */
+			if (mmio_sz + SZ_64K - 1 > MMIO_UPPER_LIMIT) {
+				ret = -E2BIG;
+				goto end_register;
+			}
+			new_range->size = SZ_64K;
+			pr_warn("Requested IO range too big, new size set to 64K\n");
+		}
+		new_range->io_start = mmio_sz;
+	} else if (new_range->flags == LOGIC_PIO_INDIRECT) {
+		if (iio_sz + new_range->size - 1 > IO_SPACE_LIMIT) {
+			ret = -E2BIG;
+			goto end_register;
+		}
+		new_range->io_start = iio_sz;
+	} else {
+		/* invalid flag */
+		ret = -EINVAL;
+		goto end_register;
+	}
+
+	list_add_tail_rcu(&new_range->list, &io_range_list);
+
+end_register:
+	mutex_unlock(&io_range_mutex);
+	return ret;
+}
+
+/**
+ * find_io_range_by_fwnode - find logical PIO range for given FW node
+ * @fwnode: FW node handle associated with logical PIO range
+ *
+ * Returns pointer to node on success, NULL otherwise
+ *
+ * Traverse the io_range_list to find the registered node whose device node
+ * and/or physical IO address match to.
+ */
+struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode)
+{
+	struct logic_pio_hwaddr *range;
+
+	list_for_each_entry_rcu(range, &io_range_list, list) {
+		if (range->fwnode == fwnode)
+			return range;
+	}
+	return NULL;
+}
+
+/* Return a registered range given an input PIO token */
+static struct logic_pio_hwaddr *find_io_range(unsigned long pio)
+{
+	struct logic_pio_hwaddr *range;
+
+	list_for_each_entry_rcu(range, &io_range_list, list) {
+		if (in_range(pio, range->io_start, range->size))
+			return range;
+	}
+	pr_err("PIO entry token invalid\n");
+	return NULL;
+}
+
+/**
+ * logic_pio_to_hwaddr - translate logical PIO to HW address
+ * @pio: logical PIO value
+ *
+ * Returns HW address if valid, ~0 otherwise
+ *
+ * Translate the input logical pio to the corresponding hardware address.
+ * The input pio should be unique in the whole logical PIO space.
+ */
+resource_size_t logic_pio_to_hwaddr(unsigned long pio)
+{
+	struct logic_pio_hwaddr *range;
+	resource_size_t hwaddr = (resource_size_t)~0;
+
+	range = find_io_range(pio);
+	if (range)
+		hwaddr = range->hw_start + pio - range->io_start;
+
+	return hwaddr;
+}
+
+/**
+ * logic_pio_trans_hwaddr - translate HW address to logical PIO
+ * @fwnode: FW node reference for the host
+ * @addr: Host-relative HW address
+ * @size: size to translate
+ *
+ * Returns Logical PIO value if successful, ~0UL otherwise
+ */
+unsigned long
+logic_pio_trans_hwaddr(struct fwnode_handle *fwnode, resource_size_t addr,
+		       resource_size_t size)
+{
+	struct logic_pio_hwaddr *range;
+
+	range = find_io_range_by_fwnode(fwnode);
+	if (!range || range->flags == LOGIC_PIO_CPU_MMIO) {
+		pr_err("range not found or invalid\n");
+		return ~0UL;
+	}
+	if (range->size < size) {
+		pr_err("resource size %pa cannot fit in IO range size %pa\n",
+		       &size, &range->size);
+		return ~0UL;
+	}
+	return addr - range->hw_start + range->io_start;
+}
+
+unsigned long
+logic_pio_trans_cpuaddr(resource_size_t addr)
+{
+	struct logic_pio_hwaddr *range;
+
+	list_for_each_entry_rcu(range, &io_range_list, list) {
+		if (range->flags != LOGIC_PIO_CPU_MMIO)
+			continue;
+		if (in_range(addr, range->hw_start, range->size))
+			return addr - range->hw_start + range->io_start;
+	}
+	pr_err("addr not registered in io_range_list\n");
+	return ~0UL;
+}
+
+#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
+#define BUILD_LOGIC_IO(bw, type)					\
+type logic_in##bw(unsigned long addr)					\
+{									\
+	type ret = (type)~0;						\
+									\
+	if (addr < MMIO_UPPER_LIMIT) {					\
+		ret = read##bw(PCI_IOBASE + addr);			\
+	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
+		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+									\
+		if (entry && entry->ops)				\
+			ret = entry->ops->in(entry->hostdata,		\
+					addr, sizeof(type));		\
+		else							\
+			WARN_ON_ONCE(1);				\
+	}								\
+	return ret;							\
+}									\
+									\
+void logic_out##bw(type value, unsigned long addr)			\
+{									\
+	if (addr < MMIO_UPPER_LIMIT) {					\
+		write##bw(value, PCI_IOBASE + addr);			\
+	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
+		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+									\
+		if (entry && entry->ops)				\
+			entry->ops->out(entry->hostdata,		\
+					addr, value, sizeof(type));	\
+		else							\
+			WARN_ON_ONCE(1);				\
+	}								\
+}									\
+									\
+void logic_ins##bw(unsigned long addr, void *buffer,		\
+		   unsigned int count)					\
+{									\
+	if (addr < MMIO_UPPER_LIMIT) {					\
+		reads##bw(PCI_IOBASE + addr, buffer, count);		\
+	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
+		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+									\
+		if (entry && entry->ops)				\
+			entry->ops->ins(entry->hostdata,		\
+				addr, buffer, sizeof(type), count);	\
+		else							\
+			WARN_ON_ONCE(1);				\
+	}								\
+									\
+}									\
+									\
+void logic_outs##bw(unsigned long addr, const void *buffer,		\
+		    unsigned int count)					\
+{									\
+	if (addr < MMIO_UPPER_LIMIT) {					\
+		writes##bw(PCI_IOBASE + addr, buffer, count);		\
+	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
+		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+									\
+		if (entry && entry->ops)				\
+			entry->ops->outs(entry->hostdata,		\
+				addr, buffer, sizeof(type), count);	\
+		else							\
+			WARN_ON_ONCE(1);				\
+	}								\
+}
+
+BUILD_LOGIC_IO(b, u8)
+EXPORT_SYMBOL(logic_inb);
+EXPORT_SYMBOL(logic_insb);
+EXPORT_SYMBOL(logic_outb);
+EXPORT_SYMBOL(logic_outsb);
+
+BUILD_LOGIC_IO(w, u16)
+EXPORT_SYMBOL(logic_inw);
+EXPORT_SYMBOL(logic_insw);
+EXPORT_SYMBOL(logic_outw);
+EXPORT_SYMBOL(logic_outsw);
+
+BUILD_LOGIC_IO(l, u32)
+EXPORT_SYMBOL(logic_inl);
+EXPORT_SYMBOL(logic_insl);
+EXPORT_SYMBOL(logic_outl);
+EXPORT_SYMBOL(logic_outsl);
+
+#endif /* CONFIG_INDIRECT_PIO && PCI_IOBASE */
-- 
1.9.1

[PATCH v17 02/10] PCI: Remove unused __weak attribute in pci_register_io_range()

[John Garry]

From: Gabriele Paoloni <gabriele.paoloni@huawei.com>

Currently pci_register_io_range() has only one definition;
therefore there is no use of the __weak attribute.

Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Tested-by: dann frazier <dann.frazier@canonical.com>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ae654e2..ff41a64 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3451,7 +3451,7 @@ struct io_range {
  * Record the PCI IO range (expressed as CPU physical address + size).
  * Return a negative value if an error has occured, zero otherwise
  */
-int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
+int pci_register_io_range(phys_addr_t addr, resource_size_t size)
 {
 	int err = 0;
 
-- 
1.9.1

[PATCH v17 03/10] PCI: Add fwnode handler as input param of pci_register_io_range()

[John Garry]

From: Gabriele Paoloni <gabriele.paoloni@huawei.com>

In preparation for having the PCI MMIO helpers to use the new generic
I/O space management(logical PIO) we need to add the fwnode handler as
extra input parameter.
This patch changes the signature of pci_register_io_range() and of
its callers as needed.

Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Tested-by: dann frazier <dann.frazier@canonical.com>
---
 drivers/acpi/pci_root.c | 8 +++++---
 drivers/of/address.c    | 4 +++-
 drivers/pci/pci.c       | 3 ++-
 include/linux/pci.h     | 3 ++-
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 6fc204a..1213479 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -729,7 +729,8 @@ static void acpi_pci_root_validate_resources(struct device *dev,
 	}
 }
 
-static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
+static void acpi_pci_root_remap_iospace(struct fwnode_handle *fwnode,
+			struct resource_entry *entry)
 {
 #ifdef PCI_IOBASE
 	struct resource *res = entry->res;
@@ -738,7 +739,7 @@ static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
 	resource_size_t length = resource_size(res);
 	unsigned long port;
 
-	if (pci_register_io_range(cpu_addr, length))
+	if (pci_register_io_range(fwnode, cpu_addr, length))
 		goto err;
 
 	port = pci_address_to_pio(cpu_addr);
@@ -780,7 +781,8 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
 	else {
 		resource_list_for_each_entry_safe(entry, tmp, list) {
 			if (entry->res->flags & IORESOURCE_IO)
-				acpi_pci_root_remap_iospace(entry);
+				acpi_pci_root_remap_iospace(&device->fwnode,
+						entry);
 
 			if (entry->res->flags & IORESOURCE_DISABLED)
 				resource_list_destroy_entry(entry);
diff --git a/drivers/of/address.c b/drivers/of/address.c
index ce4d3d8..cdf047b 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -2,6 +2,7 @@
 #define pr_fmt(fmt)	"OF: " fmt
 
 #include <linux/device.h>
+#include <linux/fwnode.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/module.h>
@@ -333,7 +334,8 @@ int of_pci_range_to_resource(struct of_pci_range *range,
 
 	if (res->flags & IORESOURCE_IO) {
 		unsigned long port;
-		err = pci_register_io_range(range->cpu_addr, range->size);
+		err = pci_register_io_range(&np->fwnode, range->cpu_addr,
+				range->size);
 		if (err)
 			goto invalid_range;
 		port = pci_address_to_pio(range->cpu_addr);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ff41a64..3f30b7d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3451,7 +3451,8 @@ struct io_range {
  * Record the PCI IO range (expressed as CPU physical address + size).
  * Return a negative value if an error has occured, zero otherwise
  */
-int pci_register_io_range(phys_addr_t addr, resource_size_t size)
+int pci_register_io_range(struct fwnode_handle *fwnode, phys_addr_t addr,
+			resource_size_t	size)
 {
 	int err = 0;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 25b7a35..17cc998 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1221,7 +1221,8 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
 			void *alignf_data);
 
 
-int pci_register_io_range(phys_addr_t addr, resource_size_t size);
+int pci_register_io_range(struct fwnode_handle *fwnode, phys_addr_t addr,
+			resource_size_t size);
 unsigned long pci_address_to_pio(phys_addr_t addr);
 phys_addr_t pci_pio_to_address(unsigned long pio);
 int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
-- 
1.9.1

[PATCH v17 04/10] PCI: Apply the new generic I/O management on PCI IO hosts

[John Garry]

From: Zhichang Yuan <yuanzhichang@hisilicon.com>

After introducing the new generic I/O space management(Logical PIO), the
original PCI MMIO relevant helpers need to be updated based on the new
interfaces defined in logical PIO.
This patch adapts the corresponding code to match the changes introduced
by logical PIO.

Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>        #earlier draft
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Tested-by: dann frazier <dann.frazier@canonical.com>
---
 drivers/pci/pci.c        | 92 +++++++++---------------------------------------
 include/asm-generic/io.h |  2 +-
 2 files changed, 18 insertions(+), 76 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 3f30b7d..09c2490 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -22,6 +22,7 @@
 #include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/log2.h>
+#include <linux/logic_pio.h>
 #include <linux/pci-aspm.h>
 #include <linux/pm_wakeup.h>
 #include <linux/interrupt.h>
@@ -3436,17 +3437,6 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)
 }
 EXPORT_SYMBOL(pci_request_regions_exclusive);
 
-#ifdef PCI_IOBASE
-struct io_range {
-	struct list_head list;
-	phys_addr_t start;
-	resource_size_t size;
-};
-
-static LIST_HEAD(io_range_list);
-static DEFINE_SPINLOCK(io_range_lock);
-#endif
-
 /*
  * Record the PCI IO range (expressed as CPU physical address + size).
  * Return a negative value if an error has occured, zero otherwise
@@ -3454,51 +3444,28 @@ struct io_range {
 int pci_register_io_range(struct fwnode_handle *fwnode, phys_addr_t addr,
 			resource_size_t	size)
 {
-	int err = 0;
-
+	int ret = 0;
 #ifdef PCI_IOBASE
-	struct io_range *range;
-	resource_size_t allocated_size = 0;
-
-	/* check if the range hasn't been previously recorded */
-	spin_lock(&io_range_lock);
-	list_for_each_entry(range, &io_range_list, list) {
-		if (addr >= range->start && addr + size <= range->start + size) {
-			/* range already registered, bail out */
-			goto end_register;
-		}
-		allocated_size += range->size;
-	}
+	struct logic_pio_hwaddr *range;
 
-	/* range not registed yet, check for available space */
-	if (allocated_size + size - 1 > IO_SPACE_LIMIT) {
-		/* if it's too big check if 64K space can be reserved */
-		if (allocated_size + SZ_64K - 1 > IO_SPACE_LIMIT) {
-			err = -E2BIG;
-			goto end_register;
-		}
-
-		size = SZ_64K;
-		pr_warn("Requested IO range too big, new size set to 64K\n");
-	}
+	if (!size || addr + size < addr)
+		return -EINVAL;
 
-	/* add the range to the list */
 	range = kzalloc(sizeof(*range), GFP_ATOMIC);
-	if (!range) {
-		err = -ENOMEM;
-		goto end_register;
-	}
+	if (!range)
+		return -ENOMEM;
 
-	range->start = addr;
+	range->fwnode = fwnode;
 	range->size = size;
+	range->hw_start = addr;
+	range->flags = LOGIC_PIO_CPU_MMIO;
 
-	list_add_tail(&range->list, &io_range_list);
-
-end_register:
-	spin_unlock(&io_range_lock);
+	ret = logic_pio_register_range(range);
+	if (ret)
+		kfree(range);
 #endif
 
-	return err;
+	return ret;
 }
 
 phys_addr_t pci_pio_to_address(unsigned long pio)
@@ -3506,21 +3473,10 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
 	phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
 
 #ifdef PCI_IOBASE
-	struct io_range *range;
-	resource_size_t allocated_size = 0;
-
-	if (pio > IO_SPACE_LIMIT)
+	if (pio >= MMIO_UPPER_LIMIT)
 		return address;
 
-	spin_lock(&io_range_lock);
-	list_for_each_entry(range, &io_range_list, list) {
-		if (pio >= allocated_size && pio < allocated_size + range->size) {
-			address = range->start + pio - allocated_size;
-			break;
-		}
-		allocated_size += range->size;
-	}
-	spin_unlock(&io_range_lock);
+	address = logic_pio_to_hwaddr(pio);
 #endif
 
 	return address;
@@ -3529,21 +3485,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
 unsigned long __weak pci_address_to_pio(phys_addr_t address)
 {
 #ifdef PCI_IOBASE
-	struct io_range *res;
-	resource_size_t offset = 0;
-	unsigned long addr = -1;
-
-	spin_lock(&io_range_lock);
-	list_for_each_entry(res, &io_range_list, list) {
-		if (address >= res->start && address < res->start + res->size) {
-			addr = address - res->start + offset;
-			break;
-		}
-		offset += res->size;
-	}
-	spin_unlock(&io_range_lock);
-
-	return addr;
+	return logic_pio_trans_cpuaddr(address);
 #else
 	if (address > IO_SPACE_LIMIT)
 		return (unsigned long)-1;
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index f76fbd6..0016413 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -901,7 +901,7 @@ static inline void __iomem *ioremap_wt(phys_addr_t offset, size_t size)
 #define ioport_map ioport_map
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
-	return PCI_IOBASE + (port & IO_SPACE_LIMIT);
+	return PCI_IOBASE + (port & MMIO_UPPER_LIMIT);
 }
 #endif
 
-- 
1.9.1

[PATCH v17 05/10] OF: Add missing I/O range exception for indirect-IO devices

[John Garry]

From: Zhichang Yuan <yuanzhichang@hisilicon.com>

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 special handling for this case.
During the OF address translation, some checking will be performed to
identify whether the device node is registered as indirect-IO. If yes,
the I/O translation will be done in a different way from that one of PCI
MMIO. In this way, the I/O 'reg' property of the special ISA/LPC devices
will be parsed correctly.

Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>    #earlier draft
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Tested-by: dann frazier <dann.frazier@canonical.com>
---
 drivers/of/address.c | 92 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 16 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index cdf047b..c434f659 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -5,6 +5,7 @@
 #include <linux/fwnode.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
+#include <linux/logic_pio.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/pci.h>
@@ -562,9 +563,14 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
  * that translation is impossible (that is we are not dealing with a value
  * that can be mapped to a cpu physical address). This is not really specified
  * that way, but this is traditionally the way IBM at least do things
+ *
+ * Whenever the translation fails, the *host pointer will be set to the
+ * device that had registered logical PIO mapping, and the return code is
+ * relative to that node.
  */
 static u64 __of_translate_address(struct device_node *dev,
-				  const __be32 *in_addr, const char *rprop)
+				  const __be32 *in_addr, const char *rprop,
+				  struct device_node **host)
 {
 	struct device_node *parent = NULL;
 	struct of_bus *bus, *pbus;
@@ -577,6 +583,7 @@ static u64 __of_translate_address(struct device_node *dev,
 	/* Increase refcount at current level */
 	of_node_get(dev);
 
+	*host = NULL;
 	/* Get parent & match bus type */
 	parent = of_get_parent(dev);
 	if (parent == NULL)
@@ -597,6 +604,8 @@ static u64 __of_translate_address(struct device_node *dev,
 
 	/* Translate */
 	for (;;) {
+		struct logic_pio_hwaddr *iorange;
+
 		/* Switch to parent bus */
 		of_node_put(dev);
 		dev = parent;
@@ -609,6 +618,19 @@ static u64 __of_translate_address(struct device_node *dev,
 			break;
 		}
 
+		/*
+		 * For indirectIO device which has no ranges property, get
+		 * the address from reg directly.
+		 */
+		iorange = find_io_range_by_fwnode(&dev->fwnode);
+		if (iorange && (iorange->flags != LOGIC_PIO_CPU_MMIO)) {
+			result = of_read_number(addr + 1, na - 1);
+			pr_debug("indirectIO matched(%pOF) 0x%llx\n",
+				 dev, result);
+			*host = of_node_get(dev);
+			break;
+		}
+
 		/* Get new parent bus and counts */
 		pbus = of_match_bus(parent);
 		pbus->count_cells(dev, &pna, &pns);
@@ -640,13 +662,32 @@ static u64 __of_translate_address(struct device_node *dev,
 
 u64 of_translate_address(struct device_node *dev, const __be32 *in_addr)
 {
-	return __of_translate_address(dev, in_addr, "ranges");
+	struct device_node *host;
+	u64 ret;
+
+	ret = __of_translate_address(dev, in_addr, "ranges", &host);
+	if (host) {
+		of_node_put(host);
+		return OF_BAD_ADDR;
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL(of_translate_address);
 
 u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr)
 {
-	return __of_translate_address(dev, in_addr, "dma-ranges");
+	struct device_node *host;
+	u64 ret;
+
+	ret = __of_translate_address(dev, in_addr, "dma-ranges", &host);
+
+	if (host) {
+		of_node_put(host);
+		return OF_BAD_ADDR;
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL(of_translate_dma_address);
 
@@ -688,29 +729,48 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
 }
 EXPORT_SYMBOL(of_get_address);
 
+static u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr,
+			u64 size)
+{
+	u64 taddr;
+	unsigned long port;
+	struct device_node *host;
+
+	taddr = __of_translate_address(dev, in_addr, "ranges", &host);
+	if (host) {
+		/* host specific port access */
+		port = logic_pio_trans_hwaddr(&host->fwnode, taddr, size);
+		of_node_put(host);
+	} else {
+		/* memory mapped I/O range */
+		port = pci_address_to_pio(taddr);
+	}
+
+	if (port == (unsigned long)-1)
+		return OF_BAD_ADDR;
+
+	return port;
+}
+
 static int __of_address_to_resource(struct device_node *dev,
 		const __be32 *addrp, u64 size, unsigned int flags,
 		const char *name, struct resource *r)
 {
 	u64 taddr;
 
-	if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
+	if (flags & IORESOURCE_MEM)
+		taddr = of_translate_address(dev, addrp);
+	else if (flags & IORESOURCE_IO)
+		taddr = of_translate_ioport(dev, addrp, size);
+	else
 		return -EINVAL;
-	taddr = of_translate_address(dev, addrp);
+
 	if (taddr == OF_BAD_ADDR)
 		return -EINVAL;
 	memset(r, 0, sizeof(struct resource));
-	if (flags & IORESOURCE_IO) {
-		unsigned long port;
-		port = pci_address_to_pio(taddr);
-		if (port == (unsigned long)-1)
-			return -EINVAL;
-		r->start = port;
-		r->end = port + size - 1;
-	} else {
-		r->start = taddr;
-		r->end = taddr + size - 1;
-	}
+
+	r->start = taddr;
+	r->end = taddr + size - 1;
 	r->flags = flags;
 	r->name = name ? name : dev->full_name;
 
-- 
1.9.1

[PATCH v17 06/10] HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings

[John Garry]

From: Zhichang Yuan <yuanzhichang@hisilicon.com>

The low-pin-count(LPC) interface of Hip06/Hip07 accesses the peripherals in
I/O port addresses. This patch implements the LPC host controller driver
which perform the I/O operations on the underlying hardware.
We don't want to touch those existing peripherals' driver, such as ipmi-bt.
So this driver applies the indirect-IO introduced in the previous patch
after registering an indirect-IO node to the indirect-IO devices list which
will be searched in the I/O accessors to retrieve the host-local I/O port.

The driver config is set as a bool instead of a trisate. The reason
here is that, by the very nature of the driver providing a logical
PIO range, it does not make sense to have this driver as a loadable
module. Another more specific reason is that the Huawei D03 board
which includes hip06 SoC requires the LPC bus for UART console, so
should be built in.

Signed-off-by: Zou Rongrong <zourongrong@huawei.com>
Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
Signed-off-by: John Garry <john.garry@huawei.com>
Acked-by: Rob Herring <robh@kernel.org> #dts part
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Tested-by: dann frazier <dann.frazier@canonical.com>
---
 .../arm/hisilicon/hisilicon-low-pin-count.txt      |  33 ++
 drivers/bus/Kconfig                                |   8 +
 drivers/bus/Makefile                               |   2 +
 drivers/bus/hisi_lpc.c                             | 421 +++++++++++++++++++++
 4 files changed, 464 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
 create mode 100644 drivers/bus/hisi_lpc.c

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..213181f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
@@ -0,0 +1,33 @@
+Hisilicon Hip06 low-pin-count device
+  Hisilicon Hip06 SoCs implement a Low Pin Count (LPC) controller, which
+  provides I/O access to some legacy ISA devices.
+  Hip06 is based on arm64 architecture where there is no I/O space. So, the
+  I/O ports here are not cpu addresses, and there is no 'ranges' property in
+  LPC device node.
+
+Required properties:
+- compatible:  value should be as follows:
+	(a) "hisilicon,hip06-lpc"
+	(b) "hisilicon,hip07-lpc"
+- #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 LPC register set 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 = "hisilicon,hip06-lpc";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	reg = <0x0 0xa01b0000 0x0 0x1000>;
+
+	ipmi0: bt@e4 {
+		compatible = "ipmi-bt";
+		device_type = "ipmi";
+		reg = <0x01 0xe4 0x04>;
+	};
+};
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 769599b..0097317 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -65,6 +65,14 @@ config BRCMSTB_GISB_ARB
 	  arbiter. This driver provides timeout and target abort error handling
 	  and internal bus master decoding.
 
+config HISILICON_LPC
+	bool "Support for ISA I/O space on Hisilicon hip06/7"
+	depends on ARM64 && (ARCH_HISI || COMPILE_TEST)
+	select INDIRECT_PIO
+	help
+	  Driver needed for some legacy ISA devices attached to Low-Pin-Count
+	  on Hisilicon hip06/7 SoC.
+
 config IMX_WEIM
 	bool "Freescale EIM DRIVER"
 	depends on ARCH_MXC
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index b666c49..b8d9ffb 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -12,6 +12,8 @@ obj-$(CONFIG_BRCMSTB_GISB_ARB)	+= brcmstb_gisb.o
 # DPAA2 fsl-mc bus
 obj-$(CONFIG_FSL_MC_BUS)	+= fsl-mc/
 
+obj-$(CONFIG_HISILICON_LPC)	+= hisi_lpc.o
+
 obj-$(CONFIG_IMX_WEIM)		+= imx-weim.o
 obj-$(CONFIG_MIPS_CDMM)		+= mips_cdmm.o
 obj-$(CONFIG_MVEBU_MBUS) 	+= mvebu-mbus.o
diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
new file mode 100644
index 0000000..c331609
--- /dev/null
+++ b/drivers/bus/hisi_lpc.c
@@ -0,0 +1,421 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ * Author: Zou Rongrong <zourongrong@huawei.com>
+ * Author: John Garry <john.garry@huawei.com>
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/logic_pio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+
+#define DRV_NAME "hisi-lpc"
+
+/*
+ * Setting this bit means each IO operation will target to a
+ * different port address:
+ * 0 means repeatedly IO operations will stick on the same port,
+ * such as BT;
+ */
+#define FG_INCRADDR_LPC		0x02
+
+struct lpc_cycle_para {
+	unsigned int opflags;
+	unsigned int csize; /* the data length of each operation */
+};
+
+struct hisi_lpc_dev {
+	spinlock_t cycle_lock;
+	void __iomem  *membase;
+	struct logic_pio_hwaddr *io_host;
+};
+
+/* The maxIO cycle counts supported is four per operation at maximum */
+#define LPC_MAX_DWIDTH	4
+
+#define LPC_REG_STARTUP_SIGNAL		0x00
+#define LPC_REG_STARTUP_SIGNAL_START	BIT(0)
+#define LPC_REG_OP_STATUS		0x04
+#define LPC_REG_OP_STATUS_IDLE		BIT(0)
+#define LPC_REG_OP_STATUS_FINISHED	BIT(1)
+#define LPC_REG_OP_LEN			0x10 /* LPC cycles count per start */
+#define LPC_REG_CMD			0x14
+#define LPC_REG_CMD_OP			BIT(0) /* 0: read, 1: write */
+#define LPC_REG_CMD_SAMEADDR		BIT(3)
+#define LPC_REG_ADDR			0x20 /* target address */
+#define LPC_REG_WDATA			0x24 /* write FIFO */
+#define LPC_REG_RDATA			0x28 /* read FIFO */
+
+/* The minimal nanosecond interval for each query on LPC cycle status. */
+#define LPC_NSEC_PERWAIT	100
+
+/*
+ * The maximum waiting time is about 128us.
+ * It is specific for stream I/O, such as ins.
+ * The fastest IO cycle time is about 390ns, but the worst case will wait
+ * for extra 256 lpc clocks, so (256 + 13) * 30ns = 8 us. The maximum
+ * burst cycles is 16. So, the maximum waiting time is about 128us under
+ * worst case.
+ * choose 1300 as the maximum.
+ */
+#define LPC_MAX_WAITCNT		1300
+/* About 10us. This is specific for single IO operation, such as inb. */
+#define LPC_PEROP_WAITCNT	100
+
+static inline int wait_lpc_idle(unsigned char *mbase,
+				unsigned int waitcnt) {
+	do {
+		u32 status;
+
+		status = readl(mbase + LPC_REG_OP_STATUS);
+		if (status & LPC_REG_OP_STATUS_IDLE)
+			return (status & LPC_REG_OP_STATUS_FINISHED) ? 0 : -EIO;
+		ndelay(LPC_NSEC_PERWAIT);
+	} while (--waitcnt);
+
+	return -ETIME;
+}
+
+/*
+ * hisi_lpc_target_in - trigger a series of LPC cycles for read operation
+ * @lpcdev: pointer to hisi lpc device
+ * @para: some parameters used to control the lpc I/O operations
+ * @addr: the lpc I/O target port address
+ * @buf: where the read back data is stored
+ * @opcnt: how many I/O operations required, i.e. data width
+ *
+ * Returns 0 on success, non-zero on fail.
+ */
+static int
+hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev, struct lpc_cycle_para *para,
+		  unsigned long addr, unsigned char *buf,
+		  unsigned long opcnt)
+{
+	unsigned int cmd_word;
+	unsigned int waitcnt;
+	unsigned long flags;
+	int ret;
+
+	if (!buf || !opcnt || !para || !para->csize || !lpcdev)
+		return -EINVAL;
+
+	cmd_word = 0; /* IO mode, Read */
+	waitcnt = LPC_PEROP_WAITCNT;
+	if (!(para->opflags & FG_INCRADDR_LPC)) {
+		cmd_word |= LPC_REG_CMD_SAMEADDR;
+		waitcnt = LPC_MAX_WAITCNT;
+	}
+
+	/* whole operation must be atomic */
+	spin_lock_irqsave(&lpcdev->cycle_lock, flags);
+
+	writel_relaxed(opcnt, lpcdev->membase + LPC_REG_OP_LEN);
+
+	writel_relaxed(cmd_word, lpcdev->membase + LPC_REG_CMD);
+
+	writel_relaxed(addr, lpcdev->membase + LPC_REG_ADDR);
+
+	writel(LPC_REG_STARTUP_SIGNAL_START,
+	       lpcdev->membase + LPC_REG_STARTUP_SIGNAL);
+
+	/* whether the operation is finished */
+	ret = wait_lpc_idle(lpcdev->membase, waitcnt);
+	if (ret) {
+		spin_unlock_irqrestore(&lpcdev->cycle_lock, flags);
+		return ret;
+	}
+
+	readsb(lpcdev->membase + LPC_REG_RDATA, buf, opcnt);
+
+	spin_unlock_irqrestore(&lpcdev->cycle_lock, flags);
+
+	return 0;
+}
+
+/*
+ * hisi_lpc_target_out - trigger a series of LPC cycles for write operation
+ * @lpcdev: pointer to hisi lpc device
+ * @para: some parameters used to control the lpc I/O operations
+ * @addr: the lpc I/O target port address
+ * @buf: where the data to be written is stored
+ * @opcnt: how many I/O operations required, i.e. data width
+ *
+ * Returns 0 on success, non-zero on fail.
+ */
+static int
+hisi_lpc_target_out(struct hisi_lpc_dev *lpcdev, struct lpc_cycle_para *para,
+		    unsigned long addr, const unsigned char *buf,
+		    unsigned long opcnt)
+{
+	unsigned int waitcnt;
+	unsigned long flags;
+	u32 cmd_word;
+	int ret;
+
+	if (!buf || !opcnt || !para || !lpcdev)
+		return -EINVAL;
+
+	/* default is increasing address */
+	cmd_word = LPC_REG_CMD_OP; /* IO mode, write */
+	waitcnt = LPC_PEROP_WAITCNT;
+	if (!(para->opflags & FG_INCRADDR_LPC)) {
+		cmd_word |= LPC_REG_CMD_SAMEADDR;
+		waitcnt = LPC_MAX_WAITCNT;
+	}
+
+	spin_lock_irqsave(&lpcdev->cycle_lock, flags);
+
+	writel_relaxed(opcnt, lpcdev->membase + LPC_REG_OP_LEN);
+	writel_relaxed(cmd_word, lpcdev->membase + LPC_REG_CMD);
+	writel_relaxed(addr, lpcdev->membase + LPC_REG_ADDR);
+
+	writesb(lpcdev->membase + LPC_REG_WDATA, buf, opcnt);
+
+	writel(LPC_REG_STARTUP_SIGNAL_START,
+	       lpcdev->membase + LPC_REG_STARTUP_SIGNAL);
+
+	/* whether the operation is finished */
+	ret = wait_lpc_idle(lpcdev->membase, waitcnt);
+
+	spin_unlock_irqrestore(&lpcdev->cycle_lock, flags);
+
+	return ret;
+}
+
+static inline unsigned long
+hisi_lpc_pio_to_addr(struct hisi_lpc_dev *lpcdev, unsigned long pio)
+{
+	return pio - lpcdev->io_host->io_start + lpcdev->io_host->hw_start;
+}
+
+/*
+ * hisi_lpc_comm_in - input the data in a single operation
+ * @hostdata: pointer to the device information relevant to LPC controller.
+ * @pio: the target I/O port address.
+ * @dwidth: the data length required to read from the target I/O port.
+ *
+ * When success, data is returned. Otherwise, ~0 is returned.
+ */
+static u32 hisi_lpc_comm_in(void *hostdata, unsigned long pio, size_t dwidth)
+{
+	struct hisi_lpc_dev *lpcdev = hostdata;
+	struct lpc_cycle_para iopara;
+	unsigned long addr;
+	u32 rd_data = 0;
+	int ret;
+
+	if (!lpcdev || !dwidth || dwidth > LPC_MAX_DWIDTH)
+		return ~0;
+
+	addr = hisi_lpc_pio_to_addr(lpcdev, pio);
+
+	iopara.opflags = FG_INCRADDR_LPC;
+	iopara.csize = dwidth;
+
+	ret = hisi_lpc_target_in(lpcdev, &iopara, addr,
+				 (unsigned char *)&rd_data, dwidth);
+	if (ret)
+		return ~0;
+
+	return le32_to_cpu(rd_data);
+}
+
+/*
+ * hisi_lpc_comm_out - output the data in a single operation
+ * @hostdata: pointer to the device information relevant to LPC controller.
+ * @pio: the target I/O port address.
+ * @val: a value to be outputted from caller, maximum is four bytes.
+ * @dwidth: the data width required writing to the target I/O port.
+ *
+ * This function is corresponding to out(b,w,l) only
+ *
+ */
+static void hisi_lpc_comm_out(void *hostdata, unsigned long pio,
+			     u32 val, size_t dwidth)
+{
+	struct hisi_lpc_dev *lpcdev = hostdata;
+	struct lpc_cycle_para iopara;
+	const unsigned char *buf;
+	unsigned long addr;
+
+	if (!lpcdev || !dwidth || dwidth > LPC_MAX_DWIDTH)
+		return;
+
+	val = cpu_to_le32(val);
+
+	buf = (const unsigned char *)&val;
+	addr = hisi_lpc_pio_to_addr(lpcdev, pio);
+
+	iopara.opflags = FG_INCRADDR_LPC;
+	iopara.csize = dwidth;
+
+	hisi_lpc_target_out(lpcdev, &iopara, addr, buf, dwidth);
+}
+
+/*
+ * hisi_lpc_comm_ins - input the data in the buffer in multiple operations
+ * @hostdata: pointer to the device information relevant to LPC controller.
+ * @pio: the target I/O port address.
+ * @buffer: a buffer where read/input data bytes are stored.
+ * @dwidth: the data width required writing to the target I/O port.
+ * @count: how many data units whose length is dwidth will be read.
+ *
+ * When success, the data read back is stored in buffer pointed by buffer.
+ * Returns 0 on success, -errno otherwise
+ *
+ */
+static u32
+hisi_lpc_comm_ins(void *hostdata, unsigned long pio, void *buffer,
+		  size_t dwidth, unsigned int count)
+{
+	struct hisi_lpc_dev *lpcdev = hostdata;
+	unsigned char *buf = buffer;
+	struct lpc_cycle_para iopara;
+	unsigned long addr;
+
+	if (!lpcdev || !buf || !count || !dwidth || dwidth > LPC_MAX_DWIDTH)
+		return -EINVAL;
+
+	iopara.opflags = 0;
+	if (dwidth > 1)
+		iopara.opflags |= FG_INCRADDR_LPC;
+	iopara.csize = dwidth;
+
+	addr = hisi_lpc_pio_to_addr(lpcdev, pio);
+
+	do {
+		int ret;
+
+		ret = hisi_lpc_target_in(lpcdev, &iopara, addr, buf, dwidth);
+		if (ret)
+			return ret;
+		buf += dwidth;
+	} while (--count);
+
+	return 0;
+}
+
+/*
+ * hisi_lpc_comm_outs - output the data in the buffer in multiple operations
+ * @hostdata: pointer to the device information relevant to LPC controller.
+ * @pio: the target I/O port address.
+ * @buffer: a buffer where write/output data bytes are stored.
+ * @dwidth: the data width required writing to the target I/O port .
+ * @count: how many data units whose length is dwidth will be written.
+ *
+ */
+static void
+hisi_lpc_comm_outs(void *hostdata, unsigned long pio, const void *buffer,
+		   size_t dwidth, unsigned int count)
+{
+	struct hisi_lpc_dev *lpcdev = hostdata;
+	struct lpc_cycle_para iopara;
+	const unsigned char *buf = buffer;
+	unsigned long addr;
+
+	if (!lpcdev || !buf || !count || !dwidth || dwidth > LPC_MAX_DWIDTH)
+		return;
+
+	iopara.opflags = 0;
+	if (dwidth > 1)
+		iopara.opflags |= FG_INCRADDR_LPC;
+	iopara.csize = dwidth;
+
+	addr = hisi_lpc_pio_to_addr(lpcdev, pio);
+	do {
+		if (hisi_lpc_target_out(lpcdev, &iopara, addr, buf, dwidth))
+			break;
+		buf += dwidth;
+	} while (--count);
+}
+
+static const struct logic_pio_host_ops hisi_lpc_ops = {
+	.in = hisi_lpc_comm_in,
+	.out = hisi_lpc_comm_out,
+	.ins = hisi_lpc_comm_ins,
+	.outs = hisi_lpc_comm_outs,
+};
+
+/*
+ * hisi_lpc_probe - the probe callback function for hisi lpc host,
+ *		   will finish all the initialization.
+ * @pdev: the platform device corresponding to hisi lpc host
+ *
+ * Returns 0 on success, non-zero on fail.
+ */
+static int hisi_lpc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct acpi_device *acpi_device = ACPI_COMPANION(dev);
+	struct logic_pio_hwaddr *range;
+	struct hisi_lpc_dev *lpcdev;
+	resource_size_t io_end;
+	struct resource *res;
+	int ret;
+
+	lpcdev = devm_kzalloc(dev, sizeof(*lpcdev), GFP_KERNEL);
+	if (!lpcdev)
+		return -ENOMEM;
+
+	spin_lock_init(&lpcdev->cycle_lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	lpcdev->membase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(lpcdev->membase))
+		return PTR_ERR(lpcdev->membase);
+
+	range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
+	if (!range)
+		return -ENOMEM;
+	range->fwnode = dev->fwnode;
+	range->flags = LOGIC_PIO_INDIRECT;
+	range->size = PIO_INDIRECT_SIZE;
+
+	ret = logic_pio_register_range(range);
+	if (ret) {
+		dev_err(dev, "register IO range failed (%d)!\n", ret);
+		return ret;
+	}
+	lpcdev->io_host = range;
+
+	/* register the LPC host PIO resources */
+	if (!acpi_device)
+		ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	if (ret)
+		return ret;
+
+	lpcdev->io_host->hostdata = lpcdev;
+	lpcdev->io_host->ops = &hisi_lpc_ops;
+
+	io_end = lpcdev->io_host->io_start + lpcdev->io_host->size;
+	dev_info(dev, "registered range [%pa - %pa]\n",
+		 &lpcdev->io_host->io_start, &io_end);
+
+	return ret;
+}
+
+static const struct of_device_id hisi_lpc_of_match[] = {
+	{ .compatible = "hisilicon,hip06-lpc", },
+	{ .compatible = "hisilicon,hip07-lpc", },
+	{}
+};
+
+static struct platform_driver hisi_lpc_driver = {
+	.driver = {
+		.name           = DRV_NAME,
+		.of_match_table = hisi_lpc_of_match,
+	},
+	.probe = hisi_lpc_probe,
+};
+
+builtin_platform_driver(hisi_lpc_driver);
-- 
1.9.1

[PATCH v17 07/10] ACPI / scan: rename acpi_is_serial_bus_slave() to widen use

[John Garry]

Currently the ACPI scan has special handling for serial bus
slaves, in that it makes it the responsibility of the the slave
device's parent to enumerate the device.

To support in future other types of slave devices which require
the same special handling, but where the bus is not strictly a
serial bus - such as devices on the HiSilicon LPC controller bus -
rename acpi_is_serial_bus_slave() to
acpi_device_enumeration_by_parent(), so that the name can fit the
wider purpose.

Associated device flag acpi_device_flags.serial_bus_slave is also
renamed to .enumeration_by_parent.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/acpi/scan.c     | 19 ++++++++++---------
 include/acpi/acpi_bus.h |  2 +-
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 8e63d93..f9e7904 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1524,7 +1524,7 @@ static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
 	return -1;
 }
 
-static bool acpi_is_serial_bus_slave(struct acpi_device *device)
+static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 {
 	struct list_head resource_list;
 	bool is_serial_bus_slave = false;
@@ -1560,7 +1560,8 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	acpi_bus_get_flags(device);
 	device->flags.match_driver = false;
 	device->flags.initialized = true;
-	device->flags.serial_bus_slave = acpi_is_serial_bus_slave(device);
+	device->flags.enumeration_by_parent =
+		acpi_device_enumeration_by_parent(device);
 	acpi_device_clear_enumerated(device);
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);
@@ -1858,10 +1859,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
 static void acpi_default_enumeration(struct acpi_device *device)
 {
 	/*
-	 * Do not enumerate SPI/I2C/UART slaves as they will be enumerated by
-	 * their respective parents.
+	 * Do not enumerate devices with enumeration_by_parent flag set as
+	 * they will be enumerated by their respective parents.
 	 */
-	if (!device->flags.serial_bus_slave) {
+	if (!device->flags.enumeration_by_parent) {
 		acpi_create_platform_device(device, NULL);
 		acpi_device_set_enumerated(device);
 	} else {
@@ -1958,7 +1959,7 @@ static void acpi_bus_attach(struct acpi_device *device)
 		return;
 
 	device->flags.match_driver = true;
-	if (ret > 0 && !device->flags.serial_bus_slave) {
+	if (ret > 0 && !device->flags.enumeration_by_parent) {
 		acpi_device_set_enumerated(device);
 		goto ok;
 	}
@@ -1967,10 +1968,10 @@ static void acpi_bus_attach(struct acpi_device *device)
 	if (ret < 0)
 		return;
 
-	if (!device->pnp.type.platform_id && !device->flags.serial_bus_slave)
-		acpi_device_set_enumerated(device);
-	else
+	if (device->pnp.type.platform_id || device->flags.enumeration_by_parent)
 		acpi_default_enumeration(device);
+	else
+		acpi_device_set_enumerated(device);
 
  ok:
 	list_for_each_entry(child, &device->children, node)
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index c9608b0..ba4dd54 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -215,7 +215,7 @@ struct acpi_device_flags {
 	u32 of_compatible_ok:1;
 	u32 coherent_dma:1;
 	u32 cca_seen:1;
-	u32 serial_bus_slave:1;
+	u32 enumeration_by_parent:1;
 	u32 reserved:19;
 };
 
-- 
1.9.1

[PATCH v17 08/10] ACPI / scan: do not enumerate Indirect IO host children

[John Garry]

Through the logical PIO framework systems which otherwise have
no IO space access to legacy ISA/LPC devices may access these
devices through so-called "indirect IO" method. In this, IO
space accesses for non-PCI hosts are redirected to a host
LLDD to manually generate the IO space (bus) accesses. Hosts
are able to register a region in logical PIO space to map to
its bus address range.

Indirect IO child devices have an associated host-specific bus
address. Special translation is required to map between
a logical PIO address for a device and it's host bus address.

Since in the ACPI tables the child device IO resources would
be the host-specific values, it is required the ACPI scan code
should not enumerate these devices, and that this should be
the responsibility of the host driver so that it can "fixup"
the resources so that they map to the appropriate logical PIO
addresses.

To avoid enumerating these child devices, we add a check from
acpi_device_enumeration_by_parent() as to whether the parent
for a device is a member of a known list of "indirect IO" hosts.
For now, the HiSilicon LPC host controller ID is added.

Signed-off-by: John Garry <john.garry@huawei.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Tested-by: dann frazier <dann.frazier@canonical.com>
---
 drivers/acpi/scan.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f9e7904..a4cbf3e 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1524,11 +1524,25 @@ static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
 	return -1;
 }
 
+static bool acpi_is_indirect_io_slave(struct acpi_device *device)
+{
+	struct acpi_device *parent = device->parent;
+	const struct acpi_device_id indirect_io_hosts[] = {
+		{"HISI0191", 0},
+		{}
+	};
+
+	return parent && !acpi_match_device_ids(parent, indirect_io_hosts);
+}
+
 static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 {
 	struct list_head resource_list;
 	bool is_serial_bus_slave = false;
 
+	if (acpi_is_indirect_io_slave(device))
+		return true;
+
 	/* Macs use device properties in lieu of _CRS resources */
 	if (x86_apple_machine &&
 	    (fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
-- 
1.9.1

[PATCH v17 09/10] HISI LPC: Add ACPI support

[John Garry]

Based on the previous patches, this patch supports the
LPC host on hip06/hip07 for ACPI FW.

It is the responsibility of the LPC host driver to
enumerate the child devices, as the ACPI scan code will
not enumerate children of "indirect IO" hosts.

The ACPI table for the LPC host controller and the child
devices is in the following format:
  Device (LPC0) {
    Name (_HID, "HISI0191")  // HiSi LPC
    Name (_CRS, ResourceTemplate () {
      Memory32Fixed (ReadWrite, 0xa01b0000, 0x1000)
    })
  }

  Device (LPC0.IPMI) {
    Name (_HID, "IPI0001")
    Name (LORS, ResourceTemplate() {
      QWordIO (
        ResourceConsumer,
	MinNotFixed,     // _MIF
	MaxNotFixed,     // _MAF
	PosDecode,
	EntireRange,
	0x0,             // _GRA
	0xe4,            // _MIN
	0x3fff,          // _MAX
	0x0,             // _TRA
	0x04,            // _LEN
	, ,
	BTIO
      )
    })

Since the IO resources of the child devices need to be
translated from LPC bus addresses to logical PIO addresses,
and we shouldn't modify the resources of the devices
generated in the FW scan, a per-child MFD is created as
a substitute. The MFD IO resources will be the translated
bus addresses of the ACPI child.

Signed-off-by: John Garry <john.garry@huawei.com>
Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Tested-by: dann frazier <dann.frazier@canonical.com>
---
 drivers/bus/hisi_lpc.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 202 insertions(+)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index c331609..9d38cfa 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -12,6 +12,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/logic_pio.h>
+#include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -346,6 +347,204 @@ static void hisi_lpc_comm_out(void *hostdata, unsigned long pio,
 	.outs = hisi_lpc_comm_outs,
 };
 
+#ifdef CONFIG_ACPI
+#define MFD_CHILD_NAME_PREFIX DRV_NAME"-"
+#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + sizeof(MFD_CHILD_NAME_PREFIX) - 1)
+
+struct hisi_lpc_mfd_cell {
+	struct mfd_cell_acpi_match acpi_match;
+	char name[MFD_CHILD_NAME_LEN];
+	char pnpid[ACPI_ID_LEN];
+};
+
+static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
+				     struct acpi_device *host,
+				     struct resource *res)
+{
+	unsigned long sys_port;
+	resource_size_t len = resource_size(res);
+
+	sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len);
+	if (sys_port == ~0UL)
+		return -EFAULT;
+
+	res->start = sys_port;
+	res->end = sys_port + len;
+
+	return 0;
+}
+
+/*
+ * hisi_lpc_acpi_set_io_res - set the resources for a child's MFD
+ * @child: the device node to be updated the I/O resource
+ * @hostdev: the device node associated with host controller
+ * @res: double pointer to be set to the address of translated resources
+ * @num_res: pointer to variable to hold the number of translated resources
+ *
+ * Returns 0 when successful, and a negative value for failure.
+ *
+ * For a given host controller, each child device will have an associated
+ * host-relative address resource. This function will return the translated
+ * logical PIO addresses for each child devices resources.
+ */
+static int hisi_lpc_acpi_set_io_res(struct device *child,
+				    struct device *hostdev,
+				    const struct resource **res,
+				    int *num_res)
+{
+	struct acpi_device *adev;
+	struct acpi_device *host;
+	struct resource_entry *rentry;
+	LIST_HEAD(resource_list);
+	struct resource *resources;
+	int count;
+	int i;
+
+	if (!child || !hostdev)
+		return -EINVAL;
+
+	host = to_acpi_device(hostdev);
+	adev = to_acpi_device(child);
+
+	/* check the device state */
+	if (!adev->status.present) {
+		dev_dbg(child, "device is not present\n");
+		return -EIO;
+	}
+	/* whether the child had been enumerated? */
+	if (acpi_device_enumerated(adev)) {
+		dev_dbg(child, "has been enumerated\n");
+		return -EIO;
+	}
+
+	/*
+	 * The following code segment to retrieve the resources is common to
+	 * acpi_create_platform_device(), so consider a common helper function
+	 * in future.
+	 */
+	count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
+	if (count <= 0) {
+		dev_dbg(child, "failed to get resources\n");
+		return count ? count : -EIO;
+	}
+
+	resources = devm_kcalloc(hostdev, count, sizeof(*resources),
+				 GFP_KERNEL);
+	if (!resources) {
+		dev_warn(hostdev, "could not allocate memory for %d resources\n",
+			 count);
+		acpi_dev_free_resource_list(&resource_list);
+		return -ENOMEM;
+	}
+	count = 0;
+	list_for_each_entry(rentry, &resource_list, node)
+		resources[count++] = *rentry->res;
+
+	acpi_dev_free_resource_list(&resource_list);
+
+	/* translate the I/O resources */
+	for (i = 0; i < count; i++) {
+		int ret;
+
+		if (!(resources[i].flags & IORESOURCE_IO))
+			continue;
+		ret = hisi_lpc_acpi_xlat_io_res(adev, host, &resources[i]);
+		if (ret) {
+			dev_err(child, "translate IO range failed(%d)\n", ret);
+			return ret;
+		}
+	}
+	*res = resources;
+	*num_res = count;
+
+	return 0;
+}
+
+/*
+ * hisi_lpc_acpi_probe - probe children for ACPI FW
+ * @hostdev: LPC host device pointer
+ *
+ * Returns 0 when successful, and a negative value for failure.
+ *
+ * Scan all child devices and create a per-device MFD with
+ * logical PIO translated IO resources.
+ */
+static int hisi_lpc_acpi_probe(struct device *hostdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(hostdev);
+	struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cells;
+	struct mfd_cell *mfd_cells;
+	struct acpi_device *child;
+	int size, ret, count = 0, cell_num = 0;
+
+	list_for_each_entry(child, &adev->children, node)
+		cell_num++;
+
+	/* allocate the mfd cell and companion ACPI info, one per child */
+	size = sizeof(*mfd_cells) + sizeof(*hisi_lpc_mfd_cells);
+	mfd_cells = devm_kcalloc(hostdev, cell_num, size, GFP_KERNEL);
+	if (!mfd_cells)
+		return -ENOMEM;
+
+	hisi_lpc_mfd_cells = (struct hisi_lpc_mfd_cell *)&mfd_cells[cell_num];
+	/* Only consider the children of the host */
+	list_for_each_entry(child, &adev->children, node) {
+		struct mfd_cell *mfd_cell = &mfd_cells[count];
+		struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cell =
+					&hisi_lpc_mfd_cells[count];
+		struct mfd_cell_acpi_match *acpi_match =
+					&hisi_lpc_mfd_cell->acpi_match;
+		char *name = hisi_lpc_mfd_cell[count].name;
+		char *pnpid = hisi_lpc_mfd_cell[count].pnpid;
+		struct mfd_cell_acpi_match match = {
+			.pnpid = pnpid,
+		};
+
+		/*
+		 * For any instances of this host controller (hip06 and hip07
+		 * are the only chipsets), we would not have multiple slaves
+		 * with the same HID. And in any system we would have just one
+		 * controller active. So don't worrry about MFD name clashes.
+		 */
+		snprintf(name, MFD_CHILD_NAME_LEN, MFD_CHILD_NAME_PREFIX"%s",
+			 acpi_device_hid(child));
+		snprintf(pnpid, ACPI_ID_LEN, "%s", acpi_device_hid(child));
+
+		memcpy(acpi_match, &match, sizeof(*acpi_match));
+		mfd_cell->name = name;
+		mfd_cell->acpi_match = acpi_match;
+
+		ret = hisi_lpc_acpi_set_io_res(&child->dev, &adev->dev,
+					       &mfd_cell->resources,
+					       &mfd_cell->num_resources);
+		if (ret) {
+			dev_warn(&child->dev, "set resource fail(%d)\n", ret);
+			return ret;
+		}
+		count++;
+	}
+
+	ret = mfd_add_devices(hostdev, PLATFORM_DEVID_NONE,
+			      mfd_cells, cell_num, NULL, 0, NULL);
+	if (ret) {
+		dev_err(hostdev, "failed to add mfd cells (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct acpi_device_id hisi_lpc_acpi_match[] = {
+	{"HISI0191"},
+	{}
+};
+#else
+static int hisi_lpc_acpi_probe(struct device *dev)
+{
+	return -ENODEV;
+}
+#endif // CONFIG_ACPI
+
 /*
  * hisi_lpc_probe - the probe callback function for hisi lpc host,
  *		   will finish all the initialization.
@@ -391,6 +590,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 	/* register the LPC host PIO resources */
 	if (!acpi_device)
 		ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	else
+		ret = hisi_lpc_acpi_probe(dev);
 	if (ret)
 		return ret;
 
@@ -414,6 +615,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 	.driver = {
 		.name           = DRV_NAME,
 		.of_match_table = hisi_lpc_of_match,
+		.acpi_match_table = ACPI_PTR(hisi_lpc_acpi_match),
 	},
 	.probe = hisi_lpc_probe,
 };
-- 
1.9.1

[PATCH v17 10/10] MAINTAINERS: Add maintainer for HiSilicon LPC driver

[John Garry]

Added maintainer for drivers/bus/hisi_lpc.c

Signed-off-by: John Garry <john.garry@huawei.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c187bc8..cd4f9dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6403,6 +6403,13 @@ W:	http://www.hisilicon.com
 S:	Maintained
 F:	drivers/net/ethernet/hisilicon/hns3/
 
+HISILICON LPC BUS DRIVER
+M:	john.garry@huawei.com
+W:	http://www.hisilicon.com
+S:	Maintained
+F:	drivers/bus/hisi_lpc.c
+F:	Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
+
 HISILICON NETWORK SUBSYSTEM DRIVER
 M:	Yisen Zhuang <yisen.zhuang@huawei.com>
 M:	Salil Mehta <salil.mehta@huawei.com>
-- 
1.9.1

Re: [PATCH v17 07/10] ACPI / scan: rename acpi_is_serial_bus_slave() to widen use

[Rafael J. Wysocki]

On Wednesday, March 14, 2018 7:15:56 PM CET John Garry wrote:
> Currently the ACPI scan has special handling for serial bus
> slaves, in that it makes it the responsibility of the the slave
> device's parent to enumerate the device.
> 
> To support in future other types of slave devices which require
> the same special handling, but where the bus is not strictly a
> serial bus - such as devices on the HiSilicon LPC controller bus -
> rename acpi_is_serial_bus_slave() to
> acpi_device_enumeration_by_parent(), so that the name can fit the
> wider purpose.
> 
> Associated device flag acpi_device_flags.serial_bus_slave is also
> renamed to .enumeration_by_parent.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/scan.c     | 19 ++++++++++---------
>  include/acpi/acpi_bus.h |  2 +-
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 8e63d93..f9e7904 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1524,7 +1524,7 @@ static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
>  	return -1;
>  }
>  
> -static bool acpi_is_serial_bus_slave(struct acpi_device *device)
> +static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
>  {
>  	struct list_head resource_list;
>  	bool is_serial_bus_slave = false;
> @@ -1560,7 +1560,8 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
>  	acpi_bus_get_flags(device);
>  	device->flags.match_driver = false;
>  	device->flags.initialized = true;
> -	device->flags.serial_bus_slave = acpi_is_serial_bus_slave(device);
> +	device->flags.enumeration_by_parent =
> +		acpi_device_enumeration_by_parent(device);
>  	acpi_device_clear_enumerated(device);
>  	device_initialize(&device->dev);
>  	dev_set_uevent_suppress(&device->dev, true);
> @@ -1858,10 +1859,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
>  static void acpi_default_enumeration(struct acpi_device *device)
>  {
>  	/*
> -	 * Do not enumerate SPI/I2C/UART slaves as they will be enumerated by
> -	 * their respective parents.
> +	 * Do not enumerate devices with enumeration_by_parent flag set as
> +	 * they will be enumerated by their respective parents.
>  	 */
> -	if (!device->flags.serial_bus_slave) {
> +	if (!device->flags.enumeration_by_parent) {
>  		acpi_create_platform_device(device, NULL);
>  		acpi_device_set_enumerated(device);
>  	} else {
> @@ -1958,7 +1959,7 @@ static void acpi_bus_attach(struct acpi_device *device)
>  		return;
>  
>  	device->flags.match_driver = true;
> -	if (ret > 0 && !device->flags.serial_bus_slave) {
> +	if (ret > 0 && !device->flags.enumeration_by_parent) {
>  		acpi_device_set_enumerated(device);
>  		goto ok;
>  	}
> @@ -1967,10 +1968,10 @@ static void acpi_bus_attach(struct acpi_device *device)
>  	if (ret < 0)
>  		return;
>  
> -	if (!device->pnp.type.platform_id && !device->flags.serial_bus_slave)
> -		acpi_device_set_enumerated(device);
> -	else
> +	if (device->pnp.type.platform_id || device->flags.enumeration_by_parent)
>  		acpi_default_enumeration(device);
> +	else
> +		acpi_device_set_enumerated(device);
>  
>   ok:
>  	list_for_each_entry(child, &device->children, node)
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index c9608b0..ba4dd54 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -215,7 +215,7 @@ struct acpi_device_flags {
>  	u32 of_compatible_ok:1;
>  	u32 coherent_dma:1;
>  	u32 cca_seen:1;
> -	u32 serial_bus_slave:1;
> +	u32 enumeration_by_parent:1;
>  	u32 reserved:19;
>  };
>  
> 

Re: [PATCH v17 08/10] ACPI / scan: do not enumerate Indirect IO host children

[Rafael J. Wysocki]

On Wednesday, March 14, 2018 7:15:57 PM CET John Garry wrote:
> Through the logical PIO framework systems which otherwise have
> no IO space access to legacy ISA/LPC devices may access these
> devices through so-called "indirect IO" method. In this, IO
> space accesses for non-PCI hosts are redirected to a host
> LLDD to manually generate the IO space (bus) accesses. Hosts
> are able to register a region in logical PIO space to map to
> its bus address range.
> 
> Indirect IO child devices have an associated host-specific bus
> address. Special translation is required to map between
> a logical PIO address for a device and it's host bus address.
> 
> Since in the ACPI tables the child device IO resources would
> be the host-specific values, it is required the ACPI scan code
> should not enumerate these devices, and that this should be
> the responsibility of the host driver so that it can "fixup"
> the resources so that they map to the appropriate logical PIO
> addresses.
> 
> To avoid enumerating these child devices, we add a check from
> acpi_device_enumeration_by_parent() as to whether the parent
> for a device is a member of a known list of "indirect IO" hosts.
> For now, the HiSilicon LPC host controller ID is added.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

You have my ACK here already.

Since I've ACKed the [7/10] too, I don't think there's anything more I can do
about this series and I'm assuming that it will be routed through other trees.

Thanks!

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Tested-by: dann frazier <dann.frazier@canonical.com>
> ---
>  drivers/acpi/scan.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f9e7904..a4cbf3e 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1524,11 +1524,25 @@ static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
>  	return -1;
>  }
>  
> +static bool acpi_is_indirect_io_slave(struct acpi_device *device)
> +{
> +	struct acpi_device *parent = device->parent;
> +	const struct acpi_device_id indirect_io_hosts[] = {
> +		{"HISI0191", 0},
> +		{}
> +	};
> +
> +	return parent && !acpi_match_device_ids(parent, indirect_io_hosts);
> +}
> +
>  static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
>  {
>  	struct list_head resource_list;
>  	bool is_serial_bus_slave = false;
>  
> +	if (acpi_is_indirect_io_slave(device))
> +		return true;
> +
>  	/* Macs use device properties in lieu of _CRS resources */
>  	if (x86_apple_machine &&
>  	    (fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
> 

Re: [PATCH v17 08/10] ACPI / scan: do not enumerate Indirect IO host children

[John Garry]

On 19/03/2018 10:30, Rafael J. Wysocki wrote:
> On Wednesday, March 14, 2018 7:15:57 PM CET John Garry wrote:
>> > Through the logical PIO framework systems which otherwise have
>> > no IO space access to legacy ISA/LPC devices may access these
>> > devices through so-called "indirect IO" method. In this, IO
>> > space accesses for non-PCI hosts are redirected to a host
>> > LLDD to manually generate the IO space (bus) accesses. Hosts
>> > are able to register a region in logical PIO space to map to
>> > its bus address range.
>> >
>> > Indirect IO child devices have an associated host-specific bus
>> > address. Special translation is required to map between
>> > a logical PIO address for a device and it's host bus address.
>> >
>> > Since in the ACPI tables the child device IO resources would
>> > be the host-specific values, it is required the ACPI scan code
>> > should not enumerate these devices, and that this should be
>> > the responsibility of the host driver so that it can "fixup"
>> > the resources so that they map to the appropriate logical PIO
>> > addresses.
>> >
>> > To avoid enumerating these child devices, we add a check from
>> > acpi_device_enumeration_by_parent() as to whether the parent
>> > for a device is a member of a known list of "indirect IO" hosts.
>> > For now, the HiSilicon LPC host controller ID is added.
>> >
>> > Signed-off-by: John Garry <john.garry@huawei.com>
>> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> You have my ACK here already.
>
> Since I've ACKed the [7/10] too, I don't think there's anything more I can do
> about this series and I'm assuming that it will be routed through other trees.
>
> Thanks!
>

Hi Rafael,

Thanks for this.

Yes, I am working on getting this whole series routed through another 
tree. Actually I think 7+8 could go separately since there is no build 
dependency, but I will try to keep the series together.

All the best, And thanks again,
John

Re: [PATCH v17 08/10] ACPI / scan: do not enumerate Indirect IO host children

[Rafael J. Wysocki]

On Mon, Mar 19, 2018 at 11:48 AM, John Garry <john.garry@huawei.com> wrote:
> On 19/03/2018 10:30, Rafael J. Wysocki wrote:
>>
>> On Wednesday, March 14, 2018 7:15:57 PM CET John Garry wrote:
>>>
>>> > Through the logical PIO framework systems which otherwise have
>>> > no IO space access to legacy ISA/LPC devices may access these
>>> > devices through so-called "indirect IO" method. In this, IO
>>> > space accesses for non-PCI hosts are redirected to a host
>>> > LLDD to manually generate the IO space (bus) accesses. Hosts
>>> > are able to register a region in logical PIO space to map to
>>> > its bus address range.
>>> >
>>> > Indirect IO child devices have an associated host-specific bus
>>> > address. Special translation is required to map between
>>> > a logical PIO address for a device and it's host bus address.
>>> >
>>> > Since in the ACPI tables the child device IO resources would
>>> > be the host-specific values, it is required the ACPI scan code
>>> > should not enumerate these devices, and that this should be
>>> > the responsibility of the host driver so that it can "fixup"
>>> > the resources so that they map to the appropriate logical PIO
>>> > addresses.
>>> >
>>> > To avoid enumerating these child devices, we add a check from
>>> > acpi_device_enumeration_by_parent() as to whether the parent
>>> > for a device is a member of a known list of "indirect IO" hosts.
>>> > For now, the HiSilicon LPC host controller ID is added.
>>> >
>>> > Signed-off-by: John Garry <john.garry@huawei.com>
>>> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> You have my ACK here already.
>>
>> Since I've ACKed the [7/10] too, I don't think there's anything more I can
>> do
>> about this series and I'm assuming that it will be routed through other
>> trees.
>>
>> Thanks!
>>
>
> Hi Rafael,
>
> Thanks for this.
>
> Yes, I am working on getting this whole series routed through another tree.
> Actually I think 7+8 could go separately since there is no build dependency,
> but I will try to keep the series together.

I can take the [7-8/10] if you want me to, so please let me know.

Thanks!

Re: [PATCH v17 08/10] ACPI / scan: do not enumerate Indirect IO host children

[John Garry]

On 19/03/2018 10:57, Rafael J. Wysocki wrote:
> On Mon, Mar 19, 2018 at 11:48 AM, John Garry <john.garry@huawei.com> wrote:
>> On 19/03/2018 10:30, Rafael J. Wysocki wrote:
>>>
>>> On Wednesday, March 14, 2018 7:15:57 PM CET John Garry wrote:
>>>>
>>>>> Through the logical PIO framework systems which otherwise have
>>>>> no IO space access to legacy ISA/LPC devices may access these
>>>>> devices through so-called "indirect IO" method. In this, IO
>>>>> space accesses for non-PCI hosts are redirected to a host
>>>>> LLDD to manually generate the IO space (bus) accesses. Hosts
>>>>> are able to register a region in logical PIO space to map to
>>>>> its bus address range.
>>>>>
>>>>> Indirect IO child devices have an associated host-specific bus
>>>>> address. Special translation is required to map between
>>>>> a logical PIO address for a device and it's host bus address.
>>>>>
>>>>> Since in the ACPI tables the child device IO resources would
>>>>> be the host-specific values, it is required the ACPI scan code
>>>>> should not enumerate these devices, and that this should be
>>>>> the responsibility of the host driver so that it can "fixup"
>>>>> the resources so that they map to the appropriate logical PIO
>>>>> addresses.
>>>>>
>>>>> To avoid enumerating these child devices, we add a check from
>>>>> acpi_device_enumeration_by_parent() as to whether the parent
>>>>> for a device is a member of a known list of "indirect IO" hosts.
>>>>> For now, the HiSilicon LPC host controller ID is added.
>>>>>
>>>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> You have my ACK here already.
>>>
>>> Since I've ACKed the [7/10] too, I don't think there's anything more I can
>>> do
>>> about this series and I'm assuming that it will be routed through other
>>> trees.
>>>
>>> Thanks!
>>>
>>
>> Hi Rafael,
>>
>> Thanks for this.
>>
>> Yes, I am working on getting this whole series routed through another tree.
>> Actually I think 7+8 could go separately since there is no build dependency,
>> but I will try to keep the series together.
>
> I can take the [7-8/10] if you want me to, so please let me know.
>

OK, thanks. Will do.

John

> Thanks!
>
> .
>

Re: [PATCH v17 00/10] LPC: legacy ISA I/O support

[Bjorn Helgaas]

On Thu, Mar 15, 2018 at 02:15:49AM +0800, John Garry wrote:
> This patchset supports the IPMI-bt device attached to the Low-Pin-Count
> interface implemented on Hisilicon Hip06/Hip07 SoC.
>                         -----------
>                         | LPC host|
>                         |         |
>                         -----------
>                              |
>                 _____________V_______________LPC
>                   |                       |
>                   V                       V
>                                      ------------
>                                      |  BT(ipmi)|
>                                      ------------
> ...

> Gabriele Paoloni (2):
>   PCI: Remove unused __weak attribute in pci_register_io_range()
>   PCI: Add fwnode handler as input param of pci_register_io_range()
> 
> John Garry (4):
>   ACPI / scan: rename acpi_is_serial_bus_slave() to widen use
>   ACPI / scan: do not enumerate Indirect IO host children
>   HISI LPC: Add ACPI support
>   MAINTAINERS: Add maintainer for HiSilicon LPC driver
> 
> Zhichang Yuan (4):
>   LIB: Introduce a generic PIO mapping method
>   PCI: Apply the new generic I/O management on PCI IO hosts
>   OF: Add missing I/O range exception for indirect-IO devices
>   HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings
> 
>  .../arm/hisilicon/hisilicon-low-pin-count.txt      |  33 ++
>  MAINTAINERS                                        |   7 +
>  drivers/acpi/pci_root.c                            |   8 +-
>  drivers/acpi/scan.c                                |  33 +-
>  drivers/bus/Kconfig                                |   8 +
>  drivers/bus/Makefile                               |   2 +
>  drivers/bus/hisi_lpc.c                             | 623 +++++++++++++++++++++
>  drivers/of/address.c                               |  96 +++-
>  drivers/pci/pci.c                                  |  95 +---
>  include/acpi/acpi_bus.h                            |   2 +-
>  include/asm-generic/io.h                           |   4 +-
>  include/linux/logic_pio.h                          | 124 ++++
>  include/linux/pci.h                                |   3 +-
>  lib/Kconfig                                        |  15 +
>  lib/Makefile                                       |   2 +
>  lib/logic_pio.c                                    | 282 ++++++++++
>  16 files changed, 1229 insertions(+), 108 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
>  create mode 100644 drivers/bus/hisi_lpc.c
>  create mode 100644 include/linux/logic_pio.h
>  create mode 100644 lib/logic_pio.c

I applied this whole series to pci/lpc for v4.17.

I made the following whitespace and other trivial corrections.
Hopefully I didn't break anything.


--- changelogs.orig	2018-03-21 18:37:47.209217927 -0500
+++ changelogs	2018-03-21 18:37:35.993074570 -0500
@@ -1,29 +1,31 @@
-commit cc88cacce96a
+commit eb3a2ff7e72e
 Author: John Garry <john.garry@huawei.com>
 Date:   Thu Mar 15 02:15:59 2018 +0800
 
-    MAINTAINERS: Add maintainer for HiSilicon LPC driver
+    MAINTAINERS: Add John Garry as maintainer for HiSilicon LPC driver
     
-    Added maintainer for drivers/bus/hisi_lpc.c
+    Add John Garry as maintainer for drivers/bus/hisi_lpc.c, the HiSilicon LPC
+    driver.
     
     Signed-off-by: John Garry <john.garry@huawei.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
 
-commit e6e915fd9c90
+commit 6d22e66ef6ea
 Author: John Garry <john.garry@huawei.com>
 Date:   Thu Mar 15 02:15:58 2018 +0800
 
     HISI LPC: Add ACPI support
     
-    Based on the previous patches, this patch supports the
-    LPC host on hip06/hip07 for ACPI FW.
+    Based on the previous patches, this patch supports the LPC host on
+    Hip06/Hip07 for ACPI FW.
     
-    It is the responsibility of the LPC host driver to
-    enumerate the child devices, as the ACPI scan code will
-    not enumerate children of "indirect IO" hosts.
+    It is the responsibility of the LPC host driver to enumerate the child
+    devices, as the ACPI scan code will not enumerate children of "indirect IO"
+    hosts.
+    
+    The ACPI table for the LPC host controller and the child devices is in the
+    following format:
     
-    The ACPI table for the LPC host controller and the child
-    devices is in the following format:
       Device (LPC0) {
         Name (_HID, "HISI0191")  // HiSi LPC
         Name (_CRS, ResourceTemplate () {
@@ -50,216 +52,214 @@
           )
         })
     
-    Since the IO resources of the child devices need to be
-    translated from LPC bus addresses to logical PIO addresses,
-    and we shouldn't modify the resources of the devices
-    generated in the FW scan, a per-child MFD is created as
-    a substitute. The MFD IO resources will be the translated
-    bus addresses of the ACPI child.
+    Since the IO resources of the child devices need to be translated from LPC
+    bus addresses to logical PIO addresses, and we shouldn't modify the
+    resources of the devices generated in the FW scan, a per-child MFD is
+    created as a substitute.  The MFD IO resources will be the translated bus
+    addresses of the ACPI child.
     
+    Tested-by: dann frazier <dann.frazier@canonical.com>
     Signed-off-by: John Garry <john.garry@huawei.com>
     Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
     Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
-    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
-    Tested-by: dann frazier <dann.frazier@canonical.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
+    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
 
-commit 3e1f89c344c6
+commit ac3f7a357d2d
 Author: John Garry <john.garry@huawei.com>
 Date:   Thu Mar 15 02:15:57 2018 +0800
 
-    ACPI / scan: do not enumerate Indirect IO host children
+    ACPI / scan: Do not enumerate Indirect IO host children
     
-    Through the logical PIO framework systems which otherwise have
-    no IO space access to legacy ISA/LPC devices may access these
-    devices through so-called "indirect IO" method. In this, IO
-    space accesses for non-PCI hosts are redirected to a host
-    LLDD to manually generate the IO space (bus) accesses. Hosts
-    are able to register a region in logical PIO space to map to
-    its bus address range.
-    
-    Indirect IO child devices have an associated host-specific bus
-    address. Special translation is required to map between
-    a logical PIO address for a device and it's host bus address.
-    
-    Since in the ACPI tables the child device IO resources would
-    be the host-specific values, it is required the ACPI scan code
-    should not enumerate these devices, and that this should be
-    the responsibility of the host driver so that it can "fixup"
-    the resources so that they map to the appropriate logical PIO
-    addresses.
-    
-    To avoid enumerating these child devices, we add a check from
-    acpi_device_enumeration_by_parent() as to whether the parent
-    for a device is a member of a known list of "indirect IO" hosts.
-    For now, the HiSilicon LPC host controller ID is added.
+    Through the logical PIO framework, systems which otherwise have no IO space
+    access to legacy ISA/LPC devices may access these devices through so-called
+    "indirect IO" method.  In this, IO space accesses for non-PCI hosts are
+    redirected to a host LLDD to manually generate the IO space (bus) accesses.
+    Hosts are able to register a region in logical PIO space to map to its bus
+    address range.
+    
+    Indirect IO child devices have an associated host-specific bus address.
+    Special translation is required to map between a logical PIO address for a
+    device and its host bus address.
+    
+    Since in the ACPI tables the child device IO resources would be the
+    host-specific values, it is required the ACPI scan code should not
+    enumerate these devices, and that this should be the responsibility of the
+    host driver so that it can "fixup" the resources so that they map to the
+    appropriate logical PIO addresses.
+    
+    To avoid enumerating these child devices, add a check from
+    acpi_device_enumeration_by_parent() as to whether the parent for a device
+    is a member of a known list of "indirect IO" hosts.  For now, the HiSilicon
+    LPC host controller ID is added.
     
-    Signed-off-by: John Garry <john.garry@huawei.com>
-    Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
-    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
     Tested-by: dann frazier <dann.frazier@canonical.com>
+    Signed-off-by: John Garry <john.garry@huawei.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
+    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
+    Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
 
-commit b4eee6c070c3
+commit 87200d40b07a
 Author: John Garry <john.garry@huawei.com>
 Date:   Thu Mar 15 02:15:56 2018 +0800
 
-    ACPI / scan: rename acpi_is_serial_bus_slave() to widen use
+    ACPI / scan: Rename acpi_is_serial_bus_slave() for more general use
     
-    Currently the ACPI scan has special handling for serial bus
-    slaves, in that it makes it the responsibility of the the slave
-    device's parent to enumerate the device.
-    
-    To support in future other types of slave devices which require
-    the same special handling, but where the bus is not strictly a
-    serial bus - such as devices on the HiSilicon LPC controller bus -
-    rename acpi_is_serial_bus_slave() to
-    acpi_device_enumeration_by_parent(), so that the name can fit the
-    wider purpose.
+    Currently the ACPI scan has special handling for serial bus slaves, in that
+    it makes it the responsibility of the slave device's parent to enumerate
+    the device.
+    
+    To support other types of slave devices which require the same special
+    handling but where the bus is not strictly a serial bus, such as devices on
+    the HiSilicon LPC controller bus, rename acpi_is_serial_bus_slave() to
+    acpi_device_enumeration_by_parent(), so that the name can fit the wider
+    purpose.
     
-    Associated device flag acpi_device_flags.serial_bus_slave is also
-    renamed to .enumeration_by_parent.
+    Also rename the associated device flag acpi_device_flags.serial_bus_slave
+    to .enumeration_by_parent.
     
     Signed-off-by: John Garry <john.garry@huawei.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
+    Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
 
-commit d9de40ca46a7
+commit a9a860ecbea9
 Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
-Date:   Wed Mar 21 18:33:39 2018 -0500
+Date:   Wed Mar 21 17:23:02 2018 -0500
 
     HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings
     
-    The low-pin-count(LPC) interface of Hip06/Hip07 accesses the peripherals in
-    I/O port addresses. This patch implements the LPC host controller driver
-    which perform the I/O operations on the underlying hardware.
-    We don't want to touch those existing peripherals' driver, such as ipmi-bt.
-    So this driver applies the indirect-IO introduced in the previous patch
-    after registering an indirect-IO node to the indirect-IO devices list which
-    will be searched in the I/O accessors to retrieve the host-local I/O port.
-    
-    The driver config is set as a bool instead of a trisate. The reason
-    here is that, by the very nature of the driver providing a logical
-    PIO range, it does not make sense to have this driver as a loadable
-    module. Another more specific reason is that the Huawei D03 board
-    which includes hip06 SoC requires the LPC bus for UART console, so
-    should be built in.
+    The low-pin-count (LPC) interface of Hip06/Hip07 accesses I/O port space of
+    peripherals.
     
+    Implement the LPC host controller driver which performs the I/O operations
+    on the underlying hardware.  We don't want to touch existing drivers such
+    as ipmi-bt, so this driver applies the indirect-IO introduced in the
+    previous patch after registering an indirect-IO node to the indirect-IO
+    devices list which will be searched by the I/O accessors to retrieve the
+    host-local I/O port.
+    
+    The driver config is set as a bool instead of a tristate.  The reason here
+    is that, by the very nature of the driver providing a logical PIO range, it
+    does not make sense to have this driver as a loadable module.  Another more
+    specific reason is that the Huawei D03 board which includes Hip06 SoC
+    requires the LPC bus for UART console, so should be built in.
+    
+    Tested-by: dann frazier <dann.frazier@canonical.com>
     Signed-off-by: Zou Rongrong <zourongrong@huawei.com>
     Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
     Signed-off-by: John Garry <john.garry@huawei.com>
-    Acked-by: Rob Herring <robh@kernel.org> #dts part
     Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
-    Tested-by: dann frazier <dann.frazier@canonical.com>
+    Acked-by: Rob Herring <robh@kernel.org> #dts part
 
-commit db64f8630d14
+commit 03be72aeeb79
 Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
 Date:   Thu Mar 15 02:15:54 2018 +0800
 
-    OF: Add missing I/O range exception for indirect-IO devices
+    of: Add missing I/O range exception for indirect-IO devices
     
     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.
+    where it is not correct to specify a 'ranges' property in the 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,
+    of_translate_address() returns an error.
+    
     Here we add special handling for this case.
+    
     During the OF address translation, some checking will be performed to
-    identify whether the device node is registered as indirect-IO. If yes,
+    identify whether the device node is registered as indirect-IO.  If it is,
     the I/O translation will be done in a different way from that one of PCI
-    MMIO. In this way, the I/O 'reg' property of the special ISA/LPC devices
+    MMIO.  In this way, the I/O 'reg' property of the special ISA/LPC devices
     will be parsed correctly.
     
+    Tested-by: dann frazier <dann.frazier@canonical.com>
     Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
     Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
-    Signed-off-by: Arnd Bergmann <arnd@arndb.de>    #earlier draft
-    Acked-by: Rob Herring <robh@kernel.org>
-    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
-    Tested-by: dann frazier <dann.frazier@canonical.com>
+    Signed-off-by: Arnd Bergmann <arnd@arndb.de>    # earlier draft
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
+    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
+    Acked-by: Rob Herring <robh@kernel.org>
 
-commit 2add4c7b6bb7
+commit 54106314daf5
 Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
 Date:   Thu Mar 15 02:15:53 2018 +0800
 
     PCI: Apply the new generic I/O management on PCI IO hosts
     
-    After introducing the new generic I/O space management(Logical PIO), the
+    After introducing the new generic I/O space management (Logical PIO), the
     original PCI MMIO relevant helpers need to be updated based on the new
     interfaces defined in logical PIO.
-    This patch adapts the corresponding code to match the changes introduced
-    by logical PIO.
     
+    Adapt the corresponding code to match the changes introduced by logical
+    PIO.
+    
+    Tested-by: dann frazier <dann.frazier@canonical.com>
     Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
     Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
-    Signed-off-by: Arnd Bergmann <arnd@arndb.de>        #earlier draft
-    Acked-by: Bjorn Helgaas <bhelgaas@google.com>
-    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
-    Tested-by: dann frazier <dann.frazier@canonical.com>
+    Signed-off-by: Arnd Bergmann <arnd@arndb.de>        # earlier draft
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
+    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
 
-commit cb37d289dfc4
+commit 32f7562cfd58
 Author: Gabriele Paoloni <gabriele.paoloni@huawei.com>
 Date:   Thu Mar 15 02:15:52 2018 +0800
 
     PCI: Add fwnode handler as input param of pci_register_io_range()
     
-    In preparation for having the PCI MMIO helpers to use the new generic
-    I/O space management(logical PIO) we need to add the fwnode handler as
+    In preparation for having the PCI MMIO helpers use the new generic I/O
+    space management (logical PIO) we need to add the fwnode handler as an
     extra input parameter.
-    This patch changes the signature of pci_register_io_range() and of
-    its callers as needed.
     
-    Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
-    Acked-by: Bjorn Helgaas <bhelgaas@google.com>
-    Acked-by: Rob Herring <robh@kernel.org>
-    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
+    Changes the signature of pci_register_io_range() and its callers as
+    needed.
+    
     Tested-by: dann frazier <dann.frazier@canonical.com>
+    Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
+    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
+    Acked-by: Rob Herring <robh@kernel.org>
 
-commit 0b0694ca8843
+commit c9f9c9eed8e2
 Author: Gabriele Paoloni <gabriele.paoloni@huawei.com>
 Date:   Thu Mar 15 02:15:51 2018 +0800
 
-    PCI: Remove unused __weak attribute in pci_register_io_range()
+    PCI: Remove __weak tag from pci_register_io_range()
     
-    Currently pci_register_io_range() has only one definition;
-    therefore there is no use of the __weak attribute.
+    pci_register_io_range() has only one definition, so there is no need for
+    the __weak attribute.  Remove it.
     
-    Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
-    Acked-by: Bjorn Helgaas <bhelgaas@google.com>
-    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
     Tested-by: dann frazier <dann.frazier@canonical.com>
+    Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
+    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
 
-commit f3ac523feb71
+commit e180fa3830cf
 Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
 Date:   Thu Mar 15 02:15:50 2018 +0800
 
-    LIB: Introduce a generic PIO mapping method
+    lib: Add generic PIO mapping method
     
-    In commit 41f8bba7f555 ("of/pci: Add pci_register_io_range() and
-    pci_pio_to_address()"), a new I/O space management was supported. With
-    that driver, the I/O ranges configured for PCI/PCIe hosts on some
-    architectures can be mapped to logical PIO, converted easily between
-    CPU address and the corresponding logicial PIO. Based on this, PCI
-    I/O devices can be accessed in a memory read/write way through the
-    unified in/out accessors.
-    
-    But on some archs/platforms, there are bus hosts which access I/O
-    peripherals with host-local I/O port addresses rather than memory
-    addresses after memory-mapped.
-    
-    To support those devices, a more generic I/O mapping method is introduced
-    here. Through this patch, both the CPU addresses and the host-local port
-    can be mapped into the logical PIO space with different logical/fake PIOs.
-    After this, all the I/O accesses to either PCI MMIO devices or host-local
-    I/O peripherals can be unified into the existing I/O accessors defined in
-    asm-generic/io.h and be redirected to the right device-specific hooks
-    based on the input logical PIO.
+    41f8bba7f555 ("of/pci: Add pci_register_io_range() and
+    pci_pio_to_address()") added support for PCI I/O space mapped into CPU
+    physical memory space.  With that support, the I/O ranges configured for
+    PCI/PCIe hosts on some architectures can be mapped to logical PIO and
+    converted easily between CPU address and the corresponding logical PIO.
+    Based on this, PCI I/O port space can be accessed via in/out accessors that
+    use memory read/write.
+    
+    But on some platforms, there are bus hosts that access I/O port space with
+    host-local I/O port addresses rather than memory addresses.
+    
+    Add a more generic I/O mapping method to support those devices.  With this
+    patch, both the CPU addresses and the host-local port can be mapped into
+    the logical PIO space with different logical/fake PIOs.  After this, all
+    the I/O accesses to either PCI MMIO devices or host-local I/O peripherals
+    can be unified into the existing I/O accessors defined in asm-generic/io.h
+    and be redirected to the right device-specific hooks based on the input
+    logical PIO.
     
+    Tested-by: dann frazier <dann.frazier@canonical.com>
     Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
     Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
     Signed-off-by: John Garry <john.garry@huawei.com>
-    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
-    Tested-by: dann frazier <dann.frazier@canonical.com>
     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
+    Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
index 213181f3aff9..10bd35f9207f 100644
--- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
+++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
@@ -1,8 +1,8 @@
-Hisilicon Hip06 low-pin-count device
+Hisilicon Hip06 Low Pin Count device
   Hisilicon Hip06 SoCs implement a Low Pin Count (LPC) controller, which
   provides I/O access to some legacy ISA devices.
   Hip06 is based on arm64 architecture where there is no I/O space. So, the
-  I/O ports here are not cpu addresses, and there is no 'ranges' property in
+  I/O ports here are not CPU addresses, and there is no 'ranges' property in
   LPC device node.
 
 Required properties:
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 88739f697ab5..a3fad0f0292f 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -66,12 +66,12 @@ config BRCMSTB_GISB_ARB
 	  and internal bus master decoding.
 
 config HISILICON_LPC
-	bool "Support for ISA I/O space on Hisilicon hip06/7"
+	bool "Support for ISA I/O space on HiSilicon Hip06/7"
 	depends on ARM64 && (ARCH_HISI || COMPILE_TEST)
 	select INDIRECT_PIO
 	help
-	  Driver needed for some legacy ISA devices attached to Low-Pin-Count
-	  on Hisilicon hip06/7 SoC.
+	  Driver to enable I/O access to devices attached to the Low Pin
+	  Count bus on the HiSilicon Hip06/7 SoC.
 
 config IMX_WEIM
 	bool "Freescale EIM DRIVER"
diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 9d38cfa17da1..2d4611e4c339 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -4,7 +4,6 @@
  * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
  * Author: Zou Rongrong <zourongrong@huawei.com>
  * Author: John Garry <john.garry@huawei.com>
- *
  */
 
 #include <linux/acpi.h>
@@ -23,16 +22,15 @@
 #define DRV_NAME "hisi-lpc"
 
 /*
- * Setting this bit means each IO operation will target to a
- * different port address:
- * 0 means repeatedly IO operations will stick on the same port,
- * such as BT;
+ * Setting this bit means each IO operation will target a different port
+ * address; 0 means repeated IO operations will use the same port,
+ * such as BT.
  */
 #define FG_INCRADDR_LPC		0x02
 
 struct lpc_cycle_para {
 	unsigned int opflags;
-	unsigned int csize; /* the data length of each operation */
+	unsigned int csize; /* data length of each operation */
 };
 
 struct hisi_lpc_dev {
@@ -41,7 +39,7 @@ struct hisi_lpc_dev {
 	struct logic_pio_hwaddr *io_host;
 };
 
-/* The maxIO cycle counts supported is four per operation at maximum */
+/* The max IO cycle counts supported is four per operation at maximum */
 #define LPC_MAX_DWIDTH	4
 
 #define LPC_REG_STARTUP_SIGNAL		0x00
@@ -57,27 +55,30 @@ struct hisi_lpc_dev {
 #define LPC_REG_WDATA			0x24 /* write FIFO */
 #define LPC_REG_RDATA			0x28 /* read FIFO */
 
-/* The minimal nanosecond interval for each query on LPC cycle status. */
+/* The minimal nanosecond interval for each query on LPC cycle status */
 #define LPC_NSEC_PERWAIT	100
 
 /*
- * The maximum waiting time is about 128us.
- * It is specific for stream I/O, such as ins.
+ * The maximum waiting time is about 128us.  It is specific for stream I/O,
+ * such as ins.
+ *
  * The fastest IO cycle time is about 390ns, but the worst case will wait
- * for extra 256 lpc clocks, so (256 + 13) * 30ns = 8 us. The maximum
- * burst cycles is 16. So, the maximum waiting time is about 128us under
- * worst case.
- * choose 1300 as the maximum.
+ * for extra 256 lpc clocks, so (256 + 13) * 30ns = 8 us. The maximum burst
+ * cycles is 16. So, the maximum waiting time is about 128us under worst
+ * case.
+ *
+ * Choose 1300 as the maximum.
  */
 #define LPC_MAX_WAITCNT		1300
-/* About 10us. This is specific for single IO operation, such as inb. */
+
+/* About 10us. This is specific for single IO operations, such as inb */
 #define LPC_PEROP_WAITCNT	100
 
-static inline int wait_lpc_idle(unsigned char *mbase,
-				unsigned int waitcnt) {
-	do {
-		u32 status;
+static int wait_lpc_idle(unsigned char *mbase, unsigned int waitcnt)
+{
+	u32 status;
 
+	do {
 		status = readl(mbase + LPC_REG_OP_STATUS);
 		if (status & LPC_REG_OP_STATUS_IDLE)
 			return (status & LPC_REG_OP_STATUS_FINISHED) ? 0 : -EIO;
@@ -97,10 +98,9 @@ static inline int wait_lpc_idle(unsigned char *mbase,
  *
  * Returns 0 on success, non-zero on fail.
  */
-static int
-hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev, struct lpc_cycle_para *para,
-		  unsigned long addr, unsigned char *buf,
-		  unsigned long opcnt)
+static int hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev,
+			      struct lpc_cycle_para *para, unsigned long addr,
+			      unsigned char *buf, unsigned long opcnt)
 {
 	unsigned int cmd_word;
 	unsigned int waitcnt;
@@ -121,9 +121,7 @@ hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev, struct lpc_cycle_para *para,
 	spin_lock_irqsave(&lpcdev->cycle_lock, flags);
 
 	writel_relaxed(opcnt, lpcdev->membase + LPC_REG_OP_LEN);
-
 	writel_relaxed(cmd_word, lpcdev->membase + LPC_REG_CMD);
-
 	writel_relaxed(addr, lpcdev->membase + LPC_REG_ADDR);
 
 	writel(LPC_REG_STARTUP_SIGNAL_START,
@@ -153,10 +151,9 @@ hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev, struct lpc_cycle_para *para,
  *
  * Returns 0 on success, non-zero on fail.
  */
-static int
-hisi_lpc_target_out(struct hisi_lpc_dev *lpcdev, struct lpc_cycle_para *para,
-		    unsigned long addr, const unsigned char *buf,
-		    unsigned long opcnt)
+static int hisi_lpc_target_out(struct hisi_lpc_dev *lpcdev,
+			       struct lpc_cycle_para *para, unsigned long addr,
+			       const unsigned char *buf, unsigned long opcnt)
 {
 	unsigned int waitcnt;
 	unsigned long flags;
@@ -193,17 +190,17 @@ hisi_lpc_target_out(struct hisi_lpc_dev *lpcdev, struct lpc_cycle_para *para,
 	return ret;
 }
 
-static inline unsigned long
-hisi_lpc_pio_to_addr(struct hisi_lpc_dev *lpcdev, unsigned long pio)
+static unsigned long hisi_lpc_pio_to_addr(struct hisi_lpc_dev *lpcdev,
+					  unsigned long pio)
 {
 	return pio - lpcdev->io_host->io_start + lpcdev->io_host->hw_start;
 }
 
 /*
  * hisi_lpc_comm_in - input the data in a single operation
- * @hostdata: pointer to the device information relevant to LPC controller.
- * @pio: the target I/O port address.
- * @dwidth: the data length required to read from the target I/O port.
+ * @hostdata: pointer to the device information relevant to LPC controller
+ * @pio: the target I/O port address
+ * @dwidth: the data length required to read from the target I/O port
  *
  * When success, data is returned. Otherwise, ~0 is returned.
  */
@@ -233,16 +230,15 @@ static u32 hisi_lpc_comm_in(void *hostdata, unsigned long pio, size_t dwidth)
 
 /*
  * hisi_lpc_comm_out - output the data in a single operation
- * @hostdata: pointer to the device information relevant to LPC controller.
- * @pio: the target I/O port address.
- * @val: a value to be outputted from caller, maximum is four bytes.
- * @dwidth: the data width required writing to the target I/O port.
- *
- * This function is corresponding to out(b,w,l) only
+ * @hostdata: pointer to the device information relevant to LPC controller
+ * @pio: the target I/O port address
+ * @val: a value to be output from caller, maximum is four bytes
+ * @dwidth: the data width required writing to the target I/O port
  *
+ * This function corresponds to out(b,w,l) only.
  */
 static void hisi_lpc_comm_out(void *hostdata, unsigned long pio,
-			     u32 val, size_t dwidth)
+			      u32 val, size_t dwidth)
 {
 	struct hisi_lpc_dev *lpcdev = hostdata;
 	struct lpc_cycle_para iopara;
@@ -265,19 +261,17 @@ static void hisi_lpc_comm_out(void *hostdata, unsigned long pio,
 
 /*
  * hisi_lpc_comm_ins - input the data in the buffer in multiple operations
- * @hostdata: pointer to the device information relevant to LPC controller.
- * @pio: the target I/O port address.
- * @buffer: a buffer where read/input data bytes are stored.
- * @dwidth: the data width required writing to the target I/O port.
- * @count: how many data units whose length is dwidth will be read.
+ * @hostdata: pointer to the device information relevant to LPC controller
+ * @pio: the target I/O port address
+ * @buffer: a buffer where read/input data bytes are stored
+ * @dwidth: the data width required writing to the target I/O port
+ * @count: how many data units whose length is dwidth will be read
  *
  * When success, the data read back is stored in buffer pointed by buffer.
- * Returns 0 on success, -errno otherwise
- *
+ * Returns 0 on success, -errno otherwise.
  */
-static u32
-hisi_lpc_comm_ins(void *hostdata, unsigned long pio, void *buffer,
-		  size_t dwidth, unsigned int count)
+static u32 hisi_lpc_comm_ins(void *hostdata, unsigned long pio, void *buffer,
+			     size_t dwidth, unsigned int count)
 {
 	struct hisi_lpc_dev *lpcdev = hostdata;
 	unsigned char *buf = buffer;
@@ -308,16 +302,15 @@ hisi_lpc_comm_ins(void *hostdata, unsigned long pio, void *buffer,
 
 /*
  * hisi_lpc_comm_outs - output the data in the buffer in multiple operations
- * @hostdata: pointer to the device information relevant to LPC controller.
- * @pio: the target I/O port address.
- * @buffer: a buffer where write/output data bytes are stored.
- * @dwidth: the data width required writing to the target I/O port .
- * @count: how many data units whose length is dwidth will be written.
- *
+ * @hostdata: pointer to the device information relevant to LPC controller
+ * @pio: the target I/O port address
+ * @buffer: a buffer where write/output data bytes are stored
+ * @dwidth: the data width required writing to the target I/O port
+ * @count: how many data units whose length is dwidth will be written
  */
-static void
-hisi_lpc_comm_outs(void *hostdata, unsigned long pio, const void *buffer,
-		   size_t dwidth, unsigned int count)
+static void hisi_lpc_comm_outs(void *hostdata, unsigned long pio,
+			       const void *buffer, size_t dwidth,
+			       unsigned int count)
 {
 	struct hisi_lpc_dev *lpcdev = hostdata;
 	struct lpc_cycle_para iopara;
@@ -384,13 +377,12 @@ static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
  * Returns 0 when successful, and a negative value for failure.
  *
  * For a given host controller, each child device will have an associated
- * host-relative address resource. This function will return the translated
+ * host-relative address resource.  This function will return the translated
  * logical PIO addresses for each child devices resources.
  */
 static int hisi_lpc_acpi_set_io_res(struct device *child,
 				    struct device *hostdev,
-				    const struct resource **res,
-				    int *num_res)
+				    const struct resource **res, int *num_res)
 {
 	struct acpi_device *adev;
 	struct acpi_device *host;
@@ -406,12 +398,11 @@ static int hisi_lpc_acpi_set_io_res(struct device *child,
 	host = to_acpi_device(hostdev);
 	adev = to_acpi_device(child);
 
-	/* check the device state */
 	if (!adev->status.present) {
 		dev_dbg(child, "device is not present\n");
 		return -EIO;
 	}
-	/* whether the child had been enumerated? */
+
 	if (acpi_device_enumerated(adev)) {
 		dev_dbg(child, "has been enumerated\n");
 		return -EIO;
@@ -450,7 +441,8 @@ static int hisi_lpc_acpi_set_io_res(struct device *child,
 			continue;
 		ret = hisi_lpc_acpi_xlat_io_res(adev, host, &resources[i]);
 		if (ret) {
-			dev_err(child, "translate IO range failed(%d)\n", ret);
+			dev_err(child, "translate IO range %pR failed (%d)\n",
+				&resources[i], ret);
 			return ret;
 		}
 	}
@@ -501,7 +493,7 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 		};
 
 		/*
-		 * For any instances of this host controller (hip06 and hip07
+		 * For any instances of this host controller (Hip06 and Hip07
 		 * are the only chipsets), we would not have multiple slaves
 		 * with the same HID. And in any system we would have just one
 		 * controller active. So don't worrry about MFD name clashes.
@@ -518,7 +510,7 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 					       &mfd_cell->resources,
 					       &mfd_cell->num_resources);
 		if (ret) {
-			dev_warn(&child->dev, "set resource fail(%d)\n", ret);
+			dev_warn(&child->dev, "set resource fail (%d)\n", ret);
 			return ret;
 		}
 		count++;
@@ -576,6 +568,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 	range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
 	if (!range)
 		return -ENOMEM;
+
 	range->fwnode = dev->fwnode;
 	range->flags = LOGIC_PIO_INDIRECT;
 	range->size = PIO_INDIRECT_SIZE;
@@ -588,10 +581,10 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 	lpcdev->io_host = range;
 
 	/* register the LPC host PIO resources */
-	if (!acpi_device)
-		ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
-	else
+	if (acpi_device)
 		ret = hisi_lpc_acpi_probe(dev);
+	else
+		ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
 	if (ret)
 		return ret;
 
@@ -619,5 +612,4 @@ static struct platform_driver hisi_lpc_driver = {
 	},
 	.probe = hisi_lpc_probe,
 };
-
 builtin_platform_driver(hisi_lpc_driver);
diff --git a/drivers/of/address.c b/drivers/of/address.c
index c434f65922bc..53349912ac75 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -738,11 +738,11 @@ static u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr,
 
 	taddr = __of_translate_address(dev, in_addr, "ranges", &host);
 	if (host) {
-		/* host specific port access */
+		/* host-specific port access */
 		port = logic_pio_trans_hwaddr(&host->fwnode, taddr, size);
 		of_node_put(host);
 	} else {
-		/* memory mapped I/O range */
+		/* memory-mapped I/O range */
 		port = pci_address_to_pio(taddr);
 	}
 
diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
index 8c78ff449d81..cbd9d8495690 100644
--- a/include/linux/logic_pio.h
+++ b/include/linux/logic_pio.h
@@ -1,9 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
+ * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved.
  * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com>
  * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
- *
  */
 
 #ifndef __LINUX_LOGIC_PIO_H
@@ -13,7 +12,7 @@
 
 enum {
 	LOGIC_PIO_INDIRECT,		/* Indirect IO flag */
-	LOGIC_PIO_CPU_MMIO,		/* Memory mapped IO flag */
+	LOGIC_PIO_CPU_MMIO,		/* Memory-mapped IO flag */
 };
 
 struct logic_pio_hwaddr {
@@ -104,8 +103,8 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count);
 #endif
 
 /*
- * Below we reserve 0x4000 bytes for Indirect IO as so far this library is only
- * used by Hisilicon LPC Host. If needed in future we may reserve a wider IO
+ * We reserve 0x4000 bytes for Indirect IO as so far this library is only
+ * used by the HiSilicon LPC Host. If needed, we can reserve a wider IO
  * area by redefining the macro below.
  */
 #define PIO_INDIRECT_SIZE 0x4000
@@ -118,7 +117,7 @@ struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode);
 unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode,
 			resource_size_t hw_addr, resource_size_t size);
 int logic_pio_register_range(struct logic_pio_hwaddr *newrange);
-extern resource_size_t logic_pio_to_hwaddr(unsigned long pio);
-extern unsigned long logic_pio_trans_cpuaddr(resource_size_t hw_addr);
+resource_size_t logic_pio_to_hwaddr(unsigned long pio);
+unsigned long logic_pio_trans_cpuaddr(resource_size_t hw_addr);
 
 #endif /* __LINUX_LOGIC_PIO_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index d9dd02cfe176..5fe577673b98 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -62,9 +62,10 @@ config INDIRECT_PIO
 	  On some platforms where no separate I/O space exists, there are I/O
 	  hosts which can not be accessed in MMIO mode. Using the logical PIO
 	  mechanism, the host-local I/O resource can be mapped into system
-	  logic PIO space shared with MMIO hosts, such as PCI/PCIE, then the
-	  system can access the I/O devices with the mapped logic PIO through
+	  logic PIO space shared with MMIO hosts, such as PCI/PCIe, then the
+	  system can access the I/O devices with the mapped-logic PIO through
 	  I/O accessors.
+
 	  This way has relatively little I/O performance cost. Please make
 	  sure your devices really need this configure item enabled.
 
diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 8394c2d4e534..dc3e9695f7f5 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -1,9 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
+ * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved.
  * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com>
  * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
- *
  */
 
 #define pr_fmt(fmt)	"LOGIC PIO: " fmt
@@ -16,7 +15,7 @@
 #include <linux/sizes.h>
 #include <linux/slab.h>
 
-/* The unique hardware address list. */
+/* The unique hardware address list */
 static LIST_HEAD(io_range_list);
 static DEFINE_MUTEX(io_range_mutex);
 
@@ -25,11 +24,11 @@ static DEFINE_MUTEX(io_range_mutex);
 
 /**
  * logic_pio_register_range - register logical PIO range for a host
- * @new_range: pointer to the io range to be registered.
+ * @new_range: pointer to the IO range to be registered.
  *
- * returns 0 on success, the error code in case of failure
+ * Returns 0 on success, the error code in case of failure.
  *
- * Register a new io range node in the io range list.
+ * Register a new IO range node in the IO range list.
  */
 int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
 {
@@ -101,10 +100,9 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
  * find_io_range_by_fwnode - find logical PIO range for given FW node
  * @fwnode: FW node handle associated with logical PIO range
  *
- * Returns pointer to node on success, NULL otherwise
+ * Returns pointer to node on success, NULL otherwise.
  *
- * Traverse the io_range_list to find the registered node whose device node
- * and/or physical IO address match to.
+ * Traverse the io_range_list to find the registered node for @fwnode.
  */
 struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode)
 {
@@ -126,7 +124,7 @@ static struct logic_pio_hwaddr *find_io_range(unsigned long pio)
 		if (in_range(pio, range->io_start, range->size))
 			return range;
 	}
-	pr_err("PIO entry token invalid\n");
+	pr_err("PIO entry token %lx invalid\n", pio);
 	return NULL;
 }
 
@@ -134,21 +132,20 @@ static struct logic_pio_hwaddr *find_io_range(unsigned long pio)
  * logic_pio_to_hwaddr - translate logical PIO to HW address
  * @pio: logical PIO value
  *
- * Returns HW address if valid, ~0 otherwise
+ * Returns HW address if valid, ~0 otherwise.
  *
- * Translate the input logical pio to the corresponding hardware address.
- * The input pio should be unique in the whole logical PIO space.
+ * Translate the input logical PIO to the corresponding hardware address.
+ * The input PIO should be unique in the whole logical PIO space.
  */
 resource_size_t logic_pio_to_hwaddr(unsigned long pio)
 {
 	struct logic_pio_hwaddr *range;
-	resource_size_t hwaddr = (resource_size_t)~0;
 
 	range = find_io_range(pio);
 	if (range)
-		hwaddr = range->hw_start + pio - range->io_start;
+		return = range->hw_start + pio - range->io_start;
 
-	return hwaddr;
+	return (resource_size_t)~0;
 }
 
 /**
@@ -159,15 +156,14 @@ resource_size_t logic_pio_to_hwaddr(unsigned long pio)
  *
  * Returns Logical PIO value if successful, ~0UL otherwise
  */
-unsigned long
-logic_pio_trans_hwaddr(struct fwnode_handle *fwnode, resource_size_t addr,
-		       resource_size_t size)
+unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode,
+				     resource_size_t addr, resource_size_t size)
 {
 	struct logic_pio_hwaddr *range;
 
 	range = find_io_range_by_fwnode(fwnode);
 	if (!range || range->flags == LOGIC_PIO_CPU_MMIO) {
-		pr_err("range not found or invalid\n");
+		pr_err("IO range not found or invalid\n");
 		return ~0UL;
 	}
 	if (range->size < size) {
@@ -178,8 +174,7 @@ logic_pio_trans_hwaddr(struct fwnode_handle *fwnode, resource_size_t addr,
 	return addr - range->hw_start + range->io_start;
 }
 
-unsigned long
-logic_pio_trans_cpuaddr(resource_size_t addr)
+unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
 {
 	struct logic_pio_hwaddr *range;
 
@@ -189,7 +184,8 @@ logic_pio_trans_cpuaddr(resource_size_t addr)
 		if (in_range(addr, range->hw_start, range->size))
 			return addr - range->hw_start + range->io_start;
 	}
-	pr_err("addr not registered in io_range_list\n");
+	pr_err("addr %llx not registered in io_range_list\n",
+	       (unsigned long long) addr);
 	return ~0UL;
 }
 

Re: [PATCH v17 00/10] LPC: legacy ISA I/O support

[John Garry]

On 21/03/2018 23:39, Bjorn Helgaas wrote:
> On Thu, Mar 15, 2018 at 02:15:49AM +0800, John Garry wrote:
>> This patchset supports the IPMI-bt device attached to the Low-Pin-Count
>> interface implemented on Hisilicon Hip06/Hip07 SoC.
>>                         -----------
>>                         | LPC host|
>>                         |         |
>>                         -----------
>>                              |
>>                 _____________V_______________LPC
>>                   |                       |
>>                   V                       V
>>                                      ------------
>>                                      |  BT(ipmi)|
>>                                      ------------
>> ...
>
>> Gabriele Paoloni (2):
>>   PCI: Remove unused __weak attribute in pci_register_io_range()
>>   PCI: Add fwnode handler as input param of pci_register_io_range()
>>
>> John Garry (4):
>>   ACPI / scan: rename acpi_is_serial_bus_slave() to widen use
>>   ACPI / scan: do not enumerate Indirect IO host children
>>   HISI LPC: Add ACPI support
>>   MAINTAINERS: Add maintainer for HiSilicon LPC driver
>>
>> Zhichang Yuan (4):
>>   LIB: Introduce a generic PIO mapping method
>>   PCI: Apply the new generic I/O management on PCI IO hosts
>>   OF: Add missing I/O range exception for indirect-IO devices
>>   HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings
>>
>>  .../arm/hisilicon/hisilicon-low-pin-count.txt      |  33 ++
>>  MAINTAINERS                                        |   7 +
>>  drivers/acpi/pci_root.c                            |   8 +-
>>  drivers/acpi/scan.c                                |  33 +-
>>  drivers/bus/Kconfig                                |   8 +
>>  drivers/bus/Makefile                               |   2 +
>>  drivers/bus/hisi_lpc.c                             | 623 +++++++++++++++++++++
>>  drivers/of/address.c                               |  96 +++-
>>  drivers/pci/pci.c                                  |  95 +---
>>  include/acpi/acpi_bus.h                            |   2 +-
>>  include/asm-generic/io.h                           |   4 +-
>>  include/linux/logic_pio.h                          | 124 ++++
>>  include/linux/pci.h                                |   3 +-
>>  lib/Kconfig                                        |  15 +
>>  lib/Makefile                                       |   2 +
>>  lib/logic_pio.c                                    | 282 ++++++++++
>>  16 files changed, 1229 insertions(+), 108 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
>>  create mode 100644 drivers/bus/hisi_lpc.c
>>  create mode 100644 include/linux/logic_pio.h
>>  create mode 100644 lib/logic_pio.c
>
> I applied this whole series to pci/lpc for v4.17.
>
> I made the following whitespace and other trivial corrections.
> Hopefully I didn't break anything.
>

Hi Bjorn,

Super thanks for doing this. In general the changes look ok. However a 
build issue has appeared, below.

I retested your pci/lpc branch (with the build fix), and it seems fine.

BTW, I am also testing with a "Serial controller: MosChip Semiconductor 
Technology Ltd. PCIe 9912 Multi-I/O Controller" in loopback mode to 
ensure PCI host IO space is not broken and this is ok.

Thanks again, and to everyone who has helped with this patchset!

John

>
> --- changelogs.orig	2018-03-21 18:37:47.209217927 -0500
> +++ changelogs	2018-03-21 18:37:35.993074570 -0500
> @@ -1,29 +1,31 @@
> -commit cc88cacce96a
> +commit eb3a2ff7e72e
>  Author: John Garry <john.garry@huawei.com>
>  Date:   Thu Mar 15 02:15:59 2018 +0800
>

[ ... ]

>
> @@ -134,21 +132,20 @@ static struct logic_pio_hwaddr *find_io_range(unsigned long pio)
>   * logic_pio_to_hwaddr - translate logical PIO to HW address
>   * @pio: logical PIO value
>   *
> - * Returns HW address if valid, ~0 otherwise
> + * Returns HW address if valid, ~0 otherwise.
>   *
> - * Translate the input logical pio to the corresponding hardware address.
> - * The input pio should be unique in the whole logical PIO space.
> + * Translate the input logical PIO to the corresponding hardware address.
> + * The input PIO should be unique in the whole logical PIO space.
>   */
>  resource_size_t logic_pio_to_hwaddr(unsigned long pio)
>  {
>  	struct logic_pio_hwaddr *range;
> -	resource_size_t hwaddr = (resource_size_t)~0;
>
>  	range = find_io_range(pio);
>  	if (range)
> -		hwaddr = range->hw_start + pio - range->io_start;
> +		return = range->hw_start + pio - range->io_start;

Please remove '='

>
> -	return hwaddr;
> +	return (resource_size_t)~0;
>  }
>
>  /**
> @@ -159,15 +156,14 @@ resource_size_t logic_pio_to_hwaddr(unsigned long pio)
>   *
>   * Returns Logical PIO value if successful, ~0UL otherwise
>   */

Re: [PATCH v17 00/10] LPC: legacy ISA I/O support

[Bjorn Helgaas]

On Thu, Mar 22, 2018 at 10:38:37AM +0000, John Garry wrote:
> On 21/03/2018 23:39, Bjorn Helgaas wrote:
> > On Thu, Mar 15, 2018 at 02:15:49AM +0800, John Garry wrote:
> > > This patchset supports the IPMI-bt device attached to the Low-Pin-Count
> > > interface implemented on Hisilicon Hip06/Hip07 SoC.
> > >                         -----------
> > >                         | LPC host|
> > >                         |         |
> > >                         -----------
> > >                              |
> > >                 _____________V_______________LPC
> > >                   |                       |
> > >                   V                       V
> > >                                      ------------
> > >                                      |  BT(ipmi)|
> > >                                      ------------
> > > ...
> > 
> > > Gabriele Paoloni (2):
> > >   PCI: Remove unused __weak attribute in pci_register_io_range()
> > >   PCI: Add fwnode handler as input param of pci_register_io_range()
> > > 
> > > John Garry (4):
> > >   ACPI / scan: rename acpi_is_serial_bus_slave() to widen use
> > >   ACPI / scan: do not enumerate Indirect IO host children
> > >   HISI LPC: Add ACPI support
> > >   MAINTAINERS: Add maintainer for HiSilicon LPC driver
> > > 
> > > Zhichang Yuan (4):
> > >   LIB: Introduce a generic PIO mapping method
> > >   PCI: Apply the new generic I/O management on PCI IO hosts
> > >   OF: Add missing I/O range exception for indirect-IO devices
> > >   HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings
> > > 
> > >  .../arm/hisilicon/hisilicon-low-pin-count.txt      |  33 ++
> > >  MAINTAINERS                                        |   7 +
> > >  drivers/acpi/pci_root.c                            |   8 +-
> > >  drivers/acpi/scan.c                                |  33 +-
> > >  drivers/bus/Kconfig                                |   8 +
> > >  drivers/bus/Makefile                               |   2 +
> > >  drivers/bus/hisi_lpc.c                             | 623 +++++++++++++++++++++
> > >  drivers/of/address.c                               |  96 +++-
> > >  drivers/pci/pci.c                                  |  95 +---
> > >  include/acpi/acpi_bus.h                            |   2 +-
> > >  include/asm-generic/io.h                           |   4 +-
> > >  include/linux/logic_pio.h                          | 124 ++++
> > >  include/linux/pci.h                                |   3 +-
> > >  lib/Kconfig                                        |  15 +
> > >  lib/Makefile                                       |   2 +
> > >  lib/logic_pio.c                                    | 282 ++++++++++
> > >  16 files changed, 1229 insertions(+), 108 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> > >  create mode 100644 drivers/bus/hisi_lpc.c
> > >  create mode 100644 include/linux/logic_pio.h
> > >  create mode 100644 lib/logic_pio.c
> > 
> > I applied this whole series to pci/lpc for v4.17.
> > 
> > I made the following whitespace and other trivial corrections.
> > Hopefully I didn't break anything.
> > 
> 
> Hi Bjorn,
> 
> Super thanks for doing this. In general the changes look ok. However a build
> issue has appeared, below.

It didn't just "appear"; I added it myself!  I fixed it and repushed the
branch.

Re: [PATCH v17 00/10] LPC: legacy ISA I/O support

[John Garry]

>>>
>>> I applied this whole series to pci/lpc for v4.17.
>>>
>>> I made the following whitespace and other trivial corrections.
>>> Hopefully I didn't break anything.
>>>
>>
>> Hi Bjorn,
>>
>> Super thanks for doing this. In general the changes look ok. However a build
>> issue has appeared, below.
>
> It didn't just "appear"; I added it myself!  I fixed it and repushed the
> branch.
>

Hi Bjorn,

I repulled and tested, and it looks ok.

Thanks,
John

> .
>

Re: [PATCH v17 04/10] PCI: Apply the new generic I/O management on PCI IO hosts

[Thierry Reding]

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

On Thu, Mar 15, 2018 at 02:15:53AM +0800, John Garry wrote:
> From: Zhichang Yuan <yuanzhichang@hisilicon.com>
> 
> After introducing the new generic I/O space management(Logical PIO), the
> original PCI MMIO relevant helpers need to be updated based on the new
> interfaces defined in logical PIO.
> This patch adapts the corresponding code to match the changes introduced
> by logical PIO.
> 
> Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>        #earlier draft
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Tested-by: dann frazier <dann.frazier@canonical.com>
> ---
>  drivers/pci/pci.c        | 92 +++++++++---------------------------------------
>  include/asm-generic/io.h |  2 +-
>  2 files changed, 18 insertions(+), 76 deletions(-)

Today's linux-next regresses on NFS boot for Jetson TK1. I was able to
bisect it to this commit. I'll comment below for where I think this is
buggy.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3f30b7d..09c2490 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -22,6 +22,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
>  #include <linux/log2.h>
> +#include <linux/logic_pio.h>
>  #include <linux/pci-aspm.h>
>  #include <linux/pm_wakeup.h>
>  #include <linux/interrupt.h>
> @@ -3436,17 +3437,6 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)
>  }
>  EXPORT_SYMBOL(pci_request_regions_exclusive);
>  
> -#ifdef PCI_IOBASE
> -struct io_range {
> -	struct list_head list;
> -	phys_addr_t start;
> -	resource_size_t size;
> -};
> -
> -static LIST_HEAD(io_range_list);
> -static DEFINE_SPINLOCK(io_range_lock);
> -#endif
> -
>  /*
>   * Record the PCI IO range (expressed as CPU physical address + size).
>   * Return a negative value if an error has occured, zero otherwise
> @@ -3454,51 +3444,28 @@ struct io_range {
>  int pci_register_io_range(struct fwnode_handle *fwnode, phys_addr_t addr,
>  			resource_size_t	size)
>  {
> -	int err = 0;
> -
> +	int ret = 0;
>  #ifdef PCI_IOBASE
> -	struct io_range *range;
> -	resource_size_t allocated_size = 0;
> -
> -	/* check if the range hasn't been previously recorded */
> -	spin_lock(&io_range_lock);
> -	list_for_each_entry(range, &io_range_list, list) {
> -		if (addr >= range->start && addr + size <= range->start + size) {
> -			/* range already registered, bail out */
> -			goto end_register;
> -		}
> -		allocated_size += range->size;
> -	}
> +	struct logic_pio_hwaddr *range;
>  
> -	/* range not registed yet, check for available space */
> -	if (allocated_size + size - 1 > IO_SPACE_LIMIT) {
> -		/* if it's too big check if 64K space can be reserved */
> -		if (allocated_size + SZ_64K - 1 > IO_SPACE_LIMIT) {
> -			err = -E2BIG;
> -			goto end_register;
> -		}
> -
> -		size = SZ_64K;
> -		pr_warn("Requested IO range too big, new size set to 64K\n");
> -	}
> +	if (!size || addr + size < addr)
> +		return -EINVAL;
>  
> -	/* add the range to the list */
>  	range = kzalloc(sizeof(*range), GFP_ATOMIC);
> -	if (!range) {
> -		err = -ENOMEM;
> -		goto end_register;
> -	}
> +	if (!range)
> +		return -ENOMEM;
>  
> -	range->start = addr;
> +	range->fwnode = fwnode;
>  	range->size = size;
> +	range->hw_start = addr;
> +	range->flags = LOGIC_PIO_CPU_MMIO;
>  
> -	list_add_tail(&range->list, &io_range_list);
> -
> -end_register:
> -	spin_unlock(&io_range_lock);
> +	ret = logic_pio_register_range(range);

This ends up returning -EFAULT at some point, causing the driver's
(pci-tegra.c) ->probe() to fail.

Let me comment on a prior patch to pinpoint what exactly goes wrong.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v17 04/10] PCI: Apply the new generic I/O management on PCI IO hosts

[John Garry]

On 03/04/2018 14:45, Thierry Reding wrote:
> On Thu, Mar 15, 2018 at 02:15:53AM +0800, John Garry wrote:
>> From: Zhichang Yuan <yuanzhichang@hisilicon.com>
>>
>> After introducing the new generic I/O space management(Logical PIO), the
>> original PCI MMIO relevant helpers need to be updated based on the new
>> interfaces defined in logical PIO.
>> This patch adapts the corresponding code to match the changes introduced
>> by logical PIO.
>>
>> Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>        #earlier draft
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Tested-by: dann frazier <dann.frazier@canonical.com>
>> ---
>>  drivers/pci/pci.c        | 92 +++++++++---------------------------------------
>>  include/asm-generic/io.h |  2 +-
>>  2 files changed, 18 insertions(+), 76 deletions(-)
>
> Today's linux-next regresses on NFS boot for Jetson TK1. I was able to
> bisect it to this commit. I'll comment below for where I think this is
> buggy.
>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 3f30b7d..09c2490 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/string.h>
>>  #include <linux/log2.h>
>> +#include <linux/logic_pio.h>
>>  #include <linux/pci-aspm.h>
>>  #include <linux/pm_wakeup.h>
>>  #include <linux/interrupt.h>
>> @@ -3436,17 +3437,6 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)
>>  }
>>  EXPORT_SYMBOL(pci_request_regions_exclusive);
>>
>> -#ifdef PCI_IOBASE
>> -struct io_range {
>> -	struct list_head list;
>> -	phys_addr_t start;
>> -	resource_size_t size;
>> -};
>> -
>> -static LIST_HEAD(io_range_list);
>> -static DEFINE_SPINLOCK(io_range_lock);
>> -#endif
>> -
>>  /*
>>   * Record the PCI IO range (expressed as CPU physical address + size).
>>   * Return a negative value if an error has occured, zero otherwise
>> @@ -3454,51 +3444,28 @@ struct io_range {
>>  int pci_register_io_range(struct fwnode_handle *fwnode, phys_addr_t addr,
>>  			resource_size_t	size)
>>  {
>> -	int err = 0;
>> -
>> +	int ret = 0;
>>  #ifdef PCI_IOBASE
>> -	struct io_range *range;
>> -	resource_size_t allocated_size = 0;
>> -
>> -	/* check if the range hasn't been previously recorded */
>> -	spin_lock(&io_range_lock);
>> -	list_for_each_entry(range, &io_range_list, list) {
>> -		if (addr >= range->start && addr + size <= range->start + size) {
>> -			/* range already registered, bail out */
>> -			goto end_register;
>> -		}
>> -		allocated_size += range->size;
>> -	}
>> +	struct logic_pio_hwaddr *range;
>>
>> -	/* range not registed yet, check for available space */
>> -	if (allocated_size + size - 1 > IO_SPACE_LIMIT) {
>> -		/* if it's too big check if 64K space can be reserved */
>> -		if (allocated_size + SZ_64K - 1 > IO_SPACE_LIMIT) {
>> -			err = -E2BIG;
>> -			goto end_register;
>> -		}
>> -
>> -		size = SZ_64K;
>> -		pr_warn("Requested IO range too big, new size set to 64K\n");
>> -	}
>> +	if (!size || addr + size < addr)
>> +		return -EINVAL;
>>
>> -	/* add the range to the list */
>>  	range = kzalloc(sizeof(*range), GFP_ATOMIC);
>> -	if (!range) {
>> -		err = -ENOMEM;
>> -		goto end_register;
>> -	}
>> +	if (!range)
>> +		return -ENOMEM;
>>
>> -	range->start = addr;
>> +	range->fwnode = fwnode;
>>  	range->size = size;
>> +	range->hw_start = addr;
>> +	range->flags = LOGIC_PIO_CPU_MMIO;
>>
>> -	list_add_tail(&range->list, &io_range_list);
>> -
>> -end_register:
>> -	spin_unlock(&io_range_lock);
>> +	ret = logic_pio_register_range(range);
>
> This ends up returning -EFAULT at some point, causing the driver's
> (pci-tegra.c) ->probe() to fail.
>
> Let me comment on a prior patch to pinpoint what exactly goes wrong.

Hi Thierry,

Thanks for the notification. So this patch is where we transistion from 
using the PCI internal pio management to the new framework.

I already had tried today's linux-next with hisi PCI host controller in 
pcie-hisi.c on an arm64 system, and it looked ok.

Function logic_pio_register_range() returns fault when we try to 
register the same range twice or there is an overlap in ranges.

I wonder if is there something special about this host controller driver 
in terms of IO space management. And what arch/system was the host?

John

>
> Thierry
>

Re: [PATCH v17 01/10] LIB: Introduce a generic PIO mapping method

[Thierry Reding]

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

On Thu, Mar 15, 2018 at 02:15:50AM +0800, John Garry wrote:
> From: Zhichang Yuan <yuanzhichang@hisilicon.com>
> 
> In commit 41f8bba7f555 ("of/pci: Add pci_register_io_range() and
> pci_pio_to_address()"), a new I/O space management was supported. With
> that driver, the I/O ranges configured for PCI/PCIe hosts on some
> architectures can be mapped to logical PIO, converted easily between
> CPU address and the corresponding logicial PIO. Based on this, PCI
> I/O devices can be accessed in a memory read/write way through the
> unified in/out accessors.
> 
> But on some archs/platforms, there are bus hosts which access I/O
> peripherals with host-local I/O port addresses rather than memory
> addresses after memory-mapped.
> 
> To support those devices, a more generic I/O mapping method is introduced
> here. Through this patch, both the CPU addresses and the host-local port
> can be mapped into the logical PIO space with different logical/fake PIOs.
> After this, all the I/O accesses to either PCI MMIO devices or host-local
> I/O peripherals can be unified into the existing I/O accessors defined in
> asm-generic/io.h and be redirected to the right device-specific hooks
> based on the input logical PIO.
> 
> Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> Signed-off-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Tested-by: dann frazier <dann.frazier@canonical.com>
> ---
>  include/asm-generic/io.h  |   2 +
>  include/linux/logic_pio.h | 124 ++++++++++++++++++++
>  lib/Kconfig               |  15 +++
>  lib/Makefile              |   2 +
>  lib/logic_pio.c           | 282 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 425 insertions(+)
>  create mode 100644 include/linux/logic_pio.h
>  create mode 100644 lib/logic_pio.c
> 
[...]
> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> new file mode 100644
> index 0000000..8394c2d
> --- /dev/null
> +++ b/lib/logic_pio.c
> @@ -0,0 +1,282 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
> + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
> + *
> + */
> +
> +#define pr_fmt(fmt)	"LOGIC PIO: " fmt
> +
> +#include <linux/of.h>
> +#include <linux/io.h>
> +#include <linux/logic_pio.h>
> +#include <linux/mm.h>
> +#include <linux/rculist.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +
> +/* The unique hardware address list. */
> +static LIST_HEAD(io_range_list);
> +static DEFINE_MUTEX(io_range_mutex);
> +
> +/* Consider a kernel general helper for this */
> +#define in_range(b, first, len)        ((b) >= (first) && (b) < (first) + (len))
> +
> +/**
> + * logic_pio_register_range - register logical PIO range for a host
> + * @new_range: pointer to the io range to be registered.
> + *
> + * returns 0 on success, the error code in case of failure
> + *
> + * Register a new io range node in the io range list.
> + */
> +int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
> +{
> +	struct logic_pio_hwaddr *range;
> +	resource_size_t start = new_range->hw_start;
> +	resource_size_t end = new_range->hw_start + new_range->size;
> +	resource_size_t mmio_sz = 0;
> +	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
> +	int ret = 0;
> +
> +	if (!new_range || !new_range->fwnode || !new_range->size)
> +		return -EINVAL;
> +
> +	mutex_lock(&io_range_mutex);
> +	list_for_each_entry_rcu(range, &io_range_list, list) {
> +		if (range->fwnode == new_range->fwnode) {
> +			/* range already there */
> +			ret = -EFAULT;
> +			goto end_register;
> +		}

This is the -EFAULT that propagates to pci-tegra.c's ->probe() and fails
to bind the driver.

I'm not exactly sure what's causing the duplicate here because it's
rather difficult to get at something useful from just the ->fwnode, but
I'm fairly sure that the reason this breaks is because the Tegra driver
will defer probe due to some regulators that aren't available on the
first try. Given the above code and the rest of this file, I can't see a
way to "fix" the driver and remove the I/O range on failure.

This is doubly bad because this doesn't only leak the ranges on probe
deferral, but also on driver unload, and we just added support for
building the Tegra driver as a loadable module, so these are actually
cases that can happen in regular uses of the driver.

I have no idea on how to fix this. Anyone know of a quick fix to restore
PCI for Tegra other than reverting all of these changes?

I suppose an API could be added to unregister the range, but the calling
sequence is rather obfuscated, so removing the range will look totally
asymmetric, I'm afraid.

Here's the call stack:

	tegra_pcie_probe()
	tegra_pcie_parse_dt()
	of_pci_range_to_resource()
	pci_register_io_range()
	logic_pio_register_range()

So the range here is registered as part of a resource parsing function,
which is supposed to not have any side-effects. There's no equivalent of
that parsing routine (i.e. no "unparse" function that would undo the
effects of parsing).

Perhaps a cleaner way would be to decouple the parsing from the actual
request step that has the side-effect.

Going back in history a little, it looks like even before this commit
the I/O range registration was triggered by the parsing code and even
the range leak was there, except that it caused pci_register_io_range()
to return 0 rather than -EFAULT. Perhaps the quickest fix for this would
be to do the same in the new code and restore drivers that accidentally
depend on this behaviour.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v17 01/10] LIB: Introduce a generic PIO mapping method

[Thierry Reding]

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

On Tue, Apr 03, 2018 at 04:04:10PM +0200, Thierry Reding wrote:
> On Thu, Mar 15, 2018 at 02:15:50AM +0800, John Garry wrote:
> > From: Zhichang Yuan <yuanzhichang@hisilicon.com>
> > 
> > In commit 41f8bba7f555 ("of/pci: Add pci_register_io_range() and
> > pci_pio_to_address()"), a new I/O space management was supported. With
> > that driver, the I/O ranges configured for PCI/PCIe hosts on some
> > architectures can be mapped to logical PIO, converted easily between
> > CPU address and the corresponding logicial PIO. Based on this, PCI
> > I/O devices can be accessed in a memory read/write way through the
> > unified in/out accessors.
> > 
> > But on some archs/platforms, there are bus hosts which access I/O
> > peripherals with host-local I/O port addresses rather than memory
> > addresses after memory-mapped.
> > 
> > To support those devices, a more generic I/O mapping method is introduced
> > here. Through this patch, both the CPU addresses and the host-local port
> > can be mapped into the logical PIO space with different logical/fake PIOs.
> > After this, all the I/O accesses to either PCI MMIO devices or host-local
> > I/O peripherals can be unified into the existing I/O accessors defined in
> > asm-generic/io.h and be redirected to the right device-specific hooks
> > based on the input logical PIO.
> > 
> > Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> > Signed-off-by: John Garry <john.garry@huawei.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Tested-by: dann frazier <dann.frazier@canonical.com>
> > ---
> >  include/asm-generic/io.h  |   2 +
> >  include/linux/logic_pio.h | 124 ++++++++++++++++++++
> >  lib/Kconfig               |  15 +++
> >  lib/Makefile              |   2 +
> >  lib/logic_pio.c           | 282 ++++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 425 insertions(+)
> >  create mode 100644 include/linux/logic_pio.h
> >  create mode 100644 lib/logic_pio.c
> > 
> [...]
> > diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> > new file mode 100644
> > index 0000000..8394c2d
> > --- /dev/null
> > +++ b/lib/logic_pio.c
> > @@ -0,0 +1,282 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2017 Hisilicon Limited, All Rights Reserved.
> > + * Author: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> > + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
> > + *
> > + */
> > +
> > +#define pr_fmt(fmt)	"LOGIC PIO: " fmt
> > +
> > +#include <linux/of.h>
> > +#include <linux/io.h>
> > +#include <linux/logic_pio.h>
> > +#include <linux/mm.h>
> > +#include <linux/rculist.h>
> > +#include <linux/sizes.h>
> > +#include <linux/slab.h>
> > +
> > +/* The unique hardware address list. */
> > +static LIST_HEAD(io_range_list);
> > +static DEFINE_MUTEX(io_range_mutex);
> > +
> > +/* Consider a kernel general helper for this */
> > +#define in_range(b, first, len)        ((b) >= (first) && (b) < (first) + (len))
> > +
> > +/**
> > + * logic_pio_register_range - register logical PIO range for a host
> > + * @new_range: pointer to the io range to be registered.
> > + *
> > + * returns 0 on success, the error code in case of failure
> > + *
> > + * Register a new io range node in the io range list.
> > + */
> > +int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
> > +{
> > +	struct logic_pio_hwaddr *range;
> > +	resource_size_t start = new_range->hw_start;
> > +	resource_size_t end = new_range->hw_start + new_range->size;
> > +	resource_size_t mmio_sz = 0;
> > +	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
> > +	int ret = 0;
> > +
> > +	if (!new_range || !new_range->fwnode || !new_range->size)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&io_range_mutex);
> > +	list_for_each_entry_rcu(range, &io_range_list, list) {
> > +		if (range->fwnode == new_range->fwnode) {
> > +			/* range already there */
> > +			ret = -EFAULT;
> > +			goto end_register;
> > +		}
> 
> This is the -EFAULT that propagates to pci-tegra.c's ->probe() and fails
> to bind the driver.
> 
> I'm not exactly sure what's causing the duplicate here because it's
> rather difficult to get at something useful from just the ->fwnode, but
> I'm fairly sure that the reason this breaks is because the Tegra driver
> will defer probe due to some regulators that aren't available on the
> first try. Given the above code and the rest of this file, I can't see a
> way to "fix" the driver and remove the I/O range on failure.
> 
> This is doubly bad because this doesn't only leak the ranges on probe
> deferral, but also on driver unload, and we just added support for
> building the Tegra driver as a loadable module, so these are actually
> cases that can happen in regular uses of the driver.
> 
> I have no idea on how to fix this. Anyone know of a quick fix to restore
> PCI for Tegra other than reverting all of these changes?
> 
> I suppose an API could be added to unregister the range, but the calling
> sequence is rather obfuscated, so removing the range will look totally
> asymmetric, I'm afraid.
> 
> Here's the call stack:
> 
> 	tegra_pcie_probe()
> 	tegra_pcie_parse_dt()
> 	of_pci_range_to_resource()
> 	pci_register_io_range()
> 	logic_pio_register_range()
> 
> So the range here is registered as part of a resource parsing function,
> which is supposed to not have any side-effects. There's no equivalent of
> that parsing routine (i.e. no "unparse" function that would undo the
> effects of parsing).
> 
> Perhaps a cleaner way would be to decouple the parsing from the actual
> request step that has the side-effect.
> 
> Going back in history a little, it looks like even before this commit
> the I/O range registration was triggered by the parsing code and even
> the range leak was there, except that it caused pci_register_io_range()
> to return 0 rather than -EFAULT. Perhaps the quickest fix for this would
> be to do the same in the new code and restore drivers that accidentally
> depend on this behaviour.

I can confirm that the following fixes the issue for me, though I don't
think it's a very clean fix given that the range will remain requested
forever, even if the driver is gone. But since that's already been the
case for quite a while, probably something that can be fixed separately.

Cc'ing linux-tegra for visibility.

Thierry

--- >8 ---
diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 29cedeadb397..4664b87e1c5f 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -46,7 +46,6 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
 	list_for_each_entry_rcu(range, &io_range_list, list) {
 		if (range->fwnode == new_range->fwnode) {
 			/* range already there */
-			ret = -EFAULT;
 			goto end_register;
 		}
 		if (range->flags == LOGIC_PIO_CPU_MMIO &&

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v17 01/10] LIB: Introduce a generic PIO mapping method

[John Garry]

>>> +int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>>> +{
>>> +	struct logic_pio_hwaddr *range;
>>> +	resource_size_t start = new_range->hw_start;
>>> +	resource_size_t end = new_range->hw_start + new_range->size;
>>> +	resource_size_t mmio_sz = 0;
>>> +	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
>>> +	int ret = 0;
>>> +
>>> +	if (!new_range || !new_range->fwnode || !new_range->size)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&io_range_mutex);
>>> +	list_for_each_entry_rcu(range, &io_range_list, list) {
>>> +		if (range->fwnode == new_range->fwnode) {
>>> +			/* range already there */
>>> +			ret = -EFAULT;
>>> +			goto end_register;
>>> +		}
>>

Hi Thierry,

>> This is the -EFAULT that propagates to pci-tegra.c's ->probe() and fails
>> to bind the driver.
>>
>> I'm not exactly sure what's causing the duplicate here because it's
>> rather difficult to get at something useful from just the ->fwnode, but
>> I'm fairly sure that the reason this breaks is because the Tegra driver
>> will defer probe due to some regulators that aren't available on the
>> first try. Given the above code and the rest of this file, I can't see a
>> way to "fix" the driver and remove the I/O range on failure.
>>
>> This is doubly bad because this doesn't only leak the ranges on probe
>> deferral, but also on driver unload, and we just added support for
>> building the Tegra driver as a loadable module, so these are actually
>> cases that can happen in regular uses of the driver.
>>
>> I have no idea on how to fix this. Anyone know of a quick fix to restore
>> PCI for Tegra other than reverting all of these changes?
>>
>> I suppose an API could be added to unregister the range, but the calling
>> sequence is rather obfuscated, so removing the range will look totally
>> asymmetric, I'm afraid.
>>
>> Here's the call stack:
>>
>> 	tegra_pcie_probe()
>> 	tegra_pcie_parse_dt()
>> 	of_pci_range_to_resource()
>> 	pci_register_io_range()
>> 	logic_pio_register_range()
>>
>> So the range here is registered as part of a resource parsing function,
>> which is supposed to not have any side-effects. There's no equivalent of
>> that parsing routine (i.e. no "unparse" function that would undo the
>> effects of parsing).
>>
>> Perhaps a cleaner way would be to decouple the parsing from the actual
>> request step that has the side-effect.

This could be added if we agreed that it would be useful.

>>
>> Going back in history a little, it looks like even before this commit
>> the I/O range registration was triggered by the parsing code and even
>> the range leak was there, except that it caused pci_register_io_range()
>> to return 0 rather than -EFAULT. Perhaps the quickest fix for this would
>> be to do the same in the new code and restore drivers that accidentally
>> depend on this behaviour.
>
> I can confirm that the following fixes the issue for me, though I don't
> think it's a very clean fix given that the range will remain requested
> forever, even if the driver is gone. But since that's already been the
> case for quite a while, probably something that can be fixed separately.
>

Right, there was no way to deregister the range previously.  From 
looking at the history here I see no reason to not support it.

As for this patch, as you said, the only difference is that we fault on 
trying to register the same range again. So this solution seems reasonable.

On another point, for the tegra driver, is it possible to defer earlier 
in the probe, before these currently irreversible steps are taken?

Thanks very much,
John

> Cc'ing linux-tegra for visibility.
>
> Thierry
>
> --- >8 ---
> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> index 29cedeadb397..4664b87e1c5f 100644
> --- a/lib/logic_pio.c
> +++ b/lib/logic_pio.c
> @@ -46,7 +46,6 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>  	list_for_each_entry_rcu(range, &io_range_list, list) {
>  		if (range->fwnode == new_range->fwnode) {
>  			/* range already there */
> -			ret = -EFAULT;
>  			goto end_register;
>  		}
>  		if (range->flags == LOGIC_PIO_CPU_MMIO &&
>

Re: [PATCH v17 01/10] LIB: Introduce a generic PIO mapping method

[Thierry Reding]

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

On Tue, Apr 03, 2018 at 05:01:37PM +0100, John Garry wrote:
> > > > +int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
> > > > +{
> > > > +	struct logic_pio_hwaddr *range;
> > > > +	resource_size_t start = new_range->hw_start;
> > > > +	resource_size_t end = new_range->hw_start + new_range->size;
> > > > +	resource_size_t mmio_sz = 0;
> > > > +	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
> > > > +	int ret = 0;
> > > > +
> > > > +	if (!new_range || !new_range->fwnode || !new_range->size)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&io_range_mutex);
> > > > +	list_for_each_entry_rcu(range, &io_range_list, list) {
> > > > +		if (range->fwnode == new_range->fwnode) {
> > > > +			/* range already there */
> > > > +			ret = -EFAULT;
> > > > +			goto end_register;
> > > > +		}
> > > 
> 
> Hi Thierry,
> 
> > > This is the -EFAULT that propagates to pci-tegra.c's ->probe() and fails
> > > to bind the driver.
> > > 
> > > I'm not exactly sure what's causing the duplicate here because it's
> > > rather difficult to get at something useful from just the ->fwnode, but
> > > I'm fairly sure that the reason this breaks is because the Tegra driver
> > > will defer probe due to some regulators that aren't available on the
> > > first try. Given the above code and the rest of this file, I can't see a
> > > way to "fix" the driver and remove the I/O range on failure.
> > > 
> > > This is doubly bad because this doesn't only leak the ranges on probe
> > > deferral, but also on driver unload, and we just added support for
> > > building the Tegra driver as a loadable module, so these are actually
> > > cases that can happen in regular uses of the driver.
> > > 
> > > I have no idea on how to fix this. Anyone know of a quick fix to restore
> > > PCI for Tegra other than reverting all of these changes?
> > > 
> > > I suppose an API could be added to unregister the range, but the calling
> > > sequence is rather obfuscated, so removing the range will look totally
> > > asymmetric, I'm afraid.
> > > 
> > > Here's the call stack:
> > > 
> > > 	tegra_pcie_probe()
> > > 	tegra_pcie_parse_dt()
> > > 	of_pci_range_to_resource()
> > > 	pci_register_io_range()
> > > 	logic_pio_register_range()
> > > 
> > > So the range here is registered as part of a resource parsing function,
> > > which is supposed to not have any side-effects. There's no equivalent of
> > > that parsing routine (i.e. no "unparse" function that would undo the
> > > effects of parsing).
> > > 
> > > Perhaps a cleaner way would be to decouple the parsing from the actual
> > > request step that has the side-effect.
> 
> This could be added if we agreed that it would be useful.

I guess in most cases these ranges will be static at least during one
boot. But it still feels like this should be removed when the driver
goes away. While this may not depend on data by the driver, and hence
won't cause a crash or anything, it just seems wrong to leave it
around when the driver no longer isn't.

> > > Going back in history a little, it looks like even before this commit
> > > the I/O range registration was triggered by the parsing code and even
> > > the range leak was there, except that it caused pci_register_io_range()
> > > to return 0 rather than -EFAULT. Perhaps the quickest fix for this would
> > > be to do the same in the new code and restore drivers that accidentally
> > > depend on this behaviour.
> > 
> > I can confirm that the following fixes the issue for me, though I don't
> > think it's a very clean fix given that the range will remain requested
> > forever, even if the driver is gone. But since that's already been the
> > case for quite a while, probably something that can be fixed separately.
> > 
> 
> Right, there was no way to deregister the range previously.  From looking at
> the history here I see no reason to not support it.
> 
> As for this patch, as you said, the only difference is that we fault on
> trying to register the same range again. So this solution seems reasonable.

Okay, I can turn this into a proper patch to fix this up. I suspect that
other drivers may be subject to the same regression. For the longer term
I think it'd be better to properly undo the registration on failure and
removal, but I suspect that it'd be quite a bit of work and not suitable
for v4.17 anymore.

> On another point, for the tegra driver, is it possible to defer earlier in
> the probe, before these currently irreversible steps are taken?

I'm sure it'd be possible. But it would be quite involved, I think. The
reason the code is the way it is is because parsing the DT didn't use to
have side-effects.

Also, I don't think it would buy us much because the probe can still
defer (or at least fail) as late as pci_scan_root_bus_bridge(). Even if
we work around the probe deferral by moving the DT parsing to a later
point we could easily run into a situation where the entry remains in
place and a subsequent attempt to reload the driver would then fail in
the same way as if we were deferring probe.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v17 01/10] LIB: Introduce a generic PIO mapping method

[John Garry]

On 03/04/2018 17:37, Thierry Reding wrote:
> On Tue, Apr 03, 2018 at 05:01:37PM +0100, John Garry wrote:
>>>>> +int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>>>>> +{
>>>>> +	struct logic_pio_hwaddr *range;
>>>>> +	resource_size_t start = new_range->hw_start;
>>>>> +	resource_size_t end = new_range->hw_start + new_range->size;
>>>>> +	resource_size_t mmio_sz = 0;
>>>>> +	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	if (!new_range || !new_range->fwnode || !new_range->size)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	mutex_lock(&io_range_mutex);
>>>>> +	list_for_each_entry_rcu(range, &io_range_list, list) {
>>>>> +		if (range->fwnode == new_range->fwnode) {
>>>>> +			/* range already there */
>>>>> +			ret = -EFAULT;
>>>>> +			goto end_register;
>>>>> +		}
>>>>
>>
>> Hi Thierry,
>>
>>>> This is the -EFAULT that propagates to pci-tegra.c's ->probe() and fails
>>>> to bind the driver.
>>>>
>>>> I'm not exactly sure what's causing the duplicate here because it's
>>>> rather difficult to get at something useful from just the ->fwnode, but
>>>> I'm fairly sure that the reason this breaks is because the Tegra driver
>>>> will defer probe due to some regulators that aren't available on the
>>>> first try. Given the above code and the rest of this file, I can't see a
>>>> way to "fix" the driver and remove the I/O range on failure.
>>>>
>>>> This is doubly bad because this doesn't only leak the ranges on probe
>>>> deferral, but also on driver unload, and we just added support for
>>>> building the Tegra driver as a loadable module, so these are actually
>>>> cases that can happen in regular uses of the driver.
>>>>
>>>> I have no idea on how to fix this. Anyone know of a quick fix to restore
>>>> PCI for Tegra other than reverting all of these changes?
>>>>
>>>> I suppose an API could be added to unregister the range, but the calling
>>>> sequence is rather obfuscated, so removing the range will look totally
>>>> asymmetric, I'm afraid.
>>>>
>>>> Here's the call stack:
>>>>
>>>> 	tegra_pcie_probe()
>>>> 	tegra_pcie_parse_dt()
>>>> 	of_pci_range_to_resource()
>>>> 	pci_register_io_range()
>>>> 	logic_pio_register_range()
>>>>
>>>> So the range here is registered as part of a resource parsing function,
>>>> which is supposed to not have any side-effects. There's no equivalent of
>>>> that parsing routine (i.e. no "unparse" function that would undo the
>>>> effects of parsing).
>>>>
>>>> Perhaps a cleaner way would be to decouple the parsing from the actual
>>>> request step that has the side-effect.
>>
>> This could be added if we agreed that it would be useful.
>
> I guess in most cases these ranges will be static at least during one
> boot. But it still feels like this should be removed when the driver
> goes away. While this may not depend on data by the driver, and hence
> won't cause a crash or anything, it just seems wrong to leave it
> around when the driver no longer isn't.

That sounds reasonable, considering we do unmap the iospace when we 
release - so it looks like currently we're leaving some IO range 
reserved which does not have a mapping.

However this change seems non-trivial, considering we're now even 
coupling the PIO range registration into DT parsing.

>
>>>> Going back in history a little, it looks like even before this commit
>>>> the I/O range registration was triggered by the parsing code and even
>>>> the range leak was there, except that it caused pci_register_io_range()
>>>> to return 0 rather than -EFAULT. Perhaps the quickest fix for this would
>>>> be to do the same in the new code and restore drivers that accidentally
>>>> depend on this behaviour.
>>>
>>> I can confirm that the following fixes the issue for me, though I don't
>>> think it's a very clean fix given that the range will remain requested
>>> forever, even if the driver is gone. But since that's already been the
>>> case for quite a while, probably something that can be fixed separately.
>>>
>>
>> Right, there was no way to deregister the range previously.  From looking at
>> the history here I see no reason to not support it.
>>
>> As for this patch, as you said, the only difference is that we fault on
>> trying to register the same range again. So this solution seems reasonable.
>
> Okay, I can turn this into a proper patch to fix this up. I suspect that
> other drivers may be subject to the same regression. For the longer term
> I think it'd be better to properly undo the registration on failure and
> removal, but I suspect that it'd be quite a bit of work and not suitable
> for v4.17 anymore.

Thanks, I had started to put the patch together but if you're happy to 
continue then that's fine. Please let me know.

>
>> On another point, for the tegra driver, is it possible to defer earlier in
>> the probe, before these currently irreversible steps are taken?
>
> I'm sure it'd be possible. But it would be quite involved, I think. The
> reason the code is the way it is is because parsing the DT didn't use to
> have side-effects.
>
> Also, I don't think it would buy us much because the probe can still
> defer (or at least fail) as late as pci_scan_root_bus_bridge(). Even if
> we work around the probe deferral by moving the DT parsing to a later
> point we could easily run into a situation where the entry remains in
> place and a subsequent attempt to reload the driver would then fail in
> the same way as if we were deferring probe.
>
> Thierry
>

Thanks,
John

Re: [PATCH v17 01/10] LIB: Introduce a generic PIO mapping method

[Bjorn Helgaas]

On Tue, Apr 03, 2018 at 06:02:43PM +0100, John Garry wrote:
> On 03/04/2018 17:37, Thierry Reding wrote:
> > On Tue, Apr 03, 2018 at 05:01:37PM +0100, John Garry wrote:
> > > > > > +int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
> > > > > > +{
> > > > > > +	struct logic_pio_hwaddr *range;
> > > > > > +	resource_size_t start = new_range->hw_start;
> > > > > > +	resource_size_t end = new_range->hw_start + new_range->size;
> > > > > > +	resource_size_t mmio_sz = 0;
> > > > > > +	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
> > > > > > +	int ret = 0;
> > > > > > +
> > > > > > +	if (!new_range || !new_range->fwnode || !new_range->size)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	mutex_lock(&io_range_mutex);
> > > > > > +	list_for_each_entry_rcu(range, &io_range_list, list) {
> > > > > > +		if (range->fwnode == new_range->fwnode) {
> > > > > > +			/* range already there */
> > > > > > +			ret = -EFAULT;
> > > > > > +			goto end_register;
> > > > > > +		}
> > > > > 
> > > 
> > > Hi Thierry,
> > > 
> > > > > This is the -EFAULT that propagates to pci-tegra.c's ->probe() and fails
> > > > > to bind the driver.
> > > > > 
> > > > > I'm not exactly sure what's causing the duplicate here because it's
> > > > > rather difficult to get at something useful from just the ->fwnode, but
> > > > > I'm fairly sure that the reason this breaks is because the Tegra driver
> > > > > will defer probe due to some regulators that aren't available on the
> > > > > first try. Given the above code and the rest of this file, I can't see a
> > > > > way to "fix" the driver and remove the I/O range on failure.
> > > > > 
> > > > > This is doubly bad because this doesn't only leak the ranges on probe
> > > > > deferral, but also on driver unload, and we just added support for
> > > > > building the Tegra driver as a loadable module, so these are actually
> > > > > cases that can happen in regular uses of the driver.
> > > > > 
> > > > > I have no idea on how to fix this. Anyone know of a quick fix to restore
> > > > > PCI for Tegra other than reverting all of these changes?
> > > > > 
> > > > > I suppose an API could be added to unregister the range, but the calling
> > > > > sequence is rather obfuscated, so removing the range will look totally
> > > > > asymmetric, I'm afraid.
> > > > > 
> > > > > Here's the call stack:
> > > > > 
> > > > > 	tegra_pcie_probe()
> > > > > 	tegra_pcie_parse_dt()
> > > > > 	of_pci_range_to_resource()
> > > > > 	pci_register_io_range()
> > > > > 	logic_pio_register_range()
> > > > > 
> > > > > So the range here is registered as part of a resource parsing function,
> > > > > which is supposed to not have any side-effects. There's no equivalent of
> > > > > that parsing routine (i.e. no "unparse" function that would undo the
> > > > > effects of parsing).
> > > > > 
> > > > > Perhaps a cleaner way would be to decouple the parsing from the actual
> > > > > request step that has the side-effect.
> > > 
> > > This could be added if we agreed that it would be useful.
> > 
> > I guess in most cases these ranges will be static at least during one
> > boot. But it still feels like this should be removed when the driver
> > goes away. While this may not depend on data by the driver, and hence
> > won't cause a crash or anything, it just seems wrong to leave it
> > around when the driver no longer isn't.
> 
> That sounds reasonable, considering we do unmap the iospace when we release
> - so it looks like currently we're leaving some IO range reserved which does
> not have a mapping.
> 
> However this change seems non-trivial, considering we're now even coupling
> the PIO range registration into DT parsing.
> 
> > 
> > > > > Going back in history a little, it looks like even before this commit
> > > > > the I/O range registration was triggered by the parsing code and even
> > > > > the range leak was there, except that it caused pci_register_io_range()
> > > > > to return 0 rather than -EFAULT. Perhaps the quickest fix for this would
> > > > > be to do the same in the new code and restore drivers that accidentally
> > > > > depend on this behaviour.
> > > > 
> > > > I can confirm that the following fixes the issue for me, though I don't
> > > > think it's a very clean fix given that the range will remain requested
> > > > forever, even if the driver is gone. But since that's already been the
> > > > case for quite a while, probably something that can be fixed separately.
> > > > 
> > > 
> > > Right, there was no way to deregister the range previously.  From looking at
> > > the history here I see no reason to not support it.
> > > 
> > > As for this patch, as you said, the only difference is that we fault on
> > > trying to register the same range again. So this solution seems reasonable.
> > 
> > Okay, I can turn this into a proper patch to fix this up. I suspect that
> > other drivers may be subject to the same regression. For the longer term
> > I think it'd be better to properly undo the registration on failure and
> > removal, but I suspect that it'd be quite a bit of work and not suitable
> > for v4.17 anymore.
> 
> Thanks, I had started to put the patch together but if you're happy to
> continue then that's fine. Please let me know.

Since you seem to agree this is the right short-term fix and I would
squash it into the original commit anyway, I went ahead and did that
so we could get this into linux-next as soon as possible.

Here's the diff from my previous "next" branch with respect to this
series:

diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 29cedeadb397..4664b87e1c5f 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -46,7 +46,6 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
 	list_for_each_entry_rcu(range, &io_range_list, list) {
 		if (range->fwnode == new_range->fwnode) {
 			/* range already there */
-			ret = -EFAULT;
 			goto end_register;
 		}
 		if (range->flags == LOGIC_PIO_CPU_MMIO &&

Re: [PATCH v17 01/10] LIB: Introduce a generic PIO mapping method

[John Garry]

On 03/04/2018 18:53, Bjorn Helgaas wrote:
> On Tue, Apr 03, 2018 at 06:02:43PM +0100, John Garry wrote:
>> On 03/04/2018 17:37, Thierry Reding wrote:
>>> On Tue, Apr 03, 2018 at 05:01:37PM +0100, John Garry wrote:
>>>>>>> +int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>>>>>>> +{
>>>>>>> +	struct logic_pio_hwaddr *range;
>>>>>>> +	resource_size_t start = new_range->hw_start;
>>>>>>> +	resource_size_t end = new_range->hw_start + new_range->size;
>>>>>>> +	resource_size_t mmio_sz = 0;
>>>>>>> +	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
>>>>>>> +	int ret = 0;
>>>>>>> +
>>>>>>> +	if (!new_range || !new_range->fwnode || !new_range->size)
>>>>>>> +		return -EINVAL;
>>>>>>> +
>>>>>>> +	mutex_lock(&io_range_mutex);
>>>>>>> +	list_for_each_entry_rcu(range, &io_range_list, list) {
>>>>>>> +		if (range->fwnode == new_range->fwnode) {
>>>>>>> +			/* range already there */
>>>>>>> +			ret = -EFAULT;
>>>>>>> +			goto end_register;
>>>>>>> +		}
>>>>>>
>>>>
>>>> Hi Thierry,
>>>>
>>>>>> This is the -EFAULT that propagates to pci-tegra.c's ->probe() and fails
>>>>>> to bind the driver.
>>>>>>
>>>>>> I'm not exactly sure what's causing the duplicate here because it's
>>>>>> rather difficult to get at something useful from just the ->fwnode, but
>>>>>> I'm fairly sure that the reason this breaks is because the Tegra driver
>>>>>> will defer probe due to some regulators that aren't available on the
>>>>>> first try. Given the above code and the rest of this file, I can't see a
>>>>>> way to "fix" the driver and remove the I/O range on failure.
>>>>>>
>>>>>> This is doubly bad because this doesn't only leak the ranges on probe
>>>>>> deferral, but also on driver unload, and we just added support for
>>>>>> building the Tegra driver as a loadable module, so these are actually
>>>>>> cases that can happen in regular uses of the driver.
>>>>>>
>>>>>> I have no idea on how to fix this. Anyone know of a quick fix to restore
>>>>>> PCI for Tegra other than reverting all of these changes?
>>>>>>
>>>>>> I suppose an API could be added to unregister the range, but the calling
>>>>>> sequence is rather obfuscated, so removing the range will look totally
>>>>>> asymmetric, I'm afraid.
>>>>>>
>>>>>> Here's the call stack:
>>>>>>
>>>>>> 	tegra_pcie_probe()
>>>>>> 	tegra_pcie_parse_dt()
>>>>>> 	of_pci_range_to_resource()
>>>>>> 	pci_register_io_range()
>>>>>> 	logic_pio_register_range()
>>>>>>
>>>>>> So the range here is registered as part of a resource parsing function,
>>>>>> which is supposed to not have any side-effects. There's no equivalent of
>>>>>> that parsing routine (i.e. no "unparse" function that would undo the
>>>>>> effects of parsing).
>>>>>>
>>>>>> Perhaps a cleaner way would be to decouple the parsing from the actual
>>>>>> request step that has the side-effect.
>>>>
>>>> This could be added if we agreed that it would be useful.
>>>
>>> I guess in most cases these ranges will be static at least during one
>>> boot. But it still feels like this should be removed when the driver
>>> goes away. While this may not depend on data by the driver, and hence
>>> won't cause a crash or anything, it just seems wrong to leave it
>>> around when the driver no longer isn't.
>>
>> That sounds reasonable, considering we do unmap the iospace when we release
>> - so it looks like currently we're leaving some IO range reserved which does
>> not have a mapping.
>>
>> However this change seems non-trivial, considering we're now even coupling
>> the PIO range registration into DT parsing.
>>
>>>
>>>>>> Going back in history a little, it looks like even before this commit
>>>>>> the I/O range registration was triggered by the parsing code and even
>>>>>> the range leak was there, except that it caused pci_register_io_range()
>>>>>> to return 0 rather than -EFAULT. Perhaps the quickest fix for this would
>>>>>> be to do the same in the new code and restore drivers that accidentally
>>>>>> depend on this behaviour.
>>>>>
>>>>> I can confirm that the following fixes the issue for me, though I don't
>>>>> think it's a very clean fix given that the range will remain requested
>>>>> forever, even if the driver is gone. But since that's already been the
>>>>> case for quite a while, probably something that can be fixed separately.
>>>>>
>>>>
>>>> Right, there was no way to deregister the range previously.  From looking at
>>>> the history here I see no reason to not support it.
>>>>
>>>> As for this patch, as you said, the only difference is that we fault on
>>>> trying to register the same range again. So this solution seems reasonable.
>>>
>>> Okay, I can turn this into a proper patch to fix this up. I suspect that
>>> other drivers may be subject to the same regression. For the longer term
>>> I think it'd be better to properly undo the registration on failure and
>>> removal, but I suspect that it'd be quite a bit of work and not suitable
>>> for v4.17 anymore.
>>
>> Thanks, I had started to put the patch together but if you're happy to
>> continue then that's fine. Please let me know.
>
> Since you seem to agree this is the right short-term fix and I would
> squash it into the original commit anyway, I went ahead and did that
> so we could get this into linux-next as soon as possible.
>

Ok, thanks.

John

> Here's the diff from my previous "next" branch with respect to this
> series:
>
> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> index 29cedeadb397..4664b87e1c5f 100644
> --- a/lib/logic_pio.c
> +++ b/lib/logic_pio.c
> @@ -46,7 +46,6 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>  	list_for_each_entry_rcu(range, &io_range_list, list) {
>  		if (range->fwnode == new_range->fwnode) {
>  			/* range already there */
> -			ret = -EFAULT;
>  			goto end_register;
>  		}
>  		if (range->flags == LOGIC_PIO_CPU_MMIO &&
>
> .
>