Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 19:
(23 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
This still needs to be addressed.
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@10 PS12, Line 10: Added
Add
Done
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: […]
Done
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. : */
/* […]
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 121: vref_ca_config
This seems to be unused.
Its Used in deltan, right?, its being configured to 0, since we have DDR_A_VREF_CA and DDR_B_VREF_CA in deltan schematics.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 131: SO-DIMM based
Not true anymore.
Done
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. […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 312: if
nit: else if
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 310: : : if
nit: else if (info->topology == SODIMM) { […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if
yes, then use else to print some message.. channel out of config/support etc..
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if
nit: else if
Ack
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.
Ack
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. […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 328: if
nit: else if
Ack
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.
It throws me warning that it can be used uninitialized in "init_spd_upds(mem_cfg, i, spd_dimm0, spd_dimm1);"
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 343: = 0
same here.
Ack
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 […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 361: if
nit: else if
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 392: for dqs pair
for *every* dqs pair
Ack
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. […]
Ack
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. […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/16/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/16/src/soc/intel/tigerlake/me... PS16, Line 292: read_sodimm_spd
To make this consistent with memory-down, it would be good to also add a call to printing out SPD in […]
Done