[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