Johnny Lin 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 41:
(11 comments)
https://review.coreboot.org/c/coreboot/+/38548/40/src/soc/intel/xeon_sp/acpi... File src/soc/intel/xeon_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/38548/40/src/soc/intel/xeon_sp/acpi... PS40, Line 137: size_t hob_size;
Iniitialize to zero
Done
https://review.coreboot.org/c/coreboot/+/38548/40/src/soc/intel/xeon_sp/acpi... PS40, Line 146: /* Local APICs */
formatting issue?
Done
https://review.coreboot.org/c/coreboot/+/38548/40/src/soc/intel/xeon_sp/acpi... PS40, Line 150: hob_size
This value will only get assigned if the GUID is found, but can be left uninitialized if not. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/40/src/soc/intel/xeon_sp/acpi... PS40, Line 160: sizeof(ioapic_ids)/sizeof(int)
ARRAY_SIZE(ioapic_ids) […]
Done
https://review.coreboot.org/c/coreboot/+/38548/40/src/soc/intel/xeon_sp/acpi... PS40, Line 161: sizeof(gsi_bases)/sizeof(int))
ARRAY_SIZE(gsi_bases)
Done
https://review.coreboot.org/c/coreboot/+/38548/40/src/soc/intel/xeon_sp/acpi... PS40, Line 172: sizeof(ioapic_ids)/sizeof(int)
ARRAY_SIZE(ioapic_ids)
Done
https://review.coreboot.org/c/coreboot/+/38548/40/src/soc/intel/xeon_sp/acpi... PS40, Line 173: sizeof(gsi_bases)/sizeof(int)
ARRAY_SIZE(gsi_bases)
Done
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.
Done
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. […]
It calls into the overridden mainboard_memory_init_params() in mb/ocp/tiogapass/romstage.c which is necessary, it would boot hang if I remove it.
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.
Done
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.
Done