ARM Versatile multi-platform support

From: Rob Herring <[email protected]>

Disable the Versatile PCI DT node when no PCI backplane is detected. This will prevent the Versatile PCI driver from probing when PCI is not populated.

Quoted 5 lines · On Tuesday 30 December 2014 13:28:32 Rob Herring wrote:

From: Rob Herring <[email protected]>

Disable the Versatile PCI DT node when no PCI backplane is detected. This will prevent the Versatile PCI driver from probing when PCI is not populated.

Can you explain why the check is done in platform code instead of the PCI driver probe function itself?

arch/arm/mach-versatile/versatile_dt.c line 56

Quoted 4 lines

arch/arm/mach-versatile/versatile_dt.c +4 -0

+     /* Check if PCI backplane is detected */
+     val = __raw_readl(base + VERSATILE_SYS_PCICTL_OFFSET);
+     if (val & 1)
+             goto err;

Using __raw_readl prevents this from working with big-endian kernels, so just use readl here.

Quoted 9 lines · Arnd Bergmann

Quoted 5 lines · On Tuesday 30 December 2014 13:28:32 Rob Herring wrote:

From: Rob Herring <[email protected]>

Disable the Versatile PCI DT node when no PCI backplane is detected. This will prevent the Versatile PCI driver from probing when PCI is not populated.

Can you explain why the check is done in platform code instead of the PCI driver probe function itself?

Because the register to check is not part of the PCI controller, but part of the system registers. There was no infrastructure for the system registers when I originally wrote this, but there's some syscon support now for VExpress that may be able to be used. I think it is cleaner if we can avoid syscon dependencies in drivers in general, but it is pretty much zero chance the Versatile PCI driver will ever be used on another platform.

Quoted 7 lines

arch/arm/mach-versatile/versatile_dt.c line 56

Quoted 4 lines

arch/arm/mach-versatile/versatile_dt.c +4 -0

+     /* Check if PCI backplane is detected */
+     val = __raw_readl(base + VERSATILE_SYS_PCICTL_OFFSET);
+     if (val & 1)
+             goto err;

Using __raw_readl prevents this from working with big-endian kernels, so just use readl here.

So does the fact that this is an ARM926, but yes there's not really any reason not to use readl.

Quoted 7 lines · On Tuesday 30 December 2014 17:05:16 Rob Herring wrote:

Because the register to check is not part of the PCI controller, but part of the system registers. There was no infrastructure for the system registers when I originally wrote this, but there's some syscon support now for VExpress that may be able to be used. I think it is cleaner if we can avoid syscon dependencies in drivers in general, but it is pretty much zero chance the Versatile PCI driver will ever be used on another platform.

Ok, I see. I guess ideally we would get a correct DTB file from the boot loader, but that is of course completely unrealistic on versatile, both on hardware and qemu.

I suspect that either the existing behavior is completely bogus because the driver always reads what it write before, and then you can skip the logic completely, or your new driver is broken because reading the register without writing it first will result in undefined behavior on real hardware.

Quoted 9 lines · Arnd Bergmann

Quoted 7 lines · On Tuesday 30 December 2014 17:05:16 Rob Herring wrote:

Because the register to check is not part of the PCI controller, but part of the system registers. There was no infrastructure for the system registers when I originally wrote this, but there's some syscon support now for VExpress that may be able to be used. I think it is cleaner if we can avoid syscon dependencies in drivers in general, but it is pretty much zero chance the Versatile PCI driver will ever be used on another platform.

Quoted 3 lines

Ok, I see. I guess ideally we would get a correct DTB file from the boot loader, but that is of course completely unrealistic on versatile, both on hardware and qemu.

Why would you want to get the bootloader to do this for you when the hardware provides a perfectly good mechanism for probing for the existence of the hardware? "Ask the hardware" is always going to be a more reliable and foolproof way to get the answer if you can do it -- device tree should just be for the cases where there is no way to probe. In any case the only way for the boot loader to determine the correct value to put into the device tree would be to read the register itself.

Quoted 5 lines

I suspect that either the existing behavior is completely bogus because the driver always reads what it write before, and then you can skip the logic completely, or your new driver is broken because reading the register without writing it first will result in undefined behavior on real hardware.

The documentation describes this register as:

# The SYS_PCICTL register at 0x10000044 enables the bridge to the PCI bus:
#
# * Setting bit 0 HIGH enables PCI bus accesses.
# * Read returns a HIGH in bit 0 if a PCI board is present
# in the expansion backplane.

ie the read and write behaviour is independent. So I suspect that this new driver's failure to write 1 means that it will break PCI on real hardware. (QEMU doesn't attempt to model this behaviour: SYS_PCICTL will always read-as-one, writes-ignored.)

From Table 4.4, I came to the conclusion that the write wasn't really necessary:

Quoted 8 lines · Peter Maydell

Why would you want to get the bootloader to do this for you when the hardware provides a perfectly good mechanism for probing for the existence of the hardware? "Ask the hardware" is always going to be a more reliable and foolproof way to get the answer if you can do it -- device tree should just be for the cases where there is no way to probe. In any case the only way for the boot loader to determine the correct value to put into the device tree would be to read the register itself.

Only that it is nice to hide all the ugly bits in the bootloader/firmware and DT already provides a standard way to enable or disable h/w. This h/w is not probeable in any sort of standard way.

From Table 4.4, I came to the conclusion that the write wasn't really necessary:

SYS_PCICTL 0x10000044 Read only -Read returns a HIGH in bit [0] if a PCI board is present in the expansion backplane.

I'll add it back.

Quoted 11 lines · Rob Herring

Quoted 8 lines · Peter Maydell

Why would you want to get the bootloader to do this for you when the hardware provides a perfectly good mechanism for probing for the existence of the hardware? "Ask the hardware" is always going to be a more reliable and foolproof way to get the answer if you can do it -- device tree should just be for the cases where there is no way to probe. In any case the only way for the boot loader to determine the correct value to put into the device tree would be to read the register itself.

Quoted 3 lines

Only that it is nice to hide all the ugly bits in the bootloader/firmware and DT already provides a standard way to enable or disable h/w. This h/w is not probeable in any sort of standard way.

This is an embedded system, you were expecting some kind of standard?

Quoted 6 lines

From Table 4.4, I came to the conclusion that the write wasn't really necessary:

SYS_PCICTL 0x10000044 Read only -Read returns a HIGH in bit [0] if a PCI board is present in the expansion backplane.

I'll add it back.

The self-contradictory VersatilePB docs strike again, I see. Interestingly, in the PB1176 docs this register is documented as definitely read/only:

# [1]Read only: Reserved.
#
# Note
#
# On the RealView Platform Baseboard for ARM926EJ-S, this bit

# was PCICONTROLIN signal and indicated that 66MHz mode was
# enabled. The PB1176JZF-S only supports 33MHz operation.
#
# [0]Read only: Status of P_DETECT pin.

...and in passing provides some info about the 926 board that isn't in the docs for the 926!

A little digging suggests that it may in fact be the case that the 1176 docs are correct and the write to bit 0 does nothing. However the safest thing here is to keep doing what the old kernel did, I think.

But that is exactly the point: While we try to ask the hardware about anything that can be reasonably queried through an in-kernel interface that talks to the hardware, it works less well when that hardware interface is rather obscure and (like this one) only used on a few special machines.

Similar examples are Marvell's SoC ID register, which you have to read by powering on the PCI host bridge and reading the Vendor/Device ID registers for the root bus, or the 'nvram' on Broadcom's wifi router chips that is in reality a partition on either NOR or NAND flash. Accessing those at early boot from a random device driver is a real pain, and it would be much nicer to have the boot loader read them and put the data into the DT for us if we had the option. Unfortunately we have to deal with legacy boot loaders that don't change the DTB.

From: Rob Herring <[email protected]>

Disable the Versatile PCI DT node when no PCI backplane is detected. This will prevent the Versatile PCI driver from probing when PCI is not populated.

Signed-off-by Rob Herring <[email protected]>

Cc Russell King <[email protected]>

Cc Linus Walleij <[email protected]>

Quoted 9 lines · Rob Herring

From: Rob Herring <[email protected]>

Disable the Versatile PCI DT node when no PCI backplane is detected. This will prevent the Versatile PCI driver from probing when PCI is not populated.

Signed-off-by Rob Herring <[email protected]>

Cc Russell King <[email protected]>

Cc Linus Walleij <[email protected]>

(...)

arch/arm/mach-versatile/versatile_dt.c line 54

Quoted 2 lines

arch/arm/mach-versatile/versatile_dt.c +2 -0

+       /* Check if PCI backplane is detected */
+       val = __raw_readl(base + VERSATILE_SYS_PCICTL_OFFSET);

I think this kind of random syscon register access should be handled using the mfd/syscon.c hub and looked up in some way to be used by the driver.

I'm thinking along the pattern of adding code in drivers/* by the pattern of e.g. drivers/video/fbdev/amba-clcd-versatile.c
i.e. an add-on that gets compiled-in only for those platform but still married to the main driver.

From: Rob Herring <[email protected]>

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.

drivers/pci/host/pci-versatile.c new file +33 -0

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

Quoted 10 lines · On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:

From: Rob Herring <[email protected]>

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.

drivers/pci/host/pci-versatile.c line 64

Quoted 12 lines

drivers/pci/host/pci-versatile.c new file +12 -0

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

Quoted 39 lines

drivers/pci/host/pci-versatile.c new file +39 -0

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

drivers/pci/host/pci-versatile.c line 181

Quoted 4 lines

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

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

Quoted 11 lines

drivers/pci/host/pci-versatile.c new file +10 -0

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

Quoted 1 line

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

Quoted 15 lines · Arnd Bergmann

Quoted 10 lines · On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:

From: Rob Herring <[email protected]>

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.

Quoted 15 lines

drivers/pci/host/pci-versatile.c line 64

Quoted 12 lines

drivers/pci/host/pci-versatile.c new file +12 -0

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

Quoted 1 line

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.

Quoted 39 lines

Quoted 39 lines

drivers/pci/host/pci-versatile.c new file +39 -0

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

Quoted 8 lines

drivers/pci/host/pci-versatile.c line 181

Quoted 4 lines

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

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

Quoted 10 lines

Quoted 10 lines

drivers/pci/host/pci-versatile.c new file +10 -0

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

Quoted 8 lines

Quoted 1 line

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

Quoted 32 lines · On Friday 02 January 2015 12:14:43 Rob Herring wrote:

Quoted 30 lines · Arnd Bergmann

Quoted 10 lines · On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:

From: Rob Herring <[email protected]>

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.

drivers/pci/host/pci-versatile.c line 64

Quoted 12 lines

drivers/pci/host/pci-versatile.c new file +12 -0

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

Quoted 2 lines

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 73 lines

Quoted 39 lines

Quoted 39 lines

drivers/pci/host/pci-versatile.c new file +39 -0

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

Quoted 8 lines

drivers/pci/host/pci-versatile.c line 181

Quoted 4 lines

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

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

Quoted 10 lines

Quoted 10 lines

drivers/pci/host/pci-versatile.c new file +10 -0

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

Quoted 8 lines

Quoted 1 line

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

Quoted 8 lines

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

Quoted 6 lines · Arnd Bergmann

Quoted 5 lines · On Friday 02 January 2015 12:14:43 Rob Herring wrote:

Quoted 3 lines · Arnd Bergmann

On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:

Maybe just return PCIBIOS_BAD_REGISTER_NUMBER in this case?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 9 lines

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 27 lines · On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:

drivers/pci/host/pci-versatile.c new file +27 -0

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

drivers/pci/host/pci-versatile.c line 181

Quoted 4 lines

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

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

Quoted 10 lines

drivers/pci/host/pci-versatile.c new file +10 -0

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

Review

Quoted 1 line

(quoted diff) +1 -0

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

From: Rob Herring <[email protected]>

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

Cc Bjorn Helgaas <[email protected]>

Cc linux-pci at vger.kernel.org

Cc Russell King <[email protected]>

Cc Linus Walleij <[email protected]>

Cc Peter Maydell <[email protected]>

+MODULE_LICENSE("GPLv2");

From: Rob Herring <[email protected]>

The Versatile boards have the same sysregs as other ARM Ltd boards. Add the nodes in preparation to enable support for 24MHz counter as sched_clock and MMC card detect and write protect support.

Signed-off-by Rob Herring <[email protected]>

Cc Russell King <[email protected]>

Cc Linus Walleij <[email protected]>

---
 arch/arm/boot/dts/versatile-ab.dts | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

arch/arm/boot/dts/versatile-ab.dts +0 -0

index 27d0d9c..62f04b0 100644

Quoted 21 lines · Rob Herring

From: Rob Herring <[email protected]>

The Versatile boards have the same sysregs as other ARM Ltd boards. Add the nodes in preparation to enable support for 24MHz counter as sched_clock and MMC card detect and write protect support.

Signed-off-by Rob Herring <[email protected]>

Cc Russell King <[email protected]>

Cc Linus Walleij <[email protected]>

---
 arch/arm/boot/dts/versatile-ab.dts | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

arch/arm/boot/dts/versatile-ab.dts +0 -0

index 27d0d9c..62f04b0 100644
@@ -252,6 +252,29 @@
                        #size-cells = <1>;
                        ranges = <0 0x10000000 0x10000>;
 

arch/arm/boot/dts/versatile-ab.dts line 256

Quoted 2 lines

arch/arm/boot/dts/versatile-ab.dts +2 -0

+                       sysreg at 0 {
+                               compatible = "arm,vexpress-sysreg";

vexpress? No...

compatible = "syscon";

arch/arm/boot/dts/versatile-ab.dts line 263

Quoted 7 lines

arch/arm/boot/dts/versatile-ab.dts +7 -0

+                               reg = <0x00000 0x1000>;
+
+                               v2m_led_gpios: sys_led at 08 {
+                                       compatible = "arm,vexpress-sysreg,sys_led";
+                                       gpio-controller;
+                                       #gpio-cells = <2>;
+                               };

These are not GPIOs. These are LED registers really.

see how to use LEDs from drivers/leds/leds-syscon.c and bindings. example in:
arch/arm/boot/dts/integrator.dtsi

Very straight-forward I think.

arch/arm/boot/dts/versatile-ab.dts line 275

Quoted 11 lines

arch/arm/boot/dts/versatile-ab.dts +11 -0

+                               v2m_mmc_gpios: sys_mci at 48 {
+                                       compatible = "arm,vexpress-sysreg,sys_mci";
+                                       gpio-controller;
+                                       #gpio-cells = <2>;
+                               };
+
+                               v2m_flash_gpios: sys_flash at 4c {
+                                       compatible = "arm,vexpress-sysreg,sys_flash";
+                                       gpio-controller;
+                                       #gpio-cells = <2>;
+                               };

I don't have drivers for these "gpio controllers" and I don't think they are GPIOs either, since they are not general purpose at all.

For the latter I have some code boiling for the Integrators:
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-integrator.git/commit/?h=multiplatform&id=9c28e114ec957fadf1dade382d8e8b8aa43c8426
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-integrator.git/commit/?h=multiplatform&id=a3d4512abb85de290cdca0586bcff9147833917f

Yours,
Linus Walleij

Quoted 2 lines · Linus Walleij

On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <[email protected]> wrote:
compatible = "syscon";

maybe? VExpress is missing that then...

Quoted 9 lines

arch/arm/boot/dts/versatile-ab.dts line 263

Quoted 7 lines

arch/arm/boot/dts/versatile-ab.dts +7 -0

+                               reg = <0x00000 0x1000>;
+
+                               v2m_led_gpios: sys_led at 08 {
+                                       compatible = "arm,vexpress-sysreg,sys_led";
+                                       gpio-controller;
+                                       #gpio-cells = <2>;
+                               };

These are not GPIOs. These are LED registers really.

A register bit that controls an i/o signal sounds like a GPIO to me.

Quoted 5 lines

see how to use LEDs from drivers/leds/leds-syscon.c and bindings. example in:
arch/arm/boot/dts/integrator.dtsi

Very straight-forward I think.

So we have 2 implementations and bindings for roughly the same hardware? Great!

Quoted 6 lines · Rob Herring

Quoted 2 lines · Linus Walleij

On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <[email protected]> wrote:
compatible = "syscon";

maybe? VExpress is missing that then...

I don't like the way some vexpress stuff has been done I think, just not enough reviewing power :(

I've tried to make an as clean separation as possible in the Integrator as it has been refactored with a minimum of time pressure and I tried to make it as reusable as possible. But it doesn't necessarily mean I did the right thing all the time ...

Quoted 11 lines

Quoted 9 lines

arch/arm/boot/dts/versatile-ab.dts line 263

Quoted 7 lines

arch/arm/boot/dts/versatile-ab.dts +7 -0

+                               reg = <0x00000 0x1000>;
+
+                               v2m_led_gpios: sys_led at 08 {
+                                       compatible = "arm,vexpress-sysreg,sys_led";
+                                       gpio-controller;
+                                       #gpio-cells = <2>;
+                               };

These are not GPIOs. These are LED registers really.

A register bit that controls an i/o signal sounds like a GPIO to me.

Are they described as general purpose in the manual for the board?

In the ARM reference design manuals I've seen these bits are described as for one purpose only. I mean you can claim the memory RE signal is "a bit that controls an I/O signal" as well, but we have to think about the abstraction here.

Adding VExpress maintainers...

Quoted 23 lines · Linus Walleij

Quoted 21 lines · Rob Herring

From: Rob Herring <[email protected]>

The Versatile boards have the same sysregs as other ARM Ltd boards. Add the nodes in preparation to enable support for 24MHz counter as sched_clock and MMC card detect and write protect support.

Signed-off-by Rob Herring <[email protected]>

Cc Russell King <[email protected]>

Cc Linus Walleij <[email protected]>

---
 arch/arm/boot/dts/versatile-ab.dts | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

arch/arm/boot/dts/versatile-ab.dts +0 -0

index 27d0d9c..62f04b0 100644
@@ -252,6 +252,29 @@
                        #size-cells = <1>;
                        ranges = <0 0x10000000 0x10000>;
 

Quoted 4 lines

arch/arm/boot/dts/versatile-ab.dts line 256

Quoted 2 lines

arch/arm/boot/dts/versatile-ab.dts +2 -0

+                       sysreg at 0 {
+                               compatible = "arm,vexpress-sysreg";

vexpress? No...

Compatible with yes. Should perhaps be '"arm,versatile-sysreg", "arm,vexpress-sysreg"' instead. Kind of backwards, as really the versatile came first, but it would work. Or we just need another match entry in the kernel.

I copied this whole chunk as is from VExpress and verified these sub nodes are all the same hence why it is here.

Quoted 1 line

compatible = "syscon";

maybe? VExpress is missing that then...

Quoted 9 lines

Quoted 8 lines

arch/arm/boot/dts/versatile-ab.dts +7 -0

+                               reg = <0x00000 0x1000>;
+
+                               v2m_led_gpios: sys_led at 08 {
+                                       compatible = "arm,vexpress-sysreg,sys_led";
+                                       gpio-controller;
+                                       #gpio-cells = <2>;
+                               };

Quoted 1 line

These are not GPIOs. These are LED registers really.

A register bit that controls an i/o signal sounds like a GPIO to me.

Quoted 5 lines

see how to use LEDs from drivers/leds/leds-syscon.c and bindings. example in:
arch/arm/boot/dts/integrator.dtsi

Very straight-forward I think.

So we have 2 implementations and bindings for roughly the same hardware? Great!

Quoted 12 lines

arch/arm/boot/dts/versatile-ab.dts line 275

Quoted 11 lines

arch/arm/boot/dts/versatile-ab.dts +11 -0

+                               v2m_mmc_gpios: sys_mci at 48 {
+                                       compatible = "arm,vexpress-sysreg,sys_mci";
+                                       gpio-controller;
+                                       #gpio-cells = <2>;
+                               };
+
+                               v2m_flash_gpios: sys_flash at 4c {
+                                       compatible = "arm,vexpress-sysreg,sys_flash";
+                                       gpio-controller;
+                                       #gpio-cells = <2>;
+                               };

Quoted 2 lines

I don't have drivers for these "gpio controllers" and I don't think they are GPIOs either, since they are not general purpose at all.

They are only general purpose until you connect the i/o lines to something. But yes, if we went around making every misc internal control line in SOCs a 1-bit GPIO controller that would be pretty crazy.

Anyway, most of this is not actually used ATM on Versatile, but it is present in VExpress. I added it only for sched_clock. We need to resolve this with VExpress platforms, but it's already in use for MMC and LEDs.

Quoted 11 lines · Rob Herring

Quoted 10 lines · Linus Walleij

Quoted 7 lines · Rob Herring

arch/arm/boot/dts/versatile-ab.dts +7 -0

+                               reg = <0x00000 0x1000>;
+
+                               v2m_led_gpios: sys_led at 08 {
+                                       compatible = "arm,vexpress-sysreg,sys_led";
+                                       gpio-controller;
+                                       #gpio-cells = <2>;
+                               };

Quoted 3 lines

Quoted 1 line

These are not GPIOs. These are LED registers really.

A register bit that controls an i/o signal sounds like a GPIO to me.

To me too. I agree that definining a gpio-controller for every possible gpio pin would soon get unwieldy, but hey, the choice made for vexpress leds makes perfect sense to me, after all they are gpio signals connected to leds, and there is a driver for that in the kernel:

drivers/leds/leds-gpio.c

we could move this stuff to syscon-leds, but honestly I think is one of those things we could argue forever about that.

From: Rob Herring <[email protected]>

In preparation for multi-platform support, move platform.h and hardware.h into mach-versatile directory. Combine them into a single file in the process.

Signed-off-by Rob Herring <[email protected]>

Cc Russell King <[email protected]>

Cc Linus Walleij <[email protected]>

---
 arch/arm/mach-versatile/core.c                     |  3 +-
 arch/arm/mach-versatile/include/mach/hardware.h    | 32 ----------------------
 .../mach-versatile/{include/mach => }/platform.h   |  5 ++++
 3 files changed, 6 insertions(+), 34 deletions(-)

From: Rob Herring <[email protected]>

This series converts ARM Versatile platform to multi-platform. I started this some time ago and some pieces were already merged. The primary piece remaining is converting the PCI host to DT which I was waiting for the common PCI DT parsing to get settled. Now that that is in place as well as a few other pieces are in place like multi-platform fixes for CLCD, we can fully convert Versatile to DT and multi-platform.

This has been tested on QEMU. A previous version was tested on Versatile/AB by Linus Walleij. Linus reported a problem with the display panel detection. Investigation on h/w is needed still as I have not been able to identify the issue by inspection and the display detection seems to work correctly on QEMU.

Linus had also mentioned to me that QEMU users may have some issue with removing non-DT code as the work flow has to change somewhat.

Copying
Peter Maydell here for comment (and there's rumors he may have actual PB h/w with PCI as well).

Quoted 8 lines · Rob Herring

This has been tested on QEMU. A previous version was tested on Versatile/AB by Linus Walleij. Linus reported a problem with the display panel detection. Investigation on h/w is needed still as I have not been able to identify the issue by inspection and the display detection seems to work correctly on QEMU.

Linus had also mentioned to me that QEMU users may have some issue with removing non-DT code as the work flow has to change somewhat.

As long as you call out the requirement for a DT now in whatever your equivalent of release notes is that should be fine. (Does the kernel provide a useful message to serial in the absence of a DT or does it now not have enough platform knowledge to do that?)

Quoted 3 lines

Copying
Peter Maydell here for comment (and there's rumors he may have actual PB h/w with PCI as well).

Will Deacon and Marc Zyngier are currently in possession of that h/w. I'm sure they'd be delighted to pull it out of the cupboard and give it a spin :-)

Quoted 3 lines · Peter Maydell

Will Deacon and Marc Zyngier are currently in possession of that h/w. I'm sure they'd be delighted to pull it out of the cupboard and give it a spin :-)

I'll try to find some time (or a victim to do the testing... ;-)

Quoted 6 lines · Marc Zyngier

Quoted 3 lines · Peter Maydell

Will Deacon and Marc Zyngier are currently in possession of that h/w. I'm sure they'd be delighted to pull it out of the cupboard and give it a spin :-)

I'll try to find some time (or a victim to do the testing... ;-)

To save you some time in trying to debug things which didn't work before this patchset, the status last time I tested on h/w was:
* PCI MMIO accesses work
* PCI IO accesses work
* PCI bus-master (DMA) doesn't work

with the failure symptom for DMA being that the card wrote to the correct area of memory but only every other word, so you got strings like "HellXXXXrld" with the Xs being garbage. This feels to me like a h/w issue rather than a driver problem.

Quoted 14 lines · Peter Maydell

Quoted 6 lines · Marc Zyngier

Quoted 3 lines · Peter Maydell

Will Deacon and Marc Zyngier are currently in possession of that h/w. I'm sure they'd be delighted to pull it out of the cupboard and give it a spin :-)

I'll try to find some time (or a victim to do the testing... ;-)

To save you some time in trying to debug things which didn't work before this patchset, the status last time I tested on h/w was:
* PCI MMIO accesses work
* PCI IO accesses work
* PCI bus-master (DMA) doesn't work

Quoted 4 lines

with the failure symptom for DMA being that the card wrote to the correct area of memory but only every other word, so you got strings like "HellXXXXrld" with the Xs being garbage. This feels to me like a h/w issue rather than a driver problem.

Looks like some 64bit address decoding getting in the way. Or something.... Do you remember the exact platform you tried this on (I have PB926, PB1176, and a bunch of RealViews with an odd mix of tiles...)?

From: Rob Herring <[email protected]>

This series converts ARM Versatile platform to multi-platform. I started this some time ago and some pieces were already merged. The primary piece remaining is converting the PCI host to DT which I was waiting for the common PCI DT parsing to get settled. Now that that is in place as well as a few other pieces are in place like multi-platform fixes for CLCD, we can fully convert Versatile to DT and multi-platform.

There's still a few things that need DT support which can be done later:
- MMC card detect and write protect. Should be able to use VExpress support
- Reboot support. Should be able to re-use Realview reboot code.
- flash phys-map support. Binding exists, but specifically Vpp control is needed.
- CLCD support. Not sure where this is at.

Quoted 9 lines · Rob Herring

From: Rob Herring <[email protected]>

This series converts ARM Versatile platform to multi-platform. I started this some time ago and some pieces were already merged. The primary piece remaining is converting the PCI host to DT which I was waiting for the common PCI DT parsing to get settled. Now that that is in place as well as a few other pieces are in place like multi-platform fixes for CLCD, we can fully convert Versatile to DT and multi-platform.

Quoted 8 lines

There's still a few things that need DT support which can be done later:
- MMC card detect and write protect. Should be able to use VExpress support
- Reboot support. Should be able to re-use Realview reboot code.
- flash phys-map support. Binding exists, but specifically Vpp control is needed.
- CLCD support. Not sure where this is at.

I like it overall but would like to see some use of the syscon pattern to access random registers in syscon, and specifically not to refer to these registers as some "gpio" because they aren't.

Quoted 20 lines · Linus Walleij

Quoted 17 lines · Rob Herring

From: Rob Herring <[email protected]>

This series converts ARM Versatile platform to multi-platform. I started this some time ago and some pieces were already merged. The primary piece remaining is converting the PCI host to DT which I was waiting for the common PCI DT parsing to get settled. Now that that is in place as well as a few other pieces are in place like multi-platform fixes for CLCD, we can fully convert Versatile to DT and multi-platform.

There's still a few things that need DT support which can be done later:
- MMC card detect and write protect. Should be able to use VExpress support
- Reboot support. Should be able to re-use Realview reboot code.
- flash phys-map support. Binding exists, but specifically Vpp control is needed.
- CLCD support. Not sure where this is at.

Quoted 3 lines

I like it overall but would like to see some use of the syscon pattern to access random registers in syscon, and specifically not to refer to these registers as some "gpio" because they aren't.

I'm not sure I follow. Creating GPIO lines for SD card detect and write protect is exactly how VExpress syscon was implemented.

From there a LED, card detect and flash protection are modeled as