Re: [RFC 2/4] PCI: generic: Add support for ARM64 and MSI(x)

[Arnd Bergmann <arnd@arndb.de>]

On Wednesday 01 October 2014 09:46:26 Liviu Dudau wrote:
> On Tue, Sep 30, 2014 at 09:01:14PM +0100, Arnd Bergmann wrote:
> > On Tuesday 30 September 2014 20:54:41 Arnd Bergmann wrote:
> > > On Tuesday 30 September 2014 18:48:21 Liviu Dudau wrote:
> > > > > > > > These are the functions I found that refer to pci_sys_data on arm32:
> > > > > > > > 
> > > > > > > > pcibios_add_bus
> > > > > > > > pcibios_remove_bus
> > > > 
> > > > These are only needed if you want to do per HB processing of the bus
> > > > 
> > > > > > > > pcibios_align_resource
> > > > 
> > > > mvebu is the only user of this function.
> > > > 
> > > > > > > > pci_mmap_page_range
> > > > 
> > > > This is only needed when mapping a PCI resource to userspace. Is that your case here?
> > > > 
> > > > > > > > pci_domain_nr
> > > > > > > > pci_proc_domain
> > > > 
> > > > We have equivalent functionality in the generic patches for those.
> > > > 
> > > 
> > > We clearly don't need those functions for the new drivers, but that's not
> > > the point. The problem is that when you build a kernel that has both
> > > a traditional host bridge driver and a new one in it, you always get those
> > > functions and they get called from the PCI core, with incorrect arguments.
> > 
> > FWIW, the last time we discussed this, I think I had suggested that the
> > functions that are currently architecture specific and have a generic
> > __weak fallback could become function pointers in a per-host structure
> > passed to pci_scan_root_bus, either a new structure or an extended
> > struct pci_ops. Something along these lines:
> 
> Agree to the general idea. But have a look why host drivers need the add_bus ops:
> to add MSI information into the bus!! If we take care of the MSI in the generic
> code there is less of a need for this function at all.


Right, if we can eliminate the need for some or all of the functions above,
we don't have to abstract them any more.

pcibios_remove_bus can just go away entirely, we don't have a single driver
on ARM that implements it. pcibios_add_bus as you say is just used for MSI
at the moment, and we could get rid of it by just moving the msi_chip
reference from pci_bus into pci_host_bridge. 

The arm32 implementations of pci_domain_nr/pci_proc_domain can probably be
removed if we change the arm32 pcibios_init_hw function to call the new
interfaces that set the domain number.

pci_mmap_page_range could either get generalized some more in an attempt
to have a __weak default implementation that works on ARM, or it could
be changed to lose the dependency on pci_sys_data instead. In either
case, the change would involve using the generic pci_host_bridge_window
list.

pcibios_align_resource should probably be per host, and we could move
that into a pointer in pci_host_bridge, something like this:

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index b7c3a5ea1fca..d9cb6c916d54 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -200,11 +200,15 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
 static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 		int resno, resource_size_t size, resource_size_t align)
 {
+	struct pci_host_bridge *host = find_pci_host_bridge(bus);
+	resource_size_t (*alignf)(void *, const struct resource *,
+				  resource_size_t, resource_size_t),
 	struct resource *res = dev->resource + resno;
 	resource_size_t min;
 	int ret;
 
 	min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
+	alignf = host->align_resource ?: pcibios_align_resource;
 
 	/*
 	 * First, try exact prefetching match.  Even if a 64-bit
@@ -215,7 +219,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	 */
 	ret = pci_bus_alloc_resource(bus, res, size, align, min,
 				     IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
-				     pcibios_align_resource, dev);
+				     alignf, dev);
 	if (ret == 0)
 		return 0;
 
@@ -227,7 +231,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	     (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
 		ret = pci_bus_alloc_resource(bus, res, size, align, min,
 					     IORESOURCE_PREFETCH,
-					     pcibios_align_resource, dev);
+					     alignf, dev);
 		if (ret == 0)
 			return 0;
 	}
@@ -240,7 +244,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	 */
 	if (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))
 		ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
-					     pcibios_align_resource, dev);
+					     alignf, dev);
 
 	return ret;
 }


If we decide constantly calling find_pci_host_bridge() is too expensive, we can
be more clever about it.

	Arnd