Change in coreboot[master]: soc/intel/mtl: Do initial Meteor Lake SoC commit till ramstage
Attention is currently required from: Wonkyu Kim, Ravishankar Sarawadi, Raj Astekar. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63364 )
Change subject: soc/intel/mtl: Do initial Meteor Lake SoC commit till ramstage ......................................................................
Patch Set 2:
(25 comments)
File src/soc/intel/meteorlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/63364/comment/41244bff_3b943dc7 PS2, Line 37: select REG_SCRIPT Shouldn't be needed
https://review.coreboot.org/c/coreboot/+/63364/comment/e7913c0a_76a85731 PS2, Line 73: select UDK_202005_BINDING : select DISPLAY_FSP_VERSION_INFO : select HECI_DISABLE_USING_SMM : # select DEBUG_GPIO nit: Order these alphabetically as well?
File src/soc/intel/meteorlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/86d9e634_0131aff2 PS2, Line 134: memcpy(&map[i], &cstate_map[set[i]], sizeof(acpi_cstate_t)); map[i] = cstate_map[set[i]];
File src/soc/intel/meteorlake/chip.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/6589e7f4_6337f957 PS2, Line 21: const char *soc_acpi_name(const struct device *dev) Not all devices are listed here, is it intentional?
File src/soc/intel/meteorlake/chipset.cb:
https://review.coreboot.org/c/coreboot/+/63364/comment/545755f2_aec4d790 PS2, Line 75: device pci 13.0 alias p2sb2 off end : device pci 13.2 alias pmc2 hidden end : device pci 13.3 alias shared_sram2 off end Given that these devices are for IOE, I'd name them as follows:
device pci 13.0 alias ioe_p2sb off end device pci 13.2 alias ioe_pmc hidden end device pci 13.3 alias ioe_shared_sram off end
File src/soc/intel/meteorlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/3400e52b_6e1b68e9 PS2, Line 171: /* : * enable_x2apic () is temporary WA for fixing S3 resume hang issue : * Once formal patch merge, this WA can be removed: : * formal patch: https://review.coreboot.org/c/coreboot/+/55262 : */ It's merged already?
File src/soc/intel/meteorlake/crashlog.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/0382a1a7_a3098213 PS2, Line 3: UPDATEME What needs to be updated?
File src/soc/intel/meteorlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/4016622a_290898f7 PS2, Line 23: static void pch_handle_sideband(config_t *config) : { : : } Is this needed?
https://review.coreboot.org/c/coreboot/+/63364/comment/168850f2_1ef816bf PS2, Line 44: if (config->PmTimerDisabled) : pmc_disable_acpi_timer(); This seems to have been removed from other platforms.
File src/soc/intel/meteorlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/c58764f9_b4fc5e62 PS2, Line 173: /* Chipset Lockdown */ : if (get_lockdown_config() == CHIPSET_LOCKDOWN_COREBOOT) { : s_cfg->PchLockDownGlobalSmi = 0; : s_cfg->PchLockDownBiosInterface = 0; : s_cfg->PchUnlockGpioPads = 1; : s_cfg->RtcMemoryLock = 0; : } else { : s_cfg->PchLockDownGlobalSmi = 1; : s_cfg->PchLockDownBiosInterface = 1; : s_cfg->PchUnlockGpioPads = 0; : s_cfg->RtcMemoryLock = 1; : } This can be simplified, see ADL code
https://review.coreboot.org/c/coreboot/+/63364/comment/08b5e616_d4a0c82f PS2, Line 285: nit: double blank line
https://review.coreboot.org/c/coreboot/+/63364/comment/1e27b252_370f1ce4 PS2, Line 323: nit: double blank line
https://review.coreboot.org/c/coreboot/+/63364/comment/4ad42c99_d9a3d62d PS2, Line 417: nit: double blank line
https://review.coreboot.org/c/coreboot/+/63364/comment/a096d067_121ab6a4 PS2, Line 426: nit: double blank line
File src/soc/intel/meteorlake/include/soc/crashlog.h:
https://review.coreboot.org/c/coreboot/+/63364/comment/ba213f60_0e4a63e7 PS2, Line 8: nit: double blank line
File src/soc/intel/meteorlake/include/soc/meminit.h:
PS2: Shouldn't this be part of the romstage commit?
File src/soc/intel/meteorlake/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/fde0e1c2_35a282f1 PS2, Line 11: { .slot = PCI_DEV_SLOT_PCIE_2, .count = 1 }, I'm pretty sure root port function swap only applies to PCIE_1.
https://review.coreboot.org/c/coreboot/+/63364/comment/9b6af33e_0356ee3e PS2, Line 17: { .slot = PCI_DEV_SLOT_PCIE_2, .count = 3 }, : { .slot = PCI_DEV_SLOT_PCIE_3, .count = 1 }, Ditto. If PCIE_2 works as it does on older platforms, the multiple functions come from bifurcation (x16, x8 + x8, x8 + x4 + x4) and function 0 always exists.
File src/soc/intel/meteorlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/8ea5f997_7cdc9e12 PS2, Line 104: Alder Hmmmmmmmmmm...
https://review.coreboot.org/c/coreboot/+/63364/comment/6b4b0075_c221f5e0 PS2, Line 122: if (CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_PEP)) { Indentation looks off, it should be one tab.
File src/soc/intel/meteorlake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/f1ec1bfa_fbb6c2e5 PS2, Line 210: * S5 because the PCH does not set the WAK_STS bit when waking : * from a true G3 state. : */ nit: add one space before the asterisks
File src/soc/intel/meteorlake/smihandler.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/25cc6366_a300f154 PS2, Line 14: ADP Hmmmmmmmmm...
File src/soc/intel/meteorlake/soc_info.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/77bea7fa_1f18ce46 PS2, Line 14: #ifdef __SIMPLE_DEVICE__ : did = pci_read_config16(PCI_DEVFN_ROOT, PCI_DEVICE_ID); : #else : struct device *dev; : dev = pcidev_path_on_root(PCI_DEVFN_ROOT); : did = pci_read_config16(dev, PCI_DEVICE_ID); : #endif Instead of doing this, why not simply #define __SIMPLE_DEVICE__ at the start of the file (before the includes)? Then, you can simply do:
did = pci_read_config16(PCI_DEVFN_ROOT, PCI_DEVICE_ID);
https://review.coreboot.org/c/coreboot/+/63364/comment/3f26730a_88325d1f PS2, Line 26: Trailing whitespace
File src/soc/intel/meteorlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/63364/comment/fb05fb72_12e422c7 PS2, Line 91: uint32_t soc_systemagent_max_chan_capacity_mib(u8 capid0_a_ddrsz) Does this still apply?
-
Angel Pons (Code Review)