From 0a7d99c089eea04ec03958f96dd180bffa6fcc4a Mon Sep 17 00:00:00 2001 From: Angel Pons Date: Tue, 12 Jan 2021 01:13:08 +0100 Subject: nb/intel/sandybridge: Fix handling of clock timing Clock is a differential signal and propagates faster than command and control, therefore its timing needs to be offset with `pi_code_offset`. It is also a periodic signal, so it can safely wrap around. To avoid potential undefined behavior, make `clk_delay` signed. It makes no difference with valid values, because the initial value can be proven to never be negative and `pi_code_offset` is always positive. With this change, it is possible to add an underflow check, for additional sanity. Change-Id: I375adf84142079f341b060fba5e79ce4dcb002be Signed-off-by: Angel Pons Reviewed-on: https://review.coreboot.org/c/coreboot/+/49319 Tested-by: build bot (Jenkins) Reviewed-by: Nico Huber --- src/northbridge/intel/sandybridge/raminit_common.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 092e34dd0a..f766867965 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -977,14 +977,24 @@ void program_timings(ramctr_timing *ctrl, int channel) } } FOR_ALL_POPULATED_RANKS { - u32 clk_delay = ctrl->timings[channel][slotrank].pi_coding + cmd_delay; + int 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", + /* + * 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; + + /* Can never happen with valid values */ + if (clk_delay < 0) { + printk(BIOS_ERR, "C%dR%d clock delay underflow: %d\n", channel, slotrank, clk_delay); - clk_delay = CCC_MAX_PI; + 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; } -- cgit v1.2.3