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
> <yuanzhichang@xxxxxxxxxxxxx> 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.
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
>
> .
>
