Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 26: Cannonlake Jasperlake here and in other places in this CL. I see multiple references to Cannonlake and CNL.
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 48: READ_SMBUS, Are we planning to read the SPD data through SMBUS or MEMPTR? If they are more for future, can we add them when those use-cases become a reality?
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... File src/soc/intel/tigerlake/jsl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 51: Nit: Remove this extra line
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 132: mem_cfg->ECT = jsl_cfg->ect; : mem_cfg->DqPinsInterleaved = jsl_cfg->dq_pins_interleaved; : mem_cfg->CaVrefConfig = jsl_cfg->vref_ca_config; Can be assigned as part of meminit_memcfg?
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 139: NOT_EXISTING: Is it valid to have 1 or 3 memory modules populated? My assumption is either 2 or 4 memory modules is a valid configuration.
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 163: meminit_memcfg(mem_cfg, jsl_cfg); Nothing DIMM_SLOT specific. I think it can be moved outside the for loop.