Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34307 )
Change subject: Revert "soc/intel/common: Set controller state to active in uart init" ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
The original CL should not have caused the UART to break. The LC was verified on CML and ICL. The All SOC including APL have same register offset and definitions. The UART init sequence is also same.
Please don't verify your CLs, please review them more carefully instead. Unless you want to test *all* paths that the affected code can take, especially untested paths need review. If somebody writes that a patch was verified, that is a reason to check every- thing twice! not to check less. I assume it wasn't "verified" that the PCI config write was actually effective in the bootblock with CONFIG_INTEL_LPSS_UART_FOR_CONSOLE.
+2 as the reverted patch introduced undefined behaviour. Not really the fault of that patch, as typing is not taken serious in soc/intel/.
NB. I don't think we should build further on this code, soc/intel/ deserves a rewrite. This time with actual reviews.
https://review.coreboot.org/c/coreboot/+/34307/4/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34307/4/src/soc/intel/common/block/... PS4, Line 74: This is proof that the type of `device` is wrong.