Change in ...coreboot[master]: soc/fsp_broadwell_de: Configure serial port UPDs correctly
Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33452
Change subject: soc/fsp_broadwell_de: Configure serial port UPDs correctly ......................................................................
soc/fsp_broadwell_de: Configure serial port UPDs correctly
The SerialPortConfigure UPD (among others) was being left unconfigured when the SoC's integrated UART was not used, leading to serial output via an SIO-attached UART being disabled upon FSP init.
Ensure that all serial-related UPDs are set correctly regardless of Kconfig options selected.
Test: Boot BDE-based board and verify SIO serial output continues past FSP init
Signed-off-by: Matt DeVillier matt.devillier@puri.sm Change-Id: I634120648afb094be762093b5f9549c241c5668a --- M src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c 1 file changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33452/1
diff --git a/src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c b/src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c index edb313e..63f9a74 100644 --- a/src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c +++ b/src/soc/intel/fsp_broadwell_de/fsp/chipset_fsp_util.c @@ -64,10 +64,17 @@ UpdData->SerialPortBaudRate = 11; else if (CONFIG(CONSOLE_SERIAL_115200)) UpdData->SerialPortBaudRate = 12; + } else { + UpdData->SerialPortConfigure = 0; + UpdData->SerialPortControllerInit0 = 0; + UpdData->SerialPortControllerInit1 = 0; }
- if (!CONFIG(CONSOLE_SERIAL)) + if (CONFIG(CONSOLE_SERIAL)) { + UpdData->SerialPortType = 1; + } else { UpdData->SerialPortType = 0; + }
UpdData->DebugOutputLevel = CONFIG_FSP_DEBUG_LEVEL;
build bot (Jenkins) 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:
(1 comment)
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 73: if (CONFIG(CONSOLE_SERIAL)) { braces {} are not necessary for any arm of this statement
Frans Hendriks 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: Code-Review+1
(1 comment)
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
You might want to init the controller even if CONSOLE_SERIAL is disabled.
The SerialPortConfigure is used to enabled/disable the onboard serial UART. This setting is not depending on CONSOLE_SERIAL. What do you mean?
The SerialPortType value is used for CONSOLE_SERIAL. Value of 0 will disable FSP console output.
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?
Patrick Rudolph 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 thing is that `INTEGRATED_UART` seems to be a badly named debug […]
The reason route 2 was used on wedge was to fix serial output without breaking other platforms (and not getting a headache trying to understand what those UPDs are meant to do, and what they actually do).
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. […]
8bit.
Matt DeVillier has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33452 )
Change subject: soc/fsp_broadwell_de: Configure serial port UPDs correctly ......................................................................
Abandoned
will configure UPDs in mainboard romstage.c instead
-
build bot (Jenkins) (Code Review) -
Frans Hendriks (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)