Change in ...coreboot[master]: soc/intel: Drop soc_uart_set_legacy_mode() implementations
Hello Patrick Rudolph, Lance Zhao,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/33093
to review the following change.
Change subject: soc/intel: Drop soc_uart_set_legacy_mode() implementations ......................................................................
soc/intel: Drop soc_uart_set_legacy_mode() implementations
They are never called: The only calling path is guarded by both !DRIVERS_UART_8250MEM_32 and INTEL_LPSS_UART_FOR_CONSOLE but the latter selects the former.
If somebody figures out how this is supposed to be used, we can easily revive the implementation.
Change-Id: I96e304bdee4eadb52725027d0d662ef75f3d4307 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/cannonlake/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/uart.c 3 files changed, 0 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/33093/1
diff --git a/src/soc/intel/cannonlake/uart.c b/src/soc/intel/cannonlake/uart.c index 2bd906a..7174a9a 100644 --- a/src/soc/intel/cannonlake/uart.c +++ b/src/soc/intel/cannonlake/uart.c @@ -24,10 +24,6 @@ #include <soc/pci_devs.h> #include <soc/pcr_ids.h>
-/* Serial IO UART controller legacy mode */ -#define PCR_SERIAL_IO_GPPRVRW7 0x618 -#define PCR_SIO_PCH_LEGACY_UART(idx) (1 << (idx)) - const struct uart_gpio_pad_config uart_gpio_pads[] = { { .console_index = 0, @@ -54,17 +50,6 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-void soc_uart_set_legacy_mode(void) -{ - pcr_write32(PID_SERIALIO, PCR_SERIAL_IO_GPPRVRW7, - PCR_SIO_PCH_LEGACY_UART(CONFIG_UART_FOR_CONSOLE)); - /* - * Dummy read after setting any of GPPRVRW7. - * Required for UART 16550 8-bit Legacy mode to become active - */ - lpss_clk_read(UART_BASE(CONFIG_UART_FOR_CONSOLE)); -} - struct device *soc_uart_console_to_device(int uart_console) { /* diff --git a/src/soc/intel/icelake/uart.c b/src/soc/intel/icelake/uart.c index 2bd906a..7174a9a 100644 --- a/src/soc/intel/icelake/uart.c +++ b/src/soc/intel/icelake/uart.c @@ -24,10 +24,6 @@ #include <soc/pci_devs.h> #include <soc/pcr_ids.h>
-/* Serial IO UART controller legacy mode */ -#define PCR_SERIAL_IO_GPPRVRW7 0x618 -#define PCR_SIO_PCH_LEGACY_UART(idx) (1 << (idx)) - const struct uart_gpio_pad_config uart_gpio_pads[] = { { .console_index = 0, @@ -54,17 +50,6 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-void soc_uart_set_legacy_mode(void) -{ - pcr_write32(PID_SERIALIO, PCR_SERIAL_IO_GPPRVRW7, - PCR_SIO_PCH_LEGACY_UART(CONFIG_UART_FOR_CONSOLE)); - /* - * Dummy read after setting any of GPPRVRW7. - * Required for UART 16550 8-bit Legacy mode to become active - */ - lpss_clk_read(UART_BASE(CONFIG_UART_FOR_CONSOLE)); -} - struct device *soc_uart_console_to_device(int uart_console) { /* diff --git a/src/soc/intel/skylake/uart.c b/src/soc/intel/skylake/uart.c index 1b2a742..8b7c99e 100644 --- a/src/soc/intel/skylake/uart.c +++ b/src/soc/intel/skylake/uart.c @@ -24,10 +24,6 @@ #include <soc/pci_devs.h> #include <soc/pcr_ids.h>
-/* Serial IO UART controller legacy mode */ -#define PCR_SERIAL_IO_GPPRVRW7 0x618 -#define PCR_SIO_PCH_LEGACY_UART(idx) (1 << (idx)) - /* UART pad configuration. Support RXD and TXD for now. */ const struct uart_gpio_pad_config uart_gpio_pads[] = { { @@ -55,17 +51,6 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-void soc_uart_set_legacy_mode(void) -{ - pcr_write32(PID_SERIALIO, PCR_SERIAL_IO_GPPRVRW7, - PCR_SIO_PCH_LEGACY_UART(CONFIG_UART_FOR_CONSOLE)); - /* - * Dummy read after setting any of GPPRVRW7. - * Required for UART 16550 8-bit Legacy mode to become active - */ - lpss_clk_read(UART_BASE(CONFIG_UART_FOR_CONSOLE)); -} - struct device *soc_uart_console_to_device(int uart_console) { /*
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33093 )
Change subject: soc/intel: Drop soc_uart_set_legacy_mode() implementations ......................................................................
Patch Set 1:
What about some mainboard still use legacy UART, like port 0x3f8? Not every Intel mainboard select LPSS_UART_FOR_CONSOLE right? Like galileo?
Hello Patrick Rudolph, Lance Zhao, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33093
to look at the new patch set (#2).
Change subject: soc/intel/{skl,cnl,icl}: Drop soc_uart_set_legacy_mode() ......................................................................
soc/intel/{skl,cnl,icl}: Drop soc_uart_set_legacy_mode()
This is never called: The only calling path is guarded by both !DRIVERS_UART_8250MEM_32 and INTEL_LPSS_UART_FOR_CONSOLE but the latter selects the former.
If somebody figures out how this is supposed to be used, we can easily revive the implementation.
Change-Id: I96e304bdee4eadb52725027d0d662ef75f3d4307 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/cannonlake/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/uart.c 3 files changed, 0 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/33093/2
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:
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.
Lance Zhao 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:
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? Drop it here will loose the possibility.
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:
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.
Lance Zhao 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:
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.
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.
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.
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).
Lance Zhao 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:
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).
I check the spec again and I believe you are right, that's 32 bit mode and 8 bit mode.
Lance Zhao 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: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33093 )
Change subject: soc/intel/{skl,cnl,icl}: Drop soc_uart_set_legacy_mode() ......................................................................
soc/intel/{skl,cnl,icl}: Drop soc_uart_set_legacy_mode()
This is never called: The only calling path is guarded by both !DRIVERS_UART_8250MEM_32 and INTEL_LPSS_UART_FOR_CONSOLE but the latter selects the former.
If somebody figures out how this is supposed to be used, we can easily revive the implementation.
Change-Id: I96e304bdee4eadb52725027d0d662ef75f3d4307 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/33093 Reviewed-by: Lance Zhao lance.zhao@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/cannonlake/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/uart.c 3 files changed, 0 insertions(+), 45 deletions(-)
Approvals: build bot (Jenkins): Verified Lance Zhao: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/uart.c b/src/soc/intel/cannonlake/uart.c index 2bd906a..7174a9a 100644 --- a/src/soc/intel/cannonlake/uart.c +++ b/src/soc/intel/cannonlake/uart.c @@ -24,10 +24,6 @@ #include <soc/pci_devs.h> #include <soc/pcr_ids.h>
-/* Serial IO UART controller legacy mode */ -#define PCR_SERIAL_IO_GPPRVRW7 0x618 -#define PCR_SIO_PCH_LEGACY_UART(idx) (1 << (idx)) - const struct uart_gpio_pad_config uart_gpio_pads[] = { { .console_index = 0, @@ -54,17 +50,6 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-void soc_uart_set_legacy_mode(void) -{ - pcr_write32(PID_SERIALIO, PCR_SERIAL_IO_GPPRVRW7, - PCR_SIO_PCH_LEGACY_UART(CONFIG_UART_FOR_CONSOLE)); - /* - * Dummy read after setting any of GPPRVRW7. - * Required for UART 16550 8-bit Legacy mode to become active - */ - lpss_clk_read(UART_BASE(CONFIG_UART_FOR_CONSOLE)); -} - struct device *soc_uart_console_to_device(int uart_console) { /* diff --git a/src/soc/intel/icelake/uart.c b/src/soc/intel/icelake/uart.c index 2bd906a..7174a9a 100644 --- a/src/soc/intel/icelake/uart.c +++ b/src/soc/intel/icelake/uart.c @@ -24,10 +24,6 @@ #include <soc/pci_devs.h> #include <soc/pcr_ids.h>
-/* Serial IO UART controller legacy mode */ -#define PCR_SERIAL_IO_GPPRVRW7 0x618 -#define PCR_SIO_PCH_LEGACY_UART(idx) (1 << (idx)) - const struct uart_gpio_pad_config uart_gpio_pads[] = { { .console_index = 0, @@ -54,17 +50,6 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-void soc_uart_set_legacy_mode(void) -{ - pcr_write32(PID_SERIALIO, PCR_SERIAL_IO_GPPRVRW7, - PCR_SIO_PCH_LEGACY_UART(CONFIG_UART_FOR_CONSOLE)); - /* - * Dummy read after setting any of GPPRVRW7. - * Required for UART 16550 8-bit Legacy mode to become active - */ - lpss_clk_read(UART_BASE(CONFIG_UART_FOR_CONSOLE)); -} - struct device *soc_uart_console_to_device(int uart_console) { /* diff --git a/src/soc/intel/skylake/uart.c b/src/soc/intel/skylake/uart.c index 1b2a742..8b7c99e 100644 --- a/src/soc/intel/skylake/uart.c +++ b/src/soc/intel/skylake/uart.c @@ -24,10 +24,6 @@ #include <soc/pci_devs.h> #include <soc/pcr_ids.h>
-/* Serial IO UART controller legacy mode */ -#define PCR_SERIAL_IO_GPPRVRW7 0x618 -#define PCR_SIO_PCH_LEGACY_UART(idx) (1 << (idx)) - /* UART pad configuration. Support RXD and TXD for now. */ const struct uart_gpio_pad_config uart_gpio_pads[] = { { @@ -55,17 +51,6 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-void soc_uart_set_legacy_mode(void) -{ - pcr_write32(PID_SERIALIO, PCR_SERIAL_IO_GPPRVRW7, - PCR_SIO_PCH_LEGACY_UART(CONFIG_UART_FOR_CONSOLE)); - /* - * Dummy read after setting any of GPPRVRW7. - * Required for UART 16550 8-bit Legacy mode to become active - */ - lpss_clk_read(UART_BASE(CONFIG_UART_FOR_CONSOLE)); -} - struct device *soc_uart_console_to_device(int uart_console) { /*
-
Lance Zhao (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)