mem_chip_info: Update to new format

The original version of the mem_chip_info structure does not record rank
information and does not allow precise modeling of certain DDR
configurations, so it falls short on its purpose to compile all
available memory information. This patch updates the format to a new
layout that remedies these issues. Since the structure was introduced so
recently that no firmware using it has been finalized and shipped yet,
we should be able to get away with this without accounting for backwards
compatibility.

BRANCH=corsola

Cq-Depend: chromium:3980175
Signed-off-by: Julius Werner <jwerner@chromium.org>
Change-Id: If34e6857439b6f6ab225344e5b4dd0ff11d8d42a
Reviewed-on: https://review.coreboot.org/c/coreboot/+/68871
Reviewed-by: Yu-Ping Wu <yupingso@google.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Paul Menzel <paulepanter@mailbox.org>
Reviewed-by: Xixi Chen <xixi.chen@mediatek.corp-partner.google.com>

src/commonlib/bsd/include/commonlib/bsd/mem_chip_info.h

@@ -13,23 +13,98 @@ enum mem_chip_type {
 	MEM_CHIP_LPDDR4X = 0x49,
 };
 
+#define MEM_CHIP_STRUCT_VERSION 0	/* Hopefully we'll never have to bump this... */
+
+/*
+ * This structure describes memory topology by channel, rank and DDR chip.
+ *
+ * Definitions:
+ *	channel: A DDR channel is an entire set of DDR pins (including DQ, CS, CA, etc.) coming
+ *		 out of the DDR controller on the SoC. An SoC may support one or more DDR
+ *		 channels. The DDR chips on different channels are entirely independent from one
+ *		 another and not aware of each others' existence (concepts like dual-channel
+ *		 mode / channel-interleaving only exist inside the DDR controller and are not
+ *		 relevant to this topology description).
+ *	chip:	 A physically distinct DDR part on the mainboard, with a single "channel" worth
+ *		 of pins (DQ, CS, CA, etc.). Parts that combine multiple "channels" worth of
+ *		 pins in one package (e.g. separate DQ[0]_A and DQ[0]_B) count as multiple
+ *		 separate chips in this description.
+ *	rank:	 DDR ranks are independent sub-units within a single physical DDR chip. Each DDR
+ *		 transaction only communicates with one rank, which is selected through the CS
+ *		 pins.
+ *	MRx:	 Mode registers, as defined in the various (LP)DDR specs. Mode registers are
+ *		 read through the DQ and written through the CA pins, and each rank on each chip
+ *		 has a separate mode register as selected by the CS pins.
+ *
+ * The basic purpose of this structure is to record information read from the mode registers on
+ * all ranks of all chips on all channels, to later allow the recipient of this information to
+ * reconstruct the topology and exact parts used on the board. Since each system may have a
+ * variable number of channels, and the chips on those channels may each have a variable number
+ * of ranks, this would require a doubly-nested structure with variable array sizes in both
+ * dimensions. The size and offset calculations in such a structure would become very cumbersome
+ * and error-prone, so instead this design just stores a one-dimensional array of "entries"
+ * where each entry stores information about one specific rank on one specific channel. This
+ * means that information which is specific to the channel itself is duplicated among all such
+ * entries for all ranks on that channel, and in a well-formed instance of this structure the
+ * values in per-channel fields (`type` and `channel_io_width`) among all entries that have the
+ * same value in the `channel` field MUST be identical.
+ *
+ * The information read from the mode registers should be decoded into normal integer values,
+ * but not otherwise adjusted in any way. For example, if the value read from MR8 on an LPDDR4
+ * chip is 0b00010000, the `density_mbits` field should be set to 8192 (MR8[5:2] = 0b0100
+ * decoded to 8192 Mbits) and the `io_width` field should be set to 16 (MR8[7:6] = 0b00 decoded
+ * to x16.
+ *
+ * Note that on some systems the I/O width (number of DQ pins) of the SoC controller's channel
+ * is a multiple of the I/O width on the DDR chip (which is the reason the two are recorded
+ * separately in this structure). This means that two identical DDR chips are wired in parallel
+ * on the same channel (e.g. one 16-bit part is wired to DQ[0:15] and the other to DQ[16:31]).
+ * All other pins beside DQ are shorted together in this configuration. This means that only the
+ * mode registers of the first chip can be read and recorded in this structure (and all mode
+ * register writes automatically write the same value to all chips through the shorted CA pins).
+ * The other chips are reported "implicitly" via the mismatch in I/O width.
+ *
+ * That means that the total amount of memory in bytes available on the whole system is:
+ *
+ * SUM[over all entries](density_mbits * (channel_io_width/io_width)) * 1024 * 1024 / 8
+ */
 struct mem_chip_info {
+	uint8_t num_entries;
+	uint8_t reserved[2];		/* Must be set to 0 */
+	uint8_t struct_version;		/* Must always be MEM_CHIP_STRUCT_VERSION */
+	struct mem_chip_entry {
+		uint8_t channel;	/* Channel number this entry belongs to */
+		uint8_t rank;		/* Rank number within that channel */
+
+		/* per-channel information */
 		uint8_t type;		/* enum mem_chip_type */
-	uint8_t num_channels;
-	uint8_t reserved[6];
-	struct mem_chip_channel {
-		uint64_t density;	/* number in _bytes_, not Megabytes! */
-		uint8_t io_width;	/* should be `8`, `16`, `32` or `64` */
+		uint8_t channel_io_width; /* I/O width of the channel (no. of DQ pins on SoC) */
+
+		/* per-rank information */
+		uint32_t density_mbits;	/* density in megabits, decoded from MR8 */
+		uint8_t io_width;	/* I/O width of the DDR chip, decoded from MR8 */
 		uint8_t manufacturer_id; /* raw value from MR5 */
 		uint8_t revision_id[2];	/* raw values from MR6 and MR7 */
-		uint8_t reserved[4];
-		uint8_t serial_id[8];	/* LPDDR5 only, MR47 - MR54 */
-	} channel[0];
+		uint8_t serial_id[8];	/* LPDDR5 only (set to 0 otherwise), MR47 - MR54 */
+	} entries[0];
 };
 
-static inline size_t mem_chip_info_size(struct mem_chip_info *info)
+static inline size_t mem_chip_info_size(int entries)
 {
-	return sizeof(*info) + sizeof(info->channel[0]) * info->num_channels;
+	return sizeof(struct mem_chip_info) + sizeof(struct mem_chip_entry) * entries;
 };
 
+static inline uint64_t mem_chip_info_total_density_bytes(const struct mem_chip_info *info)
+{
+	uint64_t bytes = 0;
+	int i;
+
+	for (i = 0; i < info->num_entries; i++) {
+		const struct mem_chip_entry *e = &info->entries[i];
+		bytes += e->density_mbits * (e->channel_io_width / e->io_width) * (MiB / 8);
+	}
+
+	return bytes;
+}
+
 #endif /* _COMMONLIB_BSD_MEM_CHIP_INFO_H_ */

src/soc/mediatek/common/emi.c

@@ -19,8 +19,7 @@ size_t sdram_size(void)
 	mc = cbmem_find(CBMEM_ID_MEM_CHIP_INFO);
 	assert(mc);
 
-	for (unsigned int i = 0; i < mc->num_channels; ++i)
-		size += mc->channel[i].density;
+	size = mem_chip_info_total_density_bytes(mc);
 
 	printk(BIOS_INFO, "dram size: %#lx\n", size);
 	return size;

src/soc/mediatek/common/memory.c

@@ -112,20 +112,26 @@ size_t mtk_dram_size(void)
 
 static void fill_dram_info(struct mem_chip_info *mc, const struct ddr_base_info *ddr)
 {
-	unsigned int i;
-	size_t size;
+	unsigned int c, r;
 
-	mc->type = MEM_CHIP_LPDDR4X;
-	mc->num_channels = CHANNEL_MAX;
-	size = mtk_dram_size();
-	assert(size);
+	mc->num_entries = CHANNEL_MAX * ddr->mrr_info.rank_nums;
+	mc->struct_version = MEM_CHIP_STRUCT_VERSION;
 
-	for (i = 0; i < mc->num_channels; ++i) {
-		mc->channel[i].density = size / mc->num_channels;
-		mc->channel[i].io_width = DQ_DATA_WIDTH_LP4;
-		mc->channel[i].manufacturer_id = ddr->mrr_info.mr5_vendor_id;
-		mc->channel[i].revision_id[0] = ddr->mrr_info.mr6_revision_id;
-		mc->channel[i].revision_id[1] = ddr->mrr_info.mr7_revision_id;
+	struct mem_chip_entry *entry = mc->entries;
+	for (c = 0; c < CHANNEL_MAX; c++) {
+		for (r = 0; r < ddr->mrr_info.rank_nums; r++) {
+			entry->channel = c;
+			entry->rank = r;
+			entry->type = MEM_CHIP_LPDDR4X;
+			entry->channel_io_width = DQ_DATA_WIDTH_LP4;
+			entry->density_mbits = ddr->mrr_info.mr8_density[r] / CHANNEL_MAX /
+					       (MiB / 8);
+			entry->io_width = DQ_DATA_WIDTH_LP4;
+			entry->manufacturer_id = ddr->mrr_info.mr5_vendor_id;
+			entry->revision_id[0] = ddr->mrr_info.mr6_revision_id;
+			entry->revision_id[1] = ddr->mrr_info.mr7_revision_id;
+			entry++;
+		}
 	}
 }
 
@@ -140,9 +146,10 @@ static void add_mem_chip_info(int unused)
 		return;
 	}
 
-	size = sizeof(*mc) + sizeof(struct mem_chip_channel) * CHANNEL_MAX;
+	size = mem_chip_info_size(CHANNEL_MAX * curr_ddr_info->mrr_info.rank_nums);
 	mc = cbmem_add(CBMEM_ID_MEM_CHIP_INFO, size);
 	assert(mc);
+	memset(mc, 0, size);
 
 	fill_dram_info(mc, curr_ddr_info);
 }

src/soc/qualcomm/common/qclib.c

@@ -21,14 +21,15 @@
 #define QCLIB_VERSION 0
 
 /* store QcLib return data until CBMEM_CREATION_HOOK runs */
-static void *mem_chip_addr;
+static struct mem_chip_info *mem_chip_info;
 
 static void write_mem_chip_information(struct qclib_cb_if_table_entry *te)
 {
+	struct mem_chip_info *info = (void *)te->blob_address;
 	if (te->size > sizeof(struct mem_chip_info) &&
-	    te->size == mem_chip_info_size((void *)te->blob_address)) {
-		/* Save mem_chip_addr in global variable ahead of hook running */
-		mem_chip_addr = (void *)te->blob_address;
+	    te->size == mem_chip_info_size(info->num_entries)) {
+		/* Save mem_chip_info in global variable ahead of hook running */
+		mem_chip_info = info;
 	}
 }
 
@@ -37,19 +38,20 @@ static void add_mem_chip_info(int unused)
 	void *mem_region_base = NULL;
 	size_t size;
 
-	if (!mem_chip_addr) {
+	if (!mem_chip_info || !mem_chip_info->num_entries ||
+	    mem_chip_info->struct_version != MEM_CHIP_STRUCT_VERSION) {
 		printk(BIOS_ERR, "Did not receive valid mem_chip_info from QcLib!");
 		return;
 	}
 
-	size = mem_chip_info_size(mem_chip_addr);
+	size = mem_chip_info_size(mem_chip_info->num_entries);
 
 	/* Add cbmem table */
 	mem_region_base = cbmem_add(CBMEM_ID_MEM_CHIP_INFO, size);
 	ASSERT(mem_region_base != NULL);
 
 	/* Migrate the data into CBMEM */
-	memcpy(mem_region_base, mem_chip_addr, size);
+	memcpy(mem_region_base, mem_chip_info, size);
 }
 
 CBMEM_CREATION_HOOK(add_mem_chip_info);