[PATCH v2 0/6] drivers: tpm: Fix Atmel/Microchip TPMv1.2 issues

[PATCH v2 0/6] drivers: tpm: Fix Atmel/Microchip TPMv1.2 issues

[Mathew McBride]

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

[PATCH v2 1/6] cmd: tpm-v1: fix compile error in TPMv1 list resources command

[Mathew McBride]

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);
-- 
2.30.1

[PATCH v2 2/6] cmd: tpm-v1: fix load_key_by_sha1 compile errors

[Mathew McBride]

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)
-- 
2.30.1

[PATCH v2 3/6] drivers: tpm: atmel_twi: drop non-DM_I2C compatibility

[Mathew McBride]

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) {
-- 
2.30.1

[PATCH v2 4/6] drivers: tpm: atmel_twi: do not use an offset byte

[Mathew McBride]

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;
 }
 
-- 
2.30.1

[PATCH v2 5/6] drivers: tpm: atmel_twi: implement get_desc operation

[Mathew McBride]

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);
 }
 
 /*
-- 
2.30.1

[PATCH v2 6/6] drivers: tpm: atmel_twi: fix printf specifier compile warning

[Mathew McBride]

%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
-- 
2.30.1

Re: [PATCH v2 0/6] drivers: tpm: Fix Atmel/Microchip TPMv1.2 issues

[Ilias Apalodimas]

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
>

Re: [PATCH v2 1/6] cmd: tpm-v1: fix compile error in TPMv1 list resources command

[Simon Glass]

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>

Re: [PATCH v2 2/6] cmd: tpm-v1: fix load_key_by_sha1 compile errors

[Simon Glass]

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?

Re: [PATCH v2 3/6] drivers: tpm: atmel_twi: drop non-DM_I2C compatibility

[Simon Glass]

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>

Re: [PATCH v2 4/6] drivers: tpm: atmel_twi: do not use an offset byte

[Simon Glass]

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>

Re: [PATCH v2 5/6] drivers: tpm: atmel_twi: implement get_desc operation

[Simon Glass]

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>

Re: [PATCH v2 6/6] drivers: tpm: atmel_twi: fix printf specifier compile warning

[Simon Glass]

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>

Re: [PATCH v2 4/6] drivers: tpm: atmel_twi: do not use an offset byte

[Ilias Apalodimas]

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!