On Tue, Dec 30, 2014 at 3:58 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Tuesday 30 December 2014 13:28:33 Rob Herring wrote:
>> From: Rob Herring <robh@xxxxxxxxxx>
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
