[PATCH v2 0/6] drivers: tpm: Fix Atmel/Microchip TPMv1.2 issues
While doing bringup/rebase for the Ten64 I did some troubleshooting for the tpm (v1.2, NOT tpm2) command which did not appear to function, despite the Linux driver and tools (tcsd) working on the same board.
Evidently the Atmel TPM driver hasn't kept up with various step changes in the I2C and TPM stacks, and while TPMv1.2 is quite dated to TPMv2 it would be nice to make some use of the hardware that is there. (Admittedly I would love to replace our hardware TPM with an fTPM but that is a project for another day)
There are also subcommands in tpm-v1 which also have been missed in changes to the TPMv1 API and are fixed in this patchset.
Changes in v2: - Include model number in get_desc output Example: "Atmel AT97SC3204T I2C 1.2 TPM (tpm@29)"
Mathew McBride (6): cmd: tpm-v1: fix compile error in TPMv1 list resources command cmd: tpm-v1: fix load_key_by_sha1 compile errors drivers: tpm: atmel_twi: drop non-DM_I2C compatibility drivers: tpm: atmel_twi: do not use an offset byte drivers: tpm: atmel_twi: implement get_desc operation drivers: tpm: atmel_twi: fix printf specifier compile warning
cmd/tpm-v1.c | 17 +++++++++++------ drivers/tpm/tpm_atmel_twi.c | 22 +++++++--------------- lib/tpm-v1.c | 4 ++-- 3 files changed, 20 insertions(+), 23 deletions(-)
This command is not compiled by default and was not updated to pass the udevice to tpm_get_capability.
Signed-off-by: Mathew McBride matt@traverse.com.au Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- cmd/tpm-v1.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/cmd/tpm-v1.c b/cmd/tpm-v1.c index 3a7e35d525..55f2aeff46 100644 --- a/cmd/tpm-v1.c +++ b/cmd/tpm-v1.c @@ -582,6 +582,7 @@ static int do_tpm_flush(struct cmd_tbl *cmdtp, int flag, int argc, static int do_tpm_list(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { + struct udevice *dev; int type = 0; u16 res_count; u8 buf[288]; @@ -589,6 +590,10 @@ static int do_tpm_list(struct cmd_tbl *cmdtp, int flag, int argc, int err; uint i;
+ err = get_tpm(&dev); + if (err) + return err; + if (argc != 2) return CMD_RET_USAGE;
@@ -619,7 +624,7 @@ static int do_tpm_list(struct cmd_tbl *cmdtp, int flag, int argc, }
/* fetch list of already loaded resources in the TPM */ - err = tpm_get_capability(TPM_CAP_HANDLE, type, buf, + err = tpm_get_capability(dev, TPM_CAP_HANDLE, type, buf, sizeof(buf)); if (err) { printf("tpm_get_capability returned error %d.\n", err);
On Wed, 10 Nov 2021 at 21:06, Mathew McBride matt@traverse.com.au wrote:
This command is not compiled by default and was not updated to pass the udevice to tpm_get_capability.
Signed-off-by: Mathew McBride matt@traverse.com.au Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
cmd/tpm-v1.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
This command is not compiled by default and has not been updated alongside changes to the tpmv1 API, such as passing the TPM udevice to the relevant functions.
Signed-off-by: Mathew McBride matt@traverse.com.au Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- cmd/tpm-v1.c | 10 +++++----- lib/tpm-v1.c | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/cmd/tpm-v1.c b/cmd/tpm-v1.c index 55f2aeff46..bf238a9f2e 100644 --- a/cmd/tpm-v1.c +++ b/cmd/tpm-v1.c @@ -406,9 +406,9 @@ static int do_tpm_load_key_by_sha1(struct cmd_tbl *cmdtp, int flag, int argc, void *key; struct udevice *dev;
- rc = get_tpm(&dev); - if (rc) - return rc; + err = get_tpm(&dev); + if (err) + return err;
if (argc < 5) return CMD_RET_USAGE; @@ -420,7 +420,7 @@ static int do_tpm_load_key_by_sha1(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_FAILURE; parse_byte_string(argv[4], usage_auth, NULL);
- err = tpm_find_key_sha1(usage_auth, parent_hash, &parent_handle); + err = tpm1_find_key_sha1(dev, usage_auth, parent_hash, &parent_handle); if (err) { printf("Could not find matching parent key (err = %d)\n", err); return CMD_RET_FAILURE; @@ -428,7 +428,7 @@ static int do_tpm_load_key_by_sha1(struct cmd_tbl *cmdtp, int flag, int argc,
printf("Found parent key %08x\n", parent_handle);
- err = tpm_load_key2_oiap(parent_handle, key, key_len, usage_auth, + err = tpm1_load_key2_oiap(dev, parent_handle, key, key_len, usage_auth, &key_handle); if (!err) { printf("Key handle is 0x%x\n", key_handle); diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c index 8dc144080c..22a769c587 100644 --- a/lib/tpm-v1.c +++ b/lib/tpm-v1.c @@ -840,7 +840,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20], unsigned int i;
/* fetch list of already loaded keys in the TPM */ - err = tpm_get_capability(dev, TPM_CAP_HANDLE, TPM_RT_KEY, buf, + err = tpm1_get_capability(dev, TPM_CAP_HANDLE, TPM_RT_KEY, buf, sizeof(buf)); if (err) return -1; @@ -852,7 +852,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20], /* now search a(/ the) key which we can access with the given auth */ for (i = 0; i < key_count; ++i) { buf_len = sizeof(buf); - err = tpm_get_pub_key_oiap(key_handles[i], auth, buf, &buf_len); + err = tpm1_get_pub_key_oiap(dev, key_handles[i], auth, buf, &buf_len); if (err && err != TPM_AUTHFAIL) return -1; if (err)
On Wed, 10 Nov 2021 at 21:06, Mathew McBride matt@traverse.com.au wrote:
This command is not compiled by default and has not been updated alongside changes to the tpmv1 API, such as passing the TPM udevice to the relevant functions.
Signed-off-by: Mathew McBride matt@traverse.com.au Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
cmd/tpm-v1.c | 10 +++++----- lib/tpm-v1.c | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Can we enable this by default for sandbox, at least?
There are no users of this driver without DM_I2C
Signed-off-by: Mathew McBride matt@traverse.com.au Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- drivers/tpm/tpm_atmel_twi.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/tpm/tpm_atmel_twi.c b/drivers/tpm/tpm_atmel_twi.c index 2dcc2af67f..4ff4cf4cd4 100644 --- a/drivers/tpm/tpm_atmel_twi.c +++ b/drivers/tpm/tpm_atmel_twi.c @@ -81,22 +81,15 @@ static int tpm_atmel_twi_xfer(struct udevice *dev, print_buffer(0, (void *)sendbuf, 1, send_size, 0); #endif
-#if !CONFIG_IS_ENABLED(DM_I2C) - res = i2c_write(0x29, 0, 0, (uchar *)sendbuf, send_size); -#else res = dm_i2c_write(dev, 0, sendbuf, send_size); -#endif if (res) { printf("i2c_write returned %d\n", res); return -1; }
start = get_timer(0); -#if !CONFIG_IS_ENABLED(DM_I2C) - while ((res = i2c_read(0x29, 0, 0, recvbuf, 10))) -#else + while ((res = dm_i2c_read(dev, 0, recvbuf, 10))) -#endif { /* TODO Use TIS_TIMEOUT from tpm_tis_infineon.h */ if (get_timer(start) > ATMEL_TPM_TIMEOUT_MS) { @@ -116,12 +109,7 @@ static int tpm_atmel_twi_xfer(struct udevice *dev, return -1; } else { *recv_len = hdr_recv_len; -#if !CONFIG_IS_ENABLED(DM_I2C) - res = i2c_read(0x29, 0, 0, recvbuf, *recv_len); -#else res = dm_i2c_read(dev, 0, recvbuf, *recv_len); -#endif - } } if (res) {
On Wed, 10 Nov 2021 at 21:06, Mathew McBride matt@traverse.com.au wrote:
There are no users of this driver without DM_I2C
Signed-off-by: Mathew McBride matt@traverse.com.au Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
drivers/tpm/tpm_atmel_twi.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
This driver was broken due to an empty offset byte being prepended at the start of every transmission.
The hardware does not mimic an EEPROM device with registers so an offset byte is not required.
Signed-off-by: Mathew McBride matt@traverse.com.au Acked-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- drivers/tpm/tpm_atmel_twi.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/tpm/tpm_atmel_twi.c b/drivers/tpm/tpm_atmel_twi.c index 4ff4cf4cd4..71b101406d 100644 --- a/drivers/tpm/tpm_atmel_twi.c +++ b/drivers/tpm/tpm_atmel_twi.c @@ -131,6 +131,7 @@ static int tpm_atmel_twi_xfer(struct udevice *dev,
static int tpm_atmel_twi_probe(struct udevice *dev) { + i2c_set_chip_offset_len(dev, 0); return 0; }
On Wed, 10 Nov 2021 at 21:06, Mathew McBride matt@traverse.com.au wrote:
This driver was broken due to an empty offset byte being prepended at the start of every transmission.
The hardware does not mimic an EEPROM device with registers so an offset byte is not required.
Signed-off-by: Mathew McBride matt@traverse.com.au Acked-by: Ilias Apalodimas ilias.apalodimas@linaro.org
drivers/tpm/tpm_atmel_twi.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org
Hi Simon,
On Thu, 25 Nov 2021 at 02:12, Simon Glass sjg@chromium.org wrote:
On Wed, 10 Nov 2021 at 21:06, Mathew McBride matt@traverse.com.au wrote:
This driver was broken due to an empty offset byte being prepended at the start of every transmission.
The hardware does not mimic an EEPROM device with registers so an offset byte is not required.
Signed-off-by: Mathew McBride matt@traverse.com.au Acked-by: Ilias Apalodimas ilias.apalodimas@linaro.org
drivers/tpm/tpm_atmel_twi.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org
Thanks those got merged a week ago into master!
Without get_desc, the tpm command will not provide a description of the device in 'tpm device' or 'tpm info'.
Due to the characteristics of the Atmel TPM it isn't possible to determine certain attributes (e.g open/close status) without using the TPM stack (compare Infineon and ST TPM drivers), so just print out the chip model and udevice name as the identifier.
Signed-off-by: Mathew McBride matt@traverse.com.au --- drivers/tpm/tpm_atmel_twi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/tpm/tpm_atmel_twi.c b/drivers/tpm/tpm_atmel_twi.c index 71b101406d..a5dfb34846 100644 --- a/drivers/tpm/tpm_atmel_twi.c +++ b/drivers/tpm/tpm_atmel_twi.c @@ -52,7 +52,10 @@ static int tpm_atmel_twi_close(struct udevice *dev) */ static int tpm_atmel_twi_get_desc(struct udevice *dev, char *buf, int size) { - return 0; + if (size < 50) + return -ENOSPC; + + return snprintf(buf, size, "Atmel AT97SC3204T I2C 1.2 TPM (%s)", dev->name); }
/*
On Wed, 10 Nov 2021 at 21:06, Mathew McBride matt@traverse.com.au wrote:
Without get_desc, the tpm command will not provide a description of the device in 'tpm device' or 'tpm info'.
Due to the characteristics of the Atmel TPM it isn't possible to determine certain attributes (e.g open/close status) without using the TPM stack (compare Infineon and ST TPM drivers), so just print out the chip model and udevice name as the identifier.
Signed-off-by: Mathew McBride matt@traverse.com.au
drivers/tpm/tpm_atmel_twi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
%d was being used as the specifier for size_t, leading to a compiler warning
Signed-off-by: Mathew McBride matt@traverse.com.au Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- drivers/tpm/tpm_atmel_twi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tpm/tpm_atmel_twi.c b/drivers/tpm/tpm_atmel_twi.c index a5dfb34846..fa4fbec6bb 100644 --- a/drivers/tpm/tpm_atmel_twi.c +++ b/drivers/tpm/tpm_atmel_twi.c @@ -116,7 +116,7 @@ static int tpm_atmel_twi_xfer(struct udevice *dev, } } if (res) { - printf("i2c_read returned %d (rlen=%d)\n", res, *recv_len); + printf("i2c_read returned %d (rlen=%zu)\n", res, *recv_len); #ifdef DEBUG print_buffer(0, recvbuf, 1, *recv_len, 0); #endif
On Wed, 10 Nov 2021 at 21:07, Mathew McBride matt@traverse.com.au wrote:
%d was being used as the specifier for size_t, leading to a compiler warning
Signed-off-by: Mathew McBride matt@traverse.com.au Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
drivers/tpm/tpm_atmel_twi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Hi Mathew,
I've picked this up on the -tpm tree
Thanks /Ilias
On Thu, 11 Nov 2021 at 06:06, Mathew McBride matt@traverse.com.au wrote:
While doing bringup/rebase for the Ten64 I did some troubleshooting for the tpm (v1.2, NOT tpm2) command which did not appear to function, despite the Linux driver and tools (tcsd) working on the same board.
Evidently the Atmel TPM driver hasn't kept up with various step changes in the I2C and TPM stacks, and while TPMv1.2 is quite dated to TPMv2 it would be nice to make some use of the hardware that is there. (Admittedly I would love to replace our hardware TPM with an fTPM but that is a project for another day)
There are also subcommands in tpm-v1 which also have been missed in changes to the TPMv1 API and are fixed in this patchset.
Changes in v2:
- Include model number in get_desc output Example: "Atmel AT97SC3204T I2C 1.2 TPM (tpm@29)"
Mathew McBride (6): cmd: tpm-v1: fix compile error in TPMv1 list resources command cmd: tpm-v1: fix load_key_by_sha1 compile errors drivers: tpm: atmel_twi: drop non-DM_I2C compatibility drivers: tpm: atmel_twi: do not use an offset byte drivers: tpm: atmel_twi: implement get_desc operation drivers: tpm: atmel_twi: fix printf specifier compile warning
cmd/tpm-v1.c | 17 +++++++++++------ drivers/tpm/tpm_atmel_twi.c | 22 +++++++--------------- lib/tpm-v1.c | 4 ++-- 3 files changed, 20 insertions(+), 23 deletions(-)
-- 2.30.1
participants (3)
-
Ilias Apalodimas -
Mathew McBride -
Simon Glass