[PATCH 0/3] Versatile PCI DT support

[PATCH 0/3] Versatile PCI DT support

[Rob Herring]

From: Rob Herring <robh@kernel.org>

This series adds a platform driver for Versatile PB's PCI host using
Liviu's recent patch series[1] for DT parsing and setup.

The first patch is a hack to get Liviu's current patches to work on ARM.
It at least shows we are not that far off from being able to use the
series on ARM.

I've tested PCI under QEMU, but need someone with actual h/w to test.
A branch with this series and which includes full conversion of
Versatile to DT is available here[2].

Rob

[1] https://lkml.org/lkml/2014/3/14/279
[2] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git versatile

Rob Herring (3):
  ARM: hackup pcibios support for commmon bridge code
  dt/bindings: add versatile PCI binding
  pci: add DT based ARM Versatile PCI host driver

 .../devicetree/bindings/pci/versatile.txt          |  59 +++++
 arch/arm/kernel/bios32.c                           |  63 +++++
 drivers/pci/host/Kconfig                           |   4 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-versatile.c                   | 275 +++++++++++++++++++++
 5 files changed, 402 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/versatile.txt
 create mode 100644 drivers/pci/host/pci-versatile.c

-- 
1.8.3.2

[PATCH 1/3] ARM: hackup pcibios support for commmon bridge code

[Rob Herring]

From: Rob Herring <robh@kernel.org>

Signed-off-by: Rob Herring <robh@kernel.org>
---
 arch/arm/kernel/bios32.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 317da88..31ec14a 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/of_pci.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/map.h>
@@ -337,6 +338,15 @@ void pcibios_fixup_bus(struct pci_bus *bus)
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		u16 cmd;
 
+		/* Ignore fully discovered devices */
+		if (dev->is_added)
+			continue;
+
+		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+		/* Read default IRQs and fixup if necessary */
+		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+
 		pci_read_config_word(dev, PCI_COMMAND, &cmd);
 		cmd |= features;
 		pci_write_config_word(dev, PCI_COMMAND, cmd);
@@ -681,3 +691,56 @@ void __init pci_map_io_early(unsigned long pfn)
 	pci_io_desc.pfn = pfn;
 	iotable_init(&pci_io_desc, 1);
 }
+
+struct ioresource {
+	struct list_head list;
+	phys_addr_t start;
+	resource_size_t size;
+};
+
+static LIST_HEAD(io_list);
+
+int pci_register_io_range(phys_addr_t address, resource_size_t size)
+{
+	struct ioresource *res;
+	resource_size_t allocated_size = 0;
+
+	/* find if the range has not been already allocated */
+	list_for_each_entry(res, &io_list, list) {
+		if (address >= res->start &&
+			address + size <= res->start + size)
+			return 0;
+		allocated_size += res->size;
+	}
+
+	/* range not already registered, check for space */
+	if (allocated_size + size > IO_SPACE_LIMIT)
+		return -E2BIG;
+
+	/* add the range in the list */
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return -ENOMEM;
+	res->start = address;
+	res->size = size;
+
+	list_add_tail(&res->list, &io_list);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_register_io_range);
+
+unsigned long pci_address_to_pio(phys_addr_t address)
+{
+	struct ioresource *res;
+
+	list_for_each_entry(res, &io_list, list) {
+		if (address >= res->start &&
+			address < res->start + res->size) {
+			return res->start - address;
+		}
+	}
+
+	return (unsigned long)-1;
+}
+EXPORT_SYMBOL_GPL(pci_address_to_pio);
-- 
1.8.3.2

[PATCH 2/3] dt/bindings: add versatile PCI binding

[Rob Herring]

From: Rob Herring <robh@kernel.org>

Add binding documentation for the PCI controller found on Versatile PB
boards.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/pci/versatile.txt          | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/versatile.txt

diff --git a/Documentation/devicetree/bindings/pci/versatile.txt b/Documentation/devicetree/bindings/pci/versatile.txt
new file mode 100644
index 0000000..2cc6071
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/versatile.txt
@@ -0,0 +1,59 @@
+* ARM Versatile Platform Baseboard PCI interface
+
+PCI host controller found on the ARM Versatile PB board's FPGA.
+
+Required properties:
+- compatible: should contain "arm,versatile-pci" to identify the Versatile PCI
+  controller.
+- reg: base addresses and lengths of the pci controller. There must be 3
+  entries:
+	- Versatile specific registers
+	- Self Config space
+	- Config space
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- bus-range: set to <0 0xff>
+- ranges: ranges for the PCI memory and I/O regions
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties	to define
+	the mapping of the PCI interface to interrupt numbers.
+
+Example:
+
+pci-controller@10001000 {
+	compatible = "arm,versatile-pci";
+	device_type = "pci";
+	reg = <0x10001000 0x1000
+	       0x41000000 0x10000
+	       0x42000000 0x100000>;
+	bus-range = <0 0xff>;
+	#address-cells = <3>;
+	#size-cells = <2>;
+	#interrupt-cells = <1>;
+
+	ranges = <0x01000000 0 0x00000000 0x43000000 0 0x00010000   /* downstream I/O */
+		  0x02000000 0 0x50000000 0x50000000 0 0x10000000   /* non-prefetchable memory */
+		  0x42000000 0 0x60000000 0x60000000 0 0x10000000>; /* prefetchable memory */
+
+	interrupt-map-mask = <0x1800 0 0 7>;
+	interrupt-map = <0x1800 0 0 1 &sic 28
+			 0x1800 0 0 2 &sic 29
+			 0x1800 0 0 3 &sic 30
+			 0x1800 0 0 4 &sic 27
+
+			 0x1000 0 0 1 &sic 27
+			 0x1000 0 0 2 &sic 28
+			 0x1000 0 0 3 &sic 29
+			 0x1000 0 0 4 &sic 30
+
+			 0x0800 0 0 1 &sic 30
+			 0x0800 0 0 2 &sic 27
+			 0x0800 0 0 3 &sic 28
+			 0x0800 0 0 4 &sic 29
+
+			 0x0000 0 0 1 &sic 29
+			 0x0000 0 0 2 &sic 30
+			 0x0000 0 0 3 &sic 27
+			 0x0000 0 0 4 &sic 28>;
+};
-- 
1.8.3.2

[PATCH 3/3] pci: add DT based ARM Versatile PCI host driver

[Rob Herring]

From: Rob Herring <robh@kernel.org>

This converts the Versatile PCI host code to a platform driver using
of_create_pci_host_bridge for parsing DT and setup.

I think more of this setup could be done by the core code. There are
accesses to the host's config space (accesses using local_pci_cfg_base)
which seem like they could be done by the core code or using standard
config space accessors. The problem is bridge->bus->self is needed, but
it does not get setup. I'm not exactly sure how that should work.

Signed-off-by: Rob Herring <robh@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/Kconfig         |   4 +
 drivers/pci/host/Makefile        |   1 +
 drivers/pci/host/pci-versatile.c | 275 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 280 insertions(+)
 create mode 100644 drivers/pci/host/pci-versatile.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 47d46c6..b4dd911 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -33,4 +33,8 @@ 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_VERSATILE
+	bool "ARM Versatile PB PCI controller"
+	depends on ARCH_VERSATILE
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 13fb333..fe67ab3 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_VERSATILE) += pci-versatile.o
diff --git a/drivers/pci/host/pci-versatile.c b/drivers/pci/host/pci-versatile.c
new file mode 100644
index 0000000..98abd1f
--- /dev/null
+++ b/drivers/pci/host/pci-versatile.c
@@ -0,0 +1,275 @@
+/*
+ * Copyright 2004 Koninklijke Philips Electronics NV
+ *
+ * Conversion to platform driver and DT:
+ * Copyright 2014 Linaro Ltd.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+ * 14/04/2005 Initial version, colin.king@philips.com
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+static void __iomem *versatile_pci_base;
+static void __iomem *versatile_cfg_base[2];
+
+#define PCI_IMAP(m)		(versatile_pci_base + ((m) * 4))
+#define PCI_SMAP(m)		(versatile_pci_base + 0x14 + ((m) * 4))
+#define PCI_SELFID		(versatile_pci_base + 0xc)
+
+#define DEVICE_ID_OFFSET		0x00
+#define CSR_OFFSET			0x04
+#define CLASS_ID_OFFSET			0x08
+
+#define VP_PCI_DEVICE_ID		0x030010ee
+#define VP_PCI_CLASS_ID			0x0b400000
+
+static unsigned long pci_slot_ignore;
+
+static int __init versatile_pci_slot_ignore(char *str)
+{
+	int retval;
+	int slot;
+
+	while ((retval = get_option(&str, &slot))) {
+		if ((slot < 0) || (slot > 31))
+			pr_err("Illegal slot value: %d\n", slot);
+		else
+			pci_slot_ignore |= (1 << slot);
+	}
+	return 1;
+}
+
+__setup("pci_slot_ignore=", versatile_pci_slot_ignore);
+
+
+static void __iomem *__pci_addr(struct pci_bus *bus,
+				unsigned int devfn, int offset)
+{
+	unsigned int busnr = bus->number;
+
+	/*
+	 * Trap out illegal values
+	 */
+	BUG_ON(offset > 255);
+	BUG_ON(busnr > 255);
+	BUG_ON(devfn > 255);
+
+	return versatile_cfg_base[1] + ((busnr << 16) |
+		(PCI_SLOT(devfn) << 11) | (PCI_FUNC(devfn) << 8) | offset);
+}
+
+static int versatile_read_config(struct pci_bus *bus, unsigned int devfn,
+				 int where, int size, u32 *val)
+{
+	void __iomem *addr = __pci_addr(bus, devfn, where & ~3);
+	u32 v;
+	int slot = PCI_SLOT(devfn);
+
+	if (pci_slot_ignore & (1 << slot)) {
+		/* Ignore this slot */
+		v = (1 << (8 * size)) - 1;
+	} else {
+		switch (size) {
+		case 1:
+			v = readl(addr);
+			if (where & 2)
+				v >>= 16;
+			if (where & 1)
+				v >>= 8;
+			v &= 0xff;
+			break;
+
+		case 2:
+			v = readl(addr);
+			if (where & 2)
+				v >>= 16;
+			v &= 0xffff;
+			break;
+
+		default:
+			v = readl(addr);
+			break;
+		}
+	}
+
+	*val = v;
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int versatile_write_config(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 val)
+{
+	void __iomem *addr = __pci_addr(bus, devfn, where);
+	int slot = PCI_SLOT(devfn);
+
+	if (pci_slot_ignore & (1 << slot))
+		return PCIBIOS_SUCCESSFUL;
+
+	switch (size) {
+	case 1:
+		writeb((u8)val, addr);
+		break;
+
+	case 2:
+		writew((u16)val, addr);
+		break;
+
+	case 4:
+		writel(val, addr);
+		break;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops pci_versatile_ops = {
+	.read	= versatile_read_config,
+	.write	= versatile_write_config,
+};
+
+static const struct of_device_id versatile_pci_of_match[] = {
+	{ .compatible = "arm,versatile-pci", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, versatile_pci_of_match);
+
+/* Unused, temporary to satisfy ARM arch code */
+struct pci_sys_data sys;
+
+static int versatile_pci_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	int ret, i, mem = 1, myslot = -1;
+	unsigned int lastbus;
+	u32 val;
+	struct pci_host_bridge *bridge;
+	void __iomem *local_pci_cfg_base;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+	versatile_pci_base = devm_ioremap_resource(&pdev->dev, res);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res)
+		return -ENODEV;
+	versatile_cfg_base[0] = devm_ioremap_resource(&pdev->dev, res);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	if (!res)
+		return -ENODEV;
+	versatile_cfg_base[1] = devm_ioremap_resource(&pdev->dev, res);
+
+	bridge = of_create_pci_host_bridge(&pdev->dev, &pci_versatile_ops, &sys);
+	if (!bridge)
+		return -ENODEV;
+
+	/*
+	 *  We need to discover the PCI core first to configure itself
+	 *  before the main PCI probing is performed
+	 */
+	for (i = 0; i < 32; i++) {
+		if ((readl(versatile_cfg_base[0] + (i << 11) + DEVICE_ID_OFFSET) == VP_PCI_DEVICE_ID) &&
+		    (readl(versatile_cfg_base[0] + (i << 11) + CLASS_ID_OFFSET) == VP_PCI_CLASS_ID)) {
+			myslot = i;
+			break;
+		}
+	}
+	if (myslot == -1) {
+		dev_err(&pdev->dev, "Cannot find PCI core!\n");
+		ret = -EIO;
+		goto out;
+	}
+	/*
+	 * Do not to map Versatile FPGA PCI device into memory space
+	 */
+	pci_slot_ignore |= (1 << myslot);
+
+	dev_info(&pdev->dev, "PCI core found (slot %d)\n", myslot);
+
+	writel(myslot, PCI_SELFID);
+	local_pci_cfg_base = versatile_cfg_base[1] + (myslot << 11);
+
+	val = readl(local_pci_cfg_base + CSR_OFFSET);
+	val |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE;
+	writel(val, local_pci_cfg_base + CSR_OFFSET);
+
+	/*
+	 * Configure the PCI inbound memory windows to be 1:1 mapped to SDRAM
+	 */
+	writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_0);
+	writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_1);
+	writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_2);
+
+	/*
+	 * For many years the kernel and QEMU were symbiotically buggy
+	 * in that they both assumed the same broken IRQ mapping.
+	 * QEMU therefore attempts to auto-detect old broken kernels
+	 * so that they still work on newer QEMU as they did on old
+	 * QEMU. Since we now use the correct (ie matching-hardware)
+	 * IRQ mapping we write a definitely different value to a
+	 * PCI_INTERRUPT_LINE register to tell QEMU that we expect
+	 * real hardware behaviour and it need not be backwards
+	 * compatible for us. This write is harmless on real hardware.
+	 */
+	writel(0, versatile_cfg_base[0] + PCI_INTERRUPT_LINE);
+
+	pci_bus_for_each_resource(bridge->bus, res, i) {
+		if (!res || (resource_type(res) != IORESOURCE_MEM))
+			continue;
+
+		writel(res->start >> 28, PCI_IMAP(mem));
+		writel(PHYS_OFFSET >> 28, PCI_SMAP(mem));
+
+		mem++;
+	}
+
+	pci_ioremap_io(0, bridge->io_base);
+
+	/* We always enable PCI domains and we keep domain 0 backward
+	 * compatible in /proc for video cards
+	 */
+	pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
+	pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
+
+	lastbus = pci_scan_child_bus(bridge->bus);
+	pci_bus_update_busn_res_end(bridge->bus, lastbus);
+
+	pci_assign_unassigned_bus_resources(bridge->bus);
+
+	pci_bus_add_devices(bridge->bus);
+
+	return 0;
+
+ out:
+	return ret;
+}
+
+static struct platform_driver versatile_pci_driver = {
+	.driver = {
+		.name = "versatile-pci",
+		.owner = THIS_MODULE,
+		.of_match_table = versatile_pci_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = versatile_pci_probe,
+};
+module_platform_driver(versatile_pci_driver);
+
+MODULE_DESCRIPTION("Versatile PCI driver");
+MODULE_LICENSE("GPLv2");
-- 
1.8.3.2

Re: [PATCH 0/3] Versatile PCI DT support

[Liviu Dudau]

On Thu, Mar 27, 2014 at 10:46:35PM +0000, Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> This series adds a platform driver for Versatile PB's PCI host using
> Liviu's recent patch series[1] for DT parsing and setup.
> 
> The first patch is a hack to get Liviu's current patches to work on ARM.
> It at least shows we are not that far off from being able to use the
> series on ARM.
> 
> I've tested PCI under QEMU, but need someone with actual h/w to test.
> A branch with this series and which includes full conversion of
> Versatile to DT is available here[2].

Hi Rob,

Thanks for doing this. Hopefully others will take inspiration and start to
convert their host bridge controllers as well.

I will post an updated series soon. (Un)fortunately it will look slightly
different than the last one I've posted as I am trying to get more code
that I've currently placed in arch/arm64/kernel/pci.c into drivers/pci,
so you will need to revisit your series.

Regarding your comment about bridge->bus->self being needed: root busses
are supposed to have bus->self == NULL, and the bridge->bus *is* the root
bus. Where would you need to use bridge->bus->self?

Best regards,
Liviu

> 
> Rob
> 
> [1] https://lkml.org/lkml/2014/3/14/279
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git versatile
> 
> Rob Herring (3):
>   ARM: hackup pcibios support for commmon bridge code
>   dt/bindings: add versatile PCI binding
>   pci: add DT based ARM Versatile PCI host driver
> 
>  .../devicetree/bindings/pci/versatile.txt          |  59 +++++
>  arch/arm/kernel/bios32.c                           |  63 +++++
>  drivers/pci/host/Kconfig                           |   4 +
>  drivers/pci/host/Makefile                          |   1 +
>  drivers/pci/host/pci-versatile.c                   | 275 +++++++++++++++++++++
>  5 files changed, 402 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/versatile.txt
>  create mode 100644 drivers/pci/host/pci-versatile.c
> 
> -- 
> 1.8.3.2
> 
> 

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

Re: [PATCH 0/3] Versatile PCI DT support

[Rob Herring]

On Fri, Mar 28, 2014 at 6:46 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Thu, Mar 27, 2014 at 10:46:35PM +0000, Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> This series adds a platform driver for Versatile PB's PCI host using
>> Liviu's recent patch series[1] for DT parsing and setup.
>>
>> The first patch is a hack to get Liviu's current patches to work on ARM.
>> It at least shows we are not that far off from being able to use the
>> series on ARM.
>>
>> I've tested PCI under QEMU, but need someone with actual h/w to test.
>> A branch with this series and which includes full conversion of
>> Versatile to DT is available here[2].
>
> Hi Rob,
>
> Thanks for doing this. Hopefully others will take inspiration and start to
> convert their host bridge controllers as well.
>
> I will post an updated series soon. (Un)fortunately it will look slightly
> different than the last one I've posted as I am trying to get more code
> that I've currently placed in arch/arm64/kernel/pci.c into drivers/pci,
> so you will need to revisit your series.
>
> Regarding your comment about bridge->bus->self being needed: root busses
> are supposed to have bus->self == NULL, and the bridge->bus *is* the root
> bus. Where would you need to use bridge->bus->self?

If I want to use pci_write_config_* accesses, then I need a struct
pci_dev pointer for the host. I can get this since I know the slot of
the host, but only after pci_bus_add_devices which is perhaps too
late. Historically, the Versatile PCI driver has blocked config
accesses to the host's config space. I'm just wondering if this is
really correct behavior and whether the core code should do the setup.
This is setup that is done which doesn't seem like it is very specific
to Versatile.

local_pci_cfg_base = versatile_cfg_base[1] + (myslot << 11);

val = readl(local_pci_cfg_base + CSR_OFFSET);
val |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE;
writel(val, local_pci_cfg_base + CSR_OFFSET);

/*
* Configure the PCI inbound memory windows to be 1:1 mapped to SDRAM
*/
writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_0);
writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_1);
writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_2);


Rob

Re: [PATCH 0/3] Versatile PCI DT support

[Liviu Dudau]

On Fri, Mar 28, 2014 at 01:27:05PM +0000, Rob Herring wrote:
> On Fri, Mar 28, 2014 at 6:46 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > On Thu, Mar 27, 2014 at 10:46:35PM +0000, Rob Herring wrote:
> >> From: Rob Herring <robh@kernel.org>
> >>
> >> This series adds a platform driver for Versatile PB's PCI host using
> >> Liviu's recent patch series[1] for DT parsing and setup.
> >>
> >> The first patch is a hack to get Liviu's current patches to work on ARM.
> >> It at least shows we are not that far off from being able to use the
> >> series on ARM.
> >>
> >> I've tested PCI under QEMU, but need someone with actual h/w to test.
> >> A branch with this series and which includes full conversion of
> >> Versatile to DT is available here[2].
> >
> > Hi Rob,
> >
> > Thanks for doing this. Hopefully others will take inspiration and start to
> > convert their host bridge controllers as well.
> >
> > I will post an updated series soon. (Un)fortunately it will look slightly
> > different than the last one I've posted as I am trying to get more code
> > that I've currently placed in arch/arm64/kernel/pci.c into drivers/pci,
> > so you will need to revisit your series.
> >
> > Regarding your comment about bridge->bus->self being needed: root busses
> > are supposed to have bus->self == NULL, and the bridge->bus *is* the root
> > bus. Where would you need to use bridge->bus->self?
> 
> If I want to use pci_write_config_* accesses, then I need a struct
> pci_dev pointer for the host. I can get this since I know the slot of
> the host, but only after pci_bus_add_devices which is perhaps too
> late. Historically, the Versatile PCI driver has blocked config
> accesses to the host's config space. I'm just wondering if this is
> really correct behavior and whether the core code should do the setup.

PCI core? It knows nothing of the Versatile registers. Versatile core code? Maybe.

> This is setup that is done which doesn't seem like it is very specific
> to Versatile.
> 
> local_pci_cfg_base = versatile_cfg_base[1] + (myslot << 11);
> 
> val = readl(local_pci_cfg_base + CSR_OFFSET);
> val |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE;
> writel(val, local_pci_cfg_base + CSR_OFFSET);

Why don't you do this in a versatile_fixup_bridge() call, like pci-tegra.c
does?

> 
> /*
> * Configure the PCI inbound memory windows to be 1:1 mapped to SDRAM
> */
> writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_0);
> writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_1);
> writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_2);

My guess is that this has to be done at probe time. It just happens that for
Versatile PCI they live in the host config space, but they are not really
PCI config space writes.

Best regards,
Liviu

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

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

Re: [PATCH 0/3] Versatile PCI DT support

[Rob Herring]

On Fri, Mar 28, 2014 at 9:57 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Fri, Mar 28, 2014 at 01:27:05PM +0000, Rob Herring wrote:
>> On Fri, Mar 28, 2014 at 6:46 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>> > On Thu, Mar 27, 2014 at 10:46:35PM +0000, Rob Herring wrote:
>> >> From: Rob Herring <robh@kernel.org>
>> >>
>> >> This series adds a platform driver for Versatile PB's PCI host using
>> >> Liviu's recent patch series[1] for DT parsing and setup.
>> >>
>> >> The first patch is a hack to get Liviu's current patches to work on ARM.
>> >> It at least shows we are not that far off from being able to use the
>> >> series on ARM.
>> >>
>> >> I've tested PCI under QEMU, but need someone with actual h/w to test.
>> >> A branch with this series and which includes full conversion of
>> >> Versatile to DT is available here[2].
>> >
>> > Hi Rob,
>> >
>> > Thanks for doing this. Hopefully others will take inspiration and start to
>> > convert their host bridge controllers as well.
>> >
>> > I will post an updated series soon. (Un)fortunately it will look slightly
>> > different than the last one I've posted as I am trying to get more code
>> > that I've currently placed in arch/arm64/kernel/pci.c into drivers/pci,
>> > so you will need to revisit your series.
>> >
>> > Regarding your comment about bridge->bus->self being needed: root busses
>> > are supposed to have bus->self == NULL, and the bridge->bus *is* the root
>> > bus. Where would you need to use bridge->bus->self?
>>
>> If I want to use pci_write_config_* accesses, then I need a struct
>> pci_dev pointer for the host. I can get this since I know the slot of
>> the host, but only after pci_bus_add_devices which is perhaps too
>> late. Historically, the Versatile PCI driver has blocked config
>> accesses to the host's config space. I'm just wondering if this is
>> really correct behavior and whether the core code should do the setup.
>
> PCI core? It knows nothing of the Versatile registers. Versatile core code? Maybe.

These are all standard config space registers based on the offsets and
documentation. The only thing that I think might make this Versatile
specific is a host bridge having its own config space at all is
optional.

>> This is setup that is done which doesn't seem like it is very specific
>> to Versatile.
>>
>> local_pci_cfg_base = versatile_cfg_base[1] + (myslot << 11);
>>
>> val = readl(local_pci_cfg_base + CSR_OFFSET);
>> val |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE;
>> writel(val, local_pci_cfg_base + CSR_OFFSET);
>
> Why don't you do this in a versatile_fixup_bridge() call, like pci-tegra.c
> does?

I could, but doesn't any host need these bits set (assuming the host
has a config space)?

As it stands now, the PCI core would never call a fixup because the
host's config space is masked out (with pci_slot_ignore). So is that
wrong?

>>
>> /*
>> * Configure the PCI inbound memory windows to be 1:1 mapped to SDRAM
>> */
>> writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_0);
>> writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_1);
>> writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_2);
>
> My guess is that this has to be done at probe time. It just happens that for
> Versatile PCI they live in the host config space, but they are not really
> PCI config space writes.

It is not completely obvious since they use private defines, but
PCI_BASE_ADDRESS_x registers are BAR registers AFAICT.

Rob

Re: [PATCH 0/3] Versatile PCI DT support

[Liviu Dudau]

On Fri, Mar 28, 2014 at 03:20:20PM +0000, Rob Herring wrote:
> On Fri, Mar 28, 2014 at 9:57 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > On Fri, Mar 28, 2014 at 01:27:05PM +0000, Rob Herring wrote:
> >> On Fri, Mar 28, 2014 at 6:46 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> >> > On Thu, Mar 27, 2014 at 10:46:35PM +0000, Rob Herring wrote:
> >> >> From: Rob Herring <robh@kernel.org>
> >> >>
> >> >> This series adds a platform driver for Versatile PB's PCI host using
> >> >> Liviu's recent patch series[1] for DT parsing and setup.
> >> >>
> >> >> The first patch is a hack to get Liviu's current patches to work on ARM.
> >> >> It at least shows we are not that far off from being able to use the
> >> >> series on ARM.
> >> >>
> >> >> I've tested PCI under QEMU, but need someone with actual h/w to test.
> >> >> A branch with this series and which includes full conversion of
> >> >> Versatile to DT is available here[2].
> >> >
> >> > Hi Rob,
> >> >
> >> > Thanks for doing this. Hopefully others will take inspiration and start to
> >> > convert their host bridge controllers as well.
> >> >
> >> > I will post an updated series soon. (Un)fortunately it will look slightly
> >> > different than the last one I've posted as I am trying to get more code
> >> > that I've currently placed in arch/arm64/kernel/pci.c into drivers/pci,
> >> > so you will need to revisit your series.
> >> >
> >> > Regarding your comment about bridge->bus->self being needed: root busses
> >> > are supposed to have bus->self == NULL, and the bridge->bus *is* the root
> >> > bus. Where would you need to use bridge->bus->self?
> >>
> >> If I want to use pci_write_config_* accesses, then I need a struct
> >> pci_dev pointer for the host. I can get this since I know the slot of
> >> the host, but only after pci_bus_add_devices which is perhaps too
> >> late. Historically, the Versatile PCI driver has blocked config
> >> accesses to the host's config space. I'm just wondering if this is
> >> really correct behavior and whether the core code should do the setup.
> >
> > PCI core? It knows nothing of the Versatile registers. Versatile core code? Maybe.
> 
> These are all standard config space registers based on the offsets and
> documentation. The only thing that I think might make this Versatile
> specific is a host bridge having its own config space at all is
> optional.

Sorry, got confused by your CSR_OFFSET macro into thinking it's Versatile specific (I
still cannot get hold of the SP810 manual, sigh). Why don't you use PCI_COMMAND ?

> 
> >> This is setup that is done which doesn't seem like it is very specific
> >> to Versatile.
> >>
> >> local_pci_cfg_base = versatile_cfg_base[1] + (myslot << 11);
> >>
> >> val = readl(local_pci_cfg_base + CSR_OFFSET);
> >> val |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE;
> >> writel(val, local_pci_cfg_base + CSR_OFFSET);
> >
> > Why don't you do this in a versatile_fixup_bridge() call, like pci-tegra.c
> > does?
> 
> I could, but doesn't any host need these bits set (assuming the host
> has a config space)?
> 
> As it stands now, the PCI core would never call a fixup because the
> host's config space is masked out (with pci_slot_ignore). So is that
> wrong?

No, it is right. Host bridge config space should not be visible if it is a PCI
host bridge. And I'm guessing that this is the reason why everyone and their dog
does the PCI_COMMAND register setup for the host brige manually.

For PCIe the spec is a bit more forgiving is you use ECAM, although it doesn't
explicitly allow writes to the config space of the host bridge. From PCI Express
Base spec, rev 3.0:

	7.2.2.1. Host Bridge Requirements 
	For those systems that implement the ECAM, the PCI Express Host Bridge is
	required to translate the memory-mapped PCI Express Configuration Space
	accesses from the host processor to PCI Express configuration transactions.
	The use of Host Bridge PCI class code is Reserved for backwards compatibility;
	host Bridge Configuration Space is opaque to standard PCI Express software
	and may be implemented in an implementation specific manner that is compatible
	with PCI Host Bridge Type 0 Configuration Space.  A PCI Express Host Bridge is
	not required to signal errors through a Root Complex Event Collector.  This
	support is optional for PCI Express Host Bridges. 

So it looks like you need to do those in the host bridge probe function, rather than
in a generic way.

Best regards,
Liviu

> 
> >>
> >> /*
> >> * Configure the PCI inbound memory windows to be 1:1 mapped to SDRAM
> >> */
> >> writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_0);
> >> writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_1);
> >> writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_2);
> >
> > My guess is that this has to be done at probe time. It just happens that for
> > Versatile PCI they live in the host config space, but they are not really
> > PCI config space writes.
> 
> It is not completely obvious since they use private defines, but
> PCI_BASE_ADDRESS_x registers are BAR registers AFAICT.
> 
> Rob
> --
> 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
> 

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

Re: [PATCH 0/3] Versatile PCI DT support

[Arnd Bergmann]

On Friday 28 March 2014 16:21:38 Liviu Dudau wrote:
> On Fri, Mar 28, 2014 at 03:20:20PM +0000, Rob Herring wrote:
> > On Fri, Mar 28, 2014 at 9:57 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > PCI core? It knows nothing of the Versatile registers. Versatile core code? Maybe.
> > 
> > These are all standard config space registers based on the offsets and
> > documentation. The only thing that I think might make this Versatile
> > specific is a host bridge having its own config space at all is
> > optional.
> 
> Sorry, got confused by your CSR_OFFSET macro into thinking it's Versatile specific (I
> still cannot get hold of the SP810 manual, sigh). Why don't you use PCI_COMMAND ?

While it's not uncommon to have PCI host controllers use a similar
layout for their own registers as the normal config space, I believe
this is not standardized anywhere. It may be better not to pretend
that this is a standard register.

> For PCIe the spec is a bit more forgiving is you use ECAM, although it doesn't
> explicitly allow writes to the config space of the host bridge. From PCI Express
> Base spec, rev 3.0:
> 
> 	7.2.2.1. Host Bridge Requirements 
> 	For those systems that implement the ECAM, the PCI Express Host Bridge is
> 	required to translate the memory-mapped PCI Express Configuration Space
> 	accesses from the host processor to PCI Express configuration transactions.
> 	The use of Host Bridge PCI class code is Reserved for backwards compatibility;
> 	host Bridge Configuration Space is opaque to standard PCI Express software
> 	and may be implemented in an implementation specific manner that is compatible
> 	with PCI Host Bridge Type 0 Configuration Space.  A PCI Express Host Bridge is
> 	not required to signal errors through a Root Complex Event Collector.  This
> 	support is optional for PCI Express Host Bridges. 
> 
> So it looks like you need to do those in the host bridge probe function, rather than
> in a generic way.

Right. FWIW, the reason why these have a normal register layout seems to be
that the same hardware is used for host controller and endpoint devices.

	Arnd

Re: [PATCH 0/3] Versatile PCI DT support

[Bjorn Helgaas]

On Thu, Mar 27, 2014 at 05:46:35PM -0500, Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> This series adds a platform driver for Versatile PB's PCI host using
> Liviu's recent patch series[1] for DT parsing and setup.
> 
> The first patch is a hack to get Liviu's current patches to work on ARM.
> It at least shows we are not that far off from being able to use the
> series on ARM.

Since this depends on Liviu's work that is not yet upstream, I guess I
can't do anything with this yet, right?  Poke me again when I can.

I assume you would want a MAINTAINERS update, too (at least, *I* do :))

> I've tested PCI under QEMU, but need someone with actual h/w to test.
> A branch with this series and which includes full conversion of
> Versatile to DT is available here[2].
> 
> Rob
> 
> [1] https://lkml.org/lkml/2014/3/14/279
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git versatile
> 
> Rob Herring (3):
>   ARM: hackup pcibios support for commmon bridge code
>   dt/bindings: add versatile PCI binding
>   pci: add DT based ARM Versatile PCI host driver
> 
>  .../devicetree/bindings/pci/versatile.txt          |  59 +++++
>  arch/arm/kernel/bios32.c                           |  63 +++++
>  drivers/pci/host/Kconfig                           |   4 +
>  drivers/pci/host/Makefile                          |   1 +
>  drivers/pci/host/pci-versatile.c                   | 275 +++++++++++++++++++++
>  5 files changed, 402 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/versatile.txt
>  create mode 100644 drivers/pci/host/pci-versatile.c
> 
> -- 
> 1.8.3.2
> 

Re: [PATCH 1/3] ARM: hackup pcibios support for commmon bridge code

[Bjorn Helgaas]

On Thu, Mar 27, 2014 at 05:46:36PM -0500, Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  arch/arm/kernel/bios32.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 317da88..31ec14a 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/of_pci.h>
>  
>  #include <asm/mach-types.h>
>  #include <asm/mach/map.h>
> @@ -337,6 +338,15 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		u16 cmd;
>  
> +		/* Ignore fully discovered devices */
> +		if (dev->is_added)
> +			continue;
> +
> +		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));

We already do this in pci_device_add(); why do we need it here, too?

> +		/* Read default IRQs and fixup if necessary */
> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);

Could this be done in pcibios_enable_device() or some other per-device
interface()?  I'd like to get rid of pcibios_fixup_bus() eventually.

>  		pci_read_config_word(dev, PCI_COMMAND, &cmd);
>  		cmd |= features;
>  		pci_write_config_word(dev, PCI_COMMAND, cmd);
> @@ -681,3 +691,56 @@ void __init pci_map_io_early(unsigned long pfn)
>  	pci_io_desc.pfn = pfn;
>  	iotable_init(&pci_io_desc, 1);
>  }
> +
> +struct ioresource {
> +	struct list_head list;
> +	phys_addr_t start;
> +	resource_size_t size;
> +};
> +
> +static LIST_HEAD(io_list);
> +
> +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> +{
> +	struct ioresource *res;
> +	resource_size_t allocated_size = 0;
> +
> +	/* find if the range has not been already allocated */
> +	list_for_each_entry(res, &io_list, list) {
> +		if (address >= res->start &&
> +			address + size <= res->start + size)
> +			return 0;
> +		allocated_size += res->size;
> +	}
> +
> +	/* range not already registered, check for space */
> +	if (allocated_size + size > IO_SPACE_LIMIT)
> +		return -E2BIG;
> +
> +	/* add the range in the list */
> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return -ENOMEM;
> +	res->start = address;
> +	res->size = size;
> +
> +	list_add_tail(&res->list, &io_list);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_register_io_range);
> +
> +unsigned long pci_address_to_pio(phys_addr_t address)
> +{
> +	struct ioresource *res;
> +
> +	list_for_each_entry(res, &io_list, list) {
> +		if (address >= res->start &&
> +			address < res->start + res->size) {
> +			return res->start - address;
> +		}
> +	}
> +
> +	return (unsigned long)-1;
> +}
> +EXPORT_SYMBOL_GPL(pci_address_to_pio);

ISTR some discussion about this, so maybe this has already been addressed,
but it doesn't seem right to export generically-named symbols like these
from the ARM arch code.  And they don't look very ARM-specific.

Bjorn

Re: [PATCH 3/3] pci: add DT based ARM Versatile PCI host driver

[Bjorn Helgaas]

On Thu, Mar 27, 2014 at 05:46:38PM -0500, Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> This converts the Versatile PCI host code to a platform driver using
> of_create_pci_host_bridge for parsing DT and setup.
> 
> I think more of this setup could be done by the core code. There are
> accesses to the host's config space (accesses using local_pci_cfg_base)
> which seem like they could be done by the core code or using standard
> config space accessors. The problem is bridge->bus->self is needed, but
> it does not get setup. I'm not exactly sure how that should work.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> ...

> +static int versatile_pci_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	int ret, i, mem = 1, myslot = -1;
> +	unsigned int lastbus;
> +	u32 val;
> +	struct pci_host_bridge *bridge;
> +	void __iomem *local_pci_cfg_base;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +	versatile_pci_base = devm_ioremap_resource(&pdev->dev, res);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res)
> +		return -ENODEV;
> +	versatile_cfg_base[0] = devm_ioremap_resource(&pdev->dev, res);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	if (!res)
> +		return -ENODEV;
> +	versatile_cfg_base[1] = devm_ioremap_resource(&pdev->dev, res);
> +
> +	bridge = of_create_pci_host_bridge(&pdev->dev, &pci_versatile_ops, &sys);

Are we still relying on the PCI core to default to a bus number range of
00-ff?  Can we not do that?

Bjorn

Re: [PATCH 3/3] pci: add DT based ARM Versatile PCI host driver

[Rob Herring]

On Thu, Apr 24, 2014 at 6:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Mar 27, 2014 at 05:46:38PM -0500, Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> This converts the Versatile PCI host code to a platform driver using
>> of_create_pci_host_bridge for parsing DT and setup.
>>
>> I think more of this setup could be done by the core code. There are
>> accesses to the host's config space (accesses using local_pci_cfg_base)
>> which seem like they could be done by the core code or using standard
>> config space accessors. The problem is bridge->bus->self is needed, but
>> it does not get setup. I'm not exactly sure how that should work.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> ...

>> +     bridge = of_create_pci_host_bridge(&pdev->dev, &pci_versatile_ops, &sys);
>
> Are we still relying on the PCI core to default to a bus number range of
> 00-ff?  Can we not do that?

No, that comes from DT.

Rob

Re: [PATCH 1/3] ARM: hackup pcibios support for commmon bridge code

[Rob Herring]

On Thu, Apr 24, 2014 at 6:20 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Mar 27, 2014 at 05:46:36PM -0500, Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>>  arch/arm/kernel/bios32.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>>
>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
>> index 317da88..31ec14a 100644
>> --- a/arch/arm/kernel/bios32.c
>> +++ b/arch/arm/kernel/bios32.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>> +#include <linux/of_pci.h>
>>
>>  #include <asm/mach-types.h>
>>  #include <asm/mach/map.h>
>> @@ -337,6 +338,15 @@ void pcibios_fixup_bus(struct pci_bus *bus)
>>       list_for_each_entry(dev, &bus->devices, bus_list) {
>>               u16 cmd;
>>
>> +             /* Ignore fully discovered devices */
>> +             if (dev->is_added)
>> +                     continue;
>> +
>> +             set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>
> We already do this in pci_device_add(); why do we need it here, too?

Because it was in the arm64 code. I'm not really sure.

>> +             /* Read default IRQs and fixup if necessary */
>> +             dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>
> Could this be done in pcibios_enable_device() or some other per-device
> interface()?  I'd like to get rid of pcibios_fixup_bus() eventually.

Probably. This should return NO_IRQ in the !OF case. We'd need to make
sure that works.

I had a loop at pcibios_fixup_bus across architectures and could make
no sense of why things were done in here. It all seemed a bit random
and left me with questions like shouldn't something always be needed.
Probably a lot of pieces are redundant now that didn't use to be. It
needs someone that knows more about PCI than me to look at.

>>               pci_read_config_word(dev, PCI_COMMAND, &cmd);
>>               cmd |= features;
>>               pci_write_config_word(dev, PCI_COMMAND, cmd);
>> @@ -681,3 +691,56 @@ void __init pci_map_io_early(unsigned long pfn)
>>       pci_io_desc.pfn = pfn;
>>       iotable_init(&pci_io_desc, 1);
>>  }
>> +
>> +struct ioresource {
>> +     struct list_head list;
>> +     phys_addr_t start;
>> +     resource_size_t size;
>> +};
>> +
>> +static LIST_HEAD(io_list);
>> +
>> +int pci_register_io_range(phys_addr_t address, resource_size_t size)
>> +{
>> +     struct ioresource *res;
>> +     resource_size_t allocated_size = 0;
>> +
>> +     /* find if the range has not been already allocated */
>> +     list_for_each_entry(res, &io_list, list) {
>> +             if (address >= res->start &&
>> +                     address + size <= res->start + size)
>> +                     return 0;
>> +             allocated_size += res->size;
>> +     }
>> +
>> +     /* range not already registered, check for space */
>> +     if (allocated_size + size > IO_SPACE_LIMIT)
>> +             return -E2BIG;
>> +
>> +     /* add the range in the list */
>> +     res = kzalloc(sizeof(*res), GFP_KERNEL);
>> +     if (!res)
>> +             return -ENOMEM;
>> +     res->start = address;
>> +     res->size = size;
>> +
>> +     list_add_tail(&res->list, &io_list);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_register_io_range);
>> +
>> +unsigned long pci_address_to_pio(phys_addr_t address)
>> +{
>> +     struct ioresource *res;
>> +
>> +     list_for_each_entry(res, &io_list, list) {
>> +             if (address >= res->start &&
>> +                     address < res->start + res->size) {
>> +                     return res->start - address;
>> +             }
>> +     }
>> +
>> +     return (unsigned long)-1;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_address_to_pio);
>
> ISTR some discussion about this, so maybe this has already been addressed,
> but it doesn't seem right to export generically-named symbols like these
> from the ARM arch code.  And they don't look very ARM-specific.

Right, I expect these to go away with Liviu's the next version.

Rob

Re: [PATCH 3/3] pci: add DT based ARM Versatile PCI host driver

[Bjorn Helgaas]

On Thu, Apr 24, 2014 at 5:37 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Apr 24, 2014 at 6:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Thu, Mar 27, 2014 at 05:46:38PM -0500, Rob Herring wrote:
>>> From: Rob Herring <robh@kernel.org>
>>>
>>> This converts the Versatile PCI host code to a platform driver using
>>> of_create_pci_host_bridge for parsing DT and setup.
>>>
>>> I think more of this setup could be done by the core code. There are
>>> accesses to the host's config space (accesses using local_pci_cfg_base)
>>> which seem like they could be done by the core code or using standard
>>> config space accessors. The problem is bridge->bus->self is needed, but
>>> it does not get setup. I'm not exactly sure how that should work.
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>> ...
>
>>> +     bridge = of_create_pci_host_bridge(&pdev->dev, &pci_versatile_ops, &sys);
>>
>> Are we still relying on the PCI core to default to a bus number range of
>> 00-ff?  Can we not do that?
>
> No, that comes from DT.

Oh, good.  That's done inside of_create_pci_host_bridge(), then?  (I
haven't looked at that code.)

Bjorn