Re: [PATCH v1 00/25] PCI: Request host bridge window resources

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Jun 07, 2016 at 10:21:36AM +0200, Arnd Bergmann wrote:
> On Monday, June 6, 2016 6:04:44 PM CEST Bjorn Helgaas wrote:
> > Several host bridge drivers (designware and all derivatives, iproc,
> > xgene, xilinx, and xilinx-nwl) don't request the MMIO and I/O port
> > windows they forward downstream to the PCI bus.
> > 
> > That means the PCI core can't request resources for PCI bridge
> > windows and PCI BARs.
> > 
> > Several other drivers (altera, generic, mvebu, rcar, tegra) do request
> > the windows, but use some duplicated code to do it.
> > 
> > This adds a new devm_request_pci_bus_resources() interface and changes
> > these drivers to use it.  It also fixes several error paths where we failed
> > to free the resource list allocated by of_pci_get_host_bridge_resources().
> > 
> > Tegra guys, please take a look at "PCI: tegra: Remove top-level resource
> > from hierarchy" in particular.  Removing the top-level resource definitely
> > makes /proc/iomem look uglier (although it will look more like that of
> > other drivers).  A short-term fix could be to include device information in
> > the resource name.  I think a better long-term fix would be to make the DT
> > or platform device core request all the resources from the DT.
> > 
> > Comments welcome.  I expect we'll trip over something here, so I marked
> > this "v1" and I don't plan to put it into -next for a while.
> > 
> > This is on my pci/host-request-windows branch, which you can pull or view
> > at https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-request-windows
> 
> This looks very nice. There is one related aspect that I have been
> grumbling about for a while, but I don't know what the driver is
> actually supposed to do there:
> 
> For the IORESOURCE_IO resources, some drivers request the MMIO address
> that the window is mapped into, some drivers request the PIO range, and
> some of them request both. I also believe the resource that gets put
> into the bridge resources list is not always the same one (or maybe
> that got fixed by now).
> 
> What do you think is the correct behavior here, should the driver only
> request the PIO range with parent=ioport_resource, or should it also
> request the MMIO window for the I/O ports with parent=iomem_resource?
> In the latter case, any idea how that can be generalized?

I think it should request both because I think iomem_resource should
contain everything in the memory map.  This would be required if we ever
did any significant reassignment of top-level devices, e.g., ACPI devices.
For example, on ia64, we do this:

  /proc/ioports:
  00000000-00003fff : PCI Bus 0000:00
  00004000-00009fff : PCI Bus 0000:80
  0000a000-0000bfff : PCI Bus 0000:a0
  0000c000-0000ffff : PCI Bus 0000:c0

  /proc/iomem:
  80000000-9fffffff : PCI Bus 0000:00
  a0000000-cfffffff : PCI Bus 0000:80
  d0000000-dfffffff : PCI Bus 0000:a0
  e0000000-fdffffff : PCI Bus 0000:c0
  80004000000-80103fffffe : PCI Bus 0000:00
  c0004000000-c0103fffffe : PCI Bus 0000:80
  d0004000000-d0103fffffe : PCI Bus 0000:a0
  e0004000000-e0103fffffe : PCI Bus 0000:c0
  3fffffc000000-3fffffcffffff : PCI Bus 0000:00 I/O Ports 00000000-00003fff
  3fffffd000000-3fffffe7fffff : PCI Bus 0000:80 I/O Ports 00004000-00009fff
  3fffffe800000-3fffffeffffff : PCI Bus 0000:a0 I/O Ports 0000a000-0000bfff
  3ffffff000000-3ffffffffffff : PCI Bus 0000:c0 I/O Ports 0000c000-0000ffff

> Another aspect is that we already have the
> gen_pci_parse_request_of_pci_ranges() function that does the same as your
> new devm_request_pci_bus_resources() and then a few other things. I
> have been wondering whether we could move that function into common
> code convert drivers to use that wherever possible, but I guess we can
> always do that as a follow-up after this series.

Oh, I didn't notice that; thanks for pointing it out.  That should be
consolidated somehow.  It also checks to be sure there is a
non-prefetchable memory resource.  A few other drivers also do that, but
most don't.  I suppose that will mostly catch DT errors.

Bjorn