Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38548 )
Change subject: Add Intel SkyLake Scalable Processor support ......................................................................
Patch Set 6:
(9 comments)
Thanks for the review!!
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@7 PS5, Line 7: Add Intel SkyLake Scalable Processor support
Please use a prefix: […]
Done
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@7 PS5, Line 7: SkyLake
Skylake
Skylake has several SKUs. This commits enables Skylake-SP (SP is short for Scalable Processor) which is a server SKU.
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@9 PS5, Line 9: SOC
Really? Skylake-SP is a SoC?
Yes, it integrates north bridge, and has other uncores, albeit it does not integrate PCH.
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@13 PS5, Line 13: that will not be shared in public, at least for the time being.
Please announce that to the mailing list, and ask, if we want such a non-working support (for everyb […]
This is first time for coreboot to support Xeon-SP. So it helps to get this initial support in, and we will optimize overtime (such as adding support for other processors of Xeon-SP family).
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@14 PS5, Line 14:
Tested how? […]
This is a new SKU. The exsiting Skylake directory is really for Skylake D SKU.
https://review.coreboot.org/c/coreboot/+/38548/6/src/soc/intel/skylake_sp/ac... File src/soc/intel/skylake_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/38548/6/src/soc/intel/skylake_sp/ac... PS6, Line 159: int gsi_bases[] = {0, 0x18, 0x20, 0x28, 0x30, 0x48, 0x50, 0x58, 0x60};
Space after {?
Done
https://review.coreboot.org/c/coreboot/+/38548/6/src/soc/intel/skylake_sp/ac... File src/soc/intel/skylake_sp/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/38548/6/src/soc/intel/skylake_sp/ac... PS6, Line 52: PM1I, 64, // 0x15 - PM1 wake status bit
Align?
Done
https://review.coreboot.org/c/coreboot/+/38548/6/src/soc/intel/skylake_sp/ac... File src/soc/intel/skylake_sp/acpi/pci_irq.asl:
https://review.coreboot.org/c/coreboot/+/38548/6/src/soc/intel/skylake_sp/ac... PS6, Line 25: * https://bugs.acpica.org/show_bug.cgi?id=1201
From the report: […]
Done
https://review.coreboot.org/c/coreboot/+/38548/6/src/soc/intel/skylake_sp/ac... PS6, Line 83: IRQ (Level, ActiveLow, Shared) {} \
Align.
Done