Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
nb/intel/sandybridge/raminit: Add ECC debug code
* Add ECC test code when DEBUG_RAM_SETUP is enabled * Move ECC scrubbing after set_scrambling_seed() to be able to observe what has been cleared in the test routine. * ECC scrubbing must happen after dram_dimm_set_mapping() * Move method out of try_init_dram_ddr3()
Change-Id: I76174ec962c9b0bb72852897586eb95d896d301e Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_native.c 2 files changed, 25 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/40946/1
diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index 6c8145d..ff563e7 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -366,10 +366,35 @@
set_scrambling_seed(&ctrl);
+ if (!s3resume && ctrl.ecc_enabled) + channel_scrub(&ctrl); + set_normal_operation(&ctrl);
final_registers(&ctrl);
+ /* can't do this earlier because it needs to be done in normal operation */ + if (CONFIG(DEBUG_RAM_SETUP) && !s3resume && ctrl.ecc_enabled) { + uint32_t i, tseg = pci_read_config32(HOST_BRIDGE, TSEGMB); + + printk(BIOS_INFO, "RAMINIT: ECC scrub test on first channel up to 0x%x\n", + tseg); + + /* Skip first MB to avoid special case for A-seg and test up to TSEG */ + for (i = 1; i < tseg >> 20; i++) { + for (int j = 0; j < 1 * MiB; j += 4096) { + uintptr_t addr = i * MiB + j; + if (read32((u32 *)addr) == 0) + continue; + + printk(BIOS_ERR, "RAMINIT: ECC scrub: DRAM not cleared at" + " addr 0x%lx\n", addr); + break; + } + } + printk(BIOS_INFO, "RAMINIT: ECC scrub test done.\n"); + } + /* Zone config */ dram_zones(&ctrl, 0);
diff --git a/src/northbridge/intel/sandybridge/raminit_native.c b/src/northbridge/intel/sandybridge/raminit_native.c index 832391f..34299a3 100644 --- a/src/northbridge/intel/sandybridge/raminit_native.c +++ b/src/northbridge/intel/sandybridge/raminit_native.c @@ -685,9 +685,6 @@ err = channel_test(ctrl); if (err) return err; - - if (ctrl->ecc_enabled) - channel_scrub(ctrl); }
/* Set MAD-DIMM registers */
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG@12 PS1, Line 12: * ECC scrubbing must happen after dram_dimm_set_mapping() Is this about the debug code?
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG@13 PS1, Line 13: * Move method out of try_init_dram_ddr3() This is a result of the second point, right?
https://review.coreboot.org/c/coreboot/+/40946/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40946/1/src/northbridge/intel/sandy... PS1, Line 385: 4096 Assuming channels might be interleaved at this point, this would likely only test a single channel, maybe even only a single rank. It really depends on "what could possibly go wrong?", which addresses make sense to test.
Some test ist better than no test at all, though.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG@12 PS1, Line 12: * ECC scrubbing must happen after dram_dimm_set_mapping()
Is this about the debug code?
No, the function enables the ECC logic to operate in normal mode. That is calculate the ECC bits and store them on write transaction.
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG@13 PS1, Line 13: * Move method out of try_init_dram_ddr3()
This is a result of the second point, right?
Yes. By coincidence this also satisfies the 2nd and 3rd point.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40946/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40946/1/src/northbridge/intel/sandy... PS1, Line 385: 4096
Assuming channels might be interleaved at this point, this would likely […]
I haven't verified, but the "rank interleave" and "enhanced interleaved" that are always enabled should cause this code to test all ranks and all channels.
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40946
to look at the new patch set (#2).
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
nb/intel/sandybridge/raminit: Add ECC debug code
* Add ECC test code when DEBUG_RAM_SETUP is enabled * Move ECC scrubbing after set_scrambling_seed() to be able to observe what has been cleared in the test routine. * ECC scrubbing must happen after dram_dimm_set_mapping() * Move method out of try_init_dram_ddr3()
Change-Id: I76174ec962c9b0bb72852897586eb95d896d301e Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_native.c 2 files changed, 35 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/40946/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40946/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40946/2/src/northbridge/intel/sandy... PS2, Line 386: * interleave are enabled, but there's no gurantee for it. 'gurantee' may be misspelled - perhaps 'guarantee'?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG@12 PS1, Line 12: * ECC scrubbing must happen after dram_dimm_set_mapping()
No, the function enables the ECC logic to operate in normal mode. […]
I was asking because it's not covered by the commit summary.
https://review.coreboot.org/c/coreboot/+/40946/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40946/1/src/northbridge/intel/sandy... PS1, Line 385: 4096
I haven't verified, but the "rank interleave" and "enhanced interleaved" that are always enabled sho […]
I'm not sure either. But with a 4096 increment, the 12 least-significant bits of all tested addresses are 0. You can take away 6 for the cache lines (those will most likely want to stick together). But that leaves 6 bits that potentially select the channel/rank and are constant 0.
There was a paper where the exact mapping for IVB was reversed, IIRC, can't find it right now :-/ there was some higher bit that got xor'd IIRC, but most channel/rank select bits were in the lower range. And I might mix things up with the cache mapping ;)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40946/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40946/2/src/northbridge/intel/sandy... PS2, Line 389: 16 `bit 16`
But that can only be fully true for 2-rank configurations.
https://review.coreboot.org/c/coreboot/+/40946/2/src/northbridge/intel/sandy... PS2, Line 390: * Rowbits start at bit 20 in physical memory map. Most probably depends on the configuration, 1/2/4 ranks 1/2 channels.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 2: Code-Review+1
Hello build bot (Jenkins), Paul Menzel, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40946
to look at the new patch set (#3).
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
nb/intel/sandybridge/raminit: Add ECC debug code
* Add ECC test code when DEBUG_RAM_SETUP is enabled * Move ECC scrubbing after set_scrambling_seed() to be able to observe what has been cleared in the test routine. If clearing happens before set_scrambling_seed the data is XORed with a different PRN. Data read from memory will look random instead of all zeros. * ECC scrubbing must happen after dram_dimm_set_mapping() The ECC logic is set to "normal mode" in dram_dimm_set_mapping(). In normal mode the ECC bits are calculated and stored on write transactions. * Move method out of try_init_dram_ddr3(). This satisfies point 2 and point 3 of the list above.
Change-Id: I76174ec962c9b0bb72852897586eb95d896d301e Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_native.c 2 files changed, 31 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/40946/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG@12 PS1, Line 12: * ECC scrubbing must happen after dram_dimm_set_mapping()
I was asking because it's not covered by the commit summary.
Updated commit message.
https://review.coreboot.org/c/coreboot/+/40946/1//COMMIT_MSG@13 PS1, Line 13: * Move method out of try_init_dram_ddr3()
Yes. By coincidence this also satisfies the 2nd and 3rd point.
Done
https://review.coreboot.org/c/coreboot/+/40946/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40946/2/src/northbridge/intel/sandy... PS2, Line 389: 16
`bit 16` […]
Removed
https://review.coreboot.org/c/coreboot/+/40946/2/src/northbridge/intel/sandy... PS2, Line 390: * Rowbits start at bit 20 in physical memory map.
Most probably depends on the configuration, 1/2/4 ranks 1/2 channels.
Removed
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40946/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40946/3/src/northbridge/intel/sandy... PS3, Line 436: * interleave are enabled, but there's no gurantee for it. 'gurantee' may be misspelled - perhaps 'guarantee'?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Other than that, LGTM
https://review.coreboot.org/c/coreboot/+/40946/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40946/3/src/northbridge/intel/sandy... PS3, Line 436: * interleave are enabled, but there's no gurantee for it.
'gurantee' may be misspelled - perhaps 'guarantee'?
It is
Hello build bot (Jenkins), Paul Menzel, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40946
to look at the new patch set (#4).
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
nb/intel/sandybridge/raminit: Add ECC debug code
* Add ECC test code when DEBUG_RAM_SETUP is enabled * Move ECC scrubbing after set_scrambling_seed() to be able to observe what has been cleared in the test routine. If clearing happens before set_scrambling_seed the data is XORed with a different PRN. Data read from memory will look random instead of all zeros. * ECC scrubbing must happen after dram_dimm_set_mapping() The ECC logic is set to "normal mode" in dram_dimm_set_mapping(). In normal mode the ECC bits are calculated and stored on write transactions. * Move method out of try_init_dram_ddr3(). This satisfies point 2 and point 3 of the list above.
Change-Id: I76174ec962c9b0bb72852897586eb95d896d301e Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_native.c 2 files changed, 31 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/40946/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40946/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40946/3/src/northbridge/intel/sandy... PS3, Line 436: * interleave are enabled, but there's no gurantee for it.
It is
Done
Patrick Rudolph has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40946 )
Change subject: nb/intel/sandybridge/raminit: Add ECC debug code ......................................................................
nb/intel/sandybridge/raminit: Add ECC debug code
* Add ECC test code when DEBUG_RAM_SETUP is enabled * Move ECC scrubbing after set_scrambling_seed() to be able to observe what has been cleared in the test routine. If clearing happens before set_scrambling_seed the data is XORed with a different PRN. Data read from memory will look random instead of all zeros. * ECC scrubbing must happen after dram_dimm_set_mapping() The ECC logic is set to "normal mode" in dram_dimm_set_mapping(). In normal mode the ECC bits are calculated and stored on write transactions. * Move method out of try_init_dram_ddr3(). This satisfies point 2 and point 3 of the list above.
Change-Id: I76174ec962c9b0bb72852897586eb95d896d301e Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40946 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_native.c 2 files changed, 31 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index 2728037..06b4d1e 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -416,10 +416,41 @@
set_scrambling_seed(&ctrl);
+ if (!s3resume && ctrl.ecc_enabled) + channel_scrub(&ctrl); + set_normal_operation(&ctrl);
final_registers(&ctrl);
+ /* can't do this earlier because it needs to be done in normal operation */ + if (CONFIG(DEBUG_RAM_SETUP) && !s3resume && ctrl.ecc_enabled) { + uint32_t i, tseg = pci_read_config32(HOST_BRIDGE, TSEGMB); + + printk(BIOS_INFO, "RAMINIT: ECC scrub test on first channel up to 0x%x\n", + tseg); + + /* + * This test helps to debug the ECC scrubbing. + * It likely tests every channel/rank, as rank interleave and enhanced + * interleave are enabled, but there's no guarantee for it. + */ + + /* Skip first MB to avoid special case for A-seg and test up to TSEG */ + for (i = 1; i < tseg >> 20; i++) { + for (int j = 0; j < 1 * MiB; j += 4096) { + uintptr_t addr = i * MiB + j; + if (read32((u32 *)addr) == 0) + continue; + + printk(BIOS_ERR, "RAMINIT: ECC scrub: DRAM not cleared at" + " addr 0x%lx\n", addr); + break; + } + } + printk(BIOS_INFO, "RAMINIT: ECC scrub test done.\n"); + } + /* Zone config */ dram_zones(&ctrl, 0);
diff --git a/src/northbridge/intel/sandybridge/raminit_native.c b/src/northbridge/intel/sandybridge/raminit_native.c index 62715a1..c23a5ac 100644 --- a/src/northbridge/intel/sandybridge/raminit_native.c +++ b/src/northbridge/intel/sandybridge/raminit_native.c @@ -687,9 +687,6 @@ err = channel_test(ctrl); if (err) return err; - - if (ctrl->ecc_enabled) - channel_scrub(ctrl); }
/* Set MAD-DIMM registers */
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)