Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38606 )
Change subject: soc/intel/tigerlake: add memory configuration support ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38606/8/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/38606/8/src/soc/intel/tigerlake/inc... PS8, Line 26: lpddr4x_ nit: I don't think this prefix is required. This can be just named: enum spd_read_type.
https://review.coreboot.org/c/coreboot/+/38606/8/src/soc/intel/tigerlake/inc... PS8, Line 32: lpddr4x_ nit: This structure can be just named struct spd_info
https://review.coreboot.org/c/coreboot/+/38606/8/src/soc/intel/tigerlake/inc... PS8, Line 71: meminit_lpddr4x_dimm0 Just a thought: If struct mb_lpddr4x_config is restricted to contain only dq_map, dqs_map, ect and then if the signature of this function is changed to something like: void meminit_lpddr4x_dimm0(FSP_M_CONFIG *mem_cfg, const struct mb_lpddr4x_cfg *board_cfg, const struct spd_info *spd, bool half_populated)
Then, it will allow you to avoid memcpy()s in the mainboard code and also you can easily make structures static const. Thoughts?