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 27:
(3 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 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;
Ack
Why is EmmcUseCustomDlls required? We can just configure the EMMC DLL params in coreboot using DLL params in soc_intel_common, right?
https://review.coreboot.org/c/coreboot/+/37783/22/src/soc/intel/tigerlake/ch... PS22, Line 145: SdCardPowerEnableActiveHigh
Ack
My point is that there is no code under src/soc/intel/tigerlake which actually uses this config. Ideally, we should add both the chip config and its corresponding code in one CL so that its use can be reviewed. It also avoids having a lot of stale entries in chip.h which is what this file had initially started with for tigerlake.
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
Ack
Apologize for the confusion. Gerrit was initially showing this function as something new that you were adding. Hence I made the comment to remove it from this CL. I probably had my diff view messed up. Can you please leave this as is? We should not add/delete GPIO PM configuration as part of this change. It should be handled separately.