Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47770 )
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
nb/intel/sandybridge: Clean up program_timings
Clarify the clock, command and control programming sequence.
Change-Id: I1aa4144197dc25dc8d6ef1d23e465280bddd95a3 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 68 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/47770/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 399ba5a..e273c9b 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -935,71 +935,89 @@
void program_timings(ramctr_timing *ctrl, int channel) { - u32 reg_roundtrip_latency, reg_pi_code, reg_logic_delay, reg_io_latency; + u32 reg_roundtrip_latency, reg_io_latency; int lane; int slotrank, slot; - int full_shift = 0; - u16 pi_coding_ctrl[NUM_SLOTS];
+ u32 ctl_delay[NUM_SLOTS] = { 0 }; + int cmd_delay = 0; + + /* Enable CLK XOVER */ + u32 clk_pi_coding = get_XOVER_CLK(ctrl->rankmap[channel]); + u32 clk_logic_dly = 0; + + /* + * Apply command delay if desired setting is negative. Find the + * most negative value: 'cmd_delay' will be the absolute value. + */ FOR_ALL_POPULATED_RANKS { - if (full_shift < -ctrl->timings[channel][slotrank].pi_coding) - full_shift = -ctrl->timings[channel][slotrank].pi_coding; + if (cmd_delay < -ctrl->timings[channel][slotrank].pi_coding) + cmd_delay = -ctrl->timings[channel][slotrank].pi_coding; + } + if (cmd_delay < 0) { + printk(BIOS_ERR, "C%d command delay underflow: %d\n", channel, cmd_delay); + cmd_delay = 0; + } + if (cmd_delay >= 128) { + printk(BIOS_ERR, "C%d command delay overflow: %d\n", channel, cmd_delay); + cmd_delay = 127; }
- for (slot = 0; slot < NUM_SLOTS; slot++) - switch ((ctrl->rankmap[channel] >> (2 * slot)) & 3) { - case 0: - default: - pi_coding_ctrl[slot] = 0x7f; - break; - case 1: - pi_coding_ctrl[slot] = - ctrl->timings[channel][2 * slot + 0].pi_coding + full_shift; - break; - case 2: - pi_coding_ctrl[slot] = - ctrl->timings[channel][2 * slot + 1].pi_coding + full_shift; - break; - case 3: - pi_coding_ctrl[slot] = - (ctrl->timings[channel][2 * slot].pi_coding + - ctrl->timings[channel][2 * slot + 1].pi_coding) / 2 + full_shift; - break; + /* Apply control and clock delay if desired setting is positive */ + if (cmd_delay == 0) { + for (slot = 0; slot < NUM_SLOTS; slot++) { + const int pi_coding_0 = ctrl->timings[channel][2 * slot + 0].pi_coding; + const int pi_coding_1 = ctrl->timings[channel][2 * slot + 1].pi_coding; + + const u8 slot_map = (ctrl->rankmap[channel] >> (2 * slot)) & 3; + + if (slot_map & 1) + ctl_delay[slot] += pi_coding_0 + cmd_delay; + + if (slot_map & 2) + ctl_delay[slot] += pi_coding_1 + cmd_delay; + + if (slot_map == 3) + ctl_delay[slot] /= 2; + + if (ctl_delay[slot] >= 128) { + printk(BIOS_ERR, "C%dS%d control delay overflow: %d\n", + channel, slot, ctl_delay[slot]); + ctl_delay[slot] = 127; + } } + FOR_ALL_POPULATED_RANKS { + const u32 pi_delay = + ctrl->timings[channel][slotrank].pi_coding + cmd_delay; + u32 logic_delay = pi_delay / 64; + + if (logic_delay > 1) { + printk(BIOS_ERR, "C%dR%d clock delay overflow: %d\n", + channel, slotrank, pi_delay); + logic_delay = 1; + } + + clk_pi_coding |= (pi_delay % 64) << (6 * slotrank); + clk_logic_dly |= logic_delay << slotrank; + } + }
/* Enable CMD XOVER */ union gdcr_cmd_pi_coding_reg cmd_pi_coding = { .raw = get_XOVER_CMD(ctrl->rankmap[channel]), }; - cmd_pi_coding.cmd_pi_code = full_shift & 0x3f; - cmd_pi_coding.cmd_logic_delay = !!(full_shift & 0x40); + cmd_pi_coding.cmd_pi_code = cmd_delay % 64; + cmd_pi_coding.cmd_logic_delay = cmd_delay / 64;
- cmd_pi_coding.ctl_pi_code_d0 = pi_coding_ctrl[0] & 0x3f; - cmd_pi_coding.ctl_pi_code_d1 = pi_coding_ctrl[1] & 0x3f; - cmd_pi_coding.ctl_logic_delay_d0 = !!(pi_coding_ctrl[0] & 0x40); - cmd_pi_coding.ctl_logic_delay_d1 = !!(pi_coding_ctrl[1] & 0x40); + cmd_pi_coding.ctl_pi_code_d0 = ctl_delay[0] % 64; + cmd_pi_coding.ctl_pi_code_d1 = ctl_delay[1] % 64; + cmd_pi_coding.ctl_logic_delay_d0 = ctl_delay[0] / 64; + cmd_pi_coding.ctl_logic_delay_d1 = ctl_delay[1] / 64;
MCHBAR32(GDCRCMDPICODING_ch(channel)) = cmd_pi_coding.raw;
- /* Enable CLK XOVER */ - reg_pi_code = get_XOVER_CLK(ctrl->rankmap[channel]); - reg_logic_delay = 0; - - FOR_ALL_POPULATED_RANKS { - int shift = ctrl->timings[channel][slotrank].pi_coding + full_shift; - int offset_pi_code; - if (shift < 0) - shift = 0; - - offset_pi_code = ctrl->pi_code_offset + shift; - - /* Set CLK phase shift */ - reg_pi_code |= (offset_pi_code & 0x3f) << (6 * slotrank); - reg_logic_delay |= ((offset_pi_code >> 6) & 1) << slotrank; - } - - MCHBAR32(GDCRCKPICODE_ch(channel)) = reg_pi_code; - MCHBAR32(GDCRCKLOGICDELAY_ch(channel)) = reg_logic_delay; + MCHBAR32(GDCRCKPICODE_ch(channel)) = clk_pi_coding; + MCHBAR32(GDCRCKLOGICDELAY_ch(channel)) = clk_logic_dly;
reg_io_latency = MCHBAR32(SC_IO_LATENCY_ch(channel)); reg_io_latency &= ~0xffff; @@ -1010,7 +1028,7 @@ int post_timA_min_high = 7, pre_timA_min_high = 7; int post_timA_max_high = 0, pre_timA_max_high = 0; int shift_402x = 0; - int shift = ctrl->timings[channel][slotrank].pi_coding + full_shift; + int shift = ctrl->timings[channel][slotrank].pi_coding + cmd_delay;
if (shift < 0) shift = 0;
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47770 )
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... PS4, Line 980: if (slot_map == 3) : ctl_delay[slot] /= 2; please add a comment on this one that it takes the average in the case of both slots on a rank being used. took me quite a while to wrap my head around what's happening there
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... PS4, Line 997: logic_delay = 1; when logic_delay is > 1, pi_delay should probably be set to its maximum value as well to saturate the value to its maximum. not sure if it would be easier to check if pi_delay is < 128 and if not clamp it to 127 and either assign the 7th bit to logic_delay then or just drop logic_delay
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47770 )
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... PS4, Line 980: if (slot_map == 3) : ctl_delay[slot] /= 2;
please add a comment on this one that it takes the average in the case of both slots on a rank being […]
Will do.
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... PS4, Line 997: logic_delay = 1;
when logic_delay is > 1, pi_delay should probably be set to its maximum value as well to saturate th […]
I guess saturating the PI coding would make sense. In any case, this can only happen if there's a bug in raminit.
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47770
to look at the new patch set (#5).
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
nb/intel/sandybridge: Clean up program_timings
Clarify the clock, command and control programming sequence.
Change-Id: I1aa4144197dc25dc8d6ef1d23e465280bddd95a3 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 67 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/47770/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47770 )
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... PS4, Line 980: if (slot_map == 3) : ctl_delay[slot] /= 2;
Will do.
I guess you meant "both ranks in a slot", but done
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... PS4, Line 997: logic_delay = 1;
I guess saturating the PI coding would make sense. […]
Removed logic_delay
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47770 )
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47770/4/src/northbridge/intel/sandy... PS4, Line 980: if (slot_map == 3) : ctl_delay[slot] /= 2;
I guess you meant "both ranks in a slot", but done
ah, that would make more sense
Hello Felix Singer, build bot (Jenkins), Nico Huber, Arthur Heymans, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47770
to look at the new patch set (#9).
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
nb/intel/sandybridge: Clean up program_timings
Clarify the clock, command and control programming sequence.
Tested on Asus P8Z77-V LX2, still boots.
Change-Id: I1aa4144197dc25dc8d6ef1d23e465280bddd95a3 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 67 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/47770/9
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47770 )
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
Patch Set 12: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47770 )
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
nb/intel/sandybridge: Clean up program_timings
Clarify the clock, command and control programming sequence.
Tested on Asus P8Z77-V LX2, still boots.
Change-Id: I1aa4144197dc25dc8d6ef1d23e465280bddd95a3 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47770 Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 67 insertions(+), 50 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 80c5186..7252574 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -934,71 +934,88 @@
void program_timings(ramctr_timing *ctrl, int channel) { - u32 reg_roundtrip_latency, reg_pi_code, reg_logic_delay, reg_io_latency; + u32 reg_roundtrip_latency, reg_io_latency; int lane; int slotrank, slot; - int full_shift = 0; - u16 pi_coding_ctrl[NUM_SLOTS];
+ u32 ctl_delay[NUM_SLOTS] = { 0 }; + int cmd_delay = 0; + + /* Enable CLK XOVER */ + u32 clk_pi_coding = get_XOVER_CLK(ctrl->rankmap[channel]); + u32 clk_logic_dly = 0; + + /* + * Apply command delay if desired setting is negative. Find the + * most negative value: 'cmd_delay' will be the absolute value. + */ FOR_ALL_POPULATED_RANKS { - if (full_shift < -ctrl->timings[channel][slotrank].pi_coding) - full_shift = -ctrl->timings[channel][slotrank].pi_coding; + if (cmd_delay < -ctrl->timings[channel][slotrank].pi_coding) + cmd_delay = -ctrl->timings[channel][slotrank].pi_coding; + } + if (cmd_delay < 0) { + printk(BIOS_ERR, "C%d command delay underflow: %d\n", channel, cmd_delay); + cmd_delay = 0; + } + if (cmd_delay >= 128) { + printk(BIOS_ERR, "C%d command delay overflow: %d\n", channel, cmd_delay); + cmd_delay = 127; }
- for (slot = 0; slot < NUM_SLOTS; slot++) - switch ((ctrl->rankmap[channel] >> (2 * slot)) & 3) { - case 0: - default: - pi_coding_ctrl[slot] = 0x7f; - break; - case 1: - pi_coding_ctrl[slot] = - ctrl->timings[channel][2 * slot + 0].pi_coding + full_shift; - break; - case 2: - pi_coding_ctrl[slot] = - ctrl->timings[channel][2 * slot + 1].pi_coding + full_shift; - break; - case 3: - pi_coding_ctrl[slot] = - (ctrl->timings[channel][2 * slot].pi_coding + - ctrl->timings[channel][2 * slot + 1].pi_coding) / 2 + full_shift; - break; + /* Apply control and clock delay if desired setting is positive */ + if (cmd_delay == 0) { + for (slot = 0; slot < NUM_SLOTS; slot++) { + const int pi_coding_0 = ctrl->timings[channel][2 * slot + 0].pi_coding; + const int pi_coding_1 = ctrl->timings[channel][2 * slot + 1].pi_coding; + + const u8 slot_map = (ctrl->rankmap[channel] >> (2 * slot)) & 3; + + if (slot_map & 1) + ctl_delay[slot] += pi_coding_0 + cmd_delay; + + if (slot_map & 2) + ctl_delay[slot] += pi_coding_1 + cmd_delay; + + /* If both ranks in a slot are populated, use the average */ + if (slot_map == 3) + ctl_delay[slot] /= 2; + + if (ctl_delay[slot] >= 128) { + printk(BIOS_ERR, "C%dS%d control delay overflow: %d\n", + channel, slot, ctl_delay[slot]); + ctl_delay[slot] = 127; + } } + FOR_ALL_POPULATED_RANKS { + u32 clk_delay = ctrl->timings[channel][slotrank].pi_coding + cmd_delay; + + if (clk_delay >= 128) { + printk(BIOS_ERR, "C%dR%d clock delay overflow: %d\n", + channel, slotrank, clk_delay); + clk_delay = 127; + } + + clk_pi_coding |= (clk_delay % 64) << (6 * slotrank); + clk_logic_dly |= (clk_delay / 64) << slotrank; + } + }
/* Enable CMD XOVER */ union gdcr_cmd_pi_coding_reg cmd_pi_coding = { .raw = get_XOVER_CMD(ctrl->rankmap[channel]), }; - cmd_pi_coding.cmd_pi_code = full_shift & 0x3f; - cmd_pi_coding.cmd_logic_delay = !!(full_shift & 0x40); + cmd_pi_coding.cmd_pi_code = cmd_delay % 64; + cmd_pi_coding.cmd_logic_delay = cmd_delay / 64;
- cmd_pi_coding.ctl_pi_code_d0 = pi_coding_ctrl[0] & 0x3f; - cmd_pi_coding.ctl_pi_code_d1 = pi_coding_ctrl[1] & 0x3f; - cmd_pi_coding.ctl_logic_delay_d0 = !!(pi_coding_ctrl[0] & 0x40); - cmd_pi_coding.ctl_logic_delay_d1 = !!(pi_coding_ctrl[1] & 0x40); + cmd_pi_coding.ctl_pi_code_d0 = ctl_delay[0] % 64; + cmd_pi_coding.ctl_pi_code_d1 = ctl_delay[1] % 64; + cmd_pi_coding.ctl_logic_delay_d0 = ctl_delay[0] / 64; + cmd_pi_coding.ctl_logic_delay_d1 = ctl_delay[1] / 64;
MCHBAR32(GDCRCMDPICODING_ch(channel)) = cmd_pi_coding.raw;
- /* Enable CLK XOVER */ - reg_pi_code = get_XOVER_CLK(ctrl->rankmap[channel]); - reg_logic_delay = 0; - - FOR_ALL_POPULATED_RANKS { - int shift = ctrl->timings[channel][slotrank].pi_coding + full_shift; - int offset_pi_code; - if (shift < 0) - shift = 0; - - offset_pi_code = ctrl->pi_code_offset + shift; - - /* Set CLK phase shift */ - reg_pi_code |= (offset_pi_code & 0x3f) << (6 * slotrank); - reg_logic_delay |= ((offset_pi_code >> 6) & 1) << slotrank; - } - - MCHBAR32(GDCRCKPICODE_ch(channel)) = reg_pi_code; - MCHBAR32(GDCRCKLOGICDELAY_ch(channel)) = reg_logic_delay; + MCHBAR32(GDCRCKPICODE_ch(channel)) = clk_pi_coding; + MCHBAR32(GDCRCKLOGICDELAY_ch(channel)) = clk_logic_dly;
reg_io_latency = MCHBAR32(SC_IO_LATENCY_ch(channel)); reg_io_latency &= ~0xffff; @@ -1009,7 +1026,7 @@ int post_timA_min_high = 7, pre_timA_min_high = 7; int post_timA_max_high = 0, pre_timA_max_high = 0; int shift_402x = 0; - int shift = ctrl->timings[channel][slotrank].pi_coding + full_shift; + int shift = ctrl->timings[channel][slotrank].pi_coding + cmd_delay;
if (shift < 0) shift = 0;
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47770 )
Change subject: nb/intel/sandybridge: Clean up program_timings ......................................................................
Patch Set 13:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/47770/comment/aa5afcd6_762eca6b PS13, Line 9: Clarify the clock, command and control programming sequence. The commit message is devoid of information. It is so empty it could implode at any moment.
https://review.coreboot.org/c/coreboot/+/47770/comment/806f08c9_94fbe435 PS13, Line 11: Tested on Asus P8Z77-V LX2, still boots. While it is true that the brokenness didn't have a serious impact (unstability was only noticed during overclocking), testing that changes to raminit don't introduce subtle regressions is harder than you think.
Ideally, you would have a testing methodology based on margin analysis, proper hardware documentation and lots of fancy debug equipment... Ideally. For now, the boards that come out of the e-waste place or similar and your patience will have to do.
Patchset:
PS13: Folks, may this change serve as an example of why doing multiple things in one commit is a very bad idea. I have given myself a post-mortem review to point out how much is wrong with it, and wrote it in a somewhat harsh-sounding tone to vent some frustration. I've also linked to the patches (yes, plural) that fix the described issues.
File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47770/comment/fb2a39d8_262247e6 PS13, Line 993: ctrl->pi_code_offset This offset is only used here. Unlike command and control, clock is a differential signal and propagates slightly faster. Therefore, it needs an additional (frequency-dependent) delay to be aligned with the other two signals. CB:49319
https://review.coreboot.org/c/coreboot/+/47770/comment/12fc904b_06c60fff PS13, Line 956: if (cmd_delay < 0) { Always false. CB:49320
https://review.coreboot.org/c/coreboot/+/47770/comment/5e88e1e9_9fafe389 PS13, Line 965: /* Apply control and clock delay if desired setting is positive */ If cmd_delay is always zero, it makes no sense to use it within the if-block. Actually, control and clock delays always need to be programmed. CB:49318
https://review.coreboot.org/c/coreboot/+/47770/comment/f46ed8bc_5391f1d4 PS13, Line 992: if (clk_delay >= 128) { Clock setting can safely overflow, but needs to be wrapped around. Clamping it is wrong. CB:49319
-
Angel Pons (Code Review) -
Felix Held (Code Review)