Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44857 )
Change subject: soc/intel/alderlake/bootblock: Do initial SoC commit till bootblock ......................................................................
Patch Set 2:
(10 comments)
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/Kco... File src/soc/intel/alderlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/Kco... PS2, Line 131: 0xe00 0x1400 is typical
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/boo... File src/soc/intel/alderlake/bootblock/cpu.c:
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/boo... PS2, Line 16: only booting from SPI device "SPI flash"
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/boo... File src/soc/intel/alderlake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/boo... PS2, Line 102: uint32_t dmi_control = pcr_read32(PID_DMI, PCR_DMI_DMICTL); nit: const
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/boo... File src/soc/intel/alderlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/boo... PS2, Line 5: 619362 This is the ADL-S EDS
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/boo... PS2, Line 99: PCI_DEVICE_ID_INTEL_ADL_S_GT1 Would any ADL-P SoC have the ADL-S GT1 IGD ?
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/boo... PS2, Line 136: aes = (cpu_feature_flag & CPUID_AES) ? 1 : 0; : txt = (cpu_feature_flag & CPUID_SMX) ? 1 : 0; : vt = (cpu_feature_flag & CPUID_VMX) ? 1 : 0; suggestion: you could use !!(cpu_feature_flag & CPUID...)
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/espi.h:
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/inc... PS2, Line 5: 621483 this is ADL-S, is there going to be a separate EDS for -P?
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/inc... PS2, Line 12: #include <stdint.h> not used here
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/inc... PS2, Line 65: What about IOM_BASE_ADDRESS ?
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/pcr_ids.h:
https://review.coreboot.org/c/coreboot/+/44857/2/src/soc/intel/alderlake/inc... PS2, Line 14: */ SDCX controller ?