ARM: PCI: implement virtual PCI host controller

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 <[email protected]>

---
 arch/arm/kernel/bios32.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

arch/arm/kernel/bios32.c +10 -25

index 317da88ae65b..9f3c76638689 100644
@@ -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;
 }
 

Signature · 1 line

1.8.2.2

Quoted 10 lines · 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).

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?

Quoted 58 lines

arch/arm/kernel/bios32.c +10 -25

index 317da88ae65b..9f3c76638689 100644
@@ -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;
 }
 

Signature · 1 line

1.8.2.2

drivers/pci/host/pci-virt.c new file +8 -0

+	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");

Documentation/devicetree/bindings/pci/linux,pci-virt.txt line 18

Quoted 5 lines · On Tuesday 04 February 2014 16:53:03 Will Deacon wrote:

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +5 -0

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

Documentation/devicetree/bindings/pci/linux,pci-virt.txt line 29

Quoted 7 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +7 -0

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

drivers/pci/host/pci-virt.c line 110

Quoted 8 lines

drivers/pci/host/pci-virt.c new file +8 -0

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

drivers/pci/host/pci-virt.c line 113

Quoted 2 lines

drivers/pci/host/pci-virt.c new file +2 -0

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

and also a mem_offset, which is something different.

Quoted 5 lines

drivers/pci/host/pci-virt.c new file +5 -0

+	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[] = {

drivers/pci/host/pci-virt.c line 123

Quoted 4 lines

drivers/pci/host/pci-virt.c new file +4 -0

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

Quoted 14 lines

drivers/pci/host/pci-virt.c new file +14 -0

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

drivers/pci/host/pci-virt.c line 161

Quoted 4 lines

drivers/pci/host/pci-virt.c new file +4 -0

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

drivers/pci/host/pci-virt.c line 184

Quoted 8 lines

drivers/pci/host/pci-virt.c new file +7 -0

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

Hi Arnd,

Thanks for having a look.

Quoted 10 lines · Arnd Bergmann

Documentation/devicetree/bindings/pci/linux,pci-virt.txt line 18

Quoted 5 lines · On Tuesday 04 February 2014 16:53:03 Will Deacon wrote:

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +5 -0

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

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.

Quoted 5 lines

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.

Quoted 3 lines

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

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

Quoted 1 line · Arnd Bergmann

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.

Quoted 11 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt line 29

Quoted 7 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +7 -0

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

Quoted 2 lines

	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.

Quoted 11 lines

drivers/pci/host/pci-virt.c line 110

Quoted 8 lines

drivers/pci/host/pci-virt.c new file +8 -0

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

Quoted 4 lines

drivers/pci/host/pci-virt.c line 113

Quoted 2 lines

drivers/pci/host/pci-virt.c new file +2 -0

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

Quoted 14 lines

Quoted 14 lines

drivers/pci/host/pci-virt.c new file +14 -0

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

Quoted 8 lines

drivers/pci/host/pci-virt.c line 161

Quoted 4 lines

drivers/pci/host/pci-virt.c new file +4 -0

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

Quoted 3 lines

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?

Quoted 3 lines

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 3 lines · Arnd Bergmann

Quoted 3 lines

Quoted 3 lines

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;

?

Quoted 4 lines

Quoted 1 line

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.

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

Quoted 8 lines · Arnd Bergmann

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.

Quoted 9 lines · Russell King - ARM Linux

Quoted 8 lines · Arnd Bergmann

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

...

Quoted 4 lines

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.

Quoted 7 lines · Arnd Bergmann

Quoted 4 lines · Russell King - ARM Linux

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.

Quoted 4 lines

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.

Quoted 11 lines · On Wednesday 05 February 2014 19:09:47 Will Deacon wrote:

Quoted 6 lines · Arnd Bergmann

This should really compute an io_offset.

drivers/pci/host/pci-virt.c line 113

Quoted 2 lines

drivers/pci/host/pci-virt.c new file +2 -0

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

Quoted 8 lines

Quoted 3 lines

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?

Quoted 5 lines

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 8 lines

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

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

Quoted 10 lines · Will Deacon

Quoted 7 lines

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

Quoted 5 lines

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?

Quoted 8 lines · Will Deacon

Quoted 5 lines

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

Quoted 11 lines

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.

Quoted 1 line

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

Quoted 4 lines · Arnd Bergmann

Quoted 1 line · 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?

Quoted 8 lines

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

Quoted 11 lines

Quoted 9 lines

Quoted 4 lines

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

Quoted 9 lines

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 at e0000000/pcie at 1,0/fpga at 0/fpga_sysmon at 1000 e0006000-e0006fff : /mbus/pex at e0000000/pcie at 1,0/fpga at 0/qdr2p at 6000

3.10 w/ old kirkwood driver:

e0000000-e7ffffff : PCIe 0 MEM
e0000000-e001ffff : 0000:00:01.0

    e0001000-e0001fff : /mbus/pex at e0000000/pcie at 1,0/fpga at 0/fpga_sysmon at 1000
    e0006000-e0006fff : /mbus/pex at e0000000/pcie at 1,0/fpga at 0/qdr2p at 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.

On Tue, Feb 11, 2014 at 11:42:52AM +0100, Arnd Bergmann wrote:

Still missing release_region..

Thoughts on upstreamining these bits?

Quoted 4 lines

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

Quoted 6 lines · 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.

Quoted 7 lines

Quoted 4 lines

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

Quoted 3 lines

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

drivers/bus/mvebu-mbus.c +1 -1

index 2394e97..7fd54e9 100644
@@ -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;

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

And this fixes the <BAD>:

drivers/pci/host/pci-mvebu.c +3 -0

index ef8691a..fbb89cb 100644
@@ -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;

Still missing release_region..

Quoted 4 lines

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

Quoted 5 lines

(quoted diff) +5 -0

+#ifdef CONFIG_PCI_DOMAINS
+       domain = sys->domain;
+#endif
+
+       snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", domain);

Review

Quoted 4 lines

(quoted diff) +4 -0

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

Quoted 1 line

Still missing release_region..

To match which request_region?

Quoted 14 lines · 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 at e0000000/pcie at 1,0/fpga at 0/fpga_sysmon at 1000 e0006000-e0006fff : /mbus/pex at e0000000/pcie at 1,0/fpga at 0/qdr2p at 6000

3.10 w/ old kirkwood driver:

e0000000-e7ffffff : PCIe 0 MEM
e0000000-e001ffff : 0000:00:01.0

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

Quoted 3 lines

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?

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 <[email protected]>

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

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +13 -0

new file mode 100644
index 000000000000..54668a283498
@@ -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"

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +15 -0

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

drivers/pci/host/Kconfig +7 -0

index 47d46c6d8468..fd4460573b81 100644
@@ -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

drivers/pci/host/Makefile +1 -0

index 13fb3333aa05..9b6775d95d3b 100644
@@ -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

drivers/pci/host/pci-virt.c new file +200 -1

new file mode 100644
index 000000000000..ded01474453b
@@ -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 <[email protected]>
+ *
+ * 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 <[email protected]>");
+MODULE_LICENSE("GPLv2");
--

1.8.2.2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 9 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +6 -0

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

Documentation/devicetree/bindings/pci/linux,pci-virt.txt line 23

Quoted 5 lines

+- #address-cells : Must be 3

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +4 -0

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

Quoted 9 lines · On Thursday 06 February 2014 14:24:03 Anup Patel wrote:

Quoted 9 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +6 -0

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

Quoted 9 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt line 23

Quoted 5 lines

+- #address-cells : Must be 3

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +4 -0

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

Quoted 43 lines · 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 <[email protected]>

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

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +18 -0

new file mode 100644
index 000000000000..54668a283498
@@ -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.
+

Documentation/devicetree/bindings/pci/linux,pci-virt.txt line 23

Quoted 5 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +5 -0

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

Quoted 250 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +15 -0

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

drivers/pci/host/Kconfig +7 -0

index 47d46c6d8468..fd4460573b81 100644
@@ -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

drivers/pci/host/Makefile +1 -0

index 13fb3333aa05..9b6775d95d3b 100644
@@ -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

drivers/pci/host/pci-virt.c new file +200 -1

new file mode 100644
index 000000000000..ded01474453b
@@ -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 <[email protected]>
+ *
+ * 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 <[email protected]>");
+MODULE_LICENSE("GPLv2");
--

1.8.2.2

Quoted 4 lines

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 44 lines · Anup Patel

Quoted 43 lines · 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 <[email protected]>

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

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +18 -0

new file mode 100644
index 000000000000..54668a283498
@@ -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.
+

Quoted 8 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt line 23

Quoted 5 lines

Documentation/devicetree/bindings/pci/linux,pci-virt.txt new file +5 -0

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

Quoted 6 lines · Liviu Dudau

Quoted 1 line · Anup Patel

On Tue, Feb 4, 2014 at 10:23 PM, Will Deacon <[email protected]> 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.

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.

Quoted 10 lines · 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

Quoted 7 lines

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?

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

Quoted 3 lines · 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.

Quoted 3 lines

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.

Quoted 13 lines · On Tuesday 15 April 2014 15:47:35 Will Deacon wrote:

Quoted 3 lines

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.

Quoted 3 lines

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.