Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
* Add ECC test code when DEBUG_RAM_SETUP * Move ECC srubbing after set_scrambling_seed() to be able to observe what has been cleared. * ECC scrubbing must happen after dram_dimm_set_mapping() * Move method out of try_init_dram_ddr3() * Add comments with observations made while fixing the code
Tested on HPZ220 with ECC memory and XeonE3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_native.c 3 files changed, 82 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40721/1
diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index 6c8145d..2b0cb5e 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -366,10 +366,37 @@
set_scrambling_seed(&ctrl);
+ if (!s3resume && ctrl.ecc_enabled) + channel_scrub(&ctrl); + set_normal_operation(&ctrl);
final_registers(&ctrl);
+ if (CONFIG(DEBUG_RAM_SETUP) && !s3resume && ctrl.ecc_enabled) { + /* Test ECC in normal mode! */ + + uint32_t tseg = pci_read_config32(HOST_BRIDGE, TSEGMB); + int i; + + printk(BIOS_INFO, "RAMINIT: ECC scrub test on first channel up to 0x%x\n", + tseg); + + /* Skip 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: DRAM not clear 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_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 087ba2b..9cbc769 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -18,8 +18,9 @@
/* FIXME: no support for 3-channel chipsets */
-/* length: [1..4] */ -#define IOSAV_RUN_ONCE(length) ((((length) - 1) << 18) | 1) +/* length: [1..4], repeat: [1..255] */ +#define IOSAV_RUN_MULTIPLE(length, repeat) ((((length) - 1) << 18) | (repeat)) +#define IOSAV_RUN_ONCE(length) IOSAV_RUN_MULTIPLE(length, 1)
static void sfence(void) { @@ -297,8 +298,14 @@ reg |= (dimmB->width / 8 - 1) << 20; }
- reg |= 1 << 21; /* Rank interleave */ - reg |= 1 << 22; /* Enhanced interleave */ + /* + * Rank interleave: Bit 16 of the physical address space sets + * the rank to use in a dual single rank DIMM configuration. + * That results in every 64KiB being interleaved between two ranks. + */ + reg |= 1 << 21; + /* Enhanced interleave */ + reg |= 1 << 22;
if ((dimmA && (dimmA->ranks > 0)) || (dimmB && (dimmB->ranks > 0))) { ctrl->mad_dimm[channel] = reg; @@ -322,7 +329,7 @@ MCHBAR32(MAD_DIMM(channel)) = ctrl->mad_dimm[channel] | ecc; }
- //udelay(10); /* TODO: Might be needed for ECC configurations; so far works without. */ + udelay(10); }
void dram_zones(ramctr_timing *ctrl, int training) @@ -2948,39 +2955,56 @@ void channel_scrub(ramctr_timing *ctrl) { int channel, slotrank, row, rowsize; + u8 bank; + + + FOR_ALL_POPULATED_CHANNELS { + wait_for_iosav(channel); + fill_pattern0(ctrl, channel, 0, 0); + MCHBAR32(IOSAV_DATA_CTL_ch(channel)) = 0; + }
FOR_ALL_POPULATED_CHANNELS FOR_ALL_POPULATED_RANKS { - rowsize = 1 << ctrl->info.dimm[channel][slotrank >> 1].row_bits; - for (row = 0; row < rowsize; row += 16) { + rowsize = 1 << ctrl->info.dimm[channel][!!(slotrank & 0xC)].row_bits; + /* Guessed based on oberservation: + * Writes 128 * 8 bytes + * Bank bits are located at 13:15 + * Rowbits start at bit 20 + */ + for (bank = 0; bank < 8; bank ++) { + for (row = 0; row < rowsize; row += 16) {
- wait_for_iosav(channel); + /* DRAM command ACT */ + MCHBAR32(IOSAV_n_SP_CMD_CTRL_ch(channel, 0)) = IOSAV_ACT; + MCHBAR32(IOSAV_n_SUBSEQ_CTRL_ch(channel, 0)) = + (ctrl->tRCD << 16) | + (MAX((ctrl->tFAW >> 2) + 1, ctrl->tRRD) << 10) + | 1; + MCHBAR32(IOSAV_n_SP_CMD_ADDR_ch(channel, 0)) = + row | 0x60000 | (slotrank << 24) | (bank << 20); + MCHBAR32(IOSAV_n_ADDR_UPDATE_ch(channel, 0)) = 0x00000241;
- /* DRAM command ACT */ - MCHBAR32(IOSAV_n_SP_CMD_CTRL_ch(channel, 0)) = IOSAV_ACT; - MCHBAR32(IOSAV_n_SUBSEQ_CTRL_ch(channel, 0)) = - (MAX((ctrl->tFAW >> 2) + 1, ctrl->tRRD) << 10) - | 1 | (ctrl->tRCD << 16); - MCHBAR32(IOSAV_n_SP_CMD_ADDR_ch(channel, 0)) = - row | 0x00060000 | (slotrank << 24); - MCHBAR32(IOSAV_n_ADDR_UPDATE_ch(channel, 0)) = 0x00000241; + /* DRAM command WR */ + MCHBAR32(IOSAV_n_SP_CMD_CTRL_ch(channel, 1)) = IOSAV_WR; + MCHBAR32(IOSAV_n_SUBSEQ_CTRL_ch(channel, 1)) = 0x08000000 | + ((ctrl->tWTR + ctrl->CWL + 8) << 16) | (4 << 10) | 0x81; + MCHBAR32(IOSAV_n_SP_CMD_ADDR_ch(channel, 1)) = row | + (slotrank << 24) | (bank << 20); + MCHBAR32(IOSAV_n_ADDR_UPDATE_ch(channel, 1)) = 0x00000122;
- /* DRAM command WR */ - MCHBAR32(IOSAV_n_SP_CMD_CTRL_ch(channel, 1)) = IOSAV_WR; - MCHBAR32(IOSAV_n_SUBSEQ_CTRL_ch(channel, 1)) = 0x08281081; - MCHBAR32(IOSAV_n_SP_CMD_ADDR_ch(channel, 1)) = row | (slotrank << 24); - MCHBAR32(IOSAV_n_ADDR_UPDATE_ch(channel, 1)) = 0x00000242; + /* DRAM command PRE */ + MCHBAR32(IOSAV_n_SP_CMD_CTRL_ch(channel, 2)) = IOSAV_PRE; + MCHBAR32(IOSAV_n_SUBSEQ_CTRL_ch(channel, 2)) = 0x00000000 | + (ctrl->tRP << 16) | (4 << 10) | 1; + MCHBAR32(IOSAV_n_SP_CMD_ADDR_ch(channel, 2)) = 0x60000 | + (slotrank << 24) | (bank << 20) | 0x400; + MCHBAR32(IOSAV_n_ADDR_UPDATE_ch(channel, 2)) = 0x00000240;
- /* DRAM command PRE */ - MCHBAR32(IOSAV_n_SP_CMD_CTRL_ch(channel, 2)) = IOSAV_PRE; - MCHBAR32(IOSAV_n_SUBSEQ_CTRL_ch(channel, 2)) = 0x00280c01; - MCHBAR32(IOSAV_n_SP_CMD_ADDR_ch(channel, 2)) = - 0x00060400 | (slotrank << 24); - MCHBAR32(IOSAV_n_ADDR_UPDATE_ch(channel, 2)) = 0x00000240; + /* execute command queue */ + MCHBAR32(IOSAV_SEQ_CTL_ch(channel)) = IOSAV_RUN_MULTIPLE(3, 16);
- /* execute command queue */ - MCHBAR32(IOSAV_SEQ_CTL_ch(channel)) = IOSAV_RUN_ONCE(3); - - wait_for_iosav(channel); + wait_for_iosav(channel); + } } } } 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 */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... PS1, Line 2974: for (bank = 0; bank < 8; bank ++) { space prohibited before that '++' (ctx:WxB)
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 1: Code-Review+1
(4 comments)
lots of magic, but it is what it is. Mostly some comments on the comments...
https://review.coreboot.org/c/coreboot/+/40721/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40721/1//COMMIT_MSG@13 PS1, Line 13: srubbing nit: scrubbing
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... PS1, Line 377: /* Test ECC in normal mode! */ what's that comment supposed to mean? Maybe "can't do this earlier because it needs to be done in normal operation"?
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... PS1, Line 386: i = 1; This skips the entire first megabyte, right? Maybe change the comment to "Test up to TSEG. Skip first MB to avoid special case for A-seg"?
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... PS1, Line 2969: oberservation nit: observation
Hello build bot (Jenkins), Patrick Georgi, Jonathan Kollasch, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40721
to look at the new patch set (#2).
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
* Add ECC test code when DEBUG_RAM_SETUP * Move ECC scrubbing after set_scrambling_seed() to be able to observe what has been cleared. * ECC scrubbing must happen after dram_dimm_set_mapping() * Move method out of try_init_dram_ddr3() * Add comments with observations made while fixing the code
Tested on HPZ220 with ECC memory and XeonE3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_native.c 3 files changed, 80 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40721/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40721/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40721/1//COMMIT_MSG@13 PS1, Line 13: srubbing
nit: scrubbing
Done
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... PS1, Line 377: /* Test ECC in normal mode! */
what's that comment supposed to mean? Maybe "can't do this earlier because it needs to be done in no […]
Done
https://review.coreboot.org/c/coreboot/+/40721/1/src/northbridge/intel/sandy... PS1, Line 386: i = 1;
This skips the entire first megabyte, right? Maybe change the comment to "Test up to TSEG. […]
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/2/src/northbridge/intel/sandy... PS2, Line 2974: for (bank = 0; bank < 8; bank ++) { space prohibited before that '++' (ctx:WxB)
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 2: Code-Review+2
(4 comments)
https://review.coreboot.org/c/coreboot/+/40721/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40721/2//COMMIT_MSG@19 PS2, Line 19: XeonE3 Space? Xeon E3
https://review.coreboot.org/c/coreboot/+/40721/2//COMMIT_MSG@19 PS2, Line 19: HPZ220 Add a space? HP Z220
https://review.coreboot.org/c/coreboot/+/40721/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/2/src/northbridge/intel/sandy... PS2, Line 2974: for (bank = 0; bank < 8; bank ++) {
space prohibited before that '++' (ctx:WxB)
Should be addressed
https://review.coreboot.org/c/coreboot/+/40721/2/src/northbridge/intel/sandy... PS2, Line 2976: This is getting rather squished against the right margin... I'll factor it out in a follow-up
Hello build bot (Jenkins), Patrick Georgi, Jonathan Kollasch, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40721
to look at the new patch set (#3).
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
* 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. * ECC scrubbing must happen after dram_dimm_set_mapping() * Move method out of try_init_dram_ddr3() * Add comments with observations made while fixing the code
Tested on HP Z220 with ECC memory and Xeon E3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_native.c 3 files changed, 80 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40721/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40721/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40721/2//COMMIT_MSG@19 PS2, Line 19: HPZ220
Add a space? HP Z220
Done
https://review.coreboot.org/c/coreboot/+/40721/2//COMMIT_MSG@19 PS2, Line 19: XeonE3
Space? Xeon E3
Done
https://review.coreboot.org/c/coreboot/+/40721/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/2/src/northbridge/intel/sandy... PS2, Line 2974: for (bank = 0; bank < 8; bank ++) {
Should be addressed
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 3: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40721/3//COMMIT_MSG@12 PS3, Line 12: * 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. : * ECC scrubbing must happen after dram_dimm_set_mapping() : * Move method out of try_init_dram_ddr3() : * Add comments with observations made while fixing the code Please split up according to this list. Cosmetic changes should be separate from functional ones. And functional changes should have move specific commit messages.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40721/3/src/northbridge/intel/sandy... PS3, Line 390: printk(BIOS_ERR, "RAMINIT: DRAM not clear at addr 0x%lx\n", Another nit: Just reading that messages, it’s not clear it’s about ECC scrubbing.
RAMINIT: ECC scrub failed: DRAM not clear at address 0x%lx\n
clear or cleared?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40721/3/src/northbridge/intel/sandy... PS3, Line 390: printk(BIOS_ERR, "RAMINIT: DRAM not clear at addr 0x%lx\n",
Another nit: Just reading that messages, it’s not clear it’s about ECC scrubbing. […]
Also, is there any use, in printing the read value at that address?
Hello build bot (Jenkins), Patrick Georgi, Jonathan Kollasch, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40721
to look at the new patch set (#4).
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
Tested on HP Z220 with ECC memory and Xeon E3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 47 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40721/4
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40721/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40721/3//COMMIT_MSG@12 PS3, Line 12: * 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. : * ECC scrubbing must happen after dram_dimm_set_mapping() : * Move method out of try_init_dram_ddr3() : * Add comments with observations made while fixing the code
Please split up according to this list. Cosmetic changes should be […]
Done
https://review.coreboot.org/c/coreboot/+/40721/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40721/3/src/northbridge/intel/sandy... PS3, Line 390: printk(BIOS_ERR, "RAMINIT: DRAM not clear at addr 0x%lx\n",
Also, is there any use, in printing the read value at that address?
I don't see any difference. If it's not clear it hasn't been cleared. I personally don't care for the exact value there, it could be anything as the data in DRAM is xored with the scrambler. Isn't it clear from the context that ECC scrubbing failed, as it's between "RAMINIT: ECC scrub test ..." lines?
Moved to CB:40946
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 4:
(8 comments)
Tried to mark things as resolved that aren't introduced by this change. I'll try to also have a closer look at the timings when I find the time.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2962: slotrank & 0xC AFAICS, `slotrank` is set to a number from 0 to 3 by FOR_ALL_POPULATED_RANKS. So this would be constant 0.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2964: * Writes 128 * 8 bytes in a loop So this is almost true for a single iteration of the innermost loop. The subsequence loop in the IOSAV writes 129 * 8 (burst length) * 8 (bus width) bytes. I would never have guessed that it refers to that when I read the comment. It should go to line 2981, IMO.
Maybe the comment here should start with something about the 4(!) nested loops that actually happen here (banks, rows, cmd sequence, cmd sub sequence). Even 5 if one includes the write burst ^^
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2965: * Bank bits are located at bit 13:15 in physical memory map : * Rowbits start at bit 20 in physical memory map What purpose do these comments serve? Even if that is the current memory map, we don't use it here.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2974: (ctrl->tRCD << 16) | It's a single sub-sequence command, is the delay applied at all with only one iteration?
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2975: (ctrl->tFAW >> 2) + 1 DIV_ROUND_UP() might be more accurate.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2978: 0x60000 (row_bits - 10) << 16
I guess it doesn't matter for a single iteration as long is it's high enough (6 is max.)
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2985: row | For this command, we set the column address. To clear the whole row, we just iterate over all columns. It doesn't matter where we start because the IOSAV wraps around for us, but `row` here looks too odd.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2994: (bank << 20) The 0x400 (bit 10) make it a PREA (precharge all banks), so the bank address is not needed.
I wonder, though, why we do a PREA :-/
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2962: slotrank & 0xC
AFAICS, `slotrank` is set to a number from 0 to 3 by FOR_ALL_POPULATED_RANKS. […]
It took me a while to understand "slotrank", but it's just the combination of a slot and a rank.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2974: (ctrl->tRCD << 16) |
It's a single sub-sequence command, is the delay applied at all with only one iteration?
This delay is applied after this subsequence is completed, but before the next one starts. So it makes sense here
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2962: slotrank & 0xC
It took me a while to understand "slotrank", but it's just the combination of a slot and a rank.
Oh, I mixed it with the rankmap. slotrank >> 1 was correct.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2965: * Bank bits are located at bit 13:15 in physical memory map : * Rowbits start at bit 20 in physical memory map
What purpose do these comments serve? Even if that is the […]
They help understand what's wrong by observing the patterns in the ecc scrub test. Should it be moved to the testing code?
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2994: (bank << 20)
The 0x400 (bit 10) make it a PREA (precharge all banks), so the bank address is not needed. […]
Probably copy&paste error from the other subseq commands. None of them specifies a bank. Will do test without that bit.
Hello build bot (Jenkins), Patrick Georgi, Jonathan Kollasch, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40721
to look at the new patch set (#5).
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
Tested on HP Z220 with ECC memory and Xeon E3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 61 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40721/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2962: slotrank & 0xC
Oh, I mixed it with the rankmap. slotrank >> 1 was correct.
Done
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2994: (bank << 20)
Probably copy&paste error from the other subseq commands. None of them specifies a bank. […]
Tested without 0x400, still works.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 5: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2964: * Writes 128 * 8 bytes in a loop
So this is almost true for a single iteration of the innermost loop. […]
Replaced the comment with a description why it's done like that.
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2965: * Bank bits are located at bit 13:15 in physical memory map : * Rowbits start at bit 20 in physical memory map
They help understand what's wrong by observing the patterns in the ecc scrub test. […]
Moved to CB:40946
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2964: * Writes 128 * 8 bytes in a loop
Replaced the comment with a description why it's done like that.
Ack
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2965: * Bank bits are located at bit 13:15 in physical memory map : * Rowbits start at bit 20 in physical memory map
Moved to CB:40946
Ack
https://review.coreboot.org/c/coreboot/+/40721/4/src/northbridge/intel/sandy... PS4, Line 2974: (ctrl->tRCD << 16) |
This delay is applied after this subsequence is completed, but before the next one starts. […]
Ah, then I meant the line below ;)
https://review.coreboot.org/c/coreboot/+/40721/5/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/5/src/northbridge/intel/sandy... PS5, Line 2967: accieved achieved
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/40721/3/src/northbridge/intel/sandy... PS3, Line 390: printk(BIOS_ERR, "RAMINIT: DRAM not clear at addr 0x%lx\n",
I don't see any difference. If it's not clear it hasn't been cleared. […]
Move to other change-set.
Hello build bot (Jenkins), Patrick Georgi, Jonathan Kollasch, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40721
to look at the new patch set (#6).
Change subject: [WIP]nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
[WIP]nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
Tested on HP Z220 with ECC memory and Xeon E3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 122 insertions(+), 87 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40721/6
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: [WIP]nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 6:
(1 comment)
Untested.
https://review.coreboot.org/c/coreboot/+/40721/5/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/5/src/northbridge/intel/sandy... PS5, Line 2967: accieved
achieved
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: [WIP]nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/6/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/6/src/northbridge/intel/sandy... PS6, Line 346: nit: Since it's only needed for ECC configs, add an `if (ctrl->ecc_enabled)` check?
Or leave a comment stating that this delay is needed for ECC configs
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: [WIP]nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 6: Code-Review+1
Hello build bot (Jenkins), Patrick Georgi, Jonathan Kollasch, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40721
to look at the new patch set (#7).
Change subject: [WIP]nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
[WIP]nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
Tested on HP Z220 with ECC memory and Xeon E3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 123 insertions(+), 87 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40721/7
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: [WIP]nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/6/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/6/src/northbridge/intel/sandy... PS6, Line 346:
nit: Since it's only needed for ECC configs, add an `if (ctrl->ecc_enabled)` check? […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: [WIP]nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 7: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: [WIP]nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40721/7//COMMIT_MSG@7 PS7, Line 7: [WIP] nit: drop?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: [WIP]nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40721/7//COMMIT_MSG@7 PS7, Line 7: [WIP]
nit: drop?
I've been told PS5 worked, but current one does not.
Hello build bot (Jenkins), Patrick Georgi, Jonathan Kollasch, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40721
to look at the new patch set (#8).
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
Tested on HP Z220 with ECC memory and Xeon E3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 126 insertions(+), 87 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40721/8
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40721/7//COMMIT_MSG@7 PS7, Line 7: [WIP]
I've been told PS5 worked, but current one does not.
Latested version was tested and working. Removed WIP.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 8: Code-Review+1
(5 comments)
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4311: .inc_addr_1 = 1, Do we need any increments with a single iteration?
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4312: .inc_addr_8 = 0, Fields that are zero on this register can be dropped. This also fixes the misalignment
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4335: ctrl->tWTR + : ctrl->CWL + 8, nit: add two spaces to align `ctrl->` on both lines
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4348: .inc_bank = 0, Fields that are zero on this register can be dropped. This also fixes the misalignment
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4381: .inc_bank = 0, Fields that are zero on this register can be dropped. This also fixes the misalignment
Hello build bot (Jenkins), Patrick Georgi, Jonathan Kollasch, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40721
to look at the new patch set (#9).
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
Tested on HP Z220 with ECC memory and Xeon E3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 116 insertions(+), 87 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40721/9
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4311: .inc_addr_1 = 1,
Do we need any increments with a single iteration?
without .inc_addr_1 = 1 it doesn't work.
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4312: .inc_addr_8 = 0,
Fields that are zero on this register can be dropped. […]
Done
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4335: ctrl->tWTR + : ctrl->CWL + 8,
nit: add two spaces to align `ctrl->` on both lines
Done
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4348: .inc_bank = 0,
Fields that are zero on this register can be dropped. […]
Done
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4381: .inc_bank = 0,
Fields that are zero on this register can be dropped. […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
Patch Set 9: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/40721/8/src/northbridge/intel/sandy... PS8, Line 4311: .inc_addr_1 = 1,
without .inc_addr_1 = 1 it doesn't work.
Ack
Patrick Rudolph has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40721 )
Change subject: nb/intel/sandybridge/raminit: Fix ECC scrub ......................................................................
nb/intel/sandybridge/raminit: Fix ECC scrub
The scrubbing method was never correct nor tested. Fix that by observations made on mrc.bin.
Tested on HP Z220 with ECC memory and Xeon E3 CPU: The whole memory is now scrubbed.
Change-Id: Ia9fcc236fbf73f51fe944c6dda5d22ba9d334ec7 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40721 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 116 insertions(+), 87 deletions(-)
Approvals: build bot (Jenkins): Verified 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 6588db5..126acbe 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -344,7 +344,8 @@ MCHBAR32(MAD_DIMM(channel)) = ctrl->mad_dimm[channel] | ecc; }
- //udelay(10); /* TODO: Might be needed for ECC configurations; so far works without. */ + if (ctrl->ecc_enabled) + udelay(10); }
void dram_zones(ramctr_timing *ctrl, int training) @@ -4260,98 +4261,126 @@ void channel_scrub(ramctr_timing *ctrl) { int channel, slotrank, row, rowsize; + u8 bank;
+ FOR_ALL_POPULATED_CHANNELS { + wait_for_iosav(channel); + fill_pattern0(ctrl, channel, 0, 0); + MCHBAR32(IOSAV_DATA_CTL_ch(channel)) = 0; + } + + /* + * During runtime the "scrubber" will periodically scan through the memory in the + * physical address space, to identify and fix CRC errors. + * The following loops writes to every DRAM address, setting the ECC bits to the + * correct value. A read from this location will no longer return a CRC error, + * except when a bit has toggled due to external events. + * The same could be accieved by writing to the physical memory map, but it's + * much more difficult due to SMM remapping, ME stolen memory, GFX stolen memory, + * and firmware running in x86_32. + */ FOR_ALL_POPULATED_CHANNELS FOR_ALL_POPULATED_RANKS { rowsize = 1 << ctrl->info.dimm[channel][slotrank >> 1].row_bits; - for (row = 0; row < rowsize; row += 16) { + for (bank = 0; bank < 8; bank++) { + for (row = 0; row < rowsize; row += 16) {
- wait_for_iosav(channel); + /* + * DRAM command ACT + * Opens the row for writing. + */ + { + u8 gap = MAX((ctrl->tFAW >> 2) + 1, ctrl->tRRD); + const struct iosav_ssq ssq = { + .sp_cmd_ctrl = { + .command = IOSAV_ACT, + .ranksel_ap = 1, + }, + .subseq_ctrl = { + .cmd_executions = 1, + .cmd_delay_gap = gap, + .post_ssq_wait = ctrl->tRCD, + .data_direction = SSQ_NA, + }, + .sp_cmd_addr = { + .address = row, + .rowbits = 6, + .bank = bank, + .rank = slotrank, + }, + .addr_update = { + .inc_addr_1 = 1, + .addr_wrap = 18, + }, + }; + iosav_write_ssq(channel, &ssq); + }
- /* DRAM command ACT */ - { - const struct iosav_ssq ssq = { - .sp_cmd_ctrl = { - .command = IOSAV_ACT, - .ranksel_ap = 1, - }, - .subseq_ctrl = { - .cmd_executions = 1, - .cmd_delay_gap = MAX((ctrl->tFAW >> 2) + 1, - ctrl->tRRD), - .post_ssq_wait = ctrl->tRCD, - .data_direction = SSQ_NA, - }, - .sp_cmd_addr = { - .address = row, - .rowbits = 6, - .bank = 0, - .rank = slotrank, - }, - .addr_update = { - .inc_addr_1 = 1, - .addr_wrap = 18, - }, - }; - iosav_write_ssq(channel, &ssq); + /* + * DRAM command WR + * Writes (128 + 1) * 8 (burst length) * 8 (bus width) + * bytes. + */ + { + const struct iosav_ssq ssq = { + .sp_cmd_ctrl = { + .command = IOSAV_WR, + .ranksel_ap = 1, + }, + .subseq_ctrl = { + .cmd_executions = 129, + .cmd_delay_gap = 4, + .post_ssq_wait = ctrl->tWTR + + ctrl->CWL + 8, + .data_direction = SSQ_WR, + }, + .sp_cmd_addr = { + .address = row, + .rowbits = 0, + .bank = bank, + .rank = slotrank, + }, + .addr_update = { + .inc_addr_8 = 1, + .addr_wrap = 9, + }, + }; + iosav_write_ssq(channel, &ssq); + } + + /* + * DRAM command PRE + * Closes the row. + */ + { + const struct iosav_ssq ssq = { + .sp_cmd_ctrl = { + .command = IOSAV_PRE, + .ranksel_ap = 1, + }, + .subseq_ctrl = { + .cmd_executions = 1, + .cmd_delay_gap = 4, + .post_ssq_wait = ctrl->tRP, + .data_direction = SSQ_NA, + }, + .sp_cmd_addr = { + .address = 0, + .rowbits = 6, + .bank = bank, + .rank = slotrank, + }, + .addr_update = { + .addr_wrap = 18, + }, + }; + iosav_write_ssq(channel, &ssq); + } + + /* Execute command queue */ + iosav_run_queue(channel, 16, 0); + + wait_for_iosav(channel); } - - /* DRAM command WR */ - { - const struct iosav_ssq ssq = { - .sp_cmd_ctrl = { - .command = IOSAV_WR, - .ranksel_ap = 1, - }, - .subseq_ctrl = { - .cmd_executions = 129, - .cmd_delay_gap = 4, - .post_ssq_wait = 40, - .data_direction = SSQ_WR, - }, - .sp_cmd_addr = { - .address = row, - .rowbits = 0, - .bank = 0, - .rank = slotrank, - }, - .addr_update = { - .inc_addr_8 = 1, - .addr_wrap = 18, - }, - }; - iosav_write_ssq(channel, &ssq); - } - - /* DRAM command PRE */ - { - const struct iosav_ssq ssq = { - .sp_cmd_ctrl = { - .command = IOSAV_PRE, - .ranksel_ap = 1, - }, - .subseq_ctrl = { - .cmd_executions = 1, - .cmd_delay_gap = 3, - .post_ssq_wait = 40, - .data_direction = SSQ_NA, - }, - .sp_cmd_addr = { - .address = 1024, - .rowbits = 6, - .bank = 0, - .rank = slotrank, - }, - .addr_update = { - .addr_wrap = 18, - }, - }; - iosav_write_ssq(channel, &ssq); - } - - /* execute command queue */ - iosav_run_once(channel); - - wait_for_iosav(channel); } } }
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)