Hello Andrey Petrov,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39270
to review the following change.
Change subject: mb/ocp/tiogapass: Add UART init in bootblock ......................................................................
mb/ocp/tiogapass: Add UART init in bootblock
Based off code from CB:38840
Signed-off-by: Andrey Petrov anpetrov@fb.com Signed-off-by: David Hendricks dhendrix@fb.com Change-Id: I9a5c17e29173110429d66ec551be5a77b1c15538 --- M src/mainboard/ocp/tiogapass/Kconfig M src/mainboard/ocp/tiogapass/Makefile.inc A src/mainboard/ocp/tiogapass/bootblock.c M src/soc/intel/xeon_sp/bootblock/bootblock.c 4 files changed, 49 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39270/1
diff --git a/src/mainboard/ocp/tiogapass/Kconfig b/src/mainboard/ocp/tiogapass/Kconfig index dfa8f54..0a35697 100644 --- a/src/mainboard/ocp/tiogapass/Kconfig +++ b/src/mainboard/ocp/tiogapass/Kconfig @@ -25,6 +25,7 @@ select SOC_INTEL_XEON_SP select MAINBOARD_USES_FSP2_0 select FSP_CAR + select SUPERIO_ASPEED_AST2400
config MAINBOARD_DIR string diff --git a/src/mainboard/ocp/tiogapass/Makefile.inc b/src/mainboard/ocp/tiogapass/Makefile.inc index f5ea591..ae5fb70 100644 --- a/src/mainboard/ocp/tiogapass/Makefile.inc +++ b/src/mainboard/ocp/tiogapass/Makefile.inc @@ -15,6 +15,8 @@ ## GNU General Public License for more details. ##
+bootblock-y += bootblock.c + ramstage-y += ramstage.c ramstage-$(CONFIG_HAVE_ACPI_TABLES) += fadt.c
diff --git a/src/mainboard/ocp/tiogapass/bootblock.c b/src/mainboard/ocp/tiogapass/bootblock.c new file mode 100644 index 0000000..bed8b1a --- /dev/null +++ b/src/mainboard/ocp/tiogapass/bootblock.c @@ -0,0 +1,41 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2020 The coreboot project Authors. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include <bootblock_common.h> +#include <console/console.h> +#include <device/pci_def.h> +#include <device/pci_ops.h> +#include <intelblocks/lpc_lib.h> +#include <intelblocks/pcr.h> +#include <intelblocks/uart.h> +#include <soc/pci_devs.h> +#include <superio/aspeed/ast2400/ast2400.h> +#include <superio/aspeed/common/aspeed.h> + +#define PID_DMI 0xef + +void bootblock_mainboard_init(void) +{ + /* + * Set up decoding windows on PCH over PCR. The CPUs use two of AST2500 SIO ports, + * one is connected to debug header and another is used as SOL. + */ + pcr_write32(PID_DMI, PCR_DMI_LPCIOD, (0 << 0) | (1 << 4)); + pcr_write32(PID_DMI, PCR_DMI_LPCIOE, (1 << 0) | (1 << 1)); + /* for unidentified reason lpc_io_setup_comm_a_b() doesn't work */ + /* enable com1 and com2 and 0x3f8 and 0x2f8, and 0x2e */ + pci_mmio_write_config32(PCH_DEV_LPC, 0x80, + (1<<28) | (1<<16) | (1<<17) | (0 << 0) | (1 << 4)); + + const pnp_devfn_t serial_dev = PNP_DEV(0x2e, AST2400_SUART1); + aspeed_enable_serial(serial_dev, CONFIG_TTYS0_BASE); + + if (CONFIG(BOOTBLOCK_CONSOLE)) { + console_init(); + } +} diff --git a/src/soc/intel/xeon_sp/bootblock/bootblock.c b/src/soc/intel/xeon_sp/bootblock/bootblock.c index 9eb7ff9..a3b7bce 100644 --- a/src/soc/intel/xeon_sp/bootblock/bootblock.c +++ b/src/soc/intel/xeon_sp/bootblock/bootblock.c @@ -15,11 +15,11 @@ */
#include <bootblock_common.h> +#include <console/console.h> #include <device/pci.h> #include <FsptUpd.h> #include <intelblocks/fast_spi.h> #include <soc/iomap.h> -#include <console/console.h>
const FSPT_UPD temp_ram_init_params = { .FspUpdHeader = { @@ -56,6 +56,9 @@
void bootblock_soc_init(void) { - if (CONFIG(BOOTBLOCK_CONSOLE)) + bootblock_mainboard_init(); + + if (CONFIG(BOOTBLOCK_CONSOLE)) { printk(BIOS_DEBUG, "FSP TempRamInit successful...\n"); + } }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39270 )
Change subject: mb/ocp/tiogapass: Add UART init in bootblock ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39270/1/src/mainboard/ocp/tiogapass... File src/mainboard/ocp/tiogapass/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39270/1/src/mainboard/ocp/tiogapass... PS1, Line 38: if (CONFIG(BOOTBLOCK_CONSOLE)) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/39270/1/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39270/1/src/soc/intel/xeon_sp/bootb... PS1, Line 61: if (CONFIG(BOOTBLOCK_CONSOLE)) { braces {} are not necessary for single statement blocks
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39270 )
Change subject: mb/ocp/tiogapass: Add UART init in bootblock ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39270/1/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
PS1: Style changes should be done separately.
Attention is currently required from: David Hendricks. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39270 )
Change subject: mb/ocp/tiogapass: Add UART init in bootblock ......................................................................
Patch Set 1:
(1 comment)
File src/mainboard/ocp/tiogapass/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39270/comment/c3966795_e71341b1 PS1, Line 24: /* : * Set up decoding windows on PCH over PCR. The CPUs use two of AST2500 SIO ports, : * one is connected to debug header and another is used as SOL. : */ : pcr_write32(PID_DMI, PCR_DMI_LPCIOD, (0 << 0) | (1 << 4)); : pcr_write32(PID_DMI, PCR_DMI_LPCIOE, (1 << 0) | (1 << 1)); : /* for unidentified reason lpc_io_setup_comm_a_b() doesn't work */ : /* enable com1 and com2 and 0x3f8 and 0x2f8, and 0x2e */ : pci_mmio_write_config32(PCH_DEV_LPC, 0x80, : (1<<28) | (1<<16) | (1<<17) | (0 << 0) | (1 << 4)); : not needed anymore. just add select `SOC_INTEL_COMMON_BLOCK_LPC_COMB_ENABLE`
Attention is currently required from: Michael Niewöhner. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39270 )
Change subject: mb/ocp/tiogapass: Add UART init in bootblock ......................................................................
Patch Set 1:
(1 comment)
File src/mainboard/ocp/tiogapass/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39270/comment/ed5c740e_787d9c63 PS1, Line 24: /* : * Set up decoding windows on PCH over PCR. The CPUs use two of AST2500 SIO ports, : * one is connected to debug header and another is used as SOL. : */ : pcr_write32(PID_DMI, PCR_DMI_LPCIOD, (0 << 0) | (1 << 4)); : pcr_write32(PID_DMI, PCR_DMI_LPCIOE, (1 << 0) | (1 << 1)); : /* for unidentified reason lpc_io_setup_comm_a_b() doesn't work */ : /* enable com1 and com2 and 0x3f8 and 0x2f8, and 0x2e */ : pci_mmio_write_config32(PCH_DEV_LPC, 0x80, : (1<<28) | (1<<16) | (1<<17) | (0 << 0) | (1 << 4)); :
not needed anymore. […]
Thanks! I actually tried rebasing this patch with your suggestion, but it looks like much of it has already been merged except for selecting `SOC_INTEL_COMMON_BLOCK_LPC_COMB_ENABLE`.
I don't have Tioga Pass hardware anymore, so I can't test it and don't know if this option is needed. That said, perhaps it's best to abandon this patch.
David Hendricks has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39270 )
Change subject: mb/ocp/tiogapass: Add UART init in bootblock ......................................................................
Abandoned
Obsolete?
-
build bot (Jenkins) (Code Review) -
David Hendricks (Code Review)
-
Michael Niewöhner (Code Review)
-
Paul Menzel (Code Review)