[PATCH v2 0/3] ARM: PCI: implement generic PCI host controller

[PATCH v2 0/3] ARM: PCI: implement generic PCI host controller

[Will Deacon]

Hello again,

This is version 2 of the patches I originally posted here:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/229679.html

Changes since v1 include:

  - Complete rename of files, comments and compatible strings to remove
    references to a `virtual' host controller

  - Support for ECAM (depending on matched compatible string)

  - Support for multiple mem resources

  - Correct use of resource offsets (I think...)

  - Removed explicit bridge enabling from bios32.c after discussion with
    Bjorn (pending testing/feedback from rmk)

Like v1, I continue to support only a single controller and therefore a
single I/O space. I'm not sure how to represent multiple controllers in the
device-tree without inventing some horrible hacks, so any ideas in this area
would be much appreciated.

I still need to take the step of moving parts of this into a library, but
I'd like to make sure the code is correct first. Tested on TC2 running KVM
with kvmtool and virtio-pci.

All feedback welcome (and thanks to Jason and Arnd for their comments on v1),

Will


Will Deacon (3):
  ARM: mach-virt: allow PCI support to be selected
  ARM: bios32: use pci_enable_resource to enable PCI resources
  PCI: ARM: add support for generic PCI host controller

 .../devicetree/bindings/pci/arm-generic-pci.txt    |  51 ++++
 arch/arm/kernel/bios32.c                           |  37 +--
 arch/arm/mach-virt/Kconfig                         |   1 +
 drivers/pci/host/Kconfig                           |   7 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-arm-generic.c                 | 318 +++++++++++++++++++++
 6 files changed, 381 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/arm-generic-pci.txt
 create mode 100644 drivers/pci/host/pci-arm-generic.c

-- 
1.8.2.2

[PATCH v2 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/mach-virt/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-virt/Kconfig b/arch/arm/mach-virt/Kconfig
index 081d46929436..f40fb55574cb 100644
--- a/arch/arm/mach-virt/Kconfig
+++ b/arch/arm/mach-virt/Kconfig
@@ -8,3 +8,4 @@ config ARCH_VIRT
 	select CPU_V7
 	select SPARSE_IRQ
 	select USE_OF
+	select MIGHT_HAVE_PCI
-- 
1.8.2.2

[PATCH v2 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Will Deacon]

This patch moves bios32 over to using the generic code for enabling PCI
resources. Since the core code takes care of bridge resources too, we
can also drop the explicit IO and MEMORY enabling for them in the arch
code.

A side-effect of this change is that we no longer explicitly enable
devices when running in PCI_PROBE_ONLY mode. This stays closer to the
meaning of the option and prevents us from trying to enable devices
without any assigned resources (the core code refuses to enable
resources without parents).

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/bios32.c | 37 +++----------------------------------
 1 file changed, 3 insertions(+), 34 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 317da88ae65b..91f48804e3bb 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -608,41 +608,10 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
  */
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	for (idx = 0; idx < 6; idx++) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1 << idx)))
-			continue;
-
-		r = dev->resource + idx;
-		if (!r->start && r->end) {
-			printk(KERN_ERR "PCI: Device %s not available because"
-			       " of resource collisions\n", pci_name(dev));
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
+	if (pci_has_flag(PCI_PROBE_ONLY))
+		return 0;
 
-	/*
-	 * Bridges (eg, cardbus bridges) need to be fully enabled
-	 */
-	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
-		cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
-
-	if (cmd != old_cmd) {
-		printk("PCI: enabling device %s (%04x -> %04x)\n",
-		       pci_name(dev), old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
+	return pci_enable_resources(dev, mask);
 }
 
 int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
-- 
1.8.2.2

Re: [PATCH v2 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Jason Gunthorpe]

On Wed, Feb 12, 2014 at 08:16:10PM +0000, Will Deacon wrote:
> This patch moves bios32 over to using the generic code for enabling PCI
> resources. Since the core code takes care of bridge resources too, we
> can also drop the explicit IO and MEMORY enabling for them in the arch
> code.

Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> (on Kirkwood)

PCI: bus1: Fast back to back transfers disabled
pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
pci 0000:00:01.0: BAR 8: assigned [mem 0xe0000000-0xe00fffff]
pci 0000:01:00.0: BAR 0: assigned [mem 0xe0000000-0xe001ffff]
pci 0000:00:01.0: PCI bridge to [bus 01]
pci 0000:00:01.0:   bridge window [mem 0xe0000000-0xe00fffff]
pci 0000:00:01.0: enabling device (0140 -> 0142)

Jason

Re: [PATCH v2 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Will Deacon]

On Wed, Feb 12, 2014 at 10:28:25PM +0000, Jason Gunthorpe wrote:
> On Wed, Feb 12, 2014 at 08:16:10PM +0000, Will Deacon wrote:
> > This patch moves bios32 over to using the generic code for enabling PCI
> > resources. Since the core code takes care of bridge resources too, we
> > can also drop the explicit IO and MEMORY enabling for them in the arch
> > code.
> 
> Tested-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> (on Kirkwood)
> 
> PCI: bus1: Fast back to back transfers disabled
> pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
> pci 0000:00:01.0: BAR 8: assigned [mem 0xe0000000-0xe00fffff]
> pci 0000:01:00.0: BAR 0: assigned [mem 0xe0000000-0xe001ffff]
> pci 0000:00:01.0: PCI bridge to [bus 01]
> pci 0000:00:01.0:   bridge window [mem 0xe0000000-0xe00fffff]
> pci 0000:00:01.0: enabling device (0140 -> 0142)

That's really helpful. Thanks for testing, Jason!

Will

Re: [PATCH v2 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources

[Jingoo Han]

On Thursday, February 13, 2014 5:16 AM, Will Deacon wrote:
> 
> This patch moves bios32 over to using the generic code for enabling PCI
> resources. Since the core code takes care of bridge resources too, we
> can also drop the explicit IO and MEMORY enabling for them in the arch
> code.
> 
> A side-effect of this change is that we no longer explicitly enable
> devices when running in PCI_PROBE_ONLY mode. This stays closer to the
> meaning of the option and prevents us from trying to enable devices
> without any assigned resources (the core code refuses to enable
> resources without parents).
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>

I tested this patch with NIC on Exynos platform.
It works properly.

Tested-by: Jingoo Han <jg1.han@samsung.com> 

[    0.406431] PCI: bus1: Fast back to back transfers disabled
[    0.411912] pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
[    0.418608] pci_bus 0000:00: busn_res: [bus 00-ff] end is updated to 01
[    0.425351] pci 0000:00:00.0: BAR 8: assigned [mem 0x40100000-0x401fffff]
[    0.432144] pci 0000:00:00.0: BAR 9: assigned [mem 0x40200000-0x402fffff pref]
[    0.439426] pci 0000:00:00.0: BAR 7: assigned [io  0x1000-0x1fff]
[    0.445599] pci 0000:01:00.0: BAR 1: assigned [mem 0x40100000-0x4017ffff]
[    0.452488] pci 0000:01:00.0: BAR 6: assigned [mem 0x40200000-0x4023ffff pref]
[    0.459745] pci 0000:01:00.0: BAR 0: assigned [mem 0x40180000-0x4019ffff]
[    0.466635] pci 0000:01:00.0: BAR 3: assigned [mem 0x401a0000-0x401a3fff]
[    0.473499] pci 0000:01:00.0: BAR 2: assigned [io  0x1000-0x101f]
[    0.479656] pci 0000:00:00.0: PCI bridge to [bus 01]

Best regards,
Jingoo Han

> ---
>  arch/arm/kernel/bios32.c | 37 +++----------------------------------
>  1 file changed, 3 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 317da88ae65b..91f48804e3bb 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -608,41 +608,10 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>   */
>  int pcibios_enable_device(struct pci_dev *dev, int mask)
>  {
> -	u16 cmd, old_cmd;
> -	int idx;
> -	struct resource *r;
> -
> -	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> -	old_cmd = cmd;
> -	for (idx = 0; idx < 6; idx++) {
> -		/* Only set up the requested stuff */
> -		if (!(mask & (1 << idx)))
> -			continue;
> -
> -		r = dev->resource + idx;
> -		if (!r->start && r->end) {
> -			printk(KERN_ERR "PCI: Device %s not available because"
> -			       " of resource collisions\n", pci_name(dev));
> -			return -EINVAL;
> -		}
> -		if (r->flags & IORESOURCE_IO)
> -			cmd |= PCI_COMMAND_IO;
> -		if (r->flags & IORESOURCE_MEM)
> -			cmd |= PCI_COMMAND_MEMORY;
> -	}
> +	if (pci_has_flag(PCI_PROBE_ONLY))
> +		return 0;
> 
> -	/*
> -	 * Bridges (eg, cardbus bridges) need to be fully enabled
> -	 */
> -	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
> -		cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
> -
> -	if (cmd != old_cmd) {
> -		printk("PCI: enabling device %s (%04x -> %04x)\n",
> -		       pci_name(dev), old_cmd, cmd);
> -		pci_write_config_word(dev, PCI_COMMAND, cmd);
> -	}
> -	return 0;
> +	return pci_enable_resources(dev, mask);
>  }
> 
>  int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
> --
> 1.8.2.2

[PATCH v2 3/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.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 .../devicetree/bindings/pci/arm-generic-pci.txt    |  51 ++++
 drivers/pci/host/Kconfig                           |   7 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-arm-generic.c                 | 318 +++++++++++++++++++++
 4 files changed, 377 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/arm-generic-pci.txt
 create mode 100644 drivers/pci/host/pci-arm-generic.c

diff --git a/Documentation/devicetree/bindings/pci/arm-generic-pci.txt b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
new file mode 100644
index 000000000000..cc7a35ecfa2d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
@@ -0,0 +1,51 @@
+* ARM 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:
+
+- compatible     : Must be "arm,pci-cam-generic" or "arm,pci-ecam-generic"
+                   depending on the layout of configuration space (CAM vs
+                   ECAM respectively)
+
+- ranges         : As described in IEEE Std 1275-1994, but must provide
+                   at least a definition of one or both of IO and Memory
+                   Space.
+
+- #address-cells : Must be 3
+
+- #size-cells    : Must be 2
+
+- reg            : The Configuration Space base address, as accessed by the
+                   parent bus.
+
+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>
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
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 13fb3333aa05..17f5555f8a29 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_ARM_GENERIC) += pci-arm-generic.o
diff --git a/drivers/pci/host/pci-arm-generic.c b/drivers/pci/host/pci-arm-generic.c
new file mode 100644
index 000000000000..31ce03ee2607
--- /dev/null
+++ b/drivers/pci/host/pci-arm-generic.c
@@ -0,0 +1,318 @@
+/*
+ * 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
+ *
+ * The current driver is limited to one I/O space per controller, and
+ * only supports a single controller.
+ *
+ * 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_accessors {
+	void __iomem *(*map_bus)(struct pci_bus *, unsigned int, int);
+	void (*unmap_bus)(struct pci_bus *);
+};
+
+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;
+};
+
+struct gen_pci {
+	struct device				*dev;
+	struct resource				*io_res;
+	struct list_head			mem_res;
+	struct gen_pci_cfg_window		cfg;
+};
+
+/*
+ * Configuration space accessors. We support CAM (simply map the entire
+ * 16M space) or ECAM (map a single 1M bus at a time).
+ */
+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;
+	u32 busn = bus->number;
+
+	return pci->cfg.base + ((busn << 16) | (devfn << 8) | where);
+}
+
+static void gen_pci_unmap_cfg_bus_cam(struct pci_bus *bus)
+{
+}
+
+static struct gen_pci_cfg_accessors gen_pci_cfg_cam_accessors = {
+	.map_bus	= gen_pci_map_cfg_bus_cam,
+	.unmap_bus	= gen_pci_unmap_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;
+	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);
+}
+
+static void gen_pci_unmap_cfg_bus_ecam(struct pci_bus *bus)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+
+	spin_unlock(&pci->cfg.lock);
+}
+
+static struct gen_pci_cfg_accessors gen_pci_cfg_ecam_accessors = {
+	.map_bus	= gen_pci_map_cfg_bus_ecam,
+	.unmap_bus	= gen_pci_unmap_cfg_bus_ecam,
+};
+
+static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn,
+				int where, int size, u32 *val)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+	void __iomem *addr = pci->cfg.accessors->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		*val = readb(addr);
+		break;
+	case 2:
+		*val = readw(addr);
+		break;
+	default:
+		*val = readl(addr);
+	}
+
+	pci->cfg.accessors->unmap_bus(bus);
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
+				 int where, int size, u32 val)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct gen_pci *pci = sys->private_data;
+	void __iomem *addr = pci->cfg.accessors->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		writeb(val, addr);
+		break;
+	case 2:
+		writew(val, addr);
+		break;
+	default:
+		writel(val, addr);
+	}
+
+	pci->cfg.accessors->unmap_bus(bus);
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops gen_pci_ops = {
+	.read	= gen_pci_config_read,
+	.write	= gen_pci_config_write,
+};
+
+static int gen_pci_setup(int nr, struct pci_sys_data *sys)
+{
+	int err;
+	struct gen_pci_resource *res;
+	struct gen_pci *pci = sys->private_data;
+
+	/* Map an initial Configuration Space window */
+	pci->cfg.base = devm_ioremap(pci->dev, pci->cfg.cpu_phys, SZ_1M);
+	if (!pci->cfg.base)
+		return -ENOMEM;
+
+	/* Register our I/O space */
+	if (pci->io_res) {
+		resource_size_t offset = nr * SZ_64K;
+
+		err = request_resource(&ioport_resource, pci->io_res);
+		if (err)
+			goto out_unmap_cfg_space;
+
+		err = pci_ioremap_io(offset, pci->io_res->start);
+		if (err)
+			goto out_release_io_res;
+
+		pci_add_resource_offset(&sys->resources, pci->io_res, offset);
+	}
+
+	/* Register our memory resources */
+	list_for_each_entry(res, &pci->mem_res, list) {
+		err = request_resource(&iomem_resource, &res->cpu_res);
+		if (err)
+			goto out_release_mem_res;
+
+		pci_add_resource_offset(&sys->resources,
+					&res->cpu_res,
+					res->offset);
+	}
+
+	return 1;
+out_release_mem_res:
+	list_for_each_entry_continue_reverse(res, &pci->mem_res, list)
+		release_resource(&res->cpu_res);
+out_release_io_res:
+	release_resource(pci->io_res);
+out_unmap_cfg_space:
+	devm_iounmap(pci->dev, pci->cfg.base);
+	return err;
+}
+
+static const struct of_device_id gen_pci_of_match[] = {
+	{ .compatible = "arm,pci-cam-generic",
+	  .data = &gen_pci_cfg_cam_accessors },
+
+	{ .compatible = "arm,pci-ecam-generic",
+	  .data = &gen_pci_cfg_ecam_accessors },
+
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gen_pci_of_match);
+
+static int gen_pci_probe(struct platform_device *pdev)
+{
+	struct hw_pci hw;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	struct gen_pci *pci;
+	const __be32 *reg;
+	const struct of_device_id *of_id;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	if (!np)
+		return -ENODEV;
+
+	if (of_pci_range_parser_init(&parser, np)) {
+		dev_err(dev, "missing \"ranges\" property\n");
+		return -EINVAL;
+	}
+
+	reg = of_get_property(np, "reg", NULL);
+	if (!reg) {
+		dev_err(dev, "missing \"reg\" property\n");
+		return -EINVAL;
+	}
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->cfg.cpu_phys = of_translate_address(np, reg);
+	if (pci->cfg.cpu_phys == OF_BAD_ADDR)
+		return -EINVAL;
+
+	of_id = of_match_node(gen_pci_of_match, np);
+	pci->cfg.accessors = of_id->data;
+	spin_lock_init(&pci->cfg.lock);
+	INIT_LIST_HEAD(&pci->mem_res);
+	pci->dev = dev;
+
+	for_each_of_pci_range(&parser, &range) {
+		struct gen_pci_resource *res;
+		u32 restype = range.flags & IORESOURCE_TYPE_BITS;
+
+		res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+		if (!res)
+			return -ENOMEM;
+
+		INIT_LIST_HEAD(&res->list);
+		of_pci_range_to_resource(&range, np, &res->cpu_res);
+
+		switch (restype) {
+		case IORESOURCE_IO:
+			if (pci->io_res) {
+				dev_warn(dev,
+					 "ignoring additional io resource\n");
+				devm_kfree(dev, res);
+			} else {
+				pci->io_res = &res->cpu_res;
+			}
+			break;
+		case IORESOURCE_MEM:
+			res->offset = range.cpu_addr - range.pci_addr;
+			list_add(&res->list, &pci->mem_res);
+			break;
+		default:
+			dev_warn(dev,
+				"ignoring unknown/unsupported resource type %x\n",
+				 restype);
+		}
+	}
+
+	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-arm-generic",
+		.owner = THIS_MODULE,
+		.of_match_table = gen_pci_of_match,
+	},
+	.probe = gen_pci_probe,
+};
+module_platform_driver(gen_pci_driver);
+
+MODULE_DESCRIPTION("ARM generic PCI host driver");
+MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
+MODULE_LICENSE("GPLv2");
-- 
1.8.2.2

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

[Arnd Bergmann]

On Wednesday 12 February 2014 20:16:11 Will Deacon wrote:
> +- 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.

> +- #address-cells : Must be 3
> +
> +- #size-cells    : Must be 2
> +
> +- reg            : The Configuration Space base address, as accessed by the
> +                   parent bus.
> +
> +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>

We probably want to add an optional 'bus-range' property (the default being
<0 255> if absent), so we don't have to map all the config space.

> 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.


> +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.

> +
> +struct gen_pci {
> +	struct device				*dev;
> +	struct resource				*io_res;
> +	struct list_head			mem_res;
> +	struct gen_pci_cfg_window		cfg;
> +};

How about making this structure derived from pci_host_bridge?
That would already contain a lot of the members, and gets two things
right: 

* it's useful to have an embedded 'struct device' here, and use dev->parent
  to point to the device from DT
* for I/O, we actually want a pci_host_bridge_window, not just a resource,
  since we should keep track of the offset

> +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.

> +static int gen_pci_probe(struct platform_device *pdev)
> +{
> +	struct hw_pci hw;
> +	struct of_pci_range range;
> +	struct of_pci_range_parser parser;
> +	struct gen_pci *pci;
> +	const __be32 *reg;
> +	const struct of_device_id *of_id;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;

Could you try to move almost all of this function into gen_pci_setup()?
I suspect this will make it easier later to share this driver with other
architectures.

> +
> +	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.

	Arnd

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

[Will Deacon]

Hi Arnd,

Thanks again for the comments.

On Wed, Feb 12, 2014 at 08:59:46PM +0000, Arnd Bergmann wrote:
> On Wednesday 12 February 2014 20:16:11 Will Deacon wrote:
> > +- 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).

> > +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>
> 
> We probably want to add an optional 'bus-range' property (the default being
> <0 255> if absent), so we don't have to map all the config space.

Yes, and that will be important given your comments later on.

> > 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).

> > +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.

> > +
> > +struct gen_pci {
> > +	struct device				*dev;
> > +	struct resource				*io_res;
> > +	struct list_head			mem_res;
> > +	struct gen_pci_cfg_window		cfg;
> > +};
> 
> How about making this structure derived from pci_host_bridge?
> That would already contain a lot of the members, and gets two things
> right: 
> 
> * it's useful to have an embedded 'struct device' here, and use dev->parent
>   to point to the device from DT
> * for I/O, we actually want a pci_host_bridge_window, not just a resource,
>   since we should keep track of the offset

Sure. Also, if we kill nr_controllers, then we can have a simple I/O space
allocator to populate the offset.

> > +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.

> > +static int gen_pci_probe(struct platform_device *pdev)
> > +{
> > +	struct hw_pci hw;
> > +	struct of_pci_range range;
> > +	struct of_pci_range_parser parser;
> > +	struct gen_pci *pci;
> > +	const __be32 *reg;
> > +	const struct of_device_id *of_id;
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> 
> Could you try to move almost all of this function into gen_pci_setup()?
> I suspect this will make it easier later to share this driver with other
> architectures.

I'll take a look. If we get rid of nr_controllers, as suggested later on,
the line between probe and setup is somewhat blurred.

> > +
> > +	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.

Will

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

[Arnd Bergmann]

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:
> > On Wednesday 12 February 2014 20:16:11 Will Deacon wrote:
> > > +- 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.

> > > 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.

> > > +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.

> > > +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.

> > > +
> > > +	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

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

[Will Deacon]

On Thu, Feb 13, 2014 at 11:47:46AM +0000, Arnd Bergmann wrote:
> On Thursday 13 February 2014 11:04:02 Will Deacon wrote:
> > On Wed, Feb 12, 2014 at 08:59:46PM +0000, Arnd Bergmann wrote:
> > > On Wednesday 12 February 2014 20:16:11 Will Deacon wrote:
> > > > +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 :(

I can allocate the resources dynamically as I parse them, not a problem at
all.

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

Yes, I fully expect this to be iterative.

> > > > +	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;
> > > > +	}
> > > 
> > > 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.

Okey doke.

> > > > +	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.

Yes, that looks sensible. An alternative is to create an hw_pci in
pci_host_bridge_register with nr_controllers = 1 then call
pci_common_init_dev. It would remove slightly more code, but obviously ties
the thing to arm.

Will

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

[Arnd Bergmann]

On Thursday 13 February 2014 12:00:42 Will Deacon wrote:
> > 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.
> 
> Yes, that looks sensible. An alternative is to create an hw_pci in
> pci_host_bridge_register with nr_controllers = 1 then call
> pci_common_init_dev. It would remove slightly more code, but obviously ties
> the thing to arm.

You'd still need to pass all the contents of the hw_pci struct that
get copied into pci_sys_data, so that's not different from passing
hw_pci as we do today.

	Arnd

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

[Kumar Gala]

On Feb 12, 2014, at 2:16 PM, Will Deacon <will.deacon@arm.com> wrote:

> 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.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> .../devicetree/bindings/pci/arm-generic-pci.txt    |  51 ++++
> drivers/pci/host/Kconfig                           |   7 +
> drivers/pci/host/Makefile                          |   1 +
> drivers/pci/host/pci-arm-generic.c                 | 318 +++++++++++++++++++++
> 4 files changed, 377 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/arm-generic-pci.txt
> create mode 100644 drivers/pci/host/pci-arm-generic.c
> 
> diff --git a/Documentation/devicetree/bindings/pci/arm-generic-pci.txt b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
> new file mode 100644
> index 000000000000..cc7a35ecfa2d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
> @@ -0,0 +1,51 @@
> +* ARM 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:
> +
> +- compatible     : Must be "arm,pci-cam-generic" or "arm,pci-ecam-generic"
> +                   depending on the layout of configuration space (CAM vs
> +                   ECAM respectively)

What’s arm specific here?  I don’t have a great suggestion, but seems odd for this to be vendor prefixed with "arm".

> +
> +- ranges         : As described in IEEE Std 1275-1994, but must provide
> +                   at least a definition of one or both of IO and Memory
> +                   Space.
> +
> +- #address-cells : Must be 3
> +
> +- #size-cells    : Must be 2
> +
> +- reg            : The Configuration Space base address, as accessed by the
> +                   parent bus.

Isn’t the size fixed here for cam or ecam?

> +
> +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>

Examples are always nice :)

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

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

[Will Deacon]

On Wed, Feb 12, 2014 at 09:51:48PM +0000, Kumar Gala wrote:
> 
> On Feb 12, 2014, at 2:16 PM, Will Deacon <will.deacon@arm.com> wrote:
> 
> > 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.
> > 
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> > .../devicetree/bindings/pci/arm-generic-pci.txt    |  51 ++++
> > drivers/pci/host/Kconfig                           |   7 +
> > drivers/pci/host/Makefile                          |   1 +
> > drivers/pci/host/pci-arm-generic.c                 | 318 +++++++++++++++++++++
> > 4 files changed, 377 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pci/arm-generic-pci.txt
> > create mode 100644 drivers/pci/host/pci-arm-generic.c
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/arm-generic-pci.txt b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
> > new file mode 100644
> > index 000000000000..cc7a35ecfa2d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
> > @@ -0,0 +1,51 @@
> > +* ARM 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:
> > +
> > +- compatible     : Must be "arm,pci-cam-generic" or "arm,pci-ecam-generic"
> > +                   depending on the layout of configuration space (CAM vs
> > +                   ECAM respectively)
> 
> What’s arm specific here?  I don’t have a great suggestion, but seems odd
> for this to be vendor prefixed with "arm".

Happy to change it, but I'm also struggling for names. Maybe "linux,..."?

> > +- ranges         : As described in IEEE Std 1275-1994, but must provide
> > +                   at least a definition of one or both of IO and Memory
> > +                   Space.
> > +
> > +- #address-cells : Must be 3
> > +
> > +- #size-cells    : Must be 2
> > +
> > +- reg            : The Configuration Space base address, as accessed by the
> > +                   parent bus.
> 
> Isn’t the size fixed here for cam or ecam?

Yes, which is why reg just specifies the base address.

> > +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>
> 
> Examples are always nice :)

Not in this case! kvmtool generates the following:

	pci {
		#address-cells = <0x3>;
		#size-cells = <0x2>;
		#interrupt-cells = <0x1>;
		compatible = "arm,pci-cam-generic";
		reg = <0x0 0x40000000>;
		ranges = <0x1000000 0x0 0x0 0x0 0x0 0x0 0x10000 0x2000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000>;
		interrupt-map = <0x0 0x0 0x0 0x1 0x1 0x0 0x4 0x1 0x800 0x0 0x0 0x1 0x1 0x0 0x5 0x1 0x1000 0x0 0x0 0x1 0x1 0x0 0x6 0x1 0x1800 0x0 0x0 0x1 0x1 0x0 0x7 0x1>;
		interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
	};

I can add it if you like, but it looks like a random bunch of numbers to me.

Will

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

[Kumar Gala]

On Feb 13, 2014, at 5:07 AM, Will Deacon <will.deacon@arm.com> wrote:

> On Wed, Feb 12, 2014 at 09:51:48PM +0000, Kumar Gala wrote:
>> 
>> On Feb 12, 2014, at 2:16 PM, Will Deacon <will.deacon@arm.com> wrote:
>> 
>>> 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.
>>> 
>>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>>> ---
>>> .../devicetree/bindings/pci/arm-generic-pci.txt    |  51 ++++
>>> drivers/pci/host/Kconfig                           |   7 +
>>> drivers/pci/host/Makefile                          |   1 +
>>> drivers/pci/host/pci-arm-generic.c                 | 318 +++++++++++++++++++++
>>> 4 files changed, 377 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/pci/arm-generic-pci.txt
>>> create mode 100644 drivers/pci/host/pci-arm-generic.c
>>> 
>>> diff --git a/Documentation/devicetree/bindings/pci/arm-generic-pci.txt b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
>>> new file mode 100644
>>> index 000000000000..cc7a35ecfa2d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
>>> @@ -0,0 +1,51 @@
>>> +* ARM 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:
>>> +
>>> +- compatible     : Must be "arm,pci-cam-generic" or "arm,pci-ecam-generic"
>>> +                   depending on the layout of configuration space (CAM vs
>>> +                   ECAM respectively)
>> 
>> What’s arm specific here?  I don’t have a great suggestion, but seems odd
>> for this to be vendor prefixed with "arm".
> 
> Happy to change it, but I'm also struggling for names. Maybe "linux,…"?

I was thinking that as well, I’d say go with “linux,”.

>>> +- ranges         : As described in IEEE Std 1275-1994, but must provide
>>> +                   at least a definition of one or both of IO and Memory
>>> +                   Space.
>>> +
>>> +- #address-cells : Must be 3
>>> +
>>> +- #size-cells    : Must be 2
>>> +
>>> +- reg            : The Configuration Space base address, as accessed by the
>>> +                   parent bus.
>> 
>> Isn’t the size fixed here for cam or ecam?
> 
> Yes, which is why reg just specifies the base address.

Huh?  The reg property clearly has the size in it (as shown in the example below).  I guess I was just asking for the description here to say what the size was for the 2 compatibles since its fixed and known.

> 
>>> +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>
>> 
>> Examples are always nice :)
> 
> Not in this case! kvmtool generates the following:
> 
> 	pci {
> 		#address-cells = <0x3>;
> 		#size-cells = <0x2>;
> 		#interrupt-cells = <0x1>;
> 		compatible = "arm,pci-cam-generic";
> 		reg = <0x0 0x40000000>;
> 		ranges = <0x1000000 0x0 0x0 0x0 0x0 0x0 0x10000 0x2000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000>;
> 		interrupt-map = <0x0 0x0 0x0 0x1 0x1 0x0 0x4 0x1 0x800 0x0 0x0 0x1 0x1 0x0 0x5 0x1 0x1000 0x0 0x0 0x1 0x1 0x0 0x6 0x1 0x1800 0x0 0x0 0x1 0x1 0x0 0x7 0x1>;
> 		interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
> 	};
> 
> I can add it if you like, but it looks like a random bunch of numbers to me.

You could clean it up a bit to be human readable even if its kvmtool that’s creating it.

	pci {
		compatible = "arm,pci-cam-generic”;
		#address-cells = <3>;
		#size-cells = <2>;
		#interrupt-cells = <1>
		reg = <0x0 0x40000000>;
		ranges = <
			0x1000000 0x0 0x00000000 0x0 0x00000000 0x0 0x00010000
			0x2000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000
			>;
		interrupt-map = <
			...
				>;

		interrupt-map-mask = <0xf800 0x0 0x0 0x7>;

		


> 
> Will

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

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

[Will Deacon]

On Thu, Feb 13, 2014 at 04:22:25PM +0000, Kumar Gala wrote:
> On Feb 13, 2014, at 5:07 AM, Will Deacon <will.deacon@arm.com> wrote:
> >>> +
> >>> +- compatible     : Must be "arm,pci-cam-generic" or "arm,pci-ecam-generic"
> >>> +                   depending on the layout of configuration space (CAM vs
> >>> +                   ECAM respectively)
> >> 
> >> What’s arm specific here?  I don’t have a great suggestion, but seems odd
> >> for this to be vendor prefixed with "arm".
> > 
> > Happy to change it, but I'm also struggling for names. Maybe "linux,…"?
> 
> I was thinking that as well, I’d say go with “linux,”.

Great, I'll make that change.

> >>> +- ranges         : As described in IEEE Std 1275-1994, but must provide
> >>> +                   at least a definition of one or both of IO and Memory
> >>> +                   Space.
> >>> +
> >>> +- #address-cells : Must be 3
> >>> +
> >>> +- #size-cells    : Must be 2
> >>> +
> >>> +- reg            : The Configuration Space base address, as accessed by the
> >>> +                   parent bus.
> >> 
> >> Isn’t the size fixed here for cam or ecam?
> > 
> > Yes, which is why reg just specifies the base address.
> 
> Huh?  The reg property clearly has the size in it (as shown in the example
> below).  I guess I was just asking for the description here to say what
> the size was for the 2 compatibles since its fixed and known.

Actually, the example just has a 64-bit CPU physical address with no size.
Do I have to add a size to the binding? It's not at all useful for the driver.

> >> Examples are always nice :)
> > 
> > Not in this case! kvmtool generates the following:
> > 
> > 	pci {
> > 		#address-cells = <0x3>;
> > 		#size-cells = <0x2>;
> > 		#interrupt-cells = <0x1>;
> > 		compatible = "arm,pci-cam-generic";
> > 		reg = <0x0 0x40000000>;
> > 		ranges = <0x1000000 0x0 0x0 0x0 0x0 0x0 0x10000 0x2000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000>;
> > 		interrupt-map = <0x0 0x0 0x0 0x1 0x1 0x0 0x4 0x1 0x800 0x0 0x0 0x1 0x1 0x0 0x5 0x1 0x1000 0x0 0x0 0x1 0x1 0x0 0x6 0x1 0x1800 0x0 0x0 0x1 0x1 0x0 0x7 0x1>;
> > 		interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
> > 	};
> > 
> > I can add it if you like, but it looks like a random bunch of numbers to me.
> 
> You could clean it up a bit to be human readable even if its kvmtool that’s creating it.
> 
> 	pci {
> 		compatible = "arm,pci-cam-generic”;
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		#interrupt-cells = <1>
> 		reg = <0x0 0x40000000>;
> 		ranges = <
> 			0x1000000 0x0 0x00000000 0x0 0x00000000 0x0 0x00010000
> 			0x2000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000
> 			>;
> 		interrupt-map = <
> 			...
> 				>;
> 
> 		interrupt-map-mask = <0xf800 0x0 0x0 0x7>;

Sure, if you think it helps.

Will

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

[Arnd Bergmann]

On Thursday 13 February 2014 10:22:25 Kumar Gala wrote:
> On Feb 13, 2014, at 5:07 AM, Will Deacon <will.deacon@arm.com> wrote:
> > Happy to change it, but I'm also struggling for names. Maybe "linux,…"?
>
> I was thinking that as well, I’d say go with “linux,”.

I see nothing linux specific in there. I would only use that namespace
for things we don't expect to work with another OS.

> >>> +- ranges         : As described in IEEE Std 1275-1994, but must provide
> >>> +                   at least a definition of one or both of IO and Memory
> >>> +                   Space.
> >>> +
> >>> +- #address-cells : Must be 3
> >>> +
> >>> +- #size-cells    : Must be 2
> >>> +
> >>> +- reg            : The Configuration Space base address, as accessed by the
> >>> +                   parent bus.
> >> 
> >> Isn’t the size fixed here for cam or ecam?
> > 
> > Yes, which is why reg just specifies the base address.
> 
> Huh?  The reg property clearly has the size in it (as shown in the example below).
> I guess I was just asking for the description here to say what the size was for
> the 2 compatibles since its fixed and known.

It's still an open question whether the config space in the reg property should
cover all 256 buses or just the ones in the bus-range. In the latter case,
it would be variable (but predictable) size.

> You could clean it up a bit to be human readable even if its kvmtool that’s creating it.
> 
> 	pci {
> 		compatible = "arm,pci-cam-generic”;
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		#interrupt-cells = <1>
> 		reg = <0x0 0x40000000>;
> 		ranges = <
> 			0x1000000 0x0 0x00000000 0x0 0x00000000 0x0 0x00010000
> 			0x2000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000
> 			>;

Make it 

 		ranges = <0x1000000 0x0 0x00000000 0x0 0x00000000 0x0 0x00010000>,
 			 <0x2000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000>;

and it will be perfect ;-)

	Arnd

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

[Mark Rutland]

[adding devicetree, Grant, Rob]

Apologies to all those who aren't too bothered about the naming, I'm
going to derail this subthread for a general device tree policy
discussion.

On Thu, Feb 13, 2014 at 04:28:20PM +0000, Arnd Bergmann wrote:
> On Thursday 13 February 2014 10:22:25 Kumar Gala wrote:
> > On Feb 13, 2014, at 5:07 AM, Will Deacon <will.deacon@arm.com> wrote:
> > > Happy to change it, but I'm also struggling for names. Maybe "linux,…"?
> >
> > I was thinking that as well, I’d say go with “linux,”.
> 
> I see nothing linux specific in there. I would only use that namespace
> for things we don't expect to work with another OS.

Let's take a stop back. This is an instance of a wider problem, and to
be honest it's a rather trivial one. As usual, because there's been no
policy there's no consistency, and everyone's got their own favourite.

All we need is a policy for how generic bindings are named/prefixed. As
long as this is consistent and unlikely to cause problems it doesn't
really matter what that specific policy is.

Regardless, we need to support our existing bindings.

So far we have instances of (in v3.14-rc2):

A) No prefix

  $ git grep 'compatible\s*=\s*"[^,]*"' -- Documentation/devicetree | \
    awk '{ print $NF }' | sort -u | wc -l
    96

  $ git grep 'compatible\s*=\s*"[^,]*"' -- arch/*/boot/dts | \
    awk ' { print $NF }' | sort -u | wc -l
    233

  These include some commonly use bindings like "fixed-clock",
  "regulator-fixed", and (everybody's favourite) "simple-bus". The
  "ns16550" binding falls here too.
 
  There are subdivisions within this, like "simple-*" and "*-generic".
  There are three "simple-" prefixed variants, perhaps that's worth it's
  own class?
 
  There are also some dubious ones like "ldo4" that are probably only
  appearing in sub-nodes and probably don't matter that much for this
  discussion.


B) "linux," prefixed

  $ git grep 'compatible\s*=\s*"linux,.*"' -- Documentation/devicetree | \
    awk ' { print $NF }' | sort -u | wc -l
    4

  $ git grep 'compatible\s*=\s*"linux,.*"' -- arch/*/boot/dts | \
    awk ' {print $NF }' | sort -u | wc -l
    1

  These include:
  * "linux,hdmi-audio"
  * "linux,spdif-dir"
  * "linux,spdif-dit"
  * "linux,wdt-gpio"

Is there another class I've missed?

If a binding originates for Linux, a "linux," prefix doesn't seem that
bad to me. It's just a namespace, it doesn't have to mean that it's only
ever going to work for Linux, and two such bindings within Linux
shouldn't be able to collide.

As long as we choose something that reduces the possibility of name
collisions (several people might come up with incompatible "pci"
bindings) then we're fine regardless of the particular prefix or lack
thereof. A "generic," prefix feels like it's too easy to collide with if
another community also creates bindings.

Thoughts?

One final option for generic bindings would be a prefix per-author (e.g.
in this case the compatible string could be "will-deacon,pci"). That
might lead to the fewest namespace issues in future, and helps to
immortalise/demonise the original author of any binding claiming to be
generic. Patch below.

Thanks,
Mark.

---->8----
>From 6ed3e3564199ed6dbd8782a740d4067fb1e6d3b6 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Thu, 13 Feb 2014 17:50:33 +0000
Subject: [PATCH] Documentation: devicetree: add vendor prefix for Will Deacon

This patch adds a vendor prefix for Will Deacon, for use in generic
hardware bindings originating from Will Deacon.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 3f900cd..b945e74 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -88,6 +88,7 @@ toumaz	Toumaz
 v3	V3 Semiconductor
 via	VIA Technologies, Inc.
 winbond Winbond Electronics corp.
+will-deacon	Will Deacon MEng
 wlf	Wolfson Microelectronics
 wm	Wondermedia Technologies, Inc.
 xlnx	Xilinx
-- 
1.8.1.1

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

[Jason Gunthorpe]

On Thu, Feb 13, 2014 at 05:28:20PM +0100, Arnd Bergmann wrote:

> > Huh?  The reg property clearly has the size in it (as shown in the
> > example below).  I guess I was just asking for the description
> > here to say what the size was for the 2 compatibles since its
> > fixed and known.
> 
> It's still an open question whether the config space in the reg
> property should cover all 256 buses or just the ones in the
> bus-range. In the latter case, it would be variable (but
> predictable) size.

The 'describe the hardware principle' says the reg should be the
entire available ECAM/CAM region the hardware is able to support.

This may be less than 256 busses, as ECAM allows the implementor to
select how many upper address bits are actually supported.

IMHO, the bus-range should be used to indicate the range of busses
discovered by the firmware, but we have historically tweaked it to
indicate the max range of bus numbers available on this bus (I think
to support the hack where two physical PCI domains were roughly glued
into a single Linux domain).

Which is not necessary when the DT stanza maps 1:1 into a PCI
domain. The driver default for bus-range should just be 0 to 255, and
it shouldn't be included in most cases.

The max busnr resource passed to the PCI core should be the lower of
the busnr property and the reg limit, IMHO.

The issue with burning VM in Linux is a Linux issue and shouldn't leak
into the DT... Ideally the solution here would be to have the PCI core
call back to the host driver when it allocates/deallocates a bus
number and then the driver can manage the config space VM mapping on a
bus by bus basis.

Jason

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

[Will Deacon]

On Thu, Feb 13, 2014 at 06:26:54PM +0000, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2014 at 05:28:20PM +0100, Arnd Bergmann wrote:
> 
> > > Huh?  The reg property clearly has the size in it (as shown in the
> > > example below).  I guess I was just asking for the description
> > > here to say what the size was for the 2 compatibles since its
> > > fixed and known.
> > 
> > It's still an open question whether the config space in the reg
> > property should cover all 256 buses or just the ones in the
> > bus-range. In the latter case, it would be variable (but
> > predictable) size.
> 
> The 'describe the hardware principle' says the reg should be the
> entire available ECAM/CAM region the hardware is able to support.
> 
> This may be less than 256 busses, as ECAM allows the implementor to
> select how many upper address bits are actually supported.

Ok, but the ECAM/CAM base always corresponds to bus 0, right?

> IMHO, the bus-range should be used to indicate the range of busses
> discovered by the firmware, but we have historically tweaked it to
> indicate the max range of bus numbers available on this bus (I think
> to support the hack where two physical PCI domains were roughly glued
> into a single Linux domain).

Ok, so this answers Kumar's point about the reg property. I'll augment it
with a size.

Will

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

[Jason Gunthorpe]

On Thu, Feb 13, 2014 at 07:53:17PM +0000, Will Deacon wrote:
> > This may be less than 256 busses, as ECAM allows the implementor to
> > select how many upper address bits are actually supported.
> 
> Ok, but the ECAM/CAM base always corresponds to bus 0, right?

Yes, or it isn't ECAM.

> Ok, so this answers Kumar's point about the reg property. I'll augment it
> with a size.

Don't forget to request_region it as well...

Jason

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

[Arnd Bergmann]

On Thursday 13 February 2014 19:53:17 Will Deacon wrote:
> On Thu, Feb 13, 2014 at 06:26:54PM +0000, Jason Gunthorpe wrote:
> > On Thu, Feb 13, 2014 at 05:28:20PM +0100, Arnd Bergmann wrote:
> > 
> > > > Huh?  The reg property clearly has the size in it (as shown in the
> > > > example below).  I guess I was just asking for the description
> > > > here to say what the size was for the 2 compatibles since its
> > > > fixed and known.
> > > 
> > > It's still an open question whether the config space in the reg
> > > property should cover all 256 buses or just the ones in the
> > > bus-range. In the latter case, it would be variable (but
> > > predictable) size.
> > 
> > The 'describe the hardware principle' says the reg should be the
> > entire available ECAM/CAM region the hardware is able to support.
> > 
> > This may be less than 256 busses, as ECAM allows the implementor to
> > select how many upper address bits are actually supported.
> 
> Ok, but the ECAM/CAM base always corresponds to bus 0, right?

Ah, plus I suppose it ought to be a power-of-two size?

> > IMHO, the bus-range should be used to indicate the range of busses
> > discovered by the firmware, but we have historically tweaked it to
> > indicate the max range of bus numbers available on this bus (I think
> > to support the hack where two physical PCI domains were roughly glued
> > into a single Linux domain).

There is an interesting point about the domain assignment, brought to
my attention by Russell's comment about the hw_pci struct: If we want
to support arbitrary combinations of pci host bridges described in DT,
we need a better policy to decide what domain to use. The approaches
I've seen so far are:

1. We assume each host bridge in DT is a domain by itself. I think
we do that for all DT probed bridges on ARM (aside from shmobile)
at the moment. In some cases, the the host bridge is a really a
fiction made up by the host driver to couple various identical
but independent PCIe root ports, but the same fiction is shared
between DT and the PCI core view of it. This requires that we
enable the PCI domain code unconditionally, and breaks all user
space that doesn't understand domains (this should be rare but
can still exist for x86 based software).

2. The architecture or platform code decides and uses a method equivalent
to ARM's pci_common_init_dev() after it has found all host bridges.
The architecture "knows" how many domains it wants and calls
pci_common_init_dev() for each domain, and then the setup() callbacks
grab as many buses as they need within the domain. For a generic
multiplatform kernel, this means we need to add a top-level driver
that looks at all pci hosts in DT before any of them are probed.
It also means the pci host drivers can't be loadable modules.

3. We assume there is only one domain, and require each host bridge
in DT to specify a bus-range that is a subset of the available 256
bus numbers. This should work for anything but really big systems
with many hot-pluggable ports, since we need to reserve a few bus
numbers on each port for hotplugging.

4. Like 3, but start a new domain if the bus-range properties don't
fit in the existing domains.

5. Like 3, but specify a generic "pci-domain" property for DT
that allows putting host bridges into explicit domains in
a predictable way.

	Arnd

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

[Liviu Dudau]

On Fri, Feb 14, 2014 at 10:59:06AM +0100, Arnd Bergmann wrote:
> On Thursday 13 February 2014 19:53:17 Will Deacon wrote:
> > On Thu, Feb 13, 2014 at 06:26:54PM +0000, Jason Gunthorpe wrote:
> > > On Thu, Feb 13, 2014 at 05:28:20PM +0100, Arnd Bergmann wrote:
> > > 
> > > > > Huh?  The reg property clearly has the size in it (as shown in the
> > > > > example below).  I guess I was just asking for the description
> > > > > here to say what the size was for the 2 compatibles since its
> > > > > fixed and known.
> > > > 
> > > > It's still an open question whether the config space in the reg
> > > > property should cover all 256 buses or just the ones in the
> > > > bus-range. In the latter case, it would be variable (but
> > > > predictable) size.
> > > 
> > > The 'describe the hardware principle' says the reg should be the
> > > entire available ECAM/CAM region the hardware is able to support.
> > > 
> > > This may be less than 256 busses, as ECAM allows the implementor to
> > > select how many upper address bits are actually supported.
> > 
> > Ok, but the ECAM/CAM base always corresponds to bus 0, right?
> 
> Ah, plus I suppose it ought to be a power-of-two size?
> 
> > > IMHO, the bus-range should be used to indicate the range of busses
> > > discovered by the firmware, but we have historically tweaked it to
> > > indicate the max range of bus numbers available on this bus (I think
> > > to support the hack where two physical PCI domains were roughly glued
> > > into a single Linux domain).
> 
> There is an interesting point about the domain assignment, brought to
> my attention by Russell's comment about the hw_pci struct: If we want
> to support arbitrary combinations of pci host bridges described in DT,
> we need a better policy to decide what domain to use. The approaches
> I've seen so far are:
> 
> 1. We assume each host bridge in DT is a domain by itself. I think
> we do that for all DT probed bridges on ARM (aside from shmobile)
> at the moment. In some cases, the the host bridge is a really a
> fiction made up by the host driver to couple various identical
> but independent PCIe root ports, but the same fiction is shared
> between DT and the PCI core view of it. This requires that we
> enable the PCI domain code unconditionally, and breaks all user
> space that doesn't understand domains (this should be rare but
> can still exist for x86 based software).
> 
> 2. The architecture or platform code decides and uses a method equivalent
> to ARM's pci_common_init_dev() after it has found all host bridges.
> The architecture "knows" how many domains it wants and calls
> pci_common_init_dev() for each domain, and then the setup() callbacks
> grab as many buses as they need within the domain. For a generic
> multiplatform kernel, this means we need to add a top-level driver
> that looks at all pci hosts in DT before any of them are probed.
> It also means the pci host drivers can't be loadable modules.
> 
> 3. We assume there is only one domain, and require each host bridge
> in DT to specify a bus-range that is a subset of the available 256
> bus numbers. This should work for anything but really big systems
> with many hot-pluggable ports, since we need to reserve a few bus
> numbers on each port for hotplugging.
> 
> 4. Like 3, but start a new domain if the bus-range properties don't
> fit in the existing domains.
> 
> 5. Like 3, but specify a generic "pci-domain" property for DT
> that allows putting host bridges into explicit domains in
> a predictable way.

What I'm going to suggest in my v2 patch (hope to send it before Monday)
is a new API in the generic PCI code that will allow you to create a
host bridge in a new domain or in the existing domain, with the management
of the domain number being done in the generic code.

Something like:

  int create_hostbridge_in_new_domain(....);
  int create_hostbridge(....);

with the functions being wrappers around the pci_hostbridge_of_init function
that I'm introducing.

What do you think?

Best regards,
Liviu

> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Arnd Bergmann]

On Friday 14 February 2014 22:00:47 Liviu Dudau wrote:
> 
> What I'm going to suggest in my v2 patch (hope to send it before Monday)
> is a new API in the generic PCI code that will allow you to create a
> host bridge in a new domain or in the existing domain, with the management
> of the domain number being done in the generic code.
> 
> Something like:
> 
>   int create_hostbridge_in_new_domain(....);
>   int create_hostbridge(....);
> 
> with the functions being wrappers around the pci_hostbridge_of_init function
> that I'm introducing.
> 
> What do you think?

Not sure. It would still keep the decision about whether to use a
new domain or not in the host bridge driver, but that doesn't seem
like the right place. The same driver might be used in different
ways based on what is around it.

I've also had a look at the MIPS implementation now, which has its
own way of dealing with this, see arch/mips/pci/pci.c.

There was also an interesting comment in the MIPS header file:

        /* For compatibility with current (as of July 2003) pciutils
           and XFree86. Eventually will be removed. */
        unsigned int need_domain_info;

This is over ten years old, so I wonder if we can start assuming that
domains work out of the box now. All references to problems from 
PCI domains are about old code (ca. pre-2007) that doesn't understand
nonzero domains and that has since been fixed. I am pretty sure we
don't need to ever worry about stuffing multiple host bridges into
a domain other than the first one, and I also doubt we have to worry
about the problem at all on arm64 as we won't run old binaries on it
(or arm32 compat mode binaries that need to manage PCI devices).

Can anyone with more experience on the subject than me (Bjorn,
Russell, Jason, ...) think of a reason why we would not want to
just use a new domain for every host bridge we find?

If we do need to stuff multiple buses into one domain, how about
using this approach in pci_hostbridge_of_init():

  The first time we add a host bridge, scan the entire DT for
  devices setting device_type="pci". If there is actually more
  than one host bridge, check all "bus-range" properties to
  see if they overlap. If there is no overlap or only one
  bridge, don't use domains.
  If there is any overlap at all, or there are multiple host
  bridge and one of them does not have a bus-range property,
  use a separate domain per host bridge.
  Remember the function was called before so it doesn't
  have to scan the DT again, and count the domain locally.

Does this make sense?

	Arnd

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

[Jason Gunthorpe]

On Sat, Feb 15, 2014 at 02:03:26PM +0100, Arnd Bergmann wrote:

> Can anyone with more experience on the subject than me (Bjorn,
> Russell, Jason, ...) think of a reason why we would not want to
> just use a new domain for every host bridge we find?

I personaly think we can safely move away from stuffing multiple host
bridges into a single domain for DT cases. The reasons for doing this
have long since been superseded.

Most importantly, I have a feeling keeping a 1:1 relationship between
domain and driver will making building a proper modular and hot
pluggable host driver infrastructure in the PCI core significantly
simpler. The domain object gives a nice natural place to put things in
sysfs, a natural place to keep function pointers and avoids all the
messy questions of what happens if probing overlaps bus numbers, how
do you number things, how do you hot plug downstream busses, etc.

Having a PCI core driver infrastructure that supports both 'as a
domain' and 'as a bunch of busses' seems much more complex, and I
can't really identify what is being gained by continuing to support
this.

As far as I know the host bridge stuffing is something that was
created before domains to solve the problem on embedded of multiple
PCI host bridges on a SOC/System Controller. I know I have used it
that way in the distant past (for MIPS).

However today PCI-SIG has a standard way to describe multi-port
root-complexes in config space, so we should not need to use the
multi-bus hack. SOCs with non-compliant HW that *really* need single
domain can get there: mvebu shows how to write a driver that provides
a software version of the missing hardware elements. Pushing mess like
this out of the core code seems like a good strategy.

The only reason I can think of to avoid using a domain is if Linux has
to interface with external firmware that uses bus:device.function
notation for coding information. (eg Intel-MP tables on x86 encode
interrupt routing use B:D.F) In this case Linux would need a way to
map between Linux B:D.F and the Firwmare B:D.F, or it would need to
use the Firmware B:D.F layout. But this argument does not apply to DT
systems as DT encodes the domain too. Presumably ACPI will be the
same.

Also, bear in mind we now already have multi-domain host drivers for
ARM, so the multi-platform kernels need to have this option turned on.

So the Liviu, I would say the API should be similar to what we see in
other OF enabled driver bases subsystems, call the core code with a
platform_device pointer and function_ops pointer and have the core
code create a domain, figure out the domain # from the DT (via
aliases?), and so on.

Jason

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

[Arnd Bergmann]

On Tuesday 18 February 2014 10:41:25 Jason Gunthorpe wrote:
> On Sat, Feb 15, 2014 at 02:03:26PM +0100, Arnd Bergmann wrote:
> 
> > Can anyone with more experience on the subject than me (Bjorn,
> > Russell, Jason, ...) think of a reason why we would not want to
> > just use a new domain for every host bridge we find?
> 
> I personaly think we can safely move away from stuffing multiple host
> bridges into a single domain for DT cases. The reasons for doing this
> have long since been superseded.

Ok, that would definitely be the simplest answer. :)

> As far as I know the host bridge stuffing is something that was
> created before domains to solve the problem on embedded of multiple
> PCI host bridges on a SOC/System Controller. I know I have used it
> that way in the distant past (for MIPS).

Apple have also used the same trick on their G5 Macs, presumably
to simplify things for OS9 and OS-X, but even at the time making
it harder for Linux.
 
> However today PCI-SIG has a standard way to describe multi-port
> root-complexes in config space, so we should not need to use the
> multi-bus hack. SOCs with non-compliant HW that *really* need single
> domain can get there: mvebu shows how to write a driver that provides
> a software version of the missing hardware elements. Pushing mess like
> this out of the core code seems like a good strategy.

Ok.

> The only reason I can think of to avoid using a domain is if Linux has
> to interface with external firmware that uses bus:device.function
> notation for coding information. (eg Intel-MP tables on x86 encode
> interrupt routing use B:D.F) In this case Linux would need a way to
> map between Linux B:D.F and the Firwmare B:D.F, or it would need to
> use the Firmware B:D.F layout. But this argument does not apply to DT
> systems as DT encodes the domain too. Presumably ACPI will be the
> same.

ACPI definitely has a concept of domains, as I noticed when looking
at the x86 ACPI PCI probing code.

> So the Liviu, I would say the API should be similar to what we see in
> other OF enabled driver bases subsystems, call the core code with a
> platform_device pointer and function_ops pointer and have the core
> code create a domain, figure out the domain # from the DT (via
> aliases?), and so on.

Do we even need stable domain numbers? If we do, aliases sound fine.
A more complex method would be to sort them by MMIO window address
or perhaps by phandle.

	Arnd

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

[Jason Gunthorpe]

On Tue, Feb 18, 2014 at 07:25:35PM +0100, Arnd Bergmann wrote:

> Apple have also used the same trick on their G5 Macs, presumably
> to simplify things for OS9 and OS-X, but even at the time making
> it harder for Linux.

I actually think some x86's have done the same thing, however the
firmware hides all that from the OS and the OS just sees a normal PCI
environment. So if we can't hide the mess in firmware, the next best
place is in drivers?

> Do we even need stable domain numbers? If we do, aliases sound fine.
> A more complex method would be to sort them by MMIO window address
> or perhaps by phandle.

PCI ordering has been a bane in the past on x86, so I think stable
domain numbers is certainly desirable. Particularly since the domain
number may be influenced by module load order :( Alises followed by
sorting by phandle sounds great to me.

Jason

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

[Arnd Bergmann]

On Tuesday 18 February 2014 11:45:42 Jason Gunthorpe wrote:
> 
> > Do we even need stable domain numbers? If we do, aliases sound fine.
> > A more complex method would be to sort them by MMIO window address
> > or perhaps by phandle.
> 
> PCI ordering has been a bane in the past on x86, so I think stable
> domain numbers is certainly desirable. Particularly since the domain
> number may be influenced by module load order  Alises followed by
> sorting by phandle sounds great to me.

Ok, makes sense. Let's make sure we come up with a stable numbering
enforced by DT then, and warn when it's not there (or refuse to
work even).

	Arnd

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

[Liviu Dudau]

On Tue, Feb 18, 2014 at 10:41:25AM -0700, Jason Gunthorpe wrote:
> On Sat, Feb 15, 2014 at 02:03:26PM +0100, Arnd Bergmann wrote:
> 
> > Can anyone with more experience on the subject than me (Bjorn,
> > Russell, Jason, ...) think of a reason why we would not want to
> > just use a new domain for every host bridge we find?
> 
> I personaly think we can safely move away from stuffing multiple host
> bridges into a single domain for DT cases. The reasons for doing this
> have long since been superseded.
> 
> Most importantly, I have a feeling keeping a 1:1 relationship between
> domain and driver will making building a proper modular and hot
> pluggable host driver infrastructure in the PCI core significantly
> simpler. The domain object gives a nice natural place to put things in
> sysfs, a natural place to keep function pointers and avoids all the
> messy questions of what happens if probing overlaps bus numbers, how
> do you number things, how do you hot plug downstream busses, etc.
> 
> Having a PCI core driver infrastructure that supports both 'as a
> domain' and 'as a bunch of busses' seems much more complex, and I
> can't really identify what is being gained by continuing to support
> this.
> 
> As far as I know the host bridge stuffing is something that was
> created before domains to solve the problem on embedded of multiple
> PCI host bridges on a SOC/System Controller. I know I have used it
> that way in the distant past (for MIPS).
> 
> However today PCI-SIG has a standard way to describe multi-port
> root-complexes in config space, so we should not need to use the
> multi-bus hack. SOCs with non-compliant HW that *really* need single
> domain can get there: mvebu shows how to write a driver that provides
> a software version of the missing hardware elements. Pushing mess like
> this out of the core code seems like a good strategy.
> 
> The only reason I can think of to avoid using a domain is if Linux has
> to interface with external firmware that uses bus:device.function
> notation for coding information. (eg Intel-MP tables on x86 encode
> interrupt routing use B:D.F) In this case Linux would need a way to
> map between Linux B:D.F and the Firwmare B:D.F, or it would need to
> use the Firmware B:D.F layout. But this argument does not apply to DT
> systems as DT encodes the domain too. Presumably ACPI will be the
> same.
> 
> Also, bear in mind we now already have multi-domain host drivers for
> ARM, so the multi-platform kernels need to have this option turned on.
> 
> So the Liviu, I would say the API should be similar to what we see in
> other OF enabled driver bases subsystems, call the core code with a
> platform_device pointer and function_ops pointer and have the core
> code create a domain, figure out the domain # from the DT (via
> aliases?), and so on.

I wish things were easier!

Lets look at the 'int pci_domain_nr(struct pci_bus *bus);' function. It is
used to obtain the domain number of the bus passed as an argument.

- include/linux/pci.h defines it as an inline function returning zero if
  !CONFIG_PCI || !CONFIG_PCI_DOMAINS. Otherwise it is silent on what the
  function might look like.

- alpha, ia64, microblaze, mips, powerpc and tile all define it as a cast
  of bus->sysdata to "struct pci_controller *" and then access a data
  member from there to get the domain number. But ... the pci_controller
  structure is different for each architecture, with few more members in
  addition that might be actually shared with generic framework code.

- arm, s390, sparc and x86 have all different names for their sysdata,
  but they all contain inside a member that is used for giving a domain
  back. sparc gets an honorary mention here for getting the API wrong
  and returning -ENXIO in certain conditions (not like the generic code
  cares).

That takes care of the implementation. But what about usage?

- drivers/pci/probe.c: pci_create_root_bus allocates a new bus structure,
  sets up the sysdata and ops member pointers and then goes straight
  into trying to find if the newly created bus doesn't already exist by
  using the bus number given as parameter and pci_domain_nr() with the
  new bus structure as argument. I'm trying really hard to figure out
  what was the intention here, but from the point of view of someone
  trying to implement the pci_domain_nr() function I have no idea what
  to return for a virgin bus structure that is not yet attached to any
  parent.

So I can see the intent of what Jason is proposing and I'm heading that
way myself, but I think I need to cleanup pci_create_root_bus first
(change the creation order between bridge and bus). And if someone has
a good idea on how to determine the domain # from DT we can pluck it
into the pcibios_root_bridge_prepare() function (either the generic
version or the arch specific one).

Best regards,
Liviu


> 
> Jason
> 

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

[Jason Gunthorpe]

On Wed, Feb 19, 2014 at 02:44:27AM +0000, Liviu Dudau wrote:
 
> I wish things were easier!

Yah, I've seen some of this complexity too..

A thought that crosses my head is to try and leave the entire arch
support intact and build a separate 'domain-driver-based' support
that completely replaces the arch support, a domain would use one or
the other but never both.

So pci_domain_nr could become:

  struct pci_domain_driver *drv = pci_get_domain_driver(bus);
  if (drv)
       return drv->domain_nr;
  else
       return _arch_pci_domain_nr(bus);

And other funcs would change effectively the same way.

> And if someone has a good idea on how to determine the domain # from
> DT we can pluck it into the pcibios_root_bridge_prepare() function
> (either the generic version or the arch specific one).

You can probably start with 'of_alias_get_id'..

Jason

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

[Arnd Bergmann]

On Wednesday 19 February 2014 02:44:27 Liviu Dudau wrote:
> On Tue, Feb 18, 2014 at 10:41:25AM -0700, Jason Gunthorpe wrote:
> 
> > So the Liviu, I would say the API should be similar to what we see in
> > other OF enabled driver bases subsystems, call the core code with a
> > platform_device pointer and function_ops pointer and have the core
> > code create a domain, figure out the domain # from the DT (via
> > aliases?), and so on.
> 
> I wish things were easier!
> 
> Lets look at the 'int pci_domain_nr(struct pci_bus *bus);' function. It is
> used to obtain the domain number of the bus passed as an argument.
> 
> - include/linux/pci.h defines it as an inline function returning zero if
>   !CONFIG_PCI || !CONFIG_PCI_DOMAINS. Otherwise it is silent on what the
>   function might look like.

I think we should come up with a default implementation that works for
any architecture using DT based probing, and requires PCI_DOMAINS to
be enabled.

> - alpha, ia64, microblaze, mips, powerpc and tile all define it as a cast
>   of bus->sysdata to "struct pci_controller *" and then access a data
>   member from there to get the domain number. But ... the pci_controller
>   structure is different for each architecture, with few more members in
>   addition that might be actually shared with generic framework code.

We do have the generic 'struct pci_host_bridge'. It would be trivial to
add a 'domain' field in there and use it in the generic implementation.

> - arm, s390, sparc and x86 have all different names for their sysdata,
>   but they all contain inside a member that is used for giving a domain
>   back. sparc gets an honorary mention here for getting the API wrong
>   and returning -ENXIO in certain conditions (not like the generic code
>   cares).

I would hope that for the generic case, we can actually remove the
use of 'sysdata' and replace it with common code that just looks
at pci_host_bridge. Some architectures (s390 in particular) will probably
need their own data to be added in sysdata, but overall the architectures
are really trying to do the same thing here. With the work that Bjorn
and others have done over the past few years, you already need much less
architecture specific code. I think it's time to get it to a point where
most architectures can do without any at all.

> That takes care of the implementation. But what about usage?
> 
> - drivers/pci/probe.c: pci_create_root_bus allocates a new bus structure,
>   sets up the sysdata and ops member pointers and then goes straight
>   into trying to find if the newly created bus doesn't already exist by
>   using the bus number given as parameter and pci_domain_nr() with the
>   new bus structure as argument. I'm trying really hard to figure out
>   what was the intention here, but from the point of view of someone
>   trying to implement the pci_domain_nr() function I have no idea what
>   to return for a virgin bus structure that is not yet attached to any
>   parent.

Right, this needs to be changed when moving the domain into pci_host_bridge.

> So I can see the intent of what Jason is proposing and I'm heading that
> way myself, but I think I need to cleanup pci_create_root_bus first
> (change the creation order between bridge and bus). And if someone has
> a good idea on how to determine the domain # from DT we can pluck it
> into the pcibios_root_bridge_prepare() function (either the generic
> version or the arch specific one).

How about the change below, to introduce a new pci_scan_domain() function
as a variant of pci_scan_root_bus()?

	Arnd

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5094943..9f2ec2f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1697,8 +1697,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
 {
 }
 
-struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+static struct pci_bus *__pci_create_root_bus(struct device *parent,
+		int domain, int bus, struct pci_ops *ops, void *sysdata,
+		struct list_head *resources)
 {
 	int error;
 	struct pci_host_bridge *bridge;
@@ -1716,7 +1717,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	b->sysdata = sysdata;
 	b->ops = ops;
 	b->number = b->busn_res.start = bus;
-	b2 = pci_find_bus(pci_domain_nr(b), bus);
+	b2 = pci_find_bus(domain, bus);
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */
 		dev_dbg(&b2->dev, "bus already known\n");
@@ -1727,6 +1728,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	if (!bridge)
 		goto err_out;
 
+	bridge->domain = domain;
 	bridge->dev.parent = parent;
 	bridge->dev.release = pci_release_host_bridge_dev;
 	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
@@ -1801,6 +1803,13 @@ err_out:
 	return NULL;
 }
 
+struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	return __pci_create_root_bus(parent, pci_domain_nr(bus), bus,
+		ops, sysdata, resources);
+}
+
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
 {
 	struct resource *res = &b->busn_res;
@@ -1899,6 +1908,42 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 }
 EXPORT_SYMBOL(pci_scan_root_bus);
 
+struct pci_bus *pci_scan_domain(struct device *parent, int domain,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	struct pci_host_bridge_window *window;
+	bool found = false;
+	struct pci_bus *b;
+	int max;
+
+	list_for_each_entry(window, resources, list)
+		if (window->res->flags & IORESOURCE_BUS) {
+			found = true;
+			break;
+		}
+
+	if (!found) {
+		dev_info(&b->dev,
+			 "No busn resource found for domain %d\n", domain);
+		return NULL;
+	}
+
+	b = __pci_create_root_bus(parent, domain, window->res->start,
+				  ops, sysdata, resources);
+	if (!b)
+		return NULL;
+
+		dev_info(&b->dev,
+		 "No busn resource found for root bus, will use [bus %02x-ff]\n",
+			bus);
+		pci_bus_insert_busn_res(b, bus, 255);
+	}
+
+	max = pci_scan_child_bus(b);
+
+	pci_bus_add_devices(b);
+	return b;
+}
 /* Deprecated; use pci_scan_root_bus() instead */
 struct pci_bus *pci_scan_bus_parented(struct device *parent,
 		int bus, struct pci_ops *ops, void *sysdata)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1e26fc6..734c016 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -395,6 +395,7 @@ struct pci_host_bridge_window {
 
 struct pci_host_bridge {
 	struct device dev;
+	int domain;
 	struct pci_bus *bus;		/* root bus */
 	struct list_head windows;	/* pci_host_bridge_windows */
 	void (*release_fn)(struct pci_host_bridge *);

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

[Liviu Dudau]

On Wed, Feb 19, 2014 at 11:24:30AM +0100, Arnd Bergmann wrote:
> On Wednesday 19 February 2014 02:44:27 Liviu Dudau wrote:
> > On Tue, Feb 18, 2014 at 10:41:25AM -0700, Jason Gunthorpe wrote:
> > 
> > > So the Liviu, I would say the API should be similar to what we see in
> > > other OF enabled driver bases subsystems, call the core code with a
> > > platform_device pointer and function_ops pointer and have the core
> > > code create a domain, figure out the domain # from the DT (via
> > > aliases?), and so on.
> > 
> > I wish things were easier!
> > 
> > Lets look at the 'int pci_domain_nr(struct pci_bus *bus);' function. It is
> > used to obtain the domain number of the bus passed as an argument.
> > 
> > - include/linux/pci.h defines it as an inline function returning zero if
> >   !CONFIG_PCI || !CONFIG_PCI_DOMAINS. Otherwise it is silent on what the
> >   function might look like.
> 
> I think we should come up with a default implementation that works for
> any architecture using DT based probing, and requires PCI_DOMAINS to
> be enabled.

Agree. What I was trying to say was that the function is not *defined* if
you have CONFIG_PCI && !CONFIG_PCI_DOMAINS, but it has a dummy implementation
for !CONFIG_PCI or CONFIG_PCI && CONFIG_PCI_DOMAINS.

> 
> > - alpha, ia64, microblaze, mips, powerpc and tile all define it as a cast
> >   of bus->sysdata to "struct pci_controller *" and then access a data
> >   member from there to get the domain number. But ... the pci_controller
> >   structure is different for each architecture, with few more members in
> >   addition that might be actually shared with generic framework code.
> 
> We do have the generic 'struct pci_host_bridge'. It would be trivial to
> add a 'domain' field in there and use it in the generic implementation.

That's what my v1 patch was trying to do, but as Tanmay reported, it is not
working, see below for the explanation why (short summary, we ask for domain
number of a bus before the associated pci_host_bridge is alive).

> 
> > - arm, s390, sparc and x86 have all different names for their sysdata,
> >   but they all contain inside a member that is used for giving a domain
> >   back. sparc gets an honorary mention here for getting the API wrong
> >   and returning -ENXIO in certain conditions (not like the generic code
> >   cares).
> 
> I would hope that for the generic case, we can actually remove the
> use of 'sysdata' and replace it with common code that just looks
> at pci_host_bridge. Some architectures (s390 in particular) will probably
> need their own data to be added in sysdata, but overall the architectures
> are really trying to do the same thing here. With the work that Bjorn
> and others have done over the past few years, you already need much less
> architecture specific code. I think it's time to get it to a point where
> most architectures can do without any at all.

We are on the same page. I was just flagging the amount of work we need to
do here to bring everyone towards common code.

> 
> > That takes care of the implementation. But what about usage?
> > 
> > - drivers/pci/probe.c: pci_create_root_bus allocates a new bus structure,
> >   sets up the sysdata and ops member pointers and then goes straight
> >   into trying to find if the newly created bus doesn't already exist by
> >   using the bus number given as parameter and pci_domain_nr() with the
> >   new bus structure as argument. I'm trying really hard to figure out
> >   what was the intention here, but from the point of view of someone
> >   trying to implement the pci_domain_nr() function I have no idea what
> >   to return for a virgin bus structure that is not yet attached to any
> >   parent.
> 
> Right, this needs to be changed when moving the domain into pci_host_bridge.

Yes, if only I understood what the intent was here. Unfortunately, the history
of the file stops (for my git tree) at Linus' first commit from v2.6.0, so
I need to dig deeper. What I am looking to understand is the answer to this
question: "When you want to create a new bus and you want to make sure it
is not duplicated, do you go across all domains or pick the current domain
and allow for same bus number in different domains?"

To give some context: I'm looking at drivers/pci/probe.c, pci_create_root_bus().
Specifically, this code:

	b = pci_alloc_bus();
	if (!b)
		return NULL;

	b->sysdata = sysdata;
	b->ops = ops;
	b->number = b->busn_res.start = bus;
	b2 = pci_find_bus(pci_domain_nr(b), bus);
	if (b2) {
		/* If we already got to this bus through a different bridge, ignore it */
		dev_dbg(&b2->dev, "bus already known\n");
		goto err_out;
	}

	bridge = pci_alloc_host_bridge(b);
	if (!bridge)
		goto err_out;


If you look at the comment after pci_find_bus, you can see that the intent was
to try to find duplicate busses covered by different bridges (but in the same
domain? maybe, because there used to be only one domain at the beginning, but
then why use pci_domain_nr() here?). Problems I see here:

  - code is trying to get the domain number of a bus that it just created but not
    plugged yet in the hierarchy
  - pci_find_bus tries to find a bus in the global list of busses that has the
    same domain number and bus number as the parameters of the function. But
    the domain number was plucked out of thin air and all architectures seem to
    actually give you the current active domain number, not the one that actually
    covers the bus (and host_bridge) that you are trying to create.


I will try to restructure the code here, but I think the check is superfluous and
can be done better by a function that tries to find duplicates when you add the
bus, rather than when creating the scaffolding.

> 
> > So I can see the intent of what Jason is proposing and I'm heading that
> > way myself, but I think I need to cleanup pci_create_root_bus first
> > (change the creation order between bridge and bus). And if someone has
> > a good idea on how to determine the domain # from DT we can pluck it
> > into the pcibios_root_bridge_prepare() function (either the generic
> > version or the arch specific one).
> 
> How about the change below, to introduce a new pci_scan_domain() function
> as a variant of pci_scan_root_bus()?
> 
> 	Arnd
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 5094943..9f2ec2f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1697,8 +1697,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
>  {
>  }
>  
> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +static struct pci_bus *__pci_create_root_bus(struct device *parent,
> +		int domain, int bus, struct pci_ops *ops, void *sysdata,
> +		struct list_head *resources)
>  {
>  	int error;
>  	struct pci_host_bridge *bridge;
> @@ -1716,7 +1717,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	b->sysdata = sysdata;
>  	b->ops = ops;
>  	b->number = b->busn_res.start = bus;
> -	b2 = pci_find_bus(pci_domain_nr(b), bus);
> +	b2 = pci_find_bus(domain, bus);
>  	if (b2) {
>  		/* If we already got to this bus through a different bridge, ignore it */
>  		dev_dbg(&b2->dev, "bus already known\n");
> @@ -1727,6 +1728,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	if (!bridge)
>  		goto err_out;
>  
> +	bridge->domain = domain;
>  	bridge->dev.parent = parent;
>  	bridge->dev.release = pci_release_host_bridge_dev;
>  	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> @@ -1801,6 +1803,13 @@ err_out:
>  	return NULL;
>  }
>  
> +struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> +		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +{
> +	return __pci_create_root_bus(parent, pci_domain_nr(bus), bus,
> +		ops, sysdata, resources);
> +}
> +
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
>  {
>  	struct resource *res = &b->busn_res;
> @@ -1899,6 +1908,42 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>  }
>  EXPORT_SYMBOL(pci_scan_root_bus);
>  
> +struct pci_bus *pci_scan_domain(struct device *parent, int domain,
> +		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +{
> +	struct pci_host_bridge_window *window;
> +	bool found = false;
> +	struct pci_bus *b;
> +	int max;
> +
> +	list_for_each_entry(window, resources, list)
> +		if (window->res->flags & IORESOURCE_BUS) {
> +			found = true;
> +			break;
> +		}
> +
> +	if (!found) {
> +		dev_info(&b->dev,
> +			 "No busn resource found for domain %d\n", domain);
> +		return NULL;
> +	}
> +
> +	b = __pci_create_root_bus(parent, domain, window->res->start,
> +				  ops, sysdata, resources);
> +	if (!b)
> +		return NULL;
> +
> +		dev_info(&b->dev,
> +		 "No busn resource found for root bus, will use [bus %02x-ff]\n",
> +			bus);
> +		pci_bus_insert_busn_res(b, bus, 255);
> +	}
> +
> +	max = pci_scan_child_bus(b);
> +
> +	pci_bus_add_devices(b);
> +	return b;
> +}
>  /* Deprecated; use pci_scan_root_bus() instead */
>  struct pci_bus *pci_scan_bus_parented(struct device *parent,
>  		int bus, struct pci_ops *ops, void *sysdata)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1e26fc6..734c016 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -395,6 +395,7 @@ struct pci_host_bridge_window {
>  
>  struct pci_host_bridge {
>  	struct device dev;
> +	int domain;
>  	struct pci_bus *bus;		/* root bus */
>  	struct list_head windows;	/* pci_host_bridge_windows */
>  	void (*release_fn)(struct pci_host_bridge *);
> 

Yes, I think this will work, as long as we can figure out if the intent of the
code is corect.

Thanks,
Liviu

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

[Arnd Bergmann]

On Wednesday 19 February 2014 11:37:55 Liviu Dudau wrote:
> 
> > Right, this needs to be changed when moving the domain into pci_host_bridge.
> 
> Yes, if only I understood what the intent was here. Unfortunately, the history
> of the file stops (for my git tree) at Linus' first commit from v2.6.0, so
> I need to dig deeper. What I am looking to understand is the answer to this
> question: "When you want to create a new bus and you want to make sure it
> is not duplicated, do you go across all domains or pick the current domain
> and allow for same bus number in different domains?"

I suspect this is for some large IA64 machines where you can see the same
domain at different addresses, depending on which CPU you are on, or
something similarly fancy.

Another explanation would be that you can have different ways to describe
the same hardware: On a PC you can assume that the PCI bus is discoverable
on I/O port 0x0cf8. At the same time, you may see it in ACPI with an
mmconfig method.

In any case, you definitely need to allow the same bus numbers on each domain,
since the most important use case of domains is to get around the 256 bus
limit.

> To give some context: I'm looking at drivers/pci/probe.c, pci_create_root_bus().
> Specifically, this code:
> 
>         b = pci_alloc_bus();
>         if (!b)
>                 return NULL;
> 
>         b->sysdata = sysdata;
>         b->ops = ops;
>         b->number = b->busn_res.start = bus;
>         b2 = pci_find_bus(pci_domain_nr(b), bus);
>         if (b2) {
>                 /* If we already got to this bus through a different bridge, ignore it */
>                 dev_dbg(&b2->dev, "bus already known\n");
>                 goto err_out;
>         }
> 
>         bridge = pci_alloc_host_bridge(b);
>         if (!bridge)
>                 goto err_out;
> 
> 
> If you look at the comment after pci_find_bus, you can see that the intent was
> to try to find duplicate busses covered by different bridges (but in the same
> domain? maybe, because there used to be only one domain at the beginning, but
> then why use pci_domain_nr() here?). Problems I see here:

Definitely duplicate buses in the same domain, as mentioned above.

>   - code is trying to get the domain number of a bus that it just created but not
>     plugged yet in the hierarchy

The sysdata is already assigned, and all architectures either assume there is
only one domain, or they get the domain number out of the sysdata that is
filled out prior to calling pci_create_root_bus.

>   - pci_find_bus tries to find a bus in the global list of busses that has the
>     same domain number and bus number as the parameters of the function. But
>     the domain number was plucked out of thin air and all architectures seem to
>     actually give you the current active domain number, not the one that actually
>     covers the bus (and host_bridge) that you are trying to create.

Not sure what you mean with 'current active domain number': They clearly look at
b->sysdata here, which is a local structure describing what we are currently
probing.

	Arnd

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

[Liviu Dudau]

On Wed, Feb 19, 2014 at 02:26:10PM +0100, Arnd Bergmann wrote:
> On Wednesday 19 February 2014 11:37:55 Liviu Dudau wrote:
> > 
> > > Right, this needs to be changed when moving the domain into pci_host_bridge.
> > 
> > Yes, if only I understood what the intent was here. Unfortunately, the history
> > of the file stops (for my git tree) at Linus' first commit from v2.6.0, so
> > I need to dig deeper. What I am looking to understand is the answer to this
> > question: "When you want to create a new bus and you want to make sure it
> > is not duplicated, do you go across all domains or pick the current domain
> > and allow for same bus number in different domains?"
> 
> I suspect this is for some large IA64 machines where you can see the same
> domain at different addresses, depending on which CPU you are on, or
> something similarly fancy.
> 
> Another explanation would be that you can have different ways to describe
> the same hardware: On a PC you can assume that the PCI bus is discoverable
> on I/O port 0x0cf8. At the same time, you may see it in ACPI with an
> mmconfig method.
> 
> In any case, you definitely need to allow the same bus numbers on each domain,
> since the most important use case of domains is to get around the 256 bus
> limit.
> 
> > To give some context: I'm looking at drivers/pci/probe.c, pci_create_root_bus().
> > Specifically, this code:
> > 
> >         b = pci_alloc_bus();
> >         if (!b)
> >                 return NULL;
> > 
> >         b->sysdata = sysdata;
> >         b->ops = ops;
> >         b->number = b->busn_res.start = bus;
> >         b2 = pci_find_bus(pci_domain_nr(b), bus);
> >         if (b2) {
> >                 /* If we already got to this bus through a different bridge, ignore it */
> >                 dev_dbg(&b2->dev, "bus already known\n");
> >                 goto err_out;
> >         }
> > 
> >         bridge = pci_alloc_host_bridge(b);
> >         if (!bridge)
> >                 goto err_out;
> > 
> > 
> > If you look at the comment after pci_find_bus, you can see that the intent was
> > to try to find duplicate busses covered by different bridges (but in the same
> > domain? maybe, because there used to be only one domain at the beginning, but
> > then why use pci_domain_nr() here?). Problems I see here:
> 
> Definitely duplicate buses in the same domain, as mentioned above.
> 
> >   - code is trying to get the domain number of a bus that it just created but not
> >     plugged yet in the hierarchy
> 
> The sysdata is already assigned, and all architectures either assume there is
> only one domain, or they get the domain number out of the sysdata that is
> filled out prior to calling pci_create_root_bus.

Arnd, the question is not how it does it, the question is what is the correct answer?
It's a new bus, does that mean that all new busses get created in the current domain?
If that is the case, then fine, things start to make more sense, with the caveat
that the domain # depends heavily on what the architecture uses for counting.

> 
> >   - pci_find_bus tries to find a bus in the global list of busses that has the
> >     same domain number and bus number as the parameters of the function. But
> >     the domain number was plucked out of thin air and all architectures seem to
> >     actually give you the current active domain number, not the one that actually
> >     covers the bus (and host_bridge) that you are trying to create.
> 
> Not sure what you mean with 'current active domain number': They clearly look at
> b->sysdata here, which is a local structure describing what we are currently
> probing.

Yes, but that definition of locality is not common to all architectures. arm for
example stores it in the domain member of pci_sys_data, but that gets initialised
every time someone calls pcibios_init_hw (or pci_common_init_dev) with the domain
value held in struct hw_pci. But the host controllers don't talk to each other,
so if you want to instantiate two different drivers that use hw_pci they likely
use an individual count that starts at zero.

Anyway, I don't think that using your suggestion is going to be a problem, so once
I get out of this god forsaken task of bringing up a board I will post an updated
patch.

Best regards,
Liviu

> 
> 	Arnd
> 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

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

[Arnd Bergmann]

On Wednesday 19 February 2014 15:30:44 Liviu Dudau wrote:
> On Wed, Feb 19, 2014 at 02:26:10PM +0100, Arnd Bergmann wrote:
> > On Wednesday 19 February 2014 11:37:55 Liviu Dudau wrote:

> > >   - code is trying to get the domain number of a bus that it just created but not
> > >     plugged yet in the hierarchy
> > 
> > The sysdata is already assigned, and all architectures either assume there is
> > only one domain, or they get the domain number out of the sysdata that is
> > filled out prior to calling pci_create_root_bus.
> 
> Arnd, the question is not how it does it, the question is what is the correct answer?
> It's a new bus, does that mean that all new busses get created in the current domain?
> If that is the case, then fine, things start to make more sense, with the caveat
> that the domain # depends heavily on what the architecture uses for counting.

Here is what each architecture does:

Alpha: managed by host bridge driver, counting the hosts or fixed in hardware
ARM: managed in host bridge driver, counting the hosts or using just one domain
IA64: taken from firmware (either ACPI or SAL)
microblaze: managed globally, counting the hosts
powerpc: managed globally, counting the hosts
s390: each PCI function has its own domain (!)
sh: managed in host bridge driver, counting the hosts
sparc: managed by host bridge driger, counting the hosts
tile: managed globally, counting the hosts
x86: managed by ACPI firmware

In each case, we either know the domain from hardware or firmware
when creating the host bridge, or we pick the next free number.

> > >   - pci_find_bus tries to find a bus in the global list of busses that has the
> > >     same domain number and bus number as the parameters of the function. But
> > >     the domain number was plucked out of thin air and all architectures seem to
> > >     actually give you the current active domain number, not the one that actually
> > >     covers the bus (and host_bridge) that you are trying to create.
> > 
> > Not sure what you mean with 'current active domain number': They clearly look at
> > b->sysdata here, which is a local structure describing what we are currently
> > probing.
> 
> Yes, but that definition of locality is not common to all architectures. arm for
> example stores it in the domain member of pci_sys_data, but that gets initialised
> every time someone calls pcibios_init_hw (or pci_common_init_dev) with the domain
> value held in struct hw_pci. But the host controllers don't talk to each other,
> so if you want to instantiate two different drivers that use hw_pci they likely
> use an individual count that starts at zero.

The assumption is that whoever calls pci_common_init{_dev} knows about all the
PCI hosts in the system. As Jason Gunthorpe said, it would be desirable to
have stable domain numbers, so it's probably best to take the number from
DT (e.g. the aliases node). It's probably safe to assume that any machine
using ATAGS for booting falls into the category where it's safe to keep the
policy local in the host bridge driver.

	Arnd

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

[Bjorn Helgaas]

On Sat, Feb 15, 2014 at 02:03:26PM +0100, Arnd Bergmann wrote:
> On Friday 14 February 2014 22:00:47 Liviu Dudau wrote:
> > 
> > What I'm going to suggest in my v2 patch (hope to send it before Monday)
> > is a new API in the generic PCI code that will allow you to create a
> > host bridge in a new domain or in the existing domain, with the management
> > of the domain number being done in the generic code.
> > 
> > Something like:
> > 
> >   int create_hostbridge_in_new_domain(....);
> >   int create_hostbridge(....);
> > 
> > with the functions being wrappers around the pci_hostbridge_of_init function
> > that I'm introducing.
> > 
> > What do you think?
> 
> Not sure. It would still keep the decision about whether to use a
> new domain or not in the host bridge driver, but that doesn't seem
> like the right place. The same driver might be used in different
> ways based on what is around it.
> 
> I've also had a look at the MIPS implementation now, which has its
> own way of dealing with this, see arch/mips/pci/pci.c.
> 
> There was also an interesting comment in the MIPS header file:
> 
>         /* For compatibility with current (as of July 2003) pciutils
>            and XFree86. Eventually will be removed. */
>         unsigned int need_domain_info;
> 
> This is over ten years old, so I wonder if we can start assuming that
> domains work out of the box now. All references to problems from 
> PCI domains are about old code (ca. pre-2007) that doesn't understand
> nonzero domains and that has since been fixed. I am pretty sure we
> don't need to ever worry about stuffing multiple host bridges into
> a domain other than the first one, and I also doubt we have to worry
> about the problem at all on arm64 as we won't run old binaries on it
> (or arm32 compat mode binaries that need to manage PCI devices).
> 
> Can anyone with more experience on the subject than me (Bjorn,
> Russell, Jason, ...) think of a reason why we would not want to
> just use a new domain for every host bridge we find?

With ACPI on x86 and ia64, we currently use _SEG directly as the Linux
PCI domain.  Are you proposing that we make the Linux PCI domain
independent of _SEG?

It will look sort of funny to have things like this:

  ACPI: PCI Root Bridge [L000] (_SEG 0 domain 0000 [bus 00-1b])
  ACPI: PCI Root Bridge [L001] (_SEG 0 domain 0001 [bus 1c-37])
  ACPI: PCI Root Bridge [L002] (_SEG 0 domain 0002 [bus 38-53])

where the firmware had _SEG==0 for all the bridges and assigned
non-overlapping bus number ranges, but since nothing in PCI really
depends on the domain, I guess it should work.

Bjorn

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

[Arnd Bergmann]

On Tuesday 18 February 2014 17:28:14 Bjorn Helgaas wrote:
> On Sat, Feb 15, 2014 at 02:03:26PM +0100, Arnd Bergmann wrote:
> > On Friday 14 February 2014 22:00:47 Liviu Dudau wrote:
> > 
> > Can anyone with more experience on the subject than me (Bjorn,
> > Russell, Jason, ...) think of a reason why we would not want to
> > just use a new domain for every host bridge we find?
> 
> With ACPI on x86 and ia64, we currently use _SEG directly as the Linux
> PCI domain.  Are you proposing that we make the Linux PCI domain
> independent of _SEG?

I don't think we should change anything for ACPI.
The point is that we don't currently have the equivalent of _SEG
for DT probing. The best two options we have are:

a) introduce a 'pci-segment' property with the same meaning as _SEG
on ACPI, and use that as the domain number

b) don't introduce the segment concept on DT but just give each
host bridge its own domain.

The second one seems a little easier to implement, and I don't
see what _SEG is used for other than to avoid having domains
when you don't need them. Is there more to it that I'm missing?

> It will look sort of funny to have things like this:
> 
>   ACPI: PCI Root Bridge [L000] (_SEG 0 domain 0000 [bus 00-1b])
>   ACPI: PCI Root Bridge [L001] (_SEG 0 domain 0001 [bus 1c-37])
>   ACPI: PCI Root Bridge [L002] (_SEG 0 domain 0002 [bus 38-53])
> 
> where the firmware had _SEG==0 for all the bridges and assigned
> non-overlapping bus number ranges, but since nothing in PCI really
> depends on the domain, I guess it should work.

I would expect that with DT, this would look like

Root bridge 0: domain 0 [bus 0-1b]
Root bridge 1: domain 1 [bus 0-1b]
Root bridge 2: domain 2 [bus 0-1b]

Since you wouldn't have a reason to use a bus range starting at
a nonzero number.

	Arnd

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

[Bjorn Helgaas]

On Wed, Feb 19, 2014 at 2:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 18 February 2014 17:28:14 Bjorn Helgaas wrote:
>> On Sat, Feb 15, 2014 at 02:03:26PM +0100, Arnd Bergmann wrote:
>> > On Friday 14 February 2014 22:00:47 Liviu Dudau wrote:
>> >
>> > Can anyone with more experience on the subject than me (Bjorn,
>> > Russell, Jason, ...) think of a reason why we would not want to
>> > just use a new domain for every host bridge we find?
>>
>> With ACPI on x86 and ia64, we currently use _SEG directly as the Linux
>> PCI domain.  Are you proposing that we make the Linux PCI domain
>> independent of _SEG?
>
> I don't think we should change anything for ACPI.
> The point is that we don't currently have the equivalent of _SEG
> for DT probing. The best two options we have are:
>
> a) introduce a 'pci-segment' property with the same meaning as _SEG
> on ACPI, and use that as the domain number
>
> b) don't introduce the segment concept on DT but just give each
> host bridge its own domain.
>
> The second one seems a little easier to implement, and I don't
> see what _SEG is used for other than to avoid having domains
> when you don't need them. Is there more to it that I'm missing?

Not really, but I do have a question related to OS management of host
bridge bus number apertures.  Currently, Linux never changes a host
bridge's bus number range, but it's conceivable that we could in some
hotplug scenario.  ACPI does provide a mechanism to do so (_PRS,
_SRS), and other host bridge drivers could also do this by programming
CSRs to change the bus number range.  The PCI domain is the logical
place to manage allocation of the 00-ff range of bus numbers.

1) x86 platforms may have constraints because PCIBIOS and the 0xcf8
config access mechanism are unaware of segments.  If a platform has to
support legacy OSes that use those, it can't reuse bus numbers even in
different segment groups.  The platform might have to use multiple
segments to allow multiple ECAM regions, but use _PRS/_SRS to prevent
bus number overlaps to keep legacy config access working.  Obviously
this is only an issue if there are non-segment aware config access
methods.

2) If two host bridges share an ECAM region, I think we're forced to
put them in the same domain: if we put them in different domains,
Linux might assign [bus 00-ff] to both bridges, and ECAM config
accesses would only work for one of the bridges.  This is quite common
on x86 and is a potential issue for any architecture.

Bjorn

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

[Arnd Bergmann]

On Wednesday 19 February 2014 11:20:19 Bjorn Helgaas wrote:
> On Wed, Feb 19, 2014 at 2:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 18 February 2014 17:28:14 Bjorn Helgaas wrote:

> > The second one seems a little easier to implement, and I don't
> > see what _SEG is used for other than to avoid having domains
> > when you don't need them. Is there more to it that I'm missing?
> 
> Not really, but I do have a question related to OS management of host
> bridge bus number apertures.  Currently, Linux never changes a host
> bridge's bus number range, but it's conceivable that we could in some
> hotplug scenario.  ACPI does provide a mechanism to do so (_PRS,
> _SRS), and other host bridge drivers could also do this by programming
> CSRs to change the bus number range.  The PCI domain is the logical
> place to manage allocation of the 00-ff range of bus numbers.
> 
> 1) x86 platforms may have constraints because PCIBIOS and the 0xcf8
> config access mechanism are unaware of segments.  If a platform has to
> support legacy OSes that use those, it can't reuse bus numbers even in
> different segment groups.  The platform might have to use multiple
> segments to allow multiple ECAM regions, but use _PRS/_SRS to prevent
> bus number overlaps to keep legacy config access working.  Obviously
> this is only an issue if there are non-segment aware config access
> methods.

Right, I don't think this will be an issue outside of x86/ia64/alpha,
since on all other architectures I'm aware of you have no PCIBIOS
and each host controller would also have its own config space.
Even host controllers using 0xfc8 would be fine because each host
bridge normally has its own I/O space.

> 2) If two host bridges share an ECAM region, I think we're forced to
> put them in the same domain: if we put them in different domains,
> Linux might assign [bus 00-ff] to both bridges, and ECAM config
> accesses would only work for one of the bridges.  This is quite common
> on x86 and is a potential issue for any architecture.

Right, this is an interesting case indeed, and I think we haven't
considered it in the binding so far. We already encode a "bus-range"
in DT, so we can easily partition the ECAM config space, but it
still violates one of the two assumptions:
a) that the register ranges for the two host bridge devices are
   non-overlapping in DT
b) that the ECAM register range as specified in DT starts at bus
   0 and is a power-of-two size.
Since the binding is not fixed that, we could change the definition to
say that the ECAM register range in the "reg" property must match
the buses listed in the "bus-range" property.

I still want to make sure I understand exactly what this case is about
though, i.e. what is shared and what is separate if you have two host
bridges with a common ECAM region:

* I assume I/O space is always shared on x86, but probably separate
  elsewhere.
* Each host would always have a fixed memory space aperture, right?
* From what I understand from your description, the hardware does
  not enforce specific bus numbers for each host. How does the
  host bridge know its root bus number then?
* Should I expect one IOMMU per host bridge or one ECAM region,
  or can either be possible?
* The IntA-IntB IRQ numbers are always per host bridge I assume.
* Memory space on one host bridge is visible to bus master DMA
  from a device on another host bridge on x86, right? I assume
  this won't normally be the case on other architectures.

	Arnd

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

[Bjorn Helgaas]

On Wed, Feb 19, 2014 at 12:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 19 February 2014 11:20:19 Bjorn Helgaas wrote:
>> On Wed, Feb 19, 2014 at 2:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday 18 February 2014 17:28:14 Bjorn Helgaas wrote:
>
>> > The second one seems a little easier to implement, and I don't
>> > see what _SEG is used for other than to avoid having domains
>> > when you don't need them. Is there more to it that I'm missing?
>>
>> Not really, but I do have a question related to OS management of host
>> bridge bus number apertures.  Currently, Linux never changes a host
>> bridge's bus number range, but it's conceivable that we could in some
>> hotplug scenario.  ACPI does provide a mechanism to do so (_PRS,
>> _SRS), and other host bridge drivers could also do this by programming
>> CSRs to change the bus number range.  The PCI domain is the logical
>> place to manage allocation of the 00-ff range of bus numbers.
>>
>> 1) x86 platforms may have constraints because PCIBIOS and the 0xcf8
>> config access mechanism are unaware of segments.  If a platform has to
>> support legacy OSes that use those, it can't reuse bus numbers even in
>> different segment groups.  The platform might have to use multiple
>> segments to allow multiple ECAM regions, but use _PRS/_SRS to prevent
>> bus number overlaps to keep legacy config access working.  Obviously
>> this is only an issue if there are non-segment aware config access
>> methods.
>
> Right, I don't think this will be an issue outside of x86/ia64/alpha,
> since on all other architectures I'm aware of you have no PCIBIOS
> and each host controller would also have its own config space.
> Even host controllers using 0xfc8 would be fine because each host
> bridge normally has its own I/O space.
>
>> 2) If two host bridges share an ECAM region, I think we're forced to
>> put them in the same domain: if we put them in different domains,
>> Linux might assign [bus 00-ff] to both bridges, and ECAM config
>> accesses would only work for one of the bridges.  This is quite common
>> on x86 and is a potential issue for any architecture.
>
> Right, this is an interesting case indeed, and I think we haven't
> considered it in the binding so far. We already encode a "bus-range"
> in DT, so we can easily partition the ECAM config space, but it
> still violates one of the two assumptions:
> a) that the register ranges for the two host bridge devices are
>    non-overlapping in DT
> b) that the ECAM register range as specified in DT starts at bus
>    0 and is a power-of-two size.
> Since the binding is not fixed that, we could change the definition to
> say that the ECAM register range in the "reg" property must match
> the buses listed in the "bus-range" property.

Addresses in the ACPI MCFG table correspond to bus number 0, but the
MCFG also provides start & end bus numbers, so the valid range does
not necessarily start with bus 0 and need not be power-of-two in size.
 Something similar sounds like a good idea for DT.

> I still want to make sure I understand exactly what this case is about
> though, i.e. what is shared and what is separate if you have two host
> bridges with a common ECAM region:
>
> * I assume I/O space is always shared on x86, but probably separate
>   elsewhere.

I think x86 *could* support multiple I/O spaces, but user-mode
inb/outb could only reach the 0-64K space.  I think ia64 has the same
limitation on user code, but it supports many spaces in the kernel.

> * Each host would always have a fixed memory space aperture, right?

The ACPI _CRS/_PRS/_SRS mechanism theoretically allows changes to the
bus number, I/O space, and memory space apertures of host bridges.
But we don't do any of those changes today, and I don't know if any
BIOSes actually allow it.

> * From what I understand from your description, the hardware does
>   not enforce specific bus numbers for each host. How does the
>   host bridge know its root bus number then?

I don't know details of any specific hardware.  I'm just saying that
ACPI provides a mechanism for the OS to manipulate the bus number
range below a host bridge.  Of course, a BIOS is free to omit _PRS and
_SRS, and in that case, the bus/IO/memory apertures reported by _CRS
are fixed and can't be changed.  We learn the root bus number from the
host bridge _CRS (but I'm sure you knew that, so maybe I missed the
point of your question).

> * Should I expect one IOMMU per host bridge or one ECAM region,
>   or can either be possible?

It's possible to have multiple IOMMUs per host bridge, and I think
they can even be buried down in the PCIe hierarchy.

> * The IntA-IntB IRQ numbers are always per host bridge I assume.

For conventional PCI, INTx are just wires that could go anywhere, so
there's no connection between them and a host bridge.  You have to
have a _PRT or similar to make sense out of them.  For PCIe, a Root
Complex maps INTx emulation messages to system interrupts in an
implementation-specific way, so we need a _PRT there, too.  I don't
think there's a requirement that these IRQ numbers be per-host bridge.

> * Memory space on one host bridge is visible to bus master DMA
>   from a device on another host bridge on x86, right? I assume
>   this won't normally be the case on other architectures.

I think this is also implementation dependent, and I'm not aware of an
ACPI or other generic way to learn what a particular platform does.
It seems like this was an issue for MPS configuration, where
peer-to-peer DMA is a problem because we don't really know how Root
Complexes handle peer-to-peer transactions.

Bjorn

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

[Arnd Bergmann]

On Wednesday 19 February 2014 13:18:24 Bjorn Helgaas wrote:
> >
> > Right, this is an interesting case indeed, and I think we haven't
> > considered it in the binding so far. We already encode a "bus-range"
> > in DT, so we can easily partition the ECAM config space, but it
> > still violates one of the two assumptions:
> > a) that the register ranges for the two host bridge devices are
> >    non-overlapping in DT
> > b) that the ECAM register range as specified in DT starts at bus
> >    0 and is a power-of-two size.
> > Since the binding is not fixed that, we could change the definition to
> > say that the ECAM register range in the "reg" property must match
> > the buses listed in the "bus-range" property.
> 
> Addresses in the ACPI MCFG table correspond to bus number 0, but the
> MCFG also provides start & end bus numbers, so the valid range does
> not necessarily start with bus 0 and need not be power-of-two in size.
>  Something similar sounds like a good idea for DT.

Hmm, we'll have to think about that. From a DT perspective, we try
to keep things local to the node using it, so listing only the
registers we are allowed to access is more natural.

Another option would be to have a separate device node for the ECAM
registers and point to that from the host controller node, which
would describe this cleanly but also add a bit of complexity that
will rarely be used.

> > I still want to make sure I understand exactly what this case is about
> > though, i.e. what is shared and what is separate if you have two host
> > bridges with a common ECAM region:
> >
> > * I assume I/O space is always shared on x86, but probably separate
> >   elsewhere.
> 
> I think x86 *could* support multiple I/O spaces, but user-mode
> inb/outb could only reach the 0-64K space.  I think ia64 has the same
> limitation on user code, but it supports many spaces in the kernel.

Ok.

> > * Each host would always have a fixed memory space aperture, right?
> 
> The ACPI _CRS/_PRS/_SRS mechanism theoretically allows changes to the
> bus number, I/O space, and memory space apertures of host bridges.
> But we don't do any of those changes today, and I don't know if any
> BIOSes actually allow it.

I mean non-overlapping apertures in particular. We also have
cases where the aperture that we list in DT is just programmed
into hardware registers by the host driver and could be arbitrary,
but you can't normally have the same MMIO address go to two
devices on internal buses (or not in a sensible way).

> > * From what I understand from your description, the hardware does
> >   not enforce specific bus numbers for each host. How does the
> >   host bridge know its root bus number then?
> 
> I don't know details of any specific hardware.  I'm just saying that
> ACPI provides a mechanism for the OS to manipulate the bus number
> range below a host bridge.  Of course, a BIOS is free to omit _PRS and
> _SRS, and in that case, the bus/IO/memory apertures reported by _CRS
> are fixed and can't be changed.  We learn the root bus number from the
> host bridge _CRS (but I'm sure you knew that, so maybe I missed the
> point of your question).

I guess the answer then is that the host bridge can have a register
for programming the root bus number, but it's not standardized and
therefore the access is hidden in the _PRS/_SRS methods. If we have
the same on DT and want to reprogram the bus numbers, we'd need to
have a kernel driver for the nonstandard registers of the host bridge.

> > * Should I expect one IOMMU per host bridge or one ECAM region,
> >   or can either be possible?
> 
> It's possible to have multiple IOMMUs per host bridge, and I think
> they can even be buried down in the PCIe hierarchy.

Oh, I didn't know that. So how do you actually find the IOMMU for
a given domain/bus/device/function combination?

> > * The IntA-IntB IRQ numbers are always per host bridge I assume.
> 
> For conventional PCI, INTx are just wires that could go anywhere, so
> there's no connection between them and a host bridge.  You have to
> have a _PRT or similar to make sense out of them.  For PCIe, a Root
> Complex maps INTx emulation messages to system interrupts in an
> implementation-specific way, so we need a _PRT there, too.  I don't
> think there's a requirement that these IRQ numbers be per-host bridge.

Right, makes sense.

Thanks for the detailed explanations!

	Arnd

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

[Jason Gunthorpe]

On Wed, Feb 19, 2014 at 09:48:48PM +0100, Arnd Bergmann wrote:

> Hmm, we'll have to think about that. From a DT perspective, we try
> to keep things local to the node using it, so listing only the
> registers we are allowed to access is more natural.

If I understand the restriction properly, in a modern PCI-E world it
boils down to a limition on the configuration of each PCI-PCI root
port bridge (eg, a limited range of valid bus values, and apertures)

AFAIK it comes from the hidden per-socket routing registers that the
firwmare configures. Range X->Y (bus #, IO and MMIO) will be routed to
a specific physical socket, and then the PCI-E bridges in that socket
claim the transatcion based on their local config to select the
ultimate egress port.

So describing and restricting the bridge DT node itself, under a
single top level PCI domain stanza seems pretty reasonable.

As does containing the restrictions in a HW driver with knowledge of
the hidden registers, especially for firmware-less embedded. This is
part of what mvebu is doing already.

Jason

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

[Bjorn Helgaas]

On Wed, Feb 19, 2014 at 1:48 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 19 February 2014 13:18:24 Bjorn Helgaas wrote:
>> >
>> > Right, this is an interesting case indeed, and I think we haven't
>> > considered it in the binding so far. We already encode a "bus-range"
>> > in DT, so we can easily partition the ECAM config space, but it
>> > still violates one of the two assumptions:
>> > a) that the register ranges for the two host bridge devices are
>> >    non-overlapping in DT
>> > b) that the ECAM register range as specified in DT starts at bus
>> >    0 and is a power-of-two size.
>> > Since the binding is not fixed that, we could change the definition to
>> > say that the ECAM register range in the "reg" property must match
>> > the buses listed in the "bus-range" property.
>>
>> Addresses in the ACPI MCFG table correspond to bus number 0, but the
>> MCFG also provides start & end bus numbers, so the valid range does
>> not necessarily start with bus 0 and need not be power-of-two in size.
>>  Something similar sounds like a good idea for DT.
>
> Hmm, we'll have to think about that. From a DT perspective, we try
> to keep things local to the node using it, so listing only the
> registers we are allowed to access is more natural.

The combination of MCFG base address for bus 00 and the bus number
range from _CRS, plus the obvious offset computation does effectively
describe the registers you're allowed to access; it's just up to the
OS to compute the offsets.  _CBA (an optional method that returns the
ECAM address for a hot-added host bridge) uses the same bus 00 base.
My guess is that _CBA uses a bus number 00 base so it can return a
constant, regardless of whether the OS changes the bus number range.
If _CBA returned the ECAM base for the current bus number aperture, it
would be dependent on _CRS (the current settings), and the OS would
have to re-evaluate _CBA if it ever changed the bus number aperture.

>> > * Each host would always have a fixed memory space aperture, right?
>>
>> The ACPI _CRS/_PRS/_SRS mechanism theoretically allows changes to the
>> bus number, I/O space, and memory space apertures of host bridges.
>> But we don't do any of those changes today, and I don't know if any
>> BIOSes actually allow it.
>
> I mean non-overlapping apertures in particular. We also have
> cases where the aperture that we list in DT is just programmed
> into hardware registers by the host driver and could be arbitrary,
> but you can't normally have the same MMIO address go to two
> devices on internal buses (or not in a sensible way).

I don't remember specific spec statements about that, but I can't
imagine how to make sense of an address that's claimed by two devices.

>> > * From what I understand from your description, the hardware does
>> >   not enforce specific bus numbers for each host. How does the
>> >   host bridge know its root bus number then?
>>
>> I don't know details of any specific hardware.  I'm just saying that
>> ACPI provides a mechanism for the OS to manipulate the bus number
>> range below a host bridge.  Of course, a BIOS is free to omit _PRS and
>> _SRS, and in that case, the bus/IO/memory apertures reported by _CRS
>> are fixed and can't be changed.  We learn the root bus number from the
>> host bridge _CRS (but I'm sure you knew that, so maybe I missed the
>> point of your question).
>
> I guess the answer then is that the host bridge can have a register
> for programming the root bus number, but it's not standardized and
> therefore the access is hidden in the _PRS/_SRS methods. If we have
> the same on DT and want to reprogram the bus numbers, we'd need to
> have a kernel driver for the nonstandard registers of the host bridge.

Exactly; that's my mental model of how it works: _CRS/_PRS/_SRS are
basically accessors for generalized BARs.

>> > * Should I expect one IOMMU per host bridge or one ECAM region,
>> >   or can either be possible?
>>
>> It's possible to have multiple IOMMUs per host bridge, and I think
>> they can even be buried down in the PCIe hierarchy.
>
> Oh, I didn't know that. So how do you actually find the IOMMU for
> a given domain/bus/device/function combination?

For VT-d on x86, there's a DMAR table that describes the remapping
units (IOMMUs), and each has a list of associated devices.  This is
one place where the FW/OS interface uses segment and bus numbers.
There's something different for AMD IOMMUs, but I think it also
involves looking up the device in a table from the firmware.

 Bjorn

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

[Arnd Bergmann]

On Wednesday 19 February 2014 14:33:54 Bjorn Helgaas wrote:
> On Wed, Feb 19, 2014 at 1:48 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 19 February 2014 13:18:24 Bjorn Helgaas wrote:
> >> >
> >> > Right, this is an interesting case indeed, and I think we haven't
> >> > considered it in the binding so far. We already encode a "bus-range"
> >> > in DT, so we can easily partition the ECAM config space, but it
> >> > still violates one of the two assumptions:
> >> > a) that the register ranges for the two host bridge devices are
> >> >    non-overlapping in DT
> >> > b) that the ECAM register range as specified in DT starts at bus
> >> >    0 and is a power-of-two size.
> >> > Since the binding is not fixed that, we could change the definition to
> >> > say that the ECAM register range in the "reg" property must match
> >> > the buses listed in the "bus-range" property.
> >>
> >> Addresses in the ACPI MCFG table correspond to bus number 0, but the
> >> MCFG also provides start & end bus numbers, so the valid range does
> >> not necessarily start with bus 0 and need not be power-of-two in size.
> >>  Something similar sounds like a good idea for DT.
> >
> > Hmm, we'll have to think about that. From a DT perspective, we try
> > to keep things local to the node using it, so listing only the
> > registers we are allowed to access is more natural.
> 
> The combination of MCFG base address for bus 00 and the bus number
> range from _CRS, plus the obvious offset computation does effectively
> describe the registers you're allowed to access; it's just up to the
> OS to compute the offsets.

That's not what I meant. The 'reg' property is supposed to list the
registers that are exclusively owned by the device. We normally
expect that we want to request_mem_region() and ioremap() the entire
range for each device, which doesn't make sense if we only want a
small part of it, or if another device owns the same registers.

Having a separate device that just owns the ECAM region would work
fine, because then it can ioremap that once and get referenced
by the host bridge drivers.

> >> > * Should I expect one IOMMU per host bridge or one ECAM region,
> >> >   or can either be possible?
> >>
> >> It's possible to have multiple IOMMUs per host bridge, and I think
> >> they can even be buried down in the PCIe hierarchy.
> >
> > Oh, I didn't know that. So how do you actually find the IOMMU for
> > a given domain/bus/device/function combination?
> 
> For VT-d on x86, there's a DMAR table that describes the remapping
> units (IOMMUs), and each has a list of associated devices.  This is
> one place where the FW/OS interface uses segment and bus numbers.
> There's something different for AMD IOMMUs, but I think it also
> involves looking up the device in a table from the firmware.

Ok, that seems complicated. I hope we don't have to get this deep
on ARM and can keep it on a per host-bridge basis rather than having
to get down all the way to the device. We wouldn't be able to encode
IOMMUs for hotplugged devices in DT, because the firmware is no
longer running by the time they show up.

	Arnd

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

[Bjorn Helgaas]

On Wed, Feb 19, 2014 at 3:12 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 19 February 2014 14:33:54 Bjorn Helgaas wrote:
>> On Wed, Feb 19, 2014 at 1:48 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Wednesday 19 February 2014 13:18:24 Bjorn Helgaas wrote:
>> >> >
>> >> > Right, this is an interesting case indeed, and I think we haven't
>> >> > considered it in the binding so far. We already encode a "bus-range"
>> >> > in DT, so we can easily partition the ECAM config space, but it
>> >> > still violates one of the two assumptions:
>> >> > a) that the register ranges for the two host bridge devices are
>> >> >    non-overlapping in DT
>> >> > b) that the ECAM register range as specified in DT starts at bus
>> >> >    0 and is a power-of-two size.
>> >> > Since the binding is not fixed that, we could change the definition to
>> >> > say that the ECAM register range in the "reg" property must match
>> >> > the buses listed in the "bus-range" property.
>> >>
>> >> Addresses in the ACPI MCFG table correspond to bus number 0, but the
>> >> MCFG also provides start & end bus numbers, so the valid range does
>> >> not necessarily start with bus 0 and need not be power-of-two in size.
>> >>  Something similar sounds like a good idea for DT.
>> >
>> > Hmm, we'll have to think about that. From a DT perspective, we try
>> > to keep things local to the node using it, so listing only the
>> > registers we are allowed to access is more natural.
>>
>> The combination of MCFG base address for bus 00 and the bus number
>> range from _CRS, plus the obvious offset computation does effectively
>> describe the registers you're allowed to access; it's just up to the
>> OS to compute the offsets.
>
> That's not what I meant. The 'reg' property is supposed to list the
> registers that are exclusively owned by the device. We normally
> expect that we want to request_mem_region() and ioremap() the entire
> range for each device, which doesn't make sense if we only want a
> small part of it, or if another device owns the same registers.

Oh, right, that makes good sense.  The ACPI approach means we have to
have special-case code for that; it can't be handled by
request_mem_region() in a generic _CRS parser.

Bjorn

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

[Rob Herring]

On Thu, Feb 13, 2014 at 10:22 AM, Kumar Gala <galak@codeaurora.org> wrote:
>
> On Feb 13, 2014, at 5:07 AM, Will Deacon <will.deacon@arm.com> wrote:
>
>> On Wed, Feb 12, 2014 at 09:51:48PM +0000, Kumar Gala wrote:
>>>
>>> On Feb 12, 2014, at 2:16 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>>>> ---
>>>> .../devicetree/bindings/pci/arm-generic-pci.txt    |  51 ++++
>>>> drivers/pci/host/Kconfig                           |   7 +
>>>> drivers/pci/host/Makefile                          |   1 +
>>>> drivers/pci/host/pci-arm-generic.c                 | 318 +++++++++++++++++++++
>>>> 4 files changed, 377 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/pci/arm-generic-pci.txt
>>>> create mode 100644 drivers/pci/host/pci-arm-generic.c
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/arm-generic-pci.txt b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
>>>> new file mode 100644
>>>> index 000000000000..cc7a35ecfa2d
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pci/arm-generic-pci.txt
>>>> @@ -0,0 +1,51 @@
>>>> +* ARM 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:
>>>> +
>>>> +- compatible     : Must be "arm,pci-cam-generic" or "arm,pci-ecam-generic"
>>>> +                   depending on the layout of configuration space (CAM vs
>>>> +                   ECAM respectively)
>>>
>>> What's arm specific here?  I don't have a great suggestion, but seems odd
>>> for this to be vendor prefixed with "arm".
>>
>> Happy to change it, but I'm also struggling for names. Maybe "linux,..."?
>
> I was thinking that as well, I'd say go with "linux,".

Just drop the prefix altogether.

I'm wondering if this should have host or rc in the name.

Rob

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

[Jason Gunthorpe]

On Thu, Feb 13, 2014 at 11:07:21AM +0000, Will Deacon wrote:

> Not in this case! kvmtool generates the following:

Well, the example is nice so someone can review it..
 
> 	pci {
> 		#address-cells = <0x3>;
> 		#size-cells = <0x2>;
> 		#interrupt-cells = <0x1>;
> 		compatible = "arm,pci-cam-generic";
> 		reg = <0x0 0x40000000>;
> 		ranges = <0x1000000 0x0 0x0 0x0 0x0 0x0 0x10000 0x2000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000>;
> 		interrupt-map = <0x0 0x0 0x0 0x1 0x1 0x0 0x4 0x1 0x800 0x0 0x0 0x1 0x1 0x0 0x5 0x1 0x1000 0x0 0x0 0x1 0x1 0x0 0x6 0x1 0x1800 0x0 0x0 0x1 0x1 0x0 0x7 0x1>;
> 		interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
> 	};


Looks like there are a few misses in the above. How about this:

pci {
    compatible = "arm,pci-cam-generic"
    device_type = "pci";
    ** ^^^^^^^^^^^^^^^^^
    ** MANDATORY for the host bridge node
    #address-cells = <3>;
    #size-cells = <2>;

    // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
    ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
                                        ^^^^^^^^^^^^^^
            ** ?? Is this why you had problems with the offset? Should
	    ** be the cpu physical address of the start of the IO window.
             <0x2000000 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  0x1  0x0 0x4 0x1
                     0x800 0x0 0x0 0x1  0x1  0x0 0x5 0x1 
                    0x1000 0x0 0x0 0x1  0x1  0x0 0x6 0x1
                    0x1800 0x0 0x0 0x1  0x1  0x0 0x7 0x1>;
                 **                    ^^^^^^
                 ** This should be a phandle to the interrupt controller
    // PCI_DEVICE(3) INT#1()
    interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
}

I keep thinking of making a pcie-dt.h file with helper macros for
all this. :|

FWWI, I like to put a double space between the logically distinct
things in the lists.

Jason

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

[Will Deacon]

On Thu, Feb 13, 2014 at 06:06:16PM +0000, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2014 at 11:07:21AM +0000, Will Deacon wrote:
> 
> > Not in this case! kvmtool generates the following:
> 
> Well, the example is nice so someone can review it..
>  
> > 	pci {
> > 		#address-cells = <0x3>;
> > 		#size-cells = <0x2>;
> > 		#interrupt-cells = <0x1>;
> > 		compatible = "arm,pci-cam-generic";
> > 		reg = <0x0 0x40000000>;
> > 		ranges = <0x1000000 0x0 0x0 0x0 0x0 0x0 0x10000 0x2000000 0x0 0x41000000 0x0 0x41000000 0x0 0x3f000000>;
> > 		interrupt-map = <0x0 0x0 0x0 0x1 0x1 0x0 0x4 0x1 0x800 0x0 0x0 0x1 0x1 0x0 0x5 0x1 0x1000 0x0 0x0 0x1 0x1 0x0 0x6 0x1 0x1800 0x0 0x0 0x1 0x1 0x0 0x7 0x1>;
> > 		interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
> > 	};
> 
> 
> Looks like there are a few misses in the above. How about this:
> 
> pci {
>     compatible = "arm,pci-cam-generic"
>     device_type = "pci";
>     ** ^^^^^^^^^^^^^^^^^
>     ** MANDATORY for the host bridge node

Ok, I'll add that.

>     #address-cells = <3>;
>     #size-cells = <2>;
> 
>     // BUS_ADDRESS(3)  CPU_PHYSICAL(2)  SIZE(2)
>     ranges = <0x1000000 0x0 0x00000000  0x0 0x00000000  0x0 0x00010000>,
>                                         ^^^^^^^^^^^^^^
>             ** ?? Is this why you had problems with the offset? Should
> 	    ** be the cpu physical address of the start of the IO window.
>              <0x2000000 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  0x1  0x0 0x4 0x1
>                      0x800 0x0 0x0 0x1  0x1  0x0 0x5 0x1 
>                     0x1000 0x0 0x0 0x1  0x1  0x0 0x6 0x1
>                     0x1800 0x0 0x0 0x1  0x1  0x0 0x7 0x1>;
>                  **                    ^^^^^^
>                  ** This should be a phandle to the interrupt controller

That is a phandle, libfdt/kvmtool just ends up dealing with the number
directly. I'll fix in the example.

>     // PCI_DEVICE(3) INT#1()
>     interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
> }
> 
> I keep thinking of making a pcie-dt.h file with helper macros for
> all this. :|

If it's not being generated automatically, that could be useful.

> FWWI, I like to put a double space between the logically distinct
> things in the lists.


Makes sense, I may well do the same.

Thanks,

Will

Re: [PATCH v2 0/3] ARM: PCI: implement generic PCI host controller

[Jason Gunthorpe]

On Wed, Feb 12, 2014 at 08:16:08PM +0000, Will Deacon wrote:

> Like v1, I continue to support only a single controller and therefore a
> single I/O space. I'm not sure how to represent multiple controllers in the
> device-tree without inventing some horrible hacks, so any ideas in this area
> would be much appreciated.

The DT representation is very straightforward, just have more copies
of what you already have. Each DT stanza should be represented in
Linux a distinct PCI domain.

In Linux you run into two small problems
 1) PCI Domain numbers needs to be allocated dynamically
    * I think there should be a core thing to allocate a domain
      object w/ a struct device, and assign a unique domain number.
      We are already seeing drivers do things like keep track
      of their own domain numbers via a counter (pcie-designware.c)
      The host bridge object is similar to this but it isn't focused
      on a domain.
 2) The space in the IO fixed mapping needs to be allocated to PCI
    host drivers dynamically
    * pci_ioremap_io_dynamic that takes a bus address + cpu_physical
      address and returns a Linux virtual address.
      The first caller can get a nice traslation where bus address ==
      Linux virtual address, everyone after can get best efforts.

You will have overlapping physical IO bus addresses - each domain will
have a 0 IO BAR - but those will have distinct CPU physical addresses
and can then be uniquely mapped into the IO mapping. So at the struct
resource level the two domains have disjoint IO addresses, but each
domain uses a different IO offset..

Jason

Re: [PATCH v2 0/3] ARM: PCI: implement generic PCI host controller

[Arnd Bergmann]

On Thursday 13 February 2014 11:26:55 Jason Gunthorpe wrote:
> The DT representation is very straightforward, just have more copies
> of what you already have. Each DT stanza should be represented in
> Linux a distinct PCI domain.
> 
> In Linux you run into two small problems
>  1) PCI Domain numbers needs to be allocated dynamically
>     * I think there should be a core thing to allocate a domain
>       object w/ a struct device, and assign a unique domain number.
>       We are already seeing drivers do things like keep track
>       of their own domain numbers via a counter (pcie-designware.c)
>       The host bridge object is similar to this but it isn't focused
>       on a domain.

Right, see also my other comment I just sent in the "Re: [PATCH v2 3/3]
PCI: ARM: add support for generic PCI host controller" thread.

The host driver is the wrong place to pick a domain number, but someone
has to do it.

>  2) The space in the IO fixed mapping needs to be allocated to PCI
>     host drivers dynamically
>     * pci_ioremap_io_dynamic that takes a bus address + cpu_physical
>       address and returns a Linux virtual address.
>       The first caller can get a nice traslation where bus address ==
>       Linux virtual address, everyone after can get best efforts.

I think we can have a helper that everything we need to do
with the I/O space:

* parse the ranges property
* pick an appropriate virtual address window
* ioremap the physical window there
* compute the io_offset
* pick a name for the resource
* request the io resource
* register the pci_host_bridge_window

> You will have overlapping physical IO bus addresses - each domain will
> have a 0 IO BAR - but those will have distinct CPU physical addresses
> and can then be uniquely mapped into the IO mapping. So at the struct
> resource level the two domains have disjoint IO addresses, but each
> domain uses a different IO offset..

This would be the common case, but when we have a generic helper function,
it's actually not that are to handle a couple of variations of that,
which we may see in the field and can easily be described with the
existing binding.

* If we allow multiple host bridges to be in the same PCI domain with
  a split bus space, we should also allow them to have a split I/O
  space, e.g. have two 32KB windows, both with io_offset=0. This would
  imply that the second bridge can only support relocatable I/O BARs.

* Similar to that, you may have multiple 64KB windows with io_offset=0.

* Some system may have hardwire I/O windows at a high bus address larger
  than IO_SPACE_LIMIT. This would mean a *negative* io_offset. I can't
  see any reason why anyone would do this, but I also don't see a reason
  to prevent it if we can easily keep the code generic enough to handle
  it without adding extra code.

* A more obscure case would be to have multiple I/O windows on a bus.
  This is allowed by the binding and by the pci_host_bridge_window,
  and while again I don't see a use case, it also doesn't seem hard
  to do, we just keep looping for all ranges rather than stop after
  the first window.

	Arnd

Re: [PATCH v2 0/3] ARM: PCI: implement generic PCI host controller

[Jason Gunthorpe]

On Fri, Feb 14, 2014 at 12:05:27PM +0100, Arnd Bergmann wrote:

> >  2) The space in the IO fixed mapping needs to be allocated to PCI
> >     host drivers dynamically
> >     * pci_ioremap_io_dynamic that takes a bus address + cpu_physical
> >       address and returns a Linux virtual address.
> >       The first caller can get a nice traslation where bus address ==
> >       Linux virtual address, everyone after can get best efforts.
> 
> I think we can have a helper that everything we need to do
> with the I/O space:
> 
> * parse the ranges property
> * pick an appropriate virtual address window
> * ioremap the physical window there
> * compute the io_offset
> * pick a name for the resource
> * request the io resource
> * register the pci_host_bridge_window

Sounds good to me

> > You will have overlapping physical IO bus addresses - each domain will
> > have a 0 IO BAR - but those will have distinct CPU physical addresses
> > and can then be uniquely mapped into the IO mapping. So at the struct
> > resource level the two domains have disjoint IO addresses, but each
> > domain uses a different IO offset..
> 
> This would be the common case, but when we have a generic helper function,
> it's actually not that are to handle a couple of variations of that,
> which we may see in the field and can easily be described with the
> existing binding.

I agree the DT binding ranges has enough flexibility to describe all
of these cases, but I kind of circle back to the domain discussion and
ask 'Why?'. 

As far as I can see there are two reasonable ways to handle IO space:
 - The IO space is 1:1 mapped to the Physical CPU Address. In most
   cases this would require 32 bit IO BARS in all devices.
 - The IO space in a domain is always 0 -> 64k and thus only ever
   requires 16 bit BARs

And this is possible too:
 - The IO space is 1:1: mapped to Linux Virtual IO port numbers
   (which are a fiction) and devices sometimes require 32 bit
   IO BARs. This gives you lspci output that matches dmesg and
   /proc/ioport.

Things get more complex if you want to support legacy non-BAR IO (eg
VGA). Then you *really* want every domain to support 0->64k and you
need driver support to setup a window for the legacy IO port. (eg on a
multi-port root complex there is non-PCI spec hardware that routes the
VGA addresses to the root port bridge that connects to the VGA card)
Plus you probably need a memory hole around 1M..

But, I think this is overthinking things. IO space really is
deprecated, and 0->64k is a fine default for everything but very
specialized cases.

Jason

Re: [PATCH v2 0/3] ARM: PCI: implement generic PCI host controller

[Arnd Bergmann]

On Tuesday 18 February 2014 11:00:05 Jason Gunthorpe wrote:
> 
> > > You will have overlapping physical IO bus addresses - each domain will
> > > have a 0 IO BAR - but those will have distinct CPU physical addresses
> > > and can then be uniquely mapped into the IO mapping. So at the struct
> > > resource level the two domains have disjoint IO addresses, but each
> > > domain uses a different IO offset..
> > 
> > This would be the common case, but when we have a generic helper function,
> > it's actually not that are to handle a couple of variations of that,
> > which we may see in the field and can easily be described with the
> > existing binding.
> 
> I agree the DT binding ranges has enough flexibility to describe all
> of these cases, but I kind of circle back to the domain discussion and
> ask 'Why?'. 
> 
> As far as I can see there are two reasonable ways to handle IO space:
>  - The IO space is 1:1 mapped to the Physical CPU Address. In most
>    cases this would require 32 bit IO BARS in all devices.

I wouldn't consider this one reasonable ;-)

In particular, it would break /dev/port access horribly.

>  - The IO space in a domain is always 0 -> 64k and thus only ever
>    requires 16 bit BARs
> 
> And this is possible too:
>  - The IO space is 1:1: mapped to Linux Virtual IO port numbers
>    (which are a fiction) and devices sometimes require 32 bit
>    IO BARs. This gives you lspci output that matches dmesg and
>    /proc/ioport.

These two seem fine.

> Things get more complex if you want to support legacy non-BAR IO (eg
> VGA). Then you *really* want every domain to support 0->64k and you
> need driver support to setup a window for the legacy IO port. (eg on a
> multi-port root complex there is non-PCI spec hardware that routes the
> VGA addresses to the root port bridge that connects to the VGA card)
> Plus you probably need a memory hole around 1M..

For the I/O space part of this, that would be the same as your second
example above. For memory space, you need both the 640k-1M window and
the 15M-16M window. We have a couple of ARM platforms that actually
have a PCI MMIO window starting at bus address 0 with a mem_offset
matching the physical base address to facilitate VGA access,
try 'git grep -w vga_base'.

> But, I think this is overthinking things. IO space really is
> deprecated, and 0->64k is a fine default for everything but very
> specialized cases.

We may not need the entire complexity to handle all cases, but
I think it's a good idea to detect the ones we don't handle and
then warn about them. For some things, just doing it right may be
easier than detecting the case where we don't.

	Arnd