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 10:
(10 comments)
Rebased to upstream tip: e4d6c033fe (origin/master, origin/HEAD) Doc/mb/lenovo: Shrink picture for x301
Tested on TiogaPass server without regression observed.
https://review.coreboot.org/c/coreboot/+/38548/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38548/10//COMMIT_MSG@13 PS10, Line 13: that will not be shared in public, at least for the time being.
please mention that SMM isn't working right now
Done
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... File src/soc/intel/skylake_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... PS10, Line 18: #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 <bootstate.h> : #include <string.h> : #include <cf9_reset.h> : #include <cpu/intel/turbo.h> : #include <cpu/x86/msr.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/soc_util.h> : #include <soc/hob_iiouds.h> : #include <soc/hob_memmap.h> : #include "chip.h"
Are you really using all of those includes ? […]
Done
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... PS10, Line 240:
Please use 'Device ()' instead of 'Processor ()' […]
The code I added/updated does not use Processor, actually with this patchset, there is no processor/socket devices declared in ACPI. Such is a to-do item noted. Right now the general code src/cpu/intel/common/acpi/cpu.asl declares _PR, which should be changed. Your commit added a comment, a step forward would be to update the common code base to move away from Processor ().
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... PS10, Line 943: void acpigen_resource_word(u16 res_type, u16 gen_flags, u16 type_flags, u16 gran,
please move to acpigen. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... PS10, Line 988: void acpigen_emit_qword(u64 data)
please move to acpigen. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/a... PS10, Line 995: void acpigen_resource_qword(u16 res_type, u16 gen_flags, u16 type_flags,
please move to acpigen. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... File src/soc/intel/skylake_sp/chip.c:
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... PS10, Line 88: static void skxsp_pci_dev_iterator(struct bus *bus,
it looks like some of those functions are copied from device/pci_* . […]
I hope not to make the scope of initial commit bigger. That being said, let's discuss the details. Do you have a proposal in mind?
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... PS10, Line 756: //fast_spi_init();
is this still needed?
Ditto!
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... File src/soc/intel/skylake_sp/cpu.c:
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... PS10, Line 225: * 1. Prevent race condition in MTRR solution. Enable MTRRs on the BSP. This
whats 2. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/10/src/soc/intel/skylake_sp/c... PS10, Line 234: /* FSP takes care of MTRR settings */
is this still true?
Done