Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: lib: Add config MAINBOARD_OPTIONS for getting options from mainboard ......................................................................
Patch Set 12:
(7 comments)
Some wording changes and other thoughts...
https://review.coreboot.org/c/coreboot/+/45134/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45134/11//COMMIT_MSG@9 PS11, Line 9: Add config MAINBOARD_OPTIONS and remove CONSOLE_OVERRIDE_LOGLEVEL, : it lets mainboard provide overridden function for getting configuration : values via get_option(). Currently it's only used for overriding : console log level.
I was trying to add to the file describing get_option but could not find it, any suggestion where I […]
It should match the code: Documentation/lib/mb_option.md and update the index.md.
https://review.coreboot.org/c/coreboot/+/45134/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45134/12//COMMIT_MSG@8 PS12, Line 8: : It's added into get_option() and when MAINBOARD_OPTIONS is selected, : it takes precedence and gets the config values from mainboard provided : overridden functions. Currently it's only used for overriding console : log level, so remove and replace config CONSOLE_OVERRIDE_LOGLEVEL with : it. I've reworded this slightly. Please feel free to use o change any of it.
"The MAINBOARD_OPTION library adds a mainboard call to the coreboot option table get_option() function. Normally, the coreboot option table is accessed in cmos, but a mainboard may have other mechanisms to store options. This replaces the need for additional config options for overriding options. This initial implementation supports console log level. Remove and replace CONSOLE_OVERRIDE_LOGLEVEL."
https://review.coreboot.org/c/coreboot/+/45134/12/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/45134/12/src/Kconfig@131 PS12, Line 131: bool "Use mainboard overridden functions for getting configuration values" "Use mainboard override functions"
https://review.coreboot.org/c/coreboot/+/45134/12/src/Kconfig@133 PS12, Line 133: Enable this option if coreboot shall read options from mainboard : overridden functions. Enable this option to use mainboard override functions for additional customization and configurations.
https://review.coreboot.org/c/coreboot/+/45134/12/src/lib/option.c File src/lib/option.c:
https://review.coreboot.org/c/coreboot/+/45134/12/src/lib/option.c@8 PS12, Line 8: : if (strcmp(name, "debug_level") == 0) { Another idea would be to make this a weak function and the mainboard implements the work of checking the option name and doing the override. I'd like other thoughts and opinions on this. Is there value in a option function dispatch here?
https://review.coreboot.org/c/coreboot/+/45134/12/src/lib/option.c@16 PS12, Line 16: } Maybe rename the files mb_option.*? option is a little generic to know what the file is for.
https://review.coreboot.org/c/coreboot/+/45134/12/src/mainboard/scaleway/tag... File src/mainboard/scaleway/tagada/Kconfig:
https://review.coreboot.org/c/coreboot/+/45134/12/src/mainboard/scaleway/tag... PS12, Line 9: EARLY_OVERRIDE_LOGLEVEL I don't think this is needed. See comments in CB:45684