Attention is currently required from: 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.
Arthur Heymans 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/a5fad7f4_94d772c1 : 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.
File src/soc/intel/xeon_sp/gnr/chip.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/230d2167_7dc75ffb : PS42, Line 48: cpu_bus_ops can also be hooked up directly in devicetree.
https://review.coreboot.org/c/coreboot/+/81316/comment/071a1816_f99ff297 : PS42, Line 55: dev->ops = &hpet_device_ops; I'd recommend a chipset.cb file which hooks up this ops.
https://review.coreboot.org/c/coreboot/+/81316/comment/73689cd8_b497249e : PS42, Line 84: .final = chip_final, Why have a function pointer to nothing?
File src/soc/intel/xeon_sp/gnr/romstage.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/cf512475_d4b7b201 : 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 for boards to set up, than succeed with invalid setting.
File src/soc/intel/xeon_sp/gnr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/0dc2269a_b61d86d9 : 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). Drop on both?