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

On Thursday 13 February 2014 11:04:02 Will Deacon wrote:

Hi Arnd,

Thanks again for the comments.

On Wed, Feb 12, 2014 at 08:59:46PM +0000, Arnd Bergmann wrote:

quoted

On Wednesday 12 February 2014 20:16:11 Will Deacon wrote:

quoted

+- ranges         : As described in IEEE Std 1275-1994, but must provide
+                   at least a definition of one or both of IO and Memory
+                   Space.

I'd say *must* provide at least non-prefetchable memory. *may* provide
prefetchable memory and/or I/O space.

Can do. Should I enforce this in the driver too? (i.e. complain if
non-prefetchable memory is absent).

Yes, good idea.

quoted

quoted

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 47d46c6d8468..491d74c36f6a 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

@@ -33,4 +33,11 @@ config PCI_RCAR_GEN2
 	  There are 3 internal PCI controllers available with a single
 	  built-in EHCI/OHCI host controller present on each one.
 
+config PCI_ARM_GENERIC
+	bool "ARM generic PCI host controller"
+	depends on ARM && OF
+	help
+	  Say Y here if you want to support a simple generic PCI host
+	  controller, such as the one emulated by kvmtool.
+
 endmenu

Looks good for now. In the long run, I'd hope to get rid of the 'ARM'
part here and make it work on any architecture, but that requires
significant work that we should not depend on here.

Agreed. arm64 is the obvious next target (once Liviu's series is sorted out
-- I'm currently using a simplified port of bios32.c for testing).

See also the reply I just sent on the previous thread for a migration
plan regarding the existing drivers.

quoted

quoted

+struct gen_pci_cfg_window {
+	u64					cpu_phys;
+	void __iomem				*base;
+	u8					bus;
+	spinlock_t				lock;
+	const struct gen_pci_cfg_accessors	*accessors;
+};
+
+struct gen_pci_resource {
+	struct list_head			list;
+	struct resource				cpu_res;
+	resource_size_t				offset;
+};

Your gen_pci_resource is actually identical to struct pci_host_bridge_window,
which I guess is coincidence, but it would be nice to actually use
the standard structure to make it easier to integrate with common
infrastructure later.

Ha, at least I managed to come up with the same struct! I'll move to the
generic version.

Hmm, I fear I was speaking too quickly, the pci_host_bridge_window actually
contains a pointer to the resource rather than the resource itself :(

There is probably a way to do this better, at least once we unify the
probe() and setup() functions.

quoted

quoted

+static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
+					      unsigned int devfn,
+					      int where)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+	u32 busn = bus->number;
+
+	spin_lock(&pci->cfg.lock);
+	if (pci->cfg.bus != busn) {
+		resource_size_t offset;
+
+		devm_iounmap(pci->dev, pci->cfg.base);
+		offset = pci->cfg.cpu_phys + (busn << 20);
+		pci->cfg.base = devm_ioremap(pci->dev, offset, SZ_1M);
+		pci->cfg.bus = busn;
+	}
+
+	return pci->cfg.base + ((devfn << 12) | where);
+}

Nice idea, but unfortunately broken: we need config space access from
atomic context, since there are drivers doing that.

That aside, I just took a spin_lock so this needs fixing regardless. The
only solution I can think of then is to map all of the buses at setup time
(making bus_ranges pretty important) and hope that I don't chew through all
of vmalloc.

It's possible we have to go further and only map the buses that are
actually used, rather than the ones that are defined for the bus,
but just mapping the entire range is a reasonable start I think.

quoted

quoted

+
+	hw = (struct hw_pci) {
+		.nr_controllers	= 1,
+		.private_data	= (void **)&pci,
+		.setup		= gen_pci_setup,
+		.map_irq	= of_irq_parse_and_map_pci,
+		.ops		= &gen_pci_ops,
+	};
+
+	pci_common_init_dev(dev, &hw);
+	return 0;
+}

A missing part here seems to be a way to propagate errors from
the pci_common_init_dev or gen_pci_setup back to the caller.

This seems to be a result of the arm pcibios implementation not
being meant for loadable modules, but I suspect it can be fixed.
The nr_controllers>1 logic gets a bit in the way there, but it's
also a model that seems to be getting out of fashion:
kirkwood/dove/orion5x/mv78xx0 use it at the moment, but are
migrating over to the new pci-mvebu code that doesn't as they
get rid of the non-DT probing. pci-rcar-gen2.c uses it, but it
seems they already ran into problems with that and are changing
it. That pretty much leaves iop13xx as the only user, but at
that point we can probably move the loop into iop13xx specific
code.

Makes sense once there are no users of the field.

With the patch I just suggested, we can simply keep
pci_common_init_dev() for older (non-multiplatform) controllers 
and not change them at all but move on to something else for
the interesting ones, i.e. those we want to share with arm64.

	Arnd