[PATCH v3 3/3] PCI: ARM: add support for generic PCI host controller

On Tue, Feb 18, 2014 at 08:15:12PM +0000, Arnd Bergmann wrote:

On Tuesday 18 February 2014 18:44:20 Will Deacon wrote:

quoted

On Tue, Feb 18, 2014 at 06:21:42PM +0000, Jason Gunthorpe wrote:

quoted

On Tue, Feb 18, 2014 at 12:20:43PM +0000, Will Deacon wrote:

quoted

+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,

                                           ^^^^^^^^^^^^^^

This probably shouldn't be 0 in the example, nor in your kvm tool
output. For example purposes any value will do.

Hmm, so kvmtool actually provides a PC ioport at 0x0, which is handy since
there's an 8250 down there. That means we have:

0x0    - 0x6200  : Weird PC stuff
0x6200 - 0x10000 : PCI IO space

Should I just change everything to be offset by 0x6200?

Some more comments about this:

* After I looked at the kvmtool code some more, I think it would be nice to
  actually move the I/O space window to a nonzero location, e.g. between the
  PCI space and the AXI space. It's really confusing the way it is, and easy
  to introduce bugs if you have any code that accidentally relies on the
  numbers matching up. For all I can tell, kvmtool should just work fine
  with any definition of ARM_IOPORT_AREA, as long as you apply the trivial
  patch below.

Well, I think we need a bit more than that otherwise our I/O BARs won't be
initialised correctly.

* pci_get_io_space_block() is a very confusing name for a function that
  allocates a memory space block in kvmtool. It seems that KVM_IOPORT_AREA
  is a misnomer for the same reason. Note that PowerPC sets KVM_IOPORT_AREA
  to zero, but has the I/O space window at SPAPR_PCI_IO_WIN_ADDR, 
  which is non-zero.

Agreed about pci_get_io_space_block being a misnomer, that's probably due
some cleanup. I don't think KVM_IOPORT_AREA is too bad though (it's zero for
PowerPC because I added it and don't have a platform to test on).

* It's interesting that you also support I/O space access to the PCI config
  space through PC-Style ports 0cf8-0cff. Not sure if that helps at all,
  but it is another standard way to probe the bus that could easily be
  implemented as an alternative to CAM and ECAM, in a generic PCI host
  driver.

I won't add that initially, but if somebody needs it then it's easy to
extend what we currently have.

Will