Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 12:
(10 comments)
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@7 PS12, Line 7: soc/intel/tigerlake: Support to initialize Memory
Add support to initialize memory
Add support to initialize DDR4 memory
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@9 PS12, Line 9: which uses SMBus address Why is this being restricted to SoDIMM memory only?
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 102: dq_map This should actually be organized as: uint8_t dq_map[DDR4_CHANNELS][DDR4_BYTES_PER_CHANNEL][BITS_PER_BYTE]; where DDR4_CHANNELS is 2 and DDR4_BYTES_PER_CHANNEL is 8.
Also, can you please add a comment similar to https://review.coreboot.org/c/coreboot/+/39865/7/src/soc/intel/tigerlake/inc... to show how the array matches with the hardware mapping.
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 110: dqs_map uint8_t dqs_map[DDR4_CHANNELS][DDR4_BYTES_PER_CHANNEL];
And comment showing the mapping : https://review.coreboot.org/c/coreboot/+/39865/7/src/soc/intel/tigerlake/inc...
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 111: /* : * Rcomp resistor value. The value represent the resistance in ohms : * of the rcomp resistor attached to the DDR_COMP pins on the DRAM. : */ : uint16_t rcomp_resistor; This is not required even for DDR4. We had got a confirmation from Intel here: https://issuetracker.google.com/142650263#comment27
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 139: _sodimm This function can be simply named: meminit_ddr4x. spd_info should provide information about the memory topology.
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 308: unsigned const
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 309: unsigned const
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 320: struct spd_block blk = { : .addr_map[0] = spd->smbus_info[0].addr_dimm0, : .addr_map[1] = spd->smbus_info[0].addr_dimm1, : .addr_map[2] = spd->smbus_info[1].addr_dimm0, : .addr_map[3] = spd->smbus_info[1].addr_dimm1, : }; : : read_smbus_spd(&blk, &spd_len, &spd_data0, &spd_data1); This function should be able to handle all three memory topologies. I am thinking something like:
if ((info->topology == MEMORY_DOWN) || (info->topology == MIXED))
-------read_md_spd(info, &spd_md_data, &spd_md_len);
if ((info->topology == SODIMM) || (info->topology == MIXED)
-------read_sodimm_spd(info, &spd_sodimm_blk);
And then for each channel you will have to:
for (i = 0; i < DDR4_CHANNELS; i++) {
-------ddr4_get_spd(i, spd_md_data, *spd_sodimm_blk, info, half_populated ------->------->-------&spd_dimm0, &spd_dimm1); -------init_spd_upds(mem_cfg, i, spd_dimm0, spd_dimm1);
}
where ddr4_get_spd can fill in the right spd pointers based on:
Channel 0: - If mixed or memory down, use spd_md_data. In this case, spd_dimm1 will be 0. - If sodimm, use spd_sodimm_blk. In this case, spd_dimm0 will be blk->spd_array[0] and spd_dimm1 will be blk->spd_array[1];
Channel 1: - If half populated, spd_dimm0 and spd_dimm1 will be 0. - If mixed or sodimm, use spd_sodimm_blk. - If memory down, use spd_md_data.
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 339: for (unsigned int i = 0; i < DDR4_CHANNELS; i++) { : for (int b = 0; b < DQ_SODIMM_SLOT; b++) { : init_dq_upds(mem_cfg, 4i+b, &board_cfg->dq_map[i][DQ_PER_CHANNEL*b]); : init_dqs_upds(mem_cfg, 4i+b, &board_cfg->dqs_map[i][DQS_PER_CHANNEL*b]); : } : } This will have to be updated as per latest changes.