[L] Change in coreboot[main]: acpi: Add acpigen_write_OSC_pci_domain

[Shuo Liu (Code Review)]

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 13:

(7 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/81375/comment/bda711a9_d0e104e0 :
PS12, Line 9: dynamic
> Sure, you cannot easily do it with static DSDT only. But most of the implementation […]

Yeah, I agree that the static ASL is more clear to read. For dynamic ASL, mainly we use it to generate a list of ASL device objects based on FSP outputs (indicates the SoC/SKU configurations) and the _OSC is a method inside (hence it would be straightforward to be generated altogether, plus some C codes managed boot configs could be referenced as well), this is used by xeon-sp only as of now. Will update the commit message to be more clarified.

File src/acpi/acpigen_pci.c:

https://review.coreboot.org/c/coreboot/+/81375/comment/962aa0e4_d98afdb0 :
PS12, Line 98: With
> Width

Done

https://review.coreboot.org/c/coreboot/+/81375/comment/67e2239f_b84afe38 :
PS12, Line 98: Name - With,  Source, Offset, Description
             :  * --------------------------------
             :  * QSUP - DWord, Local7, 0x00,   Query support
             :  * RETE - DWord, Arg3,   0x00,   Returned errors
             :  * SUPP - Dword, Arg3,   0x04,   PCIe Features that OS supported
             :  * CTRL - Dword, Arg3,   0x08,   PCIe Features that firmware grant control to OS
             :  * OTRL - Dword, Local7, 0x08,   PCIe Features that OS requests for control
             :  * SUPC - Dword, Arg3,   0x0C,   CXL Features that OS supported
             :  * CTRC - Dword, Arg3,   0x10,   CXL Features that firmware grant control to OS
             :  * OTRC - Dword, Local7, 0x10,   CXL Features that OS requests for control
> If it’s a table, I wouldn’t separate the columns by a comma.

Done

https://review.coreboot.org/c/coreboot/+/81375/comment/276e351e_204dea08 :
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);
              : 	}
              : 
              :
> I don't understand. Currently this looks like: […]

Thanks, the new flow is better. Updated.

https://review.coreboot.org/c/coreboot/+/81375/comment/6cbff211_f65d2f62 :
PS12, Line 272: |=
> Yes ... I only directly assign RETE when there is an immediately return after the assignment. […]

Done

https://review.coreboot.org/c/coreboot/+/81375/comment/172cbddf_447e928b :
PS12, Line 317: 	acpigen_write_create_dword_field(LOCAL7_OP, 0x10, "OTRC");
> > The current assumption is, for a CXL domain […]

CXL-3.1-Specification, 9.18.2

The _OSC interface defined in this section applies only to “Host Bridge” ACPI devices
that originate CXL hierarchies. As specified in Section 9.12, these ACPI devices must
have an _HID of (or a _CID that includes) EISAID(“ACPI0016”). CXL _OSC is required
for a CXL VH. CXL _OSC is optional for an RCD. A CXL Host Bridge also originates a
PCIe hierarchy and will have a _CID of EISAID(“PNP0A08”). As such, a CXL Host Bridge
device may expose both CXL _OSC and PCIe _OSC.

I will update the comment block.

https://review.coreboot.org/c/coreboot/+/81375/comment/5a8353e0_63262fc3 :
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;
              : }
> Let me rephrase: What if these are not overridden? Does a `return 0` […]

refer to: PCI firmware specification, revision 3.1

If the _OSC control method is absent from the scope of a host bridge device, then the operating
system must not enable or attempt to use any features defined in this section for the hierarchy
originated by the host bridge. Doing so could contend with platform firmware operations or
produce undesired results.

I will add comments.