Hello Jes Klinke,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to review the following change.
Change subject: WIP: A bunch of experiments for cr50 long pulses ......................................................................
WIP: A bunch of experiments for cr50 long pulses
Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/arch/x86/car.ld M src/commonlib/include/commonlib/cbmem_id.h M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/security/vboot/Makefile.inc A src/security/vboot/cr50.c A src/security/vboot/cr50.h M src/security/vboot/vboot_logic.c M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 14 files changed, 182 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 17b7748..4ba7d9a 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -58,6 +58,11 @@ . += 80; _ecar_ehci_dbg_info = .;
+ /* Reserve space for a uint32_t */ + . = ALIGN(4); + _car_tpm_board_cfg = .; + . += 4; + /* _bss and _ebss provide symbols to per-stage * variables that are not shared like the timestamp and the pre-ram * cbmem console. This is useful for clearing this area on a per-stage diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index ac271a0..5b9306c 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -55,6 +55,7 @@ #define CBMEM_ID_TCPA_TCG_LOG 0x54445041 #define CBMEM_ID_TIMESTAMP 0x54494d45 #define CBMEM_ID_TPM2_TCG_LOG 0x54504d32 +#define CBMEM_ID_TPM_BOARD_CFG 0x92263f3b #define CBMEM_ID_VBOOT_HANDOFF 0x780074f0 /* deprecated */ #define CBMEM_ID_VBOOT_SEL_REG 0x780074f1 /* deprecated */ #define CBMEM_ID_VBOOT_WORKBUF 0x78007343 @@ -117,6 +118,7 @@ { CBMEM_ID_TCPA_TCG_LOG, "TCPA TCGLOG" }, \ { CBMEM_ID_TIMESTAMP, "TIME STAMP " }, \ { CBMEM_ID_TPM2_TCG_LOG, "TPM2 TCGLOG" }, \ + { CBMEM_ID_TPM_BOARD_CFG, "TPM BRD CFG" }, \ { CBMEM_ID_VBOOT_HANDOFF, "VBOOT " }, \ { CBMEM_ID_VBOOT_SEL_REG, "VBOOT SEL " }, \ { CBMEM_ID_VBOOT_WORKBUF, "VBOOT WORK " }, \ diff --git a/src/drivers/spi/tpm/tpm.c b/src/drivers/spi/tpm/tpm.c index 24851d1..344ae1a 100644 --- a/src/drivers/spi/tpm/tpm.c +++ b/src/drivers/spi/tpm/tpm.c @@ -31,6 +31,7 @@ #define TPM_DID_VID_REG (TPM_LOCALITY_0_SPI_BASE + 0xf00) #define TPM_RID_REG (TPM_LOCALITY_0_SPI_BASE + 0xf04) #define TPM_FW_VER (TPM_LOCALITY_0_SPI_BASE + 0xf90) +#define TPM_BOARD_CFG (TPM_LOCALITY_0_SPI_BASE + 0xfe0)
#define CR50_TIMEOUT_INIT_MS 30000 /* Very long timeout for TPM init */
@@ -39,6 +40,7 @@
/* Cached TPM device identification. */ static struct tpm2_info tpm_info; +static int cr50_firmware_version[3];
/* * TODO(vbendeb): make CONFIG_DEBUG_TPM an int to allow different level of @@ -59,6 +61,11 @@ *info = tpm_info; }
+const int *cr50_get_firmware_version(void) +{ + return cr50_firmware_version; +} + __weak int tis_plat_irq_status(void) { static int warning_displayed; @@ -344,6 +351,19 @@ }
/* + * XXX + */ +int cr50_read_board_cfg(uint32_t *status) +{ + return tpm2_read_reg(TPM_BOARD_CFG, status, sizeof(*status)); +} + +int cr50_write_board_cfg(uint32_t status) +{ + return tpm2_write_reg(TPM_BOARD_CFG, &status, sizeof(status)); +} + +/* * The TPM may limit the transaction bytes count (burst count) below the 64 * bytes max. The current value is available as a field of the status * register. @@ -493,6 +513,7 @@ * Locality claimed, read the revision value and set up the tpm_info * structure. */ + tpm2_read_reg(TPM_RID_REG, &cmd, sizeof(cmd)); tpm_info.vendor_id = did_vid & 0xffff; tpm_info.device_id = did_vid >> 16; @@ -503,6 +524,7 @@
/* Let's report device FW version if available. */ if (tpm_info.vendor_id == 0x1ae0) { + static const char *version_prefix = " RW_.:"; int chunk_count = 0; size_t chunk_size; /* @@ -511,6 +533,10 @@ */ char vstr[51];
+ const char *pp = version_prefix; + int state = -1; + memset(cr50_firmware_version, 0, sizeof(cr50_firmware_version)); + chunk_size = sizeof(vstr) - 1;
printk(BIOS_INFO, "Firmware version: "); @@ -524,16 +550,40 @@ /* Print it out in sizeof(vstr) - 1 byte chunks. */ vstr[chunk_size] = 0; do { + const char *p; tpm2_read_reg(TPM_FW_VER, vstr, chunk_size); printk(BIOS_INFO, "%s", vstr);
+ for (p = vstr; *p; p++) { + if (state == -1) { + /* Looking for prefix */ + if (*p == *pp || *pp == '.') { + pp++; + } else { + pp = version_prefix; + } + if (!*pp) { + state = 0; + } + } else if (state < 3) { + /* Parsing a version number */ + if (*p >= '0' && *p <= '9') { + cr50_firmware_version[state] = 10 * cr50_firmware_version[state] + (*p - '0'); + } else if (*p == '.') { + ++state; + } else { + state = 3; + } + } + } + /* * While string is not over, and is no longer than 300 * characters. */ } while (vstr[chunk_size - 1] && (chunk_count++ < (300 / chunk_size))); - + printk(BIOS_INFO, "\n"); } return 0; diff --git a/src/drivers/spi/tpm/tpm.h b/src/drivers/spi/tpm/tpm.h index be98ed0..eb2f81b 100644 --- a/src/drivers/spi/tpm/tpm.h +++ b/src/drivers/spi/tpm/tpm.h @@ -41,4 +41,11 @@ /* Get information about previously initialized TPM device. */ void tpm2_get_info(struct tpm2_info *info);
+/* Returns a pointer to an array of three integers. */ +const int *cr50_get_firmware_version(void); + +int cr50_read_board_cfg(uint32_t *value); +int cr50_write_board_cfg(uint32_t value); + + #endif /* ! __COREBOOT_SRC_DRIVERS_SPI_TPM_TPM_H */ diff --git a/src/mainboard/google/volteer/Kconfig b/src/mainboard/google/volteer/Kconfig index 16834ae..d6fcf15 100644 --- a/src/mainboard/google/volteer/Kconfig +++ b/src/mainboard/google/volteer/Kconfig @@ -57,6 +57,9 @@ config DRIVER_TPM_SPI_BUS default 0x1
+config TPM_BOARD_CFG + default 0x1 + config MAINBOARD_DIR string default "google/volteer" diff --git a/src/mainboard/google/volteer/chromeos.c b/src/mainboard/google/volteer/chromeos.c index 09642a0..3c04ebf 100644 --- a/src/mainboard/google/volteer/chromeos.c +++ b/src/mainboard/google/volteer/chromeos.c @@ -6,7 +6,9 @@
#include <baseboard/variants.h> #include <boot/coreboot_tables.h> +#include <cbmem.h> #include <gpio.h> +#include <soc/ramstage.h> #include <variant/gpio.h> #include <vendorcode/google/chromeos/chromeos.h>
@@ -36,3 +38,17 @@ gpios = variant_cros_gpios(&num); chromeos_acpi_gpio_generate(gpios, num); } + +void mainboard_silicon_init_params(FSP_S_CONFIG *params) { + uint32_t *status = (uint32_t*)cbmem_find(CBMEM_ID_TPM_BOARD_CFG); + if (status && (*status & 0x01)) { + printk(BIOS_INFO, "Enabling S0i3.4\n"); + } else { + /* Disable S03.4, preventing the GPIO block from switching to + * slow clock. */ + printk(BIOS_INFO, "Not enabling S0i3.4\n"); + params->LpmStateEnableMask &= ~0x80; + } +} + + diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb index e0d3bea..dac7fd4 100644 --- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb @@ -207,6 +207,7 @@
# Enable S0ix register "s0ix_enable" = "1" + register "LpmStateEnableMask" = "0xFF"
# Enable DPTF register "dptf_enable" = "1" diff --git a/src/security/tpm/tss/vendor/cr50/Kconfig b/src/security/tpm/tss/vendor/cr50/Kconfig index f606459..3c16057 100644 --- a/src/security/tpm/tss/vendor/cr50/Kconfig +++ b/src/security/tpm/tss/vendor/cr50/Kconfig @@ -12,4 +12,8 @@ help Power off machine while waiting for CR50 update to take effect.
+config TPM_BOARD_CFG + hex "Value to put into Cr50 BOARD_CFG register from RO firmware" + default 0x0 + endif diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 90b2756..96057e9 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -52,6 +52,11 @@ verstage-y += bootmode.c postcar-y += bootmode.c
+ifneq ($(CONFIG_TPM_BOARD_CFG),0) +romstage-$(CONFIG_TPM_CR50) += cr50.c +verstage-$(CONFIG_TPM_CR50) += cr50.c +endif + verstage-generic-ccopts += -D__VERSTAGE__
bootblock-y += vbnv.c diff --git a/src/security/vboot/cr50.c b/src/security/vboot/cr50.c new file mode 100644 index 0000000..0253e6d --- /dev/null +++ b/src/security/vboot/cr50.c @@ -0,0 +1,67 @@ +#include <cbmem.h> +#include <console/console.h> +#include <drivers/spi/tpm/tpm.h> +#include <string.h> +#include "cr50.h" + +/** + * Variable persisted across CAR stages, defined in car.ld. + */ +extern uint32_t _car_tpm_board_cfg; + +/** + * Set the BOARD_CFG register on the TMP chip to a particular compile-time + * constant value. + */ +void set_cr50_board_cfg(void) { + const int *version = cr50_get_firmware_version(); + + /* Cr50 supports the TPM_BOARD_CFG register from version 0.5.4 / 0.6.4 + * and onwards. */ + if (version[0] == 0 && version[1] < 7 && + (version[1] < 5 || version[2] < 4)) { + printk(BIOS_INFO, "Cr50 firmware does not support" + " TPM_BOARD_CFG, version: %d.%d.%d\n", + version[0], version[1], version[2]); + return; + } + /* Set the TPM_BOARD_CFG register, for e.g. asking cr50 to use + * longer ready pulses. */ + uint32_t status; + if (!cr50_read_board_cfg(&status)) { + printk(BIOS_INFO, "Error reading from cr50\n"); + return; + } + if (status & 0x80000000U) { + /* The high bit is set, meaning that the Cr50 is already locked + * on a particular value for the register. Verify that it is + * the value we wanted. */ + _car_tpm_board_cfg = status & ~0xC0000000U; + printk(BIOS_INFO, "%sCurrent Cr50 TPM_BOARD_CFG = 0x%08x, " + "desired = 0x%08x\n", + (_car_tpm_board_cfg != CONFIG_TPM_BOARD_CFG + ? "Inconsistency!: " : ""), + status, CONFIG_TPM_BOARD_CFG); + return; + } + printk(BIOS_INFO, "Current Cr50 TPM_BOARD_CFG = 0x%08x, " + "setting to 0x%08x\n", + status, CONFIG_TPM_BOARD_CFG); + if (!cr50_write_board_cfg(CONFIG_TPM_BOARD_CFG)) { + printk(BIOS_INFO, "Error writing to cr50\n"); + return; + } + _car_tpm_board_cfg = status; +} + +static void cr50_setup_cbmem(int unused) +{ + /* Move state from CAR to CBMEM. */ + uint32_t *tpm_board_cfg_cbmem = cbmem_add(CBMEM_ID_TPM_BOARD_CFG, + sizeof(_car_tpm_board_cfg)); + if (!tpm_board_cfg_cbmem) + return; + memcpy(tpm_board_cfg_cbmem, &_car_tpm_board_cfg, + sizeof(_car_tpm_board_cfg)); +} +ROMSTAGE_CBMEM_INIT_HOOK(cr50_setup_cbmem) diff --git a/src/security/vboot/cr50.h b/src/security/vboot/cr50.h new file mode 100644 index 0000000..7fcdda1 --- /dev/null +++ b/src/security/vboot/cr50.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef __VBOOT_CR50_H__ +#define __VBOOT_CR50_H__ + +void set_cr50_board_cfg(void); + +#endif diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index e23dcc4..209aeaf 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -16,6 +16,7 @@ #include <boot_device.h>
#include "antirollback.h" +#include "cr50.h"
/* The max hash size to expect is for SHA512. */ #define VBOOT_MAX_HASH_SIZE VB2_SHA512_DIGEST_SIZE @@ -299,6 +300,9 @@ if (vboot_setup_tpm(ctx) == TPM_SUCCESS) { antirollback_read_space_firmware(ctx); antirollback_read_space_kernel(ctx); +#if CONFIG(TPM_CR50) && CONFIG_TPM_BOARD_CFG + set_cr50_board_cfg(); +#endif } timestamp_add_now(TS_END_TPMINIT);
diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h index 59dab58..2c6d49f 100644 --- a/src/soc/intel/tigerlake/chip.h +++ b/src/soc/intel/tigerlake/chip.h @@ -76,6 +76,9 @@
/* Enable S0iX support */ int s0ix_enable; + /* S0iX: Selectively enable individual sub-states */ + uint8_t LpmStateEnableMask; + /* Support for TCSS xhci, xdci, TBT PCIe root ports and DMA controllers */ uint8_t TcssD3HotEnable; /* Support for TBT PCIe root ports and DMA controllers with D3Hot->D3Cold */ diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c index cf24021..fc50c8a 100644 --- a/src/soc/intel/tigerlake/fsp_params.c +++ b/src/soc/intel/tigerlake/fsp_params.c @@ -207,6 +207,13 @@ sizeof(params->SataPortsDevSlp)); }
+ /* S0iX: Selectively enable individual sub-states. + * + * LPM0-s0i2.0, LPM1-s0i2.1, LPM2-s0i2.2, LPM3-s0i3.0, + * LPM4-s0i3.1, LPM5-s0i3.2, LPM6-s0i3.3, LPM7-s0i3.4 + */ + params->LpmStateEnableMask = config->LpmStateEnableMask; + /* * Power Optimizer for DMI and SATA. * DmiPwrOptimizeDisable and SataPwrOptimizeDisable is default to 0.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: WIP: A bunch of experiments for cr50 long pulses ......................................................................
Patch Set 1:
(15 comments)
https://review.coreboot.org/c/coreboot/+/43741/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/1/src/drivers/spi/tpm/tpm.c@5... PS1, Line 516: trailing whitespace
https://review.coreboot.org/c/coreboot/+/43741/1/src/drivers/spi/tpm/tpm.c@5... PS1, Line 516: code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43741/1/src/drivers/spi/tpm/tpm.c@5... PS1, Line 516: please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43741/1/src/drivers/spi/tpm/tpm.c@5... PS1, Line 560: if (*p == *pp || *pp == '.') { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/43741/1/src/drivers/spi/tpm/tpm.c@5... PS1, Line 565: if (!*pp) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/43741/1/src/drivers/spi/tpm/tpm.c@5... PS1, Line 570: if (*p >= '0' && *p <= '9') { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/43741/1/src/drivers/spi/tpm/tpm.c@5... PS1, Line 571: cr50_firmware_version[state] = 10 * cr50_firmware_version[state] + (*p - '0'); line over 96 characters
https://review.coreboot.org/c/coreboot/+/43741/1/src/drivers/spi/tpm/tpm.c@5... PS1, Line 586: trailing whitespace
https://review.coreboot.org/c/coreboot/+/43741/1/src/mainboard/google/voltee... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/43741/1/src/mainboard/google/voltee... PS1, Line 42: void mainboard_silicon_init_params(FSP_S_CONFIG *params) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/43741/1/src/mainboard/google/voltee... PS1, Line 43: uint32_t *status = (uint32_t*)cbmem_find(CBMEM_ID_TPM_BOARD_CFG); "(foo*)" should be "(foo *)"
https://review.coreboot.org/c/coreboot/+/43741/1/src/security/vboot/cr50.c File src/security/vboot/cr50.c:
https://review.coreboot.org/c/coreboot/+/43741/1/src/security/vboot/cr50.c@1... PS1, Line 16: void set_cr50_board_cfg(void) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/43741/1/src/security/vboot/cr50.c@5... PS1, Line 59: /* Move state from CAR to CBMEM. */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43741/1/src/security/vboot/cr50.c@5... PS1, Line 59: /* Move state from CAR to CBMEM. */ please, no space before tabs
https://review.coreboot.org/c/coreboot/+/43741/1/src/security/vboot/cr50.c@5... PS1, Line 59: /* Move state from CAR to CBMEM. */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43741/1/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/43741/1/src/soc/intel/tigerlake/fsp... PS1, Line 210: /* S0iX: Selectively enable individual sub-states. code indent should use tabs where possible
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#2).
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Enable long cr50 ready pulses for Tigerlake systems
New Kconfig setting TPM_BOARD_CFG controls new code in verstage, which will program the given value into a register, provided that Cr50 firmware is new enough to support the register.
Knowledge of whether the register was set or not is propagated through CAR and CBMEM to the point in early ramstage where parameters are passed to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses.
Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/arch/x86/car.ld M src/commonlib/include/commonlib/cbmem_id.h M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/security/vboot/Makefile.inc A src/security/vboot/cr50.c A src/security/vboot/cr50.h M src/security/vboot/vboot_logic.c M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 14 files changed, 182 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG@15 PS2, Line 15: to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 I only skimmed the bug but it sounds like all this passing around is only necessary because trying to initialize the Cr50 SPI before FSP-S leads to some not understood problems when trying to use it again afterwards, which doesn't sound like a very satisfactory reason. Is there no way to find and fix that problem? In general, I would say blobs are not allowed to disturb coreboot interfaces (I have to keep yelling at Qualcomm for that all the time too ;) ), so this is a platform problem that the blob owner needs to fix (at least with a workaround, e.g. figuring out what exactly needs to be reinitialized afterwards and then doing that at the end of the coreboot code that handles the blob execution).
Alternatively, would it maybe work out to move all the ramstage CR50 accesses before FSP-S? I don't think there's any explicit reason why the update check currently runs where it does, it could run a bit earlier. We could just combine this all in one big "handle random Cr50 tasks" function that runs in BS_PRE_DEVICE/BS_ON_ENTRY (i.e. before everything else).
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c File src/security/vboot/cr50.c:
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c@2... PS2, Line 26: return; Is all the version checking really necessary, btw? What happens if you try to read the board_cfg register on an older version? If it gives you a consistent error response, can't we just use that to detect whether this is supported?
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/vboot_lo... PS2, Line 304: set_cr50_board_cfg(); This has nothing to do with vboot, please don't put this here. I would consider this a TPM initialization task so it should be with the other TPM initialization stuff in src/drivers/spi/tpm/tpm.c. Look for the ENV_BOOTBLOCK || ... check that guards tpm_claim_locality(), that's basically the "is this the first stage using the TPM" check. We should factor that out into a separate function and use it to kick off this as well right after we read the version. (It's technically CR50-specific but so are still many other things in that file unfortunately, so I'd say just guard it with a CONFIG(TPM_CR50) for now and at some point in the future we'll have to separate all those points out into cleaner interfaces.)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG@15 PS2, Line 15: to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4
all this passing around is only necessary because trying to initialize the Cr50 SPI before FSP-S leads to some not understood problems when trying to use it again afterwards, which doesn't sound like a very satisfactory reason. Is there no way to find and fix that problem?
+1. Let's please debug the issue we are seeing rather than working around it. From the symptoms currently, it seems like the issue is mostly because of some broken behavior within FSP. Have you dumped the GSPI registers before and after FSP has run to see the difference? Also added some more comments on the bug.
Jes Klinke has removed Jes Klinke from this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Removed reviewer Jes Klinke.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG@15 PS2, Line 15: to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4
all this passing around is only necessary because trying to initialize the Cr50 SPI before FSP-S l […]
The very first proposal, was to add the functionality without adding one more point at which the AP firmware pulls the Cr50 version string, but once I realized that it was not straightforward to pass data from verstage to ramstage, I as a fallback tried adding code to read the version also in early ramstage.
When that turned out to trigger issues which we still understand poorly, that made me want to spend a little more time working on the ideal solution, which adds the minimum amount of boot time, rather than spending the same or more time understanding the FSP issue, allowing me to finish the slower solution.
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c File src/security/vboot/cr50.c:
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c@2... PS2, Line 26: return;
Is all the version checking really necessary, btw? What happens if you try to read the board_cfg reg […]
I asked Namyoon this question, and he told me that prior versions of the cr50 firmware will give random uninitialized data, if trying to read a register it does not recognize. (Anyone remember heartbleed.)
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/vboot_lo... PS2, Line 304: set_cr50_board_cfg();
This has nothing to do with vboot, please don't put this here. […]
This is a good point, I will work on that...
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#3).
Change subject: WIP: A bunch of experiments for cr50 long pulses ......................................................................
WIP: A bunch of experiments for cr50 long pulses
Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/arch/x86/car.ld M src/commonlib/include/commonlib/cbmem_id.h M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/security/vboot/Makefile.inc A src/security/vboot/cr50.c A src/security/vboot/cr50.h M src/security/vboot/vboot_logic.c M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 14 files changed, 182 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: WIP: A bunch of experiments for cr50 long pulses ......................................................................
Patch Set 3:
(11 comments)
https://review.coreboot.org/c/coreboot/+/43741/3/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/3/src/drivers/spi/tpm/tpm.c@5... PS3, Line 560: if (*p == *pp || *pp == '.') { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/43741/3/src/drivers/spi/tpm/tpm.c@5... PS3, Line 565: if (!*pp) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/43741/3/src/drivers/spi/tpm/tpm.c@5... PS3, Line 570: if (*p >= '0' && *p <= '9') { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/43741/3/src/drivers/spi/tpm/tpm.c@5... PS3, Line 571: cr50_firmware_version[state] = 10 * cr50_firmware_version[state] + (*p - '0'); line over 96 characters
https://review.coreboot.org/c/coreboot/+/43741/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/43741/3/src/mainboard/google/voltee... PS3, Line 42: void mainboard_silicon_init_params(FSP_S_CONFIG *params) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/43741/3/src/mainboard/google/voltee... PS3, Line 43: uint32_t *status = (uint32_t*)cbmem_find(CBMEM_ID_TPM_BOARD_CFG); "(foo*)" should be "(foo *)"
https://review.coreboot.org/c/coreboot/+/43741/3/src/security/vboot/cr50.c File src/security/vboot/cr50.c:
https://review.coreboot.org/c/coreboot/+/43741/3/src/security/vboot/cr50.c@1... PS3, Line 17: void set_cr50_board_cfg(void) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/43741/3/src/security/vboot/cr50.c@6... PS3, Line 60: /* Move state from CAR to CBMEM. */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43741/3/src/security/vboot/cr50.c@6... PS3, Line 60: /* Move state from CAR to CBMEM. */ please, no space before tabs
https://review.coreboot.org/c/coreboot/+/43741/3/src/security/vboot/cr50.c@6... PS3, Line 60: /* Move state from CAR to CBMEM. */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43741/3/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/43741/3/src/soc/intel/tigerlake/fsp... PS3, Line 210: /* S0iX: Selectively enable individual sub-states. code indent should use tabs where possible
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: WIP: A bunch of experiments for cr50 long pulses ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG@15 PS2, Line 15: to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4
The very first proposal, was to add the functionality without adding one more point at which the AP […]
In my opinion, it is not too bad to have the extra call in ramstage to read cr50 version to determine if we want to enable low power state. Also, Volteer family would probably be one of the few (if not the only) to actually need this check. Adding all the extra logic to pass this very specific `tpm_board_cfg` does not seem worth the complexity.
Instead, what you are running into with the GSPI issue is an actual problem that needs to be addressed irrespective of how the solution is implemented here. I think it would be better to fix that problem which would let us have this check in those few boards which really need it.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#4).
Change subject: WIP: A bunch of experiments for cr50 long pulses ......................................................................
WIP: A bunch of experiments for cr50 long pulses
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/arch/x86/car.ld M src/commonlib/include/commonlib/cbmem_id.h M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/security/vboot/vboot_logic.c M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 11 files changed, 180 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/4
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#5).
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Enable long cr50 ready pulses for Tigerlake systems
New Kconfig setting TPM_BOARD_CFG controls new code in verstage, which will program the given value into a register, provided that Cr50 firmware is new enough to support the register.
Knowledge of whether the register was set or not is propagated through CAR and CBMEM to the point in early ramstage where parameters are passed to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/arch/x86/car.ld M src/commonlib/include/commonlib/cbmem_id.h M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/security/vboot/vboot_logic.c M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 11 files changed, 180 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/5
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#6).
Change subject: WIP: A bunch of experiments for cr50 long pulses ......................................................................
WIP: A bunch of experiments for cr50 long pulses
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/arch/x86/car.ld M src/commonlib/include/commonlib/cbmem_id.h M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 10 files changed, 179 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/6
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#7).
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Enable long cr50 ready pulses for Tigerlake systems
New Kconfig setting TPM_BOARD_CFG controls new code in verstage, which will program the given value into a register, provided that Cr50 firmware is new enough to support the register.
Knowledge of whether the register was set or not is propagated through CAR and CBMEM to the point in early ramstage where parameters are passed to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/arch/x86/car.ld M src/commonlib/include/commonlib/cbmem_id.h M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 10 files changed, 179 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/7
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 7:
(2 comments)
I have moved the code out of the vboot directory.
I still think it is a superior solution to communicate with cr50 about the new register only once during boot, rather than dropping the CBMEM and reading the version/register again in early ramstage.
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG@15 PS2, Line 15: to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4
The very first proposal, was to add the functionality without adding one more point at which the AP […]
Regarding Julius' alternative proposal, of moving all the ramstage cr50 communication to be earlier than FSP-S. As far as I can tell, the existing cr50 communication in ramstage is already declared as BS_ON_ENTRY in cr50_enable_update.c, and still, it runs after the FSP-S. (I attempted switching it to BS_PRE_DEVICE, but that did not cause it to run before FSP-S.)
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/vboot_lo... PS2, Line 304: set_cr50_board_cfg();
This is a good point, I will work on that...
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/6/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/6/src/drivers/spi/tpm/tpm.c@6... PS6, Line 662: if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT)) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/43741/6/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/43741/6/src/soc/intel/tigerlake/fsp... PS6, Line 210: /* S0iX: Selectively enable individual sub-states. code indent should use tabs where possible
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 7:
Patch Set 7:
(2 comments)
I have moved the code out of the vboot directory.
I still think it is a superior solution to communicate with cr50 about the new register only once during boot, rather than dropping the CBMEM and reading the version/register again in early ramstage.
But that is at the complexity of making changes across different components which will probably be used only on this one board?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 7:
(8 comments)
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG@15 PS2, Line 15: to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4
Regarding Julius' alternative proposal, of moving all the ramstage cr50 communication to be earlier […]
The version is always loaded the first time the TPM interface is initialized in each stage, so if you don't load it in early ramstage the cr50 update check will just load it later. You're not saving any time by passing things through CBMEM (but you're wasting memory that will be reserved from the OS forever).
BS_PRE_DEVICE is the bootstage where the callback runs (which is previously BS_PAYLOAD_LOAD for the cr50 update). BS_ON_ENTRY/BS_ON_EXIT decides whether to run before or after that stage. From my reading of the FSP-S loading code it is currently loaded in the pre-device stage, so I think if you combine BS_PRE_DEVICE and BS_ON_ENTRY it should run before FSP-S loading.
I agree with Furquan though that it would be better to root cause and eliminate the SPI hardware problem rather than trying to play around with timings to sidestep it.
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@3... PS7, Line 357: int cr50_read_board_cfg(uint32_t *status) Maybe check the version number right in here and return a -1 or something if we know the board doesn't support board_cfg?
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@5... PS7, Line 571: set_cr50_board_cfg(); Twice?
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 600: static Why is this static?
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 650: index = 3; This is super complicated. Can't we just do something like
char version[REASONABLE_MAXIMUM_LETS_SAY_MAYBE_256]; do { read chunks of 50 into version } version[last_read_index + 1] = '\0'; printk(BIOS_INFO, "Firmware version: %s\n", version); /* Next part should probably be encapsulated in separate CONFIG(TPM_CR50)-only function */ char *number = strstr("RW_A:", version); if (!number) number = strstr("RW_B:", version); if (!number) { ...complain... } else { cr50_version_major = skip_atoi(number); if (*number++ != '.') ...error out... cr50_version_minor = skip_atoi(number); if (*number++ != '.') ...error out... cr50_version_rev = skip_atoi(number); }
Looks like coreboot doesn't have a strstr() yet so it's probably time to add one. Just strcmp() in a while() loop, should almost be a one-liner...
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 661: #if CONFIG(TPM_CR50) && CONFIG_TPM_BOARD_CFG Please always use
if (CONFIG(...))
instead of
#if CONFIG(...)
where possible.
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 662: ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT) Let's factor this check out into a separate function (e.g. if (first_access_this_boot()) )
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c File src/security/vboot/cr50.c:
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c@2... PS2, Line 26: return;
I asked Namyoon this question, and he told me that prior versions of the cr50 firmware will give ran […]
... I really wish I was still surprised by that at this point...
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#8).
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Enable long cr50 ready pulses for Tigerlake systems
New Kconfig setting TPM_BOARD_CFG controls new code in verstage, which will program the given value into a register, provided that Cr50 firmware is new enough to support the register.
Knowledge of whether the register was set or not is propagated through CAR and CBMEM to the point in early ramstage where parameters are passed to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/arch/x86/car.ld M src/commonlib/include/commonlib/cbmem_id.h M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/include/string.h M src/lib/string.c M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/soc/intel/common/block/gspi/gspi.c M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 13 files changed, 200 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 8:
(6 comments)
https://review.coreboot.org/c/coreboot/+/43741/8/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/8/src/drivers/spi/tpm/tpm.c@3... PS8, Line 359: uint32_t get_cr50_board_cfg() Bad function definition - uint32_t get_cr50_board_cfg() should probably be uint32_t get_cr50_board_cfg(void)
https://review.coreboot.org/c/coreboot/+/43741/8/src/drivers/spi/tpm/tpm.c@6... PS8, Line 643: if (first_access_this_boot()) { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/43741/8/src/mainboard/google/voltee... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/43741/8/src/mainboard/google/voltee... PS8, Line 45: tlcl_lib_init(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43741/8/src/mainboard/google/voltee... PS8, Line 45: tlcl_lib_init(); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43741/8/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/43741/8/src/soc/intel/common/block/... PS8, Line 263: memset(gspi_base, 0, sizeof(gspi_base)); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43741/8/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/43741/8/src/soc/intel/tigerlake/fsp... PS8, Line 210: /* S0iX: Selectively enable individual sub-states. code indent should use tabs where possible
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#9).
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Enable long cr50 ready pulses for Tigerlake systems
New Kconfig setting TPM_BOARD_CFG controls new code in verstage, which will program the given value into a register, provided that Cr50 firmware is new enough to support the register.
Knowledge of whether the register was set or not is propagated through CAR and CBMEM to the point in early ramstage where parameters are passed to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/include/string.h M src/lib/string.c M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/soc/intel/common/block/gspi/gspi.c M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 11 files changed, 188 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/c/coreboot/+/43741/9/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/9/src/drivers/spi/tpm/tpm.c@3... PS9, Line 354: uint32_t get_cr50_board_cfg() Bad function definition - uint32_t get_cr50_board_cfg() should probably be uint32_t get_cr50_board_cfg(void)
https://review.coreboot.org/c/coreboot/+/43741/9/src/drivers/spi/tpm/tpm.c@6... PS9, Line 638: if (first_access_this_boot()) { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/43741/9/src/mainboard/google/voltee... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/43741/9/src/mainboard/google/voltee... PS9, Line 45: tlcl_lib_init(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43741/9/src/mainboard/google/voltee... PS9, Line 45: tlcl_lib_init(); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43741/9/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/43741/9/src/soc/intel/common/block/... PS9, Line 263: memset(gspi_base, 0, sizeof(gspi_base)); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43741/9/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/43741/9/src/soc/intel/tigerlake/fsp... PS9, Line 210: /* S0iX: Selectively enable individual sub-states. code indent should use tabs where possible
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#10).
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Enable long cr50 ready pulses for Tigerlake systems
New Kconfig setting TPM_BOARD_CFG controls new code in verstage, which will program the given value into a register, provided that Cr50 firmware is new enough to support the register.
Knowledge of whether the register was set or not is propagated through CAR and CBMEM to the point in early ramstage where parameters are passed to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/include/string.h M src/lib/string.c M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/soc/intel/common/block/gspi/gspi.c M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 11 files changed, 186 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/10
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 10:
(6 comments)
https://review.coreboot.org/c/coreboot/+/43741/10/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/10/src/drivers/spi/tpm/tpm.c@... PS10, Line 353: uint32_t get_cr50_board_cfg() Bad function definition - uint32_t get_cr50_board_cfg() should probably be uint32_t get_cr50_board_cfg(void)
https://review.coreboot.org/c/coreboot/+/43741/10/src/drivers/spi/tpm/tpm.c@... PS10, Line 636: if (first_access_this_boot()) { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/43741/10/src/mainboard/google/volte... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/43741/10/src/mainboard/google/volte... PS10, Line 45: tlcl_lib_init(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/43741/10/src/mainboard/google/volte... PS10, Line 45: tlcl_lib_init(); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43741/10/src/soc/intel/common/block... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/43741/10/src/soc/intel/common/block... PS10, Line 263: memset(gspi_base, 0, sizeof(gspi_base)); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/43741/10/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/43741/10/src/soc/intel/tigerlake/fs... PS10, Line 210: /* S0iX: Selectively enable individual sub-states. code indent should use tabs where possible
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#11).
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Enable long cr50 ready pulses for Tigerlake systems
New Kconfig setting TPM_BOARD_CFG controls new code in verstage, which will program the given value into a register, provided that Cr50 firmware is new enough to support the register.
Knowledge of whether the register was set or not is propagated through CAR and CBMEM to the point in early ramstage where parameters are passed to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/include/string.h M src/lib/string.c M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/soc/intel/common/block/gspi/gspi.c M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 11 files changed, 185 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 353: uint32_t get_cr50_board_cfg() Bad function definition - uint32_t get_cr50_board_cfg() should probably be uint32_t get_cr50_board_cfg(void)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 635: if (first_access_this_boot()) { braces {} are not necessary for any arm of this statement
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(7 comments)
PTAL
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG@15 PS2, Line 15: to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4
The version is always loaded the first time the TPM interface is initialized in each stage, so if yo […]
Done
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@3... PS7, Line 357: int cr50_read_board_cfg(uint32_t *status)
Maybe check the version number right in here and return a -1 or something if we know the board doesn […]
I have deleted these access methods, as they do not make sense, now that the logic which previously used them has been moved into this same file.
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@5... PS7, Line 571: set_cr50_board_cfg();
Twice?
Done
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 600: static
Why is this static?
Deleted.
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 650: index = 3;
This is super complicated. Can't we just do something like […]
Done
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 661: #if CONFIG(TPM_CR50) && CONFIG_TPM_BOARD_CFG
Please always use […]
I had put some of the helper methods inside an #if block, so using if() (as opposed to #if) would have resulted in compilation errors on non-Google targets. Now I have made the helper methods static, and removed the #if around them. I assume that the compiler will silently drop these methods from the output, in cases where all their invocations are from dead code in if statements with compile-time constant conditions.
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 662: ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT)
Let's factor this check out into a separate function (e.g. […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(11 comments)
Thanks, looks much better.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 44: tpm_board_cfg I would call this (and probably also the Kconfig) cr50_board_cfg because it has nothing to do with other TPMs.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 441: version[0] Wasn't there a thing where the Cr50 team uses the major number to distinguish between dev and stable versions or something (so 1.5.3 is the stable version corresponding to 0.5.3 or something)? Or am I misremembering things? Please double-check with them that this is really the right check for the feature.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 457: uint32_t status; I find it pretty confusing that this is called 'status'... why not 'board_cfg' or something? (Or just use the tpm_board_cfg global directly?)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 474: ? "Inconsistency!: " : ""), Probably want to print this as a separate BIOS_ERR line if it happens and start with "ERROR: " to make it more noticeable?
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 511: /*const*/ What's this?
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 541: memset(cr50_firmware_version, 0, sizeof(cr50_firmware_version)); nit: unnecessary (globals are already zero-initialized)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 631: 0 nit: I'd write this '\0' for clarity
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 638: load_cr50_board_cfg(); I would only do this when get_cr50_board_cfg() is called, not in every stage.
https://review.coreboot.org/c/coreboot/+/43741/11/src/include/string.h File src/include/string.h:
https://review.coreboot.org/c/coreboot/+/43741/11/src/include/string.h@32 PS11, Line 32: char *strstr(const char *haystack, const char *needle); Please add this as a separate patch.
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c@170 PS11, Line 170: if (!strncmp(haystack, needle, needle_len)) Why not just strcmp()?
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... PS11, Line 261: static void gspi_clear_base_stash(void *unused) I would make this an exported function that is called directly at the end of the FSP-S loading code, that's more precise than boot states (e.g. it could be possible that another device that runs after the SoC wants to access the TPM in its DEV_RESOURCES step).
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(13 comments)
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@6 PS11, Line 6: This change should be split into multiple CLs for each logical change being made.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 355: tpm_board_cfg Do we really need to stash this? I think we can just read it when a caller wants it using tpm2_read_reg().
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 452: TMP TPM
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 466: 0x80000000U Can you please define a macro for this rather than using a magic value here?
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 470: 0xC0000000U Same for this.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... File src/mainboard/google/volteer/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 60: config TPM_BOARD_CFG : default 0x1 Instead of making the mainboard provide a specific value for TPM_BOARD_CFG, I think it would be better to have Kconfigs for specific feature enabling e.g. CR50_ENABLE_LONG_INTERRUPT_PULSE. So, mainboard doesn't really need to care how it is done i.e. what value to use for what register.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 6: This change should go in a separate CL.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 46: 0x01 Use macros please.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 45: tlcl_lib_init(); : if (get_cr50_board_cfg() & 0x01) { I think it would be better to have some helper functions: cr50_has_long_pulse() which returns true using its internal calls to tlcl_lib_init() and get_cr50_board_cfg().
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 52: 0x80 It would be good to add some macros for this as well.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 55: nit: extra blank lines not required.
https://review.coreboot.org/c/coreboot/+/43741/11/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/11/src/security/tpm/tss/vendo... PS11, Line 15: TPM_BOARD_CFG As mentioned on the mainboard file, I think it would be better to add Kconfig for the specific feature being enabled. TPM driver can then take care of translating that into appropriate value for the required register.
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... PS11, Line 261: static void gspi_clear_base_stash(void *unused)
I would make this an exported function that is called directly at the end of the FSP-S loading code, […]
The problem turned to be not the FSP-S invocation, but the change in BAR that happens because of resource allocation in BS_DEV_RESOURCES boot state within coreboot. Hence, the call is added on BS_ON_EXIT for BS_DEV_RESOURCES.
Jes - this change should go in a CL of its own with explanation as to why it is needed.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 621: /* : * Read chunk_size bytes at a time, last chunk will be zero : * padded. : */ : do { I am not sure if firmware version needs to be read in every stage. I see in all the stages, it is read, logged and dropped.
With all the new use-cases in ramstage, we can read the firmware version in ramstage alone.
Also with more than one use-case in ramstage, if the firmware version is already cached we can skip reading the firmware version. I believe it is not going to change within the ramstage.
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c@170 PS11, Line 170: if (!strncmp(haystack, needle, needle_len))
Why not just strcmp()?
The needle may hit '\0' before the haystack. That will cause strcmp to return *haystack - '\0'.
Also for the use-case here, tokenizing the haystack using a " " delimiter can work slightly better than strstr - eg. https://review.coreboot.org/c/coreboot/+/44277/2/src/drivers/spi/tpm/tpm.c#4...
If we take that approach, then strstr can be dropped.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 621: /* : * Read chunk_size bytes at a time, last chunk will be zero : * padded. : */ : do {
I am not sure if firmware version needs to be read in every stage. […]
Update on my previous comment:
Also with more than one use-case in ramstage, if the firmware version is already cached we can skip reading the firmware version. I believe it is not going to change within the ramstage.
It seems the idea is to do tlcl_lib_init() which is a No-op if it is already done. So that means we will not be reading the Firmware version multiple times in ramstage.
The other comment about reading firmware version in ramstage alone still holds. I don't see it being used in any other stages except for logging.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@7 PS11, Line 7: Enable long cr50 ready pulses for Tigerlake systems Missing prefix.
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@7 PS11, Line 7: Tigerlake Tiger Lake
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@20 PS11, Line 20: Bug: b:154333137 Please summarize the bug in the commit message.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#12).
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Enable long cr50 ready pulses for Tigerlake systems
New Kconfig setting TPM_BOARD_CFG controls new code in verstage, which will program the given value into a register, provided that Cr50 firmware is new enough to support the register.
Knowledge of whether the register was set or not is propagated through CAR and CBMEM to the point in early ramstage where parameters are passed to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 8 files changed, 167 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/12/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/12/src/drivers/spi/tpm/tpm.c@... PS12, Line 353: uint32_t get_cr50_board_cfg() Bad function definition - uint32_t get_cr50_board_cfg() should probably be uint32_t get_cr50_board_cfg(void)
https://review.coreboot.org/c/coreboot/+/43741/12/src/drivers/spi/tpm/tpm.c@... PS12, Line 635: if (first_access_this_boot()) { braces {} are not necessary for any arm of this statement
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#13).
Change subject: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 8 files changed, 171 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/13
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/13/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/13/src/drivers/spi/tpm/tpm.c@... PS13, Line 357: uint32_t get_cr50_board_cfg() Bad function definition - uint32_t get_cr50_board_cfg() should probably be uint32_t get_cr50_board_cfg(void)
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#14).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params.c 8 files changed, 171 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/15/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/15/src/drivers/spi/tpm/tpm.c@... PS15, Line 357: uint32_t get_cr50_board_cfg() Bad function definition - uint32_t get_cr50_board_cfg() should probably be uint32_t get_cr50_board_cfg(void)
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#16).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/mainboard/google/volteer/Kconfig M src/mainboard/google/volteer/chromeos.c M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/security/tpm/tss/vendor/cr50/Kconfig 6 files changed, 161 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/16
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#17).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 138 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/17
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/17/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/17/src/drivers/spi/tpm/tpm.c@... PS17, Line 357: uint32_t get_cr50_board_cfg() Bad function definition - uint32_t get_cr50_board_cfg() should probably be uint32_t get_cr50_board_cfg(void)
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#18).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 149 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/18
Jes Klinke has removed Jes Klinke from this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Removed reviewer Jes Klinke.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 18:
(26 comments)
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@6 PS11, Line 6:
This change should be split into multiple CLs for each logical change being made.
Ack
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@7 PS11, Line 7: Tigerlake
Tiger Lake
Done
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@7 PS11, Line 7: Enable long cr50 ready pulses for Tigerlake systems
Missing prefix.
Done
https://review.coreboot.org/c/coreboot/+/43741/11//COMMIT_MSG@20 PS11, Line 20: Bug: b:154333137
Please summarize the bug in the commit message.
I have modified the first paragraph of the comment message to capture the main requirement.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 44: tpm_board_cfg
I would call this (and probably also the Kconfig) cr50_board_cfg because it has nothing to do with o […]
Ack
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 355: tpm_board_cfg
Do we really need to stash this? I think we can just read it when a caller wants it using tpm2_read_ […]
I have changed the logic to do version number check, and conditional reading of Cr50 register "on demand" from this function.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 441: version[0]
Wasn't there a thing where the Cr50 team uses the major number to distinguish between dev and stable […]
It is something like what you describe. 0.5.3 is the stable release that corresponds to dev version 0.6.3, that is why I say that anything 0.7 or above is OK, while 0.5.4+ and 0.6.4+ are also OK.
I will double check with the Cr50 team, whether 0.{5,6}.4 is the right cutoff.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 452: TMP
TPM
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 457: uint32_t status;
I find it pretty confusing that this is called 'status'... […]
Clearly, I had spent too little thought on the code I copied from read_tpm_sts(). I have renamed the variable to board_cfg_value. Here and elsewhere.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 466: 0x80000000U
Can you please define a macro for this rather than using a magic value here?
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 470: 0xC0000000U
Same for this.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 474: ? "Inconsistency!: " : ""),
Probably want to print this as a separate BIOS_ERR line if it happens and start with "ERROR: " to ma […]
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 511: /*const*/
What's this?
I wanted to indicate to readers that this method does not modify its argument. But the skip_atoi() function demands a non-const pointer. It seems however, that through the questionable signature of strstr(), we are able to declare this argument as const for real.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 541: memset(cr50_firmware_version, 0, sizeof(cr50_firmware_version));
nit: unnecessary (globals are already zero-initialized)
Thanks.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 621: /* : * Read chunk_size bytes at a time, last chunk will be zero : * padded. : */ : do {
Update on my previous comment: […]
Correct, the firmware version will not be read and parsed multiple times in the same stage.
I agree that there may be merit in avoiding the repeated logging of Cr50 firmware version. However, since cr50 is heavily involved in the functioning of verstage. When diagnosing problems causing verstage to fail to load a ramstage, it could be useful to know the version of Cr50. So I am not sure that dropping the logging from verstage has no disadvantages, and hence, I would like to not couple it to this functional change.
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 638: load_cr50_board_cfg();
I would only do this when get_cr50_board_cfg() is called, not in every stage.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/include/string.h File src/include/string.h:
https://review.coreboot.org/c/coreboot/+/43741/11/src/include/string.h@32 PS11, Line 32: char *strstr(const char *haystack, const char *needle);
Please add this as a separate patch.
Done. https://review.coreboot.org/c/coreboot/+/44085
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c@170 PS11, Line 170: if (!strncmp(haystack, needle, needle_len))
The needle may hit '\0' before the haystack. That will cause strcmp to return *haystack - '\0'. […]
I think I agree with Julius that the current way of using strstr is probably the simplest way, that is, a random future reader of the code will have easiest time of completely understanding how the code works by merely glancing over it.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... File src/mainboard/google/volteer/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 60: config TPM_BOARD_CFG : default 0x1
Instead of making the mainboard provide a specific value for TPM_BOARD_CFG, I think it would be bett […]
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 6:
This change should go in a separate CL.
Done, please post further comments about the volteer directory in: https://review.coreboot.org/c/coreboot/+/44359
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 46: 0x01
Use macros please.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 45: tlcl_lib_init(); : if (get_cr50_board_cfg() & 0x01) {
I think it would be better to have some helper functions: cr50_has_long_pulse() which returns true u […]
I have added a method get_cr50_ready_pulse_length_us() to drivers/spi/tpm/tpm.c, that way, all knowledge of the meaning of each bit of the board_cfg register (even the existence of such a register), is encapsulated in tpm.c.
However, methods in that file should not call tlcl_lib_init, in my opinion, so I have left the explicit initialization call above.
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 52: 0x80
It would be good to add some macros for this as well.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/mainboard/google/volte... PS11, Line 55:
nit: extra blank lines not required.
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/11/src/security/tpm/tss/vendo... PS11, Line 15: TPM_BOARD_CFG
As mentioned on the mainboard file, I think it would be better to add Kconfig for the specific featu […]
Done
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/soc/intel/common/block... PS11, Line 261: static void gspi_clear_base_stash(void *unused)
The problem turned to be not the FSP-S invocation, but the change in BAR that happens because of res […]
I have created https://review.coreboot.org/c/coreboot/+/44084
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 18:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c@... PS18, Line 485: CONFIG_TPM_BOARD_CFG I think you dropped the part where this gets defined in Kconfig? Or was that already in another patch?
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c@... PS18, Line 638: padded nit: zero-terminated. zero-padded implies you're filling the whole rest of the array with zeroes.
https://review.coreboot.org/c/coreboot/+/43741/18/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/18/src/security/tpm/tss/vendo... PS18, Line 19: "Whether to request longer pulses using Cr50 BOARD_CFG register" Would probably be good to clarify that just selecting this option doesn't guarantee you're gonna get the long pulses. (Also maybe calling it CR50_REQUEST_LONG_INTERRUPT_PULSES could help make that clearer.)
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#19).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Bug: b:154333137 --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 149 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/19
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 19:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c@... PS18, Line 485: CONFIG_TPM_BOARD_CFG
I think you dropped the part where this gets defined in Kconfig? Or was that already in another patc […]
Bummer, it should have been CR50_BOARD_CFG_VALUE, which is now computed based on Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES (and in the future other separate Kconfig settings could set other bits in CR50_BOARD_CFG_VALUE.)
I am hindered by my Volteer sources running on the Google coreboot mirror, so initially I developed the patch there, and then copied it to my "pure" coreboot repository in order to send it for review. This means that it is not easy to try to compile again, after making changes in the coreboot repository.
There must be a way of cross compiling (or plain compiling) coreboot itself, not connected to a ChromeOS checkout, but I am not familiar with how to do so.
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c@... PS18, Line 638: padded
nit: zero-terminated. zero-padded implies you're filling the whole rest of the array with zeroes.
I do believe that Cr50 will truly zero-pad the sequence of bytes transmitted over SPI, even though this code does not depend on more than a single zero terminator. (It would be silly to send random bytes, and even worse to send parts of the Cr50 heap/stack.) As I have not actually verified that it is zero padded, I guess I should just stick to what this code actually requires.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 19: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c@... PS18, Line 485: CONFIG_TPM_BOARD_CFG
Bummer, it should have been CR50_BOARD_CFG_VALUE, which is now computed based on Kconfig setting CR5 […]
You can run
util/abuild/abuilt -t GOOGLE_VOLTEER -c max -x
from the upstream coreboot checkout to build a board. You may need run
make crossgcc
once to build the compiler, but then it will stick around even if you check out different commits.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 19:
(9 comments)
https://review.coreboot.org/c/coreboot/+/43741/19//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/19//COMMIT_MSG@20 PS19, Line 20: Bug: b:154333137 This needs to go before the Signed-off-by line. Also BUG=
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.h@... PS19, Line 45: get_cr50_ready_pulse_length_us Do we really need to pass a length back? I think this function can simply return a bool indicating whether cr50 supports long pulses.
bool cr50_supports_long_pulse(void);
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 451: constant value. nit: This probably fits on the previous line within the 96-column limit?
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 484: board_cfg_value, CR50_BOARD_CFG_VALUE); Should there be an early return if board_cfg_value == CR50_BOARD_CFG_VALUE?
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 628: char static?
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 645: sizeof(version_str) Shouldn't this be sizeof(version_str) - 1 to keep the logic same as before? Or was the change intentional?
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: " " not required. Add '.' instead.
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: pulses interrupt pulses.
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: " " not required
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#20).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
BUG=b:154333137
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 143 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/20
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 20:
(12 comments)
https://review.coreboot.org/c/coreboot/+/43741/19//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/19//COMMIT_MSG@20 PS19, Line 20: Bug: b:154333137
This needs to go before the Signed-off-by line. […]
Done
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.h@... PS19, Line 45: get_cr50_ready_pulse_length_us
Do we really need to pass a length back? I think this function can simply return a bool indicating w […]
You may be right that I am over-engineering, for cases where a future family/architecture requires e.g. 250us pulses, and we could have the cr50 emitting either 4us, 100us or 250us, and had in response to disable power savings to various degrees in response.
We can add such functionality when we need it, for now I have changed this method to returning bool.
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c@... PS18, Line 638: padded
I do believe that Cr50 will truly zero-pad the sequence of bytes transmitted over SPI, even though t […]
Actually, the code both before and after my change looks only at the very last of the 50 bytes read, to determine if more chunks should be read or not. That works only if it can rely on the string from cr50 being zero-padded. (Strictly speaking, if the remaning string is three or more bytes shorter than the buffer, the "middle" part of the padding need not be zero'ed, for the previous code to work, onle the first and last byte of the padding. However, I cannot imagine the cr50 code honoring that, without also filling the intervening padding with zeroes.)
I have reverted the comment to stating that the chunk will be zero padded.
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 451: constant value.
nit: This probably fits on the previous line within the 96-column limit?
Ack
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 484: board_cfg_value, CR50_BOARD_CFG_VALUE);
Should there be an early return if board_cfg_value == CR50_BOARD_CFG_VALUE?
The orignal plan was that Cr50 made the register read-only after it being written once. That has been revised such that AP jumping to RW also locks the register. Given that revision, it is safe to not explicitly set it here, if the value matches already.
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 510: if (get_cr50_board_cfg() & CR50_BOARD_CFG_100US_READY_PULSE) In a dependent CL, Furquan suggested that this method should call tlcl_lib_init(), in order to ensure that the library is initialized before calling get_cr50_board_cfg().
So far I have resisted, because I feel that this is a low level library, and that it is not appropriate for it to call the higher-level tlcl_lib. For instance the tpm2_get_info() function elsewhere in this file also does not call tlcl_lib_init(), but instead assumes that the higher level library has already been initialized, calling tpm2_init() in the process.
We could consider creating a wrapper at a higher place, which could call first tlcl_lib_init() and then this function. Furquan, do you have suggestions for an appropriate place?
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 628: char
static?
Is stack space tight? We are not using this version beyond the invocation of this method, so what is the benefit of declaring it static?
(An earlier version of this CL kept only 50 byte in memory at a time, and performed the parsing incrementally. That code was hideous.)
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 645: sizeof(version_str)
Shouldn't this be sizeof(version_str) - 1 to keep the logic same as before? Or was the change intent […]
You are right, thanks for paying attention.
https://review.coreboot.org/c/coreboot/+/43741/18/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/18/src/security/tpm/tss/vendo... PS18, Line 19: "Whether to request longer pulses using Cr50 BOARD_CFG register"
Would probably be good to clarify that just selecting this option doesn't guarantee you're gonna get […]
In time, the factory image of Cr50 will include the feature, and from that point, new designs can drop the logic around testing whether the Cr50 supports the feature. I do not want to name the setting itself to fit the transition period, but I have elaborated in the help description. Later on, we can edit the description, to not confuse future engineers with legacy concerns.
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: "
" not required. Add '.' instead.
Done
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: "
" not required
Done
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: pulses
interrupt pulses.
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 510: if (get_cr50_board_cfg() & CR50_BOARD_CFG_100US_READY_PULSE)
In a dependent CL, Furquan suggested that this method should call tlcl_lib_init(), in order to ensur […]
Honestly, long-term, I would prefer we just got rid of tlcl_lib_init() (and this whole tis_xxx() API that I think was just kinda copied from U-Boot but not enough of it to really make sense) and have a TPM stack that always automatically initializes on the first access. But for now where we still have it, I agree with Jes that it is usually called explicitly before calling low level TPM accessors so we should follow that convention.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 510: if (get_cr50_board_cfg() & CR50_BOARD_CFG_100US_READY_PULSE)
Honestly, long-term, I would prefer we just got rid of tlcl_lib_init() (and this whole tis_xxx() API […]
Is tlcl_lib_init() invoked to setup the GSPI controller? If so, setting up the GSPI controller can be done here. Since the TPM SPI device structure is local to this driver, it can be initialized once and re-used by all the subsequent use-cases.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 510: if (get_cr50_board_cfg() & CR50_BOARD_CFG_100US_READY_PULSE)
Is tlcl_lib_init() invoked to setup the GSPI controller? If so, setting up the GSPI controller can […]
I do not know how much exactly tlcl_lib_init() does, which this function relies on having been done. But I can see that e.g. tpm2_get_info() in this file does not attempt to initialize the GSPI controller, but assumes that it has already been done.
Also, tpm2_init() in this file takes some parameter, which has to come from outside this library. I do not like having some existing callers provide the argument to tpm2_init(), while having this method know how to get the parameters to call tpm2_init(). If anything, we should change this library to always "know" how to initialize itself, no matter the code path.
It sound as Julius is suggesting a refactoring like that, but I do not want this CL to be coupled to or halfway start such a refactoring.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 20:
(8 comments)
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 510: if (get_cr50_board_cfg() & CR50_BOARD_CFG_100US_READY_PULSE)
So far I have resisted, because I feel that this is a low level library, and that it is not appropriate for it to call the higher-level tlcl_lib.
Agreed. I was thinking of a wrapper in src/vendorcode/google/chromeos so that we don't add tlcl_* accesses in this driver. That way we don't really have to keep copying around the tlcl_lib_init() call in every place this function gets called. For now we can go ahead with tlcl_lib_init() in mainboard. Probably should revisit this later.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 440: (version[1] >= 5 && version[2] >= 4)) { nit: Probably fits on the previous line within the 96-column limit.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 444: " TPM_BOARD_CFG, version: %d.%d.%d\n", This can go on the previous line. printk() strings are allowed to go over the 96-column limit, so no need to split it.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 459: &board_cfg_value, sizeof(board_cfg_value))) { nit: This probably fits on the previous line within the 96-column limit?
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 484: &board_cfg_value, sizeof(board_cfg_value))) nit: Fits on previous line.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 498: &board_cfg_value, sizeof(board_cfg_value))) { nit: This probably fits on the previous line within the 96-column limit.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 622: ; = { 0 };
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 638: + 1 This is not correct. chunk_count is already incremented in line 637.
Can this loop be simplified a bit? This math looks more complicated than required.
``` char *version_ptr = version_str; char *version_end = version_ptr + sizeof(version_str) - 1;
while (version_ptr < version_end) { tpm2_read_reg(TPM_FW_VER, version_ptr, chunk_size);
if (version_ptr[chunk_size - 1] == '\0') break;
version_ptr += chunk_size; } ```
Namyoon Woo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 441: version[0]
It is something like what you describe. 0.5. […]
TPM_BOARD_CFG is supported from the version 0.{5,6}.5.
Version[0] is not used. Version[1] is major version. Even number (ex 6) is for pre-mp, and Odd number (ex 5) is for mp, a.k.a. stable in this context. Version[2] is minor version.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#21).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
BUG=b:154333137 TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 141 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/21
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 21:
(10 comments)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 441: version[0]
TPM_BOARD_CFG is supported from the version 0.{5,6}.5. […]
Thanks, I have updated the logic to only assume that the BOARD_CFG is supported on 0.5.5, and 0.6.5 and onwards. (As well as 1+.x.x, though that probably does not matter.)
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 628: char
Is stack space tight? We are not using this version beyond the invocation of this method, so what is […]
I assume this suggestion has been withdrawn.
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 645: sizeof(version_str)
You are right, thanks for paying attention.
Actually, you were not right, but I had forgotten the reason my first version was correct. What the code was doing (due chunk_count being previously incremented, and then adding one more) was to verify that the buffer has at least 50 bytes left, which has not been touched yet. On the last iteration, there will be only one byte left. This was to guard against version_str not being one more than a multiple of chunk_size.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 440: (version[1] >= 5 && version[2] >= 4)) {
nit: Probably fits on the previous line within the 96-column limit.
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 444: " TPM_BOARD_CFG, version: %d.%d.%d\n",
This can go on the previous line. […]
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 459: &board_cfg_value, sizeof(board_cfg_value))) {
nit: This probably fits on the previous line within the 96-column limit?
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 484: &board_cfg_value, sizeof(board_cfg_value)))
nit: Fits on previous line.
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 498: &board_cfg_value, sizeof(board_cfg_value))) {
nit: This probably fits on the previous line within the 96-column limit.
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 622: ;
= { 0 };
The logic does not depend on the buffer being pre-initialized to zero.
(And the explicit zero-termination after the while loop cannot be safely removed, even if we pre-fill the buffer with zeroes. In the case of version_str being re-declared to e.g. 300 bytes, or some other multiple of chunk_size, the last byte could be written to non-zero during the last iteration of the loop.)
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 638: + 1
This is not correct. chunk_count is already incremented in line 637. […]
You are right that after my previous revision, this is no longer correct. I have changed the logic somewhat, to separate and document the two exit conditions.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.c@... PS21, Line 439: if (version[0] > 0 || version[1] >= 7 || (version[1] >= 5 && version[2] >= 5)) { braces {} are not necessary for single statement blocks
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 21: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h@... PS21, Line 45: get_cr50_long_interrupt_pulses nit: Since this is returning true/false, I think we should name this something like `cr50_supports_long_interrupt_pulses()`
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 622: ;
The logic does not depend on the buffer being pre-initialized to zero. […]
We rely on version_str[**chunk_count * chunk_size - 1] to be zero when checking in line 637 below if we have to exit early i.e. before the entire 300 bytes are written. This means that the assumption here is that tpm2_read_reg will fill it with 0s. So, ideally, we can drop the explicit zero-termination after while loop if pre-initialized to 0 here. But, I think it's fine if you want to continue without this.
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.c@... PS21, Line 439: { brace can be dropped.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#22).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
BUG=b:154333137 TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 140 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/22
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.c@... PS21, Line 439: {
brace can be dropped.
Done. (My previous team had a coding policy of always using braces, not matter what. It took several years to acquire the finger memory to adhere to that. It will probably take several years again, before my fingers stop adding braces all the time.)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 631: if (version_str[++chunk_count * chunk_size - 1]) Careful, I think you flipped a while condition to a break condition here and forgot to negate? This should have an == '\0' or something at the end.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(9 comments)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3]; You do not need a global, please avoid them and parse though the call stack instead.
``` struct cr50_ver { uint8_t major; uint8_t minor; uint8_t patch; }; ```
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 434: static int cr50_supports_board_cfg(void) `static int cr50_supports_board_cfg(struct cr50_ver ver)` is the correct signature. Although these names are inconsistent with the rest of the file - there is a lot of mixing cr50 and tpm together. Consider the identifier `tpm_board_cfg_supported` instead? The signature and intent are then clearly encoded together for what the purpose is here.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50 tpm
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 487: cr50 tpm
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 515: tatic void parse_version(const char *version_str) `static int parse_version(const char *version_str, struct cr50_ver *ver)` is the correct signature.
Also move this function above the version check function above. Consider also a better name consistent with the rest of the file, `tpm_parse_fw_ver()` perhaps?
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 521: printk(BIOS_ERR, "Did not recognize Cr50 version prefix\n"); : return; return -1,2,3.. and 0 on success then print errors at call site. This way functions become more pure and easier to unit-test since we want to do more unit-testing in coreboot. Further you can use the return value to determine how the parse went wrong with just one printf.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 525: cr50_firmware_version[0] = skip_atoi(&number); : if (*number++ != '.') { : printk(BIOS_ERR, "Error parsing first dot in Cr50 version\n"); : return; : } : cr50_firmware_version[1] = skip_atoi(&number); : if (*number++ != '.') { : printk(BIOS_ERR, "Error parsing second dot in Cr50 version\n"); : return; : } : cr50_firmware_version[2] = skip_atoi(&number); parse out major.minor.patch into itermediates and only write the result into the param at the end when it is considered valid.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 638: parse_version(version_str); `struct cr50_ver cr50_fw_ver;` then pass `&cr50_fw_ver` and validate the return valid of the parse function if you can use it later.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 641: (); `set_cr50_board_cfg(cr50_fw_ver)`
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3];
You do not need a global, please avoid them and parse though the call stack instead. […]
Actually, Karthik is working on some patches that might need stashing of cr50 firmware version somewhere so that it can be queried later.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3];
Actually, Karthik is working on some patches that might need stashing of cr50 firmware version somew […]
Well that isn't happening in this patch and the control flow isn't currently good by doing this.
Why not stash it at the buttom of the call frame stack and pass by reference up? I suspect we can resolve this in the subsequent patches if a singleton really is required, however it is clearly not required here. In the absolute worst case of requiring a singleton pattern, one should access the state variable via a getter/setter functions and have a default value in the getter for uninitialised.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3];
Well that isn't happening in this patch and the control flow isn't currently good by doing this. […]
Agreed. I was just pointing out that in the changes that Karthik is working on, we are going to need some getter function to obtain cr50 firmware version which will have to be stashed in some place other than the stack. I agree that this change doesn't need it and so what you suggested makes perfect sense.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(2 comments)
I am going to address the detail-oriented comments, but first, I would like to settle the debate on whether to have a global version varialble, or not.
I believe it makes sense to gather information about the tpm chip in the tpm2_init() method, and store it in global variables, from where it can be later retrieved by getter methods, (with the contract that it is invalid to call those getters before the library has been initialized.)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3];
Agreed. […]
Passing reference to the stack frame works in one of the two places in which is it used in this CL. get_cr50_long_interrupt_pulses() also needs to know the version, and will be called "from the outside".
The tpm.h header already declares the following methods (among others):
int tpm2_init(struct spi_slave *spi_if); void tpm2_get_info(struct tpm2_info *info);
It is the contract, and clearly documented, that the getter method can only be called after the library has been initialized by tpm2_init().
I am adding one more method get_cr50_long_interrupt_pulses() with the same contract, internally it uses a singleton (global variable) to remember the version of the cr50 chip in the system from previous initialization.
As Furquan mentions, we may need to directly expose the version to the outside in yet another getter reading from the global variable.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 434: static int cr50_supports_board_cfg(void)
`static int cr50_supports_board_cfg(struct cr50_ver ver)` is the correct signature. […]
As I pointed out above, the suggested signature would mean that static uint32_t get_cr50_board_cfg() would need to call cr50_supports_board_cfg(cr50_firmware_version), referring to the global variable.
While doing that change does not have significant drawbacks, I question the need for it, assuming that we will have the global variable. (Let's resolve the global variable discussion first.)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3];
Passing reference to the stack frame works in one of the two places in which is it used in this CL. […]
I agree with Jes, this is a global for good reason and this API currently already has the assumption that the TPM has been initialized before calling any of the other functions, so adding one more with that dependency doesn't really make a difference.
Since the variable is already static I don't see a reason to have an extra getter function for use within this file. If we later want more code to query this from the outside, then yes, this should export a getter function that returns a const pointer.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50
tpm
I would actually prefer to go the other way and rename all instances of TPM_BOARD_CFG with CR50_BOARD_CFG. This is an entirely Cr50-specific thing with no relation to the TPM spec.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 521: printk(BIOS_ERR, "Did not recognize Cr50 version prefix\n"); : return;
return -1,2,3.. and 0 on success then print errors at call site. […]
I think printing here or at the caller is fine either way but in general we should always try to make the unit tests fit coreboot and not the other way round. Having a printk in a unit-tested function is no problem, we already have some of those. (In the same way, if you're worried about this function writing its result back to a static global, we have mountains of coreboot code that manipulate globals for often good reason and we shouldn't just start rewriting the data paths in half our code base to change that. There are other options to make such code testable that we should explore instead.)
Namyoon Woo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50
I would actually prefer to go the other way and rename all instances of TPM_BOARD_CFG with CR50_BOAR […]
The register name was already named as 'TPM_BOARD_CFG'. go/tpm-board-cfg. Though Julius's comment makes sense, I just named it because all other tpm registers have a prefix "TPM_" in their names.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50
The register name was already named as 'TPM_BOARD_CFG'. go/tpm-board-cfg. […]
Well, from a coreboot perspective they should still have Cr50 somewhere in the name. So either we ignore how Cr50 calls it and make our own name, or we call it CR50_TPM_BOARD_CFG or something?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 23:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.h@... PS22, Line 45: get_cr50_long_interrupt_pulses I still think that this should be named cr50_supports_long_interrupt_pulses() rather than get_cr50_...()
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50
Well, from a coreboot perspective they should still have Cr50 somewhere in the name. […]
+1. Having cr50 in the name makes it obvious that this register is not a standard register as per TPM spec. (Eventually, I think we should clean up this driver to not have cr50 specific functions/constants in here)
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 23:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3];
I agree with Jes, this is a global for good reason and this API currently already has the assumption […]
Keep in mind that when I reviewed this commit, as Furquan noted above, I only observed the current usage and it appeared to me a singleton was not necessary.
ACK on the singleton state cache for API purposes. I see there is a bit of fixation on the semantics of the API and I appricate that perspective.
However functions that can be made pure by taking it as a argument and computing some result should be kept pure and not be a closures over the global state of the singleton whenever possible. I also think it is a reasonable proposition to keep control flow bounded otherwise things start to rot into everything is a singleton where subtle bugs become an emergent phenomena.
TL;DR - functions that could just as well take the state as a argument should take the state as an argument irregardless of the singleton requirement for API purposes.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50
+1. […]
ACK, I don't know the full reasonings behind all the conventions so my view isn't strong either way - Resolved.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 487: cr50
tpm
Ack
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 521: printk(BIOS_ERR, "Did not recognize Cr50 version prefix\n"); : return;
I think printing here or at the caller is fine either way but in general we should always try to mak […]
Respectfully I disagree. Writing functions to be more pure so that they can be better testable is the correct way around not trying to deal with state in mocks that forever get more and more complex to realise. Previous code is no excuse for new code.
Specifically here, there is zero reason why this function couldn't be a pure function that is self-contained with return values that indicate the specific parse issue. That is my point, not the printf itself. This purity theme is central to my other commits in this commit.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 23:
(10 comments)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.h@... PS22, Line 45: get_cr50_long_interrupt_pulses
I still think that this should be named cr50_supports_long_interrupt_pulses() rather than get_cr50_. […]
Ack
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h@... PS21, Line 45: get_cr50_long_interrupt_pulses
nit: Since this is returning true/false, I think we should name this something like `cr50_supports_l […]
This method does not merely tell whether cr50 supports the longer pulses, but whether they are currently enabled, so I do not want the word "supports" in the name of the method. I do see that the get_ prefix may not make sense on a boolean getter, and that we want cr50_ to be the prefix of all cr50-specific functions.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3];
Keep in mind that when I reviewed this commit, as Furquan noted above, I only observed the current u […]
I have pulled the parsing, and the comparison if version is new enough to support our register, into a pure functions. These are called with reference to global variables, but could be called otherwise in tests.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 434: static int cr50_supports_board_cfg(void)
As I pointed out above, the suggested signature would mean that static uint32_t get_cr50_board_cfg() […]
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 515: tatic void parse_version(const char *version_str)
`static int parse_version(const char *version_str, struct cr50_ver *ver)` is the correct signature. […]
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 521: printk(BIOS_ERR, "Did not recognize Cr50 version prefix\n"); : return;
Respectfully I disagree. […]
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 525: cr50_firmware_version[0] = skip_atoi(&number); : if (*number++ != '.') { : printk(BIOS_ERR, "Error parsing first dot in Cr50 version\n"); : return; : } : cr50_firmware_version[1] = skip_atoi(&number); : if (*number++ != '.') { : printk(BIOS_ERR, "Error parsing second dot in Cr50 version\n"); : return; : } : cr50_firmware_version[2] = skip_atoi(&number);
parse out major.minor. […]
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 631: if (version_str[++chunk_count * chunk_size - 1])
Careful, I think you flipped a while condition to a break condition here and forgot to negate? This […]
You are right.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 638: parse_version(version_str);
`struct cr50_ver cr50_fw_ver;` then pass `&cr50_fw_ver` and validate the return valid of the parse f […]
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 641: ();
`set_cr50_board_cfg(cr50_fw_ver)`
set_cr50_board_cfg is not the only method that needs to branch on the version, and the other (get_board_cfg) does not have the option of having the version passed in as an argument, so for consistency, both of them still read the global version field.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 23:
(5 comments)
Jes - I did not see any changes in the latest patchset. Did you intend to push any? Sorry about the long back and forth. I don't mean for this change to be stuck on naming conventions. To summarize:
1. The issue that Julius pointed out with check for \0 needs to be fixed. 2. Others I think are nits w.r.t. naming of function or register. I think it would be good to have better name for the function/registers to help understand the intent/what they mean. But, as I said I don't us to get stuck on the naming forever. So, let's pick one and move ahead with it.
Can you please push a new patchset with the \0 check fixed?
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h@... PS21, Line 45: get_cr50_long_interrupt_pulses
This method does not merely tell whether cr50 supports the longer pulses, but whether they are curre […]
How about cr50_is_long_interrupt_pulse_enabled()
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50
ACK, I don't know the full reasonings behind all the conventions so my view isn't strong either way […]
Jes - do you plan to update the name of TPM_BOARD_CFG register?
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 631: if (version_str[++chunk_count * chunk_size - 1])
You are right.
This is not yet addressed even in the latest patchset.
https://review.coreboot.org/c/coreboot/+/43741/23/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/23/src/drivers/spi/tpm/tpm.c@... PS23, Line 462: "matches desired = 0x%08x\n", nit: No need to break any of the print strings. They can also go beyond the 96-column limit.
https://review.coreboot.org/c/coreboot/+/43741/23/src/drivers/spi/tpm/tpm.c@... PS23, Line 631: if (version_str[++chunk_count * chunk_size - 1]) This is not correct as Julius pointed out on a previous patchset.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#24).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
BUG=b:154333137 TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 147 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/24
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#25).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
BUG=b:154333137 TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 144 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/25
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Christian Walter, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#26).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
BUG=b:154333137 TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 144 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/26
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 26:
(5 comments)
Sorry, yesterday I failed to pay proper attention to the output of my git commands, so while I thought I had uploaded a new patchset before replying "Done" to all your comments, it turns out that I had not.
Please have a look at this one.
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/21/src/drivers/spi/tpm/tpm.h@... PS21, Line 45: get_cr50_long_interrupt_pulses
How about cr50_is_long_interrupt_pulse_enabled()
Done
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50
Jes - do you plan to update the name of TPM_BOARD_CFG register?
I have changed it into CR50_BOARD_CFG, to make it clear it is a Google extension, and not part of the TPM standard.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 631: if (version_str[++chunk_count * chunk_size - 1])
This is not yet addressed even in the latest patchset.
Done
https://review.coreboot.org/c/coreboot/+/43741/23/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/23/src/drivers/spi/tpm/tpm.c@... PS23, Line 462: "matches desired = 0x%08x\n",
nit: No need to break any of the print strings. They can also go beyond the 96-column limit.
Done
https://review.coreboot.org/c/coreboot/+/43741/23/src/drivers/spi/tpm/tpm.c@... PS23, Line 631: if (version_str[++chunk_count * chunk_size - 1])
This is not correct as Julius pointed out on a previous patchset.
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 26: Code-Review+2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 26:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/26/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/26/src/drivers/spi/tpm/tpm.c@... PS26, Line 456: /* Parsing succeeded, store complete value. */ : cr50_firmware_version = scratch; This looks like a typo here, this still overwrites the global and not the function param and the comment isn't required either. Just copy-paste this:
``` static int cr50_parse_fw_version(const char *version_str, struct cr50_firmware_version *ver) { int epoch, major, minor;
char *number = strstr(version_str, " RW_A:"); if (!number) number = strstr(version_str, " RW_B:"); if (!number) return -1; number += 6; /* Skip past the colon. */
epoch = skip_atoi(&number); if (*number++ != '.') return -2; major = skip_atoi(&number); if (*number++ != '.') return -2; minor = skip_atoi(&number);
ver->epoch = epoch; ver->major = major; ver->minor = minor;
return 0; } ```
https://review.coreboot.org/c/coreboot/+/43741/26/src/drivers/spi/tpm/tpm.c@... PS26, Line 639: if (cr50_parse_fw_version(version_str, &cr50_firmware_version)) { : printk(BIOS_ERR, "Did not recognize Cr50 version format\n"); : return -1; : } optional, you could do here, ``` int ver_valid = cr50_parse_fw_version(version_str, &cr50_firmware_version); if (ver_valid < 0) { printk(BIOS_ERR, "Did not recognize Cr50 version format with %d\n", ver_valid); return -1; } ```
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Christian Walter, Edward O'Callaghan, Jes Klinke, Julius Werner, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43741
to look at the new patch set (#27).
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
BUG=b:154333137 TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 147 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/43741/27
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 27:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/26/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/26/src/drivers/spi/tpm/tpm.c@... PS26, Line 456: /* Parsing succeeded, store complete value. */ : cr50_firmware_version = scratch;
This looks like a typo here, this still overwrites the global and not the function param and the com […]
Done
https://review.coreboot.org/c/coreboot/+/43741/26/src/drivers/spi/tpm/tpm.c@... PS26, Line 639: if (cr50_parse_fw_version(version_str, &cr50_firmware_version)) { : printk(BIOS_ERR, "Did not recognize Cr50 version format\n"); : return -1; : }
optional, you could do here, […]
I do not like such use-once variable, unless its name provides some information that is not otherwise available. In this case, the structure of the if statement, and the debug error message, makes it clear that the return must be a success/error code. Unless Coreboot has some general guideline against side effect in the conditions of if statements, I prefer to keep it the way it is.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 27: Code-Review+2
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 27:
Edward told me there is high urgency to this CL, because PUFF RO image needs the ability to extract version number of Cr50 firmware.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 27:
Patch Set 27:
Edward told me there is high urgency to this CL, because PUFF RO image needs the ability to extract version number of Cr50 firmware.
I have already +2ed this CL. This is good to land here (just waiting for some time to see if any one has any more comments). Once it lands here, it should be good to cherry-pick to chromium repo including Puff branch.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 27: Code-Review+2
Looks like we have (effectively) three +2s so that's probably good enough to merge.
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems
For Volteer (and future Tiger Lake boards) we can enable mode S0i3.4 only if we know that the Cr50 is generating 100us interrupt pulses. We have to do so, because the SoC is not guaranteed to detect pulses shorter than 100us in S0i3.4 substate.
A new Kconfig setting CR50_USE_LONG_INTERRUPT_PULSES controls new code running in verstage, which will program a new Cr50 register, provided that Cr50 firmware is new enough to support the register.
BUG=b:154333137 TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
Signed-off-by: Jes Bodi Klinke jbk@chromium.org Change-Id: If83188fd09fe69c2cda4ce1a8bf5b2efe1ca86da Reviewed-on: https://review.coreboot.org/c/coreboot/+/43741 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Julius Werner jwerner@chromium.org --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h M src/security/tpm/tss/vendor/cr50/Kconfig 3 files changed, 147 insertions(+), 25 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Julius Werner: Looks good to me, approved
diff --git a/src/drivers/spi/tpm/tpm.c b/src/drivers/spi/tpm/tpm.c index 4263dce..bc40e852 100644 --- a/src/drivers/spi/tpm/tpm.c +++ b/src/drivers/spi/tpm/tpm.c @@ -31,6 +31,15 @@ #define TPM_DID_VID_REG (TPM_LOCALITY_0_SPI_BASE + 0xf00) #define TPM_RID_REG (TPM_LOCALITY_0_SPI_BASE + 0xf04) #define TPM_FW_VER (TPM_LOCALITY_0_SPI_BASE + 0xf90) +#define CR50_BOARD_CFG (TPM_LOCALITY_0_SPI_BASE + 0xfe0) + +#define CR50_BOARD_CFG_LOCKBIT_MASK 0x80000000U +#define CR50_BOARD_CFG_FEATUREBITS_MASK 0x3FFFFFFFU + +#define CR50_BOARD_CFG_100US_READY_PULSE 0x00000001U +#define CR50_BOARD_CFG_VALUE \ + (CONFIG(CR50_USE_LONG_INTERRUPT_PULSES) \ + ? CR50_BOARD_CFG_100US_READY_PULSE : 0)
#define CR50_TIMEOUT_INIT_MS 30000 /* Very long timeout for TPM init */
@@ -39,6 +48,12 @@
/* Cached TPM device identification. */ static struct tpm2_info tpm_info; +struct cr50_firmware_version { + int epoch; + int major; + int minor; +}; +static struct cr50_firmware_version cr50_firmware_version;
/* * TODO(vbendeb): make CONFIG(DEBUG_TPM) an int to allow different level of @@ -421,12 +436,109 @@ return 0; }
+static int cr50_parse_fw_version(const char *version_str, struct cr50_firmware_version *ver) +{ + int epoch, major, minor; + + char *number = strstr(version_str, " RW_A:"); + if (!number) + number = strstr(version_str, " RW_B:"); + if (!number) + return -1; + number += 6; /* Skip past the colon. */ + + epoch = skip_atoi(&number); + if (*number++ != '.') + return -2; + major = skip_atoi(&number); + if (*number++ != '.') + return -2; + minor = skip_atoi(&number); + + ver->epoch = epoch; + ver->major = major; + ver->minor = minor; + return 0; +} + +static int cr50_fw_supports_board_cfg(struct cr50_firmware_version *version) +{ + /* Cr50 supports the CR50_BOARD_CFG register from version 0.5.5 / 0.6.5 + * and onwards. */ + if (version->epoch > 0 || version->major >= 7 + || (version->major >= 5 && version->minor >= 5)) + return 1; + printk(BIOS_INFO, "Cr50 firmware does not support CR50_BOARD_CFG, version: %d.%d.%d\n", + version->epoch, version->major, version->minor); + return 0; +} + +/** + * Set the BOARD_CFG register on the TPM chip to a particular compile-time constant value. + */ +static void cr50_set_board_cfg(void) +{ + uint32_t board_cfg_value; + if (!cr50_fw_supports_board_cfg(&cr50_firmware_version)) + return; + /* Set the CR50_BOARD_CFG register, for e.g. asking cr50 to use longer ready pulses. */ + if (!tpm2_read_reg(CR50_BOARD_CFG, &board_cfg_value, sizeof(board_cfg_value))) { + printk(BIOS_INFO, "Error reading from cr50\n"); + return; + } + if ((board_cfg_value & CR50_BOARD_CFG_FEATUREBITS_MASK) == CR50_BOARD_CFG_VALUE) { + printk(BIOS_INFO, + "Current CR50_BOARD_CFG = 0x%08x, matches desired = 0x%08x\n", + board_cfg_value, CR50_BOARD_CFG_VALUE); + return; + } + if (board_cfg_value & CR50_BOARD_CFG_LOCKBIT_MASK) { + /* The high bit is set, meaning that the Cr50 is already locked on a particular + * value for the register, but not the one we wanted. */ + printk(BIOS_ERR, + "ERROR: Current CR50_BOARD_CFG = 0x%08x, does not match desired = 0x%08x\n", + board_cfg_value, CR50_BOARD_CFG_VALUE); + return; + } + printk(BIOS_INFO, "Current CR50_BOARD_CFG = 0x%08x, setting to 0x%08x\n", + board_cfg_value, CR50_BOARD_CFG_VALUE); + board_cfg_value = CR50_BOARD_CFG_VALUE; + if (!tpm2_write_reg(CR50_BOARD_CFG, &board_cfg_value, sizeof(board_cfg_value))) + printk(BIOS_INFO, "Error writing to cr50\n"); +} + +/* + * Expose method to read the CR50_BOARD_CFG register, will return zero if + * register not supported by Cr50 firmware. + */ +static uint32_t cr50_get_board_cfg(void) +{ + uint32_t board_cfg_value; + if (!cr50_fw_supports_board_cfg(&cr50_firmware_version)) + return 0; + if (!tpm2_read_reg(CR50_BOARD_CFG, &board_cfg_value, sizeof(board_cfg_value))) { + printk(BIOS_INFO, "Error reading from cr50\n"); + return 0; + } + return board_cfg_value & CR50_BOARD_CFG_FEATUREBITS_MASK; +} + +bool cr50_is_long_interrupt_pulse_enabled(void) +{ + return cr50_get_board_cfg() & CR50_BOARD_CFG_100US_READY_PULSE; +} + /* Device/vendor ID values of the TPM devices this driver supports. */ static const uint32_t supported_did_vids[] = { 0x00281ae0, /* H1 based Cr50 security chip. */ 0x0000104a /* ST33HTPH2E32 */ };
+static int first_access_this_boot(void) +{ + return ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT); +} + int tpm2_init(struct spi_slave *spi_if) { uint32_t did_vid, status; @@ -471,7 +583,7 @@ printk(BIOS_INFO, " done!\n");
// FIXME: Move this to tpm_setup() - if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT)) + if (first_access_this_boot()) /* * Claim locality 0, do it only during the first * initialization after reset. @@ -502,18 +614,10 @@ tpm_info.vendor_id, tpm_info.device_id, tpm_info.revision);
/* Let's report device FW version if available. */ - if (tpm_info.vendor_id == 0x1ae0) { + if (CONFIG(TPM_CR50) && tpm_info.vendor_id == 0x1ae0) { int chunk_count = 0; - size_t chunk_size; - /* - * let's read 50 bytes at a time; leave room for the trailing - * zero. - */ - char vstr[51]; - - chunk_size = sizeof(vstr) - 1; - - printk(BIOS_INFO, "Firmware version: "); + size_t chunk_size = 50; + char version_str[301];
/* * Does not really matter what's written, this just makes sure @@ -521,20 +625,28 @@ */ tpm2_write_reg(TPM_FW_VER, &chunk_size, 1);
- /* Print it out in sizeof(vstr) - 1 byte chunks. */ - vstr[chunk_size] = 0; + /* + * Read chunk_size bytes at a time, last chunk will be zero padded. + */ do { - tpm2_read_reg(TPM_FW_VER, vstr, chunk_size); - printk(BIOS_INFO, "%s", vstr); - - /* - * While string is not over, and is no longer than 300 - * characters. - */ - } while (vstr[chunk_size - 1] && - (chunk_count++ < (300 / chunk_size))); - - printk(BIOS_INFO, "\n"); + tpm2_read_reg(TPM_FW_VER, + version_str + chunk_count * chunk_size, + chunk_size); + if (!version_str[++chunk_count * chunk_size - 1]) + /* Zero padding detected: end of string. */ + break; + /* Check if there is enough room for reading one more chunk. */ + } while (chunk_count * chunk_size < sizeof(version_str) - chunk_size); + version_str[chunk_count * chunk_size] = '\0'; + printk(BIOS_INFO, "Firmware version: %s\n", version_str); + if (cr50_parse_fw_version(version_str, &cr50_firmware_version)) { + printk(BIOS_ERR, "Did not recognize Cr50 version format\n"); + return -1; + } + if (CR50_BOARD_CFG_VALUE) { + if (first_access_this_boot()) + cr50_set_board_cfg(); + } } return 0; } diff --git a/src/drivers/spi/tpm/tpm.h b/src/drivers/spi/tpm/tpm.h index be98ed0..b3e3f45 100644 --- a/src/drivers/spi/tpm/tpm.h +++ b/src/drivers/spi/tpm/tpm.h @@ -41,4 +41,7 @@ /* Get information about previously initialized TPM device. */ void tpm2_get_info(struct tpm2_info *info);
+/* Indicates whether Cr50 ready pulses are guaranteed to be at least 100us. */ +bool cr50_is_long_interrupt_pulse_enabled(void); + #endif /* ! __COREBOOT_SRC_DRIVERS_SPI_TPM_TPM_H */ diff --git a/src/security/tpm/tss/vendor/cr50/Kconfig b/src/security/tpm/tss/vendor/cr50/Kconfig index f606459..52c7385 100644 --- a/src/security/tpm/tss/vendor/cr50/Kconfig +++ b/src/security/tpm/tss/vendor/cr50/Kconfig @@ -12,4 +12,11 @@ help Power off machine while waiting for CR50 update to take effect.
+config CR50_USE_LONG_INTERRUPT_PULSES + bool + default n + help + Whether to request longer interrupt pulses using Cr50 BOARD_CFG register. + If the Cr50 firmware is too old, it will not be able to honor the request. + endif
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 28:
Automatic boot test returned (PASS/FAIL/TOTAL): 6/1/7 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/15959 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/15958 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/15957 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/15956 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/15955 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/15961 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/15960
Please note: This test is under development and might not be accurate at all!
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 28:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/28/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/28/src/drivers/spi/tpm/tpm.c@... PS28, Line 443: strstr string.h doesn't seem to contain this.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 28:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43741/28/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/28/src/drivers/spi/tpm/tpm.c@... PS28, Line 443: strstr
string.h doesn't seem to contain this.
Please refer to CB:44085
-
9elements QA (Code Review) -
build bot (Jenkins) (Code Review)
-
Edward O'Callaghan (Code Review)
-
Furquan Shaikh (Code Review)
-
Jes Klinke (Code Review)
-
Julius Werner (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Namyoon Woo (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)