Attention is currently required from: Raul Rangel, Martin Roth, Paul Menzel, Karthik Ramasubramanian. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54639 )
Change subject: mb/google/guybrush: Add helpers for cbi fw_config settings. ......................................................................
Patch Set 5:
(1 comment)
File src/mainboard/google/guybrush/variants/guybrush/helpers.c:
https://review.coreboot.org/c/coreboot/+/54639/comment/3cc9a4cc_35dd2d1f PS4, Line 44: get_variant_wwan_type
Furquan, I understand what you're saying. I just disagree with the approach. […]
fw_config is used to allow OEMs to provide configuration information to software when there are multiple options they are choosing for a component on their project. This configuration information allows software to determine the appropriate steps to be performed (GPIO config, ACPI tables, etc.) at runtime. Since each OEM project is different, there is going to be a varied combination of options across the projects. Thus, the common code at the baseboard/mainboard level should not make any assumptions about the fw_config presence. In fact, it is totally fine if an OEM decides to not use fw_config at all if they intend to build just one single combination of components for the project. That is the reason why it is important to ensure that the common code at mainboard/baseboard level uses the information about devices rather than probing fw_config directly.
In this case on guybrush, you can imagine different OEMs going with different combinations for their fingerprint support: - OEMs with no fingerprint support - OEMs with all SKUs having fingerprint support - OEMs with some SKUs having fingerprint support whereas others with no fingerprint support
Out of these three, only the last case requires fw_config information.
The suggestion that I made above is to ensure that the helper functions are organized in such a way at the baseboard level that there is maximum flexibility for the OEM to choose what fw_config options they want to go with and at the same time not have to implement support for varied helper functions specific to each variant.
From your comments, it looks like the thing that you are most worried about is the runtime overhead of walking the device tree. That is not really a big problem to solve. We are already using unique device aliases in the device tree. It is just a matter of emitting all these aliases so that they can be referenced directly by the helpers. That should allow you to reduce the helper functions to a single line checks without having to rely on fw_config. I had made a comment about this earlier on this CL as well that it is possible to eliminate the device walk completely.
I don't think this makes things more complicated. In fact, it makes it much simpler for the partners to manage their own variants in they way they have designed their boards.