Attention is currently required from: Patrick Rudolph, Simon Chou, Jonathan Zhang, TimLiu-SMCI, Christian Walter, Arthur Heymans, Shuming Chu (Shuming), Shelly Chang, Elyes Haouas.
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/71967 )
Change subject: soc/intel/xeon_sp/spr: add ACPI code ......................................................................
Patch Set 22:
(13 comments)
Patchset:
PS2:
One way is to split into 3 parts: bootblock, romstage, ramstage.
Ack
File src/soc/intel/xeon_sp/spr/acpi/pch.asl:
https://review.coreboot.org/c/coreboot/+/71967/comment/e6c3149b_1439cc34 PS9, Line 186: /* PC-class System Timer */
maybe not needed.
Done
https://review.coreboot.org/c/coreboot/+/71967/comment/5fa23076_7a780f34 PS9, Line 208: /* Microsoft Sound System Compatible Device */
ditto
Done
https://review.coreboot.org/c/coreboot/+/71967/comment/781274ab_4fd0b360 PS9, Line 222: /* PNP Motherboard Resources */
ditto
Done
File src/soc/intel/xeon_sp/spr/acpi/pci_resource.asl:
https://review.coreboot.org/c/coreboot/+/71967/comment/c9a084f8_a46cdfcd PS19, Line 32: 0x00
this should be SOCKET
Done
File src/soc/intel/xeon_sp/spr/acpi/uncore.asl:
https://review.coreboot.org/c/coreboot/+/71967/comment/83ff532a_f9d7b306 PS13, Line 38: #if (CONFIG_MAX_SOCKET > 3)
This part of the DSDT code should be generated, using hob->PlatformData.numofIIO.
Same topic as the PS 13, mark resolved.
File src/soc/intel/xeon_sp/spr/acpi/uncore_irq.asl:
https://review.coreboot.org/c/coreboot/+/71967/comment/1c0b5279_8103b6a4 PS2, Line 3: Name (AR00, Package() { : // D31 : Package(){0x001FFFFF, 0, 0, 16 }, : Package(){0x001FFFFF, 1, 0, 17 }, : Package(){0x001FFFFF, 2, 0, 18 }, : Package(){0x001FFFFF, 3, 0, 19 }, : // D30 : Package(){0x001EFFFF, 0, 0, 16 }, : Package(){0x001EFFFF, 1, 0, 17 }, : Package(){0x001EFFFF, 2, 0, 18 },
We did not test irq mode, I suppose to test irq mode is to add kernel cmd line "pci=nomsi"? which we […]
Done
File src/soc/intel/xeon_sp/spr/chip.h:
https://review.coreboot.org/c/coreboot/+/71967/comment/87cfd4f7_208655ab PS2, Line 49: /** : * Device Interrupt Routing configuration : * Interrupt Pin x Route. : * 0h = PIRQA# : * 1h = PIRQB# : * 2h = PIRQC# : * 3h = PIRQD# : * 4h = PIRQE# : * 5h = PIRQF# : * 6h = PIRQG# : * 7h = PIRQH# : */ : uint16_t ir00_routing; : uint16_t ir01_routing; : uint16_t ir02_routing; : uint16_t ir03_routing; : uint16_t ir04_routing;
Johnny, could you provide feedback on this?
Done
File src/soc/intel/xeon_sp/spr/chip.c:
https://review.coreboot.org/c/coreboot/+/71967/comment/637e30bb_c01769ff PS2, Line 36: const char *soc_acpi_name(const struct device *dev); : const char *soc_acpi_name(const struct device *dev)
For some reason when I use static, there is build error. I have not been able to figure out why. […]
Ack
File src/soc/intel/xeon_sp/spr/romstage.c:
https://review.coreboot.org/c/coreboot/+/71967/comment/148b1ba3_57afb0e9 PS2, Line 177: 0x3F8;
I think: […]
Set from CONFIG_TTYS0_BASE, and the mainboard_memory_init_params() can override them from mainboard as well.
File src/soc/intel/xeon_sp/spr/romstage.c:
https://review.coreboot.org/c/coreboot/+/71967/comment/5ebd740e_99f3b574 PS9, Line 4: <arch/cpu.h>
<cpu/cpu. […]
Done
File src/soc/intel/xeon_sp/spr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/71967/comment/89fe90a9_7a53068c PS2, Line 229: typedef enum { DSDT_DINO = 0, DSDT_CPM, DSDT_HQM, DSDT_CPM1, DSDT_HQM1 } DSDT_RES;
Don't use typedef and put on different lines.
Done
File src/soc/intel/xeon_sp/spr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/71967/comment/2064226e_32a74fe5 PS13, Line 387: for (uint8_t socket = 0; socket < hob->PlatformData.numofIIO; ++socket) {
it's fine to generate the DSDT from CONFIG_MAX_SOCKET as long as the SSDT is […]
PlatformData.numofIIO does show 1 on my single socket AC, is there a way for DSDT code to use hob->PlatformData.numofIIO instead of CONFIG_MAX_SOCKET?
-
Johnny Lin (Code Review)