[PATCH v3 0/2] tpm_tis_i2c

[PATCH v3 0/2] tpm_tis_i2c

[Alexander Steffen]

Johannes asked me to take over this series. I've decided to split off the
later patches from this series, since it was suggested to split those changes
into multiple patches and I don't want to blow up this series with too many
changes that are not strictly necessary for the I2C driver to work correctly.
So let's first concentrate on getting the basic functionality right, then we
can work on additional improvements.

I've addressed all review comments for patch 1/2.

Patch 2/2 is unmodified.

Johannes Holland (2):
  tpm: Add tpm_tis_i2c backend for tpm_tis_core
  tpm: Add tpm_tis_verify_crc to the tpm_tis_phy_ops protocol layer

 drivers/char/tpm/Kconfig        |  12 +
 drivers/char/tpm/Makefile       |   1 +
 drivers/char/tpm/tpm_tis_core.c |  14 ++
 drivers/char/tpm/tpm_tis_core.h |  10 +
 drivers/char/tpm/tpm_tis_i2c.c  | 405 ++++++++++++++++++++++++++++++++
 5 files changed, 442 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_tis_i2c.c

-- 
2.25.1

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

[Alexander Steffen]

From: Johannes Holland <johannes.holland@infineon.com>

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

Co-developed-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Co-developed-by: Amir Mizinski <amirmizi6@gmail.com>
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
Changelog:
 * v3:
   * Document address_to_register function
   * Add tpm_tis_i2c prefix to address_to_register
   * Add #ifdef CONFIG_OF to of_tis_i2c_match
   * Fix typos
 * v2:
   * move CCs from copyright comment to commit message
   * fix an unchecked return code
 
As discussed, 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%.

 drivers/char/tpm/Kconfig       |  12 ++
 drivers/char/tpm/Makefile      |   1 +
 drivers/char/tpm/tpm_tis_i2c.c | 377 +++++++++++++++++++++++++++++++++
 3 files changed, 390 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..5375ebf57007
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -0,0 +1,377 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2014-2021 Nuvoton Technology corporation
+ * Copyright (C) 2019-2022 Infineon Technologies AG
+ *
+ * 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);
+}
+
+/*
+ * tpm_tis_core uses the register addresses as defined in Table 19 "Allocation
+ * of Register Space for FIFO TPM Access" of the TCG PC Client PTP
+ * Specification. In order for this code to work together with tpm_tis_core,
+ * those addresses need to mapped to the registers defined for I2C TPMs in
+ * Table 51 "I2C-TPM Register Overview".
+ *
+ * For most addresses this can be done by simply stripping off the locality
+ * information from the address. A few addresses need to be mapped explicitly,
+ * since the corresponding I2C registers have been moved around. TPM_LOC_SEL is
+ * only defined for I2C TPMs and is also mapped explicitly here to distinguish
+ * it from TPM_ACCESS(0).
+ *
+ * Locality information is ignored, since this driver assumes exclusive access
+ * to the TPM and always uses locality 0.
+ */
+static u8 tpm_tis_i2c_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 zero_mask;
+	u32 value;
+
+	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(0) & 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 = tpm_tis_i2c_address_to_register(addr);
+	int i = 0;
+	int ret;
+
+	do {
+		/* write register */
+		msg.len = sizeof(reg);
+		msg.buf = &reg;
+		msg.flags = 0;
+		ret = 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;
+		ret = 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 = tpm_tis_i2c_address_to_register(addr);
+	int ret;
+
+	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;
+	ret = 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);
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_tis_i2c_match[] = {
+	{ .compatible = "infineon,slb9673", },
+	{ .compatible = "tcg,tpm_tis-i2c", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
+#endif
+
+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.25.1

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

[Krzysztof Kozlowski]

On 20/05/2022 19:24, Alexander Steffen wrote:
>
> +MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_tis_i2c_match[] = {
> +	{ .compatible = "infineon,slb9673", },
> +	{ .compatible = "tcg,tpm_tis-i2c", },

Please run checkpatch on your patches. You add undocumented compatibles.

Without bindings, new compatibles and properties cannot be accepted, so NAK.

Best regards,
Krzysztof

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

[Alexander Steffen]

On 22.05.22 10:30, Krzysztof Kozlowski wrote:
> On 20/05/2022 19:24, Alexander Steffen wrote:
>>
>> +MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_tis_i2c_match[] = {
>> +     { .compatible = "infineon,slb9673", },
>> +     { .compatible = "tcg,tpm_tis-i2c", },
> 
> Please run checkpatch on your patches. You add undocumented compatibles.

Sorry, the old infrastructure I had to do that automatically is not in 
place at the moment, so it slipped through.

> Without bindings, new compatibles and properties cannot be accepted, so NAK.

Could you be more specific as to what the correct solution is here? 
Usually, I'd just look at what the existing code does, but that is a 
little messy:



* socionext,synquacer-tpm-mmio is documented only in 
Documentation/devicetree/bindings/trivial-devices.yaml

* nuvoton,npct601 is documented in trivial-devices.yaml and is also 
mentioned in Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt

* nuvoton,npct650 is only mentioned in tpm-i2c.txt, but appears nowhere 
in the code

* infineon,tpm_i2c_infineon appears only in tpm_i2c_infineon.c, but is 
documented nowhere

* tpm_tis_spi_main.c has all its compatibles documented in 
tpm_tis_spi.txt, except google,cr50, which is documented in 
google,cr50.txt, even though it has the same properties

* tpm_tis_i2c_cr50.c uses the exact same google,cr50, even though that 
is explicitly documented as a device "on SPI Bus" and lists 
spi-max-frequency as one of its required properties, which does not make 
any sense for an I2C device

* According to the feedback in 
https://patchwork.kernel.org/project/linux-integrity/patch/20220404081835.495-4-johannes.holland@infineon.com/#24801807, 
the text format, that is currently used everywhere in 
Documentation/devicetree/bindings/security/tpm/, is deprecated anyway 
and should be replaced by YAML



So would you be okay with just adding the compatibles from tpm_tis_i2c.c 
to trivial-devices.yaml, so that checkpatch does not complain anymore, 
and leave the cleanup of the mess above for later?



Kind regards

Alexander

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

[Krzysztof Kozlowski]

On 23/05/2022 10:55, Alexander Steffen wrote:
> On 22.05.22 10:30, Krzysztof Kozlowski wrote:
>> On 20/05/2022 19:24, Alexander Steffen wrote:
>>>
>>> +MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
>>> +
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id of_tis_i2c_match[] = {
>>> +     { .compatible = "infineon,slb9673", },
>>> +     { .compatible = "tcg,tpm_tis-i2c", },
>>
>> Please run checkpatch on your patches. You add undocumented compatibles.
> 
> Sorry, the old infrastructure I had to do that automatically is not in 
> place at the moment, so it slipped through.
> 
>> Without bindings, new compatibles and properties cannot be accepted, so NAK.
> 
> Could you be more specific as to what the correct solution is here? 
> Usually, I'd just look at what the existing code does, but that is a 
> little messy:
> 
> 
> 
> * socionext,synquacer-tpm-mmio is documented only in 
> Documentation/devicetree/bindings/trivial-devices.yaml
> 
> * nuvoton,npct601 is documented in trivial-devices.yaml and is also 
> mentioned in Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt
> 
> * nuvoton,npct650 is only mentioned in tpm-i2c.txt, but appears nowhere 
> in the code
> 
> * infineon,tpm_i2c_infineon appears only in tpm_i2c_infineon.c, but is 
> documented nowhere
> 
> * tpm_tis_spi_main.c has all its compatibles documented in 
> tpm_tis_spi.txt, except google,cr50, which is documented in 
> google,cr50.txt, even though it has the same properties
> 
> * tpm_tis_i2c_cr50.c uses the exact same google,cr50, even though that 
> is explicitly documented as a device "on SPI Bus" and lists 
> spi-max-frequency as one of its required properties, which does not make 
> any sense for an I2C device
> 
> * According to the feedback in 
> https://patchwork.kernel.org/project/linux-integrity/patch/20220404081835.495-4-johannes.holland@infineon.com/#24801807, 
> the text format, that is currently used everywhere in 
> Documentation/devicetree/bindings/security/tpm/, is deprecated anyway 
> and should be replaced by YAML
> 
> 
> 
> So would you be okay with just adding the compatibles from tpm_tis_i2c.c 
> to trivial-devices.yaml, so that checkpatch does not complain anymore, 
> and leave the cleanup of the mess above for later?

To trivial-devices you should add only bindings really trivial devices,
which do not have any other properties, even when the bindings are
finished. This means you entirely fully the hardware and still have only
reg+compatible.

If this device fits such case - no other hardware properties than reg -
then, feel free to document it in trivial-devices. However I am not sure
that TPM devices are that trivial... For example tpm-i2c.txt defines
also interrupts and label.

If the device is not trivial, it should be documented in bindings,
either dedicated or some existing ones.


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 23/05/2022 11:32, Krzysztof Kozlowski wrote:
> On 23/05/2022 10:55, Alexander Steffen wrote:
>> On 22.05.22 10:30, Krzysztof Kozlowski wrote:
>>> On 20/05/2022 19:24, Alexander Steffen wrote:
>>>>
>>>> +MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
>>>> +
>>>> +#ifdef CONFIG_OF
>>>> +static const struct of_device_id of_tis_i2c_match[] = {
>>>> +     { .compatible = "infineon,slb9673", },
>>>> +     { .compatible = "tcg,tpm_tis-i2c", },
>>>
>>> Please run checkpatch on your patches. You add undocumented compatibles.
>>
>> Sorry, the old infrastructure I had to do that automatically is not in 
>> place at the moment, so it slipped through.
>>
>>> Without bindings, new compatibles and properties cannot be accepted, so NAK.
>>
>> Could you be more specific as to what the correct solution is here? 
>> Usually, I'd just look at what the existing code does, but that is a 
>> little messy:
>>
>>
>>
>> * socionext,synquacer-tpm-mmio is documented only in 
>> Documentation/devicetree/bindings/trivial-devices.yaml
>>
>> * nuvoton,npct601 is documented in trivial-devices.yaml and is also 
>> mentioned in Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt
>>
>> * nuvoton,npct650 is only mentioned in tpm-i2c.txt, but appears nowhere 
>> in the code
>>
>> * infineon,tpm_i2c_infineon appears only in tpm_i2c_infineon.c, but is 
>> documented nowhere
>>
>> * tpm_tis_spi_main.c has all its compatibles documented in 
>> tpm_tis_spi.txt, except google,cr50, which is documented in 
>> google,cr50.txt, even though it has the same properties
>>
>> * tpm_tis_i2c_cr50.c uses the exact same google,cr50, even though that 
>> is explicitly documented as a device "on SPI Bus" and lists 
>> spi-max-frequency as one of its required properties, which does not make 
>> any sense for an I2C device
>>
>> * According to the feedback in 
>> https://patchwork.kernel.org/project/linux-integrity/patch/20220404081835.495-4-johannes.holland@infineon.com/#24801807, 
>> the text format, that is currently used everywhere in 
>> Documentation/devicetree/bindings/security/tpm/, is deprecated anyway 
>> and should be replaced by YAML
>>
>>
>>
>> So would you be okay with just adding the compatibles from tpm_tis_i2c.c 
>> to trivial-devices.yaml, so that checkpatch does not complain anymore, 
>> and leave the cleanup of the mess above for later?
> 
> To trivial-devices you should add only bindings really trivial devices,
> which do not have any other properties, even when the bindings are
> finished. This means you entirely fully the hardware and still have only
> reg+compatible.
> 
> If this device fits such case - no other hardware properties than reg -
> then, feel free to document it in trivial-devices. However I am not sure
> that TPM devices are that trivial... For example tpm-i2c.txt defines
> also interrupts and label.
> 
> If the device is not trivial, it should be documented in bindings,
> either dedicated or some existing ones.

It seems I lost few words in my message, so let me write it again, but
maybe closer to English:

To trivial-devices you should add only bindings of really trivial
devices, which do not have any other properties, even when the bindings
are finished. This means you describe fully the hardware and still have
only reg+compatible.

If this device fits such case - no other hardware properties than reg -
then, feel free to document it in trivial-devices. However I am not sure
that TPM devices are that trivial... For example tpm-i2c.txt defines
also interrupts and label.

If the device is not trivial, it should be documented in bindings,
either dedicated or some existing ones.

Best regards,
Krzysztof

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

[Alexander Steffen]

On 23.05.22 11:44, Krzysztof Kozlowski wrote:
> On 23/05/2022 11:32, Krzysztof Kozlowski wrote:
>> On 23/05/2022 10:55, Alexander Steffen wrote:
>>> On 22.05.22 10:30, Krzysztof Kozlowski wrote:
>>>> Without bindings, new compatibles and properties cannot be accepted, so NAK.
>>>
>>> Could you be more specific as to what the correct solution is here?
>>> Usually, I'd just look at what the existing code does, but that is a
>>> little messy:
>>>
>>>
>>>
>>> * socionext,synquacer-tpm-mmio is documented only in
>>> Documentation/devicetree/bindings/trivial-devices.yaml
>>>
>>> * nuvoton,npct601 is documented in trivial-devices.yaml and is also
>>> mentioned in Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt
>>>
>>> * nuvoton,npct650 is only mentioned in tpm-i2c.txt, but appears nowhere
>>> in the code
>>>
>>> * infineon,tpm_i2c_infineon appears only in tpm_i2c_infineon.c, but is
>>> documented nowhere
>>>
>>> * tpm_tis_spi_main.c has all its compatibles documented in
>>> tpm_tis_spi.txt, except google,cr50, which is documented in
>>> google,cr50.txt, even though it has the same properties
>>>
>>> * tpm_tis_i2c_cr50.c uses the exact same google,cr50, even though that
>>> is explicitly documented as a device "on SPI Bus" and lists
>>> spi-max-frequency as one of its required properties, which does not make
>>> any sense for an I2C device
>>>
>>> * According to the feedback in
>>> https://patchwork.kernel.org/project/linux-integrity/patch/20220404081835.495-4-johannes.holland@infineon.com/#24801807,
>>> the text format, that is currently used everywhere in
>>> Documentation/devicetree/bindings/security/tpm/, is deprecated anyway
>>> and should be replaced by YAML
>>>
>>>
>>>
>>> So would you be okay with just adding the compatibles from tpm_tis_i2c.c
>>> to trivial-devices.yaml, so that checkpatch does not complain anymore,
>>> and leave the cleanup of the mess above for later?
> 
> To trivial-devices you should add only bindings of really trivial
> devices, which do not have any other properties, even when the bindings
> are finished. This means you describe fully the hardware and still have
> only reg+compatible.
> 
> If this device fits such case - no other hardware properties than reg -
> then, feel free to document it in trivial-devices. However I am not sure
> that TPM devices are that trivial... For example tpm-i2c.txt defines
> also interrupts and label.
> 
> If the device is not trivial, it should be documented in bindings,
> either dedicated or some existing ones.

Ok, let's see whether I understood that correctly:



I think, in general, TPMs are trivial devices: They sit on the I2C or 
SPI bus waiting for commands, but don't do much else. They might have 
TPM-specific properties, like whether they implement the 1.2 or 2.0 
command set, but we don't encode those in the device tree, since the 
driver tries to detect the available functionality dynamically (which 
makes sense, since some TPMs can be upgraded from 1.2 to 2.0, so that is 
not a static property of the device). Other properties, such as the 
maximum SPI frequency, are not TPM-specific, but apply to every SPI 
device and might be limited by either the SPI device itself or the SPI 
controller (or the user, wishing to run at lower frequencies, for 
whatever reason).



Looking at the code, there are some TPM-specific properties in use 
though: There is the powered-while-suspended flag, which only the TPM 
driver looks at (in drivers/char/tpm/eventlog/of.c). It is not specific 
to a single TPM (a single compatible string), but can be set for all the 
TPMs. Also, the linux,sml-* properties might be TPM-specific, though 
they get set in arch/powerpc/kernel/prom_init.c to communicate some 
information to the TPM driver. And there is lpcpd-gpios, which is only 
used by st33zp24.



Now the purpose of trivial-devices.yaml is to specify a schema of valid 
device definitions. It only allows the properties reg, interrupts and 
spi-max-frequency in addition to the compatible strings. So, strictly 
speaking, none of the TPMs should be listed there, since all the TPMs 
can, in theory, use the powered-while-suspended flag, which is not 
allowed by the schema. With other properties the schema does not seem to 
be too strict, since it applies to both I2C and SPI devices, but allows 
the spi-max-frequency property, even though that does not make sense for 
I2C devices.



So the correct solution could be this: Replace all the text files in 
Documentation/devicetree/bindings/security/tpm/ with a single 
trivial-tpms.yaml (similar to trivial-devices.yaml) and also move all 
the TPMs from trivial-devices.yaml there. This allows to properly 
document the powered-while-suspended flag and other generic TPM 
properties. st33zp24 should get its own YAML, to document lpcpd-gpios, 
that is only used by this driver. I'm not quite sure what to do with 
ibmvtpm.txt, since that seems to document several properties, that are 
not referenced in the TPM driver at all but instead get used by some 
scsi driver (e.g. ibm,loc-code), so I'd probably ignore it for now. What 
do you think?



As for tpm_tis_i2c, if there are no other objections to its current 
state, I'd add its compatible strings to trivial-devices.yaml for now, 
then do the cleanup as described above later. It does not make the code 
more wrong, since trivial-devices.yaml already contains some TPM 
devices, that are very similar to what tpm_tis_i2c now supports (i.e. 
that also don't have special properties), but allows for more time to do 
the cleanup properly, without holding up tpm_tis_i2c.



Kind regards

Alexander

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

[Alexander Steffen]

On 23.05.22 11:44, Krzysztof Kozlowski wrote:
> On 23/05/2022 11:32, Krzysztof Kozlowski wrote:
>> On 23/05/2022 10:55, Alexander Steffen wrote:
>>> On 22.05.22 10:30, Krzysztof Kozlowski wrote:
>>>> Without bindings, new compatibles and properties cannot be accepted, so NAK.
>>>
>>> Could you be more specific as to what the correct solution is here?
>>> Usually, I'd just look at what the existing code does, but that is a
>>> little messy:
>>>
>>>
>>>
>>> * socionext,synquacer-tpm-mmio is documented only in
>>> Documentation/devicetree/bindings/trivial-devices.yaml
>>>
>>> * nuvoton,npct601 is documented in trivial-devices.yaml and is also
>>> mentioned in Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt
>>>
>>> * nuvoton,npct650 is only mentioned in tpm-i2c.txt, but appears nowhere
>>> in the code
>>>
>>> * infineon,tpm_i2c_infineon appears only in tpm_i2c_infineon.c, but is
>>> documented nowhere
>>>
>>> * tpm_tis_spi_main.c has all its compatibles documented in
>>> tpm_tis_spi.txt, except google,cr50, which is documented in
>>> google,cr50.txt, even though it has the same properties
>>>
>>> * tpm_tis_i2c_cr50.c uses the exact same google,cr50, even though that
>>> is explicitly documented as a device "on SPI Bus" and lists
>>> spi-max-frequency as one of its required properties, which does not make
>>> any sense for an I2C device
>>>
>>> * According to the feedback in
>>> https://patchwork.kernel.org/project/linux-integrity/patch/20220404081835.495-4-johannes.holland@infineon.com/#24801807,
>>> the text format, that is currently used everywhere in
>>> Documentation/devicetree/bindings/security/tpm/, is deprecated anyway
>>> and should be replaced by YAML
>>>
>>>
>>>
>>> So would you be okay with just adding the compatibles from tpm_tis_i2c.c
>>> to trivial-devices.yaml, so that checkpatch does not complain anymore,
>>> and leave the cleanup of the mess above for later?
>>
> To trivial-devices you should add only bindings of really trivial
> devices, which do not have any other properties, even when the bindings
> are finished. This means you describe fully the hardware and still have
> only reg+compatible.
> 
> If this device fits such case - no other hardware properties than reg -
> then, feel free to document it in trivial-devices. However I am not sure
> that TPM devices are that trivial... For example tpm-i2c.txt defines
> also interrupts and label.
> 
> If the device is not trivial, it should be documented in bindings,
> either dedicated or some existing ones.

Ok, let's see whether I understood that correctly:



I think, in general, TPMs are trivial devices: They sit on the I2C or 
SPI bus waiting for commands, but don't do much else. They might have 
TPM-specific properties, like whether they implement the 1.2 or 2.0 
command set, but we don't encode those in the device tree, since the 
driver tries to detect the available functionality dynamically (which 
makes sense, since some TPMs can be upgraded from 1.2 to 2.0, so that is 
not a static property of the device). Other properties, such as the 
maximum SPI frequency, are not TPM-specific, but apply to every SPI 
device and might be limited by either the SPI device itself or the SPI 
controller (or the user, wishing to run at lower frequencies, for 
whatever reason).



Looking at the code, there are some TPM-specific properties in use 
though: There is the powered-while-suspended flag, which only the TPM 
driver looks at (in drivers/char/tpm/eventlog/of.c). It is not specific 
to a single TPM (a single compatible string), but can be set for all the 
TPMs. Also, the linux,sml-* properties might be TPM-specific, though 
they get set in arch/powerpc/kernel/prom_init.c to communicate some 
information to the TPM driver. And there is lpcpd-gpios, which is only 
used by st33zp24.



Now the purpose of trivial-devices.yaml is to specify a schema of valid 
device definitions. It only allows the properties reg, interrupts and 
spi-max-frequency in addition to the compatible strings. So, strictly 
speaking, none of the TPMs should be listed there, since all the TPMs 
can, in theory, use the powered-while-suspended flag, which is not 
allowed by the schema. With other properties the schema does not seem to 
be too strict, since it applies to both I2C and SPI devices, but allows 
the spi-max-frequency property, even though that does not make sense for 
I2C devices.



So the correct solution could be this: Replace all the text files in 
Documentation/devicetree/bindings/security/tpm/ with a single 
trivial-tpms.yaml (similar to trivial-devices.yaml) and also move all 
the TPMs from trivial-devices.yaml there. This allows to properly 
document the powered-while-suspended flag and other generic TPM 
properties. st33zp24 should get its own YAML, to document lpcpd-gpios, 
that is only used by this driver. I'm not quite sure what to do with 
ibmvtpm.txt, since that seems to document several properties, that are 
not referenced in the TPM driver at all but instead get used by some 
scsi driver (e.g. ibm,loc-code), so I'd probably ignore it for now. What 
do you think?



As for tpm_tis_i2c, if there are no other objections to its current 
state, I'd add its compatible strings to trivial-devices.yaml for now, 
then do the cleanup as described above later. It does not make the code 
more wrong, since trivial-devices.yaml already contains some TPM 
devices, that are very similar to what tpm_tis_i2c now supports (i.e. 
that also don't have special properties), but allows for more time to do 
the cleanup properly, without holding up tpm_tis_i2c.



Kind regards

Alexander

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

[Jarkko Sakkinen]

On Fri, May 20, 2022 at 07:24:21PM +0200, Alexander Steffen wrote:
> From: Johannes Holland <johannes.holland@infineon.com>
> 
> 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).
> 
> Co-developed-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Co-developed-by: Amir Mizinski <amirmizi6@gmail.com>
> Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---
> Changelog:
>  * v3:
>    * Document address_to_register function
>    * Add tpm_tis_i2c prefix to address_to_register
>    * Add #ifdef CONFIG_OF to of_tis_i2c_match
>    * Fix typos
>  * v2:
>    * move CCs from copyright comment to commit message
>    * fix an unchecked return code
>  
> As discussed, 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%.
> 
>  drivers/char/tpm/Kconfig       |  12 ++
>  drivers/char/tpm/Makefile      |   1 +
>  drivers/char/tpm/tpm_tis_i2c.c | 377 +++++++++++++++++++++++++++++++++
>  3 files changed, 390 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..5375ebf57007
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_tis_i2c.c
> @@ -0,0 +1,377 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2014-2021 Nuvoton Technology corporation
> + * Copyright (C) 2019-2022 Infineon Technologies AG
> + *
> + * 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.

Can you document this at init_guard_time()? E.g.

/*
 * After each I2C operation, the TPM might require the I2C bus master to
 * wait.  The guard time period is vendor-specific and must be read from
 * the TPM_I2C_INTERFACE_CAPABILITY register.
 */
static int init_guard_time(struct tpm_tis_i2c_phy *phy)


> + *
> + * 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);
> +}
> +
> +/*
> + * tpm_tis_core uses the register addresses as defined in Table 19 "Allocation
> + * of Register Space for FIFO TPM Access" of the TCG PC Client PTP
> + * Specification. In order for this code to work together with tpm_tis_core,
> + * those addresses need to mapped to the registers defined for I2C TPMs in
> + * Table 51 "I2C-TPM Register Overview".
> + *
> + * For most addresses this can be done by simply stripping off the locality
> + * information from the address. A few addresses need to be mapped explicitly,
> + * since the corresponding I2C registers have been moved around. TPM_LOC_SEL is
> + * only defined for I2C TPMs and is also mapped explicitly here to distinguish
> + * it from TPM_ACCESS(0).
> + *
> + * Locality information is ignored, since this driver assumes exclusive access
> + * to the TPM and always uses locality 0.
> + */
> +static u8 tpm_tis_i2c_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)

tpm2_i2c_check_read()

> +{
> +	u32 zero_mask;
> +	u32 value;
> +
> +	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(0) & 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;

Why this isn't an error condition?

> +	}
> +
> +	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 = tpm_tis_i2c_address_to_register(addr);
> +	int i = 0;
> +	int ret;
> +
> +	do {
> +		/* write register */
> +		msg.len = sizeof(reg);
> +		msg.buf = &reg;
> +		msg.flags = 0;
> +		ret = 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;
> +		ret = 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);


Couldn't this be instead:

for (i = 0; i < TPM_RETRY; i++) {

?

> +
> +	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 = tpm_tis_i2c_address_to_register(addr);
> +	int ret;
> +
> +	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;
> +	ret = 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)

tpm2_i2c_init_guard_time()

> +{
> +	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);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_tis_i2c_match[] = {
> +	{ .compatible = "infineon,slb9673", },
> +	{ .compatible = "tcg,tpm_tis-i2c", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
> +#endif
> +
> +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.25.1
> 

BR, Jarkko

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

[Alexander Steffen]

From: Johannes Holland <johannes.holland@infineon.com>

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>
Signed-off-by: Alexander Steffen <Alexander.Steffen@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  | 28 ++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

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 6c203f36b8a1..66a5a13cd1df 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -121,6 +121,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,
@@ -188,6 +190,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 5375ebf57007..bfc26c2a4ed6 100644
--- a/drivers/char/tpm/tpm_tis_i2c.c
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -24,6 +24,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"
 
@@ -33,6 +34,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
 
@@ -269,6 +271,24 @@ static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
 	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;
@@ -302,12 +322,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;
 
@@ -332,6 +354,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.25.1

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

[Jarkko Sakkinen]

On Fri, May 20, 2022 at 07:24:22PM +0200, Alexander Steffen wrote:
> From: Johannes Holland <johannes.holland@infineon.com>
> 
> 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>
> Signed-off-by: Alexander Steffen <Alexander.Steffen@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  | 28 ++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+)
> 
> 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 6c203f36b8a1..66a5a13cd1df 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -121,6 +121,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,
> @@ -188,6 +190,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 5375ebf57007..bfc26c2a4ed6 100644
> --- a/drivers/char/tpm/tpm_tis_i2c.c
> +++ b/drivers/char/tpm/tpm_tis_i2c.c
> @@ -24,6 +24,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"
>  
> @@ -33,6 +34,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
>  
> @@ -269,6 +271,24 @@ static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
>  	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;
> @@ -302,12 +322,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;
>  
> @@ -332,6 +354,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.25.1
> 

Should be split to two patches: the hook and the implementation for I2C, as
per https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes

BR, Jarkko