Attention is currently required from: Nico Huber, Johnny Lin, Stefan Reinauer, Paul Menzel, Rocky Phagura, Christian Walter, Angel Pons, Arthur Heymans, Kyösti Mälkki, Peter Tsung Ho Wu, Deomid "rojer" Ryabkov, Ron Minnich. Rocky Phagura 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:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56386/comment/75f82282_4ce935a9 PS11, Line 14: on an OCP Delta Lake server: uart8250_init disables interrupts because it
I agree cleaning up after the uart output is useful for debugging the SMM handlers. […]
I think adding for the 8250 drives is sufficient.
File src/drivers/uart/sifive.c:
https://review.coreboot.org/c/coreboot/+/56386/comment/b8788414_8a898acf PS11, Line 58: /* TODO */
If this is for SMM, do we really need an implementation for the sifive UART?
Yes, if there is a system somewhere that uses sifive and runtime SMI's.
File src/drivers/uart/uart8250io.c:
https://review.coreboot.org/c/coreboot/+/56386/comment/146ba8e2_079f02d6 PS11, Line 58: static struct uart8250_state s_uart8250_state;
Can this be implemented without a static global variable?
Possibly. Do you have another recommendation for saving the state?
https://review.coreboot.org/c/coreboot/+/56386/comment/844875ae_158a82d9 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 uar […]
This can be done but then we would need to create another layer of API's to get to the correct console uart. How about if we surrounded this code with a CONFIG_DEBUG_SMI?