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 20:
(14 comments)
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 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
No longer required
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 363: mem_cfg->MemorySpdDataLen = spd_sodimm_len;
How do you plan to ensure that MemorySpdDataLen is correct in MIXED topology? […]
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 296: ,
;
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 297: ,
;
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 299: ,
;
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 303: uintptr_t
const
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 304: struct
const
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 318: BIOS_INFO
This is not just info, it is an error. Probably die() like Tim suggested.
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 325: } else if ((info->topology == SODIMM) || (info->topology == MIXED)) {
Comment would be helpful: […]
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 329: printk(BIOS_DEBUG, "Undefined memory topology on channel 1");
Same here.
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 331: printk(BIOS_DEBUG, "Unsupported channels");
I think this is worth a die() call instead of just a print.
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 343: = 0
Not required.
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 407: 4i+b
Or just use index? […]
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 409: 4i+b
Actually I can't get init_dqs_upds meaning... […]
It is initializing Fspm params.