Attention is currently required from: Gwendal Grignou, Paul Menzel, Paz Zcharya.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80738?usp=email )
Change subject: vc/google/chromeos: Implement dynamic ChromeOS boot logo selection ......................................................................
Patch Set 7:
(3 comments)
File src/vendorcode/google/chromeos/tpm_factory_config.c:
https://review.coreboot.org/c/coreboot/+/80738/comment/7ffce01d_6b5a41db : PS7, Line 49: * - Bits 3-0 (0x1): Must be 0x1 to signify compliance with Chromebook-Plus
To be future-proof, the test on line 61 should be
`return factory_config & CHROMEBOOK_PLUS_DEVICE;`
If you read the bug ticket number attached to the commit message, you will see that the issue this CL aims to remedy is due to the fact that we were originally returning `return factory_config & CHROMEBOOK_PLUS_DEVICE;`
When we `return factory_config & CHROMEBOOK_PLUS_DEVICE;`, that means we are considering a device qualified as CBX if `Bit 4 is set` or `Bit 0-3 is set` or `both are set`.
However, in practice, we have seen devices that are CBX HW compliant (bit 0-3) but are not branded as CBX. As a result, we are displaying the CBX logo instead of the correct device segment logo (CB). This CL is intended to fix that issue, as described in the commit message.
- If the GSC bit indicates that both Bit-4 and Bit 0-3 are set to 1, then we know that this is a HW branded CBX. - If bit 0-3 is set, then it can be either a SB CBX or a regular CB. To detect which one it is properly, we are relying on VPD read. - If none of the above logic is being met, then this device is definitely a CB.
https://review.coreboot.org/c/coreboot/+/80738/comment/573b8520_777971c1 : PS7, Line 74: * requirement.
I assume the design doc is at `go/cros-tiering-dd`. If this CL only applies to new chromebook that have identification in their GSC where `chassis_x_branded` and `hw_x_compliance_version` are properly set, then we don't need to read the VPD at all. Per design [see section `OS Runtime flow`], all the information we need in the GSC.
Please read go/multi-logo-fw-splash-screen doc. and also as explained https://review.coreboot.org/c/coreboot/+/80738/comment/d8b19982_593d16b0/, we intended to relying on GSC for decision making but there are few corner case which forces us to still reading the VPD.
``` +--------------------------------------------------------------------------------------------------------+---------------------------------------------+---------------------+---------------------------+-------------------------+ | Devices | Config selection | Factory Config | | Feature_Device_Info VPD | +--------------------------------------------------------------------------------------------------------+---------------------------------------------+---------------------+---------------------------+-------------------------+ | Legacy Devices | CONFIG_BMP_LOGO=y, CONFIG_FSP2_0_LOGO_FILE | X | X | “CAE” | | Devices with Chromebook (no-CBX and no X-components) | CONFIG_FW_SPLASH_SCREEN=y, CONFIG_CB_LOGO= | chassis_x_branded=0 | hw_x_compliance_version=0 | “CAE” | | Devices Compliant with CBX HW Spec but treated Legacy (Soft Branded Legacy) | CONFIG_FW_SPLASH_SCREEN=y ,CONFIG_CBX_LOGO= | chassis_x_branded=0 | hw_x_compliance_version=1 | “CAE” | | Devices Compliant with CBX HW Spec and Waived as CBX (Soft Branded Waived Device) aka Soft-branded CBX | CONFIG_FW_SPLASH_SCREEN=y, CONFIG_CBX_LOGO= | chassis_x_branded=0 | hw_x_compliance_version=1 | “CAI” | | Devices with Hard Chromebook-Plus branding | CONFIG_FW_SPLASH_SCREEN=y, CONFIG_CBX_LOGO= | chassis_x_branded=1 | hw_x_compliance_version=1 | “CAI” | +--------------------------------------------------------------------------------------------------------+---------------------------------------------+---------------------+---------------------------+-------------------------+
```
https://review.coreboot.org/c/coreboot/+/80738/comment/044e5b5e_4a29fbed : PS7, Line 104: return strncmp(vpd_get_feature_device_info(), "CAI", 3) == 0;
The previous comment states "all older devices will have legacy config", so we don't need the non-GSC bit parsing logic, do we?
we are relying on non GSC bit parsing aka VPD logic to uniquely identify the #2 (below) if the HW complaint device is branded as SB CBX or regular CB device.
SB CBX device
1. The device is compliant to the Chromebook-Plus Hardware Specification. 2. Product requirement makes this device qualified as Chromebook-Plus.
Regular Chromebook device
1. Device is compliant to the Chromebook-Plus Hardware Specification. 2. Product requirement makes this device to act as Chromebook (it can be branded CBX later if required)