Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45389 )
Change subject: nb/intel/x4x: Clean up TPM-related code ......................................................................
nb/intel/x4x: Clean up TPM-related code
Perform the read to the TPM base address using <arch/mmio.h> functions. Remove dead variable assignment and rename TPM base address macro.
Tested with BUILD_TIMELESS=1. Asus P5QL PRO remains identical.
Change-Id: I11d737903c57fce768b760fe717564dae8879ad0 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/x4x/bootblock.c M src/northbridge/intel/x4x/iomap.h 2 files changed, 5 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/45389/1
diff --git a/src/northbridge/intel/x4x/bootblock.c b/src/northbridge/intel/x4x/bootblock.c index 328464a..baa4ae3 100644 --- a/src/northbridge/intel/x4x/bootblock.c +++ b/src/northbridge/intel/x4x/bootblock.c @@ -1,17 +1,17 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <arch/bootblock.h> +#include <arch/mmio.h> #include <device/pci_ops.h> + #include "x4x.h" #include "iomap.h"
void bootblock_early_northbridge_init(void) { - uint32_t reg32; - /* Disable LaGrande Technology (LT) */ - reg32 = TPM32(0); + read32((void *)TPM_BASE_ADDRESS);
- reg32 = CONFIG_MMCONF_BASE_ADDRESS | 16 | 1; + const uint32_t reg32 = CONFIG_MMCONF_BASE_ADDRESS | 16 | 1; pci_io_write_config32(HOST_BRIDGE, D0F0_PCIEXBAR_LO, reg32); } diff --git a/src/northbridge/intel/x4x/iomap.h b/src/northbridge/intel/x4x/iomap.h index d016cf7..22a675f 100644 --- a/src/northbridge/intel/x4x/iomap.h +++ b/src/northbridge/intel/x4x/iomap.h @@ -8,7 +8,6 @@ #define DEFAULT_EPBAR 0xfed19000 /* 4 KB */ #define DEFAULT_HECIBAR 0xfed10000
-#define TPMBASE 0xfed40000 -#define TPM32(x) (*((volatile u32 *)(TPMBASE + (x)))) +#define TPM_BASE_ADDRESS 0xfed40000
#endif /* X4X_IOMAP_H */
Hello build bot (Jenkins), Damien Zammit, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45389
to look at the new patch set (#3).
Change subject: nb/intel/x4x: Clean up TPM-related code ......................................................................
nb/intel/x4x: Clean up TPM-related code
Perform the read to the TPM base address using <arch/mmio.h> functions. Remove dead variable assignment and rename TPM base address macro.
Tested with BUILD_TIMELESS=1. Asus P5QL PRO remains identical.
Change-Id: I11d737903c57fce768b760fe717564dae8879ad0 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/x4x/bootblock.c M src/northbridge/intel/x4x/iomap.h 2 files changed, 5 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/45389/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45389 )
Change subject: nb/intel/x4x: Clean up TPM-related code ......................................................................
Patch Set 4: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/45389/4/src/northbridge/intel/x4x/b... File src/northbridge/intel/x4x/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45389/4/src/northbridge/intel/x4x/b... PS4, Line 12: /* Disable LaGrande Technology (LT) */ that's just plain wrong.
https://review.coreboot.org/c/coreboot/+/45389/4/src/northbridge/intel/x4x/b... PS4, Line 13: reg32 = TPM32(0); you want to wait for the TPM_ACCESS_VALID bit set, so this code is again very wrong.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45389 )
Change subject: nb/intel/x4x: Clean up TPM-related code ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45389/4/src/northbridge/intel/x4x/b... File src/northbridge/intel/x4x/bootblock.c:
https://review.coreboot.org/c/coreboot/+/45389/4/src/northbridge/intel/x4x/b... PS4, Line 12: /* Disable LaGrande Technology (LT) */
that's just plain wrong.
From reference code itself:
_prepareToCallMemRefCode: ; Perform read to TPM public space to identify non-LT platform to MCH mov esi, 0FED40000h mov eax, fs:[esi]
If anything, I can change things in CB:45390
https://review.coreboot.org/c/coreboot/+/45389/4/src/northbridge/intel/x4x/b... PS4, Line 13: reg32 = TPM32(0);
you want to wait for the TPM_ACCESS_VALID bit set, so this code is again very wrong.
I think none of the supported x4x boards has a TXT-capable MCH. I have an HP machine with a TXT-capable MCH, so I could try to see what happens there. It's not currently with me, though.
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45389 )
Change subject: nb/intel/x4x: Clean up TPM-related code ......................................................................
nb/intel/x4x: Clean up TPM-related code
Perform the read to the TPM base address using <arch/mmio.h> functions. Remove dead variable assignment and rename TPM base address macro.
Tested with BUILD_TIMELESS=1. Asus P5QL PRO remains identical.
Change-Id: I11d737903c57fce768b760fe717564dae8879ad0 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45389 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/northbridge/intel/x4x/bootblock.c M src/northbridge/intel/x4x/iomap.h 2 files changed, 5 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/src/northbridge/intel/x4x/bootblock.c b/src/northbridge/intel/x4x/bootblock.c index 328464a..baa4ae3 100644 --- a/src/northbridge/intel/x4x/bootblock.c +++ b/src/northbridge/intel/x4x/bootblock.c @@ -1,17 +1,17 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <arch/bootblock.h> +#include <arch/mmio.h> #include <device/pci_ops.h> + #include "x4x.h" #include "iomap.h"
void bootblock_early_northbridge_init(void) { - uint32_t reg32; - /* Disable LaGrande Technology (LT) */ - reg32 = TPM32(0); + read32((void *)TPM_BASE_ADDRESS);
- reg32 = CONFIG_MMCONF_BASE_ADDRESS | 16 | 1; + const uint32_t reg32 = CONFIG_MMCONF_BASE_ADDRESS | 16 | 1; pci_io_write_config32(HOST_BRIDGE, D0F0_PCIEXBAR_LO, reg32); } diff --git a/src/northbridge/intel/x4x/iomap.h b/src/northbridge/intel/x4x/iomap.h index d016cf7..22a675f 100644 --- a/src/northbridge/intel/x4x/iomap.h +++ b/src/northbridge/intel/x4x/iomap.h @@ -8,7 +8,6 @@ #define DEFAULT_EPBAR 0xfed19000 /* 4 KB */ #define DEFAULT_HECIBAR 0xfed10000
-#define TPMBASE 0xfed40000 -#define TPM32(x) (*((volatile u32 *)(TPMBASE + (x)))) +#define TPM_BASE_ADDRESS 0xfed40000
#endif /* X4X_IOMAP_H */
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45389 )
Change subject: nb/intel/x4x: Clean up TPM-related code ......................................................................
Patch Set 5:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19824 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19823 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19822 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19821 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19820 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19828 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19827 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19826 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19825
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Patrick Rudolph (Code Review)