Angel Pons 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: Code-Review+1
(17 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?
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?
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 35: BOARD_TYPE_ULT_ULX This would need to change for ADL-S, I guess? At least, I'd leave a comment:
/* TODO: Make this configurable */
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:
m_cfg->CpuRatio = (rdmsr(MSR_FLEX_RATIO).lo >> 8) & 0xff;
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 using UART from a Super I/O or EC. Am I mistaken?
(I just want to make sure I understand this correctly)
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?
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 101: config->SmbusEnable See CB:43845 as an example
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
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 157: dev && dev->enabled is_dev_enabled(dev)
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`
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. If it's not overriden, then the pointer will be null. This allows us to get rid of `is_dram_part_overridden` and instead check if the pointer itself is null.
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
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 78: channel++) { nit: fits on the previous line
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 84: dimm++) { nit: fits on the previous line
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
https://review.coreboot.org/c/coreboot/+/45192/2/src/soc/intel/alderlake/rom... PS2, Line 133: Program nit: `Perform`?
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`