Subrata Banik 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:
(6 comments)
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 215: ENABLE_DISPLAY_OVER_EXT_PCIE_GFX
This seems to (partially) reinvent CONFIG_ONBOARD_VGA_IS_PRIMARY. […]
oh yes, this has added to enable dGPU over PCIE usecase on icelake RVP which might be applicable for TGL as well. let me check if we can remove for now in TGL
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?
Yes, this CL will care of updating
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 '. […]
this is good feedback to handle this in .final = but only problem is that, this file is more miscellaneous IP/controller programming so might not be bonded with any IP. So may be chip.c ops.final i can try to see if we are meeting all requirement correctly. if yes then we can change
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. […]
yes, because its not mandatory to have anyway mainboard override atleast few CRB i have seen where default UPDs are good for mainboard config
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?
i believe this file is kind of wrapper for other mainboard and common code files to get gpio related API. soc/gpio.h is the expectation from other files
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. […]
true.
as i have mentioned in soc weak implementation, its just to make sure incase mainboard doesn't have any override and mainboard_memory_init_params() is calling from fsp driver common code
-
Subrata Banik (Code Review)