Quoting Alexander Steffen (2019-07-17 08:19:19)
> On 17.07.2019 00:45, Stephen Boyd wrote:
> > From: Duncan Laurie <dlau...@chromium.org>
> >
> > Add TPM 2.0 compatible I2C interface for chips with cr50 firmware.
> >
> > The firmware running on the currently supported H1 MCU requires a
> > special driver to handle its specific protocol, and this makes it
> > unsuitable to use tpm_tis_core_* and instead it must implement the
> > underlying TPM protocol similar to the other I2C TPM drivers.
> >
> > - All 4 byes of status register must be read/written at once.
> > - FIFO and burst count is limited to 63 and must be drained by AP.
> > - Provides an interrupt to indicate when read response data is ready
> > and when the TPM is finished processing write data.
>
> And why does this prevent using the existing tpm_tis_core
> infrastructure? Taking the status register as an example, you could just
> teach read_bytes to look at the requested address, and if it lies
> between 0x18 and 0x1b read the whole status register and then return
> only the subset that has been requested originally.
>
> Both approaches might not be pretty, but I'd prefer having shared code
> with explicit code paths for the differences instead of having two
> copies of mostly the same algorithm where a simple diff will print out a
> lot more than just the crucial differences.
There are a few i2c tpm drivers in drivers/char/tpm/. I still haven't
looked at the details but maybe this will work out. I'm planning to drop
this patch from the series and revisit this after getting the SPI driver
merged.
>
> > This driver is based on the existing infineon I2C TPM driver, which
> > most closely matches the cr50 i2c protocol behavior. The driver is
> > intentionally kept very similar in structure and style to the
> > corresponding drivers in coreboot and depthcharge.
> >
> > Signed-off-by: Duncan Laurie <dlau...@chromium.org>
> > [swb...@chromium.org: Depend on i2c even if it's a module, replace
> > boilier plate with SPDX tag, drop asm/byteorder.h include, simplify
> > return from probe]
> > Signed-off-by: Stephen Boyd <swb...@chromium.org>
> > ---
> > drivers/char/tpm/Kconfig | 10 +
> > drivers/char/tpm/Makefile | 1 +
> > drivers/char/tpm/cr50_i2c.c | 705 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 716 insertions(+)
> > create mode 100644 drivers/char/tpm/cr50_i2c.c
> >
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index b7028bfa6f87..57a8c3540265 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -119,6 +119,16 @@ config TCG_CR50
> > ---help---
> > Common routines shared by drivers for Cr50-based devices.
> >
> > +config TCG_CR50_I2C
> > + tristate "Cr50 I2C Interface"
> > + depends on I2C
> > + select TCG_CR50
> > + ---help---
> > + If you have a H1 secure module running Cr50 firmware on I2C bus,
> > + say Yes and it will be accessible from within Linux. To compile
> > + this driver as a module, choose M here; the module will be called
> > + cr50_i2c.
> > +
> > config TCG_CR50_SPI
> > tristate "Cr50 SPI Interface"
> > depends on SPI
> > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > index 4e89538c73c8..3ac3448c21fa 100644
> > --- a/drivers/char/tpm/Makefile
> > +++ b/drivers/char/tpm/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
> > obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
> > obj-$(CONFIG_TCG_CR50) += cr50.o
> > obj-$(CONFIG_TCG_CR50_SPI) += cr50_spi.o
> > +obj-$(CONFIG_TCG_CR50_I2C) += cr50_i2c.o
> > obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
> > obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
> > obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
> > diff --git a/drivers/char/tpm/cr50_i2c.c b/drivers/char/tpm/cr50_i2c.c
> > new file mode 100644
> > index 000000000000..25934c038b9b
> > --- /dev/null
> > +++ b/drivers/char/tpm/cr50_i2c.c
> > @@ -0,0 +1,705 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2016 Google Inc.
> > + *
> > + * Based on Linux Kernel TPM driver by
> > + * Peter Huewe <peter.hu...@infineon.com>
> > + * Copyright (C) 2011 Infineon Technologies
> > + */
> > +
> > +/*
> > + * cr50 is a firmware for H1 secure modules that requires special
> > + * handling for the I2C interface.
> > + *
> > + * - Use an interrupt for transaction status instead of hardcoded delays
> > + * - Must use write+wait+read read protocol
> > + * - All 4 bytes of status register must be read/written at once
> > + * - Burst count max is 63 bytes, and burst count behaves
> > + * slightly differently than other I2C TPMs
> > + * - When reading from FIFO the full burstcnt must be read
> > + * instead of just reading header and determining the remainder
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/completion.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm.h>
> > +#include <linux/slab.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/wait.h>
> > +#include "cr50.h"
> > +#include "tpm.h"
> > +
> > +#define CR50_MAX_BUFSIZE 63
> > +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during
> > transactions */
> > +#define CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready
> > without IRQ */
> > +#define CR50_I2C_DID_VID 0x00281ae0L
> > +#define CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */
> > +#define CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries
> > on I2C */
> > +#define CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries
> > on I2C */
> > +
> > +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.");
> > +
> > +struct priv_data {
> > + int irq;
> > + int locality;
> > + struct completion tpm_ready;
> > + u8 buf[CR50_MAX_BUFSIZE + sizeof(u8)];
> > +};
> > +
> > +/*
> > + * 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_i2c_int_handler(int dummy, void *dev_id)
> > +{
> > + struct tpm_chip *chip = dev_id;
> > + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> > +
> > + complete(&priv->tpm_ready);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * Wait for completion interrupt if available, otherwise use a fixed
> > + * delay for the TPM to be ready.
> > + *
> > + * Returns negative number for error, positive number for success.
> > + */
> > +static int cr50_i2c_wait_tpm_ready(struct tpm_chip *chip)
> > +{
> > + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> > + long rc;
> > +
> > + /* Use a safe fixed delay if interrupt is not supported */
> > + if (priv->irq <= 0) {
> > + msleep(CR50_TIMEOUT_NOIRQ_MS);
> > + return 1;
> > + }
> > +
> > + /* Wait for interrupt to indicate TPM is ready to respond */
> > + rc = wait_for_completion_timeout(&priv->tpm_ready,
> > + msecs_to_jiffies(chip->timeout_a));
> > +
> > + if (rc == 0)
> > + dev_warn(&chip->dev, "Timeout waiting for TPM ready\n");
> > +
> > + return rc;
> > +}
> > +
> > +static void cr50_i2c_enable_tpm_irq(struct tpm_chip *chip)
> > +{
> > + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> > +
> > + if (priv->irq > 0) {
> > + reinit_completion(&priv->tpm_ready);
> > + enable_irq(priv->irq);
> > + }
> > +}
> > +
> > +static void cr50_i2c_disable_tpm_irq(struct tpm_chip *chip)
> > +{
> > + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> > +
> > + if (priv->irq > 0)
> > + disable_irq(priv->irq);
> > +}
> > +
> > +/*
> > + * cr50_i2c_transfer - transfer messages over i2c
> > + *
> > + * @adapter: i2c adapter
> > + * @msgs: array of messages to transfer
> > + * @num: number of messages in the array
> > + *
> > + * Call unlocked i2c transfer routine with the provided parameters and
> > retry
> > + * in case of bus errors. Returns the number of transferred messages.
> > + */
>
> Documentation is nice, why is it only present for some of the functions?
> Also, the dev parameter is missing in the list of parameters above.
Ok.
>
> > +static int cr50_i2c_transfer(struct device *dev, struct i2c_adapter
> > *adapter,
> > + struct i2c_msg *msgs, int num)
> > +{
> > + int rc, try;
> > +
> > + for (try = 0; try < CR50_I2C_MAX_RETRIES; try++) {
> > + rc = __i2c_transfer(adapter, msgs, num);
> > + if (rc > 0)
> > + break;
> > + if (try)
> > + dev_warn(dev, "i2c transfer failed (attempt %d/%d):
> > %d\n",
> > + try+1, CR50_I2C_MAX_RETRIES, rc);
>
> Why does this not generate a message when the first attempt fails?
>
Hmm looks like an off-by-one bug.
> > +
> > +enum tis_access {
> > + TPM_ACCESS_VALID = 0x80,
> > + TPM_ACCESS_ACTIVE_LOCALITY = 0x20,
> > + TPM_ACCESS_REQUEST_PENDING = 0x04,
> > + TPM_ACCESS_REQUEST_USE = 0x02,
> > +};
> > +
> > +enum tis_status {
> > + TPM_STS_VALID = 0x80,
> > + TPM_STS_COMMAND_READY = 0x40,
> > + TPM_STS_GO = 0x20,
> > + TPM_STS_DATA_AVAIL = 0x10,
> > + TPM_STS_DATA_EXPECT = 0x08,
> > +};
> > +
> > +enum tis_defaults {
> > + TIS_SHORT_TIMEOUT = 750, /* ms */
> > + TIS_LONG_TIMEOUT = 2000, /* 2 sec */
> > +};
>
> This is already defined in tpm_tis_core.h. Do you really need to
> redefine it here?
This whole chunk of code is copying from tpm_i2c_infineon.c and I'm not
sure why that driver redefines things either. If i include
tpm_tis_core.h then I need to undef the below defines so they can be
redefined in this file.
>
> > +
> > +#define TPM_ACCESS(l) (0x0000 | ((l) << 4))
> > +#define TPM_STS(l) (0x0001 | ((l) << 4))
> > +#define TPM_DATA_FIFO(l) (0x0005 | ((l) << 4))
> > +#define TPM_DID_VID(l) (0x0006 | ((l) << 4))
> > +
