Angel Pons 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 16:
(12 comments)
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: SkyLake
Skylake has several SKUs. […]
See latest patchset
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@9 PS5, Line 9: SOC
Yes, it integrates north bridge, and has other uncores, albeit it does not integrate PCH.
Done
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@14 PS5, Line 14:
This is a new SKU. The exsiting Skylake directory is really for Skylake D SKU.
I think "Skylake" is the only thing soc/intel/skylake/ and soc/intel/skylake_sp/ have in common
https://review.coreboot.org/c/coreboot/+/38548/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38548/16//COMMIT_MSG@7 PS16, Line 7: SkyLake The `L` should not be uppercase: Skylake
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/Kconfig File src/soc/intel/Kconfig:
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/Kconfig@12 PS5, Line 12: source "src/soc/intel/skylake_sp/Kconfig"
Sort.
The list isn't sorted, though.
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... File src/soc/intel/skylake_sp/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... PS5, Line 23: /* Global ACPI memory region. This region is used for passing information
Please use the allowed comment style.
That is:
/* * Global ACPI...
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/a... PS16, Line 53: GPEI, 64, // 0x1D - GPE wake status bit maybe give these a little push with a tab?
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... File src/soc/intel/skylake_sp/acpi/pci_irq.asl:
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... PS5, Line 29: Refer to Intel® C620 Series Chipset Platform Controller Hub section 20.11
Asterisk in the front.
Done
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi/pci_irq.asl:
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/a... PS16, Line 27: Add (PCR_ITSS_PIRQA_ROUT, Looks like indentation became spaces again?
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi/uncore_irq.asl:
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/a... PS16, Line 18: u nit: Uncore
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/c... File src/soc/intel/skylake_sp/chip.c:
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/c... PS16, Line 374: if (!(res->flags & IORESOURCE_BRIDGE) || Can this be moved out into a function? It would avoid so many tabs
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/i... PS16, Line 24: Skylake Lewisburg?