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 <[EMAIL PROTECTED]>
>
> @@ -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.
> @@ -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)?
> @@ -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().
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
