[coreboot-gerrit] Change in coreboot[master]: [RFC] lib/spd_bin: Redesign API

Nico Huber (Code Review) gerrit at coreboot.org
Mon Nov 5 18:35:55 CET 2018


Nico Huber has uploaded this change for review. ( https://review.coreboot.org/29481


Change subject: [RFC] lib/spd_bin: Redesign API
......................................................................

[RFC] lib/spd_bin: Redesign API

About every two years, somebody reinvents the wheel wrt. SPD handling.
Let's break the cycle by providing an API that is simple enough to be
used in mainboard code and handles all common patterns (e.g. reading
SPDs from CBFS or SMBus).

This is not complete yet, but I wanted to get feedback as early as
possible. The rough idea is:

  o mainboard code provides a struct with information about where to
    fetch the SPD from, *per module* (e.g. CBFS, SMBus, memory address)
  o common code gathers the SPDs and normalizes the struct, so all
    entries will eventually be memory addresses

The current version uses a simple array to represent the modules. This
doesn't reflect the actual topology of memory channels, which is in-
stead implied by the consumer of the normalized struct, atm.

This could be extended later, for instance, with a common SPD cache
that could be stored along the MRC cache data.

Note that for the latest incarnation, the SPD code of intel/cannonlake,
this removes the option to let FSP load the SPD from SMBus. It didn't
seem useful, though, as it removes all flexibility. And isn't FSP's
responsibility anyway.

Change-Id: Iac300a374e5cd56a9d202cf24ad8fe8367b780d8
Signed-off-by: Nico Huber <nico.huber at secunet.com>
---
M src/Kconfig
M src/include/spd_bin.h
M src/lib/spd_bin.c
M src/mainboard/google/fizz/Kconfig
M src/mainboard/google/fizz/romstage.c
M src/mainboard/google/kahlee/romstage.c
M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
M src/mainboard/google/kahlee/variants/baseboard/memory.c
M src/mainboard/google/zoombini/romstage.c
M src/mainboard/intel/cannonlake_rvp/romstage_fsp_params.c
M src/mainboard/intel/coffeelake_rvp/Kconfig
M src/mainboard/intel/coffeelake_rvp/romstage.c
M src/mainboard/intel/kblrvp/Kconfig
M src/mainboard/intel/kblrvp/romstage.c
M src/mainboard/intel/saddlebrook/Kconfig
M src/mainboard/intel/saddlebrook/romstage.c
M src/mainboard/kontron/bsl6/Kconfig
M src/mainboard/kontron/bsl6/romstage.c
M src/mainboard/purism/librem_skl/Kconfig
M src/mainboard/purism/librem_skl/romstage.c
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
23 files changed, 287 insertions(+), 248 deletions(-)



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

diff --git a/src/Kconfig b/src/Kconfig
index 2777d26..2938246 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -1211,6 +1211,11 @@
 	  Total SPD size that will be used for DIMM.
 	  Ex: DDR3 256, DDR4 512.
 
+config DIMM_SPD_SMBUS
+	bool
+	help
+	  Support fetching SPDs from SMBus.
+
 config SPD_READ_BY_WORD
 	bool
 
diff --git a/src/include/spd_bin.h b/src/include/spd_bin.h
index e0a50ff..1f20e8b 100644
--- a/src/include/spd_bin.h
+++ b/src/include/spd_bin.h
@@ -16,8 +16,8 @@
 #ifndef SPD_BIN_H
 #define SPD_BIN_H
 
-#include <arch/early_variables.h>
 #include <stdint.h>
+#include <stdlib.h>
 #include <commonlib/region.h>
 
 #define SPD_PAGE_LEN		256
@@ -42,18 +42,59 @@
 #define LPDDR4_SPD_PART_OFF	329
 #define LPDDR4_SPD_PART_LEN	20
 
-struct spd_block {
-	u8 addr_map[CONFIG_DIMM_MAX]; /* 7 bit I2C addresses */
-	u8 *spd_array[CONFIG_DIMM_MAX];
-	/* Length of each dimm */
-	u16 len;
+enum spd_bin_src {
+	SPD_BIN_NONE = 0,
+	SPD_BIN_SMBUS,	/* read SPD from SMBus */
+	SPD_BIN_CBFS,	/* read SPD from CBFS file `spd.bin` containing
+			   fixed entries of CONFIG_DIMM_SPD_SIZE bytes */
+	SPD_BIN_COPY,	/* copy SPD pointer from another entry */
+	SPD_BIN_PTR,	/* read SPD from memory address */
 };
 
-void print_spd_info(uint8_t spd[]);
-/* Return 0 on success & -1 on failure */
-int get_spd_cbfs_rdev(struct region_device *spd_rdev, u8 spd_index);
-void dump_spd_info(struct spd_block *blk);
-void get_spd_smbus(struct spd_block *blk);
+struct spd_bin_dimm {
+	enum spd_bin_src src;
+	union {
+		u8 smbus_addr;	/* 7 bit I2C address */
+		u8 index;	/* for SPD_BIN_CPFS or SPD_BIN_COPY */
+		const u8 *spd;
+	};
+	size_t len;
+};
+
+struct spd_binfo {
+	struct spd_bin_dimm spds[CONFIG_DIMM_MAX];
+};
+
+#define SPD_BINFO_CBFS(idx) \
+	{ .spds = { { .src = SPD_BIN_CBFS, .index = idx, }, }, }
+#define SPD_BINFO_SMBUS(a1) \
+	{ .spds = { { .src = SPD_BIN_SMBUS, .smbus_addr = a1, }, }, }
+#define SPD_BINFO_SMBUS2(a1, a2) \
+	{ .spds = { { .src = SPD_BIN_SMBUS, .smbus_addr = a1, }, \
+		    { .src = SPD_BIN_SMBUS, .smbus_addr = a2, }, }, }
+#define SPD_BINFO_SMBUS4(a1, a2, a3, a4) \
+	{ .spds = { { .src = SPD_BIN_SMBUS, .smbus_addr = a1, }, \
+		    { .src = SPD_BIN_SMBUS, .smbus_addr = a2, }, \
+		    { .src = SPD_BIN_SMBUS, .smbus_addr = a3, }, \
+		    { .src = SPD_BIN_SMBUS, .smbus_addr = a4, }, }, }
+
+/*
+ * Read SPDs from SMBus and CBFS if requested. If successful, all
+ * active entries should have SPD_BIN_PTR as `src` type and a valid
+ * `ptr` and `len`.
+ *
+ * Returns
+ *   a common length if any SPD is present and *all* present
+ *     SPDs have the same length,
+ *   * 0 if multiple SPDs are present with different lengths,
+ *   * -1 if no valid SPD could be found.
+ */
+ssize_t gather_spds(struct spd_binfo *);
+
+void dump_spd_info(const struct spd_binfo *);
+
+
+/* FIXME: conflicting API below  -------------------------------------------  */
 
 /* expects SPD size to be 128 bytes, reads from "spd.bin" in CBFS and
    verifies the checksum. Only available if CONFIG_DIMM_SPD_SIZE == 128. */
diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c
index 79bda1e..4277ea9 100644
--- a/src/lib/spd_bin.c
+++ b/src/lib/spd_bin.c
@@ -14,27 +14,20 @@
  */
 
 #include <arch/byteorder.h>
+#include <arch/early_variables.h>
 #include <cbfs.h>
+#include <commonlib/region.h>
 #include <console/console.h>
 #include <spd_bin.h>
+#include <stdint.h>
+#include <stdlib.h>
 #include <string.h>
 #include <device/early_smbus.h>
 #include <device/dram/ddr3.h>
 
 static u8 spd_data[CONFIG_DIMM_MAX * CONFIG_DIMM_SPD_SIZE] CAR_GLOBAL;
 
-void dump_spd_info(struct spd_block *blk)
-{
-	u8 i;
-
-	for (i = 0; i < CONFIG_DIMM_MAX; i++)
-		if (blk->spd_array[i] != NULL && blk->spd_array[i][0] != 0) {
-			printk(BIOS_DEBUG, "SPD @ 0x%02X\n", blk->addr_map[i]);
-			print_spd_info(blk->spd_array[i]);
-		}
-}
-
-void print_spd_info(uint8_t spd[])
+static void print_spd_info(const uint8_t spd[])
 {
 	const int spd_banks[8] = { 8, 16, 32, 64, -1, -1, -1, -1 };
 	const int spd_capmb[8] = { 1,  2,  4,  8, 16, 32, 64, 128 };
@@ -98,31 +91,17 @@
 	}
 }
 
-static void update_spd_len(struct spd_block *blk)
+void dump_spd_info(const struct spd_binfo *const info)
 {
-	u8 i, j = 0;
-	for (i = 0 ; i < CONFIG_DIMM_MAX; i++)
-		if (blk->spd_array[i] != NULL)
-			j |= blk->spd_array[i][SPD_DRAM_TYPE];
+	int i;
 
-	/* If spd used is DDR4, then its length is 512 byte. */
-	if (j == SPD_DRAM_DDR4)
-		blk->len = SPD_PAGE_LEN_DDR4;
-	else
-		blk->len = SPD_PAGE_LEN;
-}
-
-int get_spd_cbfs_rdev(struct region_device *spd_rdev, u8 spd_index)
-{
-	struct cbfsf fh;
-
-	uint32_t cbfs_type = CBFS_TYPE_SPD;
-
-	if (cbfs_boot_locate(&fh, "spd.bin", &cbfs_type) < 0)
-		return -1;
-	cbfs_file_data(spd_rdev, &fh);
-	return rdev_chain(spd_rdev, spd_rdev, spd_index * CONFIG_DIMM_SPD_SIZE,
-							CONFIG_DIMM_SPD_SIZE);
+	for (i = 0; i < CONFIG_DIMM_MAX; i++) {
+		if (info->spds[i].src == SPD_BIN_PTR &&
+		    info->spds[i].spd != NULL && info->spds[i].spd[0] != 0) {
+			printk(BIOS_DEBUG, "SPD #%d\n", i);
+			print_spd_info(info->spds[i].spd);
+		}
+	}
 }
 
 static void smbus_read_spd(u8 *spd, u8 addr)
@@ -142,14 +121,14 @@
 	}
 }
 
-static void get_spd(u8 *spd, u8 addr)
+static ssize_t get_spd_smbus(u8 *const spd, const size_t len, const u8 addr)
 {
+	if (!IS_ENABLED(CONFIG_DIMM_SPD_SMBUS))
+		return -1;
+
 	if (smbus_read_byte(0, addr, 0) == 0xff) {
-		printk(BIOS_INFO, "No memory dimm at address %02X\n",
-			addr << 1);
-		/* Make sure spd is zeroed if dimm doesn't exist. */
-		memset(spd, 0, CONFIG_DIMM_SPD_SIZE);
-		return;
+		printk(BIOS_INFO, "No SPD at address %02x.\n", addr);
+		return -1;
 	}
 	smbus_read_spd(spd, addr);
 
@@ -162,20 +141,98 @@
 		/* Restore to page 0 */
 		smbus_write_byte(0, SPD_PAGE_0, 0, 0);
 	}
+
+	/* FIXME: Actually match read length. */
+	return len;
 }
 
-void get_spd_smbus(struct spd_block *blk)
+static ssize_t get_spd_cbfs(u8 *const spd, const size_t len, const u8 index)
 {
-	u8 i;
-	unsigned char *spd_data_ptr = car_get_var_ptr(&spd_data);
+	uint32_t cbfs_type = CBFS_TYPE_SPD;
+	struct region_device rdev;
+	struct cbfsf fh;
 
-	for (i = 0 ; i < CONFIG_DIMM_MAX; i++) {
-		get_spd(spd_data_ptr + i * CONFIG_DIMM_SPD_SIZE,
-			blk->addr_map[i]);
-		blk->spd_array[i] = spd_data_ptr + i * CONFIG_DIMM_SPD_SIZE;
+	if (cbfs_boot_locate(&fh, "spd.bin", &cbfs_type) < 0)
+		return -1;
+
+	cbfs_file_data(&rdev, &fh);
+	return rdev_readat(&rdev, spd, index * len, len);
+}
+
+static ssize_t get_spd_copy(u8 **const ptr, const struct spd_binfo *const info,
+			    const int i, const size_t len)
+{
+	const u8 src_index = info->spds[i].index;
+
+	if (src_index > i) {
+		printk(BIOS_WARNING, "Can only copy preceding SPD entries.\n");
+		return -1;
 	}
 
-	update_spd_len(blk);
+	if (info->spds[src_index].src != SPD_BIN_PTR) {
+		printk(BIOS_WARNING, "Can only copy PTR SPD entries.\n");
+		return -1;
+	}
+
+	*ptr = (u8 *)info->spds[src_index].spd;
+	return info->spds[src_index].len;
+}
+
+ssize_t gather_spds(struct spd_binfo *const info)
+{
+	const size_t len = CONFIG_DIMM_SPD_SIZE;
+	u8 *const spd_buffer = car_get_var_ptr(&spd_data);
+	ssize_t common_len = -1;
+	int i;
+
+	for (i = 0; i < CONFIG_DIMM_MAX; ++i) {
+		ssize_t ret = 0;
+		u8 *ptr = NULL;
+
+		switch (info->spds[i].src) {
+		case SPD_BIN_NONE:
+			break;
+		case SPD_BIN_SMBUS:
+			ptr = spd_buffer + i * len;
+			ret = get_spd_smbus(ptr, len, info->spds[i].smbus_addr);
+			break;
+		case SPD_BIN_CBFS:
+			ptr = spd_buffer + i * len;
+			ret = get_spd_cbfs(ptr, len, info->spds[i].index);
+			break;
+		case SPD_BIN_COPY:
+			ret = get_spd_copy(&ptr, info, i, len);
+			break;
+		case SPD_BIN_PTR:
+			ptr = (u8 *)info->spds[i].spd;
+			ret = info->spds[i].len;
+			break;
+		default:
+			ret = -1;
+			break;
+		}
+
+		if (ret < 0)
+			printk(BIOS_WARNING, "Failed to fetch SPD #%d.\n", i);
+		else if (ret < len && info->spds[i].src != SPD_BIN_NONE)
+			printk(BIOS_WARNING, "Short SPD read for #%d.\n", i);
+
+		info->spds[i].spd = ptr;
+		if (ret <= 0) {
+			info->spds[i].src = SPD_BIN_NONE;
+			info->spds[i].len = 0;
+		} else {
+			info->spds[i].src = SPD_BIN_PTR;
+			info->spds[i].len = ret;
+			if (common_len == -1) {
+				common_len = ret;
+			} else if (common_len != ret) {
+				common_len = 0;
+			}
+		}
+	}
+
+	return common_len;
 }
 
 #if CONFIG_DIMM_SPD_SIZE == 128
diff --git a/src/mainboard/google/fizz/Kconfig b/src/mainboard/google/fizz/Kconfig
index 1763f19..7feb6ab 100644
--- a/src/mainboard/google/fizz/Kconfig
+++ b/src/mainboard/google/fizz/Kconfig
@@ -2,6 +2,7 @@
 config BOARD_GOOGLE_BASEBOARD_FIZZ
 	def_bool n
 	select BOARD_ROMSIZE_KB_16384
+	select DIMM_SPD_SMBUS
 	select DRIVERS_GENERIC_MAX98357A if BOARD_GOOGLE_KALISTA
 	select DRIVERS_I2C_DA7219 if BOARD_GOOGLE_KALISTA
 	select DRIVERS_I2C_GENERIC
diff --git a/src/mainboard/google/fizz/romstage.c b/src/mainboard/google/fizz/romstage.c
index 335662e..8c8b144 100644
--- a/src/mainboard/google/fizz/romstage.c
+++ b/src/mainboard/google/fizz/romstage.c
@@ -13,7 +13,10 @@
  * GNU General Public License for more details.
  */
 
+#include <console/console.h>
 #include <soc/romstage.h>
+#include <stddef.h>
+#include <stdint.h>
 #include <string.h>
 #include <spd_bin.h>
 
@@ -36,14 +39,18 @@
 	memcpy(&mem_cfg->RcompTarget, rcomp_target, sizeof(rcomp_target));
 
 	/* Read spd block to get memory config */
-	struct spd_block blk = {
-		.addr_map = { 0x50, 0x52, },
-	};
-	mem_cfg->DqPinsInterleaved = 1;
-	get_spd_smbus(&blk);
-	mem_cfg->MemorySpdDataLen = blk.len;
-	mem_cfg->MemorySpdPtr00 = (uintptr_t)blk.spd_array[0];
-	mem_cfg->MemorySpdPtr10 = (uintptr_t)blk.spd_array[1];
+	struct spd_binfo spd_info = { .spds = {
+		{ .src = SPD_BIN_SMBUS, .smbus_addr = 0x50, },
+		{ .src = SPD_BIN_SMBUS, .smbus_addr = 0x52, },
+	} };
+	const ssize_t common_len = gather_spds(&spd_info);
+	if (common_len <= 0)
+		die("Failed to read any SPD from SMBus.\n");
 
-	dump_spd_info(&blk);
+	mem_cfg->DqPinsInterleaved = 1;
+	mem_cfg->MemorySpdDataLen = common_len;
+	mem_cfg->MemorySpdPtr00 = (uintptr_t)spd_info.spds[0].spd;
+	mem_cfg->MemorySpdPtr10 = (uintptr_t)spd_info.spds[1].spd;
+
+	dump_spd_info(&spd_info);
 }
diff --git a/src/mainboard/google/kahlee/romstage.c b/src/mainboard/google/kahlee/romstage.c
index 5ec5c25..ca0e7c8 100644
--- a/src/mainboard/google/kahlee/romstage.c
+++ b/src/mainboard/google/kahlee/romstage.c
@@ -15,11 +15,30 @@
 
 #include <amdblocks/dimm_spd.h>
 #include <baseboard/variants.h>
+#include <console/console.h>
 #include <soc/romstage.h>
+#include <spd_bin.h>
+#include <stddef.h>
+#include <string.h>
 
 int mainboard_read_spd(uint8_t spdAddress, char *buf, size_t len)
 {
-	return variant_mainboard_read_spd(spdAddress, buf, len);
+	const u8 spd_index = variant_memory_sku();
+	struct spd_binfo spd_info = SPD_BINFO_CBFS(spd_index);
+
+	const ssize_t common_len = gather_spds(&spd_info);
+	if (common_len <= 0)
+		printk(BIOS_ERR, "Error: Failed to read SPD from CBFS.\n");
+		return -1;
+	}
+
+	if (len != common_len) {
+		printk(BIOS_ERR, "Error: Read SPD of incorrect length.\n");
+		return -1;
+	}
+
+	memcpy(buf, spd_info.spds[0].spd, len);
+	return 0;
 }
 
 void __weak variant_romstage_entry(int s3_resume)
diff --git a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
index 6e89105..d29cea3 100644
--- a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
+++ b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
@@ -24,7 +24,6 @@
 uint8_t variant_memory_sku(void);
 /* Return board SKU. Limited to uint8_t, so it fits into 3 decimal digits */
 uint8_t variant_board_sku(void);
-int variant_mainboard_read_spd(uint8_t spdAddress, char *buf, size_t len);
 int variant_get_xhci_oc_map(uint16_t *usb_oc_map);
 int variant_get_ehci_oc_map(uint16_t *usb_oc_map);
 const struct soc_amd_gpio *variant_early_gpio_table(size_t *size);
diff --git a/src/mainboard/google/kahlee/variants/baseboard/memory.c b/src/mainboard/google/kahlee/variants/baseboard/memory.c
index 67402fb..d7f88d6 100644
--- a/src/mainboard/google/kahlee/variants/baseboard/memory.c
+++ b/src/mainboard/google/kahlee/variants/baseboard/memory.c
@@ -14,11 +14,8 @@
  */
 
 #include <baseboard/variants.h>
-#include <console/console.h>
 #include <gpio.h> /* src/include/gpio.h */
-#include <spd_bin.h>
 #include <variant/gpio.h>
-#include <amdblocks/dimm_spd.h>
 
 uint8_t __weak variant_memory_sku(void)
 {
@@ -31,29 +28,3 @@
 
 	return gpio_base2_value(pads, ARRAY_SIZE(pads));
 }
-
-int __weak variant_mainboard_read_spd(uint8_t spdAddress,
-							char *buf, size_t len)
-{
-	struct region_device spd_rdev;
-	u8 spd_index = variant_memory_sku();
-
-	printk(BIOS_INFO, "%s SPD index %d\n", __func__, spd_index);
-
-	if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) {
-		printk(BIOS_ERR, "Error: spd.bin not found\n");
-		return -1;
-	}
-
-	if (len != region_device_sz(&spd_rdev)) {
-		printk(BIOS_ERR, "Error: spd.bin is not the correct size\n");
-		return -1;
-	}
-
-	if (rdev_readat(&spd_rdev, buf, 0, len) != len) {
-		printk(BIOS_ERR, "Error: couldn't read spd.bin\n");
-		return -1;
-	}
-
-	return 0;
-}
diff --git a/src/mainboard/google/zoombini/romstage.c b/src/mainboard/google/zoombini/romstage.c
index 4bd0ede..c07cfd9 100644
--- a/src/mainboard/google/zoombini/romstage.c
+++ b/src/mainboard/google/zoombini/romstage.c
@@ -16,14 +16,16 @@
 #include <baseboard/variants.h>
 #include <soc/cnl_memcfg_init.h>
 #include <soc/romstage.h>
+#include <spd_bin.h>
 
 void mainboard_memory_init_params(FSPM_UPD *memupd)
 {
-	const struct spd_info spd = {
-		.spd_by_index = true,
-		.spd_spec.spd_index = variant_memory_sku(),
-	};
+	struct spd_binfo spd_info = { .spds = {
+		{ .src = SPD_BIN_CBFS, .index = variant_memory_sku(), },
+		{ .src = SPD_BIN_NONE, },
+		{ .src = SPD_BIN_COPY, .index = 0, },
+	}, };
 
 	cannonlake_memcfg_init(&memupd->FspmConfig,
-				variant_lpddr4_config(), &spd);
+				variant_lpddr4_config(), &spd_info);
 }
diff --git a/src/mainboard/intel/cannonlake_rvp/romstage_fsp_params.c b/src/mainboard/intel/cannonlake_rvp/romstage_fsp_params.c
index 457be7f..642cef6 100644
--- a/src/mainboard/intel/cannonlake_rvp/romstage_fsp_params.c
+++ b/src/mainboard/intel/cannonlake_rvp/romstage_fsp_params.c
@@ -14,11 +14,12 @@
  */
 
 #include <arch/byteorder.h>
-#include <cbfs.h>
 #include <console/console.h>
 #include <fsp/api.h>
 #include <soc/romstage.h>
 #include "spd/spd.h"
+#include <stddef.h>
+#include <stdint.h>
 #include <string.h>
 #include <spd_bin.h>
 
@@ -40,14 +41,13 @@
 	mem_cfg->ECT = 1; /* Early Command Training Enabled */
 	spd_index = 2;
 
-	struct region_device spd_rdev;
+	struct spd_binfo spd_info = SPD_BINFO_CBFS(spd_index);
+	const ssize_t common_len = gather_spds(&spd_info);
+	if (common_len <= 0)
+		die("Failed to read SPD from CBFS.\n");
 
-	if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0)
-		die("spd.bin not found\n");
-
-	mem_cfg->MemorySpdDataLen = region_device_sz(&spd_rdev);
-	/* Memory leak is ok since we have memory mapped boot media */
-	mem_cfg->MemorySpdPtr00 = (uintptr_t)rdev_mmap_full(&spd_rdev);
+	mem_cfg->MemorySpdDataLen = common_len;
+	mem_cfg->MemorySpdPtr00 = (uintptr_t)spd_info.spds[0].spd;
 	mem_cfg->RefClk = 0; /* Auto Select CLK freq */
 	mem_cfg->MemorySpdPtr10 = mem_cfg->MemorySpdPtr00;
 }
diff --git a/src/mainboard/intel/coffeelake_rvp/Kconfig b/src/mainboard/intel/coffeelake_rvp/Kconfig
index f69bb71..408c7c6 100644
--- a/src/mainboard/intel/coffeelake_rvp/Kconfig
+++ b/src/mainboard/intel/coffeelake_rvp/Kconfig
@@ -4,17 +4,16 @@
 	def_bool y
 	select BOARD_ROMSIZE_KB_16384 if !BOARD_INTEL_COFFEELAKE_RVPU
 	select BOARD_ROMSIZE_KB_32768 if BOARD_INTEL_COFFEELAKE_RVPU
-	select GENERIC_SPD_BIN
 	select HAVE_ACPI_RESUME
 	select HAVE_ACPI_TABLES
 	select MAINBOARD_HAS_CHROMEOS
-	select GENERIC_SPD_BIN
 	select DRIVERS_I2C_HID
 	select DRIVERS_I2C_GENERIC
 	select SOC_INTEL_COFFEELAKE
 	select SOC_INTEL_CANNONLAKE_MEMCFG_INIT
 	select SOC_INTEL_CANNONLAKE_PCH_H if BOARD_INTEL_COFFEELAKE_RVP11 || BOARD_INTEL_COFFEELAKE_RVP8
 	select SOC_INTEL_COMMON_BLOCK_HDA_VERB if BOARD_INTEL_COFFEELAKE_RVP11 || BOARD_INTEL_COFFEELAKE_RVP8
+	select DIMM_SPD_SMBUS
 
 config MAINBOARD_DIR
 	string
diff --git a/src/mainboard/intel/coffeelake_rvp/romstage.c b/src/mainboard/intel/coffeelake_rvp/romstage.c
index 1ab2d78..1e8c153 100644
--- a/src/mainboard/intel/coffeelake_rvp/romstage.c
+++ b/src/mainboard/intel/coffeelake_rvp/romstage.c
@@ -17,16 +17,11 @@
 #include <baseboard/variants.h>
 #include <soc/cnl_memcfg_init.h>
 #include <soc/romstage.h>
+#include <spd_bin.h>
 
 void mainboard_memory_init_params(FSPM_UPD *memupd)
 {
-	const struct spd_info spd = {
-		.spd_smbus_address[0] = 0xA0,
-		.spd_smbus_address[1] = 0xA2,
-		.spd_smbus_address[2] = 0xA4,
-		.spd_smbus_address[3] = 0xA6,
-	};
-
+	struct spd_binfo spd_info = SPD_BINFO_SMBUS4(0x50, 0x51, 0x52, 0x53);
 	cannonlake_memcfg_init(&memupd->FspmConfig,
-				variant_memcfg_config(), &spd);
+				variant_memcfg_config(), &spd_info);
 }
diff --git a/src/mainboard/intel/kblrvp/Kconfig b/src/mainboard/intel/kblrvp/Kconfig
index bf17c4b..6ece334 100644
--- a/src/mainboard/intel/kblrvp/Kconfig
+++ b/src/mainboard/intel/kblrvp/Kconfig
@@ -3,6 +3,7 @@
 config BOARD_SPECIFIC_OPTIONS # dummy
 	def_bool y
 	select BOARD_ROMSIZE_KB_16384
+	select DIMM_SPD_SMBUS if BOARD_INTEL_KBLRVP7 || BOARD_INTEL_KBLRVP8
 	select EC_ACPI
 	select HAVE_ACPI_RESUME
 	select HAVE_ACPI_TABLES
diff --git a/src/mainboard/intel/kblrvp/romstage.c b/src/mainboard/intel/kblrvp/romstage.c
index a29a4af..79ab910 100644
--- a/src/mainboard/intel/kblrvp/romstage.c
+++ b/src/mainboard/intel/kblrvp/romstage.c
@@ -14,7 +14,6 @@
  */
 
 #include <arch/byteorder.h>
-#include <cbfs.h>
 #include <console/console.h>
 #include <fsp/api.h>
 #include <gpio.h>
@@ -22,11 +21,12 @@
 #include <soc/romstage.h>
 #include <soc/gpio.h>
 #include "spd/spd.h"
+#include <stddef.h>
+#include <stdint.h>
 #include <string.h>
 #include <spd_bin.h>
 #include "board_id.h"
 
-
 void mainboard_memory_init_params(FSPM_UPD *mupd)
 {
 	FSP_M_CONFIG *mem_cfg;
@@ -41,29 +41,31 @@
 	mainboard_fill_rcomp_strength_data(&mem_cfg->RcompTarget);
 
 	if (IS_ENABLED(CONFIG_BOARD_INTEL_KBLRVP3)) {
-		struct region_device spd_rdev;
+		struct spd_binfo spd_info = SPD_BINFO_CBFS(spd_index);
 
+		const ssize_t common_len = gather_spds(&spd_info);
+		if (common_len <= 0)
+			die("Failed to read SPD from CBFS.\n");
 		mem_cfg->DqPinsInterleaved = 0;
-		if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0)
-			die("spd.bin not found\n");
-		mem_cfg->MemorySpdDataLen = region_device_sz(&spd_rdev);
-		/* Memory leak is ok since we have memory mapped boot media */
-		mem_cfg->MemorySpdPtr00 = (uintptr_t)rdev_mmap_full(&spd_rdev);
+		mem_cfg->MemorySpdDataLen = common_len;
+		mem_cfg->MemorySpdPtr00 = (uintptr_t)spd_info.spds[0].spd;
 	} else { /* CONFIG_BOARD_INTEL_KBLRVP7 and CONFIG_BOARD_INTEL_KBLRVP8 */
-		struct spd_block blk = {
-			.addr_map = { 0x50, 0x51, 0x52, 0x53, },
-		};
+		struct spd_binfo spd_info =
+			SPD_BINFO_SMBUS4(0x50, 0x51, 0x52, 0x53);
 
+		const ssize_t common_len = gather_spds(&spd_info);
+		if (common_len <= 0)
+			die("Failed to read valid SPD(s) from SMBus.\n");
 		mem_cfg->DqPinsInterleaved = 1;
-		get_spd_smbus(&blk);
-		mem_cfg->MemorySpdDataLen = blk.len;
-		mem_cfg->MemorySpdPtr00 = (uintptr_t)blk.spd_array[0];
-		mem_cfg->MemorySpdPtr10 = (uintptr_t)blk.spd_array[2];
+		mem_cfg->MemorySpdDataLen = common_len;
+		mem_cfg->MemorySpdPtr00 = (uintptr_t)spd_info.spds[0].spd;
+		mem_cfg->MemorySpdPtr10 = (uintptr_t)spd_info.spds[2].spd;
 		if (IS_ENABLED(CONFIG_BOARD_INTEL_KBLRVP8)) {
-			mem_cfg->MemorySpdPtr01 = (uintptr_t)blk.spd_array[1];
-			mem_cfg->MemorySpdPtr11 = (uintptr_t)blk.spd_array[3];
+			mem_cfg->MemorySpdPtr01 =
+				(uintptr_t)spd_info.spds[1].spd;
+			mem_cfg->MemorySpdPtr11 =
+				(uintptr_t)spd_info.spds[3].spd;
 		}
-
 	}
 	mupd->FspmTestConfig.DmiVc1 = 1;
 	if (IS_ENABLED(CONFIG_BOARD_INTEL_KBLRVP8))
diff --git a/src/mainboard/intel/saddlebrook/Kconfig b/src/mainboard/intel/saddlebrook/Kconfig
index 8b8a789..5408f35 100644
--- a/src/mainboard/intel/saddlebrook/Kconfig
+++ b/src/mainboard/intel/saddlebrook/Kconfig
@@ -19,6 +19,7 @@
 	def_bool y
 	select BOARD_ROMSIZE_KB_4096
 	select CONSOLE_SERIAL
+	select DIMM_SPD_SMBUS
 	select DRIVERS_UART
 	select GENERIC_SPD_BIN
 	select HAVE_ACPI_RESUME
diff --git a/src/mainboard/intel/saddlebrook/romstage.c b/src/mainboard/intel/saddlebrook/romstage.c
index 49fa5e3..95abd61 100644
--- a/src/mainboard/intel/saddlebrook/romstage.c
+++ b/src/mainboard/intel/saddlebrook/romstage.c
@@ -26,6 +26,8 @@
 #include <soc/romstage.h>
 #include "spd/spd.h"
 #include <spd_bin.h>
+#include <stdint.h>
+#include <stddef.h>
 #include <superio/nuvoton/common/nuvoton.h>
 #include <superio/nuvoton/nct6776/nct6776.h>
 
@@ -50,16 +52,16 @@
 	struct romstage_params *params,
 	MEMORY_INIT_UPD *memory_params)
 {
-	struct spd_block blk = {
-		.addr_map = { 0x50, 0x52, },
-	};
+	struct spd_binfo spd_info = SPD_BINFO_SMBUS2(0x50, 0x52);
+	const ssize_t common_len = gather_spds(&spd_info);
+	if (common_len <= 0)
+		die("Failed to read valid SPD(s) from SMBus.\n");
+	dump_spd_info(&spd_info);
+	printk(BIOS_SPEW, "spd block length: 0x%08x\n", common_len);
 
-	get_spd_smbus(&blk);
-	dump_spd_info(&blk);
-	printk(BIOS_SPEW, "spd block length: 0x%08x\n", blk.len);
-
-	memory_params->MemorySpdPtr00 = (UINT32) blk.spd_array[0];
-	memory_params->MemorySpdPtr10 = (UINT32) blk.spd_array[2];
+	memory_params->MemorySpdDataLen = common_len;
+	memory_params->MemorySpdPtr00 = (uintptr_t)spd_info.spds[0].spd;
+	memory_params->MemorySpdPtr10 = (uintptr_t)spd_info.spds[1].spd;
 	printk(BIOS_SPEW, "0x%08x: SpdDataBuffer_0_0\n",
 		memory_params->MemorySpdPtr00);
 	printk(BIOS_SPEW, "0x%08x: SpdDataBuffer_1_0\n",
@@ -83,7 +85,5 @@
 	memcpy(memory_params->RcompTarget, params->pei_data->RcompTarget,
 			sizeof(params->pei_data->RcompTarget));
 
-	/* update spd length*/
-	memory_params->MemorySpdDataLen = blk.len;
 	memory_params->DqPinsInterleaved = TRUE;
 }
diff --git a/src/mainboard/kontron/bsl6/Kconfig b/src/mainboard/kontron/bsl6/Kconfig
index c7604a0..079dedd 100644
--- a/src/mainboard/kontron/bsl6/Kconfig
+++ b/src/mainboard/kontron/bsl6/Kconfig
@@ -18,6 +18,7 @@
 	select MAINBOARD_HAS_LIBGFXINIT
 	select DRIVERS_I2C_NCT7802Y
 	select DRIVERS_I2C_LM96000
+	select DIMM_SPD_SMBUS
 
 config MAINBOARD_DIR
 	string
diff --git a/src/mainboard/kontron/bsl6/romstage.c b/src/mainboard/kontron/bsl6/romstage.c
index efd3186..4c36f36 100644
--- a/src/mainboard/kontron/bsl6/romstage.c
+++ b/src/mainboard/kontron/bsl6/romstage.c
@@ -13,9 +13,11 @@
  * GNU General Public License for more details.
  */
 
+#include <stddef.h>
 #include <stdint.h>
 #include <string.h>
 #include <assert.h>
+#include <console/console.h>
 #include <spd_bin.h>
 #include <soc/romstage.h>
 #include <fsp/soc_binding.h>
@@ -37,17 +39,17 @@
 void mainboard_memory_init_params(FSPM_UPD *mupd)
 {
 	FSP_M_CONFIG *const memory_params = &mupd->FspmConfig;
-	struct spd_block blk = {
-		.addr_map = { 0x50, 0x52 },
-	};
+	struct spd_binfo spd_info = SPD_BINFO_SMBUS2(0x50, 0x52);
 
 	assert(sizeof(memory_params->RcompResistor) == sizeof(rcomp_resistors));
 	assert(sizeof(memory_params->RcompTarget) == sizeof(rcomp_targets));
 
-	memory_params->MemorySpdDataLen = CONFIG_DIMM_SPD_SIZE;
-	get_spd_smbus(&blk);
-	memory_params->MemorySpdPtr00 = (u32)blk.spd_array[0];
-	memory_params->MemorySpdPtr10 = (u32)blk.spd_array[1];
+	const ssize_t common_len = gather_spds(&spd_info);
+	if (common_len <= 0)
+		die("Failed to read valid SPD(s) from SMBus.\n");
+	memory_params->MemorySpdDataLen = common_len;
+	memory_params->MemorySpdPtr00 = (uintptr_t)spd_info.spds[0].spd;
+	memory_params->MemorySpdPtr10 = (uintptr_t)spd_info.spds[1].spd;
 
 	memcpy(memory_params->RcompResistor, rcomp_resistors,
 	       sizeof(memory_params->RcompResistor));
diff --git a/src/mainboard/purism/librem_skl/Kconfig b/src/mainboard/purism/librem_skl/Kconfig
index be4b7a3..7ad68dd 100644
--- a/src/mainboard/purism/librem_skl/Kconfig
+++ b/src/mainboard/purism/librem_skl/Kconfig
@@ -8,6 +8,7 @@
 	# Workaround for EC/KBC IRQ1
 	select SERIRQ_CONTINUOUS_MODE
 	select MAINBOARD_USES_FSP2_0
+	select DIMM_SPD_SMBUS
 	select SPD_READ_BY_WORD
 	select MAINBOARD_HAS_LPC_TPM
 
diff --git a/src/mainboard/purism/librem_skl/romstage.c b/src/mainboard/purism/librem_skl/romstage.c
index 48db885..a6800a7 100644
--- a/src/mainboard/purism/librem_skl/romstage.c
+++ b/src/mainboard/purism/librem_skl/romstage.c
@@ -16,8 +16,11 @@
  * GNU General Public License for more details.
  */
 
+#include <stddef.h>
+#include <stdint.h>
 #include <string.h>
 #include <assert.h>
+#include <console/console.h>
 #include <soc/romstage.h>
 #include <spd_bin.h>
 #include "pei_data.h"
@@ -25,15 +28,14 @@
 void mainboard_memory_init_params(FSPM_UPD *mupd)
 {
 	FSP_M_CONFIG *mem_cfg;
-	struct spd_block blk = {
-		.addr_map = { 0x50 },
-	};
+	struct spd_binfo spd_info = SPD_BINFO_SMBUS(0x50);
 
 	mem_cfg = &mupd->FspmConfig;
 
-	get_spd_smbus(&blk);
-	dump_spd_info(&blk);
-	assert(blk.spd_array[0][0] != 0);
+	const ssize_t common_len = gather_spds(&spd_info);
+	if (common_len <= 0)
+		die("Failed to read SPD from SMBus.\n");
+	dump_spd_info(&spd_info);
 
 	mainboard_fill_dq_map_data(&mem_cfg->DqByteMapCh0);
 	mainboard_fill_dqs_map_data(&mem_cfg->DqsMapCpu2DramCh0);
@@ -41,6 +43,6 @@
 	mainboard_fill_rcomp_strength_data(&mem_cfg->RcompTarget);
 
 	mem_cfg->DqPinsInterleaved = TRUE;
-	mem_cfg->MemorySpdDataLen = blk.len;
-	mem_cfg->MemorySpdPtr00 = (uintptr_t) blk.spd_array[0];
+	mem_cfg->MemorySpdDataLen = common_len;
+	mem_cfg->MemorySpdPtr00 = (uintptr_t)spd_info.spds[0].spd;
 }
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig
index 9452b6d..c8493fb 100644
--- a/src/soc/intel/cannonlake/Kconfig
+++ b/src/soc/intel/cannonlake/Kconfig
@@ -190,6 +190,7 @@
 config SOC_INTEL_CANNONLAKE_MEMCFG_INIT
 	bool
 	default n
+	select GENERIC_SPD_BIN
 
 config SOC_INTEL_COMMON_BLOCK_GSPI_CLOCK_MHZ
 	int
diff --git a/src/soc/intel/cannonlake/cnl_memcfg_init.c b/src/soc/intel/cannonlake/cnl_memcfg_init.c
index 4425862..87199cf 100644
--- a/src/soc/intel/cannonlake/cnl_memcfg_init.c
+++ b/src/soc/intel/cannonlake/cnl_memcfg_init.c
@@ -17,6 +17,8 @@
 #include <fsp/util.h>
 #include <soc/cnl_memcfg_init.h>
 #include <spd_bin.h>
+#include <stddef.h>
+#include <stdint.h>
 #include <string.h>
 
 static void meminit_memcfg(FSP_M_CONFIG *mem_cfg,
@@ -48,82 +50,25 @@
 		sizeof(mem_cfg->RcompTarget));
 }
 
-static void meminit_memcfg_spd(FSP_M_CONFIG *mem_cfg,
-			const struct cnl_mb_cfg *board_cfg,
-			size_t spd_data_len, uintptr_t spd_data_ptr)
-{
-	mem_cfg->MemorySpdDataLen = spd_data_len;
-	mem_cfg->MemorySpdPtr00 = spd_data_ptr;
-
-	/* Use the same spd data for channel 1, Dimm 0 */
-	mem_cfg->MemorySpdPtr10 = mem_cfg->MemorySpdPtr00;
-}
-
-/*
- * Initialize default memory settings using spd data contained in a buffer.
- */
-static void meminit_spd_data(FSP_M_CONFIG *mem_cfg,
-				const struct cnl_mb_cfg *cnl_cfg,
-				size_t spd_data_len, uintptr_t spd_data_ptr)
-{
-	assert(spd_data_ptr && spd_data_len);
-	meminit_memcfg_spd(mem_cfg, cnl_cfg, spd_data_len, spd_data_ptr);
-}
-
-/*
- * Initialize default memory settings using the spd file specified by
- * spd_index. The spd_index is an index into the SPD_SOURCES array defined
- * in spd/Makefile.inc.
- */
-static void meminit_cbfs_spd_index(FSP_M_CONFIG *mem_cfg,
-					const struct cnl_mb_cfg *cnl_cfg,
-					int spd_index)
-{
-	size_t spd_data_len;
-	uintptr_t spd_data_ptr;
-	struct region_device spd_rdev;
-
-	printk(BIOS_DEBUG, "SPD INDEX = %d\n", spd_index);
-	if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0)
-		die("spd.bin not found or incorrect index\n");
-	spd_data_len = region_device_sz(&spd_rdev);
-	/* Memory leak is ok since we have memory mapped boot media */
-	assert(IS_ENABLED(CONFIG_BOOT_DEVICE_MEMORY_MAPPED));
-	spd_data_ptr = (uintptr_t)rdev_mmap_full(&spd_rdev);
-	meminit_spd_data(mem_cfg, cnl_cfg, spd_data_len, spd_data_ptr);
-}
-
 /* Initialize onboard memory configurations for CannonLake */
 void cannonlake_memcfg_init(FSP_M_CONFIG *mem_cfg,
 			const struct cnl_mb_cfg *cnl_cfg,
-			const struct spd_info *spd)
+			struct spd_binfo *spd_info)
 {
-	bool OnModuleSpd = false;
 	/* Early Command Training Enabled */
 	mem_cfg->ECT = cnl_cfg->ect;
 	mem_cfg->DqPinsInterleaved = cnl_cfg->dq_pins_interleaved;
 	mem_cfg->CaVrefConfig = cnl_cfg->vref_ca_config;
 
-	/* Spd pointer will only be used if all smbus slave address of memory
-	* sockets on the platform is empty */
-	for (int i = 0; i < ARRAY_SIZE(mem_cfg->SpdAddressTable); i++) {
-		if (spd->spd_smbus_address[i] != 0) {
-			mem_cfg->SpdAddressTable[i] = spd->spd_smbus_address[i];
-			OnModuleSpd = true;
-		}
-	}
+	const ssize_t common_len = gather_spds(spd_info);
+	if (common_len <= 0)
+		die("Failed to read valid SPD(s).\n");
 
-	if (!OnModuleSpd) {
-		if (spd->spd_by_index) {
-			meminit_cbfs_spd_index(mem_cfg, cnl_cfg,
-				spd->spd_spec.spd_index);
-		} else {
-			meminit_spd_data(mem_cfg, cnl_cfg,
-				spd->spd_spec.spd_data_ptr_info.spd_data_len,
-				spd->spd_spec.spd_data_ptr_info.spd_data_ptr);
-		}
-	}
+	mem_cfg->MemorySpdDataLen = common_len;
+	mem_cfg->MemorySpdPtr00 = (uintptr_t)spd_info->spds[0].spd;
+	mem_cfg->MemorySpdPtr01 = (uintptr_t)spd_info->spds[1].spd;
+	mem_cfg->MemorySpdPtr10 = (uintptr_t)spd_info->spds[2].spd;
+	mem_cfg->MemorySpdPtr11 = (uintptr_t)spd_info->spds[3].spd;
 
 	meminit_memcfg(mem_cfg, cnl_cfg);
-
 }
diff --git a/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h b/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
index 245d2cf..6f2651c 100644
--- a/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
+++ b/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
@@ -19,6 +19,7 @@
 #include <stddef.h>
 #include <stdint.h>
 #include <fsp/soc_binding.h>
+#include <spd_bin.h>
 
 /* Number of dq bits controlled per dqs */
 #define DQ_BITS_PER_DQS 8
@@ -35,20 +36,6 @@
 	DDR_NUM_CHANNELS
 };
 
-struct spd_by_pointer {
-	size_t spd_data_len;
-	uintptr_t spd_data_ptr;
-};
-
-struct spd_info {
-	bool spd_by_index;
-	union spd_data_by {
-		int spd_index;
-		struct spd_by_pointer spd_data_ptr_info;
-	} spd_spec;
-	uint8_t spd_smbus_address[4];
-};
-
 /* Board-specific memory dq mapping information */
 struct cnl_mb_cfg {
 	/*
@@ -110,6 +97,6 @@
  */
 void cannonlake_memcfg_init(FSP_M_CONFIG *mem_cfg,
 			const struct cnl_mb_cfg *cnl_cfg,
-			const struct spd_info *spd);
+			struct spd_binfo *);
 
 #endif /* _SOC_CANNONLAKE_MEMCFG_INIT_H_ */

-- 
To view, visit https://review.coreboot.org/29481
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iac300a374e5cd56a9d202cf24ad8fe8367b780d8
Gerrit-Change-Number: 29481
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h at gmx.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181105/4aec2f73/attachment-0001.html>


More information about the coreboot-gerrit mailing list