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 )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: soc/intel/xeon_sp: Add Granite Rapids initial codes ......................................................................
Patch Set 56:
(8 comments)
File src/soc/intel/xeon_sp/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/81316/comment/16bb83e9_dde16fb5 : PS56, Line 8: ## GNR IBL codes are initially reused from EBG, will update later. nit: Add a "TODO: " prefix?
File src/soc/intel/xeon_sp/chip_gen6.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/5d678871_4ba894b3 : PS56, Line 35: static void iio_pci_domain_read_resources(struct device *dev) I'd recommend using the constructors in https://github.com/coreboot/coreboot/blob/90e835db2d2ef75ea7d0999090d1806cf7... instead of manually specifying `IORESOURCE_` flags.
Also, shouldn't all these resources be `IORESOURCE_FIXED` and `IORESOURCE_STORED`? https://github.com/coreboot/coreboot/blob/90e835db2d2ef75ea7d0999090d1806cf7...
https://review.coreboot.org/c/coreboot/+/81316/comment/76530acd_af9d0e8c : 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 allocate it. And `IORESOURCE_SUBTRACTIVE` shouldn't matter (the allocator doesn't allocate fixed resources).
File src/soc/intel/xeon_sp/gnr/Kconfig:
https://review.coreboot.org/c/coreboot/+/81316/comment/4e6ee804_04d19ce9 : PS56, Line 17: select FSP_SPEC_VIOLATION_XEON_SP_HEAP_WORKAROUND I don't remember if I mentioned this, but it would be great to fix FSP
https://review.coreboot.org/c/coreboot/+/81316/comment/9d4f80cb_4fbf2d73 : PS56, Line 34: default 255 Given that previous gen platforms (e.g. ibm/sbp1) have more cores/threads than that, I imagine this is a placeholder. Probably embargo-related.
File src/soc/intel/xeon_sp/gnr/romstage.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/c928080b_e75f3149 : PS56, Line 21: switch (base_addr) { OK, these are a bit tricky to handle because decimals, but how about:
``` static uint8_t get_mmcfg_base_upd_index(const uint64_t base_addr) { switch (base_addr) { case 1UL * GiB: // 1G return 0; case 1UL * GiB + 512UL * MiB: // 1.5G return 0x1; case 1UL * GiB + 768UL * MiB: // 1.75G return 0x2; case 2UL * GiB: // 2G return 0x3; case 2UL * GiB + 256UL * MiB: // 2.25G return 0x4; case 3UL * GiB: // 3G return 0x5; default: // Auto return 0x6; } } ```
I haven't tried, but this should compile fine. I know the comment alignment is wrong, Gerrit uses 4 spaces for tabs.
File src/soc/intel/xeon_sp/spr/cpu.c:
PS56: nit: Maybe move these changes to a separate commit?
File src/soc/intel/xeon_sp/spr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/81418116_ce90b053 : PS56, Line 158: unsigned long xeonsp_acpi_create_madt_lapics(unsigned long current) Where did this go?