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 11: Code-Review+2
(2 comments)
https://review.coreboot.org/#/c/32513/11/src/mainboard/google/hatch/romstage... File src/mainboard/google/hatch/romstage.c:
https://review.coreboot.org/#/c/32513/11/src/mainboard/google/hatch/romstage... PS11, Line 37: memory_sku So, one of the reasons why this was in baseboard defined as a __weak function was to allow any variants to define this differently if they choose different GPIOs or use a different way for identifying memory strap. Currently, the variants we are working on use the mem strap GPIOs the same way, but may not have the single channel strap. Its okay if you want to change this later, but we might need something like variant_is_single_channel_mem() that basically returns false on the variants that do not support it and baseboard weak function that reads GPP_F2 and returns value appropriately.
https://review.coreboot.org/#/c/32513/11/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/11/src/soc/intel/cannonlake/cnl_memcfg... PS11, Line 95: assert(mem_slot < NUM_DIMM_SLOT); Is this really required?