[RFC PATCH 0/2] tpm: Simple implementation of tpm_tis_i2c

[RFC PATCH 0/2] tpm: Simple implementation of tpm_tis_i2c

[Alexander Steffen]

Following the discussion in https://lkml.org/lkml/2019/7/5/303 I went ahead and created what I had in mind, trying to keep it as simple as possible. It is meant more as a starting point than a final implementation, as I'm not sure yet how to best integrate it with the existing tpm_tis_core (hence RFC):

It would probably be a cleaner interface, if locality and register address were two separate parameters. I did not implement it here, since that would have meant touching all the tpm_tis_core-based drivers. Also, I noticed that the kernel only ever uses locality 0 anyway. So maybe it would be even better to remove the locality parameter completely and just hardcode locality to 0 where necessary?

I also did not introduce any abstraction for register addresses, but simply remap the well-known addresses from the old TIS versions (which are used within tpm_tis_core) to the corresponding new addresses for I2C before accessing them. Again, it might be cleaner to have maybe an enum to address registers within tpm_tis_core and then a lookup table for the actual addresses in the low-level drivers?

IRQ support is deactivated for the moment, though it probably would work (or at least fall back to polling without catastrophic failures). But I'm not sure what exactly happens when the IRQ code path in tpm_tis_core accesses TPM_INT_VECTOR, which does not exist for I2C. Those accesses should just get ignored by the TPM, but actually testing it would have been more effort.

Speaking of tests, I only verified that basic communication with the TPM (i.e. reading/writing registers) works. For more tests, more test devices would be really helpful, which is why asked for that (https://lkml.org/lkml/2019/7/17/110).

Alexander Steffen (2):
  tpm: Make implementation of read16/read32/write32 optional
  tpm: Add tpm_tis_i2c backend for tpm_tis_core

 drivers/char/tpm/Kconfig        |  11 ++
 drivers/char/tpm/Makefile       |   1 +
 drivers/char/tpm/tpm_tis_core.h |  41 +++++-
 drivers/char/tpm/tpm_tis_i2c.c  | 233 ++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_tis_spi.c  |  41 ------
 5 files changed, 283 insertions(+), 44 deletions(-)
 create mode 100644 drivers/char/tpm/tpm_tis_i2c.c

-- 
2.17.1

[RFC PATCH 1/2] tpm: Make implementation of read16/read32/write32 optional

[Alexander Steffen]

Only tpm_tis has a faster way to access multiple bytes at once, every other
driver will just fall back to read_bytes/write_bytes. Therefore, move this
common code out of tpm_tis_spi into tpm_tis_core, so that it is
automatically used when low-level drivers do not implement the specialized
methods.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.h | 41 ++++++++++++++++++++++++++++++---
 drivers/char/tpm/tpm_tis_spi.c  | 41 ---------------------------------
 2 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 7337819f5d7b..2c6557b29a1d 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -122,13 +122,37 @@ static inline int tpm_tis_read8(struct tpm_tis_data *data, u32 addr, u8 *result)
 static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
 				 u16 *result)
 {
-	return data->phy_ops->read16(data, addr, result);
+	if (data->phy_ops->read16) {
+		return data->phy_ops->read16(data, addr, result);
+	} else {
+		__le16 result_le;
+		int rc;
+
+		rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
+					       (u8 *)&result_le);
+		if (!rc)
+			*result = le16_to_cpu(result_le);
+
+		return rc;
+	}
 }
 
 static inline int tpm_tis_read32(struct tpm_tis_data *data, u32 addr,
 				 u32 *result)
 {
-	return data->phy_ops->read32(data, addr, result);
+	if (data->phy_ops->read32) {
+		return data->phy_ops->read32(data, addr, result);
+	} else {
+		__le32 result_le;
+		int rc;
+
+		rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
+					       (u8 *)&result_le);
+		if (!rc)
+			*result = le32_to_cpu(result_le);
+
+		return rc;
+	}
 }
 
 static inline int tpm_tis_write_bytes(struct tpm_tis_data *data, u32 addr,
@@ -145,7 +169,18 @@ static inline int tpm_tis_write8(struct tpm_tis_data *data, u32 addr, u8 value)
 static inline int tpm_tis_write32(struct tpm_tis_data *data, u32 addr,
 				  u32 value)
 {
-	return data->phy_ops->write32(data, addr, value);
+	if (data->phy_ops->write32) {
+		return data->phy_ops->write32(data, addr, value);
+	} else {
+		__le32 value_le;
+		int rc;
+
+		value_le = cpu_to_le32(value);
+		rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
+						(u8 *)&value_le);
+
+		return rc;
+	}
 }
 
 static inline bool is_bsw(void)
diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 19513e622053..da82924b08fe 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -146,50 +146,9 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
 	return tpm_tis_spi_transfer(data, addr, len, NULL, value);
 }
 
-static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
-{
-	__le16 result_le;
-	int rc;
-
-	rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
-				       (u8 *)&result_le);
-	if (!rc)
-		*result = le16_to_cpu(result_le);
-
-	return rc;
-}
-
-static int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
-{
-	__le32 result_le;
-	int rc;
-
-	rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
-				       (u8 *)&result_le);
-	if (!rc)
-		*result = le32_to_cpu(result_le);
-
-	return rc;
-}
-
-static int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
-{
-	__le32 value_le;
-	int rc;
-
-	value_le = cpu_to_le32(value);
-	rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
-					(u8 *)&value_le);
-
-	return rc;
-}
-
 static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
 	.read_bytes = tpm_tis_spi_read_bytes,
 	.write_bytes = tpm_tis_spi_write_bytes,
-	.read16 = tpm_tis_spi_read16,
-	.read32 = tpm_tis_spi_read32,
-	.write32 = tpm_tis_spi_write32,
 };
 
 static int tpm_tis_spi_probe(struct spi_device *dev)
-- 
2.17.1

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

[Alexander Steffen]

Implements the minimal functionality necessary to talk to an I2C TPM
according to the TCG TPM I2C Interface Specification.

Limitations:
* No IRQ support
* No support for updating GUARD_TIME (uses always the default of 250µs)
* No support for Data Checksum register (optional feature only for I2C)

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
 drivers/char/tpm/Kconfig       |  11 ++
 drivers/char/tpm/Makefile      |   1 +
 drivers/char/tpm/tpm_tis_i2c.c | 233 +++++++++++++++++++++++++++++++++
 3 files changed, 245 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_tis_i2c.c

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 17bfbf9f572f..383371d30931 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -67,6 +67,17 @@ config TCG_TIS_SPI
 	  within Linux. To compile this driver as a module, choose  M here;
 	  the module will be called tpm_tis_spi.
 
+config TCG_TIS_I2C
+	tristate "TPM I2C Interface Specification"
+	depends on I2C
+	select TCG_TIS_CORE
+	---help---
+	  If you have a TPM security chip which is connected to a regular
+	  I2C master (i.e. most embedded platforms) that is compliant with the
+	  TCG TPM I2C Interface Specification 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_I2C_ATMEL
 	tristate "TPM Interface Specification 1.2 Interface (I2C - Atmel)"
 	depends on I2C
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index c354cdff9c62..c969e4250a1d 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -22,6 +22,7 @@ tpm-$(CONFIG_OF) += eventlog/of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
 obj-$(CONFIG_TCG_TIS) += tpm_tis.o
 obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.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..d6eea9e2af5b
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019 Infineon Technologies AG
+ *
+ * Authors:
+ * Alexander Steffen <alexander.steffen@infineon.com>
+ *
+ * Maintained by: <tpmdd-devel@lists.sourceforge.net>
+ *
+ * Device driver for TCG/TCPA TPM (trusted platform module).
+ * Specifications at www.trustedcomputinggroup.org
+ *
+ * This device driver implements the TPM interface as defined in
+ * the TCG TPM I2C Interface Specification Familiy 2.0, Revision 1.00.
+ *
+ * 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/i2c.h>
+#include <linux/tpm.h>
+#include "tpm.h"
+#include "tpm_tis_core.h"
+
+struct tpm_tis_i2c_phy {
+	struct tpm_tis_data priv;
+	struct i2c_client *i2c_client;
+	u8 *iobuf;
+};
+
+static inline struct tpm_tis_i2c_phy *to_tpm_tis_i2c_phy(struct tpm_tis_data *data)
+{
+	return container_of(data, struct tpm_tis_i2c_phy, priv);
+}
+
+static u8 address_to_register(u32 addr)
+{
+	addr &= 0xFFF;
+	switch (addr) {
+		// adapt register addresses that have changed compared to
+		// older TIS versions
+		case TPM_ACCESS(0):
+			return 0x04;
+		case TPM_DID_VID(0):
+			return 0x48;
+		case TPM_RID(0):
+			return 0x4C;
+		default:
+			return addr;
+	}
+}
+
+static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr,
+				  u16 len, u8 *result)
+{
+	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	int ret;
+
+	u8 locality[] = {
+		0, // TPM_LOC_SEL
+		addr >> 12, // locality
+	};
+	u8 reg = address_to_register(addr);
+	struct i2c_msg msgs[] = {
+		{
+			.addr = phy->i2c_client->addr,
+			.len = sizeof(locality),
+			.buf = locality,
+		},
+		{
+			.addr = phy->i2c_client->addr,
+			.len = sizeof(reg),
+			.buf = &reg,
+		},
+		{
+			.addr = phy->i2c_client->addr,
+			.len = len,
+			.buf = result,
+			.flags = I2C_M_RD,
+		},
+	};
+
+	ret = i2c_transfer(phy->i2c_client->adapter, msgs, ARRAY_SIZE(msgs));
+
+	if (ret < 0)
+		return ret;
+
+	usleep_range(250, 300); // wait default GUARD_TIME of 250µs
+
+	return 0;
+}
+
+static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr,
+				   u16 len, const u8 *value)
+{
+	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+	int ret;
+
+	u8 locality[] = {
+		0, // TPM_LOC_SEL
+		addr >> 12, // locality
+	};
+
+	if (phy->iobuf) {
+		if (len > TPM_BUFSIZE - 1)
+			return -EIO;
+
+		phy->iobuf[0] = address_to_register(addr);
+		memcpy(phy->iobuf + 1, value, len);
+
+		{
+			struct i2c_msg msgs[] = {
+				{
+					.addr = phy->i2c_client->addr,
+					.len = sizeof(locality),
+					.buf = locality,
+				},
+				{
+					.addr = phy->i2c_client->addr,
+					.len = len + 1,
+					.buf = phy->iobuf,
+				},
+			};
+
+			ret = i2c_transfer(phy->i2c_client->adapter, msgs,
+					   ARRAY_SIZE(msgs));
+		}
+	} else {
+		u8 reg = address_to_register(addr);
+
+		struct i2c_msg msgs[] = {
+			{
+				.addr = phy->i2c_client->addr,
+				.len = sizeof(locality),
+				.buf = locality,
+			},
+			{
+				.addr = phy->i2c_client->addr,
+				.len = sizeof(reg),
+				.buf = &reg,
+			},
+			{
+				.addr = phy->i2c_client->addr,
+				.len = len,
+				.buf = (u8*)value,
+				.flags = I2C_M_NOSTART,
+			},
+		};
+
+		ret = i2c_transfer(phy->i2c_client->adapter, msgs,
+				   ARRAY_SIZE(msgs));
+	}
+
+	if (ret < 0)
+		return ret;
+
+	usleep_range(250, 300); // wait default GUARD_TIME of 250µs
+
+	return 0;
+}
+
+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;
+
+	phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_i2c_phy),
+			   GFP_KERNEL);
+	if (!phy)
+		return -ENOMEM;
+
+	phy->i2c_client = dev;
+
+	if (!i2c_check_functionality(dev->adapter, I2C_FUNC_NOSTART)) {
+		phy->iobuf = devm_kmalloc(&dev->dev, TPM_BUFSIZE, GFP_KERNEL);
+		if (!phy->iobuf)
+			return -ENOMEM;
+	}
+
+	return tpm_tis_core_init(&dev->dev, &phy->priv, -1, &tpm_i2c_phy_ops,
+				 NULL);
+}
+
+static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
+
+static int tpm_tis_i2c_remove(struct i2c_client *dev)
+{
+	struct tpm_chip *chip = i2c_get_clientdata(dev);
+
+	tpm_chip_unregister(chip);
+	tpm_tis_remove(chip);
+	return 0;
+}
+
+static const struct i2c_device_id tpm_tis_i2c_id[] = {
+	{"tpm_tis_i2c", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
+
+static const struct of_device_id of_tis_i2c_match[] = {
+	{ .compatible = "tcg,tpm_tis-i2c", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
+
+static struct i2c_driver tpm_tis_i2c_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "tpm_tis_i2c",
+		.pm = &tpm_tis_pm,
+		.of_match_table = of_match_ptr(of_tis_i2c_match),
+	},
+	.probe = tpm_tis_i2c_probe,
+	.remove = tpm_tis_i2c_remove,
+	.id_table = tpm_tis_i2c_id,
+};
+module_i2c_driver(tpm_tis_i2c_driver);
+
+MODULE_DESCRIPTION("TPM Driver for native I2C access");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [RFC PATCH 1/2] tpm: Make implementation of read16/read32/write32 optional

[Jarkko Sakkinen]

On Thu, Jul 18, 2019 at 07:03:54PM +0200, Alexander Steffen wrote:
> Only tpm_tis has a faster way to access multiple bytes at once, every other
> driver will just fall back to read_bytes/write_bytes. Therefore, move this
> common code out of tpm_tis_spi into tpm_tis_core, so that it is
> automatically used when low-level drivers do not implement the specialized
> methods.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---
>  drivers/char/tpm/tpm_tis_core.h | 41 ++++++++++++++++++++++++++++++---
>  drivers/char/tpm/tpm_tis_spi.c  | 41 ---------------------------------
>  2 files changed, 38 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 7337819f5d7b..2c6557b29a1d 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -122,13 +122,37 @@ static inline int tpm_tis_read8(struct tpm_tis_data *data, u32 addr, u8 *result)
>  static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
>  				 u16 *result)
>  {
> -	return data->phy_ops->read16(data, addr, result);
> +	if (data->phy_ops->read16) {
> +		return data->phy_ops->read16(data, addr, result);
> +	} else {
> +		__le16 result_le;
> +		int rc;
> +
> +		rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
> +					       (u8 *)&result_le);
> +		if (!rc)
> +			*result = le16_to_cpu(result_le);
> +
> +		return rc;
> +	}

Looks great.

I prefer to have variable declarations in the beginning of function for
most of the time. Other than that looks legit.

/Jarkko

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

[Jarkko Sakkinen]

On Thu, Jul 18, 2019 at 07:03:55PM +0200, Alexander Steffen wrote:
> +static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr,
> +				   u16 len, const u8 *value)
> +{
> +	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
> +	int ret;
> +
> +	u8 locality[] = {
> +		0, // TPM_LOC_SEL
> +		addr >> 12, // locality
> +	};
> +
> +	if (phy->iobuf) {
> +		if (len > TPM_BUFSIZE - 1)
> +			return -EIO;
> +
> +		phy->iobuf[0] = address_to_register(addr);
> +		memcpy(phy->iobuf + 1, value, len);
> +
> +		{
> +			struct i2c_msg msgs[] = {
> +				{
> +					.addr = phy->i2c_client->addr,
> +					.len = sizeof(locality),
> +					.buf = locality,
> +				},
> +				{
> +					.addr = phy->i2c_client->addr,
> +					.len = len + 1,
> +					.buf = phy->iobuf,
> +				},
> +			};
> +
> +			ret = i2c_transfer(phy->i2c_client->adapter, msgs,
> +					   ARRAY_SIZE(msgs));
> +		}
> +	} else {
> +		u8 reg = address_to_register(addr);
> +
> +		struct i2c_msg msgs[] = {
> +			{
> +				.addr = phy->i2c_client->addr,
> +				.len = sizeof(locality),
> +				.buf = locality,
> +			},
> +			{
> +				.addr = phy->i2c_client->addr,
> +				.len = sizeof(reg),
> +				.buf = &reg,
> +			},
> +			{
> +				.addr = phy->i2c_client->addr,
> +				.len = len,
> +				.buf = (u8*)value,
> +				.flags = I2C_M_NOSTART,
> +			},
> +		};
> +
> +		ret = i2c_transfer(phy->i2c_client->adapter, msgs,
> +				   ARRAY_SIZE(msgs));
> +	}
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(250, 300); // wait default GUARD_TIME of 250µs

This does not look good. Would prefer to have named constants.

> +
> +	return 0;
> +}

You could probably simplify this by using branching for constructing
the message arrays and then use the same code path for transfer.

/Jarkko

[PATCH v2] tpm: Make implementation of read16/read32/write32 optional

[Johannes Holland]

From: Alexander Steffen <Alexander.Steffen@infineon.com>

Only tpm_tis and tpm_tis_synquacer have a dedicated way to access
multiple bytes at once, every other driver will just fall back to
read_bytes/write_bytes. Therefore, move this common code out of
tpm_tis_spi_main and tpm_tis_spi_cr50 into tpm_tis_core, so that it is
automatically used when low-level drivers do not implement the
specialized methods.

Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.h     | 37 +++++++++++++++++++++++---
 drivers/char/tpm/tpm_tis_spi.h      |  4 ---
 drivers/char/tpm/tpm_tis_spi_cr50.c |  3 ---
 drivers/char/tpm/tpm_tis_spi_main.c | 41 -----------------------------
 4 files changed, 34 insertions(+), 51 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 3be24f221e32..a1feff9d6c1f 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -128,13 +128,35 @@ static inline int tpm_tis_read8(struct tpm_tis_data *data, u32 addr, u8 *result)
 static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
 				 u16 *result)
 {
-	return data->phy_ops->read16(data, addr, result);
+	__le16 result_le;
+	int rc;
+
+	if (data->phy_ops->read16)
+		return data->phy_ops->read16(data, addr, result);
+
+	rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
+				       (u8 *)&result_le);
+	if (!rc)
+		*result = le16_to_cpu(result_le);
+
+	return rc;
 }
 
 static inline int tpm_tis_read32(struct tpm_tis_data *data, u32 addr,
 				 u32 *result)
 {
-	return data->phy_ops->read32(data, addr, result);
+	__le32 result_le;
+	int rc;
+
+	if (data->phy_ops->read32)
+		return data->phy_ops->read32(data, addr, result);
+
+	rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
+				       (u8 *)&result_le);
+	if (!rc)
+		*result = le32_to_cpu(result_le);
+
+	return rc;
 }
 
 static inline int tpm_tis_write_bytes(struct tpm_tis_data *data, u32 addr,
@@ -151,7 +173,16 @@ static inline int tpm_tis_write8(struct tpm_tis_data *data, u32 addr, u8 value)
 static inline int tpm_tis_write32(struct tpm_tis_data *data, u32 addr,
 				  u32 value)
 {
-	return data->phy_ops->write32(data, addr, value);
+	__le32 value_le;
+	int rc;
+
+	if (data->phy_ops->write32)
+		return data->phy_ops->write32(data, addr, value);
+
+	value_le = cpu_to_le32(value);
+	rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
+					(u8 *)&value_le);
+	return rc;
 }
 
 static inline bool is_bsw(void)
diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h
index bba73979c368..d0f66f6f1931 100644
--- a/drivers/char/tpm/tpm_tis_spi.h
+++ b/drivers/char/tpm/tpm_tis_spi.h
@@ -31,10 +31,6 @@ extern int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
 extern int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 				u8 *in, const u8 *out);
 
-extern int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result);
-extern int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result);
-extern int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value);
-
 #ifdef CONFIG_TCG_TIS_SPI_CR50
 extern int cr50_spi_probe(struct spi_device *spi);
 #else
diff --git a/drivers/char/tpm/tpm_tis_spi_cr50.c b/drivers/char/tpm/tpm_tis_spi_cr50.c
index 7bf123d3c537..9584856d955d 100644
--- a/drivers/char/tpm/tpm_tis_spi_cr50.c
+++ b/drivers/char/tpm/tpm_tis_spi_cr50.c
@@ -236,9 +236,6 @@ static int tpm_tis_spi_cr50_write_bytes(struct tpm_tis_data *data, u32 addr,
 static const struct tpm_tis_phy_ops tpm_spi_cr50_phy_ops = {
 	.read_bytes = tpm_tis_spi_cr50_read_bytes,
 	.write_bytes = tpm_tis_spi_cr50_write_bytes,
-	.read16 = tpm_tis_spi_read16,
-	.read32 = tpm_tis_spi_read32,
-	.write32 = tpm_tis_spi_write32,
 };
 
 static void cr50_print_fw_version(struct tpm_tis_data *data)
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index aaa59a00eeae..f9e937c79c59 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -152,44 +152,6 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
 	return tpm_tis_spi_transfer(data, addr, len, NULL, value);
 }
 
-int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
-{
-	__le16 result_le;
-	int rc;
-
-	rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
-				       (u8 *)&result_le);
-	if (!rc)
-		*result = le16_to_cpu(result_le);
-
-	return rc;
-}
-
-int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
-{
-	__le32 result_le;
-	int rc;
-
-	rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
-				       (u8 *)&result_le);
-	if (!rc)
-		*result = le32_to_cpu(result_le);
-
-	return rc;
-}
-
-int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
-{
-	__le32 value_le;
-	int rc;
-
-	value_le = cpu_to_le32(value);
-	rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
-					(u8 *)&value_le);
-
-	return rc;
-}
-
 int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
 		     int irq, const struct tpm_tis_phy_ops *phy_ops)
 {
@@ -205,9 +167,6 @@ int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
 static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
 	.read_bytes = tpm_tis_spi_read_bytes,
 	.write_bytes = tpm_tis_spi_write_bytes,
-	.read16 = tpm_tis_spi_read16,
-	.read32 = tpm_tis_spi_read32,
-	.write32 = tpm_tis_spi_write32,
 };
 
 static int tpm_tis_spi_probe(struct spi_device *dev)
-- 
2.31.1.windows.1

Re: [PATCH v2] tpm: Make implementation of read16/read32/write32 optional

[Jarkko Sakkinen]

On Tue, Mar 15, 2022 at 05:14:46PM +0100, Johannes Holland wrote:
> From: Alexander Steffen <Alexander.Steffen@infineon.com>
> 
> Only tpm_tis and tpm_tis_synquacer have a dedicated way to access
> multiple bytes at once, every other driver will just fall back to
> read_bytes/write_bytes. Therefore, move this common code out of
> tpm_tis_spi_main and tpm_tis_spi_cr50 into tpm_tis_core, so that it is
> automatically used when low-level drivers do not implement the
> specialized methods.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> ---

Please, add a change log to the next version, which also includes
description of v2 changes.

Redudancy is insignificant, and how things are now does not require
special cases from the upper layer.

Instead you should do this:

enum tpm_tis_io_mode {
        TPM_TIS_PHYS_8,
        TPM_TIS_PHYS_16,
        TPM_TIS_PHYS_32,
};

struct tpm_tis_phy_ops {
	int (*read_bytes)(struct tpm_tis_data *data, u32 addr, u16 len, u8 *result,
                          int mode);
	int (*write_bytes)(struct tpm_tis_data *data, u32 addr, u16 len, const u8 *value,
                           int mode);
};

And e.g.

static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
{
        rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), &result_le, TPM_TIS_PHYS_16);
}

This takes away over half of the callback API, and does not require special
cases on the top layer.

BR, Jarkko