Attention is currently required from: Nico Huber, Johnny Lin, Paul Menzel, Rocky Phagura, Christian Walter, Angel Pons, Arthur Heymans, Kyösti Mälkki, Peter Tsung Ho Wu, Deomid "rojer" Ryabkov, Ron Minnich. Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56386 )
Change subject: Add console deinit API, use in SMM handler ......................................................................
Patch Set 11:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56386/comment/2961ac09_88398c54 PS11, Line 14: on an OCP Delta Lake server: uart8250_init disables interrupts because it
Patrick - just to provide some more context... […]
I agree cleaning up after the uart output is useful for debugging the SMM handlers. our uart init functions have not been written with the OS in mind.
The question I have is: Is it worth adding this to the framework as is here, or is it enough to handle this in the two 8250 drivers. Not sure how useful this will ever be with some of the more "serious" UART drivers like speakermodem and ne2000 console ;)
File src/drivers/uart/sifive.c:
https://review.coreboot.org/c/coreboot/+/56386/comment/bbe3015c_01219138 PS11, Line 58: /* TODO */ If this is for SMM, do we really need an implementation for the sifive UART?
File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/56386/comment/c1fa3365_0d2477ca PS11, Line 58: static struct uart8250_state s_uart8250_state; Can this be implemented without a static global variable?
https://review.coreboot.org/c/coreboot/+/56386/comment/1cb7032a_4ab23212 PS11, Line 62: /* Save previous state for restoring later. */ : s_uart8250_state.IER = inb(base_port + UART8250_IER); : s_uart8250_state.FCR = inb(base_port + UART8250_FCR); : s_uart8250_state.MCR = inb(base_port + UART8250_MCR); : s_uart8250_state.LCR = inb(base_port + UART8250_LCR); : s_uart8250_state.DLL = inb(base_port + UART8250_DLL); : s_uart8250_state.DLM = inb(base_port + UART8250_DLM); : If this is only needed by SMM, could we move this into a separate function that is called before uart8250_init in SMM? No need to redo this for bootblock, romstage and ramstage.
File src/drivers/uart/uart8250mem.c:
https://review.coreboot.org/c/coreboot/+/56386/comment/8bb2f251_f2c8677c PS11, Line 83: : static struct uart8250_state s_uart8250_state; : : static void uart8250_mem_init(void *base, unsigned int divisor) : { : /* Save previous state for restoring later. */ : s_uart8250_state.IER = uart8250_read(base, UART8250_IER); : s_uart8250_state.FCR = uart8250_read(base, UART8250_FCR); : s_uart8250_state.MCR = uart8250_read(base, UART8250_MCR); : s_uart8250_state.LCR = uart8250_read(base, UART8250_LCR); : s_uart8250_state.DLL = uart8250_read(base, UART8250_DLL); : s_uart8250_state.DLM = uart8250_read(base, UART8250_DLM); : see non-memory-mapped comments.