[PATCH V3 0/4] ARM64 LPC: legacy ISA I/O support

[PATCH V3 0/4] ARM64 LPC: legacy ISA I/O support

[Zhichang Yuan]

From: "zhichang.yuan" <yuanzhichang@hisilicon.com>

This patch supports the 16550 compatible UART attached to the Low-Pin-Count
interface mplemented on Hisilicon Hip06 SoC. The periperals attached this LPC
include UART, BT, KCS, and so on.
	                -----------
			| LPC host|
	                |         |
	                -----------
	                     |
 	        _____________V_______________LPC
                  |			  |
                  V	                  V
             -----------             ------------
             |  UART   |             |  BT(ipmi)|
             -----------             ------------

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

This patch set is built based on mainline v4.8-rc6;

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

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

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

zhichang.yuan (4):
  ARM64 LPC: Indirect ISA port IO introduced
  ARM64 LPC: LPC driver implementation on Hip06
  ARM64 LPC: support serial based on low-pin-count
  ARM64 LPC: support earlycon for UART connected to LPC

 .../arm/hisilicon/hisilicon-low-pin-count.txt      |  35 +
 .../devicetree/bindings/serial/hisi-lpc-uart.txt   |  60 ++
 arch/arm64/Kconfig                                 |   6 +
 arch/arm64/include/asm/io.h                        |  90 +++
 drivers/bus/Kconfig                                |   8 +
 drivers/bus/Makefile                               |   2 +
 drivers/bus/extio.c                                |  66 ++
 drivers/bus/hisi_lpc.c                             | 766 +++++++++++++++++++++
 drivers/of/address.c                               |   9 +
 drivers/tty/serial/8250/8250_early.c               |  26 +-
 drivers/tty/serial/8250/8250_hisi_lpc.c            | 171 +++++
 drivers/tty/serial/8250/Kconfig                    |   9 +
 drivers/tty/serial/8250/Makefile                   |   1 +
 include/linux/extio.h                              |  49 ++
 14 files changed, 1296 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
 create mode 100644 Documentation/devicetree/bindings/serial/hisi-lpc-uart.txt
 create mode 100644 drivers/bus/extio.c
 create mode 100644 drivers/bus/hisi_lpc.c
 create mode 100644 drivers/tty/serial/8250/8250_hisi_lpc.c
 create mode 100644 include/linux/extio.h

-- 
1.9.1

[PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced

[Zhichang Yuan]

From: "zhichang.yuan" <yuanzhichang@hisilicon.com>

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

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

Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
---
 arch/arm64/Kconfig          |  6 +++
 arch/arm64/include/asm/io.h | 90 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/bus/extio.c         | 66 +++++++++++++++++++++++++++++++++
 include/linux/extio.h       | 49 ++++++++++++++++++++++++
 4 files changed, 211 insertions(+)
 create mode 100644 drivers/bus/extio.c
 create mode 100644 include/linux/extio.h

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index bc3f00f..9579479 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -161,6 +161,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
 config ARCH_MMAP_RND_COMPAT_BITS_MAX
        default 16
 
+config ARM64_INDIRECT_PIO
+	def_bool n
+	help
+	  Support to access the ISA I/O devices with the legacy X86 I/O port
+	  addresses in some SoCs, such as Hisilicon Hip06.
+
 config NO_IOPORT_MAP
 	def_bool y if !PCI
 
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 9b6e408..d3acf1f 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -34,6 +34,10 @@
 
 #include <xen/xen.h>
 
+#ifdef CONFIG_ARM64_INDIRECT_PIO
+#include <linux/extio.h>
+#endif
+
 /*
  * Generic IO read/write.  These perform native-endian accesses.
  */
@@ -142,6 +146,38 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
 #define writel(v,c)		({ __iowmb(); writel_relaxed((v),(c)); })
 #define writeq(v,c)		({ __iowmb(); writeq_relaxed((v),(c)); })
 
+
+#define BUILDS_RW(bwl, type)						\
+static inline void reads##bwl(const volatile void __iomem *addr,	\
+				void *buffer, unsigned int count)	\
+{									\
+	if (count) {							\
+		type *buf = buffer;					\
+									\
+		do {							\
+			type x = __raw_read##bwl(addr);			\
+			*buf++ = x;					\
+		} while (--count);					\
+	}								\
+}									\
+									\
+static inline void writes##bwl(volatile void __iomem *addr,		\
+				const void *buffer, unsigned int count)	\
+{									\
+	if (count) {							\
+		const type *buf = buffer;				\
+									\
+		do {							\
+			__raw_write##bwl(*buf++, addr);			\
+		} while (--count);					\
+	}								\
+}
+
+BUILDS_RW(b, u8)
+#define readsb readsb
+#define writesb writesb
+
+
 /*
  *  I/O port access primitives.
  */
@@ -149,6 +185,60 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
 #define IO_SPACE_LIMIT		(PCI_IO_SIZE - 1)
 #define PCI_IOBASE		((void __iomem *)PCI_IO_START)
 
+
+/*
+ * redefine the in(s)b/out(s)b for indirect-IO.
+ */
+#define inb inb
+static inline u8 inb(unsigned long addr)
+{
+#ifdef CONFIG_ARM64_INDIRECT_PIO
+	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
+			addr <= arm64_extio_ops->end)
+		return extio_inb(addr);
+#endif
+	return readb(PCI_IOBASE + addr);
+}
+
+
+#define outb outb
+static inline void outb(u8 value, unsigned long addr)
+{
+#ifdef CONFIG_ARM64_INDIRECT_PIO
+	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
+			addr <= arm64_extio_ops->end)
+		extio_outb(value, addr);
+	else
+#endif
+		writeb(value, PCI_IOBASE + addr);
+}
+
+#define insb insb
+static inline void insb(unsigned long addr, void *buffer, unsigned int count)
+{
+#ifdef CONFIG_ARM64_INDIRECT_PIO
+	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
+			addr <= arm64_extio_ops->end)
+		extio_insb(addr, buffer, count);
+	else
+#endif
+		readsb(PCI_IOBASE + addr, buffer, count);
+}
+
+#define outsb outsb
+static inline void outsb(unsigned long addr, const void *buffer,
+			 unsigned int count)
+{
+#ifdef CONFIG_ARM64_INDIRECT_PIO
+	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
+			addr <= arm64_extio_ops->end)
+		extio_outsb(addr, buffer, count);
+	else
+#endif
+		writesb(PCI_IOBASE + addr, buffer, count);
+}
+
+
 /*
  * String version of I/O memory access operations.
  */
diff --git a/drivers/bus/extio.c b/drivers/bus/extio.c
new file mode 100644
index 0000000..1e7a9c5
--- /dev/null
+++ b/drivers/bus/extio.c
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ * Author: Zou Rongrong <@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/io.h>
+
+
+struct extio_ops *arm64_extio_ops;
+
+
+u8 __weak extio_inb(unsigned long addr)
+{
+	return arm64_extio_ops->pfin ?
+		arm64_extio_ops->pfin(arm64_extio_ops->devpara,
+			addr + arm64_extio_ops->ptoffset, NULL,
+			sizeof(u8), 1) : -1;
+}
+
+void __weak extio_outb(u8 value, unsigned long addr)
+{
+	if (!arm64_extio_ops->pfout)
+		return;
+
+	arm64_extio_ops->pfout(arm64_extio_ops->devpara,
+			addr + arm64_extio_ops->ptoffset, &value,
+			sizeof(u8), 1);
+}
+
+
+void __weak extio_insb(unsigned long addr, void *buffer,
+				unsigned int count)
+{
+	if (!arm64_extio_ops->pfin)
+		return;
+
+	arm64_extio_ops->pfin(arm64_extio_ops->devpara,
+			addr + arm64_extio_ops->ptoffset, buffer,
+			sizeof(u8), count);
+}
+
+void __weak extio_outsb(unsigned long addr, const void *buffer,
+			 unsigned int count)
+{
+	if (!arm64_extio_ops->pfout)
+		return;
+
+	arm64_extio_ops->pfout(arm64_extio_ops->devpara,
+			addr + arm64_extio_ops->ptoffset, buffer,
+			sizeof(u8), count);
+}
+
+
diff --git a/include/linux/extio.h b/include/linux/extio.h
new file mode 100644
index 0000000..08d1fca
--- /dev/null
+++ b/include/linux/extio.h
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ * Author: Zou Rongrong <@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __LINUX_EXTIO_H
+#define __LINUX_EXTIO_H
+
+
+typedef u64 (*inhook)(void *devobj, unsigned long ptaddr, void *inbuf,
+				size_t dlen, unsigned int count);
+typedef void (*outhook)(void *devobj, unsigned long ptaddr,
+				const void *outbuf, size_t dlen,
+				unsigned int count);
+
+struct extio_ops {
+	unsigned long start;/* inclusive, sys io addr */
+	unsigned long end;/* inclusive, sys io addr */
+	unsigned long ptoffset;/* port Io - system Io */
+
+	inhook	pfin;
+	outhook	pfout;
+	void *devpara;
+};
+
+
+extern struct extio_ops *arm64_extio_ops;
+
+extern u8 extio_inb(unsigned long addr);
+extern void extio_outb(u8 value, unsigned long addr);
+extern void extio_insb(unsigned long addr, void *buffer, unsigned int count);
+extern void extio_outsb(unsigned long addr, const void *buffer,
+				unsigned int count);
+
+
+#endif /* __LINUX_EXTIO_H*/
-- 
1.9.1

[PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Zhichang Yuan]

From: "zhichang.yuan" <yuanzhichang@hisilicon.com>

On Hip06, the accesses to LPC peripherals work in an indirect way. A
corresponding LPC driver configure some registers in LPC master at first, then
the real accesses on LPC slave devices are finished by the LPC master, which
is transparent to LPC driver.
This patch implement the relevant driver for Hip06 LPC. Cooperating with
indirect-IO, ipmi messages is in service without any changes on ipmi driver.

Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
---
 .../arm/hisilicon/hisilicon-low-pin-count.txt      |  35 ++
 drivers/bus/Kconfig                                |   8 +
 drivers/bus/Makefile                               |   2 +
 drivers/bus/hisi_lpc.c                             | 653 +++++++++++++++++++++
 drivers/of/address.c                               |   9 +
 5 files changed, 707 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..820e26d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
@@ -0,0 +1,35 @@
+Hisilicon Hip06 low-pin-count device
+  Usually LPC controller is part of PCI host bridge, so the legacy ISA ports
+  locate on LPC bus can be accessed direclty. But some SoCs have independent
+  LPC controller, and access the legacy ports by triggering LPC I/O cycles.
+  Hisilicon Hip06 implements this LPC device.
+
+Required properties:
+- compatible: should be "hisilicon,low-pin-count"
+- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
+- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
+- reg: base address and length of the register set for the device.
+- ranges: define a 1:1 mapping between the I/O space of the child device and
+	  the parent.
+
+Note:
+  The node name before '@' must be "isa" to represent the binding stick to the
+  ISA/EISA binding specification.
+
+Example:
+
+isa@a01b0000 {
+	compatible = "hisilicom,low-pin-count";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	reg = <0x0 0xa01b0000 0x0 0x1000>;
+	ranges = <0x01 0xe4 0x0 0xe4 0x1000>;
+
+
+	ipmi0: bt@e4 {
+		compatible = "ipmi-bt";
+		device_type = "ipmi";
+		reg = <0x01 0xe4 0x04>;
+		status = "disabled";
+	};
+};
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 3b205e2..fdb232b 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -64,6 +64,14 @@ config BRCMSTB_GISB_ARB
 	  arbiter. This driver provides timeout and target abort error handling
 	  and internal bus master decoding.
 
+config HISILICON_LPC
+	bool "Workaround for nonstandard ISA I/O space on Hisilicon Hip0X"
+	depends on (ARCH_HISI || COMPILE_TEST) && ARM64
+	select ARM64_INDIRECT_PIO
+	help
+	  Driver needed for some legacy ISA devices attached to Low-Pin-Count
+	  on Hisilicon Hip0X Soc.
+
 config IMX_WEIM
 	bool "Freescale EIM DRIVER"
 	depends on ARCH_MXC
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index ac84cc4..6ffbb27 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -7,6 +7,8 @@ obj-$(CONFIG_ARM_CCI)		+= arm-cci.o
 obj-$(CONFIG_ARM_CCN)		+= arm-ccn.o
 
 obj-$(CONFIG_BRCMSTB_GISB_ARB)	+= brcmstb_gisb.o
+obj-$(CONFIG_ARM64_INDIRECT_PIO)	+= extio.o
+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..9b364d0
--- /dev/null
+++ b/drivers/bus/hisi_lpc.c
@@ -0,0 +1,653 @@
+/*
+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ * Author: Zou Rongrong <@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/acpi.h>
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/io.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/serial_8250.h>
+#include <linux/slab.h>
+
+
+struct hisilpc_dev;
+
+/* This flag is specific to differentiate earlycon operations and the others */
+#define FG_EARLYCON_LPC		0x0001
+/*
+ * this bit set means each IO operation will target to different port address;
+ * 0 means repeatly IO operations will be sticked on the same port, such as BT;
+ */
+#define FG_INCRADDR_LPC		0x0002
+
+struct lpc_cycle_para {
+	unsigned int opflags;
+	unsigned int csize;/*the data length of each operation*/
+};
+
+struct hisilpc_dev {
+	spinlock_t cycle_lock;
+	void __iomem  *membase;
+	struct platform_device *pltdev;
+};
+
+
+/* The maximum continous operations*/
+#define LPC_MAX_OPCNT	16
+
+#define LPC_REG_START		(0x00)/*start a new LPC cycle*/
+#define LPC_REG_OP_STATUS	(0x04)/*the current LPC status*/
+#define LPC_REG_IRQ_ST		(0x08)/*interrupt enable&status*/
+#define LPC_REG_OP_LEN		(0x10)/*how many LPC cycles each start*/
+#define LPC_REG_CMD		(0x14)/*command for the required LPC cycle*/
+#define LPC_REG_ADDR		(0x20)/*LPC target address*/
+#define LPC_REG_WDATA		(0x24)/*data to be written*/
+#define LPC_REG_RDATA		(0x28)/*data coming from peer*/
+
+
+/* The command register fields*/
+#define LPC_CMD_SAMEADDR_SING	(0x00000008)
+#define LPC_CMD_SAMEADDR_INC	(0x00000000)
+#define LPC_CMD_TYPE_IO		(0x00000000)
+#define LPC_CMD_TYPE_MEM	(0x00000002)
+#define LPC_CMD_TYPE_FWH	(0x00000004)
+#define LPC_CMD_WRITE		(0x00000001)
+#define LPC_CMD_READ		(0x00000000)
+
+#define LPC_IRQ_CLEAR		(0x02)
+#define LPC_IRQ_OCCURRED	(0x02)
+
+#define LPC_STATUS_IDLE		(0x01)
+#define LPC_OP_FINISHED		(0x02)
+
+#define START_WORK		(0x01)
+
+/*
+ * The minimal waiting interval... Suggest it is not less than 10.
+ * Bigger value probably will lower the performance.
+ */
+#define LPC_NSEC_PERWAIT	100
+/*
+ * The maximum waiting time is about 128us.
+ * 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 specfic for single IO operation, such as inb. */
+#define LPC_PEROP_WAITCNT	100
+
+
+static inline int wait_lpc_idle(unsigned char *mbase,
+				unsigned int waitcnt) {
+	u32 opstatus = 0;
+
+	while (waitcnt--) {
+		ndelay(LPC_NSEC_PERWAIT);
+		opstatus = readl(mbase + LPC_REG_OP_STATUS);
+		if (opstatus & LPC_STATUS_IDLE)
+			return (opstatus & LPC_OP_FINISHED) ? 0 : (-EIO);
+	}
+	return -ETIME;
+}
+
+
+/**
+ * hisilpc_target_in - trigger a series of lpc cycles to read required data
+ *		  from target periperal.
+ * @pdev: pointer to hisi lpc device
+ * @para: some paramerters used to control the lpc I/O operations
+ * @ptaddr: the lpc I/O target port address
+ * @buf: where the read back data is stored
+ * @opcnt: how many I/O operations required in this calling
+ *
+ * only one byte data is read each I/O operation.
+ *
+ * Returns 0 on success, non-zero on fail.
+ *
+ */
+static int hisilpc_target_in(struct hisilpc_dev *pdev,
+				struct lpc_cycle_para *para,
+				unsigned long ptaddr, unsigned char *buf,
+				unsigned long opcnt)
+{
+	unsigned int cmd_word;
+	unsigned int waitcnt;
+	int retval;
+	/*initialized as 0 to remove compile warning */
+	unsigned long flags = 0;
+
+	if (!buf || !opcnt || !para || !pdev)
+		return -EINVAL;
+
+	if (para->csize != 1 || opcnt  > LPC_MAX_OPCNT)
+		return -EINVAL;
+
+	cmd_word = LPC_CMD_TYPE_IO | LPC_CMD_READ;
+	waitcnt = (LPC_PEROP_WAITCNT);
+	if (!(para->opflags & FG_INCRADDR_LPC)) {
+		cmd_word |= LPC_CMD_SAMEADDR_SING;
+		waitcnt = LPC_MAX_WAITCNT;
+	}
+
+	/* whole operation must be atomic */
+	if (!(para->opflags & FG_EARLYCON_LPC))
+		spin_lock_irqsave(&pdev->cycle_lock, flags);
+
+	writel(opcnt, pdev->membase + LPC_REG_OP_LEN);
+
+	writel(cmd_word, pdev->membase + LPC_REG_CMD);
+
+	writel(ptaddr, pdev->membase + LPC_REG_ADDR);
+
+	writel(START_WORK, pdev->membase + LPC_REG_START);
+
+	/* whether the operation is finished */
+	retval = wait_lpc_idle(pdev->membase, waitcnt);
+	if (!retval) {
+		for (; opcnt--; buf++)
+			*buf = readl(pdev->membase + LPC_REG_RDATA);
+	}
+
+	if (!(para->opflags & FG_EARLYCON_LPC))
+		spin_unlock_irqrestore(&pdev->cycle_lock, flags);
+
+	return retval;
+}
+
+/**
+ * hisilpc_target_out - trigger a series of lpc cycles to write required data
+ *		  to target periperal.
+ * @pdev: pointer to hisi lpc device
+ * @para: some paramerters used to control the lpc I/O operations
+ * @ptaddr: the lpc I/O target port address
+ * @buf: where the data to be written is stored
+ * @opcnt: how many I/O operations required
+ *
+ * only one byte data is read each I/O operation.
+ *
+ * Returns 0 on success, non-zero on fail.
+ *
+ */
+static int hisilpc_target_out(struct hisilpc_dev *pdev,
+				struct lpc_cycle_para *para,
+				unsigned long ptaddr,
+				const unsigned char *buf,
+				unsigned long opcnt)
+{
+	unsigned int cmd_word;
+	unsigned int waitcnt;
+	int retval;
+	/* initialized as 0 to remove compile warning */
+	unsigned long flags = 0;
+
+
+	if (!buf || !opcnt || !para || !pdev)
+		return -EINVAL;
+
+	if (para->csize != 1 || opcnt  > LPC_MAX_OPCNT)
+		return -EINVAL;
+
+	cmd_word = LPC_CMD_TYPE_IO | LPC_CMD_WRITE;
+	waitcnt = (LPC_PEROP_WAITCNT);
+	if (!(para->opflags & FG_INCRADDR_LPC)) {
+		cmd_word |= LPC_CMD_SAMEADDR_SING;
+		waitcnt = LPC_MAX_WAITCNT;
+	}
+
+	/* whole operation must be atomic */
+	if (!(para->opflags & FG_EARLYCON_LPC))
+		spin_lock_irqsave(&pdev->cycle_lock, flags);
+
+	writel(opcnt, pdev->membase + LPC_REG_OP_LEN);
+	for (; opcnt--; buf++)
+		writel(*buf, pdev->membase + LPC_REG_WDATA);
+
+	writel(cmd_word, pdev->membase + LPC_REG_CMD);
+
+	writel(ptaddr, pdev->membase + LPC_REG_ADDR);
+
+	writel(START_WORK, pdev->membase + LPC_REG_START);
+
+	/* whether the operation is finished */
+	retval = wait_lpc_idle(pdev->membase, waitcnt);
+
+	if (!(para->opflags & FG_EARLYCON_LPC))
+		spin_unlock_irqrestore(&pdev->cycle_lock, flags);
+
+	return retval;
+}
+
+/**
+ * hisilpc_comm_inb - read/input the data from the I/O peripheral through LPC.
+ * @devobj: pointer to the device information relevant to LPC controller.
+ * @outbuf: a buffer where the data read is stored at.
+ * @ptaddr: the target I/O port address.
+ * @dlen: the data length required to read from the target I/O port.
+ * @count: how many I/O operations required in this calling.  >1 is for ins.
+ *
+ * For this lpc, only support inb/insb now.
+ *
+ * For inbs, returns 0 on success, -1 on fail.
+ * when succeed, the data read back is stored in buffer pointed by inbuf.
+ * For inb, return the data read from I/O or -1 when error occur.
+ */
+u64 hisilpc_comm_inb(void *devobj, unsigned long ptaddr,
+				void *inbuf, size_t dlen,
+				unsigned int count)
+{
+	struct hisilpc_dev *lpcdev;
+	struct lpc_cycle_para iopara;
+	unsigned int loopcnt, cntleft;
+	unsigned int rd_data;
+	unsigned char *newbuf;
+	int ret = 0;
+	/* only support data unit length is 1 now... */
+	if (!count || (!inbuf && count != 1) || !devobj || dlen != 1)
+		return -1;
+
+	newbuf = (unsigned char *)inbuf;
+	/*
+	 * the operation data len is 4 bytes, need to ensure the buffer
+	 * is big enough.
+	 */
+	if (!inbuf || count < sizeof(u32))
+		newbuf = (unsigned char *)&rd_data;
+
+	lpcdev = (struct hisilpc_dev *)devobj;
+	dev_dbg(&lpcdev->pltdev->dev, "In-IO(0x%lx), count=%u\n", ptaddr,
+			count);
+
+	iopara.opflags = FG_INCRADDR_LPC;
+	/*
+	 * to improve performance, support repeatly rd at same target
+	 * address.
+	 */
+	if (count > 1)
+		iopara.opflags &= ~FG_INCRADDR_LPC;
+
+	iopara.csize = dlen;
+
+	cntleft = count;
+	do {
+		loopcnt = (cntleft > LPC_MAX_OPCNT) ? LPC_MAX_OPCNT : cntleft;
+		ret = hisilpc_target_in(lpcdev,
+				&iopara, ptaddr, newbuf, loopcnt);
+		if (ret)
+			return -1;
+		newbuf += loopcnt;
+		cntleft -= loopcnt;
+	} while (cntleft);
+
+	/* for inb */
+	if (!inbuf)
+		return rd_data;
+	/* for insb, copy the data to the return variable */
+	if (inbuf != newbuf)
+		memcpy(inbuf, &rd_data, count);
+
+	return 0;
+}
+
+/**
+ * hisilpc_comm_outb - write/output the data in out buffer to the I/O peripheral
+ *		    through LPC.
+ * @devobj: pointer to the device information relevant to LPC controller.
+ * @outbuf: a buffer where the data to be written is stored.
+ * @ptaddr: the target I/O port address.
+ * @dlen: the data length required writing to the target I/O port .
+ * @count: how many I/O operations required in this calling. >1 is for outs.
+ *
+ * For this lpc, only support outb/outsb now.
+ *
+ */
+void hisilpc_comm_outb(void *devobj, unsigned long ptaddr,
+				const void *outbuf, size_t dlen,
+				unsigned int count)
+{
+	struct hisilpc_dev *lpcdev;
+	struct lpc_cycle_para iopara;
+	unsigned int loopcnt;
+	const unsigned char *newbuf;
+	int ret = 0;
+
+	if (!count || !outbuf || !devobj)
+		return;
+
+	newbuf = (const unsigned char *)outbuf;
+	lpcdev = (struct hisilpc_dev *)devobj;
+
+	dev_dbg(&lpcdev->pltdev->dev, "Out-IO(0x%lx), cnt=%u\n", ptaddr, count);
+
+	iopara.opflags = FG_INCRADDR_LPC;
+	/* to improve performance,  support repeatly wr same target address */
+	if (count > 1)
+		iopara.opflags &= ~FG_INCRADDR_LPC;
+
+	iopara.csize = 1;
+
+	do {
+		loopcnt = (count > LPC_MAX_OPCNT) ? LPC_MAX_OPCNT : count;
+		ret = hisilpc_target_out(lpcdev,
+				&iopara, ptaddr, newbuf, loopcnt);
+		if (ret)
+			return;
+		newbuf += loopcnt;
+		count -= loopcnt;
+	} while (count);
+}
+
+
+/**
+ * hisilpc_ischild_ipmi - check whether the designated device is ipmi
+ * @dev: the device to be checked.
+ * @data: the value used to match the acpi device in checking.
+ *
+ * Returns 1 means the device to be checked is matched.
+ * 0 means some failures.
+ *
+ */
+static int  hisilpc_ischild_ipmi(struct device *dev)
+{
+	struct acpi_device *adev;
+	struct acpi_hardware_id *hwid;
+	/* only support dts and acpi */
+	if (IS_ERR_OR_NULL(dev->fwnode) && !dev->of_node) {
+		dev_err(dev, "Not valid child device!\n");
+		return -EINVAL;
+	}
+
+	adev = ACPI_COMPANION(dev);
+	if (adev) {
+		list_for_each_entry(hwid, &adev->pnp.ids, list) {
+			dev_info(dev, "hwid is %s\n", hwid->id);
+			if (!strcmp("IPI0001", hwid->id))
+				return 1;
+		}
+	} else {
+		if (!strcmp(dev->of_node->type, "ipmi"))
+			return 1;
+	}
+
+	dev_info(dev, "not ipmi child device!\n");
+	return 0;
+}
+
+
+/**
+ * hisilpc_children_map_sysio - setup the mapping between system Io and
+ *			physical IO
+ *
+ * @child: the device whose IO is handling
+ * @data: some device specific data. For ACPI device, should be NULL.
+ *
+ * Returns >=0 means the mapping is successfully created;
+ * others mean some failures.
+ */
+static int hisilpc_children_map_sysio(struct device * child, void * data)
+{
+	struct resource *iores;
+	unsigned long cpuio;
+	struct extio_ops *opsnode;
+	int ret;
+	struct hisilpc_dev *lpcdev;
+
+	if (!child || !child->parent)
+		return -EINVAL;
+
+	iores = platform_get_resource_byname(to_platform_device(child),
+					IORESOURCE_IO, "dev_io");
+	if (!iores)
+		return -ENODEV;
+
+	/*
+	 * can not use devm_kzalloc to allocate slab for child before its driver
+	 * start probing. Here allocate the slab with the name of parent.
+	 */
+	opsnode = devm_kzalloc(child->parent, sizeof(*opsnode), GFP_KERNEL);
+	if (!opsnode)
+		return -ENOMEM;
+
+	cpuio = data ? *((unsigned long *)data) : 0;
+
+	opsnode->start = iores->start;
+	opsnode->end = iores->end;
+	opsnode->ptoffset = cpuio ? (cpuio - iores->start) : 0;
+
+	dev_info(child, "map sys port[%lx - %lx] offset=0x%lx",
+				(unsigned long)iores->start,
+				(unsigned long)iores->end,
+				opsnode->ptoffset);
+
+	opsnode->pfin = hisilpc_comm_inb;
+	opsnode->pfout = hisilpc_comm_outb;
+
+	lpcdev = platform_get_drvdata(to_platform_device(child->parent));
+	opsnode->devpara = lpcdev;
+
+	/* only apply indirect-IO to ipmi child device */
+	ret = hisilpc_ischild_ipmi(child);
+	if (ret < 0)
+		return ret;
+
+	if (ret > 0) {
+		WRITE_ONCE(arm64_extio_ops, opsnode);
+		/* update ret as successful */
+		ret = 0;
+	} else {
+		ret = platform_device_add_data(to_platform_device(child),
+						opsnode,
+						sizeof(*opsnode));
+		devm_kfree(child->parent, opsnode);
+	}
+
+	if (!ret)
+		dev_info(child, "to phy [%lx-%lx]\n", cpuio,
+			(unsigned long)(iores->end - iores->start) + cpuio);
+	else
+		dev_info(child, "FAIL(0x%x)!!\n", ret);
+
+	return ret;
+}
+
+/**
+ * of_hisilpc_register_pio - register the deivce physical IO address into
+ *			io_range_list
+ *
+ * Returns >=0 means ok.
+ * others mean some failures.
+ */
+static int of_hisilpc_register_pio(struct device_node *dev,
+					unsigned long *phyport)
+{
+	static int isfirst = 0;
+	const __be32	*addrp;
+	u64		size;
+	unsigned int	flags;
+	u64	taddr;
+	int residx = 0;
+
+
+	if (!dev || !phyport)
+		return -EINVAL;
+	/* only one IO reg exists */
+	do {
+		addrp = of_get_address(dev, residx, &size, &flags);
+		if (addrp == NULL) {
+			pr_err("%s:: get OF address(%d) FAIL!\n",
+				dev->name, residx);
+			return -EINVAL;
+		}
+		residx++;
+	} while(!(flags & IORESOURCE_IO));
+
+
+	taddr = of_translate_address(dev, addrp);
+	if (taddr == OF_BAD_ADDR) {
+		pr_err("%s:: translate IO address fail\n", dev->name);
+		return -EINVAL;
+	}
+
+	/* register one more IO byte for the first register */
+	if (!isfirst) {
+		size += 1;
+		isfirst = 1;
+	}
+
+	if (pci_register_io_range(taddr, size)) {
+		pr_err("%s::register physical range[%llx, %llx) FAIL!\n",
+			dev->name, taddr, size);
+		return -ENXIO;
+	}
+
+	pr_info("%s:: register physical range[%llx, %llx) OK\n",
+			dev->name, taddr, size);
+
+	*phyport = taddr;
+
+	return 0;
+}
+
+/**
+ * hisilpc_probe_child_dev - setup the mapping between linux IO and
+ *			physical IO for all children under hisilpc
+ *
+ * @ppdev: point to the hisilpc device
+ *
+ * Returns =0 means ok.
+ * others mean some failures.
+ */
+static int hisilpc_probe_child_dev(struct device *ppdev)
+{
+	int ret;
+
+	if (!ppdev)
+		return -EINVAL;
+
+	ret = 0;
+	/* for device tree, scan the child devices now */
+	if (!has_acpi_companion(ppdev)) {
+		struct device_node *root, *child;
+
+		root = ppdev->of_node;
+		for_each_available_child_of_node(root, child) {
+			struct platform_device *ptdev;
+			unsigned long cpuio;
+
+			/* register the IO range configured in dt */
+			ret = of_hisilpc_register_pio(child, &cpuio);
+			if (ret) {
+				dev_err(ppdev, "fail to register raw IO for %s\n",
+						child->name);
+				return ret;
+			}
+
+			ptdev = of_platform_device_create(child, NULL, ppdev);
+			if (!ptdev) {
+				dev_err(ppdev, "create platform device fail for %s\n",
+					child->name);
+				return -EFAULT;
+			}
+
+			ret = hisilpc_children_map_sysio(&ptdev->dev, &cpuio);
+			if (ret)
+				dev_err(&ptdev->dev, "Mapping sysio for dts child devices FAIL\n");
+		}
+	} else {
+		ret = device_for_each_child(ppdev, NULL,
+					hisilpc_children_map_sysio);
+		if (ret)
+			dev_err(ppdev, "Mapping sysio for ACPI child devices FAIL\n");
+	}
+
+	return ret;
+}
+
+
+/**
+ * hisilpc_probe - the probe callback function for hisi lpc device,
+ *		will finish all the intialization.
+ * @pdev: the platform device corresponding to hisi lpc
+ *
+ * Returns 0 on success, non-zero on fail.
+ *
+ */
+static int hisilpc_probe(struct platform_device *pdev)
+{
+	struct resource *iores;
+	struct hisilpc_dev *lpcdev;
+	int ret;
+
+	dev_info(&pdev->dev, "hslpc start probing...\n");
+
+	lpcdev = devm_kzalloc(&pdev->dev,
+				sizeof(struct hisilpc_dev), GFP_KERNEL);
+	if (!lpcdev)
+		return -ENOMEM;
+
+	spin_lock_init(&lpcdev->cycle_lock);
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	lpcdev->membase = devm_ioremap_resource(&pdev->dev, iores);
+	if (IS_ERR(lpcdev->membase)) {
+		dev_err(&pdev->dev, "No mem resource\n");
+		return PTR_ERR(lpcdev->membase);
+	}
+
+	lpcdev->pltdev = pdev;
+	platform_set_drvdata(pdev, lpcdev);
+
+	ret = hisilpc_probe_child_dev(&pdev->dev);
+	if (!ret)
+		dev_info(&pdev->dev, "hslpc finish probing...\n");
+	else
+		dev_err(&pdev->dev, "hslpc probe got fail(%d)!\n", -ret);
+
+	return ret;
+}
+
+
+static const struct of_device_id hisilpc_of_match[] = {
+	{
+		.compatible = "hisilicon,low-pin-count",
+	},
+	{},
+};
+
+static const struct acpi_device_id hisilpc_acpi_match[] = {
+	{"HISI0191", },
+	{},
+};
+
+static struct platform_driver hisilpc_driver = {
+	.driver = {
+		.name           = "hisi_lpc",
+		.of_match_table = hisilpc_of_match,
+		.acpi_match_table = hisilpc_acpi_match,
+	},
+	.probe = hisilpc_probe,
+};
+
+
+builtin_platform_driver(hisilpc_driver);
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903..ba4a330 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -690,9 +690,18 @@ static int __of_address_to_resource(struct device_node *dev,
 	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;
+		/*
+		 * special processing for non-pci device gurantee the linux start pio
+		 * is not ZERO. Otherwise, some drivers' initialization will fail.
+		 */
+		if (!port && (!IS_ENABLED(CONFIG_OF_ADDRESS_PCI) ||
+				!of_bus_pci_match(dev)))
+			port += 1;
+
 		r->start = port;
 		r->end = port + size - 1;
 	} else {
-- 
1.9.1

[PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count

[Zhichang Yuan]

From: "zhichang.yuan" <yuanzhichang@hisilicon.com>

On Hip06 platform, a 16550 compatible UART is connected to low-pin-count and
controlled through the LPC I/O cycles. After registering the LPC uart specific
serial_in/serial_out to 8250 core driver, serial data can be read/written
through the LPC.

Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
---
 .../devicetree/bindings/serial/hisi-lpc-uart.txt   |  60 ++++++++
 drivers/tty/serial/8250/8250_hisi_lpc.c            | 171 +++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig                    |   9 ++
 drivers/tty/serial/8250/Makefile                   |   1 +
 4 files changed, 241 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/hisi-lpc-uart.txt
 create mode 100644 drivers/tty/serial/8250/8250_hisi_lpc.c

diff --git a/Documentation/devicetree/bindings/serial/hisi-lpc-uart.txt b/Documentation/devicetree/bindings/serial/hisi-lpc-uart.txt
new file mode 100644
index 0000000..0f8ec2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/hisi-lpc-uart.txt
@@ -0,0 +1,60 @@
+* Hisilicon hip06 UART through low-pin-count
+
+Required properties:
+- compatible : "hisilicon,lpc-uart"
+- reg : offset and length of the I/O port set for the device.
+- reg-names : name the uart resources.
+		"lpc_mem" represents memory resource of the LPC parent. When
+		flat-tree earlycon is needed, this memory resource and
+		resource name are mandatory. Otherwise they are optional;
+		"dev_io" represents I/O resource of this device. It is
+		mandatory to access this device;
+
+Clock handling:
+  The clock rate of this device is same as the 8250 default clock rate, that
+  is 1843200. No need to define the clock rate in device tree.
+
+Note:
+  This device depends on its parent device whose compatible string is
+  "hisilicon,low-pin-count".
+
+  The format of "reg" property follows the I/O space definition in ISA/EISA
+  binding specification linked to:
+  http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
+
+Example:
+
+	uart0: lpc-uart@2f8 {
+		compatible = "hisilicon,lpc-uart";
+		reg = <0x01 0x2f8 0x08>;
+		status = "disabled";
+	};
+
+
+Example with low-pin-count parent device:
+
+	isa@a01b0000 {
+		compatible = "hisilicon,low-pin-count";
+		#address-cells = <2>;
+		#size-cells = <1>;
+		reg = <0x0 0xa01b0000 0x0 0x1000>;
+		ranges = <0x00 0xa01b0000 0x00 0xa01b0000 0x1000>,
+			<0x01 0xe4 0x0 0xe4 0x04>,
+			<0x01 0x2f8 0x0 0x2f8 0x08>;
+
+		ipmi0: bt@e4 {
+			compatible = "ipmi-bt";
+			device_type = "ipmi";
+			reg = <0x01 0xe4 0x04>;
+			reg-names = "dev_io";
+			status = "disabled";
+		};
+
+		uart0: lpc-uart@2f8 {
+			compatible = "hisilicon,lpc-uart";
+			reg =  <0x00 0xa01b0000 0x1000>,
+				<0x01 0x2f8 0x08>;
+			ret-names = "lpc_mem", "dev_io";
+			status = "disabled";
+		};
+	};
diff --git a/drivers/tty/serial/8250/8250_hisi_lpc.c b/drivers/tty/serial/8250/8250_hisi_lpc.c
new file mode 100644
index 0000000..a8ba1ca
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_hisi_lpc.c
@@ -0,0 +1,171 @@
+/*
+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ * Author: Zou Rongrong <@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/serial_8250.h>
+#include <asm-generic/serial.h>
+#include <linux/of_address.h>
+
+
+/**
+ * hisilpc_serial_inb - read/input data from the designated serial port.
+ * @p: the serial port where the data read from
+ * @offset:  the target I/O port address where the read is from
+ *
+ * Returns the byte data from this serial port.
+ * -1 means some failures.
+ *
+ */
+static unsigned int hisilpc_serial_inb(struct uart_port *p, int offset)
+{
+	struct extio_ops *parentops;
+
+	parentops = p->private_data;
+	if (!parentops || !parentops->pfin)
+		return -1;
+
+	return parentops->pfin(parentops->devpara,
+				p->iobase + (offset << p->regshift),
+				NULL, sizeof(u8), 1);
+}
+
+/**
+ * hisilpc_serial_outb - write/output data from the designated serial port.
+ * @p: the serial port where the data is written to
+ * @offset:  the target I/O port address where the write is from
+ *
+ */
+static void hisilpc_serial_outb(struct uart_port *p, int offset, int value)
+{
+	struct extio_ops *parentops;
+
+	parentops = p->private_data;
+	if (!parentops || !parentops->pfout)
+		return;
+
+	parentops->pfout(parentops->devpara,
+				p->iobase + (offset << p->regshift),
+				&value, sizeof(u8), 1);
+}
+
+
+static int hisilpc8250_probe(struct platform_device *pdev)
+{
+	struct uart_8250_port uart = {};
+	struct uart_port *port = &uart.port;
+	int err = 0;
+	struct resource *iores;
+	struct extio_ops *platdata;
+
+	if (!pdev->dev.parent)
+		return -ENODEV;
+	dev_info(&pdev->dev, "##probe entering\n");
+
+	/* To support the earlycon in bootargs, the first reg must be MEM */
+	iores = platform_get_resource_byname(pdev, IORESOURCE_IO,
+						"dev_io");
+	if (!iores) {
+		dev_err(&pdev->dev, "can not find the IO0\n");
+		return -ENXIO;
+	}
+
+	/*
+	 * save the platform data from parent in uart_port for serial_in,
+	 * serial_out
+	 */
+	platdata = dev_get_platdata(&pdev->dev);
+	port->private_data = (void *)platdata;
+	if (!port->private_data) {
+		dev_err(&pdev->dev, "no platform data!\n");
+		return -ENODEV;
+	}
+
+	if (platdata->start != iores->start || platdata->end != iores->end) {
+		dev_err(&pdev->dev, "PIO range[0x%lx - %lx] isn't fit!\n",
+			(unsigned long)iores->start,
+			(unsigned long)iores->end);
+		return -ENXIO;
+	}
+	port->iobase = (unsigned long)iores->start + platdata->ptoffset;
+	dev_info(&pdev->dev, "real port start is 0x%lx\n", port->iobase);
+
+	port->irq	= 0;
+	port->flags	= UPF_BOOT_AUTOCONF | UPF_FIXED_PORT;
+	port->dev	= &pdev->dev;
+	port->iotype	= UPIO_PORT;
+	port->regshift	= 0;
+	port->uartclk	= BASE_BAUD * 16;
+
+	spin_lock_init(&port->lock);
+
+	port->serial_in = hisilpc_serial_inb;
+	port->serial_out = hisilpc_serial_outb;
+
+	err = serial8250_register_8250_port(&uart);
+	if (err < 0) {
+		dev_err(&pdev->dev, "register uart FAIL(%d)!\n", -err);
+		return err;
+	}
+
+	platform_set_drvdata(pdev, (void *)&err);
+	dev_info(&pdev->dev, "##probing OK(%d)\n", err);
+	return 0;
+}
+
+static int hisilpc8250_remove(struct platform_device *pdev)
+{
+	int line = *((int *)platform_get_drvdata(pdev));
+
+	serial8250_unregister_port(line);
+
+	return 0;
+}
+
+
+static const struct of_device_id hs8250_of_match[] = {
+	{ .compatible = "hisilicon,lpc-uart" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, hs8250_of_match);
+
+static const struct acpi_device_id hs8250_acpi_match[] = {
+	/*{ "PNP0501", 0 },*/
+	{ "HISI1031", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, hs8250_acpi_match);
+
+static struct platform_driver hs_lpc8250_driver = {
+	.driver = {
+		.name		= "hisi-lpc-uart",
+		.of_match_table	= hs8250_of_match,
+		.acpi_match_table = ACPI_PTR(hs8250_acpi_match),
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+	.probe			= hisilpc8250_probe,
+	.remove			= hisilpc8250_remove,
+};
+
+module_platform_driver(hs_lpc8250_driver);
+
+
+MODULE_AUTHOR("Rongrong Zou");
+MODULE_DESCRIPTION("8250 serial probe module for Hisilicon LPC UART");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("v1.0");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 7c6f7af..c2e42f7 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -246,6 +246,15 @@ config SERIAL_8250_HUB6
 	  To compile this driver as a module, choose M here: the module
 	  will be called 8250_hub6.
 
+config SERIAL_8250_HISI_LPC
+	tristate "Support Hisilicon Hip0X UART through LPC"
+	depends on SERIAL_8250 !=n && HISILICON_LPC
+	help
+	  Say Y here if you have a hip06 board.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called 8250_hisi_lpc.
+
 #
 # Misc. options/drivers.
 #
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 367d403..1f2915b 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
+obj-$(CONFIG_SERIAL_8250_HISI_LPC)	+= 8250_hisi_lpc.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
 obj-$(CONFIG_SERIAL_8250_OMAP)		+= 8250_omap.o
-- 
1.9.1

[PATCH V3 4/4] ARM64 LPC: support earlycon for UART connected to LPC

[Zhichang Yuan]

From: "zhichang.yuan" <yuanzhichang@hisilicon.com>

This patch support the earlycon for UART connected to LPC on Hip06.
This patch is depended on the LPC driver.

Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
---
 drivers/bus/hisi_lpc.c               | 113 +++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_early.c |  26 +++++++-
 2 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 9b364d0..2269828 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -360,6 +360,119 @@ void hisilpc_comm_outb(void *devobj, unsigned long ptaddr,
 
 
 /**
+ * hisilpc_early_in - read/input operation specific for hisi LPC earlycon.
+ * @devobj: pointer to device relevant information of the caller.
+ * @inbuf: the buffer where the read back data is populated.
+ *
+ * for earlycon, dlen and count should be one.
+ *
+ * Return the data read from earlycon on success, error ID on fail.
+ *
+ */
+static unsigned int __init hisilpc_early_in(struct uart_port *port, int offset)
+{
+	unsigned int backval = 0;
+	unsigned int ret = 0;
+	struct lpc_cycle_para para;
+	struct hisilpc_dev lpcdev;
+
+	if (!port->mapbase || !port->iobase || !port->membase)
+		return -EINVAL;
+
+	para.opflags = FG_EARLYCON_LPC;
+	para.csize = 1;
+	lpcdev.membase = port->membase;
+
+	ret = hisilpc_target_in(&lpcdev, &para,
+				port->iobase + (offset << port->regshift),
+				(unsigned char *)&backval, 1);
+	return (ret) ? : backval;
+}
+
+/**
+ * hisilpc_early_out - write/output operation specific for hisi LPC earlycon.
+ * @port: pointer to uart_port of eralycon
+ *
+ * for earlycon, dlen and count should be one.
+ *
+ */
+static void __init hisilpc_early_out(struct uart_port *port, int offset,
+					int value)
+{
+	struct lpc_cycle_para para;
+	struct hisilpc_dev lpcdev;
+
+	if (!port->mapbase || !port->iobase || !port->membase)
+		return;
+
+	para.opflags = FG_EARLYCON_LPC;
+	para.csize = 1;
+	lpcdev.membase = port->membase;
+
+	(void)hisilpc_target_out(&lpcdev, &para,
+				port->iobase + (offset << port->regshift),
+				(unsigned char *)&value, 1);
+}
+
+
+/**
+ * early_hisilpc8250_setup - initilize the lpc earlycon
+ * @device: pointer to the elarycon device
+ * @options: a option string from earlycon kernel-parameter
+ *
+ * Returns 0 on success, non-zero on fail.
+ *
+ */
+static int __init early_hisilpc8250_setup(struct earlycon_device *device,
+						const char *options)
+{
+	char *p;
+	int ret;
+
+	if (!device->port.membase)
+		return -ENODEV;
+
+	if (device->port.iotype != UPIO_MEM)
+		return -EINVAL;
+
+	if (device->options) {
+		p = strchr(device->options, ',');
+		if (p && (p + 1) != '\0') {
+			ret = kstrtoul(++p, 0,
+				(unsigned long *)&device->port.iobase);
+			if (ret || device->port.iobase == 0)
+				return ret ?: -EFAULT;
+		} else
+			device->port.iobase = 0x2f8;
+	} else {
+		device->port.iobase = 0x2f8;
+		device->baud = 0;
+	}
+
+	device->port.serial_in = hisilpc_early_in;
+	device->port.serial_out = hisilpc_early_out;
+	/* must convert iotype to UPIO_PORT for Hip06 indirect-io */
+	device->port.iotype = UPIO_PORT;
+
+	/* disable interrupts from LPC */
+	writel(LPC_IRQ_CLEAR, device->port.membase + LPC_REG_IRQ_ST);
+	/* ensure the LPC is available */
+	while (!(readl(device->port.membase + LPC_REG_OP_STATUS) &
+			LPC_STATUS_IDLE))
+		cpu_relax();
+
+	return early_serial8250_setup(device, options);
+}
+
+
+
+EARLYCON_DECLARE(hisilpcuart, early_hisilpc8250_setup);
+OF_EARLYCON_DECLARE(hisilpcuart, "hisilicon,lpc-uart",
+					early_hisilpc8250_setup);
+
+
+
+/**
  * hisilpc_ischild_ipmi - check whether the designated device is ipmi
  * @dev: the device to be checked.
  * @data: the value used to match the acpi device in checking.
diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index 85a12f0..3b5d0a8 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -37,7 +37,7 @@
 #include <asm/io.h>
 #include <asm/serial.h>
 
-static unsigned int __init serial8250_early_in(struct uart_port *port, int offset)
+static unsigned int __init serial8250_early_in_raw(struct uart_port *port, int offset)
 {
 	offset <<= port->regshift;
 
@@ -57,7 +57,7 @@ static unsigned int __init serial8250_early_in(struct uart_port *port, int offse
 	}
 }
 
-static void __init serial8250_early_out(struct uart_port *port, int offset, int value)
+static void __init serial8250_early_out_raw(struct uart_port *port, int offset, int value)
 {
 	offset <<= port->regshift;
 
@@ -80,6 +80,28 @@ static void __init serial8250_early_out(struct uart_port *port, int offset, int
 	}
 }
 
+static inline void __init serial8250_early_out(struct uart_port *port,
+					int offset, int value)
+{
+	if (port->serial_out)
+		port->serial_out(port, offset, value);
+	else {
+		port->serial_out = serial8250_early_out_raw;
+		serial8250_early_out_raw(port, offset, value);
+	}
+}
+
+static inline unsigned int __init serial8250_early_in(struct uart_port *port,
+					int offset)
+{
+	if (port->serial_in)
+		return port->serial_in(port, offset);
+
+	port->serial_in = serial8250_early_in_raw;
+	return serial8250_early_in_raw(port, offset);
+}
+
+
 #define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
 
 static void __init serial_putc(struct uart_port *port, int c)
-- 
1.9.1

Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced

[Arnd Bergmann]

On Wednesday, September 14, 2016 8:15:51 PM CEST Zhichang Yuan wrote:
> From: "zhichang.yuan" <yuanzhichang@hisilicon.com>
> 
> For arm64, there is no I/O space as other architectural platforms, such as
> X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
> such as Hip06, when accessing some legacy ISA devices connected to LPC, those
> known port addresses are used to control the corresponding target devices, for
> example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
> normal MMIO mode in using.
> 
> To drive these devices, this patch introduces a method named indirect-IO.
> In this method the in/out pair in arch/arm64/include/asm/io.h will be
> redefined. When upper layer drivers call in/out with those known legacy port
> addresses to access the peripherals, the hooking functions corrresponding to
> those target peripherals will be called. Through this way, those upper layer
> drivers which depend on in/out can run on Hip06 without any changes.
> 
> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>

Looks ok overall, but I have a couple of comments for details.

> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index bc3f00f..9579479 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -161,6 +161,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
>         default 16
>  
> +config ARM64_INDIRECT_PIO
> +	def_bool n

'def_bool n' is the same as the shorter and more common 'bool'.

> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 9b6e408..d3acf1f 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -34,6 +34,10 @@
>  
>  #include <xen/xen.h>
>  
> +#ifdef CONFIG_ARM64_INDIRECT_PIO
> +#include <linux/extio.h>
> +#endif

No need to guard includes with an #ifdef.

> +#define BUILDS_RW(bwl, type)						\
> +static inline void reads##bwl(const volatile void __iomem *addr,	\
> +				void *buffer, unsigned int count)	\
> +{									\
> +	if (count) {							\
> +		type *buf = buffer;					\
> +									\
> +		do {							\
> +			type x = __raw_read##bwl(addr);			\
> +			*buf++ = x;					\
> +		} while (--count);					\
> +	}								\
> +}									\
> +									\
> +static inline void writes##bwl(volatile void __iomem *addr,		\
> +				const void *buffer, unsigned int count)	\
> +{									\
> +	if (count) {							\
> +		const type *buf = buffer;				\
> +									\
> +		do {							\
> +			__raw_write##bwl(*buf++, addr);			\
> +		} while (--count);					\
> +	}								\
> +}
> +
> +BUILDS_RW(b, u8)

Why is this in here?

> @@ -149,6 +185,60 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>  #define IO_SPACE_LIMIT		(PCI_IO_SIZE - 1)
>  #define PCI_IOBASE		((void __iomem *)PCI_IO_START)
>  
> +
> +/*
> + * redefine the in(s)b/out(s)b for indirect-IO.
> + */
> +#define inb inb
> +static inline u8 inb(unsigned long addr)
> +{
> +#ifdef CONFIG_ARM64_INDIRECT_PIO
> +	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
> +			addr <= arm64_extio_ops->end)
> +		return extio_inb(addr);
> +#endif
> +	return readb(PCI_IOBASE + addr);
> +}
> +

Looks ok, but you only seem to do this for the 8-bit
accessors, when it should be done for 16-bit and 32-bit
ones as well for consistency.

> diff --git a/drivers/bus/extio.c b/drivers/bus/extio.c
> new file mode 100644
> index 0000000..1e7a9c5
> --- /dev/null
> +++ b/drivers/bus/extio.c
> @@ -0,0 +1,66 @@

This is in a globally visible directory

> +
> +struct extio_ops *arm64_extio_ops;

But the identifier uses an architecture specific prefix. Either
move the whole file into arch/arm64, or make the naming so that
it can be used for everything.

> +u8 __weak extio_inb(unsigned long addr)
> +{
> +	return arm64_extio_ops->pfin ?
> +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,
> +			addr + arm64_extio_ops->ptoffset, NULL,
> +			sizeof(u8), 1) : -1;
> +}

No need for the __weak attribute, just make sure that the
code is always built-in when needed.

Also, it doesn't seem necessary to have an extern function if
all it does is call the one callback that you have already 
checked earlier. Either put it all into the inline
definition in asm/io.h, or put it all into the extern
version like this.

#ifdef CONFIG_ARM64_INDIRECT_PIO /* otherwise use default from asm-generic */
#define inb inb
extern u8 inb(unsigned long addr);
#endif

u8 inb(unsigned long addr)
{
	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
			addr <= arm64_extio_ops->end)
		arm64_extio_ops->pfin(arm64_extio_ops->devpara,addr + arm64_extio_ops->ptoffset, NULL,sizeof(u8), 1) : -1;
	return extio_inb(addr);
}

> +#define inb inb
> +static inline u8 inb(unsigned long addr)
> +{
> +#ifdef CONFIG_ARM64_INDIRECT_PIO
> +	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
> +			addr <= arm64_extio_ops->end)
> +		return extio_inb(addr);
> +#endif
> +	return readb(PCI_IOBASE + addr);
> +}

> diff --git a/include/linux/extio.h b/include/linux/extio.h
> new file mode 100644
> index 0000000..08d1fca
> --- /dev/null
> +++ b/include/linux/extio.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
> + * Author: Zou Rongrong <@huawei.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __LINUX_EXTIO_H
> +#define __LINUX_EXTIO_H
> +
> +
> +typedef u64 (*inhook)(void *devobj, unsigned long ptaddr, void *inbuf,
> +				size_t dlen, unsigned int count);
> +typedef void (*outhook)(void *devobj, unsigned long ptaddr,
> +				const void *outbuf, size_t dlen,
> +				unsigned int count);

I would drop the typedef and just declare the types directly in the
only place that references them.

	Arnd

Re: [PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count

[Arnd Bergmann]

On Wednesday, September 14, 2016 8:15:53 PM CEST Zhichang Yuan wrote:
> From: "zhichang.yuan" <yuanzhichang@hisilicon.com>
> 
> On Hip06 platform, a 16550 compatible UART is connected to low-pin-count and
> controlled through the LPC I/O cycles. After registering the LPC uart specific
> serial_in/serial_out to 8250 core driver, serial data can be read/written
> through the LPC.
> 
> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> 

I still think this should be handled by 8250_of.c after the addition of
support for IORESOURCE_IO.

	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan wrote:

> +Required properties:
> +- compatible: should be "hisilicon,low-pin-count"
> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> +- reg: base address and length of the register set for the device.
> +- ranges: define a 1:1 mapping between the I/O space of the child device and
> +	  the parent.

Do we still need the "ranges" here? The property in your example seems
wrong.

> +	ranges = <0x01 0xe4 0x0 0xe4 0x1000>;

You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4?

> +/**
> + * hisilpc_children_map_sysio - setup the mapping between system Io and
> + *			physical IO
> + *
> + * @child: the device whose IO is handling
> + * @data: some device specific data. For ACPI device, should be NULL.
> + *
> + * Returns >=0 means the mapping is successfully created;
> + * others mean some failures.
> + */
> +static int hisilpc_children_map_sysio(struct device * child, void * data)
> +{
> +	struct resource *iores;
> +	unsigned long cpuio;
> +	struct extio_ops *opsnode;
> +	int ret;
> +	struct hisilpc_dev *lpcdev;
> +
> +	if (!child || !child->parent)
> +		return -EINVAL;
> +
> +	iores = platform_get_resource_byname(to_platform_device(child),
> +					IORESOURCE_IO, "dev_io");
> +	if (!iores)
> +		return -ENODEV;
> +
> +	/*
> +	 * can not use devm_kzalloc to allocate slab for child before its driver
> +	 * start probing. Here allocate the slab with the name of parent.
> +	 */
> +	opsnode = devm_kzalloc(child->parent, sizeof(*opsnode), GFP_KERNEL);
> +	if (!opsnode)
> +		return -ENOMEM;
> +
> +	cpuio = data ? *((unsigned long *)data) : 0;
> +
> +	opsnode->start = iores->start;
> +	opsnode->end = iores->end;
> +	opsnode->ptoffset = cpuio ? (cpuio - iores->start) : 0;
> +
> +	dev_info(child, "map sys port[%lx - %lx] offset=0x%lx",
> +				(unsigned long)iores->start,
> +				(unsigned long)iores->end,
> +				opsnode->ptoffset);
> +
> +	opsnode->pfin = hisilpc_comm_inb;
> +	opsnode->pfout = hisilpc_comm_outb;
> +
> +	lpcdev = platform_get_drvdata(to_platform_device(child->parent));
> +	opsnode->devpara = lpcdev;
> +
> +	/* only apply indirect-IO to ipmi child device */

I don't get this part. The bus driver should not care what its
children are, just register and PIO ranges that the bus can handle
in theory, i.e. from 0x000 to 0xfff.

	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[kbuild test robot]

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

Hi zhichang.yuan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.8-rc6 next-20160914]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Zhichang-Yuan/ARM64-LPC-legacy-ISA-I-O-support/20160914-202858
config: openrisc-or1ksim_defconfig (attached as .config)
compiler: or32-linux-gcc (GCC) 4.5.1-or32-1.0rc1
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=openrisc 

All errors (new ones prefixed by >>):

   drivers/of/address.c: In function '__of_address_to_resource':
>> drivers/of/address.c:702:5: error: implicit declaration of function 'of_bus_pci_match'

vim +/of_bus_pci_match +702 drivers/of/address.c

   696				return -EINVAL;
   697			/*
   698			 * special processing for non-pci device gurantee the linux start pio
   699			 * is not ZERO. Otherwise, some drivers' initialization will fail.
   700			 */
   701			if (!port && (!IS_ENABLED(CONFIG_OF_ADDRESS_PCI) ||
 > 702					!of_bus_pci_match(dev)))
   703				port += 1;
   704	
   705			r->start = port;

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 7187 bytes --]

Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced

[zhichang.yuan]

Hi, Arnd

On 2016/9/14 20:24, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 8:15:51 PM CEST Zhichang Yuan wrote:
>> From: "zhichang.yuan" <yuanzhichang@hisilicon.com>
>>
>> For arm64, there is no I/O space as other architectural platforms, such as
>> X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
>> such as Hip06, when accessing some legacy ISA devices connected to LPC, those
>> known port addresses are used to control the corresponding target devices, for
>> example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
>> normal MMIO mode in using.
>>
>> To drive these devices, this patch introduces a method named indirect-IO.
>> In this method the in/out pair in arch/arm64/include/asm/io.h will be
>> redefined. When upper layer drivers call in/out with those known legacy port
>> addresses to access the peripherals, the hooking functions corrresponding to
>> those target peripherals will be called. Through this way, those upper layer
>> drivers which depend on in/out can run on Hip06 without any changes.
>>
>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> 
> Looks ok overall, but I have a couple of comments for details.
> 
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index bc3f00f..9579479 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -161,6 +161,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
>>         default 16
>>  
>> +config ARM64_INDIRECT_PIO
>> +	def_bool n
> 
> 'def_bool n' is the same as the shorter and more common 'bool'.
Yes. Will modify as bool "access peripherals with legacy I/O port"

> 
>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>> index 9b6e408..d3acf1f 100644
>> --- a/arch/arm64/include/asm/io.h
>> +++ b/arch/arm64/include/asm/io.h
>> @@ -34,6 +34,10 @@
>>  
>>  #include <xen/xen.h>
>>  
>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>> +#include <linux/extio.h>
>> +#endif
> 
> No need to guard includes with an #ifdef.
If remove #ifdef here, extio.h should not contain any function external declarations whose definitions are in
extio.c compiled only when CONFIG_ARM64_INDIRECT_PIO is yes.


How about removing everything about the configure item "ARM64_INDIRECT_PIO"?
This will make the indirect-IO mechanism global on ARM64.
I worry about this mechanism is not so common, so using "ARM64_INDIRECT_PIO" make this feature optional.

> 
>> +#define BUILDS_RW(bwl, type)						\
>> +static inline void reads##bwl(const volatile void __iomem *addr,	\
>> +				void *buffer, unsigned int count)	\
>> +{									\
>> +	if (count) {							\
>> +		type *buf = buffer;					\
>> +									\
>> +		do {							\
>> +			type x = __raw_read##bwl(addr);			\
>> +			*buf++ = x;					\
>> +		} while (--count);					\
>> +	}								\
>> +}									\
>> +									\
>> +static inline void writes##bwl(volatile void __iomem *addr,		\
>> +				const void *buffer, unsigned int count)	\
>> +{									\
>> +	if (count) {							\
>> +		const type *buf = buffer;				\
>> +									\
>> +		do {							\
>> +			__raw_write##bwl(*buf++, addr);			\
>> +		} while (--count);					\
>> +	}								\
>> +}
>> +
>> +BUILDS_RW(b, u8)
> 
> Why is this in here?
the readsb/writesb are defined in asm-generic/io.h which is included later, but the redefined insb/outsb need
to call them. Without these readsb/writesb definition before insb/outsb redefined, compile error occur.

It seems that copy all the definitions of "asm-generic/io.h" is not a good idea, so I move the definitions of
those function needed here....

Ok. I think your idea below defining in(s)/out(s) in a c file can solve this issue.

#ifdef CONFIG_ARM64_INDIRECT_PIO
#define inb inb
extern u8 inb(unsigned long addr);

#define outb outb
extern void outb(u8 value, unsigned long addr);

#define insb insb
extern void insb(unsigned long addr, void *buffer, unsigned int count);

#define outsb outsb
extern void outsb(unsigned long addr, const void *buffer, unsigned int count);
#endif

and definitions of all these functions are in extio.c :

u8 inb(unsigned long addr)
{
	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||
			arm64_extio_ops->end < addr)
		return readb(PCI_IOBASE + addr);
	else
		return arm64_extio_ops->pfin ?
			arm64_extio_ops->pfin(arm64_extio_ops->devpara,
				addr + arm64_extio_ops->ptoffset, NULL,
				sizeof(u8), 1) : -1;
}
.....


> 
>> @@ -149,6 +185,60 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>>  #define IO_SPACE_LIMIT		(PCI_IO_SIZE - 1)
>>  #define PCI_IOBASE		((void __iomem *)PCI_IO_START)
>>  
>> +
>> +/*
>> + * redefine the in(s)b/out(s)b for indirect-IO.
>> + */
>> +#define inb inb
>> +static inline u8 inb(unsigned long addr)
>> +{
>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>> +	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
>> +			addr <= arm64_extio_ops->end)
>> +		return extio_inb(addr);
>> +#endif
>> +	return readb(PCI_IOBASE + addr);
>> +}
>> +
> 
> Looks ok, but you only seem to do this for the 8-bit
> accessors, when it should be done for 16-bit and 32-bit
> ones as well for consistency.
Hip06 LPC only support 8-bit I/O operations on the designated port.

> 
>> diff --git a/drivers/bus/extio.c b/drivers/bus/extio.c
>> new file mode 100644
>> index 0000000..1e7a9c5
>> --- /dev/null
>> +++ b/drivers/bus/extio.c
>> @@ -0,0 +1,66 @@
> 
> This is in a globally visible directory
> 
>> +
>> +struct extio_ops *arm64_extio_ops;
> 
> But the identifier uses an architecture specific prefix. Either
> move the whole file into arch/arm64, or make the naming so that
> it can be used for everything.

I perfer to move the whole file into arch/arm64, extio.h will be moved to arch/arm64/include/asm;

> 
>> +u8 __weak extio_inb(unsigned long addr)
>> +{
>> +	return arm64_extio_ops->pfin ?
>> +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,
>> +			addr + arm64_extio_ops->ptoffset, NULL,
>> +			sizeof(u8), 1) : -1;
>> +}
> 
> No need for the __weak attribute, just make sure that the
> code is always built-in when needed.
> 
> Also, it doesn't seem necessary to have an extern function if
> all it does is call the one callback that you have already 
> checked earlier. Either put it all into the inline
> definition in asm/io.h, or put it all into the extern
> version like this.
> 
> #ifdef CONFIG_ARM64_INDIRECT_PIO /* otherwise use default from asm-generic */
> #define inb inb
> extern u8 inb(unsigned long addr);
> #endif
> 
Yes. This is good!
Although the in(s)/out(s) are not inline anymore.

I had applied this way above.

> u8 inb(unsigned long addr)
> {
> 	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
> 			addr <= arm64_extio_ops->end)
> 		arm64_extio_ops->pfin(arm64_extio_ops->devpara,addr + arm64_extio_ops->ptoffset, NULL,sizeof(u8), 1) : -1;
> 	return extio_inb(addr);
> }
> 
>> +#define inb inb
>> +static inline u8 inb(unsigned long addr)
>> +{
>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>> +	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
>> +			addr <= arm64_extio_ops->end)
>> +		return extio_inb(addr);
>> +#endif
>> +	return readb(PCI_IOBASE + addr);
>> +}
> 
>> diff --git a/include/linux/extio.h b/include/linux/extio.h
>> new file mode 100644
>> index 0000000..08d1fca
>> --- /dev/null
>> +++ b/include/linux/extio.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
>> + * Author: Zou Rongrong <@huawei.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __LINUX_EXTIO_H
>> +#define __LINUX_EXTIO_H
>> +
>> +
>> +typedef u64 (*inhook)(void *devobj, unsigned long ptaddr, void *inbuf,
>> +				size_t dlen, unsigned int count);
>> +typedef void (*outhook)(void *devobj, unsigned long ptaddr,
>> +				const void *outbuf, size_t dlen,
>> +				unsigned int count);
> 
> I would drop the typedef and just declare the types directly in the
> only place that references them.
> 
Ok. Will apply it.

Thanks!
Zhichang

> 	Arnd
> 
> .
> 

Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced

[Arnd Bergmann]

On Wednesday, September 14, 2016 10:16:28 PM CEST zhichang.yuan wrote:
> > 
> > No need to guard includes with an #ifdef.
> If remove #ifdef here, extio.h should not contain any function external declarations whose definitions are in
> extio.c compiled only when CONFIG_ARM64_INDIRECT_PIO is yes.
 
There is no problem with making declarations visible for functions that
are not part of the kernel, we do that all the time.

> >> +#define BUILDS_RW(bwl, type)                                                \
> >> +static inline void reads##bwl(const volatile void __iomem *addr,    \
> >> +                            void *buffer, unsigned int count)       \
> >> +{                                                                   \
> >> +    if (count) {                                                    \
> >> +            type *buf = buffer;                                     \
> >> +                                                                    \
> >> +            do {                                                    \
> >> +                    type x = __raw_read##bwl(addr);                 \
> >> +                    *buf++ = x;                                     \
> >> +            } while (--count);                                      \
> >> +    }                                                               \
> >> +}                                                                   \
> >> +                                                                    \
> >> +static inline void writes##bwl(volatile void __iomem *addr,         \
> >> +                            const void *buffer, unsigned int count) \
> >> +{                                                                   \
> >> +    if (count) {                                                    \
> >> +            const type *buf = buffer;                               \
> >> +                                                                    \
> >> +            do {                                                    \
> >> +                    __raw_write##bwl(*buf++, addr);                 \
> >> +            } while (--count);                                      \
> >> +    }                                                               \
> >> +}
> >> +
> >> +BUILDS_RW(b, u8)
> > 
> > Why is this in here?
> the readsb/writesb are defined in asm-generic/io.h which is included later, but the redefined insb/outsb need
> to call them. Without these readsb/writesb definition before insb/outsb redefined, compile error occur.
> 
> It seems that copy all the definitions of "asm-generic/io.h" is not a good idea, so I move the definitions of
> those function needed here....
> 
> Ok. I think your idea below defining in(s)/out(s) in a c file can solve this issue.
> 
> #ifdef CONFIG_ARM64_INDIRECT_PIO
> #define inb inb
> extern u8 inb(unsigned long addr);
> 
> #define outb outb
> extern void outb(u8 value, unsigned long addr);
> 
> #define insb insb
> extern void insb(unsigned long addr, void *buffer, unsigned int count);
> 
> #define outsb outsb
> extern void outsb(unsigned long addr, const void *buffer, unsigned int count);
> #endif
> 
> and definitions of all these functions are in extio.c :
> 
> u8 inb(unsigned long addr)
> {
>         if (!arm64_extio_ops || arm64_extio_ops->start > addr ||
>                         arm64_extio_ops->end < addr)
>                 return readb(PCI_IOBASE + addr);
>         else
>                 return arm64_extio_ops->pfin ?
>                         arm64_extio_ops->pfin(arm64_extio_ops->devpara,
>                                 addr + arm64_extio_ops->ptoffset, NULL,
>                                 sizeof(u8), 1) : -1;
> }
> .....

Yes, sounds good.

> >> @@ -149,6 +185,60 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
> >>  #define IO_SPACE_LIMIT              (PCI_IO_SIZE - 1)
> >>  #define PCI_IOBASE          ((void __iomem *)PCI_IO_START)
> >>  
> >> +
> >> +/*
> >> + * redefine the in(s)b/out(s)b for indirect-IO.
> >> + */
> >> +#define inb inb
> >> +static inline u8 inb(unsigned long addr)
> >> +{
> >> +#ifdef CONFIG_ARM64_INDIRECT_PIO
> >> +    if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
> >> +                    addr <= arm64_extio_ops->end)
> >> +            return extio_inb(addr);
> >> +#endif
> >> +    return readb(PCI_IOBASE + addr);
> >> +}
> >> +
> > 
> > Looks ok, but you only seem to do this for the 8-bit
> > accessors, when it should be done for 16-bit and 32-bit
> > ones as well for consistency.
> Hip06 LPC only support 8-bit I/O operations on the designated port.

That is an interesting limitation. Maybe still call the extio operations
and have them do WARN_ON_ONCE() instead?

If you get a driver that calls inw/outw on the range that is owned
by the LPC bus, you otherwise get an unhandled page fault in kernel
space, which is not as nice.

> >> diff --git a/drivers/bus/extio.c b/drivers/bus/extio.c
> >> new file mode 100644
> >> index 0000000..1e7a9c5
> >> --- /dev/null
> >> +++ b/drivers/bus/extio.c
> >> @@ -0,0 +1,66 @@
> > 
> > This is in a globally visible directory
> > 
> >> +
> >> +struct extio_ops *arm64_extio_ops;
> > 
> > But the identifier uses an architecture specific prefix. Either
> > move the whole file into arch/arm64, or make the naming so that
> > it can be used for everything.
> 
> I perfer to move the whole file into arch/arm64, extio.h will be moved to arch/arm64/include/asm;

Ok, that simplifies it a lot, you can just do everything in asm/io.h then.

	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[zhichang.yuan]

On 2016/9/14 20:33, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan wrote:
> 
>> +Required properties:
>> +- compatible: should be "hisilicon,low-pin-count"
>> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
>> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
>> +- reg: base address and length of the register set for the device.
>> +- ranges: define a 1:1 mapping between the I/O space of the child device and
>> +	  the parent.
> 
> Do we still need the "ranges" here? The property in your example seems
> wrong.

I think "ranges" is needed.
without this, of_translate_address --> __of_translate_address --> of_translate_one will fail when translating the child's IO resource.

> 
>> +	ranges = <0x01 0xe4 0x0 0xe4 0x1000>;
> 
> You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4?
The hip06 LPC is defined as isa type.
So, 0x01 0xe4 is the local IO address of 0xe4. With this ranges, 0xe4 of child will be 1:1 mapped as 0xe4.
It means no translation.

> 
>> +/**
>> + * hisilpc_children_map_sysio - setup the mapping between system Io and
>> + *			physical IO
>> + *
>> + * @child: the device whose IO is handling
>> + * @data: some device specific data. For ACPI device, should be NULL.
>> + *
>> + * Returns >=0 means the mapping is successfully created;
>> + * others mean some failures.
>> + */
>> +static int hisilpc_children_map_sysio(struct device * child, void * data)
>> +{
>> +	struct resource *iores;
>> +	unsigned long cpuio;
>> +	struct extio_ops *opsnode;
>> +	int ret;
>> +	struct hisilpc_dev *lpcdev;
>> +
>> +	if (!child || !child->parent)
>> +		return -EINVAL;
>> +
>> +	iores = platform_get_resource_byname(to_platform_device(child),
>> +					IORESOURCE_IO, "dev_io");
>> +	if (!iores)
>> +		return -ENODEV;
>> +
>> +	/*
>> +	 * can not use devm_kzalloc to allocate slab for child before its driver
>> +	 * start probing. Here allocate the slab with the name of parent.
>> +	 */
>> +	opsnode = devm_kzalloc(child->parent, sizeof(*opsnode), GFP_KERNEL);
>> +	if (!opsnode)
>> +		return -ENOMEM;
>> +
>> +	cpuio = data ? *((unsigned long *)data) : 0;
>> +
>> +	opsnode->start = iores->start;
>> +	opsnode->end = iores->end;
>> +	opsnode->ptoffset = cpuio ? (cpuio - iores->start) : 0;
>> +
>> +	dev_info(child, "map sys port[%lx - %lx] offset=0x%lx",
>> +				(unsigned long)iores->start,
>> +				(unsigned long)iores->end,
>> +				opsnode->ptoffset);
>> +
>> +	opsnode->pfin = hisilpc_comm_inb;
>> +	opsnode->pfout = hisilpc_comm_outb;
>> +
>> +	lpcdev = platform_get_drvdata(to_platform_device(child->parent));
>> +	opsnode->devpara = lpcdev;
>> +
>> +	/* only apply indirect-IO to ipmi child device */
> 
> I don't get this part. The bus driver should not care what its
> children are, just register and PIO ranges that the bus can handle
> in theory, i.e. from 0x000 to 0xfff.

Just as we discussed in V2, the legacy PIO range is specific to some device, such as for ipmi bt, 0xe4 - 0xe7 will be populated.
I don't want to occupy a larger PIO range in which only small part PIOs are used by our LPC. At this moment, two PIO ranges are using
through the device property configuration, 0xe4-0xe7, 0x2f8-0x2ff.
If we configure 0-0x1000 for the LPC to cover those two ranges, most PIO are wasted and other PIO device on other buses lose the chance to use the PIO below 0x1000.
Otherwise, PIO conflict will happen. So, My idea is only occupied the PIO ranges which are really needed for the children.

And there are probably multiple child devices under LPC, the global arm64_extio_ops only can cover one PIO range. It is fortunate only ipmi driver can not support I/O
operation registering, serial driver has serial_in/serial_out to be registered. So, only the PIO range for ipmi device is stored in arm64_extio_ops and the indirect-IO
works well for ipmi device.

If we think it is nearly no chance that LPC PIO range conflict with other buses, we can allocate 0xe4 - 0x2ff to LPC and store it to arm64_extio_ops. In this case,
the special processing for ipmi device is not needed.

Best,
Zhichang

> 
> 	Arnd
> 
> 
> .
> 

Re: [PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count

[zhichang.yuan]

On 2016/9/14 20:25, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 8:15:53 PM CEST Zhichang Yuan wrote:
>> From: "zhichang.yuan" <yuanzhichang@hisilicon.com>
>>
>> On Hip06 platform, a 16550 compatible UART is connected to low-pin-count and
>> controlled through the LPC I/O cycles. After registering the LPC uart specific
>> serial_in/serial_out to 8250 core driver, serial data can be read/written
>> through the LPC.
>>
>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
>>
> 
> I still think this should be handled by 8250_of.c after the addition of
> support for IORESOURCE_IO.

The 8250_hisi_lpc.c support both ACPI and dts similar to 8250_dw :

+static struct platform_driver hs_lpc8250_driver = {
+	.driver = {
+		.name		= "hisi-lpc-uart",
+		.of_match_table	= hs8250_of_match,
+		.acpi_match_table = ACPI_PTR(hs8250_acpi_match),

So, I am a little confused why we need to support dts in 8250_of.c and support ACPI in another
driver file.

best,
Zhichang


> 
> 	Arnd
> 
> 
> .
> 

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote:
> 
> On 2016/9/14 20:33, Arnd Bergmann wrote:
> > On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan wrote:
> > 
> >> +Required properties:
> >> +- compatible: should be "hisilicon,low-pin-count"
> >> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> >> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> >> +- reg: base address and length of the register set for the device.
> >> +- ranges: define a 1:1 mapping between the I/O space of the child device and
> >> +	  the parent.
> > 
> > Do we still need the "ranges" here? The property in your example seems
> > wrong.
> 
> I think "ranges" is needed.
> without this, of_translate_address --> __of_translate_address --> of_translate_one will fail when translating the child's IO resource.
>
> > 
> >> +	ranges = <0x01 0xe4 0x0 0xe4 0x1000>;
> > 
> > You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4?
> The hip06 LPC is defined as isa type.
> So, 0x01 0xe4 is the local IO address of 0xe4. With this ranges, 0xe4 of child will be 1:1 mapped as 0xe4.
> It means no translation.

No, "no translation" would be leaving out the ranges, we should
fix the code so it handles this case according to the specification
of the ISA DT binding, rather than adding an incorrect ranges
property to make it work with the incorrect Linux implementation.

of_translate_address() should fail here, and whichever code calls
it should try something else, possibly something we have to
implement that can return the correct IORESOURCE_* type.

> > 
> > I don't get this part. The bus driver should not care what its
> > children are, just register and PIO ranges that the bus can handle
> > in theory, i.e. from 0x000 to 0xfff.
> 
> Just as we discussed in V2, the legacy PIO range is specific to some
> device, such as for ipmi bt, 0xe4 - 0xe7 will be populated.
> I don't want to occupy a larger PIO range in which only small part PIOs
> are used by our LPC. At this moment, two PIO ranges are using
> through the device property configuration, 0xe4-0xe7, 0x2f8-0x2ff.
>
> If we configure 0-0x1000 for the LPC to cover those two ranges, most
> PIO are wasted and other PIO device on other buses lose the chance to
> use the PIO below 0x1000.
> Otherwise, PIO conflict will happen. So, My idea is only occupied
> the PIO ranges which are really needed for the children.

The only thing it can realistically conflict with would be another
LPC bus behind on a PCI host bridge. On ARM64, all regular PCI
devices that have IORESOURCE_IO ports are intentionally moved
to (bus) port numbers above 0x1000.

> And there are probably multiple child devices under LPC, the global arm64_extio_ops only can cover one PIO range. It is fortunate only ipmi driver can not support I/O
> operation registering, serial driver has serial_in/serial_out to
> be registered. So, only the PIO range for ipmi device is stored
> in arm64_extio_ops and the indirect-IO
> works well for ipmi device.

You should not do that in the serial driver, please just use the
normal 8250 driver that works fine once you handle the entire
port range.
 

	Arnd

Re: [PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count

[Arnd Bergmann]

On Wednesday, September 14, 2016 11:04:33 PM CEST zhichang.yuan wrote:
> The 8250_hisi_lpc.c support both ACPI and dts similar to 8250_dw :
> 
> +static struct platform_driver hs_lpc8250_driver = {
> +       .driver = {
> +               .name           = "hisi-lpc-uart",
> +               .of_match_table = hs8250_of_match,
> +               .acpi_match_table = ACPI_PTR(hs8250_acpi_match),
> 
> So, I am a little confused why we need to support dts in 8250_of.c and support ACPI in another
> driver file.
> 

After looking again, I'm pretty sure that drivers/tty/serial/8250/8250_pnp.c
will handle the ACPI case without modifications, you just need to adapt
the 8250_of driver to handle IORESOURCE_IO ports. This will be required
anyway with the next version once the LPC bridge is on the PCI bus.

	Arnd

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 14 September 2016 22:32
> To: linux-arm-kernel@lists.infradead.org
> Cc: Yuanzhichang; devicetree@vger.kernel.org;
> lorenzo.pieralisi@arm.com; Gabriele Paoloni; minyard@acm.org;
> gregkh@linuxfoundation.org; benh@kernel.crashing.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; xuwei (O); Linuxarm;
> linux-serial@vger.kernel.org; linux-pci@vger.kernel.org;
> zourongrong@gmail.com; liviu.dudau@arm.com; kantyzc@163.com;
> zhichang.yuan02@gmail.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote:
> >
> > On 2016/9/14 20:33, Arnd Bergmann wrote:
> > > On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan
> wrote:
> > >
> > >> +Required properties:
> > >> +- compatible: should be "hisilicon,low-pin-count"
> > >> +- #address-cells: must be 2 which stick to the ISA/EISA binding
> doc.
> > >> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> > >> +- reg: base address and length of the register set for the
> device.
> > >> +- ranges: define a 1:1 mapping between the I/O space of the child
> device and
> > >> +	  the parent.
> > >
> > > Do we still need the "ranges" here? The property in your example
> seems
> > > wrong.
> >
> > I think "ranges" is needed.
> > without this, of_translate_address --> __of_translate_address -->
> of_translate_one will fail when translating the child's IO resource.
> >
> > >
> > >> +	ranges = <0x01 0xe4 0x0 0xe4 0x1000>;
> > >
> > > You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4?
> > The hip06 LPC is defined as isa type.
> > So, 0x01 0xe4 is the local IO address of 0xe4. With this ranges, 0xe4
> of child will be 1:1 mapped as 0xe4.
> > It means no translation.
> 
> No, "no translation" would be leaving out the ranges, we should
> fix the code so it handles this case according to the specification
> of the ISA DT binding, rather than adding an incorrect ranges
> property to make it work with the incorrect Linux implementation.
> 
> of_translate_address() should fail here, and whichever code calls
> it should try something else, possibly something we have to
> implement that can return the correct IORESOURCE_* type.

>From <<3.1.1. Open Firmware Properties for Bus Nodes>> in 
http://www.firmware.org/1275/bindings/isa/isa0_4d.ps

I quote:
"There shall be an entry in the "ranges" property for each
of the Memory and/or I/O spaces if that address space is
mapped through the bridge."

It seems to me that it is ok to have 1:1 address mapping and that
therefore of_translate_address() should fail if "ranges" is not
present.

This is also explained quite well in
http://lxr.free-electrons.com/source/drivers/of/address.c#L490

what do you think?

Thanks

Gab

> 
> > >
> > > I don't get this part. The bus driver should not care what its
> > > children are, just register and PIO ranges that the bus can handle
> > > in theory, i.e. from 0x000 to 0xfff.
> >
> > Just as we discussed in V2, the legacy PIO range is specific to some
> > device, such as for ipmi bt, 0xe4 - 0xe7 will be populated.
> > I don't want to occupy a larger PIO range in which only small part
> PIOs
> > are used by our LPC. At this moment, two PIO ranges are using
> > through the device property configuration, 0xe4-0xe7, 0x2f8-0x2ff.
> >
> > If we configure 0-0x1000 for the LPC to cover those two ranges, most
> > PIO are wasted and other PIO device on other buses lose the chance to
> > use the PIO below 0x1000.
> > Otherwise, PIO conflict will happen. So, My idea is only occupied
> > the PIO ranges which are really needed for the children.
> 
> The only thing it can realistically conflict with would be another
> LPC bus behind on a PCI host bridge. On ARM64, all regular PCI
> devices that have IORESOURCE_IO ports are intentionally moved
> to (bus) port numbers above 0x1000.
> 
> > And there are probably multiple child devices under LPC, the global
> arm64_extio_ops only can cover one PIO range. It is fortunate only ipmi
> driver can not support I/O
> > operation registering, serial driver has serial_in/serial_out to
> > be registered. So, only the PIO range for ipmi device is stored
> > in arm64_extio_ops and the indirect-IO
> > works well for ipmi device.
> 
> You should not do that in the serial driver, please just use the
> normal 8250 driver that works fine once you handle the entire
> port range.
> 
> 
> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote:
> 
> From <<3.1.1. Open Firmware Properties for Bus Nodes>> in 
> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> 
> I quote:
> "There shall be an entry in the "ranges" property for each
> of the Memory and/or I/O spaces if that address space is
> mapped through the bridge."
> 
> It seems to me that it is ok to have 1:1 address mapping and that
> therefore of_translate_address() should fail if "ranges" is not
> present.

The key here is the definition of "mapped through the bridge".
I can only understand this as "directly mapped", i.e. an I/O
port of the child bus corresponds directly to a memory address
on the parent bus, but this is not the case here.

The problem with adding the mapping here is that it looks
like it should be valid to create a page table entry for
the address returned from the translation and access it through
a pointer dereference, but that is clearly not possible.

> This is also explained quite well in
> http://lxr.free-electrons.com/source/drivers/of/address.c#L490
> 
> what do you think?

This is a separate issue, and only relevant for Apple Macintosh
machines as well as the PA-Semi sdc.

	Arnd

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 15 September 2016 09:22
> To: Gabriele Paoloni
> Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> gregkh@linuxfoundation.org; benh@kernel.crashing.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; xuwei (O); Linuxarm;
> linux-serial@vger.kernel.org; linux-pci@vger.kernel.org;
> zourongrong@gmail.com; liviu.dudau@arm.com; kantyzc@163.com;
> zhichang.yuan02@gmail.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote:
> >
> > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in
> > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> >
> > I quote:
> > "There shall be an entry in the "ranges" property for each
> > of the Memory and/or I/O spaces if that address space is
> > mapped through the bridge."
> >
> > It seems to me that it is ok to have 1:1 address mapping and that
> > therefore of_translate_address() should fail if "ranges" is not
> > present.
> 
> The key here is the definition of "mapped through the bridge".
> I can only understand this as "directly mapped", i.e. an I/O
> port of the child bus corresponds directly to a memory address
> on the parent bus, but this is not the case here.
> 
> The problem with adding the mapping here is that it looks
> like it should be valid to create a page table entry for
> the address returned from the translation and access it through
> a pointer dereference, but that is clearly not possible.

I understand that somehow we are abusing of the ranges property
here however the point is that with the current implementation ranges
is needed because otherwise the ipmi driver probe will fail here:

of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource
-> of_translate_address -> __of_translate_address

Now we had a bit of discussion internally and to avoid
having ranges we came up with two possible solutions:

1) Using bit 3 of phys.hi cell in 2.2.1 of
http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
This would mean reworking of_bus_isa_get_flags in 
http://lxr.free-electrons.com/source/drivers/of/address.c#L398
and setting a new flag to be checked in __of_address_to_resource

2) Adding a property in the bindings of each device that is
a child of our LPC bus and modify __of_address_to_resource
to check if the property is in the DT and eventually bypass
of_translate_address

However in both 1) and 2) there are some issues:
in 1) we are not complying with the isa binding doc (we use
a bit that should be zero); in 2) we need to modify the
bindings documentation of the devices that are connected
to our LPC controller (therefore modifying other devices
bindings to fit our special case).

I think that maybe having the 1:1 range mapping doesn't
reflect well the reality but it is the less painful
solution...

What's your view?
 
Many Thanks

Gab

> 
> > This is also explained quite well in
> > http://lxr.free-electrons.com/source/drivers/of/address.c#L490
> >
> > what do you think?
> 
> This is a separate issue, and only relevant for Apple Macintosh
> machines as well as the PA-Semi sdc.
> 
> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni wrote:
> > -----Original Message-----
> > On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote:
> > >
> > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in
> > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> > >
> > > I quote:
> > > "There shall be an entry in the "ranges" property for each
> > > of the Memory and/or I/O spaces if that address space is
> > > mapped through the bridge."
> > >
> > > It seems to me that it is ok to have 1:1 address mapping and that
> > > therefore of_translate_address() should fail if "ranges" is not
> > > present.
> > 
> > The key here is the definition of "mapped through the bridge".
> > I can only understand this as "directly mapped", i.e. an I/O
> > port of the child bus corresponds directly to a memory address
> > on the parent bus, but this is not the case here.
> > 
> > The problem with adding the mapping here is that it looks
> > like it should be valid to create a page table entry for
> > the address returned from the translation and access it through
> > a pointer dereference, but that is clearly not possible.
> 
> I understand that somehow we are abusing of the ranges property
> here however the point is that with the current implementation ranges
> is needed because otherwise the ipmi driver probe will fail here:
> 
> of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource
> -> of_translate_address -> __of_translate_address
> 
> Now we had a bit of discussion internally and to avoid
> having ranges we came up with two possible solutions:
> 
> 1) Using bit 3 of phys.hi cell in 2.2.1 of
> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> This would mean reworking of_bus_isa_get_flags in 
> http://lxr.free-electrons.com/source/drivers/of/address.c#L398
> and setting a new flag to be checked in __of_address_to_resource
> 
> 2) Adding a property in the bindings of each device that is
> a child of our LPC bus and modify __of_address_to_resource
> to check if the property is in the DT and eventually bypass
> of_translate_address
> 
> However in both 1) and 2) there are some issues:
> in 1) we are not complying with the isa binding doc (we use
> a bit that should be zero); in 2) we need to modify the
> bindings documentation of the devices that are connected
> to our LPC controller (therefore modifying other devices
> bindings to fit our special case).
> 
> I think that maybe having the 1:1 range mapping doesn't
> reflect well the reality but it is the less painful
> solution...
> 
> What's your view?

We can check the 'i' bit for I/O space in of_bus_isa_get_flags,
and that should be enough to translate the I/O port number.

The only part we need to change here is to not go through
the crazy conversion all the way from PCI I/O space to a
physical address and back to a (logical) port number
that we do today with of_translate_address/pci_address_to_pio.

I can think of a several of ways to fix __of_address_to_resource
to just do the right thing according to the ISA binding to
make the normal drivers work.

The easiest solution is probably to hook into the
"taddr == OF_BAD_ADDR" case in __of_address_to_resource
and add a lookup for ISA buses there, and instead check
if some special I/O port operations were registered
for the port number, using an architecture specific
function that arm64 implements. Other architectures
like x86 that don't have a direct mapping between I/O
ports and MMIO addresses would implement that same
function differently.

	Arnd

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 15 September 2016 13:25
> To: linux-arm-kernel@lists.infradead.org
> Cc: Gabriele Paoloni; devicetree@vger.kernel.org;
> lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org;
> gregkh@linuxfoundation.org; John Garry; will.deacon@arm.com; linux-
> kernel@vger.kernel.org; Yuanzhichang; Linuxarm; xuwei (O); linux-
> serial@vger.kernel.org; benh@kernel.crashing.org;
> zourongrong@gmail.com; liviu.dudau@arm.com; kantyzc@163.com;
> zhichang.yuan02@gmail.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni
> wrote:
> > > -----Original Message-----
> > > On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni
> wrote:
> > > >
> > > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in
> > > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> > > >
> > > > I quote:
> > > > "There shall be an entry in the "ranges" property for each
> > > > of the Memory and/or I/O spaces if that address space is
> > > > mapped through the bridge."
> > > >
> > > > It seems to me that it is ok to have 1:1 address mapping and that
> > > > therefore of_translate_address() should fail if "ranges" is not
> > > > present.
> > >
> > > The key here is the definition of "mapped through the bridge".
> > > I can only understand this as "directly mapped", i.e. an I/O
> > > port of the child bus corresponds directly to a memory address
> > > on the parent bus, but this is not the case here.
> > >
> > > The problem with adding the mapping here is that it looks
> > > like it should be valid to create a page table entry for
> > > the address returned from the translation and access it through
> > > a pointer dereference, but that is clearly not possible.
> >
> > I understand that somehow we are abusing of the ranges property
> > here however the point is that with the current implementation ranges
> > is needed because otherwise the ipmi driver probe will fail here:
> >
> > of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource
> > -> of_translate_address -> __of_translate_address
> >
> > Now we had a bit of discussion internally and to avoid
> > having ranges we came up with two possible solutions:
> >
> > 1) Using bit 3 of phys.hi cell in 2.2.1 of
> > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> > This would mean reworking of_bus_isa_get_flags in
> > http://lxr.free-electrons.com/source/drivers/of/address.c#L398
> > and setting a new flag to be checked in __of_address_to_resource
> >
> > 2) Adding a property in the bindings of each device that is
> > a child of our LPC bus and modify __of_address_to_resource
> > to check if the property is in the DT and eventually bypass
> > of_translate_address
> >
> > However in both 1) and 2) there are some issues:
> > in 1) we are not complying with the isa binding doc (we use
> > a bit that should be zero); in 2) we need to modify the
> > bindings documentation of the devices that are connected
> > to our LPC controller (therefore modifying other devices
> > bindings to fit our special case).
> >
> > I think that maybe having the 1:1 range mapping doesn't
> > reflect well the reality but it is the less painful
> > solution...
> >
> > What's your view?
> 
> We can check the 'i' bit for I/O space in of_bus_isa_get_flags,
> and that should be enough to translate the I/O port number.
> 
> The only part we need to change here is to not go through
> the crazy conversion all the way from PCI I/O space to a
> physical address and back to a (logical) port number
> that we do today with of_translate_address/pci_address_to_pio.
> 
> I can think of a several of ways to fix __of_address_to_resource
> to just do the right thing according to the ISA binding to
> make the normal drivers work.
> 
> The easiest solution is probably to hook into the
> "taddr == OF_BAD_ADDR" case in __of_address_to_resource
> and add a lookup for ISA buses there, and instead check
> if some special I/O port operations were registered
> for the port number, using an architecture specific
> function that arm64 implements. Other architectures
> like x86 that don't have a direct mapping between I/O
> ports and MMIO addresses would implement that same
> function differently.

So with respect to this patchset once we enter the
"taddr == OF_BAD_ADDR" case you would add an arch
specific function that checks if the resource is an I/O 
resource, if the parent node is an ISA bus (calling 
of_bus_isa_match) and if arm64_extio_ops is non NULL. 
 
I think it can work for us and it doesn't affect current
devices. I will talk to Zhichang about this for the next
patchset version.

Many Thanks for your ideas

Gab


> 
> 	Arnd

Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced

[zhichang]

Hi, Arnd,


On 2016年09月14日 22:23, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 10:16:28 PM CEST zhichang.yuan wrote:
>>>
>>> No need to guard includes with an #ifdef.
>> If remove #ifdef here, extio.h should not contain any function external declarations whose definitions are in
>> extio.c compiled only when CONFIG_ARM64_INDIRECT_PIO is yes.
>  
> There is no problem with making declarations visible for functions that
> are not part of the kernel, we do that all the time.
> 
>>>> +#define BUILDS_RW(bwl, type)                                                \
>>>> +static inline void reads##bwl(const volatile void __iomem *addr,    \
>>>> +                            void *buffer, unsigned int count)       \
>>>> +{                                                                   \
>>>> +    if (count) {                                                    \
>>>> +            type *buf = buffer;                                     \
>>>> +                                                                    \
>>>> +            do {                                                    \
>>>> +                    type x = __raw_read##bwl(addr);                 \
>>>> +                    *buf++ = x;                                     \
>>>> +            } while (--count);                                      \
>>>> +    }                                                               \
>>>> +}                                                                   \
>>>> +                                                                    \
>>>> +static inline void writes##bwl(volatile void __iomem *addr,         \
>>>> +                            const void *buffer, unsigned int count) \
>>>> +{                                                                   \
>>>> +    if (count) {                                                    \
>>>> +            const type *buf = buffer;                               \
>>>> +                                                                    \
>>>> +            do {                                                    \
>>>> +                    __raw_write##bwl(*buf++, addr);                 \
>>>> +            } while (--count);                                      \
>>>> +    }                                                               \
>>>> +}
>>>> +
>>>> +BUILDS_RW(b, u8)
>>>
>>> Why is this in here?
>> the readsb/writesb are defined in asm-generic/io.h which is included later, but the redefined insb/outsb need
>> to call them. Without these readsb/writesb definition before insb/outsb redefined, compile error occur.
>>
>> It seems that copy all the definitions of "asm-generic/io.h" is not a good idea, so I move the definitions of
>> those function needed here....
>>
>> Ok. I think your idea below defining in(s)/out(s) in a c file can solve this issue.
>>
>> #ifdef CONFIG_ARM64_INDIRECT_PIO
>> #define inb inb
>> extern u8 inb(unsigned long addr);
>>
>> #define outb outb
>> extern void outb(u8 value, unsigned long addr);
>>
>> #define insb insb
>> extern void insb(unsigned long addr, void *buffer, unsigned int count);
>>
>> #define outsb outsb
>> extern void outsb(unsigned long addr, const void *buffer, unsigned int count);
>> #endif
>>
>> and definitions of all these functions are in extio.c :
>>
>> u8 inb(unsigned long addr)
>> {
>>         if (!arm64_extio_ops || arm64_extio_ops->start > addr ||
>>                         arm64_extio_ops->end < addr)
>>                 return readb(PCI_IOBASE + addr);
>>         else
>>                 return arm64_extio_ops->pfin ?
>>                         arm64_extio_ops->pfin(arm64_extio_ops->devpara,
>>                                 addr + arm64_extio_ops->ptoffset, NULL,
>>                                 sizeof(u8), 1) : -1;
>> }
>> .....
> 
> Yes, sounds good.
> 
>>>> @@ -149,6 +185,60 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>>>>  #define IO_SPACE_LIMIT              (PCI_IO_SIZE - 1)
>>>>  #define PCI_IOBASE          ((void __iomem *)PCI_IO_START)
>>>>  
>>>> +
>>>> +/*
>>>> + * redefine the in(s)b/out(s)b for indirect-IO.
>>>> + */
>>>> +#define inb inb
>>>> +static inline u8 inb(unsigned long addr)
>>>> +{
>>>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>>>> +    if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
>>>> +                    addr <= arm64_extio_ops->end)
>>>> +            return extio_inb(addr);
>>>> +#endif
>>>> +    return readb(PCI_IOBASE + addr);
>>>> +}
>>>> +
>>>
>>> Looks ok, but you only seem to do this for the 8-bit
>>> accessors, when it should be done for 16-bit and 32-bit
>>> ones as well for consistency.
>> Hip06 LPC only support 8-bit I/O operations on the designated port.
> 
> That is an interesting limitation. Maybe still call the extio operations
> and have them do WARN_ON_ONCE() instead?
> 
> If you get a driver that calls inw/outw on the range that is owned
> by the LPC bus, you otherwise get an unhandled page fault in kernel
> space, which is not as nice.
> 
Yes. It probably cause kernel panic.
Will define the extio operations for other IO length and add the corresponding WARNINGS.

Best,
Zhichang


>>>> diff --git a/drivers/bus/extio.c b/drivers/bus/extio.c
>>>> new file mode 100644
>>>> index 0000000..1e7a9c5
>>>> --- /dev/null
>>>> +++ b/drivers/bus/extio.c
>>>> @@ -0,0 +1,66 @@
>>>
>>> This is in a globally visible directory
>>>
>>>> +
>>>> +struct extio_ops *arm64_extio_ops;
>>>
>>> But the identifier uses an architecture specific prefix. Either
>>> move the whole file into arch/arm64, or make the naming so that
>>> it can be used for everything.
>>
>> I perfer to move the whole file into arch/arm64, extio.h will be moved to arch/arm64/include/asm;
> 
> Ok, that simplifies it a lot, you can just do everything in asm/io.h then.
> 
> 	Arnd
> 

Re: [PATCH V3 1/4] ARM64 LPC: Indirect ISA port IO introduced

[zhichang]

Hi, Arnd,


On 2016年09月14日 22:23, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 10:16:28 PM CEST zhichang.yuan wrote:
>>>
>>> No need to guard includes with an #ifdef.
>> If remove #ifdef here, extio.h should not contain any function external declarations whose definitions are in
>> extio.c compiled only when CONFIG_ARM64_INDIRECT_PIO is yes.
>  
> There is no problem with making declarations visible for functions that
> are not part of the kernel, we do that all the time.
> 
>>>> +#define BUILDS_RW(bwl, type)                                                \
>>>> +static inline void reads##bwl(const volatile void __iomem *addr,    \
>>>> +                            void *buffer, unsigned int count)       \
>>>> +{                                                                   \
>>>> +    if (count) {                                                    \
>>>> +            type *buf = buffer;                                     \
>>>> +                                                                    \
>>>> +            do {                                                    \
>>>> +                    type x = __raw_read##bwl(addr);                 \
>>>> +                    *buf++ = x;                                     \
>>>> +            } while (--count);                                      \
>>>> +    }                                                               \
>>>> +}                                                                   \
>>>> +                                                                    \
>>>> +static inline void writes##bwl(volatile void __iomem *addr,         \
>>>> +                            const void *buffer, unsigned int count) \
>>>> +{                                                                   \
>>>> +    if (count) {                                                    \
>>>> +            const type *buf = buffer;                               \
>>>> +                                                                    \
>>>> +            do {                                                    \
>>>> +                    __raw_write##bwl(*buf++, addr);                 \
>>>> +            } while (--count);                                      \
>>>> +    }                                                               \
>>>> +}
>>>> +
>>>> +BUILDS_RW(b, u8)
>>>
>>> Why is this in here?
>> the readsb/writesb are defined in asm-generic/io.h which is included later, but the redefined insb/outsb need
>> to call them. Without these readsb/writesb definition before insb/outsb redefined, compile error occur.
>>
>> It seems that copy all the definitions of "asm-generic/io.h" is not a good idea, so I move the definitions of
>> those function needed here....
>>
>> Ok. I think your idea below defining in(s)/out(s) in a c file can solve this issue.
>>
>> #ifdef CONFIG_ARM64_INDIRECT_PIO
>> #define inb inb
>> extern u8 inb(unsigned long addr);
>>
>> #define outb outb
>> extern void outb(u8 value, unsigned long addr);
>>
>> #define insb insb
>> extern void insb(unsigned long addr, void *buffer, unsigned int count);
>>
>> #define outsb outsb
>> extern void outsb(unsigned long addr, const void *buffer, unsigned int count);
>> #endif
>>
>> and definitions of all these functions are in extio.c :
>>
>> u8 inb(unsigned long addr)
>> {
>>         if (!arm64_extio_ops || arm64_extio_ops->start > addr ||
>>                         arm64_extio_ops->end < addr)
>>                 return readb(PCI_IOBASE + addr);
>>         else
>>                 return arm64_extio_ops->pfin ?
>>                         arm64_extio_ops->pfin(arm64_extio_ops->devpara,
>>                                 addr + arm64_extio_ops->ptoffset, NULL,
>>                                 sizeof(u8), 1) : -1;
>> }
>> .....
> 
> Yes, sounds good.
> 
>>>> @@ -149,6 +185,60 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>>>>  #define IO_SPACE_LIMIT              (PCI_IO_SIZE - 1)
>>>>  #define PCI_IOBASE          ((void __iomem *)PCI_IO_START)
>>>>  
>>>> +
>>>> +/*
>>>> + * redefine the in(s)b/out(s)b for indirect-IO.
>>>> + */
>>>> +#define inb inb
>>>> +static inline u8 inb(unsigned long addr)
>>>> +{
>>>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>>>> +    if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
>>>> +                    addr <= arm64_extio_ops->end)
>>>> +            return extio_inb(addr);
>>>> +#endif
>>>> +    return readb(PCI_IOBASE + addr);
>>>> +}
>>>> +
>>>
>>> Looks ok, but you only seem to do this for the 8-bit
>>> accessors, when it should be done for 16-bit and 32-bit
>>> ones as well for consistency.
>> Hip06 LPC only support 8-bit I/O operations on the designated port.
> 
> That is an interesting limitation. Maybe still call the extio operations
> and have them do WARN_ON_ONCE() instead?
> 
> If you get a driver that calls inw/outw on the range that is owned
> by the LPC bus, you otherwise get an unhandled page fault in kernel
> space, which is not as nice.

As for this issue, I provided a wrong reply in the last email.
After double-checking with SoC guys, the inw(l)/outw(l) are OK with multiple 8-bit transfers to consecutive
I/O addresses.

Sorry for the wrong information!
Will support inw(l)/outw(l) in V4.

Best,
Zhichang


> 
>>>> diff --git a/drivers/bus/extio.c b/drivers/bus/extio.c
>>>> new file mode 100644
>>>> index 0000000..1e7a9c5
>>>> --- /dev/null
>>>> +++ b/drivers/bus/extio.c
>>>> @@ -0,0 +1,66 @@
>>>
>>> This is in a globally visible directory
>>>
>>>> +
>>>> +struct extio_ops *arm64_extio_ops;
>>>
>>> But the identifier uses an architecture specific prefix. Either
>>> move the whole file into arch/arm64, or make the naming so that
>>> it can be used for everything.
>>
>> I perfer to move the whole file into arch/arm64, extio.h will be moved to arch/arm64/include/asm;
> 
> Ok, that simplifies it a lot, you can just do everything in asm/io.h then.
> 
> 	Arnd
> 

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[zhichang]

Hi, Arnd,



On 2016年09月15日 20:24, Arnd Bergmann wrote:
> On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni wrote:
>>> -----Original Message-----
>>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote:
>>>>
>>>> From <<3.1.1. Open Firmware Properties for Bus Nodes>> in
>>>> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
>>>>
>>>> I quote:
>>>> "There shall be an entry in the "ranges" property for each
>>>> of the Memory and/or I/O spaces if that address space is
>>>> mapped through the bridge."
>>>>
>>>> It seems to me that it is ok to have 1:1 address mapping and that
>>>> therefore of_translate_address() should fail if "ranges" is not
>>>> present.
>>>
>>> The key here is the definition of "mapped through the bridge".
>>> I can only understand this as "directly mapped", i.e. an I/O
>>> port of the child bus corresponds directly to a memory address
>>> on the parent bus, but this is not the case here.
>>>
>>> The problem with adding the mapping here is that it looks
>>> like it should be valid to create a page table entry for
>>> the address returned from the translation and access it through
>>> a pointer dereference, but that is clearly not possible.
>>
>> I understand that somehow we are abusing of the ranges property
>> here however the point is that with the current implementation ranges
>> is needed because otherwise the ipmi driver probe will fail here:
>>
>> of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource
>> -> of_translate_address -> __of_translate_address
>>
>> Now we had a bit of discussion internally and to avoid
>> having ranges we came up with two possible solutions:
>>
>> 1) Using bit 3 of phys.hi cell in 2.2.1 of
>> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
>> This would mean reworking of_bus_isa_get_flags in 
>> http://lxr.free-electrons.com/source/drivers/of/address.c#L398
>> and setting a new flag to be checked in __of_address_to_resource
>>
>> 2) Adding a property in the bindings of each device that is
>> a child of our LPC bus and modify __of_address_to_resource
>> to check if the property is in the DT and eventually bypass
>> of_translate_address
>>
>> However in both 1) and 2) there are some issues:
>> in 1) we are not complying with the isa binding doc (we use
>> a bit that should be zero); in 2) we need to modify the
>> bindings documentation of the devices that are connected
>> to our LPC controller (therefore modifying other devices
>> bindings to fit our special case).
>>
>> I think that maybe having the 1:1 range mapping doesn't
>> reflect well the reality but it is the less painful
>> solution...
>>
>> What's your view?
> 
> We can check the 'i' bit for I/O space in of_bus_isa_get_flags,
> and that should be enough to translate the I/O port number.
> 
> The only part we need to change here is to not go through
> the crazy conversion all the way from PCI I/O space to a
> physical address and back to a (logical) port number
> that we do today with of_translate_address/pci_address_to_pio.
> 
Sorry for the late response! Several days' leave....
Do you want to bypass of_translate_address and pci_address_to_pio for the registered specific PIO?
I think the bypass for of_translate_address is ok, but worry some new issues will emerge without the
conversion between physical address and logical/linux port number.

When PCI host bridge which support IO operations is configured and enabled, the pci_address_to_pio will
populate the logical IO range from ZERO for the first host bridge. Our LPC will also use part of the IO range
started from ZERO. It will make in/out enter the wrong branch possibly.

In V2, the 0 - 0x1000 logical IO range is reserved for LPC use only. But it seems not so good. In this way,
PCI has no chance to use low 4K IO range(logical).

So, in V3, applying the conversion from physical/cpu address to logical/linux IO port for any IO ranges,
including the LPC, but recorded the logical IO range for LPC. When calling in/out with a logical port address,
we can check this port fall into LPC logical IO range and get back the real IO.

Do you have further comments about this??


> I can think of a several of ways to fix __of_address_to_resource
> to just do the right thing according to the ISA binding to
> make the normal drivers work.
> 
> The easiest solution is probably to hook into the
> "taddr == OF_BAD_ADDR" case in __of_address_to_resource
> and add a lookup for ISA buses there, and instead check
> if some special I/O port operations were registered
> for the port number, using an architecture specific
> function that arm64 implements. Other architectures
> like x86 that don't have a direct mapping between I/O
> ports and MMIO addresses would implement that same
> function differently.

What about add the specific quirk for Hip06 LPC in of_empty_ranges_quirk()??

you know, there are several cases in which of_translate_address return OF_BAD_ADDR.
And if we only check the special port range, it seems a bit risky. If some device want to use this port range
when no hip06 LPC is configured, the checking does not work. I think we should also check the relevant device.


Best,
Zhichang


> 
> 	Arnd
> 

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

Hi Zhichang

> -----Original Message-----
> From: zhichang [mailto:zhichang.yuan02@gmail.com]
> Sent: 21 September 2016 11:09
> To: Arnd Bergmann; linux-arm-kernel@lists.infradead.org
> Cc: Gabriele Paoloni; devicetree@vger.kernel.org;
> lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org;
> gregkh@linuxfoundation.org; John Garry; will.deacon@arm.com; linux-
> kernel@vger.kernel.org; Yuanzhichang; Linuxarm; xuwei (O); linux-
> serial@vger.kernel.org; benh@kernel.crashing.org;
> zourongrong@gmail.com; liviu.dudau@arm.com; kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> Hi, Arnd,
> 
> 
> 
> On 2016年09月15日 20:24, Arnd Bergmann wrote:
> > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni
> wrote:
> >>> -----Original Message-----
> >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni
> wrote:
> >>>>
> >>>> From <<3.1.1. Open Firmware Properties for Bus Nodes>> in
> >>>> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> >>>>
> >>>> I quote:
> >>>> "There shall be an entry in the "ranges" property for each
> >>>> of the Memory and/or I/O spaces if that address space is
> >>>> mapped through the bridge."
> >>>>
> >>>> It seems to me that it is ok to have 1:1 address mapping and that
> >>>> therefore of_translate_address() should fail if "ranges" is not
> >>>> present.
> >>>
> >>> The key here is the definition of "mapped through the bridge".
> >>> I can only understand this as "directly mapped", i.e. an I/O
> >>> port of the child bus corresponds directly to a memory address
> >>> on the parent bus, but this is not the case here.
> >>>
> >>> The problem with adding the mapping here is that it looks
> >>> like it should be valid to create a page table entry for
> >>> the address returned from the translation and access it through
> >>> a pointer dereference, but that is clearly not possible.
> >>
> >> I understand that somehow we are abusing of the ranges property
> >> here however the point is that with the current implementation
> ranges
> >> is needed because otherwise the ipmi driver probe will fail here:
> >>
> >> of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource
> >> -> of_translate_address -> __of_translate_address
> >>
> >> Now we had a bit of discussion internally and to avoid
> >> having ranges we came up with two possible solutions:
> >>
> >> 1) Using bit 3 of phys.hi cell in 2.2.1 of
> >> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
> >> This would mean reworking of_bus_isa_get_flags in
> >> http://lxr.free-electrons.com/source/drivers/of/address.c#L398
> >> and setting a new flag to be checked in __of_address_to_resource
> >>
> >> 2) Adding a property in the bindings of each device that is
> >> a child of our LPC bus and modify __of_address_to_resource
> >> to check if the property is in the DT and eventually bypass
> >> of_translate_address
> >>
> >> However in both 1) and 2) there are some issues:
> >> in 1) we are not complying with the isa binding doc (we use
> >> a bit that should be zero); in 2) we need to modify the
> >> bindings documentation of the devices that are connected
> >> to our LPC controller (therefore modifying other devices
> >> bindings to fit our special case).
> >>
> >> I think that maybe having the 1:1 range mapping doesn't
> >> reflect well the reality but it is the less painful
> >> solution...
> >>
> >> What's your view?
> >
> > We can check the 'i' bit for I/O space in of_bus_isa_get_flags,
> > and that should be enough to translate the I/O port number.
> >
> > The only part we need to change here is to not go through
> > the crazy conversion all the way from PCI I/O space to a
> > physical address and back to a (logical) port number
> > that we do today with of_translate_address/pci_address_to_pio.
> >
> Sorry for the late response! Several days' leave....
> Do you want to bypass of_translate_address and pci_address_to_pio for
> the registered specific PIO?
> I think the bypass for of_translate_address is ok, but worry some new
> issues will emerge without the
> conversion between physical address and logical/linux port number.
> 
> When PCI host bridge which support IO operations is configured and
> enabled, the pci_address_to_pio will
> populate the logical IO range from ZERO for the first host bridge. Our
> LPC will also use part of the IO range
> started from ZERO. It will make in/out enter the wrong branch possibly.
> 
> In V2, the 0 - 0x1000 logical IO range is reserved for LPC use only.
> But it seems not so good. In this way,
> PCI has no chance to use low 4K IO range(logical).
> 
> So, in V3, applying the conversion from physical/cpu address to
> logical/linux IO port for any IO ranges,
> including the LPC, but recorded the logical IO range for LPC. When
> calling in/out with a logical port address,
> we can check this port fall into LPC logical IO range and get back the
> real IO.
> 
> Do you have further comments about this??

I think there are two separate issues to be discussed:

The first issue is about having of_translate_address failing due to
"range" missing. About this Arnd suggested that it is not appropriate
to have a range describing a bridge 1:1 mapping and this was discussed
before in this thread. Arnd had a suggestion about this (see below) 
however (looking twice at the code) it seems to me that such solution 
would lead to quite some duplication from __of_translate_address()
in order to retrieve the actual addr from dt...

I think extending of_empty_ranges_quirk() may be a reasonable solution.
What do you think Arnd?
  
The second issue is a conflict between cpu addresses used by the LPC
controller and i/o tokens from pci endpoints.

About this what if we modify armn64_extio_ops to have a list of ranges
rather than only one range (now we have just start/end); then in the
LPC driver we can scan the LPC child devices and 
1) populate such list of ranges
2) call pci_register_io_range for such ranges

Then when calling __of_address_to_resource we retrieve I/O tokens 
for the devices on top of the LPC driver and in the I/O accessors
we call pci_pio_to_address to figure out the cpu address and compare
it to the list of ranges in armn64_extio_ops.
  
What about this?

Thanks

Gab

> 
> 
> > I can think of a several of ways to fix __of_address_to_resource
> > to just do the right thing according to the ISA binding to
> > make the normal drivers work.
> >
> > The easiest solution is probably to hook into the
> > "taddr == OF_BAD_ADDR" case in __of_address_to_resource
> > and add a lookup for ISA buses there, and instead check
> > if some special I/O port operations were registered
> > for the port number, using an architecture specific
> > function that arm64 implements. Other architectures
> > like x86 that don't have a direct mapping between I/O
> > ports and MMIO addresses would implement that same
> > function differently.
> 
> What about add the specific quirk for Hip06 LPC in
> of_empty_ranges_quirk()??
> 
> you know, there are several cases in which of_translate_address return
> OF_BAD_ADDR.
> And if we only check the special port range, it seems a bit risky. If
> some device want to use this port range
> when no hip06 LPC is configured, the checking does not work. I think we
> should also check the relevant device.
> 
> 
> Best,
> Zhichang
> 
> 
> >
> > 	Arnd
> >

Re: [PATCH V3 3/4] ARM64 LPC: support serial based on low-pin-count

[Arnd Bergmann]

On Wednesday, September 21, 2016 6:12:28 PM CEST zhichang wrote:
> On 2016年09月15日 05:33, Arnd Bergmann wrote:
> > On Wednesday, September 14, 2016 11:04:33 PM CEST zhichang.yuan wrote:
> >> The 8250_hisi_lpc.c support both ACPI and dts similar to 8250_dw :
> >>
> >> +static struct platform_driver hs_lpc8250_driver = {
> >> +       .driver = {
> >> +               .name           = "hisi-lpc-uart",
> >> +               .of_match_table = hs8250_of_match,
> >> +               .acpi_match_table = ACPI_PTR(hs8250_acpi_match),
> >>
> >> So, I am a little confused why we need to support dts in 8250_of.c and support ACPI in another
> >> driver file.
> >>
> > 
> > After looking again, I'm pretty sure that drivers/tty/serial/8250/8250_pnp.c
> > will handle the ACPI case without modifications, you just need to adapt
> > the 8250_of driver to handle IORESOURCE_IO ports. This will be required
> > anyway with the next version once the LPC bridge is on the PCI bus.
> > 
> 
> Yes. You are right.
> 
> When the indirectIO patch is accepted, no much changes needed in these two file to support LPC UART.
> 
> BTW, what is your target to change these two files to support LPC uart?
> Do you think we don't need to add many new c files??

I think by modifying 8250_of.c, you should be able to make any UART on
some LPC bus work, regardless of what controller is used.

The ACPI version (8250_pnp.c) shouldn't need any changes at all if I
read that correctly.

	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Wednesday, September 21, 2016 4:20:55 PM CEST Gabriele Paoloni wrote:
> > -----Original Message-----
> > From: zhichang [mailto:zhichang.yuan02@gmail.com]
> > On 2016年09月15日 20:24, Arnd Bergmann wrote:
> > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni
> > wrote:
> > >>> -----Original Message-----
> > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni
> > wrote:
> > >> I think that maybe having the 1:1 range mapping doesn't
> > >> reflect well the reality but it is the less painful
> > >> solution...
> > >>
> > >> What's your view?
> > >
> > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags,
> > > and that should be enough to translate the I/O port number.
> > >
> > > The only part we need to change here is to not go through
> > > the crazy conversion all the way from PCI I/O space to a
> > > physical address and back to a (logical) port number
> > > that we do today with of_translate_address/pci_address_to_pio.
> > >
> > Sorry for the late response! Several days' leave....
> > Do you want to bypass of_translate_address and pci_address_to_pio for
> > the registered specific PIO?
> > I think the bypass for of_translate_address is ok, but worry some new
> > issues will emerge without the
> > conversion between physical address and logical/linux port number.

The same function that handles the non-translated region would
do that conversion.

> > When PCI host bridge which support IO operations is configured and
> > enabled, the pci_address_to_pio will
> > populate the logical IO range from ZERO for the first host bridge. Our
> > LPC will also use part of the IO range
> > started from ZERO. It will make in/out enter the wrong branch possibly.
> > 
> > In V2, the 0 - 0x1000 logical IO range is reserved for LPC use only.
> > But it seems not so good. In this way,
> > PCI has no chance to use low 4K IO range(logical).
> > 
> > So, in V3, applying the conversion from physical/cpu address to
> > logical/linux IO port for any IO ranges,
> > including the LPC, but recorded the logical IO range for LPC. When
> > calling in/out with a logical port address,
> > we can check this port fall into LPC logical IO range and get back the
> > real IO.

Right, and the same translation can be used in __of_address_to_resource()
going the opposite way.

> > Do you have further comments about this??
> 
> I think there are two separate issues to be discussed:
> 
> The first issue is about having of_translate_address failing due to
> "range" missing. About this Arnd suggested that it is not appropriate
> to have a range describing a bridge 1:1 mapping and this was discussed
> before in this thread. Arnd had a suggestion about this (see below) 
> however (looking twice at the code) it seems to me that such solution 
> would lead to quite some duplication from __of_translate_address()
> in order to retrieve the actual addr from dt...

I don't think we need to duplicate much, we can probably safely
assume that there are no nontrivial ranges in devices below the LPC
node, so we just walk up the bus to see if the node is a child
(or possibly grandchild etc) of the LPC bus, and treat any IO port
number under there as a physical port number, which has a known
offset from the Linux I/O port number.

> I think extending of_empty_ranges_quirk() may be a reasonable solution.
> What do you think Arnd?

I don't really like that idea, that quirk is meant to work around
broken DTs, but we can just make the DT valid and implement the
code properly.

> The second issue is a conflict between cpu addresses used by the LPC
> controller and i/o tokens from pci endpoints.
> 
> About this what if we modify armn64_extio_ops to have a list of ranges
> rather than only one range (now we have just start/end); then in the
> LPC driver we can scan the LPC child devices and 
> 1) populate such list of ranges
> 2) call pci_register_io_range for such ranges

Scanning the child devices sounds really wrong, please register just
one range that covers the bus to keep the workaround as simple
as possible.

> Then when calling __of_address_to_resource we retrieve I/O tokens 
> for the devices on top of the LPC driver and in the I/O accessors
> we call pci_pio_to_address to figure out the cpu address and compare
> it to the list of ranges in armn64_extio_ops.
>   
> What about this?

That seems really complex for something that can be quite simple.
The only thing we need to worry about is that the io_range_list
contains an entry for the LPC bus so we don't conflict with the
PCI buses.

	Arnd

	Arnd

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 21 September 2016 21:18
> To: Gabriele Paoloni
> Cc: zhichang; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Wednesday, September 21, 2016 4:20:55 PM CEST Gabriele Paoloni
> wrote:
> > > -----Original Message-----
> > > From: zhichang [mailto:zhichang.yuan02@gmail.com]
> > > On 2016年09月15日 20:24, Arnd Bergmann wrote:
> > > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni
> > > wrote:
> > > >>> -----Original Message-----
> > > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele
> Paoloni
> > > wrote:
> > > >> I think that maybe having the 1:1 range mapping doesn't
> > > >> reflect well the reality but it is the less painful
> > > >> solution...
> > > >>
> > > >> What's your view?
> > > >
> > > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags,
> > > > and that should be enough to translate the I/O port number.
> > > >
> > > > The only part we need to change here is to not go through
> > > > the crazy conversion all the way from PCI I/O space to a
> > > > physical address and back to a (logical) port number
> > > > that we do today with of_translate_address/pci_address_to_pio.
> > > >
> > > Sorry for the late response! Several days' leave....
> > > Do you want to bypass of_translate_address and pci_address_to_pio
> for
> > > the registered specific PIO?
> > > I think the bypass for of_translate_address is ok, but worry some
> new
> > > issues will emerge without the
> > > conversion between physical address and logical/linux port number.
> 
> The same function that handles the non-translated region would
> do that conversion.
> 
> > > When PCI host bridge which support IO operations is configured and
> > > enabled, the pci_address_to_pio will
> > > populate the logical IO range from ZERO for the first host bridge.
> Our
> > > LPC will also use part of the IO range
> > > started from ZERO. It will make in/out enter the wrong branch
> possibly.
> > >
> > > In V2, the 0 - 0x1000 logical IO range is reserved for LPC use
> only.
> > > But it seems not so good. In this way,
> > > PCI has no chance to use low 4K IO range(logical).
> > >
> > > So, in V3, applying the conversion from physical/cpu address to
> > > logical/linux IO port for any IO ranges,
> > > including the LPC, but recorded the logical IO range for LPC. When
> > > calling in/out with a logical port address,
> > > we can check this port fall into LPC logical IO range and get back
> the
> > > real IO.
> 
> Right, and the same translation can be used in
> __of_address_to_resource()
> going the opposite way.
> 
> > > Do you have further comments about this??
> >
> > I think there are two separate issues to be discussed:
> >
> > The first issue is about having of_translate_address failing due to
> > "range" missing. About this Arnd suggested that it is not appropriate
> > to have a range describing a bridge 1:1 mapping and this was
> discussed
> > before in this thread. Arnd had a suggestion about this (see below)
> > however (looking twice at the code) it seems to me that such solution
> > would lead to quite some duplication from __of_translate_address()
> > in order to retrieve the actual addr from dt...
> 
> I don't think we need to duplicate much, we can probably safely
> assume that there are no nontrivial ranges in devices below the LPC
> node, so we just walk up the bus to see if the node is a child
> (or possibly grandchild etc) of the LPC bus, and treat any IO port
> number under there as a physical port number, which has a known
> offset from the Linux I/O port number.
> 
> > I think extending of_empty_ranges_quirk() may be a reasonable
> solution.
> > What do you think Arnd?
> 
> I don't really like that idea, that quirk is meant to work around
> broken DTs, but we can just make the DT valid and implement the
> code properly.

Ok  I understand your point where it is not right to use of_empty_ranges_quirk()
As a quirk is used to work around broken HW or broken FW (as in this case)
rather than to fix code

What about the following? I think adding the check you suggested next to
of_empty_ranges_quirk() is adding the case we need in the right point (thus
avoiding any duplication)
 
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np)
        return NULL;
 }
 
+static inline int of_isa_indirect_io(struct device_node *np)
+{
+       /*
+        * check if the current node is an isa bus and if indirectio operation
+        * are registered
+        */
+       return (of_bus_isa_match(np) && arm64_extio_ops);
+}
+
 static int of_empty_ranges_quirk(struct device_node *np)
 {
        if (IS_ENABLED(CONFIG_PPC)) {
@@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
         * This code is only enabled on powerpc. --gcl
         */
        ranges = of_get_property(parent, rprop, &rlen);
-       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
+       if (ranges == NULL && !of_empty_ranges_quirk(parent) && !of_isa_indirect_io(parent)) {
                pr_debug("OF: no ranges; cannot translate\n");
                return 1;
        }


> 
> > The second issue is a conflict between cpu addresses used by the LPC
> > controller and i/o tokens from pci endpoints.
> >
> > About this what if we modify armn64_extio_ops to have a list of
> ranges
> > rather than only one range (now we have just start/end); then in the
> > LPC driver we can scan the LPC child devices and
> > 1) populate such list of ranges
> > 2) call pci_register_io_range for such ranges
> 
> Scanning the child devices sounds really wrong, please register just
> one range that covers the bus to keep the workaround as simple
> as possible.
> 
> > Then when calling __of_address_to_resource we retrieve I/O tokens
> > for the devices on top of the LPC driver and in the I/O accessors
> > we call pci_pio_to_address to figure out the cpu address and compare
> > it to the list of ranges in armn64_extio_ops.
> >
> > What about this?
> 
> That seems really complex for something that can be quite simple.
> The only thing we need to worry about is that the io_range_list
> contains an entry for the LPC bus so we don't conflict with the
> PCI buses.

Thanks

I discussed with Zhichang and we agreed to use only one LPC range
to be registered with pci_register_io_range.

We'll rework the accessors to check if the retrieved I/O tokens
belong to LPC or PCI IO range...

Cheers

Gab


> 
> 	Arnd
> 
> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni wrote:
> > > I think extending of_empty_ranges_quirk() may be a reasonable
> > solution.
> > > What do you think Arnd?
> > 
> > I don't really like that idea, that quirk is meant to work around
> > broken DTs, but we can just make the DT valid and implement the
> > code properly.
> 
> Ok  I understand your point where it is not right to use of_empty_ranges_quirk()
> As a quirk is used to work around broken HW or broken FW (as in this case)
> rather than to fix code
> 
> What about the following? I think adding the check you suggested next to
> of_empty_ranges_quirk() is adding the case we need in the right point (thus
> avoiding any duplication)
>  
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np)
>         return NULL;
>  }
>  
> +static inline int of_isa_indirect_io(struct device_node *np)
> +{
> +       /*
> +        * check if the current node is an isa bus and if indirectio operation
> +        * are registered
> +        */
> +       return (of_bus_isa_match(np) && arm64_extio_ops);
> +}
> +
>  static int of_empty_ranges_quirk(struct device_node *np)
>  {
>         if (IS_ENABLED(CONFIG_PPC)) {
> @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>          * This code is only enabled on powerpc. --gcl
>          */
>         ranges = of_get_property(parent, rprop, &rlen);
> -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> +       if (ranges == NULL && !of_empty_ranges_quirk(parent) && !of_isa_indirect_io(parent)) {
>                 pr_debug("OF: no ranges; cannot translate\n");
>                 return 1;
>         }

I don't see what effect that would have. What do you want to
achieve with this?

I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..a18d96843fae 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node *dev,
 	if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
 		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 (taddr == OF_BAD_ADDR)
+			port = arch_of_address_to_pio(dev, addrp)
+		else
+			port = pci_address_to_pio(taddr);
+
 		if (port == (unsigned long)-1)
 			return -EINVAL;
 		r->start = port;
 		r->end = port + size - 1;
 	} else {
+		if (taddr == OF_BAD_ADDR)
+			return -EINVAL;
+
 		r->start = taddr;
 		r->end = taddr + size - 1;
 	}



	Arnd

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 22 September 2016 13:15
> To: Gabriele Paoloni
> Cc: zhichang; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni
> wrote:
> > > > I think extending of_empty_ranges_quirk() may be a reasonable
> > > solution.
> > > > What do you think Arnd?
> > >
> > > I don't really like that idea, that quirk is meant to work around
> > > broken DTs, but we can just make the DT valid and implement the
> > > code properly.
> >
> > Ok  I understand your point where it is not right to use
> of_empty_ranges_quirk()
> > As a quirk is used to work around broken HW or broken FW (as in this
> case)
> > rather than to fix code
> >
> > What about the following? I think adding the check you suggested next
> to
> > of_empty_ranges_quirk() is adding the case we need in the right point
> (thus
> > avoiding any duplication)
> >
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct
> device_node *np)
> >         return NULL;
> >  }
> >
> > +static inline int of_isa_indirect_io(struct device_node *np)
> > +{
> > +       /*
> > +        * check if the current node is an isa bus and if indirectio
> operation
> > +        * are registered
> > +        */
> > +       return (of_bus_isa_match(np) && arm64_extio_ops);
> > +}
> > +
> >  static int of_empty_ranges_quirk(struct device_node *np)
> >  {
> >         if (IS_ENABLED(CONFIG_PPC)) {
> > @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
> >          * This code is only enabled on powerpc. --gcl
> >          */
> >         ranges = of_get_property(parent, rprop, &rlen);
> > -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> > +       if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> !of_isa_indirect_io(parent)) {
> >                 pr_debug("OF: no ranges; cannot translate\n");
> >                 return 1;
> >         }
> 
> I don't see what effect that would have. What do you want to
> achieve with this?

If I read the code correctly adding the function above would end
up in a 1:1 mapping:
http://lxr.free-electrons.com/source/drivers/of/address.c#L513

so taddr will be assigned with the cpu address space specified
in the children nodes of LPC and we are not using a quirk function
(we are just checking that we have the indirect io assigned and
that we are on a ISA bus). Now probably there is a nit in my 
code sketch where of_isa_indirect_io should be probably an architecture
specific function...

> 
> I think all we need from this function is to return '1' if
> we hit an ISA I/O window, and that should happen for the two
> interesting cases, either no 'ranges' at all, or no translation
> for the range in question, so that __of_translate_address can
> return OF_BAD_ADDR, and we can enter the special case
> handling in the caller, that handles it like
> 

I don't think this is very right as you may fail for different
reasons other than a missing range property, e.g:
http://lxr.free-electrons.com/source/drivers/of/address.c#L575

And even if the only failure case was a missing range if in the
future __of_translate_address had to be reworked we would again
make a wrong assumption...you get my point?

Thanks

Gab

> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 02b2903fe9d2..a18d96843fae 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -685,17 +685,24 @@ static int __of_address_to_resource(struct
> device_node *dev,
>  	if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>  		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 (taddr == OF_BAD_ADDR)
> +			port = arch_of_address_to_pio(dev, addrp)
> +		else
> +			port = pci_address_to_pio(taddr);
> +
>  		if (port == (unsigned long)-1)
>  			return -EINVAL;
>  		r->start = port;
>  		r->end = port + size - 1;
>  	} else {
> +		if (taddr == OF_BAD_ADDR)
> +			return -EINVAL;
> +
>  		r->start = taddr;
>  		r->end = taddr + size - 1;
>  	}
> 
> 
> 
> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
> > >  static int of_empty_ranges_quirk(struct device_node *np)
> > >  {
> > >         if (IS_ENABLED(CONFIG_PPC)) {
> > > @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node
> > *parent, struct of_bus *bus,
> > >          * This code is only enabled on powerpc. --gcl
> > >          */
> > >         ranges = of_get_property(parent, rprop, &rlen);
> > > -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> > > +       if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> > !of_isa_indirect_io(parent)) {
> > >                 pr_debug("OF: no ranges; cannot translate\n");
> > >                 return 1;
> > >         }
> > 
> > I don't see what effect that would have. What do you want to
> > achieve with this?
> 
> If I read the code correctly adding the function above would end
> up in a 1:1 mapping:
> http://lxr.free-electrons.com/source/drivers/of/address.c#L513
> 
> so taddr will be assigned with the cpu address space specified
> in the children nodes of LPC and we are not using a quirk function
> (we are just checking that we have the indirect io assigned and
> that we are on a ISA bus). Now probably there is a nit in my 
> code sketch where of_isa_indirect_io should be probably an architecture
> specific function...

But the point is that it would then return an incorrect address,
which in the worst case could be the same as another I/O space
if that happens to be at CPU address zero.

> > I think all we need from this function is to return '1' if
> > we hit an ISA I/O window, and that should happen for the two
> > interesting cases, either no 'ranges' at all, or no translation
> > for the range in question, so that __of_translate_address can
> > return OF_BAD_ADDR, and we can enter the special case
> > handling in the caller, that handles it like
> > 
> 
> I don't think this is very right as you may fail for different
> reasons other than a missing range property, e.g:
> http://lxr.free-electrons.com/source/drivers/of/address.c#L575
> 
> And even if the only failure case was a missing range if in the
> future __of_translate_address had to be reworked we would again
> make a wrong assumption...you get my point?

The newly introduced function would clearly have to make
some sanity checks. The idea is that treat the case of
not being able to translate a bus specific I/O address
into a CPU address literally and fall back to another method
of translating that address.

This matches my mental model of how we find the resource:

- start with the bus address
- try to translate that into a CPU address
- if we arrive at a CPU physical address for IORESOURCE_MEM, use that
- if we arrive at a CPU physical address for IORESOURCE_IO, translate
  that into a Linux IORESOURCE_IO token
- if there is no valid CPU physical address, try to translate
  the address into an IORESOURCE_IO using the ISA accessor
- if that fails too, give up.

If you try to fake a CPU physical address inbetween, it just
gets more confusing.

	Arnd

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 22 September 2016 15:59
> To: Gabriele Paoloni
> Cc: zhichang; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
> > > >  static int of_empty_ranges_quirk(struct device_node *np)
> > > >  {
> > > >         if (IS_ENABLED(CONFIG_PPC)) {
> > > > @@ -503,7 +512,7 @@ static int of_translate_one(struct
> device_node
> > > *parent, struct of_bus *bus,
> > > >          * This code is only enabled on powerpc. --gcl
> > > >          */
> > > >         ranges = of_get_property(parent, rprop, &rlen);
> > > > -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> > > > +       if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> > > !of_isa_indirect_io(parent)) {
> > > >                 pr_debug("OF: no ranges; cannot translate\n");
> > > >                 return 1;
> > > >         }
> > >
> > > I don't see what effect that would have. What do you want to
> > > achieve with this?
> >
> > If I read the code correctly adding the function above would end
> > up in a 1:1 mapping:
> > http://lxr.free-electrons.com/source/drivers/of/address.c#L513
> >
> > so taddr will be assigned with the cpu address space specified
> > in the children nodes of LPC and we are not using a quirk function
> > (we are just checking that we have the indirect io assigned and
> > that we are on a ISA bus). Now probably there is a nit in my
> > code sketch where of_isa_indirect_io should be probably an
> architecture
> > specific function...
> 
> But the point is that it would then return an incorrect address,
> which in the worst case could be the same as another I/O space
> if that happens to be at CPU address zero.

If we do not touch __of_address_to_resource after taddr is returned
by of_translate_address we will check for (flags & IORESOURCE_IO),
then we call pci_address_to_pio to retrieve the unique token (remember
that LPC driver will register the LPC io range to pci io_range_list).

I do not think that we can have any conflict with any other I/O space
as pci_register_io_range will guarantee that the LPC range does not
overlap with any other I/O range... 

> 
> > > I think all we need from this function is to return '1' if
> > > we hit an ISA I/O window, and that should happen for the two
> > > interesting cases, either no 'ranges' at all, or no translation
> > > for the range in question, so that __of_translate_address can
> > > return OF_BAD_ADDR, and we can enter the special case
> > > handling in the caller, that handles it like
> > >
> >
> > I don't think this is very right as you may fail for different
> > reasons other than a missing range property, e.g:
> > http://lxr.free-electrons.com/source/drivers/of/address.c#L575
> >
> > And even if the only failure case was a missing range if in the
> > future __of_translate_address had to be reworked we would again
> > make a wrong assumption...you get my point?
> 
> The newly introduced function would clearly have to make
> some sanity checks. The idea is that treat the case of
> not being able to translate a bus specific I/O address
> into a CPU address literally and fall back to another method
> of translating that address.
> 
> This matches my mental model of how we find the resource:
> 
> - start with the bus address
> - try to translate that into a CPU address
> - if we arrive at a CPU physical address for IORESOURCE_MEM, use that
> - if we arrive at a CPU physical address for IORESOURCE_IO, translate
>   that into a Linux IORESOURCE_IO token
> - if there is no valid CPU physical address, try to translate
>   the address into an IORESOURCE_IO using the ISA accessor
> - if that fails too, give up.
> 
> If you try to fake a CPU physical address inbetween, it just
> gets more confusing.
> 
> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[zhichang.yuan]

On 09/22/2016 11:20 PM, Gabriele Paoloni wrote:
>
>> -----Original Message-----
>> From: Arnd Bergmann [mailto:arnd@arndb.de]
>> Sent: 22 September 2016 15:59
>> To: Gabriele Paoloni
>> Cc: zhichang; linux-arm-kernel@lists.infradead.org;
>> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
>> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
>> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
>> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
>> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
>> kantyzc@163.com
>> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
>> Hip06
>>
>> On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
>>>>>   static int of_empty_ranges_quirk(struct device_node *np)
>>>>>   {
>>>>>          if (IS_ENABLED(CONFIG_PPC)) {
>>>>> @@ -503,7 +512,7 @@ static int of_translate_one(struct
>> device_node
>>>> *parent, struct of_bus *bus,
>>>>>           * This code is only enabled on powerpc. --gcl
>>>>>           */
>>>>>          ranges = of_get_property(parent, rprop, &rlen);
>>>>> -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>>>>> +       if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
>>>> !of_isa_indirect_io(parent)) {
>>>>>                  pr_debug("OF: no ranges; cannot translate\n");
>>>>>                  return 1;
>>>>>          }
>>>> I don't see what effect that would have. What do you want to
>>>> achieve with this?
>>> If I read the code correctly adding the function above would end
>>> up in a 1:1 mapping:
>>> http://lxr.free-electrons.com/source/drivers/of/address.c#L513
>>>
>>> so taddr will be assigned with the cpu address space specified
>>> in the children nodes of LPC and we are not using a quirk function
>>> (we are just checking that we have the indirect io assigned and
>>> that we are on a ISA bus). Now probably there is a nit in my
>>> code sketch where of_isa_indirect_io should be probably an
>> architecture
>>> specific function...
>> But the point is that it would then return an incorrect address,
>> which in the worst case could be the same as another I/O space
>> if that happens to be at CPU address zero.
> If we do not touch __of_address_to_resource after taddr is returned
> by of_translate_address we will check for (flags & IORESOURCE_IO),
> then we call pci_address_to_pio to retrieve the unique token (remember
> that LPC driver will register the LPC io range to pci io_range_list).
>
> I do not think that we can have any conflict with any other I/O space
> as pci_register_io_range will guarantee that the LPC range does not
> overlap with any other I/O range...
If we don't bypass the calling of pci_address_to_pio after 
of_translate_address,
there should no conflict between LPC logical IO range and other logical 
IO ranges
of other devices.
I guess Arnd want to skip all the translation for our LPC IO address. 
But if we do it
like that, it seems we can't avoid the possible conflict with the 
logical IO ranges of
PCI host bridges without any changes on the pci_register_io_range and 
pci_address_to_pio.
Because two completely separate I/O spaces are created without 
synchronization.

Best,
Zhichang
>>>> I think all we need from this function is to return '1' if
>>>> we hit an ISA I/O window, and that should happen for the two
>>>> interesting cases, either no 'ranges' at all, or no translation
>>>> for the range in question, so that __of_translate_address can
>>>> return OF_BAD_ADDR, and we can enter the special case
>>>> handling in the caller, that handles it like
>>>>
>>> I don't think this is very right as you may fail for different
>>> reasons other than a missing range property, e.g:
>>> http://lxr.free-electrons.com/source/drivers/of/address.c#L575
>>>
>>> And even if the only failure case was a missing range if in the
>>> future __of_translate_address had to be reworked we would again
>>> make a wrong assumption...you get my point?
>> The newly introduced function would clearly have to make
>> some sanity checks. The idea is that treat the case of
>> not being able to translate a bus specific I/O address
>> into a CPU address literally and fall back to another method
>> of translating that address.
>>
>> This matches my mental model of how we find the resource:
>>
>> - start with the bus address
>> - try to translate that into a CPU address
>> - if we arrive at a CPU physical address for IORESOURCE_MEM, use that
>> - if we arrive at a CPU physical address for IORESOURCE_IO, translate
>>    that into a Linux IORESOURCE_IO token
>> - if there is no valid CPU physical address, try to translate
>>    the address into an IORESOURCE_IO using the ISA accessor
>> - if that fails too, give up.
>>
>> If you try to fake a CPU physical address inbetween, it just
>> gets more confusing.
>>
>> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[zhichang.yuan]

On 09/22/2016 08:14 PM, Arnd Bergmann wrote:
> On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni wrote:
>>>> I think extending of_empty_ranges_quirk() may be a reasonable
>>> solution.
>>>> What do you think Arnd?
>>> I don't really like that idea, that quirk is meant to work around
>>> broken DTs, but we can just make the DT valid and implement the
>>> code properly.
>> Ok  I understand your point where it is not right to use of_empty_ranges_quirk()
>> As a quirk is used to work around broken HW or broken FW (as in this case)
>> rather than to fix code
>>
>> What about the following? I think adding the check you suggested next to
>> of_empty_ranges_quirk() is adding the case we need in the right point (thus
>> avoiding any duplication)
>>   
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np)
>>          return NULL;
>>   }
>>   
>> +static inline int of_isa_indirect_io(struct device_node *np)
>> +{
>> +       /*
>> +        * check if the current node is an isa bus and if indirectio operation
>> +        * are registered
>> +        */
>> +       return (of_bus_isa_match(np) && arm64_extio_ops);
>> +}
>> +
>>   static int of_empty_ranges_quirk(struct device_node *np)
>>   {
>>          if (IS_ENABLED(CONFIG_PPC)) {
>> @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>>           * This code is only enabled on powerpc. --gcl
>>           */
>>          ranges = of_get_property(parent, rprop, &rlen);
>> -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>> +       if (ranges == NULL && !of_empty_ranges_quirk(parent) && !of_isa_indirect_io(parent)) {
>>                  pr_debug("OF: no ranges; cannot translate\n");
>>                  return 1;
>>          }
> I don't see what effect that would have. What do you want to
> achieve with this?
>
> I think all we need from this function is to return '1' if
> we hit an ISA I/O window, and that should happen for the two
> interesting cases, either no 'ranges' at all, or no translation
> for the range in question, so that __of_translate_address can
> return OF_BAD_ADDR, and we can enter the special case
> handling in the caller, that handles it like
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 02b2903fe9d2..a18d96843fae 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node *dev,
>   	if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>   		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 (taddr == OF_BAD_ADDR)
> +			port = arch_of_address_to_pio(dev, addrp)
> +		else
> +			port = pci_address_to_pio(taddr);
> +
>   		if (port == (unsigned long)-1)
>   			return -EINVAL;
>   		r->start = port;
>   		r->end = port + size - 1;
>   	} else {
> +		if (taddr == OF_BAD_ADDR)
> +			return -EINVAL;
> +
>   		r->start = taddr;
>   		r->end = taddr + size - 1;
>   	}
>
For this patch sketch, I have a question.
Do we call pci_address_to_pio in arch_of_address_to_pio to get the 
corresponding logical IO port
for LPC??

If we don't, it seems the LPC specific IO address will conflict with PCI 
host bridges' logical IO.
Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is 
normal for ISA similar
devices), after arch_of_address_to_pio(), the r->start will be set as 
0x100, r->end will be set as
0x3FF.  And if there is one PCI host bridge who request a IO window size 
over 0x400 at the same
time, the  corresponding r->start and r->end will be set as 0x0, 0x3FF 
after of_address_to_resource
for this host bridge.  Then the IO conflict happens.

cheers,
Zhichang

>
> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> For this patch sketch, I have a question.
> Do we call pci_address_to_pio in arch_of_address_to_pio to get the 
> corresponding logical IO port
> for LPC??


No, of course not, that would be silly:

The argument to pci_address_to_pio() is a phys_addr_t, and we we don't
have one because there is no address associated with your PIO, that
is the entire point of your driver!

Also, we already know the mapping because this is what the inb/outb
workaround is looking at, so there is absolutely no reason to call it
either.

> If we don't, it seems the LPC specific IO address will conflict with PCI 
> host bridges' logical IO.
>
> Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is 
> normal for ISA similar
> devices), after arch_of_address_to_pio(), the r->start will be set as 
> 0x100, r->end will be set as
> 0x3FF.  And if there is one PCI host bridge who request a IO window size 
> over 0x400 at the same
> time, the  corresponding r->start and r->end will be set as 0x0, 0x3FF 
> after of_address_to_resource
> for this host bridge.  Then the IO conflict happens.

You would still need to reserve some space in the io_range_list
to avoid possible conflicts, which is a bit ugly with the current
definition of pci_register_io_range, but I'm sure can be done.

One way I can think of would be to change pci_register_io_range()
to just return the logical port number directly (it already
knows it!), and pass an invalid physical address (e.g. 
#define ISA_WORKAROUND_IO_PORT_WINDOW -0x10000) into it for
invalid translations.

Another alternative that just occurred to me would be to move
the pci_address_to_pio() call from __of_address_to_resource()
into of_bus_pci_translate() and then do the special handling
for the ISA/LPC bus in of_bus_isa_translate().

	Arnd

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 23 September 2016 10:52
> To: zhichang.yuan
> Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> > For this patch sketch, I have a question.
> > Do we call pci_address_to_pio in arch_of_address_to_pio to get the
> > corresponding logical IO port
> > for LPC??
> 
> 
> No, of course not, that would be silly:
> 
> The argument to pci_address_to_pio() is a phys_addr_t, and we we don't
> have one because there is no address associated with your PIO, that
> is the entire point of your driver!
> 
> Also, we already know the mapping because this is what the inb/outb
> workaround is looking at, so there is absolutely no reason to call it
> either.

Ok assume that we do not call pci_address_to_pio() for the ISA bus...
The LPC driver will register its phys address range in io_range_list,
then the IPMI driver probe will retrieve its physical address calling
of_address_to_resource and will use the indirect io to access this
address.

>From the perspective of the indirect IO function the input parameter
is an unsigned long addr that (now) can be either:
1) an IO token coming from a legacy pci device
2) a phys address that lives on the LPC bus

These are conceptually two separate address spaces (and actually they 
both start from 0).

If the input parameter can live on different address spaces that are
overlapped, even if I save the used LPC range in arm64_extio_ops->start/end
there is no way for the indirect IO to tell if the input parameter is
an I/O token or a phys address that belongs to LPC...  

Am I missing something?

Thanks

Gab

> 
> > If we don't, it seems the LPC specific IO address will conflict with
> PCI
> > host bridges' logical IO.
> >
> > Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is
> > normal for ISA similar
> > devices), after arch_of_address_to_pio(), the r->start will be set as
> > 0x100, r->end will be set as
> > 0x3FF.  And if there is one PCI host bridge who request a IO window
> size
> > over 0x400 at the same
> > time, the  corresponding r->start and r->end will be set as 0x0,
> 0x3FF
> > after of_address_to_resource
> > for this host bridge.  Then the IO conflict happens.
> 
> You would still need to reserve some space in the io_range_list
> to avoid possible conflicts, which is a bit ugly with the current
> definition of pci_register_io_range, but I'm sure can be done.
> 
> One way I can think of would be to change pci_register_io_range()
> to just return the logical port number directly (it already
> knows it!), and pass an invalid physical address (e.g.
> #define ISA_WORKAROUND_IO_PORT_WINDOW -0x10000) into it for
> invalid translations.
> 
> Another alternative that just occurred to me would be to move
> the pci_address_to_pio() call from __of_address_to_resource()
> into of_bus_pci_translate() and then do the special handling
> for the ISA/LPC bus in of_bus_isa_translate().
> 
> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote:
> Hi Arnd
> 
> > -----Original Message-----
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > Sent: 23 September 2016 10:52
> > To: zhichang.yuan
> > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org;
> > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> > Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> > benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> > kantyzc@163.com
> > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> > Hip06
> > 
> > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> > > For this patch sketch, I have a question.
> > > Do we call pci_address_to_pio in arch_of_address_to_pio to get the
> > > corresponding logical IO port
> > > for LPC??
> > 
> > 
> > No, of course not, that would be silly:
> > 
> > The argument to pci_address_to_pio() is a phys_addr_t, and we we don't
> > have one because there is no address associated with your PIO, that
> > is the entire point of your driver!
> > 
> > Also, we already know the mapping because this is what the inb/outb
> > workaround is looking at, so there is absolutely no reason to call it
> > either.
> 
> Ok assume that we do not call pci_address_to_pio() for the ISA bus...
> The LPC driver will register its phys address range in io_range_list,
> then the IPMI driver probe will retrieve its physical address calling
> of_address_to_resource and will use the indirect io to access this
> address.
> 
> From the perspective of the indirect IO function the input parameter
> is an unsigned long addr that (now) can be either:
> 1) an IO token coming from a legacy pci device
> 2) a phys address that lives on the LPC bus
> 
> These are conceptually two separate address spaces (and actually they 
> both start from 0).

Why? Any IORESOURCE_IO address always refers to the logical I/O port
range in Linux, not the physical address that is used on a bus.

> If the input parameter can live on different address spaces that are
> overlapped, even if I save the used LPC range in arm64_extio_ops->start/end
> there is no way for the indirect IO to tell if the input parameter is
> an I/O token or a phys address that belongs to LPC...  

The start address is the offset: if you get an address between 'start'
and 'end', you subtract the 'start' from it, and use that to call
the registered driver function. That works because we can safely
assume that the bus address range that the LPC driver registers starts
zero.

	Arnd

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 23 September 2016 14:43
> To: Gabriele Paoloni
> Cc: zhichang.yuan; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote:
> > Hi Arnd
> >
> > > -----Original Message-----
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > Sent: 23 September 2016 10:52
> > > To: zhichang.yuan
> > > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org;
> > > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com;
> minyard@acm.org;
> > > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> > > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> > > Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> > > benh@kernel.crashing.org; zourongrong@gmail.com;
> liviu.dudau@arm.com;
> > > kantyzc@163.com
> > > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> > > Hip06
> > >
> > > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> > > > For this patch sketch, I have a question.
> > > > Do we call pci_address_to_pio in arch_of_address_to_pio to get
> the
> > > > corresponding logical IO port
> > > > for LPC??
> > >
> > >
> > > No, of course not, that would be silly:
> > >
> > > The argument to pci_address_to_pio() is a phys_addr_t, and we we
> don't
> > > have one because there is no address associated with your PIO, that
> > > is the entire point of your driver!
> > >
> > > Also, we already know the mapping because this is what the inb/outb
> > > workaround is looking at, so there is absolutely no reason to call
> it
> > > either.
> >
> > Ok assume that we do not call pci_address_to_pio() for the ISA bus...
> > The LPC driver will register its phys address range in io_range_list,
> > then the IPMI driver probe will retrieve its physical address calling
> > of_address_to_resource and will use the indirect io to access this
> > address.
> >
> > From the perspective of the indirect IO function the input parameter
> > is an unsigned long addr that (now) can be either:
> > 1) an IO token coming from a legacy pci device
> > 2) a phys address that lives on the LPC bus
> >
> > These are conceptually two separate address spaces (and actually they
> > both start from 0).
> 
> Why? Any IORESOURCE_IO address always refers to the logical I/O port
> range in Linux, not the physical address that is used on a bus.

If I read the code correctly when you get an I/O token you just add it
to PCI_IOBASE.
This is enough since pci_remap_iospace set the virtual address to 
PCI_IOBASE + the I/O token offset; so we can read/write to
vaddr = PCI_IOBASE + token as pci_remap_iospace has mapped it correctly
to the respective PCI cpu address (that is set in the I/O range property
of the host controller)

In the patchset accessors LPC operates directly on the cpu addresses
and the input parameter of the accessors can be either an IO token or
a cpu address

+static inline void outb(u8 value, unsigned long addr)
+{
+#ifdef CONFIG_ARM64_INDIRECT_PIO
+	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
+			addr <= arm64_extio_ops->end)

Here below we operate on cpu address

+		extio_outb(value, addr);
+	else
+#endif

In the case below we have an I/O token added to PCI_IOBASE
to calculate the virtual address 

+		writeb(value, PCI_IOBASE + addr);
+}

My point is that if do not call pci_address_to_pio() in 
__of_address_to_resource for the ISA LPC exception then the accessors
are called either by passing an IO token or a cpu address...and from
the accessors perspective we do not know...

Thanks
Gab 

> 
> > If the input parameter can live on different address spaces that are
> > overlapped, even if I save the used LPC range in arm64_extio_ops-
> >start/end
> > there is no way for the indirect IO to tell if the input parameter is
> > an I/O token or a phys address that belongs to LPC...
> 
> The start address is the offset: if you get an address between 'start'
> and 'end', you subtract the 'start' from it, and use that to call
> the registered driver function. That works because we can safely
> assume that the bus address range that the LPC driver registers starts
> zero.
> 
> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Friday, September 23, 2016 2:59:55 PM CEST Gabriele Paoloni wrote:
> 
> > > From the perspective of the indirect IO function the input parameter
> > > is an unsigned long addr that (now) can be either:
> > > 1) an IO token coming from a legacy pci device
> > > 2) a phys address that lives on the LPC bus
> > >
> > > These are conceptually two separate address spaces (and actually they
> > > both start from 0).
> > 
> > Why? Any IORESOURCE_IO address always refers to the logical I/O port
> > range in Linux, not the physical address that is used on a bus.
> 
> If I read the code correctly when you get an I/O token you just add it
> to PCI_IOBASE.
> This is enough since pci_remap_iospace set the virtual address to 
> PCI_IOBASE + the I/O token offset; so we can read/write to
> vaddr = PCI_IOBASE + token as pci_remap_iospace has mapped it correctly
> to the respective PCI cpu address (that is set in the I/O range property
> of the host controller)
> 
> In the patchset accessors LPC operates directly on the cpu addresses
> and the input parameter of the accessors can be either an IO token or
> a cpu address
> 
> +static inline void outb(u8 value, unsigned long addr)
> +{
> +#ifdef CONFIG_ARM64_INDIRECT_PIO
> +       if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
> +                       addr <= arm64_extio_ops->end)
> 
> Here below we operate on cpu address
> 
> +               extio_outb(value, addr);
> +       else
> +#endif

I missed this bug earlier, this obviously needs to be

		arm64_extio_ops->outb(value, addr - arm64_extio_ops->start);

or possibly

		arm64_extio_ops->outb(arm64_extio_ops, value, addr);

as the outb function won't know what the offset is, but
that needed to be fixed regardless.

	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[zhichang]

Hi, Arnd,


On 2016年09月23日 17:51, Arnd Bergmann wrote:
> On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
>> For this patch sketch, I have a question.
>> Do we call pci_address_to_pio in arch_of_address_to_pio to get the 
>> corresponding logical IO port
>> for LPC??
> 
> 
> No, of course not, that would be silly:
> 
> The argument to pci_address_to_pio() is a phys_addr_t, and we we don't
> have one because there is no address associated with your PIO, that
> is the entire point of your driver!
> 
ok. I think I know you points. The physical addresses of LPC are only the LPC
domain addresses, not the really CPU physical addresses. That is just why you
don't support the ranges property usage in patch V3. Consequently, It is not so
reasonable to call pci_address_to_pio() with LPC address because that function
is only suitable for cpu physical address.

But just as you said in the next email reply to Gabriele, "Any IORESOURCE_IO
address always refers to the logical I/O port range in Linux, not the physical
address that is used on a bus.", Any devices which support IO accesses should
have their own unique logical IO range to drive the corresponding hardware. It
means that the drivers should know the mapping between physical port/memory
address and logical IO depend on the device specific I/O mode. At this moment,
only PCI host bridge setup a logical IO range allocation mechanism to manipulate
this logical IO range, and this way applies cpu physical address(memory) as the
input. Now, our LPC also need subrange from this common logical IO range, but
with legacy I/O port rather than CPU memory address. Ok, it break the
precondition of pci_register_io_range/pci_pio_to_address, we should not use them
directly for LPC although the calling of pci_pio_to_address is simple and less
change on the relevant code. We had done like that in V3...

So, the key issue is how to get a logical IO subrange which is not conflicted
with others, such as pci host bridges??

I list several ideas for discussion:

1. reserve a specific logical IO subrange for LPC

I describe this in "Note 1" below. Please check it.
This way seems simple without much changes, but it is not generic.

2. setup a separate logical IO subrange allocation mechanism specific for LPC/ISA

Just as your suggestion before, add the arch_of_address_to_pio() for the devices
which operate I/O with legacy I/O port address rather than memory address in
MMIO mode. That arch_of_address_to_pio() will return non-conflict logical IO
with PCI host bridge at last. But the logical IO range is global, those
functions for LPC/ISA specific logical IO subrange allocation must be
synchronized with pci_register_io_range/pci_pio_to_address to know what logical
ranges had been populated. It is not good for the implement dispersion on same
issue.

3. setup a new underlying method to control the logical IO range management

Based on the existing resource management, add a simplified logical IO range
management support which only request the logical IO ranges according the IO
range size ( similar to IORESOURCE_SIZEALIGN mode ), no matter what type the
physical address is. Then revise the current pci_register_io_range to adopt this
new method. Of-course, LPC/ISA request the logical IO with this new method too.

This is just a proposition. It is more workload compared with other solutions.

What do you think about these? Any more ideas?


> Also, we already know the mapping because this is what the inb/outb
> workaround is looking at, so there is absolutely no reason to call it
> either.
> 
>> If we don't, it seems the LPC specific IO address will conflict with PCI 
>> host bridges' logical IO.
>>
>> Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is 
>> normal for ISA similar
>> devices), after arch_of_address_to_pio(), the r->start will be set as 
>> 0x100, r->end will be set as
>> 0x3FF.  And if there is one PCI host bridge who request a IO window size 
>> over 0x400 at the same
>> time, the  corresponding r->start and r->end will be set as 0x0, 0x3FF 
>> after of_address_to_resource
>> for this host bridge.  Then the IO conflict happens.
> 
> You would still need to reserve some space in the io_range_list
> to avoid possible conflicts, which is a bit ugly with the current
> definition of pci_register_io_range, but I'm sure can be done.
> 

Note 1) Do you remember patch V2? There, I modified the pci.c like that to
reserve 0 - PCIBIOS_MIN_IO (it is 0x1000) :

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d51..ac2e569 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3221,7 +3221,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resourc

 #ifdef PCI_IOBASE
        struct io_range *range;
-       resource_size_t allocated_size = 0;
+       resource_size_t allocated_size = PCIBIOS_MIN_IO;

        /* check if the range hasn't been previously recorded */
        spin_lock(&io_range_lock);
@@ -3270,7 +3270,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)

 #ifdef PCI_IOBASE
        struct io_range *range;
-       resource_size_t allocated_size = 0;
+       resource_size_t allocated_size = PCIBIOS_MIN_IO;

        if (pio > IO_SPACE_LIMIT)
                return address;
@@ -3293,7 +3293,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t addres
 {
 #ifdef PCI_IOBASE
        struct io_range *res;
-       resource_size_t offset = 0;
+       resource_size_t offset = PCIBIOS_MIN_IO;
        unsigned long addr = -1;

        spin_lock(&io_range_lock);

Based on this, a exclusive logical IO subrange is for LPC now. Then we certainly
can add some special handling in __of_address_to_resource or
__of_translate_address --> of_translate_one to return the untranslated LPC/ISA
IO address. But to be honest, I think we don't need this special handling in
address.c anymore. We had known the LPC/ISA IO is 1:1 to logical IO, just think
any logical IO port among 0 - 0x1000 should call the LPC registered I/O hooks in
the new in/out().

Furthermore, we can make the reservation is not fixed as PCIBIOS_MIN_IO. If the
LPC/ISA probing is run before PCI host bridge probing, we can reserve a
non-fixed logcial IO subrange what LPC/ISA ask for.

This solution is based on an assumption that no any other devices have to
request the specific logical IO subrange for LPC/ISA. Probably this assumption
is ok on arm64, you known, there is no real IO space as X86. But anyway, this
reservation is not so generic, depended on some special handling.

Does this idea match your comments??


> One way I can think of would be to change pci_register_io_range()
> to just return the logical port number directly (it already
> knows it!), and pass an invalid physical address (e.g. 
> #define ISA_WORKAROUND_IO_PORT_WINDOW -0x10000) into it for
> invalid translations.
> 

I am not so clear know your idea here.  Do you want to select an unpopulated CPU
address as the parent address in range property?? or anything else???


> Another alternative that just occurred to me would be to move
> the pci_address_to_pio() call from __of_address_to_resource()
> into of_bus_pci_translate() and then do the special handling
> for the ISA/LPC bus in of_bus_isa_translate().

As for this idea, do you mean that of_translate_address will directly return the
final logical IO start address?? It seems to extend the definition of
of_translate_address.


Thanks,
Zhichang

> 
> 	Arnd
> 

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[zhichang]

Hi, Arnd,

On 2016年09月23日 23:55, Arnd Bergmann wrote:
> On Friday, September 23, 2016 2:59:55 PM CEST Gabriele Paoloni wrote:
>>
>>>> From the perspective of the indirect IO function the input parameter
>>>> is an unsigned long addr that (now) can be either:
>>>> 1) an IO token coming from a legacy pci device
>>>> 2) a phys address that lives on the LPC bus
>>>>
>>>> These are conceptually two separate address spaces (and actually they
>>>> both start from 0).
>>>
>>> Why? Any IORESOURCE_IO address always refers to the logical I/O port
>>> range in Linux, not the physical address that is used on a bus.
>>
>> If I read the code correctly when you get an I/O token you just add it
>> to PCI_IOBASE.
>> This is enough since pci_remap_iospace set the virtual address to 
>> PCI_IOBASE + the I/O token offset; so we can read/write to
>> vaddr = PCI_IOBASE + token as pci_remap_iospace has mapped it correctly
>> to the respective PCI cpu address (that is set in the I/O range property
>> of the host controller)
>>
>> In the patchset accessors LPC operates directly on the cpu addresses
>> and the input parameter of the accessors can be either an IO token or
>> a cpu address
>>
>> +static inline void outb(u8 value, unsigned long addr)
>> +{
>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>> +       if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
>> +                       addr <= arm64_extio_ops->end)
>>
>> Here below we operate on cpu address
>>
>> +               extio_outb(value, addr);
>> +       else
>> +#endif
> 
> I missed this bug earlier, this obviously needs to be
> 
> 		arm64_extio_ops->outb(value, addr - arm64_extio_ops->start);
> 
> or possibly
> 
> 		arm64_extio_ops->outb(arm64_extio_ops, value, addr);
> 
> as the outb function won't know what the offset is, but
> that needed to be fixed regardless.

In V3, the outb is :

void outb(u8 value, unsigned long addr)
{
	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||
			arm64_extio_ops->end < addr)
		writeb(value, PCI_IOBASE + addr);
	else
		if (arm64_extio_ops->pfout)
			arm64_extio_ops->pfout(arm64_extio_ops->devpara,
				addr + arm64_extio_ops->ptoffset, &value,
				sizeof(u8), 1);
}

here, arm64_extio_ops->ptoffset is the offset between the real legacy IO address
and the logical IO address, similar to the offset of primary address and
secondary address in PCI bridge.

But in V3, LPC driver call pci_address_to_pio to request the logical IO as PCI
host bridge during its probing.


cheers,
Zhichang



> 
> 	Arnd
> 

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Arnd Bergmann]

On Saturday, September 24, 2016 4:14:15 PM CEST zhichang wrote:
> 
> In V3, the outb is :
> 
> void outb(u8 value, unsigned long addr)
> {
>         if (!arm64_extio_ops || arm64_extio_ops->start > addr ||
>                         arm64_extio_ops->end < addr)
>                 writeb(value, PCI_IOBASE + addr);
>         else
>                 if (arm64_extio_ops->pfout)
>                         arm64_extio_ops->pfout(arm64_extio_ops->devpara,
>                                 addr + arm64_extio_ops->ptoffset, &value,
>                                 sizeof(u8), 1);
> }
> 
> here, arm64_extio_ops->ptoffset is the offset between the real legacy IO address
> and the logical IO address, similar to the offset of primary address and
> secondary address in PCI bridge.

Ok, though we can probably simplify this by making the assumption that
'ptoffset' is the negative of 'start', as the bus we register should
always start at port zero.

> But in V3, LPC driver call pci_address_to_pio to request the logical IO as PCI
> host bridge during its probing.

Right, so this still needs to be fixed.

	Arnd

RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 23 September 2016 14:43
> To: Gabriele Paoloni
> Cc: zhichang.yuan; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote:
> > Hi Arnd
> >
> > > -----Original Message-----
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > Sent: 23 September 2016 10:52
> > > To: zhichang.yuan
> > > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org;
> > > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com;
> minyard@acm.org;
> > > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> > > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> > > Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> > > benh@kernel.crashing.org; zourongrong@gmail.com;
> liviu.dudau@arm.com;
> > > kantyzc@163.com
> > > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> > > Hip06
> > >
> > > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> > > > For this patch sketch, I have a question.
> > > > Do we call pci_address_to_pio in arch_of_address_to_pio to get
> the
> > > > corresponding logical IO port
> > > > for LPC??
> > >
> > >
> > > No, of course not, that would be silly:
> > >
> > > The argument to pci_address_to_pio() is a phys_addr_t, and we we
> don't
> > > have one because there is no address associated with your PIO, that
> > > is the entire point of your driver!
> > >
> > > Also, we already know the mapping because this is what the inb/outb
> > > workaround is looking at, so there is absolutely no reason to call
> it
> > > either.
> >
> > Ok assume that we do not call pci_address_to_pio() for the ISA bus...
> > The LPC driver will register its phys address range in io_range_list,
> > then the IPMI driver probe will retrieve its physical address calling
> > of_address_to_resource and will use the indirect io to access this
> > address.
> >
> > From the perspective of the indirect IO function the input parameter
> > is an unsigned long addr that (now) can be either:
> > 1) an IO token coming from a legacy pci device
> > 2) a phys address that lives on the LPC bus
> >
> > These are conceptually two separate address spaces (and actually they
> > both start from 0).
> 
> Why? Any IORESOURCE_IO address always refers to the logical I/O port
> range in Linux, not the physical address that is used on a bus.
> 
> > If the input parameter can live on different address spaces that are
> > overlapped, even if I save the used LPC range in arm64_extio_ops-
> >start/end
> > there is no way for the indirect IO to tell if the input parameter is
> > an I/O token or a phys address that belongs to LPC...
>

Assume that in the probe function the LPC drivers calls pci_register_io_range
for the LPC cpu address range (0 to PCIBIOS_MIN_I0) and does not scan
the children DT nodes.

Consider for example the ipmi driver:
When the reg property is read to retrieve the ipmi <<i/o port>> in
http://lxr.free-electrons.com/source/drivers/char/ipmi/ipmi_si_intf.c#L2622
if we do not call pci_address_to_pio in __of_address_to_resource the input
parameter of inb/outb will be the cpu address of the ipmi (not translated
to a unique token id).

So inb/outb at this stage can be called passing either a cpu address or a
token io port.

If we set arm64_extio_ops->start/end to 0 and PCIBIOS_MIN_I0 respectively
we still cannot tell inside inb/outb if the passed address is a token or
an LPC cpu address as the ipmi cpu address can overlap with another device
I/O token...

My suggestion is to call pci_address_to_pio even for devices living on
the LPC bus; then in the LPC probe we set arm64_extio_ops->start/end to
the I/O tokens that correspond to the LPC cpu address range (in the LPC probe
function we call pci_address_to_pio after we have called pci_register_io_range);
finally in inb/outb we know that we can get only an I/O token as input
parameter and we check it against arm64_extio_ops->start/end to decide
whether to call the LPC accessors or readb/writeb...

> The start address is the offset: if you get an address between 'start'
> and 'end', you subtract the 'start' from it, and use that to call
> the registered driver function. That works because we can safely
> assume that the bus address range that the LPC driver registers starts
> zero.

Sorry I cannot follow what you said here above: <<if you get an address 
between 'start' and 'end'>>...in which function?

Thanks

Gab

> 
> 	Arnd

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Jon Masters]

On 09/14/2016 02:32 PM, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote:

>> And there are probably multiple child devices under LPC, the global arm64_extio_ops only can cover one PIO range. It is fortunate only ipmi driver can not support I/O
>> operation registering, serial driver has serial_in/serial_out to
>> be registered. So, only the PIO range for ipmi device is stored
>> in arm64_extio_ops and the indirect-IO
>> works well for ipmi device.
> 
> You should not do that in the serial driver, please just use the
> normal 8250 driver that works fine once you handle the entire
> port range.

Just for the record, Arnd has the right idea. There is only one type of
UART permitted by SBSA (PL011). We carved out an exception for a design
that was already in flight and allowed it to be 16550. That other design
was then corrected in future generations to be PL011 as we required it
to be. Then there's the Hip06. I've given feedback elsewhere about the
need for there to be (at most) two types of UART in the wild. This "LPC"
stuff needs cleaning up (feedback given elsewhere already on that), but
we won't be adding a third serial driver into the mix in order to make
it work. There will be standard ARM servers. There will not be the
kinda-sorta-standard. Thanks.

Jon.

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[John Garry]

On 02/10/2016 23:03, Jon Masters wrote:
> On 09/14/2016 02:32 PM, Arnd Bergmann wrote:
>> On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote:
>
>>> And there are probably multiple child devices under LPC, the global arm64_extio_ops only can cover one PIO range. It is fortunate only ipmi driver can not support I/O
>>> operation registering, serial driver has serial_in/serial_out to
>>> be registered. So, only the PIO range for ipmi device is stored
>>> in arm64_extio_ops and the indirect-IO
>>> works well for ipmi device.
>>
>> You should not do that in the serial driver, please just use the
>> normal 8250 driver that works fine once you handle the entire
>> port range.
>
> Just for the record, Arnd has the right idea. There is only one type of
> UART permitted by SBSA (PL011). We carved out an exception for a design
> that was already in flight and allowed it to be 16550. That other design
> was then corrected in future generations to be PL011 as we required it
> to be. Then there's the Hip06. I've given feedback elsewhere about the
> need for there to be (at most) two types of UART in the wild. This "LPC"
> stuff needs cleaning up (feedback given elsewhere already on that), but
> we won't be adding a third serial driver into the mix in order to make
> it work. There will be standard ARM servers. There will not be the
> kinda-sorta-standard. Thanks.
>

Right, so I think Zhichang can make the necessary generic changes to 
8250 OF driver to support IO port as well as MMIO-based.

However an LPC-based earlycon driver is still required.

A note on hip07-based D05 (for those unaware): this does not use 
LPC-based uart. It uses PL011. The hardware guys have managed some 
trickery where they loopback the serial line around the BMC/CPLD. But we 
still need it for hip06 D03 and any other boards which want to use LPC 
bus for uart.

A question on SBSA: does it propose how to provide serial via BMC for SOL?


> Jon.
>
>
> .
>

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[Benjamin Herrenschmidt]

On Tue, 2016-10-04 at 13:02 +0100, John Garry wrote:
> Right, so I think Zhichang can make the necessary generic changes to 
> 8250 OF driver to support IO port as well as MMIO-based.
> 
> However an LPC-based earlycon driver is still required.
> 
> A note on hip07-based D05 (for those unaware): this does not use 
> LPC-based uart. It uses PL011. The hardware guys have managed some 
> trickery where they loopback the serial line around the BMC/CPLD. But we 
> still need it for hip06 D03 and any other boards which want to use LPC 
> bus for uart.
> 
> A question on SBSA: does it propose how to provide serial via BMC for SOL?

Probably another reason to keep 8250 as a legal option ... The (very
popular) Aspeed BMCs tend to do this via a 8250-looking virtual UART on
LPC.

Cheers,
Ben,

Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

[John Garry]

On 06/10/2016 01:18, Benjamin Herrenschmidt wrote:
> On Tue, 2016-10-04 at 13:02 +0100, John Garry wrote:
>> Right, so I think Zhichang can make the necessary generic changes to
>> 8250 OF driver to support IO port as well as MMIO-based.
>>
>> However an LPC-based earlycon driver is still required.
>>
>> A note on hip07-based D05 (for those unaware): this does not use
>> LPC-based uart. It uses PL011. The hardware guys have managed some
>> trickery where they loopback the serial line around the BMC/CPLD. But we
>> still need it for hip06 D03 and any other boards which want to use LPC
>> bus for uart.
>>
>> A question on SBSA: does it propose how to provide serial via BMC for SOL?
>
> Probably another reason to keep 8250 as a legal option ... The (very
> popular) Aspeed BMCs tend to do this via a 8250-looking virtual UART on
> LPC.
>
> Cheers,
> Ben,

I think we're talking about the same thing for our LPC-based UART.

John

>
>
> .
>