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

[Nico Huber (Code Review)]

Attention is currently required from: Arthur Heymans, Christian Walter, Cliff Huang, Johnny Lin, Jonathan Zhang, Lance Zhao, Shuo Liu, Tim Chu, Tim Wawrzynczak.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81375?usp=email )

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

Change subject: acpi: Add acpigen_write_OSC_pci_domain
......................................................................

Patch Set 12:

(5 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/81375/comment/9138bfcd_e7ef367d :
PS12, Line 9: dynamic
Why? What runtime information does it depend on?

Patchset:

PS12: 
This is not an easy to review/maintain form, and beside a lot of boilerplate
the code seems to do a static thing. As we have the option to put it into the
DSDT and call it from each device, maybe that would be better?

(Ignore me if I missed that it relies on dynamic runtime information.)

File src/acpi/acpigen_pci.c:

https://review.coreboot.org/c/coreboot/+/81375/comment/54c965ba_571e7e9e :
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.

https://review.coreboot.org/c/coreboot/+/81375/comment/161514ca_32b6727a :
PS12, Line 272: |=
Why the `or`? Isn't `RETE` still the original input dword0?

https://review.coreboot.org/c/coreboot/+/81375/comment/c349817d_8e26f996 :
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. So why are they optional?