Subrata Banik 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:
(24 comments)
https://review.coreboot.org/c/coreboot/+/45192/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45192/2//COMMIT_MSG@10 PS2, Line 10: bootblock
romstage?
Ack
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
Yep, a comment would be nice (b/c they're not still part of 0:0. […]
Ack
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?
Ack
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 readabil […]
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 35: BOARD_TYPE_ULT_ULX
This would need to change for ADL-S, I guess? At least, I'd leave a comment: […]
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 43: msr_t flex_ratio; : flex_ratio = rdmsr(MSR_FLEX_RATIO); : m_cfg->CpuRatio = (flex_ratio.lo >> 8) & 0xff;
nit: This can be a single line: […]
Ack
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); : }
I would keep them, though: the body of the loop consists of multiple lines
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 69: 0x0
AFAIUI, this is the LPSS UART I/O base address, and is set to zero in order to avoid conflicts when […]
some details here Angel https://github.com/coreboot/coreboot/blob/master/src/vendorcode/intel/fsp/fs...
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 98: /* Channel Hash Mask:0x0001=BIT6 set(Minimal), 0x3FFF=BIT[19:6] set(Maximum) */
So bits [13:0] of this number correspond to bits [19:6] of the channel hash mask?
yes
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 101: config->SmbusEnable
See CB:43845 as an example
Ack
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
Ack
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); […]
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 151: ENABLE_VMX
Note that this is only available when CPU_INTEL_COMMON is selected
thanks Angel
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 157: dev && dev->enabled
is_dev_enabled(dev)
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 8: E
nit: no need to capitalize the `E`
Ack
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 24: mainboard_get_dram_part_num
You could make this function return a part_num pointer. […]
Ack
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 []
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 42: FSP_SMBIOS_MEMORY_INFO_GUID
fits on the previous line
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 78: channel++) {
nit: fits on the previous line
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 84: dimm++) {
nit: fits on the previous line
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 98: u8
uint8_t
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 100: SPD_SAVE_OFFSET_SERIAL;
nit: fits on the previous line
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 133: Program
nit: `Perform`?
Ack
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 135: initialize Heci
Capitalization looks odd, I'd use `Initialize HECI`
Ack