Attention is currently required from: Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Held, Jincheng Li, Johnny Lin, Jonathan Zhang, Nico Huber, Patrick Rudolph, Paul Menzel, Shuo Liu, TangYiwei, Tim Chu.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81316?usp=email )
Change subject: soc/intel/xeon_sp: Add Granite Rapids initial codes ......................................................................
Patch Set 45: Code-Review+1
(6 comments)
File src/soc/intel/xeon_sp/gnr/Kconfig:
https://review.coreboot.org/c/coreboot/+/81316/comment/af038d78_3038ea83 : PS45, Line 17: select FSP_SPEC_VIOLATION_XEON_SP_HEAP_WORKAROUND Would be great for this to be fixed in FSP
File src/soc/intel/xeon_sp/gnr/cpu.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/4487ea33_5ab37b91 : PS45, Line 33: return false; Is this correct? I thought firmware (FSP?) switches to untrusted mode at some point.
If the actual implementation is currently unavailable for some reason (e.g. not yet allowed to disclose), I would at least add a FIXME/TODO comment stating that this is currently not implemented.
File src/soc/intel/xeon_sp/gnr/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/6c127e91_10f6c117 : PS45, Line 3: /* TEMPORARY PLACE HOLDER! DO NOT USE! */ Thank you for adding this warning. I imagine this will be revised for GNR in a follow-up
File src/soc/intel/xeon_sp/gnr/soc_util.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/bbbea7fd_ca87b982 : PS45, Line 118: printk(BIOS_DEBUG, : "CXL_NODE_HOB_GUID not found: CXL may not be installed\n"); nitpick: This looks rather odd, it should fit in one line
File src/soc/intel/xeon_sp/spr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/a63b6c51_1e85c5fb : PS45, Line 158: unsigned long xeonsp_acpi_create_madt_lapics(unsigned long current) : { : struct device *cpu; : uint8_t num_cpus = 0; : : for (cpu = all_devices; cpu; cpu = cpu->next) { : if ((cpu->path.type != DEVICE_PATH_APIC) : || (cpu->upstream->dev->path.type != DEVICE_PATH_CPU_CLUSTER)) { : continue; : } : if (!cpu->enabled) : continue; : current = acpi_create_madt_one_lapic(current, num_cpus, cpu->path.apic.apic_id); : num_cpus++; : } : : return current; : } Where did this go?
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/7669b053_77aff71a : PS45, Line 268: if (CONFIG(SOC_INTEL_HAS_CXL)) { Could this change (introducing the `soc_add_dram_resources` function) be split off into a separate commit, please?