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

[PATCH 4/9] 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
the commom DT parsing and setup. The driver uses only an empty ARM
pci_sys_data struct and does not use pci_common_init_dev init function.
The old host code will be removed in a subsequent commit when Versatile
is completely converted to DT.

I've tested this on QEMU with the sym53c8xx driver in both i/o and
memory mapped modes.

Signed-off-by: Rob Herring <robh@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 drivers/pci/host/Kconfig         |   4 +
 drivers/pci/host/Makefile        |   1 +
 drivers/pci/host/pci-versatile.c | 305 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 310 insertions(+)
 create mode 100644 drivers/pci/host/pci-versatile.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index c4b6568..7b892a9 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -102,4 +102,8 @@ config PCI_LAYERSCAPE
 	help
 	  Say Y here if you want PCIe controller support on Layerscape SoCs.
 
+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 44c2699..e61d91c 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
 obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.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..a3e7bc5
--- /dev/null
+++ b/drivers/pci/host/pci-versatile.c
@@ -0,0 +1,305 @@
+/*
+ * 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>
+
+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 VP_PCI_DEVICE_ID		0x030010ee
+#define VP_PCI_CLASS_ID			0x0b400000
+
+static u32 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 int versatile_pci_parse_request_of_pci_ranges(struct device *dev,
+						     struct list_head *res)
+{
+	int err, mem = 1, res_valid = 0;
+	struct device_node *np = dev->of_node;
+	resource_size_t iobase;
+	struct pci_host_bridge_window *win;
+
+	err = of_pci_get_host_bridge_resources(np, 0, 0xff, res, &iobase);
+	if (err)
+		return err;
+
+	list_for_each_entry(win, res, list) {
+		struct resource *parent, *res = win->res;
+
+		switch (resource_type(res)) {
+		case IORESOURCE_IO:
+			parent = &ioport_resource;
+			err = pci_remap_iospace(res, iobase);
+			if (err) {
+				dev_warn(dev, "error %d: failed to map resource %pR\n",
+					 err, res);
+				continue;
+			}
+			break;
+		case IORESOURCE_MEM:
+			parent = &iomem_resource;
+			res_valid |= !(res->flags & IORESOURCE_PREFETCH);
+
+			writel(res->start >> 28, PCI_IMAP(mem));
+			writel(PHYS_OFFSET >> 28, PCI_SMAP(mem));
+			mem++;
+
+			break;
+		case IORESOURCE_BUS:
+		default:
+			continue;
+		}
+
+		err = devm_request_resource(dev, parent, res);
+		if (err)
+			goto out_release_res;
+	}
+
+	if (!res_valid) {
+		dev_err(dev, "non-prefetchable memory resource required\n");
+		err = -EINVAL;
+		goto out_release_res;
+	}
+
+	return 0;
+
+out_release_res:
+	pci_free_resource_list(res);
+	return err;
+}
+
+/* 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, myslot = -1;
+	u32 val;
+	void __iomem *local_pci_cfg_base;
+	struct pci_bus *bus;
+	LIST_HEAD(pci_res);
+
+	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);
+
+	ret = versatile_pci_parse_request_of_pci_ranges(&pdev->dev, &pci_res);
+	if (ret)
+		return ret;
+
+	/*
+	 *  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) + PCI_VENDOR_ID) == VP_PCI_DEVICE_ID) &&
+		    (readl(versatile_cfg_base[0] + (i << 11) + PCI_CLASS_REVISION) == VP_PCI_CLASS_ID)) {
+			myslot = i;
+			break;
+		}
+	}
+	if (myslot == -1) {
+		dev_err(&pdev->dev, "Cannot find PCI core!\n");
+		return -EIO;
+	}
+	/*
+	 * 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 + PCI_COMMAND);
+	val |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE;
+	writel(val, local_pci_cfg_base + PCI_COMMAND);
+
+	/*
+	 * 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_add_flags(PCI_ENABLE_PROC_DOMAINS);
+	pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
+
+	bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res);
+	if (!bus)
+		return -ENOMEM;
+
+	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+	pci_assign_unassigned_bus_resources(bus);
+
+	return 0;
+}
+
+static const struct of_device_id versatile_pci_of_match[] = {
+	{ .compatible = "arm,versatile-pci", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, versatile_pci_of_match);
+
+static struct platform_driver versatile_pci_driver = {
+	.driver = {
+		.name = "versatile-pci",
+		.of_match_table = versatile_pci_of_match,
+	},
+	.probe = versatile_pci_probe,
+};
+module_platform_driver(versatile_pci_driver);
+
+MODULE_DESCRIPTION("Versatile PCI driver");
+MODULE_LICENSE("GPLv2");
-- 
2.1.0

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

[Arnd Bergmann]

On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> This converts the Versatile PCI host code to a platform driver using
> the commom DT parsing and setup. The driver uses only an empty ARM
> pci_sys_data struct and does not use pci_common_init_dev init function.
> The old host code will be removed in a subsequent commit when Versatile
> is completely converted to DT.
> 
> I've tested this on QEMU with the sym53c8xx driver in both i/o and
> memory mapped modes.

Ah, this is quite clever, I think you tried to explain to me what you
did with the pci_sys_data before, but I didn't get it then.
 
> +
> +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);

Isn't an offset>255 something that can be triggered by a non-broken
driver or even user space?

Maybe just return PCIBIOS_BAD_REGISTER_NUMBER in this case?

> +static int versatile_pci_parse_request_of_pci_ranges(struct device *dev,
> +						     struct list_head *res)
> +{
> +	int err, mem = 1, res_valid = 0;
> +	struct device_node *np = dev->of_node;
> +	resource_size_t iobase;
> +	struct pci_host_bridge_window *win;
> +
> +	err = of_pci_get_host_bridge_resources(np, 0, 0xff, res, &iobase);
> +	if (err)
> +		return err;
> +
> +	list_for_each_entry(win, res, list) {
> +		struct resource *parent, *res = win->res;
> +
> +		switch (resource_type(res)) {
> +		case IORESOURCE_IO:
> +			parent = &ioport_resource;
> +			err = pci_remap_iospace(res, iobase);
> +			if (err) {
> +				dev_warn(dev, "error %d: failed to map resource %pR\n",
> +					 err, res);
> +				continue;
> +			}
> +			break;
> +		case IORESOURCE_MEM:
> +			parent = &iomem_resource;
> +			res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> +
> +			writel(res->start >> 28, PCI_IMAP(mem));
> +			writel(PHYS_OFFSET >> 28, PCI_SMAP(mem));
> +			mem++;
> +
> +			break;
> +		case IORESOURCE_BUS:
> +		default:
> +			continue;
> +		}
> +
> +		err = devm_request_resource(dev, parent, res);
> +		if (err)
> +			goto out_release_res;
> +	}

I wonder if we should also request the physical resource for the I/O space
window to have it show up in /proc/iomem. We are rather inconsistent in this
regard between drivers.

> +	pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
> +	pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
> +
> +	bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> +	pci_assign_unassigned_bus_resources(bus);
> +
> +	return 0;

One general question, mainly for Bjorn: pci_scan_root_bus adds all the devices
at the Linux driver level by calling pci_bus_add_devices(), but then we call
pci_fixup_irqs and pci_assign_unassigned_bus_resources after that, which will
change the devices again. Is this how it's meant to work? How do we ensure
that we have the correct irq number and resources by the time we enter the
probe() function of the PCI device driver that gets bound to a device here?

	Arnd

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

[Rob Herring]

On Tue, Dec 30, 2014 at 3:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:
>> From: Rob Herring <robh@kernel.org>
>>
>> This converts the Versatile PCI host code to a platform driver using
>> the commom DT parsing and setup. The driver uses only an empty ARM
>> pci_sys_data struct and does not use pci_common_init_dev init function.
>> The old host code will be removed in a subsequent commit when Versatile
>> is completely converted to DT.
>>
>> I've tested this on QEMU with the sym53c8xx driver in both i/o and
>> memory mapped modes.
>
> Ah, this is quite clever, I think you tried to explain to me what you
> did with the pci_sys_data before, but I didn't get it then.
>
>> +
>> +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);
>
> Isn't an offset>255 something that can be triggered by a non-broken
> driver or even user space?

I don't know. I just copied what the old driver did. Are these checks
even host controller specific?

> Maybe just return PCIBIOS_BAD_REGISTER_NUMBER in this case?

Perhaps. We could probably simplify the config space read/write
functions and just provide the PCI core a bus/devfn/offset to config
address translation function. That would not work in all cases, but
propably most that have memory mapped config space.

>> +static int versatile_pci_parse_request_of_pci_ranges(struct device *dev,
>> +                                                  struct list_head *res)
>> +{
>> +     int err, mem = 1, res_valid = 0;
>> +     struct device_node *np = dev->of_node;
>> +     resource_size_t iobase;
>> +     struct pci_host_bridge_window *win;
>> +
>> +     err = of_pci_get_host_bridge_resources(np, 0, 0xff, res, &iobase);
>> +     if (err)
>> +             return err;
>> +
>> +     list_for_each_entry(win, res, list) {
>> +             struct resource *parent, *res = win->res;
>> +
>> +             switch (resource_type(res)) {
>> +             case IORESOURCE_IO:
>> +                     parent = &ioport_resource;
>> +                     err = pci_remap_iospace(res, iobase);
>> +                     if (err) {
>> +                             dev_warn(dev, "error %d: failed to map resource %pR\n",
>> +                                      err, res);
>> +                             continue;
>> +                     }
>> +                     break;
>> +             case IORESOURCE_MEM:
>> +                     parent = &iomem_resource;
>> +                     res_valid |= !(res->flags & IORESOURCE_PREFETCH);
>> +
>> +                     writel(res->start >> 28, PCI_IMAP(mem));
>> +                     writel(PHYS_OFFSET >> 28, PCI_SMAP(mem));
>> +                     mem++;
>> +
>> +                     break;
>> +             case IORESOURCE_BUS:
>> +             default:
>> +                     continue;
>> +             }
>> +
>> +             err = devm_request_resource(dev, parent, res);
>> +             if (err)
>> +                     goto out_release_res;
>> +     }
>
> I wonder if we should also request the physical resource for the I/O space
> window to have it show up in /proc/iomem. We are rather inconsistent in this
> regard between drivers.

It's a little complicated now because we don't have that as a struct
resource any more. We could reconstruct it from iobase and the i/o
resource size.

>
>> +     pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
>> +     pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
>> +
>> +     bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res);
>> +     if (!bus)
>> +             return -ENOMEM;
>> +
>> +     pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
>> +     pci_assign_unassigned_bus_resources(bus);
>> +
>> +     return 0;
>
> One general question, mainly for Bjorn: pci_scan_root_bus adds all the devices
> at the Linux driver level by calling pci_bus_add_devices(), but then we call
> pci_fixup_irqs and pci_assign_unassigned_bus_resources after that, which will
> change the devices again. Is this how it's meant to work? How do we ensure
> that we have the correct irq number and resources by the time we enter the
> probe() function of the PCI device driver that gets bound to a device here?

I'm a bit puzzled myself. I think that the devices are not probed
until after pci_assign_unassigned_bus_resources. It certainly doesn't
work without that call. Really, I think of_irq_parse_and_map_pci
should be the default if no one else has set the device's irq (and the
host has a device node of course).

It also seems to be a bit of random set of pci functions that are
called here. It would be nice to simplify this chunk of code.

Rob

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

[Arnd Bergmann]

On Friday 02 January 2015 12:14:43 Rob Herring wrote:
> On Tue, Dec 30, 2014 at 3:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:
> >> From: Rob Herring <robh@kernel.org>
> >>
> >> This converts the Versatile PCI host code to a platform driver using
> >> the commom DT parsing and setup. The driver uses only an empty ARM
> >> pci_sys_data struct and does not use pci_common_init_dev init function.
> >> The old host code will be removed in a subsequent commit when Versatile
> >> is completely converted to DT.
> >>
> >> I've tested this on QEMU with the sym53c8xx driver in both i/o and
> >> memory mapped modes.
> >
> > Ah, this is quite clever, I think you tried to explain to me what you
> > did with the pci_sys_data before, but I didn't get it then.
> >
> >> +
> >> +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);
> >
> > Isn't an offset>255 something that can be triggered by a non-broken
> > driver or even user space?
> 
> I don't know. I just copied what the old driver did. Are these checks
> even host controller specific?

A busnr or devfn larger than 255 would be a serious bug on any host
controller, those just don't exist. A BUG_ON check here is not a problem,
but if we want to have it, I'd prefer to put it into the core code.

offset values between 256 and 4095 can be valid on some devices, but
apparently this host controller does not support them.

> > Maybe just return PCIBIOS_BAD_REGISTER_NUMBER in this case?
> 
> Perhaps. We could probably simplify the config space read/write
> functions and just provide the PCI core a bus/devfn/offset to config
> address translation function. That would not work in all cases, but
> propably most that have memory mapped config space.

Actually, thinking about this some more, the implementation seems to
be "CAM" compliant, and we could share the confic space accessors with
the ones from drivers/pci/host/pci-host-generic.c.

> >> +static int versatile_pci_parse_request_of_pci_ranges(struct device *dev,
> >> +                                                  struct list_head *res)
> >> +{
> >> +     int err, mem = 1, res_valid = 0;
> >> +     struct device_node *np = dev->of_node;
> >> +     resource_size_t iobase;
> >> +     struct pci_host_bridge_window *win;
> >> +
> >> +     err = of_pci_get_host_bridge_resources(np, 0, 0xff, res, &iobase);
> >> +     if (err)
> >> +             return err;
> >> +
> >> +     list_for_each_entry(win, res, list) {
> >> +             struct resource *parent, *res = win->res;
> >> +
> >> +             switch (resource_type(res)) {
> >> +             case IORESOURCE_IO:
> >> +                     parent = &ioport_resource;
> >> +                     err = pci_remap_iospace(res, iobase);
> >> +                     if (err) {
> >> +                             dev_warn(dev, "error %d: failed to map resource %pR\n",
> >> +                                      err, res);
> >> +                             continue;
> >> +                     }
> >> +                     break;
> >> +             case IORESOURCE_MEM:
> >> +                     parent = &iomem_resource;
> >> +                     res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> >> +
> >> +                     writel(res->start >> 28, PCI_IMAP(mem));
> >> +                     writel(PHYS_OFFSET >> 28, PCI_SMAP(mem));
> >> +                     mem++;
> >> +
> >> +                     break;
> >> +             case IORESOURCE_BUS:
> >> +             default:
> >> +                     continue;
> >> +             }
> >> +
> >> +             err = devm_request_resource(dev, parent, res);
> >> +             if (err)
> >> +                     goto out_release_res;
> >> +     }
> >
> > I wonder if we should also request the physical resource for the I/O space
> > window to have it show up in /proc/iomem. We are rather inconsistent in this
> > regard between drivers.
> 
> It's a little complicated now because we don't have that as a struct
> resource any more. We could reconstruct it from iobase and the i/o
> resource size.



> >> +     pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
> >> +     pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
> >> +
> >> +     bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res);
> >> +     if (!bus)
> >> +             return -ENOMEM;
> >> +
> >> +     pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> >> +     pci_assign_unassigned_bus_resources(bus);
> >> +
> >> +     return 0;
> >
> > One general question, mainly for Bjorn: pci_scan_root_bus adds all the devices
> > at the Linux driver level by calling pci_bus_add_devices(), but then we call
> > pci_fixup_irqs and pci_assign_unassigned_bus_resources after that, which will
> > change the devices again. Is this how it's meant to work? How do we ensure
> > that we have the correct irq number and resources by the time we enter the
> > probe() function of the PCI device driver that gets bound to a device here?
> 
> I'm a bit puzzled myself. I think that the devices are not probed
> until after pci_assign_unassigned_bus_resources. It certainly doesn't
> work without that call. Really, I think of_irq_parse_and_map_pci
> should be the default if no one else has set the device's irq (and the
> host has a device node of course).
> 
> It also seems to be a bit of random set of pci functions that are
> called here. It would be nice to simplify this chunk of code.

Yes, and we recently had some attempts at creating a better interface posted
on the mailing list, not sure what the latest status on it is. I think we
want to end up with a two-stage interface along the lines of:

	/* allocate a pci_host_bridge, scan DT, set default operations, map
	   I/O space if necessary, request all resources ... */
	phb = pci_host_bridge_create(parent, ..., sizeof(struct my_pci_private));
	if (IS_ERR(phb))
		return PTR_ERR(phb);

	/* any host specific customization */
	phb->bus->ops = my_pci_ops; /* for nonstandard config space access */
	phb->foo = my_pci_foo; /* some other optional function pointers, replacing pcibios_* */
	my_pci = &phb->priv; /* extra allocated data */
	my_pci->clk = devm_clk_get(parent, "pci");
	my_pci->regs = devm_ioremap_resource(parent, platform_get_resource(pdev,
							IORESOURCE_MEM, 0);
	
	/* scan everything */
	return pci_scan_child_bus_and_add_devices(phb->bus);


	Arnd

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

[Rob Herring]

On Fri, Jan 2, 2015 at 2:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 02 January 2015 12:14:43 Rob Herring wrote:
>> On Tue, Dec 30, 2014 at 3:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:
>> >> From: Rob Herring <robh@kernel.org>
>> >>
>> >> This converts the Versatile PCI host code to a platform driver using
>> >> the commom DT parsing and setup. The driver uses only an empty ARM
>> >> pci_sys_data struct and does not use pci_common_init_dev init function.
>> >> The old host code will be removed in a subsequent commit when Versatile
>> >> is completely converted to DT.
>> >>
>> >> I've tested this on QEMU with the sym53c8xx driver in both i/o and
>> >> memory mapped modes.
>> >
>> > Ah, this is quite clever, I think you tried to explain to me what you
>> > did with the pci_sys_data before, but I didn't get it then.
>> >
>> >> +
>> >> +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);
>> >
>> > Isn't an offset>255 something that can be triggered by a non-broken
>> > driver or even user space?
>>
>> I don't know. I just copied what the old driver did. Are these checks
>> even host controller specific?
>
> A busnr or devfn larger than 255 would be a serious bug on any host
> controller, those just don't exist. A BUG_ON check here is not a problem,
> but if we want to have it, I'd prefer to put it into the core code.

Agreed.

> offset values between 256 and 4095 can be valid on some devices, but
> apparently this host controller does not support them.

I'm going to drop the checks. The only checks I see in other drivers
are for the bus number to match the root bus which I'm not convinced
are needed either.

>> > Maybe just return PCIBIOS_BAD_REGISTER_NUMBER in this case?
>>
>> Perhaps. We could probably simplify the config space read/write
>> functions and just provide the PCI core a bus/devfn/offset to config
>> address translation function. That would not work in all cases, but
>> propably most that have memory mapped config space.
>
> Actually, thinking about this some more, the implementation seems to
> be "CAM" compliant, and we could share the confic space accessors with
> the ones from drivers/pci/host/pci-host-generic.c.

Almost. It uses readl for all size accesses. Yet writes support
different access sizes. I would guess that the h/w can support byte
and word reads if writes are supported, but I can't really verify.
Dword-only sized reads or reads and writes seem to be the main
variations for config space accesses. There's a few hosts with more
complex config space access setup, but quite a few only vary in
address decode.

>> >> +static int versatile_pci_parse_request_of_pci_ranges(struct device *dev,
>> >> +                                                  struct list_head *res)
>> >> +{
>> >> +     int err, mem = 1, res_valid = 0;
>> >> +     struct device_node *np = dev->of_node;
>> >> +     resource_size_t iobase;
>> >> +     struct pci_host_bridge_window *win;
>> >> +
>> >> +     err = of_pci_get_host_bridge_resources(np, 0, 0xff, res, &iobase);
>> >> +     if (err)
>> >> +             return err;
>> >> +
>> >> +     list_for_each_entry(win, res, list) {
>> >> +             struct resource *parent, *res = win->res;
>> >> +
>> >> +             switch (resource_type(res)) {
>> >> +             case IORESOURCE_IO:
>> >> +                     parent = &ioport_resource;
>> >> +                     err = pci_remap_iospace(res, iobase);
>> >> +                     if (err) {
>> >> +                             dev_warn(dev, "error %d: failed to map resource %pR\n",
>> >> +                                      err, res);
>> >> +                             continue;
>> >> +                     }
>> >> +                     break;
>> >> +             case IORESOURCE_MEM:
>> >> +                     parent = &iomem_resource;
>> >> +                     res_valid |= !(res->flags & IORESOURCE_PREFETCH);
>> >> +
>> >> +                     writel(res->start >> 28, PCI_IMAP(mem));
>> >> +                     writel(PHYS_OFFSET >> 28, PCI_SMAP(mem));
>> >> +                     mem++;
>> >> +
>> >> +                     break;
>> >> +             case IORESOURCE_BUS:
>> >> +             default:
>> >> +                     continue;
>> >> +             }
>> >> +
>> >> +             err = devm_request_resource(dev, parent, res);
>> >> +             if (err)
>> >> +                     goto out_release_res;
>> >> +     }
>> >
>> > I wonder if we should also request the physical resource for the I/O space
>> > window to have it show up in /proc/iomem. We are rather inconsistent in this
>> > regard between drivers.
>>
>> It's a little complicated now because we don't have that as a struct
>> resource any more. We could reconstruct it from iobase and the i/o
>> resource size.
>
>
>
>> >> +     pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
>> >> +     pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
>> >> +
>> >> +     bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res);
>> >> +     if (!bus)
>> >> +             return -ENOMEM;
>> >> +
>> >> +     pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
>> >> +     pci_assign_unassigned_bus_resources(bus);
>> >> +
>> >> +     return 0;
>> >
>> > One general question, mainly for Bjorn: pci_scan_root_bus adds all the devices
>> > at the Linux driver level by calling pci_bus_add_devices(), but then we call
>> > pci_fixup_irqs and pci_assign_unassigned_bus_resources after that, which will
>> > change the devices again. Is this how it's meant to work? How do we ensure
>> > that we have the correct irq number and resources by the time we enter the
>> > probe() function of the PCI device driver that gets bound to a device here?
>>
>> I'm a bit puzzled myself. I think that the devices are not probed
>> until after pci_assign_unassigned_bus_resources. It certainly doesn't
>> work without that call. Really, I think of_irq_parse_and_map_pci
>> should be the default if no one else has set the device's irq (and the
>> host has a device node of course).
>>
>> It also seems to be a bit of random set of pci functions that are
>> called here. It would be nice to simplify this chunk of code.
>
> Yes, and we recently had some attempts at creating a better interface posted
> on the mailing list, not sure what the latest status on it is. I think we
> want to end up with a two-stage interface along the lines of:
>
>         /* allocate a pci_host_bridge, scan DT, set default operations, map
>            I/O space if necessary, request all resources ... */
>         phb = pci_host_bridge_create(parent, ..., sizeof(struct my_pci_private));

Humm, I wondered what happened to pci_host_bridge_create and thought
we had abandoned it. It wasn't too clear reading thru the threads. The
host drivers generally still have to walk thru the resources anyway to
setup inbound and outbound windows, so we don't gain too much moving
that out. And the error clean-up gets complicated too.

Rob

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

[Arnd Bergmann]

On Friday 02 January 2015 17:13:19 Rob Herring wrote:
> On Fri, Jan 2, 2015 at 2:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 02 January 2015 12:14:43 Rob Herring wrote:
> >> On Tue, Dec 30, 2014 at 3:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:

> >> > Maybe just return PCIBIOS_BAD_REGISTER_NUMBER in this case?
> >>
> >> Perhaps. We could probably simplify the config space read/write
> >> functions and just provide the PCI core a bus/devfn/offset to config
> >> address translation function. That would not work in all cases, but
> >> propably most that have memory mapped config space.
> >
> > Actually, thinking about this some more, the implementation seems to
> > be "CAM" compliant, and we could share the confic space accessors with
> > the ones from drivers/pci/host/pci-host-generic.c.
> 
> Almost. It uses readl for all size accesses. Yet writes support
> different access sizes. I would guess that the h/w can support byte
> and word reads if writes are supported, but I can't really verify.
> Dword-only sized reads or reads and writes seem to be the main
> variations for config space accesses. There's a few hosts with more
> complex config space access setup, but quite a few only vary in
> address decode.

It was probably just done the current way because it seemed simpler
at the time, but I agree that we can just keep it like this if there
is any chance it's required as a workaround for a hardware glitch.

With your other patch you just posted, it really becomes trivial
to do.

> >> I'm a bit puzzled myself. I think that the devices are not probed
> >> until after pci_assign_unassigned_bus_resources. It certainly doesn't
> >> work without that call. Really, I think of_irq_parse_and_map_pci
> >> should be the default if no one else has set the device's irq (and the
> >> host has a device node of course).
> >>
> >> It also seems to be a bit of random set of pci functions that are
> >> called here. It would be nice to simplify this chunk of code.
> >
> > Yes, and we recently had some attempts at creating a better interface posted
> > on the mailing list, not sure what the latest status on it is. I think we
> > want to end up with a two-stage interface along the lines of:
> >
> >         /* allocate a pci_host_bridge, scan DT, set default operations, map
> >            I/O space if necessary, request all resources ... */
> >         phb = pci_host_bridge_create(parent, ..., sizeof(struct my_pci_private));
> 
> Humm, I wondered what happened to pci_host_bridge_create and thought
> we had abandoned it. It wasn't too clear reading thru the threads. The
> host drivers generally still have to walk thru the resources anyway to
> setup inbound and outbound windows, so we don't gain too much moving
> that out. 
I think the discussion has not ended yet, I'm still in favor of
doing it like that. The current of_pci_get_host_bridge_resources()
function is indeed lacking a bit because it just returns the resources
that are needed for setting up the PCI side, but it doesn't
provide a list of the host side windows that may need to get programmed
into hardware registers on machines without a firmware that has set them
up in advance (i.e. most ARM32 and MIPS32 machines).

We could add another exported function to return those, or we could
find a way to pass those back through the pci_host_bridge pointer
from the common function.

Setting up the PCI side by itself does seem useful to me though, mainly
to prevent host controller drivers from getting it wrong.

> And the error clean-up gets complicated too.

In what way? I would hope that we could come up with a way to make
pci_host_bridge_create() able to roll-back, so it does nothing in
case of an error, and allocate all resources using devm_* so it
all gets undone if probe() fails for another reason.

	Arnd

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

[Bjorn Helgaas]

On Tue, Dec 30, 2014 at 10:58:00PM +0100, Arnd Bergmann wrote:
> On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:
> > +	list_for_each_entry(win, res, list) {
> > +		struct resource *parent, *res = win->res;
> > +
> > +		switch (resource_type(res)) {
> > +		case IORESOURCE_IO:
> > +			parent = &ioport_resource;
> > +			err = pci_remap_iospace(res, iobase);
> > +			if (err) {
> > +				dev_warn(dev, "error %d: failed to map resource %pR\n",
> > +					 err, res);
> > +				continue;
> > +			}
> > +			break;
> > +		case IORESOURCE_MEM:
> > +			parent = &iomem_resource;
> > +			res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> > +
> > +			writel(res->start >> 28, PCI_IMAP(mem));
> > +			writel(PHYS_OFFSET >> 28, PCI_SMAP(mem));
> > +			mem++;
> > +
> > +			break;
> > +		case IORESOURCE_BUS:
> > +		default:
> > +			continue;
> > +		}
> > +
> > +		err = devm_request_resource(dev, parent, res);
> > +		if (err)
> > +			goto out_release_res;
> > +	}
> 
> I wonder if we should also request the physical resource for the I/O space
> window to have it show up in /proc/iomem. We are rather inconsistent in this
> regard between drivers.

I'd like that.  We are inconsistent, but I think it's useful to have this
information in /proc/iomem.  After all, it is physical address space that
we can't use for anything else, so I guess you could argue that it's
actually a bug to omit it.

> > +	pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
> > +	pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
> > +
> > +	bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res);
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > +	pci_assign_unassigned_bus_resources(bus);
> > +
> > +	return 0;
> 
> One general question, mainly for Bjorn: pci_scan_root_bus adds all the devices
> at the Linux driver level by calling pci_bus_add_devices(), but then we call
> pci_fixup_irqs and pci_assign_unassigned_bus_resources after that, which will
> change the devices again. Is this how it's meant to work? How do we ensure
> that we have the correct irq number and resources by the time we enter the
> probe() function of the PCI device driver that gets bound to a device here?

Nope, that isn't how it's meant to work.  After pci_bus_add_devices()
completes, drivers can be already bound to the device, and the PCI core
should keep its mitts off things the driver could be using.  But I think
we've had this problem for a long time, and I haven't looked at it recently
to see how hard it would be to fix.

Bjorn

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

[Bjorn Helgaas]

Sorry for the late reply; maybe this has already been queued up somewhere,
so this might be moot.

I usually capitalize the "PCI" and first letter of the subject, like:

  PCI: Add DT based ARM Versatile PCI host driver

And I try to ask for MAINTAINER updates since these drivers are under
drivers/pci, but I can't maintain them all myself.

On Tue, Dec 30, 2014 at 01:28:33PM -0600, Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> This converts the Versatile PCI host code to a platform driver using
> the commom DT parsing and setup. The driver uses only an empty ARM
> pci_sys_data struct and does not use pci_common_init_dev init function.
> The old host code will be removed in a subsequent commit when Versatile
> is completely converted to DT.
> 
> I've tested this on QEMU with the sym53c8xx driver in both i/o and
> memory mapped modes.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>

Looks fine to me.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> +++ b/drivers/pci/host/pci-versatile.c
> ...

> +	/*
> +	 *  We need to discover the PCI core first to configure itself
> +	 *  before the main PCI probing is performed

Unusual to have two spaces between the "*" and the comments here.

> +MODULE_LICENSE("GPLv2");

This needs to be "GPL v2" per license_is_gpl_compatible().