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@gmx.net
Index: corebootv3-sb600/southbridge/amd/amd8132/amd8132_bridge.c =================================================================== --- corebootv3-sb600/southbridge/amd/amd8132/amd8132_bridge.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/amd8132/amd8132_bridge.c (Arbeitskopie) @@ -213,16 +213,16 @@ chip_rev = pci_read_config8(dev, PCI_CLASS_REVISION);
/* Enable memory write and invalidate ??? */ - dword = pci_read_config32(dev, 0x04); + dword = pci_read_config32(dev, PCI_COMMAND); dword |= 0x10; dword &= ~(1<<6); // PERSP Parity Error Response - pci_write_config32(dev, 0x04, dword); + pci_write_config32(dev, PCI_COMMAND, dword);
if (chip_rev == 0x01) { /* Errata #37 */ - byte = pci_read_config8(dev, 0x0c); + byte = pci_read_config8(dev, PCI_CACHE_LINE_SIZE); if(byte == 0x08 ) - pci_write_config8(dev, 0x0c, 0x10); + pci_write_config8(dev, PCI_CACHE_LINE_SIZE, 0x10);
#if 0 /* Errata #59*/ @@ -235,9 +235,9 @@
/* Set up error reporting, enable all */ /* system error enable */ - dword = pci_read_config32(dev, 0x04); + dword = pci_read_config32(dev, PCI_COMMAND); dword |= (1<<8); - pci_write_config32(dev, 0x04, dword); + pci_write_config32(dev, PCI_COMMAND, dword); /* system and error parity enable */ dword = pci_read_config32(dev, 0x3c); Index: corebootv3-sb600/southbridge/amd/amd8151/amd8151_agp3.c =================================================================== --- corebootv3-sb600/southbridge/amd/amd8151/amd8151_agp3.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/amd8151/amd8151_agp3.c (Arbeitskopie) @@ -33,9 +33,9 @@
/* Enable BM, MEM and IO */ /* this config32 is arguably wrong but I won't change until we can test. */ - byte = pci_read_config32(dev, 0x04); + byte = pci_read_config32(dev, PCI_COMMAND); byte |= 0x07; - pci_write_config8(dev, 0x04, byte); + pci_write_config8(dev, PCI_COMMAND, byte);
return; } @@ -62,9 +62,9 @@ pci_write_config32(dev, 0xa8, value);
/* enable BM and MEM */ - value = pci_read_config32(dev, 0x4); + value = pci_read_config32(dev, PCI_COMMAND); value |= 6; - pci_write_config32(dev, 0x4, value); + pci_write_config32(dev, PCI_COMMAND, value); #if 0 /* FIXME: should we add agp aperture base and size here ? * or it is done by AGP drivers */ Index: corebootv3-sb600/southbridge/amd/rs690/pcie.c =================================================================== --- corebootv3-sb600/southbridge/amd/rs690/pcie.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/rs690/pcie.c (Arbeitskopie) @@ -112,10 +112,10 @@ printk(BIOS_DEBUG, "pcie_init in rs690_pcie.c\n");
/* System error enable */ - dword = pci_read_config32(dev, 0x04); + dword = pci_read_config32(dev, PCI_COMMAND); dword |= (1 << 8); /* System error enable */ dword |= (1 << 30); /* Clear possible errors */ - pci_write_config32(dev, 0x04, dword); + pci_write_config32(dev, PCI_COMMAND, dword); }
@@ -171,8 +171,8 @@ set_nbcfg_enable_bits(nb_dev, 0x7C, 1 << 30, 1 << 30); /* Enables writes to the BAR3 register. */ set_nbcfg_enable_bits(nb_dev, 0x84, 7 << 16, 0 << 16);
- pci_write_config32(nb_dev, 0x1C, EXT_CONF_BASE_ADDRESS); /* PCIEMiscInit */ - pci_write_config32(nb_dev, 0x20, 0x00000000); + pci_write_config32(nb_dev, PCI_BASE_ADDRESS_3, EXT_CONF_BASE_ADDRESS); /* PCIEMiscInit */ + pci_write_config32(nb_dev, PCI_BASE_ADDRESS_4, 0x00000000); set_htiu_enable_bits(nb_dev, 0x32, 1 << 28, 1 << 28); /* PCIEMiscInit */ ProgK8TempMmioBase(1, EXT_CONF_BASE_ADDRESS, TEMP_MMIO_BASE_ADDRESS); } @@ -185,7 +185,7 @@ { printk(BIOS_DEBUG, "disable_pcie_bar3()\n"); set_nbcfg_enable_bits(nb_dev, 0x7C, 1 << 30, 0 << 30); /* Disable writes to the BAR3. */ - pci_write_config32(nb_dev, 0x1C, 0); /* clear BAR3 address */ + pci_write_config32(nb_dev, PCI_BASE_ADDRESS_3, 0); /* clear BAR3 address */ ProgK8TempMmioBase(0, EXT_CONF_BASE_ADDRESS, TEMP_MMIO_BASE_ADDRESS); }
Index: corebootv3-sb600/southbridge/amd/rs690/gfx.c =================================================================== --- corebootv3-sb600/southbridge/amd/rs690/gfx.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/rs690/gfx.c (Arbeitskopie) @@ -39,7 +39,7 @@
static u32 clkind_read(struct device * dev, u32 index) { - u32 gfx_bar2 = pci_read_config32(dev, 0x18) & ~0xF; + u32 gfx_bar2 = pci_read_config32(dev, PCI_BASE_ADDRESS_2) & ~0xF;
*(u32*)(gfx_bar2+CLK_CNTL_INDEX) = index & 0x7F; return *(u32*)(gfx_bar2+CLK_CNTL_DATA); @@ -47,7 +47,7 @@
static void clkind_write(struct device * dev, u32 index, u32 data) { - u32 gfx_bar2 = pci_read_config32(dev, 0x18) & ~0xF; + u32 gfx_bar2 = pci_read_config32(dev, PCI_BASE_ADDRESS_2) & ~0xF; /* printk(BIOS_INFO, "gfx bar 2 %02x\n", gfx_bar2); */
*(u32*)(gfx_bar2+CLK_CNTL_INDEX) = index | 1<<7; @@ -66,7 +66,7 @@ Even if we write 0xFFFFFFFF into it, it will be 0xFFF00000, which tells us it is a memory address base. */ - pci_write_config32(dev, 0x24, 0x00000000); + pci_write_config32(dev, PCI_BASE_ADDRESS_5, 0x00000000);
/* Get the normal pci resources of this device */ pci_dev_read_resources(dev); Index: corebootv3-sb600/southbridge/amd/rs690/ht.c =================================================================== --- corebootv3-sb600/southbridge/amd/rs690/ht.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/rs690/ht.c (Arbeitskopie) @@ -60,10 +60,10 @@ printk(BIOS_INFO, "pcie_init in rs690_ht.c\n");
/* System error enable */ - dword = pci_read_config32(dev, 0x04); + dword = pci_read_config32(dev, PCI_COMMAND); dword |= (1 << 8); /* System error enable */ dword |= (1 << 30); /* Clear possible errors */ - pci_write_config32(dev, 0x04, dword); + pci_write_config32(dev, PCI_COMMAND, dword);
/* * 1 is APIC enable Index: corebootv3-sb600/southbridge/amd/rs690/cmn.c =================================================================== --- corebootv3-sb600/southbridge/amd/rs690/cmn.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/rs690/cmn.c (Arbeitskopie) @@ -46,7 +46,7 @@ u32 pci_ext_read_config32(struct device * nb_dev, struct device * dev, u32 reg) { /*get BAR3 base address for nbcfg0x1c */ - u32 addr = pci_read_config32(nb_dev, 0x1c); + u32 addr = pci_read_config32(nb_dev, PCI_BASE_ADDRESS_3); printk(BIOS_DEBUG, "addr=%x,bus=%x,devfn=%x\n", addr, dev->bus->secondary, dev->path.pci.devfn); addr |= dev->bus->secondary << 20 | /* bus num */ @@ -59,7 +59,7 @@ u32 reg_old, reg;
/*get BAR3 base address for nbcfg0x1c */ - u32 addr = pci_read_config32(nb_dev, 0x1c); + u32 addr = pci_read_config32(nb_dev, PCI_BASE_ADDRESS_3); printk(BIOS_DEBUG, "write: addr=%x,bus=%x,devfn=%x\n", addr, dev->bus->secondary, dev->path.pci.devfn); addr |= dev->bus->secondary << 20 | /* bus num */ Index: corebootv3-sb600/southbridge/amd/sb600/ide.c =================================================================== --- corebootv3-sb600/southbridge/amd/sb600/ide.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/sb600/ide.c (Arbeitskopie) @@ -51,9 +51,9 @@ pci_write_config8(dev, 0x56, byte);
/* Enable I/O Access&& Bus Master */ - dword = pci_read_config16(dev, 0x4); + dword = pci_read_config16(dev, PCI_COMMAND); dword |= 1 << 2; - pci_write_config16(dev, 0x4, dword); + pci_write_config16(dev, PCI_COMMAND, dword);
#if CONFIG_PCI_OPTION_ROM_RUN == 1 pci_dev_init(dev); Index: corebootv3-sb600/southbridge/amd/sb600/pci.c =================================================================== --- corebootv3-sb600/southbridge/amd/sb600/pci.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/sb600/pci.c (Arbeitskopie) @@ -62,8 +62,8 @@ pci_write_config8(dev, 0x40, byte);
/* RPR 4.5 Enables the PCIB to retain ownership of the bus on the Primary side and on the Secondary side when GNT# is deasserted */ - pci_write_config8(dev, 0x0D, 0x40); - pci_write_config8(dev, 0x1B, 0x40); + pci_write_config8(dev, PCI_LATENCY_TIMER, 0x40); + pci_write_config8(dev, PCI_SEC_LATENCY_TIMER, 0x40);
/* RPR 4.6 Enable the command matching checking function on "Memory Read" & "Memory Read Line" commands */ byte = pci_read_config8(dev, 0x4B); 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)){ die("SMBUS controller not found\r\n"); } return pci_conf1_read_config8(dev, 0x08); @@ -90,12 +91,12 @@ u32 dev;
/* Enable lpc controller */ - pci_conf1_find_device(0x1002, 0x4385, &dev); /* SMBUS controller */ + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4385, &dev); /* SMBUS controller */ reg32 = pci_conf1_read_config32(dev, 0x64); reg32 |= 0x00100000; pci_conf1_write_config32(dev, 0x64, reg32);
- pci_conf1_find_device(0x1002, 0x438d, &dev); /* LPC Controller */ + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x438d, &dev); /* LPC Controller */ /* Serial 0 */ reg8 = pci_conf1_read_config8(dev, 0x44); reg8 |= (1 << 6); @@ -126,7 +127,7 @@ u32 dev;
/* Find the device. */ - pci_conf1_find_on_bus(bus, 0x1002, 0x4385, &dev); + pci_conf1_find_on_bus(bus, PCI_VENDOR_ID_ATI, 0x4385, &dev); return (dev >> 15) & 0x1f; }
@@ -215,7 +216,7 @@ u32 dev;
/* P2P Bridge */ - pci_conf1_find_device(0x1002, 0x4384, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4384, &dev);
byte = pci_conf1_read_config8(dev, 0x40); byte |= 1 << 5; @@ -237,7 +238,7 @@ byte |= 1 << 0; pci_conf1_write_config8(dev, 0x04, byte);
- pci_conf1_find_device(0x1002, 0x438D, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x438D, &dev);
byte = pci_conf1_read_config8(dev, 0x4A); byte &= ~(1 << 5); /* disable lpc port 80 */ @@ -251,13 +252,13 @@ u32 reg32;
/* enable lpc controller */ - pci_conf1_find_device(0x1002, 0x4385, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4385, &dev); reg32 = pci_conf1_read_config32(dev, 0x64); reg32 |= 0x00100000; /* lpcEnable */ pci_conf1_write_config32(dev, 0x64, reg32);
/* enable prot80 LPC decode in pci function 3 configuration space. */ - pci_conf1_find_device(0x1002, 0x438d, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x438d, &dev); byte = pci_conf1_read_config8(dev, 0x4a); byte |= 1 << 5; /* enable port 80 */ pci_conf1_write_config8(dev, 0x4a, byte); @@ -277,7 +278,7 @@ printk(BIOS_INFO, "sb600_devices_por_init()\n"); /* SMBus Device, BDF:0-20-0 */ printk(BIOS_INFO, "sb600_devices_por_init(): SMBus Device, BDF:0-20-0\n"); - if (!pci_conf1_find_device(0x1002, 0x4385, &dev)){ + if (!pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4385, &dev)){ die("SMBUS controller not found\r\n"); } printk(BIOS_INFO, "SMBus controller enabled, sb revision is 0x%x\r\n", @@ -360,7 +361,7 @@
/* IDE Device, BDF:0-20-1 */ printk(BIOS_INFO, "sb600_devices_por_init(): IDE Device, BDF:0-20-1\n"); - pci_conf1_find_device(0x1002, 0x438C, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x438C, &dev); /* Disable prefetch */ byte = pci_conf1_read_config8(dev, 0x63); byte |= 0x1; @@ -368,7 +369,7 @@
/* LPC Device, BDF:0-20-3 */ printk(BIOS_INFO, "sb600_devices_por_init(): LPC Device, BDF:0-20-3\n"); - pci_conf1_find_device(0x1002, 0x438D, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x438D, &dev); /* DMA enable */ pci_conf1_write_config8(dev, 0x40, 0x04);
@@ -405,7 +406,7 @@ /* P2P Bridge, BDF:0-20-4, the configuration of the registers in this dev are copied from CIM, * TODO: I don't know what are their mean? */ printk(BIOS_INFO, "sb600_devices_por_init(): P2P Bridge, BDF:0-20-4\n"); - pci_conf1_find_device(0x1002, 0x4384, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4384, &dev); /* I don't know why CIM tried to write into a read-only reg! */ /*pci_conf1_write_config8(dev, 0x0c, 0x20) */ ;
@@ -436,7 +437,7 @@
/* SATA Device, BDF:0-18-0, Non-Raid-5 SATA controller */ printk(BIOS_INFO, "sb600_devices_por_init(): SATA Device, BDF:0-18-0\n"); - pci_conf1_find_device(0x1002, 0x4380, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4380, &dev);
/*PHY Global Control, we are using A14. * default: 0x2c40 for ASIC revision A12 and below @@ -555,7 +556,7 @@ u8 byte;
/* SMBus Device, BDF:0-20-0 */ - pci_conf1_find_device(0x1002, 0x4385, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4385, &dev); /* Eable the hidden revision ID, available after A13. */ byte = pci_conf1_read_config8(dev, 0x70); byte |= (1 << 8); @@ -588,14 +589,14 @@ }
/* IDE Device, BDF:0-20-1 */ - pci_conf1_find_device(0x1002, 0x438C, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x438C, &dev); /* Enable IDE Explicit prefetch, 0x63[0] clear */ byte = pci_conf1_read_config8(dev, 0x63); byte &= 0xfe; pci_conf1_write_config8(dev, 0x63, byte);
/* LPC Device, BDF:0-20-3 */ - pci_conf1_find_device(0x1002, 0x438D, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x438D, &dev); /* rpr7.2 Enabling LPC DMA function. */ byte = pci_conf1_read_config8(dev, 0x40); byte |= (1 << 2); @@ -610,7 +611,7 @@ pci_conf1_write_config8(dev, 0x78, byte);
/* SATA Device, BDF:0-18-0, Non-Raid-5 SATA controller */ - pci_conf1_find_device(0x1002, 0x4380, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4380, &dev); /* rpr6.8 Disabling SATA MSI Capability, for A13 and above, 0x42[7]. */ if (0x12 < get_sb600_revision()) { u32 reg32; @@ -620,14 +621,14 @@ }
/* EHCI Device, BDF:0-19-5, ehci usb controller */ - pci_conf1_find_device(0x1002, 0x4386, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4386, &dev); /* rpr5.10 Disabling USB EHCI MSI Capability. 0x50[6]. */ byte = pci_conf1_read_config8(dev, 0x50); byte |= (1 << 6); pci_conf1_write_config8(dev, 0x50, byte);
/* OHCI0 Device, BDF:0-19-0, ohci usb controller #0 */ - pci_conf1_find_device(0x1002, 0x4387, &dev); + pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4387, &dev); /* rpr5.11 Disabling USB OHCI MSI Capability. 0x40[12:8]=0x1f. */ byte = pci_conf1_read_config8(dev, 0x41); byte |= 0x1f; Index: corebootv3-sb600/southbridge/amd/sb600/sata.c =================================================================== --- corebootv3-sb600/southbridge/amd/sb600/sata.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/sb600/sata.c (Arbeitskopie) @@ -42,15 +42,14 @@
struct device * sm_dev; /* SATA SMBus Disable */ - /* sm_dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0); */ + /* sm_dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_ATI, 0x4385), 0); */ sm_dev = dev_find_slot(0, PCI_DEVFN(0x14, 0)); /* Disable SATA SMBUS */ byte = pci_read_config8(sm_dev, 0xad); byte |= (1 << 1); /* Enable SATA and power saving */ byte = pci_read_config8(sm_dev, 0xad); - byte |= (1 << 0); - byte |= (1 << 5); + byte |= (1 << 0) | (1 << 5); pci_write_config8(sm_dev, 0xad, byte); /* Set the interrupt Mapping to INTG# */ byte = pci_read_config8(sm_dev, 0xaf); @@ -58,12 +57,12 @@ pci_write_config8(sm_dev, 0xaf, byte);
/* 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;
printk(BIOS_DEBUG, "sata_bar0=%x\n", sata_bar0); /* 3030 */ printk(BIOS_DEBUG, "sata_bar1=%x\n", sata_bar1); /* 3070 */ @@ -72,14 +71,15 @@ printk(BIOS_DEBUG, "sata_bar4=%x\n", sata_bar4); /* 3000 */ printk(BIOS_DEBUG, "sata_bar5=%p\n", sata_bar5); /* e0309000 */
- /* Program the 2C to 0x43801002 */ - dword = 0x43801002; - pci_write_config32(dev, 0x2c, dword); + /* Program the subsystem device/vendor to 0x43801002 */ +#warning Subsystem ID setting should be in a separate function + dword = (0x4380 << 16) | PCI_VENDOR_ID_ATI; + pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID, dword);
/* SERR-Enable */ - word = pci_read_config16(dev, 0x04); + word = pci_read_config16(dev, PCI_COMMAND); word |= (1 << 8); - pci_write_config16(dev, 0x04, word); + pci_write_config16(dev, PCI_COMMAND, word);
/* Dynamic power saving */ byte = pci_read_config8(dev, 0x40); @@ -88,12 +88,11 @@
/* Set SATA Operation Mode, Set to IDE mode */ byte = pci_read_config8(dev, 0x40); - byte |= (1 << 0); - byte |= (1 << 4); + byte |= (1 << 0) | (1 << 4); pci_write_config8(dev, 0x40, byte);
dword = 0x01018f00; - pci_write_config32(dev, 0x8, dword); + pci_write_config32(dev, PCI_REVISION_ID, dword);
byte = pci_read_config8(dev, 0x40); byte &= ~(1 << 0); @@ -134,10 +133,10 @@ dword |= 1 << 25; pci_write_config32(dev, 0x40, dword);
- /* Enable the I/O ,MM ,BusMaster access for SATA */ - byte = pci_read_config8(dev, 0x4); + /* Enable the I/O, MM, BusMaster access for SATA */ + byte = pci_read_config8(dev, PCI_COMMAND); byte |= 7 << 0; - pci_write_config8(dev, 0x4, byte); + pci_write_config8(dev, PCI_COMMAND, byte);
/* RPR6.6 SATA drive detection. Currently we detect Primary Master Device only */ /* Use BAR5+0x1A8,BAR0+0x6 for Primary Slave */ Index: corebootv3-sb600/southbridge/amd/sb600/usb.c =================================================================== --- corebootv3-sb600/southbridge/amd/sb600/usb.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/sb600/usb.c (Arbeitskopie) @@ -101,7 +101,7 @@ /* dword |= 40; */ /* pci_write_config32(dev, 0xf8, dword); */
- usb2_bar0 = (u8 *) (pci_read_config32(dev, 0x10) & ~0xFF); + usb2_bar0 = (u8 *) (pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFF); printk(BIOS_INFO, "usb2_bar0=%x\n", usb2_bar0);
/* RPR5.4 Enables the USB PHY auto calibration resister to match 45ohm resistence */ Index: corebootv3-sb600/southbridge/amd/amd8111/ide.c =================================================================== --- corebootv3-sb600/southbridge/amd/amd8111/ide.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/amd8111/ide.c (Arbeitskopie) @@ -57,7 +57,7 @@
byte = 0x20 ; // Latency: 64-->32 - pci_write_config8(dev, 0xd, byte); + pci_write_config8(dev, PCI_LATENCY_TIMER, byte);
word = 0x0f; pci_write_config16(dev, 0x42, word); Index: corebootv3-sb600/southbridge/amd/amd8111/ac97.c =================================================================== --- corebootv3-sb600/southbridge/amd/amd8111/ac97.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/amd8111/ac97.c (Arbeitskopie) @@ -30,7 +30,7 @@
static void lpci_set_subsystem(struct device * dev, unsigned vendor, unsigned device) { - pci_write_config32(dev, 0x2c, + pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID, ((device & 0xffff) << 16) | (vendor & 0xffff)); }
Index: corebootv3-sb600/southbridge/amd/amd8111/pci.c =================================================================== --- corebootv3-sb600/southbridge/amd/amd8111/pci.c (Revision 906) +++ corebootv3-sb600/southbridge/amd/amd8111/pci.c (Arbeitskopie) @@ -35,10 +35,10 @@ u32 dword;
/* System error enable */ - dword = pci_read_config32(dev, 0x04); + dword = pci_read_config32(dev, PCI_COMMAND); dword |= (1<<8); /* System error enable */ dword |= (7<<28); /* Clear possible errors */ - pci_write_config32(dev, 0x04, dword); + pci_write_config32(dev, PCI_COMMAND, dword);
/* System,Parity,timer,and abort error enable */ dword = pci_read_config32(dev, 0x3c); Index: corebootv3-sb600/southbridge/nvidia/mcp55/ide.c =================================================================== --- corebootv3-sb600/southbridge/nvidia/mcp55/ide.c (Revision 906) +++ corebootv3-sb600/southbridge/nvidia/mcp55/ide.c (Arbeitskopie) @@ -60,7 +60,7 @@
byte = 0x20 ; // Latency: 64-->32 - pci_write_config8(dev, 0xd, byte); + pci_write_config8(dev, PCI_LATENCY_TIMER, byte);
dword = pci_read_config32(dev, 0xf8); dword |= 12; Index: corebootv3-sb600/southbridge/nvidia/mcp55/pci.c =================================================================== --- corebootv3-sb600/southbridge/nvidia/mcp55/pci.c (Revision 906) +++ corebootv3-sb600/southbridge/nvidia/mcp55/pci.c (Arbeitskopie) @@ -43,10 +43,10 @@ #endif
/* System error enable */ - dword = pci_read_config32(dev, 0x04); + dword = pci_read_config32(dev, PCI_COMMAND); dword |= (1<<8); /* System error enable */ dword |= (1<<30); /* Clear possible errors */ - pci_write_config32(dev, 0x04, dword); + pci_write_config32(dev, PCI_COMMAND, dword);
#if 1 //only need (a01,xx] Index: corebootv3-sb600/southbridge/nvidia/mcp55/pcie.c =================================================================== --- corebootv3-sb600/southbridge/nvidia/mcp55/pcie.c (Revision 906) +++ corebootv3-sb600/southbridge/nvidia/mcp55/pcie.c (Arbeitskopie) @@ -39,10 +39,10 @@ u32 dword;
/* System error enable */ - dword = pci_read_config32(dev, 0x04); + dword = pci_read_config32(dev, PCI_COMMAND); dword |= (1<<8); /* System error enable */ dword |= (1<<30); /* Clear possible errors */ - pci_write_config32(dev, 0x04, dword); + pci_write_config32(dev, PCI_COMMAND, dword);
}
Index: corebootv3-sb600/southbridge/intel/i82371eb/i82371eb.c =================================================================== --- corebootv3-sb600/southbridge/intel/i82371eb/i82371eb.c (Revision 906) +++ corebootv3-sb600/southbridge/intel/i82371eb/i82371eb.c (Arbeitskopie) @@ -54,9 +54,9 @@ pci_write_config16(dev, 0x42, c);
printk(BIOS_INFO, "Enabling Legacy IDE\n"); - c = pci_read_config16(dev, 4); + c = pci_read_config16(dev, PCI_COMMAND); c |= 1; - pci_write_config16(dev, 4, c); + pci_write_config16(dev, PCI_COMMAND, c); }
static void i82371eb_acpi_init(struct device *dev) @@ -71,7 +71,7 @@ /* smbus enable */ pci_write_config8(dev, 0xd2, (0x4 << 1) | 1); /* iospace enable */ - pci_write_config16(dev, 0x4, 1); + pci_write_config16(dev, PCI_COMMAND, 1);
printk(BIOS_DEBUG, "Enable Power Management Functions\n"); pm_io = 0xFF80;
Nice. Also, note that if you use kscope, you can click on PCI_COMMAND and it will take you to the definition :-)
Acked-by: Ronald G. Minnich rminnich@gmail.com
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.
In general very nice, but a few comments follow..
- dword = pci_read_config32(dev, 0x04);
- dword = pci_read_config32(dev, PCI_COMMAND); dword |= 0x10; dword &= ~(1<<6); // PERSP Parity Error Response
pci_write_config32(dev, 0x04, dword);
pci_write_config32(dev, PCI_COMMAND, dword);
I think it would be nice to fix whitespace when the line is touched anyway.
- pci_write_config32(nb_dev, 0x1C, EXT_CONF_BASE_ADDRESS); /* PCIEMiscInit */
- pci_write_config32(nb_dev, 0x20, 0x00000000);
- pci_write_config32(nb_dev, PCI_BASE_ADDRESS_3, EXT_CONF_BASE_ADDRESS); /* PCIEMiscInit */
- pci_write_config32(nb_dev, PCI_BASE_ADDRESS_4, 0x00000000);
Hm, maybe PCI_BAR3 or PCI_BAR_3?
- if (!pci_conf1_find_device(0x1002, 0x4385, &dev)){
- if (!pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4385, &dev)){
Not sure about these at all. I think I prefer the number.
- byte |= (1 << 0);
- byte |= (1 << 5);
- byte |= (1 << 0) | (1 << 5);
I like this. Others may not.
- /* Program the 2C to 0x43801002 */
Gotta love comments like this. :)
- dword = 0x43801002;
- pci_write_config32(dev, 0x2c, dword);
- /* Program the subsystem device/vendor to 0x43801002 */
+#warning Subsystem ID setting should be in a separate function
- dword = (0x4380 << 16) | PCI_VENDOR_ID_ATI;
- pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID, dword);
I think
pci_write_config32(dev,PCI_SUBSYSTEM_VENDOR_ID, 0x43801002);
looks pretty nice. I like the warning.
//Peter
On 09.10.2008 04:37, Peter Stuge wrote:
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.
In general very nice, but a few comments follow..
- dword = pci_read_config32(dev, 0x04);
- dword = pci_read_config32(dev, PCI_COMMAND); dword |= 0x10; dword &= ~(1<<6); // PERSP Parity Error Response
pci_write_config32(dev, 0x04, dword);
pci_write_config32(dev, PCI_COMMAND, dword);I think it would be nice to fix whitespace when the line is touched anyway.
Sure thing. I saw it the second after hitting "send".
- pci_write_config32(nb_dev, 0x1C, EXT_CONF_BASE_ADDRESS); /* PCIEMiscInit */
- pci_write_config32(nb_dev, 0x20, 0x00000000);
- pci_write_config32(nb_dev, PCI_BASE_ADDRESS_3, EXT_CONF_BASE_ADDRESS); /* PCIEMiscInit */
- pci_write_config32(nb_dev, PCI_BASE_ADDRESS_4, 0x00000000);
Hm, maybe PCI_BAR3 or PCI_BAR_3?
Hm yes. I decided to use existing #defines, so it was the long form. I can send a separate patch for shorter names.
- if (!pci_conf1_find_device(0x1002, 0x4385, &dev)){
- if (!pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4385, &dev)){
Not sure about these at all. I think I prefer the number.
The numbers are a bit too magic for my taste.
- byte |= (1 << 0);
- byte |= (1 << 5);
- byte |= (1 << 0) | (1 << 5);
I like this. Others may not.
- /* Program the 2C to 0x43801002 */
Gotta love comments like this. :)
- dword = 0x43801002;
- pci_write_config32(dev, 0x2c, dword);
- /* Program the subsystem device/vendor to 0x43801002 */
+#warning Subsystem ID setting should be in a separate function
- dword = (0x4380 << 16) | PCI_VENDOR_ID_ATI;
- pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID, dword);
I think
pci_write_config32(dev,PCI_SUBSYSTEM_VENDOR_ID, 0x43801002);
looks pretty nice.
Actually, that's probably the next cleanup step. This dword = val; pci_write_config32(dev, ..., dword); is a less than stellar style.
I like the warning.
Expect more of them as I walk through the code. ;-)
Regards, Carl-Daniel
On Thu, Oct 9, 2008 at 2:25 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
- if (!pci_conf1_find_device(0x1002, 0x4385, &dev)){
- if (!pci_conf1_find_device(PCI_VENDOR_ID_ATI, 0x4385, &dev)){
Not sure about these at all. I think I prefer the number.
if somebody someday put in 0x1003 for the vendor in that code, how would you know it was wrong? Names are good :-)
ron
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@gmx.net
Great stuff!
Acked-by: Uwe Hermann uwe@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.
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@gmx.net
Great stuff!
Acked-by: Uwe Hermann uwe@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
-
Carl-Daniel Hailfinger -
Peter Stuge -
ron minnich -
Uwe Hermann