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
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
