Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Default to undefined fw_config value ......................................................................
fw_config: Default to undefined fw_config value
If for some reason a value for fw_config cannot be located, set its value to a magic undefined value. The current `fw_config_probe` function is modified to return true if fw_config is undefined, and a new function `fw_config_probe_nodefault` is added which will return false if the fw_config value is undefined. A new Kconfig option is added, FW_CONFIG_IGNORE_UNDEFINED (defaults to n), which controls the behavior of fw_config probing the devicetree.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ib3046233667e97a5f78961fabacbeb3099b3d442 --- M src/Kconfig M src/include/fw_config.h M src/lib/fw_config.c 3 files changed, 43 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/47956/1
diff --git a/src/Kconfig b/src/Kconfig index dc98ca2..a0f692c 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -398,6 +398,16 @@ is not tried if FW_CONFIG_SOURCE_CBFS is enabled and the value was found in CBFS.
+config FW_CONFIG_IGNORE_UNDEFINED + bool "Undefined Firmware Configuration value will not probe devices" + depends on FW_CONFIG + default n + help + 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. Set this to 'y' if you do + not want this behavior. + config HAVE_RAMPAYLOAD bool
diff --git a/src/include/fw_config.h b/src/include/fw_config.h index 3c87725..e02a38e 100644 --- a/src/include/fw_config.h +++ b/src/include/fw_config.h @@ -45,11 +45,21 @@ * fw_config_probe() - Check if field and option matches. * @match: Structure containing field and option to probe. * - * Return %true if match is found, %false if match is not found. + * Return %true if match is found or fw_config value is undefined. + * %false if match is not found. */ bool fw_config_probe(const struct fw_config *match);
/** + * fw_config_probe_nodefault() - Check if field and option matches. + * @match: Structure containing field and option to probe. + * + * Return %true if match is found. + * %false if match is not found or fw_config value is undefined + */ +bool fw_config_probe_nodefault(const struct fw_config *match); + +/** * fw_config_for_each_found() - Call a callback for each fw_config field found * @cb: The callback function * @arg: A context argument that is passed to the callback diff --git a/src/lib/fw_config.c b/src/lib/fw_config.c index e17d40e..efbee01 100644 --- a/src/lib/fw_config.c +++ b/src/lib/fw_config.c @@ -29,7 +29,7 @@ CBFS_TYPE_RAW) != sizeof(fw_config_value)) { printk(BIOS_WARNING, "%s: Could not get fw_config from CBFS\n", __func__); - fw_config_value = 0; + fw_config_value = UNDEFINED_FW_CONFIG; } else { printk(BIOS_INFO, "FW_CONFIG value from CBFS is 0x%" PRIx64 "\n", fw_config_value); @@ -39,16 +39,24 @@
/* Read the value from EC CBI. */ if (CONFIG(FW_CONFIG_SOURCE_CHROMEEC_CBI)) { - if (google_chromeec_cbi_get_fw_config(&fw_config_value)) + if (google_chromeec_cbi_get_fw_config(&fw_config_value)) { printk(BIOS_WARNING, "%s: Could not get fw_config from EC\n", __func__); + fw_config_value = UNDEFINED_FW_CONFIG; + } }
printk(BIOS_INFO, "FW_CONFIG value is 0x%" PRIx64 "\n", fw_config_value); return fw_config_value; }
-bool fw_config_probe(const struct fw_config *match) +static bool fw_config_match(const struct fw_config *match, bool undefined_match) { + uint64_t fw_config; + + fw_config = fw_config_get(); + if (fw_config == UNDEFINED_FW_CONFIG) + return undefined_match; + /* Compare to system value. */ if ((fw_config_get() & match->mask) == match->value) { if (match->field_name && match->option_name) @@ -64,6 +72,16 @@ return false; }
+bool fw_config_probe(const struct fw_config *match) +{ + return fw_config_match(match, true); +} + +bool fw_config_probe_nodefault(const struct fw_config *match) +{ + return fw_config_match(match, false); +} + #if ENV_RAMSTAGE
/* @@ -111,7 +129,7 @@ continue;
for (probe = dev->probe_list; probe && probe->mask != 0; probe++) { - if (fw_config_probe(probe)) { + if (fw_config_match(probe, (bool)!CONFIG(FW_CONFIG_IGNORE_UNDEFINED))) { match = true; cached_configs[probe_index(probe->mask)] = probe; break;
Tim Wawrzynczak has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Default to undefined fw_config value ......................................................................
fw_config: Default to undefined fw_config value
If for some reason a value for fw_config cannot be located, set its value to a magic undefined value. The current `fw_config_probe` function is modified to return true if fw_config is undefined, and a new function `fw_config_probe_nodefault` is added which will return false if the fw_config value is undefined. A new Kconfig option is added, FW_CONFIG_IGNORE_UNDEFINED (defaults to n), which controls the behavior of fw_config probing the devicetree.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ib3046233667e97a5f78961fabacbeb3099b3d442 --- M src/Kconfig M src/include/fw_config.h M src/lib/fw_config.c 3 files changed, 44 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/47956/2
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 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47956/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47956/2//COMMIT_MSG@9 PS2, Line 9: If for some reason a value for fw_config cannot be located What use case is this change trying to target? Is it the case where fw_config is unprovisioned?
https://review.coreboot.org/c/coreboot/+/47956/2//COMMIT_MSG@12 PS2, Line 12: fw_config_probe_nodefault Under what circumstances would you use this function instead of fw_config_probe?
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. Do we really need a Kconfig for this? I think it should be the default behavior i.e. if FW_CONFIG is not provisioned, keep everything on by default. This should apply to all Chrome OS devices. I think we can add a Kconfig later if any non-Chrome OS devices using FW_CONFIG want to treat the unprovisioned case differently.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47956
to look at the new patch set (#3).
Change subject: fw_config: Default to undefined fw_config value ......................................................................
fw_config: Default to undefined fw_config value
In order to support the use case of leaving runtime probing of multi-sourced peripherals to the payload (or beyond) before a device is provisioned, this change defaults the fw_config value to a magic undefined value when a valid value can't be located.
In addition, the `fw_config_probe` function is modified to return true if fw_config is undefined.
A new function `fw_config_probe_nodefault` is also added, which will return false when fw_config is undefined. This can be useful for example when fw_config probing is used to configure GPIOs differently; when fw_config is undefined, it may be preferable to leave the GPIOs configured in whatever previous state they had been in.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ib3046233667e97a5f78961fabacbeb3099b3d442 --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 33 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/47956/3
Tim Wawrzynczak 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 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47956/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47956/2//COMMIT_MSG@9 PS2, Line 9: If for some reason a value for fw_config cannot be located
What use case is this change trying to target? Is it the case where fw_config is unprovisioned?
Will add.
https://review.coreboot.org/c/coreboot/+/47956/2//COMMIT_MSG@12 PS2, Line 12: fw_config_probe_nodefault
Under what circumstances would you use this function instead of fw_config_probe?
Will update.
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.
Do we really need a Kconfig for this? I think it should be the default behavior i.e. […]
I could leave it out for now, that's true we won't use it right now, but it's pretty easy to add back in if someone else needs it.
Duncan Laurie 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 3:
(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.
Do we really need a Kconfig for this? I think it should be the default behavior i.e. […]
It is going to result in weird undefined state for things like audio where we can't have both I2S and soundwire enabled at the same time. In volteer there are also some DB combinations that would result in pins being driven when they should not be if everything was assumed to be present.
Different boards may need to make different decisions about how to handle the unprovisioned case, but probably only to some fields so it may not need a kconfig but rather careful use of the different functions in the code.
Tim Wawrzynczak 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 3:
(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.
It is going to result in weird undefined state for things like audio where we can't have both I2S an […]
See follow-up patch 😊
Caveh Jalali 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 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/47956/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47956/3//COMMIT_MSG@9 PS3, Line 9: In order to support the use case of leaving runtime probing of : multi-sourced peripherals to the payload (or beyond) before a device is : provisioned hmm... i'm not sure i followed that.
are you basically trying to distinguish between the case where fw_config hasn't been set and assumed to be 0 vs. explicitly set to 0? we get away with that because in most? cases the sub-fields are defined so that a value of 0 means no device.
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c File src/lib/fw_config.c:
PS3: please update Documentation/lib/fw_config.md to match.
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c@17 PS3, Line 17: ; can we simply start with fw_config_value = UNDEFINED_FW_CONFIG ?
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c@75 PS3, Line 75: bool fw_config_probe(const struct fw_config *match) is there ever a case where we would still want to use this version?
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c@132 PS3, Line 132: fw_config_probe fw_config_probe_nodefault
Tim Wawrzynczak 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 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/47956/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47956/3//COMMIT_MSG@9 PS3, Line 9: In order to support the use case of leaving runtime probing of : multi-sourced peripherals to the payload (or beyond) before a device is : provisioned
hmm... i'm not sure i followed that. […]
I'll try to reword it... this is for the factory, before the device is provisioned. I checked and the EC will return an error on the host command when there is no FW_CONFIG tag defined in CBI yet.
We want all devices listed in the ACPI tables when it's in the factory, so all peripherals that can be probed will be, and then the FW_CONFIG value is decided from that information on the factory floor. That's the reason for having fw_config_init() use fw_config_probe() which will return true for UNDEFINED_FW_CONFIG.
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c File src/lib/fw_config.c:
PS3:
please update Documentation/lib/fw_config.md to match.
Fair point, will do.
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c@17 PS3, Line 17: ;
can we simply start with […]
Ack
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c@75 PS3, Line 75: bool fw_config_probe(const struct fw_config *match)
is there ever a case where we would still want to use this version?
see below.
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c@132 PS3, Line 132: fw_config_probe
fw_config_probe_nodefault
Nope, this is the case when we *do* want all devices enabled. See my response above in the commit msg. Here's the way I see this being used:
1) fw_config_init() uses _probe() because we want all devices in the ACPI tables on the factory floor so we can probe for everything and write FW_CONFIG 2) GPIO programming should use _probe_nodefault() in general (see next patch), because it's safest to leave GPIOs in the NC state than to assign invalid NFs
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47956
to look at the new patch set (#4).
Change subject: fw_config: Default to undefined fw_config value ......................................................................
fw_config: Default to undefined fw_config value
In order to support the use case of leaving runtime probing of multi-sourced peripherals to the payload (or beyond) before a device is provisioned, this change defaults the fw_config value to a magic undefined value when a valid value can't be located.
In addition, the `fw_config_probe` function is modified to return true if fw_config is undefined.
A new function `fw_config_probe_nodefault` is also added, which will return false when fw_config is undefined. This can be useful for example when fw_config probing is used to configure GPIOs differently; when fw_config is undefined, it may be preferable to leave the GPIOs configured in whatever previous state they had been in.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ib3046233667e97a5f78961fabacbeb3099b3d442 --- M Documentation/lib/fw_config.md M src/include/fw_config.h M src/lib/fw_config.c 3 files changed, 67 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/47956/4
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47956
to look at the new patch set (#5).
Change subject: fw_config: Default to undefined fw_config value ......................................................................
fw_config: Default to undefined fw_config value
In order to support the use case of leaving runtime probing of multi-sourced peripherals to the payload (or beyond) before a device is provisioned, this change defaults the fw_config value to a magic undefined value when a valid value can't be located.
In addition, the `fw_config_probe` function is modified to return true if fw_config is undefined.
A new function `fw_config_probe_nodefault` is also added, which will return false when fw_config is undefined. This can be useful for example when fw_config probing is used to configure GPIOs differently; when fw_config is undefined, it may be preferable to leave the GPIOs configured in whatever previous state they had been in.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ib3046233667e97a5f78961fabacbeb3099b3d442 --- M Documentation/lib/fw_config.md M src/include/fw_config.h M src/lib/fw_config.c 3 files changed, 70 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/47956/5
Tim Wawrzynczak 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c@17 PS3, Line 17: ;
Ack
See patchset 5,
Forbidden global variables in romstage: ffffff00 d fw_config_value.7045 make: *** [src/arch/x86/Makefile.inc:186: coreboot-builds/GOOGLE_VOLTEER2/cbfs/fallback/romstage.debug] Error 1 make: *** Deleting file 'coreboot-builds/GOOGLE_VOLTEER2/cbfs/fallback/romstage.debu
Tim Wawrzynczak 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:
ping 😊
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.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47956
to look at the new patch set (#6).
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned
A mainboard might want to configure some things differently when a device is in an unprovisioned state. In the case when fw_config comes from the Chromium EC, then usually means that there is no FW_CONFIG tag in the CBI, and hence the device is in an unprovisioned state. This patch will make the fw_config value equal to UNDEFINED_FW_CONFIG in the case of an error retrieving the value, as well as adding a function, `fw_config_is_provisioned()` to indicate the provisioning status.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ib3046233667e97a5f78961fabacbeb3099b3d442 --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/47956/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 6:
(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.
Duncan and I discussed this a little today morning. […]
Gotcha, I like your first approach too, see next patchset.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47956/6/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/47956/6/src/include/fw_config.h@60 PS6, Line 60: is if
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47956/6/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/47956/6/src/include/fw_config.h@60 PS6, Line 60: is
if
Done
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c@75 PS3, Line 75: bool fw_config_probe(const struct fw_config *match)
see below.
Ack
https://review.coreboot.org/c/coreboot/+/47956/3/src/lib/fw_config.c@132 PS3, Line 132: fw_config_probe
Nope, this is the case when we *do* want all devices enabled. […]
Ack
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47956
to look at the new patch set (#7).
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned
A mainboard might want to configure some things differently when a device is in an unprovisioned state. In the case when fw_config comes from the Chromium EC, then usually means that there is no FW_CONFIG tag in the CBI, and hence the device is in an unprovisioned state. This patch will make the fw_config value equal to UNDEFINED_FW_CONFIG in the case of an error retrieving the value, as well as adding a function, `fw_config_is_provisioned()` to indicate the provisioning status.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ib3046233667e97a5f78961fabacbeb3099b3d442 --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/47956/7
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47956/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47956/3//COMMIT_MSG@9 PS3, Line 9: In order to support the use case of leaving runtime probing of : multi-sourced peripherals to the payload (or beyond) before a device is : provisioned
I'll try to reword it... this is for the factory, before the device is provisioned. […]
Ack
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 7: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/47956/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47956/7//COMMIT_MSG@11 PS7, Line 11: then "that" ?
https://review.coreboot.org/c/coreboot/+/47956/7//COMMIT_MSG@16 PS7, Line 16: Missing BUG and TEST fields.
https://review.coreboot.org/c/coreboot/+/47956/7/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/47956/7/src/include/fw_config.h@61 PS7, Line 61: % Typo?
https://review.coreboot.org/c/coreboot/+/47956/7/src/include/fw_config.h@61 PS7, Line 61: % Is this a typo?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47956/7/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/47956/7/src/include/fw_config.h@61 PS7, Line 61: %
Is this a typo?
it is from kernel-doc style comments, %CONST to reference a constant, similar to the @parameter use. we don't actually do much with it yet in coreboot but it would be nice if we did someday..
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47956/7/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/47956/7/src/include/fw_config.h@61 PS7, Line 61: %
it is from kernel-doc style comments, %CONST to reference a constant, similar to the @parameter use. […]
Thanks for the explanation, Duncan.
https://review.coreboot.org/c/coreboot/+/47956/7/src/include/fw_config.h@61 PS7, Line 61: %
Typo?
Done
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Julius Werner, Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47956
to look at the new patch set (#8).
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned
A mainboard might want to configure some things differently when a device is in an unprovisioned state. In the case when fw_config comes from the Chromium EC, an unprovisioned device will not have a FW_CONFIG tag in its CBI. This patch will set the fw_config value to UNDEFINED_FW_CONFIG in the case of an error retrieving the value, as well as adding a function, `fw_config_is_provisioned()` to indicate the provisioning status.
BUG=none TEST=remove fw_config from chromium EC CBI, add code to mainboard to print return value of fw_config_is_provisioned() (`0`), add fw_config back to CBI, run same test and see `1`.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ib3046233667e97a5f78961fabacbeb3099b3d442 --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 15 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/47956/8
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47956/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47956/7//COMMIT_MSG@11 PS7, Line 11: then
"that" ?
Done
https://review.coreboot.org/c/coreboot/+/47956/7//COMMIT_MSG@16 PS7, Line 16:
Missing BUG and TEST fields.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 8: Code-Review+2
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
Patch Set 8: Code-Review+2
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47956 )
Change subject: fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned ......................................................................
fw_config: Use UNDEFINED_FW_CONFIG to mean unprovisioned
A mainboard might want to configure some things differently when a device is in an unprovisioned state. In the case when fw_config comes from the Chromium EC, an unprovisioned device will not have a FW_CONFIG tag in its CBI. This patch will set the fw_config value to UNDEFINED_FW_CONFIG in the case of an error retrieving the value, as well as adding a function, `fw_config_is_provisioned()` to indicate the provisioning status.
BUG=none TEST=remove fw_config from chromium EC CBI, add code to mainboard to print return value of fw_config_is_provisioned() (`0`), add fw_config back to CBI, run same test and see `1`.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ib3046233667e97a5f78961fabacbeb3099b3d442 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47956 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Nick Vaccaro nvaccaro@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 15 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Nick Vaccaro: Looks good to me, approved
diff --git a/src/include/fw_config.h b/src/include/fw_config.h index 3c87725..b702871 100644 --- a/src/include/fw_config.h +++ b/src/include/fw_config.h @@ -57,6 +57,12 @@ void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg);
/** + * fw_config_is_provisioned() - Determine if FW_CONFIG has been provisioned. + * Return %true if FW_CONFIG has been provisioned, %false otherwise. + */ +bool fw_config_is_provisioned(void); + +/** * fw_config_get_found() - Return a pointer to the fw_config struct for a given field. * @field_mask: A field mask from static.h, e.g., FW_CONFIG_FIELD_FEATURE_MASK * diff --git a/src/lib/fw_config.c b/src/lib/fw_config.c index 2f7186c..2c4c6b2 100644 --- a/src/lib/fw_config.c +++ b/src/lib/fw_config.c @@ -28,7 +28,7 @@ sizeof(fw_config_value)) != sizeof(fw_config_value)) { printk(BIOS_WARNING, "%s: Could not get fw_config from CBFS\n", __func__); - fw_config_value = 0; + fw_config_value = UNDEFINED_FW_CONFIG; } else { printk(BIOS_INFO, "FW_CONFIG value from CBFS is 0x%" PRIx64 "\n", fw_config_value); @@ -38,8 +38,10 @@
/* Read the value from EC CBI. */ if (CONFIG(FW_CONFIG_SOURCE_CHROMEEC_CBI)) { - if (google_chromeec_cbi_get_fw_config(&fw_config_value)) + if (google_chromeec_cbi_get_fw_config(&fw_config_value)) { printk(BIOS_WARNING, "%s: Could not get fw_config from EC\n", __func__); + fw_config_value = UNDEFINED_FW_CONFIG; + } }
printk(BIOS_INFO, "FW_CONFIG value is 0x%" PRIx64 "\n", fw_config_value); @@ -63,6 +65,11 @@ return false; }
+bool fw_config_is_provisioned(void) +{ + return fw_config_get() != UNDEFINED_FW_CONFIG; +} + #if ENV_RAMSTAGE
/*
-
Caveh Jalali (Code Review) -
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Nick Vaccaro (Code Review)
-
Tim Wawrzynczak (Code Review)