[U-Boot] [PATCH v3 17/25] tpm: add support for TPMv2.x SPI modules
Miquel Raynal
miquel.raynal at bootlin.com
Tue May 15 08:48:28 UTC 2018
Hi Simon,
On Wed, 2 May 2018 20:32:32 -0600, Simon Glass <sjg at chromium.org> wrote:
> Hi Miquel,
>
> On 2 May 2018 at 02:59, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> > Add the tpm2_tis_spi driver that should support any TPMv2 compliant
> > (SPI) module.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> > ---
> > drivers/tpm/Kconfig | 10 +
> > drivers/tpm/Makefile | 2 +
> > drivers/tpm/tpm2_tis_spi.c | 678 +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 690 insertions(+)
> > create mode 100644 drivers/tpm/tpm2_tis_spi.c
> >
> > diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig
> > index 01967ffd35..6661dcc1e3 100644
> > --- a/drivers/tpm/Kconfig
> > +++ b/drivers/tpm/Kconfig
> > @@ -141,6 +141,16 @@ config TPM_V2
> >
> > if TPM_V2 && !TPM_V1
> >
> > +config TPM2_TIS_SPI
> > + bool "Enable support for TPMv2.x SPI chips"
> > + depends on TPM_V2 && DM_SPI
> > + select TPM_DRIVER_SELECTED
> > + help
> > + This driver supports TPMv2.x devices connected on the SPI bus.
> > + The usual TPM operations and the 'tpm' command can be used to talk
> > + to the device using the standard TPM Interface Specification (TIS)
> > + protocol.
> > +
> > endif # TPM_V2
> >
> > endmenu
> > diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile
> > index c42a93f267..2c88b64659 100644
> > --- a/drivers/tpm/Makefile
> > +++ b/drivers/tpm/Makefile
> > @@ -11,3 +11,5 @@ obj-$(CONFIG_TPM_TIS_LPC) += tpm_tis_lpc.o
> > obj-$(CONFIG_TPM_TIS_SANDBOX) += tpm_tis_sandbox.o
> > obj-$(CONFIG_TPM_ST33ZP24_I2C) += tpm_tis_st33zp24_i2c.o
> > obj-$(CONFIG_TPM_ST33ZP24_SPI) += tpm_tis_st33zp24_spi.o
> > +
> > +obj-$(CONFIG_TPM2_TIS_SPI) += tpm2_tis_spi.o
> > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > new file mode 100644
> > index 0000000000..cfef5b8c24
> > --- /dev/null
> > +++ b/drivers/tpm/tpm2_tis_spi.c
> > @@ -0,0 +1,678 @@
> > +/*
> > + * Author:
> > + * Miquel Raynal <miquel.raynal at bootlin.com>
> > + *
> > + * Description:
> > + * SPI-level driver for TCG/TIS TPM (trusted platform module).
> > + * Specifications at www.trustedcomputinggroup.org
> > + *
> > + * This device driver implements the TPM interface as defined in
> > + * the TCG SPI protocol stack version 2.0.
> > + *
> > + * It is based on the U-Boot driver tpm_tis_infineon_i2c.c.
> > + *
> > + * SPDX-License-Identifier: GPL-2.0
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <fdtdec.h>
> > +#include <log.h>
> > +#include <spi.h>
> > +#include <tpm-v2.h>
> > +#include <linux/errno.h>
> > +#include <linux/compiler.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned/be_byteshift.h>
> > +
> > +#include "tpm_tis.h"
> > +#include "tpm_internal.h"
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +#define TPM_ACCESS(l) (0x0000 | ((l) << 12))
> > +#define TPM_INT_ENABLE(l) (0x0008 | ((l) << 12))
> > +#define TPM_STS(l) (0x0018 | ((l) << 12))
> > +#define TPM_DATA_FIFO(l) (0x0024 | ((l) << 12))
> > +#define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
> > +#define TPM_RID(l) (0x0F04 | ((l) << 12))
> > +
> > +#define MAX_SPI_FRAMESIZE 64
> > +
> > +/* Number of wait states to wait for */
> > +#define TPM_WAIT_STATES 100
> > +
> > +/*
> > + * tpm_tis_spi_read() - read from TPM register
> > + * @addr: register address to read from
> > + * @buffer: provided by caller
> > + * @len: number of bytes to read
> > + *
> > + * Read len bytes from TPM register and put them into
> > + * buffer (little-endian format, i.e. first byte is put into buffer[0]).
> > + *
> > + * NOTE: TPM is big-endian for multi-byte values. Multi-byte
> > + * values have to be swapped.
> > + *
> > + * Return -EIO on error, 0 on success.
> > + */
> > +static int tpm_tis_spi_xfer(struct udevice *dev, u32 addr, const u8 *out,
> > + u8 *in, u16 len)
> > +{
> > + struct spi_slave *slave = dev_get_parent_priv(dev);
> > + int transfer_len, ret;
> > + u8 tx_buf[MAX_SPI_FRAMESIZE];
> > + u8 rx_buf[MAX_SPI_FRAMESIZE];
> > +
> > + if (in && out) {
> > + log(LOGC_NONE, LOGL_ERR, "%s: can't do full duplex\n",
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + ret = spi_claim_bus(slave);
> > + if (ret < 0) {
> > + log(LOGC_NONE, LOGL_ERR, "%s: could not claim bus\n", __func__);
> > + return ret;
> > + }
> > +
> > + while (len) {
> > + /* Request */
> > + transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
> > + tx_buf[0] = (in ? BIT(7) : 0) | (transfer_len - 1);
> > + tx_buf[1] = 0xD4;
> > + tx_buf[2] = addr >> 8;
> > + tx_buf[3] = addr;
> > +
> > + ret = spi_xfer(slave, 4 * 8, tx_buf, rx_buf, SPI_XFER_BEGIN);
> > + if (ret < 0) {
> > + log(LOGC_NONE, LOGL_ERR,
> > + "%s: spi request transfer failed (err: %d)\n",
> > + __func__, ret);
> > + goto release_bus;
> > + }
> > +
> > + /* Wait state */
> > + if (!(rx_buf[3] & 0x1)) {
> > + int i;
> > +
> > + rx_buf[0] = 0;
>
> I don't think you need this?
This is indeed redundant. I'll remove it.
>
> > + for (i = 0; i < TPM_WAIT_STATES; i++) {
> > + ret = spi_xfer(slave, 1 * 8, NULL, rx_buf, 0);
> > + if (ret < 0) {
I also s/if (ret < 0)/if (ret)/ here ^
> > + log(LOGC_NONE, LOGL_ERR,
> > + "%s: wait state failed: %d\n",
> > + __func__, ret);
> > + goto release_bus;
> > + }
> > +
> > + if (rx_buf[0] & 0x1)
> > + break;
> > + }
> > +
> > + if (i == TPM_WAIT_STATES) {
> > + log(LOGC_NONE, LOGL_ERR,
> > + "%s: timeout on wait state\n", __func__);
> > + ret = -ETIMEDOUT;
> > + goto release_bus;
> > + }
> > + }
> > +
> > + /* Read/Write */
> > + if (out) {
> > + memcpy(tx_buf, out, transfer_len);
> > + out += transfer_len;
> > + }
> > +
> > + ret = spi_xfer(slave, transfer_len * 8,
> > + out ? tx_buf : NULL,
> > + in ? rx_buf : NULL,
> > + SPI_XFER_END);
> > + if (ret < 0) {
And here ^
> > + log(LOGC_NONE, LOGL_ERR,
> > + "%s: spi read transfer failed (err: %d)\n",
> > + __func__, ret);
> > + goto release_bus;
> > + }
> > +
> > + if (in) {
> > + memcpy(in, rx_buf, transfer_len);
> > + in += transfer_len;
> > + }
> > +
> > + len -= transfer_len;
> > + }
> > +
> > +release_bus:
> > + /* If an error occurred, release the chip by deasserting the CS */
> > + if (ret < 0)
> > + spi_xfer(slave, 0, NULL, NULL, SPI_XFER_END);
> > +
> > + spi_release_bus(slave);
> > +
> > + return ret;
> > +}
> > +
> > +static int tpm_tis_spi_read(struct udevice *dev, u16 addr, u8 *in, u16 len)
> > +{
> > + return tpm_tis_spi_xfer(dev, addr, NULL, in, len);
> > +}
> > +
> > +static __maybe_unused int tpm_tis_spi_read16(struct udevice *dev, u32 addr,
> > + u16 *result)
>
> Why is __maybe_unused needed in this file?
I used the *_spi_read16() function for development. I decided to keep
it because it could help but since nothing uses it, I'll drop it.
The __maybe_unused on the *_spi_read32() function below is not needed
though.
>
> > +{
> > + __le16 result_le;
> > + int ret;
> > +
> > + ret = tpm_tis_spi_read(dev, addr, (u8 *)&result_le, sizeof(u16));
> > + if (!ret)
> > + *result = le16_to_cpu(result_le);
> > +
> > + return ret;
> > +}
> > +
> > +static __maybe_unused int tpm_tis_spi_read32(struct udevice *dev, u32 addr,
> > + u32 *result)
> > +{
> > + __le32 result_le;
> > + int ret;
> > +
> > + ret = tpm_tis_spi_read(dev, addr, (u8 *)&result_le, sizeof(u32));
> > + if (!ret)
> > + *result = le32_to_cpu(result_le);
>
> Does this assume host endianness? Will it work on a big-endian machine?
It is the opposite, it assumes the endianness of the TPM (in the spec)
and adapts it to the host endianness. Am I missing something?
>
> [..]
>
> > +static int tpm_tis_spi_probe(struct udevice *dev)
> > +{
> > + struct tpm_chip *chip = dev_get_priv(dev);
> > + int ret;
> > +
> > + /* Ensure a minimum amount of time elapsed since reset */
> > + mdelay(30);
>
> This seems bad. Why is this needed? Where does the number come from?
I definitely should have defined this value, sorry.
This delay comes from the datasheet of the chip. I've changed that
static value into a dynamic one related to the compatible (like for
the PCR numbers).
> Can we instead check the time since reset somehow?
In this patch it looks like we could check the time since reset but
this is not reliable as the reset of the chip may happen at any time
(the reset GPIO support is added in the next commit).
>
> Regards,
> Simon
Thanks,
Miquèl
--
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
More information about the U-Boot
mailing list