[PATCH 0/2] tpm: add driver for cr50 on SPI

[PATCH 0/2] tpm: add driver for cr50 on SPI

[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 <apronin@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

[PATCH 1/2] tpm: devicetree: document properties for cr50

[Andrey Pronin]

Add TPM2.0-compatible interface to Cr50. Document its properties
in devicetree.

Signed-off-by: Andrey Pronin <apronin@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";
+
+        cr50@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

[PATCH 2/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

Add TCG TPM2.0 PTP FIFO compatible interface for Cr50 chip on SPI bus.

Signed-off-by: Andrey Pronin <apronin@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

Re: [PATCH 0/2] tpm: add driver for cr50 on SPI

[Peter Huewe]

Am 14. Juli 2016 19:20:16 GMT-07:00, schrieb Andrey Pronin <apronin@chromium.org>:
>This patchset adds a TCG TPM2.0 PTP FIFO compliant interface for
>Cr50 chip on SPI.
>
>Depends on the following patches by Andrey Pronin
><apronin@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


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

Re: [PATCH 0/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

On Thu, Jul 14, 2016 at 07:28:55PM -0700, Peter Huewe wrote:
> Am 14. Juli 2016 19:20:16 GMT-07:00, schrieb Andrey Pronin <apronin@chromium.org>:
> >This patchset adds a TCG TPM2.0 PTP FIFO compliant interface for
> >Cr50 chip on SPI.
> >
> >Depends on the following patches by Andrey Pronin
> ><apronin@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
> 
> 
> 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

Re: [PATCH 0/2] tpm: add driver for cr50 on SPI

[Jason Gunthorpe]

On Thu, Jul 14, 2016 at 07:50:26PM -0700, Andrey Pronin wrote:
> 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.

This all needs to be commented clearly in the driver..

The chip sounds pretty broken...

Jason

Re: [PATCH 2/2] tpm: add driver for cr50 on SPI

[Jason Gunthorpe]

On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:

> +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

Re: [PATCH 2/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

On Thu, Jul 14, 2016 at 09:32:36PM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
> 
> > +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.

Re: [PATCH 1/2] tpm: devicetree: document properties for cr50

[Guenter Roeck]

On Thu, Jul 14, 2016 at 7:20 PM, Andrey Pronin <apronin@chromium.org> wrote:
> Add TPM2.0-compatible interface to Cr50. Document its properties
> in devicetree.
>
> Signed-off-by: Andrey Pronin <apronin@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".

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

> +- 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";
> +
> +        cr50@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
>

Re: [PATCH 0/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

On Thu, Jul 14, 2016 at 09:28:14PM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 14, 2016 at 07:50:26PM -0700, Andrey Pronin wrote:
> > 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.
> 
> 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.

Re: [PATCH 1/2] tpm: devicetree: document properties for cr50

[Andrey Pronin]

On Thu, Jul 14, 2016 at 09:05:53PM -0700, Guenter Roeck wrote:
> On Thu, Jul 14, 2016 at 7:20 PM, Andrey Pronin <apronin@chromium.org> wrote:
> > +
> > +Required properties:
> > +- 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.

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

Re: [PATCH 1/2] tpm: devicetree: document properties for cr50

[Guenter Roeck]

On Fri, Jul 15, 2016 at 10:31 AM, Andrey Pronin <apronin@chromium.org> wrote:
> On Thu, Jul 14, 2016 at 09:05:53PM -0700, Guenter Roeck wrote:
>> On Thu, Jul 14, 2016 at 7:20 PM, Andrey Pronin <apronin@chromium.org> wrote:
>> > +
>> > +Required properties:
>> > +- 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

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

Re: [PATCH 1/2] tpm: devicetree: document properties for cr50

[Rob Herring]

On Thu, Jul 14, 2016 at 07:20:17PM -0700, Andrey Pronin wrote:
> Add TPM2.0-compatible interface to Cr50. Document its properties
> in devicetree.
> 
> Signed-off-by: Andrey Pronin <apronin@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".

I agree with dropping '_spi'. The interface is defined by the parent 
device.

> +- spi-max-frequency: Maximum SPI frequency.
> +
> +Optional properties:
> +- access-delay-msec: Required delay between subsequent transactions on SPI.

There may be a standard property for this...

> +- 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?

Re: [PATCH 2/2] tpm: add driver for cr50 on SPI

[Jarkko Sakkinen]

On Thu, Jul 14, 2016 at 08:44:44PM -0700, Andrey Pronin wrote:
> On Thu, Jul 14, 2016 at 09:32:36PM -0600, Jason Gunthorpe wrote:
> > On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
> > 
> > > +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

Re: [PATCH 0/2] tpm: add driver for cr50 on SPI

[Jarkko Sakkinen]

On Thu, Jul 14, 2016 at 07:20:16PM -0700, Andrey Pronin wrote:
> This patchset adds a TCG TPM2.0 PTP FIFO compliant interface for
> Cr50 chip on SPI.
> 
> Depends on the following patches by Andrey Pronin <apronin@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

I wonder who coul peer test this? I do not possess this hardware.

/Jarkko

Re: [PATCH 2/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

On Tue, Jul 19, 2016 at 03:55:27PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 14, 2016 at 08:44:44PM -0700, Andrey Pronin wrote:
> > On Thu, Jul 14, 2016 at 09:32:36PM -0600, Jason Gunthorpe wrote:
> > > On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
> > > 
> > > > +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

[PATCH v2 0/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

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.

Depends on the following patchset:
 - tpm_tis_core: add optional max xfer size check

v2: Removed driver-specific sysfs attributes.
    Compatible id changed to cr50 from cr50_spi.
    Updated descriptions of the supported device/interface.

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

 .../devicetree/bindings/security/tpm/cr50_spi.txt  |  32 ++
 drivers/char/tpm/Kconfig                           |   9 +
 drivers/char/tpm/Makefile                          |   1 +
 drivers/char/tpm/cr50_spi.c                        | 388 +++++++++++++++++++++
 4 files changed, 430 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
 create mode 100644 drivers/char/tpm/cr50_spi.c

-- 
2.6.6

[PATCH v2 1/2] tpm: devicetree: document properties for cr50

[Andrey Pronin]

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.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 .../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.
+
+Required properties:
+- compatible: Should be "google,cr50".
+- spi-max-frequency: Maximum SPI frequency.
+
+Optional properties:
+- access-delay-ms: Required delay between subsequent transactions on SPI.
+- 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.
+
+Example:
+
+&spi0 {
+        status = "okay";
+
+        cr50@0 {
+                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

[PATCH v2 2/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

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
specifics:
 - 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.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 drivers/char/tpm/Kconfig    |   9 +
 drivers/char/tpm/Makefile   |   1 +
 drivers/char/tpm/cr50_spi.c | 388 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 398 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..be10d75
--- /dev/null
+++ b/drivers/char/tpm/cr50_spi.c
@@ -0,0 +1,388 @@
+/*
+ * 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 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_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-ms",
+						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-ms",
+						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-ms",
+						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", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, cr50_spi_id);
+
+static const struct of_device_id of_cr50_spi_match[] = {
+	{ .compatible = "google,cr50", },
+	{}
+};
+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

Re: [PATCH 2/2] tpm: add driver for cr50 on SPI

[Jason Gunthorpe]

On Tue, Jul 19, 2016 at 05:24:11PM -0700, Andrey Pronin wrote:

> 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 ?

> (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

Re: [PATCH v2 1/2] tpm: devicetree: document properties for cr50

[Rob Herring]

On Tue, Jul 19, 2016 at 08:41:24PM -0700, Andrey Pronin wrote:
> 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.
> 
> Signed-off-by: Andrey Pronin <apronin@chromium.org>
> ---
>  .../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.
> +
> +Required properties:
> +- compatible: Should be "google,cr50".
> +- spi-max-frequency: Maximum SPI frequency.
> +
> +Optional properties:
> +- 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?

> +
> +Example:
> +
> +&spi0 {
> +        status = "okay";
> +
> +        cr50@0 {
> +                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
> 

Re: [PATCH v2 1/2] tpm: devicetree: document properties for cr50

[Andrey Pronin]

On Wed, Jul 20, 2016 at 02:03:03PM -0500, Rob Herring wrote:
> On Tue, Jul 19, 2016 at 08:41:24PM -0700, Andrey Pronin wrote:

Hi Rob,

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

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.

> > +- 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

Re: [PATCH v2 1/2] tpm: devicetree: document properties for cr50

[Jason Gunthorpe]

On Wed, Jul 20, 2016 at 12:49:12PM -0700, Andrey Pronin wrote:

> 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

Re: [PATCH 2/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

On Wed, Jul 20, 2016 at 11:03:36AM -0600, Jason Gunthorpe wrote:
> On Tue, Jul 19, 2016 at 05:24:11PM -0700, Andrey Pronin wrote:
> 
> > 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 ?
> 
> > (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

Re: [PATCH 2/2] tpm: add driver for cr50 on SPI

[Jason Gunthorpe]

On Thu, Jul 21, 2016 at 11:10:47AM -0700, Andrey Pronin wrote:
> On Wed, Jul 20, 2016 at 11:03:36AM -0600, Jason Gunthorpe wrote:
> > On Tue, Jul 19, 2016 at 05:24:11PM -0700, Andrey Pronin wrote:
> > 
> > > 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 ?
> > 
> > > (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

Re: [PATCH v2 1/2] tpm: devicetree: document properties for cr50

[Rob Herring]

On Wed, Jul 20, 2016 at 12:49:12PM -0700, Andrey Pronin wrote:
> On Wed, Jul 20, 2016 at 02:03:03PM -0500, Rob Herring wrote:
> > On Tue, Jul 19, 2016 at 08:41:24PM -0700, Andrey Pronin wrote:
> 
> Hi Rob,
> 
> > 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.
> >
> 
> 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.

> Thus, I won't be able to re-use these properties anyways based on their
> current description in bindings/spi/spi-bus.txt.
> 
> > > +- 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

Aw: [PATCH v2 0/2] tpm: add driver for cr50 on SPI

[Peter Huewe]

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

Re: [PATCH v2 1/2] tpm: devicetree: document properties for cr50

[Andrey Pronin]

On Thu, Jul 21, 2016 at 04:03:12PM -0500, Rob Herring wrote:
> On Wed, Jul 20, 2016 at 12:49:12PM -0700, Andrey Pronin wrote:
> > On Wed, Jul 20, 2016 at 02:03:03PM -0500, Rob Herring wrote:
> > > On Tue, Jul 19, 2016 at 08:41:24PM -0700, Andrey Pronin wrote:
> > 
> > Hi Rob,
> > 
> > > 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.
> > >
> > 
> > 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.
> 
> > Thus, I won't be able to re-use these properties anyways based on their
> > current description in bindings/spi/spi-bus.txt.
> > 
> > > > +- 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

Re: [PATCH v2 1/2] tpm: devicetree: document properties for cr50

[Andrey Pronin]

On Wed, Jul 20, 2016 at 01:54:22PM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 20, 2016 at 12:49:12PM -0700, Andrey Pronin wrote:
> 
> > 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

Re: [PATCH v2 0/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

Hi Peter,

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

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

> 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

[PATCH v3 0/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

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.

Depends on the following patchset:
 - tpm_tis_core: add optional max xfer size check

v2: Removed driver-specific sysfs attributes.
    Compatible id changed to cr50 from cr50_spi.
    Updated descriptions of the supported device/interface.

v3: Fixed potential race-condition with last_access_jiffies.
    Started using tx_buf/rx_buf in cr50_spi_phy to avoid
    potential problems with DMA.
    Removed DT properties for fw timing parameters.
    Fixed style.

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

 .../devicetree/bindings/security/tpm/cr50_spi.txt  |  21 ++
 drivers/char/tpm/Kconfig                           |   9 +
 drivers/char/tpm/Makefile                          |   1 +
 drivers/char/tpm/cr50_spi.c                        | 350 +++++++++++++++++++++
 4 files changed, 381 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
 create mode 100644 drivers/char/tpm/cr50_spi.c

-- 
2.6.6

[PATCH v3 1/2] tpm: devicetree: document properties for cr50

[Andrey Pronin]

Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 .../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.
+
+Required properties:
+- compatible: Should be "google,cr50".
+- spi-max-frequency: Maximum SPI frequency.
+
+Example:
+
+&spi0 {
+        status = "okay";
+
+        cr50@0 {
+                compatible = "google,cr50";
+                reg = <0>;
+                spi-max-frequency = <800000>;
+        };
+};
-- 
2.6.6

[PATCH v3 2/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

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
specifics:
 - 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.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 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"
+
+/*
+ * Cr50 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_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];
+	u8 rx_buf[MAX_SPI_FRAMESIZE];
+};
+
+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);
+
+exit:
+	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

Re: [PATCH v3 2/2] tpm: add driver for cr50 on SPI

[Dmitry Torokhov]

On Wed, Jul 27, 2016 at 09:25:17PM -0700, Andrey Pronin wrote:
> 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
> specifics:
>  - 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.
> 
> Signed-off-by: Andrey Pronin <apronin@chromium.org>
> ---
>  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"
> +
> +/*
> + * Cr50 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_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);
> +
> +exit:
> +	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

Re: [PATCH v3 2/2] tpm: add driver for cr50 on SPI

[Jason Gunthorpe]

On Thu, Jul 28, 2016 at 04:01:41PM -0700, Dmitry Torokhov wrote:

> > +	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

[PATCH v4 0/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

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.

Depends on the following patchset:
 - tpm_tis_core: add optional max xfer size check

v2: Removed driver-specific sysfs attributes.
    Compatible id changed to cr50 from cr50_spi.
    Updated descriptions of the supported device/interface.

v3: Fixed potential race-condition with last_access_jiffies.
    Started using tx_buf/rx_buf in cr50_spi_phy to avoid
    potential problems with DMA.
    Removed DT properties for fw timing parameters.
    Fixed style.

v4: Fixed cacheline alignment for xfer buffers.

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

 .../devicetree/bindings/security/tpm/cr50_spi.txt  |  21 ++
 drivers/char/tpm/Kconfig                           |   9 +
 drivers/char/tpm/Makefile                          |   1 +
 drivers/char/tpm/cr50_spi.c                        | 350 +++++++++++++++++++++
 4 files changed, 381 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/cr50_spi.txt
 create mode 100644 drivers/char/tpm/cr50_spi.c

-- 
2.6.6

[PATCH v4 1/2] tpm: devicetree: document properties for cr50

[Andrey Pronin]

Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 .../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.
+
+Required properties:
+- compatible: Should be "google,cr50".
+- spi-max-frequency: Maximum SPI frequency.
+
+Example:
+
+&spi0 {
+        status = "okay";
+
+        cr50@0 {
+                compatible = "google,cr50";
+                reg = <0>;
+                spi-max-frequency = <800000>;
+        };
+};
-- 
2.6.6

[PATCH v4 2/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

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
specifics:
 - 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.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 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"
+
+/*
+ * Cr50 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_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.
+ */
+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);
+
+exit:
+	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

Re: [PATCH v3 2/2] tpm: add driver for cr50 on SPI

[Andrey Pronin]

On Thu, Jul 28, 2016 at 05:17:06PM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 28, 2016 at 04:01:41PM -0700, Dmitry Torokhov wrote:
> 
> > > +	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

Re: [PATCH v4 1/2] tpm: devicetree: document properties for cr50

[Jason Gunthorpe]

On Thu, Jul 28, 2016 at 06:55:13PM -0700, Andrey Pronin wrote:
> 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

Re: [PATCH v4 1/2] tpm: devicetree: document properties for cr50

[Rob Herring]

On Fri, Jul 29, 2016 at 11:27:52AM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 28, 2016 at 06:55:13PM -0700, Andrey Pronin wrote:
> > 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 <robh@kernel.org>

Re: [PATCH v4 1/2] tpm: devicetree: document properties for cr50

[Jarkko Sakkinen]

On Thu, Jul 28, 2016 at 06:55:13PM -0700, Andrey Pronin wrote:
> Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
> firmware.
> 
> Signed-off-by: Andrey Pronin <apronin@chromium.org>

Acked-by: Jarkko Sakkinen <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.
> +
> +Required properties:
> +- compatible: Should be "google,cr50".
> +- spi-max-frequency: Maximum SPI frequency.
> +
> +Example:
> +
> +&spi0 {
> +        status = "okay";
> +
> +        cr50@0 {
> +                compatible = "google,cr50";
> +                reg = <0>;
> +                spi-max-frequency = <800000>;
> +        };
> +};
> -- 
> 2.6.6
> 

Re: [PATCH v4 2/2] tpm: add driver for cr50 on SPI

[Jarkko Sakkinen]

On Thu, Jul 28, 2016 at 06:55:14PM -0700, Andrey Pronin wrote:
> 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
> specifics:
>  - 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.
> 
> Signed-off-by: Andrey Pronin <apronin@chromium.org>
> ---
>  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"
> +
> +/*
> + * Cr50 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_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);
> +
> +exit:
> +	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
>