Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
sb/intel/i82801gx: Use PCI bitwise ops
Tested with BUILD_TIMELESS=1, Getac P470 does not change.
Change-Id: I2cc3e71723e9b6898e6ec29f0f38b1b3b7446f09 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/bootblock.c M src/southbridge/intel/i82801gx/early_init.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801gx/pci.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/sata.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 9 files changed, 50 insertions(+), 136 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/42191/1
diff --git a/src/southbridge/intel/i82801gx/azalia.c b/src/southbridge/intel/i82801gx/azalia.c index 205fb0d..0ce3316 100644 --- a/src/southbridge/intel/i82801gx/azalia.c +++ b/src/southbridge/intel/i82801gx/azalia.c @@ -196,37 +196,21 @@ struct resource *res; u32 codec_mask; u8 reg8; - u32 reg32;
// ESD - reg32 = pci_read_config32(dev, 0x134); - reg32 &= 0xff00ffff; - reg32 |= (2 << 16); - pci_write_config32(dev, 0x134, reg32); + pci_update_config32(dev, 0x134, ~0x00ff0000, 2 << 16);
// Link1 description - reg32 = pci_read_config32(dev, 0x140); - reg32 &= 0xff00ffff; - reg32 |= (2 << 16); - pci_write_config32(dev, 0x140, reg32); + pci_update_config32(dev, 0x140, ~0x00ff0000, 2 << 16);
// Port VC0 Resource Control Register - reg32 = pci_read_config32(dev, 0x114); - reg32 &= 0xffffff00; - reg32 |= 1; - pci_write_config32(dev, 0x114, reg32); + pci_update_config32(dev, 0x114, ~0x000000ff, 1);
// VCi traffic class - reg8 = pci_read_config8(dev, 0x44); - reg8 |= (7 << 0); // TC7 - pci_write_config8(dev, 0x44, reg8); + pci_or_config8(dev, 0x44, 7 << 0); // TC7
// VCi Resource Control - reg32 = pci_read_config32(dev, 0x120); - reg32 |= (1 << 31); - reg32 |= (1 << 24); // VCi ID - reg32 |= (0x80 << 0); // VCi map - pci_write_config32(dev, 0x120, reg32); + pci_or_config32(dev, 0x120, (1 << 31) | (1 << 24) | (0x80 << 0)); /* VCi ID and map */
/* Set Bus Master */ pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER); @@ -246,14 +230,11 @@ reg8 = pci_read_config8(dev, 0x40); printk(BIOS_DEBUG, "Azalia: codec type: %s\n", (reg8 & (1 << 1))?"Azalia":"AC97");
- // - reg8 = pci_read_config8(dev, 0x40); // Audio Control - reg8 |= 1; // Select Azalia mode. This needs to be controlled via devicetree.cb - pci_write_config8(dev, 0x40, reg8); + // Select Azalia mode. This needs to be controlled via devicetree.cb + pci_or_config8(dev, 0x40, 1); // Audio Control
- reg8 = pci_read_config8(dev, 0x4d); // Docking Status - reg8 &= ~(1 << 7); // Docking not supported - pci_write_config8(dev, 0x4d, reg8); + // Docking not supported + pci_and_config8(dev, 0x4d, (u8)~(1 << 7)); // Docking Status #if 0 /* Set routing pin */ pci_write_config32(dev, 0xf8, 0x0); diff --git a/src/southbridge/intel/i82801gx/bootblock.c b/src/southbridge/intel/i82801gx/bootblock.c index b0f73e8..308dfc8 100644 --- a/src/southbridge/intel/i82801gx/bootblock.c +++ b/src/southbridge/intel/i82801gx/bootblock.c @@ -6,13 +6,8 @@
static void enable_spi_prefetch(void) { - u8 reg8; - const pci_devfn_t dev = PCI_DEV(0, 0x1f, 0); - - reg8 = pci_read_config8(dev, BIOS_CNTL); - reg8 &= ~(3 << 2); - reg8 |= (2 << 2); /* Prefetching and Caching Enabled */ - pci_write_config8(dev, BIOS_CNTL, reg8); + /* Prefetching and Caching Enabled */ + pci_update_config8(PCI_DEV(0, 0x1f, 0), BIOS_CNTL, ~(3 << 2), 2 << 2); }
void bootblock_early_southbridge_init(void) diff --git a/src/southbridge/intel/i82801gx/early_init.c b/src/southbridge/intel/i82801gx/early_init.c index a913873..61fc8d9 100644 --- a/src/southbridge/intel/i82801gx/early_init.c +++ b/src/southbridge/intel/i82801gx/early_init.c @@ -61,9 +61,6 @@ #if ENV_ROMSTAGE void i82801gx_early_init(void) { - uint8_t reg8; - uint32_t reg32; - enable_smbus();
/* Setting up Southbridge. In the northbridge code. */ @@ -84,22 +81,14 @@ pci_write_config8(PCI_DEV(0, 0x1e, 0), SMLT, 0x20);
/* reset rtc power status */ - reg8 = pci_read_config8(PCI_DEV(0, 0x1f, 0), GEN_PMCON_3); - reg8 &= ~RTC_BATTERY_DEAD; - pci_write_config8(PCI_DEV(0, 0x1f, 0), GEN_PMCON_3, reg8); + pci_and_config8(PCI_DEV(0, 0x1f, 0), GEN_PMCON_3, ~RTC_BATTERY_DEAD);
/* USB transient disconnect */ - reg8 = pci_read_config8(PCI_DEV(0, 0x1f, 0), 0xad); - reg8 |= (3 << 0); - pci_write_config8(PCI_DEV(0, 0x1f, 0), 0xad, reg8); + pci_or_config8(PCI_DEV(0, 0x1f, 0), 0xad, 3 << 0);
- reg32 = pci_read_config32(PCI_DEV(0, 0x1d, 7), 0xfc); - reg32 |= (1 << 29) | (1 << 17); - pci_write_config32(PCI_DEV(0, 0x1d, 7), 0xfc, reg32); + pci_or_config32(PCI_DEV(0, 0x1d, 7), 0xfc, (1 << 29) | (1 << 17));
- reg32 = pci_read_config32(PCI_DEV(0, 0x1d, 7), 0xdc); - reg32 |= (1 << 31) | (1 << 27); - pci_write_config32(PCI_DEV(0, 0x1d, 7), 0xdc, reg32); + pci_or_config32(PCI_DEV(0, 0x1d, 7), 0xdc, (1 << 31) | (1 << 27));
/* Enable IOAPIC */ RCBA8(OIC) = 0x03; diff --git a/src/southbridge/intel/i82801gx/lpc.c b/src/southbridge/intel/i82801gx/lpc.c index 0bfe8eb..e11e89d 100644 --- a/src/southbridge/intel/i82801gx/lpc.c +++ b/src/southbridge/intel/i82801gx/lpc.c @@ -252,18 +252,13 @@
static void i82801gx_configure_cstates(struct device *dev) { - u8 reg8; - - reg8 = pci_read_config8(dev, 0xa9); // Cx state configuration - reg8 |= (1 << 4) | (1 << 3) | (1 << 2); // Enable Popup & Popdown - pci_write_config8(dev, 0xa9, reg8); + // Enable Popup & Popdown + pci_or_config8(dev, 0xa9, (1 << 4) | (1 << 3) | (1 << 2));
// Set Deeper Sleep configuration to recommended values - reg8 = pci_read_config8(dev, 0xaa); - reg8 &= 0xf0; - reg8 |= (2 << 2); // Deeper Sleep to Stop CPU: 34-40us - reg8 |= (2 << 0); // Deeper Sleep to Sleep: 15us - pci_write_config8(dev, 0xaa, reg8); + // Deeper Sleep to Stop CPU: 34-40us + // Deeper Sleep to Sleep: 15us + pci_update_config8(dev, 0xaa, ~0x0f, (2 << 2) | (2 << 0)); }
static void i82801gx_rtc_init(struct device *dev) diff --git a/src/southbridge/intel/i82801gx/pci.c b/src/southbridge/intel/i82801gx/pci.c index 7472438..d72bd81 100644 --- a/src/southbridge/intel/i82801gx/pci.c +++ b/src/southbridge/intel/i82801gx/pci.c @@ -10,29 +10,21 @@ static void pci_init(struct device *dev) { u16 reg16; - u8 reg8;
/* Enable Bus Master */ - reg16 = pci_read_config16(dev, PCI_COMMAND); - reg16 |= PCI_COMMAND_MASTER; - pci_write_config16(dev, PCI_COMMAND, reg16); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER);
/* This device has no interrupt */ pci_write_config8(dev, INTR, 0xff);
- /* disable parity error response and SERR */ - reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); - reg16 &= ~PCI_BRIDGE_CTL_PARITY; - reg16 &= ~PCI_BRIDGE_CTL_SERR; - pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16); + /* Disable parity error response and SERR */ + pci_and_config16(dev, PCI_BRIDGE_CONTROL, + ~(PCI_BRIDGE_CTL_PARITY | PCI_BRIDGE_CTL_SERR));
/* Master Latency Count must be set to 0x04! */ - reg8 = pci_read_config8(dev, SMLT); - reg8 &= 0x07; - reg8 |= (0x04 << 3); - pci_write_config8(dev, SMLT, reg8); + pci_update_config8(dev, SMLT, 0x07, 0x04 << 3);
- /* Clear errors in status registers */ + /* Clear errors in status registers. FIXME: Do something? */ reg16 = pci_read_config16(dev, PSTS); //reg16 |= 0xf900; pci_write_config16(dev, PSTS, reg16); diff --git a/src/southbridge/intel/i82801gx/pcie.c b/src/southbridge/intel/i82801gx/pcie.c index adf0e49..c297ac9 100644 --- a/src/southbridge/intel/i82801gx/pcie.c +++ b/src/southbridge/intel/i82801gx/pcie.c @@ -41,7 +41,6 @@ static void pci_init(struct device *dev) { u16 reg16; - u32 reg32;
printk(BIOS_DEBUG, "Initializing ICH7 PCIe bridge.\n");
@@ -50,41 +49,29 @@
/* Set Cache Line Size to 0x10 */ // This has no effect but the OS might expect it - pci_write_config8(dev, PCI_CACHE_LINE_SIZE, 0x10); + pci_write_config8(dev, 0x0c, 0x10);
- reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); - reg16 &= ~PCI_BRIDGE_CTL_PARITY; - reg16 |= PCI_BRIDGE_CTL_NO_ISA; - pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16); + pci_update_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY, + PCI_BRIDGE_CTL_NO_ISA);
/* Enable IO xAPIC on this PCIe port */ - reg32 = pci_read_config32(dev, 0xd8); - reg32 |= (1 << 7); - pci_write_config32(dev, 0xd8, reg32); + pci_or_config32(dev, 0xd8, 1 << 7);
/* Enable Backbone Clock Gating */ - reg32 = pci_read_config32(dev, 0xe1); - reg32 |= (1 << 3) | (1 << 2) | (1 << 1) | (1 << 0); - pci_write_config32(dev, 0xe1, reg32); + pci_or_config32(dev, 0xe1, (1 << 3) | (1 << 2) | (1 << 1) | (1 << 0));
/* Set VC0 transaction class */ - reg32 = pci_read_config32(dev, 0x114); - reg32 &= 0xffffff00; - reg32 |= 1; - pci_write_config32(dev, 0x114, reg32); + pci_update_config32(dev, 0x114, ~0x000000ff, 1);
/* Mask completion timeouts */ - reg32 = pci_read_config32(dev, 0x148); - reg32 |= (1 << 14); - pci_write_config32(dev, 0x148, reg32); + pci_or_config32(dev, 0x148, 1 << 14);
/* Enable common clock configuration */ // Are there cases when we don't want that? - reg16 = pci_read_config16(dev, 0x50); - reg16 |= (1 << 6); - pci_write_config16(dev, 0x50, reg16); + pci_or_config16(dev, 0x50, 1 << 6);
#ifdef EVEN_MORE_DEBUG + u32 reg32; reg32 = pci_read_config32(dev, 0x20); printk(BIOS_SPEW, " MBL = 0x%08x\n", reg32); reg32 = pci_read_config32(dev, 0x24); @@ -95,7 +82,7 @@ printk(BIOS_SPEW, " PMLU32 = 0x%08x\n", reg32); #endif
- /* Clear errors in status registers */ + /* Clear errors in status registers. FIXME: Do something? */ reg16 = pci_read_config16(dev, 0x06); //reg16 |= 0xf900; pci_write_config16(dev, 0x06, reg16); diff --git a/src/southbridge/intel/i82801gx/sata.c b/src/southbridge/intel/i82801gx/sata.c index c98a645..1718253 100644 --- a/src/southbridge/intel/i82801gx/sata.c +++ b/src/southbridge/intel/i82801gx/sata.c @@ -61,11 +61,10 @@
if (config->sata_mode == SATA_MODE_AHCI) { /* Set map to ahci */ - pci_write_config8(dev, SATA_MAP, - (pci_read_config8(dev, SATA_MAP) & ~0xc3) | 0x40); + pci_update_config8(dev, SATA_MAP, (u8)~0xc3, 0x40); } else { - /* Set map to ide */ - pci_write_config8(dev, SATA_MAP, pci_read_config8(dev, SATA_MAP) & ~0xc3); + /* Set map to ide */ + pci_and_config8(dev, SATA_MAP, (u8)~0xc3); } /* At this point, the new pci id will appear on the bus */ } @@ -73,7 +72,6 @@ static void sata_init(struct device *dev) { u32 reg32; - u16 reg16; u8 ports;
/* Get the chip configuration */ @@ -96,11 +94,10 @@ case SATA_MODE_IDE_LEGACY_COMBINED: printk(BIOS_DEBUG, "SATA controller in combined mode.\n"); /* No AHCI: clear AHCI base */ - pci_write_config32(dev, PCI_BASE_ADDRESS_5, 0x00000000); + pci_write_config32(dev, PCI_BASE_ADDRESS_5, 0); + /* And without AHCI BAR no memory decoding */ - reg16 = pci_read_config16(dev, PCI_COMMAND); - reg16 &= ~PCI_COMMAND_MEMORY; - pci_write_config16(dev, PCI_COMMAND, reg16); + pci_and_config16(dev, PCI_COMMAND, ~PCI_COMMAND_MEMORY);
pci_write_config8(dev, 0x09, 0x80);
@@ -149,9 +146,7 @@ pci_write_config32(dev, PCI_BASE_ADDRESS_5, 0x00000000);
/* And without AHCI BAR no memory decoding */ - reg16 = pci_read_config16(dev, PCI_COMMAND); - reg16 &= ~PCI_COMMAND_MEMORY; - pci_write_config16(dev, PCI_COMMAND, reg16); + pci_and_config16(dev, PCI_COMMAND, ~PCI_COMMAND_MEMORY);
/* Native mode capable on both primary and secondary (0xa) * or'ed with enabled (0x50) = 0xf @@ -192,23 +187,15 @@ pci_write_config8(dev, 0xa0, 0x78); pci_write_config8(dev, 0xa6, 0x22); pci_write_config8(dev, 0xa0, 0x88); - reg32 = pci_read_config32(dev, 0xa4); - reg32 &= 0xc0c0c0c0; - reg32 |= 0x1b1b1212; - pci_write_config32(dev, 0xa4, reg32); + pci_update_config32(dev, 0xa4, 0xc0c0c0c0, 0x1b1b1212); pci_write_config8(dev, 0xa0, 0x8c); - reg32 = pci_read_config32(dev, 0xa4); - reg32 &= 0xc0c0ff00; - reg32 |= 0x121200aa; - pci_write_config32(dev, 0xa4, reg32); + pci_update_config32(dev, 0xa4, 0xc0c0ff00, 0x121200aa); pci_write_config8(dev, 0xa0, 0x00);
pci_write_config8(dev, PCI_INTERRUPT_LINE, 0);
/* Sata Initialization Register */ - reg32 = pci_read_config32(dev, SATA_IR); - reg32 |= SCRD; // due to some bug - pci_write_config32(dev, SATA_IR, reg32); + pci_or_config32(dev, SATA_IR, SCRD); // due to some bug }
static struct device_operations sata_ops = { diff --git a/src/southbridge/intel/i82801gx/usb.c b/src/southbridge/intel/i82801gx/usb.c index 8ce57df..08ba3d7 100644 --- a/src/southbridge/intel/i82801gx/usb.c +++ b/src/southbridge/intel/i82801gx/usb.c @@ -9,8 +9,6 @@
static void usb_init(struct device *dev) { - u8 reg8; - /* USB Specification says the device must be Bus Master */ printk(BIOS_DEBUG, "UHCI: Setting up controller.. ");
@@ -20,9 +18,7 @@ pci_write_config8(dev, 0xca, 0x00);
// Yes. Another Erratum - reg8 = pci_read_config8(dev, 0xca); - reg8 |= (1 << 0); - pci_write_config8(dev, 0xca, reg8); + pci_or_config8(dev, 0xca, 1 << 0);
printk(BIOS_DEBUG, "done.\n"); } diff --git a/src/southbridge/intel/i82801gx/usb_ehci.c b/src/southbridge/intel/i82801gx/usb_ehci.c index f665ab7..0a66136 100644 --- a/src/southbridge/intel/i82801gx/usb_ehci.c +++ b/src/southbridge/intel/i82801gx/usb_ehci.c @@ -14,19 +14,13 @@ struct resource *res; u8 *base; u32 reg32; - u8 reg8;
printk(BIOS_DEBUG, "EHCI: Setting up controller.. "); pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
- reg32 = pci_read_config32(dev, 0xdc); - reg32 |= (1 << 31) | (1 << 27); - pci_write_config32(dev, 0xdc, reg32); + pci_or_config32(dev, 0xdc, (1 << 31) | (1 << 27));
- reg32 = pci_read_config32(dev, 0xfc); - reg32 &= ~(3 << 2); - reg32 |= (2 << 2) | (1 << 29) | (1 << 17); - pci_write_config32(dev, 0xfc, reg32); + pci_update_config32(dev, 0xfc, ~(3 << 2), (2 << 2) | (1 << 29) | (1 << 17));
/* Clear any pending port changes */ res = find_resource(dev, 0x10); @@ -35,9 +29,7 @@ write32(base + 0x24, reg32);
/* workaround */ - reg8 = pci_read_config8(dev, 0x84); - reg8 |= (1 << 4); - pci_write_config8(dev, 0x84, reg8); + pci_or_config8(dev, 0x84, 1 << 4);
printk(BIOS_DEBUG, "done.\n"); }
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/azalia.c:
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... PS2, Line 201: ~0x00ff0000 please why don't you keep it '0xff00ffff' ?
(same for all files)
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/pcie.c:
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... PS2, Line 52: 0x0c please why 'PCI_CACHE_LINE_SIZE' is replaced by magic number ?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/azalia.c:
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... PS2, Line 201: ~0x00ff0000
please why don't you keep it '0xff00ffff' ? […]
We usually negate and-masks when doing RMW (Read Modify Write) ops. If I were to rewrite that one as `~(0xff << 16)`, it would make more sense 😄
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/pcie.c:
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... PS2, Line 52: 0x0c
please why 'PCI_CACHE_LINE_SIZE' is replaced by magic number ?
Because I copypasted the code from another southbridge. Will fix.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/azalia.c:
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... PS2, Line 201: ~0x00ff0000
We usually negate and-masks when doing RMW (Read Modify Write) ops. […]
it will give '0xFFFFFFFFFF00FFFF' :)
I'm somehow confused by the negation :p I would keep it '0xff00ffff'.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/azalia.c:
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... PS2, Line 201: ~0x00ff0000
it will give '0xFFFFFFFFFF00FFFF' :) […]
This is 32-bit code, and the negation implicitly promotes to int only, which is 32-bit.
In any case, this causes troubles with smaller types (which then need a cast) so it's going to be easier to do `Unset_And_Set_Mask` style operations (as in libgfxinit), which do not need a negation on the and-mask.
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42191
to look at the new patch set (#4).
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
sb/intel/i82801gx: Use PCI bitwise ops
While we are at it, also reflow a few lines that fit in 96 characters.
Tested with BUILD_TIMELESS=1, Getac P470 does not change.
Change-Id: I2cc3e71723e9b6898e6ec29f0f38b1b3b7446f09 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/bootblock.c M src/southbridge/intel/i82801gx/early_init.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801gx/pci.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/sata.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 9 files changed, 49 insertions(+), 135 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/42191/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/azalia.c:
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... PS2, Line 201: ~0x00ff0000
This is 32-bit code, and the negation implicitly promotes to int only, which is 32-bit. […]
Used ~(0xff << 16)
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/pcie.c:
https://review.coreboot.org/c/coreboot/+/42191/2/src/southbridge/intel/i8280... PS2, Line 52: 0x0c
Because I copypasted the code from another southbridge. Will fix.
Done
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42191/4/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/lpc.c:
https://review.coreboot.org/c/coreboot/+/42191/4/src/southbridge/intel/i8280... PS4, Line 261: ~0x0f I prefer the old mask: "0xf0"
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42191
to look at the new patch set (#7).
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
sb/intel/i82801gx: Use PCI bitwise ops
While we are at it, also reflow a few lines that fit in 96 characters.
Tested with BUILD_TIMELESS=1, Getac P470 does not change.
Change-Id: I2cc3e71723e9b6898e6ec29f0f38b1b3b7446f09 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/early_init.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801gx/pci.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/sata.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 8 files changed, 46 insertions(+), 128 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/42191/7
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42191
to look at the new patch set (#8).
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
sb/intel/i82801gx: Use PCI bitwise ops
While we are at it, also reflow a few lines that fit in 96 characters.
Tested with BUILD_TIMELESS=1, Getac P470 does not change.
Change-Id: I2cc3e71723e9b6898e6ec29f0f38b1b3b7446f09 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/early_init.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801gx/pci.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/sata.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 8 files changed, 46 insertions(+), 128 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/42191/8
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42191/4/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/lpc.c:
https://review.coreboot.org/c/coreboot/+/42191/4/src/southbridge/intel/i8280... PS4, Line 261: ~0x0f
I prefer the old mask: "0xf0"
Done
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
Patch Set 8: Code-Review+1
Hello build bot (Jenkins), Patrick Rudolph, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42191
to look at the new patch set (#10).
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
sb/intel/i82801gx: Use PCI bitwise ops
While we are at it, also reflow a few lines that fit in 96 characters.
Tested with BUILD_TIMELESS=1, Getac P470 does not change.
Change-Id: I2cc3e71723e9b6898e6ec29f0f38b1b3b7446f09 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/early_init.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801gx/pci.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/sata.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 8 files changed, 45 insertions(+), 127 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/42191/10
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
Patch Set 10: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42191 )
Change subject: sb/intel/i82801gx: Use PCI bitwise ops ......................................................................
sb/intel/i82801gx: Use PCI bitwise ops
While we are at it, also reflow a few lines that fit in 96 characters.
Tested with BUILD_TIMELESS=1, Getac P470 does not change.
Change-Id: I2cc3e71723e9b6898e6ec29f0f38b1b3b7446f09 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42191 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/early_init.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801gx/pci.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/sata.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 8 files changed, 45 insertions(+), 127 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/southbridge/intel/i82801gx/azalia.c b/src/southbridge/intel/i82801gx/azalia.c index cea75bd9..8d626ac 100644 --- a/src/southbridge/intel/i82801gx/azalia.c +++ b/src/southbridge/intel/i82801gx/azalia.c @@ -194,37 +194,21 @@ struct resource *res; u32 codec_mask; u8 reg8; - u32 reg32;
// ESD - reg32 = pci_read_config32(dev, 0x134); - reg32 &= 0xff00ffff; - reg32 |= (2 << 16); - pci_write_config32(dev, 0x134, reg32); + pci_update_config32(dev, 0x134, ~(0xff << 16), 2 << 16);
// Link1 description - reg32 = pci_read_config32(dev, 0x140); - reg32 &= 0xff00ffff; - reg32 |= (2 << 16); - pci_write_config32(dev, 0x140, reg32); + pci_update_config32(dev, 0x140, ~(0xff << 16), 2 << 16);
// Port VC0 Resource Control Register - reg32 = pci_read_config32(dev, 0x114); - reg32 &= 0xffffff00; - reg32 |= 1; - pci_write_config32(dev, 0x114, reg32); + pci_update_config32(dev, 0x114, ~(0xff << 0), 1);
// VCi traffic class - reg8 = pci_read_config8(dev, 0x44); - reg8 |= (7 << 0); // TC7 - pci_write_config8(dev, 0x44, reg8); + pci_or_config8(dev, 0x44, 7 << 0); // TC7
// VCi Resource Control - reg32 = pci_read_config32(dev, 0x120); - reg32 |= (1 << 31); - reg32 |= (1 << 24); // VCi ID - reg32 |= (0x80 << 0); // VCi map - pci_write_config32(dev, 0x120, reg32); + pci_or_config32(dev, 0x120, (1 << 31) | (1 << 24) | (0x80 << 0)); /* VCi ID and map */
/* Set Bus Master */ pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER); @@ -244,14 +228,11 @@ reg8 = pci_read_config8(dev, 0x40); printk(BIOS_DEBUG, "Azalia: codec type: %s\n", (reg8 & (1 << 1))?"Azalia":"AC97");
- // - reg8 = pci_read_config8(dev, 0x40); // Audio Control - reg8 |= 1; // Select Azalia mode. This needs to be controlled via devicetree.cb - pci_write_config8(dev, 0x40, reg8); + // Select Azalia mode. This needs to be controlled via devicetree.cb + pci_or_config8(dev, 0x40, 1); // Audio Control
- reg8 = pci_read_config8(dev, 0x4d); // Docking Status - reg8 &= ~(1 << 7); // Docking not supported - pci_write_config8(dev, 0x4d, reg8); + // Docking not supported + pci_and_config8(dev, 0x4d, (u8)~(1 << 7)); // Docking Status
res = find_resource(dev, 0x10); if (!res) diff --git a/src/southbridge/intel/i82801gx/early_init.c b/src/southbridge/intel/i82801gx/early_init.c index ef48ed8..72281ea 100644 --- a/src/southbridge/intel/i82801gx/early_init.c +++ b/src/southbridge/intel/i82801gx/early_init.c @@ -61,9 +61,6 @@ #if ENV_ROMSTAGE void i82801gx_early_init(void) { - uint8_t reg8; - uint32_t reg32; - enable_smbus();
printk(BIOS_DEBUG, "Setting up static southbridge registers..."); @@ -83,22 +80,14 @@ pci_write_config8(PCI_DEV(0, 0x1e, 0), SMLT, 0x20);
/* reset rtc power status */ - reg8 = pci_read_config8(PCI_DEV(0, 0x1f, 0), GEN_PMCON_3); - reg8 &= ~RTC_BATTERY_DEAD; - pci_write_config8(PCI_DEV(0, 0x1f, 0), GEN_PMCON_3, reg8); + pci_and_config8(PCI_DEV(0, 0x1f, 0), GEN_PMCON_3, ~RTC_BATTERY_DEAD);
/* USB transient disconnect */ - reg8 = pci_read_config8(PCI_DEV(0, 0x1f, 0), 0xad); - reg8 |= (3 << 0); - pci_write_config8(PCI_DEV(0, 0x1f, 0), 0xad, reg8); + pci_or_config8(PCI_DEV(0, 0x1f, 0), 0xad, 3 << 0);
- reg32 = pci_read_config32(PCI_DEV(0, 0x1d, 7), 0xfc); - reg32 |= (1 << 29) | (1 << 17); - pci_write_config32(PCI_DEV(0, 0x1d, 7), 0xfc, reg32); + pci_or_config32(PCI_DEV(0, 0x1d, 7), 0xfc, (1 << 29) | (1 << 17));
- reg32 = pci_read_config32(PCI_DEV(0, 0x1d, 7), 0xdc); - reg32 |= (1 << 31) | (1 << 27); - pci_write_config32(PCI_DEV(0, 0x1d, 7), 0xdc, reg32); + pci_or_config32(PCI_DEV(0, 0x1d, 7), 0xdc, (1 << 31) | (1 << 27));
/* Enable IOAPIC */ RCBA8(OIC) = 0x03; diff --git a/src/southbridge/intel/i82801gx/lpc.c b/src/southbridge/intel/i82801gx/lpc.c index 0c76781..90a70e4 100644 --- a/src/southbridge/intel/i82801gx/lpc.c +++ b/src/southbridge/intel/i82801gx/lpc.c @@ -247,18 +247,13 @@
static void i82801gx_configure_cstates(struct device *dev) { - u8 reg8; - - reg8 = pci_read_config8(dev, 0xa9); // Cx state configuration - reg8 |= (1 << 4) | (1 << 3) | (1 << 2); // Enable Popup & Popdown - pci_write_config8(dev, 0xa9, reg8); + // Enable Popup & Popdown + pci_or_config8(dev, 0xa9, (1 << 4) | (1 << 3) | (1 << 2));
// Set Deeper Sleep configuration to recommended values - reg8 = pci_read_config8(dev, 0xaa); - reg8 &= 0xf0; - reg8 |= (2 << 2); // Deeper Sleep to Stop CPU: 34-40us - reg8 |= (2 << 0); // Deeper Sleep to Sleep: 15us - pci_write_config8(dev, 0xaa, reg8); + // Deeper Sleep to Stop CPU: 34-40us + // Deeper Sleep to Sleep: 15us + pci_update_config8(dev, 0xaa, 0xf0, (2 << 2) | (2 << 0)); }
static void i82801gx_rtc_init(struct device *dev) diff --git a/src/southbridge/intel/i82801gx/pci.c b/src/southbridge/intel/i82801gx/pci.c index 7472438..d72bd81 100644 --- a/src/southbridge/intel/i82801gx/pci.c +++ b/src/southbridge/intel/i82801gx/pci.c @@ -10,29 +10,21 @@ static void pci_init(struct device *dev) { u16 reg16; - u8 reg8;
/* Enable Bus Master */ - reg16 = pci_read_config16(dev, PCI_COMMAND); - reg16 |= PCI_COMMAND_MASTER; - pci_write_config16(dev, PCI_COMMAND, reg16); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER);
/* This device has no interrupt */ pci_write_config8(dev, INTR, 0xff);
- /* disable parity error response and SERR */ - reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); - reg16 &= ~PCI_BRIDGE_CTL_PARITY; - reg16 &= ~PCI_BRIDGE_CTL_SERR; - pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16); + /* Disable parity error response and SERR */ + pci_and_config16(dev, PCI_BRIDGE_CONTROL, + ~(PCI_BRIDGE_CTL_PARITY | PCI_BRIDGE_CTL_SERR));
/* Master Latency Count must be set to 0x04! */ - reg8 = pci_read_config8(dev, SMLT); - reg8 &= 0x07; - reg8 |= (0x04 << 3); - pci_write_config8(dev, SMLT, reg8); + pci_update_config8(dev, SMLT, 0x07, 0x04 << 3);
- /* Clear errors in status registers */ + /* Clear errors in status registers. FIXME: Do something? */ reg16 = pci_read_config16(dev, PSTS); //reg16 |= 0xf900; pci_write_config16(dev, PSTS, reg16); diff --git a/src/southbridge/intel/i82801gx/pcie.c b/src/southbridge/intel/i82801gx/pcie.c index ca0ae2e..8650673 100644 --- a/src/southbridge/intel/i82801gx/pcie.c +++ b/src/southbridge/intel/i82801gx/pcie.c @@ -41,7 +41,6 @@ static void pci_init(struct device *dev) { u16 reg16; - u32 reg32;
printk(BIOS_DEBUG, "Initializing ICH7 PCIe bridge.\n");
@@ -52,38 +51,25 @@ // This has no effect but the OS might expect it pci_write_config8(dev, PCI_CACHE_LINE_SIZE, 0x10);
- reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); - reg16 &= ~PCI_BRIDGE_CTL_PARITY; - pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16); + pci_and_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY);
/* Enable IO xAPIC on this PCIe port */ - reg32 = pci_read_config32(dev, 0xd8); - reg32 |= (1 << 7); - pci_write_config32(dev, 0xd8, reg32); + pci_or_config32(dev, 0xd8, 1 << 7);
/* Enable Backbone Clock Gating */ - reg32 = pci_read_config32(dev, 0xe1); - reg32 |= (1 << 3) | (1 << 2) | (1 << 1) | (1 << 0); - pci_write_config32(dev, 0xe1, reg32); + pci_or_config32(dev, 0xe1, (1 << 3) | (1 << 2) | (1 << 1) | (1 << 0));
/* Set VC0 transaction class */ - reg32 = pci_read_config32(dev, 0x114); - reg32 &= 0xffffff00; - reg32 |= 1; - pci_write_config32(dev, 0x114, reg32); + pci_update_config32(dev, 0x114, ~0x000000ff, 1);
/* Mask completion timeouts */ - reg32 = pci_read_config32(dev, 0x148); - reg32 |= (1 << 14); - pci_write_config32(dev, 0x148, reg32); + pci_or_config32(dev, 0x148, 1 << 14);
/* Enable common clock configuration */ // Are there cases when we don't want that? - reg16 = pci_read_config16(dev, 0x50); - reg16 |= (1 << 6); - pci_write_config16(dev, 0x50, reg16); + pci_or_config16(dev, 0x50, 1 << 6);
- /* Clear errors in status registers */ + /* Clear errors in status registers. FIXME: Do something? */ reg16 = pci_read_config16(dev, 0x06); //reg16 |= 0xf900; pci_write_config16(dev, 0x06, reg16); diff --git a/src/southbridge/intel/i82801gx/sata.c b/src/southbridge/intel/i82801gx/sata.c index 715d670..abb0e2e 100644 --- a/src/southbridge/intel/i82801gx/sata.c +++ b/src/southbridge/intel/i82801gx/sata.c @@ -59,11 +59,10 @@
if (config->sata_mode == SATA_MODE_AHCI) { /* Set map to ahci */ - pci_write_config8(dev, SATA_MAP, - (pci_read_config8(dev, SATA_MAP) & ~0xc3) | 0x40); + pci_update_config8(dev, SATA_MAP, (u8)~0xc3, 0x40); } else { - /* Set map to ide */ - pci_write_config8(dev, SATA_MAP, pci_read_config8(dev, SATA_MAP) & ~0xc3); + /* Set map to ide */ + pci_and_config8(dev, SATA_MAP, (u8)~0xc3); } /* At this point, the new pci id will appear on the bus */ } @@ -71,7 +70,6 @@ static void sata_init(struct device *dev) { u32 reg32; - u16 reg16; u8 ports;
/* Get the chip configuration */ @@ -95,11 +93,10 @@ case SATA_MODE_IDE_LEGACY_COMBINED: printk(BIOS_DEBUG, "SATA controller in combined mode.\n"); /* No AHCI: clear AHCI base */ - pci_write_config32(dev, PCI_BASE_ADDRESS_5, 0x00000000); + pci_write_config32(dev, PCI_BASE_ADDRESS_5, 0); + /* And without AHCI BAR no memory decoding */ - reg16 = pci_read_config16(dev, PCI_COMMAND); - reg16 &= ~PCI_COMMAND_MEMORY; - pci_write_config16(dev, PCI_COMMAND, reg16); + pci_and_config16(dev, PCI_COMMAND, ~PCI_COMMAND_MEMORY);
pci_write_config8(dev, 0x09, 0x80);
@@ -148,9 +145,7 @@ pci_write_config32(dev, PCI_BASE_ADDRESS_5, 0x00000000);
/* And without AHCI BAR no memory decoding */ - reg16 = pci_read_config16(dev, PCI_COMMAND); - reg16 &= ~PCI_COMMAND_MEMORY; - pci_write_config16(dev, PCI_COMMAND, reg16); + pci_and_config16(dev, PCI_COMMAND, ~PCI_COMMAND_MEMORY);
/* Native mode capable on both primary and secondary (0xa) * or'ed with enabled (0x50) = 0xf @@ -191,23 +186,15 @@ pci_write_config8(dev, 0xa0, 0x78); pci_write_config8(dev, 0xa6, 0x22); pci_write_config8(dev, 0xa0, 0x88); - reg32 = pci_read_config32(dev, 0xa4); - reg32 &= 0xc0c0c0c0; - reg32 |= 0x1b1b1212; - pci_write_config32(dev, 0xa4, reg32); + pci_update_config32(dev, 0xa4, 0xc0c0c0c0, 0x1b1b1212); pci_write_config8(dev, 0xa0, 0x8c); - reg32 = pci_read_config32(dev, 0xa4); - reg32 &= 0xc0c0ff00; - reg32 |= 0x121200aa; - pci_write_config32(dev, 0xa4, reg32); + pci_update_config32(dev, 0xa4, 0xc0c0ff00, 0x121200aa); pci_write_config8(dev, 0xa0, 0x00);
pci_write_config8(dev, PCI_INTERRUPT_LINE, 0);
/* Sata Initialization Register */ - reg32 = pci_read_config32(dev, SATA_IR); - reg32 |= SCRD; // due to some bug - pci_write_config32(dev, SATA_IR, reg32); + pci_or_config32(dev, SATA_IR, SCRD); // due to some bug }
static struct device_operations sata_ops = { diff --git a/src/southbridge/intel/i82801gx/usb.c b/src/southbridge/intel/i82801gx/usb.c index 8ce57df..08ba3d7 100644 --- a/src/southbridge/intel/i82801gx/usb.c +++ b/src/southbridge/intel/i82801gx/usb.c @@ -9,8 +9,6 @@
static void usb_init(struct device *dev) { - u8 reg8; - /* USB Specification says the device must be Bus Master */ printk(BIOS_DEBUG, "UHCI: Setting up controller.. ");
@@ -20,9 +18,7 @@ pci_write_config8(dev, 0xca, 0x00);
// Yes. Another Erratum - reg8 = pci_read_config8(dev, 0xca); - reg8 |= (1 << 0); - pci_write_config8(dev, 0xca, reg8); + pci_or_config8(dev, 0xca, 1 << 0);
printk(BIOS_DEBUG, "done.\n"); } diff --git a/src/southbridge/intel/i82801gx/usb_ehci.c b/src/southbridge/intel/i82801gx/usb_ehci.c index f665ab7..0a66136 100644 --- a/src/southbridge/intel/i82801gx/usb_ehci.c +++ b/src/southbridge/intel/i82801gx/usb_ehci.c @@ -14,19 +14,13 @@ struct resource *res; u8 *base; u32 reg32; - u8 reg8;
printk(BIOS_DEBUG, "EHCI: Setting up controller.. "); pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
- reg32 = pci_read_config32(dev, 0xdc); - reg32 |= (1 << 31) | (1 << 27); - pci_write_config32(dev, 0xdc, reg32); + pci_or_config32(dev, 0xdc, (1 << 31) | (1 << 27));
- reg32 = pci_read_config32(dev, 0xfc); - reg32 &= ~(3 << 2); - reg32 |= (2 << 2) | (1 << 29) | (1 << 17); - pci_write_config32(dev, 0xfc, reg32); + pci_update_config32(dev, 0xfc, ~(3 << 2), (2 << 2) | (1 << 29) | (1 << 17));
/* Clear any pending port changes */ res = find_resource(dev, 0x10); @@ -35,9 +29,7 @@ write32(base + 0x24, reg32);
/* workaround */ - reg8 = pci_read_config8(dev, 0x84); - reg8 |= (1 << 4); - pci_write_config8(dev, 0x84, reg8); + pci_or_config8(dev, 0x84, 1 << 4);
printk(BIOS_DEBUG, "done.\n"); }
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
HAOUAS Elyes (Code Review)