Change in coreboot[master]: nb/intel/sandybridge: improve indexed register helper macros

[Felix Held (Code Review)]

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38577 )

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

nb/intel/sandybridge: improve indexed register helper macros

Replace the multiplications with corresponding shifts, so that it's
easier to see at which bit offsets the values get assigned.

Change-Id: I0b0d5172394ff65edfe57bdad474631938e58872
Signed-off-by: Felix Held 
---
M src/northbridge/intel/sandybridge/sandybridge.h
1 file changed, 5 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/38577/1

diff --git a/src/northbridge/intel/sandybridge/sandybridge.h b/src/northbridge/intel/sandybridge/sandybridge.h
index f5c1e41..ffc1d9f 100644
--- a/src/northbridge/intel/sandybridge/sandybridge.h
+++ b/src/northbridge/intel/sandybridge/sandybridge.h
@@ -128,11 +128,11 @@
 #define MCHBAR32_AND_OR(x, and, or) (MCHBAR32(x) = (MCHBAR32(x) & (and)) | (or))

/* Indexed register helper macros */
-#define Gz(r, z)	((r) + ((z) * 0x100))
-#define Ly(r, y)	((r) + ((y) * 4))
-#define Cx(r, x)	((r) + ((x) * 0x400))
-#define CxLy(r, x, y)	((r) + ((x) * 0x400) + ((y) * 4))
-#define GzLy(r, z, y)	((r) + ((z) * 0x100) + ((y) * 4))
+#define Gz(r, z)	((r) + ((z) <<  8))
+#define Ly(r, y)	((r) + ((y) <<  2))
+#define Cx(r, x)	((r) + ((x) << 10))
+#define CxLy(r, x, y)	((r) + ((x) << 10) + ((y) << 2))
+#define GzLy(r, z, y)	((r) + ((z) <<  8) + ((y) << 2))

/* byte lane training register base addresses */
 #define LANEBASE_B0	0x0000

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38577 )

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

Patch Set 1: Code-Review+1

[Angel Pons (Code Review)]

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

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG@7 
PS1, Line 7: improve
Personally, I don't see what the improvement is. These macros aren't about bit offsets. With the multiplication, one can see that per-channel registers using Cx are separated 0x400 bytes, or that per-lane registers using Ly are contiguous DWORDs.

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38577 )

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG@7 
PS1, Line 7: improve
> Personally, I don't see what the improvement is. These macros aren't about bit offsets. […]

Since the factor it gets multiplied by is a power of two and the bits where it gets shifted to are zero, this is shifting bits into their positions. The multiplication thing IMHO sort-of hides what's happening there

[Angel Pons (Code Review)]

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

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG@7 
PS1, Line 7: improve
> Since the factor it gets multiplied by is a power of two and the bits where it gets shifted to are z […]

The bits aren't necessarily zero, hence the plus sign.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38577 )

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

Patch Set 1: Code-Review+2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38577 )

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG@7 
PS1, Line 7: improve
> The bits aren't necessarily zero, hence the plus sign.

as far as i've seen apart from GDCRTRAININGRESULT which seems to be a bit of a special case here, the bits where the channel/lane/register number get shifted to are zero

[Angel Pons (Code Review)]

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

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38577/1//COMMIT_MSG@7 
PS1, Line 7: improve
> as far as i've seen apart from GDCRTRAININGRESULT which seems to be a bit of a special case here, th […]

meh, I don't think it's worth bikeshedding about this any longer.

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38577 )

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

nb/intel/sandybridge: improve indexed register helper macros

Replace the multiplications with corresponding shifts, so that it's
easier to see at which bit offsets the values get assigned.

Change-Id: I0b0d5172394ff65edfe57bdad474631938e58872
Signed-off-by: Felix Held 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38577
Tested-by: build bot (Jenkins) 
Reviewed-by: HAOUAS Elyes 
Reviewed-by: Patrick Rudolph 
Reviewed-by: Angel Pons 
---
M src/northbridge/intel/sandybridge/sandybridge.h
1 file changed, 5 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  HAOUAS Elyes: Looks good to me, but someone else must approve
  Patrick Rudolph: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/northbridge/intel/sandybridge/sandybridge.h b/src/northbridge/intel/sandybridge/sandybridge.h
index f5c1e41..ffc1d9f 100644
--- a/src/northbridge/intel/sandybridge/sandybridge.h
+++ b/src/northbridge/intel/sandybridge/sandybridge.h
@@ -128,11 +128,11 @@
 #define MCHBAR32_AND_OR(x, and, or) (MCHBAR32(x) = (MCHBAR32(x) & (and)) | (or))

/* Indexed register helper macros */
-#define Gz(r, z)	((r) + ((z) * 0x100))
-#define Ly(r, y)	((r) + ((y) * 4))
-#define Cx(r, x)	((r) + ((x) * 0x400))
-#define CxLy(r, x, y)	((r) + ((x) * 0x400) + ((y) * 4))
-#define GzLy(r, z, y)	((r) + ((z) * 0x100) + ((y) * 4))
+#define Gz(r, z)	((r) + ((z) <<  8))
+#define Ly(r, y)	((r) + ((y) <<  2))
+#define Cx(r, x)	((r) + ((x) << 10))
+#define CxLy(r, x, y)	((r) + ((x) << 10) + ((y) << 2))
+#define GzLy(r, z, y)	((r) + ((z) <<  8) + ((y) << 2))

/* byte lane training register base addresses */
 #define LANEBASE_B0	0x0000

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38577 )

Change subject: nb/intel/sandybridge: improve indexed register helper macros
......................................................................

Patch Set 2:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/380
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/379
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/378

Please note: This test is under development and might not be accurate at all!