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, TangYiwei, Tim Chu.
Shuo Liu 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 42:
(6 comments)
File src/soc/intel/xeon_sp/gnr/Kconfig:
https://review.coreboot.org/c/coreboot/+/81316/comment/1a306075_3db3e197 : PS42, Line 113: config INTEL_TXT_SINIT_SIZE : hex : default 0x50000 : help : According to document number 572782 this needs to be 256KiB : for the SINIT module and 64KiB for SINIT data. : : config INTEL_TXT_HEAP_SIZE : hex : default 0xf0000 : help : This must be 960KiB according to 572782.
Is CBnT/TXT validated? If not I'd just remove this as it might give a false impression.
Done
File src/soc/intel/xeon_sp/gnr/chip.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/1d213ab7_e80b08b0 : PS42, Line 48: cpu_bus_ops
can also be hooked up directly in devicetree.
Done
https://review.coreboot.org/c/coreboot/+/81316/comment/f8fd2ac0_032e9751 : PS42, Line 55: dev->ops = &hpet_device_ops;
I'd recommend a chipset.cb file which hooks up this ops.
Done
https://review.coreboot.org/c/coreboot/+/81316/comment/093de309_b8767f90 : PS42, Line 84: .final = chip_final,
Why have a function pointer to nothing?
Done
File src/soc/intel/xeon_sp/gnr/romstage.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/eba617e0_336e50fb : PS42, Line 20: void __weak mainboard_memory_init_params(FSPM_UPD *mupd) : { : /* Default weak implementation */ : }
Is doing nothing valid on any board? If not I'd rather have the build fails and make it mandatory fo […]
Done
File src/soc/intel/xeon_sp/gnr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/999c2db7_8f006d6f : PS42, Line 47: nsigned 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; : }
I don't see a call to this (neither on SPR-SP). […]
Done