Change in coreboot[master]: mb/*/*: Don't guard serial setup

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

mb/*/*: Don't guard serial setup

Even without using the console it might be useful to set up the
console regardless, for instance with VBOOT or normal/fallback setups.
This will be more meaningful once C_ENVIRONMENT_BOOTBLOCK is
implemented.

Change-Id: Ieca2ec1e376cdb26d76222126307fe016572d9d7
Signed-off-by: Arthur Heymans 
---
M src/mainboard/hp/compaq_8200_elite_sff/romstage.c
M src/mainboard/hp/z220_sff_workstation/romstage.c
M src/mainboard/ocp/wedge100s/romstage.c
3 files changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/36603/1

diff --git a/src/mainboard/hp/compaq_8200_elite_sff/romstage.c b/src/mainboard/hp/compaq_8200_elite_sff/romstage.c
index 258eac7..b0dd962 100644
--- a/src/mainboard/hp/compaq_8200_elite_sff/romstage.c
+++ b/src/mainboard/hp/compaq_8200_elite_sff/romstage.c
@@ -58,8 +58,7 @@

void mainboard_config_superio(void)
 {
-	if (CONFIG(CONSOLE_SERIAL))
-		nuvoton_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
+	nuvoton_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
 }

void mainboard_get_spd(spd_raw_data *spd, bool id_only)
diff --git a/src/mainboard/hp/z220_sff_workstation/romstage.c b/src/mainboard/hp/z220_sff_workstation/romstage.c
index 6c139ed..e07007c 100644
--- a/src/mainboard/hp/z220_sff_workstation/romstage.c
+++ b/src/mainboard/hp/z220_sff_workstation/romstage.c
@@ -58,8 +58,7 @@

void mainboard_config_superio(void)
 {
-	if (CONFIG(CONSOLE_SERIAL))
-		nuvoton_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
+	nuvoton_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
 }

void mainboard_get_spd(spd_raw_data *spd, bool id_only)
diff --git a/src/mainboard/ocp/wedge100s/romstage.c b/src/mainboard/ocp/wedge100s/romstage.c
index 108d7a1..c0d2ca0 100644
--- a/src/mainboard/ocp/wedge100s/romstage.c
+++ b/src/mainboard/ocp/wedge100s/romstage.c
@@ -42,8 +42,7 @@
    pci_write_config32(PCI_DEV(0x0, LPC_DEV, LPC_FUNC), LPC_GEN2_DEC,
    		   (0 << 16) | ALIGN_DOWN(0xca2, 4) | 1);

-	if (CONFIG(CONSOLE_SERIAL))
-		ite_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
+	ite_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);

/*

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 1:

I don't see the use case. If serial console is disable it would get initialized in ramstage. Why do it early in the boot flow if unnecessary?

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

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

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 1: Code-Review+2
> Patch Set 1:
>
> I don't see the use case. If serial console is disable it would get initialized in ramstage. Why do it early in the boot flow if unnecessary?

One could enable GDB_STUB independently of CONSOLE_SERIAL.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 1: Code-Review-1

GDB_STUB depends on CONSOLE_SERIAL

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

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

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 1: Code-Review+1

Well GDB_STUB should not depend on CONSOLE_SERIAL but DRIVERS_UART.

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 1:
> Patch Set 1:
>
> I don't see the use case. If serial console is disable it would get initialized in ramstage. Why do it early in the boot flow if unnecessary?

How so? ramstage only initializes resources later on.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/36603/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36603/2//COMMIT_MSG@9 
PS2, Line 9: Even without using the console it might be useful to set up the
rephrase to: is required for ... VBOOT or normal/fallback as it might have been build with CONSOLE_SERIAL enabled

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 2:

What about these?
src/mainboard/google/veyron*
src/mainboard/google/gru
src/mainboard/sifive/hifive-unleashed

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 2: Code-Review+1

[Nico Huber (Code Review)]

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

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 2: Code-Review+2

Doesn't look like Wedge will make it to C-environment bootblock, though.

[Paul Menzel (Code Review)]

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

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 2:

Does this have any effect on the boot time?

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Does this have any effect on the boot time?

Since it's just register writes AFAIK, maybe a few ms

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> >
> > Does this have any effect on the boot time?
>
> Since it's just register writes AFAIK, maybe a few ms

More like a few us I think.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36603/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36603/2//COMMIT_MSG@6 
PS2, Line 6: 
What about these?
src/mainboard/google/veyron*
src/mainboard/google/gru
src/mainboard/sifive/hifive-unleashed

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

Kyösti Mälkki has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Removed Code-Review+1 by Kyösti Mälkki

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has uploaded a new patch set (#3) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

mb/*/*: Don't guard serial setup

Even without using the console it might be useful to set up the
console regardless, for instance with VBOOT or normal/fallback setups.
This will be more meaningful once C_ENVIRONMENT_BOOTBLOCK is
implemented.

Change-Id: Ieca2ec1e376cdb26d76222126307fe016572d9d7
Signed-off-by: Arthur Heymans 
---
M src/mainboard/dell/optiplex_9010/early_init.c
M src/mainboard/hp/compaq_8200_elite_sff/early_init.c
M src/mainboard/hp/z220_sff_workstation/early_init.c
M src/mainboard/sifive/hifive-unleashed/romstage.c
4 files changed, 4 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/36603/3

[Arthur Heymans (Code Review)]

Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36603 )

Change subject: mb/*/*: Don't guard serial setup
......................................................................

Abandoned

Not needed anymore?