linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] tpm: Add driver for cr50
@ 2019-08-06 22:07 Stephen Boyd
  2019-08-06 22:07 ` [PATCH v3 1/4] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Stephen Boyd @ 2019-08-06 22:07 UTC (permalink / raw)
  To: Peter Huewe, Jarkko Sakkinen
  Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

This patch series adds support for the the H1 secure microcontroller
running cr50 firmware found on various recent Chromebooks. This driver
is necessary to boot into a ChromeOS userspace environment. It
implements support for several functions, including TPM-like
functionality over a SPI interface.

The last time this was series sent looks to be [1]. I've looked over the
patches and review comments and tried to address any feedback that
Andrey didn't address (really minor things like newlines). I've reworked
the patches from the last version to layer on top of the existing TPM
TIS SPI implementation in tpm_tis_spi.c. Hopefully this is more
palatable than combining the two drivers together into one file.

[1] https://lkml.kernel.org/r/1469757314-116169-1-git-send-email-apronin@chromium.org

TODO:
 * Add a patch to spit out WARN_ON() when TPM is suspended and some
   kernel code attempts to use it
 * Rework the i2c driver per Alexander's comments on v2

Changes from v2:
 * Sent khwrng thread patch separately
 * New patch to expose TPM SPI functionality from tpm_tis_spi.c
 * Usage of that new patch in cr50 SPI driver
 * Drop i2c version of cr50 SPI driver for now (will resend later)
 * New patch to add a TPM chip flag indicating TPM shouldn't be reset
   over suspend. Allows us to get rid of the cr50 suspend/resume functions
   that are mostly generic

Changes from v1:
 * Dropped symlink and sysfs patches
 * Removed 'is_suspended' bits
 * Added new patch to freeze khwrng thread
 * Moved binding to google,cr50.txt and added Reviewed-by tag from Rob

Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>

Andrey Pronin (2):
  dt-bindings: tpm: document properties for cr50
  tpm: add driver for cr50 on SPI

Stephen Boyd (2):
  tpm: Add a flag to indicate TPM power is managed by firmware
  tpm: tpm_tis_spi: Export functionality to other drivers

 .../bindings/security/tpm/google,cr50.txt     |  19 +
 drivers/char/tpm/Kconfig                      |   9 +
 drivers/char/tpm/Makefile                     |   1 +
 drivers/char/tpm/cr50_spi.c                   | 373 ++++++++++++++++++
 drivers/char/tpm/tpm-interface.c              |   8 +-
 drivers/char/tpm/tpm.h                        |   1 +
 drivers/char/tpm/tpm_tis_spi.c                |  98 +++--
 drivers/char/tpm/tpm_tis_spi.h                |  37 ++
 8 files changed, 503 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/google,cr50.txt
 create mode 100644 drivers/char/tpm/cr50_spi.c
 create mode 100644 drivers/char/tpm/tpm_tis_spi.h


base-commit: 0ecfebd2b52404ae0c54a878c872bb93363ada36
prerequisite-patch-id: ce0cac49be5e67df1427e4207cf38c6e31091445
-- 
Sent by a computer through tubes


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 1/4] tpm: Add a flag to indicate TPM power is managed by firmware
  2019-08-06 22:07 [PATCH v3 0/4] tpm: Add driver for cr50 Stephen Boyd
@ 2019-08-06 22:07 ` Stephen Boyd
  2019-08-09 18:02   ` Jarkko Sakkinen
  2019-08-06 22:07 ` [PATCH v3 2/4] tpm: tpm_tis_spi: Export functionality to other drivers Stephen Boyd
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2019-08-06 22:07 UTC (permalink / raw)
  To: Peter Huewe, Jarkko Sakkinen
  Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

On some platforms, the TPM power is managed by firmware and therefore we
don't need to stop the TPM on suspend when going to a light version of
suspend such as S0ix ("freeze" suspend state). Add a chip flag to
indicate this so that certain platforms can probe for the usage of this
light suspend and avoid touching the TPM state across suspend/resume.

Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/char/tpm/tpm-interface.c | 8 +++++++-
 drivers/char/tpm/tpm.h           | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1b4f95c13e00..9bf3ddee6c8c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
+#include <linux/suspend.h>
 #include <linux/freezer.h>
 #include <linux/tpm_eventlog.h>
 
@@ -395,7 +396,11 @@ int tpm_pm_suspend(struct device *dev)
 		return -ENODEV;
 
 	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
-		return 0;
+		goto suspended;
+
+	if (chip->flags & TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED)
+		if (!pm_suspend_via_firmware())
+			goto suspended;
 
 	if (!tpm_chip_start(chip)) {
 		if (chip->flags & TPM_CHIP_FLAG_TPM2)
@@ -406,6 +411,7 @@ int tpm_pm_suspend(struct device *dev)
 		tpm_chip_stop(chip);
 	}
 
+suspended:
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_pm_suspend);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e503ffc3aa39..28f73331aa2e 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -162,6 +162,7 @@ enum tpm_chip_flags {
 	TPM_CHIP_FLAG_VIRTUAL		= BIT(3),
 	TPM_CHIP_FLAG_HAVE_TIMEOUTS	= BIT(4),
 	TPM_CHIP_FLAG_ALWAYS_POWERED	= BIT(5),
+	TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED	= BIT(6),
 };
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 2/4] tpm: tpm_tis_spi: Export functionality to other drivers
  2019-08-06 22:07 [PATCH v3 0/4] tpm: Add driver for cr50 Stephen Boyd
  2019-08-06 22:07 ` [PATCH v3 1/4] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd
@ 2019-08-06 22:07 ` Stephen Boyd
  2019-08-09 20:28   ` Jarkko Sakkinen
  2019-08-06 22:07 ` [PATCH v3 3/4] dt-bindings: tpm: document properties for cr50 Stephen Boyd
  2019-08-06 22:07 ` [PATCH v3 4/4] tpm: add driver for cr50 on SPI Stephen Boyd
  3 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2019-08-06 22:07 UTC (permalink / raw)
  To: Peter Huewe, Jarkko Sakkinen
  Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

We want to use most of the code in this driver, except we want to modify
the flow control and idle behavior. Let's "libify" this driver so that
another driver can call the code in here and slightly tweak the
behavior.

Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/char/tpm/tpm_tis_spi.c | 98 +++++++++++++++++++---------------
 drivers/char/tpm/tpm_tis_spi.h | 37 +++++++++++++
 2 files changed, 93 insertions(+), 42 deletions(-)
 create mode 100644 drivers/char/tpm/tpm_tis_spi.h

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 19513e622053..fd3fb4f9f506 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -36,26 +36,43 @@
 #include <linux/tpm.h>
 #include "tpm.h"
 #include "tpm_tis_core.h"
+#include "tpm_tis_spi.h"
 
 #define MAX_SPI_FRAMESIZE 64
 
-struct tpm_tis_spi_phy {
-	struct tpm_tis_data priv;
-	struct spi_device *spi_device;
-	u8 *iobuf;
-};
-
-static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
+static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
+				    struct spi_transfer *spi_xfer)
 {
-	return container_of(data, struct tpm_tis_spi_phy, priv);
+	struct spi_message m;
+	int ret, i;
+
+	if ((phy->iobuf[3] & 0x01) == 0) {
+		// handle SPI wait states
+		phy->iobuf[0] = 0;
+
+		for (i = 0; i < TPM_RETRY; i++) {
+			spi_xfer->len = 1;
+			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 (phy->iobuf[0] & 0x01)
+				break;
+		}
+
+		if (i == TPM_RETRY)
+			return -ETIMEDOUT;
+	}
+
+	return 0;
 }
 
-static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 				u8 *in, const u8 *out)
 {
 	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
 	int ret = 0;
-	int i;
 	struct spi_message m;
 	struct spi_transfer spi_xfer;
 	u8 transfer_len;
@@ -82,26 +99,9 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 		if (ret < 0)
 			goto exit;
 
-		if ((phy->iobuf[3] & 0x01) == 0) {
-			// handle SPI wait states
-			phy->iobuf[0] = 0;
-
-			for (i = 0; i < TPM_RETRY; i++) {
-				spi_xfer.len = 1;
-				spi_message_init(&m);
-				spi_message_add_tail(&spi_xfer, &m);
-				ret = spi_sync_locked(phy->spi_device, &m);
-				if (ret < 0)
-					goto exit;
-				if (phy->iobuf[0] & 0x01)
-					break;
-			}
-
-			if (i == TPM_RETRY) {
-				ret = -ETIMEDOUT;
-				goto exit;
-			}
-		}
+		ret = phy->flow_control(phy, &spi_xfer);
+		if (ret < 0)
+			goto exit;
 
 		spi_xfer.cs_change = 0;
 		spi_xfer.len = transfer_len;
@@ -117,6 +117,8 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 
 		spi_message_init(&m);
 		spi_message_add_tail(&spi_xfer, &m);
+		if (phy->pre_transfer)
+			phy->pre_transfer(phy);
 		ret = spi_sync_locked(phy->spi_device, &m);
 		if (ret < 0)
 			goto exit;
@@ -133,9 +135,10 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 	spi_bus_unlock(phy->spi_device->master);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(tpm_tis_spi_transfer);
 
-static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
-				  u16 len, u8 *result)
+static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
+				  u8 *result)
 {
 	return tpm_tis_spi_transfer(data, addr, len, result, NULL);
 }
@@ -146,7 +149,7 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
 	return tpm_tis_spi_transfer(data, addr, len, NULL, value);
 }
 
-static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
 {
 	__le16 result_le;
 	int rc;
@@ -158,8 +161,9 @@ static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_tis_spi_read16);
 
-static int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
+int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
 {
 	__le32 result_le;
 	int rc;
@@ -171,8 +175,9 @@ static int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_tis_spi_read32);
 
-static int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
+int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
 {
 	__le32 value_le;
 	int rc;
@@ -183,6 +188,20 @@ static int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_tis_spi_write32);
+
+int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
+		     int irq, const struct tpm_tis_phy_ops *phy_ops)
+{
+	phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL);
+	if (!phy->iobuf)
+		return -ENOMEM;
+
+	phy->spi_device = spi;
+
+	return tpm_tis_core_init(&spi->dev, &phy->priv, irq, phy_ops, NULL);
+}
+EXPORT_SYMBOL_GPL(tpm_tis_spi_init);
 
 static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
 	.read_bytes = tpm_tis_spi_read_bytes,
@@ -202,11 +221,7 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
 	if (!phy)
 		return -ENOMEM;
 
-	phy->spi_device = dev;
-
-	phy->iobuf = devm_kmalloc(&dev->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL);
-	if (!phy->iobuf)
-		return -ENOMEM;
+	phy->flow_control = tpm_tis_spi_flow_control;
 
 	/* If the SPI device has an IRQ then use that */
 	if (dev->irq > 0)
@@ -214,8 +229,7 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
 	else
 		irq = -1;
 
-	return tpm_tis_core_init(&dev->dev, &phy->priv, irq, &tpm_spi_phy_ops,
-				 NULL);
+	return tpm_tis_spi_init(dev, phy, irq, &tpm_spi_phy_ops);
 }
 
 static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h
new file mode 100644
index 000000000000..48be5130794a
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_spi.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2015 Infineon Technologies AG
+ * Copyright (C) 2016 STMicroelectronics SAS
+ */
+
+#ifndef TPM_TIS_SPI_H
+#define TPM_TIS_SPI_H
+
+#include "tpm_tis_core.h"
+
+struct tpm_tis_spi_phy {
+	struct tpm_tis_data priv;
+	struct spi_device *spi_device;
+	int (*flow_control)(struct tpm_tis_spi_phy *phy,
+			     struct spi_transfer *xfer);
+	void (*pre_transfer)(struct tpm_tis_spi_phy *phy);
+
+	u8 *iobuf;
+};
+
+static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
+{
+	return container_of(data, struct tpm_tis_spi_phy, priv);
+}
+
+extern int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
+			    int irq, const struct tpm_tis_phy_ops *phy_ops);
+
+extern int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+				u8 *in, const u8 *out);
+
+extern int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result);
+extern int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result);
+extern int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value);
+
+#endif
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 3/4] dt-bindings: tpm: document properties for cr50
  2019-08-06 22:07 [PATCH v3 0/4] tpm: Add driver for cr50 Stephen Boyd
  2019-08-06 22:07 ` [PATCH v3 1/4] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd
  2019-08-06 22:07 ` [PATCH v3 2/4] tpm: tpm_tis_spi: Export functionality to other drivers Stephen Boyd
@ 2019-08-06 22:07 ` Stephen Boyd
  2019-08-06 22:07 ` [PATCH v3 4/4] tpm: add driver for cr50 on SPI Stephen Boyd
  3 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2019-08-06 22:07 UTC (permalink / raw)
  To: Peter Huewe, Jarkko Sakkinen
  Cc: Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen, Rob Herring

From: Andrey Pronin <apronin@chromium.org>

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

Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Andrey Pronin <apronin@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 .../bindings/security/tpm/google,cr50.txt     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/google,cr50.txt

diff --git a/Documentation/devicetree/bindings/security/tpm/google,cr50.txt b/Documentation/devicetree/bindings/security/tpm/google,cr50.txt
new file mode 100644
index 000000000000..7aa65224c8b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/google,cr50.txt
@@ -0,0 +1,19 @@
+* 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 {
+        tpm@0 {
+                compatible = "google,cr50";
+                reg = <0>;
+                spi-max-frequency = <800000>;
+        };
+};
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 4/4] tpm: add driver for cr50 on SPI
  2019-08-06 22:07 [PATCH v3 0/4] tpm: Add driver for cr50 Stephen Boyd
                   ` (2 preceding siblings ...)
  2019-08-06 22:07 ` [PATCH v3 3/4] dt-bindings: tpm: document properties for cr50 Stephen Boyd
@ 2019-08-06 22:07 ` Stephen Boyd
  2019-08-09 20:31   ` Jarkko Sakkinen
  3 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2019-08-06 22:07 UTC (permalink / raw)
  To: Peter Huewe, Jarkko Sakkinen
  Cc: Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

From: Andrey Pronin <apronin@chromium.org>

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>
Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
[swboyd@chromium.org: Replace boilerplate with SPDX tag, drop
suspended bit and remove ifdef checks in cr50.h, push tpm.h
include into cr50.c, migrate to functions exported from tpm_tis_spi.h]
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/char/tpm/Kconfig    |   9 +
 drivers/char/tpm/Makefile   |   1 +
 drivers/char/tpm/cr50_spi.c | 373 ++++++++++++++++++++++++++++++++++++
 3 files changed, 383 insertions(+)
 create mode 100644 drivers/char/tpm/cr50_spi.c

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 88a3c06fc153..1fb23f145ef6 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -114,6 +114,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 "Cr50 SPI Interface"
+	depends on TCG_TIS_SPI
+	---help---
+	  If you have a H1 secure module 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 a01c4cab902a..4713e52ce1b0 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -27,6 +27,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 000000000000..c65ac16d22b6
--- /dev/null
+++ b/drivers/char/tpm/cr50_spi.c
@@ -0,0 +1,373 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2016 Google, Inc
+ *
+ * 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/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/spi/spi.h>
+#include <linux/wait.h>
+
+#include "tpm_tis_core.h"
+#include "tpm_tis_spi.h"
+
+/*
+ * Cr50 timing constants:
+ * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC.
+ * - needs up to CR50_WAKE_START_DELAY_USEC to wake after sleep.
+ * - requires waiting for "ready" IRQ, if supported; or waiting for at least
+ *   CR50_NOIRQ_ACCESS_DELAY_MSEC between transactions, if IRQ is not supported.
+ * - waits for up to CR50_FLOW_CONTROL for flow control 'ready' indication.
+ */
+#define CR50_SLEEP_DELAY_MSEC			1000
+#define CR50_WAKE_START_DELAY_USEC		1000
+#define CR50_NOIRQ_ACCESS_DELAY			msecs_to_jiffies(2)
+#define CR50_READY_IRQ_TIMEOUT			msecs_to_jiffies(TPM2_TIMEOUT_A)
+#define CR50_FLOW_CONTROL			msecs_to_jiffies(TPM2_TIMEOUT_A)
+#define MAX_IRQ_CONFIRMATION_ATTEMPTS		3
+
+#define TPM_CR50_FW_VER(l)			(0x0f90 | ((l) << 12))
+#define TPM_CR50_MAX_FW_VER_LEN			64
+#define TIS_IS_CR50				1
+
+struct cr50_spi_phy {
+	struct tpm_tis_spi_phy spi_phy;
+
+	struct mutex time_track_mutex;
+	unsigned long last_access;
+	unsigned long wake_after;
+
+	unsigned long access_delay;
+
+	struct completion ready;
+
+	unsigned int irq_confirmation_attempt;
+	bool irq_needs_confirmation;
+	bool irq_confirmed;
+};
+
+static inline struct cr50_spi_phy *to_cr50_spi_phy(struct tpm_tis_spi_phy *phy)
+{
+	return container_of(phy, struct cr50_spi_phy, spi_phy);
+}
+
+/*
+ * The cr50 interrupt handler just signals waiting threads that the
+ * interrupt was asserted.  It does not do any processing triggered
+ * by interrupts but is instead used to avoid fixed delays.
+ */
+static irqreturn_t cr50_spi_irq_handler(int dummy, void *dev_id)
+{
+	struct cr50_spi_phy *cr50_phy = dev_id;
+
+	cr50_phy->irq_confirmed = true;
+	complete(&cr50_phy->ready);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * 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)
+{
+	unsigned long allowed_access = phy->last_access + phy->access_delay;
+	unsigned long time_now = jiffies;
+	struct device *dev = &phy->spi_phy.spi_device->dev;
+
+	/*
+	 * 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.
+	 */
+	if (time_in_range_open(time_now, phy->last_access, allowed_access)) {
+		unsigned long remaining, timeout = allowed_access - time_now;
+
+		remaining = wait_for_completion_timeout(&phy->ready, timeout);
+		if (!remaining && phy->irq_confirmed)
+			dev_warn(dev, "Timeout waiting for TPM ready IRQ\n");
+	}
+
+	if (phy->irq_needs_confirmation) {
+		unsigned int attempt = ++phy->irq_confirmation_attempt;
+
+		if (phy->irq_confirmed) {
+			phy->irq_needs_confirmation = false;
+			phy->access_delay = CR50_READY_IRQ_TIMEOUT;
+			dev_info(dev, "TPM ready IRQ confirmed on attempt %u\n",
+				 attempt);
+		} else if (attempt > MAX_IRQ_CONFIRMATION_ATTEMPTS) {
+			phy->irq_needs_confirmation = false;
+			dev_warn(dev, "IRQ not confirmed - will use delays\n");
+		}
+	}
+}
+
+/*
+ * 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, phy->wake_after);
+}
+
+static void cr50_wake_if_needed(struct cr50_spi_phy *cr50_phy)
+{
+	struct tpm_tis_spi_phy *phy = &cr50_phy->spi_phy;
+
+	if (cr50_needs_waking(cr50_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 */
+		usleep_range(CR50_WAKE_START_DELAY_USEC,
+			     CR50_WAKE_START_DELAY_USEC * 2);
+	}
+
+	/* Reset the time when we need to wake Cr50 again */
+	cr50_phy->wake_after = jiffies + msecs_to_jiffies(CR50_SLEEP_DELAY_MSEC);
+}
+
+/*
+ * 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 tpm_tis_spi_phy *phy,
+				 struct spi_transfer *spi_xfer)
+{
+	struct device *dev = &phy->spi_device->dev;
+	unsigned long timeout = jiffies + CR50_FLOW_CONTROL;
+	struct spi_message m;
+	int ret;
+
+	spi_xfer->len = 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)) {
+			dev_warn(dev, "Timeout during flow control\n");
+			return -EBUSY;
+		}
+	} while (!(phy->iobuf[0] & 0x01));
+
+	return 0;
+}
+
+static void cr50_spi_reinit_completion(struct tpm_tis_spi_phy *phy)
+{
+	struct cr50_spi_phy *cr50_phy = to_cr50_spi_phy(phy);
+	reinit_completion(&cr50_phy->ready);
+}
+
+static int tpm_tis_spi_cr50_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+				     u8 *in, const u8 *out)
+{
+	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
+	struct cr50_spi_phy *cr50_phy = to_cr50_spi_phy(phy);
+	int ret;
+
+	mutex_lock(&cr50_phy->time_track_mutex);
+	/*
+	 * Do this outside of spi_bus_lock in case cr50 is not the
+	 * only device on that spi bus.
+	 */
+	cr50_ensure_access_delay(cr50_phy);
+	cr50_wake_if_needed(cr50_phy);
+
+	ret = tpm_tis_spi_transfer(data, addr, len, in, out);
+
+	cr50_phy->last_access = jiffies;
+	mutex_unlock(&cr50_phy->time_track_mutex);
+
+	return ret;
+}
+
+static int tpm_tis_spi_cr50_read_bytes(struct tpm_tis_data *data, u32 addr,
+				       u16 len, u8 *result)
+{
+	return tpm_tis_spi_cr50_transfer(data, addr, len, result, NULL);
+}
+
+static int tpm_tis_spi_cr50_write_bytes(struct tpm_tis_data *data, u32 addr,
+					u16 len, const u8 *value)
+{
+	return tpm_tis_spi_cr50_transfer(data, addr, len, NULL, value);
+}
+
+static const struct tpm_tis_phy_ops tpm_spi_cr50_phy_ops = {
+	.read_bytes = tpm_tis_spi_cr50_read_bytes,
+	.write_bytes = tpm_tis_spi_cr50_write_bytes,
+	.read16 = tpm_tis_spi_read16,
+	.read32 = tpm_tis_spi_read32,
+	.write32 = tpm_tis_spi_write32,
+};
+
+static void cr50_print_fw_version(struct tpm_tis_data *data)
+{
+	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
+	int i, len = 0;
+	char fw_ver[TPM_CR50_MAX_FW_VER_LEN + 1];
+	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';
+
+	dev_info(&phy->spi_device->dev, "Cr50 firmware version: %s\n", fw_ver);
+}
+
+static int cr50_spi_probe(struct spi_device *spi)
+{
+	struct tpm_tis_spi_phy *phy;
+	struct cr50_spi_phy *cr50_phy;
+	int ret;
+	struct tpm_chip *chip;
+
+	cr50_phy = devm_kzalloc(&spi->dev, sizeof(*cr50_phy), GFP_KERNEL);
+	if (!cr50_phy)
+		return -ENOMEM;
+
+	phy = &cr50_phy->spi_phy;
+	phy->flow_control = cr50_spi_flow_control;
+	phy->pre_transfer = cr50_spi_reinit_completion;
+
+	cr50_phy->access_delay = CR50_NOIRQ_ACCESS_DELAY;
+
+	mutex_init(&cr50_phy->time_track_mutex);
+	cr50_phy->wake_after = jiffies;
+	cr50_phy->last_access = jiffies;
+
+	init_completion(&cr50_phy->ready);
+	if (spi->irq > 0) {
+		ret = devm_request_irq(&spi->dev, spi->irq, cr50_spi_irq_handler,
+				       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+				       "cr50_spi", cr50_phy);
+		if (ret < 0) {
+			if (ret == -EPROBE_DEFER)
+				return ret;
+			dev_warn(&spi->dev, "Requesting IRQ %d failed: %d\n",
+				 spi->irq, ret);
+			/*
+			 * This is not fatal, the driver will fall back to
+			 * delays automatically, since ready will never
+			 * be completed without a registered irq handler.
+			 * So, just fall through.
+			 */
+		} else {
+			/*
+			 * IRQ requested, let's verify that it is actually
+			 * triggered, before relying on it.
+			 */
+			cr50_phy->irq_needs_confirmation = true;
+		}
+	} else {
+		dev_warn(&spi->dev,
+			 "No IRQ - will use delays between transactions.\n");
+	}
+
+	ret = tpm_tis_spi_init(spi, phy, -1, &tpm_spi_cr50_phy_ops);
+	if (ret)
+		return ret;
+
+	cr50_print_fw_version(&phy->priv);
+
+	chip = dev_get_drvdata(&spi->dev);
+	chip->flags |= TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED;
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cr50_spi_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
+	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
+	struct cr50_spi_phy *cr50_phy = to_cr50_spi_phy(phy);
+
+	/*
+	 * Jiffies not increased during suspend, so we need to reset
+	 * the time to wake Cr50 after resume.
+	 */
+	cr50_phy->wake_after = jiffies;
+
+	return tpm_tis_resume(dev);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(cr50_spi_pm, tpm_pm_suspend, cr50_spi_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_spi_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");
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/4] tpm: Add a flag to indicate TPM power is managed by firmware
  2019-08-06 22:07 ` [PATCH v3 1/4] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd
@ 2019-08-09 18:02   ` Jarkko Sakkinen
  2019-08-12 14:11     ` Stephen Boyd
  0 siblings, 1 reply; 12+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 18:02 UTC (permalink / raw)
  To: Stephen Boyd, Peter Huewe
  Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

On Tue, 2019-08-06 at 15:07 -0700, Stephen Boyd wrote:
> On some platforms, the TPM power is managed by firmware and therefore we
> don't need to stop the TPM on suspend when going to a light version of
> suspend such as S0ix ("freeze" suspend state). Add a chip flag to
> indicate this so that certain platforms can probe for the usage of this
> light suspend and avoid touching the TPM state across suspend/resume.

The commit message should mention the new constant.

> +	if (chip->flags & TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED)
> +		if (!pm_suspend_via_firmware())

Why both checks are needed?

If both checks are needed, you could write it as a single
conditional statement:

if (chip->flags & TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED &&
    !pm_suspend_via_firmware())

/Jarkko


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/4] tpm: tpm_tis_spi: Export functionality to other drivers
  2019-08-06 22:07 ` [PATCH v3 2/4] tpm: tpm_tis_spi: Export functionality to other drivers Stephen Boyd
@ 2019-08-09 20:28   ` Jarkko Sakkinen
  2019-08-12 14:16     ` Stephen Boyd
  0 siblings, 1 reply; 12+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 20:28 UTC (permalink / raw)
  To: Stephen Boyd, Peter Huewe
  Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

On Tue, 2019-08-06 at 15:07 -0700, Stephen Boyd wrote:
> We want to use most of the code in this driver, except we want to modify
> the flow control and idle behavior. Let's "libify" this driver so that
> another driver can call the code in here and slightly tweak the
> behavior.

Neither "libifying" nor "slightly tweaking" gives an idea what the
commit does. A great commit message should be in imperative form
describe what it does and why in as plain english as possible.

Often commit messages are seen just as a necessary bad and not much
energy is spent on them but for a maitainer solid commit messages have
an indispensable value.

> +	void (*pre_transfer)(struct tpm_tis_spi_phy *phy);

Adding a new calback should be a commit of its own.

/Jarkko


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 4/4] tpm: add driver for cr50 on SPI
  2019-08-06 22:07 ` [PATCH v3 4/4] tpm: add driver for cr50 on SPI Stephen Boyd
@ 2019-08-09 20:31   ` Jarkko Sakkinen
  2019-08-12 20:46     ` Stephen Boyd
  0 siblings, 1 reply; 12+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 20:31 UTC (permalink / raw)
  To: Stephen Boyd, Peter Huewe
  Cc: Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

On Tue, 2019-08-06 at 15:07 -0700, Stephen Boyd wrote:
> From: Andrey Pronin <apronin@chromium.org>
> 
> 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.

Which Chromebook models have this chip?

If I had an access to one, how do I do kernel testing with it i.e.
how do I get it to boot initramfs and bzImage from a USB stick?

/Jarkko


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/4] tpm: Add a flag to indicate TPM power is managed by firmware
  2019-08-09 18:02   ` Jarkko Sakkinen
@ 2019-08-12 14:11     ` Stephen Boyd
  0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2019-08-12 14:11 UTC (permalink / raw)
  To: Jarkko Sakkinen, Peter Huewe
  Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

Quoting Jarkko Sakkinen (2019-08-09 11:02:01)
> On Tue, 2019-08-06 at 15:07 -0700, Stephen Boyd wrote:
> > On some platforms, the TPM power is managed by firmware and therefore we
> > don't need to stop the TPM on suspend when going to a light version of
> > suspend such as S0ix ("freeze" suspend state). Add a chip flag to
> > indicate this so that certain platforms can probe for the usage of this
> > light suspend and avoid touching the TPM state across suspend/resume.
> 
> The commit message should mention the new constant.

Alright.

> 
> > +     if (chip->flags & TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED)
> > +             if (!pm_suspend_via_firmware())
> 
> Why both checks are needed?
> 
> If both checks are needed, you could write it as a single
> conditional statement:
> 
> if (chip->flags & TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED &&
>     !pm_suspend_via_firmware())
> 

Ok. I'll combine them.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/4] tpm: tpm_tis_spi: Export functionality to other drivers
  2019-08-09 20:28   ` Jarkko Sakkinen
@ 2019-08-12 14:16     ` Stephen Boyd
  0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2019-08-12 14:16 UTC (permalink / raw)
  To: Jarkko Sakkinen, Peter Huewe
  Cc: linux-kernel, linux-integrity, Andrey Pronin, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

Quoting Jarkko Sakkinen (2019-08-09 13:28:13)
> On Tue, 2019-08-06 at 15:07 -0700, Stephen Boyd wrote:
> > We want to use most of the code in this driver, except we want to modify
> > the flow control and idle behavior. Let's "libify" this driver so that
> > another driver can call the code in here and slightly tweak the
> > behavior.
> 
> Neither "libifying" nor "slightly tweaking" gives an idea what the
> commit does. A great commit message should be in imperative form
> describe what it does and why in as plain english as possible.
> 
> Often commit messages are seen just as a necessary bad and not much
> energy is spent on them but for a maitainer solid commit messages have
> an indispensable value.
> 
> > +     void (*pre_transfer)(struct tpm_tis_spi_phy *phy);
> 
> Adding a new calback should be a commit of its own.
> 

Ok. I will do that and focus the commit message.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 4/4] tpm: add driver for cr50 on SPI
  2019-08-09 20:31   ` Jarkko Sakkinen
@ 2019-08-12 20:46     ` Stephen Boyd
  2019-08-15 13:00       ` Jarkko Sakkinen
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2019-08-12 20:46 UTC (permalink / raw)
  To: Jarkko Sakkinen, Peter Huewe
  Cc: Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Guenter Roeck, Alexander Steffen

Quoting Jarkko Sakkinen (2019-08-09 13:31:04)
> On Tue, 2019-08-06 at 15:07 -0700, Stephen Boyd wrote:
> > From: Andrey Pronin <apronin@chromium.org>
> > 
> > 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.
> 
> Which Chromebook models have this chip?

Pretty much all Chromebooks released in the last year or two have this
chip in them. I don't have an exhaustive list, but you can usually check
this by putting your device into dev mode and then looking at the driver
attached to the TPM device in sysfs or by grepping the dmesg output for
cr50.

> 
> If I had an access to one, how do I do kernel testing with it i.e.
> how do I get it to boot initramfs and bzImage from a USB stick?
> 
> 

You can follow the developer guide[1] and build a USB image for the
board you have. You can usually checkout the latest upstream kernel in
place of where the kernel is built from in the chroot, typically
~/trunk/src/third_party/kernel/<version number>. The build should pick
up that it's an upstream tree and try to use some default defconfig.
This driver isn't upstream yet, so you may need to enable it in the
defconfig, located in
~/trunk/src/third_party/chromiumos-overlay/eclass/cros-kernel/ so that
the driver is actually built. After that, use 'cros flash' to flash the
new kernel image to your USB stick and boot from USB with 'ctrl+u' and
you should be on your way to chromeos kernel testing.

[1] https://chromium.googlesource.com/chromiumos/docs/+/master/developer_guide.md


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 4/4] tpm: add driver for cr50 on SPI
  2019-08-12 20:46     ` Stephen Boyd
@ 2019-08-15 13:00       ` Jarkko Sakkinen
  0 siblings, 0 replies; 12+ messages in thread
From: Jarkko Sakkinen @ 2019-08-15 13:00 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Peter Huewe, Andrey Pronin, linux-kernel, linux-integrity,
	Duncan Laurie, Jason Gunthorpe, Arnd Bergmann,
	Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen

On Mon, Aug 12, 2019 at 01:46:35PM -0700, Stephen Boyd wrote:
> Quoting Jarkko Sakkinen (2019-08-09 13:31:04)
> > On Tue, 2019-08-06 at 15:07 -0700, Stephen Boyd wrote:
> > > From: Andrey Pronin <apronin@chromium.org>
> > > 
> > > 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.
> > 
> > Which Chromebook models have this chip?
> 
> Pretty much all Chromebooks released in the last year or two have this
> chip in them. I don't have an exhaustive list, but you can usually check
> this by putting your device into dev mode and then looking at the driver
> attached to the TPM device in sysfs or by grepping the dmesg output for
> cr50.
> 
> > 
> > If I had an access to one, how do I do kernel testing with it i.e.
> > how do I get it to boot initramfs and bzImage from a USB stick?
> > 
> > 
> 
> You can follow the developer guide[1] and build a USB image for the
> board you have. You can usually checkout the latest upstream kernel in
> place of where the kernel is built from in the chroot, typically
> ~/trunk/src/third_party/kernel/<version number>. The build should pick
> up that it's an upstream tree and try to use some default defconfig.
> This driver isn't upstream yet, so you may need to enable it in the
> defconfig, located in
> ~/trunk/src/third_party/chromiumos-overlay/eclass/cros-kernel/ so that
> the driver is actually built. After that, use 'cros flash' to flash the
> new kernel image to your USB stick and boot from USB with 'ctrl+u' and
> you should be on your way to chromeos kernel testing.
> 
> [1] https://chromium.googlesource.com/chromiumos/docs/+/master/developer_guide.md

Hey, thanks for info! I'll see if I can get my hands on one.

/Jarkko

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-08-15 13:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-06 22:07 [PATCH v3 0/4] tpm: Add driver for cr50 Stephen Boyd
2019-08-06 22:07 ` [PATCH v3 1/4] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd
2019-08-09 18:02   ` Jarkko Sakkinen
2019-08-12 14:11     ` Stephen Boyd
2019-08-06 22:07 ` [PATCH v3 2/4] tpm: tpm_tis_spi: Export functionality to other drivers Stephen Boyd
2019-08-09 20:28   ` Jarkko Sakkinen
2019-08-12 14:16     ` Stephen Boyd
2019-08-06 22:07 ` [PATCH v3 3/4] dt-bindings: tpm: document properties for cr50 Stephen Boyd
2019-08-06 22:07 ` [PATCH v3 4/4] tpm: add driver for cr50 on SPI Stephen Boyd
2019-08-09 20:31   ` Jarkko Sakkinen
2019-08-12 20:46     ` Stephen Boyd
2019-08-15 13:00       ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).