Change in coreboot[4.11_branch]: soc/fsp_broadwell_de: read SPD 3 times and pick the majority element

[David Hendricks (Code Review)]

Hello Andrey Petrov,

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

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

to review the following change.

Change subject: soc/fsp_broadwell_de: read SPD 3 times and pick the majority element
......................................................................

soc/fsp_broadwell_de: read SPD 3 times and pick the majority element

On the platform we consistently observe errneous reads of SPD data over
IMC's SMBus. Since DDR4 SPD does not include CRC checksum for
"Manufacturing Information" we can not determine serial/part numbers
reliable.

The root cause for SMBus errors is unknown.

The corruption happens in 1 byte only with 3-4 bits flipped.
Interestingly this happens after fixed amount of time, in case of
tight-loop reads about every 3 seconds with almost exactly same number
of CPU cycles, or multiples of that number.

The stop-gap solution is to read data 3 times and pick out the prevalent
member

TEST=tested on OCP monolake

Signed-off-by: Andrey Petrov 
Change-Id: If145cec2766f03412e33c98befb1d98b87306615
---
M src/soc/intel/fsp_broadwell_de/smbus-imc.c
1 file changed, 44 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/37858/1

diff --git a/src/soc/intel/fsp_broadwell_de/smbus-imc.c b/src/soc/intel/fsp_broadwell_de/smbus-imc.c
index 61dc080..f483aae 100644
--- a/src/soc/intel/fsp_broadwell_de/smbus-imc.c
+++ b/src/soc/intel/fsp_broadwell_de/smbus-imc.c
@@ -13,6 +13,7 @@
  * GNU General Public License for more details.
  */

+#include <console/console.h>
 #include <stddef.h>
 #include <device/pci_def.h>
 #include <device/early_smbus.h>
@@ -20,6 +21,33 @@
 #include <soc/pci_devs.h>
 #include <spd.h>

+#define VOTE_ELEMENTS		3
+
+static bool vote(uint16_t *arr, uint8_t n, uint8_t *res_idx)
+{
+	int maxcount = 0;
+	int index = -1;
+
+	for(int i = 0; i < n; i++) {
+		int count = 0;
+		for(int j = 0; j < n; j++) {
+			if(arr[i] == arr[j])
+			count++;
+		}
+		if(count > maxcount) {
+			maxcount = count;
+			index = i;
+		}
+	}
+
+	if (maxcount > n/2) {
+		*res_idx = index;
+		return true;
+	}
+
+	return false;
+}
+
 /* read word, return value on success */
 uint16_t smbus_read_word(u32 smbus_dev, u8 addr, u8 offset)
 {
@@ -36,13 +64,24 @@
 /* read byte, return value on success */
 uint8_t smbus_read_byte(u32 smbus_dev, u8 addr, u8 offset)
 {
-	uint16_t res = 0;
+	uint16_t res[VOTE_ELEMENTS] = {0};
+	uint8_t idx = 0;

-	if (imc_smbus_spd_xfer(IMC_DEV, addr, offset, IMC_DEVICE_EEPROM, IMC_DATA_BYTE,
-			       IMC_CONTROLLER_ID0, IMC_READ, &res)
-	    == 0) {
-		return res;
+	for (int i = 0; i < ARRAY_SIZE(res); i++) {
+		if (imc_smbus_spd_xfer(IMC_DEV,
+					addr, offset, IMC_DEVICE_EEPROM, IMC_DATA_BYTE,
+					IMC_CONTROLLER_ID0, IMC_READ, &res[i]) != 0) {
+			return 0;
+		}
    }
+
+	/* vote for majority element, pick 1 out of 3 */
+	if (vote(res, ARRAY_SIZE(res), &idx)) {
+		return res[idx];
+	}
+
+	printk(BIOS_ALERT, "IMC: read failure, couldn't vote for correct byte\n");
+
    return 0;
 }

[build bot (Jenkins) (Code Review)]

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

Change subject: soc/fsp_broadwell_de: read SPD 3 times and pick the majority element
......................................................................

Patch Set 1:

(6 comments)

https://review.coreboot.org/c/coreboot/+/37858/1/src/soc/intel/fsp_broadwell... 
File src/soc/intel/fsp_broadwell_de/smbus-imc.c:

https://review.coreboot.org/c/coreboot/+/37858/1/src/soc/intel/fsp_broadwell... 
PS1, Line 31: 	for(int i = 0; i < n; i++) {
space required before the open parenthesis '('

https://review.coreboot.org/c/coreboot/+/37858/1/src/soc/intel/fsp_broadwell... 
PS1, Line 33: 		for(int j = 0; j < n; j++) {
space required before the open parenthesis '('

https://review.coreboot.org/c/coreboot/+/37858/1/src/soc/intel/fsp_broadwell... 
PS1, Line 34: 			if(arr[i] == arr[j])
suspect code indent for conditional statements (24, 24)

https://review.coreboot.org/c/coreboot/+/37858/1/src/soc/intel/fsp_broadwell... 
PS1, Line 34: 			if(arr[i] == arr[j])
space required before the open parenthesis '('

https://review.coreboot.org/c/coreboot/+/37858/1/src/soc/intel/fsp_broadwell... 
PS1, Line 37: 		if(count > maxcount) {
space required before the open parenthesis '('

https://review.coreboot.org/c/coreboot/+/37858/1/src/soc/intel/fsp_broadwell... 
PS1, Line 79: 	if (vote(res, ARRAY_SIZE(res), &idx)) {
braces {} are not necessary for single statement blocks

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37858 )

Change subject: soc/fsp_broadwell_de: read SPD 3 times and pick the majority element
......................................................................

Patch Set 1: Code-Review-2

We're not certain if it's a problem with the SoC or the mainboard, src/mainboard/ might be a more appropriate place for this.

[S] Change in coreboot[4.11_branch]: soc/fsp_broadwell_de: read SPD 3 times and pick the majority element

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37858?usp=email )

Change subject: soc/fsp_broadwell_de: read SPD 3 times and pick the majority element
......................................................................

Abandoned