Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: set sane Kconfig defaults ......................................................................
mb/google/glados: set sane Kconfig defaults
Glados boards require FSP blobs to boot, and none have an exposed serial port outside of the servo interface. Set Kconfig defaults so that a default built image is bootable and doesn't hang due to trying to send data over a non-existant serial port.
Test: build/boot google/chell with board defaults
Change-Id: Ifad6f805e66438e2c436d9fa235d9be2ecf69179 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/mainboard/google/glados/Kconfig 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/39863/1
diff --git a/src/mainboard/google/glados/Kconfig b/src/mainboard/google/glados/Kconfig index bc0c67b..9e45aaa 100644 --- a/src/mainboard/google/glados/Kconfig +++ b/src/mainboard/google/glados/Kconfig @@ -1,5 +1,6 @@ config BOARD_GOOGLE_BASEBOARD_GLADOS def_bool n + select ADD_FSP_BINARIES select BOARD_ROMSIZE_KB_16384 select DRIVERS_I2C_GENERIC select DRIVERS_I2C_NAU8825 @@ -9,6 +10,7 @@ select EC_GOOGLE_CHROMEEC_LPC select EC_GOOGLE_CHROMEEC_MEC select EC_GOOGLE_CHROMEEC_PD + select FSP_USE_REPO select HAVE_ACPI_RESUME select HAVE_ACPI_TABLES select HAVE_OPTION_TABLE @@ -93,4 +95,8 @@ int default 2
+config CONSOLE_SERIAL + bool + default n + endif
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: set sane Kconfig defaults ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: set sane Kconfig defaults ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: set sane Kconfig defaults ......................................................................
Patch Set 1:
Can't we just fix the defaults and dependencies of these options?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: set sane Kconfig defaults ......................................................................
Patch Set 1:
Alternatively CB:39882.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: set sane Kconfig defaults ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... File src/mainboard/google/glados/Kconfig:
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... PS1, Line 98: config CONSOLE_SERIAL : bool : default n oops, missed this one. My earlier comments just refer to the FSP stuff.
How is the port non-existant? Why is there a UART_FOR_CONSOLE above if there is no port?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: set sane Kconfig defaults ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... File src/mainboard/google/glados/Kconfig:
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... PS1, Line 98: config CONSOLE_SERIAL : bool : default n
oops, missed this one. My earlier comments just refer to the FSP stuff. […]
glados boards have no user-accessible serial interface, save for pre-production models with a Google debug servo header. If serial console output is left enabled on production devices, then Tianocore experiences significant boot delays due to trying to send console output to a non-existent port
Hello build bot (Jenkins), Paul Menzel, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39863
to look at the new patch set (#2).
Change subject: mb/google/glados: disable serial console by default ......................................................................
mb/google/glados: disable serial console by default
Glados boards do not have an exposed serial port outside of the servo interface. Set board Kconfig so that a default built image with Tianocore payload is bootable and doesn't hang due to trying to send data over a non-existant serial port.
Test: build/boot google/chell with board defaults
Change-Id: Ifad6f805e66438e2c436d9fa235d9be2ecf69179 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/mainboard/google/glados/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/39863/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: disable serial console by default ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: disable serial console by default ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... File src/mainboard/google/glados/Kconfig:
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... PS1, Line 98: config CONSOLE_SERIAL : bool : default n
glados boards have no user-accessible serial interface, save for pre-production models with a Google […]
I still don't understand why a port doesn't exist (and how it makes a difference to software) only because there is no servo header. The port is part of the silicon...
Anyway, my confusion shouldn't affect this change.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: disable serial console by default ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... File src/mainboard/google/glados/Kconfig:
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... PS1, Line 98: config CONSOLE_SERIAL : bool : default n
I still don't understand why a port doesn't exist (and how it makes a […]
The UART is still there on the board, but as there's no servo header, it will never be connected to anything in the vast majority of the cases. And, since logging to UART is slow, it is wasteful if nothing is going to receive the logs. Unlike desktop boards, connecting something to that UART is much harder... (let's recall that most people are afraid to use a soldering iron on the SPI flash chip, which is comparatively large)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: disable serial console by default ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... File src/mainboard/google/glados/Kconfig:
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... PS1, Line 98: config CONSOLE_SERIAL : bool : default n
Tianocore experiences significant boot delays due to trying to send console output to a non-existent port
This is what is confusing me. I'm well aware of the fact that booting will be awkwardly slow if you tell Tianocore to output debug messages. But I don't see the mentioned causality. It's not slower because the port is not connected, is it?
If not, it seems like a made up argument to hide the actual intentions. Pretending that there is something special about this board while it's actually just that somebody wants to change a default for convenience and wants to avoid further discussion by not changing the default globally. For instance why not just let CONSOLE_SERIAL default to n for Tianocore?
Sorry if I'm off the track here, I don't want to imply any bad intentions.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: disable serial console by default ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... File src/mainboard/google/glados/Kconfig:
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... PS1, Line 98: config CONSOLE_SERIAL : bool : default n
Tianocore experiences significant boot delays due to trying to send console output to a non-existent port
This is what is confusing me. I'm well aware of the fact that booting will be awkwardly slow if you tell Tianocore to output debug messages. But I don't see the mentioned causality. It's not slower because the port is not connected, is it?
I have a google/chell boards both with and without a servo header. On the board with the header, there's minimal impact to boot time (ie, 1-2s) if I leave CONSOLE_SERIAL enabled, with or without the servo connected. On the one without, if I leave it enabled then the boot time is measured in minutes.
If not, it seems like a made up argument to hide the actual intentions. Pretending that there is something special about this board while it's actually just that somebody wants to change a default for convenience and wants to avoid further discussion by not changing the default globally. For instance why not just let CONSOLE_SERIAL default to n for Tianocore?
Sorry if I'm off the track here, I don't want to imply any bad intentions.
it's not a convenience issue, on one release I accidentally left it enabled and many users thought their device was bricked due to the ~5 min boot time. defaulting CONSOLE_SERIAL to n for Tianocore would also seem like a reasonable solution, but didn't want to make a large change like that when the specific issue of ~5 min boot times appeared limited to glados boards.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: disable serial console by default ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... File src/mainboard/google/glados/Kconfig:
https://review.coreboot.org/c/coreboot/+/39863/1/src/mainboard/google/glados... PS1, Line 98: config CONSOLE_SERIAL : bool : default n
Tianocore experiences significant boot delays due to trying to send console output to a non-exis […]
Thanks for the elaboration. Let's just assume the missing header is not the only difference of the production boards. Otherwise, software shouldn't be affected.
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: disable serial console by default ......................................................................
mb/google/glados: disable serial console by default
Glados boards do not have an exposed serial port outside of the servo interface. Set board Kconfig so that a default built image with Tianocore payload is bootable and doesn't hang due to trying to send data over a non-existant serial port.
Test: build/boot google/chell with board defaults
Change-Id: Ifad6f805e66438e2c436d9fa235d9be2ecf69179 Signed-off-by: Matt DeVillier matt.devillier@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39863 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Nico Huber nico.h@gmx.de --- M src/mainboard/google/glados/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/google/glados/Kconfig b/src/mainboard/google/glados/Kconfig index c90b2bd..d0ff377 100644 --- a/src/mainboard/google/glados/Kconfig +++ b/src/mainboard/google/glados/Kconfig @@ -81,4 +81,8 @@ int default 2
+config CONSOLE_SERIAL + bool + default n + endif
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: disable serial console by default ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 5/0/5 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1901 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1900 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1899 Non-emulation targets: HP_COMPAQ_8200_ELITE_SFF_PC using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1903 HP_COMPAQ_8200_ELITE_SFF_PC using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1902
Please note: This test is under development and might not be accurate at all!
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39863 )
Change subject: mb/google/glados: disable serial console by default ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 5/0/5 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1906 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1905 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1904 Non-emulation targets: HP_COMPAQ_8200_ELITE_SFF_PC using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1908 HP_COMPAQ_8200_ELITE_SFF_PC using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1907
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)