[PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Oshri Alkoby]

This patch set adds support for TPM devices that implement the I2C
interface defined by TCG PTP specification:
https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf

The driver was tested on Raspberry-Pie 3, using Nuvoton NPCT75X TPM.

changes
-------

v2:
- Extended device tree binding description

v1:
- Initial version

Oshri Alkoby (2):
  dt-bindings: tpm: add the TPM I2C PTP device tree binding
    documentation
  char: tpm: add new driver for tpm i2c ptp

 .../bindings/security/tpm/tpm-i2c-ptp.txt     |   24 +
 drivers/char/tpm/Kconfig                      |   22 +
 drivers/char/tpm/Makefile                     |    1 +
 drivers/char/tpm/tpm_i2c_ptp.c                | 1099 +++++++++++++++++
 4 files changed, 1146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt
 create mode 100644 drivers/char/tpm/tpm_i2c_ptp.c

-- 
2.18.0

[PATCH v2 1/2] dt-bindings: tpm: add the TPM I2C PTP device tree binding documentation

[Oshri Alkoby]

Signed-off-by: Oshri Alkoby <oshrialkoby85@gmail.com>
---
 .../bindings/security/tpm/tpm-i2c-ptp.txt     | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt

diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt b/Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt
new file mode 100644
index 000000000000..8b0207fdf3e2
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt
@@ -0,0 +1,24 @@
+* Device Tree Bindings for I2C PTP based Trusted Platform Module(TPM)
+
+The TCG defines a hardware protocol, registers and interface (based
+on the TPM Interface Specification) for accessing TPM devices
+implemented with an I2C interface.
+
+Refer to the 'I2C Interface Definition' section in 'TCG PC Client
+PlatformTPMProfile(PTP) Specification' publication for specification.
+
+Required properties:
+
+- compatible     : Should be "tcg,tpm_i2c_ptp"
+- reg            : Address on the bus
+- tpm-pirq       : Input gpio pin, used for host interrupts
+
+Example (for Raspberry Pie 3 Board with Nuvoton's NPCT75X (2.0)
+-------------------------------------------------------------------
+
+tpm_i2c_ptp: tpm_i2c_ptp@2e {
+
+	compatible = "tcg,tpm_i2c_ptp";
+	reg = <0x2e>;
+	tpm-pirq = <&gpio 24 GPIO_ACTIVE_HIGH>;
+};
-- 
2.18.0

[PATCH v2 2/2] char: tpm: add new driver for tpm i2c ptp

[Oshri Alkoby]

Signed-off-by: Oshri Alkoby <oshrialkoby85@gmail.com>
---
 drivers/char/tpm/Kconfig       |   22 +
 drivers/char/tpm/Makefile      |    1 +
 drivers/char/tpm/tpm_i2c_ptp.c | 1099 ++++++++++++++++++++++++++++++++
 3 files changed, 1122 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_i2c_ptp.c

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 88a3c06fc153..ea4138145191 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -97,6 +97,28 @@ config TCG_TIS_I2C_NUVOTON
 	  To compile this driver as a module, choose M here; the module
 	  will be called tpm_i2c_nuvoton.
 
+config TCG_TIS_I2C_PTP
+	tristate "TPM Interface Specification 1.2/2.0 Interface (I2C - PTP)"
+	depends on I2C
+	select CRC_CCITT
+	---help---
+	  If you have a TPM security chip with an I2C interface that impelements
+	  the TPM I2C interface protocol defined by the PTP 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_i2c_ptp.
+
+config TCG_TIS_I2C_PTP_MAX_SIZE
+	prompt "Max I2C Buffer Size"
+	depends on TCG_TIS_I2C_PTP
+	int
+	default 32
+	help
+	  Set the maximal I2C buffer size. This will alter the default value. A
+	  different size can be set by using a module parameter (i2c_max_size)
+	  as well. If no parameter is provided when loading, this is the value
+	  that will be used.
+
 config TCG_NSC
 	tristate "National Semiconductor TPM Interface"
 	depends on X86
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a01c4cab902a..d736f22205cb 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.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
+obj-$(CONFIG_TCG_TIS_I2C_PTP) += tpm_i2c_ptp.o
 obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
 obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
 obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
diff --git a/drivers/char/tpm/tpm_i2c_ptp.c b/drivers/char/tpm/tpm_i2c_ptp.c
new file mode 100644
index 000000000000..cc1065b79c2d
--- /dev/null
+++ b/drivers/char/tpm/tpm_i2c_ptp.c
@@ -0,0 +1,1099 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2014-2019 Nuvoton Technology corporation
+ *
+ * TPM I2C PTP
+ *
+ * TPM I2C Device Driver Interface for devices that implement the TPM I2C
+ * Interface defined by TCG PC Client Platform TPM Profile (PTP) Specification
+ * Revision 01.03 v22 at www.trustedcomputinggroup.org
+ */
+
+#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/i2c.h>
+#include <linux/freezer.h>
+#include <linux/of_device.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/crc-ccitt.h>
+#include "tpm.h"
+
+/* I2C interface offsets */
+#define TPM_LOC_SEL                     0x00
+#define TPM_ACCESS                      0x04
+#define TPM_INT_ENABLE                  0x08
+#define TPM_INT_STATUS                  0x10
+#define TPM_INT_CAPABILITY              0x14
+#define TPM_STS                         0x18
+#define TPM_STS_BURST_COUNT             0x19
+#define TPM_DATA_FIFO                   0x24
+#define TPM_I2C_INTERFACE_CAPABILITY    0x30
+#define TPM_I2C_DEVICE_ADDRESS          0x38
+#define TPM_DATA_CSUM_ENABLE            0x40
+#define TPM_DATA_CSUM                   0x44
+#define TPM_VID_DID                     0x48
+#define TPM_RID                         0x4C
+
+ /* max. command/response length */
+#define TPM_I2C_BUFSIZE                 2048
+
+/* I2C bus device maximum buffer size w/o counting I2C address or command */
+#define TPM_I2C_MAX_BUF_SIZE            32
+
+#define TPM_I2C_MAX_RETRY_CNT           5
+#define TPM_I2C_RETRY_DELAY_SHORT_US    (2 * 1000)
+#define TPM_I2C_RETRY_DELAY_LONG_US     (10 * 1000)
+#define TPM_I2C_DELAY_RANGE_US          300
+
+#define OF_IS_TPM2 ((void *)1)
+#define I2C_IS_TPM2 1
+
+struct tpm_tis_data {
+	u16 manufacturer_id;
+	int locality;
+	int irq;
+	bool irq_tested;
+	unsigned int flags;
+	wait_queue_head_t int_queue;
+	wait_queue_head_t read_queue;
+	const struct tpm_tis_phy_ops *phy_ops;
+	unsigned int intrs;
+	unsigned short rng_quality;
+};
+
+#define MAX_COUNT               3
+#define SLEEP_DURATION_LOW      55
+#define SLEEP_DURATION_HI       65
+
+static int iic_tpm_read(struct i2c_client *client, u8 addr, u8 *buffer,
+			size_t len)
+{
+	struct i2c_msg msg1 = {
+		.addr = client->addr,
+		.len = 1,
+		.buf = &addr
+	};
+	struct i2c_msg msg2 = {
+		.addr = client->addr,
+		.flags = I2C_M_RD,
+		.len = len,
+		.buf = buffer
+	};
+	int rc = 0;
+	int count;
+
+	if (!client->adapter->algo->master_xfer)
+		return -EOPNOTSUPP;
+	i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
+	for (count = 0; count < MAX_COUNT; count++) {
+		rc = __i2c_transfer(client->adapter, &msg1, 1);
+		if (rc > 0)
+			break;	/* break here to skip sleep */
+		usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+	}
+	if (rc <= 0)
+		goto out;
+	for (count = 0; count < MAX_COUNT; count++) {
+		rc = __i2c_transfer(client->adapter, &msg2, 1);
+		if (rc > 0)
+			break;
+		usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+	}
+out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
+	usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+	if (rc <= 0)
+		return -EIO;
+	return len;
+}
+
+static u8 tpm_dev_buf[TPM_I2C_BUFSIZE + sizeof(u8)]; /* max buff size + addr */
+
+#ifdef CONFIG_TCG_TIS_I2C_PTP_MAX_SIZE
+	static int i2c_max_size = CONFIG_TCG_TIS_I2C_PTP_MAX_SIZE;
+#else
+	static int i2c_max_size = TPM_I2C_MAX_BUF_SIZE;
+#endif
+module_param(i2c_max_size, int, 0660);
+
+static int iic_tpm_write_generic(struct i2c_client *client,
+				 u8 addr, u8 *buffer, size_t len,
+				 unsigned int sleep_low,
+				 unsigned int sleep_hi, u8 max_count)
+{
+	int rc = -EIO;
+	int count;
+	struct i2c_msg msg1 = {
+		.addr = client->addr,
+		.len = len + 1,
+		.buf = tpm_dev_buf
+	};
+
+	if (len > TPM_BUFSIZE)
+		return -EINVAL;
+	if (!client->adapter->algo->master_xfer)
+		return -EOPNOTSUPP;
+	i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
+	tpm_dev_buf[0] = addr;
+	memcpy(&tpm_dev_buf[1], buffer, len);
+	for (count = 0; count < max_count; count++) {
+		rc = __i2c_transfer(client->adapter, &msg1, 1);
+		if (rc > 0)
+			break;
+		usleep_range(sleep_low, sleep_hi);
+	}
+	i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
+	usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+	if (rc <= 0)
+		return -EIO;
+	return 0;
+}
+
+static s32 i2c_ptp_read_buf(struct i2c_client *client, u8 offset, size_t size,
+			    u8 *data)
+{
+	s32 status;
+
+	status = iic_tpm_read(client, offset, data, size);
+	dev_dbg(&client->dev,
+		"%s(offset=%u size=%zu data=%*ph) -> sts=%d\n", __func__,
+		offset, size, (int)size, data, status);
+	return status;
+}
+
+static s32 i2c_ptp_write_buf(struct i2c_client *client, u8 offset, size_t size,
+			     u8 *data)
+{
+	s32 status;
+
+	status = iic_tpm_write_generic(client, offset, data, size,
+				       SLEEP_DURATION_LOW, SLEEP_DURATION_HI,
+				       MAX_COUNT);
+	dev_dbg(&client->dev,
+		"%s(offset=%u size=%zu data=%*ph) -> sts=%d\n", __func__,
+		offset, size, (int)size, data, status);
+
+	return status;
+}
+
+#define TPM_INT_DATA_AVAIL		(BIT(0))
+#define TPM_INT_STS_VALID		(BIT(1))
+#define TPM_INT_LOC_CHANGED		(BIT(2))
+#define TPM_INT_CMD_READY		(BIT(7))
+#define TPM_INT_GLOBAL			(BIT(31))
+
+#define TPM_INT_TO_ACTIVATE		(TPM_INT_DATA_AVAIL)
+
+static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
+				   bool check_cancel, bool *canceled)
+{
+	u8 status = chip->ops->status(chip);
+
+	*canceled = false;
+	if (status != 0xff && (status & mask) == mask)
+		return true;
+	if (check_cancel && chip->ops->req_canceled(chip, status)) {
+		*canceled = true;
+		return true;
+	}
+	return false;
+}
+
+int i2c_ptp_wait_for_tpm_stat_l(struct tpm_chip *chip, u8 mask,
+				unsigned long timeout, wait_queue_head_t *queue,
+				bool check_cancel)
+{
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	unsigned long stop, start, long_delay;
+	long rc;
+	u8 status;
+	bool canceled = false;
+	unsigned int cur_intrs;
+
+	start = jiffies;
+	stop = start + timeout;
+	long_delay = start + usecs_to_jiffies(TPM_I2C_RETRY_DELAY_LONG_US);
+
+	if (queue && chip->flags & TPM_CHIP_FLAG_IRQ) {
+again:
+		cur_intrs = priv->intrs;
+		timeout = stop - jiffies;
+		if ((long)timeout <= 0)
+			return -ETIME;
+
+		enable_irq(priv->irq);
+		rc = wait_event_interruptible_timeout(*queue,
+						      ((cur_intrs != priv->intrs) &&
+						      wait_for_tpm_stat_cond(chip, mask, check_cancel, &canceled)),
+						      timeout);
+		if (rc > 0) {
+			if (canceled)
+				return -ECANCELED;
+			return 0;
+		}
+		if (rc == -ERESTARTSYS && freezing(current)) {
+			clear_thread_flag(TIF_SIGPENDING);
+			goto again;
+		}
+	} else {
+		do {
+			status = chip->ops->status(chip);
+			if (status != 0xff && (status & mask) == mask)
+				return 0;
+
+			if (time_before(jiffies, long_delay))
+				usleep_range(TPM_I2C_RETRY_DELAY_SHORT_US,
+					     TPM_I2C_RETRY_DELAY_SHORT_US
+					     + TPM_I2C_DELAY_RANGE_US);
+			else
+				usleep_range(TPM_I2C_RETRY_DELAY_LONG_US,
+					     TPM_I2C_RETRY_DELAY_LONG_US
+					     + TPM_I2C_DELAY_RANGE_US);
+
+		} while (time_before(jiffies, stop));
+	}
+	return -ETIME;
+}
+
+/* wrapper of wait_for_tpm_stat, since we can't do the int processing during
+ * the int_handler in I2C, here we do it after the int_handler is done and
+ * wait_for_tpm_stat retruns
+ */
+static int i2c_ptp_wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
+				     unsigned long timeout,
+				     wait_queue_head_t *queue,
+				     bool check_cancel)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	u32 intsts;
+	int rc;
+
+	rc = i2c_ptp_wait_for_tpm_stat_l(chip, mask, timeout, queue,
+					 check_cancel);
+	if (rc < 0)
+		return rc;
+
+	if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+		rc = i2c_ptp_read_buf(client, TPM_INT_STATUS, 4, (u8 *)&intsts);
+		if (rc < 0) {
+			dev_err(&chip->dev,
+				"%s() fail to read TPM_INT_STATUS\n", __func__);
+			return -EIO;
+		}
+
+		/* Clear interrupts handled */
+		rc = i2c_ptp_write_buf(client, TPM_INT_STATUS, 4,
+				       (u8 *)&intsts);
+		if (rc < 0) {
+			dev_err(&chip->dev,
+				"%s() fail to clear int status\n", __func__);
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
+
+#define TPM_ACCESS_REQUEST_USE		(BIT(1))
+#define TPM_ACCESS_REQUEST_PENDING	(BIT(2))
+#define TPM_ACCESS_ACTIVE_LOCALITY	(BIT(5))
+#define TPM_ACCESS_VALID_STS		(BIT(7))
+
+/* Before we attempt to access the TPM we must see that the valid bit is set.
+ * The specification says that this bit is 0 at reset and remains 0 until the
+ * 'TPM has gone through its self test and initialization and has established
+ * correct values in the other bits.'
+ */
+static int wait_startup(struct tpm_chip *chip, int l)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	unsigned long stop = jiffies + chip->timeout_a;
+
+	do {
+		int rc;
+		u8 access;
+
+		rc = i2c_ptp_read_buf(client, TPM_ACCESS, 1, &access);
+		if (rc < 0)
+			return rc;
+
+		if (access != 0xff && (access & TPM_ACCESS_VALID_STS) != 0)
+			return 0;
+		tpm_msleep(TPM_TIMEOUT);
+	} while (time_before(jiffies, stop));
+	return -1;
+}
+
+static bool i2c_ptp_check_locality(struct tpm_chip *chip, int l)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	int rc;
+	u8 access;
+	u8 data;
+
+	data = (u8)l;
+	rc = i2c_ptp_write_buf(client, TPM_LOC_SEL, 1, &data);
+	if (rc < 0)
+		return false;
+
+	rc = i2c_ptp_read_buf(client, TPM_ACCESS, 1, &access);
+	if (rc < 0)
+		return false;
+
+	if ((access & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID_STS)) ==
+	    (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID_STS)) {
+		priv->locality = l;
+		return true;
+	}
+
+	return false;
+}
+
+static bool i2c_ptp_locality_inactive(struct tpm_chip *chip, int l)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	int rc;
+	u8 access;
+	u8 data;
+
+	data = (u8)l;
+	rc = i2c_ptp_write_buf(client, TPM_LOC_SEL, 1, &data);
+	if (rc < 0)
+		return false;
+
+	rc = i2c_ptp_read_buf(client, TPM_ACCESS, 1, &access);
+	if (rc < 0)
+		return false;
+
+	if ((access & (TPM_ACCESS_VALID_STS | TPM_ACCESS_ACTIVE_LOCALITY))
+	    == TPM_ACCESS_VALID_STS)
+		return true;
+
+	return false;
+}
+
+static int i2c_ptp_release_locality(struct tpm_chip *chip, int l)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	unsigned long stop;
+	int rc;
+	u8 data;
+
+	if (i2c_ptp_locality_inactive(chip, l))
+		return 0;
+
+	data = TPM_ACCESS_ACTIVE_LOCALITY;
+	rc = i2c_ptp_write_buf(client, TPM_ACCESS, 1, &data);
+	if (rc < 0)
+		return rc;
+
+	stop = jiffies + chip->timeout_a;
+	do {
+		if (i2c_ptp_locality_inactive(chip, l))
+			return 0;
+
+		tpm_msleep(TPM_TIMEOUT);
+	} while (time_before(jiffies, stop));
+
+	return -1;
+}
+
+static int i2c_ptp_request_locality(struct tpm_chip *chip, int l)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	unsigned long stop;
+	int rc;
+	u8 data;
+
+	if (i2c_ptp_check_locality(chip, l))
+		return l;
+
+	data = TPM_ACCESS_REQUEST_USE;
+	rc = i2c_ptp_write_buf(client, TPM_ACCESS, 1, &data);
+	if (rc < 0)
+		return rc;
+
+	stop = jiffies + chip->timeout_a;
+
+	do {
+		if (i2c_ptp_check_locality(chip, l))
+			return l;
+
+		rc = i2c_ptp_write_buf(client, TPM_ACCESS, 1, &data);
+		if (rc < 0)
+			return rc;
+
+		tpm_msleep(TPM_TIMEOUT);
+	} while (time_before(jiffies, stop));
+
+	return -1;
+}
+
+#define TPM_STS_RESPONSE_RETRY (BIT(1))
+#define TPM_STS_SELFTEST_DONE  (BIT(2))
+#define TPM_STS_EXPECT         (BIT(3))
+#define TPM_STS_DATA_AVAIL     (BIT(4))
+#define TPM_STS_GO             (BIT(5))
+#define TPM_STS_COMMAND_READY  (BIT(6))
+#define TPM_STS_VALID          (BIT(7))
+#define TPM_STS_COMMAND_CANCEL (BIT(24))
+
+/* bit1...bit0 reads always 0 */
+#define TPM_STS_ERR_VAL        (BIT(0) | BIT(1))
+
+/* read TPM_STS register */
+static u8 i2c_ptp_status(struct tpm_chip *chip)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	int rc;
+	u8 status;
+
+	rc = i2c_ptp_read_buf(client, TPM_STS, 1, &status);
+	if (rc < 0)
+		return 0;
+
+	return status;
+}
+
+/* write commandReady to TPM_STS register */
+static void i2c_ptp_ready(struct tpm_chip *chip)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	u8 data;
+
+	data = TPM_STS_COMMAND_READY;
+	i2c_ptp_write_buf(client, TPM_STS, 1, &data);
+}
+
+/* read Burst Count */
+static int i2c_ptp_get_burstcount(struct tpm_chip *chip)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	unsigned long stop;
+	int burstcnt = 0, rc;
+
+	/* wait for burstcount */
+	stop = jiffies + chip->timeout_a;
+
+	do {
+		rc = i2c_ptp_read_buf(client, TPM_STS_BURST_COUNT, 2,
+				      (u8 *)&burstcnt);
+		if (rc < 0)
+			return rc;
+
+		if (burstcnt)
+			return burstcnt;
+
+		usleep_range(TPM_TIMEOUT_USECS_MIN, TPM_TIMEOUT_USECS_MAX);
+	} while (time_before(jiffies, stop));
+
+	return -EBUSY;
+}
+
+/* write commandCancel to TPM_STS register */
+static void i2c_ptp_cancel(struct tpm_chip *chip)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	u32 data;
+
+	data = TPM_STS_COMMAND_CANCEL;
+	i2c_ptp_write_buf(client, TPM_STS, 4, (u8 *)&data);
+}
+
+/* enable checksum */
+static int i2c_ptp_enable_csum(struct tpm_chip *chip)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	s32 rc;
+	u8 data = 1;
+
+	/* Enable CSUM */
+	rc = i2c_ptp_write_buf(client, TPM_DATA_CSUM_ENABLE, 1, &data);
+	if (rc < 0) {
+		dev_err(&chip->dev,
+			"%s() fail to read to TPM_DATA_CSUM_ENABLE: %d\n",
+			__func__, rc);
+		return rc;
+	}
+	return 0;
+}
+
+/* Caclculate crc16 of the buffer and compares it to TPM_DATA_CSUM */
+static int i2c_ptp_check_crc(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	s32 status;
+	u16 tpm_csum = 0;
+	u16 crc = 0;
+
+	crc = crc_ccitt(crc, buf, len);
+	crc = be16_to_cpu(crc);
+	status = i2c_ptp_read_buf(client, TPM_DATA_CSUM, 2, (u8 *)&tpm_csum);
+	if (status <= 0) {
+		dev_err(&chip->dev, "%s() fail to read to TPM_DATA_CSUM: %d\n",
+			__func__, status);
+		return -EIO;
+	}
+
+	if (tpm_csum != crc) {
+		dev_err(&chip->dev, "%s() bad data csum\n", __func__);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int i2c_ptp_recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	int size = 0, burstcnt, rc, bytes2read = count;
+	bool paramsize_flag = false;
+
+	while (size < bytes2read) {
+		burstcnt = i2c_ptp_get_burstcount(chip);
+		if (burstcnt <= 0)
+			return burstcnt;
+
+		burstcnt = min_t(int, (bytes2read - size), burstcnt);
+		rc = i2c_ptp_read_buf(client, TPM_DATA_FIFO, burstcnt,
+				      buf + size);
+		if (rc < 0)
+			return rc;
+
+		size += burstcnt;
+
+		if (size >= 6 && !paramsize_flag) {
+			bytes2read = be32_to_cpu(*(__be32 *)(buf + 2));
+			paramsize_flag = true;
+		}
+	}
+
+	return size;
+}
+
+static int i2c_ptp_recv(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	int size = 0;
+	int status, retries;
+
+	if (count < TPM_HEADER_SIZE) {
+		status = -EIO;
+		goto out;
+	}
+
+	for (retries = 0; retries < TPM_I2C_MAX_RETRY_CNT; retries++) {
+		size = 0;
+
+		if (retries > 0) {
+			/* if this is not the first trial, set responseRetry */
+			u8 data = TPM_STS_RESPONSE_RETRY;
+
+			i2c_ptp_write_buf(client, TPM_STS, 1, &data);
+		}
+
+		status = i2c_ptp_wait_for_tpm_stat(chip,
+						   (TPM_STS_DATA_AVAIL | TPM_STS_VALID),
+						   chip->timeout_b, NULL,
+						   false);
+		if (status < 0)
+			return status;
+
+		size = i2c_ptp_recv_data(chip, buf, TPM_HEADER_SIZE);
+		if (size < TPM_HEADER_SIZE)
+			continue;
+
+		status = i2c_ptp_wait_for_tpm_stat(chip, TPM_STS_VALID,
+						   chip->timeout_a, NULL,
+						   false);
+		if (status < 0)
+			continue;
+
+		/* check CRC */
+		status = i2c_ptp_check_crc(chip, buf, size);
+		if (status < 0)
+			continue;
+
+		status = size;
+		break;
+	}
+
+out:
+	i2c_ptp_ready(chip);
+	return status;
+}
+
+/*
+ * If interrupts are used (signaled by an irq set in the vendor structure)
+ * tpm.c can skip polling for the data to be available as the interrupt is
+ * waited for here
+ */
+static int i2c_ptp_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	int rc, status, burstcnt, retries;
+	size_t count = 0, bytes2send;
+	u32 ordinal = be32_to_cpu(*((__be32 *)(buf + 6)));
+	u32 intmask;
+
+	/* When NPCT7XX FU Mode command or startup (when TPM I2C may be has
+	 * been reset), wait for STS_VALID and re-initialize I2C regs
+	 */
+	if (ordinal == 0x20000201 || ordinal == 0x144) {
+		// wait for I2C to be ready
+		if (wait_startup(chip, 0) != 0)
+			return -ENODEV;
+
+		rc = i2c_ptp_enable_csum(chip);
+		if (rc < 0) {
+			dev_err(&chip->dev, "%s() fail to enable checksum\n",
+				__func__);
+			return rc;
+		}
+
+		// in interrupt mode re-eanble and clear the interrupts
+		if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+			intmask = (TPM_INT_TO_ACTIVATE | TPM_INT_GLOBAL);
+			i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4,
+					  (u8 *)&intmask);
+			i2c_ptp_write_buf(client, TPM_INT_STATUS, 4,
+					  (u8 *)&intmask);
+		}
+	}
+
+	for (retries = 0; retries < TPM_I2C_MAX_RETRY_CNT; retries++) {
+		count = 0;
+		rc = -1;
+
+		if (retries > 0) {
+			/* if this is not the first trial, abort the command */
+			i2c_ptp_ready(chip);
+		}
+
+		status = i2c_ptp_status(chip);
+		if ((status & TPM_STS_COMMAND_READY) == 0) {
+			i2c_ptp_ready(chip);
+			if (i2c_ptp_wait_for_tpm_stat
+			    (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
+			     NULL, false) < 0) {
+				rc = -ETIME;
+				continue;
+			}
+		}
+
+		while (count < len) {
+			burstcnt = i2c_ptp_get_burstcount(chip);
+			if (burstcnt < 0) {
+				dev_err(&chip->dev, "Unable to read burstcount\n");
+				rc = burstcnt;
+				break;
+			}
+
+			bytes2send = min_t(int, i2c_max_size, burstcnt);
+			bytes2send = min_t(int, bytes2send, (len - count));
+			rc = i2c_ptp_write_buf(client, TPM_DATA_FIFO,
+					       bytes2send, buf + count);
+			if (rc < 0)
+				break;
+
+			count += bytes2send;
+		}
+
+		if (rc < 0)
+			continue;
+
+		if (i2c_ptp_wait_for_tpm_stat(chip, TPM_STS_VALID,
+					      chip->timeout_a, NULL,
+					      false) < 0) {
+			rc = -ETIME;
+			continue;
+		}
+
+		/* check CRC */
+		rc = i2c_ptp_check_crc(chip, buf, len);
+		if (rc < 0)
+			continue;
+
+		return 0;
+	}
+
+	i2c_ptp_ready(chip);
+	return rc;
+}
+
+static void disable_interrupts(struct tpm_chip *chip)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	u32 intmask;
+	int rc;
+
+	rc = i2c_ptp_read_buf(client, TPM_INT_ENABLE, 4, (u8 *)&intmask);
+	if (rc < 0)
+		intmask = 0;
+
+	intmask &= ~TPM_INT_GLOBAL;
+	rc = i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4, (u8 *)&intmask);
+	if (rc < 0)
+		dev_err(&chip->dev, "%s() fail to write TPM_INT_ENABLE\n",
+			__func__);
+
+	devm_free_irq(chip->dev.parent, priv->irq, chip);
+	priv->irq = 0;
+	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
+}
+
+/*
+ * If interrupts are used (signaled by an irq set in the vendor structure)
+ * tpm.c can skip polling for the data to be available as the interrupt is
+ * waited for here
+ */
+static int i2c_ptp_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	int rc;
+	u32 ordinal;
+	unsigned long dur;
+	u8 data;
+
+	rc = i2c_ptp_send_data(chip, buf, len);
+	if (rc < 0)
+		return rc;
+
+	/* go and do it */
+	data = TPM_STS_GO;
+	rc = i2c_ptp_write_buf(client, TPM_STS, 1, &data);
+	if (rc < 0)
+		goto out_err;
+
+	ordinal = be32_to_cpu(*((__be32 *)(buf + 6)));
+
+	dur = tpm_calc_ordinal_duration(chip, ordinal);
+	if (i2c_ptp_wait_for_tpm_stat
+	    (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
+	     &priv->int_queue, false) < 0) {
+		rc = -ETIME;
+		goto out_err;
+	}
+
+	return 0;
+out_err:
+	i2c_ptp_ready(chip);
+	return rc;
+}
+
+static int i2c_ptp_send(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+	int rc, irq;
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+
+	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
+		return i2c_ptp_send_main(chip, buf, len);
+
+	/* Verify receipt of the expected IRQ */
+	irq = priv->irq;
+	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
+	rc = i2c_ptp_send_main(chip, buf, len);
+	priv->irq = irq;
+	chip->flags |= TPM_CHIP_FLAG_IRQ;
+	if (!priv->irq_tested)
+		tpm_msleep(1);
+	if (!priv->irq_tested)
+		disable_interrupts(chip);
+	priv->irq_tested = true;
+	return rc;
+}
+
+static bool i2c_ptp_req_canceled(struct tpm_chip *chip, u8 status)
+{
+	return (status == TPM_STS_COMMAND_READY);
+}
+
+/* The only purpose for the handler is to signal to any waiting threads that
+ * the interrupt is currently being asserted. The driver does not do any
+ * processing triggered by interrupts, and the chip provides no way to mask at
+ * the source (plus that would be slow over I2C). Run the IRQ as a one-shot,
+ * this means it cannot be shared. The processing of the interrupt status
+ * is done in i2c_ptp_wait_for_tpm_stat
+ */
+static irqreturn_t i2c_ptp_int_handler(int dummy, void *dev_id)
+{
+	struct tpm_chip *chip = dev_id;
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+
+	priv->irq_tested = true;
+	priv->intrs++;
+	wake_up_interruptible(&priv->int_queue);
+	disable_irq_nosync(priv->irq);
+	return IRQ_HANDLED;
+}
+
+static int i2c_ptp_gen_interrupt(struct tpm_chip *chip)
+{
+	const char *desc = "attempting to generate an interrupt";
+	u32 cap2;
+
+	return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
+}
+
+/* Register the IRQ and issue a command that will cause an interrupt. If an
+ * irq is seen then leave the chip setup for IRQ operation, otherwise reverse
+ * everything and leave in polling mode. Returns 0 on success.
+ */
+static int i2c_ptp_probe_irq_single(struct tpm_chip *chip, u32 intmask,
+				    int flags, int irq)
+{
+	struct i2c_client *client = to_i2c_client(chip->dev.parent);
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	int rc;
+	u32 int_support;
+
+	if (devm_request_irq(chip->dev.parent, irq, i2c_ptp_int_handler, flags,
+			     dev_name(&chip->dev), chip) != 0) {
+		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
+			 irq);
+		return -1;
+	}
+	priv->irq = irq;
+
+	chip->flags |= TPM_CHIP_FLAG_IRQ;
+
+	/* check what are the interrupts that the TPM supports */
+	rc = i2c_ptp_read_buf(client, TPM_INT_CAPABILITY, 4, (u8 *)
+			   &int_support);
+	if (rc < 0)
+		return rc;
+
+	/* Clear all existing */
+	rc = i2c_ptp_write_buf(client, TPM_INT_STATUS, 4, (u8 *)&int_support);
+	if (rc < 0)
+		return rc;
+
+	/* Turn on */
+	int_support &= TPM_INT_TO_ACTIVATE;
+	int_support |= TPM_INT_GLOBAL;
+	rc = i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4, (u8 *)&int_support);
+	if (rc < 0)
+		return rc;
+
+	priv->irq_tested = false;
+
+	/* Generate an interrupt by having the core call through to
+	 * i2c_ptp_send
+	 */
+	rc = i2c_ptp_gen_interrupt(chip);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static int i2c_ptp_remove(struct i2c_client *client)
+{
+	u32 interrupt;
+	int rc;
+
+	rc = i2c_ptp_read_buf(client, TPM_INT_ENABLE, 4, (u8 *)&interrupt);
+	if (rc < 0)
+		interrupt = 0;
+
+	// Clear and disable interrupts
+	interrupt &= ~TPM_INT_GLOBAL;
+	rc = i2c_ptp_write_buf(client, TPM_INT_STATUS, 4, (u8 *)&interrupt);
+	rc = i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4, (u8 *)&interrupt);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(i2c_ptp_remove);
+
+static const struct tpm_class_ops tpm_i2c = {
+	.flags = TPM_OPS_AUTO_STARTUP,
+	.status = i2c_ptp_status,
+	.recv = i2c_ptp_recv,
+	.send = i2c_ptp_send,
+	.cancel = i2c_ptp_cancel,
+	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+	.req_canceled = i2c_ptp_req_canceled,
+	.request_locality = i2c_ptp_request_locality,
+	.relinquish_locality = i2c_ptp_release_locality,
+};
+
+int i2c_ptp_core_init(struct i2c_client *client, struct tpm_tis_data *priv,
+		      int irq, const struct tpm_tis_phy_ops *phy_ops,
+		      acpi_handle acpi_dev_handle)
+{
+	u32 vendor;
+	u32 intmask;
+	u8 rid;
+	int rc;
+	struct tpm_chip *chip;
+	struct device *dev = &client->dev;
+
+	chip = tpmm_chip_alloc(dev, &tpm_i2c);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	chip->hwrng.quality = priv->rng_quality;
+
+	/* Maximum timeouts */
+	chip->timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A);
+	chip->timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B);
+	chip->timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C);
+	chip->timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D);
+	priv->phy_ops = phy_ops;
+	priv->intrs = 0;
+	dev_set_drvdata(&chip->dev, priv);
+
+	if (wait_startup(chip, 0) != 0) {
+		rc = -ENODEV;
+		goto out_err;
+	}
+
+	/* Take control of the TPM's interrupt hardware and shut it off */
+	rc = i2c_ptp_read_buf(client, TPM_INT_ENABLE, 4, (u8 *)&intmask);
+	if (rc < 0)
+		goto out_err;
+
+	intmask = TPM_INT_TO_ACTIVATE;	// global should be off now
+	i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4, (u8 *)&intmask);
+
+	rc = i2c_ptp_enable_csum(chip);
+	if (rc < 0) {
+		dev_err(&chip->dev, "%s() fail to enable checksum\n", __func__);
+		rc = -ENODEV;
+		goto out_err;
+	}
+
+	rc = tpm_chip_start(chip);
+	if (rc)
+		goto out_err;
+
+	rc = tpm2_probe(chip);
+	if (rc)
+		goto out_err;
+
+	rc = i2c_ptp_read_buf(client, TPM_VID_DID, 4, (u8 *)&vendor);
+	if (rc < 0)
+		goto out_err;
+
+	priv->manufacturer_id = vendor;
+
+	rc = i2c_ptp_read_buf(client, TPM_RID, 1, &rid);
+	if (rc < 0)
+		goto out_err;
+
+	dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
+		 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",
+		 vendor >> 16, rid);
+
+	/* INTERRUPT Setup */
+	init_waitqueue_head(&priv->int_queue);
+	if (irq != -1) {
+		/* Before doing irq testing issue a command to the TPM in polling mode
+		 * to make sure it works. May as well use that command to set the
+		 * proper timeouts for the driver.
+		 */
+		if (tpm_get_timeouts(chip)) {
+			dev_err(dev, "Could not get TPM timeouts and durations\n");
+			rc = -ENODEV;
+			goto out_err;
+		}
+
+		i2c_ptp_probe_irq_single(chip, intmask,	IRQF_TRIGGER_LOW, irq);
+		if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
+			dev_err(&chip->dev, FW_BUG
+				"TPM interrupt not working, polling instead\n");
+	}
+
+	rc = tpm_chip_register(chip);
+	if (rc)
+		goto out_err;
+
+	return 0;
+
+out_err:
+	i2c_ptp_remove(client);
+
+	return rc;
+}
+
+static int i2c_ptp_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	int irq = -1, gpio;
+	struct tpm_tis_data *priv;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	priv = devm_kzalloc(&client->dev, sizeof(struct tpm_tis_data),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	if (i2c_max_size < 1)
+		i2c_max_size = CONFIG_TCG_TIS_I2C_PTP_MAX_SIZE;
+
+	// Get interrupt from board device
+	if (client->irq) {
+		irq = client->irq;
+		goto out;
+	}
+
+	// If IRQ doesn't exists, get GPIO from device tree
+	gpio = of_get_named_gpio(client->dev.of_node, "tpm-pirq", 0);
+	if (gpio < 0) {
+		dev_err(&client->dev, "Failed to retrieve tpm-pirq\n");
+		goto out;
+	}
+
+	// GPIO request and configuration
+	if (devm_gpio_request_one(&client->dev, gpio, GPIOF_IN, "TPM PIRQ")) {
+		dev_err(&client->dev, "Failed to request tpm-pirq pin\n");
+		goto out;
+	}
+
+	irq = gpio_to_irq(gpio);
+
+out:
+	return i2c_ptp_core_init(client, priv, irq, NULL, 0);
+}
+
+static const struct of_device_id of_i2c_ptp_match[] = {
+	{.compatible = "tcg,tpm_i2c_ptp", .data = OF_IS_TPM2},
+	{},
+};
+
+static const struct i2c_device_id i2c_ptp_id[] = {
+	{"tpm_i2c_ptp"},
+	{"tpm2_i2c_ptp", .driver_data = I2C_IS_TPM2},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, i2c_ptp_id);
+
+static SIMPLE_DEV_PM_OPS(i2c_ptp_pm_ops, tpm_pm_suspend, tpm_pm_resume);
+
+static struct i2c_driver i2c_ptp_driver = {
+	.id_table = i2c_ptp_id,
+	.probe = i2c_ptp_probe,
+	.remove = i2c_ptp_remove,
+	.driver = {
+		.name = "tpm_i2c_ptp",
+		.pm = &i2c_ptp_pm_ops,
+		.of_match_table = of_match_ptr(of_i2c_ptp_match),
+	},
+};
+module_i2c_driver(i2c_ptp_driver);
+
+MODULE_AUTHOR("Oshri Alkoby (oshri.alkoby@nuvoton.com)");
+MODULE_DESCRIPTION("TPM I2C Driver");
+MODULE_VERSION("2.1.3");
+MODULE_LICENSE("GPL");
-- 
2.18.0

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Jarkko Sakkinen]

On Fri, 2019-06-28 at 18:13 +0300, Oshri Alkoby wrote:

The long descriptions are still missing. Please take the time and write
a proper commit messages that clearly tell what the patch does.

Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements
that spec so you should only implement a new physical layer.

/Jarkko

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Alexander Steffen]

On 04.07.2019 10:43, Jarkko Sakkinen wrote:
> Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements
> that spec so you should only implement a new physical layer.

I had the same thought. Unfortunately, the I2C-TIS specification 
introduces two relevant changes compared to tpm_tis/tpm_tis_spi:

1. Locality is not encoded into register addresses anymore, but stored 
in a separate register.
2. Several register addresses have changed (but still contain compatible 
contents).

I'd still prefer not to duplicate all the high-level logic from 
tpm_tis_core. But this will probably mean to introduce some new 
interfaces between tpm_tis_core and the physical layers.

Also, shouldn't the new driver be called tpm_tis_i2c, to group it with 
all the other (TIS) drivers, that implement a vendor-independent 
protocol? With tpm_i2c_ptp users might assume that ptp is just another 
vendor.

Alexander

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Jarkko Sakkinen]

On Thu, 2019-07-04 at 13:29 +0200, Alexander Steffen wrote:
> On 04.07.2019 10:43, Jarkko Sakkinen wrote:
> > Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements
> > that spec so you should only implement a new physical layer.
> 
> I had the same thought. Unfortunately, the I2C-TIS specification 
> introduces two relevant changes compared to tpm_tis/tpm_tis_spi:

I doubt that there was any comparison made.

> 1. Locality is not encoded into register addresses anymore, but stored 
> in a separate register.
> 2. Several register addresses have changed (but still contain compatible 
> contents).
> 
> I'd still prefer not to duplicate all the high-level logic from 
> tpm_tis_core. But this will probably mean to introduce some new 
> interfaces between tpm_tis_core and the physical layers.

Agreed. Some plumbing needs to be done in tpm_tis_core to make it work
for this. We definitely do not want to duplicate code that has been
field tested for years.

> Also, shouldn't the new driver be called tpm_tis_i2c, to group it with 
> all the other (TIS) drivers, that implement a vendor-independent 
> protocol? With tpm_i2c_ptp users might assume that ptp is just another 
> vendor.

Yes, absolutely. I guess the driver has been done without looking at
what already exist in the TPM kernel stack.

/Jarkko

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Jarkko Sakkinen]

On Thu, 2019-07-04 at 12:48 -0500, Oshri Alkobi wrote:
> Alex, Jarkko, thank you very much for your feedbacks!

Please configure your email client to use plain text.

> I totally agree, there are some duplications that can be common, indeed it
> will require some work in tpm_tis_core.
> Since I believe it is not going to happen soon, I would suggest to examine
> what duplications can currently be dropped from the new driver, so the kernel
> will support the PTP I2C interface in the meantime.
> I will appreciate getting ideas about any tpm_tis_core logic that currently
> can be used as is by the new drive.

I rather wait for a solution that integrates with our mature stack for
TIS (or these days FIFO) than integrate something half-baked. If you
want something in, please do right things right.

What you are proposing would mean maintaining duplicate stacks forever.

> Since the TIS is an old specification that mostly defines FIFO for TPM1.2 I
> would say the name tpm_tis_i2c does not completely reflect its goal. However
> we really don't have any problem with any name that the group will agree on.
> Does tpm_ptp_i2c sound better than the current name?

Absolutely not going to use that name. The naming convention is what
it is for other drivers that are adapt tpm_tis_core to different HW
interfaces.

/Jarkko

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Jarkko Sakkinen]

On Mon, Jul 15, 2019 at 11:08:47AM +0300, Tomer Maimon wrote:
>    Thanks for your feedback and sorry for the late response.
>
>    Due to the amount of work required to handle this technical feedback and
>    project constraints we need to put this task on hold for the near future.
>
>    In the meantime, anyone from the community is welcome to take over this
>    code and handle the re-design for the benefit of the entire TPM community.

Ok, so there is already driver for this called tpm_tis_core.

So you go and create a new module, whose name given the framework of
things that we already have deployed, is destined to be tpm_tis_i2c.

Then you roughly implement a new physical layer by using  a callback
interface provided to you by tpm_tis_core.

The so called re-design was already addressed by Alexander [1].

How hard can it be seriously?

[1] https://lkml.org/lkml/2019/7/4/331

/Jarkko

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Alexander Steffen]

On 15.07.2019 10:08, Tomer Maimon wrote:
> Hi Jarkko and All,
> 
> Thanks for your feedback and sorry for the late response.
> 
> 
> Due to the amount of work required to handle this technical feedback and 
> project constraints we need to put this task on hold for the near future.
> 
> In the meantime, anyone from the community is welcome to take over this 
> code and handle the re-design for the benefit of the entire TPM community.

Too bad you lack the time to finish this.

If somebody else were to pick it up, would it be possible for you to 
provide a couple of TPMs that implement this protocol, so that it can be 
properly tested?

Alexander

RE: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Eyal.Cohen]

Hi Jarkko and Alexander,

We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
At a minimum we thought of:
1. Handling TPM Localities in I2C is different
2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors
4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry)
Besides this, during development we might encounter additional differences between SPI and I2C.

We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window.

Regards,
Eyal.

-----Original Message-----
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Sent: Monday, July 15, 2019 12:46 PM
To: Tomer Maimon <tmaimon77@gmail.com>
Cc: Oshri Alkobi <oshrialkoby85@gmail.com>; Alexander Steffen <Alexander.Steffen@infineon.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; peterhuewe@gmx.de; jgg@ziepe.ca; Arnd Bergmann <arnd@arndb.de>; Greg KH <gregkh@linuxfoundation.org>; IS20 Oshri Alkoby <oshri.alkoby@nuvoton.com>; devicetree <devicetree@vger.kernel.org>; AP MS30 Linux Kernel community <linux-kernel@vger.kernel.org>; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; IS30 Dan Morav <Dan.Morav@nuvoton.com>; IS20 Eyal Cohen <Eyal.Cohen@nuvoton.com>
Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

On Mon, Jul 15, 2019 at 11:08:47AM +0300, Tomer Maimon wrote:
>    Thanks for your feedback and sorry for the late response.
>
>    Due to the amount of work required to handle this technical feedback and
>    project constraints we need to put this task on hold for the near future.
>
>    In the meantime, anyone from the community is welcome to take over this
>    code and handle the re-design for the benefit of the entire TPM community.

Ok, so there is already driver for this called tpm_tis_core.

So you go and create a new module, whose name given the framework of things that we already have deployed, is destined to be tpm_tis_i2c.

Then you roughly implement a new physical layer by using  a callback interface provided to you by tpm_tis_core.

The so called re-design was already addressed by Alexander [1].

How hard can it be seriously?

[1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2019_7_4_331&d=DwIBAg&c=ue8mO8zgC4VZ4q_aNVKt8G9MC01UFDmisvMR1k-EoDM&r=hjzIxEsS8wf3Ezr__r0ZOjw3kki8jIlQK-SQ5pWhXaM&m=aYs86sTFmxqvlI5rE2AWLGRl0lb9XRuiRKVtsCaXdRM&s=_3MMoq5UISFwMfK4OfgLbA2kMfZVgEfgkaWKqDEUXGw&e=

/Jarkko


===========================================================================================
The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Alexander Steffen]

On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
> Hi Jarkko and Alexander,
> 
> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.

Great :) In the meantime, I've done some experiments creating an I2C 
driver based on tpm_tis_core, see 
https://patchwork.kernel.org/patch/11049363/ Please have a look at that 
and provide your feedback (and/or use it as a basis for further 
implementations).

> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> At a minimum we thought of:
> 1. Handling TPM Localities in I2C is different

It turned out not to be that different in the end, see the code 
mentioned above and my comment here: 
https://patchwork.kernel.org/cover/11049365/

> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core

That is completely optional, so there is no need to implement it in the 
beginning. Also, do you expect a huge benefit from that functionality? 
Are bit flips that much more likely on I2C compared to SPI, which has no 
CRC at all, but still works fine?

> 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors

Right, that seems similar to the cr50 issues 
(https://lkml.org/lkml/2019/7/17/677), so there should probably be a 
similar way to do it.

> 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry)

Optimizations are always welcome, but I'd expect basic communication to 
work already with the current code (though maybe not as efficiently as 
possible).

> Besides this, during development we might encounter additional differences between SPI and I2C.
> 
> We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window.
> 
> Regards,
> Eyal.

RE: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Benoit HOUYERE]

Hi Alexander, Jarkko and Eyal,

A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago.

https://patchwork.kernel.org/patch/8628681/

At the time, we have had two concerns :
	1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver.
	2) Lots changing  was already provided by tpm_tis_spi.c on 4.8.

That's why Tpm_tis_i2c.c has been postponed.

Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august).

Best Regards,

Benoit



-----Original Message-----
From: linux-integrity-owner@vger.kernel.org <linux-integrity-owner@vger.kernel.org> On Behalf Of Alexander Steffen
Sent: jeudi 18 juillet 2019 19:10
To: Eyal.Cohen@nuvoton.com; jarkko.sakkinen@linux.intel.com; tmaimon77@gmail.com
Cc: oshrialkoby85@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com; peterhuewe@gmx.de; jgg@ziepe.ca; arnd@arndb.de; gregkh@linuxfoundation.org; oshri.alkoby@nuvoton.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; Dan.Morav@nuvoton.com; oren.tanami@nuvoton.com
Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
> Hi Jarkko and Alexander,
> 
> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.

Great :) In the meantime, I've done some experiments creating an I2C driver based on tpm_tis_core, see https://patchwork.kernel.org/patch/11049363/ Please have a look at that and provide your feedback (and/or use it as a basis for further implementations).

> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> At a minimum we thought of:
> 1. Handling TPM Localities in I2C is different

It turned out not to be that different in the end, see the code mentioned above and my comment here: 
https://patchwork.kernel.org/cover/11049365/

> 2. Handling I2C CRC - relevant only to I2C bus hence not supported 
> today by TIS core

That is completely optional, so there is no need to implement it in the beginning. Also, do you expect a huge benefit from that functionality? 
Are bit flips that much more likely on I2C compared to SPI, which has no CRC at all, but still works fine?

> 3. Handling Chip specific issues, since I2C implementation might be 
> slightly different across the various TPM vendors

Right, that seems similar to the cr50 issues (https://lkml.org/lkml/2019/7/17/677), so there should probably be a similar way to do it.

> 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according 
> the TCG Device Driver Guide (optimization on TPM_STS access and 
> send/recv retry)

Optimizations are always welcome, but I'd expect basic communication to work already with the current code (though maybe not as efficiently as possible).

> Besides this, during development we might encounter additional differences between SPI and I2C.
> 
> We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window.
> 
> Regards,
> Eyal.

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Alexander Steffen]

Hi Benoit,

good to see you're still around.

On 30.07.2019 10:39, Benoit HOUYERE wrote:
> Hi Alexander, Jarkko and Eyal,
> 
> A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago.
> 
> https://patchwork.kernel.org/patch/8628681/

Thanks for mentioning this. I forgot it exists, since it was still on 
the old mailing list.

> At the time, we have had two concerns :
> 	1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver.
> 	2) Lots changing  was already provided by tpm_tis_spi.c on 4.8.
> 
> That's why Tpm_tis_i2c.c has been postponed.
> 
> Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august).

Could you run your tests against the simple implementation that I posted 
a while ago (https://patchwork.kernel.org/cover/11049365/) and provide 
your feedback? Since it is already based on the current tpm_tis_core, it 
is probably easier to integrate necessary changes there.

By the way, has it gotten any easier in the meantime to get hold of your 
TPMs to use them for kernel tests?

Alexander

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Oshri Alkobi]

On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen
<Alexander.Steffen@infineon.com> wrote:
>
> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
> > Hi Jarkko and Alexander,
> >
> > We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
>
> Great :) In the meantime, I've done some experiments creating an I2C
> driver based on tpm_tis_core, see
> https://patchwork.kernel.org/patch/11049363/ Please have a look at that
> and provide your feedback (and/or use it as a basis for further
> implementations).
>

Sorry for the late response.

Thanks Alexander, indeed it looks much simpler.
I've checked it with Nuvoton's TPM - basic TPM commands work, I only
had to remove the first msg from the read/write I2C transmitting
(from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in
a sequence.
Actually it is more efficient to set TPM_LOC_SEL only before locality
check/request/relinquish - it is sticky.
I still didn't manage to work with interrupts, will debug it.

We weren't aware to the implementation of Christophe/ST which looks
good and can be complement to yours.
If no one is currently working on that, we can prepare a new patch
that is based on both.
Please let us know.

> > However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> > At a minimum we thought of:
> > 1. Handling TPM Localities in I2C is different
>
> It turned out not to be that different in the end, see the code
> mentioned above and my comment here:
> https://patchwork.kernel.org/cover/11049365/
>
> > 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
>
> That is completely optional, so there is no need to implement it in the
> beginning. Also, do you expect a huge benefit from that functionality?
> Are bit flips that much more likely on I2C compared to SPI, which has no
> CRC at all, but still works fine?
>

I2C is noisy bus with potentially more devices with larger variety
than SPI. I2C may have more than one master and may have collisions
and/or arbitration.
Still we can start w/o CRC for the first stage and add it later.
BTW, Christophe already did most of the work
(https://patchwork.kernel.org/patch/8628661/)

> > 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors
>
> Right, that seems similar to the cr50 issues
> (https://lkml.org/lkml/2019/7/17/677), so there should probably be a
> similar way to do it.

Got it. We hope things will work for us without having another driver,
but it's an option.

>
> > 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry)
>
> Optimizations are always welcome, but I'd expect basic communication to
> work already with the current code (though maybe not as efficiently as
> possible).
>
> > Besides this, during development we might encounter additional differences between SPI and I2C.
> >
> > We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window.
> >
> > Regards,
> > Eyal.

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Alexander Steffen]

On 15.08.2019 19:03, Oshri Alkobi wrote:
> On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen
> <Alexander.Steffen@infineon.com> wrote:
>>
>> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
>>> Hi Jarkko and Alexander,
>>>
>>> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
>>
>> Great :) In the meantime, I've done some experiments creating an I2C
>> driver based on tpm_tis_core, see
>> https://patchwork.kernel.org/patch/11049363/ Please have a look at that
>> and provide your feedback (and/or use it as a basis for further
>> implementations).
>>
> 
> Sorry for the late response.
> 
> Thanks Alexander, indeed it looks much simpler.
> I've checked it with Nuvoton's TPM - basic TPM commands work

Nice :)

> I only
> had to remove the first msg from the read/write I2C transmitting
> (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in
> a sequence.
> Actually it is more efficient to set TPM_LOC_SEL only before locality
> check/request/relinquish - it is sticky.

There is one problem though: Do we assume that only the kernel driver 
will communicate with the TPM or might there be something else that also 
talks to the TPM?

If it is only the kernel driver, we could probably skip setting 
TPM_LOC_SEL at all, since it defaults to 0 and the driver will never use 
anything else. If something else does its own I2C communication with the 
TPM, it might write different values to TPM_LOC_SEL at any time, causing 
the kernel driver to use a different locality than intended. This was 
the reason I always set TPM_LOC_SEL within the same transaction.

> I still didn't manage to work with interrupts, will debug it.

Interrupt support might be broken in general at the moment, see this 
thread: https://www.spinics.net/lists/linux-integrity/msg08663.html

> We weren't aware to the implementation of Christophe/ST which looks
> good and can be complement to yours.
> If no one is currently working on that, we can prepare a new patch
> that is based on both.
> Please let us know.

I won't have the time to do anything, at least for the next two weeks, 
so feel free to pick it up.

>>> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
>>> At a minimum we thought of:
>>> 1. Handling TPM Localities in I2C is different
>>
>> It turned out not to be that different in the end, see the code
>> mentioned above and my comment here:
>> https://patchwork.kernel.org/cover/11049365/
>>
>>> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
>>
>> That is completely optional, so there is no need to implement it in the
>> beginning. Also, do you expect a huge benefit from that functionality?
>> Are bit flips that much more likely on I2C compared to SPI, which has no
>> CRC at all, but still works fine?
>>
> 
> I2C is noisy bus with potentially more devices with larger variety
> than SPI. I2C may have more than one master and may have collisions
> and/or arbitration.

If multi-master usage is a concern, there are probably a lot more places 
in the driver that need to be adapted to deal with concurrent 
access/data corruption. For now, I'd assume a single master, similar to SPI.

Alexander

Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Oshri Alkobi]

On Fri, Aug 16, 2019 at 7:12 PM Alexander Steffen
<Alexander.Steffen@infineon.com> wrote:
>
> On 15.08.2019 19:03, Oshri Alkobi wrote:
> > On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen
> > <Alexander.Steffen@infineon.com> wrote:
> >>
> >> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
> >>> Hi Jarkko and Alexander,
> >>>
> >>> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
> >>
> >> Great :) In the meantime, I've done some experiments creating an I2C
> >> driver based on tpm_tis_core, see
> >> https://patchwork.kernel.org/patch/11049363/ Please have a look at that
> >> and provide your feedback (and/or use it as a basis for further
> >> implementations).
> >>
> >
> > Sorry for the late response.
> >
> > Thanks Alexander, indeed it looks much simpler.
> > I've checked it with Nuvoton's TPM - basic TPM commands work
>
> Nice :)
>
> > I only
> > had to remove the first msg from the read/write I2C transmitting
> > (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in
> > a sequence.
> > Actually it is more efficient to set TPM_LOC_SEL only before locality
> > check/request/relinquish - it is sticky.
>
> There is one problem though: Do we assume that only the kernel driver
> will communicate with the TPM or might there be something else that also
> talks to the TPM?
>
> If it is only the kernel driver, we could probably skip setting
> TPM_LOC_SEL at all, since it defaults to 0 and the driver will never use
> anything else. If something else does its own I2C communication with the
> TPM, it might write different values to TPM_LOC_SEL at any time, causing
> the kernel driver to use a different locality than intended. This was
> the reason I always set TPM_LOC_SEL within the same transaction.
>
> > I still didn't manage to work with interrupts, will debug it.
>
> Interrupt support might be broken in general at the moment, see this
> thread: https://www.spinics.net/lists/linux-integrity/msg08663.html
>
> > We weren't aware to the implementation of Christophe/ST which looks
> > good and can be complement to yours.
> > If no one is currently working on that, we can prepare a new patch
> > that is based on both.
> > Please let us know.
>
> I won't have the time to do anything, at least for the next two weeks,
> so feel free to pick it up.
>

Great, we will start working on it.

> >>> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> >>> At a minimum we thought of:
> >>> 1. Handling TPM Localities in I2C is different
> >>
> >> It turned out not to be that different in the end, see the code
> >> mentioned above and my comment here:
> >> https://patchwork.kernel.org/cover/11049365/
> >>
> >>> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
> >>
> >> That is completely optional, so there is no need to implement it in the
> >> beginning. Also, do you expect a huge benefit from that functionality?
> >> Are bit flips that much more likely on I2C compared to SPI, which has no
> >> CRC at all, but still works fine?
> >>
> >
> > I2C is noisy bus with potentially more devices with larger variety
> > than SPI. I2C may have more than one master and may have collisions
> > and/or arbitration.
>
> If multi-master usage is a concern, there are probably a lot more places
> in the driver that need to be adapted to deal with concurrent
> access/data corruption. For now, I'd assume a single master, similar to SPI.
>
> Alexander

Oshri

RE: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

[Benoit HOUYERE]

Hi Steffen,

We have performed test against your simple implementation. For basic test it was ok, however for stress test, your implementation does not take in account NACK and it failed.

In PTP document, in chapter7.2.2.1.2 (Register write with address NACK), it indicates : "it's a good practice to repeat the current cycle using the correct I2C device address".

In other implementation, 
https://patchwork.kernel.org/patch/8628681/

tpm_tis_i2c_read_bytes and tpm_tis_i2c_write_bytes handle NACK with a for loop.

+	for (i = 0; i < TPM_RETRY && ret < 0; i++) {
+		tpm_tis_i2c_sleep_guard_time(phy, TPM_I2C_SEND);
+		ret = i2c_master_send(phy->client, &i2c_reg, 1);
+		mod_timer(&phy->guard_timer, phy->guard_time);
+	}
+
+	if (ret < 0)
+		goto exit;
+
+	ret = -1;
+	for (i = 0; i < TPM_RETRY && ret < 0; i++) {
+		tpm_tis_i2c_sleep_guard_time(phy, TPM_I2C_RECV);
+		ret = i2c_master_recv(phy->client, result, len * size);
+		mod_timer(&phy->guard_timer, phy->guard_time);
+	}

I think that we should implement it before to include tpm_tis_i2c.c officially.

Thanks in advance,

Best Regards,

Benoit

-----Original Message-----
From: Alexander Steffen <Alexander.Steffen@infineon.com> 
Sent: mardi 30 juillet 2019 19:42
To: Benoit HOUYERE <benoit.houyere@st.com>; Eyal.Cohen@nuvoton.com; jarkko.sakkinen@linux.intel.com; tmaimon77@gmail.com
Cc: oshrialkoby85@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com; peterhuewe@gmx.de; jgg@ziepe.ca; arnd@arndb.de; gregkh@linuxfoundation.org; oshri.alkoby@nuvoton.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; Dan.Morav@nuvoton.com; oren.tanami@nuvoton.com; Christophe Ricard (christophe.ricard@gmail.com) <christophe.ricard@gmail.com>; Elena WILLIS <elena.willis@st.com>; Olivier COLLART <olivier.collart@st.com>
Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp

Hi Benoit,

good to see you're still around.

On 30.07.2019 10:39, Benoit HOUYERE wrote:
> Hi Alexander, Jarkko and Eyal,
> 
> A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago.
> 
> https://patchwork.kernel.org/patch/8628681/

Thanks for mentioning this. I forgot it exists, since it was still on the old mailing list.

> At the time, we have had two concerns :
> 	1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver.
> 	2) Lots changing  was already provided by tpm_tis_spi.c on 4.8.
> 
> That's why Tpm_tis_i2c.c has been postponed.
> 
> Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august).

Could you run your tests against the simple implementation that I posted a while ago (https://patchwork.kernel.org/cover/11049365/) and provide your feedback? Since it is already based on the current tpm_tis_core, it is probably easier to integrate necessary changes there.

By the way, has it gotten any easier in the meantime to get hold of your TPMs to use them for kernel tests?

Alexander