David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38548 )
Change subject: soc/intel: Add Intel Xeon Scalable Processor support ......................................................................
Patch Set 40: Code-Review-1
(4 comments)
just a few more drive-by comments
https://review.coreboot.org/c/coreboot/+/38548/37/src/soc/intel/xeon_sp/roms... File src/soc/intel/xeon_sp/romstage.c:
https://review.coreboot.org/c/coreboot/+/38548/37/src/soc/intel/xeon_sp/roms... PS37, Line 35: mainboard_config_gpios(); No need to call this, mainboard_config_gpios() is an empty function.
https://review.coreboot.org/c/coreboot/+/38548/37/src/soc/intel/xeon_sp/roms... PS37, Line 78: mainboard_memory_init_params(mupd); This doesn't do anything, so it should be removed. I suspect it was leftover from Denverton since in that case we need to worry about memory-down configs without SPDs.
https://review.coreboot.org/c/coreboot/+/38548/37/src/soc/intel/xeon_sp/roms... PS37, Line 85: __weak void mainboard_config_gpios(void) Not needed only called from car_stage_entry() above.
https://review.coreboot.org/c/coreboot/+/38548/37/src/soc/intel/xeon_sp/roms... PS37, Line 89: __weak void mainboard_memory_init_params(FSPM_UPD *mupd) Similar as above, this can be removed.