From: Stephen Boyd <swboyd@chromium.org>
To: Alexander Steffen <Alexander.Steffen@infineon.com>,
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
Peter Huewe <peterhuewe@gmx.de>
Cc: Andrey Pronin <apronin@chromium.org>,
linux-kernel@vger.kernel.org, Jason Gunthorpe <jgg@ziepe.ca>,
Arnd Bergmann <arnd@arndb.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-integrity@vger.kernel.org,
Duncan Laurie <dlaurie@chromium.org>,
Guenter Roeck <groeck@chromium.org>
Subject: Re: [PATCH v2 5/6] tpm: add driver for cr50 on SPI
Date: Wed, 17 Jul 2019 12:57:34 -0700 [thread overview]
Message-ID: <5d2f7daf.1c69fb81.c0b13.c3d4@mx.google.com> (raw)
In-Reply-To: <f824e3ab-ae2f-8c2f-549a-16569b10966e@infineon.com>
Quoting Alexander Steffen (2019-07-17 05:00:06)
> On 17.07.2019 00:45, 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.
> >
> > Signed-off-by: Andrey Pronin <apronin@chromium.org>
> > [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]
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c
> > new file mode 100644
> > index 000000000000..3c1b472297ad
> > --- /dev/null
> > +++ b/drivers/char/tpm/cr50_spi.c
> > @@ -0,0 +1,450 @@
> > +// 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 "cr50.h"
> > +#include "tpm_tis_core.h"
> > +
> > +/*
> > + * Cr50 timing constants:
> > + * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC.
> > + * - needs up to CR50_WAKE_START_DELAY_MSEC to wake after sleep.
> > + * - requires 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_MSEC for flow control 'ready' indication.
> > + */
> > +#define CR50_SLEEP_DELAY_MSEC 1000
> > +#define CR50_WAKE_START_DELAY_MSEC 1
> > +#define CR50_NOIRQ_ACCESS_DELAY_MSEC 2
> > +#define CR50_READY_IRQ_TIMEOUT_MSEC TPM2_TIMEOUT_A
> > +#define CR50_FLOW_CONTROL_MSEC TPM2_TIMEOUT_A
> > +#define MAX_IRQ_CONFIRMATION_ATTEMPTS 3
> > +
> > +#define MAX_SPI_FRAMESIZE 64
> > +
> > +#define TPM_CR50_FW_VER(l) (0x0F90 | ((l) << 12))
> > +#define TPM_CR50_MAX_FW_VER_LEN 64
> > +
> > +static unsigned short rng_quality = 1022;
> > +module_param(rng_quality, ushort, 0644);
> > +MODULE_PARM_DESC(rng_quality,
> > + "Estimation of true entropy, in bits per 1024 bits.");
>
> What is the purpose of this parameter? None of the other TPM drivers
> have it.
I think the idea is to let users override the quality if they decide
that they don't want to use the default value specified in the driver.
>
> > +
> > +struct cr50_spi_phy {
> > + struct tpm_tis_data priv;
> > + struct spi_device *spi_device;
> > +
> > + struct mutex time_track_mutex;
> > + unsigned long last_access_jiffies;
> > + unsigned long wake_after_jiffies;
> > +
> > + unsigned long access_delay_jiffies;
> > + unsigned long sleep_delay_jiffies;
> > + unsigned int wake_start_delay_msec;
> > +
> > + struct completion tpm_ready;
> > +
> > + unsigned int irq_confirmation_attempt;
> > + bool irq_needs_confirmation;
> > + bool irq_confirmed;
> > +
> > + u8 tx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
> > + u8 rx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
> > +};
> > +
> > +static struct cr50_spi_phy *to_cr50_spi_phy(struct tpm_tis_data *data)
> > +{
> > + return container_of(data, struct cr50_spi_phy, priv);
> > +}
> > +
> > +/*
> > + * 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 *phy = dev_id;
> > +
> > + phy->irq_confirmed = true;
> > + complete(&phy->tpm_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)
> > +{
> > + /*
> > + * Note: There is a small chance, if Cr50 is not accessed in a few days,
> > + * that time_in_range will not provide the correct result after the wrap
> > + * around for jiffies. In this case, we'll have an unneeded short delay,
> > + * which is fine.
> > + */
> > + unsigned long allowed_access =
> > + phy->last_access_jiffies + phy->access_delay_jiffies;
> > + unsigned long time_now = jiffies;
> > +
> > + if (time_in_range_open(time_now,
> > + phy->last_access_jiffies, allowed_access)) {
> > + unsigned long remaining =
> > + wait_for_completion_timeout(&phy->tpm_ready,
> > + allowed_access - time_now);
> > + if (remaining == 0 && phy->irq_confirmed) {
> > + dev_warn(&phy->spi_device->dev,
> > + "Timeout waiting for TPM ready IRQ\n");
> > + }
> > + }
> > + if (phy->irq_needs_confirmation) {
> > + if (phy->irq_confirmed) {
> > + phy->irq_needs_confirmation = false;
> > + phy->access_delay_jiffies =
> > + msecs_to_jiffies(CR50_READY_IRQ_TIMEOUT_MSEC);
> > + dev_info(&phy->spi_device->dev,
> > + "TPM ready IRQ confirmed on attempt %u\n",
> > + phy->irq_confirmation_attempt);
> > + } else if (++phy->irq_confirmation_attempt >
> > + MAX_IRQ_CONFIRMATION_ATTEMPTS) {
> > + phy->irq_needs_confirmation = false;
> > + dev_warn(&phy->spi_device->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_jiffies,
> > + phy->wake_after_jiffies);
> > +}
> > +
> > +static void cr50_wake_if_needed(struct cr50_spi_phy *phy)
> > +{
> > + if (cr50_needs_waking(phy)) {
> > + /* Assert CS, wait 1 msec, deassert CS */
> > + struct spi_transfer spi_cs_wake = { .delay_usecs = 1000 };
> > +
> > + spi_sync_transfer(phy->spi_device, &spi_cs_wake, 1);
> > + /* Wait for it to fully wake */
> > + msleep(phy->wake_start_delay_msec);
>
> wake_start_delay_msec is always 1, isn't it? (Why is that a variable at
> all? I see only one place that ever sets it.) Then msleep is not the
> best function to use, since it will usually sleep much longer. Use
> usleep_range instead. See Documentation/timers/timers-howto.txt.
Thanks. Will fix to be 1ms to 2ms range.
>
> > + }
> > + /* Reset the time when we need to wake Cr50 again */
> > + phy->wake_after_jiffies = jiffies + phy->sleep_delay_jiffies;
> > +}
> > +
> > +/*
> > + * Flow control: clock the bus and wait for cr50 to set LSB before
> > + * sending/receiving data. TCG PTP spec allows it to happen during
> > + * the last byte of header, but cr50 never does that in practice,
> > + * and earlier versions had a bug when it was set too early, so don't
> > + * check for it during header transfer.
> > + */
> > +static int cr50_spi_flow_control(struct cr50_spi_phy *phy)
> > +{
> > + unsigned long timeout_jiffies =
> > + jiffies + msecs_to_jiffies(CR50_FLOW_CONTROL_MSEC);
> > + struct spi_message m;
> > + int ret;
> > + struct spi_transfer spi_xfer = {
> > + .rx_buf = phy->rx_buf,
> > + .len = 1,
> > + .cs_change = 1,
> > + };
> > +
> > + do {
> > + spi_message_init(&m);
> > + spi_message_add_tail(&spi_xfer, &m);
> > + ret = spi_sync_locked(phy->spi_device, &m);
> > + if (ret < 0)
> > + return ret;
> > + if (time_after(jiffies, timeout_jiffies)) {
> > + dev_warn(&phy->spi_device->dev,
> > + "Timeout during flow control\n");
> > + return -EBUSY;
> > + }
> > + } while (!(phy->rx_buf[0] & 0x01));
> > + return 0;
> > +}
> > +
> > +static int cr50_spi_xfer_bytes(struct tpm_tis_data *data, u32 addr,
> > + u16 len, const u8 *tx, u8 *rx)
> > +{
> > + struct cr50_spi_phy *phy = to_cr50_spi_phy(data);
> > + struct spi_message m;
> > + struct spi_transfer spi_xfer = {
> > + .tx_buf = phy->tx_buf,
> > + .rx_buf = phy->rx_buf,
> > + .len = 4,
> > + .cs_change = 1,
> > + };
> > + int ret;
> > +
> > + if (len > MAX_SPI_FRAMESIZE)
> > + return -EINVAL;
> > +
> > + /*
> > + * Do this outside of spi_bus_lock in case cr50 is not the
> > + * only device on that spi bus.
> > + */
> > + mutex_lock(&phy->time_track_mutex);
> > + cr50_ensure_access_delay(phy);
> > + cr50_wake_if_needed(phy);
> > +
> > + phy->tx_buf[0] = (tx ? 0x00 : 0x80) | (len - 1);
> > + phy->tx_buf[1] = 0xD4;
> > + phy->tx_buf[2] = (addr >> 8) & 0xFF;
> > + phy->tx_buf[3] = addr & 0xFF;
> > +
> > + spi_message_init(&m);
> > + spi_message_add_tail(&spi_xfer, &m);
> > +
> > + spi_bus_lock(phy->spi_device->master);
> > + ret = spi_sync_locked(phy->spi_device, &m);
> > + if (ret < 0)
> > + goto exit;
> > +
> > + ret = cr50_spi_flow_control(phy);
> > + if (ret < 0)
> > + goto exit;
> > +
> > + spi_xfer.cs_change = 0;
> > + spi_xfer.len = len;
> > + if (tx) {
> > + memcpy(phy->tx_buf, tx, len);
> > + spi_xfer.rx_buf = NULL;
> > + } else {
> > + spi_xfer.tx_buf = NULL;
> > + }
> > +
> > + spi_message_init(&m);
> > + spi_message_add_tail(&spi_xfer, &m);
> > + reinit_completion(&phy->tpm_ready);
> > + ret = spi_sync_locked(phy->spi_device, &m);
> > + if (rx)
> > + memcpy(rx, phy->rx_buf, len);
> > +
> > +exit:
> > + spi_bus_unlock(phy->spi_device->master);
> > + phy->last_access_jiffies = jiffies;
> > + mutex_unlock(&phy->time_track_mutex);
> > +
> > + return ret;
> > +}
>
> This copies a lot of code from tpm_tis_spi, but then slightly changes
> some things, without really explaining why.
The commit text has some explanations. Here's the copy/paste from above:
> > - 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.
Do you want me to describe something further?
> For example, struct
> cr50_spi_phy contains both tx_buf and rx_buf, whereas tpm_tis_spi uses a
> single iobuf, that is allocated via devm_kmalloc instead of being part
> of the struct. Maybe the difference matters, maybe not, who knows?
Ok. Are you asking if this is a full-duplex SPI device?
>
> Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
> tpm_tis_spi, so that it can intercept the calls, execute the additional
> actions (like waking up the device), but then let tpm_tis_spi do the
> common work?
>
I suppose the read{16,32} and write32 functions could be reused. I'm not
sure how great it will be if we combine these two drivers, but I can
give it a try today and see how it looks.
next prev parent reply other threads:[~2019-07-17 19:57 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-16 22:45 [PATCH v2 0/6] tpm: Add driver for cr50 Stephen Boyd
2019-07-16 22:45 ` [PATCH v2 1/6] hwrng: core: Freeze khwrng thread during suspend Stephen Boyd
2019-07-17 1:43 ` Herbert Xu
2019-07-17 16:36 ` Stephen Boyd
2019-07-17 11:39 ` Jason Gunthorpe
2019-07-17 16:42 ` Stephen Boyd
2019-07-17 16:50 ` Jason Gunthorpe
2019-07-17 17:03 ` Stephen Boyd
2019-08-02 22:50 ` Stephen Boyd
2019-08-16 15:56 ` Alexander Steffen
2019-08-16 23:55 ` Jarkko Sakkinen
2019-08-02 20:22 ` Jarkko Sakkinen
2019-08-02 23:18 ` Stephen Boyd
2019-07-16 22:45 ` [PATCH v2 2/6] tpm_tis_core: add optional max xfer size check Stephen Boyd
2019-07-16 22:45 ` [PATCH v2 3/6] tpm_tis_spi: add max xfer size Stephen Boyd
2019-07-17 8:07 ` Alexander Steffen
2019-07-17 18:19 ` Stephen Boyd
2019-07-16 22:45 ` [PATCH v2 4/6] dt-bindings: tpm: document properties for cr50 Stephen Boyd
2019-07-16 22:45 ` [PATCH v2 5/6] tpm: add driver for cr50 on SPI Stephen Boyd
2019-07-17 12:00 ` Alexander Steffen
2019-07-17 12:25 ` Jason Gunthorpe
2019-07-17 16:49 ` Stephen Boyd
2019-07-17 16:56 ` Jason Gunthorpe
2019-07-17 17:05 ` Stephen Boyd
2019-07-17 17:12 ` Jason Gunthorpe
2019-07-17 17:22 ` Stephen Boyd
2019-07-17 17:25 ` Jason Gunthorpe
2019-07-17 18:21 ` Stephen Boyd
2019-07-17 18:30 ` Guenter Roeck
2019-07-17 18:36 ` Jason Gunthorpe
2019-07-17 19:57 ` Stephen Boyd [this message]
2019-07-17 21:38 ` Stephen Boyd
2019-07-17 22:17 ` Stephen Boyd
2019-07-18 16:47 ` Alexander Steffen
2019-07-18 18:11 ` Stephen Boyd
2019-07-19 7:53 ` Alexander Steffen
2019-08-01 16:02 ` Stephen Boyd
2019-08-02 15:21 ` Jarkko Sakkinen
2019-08-02 18:03 ` Stephen Boyd
2019-08-02 19:43 ` Jarkko Sakkinen
2019-07-18 16:47 ` Alexander Steffen
2019-07-18 18:07 ` Stephen Boyd
2019-07-19 7:51 ` Alexander Steffen
2019-08-02 20:43 ` Jarkko Sakkinen
2019-08-02 22:32 ` Stephen Boyd
2019-08-02 20:41 ` Jarkko Sakkinen
2019-07-16 22:45 ` [PATCH v2 6/6] tpm: Add driver for cr50 on I2C Stephen Boyd
2019-07-17 15:19 ` Alexander Steffen
2019-08-05 23:52 ` Stephen Boyd
2019-08-02 20:44 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5d2f7daf.1c69fb81.c0b13.c3d4@mx.google.com \
--to=swboyd@chromium.org \
--cc=Alexander.Steffen@infineon.com \
--cc=apronin@chromium.org \
--cc=arnd@arndb.de \
--cc=dlaurie@chromium.org \
--cc=gregkh@linuxfoundation.org \
--cc=groeck@chromium.org \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=jgg@ziepe.ca \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peterhuewe@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).