Attention is currently required from: Arthur Heymans, Christian Walter, Cliff Huang, Johnny Lin, Jonathan Zhang, Lance Zhao, Nico Huber, Paul Menzel, Tim Chu, Tim Wawrzynczak.
Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81375?usp=email )
Change subject: acpi: Add acpigen_write_OSC_pci_domain ......................................................................
Patch Set 12:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81375/comment/38b83eed_62b25fe9 : PS12, Line 9: dynamic
Why? What runtime information does it depend on?
This is mainly for xeon-sp codes that one set of codes needs to handle multiple SKU or silicon, where the PCIe/CXL domain layout are totally different, thus cannot be easily covered by one set of static DSDT.
https://review.coreboot.org/c/coreboot/+/81375/comment/6bee088e_eff7be17 : PS12, Line 11:
Tested how?
This patch is tested afterwards in https://review.coreboot.org/c/coreboot/+/81377.
File src/acpi/acpigen_pci.c:
https://review.coreboot.org/c/coreboot/+/81375/comment/7756058b_0c973ebe : PS12, Line 127: : if (is_cxl_domain) { : /* : * If ((Arg0 != ToUUID (PCI_HOST_BRIDGE_OSC_UUID)) && : * (Arg0 != ToUUID (CXL_HOST_BRIDGE_OSC_UUID))) : */ : acpigen_write_if(); : acpigen_emit_byte(LAND_OP); : acpigen_emit_byte(LNOT_OP); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_byte(ARG0_OP); : acpigen_write_uuid(PCI_HOST_BRIDGE_OSC_UUID); : acpigen_emit_byte(LNOT_OP); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_byte(ARG0_OP); : acpigen_write_uuid(CXL_HOST_BRIDGE_OSC_UUID); : } else { : /* : * If (Arg0 != ToUUID (PCI_HOST_BRIDGE_OSC_UUID)) : */ : acpigen_write_if(); : acpigen_emit_byte(LNOT_OP); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_byte(ARG0_OP); : acpigen_write_uuid(PCI_HOST_BRIDGE_OSC_UUID); : } : :
This seems unnecessary, you could also make it an `If ... ElseIf ... Else UNRECOGNIZED_UUID` below.
For PCIe domain, only PCIe UUID can be handled; but for CXL domain, it can handle both. Personally I think to separately list PCIe/CXL cases would be more clear?
https://review.coreboot.org/c/coreboot/+/81375/comment/ac21714f_f6466284 : PS12, Line 272: |=
I see `RETE` is initially set to `0`. But the `|=` still seems odd (given […]
Yes ... I only directly assign RETE when there is an immediately return after the assignment. For this site, since no directly return (at least in this function) the pattern is not used.
https://review.coreboot.org/c/coreboot/+/81375/comment/04d67eb8_6340a858 : PS12, Line 317: acpigen_write_create_dword_field(LOCAL7_OP, 0x10, "OTRC");
What about the PCIe fields? On this path, it seems we would return CTRL unaltered. […]
The current assumption is, for a CXL domain, if the OS calls _OSC using a PCIe UUID, acpigen_OSC_handle_pcie_request will handle; if the OS calls _OSC using a CXL UUID, acpigen_OSC_handle_cxl_request will handle. For the current tested case, such logic is adequate. Maybe we could update later when there is some further needs?
https://review.coreboot.org/c/coreboot/+/81375/comment/f5bf627d_3c2971f2 : PS12, Line 348: __weak unsigned long get_granted_pci_features(const struct device *domain) : { : return 0; : } : : __weak unsigned long get_granted_cxl_features(const struct device *domain) : { : return 0; : }
The whole code above seems to do nothing without these. […]
Please refer to https://review.coreboot.org/c/coreboot/+/81377 where the code is called and tested. The main reason is that different mainboard/SoC might have different _OSC feature granted based on its capability and user config, hence the mainboard/SoC codes can override this to link their logics.
-
Shuo Liu (Code Review)