Re: [PATCH] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann <arnd@arndb.de>]

On Monday 03 February 2014 19:06:49 Liviu Dudau wrote:
> On Mon, Feb 03, 2014 at 06:46:10PM +0000, Arnd Bergmann wrote:
> > On Monday 03 February 2014 18:33:48 Liviu Dudau wrote:
> > > +/**
> > > + * pci_host_bridge_of_get_ranges - Parse PCI host bridge resources from DT
> > > + * @dev: device node of the host bridge having the range property
> > > + * @resources: list where the range of resources will be added after DT parsing
> > > + *
> > > + * This function will parse the "ranges" property of a PCI host bridge device
> > > + * node and setup the resource mapping based on its content. It is expected
> > > + * that the property conforms with the Power ePAPR document.
> > > + *
> > > + * Each architecture will then apply their filtering based on the limitations
> > > + * of each platform. One general restriction seems to be the number of IO space
> > > + * ranges, the PCI framework makes intensive use of struct resource management,
> > > + * and for IORESOURCE_IO types they can only be requested if they are contained
> > > + * within the global ioport_resource, so that should be limited to one IO space
> > > + * range.
> >
> > Actually we have quite a different set of restrictions around I/O space on ARM32
> > at the moment: Each host bridge can have its own 64KB range in an arbitrary
> > location on MMIO space, and the total must not exceed 2MB of I/O space.
> 
> And that is why the filtering is not (yet) imposed in the generic code. But once
> you use pci_request_region, that will call request_region which will check
> against ioport_resource as parent for the requested resource. That should fail
> if is is not in the correct range, so I don't know how arm arch code manages
> multiple IO ranges.

Let's try to come up with nomenclature so we can talk about this better

The ioport_resource is in "logical I/O space", which is a Linux fiction,
it goes from 0 to IO_SPACE_LIMIT (2MB on ARM) and is mapped into "virtual
I/O space", which start at (void __iomem *)PCI_IO_VIRT_BASE.

Each PCI domain can have its own "bus I/O aperture", which is typically
between 0x1000 and 0xffff and reflects the address that is used in PCI
transactions and in BARs. The aperture here reflects the subset of the
4GB bus I/O space that is actually mapped into a CPU visible "physical
I/O aperture" using an inbound mapping of the host bridge. The physical
I/O aperture in turn gets mapped to the virtual I/O space using 
pci_ioremap_io. The difference between a bus I/O address and a logical
I/O address is stored in the io_offset.

So much for basic definitions. When a device driver calls pci_request_region,
the port number it sees is the bus I/O port number adjusted using the
io_offset to turn it into a logical I/O port number, which should
always be within the host bridge window, which in turn is a subset
of the ioport_resource.

> > > +static int pci_host_bridge_of_get_ranges(struct device_node *dev,
> > > +                                   struct list_head *resources)
> > > +{
> > > +   struct resource *res;
> > > +   struct of_pci_range range;
> > > +   struct of_pci_range_parser parser;
> > > +   int err;
> > > +
> > > +   pr_info("PCI host bridge %s ranges:\n", dev->full_name);
> > > +
> > > +   /* Check for ranges property */
> > > +   err = of_pci_range_parser_init(&parser, dev);
> > > +   if (err)
> > > +           return err;
> > > +
> > > +   pr_debug("Parsing ranges property...\n");
> > > +   for_each_of_pci_range(&parser, &range) {
> > > +           /* Read next ranges element */
> > > +           pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> > > +                           range.pci_space, range.pci_addr);
> > > +           pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> > > +                                   range.cpu_addr, range.size);
> > > +
> > > +           /* If we failed translation or got a zero-sized region
> > > +            * (some FW try to feed us with non sensical zero sized regions
> > > +            * such as power3 which look like some kind of attempt
> > > +            * at exposing the VGA memory hole) then skip this range
> > > +            */
> > > +           if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> > > +                   continue;
> > > +
> > > +           res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> > > +           if (!res) {
> > > +                   err = -ENOMEM;
> > > +                   goto bridge_ranges_nomem;
> > > +           }
> > > +
> > > +           of_pci_range_to_resource(&range, dev, res);
> > > +
> > > +           pci_add_resource_offset(resources, res,
> > > +                           range.cpu_addr - range.pci_addr);
> > > +   }
> >
> > I believe of_pci_range_to_resource() will return the MMIO aperture for the
> > I/O space window here, which is not what you are supposed to pass into
> > pci_add_resource_offset.
> 
> And that is why the code in probe.c has been added to deal with that. It is
> too early to do the adjustments here as all we have is the list of resources
> and that might get culled by the architecture fixup code. Remembering the
> io_offset will happen once the pci_host_bridge gets created, and the resources
> are then adjusted.

So you want to register an incorrect I/O resource first and then
have it fixed up later, rather than registering the correct
one from the start as everyone else?

> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 6e34498..16febae 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -1787,6 +1787,17 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > >     list_for_each_entry_safe(window, n, resources, list) {
> > >             list_move_tail(&window->list, &bridge->windows);
> > >             res = window->res;
> > > +           /*
> > > +            * IO resources are stored in the kernel with a CPU start
> > > +            * address of zero. Adjust the data accordingly and remember
> > > +            * the offset
> > > +            */
> > > +           if (resource_type(res) == IORESOURCE_IO) {
> > > +                   bridge->io_offset = res->start;
> > > +                   res->end -= res->start;
> > > +                   window->offset -= res->start;
> > > +                   res->start = 0;
> > > +           }
> > >             offset = window->offset;
> > >             if (res->flags & IORESOURCE_BUS)
> >
> > Won't this break all existing host bridges?
> 
> I am not sure. I believe not, due to what I've explained earlier, but you might be right.
> 
> The adjustment happens before the resource is added to the host bridge windows and translates
> it from MMIO range into IO range.

AFAICT, the resource_type of the resource you register above should be
IORESOURCE_MEM, so you are not actually matching it here.

	Arnd