Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
fw_config: Add caching to successfully probed fields
Add a backing cache for all successfully probed fw_config fields. This allows recall of the `struct fw_config` which was probed.
BUG=b:161963281 TEST=tested with follower patch
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I0d014206a4ee6cc7592e12e704a7708652330eaf --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 37 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/44782/1
diff --git a/src/include/fw_config.h b/src/include/fw_config.h index d41afd6..ca4e8c6 100644 --- a/src/include/fw_config.h +++ b/src/include/fw_config.h @@ -40,6 +40,13 @@ */ bool fw_config_probe(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 + */ +void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg); + #else
static inline bool fw_config_probe(const struct fw_config *match) diff --git a/src/lib/fw_config.c b/src/lib/fw_config.c index e97cfdc..9dea196 100644 --- a/src/lib/fw_config.c +++ b/src/lib/fw_config.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <assert.h> #include <bootstate.h> #include <cbfs.h> #include <console/console.h> @@ -9,6 +10,17 @@ #include <stdbool.h> #include <stdint.h>
+#if ENV_RAMSTAGE +/* + * The maximum number of fw_config fields is limited by the 32-bit mask that is used to + * represent them. + */ +#define MAX_CACHE_ELEMENTS (8 * sizeof(uint32_t)) + +static struct fw_config cached_configs[MAX_CACHE_ELEMENTS]; +static size_t cache_count; +#endif + /** * fw_config_get() - Provide firmware configuration value. * @@ -59,6 +71,15 @@ else printk(BIOS_INFO, "fw_config match found: mask=0x%08x value=0x%08x\n", match->mask, match->value); + +#if ENV_RAMSTAGE + if (fw_config_get_cached(match->mask) == NULL) { + assert(cache_count < MAX_CACHE_ELEMENTS); + memcpy(&cached_configs[cache_count], match, sizeof(struct fw_config)); + ++cache_count; + } +#endif + return true; }
@@ -66,6 +87,15 @@ }
#if ENV_RAMSTAGE + +void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg) +{ + size_t i; + + for (i = 0; i < cache_count; ++i) + cb(&cached_configs[i], arg); +} + static void fw_config_init(void *unused) { struct device *dev;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@91 PS1, Line 91: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg) that open brace { should be on the previous line
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@75 PS1, Line 75: #if ENV_RAMSTAGE Why is a macro needed? Why can't it just be a normal C conditional.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
is this problem much different from specifying the underlying USB port numbers in the conn driver? can we add support for a "i2s-port" field to the appropriate coreboot driver for the kernel driver to pick up? we might need to add a skeletal alc5682i.c driver if the max98*.c driver isn't the right place.
actually, soundwire alc5682/max98373 use the generic x.y path specifier, so maybe that's another option to specify the i2s port number needed to pass to the kernel.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@75 PS1, Line 75: #if ENV_RAMSTAGE
Why is a macro needed? Why can't it just be a normal C conditional.
Right now, cache_count and cache_configs are defined within an ENV_RAMSTAGE (line 20)
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
Patch Set 1:
is this problem much different from specifying the underlying USB port numbers in the conn driver? can we add support for a "i2s-port" field to the appropriate coreboot driver for the kernel driver to pick up? we might need to add a skeletal alc5682i.c driver if the max98*.c driver isn't the right place.
actually, soundwire alc5682/max98373 use the generic x.y path specifier, so maybe that's another option to specify the i2s port number needed to pass to the kernel.
Is there already kernel support for an i2s-port field somewhere?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
is this problem much different from specifying the underlying USB port numbers in the conn driver? can we add support for a "i2s-port" field to the appropriate coreboot driver for the kernel driver to pick up? we might need to add a skeletal alc5682i.c driver if the max98*.c driver isn't the right place.
actually, soundwire alc5682/max98373 use the generic x.y path specifier, so maybe that's another option to specify the i2s port number needed to pass to the kernel.
Is there already kernel support for an i2s-port field somewhere?
The kernel audio for intel is all based around DMI checks currently, but there is discussion about moving to something more flexible (like audio-graph) in the future. Lots of early discussion in the bug but some of it moved to email later..
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@75 PS1, Line 75: #if ENV_RAMSTAGE
Right now, cache_count and cache_configs are defined within an ENV_RAMSTAGE (line 20)
Ya, but you added that. That isn't necessary, though.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
I like that this solves it in a generic way, having all the probed options in DMI will be useful for debug.
You might consider having arch/x86/smbios.c:smbios_write_type11() call an fw_config function to add the strings rather than needing each mainboard to add a handler.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@13 PS1, Line 13: #if ENV_RAMSTAGE After moving things around I think you can just merge this to the ENV_RAMSTAGE block below.
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@20 PS1, Line 20: static struct fw_config cached_configs[MAX_CACHE_ELEMENTS]; If you tie this to fw_config_init() you could just store pointers here, rather than having to memcpy the whole thing.
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@21 PS1, Line 21: static size_t cache_count; Rather than the increasing count I would consider using __ffs(match->mask) as the index. That way, duplicates will automatically map to the same slot. You can also easily implement the fw_config_get_cached() API that way if you want.
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@112 PS1, Line 112: match = true; I think you should put this here, since it's supposed to be tied to the device tree parsing. Running it for manually called fw_config_probe() doesn't seem useful and would only add unpredictability.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
Patch Set 1:
I like that this solves it in a generic way, having all the probed options in DMI will be useful for debug.
You might consider having arch/x86/smbios.c:smbios_write_type11() call an fw_config function to add the strings rather than needing each mainboard to add a handler.
I thought about that, but I disliked the direct coupling between fw_config & smbios. How about a new file src/vendorcode/google/chromeos/smbios.c with the implementation there?
If we do that, we actually may want to add something in vendorcode to set up the x86 mainboard callbacks (acpi_inject_dsdt, now get_smbios_strings) as well
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@75 PS1, Line 75: #if ENV_RAMSTAGE
Ya, but you added that. That isn't necessary, though.
I didn't want to waste the space in bootblock, verstage and romstage (cache size is already 512 bytes, probably 1K when we inevitably double the size of fw_config).
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@112 PS1, Line 112: match = true;
I think you should put this here, since it's supposed to be tied to the device tree parsing. […]
That's true, but when I wrote this, I intentionally did it this way for the manually called ones as well, say TABLETMODE, which seems less likely to get used in devtree, as it's probably consumed by the EC, which carries the accel/gyro.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@21 PS1, Line 21: static size_t cache_count;
Rather than the increasing count I would consider using __ffs(match->mask) as the index. […]
that's exactly what I was trying to remember the intrinsic for and I couldn't to do exactly what you said, thanks!
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
I like that this solves it in a generic way, having all the probed options in DMI will be useful for debug.
You might consider having arch/x86/smbios.c:smbios_write_type11() call an fw_config function to add the strings rather than needing each mainboard to add a handler.
I thought about that, but I disliked the direct coupling between fw_config & smbios. How about a new file src/vendorcode/google/chromeos/smbios.c with the implementation there?
If we do that, we actually may want to add something in vendorcode to set up the x86 mainboard callbacks (acpi_inject_dsdt, now get_smbios_strings) as well
It might be a problem for people using a build without CONFIG_CHROMEOS. The current function walks all devices so cros_ec could register a handler too, but that makes a bit of a circle where fw_config calls cros_ec to get the value and cros_ec calls fw_config to print the strings. maybe just having it in the baseboard like you do now is fine...
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@75 PS1, Line 75: #if ENV_RAMSTAGE
I didn't want to waste the space in bootblock, verstage and romstage (cache size is already 512 byte […]
If we switch to doing it in _init(), then we can stash pointers, and the cache size isn't as big of a deal.
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@112 PS1, Line 112: match = true;
That's true, but when I wrote this, I intentionally did it this way for the manually called ones as […]
Although this does all become a lot simpler if it's just done in _init()... we can keep pointers and then the cache isn't a big deal for non-ramstage
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44782
to look at the new patch set (#2).
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
fw_config: Add caching to successfully probed fields
Add a backing cache for all successfully probed fw_config fields that originated as `probe` statements in the devicetree. This allows recall of the `struct fw_config` which was probed.
BUG=b:161963281 TEST=tested with follower patch
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I0d014206a4ee6cc7592e12e704a7708652330eaf --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/44782/2
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44782
to look at the new patch set (#3).
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
fw_config: Add caching to successfully probed fields
Add a backing cache for all successfully probed fw_config fields that originated as `probe` statements in the devicetree. This allows recall of the `struct fw_config` which was probed.
BUG=b:161963281 TEST=tested with follower patch
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I0d014206a4ee6cc7592e12e704a7708652330eaf --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/44782/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@75 PS1, Line 75: #if ENV_RAMSTAGE
If we switch to doing it in _init(), then we can stash pointers, and the cache size isn't as big of […]
If the references to the symbols are guarded by ENV_RAMSTAGE the garbage collector will remove the space at link time. There is no space impact for !ENV_RAMSTAGE environments.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c@78 PS3, Line 78: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg) that open brace { should be on the previous line
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@75 PS1, Line 75: #if ENV_RAMSTAGE
If the references to the symbols are guarded by ENV_RAMSTAGE the garbage collector will remove the s […]
It certainly has gotten a lot better, years ago I would not have expected that, so I checked and fw_config_for_each_found() turns into a `ret` only for !RAMSTAGE. Nice.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@13 PS1, Line 13: #if ENV_RAMSTAGE
After moving things around I think you can just merge this to the ENV_RAMSTAGE block below.
Not even needed now, after moving caching to fw_config_init(), the linker does a fantastic job removing the unneeded statics from the !RAMSTAGE .o file.
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@20 PS1, Line 20: static struct fw_config cached_configs[MAX_CACHE_ELEMENTS];
If you tie this to fw_config_init() you could just store pointers here, rather than having to memcpy […]
Done
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@21 PS1, Line 21: static size_t cache_count;
that's exactly what I was trying to remember the intrinsic for and I couldn't to do exactly what you […]
Done
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@112 PS1, Line 112: match = true;
Although this does all become a lot simpler if it's just done in _init()... […]
Much nicer, I'm convinced.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/1/src/lib/fw_config.c@75 PS1, Line 75: #if ENV_RAMSTAGE
It certainly has gotten a lot better, years ago I would not have expected that, so I checked and fw_ […]
And even that symbol won't be included. We employ -ffunction-sections -fdata-sections during compilation. If you look at the objects w/ readelf you'll see all objects (both function and data) in their own sections,. Then when linking we use -gc-sections to garbage collect unreferenced sections (both data and functions).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44782/3/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/44782/3/src/include/fw_config.h@48 PS3, Line 48: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg); For the use case you have, wouldn't a fw_config_get_cached(field_mask) API be more useful? You're just iterating through them all to find a specific one anyway, and you know the index you want to check already.
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c@78 PS3, Line 78: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg) I think you should move this under the #if ENV_RAMSTAGE block, since it can't possibly do anything useful in other stages. Forcing a link error when someone tries to use it in a way that cannot work may help prevent mistakes.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c@91 PS3, Line 91: return __ffs(probe->mask); This can return -1:
/* Find First Set: __ffs(1) == 0, __ffs(0) == -1, __ffs(1<<31) == 31 */
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44782
to look at the new patch set (#4).
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
fw_config: Add caching to successfully probed fields
Add a backing cache for all successfully probed fw_config fields that originated as `probe` statements in the devicetree. This allows recall of the `struct fw_config` which was probed.
BUG=b:161963281 TEST=tested with follower patch
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I0d014206a4ee6cc7592e12e704a7708652330eaf --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 51 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/44782/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/4/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/4/src/lib/fw_config.c@95 PS4, Line 95: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg) that open brace { should be on the previous line
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44782
to look at the new patch set (#5).
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
fw_config: Add caching to successfully probed fields
Add a backing cache for all successfully probed fw_config fields that originated as `probe` statements in the devicetree. This allows recall of the `struct fw_config` which was probed.
BUG=b:161963281 TEST=tested with follower patch
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I0d014206a4ee6cc7592e12e704a7708652330eaf --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/44782/5
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44782/3/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/44782/3/src/include/fw_config.h@48 PS3, Line 48: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg);
For the use case you have, wouldn't a fw_config_get_cached(field_mask) API be more useful? You're ju […]
Yeah, they're small functions, how about I just leave both of them here.
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c@78 PS3, Line 78: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg)
I think you should move this under the #if ENV_RAMSTAGE block, since it can't possibly do anything u […]
Done
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c@91 PS3, Line 91: return __ffs(probe->mask);
This can return -1: […]
The constants intended to be used here are generated in static.h and I believe sconfig will barf on fields that have zero bits, but I can add a check here.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/4/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/4/src/lib/fw_config.c@85 PS4, Line 85: mask Something to keep in mind is that not everything would necessarily be a boolean. It could be value based. This code would still work, but we may want to call that out.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/5/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/5/src/lib/fw_config.c@96 PS5, Line 96: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg) that open brace { should be on the previous line
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/4/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/4/src/lib/fw_config.c@85 PS4, Line 85: mask
Something to keep in mind is that not everything would necessarily be a boolean. […]
There is a comment in the .h file, you're supposed to use, for a volteer example, `FW_CONFIG_FIELD_AUDIO_MASK` which is 0x700. I could rename the variable, something like `field_mask` to be a little more helpful.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44782
to look at the new patch set (#6).
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
fw_config: Add caching to successfully probed fields
Add a backing cache for all successfully probed fw_config fields that originated as `probe` statements in the devicetree. This allows recall of the `struct fw_config` which was probed.
BUG=b:161963281 TEST=tested with follower patch
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I0d014206a4ee6cc7592e12e704a7708652330eaf --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/44782/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/6/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/6/src/lib/fw_config.c@96 PS6, Line 96: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg) that open brace { should be on the previous line
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44782
to look at the new patch set (#7).
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
fw_config: Add caching to successfully probed fields
Add a backing cache for all successfully probed fw_config fields that originated as `probe` statements in the devicetree. This allows recall of the `struct fw_config` which was probed.
BUG=b:161963281 TEST=tested with follower patch
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I0d014206a4ee6cc7592e12e704a7708652330eaf --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 52 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/44782/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44782/7/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/7/src/lib/fw_config.c@96 PS7, Line 96: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg) that open brace { should be on the previous line
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 7: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44782/3/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/44782/3/src/include/fw_config.h@48 PS3, Line 48: void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg);
Yeah, they're small functions, how about I just leave both of them here.
Done
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/3/src/lib/fw_config.c@91 PS3, Line 91: return __ffs(probe->mask);
The constants intended to be used here are generated in static. […]
Done
https://review.coreboot.org/c/coreboot/+/44782/4/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/44782/4/src/lib/fw_config.c@85 PS4, Line 85: mask
There is a comment in the . […]
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44782 )
Change subject: fw_config: Add caching to successfully probed fields ......................................................................
fw_config: Add caching to successfully probed fields
Add a backing cache for all successfully probed fw_config fields that originated as `probe` statements in the devicetree. This allows recall of the `struct fw_config` which was probed.
BUG=b:161963281 TEST=tested with follower patch
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I0d014206a4ee6cc7592e12e704a7708652330eaf Reviewed-on: https://review.coreboot.org/c/coreboot/+/44782 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/include/fw_config.h M src/lib/fw_config.c 2 files changed, 52 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/include/fw_config.h b/src/include/fw_config.h index d41afd6..81980b9 100644 --- a/src/include/fw_config.h +++ b/src/include/fw_config.h @@ -40,6 +40,21 @@ */ bool fw_config_probe(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 + */ +void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg); + +/** + * 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 + * + * Return pointer to cached `struct fw_config` if successfully probed, otherwise NULL. +*/ +const struct fw_config *fw_config_get_found(uint32_t field_mask); + #else
static inline bool fw_config_probe(const struct fw_config *match) diff --git a/src/lib/fw_config.c b/src/lib/fw_config.c index e97cfdc..fdfab0a 100644 --- a/src/lib/fw_config.c +++ b/src/lib/fw_config.c @@ -1,11 +1,13 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <assert.h> #include <bootstate.h> #include <cbfs.h> #include <console/console.h> #include <device/device.h> #include <ec/google/chromeec/ec.h> #include <fw_config.h> +#include <lib.h> #include <stdbool.h> #include <stdint.h>
@@ -66,6 +68,40 @@ }
#if ENV_RAMSTAGE + +/* + * The maximum number of fw_config fields is limited by the 32-bit mask that is used to + * represent them. + */ +#define MAX_CACHE_ELEMENTS (8 * sizeof(uint32_t)) + +static const struct fw_config *cached_configs[MAX_CACHE_ELEMENTS]; + +static size_t probe_index(uint32_t mask) +{ + assert(mask); + return __ffs(mask); +} + +const struct fw_config *fw_config_get_found(uint32_t field_mask) +{ + const struct fw_config *config; + config = cached_configs[probe_index(field_mask)]; + if (config && config->mask == field_mask) + return config; + + return NULL; +} + +void fw_config_for_each_found(void (*cb)(const struct fw_config *config, void *arg), void *arg) +{ + size_t i; + + for (i = 0; i < MAX_CACHE_ELEMENTS; ++i) + if (cached_configs[i]) + cb(cached_configs[i], arg); +} + static void fw_config_init(void *unused) { struct device *dev; @@ -80,6 +116,7 @@ for (probe = dev->probe_list; probe && probe->mask != 0; probe++) { if (fw_config_probe(probe)) { match = true; + cached_configs[probe_index(probe->mask)] = probe; break; } }
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Caveh Jalali (Code Review)
-
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Tim Wawrzynczak (Code Review)