Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )
Change subject: soc/intel/cannonlake: Add field to identify single channel memory ......................................................................
Patch Set 8:
(6 comments)
The whole cannonlake_memcfg_init() seems to be not designed very well. Please improve the api.
You could add an enum for each DIMM: NOT_EXISTING, READ_SMBUS, READ_SPD_CBFS, READ_SPD_MEMPTR
That way the board config would hold all possible combinations.
https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 52: static void meminit_memcfg_spd(FSP_M_CONFIG *mem_cfg, only used once in this file. Please squash with meminit_spd_data().
https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 53: const struct cnl_mb_cfg *board_cfg, why is it board_cfg, but below it's cnl_cfg ?
https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 60: mem_cfg->MemorySpdPtr10 = 0; why do you assume channel0 is connected to DRAM ? That's not documented.
https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 103: const struct cnl_mb_cfg *cnl_cfg, why isn't struct spd_info part if struct cnl_mb_cfg ?
https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 112: /* Spd pointer will only be used if all smbus slave address of memory why ?
https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/include/soc... PS8, Line 108: * Flag to indicate if the memory configuration is single it's only used for soldered DRAM where SPD resides in CBFS, but that's not documented