Paul Fagerburg 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
(2 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 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]. […]
What Furquan said. Just make sure to add a comment to explain why spd[1] and spd[3] are never modified.
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 122: spdi = cnl_cfg->spd[i]; As Furquan suggested, use a pointer. This assignment is doing a bitwise-copy of the entire struct because spdi is declared as a struct instead of a pointer to a struct. It would be more efficient to have a pointer to a struct.