From: "[email protected]" <[email protected]>
Add TPM 2.0 compatible I2C interface for chips with cr50 firmware.
The firmware running on the currently supported H1 MCU requires a
special driver to handle its specific protocol, and this makes it
unsuitable to use tpm_tis_core_* and instead it must implement the
underlying TPM protocol similar to the other I2C TPM drivers.
- All 4 byes of status register must be read/written at once.
- FIFO and burst count is limited to 63 and must be drained by AP.
- Provides an interrupt to indicate when read response data is ready
and when the TPM is finished processing write data.
This driver is based on the existing infineon I2C TPM driver, which
most closely matches the cr50 i2c protocol behavior.
Cc: Helen Koike <[email protected]>
Signed-off-by: Duncan Laurie <[email protected]>
[[email protected]: Depend on i2c even if it's a module, replace
boilier plate with SPDX tag, drop asm/byteorder.h include, simplify
return from probe]
Signed-off-by: Stephen Boyd <[email protected]>
Signed-off-by: Fabien Lahoudere <[email protected]>
Signed-off-by: Adrian Ratiu <[email protected]>
---
Changes in v2:
- Various small fixes all over (reorder includes, MAX_BUFSIZE, comments, etc)
- Reworked return values of i2c_wait_tpm_ready() to fix timeout mis-handling
so ret == 0 now means success, the wait period jiffies is ignored because that
number is meaningless and return a proper timeout error in case jiffies == 0.
- Make i2c default to 1 message per transfer (requested by Helen)
- Move -EIO error reporting to transfer function to cleanup transfer() itself
and its R/W callers
- Remove magic value hardcodings and introduce enum force_release.
v1 posted at https://lkml.org/lkml/2020/2/25/349
Applies on next-20201120, tested on Chromebook EVE.
---
drivers/char/tpm/Kconfig | 10 +
drivers/char/tpm/Makefile | 2 +
drivers/char/tpm/tpm_tis_i2c_cr50.c | 768 ++++++++++++++++++++++++++++
3 files changed, 780 insertions(+)
create mode 100644 drivers/char/tpm/tpm_tis_i2c_cr50.c
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index a18c314da211..4308f9ca7a43 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
To compile this driver as a module, choose M here;
the module will be called tpm_tis_synquacer.
+config TCG_TIS_I2C_CR50
+ tristate "TPM Interface Specification 2.0 Interface (I2C - CR50)"
+ depends on I2C
+ select TCG_CR50
+ help
+ This is a driver for the Google cr50 I2C TPM interface which is a
+ custom microcontroller and requires a custom i2c protocol interface
+ to handle the limitations of the hardware. To compile this driver
+ as a module, choose M here; the module will be called tcg_tis_i2c_cr50.
+
config TCG_TIS_I2C_ATMEL
tristate "TPM Interface Specification 1.2 Interface (I2C - Atmel)"
depends on I2C
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 84db4fb3a9c9..66d39ea6bd10 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
tpm_tis_spi-y := tpm_tis_spi_main.o
tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
+obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
+
obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
new file mode 100644
index 000000000000..37555dafdca0
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -0,0 +1,768 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2016 Google Inc.
+ *
+ * Based on Linux Kernel TPM driver by
+ * Peter Huewe <[email protected]>
+ * Copyright (C) 2011 Infineon Technologies
+ */
+
+/*
+ * cr50 is a firmware for H1 secure modules that requires special
+ * handling for the I2C interface.
+ *
+ * - Use an interrupt for transaction status instead of hardcoded delays
+ * - Must use write+wait+read read protocol
+ * - All 4 bytes of status register must be read/written at once
+ * - Burst count max is 63 bytes, and burst count behaves
+ * slightly differently than other I2C TPMs
+ * - When reading from FIFO the full burstcnt must be read
+ * instead of just reading header and determining the remainder
+ */
+
+#include <linux/acpi.h>
+#include <linux/completion.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/wait.h>
+
+#include "tpm_tis_core.h"
+
+#define CR50_MAX_BUFSIZE 64
+#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during transactions */
+#define CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready without IRQ */
+#define CR50_I2C_DID_VID 0x00281ae0L
+#define CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */
+#define CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries on I2C */
+#define CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries on I2C */
+
+#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
+#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
+#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
+#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4))
+
+struct priv_data {
+ int irq;
+ int locality;
+ struct completion tpm_ready;
+ u8 buf[CR50_MAX_BUFSIZE];
+};
+
+enum force_release {
+ CR50_NO_FORCE = 0x0,
+ CR50_FORCE = 0x1,
+};
+
+/*
+ * The cr50 interrupt handler just signals waiting threads that the
+ * interrupt was asserted. It does not do any processing triggered
+ * by interrupts but is instead used to avoid fixed delays.
+ *
+ * @dummy: unuesed parameter
+ * @dev_id: TPM chip information
+ */
+static irqreturn_t cr50_i2c_int_handler(int dummy, void *dev_id)
+{
+ struct tpm_chip *chip = dev_id;
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+
+ complete(&priv->tpm_ready);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * Wait for completion interrupt if available, otherwise use a fixed
+ * delay for the TPM to be ready.
+ *
+ * @chip: TPM chip information
+ *
+ * Returns 0 for success, negative number for timeout
+ */
+static int cr50_i2c_wait_tpm_ready(struct tpm_chip *chip)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+
+ /* Use a safe fixed delay if interrupt is not supported */
+ if (priv->irq <= 0) {
+ msleep(CR50_TIMEOUT_NOIRQ_MS);
+ return 0;
+ }
+
+ /* Wait for interrupt to indicate TPM is ready to respond */
+ if (!wait_for_completion_timeout(&priv->tpm_ready,
+ msecs_to_jiffies(chip->timeout_a))) {
+ dev_warn(&chip->dev, "Timeout waiting for TPM ready\n");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+/*
+ * cr50_i2c_enable_tpm_irq - enable TPM irq
+ *
+ * @chip: TPM chip information
+ */
+static void cr50_i2c_enable_tpm_irq(struct tpm_chip *chip)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+
+ if (priv->irq > 0) {
+ reinit_completion(&priv->tpm_ready);
+ enable_irq(priv->irq);
+ }
+}
+
+/*
+ * cr50_i2c_disable_tpm_irq - disable TPM irq
+ *
+ * @chip: TPM chip information
+ */
+static void cr50_i2c_disable_tpm_irq(struct tpm_chip *chip)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+
+ if (priv->irq > 0)
+ disable_irq(priv->irq);
+}
+
+/*
+ * cr50_i2c_transfer_message - transfer a message over i2c
+ *
+ * @dev: device information
+ * @adapter: i2c adapter
+ * @msg: message to transfer
+ *
+ * Call unlocked i2c transfer routine with the provided parameters and retry
+ * in case of bus errors. Returns 0 on success, otherwise negative errno.
+ */
+static int cr50_i2c_transfer_message(struct device *dev,
+ struct i2c_adapter *adapter,
+ struct i2c_msg *msg)
+{
+ int rc = 0;
+ unsigned int try;
+
+ for (try = 0; try < CR50_I2C_MAX_RETRIES; try++) {
+ rc = __i2c_transfer(adapter, msg, 1);
+ if (rc == 1)
+ return 0; /* Successfully transferred the message */
+ if (try)
+ dev_warn(dev, "i2c transfer failed (attempt %d/%d): %d\n",
+ try + 1, CR50_I2C_MAX_RETRIES, rc);
+ usleep_range(CR50_I2C_RETRY_DELAY_LO, CR50_I2C_RETRY_DELAY_HI);
+ }
+
+ return -EIO; /* No i2c message transferred */
+}
+
+/*
+ * cr50_i2c_read() - read from TPM register
+ *
+ * @chip: TPM chip information
+ * @addr: register address to read from
+ * @buffer: provided by caller
+ * @len: number of bytes to read
+ *
+ * 1) send register address byte 'addr' to the TPM
+ * 2) wait for TPM to indicate it is ready
+ * 3) read 'len' bytes of TPM response into the provided 'buffer'
+ *
+ * Returns negative number for error, 0 for success.
+ */
+static int cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t len)
+{
+ struct i2c_client *client = to_i2c_client(chip->dev.parent);
+ struct i2c_msg msg1 = {
+ .addr = client->addr,
+ .len = 1,
+ .buf = &addr
+ };
+ struct i2c_msg msg2 = {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .len = len,
+ .buf = buffer
+ };
+ int rc;
+
+ i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+ /* Prepare for completion interrupt */
+ cr50_i2c_enable_tpm_irq(chip);
+
+ /* Send the register address byte to the TPM */
+ rc = cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
+ if (rc < 0)
+ goto out;
+
+ /* Wait for TPM to be ready with response data */
+ rc = cr50_i2c_wait_tpm_ready(chip);
+ if (rc < 0)
+ goto out;
+
+ /* Read response data from the TPM */
+ rc = cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg2);
+
+out:
+ cr50_i2c_disable_tpm_irq(chip);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+ if (rc < 0)
+ return rc;
+
+ return 0;
+}
+
+/*
+ * cr50_i2c_write() - write to TPM register
+ *
+ * @chip: TPM chip information
+ * @addr: register address to write to
+ * @buffer: data to write
+ * @len: number of bytes to write
+ *
+ * 1) prepend the provided address to the provided data
+ * 2) send the address+data to the TPM
+ * 3) wait for TPM to indicate it is done writing
+ *
+ * Returns negative number for error, 0 for success.
+ */
+static int cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
+ size_t len)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ struct i2c_client *client = to_i2c_client(chip->dev.parent);
+ struct i2c_msg msg1 = {
+ .addr = client->addr,
+ .len = len + 1,
+ .buf = priv->buf
+ };
+ int rc;
+
+ if (len > CR50_MAX_BUFSIZE - 1)
+ return -EINVAL;
+
+ i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+ /* Prepend the 'register address' to the buffer */
+ priv->buf[0] = addr;
+ memcpy(priv->buf + 1, buffer, len);
+
+ /* Prepare for completion interrupt */
+ cr50_i2c_enable_tpm_irq(chip);
+
+ /* Send write request buffer with address */
+ rc = cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
+ if (rc < 0)
+ goto out;
+
+ /* Wait for TPM to be ready, ignore timeout */
+ cr50_i2c_wait_tpm_ready(chip);
+
+out:
+ cr50_i2c_disable_tpm_irq(chip);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+ if (rc < 0)
+ return rc;
+
+ return 0;
+}
+
+/*
+ * cr50_check_locality - verify TPM locality
+ *
+ * @chip: TPM chip information
+ *
+ * Returns negative number for error, 0 for success.
+ */
+static int cr50_check_locality(struct tpm_chip *chip)
+{
+ u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
+ u8 buf;
+ int rc;
+
+ rc = cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
+ if (rc < 0)
+ return rc;
+
+ if ((buf & mask) == mask)
+ return 0;
+
+ return -EIO;
+}
+
+/*
+ * cr50_release_locality - release TPM locality
+ *
+ * @chip: TPM chip information
+ * @force: flag to force release if set
+ */
+static void cr50_release_locality(struct tpm_chip *chip,
+ enum force_release force)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
+ u8 addr = TPM_I2C_ACCESS(priv->locality);
+ u8 buf;
+
+ if (cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0)
+ return;
+
+ if (force || (buf & mask) == mask) {
+ buf = TPM_ACCESS_ACTIVE_LOCALITY;
+ cr50_i2c_write(chip, addr, &buf, sizeof(buf));
+ }
+
+ priv->locality = 0;
+}
+
+/*
+ * request_locality - request TPM locality
+ *
+ * @chip: TPM chip information
+ */
+static int request_locality(struct tpm_chip *chip)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ u8 buf = TPM_ACCESS_REQUEST_USE;
+ unsigned long stop;
+ int rc;
+
+ if (!cr50_check_locality(chip))
+ return 0;
+
+ rc = cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
+ if (rc < 0)
+ return rc;
+
+ stop = jiffies + chip->timeout_a;
+ do {
+ if (!cr50_check_locality(chip)) {
+ priv->locality = 0;
+ return 0;
+ }
+ msleep(CR50_TIMEOUT_SHORT_MS);
+ } while (time_before(jiffies, stop));
+
+ return -ETIMEDOUT;
+}
+
+/*
+ * cr50 requires all 4 bytes of status register to be read
+ *
+ * @chip: TPM chip information
+ *
+ * Returns TPM status
+ */
+static u8 cr50_i2c_tis_status(struct tpm_chip *chip)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ u8 buf[4];
+
+ if (cr50_i2c_read(chip, TPM_I2C_STS(priv->locality),
+ buf, sizeof(buf)) < 0)
+ return 0;
+ return buf[0];
+}
+
+/*
+ * cr50 requires all 4 bytes of status register to be written
+ *
+ * @chip: TPM chip information
+ */
+static void cr50_i2c_tis_ready(struct tpm_chip *chip)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ u8 buf[4] = { TPM_STS_COMMAND_READY };
+
+ cr50_i2c_write(chip, TPM_I2C_STS(priv->locality), buf, sizeof(buf));
+ msleep(CR50_TIMEOUT_SHORT_MS);
+}
+
+/*
+ * cr50 uses bytes 3:2 of status register for burst count and
+ * all 4 bytes must be read
+ *
+ * @chip: TPM chip information
+ * @mask: status mask
+ * @burst: return value for burst
+ * @status: return value for statis
+ *
+ * Returns negative number for error, 0 for success.
+ */
+static int cr50_i2c_wait_burststs(struct tpm_chip *chip, u8 mask,
+ size_t *burst, int *status)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ unsigned long stop;
+ u8 buf[4];
+
+ /* wait for burstcount */
+ stop = jiffies + chip->timeout_b;
+ do {
+ if (cr50_i2c_read(chip, TPM_I2C_STS(priv->locality),
+ buf, sizeof(buf)) < 0) {
+ msleep(CR50_TIMEOUT_SHORT_MS);
+ continue;
+ }
+
+ *status = *buf;
+ *burst = le16_to_cpup((__le16 *)(buf + 1));
+
+ if ((*status & mask) == mask &&
+ *burst > 0 && *burst <= CR50_MAX_BUFSIZE - 1)
+ return 0;
+
+ msleep(CR50_TIMEOUT_SHORT_MS);
+ } while (time_before(jiffies, stop));
+
+ dev_err(&chip->dev, "Timeout reading burst and status\n");
+ return -ETIMEDOUT;
+}
+
+/*
+ * cr50_i2c_tis_recv - TPM reception callback
+ *
+ * @chip: TPM chip information
+ * @buf: reception buffer
+ * @buf_len: buffer length to read
+ *
+ * Returns negative number for error, number of bytes read for success.
+ */
+static int cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ int status, rc;
+ size_t burstcnt, cur, len, expected;
+ u8 addr = TPM_I2C_DATA_FIFO(priv->locality);
+ u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
+
+ if (buf_len < TPM_HEADER_SIZE)
+ return -EINVAL;
+
+ rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+
+ if (burstcnt > buf_len || burstcnt < TPM_HEADER_SIZE) {
+ dev_err(&chip->dev,
+ "Unexpected burstcnt: %zu (max=%zu, min=%d)\n",
+ burstcnt, buf_len, TPM_HEADER_SIZE);
+ rc = -EIO;
+ goto out_err;
+ }
+
+ /* Read first chunk of burstcnt bytes */
+ rc = cr50_i2c_read(chip, addr, buf, burstcnt);
+ if (rc < 0) {
+ dev_err(&chip->dev, "Read of first chunk failed\n");
+ goto out_err;
+ }
+
+ /* Determine expected data in the return buffer */
+ expected = be32_to_cpup((__be32 *)(buf + 2));
+ if (expected > buf_len) {
+ dev_err(&chip->dev, "Buffer too small to receive i2c data\n");
+ goto out_err;
+ }
+
+ /* Now read the rest of the data */
+ cur = burstcnt;
+ while (cur < expected) {
+ /* Read updated burst count and check status */
+ rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+
+ len = min_t(size_t, burstcnt, expected - cur);
+ rc = cr50_i2c_read(chip, addr, buf + cur, len);
+ if (rc < 0) {
+ dev_err(&chip->dev, "Read failed\n");
+ goto out_err;
+ }
+
+ cur += len;
+ }
+
+ /* Ensure TPM is done reading data */
+ rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+ if (status & TPM_STS_DATA_AVAIL) {
+ dev_err(&chip->dev, "Data still available\n");
+ rc = -EIO;
+ goto out_err;
+ }
+
+ cr50_release_locality(chip, CR50_NO_FORCE);
+ return cur;
+
+out_err:
+ /* Abort current transaction if still pending */
+ if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
+ cr50_i2c_tis_ready(chip);
+
+ cr50_release_locality(chip, CR50_NO_FORCE);
+ return rc;
+}
+
+/*
+ * cr50_i2c_tis_send - TPM emission callback
+ *
+ * @chip: TPM chip information
+ * @buf: buffer to send
+ * @len: buffer length
+ *
+ * Returns negative number for error, 0 for success.
+ */
+static int cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ int rc, status;
+ size_t burstcnt, limit, sent = 0;
+ u8 tpm_go[4] = { TPM_STS_GO };
+ unsigned long stop;
+
+ rc = request_locality(chip);
+ if (rc < 0)
+ return rc;
+
+ /* Wait until TPM is ready for a command */
+ stop = jiffies + chip->timeout_b;
+ while (!(cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)) {
+ if (time_after(jiffies, stop)) {
+ rc = -ETIMEDOUT;
+ goto out_err;
+ }
+
+ cr50_i2c_tis_ready(chip);
+ }
+
+ while (len > 0) {
+ u8 mask = TPM_STS_VALID;
+
+ /* Wait for data if this is not the first chunk */
+ if (sent > 0)
+ mask |= TPM_STS_DATA_EXPECT;
+
+ /* Read burst count and check status */
+ rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+
+ /*
+ * Use burstcnt - 1 to account for the address byte
+ * that is inserted by cr50_i2c_write()
+ */
+ limit = min_t(size_t, burstcnt - 1, len);
+ rc = cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(priv->locality),
+ &buf[sent], limit);
+ if (rc < 0) {
+ dev_err(&chip->dev, "Write failed\n");
+ goto out_err;
+ }
+
+ sent += limit;
+ len -= limit;
+ }
+
+ /* Ensure TPM is not expecting more data */
+ rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+ if (status & TPM_STS_DATA_EXPECT) {
+ dev_err(&chip->dev, "Data still expected\n");
+ rc = -EIO;
+ goto out_err;
+ }
+
+ /* Start the TPM command */
+ rc = cr50_i2c_write(chip, TPM_I2C_STS(priv->locality), tpm_go,
+ sizeof(tpm_go));
+ if (rc < 0) {
+ dev_err(&chip->dev, "Start command failed\n");
+ goto out_err;
+ }
+ return 0;
+
+out_err:
+ /* Abort current transaction if still pending */
+ if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
+ cr50_i2c_tis_ready(chip);
+
+ cr50_release_locality(chip, CR50_NO_FORCE);
+ return rc;
+}
+
+/*
+ * cr50_i2c_req_canceled - callback to notify a request cancel
+ *
+ * @chip: TPM chip information
+ * @status: status given by the cancel callback
+ *
+ * Return if command is ready or not
+ */
+static bool cr50_i2c_req_canceled(struct tpm_chip *chip, u8 status)
+{
+ return (status == TPM_STS_COMMAND_READY);
+}
+
+static const struct tpm_class_ops cr50_i2c = {
+ .flags = TPM_OPS_AUTO_STARTUP,
+ .status = &cr50_i2c_tis_status,
+ .recv = &cr50_i2c_tis_recv,
+ .send = &cr50_i2c_tis_send,
+ .cancel = &cr50_i2c_tis_ready,
+ .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ .req_canceled = &cr50_i2c_req_canceled,
+};
+
+static const struct i2c_device_id cr50_i2c_table[] = {
+ {"cr50_i2c", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, cr50_i2c_table);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id cr50_i2c_acpi_id[] = {
+ { "GOOG0005", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, cr50_i2c_acpi_id);
+#endif
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_cr50_i2c_match[] = {
+ { .compatible = "google,cr50", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_cr50_i2c_match);
+#endif
+
+/*
+ * cr50_i2c_probe - driver prbe function
+ *
+ * @client: i2x client information
+ * @id: i2c device id
+ *
+ * Returns negative number for error, 0 for success.
+ */
+static int cr50_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct tpm_chip *chip;
+ struct priv_data *priv;
+ u8 buf[4];
+ u32 vendor;
+ int rc;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ return -ENODEV;
+
+ chip = tpmm_chip_alloc(dev, &cr50_i2c);
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ /* cr50 is a TPM 2.0 chip */
+ chip->flags |= TPM_CHIP_FLAG_TPM2;
+ chip->flags |= TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED;
+
+ /* Default timeouts */
+ chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
+ chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+
+ dev_set_drvdata(&chip->dev, priv);
+ init_completion(&priv->tpm_ready);
+
+ if (client->irq > 0) {
+ rc = devm_request_irq(dev, client->irq, cr50_i2c_int_handler,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ dev->driver->name, chip);
+ if (rc < 0) {
+ dev_err(dev, "Failed to probe IRQ %d\n", client->irq);
+ return rc;
+ }
+
+ disable_irq(client->irq);
+ priv->irq = client->irq;
+ } else {
+ dev_warn(dev, "No IRQ, will use %ums delay for TPM ready\n",
+ CR50_TIMEOUT_NOIRQ_MS);
+ }
+
+ rc = request_locality(chip);
+ if (rc < 0) {
+ dev_err(dev, "Could not request locality\n");
+ return rc;
+ }
+
+ /* Read four bytes from DID_VID register */
+ rc = cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf));
+ if (rc < 0) {
+ dev_err(dev, "Could not read vendor id\n");
+ cr50_release_locality(chip, CR50_FORCE);
+ return rc;
+ }
+
+ vendor = le32_to_cpup((__le32 *)buf);
+ if (vendor != CR50_I2C_DID_VID) {
+ dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
+ cr50_release_locality(chip, CR50_FORCE);
+ return -ENODEV;
+ }
+
+ dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n",
+ client->addr, client->irq, vendor >> 16);
+
+ return tpm_chip_register(chip);
+}
+
+/*
+ * cr50_i2c_probe - driver prbe function
+ *
+ * @client: i2x client information
+ *
+ * Returns 0
+ */
+static int cr50_i2c_remove(struct i2c_client *client)
+{
+ struct tpm_chip *chip = i2c_get_clientdata(client);
+
+ tpm_chip_unregister(chip);
+ cr50_release_locality(chip, CR50_FORCE);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume);
+
+static struct i2c_driver cr50_i2c_driver = {
+ .id_table = cr50_i2c_table,
+ .probe = cr50_i2c_probe,
+ .remove = cr50_i2c_remove,
+ .driver = {
+ .name = "cr50_i2c",
+ .pm = &cr50_i2c_pm,
+ .acpi_match_table = ACPI_PTR(cr50_i2c_acpi_id),
+ .of_match_table = of_match_ptr(of_cr50_i2c_match),
+ },
+};
+
+module_i2c_driver(cr50_i2c_driver);
+
+MODULE_DESCRIPTION("cr50 TPM I2C Driver");
+MODULE_LICENSE("GPL");
--
2.29.2
Hello Adrian,
I just spotted small things (nothing major), please see below.
On 11/20/20 2:23 PM, Adrian Ratiu wrote:
> From: "[email protected]" <[email protected]>
>
> Add TPM 2.0 compatible I2C interface for chips with cr50 firmware.
>
> The firmware running on the currently supported H1 MCU requires a
> special driver to handle its specific protocol, and this makes it
> unsuitable to use tpm_tis_core_* and instead it must implement the
> underlying TPM protocol similar to the other I2C TPM drivers.
>
> - All 4 byes of status register must be read/written at once.
s/byes/bytes
> - FIFO and burst count is limited to 63 and must be drained by AP.
> - Provides an interrupt to indicate when read response data is ready
> and when the TPM is finished processing write data.
>
> This driver is based on the existing infineon I2C TPM driver, which
> most closely matches the cr50 i2c protocol behavior.
>
> Cc: Helen Koike <[email protected]>
> Signed-off-by: Duncan Laurie <[email protected]>
> [[email protected]: Depend on i2c even if it's a module, replace
> boilier plate with SPDX tag, drop asm/byteorder.h include, simplify
> return from probe]
> Signed-off-by: Stephen Boyd <[email protected]>
> Signed-off-by: Fabien Lahoudere <[email protected]>
> Signed-off-by: Adrian Ratiu <[email protected]>
> ---
> Changes in v2:
> - Various small fixes all over (reorder includes, MAX_BUFSIZE, comments, etc)
> - Reworked return values of i2c_wait_tpm_ready() to fix timeout mis-handling
> so ret == 0 now means success, the wait period jiffies is ignored because that
> number is meaningless and return a proper timeout error in case jiffies == 0.
> - Make i2c default to 1 message per transfer (requested by Helen)
> - Move -EIO error reporting to transfer function to cleanup transfer() itself
> and its R/W callers
> - Remove magic value hardcodings and introduce enum force_release.
>
> v1 posted at https://lkml.org/lkml/2020/2/25/349
>
> Applies on next-20201120, tested on Chromebook EVE.
> ---
> drivers/char/tpm/Kconfig | 10 +
> drivers/char/tpm/Makefile | 2 +
> drivers/char/tpm/tpm_tis_i2c_cr50.c | 768 ++++++++++++++++++++++++++++
> 3 files changed, 780 insertions(+)
> create mode 100644 drivers/char/tpm/tpm_tis_i2c_cr50.c
>
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index a18c314da211..4308f9ca7a43 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
> To compile this driver as a module, choose M here;
> the module will be called tpm_tis_synquacer.
>
> +config TCG_TIS_I2C_CR50
> + tristate "TPM Interface Specification 2.0 Interface (I2C - CR50)"
> + depends on I2C
> + select TCG_CR50
> + help
> + This is a driver for the Google cr50 I2C TPM interface which is a
> + custom microcontroller and requires a custom i2c protocol interface
> + to handle the limitations of the hardware. To compile this driver
> + as a module, choose M here; the module will be called tcg_tis_i2c_cr50.
> +
> config TCG_TIS_I2C_ATMEL
> tristate "TPM Interface Specification 1.2 Interface (I2C - Atmel)"
> depends on I2C
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 84db4fb3a9c9..66d39ea6bd10 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> tpm_tis_spi-y := tpm_tis_spi_main.o
> tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
>
> +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
> +
> obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
> obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
> obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> new file mode 100644
> index 000000000000..37555dafdca0
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> @@ -0,0 +1,768 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2016 Google Inc.
> + *
> + * Based on Linux Kernel TPM driver by
> + * Peter Huewe <[email protected]>
> + * Copyright (C) 2011 Infineon Technologies
> + */
> +
> +/*
> + * cr50 is a firmware for H1 secure modules that requires special
> + * handling for the I2C interface.
> + *
> + * - Use an interrupt for transaction status instead of hardcoded delays
> + * - Must use write+wait+read read protocol
> + * - All 4 bytes of status register must be read/written at once
> + * - Burst count max is 63 bytes, and burst count behaves
> + * slightly differently than other I2C TPMs
> + * - When reading from FIFO the full burstcnt must be read
> + * instead of just reading header and determining the remainder
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/completion.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>
> +
> +#include "tpm_tis_core.h"
> +
> +#define CR50_MAX_BUFSIZE 64
> +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during transactions */
> +#define CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready without IRQ */
> +#define CR50_I2C_DID_VID 0x00281ae0L
> +#define CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */
> +#define CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries on I2C */
> +#define CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries on I2C */
> +
> +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
> +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
> +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
> +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4))
> +
> +struct priv_data {
> + int irq;
> + int locality;
> + struct completion tpm_ready;
> + u8 buf[CR50_MAX_BUFSIZE];
> +};
> +
> +enum force_release {
> + CR50_NO_FORCE = 0x0,
> + CR50_FORCE = 0x1,
> +};
No need for hexadecimal
> +
> +/*
> + * The cr50 interrupt handler just signals waiting threads that the
> + * interrupt was asserted. It does not do any processing triggered
> + * by interrupts but is instead used to avoid fixed delays.
> + *
> + * @dummy: unuesed parameter
s/unuesed/unused
> + * @dev_id: TPM chip information
> + */
> +static irqreturn_t cr50_i2c_int_handler(int dummy, void *dev_id)
> +{
> + struct tpm_chip *chip = dev_id;
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> +
> + complete(&priv->tpm_ready);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/*
> + * Wait for completion interrupt if available, otherwise use a fixed
> + * delay for the TPM to be ready.
> + *
> + * @chip: TPM chip information
> + *
> + * Returns 0 for success, negative number for timeout
> + */
> +static int cr50_i2c_wait_tpm_ready(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> +
> + /* Use a safe fixed delay if interrupt is not supported */
> + if (priv->irq <= 0) {
> + msleep(CR50_TIMEOUT_NOIRQ_MS);
> + return 0;
> + }
> +
> + /* Wait for interrupt to indicate TPM is ready to respond */
> + if (!wait_for_completion_timeout(&priv->tpm_ready,
> + msecs_to_jiffies(chip->timeout_a))) {
> + dev_warn(&chip->dev, "Timeout waiting for TPM ready\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * cr50_i2c_enable_tpm_irq - enable TPM irq
> + *
> + * @chip: TPM chip information
> + */
> +static void cr50_i2c_enable_tpm_irq(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> +
> + if (priv->irq > 0) {
> + reinit_completion(&priv->tpm_ready);
> + enable_irq(priv->irq);
> + }
> +}
> +
> +/*
> + * cr50_i2c_disable_tpm_irq - disable TPM irq
> + *
> + * @chip: TPM chip information
> + */
> +static void cr50_i2c_disable_tpm_irq(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> +
> + if (priv->irq > 0)
> + disable_irq(priv->irq);
> +}
> +
> +/*
> + * cr50_i2c_transfer_message - transfer a message over i2c
> + *
> + * @dev: device information
> + * @adapter: i2c adapter
> + * @msg: message to transfer
> + *
> + * Call unlocked i2c transfer routine with the provided parameters and retry
> + * in case of bus errors. Returns 0 on success, otherwise negative errno.
> + */
> +static int cr50_i2c_transfer_message(struct device *dev,
> + struct i2c_adapter *adapter,
> + struct i2c_msg *msg)
> +{
> + int rc = 0;
No need to init to zero.
> + unsigned int try;
> +
> + for (try = 0; try < CR50_I2C_MAX_RETRIES; try++) {
> + rc = __i2c_transfer(adapter, msg, 1);
> + if (rc == 1)
> + return 0; /* Successfully transferred the message */
> + if (try)
> + dev_warn(dev, "i2c transfer failed (attempt %d/%d): %d\n",
> + try + 1, CR50_I2C_MAX_RETRIES, rc);
> + usleep_range(CR50_I2C_RETRY_DELAY_LO, CR50_I2C_RETRY_DELAY_HI);
> + }
> +
> + return -EIO; /* No i2c message transferred */
> +}
> +
> +/*
> + * cr50_i2c_read() - read from TPM register
> + *
> + * @chip: TPM chip information
> + * @addr: register address to read from
> + * @buffer: provided by caller
> + * @len: number of bytes to read
> + *
> + * 1) send register address byte 'addr' to the TPM
> + * 2) wait for TPM to indicate it is ready
> + * 3) read 'len' bytes of TPM response into the provided 'buffer'
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t len)
> +{
> + struct i2c_client *client = to_i2c_client(chip->dev.parent);
> + struct i2c_msg msg1 = {
Maybe rename to msg_reg_addr (I think it is more meaningful then numbers).
> + .addr = client->addr,
> + .len = 1,
> + .buf = &addr
> + };
> + struct i2c_msg msg2 = {
Maybe rename to msg_response (I think it is more meaningful then numbers).
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = len,
> + .buf = buffer
> + };
> + int rc;
> +
> + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + /* Prepare for completion interrupt */
> + cr50_i2c_enable_tpm_irq(chip);
> +
> + /* Send the register address byte to the TPM */
> + rc = cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
> + if (rc < 0)
> + goto out;
> +
> + /* Wait for TPM to be ready with response data */
> + rc = cr50_i2c_wait_tpm_ready(chip);
> + if (rc < 0)
> + goto out;
> +
> + /* Read response data from the TPM */
> + rc = cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg2);
> +
> +out:
> + cr50_i2c_disable_tpm_irq(chip);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + if (rc < 0)
> + return rc;
> +
> + return 0;
> +}
> +
> +/*
> + * cr50_i2c_write() - write to TPM register
> + *
> + * @chip: TPM chip information
> + * @addr: register address to write to
> + * @buffer: data to write
> + * @len: number of bytes to write
> + *
> + * 1) prepend the provided address to the provided data
> + * 2) send the address+data to the TPM
> + * 3) wait for TPM to indicate it is done writing
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
> + size_t len)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + struct i2c_client *client = to_i2c_client(chip->dev.parent);
> + struct i2c_msg msg1 = {
Maybe rename to just msg.
> + .addr = client->addr,
> + .len = len + 1,
> + .buf = priv->buf
> + };
> + int rc;
> +
> + if (len > CR50_MAX_BUFSIZE - 1)
> + return -EINVAL;
> +
> + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + /* Prepend the 'register address' to the buffer */
> + priv->buf[0] = addr;
> + memcpy(priv->buf + 1, buffer, len);
Shouldn't this copy be done before the lock?
> +
> + /* Prepare for completion interrupt */
> + cr50_i2c_enable_tpm_irq(chip);
> +
> + /* Send write request buffer with address */
> + rc = cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
> + if (rc < 0)
> + goto out;
> +
> + /* Wait for TPM to be ready, ignore timeout */
> + cr50_i2c_wait_tpm_ready(chip);
> +
> +out:
> + cr50_i2c_disable_tpm_irq(chip);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + if (rc < 0)
> + return rc;
> +
> + return 0;
> +}
> +
> +/*
> + * cr50_check_locality - verify TPM locality
> + *
> + * @chip: TPM chip information
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_check_locality(struct tpm_chip *chip)
> +{
> + u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
> + u8 buf;
> + int rc;
> +
> + rc = cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> + if (rc < 0)
> + return rc;
> +
> + if ((buf & mask) == mask)
> + return 0;
> +
> + return -EIO;
> +}
> +
> +/*
> + * cr50_release_locality - release TPM locality
> + *
> + * @chip: TPM chip information
> + * @force: flag to force release if set
> + */
> +static void cr50_release_locality(struct tpm_chip *chip,
> + enum force_release force)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
> + u8 addr = TPM_I2C_ACCESS(priv->locality);
> + u8 buf;
> +
> + if (cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0)
> + return;
> +
> + if (force || (buf & mask) == mask) {
> + buf = TPM_ACCESS_ACTIVE_LOCALITY;
> + cr50_i2c_write(chip, addr, &buf, sizeof(buf));
> + }
> +
> + priv->locality = 0;
I'm probally missing something, I don't see priv->locality being
set to another value other then 0 in the code.
Shoul it be set in request_locality() or in cr50_release_locality() ?
> +}
> +
> +/*
> + * request_locality - request TPM locality
> + *
> + * @chip: TPM chip information
> + */
> +static int request_locality(struct tpm_chip *chip)
All the other function have c50_ prefix.
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 buf = TPM_ACCESS_REQUEST_USE;
> + unsigned long stop;
> + int rc;
> +
> + if (!cr50_check_locality(chip))
> + return 0;
> +
> + rc = cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> + if (rc < 0)
> + return rc;
> +
> + stop = jiffies + chip->timeout_a;
> + do {
> + if (!cr50_check_locality(chip)) {
> + priv->locality = 0;
> + return 0;
> + }
> + msleep(CR50_TIMEOUT_SHORT_MS);
> + } while (time_before(jiffies, stop));
> +
> + return -ETIMEDOUT;
> +}
> +
> +/*
> + * cr50 requires all 4 bytes of status register to be read
> + *
> + * @chip: TPM chip information
> + *
> + * Returns TPM status
> + */
> +static u8 cr50_i2c_tis_status(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 buf[4];
> +
> + if (cr50_i2c_read(chip, TPM_I2C_STS(priv->locality),
> + buf, sizeof(buf)) < 0)
> + return 0;
> + return buf[0];
> +}
> +
> +/*
> + * cr50 requires all 4 bytes of status register to be written
> + *
> + * @chip: TPM chip information
> + */
> +static void cr50_i2c_tis_ready(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 buf[4] = { TPM_STS_COMMAND_READY };
> +
> + cr50_i2c_write(chip, TPM_I2C_STS(priv->locality), buf, sizeof(buf));
> + msleep(CR50_TIMEOUT_SHORT_MS);
> +}
> +
> +/*
> + * cr50 uses bytes 3:2 of status register for burst count and
> + * all 4 bytes must be read
> + *
> + * @chip: TPM chip information
> + * @mask: status mask
> + * @burst: return value for burst
> + * @status: return value for statis
s/statis/status
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_wait_burststs(struct tpm_chip *chip, u8 mask,
Maybe rename to cr50_i2c_get_burst_and_status().
> + size_t *burst, int *status)
Should status be unsigned?
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + unsigned long stop;
> + u8 buf[4];
> +
> + /* wait for burstcount */
> + stop = jiffies + chip->timeout_b;
> + do {
> + if (cr50_i2c_read(chip, TPM_I2C_STS(priv->locality),
> + buf, sizeof(buf)) < 0) {
> + msleep(CR50_TIMEOUT_SHORT_MS);
> + continue;
Should "status" be set to zero in the begining of the function?
If the code hits this "continue" until timeout status remain
unchanged.
> + }
> +
> + *status = *buf;
> + *burst = le16_to_cpup((__le16 *)(buf + 1));
> +
> + if ((*status & mask) == mask &&
> + *burst > 0 && *burst <= CR50_MAX_BUFSIZE - 1)
> + return 0;
> +
> + msleep(CR50_TIMEOUT_SHORT_MS);
> + } while (time_before(jiffies, stop));
> +
> + dev_err(&chip->dev, "Timeout reading burst and status\n");
> + return -ETIMEDOUT;
> +}
> +
> +/*
> + * cr50_i2c_tis_recv - TPM reception callback
> + *
> + * @chip: TPM chip information
> + * @buf: reception buffer
> + * @buf_len: buffer length to read
> + *
> + * Returns negative number for error, number of bytes read for success.
> + */
> +static int cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + int status, rc;
> + size_t burstcnt, cur, len, expected;
> + u8 addr = TPM_I2C_DATA_FIFO(priv->locality);
> + u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
> +
> + if (buf_len < TPM_HEADER_SIZE)
> + return -EINVAL;
> +
> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> +
> + if (burstcnt > buf_len || burstcnt < TPM_HEADER_SIZE) {
> + dev_err(&chip->dev,
> + "Unexpected burstcnt: %zu (max=%zu, min=%d)\n",
> + burstcnt, buf_len, TPM_HEADER_SIZE);
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + /* Read first chunk of burstcnt bytes */
> + rc = cr50_i2c_read(chip, addr, buf, burstcnt);
> + if (rc < 0) {
> + dev_err(&chip->dev, "Read of first chunk failed\n");
> + goto out_err;
> + }
> +
> + /* Determine expected data in the return buffer */
> + expected = be32_to_cpup((__be32 *)(buf + 2));
> + if (expected > buf_len) {
> + dev_err(&chip->dev, "Buffer too small to receive i2c data\n");
> + goto out_err;
> + }
> +
> + /* Now read the rest of the data */
> + cur = burstcnt;
> + while (cur < expected) {
> + /* Read updated burst count and check status */
> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> +
> + len = min_t(size_t, burstcnt, expected - cur);
> + rc = cr50_i2c_read(chip, addr, buf + cur, len);
> + if (rc < 0) {
> + dev_err(&chip->dev, "Read failed\n");
> + goto out_err;
> + }
> +
> + cur += len;
> + }
> +
> + /* Ensure TPM is done reading data */
> + rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> + if (status & TPM_STS_DATA_AVAIL) {
> + dev_err(&chip->dev, "Data still available\n");
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + cr50_release_locality(chip, CR50_NO_FORCE);
> + return cur;
> +
> +out_err:
> + /* Abort current transaction if still pending */
> + if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
> + cr50_i2c_tis_ready(chip);
> +
> + cr50_release_locality(chip, CR50_NO_FORCE);
> + return rc;
> +}
> +
> +/*
> + * cr50_i2c_tis_send - TPM emission callback
> + *
> + * @chip: TPM chip information
> + * @buf: buffer to send
> + * @len: buffer length
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + int rc, status;
> + size_t burstcnt, limit, sent = 0;
> + u8 tpm_go[4] = { TPM_STS_GO };
> + unsigned long stop;
> +
> + rc = request_locality(chip);
> + if (rc < 0)
> + return rc;
> +
> + /* Wait until TPM is ready for a command */
> + stop = jiffies + chip->timeout_b;
> + while (!(cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)) {
> + if (time_after(jiffies, stop)) {
> + rc = -ETIMEDOUT;
> + goto out_err;
> + }
> +
> + cr50_i2c_tis_ready(chip);
> + }
> +
> + while (len > 0) {
> + u8 mask = TPM_STS_VALID;
> +
> + /* Wait for data if this is not the first chunk */
> + if (sent > 0)
> + mask |= TPM_STS_DATA_EXPECT;
> +
> + /* Read burst count and check status */
> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> +
> + /*
> + * Use burstcnt - 1 to account for the address byte
> + * that is inserted by cr50_i2c_write()
> + */
> + limit = min_t(size_t, burstcnt - 1, len);
> + rc = cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(priv->locality),
> + &buf[sent], limit);
> + if (rc < 0) {
> + dev_err(&chip->dev, "Write failed\n");
> + goto out_err;
> + }
> +
> + sent += limit;
> + len -= limit;
> + }
> +
> + /* Ensure TPM is not expecting more data */
> + rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> + if (status & TPM_STS_DATA_EXPECT) {
> + dev_err(&chip->dev, "Data still expected\n");
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + /* Start the TPM command */
> + rc = cr50_i2c_write(chip, TPM_I2C_STS(priv->locality), tpm_go,
> + sizeof(tpm_go));
> + if (rc < 0) {
> + dev_err(&chip->dev, "Start command failed\n");
> + goto out_err;
> + }
> + return 0;
> +
> +out_err:
> + /* Abort current transaction if still pending */
> + if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
> + cr50_i2c_tis_ready(chip);
> +
> + cr50_release_locality(chip, CR50_NO_FORCE);
> + return rc;
> +}
> +
> +/*
> + * cr50_i2c_req_canceled - callback to notify a request cancel
> + *
> + * @chip: TPM chip information
> + * @status: status given by the cancel callback
> + *
> + * Return if command is ready or not
> + */
> +static bool cr50_i2c_req_canceled(struct tpm_chip *chip, u8 status)
> +{
> + return (status == TPM_STS_COMMAND_READY);
> +}
> +
> +static const struct tpm_class_ops cr50_i2c = {
> + .flags = TPM_OPS_AUTO_STARTUP,
> + .status = &cr50_i2c_tis_status,
> + .recv = &cr50_i2c_tis_recv,
> + .send = &cr50_i2c_tis_send,
> + .cancel = &cr50_i2c_tis_ready,
> + .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> + .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> + .req_canceled = &cr50_i2c_req_canceled,
> +};
> +
> +static const struct i2c_device_id cr50_i2c_table[] = {
> + {"cr50_i2c", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, cr50_i2c_table);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id cr50_i2c_acpi_id[] = {
> + { "GOOG0005", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, cr50_i2c_acpi_id);
> +#endif
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_cr50_i2c_match[] = {
> + { .compatible = "google,cr50", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_cr50_i2c_match);
> +#endif
> +
> +/*
> + * cr50_i2c_probe - driver prbe function
s/prbe/probe
> + *
> + * @client: i2x client information
> + * @id: i2c device id
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct tpm_chip *chip;
> + struct priv_data *priv;
> + u8 buf[4];
> + u32 vendor;
> + int rc;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -ENODEV;
> +
> + chip = tpmm_chip_alloc(dev, &cr50_i2c);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + /* cr50 is a TPM 2.0 chip */
> + chip->flags |= TPM_CHIP_FLAG_TPM2;
> + chip->flags |= TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED;
> +
> + /* Default timeouts */
> + chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> + chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
> + chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> + chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> +
> + dev_set_drvdata(&chip->dev, priv);
> + init_completion(&priv->tpm_ready);
> +
> + if (client->irq > 0) {
> + rc = devm_request_irq(dev, client->irq, cr50_i2c_int_handler,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + dev->driver->name, chip);
> + if (rc < 0) {
> + dev_err(dev, "Failed to probe IRQ %d\n", client->irq);
> + return rc;
> + }
> +
> + disable_irq(client->irq);
> + priv->irq = client->irq;
> + } else {
> + dev_warn(dev, "No IRQ, will use %ums delay for TPM ready\n",
> + CR50_TIMEOUT_NOIRQ_MS);
> + }
> +
> + rc = request_locality(chip);
> + if (rc < 0) {
> + dev_err(dev, "Could not request locality\n");
> + return rc;
> + }
> +
> + /* Read four bytes from DID_VID register */
> + rc = cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf));
> + if (rc < 0) {
> + dev_err(dev, "Could not read vendor id\n");
> + cr50_release_locality(chip, CR50_FORCE);
> + return rc;
> + }
> +
> + vendor = le32_to_cpup((__le32 *)buf);
> + if (vendor != CR50_I2C_DID_VID) {
> + dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
> + cr50_release_locality(chip, CR50_FORCE);
> + return -ENODEV;
> + }
> +
> + dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n",
> + client->addr, client->irq, vendor >> 16);
> +
> + return tpm_chip_register(chip);
> +}
> +
> +/*
> + * cr50_i2c_probe - driver prbe function
s/probe/remove
s/prbr/remove
Thanks,
Helen
> + *
> + * @client: i2x client information
> + *
> + * Returns 0
> + */
> +static int cr50_i2c_remove(struct i2c_client *client)
> +{
> + struct tpm_chip *chip = i2c_get_clientdata(client);
> +
> + tpm_chip_unregister(chip);
> + cr50_release_locality(chip, CR50_FORCE);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume);
> +
> +static struct i2c_driver cr50_i2c_driver = {
> + .id_table = cr50_i2c_table,
> + .probe = cr50_i2c_probe,
> + .remove = cr50_i2c_remove,
> + .driver = {
> + .name = "cr50_i2c",
> + .pm = &cr50_i2c_pm,
> + .acpi_match_table = ACPI_PTR(cr50_i2c_acpi_id),
> + .of_match_table = of_match_ptr(of_cr50_i2c_match),
> + },
> +};
> +
> +module_i2c_driver(cr50_i2c_driver);
> +
> +MODULE_DESCRIPTION("cr50 TPM I2C Driver");
> +MODULE_LICENSE("GPL");
>
On Fri, 20 Nov 2020, Helen Koike <[email protected]>
wrote:
> Hello Adrian,
>
> I just spotted small things (nothing major), please see below.
Hi Helen,
I've addressed all the points you raised and left some minor
comments below, but I will wait a little longer before posting v3
so others will also have a chance to review. Thank you very much!
>
> On 11/20/20 2:23 PM, Adrian Ratiu wrote:
>> From: "[email protected]" <[email protected]> Add TPM
>> 2.0 compatible I2C interface for chips with cr50 firmware.
>> The firmware running on the currently supported H1 MCU requires
>> a special driver to handle its specific protocol, and this
>> makes it unsuitable to use tpm_tis_core_* and instead it must
>> implement the underlying TPM protocol similar to the other I2C
>> TPM drivers. - All 4 byes of status register must be
>> read/written at once.
>
> s/byes/bytes
>
>> - FIFO and burst count is limited to 63 and must be drained by
>> AP. - Provides an interrupt to indicate when read response
>> data is ready and when the TPM is finished processing write
>> data. This driver is based on the existing infineon I2C TPM
>> driver, which most closely matches the cr50 i2c protocol
>> behavior. Cc: Helen Koike <[email protected]>
>> Signed-off-by: Duncan Laurie <[email protected]>
>> [[email protected]: Depend on i2c even if it's a module,
>> replace boilier plate with SPDX tag, drop asm/byteorder.h
>> include, simplify return from probe] Signed-off-by: Stephen
>> Boyd <[email protected]> Signed-off-by: Fabien Lahoudere
>> <[email protected]> Signed-off-by: Adrian Ratiu
>> <[email protected]> --- Changes in v2:
>> - Various small fixes all over (reorder includes,
>> MAX_BUFSIZE, comments, etc) - Reworked return values of
>> i2c_wait_tpm_ready() to fix timeout mis-handling
>> so ret == 0 now means success, the wait period jiffies is
>> ignored because that number is meaningless and return a proper
>> timeout error in case jiffies == 0.
>> - Make i2c default to 1 message per transfer (requested by
>> Helen) - Move -EIO error reporting to transfer function to
>> cleanup transfer() itself
>> and its R/W callers
>> - Remove magic value hardcodings and introduce enum
>> force_release.
>> v1 posted at https://lkml.org/lkml/2020/2/25/349 Applies on
>> next-20201120, tested on Chromebook EVE. ---
>> drivers/char/tpm/Kconfig | 10 +
>> drivers/char/tpm/Makefile | 2 +
>> drivers/char/tpm/tpm_tis_i2c_cr50.c | 768
>> ++++++++++++++++++++++++++++ 3 files changed, 780
>> insertions(+) create mode 100644
>> drivers/char/tpm/tpm_tis_i2c_cr50.c
>> diff --git a/drivers/char/tpm/Kconfig
>> b/drivers/char/tpm/Kconfig index a18c314da211..4308f9ca7a43
>> 100644 --- a/drivers/char/tpm/Kconfig +++
>> b/drivers/char/tpm/Kconfig @@ -86,6 +86,16 @@ config
>> TCG_TIS_SYNQUACER
>> To compile this driver as a module, choose M here; the
>> module will be called tpm_tis_synquacer.
>> +config TCG_TIS_I2C_CR50 + tristate "TPM Interface
>> Specification 2.0 Interface (I2C - CR50)" + depends on I2C +
>> select TCG_CR50 + help + This is a driver for the Google
>> cr50 I2C TPM interface which is a + custom microcontroller
>> and requires a custom i2c protocol interface + to
>> handle the limitations of the hardware. To compile this driver
>> + as a module, choose M here; the module will be called
>> tcg_tis_i2c_cr50. +
>> config TCG_TIS_I2C_ATMEL tristate "TPM Interface Specification
>> 1.2 Interface (I2C - Atmel)" depends on I2C
>> diff --git a/drivers/char/tpm/Makefile
>> b/drivers/char/tpm/Makefile index 84db4fb3a9c9..66d39ea6bd10
>> 100644 --- a/drivers/char/tpm/Makefile +++
>> b/drivers/char/tpm/Makefile @@ -27,6 +27,8 @@
>> obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
>> tpm_tis_spi-y := tpm_tis_spi_main.o
>> tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
>> +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o +
>> obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
>> obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
>> obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
>> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c
>> b/drivers/char/tpm/tpm_tis_i2c_cr50.c new file mode 100644
>> index 000000000000..37555dafdca0 --- /dev/null +++
>> b/drivers/char/tpm/tpm_tis_i2c_cr50.c @@ -0,0 +1,768 @@ +//
>> SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2016 Google
>> Inc. + * + * Based on Linux Kernel TPM driver by + * Peter
>> Huewe <[email protected]> + * Copyright (C) 2011
>> Infineon Technologies + */ + +/* + * cr50 is a firmware for H1
>> secure modules that requires special + * handling for the I2C
>> interface. + * + * - Use an interrupt for transaction status
>> instead of hardcoded delays + * - Must use write+wait+read read
>> protocol + * - All 4 bytes of status register must be
>> read/written at once + * - Burst count max is 63 bytes, and
>> burst count behaves + * slightly differently than other I2C
>> TPMs + * - When reading from FIFO the full burstcnt must be
>> read + * instead of just reading header and determining the
>> remainder + */ + +#include <linux/acpi.h> +#include
>> <linux/completion.h> +#include <linux/i2c.h> +#include
>> <linux/interrupt.h> +#include <linux/module.h> +#include
>> <linux/pm.h> +#include <linux/slab.h> +#include <linux/wait.h>
>> + +#include "tpm_tis_core.h" + +#define CR50_MAX_BUFSIZE 64
>> +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout
>> during transactions */ +#define CR50_TIMEOUT_NOIRQ_MS 20
>> /* Timeout for TPM ready without IRQ */ +#define
>> CR50_I2C_DID_VID 0x00281ae0L +#define CR50_I2C_MAX_RETRIES
>> 3 /* Max retries due to I2C errors */ +#define
>> CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between
>> retries on I2C */ +#define CR50_I2C_RETRY_DELAY_HI 65 /*
>> Max usecs between retries on I2C */ + +#define
>> TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4)) +#define
>> TPM_I2C_STS(l) (0x0001 | ((l) << 4)) +#define
>> TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4)) +#define
>> TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4)) + +struct priv_data
>> { + int irq; + int locality; + struct completion
>> tpm_ready; + u8 buf[CR50_MAX_BUFSIZE]; +}; + +enum
>> force_release { + CR50_NO_FORCE = 0x0, + CR50_FORCE = 0x1,
>> +};
>
> No need for hexadecimal
Indeed, just a bad habbit of mine, will drop the numbers entirely
as the values are always the same.
>
>> + +/* + * The cr50 interrupt handler just signals waiting
>> threads that the + * interrupt was asserted. It does not do
>> any processing triggered + * by interrupts but is instead used
>> to avoid fixed delays. + * + * @dummy: unuesed parameter
>
> s/unuesed/unused
>
Really should enable my spellchecker :) Thanks. All mispellings
you pointed out are fixed now.
>> + * @dev_id: TPM chip information + */ +static irqreturn_t
>> cr50_i2c_int_handler(int dummy, void *dev_id) +{ + struct
>> tpm_chip *chip = dev_id; + struct priv_data *priv =
>> dev_get_drvdata(&chip->dev); + +
>> complete(&priv->tpm_ready); + + return IRQ_HANDLED; +} +
>> +/* + * Wait for completion interrupt if available, otherwise
>> use a fixed + * delay for the TPM to be ready. + * + * @chip:
>> TPM chip information + * + * Returns 0 for success, negative
>> number for timeout + */ +static int
>> cr50_i2c_wait_tpm_ready(struct tpm_chip *chip) +{ + struct
>> priv_data *priv = dev_get_drvdata(&chip->dev); + + /* Use a
>> safe fixed delay if interrupt is not supported */ + if
>> (priv->irq <= 0) { + msleep(CR50_TIMEOUT_NOIRQ_MS); +
>> return 0; + } + + /* Wait for interrupt to indicate TPM is
>> ready to respond */ + if
>> (!wait_for_completion_timeout(&priv->tpm_ready, +
>> msecs_to_jiffies(chip->timeout_a))) { +
>> dev_warn(&chip->dev, "Timeout waiting for TPM ready\n"); +
>> return -ETIMEDOUT; + } + + return 0; +} + +/* + *
>> cr50_i2c_enable_tpm_irq - enable TPM irq + * + * @chip: TPM
>> chip information + */ +static void
>> cr50_i2c_enable_tpm_irq(struct tpm_chip *chip) +{ + struct
>> priv_data *priv = dev_get_drvdata(&chip->dev); + + if
>> (priv->irq > 0) { +
>> reinit_completion(&priv->tpm_ready); +
>> enable_irq(priv->irq); + } +} + +/* + *
>> cr50_i2c_disable_tpm_irq - disable TPM irq + * + * @chip: TPM
>> chip information + */ +static void
>> cr50_i2c_disable_tpm_irq(struct tpm_chip *chip) +{ + struct
>> priv_data *priv = dev_get_drvdata(&chip->dev); + + if
>> (priv->irq > 0) + disable_irq(priv->irq); +} + +/* +
>> * cr50_i2c_transfer_message - transfer a message over i2c + * +
>> * @dev: device information + * @adapter: i2c adapter + * @msg:
>> message to transfer + * + * Call unlocked i2c transfer routine
>> with the provided parameters and retry + * in case of bus
>> errors. Returns 0 on success, otherwise negative errno. + */
>> +static int cr50_i2c_transfer_message(struct device *dev, +
>> struct i2c_adapter *adapter, +
>> struct i2c_msg *msg) +{ + int rc = 0;
>
> No need to init to zero.
>
>> + unsigned int try; + + for (try = 0; try <
>> CR50_I2C_MAX_RETRIES; try++) { + rc =
>> __i2c_transfer(adapter, msg, 1); + if (rc == 1) +
>> return 0; /* Successfully transferred the message */ +
>> if (try) + dev_warn(dev, "i2c transfer failed
>> (attempt %d/%d): %d\n", + try + 1,
>> CR50_I2C_MAX_RETRIES, rc); +
>> usleep_range(CR50_I2C_RETRY_DELAY_LO, CR50_I2C_RETRY_DELAY_HI);
>> + } + + return -EIO; /* No i2c message transferred */ +} +
>> +/* + * cr50_i2c_read() - read from TPM register + * + * @chip:
>> TPM chip information + * @addr: register address to read from +
>> * @buffer: provided by caller + * @len: number of bytes to read
>> + * + * 1) send register address byte 'addr' to the TPM + * 2)
>> wait for TPM to indicate it is ready + * 3) read 'len' bytes of
>> TPM response into the provided 'buffer' + * + * Returns
>> negative number for error, 0 for success. + */ +static int
>> cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer,
>> size_t len) +{ + struct i2c_client *client =
>> to_i2c_client(chip->dev.parent); + struct i2c_msg msg1 = {
>
> Maybe rename to msg_reg_addr (I think it is more meaningful then
> numbers).
>
>> + .addr = client->addr, + .len = 1, +
>> .buf = &addr + }; + struct i2c_msg msg2 = {
>
> Maybe rename to msg_response (I think it is more meaningful then
> numbers).
>
>> + .addr = client->addr, + .flags = I2C_M_RD,
>> + .len = len, + .buf = buffer + }; +
>> int rc; + + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); +
>> + /* Prepare for completion interrupt */ +
>> cr50_i2c_enable_tpm_irq(chip); + + /* Send the register
>> address byte to the TPM */ + rc =
>> cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
>> + if (rc < 0) + goto out; + + /* Wait for TPM to
>> be ready with response data */ + rc =
>> cr50_i2c_wait_tpm_ready(chip); + if (rc < 0) +
>> goto out; + + /* Read response data from the TPM */ + rc
>> = cr50_i2c_transfer_message(&chip->dev, client->adapter,
>> &msg2); + +out: + cr50_i2c_disable_tpm_irq(chip); +
>> i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); + + if
>> (rc < 0) + return rc; + + return 0; +} + +/* + *
>> cr50_i2c_write() - write to TPM register + * + * @chip: TPM
>> chip information + * @addr: register address to write to + *
>> @buffer: data to write + * @len: number of bytes to write + * +
>> * 1) prepend the provided address to the provided data + * 2)
>> send the address+data to the TPM + * 3) wait for TPM to
>> indicate it is done writing + * + * Returns negative number for
>> error, 0 for success. + */ +static int cr50_i2c_write(struct
>> tpm_chip *chip, u8 addr, u8 *buffer, +
>> size_t len) +{ + struct priv_data *priv =
>> dev_get_drvdata(&chip->dev); + struct i2c_client *client
>> = to_i2c_client(chip->dev.parent); + struct i2c_msg msg1 = {
>
> Maybe rename to just msg.
>
Done done and done :)
>> + .addr = client->addr, + .len = len + 1, +
>> .buf = priv->buf + }; + int rc; + + if (len >
>> CR50_MAX_BUFSIZE - 1) + return -EINVAL; + +
>> i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); + + /* Prepend
>> the 'register address' to the buffer */ + priv->buf[0] =
>> addr; + memcpy(priv->buf + 1, buffer, len);
>
> Shouldn't this copy be done before the lock?
>
Yes, we can move the copy before the lock because it's always a
good idea to minimize critical sections.
>> + + /* Prepare for completion interrupt */ +
>> cr50_i2c_enable_tpm_irq(chip); + + /* Send write request
>> buffer with address */ + rc =
>> cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
>> + if (rc < 0) + goto out; + + /* Wait for TPM to
>> be ready, ignore timeout */ +
>> cr50_i2c_wait_tpm_ready(chip); + +out: +
>> cr50_i2c_disable_tpm_irq(chip); +
>> i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); + + if
>> (rc < 0) + return rc; + + return 0; +} + +/* + *
>> cr50_check_locality - verify TPM locality + * + * @chip: TPM
>> chip information + * + * Returns negative number for error, 0
>> for success. + */ +static int cr50_check_locality(struct
>> tpm_chip *chip) +{ + u8 mask = TPM_ACCESS_VALID |
>> TPM_ACCESS_ACTIVE_LOCALITY; + u8 buf; + int rc; +
>> + rc = cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf,
>> sizeof(buf)); + if (rc < 0) + return rc; + + if
>> ((buf & mask) == mask) + return 0; + + return
>> -EIO; +} + +/* + * cr50_release_locality - release TPM locality
>> + * + * @chip: TPM chip information + * @force: flag to force
>> release if set + */ +static void cr50_release_locality(struct
>> tpm_chip *chip, + enum
>> force_release force) +{ + struct priv_data *priv =
>> dev_get_drvdata(&chip->dev); + u8 mask = TPM_ACCESS_VALID
>> | TPM_ACCESS_REQUEST_PENDING; + u8 addr =
>> TPM_I2C_ACCESS(priv->locality); + u8 buf; + + if
>> (cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0) +
>> return; + + if (force || (buf & mask) == mask) { +
>> buf = TPM_ACCESS_ACTIVE_LOCALITY; +
>> cr50_i2c_write(chip, addr, &buf, sizeof(buf)); + } + +
>> priv->locality = 0;
>
> I'm probally missing something, I don't see priv->locality being
> set to another value other then 0 in the code.
>
> Shoul it be set in request_locality() or in
> cr50_release_locality() ?
>
Oh I thought I deleted that but turns out it was missed while
addressing your initial review. Oops. Thanks for the dilligence.
In a nutshell this chip always uses locality 0 and I think that
private variable is only a reminiscence from the infineon code
this was based upon, so we should drop it as we don't need to
track a constant value.
>> +} + +/* + * request_locality - request TPM locality + * + *
>> @chip: TPM chip information + */ +static int
>> request_locality(struct tpm_chip *chip)
>
> All the other function have c50_ prefix.
>
>> +{ + struct priv_data *priv = dev_get_drvdata(&chip->dev); + u8
>> buf = TPM_ACCESS_REQUEST_USE; + unsigned long stop; +
>> int rc; + + if (!cr50_check_locality(chip)) +
>> return 0; + + rc = cr50_i2c_write(chip,
>> TPM_I2C_ACCESS(0), &buf, sizeof(buf)); + if (rc < 0) +
>> return rc; + + stop = jiffies + chip->timeout_a; + do
>> { + if (!cr50_check_locality(chip)) { +
>> priv->locality = 0; + return 0; +
>> } + msleep(CR50_TIMEOUT_SHORT_MS); + } while
>> (time_before(jiffies, stop)); + + return -ETIMEDOUT; +} +
>> +/* + * cr50 requires all 4 bytes of status register to be read
>> + * + * @chip: TPM chip information + * + * Returns TPM status
>> + */ +static u8 cr50_i2c_tis_status(struct tpm_chip *chip) +{ +
>> struct priv_data *priv = dev_get_drvdata(&chip->dev); + u8
>> buf[4]; + + if (cr50_i2c_read(chip,
>> TPM_I2C_STS(priv->locality), + buf,
>> sizeof(buf)) < 0) + return 0; + return buf[0]; +}
>> + +/* + * cr50 requires all 4 bytes of status register to be
>> written + * + * @chip: TPM chip information + */ +static void
>> cr50_i2c_tis_ready(struct tpm_chip *chip) +{ + struct
>> priv_data *priv = dev_get_drvdata(&chip->dev); + u8 buf[4]
>> = { TPM_STS_COMMAND_READY }; + + cr50_i2c_write(chip,
>> TPM_I2C_STS(priv->locality), buf, sizeof(buf)); +
>> msleep(CR50_TIMEOUT_SHORT_MS); +} + +/* + * cr50 uses bytes 3:2
>> of status register for burst count and + * all 4 bytes must be
>> read + * + * @chip: TPM chip information + * @mask: status mask
>> + * @burst: return value for burst + * @status: return value
>> for statis
>
> s/statis/status
>
>> + * + * Returns negative number for error, 0 for success. + */
>> +static int cr50_i2c_wait_burststs(struct tpm_chip *chip, u8
>> mask,
>
> Maybe rename to cr50_i2c_get_burst_and_status().
>
>> + size_t *burst, int *status)
>
> Should status be unsigned?
>
Yes, done.
>> +{ + struct priv_data *priv = dev_get_drvdata(&chip->dev); +
>> unsigned long stop; + u8 buf[4]; + + /* wait for
>> burstcount */ + stop = jiffies + chip->timeout_b; + do
>> { + if (cr50_i2c_read(chip,
>> TPM_I2C_STS(priv->locality), +
>> buf, sizeof(buf)) < 0) { +
>> msleep(CR50_TIMEOUT_SHORT_MS); + continue;
>
> Should "status" be set to zero in the begining of the function?
> If the code hits this "continue" until timeout status remain
> unchanged.
>
Yes, nice find.
>> + }
>> +
>> + *status = *buf;
>> + *burst = le16_to_cpup((__le16 *)(buf + 1));
>> +
>> + if ((*status & mask) == mask &&
>> + *burst > 0 && *burst <= CR50_MAX_BUFSIZE - 1)
>> + return 0;
>> +
>> + msleep(CR50_TIMEOUT_SHORT_MS);
>> + } while (time_before(jiffies, stop));
>> +
>> + dev_err(&chip->dev, "Timeout reading burst and status\n");
>> + return -ETIMEDOUT;
>> +}
>> +
>> +/*
>> + * cr50_i2c_tis_recv - TPM reception callback
>> + *
>> + * @chip: TPM chip information
>> + * @buf: reception buffer
>> + * @buf_len: buffer length to read
>> + *
>> + * Returns negative number for error, number of bytes read for success.
>> + */
>> +static int cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
>> +{
>> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
>> + int status, rc;
>> + size_t burstcnt, cur, len, expected;
>> + u8 addr = TPM_I2C_DATA_FIFO(priv->locality);
>> + u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
>> +
>> + if (buf_len < TPM_HEADER_SIZE)
>> + return -EINVAL;
>> +
>> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
>> + if (rc < 0)
>> + goto out_err;
>> +
>> + if (burstcnt > buf_len || burstcnt < TPM_HEADER_SIZE) {
>> + dev_err(&chip->dev,
>> + "Unexpected burstcnt: %zu (max=%zu, min=%d)\n",
>> + burstcnt, buf_len, TPM_HEADER_SIZE);
>> + rc = -EIO;
>> + goto out_err;
>> + }
>> +
>> + /* Read first chunk of burstcnt bytes */
>> + rc = cr50_i2c_read(chip, addr, buf, burstcnt);
>> + if (rc < 0) {
>> + dev_err(&chip->dev, "Read of first chunk failed\n");
>> + goto out_err;
>> + }
>> +
>> + /* Determine expected data in the return buffer */
>> + expected = be32_to_cpup((__be32 *)(buf + 2));
>> + if (expected > buf_len) {
>> + dev_err(&chip->dev, "Buffer too small to receive i2c data\n");
>> + goto out_err;
>> + }
>> +
>> + /* Now read the rest of the data */
>> + cur = burstcnt;
>> + while (cur < expected) {
>> + /* Read updated burst count and check status */
>> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
>> + if (rc < 0)
>> + goto out_err;
>> +
>> + len = min_t(size_t, burstcnt, expected - cur);
>> + rc = cr50_i2c_read(chip, addr, buf + cur, len);
>> + if (rc < 0) {
>> + dev_err(&chip->dev, "Read failed\n");
>> + goto out_err;
>> + }
>> +
>> + cur += len;
>> + }
>> +
>> + /* Ensure TPM is done reading data */
>> + rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
>> + if (rc < 0)
>> + goto out_err;
>> + if (status & TPM_STS_DATA_AVAIL) {
>> + dev_err(&chip->dev, "Data still available\n");
>> + rc = -EIO;
>> + goto out_err;
>> + }
>> +
>> + cr50_release_locality(chip, CR50_NO_FORCE);
>> + return cur;
>> +
>> +out_err:
>> + /* Abort current transaction if still pending */
>> + if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
>> + cr50_i2c_tis_ready(chip);
>> +
>> + cr50_release_locality(chip, CR50_NO_FORCE);
>> + return rc;
>> +}
>> +
>> +/*
>> + * cr50_i2c_tis_send - TPM emission callback
>> + *
>> + * @chip: TPM chip information
>> + * @buf: buffer to send
>> + * @len: buffer length
>> + *
>> + * Returns negative number for error, 0 for success.
>> + */
>> +static int cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>> +{
>> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
>> + int rc, status;
>> + size_t burstcnt, limit, sent = 0;
>> + u8 tpm_go[4] = { TPM_STS_GO };
>> + unsigned long stop;
>> +
>> + rc = request_locality(chip);
>> + if (rc < 0)
>> + return rc;
>> +
>> + /* Wait until TPM is ready for a command */
>> + stop = jiffies + chip->timeout_b;
>> + while (!(cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)) {
>> + if (time_after(jiffies, stop)) {
>> + rc = -ETIMEDOUT;
>> + goto out_err;
>> + }
>> +
>> + cr50_i2c_tis_ready(chip);
>> + }
>> +
>> + while (len > 0) {
>> + u8 mask = TPM_STS_VALID;
>> +
>> + /* Wait for data if this is not the first chunk */
>> + if (sent > 0)
>> + mask |= TPM_STS_DATA_EXPECT;
>> +
>> + /* Read burst count and check status */
>> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
>> + if (rc < 0)
>> + goto out_err;
>> +
>> + /*
>> + * Use burstcnt - 1 to account for the address byte
>> + * that is inserted by cr50_i2c_write()
>> + */
>> + limit = min_t(size_t, burstcnt - 1, len);
>> + rc = cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(priv->locality),
>> + &buf[sent], limit);
>> + if (rc < 0) {
>> + dev_err(&chip->dev, "Write failed\n");
>> + goto out_err;
>> + }
>> +
>> + sent += limit;
>> + len -= limit;
>> + }
>> +
>> + /* Ensure TPM is not expecting more data */
>> + rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
>> + if (rc < 0)
>> + goto out_err;
>> + if (status & TPM_STS_DATA_EXPECT) {
>> + dev_err(&chip->dev, "Data still expected\n");
>> + rc = -EIO;
>> + goto out_err;
>> + }
>> +
>> + /* Start the TPM command */
>> + rc = cr50_i2c_write(chip, TPM_I2C_STS(priv->locality), tpm_go,
>> + sizeof(tpm_go));
>> + if (rc < 0) {
>> + dev_err(&chip->dev, "Start command failed\n");
>> + goto out_err;
>> + }
>> + return 0;
>> +
>> +out_err:
>> + /* Abort current transaction if still pending */
>> + if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
>> + cr50_i2c_tis_ready(chip);
>> +
>> + cr50_release_locality(chip, CR50_NO_FORCE);
>> + return rc;
>> +}
>> +
>> +/*
>> + * cr50_i2c_req_canceled - callback to notify a request cancel
>> + *
>> + * @chip: TPM chip information
>> + * @status: status given by the cancel callback
>> + *
>> + * Return if command is ready or not
>> + */
>> +static bool cr50_i2c_req_canceled(struct tpm_chip *chip, u8 status)
>> +{
>> + return (status == TPM_STS_COMMAND_READY);
>> +}
>> +
>> +static const struct tpm_class_ops cr50_i2c = {
>> + .flags = TPM_OPS_AUTO_STARTUP,
>> + .status = &cr50_i2c_tis_status,
>> + .recv = &cr50_i2c_tis_recv,
>> + .send = &cr50_i2c_tis_send,
>> + .cancel = &cr50_i2c_tis_ready,
>> + .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> + .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> + .req_canceled = &cr50_i2c_req_canceled,
>> +};
>> +
>> +static const struct i2c_device_id cr50_i2c_table[] = {
>> + {"cr50_i2c", 0},
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, cr50_i2c_table);
>> +
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id cr50_i2c_acpi_id[] = {
>> + { "GOOG0005", 0 },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, cr50_i2c_acpi_id);
>> +#endif
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_cr50_i2c_match[] = {
>> + { .compatible = "google,cr50", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, of_cr50_i2c_match);
>> +#endif
>> +
>> +/*
>> + * cr50_i2c_probe - driver prbe function
>
> s/prbe/probe
>
>> + *
>> + * @client: i2x client information
>> + * @id: i2c device id
>> + *
>> + * Returns negative number for error, 0 for success.
>> + */
>> +static int cr50_i2c_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct device *dev = &client->dev;
>> + struct tpm_chip *chip;
>> + struct priv_data *priv;
>> + u8 buf[4];
>> + u32 vendor;
>> + int rc;
>> +
>> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>> + return -ENODEV;
>> +
>> + chip = tpmm_chip_alloc(dev, &cr50_i2c);
>> + if (IS_ERR(chip))
>> + return PTR_ERR(chip);
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + /* cr50 is a TPM 2.0 chip */
>> + chip->flags |= TPM_CHIP_FLAG_TPM2;
>> + chip->flags |= TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED;
>> +
>> + /* Default timeouts */
>> + chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
>> + chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
>> + chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
>> + chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
>> +
>> + dev_set_drvdata(&chip->dev, priv);
>> + init_completion(&priv->tpm_ready);
>> +
>> + if (client->irq > 0) {
>> + rc = devm_request_irq(dev, client->irq, cr50_i2c_int_handler,
>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> + dev->driver->name, chip);
>> + if (rc < 0) {
>> + dev_err(dev, "Failed to probe IRQ %d\n", client->irq);
>> + return rc;
>> + }
>> +
>> + disable_irq(client->irq);
>> + priv->irq = client->irq;
>> + } else {
>> + dev_warn(dev, "No IRQ, will use %ums delay for TPM ready\n",
>> + CR50_TIMEOUT_NOIRQ_MS);
>> + }
>> +
>> + rc = request_locality(chip);
>> + if (rc < 0) {
>> + dev_err(dev, "Could not request locality\n");
>> + return rc;
>> + }
>> +
>> + /* Read four bytes from DID_VID register */
>> + rc = cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf));
>> + if (rc < 0) {
>> + dev_err(dev, "Could not read vendor id\n");
>> + cr50_release_locality(chip, CR50_FORCE);
>> + return rc;
>> + }
>> +
>> + vendor = le32_to_cpup((__le32 *)buf);
>> + if (vendor != CR50_I2C_DID_VID) {
>> + dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
>> + cr50_release_locality(chip, CR50_FORCE);
>> + return -ENODEV;
>> + }
>> +
>> + dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n",
>> + client->addr, client->irq, vendor >> 16);
>> +
>> + return tpm_chip_register(chip);
>> +}
>> +
>> +/*
>> + * cr50_i2c_probe - driver prbe function
>
> s/probe/remove
> s/prbr/remove
>
>
> Thanks,
> Helen
>
>> + *
>> + * @client: i2x client information
>> + *
>> + * Returns 0
>> + */
>> +static int cr50_i2c_remove(struct i2c_client *client)
>> +{
>> + struct tpm_chip *chip = i2c_get_clientdata(client);
>> +
>> + tpm_chip_unregister(chip);
>> + cr50_release_locality(chip, CR50_FORCE);
>> +
>> + return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume);
>> +
>> +static struct i2c_driver cr50_i2c_driver = {
>> + .id_table = cr50_i2c_table,
>> + .probe = cr50_i2c_probe,
>> + .remove = cr50_i2c_remove,
>> + .driver = {
>> + .name = "cr50_i2c",
>> + .pm = &cr50_i2c_pm,
>> + .acpi_match_table = ACPI_PTR(cr50_i2c_acpi_id),
>> + .of_match_table = of_match_ptr(of_cr50_i2c_match),
>> + },
>> +};
>> +
>> +module_i2c_driver(cr50_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("cr50 TPM I2C Driver");
>> +MODULE_LICENSE("GPL");
>>
On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu wrote:
> From: "[email protected]" <[email protected]>
>
> Add TPM 2.0 compatible I2C interface for chips with cr50 firmware.
>
> The firmware running on the currently supported H1 MCU requires a
> special driver to handle its specific protocol, and this makes it
> unsuitable to use tpm_tis_core_* and instead it must implement the
> underlying TPM protocol similar to the other I2C TPM drivers.
>
> - All 4 byes of status register must be read/written at once.
> - FIFO and burst count is limited to 63 and must be drained by AP.
> - Provides an interrupt to indicate when read response data is ready
> and when the TPM is finished processing write data.
>
> This driver is based on the existing infineon I2C TPM driver, which
> most closely matches the cr50 i2c protocol behavior.
>
> Cc: Helen Koike <[email protected]>
> Signed-off-by: Duncan Laurie <[email protected]>
> [[email protected]: Depend on i2c even if it's a module, replace
> boilier plate with SPDX tag, drop asm/byteorder.h include, simplify
> return from probe]
> Signed-off-by: Stephen Boyd <[email protected]>
> Signed-off-by: Fabien Lahoudere <[email protected]>
> Signed-off-by: Adrian Ratiu <[email protected]>
> ---
> Changes in v2:
> - Various small fixes all over (reorder includes, MAX_BUFSIZE, comments, etc)
> - Reworked return values of i2c_wait_tpm_ready() to fix timeout mis-handling
> so ret == 0 now means success, the wait period jiffies is ignored because that
> number is meaningless and return a proper timeout error in case jiffies == 0.
> - Make i2c default to 1 message per transfer (requested by Helen)
> - Move -EIO error reporting to transfer function to cleanup transfer() itself
> and its R/W callers
> - Remove magic value hardcodings and introduce enum force_release.
>
> v1 posted at https://lkml.org/lkml/2020/2/25/349
>
> Applies on next-20201120, tested on Chromebook EVE.
> ---
> drivers/char/tpm/Kconfig | 10 +
> drivers/char/tpm/Makefile | 2 +
> drivers/char/tpm/tpm_tis_i2c_cr50.c | 768 ++++++++++++++++++++++++++++
> 3 files changed, 780 insertions(+)
> create mode 100644 drivers/char/tpm/tpm_tis_i2c_cr50.c
>
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index a18c314da211..4308f9ca7a43 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
> To compile this driver as a module, choose M here;
> the module will be called tpm_tis_synquacer.
>
> +config TCG_TIS_I2C_CR50
> + tristate "TPM Interface Specification 2.0 Interface (I2C - CR50)"
> + depends on I2C
> + select TCG_CR50
> + help
> + This is a driver for the Google cr50 I2C TPM interface which is a
> + custom microcontroller and requires a custom i2c protocol interface
> + to handle the limitations of the hardware. To compile this driver
> + as a module, choose M here; the module will be called tcg_tis_i2c_cr50.
> +
> config TCG_TIS_I2C_ATMEL
> tristate "TPM Interface Specification 1.2 Interface (I2C - Atmel)"
> depends on I2C
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 84db4fb3a9c9..66d39ea6bd10 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> tpm_tis_spi-y := tpm_tis_spi_main.o
> tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
>
> +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
> +
> obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
> obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
> obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> new file mode 100644
> index 000000000000..37555dafdca0
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> @@ -0,0 +1,768 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2016 Google Inc.
> + *
> + * Based on Linux Kernel TPM driver by
> + * Peter Huewe <[email protected]>
> + * Copyright (C) 2011 Infineon Technologies
> + */
> +
> +/*
> + * cr50 is a firmware for H1 secure modules that requires special
> + * handling for the I2C interface.
> + *
> + * - Use an interrupt for transaction status instead of hardcoded delays
> + * - Must use write+wait+read read protocol
> + * - All 4 bytes of status register must be read/written at once
> + * - Burst count max is 63 bytes, and burst count behaves
> + * slightly differently than other I2C TPMs
> + * - When reading from FIFO the full burstcnt must be read
> + * instead of just reading header and determining the remainder
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/completion.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>
> +
> +#include "tpm_tis_core.h"
> +
> +#define CR50_MAX_BUFSIZE 64
> +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during transactions */
> +#define CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready without IRQ */
> +#define CR50_I2C_DID_VID 0x00281ae0L
> +#define CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */
> +#define CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries on I2C */
> +#define CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries on I2C */
CR50_ -> TPM_CR50_
> +
> +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
> +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
> +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
> +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4))
> +
> +struct priv_data {
> + int irq;
> + int locality;
> + struct completion tpm_ready;
> + u8 buf[CR50_MAX_BUFSIZE];
> +};
tpm_i2c_cr50_priv_data
> +
> +enum force_release {
> + CR50_NO_FORCE = 0x0,
> + CR50_FORCE = 0x1,
> +};
I'd just
#define TPM_I2C_CR50_NO_FORCE 0
#define TPM_I2C_CR50_FORCE 1
> +
> +/*
> + * The cr50 interrupt handler just signals waiting threads that the
> + * interrupt was asserted. It does not do any processing triggered
> + * by interrupts but is instead used to avoid fixed delays.
> + *
> + * @dummy: unuesed parameter
> + * @dev_id: TPM chip information
> + */
Please review for kdoc:
https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> +static irqreturn_t cr50_i2c_int_handler(int dummy, void *dev_id)
cr50 -> tpm_i2c_cr50_
> +{
> + struct tpm_chip *chip = dev_id;
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> +
> + complete(&priv->tpm_ready);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/*
> + * Wait for completion interrupt if available, otherwise use a fixed
> + * delay for the TPM to be ready.
> + *
> + * @chip: TPM chip information
> + *
> + * Returns 0 for success, negative number for timeout
> + */
> +static int cr50_i2c_wait_tpm_ready(struct tpm_chip *chip)
Ditto.
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> +
> + /* Use a safe fixed delay if interrupt is not supported */
> + if (priv->irq <= 0) {
> + msleep(CR50_TIMEOUT_NOIRQ_MS);
> + return 0;
> + }
> +
> + /* Wait for interrupt to indicate TPM is ready to respond */
> + if (!wait_for_completion_timeout(&priv->tpm_ready,
> + msecs_to_jiffies(chip->timeout_a))) {
> + dev_warn(&chip->dev, "Timeout waiting for TPM ready\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * cr50_i2c_enable_tpm_irq - enable TPM irq
> + *
> + * @chip: TPM chip information
> + */
> +static void cr50_i2c_enable_tpm_irq(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> +
> + if (priv->irq > 0) {
> + reinit_completion(&priv->tpm_ready);
> + enable_irq(priv->irq);
> + }
> +}
> +
> +/*
> + * cr50_i2c_disable_tpm_irq - disable TPM irq
> + *
> + * @chip: TPM chip information
> + */
> +static void cr50_i2c_disable_tpm_irq(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> +
> + if (priv->irq > 0)
> + disable_irq(priv->irq);
> +}
> +
> +/*
> + * cr50_i2c_transfer_message - transfer a message over i2c
> + *
> + * @dev: device information
> + * @adapter: i2c adapter
> + * @msg: message to transfer
> + *
> + * Call unlocked i2c transfer routine with the provided parameters and retry
> + * in case of bus errors. Returns 0 on success, otherwise negative errno.
> + */
> +static int cr50_i2c_transfer_message(struct device *dev,
> + struct i2c_adapter *adapter,
> + struct i2c_msg *msg)
> +{
> + int rc = 0;
> + unsigned int try;
> +
> + for (try = 0; try < CR50_I2C_MAX_RETRIES; try++) {
> + rc = __i2c_transfer(adapter, msg, 1);
> + if (rc == 1)
> + return 0; /* Successfully transferred the message */
> + if (try)
> + dev_warn(dev, "i2c transfer failed (attempt %d/%d): %d\n",
> + try + 1, CR50_I2C_MAX_RETRIES, rc);
> + usleep_range(CR50_I2C_RETRY_DELAY_LO, CR50_I2C_RETRY_DELAY_HI);
> + }
> +
> + return -EIO; /* No i2c message transferred */
> +}
> +
> +/*
> + * cr50_i2c_read() - read from TPM register
> + *
Not only wrong formatting but seems that these kdoc's have differing
formatting (no newline).
> + * @chip: TPM chip information
> + * @addr: register address to read from
> + * @buffer: provided by caller
> + * @len: number of bytes to read
> + *
> + * 1) send register address byte 'addr' to the TPM
> + * 2) wait for TPM to indicate it is ready
> + * 3) read 'len' bytes of TPM response into the provided 'buffer'
Please write proper sentences instead of lower-case (also applies
to parameters as you can see from kernel documentation).
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t len)
> +{
> + struct i2c_client *client = to_i2c_client(chip->dev.parent);
> + struct i2c_msg msg1 = {
> + .addr = client->addr,
> + .len = 1,
> + .buf = &addr
> + };
> + struct i2c_msg msg2 = {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = len,
> + .buf = buffer
> + };
> + int rc;
> +
> + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + /* Prepare for completion interrupt */
> + cr50_i2c_enable_tpm_irq(chip);
> +
> + /* Send the register address byte to the TPM */
> + rc = cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
> + if (rc < 0)
> + goto out;
> +
> + /* Wait for TPM to be ready with response data */
> + rc = cr50_i2c_wait_tpm_ready(chip);
> + if (rc < 0)
> + goto out;
> +
> + /* Read response data from the TPM */
> + rc = cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg2);
> +
> +out:
> + cr50_i2c_disable_tpm_irq(chip);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + if (rc < 0)
> + return rc;
> +
> + return 0;
> +}
> +
> +/*
> + * cr50_i2c_write() - write to TPM register
> + *
> + * @chip: TPM chip information
> + * @addr: register address to write to
> + * @buffer: data to write
> + * @len: number of bytes to write
> + *
> + * 1) prepend the provided address to the provided data
> + * 2) send the address+data to the TPM
> + * 3) wait for TPM to indicate it is done writing
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
> + size_t len)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + struct i2c_client *client = to_i2c_client(chip->dev.parent);
> + struct i2c_msg msg1 = {
> + .addr = client->addr,
> + .len = len + 1,
> + .buf = priv->buf
> + };
> + int rc;
> +
> + if (len > CR50_MAX_BUFSIZE - 1)
> + return -EINVAL;
> +
> + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + /* Prepend the 'register address' to the buffer */
> + priv->buf[0] = addr;
> + memcpy(priv->buf + 1, buffer, len);
> +
> + /* Prepare for completion interrupt */
> + cr50_i2c_enable_tpm_irq(chip);
> +
> + /* Send write request buffer with address */
> + rc = cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
> + if (rc < 0)
> + goto out;
> +
> + /* Wait for TPM to be ready, ignore timeout */
> + cr50_i2c_wait_tpm_ready(chip);
> +
> +out:
> + cr50_i2c_disable_tpm_irq(chip);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + if (rc < 0)
> + return rc;
> +
> + return 0;
> +}
> +
> +/*
> + * cr50_check_locality - verify TPM locality
> + *
> + * @chip: TPM chip information
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_check_locality(struct tpm_chip *chip)
> +{
> + u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
> + u8 buf;
> + int rc;
> +
> + rc = cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> + if (rc < 0)
> + return rc;
> +
> + if ((buf & mask) == mask)
> + return 0;
> +
> + return -EIO;
> +}
> +
> +/*
> + * cr50_release_locality - release TPM locality
> + *
> + * @chip: TPM chip information
> + * @force: flag to force release if set
> + */
> +static void cr50_release_locality(struct tpm_chip *chip,
> + enum force_release force)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
> + u8 addr = TPM_I2C_ACCESS(priv->locality);
> + u8 buf;
> +
> + if (cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0)
> + return;
> +
> + if (force || (buf & mask) == mask) {
> + buf = TPM_ACCESS_ACTIVE_LOCALITY;
> + cr50_i2c_write(chip, addr, &buf, sizeof(buf));
> + }
> +
> + priv->locality = 0;
> +}
> +
> +/*
> + * request_locality - request TPM locality
> + *
> + * @chip: TPM chip information
> + */
> +static int request_locality(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 buf = TPM_ACCESS_REQUEST_USE;
> + unsigned long stop;
> + int rc;
> +
> + if (!cr50_check_locality(chip))
> + return 0;
> +
> + rc = cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> + if (rc < 0)
> + return rc;
> +
> + stop = jiffies + chip->timeout_a;
> + do {
> + if (!cr50_check_locality(chip)) {
> + priv->locality = 0;
> + return 0;
> + }
> + msleep(CR50_TIMEOUT_SHORT_MS);
> + } while (time_before(jiffies, stop));
> +
> + return -ETIMEDOUT;
> +}
> +
> +/*
> + * cr50 requires all 4 bytes of status register to be read
> + *
> + * @chip: TPM chip information
> + *
> + * Returns TPM status
> + */
> +static u8 cr50_i2c_tis_status(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 buf[4];
> +
> + if (cr50_i2c_read(chip, TPM_I2C_STS(priv->locality),
> + buf, sizeof(buf)) < 0)
> + return 0;
> + return buf[0];
> +}
> +
> +/*
> + * cr50 requires all 4 bytes of status register to be written
> + *
> + * @chip: TPM chip information
> + */
> +static void cr50_i2c_tis_ready(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 buf[4] = { TPM_STS_COMMAND_READY };
> +
> + cr50_i2c_write(chip, TPM_I2C_STS(priv->locality), buf, sizeof(buf));
> + msleep(CR50_TIMEOUT_SHORT_MS);
> +}
> +
> +/*
> + * cr50 uses bytes 3:2 of status register for burst count and
> + * all 4 bytes must be read
> + *
> + * @chip: TPM chip information
> + * @mask: status mask
> + * @burst: return value for burst
> + * @status: return value for statis
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_wait_burststs(struct tpm_chip *chip, u8 mask,
> + size_t *burst, int *status)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + unsigned long stop;
> + u8 buf[4];
> +
> + /* wait for burstcount */
> + stop = jiffies + chip->timeout_b;
> + do {
> + if (cr50_i2c_read(chip, TPM_I2C_STS(priv->locality),
> + buf, sizeof(buf)) < 0) {
> + msleep(CR50_TIMEOUT_SHORT_MS);
> + continue;
> + }
> +
> + *status = *buf;
> + *burst = le16_to_cpup((__le16 *)(buf + 1));
> +
> + if ((*status & mask) == mask &&
> + *burst > 0 && *burst <= CR50_MAX_BUFSIZE - 1)
> + return 0;
> +
> + msleep(CR50_TIMEOUT_SHORT_MS);
> + } while (time_before(jiffies, stop));
> +
> + dev_err(&chip->dev, "Timeout reading burst and status\n");
> + return -ETIMEDOUT;
> +}
> +
> +/*
> + * cr50_i2c_tis_recv - TPM reception callback
> + *
> + * @chip: TPM chip information
> + * @buf: reception buffer
> + * @buf_len: buffer length to read
> + *
> + * Returns negative number for error, number of bytes read for success.
> + */
> +static int cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + int status, rc;
> + size_t burstcnt, cur, len, expected;
> + u8 addr = TPM_I2C_DATA_FIFO(priv->locality);
> + u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
> +
> + if (buf_len < TPM_HEADER_SIZE)
> + return -EINVAL;
> +
> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> +
> + if (burstcnt > buf_len || burstcnt < TPM_HEADER_SIZE) {
> + dev_err(&chip->dev,
> + "Unexpected burstcnt: %zu (max=%zu, min=%d)\n",
> + burstcnt, buf_len, TPM_HEADER_SIZE);
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + /* Read first chunk of burstcnt bytes */
> + rc = cr50_i2c_read(chip, addr, buf, burstcnt);
> + if (rc < 0) {
> + dev_err(&chip->dev, "Read of first chunk failed\n");
> + goto out_err;
> + }
> +
> + /* Determine expected data in the return buffer */
> + expected = be32_to_cpup((__be32 *)(buf + 2));
> + if (expected > buf_len) {
> + dev_err(&chip->dev, "Buffer too small to receive i2c data\n");
> + goto out_err;
> + }
> +
> + /* Now read the rest of the data */
> + cur = burstcnt;
> + while (cur < expected) {
> + /* Read updated burst count and check status */
> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> +
> + len = min_t(size_t, burstcnt, expected - cur);
> + rc = cr50_i2c_read(chip, addr, buf + cur, len);
> + if (rc < 0) {
> + dev_err(&chip->dev, "Read failed\n");
> + goto out_err;
> + }
> +
> + cur += len;
> + }
> +
> + /* Ensure TPM is done reading data */
> + rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> + if (status & TPM_STS_DATA_AVAIL) {
> + dev_err(&chip->dev, "Data still available\n");
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + cr50_release_locality(chip, CR50_NO_FORCE);
> + return cur;
> +
> +out_err:
> + /* Abort current transaction if still pending */
> + if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
> + cr50_i2c_tis_ready(chip);
> +
> + cr50_release_locality(chip, CR50_NO_FORCE);
> + return rc;
> +}
> +
> +/*
> + * cr50_i2c_tis_send - TPM emission callback
> + *
> + * @chip: TPM chip information
> + * @buf: buffer to send
> + * @len: buffer length
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + int rc, status;
> + size_t burstcnt, limit, sent = 0;
> + u8 tpm_go[4] = { TPM_STS_GO };
> + unsigned long stop;
> +
> + rc = request_locality(chip);
> + if (rc < 0)
> + return rc;
> +
> + /* Wait until TPM is ready for a command */
> + stop = jiffies + chip->timeout_b;
> + while (!(cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)) {
> + if (time_after(jiffies, stop)) {
> + rc = -ETIMEDOUT;
> + goto out_err;
> + }
> +
> + cr50_i2c_tis_ready(chip);
> + }
> +
> + while (len > 0) {
> + u8 mask = TPM_STS_VALID;
> +
> + /* Wait for data if this is not the first chunk */
> + if (sent > 0)
> + mask |= TPM_STS_DATA_EXPECT;
> +
> + /* Read burst count and check status */
> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> +
> + /*
> + * Use burstcnt - 1 to account for the address byte
> + * that is inserted by cr50_i2c_write()
> + */
> + limit = min_t(size_t, burstcnt - 1, len);
> + rc = cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(priv->locality),
> + &buf[sent], limit);
> + if (rc < 0) {
> + dev_err(&chip->dev, "Write failed\n");
> + goto out_err;
> + }
> +
> + sent += limit;
> + len -= limit;
> + }
> +
> + /* Ensure TPM is not expecting more data */
> + rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> + if (status & TPM_STS_DATA_EXPECT) {
> + dev_err(&chip->dev, "Data still expected\n");
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + /* Start the TPM command */
> + rc = cr50_i2c_write(chip, TPM_I2C_STS(priv->locality), tpm_go,
> + sizeof(tpm_go));
> + if (rc < 0) {
> + dev_err(&chip->dev, "Start command failed\n");
> + goto out_err;
> + }
> + return 0;
> +
> +out_err:
> + /* Abort current transaction if still pending */
> + if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
> + cr50_i2c_tis_ready(chip);
> +
> + cr50_release_locality(chip, CR50_NO_FORCE);
> + return rc;
> +}
> +
> +/*
> + * cr50_i2c_req_canceled - callback to notify a request cancel
> + *
> + * @chip: TPM chip information
> + * @status: status given by the cancel callback
> + *
> + * Return if command is ready or not
> + */
> +static bool cr50_i2c_req_canceled(struct tpm_chip *chip, u8 status)
> +{
> + return (status == TPM_STS_COMMAND_READY);
> +}
> +
> +static const struct tpm_class_ops cr50_i2c = {
> + .flags = TPM_OPS_AUTO_STARTUP,
> + .status = &cr50_i2c_tis_status,
> + .recv = &cr50_i2c_tis_recv,
> + .send = &cr50_i2c_tis_send,
> + .cancel = &cr50_i2c_tis_ready,
> + .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> + .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> + .req_canceled = &cr50_i2c_req_canceled,
> +};
> +
> +static const struct i2c_device_id cr50_i2c_table[] = {
> + {"cr50_i2c", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, cr50_i2c_table);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id cr50_i2c_acpi_id[] = {
> + { "GOOG0005", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, cr50_i2c_acpi_id);
> +#endif
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_cr50_i2c_match[] = {
> + { .compatible = "google,cr50", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_cr50_i2c_match);
> +#endif
> +
> +/*
> + * cr50_i2c_probe - driver prbe function
> + *
> + * @client: i2x client information
> + * @id: i2c device id
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct tpm_chip *chip;
> + struct priv_data *priv;
> + u8 buf[4];
> + u32 vendor;
> + int rc;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -ENODEV;
> +
> + chip = tpmm_chip_alloc(dev, &cr50_i2c);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + /* cr50 is a TPM 2.0 chip */
> + chip->flags |= TPM_CHIP_FLAG_TPM2;
> + chip->flags |= TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED;
> +
> + /* Default timeouts */
> + chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> + chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
> + chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> + chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> +
> + dev_set_drvdata(&chip->dev, priv);
> + init_completion(&priv->tpm_ready);
> +
> + if (client->irq > 0) {
> + rc = devm_request_irq(dev, client->irq, cr50_i2c_int_handler,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + dev->driver->name, chip);
> + if (rc < 0) {
> + dev_err(dev, "Failed to probe IRQ %d\n", client->irq);
> + return rc;
> + }
> +
> + disable_irq(client->irq);
> + priv->irq = client->irq;
> + } else {
> + dev_warn(dev, "No IRQ, will use %ums delay for TPM ready\n",
> + CR50_TIMEOUT_NOIRQ_MS);
> + }
> +
> + rc = request_locality(chip);
> + if (rc < 0) {
> + dev_err(dev, "Could not request locality\n");
> + return rc;
> + }
> +
> + /* Read four bytes from DID_VID register */
> + rc = cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf));
> + if (rc < 0) {
> + dev_err(dev, "Could not read vendor id\n");
> + cr50_release_locality(chip, CR50_FORCE);
> + return rc;
> + }
> +
> + vendor = le32_to_cpup((__le32 *)buf);
> + if (vendor != CR50_I2C_DID_VID) {
> + dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
> + cr50_release_locality(chip, CR50_FORCE);
> + return -ENODEV;
> + }
> +
> + dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n",
> + client->addr, client->irq, vendor >> 16);
> +
> + return tpm_chip_register(chip);
> +}
> +
> +/*
> + * cr50_i2c_probe - driver prbe function
> + *
> + * @client: i2x client information
> + *
> + * Returns 0
> + */
> +static int cr50_i2c_remove(struct i2c_client *client)
> +{
> + struct tpm_chip *chip = i2c_get_clientdata(client);
> +
> + tpm_chip_unregister(chip);
> + cr50_release_locality(chip, CR50_FORCE);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume);
> +
> +static struct i2c_driver cr50_i2c_driver = {
> + .id_table = cr50_i2c_table,
> + .probe = cr50_i2c_probe,
> + .remove = cr50_i2c_remove,
> + .driver = {
> + .name = "cr50_i2c",
> + .pm = &cr50_i2c_pm,
> + .acpi_match_table = ACPI_PTR(cr50_i2c_acpi_id),
> + .of_match_table = of_match_ptr(of_cr50_i2c_match),
> + },
> +};
> +
> +module_i2c_driver(cr50_i2c_driver);
> +
> +MODULE_DESCRIPTION("cr50 TPM I2C Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.29.2
>
>
I did not enumerate every possible style error but instead showed some
examples, because there were so many.
/Jarkko
On Tue, 24 Nov 2020, Jarkko Sakkinen <[email protected]> wrote:
> On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu wrote:
>> From: "[email protected]" <[email protected]> Add TPM
>> 2.0 compatible I2C interface for chips with cr50 firmware.
>> The firmware running on the currently supported H1 MCU requires
>> a special driver to handle its specific protocol, and this
>> makes it unsuitable to use tpm_tis_core_* and instead it must
>> implement the underlying TPM protocol similar to the other I2C
>> TPM drivers. - All 4 byes of status register must be
>> read/written at once. - FIFO and burst count is limited to 63
>> and must be drained by AP. - Provides an interrupt to indicate
>> when read response data is ready and when the TPM is finished
>> processing write data. This driver is based on the existing
>> infineon I2C TPM driver, which most closely matches the cr50
>> i2c protocol behavior. Cc: Helen Koike
>> <[email protected]> Signed-off-by: Duncan Laurie
>> <[email protected]> [[email protected]: Depend on i2c even
>> if it's a module, replace boilier plate with SPDX tag, drop
>> asm/byteorder.h include, simplify return from probe]
>> Signed-off-by: Stephen Boyd <[email protected]>
>> Signed-off-by: Fabien Lahoudere
>> <[email protected]> Signed-off-by: Adrian Ratiu
>> <[email protected]> --- Changes in v2:
>> - Various small fixes all over (reorder includes,
>> MAX_BUFSIZE, comments, etc) - Reworked return values of
>> i2c_wait_tpm_ready() to fix timeout mis-handling
>> so ret == 0 now means success, the wait period jiffies is
>> ignored because that number is meaningless and return a proper
>> timeout error in case jiffies == 0.
>> - Make i2c default to 1 message per transfer (requested by
>> Helen) - Move -EIO error reporting to transfer function to
>> cleanup transfer() itself
>> and its R/W callers
>> - Remove magic value hardcodings and introduce enum
>> force_release.
>> v1 posted at https://lkml.org/lkml/2020/2/25/349 Applies on
>> next-20201120, tested on Chromebook EVE. ---
>> drivers/char/tpm/Kconfig | 10 +
>> drivers/char/tpm/Makefile | 2 +
>> drivers/char/tpm/tpm_tis_i2c_cr50.c | 768
>> ++++++++++++++++++++++++++++ 3 files changed, 780
>> insertions(+) create mode 100644
>> drivers/char/tpm/tpm_tis_i2c_cr50.c
>> diff --git a/drivers/char/tpm/Kconfig
>> b/drivers/char/tpm/Kconfig index a18c314da211..4308f9ca7a43
>> 100644 --- a/drivers/char/tpm/Kconfig +++
>> b/drivers/char/tpm/Kconfig @@ -86,6 +86,16 @@ config
>> TCG_TIS_SYNQUACER
>> To compile this driver as a module, choose M here; the
>> module will be called tpm_tis_synquacer.
>> +config TCG_TIS_I2C_CR50 + tristate "TPM Interface
>> Specification 2.0 Interface (I2C - CR50)" + depends on I2C +
>> select TCG_CR50 + help + This is a driver for the Google
>> cr50 I2C TPM interface which is a + custom microcontroller
>> and requires a custom i2c protocol interface + to
>> handle the limitations of the hardware. To compile this driver
>> + as a module, choose M here; the module will be called
>> tcg_tis_i2c_cr50. +
>> config TCG_TIS_I2C_ATMEL tristate "TPM Interface Specification
>> 1.2 Interface (I2C - Atmel)" depends on I2C
>> diff --git a/drivers/char/tpm/Makefile
>> b/drivers/char/tpm/Makefile index 84db4fb3a9c9..66d39ea6bd10
>> 100644 --- a/drivers/char/tpm/Makefile +++
>> b/drivers/char/tpm/Makefile @@ -27,6 +27,8 @@
>> obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
>> tpm_tis_spi-y := tpm_tis_spi_main.o
>> tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
>> +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o +
>> obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
>> obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
>> obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
>> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c
>> b/drivers/char/tpm/tpm_tis_i2c_cr50.c new file mode 100644
>> index 000000000000..37555dafdca0 --- /dev/null +++
>> b/drivers/char/tpm/tpm_tis_i2c_cr50.c @@ -0,0 +1,768 @@ +//
>> SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2016 Google
>> Inc. + * + * Based on Linux Kernel TPM driver by + * Peter
>> Huewe <[email protected]> + * Copyright (C) 2011
>> Infineon Technologies + */ + +/* + * cr50 is a firmware for H1
>> secure modules that requires special + * handling for the I2C
>> interface. + * + * - Use an interrupt for transaction status
>> instead of hardcoded delays + * - Must use write+wait+read read
>> protocol + * - All 4 bytes of status register must be
>> read/written at once + * - Burst count max is 63 bytes, and
>> burst count behaves + * slightly differently than other I2C
>> TPMs + * - When reading from FIFO the full burstcnt must be
>> read + * instead of just reading header and determining the
>> remainder + */ + +#include <linux/acpi.h> +#include
>> <linux/completion.h> +#include <linux/i2c.h> +#include
>> <linux/interrupt.h> +#include <linux/module.h> +#include
>> <linux/pm.h> +#include <linux/slab.h> +#include <linux/wait.h>
>> + +#include "tpm_tis_core.h" + +#define CR50_MAX_BUFSIZE 64
>> +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout
>> during transactions */ +#define CR50_TIMEOUT_NOIRQ_MS 20
>> /* Timeout for TPM ready without IRQ */ +#define
>> CR50_I2C_DID_VID 0x00281ae0L +#define CR50_I2C_MAX_RETRIES
>> 3 /* Max retries due to I2C errors */ +#define
>> CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between
>> retries on I2C */ +#define CR50_I2C_RETRY_DELAY_HI 65 /*
>> Max usecs between retries on I2C */
>
> CR50_ -> TPM_CR50_
>
>> + +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4)) +#define
>> TPM_I2C_STS(l) (0x0001 | ((l) << 4)) +#define
>> TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4)) +#define
>> TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4)) + +struct priv_data
>> { + int irq; + int locality; + struct completion
>> tpm_ready; + u8 buf[CR50_MAX_BUFSIZE]; +};
>
> tpm_i2c_cr50_priv_data
>
>> + +enum force_release { + CR50_NO_FORCE = 0x0, + CR50_FORCE
>> = 0x1, +};
>
> I'd just
>
> #define TPM_I2C_CR50_NO_FORCE 0 #define TPM_I2C_CR50_FORCE 1
>
>> + +/* + * The cr50 interrupt handler just signals waiting
>> threads that the + * interrupt was asserted. It does not do
>> any processing triggered + * by interrupts but is instead used
>> to avoid fixed delays. + * + * @dummy: unuesed parameter + *
>> @dev_id: TPM chip information + */
>
> Please review for kdoc:
>
> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>
>> +static irqreturn_t cr50_i2c_int_handler(int dummy, void
>> *dev_id)
>
> cr50 -> tpm_i2c_cr50_
>
>> +{ + struct tpm_chip *chip = dev_id; + struct priv_data
>> *priv = dev_get_drvdata(&chip->dev); + +
>> complete(&priv->tpm_ready); + + return IRQ_HANDLED; +} +
>> +/* + * Wait for completion interrupt if available, otherwise
>> use a fixed + * delay for the TPM to be ready. + * + * @chip:
>> TPM chip information + * + * Returns 0 for success, negative
>> number for timeout + */ +static int
>> cr50_i2c_wait_tpm_ready(struct tpm_chip *chip)
>
> Ditto.
>
>> +{ + struct priv_data *priv = dev_get_drvdata(&chip->dev); + +
>> /* Use a safe fixed delay if interrupt is not supported */ + if
>> (priv->irq <= 0) { + msleep(CR50_TIMEOUT_NOIRQ_MS); +
>> return 0; + } + + /* Wait for interrupt to indicate TPM is
>> ready to respond */ + if
>> (!wait_for_completion_timeout(&priv->tpm_ready, +
>> msecs_to_jiffies(chip->timeout_a))) { +
>> dev_warn(&chip->dev, "Timeout waiting for TPM ready\n"); +
>> return -ETIMEDOUT; + } + + return 0; +} + +/* + *
>> cr50_i2c_enable_tpm_irq - enable TPM irq + * + * @chip: TPM
>> chip information + */ +static void
>> cr50_i2c_enable_tpm_irq(struct tpm_chip *chip) +{ + struct
>> priv_data *priv = dev_get_drvdata(&chip->dev); + + if
>> (priv->irq > 0) { +
>> reinit_completion(&priv->tpm_ready); +
>> enable_irq(priv->irq); + } +} + +/* + *
>> cr50_i2c_disable_tpm_irq - disable TPM irq + * + * @chip: TPM
>> chip information + */ +static void
>> cr50_i2c_disable_tpm_irq(struct tpm_chip *chip) +{ + struct
>> priv_data *priv = dev_get_drvdata(&chip->dev); + + if
>> (priv->irq > 0) + disable_irq(priv->irq); +} + +/* +
>> * cr50_i2c_transfer_message - transfer a message over i2c + * +
>> * @dev: device information + * @adapter: i2c adapter + * @msg:
>> message to transfer + * + * Call unlocked i2c transfer routine
>> with the provided parameters and retry + * in case of bus
>> errors. Returns 0 on success, otherwise negative errno. + */
>> +static int cr50_i2c_transfer_message(struct device *dev, +
>> struct i2c_adapter *adapter, +
>> struct i2c_msg *msg) +{ + int rc = 0; + unsigned int try;
>> + + for (try = 0; try < CR50_I2C_MAX_RETRIES; try++) { +
>> rc = __i2c_transfer(adapter, msg, 1); + if (rc ==
>> 1) + return 0; /* Successfully transferred the
>> message */ + if (try) +
>> dev_warn(dev, "i2c transfer failed (attempt %d/%d): %d\n", +
>> try + 1, CR50_I2C_MAX_RETRIES, rc); +
>> usleep_range(CR50_I2C_RETRY_DELAY_LO, CR50_I2C_RETRY_DELAY_HI);
>> + } + + return -EIO; /* No i2c message transferred */ +} +
>> +/* + * cr50_i2c_read() - read from TPM register + *
>
> Not only wrong formatting but seems that these kdoc's have
> differing formatting (no newline).
>
>> + * @chip: TPM chip information + * @addr: register address to
>> read from + * @buffer: provided by caller + * @len: number of
>> bytes to read + * + * 1) send register address byte 'addr' to
>> the TPM + * 2) wait for TPM to indicate it is ready + * 3) read
>> 'len' bytes of TPM response into the provided 'buffer'
>
> Please write proper sentences instead of lower-case (also
> applies to parameters as you can see from kernel documentation).
>
>> + * + * Returns negative number for error, 0 for success. + */
>> +static int cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8
>> *buffer, size_t len) +{ + struct i2c_client *client =
>> to_i2c_client(chip->dev.parent); + struct i2c_msg msg1 = { +
>> .addr = client->addr, + .len = 1, +
>> .buf = &addr + }; + struct i2c_msg msg2 = { +
>> .addr = client->addr, + .flags = I2C_M_RD, +
>> .len = len, + .buf = buffer + }; + int rc; +
>> + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); + + /*
>> Prepare for completion interrupt */ +
>> cr50_i2c_enable_tpm_irq(chip); + + /* Send the register
>> address byte to the TPM */ + rc =
>> cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
>> + if (rc < 0) + goto out; + + /* Wait for TPM to
>> be ready with response data */ + rc =
>> cr50_i2c_wait_tpm_ready(chip); + if (rc < 0) +
>> goto out; + + /* Read response data from the TPM */ + rc
>> = cr50_i2c_transfer_message(&chip->dev, client->adapter,
>> &msg2); + +out: + cr50_i2c_disable_tpm_irq(chip); +
>> i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); + + if
>> (rc < 0) + return rc; + + return 0; +} + +/* + *
>> cr50_i2c_write() - write to TPM register + * + * @chip: TPM
>> chip information + * @addr: register address to write to + *
>> @buffer: data to write + * @len: number of bytes to write + * +
>> * 1) prepend the provided address to the provided data + * 2)
>> send the address+data to the TPM + * 3) wait for TPM to
>> indicate it is done writing + * + * Returns negative number for
>> error, 0 for success. + */ +static int cr50_i2c_write(struct
>> tpm_chip *chip, u8 addr, u8 *buffer, +
>> size_t len) +{ + struct priv_data *priv =
>> dev_get_drvdata(&chip->dev); + struct i2c_client *client
>> = to_i2c_client(chip->dev.parent); + struct i2c_msg msg1 = { +
>> .addr = client->addr, + .len = len + 1, +
>> .buf = priv->buf + }; + int rc; + + if (len >
>> CR50_MAX_BUFSIZE - 1) + return -EINVAL; + +
>> i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); + + /* Prepend
>> the 'register address' to the buffer */ + priv->buf[0] =
>> addr; + memcpy(priv->buf + 1, buffer, len); + + /* Prepare
>> for completion interrupt */ +
>> cr50_i2c_enable_tpm_irq(chip); + + /* Send write request
>> buffer with address */ + rc =
>> cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1);
>> + if (rc < 0) + goto out; + + /* Wait for TPM to
>> be ready, ignore timeout */ +
>> cr50_i2c_wait_tpm_ready(chip); + +out: +
>> cr50_i2c_disable_tpm_irq(chip); +
>> i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); + + if
>> (rc < 0) + return rc; + + return 0; +} + +/* + *
>> cr50_check_locality - verify TPM locality + * + * @chip: TPM
>> chip information + * + * Returns negative number for error, 0
>> for success. + */ +static int cr50_check_locality(struct
>> tpm_chip *chip) +{ + u8 mask = TPM_ACCESS_VALID |
>> TPM_ACCESS_ACTIVE_LOCALITY; + u8 buf; + int rc; +
>> + rc = cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf,
>> sizeof(buf)); + if (rc < 0) + return rc; + + if
>> ((buf & mask) == mask) + return 0; + + return
>> -EIO; +} + +/* + * cr50_release_locality - release TPM locality
>> + * + * @chip: TPM chip information + * @force: flag to force
>> release if set + */ +static void cr50_release_locality(struct
>> tpm_chip *chip, + enum
>> force_release force) +{ + struct priv_data *priv =
>> dev_get_drvdata(&chip->dev); + u8 mask = TPM_ACCESS_VALID
>> | TPM_ACCESS_REQUEST_PENDING; + u8 addr =
>> TPM_I2C_ACCESS(priv->locality); + u8 buf; + + if
>> (cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0) +
>> return; + + if (force || (buf & mask) == mask) { +
>> buf = TPM_ACCESS_ACTIVE_LOCALITY; +
>> cr50_i2c_write(chip, addr, &buf, sizeof(buf)); + } + +
>> priv->locality = 0; +} + +/* + * request_locality - request TPM
>> locality + * + * @chip: TPM chip information + */ +static int
>> request_locality(struct tpm_chip *chip) +{ + struct priv_data
>> *priv = dev_get_drvdata(&chip->dev); + u8 buf =
>> TPM_ACCESS_REQUEST_USE; + unsigned long stop; + int rc; +
>> + if (!cr50_check_locality(chip)) + return 0;
>> + + rc = cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf,
>> sizeof(buf)); + if (rc < 0) + return rc; + +
>> stop = jiffies + chip->timeout_a; + do { + if
>> (!cr50_check_locality(chip)) { +
>> priv->locality = 0; + return 0; +
>> } + msleep(CR50_TIMEOUT_SHORT_MS); + } while
>> (time_before(jiffies, stop)); + + return -ETIMEDOUT; +} +
>> +/* + * cr50 requires all 4 bytes of status register to be read
>> + * + * @chip: TPM chip information + * + * Returns TPM status
>> + */ +static u8 cr50_i2c_tis_status(struct tpm_chip *chip) +{ +
>> struct priv_data *priv = dev_get_drvdata(&chip->dev); + u8
>> buf[4]; + + if (cr50_i2c_read(chip,
>> TPM_I2C_STS(priv->locality), + buf,
>> sizeof(buf)) < 0) + return 0; + return buf[0]; +}
>> + +/* + * cr50 requires all 4 bytes of status register to be
>> written + * + * @chip: TPM chip information + */ +static void
>> cr50_i2c_tis_ready(struct tpm_chip *chip) +{ + struct
>> priv_data *priv = dev_get_drvdata(&chip->dev); + u8 buf[4]
>> = { TPM_STS_COMMAND_READY }; + + cr50_i2c_write(chip,
>> TPM_I2C_STS(priv->locality), buf, sizeof(buf)); +
>> msleep(CR50_TIMEOUT_SHORT_MS); +} + +/* + * cr50 uses bytes 3:2
>> of status register for burst count and + * all 4 bytes must be
>> read + * + * @chip: TPM chip information + * @mask: status mask
>> + * @burst: return value for burst + * @status: return value
>> for statis + * + * Returns negative number for error, 0 for
>> success. + */ +static int cr50_i2c_wait_burststs(struct
>> tpm_chip *chip, u8 mask, + size_t
>> *burst, int *status) +{ + struct priv_data *priv =
>> dev_get_drvdata(&chip->dev); + unsigned long stop; + u8
>> buf[4]; + + /* wait for burstcount */ + stop = jiffies +
>> chip->timeout_b; + do { + if (cr50_i2c_read(chip,
>> TPM_I2C_STS(priv->locality), +
>> buf, sizeof(buf)) < 0) { +
>> msleep(CR50_TIMEOUT_SHORT_MS); + continue;
>> + } + + *status = *buf; +
>> *burst = le16_to_cpup((__le16 *)(buf + 1)); + + if
>> ((*status & mask) == mask && + *burst > 0 &&
>> *burst <= CR50_MAX_BUFSIZE - 1) + return 0;
>> + + msleep(CR50_TIMEOUT_SHORT_MS); + } while
>> (time_before(jiffies, stop)); + + dev_err(&chip->dev,
>> "Timeout reading burst and status\n"); + return -ETIMEDOUT;
>> +} + +/* + * cr50_i2c_tis_recv - TPM reception callback + * + *
>> @chip: TPM chip information + * @buf: reception buffer + *
>> @buf_len: buffer length to read + * + * Returns negative number
>> for error, number of bytes read for success. + */ +static int
>> cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t
>> buf_len) +{ + struct priv_data *priv =
>> dev_get_drvdata(&chip->dev); + int status, rc; +
>> size_t burstcnt, cur, len, expected; + u8 addr =
>> TPM_I2C_DATA_FIFO(priv->locality); + u8 mask = TPM_STS_VALID |
>> TPM_STS_DATA_AVAIL; + + if (buf_len < TPM_HEADER_SIZE) +
>> return -EINVAL; + + rc = cr50_i2c_wait_burststs(chip, mask,
>> &burstcnt, &status); + if (rc < 0) + goto
>> out_err; + + if (burstcnt > buf_len || burstcnt <
>> TPM_HEADER_SIZE) { + dev_err(&chip->dev, +
>> "Unexpected burstcnt: %zu (max=%zu, min=%d)\n", +
>> burstcnt, buf_len, TPM_HEADER_SIZE); + rc = -EIO;
>> + goto out_err; + } + + /* Read first chunk of
>> burstcnt bytes */ + rc = cr50_i2c_read(chip, addr, buf,
>> burstcnt); + if (rc < 0) { + dev_err(&chip->dev, "Read
>> of first chunk failed\n"); + goto out_err; + } + + /*
>> Determine expected data in the return buffer */ + expected =
>> be32_to_cpup((__be32 *)(buf + 2)); + if (expected > buf_len) {
>> + dev_err(&chip->dev, "Buffer too small to receive
>> i2c data\n"); + goto out_err; + } + + /* Now
>> read the rest of the data */ + cur = burstcnt; +
>> while (cur < expected) { + /* Read updated burst
>> count and check status */ + rc =
>> cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status); +
>> if (rc < 0) + goto out_err; + +
>> len = min_t(size_t, burstcnt, expected - cur); + rc
>> = cr50_i2c_read(chip, addr, buf + cur, len); + if
>> (rc < 0) { + dev_err(&chip->dev, "Read
>> failed\n"); + goto out_err; + }
>> + + cur += len; + } + + /* Ensure TPM is done
>> reading data */ + rc = cr50_i2c_wait_burststs(chip,
>> TPM_STS_VALID, &burstcnt, &status); + if (rc < 0) +
>> goto out_err; + if (status & TPM_STS_DATA_AVAIL) { +
>> dev_err(&chip->dev, "Data still available\n"); + rc
>> = -EIO; + goto out_err; + } + +
>> cr50_release_locality(chip, CR50_NO_FORCE); + return
>> cur; + +out_err: + /* Abort current transaction if still
>> pending */ + if (cr50_i2c_tis_status(chip) &
>> TPM_STS_COMMAND_READY) + cr50_i2c_tis_ready(chip);
>> + + cr50_release_locality(chip, CR50_NO_FORCE); + return rc;
>> +} + +/* + * cr50_i2c_tis_send - TPM emission callback + * + *
>> @chip: TPM chip information + * @buf: buffer to send + * @len:
>> buffer length + * + * Returns negative number for error, 0 for
>> success. + */ +static int cr50_i2c_tis_send(struct tpm_chip
>> *chip, u8 *buf, size_t len) +{ + struct priv_data *priv =
>> dev_get_drvdata(&chip->dev); + int rc, status; +
>> size_t burstcnt, limit, sent = 0; + u8 tpm_go[4] = {
>> TPM_STS_GO }; + unsigned long stop; + + rc =
>> request_locality(chip); + if (rc < 0) + return rc;
>> + + /* Wait until TPM is ready for a command */ + stop =
>> jiffies + chip->timeout_b; + while (!(cr50_i2c_tis_status(chip)
>> & TPM_STS_COMMAND_READY)) { + if
>> (time_after(jiffies, stop)) { + rc =
>> -ETIMEDOUT; + goto out_err; + }
>> + + cr50_i2c_tis_ready(chip); + } + + while (len
>> > 0) { + u8 mask = TPM_STS_VALID; + + /*
>> Wait for data if this is not the first chunk */ + if
>> (sent > 0) + mask |= TPM_STS_DATA_EXPECT; + +
>> /* Read burst count and check status */ + rc =
>> cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status); +
>> if (rc < 0) + goto out_err; + +
>> /* + * Use burstcnt - 1 to account for the address
>> byte + * that is inserted by cr50_i2c_write() +
>> */ + limit = min_t(size_t, burstcnt - 1, len); +
>> rc = cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(priv->locality), +
>> &buf[sent], limit); + if (rc < 0) { +
>> dev_err(&chip->dev, "Write failed\n"); +
>> goto out_err; + } + + sent += limit; +
>> len -= limit; + } + + /* Ensure TPM is not expecting
>> more data */ + rc = cr50_i2c_wait_burststs(chip,
>> TPM_STS_VALID, &burstcnt, &status); + if (rc < 0) +
>> goto out_err; + if (status & TPM_STS_DATA_EXPECT) { +
>> dev_err(&chip->dev, "Data still expected\n"); + rc
>> = -EIO; + goto out_err; + } + + /* Start the TPM
>> command */ + rc = cr50_i2c_write(chip,
>> TPM_I2C_STS(priv->locality), tpm_go, +
>> sizeof(tpm_go)); + if (rc < 0) { +
>> dev_err(&chip->dev, "Start command failed\n"); +
>> goto out_err; + } + return 0; + +out_err: + /* Abort
>> current transaction if still pending */ + if
>> (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY) +
>> cr50_i2c_tis_ready(chip); + +
>> cr50_release_locality(chip, CR50_NO_FORCE); + return rc;
>> +} + +/* + * cr50_i2c_req_canceled - callback to notify a
>> request cancel + * + * @chip: TPM chip information + * @status:
>> status given by the cancel callback + * + * Return if command
>> is ready or not + */ +static bool cr50_i2c_req_canceled(struct
>> tpm_chip *chip, u8 status) +{ + return (status ==
>> TPM_STS_COMMAND_READY); +} + +static const struct tpm_class_ops
>> cr50_i2c = { + .flags = TPM_OPS_AUTO_STARTUP, +
>> .status = &cr50_i2c_tis_status, + .recv =
>> &cr50_i2c_tis_recv, + .send = &cr50_i2c_tis_send, +
>> .cancel = &cr50_i2c_tis_ready, + .req_complete_mask =
>> TPM_STS_DATA_AVAIL | TPM_STS_VALID, + .req_complete_val
>> = TPM_STS_DATA_AVAIL | TPM_STS_VALID, + .req_canceled =
>> &cr50_i2c_req_canceled, +}; + +static const struct
>> i2c_device_id cr50_i2c_table[] = { + {"cr50_i2c", 0}, + {}
>> +}; +MODULE_DEVICE_TABLE(i2c, cr50_i2c_table); + +#ifdef
>> CONFIG_ACPI +static const struct acpi_device_id
>> cr50_i2c_acpi_id[] = { + { "GOOG0005", 0 }, + {} +};
>> +MODULE_DEVICE_TABLE(acpi, cr50_i2c_acpi_id); +#endif + +#ifdef
>> CONFIG_OF +static const struct of_device_id of_cr50_i2c_match[]
>> = { + { .compatible = "google,cr50", }, + {} +};
>> +MODULE_DEVICE_TABLE(of, of_cr50_i2c_match); +#endif + +/* + *
>> cr50_i2c_probe - driver prbe function + * + * @client: i2x
>> client information + * @id: i2c device id + * + * Returns
>> negative number for error, 0 for success. + */ +static int
>> cr50_i2c_probe(struct i2c_client *client, +
>> const struct i2c_device_id *id) +{ + struct device *dev =
>> &client->dev; + struct tpm_chip *chip; + struct
>> priv_data *priv; + u8 buf[4]; + u32 vendor; + int rc; +
>> + if (!i2c_check_functionality(client->adapter,
>> I2C_FUNC_I2C)) + return -ENODEV; + + chip =
>> tpmm_chip_alloc(dev, &cr50_i2c); + if (IS_ERR(chip)) +
>> return PTR_ERR(chip); + + priv = devm_kzalloc(dev,
>> sizeof(*priv), GFP_KERNEL); + if (!priv) +
>> return -ENOMEM; + + /* cr50 is a TPM 2.0 chip */ +
>> chip->flags |= TPM_CHIP_FLAG_TPM2; + chip->flags |=
>> TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED; + + /* Default
>> timeouts */ + chip->timeout_a =
>> msecs_to_jiffies(TIS_SHORT_TIMEOUT); + chip->timeout_b =
>> msecs_to_jiffies(TIS_LONG_TIMEOUT); + chip->timeout_c =
>> msecs_to_jiffies(TIS_SHORT_TIMEOUT); + chip->timeout_d =
>> msecs_to_jiffies(TIS_SHORT_TIMEOUT); + +
>> dev_set_drvdata(&chip->dev, priv); +
>> init_completion(&priv->tpm_ready); + + if (client->irq >
>> 0) { + rc = devm_request_irq(dev, client->irq,
>> cr50_i2c_int_handler, +
>> IRQF_TRIGGER_FALLING | IRQF_ONESHOT, +
>> dev->driver->name, chip); + if (rc < 0) { +
>> dev_err(dev, "Failed to probe IRQ %d\n", client->irq); +
>> return rc; + } + + disable_irq(client->irq);
>> + priv->irq = client->irq; + } else { +
>> dev_warn(dev, "No IRQ, will use %ums delay for TPM ready\n", +
>> CR50_TIMEOUT_NOIRQ_MS); + } + + rc =
>> request_locality(chip); + if (rc < 0) { +
>> dev_err(dev, "Could not request locality\n"); +
>> return rc; + } + + /* Read four bytes from DID_VID register
>> */ + rc = cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf,
>> sizeof(buf)); + if (rc < 0) { + dev_err(dev,
>> "Could not read vendor id\n"); +
>> cr50_release_locality(chip, CR50_FORCE); + return rc;
>> + } + + vendor = le32_to_cpup((__le32 *)buf); + if (vendor
>> != CR50_I2C_DID_VID) { + dev_err(dev, "Vendor ID
>> did not match! ID was %08x\n", vendor); +
>> cr50_release_locality(chip, CR50_FORCE); + return
>> -ENODEV; + } + + dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x
>> irq %d id 0x%x)\n", + client->addr,
>> client->irq, vendor >> 16); + + return
>> tpm_chip_register(chip); +} + +/* + * cr50_i2c_probe - driver
>> prbe function + * + * @client: i2x client information + * + *
>> Returns 0 + */ +static int cr50_i2c_remove(struct i2c_client
>> *client) +{ + struct tpm_chip *chip =
>> i2c_get_clientdata(client); + + tpm_chip_unregister(chip);
>> + cr50_release_locality(chip, CR50_FORCE); + + return 0;
>> +} + +static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend,
>> tpm_pm_resume); + +static struct i2c_driver cr50_i2c_driver = {
>> + .id_table = cr50_i2c_table, + .probe = cr50_i2c_probe, +
>> .remove = cr50_i2c_remove, + .driver = { + .name =
>> "cr50_i2c", + .pm = &cr50_i2c_pm, +
>> .acpi_match_table = ACPI_PTR(cr50_i2c_acpi_id), +
>> .of_match_table = of_match_ptr(of_cr50_i2c_match), + }, +}; +
>> +module_i2c_driver(cr50_i2c_driver); +
>> +MODULE_DESCRIPTION("cr50 TPM I2C Driver");
>> +MODULE_LICENSE("GPL"); -- 2.29.2
>
> I did not enumerate every possible style error but instead
> showed some examples, because there were so many.
Hi Jarkko,
I'll address the style issues in v3. Thank you for the feedback.
Adrian
>
> /Jarkko
Hi Jarkko,
Thanks for your review.
On Tue, 2020-11-24 at 00:06 +0200, Jarkko Sakkinen wrote:
> On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu wrote:
> > From: "[email protected]" <[email protected]>
> >
> > Add TPM 2.0 compatible I2C interface for chips with cr50 firmware.
> >
> > The firmware running on the currently supported H1 MCU requires a
> > special driver to handle its specific protocol, and this makes it
> > unsuitable to use tpm_tis_core_* and instead it must implement the
> > underlying TPM protocol similar to the other I2C TPM drivers.
> >
> > - All 4 byes of status register must be read/written at once.
> > - FIFO and burst count is limited to 63 and must be drained by AP.
> > - Provides an interrupt to indicate when read response data is ready
> > and when the TPM is finished processing write data.
> >
> > This driver is based on the existing infineon I2C TPM driver, which
> > most closely matches the cr50 i2c protocol behavior.
> >
> > Cc: Helen Koike <[email protected]>
> > Signed-off-by: Duncan Laurie <[email protected]>
> > [[email protected]: Depend on i2c even if it's a module, replace
> > boilier plate with SPDX tag, drop asm/byteorder.h include, simplify
> > return from probe]
> > Signed-off-by: Stephen Boyd <[email protected]>
> > Signed-off-by: Fabien Lahoudere <[email protected]>
> > Signed-off-by: Adrian Ratiu <[email protected]>
> > ---
> > Changes in v2:
> > - Various small fixes all over (reorder includes, MAX_BUFSIZE, comments, etc)
> > - Reworked return values of i2c_wait_tpm_ready() to fix timeout mis-handling
> > so ret == 0 now means success, the wait period jiffies is ignored because that
> > number is meaningless and return a proper timeout error in case jiffies == 0.
> > - Make i2c default to 1 message per transfer (requested by Helen)
> > - Move -EIO error reporting to transfer function to cleanup transfer() itself
> > and its R/W callers
> > - Remove magic value hardcodings and introduce enum force_release.
> >
> > v1 posted at https://lkml.org/lkml/2020/2/25/349
> >
> > Applies on next-20201120, tested on Chromebook EVE.
> > ---
> > drivers/char/tpm/Kconfig | 10 +
> > drivers/char/tpm/Makefile | 2 +
> > drivers/char/tpm/tpm_tis_i2c_cr50.c | 768 ++++++++++++++++++++++++++++
> > 3 files changed, 780 insertions(+)
> > create mode 100644 drivers/char/tpm/tpm_tis_i2c_cr50.c
> >
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index a18c314da211..4308f9ca7a43 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
> > To compile this driver as a module, choose M here;
> > the module will be called tpm_tis_synquacer.
> >
> > +config TCG_TIS_I2C_CR50
> > + tristate "TPM Interface Specification 2.0 Interface (I2C - CR50)"
> > + depends on I2C
> > + select TCG_CR50
> > + help
> > + This is a driver for the Google cr50 I2C TPM interface which is a
> > + custom microcontroller and requires a custom i2c protocol interface
> > + to handle the limitations of the hardware. To compile this driver
> > + as a module, choose M here; the module will be called tcg_tis_i2c_cr50.
> > +
> > config TCG_TIS_I2C_ATMEL
> > tristate "TPM Interface Specification 1.2 Interface (I2C - Atmel)"
> > depends on I2C
> > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > index 84db4fb3a9c9..66d39ea6bd10 100644
> > --- a/drivers/char/tpm/Makefile
> > +++ b/drivers/char/tpm/Makefile
> > @@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> > tpm_tis_spi-y := tpm_tis_spi_main.o
> > tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
> >
> > +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
> > +
> > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
> > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
> > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > new file mode 100644
> > index 000000000000..37555dafdca0
> > --- /dev/null
> > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > @@ -0,0 +1,768 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2016 Google Inc.
> > + *
> > + * Based on Linux Kernel TPM driver by
> > + * Peter Huewe <[email protected]>
> > + * Copyright (C) 2011 Infineon Technologies
> > + */
> > +
> > +/*
> > + * cr50 is a firmware for H1 secure modules that requires special
> > + * handling for the I2C interface.
> > + *
> > + * - Use an interrupt for transaction status instead of hardcoded delays
> > + * - Must use write+wait+read read protocol
> > + * - All 4 bytes of status register must be read/written at once
> > + * - Burst count max is 63 bytes, and burst count behaves
> > + * slightly differently than other I2C TPMs
> > + * - When reading from FIFO the full burstcnt must be read
> > + * instead of just reading header and determining the remainder
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/completion.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/pm.h>
> > +#include <linux/slab.h>
> > +#include <linux/wait.h>
> > +
> > +#include "tpm_tis_core.h"
> > +
> > +#define CR50_MAX_BUFSIZE 64
> > +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during transactions */
> > +#define CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready without IRQ */
> > +#define CR50_I2C_DID_VID 0x00281ae0L
> > +#define CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */
> > +#define CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries on I2C */
> > +#define CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries on I2C */
>
> CR50_ -> TPM_CR50_
>
> > +
> > +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
> > +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
> > +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
> > +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4))
> > +
> > +struct priv_data {
> > + int irq;
> > + int locality;
> > + struct completion tpm_ready;
> > + u8 buf[CR50_MAX_BUFSIZE];
> > +};
>
> tpm_i2c_cr50_priv_data
>
> > +
> > +enum force_release {
> > + CR50_NO_FORCE = 0x0,
> > + CR50_FORCE = 0x1,
> > +};
>
> I'd just
>
> #define TPM_I2C_CR50_NO_FORCE 0
> #define TPM_I2C_CR50_FORCE 1
>
A proper enumerated type has advantages over a preprocessor macro:
even if the compiler won't warn you, static analyzers can warn
about a misuse.
Also, it allows for a more obvious prototype.
I am curious why do you propose this change?
Thanks,
Ezequiel
On Tue, 2020-11-24 at 10:14 -0300, Ezequiel Garcia wrote:
> Hi Jarkko,
>
> Thanks for your review.
>
> On Tue, 2020-11-24 at 00:06 +0200, Jarkko Sakkinen wrote:
> > On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu wrote:
> > > From: "[email protected]" <[email protected]>
> > >
> > > Add TPM 2.0 compatible I2C interface for chips with cr50
> > > firmware.
> > >
> > > The firmware running on the currently supported H1 MCU requires a
> > > special driver to handle its specific protocol, and this makes it
> > > unsuitable to use tpm_tis_core_* and instead it must implement
> > > the
> > > underlying TPM protocol similar to the other I2C TPM drivers.
> > >
> > > - All 4 byes of status register must be read/written at once.
> > > - FIFO and burst count is limited to 63 and must be drained by
> > > AP.
> > > - Provides an interrupt to indicate when read response data is
> > > ready
> > > and when the TPM is finished processing write data.
> > >
> > > This driver is based on the existing infineon I2C TPM driver,
> > > which
> > > most closely matches the cr50 i2c protocol behavior.
> > >
> > > Cc: Helen Koike <[email protected]>
> > > Signed-off-by: Duncan Laurie <[email protected]>
> > > [[email protected]: Depend on i2c even if it's a module,
> > > replace
> > > boilier plate with SPDX tag, drop asm/byteorder.h include,
> > > simplify
> > > return from probe]
> > > Signed-off-by: Stephen Boyd <[email protected]>
> > > Signed-off-by: Fabien Lahoudere <[email protected]>
> > > Signed-off-by: Adrian Ratiu <[email protected]>
> > > ---
> > > Changes in v2:
> > > - Various small fixes all over (reorder includes, MAX_BUFSIZE,
> > > comments, etc)
> > > - Reworked return values of i2c_wait_tpm_ready() to fix timeout
> > > mis-handling
> > > so ret == 0 now means success, the wait period jiffies is ignored
> > > because that
> > > number is meaningless and return a proper timeout error in case
> > > jiffies == 0.
> > > - Make i2c default to 1 message per transfer (requested by
> > > Helen)
> > > - Move -EIO error reporting to transfer function to cleanup
> > > transfer() itself
> > > and its R/W callers
> > > - Remove magic value hardcodings and introduce enum
> > > force_release.
> > >
> > > v1 posted at https://lkml.org/lkml/2020/2/25/349
> > >
> > > Applies on next-20201120, tested on Chromebook EVE.
> > > ---
> > > drivers/char/tpm/Kconfig | 10 +
> > > drivers/char/tpm/Makefile | 2 +
> > > drivers/char/tpm/tpm_tis_i2c_cr50.c | 768
> > > ++++++++++++++++++++++++++++
> > > 3 files changed, 780 insertions(+)
> > > create mode 100644 drivers/char/tpm/tpm_tis_i2c_cr50.c
> > >
> > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > index a18c314da211..4308f9ca7a43 100644
> > > --- a/drivers/char/tpm/Kconfig
> > > +++ b/drivers/char/tpm/Kconfig
> > > @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
> > > To compile this driver as a module, choose M here;
> > > the module will be called tpm_tis_synquacer.
> > >
> > > +config TCG_TIS_I2C_CR50
> > > + tristate "TPM Interface Specification 2.0 Interface (I2C
> > > - CR50)"
> > > + depends on I2C
> > > + select TCG_CR50
> > > + help
> > > + This is a driver for the Google cr50 I2C TPM interface
> > > which is a
> > > + custom microcontroller and requires a custom i2c
> > > protocol interface
> > > + to handle the limitations of the hardware. To compile
> > > this driver
> > > + as a module, choose M here; the module will be called
> > > tcg_tis_i2c_cr50.
> > > +
> > > config TCG_TIS_I2C_ATMEL
> > > tristate "TPM Interface Specification 1.2 Interface (I2C
> > > - Atmel)"
> > > depends on I2C
> > > diff --git a/drivers/char/tpm/Makefile
> > > b/drivers/char/tpm/Makefile
> > > index 84db4fb3a9c9..66d39ea6bd10 100644
> > > --- a/drivers/char/tpm/Makefile
> > > +++ b/drivers/char/tpm/Makefile
> > > @@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> > > tpm_tis_spi-y := tpm_tis_spi_main.o
> > > tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
> > >
> > > +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
> > > +
> > > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
> > > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
> > > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > new file mode 100644
> > > index 000000000000..37555dafdca0
> > > --- /dev/null
> > > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > @@ -0,0 +1,768 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2016 Google Inc.
> > > + *
> > > + * Based on Linux Kernel TPM driver by
> > > + * Peter Huewe <[email protected]>
> > > + * Copyright (C) 2011 Infineon Technologies
> > > + */
> > > +
> > > +/*
> > > + * cr50 is a firmware for H1 secure modules that requires
> > > special
> > > + * handling for the I2C interface.
> > > + *
> > > + * - Use an interrupt for transaction status instead of
> > > hardcoded delays
> > > + * - Must use write+wait+read read protocol
> > > + * - All 4 bytes of status register must be read/written at once
> > > + * - Burst count max is 63 bytes, and burst count behaves
> > > + * slightly differently than other I2C TPMs
> > > + * - When reading from FIFO the full burstcnt must be read
> > > + * instead of just reading header and determining the
> > > remainder
> > > + */
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pm.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/wait.h>
> > > +
> > > +#include "tpm_tis_core.h"
> > > +
> > > +#define CR50_MAX_BUFSIZE 64
> > > +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during
> > > transactions */
> > > +#define CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready
> > > without IRQ */
> > > +#define CR50_I2C_DID_VID 0x00281ae0L
> > > +#define CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C
> > > errors */
> > > +#define CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs
> > > between retries on I2C */
> > > +#define CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs
> > > between retries on I2C */
> >
> > CR50_ -> TPM_CR50_
> >
> > > +
> > > +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
> > > +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
> > > +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
> > > +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4))
> > > +
> > > +struct priv_data {
> > > + int irq;
> > > + int locality;
> > > + struct completion tpm_ready;
> > > + u8 buf[CR50_MAX_BUFSIZE];
> > > +};
> >
> > tpm_i2c_cr50_priv_data
> >
> > > +
> > > +enum force_release {
> > > + CR50_NO_FORCE = 0x0,
> > > + CR50_FORCE = 0x1,
> > > +};
> >
> > I'd just
> >
> > #define TPM_I2C_CR50_NO_FORCE 0
> > #define TPM_I2C_CR50_FORCE 1
> >
>
> A proper enumerated type has advantages over a preprocessor macro:
> even if the compiler won't warn you, static analyzers can warn
> about a misuse.
Why don't you just use "bool", "true" and "false"? I ignored that
this has nothing to do with the hardware last time.
> Also, it allows for a more obvious prototype.
>
> I am curious why do you propose this change?
>
> Thanks,
> Ezequiel
/Jarkko
On Thu, 2020-11-26 at 05:30 +0200, Jarkko Sakkinen wrote:
> On Tue, 2020-11-24 at 10:14 -0300, Ezequiel Garcia wrote:
> > Hi Jarkko,
> >
> > Thanks for your review.
> >
> > On Tue, 2020-11-24 at 00:06 +0200, Jarkko Sakkinen wrote:
> > > On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu wrote:
> > > > From: "[email protected]" <[email protected]>
> > > >
> > > > Add TPM 2.0 compatible I2C interface for chips with cr50
> > > > firmware.
> > > >
> > > > The firmware running on the currently supported H1 MCU requires a
> > > > special driver to handle its specific protocol, and this makes it
> > > > unsuitable to use tpm_tis_core_* and instead it must implement
> > > > the
> > > > underlying TPM protocol similar to the other I2C TPM drivers.
> > > >
> > > > - All 4 byes of status register must be read/written at once.
> > > > - FIFO and burst count is limited to 63 and must be drained by
> > > > AP.
> > > > - Provides an interrupt to indicate when read response data is
> > > > ready
> > > > and when the TPM is finished processing write data.
> > > >
> > > > This driver is based on the existing infineon I2C TPM driver,
> > > > which
> > > > most closely matches the cr50 i2c protocol behavior.
> > > >
> > > > Cc: Helen Koike <[email protected]>
> > > > Signed-off-by: Duncan Laurie <[email protected]>
> > > > [[email protected]: Depend on i2c even if it's a module,
> > > > replace
> > > > boilier plate with SPDX tag, drop asm/byteorder.h include,
> > > > simplify
> > > > return from probe]
> > > > Signed-off-by: Stephen Boyd <[email protected]>
> > > > Signed-off-by: Fabien Lahoudere <[email protected]>
> > > > Signed-off-by: Adrian Ratiu <[email protected]>
> > > > ---
> > > > Changes in v2:
> > > > - Various small fixes all over (reorder includes, MAX_BUFSIZE,
> > > > comments, etc)
> > > > - Reworked return values of i2c_wait_tpm_ready() to fix timeout
> > > > mis-handling
> > > > so ret == 0 now means success, the wait period jiffies is ignored
> > > > because that
> > > > number is meaningless and return a proper timeout error in case
> > > > jiffies == 0.
> > > > - Make i2c default to 1 message per transfer (requested by
> > > > Helen)
> > > > - Move -EIO error reporting to transfer function to cleanup
> > > > transfer() itself
> > > > and its R/W callers
> > > > - Remove magic value hardcodings and introduce enum
> > > > force_release.
> > > >
> > > > v1 posted at https://lkml.org/lkml/2020/2/25/349
> > > >
> > > > Applies on next-20201120, tested on Chromebook EVE.
> > > > ---
> > > > drivers/char/tpm/Kconfig | 10 +
> > > > drivers/char/tpm/Makefile | 2 +
> > > > drivers/char/tpm/tpm_tis_i2c_cr50.c | 768
> > > > ++++++++++++++++++++++++++++
> > > > 3 files changed, 780 insertions(+)
> > > > create mode 100644 drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > >
> > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > index a18c314da211..4308f9ca7a43 100644
> > > > --- a/drivers/char/tpm/Kconfig
> > > > +++ b/drivers/char/tpm/Kconfig
> > > > @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
> > > > To compile this driver as a module, choose M here;
> > > > the module will be called tpm_tis_synquacer.
> > > >
> > > > +config TCG_TIS_I2C_CR50
> > > > + tristate "TPM Interface Specification 2.0 Interface (I2C
> > > > - CR50)"
> > > > + depends on I2C
> > > > + select TCG_CR50
> > > > + help
> > > > + This is a driver for the Google cr50 I2C TPM interface
> > > > which is a
> > > > + custom microcontroller and requires a custom i2c
> > > > protocol interface
> > > > + to handle the limitations of the hardware. To compile
> > > > this driver
> > > > + as a module, choose M here; the module will be called
> > > > tcg_tis_i2c_cr50.
> > > > +
> > > > config TCG_TIS_I2C_ATMEL
> > > > tristate "TPM Interface Specification 1.2 Interface (I2C
> > > > - Atmel)"
> > > > depends on I2C
> > > > diff --git a/drivers/char/tpm/Makefile
> > > > b/drivers/char/tpm/Makefile
> > > > index 84db4fb3a9c9..66d39ea6bd10 100644
> > > > --- a/drivers/char/tpm/Makefile
> > > > +++ b/drivers/char/tpm/Makefile
> > > > @@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> > > > tpm_tis_spi-y := tpm_tis_spi_main.o
> > > > tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
> > > >
> > > > +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
> > > > +
> > > > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
> > > > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
> > > > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> > > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > new file mode 100644
> > > > index 000000000000..37555dafdca0
> > > > --- /dev/null
> > > > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > @@ -0,0 +1,768 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright 2016 Google Inc.
> > > > + *
> > > > + * Based on Linux Kernel TPM driver by
> > > > + * Peter Huewe <[email protected]>
> > > > + * Copyright (C) 2011 Infineon Technologies
> > > > + */
> > > > +
> > > > +/*
> > > > + * cr50 is a firmware for H1 secure modules that requires
> > > > special
> > > > + * handling for the I2C interface.
> > > > + *
> > > > + * - Use an interrupt for transaction status instead of
> > > > hardcoded delays
> > > > + * - Must use write+wait+read read protocol
> > > > + * - All 4 bytes of status register must be read/written at once
> > > > + * - Burst count max is 63 bytes, and burst count behaves
> > > > + * slightly differently than other I2C TPMs
> > > > + * - When reading from FIFO the full burstcnt must be read
> > > > + * instead of just reading header and determining the
> > > > remainder
> > > > + */
> > > > +
> > > > +#include <linux/acpi.h>
> > > > +#include <linux/completion.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/pm.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/wait.h>
> > > > +
> > > > +#include "tpm_tis_core.h"
> > > > +
> > > > +#define CR50_MAX_BUFSIZE 64
> > > > +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during
> > > > transactions */
> > > > +#define CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready
> > > > without IRQ */
> > > > +#define CR50_I2C_DID_VID 0x00281ae0L
> > > > +#define CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C
> > > > errors */
> > > > +#define CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs
> > > > between retries on I2C */
> > > > +#define CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs
> > > > between retries on I2C */
> > >
> > > CR50_ -> TPM_CR50_
> > >
> > > > +
> > > > +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
> > > > +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
> > > > +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
> > > > +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4))
> > > > +
> > > > +struct priv_data {
> > > > + int irq;
> > > > + int locality;
> > > > + struct completion tpm_ready;
> > > > + u8 buf[CR50_MAX_BUFSIZE];
> > > > +};
> > >
> > > tpm_i2c_cr50_priv_data
> > >
> > > > +
> > > > +enum force_release {
> > > > + CR50_NO_FORCE = 0x0,
> > > > + CR50_FORCE = 0x1,
> > > > +};
> > >
> > > I'd just
> > >
> > > #define TPM_I2C_CR50_NO_FORCE 0
> > > #define TPM_I2C_CR50_FORCE 1
> > >
> >
> > A proper enumerated type has advantages over a preprocessor macro:
> > even if the compiler won't warn you, static analyzers can warn
> > about a misuse.
>
> Why don't you just use "bool", "true" and "false"? I ignored that
> this has nothing to do with the hardware last time.
>
Well, boolean parameters are a known anti-pattern [1].
[1] https://people.mpi-inf.mpg.de/~jblanche/api-design.pdf
On Thu, 26 Nov 2020, Ezequiel Garcia <[email protected]>
wrote:
> On Thu, 2020-11-26 at 05:30 +0200, Jarkko Sakkinen wrote:
>> On Tue, 2020-11-24 at 10:14 -0300, Ezequiel Garcia wrote:
>> > Hi Jarkko, Thanks for your review. On Tue, 2020-11-24 at
>> > 00:06 +0200, Jarkko Sakkinen wrote:
>> > > On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu
>> > > wrote:
>> > > > From: "[email protected]" <[email protected]> Add
>> > > > TPM 2.0 compatible I2C interface for chips with cr50
>> > > > firmware. The firmware running on the currently
>> > > > supported H1 MCU requires a special driver to handle its
>> > > > specific protocol, and this makes it unsuitable to use
>> > > > tpm_tis_core_* and instead it must implement the
>> > > > underlying TPM protocol similar to the other I2C TPM
>> > > > drivers. - All 4 byes of status register must be
>> > > > read/written at once. - FIFO and burst count is limited
>> > > > to 63 and must be drained by AP. - Provides an interrupt
>> > > > to indicate when read response data is ready and when the
>> > > > TPM is finished processing write data. This driver is
>> > > > based on the existing infineon I2C TPM driver, which most
>> > > > closely matches the cr50 i2c protocol behavior. Cc:
>> > > > Helen Koike <[email protected]> Signed-off-by:
>> > > > Duncan Laurie <[email protected]>
>> > > > [[email protected]: Depend on i2c even if it's a
>> > > > module, replace boilier plate with SPDX tag, drop
>> > > > asm/byteorder.h include, simplify return from probe]
>> > > > Signed-off-by: Stephen Boyd <[email protected]>
>> > > > Signed-off-by: Fabien Lahoudere
>> > > > <[email protected]> Signed-off-by: Adrian
>> > > > Ratiu <[email protected]> --- Changes in v2:
>> > > > - Various small fixes all over (reorder includes,
>> > > > MAX_BUFSIZE,
>> > > > comments, etc)
>> > > > - Reworked return values of i2c_wait_tpm_ready() to fix
>> > > > timeout
>> > > > mis-handling so ret == 0 now means success, the wait
>> > > > period jiffies is ignored because that number is
>> > > > meaningless and return a proper timeout error in case
>> > > > jiffies == 0.
>> > > > - Make i2c default to 1 message per transfer (requested
>> > > > by
>> > > > Helen)
>> > > > - Move -EIO error reporting to transfer function to
>> > > > cleanup
>> > > > transfer() itself and its R/W callers
>> > > > - Remove magic value hardcodings and introduce enum
>> > > > force_release. v1 posted at
>> > > > https://lkml.org/lkml/2020/2/25/349 Applies on
>> > > > next-20201120, tested on Chromebook EVE. ---
>> > > > drivers/char/tpm/Kconfig | 10 +
>> > > > drivers/char/tpm/Makefile | 2 +
>> > > > drivers/char/tpm/tpm_tis_i2c_cr50.c | 768
>> > > > ++++++++++++++++++++++++++++
>> > > > 3 files changed, 780 insertions(+) create mode 100644
>> > > > drivers/char/tpm/tpm_tis_i2c_cr50.c
>> > > > diff --git a/drivers/char/tpm/Kconfig
>> > > > b/drivers/char/tpm/Kconfig index
>> > > > a18c314da211..4308f9ca7a43 100644 ---
>> > > > a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig
>> > > > @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
>> > > > To compile this driver as a module, choose M
>> > > > here; the module will be called
>> > > > tpm_tis_synquacer.
>> > > >
>> > > > +config TCG_TIS_I2C_CR50 + tristate "TPM Interface
>> > > > Specification 2.0 Interface (I2C - CR50)" + depends
>> > > > on I2C + select TCG_CR50 + help +
>> > > > This is a driver for the Google cr50 I2C TPM interface
>> > > > which is a + custom microcontroller and requires
>> > > > a custom i2c protocol interface + to handle the
>> > > > limitations of the hardware. To compile this driver +
>> > > > as a module, choose M here; the module will be called
>> > > > tcg_tis_i2c_cr50. +
>> > > > config TCG_TIS_I2C_ATMEL
>> > > > tristate "TPM Interface Specification 1.2
>> > > > Interface (I2C
>> > > > - Atmel)"
>> > > > depends on I2C
>> > > > diff --git a/drivers/char/tpm/Makefile
>> > > > b/drivers/char/tpm/Makefile index
>> > > > 84db4fb3a9c9..66d39ea6bd10 100644 ---
>> > > > a/drivers/char/tpm/Makefile +++
>> > > > b/drivers/char/tpm/Makefile @@ -27,6 +27,8 @@
>> > > > obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
>> > > > tpm_tis_spi-y := tpm_tis_spi_main.o
>> > > > tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) +=
>> > > > tpm_tis_spi_cr50.o
>> > > > +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o +
>> > > > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
>> > > > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
>> > > > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
>> > > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c
>> > > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c new file mode
>> > > > 100644 index 000000000000..37555dafdca0 --- /dev/null +++
>> > > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c @@ -0,0 +1,768 @@
>> > > > +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright
>> > > > 2016 Google Inc. + * + * Based on Linux Kernel TPM
>> > > > driver by + * Peter Huewe <[email protected]> + *
>> > > > Copyright (C) 2011 Infineon Technologies + */ + +/* + *
>> > > > cr50 is a firmware for H1 secure modules that requires
>> > > > special + * handling for the I2C interface. + * + * -
>> > > > Use an interrupt for transaction status instead of
>> > > > hardcoded delays + * - Must use write+wait+read read
>> > > > protocol + * - All 4 bytes of status register must be
>> > > > read/written at once + * - Burst count max is 63 bytes,
>> > > > and burst count behaves + * slightly differently than
>> > > > other I2C TPMs + * - When reading from FIFO the full
>> > > > burstcnt must be read + * instead of just reading
>> > > > header and determining the remainder + */ + +#include
>> > > > <linux/acpi.h> +#include <linux/completion.h> +#include
>> > > > <linux/i2c.h> +#include <linux/interrupt.h> +#include
>> > > > <linux/module.h> +#include <linux/pm.h> +#include
>> > > > <linux/slab.h> +#include <linux/wait.h> + +#include
>> > > > "tpm_tis_core.h" + +#define CR50_MAX_BUFSIZE 64
>> > > > +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout
>> > > > during transactions */ +#define CR50_TIMEOUT_NOIRQ_MS 20
>> > > > /* Timeout for TPM ready without IRQ */ +#define
>> > > > CR50_I2C_DID_VID 0x00281ae0L +#define
>> > > > CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C
>> > > > errors */ +#define CR50_I2C_RETRY_DELAY_LO 55
>> > > > /* Min usecs between retries on I2C */ +#define
>> > > > CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs
>> > > > between retries on I2C */
>> > > CR50_ -> TPM_CR50_
>> > > > + +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
>> > > > +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
>> > > > +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
>> > > > +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4)) +
>> > > > +struct priv_data { + int irq; + int
>> > > > locality; + struct completion tpm_ready; + u8
>> > > > buf[CR50_MAX_BUFSIZE]; +};
>> > > tpm_i2c_cr50_priv_data
>> > > > + +enum force_release { + CR50_NO_FORCE = 0x0, +
>> > > > CR50_FORCE = 0x1, +};
>> > > I'd just #define TPM_I2C_CR50_NO_FORCE 0 #define
>> > > TPM_I2C_CR50_FORCE 1
>> > A proper enumerated type has advantages over a preprocessor
>> > macro: even if the compiler won't warn you, static analyzers
>> > can warn about a misuse.
>> Why don't you just use "bool", "true" and "false"? I ignored
>> that this has nothing to do with the hardware last time.
>
> Well, boolean parameters are a known anti-pattern [1].
>
> [1] https://people.mpi-inf.mpg.de/~jblanche/api-design.pdf
Funny because that's what I wrote a few days ago in my v3 WIP
branch, #defines and a bool function parameter. XD
As Jarkko correctly observed these values are unrelated to the HW,
we just need to distinguish FORCE vs NO_FORCE and IMO any method
will do just as well for such a small and obvious use case.
So based on the arguments given, I'll stop bikeshedding and just
use enums :)
Thank you both,
Adrian
On Thu, Nov 26, 2020 at 03:19:24AM -0300, Ezequiel Garcia wrote:
> On Thu, 2020-11-26 at 05:30 +0200, Jarkko Sakkinen wrote:
> > On Tue, 2020-11-24 at 10:14 -0300, Ezequiel Garcia wrote:
> > > Hi Jarkko,
> > >
> > > Thanks for your review.
> > >
> > > On Tue, 2020-11-24 at 00:06 +0200, Jarkko Sakkinen wrote:
> > > > On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu wrote:
> > > > > From: "[email protected]" <[email protected]>
> > > > >
> > > > > Add TPM 2.0 compatible I2C interface for chips with cr50
> > > > > firmware.
> > > > >
> > > > > The firmware running on the currently supported H1 MCU requires a
> > > > > special driver to handle its specific protocol, and this makes it
> > > > > unsuitable to use tpm_tis_core_* and instead it must implement
> > > > > the
> > > > > underlying TPM protocol similar to the other I2C TPM drivers.
> > > > >
> > > > > - All 4 byes of status register must be read/written at once.
> > > > > - FIFO and burst count is limited to 63 and must be drained by
> > > > > AP.
> > > > > - Provides an interrupt to indicate when read response data is
> > > > > ready
> > > > > and when the TPM is finished processing write data.
> > > > >
> > > > > This driver is based on the existing infineon I2C TPM driver,
> > > > > which
> > > > > most closely matches the cr50 i2c protocol behavior.
> > > > >
> > > > > Cc: Helen Koike <[email protected]>
> > > > > Signed-off-by: Duncan Laurie <[email protected]>
> > > > > [[email protected]: Depend on i2c even if it's a module,
> > > > > replace
> > > > > boilier plate with SPDX tag, drop asm/byteorder.h include,
> > > > > simplify
> > > > > return from probe]
> > > > > Signed-off-by: Stephen Boyd <[email protected]>
> > > > > Signed-off-by: Fabien Lahoudere <[email protected]>
> > > > > Signed-off-by: Adrian Ratiu <[email protected]>
> > > > > ---
> > > > > Changes in v2:
> > > > > - Various small fixes all over (reorder includes, MAX_BUFSIZE,
> > > > > comments, etc)
> > > > > - Reworked return values of i2c_wait_tpm_ready() to fix timeout
> > > > > mis-handling
> > > > > so ret == 0 now means success, the wait period jiffies is ignored
> > > > > because that
> > > > > number is meaningless and return a proper timeout error in case
> > > > > jiffies == 0.
> > > > > - Make i2c default to 1 message per transfer (requested by
> > > > > Helen)
> > > > > - Move -EIO error reporting to transfer function to cleanup
> > > > > transfer() itself
> > > > > and its R/W callers
> > > > > - Remove magic value hardcodings and introduce enum
> > > > > force_release.
> > > > >
> > > > > v1 posted at https://lkml.org/lkml/2020/2/25/349
> > > > >
> > > > > Applies on next-20201120, tested on Chromebook EVE.
> > > > > ---
> > > > > drivers/char/tpm/Kconfig | 10 +
> > > > > drivers/char/tpm/Makefile | 2 +
> > > > > drivers/char/tpm/tpm_tis_i2c_cr50.c | 768
> > > > > ++++++++++++++++++++++++++++
> > > > > 3 files changed, 780 insertions(+)
> > > > > create mode 100644 drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > >
> > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > index a18c314da211..4308f9ca7a43 100644
> > > > > --- a/drivers/char/tpm/Kconfig
> > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
> > > > > To compile this driver as a module, choose M here;
> > > > > the module will be called tpm_tis_synquacer.
> > > > >
> > > > > +config TCG_TIS_I2C_CR50
> > > > > + tristate "TPM Interface Specification 2.0 Interface (I2C
> > > > > - CR50)"
> > > > > + depends on I2C
> > > > > + select TCG_CR50
> > > > > + help
> > > > > + This is a driver for the Google cr50 I2C TPM interface
> > > > > which is a
> > > > > + custom microcontroller and requires a custom i2c
> > > > > protocol interface
> > > > > + to handle the limitations of the hardware. To compile
> > > > > this driver
> > > > > + as a module, choose M here; the module will be called
> > > > > tcg_tis_i2c_cr50.
> > > > > +
> > > > > config TCG_TIS_I2C_ATMEL
> > > > > tristate "TPM Interface Specification 1.2 Interface (I2C
> > > > > - Atmel)"
> > > > > depends on I2C
> > > > > diff --git a/drivers/char/tpm/Makefile
> > > > > b/drivers/char/tpm/Makefile
> > > > > index 84db4fb3a9c9..66d39ea6bd10 100644
> > > > > --- a/drivers/char/tpm/Makefile
> > > > > +++ b/drivers/char/tpm/Makefile
> > > > > @@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> > > > > tpm_tis_spi-y := tpm_tis_spi_main.o
> > > > > tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
> > > > >
> > > > > +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
> > > > > +
> > > > > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
> > > > > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
> > > > > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> > > > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > > new file mode 100644
> > > > > index 000000000000..37555dafdca0
> > > > > --- /dev/null
> > > > > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > > @@ -0,0 +1,768 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright 2016 Google Inc.
> > > > > + *
> > > > > + * Based on Linux Kernel TPM driver by
> > > > > + * Peter Huewe <[email protected]>
> > > > > + * Copyright (C) 2011 Infineon Technologies
> > > > > + */
> > > > > +
> > > > > +/*
> > > > > + * cr50 is a firmware for H1 secure modules that requires
> > > > > special
> > > > > + * handling for the I2C interface.
> > > > > + *
> > > > > + * - Use an interrupt for transaction status instead of
> > > > > hardcoded delays
> > > > > + * - Must use write+wait+read read protocol
> > > > > + * - All 4 bytes of status register must be read/written at once
> > > > > + * - Burst count max is 63 bytes, and burst count behaves
> > > > > + * slightly differently than other I2C TPMs
> > > > > + * - When reading from FIFO the full burstcnt must be read
> > > > > + * instead of just reading header and determining the
> > > > > remainder
> > > > > + */
> > > > > +
> > > > > +#include <linux/acpi.h>
> > > > > +#include <linux/completion.h>
> > > > > +#include <linux/i2c.h>
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/pm.h>
> > > > > +#include <linux/slab.h>
> > > > > +#include <linux/wait.h>
> > > > > +
> > > > > +#include "tpm_tis_core.h"
> > > > > +
> > > > > +#define CR50_MAX_BUFSIZE 64
> > > > > +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during
> > > > > transactions */
> > > > > +#define CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready
> > > > > without IRQ */
> > > > > +#define CR50_I2C_DID_VID 0x00281ae0L
> > > > > +#define CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C
> > > > > errors */
> > > > > +#define CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs
> > > > > between retries on I2C */
> > > > > +#define CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs
> > > > > between retries on I2C */
> > > >
> > > > CR50_ -> TPM_CR50_
> > > >
> > > > > +
> > > > > +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
> > > > > +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
> > > > > +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
> > > > > +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4))
> > > > > +
> > > > > +struct priv_data {
> > > > > + int irq;
> > > > > + int locality;
> > > > > + struct completion tpm_ready;
> > > > > + u8 buf[CR50_MAX_BUFSIZE];
> > > > > +};
> > > >
> > > > tpm_i2c_cr50_priv_data
> > > >
> > > > > +
> > > > > +enum force_release {
> > > > > + CR50_NO_FORCE = 0x0,
> > > > > + CR50_FORCE = 0x1,
> > > > > +};
> > > >
> > > > I'd just
> > > >
> > > > #define TPM_I2C_CR50_NO_FORCE 0
> > > > #define TPM_I2C_CR50_FORCE 1
> > > >
> > >
> > > A proper enumerated type has advantages over a preprocessor macro:
> > > even if the compiler won't warn you, static analyzers can warn
> > > about a misuse.
> >
> > Why don't you just use "bool", "true" and "false"? I ignored that
> > this has nothing to do with the hardware last time.
> >
>
> Well, boolean parameters are a known anti-pattern [1].
>
> [1] https://people.mpi-inf.mpg.de/~jblanche/api-design.pdf
It's an internal function to begin with.
/Jarkko
On Sun, 2020-11-29 at 05:29 +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 26, 2020 at 03:19:24AM -0300, Ezequiel Garcia wrote:
> > On Thu, 2020-11-26 at 05:30 +0200, Jarkko Sakkinen wrote:
> > > On Tue, 2020-11-24 at 10:14 -0300, Ezequiel Garcia wrote:
> > > > Hi Jarkko,
> > > >
> > > > Thanks for your review.
> > > >
> > > > On Tue, 2020-11-24 at 00:06 +0200, Jarkko Sakkinen wrote:
> > > > > On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu wrote:
> > > > > > From: "[email protected]" <[email protected]>
> > > > > >
> > > > > > Add TPM 2.0 compatible I2C interface for chips with cr50
> > > > > > firmware.
> > > > > >
> > > > > > The firmware running on the currently supported H1 MCU requires a
> > > > > > special driver to handle its specific protocol, and this makes it
> > > > > > unsuitable to use tpm_tis_core_* and instead it must implement
> > > > > > the
> > > > > > underlying TPM protocol similar to the other I2C TPM drivers.
> > > > > >
> > > > > > - All 4 byes of status register must be read/written at once.
> > > > > > - FIFO and burst count is limited to 63 and must be drained by
> > > > > > AP.
> > > > > > - Provides an interrupt to indicate when read response data is
> > > > > > ready
> > > > > > and when the TPM is finished processing write data.
> > > > > >
> > > > > > This driver is based on the existing infineon I2C TPM driver,
> > > > > > which
> > > > > > most closely matches the cr50 i2c protocol behavior.
> > > > > >
> > > > > > Cc: Helen Koike <[email protected]>
> > > > > > Signed-off-by: Duncan Laurie <[email protected]>
> > > > > > [[email protected]: Depend on i2c even if it's a module,
> > > > > > replace
> > > > > > boilier plate with SPDX tag, drop asm/byteorder.h include,
> > > > > > simplify
> > > > > > return from probe]
> > > > > > Signed-off-by: Stephen Boyd <[email protected]>
> > > > > > Signed-off-by: Fabien Lahoudere <[email protected]>
> > > > > > Signed-off-by: Adrian Ratiu <[email protected]>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Various small fixes all over (reorder includes, MAX_BUFSIZE,
> > > > > > comments, etc)
> > > > > > - Reworked return values of i2c_wait_tpm_ready() to fix timeout
> > > > > > mis-handling
> > > > > > so ret == 0 now means success, the wait period jiffies is ignored
> > > > > > because that
> > > > > > number is meaningless and return a proper timeout error in case
> > > > > > jiffies == 0.
> > > > > > - Make i2c default to 1 message per transfer (requested by
> > > > > > Helen)
> > > > > > - Move -EIO error reporting to transfer function to cleanup
> > > > > > transfer() itself
> > > > > > and its R/W callers
> > > > > > - Remove magic value hardcodings and introduce enum
> > > > > > force_release.
> > > > > >
> > > > > > v1 posted at https://lkml.org/lkml/2020/2/25/349
> > > > > >
> > > > > > Applies on next-20201120, tested on Chromebook EVE.
> > > > > > ---
> > > > > > drivers/char/tpm/Kconfig | 10 +
> > > > > > drivers/char/tpm/Makefile | 2 +
> > > > > > drivers/char/tpm/tpm_tis_i2c_cr50.c | 768
> > > > > > ++++++++++++++++++++++++++++
> > > > > > 3 files changed, 780 insertions(+)
> > > > > > create mode 100644 drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > > >
> > > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > > index a18c314da211..4308f9ca7a43 100644
> > > > > > --- a/drivers/char/tpm/Kconfig
> > > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > > @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER
> > > > > > To compile this driver as a module, choose M here;
> > > > > > the module will be called tpm_tis_synquacer.
> > > > > >
> > > > > > +config TCG_TIS_I2C_CR50
> > > > > > + tristate "TPM Interface Specification 2.0 Interface (I2C
> > > > > > - CR50)"
> > > > > > + depends on I2C
> > > > > > + select TCG_CR50
> > > > > > + help
> > > > > > + This is a driver for the Google cr50 I2C TPM interface
> > > > > > which is a
> > > > > > + custom microcontroller and requires a custom i2c
> > > > > > protocol interface
> > > > > > + to handle the limitations of the hardware. To compile
> > > > > > this driver
> > > > > > + as a module, choose M here; the module will be called
> > > > > > tcg_tis_i2c_cr50.
> > > > > > +
> > > > > > config TCG_TIS_I2C_ATMEL
> > > > > > tristate "TPM Interface Specification 1.2 Interface (I2C
> > > > > > - Atmel)"
> > > > > > depends on I2C
> > > > > > diff --git a/drivers/char/tpm/Makefile
> > > > > > b/drivers/char/tpm/Makefile
> > > > > > index 84db4fb3a9c9..66d39ea6bd10 100644
> > > > > > --- a/drivers/char/tpm/Makefile
> > > > > > +++ b/drivers/char/tpm/Makefile
> > > > > > @@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> > > > > > tpm_tis_spi-y := tpm_tis_spi_main.o
> > > > > > tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
> > > > > >
> > > > > > +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o
> > > > > > +
> > > > > > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
> > > > > > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
> > > > > > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> > > > > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..37555dafdca0
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > > > > > @@ -0,0 +1,768 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Copyright 2016 Google Inc.
> > > > > > + *
> > > > > > + * Based on Linux Kernel TPM driver by
> > > > > > + * Peter Huewe <[email protected]>
> > > > > > + * Copyright (C) 2011 Infineon Technologies
> > > > > > + */
> > > > > > +
> > > > > > +/*
> > > > > > + * cr50 is a firmware for H1 secure modules that requires
> > > > > > special
> > > > > > + * handling for the I2C interface.
> > > > > > + *
> > > > > > + * - Use an interrupt for transaction status instead of
> > > > > > hardcoded delays
> > > > > > + * - Must use write+wait+read read protocol
> > > > > > + * - All 4 bytes of status register must be read/written at once
> > > > > > + * - Burst count max is 63 bytes, and burst count behaves
> > > > > > + * slightly differently than other I2C TPMs
> > > > > > + * - When reading from FIFO the full burstcnt must be read
> > > > > > + * instead of just reading header and determining the
> > > > > > remainder
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/acpi.h>
> > > > > > +#include <linux/completion.h>
> > > > > > +#include <linux/i2c.h>
> > > > > > +#include <linux/interrupt.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/pm.h>
> > > > > > +#include <linux/slab.h>
> > > > > > +#include <linux/wait.h>
> > > > > > +
> > > > > > +#include "tpm_tis_core.h"
> > > > > > +
> > > > > > +#define CR50_MAX_BUFSIZE 64
> > > > > > +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during
> > > > > > transactions */
> > > > > > +#define CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready
> > > > > > without IRQ */
> > > > > > +#define CR50_I2C_DID_VID 0x00281ae0L
> > > > > > +#define CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C
> > > > > > errors */
> > > > > > +#define CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs
> > > > > > between retries on I2C */
> > > > > > +#define CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs
> > > > > > between retries on I2C */
> > > > >
> > > > > CR50_ -> TPM_CR50_
> > > > >
> > > > > > +
> > > > > > +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
> > > > > > +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
> > > > > > +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4))
> > > > > > +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4))
> > > > > > +
> > > > > > +struct priv_data {
> > > > > > + int irq;
> > > > > > + int locality;
> > > > > > + struct completion tpm_ready;
> > > > > > + u8 buf[CR50_MAX_BUFSIZE];
> > > > > > +};
> > > > >
> > > > > tpm_i2c_cr50_priv_data
> > > > >
> > > > > > +
> > > > > > +enum force_release {
> > > > > > + CR50_NO_FORCE = 0x0,
> > > > > > + CR50_FORCE = 0x1,
> > > > > > +};
> > > > >
> > > > > I'd just
> > > > >
> > > > > #define TPM_I2C_CR50_NO_FORCE 0
> > > > > #define TPM_I2C_CR50_FORCE 1
> > > > >
> > > >
> > > > A proper enumerated type has advantages over a preprocessor macro:
> > > > even if the compiler won't warn you, static analyzers can warn
> > > > about a misuse.
> > >
> > > Why don't you just use "bool", "true" and "false"? I ignored that
> > > this has nothing to do with the hardware last time.
> > >
> >
> > Well, boolean parameters are a known anti-pattern [1].
> >
> > [1] https://people.mpi-inf.mpg.de/~jblanche/api-design.pdf
>
> It's an internal function to begin with.
>
Too bad you see it that way. Anti-patterns spread like the flu,
and then it takes a lot of energy to take care of them.
Cheers,
Ezequiel