[RESEND PATCH 0/3] Support for Generic PCI Host Controller

[RESEND PATCH 0/3] Support for Generic PCI Host Controller

[Will Deacon]

Hi all,

This is a resend of the generic PCI host controller patch I last posted
in March:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/237540.html

I've also included two additional patches to enable PCI for mach-virt
and add a MAINTAINERS entry for the new driver (Arnd -- I'm happy to add
you here as well if you like).

I've tested successfully on a 3.15-rc3-based kernel with kvmtool and would
really like to see this merged for 3.16.

All feedback welcome,

Will

Will Deacon (3):
  ARM: mach-virt: allow PCI support to be selected
  PCI: ARM: add support for generic PCI host controller
  MAINTAINERS: add entry for generic PCI host controller driver

 .../devicetree/bindings/pci/host-generic-pci.txt   | 100 ++++++
 MAINTAINERS                                        |   8 +
 arch/arm/Kconfig                                   |   1 +
 drivers/pci/host/Kconfig                           |   7 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-host-generic.c                | 369 +++++++++++++++++++++
 6 files changed, 486 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/host-generic-pci.txt
 create mode 100644 drivers/pci/host/pci-host-generic.c

-- 
1.9.2

[RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected

[Will Deacon]

mach-virt can make use of virtio-pci devices, which requires the guest
kernel to have PCI support selected.

This patch selects CONFIG_MIGHT_HAVE_PCI when CONFIG_ARCH_VIRT=y.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index db3c5414223e..f24356c4d630 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -941,6 +941,7 @@ config ARCH_VIRT
 	select ARM_GIC
 	select ARM_PSCI
 	select HAVE_ARM_ARCH_TIMER
+	select MIGHT_HAVE_PCI
 
 #
 # This is sorted alphabetically by mach-* pathname.  However, plat-*
-- 
1.9.2

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

[Will Deacon]

This patch adds support for a generic PCI host controller, such as a
firmware-initialised device with static windows or an emulation by
something such as kvmtool.

The controller itself has no configuration registers and has its address
spaces described entirely by the device-tree (using the bindings from
ePAPR). Both CAM and ECAM are supported for Config Space accesses.

Corresponding documentation is added for the DT binding.

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 .../devicetree/bindings/pci/host-generic-pci.txt   | 100 ++++++
 drivers/pci/host/Kconfig                           |   7 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-host-generic.c                | 369 +++++++++++++++++++++
 4 files changed, 477 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/host-generic-pci.txt
 create mode 100644 drivers/pci/host/pci-host-generic.c

diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
new file mode 100644
index 000000000000..f0b0436807b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
@@ -0,0 +1,100 @@
+* Generic PCI host controller
+
+Firmware-initialised PCI host controllers and PCI emulations, such as the
+virtio-pci implementations found in kvmtool and other para-virtualised
+systems, do not require driver support for complexities such as regulator
+and clock management. In fact, the controller may not even require the
+configuration of a control interface by the operating system, instead
+presenting a set of fixed windows describing a subset of IO, Memory and
+Configuration Spaces.
+
+Such a controller can be described purely in terms of the standardized device
+tree bindings communicated in pci.txt:
+
+
+Properties of the host controller node:
+
+- compatible     : Must be "pci-host-cam-generic" or "pci-host-ecam-generic"
+                   depending on the layout of configuration space (CAM vs
+                   ECAM respectively).
+
+- device_type    : Must be "pci".
+
+- ranges         : As described in IEEE Std 1275-1994, but must provide
+                   at least a definition of non-prefetchable memory. One
+                   or both of prefetchable Memory and IO Space may also
+                   be provided.
+
+- bus-range      : Optional property (also described in IEEE Std 1275-1994)
+                   to indicate the range of bus numbers for this controller.
+                   If absent, defaults to <0 255> (i.e. all buses).
+
+- #address-cells : Must be 3.
+
+- #size-cells    : Must be 2.
+
+- reg            : The Configuration Space base address and size, as accessed
+                   from the parent bus.
+
+
+Properties of the /chosen node:
+
+- linux,pci-probe-only
+                 : Optional property which takes a single-cell argument.
+                   If '0', then Linux will assign devices in its usual manner,
+                   otherwise it will not try to assign devices and instead use
+                   them as they are configured already.
+
+Configuration Space is assumed to be memory-mapped (as opposed to being
+accessed via an ioport) and laid out with a direct correspondence to the
+geography of a PCI bus address by concatenating the various components to
+form an offset.
+
+For CAM, this 24-bit offset is:
+
+        cfg_offset(bus, device, function, register) =
+                   bus << 16 | device << 11 | function << 8 | register
+
+Whilst ECAM extends this by 4 bits to accomodate 4k of function space:
+
+        cfg_offset(bus, device, function, register) =
+                   bus << 20 | device << 15 | function << 12 | register
+
+Interrupt mapping is exactly as described in `Open Firmware Recommended
+Practice: Interrupt Mapping' and requires the following properties:
+
+- #interrupt-cells   : Must be 1
+
+- interrupt-map      : <see aforementioned specification>
+
+- interrupt-map-mask : <see aforementioned specification>
+
+
+Example:
+
+pci {
+    compatible = "pci-host-cam-generic"
+    device_type = "pci";
+    #address-cells = <3>;
+    #size-cells = <2>;
+    bus-range = <0x0 0x1>;
+
+    // CPU_PHYSICAL(2)  SIZE(2)
+    reg = <0x0 0x40000000  0x0 0x1000000>;
+
+    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
+    ranges = <0x01000000 0x0 0x01000000  0x0 0x01000000  0x0 0x00010000>,
+             <0x02000000 0x0 0x41000000  0x0 0x41000000  0x0 0x3f000000>;
+
+
+    #interrupt-cells = <0x1>;
+
+    // PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(3)
+    interrupt-map = <  0x0 0x0 0x0  0x1  &gic  0x0 0x4 0x1
+                     0x800 0x0 0x0  0x1  &gic  0x0 0x5 0x1
+                    0x1000 0x0 0x0  0x1  &gic  0x0 0x6 0x1
+                    0x1800 0x0 0x0  0x1  &gic  0x0 0x7 0x1>;
+
+    // PCI_DEVICE(3)  INT#(1)
+    interrupt-map-mask = <0xf800 0x0 0x0  0x7>;
+}
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index a6f67ec8882f..32d446effbb3 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_HOST_GENERIC
+	bool "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
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 13fb3333aa05..bd1bf1ab4ac8 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
 obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
 obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
 obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
+obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
new file mode 100644
index 000000000000..b761f3e64a6a
--- /dev/null
+++ b/drivers/pci/host/pci-host-generic.c
@@ -0,0 +1,369 @@
+/*
+ * Simple, generic PCI host controller driver targetting firmware-initialised
+ * systems and virtual machines (e.g. the PCI emulation provided by kvmtool).
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (C) 2014 ARM Limited
+ *
+ * Author: Will Deacon <will.deacon@arm.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/platform_device.h>
+
+struct gen_pci_cfg_bus_ops {
+	u32 bus_shift;
+	void __iomem *(*map_bus)(struct pci_bus *, unsigned int, int);
+};
+
+struct gen_pci_cfg_windows {
+	struct resource				res;
+	struct resource				bus_range;
+	void __iomem				**win;
+
+	const struct gen_pci_cfg_bus_ops	*ops;
+};
+
+struct gen_pci {
+	struct pci_host_bridge			host;
+	struct gen_pci_cfg_windows		cfg;
+};
+
+static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
+					     unsigned int devfn,
+					     int where)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+	resource_size_t idx = bus->number - pci->cfg.bus_range.start;
+
+	return pci->cfg.win[idx] + ((devfn << 8) | where);
+}
+
+static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = {
+	.bus_shift	= 16,
+	.map_bus	= gen_pci_map_cfg_bus_cam,
+};
+
+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;
+	resource_size_t idx = bus->number - pci->cfg.bus_range.start;
+
+	return pci->cfg.win[idx] + ((devfn << 12) | where);
+}
+
+static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = {
+	.bus_shift	= 20,
+	.map_bus	= gen_pci_map_cfg_bus_ecam,
+};
+
+static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn,
+				int where, int size, u32 *val)
+{
+	void __iomem *addr;
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+
+	addr = pci->cfg.ops->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		*val = readb(addr);
+		break;
+	case 2:
+		*val = readw(addr);
+		break;
+	default:
+		*val = readl(addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
+				 int where, int size, u32 val)
+{
+	void __iomem *addr;
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+
+	addr = pci->cfg.ops->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		writeb(val, addr);
+		break;
+	case 2:
+		writew(val, addr);
+		break;
+	default:
+		writel(val, addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops gen_pci_ops = {
+	.read	= gen_pci_config_read,
+	.write	= gen_pci_config_write,
+};
+
+static int gen_pci_calc_io_offset(struct device *dev,
+				  struct of_pci_range *range,
+				  struct resource *res,
+				  resource_size_t *offset)
+{
+	static atomic_t wins = ATOMIC_INIT(0);
+	int err, idx, max_win;
+	unsigned int window;
+
+	if (!PAGE_ALIGNED(range->cpu_addr))
+		return -EINVAL;
+
+	max_win = (IO_SPACE_LIMIT + 1) / SZ_64K;
+	idx = atomic_inc_return(&wins);
+	if (idx >= max_win)
+		return -ENOSPC;
+
+	window = (idx - 1) * SZ_64K;
+	err = pci_ioremap_io(window, range->cpu_addr);
+	if (err)
+		return err;
+
+	of_pci_range_to_resource(range, dev->of_node, res);
+	res->start = window;
+	res->end = res->start + range->size - 1;
+	*offset = window - range->pci_addr;
+	return 0;
+}
+
+static int gen_pci_calc_mem_offset(struct device *dev,
+				   struct of_pci_range *range,
+				   struct resource *res,
+				   resource_size_t *offset)
+{
+	of_pci_range_to_resource(range, dev->of_node, res);
+	*offset = range->cpu_addr - range->pci_addr;
+	return 0;
+}
+
+static const struct of_device_id gen_pci_of_match[] = {
+	{ .compatible = "pci-host-cam-generic",
+	  .data = &gen_pci_cfg_cam_bus_ops },
+
+	{ .compatible = "pci-host-ecam-generic",
+	  .data = &gen_pci_cfg_ecam_bus_ops },
+
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gen_pci_of_match);
+
+static int gen_pci_setup(int nr, struct pci_sys_data *sys)
+{
+	int err, res_valid;
+	u8 bus_max;
+	struct pci_host_bridge_window *win;
+	struct resource *bus_range;
+	resource_size_t busn;
+	const char *type;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	const struct of_device_id *of_id;
+	const int *prop;
+	struct gen_pci *pci = sys->private_data;
+	struct device *dev = pci->host.dev.parent;
+	struct device_node *np = dev->of_node;
+
+	type = of_get_property(np, "device_type", NULL);
+	if (!type || strcmp(type, "pci")) {
+		dev_err(dev, "invalid \"device_type\" %s\n", type);
+		return -EINVAL;
+	}
+
+	if (of_pci_range_parser_init(&parser, np)) {
+		dev_err(dev, "missing \"ranges\" property\n");
+		return -EINVAL;
+	}
+
+	prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL);
+	if (prop) {
+		if (*prop)
+			pci_add_flags(PCI_PROBE_ONLY);
+		else
+			pci_clear_flags(PCI_PROBE_ONLY);
+	}
+
+	if (of_pci_parse_bus_range(np, &pci->cfg.bus_range))
+		pci->cfg.bus_range = (struct resource) {
+			.name	= np->name,
+			.start	= 0,
+			.end	= 0xff,
+			.flags	= IORESOURCE_BUS,
+		};
+
+	err = of_address_to_resource(np, 0, &pci->cfg.res);
+	if (err) {
+		dev_err(dev, "missing \"reg\" property\n");
+		return err;
+	}
+
+	pci->cfg.win = devm_kcalloc(dev, resource_size(&pci->cfg.bus_range),
+				    sizeof(*pci->cfg.win), GFP_KERNEL);
+	if (!pci->cfg.win)
+		return -ENOMEM;
+
+	of_id = of_match_node(gen_pci_of_match, np);
+	pci->cfg.ops = of_id->data;
+	INIT_LIST_HEAD(&pci->host.windows);
+
+	/* Limit the bus-range to fit within reg */
+	bus_max = pci->cfg.bus_range.start +
+		  (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
+	pci->cfg.bus_range.end = min_t(resource_size_t, pci->cfg.bus_range.end,
+				       bus_max);
+
+	/* Create and register resources from the ranges property */
+	res_valid = 0;
+	for_each_of_pci_range(&parser, &range) {
+		struct resource *parent, *res;
+		resource_size_t offset;
+		u32 restype = range.flags & IORESOURCE_TYPE_BITS;
+
+		res = devm_kmalloc(dev, sizeof(*res), GFP_KERNEL);
+		if (!res) {
+			err = -ENOMEM;
+			goto out_release_res;
+		}
+
+		switch (restype) {
+		case IORESOURCE_IO:
+			parent = &ioport_resource;
+			err = gen_pci_calc_io_offset(dev, &range, res, &offset);
+			break;
+		case IORESOURCE_MEM:
+			parent = &iomem_resource;
+			err = gen_pci_calc_mem_offset(dev, &range, res, &offset);
+			res_valid |= !(res->flags & IORESOURCE_PREFETCH || err);
+			break;
+		default:
+			err = -EINVAL;
+			continue;
+		}
+
+		if (err) {
+			dev_warn(dev,
+				 "error %d: failed to add resource [type 0x%x, %lld bytes]\n",
+				 err, restype, range.size);
+			continue;
+		}
+
+		err = request_resource(parent, res);
+		if (err)
+			goto out_release_res;
+
+		pci_add_resource_offset(&sys->resources, res, offset);
+	}
+
+	if (!res_valid) {
+		dev_err(dev, "non-prefetchable memory resource required\n");
+		err = -EINVAL;
+		goto out_release_res;
+	}
+
+	/* Map our Configuration Space windows */
+	if (!request_mem_region(pci->cfg.res.start,
+				resource_size(&pci->cfg.res),
+				"Configuration Space"))
+		goto out_release_res;
+
+	bus_range = &pci->cfg.bus_range;
+	for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
+		u32 idx = busn - bus_range->start;
+		u32 sz = 1 << pci->cfg.ops->bus_shift;
+
+		pci->cfg.win[idx] = devm_ioremap(dev,
+						 pci->cfg.res.start + busn * sz,
+						 sz);
+		if (!pci->cfg.win[idx]) {
+			err = -ENOMEM;
+			goto out_unmap_cfg;
+		}
+	}
+
+	/* Register bus resource */
+	pci_add_resource(&sys->resources, bus_range);
+	return 1;
+
+out_unmap_cfg:
+	while (busn-- > bus_range->start)
+		devm_iounmap(dev, pci->cfg.win[busn - bus_range->start]);
+
+out_release_res:
+	release_child_resources(&iomem_resource);
+	release_child_resources(&ioport_resource);
+
+	list_for_each_entry(win, &sys->resources, list)
+		devm_kfree(dev, win->res);
+	pci_free_resource_list(&sys->resources);
+
+	devm_kfree(dev, pci->cfg.win);
+	return err;
+}
+
+static int gen_pci_probe(struct platform_device *pdev)
+{
+	struct hw_pci hw;
+	struct gen_pci *pci;
+	struct device *dev = &pdev->dev;
+
+	if (!dev->of_node)
+		return -ENODEV;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->host.dev.parent = dev;
+	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;
+}
+
+static struct platform_driver gen_pci_driver = {
+	.driver = {
+		.name = "pci-host-generic",
+		.owner = THIS_MODULE,
+		.of_match_table = gen_pci_of_match,
+	},
+	.probe = gen_pci_probe,
+};
+module_platform_driver(gen_pci_driver);
+
+MODULE_DESCRIPTION("Generic PCI host driver");
+MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
+MODULE_LICENSE("GPLv2");
-- 
1.9.2

[RESEND PATCH 3/3] MAINTAINERS: add entry for generic PCI host controller driver

[Will Deacon]

Add myself as the maintainer for the generic PCI host controller driver.

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e67ea2442041..d5db11484062 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6740,6 +6740,14 @@ L:	linux-pci@vger.kernel.org
 S:	Maintained
 F:	drivers/pci/host/*designware*
 
+PCI DRIVER FOR GENERIC OF HOSTS
+M:	Will Deacon <will.deacon@arm.com>
+L:	linux-pci@vger.kernel.org
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/host-generic-pci.txt
+F:	drivers/pci/host/pci-host-generic.c
+
 PCMCIA SUBSYSTEM
 P:	Linux PCMCIA Team
 L:	linux-pcmcia@lists.infradead.org
-- 
1.9.2

Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[Jason Gunthorpe]

On Fri, May 02, 2014 at 05:41:15PM +0100, Will Deacon wrote:

> +       bus_range = &pci->cfg.bus_range;
> +       for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
> +               u32 idx = busn - bus_range->start;
> +               u32 sz = 1 << pci->cfg.ops->bus_shift;
> +
> +               pci->cfg.win[idx] = devm_ioremap(dev,
> +                                                pci->cfg.res.start + busn * sz,
> +                                                sz);

Why map each bus individually? Both CAM and ECAM define consecutive
busses consecutively in the address space, and I though ioremap was OK
with unaligned stuff?

> +out_unmap_cfg:
> +	while (busn-- > bus_range->start)
> +		devm_iounmap(dev, pci->cfg.win[busn - bus_range->start]);

Is there a reason to explicitly clean up devm resources? I guess it is
because this is in setup not probe?

It seems strange to me for a driver to do this sort of work in a setup
function, typically probe acquires as much stuff as it can, that way
defered probe can work properly.

Looking at pci-mvebu, 'setup' is only populating the resource list, I
would suggest the same split for this driver.

> +out_release_res:
> +	release_child_resources(&iomem_resource);
> +	release_child_resources(&ioport_resource);

This looks really off to me, doesn't this free all resources in the
system?

This isn't enough?:

> +	list_for_each_entry(win, &sys->resources, list)
> +		devm_kfree(dev, win->res);
> +	pci_free_resource_list(&sys->resources);


At worst, I would guess 'release_child_resources(win->res);' in the
loop. But since no bus scan has been done, is there any chance of
child resources at this point?

Regards,
Jason

Re: [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected

[Arnd Bergmann]

On Friday 02 May 2014 17:41:14 Will Deacon wrote:
> mach-virt can make use of virtio-pci devices, which requires the guest
> kernel to have PCI support selected.
> 
> This patch selects CONFIG_MIGHT_HAVE_PCI when CONFIG_ARCH_VIRT=y.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index db3c5414223e..f24356c4d630 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -941,6 +941,7 @@ config ARCH_VIRT
>         select ARM_GIC
>         select ARM_PSCI
>         select HAVE_ARM_ARCH_TIMER
> +       select MIGHT_HAVE_PCI

I would actually just put it under ARCH_MULTIPLATFORM. Anything can
have PCI really, and it doesn't cause any harm.

	Arnd

Re: [RESEND PATCH 1/3] ARM: mach-virt: allow PCI support to be selected

[Will Deacon]

Hi Arnd,

On Fri, May 02, 2014 at 07:11:04PM +0100, Arnd Bergmann wrote:
> On Friday 02 May 2014 17:41:14 Will Deacon wrote:
> > mach-virt can make use of virtio-pci devices, which requires the guest
> > kernel to have PCI support selected.
> > 
> > This patch selects CONFIG_MIGHT_HAVE_PCI when CONFIG_ARCH_VIRT=y.
> > 
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index db3c5414223e..f24356c4d630 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -941,6 +941,7 @@ config ARCH_VIRT
> >         select ARM_GIC
> >         select ARM_PSCI
> >         select HAVE_ARM_ARCH_TIMER
> > +       select MIGHT_HAVE_PCI
> 
> I would actually just put it under ARCH_MULTIPLATFORM. Anything can
> have PCI really, and it doesn't cause any harm.

Sure, I can change that easily enough.

Will

Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Friday 02 May 2014 11:23:18 Jason Gunthorpe wrote:
> On Fri, May 02, 2014 at 05:41:15PM +0100, Will Deacon wrote:
> 
> > +       bus_range = &pci->cfg.bus_range;
> > +       for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
> > +               u32 idx = busn - bus_range->start;
> > +               u32 sz = 1 << pci->cfg.ops->bus_shift;
> > +
> > +               pci->cfg.win[idx] = devm_ioremap(dev,
> > +                                                pci->cfg.res.start + busn * sz,
> > +                                                sz);
> 
> Why map each bus individually? Both CAM and ECAM define consecutive
> busses consecutively in the address space, and I though ioremap was OK
> with unaligned stuff?

One optimization we discussed before was to do this ioremap on the first
access to any config space register in one bus, so we don't actually have
to map all of them but only the ones that are in use.

I don't know if there was a technical problem with that. We can't just
map/unmap on every config space access, because it can be called from
atomic context and ioremap can sleep, but the initial bus scan is
always done in a context in which we are allowed to sleep.

> > +out_unmap_cfg:
> > +     while (busn-- > bus_range->start)
> > +             devm_iounmap(dev, pci->cfg.win[busn - bus_range->start]);
> 
> Is there a reason to explicitly clean up devm resources? I guess it is
> because this is in setup not probe?

Setup is called from probe, through pci_common_init_dev(), so that shouldn't
make a difference.

> It seems strange to me for a driver to do this sort of work in a setup
> function, typically probe acquires as much stuff as it can, that way
> defered probe can work properly.
> 
> Looking at pci-mvebu, 'setup' is only populating the resource list, I
> would suggest the same split for this driver.

I suggested moving it all into setup, to make it easier to port this code
to arm64: I don't expect we will have the same pci_common_init_dev()
mechanism there, so setup will get called directly from the probe
function.

The alternative is to do everything in probe() as well for arm32, and only
do a single list_move() of the resources list to sys->resources in the
setup function. That was the advice I gave in the xilinx pci host driver
review for the same reason. I only now noticed that I recommended the opposite
here. Anyway it shouldn't matter where we do all the things, but I feel
it's better to have only one function that does all the work for the case
of having nr_controllers=1, as we always do for loadable host drivers.

	Arnd

Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

Hi Arnd, Jason,

Thanks for taking a look.

On Fri, May 02, 2014 at 07:25:36PM +0100, Arnd Bergmann wrote:
> On Friday 02 May 2014 11:23:18 Jason Gunthorpe wrote:
> > On Fri, May 02, 2014 at 05:41:15PM +0100, Will Deacon wrote:
> > 
> > > +       bus_range = &pci->cfg.bus_range;
> > > +       for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
> > > +               u32 idx = busn - bus_range->start;
> > > +               u32 sz = 1 << pci->cfg.ops->bus_shift;
> > > +
> > > +               pci->cfg.win[idx] = devm_ioremap(dev,
> > > +                                                pci->cfg.res.start + busn * sz,
> > > +                                                sz);
> > 
> > Why map each bus individually? Both CAM and ECAM define consecutive
> > busses consecutively in the address space, and I though ioremap was OK
> > with unaligned stuff?
> 
> One optimization we discussed before was to do this ioremap on the first
> access to any config space register in one bus, so we don't actually have
> to map all of them but only the ones that are in use.

Right, and this optimisation is because we don't have a lot of virtual
address space on 32-bit ARM, so blowing away 256M on ECAM isn't viable.

> I don't know if there was a technical problem with that. We can't just
> map/unmap on every config space access, because it can be called from
> atomic context and ioremap can sleep, but the initial bus scan is
> always done in a context in which we are allowed to sleep.

It just doesn't seem worth it, given that we have the bus-range property
in the DT. I can revisit it if there are strong objections to the current
code though (looking back, I ended up needing to take a lock last time I
tried this).

> > > +out_unmap_cfg:
> > > +     while (busn-- > bus_range->start)
> > > +             devm_iounmap(dev, pci->cfg.win[busn - bus_range->start]);
> > 
> > Is there a reason to explicitly clean up devm resources? I guess it is
> > because this is in setup not probe?
> 
> Setup is called from probe, through pci_common_init_dev(), so that shouldn't
> make a difference.

Given that the idea was to separate setup() and probe(), I didn't want to
make the assumption that I was called in probe context.

> > It seems strange to me for a driver to do this sort of work in a setup
> > function, typically probe acquires as much stuff as it can, that way
> > defered probe can work properly.
> > 
> > Looking at pci-mvebu, 'setup' is only populating the resource list, I
> > would suggest the same split for this driver.
> 
> I suggested moving it all into setup, to make it easier to port this code
> to arm64: I don't expect we will have the same pci_common_init_dev()
> mechanism there, so setup will get called directly from the probe
> function.

In which case, I *could* remove that freeing code, but I'd rather wait until
we know for sure that it's not needed (that is, when I go about plumbing in
the support for arm64 after Liviu's patches are merged).

> The alternative is to do everything in probe() as well for arm32, and only
> do a single list_move() of the resources list to sys->resources in the
> setup function. That was the advice I gave in the xilinx pci host driver
> review for the same reason. I only now noticed that I recommended the opposite
> here. Anyway it shouldn't matter where we do all the things, but I feel
> it's better to have only one function that does all the work for the case
> of having nr_controllers=1, as we always do for loadable host drivers.

Ok, I'll leave it like it is for now then.

Will

Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[Jason Gunthorpe]

On Fri, May 02, 2014 at 07:44:21PM +0100, Will Deacon wrote:
> > > > +       bus_range = &pci->cfg.bus_range;
> > > > +       for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
> > > > +               u32 idx = busn - bus_range->start;
> > > > +               u32 sz = 1 << pci->cfg.ops->bus_shift;
> > > > +
> > > > +               pci->cfg.win[idx] = devm_ioremap(dev,
> > > > +                                                pci->cfg.res.start + busn * sz,
> > > > +                                                sz);
> > > 
> > > Why map each bus individually? Both CAM and ECAM define consecutive
> > > busses consecutively in the address space, and I though ioremap was OK
> > > with unaligned stuff?
> > 
> > One optimization we discussed before was to do this ioremap on the first
> > access to any config space register in one bus, so we don't actually have
> > to map all of them but only the ones that are in use.
> 
> Right, and this optimisation is because we don't have a lot of virtual
> address space on 32-bit ARM, so blowing away 256M on ECAM isn't viable.

But why not just

devm_ioremap(dev, pci->cfg.res.start + bus_range->start * sz,
		  resource_size(&bus_range) * sz);

?

> > Setup is called from probe, through pci_common_init_dev(), so that shouldn't
> > make a difference.
> 
> Given that the idea was to separate setup() and probe(), I didn't want to
> make the assumption that I was called in probe context.

IMHO, we need to have clear purposes for setup() and probe().

Probe is well defined already, it requests resources, claims the
device, puts it in reset and then does some subsystem specific thing.
The interaction with probe and devm is also already well specified.

It doesn't matter for this driver, but look at mvebu, you cannot move
the interrupt, gpio and clock acquisitions from probe() to setup(), as
they could all trigger a defered probe. Better to be consistent,
especially if this is the golden reference driver we want everyone to
follow (sorry Will)

To me setup() is more like netdev open(), so it should just do that
final step to enable the links and bring up the PCI network and be
ready to run PCI discovery. Consider, someday we might have an
unsetup() for power mangement reasons, just like we have a close() in
netdev.

If the long term plan is to keep probe() then I don't think it makes
sense to move probe() duties into setup(). That just restricts what we
can do with the setup() call if setup() is now required to always
handle defered probe and so forth.

Will, don't forget this q:

------
> +out_release_res:
> +     release_child_resources(&iomem_resource);
> +     release_child_resources(&ioport_resource);

This looks really off to me, doesn't this free all resources in the
system?
-------

Regards,
Jason

Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Friday 02 May 2014 13:03:18 Jason Gunthorpe wrote:
> 
> > > Setup is called from probe, through pci_common_init_dev(), so that shouldn't
> > > make a difference.
> > 
> > Given that the idea was to separate setup() and probe(), I didn't want to
> > make the assumption that I was called in probe context.
> 
> IMHO, we need to have clear purposes for setup() and probe().
> 
> Probe is well defined already, it requests resources, claims the
> device, puts it in reset and then does some subsystem specific thing.
> The interaction with probe and devm is also already well specified.
> 
> It doesn't matter for this driver, but look at mvebu, you cannot move
> the interrupt, gpio and clock acquisitions from probe() to setup(), as
> they could all trigger a defered probe. Better to be consistent,
> especially if this is the golden reference driver we want everyone to
> follow (sorry Will)

Fair enough. It shouldn't be hard to move.

> To me setup() is more like netdev open(), so it should just do that
> final step to enable the links and bring up the PCI network and be
> ready to run PCI discovery. Consider, someday we might have an
> unsetup() for power mangement reasons, just like we have a close() in
> netdev.

I expect setup() to just go away in the long run for loadable host
controllers, and get merged into probe().

> If the long term plan is to keep probe() then I don't think it makes
> sense to move probe() duties into setup(). That just restricts what we
> can do with the setup() call if setup() is now required to always
> handle defered probe and so forth.

setup() makes sense for the traditional ARM32 PCI support with
nr_controllers > 1: dove, kirkwood, mv78xx0, orion5x and iop13xx
(most of these are gradually getting replace with pci-mvebu.c,
which doesn't do that, as you know). All other classic drivers only
support one host bridge anyway, and the loadable driver just
use one domain per host bridge, which means you lose all the
advantages of having pci_common_init_dev() assign the bus
numbers and call setup functions for each host bridge.

It should be fine to have a trivial setup function like this

static int gen_pci_setup(int nr, struct pci_sys_data *sys)
{
	struct gen_pci *pci = sys->private_data;
	list_splice_init(pci->resource, sys->resources);
	return 1;
}

and do everything in probe().

	Arnd

Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Fri, May 02, 2014 at 08:03:18PM +0100, Jason Gunthorpe wrote:
> On Fri, May 02, 2014 at 07:44:21PM +0100, Will Deacon wrote:
> > > > > +       bus_range = &pci->cfg.bus_range;
> > > > > +       for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
> > > > > +               u32 idx = busn - bus_range->start;
> > > > > +               u32 sz = 1 << pci->cfg.ops->bus_shift;
> > > > > +
> > > > > +               pci->cfg.win[idx] = devm_ioremap(dev,
> > > > > +                                                pci->cfg.res.start + busn * sz,
> > > > > +                                                sz);
> > > > 
> > > > Why map each bus individually? Both CAM and ECAM define consecutive
> > > > busses consecutively in the address space, and I though ioremap was OK
> > > > with unaligned stuff?
> > > 
> > > One optimization we discussed before was to do this ioremap on the first
> > > access to any config space register in one bus, so we don't actually have
> > > to map all of them but only the ones that are in use.
> > 
> > Right, and this optimisation is because we don't have a lot of virtual
> > address space on 32-bit ARM, so blowing away 256M on ECAM isn't viable.
> 
> But why not just
> 
> devm_ioremap(dev, pci->cfg.res.start + bus_range->start * sz,
> 		  resource_size(&bus_range) * sz);
> 
> ?

I was just trying to avoid the need for a single, virtually contiguous range
where it's not actually required. If the driver is loaded as a module, we
could already have a bunch of fragmentation in the vmalloc space.

> > > Setup is called from probe, through pci_common_init_dev(), so that shouldn't
> > > make a difference.
> > 
> > Given that the idea was to separate setup() and probe(), I didn't want to
> > make the assumption that I was called in probe context.
> 
> IMHO, we need to have clear purposes for setup() and probe().
> 
> Probe is well defined already, it requests resources, claims the
> device, puts it in reset and then does some subsystem specific thing.
> The interaction with probe and devm is also already well specified.
> 
> It doesn't matter for this driver, but look at mvebu, you cannot move
> the interrupt, gpio and clock acquisitions from probe() to setup(), as
> they could all trigger a defered probe. Better to be consistent,
> especially if this is the golden reference driver we want everyone to
> follow (sorry Will)

Groan!

> To me setup() is more like netdev open(), so it should just do that
> final step to enable the links and bring up the PCI network and be
> ready to run PCI discovery. Consider, someday we might have an
> unsetup() for power mangement reasons, just like we have a close() in
> netdev.

That's what I did originally! Anyway, this is just refactoring so shouldn't
be too much work. After reading Arnd's reply,  I'll move the bulk into
->probe().

> If the long term plan is to keep probe() then I don't think it makes
> sense to move probe() duties into setup(). That just restricts what we
> can do with the setup() call if setup() is now required to always
> handle defered probe and so forth.
> 
> Will, don't forget this q:

Sorry, that got chopped off somehow...

> ------
> > +out_release_res:
> > +     release_child_resources(&iomem_resource);
> > +     release_child_resources(&ioport_resource);
> 
> This looks really off to me, doesn't this free all resources in the
> system?

... yes, I suppose that would cause problems if we have multiple instances
of the driver. I could probably just release_resource(win->res) instead
of freeing the resources and use devm_request_mem_region for configuration
space.

Will

Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

On Fri, May 02, 2014 at 08:29:30PM +0100, Arnd Bergmann wrote:
> On Friday 02 May 2014 13:03:18 Jason Gunthorpe wrote:
> > > Given that the idea was to separate setup() and probe(), I didn't want to
> > > make the assumption that I was called in probe context.
> > 
> > It doesn't matter for this driver, but look at mvebu, you cannot move
> > the interrupt, gpio and clock acquisitions from probe() to setup(), as
> > they could all trigger a defered probe. Better to be consistent,
> > especially if this is the golden reference driver we want everyone to
> > follow (sorry Will)
> 
> Fair enough. It shouldn't be hard to move.

[...]

> It should be fine to have a trivial setup function like this
> 
> static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> {
> 	struct gen_pci *pci = sys->private_data;
> 	list_splice_init(pci->resource, sys->resources);
> 	return 1;
> }
> 
> and do everything in probe().

Ok, I've respun the patch and included it below. It's turned probe() into a
bit of a beast, but the cleanup is a lot simpler (unless I'm missing
something).

Will

--->8

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
new file mode 100644
index 000000000000..199a8795cb77
--- /dev/null
+++ b/drivers/pci/host/pci-host-generic.c
@@ -0,0 +1,361 @@
+/*
+ * Simple, generic PCI host controller driver targetting firmware-initialised
+ * systems and virtual machines (e.g. the PCI emulation provided by kvmtool).
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (C) 2014 ARM Limited
+ *
+ * Author: Will Deacon <will.deacon@arm.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/platform_device.h>
+
+struct gen_pci_cfg_bus_ops {
+	u32 bus_shift;
+	void __iomem *(*map_bus)(struct pci_bus *, unsigned int, int);
+};
+
+struct gen_pci_cfg_windows {
+	struct resource				res;
+	struct resource				bus_range;
+	void __iomem				**win;
+
+	const struct gen_pci_cfg_bus_ops	*ops;
+};
+
+struct gen_pci {
+	struct pci_host_bridge			host;
+	struct gen_pci_cfg_windows		cfg;
+	struct list_head			resources;
+};
+
+static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
+					     unsigned int devfn,
+					     int where)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+	resource_size_t idx = bus->number - pci->cfg.bus_range.start;
+
+	return pci->cfg.win[idx] + ((devfn << 8) | where);
+}
+
+static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = {
+	.bus_shift	= 16,
+	.map_bus	= gen_pci_map_cfg_bus_cam,
+};
+
+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;
+	resource_size_t idx = bus->number - pci->cfg.bus_range.start;
+
+	return pci->cfg.win[idx] + ((devfn << 12) | where);
+}
+
+static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = {
+	.bus_shift	= 20,
+	.map_bus	= gen_pci_map_cfg_bus_ecam,
+};
+
+static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn,
+				int where, int size, u32 *val)
+{
+	void __iomem *addr;
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+
+	addr = pci->cfg.ops->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		*val = readb(addr);
+		break;
+	case 2:
+		*val = readw(addr);
+		break;
+	default:
+		*val = readl(addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
+				 int where, int size, u32 val)
+{
+	void __iomem *addr;
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+
+	addr = pci->cfg.ops->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		writeb(val, addr);
+		break;
+	case 2:
+		writew(val, addr);
+		break;
+	default:
+		writel(val, addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops gen_pci_ops = {
+	.read	= gen_pci_config_read,
+	.write	= gen_pci_config_write,
+};
+
+static int gen_pci_calc_io_offset(struct device *dev,
+				  struct of_pci_range *range,
+				  struct resource *res,
+				  resource_size_t *offset)
+{
+	static atomic_t wins = ATOMIC_INIT(0);
+	int err, idx, max_win;
+	unsigned int window;
+
+	if (!PAGE_ALIGNED(range->cpu_addr))
+		return -EINVAL;
+
+	max_win = (IO_SPACE_LIMIT + 1) / SZ_64K;
+	idx = atomic_inc_return(&wins);
+	if (idx >= max_win)
+		return -ENOSPC;
+
+	window = (idx - 1) * SZ_64K;
+	err = pci_ioremap_io(window, range->cpu_addr);
+	if (err)
+		return err;
+
+	of_pci_range_to_resource(range, dev->of_node, res);
+	res->start = window;
+	res->end = res->start + range->size - 1;
+	*offset = window - range->pci_addr;
+	return 0;
+}
+
+static int gen_pci_calc_mem_offset(struct device *dev,
+				   struct of_pci_range *range,
+				   struct resource *res,
+				   resource_size_t *offset)
+{
+	of_pci_range_to_resource(range, dev->of_node, res);
+	*offset = range->cpu_addr - range->pci_addr;
+	return 0;
+}
+
+static const struct of_device_id gen_pci_of_match[] = {
+	{ .compatible = "pci-host-cam-generic",
+	  .data = &gen_pci_cfg_cam_bus_ops },
+
+	{ .compatible = "pci-host-ecam-generic",
+	  .data = &gen_pci_cfg_ecam_bus_ops },
+
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gen_pci_of_match);
+
+static int gen_pci_setup(int nr, struct pci_sys_data *sys)
+{
+	struct gen_pci *pci = sys->private_data;
+	list_splice_init(&pci->resources, &sys->resources);
+	return 1;
+}
+
+static int gen_pci_probe(struct platform_device *pdev)
+{
+	struct gen_pci *pci;
+	int err, res_valid;
+	struct pci_host_bridge_window *win;
+	struct resource *bus_range;
+	resource_size_t busn;
+	const char *type;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	struct hw_pci hw;
+	const struct of_device_id *of_id;
+	const int *prop;
+	u8 bus_max;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	if (!dev->of_node)
+		return -ENODEV;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->host.dev.parent = dev;
+	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,
+	};
+
+	type = of_get_property(np, "device_type", NULL);
+	if (!type || strcmp(type, "pci")) {
+		dev_err(dev, "invalid \"device_type\" %s\n", type);
+		return -EINVAL;
+	}
+
+	if (of_pci_range_parser_init(&parser, np)) {
+		dev_err(dev, "missing \"ranges\" property\n");
+		return -EINVAL;
+	}
+
+	prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL);
+	if (prop) {
+		if (*prop)
+			pci_add_flags(PCI_PROBE_ONLY);
+		else
+			pci_clear_flags(PCI_PROBE_ONLY);
+	}
+
+	if (of_pci_parse_bus_range(np, &pci->cfg.bus_range))
+		pci->cfg.bus_range = (struct resource) {
+			.name	= np->name,
+			.start	= 0,
+			.end	= 0xff,
+			.flags	= IORESOURCE_BUS,
+		};
+
+	err = of_address_to_resource(np, 0, &pci->cfg.res);
+	if (err) {
+		dev_err(dev, "missing \"reg\" property\n");
+		return err;
+	}
+
+	pci->cfg.win = devm_kcalloc(dev, resource_size(&pci->cfg.bus_range),
+				    sizeof(*pci->cfg.win), GFP_KERNEL);
+	if (!pci->cfg.win)
+		return -ENOMEM;
+
+	of_id = of_match_node(gen_pci_of_match, np);
+	pci->cfg.ops = of_id->data;
+	INIT_LIST_HEAD(&pci->host.windows);
+	INIT_LIST_HEAD(&pci->resources);
+
+	/* Limit the bus-range to fit within reg */
+	bus_max = pci->cfg.bus_range.start +
+		  (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
+	pci->cfg.bus_range.end = min_t(resource_size_t, pci->cfg.bus_range.end,
+				       bus_max);
+
+	/* Create and register resources from the ranges property */
+	res_valid = 0;
+	for_each_of_pci_range(&parser, &range) {
+		struct resource *parent, *res;
+		resource_size_t offset;
+		u32 restype = range.flags & IORESOURCE_TYPE_BITS;
+
+		res = devm_kmalloc(dev, sizeof(*res), GFP_KERNEL);
+		if (!res) {
+			err = -ENOMEM;
+			goto out_release_res;
+		}
+
+		switch (restype) {
+		case IORESOURCE_IO:
+			parent = &ioport_resource;
+			err = gen_pci_calc_io_offset(dev, &range, res, &offset);
+			break;
+		case IORESOURCE_MEM:
+			parent = &iomem_resource;
+			err = gen_pci_calc_mem_offset(dev, &range, res, &offset);
+			res_valid |= !(res->flags & IORESOURCE_PREFETCH || err);
+			break;
+		default:
+			err = -EINVAL;
+			continue;
+		}
+
+		if (err) {
+			dev_warn(dev,
+				 "error %d: failed to add resource [type 0x%x, %lld bytes]\n",
+				 err, restype, range.size);
+			continue;
+		}
+
+		err = request_resource(parent, res);
+		if (err)
+			goto out_release_res;
+
+		pci_add_resource_offset(&pci->resources, res, offset);
+	}
+
+	if (!res_valid) {
+		dev_err(dev, "non-prefetchable memory resource required\n");
+		err = -EINVAL;
+		goto out_release_res;
+	}
+
+	/* Map our Configuration Space windows */
+	err = -ENOMEM;
+	if (!devm_request_mem_region(dev, pci->cfg.res.start,
+				     resource_size(&pci->cfg.res),
+				     "Configuration Space"))
+		goto out_release_res;
+
+	bus_range = &pci->cfg.bus_range;
+	for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
+		u32 idx = busn - bus_range->start;
+		u32 sz = 1 << pci->cfg.ops->bus_shift;
+
+		pci->cfg.win[idx] = devm_ioremap(dev,
+						 pci->cfg.res.start + busn * sz,
+						 sz);
+		if (!pci->cfg.win[idx])
+			goto out_release_res;
+	}
+
+	/* Register bus resource */
+	pci_add_resource(&pci->resources, bus_range);
+	pci_common_init_dev(dev, &hw);
+	return 0;
+
+out_release_res:
+	list_for_each_entry(win, &pci->resources, list)
+		release_resource(win->res);
+
+	pci_free_resource_list(&pci->resources);
+	return err;
+}
+
+static struct platform_driver gen_pci_driver = {
+	.driver = {
+		.name = "pci-host-generic",
+		.owner = THIS_MODULE,
+		.of_match_table = gen_pci_of_match,
+	},
+	.probe = gen_pci_probe,
+};
+module_platform_driver(gen_pci_driver);
+
+MODULE_DESCRIPTION("Generic PCI host driver");
+MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
+MODULE_LICENSE("GPLv2");

Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[Arnd Bergmann]

On Tuesday 06 May 2014 19:38:12 Will Deacon wrote:
> Ok, I've respun the patch and included it below. It's turned probe() into a
> bit of a beast, but the cleanup is a lot simpler (unless I'm missing
> something).

Let's see if we can make it a bit more readable. None of these are important,
but together they could improve readability.

> +static int gen_pci_probe(struct platform_device *pdev)
> +{
> +       struct gen_pci *pci;
> +       int err, res_valid;
> +       struct pci_host_bridge_window *win;
> +       struct resource *bus_range;
> +       resource_size_t busn;
> +       const char *type;
> +       struct of_pci_range range;
> +       struct of_pci_range_parser parser;
> +       struct hw_pci hw;
> +       const struct of_device_id *of_id;
> +       const int *prop;
> +       u8 bus_max;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +
> +       if (!dev->of_node)
> +               return -ENODEV;

This check can go away: you don't get here without an of node.

> +       pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> +       if (!pci)
> +               return -ENOMEM;
> +
> +       pci->host.dev.parent = dev;
> +       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,
> +       };

C constructors are actually a gcc extension, I think it would be more
common to turn this into an initializer by moving it to the declaration:

	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
	struct hw_pci hw = {
               .nr_controllers = 1,
               .private_data   = (void **)&pci,
               .setup          = gen_pci_setup,
               .map_irq        = of_irq_parse_and_map_pci,
               .ops            = &gen_pci_ops,
       };


> +       if (of_pci_range_parser_init(&parser, np)) {
> +               dev_err(dev, "missing \"ranges\" property\n");
> +               return -EINVAL;
> +       }

If you move everything related to the range parser into a separate function,
you end up with two more compact parts. 

> +       /* Create and register resources from the ranges property */
> +       res_valid = 0;
> +       for_each_of_pci_range(&parser, &range) {
> +               struct resource *parent, *res;
> +               resource_size_t offset;
> +               u32 restype = range.flags & IORESOURCE_TYPE_BITS;

You could also move the body of the for loop into another function, or
both.

	Arnd

Re: [RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[Will Deacon]

Hi Arnd,

On Tue, May 06, 2014 at 08:11:48PM +0100, Arnd Bergmann wrote:
> On Tuesday 06 May 2014 19:38:12 Will Deacon wrote:
> > Ok, I've respun the patch and included it below. It's turned probe() into a
> > bit of a beast, but the cleanup is a lot simpler (unless I'm missing
> > something).
> 
> Let's see if we can make it a bit more readable. None of these are important,
> but together they could improve readability.

Sure, I managed to incorporate most of your ideas, but ended up splitting
probe into two other functions; one for dealing with the ranges and the
other for dealing with config space. I'll send a v6 later on.

Cheers,

Will