Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Piotr Król, Subrata Banik, Matt DeVillier, Stefan Reinauer, Duncan Laurie, build bot (Jenkins), Patrick Georgi, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/30825
to review the following change.
Change subject: Revert "soc/intel: Clean mess around UART_DEBUG" ......................................................................
Revert "soc/intel: Clean mess around UART_DEBUG"
This reverts commit a96e66a76f21c41b0c15db8d9df1d721f4a8a9af.
Reason for revert: <The change will break multiple platform's ability to get serial log, also it didn't address Aaron's original request>
Change-Id: I069e2b3a8690c96fa3ed3e0e791da82a8f332946 --- M src/console/Kconfig M src/mainboard/intel/leafhill/Kconfig M src/mainboard/intel/minnow3/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/apollolake/Makefile.inc M src/soc/intel/apollolake/bootblock/bootblock.c M src/soc/intel/apollolake/cse.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/Kconfig M src/soc/intel/cannonlake/Makefile.inc M src/soc/intel/cannonlake/bootblock/bootblock.c M src/soc/intel/common/block/uart/Kconfig M src/soc/intel/common/block/uart/uart.c M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/Makefile.inc M src/soc/intel/icelake/bootblock/bootblock.c M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/Makefile.inc M src/soc/intel/skylake/bootblock/bootblock.c M src/soc/intel/skylake/me.c 20 files changed, 100 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/30825/1
diff --git a/src/console/Kconfig b/src/console/Kconfig index 3db7005..e7e3298 100644 --- a/src/console/Kconfig +++ b/src/console/Kconfig @@ -48,16 +48,8 @@ comment "device-specific UART" depends on HAVE_UART_SPECIAL
-config FIXED_UART_FOR_CONSOLE - bool - help - Select to remove the prompt from UART_FOR_CONSOLE in case a - specific UART has to be used (e.g. when the platform code - performs dangerous configurations). - config UART_FOR_CONSOLE - int - prompt "Index for UART port to use for console" if !FIXED_UART_FOR_CONSOLE + int "Index for UART port to use for console" default 0 help Select an I/O port to use for serial console: diff --git a/src/mainboard/intel/leafhill/Kconfig b/src/mainboard/intel/leafhill/Kconfig index 69bfde7..94d0493 100644 --- a/src/mainboard/intel/leafhill/Kconfig +++ b/src/mainboard/intel/leafhill/Kconfig @@ -5,7 +5,7 @@ select SOC_INTEL_APOLLOLAKE select BOARD_ROMSIZE_KB_16384 select HAVE_ACPI_TABLES - select INTEL_LPSS_UART_FOR_CONSOLE + select UART_DEBUG
config MAINBOARD_DIR string @@ -24,6 +24,7 @@ default "src/mainboard/$(CONFIG_MAINBOARD_DIR)/leafhill.$(CONFIG_COREBOOT_ROMSIZE_KB).fmd"
config UART_FOR_CONSOLE + int "Number of UART port to use for serial log" default 2
config NEED_IFWI diff --git a/src/mainboard/intel/minnow3/Kconfig b/src/mainboard/intel/minnow3/Kconfig index a787a2d..531036a 100644 --- a/src/mainboard/intel/minnow3/Kconfig +++ b/src/mainboard/intel/minnow3/Kconfig @@ -5,7 +5,7 @@ select SOC_INTEL_APOLLOLAKE select BOARD_ROMSIZE_KB_16384 select HAVE_ACPI_TABLES - select INTEL_LPSS_UART_FOR_CONSOLE + select UART_DEBUG
config MAINBOARD_DIR string @@ -20,6 +20,7 @@ default "src/mainboard/$(CONFIG_MAINBOARD_DIR)/minnow3.fmd"
config UART_FOR_CONSOLE + int "Number of UART port to use for serial log" default 2
config NEED_IFWI diff --git a/src/soc/intel/apollolake/Kconfig b/src/soc/intel/apollolake/Kconfig index 7a5a11c..4a841be 100644 --- a/src/soc/intel/apollolake/Kconfig +++ b/src/soc/intel/apollolake/Kconfig @@ -169,6 +169,14 @@ int default 133
+config UART_DEBUG + bool "Enable SoC UART debug port selected by UART_FOR_CONSOLE." + default n + select CONSOLE_SERIAL + select DRIVERS_UART + select DRIVERS_UART_8250MEM_32 + select NO_UART_ON_SUPERIO + # 32KiB bootblock is all that is mapped in by the CSE at top of 4GiB. config C_ENV_BOOTBLOCK_SIZE hex diff --git a/src/soc/intel/apollolake/Makefile.inc b/src/soc/intel/apollolake/Makefile.inc index 822158c..19ebe7c 100644 --- a/src/soc/intel/apollolake/Makefile.inc +++ b/src/soc/intel/apollolake/Makefile.inc @@ -18,14 +18,14 @@ bootblock-y += mmap_boot.c bootblock-y += pmutil.c bootblock-y += spi.c -bootblock-y += uart.c +bootblock-$(CONFIG_UART_DEBUG) += uart.c
romstage-y += car.c romstage-$(CONFIG_PLATFORM_USES_FSP2_0) += romstage.c romstage-y += gspi.c romstage-y += heci.c romstage-y += i2c.c -romstage-y += uart.c +romstage-$(CONFIG_UART_DEBUG) += uart.c romstage-y += memmap.c romstage-y += meminit.c ifeq ($(CONFIG_SOC_INTEL_GLK),y) @@ -42,7 +42,7 @@ smm-y += pmutil.c smm-y += smihandler.c smm-y += spi.c -smm-y += uart.c +smm-$(CONFIG_UART_DEBUG) += uart.c smm-y += elog.c
ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi.c @@ -57,7 +57,7 @@ ramstage-y += lpc.c ramstage-y += memmap.c ramstage-y += mmap_boot.c -ramstage-y += uart.c +ramstage-$(CONFIG_UART_DEBUG) += uart.c ramstage-y += nhlt.c ramstage-y += spi.c ramstage-y += systemagent.c @@ -74,7 +74,7 @@ postcar-y += i2c.c postcar-$(CONFIG_RESET_ON_INVALID_RAMSTAGE_CACHE) += heci.c postcar-$(CONFIG_RESET_ON_INVALID_RAMSTAGE_CACHE) += reset.c -postcar-y += uart.c +postcar-$(CONFIG_UART_DEBUG) += uart.c postcar-$(CONFIG_VBOOT_MEASURED_BOOT) += gspi.c
verstage-y += car.c @@ -83,7 +83,7 @@ verstage-y += heci.c verstage-y += memmap.c verstage-y += mmap_boot.c -verstage-y += uart.c +verstage-$(CONFIG_UART_DEBUG) += uart.c verstage-y += pmutil.c verstage-y += reset.c verstage-y += spi.c diff --git a/src/soc/intel/apollolake/bootblock/bootblock.c b/src/soc/intel/apollolake/bootblock/bootblock.c index ce89f13..ac3e0cc 100644 --- a/src/soc/intel/apollolake/bootblock/bootblock.c +++ b/src/soc/intel/apollolake/bootblock/bootblock.c @@ -95,7 +95,7 @@ pmc_global_reset_enable(0);
/* Prepare UART for serial console. */ - if (IS_ENABLED(CONFIG_INTEL_LPSS_UART_FOR_CONSOLE)) + if (IS_ENABLED(CONFIG_UART_DEBUG)) uart_bootblock_init();
if (IS_ENABLED(CONFIG_TPM_ON_FAST_SPI)) diff --git a/src/soc/intel/apollolake/cse.c b/src/soc/intel/apollolake/cse.c index 0ff7dcc..d761a6c 100644 --- a/src/soc/intel/apollolake/cse.c +++ b/src/soc/intel/apollolake/cse.c @@ -222,7 +222,7 @@ * Print ME version only if UART debugging is enabled. Else, it takes * ~0.6 second to talk to ME and get this information. */ - if (!IS_ENABLED(CONFIG_CONSOLE_SERIAL)) + if (!IS_ENABLED(CONFIG_UART_DEBUG)) return;
msg.mkhi_hdr.fields.group_id = MKHI_GROUP_ID_GEN; diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c index 9cc5166..4f4f9f5 100644 --- a/src/soc/intel/apollolake/romstage.c +++ b/src/soc/intel/apollolake/romstage.c @@ -278,7 +278,7 @@ static void fill_console_params(FSPM_UPD *mupd) { if (IS_ENABLED(CONFIG_CONSOLE_SERIAL)) { - if (IS_ENABLED(CONFIG_INTEL_LPSS_UART_FOR_CONSOLE)) { + if (IS_ENABLED(CONFIG_UART_DEBUG)) { mupd->FspmConfig.SerialDebugPortDevice = CONFIG_UART_FOR_CONSOLE; /* use MMIO port type */ diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index f8193cd..9e007b6 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -77,6 +77,23 @@ select DISPLAY_FSP_VERSION_INFO select FSP_T_XIP if FSP_CAR
+config UART_DEBUG + bool "Enable UART debug port." + default n + select CONSOLE_SERIAL + select BOOTBLOCK_CONSOLE + select DRIVERS_UART + select DRIVERS_UART_8250MEM_32 + select NO_UART_ON_SUPERIO + +config UART_FOR_CONSOLE + int "Index for LPSS UART port to use for console" + default 2 if DRIVERS_UART_8250MEM_32 + default 0 + help + Index for LPSS UART port to use for console: + 0 = LPSS UART0, 1 = LPSS UART1, 2 = LPSS UART2 + config DCACHE_RAM_BASE default 0xfef00000
diff --git a/src/soc/intel/cannonlake/Makefile.inc b/src/soc/intel/cannonlake/Makefile.inc index fb2c849..faa22f0 100644 --- a/src/soc/intel/cannonlake/Makefile.inc +++ b/src/soc/intel/cannonlake/Makefile.inc @@ -19,7 +19,7 @@ bootblock-y += spi.c bootblock-y += lpc.c bootblock-y += p2sb.c -bootblock-y += uart.c +bootblock-$(CONFIG_UART_DEBUG) += uart.c
romstage-$(CONFIG_SOC_INTEL_CANNONLAKE_MEMCFG_INIT) += cnl_memcfg_init.c romstage-y += gspi.c @@ -29,7 +29,7 @@ romstage-y += pmutil.c romstage-y += reset.c romstage-y += spi.c -romstage-y += uart.c +romstage-$(CONFIG_UART_DEBUG) += uart.c
ramstage-y += acpi.c ramstage-y += chip.c @@ -51,27 +51,27 @@ ramstage-y += smmrelocate.c ramstage-y += spi.c ramstage-y += systemagent.c -ramstage-y += uart.c +ramstage-$(CONFIG_UART_DEBUG) += uart.c ramstage-y += vr_config.c ramstage-y += sd.c
smm-y += p2sb.c smm-y += pmutil.c smm-y += smihandler.c -smm-y += uart.c +smm-$(CONFIG_UART_DEBUG) += uart.c
postcar-y += memmap.c postcar-y += pmutil.c postcar-y += i2c.c postcar-y += gspi.c postcar-y += spi.c -postcar-y += uart.c +postcar-$(CONFIG_UART_DEBUG) += uart.c
verstage-y += gspi.c verstage-y += i2c.c verstage-y += pmutil.c verstage-y += spi.c -verstage-y += uart.c +verstage-$(CONFIG_UART_DEBUG) += uart.c
ifeq ($(CONFIG_SOC_INTEL_CANNONLAKE_PCH_H),y) bootblock-y += gpio_cnp_h.c diff --git a/src/soc/intel/cannonlake/bootblock/bootblock.c b/src/soc/intel/cannonlake/bootblock/bootblock.c index 08a13ea..b7d00cd 100644 --- a/src/soc/intel/cannonlake/bootblock/bootblock.c +++ b/src/soc/intel/cannonlake/bootblock/bootblock.c @@ -53,7 +53,7 @@ bootblock_pch_early_init(); bootblock_cpu_init(); pch_early_iorange_init(); - if (IS_ENABLED(CONFIG_INTEL_LPSS_UART_FOR_CONSOLE)) + if (IS_ENABLED(CONFIG_UART_DEBUG)) uart_bootblock_init(); }
diff --git a/src/soc/intel/common/block/uart/Kconfig b/src/soc/intel/common/block/uart/Kconfig index e731465..f4a0e4e 100644 --- a/src/soc/intel/common/block/uart/Kconfig +++ b/src/soc/intel/common/block/uart/Kconfig @@ -15,14 +15,3 @@ hex help Clock m-divisor value for m/n divider - -config INTEL_LPSS_UART_FOR_CONSOLE - bool - depends on SOC_INTEL_COMMON_BLOCK_UART - select DRIVERS_UART_8250MEM_32 - select FIXED_UART_FOR_CONSOLE - help - Selected by mainboards that use one of the SoC's LPSS UARTS - for the coreboot console. - WARNING: UART_FOR_CONSOLE has to be set to a correct value, - otherwise wrong pad configurations might be selected. diff --git a/src/soc/intel/common/block/uart/uart.c b/src/soc/intel/common/block/uart/uart.c index f0b6b24..f7235cf 100644 --- a/src/soc/intel/common/block/uart/uart.c +++ b/src/soc/intel/common/block/uart/uart.c @@ -88,11 +88,12 @@ struct device *uart_get_device(void) { /* - * This function will get called even if INTEL_LPSS_UART_FOR_CONSOLE - * config option is not selected. - * By default return NULL in this case to avoid compilation errors. + * This function will get called even if UART_DEBUG config options is + * not selected. + * By default returning NULL in case CONFIG_UART_DEBUG option is not + * selected to avoid compilation errors. */ - if (!IS_ENABLED(CONFIG_INTEL_LPSS_UART_FOR_CONSOLE)) + if (!IS_ENABLED(CONFIG_UART_DEBUG)) return NULL;
int console_index = uart_get_valid_index(); @@ -156,8 +157,7 @@ pci_dev_read_resources(dev);
/* Set the configured UART base address for the debug port */ - if (IS_ENABLED(CONFIG_INTEL_LPSS_UART_FOR_CONSOLE) && - uart_is_debug_controller(dev)) { + if (IS_ENABLED(CONFIG_UART_DEBUG) && uart_is_debug_controller(dev)) { struct resource *res = find_resource(dev, PCI_BASE_ADDRESS_0); /* Need to set the base and size for the resource allocator. */ res->base = UART_BASE(CONFIG_UART_FOR_CONSOLE); diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 1d687f0..6343ca5 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -62,6 +62,23 @@ select UDK_2017_BINDING select DISPLAY_FSP_VERSION_INFO
+config UART_DEBUG + bool "Enable UART debug port." + default n + select CONSOLE_SERIAL + select BOOTBLOCK_CONSOLE + select DRIVERS_UART + select DRIVERS_UART_8250MEM_32 + select NO_UART_ON_SUPERIO + +config UART_FOR_CONSOLE + int "Index for LPSS UART port to use for console" + default 2 if DRIVERS_UART_8250MEM_32 + default 0 + help + Index for LPSS UART port to use for console: + 0 = LPSS UART0, 1 = LPSS UART1, 2 = LPSS UART2 + config DCACHE_RAM_BASE default 0xfef00000
diff --git a/src/soc/intel/icelake/Makefile.inc b/src/soc/intel/icelake/Makefile.inc index 74c9182..a81edd4 100644 --- a/src/soc/intel/icelake/Makefile.inc +++ b/src/soc/intel/icelake/Makefile.inc @@ -20,7 +20,7 @@ bootblock-y += spi.c bootblock-y += lpc.c bootblock-y += p2sb.c -bootblock-y += uart.c +bootblock-$(CONFIG_UART_DEBUG) += uart.c
romstage-y += gpio.c romstage-y += gspi.c @@ -30,7 +30,7 @@ romstage-y += pmutil.c romstage-y += reset.c romstage-y += spi.c -romstage-y += uart.c +romstage-$(CONFIG_UART_DEBUG) += uart.c
ramstage-y += acpi.c ramstage-y += chip.c @@ -52,27 +52,27 @@ ramstage-y += smmrelocate.c ramstage-y += spi.c ramstage-y += systemagent.c -ramstage-y += uart.c +ramstage-$(CONFIG_UART_DEBUG) += uart.c ramstage-y += sd.c
smm-y += gpio.c smm-y += p2sb.c smm-y += pmutil.c smm-y += smihandler.c -smm-y += uart.c +smm-$(CONFIG_UART_DEBUG) += uart.c
postcar-y += memmap.c postcar-y += pmutil.c postcar-y += i2c.c postcar-y += gspi.c postcar-y += spi.c -postcar-y += uart.c +postcar-$(CONFIG_UART_DEBUG) += uart.c
verstage-y += gspi.c verstage-y += i2c.c verstage-y += pmutil.c verstage-y += spi.c -verstage-y += uart.c +verstage-$(CONFIG_UART_DEBUG) += uart.c
CPPFLAGS_common += -I$(src)/soc/intel/icelake CPPFLAGS_common += -I$(src)/soc/intel/icelake/include diff --git a/src/soc/intel/icelake/bootblock/bootblock.c b/src/soc/intel/icelake/bootblock/bootblock.c index d26fa42..40c2d41 100644 --- a/src/soc/intel/icelake/bootblock/bootblock.c +++ b/src/soc/intel/icelake/bootblock/bootblock.c @@ -32,7 +32,7 @@ bootblock_pch_early_init(); bootblock_cpu_init(); pch_early_iorange_init(); - if (IS_ENABLED(CONFIG_INTEL_LPSS_UART_FOR_CONSOLE)) + if (IS_ENABLED(CONFIG_UART_DEBUG)) uart_bootblock_init(); }
diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index 7dcf8a9..c2aecfd 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -182,6 +182,22 @@ string default "8086,0406"
+config UART_DEBUG + bool "Enable UART debug port." + default n + select CONSOLE_SERIAL + select DRIVERS_UART + select DRIVERS_UART_8250MEM_32 + select NO_UART_ON_SUPERIO + +config UART_FOR_CONSOLE + int "Index for LPSS UART port to use for console" + default 2 if DRIVERS_UART_8250MEM + default 0 + help + Index for LPSS UART port to use for console: + 0 = LPSS UART0, 1 = LPSS UART1, 2 = LPSS UART2 + config SKYLAKE_SOC_PCH_H bool default n diff --git a/src/soc/intel/skylake/Makefile.inc b/src/soc/intel/skylake/Makefile.inc index ee2c928..480c71e 100644 --- a/src/soc/intel/skylake/Makefile.inc +++ b/src/soc/intel/skylake/Makefile.inc @@ -22,13 +22,13 @@ bootblock-y += pmutil.c bootblock-y += spi.c bootblock-y += lpc.c -bootblock-y += uart.c +bootblock-$(CONFIG_UART_DEBUG) += uart.c
verstage-y += gspi.c verstage-y += pmutil.c verstage-y += i2c.c verstage-y += spi.c -verstage-y += uart.c +verstage-$(CONFIG_UART_DEBUG) += uart.c
romstage-y += gpio.c romstage-y += gspi.c @@ -40,7 +40,7 @@ romstage-y += pmutil.c romstage-$(CONFIG_PLATFORM_USES_FSP2_0) += reset.c romstage-y += spi.c -romstage-y += uart.c +romstage-$(CONFIG_UART_DEBUG) += uart.c
ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi.c ramstage-$(CONFIG_PLATFORM_USES_FSP1_1) += chip.c @@ -67,7 +67,7 @@ ramstage-y += spi.c ramstage-y += systemagent.c ramstage-y += thermal.c -ramstage-y += uart.c +ramstage-$(CONFIG_UART_DEBUG) += uart.c ramstage-y += vr_config.c
smm-y += elog.c @@ -75,13 +75,13 @@ smm-y += p2sb.c smm-y += pmutil.c smm-y += smihandler.c -smm-y += uart.c +smm-$(CONFIG_UART_DEBUG) += uart.c
postcar-y += memmap.c postcar-y += gspi.c postcar-y += spi.c postcar-y += i2c.c -postcar-y += uart.c +postcar-$(CONFIG_UART_DEBUG) += uart.c
# Skylake D0 diff --git a/src/soc/intel/skylake/bootblock/bootblock.c b/src/soc/intel/skylake/bootblock/bootblock.c index acf25ff..a2bcaaf 100644 --- a/src/soc/intel/skylake/bootblock/bootblock.c +++ b/src/soc/intel/skylake/bootblock/bootblock.c @@ -32,7 +32,7 @@ bootblock_cpu_init(); pch_early_iorange_init();
- if (IS_ENABLED(CONFIG_INTEL_LPSS_UART_FOR_CONSOLE)) + if (IS_ENABLED(CONFIG_UART_DEBUG)) uart_bootblock_init(); }
diff --git a/src/soc/intel/skylake/me.c b/src/soc/intel/skylake/me.c index f67086b..64a94ab 100644 --- a/src/soc/intel/skylake/me.c +++ b/src/soc/intel/skylake/me.c @@ -239,7 +239,7 @@ * Print ME version only if UART debugging is enabled. Else, it takes ~1 * second to talk to ME and get this information. */ - if (!IS_ENABLED(CONFIG_CONSOLE_SERIAL)) + if (!IS_ENABLED(CONFIG_UART_DEBUG)) return;
hfs.data = me_read_config32(PCI_ME_HFSTS1);
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30825 )
Change subject: Revert "soc/intel: Clean mess around UART_DEBUG" ......................................................................
Patch Set 2:
Patch Set 2:
As i could see u need this original CL because of some customer requirement but that CL causes issue on all chrome platform that i know like CNL, ICL etc. so, what will be trade-off to satisfy both platfom. because its all IA platform and we want to provide solution to all. :)
There was no customer requirement. Only my own sense for quality. As coreboot quality was degrading much after Intel started to work upstream, I started to clean up after you. As you can imagine, there isn't a lot testing involved when a single person cleans up after whole teams.
The following is from coreboot guideline.
Test your patches before submitting them to gerrit. It’s also appreciated if you add a line to the commit message describing how the patch was tested. This prevents people from having to ask whether and how the patch was tested. Examples of this sort of comment would be ‘TEST=Built platform’ or ‘Tested by building and booting platform’. Stating that the patch was not tested is also fine, although you might be asked to do some testing in cases where that would be reasonable.
I am appreciated for your contribution and thanks a lot for the clean up, please help review my following patch as well.
Lijian Zhao has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/30825 )
Change subject: Revert "soc/intel: Clean mess around UART_DEBUG" ......................................................................
Abandoned
Fix already in.
-
Lijian Zhao (Code Review)