Re: [PATCH edk2-platforms 00/12] Hisilicon/D0x: Switch to generic PciHostBridge

[Guo Heyi <heyi.guo@linaro.org>]

Hi Ard,

Thanks for your time of reviewing the patches.
Please see my opinions below.

On Fri, Mar 30, 2018 at 05:40:20PM +0200, Ard Biesheuvel wrote:
> On 29 March 2018 at 02:20, Guo Heyi <heyi.guo@linaro.org> wrote:
> > On Wed, Mar 28, 2018 at 10:43:41AM +0100, Ard Biesheuvel wrote:
> >> On 28 March 2018 at 02:05, Guo Heyi <heyi.guo@linaro.org> wrote:
> >> > Hi Leif, Ard,
> >> >
> >> > Any comments for this series of patches?
> >> >
> >>
> >> Hello Heyi,
> >>
> >> Thanks for sending these patches. Leif is at the plugfest, but I will
> >> look at these before the end of the week.
> >
> > Forgot the plugfest as I am not attending :)
> 
> Hello Heyi,
> 
> I think the series looks mostly fine in general, but there are two
> things that I'd like you to change (as noted in my replies):
> - please split the PCI to CPU I/O translation from the CPU I/O to CPU
> MMIO translation

I heard that OS did the same as you indicated, but the reasons of why I
translated IO address into memory address in PCI host bridge are like below:

1. If we add an intermediate level of "CPU IO address space", it makes things a
little more complicated but I don't see any real benefit. If we just make a
simple policy that on ARM/AARCH64 CPU IO address is equal to CPU memory address,
we can even use a unified CPU IO driver for all ARM/AARCH64 platforms, while the
translation is covered by PCI host bridge driver.

2. From hardware perspective, the translation is done by PCIe ATU; for IO bar
access, the address from CPU to ATU is a CPU memory address, not an intermediate
CPU IO address; the intermediate CPU IO address is a totally logical concept,
and it also splits ATU function into separate drivers (CPU IO protocol driver
also sees part of ATU function).

Please let me know if my understanding is not right.

> - please fix the APPETURE spelling (in whichever way is most
> convenient for you: separate patch at the beginning, at the end, etc
> etc)

Nice catch :) will fix that.

Thanks,

Heyi