[PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Ilias Apalodimas]

There's a lot of code duplication in U-Boot right now.  All the TPM TIS
compatible drivers we have at the moment have their own copy of a TIS
implementation.

So let's create a common layer which implements the core TIS functions.
Any driver added from now own, which is compatible with the TIS spec, will
only have to provide the underlying bus communication mechanisms.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes since v1:
- 
 drivers/tpm/tpm2_tis_core.c | 545 ++++++++++++++++++++++++++++++++++++
 drivers/tpm/tpm_tis.h       |  40 +++
 include/tpm-v2.h            |   1 +
 3 files changed, 586 insertions(+)
 create mode 100644 drivers/tpm/tpm2_tis_core.c

diff --git a/drivers/tpm/tpm2_tis_core.c b/drivers/tpm/tpm2_tis_core.c
new file mode 100644
index 000000000000..9860ce2379e0
--- /dev/null
+++ b/drivers/tpm/tpm2_tis_core.c
@@ -0,0 +1,545 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020, Linaro Limited
+ *
+ * Based on the Linux TIS core interface
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <tpm-v2.h>
+#include <linux/delay.h>
+#include <linux/unaligned/be_byteshift.h>
+#include "tpm_tis.h"
+
+/**
+ * tpm_tis_get_desc - Get the TPM description
+ *
+ * @udev: udevice
+ * @buf:  buffer to fill data
+ * @size: buffer size
+ *
+ * @Return: Number of characters written (or would have been written) in buffer
+ */
+int tpm_tis_get_desc(struct udevice *udev, char *buf, int size)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+
+	if (size < 80)
+		return -ENOSPC;
+
+	return snprintf(buf, size,
+			"%s v2.0: VendorID 0x%04x, DeviceID 0x%04x, RevisionID 0x%02x [%s]",
+			udev->name, chip->vend_dev & 0xFFFF,
+			chip->vend_dev >> 16, chip->rid,
+			(chip->is_open ? "open" : "closed"));
+}
+
+/**
+ * tpm_tis_check_locality - Check the current TPM locality
+ *
+ * @udev: udevice
+ * @loc:  locality
+ *
+ * Return: True if the tested locality matches
+ */
+static bool tpm_tis_check_locality(struct udevice *udev, int loc)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	struct tpm_tis_phy_ops *phy_ops = chip->phy_ops;
+	u8 locality;
+
+	if (!phy_ops)
+		return false;
+
+	phy_ops->read_bytes(udev, TPM_ACCESS(loc), 1, &locality);
+	if ((locality & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID |
+	    TPM_ACCESS_REQUEST_USE)) ==
+	    (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) {
+		chip->locality = loc;
+		return true;
+	}
+
+	return false;
+}
+
+/**
+ * tpm_tis_request_locality - Request a locality from the TPM
+ *
+ * @udev: udevce
+ * @loc:  requested locality
+ *
+ * Return: 0 on success -1 on failure
+ */
+int tpm_tis_request_locality(struct udevice *udev, int loc)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	struct tpm_tis_phy_ops *phy_ops = chip->phy_ops;
+	u8 buf = TPM_ACCESS_REQUEST_USE;
+	unsigned long start, stop;
+
+	if (!phy_ops)
+		return -1;
+
+	if (tpm_tis_check_locality(udev, loc))
+		return 0;
+
+	phy_ops->write_bytes(udev, TPM_ACCESS(loc), 1, &buf);
+	start = get_timer(0);
+	stop = chip->timeout_a;
+	do {
+		if (tpm_tis_check_locality(udev, loc))
+			return 0;
+		mdelay(TPM_TIMEOUT_MS);
+	} while (get_timer(start) < stop);
+
+	return -1;
+}
+
+/**
+ * tpm_tis_status - Check the current device status
+ *
+ * @udev:   udevice
+ * @status: return value of status
+ *
+ * Return: 0 on success, negative on failure
+ */
+static int tpm_tis_status(struct udevice *udev, u8 *status)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	struct tpm_tis_phy_ops *phy_ops = chip->phy_ops;
+
+	if (!phy_ops)
+		return -EINVAL;
+
+	if (chip->locality < 0)
+		return -EINVAL;
+
+	phy_ops->read_bytes(udev, TPM_STS(chip->locality), 1, status);
+
+	if ((*status & TPM_STS_READ_ZERO)) {
+		log_err("TPM returned invalid status\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * tpm_tis_release_locality - Release the requested locality
+ *
+ * @udev: udevice
+ * @loc:  requested locality
+ *
+ * Return: 0 on success, negative on failure
+ */
+int tpm_tis_release_locality(struct udevice *udev, int loc)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	struct tpm_tis_phy_ops *phy_ops = chip->phy_ops;
+	u8 buf = TPM_ACCESS_ACTIVE_LOCALITY;
+	int ret;
+
+	if (!phy_ops)
+		return -1;
+
+	if (chip->locality < 0)
+		return 0;
+
+	ret = phy_ops->write_bytes(udev, TPM_ACCESS(loc), 1, &buf);
+	if (!ret)
+		chip->locality = -1;
+
+	return ret;
+}
+
+/**
+ * tpm_tis_wait_for_stat - Wait for TPM to become ready
+ *
+ * @udev:    udev
+ * @mask:    mask to match
+ * @timeout: timeout for retries
+ * @status:  current status
+ *
+ * Return: 0 on success, negative on failure
+ */
+static int tpm_tis_wait_for_stat(struct udevice *udev, u8 mask,
+				 unsigned long timeout, u8 *status)
+{
+	unsigned long start = get_timer(0);
+	unsigned long stop = timeout;
+	int ret;
+
+	do {
+		mdelay(TPM_TIMEOUT_MS);
+		ret = tpm_tis_status(udev, status);
+		if (ret)
+			return ret;
+
+		if ((*status & mask) == mask)
+			return 0;
+	} while (get_timer(start) < stop);
+
+	return -ETIMEDOUT;
+}
+
+/**
+ * tpm_tis_get_burstcount - Get the burstcount for the data FIFO
+ *
+ * @udev:       udevice
+ * @burstcount: current burstcount
+ *
+ * Return: 0 on success, negative on failure
+ */
+static int tpm_tis_get_burstcount(struct udevice *udev, size_t *burstcount)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	struct tpm_tis_phy_ops *phy_ops = chip->phy_ops;
+	unsigned long start, stop;
+	u32 burst;
+
+	if (!phy_ops)
+		return -EINVAL;
+
+	if (chip->locality < 0)
+		return -EINVAL;
+
+	/* wait for burstcount */
+	start = get_timer(0);
+	/*
+	 * This is the TPMv2 defined timeout. Change this in case you want to
+	 * make the driver compatile to TPMv1
+	 */
+	stop = chip->timeout_a;
+	do {
+		phy_ops->read32(udev, TPM_STS(chip->locality), &burst);
+		*burstcount = (burst >> 8) & 0xFFFF;
+		if (*burstcount)
+			return 0;
+
+		mdelay(TPM_TIMEOUT_MS);
+	} while (get_timer(start) < stop);
+
+	return -ETIMEDOUT;
+}
+
+/**
+ * tpm_tis_ready - Cancel pending comands and get the device on a ready state
+ *
+ * @udev: udevcie
+ *
+ * Return: 0 on success, negative on failure
+ */
+static int tpm_tis_ready(struct udevice *udev)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	struct tpm_tis_phy_ops *phy_ops = chip->phy_ops;
+	u8 data = TPM_STS_COMMAND_READY;
+
+	if (!phy_ops)
+		return -1;
+
+	/* This will cancel any pending commands */
+	return phy_ops->write_bytes(udev, TPM_STS(chip->locality), 1, &data);
+}
+
+/**
+ * tpm_tis_send - send data to the device
+ *
+ * @udev: udevice
+ * @buf:  buffer to send
+ * @len:  size of the buffer
+ *
+ * Return: number of bytes sent or negative on failure
+ */
+int tpm_tis_send(struct udevice *udev, const u8 *buf, size_t len)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	struct tpm_tis_phy_ops *phy_ops = chip->phy_ops;
+	size_t burstcnt, wr_size, sent = 0;
+	u8 data = TPM_STS_GO;
+	u8 status;
+	int ret;
+
+	if (!phy_ops)
+		return -EINVAL;
+
+	if (!chip)
+		return -ENODEV;
+
+	ret = tpm_tis_request_locality(udev, 0);
+	if (ret < 0)
+		return -EBUSY;
+
+	ret = tpm_tis_status(udev, &status);
+	if (ret)
+		goto release_locality;
+
+	if (!(status & TPM_STS_COMMAND_READY)) {
+		ret = tpm_tis_ready(udev);
+		if (ret) {
+			log_err("Can't cancel previous TPM operation\n");
+			goto release_locality;
+		}
+		ret = tpm_tis_wait_for_stat(udev, TPM_STS_COMMAND_READY,
+					    chip->timeout_b, &status);
+		if (ret) {
+			log_err("TPM not ready\n");
+			goto release_locality;
+		}
+	}
+
+	while (len > 0) {
+		ret = tpm_tis_get_burstcount(udev, &burstcnt);
+		if (ret)
+			goto release_locality;
+
+		wr_size = min(len, burstcnt);
+		ret = phy_ops->write_bytes(udev, TPM_DATA_FIFO(chip->locality),
+					   wr_size, buf + sent);
+		if (ret < 0)
+			goto release_locality;
+
+		ret = tpm_tis_wait_for_stat(udev, TPM_STS_VALID,
+					    chip->timeout_c, &status);
+		if (ret)
+			goto release_locality;
+
+		sent += wr_size;
+		len -= wr_size;
+		/* make sure the TPM expects more data */
+		if (len && !(status & TPM_STS_DATA_EXPECT)) {
+			ret = -EIO;
+			goto release_locality;
+		}
+	}
+
+	/*
+	 * Make a final check ensuring everything is ok and the TPM expects no
+	 * more data
+	 */
+	ret = tpm_tis_wait_for_stat(udev, TPM_STS_VALID, chip->timeout_c,
+				    &status);
+	if (ret)
+		goto release_locality;
+
+	if (status & TPM_STS_DATA_EXPECT) {
+		ret = -EIO;
+		goto release_locality;
+	}
+
+	ret = phy_ops->write_bytes(udev, TPM_STS(chip->locality), 1, &data);
+	if (ret)
+		goto release_locality;
+
+	tpm_tis_release_locality(udev, chip->locality);
+	return sent;
+
+release_locality:
+	tpm_tis_ready(udev);
+	tpm_tis_release_locality(udev, chip->locality);
+
+	return ret;
+}
+
+/**
+ * tpm_tis_recv_data - Receive data from a device. Wrapper for tpm_tis_recv
+ *
+ * @udev: udevice
+ * @buf:  buffer to copy data
+ * @size: buffer size
+ *
+ * Return: bytes read or negative on failure
+ */
+static int tpm_tis_recv_data(struct udevice *udev, u8 *buf, size_t count)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	struct tpm_tis_phy_ops *phy_ops = chip->phy_ops;
+	int size = 0, len, ret;
+	size_t burstcnt;
+	u8 status;
+
+	if (!phy_ops)
+		return -EINVAL;
+
+	while (size < count &&
+	       tpm_tis_wait_for_stat(udev, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+				     chip->timeout_c, &status) == 0) {
+		ret = tpm_tis_get_burstcount(udev, &burstcnt);
+		if (ret)
+			return burstcnt;
+
+		len = min_t(int, burstcnt, count - size);
+		ret = phy_ops->read_bytes(udev, TPM_DATA_FIFO(chip->locality),
+					  len, buf + size);
+		if (ret < 0)
+			return ret;
+
+		size += len;
+	}
+
+	return size;
+}
+
+/**
+ * tpm_tis_recv - Receive data from a device
+ *
+ * @udev: udevice
+ * @buf:  buffer to copy data
+ * @size: buffer size
+ *
+ * Return: bytes read or negative on failure
+ */
+int tpm_tis_recv(struct udevice *udev, u8 *buf, size_t count)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	int ret;
+	int size, expected;
+
+	if (!chip)
+		return -ENODEV;
+
+	if (count < TPM_HEADER_SIZE)
+		return -E2BIG;
+
+	ret = tpm_tis_request_locality(udev, 0);
+	if (ret < 0)
+		return -EBUSY;
+
+	size = tpm_tis_recv_data(udev, buf, TPM_HEADER_SIZE);
+	if (size < TPM_HEADER_SIZE) {
+		log_err("TPM error, unable to read header\n");
+		goto out;
+	}
+
+	expected = get_unaligned_be32(buf + TPM_CMD_COUNT_OFFSET);
+	if (expected > count) {
+		size = -EIO;
+		log_warning("Too much data: %d > %zu\n", expected, count);
+		goto out;
+	}
+
+	size += tpm_tis_recv_data(udev, &buf[TPM_HEADER_SIZE],
+				   expected - TPM_HEADER_SIZE);
+	if (size < expected) {
+		log(LOGC_NONE, LOGL_ERR,
+		    "TPM error, unable to read remaining bytes of result\n");
+		size = -EIO;
+		goto out;
+	}
+
+out:
+	tpm_tis_ready(udev);
+	tpm_tis_release_locality(udev, chip->locality);
+
+	return size;
+}
+
+/** tpm_tis_cleanup - Get the device in ready state and release locality
+ *
+ * @udev: udevice
+ *
+ * Return: always 0
+ */
+int tpm_tis_cleanup(struct udevice *udev)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+
+	tpm_tis_ready(udev);
+	tpm_tis_release_locality(udev, chip->locality);
+
+	return 0;
+}
+
+/**
+ * tpm_tis_open - Open the device and request locality 0
+ *
+ * @udev: udevice
+ *
+ * Return: 0 on success, negative on failure
+ */
+int tpm_tis_open(struct udevice *udev)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	int ret;
+
+	if (chip->is_open)
+		return -EBUSY;
+
+	ret = tpm_tis_request_locality(udev, 0);
+	if (!ret)
+		chip->is_open = 1;
+
+	return ret;
+}
+
+/**
+ * tpm_tis_ops_register - register the PHY ops for the device
+ *
+ * @udev: udevice
+ * @ops: bus ops for the device
+ */
+void tpm_tis_ops_register(struct udevice *udev, struct tpm_tis_phy_ops *ops)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+
+	chip->phy_ops = ops;
+}
+
+/**
+ * tpm_tis_init - inititalize the device
+ *
+ * @udev: udevice
+ *
+ * Return: 0 on success, negative on failure
+ */
+int tpm_tis_init(struct udevice *udev)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	struct tpm_tis_phy_ops *phy_ops = chip->phy_ops;
+	int ret;
+	u32 tmp;
+
+	if (!phy_ops)
+		return -1;
+	ret = tpm_tis_request_locality(udev, 0);
+	if (ret)
+		return ret;
+
+	chip->timeout_a = TIS_SHORT_TIMEOUT_MS;
+	chip->timeout_b = TIS_LONG_TIMEOUT_MS;
+	chip->timeout_c = TIS_SHORT_TIMEOUT_MS;
+	chip->timeout_d = TIS_SHORT_TIMEOUT_MS;
+
+	/* Disable interrupts */
+	phy_ops->read32(udev, TPM_INT_ENABLE(chip->locality), &tmp);
+	tmp |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
+	       TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
+	tmp &= ~TPM_GLOBAL_INT_ENABLE;
+	phy_ops->write32(udev, TPM_INT_ENABLE(chip->locality), tmp);
+
+	phy_ops->read_bytes(udev, TPM_RID(chip->locality), 1, &chip->rid);
+	phy_ops->read32(udev, TPM_DID_VID(chip->locality), &chip->vend_dev);
+
+	return tpm_tis_release_locality(udev, chip->locality);
+}
+
+/**
+ * tpm_tis_close - Close the device and release locality
+ *
+ * @udev: udevice
+ *
+ * Return: 0 on success, negative on failure
+ */
+int tpm_tis_close(struct udevice *udev)
+{
+	struct tpm_chip *chip = dev_get_priv(udev);
+	int ret = 0;
+
+	if (chip->is_open) {
+		ret = tpm_tis_release_locality(udev, chip->locality);
+		chip->is_open = 0;
+	}
+
+	return ret;
+}
diff --git a/drivers/tpm/tpm_tis.h b/drivers/tpm/tpm_tis.h
index 2a160fe05c9a..fde3bb71f7c2 100644
--- a/drivers/tpm/tpm_tis.h
+++ b/drivers/tpm/tpm_tis.h
@@ -21,6 +21,37 @@
 #include <linux/compiler.h>
 #include <linux/types.h>
 
+struct tpm_tis_phy_ops {
+	int (*read_bytes)(struct udevice *udev, u32 addr, u16 len,
+			  u8 *result);
+	int (*write_bytes)(struct udevice *udev, u32 addr, u16 len,
+			   const u8 *value);
+	int (*read16)(struct udevice *udev, u32 addr, u16 *result);
+	int (*read32)(struct udevice *udev, u32 addr, u32 *result);
+	int (*write32)(struct udevice *udev, u32 addr, u32 src);
+};
+
+enum tis_int_flags {
+	TPM_GLOBAL_INT_ENABLE = 0x80000000,
+	TPM_INTF_BURST_COUNT_STATIC = 0x100,
+	TPM_INTF_CMD_READY_INT = 0x080,
+	TPM_INTF_INT_EDGE_FALLING = 0x040,
+	TPM_INTF_INT_EDGE_RISING = 0x020,
+	TPM_INTF_INT_LEVEL_LOW = 0x010,
+	TPM_INTF_INT_LEVEL_HIGH = 0x008,
+	TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
+	TPM_INTF_STS_VALID_INT = 0x002,
+	TPM_INTF_DATA_AVAIL_INT = 0x001,
+};
+
+#define TPM_ACCESS(l)                   (0x0000 | ((l) << 12))
+#define TPM_INT_ENABLE(l)               (0x0008 | ((l) << 12))
+#define TPM_STS(l)                      (0x0018 | ((l) << 12))
+#define TPM_DATA_FIFO(l)                (0x0024 | ((l) << 12))
+#define TPM_DID_VID(l)                  (0x0F00 | ((l) << 12))
+#define TPM_RID(l)                      (0x0F04 | ((l) << 12))
+#define TPM_INTF_CAPS(l)                (0x0014 | ((l) << 12))
+
 enum tpm_timeout {
 	TPM_TIMEOUT_MS			= 5,
 	TIS_SHORT_TIMEOUT_MS		= 750,
@@ -43,6 +74,7 @@ struct tpm_chip {
 	u8 rid;
 	unsigned long timeout_a, timeout_b, timeout_c, timeout_d;  /* msec */
 	ulong chip_type;
+	struct tpm_tis_phy_ops *phy_ops;
 };
 
 struct tpm_input_header {
@@ -130,4 +162,12 @@ enum tis_status {
 };
 #endif
 
+int tpm_tis_open(struct udevice *udev);
+int tpm_tis_close(struct udevice *udev);
+int tpm_tis_cleanup(struct udevice *udev);
+int tpm_tis_send(struct udevice *udev, const u8 *buf, size_t len);
+int tpm_tis_recv(struct udevice *udev, u8 *buf, size_t count);
+int tpm_tis_get_desc(struct udevice *udev, char *buf, int size);
+int tpm_tis_init(struct udevice *udev);
+void tpm_tis_ops_register(struct udevice *udev, struct tpm_tis_phy_ops *ops);
 #endif
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index 247b38696766..3e48e358613f 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -378,6 +378,7 @@ enum {
 	TPM_STS_DATA_EXPECT		= 1 << 3,
 	TPM_STS_SELF_TEST_DONE		= 1 << 2,
 	TPM_STS_RESPONSE_RETRY		= 1 << 1,
+	TPM_STS_READ_ZERO               = 0x23
 };
 
 enum {
-- 
2.32.0.rc0

[PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Ilias Apalodimas]

Add support for devices that expose a TPMv2 though MMIO.
Apart from those devices, we can use the driver in our QEMU setups and
test TPM related code which is difficult to achieve using the sandbox
driver (e.g test the EFI TCG2 protocol).

It's worth noting that a previous patch added TPMv2 TIS core functions,
which the current driver is consuming.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes since v1:
- split off the tis core code into a different file
 drivers/tpm/Kconfig         |   9 +++
 drivers/tpm/Makefile        |   1 +
 drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+)
 create mode 100644 drivers/tpm/tpm2_tis_mmio.c

diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig
index 9eebab5cfd90..406ee8716e1e 100644
--- a/drivers/tpm/Kconfig
+++ b/drivers/tpm/Kconfig
@@ -161,6 +161,15 @@ config TPM2_FTPM_TEE
 	help
 	  This driver supports firmware TPM running in TEE.
 
+config TPM2_MMIO
+	bool "MMIO based TPM2 Interface"
+	depends on TPM_V2
+	help
+	  This driver supports firmware TPM2.0 MMIO interface.
+	  The usual TPM operations and the 'tpm' command can be used to talk
+	  to the device using the standard TPM Interface Specification (TIS)
+	  protocol.
+
 endif # TPM_V2
 
 endmenu
diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile
index f64d20067f88..1065c1874f58 100644
--- a/drivers/tpm/Makefile
+++ b/drivers/tpm/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_$(SPL_TPL_)TPM2_CR50_I2C) += cr50_i2c.o
 obj-$(CONFIG_TPM2_TIS_SANDBOX) += tpm2_tis_sandbox.o
 obj-$(CONFIG_TPM2_TIS_SPI) += tpm2_tis_spi.o
 obj-$(CONFIG_TPM2_FTPM_TEE) += tpm2_ftpm_tee.o
+obj-$(CONFIG_TPM2_MMIO) += tpm2_tis_core.o tpm2_tis_mmio.o
diff --git a/drivers/tpm/tpm2_tis_mmio.c b/drivers/tpm/tpm2_tis_mmio.c
new file mode 100644
index 000000000000..2183a2807162
--- /dev/null
+++ b/drivers/tpm/tpm2_tis_mmio.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * driver for mmio TCG/TIS TPM (trusted platform module).
+ *
+ * Specifications at www.trustedcomputinggroup.org
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <log.h>
+#include <tpm-v2.h>
+#include <linux/bitops.h>
+#include <linux/compiler.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/types.h>
+#include <linux/io.h>
+#include <linux/unaligned/be_byteshift.h>
+#include "tpm_tis.h"
+#include "tpm_internal.h"
+
+struct tpm_tis_chip_data {
+	unsigned int pcr_count;
+	unsigned int pcr_select_min;
+	unsigned int time_before_first_cmd_ms;
+	void __iomem *iobase;
+};
+
+static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len,
+			   u8 *result)
+{
+	struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
+
+	while (len--)
+		*result++ = ioread8(drv_data->iobase + addr);
+	return 0;
+}
+
+static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len,
+			    const u8 *value)
+{
+	struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
+
+	while (len--)
+		iowrite8(*value++, drv_data->iobase + addr);
+	return 0;
+}
+
+static int mmio_read16(struct udevice *udev, u32 addr, u16 *result)
+{
+	struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
+
+	*result = ioread16(drv_data->iobase + addr);
+	return 0;
+}
+
+static int mmio_read32(struct udevice *udev, u32 addr, u32 *result)
+{
+	struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
+
+	*result = ioread32(drv_data->iobase + addr);
+	return 0;
+}
+
+static int mmio_write32(struct udevice *udev, u32 addr, u32 value)
+{
+	struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
+
+	iowrite32(value, drv_data->iobase + addr);
+	return 0;
+}
+
+static struct tpm_tis_phy_ops phy_ops = {
+	.read_bytes = mmio_read_bytes,
+	.write_bytes = mmio_write_bytes,
+	.read16 = mmio_read16,
+	.read32 = mmio_read32,
+	.write32 = mmio_write32,
+};
+
+static int tpm_tis_probe(struct udevice *udev)
+{
+	struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
+	struct tpm_chip_priv *priv = dev_get_uclass_priv(udev);
+	int ret = 0;
+	fdt_addr_t ioaddr;
+	u64 sz;
+
+	ioaddr = dev_read_addr(udev);
+	if (ioaddr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+
+	ret = dev_read_u64(udev, "reg", &sz);
+	if (ret)
+		return -EINVAL;
+
+	drv_data->iobase = ioremap(ioaddr, sz);
+	log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz);
+	tpm_tis_ops_register(udev, &phy_ops);
+	ret = tpm_tis_init(udev);
+	if (ret)
+		goto iounmap;
+
+	priv->pcr_count = drv_data->pcr_count;
+	priv->pcr_select_min = drv_data->pcr_select_min;
+	/*
+	 * Although the driver probably works with a TPMv1 our Kconfig
+	 * limits the driver to TPMv2 only
+	 */
+	priv->version = TPM_V2;
+
+	return ret;
+iounmap:
+	iounmap(drv_data->iobase);
+	return -EINVAL;
+}
+
+static int tpm_tis_remove(struct udevice *udev)
+{
+	struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
+
+	iounmap(drv_data->iobase);
+	return tpm_tis_cleanup(udev);
+}
+
+static const struct tpm_ops tpm_tis_ops = {
+	.open		= tpm_tis_open,
+	.close		= tpm_tis_close,
+	.get_desc	= tpm_tis_get_desc,
+	.send		= tpm_tis_send,
+	.recv		= tpm_tis_recv,
+	.cleanup	= tpm_tis_cleanup,
+};
+
+static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
+	.pcr_count = 24,
+	.pcr_select_min = 3,
+};
+
+static const struct udevice_id tpm_tis_ids[] = {
+	{
+		.compatible = "tcg,tpm-tis-mmio",
+		.data = (ulong)&tpm_tis_std_chip_data,
+	},
+	{ }
+};
+
+U_BOOT_DRIVER(tpm_tis_mmio) = {
+	.name   = "tpm_tis_mmio",
+	.id     = UCLASS_TPM,
+	.of_match = tpm_tis_ids,
+	.ops    = &tpm_tis_ops,
+	.probe	= tpm_tis_probe,
+	.remove	= tpm_tis_remove,
+	.priv_auto	= sizeof(struct tpm_chip),
+};
-- 
2.32.0.rc0

Re: [PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Simon Glass]

Hi Ilias,

On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> There's a lot of code duplication in U-Boot right now.  All the TPM TIS

You mean in the TPM code I think.

> compatible drivers we have at the moment have their own copy of a TIS
> implementation.
>
> So let's create a common layer which implements the core TIS functions.
> Any driver added from now own, which is compatible with the TIS spec, will
> only have to provide the underlying bus communication mechanisms.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Changes since v1:
> -
>  drivers/tpm/tpm2_tis_core.c | 545 ++++++++++++++++++++++++++++++++++++
>  drivers/tpm/tpm_tis.h       |  40 +++
>  include/tpm-v2.h            |   1 +
>  3 files changed, 586 insertions(+)
>  create mode 100644 drivers/tpm/tpm2_tis_core.c

[..]

> diff --git a/drivers/tpm/tpm_tis.h b/drivers/tpm/tpm_tis.h
> index 2a160fe05c9a..fde3bb71f7c2 100644
> --- a/drivers/tpm/tpm_tis.h
> +++ b/drivers/tpm/tpm_tis.h
> @@ -21,6 +21,37 @@
>  #include <linux/compiler.h>
>  #include <linux/types.h>
>
> +struct tpm_tis_phy_ops {
> +       int (*read_bytes)(struct udevice *udev, u32 addr, u16 len,
> +                         u8 *result);
> +       int (*write_bytes)(struct udevice *udev, u32 addr, u16 len,
> +                          const u8 *value);
> +       int (*read16)(struct udevice *udev, u32 addr, u16 *result);
> +       int (*read32)(struct udevice *udev, u32 addr, u32 *result);
> +       int (*write32)(struct udevice *udev, u32 addr, u32 src);

A few points:

- these need comments
- can we use uint instead of u32 for the value args? We should use
native types where we can
- it seems like this should be a driver interface - see for example
how cros_ec.c works. It has a shared code library and the drivers each
implement an interface similar to the above, but on different buses.
In general function pointers are a sign we should be using a driver

> +};
> +
> +enum tis_int_flags {
> +       TPM_GLOBAL_INT_ENABLE = 0x80000000,
> +       TPM_INTF_BURST_COUNT_STATIC = 0x100,
> +       TPM_INTF_CMD_READY_INT = 0x080,
> +       TPM_INTF_INT_EDGE_FALLING = 0x040,
> +       TPM_INTF_INT_EDGE_RISING = 0x020,
> +       TPM_INTF_INT_LEVEL_LOW = 0x010,
> +       TPM_INTF_INT_LEVEL_HIGH = 0x008,
> +       TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
> +       TPM_INTF_STS_VALID_INT = 0x002,
> +       TPM_INTF_DATA_AVAIL_INT = 0x001,
> +};
> +
> +#define TPM_ACCESS(l)                   (0x0000 | ((l) << 12))
> +#define TPM_INT_ENABLE(l)               (0x0008 | ((l) << 12))
> +#define TPM_STS(l)                      (0x0018 | ((l) << 12))
> +#define TPM_DATA_FIFO(l)                (0x0024 | ((l) << 12))
> +#define TPM_DID_VID(l)                  (0x0F00 | ((l) << 12))
> +#define TPM_RID(l)                      (0x0F04 | ((l) << 12))
> +#define TPM_INTF_CAPS(l)                (0x0014 | ((l) << 12))
> +
>  enum tpm_timeout {
>         TPM_TIMEOUT_MS                  = 5,
>         TIS_SHORT_TIMEOUT_MS            = 750,
> @@ -43,6 +74,7 @@ struct tpm_chip {
>         u8 rid;
>         unsigned long timeout_a, timeout_b, timeout_c, timeout_d;  /* msec */
>         ulong chip_type;
> +       struct tpm_tis_phy_ops *phy_ops;
>  };
>
>  struct tpm_input_header {
> @@ -130,4 +162,12 @@ enum tis_status {
>  };
>  #endif
>
> +int tpm_tis_open(struct udevice *udev);
> +int tpm_tis_close(struct udevice *udev);
> +int tpm_tis_cleanup(struct udevice *udev);
> +int tpm_tis_send(struct udevice *udev, const u8 *buf, size_t len);
> +int tpm_tis_recv(struct udevice *udev, u8 *buf, size_t count);
> +int tpm_tis_get_desc(struct udevice *udev, char *buf, int size);
> +int tpm_tis_init(struct udevice *udev);
> +void tpm_tis_ops_register(struct udevice *udev, struct tpm_tis_phy_ops *ops);

comments on all of these

>  #endif
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index 247b38696766..3e48e358613f 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -378,6 +378,7 @@ enum {
>         TPM_STS_DATA_EXPECT             = 1 << 3,
>         TPM_STS_SELF_TEST_DONE          = 1 << 2,
>         TPM_STS_RESPONSE_RETRY          = 1 << 1,
> +       TPM_STS_READ_ZERO               = 0x23

Does this below in another patch?

>  };
>
>  enum {
> --
> 2.32.0.rc0
>

I feel that this API could be useful in reducing code duplication, but
in fact it has just created more, so far as I can see from this series
:-) So I think you should convert at least one driver to show its
value (and not make things any worse).

Regards,
Simon

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Simon Glass]

Hi Ilias,

On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Add support for devices that expose a TPMv2 though MMIO.
> Apart from those devices, we can use the driver in our QEMU setups and
> test TPM related code which is difficult to achieve using the sandbox
> driver (e.g test the EFI TCG2 protocol).
>
> It's worth noting that a previous patch added TPMv2 TIS core functions,
> which the current driver is consuming.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Changes since v1:
> - split off the tis core code into a different file
>  drivers/tpm/Kconfig         |   9 +++
>  drivers/tpm/Makefile        |   1 +
>  drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 166 insertions(+)
>  create mode 100644 drivers/tpm/tpm2_tis_mmio.c

Looks good to me

>
> diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig
> index 9eebab5cfd90..406ee8716e1e 100644
> --- a/drivers/tpm/Kconfig
> +++ b/drivers/tpm/Kconfig
> @@ -161,6 +161,15 @@ config TPM2_FTPM_TEE
>         help
>           This driver supports firmware TPM running in TEE.
>
> +config TPM2_MMIO
> +       bool "MMIO based TPM2 Interface"
> +       depends on TPM_V2
> +       help
> +         This driver supports firmware TPM2.0 MMIO interface.
> +         The usual TPM operations and the 'tpm' command can be used to talk
> +         to the device using the standard TPM Interface Specification (TIS)
> +         protocol.
> +
>  endif # TPM_V2
>
>  endmenu
> diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile
> index f64d20067f88..1065c1874f58 100644
> --- a/drivers/tpm/Makefile
> +++ b/drivers/tpm/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_$(SPL_TPL_)TPM2_CR50_I2C) += cr50_i2c.o
>  obj-$(CONFIG_TPM2_TIS_SANDBOX) += tpm2_tis_sandbox.o
>  obj-$(CONFIG_TPM2_TIS_SPI) += tpm2_tis_spi.o
>  obj-$(CONFIG_TPM2_FTPM_TEE) += tpm2_ftpm_tee.o
> +obj-$(CONFIG_TPM2_MMIO) += tpm2_tis_core.o tpm2_tis_mmio.o
> diff --git a/drivers/tpm/tpm2_tis_mmio.c b/drivers/tpm/tpm2_tis_mmio.c
> new file mode 100644
> index 000000000000..2183a2807162
> --- /dev/null
> +++ b/drivers/tpm/tpm2_tis_mmio.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * driver for mmio TCG/TIS TPM (trusted platform module).
> + *
> + * Specifications at www.trustedcomputinggroup.org
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <log.h>
> +#include <tpm-v2.h>
> +#include <linux/bitops.h>
> +#include <linux/compiler.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +#include <linux/io.h>
> +#include <linux/unaligned/be_byteshift.h>
> +#include "tpm_tis.h"
> +#include "tpm_internal.h"
> +
> +struct tpm_tis_chip_data {
> +       unsigned int pcr_count;
> +       unsigned int pcr_select_min;
> +       unsigned int time_before_first_cmd_ms;
> +       void __iomem *iobase;
> +};

comments

> +
> +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len,
> +                          u8 *result)
> +{
> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> +
> +       while (len--)
> +               *result++ = ioread8(drv_data->iobase + addr);

We normally put a blank line before the final return

> +       return 0;
> +}
> +
> +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len,
> +                           const u8 *value)
> +{
> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> +
> +       while (len--)
> +               iowrite8(*value++, drv_data->iobase + addr);
> +       return 0;
> +}
> +
> +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result)
> +{
> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> +
> +       *result = ioread16(drv_data->iobase + addr);
> +       return 0;
> +}
> +
> +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result)
> +{
> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> +
> +       *result = ioread32(drv_data->iobase + addr);
> +       return 0;
> +}
> +
> +static int mmio_write32(struct udevice *udev, u32 addr, u32 value)
> +{
> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> +
> +       iowrite32(value, drv_data->iobase + addr);
> +       return 0;
> +}
> +
> +static struct tpm_tis_phy_ops phy_ops = {

Should be a uclass interface.

> +       .read_bytes = mmio_read_bytes,
> +       .write_bytes = mmio_write_bytes,
> +       .read16 = mmio_read16,
> +       .read32 = mmio_read32,
> +       .write32 = mmio_write32,
> +};
> +
> +static int tpm_tis_probe(struct udevice *udev)
> +{
> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> +       struct tpm_chip_priv *priv = dev_get_uclass_priv(udev);
> +       int ret = 0;
> +       fdt_addr_t ioaddr;
> +       u64 sz;
> +
> +       ioaddr = dev_read_addr(udev);
> +       if (ioaddr == FDT_ADDR_T_NONE)
> +               return -EINVAL;

consider this for easier debugging:

   return log_msg_ret("ioaddr", -EINVAL);

> +
> +       ret = dev_read_u64(udev, "reg", &sz);
> +       if (ret)
> +               return -EINVAL;
> +
> +       drv_data->iobase = ioremap(ioaddr, sz);
> +       log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz);

log_debug() I think?

> +       tpm_tis_ops_register(udev, &phy_ops);
> +       ret = tpm_tis_init(udev);
> +       if (ret)
> +               goto iounmap;
> +
> +       priv->pcr_count = drv_data->pcr_count;
> +       priv->pcr_select_min = drv_data->pcr_select_min;
> +       /*
> +        * Although the driver probably works with a TPMv1 our Kconfig
> +        * limits the driver to TPMv2 only
> +        */
> +       priv->version = TPM_V2;
> +
> +       return ret;
> +iounmap:
> +       iounmap(drv_data->iobase);
> +       return -EINVAL;
> +}
> +
> +static int tpm_tis_remove(struct udevice *udev)
> +{
> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> +
> +       iounmap(drv_data->iobase);
> +       return tpm_tis_cleanup(udev);
> +}
> +
> +static const struct tpm_ops tpm_tis_ops = {
> +       .open           = tpm_tis_open,
> +       .close          = tpm_tis_close,
> +       .get_desc       = tpm_tis_get_desc,
> +       .send           = tpm_tis_send,
> +       .recv           = tpm_tis_recv,
> +       .cleanup        = tpm_tis_cleanup,
> +};
> +
> +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
> +       .pcr_count = 24,
> +       .pcr_select_min = 3,
> +};
> +
> +static const struct udevice_id tpm_tis_ids[] = {
> +       {
> +               .compatible = "tcg,tpm-tis-mmio",
> +               .data = (ulong)&tpm_tis_std_chip_data,
> +       },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(tpm_tis_mmio) = {
> +       .name   = "tpm_tis_mmio",
> +       .id     = UCLASS_TPM,
> +       .of_match = tpm_tis_ids,
> +       .ops    = &tpm_tis_ops,
> +       .probe  = tpm_tis_probe,
> +       .remove = tpm_tis_remove,
> +       .priv_auto      = sizeof(struct tpm_chip),
> +};
> --
> 2.32.0.rc0
>

Regards,
Simon

Re: [PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Ilias Apalodimas]

On Sat, Jul 10, 2021 at 06:00:57PM -0600, Simon Glass wrote:
> Hi Ilias,
> 
> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > There's a lot of code duplication in U-Boot right now.  All the TPM TIS
> 
> You mean in the TPM code I think.
> 

Yes. Basically al TPM drivers duplicate this.

> > compatible drivers we have at the moment have their own copy of a TIS
> > implementation.
> >
> > So let's create a common layer which implements the core TIS functions.
> > Any driver added from now own, which is compatible with the TIS spec, will
> > only have to provide the underlying bus communication mechanisms.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes since v1:
> > -
> >  drivers/tpm/tpm2_tis_core.c | 545 ++++++++++++++++++++++++++++++++++++
> >  drivers/tpm/tpm_tis.h       |  40 +++
> >  include/tpm-v2.h            |   1 +
> >  3 files changed, 586 insertions(+)
> >  create mode 100644 drivers/tpm/tpm2_tis_core.c
> 
> [..]
> 
> > diff --git a/drivers/tpm/tpm_tis.h b/drivers/tpm/tpm_tis.h
> > index 2a160fe05c9a..fde3bb71f7c2 100644
> > --- a/drivers/tpm/tpm_tis.h
> > +++ b/drivers/tpm/tpm_tis.h
> > @@ -21,6 +21,37 @@
> >  #include <linux/compiler.h>
> >  #include <linux/types.h>
> >
> > +struct tpm_tis_phy_ops {
> > +       int (*read_bytes)(struct udevice *udev, u32 addr, u16 len,
> > +                         u8 *result);
> > +       int (*write_bytes)(struct udevice *udev, u32 addr, u16 len,
> > +                          const u8 *value);
> > +       int (*read16)(struct udevice *udev, u32 addr, u16 *result);
> > +       int (*read32)(struct udevice *udev, u32 addr, u32 *result);
> > +       int (*write32)(struct udevice *udev, u32 addr, u32 src);
> 
> A few points:
> 
> - these need comments
> - can we use uint instead of u32 for the value args? We should use
> native types where we can

Yes probably, I'll have a look
`
> - it seems like this should be a driver interface - see for example
> how cros_ec.c works. It has a shared code library and the drivers each
> implement an interface similar to the above, but on different buses.
> In general function pointers are a sign we should be using a driver
> 

I am not sure I am following, but I'll have a look on the code you pointed
out.

> > +};
> > +
> > +enum tis_int_flags {
> > +       TPM_GLOBAL_INT_ENABLE = 0x80000000,
> > +       TPM_INTF_BURST_COUNT_STATIC = 0x100,
> > +       TPM_INTF_CMD_READY_INT = 0x080,
> > +       TPM_INTF_INT_EDGE_FALLING = 0x040,
> > +       TPM_INTF_INT_EDGE_RISING = 0x020,
> > +       TPM_INTF_INT_LEVEL_LOW = 0x010,
> > +       TPM_INTF_INT_LEVEL_HIGH = 0x008,
> > +       TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
> > +       TPM_INTF_STS_VALID_INT = 0x002,
> > +       TPM_INTF_DATA_AVAIL_INT = 0x001,
> > +};
> > +
> > +#define TPM_ACCESS(l)                   (0x0000 | ((l) << 12))
> > +#define TPM_INT_ENABLE(l)               (0x0008 | ((l) << 12))
> > +#define TPM_STS(l)                      (0x0018 | ((l) << 12))
> > +#define TPM_DATA_FIFO(l)                (0x0024 | ((l) << 12))
> > +#define TPM_DID_VID(l)                  (0x0F00 | ((l) << 12))
> > +#define TPM_RID(l)                      (0x0F04 | ((l) << 12))
> > +#define TPM_INTF_CAPS(l)                (0x0014 | ((l) << 12))
> > +
> >  enum tpm_timeout {
> >         TPM_TIMEOUT_MS                  = 5,
> >         TIS_SHORT_TIMEOUT_MS            = 750,
> > @@ -43,6 +74,7 @@ struct tpm_chip {
> >         u8 rid;
> >         unsigned long timeout_a, timeout_b, timeout_c, timeout_d;  /* msec */
> >         ulong chip_type;
> > +       struct tpm_tis_phy_ops *phy_ops;
> >  };
> >
> >  struct tpm_input_header {
> > @@ -130,4 +162,12 @@ enum tis_status {
> >  };
> >  #endif
> >
> > +int tpm_tis_open(struct udevice *udev);
> > +int tpm_tis_close(struct udevice *udev);
> > +int tpm_tis_cleanup(struct udevice *udev);
> > +int tpm_tis_send(struct udevice *udev, const u8 *buf, size_t len);
> > +int tpm_tis_recv(struct udevice *udev, u8 *buf, size_t count);
> > +int tpm_tis_get_desc(struct udevice *udev, char *buf, int size);
> > +int tpm_tis_init(struct udevice *udev);
> > +void tpm_tis_ops_register(struct udevice *udev, struct tpm_tis_phy_ops *ops);
> 
> comments on all of these
> 
> >  #endif
> > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > index 247b38696766..3e48e358613f 100644
> > --- a/include/tpm-v2.h
> > +++ b/include/tpm-v2.h
> > @@ -378,6 +378,7 @@ enum {
> >         TPM_STS_DATA_EXPECT             = 1 << 3,
> >         TPM_STS_SELF_TEST_DONE          = 1 << 2,
> >         TPM_STS_RESPONSE_RETRY          = 1 << 1,
> > +       TPM_STS_READ_ZERO               = 0x23
> 
> Does this below in another patch?
> 

It's a general tpm2 update. I can move it to the driver patch if it makes
more sense.

> >  };
> >
> >  enum {
> > --
> > 2.32.0.rc0
> >
> 
> I feel that this API could be useful in reducing code duplication, but
> in fact it has just created more, so far as I can see from this series
> :-) So I think you should convert at least one driver to show its
> value (and not make things any worse).

The mmio tpm driver uses it and instead of ~700 lines (like the tpmv2 spi
driver) it drops down to ~100.  I don't have access to any other TPM
hardware to rewrite any of those.  

> 
> Regards,
> Simon

Re: [PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Simon Glass]

Hi Ilias,

On Mon, 12 Jul 2021 at 00:24, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Sat, Jul 10, 2021 at 06:00:57PM -0600, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > There's a lot of code duplication in U-Boot right now.  All the TPM TIS
> >
> > You mean in the TPM code I think.
> >
>
> Yes. Basically al TPM drivers duplicate this.
>
> > > compatible drivers we have at the moment have their own copy of a TIS
> > > implementation.
> > >
> > > So let's create a common layer which implements the core TIS functions.
> > > Any driver added from now own, which is compatible with the TIS spec, will
> > > only have to provide the underlying bus communication mechanisms.
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Changes since v1:
> > > -
> > >  drivers/tpm/tpm2_tis_core.c | 545 ++++++++++++++++++++++++++++++++++++
> > >  drivers/tpm/tpm_tis.h       |  40 +++
> > >  include/tpm-v2.h            |   1 +
> > >  3 files changed, 586 insertions(+)
> > >  create mode 100644 drivers/tpm/tpm2_tis_core.c
> >
> > [..]
> >
> > > diff --git a/drivers/tpm/tpm_tis.h b/drivers/tpm/tpm_tis.h
> > > index 2a160fe05c9a..fde3bb71f7c2 100644
> > > --- a/drivers/tpm/tpm_tis.h
> > > +++ b/drivers/tpm/tpm_tis.h
> > > @@ -21,6 +21,37 @@
> > >  #include <linux/compiler.h>
> > >  #include <linux/types.h>
> > >
> > > +struct tpm_tis_phy_ops {
> > > +       int (*read_bytes)(struct udevice *udev, u32 addr, u16 len,
> > > +                         u8 *result);
> > > +       int (*write_bytes)(struct udevice *udev, u32 addr, u16 len,
> > > +                          const u8 *value);
> > > +       int (*read16)(struct udevice *udev, u32 addr, u16 *result);
> > > +       int (*read32)(struct udevice *udev, u32 addr, u32 *result);
> > > +       int (*write32)(struct udevice *udev, u32 addr, u32 src);
> >
> > A few points:
> >
> > - these need comments
> > - can we use uint instead of u32 for the value args? We should use
> > native types where we can
>
> Yes probably, I'll have a look
> `
> > - it seems like this should be a driver interface - see for example
> > how cros_ec.c works. It has a shared code library and the drivers each
> > implement an interface similar to the above, but on different buses.
> > In general function pointers are a sign we should be using a driver
> >
>
> I am not sure I am following, but I'll have a look on the code you pointed
> out.
>
> > > +};
> > > +
> > > +enum tis_int_flags {
> > > +       TPM_GLOBAL_INT_ENABLE = 0x80000000,
> > > +       TPM_INTF_BURST_COUNT_STATIC = 0x100,
> > > +       TPM_INTF_CMD_READY_INT = 0x080,
> > > +       TPM_INTF_INT_EDGE_FALLING = 0x040,
> > > +       TPM_INTF_INT_EDGE_RISING = 0x020,
> > > +       TPM_INTF_INT_LEVEL_LOW = 0x010,
> > > +       TPM_INTF_INT_LEVEL_HIGH = 0x008,
> > > +       TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
> > > +       TPM_INTF_STS_VALID_INT = 0x002,
> > > +       TPM_INTF_DATA_AVAIL_INT = 0x001,
> > > +};
> > > +
> > > +#define TPM_ACCESS(l)                   (0x0000 | ((l) << 12))
> > > +#define TPM_INT_ENABLE(l)               (0x0008 | ((l) << 12))
> > > +#define TPM_STS(l)                      (0x0018 | ((l) << 12))
> > > +#define TPM_DATA_FIFO(l)                (0x0024 | ((l) << 12))
> > > +#define TPM_DID_VID(l)                  (0x0F00 | ((l) << 12))
> > > +#define TPM_RID(l)                      (0x0F04 | ((l) << 12))
> > > +#define TPM_INTF_CAPS(l)                (0x0014 | ((l) << 12))
> > > +
> > >  enum tpm_timeout {
> > >         TPM_TIMEOUT_MS                  = 5,
> > >         TIS_SHORT_TIMEOUT_MS            = 750,
> > > @@ -43,6 +74,7 @@ struct tpm_chip {
> > >         u8 rid;
> > >         unsigned long timeout_a, timeout_b, timeout_c, timeout_d;  /* msec */
> > >         ulong chip_type;
> > > +       struct tpm_tis_phy_ops *phy_ops;
> > >  };
> > >
> > >  struct tpm_input_header {
> > > @@ -130,4 +162,12 @@ enum tis_status {
> > >  };
> > >  #endif
> > >
> > > +int tpm_tis_open(struct udevice *udev);
> > > +int tpm_tis_close(struct udevice *udev);
> > > +int tpm_tis_cleanup(struct udevice *udev);
> > > +int tpm_tis_send(struct udevice *udev, const u8 *buf, size_t len);
> > > +int tpm_tis_recv(struct udevice *udev, u8 *buf, size_t count);
> > > +int tpm_tis_get_desc(struct udevice *udev, char *buf, int size);
> > > +int tpm_tis_init(struct udevice *udev);
> > > +void tpm_tis_ops_register(struct udevice *udev, struct tpm_tis_phy_ops *ops);
> >
> > comments on all of these
> >
> > >  #endif
> > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > index 247b38696766..3e48e358613f 100644
> > > --- a/include/tpm-v2.h
> > > +++ b/include/tpm-v2.h
> > > @@ -378,6 +378,7 @@ enum {
> > >         TPM_STS_DATA_EXPECT             = 1 << 3,
> > >         TPM_STS_SELF_TEST_DONE          = 1 << 2,
> > >         TPM_STS_RESPONSE_RETRY          = 1 << 1,
> > > +       TPM_STS_READ_ZERO               = 0x23
> >
> > Does this below in another patch?
> >
>
> It's a general tpm2 update. I can move it to the driver patch if it makes
> more sense.
>
> > >  };
> > >
> > >  enum {
> > > --
> > > 2.32.0.rc0
> > >
> >
> > I feel that this API could be useful in reducing code duplication, but
> > in fact it has just created more, so far as I can see from this series
> > :-) So I think you should convert at least one driver to show its
> > value (and not make things any worse).
>
> The mmio tpm driver uses it and instead of ~700 lines (like the tpmv2 spi
> driver) it drops down to ~100.  I don't have access to any other TPM
> hardware to rewrite any of those.

Yes, but I hope you see my point, that you have added a new interface.
It is definitely better than adding a new driver and duplicating all
the code, but it is still one more copy and in fact, the code is
duplicated.

Can you get access to TPM hardware? I see that you have offered to be
the maintainer for this subsystem, so I think that would be useful.
Can sandbox use your new API?

Regards,
Simon

Re: [PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Ilias Apalodimas]

> > > >         TPM_STS_SELF_TEST_DONE          = 1 << 2,

[...]

> > > >         TPM_STS_RESPONSE_RETRY          = 1 << 1,
> > > > +       TPM_STS_READ_ZERO               = 0x23
> > >
> > > Does this below in another patch?
> > >
> >
> > It's a general tpm2 update. I can move it to the driver patch if it makes
> > more sense.
> >
> > > >  };
> > > >
> > > >  enum {
> > > > --
> > > > 2.32.0.rc0
> > > >
> > >
> > > I feel that this API could be useful in reducing code duplication, but
> > > in fact it has just created more, so far as I can see from this series
> > > :-) So I think you should convert at least one driver to show its
> > > value (and not make things any worse).
> >
> > The mmio tpm driver uses it and instead of ~700 lines (like the tpmv2 spi
> > driver) it drops down to ~100.  I don't have access to any other TPM
> > hardware to rewrite any of those.
> 
> Yes, but I hope you see my point, that you have added a new interface.
> It is definitely better than adding a new driver and duplicating all
> the code, but it is still one more copy and in fact, the code is
> duplicated.
> 

I get the point but I don't exactly agree here.  It's not duplicated code.  
We need to plug in the mmio driver.  The original code was just doing what 
every TPM does.  It carried the TIS relevant code in the new driver.  
The new approach defines an API for everyone to use and the new driver uses it. 
So I don't see the duplication here.  You need the TIS code one way or the
other.  Now it's on a common interface for everyone to use.

> Can you get access to TPM hardware? I see that you have offered to be
> the maintainer for this subsystem, so I think that would be useful.
> Can sandbox use your new API?

It depends, is the sandbox TIS compatible? If it is sure we could use it. 
I offered to maintain the drivers because I wrote the API and I have an
idea of how TPMs should work.  If that means I'll have to go and get every
hardware we support, I'll just volunteer into maintaining the TIS layer.
Moreover I dont see why I should start porting drivers to use that API.  
People decided to duplicate that code in every driver (in fact multiple times).

I am happy to work with you on the cr50 i2c driver if that would help.

Regards
/Ilias

> 
> Regards,
> Simon

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Ilias Apalodimas]

Hi Simon, 
On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote:
> Hi Ilias,
> 
> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Add support for devices that expose a TPMv2 though MMIO.
> > Apart from those devices, we can use the driver in our QEMU setups and
> > test TPM related code which is difficult to achieve using the sandbox
> > driver (e.g test the EFI TCG2 protocol).
> >
> > It's worth noting that a previous patch added TPMv2 TIS core functions,
> > which the current driver is consuming.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes since v1:
> > - split off the tis core code into a different file
> >  drivers/tpm/Kconfig         |   9 +++
> >  drivers/tpm/Makefile        |   1 +
> >  drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 166 insertions(+)
> >  create mode 100644 drivers/tpm/tpm2_tis_mmio.c
> 
> Looks good to me

Thanks! 

> >
> > +struct tpm_tis_chip_data {
> > +       unsigned int pcr_count;
> > +       unsigned int pcr_select_min;
> > +       unsigned int time_before_first_cmd_ms;
> > +       void __iomem *iobase;
> > +};
> 
> comments
> 

You mean on all the internal functions?
Sure I can add them

> > +
> > +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len,
> > +                          u8 *result)
> > +{
> > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > +
> > +       while (len--)
> > +               *result++ = ioread8(drv_data->iobase + addr);
> 
> We normally put a blank line before the final return
> 

sure

> > +       return 0;
> > +}
> > +
> > +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len,
> > +                           const u8 *value)
> > +{
> > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > +
> > +       while (len--)
> > +               iowrite8(*value++, drv_data->iobase + addr);
> > +       return 0;
> > +}
> > +
> > +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result)
> > +{
> > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > +
> > +       *result = ioread16(drv_data->iobase + addr);
> > +       return 0;
> > +}
> > +
> > +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result)
> > +{
> > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > +
> > +       *result = ioread32(drv_data->iobase + addr);
> > +       return 0;
> > +}
> > +
> > +static int mmio_write32(struct udevice *udev, u32 addr, u32 value)
> > +{
> > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > +
> > +       iowrite32(value, drv_data->iobase + addr);
> > +       return 0;
> > +}
> > +
> > +static struct tpm_tis_phy_ops phy_ops = {
> 
> Should be a uclass interface.
>

Why? A uclass is supposed to describe and abstract hardware.  This is just
a specific implementation of a TPM, not all TPMs are TIS compliant. We already 
have a uclass for those.

> > +       .read_bytes = mmio_read_bytes,
> > +       .write_bytes = mmio_write_bytes,
> > +       .read16 = mmio_read16,
> > +       .read32 = mmio_read32,
> > +       .write32 = mmio_write32,
> > +};
> > +
> > +static int tpm_tis_probe(struct udevice *udev)
> > +{
> > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > +       struct tpm_chip_priv *priv = dev_get_uclass_priv(udev);
> > +       int ret = 0;
> > +       fdt_addr_t ioaddr;
> > +       u64 sz;
> > +
> > +       ioaddr = dev_read_addr(udev);
> > +       if (ioaddr == FDT_ADDR_T_NONE)
> > +               return -EINVAL;
> 
> consider this for easier debugging:
> 
>    return log_msg_ret("ioaddr", -EINVAL);
> 

Sure

> > +
> > +       ret = dev_read_u64(udev, "reg", &sz);
> > +       if (ret)
> > +               return -EINVAL;
> > +
> > +       drv_data->iobase = ioremap(ioaddr, sz);
> > +       log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz);
> 
> log_debug() I think?
>
Yea, that's a leftover of my initial code, were I needed to make sure the
ioremap worked.

> > +       tpm_tis_ops_register(udev, &phy_ops);
> > +       ret = tpm_tis_init(udev);
> > +       if (ret)
> > +               goto iounmap;
> > +
> > +       priv->pcr_count = drv_data->pcr_count;
> > +       priv->pcr_select_min = drv_data->pcr_select_min;
> > +       /*
> > +        * Although the driver probably works with a TPMv1 our Kconfig
> > +        * limits the driver to TPMv2 only
> > +        */
> > +       priv->version = TPM_V2;
> > +
> > +       return ret;
> > +iounmap:
> > +       iounmap(drv_data->iobase);
> > +       return -EINVAL;
> > +}
> > +
> > +static int tpm_tis_remove(struct udevice *udev)
> > +{
> > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > +
> > +       iounmap(drv_data->iobase);
> > +       return tpm_tis_cleanup(udev);
> > +}
> > +
> > +static const struct tpm_ops tpm_tis_ops = {
> > +       .open           = tpm_tis_open,
> > +       .close          = tpm_tis_close,
> > +       .get_desc       = tpm_tis_get_desc,
> > +       .send           = tpm_tis_send,
> > +       .recv           = tpm_tis_recv,
> > +       .cleanup        = tpm_tis_cleanup,
> > +};
> > +
> > +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
> > +       .pcr_count = 24,
> > +       .pcr_select_min = 3,
> > +};
> > +
> > +static const struct udevice_id tpm_tis_ids[] = {
> > +       {
> > +               .compatible = "tcg,tpm-tis-mmio",
> > +               .data = (ulong)&tpm_tis_std_chip_data,
> > +       },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(tpm_tis_mmio) = {
> > +       .name   = "tpm_tis_mmio",
> > +       .id     = UCLASS_TPM,
> > +       .of_match = tpm_tis_ids,
> > +       .ops    = &tpm_tis_ops,
> > +       .probe  = tpm_tis_probe,
> > +       .remove = tpm_tis_remove,
> > +       .priv_auto      = sizeof(struct tpm_chip),
> > +};
> > --
> > 2.32.0.rc0
> >
> 
> Regards,
> Simon

Thanks for looking at this
/Ilias

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Heinrich Schuchardt]

On 7/12/21 8:22 PM, Ilias Apalodimas wrote:
> Hi Simon,
> On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote:
>> Hi Ilias,
>>
>> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>>>
>>> Add support for devices that expose a TPMv2 though MMIO.
>>> Apart from those devices, we can use the driver in our QEMU setups and
>>> test TPM related code which is difficult to achieve using the sandbox
>>> driver (e.g test the EFI TCG2 protocol).
>>>
>>> It's worth noting that a previous patch added TPMv2 TIS core functions,
>>> which the current driver is consuming.
>>>
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>> Changes since v1:
>>> - split off the tis core code into a different file
>>>   drivers/tpm/Kconfig         |   9 +++
>>>   drivers/tpm/Makefile        |   1 +
>>>   drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 166 insertions(+)
>>>   create mode 100644 drivers/tpm/tpm2_tis_mmio.c
>>
>> Looks good to me
>
> Thanks!
>
>>>
>>> +struct tpm_tis_chip_data {
>>> +       unsigned int pcr_count;
>>> +       unsigned int pcr_select_min;
>>> +       unsigned int time_before_first_cmd_ms;
>>> +       void __iomem *iobase;
>>> +};
>>
>> comments
>>
>
> You mean on all the internal functions?
> Sure I can add them
>
>>> +
>>> +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len,
>>> +                          u8 *result)
>>> +{
>>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
>>> +
>>> +       while (len--)
>>> +               *result++ = ioread8(drv_data->iobase + addr);
>>
>> We normally put a blank line before the final return
>>
>
> sure
>
>>> +       return 0;
>>> +}
>>> +
>>> +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len,
>>> +                           const u8 *value)
>>> +{
>>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
>>> +
>>> +       while (len--)
>>> +               iowrite8(*value++, drv_data->iobase + addr);
>>> +       return 0;
>>> +}
>>> +
>>> +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result)
>>> +{
>>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
>>> +
>>> +       *result = ioread16(drv_data->iobase + addr);
>>> +       return 0;
>>> +}
>>> +
>>> +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result)
>>> +{
>>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
>>> +
>>> +       *result = ioread32(drv_data->iobase + addr);
>>> +       return 0;
>>> +}
>>> +
>>> +static int mmio_write32(struct udevice *udev, u32 addr, u32 value)
>>> +{
>>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
>>> +
>>> +       iowrite32(value, drv_data->iobase + addr);
>>> +       return 0;
>>> +}
>>> +
>>> +static struct tpm_tis_phy_ops phy_ops = {
>>
>> Should be a uclass interface.
>>
>
> Why? A uclass is supposed to describe and abstract hardware.  This is just
> a specific implementation of a TPM, not all TPMs are TIS compliant. We already
> have a uclass for those.

The design proposed by Ilias matches what we find in Linux for TPM.
Keeping the same structure should render porting of drivers for
additional devices easier.

Best regards

Heinrich

>
>>> +       .read_bytes = mmio_read_bytes,
>>> +       .write_bytes = mmio_write_bytes,
>>> +       .read16 = mmio_read16,
>>> +       .read32 = mmio_read32,
>>> +       .write32 = mmio_write32,
>>> +};
>>> +
>>> +static int tpm_tis_probe(struct udevice *udev)
>>> +{
>>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
>>> +       struct tpm_chip_priv *priv = dev_get_uclass_priv(udev);
>>> +       int ret = 0;
>>> +       fdt_addr_t ioaddr;
>>> +       u64 sz;
>>> +
>>> +       ioaddr = dev_read_addr(udev);
>>> +       if (ioaddr == FDT_ADDR_T_NONE)
>>> +               return -EINVAL;
>>
>> consider this for easier debugging:
>>
>>     return log_msg_ret("ioaddr", -EINVAL);
>>
>
> Sure
>
>>> +
>>> +       ret = dev_read_u64(udev, "reg", &sz);
>>> +       if (ret)
>>> +               return -EINVAL;
>>> +
>>> +       drv_data->iobase = ioremap(ioaddr, sz);
>>> +       log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz);
>>
>> log_debug() I think?
>>
> Yea, that's a leftover of my initial code, were I needed to make sure the
> ioremap worked.
>
>>> +       tpm_tis_ops_register(udev, &phy_ops);
>>> +       ret = tpm_tis_init(udev);
>>> +       if (ret)
>>> +               goto iounmap;
>>> +
>>> +       priv->pcr_count = drv_data->pcr_count;
>>> +       priv->pcr_select_min = drv_data->pcr_select_min;
>>> +       /*
>>> +        * Although the driver probably works with a TPMv1 our Kconfig
>>> +        * limits the driver to TPMv2 only
>>> +        */
>>> +       priv->version = TPM_V2;
>>> +
>>> +       return ret;
>>> +iounmap:
>>> +       iounmap(drv_data->iobase);
>>> +       return -EINVAL;
>>> +}
>>> +
>>> +static int tpm_tis_remove(struct udevice *udev)
>>> +{
>>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
>>> +
>>> +       iounmap(drv_data->iobase);
>>> +       return tpm_tis_cleanup(udev);
>>> +}
>>> +
>>> +static const struct tpm_ops tpm_tis_ops = {
>>> +       .open           = tpm_tis_open,
>>> +       .close          = tpm_tis_close,
>>> +       .get_desc       = tpm_tis_get_desc,
>>> +       .send           = tpm_tis_send,
>>> +       .recv           = tpm_tis_recv,
>>> +       .cleanup        = tpm_tis_cleanup,
>>> +};
>>> +
>>> +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
>>> +       .pcr_count = 24,
>>> +       .pcr_select_min = 3,
>>> +};
>>> +
>>> +static const struct udevice_id tpm_tis_ids[] = {
>>> +       {
>>> +               .compatible = "tcg,tpm-tis-mmio",
>>> +               .data = (ulong)&tpm_tis_std_chip_data,
>>> +       },
>>> +       { }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(tpm_tis_mmio) = {
>>> +       .name   = "tpm_tis_mmio",
>>> +       .id     = UCLASS_TPM,
>>> +       .of_match = tpm_tis_ids,
>>> +       .ops    = &tpm_tis_ops,
>>> +       .probe  = tpm_tis_probe,
>>> +       .remove = tpm_tis_remove,
>>> +       .priv_auto      = sizeof(struct tpm_chip),
>>> +};
>>> --
>>> 2.32.0.rc0
>>>
>>
>> Regards,
>> Simon
>
> Thanks for looking at this
> /Ilias
>

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Simon Glass]

Hi Ilias,

On Mon, 12 Jul 2021 at 12:22, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
> On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Add support for devices that expose a TPMv2 though MMIO.
> > > Apart from those devices, we can use the driver in our QEMU setups and
> > > test TPM related code which is difficult to achieve using the sandbox
> > > driver (e.g test the EFI TCG2 protocol).
> > >
> > > It's worth noting that a previous patch added TPMv2 TIS core functions,
> > > which the current driver is consuming.
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Changes since v1:
> > > - split off the tis core code into a different file
> > >  drivers/tpm/Kconfig         |   9 +++
> > >  drivers/tpm/Makefile        |   1 +
> > >  drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 166 insertions(+)
> > >  create mode 100644 drivers/tpm/tpm2_tis_mmio.c
> >
> > Looks good to me
>
> Thanks!
>
> > >
> > > +struct tpm_tis_chip_data {
> > > +       unsigned int pcr_count;
> > > +       unsigned int pcr_select_min;
> > > +       unsigned int time_before_first_cmd_ms;
> > > +       void __iomem *iobase;
> > > +};
> >
> > comments
> >
>
> You mean on all the internal functions?
> Sure I can add them

I mean for struct members.

For internal functions a comment is a good idea depending on how
valuable it is - things like return values, args. But if just
implementing a fully documented API, it may not really add much.

>
> > > +
> > > +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len,
> > > +                          u8 *result)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       while (len--)
> > > +               *result++ = ioread8(drv_data->iobase + addr);
> >
> > We normally put a blank line before the final return
> >
>
> sure
>
> > > +       return 0;
> > > +}
> > > +
> > > +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len,
> > > +                           const u8 *value)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       while (len--)
> > > +               iowrite8(*value++, drv_data->iobase + addr);
> > > +       return 0;
> > > +}
> > > +
> > > +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       *result = ioread16(drv_data->iobase + addr);
> > > +       return 0;
> > > +}
> > > +
> > > +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       *result = ioread32(drv_data->iobase + addr);
> > > +       return 0;
> > > +}
> > > +
> > > +static int mmio_write32(struct udevice *udev, u32 addr, u32 value)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       iowrite32(value, drv_data->iobase + addr);
> > > +       return 0;
> > > +}
> > > +
> > > +static struct tpm_tis_phy_ops phy_ops = {
> >
> > Should be a uclass interface.
> >
>
> Why? A uclass is supposed to describe and abstract hardware.  This is just
> a specific implementation of a TPM, not all TPMs are TIS compliant. We already
> have a uclass for those.

Who told you that a uclass is supposed to describe and abstract hardware? :-)

The uclass is how driver model does APIs, so normally a uclass would
be used for any API. There are exceptions, but this one actually looks
like a useful interface we should have.

>
> > > +       .read_bytes = mmio_read_bytes,
> > > +       .write_bytes = mmio_write_bytes,
> > > +       .read16 = mmio_read16,
> > > +       .read32 = mmio_read32,
> > > +       .write32 = mmio_write32,
> > > +};
> > > +
> > > +static int tpm_tis_probe(struct udevice *udev)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +       struct tpm_chip_priv *priv = dev_get_uclass_priv(udev);
> > > +       int ret = 0;
> > > +       fdt_addr_t ioaddr;
> > > +       u64 sz;
> > > +
> > > +       ioaddr = dev_read_addr(udev);
> > > +       if (ioaddr == FDT_ADDR_T_NONE)
> > > +               return -EINVAL;
> >
> > consider this for easier debugging:
> >
> >    return log_msg_ret("ioaddr", -EINVAL);
> >
>
> Sure
>
> > > +
> > > +       ret = dev_read_u64(udev, "reg", &sz);
> > > +       if (ret)
> > > +               return -EINVAL;
> > > +
> > > +       drv_data->iobase = ioremap(ioaddr, sz);
> > > +       log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz);
> >
> > log_debug() I think?
> >
> Yea, that's a leftover of my initial code, were I needed to make sure the
> ioremap worked.
>
> > > +       tpm_tis_ops_register(udev, &phy_ops);
> > > +       ret = tpm_tis_init(udev);
> > > +       if (ret)
> > > +               goto iounmap;
> > > +
> > > +       priv->pcr_count = drv_data->pcr_count;
> > > +       priv->pcr_select_min = drv_data->pcr_select_min;
> > > +       /*
> > > +        * Although the driver probably works with a TPMv1 our Kconfig
> > > +        * limits the driver to TPMv2 only
> > > +        */
> > > +       priv->version = TPM_V2;
> > > +
> > > +       return ret;
> > > +iounmap:
> > > +       iounmap(drv_data->iobase);
> > > +       return -EINVAL;
> > > +}
> > > +
> > > +static int tpm_tis_remove(struct udevice *udev)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       iounmap(drv_data->iobase);
> > > +       return tpm_tis_cleanup(udev);
> > > +}
> > > +
> > > +static const struct tpm_ops tpm_tis_ops = {
> > > +       .open           = tpm_tis_open,
> > > +       .close          = tpm_tis_close,
> > > +       .get_desc       = tpm_tis_get_desc,
> > > +       .send           = tpm_tis_send,
> > > +       .recv           = tpm_tis_recv,
> > > +       .cleanup        = tpm_tis_cleanup,
> > > +};
> > > +
> > > +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
> > > +       .pcr_count = 24,
> > > +       .pcr_select_min = 3,
> > > +};
> > > +
> > > +static const struct udevice_id tpm_tis_ids[] = {
> > > +       {
> > > +               .compatible = "tcg,tpm-tis-mmio",
> > > +               .data = (ulong)&tpm_tis_std_chip_data,
> > > +       },
> > > +       { }
> > > +};
> > > +
> > > +U_BOOT_DRIVER(tpm_tis_mmio) = {
> > > +       .name   = "tpm_tis_mmio",
> > > +       .id     = UCLASS_TPM,
> > > +       .of_match = tpm_tis_ids,
> > > +       .ops    = &tpm_tis_ops,
> > > +       .probe  = tpm_tis_probe,
> > > +       .remove = tpm_tis_remove,
> > > +       .priv_auto      = sizeof(struct tpm_chip),
> > > +};
> > > --
> > > 2.32.0.rc0
> > >
> >
> > Regards,
> > Simon
>
> Thanks for looking at this
> /Ilias

Likewise.

Regards,
Simon

Re: [PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Simon Glass]

Hi Ilias,

On Mon, 12 Jul 2021 at 08:03, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> > > > >         TPM_STS_SELF_TEST_DONE          = 1 << 2,
>
> [...]
>
> > > > >         TPM_STS_RESPONSE_RETRY          = 1 << 1,
> > > > > +       TPM_STS_READ_ZERO               = 0x23
> > > >
> > > > Does this below in another patch?
> > > >
> > >
> > > It's a general tpm2 update. I can move it to the driver patch if it makes
> > > more sense.
> > >
> > > > >  };
> > > > >
> > > > >  enum {
> > > > > --
> > > > > 2.32.0.rc0
> > > > >
> > > >
> > > > I feel that this API could be useful in reducing code duplication, but
> > > > in fact it has just created more, so far as I can see from this series
> > > > :-) So I think you should convert at least one driver to show its
> > > > value (and not make things any worse).
> > >
> > > The mmio tpm driver uses it and instead of ~700 lines (like the tpmv2 spi
> > > driver) it drops down to ~100.  I don't have access to any other TPM
> > > hardware to rewrite any of those.
> >
> > Yes, but I hope you see my point, that you have added a new interface.
> > It is definitely better than adding a new driver and duplicating all
> > the code, but it is still one more copy and in fact, the code is
> > duplicated.
> >
>
> I get the point but I don't exactly agree here.  It's not duplicated code.
> We need to plug in the mmio driver.  The original code was just doing what
> every TPM does.  It carried the TIS relevant code in the new driver.
> The new approach defines an API for everyone to use and the new driver uses it.
> So I don't see the duplication here.  You need the TIS code one way or the
> other.  Now it's on a common interface for everyone to use.

Well how about converting a TPM blindly and then we'll find someone to test it?

>
> > Can you get access to TPM hardware? I see that you have offered to be
> > the maintainer for this subsystem, so I think that would be useful.
> > Can sandbox use your new API?
>
> It depends, is the sandbox TIS compatible? If it is sure we could use it.

At present sandbox implements the tpm_ops API. So if we did that we
would need to tear it apart to insert this new API as well.

> I offered to maintain the drivers because I wrote the API and I have an
> idea of how TPMs should work.  If that means I'll have to go and get every
> hardware we support, I'll just volunteer into maintaining the TIS layer.
> Moreover I dont see why I should start porting drivers to use that API.
> People decided to duplicate that code in every driver (in fact multiple times).

See https://xkcd.com/927/ :-)

>
> I am happy to work with you on the cr50 i2c driver if that would help.

Sure that might be easier as I can definitely test it.

Regards,
Siomn

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Simon Glass]

Hi Heinrich,

On Mon, 12 Jul 2021 at 13:19, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 7/12/21 8:22 PM, Ilias Apalodimas wrote:
> > Hi Simon,
> > On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote:
> >> Hi Ilias,
> >>
> >> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
> >> <ilias.apalodimas@linaro.org> wrote:
> >>>
> >>> Add support for devices that expose a TPMv2 though MMIO.
> >>> Apart from those devices, we can use the driver in our QEMU setups and
> >>> test TPM related code which is difficult to achieve using the sandbox
> >>> driver (e.g test the EFI TCG2 protocol).
> >>>
> >>> It's worth noting that a previous patch added TPMv2 TIS core functions,
> >>> which the current driver is consuming.
> >>>
> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> ---
> >>> Changes since v1:
> >>> - split off the tis core code into a different file
> >>>   drivers/tpm/Kconfig         |   9 +++
> >>>   drivers/tpm/Makefile        |   1 +
> >>>   drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++
> >>>   3 files changed, 166 insertions(+)
> >>>   create mode 100644 drivers/tpm/tpm2_tis_mmio.c
> >>
> >> Looks good to me
> >
> > Thanks!
> >
> >>>
> >>> +struct tpm_tis_chip_data {
> >>> +       unsigned int pcr_count;
> >>> +       unsigned int pcr_select_min;
> >>> +       unsigned int time_before_first_cmd_ms;
> >>> +       void __iomem *iobase;
> >>> +};
> >>
> >> comments
> >>
> >
> > You mean on all the internal functions?
> > Sure I can add them
> >
> >>> +
> >>> +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len,
> >>> +                          u8 *result)
> >>> +{
> >>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> >>> +
> >>> +       while (len--)
> >>> +               *result++ = ioread8(drv_data->iobase + addr);
> >>
> >> We normally put a blank line before the final return
> >>
> >
> > sure
> >
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len,
> >>> +                           const u8 *value)
> >>> +{
> >>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> >>> +
> >>> +       while (len--)
> >>> +               iowrite8(*value++, drv_data->iobase + addr);
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result)
> >>> +{
> >>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> >>> +
> >>> +       *result = ioread16(drv_data->iobase + addr);
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result)
> >>> +{
> >>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> >>> +
> >>> +       *result = ioread32(drv_data->iobase + addr);
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +static int mmio_write32(struct udevice *udev, u32 addr, u32 value)
> >>> +{
> >>> +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> >>> +
> >>> +       iowrite32(value, drv_data->iobase + addr);
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +static struct tpm_tis_phy_ops phy_ops = {
> >>
> >> Should be a uclass interface.
> >>
> >
> > Why? A uclass is supposed to describe and abstract hardware.  This is just
> > a specific implementation of a TPM, not all TPMs are TIS compliant. We already
> > have a uclass for those.
>
> The design proposed by Ilias matches what we find in Linux for TPM.
> Keeping the same structure should render porting of drivers for
> additional devices easier.

Can you please be more specific in your comment? What change are you asking for?

Regards,
Simon

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Heinrich Schuchardt]

Am 12. Juli 2021 21:43:53 MESZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Mon, 12 Jul 2021 at 13:19, Heinrich Schuchardt <xypron.glpk@gmx.de>
>wrote:
>>
>> On 7/12/21 8:22 PM, Ilias Apalodimas wrote:
>> > Hi Simon,
>> > On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote:
>> >> Hi Ilias,
>> >>
>> >> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
>> >> <ilias.apalodimas@linaro.org> wrote:
>> >>>
>> >>> Add support for devices that expose a TPMv2 though MMIO.
>> >>> Apart from those devices, we can use the driver in our QEMU
>setups and
>> >>> test TPM related code which is difficult to achieve using the
>sandbox
>> >>> driver (e.g test the EFI TCG2 protocol).
>> >>>
>> >>> It's worth noting that a previous patch added TPMv2 TIS core
>functions,
>> >>> which the current driver is consuming.
>> >>>
>> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> >>> ---
>> >>> Changes since v1:
>> >>> - split off the tis core code into a different file
>> >>>   drivers/tpm/Kconfig         |   9 +++
>> >>>   drivers/tpm/Makefile        |   1 +
>> >>>   drivers/tpm/tpm2_tis_mmio.c | 156
>++++++++++++++++++++++++++++++++++++
>> >>>   3 files changed, 166 insertions(+)
>> >>>   create mode 100644 drivers/tpm/tpm2_tis_mmio.c
>> >>
>> >> Looks good to me
>> >
>> > Thanks!
>> >
>> >>>
>> >>> +struct tpm_tis_chip_data {
>> >>> +       unsigned int pcr_count;
>> >>> +       unsigned int pcr_select_min;
>> >>> +       unsigned int time_before_first_cmd_ms;
>> >>> +       void __iomem *iobase;
>> >>> +};
>> >>
>> >> comments
>> >>
>> >
>> > You mean on all the internal functions?
>> > Sure I can add them
>> >
>> >>> +
>> >>> +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16
>len,
>> >>> +                          u8 *result)
>> >>> +{
>> >>> +       struct tpm_tis_chip_data *drv_data = (void
>*)dev_get_driver_data(udev);
>> >>> +
>> >>> +       while (len--)
>> >>> +               *result++ = ioread8(drv_data->iobase + addr);
>> >>
>> >> We normally put a blank line before the final return
>> >>
>> >
>> > sure
>> >
>> >>> +       return 0;
>> >>> +}
>> >>> +
>> >>> +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16
>len,
>> >>> +                           const u8 *value)
>> >>> +{
>> >>> +       struct tpm_tis_chip_data *drv_data = (void
>*)dev_get_driver_data(udev);
>> >>> +
>> >>> +       while (len--)
>> >>> +               iowrite8(*value++, drv_data->iobase + addr);
>> >>> +       return 0;
>> >>> +}
>> >>> +
>> >>> +static int mmio_read16(struct udevice *udev, u32 addr, u16
>*result)
>> >>> +{
>> >>> +       struct tpm_tis_chip_data *drv_data = (void
>*)dev_get_driver_data(udev);
>> >>> +
>> >>> +       *result = ioread16(drv_data->iobase + addr);
>> >>> +       return 0;
>> >>> +}
>> >>> +
>> >>> +static int mmio_read32(struct udevice *udev, u32 addr, u32
>*result)
>> >>> +{
>> >>> +       struct tpm_tis_chip_data *drv_data = (void
>*)dev_get_driver_data(udev);
>> >>> +
>> >>> +       *result = ioread32(drv_data->iobase + addr);
>> >>> +       return 0;
>> >>> +}
>> >>> +
>> >>> +static int mmio_write32(struct udevice *udev, u32 addr, u32
>value)
>> >>> +{
>> >>> +       struct tpm_tis_chip_data *drv_data = (void
>*)dev_get_driver_data(udev);
>> >>> +
>> >>> +       iowrite32(value, drv_data->iobase + addr);
>> >>> +       return 0;
>> >>> +}
>> >>> +
>> >>> +static struct tpm_tis_phy_ops phy_ops = {
>> >>
>> >> Should be a uclass interface.
>> >>
>> >
>> > Why? A uclass is supposed to describe and abstract hardware.  This
>is just
>> > a specific implementation of a TPM, not all TPMs are TIS compliant.
>We already
>> > have a uclass for those.
>>
>> The design proposed by Ilias matches what we find in Linux for TPM.
>> Keeping the same structure should render porting of drivers for
>> additional devices easier.
>
>Can you please be more specific in your comment? What change are you
>asking for?
>
>Regards,
>Simon

None. I think the design is ok as is.

I was responding to your idea of adding another uclass.

Best regards

Heinrich

Re: [PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Ilias Apalodimas]

Hi Simon,
> > >

[...]

> > > Yes, but I hope you see my point, that you have added a new interface.
> > > It is definitely better than adding a new driver and duplicating all
> > > the code, but it is still one more copy and in fact, the code is
> > > duplicated.
> > >
> >
> > I get the point but I don't exactly agree here.  It's not duplicated code.
> > We need to plug in the mmio driver.  The original code was just doing what
> > every TPM does.  It carried the TIS relevant code in the new driver.
> > The new approach defines an API for everyone to use and the new driver uses it.
> > So I don't see the duplication here.  You need the TIS code one way or the
> > other.  Now it's on a common interface for everyone to use.
> 
> Well how about converting a TPM blindly and then we'll find someone to test it?

Let's do the cr50

> 
> >
> > > Can you get access to TPM hardware? I see that you have offered to be
> > > the maintainer for this subsystem, so I think that would be useful.
> > > Can sandbox use your new API?
> >
> > It depends, is the sandbox TIS compatible? If it is sure we could use it.
> 
> At present sandbox implements the tpm_ops API. So if we did that we
> would need to tear it apart to insert this new API as well.

Ok then that might make too much sense for the sandbox.

> 
> > I offered to maintain the drivers because I wrote the API and I have an
> > idea of how TPMs should work.  If that means I'll have to go and get every
> > hardware we support, I'll just volunteer into maintaining the TIS layer.
> > Moreover I dont see why I should start porting drivers to use that API.
> > People decided to duplicate that code in every driver (in fact multiple times).
> 
> See https://xkcd.com/927/ :-)
> 

Yea I don't disagree with that.  That's one of the points of adding myself
as a maintainer for the entire tpm/drivers/*.  I can just reply 'no thanks'
at least for new drivers that don't use it.  But frankly I don't see why,
adding a new drivers, while using the TIS API boils down to a few lines of
code defining the bus accesses 

> >
> > I am happy to work with you on the cr50 i2c driver if that would help.
> 
> Sure that might be easier as I can definitely test it.

Ok, let me have a look at that, I still think the patch should go in
regardless though.  We can always send a follow up for cr50 once we are
done testing

Cheers
/Ilias
> 
> Regards,
> Siomn

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Simon Glass]

Hi Heinrich,

On Mon, 12 Jul 2021 at 16:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 12. Juli 2021 21:43:53 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Mon, 12 Jul 2021 at 13:19, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >wrote:
> >>
> >> On 7/12/21 8:22 PM, Ilias Apalodimas wrote:
> >> > Hi Simon,
> >> > On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote:
> >> >> Hi Ilias,
> >> >>
> >> >> On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
> >> >> <ilias.apalodimas@linaro.org> wrote:
> >> >>>
> >> >>> Add support for devices that expose a TPMv2 though MMIO.
> >> >>> Apart from those devices, we can use the driver in our QEMU
> >setups and
> >> >>> test TPM related code which is difficult to achieve using the
> >sandbox
> >> >>> driver (e.g test the EFI TCG2 protocol).
> >> >>>
> >> >>> It's worth noting that a previous patch added TPMv2 TIS core
> >functions,
> >> >>> which the current driver is consuming.
> >> >>>
> >> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> >>> ---
> >> >>> Changes since v1:
> >> >>> - split off the tis core code into a different file
> >> >>>   drivers/tpm/Kconfig         |   9 +++
> >> >>>   drivers/tpm/Makefile        |   1 +
> >> >>>   drivers/tpm/tpm2_tis_mmio.c | 156
> >++++++++++++++++++++++++++++++++++++
> >> >>>   3 files changed, 166 insertions(+)
> >> >>>   create mode 100644 drivers/tpm/tpm2_tis_mmio.c
> >> >>
> >> >> Looks good to me
> >> >
> >> > Thanks!
> >> >
> >> >>>
> >> >>> +struct tpm_tis_chip_data {
> >> >>> +       unsigned int pcr_count;
> >> >>> +       unsigned int pcr_select_min;
> >> >>> +       unsigned int time_before_first_cmd_ms;
> >> >>> +       void __iomem *iobase;
> >> >>> +};
> >> >>
> >> >> comments
> >> >>
> >> >
> >> > You mean on all the internal functions?
> >> > Sure I can add them
> >> >
> >> >>> +
> >> >>> +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16
> >len,
> >> >>> +                          u8 *result)
> >> >>> +{
> >> >>> +       struct tpm_tis_chip_data *drv_data = (void
> >*)dev_get_driver_data(udev);
> >> >>> +
> >> >>> +       while (len--)
> >> >>> +               *result++ = ioread8(drv_data->iobase + addr);
> >> >>
> >> >> We normally put a blank line before the final return
> >> >>
> >> >
> >> > sure
> >> >
> >> >>> +       return 0;
> >> >>> +}
> >> >>> +
> >> >>> +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16
> >len,
> >> >>> +                           const u8 *value)
> >> >>> +{
> >> >>> +       struct tpm_tis_chip_data *drv_data = (void
> >*)dev_get_driver_data(udev);
> >> >>> +
> >> >>> +       while (len--)
> >> >>> +               iowrite8(*value++, drv_data->iobase + addr);
> >> >>> +       return 0;
> >> >>> +}
> >> >>> +
> >> >>> +static int mmio_read16(struct udevice *udev, u32 addr, u16
> >*result)
> >> >>> +{
> >> >>> +       struct tpm_tis_chip_data *drv_data = (void
> >*)dev_get_driver_data(udev);
> >> >>> +
> >> >>> +       *result = ioread16(drv_data->iobase + addr);
> >> >>> +       return 0;
> >> >>> +}
> >> >>> +
> >> >>> +static int mmio_read32(struct udevice *udev, u32 addr, u32
> >*result)
> >> >>> +{
> >> >>> +       struct tpm_tis_chip_data *drv_data = (void
> >*)dev_get_driver_data(udev);
> >> >>> +
> >> >>> +       *result = ioread32(drv_data->iobase + addr);
> >> >>> +       return 0;
> >> >>> +}
> >> >>> +
> >> >>> +static int mmio_write32(struct udevice *udev, u32 addr, u32
> >value)
> >> >>> +{
> >> >>> +       struct tpm_tis_chip_data *drv_data = (void
> >*)dev_get_driver_data(udev);
> >> >>> +
> >> >>> +       iowrite32(value, drv_data->iobase + addr);
> >> >>> +       return 0;
> >> >>> +}
> >> >>> +
> >> >>> +static struct tpm_tis_phy_ops phy_ops = {
> >> >>
> >> >> Should be a uclass interface.
> >> >>
> >> >
> >> > Why? A uclass is supposed to describe and abstract hardware.  This
> >is just
> >> > a specific implementation of a TPM, not all TPMs are TIS compliant.
> >We already
> >> > have a uclass for those.
> >>
> >> The design proposed by Ilias matches what we find in Linux for TPM.
> >> Keeping the same structure should render porting of drivers for
> >> additional devices easier.
> >
> >Can you please be more specific in your comment? What change are you
> >asking for?
> >
> >Regards,
> >Simon
>
> None. I think the design is ok as is.
>
> I was responding to your idea of adding another uclass.

It seems obvious to me that the proposed API is useful outside TPMs.
It provides an MMIO interface, after all.

Regards,
Simon

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Ilias Apalodimas]

[...]
> > > Should be a uclass interface.
> > >
> >
> > Why? A uclass is supposed to describe and abstract hardware.  This is just
> > a specific implementation of a TPM, not all TPMs are TIS compliant. We already
> > have a uclass for those.
> 
> Who told you that a uclass is supposed to describe and abstract hardware? :-)
> 

That's what I've mostly seen it used for, maybe i got the idea wrong.

> The uclass is how driver model does APIs, so normally a uclass would
> be used for any API. There are exceptions, but this one actually looks
> like a useful interface we should have.
> 

the point is we already have a uclass for tpm devices.  So why should the
we add another one that just describes the TIS interface?

> >
> > > > +       .read_bytes = mmio_read_bytes,
> > > > +       .write_bytes = mmio_write_bytes,
> > > > +       .read16 = mmio_read16,
> > > > +       .read32 = mmio_read32,
> > > > +       .write32 = mmio_write32,
> > > > +};
> > > > +
> > > > +static int tpm_tis_probe(struct udevice *udev)
> > > > +{
> > > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > > +       struct tpm_chip_priv *priv = dev_get_uclass_priv(udev);
> > > > +       int ret = 0;
> > > > +       fdt_addr_t ioaddr;
> > > > +       u64 sz;
> > > > +
> > > > +       ioaddr = dev_read_addr(udev);
> > > > +       if (ioaddr == FDT_ADDR_T_NONE)
> > > > +               return -EINVAL;
> > >
> > > consider this for easier debugging:
> > >
> > >    return log_msg_ret("ioaddr", -EINVAL);
> > >
> >
> > Sure
> >
> > > > +
> > > > +       ret = dev_read_u64(udev, "reg", &sz);
> > > > +       if (ret)
> > > > +               return -EINVAL;
> > > > +
> > > > +       drv_data->iobase = ioremap(ioaddr, sz);
> > > > +       log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz);
> > >
> > > log_debug() I think?
> > >
> > Yea, that's a leftover of my initial code, were I needed to make sure the
> > ioremap worked.
> >
> > > > +       tpm_tis_ops_register(udev, &phy_ops);
> > > > +       ret = tpm_tis_init(udev);
> > > > +       if (ret)
> > > > +               goto iounmap;
> > > > +
> > > > +       priv->pcr_count = drv_data->pcr_count;
> > > > +       priv->pcr_select_min = drv_data->pcr_select_min;
> > > > +       /*
> > > > +        * Although the driver probably works with a TPMv1 our Kconfig
> > > > +        * limits the driver to TPMv2 only
> > > > +        */
> > > > +       priv->version = TPM_V2;
> > > > +
> > > > +       return ret;
> > > > +iounmap:
> > > > +       iounmap(drv_data->iobase);
> > > > +       return -EINVAL;
> > > > +}
> > > > +
> > > > +static int tpm_tis_remove(struct udevice *udev)
> > > > +{
> > > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > > +
> > > > +       iounmap(drv_data->iobase);
> > > > +       return tpm_tis_cleanup(udev);
> > > > +}
> > > > +
> > > > +static const struct tpm_ops tpm_tis_ops = {
> > > > +       .open           = tpm_tis_open,
> > > > +       .close          = tpm_tis_close,
> > > > +       .get_desc       = tpm_tis_get_desc,
> > > > +       .send           = tpm_tis_send,
> > > > +       .recv           = tpm_tis_recv,
> > > > +       .cleanup        = tpm_tis_cleanup,
> > > > +};
> > > > +
> > > > +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
> > > > +       .pcr_count = 24,
> > > > +       .pcr_select_min = 3,
> > > > +};
> > > > +
> > > > +static const struct udevice_id tpm_tis_ids[] = {
> > > > +       {
> > > > +               .compatible = "tcg,tpm-tis-mmio",
> > > > +               .data = (ulong)&tpm_tis_std_chip_data,
> > > > +       },
> > > > +       { }
> > > > +};
> > > > +
> > > > +U_BOOT_DRIVER(tpm_tis_mmio) = {
> > > > +       .name   = "tpm_tis_mmio",
> > > > +       .id     = UCLASS_TPM,
> > > > +       .of_match = tpm_tis_ids,
> > > > +       .ops    = &tpm_tis_ops,
> > > > +       .probe  = tpm_tis_probe,
> > > > +       .remove = tpm_tis_remove,
> > > > +       .priv_auto      = sizeof(struct tpm_chip),
> > > > +};
> > > > --
> > > > 2.32.0.rc0
> > > >
> > >
> > > Regards,
> > > Simon
> >
> > Thanks for looking at this
> > /Ilias
> 
> Likewise.
> 
> Regards,
> Simon

Re: [PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Simon Glass]

Hi Ilias,

On Mon, 12 Jul 2021 at 23:51, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
> > > >
>
> [...]
>
> > > > Yes, but I hope you see my point, that you have added a new interface.
> > > > It is definitely better than adding a new driver and duplicating all
> > > > the code, but it is still one more copy and in fact, the code is
> > > > duplicated.
> > > >
> > >
> > > I get the point but I don't exactly agree here.  It's not duplicated code.
> > > We need to plug in the mmio driver.  The original code was just doing what
> > > every TPM does.  It carried the TIS relevant code in the new driver.
> > > The new approach defines an API for everyone to use and the new driver uses it.
> > > So I don't see the duplication here.  You need the TIS code one way or the
> > > other.  Now it's on a common interface for everyone to use.
> >
> > Well how about converting a TPM blindly and then we'll find someone to test it?
>
> Let's do the cr50
>
> >
> > >
> > > > Can you get access to TPM hardware? I see that you have offered to be
> > > > the maintainer for this subsystem, so I think that would be useful.
> > > > Can sandbox use your new API?
> > >
> > > It depends, is the sandbox TIS compatible? If it is sure we could use it.
> >
> > At present sandbox implements the tpm_ops API. So if we did that we
> > would need to tear it apart to insert this new API as well.
>
> Ok then that might make too much sense for the sandbox.
>
> >
> > > I offered to maintain the drivers because I wrote the API and I have an
> > > idea of how TPMs should work.  If that means I'll have to go and get every
> > > hardware we support, I'll just volunteer into maintaining the TIS layer.
> > > Moreover I dont see why I should start porting drivers to use that API.
> > > People decided to duplicate that code in every driver (in fact multiple times).
> >
> > See https://xkcd.com/927/ :-)
> >
>
> Yea I don't disagree with that.  That's one of the points of adding myself
> as a maintainer for the entire tpm/drivers/*.  I can just reply 'no thanks'
> at least for new drivers that don't use it.  But frankly I don't see why,
> adding a new drivers, while using the TIS API boils down to a few lines of
> code defining the bus accesses
>
> > >
> > > I am happy to work with you on the cr50 i2c driver if that would help.
> >
> > Sure that might be easier as I can definitely test it.
>
> Ok, let me have a look at that, I still think the patch should go in
> regardless though.  We can always send a follow up for cr50 once we are
> done testing

Well if you set it up as an MMIO uclass with a sandbox test then I
think there is enough value to this approach and we avoid the xkcd
issue.

Regards,
Simon

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Simon Glass]

Hi Ilias,

On Tue, 13 Jul 2021 at 14:11, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
>
> [...]
> > > > Should be a uclass interface.
> > > >
> > >
> > > Why? A uclass is supposed to describe and abstract hardware.  This is just
> > > a specific implementation of a TPM, not all TPMs are TIS compliant. We already
> > > have a uclass for those.
> >
> > Who told you that a uclass is supposed to describe and abstract hardware? :-)
> >
>
> That's what I've mostly seen it used for, maybe i got the idea wrong.

A uclass is basically a software construct. It is an interface between
clients and the driver, typically. Quite often the uclass is an
interface on top of the hardware (actually the driver). But quite
often it is not. For example, we use an GPIO uclass to access a pmic's
GPIOs, we use an I2C uclass to access the cros_ec I2C tunnel. Anywhere
where it makes sense to have an abstraction, we use a uclass.

>
> > The uclass is how driver model does APIs, so normally a uclass would
> > be used for any API. There are exceptions, but this one actually looks
> > like a useful interface we should have.
> >
>
> the point is we already have a uclass for tpm devices.  So why should the
> we add another one that just describes the TIS interface?

You have already added another API, right? All we are discussing is
whether it should be a uclass or not. Unless there is a very good
reason, we should avoid creating custom interfaces that don't use
driver model. I actually think the interface you've created (MMIO)
will be very useful as a uclass.

[..]

Regards,
Simon

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Ilias Apalodimas]

On Tue, Jul 13, 2021 at 08:49:21PM -0600, Simon Glass wrote:
> Hi Ilias,
> 
> On Tue, 13 Jul 2021 at 14:11, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> >
> > [...]
> > > > > Should be a uclass interface.
> > > > >
> > > >
> > > > Why? A uclass is supposed to describe and abstract hardware.  This is just
> > > > a specific implementation of a TPM, not all TPMs are TIS compliant. We already
> > > > have a uclass for those.
> > >
> > > Who told you that a uclass is supposed to describe and abstract hardware? :-)
> > >
> >
> > That's what I've mostly seen it used for, maybe i got the idea wrong.
> 
> A uclass is basically a software construct. It is an interface between
> clients and the driver, typically. Quite often the uclass is an
> interface on top of the hardware (actually the driver). But quite
> often it is not. For example, we use an GPIO uclass to access a pmic's
> GPIOs, we use an I2C uclass to access the cros_ec I2C tunnel. Anywhere
> where it makes sense to have an abstraction, we use a uclass.
> 
> >
> > > The uclass is how driver model does APIs, so normally a uclass would
> > > be used for any API. There are exceptions, but this one actually looks
> > > like a useful interface we should have.
> > >
> >
> > the point is we already have a uclass for tpm devices.  So why should the
> > we add another one that just describes the TIS interface?
> 
> You have already added another API, right? All we are discussing is
> whether it should be a uclass or not. Unless there is a very good
> reason, we should avoid creating custom interfaces that don't use
> driver model. I actually think the interface you've created (MMIO)
> will be very useful as a uclass.
> 

So you are basically looking into adding something similar to
dm_i2c_read/dm_i2c_write etc?
I assume this is gong to be the default read method passed on the TIS API
when we want to support i2c TPMs.
For the MMIO case that would essentially mean, move the functions on a
different file, add them on a header and define a UCLASS_DRIVER with only
the .id and .name defined?

Thanks
/Ilias

> [..]
> 
> Regards,
> Simon

Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Simon Glass]

Hi Ilias,

On Tue, 13 Jul 2021 at 23:23, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Tue, Jul 13, 2021 at 08:49:21PM -0600, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Tue, 13 Jul 2021 at 14:11, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > >
> > > [...]
> > > > > > Should be a uclass interface.
> > > > > >
> > > > >
> > > > > Why? A uclass is supposed to describe and abstract hardware.  This is just
> > > > > a specific implementation of a TPM, not all TPMs are TIS compliant. We already
> > > > > have a uclass for those.
> > > >
> > > > Who told you that a uclass is supposed to describe and abstract hardware? :-)
> > > >
> > >
> > > That's what I've mostly seen it used for, maybe i got the idea wrong.
> >
> > A uclass is basically a software construct. It is an interface between
> > clients and the driver, typically. Quite often the uclass is an
> > interface on top of the hardware (actually the driver). But quite
> > often it is not. For example, we use an GPIO uclass to access a pmic's
> > GPIOs, we use an I2C uclass to access the cros_ec I2C tunnel. Anywhere
> > where it makes sense to have an abstraction, we use a uclass.
> >
> > >
> > > > The uclass is how driver model does APIs, so normally a uclass would
> > > > be used for any API. There are exceptions, but this one actually looks
> > > > like a useful interface we should have.
> > > >
> > >
> > > the point is we already have a uclass for tpm devices.  So why should the
> > > we add another one that just describes the TIS interface?
> >
> > You have already added another API, right? All we are discussing is
> > whether it should be a uclass or not. Unless there is a very good
> > reason, we should avoid creating custom interfaces that don't use
> > driver model. I actually think the interface you've created (MMIO)
> > will be very useful as a uclass.
> >
>
> So you are basically looking into adding something similar to
> dm_i2c_read/dm_i2c_write etc?
> I assume this is gong to be the default read method passed on the TIS API
> when we want to support i2c TPMs.

Well I'm not quite sure, but if MMIO can sit on top of I2C, then yes.

> For the MMIO case that would essentially mean, move the functions on a
> different file, add them on a header and define a UCLASS_DRIVER with only
> the .id and .name defined?

Yes. Something like:

- add a new uclass ID to dm/uciass-id.h
- add include/mmio.h (or whatever it is called) with operations (see
pch.h as an example)
- add drivers/misc/mmio-uclass.c with the UCLASS_DRIVER and stubs for the API
- add drivers/misc/sandbox_mmio.c with a sandbox driver that does very
little (perhaps just read canned data?)
- add test/dm/mmio.c sandbox test that calls each API function and
checks the result

As I said pch is a good example since it is fairly simple.

Regards,
Simon