Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42517 )
Change subject: soc/amd/picasso/uart: factor out console-related functions ......................................................................
soc/amd/picasso/uart: factor out console-related functions
Move uart_platform_base and uart_platform_refclk to their own compilation unit to avoid preprocessor usage. The newly created compilation unit is only added to the build when PICASSO_CONSOLE_UART is selected.
Change-Id: I56911addc8c000a0772156e5166720867cdd26fe Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/include/soc/southbridge.h A src/soc/amd/picasso/include/soc/uart.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/picasso/uart.c A src/soc/amd/picasso/uart_console.c 6 files changed, 41 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/42517/1
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 796f77c..39269e9 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -16,6 +16,7 @@ bootblock-y += southbridge.c bootblock-y += i2c.c bootblock-y += uart.c +bootblock-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c bootblock-y += tsc_freq.c bootblock-y += gpio.c bootblock-y += smi_util.c @@ -27,6 +28,7 @@ romstage-y += pmutil.c romstage-y += memmap.c romstage-y += uart.c +romstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c romstage-y += tsc_freq.c romstage-y += aoac.c romstage-y += southbridge.c @@ -41,6 +43,7 @@ verstage-y += config.c verstage-y += aoac.c verstage-y += uart.c +verstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c verstage-y += tsc_freq.c
ramstage-y += i2c.c @@ -61,6 +64,7 @@ ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi.c ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi_util.c ramstage-y += uart.c +ramstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c ramstage-y += usb.c ramstage-y += tsc_freq.c ramstage-y += finalize.c @@ -76,7 +80,10 @@ smm-y += smihandler.c smm-y += smi_util.c smm-y += tsc_freq.c -smm-$(CONFIG_DEBUG_SMI) += uart.c +ifeq ($(CONFIG_DEBUG_SMI),y) +smm-y += uart.c +smm-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c +endif smm-y += gpio.c smm-y += psp.c smm-y += smu.c diff --git a/src/soc/amd/picasso/include/soc/southbridge.h b/src/soc/amd/picasso/include/soc/southbridge.h index 97b0ab4..e9f7e2e 100644 --- a/src/soc/amd/picasso/include/soc/southbridge.h +++ b/src/soc/amd/picasso/include/soc/southbridge.h @@ -287,8 +287,6 @@ void southbridge_init(void *chip_info); void fch_pre_init(void); void fch_early_init(void); -void set_uart_config(int idx); -void clear_uart_legacy_config(void);
/* Initialize all the i2c buses that are marked with early init. */ void i2c_soc_early_init(void); diff --git a/src/soc/amd/picasso/include/soc/uart.h b/src/soc/amd/picasso/include/soc/uart.h new file mode 100644 index 0000000..7cfd40a --- /dev/null +++ b/src/soc/amd/picasso/include/soc/uart.h @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __PICASSO_UART_H__ +#define __PICASSO_UART_H__ + +void set_uart_config(int idx); +void clear_uart_legacy_config(void); + +uintptr_t get_uart_base(int idx); + +#endif /* __PICASSO_UART_H__ */ diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 0ff5606..0d908c3 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -21,6 +21,7 @@ #include <soc/i2c.h> #include <soc/southbridge.h> #include <soc/smi.h> +#include <soc/uart.h> #include <soc/amd_pci_int_defs.h> #include <delay.h> #include <soc/pci_devs.h> diff --git a/src/soc/amd/picasso/uart.c b/src/soc/amd/picasso/uart.c index 0d97fd7..3a79faa 100644 --- a/src/soc/amd/picasso/uart.c +++ b/src/soc/amd/picasso/uart.c @@ -1,12 +1,12 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <console/uart.h> #include <commonlib/helpers.h> #include <device/mmio.h> #include <amdblocks/gpio_banks.h> #include <amdblocks/acpimmio.h> #include <soc/southbridge.h> #include <soc/gpio.h> +#include <soc/uart.h>
static const struct _uart_info { uintptr_t base; @@ -30,17 +30,7 @@ } }, };
-/* - * Don't provide uart_platform_base and uart_platform_refclk functions if PICASSO_CONSOLE_UART - * isn't selected. Those two functions are used by the console UART driver and need to be - * provided exactly once and only by the UART that is used for console. - * - * TODO: Replace the #if block by factoring out the two functions into a different compilation - * unit. - */ -#if CONFIG(PICASSO_CONSOLE_UART) - -uintptr_t uart_platform_base(int idx) +uintptr_t get_uart_base(int idx) { if (idx < 0 || idx >= ARRAY_SIZE(uart_info)) return 0; @@ -48,13 +38,6 @@ return uart_info[idx].base; }
-unsigned int uart_platform_refclk(void) -{ - return CONFIG(PICASSO_UART_48MZ) ? 48000000 : 115200 * 16; -} - -#endif /* PICASSO_CONSOLE_UART */ - void clear_uart_legacy_config(void) { write16((void *)FCH_UART_LEGACY_DECODE, 0); diff --git a/src/soc/amd/picasso/uart_console.c b/src/soc/amd/picasso/uart_console.c new file mode 100644 index 0000000..18ed585 --- /dev/null +++ b/src/soc/amd/picasso/uart_console.c @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <console/uart.h> +#include <soc/uart.h> + +/* + * uart_platform_base and uart_platform_refclk are used by the console UART driver and need to + * be provided exactly once and only by the UART that is used for console. + */ + +uintptr_t uart_platform_base(int idx) +{ + return get_uart_base(idx); +} + +unsigned int uart_platform_refclk(void) +{ + return CONFIG(PICASSO_UART_48MZ) ? 48000000 : 115200 * 16; +} \ No newline at end of file
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42517 )
Change subject: soc/amd/picasso/uart: factor out console-related functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42517/1/src/soc/amd/picasso/uart_co... File src/soc/amd/picasso/uart_console.c:
https://review.coreboot.org/c/coreboot/+/42517/1/src/soc/amd/picasso/uart_co... PS1, Line 19: } adding a line without newline at end of file
Hello Jason Glenesk, Raul Rangel, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42517
to look at the new patch set (#2).
Change subject: soc/amd/picasso/uart: factor out console-related functions ......................................................................
soc/amd/picasso/uart: factor out console-related functions
Move uart_platform_base and uart_platform_refclk to their own compilation unit to avoid preprocessor usage. The newly created compilation unit is only added to the build when PICASSO_CONSOLE_UART is selected.
Change-Id: I56911addc8c000a0772156e5166720867cdd26fe Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/include/soc/southbridge.h A src/soc/amd/picasso/include/soc/uart.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/picasso/uart.c A src/soc/amd/picasso/uart_console.c 6 files changed, 41 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/42517/2
Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Furquan Shaikh, Patrick Georgi, Martin Roth, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42517
to look at the new patch set (#3).
Change subject: soc/amd/picasso/uart: factor out console-related functions ......................................................................
soc/amd/picasso/uart: factor out console-related functions
Move uart_platform_base and uart_platform_refclk to their own compilation unit to avoid preprocessor usage. The newly created compilation unit is only added to the build when PICASSO_CONSOLE_UART is selected.
Change-Id: I56911addc8c000a0772156e5166720867cdd26fe Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/include/soc/southbridge.h A src/soc/amd/picasso/include/soc/uart.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/picasso/uart.c A src/soc/amd/picasso/uart_console.c 6 files changed, 41 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/42517/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42517 )
Change subject: soc/amd/picasso/uart: factor out console-related functions ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42517/3/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/uart.h:
https://review.coreboot.org/c/coreboot/+/42517/3/src/soc/amd/picasso/include... PS3, Line 7: void clear_uart_legacy_config(void); Add some commentary for thefunctions?
https://review.coreboot.org/c/coreboot/+/42517/3/src/soc/amd/picasso/include... PS3, Line 9: uintptr_t #include headers that provide the types you are using.
Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Furquan Shaikh, Patrick Georgi, Martin Roth, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42517
to look at the new patch set (#4).
Change subject: soc/amd/picasso/uart: factor out console-related functions ......................................................................
soc/amd/picasso/uart: factor out console-related functions
Move uart_platform_base and uart_platform_refclk to their own compilation unit to avoid preprocessor usage. The newly created compilation unit is only added to the build when PICASSO_CONSOLE_UART is selected.
Change-Id: I56911addc8c000a0772156e5166720867cdd26fe Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/include/soc/southbridge.h A src/soc/amd/picasso/include/soc/uart.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/picasso/uart.c A src/soc/amd/picasso/uart_console.c 6 files changed, 43 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/42517/4
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42517 )
Change subject: soc/amd/picasso/uart: factor out console-related functions ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42517/3/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/uart.h:
https://review.coreboot.org/c/coreboot/+/42517/3/src/soc/amd/picasso/include... PS3, Line 7: void clear_uart_legacy_config(void);
Add some commentary for thefunctions?
Done
https://review.coreboot.org/c/coreboot/+/42517/3/src/soc/amd/picasso/include... PS3, Line 9: uintptr_t
#include headers that provide the types you are using.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42517 )
Change subject: soc/amd/picasso/uart: factor out console-related functions ......................................................................
Patch Set 4: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42517 )
Change subject: soc/amd/picasso/uart: factor out console-related functions ......................................................................
soc/amd/picasso/uart: factor out console-related functions
Move uart_platform_base and uart_platform_refclk to their own compilation unit to avoid preprocessor usage. The newly created compilation unit is only added to the build when PICASSO_CONSOLE_UART is selected.
Change-Id: I56911addc8c000a0772156e5166720867cdd26fe Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/42517 Reviewed-by: Raul Rangel rrangel@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/include/soc/southbridge.h A src/soc/amd/picasso/include/soc/uart.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/picasso/uart.c A src/soc/amd/picasso/uart_console.c 6 files changed, 43 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 796f77c..39269e9 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -16,6 +16,7 @@ bootblock-y += southbridge.c bootblock-y += i2c.c bootblock-y += uart.c +bootblock-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c bootblock-y += tsc_freq.c bootblock-y += gpio.c bootblock-y += smi_util.c @@ -27,6 +28,7 @@ romstage-y += pmutil.c romstage-y += memmap.c romstage-y += uart.c +romstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c romstage-y += tsc_freq.c romstage-y += aoac.c romstage-y += southbridge.c @@ -41,6 +43,7 @@ verstage-y += config.c verstage-y += aoac.c verstage-y += uart.c +verstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c verstage-y += tsc_freq.c
ramstage-y += i2c.c @@ -61,6 +64,7 @@ ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi.c ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi_util.c ramstage-y += uart.c +ramstage-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c ramstage-y += usb.c ramstage-y += tsc_freq.c ramstage-y += finalize.c @@ -76,7 +80,10 @@ smm-y += smihandler.c smm-y += smi_util.c smm-y += tsc_freq.c -smm-$(CONFIG_DEBUG_SMI) += uart.c +ifeq ($(CONFIG_DEBUG_SMI),y) +smm-y += uart.c +smm-$(CONFIG_PICASSO_CONSOLE_UART) += uart_console.c +endif smm-y += gpio.c smm-y += psp.c smm-y += smu.c diff --git a/src/soc/amd/picasso/include/soc/southbridge.h b/src/soc/amd/picasso/include/soc/southbridge.h index 97b0ab4..e9f7e2e 100644 --- a/src/soc/amd/picasso/include/soc/southbridge.h +++ b/src/soc/amd/picasso/include/soc/southbridge.h @@ -287,8 +287,6 @@ void southbridge_init(void *chip_info); void fch_pre_init(void); void fch_early_init(void); -void set_uart_config(int idx); -void clear_uart_legacy_config(void);
/* Initialize all the i2c buses that are marked with early init. */ void i2c_soc_early_init(void); diff --git a/src/soc/amd/picasso/include/soc/uart.h b/src/soc/amd/picasso/include/soc/uart.h new file mode 100644 index 0000000..3aac936 --- /dev/null +++ b/src/soc/amd/picasso/include/soc/uart.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __PICASSO_UART_H__ +#define __PICASSO_UART_H__ + +#include <types.h> + +void set_uart_config(int idx); /* configure hardware of FCH UART selected by idx */ +void clear_uart_legacy_config(void); /* disable legacy I/O decode for FCH UART */ + +uintptr_t get_uart_base(int idx); /* get MMIO base address of FCH UART */ + +#endif /* __PICASSO_UART_H__ */ diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 0ff5606..0d908c3 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -21,6 +21,7 @@ #include <soc/i2c.h> #include <soc/southbridge.h> #include <soc/smi.h> +#include <soc/uart.h> #include <soc/amd_pci_int_defs.h> #include <delay.h> #include <soc/pci_devs.h> diff --git a/src/soc/amd/picasso/uart.c b/src/soc/amd/picasso/uart.c index 42ae8e6..d892333 100644 --- a/src/soc/amd/picasso/uart.c +++ b/src/soc/amd/picasso/uart.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <acpi/acpigen.h> -#include <console/uart.h> #include <console/console.h> #include <commonlib/helpers.h> #include <device/mmio.h> @@ -9,6 +8,7 @@ #include <amdblocks/acpimmio.h> #include <soc/southbridge.h> #include <soc/gpio.h> +#include <soc/uart.h>
static const struct _uart_info { uintptr_t base; @@ -32,17 +32,7 @@ } }, };
-/* - * Don't provide uart_platform_base and uart_platform_refclk functions if PICASSO_CONSOLE_UART - * isn't selected. Those two functions are used by the console UART driver and need to be - * provided exactly once and only by the UART that is used for console. - * - * TODO: Replace the #if block by factoring out the two functions into a different compilation - * unit. - */ -#if CONFIG(PICASSO_CONSOLE_UART) - -uintptr_t uart_platform_base(int idx) +uintptr_t get_uart_base(int idx) { if (idx < 0 || idx >= ARRAY_SIZE(uart_info)) return 0; @@ -50,13 +40,6 @@ return uart_info[idx].base; }
-unsigned int uart_platform_refclk(void) -{ - return CONFIG(PICASSO_UART_48MZ) ? 48000000 : 115200 * 16; -} - -#endif /* PICASSO_CONSOLE_UART */ - void clear_uart_legacy_config(void) { write16((void *)FCH_UART_LEGACY_DECODE, 0); diff --git a/src/soc/amd/picasso/uart_console.c b/src/soc/amd/picasso/uart_console.c new file mode 100644 index 0000000..3ab2a70 --- /dev/null +++ b/src/soc/amd/picasso/uart_console.c @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <console/uart.h> +#include <soc/uart.h> + +/* + * uart_platform_base and uart_platform_refclk are used by the console UART driver and need to + * be provided exactly once and only by the UART that is used for console. + */ + +uintptr_t uart_platform_base(int idx) +{ + return get_uart_base(idx); +} + +unsigned int uart_platform_refclk(void) +{ + return CONFIG(PICASSO_UART_48MZ) ? 48000000 : 115200 * 16; +}
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Raul Rangel (Code Review)