[coreboot] [PATCH] v3: improve PCI device doxygen comments and printks
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Fri Aug 22 13:14:32 CEST 2008
Stefan?
I merged the part which you did not object to. If you answer the
questions below, we should be able to resolve this completely.
Regards,
Carl-Daniel
On 18.08.2008 13:39, Carl-Daniel Hailfinger wrote:
> On 18.08.2008 13:26, Stefan Reinauer wrote:
>
>> Carl-Daniel Hailfinger wrote:
>>
>>
>>>>> + * - known knowns. In this case the device is in the tree, i.e. not NULL,
>>>>> + * and we know it's there: it's soldered down or part of the on-chip
>>>>> + * hardware. In this case dev is not NULL.
>>>>>
>>>>>
>
> The point above is the zeroth/third case.
>
>
>>>>> + * - known unknowns. This is a device that might be there, but we don't
>>>>> + * know. So we have to probe it. It's in the dts, which is why
>>>>> + * it is a known unknown.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>> How s the function called for the known knowns? Those are neither PCI
>>>>
>>>>
>>>>
>>>>
>>> Did you mean "called for the known UNknowns"?
>>>
>>>
>>>
>> No.
>>
>> 1. The known unknowns ("listed, or expected pci devices") are called
>> from the dts, as you write.
>>
>>
>
> 1. is "listed, but not necessarily present in hardware"
>
>
>> 2. The unknown unknowns ("scanned pci devices") are not in the dts but
>> scanned.
>>
>> So what's the third case? A device is both listed in the dts and found
>> while scanning? Are those "expected and found"?
>>
>>
>
> The third (or zeroth) case are the known knowns ("listed and present in
> hardware").
>
>
>>>>> @@ -1011,8 +1029,8 @@
>>>>> if ((id == 0xffffffff) || (id == 0x00000000) ||
>>>>> (id == 0x0000ffff) || (id == 0xffff0000)) {
>>>>> if (dev->enabled) {
>>>>> - printk(BIOS_INFO,
>>>>> - "Disabling static device: %s\n",
>>>>> + printk(BIOS_INFO, "PCI: Static device not "
>>>>> + "found, setting enabled=0: %s\n",
>>>>> dev_path(dev));
>>>>>
>>>>>
>>>>>
>>>>>
>>>> I dislike this change of output. It's not a BIOS_DEBUG message.
>>>>
>>>>
>>>>
>>> Do you dislike the severity level (same as before) or the new wording
>>> (the old wording was wrong)?
>>>
>>>
>> Why did you consider it wrong?
>>
>>
>
> The old message said "disabling static device", but the device is not
> touched at all.
>
>
>> Either the message should stay or the level should be raised to debug.
>> Not sure. "Setting enabled=0" implies you have to understand the code in
>> order to understand the message. This is bad for BIOS_INFO type messages
>> (Imho, for everything more "severe" than DEBUG)
>>
>>
>
> I have yet to find a better wording which says that we don't touch the
> physical device and set the matching struct device to disabled instead.
>
Improve/fix PCI device doxygen comments and printks. As a bonus, the
boot messages are more understandable now.
Thanks to Ron for supplying better explanations. I've included most of
them verbatim.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
Acked-by: Ronald G. Minnich <rminnich at gmail.com>
Index: corebootv3-pci_device_better_prints_comments/device/pci_device.c
===================================================================
--- corebootv3-pci_device_better_prints_comments/device/pci_device.c (Revision 801)
+++ corebootv3-pci_device_better_prints_comments/device/pci_device.c (Arbeitskopie)
@@ -963,6 +963,17 @@
* or the passed in device structure with enabled=0 if the device
* does not exist in hardware and only in the tree
* or NULL if no device is found and dev==NULL was passed in.
+ *
+ * There are three cases:
+ * - known knowns. In this case the device is in the tree, i.e. not NULL,
+ * and we know it's there: it's soldered down or part of the on-chip
+ * hardware. In this case dev is not NULL.
+ * - known unknowns. This is a device that might be there, but we don't
+ * know. So we have to probe it. It's in the dts, which is why
+ * it is a known unknown.
+ * - unknown unknowns. A PCI card in a PCI slot. We can't cover all
+ * possible cards. dev is NULL. We are checking to see if something is
+ * there; if so, we will allocate a dev and put it in the three.
*/
struct device *pci_probe_dev(struct device *dev, struct bus *bus,
unsigned int devfn)
@@ -1018,8 +1029,8 @@
if ((id == 0xffffffff) || (id == 0x00000000) ||
(id == 0x0000ffff) || (id == 0xffff0000)) {
if (dev->enabled) {
- printk(BIOS_INFO,
- "Disabling static device: %s\n",
+ printk(BIOS_INFO, "PCI: Static device not "
+ "found, setting enabled=0: %s\n",
dev_path(dev));
dev->enabled = 0;
}
--
http://www.hailfinger.org/
More information about the coreboot
mailing list