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

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

[Will Deacon]

Hello,

This small set of patches brings PCI support to mach-virt based upon an
idealised host controller (see patch 2 for more details).

This has been tested with kvmtool, for which I have a corresponding set
of patches which you can find in my kvmtool/pci branch at:

  git://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git

Once the arm64 PCI patches from Liviu have stabilised, I plan to port
this host controller to work there as well.

The main issue I can see with this code is how to describe configuration
space in the device-tree. I'm following the ePAPR PCI bindings (SS == 0)
, but this adds an ugly 'case 0:' line in the pci range parser, which
also exists in mainline for the pcie-designware.c driver.

All feedback welcome,

Will


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

 .../devicetree/bindings/pci/linux,pci-virt.txt     |  38 ++++
 arch/arm/kernel/bios32.c                           |  35 ++--
 arch/arm/mach-virt/Kconfig                         |   1 +
 drivers/pci/host/Kconfig                           |   7 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-virt.c                        | 200 +++++++++++++++++++++
 6 files changed, 257 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/linux,pci-virt.txt
 create mode 100644 drivers/pci/host/pci-virt.c

-- 
1.8.2.2

[PATCH 1/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. All that's left to take care of is the case of PCI bridges,
which need to be enabled for both IO and MEMORY, regardless of the
resource types.

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 | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 317da88ae65b..9f3c76638689 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -608,40 +608,25 @@ 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;
+	int err;
+	u16 cmd;
 
-	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;
+	if (pci_has_flag(PCI_PROBE_ONLY))
+		return 0;
 
-		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;
-	}
+	err = pci_enable_resources(dev, mask);
+	if (err)
+		return err;
 
 	/*
 	 * Bridges (eg, cardbus bridges) need to be fully enabled
 	 */
-	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
+	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) {
+		pci_read_config_word(dev, PCI_COMMAND, &cmd);
 		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;
 }
 
-- 
1.8.2.2

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

[Bjorn Helgaas]

On Tue, Feb 04, 2014 at 04:53:02PM +0000, Will Deacon wrote:
> This patch moves bios32 over to using the generic code for enabling PCI
> resources. All that's left to take care of is the case of PCI bridges,
> which need to be enabled for both IO and MEMORY, regardless of the
> resource types.
> 
> 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).

Heh, I've been looking at this, too :)  I have a similar patch for ARM and
other architectures with their own versions of pcibios_enable_device().

Several of them (arm m68k tile tegra unicore32) have this special code that
enables IO and MEMORY for bridges unconditionally.  But from a PCI
perspective, I don't know why we should do this unconditionally.  If a
bridge doesn't have an enabled MEM window or MEM BAR, I don't think we
should have to enable PCI_COMMAND_MEMORY for it.

The architectures that do this only check for valid MEM BARs, i.e., they
only look at resources 0-5, and they don't look at the window resources.

The architectures that don't enable IO and MEMORY for bridges
unconditionally check *all* the resources up to PCI_NUM_RESOURCES, which
includes the BARs, bridge windows, and any IOV resources.

The generic pci_enable_resources() does check all the resources, so I
*think* it should be safe for all architectures to use that and just drop
the check for bridges.  What do you think?

Bjorn

> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/kernel/bios32.c | 35 ++++++++++-------------------------
>  1 file changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 317da88ae65b..9f3c76638689 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -608,40 +608,25 @@ 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;
> +	int err;
> +	u16 cmd;
>  
> -	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;
> +	if (pci_has_flag(PCI_PROBE_ONLY))
> +		return 0;
>  
> -		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;
> -	}
> +	err = pci_enable_resources(dev, mask);
> +	if (err)
> +		return err;
>  
>  	/*
>  	 * Bridges (eg, cardbus bridges) need to be fully enabled
>  	 */
> -	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
> +	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) {
> +		pci_read_config_word(dev, PCI_COMMAND, &cmd);
>  		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;
>  }
>  
> -- 
> 1.8.2.2
> 

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

[Will Deacon]

Hi Bjorn,

[Adding rmk]

On Wed, Feb 12, 2014 at 01:06:50AM +0000, Bjorn Helgaas wrote:
> On Tue, Feb 04, 2014 at 04:53:02PM +0000, Will Deacon wrote:
> > This patch moves bios32 over to using the generic code for enabling PCI
> > resources. All that's left to take care of is the case of PCI bridges,
> > which need to be enabled for both IO and MEMORY, regardless of the
> > resource types.
> > 
> > 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).
> 
> Heh, I've been looking at this, too :)  I have a similar patch for ARM and
> other architectures with their own versions of pcibios_enable_device().
> 
> Several of them (arm m68k tile tegra unicore32) have this special code that
> enables IO and MEMORY for bridges unconditionally.  But from a PCI
> perspective, I don't know why we should do this unconditionally.  If a
> bridge doesn't have an enabled MEM window or MEM BAR, I don't think we
> should have to enable PCI_COMMAND_MEMORY for it.
> 
> The architectures that do this only check for valid MEM BARs, i.e., they
> only look at resources 0-5, and they don't look at the window resources.

Ok, so they would miss the bridge resources, which would explain the
additional step to enable both IO and MEM unconditionally.

> The architectures that don't enable IO and MEMORY for bridges
> unconditionally check *all* the resources up to PCI_NUM_RESOURCES, which
> includes the BARs, bridge windows, and any IOV resources.
> 
> The generic pci_enable_resources() does check all the resources, so I
> *think* it should be safe for all architectures to use that and just drop
> the check for bridges.  What do you think?

Right, your explanation certainly makes sense to me: if
pci_enable_resources() enables bridge resources, then there's no reason for
the extra logic in the caller.

The problem is, I don't have a platform to test our theory. I've added
Russell, since he might have a relevant ARM platform where we could test our
change.

Will

> > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> > index 317da88ae65b..9f3c76638689 100644
> > --- a/arch/arm/kernel/bios32.c
> > +++ b/arch/arm/kernel/bios32.c
> > @@ -608,40 +608,25 @@ 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;
> > +	int err;
> > +	u16 cmd;
> >  
> > -	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;
> > +	if (pci_has_flag(PCI_PROBE_ONLY))
> > +		return 0;
> >  
> > -		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;
> > -	}
> > +	err = pci_enable_resources(dev, mask);
> > +	if (err)
> > +		return err;
> >  
> >  	/*
> >  	 * Bridges (eg, cardbus bridges) need to be fully enabled
> >  	 */
> > -	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
> > +	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) {
> > +		pci_read_config_word(dev, PCI_COMMAND, &cmd);
> >  		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;
> >  }
> >  
> > -- 
> > 1.8.2.2
> > 
> 

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

[Will Deacon]

This patch adds support for an extremely simple virtual PCI host
controller. The controller itself has no configuration registers, and
has its address spaces described entirely by the device-tree (using the
bindings described by ePAPR). This allows emulations, such as kvmtool,
to provide a simple means for a guest Linux instance to make use of
PCI devices.

Corresponding documentation is added for the DT binding.

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

diff --git a/Documentation/devicetree/bindings/pci/linux,pci-virt.txt b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
new file mode 100644
index 000000000000..54668a283498
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
@@ -0,0 +1,38 @@
+* ARM Basic Virtual PCI controller
+
+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 have a control interface visible to 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 "linux,pci-virt"
+
+- ranges         : As described in IEEE Std 1275-1994, but must provide
+                   at least a definition of the Configuration Space plus
+                   one or both of IO and Memory Space.
+
+- #address-cells : Must be 3
+
+- #size-cells    : Must be 2
+
+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 a 24-bit offset:
+
+        cfg_offset(bus, device, function, register) =
+                bus << 16 | device << 11 | function << 8 | 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..fd4460573b81 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_VIRT_HOST
+	bool "Virtual PCI host controller"
+	depends on ARM && OF
+	help
+	  Say Y here if you want to support a very simple virtual 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..9b6775d95d3b 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_VIRT_HOST) += pci-virt.o
diff --git a/drivers/pci/host/pci-virt.c b/drivers/pci/host/pci-virt.c
new file mode 100644
index 000000000000..ded01474453b
--- /dev/null
+++ b/drivers/pci/host/pci-virt.c
@@ -0,0 +1,200 @@
+/*
+ * Very basic PCI host controller driver targetting virtual machines
+ * (e.g. the PCI emulation provided by kvmtool).
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (C) 2014 ARM Limited
+ *
+ * Author: Will Deacon <will.deacon@arm.com>
+ *
+ * This driver currently supports (per instance):
+ *	- A single controller
+ *	- A single memory space and/or port space
+ *	- A memory-mapped configuration space
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/platform_device.h>
+
+struct virt_pci {
+	struct device	*dev;
+
+	struct resource	cfg;
+	struct resource	io;
+	struct resource	mem;
+
+	void __iomem	*cfg_base;
+};
+
+static void __iomem *virt_pci_config_address(struct pci_bus *bus,
+					     unsigned int devfn,
+					     int where)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct virt_pci *pci = sys->private_data;
+	void __iomem *addr = pci->cfg_base;
+
+	/*
+	 * We construct config space addresses by simply sandwiching
+	 * together all of the PCI address components and using the
+	 * result as an offset into a 16M region.
+	 */
+	return addr + (((u32)bus->number << 16) | (devfn << 8) | where);
+}
+
+
+static int virt_pci_config_read(struct pci_bus *bus, unsigned int devfn,
+				int where, int size, u32 *val)
+{
+	void __iomem *addr = virt_pci_config_address(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		*val = readb(addr);
+		break;
+	case 2:
+		*val = readw(addr);
+		break;
+	default:
+		*val = readl(addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int virt_pci_config_write(struct pci_bus *bus, unsigned int devfn,
+				 int where, int size, u32 val)
+{
+	void __iomem *addr = virt_pci_config_address(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		writeb(val, addr);
+		break;
+	case 2:
+		writew(val, addr);
+		break;
+	default:
+		writel(val, addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops virt_pci_ops = {
+	.read	= virt_pci_config_read,
+	.write	= virt_pci_config_write,
+};
+
+static int virt_pci_setup(int nr, struct pci_sys_data *sys)
+{
+	struct virt_pci *pci = sys->private_data;
+
+	if (resource_type(&pci->io)) {
+		pci_add_resource(&sys->resources, &pci->io);
+		pci_ioremap_io(nr * resource_size(&pci->io), pci->io.start);
+	}
+
+	if (resource_type(&pci->mem))
+		pci_add_resource(&sys->resources, &pci->mem);
+
+	pci->cfg_base = devm_ioremap_resource(pci->dev, &pci->cfg);
+	return !IS_ERR(pci->cfg_base);
+}
+
+static const struct of_device_id virt_pci_of_match[] = {
+	{ .compatible = "linux,pci-virt" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, virt_pci_of_match);
+
+static int virt_pci_probe(struct platform_device *pdev)
+{
+	struct hw_pci hw;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	struct virt_pci *pci;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	if (of_pci_range_parser_init(&parser, np)) {
+		dev_err(dev, "missing \"ranges\" property\n");
+		return -EINVAL;
+	}
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->dev = dev;
+	for_each_of_pci_range(&parser, &range) {
+		u32 restype = range.flags & IORESOURCE_TYPE_BITS;
+
+		switch (restype) {
+		case IORESOURCE_IO:
+			if (resource_type(&pci->io))
+				dev_warn(dev,
+					 "ignoring additional io resource\n");
+			else
+				of_pci_range_to_resource(&range, np, &pci->io);
+			break;
+		case IORESOURCE_MEM:
+			if (resource_type(&pci->mem))
+				dev_warn(dev,
+					 "ignoring additional mem resource\n");
+			else
+				of_pci_range_to_resource(&range, np, &pci->mem);
+			break;
+		case 0:	/* cfg */
+			if (resource_type(&pci->cfg)) {
+				dev_warn(dev,
+					 "ignoring additional cfg resource\n");
+			} else {
+				of_pci_range_to_resource(&range, np, &pci->cfg);
+				pci->cfg.flags |= IORESOURCE_MEM;
+			}
+			break;
+		default:
+			dev_warn(dev,
+				"ignoring unknown/unsupported resource type %x\n",
+				 restype);
+		}
+	}
+
+	memset(&hw, 0, sizeof(hw));
+	hw.nr_controllers	= 1;
+	hw.private_data		= (void **)&pci;
+	hw.setup		= virt_pci_setup;
+	hw.map_irq		= of_irq_parse_and_map_pci;
+	hw.ops			= &virt_pci_ops;
+	pci_common_init_dev(dev, &hw);
+	return 0;
+}
+
+static struct platform_driver virt_pci_driver = {
+	.driver = {
+		.name = "pci-virt",
+		.owner = THIS_MODULE,
+		.of_match_table = virt_pci_of_match,
+	},
+	.probe = virt_pci_probe,
+};
+module_platform_driver(virt_pci_driver);
+
+MODULE_DESCRIPTION("Virtual PCI host driver");
+MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
+MODULE_LICENSE("GPLv2");
-- 
1.8.2.2

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

[Arnd Bergmann]

On Tuesday 04 February 2014 16:53:03 Will Deacon wrote:
> +
> +- ranges         : As described in IEEE Std 1275-1994, but must provide
> +                   at least a definition of the Configuration Space plus
> +                   one or both of IO and Memory Space.
> +

I might need to reread the spec, but I think the config space is not
actually supposed to be in the 'ranges' of the host bridge at all,
and it should just be listed in the 'reg'.

IIRC the reason why the config space is part of the three-cell address
is so that you can have funky ways to say "memory space of the device
with bus/dev/fn is actually translated to address X rather then Y".

It's too late to change that for the other drivers now, after the
binding is established.

> +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 a 24-bit offset:
> +
> +        cfg_offset(bus, device, function, register) =
> +                bus << 16 | device << 11 | function << 8 | register

This won't allow extended config space. Why not just do the
regular mmconfig layout and make this:

	cfg_offset(bus, device, function, register) =
		bus << 20 | device << 15 | function << 12 | register;

> +static int virt_pci_setup(int nr, struct pci_sys_data *sys)
> +{
> +	struct virt_pci *pci = sys->private_data;
> +
> +	if (resource_type(&pci->io)) {
> +		pci_add_resource(&sys->resources, &pci->io);
> +		pci_ioremap_io(nr * resource_size(&pci->io), pci->io.start);
> +	}

This should really compute an io_offset.

> +	if (resource_type(&pci->mem))
> +		pci_add_resource(&sys->resources, &pci->mem);

and also a mem_offset, which is something different.

> +	pci->cfg_base = devm_ioremap_resource(pci->dev, &pci->cfg);
> +	return !IS_ERR(pci->cfg_base);
> +}
> +
> +static const struct of_device_id virt_pci_of_match[] = {
> +	{ .compatible = "linux,pci-virt" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, virt_pci_of_match);

I don't think we even want "virt" in the compatible string. The
binding should be generic enough that it can actually work with
real hardware.

> +	for_each_of_pci_range(&parser, &range) {
> +		u32 restype = range.flags & IORESOURCE_TYPE_BITS;
> +
> +		switch (restype) {
> +		case IORESOURCE_IO:
> +			if (resource_type(&pci->io))
> +				dev_warn(dev,
> +					 "ignoring additional io resource\n");
> +			else
> +				of_pci_range_to_resource(&range, np, &pci->io);
> +			break;
> +		case IORESOURCE_MEM:
> +			if (resource_type(&pci->mem))
> +				dev_warn(dev,
> +					 "ignoring additional mem resource\n");
> +			else
> +				of_pci_range_to_resource(&range, np, &pci->mem);
> +			break;

This shows once more that the range parser code is suboptimal. So far
every single driver got the I/O space wrong here, because the obvious
way to write this function is also completely wrong.

What you get out of "of_pci_range_to_resource(&range, np, &pci->io)"
is not the resource you want to pass into pci_add_resource()
later.

> +	memset(&hw, 0, sizeof(hw));
> +	hw.nr_controllers	= 1;
> +	hw.private_data		= (void **)&pci;
> +	hw.setup		= virt_pci_setup;
> +	hw.map_irq		= of_irq_parse_and_map_pci;
> +	hw.ops			= &virt_pci_ops;
> +	pci_common_init_dev(dev, &hw);

Since most fields here are constant, I'd just write this as

	struct hw_pci hw = {
		.nr_controllers = 1,
		.setup = virt_pci_setup,
		...
	};

	Arnd

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

[Will Deacon]

Hi Arnd,

Thanks for having a look.

On Tue, Feb 04, 2014 at 07:13:49PM +0000, Arnd Bergmann wrote:
> On Tuesday 04 February 2014 16:53:03 Will Deacon wrote:
> > +
> > +- ranges         : As described in IEEE Std 1275-1994, but must provide
> > +                   at least a definition of the Configuration Space plus
> > +                   one or both of IO and Memory Space.
> > +
> 
> I might need to reread the spec, but I think the config space is not
> actually supposed to be in the 'ranges' of the host bridge at all,
> and it should just be listed in the 'reg'.

This wasn't at all clear to me (I listed it in the cover-letter as being
something to sort out).

> IIRC the reason why the config space is part of the three-cell address
> is so that you can have funky ways to say "memory space of the device
> with bus/dev/fn is actually translated to address X rather then Y".
> 
> It's too late to change that for the other drivers now, after the
> binding is established.

The spec is based on the idea that open-firmware enumerates your entire PCI
bus topology, then provides the Conriguation Space address for each device
using a reg property.

Since:

  (a) This doesn't match what we're planning to support
  (b) Runs the risk of making the "reg" encoding something specific to this
      driver
  (c) Doesn't match how we describe Memory and IO Spaces
  (d) There is already precendence in mainline

I chose to use "ranges" instead.

Now, if "reg" is definitely the correct thing to do, is it simply a matter
of putting the Configuration Space base address in there, or do we also need
to do the rest of what ePAPR says (expansion ROM details, ...)? I don't like
the idea of enumerating the entire bus in the DT when we don't need to.

> > +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 a 24-bit offset:
> > +
> > +        cfg_offset(bus, device, function, register) =
> > +                bus << 16 | device << 11 | function << 8 | register
> 
> This won't allow extended config space. Why not just do the
> regular mmconfig layout and make this:
> 
> 	cfg_offset(bus, device, function, register) =
> 		bus << 20 | device << 15 | function << 12 | register;

Is it worth adding a DT property to support both, or is ECAM the only thing
to care about? I'm happy either way, although I'll need to hack kvmtool to
use the new scheme.

> > +static int virt_pci_setup(int nr, struct pci_sys_data *sys)
> > +{
> > +	struct virt_pci *pci = sys->private_data;
> > +
> > +	if (resource_type(&pci->io)) {
> > +		pci_add_resource(&sys->resources, &pci->io);
> > +		pci_ioremap_io(nr * resource_size(&pci->io), pci->io.start);
> > +	}
> 
> This should really compute an io_offset.
> 
> > +	if (resource_type(&pci->mem))
> > +		pci_add_resource(&sys->resources, &pci->mem);
> 
> and also a mem_offset, which is something different.

As somebody new to PCI, I'm afraid you've lost me here. Are you referring to
using pci_add_resource_offset instead, then removing my restriction on
having a single resource from the parsing code?

> > +	pci->cfg_base = devm_ioremap_resource(pci->dev, &pci->cfg);
> > +	return !IS_ERR(pci->cfg_base);
> > +}
> > +
> > +static const struct of_device_id virt_pci_of_match[] = {
> > +	{ .compatible = "linux,pci-virt" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, virt_pci_of_match);
> 
> I don't think we even want "virt" in the compatible string. The
> binding should be generic enough that it can actually work with
> real hardware.

Sure. How about "arm,pci-generic" ? Alternatives are
"arm,pcie-generic" or "linux,pci-generic".

> > +	for_each_of_pci_range(&parser, &range) {
> > +		u32 restype = range.flags & IORESOURCE_TYPE_BITS;
> > +
> > +		switch (restype) {
> > +		case IORESOURCE_IO:
> > +			if (resource_type(&pci->io))
> > +				dev_warn(dev,
> > +					 "ignoring additional io resource\n");
> > +			else
> > +				of_pci_range_to_resource(&range, np, &pci->io);
> > +			break;
> > +		case IORESOURCE_MEM:
> > +			if (resource_type(&pci->mem))
> > +				dev_warn(dev,
> > +					 "ignoring additional mem resource\n");
> > +			else
> > +				of_pci_range_to_resource(&range, np, &pci->mem);
> > +			break;
> 
> This shows once more that the range parser code is suboptimal. So far
> every single driver got the I/O space wrong here, because the obvious
> way to write this function is also completely wrong.

I see you mentioned to Liviu that you should register a logical resource,
rather than physical resource returned from the parser. It seems odd that
I/O space appears to work with this code as-is (I've tested it on arm using
kvmtool by removing the memory bars).

> What you get out of "of_pci_range_to_resource(&range, np, &pci->io)"
> is not the resource you want to pass into pci_add_resource()
> later.

Do I need to open-code the resource translation from phys -> logical?

> 
> > +	memset(&hw, 0, sizeof(hw));
> > +	hw.nr_controllers	= 1;
> > +	hw.private_data		= (void **)&pci;
> > +	hw.setup		= virt_pci_setup;
> > +	hw.map_irq		= of_irq_parse_and_map_pci;
> > +	hw.ops			= &virt_pci_ops;
> > +	pci_common_init_dev(dev, &hw);
> 
> Since most fields here are constant, I'd just write this as
> 
> 	struct hw_pci hw = {
> 		.nr_controllers = 1,
> 		.setup = virt_pci_setup,
> 		...
> 	};

Can do.

Thanks,

Will

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

[Jason Gunthorpe]

On Wed, Feb 05, 2014 at 07:09:47PM +0000, Will Deacon wrote:
> Hi Arnd,
> 
> Thanks for having a look.
> 
> On Tue, Feb 04, 2014 at 07:13:49PM +0000, Arnd Bergmann wrote:
> > On Tuesday 04 February 2014 16:53:03 Will Deacon wrote:
> > > +
> > > +- ranges         : As described in IEEE Std 1275-1994, but must provide
> > > +                   at least a definition of the Configuration Space plus
> > > +                   one or both of IO and Memory Space.
> > > +
> > 
> > I might need to reread the spec, but I think the config space is not
> > actually supposed to be in the 'ranges' of the host bridge at all,
> > and it should just be listed in the 'reg'.
> 
> This wasn't at all clear to me (I listed it in the cover-letter as being
> something to sort out).

When we talked about this earlier on the DT bindings list the
consensus seemed to be that configuration MMIO ranges should only be
used if the underlying memory was exactly ECAM, and was not to be used
for random configuration related register blocks.

The rational being that generic code, upon seeing that ranges entry,
could just go ahead and assume ECAM mapping.

> Now, if "reg" is definitely the correct thing to do, is it simply a matter
> of putting the Configuration Space base address in there, or do we also need
> to do the rest of what ePAPR says (expansion ROM details, ...)? I don't like
> the idea of enumerating the entire bus in the DT when we don't need to.

If you use 'reg' then it is a private base address to your driver and
you can do whatever you want with it.

Most of the ePAPR things are only needed if the FW is going to
communicate detailed information to the OS, for an environment that
relies on Linux resource assignment and discovery you can just ignore
all that.

> > This won't allow extended config space. Why not just do the
> > regular mmconfig layout and make this:
> > 
> > 	cfg_offset(bus, device, function, register) =
> > 		bus << 20 | device << 15 | function << 12 | register;
> 
> Is it worth adding a DT property to support both, or is ECAM the only thing
> to care about? I'm happy either way, although I'll need to hack kvmtool to
> use the new scheme.

If you use ECAM then I wonder if your driver might be a generic SBSA
driver too?

I can't think of a reason to support alternate MMIO layouts if
kvmtool is the only user and can be changed.

> > I don't think we even want "virt" in the compatible string. The
> > binding should be generic enough that it can actually work with
> > real hardware.
> 
> Sure. How about "arm,pci-generic" ? Alternatives are
> "arm,pcie-generic" or "linux,pci-generic".

arm,pci-ecam-generic ?

Regards,
Jason

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

[Peter Maydell]

On 5 February 2014 19:27, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> I can't think of a reason to support alternate MMIO layouts if
> kvmtool is the only user and can be changed.

I expect we'll get a QEMU implementation too at some point,
but we should be able to just make that match whatever you
decide is the correct layout and behaviour.

The only constraint QEMU has which kvmtool doesn't is that
we care about not randomly shuffling things around between
QEMU versions, so whatever we pick we're more or less
stuck with. So a layout which leaves scope for "supports
PCI now, may do PCI-e later, MSI after that" would be good.
(My PCI knowledge is very piecemeal, I hope that makes sense...)

thanks
-- PMM

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

[Arnd Bergmann]

On Wednesday 05 February 2014 19:09:47 Will Deacon wrote:

> On Tue, Feb 04, 2014 at 07:13:49PM +0000, Arnd Bergmann wrote:
> > On Tuesday 04 February 2014 16:53:03 Will Deacon wrote:
> Now, if "reg" is definitely the correct thing to do, is it simply a matter
> of putting the Configuration Space base address in there, or do we also need
> to do the rest of what ePAPR says (expansion ROM details, ...)? I don't like
> the idea of enumerating the entire bus in the DT when we don't need to.

You won't have to worry about the per-device stuff, aside from that,
do what Jason says ;-)
 
> > > +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 a 24-bit offset:
> > > +
> > > +        cfg_offset(bus, device, function, register) =
> > > +                bus << 16 | device << 11 | function << 8 | register
> > 
> > This won't allow extended config space. Why not just do the
> > regular mmconfig layout and make this:
> > 
> > 	cfg_offset(bus, device, function, register) =
> > 		bus << 20 | device << 15 | function << 12 | register;
> 
> Is it worth adding a DT property to support both, or is ECAM the only thing
> to care about? I'm happy either way, although I'll need to hack kvmtool to
> use the new scheme.

For any 64-bit system, ECAM is the only thing we need. On 32-bit
systems, we can't just map the entire config space though, since
that would require 256MB of vmalloc space. Ideally the driver is
smart enough to map only the space for the present buses (1MB
per bus), which I think is what x86 does, or one page per
PCI function that is present, but that can be tricky for hotplugging.

> > > +static int virt_pci_setup(int nr, struct pci_sys_data *sys)
> > > +{
> > > +	struct virt_pci *pci = sys->private_data;
> > > +
> > > +	if (resource_type(&pci->io)) {
> > > +		pci_add_resource(&sys->resources, &pci->io);
> > > +		pci_ioremap_io(nr * resource_size(&pci->io), pci->io.start);
> > > +	}
> > 
> > This should really compute an io_offset.
> > 
> > > +	if (resource_type(&pci->mem))
> > > +		pci_add_resource(&sys->resources, &pci->mem);
> > 
> > and also a mem_offset, which is something different.
> 
> As somebody new to PCI, I'm afraid you've lost me here. Are you referring to
> using pci_add_resource_offset instead, then removing my restriction on
> having a single resource from the parsing code?

I mean pci_add_resource_offset, but I don't understand what the
restriction is that you are talking about. To elaborate on the offsets,
the common case is that the PCI memory space is the same as the
host physical address space for both MMIO and DMA, while you have
only one PCI host with a single I/O space range from port 0 to 65536.

If you mandate that, your code is actually correct and you do not
require an io_offset or mem_offset.

In practice, there can be various ways that a system requires something
more complex:

* You can have a memory space range that puts PCI bus address zero
  at the start of the pci->mem resource. In this case, you have
  mem_offset = pci->mem.start. We should probably try not to do
  this, but there is existing hardware doing it.

* You might have multiple sections of the PCI bus space mapped
  into CPU physical space. If you want to support legacy VGA
  console, you probably want to map the first 16MB of the bus
  space at an arbitrary location (with the mem_offset as above),
  plus a second, larger section of the bus space with an identity
  mapping (mem_offset_= 0) for all devices other than VGA.
  You'd also need to copy some VGA specific code from arm32 to
  arm64 to actually make this work.

* If you have two PCI host bridges and each of them comes with
  its own I/O space range starting between 0x0-0xffff, you have
  to map one of them into logical I/O space address 0x10000-0x1ffff
  and set io_offset = 0x10000 for that bus.

* Alternatively, the second bus in that case can use *bus* I/O
  port address 0x10000-0x1ffff and use io_offset=0, but that
  prevents you from having legacy ISA I/O ports on the
  second bus, since they are hardwired to a known port number
  in the range 0x0-0x1000 (the first 4KB). This includes VGA
  console.

> > > +	for_each_of_pci_range(&parser, &range) {
> > > +		u32 restype = range.flags & IORESOURCE_TYPE_BITS;
> > > +
> > > +		switch (restype) {
> > > +		case IORESOURCE_IO:
> > > +			if (resource_type(&pci->io))
> > > +				dev_warn(dev,
> > > +					 "ignoring additional io resource\n");
> > > +			else
> > > +				of_pci_range_to_resource(&range, np, &pci->io);
> > > +			break;
> > > +		case IORESOURCE_MEM:
> > > +			if (resource_type(&pci->mem))
> > > +				dev_warn(dev,
> > > +					 "ignoring additional mem resource\n");
> > > +			else
> > > +				of_pci_range_to_resource(&range, np, &pci->mem);
> > > +			break;
> > 
> > This shows once more that the range parser code is suboptimal. So far
> > every single driver got the I/O space wrong here, because the obvious
> > way to write this function is also completely wrong.
> 
> I see you mentioned to Liviu that you should register a logical resource,
> rather than physical resource returned from the parser. It seems odd that
> I/O space appears to work with this code as-is (I've tested it on arm using
> kvmtool by removing the memory bars).

what do you see in /proc/ioports and /proc/iomem then?

> > What you get out of "of_pci_range_to_resource(&range, np, &pci->io)"
> > is not the resource you want to pass into pci_add_resource()
> > later.
> 
> Do I need to open-code the resource translation from phys -> logical?

I think we should have some common infrastructure that lets you
get this right more easily.

	Arnd

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

[Jason Gunthorpe]

On Wed, Feb 05, 2014 at 09:26:17PM +0100, Arnd Bergmann wrote:
> > > What you get out of "of_pci_range_to_resource(&range, np, &pci->io)"
> > > is not the resource you want to pass into pci_add_resource()
> > > later.

Right, of_pci_range_to_resource returns the CPU MMIO address. A big
problem here is that struct resource is being re-used for bus
physical, CPU MMIO, and Linux Driver addressing domains without any
helpful tagging.

typedef struct resource resource_bus;
typedef struct resource resource_cpu;

?

> > Do I need to open-code the resource translation from phys -> logical?
> 
> I think we should have some common infrastructure that lets you
> get this right more easily.

The offset stuff seems to be very confusing to people, removing it
from the APIs and forcing drivers to talk about bus addresess, CPU
addresses and internal Linux addresses seems a bit more plain?

What do you think about something like this:

int of_pci_alloc_io(.. resources,
                    struct of_pci_range *range,
                    struct device_node *np)
{
  struct resource bus_address, mmio_window, res;

  bus_address.start = range->pci_addr;
  bus_address.end = range->pci_addr + range->size - 1;

  mmio_window.start = range->cpu_addr;
  mmio_window.end = range->cpu_addr + range->size - 1;

  /* Input bus_address - addresses seen on the bus
           mmio_window - physical CPU address to create the bus
	   	         addreses
     Output res - Address suitable for use in drivers
     This does the pci_ioremap_io too */
  pci_alloc_virtual_io_window(&bus_address, &mmio_window, &res);

  /* bus_address - addresses seen on the bus
     res - matching driver view for the bus addresses */
  pci_add_resource_bus(&resources, &bus_address, &res);
}

And a similar function for MMIO that just omits
pci_alloc_virtual_io_window.

Jason

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

[Arnd Bergmann]

On Wednesday 05 February 2014, Jason Gunthorpe wrote:
> On Wed, Feb 05, 2014 at 09:26:17PM +0100, Arnd Bergmann wrote:
> > > > What you get out of "of_pci_range_to_resource(&range, np, &pci->io)"
> > > > is not the resource you want to pass into pci_add_resource()
> > > > later.
> 
> Right, of_pci_range_to_resource returns the CPU MMIO address. A big
> problem here is that struct resource is being re-used for bus
> physical, CPU MMIO, and Linux Driver addressing domains without any
> helpful tagging.
> 
> typedef struct resource resource_bus;
> typedef struct resource resource_cpu;
> 
> ?

I fear the resource management needs a bigger overhaul than than.
Most importantly, 'struct resource' is not connected to 'struct device'
at the moment, and it only deals with resource types that were around
15 years ago.

> > > Do I need to open-code the resource translation from phys -> logical?
> > 
> > I think we should have some common infrastructure that lets you
> > get this right more easily.
> 
> The offset stuff seems to be very confusing to people, removing it
> from the APIs and forcing drivers to talk about bus addresess, CPU
> addresses and internal Linux addresses seems a bit more plain?

Interesting idea. I'm not sure how Bjorn will like that
after he just did an overhaul of all users of the offset
API some time ago, but let's see what he thinks.

> What do you think about something like this:
> 
> int of_pci_alloc_io(.. resources,
>                     struct of_pci_range *range,
>                     struct device_node *np)
> {
>   struct resource bus_address, mmio_window, res;
> 
>   bus_address.start = range->pci_addr;
>   bus_address.end = range->pci_addr + range->size - 1;
> 
>   mmio_window.start = range->cpu_addr;
>   mmio_window.end = range->cpu_addr + range->size - 1;
> 
>   /* Input bus_address - addresses seen on the bus
>            mmio_window - physical CPU address to create the bus
> 	   	         addreses
>      Output res - Address suitable for use in drivers
>      This does the pci_ioremap_io too */
>   pci_alloc_virtual_io_window(&bus_address, &mmio_window, &res);
> 
>   /* bus_address - addresses seen on the bus
>      res - matching driver view for the bus addresses */
>   pci_add_resource_bus(&resources, &bus_address, &res);
> }
> 
> And a similar function for MMIO that just omits
> pci_alloc_virtual_io_window.

It certainly seems workable. OTOH if we just manage to do a
helper that scans the OF ranges, allocates the I/O window,
remaps it and calls the existing pci_add_resource_offset()
helper, PCI host drivers don't need to worry about the
io_offsets computation either and just need to pull out the
correct window locations if they need to set up the hardware
translation windows (which I'd hope we can often let the boot
loader take care of).

	Arnd

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

[Russell King - ARM Linux]

On Thu, Feb 06, 2014 at 09:28:52AM +0100, Arnd Bergmann wrote:
> It certainly seems workable. OTOH if we just manage to do a
> helper that scans the OF ranges, allocates the I/O window,
> remaps it and calls the existing pci_add_resource_offset()
> helper, PCI host drivers don't need to worry about the
> io_offsets computation either and just need to pull out the
> correct window locations if they need to set up the hardware
> translation windows (which I'd hope we can often let the boot
> loader take care of).

Wrong.  Think about it for a moment.

Let's say you have two host bridges.  One host bridge has an IO window
which maps to bus I/O address 0 at 0x40000000.  The other host bridge
has it's IO window which maps bus I/O address 0 to 0x48000000.

So, the contents of the /hardware/ BARs is going to be in the range
of 0x0000 to 0xffff.  That means the value found in them is meaningless
without knowing which bus it's on.

Now, remember we said that I/O was going to be in a fixed location of
a fixed size, that being 0xfee00000 and 1MB in size.  So, we map the
first IO window to 0xfee00000.  What about the other one?  Well,
that could be mapped to 0xfee10000.

However, we need the IO addresses visible to the Linux kernel to be
offset by 0x10000 for the second bus - merely reading the BAR and
storing that in a resource, for the driver to later pick up and pass
into inb()/outb() won't work.  There needs to be offsetting.  This is
the exact reason why we have the offsetting for IO windows.

Exactly the same goes for memory windows as well.  It's no good working
in the hosts physical address space when looking at BARs, because they're
not in that address space.  They're in the bus address space which can
be entirely different.

So, whenever you enumerate a PCI bus, and read the resource information
out of the BARs, you /must/ know how that address region specified in
the BAR as a /bus/ address maps to the /host/ address space.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Arnd Bergmann]

On Thursday 06 February 2014, Russell King - ARM Linux wrote:
> On Thu, Feb 06, 2014 at 09:28:52AM +0100, Arnd Bergmann wrote:
> > It certainly seems workable. OTOH if we just manage to do a
> > helper that scans the OF ranges, allocates the I/O window,
> > remaps it and calls the existing pci_add_resource_offset()
> > helper, PCI host drivers don't need to worry about the
> > io_offsets computation either and just need to pull out the
> > correct window locations if they need to set up the hardware
> > translation windows (which I'd hope we can often let the boot
> > loader take care of).

...

> So, whenever you enumerate a PCI bus, and read the resource information
> out of the BARs, you must know how that address region specified in
> the BAR as a bus address maps to the host address space.
> 

None of that contradicts what I wrote. Please try to understand what
I suggested, which is to have a common way to communicate that
information from DT to the PCI core without involving the PCI host
bridge driver.

All the bus scanning is done in common code, which already knows
how to factor in io_offset and mem_offset. The mem_offset can be
trivially computed from the ranges property, and the io_offset
is known by the time we call pci_ioremap_io() or rather a
replacement such as the one I proposed that also contains an
allocator.

	Arnd

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

[Russell King - ARM Linux]

On Sun, Feb 09, 2014 at 09:18:19PM +0100, Arnd Bergmann wrote:
> On Thursday 06 February 2014, Russell King - ARM Linux wrote:
> > On Thu, Feb 06, 2014 at 09:28:52AM +0100, Arnd Bergmann wrote:
> > > It certainly seems workable. OTOH if we just manage to do a
> > > helper that scans the OF ranges, allocates the I/O window,
> > > remaps it and calls the existing pci_add_resource_offset()
> > > helper, PCI host drivers don't need to worry about the
> > > io_offsets computation either and just need to pull out the
> > > correct window locations if they need to set up the hardware
> > > translation windows (which I'd hope we can often let the boot
> > > loader take care of).
> 
> ...
> 
> > So, whenever you enumerate a PCI bus, and read the resource information
> > out of the BARs, you must know how that address region specified in
> > the BAR as a bus address maps to the host address space.
> > 
> 
> None of that contradicts what I wrote. Please try to understand what
> I suggested, which is to have a common way to communicate that
> information from DT to the PCI core without involving the PCI host
> bridge driver.

Please explain it better then.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Arnd Bergmann]

On Sunday 09 February 2014, Russell King - ARM Linux wrote:
> On Sun, Feb 09, 2014 at 09:18:19PM +0100, Arnd Bergmann wrote:
> > On Thursday 06 February 2014, Russell King - ARM Linux wrote:
> > 
> > > So, whenever you enumerate a PCI bus, and read the resource information
> > > out of the BARs, you must know how that address region specified in
> > > the BAR as a bus address maps to the host address space.
> > > 
> > 
> > None of that contradicts what I wrote. Please try to understand what
> > I suggested, which is to have a common way to communicate that
> > information from DT to the PCI core without involving the PCI host
> > bridge driver.
> 
> Please explain it better then.
>

Let me try again: Looking at the dw_pcie driver (since that is one of
the few that gets it mostly right), we have quite a bit of generic
code in dw_pcie_host_init() and in dw_pcie_setup(). All the DT parsing
in there just implements the generic PCI DT binding, and what the
setup function does depends exclusively on what comes out of the
parser.

If we manage to move all the common parts into a generic helper
function that gets called by the setup() on arm32, we no longer
need to worry about host drivers implementing an incorrect DT
parser or getting the io_offset calculation wrong, because they
don't actually see any of that, plus we save a lot of duplicate
code.

How it fits together with the new arm64 pci support is a different
question, but if it's just a helper function, it should really work
on any architecture.

	Arnd

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

[Will Deacon]

Hi Arnd, Jason,

First of all, thanks for the really helpful feedback. I'll take it on-board
for v2.

On Wed, Feb 05, 2014 at 08:26:17PM +0000, Arnd Bergmann wrote:
> On Wednesday 05 February 2014 19:09:47 Will Deacon wrote:
> > On Tue, Feb 04, 2014 at 07:13:49PM +0000, Arnd Bergmann wrote:
> > > This should really compute an io_offset.
> > > 
> > > > +	if (resource_type(&pci->mem))
> > > > +		pci_add_resource(&sys->resources, &pci->mem);
> > > 
> > > and also a mem_offset, which is something different.
> > 
> > As somebody new to PCI, I'm afraid you've lost me here. Are you referring to
> > using pci_add_resource_offset instead, then removing my restriction on
> > having a single resource from the parsing code?
> 
> I mean pci_add_resource_offset, but I don't understand what the
> restriction is that you are talking about. To elaborate on the offsets,
> the common case is that the PCI memory space is the same as the
> host physical address space for both MMIO and DMA, while you have
> only one PCI host with a single I/O space range from port 0 to 65536.
> 
> If you mandate that, your code is actually correct and you do not
> require an io_offset or mem_offset.

Right, so that's what I've currently been relying on. If I mandate that,
will I be making this driver significantly less useful?

> In practice, there can be various ways that a system requires something
> more complex:
> 
> * You can have a memory space range that puts PCI bus address zero
>   at the start of the pci->mem resource. In this case, you have
>   mem_offset = pci->mem.start. We should probably try not to do
>   this, but there is existing hardware doing it.

If it's not the common case, then the generic driver might not need to care
(at least, initially).

> * You might have multiple sections of the PCI bus space mapped
>   into CPU physical space. If you want to support legacy VGA
>   console, you probably want to map the first 16MB of the bus
>   space at an arbitrary location (with the mem_offset as above),
>   plus a second, larger section of the bus space with an identity
>   mapping (mem_offset_= 0) for all devices other than VGA.
>   You'd also need to copy some VGA specific code from arm32 to
>   arm64 to actually make this work.

Again, I'd rather cross that bridge (no pun intended) when we decide we want
legacy VGA.

> * If you have two PCI host bridges and each of them comes with
>   its own I/O space range starting between 0x0-0xffff, you have
>   to map one of them into logical I/O space address 0x10000-0x1ffff
>   and set io_offset = 0x10000 for that bus.

Right, this sounds a lot more plausible from the perspective of a generic
driver. Since the current code only allows exactly one I/O space region, we
avoid the issue but I can remove that restriction (that I mentioned earlier)
and offset the region based on the bridge.

> > > This shows once more that the range parser code is suboptimal. So far
> > > every single driver got the I/O space wrong here, because the obvious
> > > way to write this function is also completely wrong.
> > 
> > I see you mentioned to Liviu that you should register a logical resource,
> > rather than physical resource returned from the parser. It seems odd that
> > I/O space appears to work with this code as-is (I've tested it on arm using
> > kvmtool by removing the memory bars).
> 
> what do you see in /proc/ioports and /proc/iomem then?

bash-4.2# cat /proc/ioports
00006200-000065ff : virtio-pci
00006600-000069ff : virtio-pci
00006a00-00006dff : virtio-pci
00006e00-000071ff : virtio-pci

bash-4.2# cat /proc/iomem
40000000-40ffffff : /pci
41000400-410005ff : virtio-pci
41000c00-41000dff : virtio-pci
41001400-410015ff : virtio-pci
41001c00-41001dff : virtio-pci
80000000-93ffffff : System RAM
  80008000-8053df0b : Kernel code
  80570000-805c07fb : Kernel data

> > > What you get out of "of_pci_range_to_resource(&range, np, &pci->io)"
> > > is not the resource you want to pass into pci_add_resource()
> > > later.
> > 
> > Do I need to open-code the resource translation from phys -> logical?
> 
> I think we should have some common infrastructure that lets you
> get this right more easily.

Okey doke, is anybody working on that? (I see the follow up from Jason, but
it's not clear whether that's going to be merged).

Cheers,

Will

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

[Jason Gunthorpe]

On Fri, Feb 07, 2014 at 11:46:07AM +0000, Will Deacon wrote:

> > In practice, there can be various ways that a system requires something
> > more complex:
> > 
> > * You can have a memory space range that puts PCI bus address zero
> >   at the start of the pci->mem resource. In this case, you have
> >   mem_offset = pci->mem.start. We should probably try not to do
> >   this, but there is existing hardware doing it.
> 
> If it's not the common case, then the generic driver might not need to care
> (at least, initially).

Something to think about, other people are going to reference this
driver when writing drivers for their own hardware, it would be nice
to see it perfect..

AFAIK, the job is fairly simple, when you call pci_add_resource_offset
for memory compute the offset from 
  of_pci_range.pci_addr - of_pci_range.cpu_addr

(or is it the other way around ?)

And when you do it for IO then you compute the offset between the
requested io mapping base to the pci_addr.

> > * You might have multiple sections of the PCI bus space mapped
> >   into CPU physical space. If you want to support legacy VGA
> >   console, you probably want to map the first 16MB of the bus
> >   space at an arbitrary location (with the mem_offset as above),
> >   plus a second, larger section of the bus space with an identity
> >   mapping (mem_offset_= 0) for all devices other than VGA.
> >   You'd also need to copy some VGA specific code from arm32 to
> >   arm64 to actually make this work.
> 
> Again, I'd rather cross that bridge (no pun intended) when we decide we want
> legacy VGA.

Fortuantely if you compute the offset directly from the DT then you
don't need to do anything more. If someone wants to use this
arrangement then they just have to setup the HW and write a proper DT
with two ranges lines for memory and everything should just work.

> Okey doke, is anybody working on that? (I see the follow up from
> Jason, but it's not clear whether that's going to be merged).

Nope, just a thought to stimulate discussion :)

Regards,
Jason

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

[Will Deacon]

On Fri, Feb 07, 2014 at 05:54:10PM +0000, Jason Gunthorpe wrote:
> On Fri, Feb 07, 2014 at 11:46:07AM +0000, Will Deacon wrote:
> 
> > > In practice, there can be various ways that a system requires something
> > > more complex:
> > > 
> > > * You can have a memory space range that puts PCI bus address zero
> > >   at the start of the pci->mem resource. In this case, you have
> > >   mem_offset = pci->mem.start. We should probably try not to do
> > >   this, but there is existing hardware doing it.
> > 
> > If it's not the common case, then the generic driver might not need to care
> > (at least, initially).
> 
> Something to think about, other people are going to reference this
> driver when writing drivers for their own hardware, it would be nice
> to see it perfect..
> 
> AFAIK, the job is fairly simple, when you call pci_add_resource_offset
> for memory compute the offset from 
>   of_pci_range.pci_addr - of_pci_range.cpu_addr
> 
> (or is it the other way around ?)

I think it's the other way round: bus = cpu - offset, then Arnd's example of
PCI bus 0 works out as: 0 = cpu - pci->mem_start.

I added that to my driver, but I get some weird looking bus addresses in
dmesg:

[    0.307585] pci-arm-generic 40000000.pci: PCI host bridge to bus 0000:00
[    0.307601] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[    0.307615] pci_bus 0000:00: root bus resource [mem 0x00000000-0x3effffff] (bus address [0xffffffffbf000000-0xfffffffffdffffff])

Looking at drivers/pci/probe.c, it seems to think that res->start - offset
gives a bus address, which implies that the resources are indeed *CPU*
addresses.

Are you sure pci_add_resource_offset wants bus addresses?

Will

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

[Jason Gunthorpe]

On Wed, Feb 12, 2014 at 06:10:15PM +0000, Will Deacon wrote:

> > AFAIK, the job is fairly simple, when you call pci_add_resource_offset
> > for memory compute the offset from 
> >   of_pci_range.pci_addr - of_pci_range.cpu_addr
> > 
> > (or is it the other way around ?)
> 
> I think it's the other way round: bus = cpu - offset, then Arnd's example of
> PCI bus 0 works out as: 0 = cpu - pci->mem_start.

That looks right to me

> I added that to my driver, but I get some weird looking bus addresses in
> dmesg:
> 
> [    0.307585] pci-arm-generic 40000000.pci: PCI host bridge to bus 0000:00
> [    0.307601] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
> [    0.307615] pci_bus 0000:00: root bus resource [mem 0x00000000-0x3effffff] (bus address [0xffffffffbf000000-0xfffffffffdffffff])
> 
> Looking at drivers/pci/probe.c, it seems to think that res->start - offset
> gives a bus address, which implies that the resources are indeed *CPU*
> addresses.
> 
> Are you sure pci_add_resource_offset wants bus addresses?

Sorry, I wasn't clear: It accepts a cpu address in the struct
resource and an offset to convert back to a bus address.

You should compute 0 as the offset in the normal case, ie
of_pci_range.pci_addr and of_pci_range.cpu_addr should be identical,
which depends on the DT ranges being correct..

Jason

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

[Will Deacon]

On Wed, Feb 12, 2014 at 06:19:13PM +0000, Jason Gunthorpe wrote:
> On Wed, Feb 12, 2014 at 06:10:15PM +0000, Will Deacon wrote:
> 
> > > AFAIK, the job is fairly simple, when you call pci_add_resource_offset
> > > for memory compute the offset from 
> > >   of_pci_range.pci_addr - of_pci_range.cpu_addr
> > > 
> > > (or is it the other way around ?)
> > 
> > I think it's the other way round: bus = cpu - offset, then Arnd's example of
> > PCI bus 0 works out as: 0 = cpu - pci->mem_start.
> 
> That looks right to me
> 
> > I added that to my driver, but I get some weird looking bus addresses in
> > dmesg:
> > 
> > [    0.307585] pci-arm-generic 40000000.pci: PCI host bridge to bus 0000:00
> > [    0.307601] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
> > [    0.307615] pci_bus 0000:00: root bus resource [mem 0x00000000-0x3effffff] (bus address [0xffffffffbf000000-0xfffffffffdffffff])
> > 
> > Looking at drivers/pci/probe.c, it seems to think that res->start - offset
> > gives a bus address, which implies that the resources are indeed *CPU*
> > addresses.
> > 
> > Are you sure pci_add_resource_offset wants bus addresses?
> 
> Sorry, I wasn't clear: It accepts a cpu address in the struct
> resource and an offset to convert back to a bus address.
> 
> You should compute 0 as the offset in the normal case, ie
> of_pci_range.pci_addr and of_pci_range.cpu_addr should be identical,
> which depends on the DT ranges being correct..

Aha! That explains all of the confusion. I'll remove my homebrew
resource translation code then :)

Thanks,

Will

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

[Arnd Bergmann]

On Friday 07 February 2014, Will Deacon wrote:
> On Wed, Feb 05, 2014 at 08:26:17PM +0000, Arnd Bergmann wrote:
> > On Wednesday 05 February 2014 19:09:47 Will Deacon wrote:
> > > On Tue, Feb 04, 2014 at 07:13:49PM +0000, Arnd Bergmann wrote:
> > If you mandate that, your code is actually correct and you do not
> > require an io_offset or mem_offset.
> 
> Right, so that's what I've currently been relying on. If I mandate that,
> will I be making this driver significantly less useful?

After thinking about it some more, I think we should really try to
keep that logic completely out of the host controller driver and instead
make it generic enough to cover all possible cases.

Let's make sure that nobody ever has to call the range parser from
a PCI host driver again, no matter how weird their hardware setup is,
and put the necessary code in a common location. This will make your
driver even simpler.

> > * You might have multiple sections of the PCI bus space mapped
> >   into CPU physical space. If you want to support legacy VGA
> >   console, you probably want to map the first 16MB of the bus
> >   space at an arbitrary location (with the mem_offset as above),
> >   plus a second, larger section of the bus space with an identity
> >   mapping (mem_offset_= 0) for all devices other than VGA.
> >   You'd also need to copy some VGA specific code from arm32 to
> >   arm64 to actually make this work.
> 
> Again, I'd rather cross that bridge (no pun intended) when we decide we want
> legacy VGA.

Fair enough.

> 
> > > > This shows once more that the range parser code is suboptimal. So far
> > > > every single driver got the I/O space wrong here, because the obvious
> > > > way to write this function is also completely wrong.
> > > 
> > > I see you mentioned to Liviu that you should register a logical resource,
> > > rather than physical resource returned from the parser. It seems odd that
> > > I/O space appears to work with this code as-is (I've tested it on arm using
> > > kvmtool by removing the memory bars).
> > 
> > what do you see in /proc/ioports and /proc/iomem then?
> 
> bash-4.2# cat /proc/ioports
> 00006200-000065ff : virtio-pci
> 00006600-000069ff : virtio-pci
> 00006a00-00006dff : virtio-pci
> 00006e00-000071ff : virtio-pci
> 
> bash-4.2# cat /proc/iomem
> 40000000-40ffffff : /pci
> 41000400-410005ff : virtio-pci
> 41000c00-41000dff : virtio-pci
> 41001400-410015ff : virtio-pci
> 41001c00-41001dff : virtio-pci
> 80000000-93ffffff : System RAM
>   80008000-8053df0b : Kernel code
>   80570000-805c07fb : Kernel data

You should normally see a parent resource for the PCI bus and the virtio-pci
resources under that. For some reason, neither of the two appears to have
been registered correctly.

	Arnd

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

[Jason Gunthorpe]

On Sun, Feb 09, 2014 at 09:30:25PM +0100, Arnd Bergmann wrote:

> > bash-4.2# cat /proc/iomem
> > 40000000-40ffffff : /pci
> > 41000400-410005ff : virtio-pci
> > 41000c00-41000dff : virtio-pci
> > 41001400-410015ff : virtio-pci
> > 41001c00-41001dff : virtio-pci
> > 80000000-93ffffff : System RAM
> >   80008000-8053df0b : Kernel code
> >   80570000-805c07fb : Kernel data
> 
> You should normally see a parent resource for the PCI bus and the virtio-pci
> resources under that. For some reason, neither of the two appears to have
> been registered correctly.

I noticed this on mvebu as well..

3.13 w/ mvebu driver:

e0001000-e0001fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/fpga_sysmon@1000
e0006000-e0006fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/qdr2p@6000

3.10 w/ old kirkwood driver:

e0000000-e7ffffff : PCIe 0 MEM
  e0000000-e001ffff : 0000:00:01.0
    e0001000-e0001fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/fpga_sysmon@1000
    e0006000-e0006fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/qdr2p@6000

The latter is obviously correct and matches x86. I'm not sure where
the new style host drivers are going wrong, even the resource that
should be added by the PCI core itself for the BAR is missing..

Jason

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

[Arnd Bergmann]

On Monday 10 February 2014 10:34:50 Jason Gunthorpe wrote:

> I noticed this on mvebu as well..
> 
> 3.13 w/ mvebu driver:
> 
> e0001000-e0001fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/fpga_sysmon@1000
> e0006000-e0006fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/qdr2p@6000
> 
> 3.10 w/ old kirkwood driver:
> 
> e0000000-e7ffffff : PCIe 0 MEM
>   e0000000-e001ffff : 0000:00:01.0
>     e0001000-e0001fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/fpga_sysmon@1000
>     e0006000-e0006fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/qdr2p@6000
> 
> The latter is obviously correct and matches x86. I'm not sure where
> the new style host drivers are going wrong, even the resource that
> should be added by the PCI core itself for the BAR is missing..

I looked briefly at the code and found that mach-kirkwood/pcie.c does
both request_resource() and pci_add_resource_offset(), while
drivers/pci/host/pci-mvebu.c only does the latter. Does the patch
below restore the previous behavior?

Since the mvebu_pcie_setup() function seems very generic at this,
we should probably try to factor out that code into a common
helper, at least for arm64, but ideally shared with arm32
as well.

	Arnd

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13478ec..b55e9a6 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -680,9 +680,17 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
 	struct mvebu_pcie *pcie = sys_to_pcie(sys);
 	int i;
 
-	if (resource_size(&pcie->realio) != 0)
+	if (request_resource(&iomem_resource, &pcie->mem))
+		return 0;
+
+	if (resource_size(&pcie->realio) != 0) {
+		if (request_resource(&ioport_resource, &pcie->realio)) {
+			release_resource(&pcie->mem);
+			return 0;
+		}
 		pci_add_resource_offset(&sys->resources, &pcie->realio,
 					sys->io_offset);
+	}
 	pci_add_resource_offset(&sys->resources, &pcie->mem, sys->mem_offset);
 	pci_add_resource(&sys->resources, &pcie->busn);
 

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

[Jason Gunthorpe]

On Tue, Feb 11, 2014 at 11:42:52AM +0100, Arnd Bergmann wrote:
> I looked briefly at the code and found that mach-kirkwood/pcie.c does
> both request_resource() and pci_add_resource_offset(), while
> drivers/pci/host/pci-mvebu.c only does the latter. Does the patch
> below restore the previous behavior?

It gets closer:

e0000000-f0000000 : <BAD>
  e0000000-e00fffff : PCI Bus 0000:01
    e0000000-e001ffff : 0000:01:00.0
      e0001000-e0001fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/fpga_sysmon@1000

This patch:

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 2394e97..7fd54e9 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -876,14 +876,14 @@ static void __init mvebu_mbus_get_pcie_resources(struct device_node *np,
        ret = of_property_read_u32_array(np, "pcie-mem-aperture", reg, ARRAY_SIZE(reg));
        if (!ret) {
                mem->start = reg[0];
-               mem->end = mem->start + reg[1];
+               mem->end = mem->start + reg[1] - 1;
                mem->flags = IORESOURCE_MEM;
        }
 
        ret = of_property_read_u32_array(np, "pcie-io-aperture", reg, ARRAY_SIZE(reg));
        if (!ret) {
                io->start = reg[0];
-               io->end = io->start + reg[1];
+               io->end = io->start + reg[1] - 1;
                io->flags = IORESOURCE_IO;
        }
 }

Fixes the wrong length (e0000000-f0000000 should be e0000000-efffffff)

And this fixes the <BAD>:

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index ef8691a..fbb89cb 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -109,7 +109,9 @@ struct mvebu_pcie {
        struct mvebu_pcie_port *ports;
        struct msi_chip *msi;
        struct resource io;
+       char io_name[30];
        struct resource realio;
+       char mem_name[30];
        struct resource mem;
        struct resource busn;
        int nports;
@@ -681,10 +683,29 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
 {
        struct mvebu_pcie *pcie = sys_to_pcie(sys);
        int i;
+       int domain = 0;
 
+#ifdef CONFIG_PCI_DOMAINS
+       domain = sys->domain;
+#endif
+
+       snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", domain);
+       pcie->mem.name = pcie->mem_name;
+
+       snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
+       pcie->realio.name = pcie->io_name;


Still missing release_region..

Thoughts on upstreamining these bits?

> Since the mvebu_pcie_setup() function seems very generic at this,
> we should probably try to factor out that code into a common
> helper, at least for arm64, but ideally shared with arm32
> as well.

Yah, especially since people are not getting it completely right..

But some of the trouble here is a lack of a generic pci host driver
structure, eg I have to pull the domain number out of the ARM32
specific structure ..

Jason

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

[Arnd Bergmann]

On Wednesday 12 February 2014 12:43:13 Jason Gunthorpe wrote:
> On Tue, Feb 11, 2014 at 11:42:52AM +0100, Arnd Bergmann wrote:
> 
> 
> Still missing release_region..
> 
> Thoughts on upstreamining these bits?

Upstreaming them would be great. Patches look correct by inspection as well.

> > Since the mvebu_pcie_setup() function seems very generic at this,
> > we should probably try to factor out that code into a common
> > helper, at least for arm64, but ideally shared with arm32
> > as well.
> 
> Yah, especially since people are not getting it completely right..
> 
> But some of the trouble here is a lack of a generic pci host driver
> structure, eg I have to pull the domain number out of the ARM32
> specific structure ..

I'm sure that Bjorn also has some plans of his own, but I think
we should have a generic host driver structure and gradually move
stuff into it from the architecture specific structs.

Now there is a 'struct pci_host_bridge' that is used on some
architectures already but not on ARM. It also contains a list
of resource windows plus offsets, and there is a set of functions
associated with it:

pci_create_root_bus()
pcibios_root_bridge_prepare()

etc.

Maybe all that's needed is to actually start using those on arm32?

	Arnd

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

[Bjorn Helgaas]

On Wed, Feb 12, 2014 at 1:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 12 February 2014 12:43:13 Jason Gunthorpe wrote:
>> On Tue, Feb 11, 2014 at 11:42:52AM +0100, Arnd Bergmann wrote:
>>
>>
>> Still missing release_region..
>>
>> Thoughts on upstreamining these bits?
>
> Upstreaming them would be great. Patches look correct by inspection as well.
>
>> > Since the mvebu_pcie_setup() function seems very generic at this,
>> > we should probably try to factor out that code into a common
>> > helper, at least for arm64, but ideally shared with arm32
>> > as well.
>>
>> Yah, especially since people are not getting it completely right..
>>
>> But some of the trouble here is a lack of a generic pci host driver
>> structure, eg I have to pull the domain number out of the ARM32
>> specific structure ..
>
> I'm sure that Bjorn also has some plans of his own, but I think
> we should have a generic host driver structure and gradually move
> stuff into it from the architecture specific structs.

I don't have plans in this area right now, but I would definitely like
to migrate things like the domain, NUMA node, etc., into a generic
structure.

> Now there is a 'struct pci_host_bridge' that is used on some
> architectures already but not on ARM. It also contains a list
> of resource windows plus offsets, and there is a set of functions
> associated with it:
>
> pci_create_root_bus()
> pcibios_root_bridge_prepare()
>
> etc.
>
> Maybe all that's needed is to actually start using those on arm32?
>
>         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 2/3] PCI: ARM: add support for virtual PCI host controller

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Wed, 12 Feb 2014 12:43:13 -0700, Jason Gunthorpe wrote:

> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index 2394e97..7fd54e9 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -876,14 +876,14 @@ static void __init mvebu_mbus_get_pcie_resources(struct device_node *np,
>         ret = of_property_read_u32_array(np, "pcie-mem-aperture", reg, ARRAY_SIZE(reg));
>         if (!ret) {
>                 mem->start = reg[0];
> -               mem->end = mem->start + reg[1];
> +               mem->end = mem->start + reg[1] - 1;
>                 mem->flags = IORESOURCE_MEM;
>         }
>  
>         ret = of_property_read_u32_array(np, "pcie-io-aperture", reg, ARRAY_SIZE(reg));
>         if (!ret) {
>                 io->start = reg[0];
> -               io->end = io->start + reg[1];
> +               io->end = io->start + reg[1] - 1;
>                 io->flags = IORESOURCE_IO;
>         }
>  }

This certainly looks good.

> Fixes the wrong length (e0000000-f0000000 should be e0000000-efffffff)
> 
> And this fixes the <BAD>:
> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index ef8691a..fbb89cb 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -109,7 +109,9 @@ struct mvebu_pcie {
>         struct mvebu_pcie_port *ports;
>         struct msi_chip *msi;
>         struct resource io;
> +       char io_name[30];
>         struct resource realio;
> +       char mem_name[30];
>         struct resource mem;
>         struct resource busn;
>         int nports;
> @@ -681,10 +683,29 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
>  {
>         struct mvebu_pcie *pcie = sys_to_pcie(sys);
>         int i;
> +       int domain = 0;
>  
> +#ifdef CONFIG_PCI_DOMAINS
> +       domain = sys->domain;
> +#endif
> +
> +       snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", domain);
> +       pcie->mem.name = pcie->mem_name;
> +
> +       snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
> +       pcie->realio.name = pcie->io_name;

I honestly don't know what PCI domains are, but this change looks
fairly harmless to me. I would however use kasprintf() instead maybe. I
can submit patches for those two changes if you want.

> Still missing release_region..

To match which request_region?

> > Since the mvebu_pcie_setup() function seems very generic at this,
> > we should probably try to factor out that code into a common
> > helper, at least for arm64, but ideally shared with arm32
> > as well.
> 
> Yah, especially since people are not getting it completely right..
> 
> But some of the trouble here is a lack of a generic pci host driver
> structure, eg I have to pull the domain number out of the ARM32
> specific structure ..

There is indeed a good deal of duplication of PCI related code in the
different architectures. The PCI_DOMAINS Kconfig option is for example
replicated in multiple architectures.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Jason Gunthorpe]

On Wed, Feb 12, 2014 at 10:15:26PM +0100, Thomas Petazzoni wrote:

> > +#ifdef CONFIG_PCI_DOMAINS
> > +       domain = sys->domain;
> > +#endif
> > +
> > +       snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", domain);
> > +       pcie->mem.name = pcie->mem_name;
> > +
> > +       snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
> > +       pcie->realio.name = pcie->io_name;
> 
> I honestly don't know what PCI domains are, but this change looks
> fairly harmless to me. I would however use kasprintf() instead maybe. I
> can submit patches for those two changes if you want.

For this purpose imagine that each PCI host controller driver gets a
unique domain, and every domain has a unique
aperture. domain:bus:device:function is the full unique path to a PCI
device.

So when you look at the resource hierarchy it makes some logical
sense:

e0000000-efffffff : PCI MEM 0000
  e0000000-e00fffff : PCI Bus 0000:01
    e0000000-e001ffff : 0000:01:00.0

PCI Domain -> domain + subordinate bus # -> domain:bus:device:function, bar #

The ARM PCI BIOS code used bus number, and Will's version ended up
using the DT path to the PCI node. Would be nice to see an
agreement/common code :)
 
> > Still missing release_region..
> 
> To match which request_region?

Ah, sorry, Arnd added one:

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13478ec..b55e9a6 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -680,9 +680,17 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
        struct mvebu_pcie *pcie = sys_to_pcie(sys);
        int i;

-       if (resource_size(&pcie->realio) != 0)
+       if (request_resource(&iomem_resource, &pcie->mem))
+               return 0;
+
+       if (resource_size(&pcie->realio) != 0) {
+               if (request_resource(&ioport_resource, &pcie->realio)) {
+                       release_resource(&pcie->mem);
+                       return 0;
+               }
                pci_add_resource_offset(&sys->resources, &pcie->realio,
                                        sys->io_offset);
+       }
        pci_add_resource_offset(&sys->resources, &pcie->mem, sys->mem_offset);
        pci_add_resource(&sys->resources, &pcie->busn);


Which is why I didn't use kasprintf, that would complicate freeing
further.

Ideally we'd be able to use a devm_request_region someday.

Jason

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

[Will Deacon]

On Tue, Feb 11, 2014 at 10:42:52AM +0000, Arnd Bergmann wrote:
> On Monday 10 February 2014 10:34:50 Jason Gunthorpe wrote:
> 
> > I noticed this on mvebu as well..
> > 
> > 3.13 w/ mvebu driver:
> > 
> > e0001000-e0001fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/fpga_sysmon@1000
> > e0006000-e0006fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/qdr2p@6000
> > 
> > 3.10 w/ old kirkwood driver:
> > 
> > e0000000-e7ffffff : PCIe 0 MEM
> >   e0000000-e001ffff : 0000:00:01.0
> >     e0001000-e0001fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/fpga_sysmon@1000
> >     e0006000-e0006fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/qdr2p@6000
> > 
> > The latter is obviously correct and matches x86. I'm not sure where
> > the new style host drivers are going wrong, even the resource that
> > should be added by the PCI core itself for the BAR is missing..
> 
> I looked briefly at the code and found that mach-kirkwood/pcie.c does
> both request_resource() and pci_add_resource_offset(), while
> drivers/pci/host/pci-mvebu.c only does the latter. Does the patch
> below restore the previous behavior?

Making the equivalent changes in my generic driver fixes the issue, cheers
Arnd!

  bash-4.2# cat /proc/ioports
  00000000-0000ffff : /pci
    00006200-000065ff : virtio-pci
    00006600-000069ff : virtio-pci
    00006a00-00006dff : virtio-pci
    00006e00-000071ff : virtio-pci
  bash-4.2# cat /proc/iomem
  41000000-7fffffff : /pci
    41000000-410003ff : virtio-pci
    41000400-410005ff : virtio-pci
    41000800-41000bff : virtio-pci
    41000c00-41000dff : virtio-pci
    41001000-410013ff : virtio-pci
    41001400-410015ff : virtio-pci
    41001800-41001bff : virtio-pci
    41001c00-41001dff : virtio-pci
  80000000-93ffffff : System RAM
    80008000-80697e4f : Kernel code
    806f6000-8077b4c3 : Kernel data

Will

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

[Anup Patel]

On Tue, Feb 4, 2014 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> This patch adds support for an extremely simple virtual PCI host
> controller. The controller itself has no configuration registers, and
> has its address spaces described entirely by the device-tree (using the
> bindings described by ePAPR). This allows emulations, such as kvmtool,
> to provide a simple means for a guest Linux instance to make use of
> PCI devices.
>
> Corresponding documentation is added for the DT binding.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  .../devicetree/bindings/pci/linux,pci-virt.txt     |  38 ++++
>  drivers/pci/host/Kconfig                           |   7 +
>  drivers/pci/host/Makefile                          |   1 +
>  drivers/pci/host/pci-virt.c                        | 200 +++++++++++++++++++++
>  4 files changed, 246 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/linux,pci-virt.txt
>  create mode 100644 drivers/pci/host/pci-virt.c
>
> diff --git a/Documentation/devicetree/bindings/pci/linux,pci-virt.txt b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
> new file mode 100644
> index 000000000000..54668a283498
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
> @@ -0,0 +1,38 @@
> +* ARM Basic Virtual PCI controller
> +
> +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 have a control interface visible to 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 "linux,pci-virt"
> +
> +- ranges         : As described in IEEE Std 1275-1994, but must provide
> +                   at least a definition of the Configuration Space plus
> +                   one or both of IO and Memory Space.
> +
> +- #address-cells : Must be 3
> +
> +- #size-cells    : Must be 2
> +
> +Configuration Space is assumed to be memory-mapped (as opposed to being

It would be great to have a flag to specify whether Configuration Space is over
ioports or memory mapped.

Regards,
Anup

> +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 a 24-bit offset:
> +
> +        cfg_offset(bus, device, function, register) =
> +                bus << 16 | device << 11 | function << 8 | 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..fd4460573b81 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_VIRT_HOST
> +       bool "Virtual PCI host controller"
> +       depends on ARM && OF
> +       help
> +         Say Y here if you want to support a very simple virtual 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..9b6775d95d3b 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_VIRT_HOST) += pci-virt.o
> diff --git a/drivers/pci/host/pci-virt.c b/drivers/pci/host/pci-virt.c
> new file mode 100644
> index 000000000000..ded01474453b
> --- /dev/null
> +++ b/drivers/pci/host/pci-virt.c
> @@ -0,0 +1,200 @@
> +/*
> + * Very basic PCI host controller driver targetting virtual machines
> + * (e.g. the PCI emulation provided by kvmtool).
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (C) 2014 ARM Limited
> + *
> + * Author: Will Deacon <will.deacon@arm.com>
> + *
> + * This driver currently supports (per instance):
> + *     - A single controller
> + *     - A single memory space and/or port space
> + *     - A memory-mapped configuration space
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/platform_device.h>
> +
> +struct virt_pci {
> +       struct device   *dev;
> +
> +       struct resource cfg;
> +       struct resource io;
> +       struct resource mem;
> +
> +       void __iomem    *cfg_base;
> +};
> +
> +static void __iomem *virt_pci_config_address(struct pci_bus *bus,
> +                                            unsigned int devfn,
> +                                            int where)
> +{
> +       struct pci_sys_data *sys = bus->sysdata;
> +       struct virt_pci *pci = sys->private_data;
> +       void __iomem *addr = pci->cfg_base;
> +
> +       /*
> +        * We construct config space addresses by simply sandwiching
> +        * together all of the PCI address components and using the
> +        * result as an offset into a 16M region.
> +        */
> +       return addr + (((u32)bus->number << 16) | (devfn << 8) | where);
> +}
> +
> +
> +static int virt_pci_config_read(struct pci_bus *bus, unsigned int devfn,
> +                               int where, int size, u32 *val)
> +{
> +       void __iomem *addr = virt_pci_config_address(bus, devfn, where);
> +
> +       switch (size) {
> +       case 1:
> +               *val = readb(addr);
> +               break;
> +       case 2:
> +               *val = readw(addr);
> +               break;
> +       default:
> +               *val = readl(addr);
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int virt_pci_config_write(struct pci_bus *bus, unsigned int devfn,
> +                                int where, int size, u32 val)
> +{
> +       void __iomem *addr = virt_pci_config_address(bus, devfn, where);
> +
> +       switch (size) {
> +       case 1:
> +               writeb(val, addr);
> +               break;
> +       case 2:
> +               writew(val, addr);
> +               break;
> +       default:
> +               writel(val, addr);
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static struct pci_ops virt_pci_ops = {
> +       .read   = virt_pci_config_read,
> +       .write  = virt_pci_config_write,
> +};
> +
> +static int virt_pci_setup(int nr, struct pci_sys_data *sys)
> +{
> +       struct virt_pci *pci = sys->private_data;
> +
> +       if (resource_type(&pci->io)) {
> +               pci_add_resource(&sys->resources, &pci->io);
> +               pci_ioremap_io(nr * resource_size(&pci->io), pci->io.start);
> +       }
> +
> +       if (resource_type(&pci->mem))
> +               pci_add_resource(&sys->resources, &pci->mem);
> +
> +       pci->cfg_base = devm_ioremap_resource(pci->dev, &pci->cfg);
> +       return !IS_ERR(pci->cfg_base);
> +}
> +
> +static const struct of_device_id virt_pci_of_match[] = {
> +       { .compatible = "linux,pci-virt" },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, virt_pci_of_match);
> +
> +static int virt_pci_probe(struct platform_device *pdev)
> +{
> +       struct hw_pci hw;
> +       struct of_pci_range range;
> +       struct of_pci_range_parser parser;
> +       struct virt_pci *pci;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +
> +       if (of_pci_range_parser_init(&parser, np)) {
> +               dev_err(dev, "missing \"ranges\" property\n");
> +               return -EINVAL;
> +       }
> +
> +       pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> +       if (!pci)
> +               return -ENOMEM;
> +
> +       pci->dev = dev;
> +       for_each_of_pci_range(&parser, &range) {
> +               u32 restype = range.flags & IORESOURCE_TYPE_BITS;
> +
> +               switch (restype) {
> +               case IORESOURCE_IO:
> +                       if (resource_type(&pci->io))
> +                               dev_warn(dev,
> +                                        "ignoring additional io resource\n");
> +                       else
> +                               of_pci_range_to_resource(&range, np, &pci->io);
> +                       break;
> +               case IORESOURCE_MEM:
> +                       if (resource_type(&pci->mem))
> +                               dev_warn(dev,
> +                                        "ignoring additional mem resource\n");
> +                       else
> +                               of_pci_range_to_resource(&range, np, &pci->mem);
> +                       break;
> +               case 0: /* cfg */
> +                       if (resource_type(&pci->cfg)) {
> +                               dev_warn(dev,
> +                                        "ignoring additional cfg resource\n");
> +                       } else {
> +                               of_pci_range_to_resource(&range, np, &pci->cfg);
> +                               pci->cfg.flags |= IORESOURCE_MEM;
> +                       }
> +                       break;
> +               default:
> +                       dev_warn(dev,
> +                               "ignoring unknown/unsupported resource type %x\n",
> +                                restype);
> +               }
> +       }
> +
> +       memset(&hw, 0, sizeof(hw));
> +       hw.nr_controllers       = 1;
> +       hw.private_data         = (void **)&pci;
> +       hw.setup                = virt_pci_setup;
> +       hw.map_irq              = of_irq_parse_and_map_pci;
> +       hw.ops                  = &virt_pci_ops;
> +       pci_common_init_dev(dev, &hw);
> +       return 0;
> +}
> +
> +static struct platform_driver virt_pci_driver = {
> +       .driver = {
> +               .name = "pci-virt",
> +               .owner = THIS_MODULE,
> +               .of_match_table = virt_pci_of_match,
> +       },
> +       .probe = virt_pci_probe,
> +};
> +module_platform_driver(virt_pci_driver);
> +
> +MODULE_DESCRIPTION("Virtual PCI host driver");
> +MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
> +MODULE_LICENSE("GPLv2");
> --
> 1.8.2.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arnd Bergmann]

On Thursday 06 February 2014 14:24:03 Anup Patel wrote:
> > diff --git a/Documentation/devicetree/bindings/pci/linux,pci-virt.txt b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
> > new file mode 100644
> > index 000000000000..54668a283498
> > +- compatible     : Must be "linux,pci-virt"
> > +
> > +- ranges         : As described in IEEE Std 1275-1994, but must provide
> > +                   at least a definition of the Configuration Space plus
> > +                   one or both of IO and Memory Space.
> > +
> > +- #address-cells : Must be 3
> > +
> > +- #size-cells    : Must be 2
> > +
> > +Configuration Space is assumed to be memory-mapped (as opposed to being
> 
> It would be great to have a flag to specify whether Configuration Space is over
> ioports or memory mapped.
> 

I haven't seen config space access through ioport for a long
time on non-x86. Do you have reason to believe that we will need
that?

	Arnd

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

[Anup Patel]

On Thu, Feb 6, 2014 at 3:56 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 06 February 2014 14:24:03 Anup Patel wrote:
>> > diff --git a/Documentation/devicetree/bindings/pci/linux,pci-virt.txt b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
>> > new file mode 100644
>> > index 000000000000..54668a283498
>> > +- compatible     : Must be "linux,pci-virt"
>> > +
>> > +- ranges         : As described in IEEE Std 1275-1994, but must provide
>> > +                   at least a definition of the Configuration Space plus
>> > +                   one or both of IO and Memory Space.
>> > +
>> > +- #address-cells : Must be 3
>> > +
>> > +- #size-cells    : Must be 2
>> > +
>> > +Configuration Space is assumed to be memory-mapped (as opposed to being
>>
>> It would be great to have a flag to specify whether Configuration Space is over
>> ioports or memory mapped.
>>
>
> I haven't seen config space access through ioport for a long
> time on non-x86. Do you have reason to believe that we will need
> that?

I suggested it to make "pci-virt" useful for x86-world too.

>
>         Arnd

--
Anup

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

[Liviu Dudau]

On Thu, Feb 06, 2014 at 08:54:03AM +0000, Anup Patel wrote:
> On Tue, Feb 4, 2014 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> > This patch adds support for an extremely simple virtual PCI host
> > controller. The controller itself has no configuration registers, and
> > has its address spaces described entirely by the device-tree (using the
> > bindings described by ePAPR). This allows emulations, such as kvmtool,
> > to provide a simple means for a guest Linux instance to make use of
> > PCI devices.
> >
> > Corresponding documentation is added for the DT binding.
> >
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  .../devicetree/bindings/pci/linux,pci-virt.txt     |  38 ++++
> >  drivers/pci/host/Kconfig                           |   7 +
> >  drivers/pci/host/Makefile                          |   1 +
> >  drivers/pci/host/pci-virt.c                        | 200 +++++++++++++++++++++
> >  4 files changed, 246 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/linux,pci-virt.txt
> >  create mode 100644 drivers/pci/host/pci-virt.c
> >
> > diff --git a/Documentation/devicetree/bindings/pci/linux,pci-virt.txt b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
> > new file mode 100644
> > index 000000000000..54668a283498
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
> > @@ -0,0 +1,38 @@
> > +* ARM Basic Virtual PCI controller
> > +
> > +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 have a control interface visible to 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 "linux,pci-virt"
> > +
> > +- ranges         : As described in IEEE Std 1275-1994, but must provide
> > +                   at least a definition of the Configuration Space plus
> > +                   one or both of IO and Memory Space.
> > +
> > +- #address-cells : Must be 3
> > +
> > +- #size-cells    : Must be 2
> > +
> > +Configuration Space is assumed to be memory-mapped (as opposed to being
> 
> It would be great to have a flag to specify whether Configuration Space is over
> ioports or memory mapped.

This is another reason why I prefer the reg property for specifying the configuration
space address range. I don't see a straight way of making the distinction you
need using the ranges property.

> 
> Regards,
> Anup
> 
> > +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 a 24-bit offset:
> > +
> > +        cfg_offset(bus, device, function, register) =
> > +                bus << 16 | device << 11 | function << 8 | 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..fd4460573b81 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_VIRT_HOST
> > +       bool "Virtual PCI host controller"
> > +       depends on ARM && OF
> > +       help
> > +         Say Y here if you want to support a very simple virtual 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..9b6775d95d3b 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_VIRT_HOST) += pci-virt.o
> > diff --git a/drivers/pci/host/pci-virt.c b/drivers/pci/host/pci-virt.c
> > new file mode 100644
> > index 000000000000..ded01474453b
> > --- /dev/null
> > +++ b/drivers/pci/host/pci-virt.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * Very basic PCI host controller driver targetting virtual machines
> > + * (e.g. the PCI emulation provided by kvmtool).
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + * Copyright (C) 2014 ARM Limited
> > + *
> > + * Author: Will Deacon <will.deacon@arm.com>
> > + *
> > + * This driver currently supports (per instance):
> > + *     - A single controller
> > + *     - A single memory space and/or port space
> > + *     - A memory-mapped configuration space
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/platform_device.h>
> > +
> > +struct virt_pci {
> > +       struct device   *dev;
> > +
> > +       struct resource cfg;
> > +       struct resource io;
> > +       struct resource mem;
> > +
> > +       void __iomem    *cfg_base;
> > +};
> > +
> > +static void __iomem *virt_pci_config_address(struct pci_bus *bus,
> > +                                            unsigned int devfn,
> > +                                            int where)
> > +{
> > +       struct pci_sys_data *sys = bus->sysdata;
> > +       struct virt_pci *pci = sys->private_data;
> > +       void __iomem *addr = pci->cfg_base;
> > +
> > +       /*
> > +        * We construct config space addresses by simply sandwiching
> > +        * together all of the PCI address components and using the
> > +        * result as an offset into a 16M region.
> > +        */
> > +       return addr + (((u32)bus->number << 16) | (devfn << 8) | where);
> > +}
> > +
> > +
> > +static int virt_pci_config_read(struct pci_bus *bus, unsigned int devfn,
> > +                               int where, int size, u32 *val)
> > +{
> > +       void __iomem *addr = virt_pci_config_address(bus, devfn, where);
> > +
> > +       switch (size) {
> > +       case 1:
> > +               *val = readb(addr);
> > +               break;
> > +       case 2:
> > +               *val = readw(addr);
> > +               break;
> > +       default:
> > +               *val = readl(addr);
> > +       }
> > +
> > +       return PCIBIOS_SUCCESSFUL;
> > +}
> > +
> > +static int virt_pci_config_write(struct pci_bus *bus, unsigned int devfn,
> > +                                int where, int size, u32 val)
> > +{
> > +       void __iomem *addr = virt_pci_config_address(bus, devfn, where);
> > +
> > +       switch (size) {
> > +       case 1:
> > +               writeb(val, addr);
> > +               break;
> > +       case 2:
> > +               writew(val, addr);
> > +               break;
> > +       default:
> > +               writel(val, addr);
> > +       }
> > +
> > +       return PCIBIOS_SUCCESSFUL;
> > +}
> > +
> > +static struct pci_ops virt_pci_ops = {
> > +       .read   = virt_pci_config_read,
> > +       .write  = virt_pci_config_write,
> > +};
> > +
> > +static int virt_pci_setup(int nr, struct pci_sys_data *sys)
> > +{
> > +       struct virt_pci *pci = sys->private_data;
> > +
> > +       if (resource_type(&pci->io)) {
> > +               pci_add_resource(&sys->resources, &pci->io);
> > +               pci_ioremap_io(nr * resource_size(&pci->io), pci->io.start);
> > +       }
> > +
> > +       if (resource_type(&pci->mem))
> > +               pci_add_resource(&sys->resources, &pci->mem);
> > +
> > +       pci->cfg_base = devm_ioremap_resource(pci->dev, &pci->cfg);
> > +       return !IS_ERR(pci->cfg_base);
> > +}
> > +
> > +static const struct of_device_id virt_pci_of_match[] = {
> > +       { .compatible = "linux,pci-virt" },
> > +       { },
> > +};
> > +MODULE_DEVICE_TABLE(of, virt_pci_of_match);
> > +
> > +static int virt_pci_probe(struct platform_device *pdev)
> > +{
> > +       struct hw_pci hw;
> > +       struct of_pci_range range;
> > +       struct of_pci_range_parser parser;
> > +       struct virt_pci *pci;
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *np = dev->of_node;
> > +
> > +       if (of_pci_range_parser_init(&parser, np)) {
> > +               dev_err(dev, "missing \"ranges\" property\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > +       if (!pci)
> > +               return -ENOMEM;
> > +
> > +       pci->dev = dev;
> > +       for_each_of_pci_range(&parser, &range) {
> > +               u32 restype = range.flags & IORESOURCE_TYPE_BITS;
> > +
> > +               switch (restype) {
> > +               case IORESOURCE_IO:
> > +                       if (resource_type(&pci->io))
> > +                               dev_warn(dev,
> > +                                        "ignoring additional io resource\n");
> > +                       else
> > +                               of_pci_range_to_resource(&range, np, &pci->io);
> > +                       break;
> > +               case IORESOURCE_MEM:
> > +                       if (resource_type(&pci->mem))
> > +                               dev_warn(dev,
> > +                                        "ignoring additional mem resource\n");
> > +                       else
> > +                               of_pci_range_to_resource(&range, np, &pci->mem);
> > +                       break;
> > +               case 0: /* cfg */
> > +                       if (resource_type(&pci->cfg)) {
> > +                               dev_warn(dev,
> > +                                        "ignoring additional cfg resource\n");
> > +                       } else {
> > +                               of_pci_range_to_resource(&range, np, &pci->cfg);
> > +                               pci->cfg.flags |= IORESOURCE_MEM;
> > +                       }
> > +                       break;
> > +               default:
> > +                       dev_warn(dev,
> > +                               "ignoring unknown/unsupported resource type %x\n",
> > +                                restype);
> > +               }
> > +       }
> > +
> > +       memset(&hw, 0, sizeof(hw));
> > +       hw.nr_controllers       = 1;
> > +       hw.private_data         = (void **)&pci;
> > +       hw.setup                = virt_pci_setup;
> > +       hw.map_irq              = of_irq_parse_and_map_pci;
> > +       hw.ops                  = &virt_pci_ops;
> > +       pci_common_init_dev(dev, &hw);
> > +       return 0;
> > +}
> > +
> > +static struct platform_driver virt_pci_driver = {
> > +       .driver = {
> > +               .name = "pci-virt",
> > +               .owner = THIS_MODULE,
> > +               .of_match_table = virt_pci_of_match,
> > +       },
> > +       .probe = virt_pci_probe,
> > +};
> > +module_platform_driver(virt_pci_driver);
> > +
> > +MODULE_DESCRIPTION("Virtual PCI host driver");
> > +MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
> > +MODULE_LICENSE("GPLv2");
> > --
> > 1.8.2.2
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Will Deacon]

On Thu, Feb 06, 2014 at 10:54:42AM +0000, Liviu Dudau wrote:
> On Thu, Feb 06, 2014 at 08:54:03AM +0000, Anup Patel wrote:
> > On Tue, Feb 4, 2014 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > This patch adds support for an extremely simple virtual PCI host
> > > controller. The controller itself has no configuration registers, and
> > > has its address spaces described entirely by the device-tree (using the
> > > bindings described by ePAPR). This allows emulations, such as kvmtool,
> > > to provide a simple means for a guest Linux instance to make use of
> > > PCI devices.
> > >
> > > Corresponding documentation is added for the DT binding.
> > >
> > > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > > ---
> > >  .../devicetree/bindings/pci/linux,pci-virt.txt     |  38 ++++
> > >  drivers/pci/host/Kconfig                           |   7 +
> > >  drivers/pci/host/Makefile                          |   1 +
> > >  drivers/pci/host/pci-virt.c                        | 200 +++++++++++++++++++++
> > >  4 files changed, 246 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pci/linux,pci-virt.txt
> > >  create mode 100644 drivers/pci/host/pci-virt.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/linux,pci-virt.txt b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
> > > new file mode 100644
> > > index 000000000000..54668a283498
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/linux,pci-virt.txt
> > > @@ -0,0 +1,38 @@
> > > +* ARM Basic Virtual PCI controller
> > > +
> > > +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 have a control interface visible to 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 "linux,pci-virt"
> > > +
> > > +- ranges         : As described in IEEE Std 1275-1994, but must provide
> > > +                   at least a definition of the Configuration Space plus
> > > +                   one or both of IO and Memory Space.
> > > +
> > > +- #address-cells : Must be 3
> > > +
> > > +- #size-cells    : Must be 2
> > > +
> > > +Configuration Space is assumed to be memory-mapped (as opposed to being
> > 
> > It would be great to have a flag to specify whether Configuration Space is over
> > ioports or memory mapped.
> 
> This is another reason why I prefer the reg property for specifying the configuration
> space address range. I don't see a straight way of making the distinction you
> need using the ranges property.

Well if we need to distinguish cam vs ecam, then adding ioport to the mix
should be (conceptually) easy and I don't think it would involve the "reg"
property.

However, I'm not planning to add ioport support myself.

Will

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

[Arnd Bergmann]

On Thursday 06 February 2014 11:00:16 Will Deacon wrote:
> On Thu, Feb 06, 2014 at 10:54:42AM +0000, Liviu Dudau wrote:
> > On Thu, Feb 06, 2014 at 08:54:03AM +0000, Anup Patel wrote:
> > > On Tue, Feb 4, 2014 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> > 
> > This is another reason why I prefer the reg property for specifying the configuration
> > space address range. I don't see a straight way of making the distinction you
> > need using the ranges property.
> 
> Well if we need to distinguish cam vs ecam, then adding ioport to the mix
> should be (conceptually) easy and I don't think it would involve the "reg"
> property.
> 
> However, I'm not planning to add ioport support myself.

Maybe it's better to have separate compatible strings for these cases,
can call it "pci-ecam-generic" or "pci-cam-generic" to have an easy
distinction. Or we just use the "reg-names" property so the device
can have a "cam" register or an "ecam" register or both.

I don't think we can make the driver generic enough for
x86 as Anup suggests though, since x86 has its own set of quirks to
deal with the various PCI config space access methods.

	Arnd

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

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

[Bjorn Helgaas]

On Tue, Feb 04, 2014 at 04:53:01PM +0000, Will Deacon wrote:
> Hello,
> 
> This small set of patches brings PCI support to mach-virt based upon an
> idealised host controller (see patch 2 for more details).
> 
> This has been tested with kvmtool, for which I have a corresponding set
> of patches which you can find in my kvmtool/pci branch at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git
> 
> Once the arm64 PCI patches from Liviu have stabilised, I plan to port
> this host controller to work there as well.
> 
> The main issue I can see with this code is how to describe configuration
> space in the device-tree. I'm following the ePAPR PCI bindings (SS == 0)
> , but this adds an ugly 'case 0:' line in the pci range parser, which
> also exists in mainline for the pcie-designware.c driver.

There was a long discussion about this (especially 2/3), and I didn't
follow it closely enough to get the resolution, and I don't think I
picked up the patches.

Can you post them again if they're still applicable?

Thanks,
  Bjorn

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

[Will Deacon]

Hi Bjorn,

(apologies for the delay, I've been away for a month)

On Thu, Apr 03, 2014 at 11:07:13PM +0100, Bjorn Helgaas wrote:
> On Tue, Feb 04, 2014 at 04:53:01PM +0000, Will Deacon wrote:
> > Hello,
> > 
> > This small set of patches brings PCI support to mach-virt based upon an
> > idealised host controller (see patch 2 for more details).
> > 
> > This has been tested with kvmtool, for which I have a corresponding set
> > of patches which you can find in my kvmtool/pci branch at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git
> > 
> > Once the arm64 PCI patches from Liviu have stabilised, I plan to port
> > this host controller to work there as well.
> > 
> > The main issue I can see with this code is how to describe configuration
> > space in the device-tree. I'm following the ePAPR PCI bindings (SS == 0)
> > , but this adds an ugly 'case 0:' line in the pci range parser, which
> > also exists in mainline for the pcie-designware.c driver.
> 
> There was a long discussion about this (especially 2/3), and I didn't
> follow it closely enough to get the resolution, and I don't think I
> picked up the patches.
> 
> Can you post them again if they're still applicable?

I ended up at v5 of the series:

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

which hasn't received any feedback. My plan is to get the driver working
with arm64 once Liviu's series is merged, but I'd be happy to see this
merged for 32-bit ARM before then.

Arnd: Is there anything more you'd like me to do with this series? Given
that it would be targetting 3.16, there's still plenty of time to make
changes (we talked about extracting some library functions in the past).

Will

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

[Arnd Bergmann]

On Monday 14 April 2014 18:13:00 Will Deacon wrote:
> Arnd: Is there anything more you'd like me to do with this series? Given
> that it would be targetting 3.16, there's still plenty of time to make
> changes (we talked about extracting some library functions in the past).

I don't remember the latest status of your patch set, but you might want
to read the discussions we had about Liviu's patches last week.

We still haven't figured out exactly how to handle I/O space in the common
code, but I think we are making progress.

We also have discussed some ideas about how to restructure the PCI
code layer to make it easier to share host drivers across architectures
and clean up the interfaces in the process.

	Arnd

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

[Will Deacon]

Hi Arnd,

On Mon, Apr 14, 2014 at 07:48:17PM +0100, Arnd Bergmann wrote:
> On Monday 14 April 2014 18:13:00 Will Deacon wrote:
> > Arnd: Is there anything more you'd like me to do with this series? Given
> > that it would be targetting 3.16, there's still plenty of time to make
> > changes (we talked about extracting some library functions in the past).
> 
> I don't remember the latest status of your patch set, but you might want
> to read the discussions we had about Liviu's patches last week.

The latest status (v5) was that I had something working (I got hung up on
the I/O offset handing), with all feedback addressed. The next step was to
try and move parts into library code, but it looks like Liviu is handling a
lot of that with his arm64 stuff.

> We still haven't figured out exactly how to handle I/O space in the common
> code, but I think we are making progress.

The discussions certainly seem to be making progress.

> We also have discussed some ideas about how to restructure the PCI
> code layer to make it easier to share host drivers across architectures
> and clean up the interfaces in the process.

Ok. Whilst this all sounds good from an arm64 perspective (with Liviu
currently doing the work), it's not clear to me where that leaves my 32-bit
ARM kvmtool code. That was the main reason for me writing this driver, and it
seems a shame to have to wait for all the generic code to be sorted out
before it can be used on AArch32, where there is already a functional
pcibios implementation. The discussions mention things like generic
pci_host_bridge_ops, but having that for arch/arm/ doesn't sound like
something that is imminent.

Of course, I plan to port my driver to the new infrastructure when it lands
(since I want to support arm64), but it would be good to have something for
AArch32 in the meantime.

Will

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

[Arnd Bergmann]

On Tuesday 15 April 2014 15:47:35 Will Deacon wrote:
> > We also have discussed some ideas about how to restructure the PCI
> > code layer to make it easier to share host drivers across architectures
> > and clean up the interfaces in the process.
> 
> Ok. Whilst this all sounds good from an arm64 perspective (with Liviu
> currently doing the work), it's not clear to me where that leaves my 32-bit
> ARM kvmtool code. That was the main reason for me writing this driver, and it
> seems a shame to have to wait for all the generic code to be sorted out
> before it can be used on AArch32, where there is already a functional
> pcibios implementation. The discussions mention things like generic
> pci_host_bridge_ops, but having that for arch/arm/ doesn't sound like
> something that is imminent.
> 
> Of course, I plan to port my driver to the new infrastructure when it lands
> (since I want to support arm64), but it would be good to have something for
> AArch32 in the meantime.

Agreed. You'll probably have to add a few #ifdef until we have the
infrastructure in place. However, I'd prefer not having to do that for
a lot of other drivers. It's only a matter of time until someone wants
one of the existing arm32 drivers to work on arm64, and we really
shouldn't have to duplicate a lot of #ifdef logic across them, just
to deal with the architectures being different.

	Arnd

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

[Will Deacon]

On Tue, Apr 15, 2014 at 04:26:39PM +0100, Arnd Bergmann wrote:
> On Tuesday 15 April 2014 15:47:35 Will Deacon wrote:
> > > We also have discussed some ideas about how to restructure the PCI
> > > code layer to make it easier to share host drivers across architectures
> > > and clean up the interfaces in the process.
> > 
> > Ok. Whilst this all sounds good from an arm64 perspective (with Liviu
> > currently doing the work), it's not clear to me where that leaves my 32-bit
> > ARM kvmtool code. That was the main reason for me writing this driver, and it
> > seems a shame to have to wait for all the generic code to be sorted out
> > before it can be used on AArch32, where there is already a functional
> > pcibios implementation. The discussions mention things like generic
> > pci_host_bridge_ops, but having that for arch/arm/ doesn't sound like
> > something that is imminent.
> > 
> > Of course, I plan to port my driver to the new infrastructure when it lands
> > (since I want to support arm64), but it would be good to have something for
> > AArch32 in the meantime.
> 
> Agreed. You'll probably have to add a few #ifdef until we have the
> infrastructure in place. However, I'd prefer not having to do that for
> a lot of other drivers. It's only a matter of time until someone wants
> one of the existing arm32 drivers to work on arm64, and we really
> shouldn't have to duplicate a lot of #ifdef logic across them, just
> to deal with the architectures being different.

Ok, cheers Arnd. I'll repost the patches sometime next week.

Will