Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33093 )
Change subject: soc/intel/{skl,cnl,icl}: Drop soc_uart_set_legacy_mode() ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
What about some mainboard still use legacy UART, like port 0x3f8? Not every Intel mainboard select LPSS_UART_FOR_CONSOLE right? Like galileo?
Sorry for the confusion. This is not about common code. I've adapted the commit message, hoping that makes it clearer.
Currently that's okay for all the platform we have supported in coreboot, but what about some new board porting will require the legacy UART port to be wokring?
Sorry, I don't quite follow. What is "the legacy UART port" in this context? How would the removed code help in this case?
It rather seems to me that this code allows to use the LPSS UART in a special legacy mode (e.g. to allow legacy software to use it). But we don't run legacy software here.
Drop it here will loose the possibility.
It would still be in the Git history and could easily be restored. OTOH, if we allow dead code in the tree, it becomes a maintenance burden.
LPSS UART is PCH UART, which is 8250 MEM32 like UART port. The legacy UART like UART port of superIO or EC. Most of Intel RVP platform support both as two different physical header on mainboard, people can use jumper to select which is more convenient. FSP debug code will blindly printing on both UART port anyway.
I don't see a relation here to physical ports. I checked the datasheet again, it seems to me that "legacy" here means a 8-bit memory mapped interface instead of 32 bit.
Those code had been proved working during our early bring up stage on RVP board, but we switch back to LPSS UART port for firmware automation purpose then it became dead code. I don't believe SOC vendor can predict how the future platform be designed.
I do understand that the code was once useful. Though, it seems that since this commit:
e33a1724b3 (skylake: fix serial port with new code base)
coreboot is always set to the 32-bit mode. It's just a matter of keeping the hardware and coreboot in sync. But I don't see a reason to support both modes.
Myself is okay with the clean up purpose, highly possible it will be safe. Hope in the future PCH became "real" legacy free then we don't need that any more.
I've arranged the first three commits so that it's clearly visible that they don't change anything: The first removes the implementations, if they were called, Jenkins should complain. The third removes the call that wasn't effective anyway (as it built without the implementation).