Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33452 )
Change subject: soc/fsp_broadwell_de: Configure serial port UPDs correctly ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33452/1/src/soc/intel/fsp_broadwell_de/fsp/c... File src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c:
https://review.coreboot.org/#/c/33452/1/src/soc/intel/fsp_broadwell_de/fsp/c... PS1, Line 69: SerialPortControllerInit0
The SerialPortConfigure is used to enabled/disable the onboard serial UART. […]
The thing is that `INTEGRATED_UART` seems to be a badly named debug option (imo, what it does is rather `INTEGRATED_UART_FOR_CONSOLE`). So the else here is also run when a board uses the integrated UART, but not for debugging. Then it boils down to the choice: 1. Should we always set the `SerialPortControllerInit*` UPDs to 0 and let the mainboard code override? or 2. Should we allow to use the presets in the binary?
OCP/Wedge seems to go the 2. route and just ensures the 0 at the mainboard level. But as long as we keep 2., I fear we'll have this discussion over and over again ;) (this is my second time, iirc)
(IMHO, no UPD should ever be left at its preset value in coreboot.)
https://review.coreboot.org/#/c/33452/1/src/soc/intel/fsp_broadwell_de/fsp/c... PS1, Line 74: SerialPortType The value should depend on the several DRIVERS_UART_8250* configs.
2 for MMIO
8 bit or 32 bit?