Change in coreboot[master]: i945/early_init.c: Use BIT(x) macro

[HAOUAS Elyes (Code Review)]

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 
---
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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code 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