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 15:
(21 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 DDR4 memory
This still needs to be addressed.
https://review.coreboot.org/c/coreboot/+/39847/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/15//COMMIT_MSG@9 PS15, Line 9: hich uses SMBus address not true anymore.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 99: /* DQ mapping */ I think it would be helpful to add some information like lpddr4x. Example:
/* * DQ CPU<>DRAM map: * DDR4 memory interface has 8 DQs per channel. Each DQ consists of 8 bits(1 * byte). Thus, dq_map is represented as DDR[1-0]_DQ[7-0][7:0], where * DDR[1-0] : DDR4 channel # * DQ[7-0] : DQ # within the channel * [7:0] : Bits within the DQ * * Index of the array represents DQ pin# on the CPU, whereas value in * the array represents DQ pin# on the memory part. */
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 101: /* : * DQS CPU<>DRAM map. Each array entry represents a : * mapping of a dq bit on the CPU to the bit it's connected to on : * the memory part. The array index represents the dqs bit number : * on the memory part, and the values in the array represent which : * pin on the CPU that DRAM pin connects to. : */ /* * DQS CPU<>DRAM map: * DDR4 memory interface has 8 DQS pairs per channel. Thus, dqs_map is represented as * DDR[1-0]_DQS[7-0], where * DDR[1-0] : DDR4 channel # * DQS[7-0] : DQS # within the channel * * Index of the array represents DQS pin# on the CPU, whereas value in * the array represents DQ pin# on the memory part. */
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 121: vref_ca_config This seems to be unused.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 131: SO-DIMM based Not true anymore.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 307: if ((info->topology == MEMORY_DOWN) || (info->topology == MIXED)) { A comment here would be helpful indicating that for mixed topologies, ch0 can only be MD. Helps to tie things together.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 310: : : if nit: else if (info->topology == SODIMM) { ... }
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if nit: else if
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 321: return Using `else if` should avoid the need of adding early return here.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 324: || (info->topology == MIXED) This is not correct. If mixed topology is used, channel1 has to be SODIMM. This check needs to go down along with SODIMM. Also, a comment for the mixed topology would be helpful.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 342: = 0 I don't think these need to be set. It is initialized before being used.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 343: = 0 same here.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 349: = { : .addr_map[0] = info->smbus_info[0].addr_dimm0, : .addr_map[1] = info->smbus_info[0].addr_dimm1, : .addr_map[2] = info->smbus_info[1].addr_dimm0, : .addr_map[3] = info->smbus_info[1].addr_dimm1, : }; nit: Since you have a separate function read_sodimm_spd(), I think it makes sense to move setting of addr_map[3-0] to it. spd_sodimm_blk still needs to be passed in from this function to read_sodimm_spd() since it is used in this function later on.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 363: mem_cfg->MemorySpdDataLen = spd_sodimm_len; How do you plan to ensure that MemorySpdDataLen is correct in MIXED topology?
if ((info->topology == MIXED) && (mem_cfg->MemorySpdDataLen != spd_sodimm_len)) die(...); else mem_cfg->MemorySpdDataLen = spd_sodimm_len;
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 373: DDR4x Just DDR4.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 377: for dq pair for *every* dq pair
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 389: DDR4x Just DDR4
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 392: for dqs pair for *every* dqs pair
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 403: unsigned int i Can you please move this declaration to start of this function. No need to declare it in each for loop.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 405: init_dq_upds(mem_cfg, 4i+b, board_cfg->dq_map[i][b], : board_cfg->dq_map[i][b+1]); : init_dqs_upds(mem_cfg, 4i+b, board_cfg->dqs_map[i][b], : board_cfg->dqs_map[i][b+1]); This will have to take into account empty v/s populated slots.
/* Channel 0 */ for (b = 0, i = 0; b < DDR4_BYTES_PER_CHANNEL; i++, b+=2) { init_dq_upds(mem_cfg, i, board_cfg->dq_map[0][b], board_cfg->dq_map[0][b+1]; init_dqs_upds(mem_cfg, i, board_cfg->dqs_map[0][b], board_cfg->dqs_map[0][b+1]; }
/* Channel 1 */ for (b = 0; b < DDR4_BYTES_PER_CHANNEL; i++, b+=2) { if (half_populated) { init_dq_upds_empty(mem_cfg, i); init_dqs_upds_empty(mem_cfg, i); } else { init_dq_upds(mem_cfg, i, board_cfg->dq_map[1][b], board_cfg->dq_map[1][b+1]; init_dqs_upds(mem_cfg, i, board_cfg->dqs_map[1][b], board_cfg->dqs_map[1][b+1]; } }
or, maybe:
index = 0; for (i = 0; i < DDR4_CHANNELS; i++) { for (int b = 0; b < DDR4_BYTES_PER_CHANNEL; b += 2) { if (half_populated && (i == 1)) { init_dq_upds_empty(mem_cfg, index); init_dqs_upds_empty(mem_cfg, index); } else { init_dq_upds(mem_cfg, index, board_cfg->dq_map[i][b], board_cfg->dq_map[i][b+1]; init_dqs_upds(mem_cfg, index, board_cfg->dqs_map[i][b], board_cfg->dqs_map[i][b+1]; } index++; } }