Shelley Chen 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 10:
(6 comments)
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. […]
I can take a look at this in a follow up cleanup CL.
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 ?
Ack
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 ? […]
Have updated the flag to accommodate single channel configurations with either channel 0 or 1.
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 ?
I can look into doing this in a follow up clean up CL.
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 ?
This code will be refactored with the followup clean up CL. Once we can detect by DRAM type (smbus, spd, etc.) we can check with that enum type instead of whether spd_smbus_address is set to 0.
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
Added some comments to address this.