Attention is currently required from: Wonkyu Kim, Ravishankar Sarawadi, Raj Astekar. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63363 )
Change subject: soc/intel/mtl: Do initial Meteor Lake SoC commit till romstage ......................................................................
Patch Set 3:
(20 comments)
File src/soc/intel/meteorlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/63363/comment/9bc5a86c_b8fead2b PS3, Line 80: config MMCONF_BASE_ADDRESS This was correct, the options were renamed
File src/soc/intel/meteorlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/63363/comment/fef522dd_d763a1eb PS3, Line 6: subdirs-y += ../../../cpu/x86/lapic See CB:44228
https://review.coreboot.org/c/coreboot/+/63363/comment/7396225e_3efea21f PS3, Line 7: subdirs-y += ../../../cpu/x86/mtrr See CB:44230
https://review.coreboot.org/c/coreboot/+/63363/comment/52de70e1_79fdab82 PS3, Line 8: subdirs-y += ../../../cpu/x86/tsc See CB:57456
File src/soc/intel/meteorlake/chip.h:
https://review.coreboot.org/c/coreboot/+/63363/comment/effe332b_ec5362e7 PS3, Line 88: uint8_t SataEnable; Other platforms don't need this, the FSP UPD is programmed as follows:
s_cfg->SataEnable = is_devfn_enabled(PCH_DEVFN_SATA);
https://review.coreboot.org/c/coreboot/+/63363/comment/2fa32fc7_286657a1 PS3, Line 133: /* Gfx related */ : enum { : IGD_SM_0MB = 0x00, : IGD_SM_32MB = 0x01, : IGD_SM_64MB = 0x02, : IGD_SM_96MB = 0x03, : IGD_SM_128MB = 0x04, : IGD_SM_160MB = 0x05, : IGD_SM_4MB = 0xF0, : IGD_SM_8MB = 0xF1, : IGD_SM_12MB = 0xF2, : IGD_SM_16MB = 0xF3, : IGD_SM_20MB = 0xF4, : IGD_SM_24MB = 0xF5, : IGD_SM_28MB = 0xF6, : IGD_SM_36MB = 0xF8, : IGD_SM_40MB = 0xF9, : IGD_SM_44MB = 0xFA, : IGD_SM_48MB = 0xFB, : IGD_SM_52MB = 0xFC, : IGD_SM_56MB = 0xFD, : IGD_SM_60MB = 0xFE, : } IgdDvmt50PreAlloc; : uint8_t SkipExtGfxScan; I'm pretty sure this won't be used
https://review.coreboot.org/c/coreboot/+/63363/comment/b081a3f2_6332b4bd PS3, Line 160: uint8_t HeciEnabled; This is no longer used, see https://review.coreboot.org/q/topic:retire_hecienabled
https://review.coreboot.org/c/coreboot/+/63363/comment/63100857_5ceb830d PS3, Line 167: uint8_t PmTimerDisabled; This doesn't seem to be used
https://review.coreboot.org/c/coreboot/+/63363/comment/6f894473_1c846bbb PS3, Line 219: ADL Hmmmmmmmmm...
https://review.coreboot.org/c/coreboot/+/63363/comment/194ab8cf_6d8f7b57 PS3, Line 293: IsaSerialUartBase This shouldn't be a devicetree option, the correct value can be determined from Kconfig options. ADL always uses 3F8 anyway.
File src/soc/intel/meteorlake/espi.c:
https://review.coreboot.org/c/coreboot/+/63363/comment/9dc9d337_d03033f9 PS3, Line 3: nit: double empty line
File src/soc/intel/meteorlake/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/63363/comment/06d98ce6_f0b83786 PS3, Line 8: #define MSR_VR_MISC_CONFIG2 0x636 Does this MSR actually exist? Looks like it has been copy-pasted from Haswell/Broadwell code...
File src/soc/intel/meteorlake/include/soc/p2sb.h:
https://review.coreboot.org/c/coreboot/+/63363/comment/efae6a96_4d0f794a PS3, Line 3: /* : * This file is created based on Intel Alder Lake Processor PCH Datasheet : * Document number: 621483 : * Chapter number: 3 : */ : : #ifndef _SOC_ALDERLAKE_P2SB_H_ : #define _SOC_ALDERLAKE_P2SB_H_ Hmmmmmmmmm...
File src/soc/intel/meteorlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/63363/comment/df0b1ce9_f323a9f3 PS3, Line 10: mainboard_update_premem_soc_chip_config How is this supposed to work? The devicetree is read-only in pre-mem stages.
File src/soc/intel/meteorlake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/63363/comment/3feaf790_9a9e6c26 PS3, Line 3: nit: double blank line
https://review.coreboot.org/c/coreboot/+/63363/comment/c85eb49b_1eda2cd3 PS3, Line 12: #define DMIBAR 0x68 I thought MTL doesn't have DMI?
File src/soc/intel/meteorlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/63363/comment/d8be46cd_7687abcb PS3, Line 31: trailing whitespace
https://review.coreboot.org/c/coreboot/+/63363/comment/c6d254a6_48749d7a PS3, Line 73: m_cfg->CpuPcieRpEnableMask = 0; Always 0?
https://review.coreboot.org/c/coreboot/+/63363/comment/9631e134_feb87539 PS3, Line 229: trailing whitespace
File src/soc/intel/meteorlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/63363/comment/2c8cc77d_ebdedb71 PS3, Line 129: /* FIXME: Reset Heci interface : * This is a W/A added until heci_reset is included to heci_init : */ : heci_reset(); Is this still needed?