lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 14 Feb 2017 14:29:14 +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 13/02/2017 15:39, zhichang.yuan wrote:
> Hi, Alex,
>
> Thanks for your review!
>
> John had replied most of your comments, I only mentioned those which haven't made clear.
>
>
> On 2017/1/31 4:08, Alexander Graf wrote:
>>
>>
>> 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.
>
> yes. The LPC bus is MMIO-to-LPC bridge.
> My comment is not clear.
>
> How about "The maximal IO size of LPC bus"?

That sounds better, but doesn't really tell me what it's about yet ;).

>
>>
>>> + * 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?
>
> The limit here is to define an IO window for LPC.
> It is not acceptable to describe the IO window with 'ranges' property in LPC host node, so we choose a fixable upper IO limit
> for the static LPC IO space. Without this, LPC can't register its IO space through extio helpers.
>
> Why 0x400 is chosen? For our LPC, 0x400 cover all the peripherals under the LPC.

This is information that should come from firmware then. Whether by 
coincidence addresses 0x0 - 0x400 include all devices is an 
implementation detail that the driver shouldn't have to know about.

That said, as mentioned elsewhere, we don't need to define single 
windows at all. Instead, can't we just do it like PCI BARs and declare 
all PIO regions we get from firmware as readily allocated? Then we only 
need to explicitly assign ports 0x80, 0x81, 0x99, etc to the LPC bridge 
and leave the rest for grabs (by PCI hotplug for example)

>
>
>
>
>>
>>> +#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?
>
> I think there is no other choices. We have to ensure the I/O cycles triggered in serial mode....

As nobody else accesses the LPC device in the meantime thanks to the 
lock, you don't need to disable interrupts, right?

IIRC in PREEMPT_RT, a normal spinlock becomes a mutex and allows for 
resched during the LPC transaction.


Alex

Powered by blists - more mailing lists