Discussion:
[PATCH 0/2] tpm: add driver for cr50 on SPI
(too old to reply)
Andrey Pronin
2016-07-15 02:20:30 UTC
Permalink
This patchset adds a TCG TPM2.0 PTP FIFO compliant interface for
Cr50 chip on SPI.

Depends on the following patches by Andrey Pronin <***@chromium.org>
that add new members to phy_ops in tpm_tis_core:
- tpm: support driver-specific sysfs attrs in tpm_tis_core
- tpm_tis_core: add optional max xfer size check

Andrey Pronin (2):
tpm: devicetree: document properties for cr50
tpm: add driver for cr50 on SPI

.../devicetree/bindings/security/tpm/cr50_spi.txt | 30 ++
drivers/char/tpm/Kconfig | 9 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/cr50_spi.c | 409 +++++++++++++++++++++
4 files changed, 449 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
create mode 100644 drivers/char/tpm/cr50_spi.c
--
2.6.6
Andrey Pronin
2016-07-15 02:20:59 UTC
Permalink
Add TPM2.0-compatible interface to Cr50. Document its properties
in devicetree.

Signed-off-by: Andrey Pronin <***@chromium.org>
---
.../devicetree/bindings/security/tpm/cr50_spi.txt | 30 ++++++++++++++++++++++
1 file changed, 30 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/cr50_spi.txt

diff --git a/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
new file mode 100644
index 0000000..1b05e51
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
@@ -0,0 +1,30 @@
+* Cr50 Chip on SPI.
+
+TCG PTP FIFO Compliant Interface to Cr50 on SPI bus.
+
+Required properties:
+- compatible: Should be "google,cr50_spi".
+- spi-max-frequency: Maximum SPI frequency.
+
+Optional properties:
+- access-delay-msec: Required delay between subsequent transactions on SPI.
+- sleep-delay-msec: Time after the last SPI activity, after which the chip
+ may go to sleep.
+- wake-start-delay-msec: Time after initiating wake up before the chip is
+ ready to accept commands over SPI.
+
+Example:
+
+&spi0 {
+ status = "okay";
+
+ ***@0 {
+ compatible = "google,cr50_spi";
+ reg = <0>;
+ spi-max-frequency = <800000>;
+
+ access-delay-msec = <2>;
+ sleep-delay-msec = <1000>;
+ wake-start-delay-msec = <60>;
+ };
+};
--
2.6.6
Guenter Roeck
2016-07-15 04:06:01 UTC
Permalink
Post by Andrey Pronin
Add TPM2.0-compatible interface to Cr50. Document its properties
in devicetree.
---
.../devicetree/bindings/security/tpm/cr50_spi.txt | 30 ++++++++++++++++++++++
1 file changed, 30 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
diff --git a/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
new file mode 100644
index 0000000..1b05e51
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
@@ -0,0 +1,30 @@
+* Cr50 Chip on SPI.
+
+TCG PTP FIFO Compliant Interface to Cr50 on SPI bus.
+
+- compatible: Should be "google,cr50_spi".
google,cr50, maybe ? The "_spi" seems redundant.

Also, I agree with comments from others - the term cr50 really needs
an explanation (Google thinks that it is a motor bike, a scanner, or a
coffee roaster).

Thanks,
Guenter
Post by Andrey Pronin
+- spi-max-frequency: Maximum SPI frequency.
+
+- access-delay-msec: Required delay between subsequent transactions on SPI.
+- sleep-delay-msec: Time after the last SPI activity, after which the chip
+ may go to sleep.
+- wake-start-delay-msec: Time after initiating wake up before the chip is
+ ready to accept commands over SPI.
+
+
+&spi0 {
+ status = "okay";
+
+ compatible = "google,cr50_spi";
+ reg = <0>;
+ spi-max-frequency = <800000>;
+
+ access-delay-msec = <2>;
+ sleep-delay-msec = <1000>;
+ wake-start-delay-msec = <60>;
+ };
+};
--
2.6.6
Andrey Pronin
2016-07-15 17:31:22 UTC
Permalink
Post by Guenter Roeck
Post by Andrey Pronin
+
+- compatible: Should be "google,cr50_spi".
google,cr50, maybe ? The "_spi" seems redundant.
I believe "_spi" is warranted. It's the driver that handles the SPI
interface for Cr50 specifically.
But if the same firmware ever talks through a different interface (say,
I2C), this driver will not be compatible.
Post by Guenter Roeck
Also, I agree with comments from others - the term cr50 really needs
an explanation (Google thinks that it is a motor bike, a scanner, or a
coffee roaster).
Yes, will add a better description of what it is. My original one was
too brief and imprecise at the same time.
Guenter Roeck
2016-07-15 18:28:43 UTC
Permalink
Post by Andrey Pronin
Post by Guenter Roeck
Post by Andrey Pronin
+
+- compatible: Should be "google,cr50_spi".
google,cr50, maybe ? The "_spi" seems redundant.
I believe "_spi" is warranted. It's the driver that handles the SPI
interface for Cr50 specifically.
But if the same firmware ever talks through a different interface (say,
I2C), this driver will not be compatible.
I meant in the context of it being attached to a spi device, which
implies that it is connected through a spi bus.

Guenter
Post by Andrey Pronin
Post by Guenter Roeck
Also, I agree with comments from others - the term cr50 really needs
an explanation (Google thinks that it is a motor bike, a scanner, or a
coffee roaster).
Yes, will add a better description of what it is. My original one was
too brief and imprecise at the same time.
Rob Herring
2016-07-17 13:28:28 UTC
Permalink
Post by Andrey Pronin
Add TPM2.0-compatible interface to Cr50. Document its properties
in devicetree.
---
.../devicetree/bindings/security/tpm/cr50_spi.txt | 30 ++++++++++++++++++++++
1 file changed, 30 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
diff --git a/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
new file mode 100644
index 0000000..1b05e51
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
@@ -0,0 +1,30 @@
+* Cr50 Chip on SPI.
+
+TCG PTP FIFO Compliant Interface to Cr50 on SPI bus.
+
+- compatible: Should be "google,cr50_spi".
I agree with dropping '_spi'. The interface is defined by the parent
device.
Post by Andrey Pronin
+- spi-max-frequency: Maximum SPI frequency.
+
+- access-delay-msec: Required delay between subsequent transactions on SPI.
There may be a standard property for this...
Post by Andrey Pronin
+- sleep-delay-msec: Time after the last SPI activity, after which the chip
+ may go to sleep.
+- wake-start-delay-msec: Time after initiating wake up before the chip is
+ ready to accept commands over SPI.
Use the standard unit '-ms' instead of '-msec'.

Do these times really vary much and need to be in DT?
Andrey Pronin
2016-07-15 02:21:12 UTC
Permalink
Add TCG TPM2.0 PTP FIFO compatible interface for Cr50 chip on SPI bus.

Signed-off-by: Andrey Pronin <***@chromium.org>
---
drivers/char/tpm/Kconfig | 9 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/cr50_spi.c | 409 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 419 insertions(+)
create mode 100644 drivers/char/tpm/cr50_spi.c

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 9faa0b1..6773a06 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -100,6 +100,15 @@ config TCG_ATMEL
will be accessible from within Linux. To compile this driver
as a module, choose M here; the module will be called tpm_atmel.

+config TCG_CR50_SPI
+ tristate "TCG PTP FIFO Compliant Interface over SPI for Cr50"
+ depends on SPI
+ select TCG_TIS_CORE
+ ---help---
+ If you have a Cr50 chip on SPI bus, say Yes and it will be
+ accessible from within Linux. To compile this driver as a module,
+ choose M here; the module will be called cr50_spi.
+
config TCG_INFINEON
tristate "Infineon Technologies TPM Interface"
depends on PNP
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a385fb8..b346306 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
+obj-$(CONFIG_TCG_CR50_SPI) += cr50_spi.o
obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c
new file mode 100644
index 0000000..0672434
--- /dev/null
+++ b/drivers/char/tpm/cr50_spi.c
@@ -0,0 +1,409 @@
+/*
+ * Copyright (C) 2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2, as published by
+ * the Free Software Foundation.
+ *
+ * This device driver implements a TCG PTP FIFO compliant interface over SPI
+ * for Cr50 devices.
+ * It is based on cr50_spi driver by Peter Huewe and Christophe Ricard.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/wait.h>
+#include <linux/freezer.h>
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/gpio.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/tpm.h>
+#include "tpm.h"
+#include "tpm_tis_core.h"
+
+#define MAX_SPI_FRAMESIZE 64
+
+/* Cr50 default timing constants:
+ * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC
+ * - needs up to CR50_WAKE_START_DELAY_MSEC to wake after sleep
+ * - requires at least CR50_ACCESS_DELAY_MSEC between transactions
+ */
+#define CR50_DFT_SLEEP_DELAY_MSEC 1000
+#define CR50_DFT_WAKE_START_DELAY_MSEC 60
+#define CR50_DFT_ACCESS_DELAY_MSEC 2
+
+#define TPM_CR50_FW_VER(l) (0x0F90 | ((l) << 12))
+#define TPM_CR50_MAX_FW_VER_LEN 64
+
+struct cr50_spi_phy {
+ struct tpm_tis_data priv;
+ struct spi_device *spi_device;
+
+ struct mutex time_track_mutex;
+ unsigned long last_access_jiffies;
+ unsigned long wake_after_jiffies;
+
+ unsigned long access_delay_jiffies;
+ unsigned long sleep_delay_jiffies;
+ unsigned int wake_start_delay_msec;
+};
+
+static inline struct cr50_spi_phy *to_cr50_spi_phy(struct tpm_tis_data *data)
+{
+ return container_of(data, struct cr50_spi_phy, priv);
+}
+
+/* Cr50 needs to have at least some delay between consecutive
+ * transactions. Make sure we wait.
+ */
+static inline void cr50_ensure_access_delay(struct cr50_spi_phy *phy)
+{
+ /* Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll have an unneeded short delay,
+ * which is fine.
+ */
+ unsigned long allowed_access =
+ phy->last_access_jiffies + phy->access_delay_jiffies;
+ unsigned long time_now = jiffies;
+
+ if (time_in_range_open(time_now,
+ phy->last_access_jiffies, allowed_access))
+ mdelay(jiffies_to_msecs(allowed_access - time_now));
+}
+
+/* Cr50 might go to sleep if there is no SPI activity for some time and
+ * miss the first few bits/bytes on the bus. In such case, wake it up
+ * by asserting CS and give it time to start up.
+ */
+static inline bool cr50_needs_waking(struct cr50_spi_phy *phy)
+{
+ /* Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll probably timeout or read
+ * incorrect value from TPM_STS and just retry the operation.
+ */
+ return !time_in_range_open(jiffies,
+ phy->last_access_jiffies,
+ phy->wake_after_jiffies);
+}
+static inline void cr50_wake_if_needed(struct cr50_spi_phy *phy)
+{
+ if (cr50_needs_waking(phy)) {
+ /* assert CS, wait 1 msec, deassert CS */
+ struct spi_transfer spi_cs_wake = { .delay_usecs = 1000 };
+
+ spi_sync_transfer(phy->spi_device, &spi_cs_wake, 1);
+ /* wait for it to fully wake */
+ msleep(phy->wake_start_delay_msec);
+ }
+ /* Reset the time when we need to wake Cr50 again */
+ phy->wake_after_jiffies = jiffies + phy->sleep_delay_jiffies;
+}
+
+/* Flow control: clock the bus and wait for cr50 to set LSB before
+ * sending/receiving data. TCG PTP spec allows it to happen during
+ * the last byte of header, but cr50 never does that in practice,
+ * and earlier versions had a bug when it was set too early, so don't
+ * check for it during header transfer.
+ */
+static int cr50_spi_flow_control(struct cr50_spi_phy *phy)
+{
+ unsigned long timeout_jiffies =
+ jiffies + msecs_to_jiffies(TPM_RETRY * TPM_TIMEOUT_RETRY);
+ struct spi_message m;
+ int ret;
+ u8 rx = 0;
+ struct spi_transfer spi_xfer = {
+ .rx_buf = &rx,
+ .len = 1,
+ .cs_change = 1,
+ };
+
+ do {
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ return ret;
+ if (time_after(jiffies, timeout_jiffies))
+ return -EBUSY;
+ } while (!(rx & 0x01));
+ return 0;
+}
+
+static int cr50_spi_xfer_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *buf, bool do_write)
+{
+ struct cr50_spi_phy *phy = to_cr50_spi_phy(data);
+ struct spi_message m;
+ u8 tx_buf[4];
+ u8 rx_buf[4];
+ struct spi_transfer spi_xfer = {
+ .tx_buf = tx_buf,
+ .rx_buf = rx_buf,
+ .len = 4,
+ .cs_change = 1,
+ };
+ int ret;
+
+ if (len > MAX_SPI_FRAMESIZE)
+ return -EINVAL;
+
+ /* Do this outside of spi_bus_lock in case cr50 is not the
+ * only device on that spi bus.
+ */
+ mutex_lock(&phy->time_track_mutex);
+ cr50_ensure_access_delay(phy);
+ cr50_wake_if_needed(phy);
+ mutex_unlock(&phy->time_track_mutex);
+
+ tx_buf[0] = (do_write ? 0x00 : 0x80) | (len - 1);
+ tx_buf[1] = 0xD4;
+ tx_buf[2] = (addr >> 8) & 0xFF;
+ tx_buf[3] = addr & 0xFF;
+
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+
+ spi_bus_lock(phy->spi_device->master);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ goto exit;
+
+ ret = cr50_spi_flow_control(phy);
+ if (ret < 0)
+ goto exit;
+
+ spi_xfer.cs_change = 0;
+ spi_xfer.len = len;
+ if (do_write) {
+ spi_xfer.tx_buf = buf;
+ spi_xfer.rx_buf = NULL;
+ } else {
+ spi_xfer.tx_buf = NULL;
+ spi_xfer.rx_buf = buf;
+ }
+
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+
+exit:
+ spi_bus_unlock(phy->spi_device->master);
+
+ mutex_lock(&phy->time_track_mutex);
+ phy->last_access_jiffies = jiffies;
+ mutex_unlock(&phy->time_track_mutex);
+
+ return ret;
+}
+
+static int cr50_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result)
+{
+ return cr50_spi_xfer_bytes(data, addr, len, result, false);
+}
+
+static int cr50_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *value)
+{
+ return cr50_spi_xfer_bytes(data, addr, len, value, true);
+}
+
+static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+{
+ int rc;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
+ if (!rc)
+ *result = le16_to_cpu(*result);
+ return rc;
+}
+
+static int cr50_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
+{
+ int rc;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u32), (u8 *)result);
+ if (!rc)
+ *result = le32_to_cpu(*result);
+ return rc;
+}
+
+static int cr50_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
+{
+ value = cpu_to_le32(value);
+ return data->phy_ops->write_bytes(data, addr, sizeof(u32),
+ (u8 *)&value);
+}
+
+static void cr50_get_fw_version(struct tpm_tis_data *data, char *fw_ver)
+{
+ int i, len = 0;
+ char fw_ver_block[4];
+
+ /* Write anything to TPM_CR50_FW_VER to start from the beg of string */
+ tpm_tis_write8(data, TPM_CR50_FW_VER(data->locality), 0);
+
+ /* Read the string, 4 bytes at a time, until we get '\0' */
+ do {
+ tpm_tis_read_bytes(data,
+ TPM_CR50_FW_VER(data->locality), 4, fw_ver_block);
+ for (i = 0; i < 4 && fw_ver_block[i]; )
+ fw_ver[len++] = fw_ver_block[i++];
+ } while (i == 4 && len < TPM_CR50_MAX_FW_VER_LEN);
+ fw_ver[len] = 0;
+}
+
+static ssize_t fw_version_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ char fw_ver[TPM_CR50_MAX_FW_VER_LEN+1];
+ struct tpm_tis_data *data = dev_get_drvdata(dev);
+
+ cr50_get_fw_version(data, fw_ver);
+ return sprintf(buf, "%s\n", fw_ver);
+}
+static DEVICE_ATTR_RO(fw_version);
+
+static struct attribute *cr50_attrs[] = {
+ &dev_attr_fw_version.attr,
+ NULL,
+};
+
+static const struct attribute_group cr50_attr_group = {
+ .attrs = cr50_attrs,
+};
+
+static const struct tpm_tis_phy_ops cr50_spi_phy_ops = {
+ .read_bytes = cr50_spi_read_bytes,
+ .write_bytes = cr50_spi_write_bytes,
+ .read16 = cr50_spi_read16,
+ .read32 = cr50_spi_read32,
+ .write32 = cr50_spi_write32,
+ .attr_group = &cr50_attr_group,
+ .max_xfer_size = MAX_SPI_FRAMESIZE,
+};
+
+static int cr50_of_property_read_u32_optional(struct spi_device *dev,
+ const char *name,
+ u32 default_value,
+ u32 *value)
+{
+ struct device_node *np = dev->dev.of_node;
+ int rc;
+
+ if (of_find_property(np, name, NULL)) {
+ rc = of_property_read_u32(np, name, value);
+ if (rc < 0) {
+ dev_err(&dev->dev,
+ "invalid '%s' property (%d)\n",
+ name, rc);
+ return rc;
+ }
+ } else {
+ *value = default_value;
+ }
+
+ dev_dbg(&dev->dev, "%s = %u\n", name, *value);
+ return 0;
+}
+
+static int cr50_spi_probe(struct spi_device *dev)
+{
+ char fw_ver[TPM_CR50_MAX_FW_VER_LEN+1];
+ struct cr50_spi_phy *phy;
+ int rc;
+ u32 value;
+
+ phy = devm_kzalloc(&dev->dev, sizeof(struct cr50_spi_phy),
+ GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ phy->spi_device = dev;
+
+ /* Cr50 timing properties.
+ */
+ rc = cr50_of_property_read_u32_optional(dev, "access-delay-msec",
+ CR50_DFT_ACCESS_DELAY_MSEC,
+ &value);
+ if (rc < 0)
+ return rc;
+ phy->access_delay_jiffies = msecs_to_jiffies(value);
+
+ rc = cr50_of_property_read_u32_optional(dev, "sleep-delay-msec",
+ CR50_DFT_SLEEP_DELAY_MSEC,
+ &value);
+ if (rc < 0)
+ return rc;
+ phy->sleep_delay_jiffies = msecs_to_jiffies(value);
+
+ rc = cr50_of_property_read_u32_optional(dev, "wake-start-delay-msec",
+ CR50_DFT_WAKE_START_DELAY_MSEC,
+ &value);
+ if (rc < 0)
+ return rc;
+ phy->wake_start_delay_msec = value;
+
+ mutex_init(&phy->time_track_mutex);
+ phy->wake_after_jiffies = jiffies;
+ phy->last_access_jiffies = jiffies;
+
+ rc = tpm_tis_core_init(&dev->dev, &phy->priv, -1, &cr50_spi_phy_ops,
+ NULL);
+ if (rc < 0)
+ return rc;
+
+ cr50_get_fw_version(&phy->priv, fw_ver);
+ dev_info(&dev->dev, "Cr50 firmware version: %s\n", fw_ver);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cr50_pm, tpm_pm_suspend, tpm_tis_resume);
+
+static int cr50_spi_remove(struct spi_device *dev)
+{
+ struct tpm_chip *chip = spi_get_drvdata(dev);
+
+ tpm_chip_unregister(chip);
+ tpm_tis_remove(chip);
+ return 0;
+}
+
+static const struct spi_device_id cr50_spi_id[] = {
+ {"cr50_spi", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(spi, cr50_spi_id);
+
+static const struct of_device_id of_cr50_spi_match[] = {
+ { .compatible = "google,cr50_spi", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_cr50_spi_match);
+
+static struct spi_driver cr50_spi_driver = {
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = "cr50_spi",
+ .pm = &cr50_pm,
+ .of_match_table = of_match_ptr(of_cr50_spi_match),
+ },
+ .probe = cr50_spi_probe,
+ .remove = cr50_spi_remove,
+ .id_table = cr50_spi_id,
+};
+module_spi_driver(cr50_spi_driver);
+
+MODULE_DESCRIPTION("Cr50 TCG PTP FIFO SPI driver");
+MODULE_LICENSE("GPL v2");
--
2.6.6
Jason Gunthorpe
2016-07-15 03:32:55 UTC
Permalink
Post by Andrey Pronin
+static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+{
+ int rc;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
+ if (!rc)
+ *result = le16_to_cpu(*result);
+ return rc;
+}
I thought we had core support for this pattern?

Christophe ?

Please change this so this code isn't duplicated.

Jason
Andrey Pronin
2016-07-15 03:44:55 UTC
Permalink
Post by Jason Gunthorpe
Post by Andrey Pronin
+static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+{
+ int rc;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
+ if (!rc)
+ *result = le16_to_cpu(*result);
+ return rc;
+}
I thought we had core support for this pattern?
Christophe ?
Please change this so this code isn't duplicated.
Jason
Hmm, didn't see the support. Would be great if there is.
The pattern itself is copied from tpm_tis_spi as is.
read_bytes/write_bytes were de-dup'ed as they used a lot of common code
(even more for this driver than for tpm_tis_spi).
But as for _readNN/_writeNN, there're only three of these functions,
so it din't seem too bad.
Jarkko Sakkinen
2016-07-19 12:55:39 UTC
Permalink
Post by Andrey Pronin
Post by Jason Gunthorpe
Post by Andrey Pronin
+static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+{
+ int rc;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
+ if (!rc)
+ *result = le16_to_cpu(*result);
+ return rc;
+}
I thought we had core support for this pattern?
Christophe ?
Please change this so this code isn't duplicated.
Jason
Hmm, didn't see the support. Would be great if there is.
The pattern itself is copied from tpm_tis_spi as is.
read_bytes/write_bytes were de-dup'ed as they used a lot of common code
(even more for this driver than for tpm_tis_spi).
But as for _readNN/_writeNN, there're only three of these functions,
so it din't seem too bad.
If there isn't, please add a separate commit before this that adds an
inline function to tpm_tis_core.h.

/Jarkko
Andrey Pronin
2016-07-20 00:24:43 UTC
Permalink
Post by Jarkko Sakkinen
Post by Andrey Pronin
Post by Jason Gunthorpe
Post by Andrey Pronin
+static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+{
+ int rc;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
+ if (!rc)
+ *result = le16_to_cpu(*result);
+ return rc;
+}
I thought we had core support for this pattern?
Christophe ?
Please change this so this code isn't duplicated.
Jason
Hmm, didn't see the support. Would be great if there is.
The pattern itself is copied from tpm_tis_spi as is.
read_bytes/write_bytes were de-dup'ed as they used a lot of common code
(even more for this driver than for tpm_tis_spi).
But as for _readNN/_writeNN, there're only three of these functions,
so it din't seem too bad.
If there isn't, please add a separate commit before this that adds an
inline function to tpm_tis_core.h.
tpm_tis_core.h currently has access functions defined like:
static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
return data->phy_ops->read16(data, addr, result);
}

And it's up to the drivers implementing phy_ops to do (or not)
byte-swapping as necessary before/after IO ops. For example,
tpm_tis.c in its phy_ops->read16 simply does ioread16(), while
tpm_tis_spi.c (and cr50_spi.c) does phy_ops->read_bytes()
followed by le16_to_cpu().

Still, I can create add'l inline functions:
static inline int tpm_tis_read_le16(struct tpm_tis_data *data,
u32 addr, u16 *result)
{
int rc;

rc = data->phy_ops->read_bytes(data, addr,
sizeof(u16), (u8 *)result);
if (!rc)
*result = le16_to_cpu(*result);
return rc;
}
and re-use them both in cr50_spi.c and tpm_tis_spi.c

The only two things that bother me with such approach are
(1) whatever names I pick for the new set of functions, they
will be similar to and thus might be confused with the
original tpm_tis_read/writeXX;
(2) these functions are phy-specific, so possibly it's better
to create tpm_tis_spi.h and put them there with proper
name prefixes. And then use in tpm_tis_spi and cr50_spi.

Any preferences on what I should better do?

Andrey
Jason Gunthorpe
2016-07-20 17:03:56 UTC
Permalink
Post by Andrey Pronin
The only two things that bother me with such approach are
(1) whatever names I pick for the new set of functions, they
will be similar to and thus might be confused with the
original tpm_tis_read/writeXX;
tpm_tis_helper_read16 ?
Post by Andrey Pronin
(2) these functions are phy-specific, so possibly it's better
to create tpm_tis_spi.h and put them there with proper
name prefixes. And then use in tpm_tis_spi and cr50_spi.
No, they are generic to any tis phy that implements read only through
read_bytes.

(Honestly, I'm not sure we made the best choice here having phy
functions for all the versions, we are not that performance
sensitive, just getting rid of everything but read_bytes from the
phy_ops would probably also be a reasonable thing to do.)

Jason
Andrey Pronin
2016-07-21 18:10:58 UTC
Permalink
Post by Jason Gunthorpe
Post by Andrey Pronin
The only two things that bother me with such approach are
(1) whatever names I pick for the new set of functions, they
will be similar to and thus might be confused with the
original tpm_tis_read/writeXX;
tpm_tis_helper_read16 ?
Post by Andrey Pronin
(2) these functions are phy-specific, so possibly it's better
to create tpm_tis_spi.h and put them there with proper
name prefixes. And then use in tpm_tis_spi and cr50_spi.
No, they are generic to any tis phy that implements read only through
read_bytes.
(Honestly, I'm not sure we made the best choice here having phy
functions for all the versions, we are not that performance
sensitive, just getting rid of everything but read_bytes from the
phy_ops would probably also be a reasonable thing to do.)
One thing we can do is re-implement functions tpm_tis_read/writeXX
to use phy-specific implementations of read16, read32, write32 if they
are provided. But if those function pointers are left NULL in phy_ops,
fallback to using read/write_bytes and byte-swapping.

I.e., instead of:

static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
return data->phy_ops->read16(data, addr, result);
}

do the following:

static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
int rc;

if (data->phy_ops->read16)
return data->phy_ops->read16(data, addr, result);

rc = data->phy_ops->read_bytes(data, addr,
sizeof(u16), (u8 *)result);
if (!rc)
*result = le16_to_cpu(*result);
return rc;
}

If you like the idea, I'll submit it as a separate patch.

Andrey
Jason Gunthorpe
2016-07-21 21:00:51 UTC
Permalink
Post by Andrey Pronin
Post by Jason Gunthorpe
Post by Andrey Pronin
The only two things that bother me with such approach are
(1) whatever names I pick for the new set of functions, they
will be similar to and thus might be confused with the
original tpm_tis_read/writeXX;
tpm_tis_helper_read16 ?
Post by Andrey Pronin
(2) these functions are phy-specific, so possibly it's better
to create tpm_tis_spi.h and put them there with proper
name prefixes. And then use in tpm_tis_spi and cr50_spi.
No, they are generic to any tis phy that implements read only through
read_bytes.
(Honestly, I'm not sure we made the best choice here having phy
functions for all the versions, we are not that performance
sensitive, just getting rid of everything but read_bytes from the
phy_ops would probably also be a reasonable thing to do.)
One thing we can do is re-implement functions tpm_tis_read/writeXX
to use phy-specific implementations of read16, read32, write32 if they
are provided. But if those function pointers are left NULL in phy_ops,
fallback to using read/write_bytes and byte-swapping.
I was thinking of just getting rid of phy_ops->read16 entirely and
only use read_bytes at the ops layer.

Jason
Peter Huewe
2016-07-15 02:29:38 UTC
Permalink
Post by Andrey Pronin
This patchset adds a TCG TPM2.0 PTP FIFO compliant interface for
Cr50 chip on SPI.
Depends on the following patches by Andrey Pronin
- tpm: support driver-specific sysfs attrs in tpm_tis_core
- tpm_tis_core: add optional max xfer size check
tpm: devicetree: document properties for cr50
tpm: add driver for cr50 on SPI
.../devicetree/bindings/security/tpm/cr50_spi.txt | 30 ++
drivers/char/tpm/Kconfig | 9 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/cr50_spi.c | 409
+++++++++++++++++++++
4 files changed, 449 insertions(+)
create mode 100644
Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
create mode 100644 drivers/char/tpm/cr50_spi.c
Hi,
can you explain a bit more about this device? And why it needs a special driver and cannot be handled by tpm_tis_spi if its tcg compliant?

Peter
--
Sent from my mobile
Andrey Pronin
2016-07-15 02:50:36 UTC
Permalink
Post by Peter Huewe
Post by Andrey Pronin
This patchset adds a TCG TPM2.0 PTP FIFO compliant interface for
Cr50 chip on SPI.
Depends on the following patches by Andrey Pronin
- tpm: support driver-specific sysfs attrs in tpm_tis_core
- tpm_tis_core: add optional max xfer size check
tpm: devicetree: document properties for cr50
tpm: add driver for cr50 on SPI
.../devicetree/bindings/security/tpm/cr50_spi.txt | 30 ++
drivers/char/tpm/Kconfig | 9 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/cr50_spi.c | 409
+++++++++++++++++++++
4 files changed, 449 insertions(+)
create mode 100644
Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
create mode 100644 drivers/char/tpm/cr50_spi.c
Hi,
can you explain a bit more about this device? And why it needs a special driver and cannot be handled by tpm_tis_spi if its tcg compliant?
Peter
--
Sent from my mobile
Hi Peter,

Yes, it has a TCG-compliant interface, however, there are several things
specific to this device:
- need to ensure a certain delay between spi transactions, or else
the chip can miss several first bytes.
- if there is no spi activity for this chip, it may go to sleep, and
needs to be waken up before sending further commands.
- it has some vendor-specific registers accessible from spi bus.

All that combined to me seemed to be enough justification to add a
device-specific driver rather than adding vendor-specific code to
tpm_tis_spi in multiple places.

Plus, where it seemed appropriate, I added additional hooks to
tpm_tis_core (device-specific sysfs attributes, capping burstcnt
in case of chip error) to support this chip specifics.

Best regards,
Andrey
Jason Gunthorpe
2016-07-15 03:28:26 UTC
Permalink
Post by Andrey Pronin
Yes, it has a TCG-compliant interface, however, there are several things
- need to ensure a certain delay between spi transactions, or else
the chip can miss several first bytes.
- if there is no spi activity for this chip, it may go to sleep, and
needs to be waken up before sending further commands.
- it has some vendor-specific registers accessible from spi bus.
This all needs to be commented clearly in the driver..

The chip sounds pretty broken...

Jason
Andrey Pronin
2016-07-15 17:16:09 UTC
Permalink
Post by Jason Gunthorpe
Post by Andrey Pronin
Yes, it has a TCG-compliant interface, however, there are several things
- need to ensure a certain delay between spi transactions, or else
the chip can miss several first bytes.
- if there is no spi activity for this chip, it may go to sleep, and
needs to be waken up before sending further commands.
- it has some vendor-specific registers accessible from spi bus.
This all needs to be commented clearly in the driver..
Will provide more details in the patch description. The driver code
has comments about those cases in the code: see cr50_ensure_access_delay
and cr50_needs_waking, for example.
Jarkko Sakkinen
2016-07-19 12:56:42 UTC
Permalink
Post by Andrey Pronin
This patchset adds a TCG TPM2.0 PTP FIFO compliant interface for
Cr50 chip on SPI.
- tpm: support driver-specific sysfs attrs in tpm_tis_core
- tpm_tis_core: add optional max xfer size check
tpm: devicetree: document properties for cr50
tpm: add driver for cr50 on SPI
I wonder who coul peer test this? I do not possess this hardware.

/Jarkko
Rob Herring
2016-07-20 19:03:16 UTC
Permalink
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware. Several timing-related properties that may differ from
one firmware version to another are added to devicetree.
Document these properties.
---
.../devicetree/bindings/security/tpm/cr50_spi.txt | 32 ++++++++++++++++++++++
1 file changed, 32 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
diff --git a/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
new file mode 100644
index 0000000..f212b6b
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
@@ -0,0 +1,32 @@
+* H1 Secure Microcontroller with Cr50 Firmware on SPI Bus.
+
+H1 Secure Microcontroller running Cr50 firmware provides several
+functions, including TPM-like functionality. It communicates over
+SPI using the FIFO protocol described in the PTP Spec, section 6.
+
+- compatible: Should be "google,cr50".
+- spi-max-frequency: Maximum SPI frequency.
+
+- access-delay-ms: Required delay between subsequent transactions on SPI.
As I mentioned, there may be common properties. It doesn't seem you
looked, so I did:

- spi-rx-delay-us - (optional) Microsecond delay after a read transfer.
- spi-tx-delay-us - (optional) Microsecond delay after a write transfer.

Seems to me setting one or both of these should work for you.
+- sleep-delay-ms: Time after the last SPI activity, after which the chip
+ may go to sleep.
+- wake-start-delay-ms: Time after initiating wake up before the chip is
+ ready to accept commands over SPI.
I also asked why these 2 can't be hard-coded in the driver?
+
+
+&spi0 {
+ status = "okay";
+
+ compatible = "google,cr50";
+ reg = <0>;
+ spi-max-frequency = <800000>;
+
+ access-delay-ms = <2>;
+ sleep-delay-ms = <1000>;
+ wake-start-delay-ms = <60>;
+ };
+};
--
2.6.6
Andrey Pronin
2016-07-20 19:49:22 UTC
Permalink
Hi Rob,
Post by Rob Herring
As I mentioned, there may be common properties. It doesn't seem you
- spi-rx-delay-us - (optional) Microsecond delay after a read transfer.
- spi-tx-delay-us - (optional) Microsecond delay after a write transfer.
Seems to me setting one or both of these should work for you.
Yes, good catch, my fault I didn't see those.
But they are not exactly what I mean and need. I don't need delay after
each read or write transfer. What is needed is a guaranteed time
between transfers.

So, if the next transaction doesn't come withing the next X ms (or us),
we don't waste time on inserting a delays after this transaction at all.
Following the description and always inserting a delay must work well
for short microseconds-long delays. For longer milliseconds-long delays
a different strategy of checking the time when the previous transaction
was and only delaying if it was not too long ago is better.

Thus, I won't be able to re-use these properties anyways based on their
current description in bindings/spi/spi-bus.txt.
Post by Rob Herring
+- sleep-delay-ms: Time after the last SPI activity, after which the chip
+ may go to sleep.
+- wake-start-delay-ms: Time after initiating wake up before the chip is
+ ready to accept commands over SPI.
I also asked why these 2 can't be hard-coded in the driver?
Sorry, I just updated this patch description in v2 to indicate why they are not
hard-coded, but didn't answer explicitly. As the firmware changes, a different
revision of it can have a different time before it sleeps in its configuration,
or the time it takes it to startup may be different. Thus, there's a way to
set it here w/o changing the driver.

Andrey
Jason Gunthorpe
2016-07-20 19:54:56 UTC
Permalink
Post by Andrey Pronin
Sorry, I just updated this patch description in v2 to indicate why they are not
hard-coded, but didn't answer explicitly. As the firmware changes, a different
revision of it can have a different time before it sleeps in its configuration,
or the time it takes it to startup may be different. Thus, there's a way to
set it here w/o changing the driver.
This sort of stuff should be read out of the firmware, not DT..

Why has Google created such a non-standard TPM firmware???

Jason
Andrey Pronin
2016-07-27 21:02:42 UTC
Permalink
Post by Jason Gunthorpe
Post by Andrey Pronin
Sorry, I just updated this patch description in v2 to indicate why they are not
hard-coded, but didn't answer explicitly. As the firmware changes, a different
revision of it can have a different time before it sleeps in its configuration,
or the time it takes it to startup may be different. Thus, there's a way to
set it here w/o changing the driver.
This sort of stuff should be read out of the firmware, not DT..
Why has Google created such a non-standard TPM firmware???
Jason
Thanks, Jason. Will hard-code those in the driver instead of reading
from DT.

Andrey
Rob Herring
2016-07-21 21:03:21 UTC
Permalink
Post by Andrey Pronin
Hi Rob,
Post by Rob Herring
As I mentioned, there may be common properties. It doesn't seem you
- spi-rx-delay-us - (optional) Microsecond delay after a read transfer.
- spi-tx-delay-us - (optional) Microsecond delay after a write transfer.
Seems to me setting one or both of these should work for you.
Yes, good catch, my fault I didn't see those.
But they are not exactly what I mean and need. I don't need delay after
each read or write transfer. What is needed is a guaranteed time
between transfers.
So, if the next transaction doesn't come withing the next X ms (or us),
we don't waste time on inserting a delays after this transaction at all.
Following the description and always inserting a delay must work well
for short microseconds-long delays. For longer milliseconds-long delays
a different strategy of checking the time when the previous transaction
was and only delaying if it was not too long ago is better.
I'd guess that the intent is the same for all. A simple delay is
just much easier to implement. I would think implementing the more
sophisticated algorithm would work for all users. Perhaps with some
threshold for a simple delay.
Post by Andrey Pronin
Thus, I won't be able to re-use these properties anyways based on their
current description in bindings/spi/spi-bus.txt.
Post by Rob Herring
+- sleep-delay-ms: Time after the last SPI activity, after which the chip
+ may go to sleep.
+- wake-start-delay-ms: Time after initiating wake up before the chip is
+ ready to accept commands over SPI.
I also asked why these 2 can't be hard-coded in the driver?
Sorry, I just updated this patch description in v2 to indicate why they are not
hard-coded, but didn't answer explicitly. As the firmware changes, a different
revision of it can have a different time before it sleeps in its configuration,
or the time it takes it to startup may be different. Thus, there's a way to
set it here w/o changing the driver.
The firmware and DT may not be updated in sync especially if you are
loading the firmware from the rootfs. Are you doing DT and firmware
updates without changing the kernel?

Rob
Andrey Pronin
2016-07-27 21:00:59 UTC
Permalink
Post by Rob Herring
Post by Andrey Pronin
Hi Rob,
Post by Rob Herring
As I mentioned, there may be common properties. It doesn't seem you
- spi-rx-delay-us - (optional) Microsecond delay after a read transfer.
- spi-tx-delay-us - (optional) Microsecond delay after a write transfer.
Seems to me setting one or both of these should work for you.
Yes, good catch, my fault I didn't see those.
But they are not exactly what I mean and need. I don't need delay after
each read or write transfer. What is needed is a guaranteed time
between transfers.
So, if the next transaction doesn't come withing the next X ms (or us),
we don't waste time on inserting a delays after this transaction at all.
Following the description and always inserting a delay must work well
for short microseconds-long delays. For longer milliseconds-long delays
a different strategy of checking the time when the previous transaction
was and only delaying if it was not too long ago is better.
I'd guess that the intent is the same for all. A simple delay is
just much easier to implement. I would think implementing the more
sophisticated algorithm would work for all users. Perhaps with some
threshold for a simple delay.
Post by Andrey Pronin
Thus, I won't be able to re-use these properties anyways based on their
current description in bindings/spi/spi-bus.txt.
Post by Rob Herring
+- sleep-delay-ms: Time after the last SPI activity, after which the chip
+ may go to sleep.
+- wake-start-delay-ms: Time after initiating wake up before the chip is
+ ready to accept commands over SPI.
I also asked why these 2 can't be hard-coded in the driver?
Sorry, I just updated this patch description in v2 to indicate why they are not
hard-coded, but didn't answer explicitly. As the firmware changes, a different
revision of it can have a different time before it sleeps in its configuration,
or the time it takes it to startup may be different. Thus, there's a way to
set it here w/o changing the driver.
The firmware and DT may not be updated in sync especially if you are
loading the firmware from the rootfs. Are you doing DT and firmware
updates without changing the kernel?
Rob
Hi Rob,

Thanks for the feedback. I will hard-code those parameters in the
driver instead of reading from DT.

Thanks,
Andrey
Peter Huewe
2016-07-25 17:09:56 UTC
Permalink
Hi Andrey,

thanks for the update.
This patchset adds support for H1 Secure Microcontroller running
Cr50 firmware. It implements several functions, including TPM-like
functionality, and communicates over SPI using the FIFO protocol
described in the PTP Spec, section 6.
H1 is a proprietary chip that the Chrome OS team is investigating
for inclusion in future Chromebooks.
so is this "broken" device already in the field? (i.e. can I buy it? how many of them)
from the description it seems not. ("future chromebooks")
--> how likely is it that the firmware of that device will be fixed that it can work with the regular driver?

Although I really like to see driver upstreamed, I also want to avoid maintenance hell for obscure hardware :/

Thanks,
Peter
Andrey Pronin
2016-07-27 21:13:22 UTC
Permalink
Hi Peter,
Post by Peter Huewe
This patchset adds support for H1 Secure Microcontroller running
Cr50 firmware. It implements several functions, including TPM-like
functionality, and communicates over SPI using the FIFO protocol
described in the PTP Spec, section 6.
H1 is a proprietary chip that the Chrome OS team is investigating
for inclusion in future Chromebooks.
so is this "broken" device already in the field? (i.e. can I buy it? how many of them)
from the description it seems not. ("future chromebooks")
You're right the device is not in the field yet. I'm sending this driver
upstream before the hardware is publicly available, so people can start
using it when the devices are available. And I've gathered quite a lot
of useful feedback already.
Post by Peter Huewe
--> how likely is it that the firmware of that device will be fixed that it can work with the regular driver?
It's hard to tell what will change in the future versions of firmware. I
expect that at least some specifics will stay.
Post by Peter Huewe
Although I really like to see driver upstreamed, I also want to avoid maintenance hell for obscure hardware :/
I understand, but the idea was to support the hardware that is not
available yet, but will be in the future.

I'll be sending the new versions of the cr50 driver patches shortly.

Thanks,
Andrey
Dmitry Torokhov
2016-07-28 23:01:52 UTC
Permalink
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware. The firmware running on the currently supported H1
Secure Microcontroller requires a special driver to handle its
- need to ensure a certain delay between spi transactions, or else
the chip may miss some part of the next transaction;
- if there is no spi activity for some time, it may go to sleep,
and needs to be waken up before sending further commands;
- access to vendor-specific registers.
---
drivers/char/tpm/Kconfig | 9 ++
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/cr50_spi.c | 350 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 360 insertions(+)
create mode 100644 drivers/char/tpm/cr50_spi.c
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 9faa0b1..3320acc 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -100,6 +100,15 @@ config TCG_ATMEL
will be accessible from within Linux. To compile this driver
as a module, choose M here; the module will be called tpm_atmel.
+config TCG_CR50_SPI
+ tristate "TCG PTP FIFO Interface over SPI - Chips with Cr50 Firmware"
+ depends on SPI
+ select TCG_TIS_CORE
+ ---help---
+ If you have a chip running Cr50 firmware on SPI bus, say Yes and it
+ will be accessible from within Linux. To compile this driver as a
+ module, choose M here; the module will be called cr50_spi.
+
config TCG_INFINEON
tristate "Infineon Technologies TPM Interface"
depends on PNP
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a385fb8..b346306 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
+obj-$(CONFIG_TCG_CR50_SPI) += cr50_spi.o
obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c
new file mode 100644
index 0000000..57ba8b3
--- /dev/null
+++ b/drivers/char/tpm/cr50_spi.c
@@ -0,0 +1,350 @@
+/*
+ * Copyright (C) 2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2, as published by
+ * the Free Software Foundation.
+ *
+ * This device driver implements a TCG PTP FIFO interface over SPI for chips
+ * with Cr50 firmware.
+ * It is based on tpm_tis_spi driver by Peter Huewe and Christophe Ricard.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/wait.h>
+#include <linux/of.h>
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/tpm.h>
+#include "tpm.h"
+#include "tpm_tis_core.h"
+
+/*
+ * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC
+ * - needs up to CR50_WAKE_START_DELAY_MSEC to wake after sleep
+ * - requires at least CR50_ACCESS_DELAY_MSEC between transactions
+ */
+#define CR50_SLEEP_DELAY_MSEC 1000
+#define CR50_WAKE_START_DELAY_MSEC 60
+#define CR50_ACCESS_DELAY_MSEC 2
+
+#define MAX_SPI_FRAMESIZE 64
+
+#define TPM_CR50_FW_VER(l) (0x0F90 | ((l) << 12))
+#define TPM_CR50_MAX_FW_VER_LEN 64
Misaligned?
+
+struct cr50_spi_phy {
+ struct tpm_tis_data priv;
+ struct spi_device *spi_device;
+
+ struct mutex time_track_mutex;
+ unsigned long last_access_jiffies;
+ unsigned long wake_after_jiffies;
+
+ unsigned long access_delay_jiffies;
+ unsigned long sleep_delay_jiffies;
+ unsigned int wake_start_delay_msec;
+
+ u8 tx_buf[MAX_SPI_FRAMESIZE];
+ u8 rx_buf[MAX_SPI_FRAMESIZE];
Both of these need to be annotated as "____cacheline_aligned" since we
eye them for use in DMA transfers.
+};
+
+static struct cr50_spi_phy *to_cr50_spi_phy(struct tpm_tis_data *data)
+{
+ return container_of(data, struct cr50_spi_phy, priv);
+}
+
+/*
+ * Cr50 needs to have at least some delay between consecutive
+ * transactions. Make sure we wait.
+ */
+static void cr50_ensure_access_delay(struct cr50_spi_phy *phy)
+{
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll have an unneeded short delay,
+ * which is fine.
+ */
+ unsigned long allowed_access =
+ phy->last_access_jiffies + phy->access_delay_jiffies;
+ unsigned long time_now = jiffies;
+
+ if (time_in_range_open(time_now,
+ phy->last_access_jiffies, allowed_access))
+ mdelay(jiffies_to_msecs(allowed_access - time_now));
+}
+
+/*
+ * Cr50 might go to sleep if there is no SPI activity for some time and
+ * miss the first few bits/bytes on the bus. In such case, wake it up
+ * by asserting CS and give it time to start up.
+ */
+static bool cr50_needs_waking(struct cr50_spi_phy *phy)
+{
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll probably timeout or read
+ * incorrect value from TPM_STS and just retry the operation.
+ */
+ return !time_in_range_open(jiffies,
+ phy->last_access_jiffies,
+ phy->wake_after_jiffies);
+}
+
+static void cr50_wake_if_needed(struct cr50_spi_phy *phy)
+{
+ if (cr50_needs_waking(phy)) {
+ /* Assert CS, wait 1 msec, deassert CS */
+ struct spi_transfer spi_cs_wake = { .delay_usecs = 1000 };
+
+ spi_sync_transfer(phy->spi_device, &spi_cs_wake, 1);
+ /* Wait for it to fully wake */
+ msleep(phy->wake_start_delay_msec);
+ }
+ /* Reset the time when we need to wake Cr50 again */
+ phy->wake_after_jiffies = jiffies + phy->sleep_delay_jiffies;
+}
+
+/*
+ * Flow control: clock the bus and wait for cr50 to set LSB before
+ * sending/receiving data. TCG PTP spec allows it to happen during
+ * the last byte of header, but cr50 never does that in practice,
+ * and earlier versions had a bug when it was set too early, so don't
+ * check for it during header transfer.
+ */
+static int cr50_spi_flow_control(struct cr50_spi_phy *phy)
+{
+ unsigned long timeout_jiffies =
+ jiffies + msecs_to_jiffies(TPM_RETRY * TPM_TIMEOUT_RETRY);
+ struct spi_message m;
+ int ret;
+ struct spi_transfer spi_xfer = {
+ .rx_buf = phy->rx_buf,
+ .len = 1,
+ .cs_change = 1,
+ };
+
+ do {
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ return ret;
+ if (time_after(jiffies, timeout_jiffies))
+ return -EBUSY;
+ } while (!(phy->rx_buf[0] & 0x01));
+ return 0;
+}
+
+static int cr50_spi_xfer_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *buf, bool do_write)
+{
+ struct cr50_spi_phy *phy = to_cr50_spi_phy(data);
+ struct spi_message m;
+ struct spi_transfer spi_xfer = {
+ .tx_buf = phy->tx_buf,
+ .rx_buf = phy->rx_buf,
+ .len = 4,
+ .cs_change = 1,
+ };
+ int ret;
+
+ if (len > MAX_SPI_FRAMESIZE)
+ return -EINVAL;
+
+ /*
+ * Do this outside of spi_bus_lock in case cr50 is not the
+ * only device on that spi bus.
+ */
+ mutex_lock(&phy->time_track_mutex);
+ cr50_ensure_access_delay(phy);
+ cr50_wake_if_needed(phy);
+
+ phy->tx_buf[0] = (do_write ? 0x00 : 0x80) | (len - 1);
+ phy->tx_buf[1] = 0xD4;
+ phy->tx_buf[2] = (addr >> 8) & 0xFF;
+ phy->tx_buf[3] = addr & 0xFF;
+
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+
+ spi_bus_lock(phy->spi_device->master);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ goto exit;
+
+ ret = cr50_spi_flow_control(phy);
+ if (ret < 0)
+ goto exit;
+
+ spi_xfer.cs_change = 0;
+ spi_xfer.len = len;
+ if (do_write) {
+ memcpy(phy->tx_buf, buf, len);
+ spi_xfer.rx_buf = NULL;
+ } else {
+ spi_xfer.tx_buf = NULL;
+ }
+
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (!do_write)
+ memcpy(buf, phy->rx_buf, len);
+
+ spi_bus_unlock(phy->spi_device->master);
+ phy->last_access_jiffies = jiffies;
+ mutex_unlock(&phy->time_track_mutex);
+
+ return ret;
+}
+
+static int cr50_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result)
+{
+ return cr50_spi_xfer_bytes(data, addr, len, result, false);
+}
+
+static int cr50_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *value)
+{
+ return cr50_spi_xfer_bytes(data, addr, len, value, true);
+}
+
+static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+{
+ int rc;
+ __le16 le_val;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)&le_val);
+ if (!rc)
+ *result = le16_to_cpu(le_val);
+ return rc;
+}
+
+static int cr50_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
+{
+ int rc;
+ __le32 le_val;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u32), (u8 *)&le_val);
+ if (!rc)
+ *result = le32_to_cpu(le_val);
+ return rc;
+}
+
+static int cr50_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
+{
+ __le32 le_val = cpu_to_le32(value);
+
+ return data->phy_ops->write_bytes(data, addr, sizeof(u32),
+ (u8 *)&le_val);
+}
+
+static void cr50_get_fw_version(struct tpm_tis_data *data, char *fw_ver)
+{
+ int i, len = 0;
+ char fw_ver_block[4];
+
+ /*
+ * Write anything to TPM_CR50_FW_VER to start from the beginning
+ * of the version string
+ */
+ tpm_tis_write8(data, TPM_CR50_FW_VER(data->locality), 0);
+
+ /* Read the string, 4 bytes at a time, until we get '\0' */
+ do {
+ tpm_tis_read_bytes(data, TPM_CR50_FW_VER(data->locality), 4,
+ fw_ver_block);
+ for (i = 0; i < 4 && fw_ver_block[i]; ++len, ++i)
+ fw_ver[len] = fw_ver_block[i];
+ } while (i == 4 && len < TPM_CR50_MAX_FW_VER_LEN);
+ fw_ver[len] = 0;
+}
+
+static const struct tpm_tis_phy_ops cr50_spi_phy_ops = {
+ .read_bytes = cr50_spi_read_bytes,
+ .write_bytes = cr50_spi_write_bytes,
+ .read16 = cr50_spi_read16,
+ .read32 = cr50_spi_read32,
+ .write32 = cr50_spi_write32,
+ .max_xfer_size = MAX_SPI_FRAMESIZE,
+};
+
+static int cr50_spi_probe(struct spi_device *dev)
+{
+ char fw_ver[TPM_CR50_MAX_FW_VER_LEN + 1];
+ struct cr50_spi_phy *phy;
+ int rc;
+
+ phy = devm_kzalloc(&dev->dev, sizeof(struct cr50_spi_phy),
+ GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ phy->spi_device = dev;
+
+ phy->access_delay_jiffies = CR50_ACCESS_DELAY_MSEC;
+ phy->sleep_delay_jiffies = CR50_SLEEP_DELAY_MSEC;
+ phy->wake_start_delay_msec = CR50_WAKE_START_DELAY_MSEC;
+
+ mutex_init(&phy->time_track_mutex);
+ phy->wake_after_jiffies = jiffies;
+ phy->last_access_jiffies = jiffies;
+
+ rc = tpm_tis_core_init(&dev->dev, &phy->priv, -1, &cr50_spi_phy_ops,
+ NULL);
+ if (rc < 0)
+ return rc;
+
+ cr50_get_fw_version(&phy->priv, fw_ver);
+ dev_info(&dev->dev, "Cr50 firmware version: %s\n", fw_ver);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cr50_pm, tpm_pm_suspend, tpm_tis_resume);
+
+static int cr50_spi_remove(struct spi_device *dev)
+{
+ struct tpm_chip *chip = spi_get_drvdata(dev);
+
+ tpm_chip_unregister(chip);
+ tpm_tis_remove(chip);
+ return 0;
+}
+
+static const struct spi_device_id cr50_spi_id[] = {
+ { "cr50", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(spi, cr50_spi_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_cr50_spi_match[] = {
+ { .compatible = "google,cr50", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_cr50_spi_match);
+#endif
+
+static struct spi_driver cr50_spi_driver = {
+ .driver = {
+ .name = "cr50_spi",
+ .pm = &cr50_pm,
+ .of_match_table = of_match_ptr(of_cr50_spi_match),
+ },
+ .probe = cr50_spi_probe,
+ .remove = cr50_spi_remove,
+ .id_table = cr50_spi_id,
+};
+module_spi_driver(cr50_spi_driver);
+
+MODULE_DESCRIPTION("Cr50 TCG PTP FIFO SPI driver");
+MODULE_LICENSE("GPL v2");
--
2.6.6
Thanks.
--
Dmitry
Jason Gunthorpe
2016-07-28 23:17:37 UTC
Permalink
Post by Dmitry Torokhov
+ u8 tx_buf[MAX_SPI_FRAMESIZE];
+ u8 rx_buf[MAX_SPI_FRAMESIZE];
Both of these need to be annotated as "____cacheline_aligned" since we
eye them for use in DMA transfers.
Huh. That sure looks true to me..

We make that same mistake in all other tpm SPI drivers.

Can someone send a patch please?

Thanks,
Jason
Andrey Pronin
2016-07-29 03:01:14 UTC
Permalink
Post by Jason Gunthorpe
Post by Dmitry Torokhov
+ u8 tx_buf[MAX_SPI_FRAMESIZE];
+ u8 rx_buf[MAX_SPI_FRAMESIZE];
Both of these need to be annotated as "____cacheline_aligned" since we
eye them for use in DMA transfers.
Huh. That sure looks true to me..
We make that same mistake in all other tpm SPI drivers.
Can someone send a patch please?
Just did.

Thanks,
Andrey
Jason Gunthorpe
2016-07-29 17:28:15 UTC
Permalink
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware.
Since this is now a trivial device, does it still need a dedicated
file?

Jason
Rob Herring
2016-07-29 21:42:18 UTC
Permalink
Post by Jason Gunthorpe
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware.
Since this is now a trivial device, does it still need a dedicated
file?
There is no trivial devices file for SPI, only I2C. We could add one,
but this is fine as is for me.

Acked-by: Rob Herring <***@kernel.org>
Jarkko Sakkinen
2016-08-09 10:08:41 UTC
Permalink
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware.
Acked-by: Jarkko Sakkinen <***@linux.intel.com>

/Jarkko
---
.../devicetree/bindings/security/tpm/cr50_spi.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
diff --git a/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
new file mode 100644
index 0000000..2fbebd3
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
@@ -0,0 +1,21 @@
+* H1 Secure Microcontroller with Cr50 Firmware on SPI Bus.
+
+H1 Secure Microcontroller running Cr50 firmware provides several
+functions, including TPM-like functionality. It communicates over
+SPI using the FIFO protocol described in the PTP Spec, section 6.
+
+- compatible: Should be "google,cr50".
+- spi-max-frequency: Maximum SPI frequency.
+
+
+&spi0 {
+ status = "okay";
+
+ compatible = "google,cr50";
+ reg = <0>;
+ spi-max-frequency = <800000>;
+ };
+};
--
2.6.6
Jarkko Sakkinen
2016-08-09 10:20:43 UTC
Permalink
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware. The firmware running on the currently supported H1
Secure Microcontroller requires a special driver to handle its
- need to ensure a certain delay between spi transactions, or else
the chip may miss some part of the next transaction;
- if there is no spi activity for some time, it may go to sleep,
and needs to be waken up before sending further commands;
- access to vendor-specific registers.
---
drivers/char/tpm/Kconfig | 9 ++
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/cr50_spi.c | 350 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 360 insertions(+)
create mode 100644 drivers/char/tpm/cr50_spi.c
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 9faa0b1..3320acc 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -100,6 +100,15 @@ config TCG_ATMEL
will be accessible from within Linux. To compile this driver
as a module, choose M here; the module will be called tpm_atmel.
+config TCG_CR50_SPI
+ tristate "TCG PTP FIFO Interface over SPI - Chips with Cr50 Firmware"
+ depends on SPI
+ select TCG_TIS_CORE
+ ---help---
+ If you have a chip running Cr50 firmware on SPI bus, say Yes and it
+ will be accessible from within Linux. To compile this driver as a
+ module, choose M here; the module will be called cr50_spi.
+
config TCG_INFINEON
tristate "Infineon Technologies TPM Interface"
depends on PNP
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a385fb8..b346306 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
+obj-$(CONFIG_TCG_CR50_SPI) += cr50_spi.o
obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c
new file mode 100644
index 0000000..9cc1620
--- /dev/null
+++ b/drivers/char/tpm/cr50_spi.c
@@ -0,0 +1,350 @@
+/*
+ * Copyright (C) 2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2, as published by
+ * the Free Software Foundation.
+ *
+ * This device driver implements a TCG PTP FIFO interface over SPI for chips
+ * with Cr50 firmware.
+ * It is based on tpm_tis_spi driver by Peter Huewe and Christophe Ricard.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/wait.h>
+#include <linux/of.h>
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/tpm.h>
+#include "tpm.h"
+#include "tpm_tis_core.h"
+
+/*
+ * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC
+ * - needs up to CR50_WAKE_START_DELAY_MSEC to wake after sleep
+ * - requires at least CR50_ACCESS_DELAY_MSEC between transactions
+ */
+#define CR50_SLEEP_DELAY_MSEC 1000
+#define CR50_WAKE_START_DELAY_MSEC 60
+#define CR50_ACCESS_DELAY_MSEC 2
+
+#define MAX_SPI_FRAMESIZE 64
+
+#define TPM_CR50_FW_VER(l) (0x0F90 | ((l) << 12))
+#define TPM_CR50_MAX_FW_VER_LEN 64
+
+struct cr50_spi_phy {
+ struct tpm_tis_data priv;
+ struct spi_device *spi_device;
+
+ struct mutex time_track_mutex;
+ unsigned long last_access_jiffies;
+ unsigned long wake_after_jiffies;
+
+ unsigned long access_delay_jiffies;
+ unsigned long sleep_delay_jiffies;
+ unsigned int wake_start_delay_msec;
+
+ u8 tx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
+ u8 rx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
+};
+
+static struct cr50_spi_phy *to_cr50_spi_phy(struct tpm_tis_data *data)
+{
+ return container_of(data, struct cr50_spi_phy, priv);
+}
+
+/*
+ * Cr50 needs to have at least some delay between consecutive
+ * transactions. Make sure we wait.
+ */
"How to format kernel-doc comments" [1]

[1] https://www.kernel.org/doc/Documentation/kernel-documentation.rst

/Jarkko
+static void cr50_ensure_access_delay(struct cr50_spi_phy *phy)
+{
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll have an unneeded short delay,
+ * which is fine.
+ */
+ unsigned long allowed_access =
+ phy->last_access_jiffies + phy->access_delay_jiffies;
+ unsigned long time_now = jiffies;
+
+ if (time_in_range_open(time_now,
+ phy->last_access_jiffies, allowed_access))
+ mdelay(jiffies_to_msecs(allowed_access - time_now));
+}
+
+/*
+ * Cr50 might go to sleep if there is no SPI activity for some time and
+ * miss the first few bits/bytes on the bus. In such case, wake it up
+ * by asserting CS and give it time to start up.
+ */
+static bool cr50_needs_waking(struct cr50_spi_phy *phy)
+{
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll probably timeout or read
+ * incorrect value from TPM_STS and just retry the operation.
+ */
+ return !time_in_range_open(jiffies,
+ phy->last_access_jiffies,
+ phy->wake_after_jiffies);
+}
+
+static void cr50_wake_if_needed(struct cr50_spi_phy *phy)
+{
+ if (cr50_needs_waking(phy)) {
+ /* Assert CS, wait 1 msec, deassert CS */
+ struct spi_transfer spi_cs_wake = { .delay_usecs = 1000 };
+
+ spi_sync_transfer(phy->spi_device, &spi_cs_wake, 1);
+ /* Wait for it to fully wake */
+ msleep(phy->wake_start_delay_msec);
+ }
+ /* Reset the time when we need to wake Cr50 again */
+ phy->wake_after_jiffies = jiffies + phy->sleep_delay_jiffies;
+}
+
+/*
+ * Flow control: clock the bus and wait for cr50 to set LSB before
+ * sending/receiving data. TCG PTP spec allows it to happen during
+ * the last byte of header, but cr50 never does that in practice,
+ * and earlier versions had a bug when it was set too early, so don't
+ * check for it during header transfer.
+ */
+static int cr50_spi_flow_control(struct cr50_spi_phy *phy)
+{
+ unsigned long timeout_jiffies =
+ jiffies + msecs_to_jiffies(TPM_RETRY * TPM_TIMEOUT_RETRY);
+ struct spi_message m;
+ int ret;
+ struct spi_transfer spi_xfer = {
+ .rx_buf = phy->rx_buf,
+ .len = 1,
+ .cs_change = 1,
+ };
+
+ do {
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ return ret;
+ if (time_after(jiffies, timeout_jiffies))
+ return -EBUSY;
+ } while (!(phy->rx_buf[0] & 0x01));
+ return 0;
+}
+
+static int cr50_spi_xfer_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *buf, bool do_write)
+{
+ struct cr50_spi_phy *phy = to_cr50_spi_phy(data);
+ struct spi_message m;
+ struct spi_transfer spi_xfer = {
+ .tx_buf = phy->tx_buf,
+ .rx_buf = phy->rx_buf,
+ .len = 4,
+ .cs_change = 1,
+ };
+ int ret;
+
+ if (len > MAX_SPI_FRAMESIZE)
+ return -EINVAL;
+
+ /*
+ * Do this outside of spi_bus_lock in case cr50 is not the
+ * only device on that spi bus.
+ */
+ mutex_lock(&phy->time_track_mutex);
+ cr50_ensure_access_delay(phy);
+ cr50_wake_if_needed(phy);
+
+ phy->tx_buf[0] = (do_write ? 0x00 : 0x80) | (len - 1);
+ phy->tx_buf[1] = 0xD4;
+ phy->tx_buf[2] = (addr >> 8) & 0xFF;
+ phy->tx_buf[3] = addr & 0xFF;
+
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+
+ spi_bus_lock(phy->spi_device->master);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ goto exit;
+
+ ret = cr50_spi_flow_control(phy);
+ if (ret < 0)
+ goto exit;
+
+ spi_xfer.cs_change = 0;
+ spi_xfer.len = len;
+ if (do_write) {
+ memcpy(phy->tx_buf, buf, len);
+ spi_xfer.rx_buf = NULL;
+ } else {
+ spi_xfer.tx_buf = NULL;
+ }
+
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (!do_write)
+ memcpy(buf, phy->rx_buf, len);
+
+ spi_bus_unlock(phy->spi_device->master);
+ phy->last_access_jiffies = jiffies;
+ mutex_unlock(&phy->time_track_mutex);
+
+ return ret;
+}
+
+static int cr50_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result)
+{
+ return cr50_spi_xfer_bytes(data, addr, len, result, false);
+}
+
+static int cr50_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *value)
+{
+ return cr50_spi_xfer_bytes(data, addr, len, value, true);
+}
+
+static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+{
+ int rc;
+ __le16 le_val;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)&le_val);
+ if (!rc)
+ *result = le16_to_cpu(le_val);
+ return rc;
+}
+
+static int cr50_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
+{
+ int rc;
+ __le32 le_val;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u32), (u8 *)&le_val);
+ if (!rc)
+ *result = le32_to_cpu(le_val);
+ return rc;
+}
+
+static int cr50_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
+{
+ __le32 le_val = cpu_to_le32(value);
+
+ return data->phy_ops->write_bytes(data, addr, sizeof(u32),
+ (u8 *)&le_val);
+}
+
+static void cr50_get_fw_version(struct tpm_tis_data *data, char *fw_ver)
+{
+ int i, len = 0;
+ char fw_ver_block[4];
+
+ /*
+ * Write anything to TPM_CR50_FW_VER to start from the beginning
+ * of the version string
+ */
+ tpm_tis_write8(data, TPM_CR50_FW_VER(data->locality), 0);
+
+ /* Read the string, 4 bytes at a time, until we get '\0' */
+ do {
+ tpm_tis_read_bytes(data, TPM_CR50_FW_VER(data->locality), 4,
+ fw_ver_block);
+ for (i = 0; i < 4 && fw_ver_block[i]; ++len, ++i)
+ fw_ver[len] = fw_ver_block[i];
+ } while (i == 4 && len < TPM_CR50_MAX_FW_VER_LEN);
+ fw_ver[len] = 0;
+}
+
+static const struct tpm_tis_phy_ops cr50_spi_phy_ops = {
+ .read_bytes = cr50_spi_read_bytes,
+ .write_bytes = cr50_spi_write_bytes,
+ .read16 = cr50_spi_read16,
+ .read32 = cr50_spi_read32,
+ .write32 = cr50_spi_write32,
+ .max_xfer_size = MAX_SPI_FRAMESIZE,
+};
+
+static int cr50_spi_probe(struct spi_device *dev)
+{
+ char fw_ver[TPM_CR50_MAX_FW_VER_LEN + 1];
+ struct cr50_spi_phy *phy;
+ int rc;
+
+ phy = devm_kzalloc(&dev->dev, sizeof(struct cr50_spi_phy),
+ GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ phy->spi_device = dev;
+
+ phy->access_delay_jiffies = CR50_ACCESS_DELAY_MSEC;
+ phy->sleep_delay_jiffies = CR50_SLEEP_DELAY_MSEC;
+ phy->wake_start_delay_msec = CR50_WAKE_START_DELAY_MSEC;
+
+ mutex_init(&phy->time_track_mutex);
+ phy->wake_after_jiffies = jiffies;
+ phy->last_access_jiffies = jiffies;
+
+ rc = tpm_tis_core_init(&dev->dev, &phy->priv, -1, &cr50_spi_phy_ops,
+ NULL);
+ if (rc < 0)
+ return rc;
+
+ cr50_get_fw_version(&phy->priv, fw_ver);
+ dev_info(&dev->dev, "Cr50 firmware version: %s\n", fw_ver);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cr50_pm, tpm_pm_suspend, tpm_tis_resume);
+
+static int cr50_spi_remove(struct spi_device *dev)
+{
+ struct tpm_chip *chip = spi_get_drvdata(dev);
+
+ tpm_chip_unregister(chip);
+ tpm_tis_remove(chip);
+ return 0;
+}
+
+static const struct spi_device_id cr50_spi_id[] = {
+ { "cr50", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(spi, cr50_spi_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_cr50_spi_match[] = {
+ { .compatible = "google,cr50", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_cr50_spi_match);
+#endif
+
+static struct spi_driver cr50_spi_driver = {
+ .driver = {
+ .name = "cr50_spi",
+ .pm = &cr50_pm,
+ .of_match_table = of_match_ptr(of_cr50_spi_match),
+ },
+ .probe = cr50_spi_probe,
+ .remove = cr50_spi_remove,
+ .id_table = cr50_spi_id,
+};
+module_spi_driver(cr50_spi_driver);
+
+MODULE_DESCRIPTION("Cr50 TCG PTP FIFO SPI driver");
+MODULE_LICENSE("GPL v2");
--
2.6.6
b***@chromium.org
2018-06-05 17:08:39 UTC
Permalink
[Hopefully this gets formatted correctly; I wasn't copied on the original
email, so I tried to get a copy from not-my-inbox]

Hi Jarkko, Andrey,
Post by Jarkko Sakkinen
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware. The firmware running on the currently supported H1
Secure Microcontroller requires a special driver to handle its
- need to ensure a certain delay between spi transactions, or else
the chip may miss some part of the next transaction;
- if there is no spi activity for some time, it may go to sleep,
and needs to be waken up before sending further commands;
- access to vendor-specific registers.
---
drivers/char/tpm/Kconfig | 9 ++
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/cr50_spi.c | 350 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 360 insertions(+)
create mode 100644 drivers/char/tpm/cr50_spi.c
...
Post by Jarkko Sakkinen
diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c
new file mode 100644
index 0000000..9cc1620
--- /dev/null
+++ b/drivers/char/tpm/cr50_spi.c
@@ -0,0 +1,350 @@
...
Post by Jarkko Sakkinen
+/*
+ * Cr50 needs to have at least some delay between consecutive
+ * transactions. Make sure we wait.
+ */
"How to format kernel-doc comments" [1]
[1] https://www.kernel.org/doc/Documentation/kernel-documentation.rst
It looks like it's intentionally *not* a kernel-doc comment. There's
not really much need for that, is there?

Regardless: is this the only outstanding comment on this series?
Any reason we can't just get a v5 and be done with this? There are a
lot of platforms out there starting to use this hardware, and it would
be nice to have a kernel driver for it...

Regards,
Brian

Loading...