HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37269 )
Change subject: i945/early_init.c: Use BIT(x) macro ......................................................................
i945/early_init.c: Use BIT(x) macro
Change-Id: I78634ea4f51deb82e98939abbed8f07ce98c117b Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/intel/i945/early_init.c 1 file changed, 46 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/37269/1
diff --git a/src/northbridge/intel/i945/early_init.c b/src/northbridge/intel/i945/early_init.c index 13dce61..f0f6261 100644 --- a/src/northbridge/intel/i945/early_init.c +++ b/src/northbridge/intel/i945/early_init.c @@ -173,7 +173,7 @@ which requires to have TSEG_BASE aligned to TSEG_SIZE. */ reg8 = pci_read_config8(PCI_DEV(0, 0, 0), ESMRAMC); reg8 &= ~0x7; - reg8 |= (1 << 1) | (1 << 0); /* 2M and TSEG_Enable */ + reg8 |= BIT(1) | BIT(0); /* 2M and TSEG_Enable */ pci_write_config8(PCI_DEV(0, 0, 0), ESMRAMC, reg8);
/* Set C0000-FFFFF to access RAM on both reads and writes */ @@ -262,17 +262,17 @@
/* Is internal graphics enabled? */ if (pci_read_config8(PCI_DEV(0, 0x0, 0), DEVEN) & (DEVEN_D2F0 | DEVEN_D2F1)) - MCHBAR32(MMARB1) |= (1 << 17); + MCHBAR32(MMARB1) |= BIT(17);
/* Assign Virtual Channel ID 1 to VC1 */ reg32 = EPBAR32(EPVC1RCTL); reg32 &= ~(7 << 24); - reg32 |= (1 << 24); + reg32 |= BIT(24); EPBAR32(EPVC1RCTL) = reg32;
reg32 = EPBAR32(EPVC1RCTL); reg32 &= 0xffffff01; - reg32 |= (1 << 7); + reg32 |= BIT(7); EPBAR32(EPVC1RCTL) = reg32;
EPBAR32(PORTARB + 0x00) = 0x01000001; @@ -284,8 +284,8 @@ EPBAR32(PORTARB + 0x18) = 0x00001000; EPBAR32(PORTARB + 0x1c) = 0x00000040;
- EPBAR32(EPVC1RCTL) |= (1 << 16); - EPBAR32(EPVC1RCTL) |= (1 << 16); + EPBAR32(EPVC1RCTL) |= BIT(16); + EPBAR32(EPVC1RCTL) |= BIT(16);
printk(BIOS_DEBUG, "Loading port arbitration table ..."); /* Loop until bit 0 becomes 0 */ @@ -298,12 +298,12 @@ printk(BIOS_DEBUG, "ok\n");
/* Now enable VC1 */ - EPBAR32(EPVC1RCTL) |= (1 << 31); + EPBAR32(EPVC1RCTL) |= BIT(31);
printk(BIOS_DEBUG, "Wait for VC1 negotiation ..."); /* Wait for VC1 negotiation pending */ timeout = 0x7fff; - while ((EPBAR16(EPVC1RSTS) & (1 << 1)) && --timeout) + while ((EPBAR16(EPVC1RSTS) & BIT(1)) && --timeout) ; if (!timeout) printk(BIOS_DEBUG, "timeout!\n"); @@ -334,7 +334,7 @@
reg32 = RCBA32(V1CTL); reg32 &= ~((0x7f << 1) | (7 << 17) | (7 << 24)); - reg32 |= (0x40 << 1) | (4 << 17) | (1 << 24) | (1 << 31); + reg32 |= (0x40 << 1) | (4 << 17) | BIT(24) | BIT(31); RCBA32(V1CTL) = reg32;
RCBA32(LCAP) |= (3 << 10); @@ -360,21 +360,21 @@
reg32 = DMIBAR32(DMIVC1RCTL); reg32 &= ~(7 << 24); - reg32 |= (1 << 24); /* NOTE: This ID must match ICH7 side */ + reg32 |= BIT(24); /* NOTE: This ID must match ICH7 side */ DMIBAR32(DMIVC1RCTL) = reg32;
reg32 = DMIBAR32(DMIVC1RCTL); reg32 &= 0xffffff01; - reg32 |= (1 << 7); + reg32 |= BIT(7); DMIBAR32(DMIVC1RCTL) = reg32;
/* Now enable VC1 */ - DMIBAR32(DMIVC1RCTL) |= (1 << 31); + DMIBAR32(DMIVC1RCTL) |= BIT(31);
printk(BIOS_DEBUG, "Wait for VC1 negotiation ..."); /* Wait for VC1 negotiation pending */ timeout = 0x7ffff; - while ((DMIBAR16(DMIVC1RSTS) & (1 << 1)) && --timeout) + while ((DMIBAR16(DMIVC1RSTS) & BIT(1)) && --timeout) ; if (!timeout) printk(BIOS_DEBUG, "timeout!\n"); @@ -395,9 +395,9 @@ reg32 = DMIBAR32(DMICC); reg32 &= 0x00ffffff; reg32 &= ~(3 << 0); - reg32 |= (1 << 0); + reg32 |= BIT(0); reg32 &= ~(3 << 20); - reg32 |= (1 << 20); + reg32 |= BIT(20);
DMIBAR32(DMICC) = reg32;
@@ -424,14 +424,14 @@
if (pci_read_config8(PCI_DEV(0, 0x0, 0), DEVEN) & (DEVEN_D2F0 | DEVEN_D2F1)) { printk(BIOS_DEBUG, "Internal graphics: enabled\n"); - DMIBAR32(0x200) |= (1 << 21); + DMIBAR32(0x200) |= BIT(21); } else { printk(BIOS_DEBUG, "Internal graphics: disabled\n"); - DMIBAR32(0x200) &= ~(1 << 21); + DMIBAR32(0x200) &= ~BIT(21); }
reg32 = DMIBAR32(0x204); - reg32 &= ~((1 << 11) | (1 << 10)); + reg32 &= ~(BIT(11) | BIT(10)); DMIBAR32(0x204) = reg32;
reg32 = DMIBAR32(0x204); @@ -446,8 +446,8 @@ reg32 |= (0x02 << 26); DMIBAR32(0x200) = reg32;
- DMIBAR32(DMIDRCCFG) &= ~(1 << 31); - DMIBAR32(DMICTL2) |= (1 << 31); + DMIBAR32(DMIDRCCFG) &= ~BIT(31); + DMIBAR32(DMICTL2) |= BIT(31);
if (i945_silicon_revision() >= 3) { reg32 = DMIBAR32(0xec0); @@ -474,7 +474,7 @@ /* wait for bit toggle to 0 */ printk(BIOS_DEBUG, "Waiting for DMI hardware..."); timeout = 0x7fffff; - while ((DMIBAR8(0x32) & (1 << 1)) && --timeout) + while ((DMIBAR8(0x32) & BIT(1)) && --timeout) ; if (!timeout) printk(BIOS_DEBUG, "timeout!\n"); @@ -494,7 +494,7 @@ DMIBAR32(0x334) = DMIBAR32(0x334); DMIBAR32(0x338) = DMIBAR32(0x338);
- if (i945_silicon_revision() == 1 && (MCHBAR8(DFT_STRAP1) & (1 << 5))) { + if (i945_silicon_revision() == 1 && (MCHBAR8(DFT_STRAP1) & BIT(5))) { if ((MCHBAR32(0x214) & 0xf) != 0x3) { printk(BIOS_INFO, "DMI link requires A1 stepping workaround. Rebooting.\n"); reg32 = DMIBAR32(0x224); @@ -523,7 +523,7 @@ pci_write_config16(PCI_DEV(0, 0x00, 0), DEVEN, reg16);
reg32 = pci_read_config32(p2peg, PEGCC); - reg32 &= ~(1 << 8); + reg32 &= ~BIT(8); pci_write_config32(p2peg, PEGCC, reg32);
/* We have no success with querying the usual PCIe registers @@ -539,20 +539,20 @@ printk(BIOS_DEBUG, "SLOTSTS: %04x\n", reg16); if (!(reg16 & 0x48)) goto disable_pciexpress_x16_link; - reg16 |= (1 << 4) | (1 << 0); + reg16 |= BIT(4) | BIT(0); pci_write_config16(p2peg, SLOTSTS, reg16);
pci_s_bridge_set_secondary(p2peg, tmp_secondary);
reg32 = pci_read_config32(p2peg, 0x224); - reg32 &= ~(1 << 8); + reg32 &= ~BIT(8); pci_write_config32(p2peg, 0x224, reg32);
- MCHBAR16(UPMC1) &= ~((1 << 5) | (1 << 0)); + MCHBAR16(UPMC1) &= ~(BIT(5) | BIT(0));
/* Initialize PEG_CAP */ reg16 = pci_read_config16(p2peg, PEG_CAP); - reg16 |= (1 << 8); + reg16 |= BIT(8); pci_write_config16(p2peg, PEG_CAP, reg16);
/* Setup SLOTCAP */ @@ -630,7 +630,7 @@ printk(BIOS_DEBUG, "PCIe device class: %06x\n", reg32); if (reg32 == 0x030000) { printk(BIOS_DEBUG, "PCIe device is VGA. Disabling IGD.\n"); - reg16 = (1 << 1); + reg16 = BIT(1); pci_write_config16(PCI_DEV(0, 0x0, 0), GGC, reg16);
reg32 = pci_read_config32(PCI_DEV(0, 0x0, 0), DEVEN); @@ -640,7 +640,7 @@
/* Enable GPEs */ reg32 = pci_read_config32(p2peg, PEG_LC); - reg32 |= (1 << 2) | (1 << 1) | (1 << 0); /* PMEGPE, HPGPE, GENGPE */ + reg32 |= BIT(2) | BIT(1) | BIT(0); /* PMEGPE, HPGPE, GENGPE */ pci_write_config32(p2peg, PEG_LC, reg32);
/* Virtual Channel Configuration: Only VC0 on PCIe x16 */ @@ -689,7 +689,7 @@ pci_write_config32(p2peg, 0xf0, reg32);
reg32 = pci_read_config32(p2peg, 0xf0); - reg32 |= (1 << 5); + reg32 |= BIT(5); pci_write_config32(p2peg, 0xf0, reg32);
reg32 = pci_read_config32(p2peg, 0x200); @@ -699,17 +699,17 @@
reg32 = pci_read_config32(p2peg, 0xe80); if (i945_silicon_revision() >= 2) - reg32 |= (1 << 12); + reg32 |= BIT(12); else - reg32 &= ~(1 << 12); + reg32 &= ~BIT(12); pci_write_config32(p2peg, 0xe80, reg32);
reg32 = pci_read_config32(p2peg, 0xeb4); - reg32 &= ~(1 << 31); + reg32 &= ~BIT(31); pci_write_config32(p2peg, 0xeb4, reg32);
reg32 = pci_read_config32(p2peg, 0xfc); - reg32 |= (1 << 31); + reg32 |= BIT(31); pci_write_config32(p2peg, 0xfc, reg32);
if (i945_silicon_revision() >= 3) { @@ -732,7 +732,7 @@ /* Set voltage specific parameters */ reg32 = pci_read_config32(p2peg, 0xe80); reg32 &= (0xf << 4); /* Default case 1.05V */ - if ((MCHBAR32(DFT_STRAP1) & (1 << 20)) == 0) { /* 1.50V */ + if ((MCHBAR32(DFT_STRAP1) & BIT(20)) == 0) { /* 1.50V */ reg32 |= (7 << 4); } pci_write_config32(p2peg, 0xe80, reg32); @@ -744,13 +744,13 @@ /* For now we just disable the x16 link */ printk(BIOS_DEBUG, "Disabling PCI Express x16 Link\n");
- MCHBAR16(UPMC1) |= (1 << 5) | (1 << 0); + MCHBAR16(UPMC1) |= BIT(5) | BIT(0);
/* Toggle PCIRST# */ pci_s_assert_secondary_reset(p2peg);
reg32 = pci_read_config32(p2peg, 0x224); - reg32 |= (1 << 8); + reg32 |= BIT(8); pci_write_config32(p2peg, 0x224, reg32);
pci_s_deassert_secondary_reset(p2peg); @@ -781,14 +781,14 @@
reg32 = EPBAR32(EPESD); reg32 &= 0xff00ffff; - reg32 |= (1 << 16); + reg32 |= BIT(16); EPBAR32(EPESD) = reg32;
- EPBAR32(EPLE1D) |= (1 << 16) | (1 << 0); + EPBAR32(EPLE1D) |= BIT(16) | BIT(0);
EPBAR32(EPLE1A) = (uintptr_t)DEFAULT_DMIBAR;
- EPBAR32(EPLE2D) |= (1 << 16) | (1 << 0); + EPBAR32(EPLE2D) |= BIT(16) | BIT(0);
/* DMI Port Root Topology */
@@ -798,12 +798,12 @@ reg32 &= 0xff00ffff; reg32 |= (2 << 16);
- reg32 |= (1 << 0); + reg32 |= BIT(0); DMIBAR32(DMILE1D) = reg32;
DMIBAR32(DMILE1A) = (uintptr_t)DEFAULT_RCBA;
- DMIBAR32(DMILE2D) |= (1 << 16) | (1 << 0); + DMIBAR32(DMILE2D) |= BIT(16) | BIT(0);
DMIBAR32(DMILE2A) = DEFAULT_EPBAR;
@@ -811,7 +811,7 @@ if (pci_read_config8(PCI_DEV(0, 0x00, 0), DEVEN) & DEVEN_D1F0) { pci_write_config32(p2peg, LE1A, DEFAULT_EPBAR); reg32 = pci_read_config32(p2peg, LE1D); - reg32 |= (1 << 0); + reg32 |= BIT(0); pci_write_config32(p2peg, LE1D, reg32); } } @@ -822,7 +822,7 @@
RCBA32(ESD) |= (2 << 16);
- RCBA32(ULD) |= (1 << 24) | (1 << 16); + RCBA32(ULD) |= BIT(24) | BIT(16);
RCBA32(ULBA) = (uintptr_t)DEFAULT_DMIBAR; /* Write ESD.CID to TCID */ @@ -837,7 +837,7 @@
static void ich7_setup_pci_express(void) { - RCBA32(CG) |= (1 << 0); + RCBA32(CG) |= BIT(0);
/* Initialize slot power limit for root ports */ pci_write_config32(PCI_DEV(0, 0x1c, 0), 0x54, 0x00000060); @@ -869,7 +869,7 @@ RCBA32(GCS) &= (~0x04);
/* Just do it that way */ - RCBA32(0x2010) |= (1 << 10); + RCBA32(0x2010) |= BIT(10); }
static void i945_prepare_resume(int s3resume)
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37269 )
Change subject: i945/early_init.c: Use BIT(x) macro ......................................................................
Patch Set 1:
I don't really like such changes. Did you check that *all* those bits in the datasheets are single bit configuration options?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37269 )
Change subject: i945/early_init.c: Use BIT(x) macro ......................................................................
Patch Set 1:
My very personal opinion: All-caps macros in the code are ugly and if there are too many of them, it's harder to read. BIT() is not that bad, there are worse. But still, I don't see an improvement here.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37269 )
Change subject: i945/early_init.c: Use BIT(x) macro ......................................................................
Patch Set 1:
Patch Set 1:
I don't really like such changes. Did you check that *all* those bits in the datasheets are single bit configuration options?
No, there are some undocumented registers ...
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37269 )
Change subject: i945/early_init.c: Use BIT(x) macro ......................................................................
Patch Set 1:
Patch Set 1:
My very personal opinion: All-caps macros in the code are ugly and if there are too many of them, it's harder to read. BIT() is not that bad, there are worse. But still, I don't see an improvement here.
ok, understood :) I'll drop this CL
Thank you for the review
HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37269 )
Change subject: i945/early_init.c: Use BIT(x) macro ......................................................................
Abandoned
-
Arthur Heymans (Code Review) -
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)