Furquan Shaikh 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:
(15 comments)
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/in... PS19, Line 98: ddr4x_ ddr4
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 121: vref_ca_config
Its Used in deltan, right?, its being configured to 0, since we have DDR_A_VREF_CA and DDR_B_VREF_CA […]
I meant it is unused in this patch. Don't you need to set some UPD based on this?
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 342: = 0
It throws me warning that it can be used uninitialized in "init_spd_upds(mem_cfg, i, spd_dimm0, spd_ […]
Probably because ddr4_get_spd() has a branch where spd_dimm0 and spd_dimm1 are not set. Not sure if you add die the compiler would be smart enough to understand that. Something to try.
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: , ;
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 297: , ;
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 298: , ;
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 299: , ;
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 303: uintptr_t const
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 304: struct const
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.
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: /* For mixed topology, channel 1 can only be SODIMM */
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.
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 343: = 0 Not required.
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 407: 4i+b
4 * i + b?
Or just use index?
init_dq_upds(memcfg, index, ...);
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 409: 4i+b
4 * i + b?
Same here.