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 15:
(24 comments)
https://review.coreboot.org/c/coreboot/+/38548/15/src/drivers/intel/fsp2_0/K... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/38548/15/src/drivers/intel/fsp2_0/K... PS15, Line 59: SOC_INTEL_SKYLAKE_SP Not yet
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/K... File src/soc/intel/skylake_sp/Kconfig:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/K... PS15, Line 154: Do not change this value This kind of message makes me very curious about things. Sooo, what happens if I change it? Will the server start sprouting daisies or something? :D
On a more serious note, I guess this is for Intel QuickAssist Technology stuff?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/M... File src/soc/intel/skylake_sp/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/M... PS15, Line 23: subdirs-$(CONFIG_HAVE_SMI_HANDLER) += ../../../cpu/x86/smm I'd move this line at the end of the block, as it's the only one that is conditional
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi/iiostack.asl:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... PS15, Line 17: #define MAKE_IIO_DEV(id,rt) \ wait... that's a macro? O_o
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi/pci_irq.asl:
PS15: Indenting could use some more tabs
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... PS15, Line 19: Datasheet? EDS? BIOS spec? BWG?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi/uncore_irq.asl:
PS15: Do these have some sort of pattern, or could be grouped in logical blocks somehow?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/a... PS15, Line 50: Package (0x04) { 0xFFFF, 0x00, LNKA, 0x00 }, Looks odd, is this correct?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/c... File src/soc/intel/skylake_sp/cpu.c:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/c... PS15, Line 147: msr1 Is this intentional?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/c... PS15, Line 305: printk(BIOS_ERR, "MP initialization failure.\n"); Can we survive if MP init failed?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 24: CPUID_SKYLAKESP_A0_A1 nit: add an underscore `_` to split SKYLAKESP:
CPUID_SKYLAKE_SP_A0_A1
(Also applies to the other CPUIDs)
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/gpio_fsp.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 86: (0x1 | (0x1 << 3)), ///< Set pad for both output and input This looks a bit odd
https://review.coreboot.org/c/coreboot/+/38548/15/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/15/src/soc/intel/skylake_sp/i... PS15, Line 24: H Hm?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/hob_iiouds.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 102: PciResourceIoLimit poor things, they are flying away
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 149: CSI Link CSI... Isn't that QPI or UPI nowadays?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/hob_memmap.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 249: SV_HOOKS Is this used?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 23: #define RDMSR(id) \ This could very well be a function returning the read msr
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 27: 08x 08?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/ramstage.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 16: double blank line
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 30: int dimm, int index); should fit in 96 chars
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... File src/soc/intel/skylake_sp/include/soc/soc_config.h:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 16: double blank line
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/i... PS15, Line 37: double blank line
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/s... File src/soc/intel/skylake_sp/smihandler.c:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/s... PS15, Line 24: die("Not implemented"); Do we need to die here?
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/u... File src/soc/intel/skylake_sp/upd_display.c:
https://review.coreboot.org/c/coreboot/+/38548/15/src/soc/intel/skylake_sp/u... PS15, Line 24: old->field, new->field) fits in 96 chars