[coreboot-gerrit] Change in coreboot[master]: device/dram/ddr2.c: Improve error returning and debug output

Arthur Heymans (Code Review) gerrit at coreboot.org
Sun Sep 10 21:01:25 CEST 2017


Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/21480


Change subject: device/dram/ddr2.c: Improve error returning and debug output
......................................................................

device/dram/ddr2.c: Improve error returning and debug output

This patch outputs decoding errors with BIOS_WARNING instead of
depending on CONFIG_DEBUG_RAM_SETUP.

Returns SPD_STATUS_INVALID on invalid settings for tRR, bcd and tCK.

Change-Id: Iee434d1fa1a9d911cc3683b88b260881ed6434ea
Signed-off-by: Arthur Heymans <arthur at aheymans.xyz>
---
M src/device/dram/ddr2.c
1 file changed, 88 insertions(+), 36 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/21480/1

diff --git a/src/device/dram/ddr2.c b/src/device/dram/ddr2.c
index 62c1079..aa6027f 100644
--- a/src/device/dram/ddr2.c
+++ b/src/device/dram/ddr2.c
@@ -113,9 +113,10 @@
  * Decodes a raw SPD data from a DDR2 DIMM.
  * Returns cycle time in 1/256th ns.
  */
-static u32 spd_decode_tck_time(u8 c)
+static int spd_decode_tck_time(u8 c, u32 *tck)
 {
 	u8 high, low;
+	int ret = CB_SUCCESS;
 
 	high = c >> 4;
 
@@ -132,11 +133,19 @@
 	case 0xd:
 		low = 75;
 		break;
+	case 0xe:
+	case 0xf:
+		printk(BIOS_WARNING, "Invalid tck setting. "
+			"Using 0 for lower nibble\n");
+		low = 0;
+		ret = CB_ERR;
+		break;
 	default:
 		low = (c & 0xf) * 10;
 	}
 
-	return ((high * 100 + low) << 8) / 100;
+	*tck = ((high * 100 + low) << 8) / 100;
+	return ret;
 }
 
 /**
@@ -145,14 +154,18 @@
  * Decodes a raw SPD data from a DDR2 DIMM.
  * Returns cycle time in 1/256th ns.
  */
-static u32 spd_decode_bcd_time(u8 c)
+static u32 spd_decode_bcd_time(u8 c, u32 *bcd)
 {
 	u8 high, low;
+	int ret = CB_SUCCESS;
 
 	high = c >> 4;
 	low = c & 0xf;
+	if (high > 10 || low > 10)
+		ret = CB_ERR;
 
-	return ((high * 10 + low) << 8) / 100;
+	*bcd = ((high * 10 + low) << 8) / 100;
+	return ret;
 }
 
 /**
@@ -177,26 +190,29 @@
  * Decodes a raw SPD data from a DDR2 DIMM.
  * Returns cycle time in 1/256th us.
  */
-static u32 spd_decode_tRR_time(u8 c)
+static int spd_decode_tRR_time(u8 c, u32 *tRR)
 {
+	int ret = CB_SUCCESS;
 	switch (c) {
 	default:
-		printk(BIOS_DEBUG,
+		printk(BIOS_WARNING,
 			"Unknown tRR value, using default of 15.6us.");
+		ret = CB_ERR;
 		/* Fallthrough */
 	case 0x80:
-		return 15625 << 8;
+		*tRR = 15625 << 8;
 	case 0x81:
-		return 15625 << 6;
+		*tRR = 15625 << 6;
 	case 0x82:
-		return 15625 << 7;
+		*tRR = 15625 << 7;
 	case 0x83:
-		return 15625 << 9;
+		*tRR = 15625 << 9;
 	case 0x84:
-		return 15625 << 10;
+		*tRR = 15625 << 10;
 	case 0x85:
-		return 15625 << 11;
+		*tRR = 15625 << 11;
 	}
+	return ret;
 }
 
 /**
@@ -297,20 +313,20 @@
 	printram("SPD contains 0x%02x bytes\n", spd_size);
 
 	if (spd_size < 64 || eeprom_size < 64) {
-		printram("ERROR: SPD to small\n");
+		printk(BIOS_WARNING, "ERROR: SPD to small\n");
 		dimm->dram_type = SPD_MEMORY_TYPE_UNDEFINED;
 		return SPD_STATUS_INVALID;
 	}
 
 	if (spd_ddr2_calc_checksum(spd, spd_size) != spd[63]) {
-		printram("ERROR: SPD checksum error\n");
+		printk(BIOS_WARNING, "ERROR: SPD checksum error\n");
 		dimm->dram_type = SPD_MEMORY_TYPE_UNDEFINED;
 		return SPD_STATUS_CRC_ERROR;
 	}
 
 	reg8 = spd[62];
 	if ((reg8 & 0xf0) != 0x10) {
-		printram("ERROR: Unsupported SPD revision %01x.%01x\n",
+	        printk(BIOS_WARNING, "ERROR: Unsupported SPD revision %01x.%01x\n",
 				reg8 >> 4, reg8 & 0xf);
 		dimm->dram_type = SPD_MEMORY_TYPE_UNDEFINED;
 		return SPD_STATUS_INVALID;
@@ -321,7 +337,7 @@
 	reg8 = spd[2];
 	printram("  Type               : 0x%02x\n", reg8);
 	if (reg8 != 0x08) {
-		printram("ERROR: Unsupported SPD type %x\n", reg8);
+	        printk(BIOS_WARNING, "ERROR: Unsupported SPD type %x\n", reg8);
 		dimm->dram_type = SPD_MEMORY_TYPE_UNDEFINED;
 		return SPD_STATUS_INVALID;
 	}
@@ -331,14 +347,14 @@
 	printram("  Rows               : %u\n", dimm->row_bits);
 	if ((dimm->row_bits > 31) ||
 	    ((dimm->row_bits > 15) && (dimm->rev < 0x13))) {
-		printram("  Invalid number of memory rows\n");
+	        printk(BIOS_WARNING, "  Invalid number of memory rows\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 
 	dimm->col_bits = spd[4];
 	printram("  Columns            : %u\n", dimm->col_bits);
 	if (dimm->col_bits > 15) {
-		printram("  Invalid number of memory columns\n");
+	        printk(BIOS_WARNING, "  Invalid number of memory columns\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 
@@ -348,21 +364,21 @@
 	dimm->mod_width = spd[6];
 	printram("  Module data width  : x%u\n", dimm->mod_width);
 	if (!dimm->mod_width) {
-		printram("  Invalid module data width\n");
+	        printk(BIOS_WARNING, "  Invalid module data width\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 
 	dimm->width = spd[13];
 	printram("  SDRAM width        : x%u\n", dimm->width);
 	if (!dimm->width) {
-		printram("  Invalid SDRAM width\n");
+	        printk(BIOS_WARNING, "  Invalid SDRAM width\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 
 	dimm->banks = spd[17];
 	printram("  Banks              : %u\n", dimm->banks);
 	if (!dimm->banks) {
-		printram("  Invalid module banks count\n");
+	        printk(BIOS_WARNING, "  Invalid module banks count\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 
@@ -392,23 +408,23 @@
 		printram("  Voltage            : 1.8V\n");
 		break;
 	default:
-		printram("  Unknown voltage level.\n");
+	        printk(BIOS_WARNING, "  Unknown voltage level.\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 
 	dimm->cas_supported = spd[18];
 	if ((dimm->cas_supported & 0x3) || !dimm->cas_supported) {
-		printram("  Invalid CAS support advertised.\n");
+	        printk(BIOS_WARNING, "  Invalid CAS support advertised.\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 	printram("  Supported CAS mask : 0x%x\n", dimm->cas_supported);
 
 	if ((dimm->rev < 0x13) && (dimm->cas_supported & 0x80)) {
-		printram("  Invalid CAS support advertised.\n");
+	        printk(BIOS_WARNING, "  Invalid CAS support advertised.\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 	if ((dimm->rev < 0x12) && (dimm->cas_supported & 0x40)) {
-		printram("  Invalid CAS support advertised.\n");
+	        printk(BIOS_WARNING, "  Invalid CAS support advertised.\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 
@@ -416,26 +432,52 @@
 	cl = spd_get_msbs(dimm->cas_supported);
 
 	/* SDRAM Cycle time at Maximum Supported CAS Latency (CL), CL=X */
-	dimm->cycle_time[cl] = spd_decode_tck_time(spd[9]);
+	if (spd_decode_tck_time(spd[9], &dimm->cycle_time[cl]) != CB_SUCCESS) {
+		printk(BIOS_WARNING, "Invalid min tCL for CAS%d\n", cl);
+		ret = SPD_STATUS_INVALID_FIELD;
+	}
 	/* SDRAM Access from Clock */
-	dimm->access_time[cl] = spd_decode_bcd_time(spd[10]);
+	if (spd_decode_bcd_time(spd[10], &dimm->access_time[cl]) != CB_SUCCESS) {
+		printk(BIOS_WARNING, "Invalid min tAC for CAS%d\n", cl);
+		ret = SPD_STATUS_INVALID_FIELD;
+	}
 
 	if (dimm->cas_supported & (1 << (cl - 1))) {
 		/* Minimum Clock Cycle at CLX-1 */
-		dimm->cycle_time[cl - 1] = spd_decode_tck_time(spd[23]);
+		if (spd_decode_tck_time(spd[23], &dimm->cycle_time[cl - 1])
+				!= CB_SUCCESS) {
+			printk(BIOS_WARNING, "Invalid min tCL for CAS%d\n",
+				cl - 1);
+			ret = SPD_STATUS_INVALID_FIELD;
+		}
 		/* Maximum Data Access Time (tAC) from Clock at CLX-1 */
-		dimm->access_time[cl - 1] = spd_decode_bcd_time(spd[24]);
+		if (spd_decode_bcd_time(spd[24], &dimm->access_time[cl - 1])
+				!= CB_SUCCESS) {
+			printk(BIOS_WARNING, "Invalid min tAC for CAS%d\n",
+				cl - 1);
+			ret = SPD_STATUS_INVALID_FIELD;
+		}
 	}
 	if (dimm->cas_supported & (1 << (cl - 2))) {
 		/* Minimum Clock Cycle at CLX-2 */
-		dimm->cycle_time[cl - 2] = spd_decode_tck_time(spd[25]);
+		if (spd_decode_tck_time(spd[25], &dimm->cycle_time[cl - 2])
+				!= CB_SUCCESS) {
+			printk(BIOS_WARNING, "Invalid min tCL for CAS%d\n",
+				cl - 2);
+			ret = SPD_STATUS_INVALID_FIELD;
+		}
 		/* Maximum Data Access Time (tAC) from Clock at CLX-2 */
-		dimm->access_time[cl - 2] = spd_decode_bcd_time(spd[26]);
+		if (spd_decode_bcd_time(spd[26], &dimm->access_time[cl - 2])
+				!= CB_SUCCESS) {
+			printk(BIOS_WARNING, "Invalid min tAC for CAS%d\n", cl - 2);
+			ret = SPD_STATUS_INVALID_FIELD;
+		}
+		
 	}
 
 	reg8 = (spd[31] >> 5) | (spd[31] << 3);
 	if (!reg8) {
-		printram("  Invalid rank density.\n");
+	        printk(BIOS_WARNING,"  Invalid rank density.\n");
 		ret = SPD_STATUS_INVALID_FIELD;
 	}
 
@@ -449,7 +491,10 @@
 		printram("  Capacity           : %u GB\n", dimm->size_mb >> 10);
 
 	/* SDRAM Maximum Cycle Time (tCKmax) */
-	dimm->tCK = spd_decode_tck_time(spd[43]);
+	if (spd_decode_bcd_time(spd[43], &dimm->tCK) != CB_SUCCESS) {
+		printk(BIOS_WARNING, "Invalid Max tCK\n");
+		ret = SPD_STATUS_INVALID_FIELD;
+	}
 	/* Minimum Write Recovery Time (tWRmin) */
 	dimm->tWR = spd_decode_quarter_time(spd[36]);
 	/* Minimum RAS# to CAS# Delay Time (tRCDmin) */
@@ -468,9 +513,15 @@
 	/* Minimum Internal Read to Precharge Command Delay Time (tRTPmin) */
 	dimm->tRTP = spd_decode_quarter_time(spd[38]);
 	/* Data Input Setup Time Before Strobe */
-	dimm->tDS = spd_decode_bcd_time(spd[34]);
+	if (spd_decode_bcd_time(spd[34], &dimm->tDS) != CB_SUCCESS) {
+		printk(BIOS_WARNING, "Invalid tDS\n");
+		ret = SPD_STATUS_INVALID_FIELD;
+	}
 	/* Data Input Hold Time After Strobe */
-	dimm->tDH = spd_decode_bcd_time(spd[35]);
+	if (spd_decode_bcd_time(spd[35], &dimm->tDH) != CB_SUCCESS) {
+		printk(BIOS_WARNING, "Invalid tDH\n");
+		ret = SPD_STATUS_INVALID_FIELD;
+	}
 	/* SDRAM Device DQS-DQ Skew for DQS and associated DQ signals */
 	dimm->tDQSQ = (spd[44] << 8) / 100;
 	/* SDRAM Device Maximum Read Data Hold Skew Factor */
@@ -478,7 +529,8 @@
 	/* PLL Relock Time in us */
 	dimm->tPLL = spd[46] << 8;
 	/* Refresh rate in us */
-	dimm->tRR = spd_decode_tRR_time(spd[12]);
+	if (spd_decode_tRR_time(spd[12], &dimm->tRR) != CB_SUCCESS)
+		ret = SPD_STATUS_INVALID_FIELD;
 
 	/* Number of PLLs on DIMM */
 	if (dimm->rev >= 0x11)

-- 
To view, visit https://review.coreboot.org/21480
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iee434d1fa1a9d911cc3683b88b260881ed6434ea
Gerrit-Change-Number: 21480
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur at aheymans.xyz>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20170910/df952e36/attachment-0001.html>


More information about the coreboot-gerrit mailing list