Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38548 )
Change subject: Add Intel SkyLake Scalable Processor support ......................................................................
Patch Set 5:
(14 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
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@7 PS5, Line 7: Add Intel SkyLake Scalable Processor support Please use a prefix:
soc/intel: Add Skylake Scalable Processor support
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@13 PS5, Line 13: Remove.
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 everybody) in the tree.
https://review.coreboot.org/c/coreboot/+/38548/5//COMMIT_MSG@14 PS5, Line 14: Tested how?
Please elaborate, why a new directory is needed, and it cannot be supported by extending the existing Skylake code.
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.
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... File src/soc/intel/skylake_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... PS5, Line 4: . Remove.
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 5: . Remove.
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.
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... File src/soc/intel/skylake_sp/acpi/iiostack.asl:
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... PS5, Line 17: #define MAKE_IIO_DEV(id,rt) \ Why add the blank lines?
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 6: . Remove.
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.
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... PS5, Line 70: #define MAKE_LINK_DEV(id,uid) \ Please align the last .
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... File src/soc/intel/skylake_sp/acpi/uncore.asl:
https://review.coreboot.org/c/coreboot/+/38548/5/src/soc/intel/skylake_sp/ac... PS5, Line 4: . Remove.