Attention is currently required from: Raul Rangel, Furquan Shaikh, Paul Menzel, Karthik Ramasubramanian. Martin Roth 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/5ce91136_0f91dc28 PS4, Line 44: get_variant_wwan_type
fw_config is used to allow OEMs to provide configuration information […]
My counter proposal to your change is this: - Each variant still supplies the option bit definitions as we do now. - All fw_config settings are defined in devicetree at a platform level - not about which bits are used for the settings, just having a definition so that it can be accessed by all variants. - All variants are expected to use the same names for things that exist. This isn't strictly required, but would make things less confusing - The fw configs bits can be defined as "always present", so the cbi FP_CONFIG doesn't even need to be checked, - The top level helpers can just check fw_config for anything they need to, and it's expected that they'd get back a response. If the config option doesn't exist on that platform, it's just returned as it's not enabled.
Looking at your example of the fingerprint on guybrush:
1) OEMs with no fingerprint support 2) OEMs with all SKUs having fingerprint support 3) OEMs with some SKUs having fingerprint support whereas others with no fingerprint support
Option 1: The FP option isn't even included in the variant devicetree - always returns false Option 2: The FP option is set to ENABLED in the variant devicetree - always returns true Option 3: The FP option is actually in the CBI, which is checked to see if the option is enabled or disabled.
This solution doesn't require that OEMs define fields that they don't care about. This solution doesn't add any significant overhead. This solution uses the current flow, and doesn't require that platforms be changed significantly. The values can be obtained and acted upon without having to find or define the devicetree structure.
I have no objection to your solution for ramstage and determining if a device in a platform is enabled - It seems like a nice way to do it. My issue with your solution is that it seems heavier than is needed in bootblock/romstage and doesn't give any benefit there. There are other ways that are less intrusive that it can be done. What I've proposed above doesn't require that we throw away all of the experience we have about this right now - it's an almost trivial expansion on what we already have.