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 6:
(23 comments)
https://review.coreboot.org/c/coreboot/+/37783/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37783/6//COMMIT_MSG@11 PS6, Line 11: update soc_intel_tigerlake_config struct What is the motivation behind this update? I see some of the configs being dropped from this struct, but those configs either in the current or some updated form would be required eventually. Do you plan to add those later when required?
Also, I see some configs which are not used or might not ever get used for TGL, but are still left as is. What is the plan w.r.t. those configs?
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 119: Audio related Some/all of these configs are going to be used eventually for enabling audio. Do you plan to add them back later on?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 257: gpio_override_pm Why are these removed? These are required on TGL as well.
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 39: GPP_[A:G] Is this correct?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 70: PlatformMemorySize Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 71: SmramMask Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 72: MrcFastBoot Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 73: TsegSize Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 74: MmioSize Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 78: DdrFreqLimit Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 82: FreqSaGvLow Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 86: FreqSaGvMid Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 92: SaGv_Disabled, : SaGv_FixedLow, : SaGv_FixedMid, : SaGv_FixedHigh, : SaGv_Enabled, Are these correct for TGL?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 105: SsicPortEnable Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 123: CONFIG_MAX_ROOT_PORTS Is this really correct?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 126: CONFIG_MAX_ROOT_PORTS Is this really correct?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 143: GpioIrqRoute Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 145: SciIrqSelect Is this used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 147: uint8_t TcoIrqSelect; : uint8_t TcoIrqEnable; : Are these used?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 157: /* Enable VR specific mailbox command : * 00b - no VR specific cmd sent : * 01b - VR mailbox cmd specifically for the MPS IMPV8 VR will be sent : * 10b - VR specific cmd sent for PS4 exit issue : * 11b - Reserved */ : uint8_t SendVrMbxCmd; Is this applicable for TGL?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 180: ProbeTypeDisable = 0x00, Why is 0x01 missing?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 187: ProbeTypeMax Do you want ProbeTypeMax to be ProbeTypeManual?
https://review.coreboot.org/c/coreboot/+/37783/6/src/soc/intel/tigerlake/chi... PS6, Line 221: PLATFORM_POR, : FORCE_ENABLE, : FORCE_DISABLE Is this correct?