Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Default to undefined fw_config value ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47956/2/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/47956/2/src/Kconfig@406 PS2, Line 406: When probing devices with fw_config, when the value is undefined or : unable to be located, all devices will be successfully probed, and : fw_config_probe will always return true.
See follow-up patch 😊
Duncan and I discussed this a little today morning.
My earlier comment was incorrect. As Duncan pointed out, assuming everything enabled by default can be wrong in a number of cases e.g. audio - where there can be conflicting configuration required depending upon what option needs to be kept enabled.
The only case where we can have an unprovisioned FW_CONFIG is the first boot in factory. In such cases, we only care about booting to the OS to allow the factory toolkit to provision FW_CONFIG correctly. Once that is done, all future boots will have all components configured based on FW_CONFIG. For the first boot, it doesn't matter if we have certain components non-functional e.g. audio, sd, usb, etc. (It is expected that a system won't be fully functional until provisioning is complete).
** approach 1 ** Given that, I think we should leave fw_config_match as it is right now and let mainboard handle what it wants to do when FW_CONFIG is unprovisioned. Thus, mainboard can decide what devices it wants enabled during the factory boot by using a helper function fw_config_is_provisioned().
** approach 2 ** There is another way in which this can be handled: 1. In mainboard devicetree, keep all devices that are to be probed "off" by default. 2. Any devices that are critical for factory boot and use FW_CONFIG probing should be kept "on" by default. 3. fw_config library will keep device state untouched if FW_CONFIG is unprovisioned. 4. If FW_CONFIG is provisioned, then fw_config library will enable devices that match the probe condition and disable devices that don't. (as opposed to only disabling like it is done right now).
This eliminates the need for different fw_config_probe helper functions as it is extremely confusing which to use when(looking at the follow-up CL).
Out of the two approaches above, I think approach 1 is better. Even though it requires some additional code in mainboard to enable devices for factory boot, it keeps the device tree representation as well as fw_config_probe calls in mainboard consistent. We don't need to think about whether to use one function v/s other or whether to keep a device on/off etc. Instead a single function in mainboard can handle enable_factory_boot_devices(). Only the devices that are critical for factory boot can be added to that function as required.