Meera Ravindranath 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 3:
(7 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.
Done
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 ad […]
Since this is SoC code, prefer to have all the methods to provide SPD info. This could be used not only by chrome systems where we are using soldered down memory, but by other board manufacturers which use replaceable DIMMs as well.
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 136: jasperlake
Nit: I don't think we need jasperlake or jsl prefix in the structures and functions.
Since this SoC folder supports two SoCs and Tigerlake is using a different way of configuring the DIMMs and these functions are JSL specific, we would like to prefix it with jsl.
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
Done
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?
Ack
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 i […]
Yes, it is possible to have only 1 or 3 DIMMs. Whether such a configuration is optimal or not is debatable.
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.
Ack