Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: mb/ocp/deltalake: Set coreboot log level via VPD variable ......................................................................
mb/ocp/deltalake: Set coreboot log level via VPD variable
If the VPD variable 'coreboot_log_level' is not found, will set to COREBOOT_LOG_LEVEL_DEFAULT.
Tested=On OCP Delta Lake, coreboot log level can be changed via VPD.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/mainboard/ocp/deltalake/Kconfig M src/mainboard/ocp/deltalake/Makefile.inc A src/mainboard/ocp/deltalake/loglevel.c M src/mainboard/ocp/deltalake/vpd.h 4 files changed, 31 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/1
diff --git a/src/mainboard/ocp/deltalake/Kconfig b/src/mainboard/ocp/deltalake/Kconfig index b229c94..9d8f5d8 100644 --- a/src/mainboard/ocp/deltalake/Kconfig +++ b/src/mainboard/ocp/deltalake/Kconfig @@ -3,6 +3,7 @@ config BOARD_SPECIFIC_OPTIONS def_bool y select BOARD_ROMSIZE_KB_65536 + select CONSOLE_OVERRIDE_LOGLEVEL select FSP_CAR select HAVE_ACPI_TABLES select MAINBOARD_USES_FSP2_0 diff --git a/src/mainboard/ocp/deltalake/Makefile.inc b/src/mainboard/ocp/deltalake/Makefile.inc index be6af24..262d6c0 100644 --- a/src/mainboard/ocp/deltalake/Makefile.inc +++ b/src/mainboard/ocp/deltalake/Makefile.inc @@ -1,11 +1,14 @@ ## SPDX-License-Identifier: GPL-2.0-or-later
bootblock-y += bootblock.c - +bootblock-$(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) += loglevel.c +postcar-$(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) += loglevel.c romstage-y += romstage.c +romstage-$(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) += loglevel.c romstage-$(CONFIG_IPMI_KCS_ROMSTAGE) += ipmi.c
ramstage-y += ramstage.c ipmi.c +ramstage-$(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) += loglevel.c ramstage-$(CONFIG_HAVE_ACPI_TABLES) += fadt.c
CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/include diff --git a/src/mainboard/ocp/deltalake/loglevel.c b/src/mainboard/ocp/deltalake/loglevel.c new file mode 100644 index 0000000..fb00f8d --- /dev/null +++ b/src/mainboard/ocp/deltalake/loglevel.c @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <console/console.h> +#include <drivers/vpd/vpd.h> +#include <string.h> + +#include "vpd.h" + +int get_console_loglevel(void) +{ + uint8_t val; + char val_str[VPD_LEN]; + + if (vpd_gets(COREBOOT_LOG_LEVEL, val_str, VPD_LEN, VPD_RW_THEN_RO)) { + val = (uint8_t)atol(val_str); + if (val > 8) + return COREBOOT_LOG_LEVEL_DEFAULT; + return val; + } else { + return COREBOOT_LOG_LEVEL_DEFAULT; + } +} diff --git a/src/mainboard/ocp/deltalake/vpd.h b/src/mainboard/ocp/deltalake/vpd.h index ae2099d..43070c2 100644 --- a/src/mainboard/ocp/deltalake/vpd.h +++ b/src/mainboard/ocp/deltalake/vpd.h @@ -32,4 +32,8 @@ #define FSP_DCI "fsp_dci_enable" /* 1 or 0: enable or disable DCI */ #define FSP_DCI_DEFAULT 0 /* Default value when the VPD variable is not found */
+/* coreboot log level */ +#define COREBOOT_LOG_LEVEL "coreboot_log_level" +#define COREBOOT_LOG_LEVEL_DEFAULT 4 + #endif
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: mb/ocp/deltalake: Set coreboot log level via VPD variable ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/45134/1/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45134/1/src/mainboard/ocp/deltalake... PS1, Line 4: bootblock-$(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) += loglevel.c Instead of adding four lines, use `all`:
all-$(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) += loglevel.c
https://review.coreboot.org/c/coreboot/+/45134/1/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/loglevel.c:
https://review.coreboot.org/c/coreboot/+/45134/1/src/mainboard/ocp/deltalake... PS1, Line 16: val > 8 val >= BIOS_NEVER
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#2).
Change subject: mb/ocp/deltalake: Set coreboot log level via VPD variable ......................................................................
mb/ocp/deltalake: Set coreboot log level via VPD variable
If the VPD variable 'coreboot_log_level' is not found, will set to COREBOOT_LOG_LEVEL_DEFAULT.
Tested=On OCP Delta Lake, coreboot log level can be changed via VPD.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/mainboard/ocp/deltalake/Kconfig M src/mainboard/ocp/deltalake/Makefile.inc A src/mainboard/ocp/deltalake/loglevel.c M src/mainboard/ocp/deltalake/vpd.h 4 files changed, 28 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/2
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: mb/ocp/deltalake: Set coreboot log level via VPD variable ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45134/1/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45134/1/src/mainboard/ocp/deltalake... PS1, Line 4: bootblock-$(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) += loglevel.c
Instead of adding four lines, use `all`: […]
Done
https://review.coreboot.org/c/coreboot/+/45134/1/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/loglevel.c:
https://review.coreboot.org/c/coreboot/+/45134/1/src/mainboard/ocp/deltalake... PS1, Line 16: val > 8
val >= BIOS_NEVER
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: mb/ocp/deltalake: Set coreboot log level via VPD variable ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: mb/ocp/deltalake: Set coreboot log level via VPD variable ......................................................................
Patch Set 2:
I guess this could be moved to some generic location?
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: mb/ocp/deltalake: Set coreboot log level via VPD variable ......................................................................
Patch Set 2:
Patch Set 2:
I guess this could be moved to some generic location?
Maybe drivers/vpd or src/console and add a new config option for this?
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: mb/ocp/deltalake: Set coreboot log level via VPD variable ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
I guess this could be moved to some generic location?
Maybe drivers/vpd or src/console and add a new config option for this?
src/console makes more sense to me. The input may be come from VPD, or something else (depending on platform). So: * Use the CONFIG_DEFAULT_CONSOLE_LOGLEVEL_* as default. * If CONFIG_CONSOLE_LOGLEVEL_OVERRIDE is defined, call a platform specific function to get the desired log level, and use it. * Deltalake platform code implements the platform specific function which queries COREBOOT_LOG_LEVEL vpd variable.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#3).
Change subject: console: Override coreboot log level via VPD variable ......................................................................
console: Override coreboot log level via VPD variable
Add config CONSOLE_VPD_OVERRIDE_LOGLEVEL that overrides loglevel from VPD variable 'coreboot_log_level' when selected. It will look for it in RW_VPD first then RO_VPD, if both places cannot find it the loglevel will be set to CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL.
Tested=On OCP Delta Lake, coreboot log level can be changed via VPD.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/console/Kconfig M src/console/Makefile.inc A src/console/loglevel_vpd.c 3 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/3
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 3:
Patch Set 2:
Patch Set 2:
Patch Set 2:
I guess this could be moved to some generic location?
Maybe drivers/vpd or src/console and add a new config option for this?
src/console makes more sense to me. The input may be come from VPD, or something else (depending on platform). So:
- Use the CONFIG_DEFAULT_CONSOLE_LOGLEVEL_* as default.
- If CONFIG_CONSOLE_LOGLEVEL_OVERRIDE is defined, call a platform specific function to get the desired log level, and use it.
- Deltalake platform code implements the platform specific function which queries COREBOOT_LOG_LEVEL vpd variable.
CONFIG_CONSOLE_LOGLEVEL_OVERRIDE and CONFIG_DEFAULT_CONSOLE_LOGLEVEL_* are mutual exclusive, so I need to add CONSOLE_VPD_DEFAULT_LOGLEVEL as the fallback default loglevel. Added loglevel_vpd.c to be the general code instead of mainboard-specific implementation per Patrick's request.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 2:
Patch Set 2:
Patch Set 2:
I guess this could be moved to some generic location?
Maybe drivers/vpd or src/console and add a new config option for this?
src/console makes more sense to me. The input may be come from VPD, or something else (depending on platform). So:
- Use the CONFIG_DEFAULT_CONSOLE_LOGLEVEL_* as default.
- If CONFIG_CONSOLE_LOGLEVEL_OVERRIDE is defined, call a platform specific function to get the desired log level, and use it.
- Deltalake platform code implements the platform specific function which queries COREBOOT_LOG_LEVEL vpd variable.
CONFIG_CONSOLE_LOGLEVEL_OVERRIDE and CONFIG_DEFAULT_CONSOLE_LOGLEVEL_* are mutual exclusive, so I need to add CONSOLE_VPD_DEFAULT_LOGLEVEL as the fallback default loglevel. Added loglevel_vpd.c to be the general code instead of mainboard-specific implementation per Patrick's request.
We can do this to make them not mutually exclusive, and not to include another Kconfig option: * Use the CONFIG_DEFAULT_CONSOLE_LOGLEVEL_* as default, if CONFIG_CONSOLE_LOGLEVEL_OVERRIDE is not defined. * If CONFIG_CONSOLE_LOGLEVEL_OVERRIDE is defined, call a platform specific function to get the desired log level, and use it. ** If the platform specific function does not return a desired log level, fall back to the log level defined by CONFIG_DEFAULT_CONSOLE_LOGLEVEL_*. * Deltalake platform code implements the platform specific function which queries COREBOOT_LOG_LEVEL vpd variable.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 3: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig@319 PS3, Line 319: bool Should we provide a user-visible prompt for this? Note that if we do, we should not select this option from other places, but have it `default y` instead.
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig@330 PS3, Line 330: default 7 Ideally, this should be constrained to the valid loglevel values. It would be nice if we could reuse the prompt right below. Currently, it is hidden when CONSOLE_OVERRIDE_LOGLEVEL is selected, but we can change that:
if !CONSOLE_OVERRIDE_LOGLEVEL || CONSOLE_VPD_OVERRIDE_LOGLEVEL
choice
[...]
endchoice
config DEFAULT_CONSOLE_LOGLEVEL int default 0 if DEFAULT_CONSOLE_LOGLEVEL_0 default 1 if DEFAULT_CONSOLE_LOGLEVEL_1 default 2 if DEFAULT_CONSOLE_LOGLEVEL_2 default 3 if DEFAULT_CONSOLE_LOGLEVEL_3 default 4 if DEFAULT_CONSOLE_LOGLEVEL_4 default 5 if DEFAULT_CONSOLE_LOGLEVEL_5 default 6 if DEFAULT_CONSOLE_LOGLEVEL_6 default 7 if DEFAULT_CONSOLE_LOGLEVEL_7 default 8 if DEFAULT_CONSOLE_LOGLEVEL_8 help Map the log level config names to an integer.
endif ## !CONSOLE_OVERRIDE_LOGLEVEL || CONSOLE_VPD_OVERRIDE_LOGLEVEL
Then, the CONSOLE_VPD_DEFAULT_LOGLEVEL symbol can be replaced with DEFAULT_CONSOLE_LOGLEVEL.
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/loglevel_vpd.c File src/console/loglevel_vpd.c:
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/loglevel_vpd.c@... PS3, Line 12: uint8_t To use this type, you should include it: #include <stdint.h>
However, if we're returning an int, we might as well use an int here for consistency.
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/loglevel_vpd.c@... PS3, Line 12: uint8_t val; : char val_str[VPD_LEN]; : : if (vpd_gets(COREBOOT_LOG_LEVEL, val_str, VPD_LEN, VPD_RW_THEN_RO)) { : val = (uint8_t)atol(val_str); : if (val >= BIOS_NEVER) : return CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; : return val; : } else { : return CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; : } I'd say this would look better with a single `return` statement:
int log_level = CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; char val_str[VPD_LEN];
if (vpd_gets(COREBOOT_LOG_LEVEL, val_str, VPD_LEN, VPD_RW_THEN_RO)) { log_level = atol(val_str); if (log_level < 0 || log_level >= BIOS_NEVER) log_level = CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; } return log_level;
Also, we can rename `val` to `log_level` for clarity.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 3: Code-Review+1
Patch Set 3:
Patch Set 3:
Patch Set 2:
Patch Set 2:
Patch Set 2:
I guess this could be moved to some generic location?
Maybe drivers/vpd or src/console and add a new config option for this?
src/console makes more sense to me. The input may be come from VPD, or something else (depending on platform). So:
- Use the CONFIG_DEFAULT_CONSOLE_LOGLEVEL_* as default.
- If CONFIG_CONSOLE_LOGLEVEL_OVERRIDE is defined, call a platform specific function to get the desired log level, and use it.
- Deltalake platform code implements the platform specific function which queries COREBOOT_LOG_LEVEL vpd variable.
CONFIG_CONSOLE_LOGLEVEL_OVERRIDE and CONFIG_DEFAULT_CONSOLE_LOGLEVEL_* are mutual exclusive, so I need to add CONSOLE_VPD_DEFAULT_LOGLEVEL as the fallback default loglevel. Added loglevel_vpd.c to be the general code instead of mainboard-specific implementation per Patrick's request.
We can do this to make them not mutually exclusive, and not to include another Kconfig option:
- Use the CONFIG_DEFAULT_CONSOLE_LOGLEVEL_* as default, if CONFIG_CONSOLE_LOGLEVEL_OVERRIDE is not defined.
- If CONFIG_CONSOLE_LOGLEVEL_OVERRIDE is defined, call a platform specific function to get the desired log level, and use it.
** If the platform specific function does not return a desired log level, fall back to the log level defined by CONFIG_DEFAULT_CONSOLE_LOGLEVEL_*.
- Deltalake platform code implements the platform specific function which queries COREBOOT_LOG_LEVEL vpd variable.
As committed through 4ecd42f9b52cd01b659c, DEFAULT_CONSOLE_LOGLEVEL_* are defined only when CONSOLE_OVERRIDE_LOGLEVEL is not selected. So I agree to add CONSOLE_VPD_DEFAULT_LOGLEVEL as the fallback default log level.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#4).
Change subject: console: Override coreboot log level via VPD variable ......................................................................
console: Override coreboot log level via VPD variable
Add config CONSOLE_VPD_OVERRIDE_LOGLEVEL that overrides loglevel from VPD variable 'coreboot_log_level' when selected. It will look for it in RW_VPD first then RO_VPD, if both places cannot find it the loglevel will be set to CONFIG_DEFAULT_CONSOLE_LOGLEVEL.
Tested=On OCP Delta Lake, coreboot log level can be changed via VPD.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/console/Kconfig M src/console/Makefile.inc A src/console/loglevel_vpd.c 3 files changed, 34 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/4
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig@319 PS3, Line 319: bool
Should we provide a user-visible prompt for this? Note that if we do, we should not select this opti […]
Updated, but I still set default n because most people don't want to enable this by default.
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig@330 PS3, Line 330: default 7
Ideally, this should be constrained to the valid loglevel values. […]
Yes it's better if we can reuse DEFAULT_CONSOLE_LOGLEVEL, thanks.
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/loglevel_vpd.c File src/console/loglevel_vpd.c:
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/loglevel_vpd.c@... PS3, Line 12: uint8_t
To use this type, you should include it: #include <stdint.h> […]
Done
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/loglevel_vpd.c@... PS3, Line 12: uint8_t val; : char val_str[VPD_LEN]; : : if (vpd_gets(COREBOOT_LOG_LEVEL, val_str, VPD_LEN, VPD_RW_THEN_RO)) { : val = (uint8_t)atol(val_str); : if (val >= BIOS_NEVER) : return CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; : return val; : } else { : return CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; : }
I'd say this would look better with a single `return` statement: […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
LGTM, thanks!
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig@319 PS3, Line 319: bool
Updated, but I still set default n because most people don't want to enable this by default.
Sounds good. Sorry if I explained myself poorly. What I meant with `default y` is that Deltalake should use that instead of `select CONSOLE_VPD_OVERRIDE_LOGLEVEL`, since this allows one to easily bypass VPD settings when building test images.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig@319 PS4, Line 319: loglevel log level (as in the choice prompt below – above it’s written incorrectly in the description)
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig@324 PS4, Line 324: loglevel log level
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig@324 PS4, Line 324: will : look for it in RW_VPD first then RO_VPD, if both places cannot find : it the loglevel will be set to CONFIG_DEFAULT_CONSOLE_LOGLEVEL. Maybe:
The RW_VPD is checked first, and then the RO_VPD. If no settings are found, CONFIG_DEFAULT_CONSOLE_LOGLEVEL will be used.
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig@326 PS4, Line 326: loglevel log level
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 4:
The design with `CONSOLE_OVERRIDE_LOGLEVEL` looks a little strange. Can’t it be implemented the same as NVRAM/CMOS (User Option Table)? If VPD is selected, always check for the log level setting in VPD.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 4:
Patch Set 4:
The design with `CONSOLE_OVERRIDE_LOGLEVEL` looks a little strange. Can’t it be implemented the same as NVRAM/CMOS (User Option Table)? If VPD is selected, always check for the log level setting in VPD.
Not all users of VPD store the log level in VPD. And I think NVRAM/CMOS wouldn't a good idea, because that can't be updated through the BMC interface (on OCP servers).
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 4:
The design with `CONSOLE_OVERRIDE_LOGLEVEL` looks a little strange. Can’t it be implemented the same as NVRAM/CMOS (User Option Table)? If VPD is selected, always check for the log level setting in VPD.
Not all users of VPD store the log level in VPD. And I think NVRAM/CMOS wouldn't a good idea, because that can't be updated through the BMC interface (on OCP servers).
Implementing the API like the one used for CMOS doesn't imply to use CMOS...
I agree that CONSOLE_OVERRIDE_LOGLEVEL is just an odd hack and obviously doesn't scale (one can't mix override implementations). It would be better to unify option APIs. Trying to say it differently: nobody said that `get_option(..., "debug_level");` can only query CMOS.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 4: Code-Review+1
Patch Set 4:
The design with `CONSOLE_OVERRIDE_LOGLEVEL` looks a little strange. Can’t it be implemented the same as NVRAM/CMOS (User Option Table)? If VPD is selected, always check for the log level setting in VPD.
Not all users of VPD store the log level in VPD. And I think NVRAM/CMOS wouldn't a good idea, because that can't be updated through the BMC interface (on OCP servers).
Implementing the API like the one used for CMOS doesn't imply to use CMOS...
I agree that CONSOLE_OVERRIDE_LOGLEVEL is just an odd hack and obviously doesn't scale (one can't mix override implementations). It would be better to unify option APIs. Trying to say it differently: nobody said that `get_option(..., "debug_level");` can only query CMOS.
Good idea. Thank you for saying it crystal clear.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#5).
Change subject: console: Override coreboot log level via VPD variable ......................................................................
console: Override coreboot log level via VPD variable
Add config CONSOLE_VPD_OVERRIDE_LOGLEVEL that overrides loglevel from VPD variable 'coreboot_log_level' when selected. It will look for it in RW_VPD first then RO_VPD, if both places cannot find it the loglevel will be set to CONFIG_DEFAULT_CONSOLE_LOGLEVEL.
Tested=On OCP Delta Lake, coreboot log level can be changed via VPD.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/console/Kconfig M src/console/Makefile.inc A src/console/loglevel_vpd.c 3 files changed, 34 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/5
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#6).
Change subject: console: Override coreboot log level via VPD variable ......................................................................
console: Override coreboot log level via VPD variable
Add config CONSOLE_VPD_OVERRIDE_LOGLEVEL that overrides log level from VPD variable 'coreboot_log_level' when selected. It will look for it in RW_VPD first then RO_VPD, if both places cannot find it the log level will be set to CONFIG_DEFAULT_CONSOLE_LOGLEVEL.
Tested=On OCP Delta Lake, coreboot log level can be changed via VPD.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/console/Kconfig M src/console/Makefile.inc A src/console/loglevel_vpd.c 3 files changed, 34 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/6
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig@319 PS4, Line 319: loglevel
log level (as in the choice prompt below – above it’s written incorrectly in the description)
Done
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig@324 PS4, Line 324: loglevel
log level
Done
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig@326 PS4, Line 326: loglevel
log level
Done
https://review.coreboot.org/c/coreboot/+/45134/4/src/console/Kconfig@324 PS4, Line 324: will : look for it in RW_VPD first then RO_VPD, if both places cannot find : it the loglevel will be set to CONFIG_DEFAULT_CONSOLE_LOGLEVEL.
Maybe: […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45134/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45134/7//COMMIT_MSG@12 PS7, Line 12: the log level will be set to CONFIG_DEFAULT_CONSOLE_LOGLEVEL. Please read Nico’s comment. Your current approach should not be excepted, and you should extend the code, so `get_option(…)` can be used.
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45134/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45134/7//COMMIT_MSG@12 PS7, Line 12: the log level will be set to CONFIG_DEFAULT_CONSOLE_LOGLEVEL.
Please read Nico’s comment. […]
So I should move the get_console_loglevel() into get_option() to unify the function interface, right? My question is in src/console/init.c get_log_level(), get_option() seems cannot be used when CONSOLE_LEVEL_CONST is true, it doesn't boot. Also it can be very inefficient because this function got called in each printk and it tries to read cmos every time, any suggestion? Thanks. - if (CONSOLE_LEVEL_CONST) - return get_console_loglevel(); + if (CONSOLE_LEVEL_CONST) { + int debug_level = CONFIG_DEFAULT_CONSOLE_LOGLEVEL; + get_option(&debug_level, "debug_level"); + return debug_level; + }
Hello Marc Jones, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#8).
Change subject: console: Unify overriding console log level with get_option() ......................................................................
console: Unify overriding console log level with get_option()
1. Integrate get_console_loglevel() into get_option(), if both USE_OPTION_TABLE and CONSOLE_OVERRIDE_LOGLEVEL are selected, log level will be set by USE_OPTION_TABLE.
2. Remove CONSOLE_LEVEL_CONST and let init_log_level() set log level for all stages. get_log_level() is called by printk so avoid calling an overridden function in case it impacts performance.
3.CONFIG_DEFAULT_CONSOLE_LOGLEVEL can still be used when CONSOLE_OVERRIDE_LOGLEVEL is selected.
Tested=On OCP Delta Lake, coreboot log level can be overridden.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/console/Kconfig M src/console/Makefile.inc M src/console/init.c A src/console/option.c M src/include/console/console.h M src/include/option.h 6 files changed, 25 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/8
Hello build bot (Jenkins), Marc Jones, Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#9).
Change subject: console: Unify overriding console log level with get_option() ......................................................................
console: Unify overriding console log level with get_option()
1. Integrate get_console_loglevel() into get_option(), if both USE_OPTION_TABLE and CONSOLE_OVERRIDE_LOGLEVEL are selected, log level will be set by USE_OPTION_TABLE.
2. Remove CONSOLE_LEVEL_CONST and let init_log_level() set log level for all stages. get_log_level() is called by printk so avoid calling an overridden function in case it impacts performance.
3.CONFIG_DEFAULT_CONSOLE_LOGLEVEL can still be used when CONSOLE_OVERRIDE_LOGLEVEL is selected.
Tested=On OCP Delta Lake, coreboot log level can be overridden.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/console/Kconfig M src/console/Makefile.inc M src/console/init.c A src/console/option.c M src/include/console/console.h M src/include/option.h 6 files changed, 24 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/9
Hello build bot (Jenkins), Marc Jones, Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#10).
Change subject: console: Unify overriding console log level with get_option() ......................................................................
console: Unify overriding console log level with get_option()
1. Integrate get_console_loglevel() into get_option(), if both USE_OPTION_TABLE and CONSOLE_OVERRIDE_LOGLEVEL are selected, log level will be set by USE_OPTION_TABLE.
2. Remove CONSOLE_LEVEL_CONST and let init_log_level() set log level for all stages. get_log_level() is called by printk so avoid calling an overridden function in case it impacts performance.
3.CONFIG_DEFAULT_CONSOLE_LOGLEVEL can still be used when CONSOLE_OVERRIDE_LOGLEVEL is selected.
Tested=On OCP Delta Lake, coreboot log level can be overridden.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/console/Kconfig M src/console/Makefile.inc M src/console/init.c A src/console/option.c M src/include/console/console.h M src/include/option.h 6 files changed, 24 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/10
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Unify overriding console log level with get_option() ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45134/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45134/7//COMMIT_MSG@12 PS7, Line 12: the log level will be set to CONFIG_DEFAULT_CONSOLE_LOGLEVEL.
So I should move the get_console_loglevel() into get_option() to unify the function interface, right […]
In order to set log level before POST CAR, on Cooper Lake I can remove the check for CONSOLE_LEVEL_CONST and let init_log_level() set the log level, not sure if it works for other platforms/architectures. Or maybe add an option for allowing changing log level before POST CAR and default set to n.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Unify overriding console log level with get_option() ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h File src/include/option.h:
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h@26 PS10, Line 26: return cmos_get_option(dest, name); Similarly, we could have here:
if (CONFIG(VPD_OPTIONS)) /* opt-in for trying to find options in VPD */ return vpd_get_option(dest, name);
Then, a platform using VPD for options wouldn't have to use hacks like CONSOLE_OVERRIDE_LOGLEVEL at all.
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h@30 PS10, Line 30: return get_console_override_option(dest, name); I don't understand why this needs another function. We could also have the code directly here:
if (CONFIG(CONSOLE_OVERRIDE_LOGLEVEL) && ...) { *(int *)dest = get_console_loglevel(); return CB_SUCCESS; }
However, checks on the option `name` don't belong here. So I would keep the `if (CONFIG(CONSOLE_OVERRIDE_LOGLEVEL))` in `console/init.c`. Or, alternatively, move it to the implementation. For instance, get rid of `get_console_loglevel()` and create a generic mainboard get_option() instead, which would leave us here with
if (CONFIG(MAINBOARD_OPTIONS)) return mainboard_get_option(dest, name);
There's only a single implementation of `get_console_loglevel()`, so that wouldn't be much work. That one could then implement the `strcmp(name, "debug_level")` check.
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h@32 PS10, Line 32: } If we decide for a normalized
if (CONFIG(XXX)) return xxx_get_option();
the order should probably be different. For the one get_console_loglevel() implementation, it was intended that it take precedence.
Hello build bot (Jenkins), Marc Jones, Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#11).
Change subject: console: Unify overriding console log level with get_option() ......................................................................
console: Unify overriding console log level with get_option()
1. 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.
2. Remove CONSOLE_LEVEL_CONST and let init_log_level() set log level for all stages. get_log_level() is called by printk so avoid calling an overridden function in case it impacts performance.
Tested=On OCP Delta Lake, coreboot log level can be overridden by mainboard function.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/console/Kconfig M src/console/init.c M src/include/console/console.h M src/include/option.h M src/lib/Makefile.inc A src/lib/option.c M src/mainboard/scaleway/tagada/Kconfig 8 files changed, 29 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/11
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Unify overriding console log level with get_option() ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h File src/include/option.h:
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h@26 PS10, Line 26: return cmos_get_option(dest, name);
Similarly, we could have here: […]
I added a generic mainboard_get_option() because mainboard may not choose to use VPD.
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h@30 PS10, Line 30: return get_console_override_option(dest, name);
I don't understand why this needs another function. We could also have the […]
Done
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h@32 PS10, Line 32: }
If we decide for a normalized […]
Done
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Unify overriding console log level with get_option() ......................................................................
Patch Set 11: Code-Review-1
(3 comments)
I think that this has a couple of issues, but it headed the right direction.
The commit should be split.
This probably breaks arm and other xip platforms and needs to be checked.
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. Add a description of how this works in the Documentation/ since it is a framework for going forward.
This comment could be reworded with that in mind
https://review.coreboot.org/c/coreboot/+/45134/11//COMMIT_MSG@17 PS11, Line 17: performance. This is a nice cleanup and could be a separate commit.
https://review.coreboot.org/c/coreboot/+/45134/11/src/console/init.c File src/console/init.c:
https://review.coreboot.org/c/coreboot/+/45134/11/src/console/init.c@30 PS11, Line 30: The following will break on !ENV_STAGE_HAS_DATA_SECTION where the variable can't be set (ARM and other XIP with no CAR or RAM). I think you still need (CONSOLE_LEVEL_CONST)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Unify overriding console log level with get_option() ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45134/11/src/console/init.c File src/console/init.c:
https://review.coreboot.org/c/coreboot/+/45134/11/src/console/init.c@30 PS11, Line 30:
The following will break on !ENV_STAGE_HAS_DATA_SECTION where the variable can't be set (ARM and oth […]
AIUI, `.bss` would be used for `console_loglevel` because the initialization is dropped. And I don't know if there is any platform left that doesn't support it. x86 pre-RAM is the only case without `.data` according to `rules.h`.
But I agree that it should be discussed in a separate change.
Hello Marc Jones, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, Julien Viard de Galbert, insomniac, Jingle Hsu, Angel Pons, Rocky Phagura, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45134
to look at the new patch set (#12).
Change subject: lib: Add config MAINBOARD_OPTIONS for getting options from mainboard ......................................................................
lib: Add config MAINBOARD_OPTIONS for getting options from mainboard
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.
Change-Id: I278e392bed178df7a8cdb90685963c1fedf0bcc4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/console/Kconfig M src/include/console/console.h M src/include/option.h M src/lib/Makefile.inc A src/lib/option.c M src/mainboard/scaleway/tagada/Kconfig 7 files changed, 28 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/45134/12
Johnny Lin 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:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45134/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45134/7//COMMIT_MSG@12 PS7, Line 12: the log level will be set to CONFIG_DEFAULT_CONSOLE_LOGLEVEL.
In order to set log level before POST CAR, on Cooper Lake I can remove the check for CONSOLE_LEVEL_C […]
Add a config in CB:45684, so keep the current behavior the same.
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.
Add a description of how this works in the Documentation/ since it is a framework for going forward. […]
I was trying to add to the file describing get_option but could not find it, any suggestion where I should add this?
https://review.coreboot.org/c/coreboot/+/45134/11//COMMIT_MSG@17 PS11, Line 17: performance.
This is a nice cleanup and could be a separate commit.
Split to CB:45684
https://review.coreboot.org/c/coreboot/+/45134/11/src/console/init.c File src/console/init.c:
https://review.coreboot.org/c/coreboot/+/45134/11/src/console/init.c@30 PS11, Line 30:
AIUI, `.bss` would be used for `console_loglevel` because the initialization […]
Add a config in CB:45684, so keep the current behavior the same.
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
Martin Roth 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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45134/12/src/include/option.h File src/include/option.h:
https://review.coreboot.org/c/coreboot/+/45134/12/src/include/option.h@25 PS12, Line 25: mainboard_get_option I guess I don't completely understand what the mainboard_get_option is supposed to be. Is there a reason not to make it specific to the method of getting the options? A mainboard specific method seems like it's going to further fragment the option-getting methods when we could be consolidating them.
What I'd personally like to see, and I understand that it's outside of the scope of this patch, a well enumerated list of possible places to get options from: - boot_rom_get_option - gpio_get_option - i2c_get_option ???
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 […]
I agree that it would make sense to let the mainboard choose what options to implement instead of setting "mainboard" options that are really fixed across all of coreboot.
This might make this particular patch a little more difficult since it's replacing a global option with one that is supposed to be mainboard specific, so it would have to be implemented in any platform that wanted to use the existing Kconfig option.
Johnny Lin 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45134/12/src/include/option.h File src/include/option.h:
https://review.coreboot.org/c/coreboot/+/45134/12/src/include/option.h@25 PS12, Line 25: mainboard_get_option
I guess I don't completely understand what the mainboard_get_option is supposed to be. […]
The purpose of the change was to integrate getting debug log level from a mainboard defined function with get_option() API, so a generic mainboard_get_option() was added in get_option(). But if it makes more sense to make it specific to the method of getting the option, I can revert mainboard_get_option and only focus on the console log level part. Something like below: console/init.c static void init_log_level(void) { int debug_level = CONFIG_DEFAULT_CONSOLE_LOGLEVEL; if(CONFIG(CONSOLE_OVERRIDE_LOGLEVEL)) debug_level = get_console_loglevel(); else get_option(&debug_level, "debug_level");
set_log_level(debug_level); } To limit the scope of this change I am leaning toward this direction, to integrate into get_option requires more discussion.
Nico Huber 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:
(1 comment)
There was some misunderstanding. I'll try to put my earlier proposition of unified get_option() implementations into some code, that should be less ambiguous.
https://review.coreboot.org/c/coreboot/+/45134/12/src/include/option.h File src/include/option.h:
https://review.coreboot.org/c/coreboot/+/45134/12/src/include/option.h@25 PS12, Line 25: mainboard_get_option
I guess I don't completely understand what the mainboard_get_option is supposed to be. Is there a reason not to make it specific to the method of getting the options? A mainboard specific method seems like it's going to further fragment the option-getting methods when we could be consolidating them.
It was my idea. To get the already existing get_console_loglevel() in line. Currently there is fragmentation on an orthogonal axis: the queried option. There is currently only one implementation (scaleway/tagada). I see no better way there as the queried value really comes from some mainboard specific code.
You are right, Martin, that this may encourage further mainboard-specific solutions, even where they aren't necessary. Still, it would really be nice to have a single get_option() API, for code like `console/init.c`.
Nico Huber 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:
There was some misunderstanding. I'll try to put my earlier proposition of unified get_option() implementations into some code, that should be less ambiguous.
This: https://review.coreboot.org/c/coreboot/+/45774/1/src/include/option.h
It would allow to keep the calling site clean: https://review.coreboot.org/c/coreboot/+/45765/1/src/console/init.c#22
Johnny Lin has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: lib: Add config MAINBOARD_OPTIONS for getting options from mainboard ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Johnny Lin (Code Review)
-
Jonathan Zhang (Code Review)
-
Marc Jones (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)