Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/#/c/32513/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/32513/3/src/mainboard/google/hatch/variants/... PS3, Line 23: /* Access memory info using the spd file specified by spd_index */ : .spd = {.read_type = READ_SPD_CBFS}, I think its better to set this and spd_index in romstage.c since it will be the same for all hatch variants.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/cnl_memcfg_... PS3, Line 106: if (!cnl_cfg->spd.spd_spec.spd_smbus_address[i]) : continue; Do we need this check? If the user is explicitly setting READ_SMBUS now, we should just copy whatever is set in spd_smbus_address.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/cnl_memcfg_... PS3, Line 121: printk Probably add a die here? since this should not occur in production, but should be caught in development.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 43: mem_info_read_type It would be helpful to add comments indicating what each type means.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 52: spd_data_by A comment to explain how the data relates to the type would be helpful.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 62: struct spd_info spd;
how do you support configurations where you have one SPD in CBFS and one stored on DDR4 EEPROM?
There should be one mem_info_read_type for each slot.
Currently, FSP does not support that. IIUC, if SMBUS address is provided, it does not use the spd data pointers: https://review.coreboot.org/cgit/coreboot.git/tree/src/vendorcode/intel/fsp/...