Date:   Mon, 30 Jan 2017 21:08:59 +0100
From:   Alexander Graf <agraf@...e.de>
To:     "zhichang.yuan" <yuanzhichang@...ilicon.com>,
        catalin.marinas@....com, will.deacon@....com, robh+dt@...nel.org,
        frowand.list@...il.com, bhelgaas@...gle.com, rafael@...nel.org,
        mark.rutland@....com, brian.starkey@....com, olof@...om.net,
        arnd@...db.de, linux-arm-kernel@...ts.infradead.org
Cc:     lorenzo.pieralisi@....com, benh@...nel.crashing.org,
        linux-kernel@...r.kernel.org, linuxarm@...wei.com,
        devicetree@...r.kernel.org, linux-pci@...r.kernel.org,
        linux-serial@...r.kernel.org, minyard@....org, liviu.dudau@....com,
        zourongrong@...il.com, john.garry@...wei.com,
        gabriele.paoloni@...wei.com, zhichang.yuan02@...il.com,
        kantyzc@....com, xuwei5@...ilicon.com
Subject: Re: [PATCH V6 4/5] LPC: Support the device-tree LPC host on
 Hip06/Hip07



On 24/01/2017 08:05, zhichang.yuan wrote:
> The low-pin-count(LPC) interface of Hip06/Hip07 accesses the peripherals in
> I/O port addresses. This patch implements the LPC host controller driver which
> perform the I/O operations on the underlying hardware.
> We don't want to touch those existing peripherals' driver, such as ipmi-bt. So
> this driver applies the indirect-IO introduced in the previous patch after
> registering an indirect-IO node to the indirect-IO devices list which will be
> searched in the I/O accessors.
> As the I/O translations for LPC children depend on the host I/O registration,
> we should ensure the host I/O registration is finished before all the LPC
> children scanning. That is why an arch_init() hook was added in this patch.
>
> Signed-off-by: zhichang.yuan <yuanzhichang@...ilicon.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@...wei.com>
> ---
>  .../arm/hisilicon/hisilicon-low-pin-count.txt      |  33 ++
>  MAINTAINERS                                        |   9 +
>  arch/arm64/boot/dts/hisilicon/hip06-d03.dts        |   4 +
>  arch/arm64/boot/dts/hisilicon/hip06.dtsi           |  14 +
>  drivers/bus/Kconfig                                |   8 +
>  drivers/bus/Makefile                               |   1 +
>  drivers/bus/hisi_lpc.c                             | 599 +++++++++++++++++++++
>  7 files changed, 668 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
>  create mode 100644 drivers/bus/hisi_lpc.c
>
> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> new file mode 100644
> index 0000000..213181f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> @@ -0,0 +1,33 @@
> +Hisilicon Hip06 low-pin-count device
> +  Hisilicon Hip06 SoCs implement a Low Pin Count (LPC) controller, which
> +  provides I/O access to some legacy ISA devices.
> +  Hip06 is based on arm64 architecture where there is no I/O space. So, the
> +  I/O ports here are not cpu addresses, and there is no 'ranges' property in
> +  LPC device node.
> +
> +Required properties:
> +- compatible:  value should be as follows:
> +	(a) "hisilicon,hip06-lpc"
> +	(b) "hisilicon,hip07-lpc"
> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> +- reg: base memory range where the LPC register set is mapped.
> +
> +Note:
> +  The node name before '@' must be "isa" to represent the binding stick to the
> +  ISA/EISA binding specification.
> +
> +Example:
> +
> +isa@...b0000 {
> +	compatible = "hisilicon,hip06-lpc";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	reg = <0x0 0xa01b0000 0x0 0x1000>;
> +
> +	ipmi0: bt@e4 {
> +		compatible = "ipmi-bt";
> +		device_type = "ipmi";
> +		reg = <0x01 0xe4 0x04>;
> +	};
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 26edd83..0153707 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5855,6 +5855,15 @@ F:	include/uapi/linux/if_hippi.h
>  F:	net/802/hippi.c
>  F:	drivers/net/hippi/
>
> +HISILICON LPC BUS DRIVER
> +M:	Zhichang Yuan <yuanzhichang@...ilicon.com>
> +L:	linux-arm-kernel@...ts.infradead.org
> +W:	http://www.hisilicon.com
> +S:	Maintained
> +F:	drivers/bus/hisi_lpc.c
> +F:	lib/extio.c
> +F:	Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> +
>  HISILICON NETWORK SUBSYSTEM DRIVER
>  M:	Yisen Zhuang <yisen.zhuang@...wei.com>
>  M:	Salil Mehta <salil.mehta@...wei.com>
> diff --git a/arch/arm64/boot/dts/hisilicon/hip06-d03.dts b/arch/arm64/boot/dts/hisilicon/hip06-d03.dts
> index 7c4114a..75b2b5c 100644
> --- a/arch/arm64/boot/dts/hisilicon/hip06-d03.dts
> +++ b/arch/arm64/boot/dts/hisilicon/hip06-d03.dts
> @@ -52,3 +52,7 @@
>  &usb_ehci {
>  	status = "ok";
>  };
> +
> +&ipmi0 {
> +	status = "ok";
> +};
> diff --git a/arch/arm64/boot/dts/hisilicon/hip06.dtsi b/arch/arm64/boot/dts/hisilicon/hip06.dtsi
> index a049b64..c450f8d 100644
> --- a/arch/arm64/boot/dts/hisilicon/hip06.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hip06.dtsi
> @@ -318,6 +318,20 @@
>  		#size-cells = <2>;
>  		ranges;
>
> +		isa@...b0000 {
> +			compatible = "hisilicon,hip06-lpc";
> +			#size-cells = <1>;
> +			#address-cells = <2>;
> +			reg = <0x0 0xa01b0000 0x0 0x1000>;
> +
> +			ipmi0: bt@e4 {
> +				compatible = "ipmi-bt";
> +				device_type = "ipmi";
> +				reg = <0x01 0xe4 0x04>;
> +				status = "disabled";
> +			};
> +		};
> +
>  		refclk: refclk {
>  			compatible = "fixed-clock";
>  			clock-frequency = <50000000>;
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index b9e8cfc..58cee84 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"

It's not a workaround, it's support. Better word it like

   "Support for ISA I/O space on Hisilicon HIP0X"

> +	depends on (ARM64 && ARCH_HISI && PCI) || COMPILE_TEST
> +	select 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 cc6364b..28e3862 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..a96e384
> --- /dev/null
> +++ b/drivers/bus/hisi_lpc.c
> @@ -0,0 +1,599 @@
> +/*
> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@...ilicon.com>
> + * Author: Zou Rongrong <zourongrong@...wei.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 a
> + * different port address:
> + * 0 means repeatedly IO operations will stick on the same port,
> + * such as BT;
> + */
> +#define FG_INCRADDR_LPC		0x02
> +
> +struct lpc_cycle_para {
> +	unsigned int opflags;
> +	unsigned int csize; /* the data length of each operation */
> +};
> +
> +struct hisilpc_dev {
> +	spinlock_t cycle_lock;
> +	void __iomem  *membase;
> +	struct extio_node *extio;
> +};
> +
> +/* bounds of the LPC bus address range */
> +#define LPC_MIN_BUS_RANGE	0x0
> +
> +/*
> + * The maximal IO size for each leagcy bus.

legacy?

I don't really understand why this bus is legacy though. It looks like a 
simple MMIO-to-LPC bridge to me.

> + * The port size of legacy I/O devices is normally less than 0x400.
> + * Defining the I/O range size as 0x400 here should be sufficient for
> + * all peripherals under one bus.
> + */

This comment doesn't make a lot of sense. What is the limit? Is there a 
hardware limit?

We don't dynamically allocate devices on the lpc bus, so why imply a 
limit at all?

> +#define LPC_BUS_IO_SIZE		0x400
> +
> +/* The maximum continuous 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.

Are you sure you want this comment to be upstream? :)

> + */
> +#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 specific for single IO operation, such as inb. */
> +#define LPC_PEROP_WAITCNT	100
> +
> +
> +static inline int wait_lpc_idle(unsigned char *mbase,

No need to specify inline.

> +				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);

It's a shame we have to busy loop, but I guess no calling code outside 
is prepared for rescheduling at this point.

> +	}
> +	return -ETIME;
> +}
> +
> +/*
> + * hisilpc_target_in - trigger a series of lpc cycles to read required data
> + *		       from target peripheral.
> + * @pdev: pointer to hisi lpc device
> + * @para: some parameters 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);

Ouch. This is going to kill your RT jitter. Is there no better way?

> +
> +		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 peripheral.
> + * @pdev: pointer to hisi lpc device
> + * @para: some parameters 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);

Same thing here

> +
> +		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;
> +}
> +
> +static inline unsigned long

Don't explicitly mention inline, the compiler will figure that out for you.

> +hisi_lpc_pio_to_addr(struct hisilpc_dev *lpcdev, unsigned long pio)
> +{
> +	return pio - lpcdev->extio->io_start + lpcdev->extio->bus_start;
> +}
> +
> +
> +/**
> + * hisilpc_comm_in - read/input the data from the I/O peripheral
> + *		     through LPC.
> + * @devobj: pointer to the device information relevant to LPC controller.
> + * @pio: 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 pio, size_t dlen)
> +{
> +	struct hisilpc_dev *lpcdev = devobj;
> +	struct lpc_cycle_para iopara;
> +	u32 rd_data;

rd_data needs to be initialized to 0. Otherwise it may contain stale 
stack contents and corrupt non-32bit dlen returns.

> +	unsigned char *newbuf;
> +	int ret = 0;
> +	unsigned long ptaddr;
> +
> +	if (!lpcdev || !dlen || dlen > LPC_MAX_DULEN ||	(dlen & (dlen - 1)))
> +		return -1;

Isn't this -EINVAL?

> +
> +	/* the local buffer must be enough for one data unit */
> +	if (sizeof(rd_data) < dlen)
> +		return -1;

Same here.

Also, the above seems a very convoluted way of saying

   switch (dlen) {
   case 1:
   case 2:
   case 4:
     break;
   default:
     return -EINVAL;
   }

But I guess the way you write it doesn't hurt ;)


Alex

Powered by blists - more mailing lists