[PATCH 0/2] tpm: Add support for TPMv2.x I2C chips

[PATCH 0/2] tpm: Add support for TPMv2.x I2C chips

[Eddie James]

Add a tpm driver that should support any TPMv2 compliant I2C chips,
such as the NPCT75X chip. In my testing I also noticed that the timeouts
weren't set before requesting the locality so I have included a fix.

Eddie James (2):
  tpm: core: Set timeouts before requesting locality
  tpm: add support for TPMv2.x I2C chips

 drivers/tpm/Kconfig         |   9 ++
 drivers/tpm/Makefile        |   1 +
 drivers/tpm/tpm2_tis_core.c |   7 +-
 drivers/tpm/tpm2_tis_i2c.c  | 171 ++++++++++++++++++++++++++++++++++++
 4 files changed, 185 insertions(+), 3 deletions(-)
 create mode 100644 drivers/tpm/tpm2_tis_i2c.c

-- 
2.27.0

[PATCH 1/2] tpm: core: Set timeouts before requesting locality

[Eddie James]

Requesting the locality uses the timeout values, so they need
to be set beforehand.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/tpm/tpm2_tis_core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tpm/tpm2_tis_core.c b/drivers/tpm/tpm2_tis_core.c
index 51392c4584..985a816219 100644
--- a/drivers/tpm/tpm2_tis_core.c
+++ b/drivers/tpm/tpm2_tis_core.c
@@ -433,15 +433,16 @@ int tpm_tis_init(struct udevice *dev)
 		log_err("Driver bug. No bus ops defined\n");
 		return -1;
 	}
-	ret = tpm_tis_request_locality(dev, 0);
-	if (ret)
-		return ret;
 
 	chip->timeout_a = TIS_SHORT_TIMEOUT_MS;
 	chip->timeout_b = TIS_LONG_TIMEOUT_MS;
 	chip->timeout_c = TIS_SHORT_TIMEOUT_MS;
 	chip->timeout_d = TIS_SHORT_TIMEOUT_MS;
 
+	ret = tpm_tis_request_locality(dev, 0);
+	if (ret)
+		return ret;
+
 	/* Disable interrupts */
 	phy_ops->read32(dev, TPM_INT_ENABLE(chip->locality), &tmp);
 	tmp |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
-- 
2.27.0

[PATCH 2/2] tpm: add support for TPMv2.x I2C chips

[Eddie James]

Add the tpm2_tis_i2c driver that should support any TPMv2 compliant
I2C chips, such as the NPCT75X chip.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/tpm/Kconfig        |   9 ++
 drivers/tpm/Makefile       |   1 +
 drivers/tpm/tpm2_tis_i2c.c | 171 +++++++++++++++++++++++++++++++++++++
 3 files changed, 181 insertions(+)
 create mode 100644 drivers/tpm/tpm2_tis_i2c.c

diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig
index eceff27d5f..d59102d9a6 100644
--- a/drivers/tpm/Kconfig
+++ b/drivers/tpm/Kconfig
@@ -185,6 +185,15 @@ config TPM2_TIS_SPI
 	  to the device using the standard TPM Interface Specification (TIS)
 	  protocol.
 
+config TPM2_TIS_I2C
+	bool "Enable support for TPMv2.x I2C chips"
+	depends on TPM_V2 && DM_I2C
+	help
+	  This driver supports TPMv2.x devices connected on the I2C bus.
+	  The usual TPM operations and the 'tpm' command can be used to talk
+	  to the device using the standard TPM Interface Specification (TIS)
+	  protocol.
+
 config TPM2_FTPM_TEE
 	bool "TEE based fTPM Interface"
 	depends on TEE && OPTEE && TPM_V2
diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile
index 51725230c7..9540fd7fe7 100644
--- a/drivers/tpm/Makefile
+++ b/drivers/tpm/Makefile
@@ -13,5 +13,6 @@ obj-$(CONFIG_TPM_ST33ZP24_SPI) += tpm_tis_st33zp24_spi.o
 obj-$(CONFIG_$(SPL_TPL_)TPM2_CR50_I2C) += cr50_i2c.o
 obj-$(CONFIG_TPM2_TIS_SANDBOX) += tpm2_tis_sandbox.o sandbox_common.o
 obj-$(CONFIG_TPM2_TIS_SPI) += tpm2_tis_core.o tpm2_tis_spi.o
+obj-$(CONFIG_TPM2_TIS_I2C) += tpm2_tis_core.o tpm2_tis_i2c.o
 obj-$(CONFIG_TPM2_FTPM_TEE) += tpm2_ftpm_tee.o
 obj-$(CONFIG_TPM2_MMIO) += tpm2_tis_core.o tpm2_tis_mmio.o
diff --git a/drivers/tpm/tpm2_tis_i2c.c b/drivers/tpm/tpm2_tis_i2c.c
new file mode 100644
index 0000000000..33cd5bb84b
--- /dev/null
+++ b/drivers/tpm/tpm2_tis_i2c.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2022 IBM Corp.
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <fdtdec.h>
+#include <i2c.h>
+#include <log.h>
+#include <tpm-v2.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/compiler.h>
+#include <linux/types.h>
+#include <linux/unaligned/be_byteshift.h>
+#include <asm-generic/gpio.h>
+
+#include "tpm_tis.h"
+#include "tpm_internal.h"
+
+struct tpm_tis_chip_data {
+	unsigned int pcr_count;
+	unsigned int pcr_select_min;
+};
+
+static uint tpm_tis_i2c_address_to_register(u32 addr)
+{
+	addr &= 0xFFF;
+
+	/*
+	 * Adapt register addresses that have changed compared to older TIS
+	 * version.
+	 */
+	switch (addr) {
+	case TPM_ACCESS(0):
+		return 0x04;
+	case TPM_DID_VID(0):
+		return 0x48;
+	case TPM_RID(0):
+		return 0x4C;
+	default:
+		return addr;
+	}
+}
+
+static int tpm_tis_i2c_read(struct udevice *dev, u32 addr, u16 len, u8 *in)
+{
+	int rc;
+	int count = 0;
+	uint reg = tpm_tis_i2c_address_to_register(addr);
+
+	do {
+		rc = dm_i2c_read(dev, reg, in, len);
+		udelay(SLEEP_DURATION_US);
+	} while (rc && count++ < MAX_COUNT);
+
+	return rc;
+}
+
+static int tpm_tis_i2c_write(struct udevice *dev, u32 addr, u16 len,
+			     const u8 *out)
+{
+	int rc;
+	int count = 0;
+	uint reg = tpm_tis_i2c_address_to_register(addr);
+
+	do {
+		rc = dm_i2c_write(dev, reg, out, len);
+		udelay(SLEEP_DURATION_US);
+	} while (rc && count++ < MAX_COUNT);
+
+	return rc;
+}
+
+static int tpm_tis_i2c_read32(struct udevice *dev, u32 addr, u32 *result)
+{
+	__le32 result_le;
+	int rc;
+
+	rc = tpm_tis_i2c_read(dev, addr, sizeof(u32), (u8 *)&result_le);
+	if (!rc)
+		*result = le32_to_cpu(result_le);
+
+	return rc;
+}
+
+static int tpm_tis_i2c_write32(struct udevice *dev, u32 addr, u32 value)
+{
+	__le32 value_le = cpu_to_le32(value);
+
+	return tpm_tis_i2c_write(dev, addr, sizeof(value), (u8 *)&value_le);
+}
+
+static struct tpm_tis_phy_ops phy_ops = {
+	.read_bytes = tpm_tis_i2c_read,
+	.write_bytes = tpm_tis_i2c_write,
+	.read32 = tpm_tis_i2c_read32,
+	.write32 = tpm_tis_i2c_write32,
+};
+
+static int tpm_tis_i2c_probe(struct udevice *udev)
+{
+	struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
+	struct tpm_chip_priv *priv = dev_get_uclass_priv(udev);
+	int rc;
+	u8 loc = 0;
+
+	tpm_tis_ops_register(udev, &phy_ops);
+
+	/*
+	 * Force locality 0. The core driver doesn't actually write the
+	 * locality register and instead just reads/writes various access
+	 * bits of the selected locality.
+	 */
+	rc = dm_i2c_write(udev, 0, &loc, 1);
+	if (rc)
+		return rc;
+
+	rc = tpm_tis_init(udev);
+	if (rc)
+		return rc;
+
+	priv->pcr_count = drv_data->pcr_count;
+	priv->pcr_select_min = drv_data->pcr_select_min;
+	priv->version = TPM_V2;
+
+	return 0;
+}
+
+static int tpm_tis_i2c_remove(struct udevice *udev)
+{
+	return tpm_tis_cleanup(udev);
+}
+
+static const struct tpm_ops tpm_tis_i2c_ops = {
+	.open = tpm_tis_open,
+	.close = tpm_tis_close,
+	.get_desc = tpm_tis_get_desc,
+	.send = tpm_tis_send,
+	.recv = tpm_tis_recv,
+	.cleanup = tpm_tis_cleanup,
+};
+
+static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
+	.pcr_count = 24,
+	.pcr_select_min = 3,
+};
+
+static const struct udevice_id tpm_tis_i2c_ids[] = {
+	{
+		.compatible = "nuvoton,npct75x",
+		.data = (ulong)&tpm_tis_std_chip_data,
+	},
+	{
+		.compatible = "tcg,tpm-tis-i2c",
+		.data = (ulong)&tpm_tis_std_chip_data,
+	},
+	{ }
+};
+
+U_BOOT_DRIVER(tpm_tis_i2c) = {
+	.name = "tpm_tis_i2c",
+	.id = UCLASS_TPM,
+	.of_match = tpm_tis_i2c_ids,
+	.ops = &tpm_tis_i2c_ops,
+	.probe = tpm_tis_i2c_probe,
+	.remove = tpm_tis_i2c_remove,
+	.priv_auto_alloc_size = sizeof(struct tpm_chip),
+};
-- 
2.27.0

Re: [PATCH 1/2] tpm: core: Set timeouts before requesting locality

[Ilias Apalodimas]

Hi Eddie,

Thanks for the patches.   I am  currently traveling so apologies for
the slow replies.  This one looks good, I'll have a look  at the rest
once I  get back

On Fri, 13 May 2022 at 19:30, Eddie James <eajames@linux.ibm.com> wrote:
>
> Requesting the locality uses the timeout values, so they need
> to be set beforehand.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/tpm/tpm2_tis_core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tpm/tpm2_tis_core.c b/drivers/tpm/tpm2_tis_core.c
> index 51392c4584..985a816219 100644
> --- a/drivers/tpm/tpm2_tis_core.c
> +++ b/drivers/tpm/tpm2_tis_core.c
> @@ -433,15 +433,16 @@ int tpm_tis_init(struct udevice *dev)
>                 log_err("Driver bug. No bus ops defined\n");
>                 return -1;
>         }
> -       ret = tpm_tis_request_locality(dev, 0);
> -       if (ret)
> -               return ret;
>
>         chip->timeout_a = TIS_SHORT_TIMEOUT_MS;
>         chip->timeout_b = TIS_LONG_TIMEOUT_MS;
>         chip->timeout_c = TIS_SHORT_TIMEOUT_MS;
>         chip->timeout_d = TIS_SHORT_TIMEOUT_MS;
>
> +       ret = tpm_tis_request_locality(dev, 0);
> +       if (ret)
> +               return ret;
> +
>         /* Disable interrupts */
>         phy_ops->read32(dev, TPM_INT_ENABLE(chip->locality), &tmp);
>         tmp |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
> --
> 2.27.0
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 2/2] tpm: add support for TPMv2.x I2C chips

[Joel Stanley]

On Fri, 13 May 2022 at 18:30, Eddie James <eajames@linux.ibm.com> wrote:
>
> Add the tpm2_tis_i2c driver that should support any TPMv2 compliant
> I2C chips, such as the NPCT75X chip.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/tpm/Kconfig        |   9 ++
>  drivers/tpm/Makefile       |   1 +
>  drivers/tpm/tpm2_tis_i2c.c | 171 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
>  create mode 100644 drivers/tpm/tpm2_tis_i2c.c
>
> diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig
> index eceff27d5f..d59102d9a6 100644
> --- a/drivers/tpm/Kconfig
> +++ b/drivers/tpm/Kconfig
> @@ -185,6 +185,15 @@ config TPM2_TIS_SPI
>           to the device using the standard TPM Interface Specification (TIS)
>           protocol.
>
> +config TPM2_TIS_I2C
> +       bool "Enable support for TPMv2.x I2C chips"
> +       depends on TPM_V2 && DM_I2C
> +       help
> +         This driver supports TPMv2.x devices connected on the I2C bus.
> +         The usual TPM operations and the 'tpm' command can be used to talk
> +         to the device using the standard TPM Interface Specification (TIS)
> +         protocol.
> +
>  config TPM2_FTPM_TEE
>         bool "TEE based fTPM Interface"
>         depends on TEE && OPTEE && TPM_V2
> diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile
> index 51725230c7..9540fd7fe7 100644
> --- a/drivers/tpm/Makefile
> +++ b/drivers/tpm/Makefile
> @@ -13,5 +13,6 @@ obj-$(CONFIG_TPM_ST33ZP24_SPI) += tpm_tis_st33zp24_spi.o
>  obj-$(CONFIG_$(SPL_TPL_)TPM2_CR50_I2C) += cr50_i2c.o
>  obj-$(CONFIG_TPM2_TIS_SANDBOX) += tpm2_tis_sandbox.o sandbox_common.o
>  obj-$(CONFIG_TPM2_TIS_SPI) += tpm2_tis_core.o tpm2_tis_spi.o
> +obj-$(CONFIG_TPM2_TIS_I2C) += tpm2_tis_core.o tpm2_tis_i2c.o
>  obj-$(CONFIG_TPM2_FTPM_TEE) += tpm2_ftpm_tee.o
>  obj-$(CONFIG_TPM2_MMIO) += tpm2_tis_core.o tpm2_tis_mmio.o
> diff --git a/drivers/tpm/tpm2_tis_i2c.c b/drivers/tpm/tpm2_tis_i2c.c
> new file mode 100644
> index 0000000000..33cd5bb84b
> --- /dev/null
> +++ b/drivers/tpm/tpm2_tis_i2c.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2022 IBM Corp.
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <i2c.h>
> +#include <log.h>
> +#include <tpm-v2.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/compiler.h>
> +#include <linux/types.h>
> +#include <linux/unaligned/be_byteshift.h>
> +#include <asm-generic/gpio.h>
> +
> +#include "tpm_tis.h"
> +#include "tpm_internal.h"
> +
> +struct tpm_tis_chip_data {
> +       unsigned int pcr_count;
> +       unsigned int pcr_select_min;
> +};
> +
> +static uint tpm_tis_i2c_address_to_register(u32 addr)
> +{
> +       addr &= 0xFFF;
> +
> +       /*
> +        * Adapt register addresses that have changed compared to older TIS
> +        * version.
> +        */
> +       switch (addr) {
> +       case TPM_ACCESS(0):
> +               return 0x04;
> +       case TPM_DID_VID(0):
> +               return 0x48;
> +       case TPM_RID(0):
> +               return 0x4C;
> +       default:
> +               return addr;
> +       }
> +}
> +
> +static int tpm_tis_i2c_read(struct udevice *dev, u32 addr, u16 len, u8 *in)
> +{
> +       int rc;
> +       int count = 0;
> +       uint reg = tpm_tis_i2c_address_to_register(addr);
> +
> +       do {
> +               rc = dm_i2c_read(dev, reg, in, len);
> +               udelay(SLEEP_DURATION_US);
> +       } while (rc && count++ < MAX_COUNT);
> +
> +       return rc;
> +}
> +
> +static int tpm_tis_i2c_write(struct udevice *dev, u32 addr, u16 len,
> +                            const u8 *out)
> +{
> +       int rc;
> +       int count = 0;
> +       uint reg = tpm_tis_i2c_address_to_register(addr);
> +
> +       do {
> +               rc = dm_i2c_write(dev, reg, out, len);
> +               udelay(SLEEP_DURATION_US);
> +       } while (rc && count++ < MAX_COUNT);
> +
> +       return rc;
> +}
> +
> +static int tpm_tis_i2c_read32(struct udevice *dev, u32 addr, u32 *result)
> +{
> +       __le32 result_le;
> +       int rc;
> +
> +       rc = tpm_tis_i2c_read(dev, addr, sizeof(u32), (u8 *)&result_le);
> +       if (!rc)
> +               *result = le32_to_cpu(result_le);
> +
> +       return rc;
> +}
> +
> +static int tpm_tis_i2c_write32(struct udevice *dev, u32 addr, u32 value)
> +{
> +       __le32 value_le = cpu_to_le32(value);
> +
> +       return tpm_tis_i2c_write(dev, addr, sizeof(value), (u8 *)&value_le);
> +}
> +
> +static struct tpm_tis_phy_ops phy_ops = {
> +       .read_bytes = tpm_tis_i2c_read,
> +       .write_bytes = tpm_tis_i2c_write,
> +       .read32 = tpm_tis_i2c_read32,
> +       .write32 = tpm_tis_i2c_write32,
> +};
> +
> +static int tpm_tis_i2c_probe(struct udevice *udev)
> +{
> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> +       struct tpm_chip_priv *priv = dev_get_uclass_priv(udev);
> +       int rc;
> +       u8 loc = 0;
> +
> +       tpm_tis_ops_register(udev, &phy_ops);
> +
> +       /*
> +        * Force locality 0. The core driver doesn't actually write the
> +        * locality register and instead just reads/writes various access
> +        * bits of the selected locality.
> +        */
> +       rc = dm_i2c_write(udev, 0, &loc, 1);
> +       if (rc)
> +               return rc;
> +
> +       rc = tpm_tis_init(udev);
> +       if (rc)
> +               return rc;
> +
> +       priv->pcr_count = drv_data->pcr_count;
> +       priv->pcr_select_min = drv_data->pcr_select_min;
> +       priv->version = TPM_V2;
> +
> +       return 0;
> +}
> +
> +static int tpm_tis_i2c_remove(struct udevice *udev)
 > +{
> +       return tpm_tis_cleanup(udev);
> +}
> +
> +static const struct tpm_ops tpm_tis_i2c_ops = {
> +       .open = tpm_tis_open,
> +       .close = tpm_tis_close,
> +       .get_desc = tpm_tis_get_desc,
> +       .send = tpm_tis_send,
> +       .recv = tpm_tis_recv,
> +       .cleanup = tpm_tis_cleanup,
> +};
> +
> +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
> +       .pcr_count = 24,
> +       .pcr_select_min = 3,
> +};
> +
> +static const struct udevice_id tpm_tis_i2c_ids[] = {
> +       {
> +               .compatible = "nuvoton,npct75x",
> +               .data = (ulong)&tpm_tis_std_chip_data,
> +       },
> +       {
> +               .compatible = "tcg,tpm-tis-i2c",
> +               .data = (ulong)&tpm_tis_std_chip_data,

The .data pointers are the same, so could we do away with it all
together and just have

#define TPM_TIS_I2C_PCR_COUNT 24
#define TPM_TIS_I2C_PCR_SELECT_MIN 3

Aside from that small cleanup:

Reviewed-by: Joel Stanley <joel@jms.id.au>


> +       },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(tpm_tis_i2c) = {
> +       .name = "tpm_tis_i2c",
> +       .id = UCLASS_TPM,
> +       .of_match = tpm_tis_i2c_ids,
> +       .ops = &tpm_tis_i2c_ops,
> +       .probe = tpm_tis_i2c_probe,
> +       .remove = tpm_tis_i2c_remove,
> +       .priv_auto_alloc_size = sizeof(struct tpm_chip),
> +};
> --
> 2.27.0
>

Re: [PATCH 1/2] tpm: core: Set timeouts before requesting locality

[Joel Stanley]

On Fri, 13 May 2022 at 18:30, Eddie James <eajames@linux.ibm.com> wrote:
>
> Requesting the locality uses the timeout values, so they need
> to be set beforehand.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  drivers/tpm/tpm2_tis_core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tpm/tpm2_tis_core.c b/drivers/tpm/tpm2_tis_core.c
> index 51392c4584..985a816219 100644
> --- a/drivers/tpm/tpm2_tis_core.c
> +++ b/drivers/tpm/tpm2_tis_core.c
> @@ -433,15 +433,16 @@ int tpm_tis_init(struct udevice *dev)
>                 log_err("Driver bug. No bus ops defined\n");
>                 return -1;
>         }
> -       ret = tpm_tis_request_locality(dev, 0);
> -       if (ret)
> -               return ret;
>
>         chip->timeout_a = TIS_SHORT_TIMEOUT_MS;
>         chip->timeout_b = TIS_LONG_TIMEOUT_MS;
>         chip->timeout_c = TIS_SHORT_TIMEOUT_MS;
>         chip->timeout_d = TIS_SHORT_TIMEOUT_MS;
>
> +       ret = tpm_tis_request_locality(dev, 0);
> +       if (ret)
> +               return ret;
> +
>         /* Disable interrupts */
>         phy_ops->read32(dev, TPM_INT_ENABLE(chip->locality), &tmp);
>         tmp |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
> --
> 2.27.0
>

Re: [PATCH 2/2] tpm: add support for TPMv2.x I2C chips

[Ilias Apalodimas]

Hi Eddie, 

Thanks for the patch.

[...]

> +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
> +	.pcr_count = 24,
> +	.pcr_select_min = 3,
> +};
> +
> +static const struct udevice_id tpm_tis_i2c_ids[] = {
> +	{
> +		.compatible = "nuvoton,npct75x",
> +		.data = (ulong)&tpm_tis_std_chip_data,
> +	},
> +	{
> +		.compatible = "tcg,tpm-tis-i2c",
> +		.data = (ulong)&tpm_tis_std_chip_data,
> +	},
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(tpm_tis_i2c) = {
> +	.name = "tpm_tis_i2c",
> +	.id = UCLASS_TPM,
> +	.of_match = tpm_tis_i2c_ids,
> +	.ops = &tpm_tis_i2c_ops,
> +	.probe = tpm_tis_i2c_probe,
> +	.remove = tpm_tis_i2c_remove,
> +	.priv_auto_alloc_size = sizeof(struct tpm_chip),

Shouldn't this be .priv_auto only? IIRC we got rid of the
.priv_auto_alloc_size a while back?  If so I can fix this while merging

Regards
/Ilias

> +};
> -- 
> 2.27.0
> 

Re: [PATCH 2/2] tpm: add support for TPMv2.x I2C chips

[Eddie James]

On 5/23/22 01:12, Ilias Apalodimas wrote:
> Hi Eddie,
>
> Thanks for the patch.
>
> [...]
>
>> +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
>> +	.pcr_count = 24,
>> +	.pcr_select_min = 3,
>> +};
>> +
>> +static const struct udevice_id tpm_tis_i2c_ids[] = {
>> +	{
>> +		.compatible = "nuvoton,npct75x",
>> +		.data = (ulong)&tpm_tis_std_chip_data,
>> +	},
>> +	{
>> +		.compatible = "tcg,tpm-tis-i2c",
>> +		.data = (ulong)&tpm_tis_std_chip_data,
>> +	},
>> +	{ }
>> +};
>> +
>> +U_BOOT_DRIVER(tpm_tis_i2c) = {
>> +	.name = "tpm_tis_i2c",
>> +	.id = UCLASS_TPM,
>> +	.of_match = tpm_tis_i2c_ids,
>> +	.ops = &tpm_tis_i2c_ops,
>> +	.probe = tpm_tis_i2c_probe,
>> +	.remove = tpm_tis_i2c_remove,
>> +	.priv_auto_alloc_size = sizeof(struct tpm_chip),
> Shouldn't this be .priv_auto only? IIRC we got rid of the
> .priv_auto_alloc_size a while back?  If so I can fix this while merging


Yes, I think so. I tested with an older u-boot (openbmc uses a modified 
v2019.04) with all the TPM core patches, and I think I missed this bit 
in rebasing.

Thanks,

Eddie


>
> Regards
> /Ilias
>
>> +};
>> -- 
>> 2.27.0
>>

Re: [PATCH 2/2] tpm: add support for TPMv2.x I2C chips

[Ilias Apalodimas]

On Mon, May 23, 2022 at 08:19:51AM -0500, Eddie James wrote:
> 
> On 5/23/22 01:12, Ilias Apalodimas wrote:
> > Hi Eddie,
> > 
> > Thanks for the patch.
> > 
> > [...]
> > 
> > > +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
> > > +	.pcr_count = 24,
> > > +	.pcr_select_min = 3,
> > > +};
> > > +
> > > +static const struct udevice_id tpm_tis_i2c_ids[] = {
> > > +	{
> > > +		.compatible = "nuvoton,npct75x",
> > > +		.data = (ulong)&tpm_tis_std_chip_data,
> > > +	},
> > > +	{
> > > +		.compatible = "tcg,tpm-tis-i2c",
> > > +		.data = (ulong)&tpm_tis_std_chip_data,
> > > +	},
> > > +	{ }
> > > +};
> > > +
> > > +U_BOOT_DRIVER(tpm_tis_i2c) = {
> > > +	.name = "tpm_tis_i2c",
> > > +	.id = UCLASS_TPM,
> > > +	.of_match = tpm_tis_i2c_ids,
> > > +	.ops = &tpm_tis_i2c_ops,
> > > +	.probe = tpm_tis_i2c_probe,
> > > +	.remove = tpm_tis_i2c_remove,
> > > +	.priv_auto_alloc_size = sizeof(struct tpm_chip),
> > Shouldn't this be .priv_auto only? IIRC we got rid of the
> > .priv_auto_alloc_size a while back?  If so I can fix this while merging
> 
> 
> Yes, I think so. I tested with an older u-boot (openbmc uses a modified
> v2019.04) with all the TPM core patches, and I think I missed this bit in
> rebasing.

No worries, I can fix this while merging, there's no need for a v2.  I
don't see anything obviously wrong with the patchset,  unfortunately I
don't have an i2c tpm to test.  Anyway 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

P.S: Was the new TIS API useful?

Thanks
/Ilias
> 
> Thanks,
> 
> Eddie
> 
> 
> > 
> > Regards
> > /Ilias
> > 
> > > +};
> > > -- 
> > > 2.27.0
> > > 

Re: [PATCH 2/2] tpm: add support for TPMv2.x I2C chips

[Joel Stanley]

On Mon, 23 May 2022 at 13:25, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Mon, May 23, 2022 at 08:19:51AM -0500, Eddie James wrote:
> > Yes, I think so. I tested with an older u-boot (openbmc uses a modified
> > v2019.04) with all the TPM core patches, and I think I missed this bit in
> > rebasing.
>
> No worries, I can fix this while merging, there's no need for a v2.  I
> don't see anything obviously wrong with the patchset,  unfortunately I
> don't have an i2c tpm to test.  Anyway
>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> P.S: Was the new TIS API useful?

I'll reply on behalf of Eddie; he said it made it much easier to
implement! Thanks!

We've backported the patches to our v2019.04 branch and are using them there.

Cheers,

Joel

Re: [PATCH 2/2] tpm: add support for TPMv2.x I2C chips

[Ilias Apalodimas]

On Fri, 27 May 2022 at 10:26, Joel Stanley <joel@jms.id.au> wrote:
>
> On Mon, 23 May 2022 at 13:25, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Mon, May 23, 2022 at 08:19:51AM -0500, Eddie James wrote:
> > > Yes, I think so. I tested with an older u-boot (openbmc uses a modified
> > > v2019.04) with all the TPM core patches, and I think I missed this bit in
> > > rebasing.
> >
> > No worries, I can fix this while merging, there's no need for a v2.  I
> > don't see anything obviously wrong with the patchset,  unfortunately I
> > don't have an i2c tpm to test.  Anyway
> >
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
> > P.S: Was the new TIS API useful?
>
> I'll reply on behalf of Eddie; he said it made it much easier to
> implement! Thanks!
>
> We've backported the patches to our v2019.04 branch and are using them there.
>
> Cheers,

Great thanks! Patches are in -master now

Cheers
/Ilias
>
> Joel