Change in coreboot[master]: [WIP] nb/intel/sandybridge: Use macros for DMI init

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39634 )

Change subject: [WIP] nb/intel/sandybridge: Use macros for DMI init
......................................................................

[WIP] nb/intel/sandybridge: Use macros for DMI init

Put names to DMI registers, and use for-each style macros.

With BUILD_TIMELESS=1, the binary of ASUS P8Z77-V LX2 remains identical.

Change-Id: I3c155f30b5642828c958cd2650aad5547b58d00b
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/sandybridge/early_dmi.c
M src/northbridge/intel/sandybridge/sandybridge.h
2 files changed, 113 insertions(+), 87 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/39634/1

diff --git a/src/northbridge/intel/sandybridge/early_dmi.c b/src/northbridge/intel/sandybridge/early_dmi.c
index 0ffabd1..82a1292 100644
--- a/src/northbridge/intel/sandybridge/early_dmi.c
+++ b/src/northbridge/intel/sandybridge/early_dmi.c
@@ -18,99 +18,99 @@

void early_init_dmi(void)
 {
-	int i;
+	int lane, bundle;

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0914 + (i << 5)) |= 1 << 31;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(LOADBUSCTL0(bundle)) |= 1 << 31;
    }

-	for (i = 0; i < 4; i++) {
-		DMIBAR32(0x0a00 + (i << 4)) &= ~0x0c000000;
-		DMIBAR32(0x0a04 + (i << 4)) |= (1 << 11);
+	FOR_EACH_P_LANE(lane) {
+		DMIBAR32(AFELNxCFG0(lane)) &= ~0x0c000000;
+		DMIBAR32(AFELNxCFG1(lane)) |= (1 << 11);
    }
-	DMIBAR32(0x0c30) = (DMIBAR32(0x0c30) & 0x0fffffff) | (1 << 30);
+	DMIBAR32(AFECMNCFG7) = (DMIBAR32(AFECMNCFG7) & 0x0fffffff) | (1 << 30);

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0904 + (i << 5)) &= ~0x01c00000;
-		DMIBAR32(0x090c + (i << 5)) &= ~0x000e0000;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG1(bundle)) &= ~0x01c00000;
+		DMIBAR32(AFEBNDxCFG3(bundle)) &= ~0x000e0000;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x090c + (i << 5)) &= ~0x01e00000;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG3(bundle)) &= ~0x01e00000;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0904 + (i << 5));	// !!! = 0x7a1842ec
-		DMIBAR32(0x0904 + (i << 5)) = 0x7a1842ec;
-		DMIBAR32(0x090c + (i << 5));	// !!! = 0x00000208
-		DMIBAR32(0x090c + (i << 5)) = 0x00000128;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG1(bundle));	// !!! = 0x7a1842ec
+		DMIBAR32(AFEBNDxCFG1(bundle)) = 0x7a1842ec;
+		DMIBAR32(AFEBNDxCFG3(bundle));	// !!! = 0x00000208
+		DMIBAR32(AFEBNDxCFG3(bundle)) = 0x00000128;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0700 + (i << 5));	// !!! = 0x46139008
-		DMIBAR32(0x0700 + (i << 5)) = 0x46139008;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(MAGIC_CHICKEN(bundle));	// !!! = 0x46139008
+		DMIBAR32(MAGIC_CHICKEN(bundle)) = 0x46139008;
    }

DMIBAR32(0x0c04);	// !!! = 0x2e680008
    DMIBAR32(0x0c04) = 0x2e680008;

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0904 + (i << 5));	// !!! = 0x7a1842ec
-		DMIBAR32(0x0904 + (i << 5)) = 0x3a1842ec;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG1(bundle));	// !!! = 0x7a1842ec
+		DMIBAR32(AFEBNDxCFG1(bundle)) = 0x3a1842ec;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0910 + (i << 5));	// !!! = 0x00006300
-		DMIBAR32(0x0910 + (i << 5)) = 0x00004300;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG4(bundle));	// !!! = 0x00006300
+		DMIBAR32(AFEBNDxCFG4(bundle)) = 0x00004300;
    }

-	for (i = 0; i < 4; i++) {
-		DMIBAR32(0x0a00 + (i << 4));	// !!! = 0x03042010
-		DMIBAR32(0x0a00 + (i << 4)) = 0x03042018;
+	FOR_EACH_P_LANE(lane) {
+		DMIBAR32(AFELNxCFG0(lane));	// !!! = 0x03042010
+		DMIBAR32(AFELNxCFG0(lane)) = 0x03042018;
    }

DMIBAR32(0x0c00);	// !!! = 0x29700c08
    DMIBAR32(0x0c00) = 0x29700c08;

-	for (i = 0; i < 4; i++) {
-		DMIBAR32(0x0a04 + (i << 4));	// !!! = 0x0c0708f0
-		DMIBAR32(0x0a04 + (i << 4)) = 0x0c0718f0;
+	FOR_EACH_P_LANE(lane) {
+		DMIBAR32(AFELNxCFG1(lane));	// !!! = 0x0c0708f0
+		DMIBAR32(AFELNxCFG1(lane)) = 0x0c0718f0;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0900 + (i << 5));	// !!! = 0x50000000
-		DMIBAR32(0x0900 + (i << 5)) = 0x50000000;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG0(bundle));	// !!! = 0x50000000
+		DMIBAR32(AFEBNDxCFG0(bundle)) = 0x50000000;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0908 + (i << 5));	// !!! = 0x51ffffff
-		DMIBAR32(0x0908 + (i << 5)) = 0x51ffffff;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG2(bundle));	// !!! = 0x51ffffff
+		DMIBAR32(AFEBNDxCFG2(bundle)) = 0x51ffffff;
    }

-	for (i = 0; i < 4; i++) {
-		DMIBAR32(0x0a00 + (i << 4));	// !!! = 0x03042018
-		DMIBAR32(0x0a00 + (i << 4)) = 0x03042018;
+	FOR_EACH_P_LANE(lane) {
+		DMIBAR32(AFELNxCFG0(lane));	// !!! = 0x03042018
+		DMIBAR32(AFELNxCFG0(lane)) = 0x03042018;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0700 + (i << 5));	// !!! = 0x46139008
-		DMIBAR32(0x0700 + (i << 5)) = 0x46139008;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(MAGIC_CHICKEN(bundle));	// !!! = 0x46139008
+		DMIBAR32(MAGIC_CHICKEN(bundle)) = 0x46139008;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0904 + (i << 5));	// !!! = 0x3a1842ec
-		DMIBAR32(0x0904 + (i << 5)) = 0x3a1846ec;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG1(bundle));	// !!! = 0x3a1842ec
+		DMIBAR32(AFEBNDxCFG1(bundle)) = 0x3a1846ec;
    }

-	for (i = 0; i < 4; i++) {
-		DMIBAR32(0x0a00 + (i << 4));	// !!! = 0x03042018
-		DMIBAR32(0x0a00 + (i << 4)) = 0x03042018;
+	FOR_EACH_P_LANE(lane) {
+		DMIBAR32(AFELNxCFG0(lane));	// !!! = 0x03042018
+		DMIBAR32(AFELNxCFG0(lane)) = 0x03042018;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0908 + (i << 5));	// !!! = 0x51ffffff
-		DMIBAR32(0x0908 + (i << 5)) = 0x51ffffff;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG2(bundle));	// !!! = 0x51ffffff
+		DMIBAR32(AFEBNDxCFG2(bundle)) = 0x51ffffff;
    }

DMIBAR32(0x0c00);	// !!! = 0x29700c08
@@ -119,58 +119,58 @@
    DMIBAR32(0x0c0c);	// !!! = 0x16063400
    DMIBAR32(0x0c0c) = 0x00063400;

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0700 + (i << 5));	// !!! = 0x46139008
-		DMIBAR32(0x0700 + (i << 5)) = 0x46339008;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(MAGIC_CHICKEN(bundle));	// !!! = 0x46139008
+		DMIBAR32(MAGIC_CHICKEN(bundle)) = 0x46339008;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0700 + (i << 5));	// !!! = 0x46339008
-		DMIBAR32(0x0700 + (i << 5)) = 0x45339008;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(MAGIC_CHICKEN(bundle));	// !!! = 0x46339008
+		DMIBAR32(MAGIC_CHICKEN(bundle)) = 0x45339008;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0700 + (i << 5));	// !!! = 0x45339008
-		DMIBAR32(0x0700 + (i << 5)) = 0x453b9008;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(MAGIC_CHICKEN(bundle));	// !!! = 0x45339008
+		DMIBAR32(MAGIC_CHICKEN(bundle)) = 0x453b9008;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0700 + (i << 5));	// !!! = 0x453b9008
-		DMIBAR32(0x0700 + (i << 5)) = 0x45bb9008;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(MAGIC_CHICKEN(bundle));	// !!! = 0x453b9008
+		DMIBAR32(MAGIC_CHICKEN(bundle)) = 0x45bb9008;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0700 + (i << 5));	// !!! = 0x45bb9008
-		DMIBAR32(0x0700 + (i << 5)) = 0x45fb9008;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(MAGIC_CHICKEN(bundle));	// !!! = 0x45bb9008
+		DMIBAR32(MAGIC_CHICKEN(bundle)) = 0x45fb9008;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0914 + (i << 5));	// !!! = 0x9021a080
-		DMIBAR32(0x0914 + (i << 5)) = 0x9021a280;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(LOADBUSCTL0(bundle));	// !!! = 0x9021a080
+		DMIBAR32(LOADBUSCTL0(bundle)) = 0x9021a280;
    }

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0914 + (i << 5));	// !!! = 0x9021a080
-		DMIBAR32(0x0914 + (i << 5)) = 0x9821a280;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(LOADBUSCTL0(bundle));	// !!! = 0x9021a080
+		DMIBAR32(LOADBUSCTL0(bundle)) = 0x9821a280;
    }

-	for (i = 0; i < 4; i++) {
-		DMIBAR32(0x0a00 + (i << 4));	// !!! = 0x03042018
-		DMIBAR32(0x0a00 + (i << 4)) = 0x03242018;
+	FOR_EACH_P_LANE(lane) {
+		DMIBAR32(AFELNxCFG0(lane));	// !!! = 0x03042018
+		DMIBAR32(AFELNxCFG0(lane)) = 0x03242018;
    }

-	DMIBAR32(0x0258);	// !!! = 0x40000600
-	DMIBAR32(0x0258) = 0x60000600;
+	DMIBAR32(PEG_CFG4);	// !!! = 0x40000600
+	DMIBAR32(PEG_CFG4) = 0x60000600;

-	for (i = 0; i < 2; i++) {
-		DMIBAR32(0x0904 + (i << 5));	// !!! = 0x3a1846ec
-		DMIBAR32(0x0904 + (i << 5)) = 0x2a1846ec;
-		DMIBAR32(0x0914 + (i << 5));	// !!! = 0x9821a280
-		DMIBAR32(0x0914 + (i << 5)) = 0x98200280;
+	FOR_EACH_BUNDLE(bundle) {
+		DMIBAR32(AFEBNDxCFG1(bundle));	// !!! = 0x3a1846ec
+		DMIBAR32(AFEBNDxCFG1(bundle)) = 0x2a1846ec;
+		DMIBAR32(LOADBUSCTL0(bundle));	// !!! = 0x9821a280
+		DMIBAR32(LOADBUSCTL0(bundle)) = 0x98200280;
    }

-	DMIBAR32(0x022c);	// !!! = 0x00c26460
-	DMIBAR32(0x022c) = 0x00c2403c;
+	DMIBAR32(PEG_L0SLAT);	// !!! = 0x00c26460
+	DMIBAR32(PEG_L0SLAT) = 0x00c2403c;

early_pch_init_native_dmi_pre();

diff --git a/src/northbridge/intel/sandybridge/sandybridge.h b/src/northbridge/intel/sandybridge/sandybridge.h
index 8fb72cc..e40106c 100644
--- a/src/northbridge/intel/sandybridge/sandybridge.h
+++ b/src/northbridge/intel/sandybridge/sandybridge.h
@@ -115,7 +115,33 @@

/* Devices 0:1.0, 0:1.1, 0:1.2, 0:6.0 PCI configuration space (PCI Express Graphics) */

-#define AFE_PWRON	0xc24	/* PEG Analog Front-End Power-On */
+/* Per-bundle and per-lane register group helpers */
+#define BUNDLE(reg, bundle)	((reg) + ((bundle) << 5))
+#define P_LANE(reg, p_lane)	((reg) + ((p_lane) << 4))
+
+/* Per-bundle and per-lane register group iterators */
+#define FOR_EACH_BUNDLE(bundle)	for (bundle = 0; bundle < 2; bundle++)
+#define FOR_EACH_P_LANE(p_lane)	for (p_lane = 0; p_lane < 4; p_lane++)
+
+/* Register definitions */
+#define PEG_CFG4		0x258			/* PEG Config 4 */
+#define PEG_L0SLAT		0x22c			/* PEG L0s Control */
+
+#define MAGIC_CHICKEN(bundle)	BUNDLE(0x700, bundle)	/* Undocumented chicken bits */
+
+#define AFEBNDxCFG0(bundle)	BUNDLE(0x900, bundle)	/* PEG AFE Bundle Config 0 */
+#define AFEBNDxCFG1(bundle)	BUNDLE(0x904, bundle)	/* PEG AFE Bundle Config 1 */
+#define AFEBNDxCFG2(bundle)	BUNDLE(0x908, bundle)	/* PEG AFE Bundle Config 2 */
+#define AFEBNDxCFG3(bundle)	BUNDLE(0x90c, bundle)	/* PEG AFE Bundle Config 3 */
+#define AFEBNDxCFG4(bundle)	BUNDLE(0x910, bundle)	/* PEG AFE Bundle Config 4 */
+#define LOADBUSCTL0(bundle)	BUNDLE(0x914, bundle)	/* PEG Load Bus Control */
+
+#define AFELNxCFG0(lane)	P_LANE(0xa00, lane)	/* PEG AFE Lane Config 0 */
+#define AFELNxCFG1(lane)	P_LANE(0xa04, lane)	/* PEG AFE Lane Config 1 */
+
+#define AFE_PWRON		0xc24			/* PEG Analog Front-End Power-On */
+
+#define AFECMNCFG7		0xc30			/* DMI AFE Common Config 7 */

/* Device 0:2.0 PCI configuration space (Graphics Device) */

Change in coreboot[master]: nb/intel/sandybridge: Use macros for DMI init

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39634

to look at the new patch set (#2).

Change subject: nb/intel/sandybridge: Use macros for DMI init
......................................................................

nb/intel/sandybridge: Use macros for DMI init

Put names to DMI registers, and use for-each style macros.

With BUILD_TIMELESS=1, the binary of ASUS P8Z77-V LX2 remains identical.

Change-Id: I3c155f30b5642828c958cd2650aad5547b58d00b
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/sandybridge/early_dmi.c
M src/northbridge/intel/sandybridge/northbridge.c
M src/northbridge/intel/sandybridge/sandybridge.h
3 files changed, 131 insertions(+), 100 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/39634/2

Change in coreboot[master]: nb/intel/sandybridge: Use macros for DMI init

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39634

to look at the new patch set (#3).

Change subject: nb/intel/sandybridge: Use macros for DMI init
......................................................................

nb/intel/sandybridge: Use macros for DMI init

Put names to DMI registers, and use for-each style macros.

With BUILD_TIMELESS=1, the binary of ASUS P8Z77-V LX2 remains identical.

Change-Id: I3c155f30b5642828c958cd2650aad5547b58d00b
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/sandybridge/early_dmi.c
M src/northbridge/intel/sandybridge/northbridge.c
M src/northbridge/intel/sandybridge/sandybridge.h
3 files changed, 131 insertions(+), 100 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/39634/3

Change in coreboot[master]: nb/intel/sandybridge: Use macros for DMI init

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39634 )

Change subject: nb/intel/sandybridge: Use macros for DMI init
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: nb/intel/sandybridge: Use macros for DMI init

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39634 )

Change subject: nb/intel/sandybridge: Use macros for DMI init
......................................................................

Patch Set 3: Code-Review-1

(3 comments)

Sorry for this, but um, I fear this doesn't make the code more
readable. I see that it makes things look prettier, though,
but that's not the final goal, is it?

When I look at

DMIBAR32(0x900 + x)...
   DMIBAR32(0x900 + x)...

DMIBAR32(0x904 + x)...
   DMIBAR32(0x904 + x)...

I see different numbers. But when we use an unspeakable, all-caps
macro name, where the difference is only a small portion of the name

DMIBAR32(AFEBNDxCFG0(x))...
   DMIBAR32(AFEBNDxCFG0(x))...

DMIBAR32(AFEBNDxCFG1(x))...
   DMIBAR32(AFEBNDxCFG1(x))...

I just see a search-image and have to concentrate to read it.

One way to make this more readable again, would be to make the
number an argument, e.g.

DMIBAR32(AFEBNDxCFG(x, 0))...
   DMIBAR32(AFEBNDxCFG(x, 0))...

DMIBAR32(AFEBNDxCFG(x, 1))...
   DMIBAR32(AFEBNDxCFG(x, 1))...

Maybe some underscores would help too, `AFE_BNDx_CFG()`?

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/sandybridge.h:

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
PS3, Line 118: /* Per-bundle and per-lane register group helpers */
             : #define BUNDLE(reg, bundle)	((reg) + ((bundle) << 5))
             : #define P_LANE(reg, p_lane)	((reg) + ((p_lane) << 4))
These add another layer of indirection when reading the code, and
I don't see what we gain from them. Isn't

(0x900 + (bundle) * 32)

much easier to read than

BUNDLE(0x900, bundle)

?

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
PS3, Line 122: /* Per-bundle and per-lane register group iterators */
             : #define FOR_EACH_BUNDLE(bundle)	for (bundle = 0; bundle < 2; bundle++)
             : #define FOR_EACH_P_LANE(p_lane)	for (p_lane = 0; p_lane < 4; p_lane++)
Will these be used for non-DMI things? If you define them at this scope, they should
have `DMI` in the name, it's too easy to confuse with other lanes. Alternatively, if
that works, move them to `early_dmi.c`?

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
PS3, Line 131: MAGIC_CHICKEN
Is this name used anywhere else? If not, how is it better than
using the number in the code?

Change in coreboot[master]: nb/intel/sandybridge: Use macros for DMI init

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39634 )

Change subject: nb/intel/sandybridge: Use macros for DMI init
......................................................................

Patch Set 3:

(3 comments)
> Patch Set 3: Code-Review-1
>
> (3 comments)
>
> Sorry for this, but um, I fear this doesn't make the code more
> readable. I see that it makes things look prettier, though,
> but that's not the final goal, is it?
>
> When I look at
>
> DMIBAR32(0x900 + x)...
>    DMIBAR32(0x900 + x)...
>
> DMIBAR32(0x904 + x)...
>    DMIBAR32(0x904 + x)...
>
> I see different numbers. But when we use an unspeakable, all-caps
> macro name, where the difference is only a small portion of the name
>
> DMIBAR32(AFEBNDxCFG0(x))...
>    DMIBAR32(AFEBNDxCFG0(x))...
>
> DMIBAR32(AFEBNDxCFG1(x))...
>    DMIBAR32(AFEBNDxCFG1(x))...
>
> I just see a search-image and have to concentrate to read it.
>
> One way to make this more readable again, would be to make the
> number an argument, e.g.
>
> DMIBAR32(AFEBNDxCFG(x, 0))...
>    DMIBAR32(AFEBNDxCFG(x, 0))...
>
> DMIBAR32(AFEBNDxCFG(x, 1))...
>    DMIBAR32(AFEBNDxCFG(x, 1))...
>
> Maybe some underscores would help too, `AFE_BNDx_CFG()`?

Right, I can use underscores if you want. For example, `AFE_BNDx_CFGy(x, y)`

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/sandybridge.h:

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
PS3, Line 118: /* Per-bundle and per-lane register group helpers */
             : #define BUNDLE(reg, bundle)	((reg) + ((bundle) << 5))
             : #define P_LANE(reg, p_lane)	((reg) + ((p_lane) << 4))
> These add another layer of indirection when reading the code, and […]

This was done in the same way as the per-channel MCHBAR register helpers.

In any case, I would use `0x20` over `32` as the registers are in hex

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
PS3, Line 122: /* Per-bundle and per-lane register group iterators */
             : #define FOR_EACH_BUNDLE(bundle)	for (bundle = 0; bundle < 2; bundle++)
             : #define FOR_EACH_P_LANE(p_lane)	for (p_lane = 0; p_lane < 4; p_lane++)
> Will these be used for non-DMI things? If you define them at this scope, they should […]

They can be used for PCIe, but I think the number of bundles/lanes can change. I can move them to early_dmi.c if desired.

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
PS3, Line 131: MAGIC_CHICKEN
> Is this name used anywhere else? If not, how is it better than […]

I don't think so, but not giving it a name results in inconsistencies

Change in coreboot[master]: nb/intel/sandybridge: Use macros for DMI init

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39634 )

Change subject: nb/intel/sandybridge: Use macros for DMI init
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/sandybridge.h:

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
PS3, Line 118: /* Per-bundle and per-lane register group helpers */
             : #define BUNDLE(reg, bundle)	((reg) + ((bundle) << 5))
             : #define P_LANE(reg, p_lane)	((reg) + ((p_lane) << 4))
> This was done in the same way as the per-channel MCHBAR register helpers.

Hmmm, that I didn't review it doesn't mean that I like it ;) I can guess
what G, L and C refer too, but probably only because I wrote similar code
for GM45 (I did follow some common pratices back then, too, which I might
avoid today). I think the names should be more descriptive, and if we
want to avoid confusion between DRAM byte lanes and DMI lanes, why not
just put the definitions where they are used? or, if they are used in
multiple compilation units, add a DMI header?
> In any case, I would use `0x20` over `32` as the registers are in hex

sounds good, actually I prefer it :) I was just translating the << 5 while
typing, I guess.

https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... 
PS3, Line 131: MAGIC_CHICKEN
> I don't think so, but not giving it a name results in inconsistencies

I would choose readability over cosmetic consistency. However, I can't
say what would be more readable in this case. If one wants to compare
the code with some dull dump or trace, a number would make things easier.
OTOH, if one wanted to compare it to reference code, it probably wouldn't
matter.

Change in coreboot[master]: nb/intel/sandybridge: Use macros for DMI init

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Arthur Heymans, Patrick Rudolph, Felix Held,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39634

to look at the new patch set (#4).

Change subject: nb/intel/sandybridge: Use macros for DMI init
......................................................................

nb/intel/sandybridge: Use macros for DMI init

Put names to DMI registers, and use for-each style macros.

With BUILD_TIMELESS=1, the binary of ASUS P8Z77-V LX2 remains identical.

Change-Id: I3c155f30b5642828c958cd2650aad5547b58d00b
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/sandybridge/early_dmi.c
M src/northbridge/intel/sandybridge/northbridge.c
M src/northbridge/intel/sandybridge/sandybridge.h
3 files changed, 131 insertions(+), 100 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/39634/4

Change in coreboot[master]: nb/intel/sandybridge: Use macros for DMI init

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39634 )

Change subject: nb/intel/sandybridge: Use macros for DMI init
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39634/4/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/sandybridge.h:

https://review.coreboot.org/c/coreboot/+/39634/4/src/northbridge/intel/sandy... 
PS4, Line 125: #define PEG_L0SLAT		0x22c			/* PEG L0s Control */
hell pls, this one is not in order

Change in coreboot[master]: nb/intel/sandybridge: Use macros for DMI init

[Angel Pons (Code Review)]

Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39634 )

Change subject: nb/intel/sandybridge: Use macros for DMI init
......................................................................

Abandoned

Nobody likes it