[coreboot] pci_device.c cleanup
Myles Watson
mylesgw at gmail.com
Wed Nov 5 03:58:52 CET 2008
> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:c-d.hailfinger.devel.2006 at gmx.net]
> Sent: Tuesday, November 04, 2008 5:09 PM
> To: Myles Watson
> Cc: Coreboot
> Subject: Re: [coreboot] pci_device.c cleanup
>
> On 04.11.2008 21:04, Myles Watson wrote:
> > This patch clarifies/adds comments and changes names in
> device/pci_device.c
> >
> > It also changes %p debug statements in various places. I think they get
> in
> > the way of diffs when you have log files to compare. I don't want to
> see
> > the
> > allocation differences most of the time. I turned most of them into
> NULL
> > checks. If they were supposed to be "Where are we in device
> allocation?"
> > checks, we could make them into that too.
> >
> > It's a work-in-progress. Comments welcome.
> >
> > I think most of the changes are self explanatory, but this one might not
> be:
> >
> > If you are reading all the BARs from a device, and you come to a 64-bit
> BAR.
> > No matter why you skip it, you should skip it as a 64-bit BAR, and not
> try
> > to
> > read the upper half as the next 32-bit BAR.
> >
> > Because of that, set the 64-bit flag IORESOURCE_PCI64 early, and don't
> clear
> > it on return.
> >
> > Signed-off-by: Myles Watson <mylesgw at gmail.com>
> >
> > @@ -899,18 +924,18 @@
> > * Given a linked list of PCI device structures and a devfn number,
> find the
> > * device structure correspond to the devfn, if present. This function
> also
> > * removes the device structure from the linked list.
> > - *
> > + *
> > * @param list The device structure list.
> > * @param devfn A device/function number.
> > * @return Pointer to the device structure found or NULL of we have not
> > * allocated a device for this devfn yet.
> > */
> > -static struct device *pci_scan_get_dev(struct device **list, unsigned
> int devfn)
> > +static struct device *pci_get_dev(struct device **list, unsigned int
> devfn)
> > {
> > struct device *dev;
> > dev = 0;
> > - printk(BIOS_SPEW, "%s: list is %p, *list is %p\n", __func__, list,
> > - *list);
> > + printk(BIOS_SPEW, "%s: list is %sNULL, *list is %sNULL\n", __func__,
> > + list?"NOT ":"", *list?"NOT ":"");
> > for (; *list; list = &(*list)->sibling) {
> > printk(BIOS_SPEW, "%s: check dev %s \n", __func__,
> > (*list)->dtsname);
> >
>
> Was that change really intentional? Sure, it makes diffing easier, but
> some diagnostics (especially considering the still buggy device
> enumeration) are now more difficult.
Since this is looking through the list of children, and the function prints
out the dtsname of each child, I didn't find the actual pointer to be
useful. The dtsname is much more useful.
>
> > @@ -1043,15 +1068,22 @@
> > dev->irq_pin = pci_read_config8(dev, PCI_INTERRUPT_PIN);
> > dev->min_gnt = pci_read_config8(dev, PCI_MIN_GNT);
> > dev->max_lat = pci_read_config8(dev, PCI_MAX_LAT);
> > -#warning Per-device subsystem ID should only be read from the device if
> none has been specified for the device in the dts.
> > - dev->subsystem_vendor = pci_read_config16(dev,
> PCI_SUBSYSTEM_VENDOR_ID);
> > - dev->subsystem_device = pci_read_config16(dev, PCI_SUBSYSTEM_ID);
> >
> > - /* Store the interesting information in the device structure. */
> > + /*Per-device subsystem ID should only be read from the device if
> none
> > + * has been specified for the device in the dts.
> > + */
> > + if (!dev->subsystem_vendor && !dev->subsystem_device) {
> > + dev->subsystem_vendor =
> > + pci_read_config16(dev,
PCI_SUBSYSTEM_VENDOR_ID);
> > + dev->subsystem_device =
> > + pci_read_config16(dev, PCI_SUBSYSTEM_ID);
> > + }
> > +
> > dev->id.type = DEVICE_ID_PCI;
> > dev->id.pci.vendor = id & 0xffff;
> > dev->id.pci.device = (id >> 16) & 0xffff;
> > dev->hdr_type = hdr_type;
> > +
> > /* Class code, the upper 3 bytes of PCI_CLASS_REVISION. */
> > dev->class = class >> 8;
> >
> >
>
> Hm. Although you fixed the code as indicated in the #warning, I fear
> there's still something missing and that's the reason the warning was
> there. Where do we set the subsystem ID of the real device (not struct
> device)?
There's another place in the code where there's a warning that addresses
this same issue. I didn't think we needed it in both places.
>
> > @@ -1105,6 +1135,8 @@
> >
> > printk(BIOS_DEBUG, "%s start bus %p, bus->dev %p\n", __func__, bus,
> > bus->dev);
> > +
> > +#warning This check needs checking.
> > if (bus->dev->path.type != DEVICE_PATH_PCI_BUS)
> > printk(BIOS_ERR, "ERROR: pci_scan_bus called with incorrect
"
> > "bus->dev->path.type, path is %s\n", dev_path(bus-
> >dev));
> >
>
> If you manage to fix the device code in a way which doesn't trigger this
> anymore, your static devices will suddenly be found. Since this check is
> only triggered if your code and/or device tree is really buggy, we could
> upgrade it to a die().
I realize it's a problem. Since I removed a couple of the other warnings, I
thought I'd put one in this trouble spot.
Thanks for the review,
Myles
More information about the coreboot
mailing list