<p>Arthur Heymans has uploaded this change for <strong>review</strong>.</p><p><a href="https://review.coreboot.org/21480">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">device/dram/ddr2.c: Improve error returning and debug output<br><br>This patch outputs decoding errors with BIOS_WARNING instead of<br>depending on CONFIG_DEBUG_RAM_SETUP.<br><br>Returns SPD_STATUS_INVALID on invalid settings for tRR, bcd and tCK.<br><br>Change-Id: Iee434d1fa1a9d911cc3683b88b260881ed6434ea<br>Signed-off-by: Arthur Heymans <arthur@aheymans.xyz><br>---<br>M src/device/dram/ddr2.c<br>1 file changed, 88 insertions(+), 36 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/21480/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/src/device/dram/ddr2.c b/src/device/dram/ddr2.c<br>index 62c1079..aa6027f 100644<br>--- a/src/device/dram/ddr2.c<br>+++ b/src/device/dram/ddr2.c<br>@@ -113,9 +113,10 @@<br> * Decodes a raw SPD data from a DDR2 DIMM.<br> * Returns cycle time in 1/256th ns.<br> */<br>-static u32 spd_decode_tck_time(u8 c)<br>+static int spd_decode_tck_time(u8 c, u32 *tck)<br> {<br> u8 high, low;<br>+ int ret = CB_SUCCESS;<br> <br> high = c >> 4;<br> <br>@@ -132,11 +133,19 @@<br> case 0xd:<br> low = 75;<br> break;<br>+ case 0xe:<br>+ case 0xf:<br>+ printk(BIOS_WARNING, "Invalid tck setting. "<br>+ "Using 0 for lower nibble\n");<br>+ low = 0;<br>+ ret = CB_ERR;<br>+ break;<br> default:<br> low = (c & 0xf) * 10;<br> }<br> <br>- return ((high * 100 + low) << 8) / 100;<br>+ *tck = ((high * 100 + low) << 8) / 100;<br>+ return ret;<br> }<br> <br> /**<br>@@ -145,14 +154,18 @@<br> * Decodes a raw SPD data from a DDR2 DIMM.<br> * Returns cycle time in 1/256th ns.<br> */<br>-static u32 spd_decode_bcd_time(u8 c)<br>+static u32 spd_decode_bcd_time(u8 c, u32 *bcd)<br> {<br> u8 high, low;<br>+ int ret = CB_SUCCESS;<br> <br> high = c >> 4;<br> low = c & 0xf;<br>+ if (high > 10 || low > 10)<br>+ ret = CB_ERR;<br> <br>- return ((high * 10 + low) << 8) / 100;<br>+ *bcd = ((high * 10 + low) << 8) / 100;<br>+ return ret;<br> }<br> <br> /**<br>@@ -177,26 +190,29 @@<br> * Decodes a raw SPD data from a DDR2 DIMM.<br> * Returns cycle time in 1/256th us.<br> */<br>-static u32 spd_decode_tRR_time(u8 c)<br>+static int spd_decode_tRR_time(u8 c, u32 *tRR)<br> {<br>+ int ret = CB_SUCCESS;<br> switch (c) {<br> default:<br>- printk(BIOS_DEBUG,<br>+ printk(BIOS_WARNING,<br> "Unknown tRR value, using default of 15.6us.");<br>+ ret = CB_ERR;<br> /* Fallthrough */<br> case 0x80:<br>- return 15625 << 8;<br>+ *tRR = 15625 << 8;<br> case 0x81:<br>- return 15625 << 6;<br>+ *tRR = 15625 << 6;<br> case 0x82:<br>- return 15625 << 7;<br>+ *tRR = 15625 << 7;<br> case 0x83:<br>- return 15625 << 9;<br>+ *tRR = 15625 << 9;<br> case 0x84:<br>- return 15625 << 10;<br>+ *tRR = 15625 << 10;<br> case 0x85:<br>- return 15625 << 11;<br>+ *tRR = 15625 << 11;<br> }<br>+ return ret;<br> }<br> <br> /**<br>@@ -297,20 +313,20 @@<br> printram("SPD contains 0x%02x bytes\n", spd_size);<br> <br> if (spd_size < 64 || eeprom_size < 64) {<br>- printram("ERROR: SPD to small\n");<br>+ printk(BIOS_WARNING, "ERROR: SPD to small\n");<br> dimm->dram_type = SPD_MEMORY_TYPE_UNDEFINED;<br> return SPD_STATUS_INVALID;<br> }<br> <br> if (spd_ddr2_calc_checksum(spd, spd_size) != spd[63]) {<br>- printram("ERROR: SPD checksum error\n");<br>+ printk(BIOS_WARNING, "ERROR: SPD checksum error\n");<br> dimm->dram_type = SPD_MEMORY_TYPE_UNDEFINED;<br> return SPD_STATUS_CRC_ERROR;<br> }<br> <br> reg8 = spd[62];<br> if ((reg8 & 0xf0) != 0x10) {<br>- printram("ERROR: Unsupported SPD revision %01x.%01x\n",<br>+ printk(BIOS_WARNING, "ERROR: Unsupported SPD revision %01x.%01x\n",<br> reg8 >> 4, reg8 & 0xf);<br> dimm->dram_type = SPD_MEMORY_TYPE_UNDEFINED;<br> return SPD_STATUS_INVALID;<br>@@ -321,7 +337,7 @@<br> reg8 = spd[2];<br> printram(" Type : 0x%02x\n", reg8);<br> if (reg8 != 0x08) {<br>- printram("ERROR: Unsupported SPD type %x\n", reg8);<br>+ printk(BIOS_WARNING, "ERROR: Unsupported SPD type %x\n", reg8);<br> dimm->dram_type = SPD_MEMORY_TYPE_UNDEFINED;<br> return SPD_STATUS_INVALID;<br> }<br>@@ -331,14 +347,14 @@<br> printram(" Rows : %u\n", dimm->row_bits);<br> if ((dimm->row_bits > 31) ||<br> ((dimm->row_bits > 15) && (dimm->rev < 0x13))) {<br>- printram(" Invalid number of memory rows\n");<br>+ printk(BIOS_WARNING, " Invalid number of memory rows\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> <br> dimm->col_bits = spd[4];<br> printram(" Columns : %u\n", dimm->col_bits);<br> if (dimm->col_bits > 15) {<br>- printram(" Invalid number of memory columns\n");<br>+ printk(BIOS_WARNING, " Invalid number of memory columns\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> <br>@@ -348,21 +364,21 @@<br> dimm->mod_width = spd[6];<br> printram(" Module data width : x%u\n", dimm->mod_width);<br> if (!dimm->mod_width) {<br>- printram(" Invalid module data width\n");<br>+ printk(BIOS_WARNING, " Invalid module data width\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> <br> dimm->width = spd[13];<br> printram(" SDRAM width : x%u\n", dimm->width);<br> if (!dimm->width) {<br>- printram(" Invalid SDRAM width\n");<br>+ printk(BIOS_WARNING, " Invalid SDRAM width\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> <br> dimm->banks = spd[17];<br> printram(" Banks : %u\n", dimm->banks);<br> if (!dimm->banks) {<br>- printram(" Invalid module banks count\n");<br>+ printk(BIOS_WARNING, " Invalid module banks count\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> <br>@@ -392,23 +408,23 @@<br> printram(" Voltage : 1.8V\n");<br> break;<br> default:<br>- printram(" Unknown voltage level.\n");<br>+ printk(BIOS_WARNING, " Unknown voltage level.\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> <br> dimm->cas_supported = spd[18];<br> if ((dimm->cas_supported & 0x3) || !dimm->cas_supported) {<br>- printram(" Invalid CAS support advertised.\n");<br>+ printk(BIOS_WARNING, " Invalid CAS support advertised.\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> printram(" Supported CAS mask : 0x%x\n", dimm->cas_supported);<br> <br> if ((dimm->rev < 0x13) && (dimm->cas_supported & 0x80)) {<br>- printram(" Invalid CAS support advertised.\n");<br>+ printk(BIOS_WARNING, " Invalid CAS support advertised.\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> if ((dimm->rev < 0x12) && (dimm->cas_supported & 0x40)) {<br>- printram(" Invalid CAS support advertised.\n");<br>+ printk(BIOS_WARNING, " Invalid CAS support advertised.\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> <br>@@ -416,26 +432,52 @@<br> cl = spd_get_msbs(dimm->cas_supported);<br> <br> /* SDRAM Cycle time at Maximum Supported CAS Latency (CL), CL=X */<br>- dimm->cycle_time[cl] = spd_decode_tck_time(spd[9]);<br>+ if (spd_decode_tck_time(spd[9], &dimm->cycle_time[cl]) != CB_SUCCESS) {<br>+ printk(BIOS_WARNING, "Invalid min tCL for CAS%d\n", cl);<br>+ ret = SPD_STATUS_INVALID_FIELD;<br>+ }<br> /* SDRAM Access from Clock */<br>- dimm->access_time[cl] = spd_decode_bcd_time(spd[10]);<br>+ if (spd_decode_bcd_time(spd[10], &dimm->access_time[cl]) != CB_SUCCESS) {<br>+ printk(BIOS_WARNING, "Invalid min tAC for CAS%d\n", cl);<br>+ ret = SPD_STATUS_INVALID_FIELD;<br>+ }<br> <br> if (dimm->cas_supported & (1 << (cl - 1))) {<br> /* Minimum Clock Cycle at CLX-1 */<br>- dimm->cycle_time[cl - 1] = spd_decode_tck_time(spd[23]);<br>+ if (spd_decode_tck_time(spd[23], &dimm->cycle_time[cl - 1])<br>+ != CB_SUCCESS) {<br>+ printk(BIOS_WARNING, "Invalid min tCL for CAS%d\n",<br>+ cl - 1);<br>+ ret = SPD_STATUS_INVALID_FIELD;<br>+ }<br> /* Maximum Data Access Time (tAC) from Clock at CLX-1 */<br>- dimm->access_time[cl - 1] = spd_decode_bcd_time(spd[24]);<br>+ if (spd_decode_bcd_time(spd[24], &dimm->access_time[cl - 1])<br>+ != CB_SUCCESS) {<br>+ printk(BIOS_WARNING, "Invalid min tAC for CAS%d\n",<br>+ cl - 1);<br>+ ret = SPD_STATUS_INVALID_FIELD;<br>+ }<br> }<br> if (dimm->cas_supported & (1 << (cl - 2))) {<br> /* Minimum Clock Cycle at CLX-2 */<br>- dimm->cycle_time[cl - 2] = spd_decode_tck_time(spd[25]);<br>+ if (spd_decode_tck_time(spd[25], &dimm->cycle_time[cl - 2])<br>+ != CB_SUCCESS) {<br>+ printk(BIOS_WARNING, "Invalid min tCL for CAS%d\n",<br>+ cl - 2);<br>+ ret = SPD_STATUS_INVALID_FIELD;<br>+ }<br> /* Maximum Data Access Time (tAC) from Clock at CLX-2 */<br>- dimm->access_time[cl - 2] = spd_decode_bcd_time(spd[26]);<br>+ if (spd_decode_bcd_time(spd[26], &dimm->access_time[cl - 2])<br>+ != CB_SUCCESS) {<br>+ printk(BIOS_WARNING, "Invalid min tAC for CAS%d\n", cl - 2);<br>+ ret = SPD_STATUS_INVALID_FIELD;<br>+ }<br>+ <br> }<br> <br> reg8 = (spd[31] >> 5) | (spd[31] << 3);<br> if (!reg8) {<br>- printram(" Invalid rank density.\n");<br>+ printk(BIOS_WARNING," Invalid rank density.\n");<br> ret = SPD_STATUS_INVALID_FIELD;<br> }<br> <br>@@ -449,7 +491,10 @@<br> printram(" Capacity : %u GB\n", dimm->size_mb >> 10);<br> <br> /* SDRAM Maximum Cycle Time (tCKmax) */<br>- dimm->tCK = spd_decode_tck_time(spd[43]);<br>+ if (spd_decode_bcd_time(spd[43], &dimm->tCK) != CB_SUCCESS) {<br>+ printk(BIOS_WARNING, "Invalid Max tCK\n");<br>+ ret = SPD_STATUS_INVALID_FIELD;<br>+ }<br> /* Minimum Write Recovery Time (tWRmin) */<br> dimm->tWR = spd_decode_quarter_time(spd[36]);<br> /* Minimum RAS# to CAS# Delay Time (tRCDmin) */<br>@@ -468,9 +513,15 @@<br> /* Minimum Internal Read to Precharge Command Delay Time (tRTPmin) */<br> dimm->tRTP = spd_decode_quarter_time(spd[38]);<br> /* Data Input Setup Time Before Strobe */<br>- dimm->tDS = spd_decode_bcd_time(spd[34]);<br>+ if (spd_decode_bcd_time(spd[34], &dimm->tDS) != CB_SUCCESS) {<br>+ printk(BIOS_WARNING, "Invalid tDS\n");<br>+ ret = SPD_STATUS_INVALID_FIELD;<br>+ }<br> /* Data Input Hold Time After Strobe */<br>- dimm->tDH = spd_decode_bcd_time(spd[35]);<br>+ if (spd_decode_bcd_time(spd[35], &dimm->tDH) != CB_SUCCESS) {<br>+ printk(BIOS_WARNING, "Invalid tDH\n");<br>+ ret = SPD_STATUS_INVALID_FIELD;<br>+ }<br> /* SDRAM Device DQS-DQ Skew for DQS and associated DQ signals */<br> dimm->tDQSQ = (spd[44] << 8) / 100;<br> /* SDRAM Device Maximum Read Data Hold Skew Factor */<br>@@ -478,7 +529,8 @@<br> /* PLL Relock Time in us */<br> dimm->tPLL = spd[46] << 8;<br> /* Refresh rate in us */<br>- dimm->tRR = spd_decode_tRR_time(spd[12]);<br>+ if (spd_decode_tRR_time(spd[12], &dimm->tRR) != CB_SUCCESS)<br>+ ret = SPD_STATUS_INVALID_FIELD;<br> <br> /* Number of PLLs on DIMM */<br> if (dimm->rev >= 0x11)<br></pre><p>To view, visit <a href="https://review.coreboot.org/21480">change 21480</a>. To unsubscribe, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/21480"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: Iee434d1fa1a9d911cc3683b88b260881ed6434ea </div>
<div style="display:none"> Gerrit-Change-Number: 21480 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> </div>