Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38548 )
Change subject: soc/intel: Add Intel SkyLake Scalable Processor support ......................................................................
Patch Set 10:
(9 comments)
https://review.coreboot.org/c/coreboot/+/38548/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38548/10//COMMIT_MSG@13 PS10, Line 13: that will not be shared in public, at least for the time being. please mention that SMM isn't working right now
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... PS10, Line 943: void acpigen_resource_word(u16 res_type, u16 gen_flags, u16 type_flags, u16 gran, please move to acpigen.c
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... PS10, Line 988: void acpigen_emit_qword(u64 data) please move to acpigen.c
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... PS10, Line 995: void acpigen_resource_qword(u16 res_type, u16 gen_flags, u16 type_flags, please move to acpigen.c
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... File src/soc/intel/skylake_sp/chip.c:
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... PS10, Line 88: static void skxsp_pci_dev_iterator(struct bus *bus, it looks like some of those functions are copied from device/pci_* . Can you update the shared codebase to work with multiple domains?
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... PS10, Line 578: return ""; that's not correct
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... PS10, Line 756: //fast_spi_init(); is this still needed?
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... File src/soc/intel/skylake_sp/cpu.c:
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... PS10, Line 225: * 1. Prevent race condition in MTRR solution. Enable MTRRs on the BSP. This whats 2.?
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... PS10, Line 234: /* FSP takes care of MTRR settings */ is this still true?