On 05.11.2008 03:58, Myles Watson wrote:
>
>> -----Original Message-----
>> From: Carl-Daniel Hailfinger [mailto:[EMAIL PROTECTED]
>> 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 <[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.
>>
>
> 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.
>
Point taken.
>>> @@ -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.
>
Agreed. I'll try to send a patch which fixes this FIXME.
>>> @@ -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.
>
Good idea.
> Thanks for the review,
>
Thanks for going over this code. It's easy to get lost in there and your
patches definitely improve the code.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
