Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 support ......................................................................
Patch Set 4:
(6 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 […]
As far as I know this was the only option right now.
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... PS4, Line 87: const struct mb_ddr4x_cfg *mem_cfg_data);
Should fit on one line.
Ack
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?
Ack
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 174: const struct mb_ddr4x_cfg *mem_cfg_data)
Should fit on one line.
Ack
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 175:
blank line not required.
Ack
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, […]
I need look into how to refactor using the smbus library APIs. Can we do that in the next change set ?