Re: [PATCH 07/11] x86/dtb: add support for PCI devices backed by dtb nodes

[Sebastian Andrzej Siewior]

* Benjamin Herrenschmidt | 2010-11-28 09:33:51 [+1100]:

>That's something we need to eventually put into common code, ie matching
>device nodes to PCI devices... In the meantime, your approach will do,
>some nits:
>
>> +static int of_irq_map_pci(struct pci_dev *dev, struct of_irq *oirq)
>> +{
>> +	struct device_node *node;
>> +	__be32 laddr[3];
>> +	__be32 lspec[2];
>> +	int ret;
>> +	u8 pin;
>> +
>> +	node = dev->dev.of_node;
>> +	if (!node) {
>> +		node = dev->bus->dev.of_node;
>> +		if (node) {
>> +			ret = of_irq_map_one(node, 0, oirq);
>> +			if (!ret)
>> +				return ret;
>> +		}
>> +	}
>
>I don't quite get the logic in getting to the bus' interrupts if you
>can't find a device own nodes here...
the of_irq_map_one() is here in case the device has an interrupt node in
the device tree. If not, looks it up in the device tree based on the pin
and the device address.

>> +	ret = pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
>> +	if (ret)
>> +		return ret;
>> +	if (!pin)
>> +		return -EINVAL;
>> +
>> +	laddr[0] = cpu_to_be32((dev->bus->number << 16) | (dev->devfn << 8));
>> +	laddr[1] = 0;
>> +	laddr[2] = 0;
>> +
>> +	lspec[0] = cpu_to_be32(pin);
>> +	lspec[1] = cpu_to_be32(0);
>> +	ret = of_irq_map_raw(node, lspec, 1, laddr, oirq);
>> +	if (ret)
>> +		return ret;
>> +	return 0;
>> +}
>
>Ok so I see what you are trying to do, but I think it's not completely
>correct, besides you miss the swizzling when crossing P2P bridges and
>similar.
It should be correct. I did not understand the P2P bridge so I left it
out.

>I suppose you looked at powerpc's of_irq_map_pci() so I'm not sure why
>you modified it the way you did :-) You should probably either move it
>to a generic place or copy it for now with a comment indicating where it
>comes from so we spot it when we put it into a common code.
Microblaze had its own copy of this code so I though there is something
specific about it. If it is okay with you, I would move it to drivers/of
and share. Then I would have the swizzle part :)

>> +void __cpuinit x86_of_pci_init(void)
>> +{
>> +	struct device_node *np;
>> +
>> +	pcibios_enable_irq = x86_of_pci_irq_enable;
>> +	pcibios_disable_irq = x86_of_pci_irq_disable;
>> +
>> +	for_each_node_by_type(np, "pci") {
>> +		const void *prop;
>> +		struct pci_bus *bus;
>> +		unsigned int bus_min;
>> +		struct device_node *child;
>> +
>> +		prop = of_get_property(np, "bus-range", NULL);
>> +		if (!prop)
>> +			continue;
>> +		bus_min = be32_to_cpup(prop);
>> +
>> +		bus = pci_find_bus(0, bus_min);
>> +		if (!bus) {
>> +			printk(KERN_ERR "Can't find a node for bus %s.\n",
>> +					np->full_name);
>> +			continue;
>> +		}
>> +
>> +		bus->dev.of_node = np;
>> +		for_each_child_of_node(np, child) {
>> +			struct pci_dev *dev;
>> +			u32 devfn;
>> +
>> +			prop = of_get_property(child, "reg", NULL);
>> +			if (!prop)
>> +				continue;
>> +
>> +			devfn = (be32_to_cpup(prop) >> 8) & 0xff;
>> +			dev = pci_get_slot(bus, devfn);
>> +			if (!dev)
>> +				continue;
>> +			dev->dev.of_node = child;
>> +		}
>> +	}
>> +}
>
>That too won't go down bridges, atom never have any ? (no PCIe root
>complex at all ? ever will be ? even then, it should be supported as got
>knows what we'll handle in the future).
The cpu I have here for testing does not have any.

>Eventually we want that matching between PCI devices and OF nodes to be
>in generic code, so that's not a big deal to have an "inferior" version
>temporarily in there I suppose.

Inferior, I see :)

>Also, aren't you missing a pci_dev_put() after pci_get_slot() ?
probably. I will check.

>Cheers,
>Ben.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/