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

[Uwe Hermann]

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>

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


> 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).


>  	/* 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.

  
Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org