Attention is currently required from: Felix Singer, Nico Huber, Arthur Heymans, Patrick Rudolph. Hello Felix Singer, Nico Huber, Arthur Heymans, Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/49287
to review the following change.
Change subject: nb/intel/sandybridge: Fix programming command timings ......................................................................
nb/intel/sandybridge: Fix programming command timings
Commit 7584e550cc (nb/intel/sandybridge: Clean up program_timings) introduced suspicious code that turned out to be wrong. Correct it.
Tested on Asus P8Z77-V LX2, still boots.
Change-Id: I33b2271803e6fa3d8e625bfe5a830152cab2b9d4 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 48 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/49287/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index eac0b50..f99c4eb 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -927,10 +927,9 @@ void program_timings(ramctr_timing *ctrl, int channel) { u32 reg_roundtrip_latency, reg_io_latency; - int lane; - int slotrank, slot; + int slotrank, lane;
- u32 ctl_delay[NUM_SLOTS] = { 0 }; + int ctl_delay[NUM_SLOTS] = { 0 }; int cmd_delay = 0;
/* Enable CLK XOVER */ @@ -938,60 +937,65 @@ 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. + * Compute command phase shift as the most negative CCC setting + * across all ranks. Use zero if none of the values is negative. */ FOR_ALL_POPULATED_RANKS { - 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 > CCC_MAX_PI) { - printk(BIOS_ERR, "C%d command delay overflow: %d\n", channel, cmd_delay); - cmd_delay = CCC_MAX_PI; + cmd_delay = MAX(cmd_delay, -ctrl->timings[channel][slotrank].pi_coding); }
- /* 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; + /* Compute control phase shift */ + for (int slot = 0; slot < NUM_SLOTS; slot++) {
- const u8 slot_map = (ctrl->rankmap[channel] >> (2 * slot)) & 3; + 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 & 1) + ctl_delay[slot] += ctrl->timings[channel][2 * slot + 0].pi_coding;
- if (slot_map & 2) - ctl_delay[slot] += pi_coding_1 + cmd_delay; + if (slot_map & 2) + ctl_delay[slot] += ctrl->timings[channel][2 * slot + 1].pi_coding;
- /* If both ranks in a slot are populated, use the average */ - if (slot_map == 3) - ctl_delay[slot] /= 2; + /* If both ranks in a slot are populated, use the average */ + if (slot_map == 3) + ctl_delay[slot] /= 2;
- if (ctl_delay[slot] > CCC_MAX_PI) { - printk(BIOS_ERR, "C%dS%d control delay overflow: %d\n", - channel, slot, ctl_delay[slot]); - ctl_delay[slot] = CCC_MAX_PI; - } + ctl_delay[slot] += cmd_delay; + + if (ctl_delay[slot] < 0) { + printk(BIOS_ERR, "C%dS%d control delay underflow: %d\n", + channel, slot, ctl_delay[slot]); + ctl_delay[slot] = 0; } - FOR_ALL_POPULATED_RANKS { - u32 clk_delay = ctrl->timings[channel][slotrank].pi_coding + cmd_delay; - - if (clk_delay > CCC_MAX_PI) { - printk(BIOS_ERR, "C%dR%d clock delay overflow: %d\n", - channel, slotrank, clk_delay); - clk_delay = CCC_MAX_PI; - } - - clk_pi_coding |= (clk_delay % QCLK_PI) << (6 * slotrank); - clk_logic_dly |= (clk_delay / QCLK_PI) << slotrank; + if (ctl_delay[slot] > CCC_MAX_PI) { + printk(BIOS_ERR, "C%dS%d control delay overflow: %d\n", + channel, slot, ctl_delay[slot]); + ctl_delay[slot] = CCC_MAX_PI; } }
+ /* Compute clock phase shift */ + FOR_ALL_POPULATED_RANKS { + int clk_delay = ctrl->timings[channel][slotrank].pi_coding + cmd_delay; + + /* + * Clock is a differential signal, whereas command and control are not. + * This affects its timing, and it is also why it needs a magic offset. + */ + clk_delay += ctrl->pi_code_offset; + + if (clk_delay < 0) { + printk(BIOS_ERR, "C%dR%d clock delay underflow: %d\n", + channel, slotrank, clk_delay); + clk_delay = 0; + } + + /* Clock can safely wrap around because it is a periodic signal */ + clk_delay %= CCC_MAX_PI + 1; + + clk_pi_coding |= (clk_delay % QCLK_PI) << (6 * slotrank); + clk_logic_dly |= (clk_delay / QCLK_PI) << slotrank; + } + /* Enable CMD XOVER */ union gdcr_cmd_pi_coding_reg cmd_pi_coding = { .raw = get_XOVER_CMD(ctrl->rankmap[channel]),