Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41209 )
Change subject: fw_config: Add firmware configuration interface ......................................................................
Patch Set 3:
(8 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/41209/3/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/41209/3/src/include/fw_config.h@19 PS3, Line 19: #define FW_CONFIG(__field, __option) &(const struct fw_config) { \
Macros with complex values should be enclosed in parentheses
Ack
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/Kconfig File src/lib/Kconfig:
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/Kconfig@86 PS3, Line 86: bool
I assume we want these to show up in menuconfig? Then they need prompt text.
Initially I was thinking just selected at the mainboard level since it defaulted to trying cbfs first, but I'll expose these to menuconfig since I'm adding separate enables.
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/Kconfig@94 PS3, Line 94: default "fw_config"
Should depends on FW_CONFIG (to hide in menuconfig when disabled).
Done
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/Makefile.inc File src/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/Makefile.inc@69 PS3, Line 69: romstage-$(CONFIG_FW_CONFIG) += fw_config.c
nit: why not all stages right away?
I'll add to bootblock and verstage.
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/fw_config.c@22 PS3, Line 22: if (cbfs_boot_locate(&file, CONFIG_FW_CONFIG_CBFS_FILE, &matchraw))
I think you can just use cbfs_boot_load_file() for all of this.
ah yes that is the shortcut I was looking for, all the examples I was looking at were doing it the long way.
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/fw_config.c@44 PS3, Line 44: if (fw_config_value)
Are these really guaranteed to always be non-zero? What if you just pick the first option for everyt […]
I can add a separate "initialized" flag to prevent reading every time if the value is zero.
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/fw_config.c@48 PS3, Line 48: if (fw_config_prepare_cbfs())
This would be a problem if we wanted to use it in early bootblock code (at least on Arm). […]
I have been going back and forth on this, it doesn't necessarily make sense to probe CBFS by default anyway if you don't need to.
https://review.coreboot.org/c/coreboot/+/41209/3/src/lib/fw_config.c@52 PS3, Line 52: if (CONFIG(EC_GOOGLE_CHROMEEC)) {
Shouldn't this be a separate Kconfig? Not all devices using CHROMEEC use a CBI, and not all of those […]
In theory they wouldn't enable CONFIG_FW_CONFIG without needing it, but it does make sense for both of these to be behind a kconfig.