lkml.org 
[lkml]   [2017]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH V6 4/5] LPC: Support the device-tree LPC host on Hip06/Hip07
    From
    Date
    On 30/01/2017 20:08, Alexander Graf wrote:
    >

    Alex,

    Thanks for checking.

    >
    > 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@hisilicon.com>
    >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.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@a01b0000 {
    >> + compatible = "hisilicon,hip06-lpc";
    >> + #address-cells = <2>;
    >> + #size-cells = <1>;
    >> + reg = <0x0 0xa01b0000 0x0 0x1000>;
    >> +
    >> + ipmi0: bt@e4 {
    >> + compatible = "ipmi-bt";
    >> + device_type = "ipmi";
    >> + reg = <0x01 0xe4 0x04>;
    >> + };
    >> +};
    >> diff --git a/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@hisilicon.com>
    >> +L: linux-arm-kernel@lists.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@huawei.com>
    >> M: Salil Mehta <salil.mehta@huawei.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@a01b0000 {
    >> + 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"
    >

    Agreed

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

    I think that he means legacy ISA.

    >> + * 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?
    >

    IIRC from previously asking Zhichang this before, this is the upper
    range we can address devices on the LPC bus. But the value was 0x1000 then.

    >> +#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? :)
    >

    We will remove or refine

    >> + */
    >> +#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.
    >

    Agreed

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

    Obviously the bus register driving is non-atomic, so we need some way to
    lock out.

    I think that it is not so critical for low-speed/infrequent-access bus.

    If we were going to use virtual UART in the BMC on the LPC bus then we
    could consider more.

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

    As above

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

    Sure

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

    I think so, since we read into this value byte-by-byte. We also seem to
    return a 32b value but should return 64b value according to the prototype.

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

    Not sure. This value is returned directly to the inb/outb caller, which
    would not check this value for error.

    It could be argued that the checking is paranoia. If not, we should
    treat the failure as a more severe event.

    >
    >> +
    >> + /* 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:

    It's better

    > break;
    > default:
    > return -EINVAL;
    > }
    >
    > But I guess the way you write it doesn't hurt ;)
    >
    >
    > Alex
    >
    > .
    >


    \
     
     \ /
      Last update: 2017-01-31 11:13    [W:4.816 / U:0.804 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site