Dear Andrew Murray,

On Thu, 7 Feb 2013 15:51:17 +0000, Andrew Murray wrote:

quoted

First of all, I didn't reimplement the pcibios_get_phb_of_node(), but
instead, as Thierry Reding suggested, simply implemented the
hw_pci.scan() function as follows:

I've not had any time to test Thierry's solution to avoid implementing
pcibios_get_phb_of_node - but it did seem to work for him and seem correct
at the time.

At least there are device tree nodes associated to PCI devices, so it
looks good from this perspective.

quoted

To me, the "struct pci_dev" representing the real PCIe devices should
have a NULL "struct device_node" pointer, because those device are not
represented in the DT. If this was the case, then the of_irq_map_pci()
would go one level up in the PCI hierarchy, find the "struct pci_dev"
that corresponds to the PCI-to-PCI bridge, which generates an laddr[]
having a bus number a devfn value matching the interrupt-map property.

Yes this is my current understanding.

Ok. But that's not what happens: the "struct pci_dev" representing the
real PCIe device *DOES* have a non-NULL struct device_node pointer. And
this is what makes the entire thing fail.

I would suggest the issue isn't in the PCI/DT code. This is what I see

I believe it is, because as I said above, the struct pci_dev associated
to a real PCIe device should not have a struct device_node pointer,
because this PCIe device has been dynamically enumerated and is
therefore not part of the device tree.

with my implementation (which uses an implementation of
pcibios_get_phb_of_node) - I'd like to believe this is correct behaviour:

- of_irq_map_pci is called for an endpoint (5:0:0), its pdev->dev.of_node
  is NULL. As I don't have a representation of this endpoint in my DT the
  of_irq_map_pci code proceeds to walk the fabric.

I believe this should be the behavior. But this not what happens: the
pdev->dev.of_node of an endpoint pci_dev is not NULL.

- Starting with the pdev for 5:0:0, of_irq_map_pci sees it has a parent
  (downstream switch bridge), in this case pdev(5:0:0)->bus->self->dev.of_node
  is NULL, due to this swizzling occurs and we walk the parent's bus (bus 2)
- This continues to bus 1 and then bus 0 where of_irq_map_pci realises that
  it has no parent (its the host bridge). Due to the implementation of
  pcibios_get_phb_of_node a of_node is produced, this is then used to construct
  a lspec (for bus number 0).

The of_irq_map_pci code stops walking as soon as it finds a function in the
tree that has a device node. This suggests that if you represent a bridge
you must also include an interrupt-map. The problem here is that you have
represented a bridge but not included a map.

I understood that it walks up the PCI hierarchy, and that's fine. As
I've shown in my previous e-mail, the only problem is that this
pdev->dev.of_node should be NULL for the PCIe endpoint device. If it
were NULL, then everything would work correctly, as I could confirmed
by the hack I did in of_irq_map_pci().

I can think of three solutions:

1. Something like this:

                 } else {
                         /* We found a P2P bridge, check if it has a node */
                         ppnode = pci_device_to_OF_node(ppdev);
 +                       if (ppnode doesnt have an interrupt-map)//if (pdev->bus->number != 0)
 +                               ppnode = NULL;
                 }

2. Remove the bridges from the DT? Or remove the map from pcie-controller and
   add a map each to pcie at 0,1 and pcie at 1,1?

3. Change the mask of your map so that it doesn't care about bus numbers. I
   have a map that looks like this:

                interrupt-map-mask = <0 0 0 7>;
                interrupt-map = <0 0 0 1 ... //anything coming in on INTA
                                 0 0 0 2 ... //anything coming in on INTB
                                 0 0 0 3 ... ...
                                 0 0 0 4 ... ...

Unfortunately, I don't quite agree with any of your three solutions. I
still do believe the root problem is that pdev->dev.of_node should be
NULL for the PCIe endpoints, since those devices are not probed with
the Device Tree.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com