Change in coreboot[master]: dram: Add CRC16 checks for DDR4

[Andrey Petrov (Code Review)]

Andrey Petrov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34781 )

Change subject: dram: Add CRC16 checks for DDR4
......................................................................

dram: Add CRC16 checks for DDR4

Move crc routine in common file as it shared for all memory types.
Implement crc check for DDR4.

TEST=tested on Broadwell-DE (OCP Monolake) with few memory modules.

Signed-off-by: Andrey Petrov 
Change-Id: Ief5a5f0575f3b8e34bb050b1e09a4b2462a93450
Signed-off-by: Andrey Petrov 
---
M src/device/dram/Makefile.inc
M src/device/dram/ddr2.c
M src/device/dram/ddr3.c
M src/device/dram/ddr4.c
A src/device/dram/ddr_common.c
M src/include/device/dram/common.h
6 files changed, 152 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/34781/1

diff --git a/src/device/dram/Makefile.inc b/src/device/dram/Makefile.inc
index 377a2c0..f397a53 100644
--- a/src/device/dram/Makefile.inc
+++ b/src/device/dram/Makefile.inc
@@ -1 +1 @@
-romstage-y += ddr4.c ddr3.c ddr2.c
+romstage-y += ddr4.c ddr3.c ddr2.c ddr_common.c
diff --git a/src/device/dram/ddr2.c b/src/device/dram/ddr2.c
index 60588b8..d66b576 100644
--- a/src/device/dram/ddr2.c
+++ b/src/device/dram/ddr2.c
@@ -93,7 +93,7 @@
    for (i = 93; i <= 98; i++)
    	id_bytes[j++] = spd[i];

-	return ddr3_crc16(id_bytes, 15);
+	return ddr_crc16(id_bytes, 15);
 }

/**
diff --git a/src/device/dram/ddr3.c b/src/device/dram/ddr3.c
index 834dc83..4a900fa 100644
--- a/src/device/dram/ddr3.c
+++ b/src/device/dram/ddr3.c
@@ -23,6 +23,7 @@
 #include <console/console.h>
 #include <device/device.h>
 #include <device/dram/ddr3.h>
+#include <device/dram/common.h>
 #include <string.h>
 #include <memory_info.h>
 #include <cbmem.h>
@@ -50,24 +51,6 @@
    return 0;
 }

-u16 ddr3_crc16(const u8 *ptr, int n_crc)
-{
-	int i;
-	u16 crc = 0;
-
-	while (--n_crc >= 0) {
-		crc = crc ^ ((int)*ptr++ << 8);
-		for (i = 0; i < 8; ++i)
-			if (crc & 0x8000) {
-				crc = (crc << 1) ^ 0x1021;
-			} else {
-				crc = crc << 1;
-			}
-	}
-
-	return crc;
-}
-
 /**
  * \brief Calculate the CRC of a DDR3 SPD
  *
@@ -91,7 +74,7 @@
    	/* Not enough bytes available to get the CRC */
    	return 0;

-	return ddr3_crc16(spd, n_crc);
+	return ddr_crc16(spd, n_crc);
 }

/**
@@ -108,7 +91,7 @@
    	/* Not enough bytes available to get the CRC */
    	return 0;

-	return ddr3_crc16(&spd[117], 11);
+	return ddr_crc16(&spd[117], 11);
 }

/**
diff --git a/src/device/dram/ddr4.c b/src/device/dram/ddr4.c
index 0e7caad..86318ce 100644
--- a/src/device/dram/ddr4.c
+++ b/src/device/dram/ddr4.c
@@ -22,6 +22,66 @@
 #include <smbios.h>
 #include <types.h>

+typedef enum {
+	BLOCK_0, /* Base Configuration and DRAM Parameters */
+	BLOCK_1,
+	BLOCK_1_L, /* Standard Module Parameters */
+	BLOCK_1_H, /* Hybrid Module Parameters */
+	BLOCK_2,
+	BLOCK_2_L, /* Hybrid Module Extended Function Parameters */
+	BLOCK_2_H, /* Manufacturing Information */
+	BLOCK_3    /* End user programmable */
+} spd_block_type;
+
+typedef struct {
+	spd_block_type type;
+	uint16_t start;     /* starting offset from beginning of the spd */
+	uint16_t len;       /* size of the block */
+	uint16_t crc_start; /* offset from start of crc bytes, 0 if none */
+} spd_block;
+
+/* 'SPD contents architecture' as per datasheet */
+const spd_block spd_blocks[] = {
+	{.type = BLOCK_0, 0, 128, 126},   {.type = BLOCK_1, 128, 128, 126},
+	{.type = BLOCK_1_L, 128, 64, 0},  {.type = BLOCK_1_H, 192, 64, 0},
+	{.type = BLOCK_2_L, 256, 64, 62}, {.type = BLOCK_2_H, 320, 64, 0},
+	{.type = BLOCK_3, 384, 128, 0}};
+
+static bool verify_block(const spd_block *block, spd_raw_data spd)
+{
+	uint16_t crc, spd_crc;
+
+	spd_crc = (spd[block->start + block->crc_start + 1] << 8)
+		  | spd[block->start + block->crc_start];
+	crc = ddr_crc16(&spd[block->start], block->len - 2);
+
+	return spd_crc == crc;
+}
+
+/* Check if given block is 'reserved' for a given module type */
+static bool block_exists(spd_block_type type, u8 dimm_type)
+{
+	bool is_hybrid;
+
+	switch (type) {
+	case BLOCK_0: /* fall-through */
+	case BLOCK_1: /* fall-through */
+	case BLOCK_1_L: /* fall-through */
+	case BLOCK_1_H: /* fall-through */
+	case BLOCK_2_H: /* fall-through */
+	case BLOCK_3: /* fall-through */
+		return true;
+	case BLOCK_2_L:
+		is_hybrid = (dimm_type >> 4) & ((1 << 3) - 1);
+		if (is_hybrid)
+			return true;
+		return false;
+	default: /* fall-through */
+		return false;
+	}
+}
+
+
 /**
  * \brief Decode the raw SPD data
  *
@@ -36,20 +96,51 @@
  * @return @ref spd_status enumerator
  *		SPD_STATUS_OK -- decoding was successful
  *		SPD_STATUS_INVALID -- invalid SPD or not a DDR4 SPD
+ *		SPD_STATUS_CRC_ERROR -- checksum mismatch
  */
 int spd_decode_ddr4(dimm_attr *dimm, spd_raw_data spd)
 {
    u8 reg8;
    u8 bus_width, sdram_width;
    u16 cap_per_die_mbit;
+	u16 spd_bytes_total, spd_bytes_used;
+	const uint16_t spd_bytes_used_table[] = {0, 128, 256, 384, 512};

-	/* Make sure that the SPD dump is indeed from a DDR3 module */
+	/* Make sure that the SPD dump is indeed from a DDR4 module */
    if (spd[2] != SPD_MEMORY_TYPE_DDR4_SDRAM) {
    	printk(BIOS_ERR, "Not a DDR4 SPD!\n");
    	dimm->dram_type = SPD_MEMORY_TYPE_UNDEFINED;
    	return SPD_STATUS_INVALID;
    }

+	spd_bytes_total = (spd[0] >> 4) & ((1 << 3) - 1);
+	spd_bytes_used = spd[0] & ((1 << 4) - 1);
+
+	if (!spd_bytes_total || !spd_bytes_used) {
+		printk(BIOS_ERR, "SPD failed basic sanity checks\n");
+		return SPD_STATUS_INVALID;
+	}
+
+	spd_bytes_total = 256 << (spd_bytes_total - 1);
+	spd_bytes_used = spd_bytes_used_table[spd_bytes_used];
+
+	/* Verify CRC of blocks that have them, do not step over 'used' length */
+	for (int i = 0; i < ARRAY_SIZE(spd_blocks); i++) {
+		/* this block is not checksumed */
+		if (spd_blocks[i].crc_start == 0)
+			continue;
+		/* we shouldn't have this block */
+		if (spd_blocks[i].start + spd_blocks[i].len > spd_bytes_used)
+			continue;
+		/* check if block exists in the current schema */
+		if (!block_exists(spd_blocks[i].type, spd[3]))
+			continue;
+		if (!verify_block(&spd_blocks[i], spd)) {
+			printk(BIOS_ERR, "CRC failed for block %d\n", i);
+			return SPD_STATUS_CRC_ERROR;
+		}
+	}
+
    dimm->dram_type = SPD_MEMORY_TYPE_DDR4_SDRAM;
    dimm->dimm_type = spd[3] & ((1 << 4) - 1);

@@ -74,17 +165,19 @@
    	return SPD_STATUS_INVALID;
    }

-	/* calculate size */
-	dimm->size_mb = cap_per_die_mbit / 8 * bus_width / sdram_width * dimm->ranks;
-
-	dimm->manufacturer_id = (spd[351] << 8) | spd[350];
-
    /* seems to be only one, in mV */
    dimm->vdd_voltage = 1200;

-	memcpy(dimm->part_number, &spd[329], SPD_DDR4_PART_LEN);
-	dimm->part_number[SPD_DDR4_PART_LEN] = 0;
-	memcpy(dimm->serial_number, &spd[325], sizeof(dimm->serial_number));
+	/* calculate size */
+	dimm->size_mb = cap_per_die_mbit / 8 * bus_width / sdram_width * dimm->ranks;
+
+	/* make sure we have the manufacturing information block */
+	if (spd_bytes_used > 320) {
+		dimm->manufacturer_id = (spd[351] << 8) | spd[350];
+		memcpy(dimm->part_number, &spd[329], SPD_DDR4_PART_LEN);
+		dimm->part_number[SPD_DDR4_PART_LEN] = 0;
+		memcpy(dimm->serial_number, &spd[325], sizeof(dimm->serial_number));
+	}
    return SPD_STATUS_OK;
 }

diff --git a/src/device/dram/ddr_common.c b/src/device/dram/ddr_common.c
new file mode 100644
index 0000000..bcc9a8f
--- /dev/null
+++ b/src/device/dram/ddr_common.c
@@ -0,0 +1,44 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright (C) 2019 Facebook, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <console/console.h>
+#include <device/dram/common.h>
+#include <types.h>
+
+/**
+ * \brief Calculate the CRC of a DDR SPD data
+ *
+ * @param spd pointer to raw SPD data
+ * @param len length of data in SPD
+ *
+ * @return the CRC of the SPD data
+ */
+u16 ddr_crc16(const u8 *ptr, int n_crc)
+{
+	int i;
+	u16 crc = 0;
+
+	while (--n_crc >= 0) {
+		crc = crc ^ ((int)*ptr++ << 8);
+		for (i = 0; i < 8; ++i)
+			if (crc & 0x8000) {
+				crc = (crc << 1) ^ 0x1021;
+			} else {
+				crc = crc << 1;
+			}
+	}
+
+	return crc;
+}
diff --git a/src/include/device/dram/common.h b/src/include/device/dram/common.h
index b1677c8..4c6f02e 100644
--- a/src/include/device/dram/common.h
+++ b/src/include/device/dram/common.h
@@ -67,6 +67,6 @@
    SPD_STATUS_INVALID_FIELD,
 };

-u16 ddr3_crc16(const u8 *ptr, int n_crc);
+u16 ddr_crc16(const u8 *ptr, int n_crc);

#endif /* DEVICE_DRAM_COMMON_H */

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34781 )

Change subject: dram: Add CRC16 checks for DDR4
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34781/1/src/device/dram/ddr4.c 
File src/device/dram/ddr4.c:

https://review.coreboot.org/c/coreboot/+/34781/1/src/device/dram/ddr4.c@48 
PS1, Line 48: 	{.type = BLOCK_3, 384, 128, 0}};
space required after that close brace '}'

https://review.coreboot.org/c/coreboot/+/34781/1/src/device/dram/ddr_common.... 
File src/device/dram/ddr_common.c:

https://review.coreboot.org/c/coreboot/+/34781/1/src/device/dram/ddr_common.... 
PS1, Line 36: 			if (crc & 0x8000) {
braces {} are not necessary for any arm of this statement

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34781 )

Change subject: dram: Add CRC16 checks for DDR4
......................................................................

Patch Set 1:

It does more than the commit message says. Please squash the DDR4 updates with "dram: Add basic DDR4 parsing" patch.

[Andrey Petrov (Code Review)]

Andrey Petrov has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34781 )

Change subject: dram: Add CRC16 checks for DDR4
......................................................................

Abandoned

squashing into earlier patch, by popular request