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

Groups > linux.kernel > #1516761 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH V5 0/3] ARM64 LPC: legacy ISA I/O support "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-11-08 04:30 +0100
    [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-11-08 04:50 +0100
      Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-08 17:30 +0100
        RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-09 13:20 +0100
          Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-09 22:40 +0100
            Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-11-10 07:50 +0100
              Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-10 10:20 +0100
                Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-11-10 13:40 +0100
                RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-10 16:40 +0100
                  Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-11-10 17:10 +0100
                    Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-11-11 11:20 +0100
                    Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "liviu.dudau@arm.com" <liviu.dudau@arm.com> - 2016-11-11 11:50 +0100
                    RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-11 14:50 +0100
                      Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "liviu.dudau@arm.com" <liviu.dudau@arm.com> - 2016-11-11 15:50 +0100
                        RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-11-11 17:00 +0100
                          Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "liviu.dudau@arm.com" <liviu.dudau@arm.com> - 2016-11-11 19:20 +0100
                        Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06 "zhichang.yuan" <zhichang.yuan02@gmail.com> - 2016-11-11 18:10 +0100

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

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

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

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

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

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

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

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

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

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

-- 
1.9.1

[toc] | [next] | [standalone]

#1516766 — [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

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>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 MAINTAINERS            |   8 +
 drivers/bus/Kconfig    |   8 +
 drivers/bus/Makefile   |   1 +
 drivers/bus/hisi_lpc.c | 501 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 518 insertions(+)
 create mode 100644 drivers/bus/hisi_lpc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ccae35b..4c7a350 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5729,6 +5729,14 @@ F:	include/uapi/linux/if_hippi.h
 F:	net/802/hippi.c
 F:	drivers/net/hippi/
 
+HISILICON LPC BUS DRIVER
+M:	Zhichang Yuan <yuanzhichang@hisilicon.com>
+L:	linux-arm-kernel@lists.infradead.org
+W:	http://www.hisilicon.com
+S:	Maintained
+F:	drivers/bus/hisi_lpc.c
+F:	Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
+
 HISILICON NETWORK SUBSYSTEM DRIVER
 M:	Yisen Zhuang <yisen.zhuang@huawei.com>
 M:	Salil Mehta <salil.mehta@huawei.com>
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 7875105..4fa8ab4 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 c6cfa6b..10b4983 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ARM_CCI)		+= arm-cci.o
 obj-$(CONFIG_ARM_CCN)		+= arm-ccn.o
 
 obj-$(CONFIG_BRCMSTB_GISB_ARB)	+= brcmstb_gisb.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..47dc081
--- /dev/null
+++ b/drivers/bus/hisi_lpc.c
@@ -0,0 +1,501 @@
+/*
+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
+ * Author: Zou Rongrong <zourongrong@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>
+
+/*
+ * setting this bit 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		0x02
+
+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 extio_ops io_ops;
+};
+
+
+/* The maximum continous operations*/
+#define LPC_MAX_OPCNT	16
+/* only support IO data unit length is four at maximum */
+#define LPC_MAX_DULEN	4
+#if LPC_MAX_DULEN > LPC_MAX_OPCNT
+#error "LPC.. MAX_DULEN must be not bigger than MAX_OPCNT!"
+#endif
+
+#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	0x08
+#define LPC_CMD_TYPE_IO		0x00
+#define LPC_CMD_WRITE		0x01
+#define LPC_CMD_READ		0x00
+/* the bit attribute is W1C. 1 represents OK. */
+#define LPC_STAT_BYIRQ		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;
+
+	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 *lpcdev,
+				struct lpc_cycle_para *para,
+				unsigned long ptaddr, unsigned char *buf,
+				unsigned long opcnt)
+{
+	unsigned long cnt_per_trans;
+	unsigned int cmd_word;
+	unsigned int waitcnt;
+	int ret;
+
+	if (!buf || !opcnt || !para || !para->csize || !lpcdev)
+		return -EINVAL;
+
+	if (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;
+		waitcnt = LPC_MAX_WAITCNT;
+	}
+
+	ret = 0;
+	cnt_per_trans = (para->csize == 1) ? opcnt : para->csize;
+	for (; opcnt && !ret; cnt_per_trans = para->csize) {
+		unsigned long flags;
+
+		/* whole operation must be atomic */
+		spin_lock_irqsave(&lpcdev->cycle_lock, flags);
+
+		writel(cnt_per_trans, lpcdev->membase + LPC_REG_OP_LEN);
+
+		writel(cmd_word, lpcdev->membase + LPC_REG_CMD);
+
+		writel(ptaddr, lpcdev->membase + LPC_REG_ADDR);
+
+		writel(START_WORK, lpcdev->membase + LPC_REG_START);
+
+		/* whether the operation is finished */
+		ret = wait_lpc_idle(lpcdev->membase, waitcnt);
+		if (!ret) {
+			opcnt -= cnt_per_trans;
+			for (; cnt_per_trans--; buf++)
+				*buf = readl(lpcdev->membase + LPC_REG_RDATA);
+		}
+
+		spin_unlock_irqrestore(&lpcdev->cycle_lock, flags);
+	}
+
+	return ret;
+}
+
+/**
+ * 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 *lpcdev,
+				struct lpc_cycle_para *para,
+				unsigned long ptaddr,
+				const unsigned char *buf,
+				unsigned long opcnt)
+{
+	unsigned long cnt_per_trans;
+	unsigned int cmd_word;
+	unsigned int waitcnt;
+	int ret;
+
+	if (!buf || !opcnt || !para || !lpcdev)
+		return -EINVAL;
+
+	if (opcnt > LPC_MAX_OPCNT)
+		return -EINVAL;
+	/* default is increasing address */
+	cmd_word = LPC_CMD_TYPE_IO | LPC_CMD_WRITE;
+	waitcnt = (LPC_PEROP_WAITCNT);
+	if (!(para->opflags & FG_INCRADDR_LPC)) {
+		cmd_word |= LPC_CMD_SAMEADDR;
+		waitcnt = LPC_MAX_WAITCNT;
+	}
+
+	ret = 0;
+	cnt_per_trans = (para->csize == 1) ? opcnt : para->csize;
+	for (; opcnt && !ret; cnt_per_trans = para->csize) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&lpcdev->cycle_lock, flags);
+
+		writel(cnt_per_trans, lpcdev->membase + LPC_REG_OP_LEN);
+		opcnt -= cnt_per_trans;
+		for (; cnt_per_trans--; buf++)
+			writel(*buf, lpcdev->membase + LPC_REG_WDATA);
+
+		writel(cmd_word, lpcdev->membase + LPC_REG_CMD);
+
+		writel(ptaddr, lpcdev->membase + LPC_REG_ADDR);
+
+		writel(START_WORK, lpcdev->membase + LPC_REG_START);
+
+		/* whether the operation is finished */
+		ret = wait_lpc_idle(lpcdev->membase, waitcnt);
+
+		spin_unlock_irqrestore(&lpcdev->cycle_lock, flags);
+	}
+
+	return ret;
+}
+
+/**
+ * hisilpc_comm_in - read/input the data from the I/O peripheral through LPC.
+ * @devobj: pointer to the device information relevant to LPC controller.
+ * @ptaddr: the target I/O port address.
+ * @dlen: the data length required to read from the target I/O port.
+ *
+ * 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.
+ */
+static u64 hisilpc_comm_in(void *devobj, unsigned long ptaddr, size_t dlen)
+{
+	struct hisilpc_dev *lpcdev;
+	struct lpc_cycle_para iopara;
+	u32 rd_data;
+	unsigned char *newbuf;
+	int ret = 0;
+
+	if (!devobj || !dlen || dlen > LPC_MAX_DULEN ||	(dlen & (dlen - 1)))
+		return -1;
+
+	/* the local buffer must be enough for one data unit */
+	if (sizeof(rd_data) < dlen)
+		return -1;
+
+	newbuf = (unsigned char *)&rd_data;
+
+	lpcdev = (struct hisilpc_dev *)devobj;
+
+	iopara.opflags = FG_INCRADDR_LPC;
+	iopara.csize = dlen;
+
+	ret = hisilpc_target_in(lpcdev, &iopara, ptaddr, newbuf, dlen);
+	if (ret)
+		return -1;
+
+	return le32_to_cpu(rd_data);
+}
+
+/**
+ * hisilpc_comm_out - write/output the data whose maximal length is four bytes to
+ *		the I/O peripheral through LPC.
+ * @devobj: pointer to the device information relevant to LPC controller.
+ * @outval: a value to be outputed from caller, maximum is four bytes.
+ * @ptaddr: the target I/O port address.
+ * @dlen: the data length required writing to the target I/O port .
+ *
+ * This function is corresponding to out(b,w,l) only
+ *
+ */
+static void hisilpc_comm_out(void *devobj, unsigned long ptaddr,
+				u32 outval, size_t dlen)
+{
+	struct hisilpc_dev *lpcdev;
+	struct lpc_cycle_para iopara;
+	const unsigned char *newbuf;
+
+	if (!devobj || !dlen || dlen > LPC_MAX_DULEN)
+		return;
+
+	if (sizeof(outval) < dlen)
+		return;
+
+	outval = cpu_to_le32(outval);
+
+	newbuf = (const unsigned char *)&outval;
+	lpcdev = (struct hisilpc_dev *)devobj;
+
+	iopara.opflags = FG_INCRADDR_LPC;
+	iopara.csize = dlen;
+
+	hisilpc_target_out(lpcdev, &iopara, ptaddr, newbuf, dlen);
+}
+
+/**
+ * hisilpc_comm_ins - read/input the data in buffer to the I/O peripheral
+ *		    through LPC, it corresponds to ins(b,w,l)
+ * @devobj: pointer to the device information relevant to LPC controller.
+ * @ptaddr: the target I/O port address.
+ * @inbuf: a buffer where read/input data bytes are stored.
+ * @dlen: the data length required writing to the target I/O port.
+ * @count: how many data units whose length is dlen will be read.
+ *
+ */
+static u64 hisilpc_comm_ins(void *devobj, unsigned long ptaddr,
+			void *inbuf, size_t dlen, unsigned int count)
+{
+	struct hisilpc_dev *lpcdev;
+	struct lpc_cycle_para iopara;
+	unsigned char *newbuf;
+	unsigned int loopcnt, cntleft;
+	unsigned int max_perburst;
+	int ret = 0;
+
+	if (!devobj || !inbuf || !count || !dlen ||
+			dlen > LPC_MAX_DULEN || (dlen & (dlen - 1)))
+		return -1;
+
+	iopara.opflags = 0;
+	if (dlen > 1)
+		iopara.opflags |= FG_INCRADDR_LPC;
+	iopara.csize = dlen;
+
+	lpcdev = (struct hisilpc_dev *)devobj;
+	newbuf = (unsigned char *)inbuf;
+	/*
+	 * ensure data stream whose length is multiple of dlen to be processed
+	 * each IO input
+	 */
+	max_perburst = LPC_MAX_OPCNT & (~(dlen - 1));
+	cntleft = count * dlen;
+	do {
+		loopcnt = (cntleft >= max_perburst) ? max_perburst : cntleft;
+		ret = hisilpc_target_in(lpcdev, &iopara, ptaddr, newbuf,
+						loopcnt);
+		if (ret)
+			break;
+		newbuf += loopcnt;
+		cntleft -= loopcnt;
+	} while (cntleft);
+
+	return ret;
+}
+
+/**
+ * hisilpc_comm_outs - write/output the data in buffer to the I/O peripheral
+ *		    through LPC, it corresponds to outs(b,w,l)
+ * @devobj: pointer to the device information relevant to LPC controller.
+ * @ptaddr: the target I/O port address.
+ * @outbuf: a buffer where write/output data bytes are stored.
+ * @dlen: the data length required writing to the target I/O port .
+ * @count: how many data units whose length is dlen will be written.
+ *
+ */
+static void hisilpc_comm_outs(void *devobj, unsigned long ptaddr,
+			const void *outbuf, size_t dlen, unsigned int count)
+{
+	struct hisilpc_dev *lpcdev;
+	struct lpc_cycle_para iopara;
+	const unsigned char *newbuf;
+	unsigned int loopcnt, cntleft;
+	unsigned int max_perburst;
+	int ret = 0;
+
+	if (!devobj || !outbuf || !count || !dlen ||
+			dlen > LPC_MAX_DULEN || (dlen & (dlen - 1)))
+		return;
+
+	iopara.opflags = 0;
+	if (dlen > 1)
+		iopara.opflags |= FG_INCRADDR_LPC;
+	iopara.csize = dlen;
+
+	lpcdev = (struct hisilpc_dev *)devobj;
+	newbuf = (unsigned char *)outbuf;
+	/*
+	 * ensure data stream whose lenght is multiple of dlen to be processed
+	 * each IO input
+	 */
+	max_perburst = LPC_MAX_OPCNT & (~(dlen - 1));
+	cntleft = count * dlen;
+	do {
+		loopcnt = (cntleft >= max_perburst) ? max_perburst : cntleft;
+		ret = hisilpc_target_out(lpcdev, &iopara, ptaddr, newbuf,
+						loopcnt);
+		if (ret)
+			break;
+		newbuf += loopcnt;
+		cntleft -= loopcnt;
+	} while (cntleft);
+}
+
+/**
+ * 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, "probing hslpc...\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, "ioremap memory FAIL(%d)!\n",
+				PTR_ERR(lpcdev->membase));
+		return PTR_ERR(lpcdev->membase);
+	}
+	/*
+	 * The first PCIBIOS_MIN_IO is reserved specifically for indirectIO.
+	 * It will separate indirectIO range from pci host bridge to
+	 * avoid the possible PIO conflict.
+	 * Set the indirectIO range directly here.
+	 */
+	lpcdev->io_ops.start = 0;
+	lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
+	lpcdev->io_ops.devpara = lpcdev;
+	lpcdev->io_ops.pfin = hisilpc_comm_in;
+	lpcdev->io_ops.pfout = hisilpc_comm_out;
+	lpcdev->io_ops.pfins = hisilpc_comm_ins;
+	lpcdev->io_ops.pfouts = hisilpc_comm_outs;
+
+	platform_set_drvdata(pdev, lpcdev);
+
+	arm64_set_extops(&lpcdev->io_ops);
+
+	/*
+	 * The children scanning is only for dts mode. For ACPI children,
+	 * the corresponding devices had be created during acpi scanning.
+	 */
+	ret = 0;
+	if (!has_acpi_companion(&pdev->dev))
+		ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,
+				&pdev->dev);
+
+	if (!ret)
+		dev_info(&pdev->dev, "hslpc end probing. range[0x%lx - %lx]\n",
+			arm64_extio_ops->start, arm64_extio_ops->end);
+	else
+		dev_info(&pdev->dev, "hslpc probing is fail(%d)\n", ret);
+
+	return ret;
+}
+
+static const struct of_device_id hisilpc_of_match[] = {
+	{
+		.compatible = "hisilicon,hip06-lpc",
+	},
+	{},
+};
+
+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);
-- 
1.9.1

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

#1517358 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote:
> +       /*
> +        * The first PCIBIOS_MIN_IO is reserved specifically for indirectIO.
> +        * It will separate indirectIO range from pci host bridge to
> +        * avoid the possible PIO conflict.
> +        * Set the indirectIO range directly here.
> +        */
> +       lpcdev->io_ops.start = 0;
> +       lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
> +       lpcdev->io_ops.devpara = lpcdev;
> +       lpcdev->io_ops.pfin = hisilpc_comm_in;
> +       lpcdev->io_ops.pfout = hisilpc_comm_out;
> +       lpcdev->io_ops.pfins = hisilpc_comm_ins;
> +       lpcdev->io_ops.pfouts = hisilpc_comm_outs;

I have to look at patch 2 in more detail again, after missing a few review
rounds. I'm still a bit skeptical about hardcoding a logical I/O port
range here, and would hope that we can just go through the same
assignment of logical port ranges that we have for PCI buses, decoupling
the bus addresses from the linux-internal ones.

	Arnd

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

#1518109 — RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 08 November 2016 16:25
> To: Yuanzhichang
> Cc: catalin.marinas@arm.com; will.deacon@arm.com; robh+dt@kernel.org;
> bhelgaas@google.com; mark.rutland@arm.com; olof@lixom.net; linux-arm-
> kernel@lists.infradead.org; lorenzo.pieralisi@arm.com; linux-
> kernel@vger.kernel.org; Linuxarm; devicetree@vger.kernel.org; linux-
> pci@vger.kernel.org; linux-serial@vger.kernel.org; minyard@acm.org;
> benh@kernel.crashing.org; liviu.dudau@arm.com; zourongrong@gmail.com;
> John Garry; Gabriele Paoloni; zhichang.yuan02@gmail.com;
> kantyzc@163.com; xuwei (O)
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote:
> > +       /*
> > +        * The first PCIBIOS_MIN_IO is reserved specifically for
> indirectIO.
> > +        * It will separate indirectIO range from pci host bridge to
> > +        * avoid the possible PIO conflict.
> > +        * Set the indirectIO range directly here.
> > +        */
> > +       lpcdev->io_ops.start = 0;
> > +       lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
> > +       lpcdev->io_ops.devpara = lpcdev;
> > +       lpcdev->io_ops.pfin = hisilpc_comm_in;
> > +       lpcdev->io_ops.pfout = hisilpc_comm_out;
> > +       lpcdev->io_ops.pfins = hisilpc_comm_ins;
> > +       lpcdev->io_ops.pfouts = hisilpc_comm_outs;
> 
> I have to look at patch 2 in more detail again, after missing a few
> review
> rounds. I'm still a bit skeptical about hardcoding a logical I/O port
> range here, and would hope that we can just go through the same
> assignment of logical port ranges that we have for PCI buses,
> decoupling
> the bus addresses from the linux-internal ones.

The point here is that we want to avoid any conflict/overlap between
the LPC I/O space and the PCI I/O space. With the assignment above
we make sure that LPC never interfere with PCI I/O space.

Thanks

Gab

> 
> 	Arnd

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

#1518505 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote:
> > On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote:
> > > +       /*
> > > +        * The first PCIBIOS_MIN_IO is reserved specifically for
> > indirectIO.
> > > +        * It will separate indirectIO range from pci host bridge to
> > > +        * avoid the possible PIO conflict.
> > > +        * Set the indirectIO range directly here.
> > > +        */
> > > +       lpcdev->io_ops.start = 0;
> > > +       lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
> > > +       lpcdev->io_ops.devpara = lpcdev;
> > > +       lpcdev->io_ops.pfin = hisilpc_comm_in;
> > > +       lpcdev->io_ops.pfout = hisilpc_comm_out;
> > > +       lpcdev->io_ops.pfins = hisilpc_comm_ins;
> > > +       lpcdev->io_ops.pfouts = hisilpc_comm_outs;
> > 
> > I have to look at patch 2 in more detail again, after missing a few
> > review
> > rounds. I'm still a bit skeptical about hardcoding a logical I/O port
> > range here, and would hope that we can just go through the same
> > assignment of logical port ranges that we have for PCI buses,
> > decoupling
> > the bus addresses from the linux-internal ones.
> 
> The point here is that we want to avoid any conflict/overlap between
> the LPC I/O space and the PCI I/O space. With the assignment above
> we make sure that LPC never interfere with PCI I/O space.

But we already abstract the PCI I/O space using dynamic registration.
There is no need to hardcode the logical address for ISA, though
I think we can hardcode the bus address to start at zero here.

	Arnd

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

#1518711 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Hi, Arnd,

On 2016/11/10 5:34, Arnd Bergmann wrote:
> On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote:
>>> On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote:
>>>> +       /*
>>>> +        * The first PCIBIOS_MIN_IO is reserved specifically for
>>> indirectIO.
>>>> +        * It will separate indirectIO range from pci host bridge to
>>>> +        * avoid the possible PIO conflict.
>>>> +        * Set the indirectIO range directly here.
>>>> +        */
>>>> +       lpcdev->io_ops.start = 0;
>>>> +       lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
>>>> +       lpcdev->io_ops.devpara = lpcdev;
>>>> +       lpcdev->io_ops.pfin = hisilpc_comm_in;
>>>> +       lpcdev->io_ops.pfout = hisilpc_comm_out;
>>>> +       lpcdev->io_ops.pfins = hisilpc_comm_ins;
>>>> +       lpcdev->io_ops.pfouts = hisilpc_comm_outs;
>>>
>>> I have to look at patch 2 in more detail again, after missing a few
>>> review
>>> rounds. I'm still a bit skeptical about hardcoding a logical I/O port
>>> range here, and would hope that we can just go through the same
>>> assignment of logical port ranges that we have for PCI buses,
>>> decoupling
>>> the bus addresses from the linux-internal ones.
>>
>> The point here is that we want to avoid any conflict/overlap between
>> the LPC I/O space and the PCI I/O space. With the assignment above
>> we make sure that LPC never interfere with PCI I/O space.
> 
> But we already abstract the PCI I/O space using dynamic registration.
> There is no need to hardcode the logical address for ISA, though
> I think we can hardcode the bus address to start at zero here.

Do you means that we can pick up the maximal I/O address from all children's
device resources??

Thanks,
Zhichang

> 
> 	Arnd
> 
> .
> 

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

#1518771 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

On Thursday, November 10, 2016 2:40:26 PM CET zhichang.yuan wrote:
> On 2016/11/10 5:34, Arnd Bergmann wrote:
> > On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote:
> >>> On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote:
> >>>> +       /*
> >>>> +        * The first PCIBIOS_MIN_IO is reserved specifically for
> >>> indirectIO.
> >>>> +        * It will separate indirectIO range from pci host bridge to
> >>>> +        * avoid the possible PIO conflict.
> >>>> +        * Set the indirectIO range directly here.
> >>>> +        */
> >>>> +       lpcdev->io_ops.start = 0;
> >>>> +       lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
> >>>> +       lpcdev->io_ops.devpara = lpcdev;
> >>>> +       lpcdev->io_ops.pfin = hisilpc_comm_in;
> >>>> +       lpcdev->io_ops.pfout = hisilpc_comm_out;
> >>>> +       lpcdev->io_ops.pfins = hisilpc_comm_ins;
> >>>> +       lpcdev->io_ops.pfouts = hisilpc_comm_outs;
> >>>
> >>> I have to look at patch 2 in more detail again, after missing a few
> >>> review
> >>> rounds. I'm still a bit skeptical about hardcoding a logical I/O port
> >>> range here, and would hope that we can just go through the same
> >>> assignment of logical port ranges that we have for PCI buses,
> >>> decoupling
> >>> the bus addresses from the linux-internal ones.
> >>
> >> The point here is that we want to avoid any conflict/overlap between
> >> the LPC I/O space and the PCI I/O space. With the assignment above
> >> we make sure that LPC never interfere with PCI I/O space.
> > 
> > But we already abstract the PCI I/O space using dynamic registration.
> > There is no need to hardcode the logical address for ISA, though
> > I think we can hardcode the bus address to start at zero here.
> 
> Do you means that we can pick up the maximal I/O address from all children's
> device resources??

The driver should not look at the resources of its children, just
register a range of addresses dynamically, as I suggested in an
earlier review.


Your current version has

        if (arm64_extio_ops->pfout)                             \
                arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
                       addr, value, sizeof(type));             \

Instead, just subtract the start of the range from the logical
port number to transform it back into a bus-local port number:

        if (arm64_extio_ops->pfout)                             \
                arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
                       addr - arm64_extio_ops->start, value, sizeof(type)); \

We know that the ISA/LPC bus can only have up to 65536 ports,
so you can register all of those, or possibly limit it further to
1024 or 4096 ports, whichever matches the bus implementation.

	Arnd

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

#1518950 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Hi, Arnd,

On 2016/11/10 17:12, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 2:40:26 PM CET zhichang.yuan wrote:
>> On 2016/11/10 5:34, Arnd Bergmann wrote:
>>> On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote:
>>>>> On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote:
>>>>>> +       /*
>>>>>> +        * The first PCIBIOS_MIN_IO is reserved specifically for
>>>>> indirectIO.
>>>>>> +        * It will separate indirectIO range from pci host bridge to
>>>>>> +        * avoid the possible PIO conflict.
>>>>>> +        * Set the indirectIO range directly here.
>>>>>> +        */
>>>>>> +       lpcdev->io_ops.start = 0;
>>>>>> +       lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
>>>>>> +       lpcdev->io_ops.devpara = lpcdev;
>>>>>> +       lpcdev->io_ops.pfin = hisilpc_comm_in;
>>>>>> +       lpcdev->io_ops.pfout = hisilpc_comm_out;
>>>>>> +       lpcdev->io_ops.pfins = hisilpc_comm_ins;
>>>>>> +       lpcdev->io_ops.pfouts = hisilpc_comm_outs;
>>>>>
>>>>> I have to look at patch 2 in more detail again, after missing a few
>>>>> review
>>>>> rounds. I'm still a bit skeptical about hardcoding a logical I/O port
>>>>> range here, and would hope that we can just go through the same
>>>>> assignment of logical port ranges that we have for PCI buses,
>>>>> decoupling
>>>>> the bus addresses from the linux-internal ones.
>>>>
>>>> The point here is that we want to avoid any conflict/overlap between
>>>> the LPC I/O space and the PCI I/O space. With the assignment above
>>>> we make sure that LPC never interfere with PCI I/O space.
>>>
>>> But we already abstract the PCI I/O space using dynamic registration.
>>> There is no need to hardcode the logical address for ISA, though
>>> I think we can hardcode the bus address to start at zero here.
>>
>> Do you means that we can pick up the maximal I/O address from all children's
>> device resources??
> 
> The driver should not look at the resources of its children, just
> register a range of addresses dynamically, as I suggested in an
> earlier review.
>
Sorry! I can't catch your idea yet:(

When to register the I/O range? Is it done just after the successfully
of_translate_address() during the children scanning?

If yes, when a child is scanning, there is no range data in arm64_extio_ops. The
addr_is_indirect_io() calling in of_get_isa_indirect_io() don't need. All we can
check is just whether the address to be translated is IO and is under a parent
device which has no 'ranges' property.

> 
> Your current version has
> 
>         if (arm64_extio_ops->pfout)                             \
>                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>                        addr, value, sizeof(type));             \
> 
> Instead, just subtract the start of the range from the logical
> port number to transform it back into a bus-local port number:
> 
>         if (arm64_extio_ops->pfout)                             \
>                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>                        addr - arm64_extio_ops->start, value, sizeof(type)); \
> 
I think there is some information needed sync.
In the old patch-set, we don't bypass the pci_address_to_pio() after
successfully of_translate_address(). In this way, we don't need to reserve any
PIO space for our LPC since the logical port are from the same mapping
algorithm. Based on this way, the port number in the device resource is logical
one, then we need to subtract the start of the resource to get back the
bus-local port.

From V3, we don't apply the mapping based on pci_address_to_pio(), the
of_translate_address() return the bus-local port directly and store into
relevant device resource. So, in the current arm64_extio_ops->pfout(), the
reverse translation don't need anymore. The input "addr" is bus-local port now.


Thanks,
Zhichang


> We know that the ISA/LPC bus can only have up to 65536 ports,
> so you can register all of those, or possibly limit it further to
> 1024 or 4096 ports, whichever matches the bus implementation.
> 
> 	Arnd
> 
> .
> 

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

#1519064 — RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 10 November 2016 09:12
> To: linux-arm-kernel@lists.infradead.org
> Cc: Yuanzhichang; mark.rutland@arm.com; devicetree@vger.kernel.org;
> lorenzo.pieralisi@arm.com; Gabriele Paoloni; minyard@acm.org; linux-
> pci@vger.kernel.org; benh@kernel.crashing.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; xuwei (O); Linuxarm;
> zourongrong@gmail.com; robh+dt@kernel.org; kantyzc@163.com; linux-
> serial@vger.kernel.org; catalin.marinas@arm.com; olof@lixom.net;
> liviu.dudau@arm.com; bhelgaas@google.com; zhichang.yuan02@gmail.com
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Thursday, November 10, 2016 2:40:26 PM CET zhichang.yuan wrote:
> > On 2016/11/10 5:34, Arnd Bergmann wrote:
> > > On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni
> wrote:
> > >>> On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote:
> > >>>> +       /*
> > >>>> +        * The first PCIBIOS_MIN_IO is reserved specifically for
> > >>> indirectIO.
> > >>>> +        * It will separate indirectIO range from pci host
> bridge to
> > >>>> +        * avoid the possible PIO conflict.
> > >>>> +        * Set the indirectIO range directly here.
> > >>>> +        */
> > >>>> +       lpcdev->io_ops.start = 0;
> > >>>> +       lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
> > >>>> +       lpcdev->io_ops.devpara = lpcdev;
> > >>>> +       lpcdev->io_ops.pfin = hisilpc_comm_in;
> > >>>> +       lpcdev->io_ops.pfout = hisilpc_comm_out;
> > >>>> +       lpcdev->io_ops.pfins = hisilpc_comm_ins;
> > >>>> +       lpcdev->io_ops.pfouts = hisilpc_comm_outs;
> > >>>
> > >>> I have to look at patch 2 in more detail again, after missing a
> few
> > >>> review
> > >>> rounds. I'm still a bit skeptical about hardcoding a logical I/O
> port
> > >>> range here, and would hope that we can just go through the same
> > >>> assignment of logical port ranges that we have for PCI buses,
> > >>> decoupling
> > >>> the bus addresses from the linux-internal ones.
> > >>
> > >> The point here is that we want to avoid any conflict/overlap
> between
> > >> the LPC I/O space and the PCI I/O space. With the assignment above
> > >> we make sure that LPC never interfere with PCI I/O space.
> > >
> > > But we already abstract the PCI I/O space using dynamic
> registration.
> > > There is no need to hardcode the logical address for ISA, though
> > > I think we can hardcode the bus address to start at zero here.
> >
> > Do you means that we can pick up the maximal I/O address from all
> children's
> > device resources??
> 
> The driver should not look at the resources of its children, just
> register a range of addresses dynamically, as I suggested in an
> earlier review.

Where should we get the range from? For LPC we know that it is going
Work on anything that is not used by PCI I/O space, and this is 
why we use [0, PCIBIOS_MIN_IO]

> 
> 
> Your current version has
> 
>         if (arm64_extio_ops->pfout)                             \
>                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>                        addr, value, sizeof(type));             \
> 
> Instead, just subtract the start of the range from the logical
> port number to transform it back into a bus-local port number:

These accessors do not operate on IO tokens:

If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
addr is not going to be an I/O token; in fact patch 2/3 imposes that
the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO
we have free physical addresses that the accessors can operate on.

Thanks

Gab 

> 
>         if (arm64_extio_ops->pfout)                             \
>                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>                        addr - arm64_extio_ops->start, value,
> sizeof(type)); \
> 
> We know that the ISA/LPC bus can only have up to 65536 ports,
> so you can register all of those, or possibly limit it further to
> 1024 or 4096 ports, whichever matches the bus implementation.
> 
> 	Arnd

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

#1519091 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
> 
> Where should we get the range from? For LPC we know that it is going
> Work on anything that is not used by PCI I/O space, and this is 
> why we use [0, PCIBIOS_MIN_IO]

It should be allocated the same way we allocate PCI config space
segments. This is currently done with the io_range list in
drivers/pci/pci.c, which isn't perfect but could be extended
if necessary. Based on what others commented here, I'd rather
make the differences between ISA/LPC and PCI I/O ranges smaller
than larger.

> > Your current version has
> > 
> >         if (arm64_extio_ops->pfout)                             \
> >                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> >                        addr, value, sizeof(type));             \
> > 
> > Instead, just subtract the start of the range from the logical
> > port number to transform it back into a bus-local port number:
> 
> These accessors do not operate on IO tokens:
> 
> If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> addr is not going to be an I/O token; in fact patch 2/3 imposes that
> the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO
> we have free physical addresses that the accessors can operate on.

Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
the logical I/O tokens, the purpose of that macro is really meant
for allocating PCI I/O port numbers within the address space of
one bus.

Note that it's equally likely that whichever next platform needs
non-mapped I/O access like this actually needs them for PCI I/O space,
and that will use it on addresses registered to a PCI host bridge.

If we separate the two steps:

a) assign a range of logical I/O port numbers to a bus
b) register a set of helpers for redirecting logical I/O
   port to a helper function

then I think the code will get cleaner and more flexible.
It should actually then be able to replace the powerpc
specific implementation.

	Arnd

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

#1519666 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Hi, Arnd,

On 2016/11/11 0:07, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
>>
>> Where should we get the range from? For LPC we know that it is going
>> Work on anything that is not used by PCI I/O space, and this is 
>> why we use [0, PCIBIOS_MIN_IO]
> 
> It should be allocated the same way we allocate PCI config space
> segments. This is currently done with the io_range list in
> drivers/pci/pci.c, which isn't perfect but could be extended
> if necessary. Based on what others commented here, I'd rather
> make the differences between ISA/LPC and PCI I/O ranges smaller
> than larger.
> 
>>> Your current version has
>>>
>>>         if (arm64_extio_ops->pfout)                             \
>>>                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>>>                        addr, value, sizeof(type));             \
>>>
>>> Instead, just subtract the start of the range from the logical
>>> port number to transform it back into a bus-local port number:
>>
>> These accessors do not operate on IO tokens:
>>
>> If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
>> addr is not going to be an I/O token; in fact patch 2/3 imposes that
>> the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO
>> we have free physical addresses that the accessors can operate on.
> 
> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> the logical I/O tokens, the purpose of that macro is really meant
> for allocating PCI I/O port numbers within the address space of
> one bus.
> 
> Note that it's equally likely that whichever next platform needs
> non-mapped I/O access like this actually needs them for PCI I/O space,
> and that will use it on addresses registered to a PCI host bridge.
> 
> If we separate the two steps:
> 
> a) assign a range of logical I/O port numbers to a bus
> b) register a set of helpers for redirecting logical I/O
>    port to a helper function
>
It seems that we need to add a new bus and the corresponding resource management
which can also cover current PCI pio mapping, is it right?

Thanks,
Zhichang

> then I think the code will get cleaner and more flexible.
> It should actually then be able to replace the powerpc
> specific implementation.
> 
> 	Arnd
> 
> .
> 

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

#1519685 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Hi Arnd,

On Thu, Nov 10, 2016 at 05:07:21PM +0100, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
> > 
> > Where should we get the range from? For LPC we know that it is going
> > Work on anything that is not used by PCI I/O space, and this is 
> > why we use [0, PCIBIOS_MIN_IO]
> 
> It should be allocated the same way we allocate PCI config space
> segments. This is currently done with the io_range list in
> drivers/pci/pci.c, which isn't perfect but could be extended
> if necessary. Based on what others commented here, I'd rather
> make the differences between ISA/LPC and PCI I/O ranges smaller
> than larger.
> 
> > > Your current version has
> > > 
> > >         if (arm64_extio_ops->pfout)                             \
> > >                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > >                        addr, value, sizeof(type));             \
> > > 
> > > Instead, just subtract the start of the range from the logical
> > > port number to transform it back into a bus-local port number:
> > 
> > These accessors do not operate on IO tokens:
> > 
> > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> > addr is not going to be an I/O token; in fact patch 2/3 imposes that
> > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO
> > we have free physical addresses that the accessors can operate on.
> 
> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> the logical I/O tokens, the purpose of that macro is really meant
> for allocating PCI I/O port numbers within the address space of
> one bus.
> 
> Note that it's equally likely that whichever next platform needs
> non-mapped I/O access like this actually needs them for PCI I/O space,
> and that will use it on addresses registered to a PCI host bridge.
> 
> If we separate the two steps:
> 
> a) assign a range of logical I/O port numbers to a bus

Except that currently when we add ranges to io_range_list we don't have
a bus number yet, because the parsing happens before the host bridge
has been created. Maybe register_io_range() can take a bus number as an
argument, but I'm not sure how we are going to use that in pci_pio_to_address()
or pci_address_to_pio().

Best regards,
Liviu

> b) register a set of helpers for redirecting logical I/O
>    port to a helper function
> 
> then I think the code will get cleaner and more flexible.
> It should actually then be able to replace the powerpc
> specific implementation.
> 
> 	Arnd

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

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

#1519803 — RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 10 November 2016 16:07
> To: Gabriele Paoloni
> Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang;
> mark.rutland@arm.com; devicetree@vger.kernel.org;
> lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org;
> benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux-
> kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com;
> robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org;
> catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com;
> bhelgaas@googl e.com; zhichang.yuan02@gmail.com
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
> >
> > Where should we get the range from? For LPC we know that it is going
> > Work on anything that is not used by PCI I/O space, and this is
> > why we use [0, PCIBIOS_MIN_IO]
> 
> It should be allocated the same way we allocate PCI config space
> segments. This is currently done with the io_range list in
> drivers/pci/pci.c, which isn't perfect but could be extended
> if necessary. Based on what others commented here, I'd rather
> make the differences between ISA/LPC and PCI I/O ranges smaller
> than larger.

I am not sure this would make sense...

IMHO all the mechanism around io_range_list is needed to provide the
"mapping" between I/O tokens and physical CPU addresses.

Currently the available tokens range from 0 to IO_SPACE_LIMIT.

As you know the I/O memory accessors operate on whatever
__of_address_to_resource sets into the resource (start, end).

With this special device in place we cannot know if a resource is
assigned with an I/O token or a physical address, unless we forbid
the I/O tokens to be in a specific range.

So this is why we are changing the offsets of all the functions
handling io_range_list (to make sure that a range is forbidden to
the tokens and is available to the physical addresses).

We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO)
because this is the maximum physical I/O range that a non PCI device
can operate on and because we believe this does not impose much
restriction on the available I/O token range; that now is 
[PCIBIOS_MIN_IO, IO_SPACE_LIMIT].
So we believe that the chosen forbidden range can accommodate
any special ISA bus device with no much constraint on the rest
of I/O tokens...

> 
> > > Your current version has
> > >
> > >         if (arm64_extio_ops->pfout)                             \
> > >                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > >                        addr, value, sizeof(type));             \
> > >
> > > Instead, just subtract the start of the range from the logical
> > > port number to transform it back into a bus-local port number:
> >
> > These accessors do not operate on IO tokens:
> >
> > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> > addr is not going to be an I/O token; in fact patch 2/3 imposes that
> > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to
> PCIBIOS_MIN_IO
> > we have free physical addresses that the accessors can operate on.
> 
> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> the logical I/O tokens, the purpose of that macro is really meant
> for allocating PCI I/O port numbers within the address space of
> one bus.

As I mentioned above, special devices operate on CPU addresses directly,
not I/O tokens. For them there is no way to distinguish....

> 
> Note that it's equally likely that whichever next platform needs
> non-mapped I/O access like this actually needs them for PCI I/O space,
> and that will use it on addresses registered to a PCI host bridge.

Ok so here you are talking about a platform that has got an I/O range
under the PCI host controller, right?
And this I/O range cannot be directly memory mapped but needs special
redirections for the I/O tokens, right?

In this scenario registering the I/O ranges with the forbidden range
implemented by the current patch would still allow to redirect I/O
tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO

So effectively the special PCI host controller
1) knows the physical range that needs special redirection
2) register such range
3) uses pci_pio_to_address() to retrieve the IO tokens for the
   special accessors
4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3)

So to be honest I think this patch can fit well both with
special PCI controllers that need I/O tokens redirection and with
special non-PCI controllers that need non-PCI I/O physical
address redirection...

Thanks (and sorry for the long reply but I didn't know how
to make the explanation shorter :) )

Gab

> 
> If we separate the two steps:
> 
> a) assign a range of logical I/O port numbers to a bus
> b) register a set of helpers for redirecting logical I/O
>    port to a helper function
> 
> then I think the code will get cleaner and more flexible.
> It should actually then be able to replace the powerpc
> specific implementation.
> 
> 	Arnd

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

#1519839 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote:
> Hi Arnd
> 
> > -----Original Message-----
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > Sent: 10 November 2016 16:07
> > To: Gabriele Paoloni
> > Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang;
> > mark.rutland@arm.com; devicetree@vger.kernel.org;
> > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org;
> > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux-
> > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com;
> > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org;
> > catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com;
> > bhelgaas@googl e.com; zhichang.yuan02@gmail.com
> > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> > Hip06
> > 
> > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
> > >
> > > Where should we get the range from? For LPC we know that it is going
> > > Work on anything that is not used by PCI I/O space, and this is
> > > why we use [0, PCIBIOS_MIN_IO]
> > 
> > It should be allocated the same way we allocate PCI config space
> > segments. This is currently done with the io_range list in
> > drivers/pci/pci.c, which isn't perfect but could be extended
> > if necessary. Based on what others commented here, I'd rather
> > make the differences between ISA/LPC and PCI I/O ranges smaller
> > than larger.

Gabriele,

> 
> I am not sure this would make sense...
> 
> IMHO all the mechanism around io_range_list is needed to provide the
> "mapping" between I/O tokens and physical CPU addresses.
> 
> Currently the available tokens range from 0 to IO_SPACE_LIMIT.
> 
> As you know the I/O memory accessors operate on whatever
> __of_address_to_resource sets into the resource (start, end).
> 
> With this special device in place we cannot know if a resource is
> assigned with an I/O token or a physical address, unless we forbid
> the I/O tokens to be in a specific range.
> 
> So this is why we are changing the offsets of all the functions
> handling io_range_list (to make sure that a range is forbidden to
> the tokens and is available to the physical addresses).
> 
> We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO)
> because this is the maximum physical I/O range that a non PCI device
> can operate on and because we believe this does not impose much
> restriction on the available I/O token range; that now is 
> [PCIBIOS_MIN_IO, IO_SPACE_LIMIT].
> So we believe that the chosen forbidden range can accommodate
> any special ISA bus device with no much constraint on the rest
> of I/O tokens...

Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you
actually need another variable for "reserving" an area in the I/O space
that can be used for physical addresses rather than I/O tokens.

The one good example for using PCIBIOS_MIN_IO is when your platform/architecture
does not support legacy ISA operations *at all*. In that case someone
sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O range
so that it doesn't get used. With Zhichang's patch you now start forcing
those platforms to have a valid address below PCIBIOS_MIN_IO.

For the general case you also have to bear in mind that PCIBIOS_MIN_IO could
be zero. In that case, what is your "forbidden" range? [0, 0) ? So it makes
sense to add a new #define that should only be defined by those architectures/
platforms that want to reserve on top of PCIBIOS_MIN_IO another region
where I/O tokens can't be generated for.

Best regards,
Liviu

> 
> > 
> > > > Your current version has
> > > >
> > > >         if (arm64_extio_ops->pfout)                             \
> > > >                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > > >                        addr, value, sizeof(type));             \
> > > >
> > > > Instead, just subtract the start of the range from the logical
> > > > port number to transform it back into a bus-local port number:
> > >
> > > These accessors do not operate on IO tokens:
> > >
> > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> > > addr is not going to be an I/O token; in fact patch 2/3 imposes that
> > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to
> > PCIBIOS_MIN_IO
> > > we have free physical addresses that the accessors can operate on.
> > 
> > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> > the logical I/O tokens, the purpose of that macro is really meant
> > for allocating PCI I/O port numbers within the address space of
> > one bus.
> 
> As I mentioned above, special devices operate on CPU addresses directly,
> not I/O tokens. For them there is no way to distinguish....
> 
> > 
> > Note that it's equally likely that whichever next platform needs
> > non-mapped I/O access like this actually needs them for PCI I/O space,
> > and that will use it on addresses registered to a PCI host bridge.
> 
> Ok so here you are talking about a platform that has got an I/O range
> under the PCI host controller, right?
> And this I/O range cannot be directly memory mapped but needs special
> redirections for the I/O tokens, right?
> 
> In this scenario registering the I/O ranges with the forbidden range
> implemented by the current patch would still allow to redirect I/O
> tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO
> 
> So effectively the special PCI host controller
> 1) knows the physical range that needs special redirection
> 2) register such range
> 3) uses pci_pio_to_address() to retrieve the IO tokens for the
>    special accessors
> 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3)
> 
> So to be honest I think this patch can fit well both with
> special PCI controllers that need I/O tokens redirection and with
> special non-PCI controllers that need non-PCI I/O physical
> address redirection...
> 
> Thanks (and sorry for the long reply but I didn't know how
> to make the explanation shorter :) )
> 
> Gab
> 
> > 
> > If we separate the two steps:
> > 
> > a) assign a range of logical I/O port numbers to a bus
> > b) register a set of helpers for redirecting logical I/O
> >    port to a helper function
> > 
> > then I think the code will get cleaner and more flexible.
> > It should actually then be able to replace the powerpc
> > specific implementation.
> > 
> > 	Arnd

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

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

#1519871 — RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Hi Liviu

> -----Original Message-----
> From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> Sent: 11 November 2016 14:46
> To: Gabriele Paoloni
> Cc: Arnd Bergmann; linux-arm-kernel@lists.infradead.org; Yuanzhichang;
> mark.rutland@arm.com; devicetree@vger.kernel.org;
> lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org;
> benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux-
> kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com;
> robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org;
> catalin.marinas@arm.com; olof@lixom.net; bhelgaas@googl e.com;
> zhichang.yuan02@gmail.com
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote:
> > Hi Arnd
> >
> > > -----Original Message-----
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > Sent: 10 November 2016 16:07
> > > To: Gabriele Paoloni
> > > Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang;
> > > mark.rutland@arm.com; devicetree@vger.kernel.org;
> > > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-
> pci@vger.kernel.org;
> > > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux-
> > > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com;
> > > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org;
> > > catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com;
> > > bhelgaas@googl e.com; zhichang.yuan02@gmail.com
> > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> > > Hip06
> > >
> > > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni
> wrote:
> > > >
> > > > Where should we get the range from? For LPC we know that it is
> going
> > > > Work on anything that is not used by PCI I/O space, and this is
> > > > why we use [0, PCIBIOS_MIN_IO]
> > >
> > > It should be allocated the same way we allocate PCI config space
> > > segments. This is currently done with the io_range list in
> > > drivers/pci/pci.c, which isn't perfect but could be extended
> > > if necessary. Based on what others commented here, I'd rather
> > > make the differences between ISA/LPC and PCI I/O ranges smaller
> > > than larger.
> 
> Gabriele,
> 
> >
> > I am not sure this would make sense...
> >
> > IMHO all the mechanism around io_range_list is needed to provide the
> > "mapping" between I/O tokens and physical CPU addresses.
> >
> > Currently the available tokens range from 0 to IO_SPACE_LIMIT.
> >
> > As you know the I/O memory accessors operate on whatever
> > __of_address_to_resource sets into the resource (start, end).
> >
> > With this special device in place we cannot know if a resource is
> > assigned with an I/O token or a physical address, unless we forbid
> > the I/O tokens to be in a specific range.
> >
> > So this is why we are changing the offsets of all the functions
> > handling io_range_list (to make sure that a range is forbidden to
> > the tokens and is available to the physical addresses).
> >
> > We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO)
> > because this is the maximum physical I/O range that a non PCI device
> > can operate on and because we believe this does not impose much
> > restriction on the available I/O token range; that now is
> > [PCIBIOS_MIN_IO, IO_SPACE_LIMIT].
> > So we believe that the chosen forbidden range can accommodate
> > any special ISA bus device with no much constraint on the rest
> > of I/O tokens...
> 
> Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you
> actually need another variable for "reserving" an area in the I/O space
> that can be used for physical addresses rather than I/O tokens.
> 
> The one good example for using PCIBIOS_MIN_IO is when your
> platform/architecture
> does not support legacy ISA operations *at all*. In that case someone
> sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O range
> so that it doesn't get used. With Zhichang's patch you now start
> forcing
> those platforms to have a valid address below PCIBIOS_MIN_IO.

But if PCIBIOS_MIN_IO is 0 then it means that all I/O space is to be used
by PCI controllers only...so if you have a special bus device using
an I/O range in this case should be a PCI controller...i.e. I would
expect it to fall back into the case of I/O tokens redirection rather than
physical addresses redirection (as mentioned below from my previous reply).
What do you think?

Thanks

Gab


> 
> For the general case you also have to bear in mind that PCIBIOS_MIN_IO
> could
> be zero. In that case, what is your "forbidden" range? [0, 0) ? So it
> makes
> sense to add a new #define that should only be defined by those
> architectures/
> platforms that want to reserve on top of PCIBIOS_MIN_IO another region
> where I/O tokens can't be generated for.
> 
> Best regards,
> Liviu
> 
> >
> > >
> > > > > Your current version has
> > > > >
> > > > >         if (arm64_extio_ops->pfout)
> \
> > > > >                 arm64_extio_ops->pfout(arm64_extio_ops-
> >devpara,\
> > > > >                        addr, value, sizeof(type));
> \
> > > > >
> > > > > Instead, just subtract the start of the range from the logical
> > > > > port number to transform it back into a bus-local port number:
> > > >
> > > > These accessors do not operate on IO tokens:
> > > >
> > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> > > > addr is not going to be an I/O token; in fact patch 2/3 imposes
> that
> > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to
> > > PCIBIOS_MIN_IO
> > > > we have free physical addresses that the accessors can operate
> on.
> > >
> > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer
> to
> > > the logical I/O tokens, the purpose of that macro is really meant
> > > for allocating PCI I/O port numbers within the address space of
> > > one bus.
> >
> > As I mentioned above, special devices operate on CPU addresses
> directly,
> > not I/O tokens. For them there is no way to distinguish....
> >
> > >
> > > Note that it's equally likely that whichever next platform needs
> > > non-mapped I/O access like this actually needs them for PCI I/O
> space,
> > > and that will use it on addresses registered to a PCI host bridge.
> >
> > Ok so here you are talking about a platform that has got an I/O range
> > under the PCI host controller, right?
> > And this I/O range cannot be directly memory mapped but needs special
> > redirections for the I/O tokens, right?
> >
> > In this scenario registering the I/O ranges with the forbidden range
> > implemented by the current patch would still allow to redirect I/O
> > tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO
> >
> > So effectively the special PCI host controller
> > 1) knows the physical range that needs special redirection
> > 2) register such range
> > 3) uses pci_pio_to_address() to retrieve the IO tokens for the
> >    special accessors
> > 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3)
> >
> > So to be honest I think this patch can fit well both with
> > special PCI controllers that need I/O tokens redirection and with
> > special non-PCI controllers that need non-PCI I/O physical
> > address redirection...
> >
> > Thanks (and sorry for the long reply but I didn't know how
> > to make the explanation shorter :) )
> >
> > Gab
> >
> > >
> > > If we separate the two steps:
> > >
> > > a) assign a range of logical I/O port numbers to a bus
> > > b) register a set of helpers for redirecting logical I/O
> > >    port to a helper function
> > >
> > > then I think the code will get cleaner and more flexible.
> > > It should actually then be able to replace the powerpc
> > > specific implementation.
> > >
> > > 	Arnd
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

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

#1519948 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

On Fri, Nov 11, 2016 at 03:53:53PM +0000, Gabriele Paoloni wrote:
> Hi Liviu

Hi Gabriele,

> 
> > -----Original Message-----
> > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> > Sent: 11 November 2016 14:46
> > To: Gabriele Paoloni
> > Cc: Arnd Bergmann; linux-arm-kernel@lists.infradead.org; Yuanzhichang;
> > mark.rutland@arm.com; devicetree@vger.kernel.org;
> > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org;
> > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux-
> > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com;
> > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org;
> > catalin.marinas@arm.com; olof@lixom.net; bhelgaas@googl e.com;
> > zhichang.yuan02@gmail.com
> > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> > Hip06
> > 
> > On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote:
> > > Hi Arnd
> > >
> > > > -----Original Message-----
> > > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > > Sent: 10 November 2016 16:07
> > > > To: Gabriele Paoloni
> > > > Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang;
> > > > mark.rutland@arm.com; devicetree@vger.kernel.org;
> > > > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-
> > pci@vger.kernel.org;
> > > > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux-
> > > > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com;
> > > > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org;
> > > > catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com;
> > > > bhelgaas@googl e.com; zhichang.yuan02@gmail.com
> > > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> > > > Hip06
> > > >
> > > > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni
> > wrote:
> > > > >
> > > > > Where should we get the range from? For LPC we know that it is
> > going
> > > > > Work on anything that is not used by PCI I/O space, and this is
> > > > > why we use [0, PCIBIOS_MIN_IO]
> > > >
> > > > It should be allocated the same way we allocate PCI config space
> > > > segments. This is currently done with the io_range list in
> > > > drivers/pci/pci.c, which isn't perfect but could be extended
> > > > if necessary. Based on what others commented here, I'd rather
> > > > make the differences between ISA/LPC and PCI I/O ranges smaller
> > > > than larger.
> > 
> > Gabriele,
> > 
> > >
> > > I am not sure this would make sense...
> > >
> > > IMHO all the mechanism around io_range_list is needed to provide the
> > > "mapping" between I/O tokens and physical CPU addresses.
> > >
> > > Currently the available tokens range from 0 to IO_SPACE_LIMIT.
> > >
> > > As you know the I/O memory accessors operate on whatever
> > > __of_address_to_resource sets into the resource (start, end).
> > >
> > > With this special device in place we cannot know if a resource is
> > > assigned with an I/O token or a physical address, unless we forbid
> > > the I/O tokens to be in a specific range.
> > >
> > > So this is why we are changing the offsets of all the functions
> > > handling io_range_list (to make sure that a range is forbidden to
> > > the tokens and is available to the physical addresses).
> > >
> > > We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO)
> > > because this is the maximum physical I/O range that a non PCI device
> > > can operate on and because we believe this does not impose much
> > > restriction on the available I/O token range; that now is
> > > [PCIBIOS_MIN_IO, IO_SPACE_LIMIT].
> > > So we believe that the chosen forbidden range can accommodate
> > > any special ISA bus device with no much constraint on the rest
> > > of I/O tokens...
> > 
> > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you
> > actually need another variable for "reserving" an area in the I/O space
> > that can be used for physical addresses rather than I/O tokens.
> > 
> > The one good example for using PCIBIOS_MIN_IO is when your
> > platform/architecture
> > does not support legacy ISA operations *at all*. In that case someone
> > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O range
> > so that it doesn't get used. With Zhichang's patch you now start
> > forcing
> > those platforms to have a valid address below PCIBIOS_MIN_IO.
> 
> But if PCIBIOS_MIN_IO is 0 then it means that all I/O space is to be used
> by PCI controllers only...

Nope, that is not what it means. It means that PCI devices can see I/O addresses
on the bus that start from 0. There never was any usage for non-PCI controllers
when PCIBIOS_MIN_IO != 0. That is what Zhichang is trying to do now and what
I think is not the right thing (and not enough anyway).

> so if you have a special bus device using
> an I/O range in this case should be a PCI controller...

That has always been the case. It is this series that wants to introduce the
new meaning.

> i.e. I would
> expect it to fall back into the case of I/O tokens redirection rather than
> physical addresses redirection (as mentioned below from my previous reply).
> What do you think?

I think you have looked too much at the code *with* Zhichang's patches applied.
Take a step back and look at how PCIBIOS_MIN_IO is used now, before you apply
the patches. It is all about PCI addresses and there is no notion of non-PCI
busses using PCI framework. Only platforms and architectures that try to work
around some legacy standards (ISA) or HW restrictions.

Best regards,
Liviu

> 
> Thanks
> 
> Gab
> 
> 
> > 
> > For the general case you also have to bear in mind that PCIBIOS_MIN_IO
> > could
> > be zero. In that case, what is your "forbidden" range? [0, 0) ? So it
> > makes
> > sense to add a new #define that should only be defined by those
> > architectures/
> > platforms that want to reserve on top of PCIBIOS_MIN_IO another region
> > where I/O tokens can't be generated for.
> > 
> > Best regards,
> > Liviu
> > 
> > >
> > > >
> > > > > > Your current version has
> > > > > >
> > > > > >         if (arm64_extio_ops->pfout)
> > \
> > > > > >                 arm64_extio_ops->pfout(arm64_extio_ops-
> > >devpara,\
> > > > > >                        addr, value, sizeof(type));
> > \
> > > > > >
> > > > > > Instead, just subtract the start of the range from the logical
> > > > > > port number to transform it back into a bus-local port number:
> > > > >
> > > > > These accessors do not operate on IO tokens:
> > > > >
> > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> > > > > addr is not going to be an I/O token; in fact patch 2/3 imposes
> > that
> > > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to
> > > > PCIBIOS_MIN_IO
> > > > > we have free physical addresses that the accessors can operate
> > on.
> > > >
> > > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer
> > to
> > > > the logical I/O tokens, the purpose of that macro is really meant
> > > > for allocating PCI I/O port numbers within the address space of
> > > > one bus.
> > >
> > > As I mentioned above, special devices operate on CPU addresses
> > directly,
> > > not I/O tokens. For them there is no way to distinguish....
> > >
> > > >
> > > > Note that it's equally likely that whichever next platform needs
> > > > non-mapped I/O access like this actually needs them for PCI I/O
> > space,
> > > > and that will use it on addresses registered to a PCI host bridge.
> > >
> > > Ok so here you are talking about a platform that has got an I/O range
> > > under the PCI host controller, right?
> > > And this I/O range cannot be directly memory mapped but needs special
> > > redirections for the I/O tokens, right?
> > >
> > > In this scenario registering the I/O ranges with the forbidden range
> > > implemented by the current patch would still allow to redirect I/O
> > > tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO
> > >
> > > So effectively the special PCI host controller
> > > 1) knows the physical range that needs special redirection
> > > 2) register such range
> > > 3) uses pci_pio_to_address() to retrieve the IO tokens for the
> > >    special accessors
> > > 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3)
> > >
> > > So to be honest I think this patch can fit well both with
> > > special PCI controllers that need I/O tokens redirection and with
> > > special non-PCI controllers that need non-PCI I/O physical
> > > address redirection...
> > >
> > > Thanks (and sorry for the long reply but I didn't know how
> > > to make the explanation shorter :) )
> > >
> > > Gab
> > >
> > > >
> > > > If we separate the two steps:
> > > >
> > > > a) assign a range of logical I/O port numbers to a bus
> > > > b) register a set of helpers for redirecting logical I/O
> > > >    port to a helper function
> > > >
> > > > then I think the code will get cleaner and more flexible.
> > > > It should actually then be able to replace the powerpc
> > > > specific implementation.
> > > >
> > > > 	Arnd
> > 
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯

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

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

#1519912 — Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

Hi, Liviu,


On 11/11/2016 10:45 PM, liviu.dudau@arm.com wrote:
> On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote:
>> Hi Arnd
>>
>>> -----Original Message-----
>>> From: Arnd Bergmann [mailto:arnd@arndb.de]
>>> Sent: 10 November 2016 16:07
>>> To: Gabriele Paoloni
>>> Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang;
>>> mark.rutland@arm.com; devicetree@vger.kernel.org;
>>> lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org;
>>> benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux-
>>> kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com;
>>> robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org;
>>> catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com;
>>> bhelgaas@googl e.com; zhichang.yuan02@gmail.com
>>> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
>>> Hip06
>>>
>>> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
>>>>
>>>> Where should we get the range from? For LPC we know that it is going
>>>> Work on anything that is not used by PCI I/O space, and this is
>>>> why we use [0, PCIBIOS_MIN_IO]
>>>
>>> It should be allocated the same way we allocate PCI config space
>>> segments. This is currently done with the io_range list in
>>> drivers/pci/pci.c, which isn't perfect but could be extended
>>> if necessary. Based on what others commented here, I'd rather
>>> make the differences between ISA/LPC and PCI I/O ranges smaller
>>> than larger.
> 
> Gabriele,
> 
>>
>> I am not sure this would make sense...
>>
>> IMHO all the mechanism around io_range_list is needed to provide the
>> "mapping" between I/O tokens and physical CPU addresses.
>>
>> Currently the available tokens range from 0 to IO_SPACE_LIMIT.
>>
>> As you know the I/O memory accessors operate on whatever
>> __of_address_to_resource sets into the resource (start, end).
>>
>> With this special device in place we cannot know if a resource is
>> assigned with an I/O token or a physical address, unless we forbid
>> the I/O tokens to be in a specific range.
>>
>> So this is why we are changing the offsets of all the functions
>> handling io_range_list (to make sure that a range is forbidden to
>> the tokens and is available to the physical addresses).
>>
>> We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO)
>> because this is the maximum physical I/O range that a non PCI device
>> can operate on and because we believe this does not impose much
>> restriction on the available I/O token range; that now is 
>> [PCIBIOS_MIN_IO, IO_SPACE_LIMIT].
>> So we believe that the chosen forbidden range can accommodate
>> any special ISA bus device with no much constraint on the rest
>> of I/O tokens...
> 
> Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you
> actually need another variable for "reserving" an area in the I/O space
> that can be used for physical addresses rather than I/O tokens.
> 
I think selecting PCIBIOS_MIN_IO as the separator of mapped and non-mapped I/O
range probably is not so reasonable.
PCIBIOS_MIN_IN is specific to PCI devices, it seems as the recommended minimal
start I/O address when assigning the pci device I/O region. It is probably not
defined in some platforms/architectures when no PCI is needed there. That is why
my patch caused some compile error on some archs;

But more important thing is that the PCIBIOS_MIN_IO has different value on
different platforms/architectures. On Arm64, it is 4K currently, but in other
archs, it is not true. And the maximum LPC I/O address should be 64K
theoretically, although for compatible ISA, 2K is enough.
So, It means using PCIBIOS_MIN_IO on arm64 can match our I/O reservation
require. But we can not make this indirectIO work well on other architectures.

I am thinking Arnd's suggestion. But I worry about I haven't completely
understood his idea. What about create a new bus host for LPC/ISA whose I/O
range can be 64KB? This LPC/ISA I/O range works similar to PCI host bridge's I/O
window, all the downstream devices under LPC/ISA should request I/O from that
root resource. But it seems Arnd want this root resource registered dynamically,
I am not sure how to do...

Anyway, if we have this root I/O resource, we don't need any new macro or
variable for the LPC/ISA I/O reservation.

Hope my thought is right.

Best,
Zhichang


> The one good example for using PCIBIOS_MIN_IO is when your platform/architecture
> does not support legacy ISA operations *at all*. In that case someone
> sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O range
> so that it doesn't get used. With Zhichang's patch you now start forcing
> those platforms to have a valid address below PCIBIOS_MIN_IO.
> 
> For the general case you also have to bear in mind that PCIBIOS_MIN_IO could
> be zero. In that case, what is your "forbidden" range? [0, 0) ? So it makes
> sense to add a new #define that should only be defined by those architectures/
> platforms that want to reserve on top of PCIBIOS_MIN_IO another region
> where I/O tokens can't be generated for.
> 
> Best regards,
> Liviu
> 
>>
>>>
>>>>> Your current version has
>>>>>
>>>>>         if (arm64_extio_ops->pfout)                             \
>>>>>                 arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>>>>>                        addr, value, sizeof(type));             \
>>>>>
>>>>> Instead, just subtract the start of the range from the logical
>>>>> port number to transform it back into a bus-local port number:
>>>>
>>>> These accessors do not operate on IO tokens:
>>>>
>>>> If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
>>>> addr is not going to be an I/O token; in fact patch 2/3 imposes that
>>>> the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to
>>> PCIBIOS_MIN_IO
>>>> we have free physical addresses that the accessors can operate on.
>>>
>>> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
>>> the logical I/O tokens, the purpose of that macro is really meant
>>> for allocating PCI I/O port numbers within the address space of
>>> one bus.
>>
>> As I mentioned above, special devices operate on CPU addresses directly,
>> not I/O tokens. For them there is no way to distinguish....
>>
>>>
>>> Note that it's equally likely that whichever next platform needs
>>> non-mapped I/O access like this actually needs them for PCI I/O space,
>>> and that will use it on addresses registered to a PCI host bridge.
>>
>> Ok so here you are talking about a platform that has got an I/O range
>> under the PCI host controller, right?
>> And this I/O range cannot be directly memory mapped but needs special
>> redirections for the I/O tokens, right?
>>
>> In this scenario registering the I/O ranges with the forbidden range
>> implemented by the current patch would still allow to redirect I/O
>> tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO
>>
>> So effectively the special PCI host controller
>> 1) knows the physical range that needs special redirection
>> 2) register such range
>> 3) uses pci_pio_to_address() to retrieve the IO tokens for the
>>    special accessors
>> 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3)
>>
>> So to be honest I think this patch can fit well both with
>> special PCI controllers that need I/O tokens redirection and with
>> special non-PCI controllers that need non-PCI I/O physical
>> address redirection...
>>
>> Thanks (and sorry for the long reply but I didn't know how
>> to make the explanation shorter :) )
>>
>> Gab
>>
>>>
>>> If we separate the two steps:
>>>
>>> a) assign a range of logical I/O port numbers to a bus
>>> b) register a set of helpers for redirecting logical I/O
>>>    port to a helper function
>>>
>>> then I think the code will get cleaner and more flexible.
>>> It should actually then be able to replace the powerpc
>>> specific implementation.
>>>
>>> 	Arnd
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web