Jonathan Zhang 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:
(8 comments)
Thanks. Updated version of patch set pushed.
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"
The list isn't sorted, though.
Done
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/38548/16/src/soc/intel/skylake_sp/a... PS16, Line 17: #include <assert.h> : #include <arch/acpi.h> : #include <arch/cpu.h> : #include <arch/acpigen.h> : #include <arch/ioapic.h> : #include <arch/smp/mpspec.h> : #include <string.h> : #include <cpu/intel/turbo.h> : #include <cpu/x86/smm.h> : #include <intelblocks/acpi.h> : #include <intelblocks/msr.h> : #include <intelblocks/pmclib.h> : #include <device/pci.h> : #include <cbmem.h> : #include <soc/acpi.h> : #include <soc/cpu.h> : #include <soc/soc_util.h> : #include <soc/pm.h> : #include <soc/pmc.h> : #include <soc/pci_devs.h> : #include <soc/hob_iiouds.h> : #include <soc/hob_memmap.h> : #include "chip.h"
are you sure that you use all of those includes? […]
"chip.h" is needed. I have checked other files.
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.
Done
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
That is: […]
Done
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?
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?
Yes, I am using spaces in ASL files to be consistent.
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
Done
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?
This refers to chipset. Let me update the comment more clearly.