[coreboot] [PATCH] v3: Replace magic numbers with symbolic constants

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Fri Oct 10 00:22:04 CEST 2008


On 09.10.2008 12:03, Uwe Hermann wrote:
> On Thu, Oct 09, 2008 at 04:01:27AM +0200, Carl-Daniel Hailfinger wrote:
>   
>> Replace magic numbers with existing symbolic constants. SB600 is heavily
>> affected. This mostly targets pci_*_config*() calls.
>>
>> This is part of my quest to make existing code more readable without
>> looking up magic numbers.
>>
>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>>     
>
> Great stuff!
>
> Acked-by: Uwe Hermann <uwe at hermann-uwe.de>
>   

Thanks, r913.


> Do you plan to also port this to v2? If not, I might do it.
>   

Yes, sometime in the next few weeks.


>> Index: corebootv3-sb600/southbridge/amd/sb600/stage1.c
>> ===================================================================
>> --- corebootv3-sb600/southbridge/amd/sb600/stage1.c	(Revision 906)
>> +++ corebootv3-sb600/southbridge/amd/sb600/stage1.c	(Arbeitskopie)
>> @@ -23,6 +23,7 @@
>>  #include <msr.h>
>>  #include <legacy.h>
>>  #include <device/pci.h>
>> +#include <device/pci_ids.h>
>>  #include <statictree.h>
>>  #include <config.h>
>>  #include <io.h>
>> @@ -68,7 +69,7 @@
>>  static u8 get_sb600_revision(void)
>>  {
>>  	u32 dev;
>> -	if (!pci_conf1_find_device(0x1002,  0x4385, &dev)){
>> +	if (!pci_conf1_find_device(PCI_VENDOR_ID_ATI,  0x4385, &dev)){
>>     
>
> That may be for another patch, but the 0x4385 should of course also be
> replaced by PCI_DEVICE_ID_ATI_SB600_SM (same for all other chipset devices).
>
> Currently those #defines are all in southbridge/amd/sb600/sb600.h, but
> they should be moved to pci_ids.h (both in v2 and in v3).
>   

Sure. Will be done in another patch. Right now I wanted at least some
basic sanity committed before continuing.


>>  	/* get base addresss */
>> -	sata_bar5 = (u8 *) (pci_read_config32(dev, 0x24) & ~0x3FF);
>> -	sata_bar0 = pci_read_config16(dev, 0x10) & ~0x7;
>> -	sata_bar1 = pci_read_config16(dev, 0x14) & ~0x7;
>> -	sata_bar2 = pci_read_config16(dev, 0x18) & ~0x7;
>> -	sata_bar3 = pci_read_config16(dev, 0x1C) & ~0x7;
>> -	sata_bar4 = pci_read_config16(dev, 0x20) & ~0x7;
>> +	sata_bar5 = (u8 *) (pci_read_config32(dev, PCI_BASE_ADDRESS_5) & ~0x3FF);
>> +	sata_bar0 = pci_read_config16(dev, PCI_BASE_ADDRESS_0) & ~0x7;
>> +	sata_bar1 = pci_read_config16(dev, PCI_BASE_ADDRESS_1) & ~0x7;
>> +	sata_bar2 = pci_read_config16(dev, PCI_BASE_ADDRESS_2) & ~0x7;
>> +	sata_bar3 = pci_read_config16(dev, PCI_BASE_ADDRESS_3) & ~0x7;
>> +	sata_bar4 = pci_read_config16(dev, PCI_BASE_ADDRESS_4) & ~0x7;
>>     
>
> Not related to this patch, but I'm wondering why BAR5 is read first, and
> then 0, 1, 2, 3, 4. Is there some technical reason for that? If yes,
> there should be a comment here.
>   

No idea. Perhaps due to the fact that bar5 is stored in a different type.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list