Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38998 )
Change subject: mb/intel/tglrvp: add Tiger Lake memory initialization support ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38998/9/src/mainboard/intel/tglrvp/... File src/mainboard/intel/tglrvp/romstage_fsp_params.c:
https://review.coreboot.org/c/coreboot/+/38998/9/src/mainboard/intel/tglrvp/... PS9, Line 63: mem_cfg->SpdAddressTable[0] = 0xA0; : mem_cfg->SpdAddressTable[1] = 0xA2; : mem_cfg->SpdAddressTable[2] = 0xA4; : mem_cfg->SpdAddressTable[3] = 0xA6; : : const struct rcomp_cfg *rcomp_data = get_rcomp_data(); : mem_cfg->RcompResistor = rcomp_data->rcomp_resistor; : memcpy(mem_cfg->RcompTarget, rcomp_data->rcomp_target, : sizeof(rcomp_data->rcomp_target)); There should be a function like meminit_lpddr4x_dimm0() that is used specifically for DDR4 initialization.
https://review.coreboot.org/c/coreboot/+/38998/9/src/mainboard/intel/tglrvp/... PS9, Line 84: mem_cfg->SpdAddressTable[0] = 0x0; : mem_cfg->SpdAddressTable[1] = 0x0; : mem_cfg->SpdAddressTable[2] = 0x0; : mem_cfg->SpdAddressTable[3] = 0x0; Why is this done here?
https://review.coreboot.org/c/coreboot/+/38998/9/src/mainboard/intel/tglrvp/... File src/mainboard/intel/tglrvp/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/38998/9/src/mainboard/intel/tglrvp/... PS9, Line 31: struct memory_config { : const void *dq_map; : size_t dq_map_size; : const void *dqs_map; : size_t dqs_map_size; : u16 rcomp_resistor; : const void *rcomp_target; : size_t rcomp_target_size; : }; This is basically unused.