Change in coreboot[master]: console/init: Drop CONSOLE_LEVEL_CONST

[Nico Huber (Code Review)]

Hello Martin Roth, Marc Jones, Johnny Lin, Angel Pons, Kyösti Mälkki,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/45765

to review the following change.

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

console/init: Drop CONSOLE_LEVEL_CONST

We limited the configurability of the debug level to stages that have
a `.data` section. This is not really a requirement, because a `.bss`
section should suffice and we always have that now.

Making the debug level always configurable, adds an early call to
get_option(). But that shouldn't hurt: The value is cached locally.

Change-Id: I11484fc32dcbba8d31772bd0b82785f17b2fba11
Signed-off-by: Nico Huber 
---
M src/console/init.c
1 file changed, 2 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/45765/1

diff --git a/src/console/init.c b/src/console/init.c
index 1dba9ad..9776e2a 100644
--- a/src/console/init.c
+++ b/src/console/init.c
@@ -8,40 +8,24 @@
 #include <option.h>
 #include <version.h>

-/* Mutable console log level only allowed when RAM comes online. */
-#define CONSOLE_LEVEL_CONST !ENV_STAGE_HAS_DATA_SECTION
-
 static int console_inited;
-static int console_loglevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL;
+static int console_loglevel;

static inline int get_log_level(void)
 {
    if (console_inited == 0)
    	return -1;
-	if (CONSOLE_LEVEL_CONST)
-		return get_console_loglevel();

return console_loglevel;
 }

-static inline void set_log_level(int new_level)
-{
-	if (CONSOLE_LEVEL_CONST)
-		return;
-
-	console_loglevel = new_level;
-}
-
 static void init_log_level(void)
 {
    int debug_level = get_console_loglevel();

-	if (CONSOLE_LEVEL_CONST)
-		return;
-
    get_option(&debug_level, "debug_level");

-	set_log_level(debug_level);
+	console_loglevel = debug_level;
 }

int console_log_level(int msg_level)

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45765/1/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/1/src/console/init.c@49 
PS1, Line 49: 	init_log_level();
You need to check the access control of lower/upper RTC banks (128 bytes each) in intel bootblocks gets done first? Or is it so that option_tables are still restricted to lower bank?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45765/1/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/1/src/console/init.c@49 
PS1, Line 49: 	init_log_level();
> You need to check the access control of lower/upper RTC banks (128 bytes each) in intel bootblocks g […]

I recall seeing something on `bootblock_southbridge_early_init` but I'm not sure if all platforms have it.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 1:

(1 comment)
> https://qa.coreboot.org/job/coreboot-gerrit/143008/ : UNSTABLE

So verstage lacks cmos_get_option(), but we are allowed to select that
combination in Kconfig. What would be the correct semantics? I don't
want to enable option support in something called `verstage` without
asking :)

https://review.coreboot.org/c/coreboot/+/45765/1/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/1/src/console/init.c@49 
PS1, Line 49: 	init_log_level();
> I recall seeing something on `bootblock_southbridge_early_init` but I'm not sure if all platforms ha […]

AFAIR, most of the common option-table code ignores the upper bank (e.g.
when restoring defaults, we copy up to 128B).

I've also checked the `cmos.layout` files. `debug_level` and the cheksum
are always below 1024 (bits).

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 1: Code-Review+1

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 1:
> Patch Set 1:
>
> (1 comment)
> > https://qa.coreboot.org/job/coreboot-gerrit/143008/ : UNSTABLE
>
> So verstage lacks cmos_get_option(), but we are allowed to select that
> combination in Kconfig. What would be the correct semantics? I don't
> want to enable option support in something called `verstage` without
> asking :)

You can add it in, but we would never use it on Chromebooks. As long as cmos_get_option() is optional and not unconditionally required then I don't see a problem.

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 4:

At the time I pushed I didn't notice there was another dependent change, now it's fixed and sorry about that.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c@26 
PS2, Line 26: 	get_option(&debug_level, "debug_level");
Julius made a good point on another patch that this adds complexity (that
may fail) before the first console is initialized. How about we skip this
for the first console? e.g.

#define FIRST_CONSOLE (ENV_BOOTBLOCK || CONFIG(NO_BOOTBLOCK_CONSOLE) && ENV_ROMSTAGE)

if (!FIRST_CONSOLE)
            get_option(...

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45765/1/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/1/src/console/init.c@49 
PS1, Line 49: 	init_log_level();
> AFAIR, most of the common option-table code ignores the upper bank (e.g. […]

Done

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c@26 
PS2, Line 26: 	get_option(&debug_level, "debug_level");
> Julius made a good point on another patch that this adds complexity (that […]

For our case since the logs before ramstage are not a lot, so I guess the need to be able to change their log levels is not really high. The only existing code that changes the early log levels is mb/scaleway, so only they will see the difference but I personally agree.

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c@26 
PS2, Line 26: 	get_option(&debug_level, "debug_level");
> For our case since the logs before ramstage are not a lot, so I guess the need to be able to change  […]

Oh you mean only skip the get_option() part, then get_console_loglevel() still can change early log levels.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c@26 
PS2, Line 26: 	get_option(&debug_level, "debug_level");
> Oh you mean only skip the get_option() part, then get_console_loglevel() still can change early log  […]

Well, you are right about get_console_loglevel(). If we unify it with
get_option(), boards using it would be affected too. Maybe we should
just keep it as is. AFAIK, it was explicitly meant to control things
in all stages.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c@26 
PS2, Line 26: 	get_option(&debug_level, "debug_level");
> Well, you are right about get_console_loglevel(). If we unify it with […]

If there are no other (strong) opinions, I'm going to update this to
call get_option() only late.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 5: Code-Review+1

Nice. Also something for the release notes.

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 5: Code-Review+2

[Nico Huber (Code Review)]

Hello Marc Jones, build bot (Jenkins), Martin Roth, Jonathan Zhang, Marc Jones, Johnny Lin, Paul Menzel, insomniac, Angel Pons, Julius Werner, Kyösti Mälkki, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45765

to look at the new patch set (#6).

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

console/init: Drop CONSOLE_LEVEL_CONST

We limited the configurability of the debug level to stages that have
a `.data` section. This is not really a requirement, because a `.bss`
section should suffice and we always have that now.

We want to make the debug level configurable early but also want to
avoid calling get_option() early, as an error therein could result
in no console output at all. Hence, we compromise and start using
get_option() from the second console init on.

TEST=Booted QEMU once with `debug_level=Debug` and once with
     `debug_level=Notice`. On the second boot, most messages
     vanished for all stages but the bootblock.

Change-Id: I11484fc32dcbba8d31772bd0b82785f17b2fba11
Signed-off-by: Nico Huber 
---
M src/console/init.c
1 file changed, 5 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/45765/6

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Marc Jones, Martin Roth, Jonathan Zhang, Marc Jones, Johnny Lin, Paul Menzel, insomniac, Angel Pons, Julius Werner, Kyösti Mälkki, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45765

to look at the new patch set (#7).

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

console/init: Drop CONSOLE_LEVEL_CONST

We limited the configurability of the debug level to stages that have
a `.data` section. This is not really a requirement, because a `.bss`
section should suffice and we always have that now.

We want to make the debug level configurable early but also want to
avoid calling get_option() early, as an error therein could result
in no console output at all. Hence, we compromise and start using
get_option() from the second console init on.

TEST=Booted QEMU once with `debug_level=Debug` and once with
     `debug_level=Notice`. On the second boot, most messages
     vanished for all stages but the bootblock.

Change-Id: I11484fc32dcbba8d31772bd0b82785f17b2fba11
Signed-off-by: Nico Huber 
---
M src/console/init.c
1 file changed, 5 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/45765/7

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c 
File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/45765/2/src/console/init.c@26 
PS2, Line 26: 	get_option(&debug_level, "debug_level");
> If there are no other (strong) opinions, I'm going to update this to […]

Done

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 7: Code-Review+1

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

Patch Set 7: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45765 )

Change subject: console/init: Drop CONSOLE_LEVEL_CONST
......................................................................

console/init: Drop CONSOLE_LEVEL_CONST

We limited the configurability of the debug level to stages that have
a `.data` section. This is not really a requirement, because a `.bss`
section should suffice and we always have that now.

We want to make the debug level configurable early but also want to
avoid calling get_option() early, as an error therein could result
in no console output at all. Hence, we compromise and start using
get_option() from the second console init on.

TEST=Booted QEMU once with `debug_level=Debug` and once with
     `debug_level=Notice`. On the second boot, most messages
     vanished for all stages but the bootblock.

Change-Id: I11484fc32dcbba8d31772bd0b82785f17b2fba11
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45765
Reviewed-by: Angel Pons 
Reviewed-by: Marc Jones 
Tested-by: build bot (Jenkins) 
---
M src/console/init.c
1 file changed, 5 insertions(+), 20 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Marc Jones: Looks good to me, approved
  Angel Pons: Looks good to me, but someone else must approve

diff --git a/src/console/init.c b/src/console/init.c
index 1dba9ad..c598077 100644
--- a/src/console/init.c
+++ b/src/console/init.c
@@ -8,40 +8,25 @@
 #include <option.h>
 #include <version.h>

-/* Mutable console log level only allowed when RAM comes online. */
-#define CONSOLE_LEVEL_CONST !ENV_STAGE_HAS_DATA_SECTION
+#define FIRST_CONSOLE (ENV_BOOTBLOCK || (CONFIG(NO_BOOTBLOCK_CONSOLE) && ENV_ROMSTAGE))

static int console_inited;
-static int console_loglevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL;
+static int console_loglevel;

static inline int get_log_level(void)
 {
    if (console_inited == 0)
    	return -1;
-	if (CONSOLE_LEVEL_CONST)
-		return get_console_loglevel();

return console_loglevel;
 }

-static inline void set_log_level(int new_level)
-{
-	if (CONSOLE_LEVEL_CONST)
-		return;
-
-	console_loglevel = new_level;
-}
-
 static void init_log_level(void)
 {
-	int debug_level = get_console_loglevel();
+	console_loglevel = get_console_loglevel();

-	if (CONSOLE_LEVEL_CONST)
-		return;
-
-	get_option(&debug_level, "debug_level");
-
-	set_log_level(debug_level);
+	if (!FIRST_CONSOLE)
+		get_option(&console_loglevel, "debug_level");
 }

int console_log_level(int msg_level)