Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 support ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... PS4, Line 75: spd_addr_table So, SPD read over SMBUS is the only option for DDR4? Can't a board have SPD in CBFS? Or just provide a memptr to SPD? See enum on line 28.
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 160: /* Set SpdAddressTable to 0 when using MemorySpdPtr */ : mem_cfg->SpdAddressTable[0] = 0x0; : mem_cfg->SpdAddressTable[1] = 0x0; : mem_cfg->SpdAddressTable[2] = 0x0; : mem_cfg->SpdAddressTable[3] = 0x0; Just use memset?
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 175: blank line not required.
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 177: mem_cfg->SpdAddressTable[0] = mem_cfg_data->spd_addr_table[0]; : mem_cfg->SpdAddressTable[1] = mem_cfg_data->spd_addr_table[1]; : mem_cfg->SpdAddressTable[2] = mem_cfg_data->spd_addr_table[2]; : mem_cfg->SpdAddressTable[3] = mem_cfg_data->spd_addr_table[3]; Can we just use get_spd_smbus() to read out the SPD in coreboot and pass that using MemorySpdPtr00, etc.?