[PATCH] v3: Fix up PCI device code

[Carl-Daniel Hailfinger]

The current PCI device code calls pci_scan_bus with a PCI domain instead
of a PCI bus as parameter. That causes all sorts of havoc, including
double initialization of hardware and ignoring devices in the dts.

This patch attempts to fix these bugs, but it has other side effects.
Somehow, resource allocation is now skipped.

I'd appreciate logs with and without this patch for every target,
especially K8.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-pci_scan_bus_nexttry_minimal_fixup/device/pci_device.c
===================================================================
--- corebootv3-pci_scan_bus_nexttry_minimal_fixup/device/pci_device.c	(Revision 980)
+++ corebootv3-pci_scan_bus_nexttry_minimal_fixup/device/pci_device.c	(Arbeitskopie)
@@ -1234,8 +1234,24 @@
 unsigned int pci_domain_scan_bus(struct device *dev, unsigned int curr_bus)
 {
    printk(BIOS_SPEW, "pci_domain_scan_bus: calling pci_scan_bus\n");
-	/* There is only one link on this device, and it is always link 0. */
-	return pci_scan_bus(&dev->link[0], PCI_DEVFN(0, 0), 0xff, curr_bus);
+	/* There is only one link on a bus, and it is always link 0.
+	 * dev->link[0] for a PCI domain is the domain link.
+	 * The child of the domain link is the PCI bus device.
+	 * We want to scan the bus link of the PCI bus device.
+	 * dev->link[0].children->link[0] is that PCI bus link.
+	 * If there can be multiple PCI buses directly below a PCI domain,
+	 * we have to iterate over the PCI buses in a loop.
+	 */
+#if 1
+	return pci_scan_bus(&dev->link[0].children->link[0],
+				   PCI_DEVFN(0, 0), 0xff, curr_bus);
+#else
+	struct device *list;
+	for (list = dev->link[0].children; list; list = list->sibling)
+		curr_bus = pci_scan_bus(&list->link[0],
+				   PCI_DEVFN(0, 0), 0xff, curr_bus);
+	return curr_bus;
+#endif
 }

/**

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Carl-Daniel Hailfinger
> Sent: Tuesday, November 04, 2008 8:02 PM
> To: Coreboot
> Subject: [coreboot] [PATCH] v3: Fix up PCI device code
>
> The current PCI device code calls pci_scan_bus with a PCI domain instead
> of a PCI bus as parameter. That causes all sorts of havoc, including
> double initialization of hardware and ignoring devices in the dts.
>
> This patch attempts to fix these bugs, but it has other side effects.
> Somehow, resource allocation is now skipped.
>
> I'd appreciate logs with and without this patch for every target,
> especially K8.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: corebootv3-pci_scan_bus_nexttry_minimal_fixup/device/pci_device.c
>
> --- corebootv3-pci_scan_bus_nexttry_minimal_fixup/device/pci_device.c
>    (Revision 980)
> +++ corebootv3-pci_scan_bus_nexttry_minimal_fixup/device/pci_device.c
>    (Arbeitskopie)
> @@ -1234,8 +1234,24 @@
>  unsigned int pci_domain_scan_bus(struct device *dev, unsigned int
> curr_bus)
>  {
>    printk(BIOS_SPEW, "pci_domain_scan_bus: calling pci_scan_bus\n");
> /* There is only one link on this device, and it is always link 0.
>
> */
> return pci_scan_bus(&dev->link[0], PCI_DEVFN(0, 0), 0xff, curr_bus);
> /* There is only one link on a bus, and it is always link 0.
> * dev->link[0] for a PCI domain is the domain link.
> * The child of the domain link is the PCI bus device.

This is where it gets a little strange.  The domain has lots of children,
but none of them are a "pci bus device."  If you look at Stefan's old pngs
from v2, there were a lot more devices and children than there are in v3 so
far.  I think looking at the output of show_all_devs makes this clear.  Any
log that's been posted to the link will have that.

I'm still trying to understand the pci enumeration, which is why I was
cleaning up the pci_device code.

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 05.11.2008 04:12, Myles Watson wrote:
> > -----Original Message-----
> > From: [e-mail] [mailto:[e-mail]]
> > On Behalf Of Carl-Daniel Hailfinger
> > Sent: Tuesday, November 04, 2008 8:02 PM
> > To: Coreboot
> > Subject: [coreboot] [PATCH] v3: Fix up PCI device code
> >
> > The current PCI device code calls pci_scan_bus with a PCI domain instead
> > of a PCI bus as parameter. That causes all sorts of havoc, including
> > double initialization of hardware and ignoring devices in the dts.
> >
> > This patch attempts to fix these bugs, but it has other side effects.
> > Somehow, resource allocation is now skipped.
> >
> > I'd appreciate logs with and without this patch for every target,
> > especially K8.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: corebootv3-pci_scan_bus_nexttry_minimal_fixup/device/pci_device.c
> >
> > --- corebootv3-pci_scan_bus_nexttry_minimal_fixup/device/pci_device.c
> >    (Revision 980)
> > +++ corebootv3-pci_scan_bus_nexttry_minimal_fixup/device/pci_device.c
> >    (Arbeitskopie)
> > @@ -1234,8 +1234,24 @@
> >  unsigned int pci_domain_scan_bus(struct device *dev, unsigned int
> > curr_bus)
> >  {
> >    printk(BIOS_SPEW, "pci_domain_scan_bus: calling pci_scan_bus\n");
> > /* There is only one link on this device, and it is always link 0.
> >
> > */
> > return pci_scan_bus(&dev->link[0], PCI_DEVFN(0, 0), 0xff, curr_bus);
> > /* There is only one link on a bus, and it is always link 0.
> > * dev->link[0] for a PCI domain is the domain link.
> > * The child of the domain link is the PCI bus device.
>
> This is where it gets a little strange.  The domain has lots of children,
> but none of them are a "pci bus device."

At least for qemu in v3, the domain link has the PCI bus device as a
child. I can post logs to prove it.
That's also why I believe the various K8 board dts in the tree have an
incorrect structure.
> If you look at Stefan's old pngs
> from v2, there were a lot more devices and children than there are in v3 so
> far.  I think looking at the output of show_all_devs makes this clear.  Any
> log that's been posted to the link will have that.

Yes, but AFAICS the structure of the configuration is different for v2
and v3.
show_all_devs also makes one bug of the current tree very visible: The
devices mentioned in the dts are listed twice. My patch makes them
appear only once, but the resources of the domain are somehow not
inherited by the bus and thus not available for the PCI devices.
> I'm still trying to understand the pci enumeration, which is why I was
> cleaning up the pci_device code.

Thanks for that cleanup, it is appreciated.

Regards,
Carl-Daniel

[ron minnich]

I am glad you are scanning that code. We need more people scanning
but, more important, we need people writing docs. If somebody wants to
take a look at (e.g.) pci1234 and document what it does that is always
good. We can then compile the writeups into a book.

ron

[ron minnich]

as noted, the device tree code is very tricky. We definitely don't
want this patch -- you are skipping around the child to the child ...
bad idea.

Myles and I are doing a very slow cleanup, as you are going to see
from a very slow stream of patches every couple days. But unwinding
this tangle is hard, and we're not going to go fast.

thanks

ron