soc/intel/cannonlake: Support different SPD read type for each slot

Also clean up cannonlake_memcfg_init.

The major changes include:
(1) Add enum 'mem_info_read_type' to spd_info.
(2) Add per-dimm-slot spd_info to cnl_mb_cfg.
(3) Setup memory config for each slot independently.
(4) Squash meminit_memcfg_spd().

BUG=chromium:960581, b:124990009
BRANCH=none
TEST=boot hatch, hatch_whl, and kohaku

Change-Id: I686a85996858204c20fd05ef24787a0487817c34
Signed-off-by: Philip Chen <philipchen@google.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32513
Reviewed-by: Paul Fagerburg <pfagerburg@chromium.org>
Reviewed-by: Furquan Shaikh <furquan@google.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>

src/mainboard/google/hatch/romstage.c

@@ -16,23 +16,63 @@
 #include <baseboard/variants.h>
 #include <console/console.h>
 #include <ec/google/chromeec/ec.h>
+#include <gpio.h>
 #include <memory_info.h>
 #include <soc/cnl_memcfg_init.h>
 #include <soc/romstage.h>
 #include <string.h>
 
+/* Memory configuration board straps */
+#define GPIO_MEM_CONFIG_0	GPP_F20
+#define GPIO_MEM_CONFIG_1	GPP_F21
+#define GPIO_MEM_CONFIG_2	GPP_F11
+#define GPIO_MEM_CONFIG_3	GPP_F22
+
+/*
+ * GPIO_MEM_CH_SEL is set to 1 for single channel skus
+ * and 0 for dual channel skus.
+ */
+#define GPIO_MEM_CH_SEL		GPP_F2
+
+static int memory_sku(void)
+{
+	const gpio_t spd_gpios[] = {
+		GPIO_MEM_CONFIG_0,
+		GPIO_MEM_CONFIG_1,
+		GPIO_MEM_CONFIG_2,
+		GPIO_MEM_CONFIG_3,
+	};
+
+	return gpio_base2_value(spd_gpios, ARRAY_SIZE(spd_gpios));
+}
+
 void mainboard_memory_init_params(FSPM_UPD *memupd)
 {
 	struct cnl_mb_cfg memcfg;
-
+	int mem_sku;
-	const struct spd_info spd = {
+	int is_single_ch_mem;
-		.spd_by_index = true,
-		.spd_spec.spd_index = variant_memory_sku(),
-	};
 
 	variant_memory_params(&memcfg);
-	cannonlake_memcfg_init(&memupd->FspmConfig,
+	mem_sku = memory_sku();
-			       &memcfg, &spd);
+	/*
+	 * GPP_F2 is the MEM_CH_SEL gpio, which is set to 1 for single
+	 * channel skus and 0 for dual channel skus.
+	 */
+	is_single_ch_mem = gpio_get(GPIO_MEM_CH_SEL);
+
+	/*
+	 * spd[0]-spd[3] map to CH0D0, CH0D1, CH1D0, CH1D1 respectively.
+	 * Dual-DIMM memory is not used in hatch family, so we only
+	 * fill in spd_info for CH0D0 and CH1D0 here.
+	 */
+	memcfg.spd[0].read_type = READ_SPD_CBFS;
+	memcfg.spd[0].spd_spec.spd_index = mem_sku;
+	if (!is_single_ch_mem) {
+		memcfg.spd[2].read_type = READ_SPD_CBFS;
+		memcfg.spd[2].spd_spec.spd_index = mem_sku;
+	}
+
+	cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg);
 }
 
 void mainboard_get_dram_part_num(const char **part_num, size_t *len)

src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h

@@ -22,13 +22,6 @@
 
 #define GPIO_PCH_WP	        GPP_C20
 
-/* Memory configuration board straps */
-#define GPIO_MEM_CONFIG_0	GPP_F20
-#define GPIO_MEM_CONFIG_1	GPP_F21
-#define GPIO_MEM_CONFIG_2	GPP_F11
-#define GPIO_MEM_CONFIG_3	GPP_F22
-
-
 /* EC wake pin is LAN_WAKE# */
 #define GPE_EC_WAKE		GPE0_LAN_WAK
 

src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h

@@ -31,9 +31,6 @@ const struct pad_config *base_early_gpio_table(size_t *num);
 const struct pad_config *override_gpio_table(size_t *num);
 const struct pad_config *override_early_gpio_table(size_t *num);
 
-/* Return memory SKU for the board. */
-int variant_memory_sku(void);
-
 /* Return board specific memory configuration */
 void variant_memory_params(struct cnl_mb_cfg *bcfg);
 

src/mainboard/google/hatch/variants/baseboard/memory.c

@@ -15,7 +15,6 @@
 
 #include <baseboard/variants.h>
 #include <baseboard/gpio.h>
-#include <gpio.h>
 #include <soc/cnl_memcfg_init.h>
 #include <string.h>
 
@@ -36,32 +35,4 @@ static const struct cnl_mb_cfg baseboard_memcfg = {
 void __weak variant_memory_params(struct cnl_mb_cfg *bcfg)
 {
 	memcpy(bcfg, &baseboard_memcfg, sizeof(baseboard_memcfg));
-	/*
-	 * GPP_F2 is the MEM_CH_SEL gpio, which is set to 1 for single
-	 * channel skus and 0 for dual channel skus.
-	 */
-	if (gpio_get(GPP_F2) == 1) {
-		/*
-		 * Single channel config: for Hatch, Channel 0 is
-		 * always populated.
-		 */
-		bcfg->channel_empty[0] = 0;
-		bcfg->channel_empty[1] = 1;
-	} else {
-		/* Dual channel config: both channels populated. */
-		bcfg->channel_empty[0] = 0;
-		bcfg->channel_empty[1] = 0;
-	}
-}
-
-int __weak variant_memory_sku(void)
-{
-	const gpio_t spd_gpios[] = {
-		GPIO_MEM_CONFIG_0,
-		GPIO_MEM_CONFIG_1,
-		GPIO_MEM_CONFIG_2,
-		GPIO_MEM_CONFIG_3,
-	};
-
-	return gpio_base2_value(spd_gpios, ARRAY_SIZE(spd_gpios));
 }

src/mainboard/google/hatch/variants/kohaku/memory.c

@@ -15,7 +15,6 @@
 
 #include <baseboard/variants.h>
 #include <baseboard/gpio.h>
-#include <gpio.h>
 #include <soc/cnl_memcfg_init.h>
 #include <string.h>
 
@@ -66,20 +65,4 @@ static const struct cnl_mb_cfg baseboard_memcfg = {
 void variant_memory_params(struct cnl_mb_cfg *bcfg)
 {
 	memcpy(bcfg, &baseboard_memcfg, sizeof(baseboard_memcfg));
-	/*
-	 * GPP_F2 is the MEM_CH_SEL gpio, which is set to 1 for single
-	 * channel skus and 0 for dual channel skus.
-	 */
-	if (gpio_get(GPP_F2) == 1) {
-		/*
-		 * Single channel config: for kohaku, Channel 0 is
-		 * always populated.
-		 */
-		bcfg->channel_empty[0] = 0;
-		bcfg->channel_empty[1] = 1;
-	} else {
-		/* Dual channel config: both channels populated. */
-		bcfg->channel_empty[0] = 0;
-		bcfg->channel_empty[1] = 0;
-	}
 }

src/mainboard/google/sarien/romstage.c

@@ -18,6 +18,18 @@
 #include <soc/romstage.h>
 
 static const struct cnl_mb_cfg memcfg = {
+	/* Access memory info through SMBUS. */
+	.spd[0] = {
+		.read_type = READ_SMBUS,
+		.spd_spec = {.spd_smbus_address = 0xa0},
+	},
+	.spd[1] = {.read_type = NOT_EXISTING},
+	.spd[2] = {
+		.read_type = READ_SMBUS,
+		.spd_spec = {.spd_smbus_address = 0xa4},
+	},
+	.spd[3] = {.read_type = NOT_EXISTING},
+
 	/*
 	 * The dqs_map arrays map the ddr4 pins to the SoC pins
 	 * for both channels.
@@ -45,12 +57,7 @@ static const struct cnl_mb_cfg memcfg = {
 
 void mainboard_memory_init_params(FSPM_UPD *memupd)
 {
-	const struct spd_info spd = {
-		.spd_smbus_address[0] = 0xa0,
-		.spd_smbus_address[2] = 0xa4
-	};
-
 	wilco_ec_romstage_init();
 
-	cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg, &spd);
+	cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg);
 }

src/mainboard/intel/coffeelake_rvp/memory.c

@@ -20,6 +20,23 @@
 #include <soc/cnl_memcfg_init.h>
 
 static const struct cnl_mb_cfg baseboard_memcfg_cfg = {
+	/* Access memory info through SMBUS. */
+	.spd[0] = {
+		.read_type = READ_SMBUS,
+		.spd_spec = {.spd_smbus_address = 0xA0}
+	},
+	.spd[1] = {
+		.read_type = READ_SMBUS,
+		.spd_spec = {.spd_smbus_address = 0xA2}
+	},
+	.spd[2] = {
+		.read_type = READ_SMBUS,
+		.spd_spec = {.spd_smbus_address = 0xA4}
+	},
+	.spd[3] = {
+		.read_type = READ_SMBUS,
+		.spd_spec = {.spd_smbus_address = 0xA6}
+	},
 	/*
 	 * The dqs_map arrays map the ddr4 pins to the SoC pins
 	 * for both channels.

src/mainboard/intel/coffeelake_rvp/romstage.c

@@ -20,13 +20,5 @@
 
 void mainboard_memory_init_params(FSPM_UPD *memupd)
 {
-	const struct spd_info spd = {
+	cannonlake_memcfg_init(&memupd->FspmConfig, variant_memcfg_config());
-		.spd_smbus_address[0] = 0xA0,
-		.spd_smbus_address[1] = 0xA2,
-		.spd_smbus_address[2] = 0xA4,
-		.spd_smbus_address[3] = 0xA6,
-	};
-
-	cannonlake_memcfg_init(&memupd->FspmConfig,
-				variant_memcfg_config(), &spd);
 }

src/soc/intel/cannonlake/cnl_memcfg_init.c

@@ -48,28 +48,39 @@ static void meminit_memcfg(FSP_M_CONFIG *mem_cfg,
 	       sizeof(mem_cfg->RcompTarget));
 }
 
-static void meminit_memcfg_spd(FSP_M_CONFIG *mem_cfg,
-			const struct cnl_mb_cfg *cnl_cfg,
-			size_t spd_data_len, uintptr_t spd_data_ptr)
-{
-	mem_cfg->MemorySpdDataLen = spd_data_len;
-
-	if (cnl_cfg->channel_empty[0] == 0)
-		mem_cfg->MemorySpdPtr00 = spd_data_ptr;
-
-	if (cnl_cfg->channel_empty[1] == 0)
-		mem_cfg->MemorySpdPtr10 = spd_data_ptr;
-}
-
 /*
  * Initialize default memory settings using spd data contained in a buffer.
  */
-static void meminit_spd_data(FSP_M_CONFIG *mem_cfg,
+static void meminit_spd_data(FSP_M_CONFIG *mem_cfg, uint8_t mem_slot,
-				const struct cnl_mb_cfg *cnl_cfg,
 			     size_t spd_data_len, uintptr_t spd_data_ptr)
 {
-	assert(spd_data_ptr && spd_data_len);
+	static size_t last_set_spd_data_len = 0;
-	meminit_memcfg_spd(mem_cfg, cnl_cfg, spd_data_len, spd_data_ptr);
+
+	assert(spd_data_ptr != 0 && spd_data_len != 0);
+
+	if (last_set_spd_data_len != 0 &&
+	    last_set_spd_data_len != spd_data_len)
+		die("spd data length disparity among slots");
+
+	mem_cfg->MemorySpdDataLen = spd_data_len;
+	last_set_spd_data_len = spd_data_len;
+
+	switch (mem_slot) {
+	case 0:
+		mem_cfg->MemorySpdPtr00 = spd_data_ptr;
+		break;
+	case 1:
+		mem_cfg->MemorySpdPtr01 = spd_data_ptr;
+		break;
+	case 2:
+		mem_cfg->MemorySpdPtr10 = spd_data_ptr;
+		break;
+	case 3:
+		mem_cfg->MemorySpdPtr11 = spd_data_ptr;
+		break;
+	default:
+		die("nonexistent memory slot");
+	}
 }
 
 /*
@@ -78,13 +89,13 @@ static void meminit_spd_data(FSP_M_CONFIG *mem_cfg,
  * 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, uint8_t mem_slot)
-					int spd_index)
 {
 	size_t spd_data_len;
 	uintptr_t spd_data_ptr;
 	struct region_device spd_rdev;
 
+	assert(mem_slot < NUM_DIMM_SLOT);
 	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");
@@ -92,40 +103,42 @@ static void meminit_cbfs_spd_index(FSP_M_CONFIG *mem_cfg,
 	/* Memory leak is ok since we have memory mapped boot media */
 	assert(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);
+	meminit_spd_data(mem_cfg, mem_slot, 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 cnl_mb_cfg *cnl_cfg)
-			const struct spd_info *spd)
 {
-	bool OnModuleSpd = false;
+	const struct spd_info *spdi;
+
 	/* 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
+	for (int i = 0; i < NUM_DIMM_SLOT; i++) {
-	* sockets on the platform is empty */
+		spdi = &(cnl_cfg->spd[i]);
-	for (int i = 0; i < ARRAY_SIZE(mem_cfg->SpdAddressTable); i++) {
+		switch (spdi->read_type) {
-		if (spd->spd_smbus_address[i] != 0) {
+		case NOT_EXISTING:
-			mem_cfg->SpdAddressTable[i] = spd->spd_smbus_address[i];
+			break;
-			OnModuleSpd = true;
+		case READ_SMBUS:
-		}
+			mem_cfg->SpdAddressTable[i] =
-	}
+				spdi->spd_spec.spd_smbus_address;
-
+			break;
-	if (!OnModuleSpd) {
+		case READ_SPD_CBFS:
-		if (spd->spd_by_index) {
+			meminit_cbfs_spd_index(mem_cfg,
-			meminit_cbfs_spd_index(mem_cfg, cnl_cfg,
+				spdi->spd_spec.spd_index, i);
-				spd->spd_spec.spd_index);
+			break;
-		} else {
+		case READ_SPD_MEMPTR:
-			meminit_spd_data(mem_cfg, cnl_cfg,
+			meminit_spd_data(mem_cfg, i,
-				spd->spd_spec.spd_data_ptr_info.spd_data_len,
+				spdi->spd_spec.spd_data_ptr_info.spd_data_len,
-				spd->spd_spec.spd_data_ptr_info.spd_data_ptr);
+				spdi->spd_spec.spd_data_ptr_info.spd_data_ptr);
-		}
+			break;
+		default:
+			die("no valid way to read mem info");
 		}
 
 		meminit_memcfg(mem_cfg, cnl_cfg);
-
+	}
 }

src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h

@@ -23,6 +23,9 @@
 /* Number of dq bits controlled per dqs */
 #define DQ_BITS_PER_DQS 8
 
+/* Number of memory DIMM slots available on Cannonlake board */
+#define NUM_DIMM_SLOT 4
+
 /*
  * Number of memory packages, where a "package" represents a 64-bit solution.
  */
@@ -40,17 +43,32 @@ struct spd_by_pointer {
 	uintptr_t spd_data_ptr;
 };
 
+enum mem_info_read_type {
+	NOT_EXISTING,	/* No memory in this slot */
+	READ_SMBUS,	/* Read on-module spd by SMBUS. */
+	READ_SPD_CBFS,	/* Find spd file in CBFS. */
+	READ_SPD_MEMPTR /* Find spd data from pointer. */
+};
+
 struct spd_info {
-	bool spd_by_index;
+	enum mem_info_read_type read_type;
 	union spd_data_by {
+		/* To read on-module spd when read_type is READ_SMBUS. */
+		uint8_t spd_smbus_address;
+
+		/* To identify spd file when read_type is READ_SPD_CBFS. */
 		int spd_index;
+
+		/* To find spd data when read_type is READ_SPD_MEMPTR. */
 		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 {
+	/* Parameters required to access SPD for CH0D0/CH0D1/CH1D0/CH1D1. */
+	struct spd_info spd[NUM_DIMM_SLOT];
+
 	/*
 	 * For each channel, there are 6 sets of DQ byte mappings,
 	 * where each set has a package 0 and a package 1 value (package 0
@@ -107,26 +125,12 @@ struct cnl_mb_cfg {
 
 	/* Early Command Training Enabled */
 	uint8_t ect;
-
-	/*
-	 * Flags to indicate which channels are populated.  We
-	 * currently support single or dual channel configurations.
-	 * Set 1 to indicate that the channel is not populated Set 0
-	 * to indicate that the channel is populated.  For example,
-	 * dual channel memory configuration would have both
-	 * channel_empty[0] = 0 and channel_empty[1] = 0.  Note that
-	 * this flag is only used for soldered down DRAM where we get
-	 * SPD data from CBFS.  We need the value 0 to default to
-	 * populated in order to support existing boards.
-	 */
-	uint8_t channel_empty[2];
 };
 
 /*
  * Initialize default memory configurations for CannonLake.
  */
 void cannonlake_memcfg_init(FSP_M_CONFIG *mem_cfg,
-			const struct cnl_mb_cfg *cnl_cfg,
+			    const struct cnl_mb_cfg *cnl_cfg);
-			const struct spd_info *spd);
 
 #endif /* _SOC_CANNONLAKE_MEMCFG_INIT_H_ */