Attention is currently required from: Chen, Gang C, Christian Walter, David Hendricks, Jincheng Li, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, 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 33: Code-Review+1
(10 comments)
File src/soc/intel/xeon_sp/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/81316/comment/65342e0d_b2c0b3dc : PS33, Line 8: ebg : This is confusing. There is no PCH right?
File src/soc/intel/xeon_sp/chip_gen6.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/dd3ee799_b227ef8a : PS33, Line 44: IOINDEX Just use index directly? this one should be deprecated as links are not in struct bus anymore.
File src/soc/intel/xeon_sp/gnr/acpi/gpe.asl:
https://review.coreboot.org/c/coreboot/+/81316/comment/241f1da7_56add705 : PS33, Line 27: Method (_L62, 0, NotSerialized) : { : DBGO("\_GPE\_L62\n") : SGPC = 0 // clear SWGPE control : SGPS = 1 // clear SWGPE Status : } What do you need this for?
File src/soc/intel/xeon_sp/gnr/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/81316/comment/9a219866_c75e9c97 : PS33, Line 6: Name (_S0, Package () // mandatory system state : { : 0x00, 0x00, 0x00, 0x00 : }) : : Name (_S5, Package () // mandatory system state : { : 0x07, 0x00, 0x00, 0x00 : }) can you sleepstates.asl from common code?
File src/soc/intel/xeon_sp/gnr/chip.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/a15a454f_6260b96a : PS33, Line 17: uint8_t x2apic; unused?
File src/soc/intel/xeon_sp/gnr/cpu.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/ce5bd3b0_1e343316 : PS33, Line 63: {0, 0}, CPU_TABLE_END
File src/soc/intel/xeon_sp/gnr/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/f451834d_f8f1c124 : PS33, Line 6: #include <device/device.h> : #include <cpu/x86/msr.h> This should not be in this header.
File src/soc/intel/xeon_sp/gnr/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/fbfd35ca_9fa95ff0 : PS33, Line 179: DMIRCBAR I'm curious. Is there DMI when there is no PCH?
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/942737da_4b147e19 : PS5, Line 306: #if CONFIG(PLATFORM_USES_FSP2_4) : int soc_add_dram_resources(struct device *dev, int start_index) : { : return 0; : } : #else
In latest version, the soc resource is added after the generic resources. Suppose this should be better?
Maybe I got confused by the API that adds index count. Thanks, I like the latest version indeed :-)
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/70df99ea_ddc6f5ba : PS33, Line 35: enum { : TOHM_REG, : MMIOL_REG, : MMCFG_BASE_REG, : MMCFG_LIMIT_REG, : TOLM_REG, : /* NCMEM and ME ranges are mutually exclusive */ : NCMEM_BASE_REG, : NCMEM_LIMIT_REG, : ME_BASE_REG, : ME_LIMIT_REG, : TSEG_BASE_REG, : TSEG_LIMIT_REG, : VTDBAR_REG, : /* Must be last. */ : NUM_MAP_ENTRIES : }; Does it make sense to move this to a header? It's used as index for the array below.
-
Arthur Heymans (Code Review)