Attention is currently required from: yuchi.chen@intel.com.
Shuo Liu has posted comments on this change by yuchi.chen@intel.com. ( https://review.coreboot.org/c/coreboot/+/83320?usp=email )
Change subject: soc/intel/common/block/imc: add Integrated Memory Controller driver ......................................................................
Patch Set 3:
(4 comments)
File src/soc/intel/common/block/imc/Kconfig:
https://review.coreboot.org/c/coreboot/+/83320/comment/97eda60a_99d6dc77?usp... : PS3, Line 4: bool I guess you are using an IMC with an SMBUS in it? Would it be possible to be more explicit on naming?
File src/soc/intel/common/block/imc/imc.c:
https://review.coreboot.org/c/coreboot/+/83320/comment/51982270_8abfef5d?usp... : PS3, Line 57: void imc_smbus_spd_init(pci_devfn_t dev) imc_spd_smbus_init?
File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/83320/comment/d3de02c6_6ee8b429?usp... : PS3, Line 10: { Can we not using weak since it would be confusing? IMO, there are 2 implementation, one is the default, one is the IMC SMBUS based.
https://review.coreboot.org/c/coreboot/+/83320/comment/7844c74e_f0c9e8e6?usp... : PS3, Line 96: if (!CONFIG(SOC_INTEL_COMMON_BLOCK_IMC) && blk->addr_map[i] == 0) { I guess SOC_INTEL_COMMON_BLOCK_IMC will provide SPD data not depending on blk->addr_map[i]?