* [PATCH v6 0/4] tpm: Add driver for cr50 @ 2019-08-29 22:41 Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 1/4] dt-bindings: tpm: document properties " Stephen Boyd ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Stephen Boyd @ 2019-08-29 22:41 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, Heiko Stuebner 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. Please review so we can get the approach to supporting this device sorted out. [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 v5 (https://lkml.kernel.org/r/20190828082150.42194-1-swboyd@chromium.org): * Picked up Jarkko's ack/review tags * Fixed bug with irqs happening before completion is initialized * Dropped is_cr50 bool * Moved wake_after to tpm_tis_spi struct * Changed authorship of main cr50 patch to Andrey as I'm just shuffling code here Changes from v4 (https://lkml.kernel.org/r/20190812223622.73297-1-swboyd@chromium.org): * Dropped the 'pre-transfer' hook patch and added a 'ready' member instead * Combined cr50_spi and tpm_tis_spi into one kernel module * Introduced a swizzle in tpm_tis_spi probe routine to jump to cr50 probe path * Moved binding to start of the thread * Picked up Jarkko reviewed-by tag on new flag for suspend/resume * Added a comment to flow control patch indicating what it's all about Changes from v3: * Split out hooks into separate patches * Update commit text to not say "libify" * Collapse if statement into one for first patch * Update commit text on first patch to mention flag * Drop TIS_IS_CR50 as it's unused 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> Cc: Heiko Stuebner <heiko@sntech.de> Andrey Pronin (2): dt-bindings: tpm: document properties for cr50 tpm: tpm_tis_spi: Support cr50 devices Stephen Boyd (2): tpm: Add a flag to indicate TPM power is managed by firmware tpm: tpm_tis_spi: Introduce a flow control callback .../bindings/security/tpm/google,cr50.txt | 19 ++ drivers/char/tpm/Makefile | 2 +- drivers/char/tpm/cr50_spi.c | 321 ++++++++++++++++++ drivers/char/tpm/tpm-interface.c | 8 +- drivers/char/tpm/tpm.h | 1 + drivers/char/tpm/tpm_tis_spi.c | 130 ++++--- drivers/char/tpm/tpm_tis_spi.h | 46 +++ 7 files changed, 478 insertions(+), 49 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 -- Sent by a computer through tubes ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 1/4] dt-bindings: tpm: document properties for cr50 2019-08-29 22:41 [PATCH v6 0/4] tpm: Add driver for cr50 Stephen Boyd @ 2019-08-29 22:41 ` Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 2/4] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2019-08-29 22:41 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, Heiko Stuebner, 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> Cc: Heiko Stuebner <heiko@sntech.de> Signed-off-by: Andrey Pronin <apronin@chromium.org> Reviewed-by: Rob Herring <robh@kernel.org> Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> 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..cd69c2efdd37 --- /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] 13+ messages in thread
* [PATCH v6 2/4] tpm: Add a flag to indicate TPM power is managed by firmware 2019-08-29 22:41 [PATCH v6 0/4] tpm: Add driver for cr50 Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 1/4] dt-bindings: tpm: document properties " Stephen Boyd @ 2019-08-29 22:41 ` Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 3/4] tpm: tpm_tis_spi: Introduce a flow control callback Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices Stephen Boyd 3 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2019-08-29 22:41 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, Heiko Stuebner 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, TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED, 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> Cc: Heiko Stuebner <heiko@sntech.de> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.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..0b3def8e8186 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) && + !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] 13+ messages in thread
* [PATCH v6 3/4] tpm: tpm_tis_spi: Introduce a flow control callback 2019-08-29 22:41 [PATCH v6 0/4] tpm: Add driver for cr50 Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 1/4] dt-bindings: tpm: document properties " Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 2/4] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd @ 2019-08-29 22:41 ` Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices Stephen Boyd 3 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2019-08-29 22:41 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, Heiko Stuebner Cr50 firmware has a different flow control protocol than the one used by this TPM PTP SPI driver. Introduce a flow control callback so we can override the standard sequence with the custom one that Cr50 uses. 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> Cc: Heiko Stuebner <heiko@sntech.de> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/char/tpm/tpm_tis_spi.c | 62 ++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 21 deletions(-) diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c index 19513e622053..b3ed85671dd8 100644 --- a/drivers/char/tpm/tpm_tis_spi.c +++ b/drivers/char/tpm/tpm_tis_spi.c @@ -42,6 +42,8 @@ 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); u8 *iobuf; }; @@ -50,12 +52,46 @@ static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *da return container_of(data, struct tpm_tis_spi_phy, priv); } +/* + * TCG SPI flow control is documented in section 6.4 of the spec[1]. In short, + * keep trying to read from the device until MISO goes high indicating the + * wait state has ended. + * + * [1] https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/ + */ +static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy, + struct spi_transfer *spi_xfer) +{ + 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, 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 +118,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; @@ -207,6 +226,7 @@ static int tpm_tis_spi_probe(struct 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) -- Sent by a computer through tubes ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices 2019-08-29 22:41 [PATCH v6 0/4] tpm: Add driver for cr50 Stephen Boyd ` (2 preceding siblings ...) 2019-08-29 22:41 ` [PATCH v6 3/4] tpm: tpm_tis_spi: Introduce a flow control callback Stephen Boyd @ 2019-08-29 22:41 ` Stephen Boyd 2019-08-30 15:33 ` Heiko Stuebner ` (3 more replies) 3 siblings, 4 replies; 13+ messages in thread From: Stephen Boyd @ 2019-08-29 22:41 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, Heiko Stuebner 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 Cr50 firmware has a requirement to wait for the TPM to wakeup before sending commands over the SPI bus. Otherwise, the firmware could be in deep sleep and not respond. The method to wait for the device to wakeup is slightly different than the usual flow control mechanism described in the TCG SPI spec. Add a completion to tpm_tis_spi_transfer() before we start a SPI transfer so we can keep track of the last time the TPM driver accessed the SPI bus to support the flow control mechanism. Split the cr50 logic off into a different file to keep it out of the normal code flow of the existing SPI driver while making it all part of the same module when the code is optionally compiled into the same module. Export a new function, tpm_tis_spi_init(), and the associated read/write/transfer APIs so that we can do this. Make the cr50 code wrap the tpm_tis_spi_phy struct with its own struct to override the behavior of tpm_tis_spi_transfer() by supplying a custom flow control hook. This shares the most code between the core driver and the cr50 support without combining everything into the core driver or exporting module symbols. 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> Cc: Heiko Stuebner <heiko@sntech.de> [swboyd@chromium.org: Replace boilerplate with SPDX tag, drop suspended bit and remove ifdef checks in cr50.h, migrate to functions exported in tpm_tis_spi.h, combine into one module instead of two] Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/char/tpm/Makefile | 2 +- drivers/char/tpm/cr50_spi.c | 321 +++++++++++++++++++++++++++++++++ drivers/char/tpm/tpm_tis_spi.c | 78 ++++---- drivers/char/tpm/tpm_tis_spi.h | 46 +++++ 4 files changed, 415 insertions(+), 32 deletions(-) create mode 100644 drivers/char/tpm/cr50_spi.c create mode 100644 drivers/char/tpm/tpm_tis_spi.h diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index a01c4cab902a..6ff135d6f008 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -21,7 +21,7 @@ tpm-$(CONFIG_EFI) += eventlog/efi.o tpm-$(CONFIG_OF) += eventlog/of.o obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o obj-$(CONFIG_TCG_TIS) += tpm_tis.o -obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o +obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o cr50_spi.o obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c new file mode 100644 index 000000000000..187a023c2556 --- /dev/null +++ b/drivers/char/tpm/cr50_spi.c @@ -0,0 +1,321 @@ +// 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/completion.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 + +struct cr50_spi_phy { + struct tpm_tis_spi_phy spi_phy; + + struct mutex time_track_mutex; + unsigned long last_access; + + unsigned long access_delay; + + 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->spi_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->spi_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->spi_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 */ + 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 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); +} + +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->wake_after = jiffies; + init_completion(&phy->ready); + + cr50_phy->access_delay = CR50_NOIRQ_ACCESS_DELAY; + cr50_phy->last_access = jiffies; + mutex_init(&cr50_phy->time_track_mutex); + + 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 +int tpm_tis_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); + /* + * Jiffies not increased during suspend, so we need to reset + * the time to wake Cr50 after resume. + */ + phy->wake_after = jiffies; + + return tpm_tis_resume(dev); +} +#endif diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c index b3ed85671dd8..5e4253e7c080 100644 --- a/drivers/char/tpm/tpm_tis_spi.c +++ b/drivers/char/tpm/tpm_tis_spi.c @@ -20,6 +20,7 @@ * Dorn and Kyleen Hall and Jarko Sakkinnen. */ +#include <linux/completion.h> #include <linux/init.h> #include <linux/module.h> #include <linux/moduleparam.h> @@ -31,27 +32,16 @@ #include <linux/spi/spi.h> #include <linux/gpio.h> +#include <linux/of_device.h> #include <linux/of_irq.h> #include <linux/of_gpio.h> #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; - int (*flow_control)(struct tpm_tis_spi_phy *phy, - struct spi_transfer *xfer); - 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); -} - /* * TCG SPI flow control is documented in section 6.4 of the spec[1]. In short, * keep trying to read from the device until MISO goes high indicating the @@ -87,8 +77,8 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy, return 0; } -static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len, - u8 *in, const u8 *out) +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; @@ -136,6 +126,7 @@ 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); + reinit_completion(&phy->ready); ret = spi_sync_locked(phy->spi_device, &m); if (ret < 0) goto exit; @@ -165,7 +156,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; @@ -178,7 +169,7 @@ static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result) return rc; } -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; @@ -191,7 +182,7 @@ static int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result) return rc; } -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; @@ -203,6 +194,18 @@ static int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value) return rc; } +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); +} + static const struct tpm_tis_phy_ops tpm_spi_phy_ops = { .read_bytes = tpm_tis_spi_read_bytes, .write_bytes = tpm_tis_spi_write_bytes, @@ -221,11 +224,6 @@ 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 */ @@ -234,11 +232,27 @@ 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); + init_completion(&phy->ready); + return tpm_tis_spi_init(dev, phy, irq, &tpm_spi_phy_ops); +} + +typedef int (*tpm_tis_spi_probe_func)(struct spi_device *); + +static int tpm_tis_spi_driver_probe(struct spi_device *spi) +{ + const struct spi_device_id *spi_dev_id = spi_get_device_id(spi); + tpm_tis_spi_probe_func probe_func; + + probe_func = of_device_get_match_data(&spi->dev); + if (!probe_func && spi_dev_id) + probe_func = (tpm_tis_spi_probe_func)spi_dev_id->driver_data; + if (!probe_func) + return -ENODEV; + + return probe_func(spi); } -static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume); +static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_spi_resume); static int tpm_tis_spi_remove(struct spi_device *dev) { @@ -250,15 +264,17 @@ static int tpm_tis_spi_remove(struct spi_device *dev) } static const struct spi_device_id tpm_tis_spi_id[] = { - {"tpm_tis_spi", 0}, + { "tpm_tis_spi", (unsigned long)tpm_tis_spi_probe }, + { "cr50", (unsigned long)cr50_spi_probe }, {} }; MODULE_DEVICE_TABLE(spi, tpm_tis_spi_id); static const struct of_device_id of_tis_spi_match[] = { - { .compatible = "st,st33htpm-spi", }, - { .compatible = "infineon,slb9670", }, - { .compatible = "tcg,tpm_tis-spi", }, + { .compatible = "st,st33htpm-spi", .data = tpm_tis_spi_probe }, + { .compatible = "infineon,slb9670", .data = tpm_tis_spi_probe }, + { .compatible = "tcg,tpm_tis-spi", .data = tpm_tis_spi_probe }, + { .compatible = "google,cr50", .data = cr50_spi_probe }, {} }; MODULE_DEVICE_TABLE(of, of_tis_spi_match); @@ -277,7 +293,7 @@ static struct spi_driver tpm_tis_spi_driver = { .of_match_table = of_match_ptr(of_tis_spi_match), .acpi_match_table = ACPI_PTR(acpi_tis_spi_match), }, - .probe = tpm_tis_spi_probe, + .probe = tpm_tis_spi_driver_probe, .remove = tpm_tis_spi_remove, .id_table = tpm_tis_spi_id, }; diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h new file mode 100644 index 000000000000..afb0e9861c4f --- /dev/null +++ b/drivers/char/tpm/tpm_tis_spi.h @@ -0,0 +1,46 @@ +/* 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); + struct completion ready; + unsigned long wake_after; + + 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); + +extern int cr50_spi_probe(struct spi_device *spi); + +#ifdef CONFIG_PM_SLEEP +extern int tpm_tis_spi_resume(struct device *dev); +#else +#define tpm_tis_spi_resume NULL +#endif + +#endif -- Sent by a computer through tubes ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices 2019-08-29 22:41 ` [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices Stephen Boyd @ 2019-08-30 15:33 ` Heiko Stuebner 2019-08-30 15:40 ` Stephen Boyd 2019-09-02 16:53 ` kbuild test robot ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Heiko Stuebner @ 2019-08-30 15:33 UTC (permalink / raw) To: Stephen Boyd Cc: Peter Huewe, Jarkko Sakkinen, Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen Am Freitag, 30. August 2019, 00:41:10 CEST schrieb Stephen Boyd: > 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 > > Cr50 firmware has a requirement to wait for the TPM to wakeup before > sending commands over the SPI bus. Otherwise, the firmware could be in > deep sleep and not respond. The method to wait for the device to wakeup > is slightly different than the usual flow control mechanism described in > the TCG SPI spec. Add a completion to tpm_tis_spi_transfer() before we > start a SPI transfer so we can keep track of the last time the TPM > driver accessed the SPI bus to support the flow control mechanism. > > Split the cr50 logic off into a different file to keep it out of the > normal code flow of the existing SPI driver while making it all part of > the same module when the code is optionally compiled into the same > module. Export a new function, tpm_tis_spi_init(), and the associated > read/write/transfer APIs so that we can do this. Make the cr50 code wrap > the tpm_tis_spi_phy struct with its own struct to override the behavior > of tpm_tis_spi_transfer() by supplying a custom flow control hook. This > shares the most code between the core driver and the cr50 support > without combining everything into the core driver or exporting module > symbols. > > 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> > Cc: Heiko Stuebner <heiko@sntech.de> > [swboyd@chromium.org: Replace boilerplate with SPDX tag, drop > suspended bit and remove ifdef checks in cr50.h, migrate to functions > exported in tpm_tis_spi.h, combine into one module instead of two] > Signed-off-by: Stephen Boyd <swboyd@chromium.org> This variant breaks building the tpm as a module: WARNING: modpost: missing MODULE_LICENSE() in drivers/char/tpm/cr50_spi.o see include/linux/module.h for more information ERROR: "tpm_tis_spi_write32" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_read32" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_read16" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_init" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_transfer" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_resume" [drivers/char/tpm/tpm_tis_spi.ko] undefined! ERROR: "cr50_spi_probe" [drivers/char/tpm/tpm_tis_spi.ko] undefined! make[2]: *** [../scripts/Makefile.modpost:103: modules-modpost] Fehler 1 make[1]: *** [/home/devel/hstuebner/00_git-repos/linux-rockchip/Makefile:1302: modules] Fehler 2 make[1]: Verzeichnis „/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64“ wird verlassen make: *** [Makefile:179: sub-make] Fehler 2 After adding a dummy MODULE_LICENSE I end up with: ERROR: "tpm_tis_spi_write32" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_read32" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_read16" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_init" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_transfer" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_resume" [drivers/char/tpm/tpm_tis_spi.ko] undefined! ERROR: "cr50_spi_probe" [drivers/char/tpm/tpm_tis_spi.ko] undefined! So building things as modules this way clearly is not working. Going back to the previous approach makes the module happy again, aka: diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index 6ff135d6f008..477fcddbff8c 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -21,7 +21,9 @@ tpm-$(CONFIG_EFI) += eventlog/efi.o tpm-$(CONFIG_OF) += eventlog/of.o obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o obj-$(CONFIG_TCG_TIS) += tpm_tis.o -obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o cr50_spi.o +obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi_mod.o +tpm_tis_spi_mod-y := tpm_tis_spi.o +tpm_tis_spi_mod-y += cr50_spi.o obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o > --- > drivers/char/tpm/Makefile | 2 +- > drivers/char/tpm/cr50_spi.c | 321 +++++++++++++++++++++++++++++++++ > drivers/char/tpm/tpm_tis_spi.c | 78 ++++---- > drivers/char/tpm/tpm_tis_spi.h | 46 +++++ > 4 files changed, 415 insertions(+), 32 deletions(-) > create mode 100644 drivers/char/tpm/cr50_spi.c > create mode 100644 drivers/char/tpm/tpm_tis_spi.h > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile > index a01c4cab902a..6ff135d6f008 100644 > --- a/drivers/char/tpm/Makefile > +++ b/drivers/char/tpm/Makefile > @@ -21,7 +21,7 @@ tpm-$(CONFIG_EFI) += eventlog/efi.o > tpm-$(CONFIG_OF) += eventlog/of.o > obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o > obj-$(CONFIG_TCG_TIS) += tpm_tis.o > -obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o > +obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o cr50_spi.o > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o > diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c > new file mode 100644 > index 000000000000..187a023c2556 > --- /dev/null > +++ b/drivers/char/tpm/cr50_spi.c > @@ -0,0 +1,321 @@ > +// 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/completion.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 > + > +struct cr50_spi_phy { > + struct tpm_tis_spi_phy spi_phy; > + > + struct mutex time_track_mutex; > + unsigned long last_access; > + > + unsigned long access_delay; > + > + 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->spi_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->spi_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->spi_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 */ > + 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 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); > +} > + > +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->wake_after = jiffies; > + init_completion(&phy->ready); > + > + cr50_phy->access_delay = CR50_NOIRQ_ACCESS_DELAY; > + cr50_phy->last_access = jiffies; > + mutex_init(&cr50_phy->time_track_mutex); > + > + 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 > +int tpm_tis_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); > + /* > + * Jiffies not increased during suspend, so we need to reset > + * the time to wake Cr50 after resume. > + */ > + phy->wake_after = jiffies; > + > + return tpm_tis_resume(dev); > +} > +#endif > diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c > index b3ed85671dd8..5e4253e7c080 100644 > --- a/drivers/char/tpm/tpm_tis_spi.c > +++ b/drivers/char/tpm/tpm_tis_spi.c > @@ -20,6 +20,7 @@ > * Dorn and Kyleen Hall and Jarko Sakkinnen. > */ > > +#include <linux/completion.h> > #include <linux/init.h> > #include <linux/module.h> > #include <linux/moduleparam.h> > @@ -31,27 +32,16 @@ > > #include <linux/spi/spi.h> > #include <linux/gpio.h> > +#include <linux/of_device.h> > #include <linux/of_irq.h> > #include <linux/of_gpio.h> > #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; > - int (*flow_control)(struct tpm_tis_spi_phy *phy, > - struct spi_transfer *xfer); > - 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); > -} > - > /* > * TCG SPI flow control is documented in section 6.4 of the spec[1]. In short, > * keep trying to read from the device until MISO goes high indicating the > @@ -87,8 +77,8 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy, > return 0; > } > > -static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len, > - u8 *in, const u8 *out) > +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; > @@ -136,6 +126,7 @@ 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); > + reinit_completion(&phy->ready); > ret = spi_sync_locked(phy->spi_device, &m); > if (ret < 0) > goto exit; > @@ -165,7 +156,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; > @@ -178,7 +169,7 @@ static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result) > return rc; > } > > -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; > @@ -191,7 +182,7 @@ static int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result) > return rc; > } > > -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; > @@ -203,6 +194,18 @@ static int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value) > return rc; > } > > +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); > +} > + > static const struct tpm_tis_phy_ops tpm_spi_phy_ops = { > .read_bytes = tpm_tis_spi_read_bytes, > .write_bytes = tpm_tis_spi_write_bytes, > @@ -221,11 +224,6 @@ 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 */ > @@ -234,11 +232,27 @@ 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); > + init_completion(&phy->ready); > + return tpm_tis_spi_init(dev, phy, irq, &tpm_spi_phy_ops); > +} > + > +typedef int (*tpm_tis_spi_probe_func)(struct spi_device *); > + > +static int tpm_tis_spi_driver_probe(struct spi_device *spi) > +{ > + const struct spi_device_id *spi_dev_id = spi_get_device_id(spi); > + tpm_tis_spi_probe_func probe_func; > + > + probe_func = of_device_get_match_data(&spi->dev); > + if (!probe_func && spi_dev_id) > + probe_func = (tpm_tis_spi_probe_func)spi_dev_id->driver_data; > + if (!probe_func) > + return -ENODEV; > + > + return probe_func(spi); > } > > -static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume); > +static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_spi_resume); > > static int tpm_tis_spi_remove(struct spi_device *dev) > { > @@ -250,15 +264,17 @@ static int tpm_tis_spi_remove(struct spi_device *dev) > } > > static const struct spi_device_id tpm_tis_spi_id[] = { > - {"tpm_tis_spi", 0}, > + { "tpm_tis_spi", (unsigned long)tpm_tis_spi_probe }, > + { "cr50", (unsigned long)cr50_spi_probe }, > {} > }; > MODULE_DEVICE_TABLE(spi, tpm_tis_spi_id); > > static const struct of_device_id of_tis_spi_match[] = { > - { .compatible = "st,st33htpm-spi", }, > - { .compatible = "infineon,slb9670", }, > - { .compatible = "tcg,tpm_tis-spi", }, > + { .compatible = "st,st33htpm-spi", .data = tpm_tis_spi_probe }, > + { .compatible = "infineon,slb9670", .data = tpm_tis_spi_probe }, > + { .compatible = "tcg,tpm_tis-spi", .data = tpm_tis_spi_probe }, > + { .compatible = "google,cr50", .data = cr50_spi_probe }, > {} > }; > MODULE_DEVICE_TABLE(of, of_tis_spi_match); > @@ -277,7 +293,7 @@ static struct spi_driver tpm_tis_spi_driver = { > .of_match_table = of_match_ptr(of_tis_spi_match), > .acpi_match_table = ACPI_PTR(acpi_tis_spi_match), > }, > - .probe = tpm_tis_spi_probe, > + .probe = tpm_tis_spi_driver_probe, > .remove = tpm_tis_spi_remove, > .id_table = tpm_tis_spi_id, > }; > diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h > new file mode 100644 > index 000000000000..afb0e9861c4f > --- /dev/null > +++ b/drivers/char/tpm/tpm_tis_spi.h > @@ -0,0 +1,46 @@ > +/* 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); > + struct completion ready; > + unsigned long wake_after; > + > + 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); > + > +extern int cr50_spi_probe(struct spi_device *spi); > + > +#ifdef CONFIG_PM_SLEEP > +extern int tpm_tis_spi_resume(struct device *dev); > +#else > +#define tpm_tis_spi_resume NULL > +#endif > + > +#endif > ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices 2019-08-30 15:33 ` Heiko Stuebner @ 2019-08-30 15:40 ` Stephen Boyd 0 siblings, 0 replies; 13+ messages in thread From: Stephen Boyd @ 2019-08-30 15:40 UTC (permalink / raw) To: Heiko Stuebner, Masahiro Yamada Cc: Peter Huewe, Jarkko Sakkinen, Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen, linux-kbuild Quoting Heiko Stuebner (2019-08-30 08:33:48) > Am Freitag, 30. August 2019, 00:41:10 CEST schrieb Stephen Boyd: > > 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 > > > > Cr50 firmware has a requirement to wait for the TPM to wakeup before > > sending commands over the SPI bus. Otherwise, the firmware could be in > > deep sleep and not respond. The method to wait for the device to wakeup > > is slightly different than the usual flow control mechanism described in > > the TCG SPI spec. Add a completion to tpm_tis_spi_transfer() before we > > start a SPI transfer so we can keep track of the last time the TPM > > driver accessed the SPI bus to support the flow control mechanism. > > > > Split the cr50 logic off into a different file to keep it out of the > > normal code flow of the existing SPI driver while making it all part of > > the same module when the code is optionally compiled into the same > > module. Export a new function, tpm_tis_spi_init(), and the associated > > read/write/transfer APIs so that we can do this. Make the cr50 code wrap > > the tpm_tis_spi_phy struct with its own struct to override the behavior > > of tpm_tis_spi_transfer() by supplying a custom flow control hook. This > > shares the most code between the core driver and the cr50 support > > without combining everything into the core driver or exporting module > > symbols. > > > > 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> > > Cc: Heiko Stuebner <heiko@sntech.de> > > [swboyd@chromium.org: Replace boilerplate with SPDX tag, drop > > suspended bit and remove ifdef checks in cr50.h, migrate to functions > > exported in tpm_tis_spi.h, combine into one module instead of two] > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > This variant breaks building the tpm as a module: > > > WARNING: modpost: missing MODULE_LICENSE() in drivers/char/tpm/cr50_spi.o > see include/linux/module.h for more information > ERROR: "tpm_tis_spi_write32" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_read32" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_read16" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_init" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_transfer" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_resume" [drivers/char/tpm/tpm_tis_spi.ko] undefined! > ERROR: "cr50_spi_probe" [drivers/char/tpm/tpm_tis_spi.ko] undefined! > make[2]: *** [../scripts/Makefile.modpost:103: modules-modpost] Fehler 1 > make[1]: *** [/home/devel/hstuebner/00_git-repos/linux-rockchip/Makefile:1302: modules] Fehler 2 > make[1]: Verzeichnis „/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64“ wird verlassen > make: *** [Makefile:179: sub-make] Fehler 2 > > > After adding a dummy MODULE_LICENSE I end up with: > > ERROR: "tpm_tis_spi_write32" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_read32" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_read16" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_init" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_transfer" [drivers/char/tpm/cr50_spi.ko] undefined! > ERROR: "tpm_tis_spi_resume" [drivers/char/tpm/tpm_tis_spi.ko] undefined! > ERROR: "cr50_spi_probe" [drivers/char/tpm/tpm_tis_spi.ko] undefined! > > > So building things as modules this way clearly is not working. Going back > to the previous approach makes the module happy again, aka: > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile > index 6ff135d6f008..477fcddbff8c 100644 > --- a/drivers/char/tpm/Makefile > +++ b/drivers/char/tpm/Makefile > @@ -21,7 +21,9 @@ tpm-$(CONFIG_EFI) += eventlog/efi.o > tpm-$(CONFIG_OF) += eventlog/of.o > obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o > obj-$(CONFIG_TCG_TIS) += tpm_tis.o > -obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o cr50_spi.o > +obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi_mod.o > +tpm_tis_spi_mod-y := tpm_tis_spi.o > +tpm_tis_spi_mod-y += cr50_spi.o > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o > I wasn't happy that I had to make a tpm_tis_spi_mod target to get the .ko to build properly but I thought it was because of the extra Kconfig option. If two files can't be combined into one .ko and have it be named tpm_tis_spi.ko then we have to do the above workaround. Or we can #include the cr50_spi.c file into tpm_tis_spi.c file. Or we can rename tpm_tis_spi.c to tpm_tis_spi_base.c and then do the above change with a tpm_tis_spi-y. Maybe Masahiro has some hints. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices 2019-08-29 22:41 ` [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices Stephen Boyd 2019-08-30 15:33 ` Heiko Stuebner @ 2019-09-02 16:53 ` kbuild test robot 2019-09-02 18:19 ` kbuild test robot 2019-09-03 16:39 ` Jarkko Sakkinen 3 siblings, 0 replies; 13+ messages in thread From: kbuild test robot @ 2019-09-02 16:53 UTC (permalink / raw) To: Stephen Boyd Cc: kbuild-all, Peter Huewe, Jarkko Sakkinen, Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen, Heiko Stuebner [-- Attachment #1: Type: text/plain, Size: 1257 bytes --] Hi Stephen, Thank you for the patch! Yet something to improve: [auto build test ERROR on ] url: https://github.com/0day-ci/linux/commits/Stephen-Boyd/tpm-Add-driver-for-cr50/20190901-100447 base: config: i386-randconfig-a004-201935 (attached as .config) compiler: gcc-7 (Debian 7.4.0-11) 7.4.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): WARNING: modpost: missing MODULE_LICENSE() in drivers/char/tpm/cr50_spi.o see include/linux/module.h for more information >> ERROR: "cr50_spi_probe" [drivers/char/tpm/tpm_tis_spi.ko] undefined! >> ERROR: "tpm_tis_spi_write32" [drivers/char/tpm/cr50_spi.ko] undefined! >> ERROR: "tpm_tis_spi_read32" [drivers/char/tpm/cr50_spi.ko] undefined! >> ERROR: "tpm_tis_spi_read16" [drivers/char/tpm/cr50_spi.ko] undefined! >> ERROR: "tpm_tis_spi_init" [drivers/char/tpm/cr50_spi.ko] undefined! >> ERROR: "tpm_tis_spi_transfer" [drivers/char/tpm/cr50_spi.ko] undefined! --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 35054 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices 2019-08-29 22:41 ` [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices Stephen Boyd 2019-08-30 15:33 ` Heiko Stuebner 2019-09-02 16:53 ` kbuild test robot @ 2019-09-02 18:19 ` kbuild test robot 2019-09-03 16:39 ` Jarkko Sakkinen 3 siblings, 0 replies; 13+ messages in thread From: kbuild test robot @ 2019-09-02 18:19 UTC (permalink / raw) To: Stephen Boyd Cc: kbuild-all, Peter Huewe, Jarkko Sakkinen, Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen, Heiko Stuebner [-- Attachment #1: Type: text/plain, Size: 1337 bytes --] Hi Stephen, Thank you for the patch! Yet something to improve: [auto build test ERROR on ] url: https://github.com/0day-ci/linux/commits/Stephen-Boyd/tpm-Add-driver-for-cr50/20190901-100447 base: config: x86_64-randconfig-s1-09021303 (attached as .config) compiler: gcc-7 (Debian 7.4.0-11) 7.4.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): WARNING: modpost: missing MODULE_LICENSE() in drivers/char/tpm/cr50_spi.o see include/linux/module.h for more information >> ERROR: "tpm_tis_spi_resume" [drivers/char/tpm/tpm_tis_spi.ko] undefined! ERROR: "cr50_spi_probe" [drivers/char/tpm/tpm_tis_spi.ko] undefined! ERROR: "tpm_tis_spi_read16" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_write32" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_read32" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_init" [drivers/char/tpm/cr50_spi.ko] undefined! ERROR: "tpm_tis_spi_transfer" [drivers/char/tpm/cr50_spi.ko] undefined! --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 33274 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices 2019-08-29 22:41 ` [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices Stephen Boyd ` (2 preceding siblings ...) 2019-09-02 18:19 ` kbuild test robot @ 2019-09-03 16:39 ` Jarkko Sakkinen 2019-09-03 16:52 ` Stephen Boyd 3 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2019-09-03 16:39 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, Heiko Stuebner On Thu, 2019-08-29 at 15:41 -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 > > Cr50 firmware has a requirement to wait for the TPM to wakeup before > sending commands over the SPI bus. Otherwise, the firmware could be in > deep sleep and not respond. The method to wait for the device to wakeup > is slightly different than the usual flow control mechanism described in > the TCG SPI spec. Add a completion to tpm_tis_spi_transfer() before we > start a SPI transfer so we can keep track of the last time the TPM > driver accessed the SPI bus to support the flow control mechanism. > > Split the cr50 logic off into a different file to keep it out of the > normal code flow of the existing SPI driver while making it all part of > the same module when the code is optionally compiled into the same > module. Export a new function, tpm_tis_spi_init(), and the associated > read/write/transfer APIs so that we can do this. Make the cr50 code wrap > the tpm_tis_spi_phy struct with its own struct to override the behavior > of tpm_tis_spi_transfer() by supplying a custom flow control hook. This > shares the most code between the core driver and the cr50 support > without combining everything into the core driver or exporting module > symbols. > > 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> > Cc: Heiko Stuebner <heiko@sntech.de> Had to time to look at this patch set after all before LPC. I just realized that the kconfig has taken away. Not sure why is that because there's been only request to not have a new LKM. There still should be ability opt-out to have Cr50 support in vmlinux. /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices 2019-09-03 16:39 ` Jarkko Sakkinen @ 2019-09-03 16:52 ` Stephen Boyd 2019-09-07 17:04 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Stephen Boyd @ 2019-09-03 16:52 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, Heiko Stuebner Quoting Jarkko Sakkinen (2019-09-03 09:39:51) > On Thu, 2019-08-29 at 15:41 -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 > > > > Cr50 firmware has a requirement to wait for the TPM to wakeup before > > sending commands over the SPI bus. Otherwise, the firmware could be in > > deep sleep and not respond. The method to wait for the device to wakeup > > is slightly different than the usual flow control mechanism described in > > the TCG SPI spec. Add a completion to tpm_tis_spi_transfer() before we > > start a SPI transfer so we can keep track of the last time the TPM > > driver accessed the SPI bus to support the flow control mechanism. > > > > Split the cr50 logic off into a different file to keep it out of the > > normal code flow of the existing SPI driver while making it all part of > > the same module when the code is optionally compiled into the same > > module. Export a new function, tpm_tis_spi_init(), and the associated > > read/write/transfer APIs so that we can do this. Make the cr50 code wrap > > the tpm_tis_spi_phy struct with its own struct to override the behavior > > of tpm_tis_spi_transfer() by supplying a custom flow control hook. This > > shares the most code between the core driver and the cr50 support > > without combining everything into the core driver or exporting module > > symbols. > > > > 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> > > Cc: Heiko Stuebner <heiko@sntech.de> > > Had to time to look at this patch set after all before LPC. I just > realized that the kconfig has taken away. Not sure why is that > because there's been only request to not have a new LKM. There > still should be ability opt-out to have Cr50 support in vmlinux. > That's fair. I'll put the Kconfig option back. There's still the small issue of what to do about the module name. Should I rename the tpm_tis_spi.c file to something else so that the module can keep the same name? Or was the tpm_tis_spi_mod.ko trick from v5 good enough? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices 2019-09-03 16:52 ` Stephen Boyd @ 2019-09-07 17:04 ` Jarkko Sakkinen 2019-09-07 17:20 ` Heiko Stübner 0 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2019-09-07 17:04 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, Heiko Stuebner On Tue, 2019-09-03 at 09:52 -0700, Stephen Boyd wrote: > That's fair. I'll put the Kconfig option back. There's still the small > issue of what to do about the module name. Should I rename the > tpm_tis_spi.c file to something else so that the module can keep the > same name? Or was the tpm_tis_spi_mod.ko trick from v5 good enough? Not sure I understood the question correctly but how I think this should be deployed is: - A boolean CONFIG_TCG_TIS_SPI_CR50. - tpm_tis_spi_cr50.c that gets compiled in as part of tpm_tis_spi when the config option is selected. I think this would best follow the conventions that are in place already. Please tell if I got something wrong or if there is some bottleneck in this framework but this is anyway what I would prefer with the knowledge I have... /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices 2019-09-07 17:04 ` Jarkko Sakkinen @ 2019-09-07 17:20 ` Heiko Stübner 0 siblings, 0 replies; 13+ messages in thread From: Heiko Stübner @ 2019-09-07 17:20 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Stephen Boyd, Peter Huewe, Andrey Pronin, linux-kernel, linux-integrity, Duncan Laurie, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, Guenter Roeck, Alexander Steffen Hi Jarkko, Am Samstag, 7. September 2019, 19:04:15 CEST schrieb Jarkko Sakkinen: > On Tue, 2019-09-03 at 09:52 -0700, Stephen Boyd wrote: > > That's fair. I'll put the Kconfig option back. There's still the small > > issue of what to do about the module name. Should I rename the > > tpm_tis_spi.c file to something else so that the module can keep the > > same name? Or was the tpm_tis_spi_mod.ko trick from v5 good enough? > > Not sure I understood the question correctly but how I think > this should be deployed is: > > - A boolean CONFIG_TCG_TIS_SPI_CR50. > - tpm_tis_spi_cr50.c that gets compiled in as part of tpm_tis_spi > when the config option is selected. > > I think this would best follow the conventions that are in place > already. Please tell if I got something wrong or if there is some > bottleneck in this framework but this is anyway what I would prefer > with the knowledge I have... There is an implementation detail to iron out: Doing obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o cr50_spi.o as in this patch results in it failing to build as a module, due to them getting compiled to separate modules, yt sharing code. So I guess doing obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o obj-$(CONFIG_TCG_TIS_SPI_CR50) += cr50_spi.o will result in the same error, hence the question of doing something like obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi_mod.o tpm_tis_spi_mod-y := tpm_tis_spi.o tpm_tis_spi_mod-$(CONFIG_TCG_TIS_SPI_CR50) += cr50_spi.o so that sources get compiled and the module getting build from the result. Heiko ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-09-07 17:20 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-29 22:41 [PATCH v6 0/4] tpm: Add driver for cr50 Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 1/4] dt-bindings: tpm: document properties " Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 2/4] tpm: Add a flag to indicate TPM power is managed by firmware Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 3/4] tpm: tpm_tis_spi: Introduce a flow control callback Stephen Boyd 2019-08-29 22:41 ` [PATCH v6 4/4] tpm: tpm_tis_spi: Support cr50 devices Stephen Boyd 2019-08-30 15:33 ` Heiko Stuebner 2019-08-30 15:40 ` Stephen Boyd 2019-09-02 16:53 ` kbuild test robot 2019-09-02 18:19 ` kbuild test robot 2019-09-03 16:39 ` Jarkko Sakkinen 2019-09-03 16:52 ` Stephen Boyd 2019-09-07 17:04 ` Jarkko Sakkinen 2019-09-07 17:20 ` Heiko Stübner
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).