Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37783 )
Change subject: soc/intel/tigerlake: Update chip files ......................................................................
Patch Set 22:
(9 comments)
https://review.coreboot.org/c/coreboot/+/37783/22/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/37783/22/src/soc/intel/tigerlake/ch... PS22, Line 39: GPP_[A:G] or GPD This is not correct. These values need to be of the form PMC_GPP_* because they are directly configured in PMC GPIO_CFG register. Also, [A:G] is not correct. Please see latest revision here: https://review.coreboot.org/c/coreboot/+/37427/16/src/soc/intel/tigerlake/in...
https://review.coreboot.org/c/coreboot/+/37783/22/src/soc/intel/tigerlake/ch... PS22, Line 40: gpe0_dw0 Can we please name these pmc_gpe0_dw0 and so on. GPE0_DW0 values for groups in PMC GPIO_CFG and GPIO MISCCFG are very different and so it is important to name this properly.
https://review.coreboot.org/c/coreboot/+/37783/22/src/soc/intel/tigerlake/ch... PS22, Line 135: /* Need to update DLL setting to get Emmc running at HS400 speed */ : uint8_t EmmcUseCustomDlls; : uint32_t EmmcTxCmdDelayRegValue; : uint32_t EmmcTxDataDelay1RegValue; : uint32_t EmmcTxDataDelay2RegValue; : uint32_t EmmcRxCmdDataDelay1RegValue; : uint32_t EmmcRxCmdDataDelay2RegValue; : uint32_t EmmcRxStrobeDelayRegValue; soc_intel_common_config on line 36 already includes mmc_dll_params which has the same params as these. I don't think this needs to be added again.
https://review.coreboot.org/c/coreboot/+/37783/22/src/soc/intel/tigerlake/ch... PS22, Line 145: SdCardPowerEnableActiveHigh Can we please add this when/if really required? Currently, all these params are being added which are not even used in code.
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 70: PlatformMemorySize
Done
But, I don't see any code actually utilizing this.
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 73: TsegSize
Done
FSP UPD is set to CONFIG_SMM_TSEG_SIZE. So, this should not be required.
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 123: CONFIG_MAX_ROOT_PORTS
Done
As far as I can see, this is the same for TGL and JSL. So, using CONFIG_MAX_ROOT_PORTS is not correct here.
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 126: CONFIG_MAX_ROOT_PORTS
Done
Same here.
https://review.coreboot.org/c/coreboot/+/37783/22/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/37783/22/src/soc/intel/tigerlake/ch... PS22, Line 124: soc_fill_gpio_pm_configuration Can you please move this change to a CL of its own. On CNL/WHL/CML, it was identified that the GPIO PM configuration needs to be done in two places. Let's add this and the gpio_override_pm and gpio_pm in chip.h in a separate CL.