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.
10 comments:
File src/soc/intel/xeon_sp/Makefile.mk:
Patch Set #56, 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:
Patch Set #56, 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_resource_producer.c#L78
2. For IORESOURCE_FIXED, which is conflict with below logics,
https://github.com/coreboot/coreboot/blob/main/src/device/resource_allocator_v4.c#L349
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_resource_producer.c#L15 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)
Patch Set #56, 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:
Patch Set #56, 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:
Patch Set #56, 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:
Patch Set #56, 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:
Patch Set #56, Line 5: #include <console/console.h>
do you need all those headers?
Good catch! The list can be largely reduced. Updated.
Patch Set #56, 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:
nit: Maybe move these changes to a separate commit?
Done
File src/soc/intel/xeon_sp/spr/soc_acpi.c:
Patch Set #56, 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;
}
To view, visit change 81316. To unsubscribe, or for help writing mail filters, visit settings.