Attention is currently required from: Angel Pons, Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Held, Jincheng Li, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, 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 57:
(10 comments)
File src/soc/intel/xeon_sp/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/81316/comment/60ce7cef_fcfbd48e : PS56, Line 8: ## GNR IBL codes are initially reused from EBG, will update later.
nit: Add a "TODO: " prefix?
Done
File src/soc/intel/xeon_sp/chip_gen6.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/461fc425_0e836dd1 : PS56, Line 35: static void iio_pci_domain_read_resources(struct device *dev)
I'd recommend using the constructors in https://github. […]
I updated legacy IO resources with constructors, however, for PCI domains we have some restrictions as below,
1. We cannot use IORESOURCE_STORED because, https://github.com/coreboot/coreboot/blob/main/src/acpi/acpigen_pci_root_res...
2. For IORESOURCE_FIXED, which is conflict with below logics, https://github.com/coreboot/coreboot/blob/main/src/device/resource_allocator...
For existing constructors which are based on fixed_resource_range_idx, they are not filling resource limits and hence cannot be correctly handled by https://github.com/coreboot/coreboot/blob/main/src/acpi/acpigen_pci_root_res... which is based on {base, limits}. Besides, they are with IORESOURCE_FIXED resource set by default, which cannot correctly work with resource allocators.
Maybe we could have constructor utils for domain resource creation in device_util.c? (we can discuss in a separate thread)
https://review.coreboot.org/c/coreboot/+/81316/comment/0e4eed9a_32c695bb : PS56, Line 56: res->flags = IORESOURCE_IO | IORESOURCE_SUBTRACTIVE | IORESOURCE_ASSIGNED;
I asked on IRC, this resource should be `IORESOURCE_FIXED` so that the allocator doesn't try to allo […]
Good suggestion! Updated.
File src/soc/intel/xeon_sp/gnr/Kconfig:
https://review.coreboot.org/c/coreboot/+/81316/comment/37f7a43b_05f17ef3 : PS56, Line 17: select FSP_SPEC_VIOLATION_XEON_SP_HEAP_WORKAROUND
Intel said this issue is not present on GNR, so this is wrong.
GNR still needs this ... the related discussion history is at:
https://review.coreboot.org/c/coreboot/+/80579 https://review.coreboot.org/c/coreboot/+/80328
File src/soc/intel/xeon_sp/gnr/chip.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/b1937c2d_2cce8044 : PS56, Line 13: intelblocks
do you need all those headers?
Good catch! The list can be largely reduced. Updated.
File src/soc/intel/xeon_sp/gnr/cpu.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/fb1e4cfb_c89ad987 : PS56, Line 9: common
do you need all those headers?
Good catch! The list can be largely reduced. Updated.
File src/soc/intel/xeon_sp/gnr/romstage.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/d8e5d3a7_a146220c : PS56, Line 5: #include <console/console.h>
do you need all those headers?
Good catch! The list can be largely reduced. Updated.
https://review.coreboot.org/c/coreboot/+/81316/comment/8448bae8_dbf5781b : PS56, Line 21: switch (base_addr) {
OK, these are a bit tricky to handle because decimals, but how about: […]
It would make sense to align with UPD definition which is in decimal form. Updated.
File src/soc/intel/xeon_sp/spr/cpu.c:
PS56:
nit: Maybe move these changes to a separate commit?
Done
File src/soc/intel/xeon_sp/spr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/87862398_d1cb1cb2 : PS56, Line 158: unsigned long xeonsp_acpi_create_madt_lapics(unsigned long current)
Where did this go?
Now the common acpi codes can handle madt creation well.
unsigned long acpi_arch_fill_madt(acpi_madt_t *madt, unsigned long current) { madt->lapic_addr = cpu_get_lapic_addr();
if (CONFIG(ACPI_HAVE_PCAT_8259)) madt->flags |= 1;
if (CONFIG(ACPI_COMMON_MADT_LAPIC)) -> ACPI_COMMON_MADT_LAPIC will be set current = acpi_create_madt_lapics_with_nmis(current);
if (CONFIG(ACPI_COMMON_MADT_IOAPIC)) current = acpi_create_madt_ioapic_gsi0_default(current);
return current; }