Hi, Arnd,

Many thanks for your review!

On 2017/3/14 16:39, Arnd Bergmann wrote:

On Mon, Mar 13, 2017 at 3:42 AM, zhichang.yuan
[off-list ref] wrote:

quoted

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.

Thanks again for your helps in Linaro Connect!
I think we are heading for this direction, is it?

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

Ok. We will make a better name:)

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

Yes, I think our LIBIO will break the ioport_map() at this moment.
I try to solve this issue. Could you help to check the following ideas?
I am not deeper understanding the whole I/O framework, the following maybe not correct:(

ioport_map seems architecture-dependent. For our LIBIO, we don't want to replace the existing I/O
frameworks which support MMIO at this moment. Can we add these two revise to solve this issue?
1) Make LIBIO only target for non GENERIC_IOMAP platforms

config LIBIO
        bool "Generic logical IO management"
        depends on !GENERIC_IOMAP
        def_bool y if PCI && (ARC || MN10300 || UNICORE32 || SPARC || MICROBLAZE || S390 || AVR32 || CRIS || BLACKFIN || XTENSA || ARM64)

2) Modify the ioport_map() defined in asm-generic/io.h
Add the checks to identify the input 'port' is MMIO, otherwise, return NULL;

Then is it enough to avoid the negative effect on the existing I/O framework?

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

Yes, It will be more efficient.

But the issue remained here is still how to coexist with the existing I/O frameworks.
I will continue to look into.

Thanks,
Zhichang

      Arnd

.