Nico Huber 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 19:
(2 comments)
Patchset:
PS19: Please consider moving this back to `soc/intel/`. The code seems to be tailored to a specific, downstream (? at least, I couldn't find it in upstream Linux) interpre- tation of a non-public spec (probably outdated, I've seen a patch somewhere that stated the CXL UUID used here is not to be used).
Also, the design doesn't look like our usual acpigen code which normally isn't coupled that closely to other code (i.e. no callbacks, more simple building blocks instead of long procedures with a lot of `if`s).
File src/acpi/acpigen_pci.c:
https://review.coreboot.org/c/coreboot/+/81375/comment/10bbf85b_551d027e : 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; : }
refer to: PCI firmware specification, revision 3.1 […]
This explains what would happen not why it would make sense to generate all the code in a no-op state (when the functions aren't overridden). I still don't understand why there are weak implementations.
Thinking about it further, why are there callbacks at all? With the `const` for the parameter, the API even suggests that the devicetree wouldn't change because of a call. So further calls should still behave the same and then you could just have passed the feature mask as an argument to acpigen_write_OSC_pci_domain().