Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39401 )
Change subject: soc/intel/tigerlake:add support to read spd data from SMBUS ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39401/1/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39401/1/src/soc/intel/tigerlake/mem... PS1, Line 50: memset(&mem_cfg->SpdAddressTable, 0, sizeof(mem_cfg->SpdAddressTable)); two instances not needed, we can keep it as original place, the sbmus would re-initialize it anyway.
https://review.coreboot.org/c/coreboot/+/39401/1/src/soc/intel/tigerlake/mem... PS1, Line 120: spd_data_ptr this is not getting initialized now, meminit_channels would set the invalid values for MemorySpdPtrXX for non SMBUS case.
https://review.coreboot.org/c/coreboot/+/39401/1/src/soc/intel/tigerlake/mem... PS1, Line 122: get_spd_data We can handle the SMBUS case here instead of changing the func to accommodate SMBUS, get_spd_data, should be limited to non SMBUS case, since for SMBUS it is read dynamically:
something like this:
if (spd_info->read_type != READ_SPD_CBFS) { get_spd_data(spd_info, &spd_data_ptr, &spd_data_len); print_spd_info((unsigned char *)spd_data_ptr); mem_cfg->MemorySpdDataLen = spd_data_len; } else { for (int i = 0; i < NUM_DIMM_SLOT; i++) mem_cfg->SpdAddressTable[i]= spd_info->spd_spec.spd_smbus_address[i]; spd_data_ptr = 0; } meminit_channels(mem_cfg, board_cfg, spd_data_ptr, half_populated);