Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 8: Code-Review-1
(5 comments)
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/kohaku/memory.c:
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... PS8, Line 66: __weak Why is weak added here?
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... PS8, Line 80: for (int i = 0; i < NUM_DIMM_SLOT; i++) { : if (is_single_ch_mem && i >= (NUM_DIMM_SLOT / 2)) : bcfg->spd[i].read_type = NOT_EXISTING; : else { : bcfg->spd[i].read_type = READ_SPD_CBFS; : bcfg->spd[i].spd_spec.spd_index = mem_sku; : } This is not correct. Kohaku and Hatch need to just fill in spd[0] and spd[2]. spd[1] and spd[3] are always NOT_EXISTING. You can just do something like this:
bcfg->spd[0].read_type = READ_SPD_CBFS; bcfg->spd[0].spd_spec.spd_index = mem_sku;
if (!is_single_ch_mem) { bcfg->spd[2].read_type = READ_SPD_CBFS; bcfg->spd[2].spd_spec.spd_index = mem_sku; }
In fact, everything except memcpy can be done in mainboard_memory_init_params in romstage.c since it is the same for kohaku and hatch.
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 114: struct spd_info spdi; Just use a pointer? struct spd_info *spdi;
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 131: cnl_cfg Why not just pass in spd_index here?
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/include/soc... PS8, Line 62: MEMMPTR nit: MEMPTR