Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 )
Change subject: soc/intel/tigerlake: Do initial SoC commit ......................................................................
Patch Set 10:
(17 comments)
I appreciate the effort to fix some issues in other intel/soc targets before moving forward with TGL. It might be a bit easier to track changes if this commit was split up in smaller changes. e.g. split ACPI, bootblock, romstage, ramstage callbacks / PCI drivers, ... There is no mainboard target here so for buildtests it's mostly fine to have one mainboard on top of the soc CL. If one smaller change is ready it can get +2 and get merged independently from other changes. Also coreboot is a moving target so if something changes only a smaller CL needs updating on a rebase vs the big all in one change.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Kc... PS10, Line 73: FSP_USES_CB_STACK PLATFORM_USES_FSP2_1 indirectly selects this. You might want to drop the dependency.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Kc... PS10, Line 215: ENABLE_DISPLAY_OVER_EXT_PCIE_GFX This seems to (partially) reinvent CONFIG_ONBOARD_VGA_IS_PRIMARY. Could you get rid of it?
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Ma... File src/soc/intel/tigerlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Ma... PS10, Line 1: feq ($(CONFIG_SOC_INTEL_TIGERLAKE),y) There is now an 'all' class to link code in bootblock,verstage,romstage,postcar,ramstage. It could reduce some lines here.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/bo... File src/soc/intel/tigerlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/bo... PS10, Line 29: : static struct { : u32 cpuid; : const char *name; : } cpu_table[] = { : { CPUID_ICELAKE_A0, "Icelake A0" }, : { CPUID_ICELAKE_B0, "Icelake B0" }, : }; : : static struct { : u16 mchid; : const char *name; : } mch_table[] = { : { PCI_DEVICE_ID_INTEL_ICL_ID_U, "Icelake-U" }, : { PCI_DEVICE_ID_INTEL_ICL_ID_U_2_2, "Icelake-U-2-2" }, : { PCI_DEVICE_ID_INTEL_ICL_ID_Y, "Icelake-Y" }, : { PCI_DEVICE_ID_INTEL_ICL_ID_Y_2, "Icelake-Y-2" }, : }; : : static struct { : u16 espiid; : const char *name; : } pch_table[] = { : { PCI_DEVICE_ID_INTEL_ICL_BASE_U_ESPI, "Icelake-U Base" }, : { PCI_DEVICE_ID_INTEL_ICL_BASE_Y_ESPI, "Icelake-Y Base" }, : { PCI_DEVICE_ID_INTEL_ICL_U_PREMIUM_ESPI, "Icelake-U Premium" }, : { PCI_DEVICE_ID_INTEL_ICL_U_SUPER_U_ESPI, "Icelake-U Super" }, : { PCI_DEVICE_ID_INTEL_ICL_U_SUPER_U_ESPI_REV0, "Icelake-U Super REV0" }, : { PCI_DEVICE_ID_INTEL_ICL_SUPER_Y_ESPI, "Icelake-Y Super" }, : { PCI_DEVICE_ID_INTEL_ICL_Y_PREMIUM_ESPI, "Icelake-Y Premium" }, : }; : : static struct { : u16 igdid; : const char *name; : } igd_table[] = { : { PCI_DEVICE_ID_INTEL_ICL_GT0_ULT, "Icelake ULT GT0" }, : { PCI_DEVICE_ID_INTEL_ICL_GT0_5_ULT, "Icelake ULT GT0.5" }, : { PCI_DEVICE_ID_INTEL_ICL_GT1_ULT, "Icelake U GT1" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_0, "Icelake Y GT2" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_1, "Icelake Y GT2_1" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_1, "Icelake U GT2_1" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_2, "Icelake Y GT2_2" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_2, "Icelake U GT2_2" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_3, "Icelake Y GT2_3" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_3, "Icelake U GT2_3" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_4, "Icelake Y GT2_4" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_4, "Icelake U GT2_4" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_5, "Icelake Y GT2_5" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_5, "Icelake U GT2_5" }, : { PCI_DEVICE_ID_INTEL_ICL_GT3_ULT, "Icelake U GT3" }, Does this need updating?
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/bo... PS10, Line 96: cpu_string[50], This seems to be only changed in the case that cpuidr.eax < 0x80000004. Change this to const char cpu_not_found = "Platform info not available" and update cpu_name accordingly.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/bo... PS10, Line 121: cpu_name[0] == ' ' Check for bounds.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/fi... File src/soc/intel/tigerlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/fi... PS10, Line 118: : BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, soc_finalize, NULL); : BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_LOAD, BS_ON_EXIT, soc_finalize, NULL); Any reason to not make this a '.final' op somewhere? The BOOT_STATE_ macro's should not be used too much.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/fs... PS10, Line 42: __weak I'd not make this callback weak. You this to fail at compiletime if not implemented in the mainboard.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/gr... File src/soc/intel/tigerlake/graphics.c:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/gr... PS10, Line 53: /* : * GFX PEIM module inside FSP binary is taking care of graphics : * initialization based on INTEL_GMA_ADD_VBT Kconfig : * option and input VBT file. Hence no need to load/execute legacy VGA : * OpROM in order to initialize GFX. : * : * In case of non-FSP solution, SoC need to select VGA_ROM_RUN : * Kconfig to perform GFX initialization through VGA OpRom. : */ : if (CONFIG(INTEL_GMA_ADD_VBT)) : return; This seems like a bad idea to use a Kconfig option to do something that it is not meant for. Please guard this differently. Don't assume that adding VBT automatically means running the GFX PEIM inside FSP.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/bootblock.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 18: : #include <intelblocks/systemagent.h> Don't include it here but in the file implementing the callbacks below.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 18: : #include <device/device.h> : #include <intelblocks/msr.h> unused here. Include this in the file defining the function.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 21: : /* Latency times in units of 32768ns */ : #define C_STATE_LATENCY_CONTROL_0_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_1_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_2_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_3_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_4_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_5_LIMIT 0x9d : : /* Power in units of mW */ : #define C1_POWER 0x3e8 : #define C6_POWER 0x15e : #define C7_POWER 0xc8 : #define C8_POWER 0xc8 : #define C9_POWER 0xc8 : #define C10_POWER 0xc8 : : /* Common Timer Copy (CTC) frequency - 38.4MHz. */ : #define CTC_FREQ 38400000 : : #define C_STATE_LATENCY_MICRO_SECONDS(limit, base) \ : (((1 << ((base)*5)) * (limit)) / 1000) : #define C_STATE_LATENCY_FROM_LAT_REG(reg) \ : C_STATE_LATENCY_MICRO_SECONDS(C_STATE_LATENCY_CONTROL_ ##reg## _LIMIT, \ : (IRTL_1024_NS >> 10)) Is this used in multiple files? If not better move this inside the compilation unit defining the function.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/ebda.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 21: struct ebda_config { : uint32_t signature; /* 0x00 - EBDA signature */ : uint32_t tolum_base; /* 0x04 - coreboot memory start */ : uint32_t reserved_mem_size; /* 0x08 - chipset reserved memory size */ : }; note: we might get rid of this soon: CB:36362
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/espi.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 19: #include <stdint.h> not needed here?
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 19: #include <soc/gpio_defs.h> : #include <intelblocks/gpio.h> : : #define CROS_GPIO_DEVICE_NAME "INT3455:00" Is this file needed? Can headers be included directly?
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/ro... PS10, Line 26: __weak Try to avoid __weak functions like that. It is often more valuable for builds to fail than to have something that is necessarily wrong build.
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/sm... File src/soc/intel/tigerlake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/sm... PS10, Line 180: tseg_base You should check here that TSEG_BASE is aligned to TSEG_SIZE. It often causes weird problems if that is not the case.