Date:   Tue, 14 Mar 2017 09:39:15 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     "zhichang.yuan" <yuanzhichang@...ilicon.com>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Rafael Wysocki <rafael@...nel.org>,
        Mark Rutland <mark.rutland@....com>, rjw@...ysocki.net,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linuxarm@...wei.com, devicetree@...r.kernel.org,
        linux-pci <linux-pci@...r.kernel.org>,
        linux-serial@...r.kernel.org, Corey Minyard <minyard@....org>,
        liviu.dudau@....com, Zou Rongrong <zourongrong@...il.com>,
        John Garry <john.garry@...wei.com>,
        Gabriele Paoloni <gabriele.paoloni@...wei.com>,
        zhichang.yuan02@...il.com, kantyzc@....com,
        Wei Xu <xuwei5@...ilicon.com>
Subject: Re: [PATCH V7 0/7] LPC: legacy ISA I/O support

On Mon, Mar 13, 2017 at 3:42 AM, zhichang.yuan
<yuanzhichang@...ilicon.com> wrote:
> This patchset supports the IPMI-bt device attached to the Low-Pin-Count
> interface implemented on Hisilicon Hip06/Hip07 SoC.
>                         -----------
>                         | LPC host|
>                         |         |
>                         -----------
>                              |
>                 _____________V_______________LPC
>                   |                       |
>                   V                       V
>                                      ------------
>                                      |  BT(ipmi)|
>                                      ------------
>
> When master accesses those peripherals beneath the Hip06/Hip07 LPC, a specific
> LPC driver is needed to make LPC host generate the standard LPC I/O cycles with
> the target peripherals'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/Hip07 LPC.
> To solve this issue and keep the relevant existing peripherals' driver
> untouched, this patchset implements:
>   - introduces a generic I/O space management framwork, LIBIO, to support I/O
>     operations of both MMIO buses and the host controllers which access their
>     peripherals with host local I/O addresses;
>   - redefines the in/out accessors to provide unified interfaces for MMIO and
>     legacy I/O. Based on the LIBIO, the calling of in/out() from upper-layer
>     drivers, such as ipmi-si, will be redirected to the corresponding
>     device-specific I/O hooks to perfrom the I/O accesses.
> Based on this patch-set, all the I/O accesses to Hip06/Hip07 LPC peripherals
> can be supported without any changes on the existing ipmi-si driver.

Thanks for reposting this. I have a few high-level comments first, based on
the walk through the code I did with Gabriele and John last week:

- I think the libio framework is more generic than it needs to be, but as
  Alex really liked it this way and it was done like this based on his earlier
  comments, I think that's ok.

- after we went back and forth on the ACPI implementation, we concluded
  that it is correct to do the same as on DT and completely abstract the
  number space for I/O ports. No code should rely on a Linux port number
  to have any particular relation to the physical address or the the address
  on a PCI or LPC bus.

- The name "libio" still needs to be changed, this is way too generic, as
  "I/O" can refer to many things in the kernel, and almost none of them
  are related to x86 programmed I/O ports in any way. My suggestion
  would be "generic_ioport", or possibly "libioport", "libpio" or "pci_io". Any
  of them would work for me, or someone else could come up with a better
  name that describes what it is.

- I'm pretty sure the current implementation is broken for the ioport_map
  function that tries to turn an IORESOURCE_IO number into a pointer.
  Forcing CONFIG_GENERIC_IOMAP on would solve this, but also
  make all MMIO operations slower, which we probably don't want.
  It's probably enough to add a check in ioport_map() to see if the range
  is mapped into a virtual address or not.

- We could simplify the lookup a bit by using the trick from arch/ia64
  of using an array instead of linked list for walking the port numbers.
  There, the upper bits of the port number refer to an address space
  number while the lower bits refer to the bus address within that
  address space. This should work just as well as the current
  implementation but would be a little easier to understand. Maybe
  Bjorn can comment on this too, as I think he was involved with the
  ia64 implementation.

      Arnd

Powered by blists - more mailing lists