[coreboot-gerrit] Change in coreboot[master]: nb/intel/sandybridge: Fix handling of clock timing

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber.
Hello Nico Huber,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/49319

to review the following change.

Change subject: nb/intel/sandybridge: Fix handling of clock timing
......................................................................

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 
---
M src/northbridge/intel/sandybridge/raminit_common.c
1 file changed, 14 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/49319/1

diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c
index 092e34d..f766867 100644
--- a/src/northbridge/intel/sandybridge/raminit_common.c
+++ b/src/northbridge/intel/sandybridge/raminit_common.c
@@ -977,14 +977,24 @@
    	}
    }
    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;
    }