[PATCH 1/4] tpm: Add tpm_tis_i2c backend for tpm_tis_core

[PATCH 1/4] tpm: Add tpm_tis_i2c backend for tpm_tis_core

[Johannes Holland]

Implement the TCG I2C Interface driver, as specified in the TCG PC
Client Platform TPM Profile (PTP) specification for TPM 2.0 v1.04
revision 14, section 8, I2C Interface Definition.

This driver supports Guard Times. That is, if required by the TPM, the
driver has to wait by a vendor-specific time after each I2C read/write.
The specific time is read from the TPM_I2C_INTERFACE_CAPABILITY register.

Unfortunately, the TCG specified almost but not quite compatible
register addresses. Therefore, the TIS register addresses need to be
mapped to I2C ones. The locality is stripped because for now, only
locality 0 is supported.

Add a sanity check to I2C reads of e.g. TPM_ACCESS and TPM_STS. This is
to detect communication errors and issues due to non-standard behaviour
(E.g. the clock stretching quirk in the BCM2835, see 4dbfb5f4401f). In
case the sanity check fails, attempt a retry.

The CRC over the FIFO register is not implemented here since a new call
has to be added to the API (tpm_tis_phy_ops).

Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
---
 drivers/char/tpm/Kconfig       |  12 ++
 drivers/char/tpm/Makefile      |   1 +
 drivers/char/tpm/tpm_tis_i2c.c | 365 +++++++++++++++++++++++++++++++++
 3 files changed, 378 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_tis_i2c.c

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 4a5516406c22..927088b2c3d3 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -74,6 +74,18 @@ config TCG_TIS_SPI_CR50
 	  If you have a H1 secure module running Cr50 firmware on SPI bus,
 	  say Yes and it will be accessible from within Linux.
 
+config TCG_TIS_I2C
+	tristate "TPM Interface Specification 1.3 Interface / TPM 2.0 FIFO Interface - (I2C - generic)"
+	depends on I2C
+	select CRC_CCITT
+	select TCG_TIS_CORE
+	help
+	  If you have a TPM security chip, compliant with the TCG TPM PTP
+	  (I2C interface) specification and connected to an I2C bus master,
+	  say Yes and it will be accessible from within Linux.
+	  To compile this driver as a module, choose M here;
+	  the module will be called tpm_tis_i2c.
+
 config TCG_TIS_SYNQUACER
 	tristate "TPM Interface Specification 1.2 Interface / TPM 2.0 FIFO Interface (MMIO - SynQuacer)"
 	depends on ARCH_SYNQUACER || COMPILE_TEST
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 66d39ea6bd10..0222b1ddb310 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -29,6 +29,7 @@ tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
 
 obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
 
+obj-$(CONFIG_TCG_TIS_I2C) += tpm_tis_i2c.o
 obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
 obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
 obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
new file mode 100644
index 000000000000..206406c97325
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -0,0 +1,365 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2014-2021 Nuvoton Technology corporation
+ * Copyright (C) 2019-2022 Infineon Technologies AG
+ *
+ * Authors:
+ * Alexander Steffen <alexander.steffen@infineon.com>
+ * Amir Mizinski <amirmizi6@gmail.com>
+ * Johannes Holland <johannes.holland@infineon.com>
+ *
+ * This device driver implements the TPM interface as defined in the TCG PC
+ * Client Platform TPM Profile (PTP) Specification for TPM 2.0 v1.04
+ * Revision 14.
+ *
+ * It is based on the tpm_tis_spi device driver.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/wait.h>
+#include <linux/acpi.h>
+#include <linux/freezer.h>
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/tpm.h>
+#include "tpm_tis_core.h"
+
+/* TPM registers */
+#define TPM_I2C_LOC_SEL 0x00
+#define TPM_I2C_ACCESS 0x04
+#define TPM_I2C_INTERFACE_CAPABILITY 0x30
+#define TPM_I2C_DEVICE_ADDRESS 0x38
+#define TPM_I2C_DATA_CSUM_ENABLE 0x40
+#define TPM_I2C_DID_VID 0x48
+#define TPM_I2C_RID 0x4C
+
+/* TIS-compatible register address to avoid clash with TPM_ACCESS (0x00) */
+#define TPM_LOC_SEL 0x0FFF
+
+/* Mask to extract the I2C register from TIS register addresses */
+#define TPM_TIS_REGISTER_MASK 0x0FFF
+
+/*
+ * Guard Time:
+ * After each I2C operation, the TPM might require the master to wait.
+ * The time period is vendor-specific and must be read from the
+ * TPM_I2C_INTERFACE_CAPABILITY register.
+ *
+ * Before the Guard Time is read (or after the TPM failed to send an I2C NACK),
+ * a Guard Time of 250µs applies.
+ *
+ * Various flags in the same register indicate if a guard time is needed:
+ *  - SR: <I2C read with repeated start> <guard time> <I2C read>
+ *  - RR: <I2C read> <guard time> <I2C read>
+ *  - RW: <I2C read> <guard time> <I2C write>
+ *  - WR: <I2C write> <guard time> <I2C read>
+ *  - WW: <I2C write> <guard time> <I2C write>
+ *
+ * See TCG PC Client PTP Specification v1.04, 8.1.10 GUARD_TIME
+ */
+
+/* Default Guard Time until interface capability register is read */
+#define GUARD_TIME_DEFAULT_MIN 250
+#define GUARD_TIME_DEFAULT_MAX 300
+
+/* Guard Time after I2C slave NACK */
+#define GUARD_TIME_ERR_MIN 250
+#define GUARD_TIME_ERR_MAX 300
+
+/* Guard Time bit masks; SR is repeated start, RW is read then write, etc. */
+#define TPM_GUARD_TIME_SR_MASK 0x40000000
+#define TPM_GUARD_TIME_RR_MASK 0x00100000
+#define TPM_GUARD_TIME_RW_MASK 0x00080000
+#define TPM_GUARD_TIME_WR_MASK 0x00040000
+#define TPM_GUARD_TIME_WW_MASK 0x00020000
+#define TPM_GUARD_TIME_MIN_MASK 0x0001FE00
+#define TPM_GUARD_TIME_MIN_SHIFT 9
+
+/* Masks with bits that must be read zero */
+#define TPM_ACCESS_READ_ZERO 0x48
+#define TPM_INT_ENABLE_ZERO 0x7FFFFF6
+#define TPM_STS_READ_ZERO 0x23
+#define TPM_INTF_CAPABILITY_ZERO 0x0FFFF000
+#define TPM_I2C_INTERFACE_CAPABILITY_ZERO 0x80000000
+
+struct tpm_tis_i2c_phy {
+	struct tpm_tis_data priv;
+	struct i2c_client *i2c_client;
+	bool guard_time_read;
+	bool guard_time_write;
+	u16 guard_time_min;
+	u16 guard_time_max;
+	u8 *io_buf;
+};
+
+static inline struct tpm_tis_i2c_phy *
+to_tpm_tis_i2c_phy(struct tpm_tis_data *data)
+{
+	return container_of(data, struct tpm_tis_i2c_phy, priv);
+}
+
+static u8 address_to_register(u32 addr)
+{
+	addr &= TPM_TIS_REGISTER_MASK;
+
+	switch (addr) {
+	case TPM_ACCESS(0):
+		return TPM_I2C_ACCESS;
+	case TPM_LOC_SEL:
+		return TPM_I2C_LOC_SEL;
+	case TPM_DID_VID(0):
+		return TPM_I2C_DID_VID;
+	case TPM_RID(0):
+		return TPM_I2C_RID;
+	default:
+		return addr;
+	}
+}
+
+static int retry_i2c_transfer_until_ack(struct tpm_tis_data *data,
+					struct i2c_msg *msg)
+{
+	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	bool guard_time;
+	int i = 0;
+	int ret;
+
+	if (msg->flags & I2C_M_RD)
+		guard_time = phy->guard_time_read;
+	else
+		guard_time = phy->guard_time_write;
+
+	do {
+		ret = i2c_transfer(phy->i2c_client->adapter, msg, 1);
+		if (ret < 0)
+			usleep_range(GUARD_TIME_ERR_MIN, GUARD_TIME_ERR_MAX);
+		else if (guard_time)
+			usleep_range(phy->guard_time_min, phy->guard_time_max);
+		/* retry on TPM NACK */
+	} while (ret < 0 && i++ < TPM_RETRY);
+
+	return ret;
+}
+
+/* Check that bits which must be read zero are not set */
+static int sanity_check_read(u8 reg, u16 len, u8 *buf)
+{
+	u32 value;
+	u32 zero_mask;
+
+	switch (len) {
+	case sizeof(u8):
+		value = buf[0];
+		break;
+	case sizeof(u16):
+		value = le16_to_cpup((__le16 *)buf);
+		break;
+	case sizeof(u32):
+		value = le32_to_cpup((__le32 *)buf);
+		break;
+	default:
+		return 0;
+	}
+
+	switch (reg) {
+	case TPM_I2C_ACCESS:
+		zero_mask = TPM_ACCESS_READ_ZERO;
+		break;
+	case TPM_INT_ENABLE(0) & TPM_TIS_REGISTER_MASK:
+		zero_mask = TPM_INT_ENABLE_ZERO;
+		break;
+	case TPM_STS(1) & TPM_TIS_REGISTER_MASK:
+		zero_mask = TPM_STS_READ_ZERO;
+		break;
+	case TPM_INTF_CAPS(0) & TPM_TIS_REGISTER_MASK:
+		zero_mask = TPM_INTF_CAPABILITY_ZERO;
+		break;
+	case TPM_I2C_INTERFACE_CAPABILITY:
+		zero_mask = TPM_I2C_INTERFACE_CAPABILITY_ZERO;
+		break;
+	default:
+		return 0;
+	}
+
+	if (unlikely((value & zero_mask) != 0x00)) {
+		pr_debug("TPM I2C read of register 0x%02x failed sanity check: 0x%x\n", reg, value);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
+				  u8 *result, enum tpm_tis_io_mode io_mode)
+{
+	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	struct i2c_msg msg = { .addr = phy->i2c_client->addr };
+	u8 reg = address_to_register(addr);
+	int i = 0;
+	int ret;
+
+	do {
+		/* write register */
+		msg.len = sizeof(reg);
+		msg.buf = &reg;
+		msg.flags = 0;
+		retry_i2c_transfer_until_ack(data, &msg);
+		if (ret < 0)
+			return ret;
+
+		/* read data */
+		msg.buf = result;
+		msg.len = len;
+		msg.flags = I2C_M_RD;
+		retry_i2c_transfer_until_ack(data, &msg);
+		if (ret < 0)
+			return ret;
+
+		ret = sanity_check_read(reg, len, result);
+		if (ret == 0)
+			return 0;
+
+		usleep_range(GUARD_TIME_ERR_MIN, GUARD_TIME_ERR_MAX);
+	} while (i++ < TPM_RETRY);
+
+	return ret;
+}
+
+static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
+				   const u8 *value,
+				   enum tpm_tis_io_mode io_mode)
+{
+	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	struct i2c_msg msg = { .addr = phy->i2c_client->addr };
+	u8 reg = address_to_register(addr);
+	int ret = 0;
+
+	if (len > TPM_BUFSIZE - 1)
+		return -EIO;
+
+	/* write register and data in one go */
+	phy->io_buf[0] = reg;
+	memcpy(phy->io_buf + sizeof(reg), value, len);
+
+	msg.len = sizeof(reg) + len;
+	msg.buf = phy->io_buf;
+	retry_i2c_transfer_until_ack(data, &msg);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int init_guard_time(struct tpm_tis_i2c_phy *phy)
+{
+	u32 i2c_caps;
+	int ret;
+
+	phy->guard_time_read = true;
+	phy->guard_time_write = true;
+	phy->guard_time_min = GUARD_TIME_DEFAULT_MIN;
+	phy->guard_time_max = GUARD_TIME_DEFAULT_MAX;
+
+	ret = tpm_tis_i2c_read_bytes(&phy->priv, TPM_I2C_INTERFACE_CAPABILITY,
+				     sizeof(i2c_caps), (u8 *)&i2c_caps,
+				     TPM_TIS_PHYS_32);
+	if (ret)
+		return ret;
+
+	phy->guard_time_read = (i2c_caps & TPM_GUARD_TIME_RR_MASK) ||
+			       (i2c_caps & TPM_GUARD_TIME_RW_MASK);
+	phy->guard_time_write = (i2c_caps & TPM_GUARD_TIME_WR_MASK) ||
+				(i2c_caps & TPM_GUARD_TIME_WW_MASK);
+	phy->guard_time_min = (i2c_caps & TPM_GUARD_TIME_MIN_MASK) >>
+			      TPM_GUARD_TIME_MIN_SHIFT;
+	/* guard_time_max = guard_time_min * 1.2 */
+	phy->guard_time_max = phy->guard_time_min + phy->guard_time_min / 5;
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
+
+static const struct tpm_tis_phy_ops tpm_i2c_phy_ops = {
+	.read_bytes = tpm_tis_i2c_read_bytes,
+	.write_bytes = tpm_tis_i2c_write_bytes,
+};
+
+static int tpm_tis_i2c_probe(struct i2c_client *dev,
+			     const struct i2c_device_id *id)
+{
+	struct tpm_tis_i2c_phy *phy;
+	const u8 locality = 0;
+	int ret;
+
+	phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_i2c_phy),
+			   GFP_KERNEL);
+	if (!phy)
+		return -ENOMEM;
+
+	phy->io_buf = devm_kzalloc(&dev->dev, TPM_BUFSIZE, GFP_KERNEL);
+	if (!phy->io_buf)
+		return -ENOMEM;
+
+	phy->i2c_client = dev;
+
+	/* must precede all communication with the tpm */
+	ret = init_guard_time(phy);
+	if (ret)
+		return ret;
+
+	ret = tpm_tis_i2c_write_bytes(&phy->priv, TPM_LOC_SEL, sizeof(locality),
+				      &locality, TPM_TIS_PHYS_8);
+	if (ret)
+		return ret;
+
+	return tpm_tis_core_init(&dev->dev, &phy->priv, -1, &tpm_i2c_phy_ops,
+				 NULL);
+}
+
+static int tpm_tis_i2c_remove(struct i2c_client *client)
+{
+	struct tpm_chip *chip = i2c_get_clientdata(client);
+
+	tpm_chip_unregister(chip);
+	tpm_tis_remove(chip);
+	return 0;
+}
+
+static const struct i2c_device_id tpm_tis_i2c_id[] = {
+	{ "tpm_tis_i2c", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
+
+static const struct of_device_id of_tis_i2c_match[] = {
+	{ .compatible = "infineon,slb9673", },
+	{ .compatible = "nuvoton,npct75x", },
+	{ .compatible = "tcg,tpm-tis-i2c", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
+
+static struct i2c_driver tpm_tis_i2c_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "tpm_tis_i2c",
+		.pm = &tpm_tis_pm,
+		.of_match_table = of_match_ptr(of_tis_i2c_match),
+	},
+	.probe = tpm_tis_i2c_probe,
+	.remove = tpm_tis_i2c_remove,
+	.id_table = tpm_tis_i2c_id,
+};
+module_i2c_driver(tpm_tis_i2c_driver);
+
+MODULE_DESCRIPTION("TPM Driver for native I2C access");
+MODULE_LICENSE("GPL");
-- 
2.31.1.windows.1

[PATCH 2/4] tpm: Add tpm_tis_verify_crc to the tpm_tis_phy_ops protocol layer

[Johannes Holland]

Other than SPI TPMs, I2C TPMs implement a CRC feature for command and
response blobs. Add CRC validation to the TIS protocol according to the
TCG PC Client Platform TPM Profile (PTP) specification for TPM 2.0 v1.04
revision 14

The CRC is calculated over the entirety of the FIFO register. Since the
phy_ops layer is not aware when the core layer is done reading/writing
the FIFO, CRC verification must be triggered from the core layer. To
this end, add an optional phy_ops API call.

Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.c | 14 ++++++++++++++
 drivers/char/tpm/tpm_tis_core.h | 10 ++++++++++
 drivers/char/tpm/tpm_tis_i2c.c  | 34 ++++++++++++++++++++++++++++++---
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index dc56b976d816..f1c893a5a38f 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -289,6 +289,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	int size = 0;
 	int status;
 	u32 expected;
+	int rc;
 
 	if (count < TPM_HEADER_SIZE) {
 		size = -EIO;
@@ -328,6 +329,13 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 		goto out;
 	}
 
+	rc = tpm_tis_verify_crc(priv, (size_t)size, buf);
+	if (rc < 0) {
+		dev_err(&chip->dev, "Error crc mismatch for response.\n");
+		size = rc;
+		goto out;
+	}
+
 out:
 	tpm_tis_ready(chip);
 	return size;
@@ -443,6 +451,12 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
 	if (rc < 0)
 		return rc;
 
+	rc = tpm_tis_verify_crc(priv, len, buf);
+	if (rc < 0) {
+		dev_err(&chip->dev, "Error crc mismatch for command.\n");
+		return rc;
+	}
+
 	/* go and do it */
 	rc = tpm_tis_write8(priv, TPM_STS(priv->locality), TPM_STS_GO);
 	if (rc < 0)
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index e2b8e6de25b4..21b4a67c9aac 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -116,6 +116,8 @@ struct tpm_tis_phy_ops {
 			  u8 *result, enum tpm_tis_io_mode mode);
 	int (*write_bytes)(struct tpm_tis_data *data, u32 addr, u16 len,
 			   const u8 *value, enum tpm_tis_io_mode mode);
+	int (*verify_crc)(struct tpm_tis_data *data, size_t len,
+			  const u8 *value);
 };
 
 static inline int tpm_tis_read_bytes(struct tpm_tis_data *data, u32 addr,
@@ -183,6 +185,14 @@ static inline int tpm_tis_write32(struct tpm_tis_data *data, u32 addr,
 	return rc;
 }
 
+static inline int tpm_tis_verify_crc(struct tpm_tis_data *data, size_t len,
+				     const u8 *value)
+{
+	if (!data->phy_ops->verify_crc)
+		return 0;
+	return data->phy_ops->verify_crc(data, len, value);
+}
+
 static inline bool is_bsw(void)
 {
 #ifdef CONFIG_X86
diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
index 206406c97325..0608bb6c7b90 100644
--- a/drivers/char/tpm/tpm_tis_i2c.c
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -29,6 +29,7 @@
 #include <linux/gpio.h>
 #include <linux/of_irq.h>
 #include <linux/of_gpio.h>
+#include <linux/crc-ccitt.h>
 #include <linux/tpm.h>
 #include "tpm_tis_core.h"
 
@@ -38,6 +39,7 @@
 #define TPM_I2C_INTERFACE_CAPABILITY 0x30
 #define TPM_I2C_DEVICE_ADDRESS 0x38
 #define TPM_I2C_DATA_CSUM_ENABLE 0x40
+#define TPM_DATA_CSUM 0x44
 #define TPM_I2C_DID_VID 0x48
 #define TPM_I2C_RID 0x4C
 
@@ -211,7 +213,7 @@ static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
 		msg.len = sizeof(reg);
 		msg.buf = &reg;
 		msg.flags = 0;
-		retry_i2c_transfer_until_ack(data, &msg);
+		ret = retry_i2c_transfer_until_ack(data, &msg);
 		if (ret < 0)
 			return ret;
 
@@ -219,7 +221,7 @@ static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
 		msg.buf = result;
 		msg.len = len;
 		msg.flags = I2C_M_RD;
-		retry_i2c_transfer_until_ack(data, &msg);
+		ret = retry_i2c_transfer_until_ack(data, &msg);
 		if (ret < 0)
 			return ret;
 
@@ -251,13 +253,31 @@ static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
 
 	msg.len = sizeof(reg) + len;
 	msg.buf = phy->io_buf;
-	retry_i2c_transfer_until_ack(data, &msg);
+	ret = retry_i2c_transfer_until_ack(data, &msg);
 	if (ret < 0)
 		return ret;
 
 	return 0;
 }
 
+static int tpm_tis_i2c_verify_crc(struct tpm_tis_data *data, size_t len,
+				  const u8 *value)
+{
+	u16 crc_tpm, crc_host;
+	int rc;
+
+	rc = tpm_tis_read16(data, TPM_DATA_CSUM, &crc_tpm);
+	if (rc < 0)
+		return rc;
+
+	/* reflect crc result, regardless of host endianness */
+	crc_host = swab16(crc_ccitt(0, value, len));
+	if (crc_tpm != crc_host)
+		return -EIO;
+
+	return 0;
+}
+
 static int init_guard_time(struct tpm_tis_i2c_phy *phy)
 {
 	u32 i2c_caps;
@@ -291,12 +311,14 @@ static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
 static const struct tpm_tis_phy_ops tpm_i2c_phy_ops = {
 	.read_bytes = tpm_tis_i2c_read_bytes,
 	.write_bytes = tpm_tis_i2c_write_bytes,
+	.verify_crc = tpm_tis_i2c_verify_crc,
 };
 
 static int tpm_tis_i2c_probe(struct i2c_client *dev,
 			     const struct i2c_device_id *id)
 {
 	struct tpm_tis_i2c_phy *phy;
+	const u8 crc_enable = 1;
 	const u8 locality = 0;
 	int ret;
 
@@ -321,6 +343,12 @@ static int tpm_tis_i2c_probe(struct i2c_client *dev,
 	if (ret)
 		return ret;
 
+	ret = tpm_tis_i2c_write_bytes(&phy->priv, TPM_I2C_DATA_CSUM_ENABLE,
+				      sizeof(crc_enable), &crc_enable,
+				      TPM_TIS_PHYS_8);
+	if (ret)
+		return ret;
+
 	return tpm_tis_core_init(&dev->dev, &phy->priv, -1, &tpm_i2c_phy_ops,
 				 NULL);
 }
-- 
2.31.1.windows.1

[PATCH 3/4] tpm: Implement command and response retry in tpm_tis_core

[Johannes Holland]

Some errors during command transmission and response reception are
recoverable. Implement the specified retry mechanisms.

Recoverable errors during response reception:
 * invalid response size during header read
 * left over data:
   a communication error can lead to a FIFO read of 0xFFs and an
   unexpected STS.dataAvail = 1, subsequently
 * CRC mismatch

Recoverable errors during transmit:
 * CRC mismatch

Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.c | 98 +++++++++++++++++++--------------
 drivers/char/tpm/tpm_tis_core.h |  1 +
 2 files changed, 57 insertions(+), 42 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index f1c893a5a38f..a2b6fba7f719 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -287,6 +287,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int size = 0;
+	int i = 0;
 	int status;
 	u32 expected;
 	int rc;
@@ -296,45 +297,52 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 		goto out;
 	}
 
-	size = recv_data(chip, buf, TPM_HEADER_SIZE);
-	/* read first 10 bytes, including tag, paramsize, and result */
-	if (size < TPM_HEADER_SIZE) {
-		dev_err(&chip->dev, "Unable to read header\n");
-		goto out;
-	}
+	do {
+		if (size < 0)
+			tpm_tis_write8(priv, TPM_STS(priv->locality),
+				       TPM_STS_RESPONSE_RETRY);
+
+		size = recv_data(chip, buf, TPM_HEADER_SIZE);
+		/* read first 10 bytes, including tag, paramsize, and result */
+		if (size < TPM_HEADER_SIZE) {
+			dev_err(&chip->dev, "Unable to read header\n");
+			goto out;
+		}
 
-	expected = be32_to_cpu(*(__be32 *) (buf + 2));
-	if (expected > count || expected < TPM_HEADER_SIZE) {
-		size = -EIO;
-		goto out;
-	}
+		expected = be32_to_cpu(*(__be32 *)(buf + 2));
+		if (expected > count || expected < TPM_HEADER_SIZE) {
+			dev_info(&chip->dev, "Bad response size: %d. Retry...\n", expected);
+			size = -EIO;
+			continue;
+		}
 
-	size += recv_data(chip, &buf[TPM_HEADER_SIZE],
-			  expected - TPM_HEADER_SIZE);
-	if (size < expected) {
-		dev_err(&chip->dev, "Unable to read remainder of result\n");
-		size = -ETIME;
-		goto out;
-	}
+		size += recv_data(chip, &buf[TPM_HEADER_SIZE],
+				expected - TPM_HEADER_SIZE);
+		if (size < expected) {
+			dev_err(&chip->dev, "Unable to read remainder of result\n");
+			size = -ETIME;
+			goto out;
+		}
 
-	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-				&priv->int_queue, false) < 0) {
-		size = -ETIME;
-		goto out;
-	}
-	status = tpm_tis_status(chip);
-	if (status & TPM_STS_DATA_AVAIL) {	/* retry? */
-		dev_err(&chip->dev, "Error left over data\n");
-		size = -EIO;
-		goto out;
-	}
+		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
+				      &priv->int_queue, false) < 0) {
+			size = -ETIME;
+			goto out;
+		}
+		status = tpm_tis_status(chip);
+		if (status & TPM_STS_DATA_AVAIL) {
+			dev_info(&chip->dev, "Error left over data. Retry...\n");
+			size = -EIO;
+			continue;
+		}
 
-	rc = tpm_tis_verify_crc(priv, (size_t)size, buf);
-	if (rc < 0) {
-		dev_err(&chip->dev, "Error crc mismatch for response.\n");
-		size = rc;
-		goto out;
-	}
+		rc = tpm_tis_verify_crc(priv, (size_t)size, buf);
+		if (rc < 0) {
+			dev_info(&chip->dev, "Error crc mismatch for response. Retry...\n");
+			size = rc;
+			continue;
+		}
+	} while (unlikely(size < 0) && i++ < TPM_RETRY);
 
 out:
 	tpm_tis_ready(chip);
@@ -444,18 +452,24 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc;
+	int i = 0;
 	u32 ordinal;
 	unsigned long dur;
 
-	rc = tpm_tis_send_data(chip, buf, len);
-	if (rc < 0)
-		return rc;
+	do {
+		rc = tpm_tis_send_data(chip, buf, len);
+		if (rc < 0)
+			return rc;
 
-	rc = tpm_tis_verify_crc(priv, len, buf);
-	if (rc < 0) {
-		dev_err(&chip->dev, "Error crc mismatch for command.\n");
+		rc = tpm_tis_verify_crc(priv, len, buf);
+		if (rc < 0) {
+			dev_info(&chip->dev, "Error crc mismatch for command. Retry...\n");
+			tpm_tis_ready(chip);
+		}
+	} while (unlikely(rc < 0) && i++ < TPM_RETRY);
+
+	if (rc < 0)
 		return rc;
-	}
 
 	/* go and do it */
 	rc = tpm_tis_write8(priv, TPM_STS(priv->locality), TPM_STS_GO);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 21b4a67c9aac..b4b36d5c5514 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -34,6 +34,7 @@ enum tis_status {
 	TPM_STS_GO = 0x20,
 	TPM_STS_DATA_AVAIL = 0x10,
 	TPM_STS_DATA_EXPECT = 0x08,
+	TPM_STS_RESPONSE_RETRY = 0x02,
 	TPM_STS_READ_ZERO = 0x23, /* bits that must be zero on read */
 };
 
-- 
2.31.1.windows.1

[PATCH 4/4] tpm: Add YAML schema for the TPM TIS I2C options

[Johannes Holland]

Add a YAML schema to support device tree bindings for the generic I2C
physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
Specification for TPM 2.0 v1.04 Revision 14.

Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
---
 .../bindings/security/tpm/tpm-tis-i2c.yaml    | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml

diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
new file mode 100644
index 000000000000..7948867ff3f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C PTP based TPM Device Tree Bindings
+
+maintainers:
+  - Johannes Holland <johannes.holland@infineon.com>
+
+description:
+  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
+
+properties:
+  compatible:
+    items:
+      - enum:
+          # Infineon's Trusted Platform Module (TPM) (SLB9673)
+          - infineon,slb9673
+          # Nuvoton's Trusted Platform Module (TPM) (NPCT75x)
+          - nuvoton,npct75x
+      - const: tcg,tpm-tis-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupt:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      tpm@2e {
+        compatible = "infineon,slb9673", "nuvoton,npct75x", "tcg,tpm-tis-i2c";
+        reg = <0x2e>;
+      };
+    };
+...
-- 
2.31.1.windows.1

Re: [PATCH 4/4] tpm: Add YAML schema for the TPM TIS I2C options

[Rob Herring]

On Mon, 04 Apr 2022 10:18:35 +0200, Johannes Holland wrote:
> Add a YAML schema to support device tree bindings for the generic I2C
> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
> Specification for TPM 2.0 v1.04 Revision 14.
> 
> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> ---
>  .../bindings/security/tpm/tpm-tis-i2c.yaml    | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.example.dt.yaml: tpm@2e: compatible:1: 'tcg,tpm-tis-i2c' was expected
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.example.dt.yaml: tpm@2e: compatible: ['infineon,slb9673', 'nuvoton,npct75x', 'tcg,tpm-tis-i2c'] is too long
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 4/4] tpm: Add YAML schema for the TPM TIS I2C options

[Rob Herring]

On Mon, Apr 04, 2022 at 10:18:35AM +0200, Johannes Holland wrote:
> Add a YAML schema to support device tree bindings for the generic I2C
> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
> Specification for TPM 2.0 v1.04 Revision 14.

Bindings are for devices. A protocol layer does not make a device.

> 
> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> ---
>  .../bindings/security/tpm/tpm-tis-i2c.yaml    | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml

We already have a binding for I2C TPM. That one should be converted.

> 
> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> new file mode 100644
> index 000000000000..7948867ff3f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C PTP based TPM Device Tree Bindings
> +
> +maintainers:
> +  - Johannes Holland <johannes.holland@infineon.com>
> +
> +description:
> +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          # Infineon's Trusted Platform Module (TPM) (SLB9673)
> +          - infineon,slb9673
> +          # Nuvoton's Trusted Platform Module (TPM) (NPCT75x)
> +          - nuvoton,npct75x

I see this is already used, but in general wildcards should not be used 
in device compatibles.

> +      - const: tcg,tpm-tis-i2c

Pretty sure I killed this off when originally reviewing the TPM I2C 
binding.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      tpm@2e {
> +        compatible = "infineon,slb9673", "nuvoton,npct75x", "tcg,tpm-tis-i2c";
> +        reg = <0x2e>;
> +      };
> +    };
> +...
> -- 
> 2.31.1.windows.1
> 
> 

Re: [PATCH 1/4] tpm: Add tpm_tis_i2c backend for tpm_tis_core

[Jarkko Sakkinen]

On Mon, Apr 04, 2022 at 10:18:32AM +0200, Johannes Holland wrote:
> Implement the TCG I2C Interface driver, as specified in the TCG PC
> Client Platform TPM Profile (PTP) specification for TPM 2.0 v1.04
> revision 14, section 8, I2C Interface Definition.
> 
> This driver supports Guard Times. That is, if required by the TPM, the
> driver has to wait by a vendor-specific time after each I2C read/write.
> The specific time is read from the TPM_I2C_INTERFACE_CAPABILITY register.
> 
> Unfortunately, the TCG specified almost but not quite compatible
> register addresses. Therefore, the TIS register addresses need to be
> mapped to I2C ones. The locality is stripped because for now, only
> locality 0 is supported.
> 
> Add a sanity check to I2C reads of e.g. TPM_ACCESS and TPM_STS. This is
> to detect communication errors and issues due to non-standard behaviour
> (E.g. the clock stretching quirk in the BCM2835, see 4dbfb5f4401f). In
> case the sanity check fails, attempt a retry.
> 
> The CRC over the FIFO register is not implemented here since a new call
> has to be added to the API (tpm_tis_phy_ops).
> 
> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> ---
>  drivers/char/tpm/Kconfig       |  12 ++
>  drivers/char/tpm/Makefile      |   1 +
>  drivers/char/tpm/tpm_tis_i2c.c | 365 +++++++++++++++++++++++++++++++++
>  3 files changed, 378 insertions(+)
>  create mode 100644 drivers/char/tpm/tpm_tis_i2c.c
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 4a5516406c22..927088b2c3d3 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -74,6 +74,18 @@ config TCG_TIS_SPI_CR50
>  	  If you have a H1 secure module running Cr50 firmware on SPI bus,
>  	  say Yes and it will be accessible from within Linux.
>  
> +config TCG_TIS_I2C
> +	tristate "TPM Interface Specification 1.3 Interface / TPM 2.0 FIFO Interface - (I2C - generic)"
> +	depends on I2C
> +	select CRC_CCITT
> +	select TCG_TIS_CORE
> +	help
> +	  If you have a TPM security chip, compliant with the TCG TPM PTP
> +	  (I2C interface) specification and connected to an I2C bus master,
> +	  say Yes and it will be accessible from within Linux.
> +	  To compile this driver as a module, choose M here;
> +	  the module will be called tpm_tis_i2c.
> +
>  config TCG_TIS_SYNQUACER
>  	tristate "TPM Interface Specification 1.2 Interface / TPM 2.0 FIFO Interface (MMIO - SynQuacer)"
>  	depends on ARCH_SYNQUACER || COMPILE_TEST
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 66d39ea6bd10..0222b1ddb310 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -29,6 +29,7 @@ tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
>  
>  obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
>  
> +obj-$(CONFIG_TCG_TIS_I2C) += tpm_tis_i2c.o
>  obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
>  obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
>  obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
> new file mode 100644
> index 000000000000..206406c97325
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_tis_i2c.c
> @@ -0,0 +1,365 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2014-2021 Nuvoton Technology corporation
> + * Copyright (C) 2019-2022 Infineon Technologies AG
> + *
> + * Authors:
> + * Alexander Steffen <alexander.steffen@infineon.com>
> + * Amir Mizinski <amirmizi6@gmail.com>
> + * Johannes Holland <johannes.holland@infineon.com>

Please, use co-developed-by instead in the commit message:

https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html

> + *
> + * This device driver implements the TPM interface as defined in the TCG PC
> + * Client Platform TPM Profile (PTP) Specification for TPM 2.0 v1.04
> + * Revision 14.
> + *
> + * It is based on the tpm_tis_spi device driver.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/wait.h>
> +#include <linux/acpi.h>
> +#include <linux/freezer.h>
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_gpio.h>
> +#include <linux/tpm.h>
> +#include "tpm_tis_core.h"
> +
> +/* TPM registers */
> +#define TPM_I2C_LOC_SEL 0x00
> +#define TPM_I2C_ACCESS 0x04
> +#define TPM_I2C_INTERFACE_CAPABILITY 0x30
> +#define TPM_I2C_DEVICE_ADDRESS 0x38
> +#define TPM_I2C_DATA_CSUM_ENABLE 0x40
> +#define TPM_I2C_DID_VID 0x48
> +#define TPM_I2C_RID 0x4C
> +
> +/* TIS-compatible register address to avoid clash with TPM_ACCESS (0x00) */
> +#define TPM_LOC_SEL 0x0FFF
> +
> +/* Mask to extract the I2C register from TIS register addresses */
> +#define TPM_TIS_REGISTER_MASK 0x0FFF
> +
> +/*
> + * Guard Time:
> + * After each I2C operation, the TPM might require the master to wait.
> + * The time period is vendor-specific and must be read from the
> + * TPM_I2C_INTERFACE_CAPABILITY register.
> + *
> + * Before the Guard Time is read (or after the TPM failed to send an I2C NACK),
> + * a Guard Time of 250µs applies.
> + *
> + * Various flags in the same register indicate if a guard time is needed:
> + *  - SR: <I2C read with repeated start> <guard time> <I2C read>
> + *  - RR: <I2C read> <guard time> <I2C read>
> + *  - RW: <I2C read> <guard time> <I2C write>
> + *  - WR: <I2C write> <guard time> <I2C read>
> + *  - WW: <I2C write> <guard time> <I2C write>
> + *
> + * See TCG PC Client PTP Specification v1.04, 8.1.10 GUARD_TIME
> + */
> +
> +/* Default Guard Time until interface capability register is read */
> +#define GUARD_TIME_DEFAULT_MIN 250
> +#define GUARD_TIME_DEFAULT_MAX 300
> +
> +/* Guard Time after I2C slave NACK */
> +#define GUARD_TIME_ERR_MIN 250
> +#define GUARD_TIME_ERR_MAX 300
> +
> +/* Guard Time bit masks; SR is repeated start, RW is read then write, etc. */
> +#define TPM_GUARD_TIME_SR_MASK 0x40000000
> +#define TPM_GUARD_TIME_RR_MASK 0x00100000
> +#define TPM_GUARD_TIME_RW_MASK 0x00080000
> +#define TPM_GUARD_TIME_WR_MASK 0x00040000
> +#define TPM_GUARD_TIME_WW_MASK 0x00020000
> +#define TPM_GUARD_TIME_MIN_MASK 0x0001FE00
> +#define TPM_GUARD_TIME_MIN_SHIFT 9
> +
> +/* Masks with bits that must be read zero */
> +#define TPM_ACCESS_READ_ZERO 0x48
> +#define TPM_INT_ENABLE_ZERO 0x7FFFFF6
> +#define TPM_STS_READ_ZERO 0x23
> +#define TPM_INTF_CAPABILITY_ZERO 0x0FFFF000
> +#define TPM_I2C_INTERFACE_CAPABILITY_ZERO 0x80000000
> +
> +struct tpm_tis_i2c_phy {
> +	struct tpm_tis_data priv;
> +	struct i2c_client *i2c_client;
> +	bool guard_time_read;
> +	bool guard_time_write;
> +	u16 guard_time_min;
> +	u16 guard_time_max;
> +	u8 *io_buf;
> +};
> +
> +static inline struct tpm_tis_i2c_phy *
> +to_tpm_tis_i2c_phy(struct tpm_tis_data *data)
> +{
> +	return container_of(data, struct tpm_tis_i2c_phy, priv);
> +}
> +
> +static u8 address_to_register(u32 addr)
> +{
> +	addr &= TPM_TIS_REGISTER_MASK;
> +
> +	switch (addr) {
> +	case TPM_ACCESS(0):
> +		return TPM_I2C_ACCESS;
> +	case TPM_LOC_SEL:
> +		return TPM_I2C_LOC_SEL;
> +	case TPM_DID_VID(0):
> +		return TPM_I2C_DID_VID;
> +	case TPM_RID(0):
> +		return TPM_I2C_RID;
> +	default:
> +		return addr;
> +	}
> +}
> +
> +static int retry_i2c_transfer_until_ack(struct tpm_tis_data *data,
> +					struct i2c_msg *msg)
> +{
> +	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
> +	bool guard_time;
> +	int i = 0;
> +	int ret;
> +
> +	if (msg->flags & I2C_M_RD)
> +		guard_time = phy->guard_time_read;
> +	else
> +		guard_time = phy->guard_time_write;
> +
> +	do {
> +		ret = i2c_transfer(phy->i2c_client->adapter, msg, 1);
> +		if (ret < 0)
> +			usleep_range(GUARD_TIME_ERR_MIN, GUARD_TIME_ERR_MAX);
> +		else if (guard_time)
> +			usleep_range(phy->guard_time_min, phy->guard_time_max);
> +		/* retry on TPM NACK */
> +	} while (ret < 0 && i++ < TPM_RETRY);
> +
> +	return ret;
> +}
> +
> +/* Check that bits which must be read zero are not set */
> +static int sanity_check_read(u8 reg, u16 len, u8 *buf)
> +{
> +	u32 value;
> +	u32 zero_mask;
> +
> +	switch (len) {
> +	case sizeof(u8):
> +		value = buf[0];
> +		break;
> +	case sizeof(u16):
> +		value = le16_to_cpup((__le16 *)buf);
> +		break;
> +	case sizeof(u32):
> +		value = le32_to_cpup((__le32 *)buf);
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	switch (reg) {
> +	case TPM_I2C_ACCESS:
> +		zero_mask = TPM_ACCESS_READ_ZERO;
> +		break;
> +	case TPM_INT_ENABLE(0) & TPM_TIS_REGISTER_MASK:
> +		zero_mask = TPM_INT_ENABLE_ZERO;
> +		break;
> +	case TPM_STS(1) & TPM_TIS_REGISTER_MASK:
> +		zero_mask = TPM_STS_READ_ZERO;
> +		break;
> +	case TPM_INTF_CAPS(0) & TPM_TIS_REGISTER_MASK:
> +		zero_mask = TPM_INTF_CAPABILITY_ZERO;
> +		break;
> +	case TPM_I2C_INTERFACE_CAPABILITY:
> +		zero_mask = TPM_I2C_INTERFACE_CAPABILITY_ZERO;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	if (unlikely((value & zero_mask) != 0x00)) {
> +		pr_debug("TPM I2C read of register 0x%02x failed sanity check: 0x%x\n", reg, value);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
> +				  u8 *result, enum tpm_tis_io_mode io_mode)
> +{
> +	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
> +	struct i2c_msg msg = { .addr = phy->i2c_client->addr };
> +	u8 reg = address_to_register(addr);
> +	int i = 0;
> +	int ret;
> +
> +	do {
> +		/* write register */
> +		msg.len = sizeof(reg);
> +		msg.buf = &reg;
> +		msg.flags = 0;
> +		retry_i2c_transfer_until_ack(data, &msg);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* read data */
> +		msg.buf = result;
> +		msg.len = len;
> +		msg.flags = I2C_M_RD;
> +		retry_i2c_transfer_until_ack(data, &msg);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = sanity_check_read(reg, len, result);
> +		if (ret == 0)
> +			return 0;
> +
> +		usleep_range(GUARD_TIME_ERR_MIN, GUARD_TIME_ERR_MAX);
> +	} while (i++ < TPM_RETRY);
> +
> +	return ret;
> +}
> +
> +static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
> +				   const u8 *value,
> +				   enum tpm_tis_io_mode io_mode)
> +{
> +	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
> +	struct i2c_msg msg = { .addr = phy->i2c_client->addr };
> +	u8 reg = address_to_register(addr);
> +	int ret = 0;
> +
> +	if (len > TPM_BUFSIZE - 1)
> +		return -EIO;
> +
> +	/* write register and data in one go */
> +	phy->io_buf[0] = reg;
> +	memcpy(phy->io_buf + sizeof(reg), value, len);
> +
> +	msg.len = sizeof(reg) + len;
> +	msg.buf = phy->io_buf;
> +	retry_i2c_transfer_until_ack(data, &msg);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int init_guard_time(struct tpm_tis_i2c_phy *phy)
> +{
> +	u32 i2c_caps;
> +	int ret;
> +
> +	phy->guard_time_read = true;
> +	phy->guard_time_write = true;
> +	phy->guard_time_min = GUARD_TIME_DEFAULT_MIN;
> +	phy->guard_time_max = GUARD_TIME_DEFAULT_MAX;
> +
> +	ret = tpm_tis_i2c_read_bytes(&phy->priv, TPM_I2C_INTERFACE_CAPABILITY,
> +				     sizeof(i2c_caps), (u8 *)&i2c_caps,
> +				     TPM_TIS_PHYS_32);
> +	if (ret)
> +		return ret;
> +
> +	phy->guard_time_read = (i2c_caps & TPM_GUARD_TIME_RR_MASK) ||
> +			       (i2c_caps & TPM_GUARD_TIME_RW_MASK);
> +	phy->guard_time_write = (i2c_caps & TPM_GUARD_TIME_WR_MASK) ||
> +				(i2c_caps & TPM_GUARD_TIME_WW_MASK);
> +	phy->guard_time_min = (i2c_caps & TPM_GUARD_TIME_MIN_MASK) >>
> +			      TPM_GUARD_TIME_MIN_SHIFT;
> +	/* guard_time_max = guard_time_min * 1.2 */
> +	phy->guard_time_max = phy->guard_time_min + phy->guard_time_min / 5;
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
> +
> +static const struct tpm_tis_phy_ops tpm_i2c_phy_ops = {
> +	.read_bytes = tpm_tis_i2c_read_bytes,
> +	.write_bytes = tpm_tis_i2c_write_bytes,
> +};
> +
> +static int tpm_tis_i2c_probe(struct i2c_client *dev,
> +			     const struct i2c_device_id *id)
> +{
> +	struct tpm_tis_i2c_phy *phy;
> +	const u8 locality = 0;
> +	int ret;
> +
> +	phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_i2c_phy),
> +			   GFP_KERNEL);
> +	if (!phy)
> +		return -ENOMEM;
> +
> +	phy->io_buf = devm_kzalloc(&dev->dev, TPM_BUFSIZE, GFP_KERNEL);
> +	if (!phy->io_buf)
> +		return -ENOMEM;
> +
> +	phy->i2c_client = dev;
> +
> +	/* must precede all communication with the tpm */
> +	ret = init_guard_time(phy);
> +	if (ret)
> +		return ret;
> +
> +	ret = tpm_tis_i2c_write_bytes(&phy->priv, TPM_LOC_SEL, sizeof(locality),
> +				      &locality, TPM_TIS_PHYS_8);
> +	if (ret)
> +		return ret;
> +
> +	return tpm_tis_core_init(&dev->dev, &phy->priv, -1, &tpm_i2c_phy_ops,
> +				 NULL);
> +}
> +
> +static int tpm_tis_i2c_remove(struct i2c_client *client)
> +{
> +	struct tpm_chip *chip = i2c_get_clientdata(client);
> +
> +	tpm_chip_unregister(chip);
> +	tpm_tis_remove(chip);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id tpm_tis_i2c_id[] = {
> +	{ "tpm_tis_i2c", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
> +
> +static const struct of_device_id of_tis_i2c_match[] = {
> +	{ .compatible = "infineon,slb9673", },
> +	{ .compatible = "nuvoton,npct75x", },
> +	{ .compatible = "tcg,tpm-tis-i2c", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
> +
> +static struct i2c_driver tpm_tis_i2c_driver = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "tpm_tis_i2c",
> +		.pm = &tpm_tis_pm,
> +		.of_match_table = of_match_ptr(of_tis_i2c_match),
> +	},
> +	.probe = tpm_tis_i2c_probe,
> +	.remove = tpm_tis_i2c_remove,
> +	.id_table = tpm_tis_i2c_id,
> +};
> +module_i2c_driver(tpm_tis_i2c_driver);
> +
> +MODULE_DESCRIPTION("TPM Driver for native I2C access");
> +MODULE_LICENSE("GPL");
> -- 
> 2.31.1.windows.1
> 

Quality looks quite solid. Have you tried:

- TPM2 kselftest
- rmmod + modprobe (sanity check)

BR, Jarkko

Re: [PATCH 4/4] tpm: Add YAML schema for the TPM TIS I2C options

[Johannes Holland]

On 04.04.2022 18:18, Rob Herring wrote:
> On Mon, Apr 04, 2022 at 10:18:35AM +0200, Johannes Holland wrote:
>> Add a YAML schema to support device tree bindings for the generic I2C
>> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
>> Specification for TPM 2.0 v1.04 Revision 14.
>
> Bindings are for devices. A protocol layer does not make a device.

Agreed. I will change this in my next patch.

>
>>
>> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
>> ---
>>  .../bindings/security/tpm/tpm-tis-i2c.yaml    | 48 +++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>
> We already have a binding for I2C TPM. That one should be converted.

Will do. There are two required properties which are in fact not needed
by any I2C driver. If that is ok with you, I would like to turn them
optional.

- linux,sml-base
- linux,sml-size

>>>
>> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>> new file mode 100644
>> index 000000000000..7948867ff3f7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>> @@ -0,0 +1,48 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: I2C PTP based TPM Device Tree Bindings
>> +
>> +maintainers:
>> +  - Johannes Holland <johannes.holland@infineon.com>
>> +
>> +description:
>> +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          # Infineon's Trusted Platform Module (TPM) (SLB9673)
>> +          - infineon,slb9673
>> +          # Nuvoton's Trusted Platform Module (TPM) (NPCT75x)
>> +          - nuvoton,npct75x
>
> I see this is already used, but in general wildcards should not be used
> in device compatibles.

Ok, I took this over from a previous patch. Since I am not familiar with
Nuvoton products, so I am going to remove this for now.

>
>> +      - const: tcg,tpm-tis-i2c
>
> Pretty sure I killed this off when originally reviewing the TPM I2C
> binding.
>

Sorry, I did not see any discussion related to this.

IMHO, the TPM is a open standard device. That should allow for plug
and play, regardless of the manufacturer. For SPI, we also have
tcg,tpm_tis-spi. However, if you want it removed, I can do that.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupt:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      tpm@2e {
>> +        compatible = "infineon,slb9673", "nuvoton,npct75x", "tcg,tpm-tis-i2c";
>> +        reg = <0x2e>;
>> +      };
>> +    };
>> +...
>> --
>> 2.31.1.windows.1
>>
>>

Re: [PATCH 4/4] tpm: Add YAML schema for the TPM TIS I2C options

[Johannes Holland]

On 04.04.2022 18:08, Rob Herring wrote:> On Mon, 04 Apr 2022 10:18:35 +0200, Johannes Holland wrote:
>> Add a YAML schema to support device tree bindings for the generic I2C
>> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
>> Specification for TPM 2.0 v1.04 Revision 14.
>>
>> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
>> ---
>>  .../bindings/security/tpm/tpm-tis-i2c.yaml    | 48 +++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.example.dt.yaml: tpm@2e: compatible:1: 'tcg,tpm-tis-i2c' was expected
>         From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.example.dt.yaml: tpm@2e: compatible: ['infineon,slb9673', 'nuvoton,npct75x', 'tcg,tpm-tis-i2c'] is too long
>         From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/patch/
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.
> 

Sorry for the inconvenience. This will be fixed in the next patch.

Thanks,
Johannes

Re: [PATCH 1/4] tpm: Add tpm_tis_i2c backend for tpm_tis_core

[Johannes Holland]

On 07.04.2022 12:07, Jarkko Sakkinen wrote:
> On Mon, Apr 04, 2022 at 10:18:32AM +0200, Johannes Holland wrote:
>> Implement the TCG I2C Interface driver, as specified in the TCG PC
>> Client Platform TPM Profile (PTP) specification for TPM 2.0 v1.04
>> revision 14, section 8, I2C Interface Definition.
>>
>> This driver supports Guard Times. That is, if required by the TPM, the
>> driver has to wait by a vendor-specific time after each I2C read/write.
>> The specific time is read from the TPM_I2C_INTERFACE_CAPABILITY register.
>>
>> Unfortunately, the TCG specified almost but not quite compatible
>> register addresses. Therefore, the TIS register addresses need to be
>> mapped to I2C ones. The locality is stripped because for now, only
>> locality 0 is supported.
>>
>> Add a sanity check to I2C reads of e.g. TPM_ACCESS and TPM_STS. This is
>> to detect communication errors and issues due to non-standard behaviour
>> (E.g. the clock stretching quirk in the BCM2835, see 4dbfb5f4401f). In
>> case the sanity check fails, attempt a retry.
>>
>> The CRC over the FIFO register is not implemented here since a new call
>> has to be added to the API (tpm_tis_phy_ops).
>>
>> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
>> ---
>>  drivers/char/tpm/Kconfig       |  12 ++
>>  drivers/char/tpm/Makefile      |   1 +
>>  drivers/char/tpm/tpm_tis_i2c.c | 365 +++++++++++++++++++++++++++++++++
>>  3 files changed, 378 insertions(+)
>>  create mode 100644 drivers/char/tpm/tpm_tis_i2c.c
>>
>> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
>> index 4a5516406c22..927088b2c3d3 100644
>> --- a/drivers/char/tpm/Kconfig
>> +++ b/drivers/char/tpm/Kconfig
>> @@ -74,6 +74,18 @@ config TCG_TIS_SPI_CR50
>>         If you have a H1 secure module running Cr50 firmware on SPI bus,
>>         say Yes and it will be accessible from within Linux.
>>
>> +config TCG_TIS_I2C
>> +     tristate "TPM Interface Specification 1.3 Interface / TPM 2.0 FIFO Interface - (I2C - generic)"
>> +     depends on I2C
>> +     select CRC_CCITT
>> +     select TCG_TIS_CORE
>> +     help
>> +       If you have a TPM security chip, compliant with the TCG TPM PTP
>> +       (I2C interface) specification and connected to an I2C bus master,
>> +       say Yes and it will be accessible from within Linux.
>> +       To compile this driver as a module, choose M here;
>> +       the module will be called tpm_tis_i2c.
>> +
>>  config TCG_TIS_SYNQUACER
>>       tristate "TPM Interface Specification 1.2 Interface / TPM 2.0 FIFO Interface (MMIO - SynQuacer)"
>>       depends on ARCH_SYNQUACER || COMPILE_TEST
>> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
>> index 66d39ea6bd10..0222b1ddb310 100644
>> --- a/drivers/char/tpm/Makefile
>> +++ b/drivers/char/tpm/Makefile
>> @@ -29,6 +29,7 @@ tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
>>
>>  obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
>>
>> +obj-$(CONFIG_TCG_TIS_I2C) += tpm_tis_i2c.o
>>  obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
>>  obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
>>  obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
>> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
>> new file mode 100644
>> index 000000000000..206406c97325
>> --- /dev/null
>> +++ b/drivers/char/tpm/tpm_tis_i2c.c
>> @@ -0,0 +1,365 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2014-2021 Nuvoton Technology corporation
>> + * Copyright (C) 2019-2022 Infineon Technologies AG
>> + *
>> + * Authors:
>> + * Alexander Steffen <alexander.steffen@infineon.com>
>> + * Amir Mizinski <amirmizi6@gmail.com>
>> + * Johannes Holland <johannes.holland@infineon.com>
> 
> Please, use co-developed-by instead in the commit message:
> 
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html

Will do.

> 
>> + *
>> + * This device driver implements the TPM interface as defined in the TCG PC
>> + * Client Platform TPM Profile (PTP) Specification for TPM 2.0 v1.04
>> + * Revision 14.
>> + *
>> + * It is based on the tpm_tis_spi device driver.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/wait.h>
>> +#include <linux/acpi.h>
>> +#include <linux/freezer.h>
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/tpm.h>
>> +#include "tpm_tis_core.h"
>> +
>> +/* TPM registers */
>> +#define TPM_I2C_LOC_SEL 0x00
>> +#define TPM_I2C_ACCESS 0x04
>> +#define TPM_I2C_INTERFACE_CAPABILITY 0x30
>> +#define TPM_I2C_DEVICE_ADDRESS 0x38
>> +#define TPM_I2C_DATA_CSUM_ENABLE 0x40
>> +#define TPM_I2C_DID_VID 0x48
>> +#define TPM_I2C_RID 0x4C
>> +
>> +/* TIS-compatible register address to avoid clash with TPM_ACCESS (0x00) */
>> +#define TPM_LOC_SEL 0x0FFF
>> +
>> +/* Mask to extract the I2C register from TIS register addresses */
>> +#define TPM_TIS_REGISTER_MASK 0x0FFF
>> +
>> +/*
>> + * Guard Time:
>> + * After each I2C operation, the TPM might require the master to wait.
>> + * The time period is vendor-specific and must be read from the
>> + * TPM_I2C_INTERFACE_CAPABILITY register.
>> + *
>> + * Before the Guard Time is read (or after the TPM failed to send an I2C NACK),
>> + * a Guard Time of 250µs applies.
>> + *
>> + * Various flags in the same register indicate if a guard time is needed:
>> + *  - SR: <I2C read with repeated start> <guard time> <I2C read>
>> + *  - RR: <I2C read> <guard time> <I2C read>
>> + *  - RW: <I2C read> <guard time> <I2C write>
>> + *  - WR: <I2C write> <guard time> <I2C read>
>> + *  - WW: <I2C write> <guard time> <I2C write>
>> + *
>> + * See TCG PC Client PTP Specification v1.04, 8.1.10 GUARD_TIME
>> + */
>> +
>> +/* Default Guard Time until interface capability register is read */
>> +#define GUARD_TIME_DEFAULT_MIN 250
>> +#define GUARD_TIME_DEFAULT_MAX 300
>> +
>> +/* Guard Time after I2C slave NACK */
>> +#define GUARD_TIME_ERR_MIN 250
>> +#define GUARD_TIME_ERR_MAX 300
>> +
>> +/* Guard Time bit masks; SR is repeated start, RW is read then write, etc. */
>> +#define TPM_GUARD_TIME_SR_MASK 0x40000000
>> +#define TPM_GUARD_TIME_RR_MASK 0x00100000
>> +#define TPM_GUARD_TIME_RW_MASK 0x00080000
>> +#define TPM_GUARD_TIME_WR_MASK 0x00040000
>> +#define TPM_GUARD_TIME_WW_MASK 0x00020000
>> +#define TPM_GUARD_TIME_MIN_MASK 0x0001FE00
>> +#define TPM_GUARD_TIME_MIN_SHIFT 9
>> +
>> +/* Masks with bits that must be read zero */
>> +#define TPM_ACCESS_READ_ZERO 0x48
>> +#define TPM_INT_ENABLE_ZERO 0x7FFFFF6
>> +#define TPM_STS_READ_ZERO 0x23
>> +#define TPM_INTF_CAPABILITY_ZERO 0x0FFFF000
>> +#define TPM_I2C_INTERFACE_CAPABILITY_ZERO 0x80000000
>> +
>> +struct tpm_tis_i2c_phy {
>> +     struct tpm_tis_data priv;
>> +     struct i2c_client *i2c_client;
>> +     bool guard_time_read;
>> +     bool guard_time_write;
>> +     u16 guard_time_min;
>> +     u16 guard_time_max;
>> +     u8 *io_buf;
>> +};
>> +
>> +static inline struct tpm_tis_i2c_phy *
>> +to_tpm_tis_i2c_phy(struct tpm_tis_data *data)
>> +{
>> +     return container_of(data, struct tpm_tis_i2c_phy, priv);
>> +}
>> +
>> +static u8 address_to_register(u32 addr)
>> +{
>> +     addr &= TPM_TIS_REGISTER_MASK;
>> +
>> +     switch (addr) {
>> +     case TPM_ACCESS(0):
>> +             return TPM_I2C_ACCESS;
>> +     case TPM_LOC_SEL:
>> +             return TPM_I2C_LOC_SEL;
>> +     case TPM_DID_VID(0):
>> +             return TPM_I2C_DID_VID;
>> +     case TPM_RID(0):
>> +             return TPM_I2C_RID;
>> +     default:
>> +             return addr;
>> +     }
>> +}
>> +
>> +static int retry_i2c_transfer_until_ack(struct tpm_tis_data *data,
>> +                                     struct i2c_msg *msg)
>> +{
>> +     struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
>> +     bool guard_time;
>> +     int i = 0;
>> +     int ret;
>> +
>> +     if (msg->flags & I2C_M_RD)
>> +             guard_time = phy->guard_time_read;
>> +     else
>> +             guard_time = phy->guard_time_write;
>> +
>> +     do {
>> +             ret = i2c_transfer(phy->i2c_client->adapter, msg, 1);
>> +             if (ret < 0)
>> +                     usleep_range(GUARD_TIME_ERR_MIN, GUARD_TIME_ERR_MAX);
>> +             else if (guard_time)
>> +                     usleep_range(phy->guard_time_min, phy->guard_time_max);
>> +             /* retry on TPM NACK */
>> +     } while (ret < 0 && i++ < TPM_RETRY);
>> +
>> +     return ret;
>> +}
>> +
>> +/* Check that bits which must be read zero are not set */
>> +static int sanity_check_read(u8 reg, u16 len, u8 *buf)
>> +{
>> +     u32 value;
>> +     u32 zero_mask;
>> +
>> +     switch (len) {
>> +     case sizeof(u8):
>> +             value = buf[0];
>> +             break;
>> +     case sizeof(u16):
>> +             value = le16_to_cpup((__le16 *)buf);
>> +             break;
>> +     case sizeof(u32):
>> +             value = le32_to_cpup((__le32 *)buf);
>> +             break;
>> +     default:
>> +             return 0;
>> +     }
>> +
>> +     switch (reg) {
>> +     case TPM_I2C_ACCESS:
>> +             zero_mask = TPM_ACCESS_READ_ZERO;
>> +             break;
>> +     case TPM_INT_ENABLE(0) & TPM_TIS_REGISTER_MASK:
>> +             zero_mask = TPM_INT_ENABLE_ZERO;
>> +             break;
>> +     case TPM_STS(1) & TPM_TIS_REGISTER_MASK:
>> +             zero_mask = TPM_STS_READ_ZERO;
>> +             break;
>> +     case TPM_INTF_CAPS(0) & TPM_TIS_REGISTER_MASK:
>> +             zero_mask = TPM_INTF_CAPABILITY_ZERO;
>> +             break;
>> +     case TPM_I2C_INTERFACE_CAPABILITY:
>> +             zero_mask = TPM_I2C_INTERFACE_CAPABILITY_ZERO;
>> +             break;
>> +     default:
>> +             return 0;
>> +     }
>> +
>> +     if (unlikely((value & zero_mask) != 0x00)) {
>> +             pr_debug("TPM I2C read of register 0x%02x failed sanity check: 0x%x\n", reg, value);
>> +             return -EIO;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
>> +                               u8 *result, enum tpm_tis_io_mode io_mode)
>> +{
>> +     struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
>> +     struct i2c_msg msg = { .addr = phy->i2c_client->addr };
>> +     u8 reg = address_to_register(addr);
>> +     int i = 0;
>> +     int ret;
>> +
>> +     do {
>> +             /* write register */
>> +             msg.len = sizeof(reg);
>> +             msg.buf = &reg;
>> +             msg.flags = 0;
>> +             retry_i2c_transfer_until_ack(data, &msg);
>> +             if (ret < 0)
>> +                     return ret;
>> +
>> +             /* read data */
>> +             msg.buf = result;
>> +             msg.len = len;
>> +             msg.flags = I2C_M_RD;
>> +             retry_i2c_transfer_until_ack(data, &msg);
>> +             if (ret < 0)
>> +                     return ret;
>> +
>> +             ret = sanity_check_read(reg, len, result);
>> +             if (ret == 0)
>> +                     return 0;
>> +
>> +             usleep_range(GUARD_TIME_ERR_MIN, GUARD_TIME_ERR_MAX);
>> +     } while (i++ < TPM_RETRY);
>> +
>> +     return ret;
>> +}
>> +
>> +static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
>> +                                const u8 *value,
>> +                                enum tpm_tis_io_mode io_mode)
>> +{
>> +     struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
>> +     struct i2c_msg msg = { .addr = phy->i2c_client->addr };
>> +     u8 reg = address_to_register(addr);
>> +     int ret = 0;
>> +
>> +     if (len > TPM_BUFSIZE - 1)
>> +             return -EIO;
>> +
>> +     /* write register and data in one go */
>> +     phy->io_buf[0] = reg;
>> +     memcpy(phy->io_buf + sizeof(reg), value, len);
>> +
>> +     msg.len = sizeof(reg) + len;
>> +     msg.buf = phy->io_buf;
>> +     retry_i2c_transfer_until_ack(data, &msg);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return 0;
>> +}
>> +
>> +static int init_guard_time(struct tpm_tis_i2c_phy *phy)
>> +{
>> +     u32 i2c_caps;
>> +     int ret;
>> +
>> +     phy->guard_time_read = true;
>> +     phy->guard_time_write = true;
>> +     phy->guard_time_min = GUARD_TIME_DEFAULT_MIN;
>> +     phy->guard_time_max = GUARD_TIME_DEFAULT_MAX;
>> +
>> +     ret = tpm_tis_i2c_read_bytes(&phy->priv, TPM_I2C_INTERFACE_CAPABILITY,
>> +                                  sizeof(i2c_caps), (u8 *)&i2c_caps,
>> +                                  TPM_TIS_PHYS_32);
>> +     if (ret)
>> +             return ret;
>> +
>> +     phy->guard_time_read = (i2c_caps & TPM_GUARD_TIME_RR_MASK) ||
>> +                            (i2c_caps & TPM_GUARD_TIME_RW_MASK);
>> +     phy->guard_time_write = (i2c_caps & TPM_GUARD_TIME_WR_MASK) ||
>> +                             (i2c_caps & TPM_GUARD_TIME_WW_MASK);
>> +     phy->guard_time_min = (i2c_caps & TPM_GUARD_TIME_MIN_MASK) >>
>> +                           TPM_GUARD_TIME_MIN_SHIFT;
>> +     /* guard_time_max = guard_time_min * 1.2 */
>> +     phy->guard_time_max = phy->guard_time_min + phy->guard_time_min / 5;
>> +
>> +     return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
>> +
>> +static const struct tpm_tis_phy_ops tpm_i2c_phy_ops = {
>> +     .read_bytes = tpm_tis_i2c_read_bytes,
>> +     .write_bytes = tpm_tis_i2c_write_bytes,
>> +};
>> +
>> +static int tpm_tis_i2c_probe(struct i2c_client *dev,
>> +                          const struct i2c_device_id *id)
>> +{
>> +     struct tpm_tis_i2c_phy *phy;
>> +     const u8 locality = 0;
>> +     int ret;
>> +
>> +     phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_i2c_phy),
>> +                        GFP_KERNEL);
>> +     if (!phy)
>> +             return -ENOMEM;
>> +
>> +     phy->io_buf = devm_kzalloc(&dev->dev, TPM_BUFSIZE, GFP_KERNEL);
>> +     if (!phy->io_buf)
>> +             return -ENOMEM;
>> +
>> +     phy->i2c_client = dev;
>> +
>> +     /* must precede all communication with the tpm */
>> +     ret = init_guard_time(phy);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = tpm_tis_i2c_write_bytes(&phy->priv, TPM_LOC_SEL, sizeof(locality),
>> +                                   &locality, TPM_TIS_PHYS_8);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return tpm_tis_core_init(&dev->dev, &phy->priv, -1, &tpm_i2c_phy_ops,
>> +                              NULL);
>> +}
>> +
>> +static int tpm_tis_i2c_remove(struct i2c_client *client)
>> +{
>> +     struct tpm_chip *chip = i2c_get_clientdata(client);
>> +
>> +     tpm_chip_unregister(chip);
>> +     tpm_tis_remove(chip);
>> +     return 0;
>> +}
>> +
>> +static const struct i2c_device_id tpm_tis_i2c_id[] = {
>> +     { "tpm_tis_i2c", 0 },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
>> +
>> +static const struct of_device_id of_tis_i2c_match[] = {
>> +     { .compatible = "infineon,slb9673", },
>> +     { .compatible = "nuvoton,npct75x", },
>> +     { .compatible = "tcg,tpm-tis-i2c", },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
>> +
>> +static struct i2c_driver tpm_tis_i2c_driver = {
>> +     .driver = {
>> +             .owner = THIS_MODULE,
>> +             .name = "tpm_tis_i2c",
>> +             .pm = &tpm_tis_pm,
>> +             .of_match_table = of_match_ptr(of_tis_i2c_match),
>> +     },
>> +     .probe = tpm_tis_i2c_probe,
>> +     .remove = tpm_tis_i2c_remove,
>> +     .id_table = tpm_tis_i2c_id,
>> +};
>> +module_i2c_driver(tpm_tis_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("TPM Driver for native I2C access");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.31.1.windows.1
>>
> 
> Quality looks quite solid. Have you tried:
> 
> - TPM2 kselftest

Yes, kselftest runs successfully. However, it can time out.
I will fix this in a subsequent patch. Basically:

echo timeout=1500 > tools/testing/selftests/tpm2/settings

The line coverage for tpm_tis_i2c.c is 98.1%.

> - rmmod + modprobe (sanity check)

Ran that in a loop. No errors.

> 
> BR, Jarkko

Thanks, Johannes