Attention is currently required from: Cliff Huang, Lance Zhao, Nico Huber, Paul Menzel, Subrata Banik, Subrata Banik, Tim Wawrzynczak.
David Ruth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80170?usp=email )
Change subject: Add MTCL function to ACPI SSDT tables ......................................................................
Patch Set 7:
(4 comments)
Patchset:
PS6:
Sorry, is there a misunderstanding? I meant the coreboot firmware (not the firmware […]
My answer applied to the data that is being stored in coreboot. I think there must be some misunderstanding. I've tried to rephrase what I meant with more detail. Perhaps I don't understand what aspects of your general question are important to answer.
This information is being put here because it should be available to the kernel immediately at system boot. It is specific to WiFi chips made by MediaTek that support 6GHz band operation, but could be used theoretically with any CPU (although I have only tested this with Intel platforms). This is intended only to be enabled on certain Chromebook boards, but there's no reason why it couldn't be used by others.
File src/drivers/wifi/generic/acpi.c:
https://review.coreboot.org/c/coreboot/+/80170/comment/fdc4d78d_2a13ae7b : PS6, Line 592: uint8_t mtcl_package[sizeof(struct wifi_mtcl)];
Ok, I think I understand better now why you want this part to be more generic. […]
Addressing the organization of the code (and updated the commit message to hopefully make what I'm trying to do clearer):
The addition to acpigen generates the ACPI package. I observed similar functions in that file, but if vendor-specific methods and extensions don't belong there, I can move it.
If it belongs elsewhere, I could refactor it out, but I think based on the discussion related to chromeos, it would belong best in the src/drivers/wifi tree somewhere, perhaps in mtcl.c.
The additions to drivers/wifi are because the goal is to add a function to the SSDT for WiFi chipsets for which it is pertinent. The SSDT for WiFi devices is generated there, so that seems like the correct place to call out to this functionality for certain.
The functionality put in chromeos was done so because all other WiFi functionality related to SSDT table generation used in chromeos is there, and I was modeling the structure after existing code.
Please let me know if you have strong preferences about reorganization, I'm relatively new to the product, and was attempting to follow the same patterns observed in similar changes that were made previously to implement WiFi SSDT functionality.
File src/vendorcode/google/chromeos/mtcl.c:
PS6:
The functionality is being added specifically for Chromebooks and ChromeOS. […]
Moved to drivers/wifi/generic/.
https://review.coreboot.org/c/coreboot/+/80170/comment/348e4aa0_cd49cf02 : PS6, Line 59: if (mtcl_bin_len != sizeof(struct wifi_mtcl)) {
Do we expect any other callers, any other implementations?
I don't expect any other callers (this function must be in the WiFi device's SSDT adn this is only generated in src/drivers/wifi/generic/acpi.c as far as I could tell). What I expect are new revisions to change the structure of the data internally. It'll still be opaque bytes in and out, but I want to allow for structural changes to be abstracted away from the callers, and keep them localized to this file. I also don't see any benefit to passing the structured data to callers, because I expect them to just write bytes through acpigen, and writing a byte array is more straightforward than writing structured data.
Is there a benefit to passing the structured data around? I don't see one, but if I'm missing it, it'd help me understand why it should be in the signature.