Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37928 )
Change subject: mb/intel/tglrvp: Add correct memory SPD settings ......................................................................
Patch Set 5:
(4 comments)
Sorry, didn't notice until I was done reviewing that the CL is marked Work In Progress until after I finished adding some review comments. I thought I would submit the comments anyway in case it helps / points out a case that you didn't know about or aren't currently working on changing. Next time I will wait until CL is no longer marked "Work in Progress" before reviewing.
https://review.coreboot.org/c/coreboot/+/37928/5/src/mainboard/intel/tglrvp/... File src/mainboard/intel/tglrvp/romstage_fsp_params.c:
https://review.coreboot.org/c/coreboot/+/37928/5/src/mainboard/intel/tglrvp/... PS5, Line 16: #include <console/console.h> Is this still needed?
https://review.coreboot.org/c/coreboot/+/37928/5/src/mainboard/intel/tglrvp/... PS5, Line 55: static const u16 RcompTarget[] = { 60, 40, 40, 40, 30 }; Are these needed? I was told Tigerlake didn't care about RCompTarget or rcomp_resistor, so volteer doesn't have them.
https://review.coreboot.org/c/coreboot/+/37928/5/src/mainboard/intel/tglrvp/... PS5, Line 71: #if 0 Please remove commented out code.
https://review.coreboot.org/c/coreboot/+/37928/5/src/mainboard/intel/tglrvp/... File src/mainboard/intel/tglrvp/spd/spd.h:
https://review.coreboot.org/c/coreboot/+/37928/5/src/mainboard/intel/tglrvp/... PS5, Line 21: /* SPD index definition should be matched with the order of SPD_SOURCES */ The strap table in tglrvp/spd/Makefile.inc has ID 0 set to empty (https://review.coreboot.org/c/coreboot/+/37928/5/src/mainboard/intel/tglrvp/...), which doesn't match these defines. I do see another SPD_SOURCES being set for tglrvp_up3 in this order. Why does tglrvp define SPD in two places? Does table in tglrvp_up3 override settings in tglrvp/spd/Makefile.inc when building for up3?