Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38405 )
Change subject: nb/intel/sandybridge: refactor lane_registers[] ......................................................................
nb/intel/sandybridge: refactor lane_registers[]
Rename array and use defines for the values.
The patch doesn't change the resulting binary when using BUILD_TIMELESS=1
Change-Id: I774373d231a0f4a2fe82ab7c6f1318fc56bcc678 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/sandybridge.h 2 files changed, 23 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/38405/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 4c36600..e7d5ac5 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -1040,10 +1040,10 @@ } }
-static const u32 lane_registers[] = { - 0x0000, 0x0200, 0x0400, 0x0600, - 0x1000, 0x1200, 0x1400, 0x1600, - 0x0800 +static const u32 lane_base[] = { + LANEBASE_B0, LANEBASE_B1, LANEBASE_B2, LANEBASE_B3, + LANEBASE_B4, LANEBASE_B5, LANEBASE_B6, LANEBASE_B7, + LANEBASE_ECC };
void program_timings(ramctr_timing *ctrl, int channel) @@ -1155,9 +1155,7 @@ shift_402x) << (8 * slotrank);
FOR_ALL_LANES { - MCHBAR32(lane_registers[lane] + 0x10 + channel * 0x100 + - 4 * slotrank) - = + MCHBAR32(lane_base[lane] + 0x10 + channel * 0x100 + 4 * slotrank) = (((ctrl->timings[channel][slotrank].lanes[lane]. timA + shift) & 0x3f) | @@ -1170,9 +1168,7 @@ | ((ctrl->timings[channel][slotrank].lanes[lane]. falling + shift) << 20));
- MCHBAR32(lane_registers[lane] + 0x20 + channel * 0x100 + - 4 * slotrank) - = + MCHBAR32(lane_base[lane] + 0x20 + channel * 0x100 + 4 * slotrank) = (((ctrl->timings[channel][slotrank].lanes[lane]. timC + shift) & 0x3f) | @@ -1232,7 +1228,7 @@ int lane) { u32 timA = ctrl->timings[channel][slotrank].lanes[lane].timA; - return ((MCHBAR32(lane_registers[lane] + channel * 0x100 + 4 + + return ((MCHBAR32(lane_base[lane] + channel * 0x100 + 4 + ((timA / 32) & 1) * 4) >> (timA % 32)) & 1); }
@@ -1894,7 +1890,7 @@
FOR_ALL_LANES { statistics[lane][timB] = - !((MCHBAR32(lane_registers[lane] + + !((MCHBAR32(lane_base[lane] + channel * 0x100 + 4 + ((timB / 32) & 1) * 4) >> (timB % 32)) & 1); } @@ -2017,9 +2013,8 @@
wait_for_iosav(channel); FOR_ALL_LANES { - u64 res = MCHBAR32(lane_registers[lane] + channel * 0x100 + 4); - res |= ((u64) MCHBAR32(lane_registers[lane] + - channel * 0x100 + 8)) << 32; + u64 res = MCHBAR32(lane_base[lane] + channel * 0x100 + 4); + res |= ((u64) MCHBAR32(lane_base[lane] + channel * 0x100 + 8)) << 32; old = ctrl->timings[channel][slotrank].lanes[lane].timB; ctrl->timings[channel][slotrank].lanes[lane].timB += get_timB_high_adjust(res) * 64; @@ -3025,9 +3020,9 @@ int channel, slotrank;
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { - MCHBAR32(0x0004 + channel * 0x100 + lane_registers[slotrank]) = + MCHBAR32(0x0004 + channel * 0x100 + lane_base[slotrank]) = make_mr0(ctrl, slotrank); - MCHBAR32(0x0008 + channel * 0x100 + lane_registers[slotrank]) = + MCHBAR32(0x0008 + channel * 0x100 + lane_base[slotrank]) = make_mr1(ctrl, slotrank, channel); } } diff --git a/src/northbridge/intel/sandybridge/sandybridge.h b/src/northbridge/intel/sandybridge/sandybridge.h index f12feaa..53fb3eb 100644 --- a/src/northbridge/intel/sandybridge/sandybridge.h +++ b/src/northbridge/intel/sandybridge/sandybridge.h @@ -133,6 +133,17 @@ #define Cx(r, x) ((r) + ((x) * 0x400)) #define CxLy(r, x, y) ((r) + ((x) * 0x400) + ((y) * 4))
+/* byte lane training register base addresses */ +#define LANEBASE_B0 0x0000 +#define LANEBASE_B1 0x0200 +#define LANEBASE_B2 0x0400 +#define LANEBASE_B3 0x0600 +#define LANEBASE_B4 0x1000 +#define LANEBASE_B5 0x1200 +#define LANEBASE_B6 0x1400 +#define LANEBASE_B7 0x1600 +#define LANEBASE_ECC 0x0800 /* ECC lane is in the middle of the data lanes */ + /* Register definitions */ #define GDCRCLKRANKSUSED_ch(ch) Gz(0x0c00, ch) /* Indicates which rank is populated */ #define GDCRCLKCOMP_ch(ch) Gz(0x0c04, ch) /* RCOMP result register */
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38405 )
Change subject: nb/intel/sandybridge: refactor lane_registers[] ......................................................................
Patch Set 1: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38405 )
Change subject: nb/intel/sandybridge: refactor lane_registers[] ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38405 )
Change subject: nb/intel/sandybridge: refactor lane_registers[] ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38405/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/sandybridge.h:
https://review.coreboot.org/c/coreboot/+/38405/2/src/northbridge/intel/sandy... PS2, Line 145: #define LANEBASE_ECC 0x0800 /* ECC lane is in the middle of the data lanes */ Maybe put it in the middle of the data lanes? The rest of the MCHBAR registers are numerically sorted
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38405 )
Change subject: nb/intel/sandybridge: refactor lane_registers[] ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38405/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/sandybridge.h:
https://review.coreboot.org/c/coreboot/+/38405/2/src/northbridge/intel/sandy... PS2, Line 145: #define LANEBASE_ECC 0x0800 /* ECC lane is in the middle of the data lanes */
Maybe put it in the middle of the data lanes? The rest of the MCHBAR registers are numerically sorte […]
i wanted to match the order of the elements in the array in the c file, but you got a point there. will probably do that as follow-up, since editing a patch in a patch train is a bit of a pain
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38405 )
Change subject: nb/intel/sandybridge: refactor lane_registers[] ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38405/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/sandybridge.h:
https://review.coreboot.org/c/coreboot/+/38405/2/src/northbridge/intel/sandy... PS2, Line 145: #define LANEBASE_ECC 0x0800 /* ECC lane is in the middle of the data lanes */
i wanted to match the order of the elements in the array in the c file, but you got a point there. […]
Ack
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38405 )
Change subject: nb/intel/sandybridge: refactor lane_registers[] ......................................................................
nb/intel/sandybridge: refactor lane_registers[]
Rename array and use defines for the values.
The patch doesn't change the resulting binary when using BUILD_TIMELESS=1
Change-Id: I774373d231a0f4a2fe82ab7c6f1318fc56bcc678 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/38405 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/sandybridge.h 2 files changed, 23 insertions(+), 17 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: 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/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 795775c..4851542 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -1040,10 +1040,10 @@ } }
-static const u32 lane_registers[] = { - 0x0000, 0x0200, 0x0400, 0x0600, - 0x1000, 0x1200, 0x1400, 0x1600, - 0x0800 +static const u32 lane_base[] = { + LANEBASE_B0, LANEBASE_B1, LANEBASE_B2, LANEBASE_B3, + LANEBASE_B4, LANEBASE_B5, LANEBASE_B6, LANEBASE_B7, + LANEBASE_ECC };
void program_timings(ramctr_timing *ctrl, int channel) @@ -1155,9 +1155,7 @@ shift_402x) << (8 * slotrank);
FOR_ALL_LANES { - MCHBAR32(lane_registers[lane] + 0x10 + channel * 0x100 + - 4 * slotrank) - = + MCHBAR32(lane_base[lane] + 0x10 + channel * 0x100 + 4 * slotrank) = (((ctrl->timings[channel][slotrank].lanes[lane]. timA + shift) & 0x3f) | @@ -1170,9 +1168,7 @@ | ((ctrl->timings[channel][slotrank].lanes[lane]. falling + shift) << 20));
- MCHBAR32(lane_registers[lane] + 0x20 + channel * 0x100 + - 4 * slotrank) - = + MCHBAR32(lane_base[lane] + 0x20 + channel * 0x100 + 4 * slotrank) = (((ctrl->timings[channel][slotrank].lanes[lane]. timC + shift) & 0x3f) | @@ -1232,7 +1228,7 @@ int lane) { u32 timA = ctrl->timings[channel][slotrank].lanes[lane].timA; - return ((MCHBAR32(lane_registers[lane] + channel * 0x100 + 4 + + return ((MCHBAR32(lane_base[lane] + channel * 0x100 + 4 + ((timA / 32) & 1) * 4) >> (timA % 32)) & 1); }
@@ -1894,7 +1890,7 @@
FOR_ALL_LANES { statistics[lane][timB] = - !((MCHBAR32(lane_registers[lane] + + !((MCHBAR32(lane_base[lane] + channel * 0x100 + 4 + ((timB / 32) & 1) * 4) >> (timB % 32)) & 1); } @@ -2017,9 +2013,8 @@
wait_for_iosav(channel); FOR_ALL_LANES { - u64 res = MCHBAR32(lane_registers[lane] + channel * 0x100 + 4); - res |= ((u64) MCHBAR32(lane_registers[lane] + - channel * 0x100 + 8)) << 32; + u64 res = MCHBAR32(lane_base[lane] + channel * 0x100 + 4); + res |= ((u64) MCHBAR32(lane_base[lane] + channel * 0x100 + 8)) << 32; old = ctrl->timings[channel][slotrank].lanes[lane].timB; ctrl->timings[channel][slotrank].lanes[lane].timB += get_timB_high_adjust(res) * 64; @@ -3025,9 +3020,9 @@ int channel, slotrank;
FOR_ALL_CHANNELS FOR_ALL_POPULATED_RANKS { - MCHBAR32(0x0004 + channel * 0x100 + lane_registers[slotrank]) = + MCHBAR32(0x0004 + channel * 0x100 + lane_base[slotrank]) = make_mr0(ctrl, slotrank); - MCHBAR32(0x0008 + channel * 0x100 + lane_registers[slotrank]) = + MCHBAR32(0x0008 + channel * 0x100 + lane_base[slotrank]) = make_mr1(ctrl, slotrank, channel); } } diff --git a/src/northbridge/intel/sandybridge/sandybridge.h b/src/northbridge/intel/sandybridge/sandybridge.h index f12feaa..53fb3eb 100644 --- a/src/northbridge/intel/sandybridge/sandybridge.h +++ b/src/northbridge/intel/sandybridge/sandybridge.h @@ -133,6 +133,17 @@ #define Cx(r, x) ((r) + ((x) * 0x400)) #define CxLy(r, x, y) ((r) + ((x) * 0x400) + ((y) * 4))
+/* byte lane training register base addresses */ +#define LANEBASE_B0 0x0000 +#define LANEBASE_B1 0x0200 +#define LANEBASE_B2 0x0400 +#define LANEBASE_B3 0x0600 +#define LANEBASE_B4 0x1000 +#define LANEBASE_B5 0x1200 +#define LANEBASE_B6 0x1400 +#define LANEBASE_B7 0x1600 +#define LANEBASE_ECC 0x0800 /* ECC lane is in the middle of the data lanes */ + /* Register definitions */ #define GDCRCLKRANKSUSED_ch(ch) Gz(0x0c00, ch) /* Indicates which rank is populated */ #define GDCRCLKCOMP_ch(ch) Gz(0x0c04, ch) /* RCOMP result register */
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38405 )
Change subject: nb/intel/sandybridge: refactor lane_registers[] ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38405/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/sandybridge.h:
https://review.coreboot.org/c/coreboot/+/38405/2/src/northbridge/intel/sandy... PS2, Line 145: #define LANEBASE_ECC 0x0800 /* ECC lane is in the middle of the data lanes */
Ack
done in 38437
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38405 )
Change subject: nb/intel/sandybridge: refactor lane_registers[] ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : No test failed. EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : No test failed. EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : No test failed.
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)