Hello Felix Singer, Nico Huber, Arthur Heymans, Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48550
to review the following change.
Change subject: nb/intel/sandybridge: Fix blunder in MR2 shadow code ......................................................................
nb/intel/sandybridge: Fix blunder in MR2 shadow code
Commit 7f1363d9b4 (nb/intel/sandybridge: Program MR2 shadow register) has a bug where the system locks up and power cycles when booting Linux, but is still able to pass memtest86+ with flying colors. The issue will occur when the following conditions are true:
- CPU is Ivy Bridge - Memory speed is not greater than 1066 MHz (DDR3-2133 or slower) - System contains dual-rank DIMMs - The second rank of the dual-rank DIMMs is mirrored - All DIMMs support Extended Temperature Range - At least one of the DIMMs does not support Auto Self-Refresh
If all of these conditions are met, the final value of the MR2 Shadow registers configures the memory controller to issue a MRS command to update MR2 before entering self-refresh mode, but indicates that rank mirroring is not required (the first rank on a DIMM is never mirrored). Before the memory controller enters self-refresh, it sends MRS commands to all ranks to update MR2, but the missing address and bank mirroring means DRAM chips on mirrored ranks instead clobber MR1 with junk data. With garbage in MR1, the mirrored ranks no longer function properly, which ultimately leads to all hell breaking loose (undefined behavior).
The condition is backwards, since only odd ranks can be mirrored. To avoid this problem completely, simply remove the condition. The final register value will still be correct, since the bits are always ORed.
Tested on Asus P8Z77-V LX2, fixes booting Linux with dual-rank DIMMs.
Change-Id: Iceff741eb85fab0ae846e50af0080e5ff405404c Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 6 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/48550/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 399ba5a..80c5186 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -815,13 +815,12 @@
reg32 |= mr2reg & ~(3 << 6);
- if (rank & 1) { - if (srt) - reg32 |= 1 << (rank / 2 + 6); - } else { - if (ctrl->rank_mirror[channel][rank]) - reg32 |= 1 << (rank / 2 + 14); - } + if (srt) + reg32 |= 1 << (rank / 2 + 6); + + if (ctrl->rank_mirror[channel][rank]) + reg32 |= 1 << (rank / 2 + 14); + MCHBAR32(TC_MR2_SHADOW_ch(channel)) = reg32; }
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48550 )
Change subject: nb/intel/sandybridge: Fix blunder in MR2 shadow code ......................................................................
Patch Set 1: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48550 )
Change subject: nb/intel/sandybridge: Fix blunder in MR2 shadow code ......................................................................
Patch Set 1: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48550 )
Change subject: nb/intel/sandybridge: Fix blunder in MR2 shadow code ......................................................................
Patch Set 1: Code-Review+2
Good commit message!
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48550 )
Change subject: nb/intel/sandybridge: Fix blunder in MR2 shadow code ......................................................................
nb/intel/sandybridge: Fix blunder in MR2 shadow code
Commit 7f1363d9b4 (nb/intel/sandybridge: Program MR2 shadow register) has a bug where the system locks up and power cycles when booting Linux, but is still able to pass memtest86+ with flying colors. The issue will occur when the following conditions are true:
- CPU is Ivy Bridge - Memory speed is not greater than 1066 MHz (DDR3-2133 or slower) - System contains dual-rank DIMMs - The second rank of the dual-rank DIMMs is mirrored - All DIMMs support Extended Temperature Range - At least one of the DIMMs does not support Auto Self-Refresh
If all of these conditions are met, the final value of the MR2 Shadow registers configures the memory controller to issue a MRS command to update MR2 before entering self-refresh mode, but indicates that rank mirroring is not required (the first rank on a DIMM is never mirrored). Before the memory controller enters self-refresh, it sends MRS commands to all ranks to update MR2, but the missing address and bank mirroring means DRAM chips on mirrored ranks instead clobber MR1 with junk data. With garbage in MR1, the mirrored ranks no longer function properly, which ultimately leads to all hell breaking loose (undefined behavior).
The condition is backwards, since only odd ranks can be mirrored. To avoid this problem completely, simply remove the condition. The final register value will still be correct, since the bits are always ORed.
Tested on Asus P8Z77-V LX2, fixes booting Linux with dual-rank DIMMs.
Change-Id: Iceff741eb85fab0ae846e50af0080e5ff405404c Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48550 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 6 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Patrick Rudolph: Looks good to me, approved Arthur Heymans: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 399ba5a..80c5186 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -815,13 +815,12 @@
reg32 |= mr2reg & ~(3 << 6);
- if (rank & 1) { - if (srt) - reg32 |= 1 << (rank / 2 + 6); - } else { - if (ctrl->rank_mirror[channel][rank]) - reg32 |= 1 << (rank / 2 + 14); - } + if (srt) + reg32 |= 1 << (rank / 2 + 6); + + if (ctrl->rank_mirror[channel][rank]) + reg32 |= 1 << (rank / 2 + 14); + MCHBAR32(TC_MR2_SHADOW_ch(channel)) = reg32; }
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)