Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45192 )
Change subject: soc/intel/alderlake/romstage: Do initial SoC commit till romstage ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/inc... PS2, Line 21: #define BIOS_RESET_CPL 0x5da8 : #define GFXVTBAR 0x5400 : #define EDRAMBAR 0x5408 : #define VTVC0BAR 0x5410 : #define REGBAR 0x5420 : #define IPUVTBAR 0x7880 : #define TBT0BAR 0x7888 : #define TBT1BAR 0x7890 : #define TBT2BAR 0x7898 : #define TBT3BAR 0x78A0
These are MCHBAR offsets, right?
Yep, a comment would be nice (b/c they're not still part of 0:0.0 PCI config space)
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/inc... PS2, Line 36: #define MCH_PKG_POWER_LIMIT_LO 0x59a0 : #define MCH_PKG_POWER_LIMIT_HI 0x59a4 : #define MCH_DDR_POWER_LIMIT_LO 0x58e0 : #define MCH_DDR_POWER_LIMIT_HI 0x58e4 : : #define IMRBASE 0x6A40 : #define IMRLIMIT 0x6A48 also MCHBAR offsets, how about keeping them all together and ordered?
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 31: 0xFE The encoding of this register is bizarre, could we convert these to an enum type for better readability?
enum igd_stolen_mem { IGD_SM_60MB = 0xFE, ... };
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 48: for (i = 0; i < ARRAY_SIZE(config->PcieRpEnable); i++) { : if (config->PcieRpEnable[i]) : mask |= (1 << i); : } { and } not required here
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 109: sizeof(m_cfg->PchHdaAudioLinkDmicEnable)); nit: 1 tab past the `memcpy` is enough
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 123: m_cfg->TcssXhciEn = config->TcssXhciEn; : m_cfg->TcssXdciEn = config->TcssXdciEn; dev = pcidev_path_on_root(SA_DEVFN_TCSS_XHCI); m_cfg->TcssXhciEn = is_dev_enabled(dev); ? same for xdci ?
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 41: 16 nit: sizeof(FSP_SMBIOS_MEMORY_INFO_GUID) or leave as []
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 98: u8 uint8_t