Re: [PATCH V6 4/5] LPC: Support the device-tree LPC host on Hip06/Hip07

[John Garry]

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@xxxxxxxxxxxxx>
> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxxxx>
> > +L:    linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > +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@xxxxxxxxxx>
> >  M:    Salil Mehta <salil.mehta@xxxxxxxxxx>
> > 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@xxxxxxxxxxxxx>
> > + * Author: Zou Rongrong <zourongrong@xxxxxxxxxx>
> > + *
> > + * 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
>
> .