Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47614 )
Change subject: nb/intel/sandybridge: Use one sequence for write leveling ......................................................................
nb/intel/sandybridge: Use one sequence for write leveling
In order to run a write leveling test, one needs to unset the Qoff bit in MR1, then run the test, and finally set Qoff again. The current IOSAV sequence uses two subsequences to perform the test, while the other two are unused. It is possible to perform the two necessary MR1 updates in the same sequence, which can potentially improve runtime (not measured).
Tested on Asus P8H61-M PRO, still boots.
Change-Id: I65ca1aa32cdb177d2a9e27c3b02e74ac0c882794 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 48 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/47614/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 60ce50b..2a3fb8c 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -1716,15 +1716,40 @@
static void test_timB(ramctr_timing *ctrl, int channel, int slotrank) { - /* enable DQs on this slotrank */ - write_mrreg(ctrl, channel, slotrank, 1, make_mr1(ctrl, slotrank, channel) | 1 << 7); + /* First DQS/DQS# rising edge after write leveling mode is programmed */ + const u32 tWLMRD = 40; + + u32 mr1reg = make_mr1(ctrl, slotrank, channel) | 1 << 7; + int bank = 1; + + if (ctrl->rank_mirror[channel][slotrank]) + ddr3_mirror_mrreg(&bank, &mr1reg);
wait_for_iosav(channel);
const struct iosav_ssq sequence[] = { - /* DRAM command NOP */ + /* DRAM command MRS: enable DQs on this slotrank */ [0] = { .sp_cmd_ctrl = { + .command = IOSAV_MRS, + .ranksel_ap = 1, + }, + .subseq_ctrl = { + .cmd_executions = 1, + .cmd_delay_gap = 3, + .post_ssq_wait = tWLMRD, + .data_direction = SSQ_NA, + }, + .sp_cmd_addr = { + .address = mr1reg, + .rowbits = 6, + .bank = bank, + .rank = slotrank, + }, + }, + /* DRAM command NOP */ + [1] = { + .sp_cmd_ctrl = { .command = IOSAV_NOP, .ranksel_ap = 1, }, @@ -1742,7 +1767,7 @@ }, }, /* DRAM command NOP */ - [1] = { + [2] = { .sp_cmd_ctrl = { .command = IOSAV_NOP_ALT, .ranksel_ap = 1, @@ -1760,6 +1785,25 @@ .rank = slotrank, }, }, + /* DRAM command MRS: disable DQs on this slotrank */ + [3] = { + .sp_cmd_ctrl = { + .command = IOSAV_MRS, + .ranksel_ap = 1, + }, + .subseq_ctrl = { + .cmd_executions = 1, + .cmd_delay_gap = 3, + .post_ssq_wait = ctrl->tMOD, + .data_direction = SSQ_NA, + }, + .sp_cmd_addr = { + .address = mr1reg | 1 << 12, + .rowbits = 6, + .bank = bank, + .rank = slotrank, + }, + }, }; iosav_write_sequence(channel, sequence, ARRAY_SIZE(sequence));
@@ -1767,10 +1811,6 @@ iosav_run_once(channel);
wait_for_iosav(channel); - - /* disable DQs on this slotrank */ - write_mrreg(ctrl, channel, slotrank, 1, - make_mr1(ctrl, slotrank, channel) | 1 << 12 | 1 << 7); }
static int discover_timB(ramctr_timing *ctrl, int channel, int slotrank)
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47614 )
Change subject: nb/intel/sandybridge: Use one sequence for write leveling ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47614/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47614/1/src/northbridge/intel/sandy... PS1, Line 1725: if (ctrl->rank_mirror[channel][slotrank]) : ddr3_mirror_mrreg(&bank, &mr1reg); This looks like a functional difference. Maybe mention it in the commit message?
Hello build bot (Jenkins), Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47614
to look at the new patch set (#2).
Change subject: nb/intel/sandybridge: Use one sequence for write leveling ......................................................................
nb/intel/sandybridge: Use one sequence for write leveling
In order to run a write leveling test, one needs to unset the Qoff bit in MR1, then run the test, and finally set Qoff again. The current IOSAV sequence uses two subsequences to perform the test, while the other two are unused. It is possible to perform the two necessary MR1 updates in the same sequence, which can potentially improve runtime (not measured).
Since `write_mrreg` is no longer used, it is necessary to handle address mirroring explicitly. This can be accomplished with the recently-added `ddr3_mirror_mrreg` function, which is also used in `write_mrreg`.
Tested on Asus P8H61-M PRO, still boots.
Change-Id: I65ca1aa32cdb177d2a9e27c3b02e74ac0c882794 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 48 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/47614/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47614 )
Change subject: nb/intel/sandybridge: Use one sequence for write leveling ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47614/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47614/1/src/northbridge/intel/sandy... PS1, Line 1725: if (ctrl->rank_mirror[channel][slotrank]) : ddr3_mirror_mrreg(&bank, &mr1reg);
This looks like a functional difference. […]
It isn't, but I'll add some logic lubricant. Hint: `ddr3_mirror_mrreg` was created in the immediately preceding commit
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47614 )
Change subject: nb/intel/sandybridge: Use one sequence for write leveling ......................................................................
Patch Set 5: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47614 )
Change subject: nb/intel/sandybridge: Use one sequence for write leveling ......................................................................
nb/intel/sandybridge: Use one sequence for write leveling
In order to run a write leveling test, one needs to unset the Qoff bit in MR1, then run the test, and finally set Qoff again. The current IOSAV sequence uses two subsequences to perform the test, while the other two are unused. It is possible to perform the two necessary MR1 updates in the same sequence, which can potentially improve runtime (not measured).
Since `write_mrreg` is no longer used, it is necessary to handle address mirroring explicitly. This can be accomplished with the recently-added `ddr3_mirror_mrreg` function, which is also used in `write_mrreg`.
Tested on Asus P8H61-M PRO, still boots.
Change-Id: I65ca1aa32cdb177d2a9e27c3b02e74ac0c882794 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47614 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 48 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified 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 885689c..92d0c4f 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -1696,15 +1696,40 @@
static void test_timB(ramctr_timing *ctrl, int channel, int slotrank) { - /* enable DQs on this slotrank */ - write_mrreg(ctrl, channel, slotrank, 1, make_mr1(ctrl, slotrank, channel) | 1 << 7); + /* First DQS/DQS# rising edge after write leveling mode is programmed */ + const u32 tWLMRD = 40; + + u32 mr1reg = make_mr1(ctrl, slotrank, channel) | 1 << 7; + int bank = 1; + + if (ctrl->rank_mirror[channel][slotrank]) + ddr3_mirror_mrreg(&bank, &mr1reg);
wait_for_iosav(channel);
const struct iosav_ssq sequence[] = { - /* DRAM command NOP */ + /* DRAM command MRS: enable DQs on this slotrank */ [0] = { .sp_cmd_ctrl = { + .command = IOSAV_MRS, + .ranksel_ap = 1, + }, + .subseq_ctrl = { + .cmd_executions = 1, + .cmd_delay_gap = 3, + .post_ssq_wait = tWLMRD, + .data_direction = SSQ_NA, + }, + .sp_cmd_addr = { + .address = mr1reg, + .rowbits = 6, + .bank = bank, + .rank = slotrank, + }, + }, + /* DRAM command NOP */ + [1] = { + .sp_cmd_ctrl = { .command = IOSAV_NOP, .ranksel_ap = 1, }, @@ -1722,7 +1747,7 @@ }, }, /* DRAM command NOP */ - [1] = { + [2] = { .sp_cmd_ctrl = { .command = IOSAV_NOP_ALT, .ranksel_ap = 1, @@ -1740,6 +1765,25 @@ .rank = slotrank, }, }, + /* DRAM command MRS: disable DQs on this slotrank */ + [3] = { + .sp_cmd_ctrl = { + .command = IOSAV_MRS, + .ranksel_ap = 1, + }, + .subseq_ctrl = { + .cmd_executions = 1, + .cmd_delay_gap = 3, + .post_ssq_wait = ctrl->tMOD, + .data_direction = SSQ_NA, + }, + .sp_cmd_addr = { + .address = mr1reg | 1 << 12, + .rowbits = 6, + .bank = bank, + .rank = slotrank, + }, + }, }; iosav_write_sequence(channel, sequence, ARRAY_SIZE(sequence));
@@ -1747,10 +1791,6 @@ iosav_run_once(channel);
wait_for_iosav(channel); - - /* disable DQs on this slotrank */ - write_mrreg(ctrl, channel, slotrank, 1, - make_mr1(ctrl, slotrank, channel) | 1 << 12 | 1 << 7); }
static int discover_timB(ramctr_timing *ctrl, int channel, int slotrank)
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Felix Held (Code Review)