Change in ...coreboot[master]: Console: Allow console UART to be enabled without serial console
Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33380
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Console: Allow console UART to be enabled without serial console
Currently, when we disable serial console, the serial console uart remains uninitialized. This patch allows coreboot to still set up the UART, even if we're not sending the console to it.
BUG=b:74392237 TEST=Verify UART still works, even with coreboot console disabled
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: Ic0942634ab8a9fcafdc1ea099721c127202e9f9a --- M src/console/Kconfig M src/drivers/uart/Kconfig M src/include/console/uart.h 3 files changed, 28 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/33380/1
diff --git a/src/console/Kconfig b/src/console/Kconfig index 61ba667..da769d3 100644 --- a/src/console/Kconfig +++ b/src/console/Kconfig @@ -33,6 +33,7 @@ bool "Serial port console output" default y depends on DRIVERS_UART + select ENABLE_UART help Send coreboot debug output to a serial port.
@@ -47,7 +48,7 @@ specific UART has to be used (e.g. when the platform code performs dangerous configurations).
-if CONSOLE_SERIAL +if ENABLE_UART
comment "I/O mapped, 8250-compatible" depends on DRIVERS_UART_8250IO @@ -79,13 +80,13 @@ Map the COM port number to the respective I/O port.
comment "Serial port base address = 0x3f8" -depends on UART_FOR_CONSOLE = 0 +depends on UART_FOR_CONSOLE = 0 && DRIVERS_UART_8250IO comment "Serial port base address = 0x2f8" -depends on UART_FOR_CONSOLE = 1 +depends on UART_FOR_CONSOLE = 1 && DRIVERS_UART_8250IO comment "Serial port base address = 0x3e8" -depends on UART_FOR_CONSOLE = 2 +depends on UART_FOR_CONSOLE = 2 && DRIVERS_UART_8250IO comment "Serial port base address = 0x2e8" -depends on UART_FOR_CONSOLE = 3 +depends on UART_FOR_CONSOLE = 3 && DRIVERS_UART_8250IO
config UART_OVERRIDE_BAUDRATE boolean @@ -156,7 +157,7 @@ default 3 depends on DRIVERS_UART_8250IO || DRIVERS_UART_8250MEM
-endif # CONSOLE_SERIAL +endif # ENABLE_UART
config SPKMODEM bool "spkmodem (console on speaker) console output" diff --git a/src/drivers/uart/Kconfig b/src/drivers/uart/Kconfig index 1f23a19..9b242fd 100644 --- a/src/drivers/uart/Kconfig +++ b/src/drivers/uart/Kconfig @@ -1,6 +1,21 @@ config DRIVERS_UART bool
+config ENABLE_UART + def_bool n + +config ENABLE_UART_WITHOUT_CONSOLE + bool "Always configure primary UART" + depends on DRIVERS_UART + select ENABLE_UART + help + The primary UART has previously only been set up when the serial console + is enabled. + Selecting this choice will configure the console UART even if the serial + console is disabled. + + Select the UART in the console menu + config DRIVERS_UART_8250IO # FIXME: Shouldn't have a prompt, should default to n, and # should be selected by boards that have it instead. diff --git a/src/include/console/uart.h b/src/include/console/uart.h index aed67c2..6bd88ae 100644 --- a/src/include/console/uart.h +++ b/src/include/console/uart.h @@ -67,11 +67,16 @@ (ENV_BOOTBLOCK || ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_VERSTAGE || \ ENV_POSTCAR || (ENV_SMM && CONFIG(DEBUG_SMI))))
-#if __CONSOLE_SERIAL_ENABLE__ +#if CONFIG(ENABLE_UART) static inline void __uart_init(void) { uart_init(CONFIG_UART_FOR_CONSOLE); } +#else +static inline void __uart_init(void) {} +#endif + +#if __CONSOLE_SERIAL_ENABLE__ static inline void __uart_tx_byte(u8 data) { uart_tx_byte(CONFIG_UART_FOR_CONSOLE, data); @@ -81,7 +86,6 @@ uart_tx_flush(CONFIG_UART_FOR_CONSOLE); } #else -static inline void __uart_init(void) {} static inline void __uart_tx_byte(u8 data) {} static inline void __uart_tx_flush(void) {} #endif
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
(3 comments)
Note that adding UART info to the coreboot table is still guarded by CONSOLE_SERIAL, not sure if that's intentional (conceptually, since this enables passing UART info to the kernel via ACPI, maybe it should also enable passing UART info to payloads?).
Also, there is a lot of SoC and mainboard code using CONFIG(CONSOLE_SERIAL) to check for things that should really be guarded by ENABLE_UART instead after this. Have you considered that it might be easier to just add an option that makes DEFAULT_CONSOLE_LOGLEVEL -1 instead? That would also allow users on systems that allow runtime-configured log levels to turn it up later if desired, which might be nice. (Alternatively, for a production Chromebook, I think setting it at BIOS_EMERG would be fine as well since that should essentially never output anything unless the machine hangs anyway.)
https://review.coreboot.org/#/c/33380/1/src/drivers/uart/Kconfig File src/drivers/uart/Kconfig:
https://review.coreboot.org/#/c/33380/1/src/drivers/uart/Kconfig@4 PS1, Line 4: config ENABLE_UART nit: why not just make this option directly menuconfig-visible? I think it would have the same effect.
https://review.coreboot.org/#/c/33380/1/src/drivers/uart/Kconfig@12 PS1, Line 12: The primary UART has previously only been set up when the serial console nit: I think explaining how older versions used to work is confusing and somewhat irrelevant here... I'd just explain what it does.
https://review.coreboot.org/#/c/33380/1/src/include/console/uart.h File src/include/console/uart.h:
https://review.coreboot.org/#/c/33380/1/src/include/console/uart.h@70 PS1, Line 70: #if CONFIG(ENABLE_UART) This is changing the effect for (ENV_SMM && !CONFIG(DEBUG_SMI)). Not sure if that's intentional or whether it matters in practice.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
Sorry for the basic question, but: What does this fix?
If this is needed to make something work in the OS, I guess something at the chip level is accidentally guarded by SERIAL_CONSOLE? At least the code in drivers/uart/ shouldn't make any difference regarding the general availability of the UART.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
Maybe we should have separate dirs for drivers used by coreboot to get something done and drivers that only do device initialization? Might make things less confusing.
AFAICT, drivers/uart/ belongs into the former class.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1: Code-Review-1
The SuperIO/SoC code has to take care to initialise the UART, even if serial console is disabled.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
(3 comments)
Patch Set 1:
Sorry for the basic question, but: What does this fix?
If this is needed to make something work in the OS, I guess something at the chip level is accidentally guarded by SERIAL_CONSOLE? At least the code in drivers/uart/ shouldn't make any difference regarding the general availability of the UART.
This is just to set up the UAER even if coreboot doesn't use it. This also goes along with the following patches to set up the SPCR ACPI table.
https://review.coreboot.org/#/c/33380/1/src/drivers/uart/Kconfig File src/drivers/uart/Kconfig:
https://review.coreboot.org/#/c/33380/1/src/drivers/uart/Kconfig@4 PS1, Line 4: config ENABLE_UART
nit: why not just make this option directly menuconfig-visible? I think it would have the same effec […]
This should be separate because this symbol can be turned on by selecting the serial console. At that point, it's not a change that gets saved in defconfig.
Right now, if you select ENABLE_UART_WITHOUT_CONSOLE, that gets saved in a defconfig, regardless of the state of the serial console option.
https://review.coreboot.org/#/c/33380/1/src/drivers/uart/Kconfig@12 PS1, Line 12: The primary UART has previously only been set up when the serial console
nit: I think explaining how older versions used to work is confusing and somewhat irrelevant here... […]
I'll update it.
https://review.coreboot.org/#/c/33380/1/src/include/console/uart.h File src/include/console/uart.h:
https://review.coreboot.org/#/c/33380/1/src/include/console/uart.h@70 PS1, Line 70: #if CONFIG(ENABLE_UART)
This is changing the effect for (ENV_SMM && !CONFIG(DEBUG_SMI)). […]
It shouldn't matter if it gets enabled in a stage that doesn't call it. I could specifically exclude it in verstage, postcar, and SMM, but I don't think that will actually change anything.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
Sorry for the basic question, but: What does this fix?
If this is needed to make something work in the OS, I guess something at the chip level is accidentally guarded by SERIAL_CONSOLE? At least the code in drivers/uart/ shouldn't make any difference regarding the general availability of the UART.
This is just to set up the UAER even if coreboot doesn't use it.
What is UAER? What is there to set up? Why should coreboot do it?
This also goes along with the following patches to set up the SPCR ACPI table.
This has `console` in its name, why is it useful without console?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33380/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33380/1//COMMIT_MSG@14 PS1, Line 14: TEST=Verify UART still works, even with coreboot console disabled Did you test that it doesn't work without this change and coreboot console disabled? If so, what was the exact issue?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
(1 comment)
What about the loglevel approach? You could achieve the same thing with much less code churn.
https://review.coreboot.org/#/c/33380/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33380/1//COMMIT_MSG@14 PS1, Line 14: TEST=Verify UART still works, even with coreboot console disabled
Did you test that it doesn't work without this change and coreboot […]
I assume it's an issue with the use of get_uart_baudrate() in CB:25344 and CONFIG_UART_FOR_CONSOLE in CB:25345? Without CONSOLE_SERIAL those are always 0. So I guess an alternative to this would be to decouple those options from CONSOLE_SERIAL and make them always visible, but I'm not sure if that's really what we want.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
What about the loglevel approach? You could achieve the same thing with much less code churn.
It's only the same thing if you don't want any logs anywhere. If you want logs in the cbmem console, but not on the serial console, then an approach like this is needed.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
What is UAER
Sorry UART.
What is there to set up?
The UART hardware.
Why should coreboot do it?
Because coreboot sets up the hardware.
This also goes along with the following patches to set up the SPCR ACPI table.
This has `console` in its name, why is it useful without console?
The coreboot console is not the only thing the serial port can be used for. It's not the only console. Enabling the coreboot console slows down the boot process considerably, so in my opinion, it's entirely reasonable to leave the coreboot serial console disabled, but still want to have the serial port set up.
I don't understand the argument. If you or anyone else doesn't want to use this feature, don't enable it. Sure, there's still some work to do on the patch, but your arguments make it seem like you're about to give it a -2, and I don't understand the objection.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
Patch Set 1:
What is UAER
Sorry UART.
What is there to set up?
The UART hardware.
Usually there is nothing to set up (beyond the chip confi- guration which is not done here in drivers/uart/). Can you elaborate, please.
So far, all my serial ports worked fine without CONSOLE_ SERIAL. I'm trying to figure out what makes your case so special. And it seems Julius already gave the right hint, you are reusing our console driver infrastructure to fill an ACPI table. That's ok, I guess. But if it's really just that, your commit message and help texts need an update, because currently it makes it look like you are talking about hardware.
Why should coreboot do it?
Because coreboot sets up the hardware.
This also goes along with the following patches to set up the SPCR ACPI table.
This has `console` in its name, why is it useful without console?
The coreboot console is not the only thing the serial port can be used for. It's not the only console. Enabling the coreboot console slows down the boot process considerably, so in my opinion, it's entirely reasonable to leave the coreboot serial console disabled, but still want to have the serial port set up.
What I'm trying to tell you all along, the drivers here are *not* for "serial port set up" they are for using the serial port usage.
I don't understand the argument. If you or anyone else doesn't want to use this feature, don't enable it. Sure, there's still some work to do on the patch, but your arguments make it seem like you're about to give it a -2, and I don't understand the objection.
As the reason for this change is still unclear to me, I have no idea what to do. My main concern is that when people are already confused during review of a new Kconfig option, there will be much more confusion once it is merged.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
What about the loglevel approach? You could achieve the same thing with much less code churn.
It's only the same thing if you don't want any logs anywhere. If you want logs in the cbmem console, but not on the serial console, then an approach like this is needed.
Thanks to Kyösti's work in CB:31370, CBMEM console now always prints at DEBUG and you can use the loglevel to control the consoles that actually cost boot time.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Patch Set 1:
Patch Set 1:
What about the loglevel approach? You could achieve the same thing with much less code churn.
It's only the same thing if you don't want any logs anywhere. If you want logs in the cbmem console, but not on the serial console, then an approach like this is needed.
Thanks to Kyösti's work in CB:31370, CBMEM console now always prints at DEBUG and you can use the loglevel to control the consoles that actually cost boot time.
Thanks, I hadn't seen that. I'll take a look.
Martin Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33380 )
Change subject: Console: Allow console UART to be enabled without serial console ......................................................................
Abandoned
-
Julius Werner (Code Review) -
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)