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

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

Hi Ard,

Any comments?

Anyway we can modify the code if you insist on using an intermediate CPU IO
address space.

Thanks,

Heyi

On Sat, Mar 31, 2018 at 09:37:47AM +0800, Guo Heyi wrote:
> 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