* [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp
@ 2019-06-28 15:13 Oshri Alkoby
2019-06-28 15:13 ` [PATCH v2 1/2] dt-bindings: tpm: add the TPM I2C PTP device tree binding documentation Oshri Alkoby
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Oshri Alkoby @ 2019-06-28 15:13 UTC (permalink / raw)
To: robh+dt, mark.rutland, peterhuewe, jarkko.sakkinen, jgg, arnd,
gregkh, oshrialkoby85, oshri.alkoby
Cc: devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman,
nayna, dan.morav, tomer.maimon
This patch set adds support for TPM devices that implement the I2C
interface defined by TCG PTP specification:
https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf
The driver was tested on Raspberry-Pie 3, using Nuvoton NPCT75X TPM.
changes
-------
v2:
- Extended device tree binding description
v1:
- Initial version
Oshri Alkoby (2):
dt-bindings: tpm: add the TPM I2C PTP device tree binding
documentation
char: tpm: add new driver for tpm i2c ptp
.../bindings/security/tpm/tpm-i2c-ptp.txt | 24 +
drivers/char/tpm/Kconfig | 22 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm_i2c_ptp.c | 1099 +++++++++++++++++
4 files changed, 1146 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt
create mode 100644 drivers/char/tpm/tpm_i2c_ptp.c
--
2.18.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v2 1/2] dt-bindings: tpm: add the TPM I2C PTP device tree binding documentation 2019-06-28 15:13 [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp Oshri Alkoby @ 2019-06-28 15:13 ` Oshri Alkoby 2019-06-28 15:13 ` [PATCH v2 2/2] char: tpm: add new driver for tpm i2c ptp Oshri Alkoby 2019-07-04 8:43 ` [PATCH v2 0/2] " Jarkko Sakkinen 2 siblings, 0 replies; 17+ messages in thread From: Oshri Alkoby @ 2019-06-28 15:13 UTC (permalink / raw) To: robh+dt, mark.rutland, peterhuewe, jarkko.sakkinen, jgg, arnd, gregkh, oshrialkoby85, oshri.alkoby Cc: devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, dan.morav, tomer.maimon Signed-off-by: Oshri Alkoby <oshrialkoby85@gmail.com> --- .../bindings/security/tpm/tpm-i2c-ptp.txt | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt b/Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt new file mode 100644 index 000000000000..8b0207fdf3e2 --- /dev/null +++ b/Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt @@ -0,0 +1,24 @@ +* Device Tree Bindings for I2C PTP based Trusted Platform Module(TPM) + +The TCG defines a hardware protocol, registers and interface (based +on the TPM Interface Specification) for accessing TPM devices +implemented with an I2C interface. + +Refer to the 'I2C Interface Definition' section in 'TCG PC Client +PlatformTPMProfile(PTP) Specification' publication for specification. + +Required properties: + +- compatible : Should be "tcg,tpm_i2c_ptp" +- reg : Address on the bus +- tpm-pirq : Input gpio pin, used for host interrupts + +Example (for Raspberry Pie 3 Board with Nuvoton's NPCT75X (2.0) +------------------------------------------------------------------- + +tpm_i2c_ptp: tpm_i2c_ptp@2e { + + compatible = "tcg,tpm_i2c_ptp"; + reg = <0x2e>; + tpm-pirq = <&gpio 24 GPIO_ACTIVE_HIGH>; +}; -- 2.18.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] char: tpm: add new driver for tpm i2c ptp 2019-06-28 15:13 [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp Oshri Alkoby 2019-06-28 15:13 ` [PATCH v2 1/2] dt-bindings: tpm: add the TPM I2C PTP device tree binding documentation Oshri Alkoby @ 2019-06-28 15:13 ` Oshri Alkoby 2019-07-04 8:43 ` [PATCH v2 0/2] " Jarkko Sakkinen 2 siblings, 0 replies; 17+ messages in thread From: Oshri Alkoby @ 2019-06-28 15:13 UTC (permalink / raw) To: robh+dt, mark.rutland, peterhuewe, jarkko.sakkinen, jgg, arnd, gregkh, oshrialkoby85, oshri.alkoby Cc: devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, dan.morav, tomer.maimon Signed-off-by: Oshri Alkoby <oshrialkoby85@gmail.com> --- drivers/char/tpm/Kconfig | 22 + drivers/char/tpm/Makefile | 1 + drivers/char/tpm/tpm_i2c_ptp.c | 1099 ++++++++++++++++++++++++++++++++ 3 files changed, 1122 insertions(+) create mode 100644 drivers/char/tpm/tpm_i2c_ptp.c diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index 88a3c06fc153..ea4138145191 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -97,6 +97,28 @@ config TCG_TIS_I2C_NUVOTON To compile this driver as a module, choose M here; the module will be called tpm_i2c_nuvoton. +config TCG_TIS_I2C_PTP + tristate "TPM Interface Specification 1.2/2.0 Interface (I2C - PTP)" + depends on I2C + select CRC_CCITT + ---help--- + If you have a TPM security chip with an I2C interface that impelements + the TPM I2C interface protocol defined by the PTP 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 tpm_i2c_ptp. + +config TCG_TIS_I2C_PTP_MAX_SIZE + prompt "Max I2C Buffer Size" + depends on TCG_TIS_I2C_PTP + int + default 32 + help + Set the maximal I2C buffer size. This will alter the default value. A + different size can be set by using a module parameter (i2c_max_size) + as well. If no parameter is provided when loading, this is the value + that will be used. + config TCG_NSC tristate "National Semiconductor TPM Interface" depends on X86 diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index a01c4cab902a..d736f22205cb 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -25,6 +25,7 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_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 +obj-$(CONFIG_TCG_TIS_I2C_PTP) += tpm_i2c_ptp.o obj-$(CONFIG_TCG_NSC) += tpm_nsc.o obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o diff --git a/drivers/char/tpm/tpm_i2c_ptp.c b/drivers/char/tpm/tpm_i2c_ptp.c new file mode 100644 index 000000000000..cc1065b79c2d --- /dev/null +++ b/drivers/char/tpm/tpm_i2c_ptp.c @@ -0,0 +1,1099 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2014-2019 Nuvoton Technology corporation + * + * TPM I2C PTP + * + * TPM I2C Device Driver Interface for devices that implement the TPM I2C + * Interface defined by TCG PC Client Platform TPM Profile (PTP) Specification + * Revision 01.03 v22 at www.trustedcomputinggroup.org + */ + +#include <linux/init.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/slab.h> +#include <linux/interrupt.h> +#include <linux/wait.h> +#include <linux/i2c.h> +#include <linux/freezer.h> +#include <linux/of_device.h> +#include <linux/gpio.h> +#include <linux/of_gpio.h> +#include <linux/crc-ccitt.h> +#include "tpm.h" + +/* I2C interface offsets */ +#define TPM_LOC_SEL 0x00 +#define TPM_ACCESS 0x04 +#define TPM_INT_ENABLE 0x08 +#define TPM_INT_STATUS 0x10 +#define TPM_INT_CAPABILITY 0x14 +#define TPM_STS 0x18 +#define TPM_STS_BURST_COUNT 0x19 +#define TPM_DATA_FIFO 0x24 +#define TPM_I2C_INTERFACE_CAPABILITY 0x30 +#define TPM_I2C_DEVICE_ADDRESS 0x38 +#define TPM_DATA_CSUM_ENABLE 0x40 +#define TPM_DATA_CSUM 0x44 +#define TPM_VID_DID 0x48 +#define TPM_RID 0x4C + + /* max. command/response length */ +#define TPM_I2C_BUFSIZE 2048 + +/* I2C bus device maximum buffer size w/o counting I2C address or command */ +#define TPM_I2C_MAX_BUF_SIZE 32 + +#define TPM_I2C_MAX_RETRY_CNT 5 +#define TPM_I2C_RETRY_DELAY_SHORT_US (2 * 1000) +#define TPM_I2C_RETRY_DELAY_LONG_US (10 * 1000) +#define TPM_I2C_DELAY_RANGE_US 300 + +#define OF_IS_TPM2 ((void *)1) +#define I2C_IS_TPM2 1 + +struct tpm_tis_data { + u16 manufacturer_id; + int locality; + int irq; + bool irq_tested; + unsigned int flags; + wait_queue_head_t int_queue; + wait_queue_head_t read_queue; + const struct tpm_tis_phy_ops *phy_ops; + unsigned int intrs; + unsigned short rng_quality; +}; + +#define MAX_COUNT 3 +#define SLEEP_DURATION_LOW 55 +#define SLEEP_DURATION_HI 65 + +static int iic_tpm_read(struct i2c_client *client, u8 addr, u8 *buffer, + size_t len) +{ + struct i2c_msg msg1 = { + .addr = client->addr, + .len = 1, + .buf = &addr + }; + struct i2c_msg msg2 = { + .addr = client->addr, + .flags = I2C_M_RD, + .len = len, + .buf = buffer + }; + int rc = 0; + int count; + + if (!client->adapter->algo->master_xfer) + return -EOPNOTSUPP; + i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); + for (count = 0; count < MAX_COUNT; count++) { + rc = __i2c_transfer(client->adapter, &msg1, 1); + if (rc > 0) + break; /* break here to skip sleep */ + usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI); + } + if (rc <= 0) + goto out; + for (count = 0; count < MAX_COUNT; count++) { + rc = __i2c_transfer(client->adapter, &msg2, 1); + if (rc > 0) + break; + usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI); + } +out: + i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); + usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI); + if (rc <= 0) + return -EIO; + return len; +} + +static u8 tpm_dev_buf[TPM_I2C_BUFSIZE + sizeof(u8)]; /* max buff size + addr */ + +#ifdef CONFIG_TCG_TIS_I2C_PTP_MAX_SIZE + static int i2c_max_size = CONFIG_TCG_TIS_I2C_PTP_MAX_SIZE; +#else + static int i2c_max_size = TPM_I2C_MAX_BUF_SIZE; +#endif +module_param(i2c_max_size, int, 0660); + +static int iic_tpm_write_generic(struct i2c_client *client, + u8 addr, u8 *buffer, size_t len, + unsigned int sleep_low, + unsigned int sleep_hi, u8 max_count) +{ + int rc = -EIO; + int count; + struct i2c_msg msg1 = { + .addr = client->addr, + .len = len + 1, + .buf = tpm_dev_buf + }; + + if (len > TPM_BUFSIZE) + return -EINVAL; + if (!client->adapter->algo->master_xfer) + return -EOPNOTSUPP; + i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); + tpm_dev_buf[0] = addr; + memcpy(&tpm_dev_buf[1], buffer, len); + for (count = 0; count < max_count; count++) { + rc = __i2c_transfer(client->adapter, &msg1, 1); + if (rc > 0) + break; + usleep_range(sleep_low, sleep_hi); + } + i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); + usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI); + if (rc <= 0) + return -EIO; + return 0; +} + +static s32 i2c_ptp_read_buf(struct i2c_client *client, u8 offset, size_t size, + u8 *data) +{ + s32 status; + + status = iic_tpm_read(client, offset, data, size); + dev_dbg(&client->dev, + "%s(offset=%u size=%zu data=%*ph) -> sts=%d\n", __func__, + offset, size, (int)size, data, status); + return status; +} + +static s32 i2c_ptp_write_buf(struct i2c_client *client, u8 offset, size_t size, + u8 *data) +{ + s32 status; + + status = iic_tpm_write_generic(client, offset, data, size, + SLEEP_DURATION_LOW, SLEEP_DURATION_HI, + MAX_COUNT); + dev_dbg(&client->dev, + "%s(offset=%u size=%zu data=%*ph) -> sts=%d\n", __func__, + offset, size, (int)size, data, status); + + return status; +} + +#define TPM_INT_DATA_AVAIL (BIT(0)) +#define TPM_INT_STS_VALID (BIT(1)) +#define TPM_INT_LOC_CHANGED (BIT(2)) +#define TPM_INT_CMD_READY (BIT(7)) +#define TPM_INT_GLOBAL (BIT(31)) + +#define TPM_INT_TO_ACTIVATE (TPM_INT_DATA_AVAIL) + +static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, + bool check_cancel, bool *canceled) +{ + u8 status = chip->ops->status(chip); + + *canceled = false; + if (status != 0xff && (status & mask) == mask) + return true; + if (check_cancel && chip->ops->req_canceled(chip, status)) { + *canceled = true; + return true; + } + return false; +} + +int i2c_ptp_wait_for_tpm_stat_l(struct tpm_chip *chip, u8 mask, + unsigned long timeout, wait_queue_head_t *queue, + bool check_cancel) +{ + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + unsigned long stop, start, long_delay; + long rc; + u8 status; + bool canceled = false; + unsigned int cur_intrs; + + start = jiffies; + stop = start + timeout; + long_delay = start + usecs_to_jiffies(TPM_I2C_RETRY_DELAY_LONG_US); + + if (queue && chip->flags & TPM_CHIP_FLAG_IRQ) { +again: + cur_intrs = priv->intrs; + timeout = stop - jiffies; + if ((long)timeout <= 0) + return -ETIME; + + enable_irq(priv->irq); + rc = wait_event_interruptible_timeout(*queue, + ((cur_intrs != priv->intrs) && + wait_for_tpm_stat_cond(chip, mask, check_cancel, &canceled)), + timeout); + if (rc > 0) { + if (canceled) + return -ECANCELED; + return 0; + } + if (rc == -ERESTARTSYS && freezing(current)) { + clear_thread_flag(TIF_SIGPENDING); + goto again; + } + } else { + do { + status = chip->ops->status(chip); + if (status != 0xff && (status & mask) == mask) + return 0; + + if (time_before(jiffies, long_delay)) + usleep_range(TPM_I2C_RETRY_DELAY_SHORT_US, + TPM_I2C_RETRY_DELAY_SHORT_US + + TPM_I2C_DELAY_RANGE_US); + else + usleep_range(TPM_I2C_RETRY_DELAY_LONG_US, + TPM_I2C_RETRY_DELAY_LONG_US + + TPM_I2C_DELAY_RANGE_US); + + } while (time_before(jiffies, stop)); + } + return -ETIME; +} + +/* wrapper of wait_for_tpm_stat, since we can't do the int processing during + * the int_handler in I2C, here we do it after the int_handler is done and + * wait_for_tpm_stat retruns + */ +static int i2c_ptp_wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, + unsigned long timeout, + wait_queue_head_t *queue, + bool check_cancel) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + u32 intsts; + int rc; + + rc = i2c_ptp_wait_for_tpm_stat_l(chip, mask, timeout, queue, + check_cancel); + if (rc < 0) + return rc; + + if (chip->flags & TPM_CHIP_FLAG_IRQ) { + rc = i2c_ptp_read_buf(client, TPM_INT_STATUS, 4, (u8 *)&intsts); + if (rc < 0) { + dev_err(&chip->dev, + "%s() fail to read TPM_INT_STATUS\n", __func__); + return -EIO; + } + + /* Clear interrupts handled */ + rc = i2c_ptp_write_buf(client, TPM_INT_STATUS, 4, + (u8 *)&intsts); + if (rc < 0) { + dev_err(&chip->dev, + "%s() fail to clear int status\n", __func__); + return -EIO; + } + } + + return 0; +} + +#define TPM_ACCESS_REQUEST_USE (BIT(1)) +#define TPM_ACCESS_REQUEST_PENDING (BIT(2)) +#define TPM_ACCESS_ACTIVE_LOCALITY (BIT(5)) +#define TPM_ACCESS_VALID_STS (BIT(7)) + +/* Before we attempt to access the TPM we must see that the valid bit is set. + * The specification says that this bit is 0 at reset and remains 0 until the + * 'TPM has gone through its self test and initialization and has established + * correct values in the other bits.' + */ +static int wait_startup(struct tpm_chip *chip, int l) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + unsigned long stop = jiffies + chip->timeout_a; + + do { + int rc; + u8 access; + + rc = i2c_ptp_read_buf(client, TPM_ACCESS, 1, &access); + if (rc < 0) + return rc; + + if (access != 0xff && (access & TPM_ACCESS_VALID_STS) != 0) + return 0; + tpm_msleep(TPM_TIMEOUT); + } while (time_before(jiffies, stop)); + return -1; +} + +static bool i2c_ptp_check_locality(struct tpm_chip *chip, int l) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + int rc; + u8 access; + u8 data; + + data = (u8)l; + rc = i2c_ptp_write_buf(client, TPM_LOC_SEL, 1, &data); + if (rc < 0) + return false; + + rc = i2c_ptp_read_buf(client, TPM_ACCESS, 1, &access); + if (rc < 0) + return false; + + if ((access & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID_STS)) == + (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID_STS)) { + priv->locality = l; + return true; + } + + return false; +} + +static bool i2c_ptp_locality_inactive(struct tpm_chip *chip, int l) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + int rc; + u8 access; + u8 data; + + data = (u8)l; + rc = i2c_ptp_write_buf(client, TPM_LOC_SEL, 1, &data); + if (rc < 0) + return false; + + rc = i2c_ptp_read_buf(client, TPM_ACCESS, 1, &access); + if (rc < 0) + return false; + + if ((access & (TPM_ACCESS_VALID_STS | TPM_ACCESS_ACTIVE_LOCALITY)) + == TPM_ACCESS_VALID_STS) + return true; + + return false; +} + +static int i2c_ptp_release_locality(struct tpm_chip *chip, int l) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + unsigned long stop; + int rc; + u8 data; + + if (i2c_ptp_locality_inactive(chip, l)) + return 0; + + data = TPM_ACCESS_ACTIVE_LOCALITY; + rc = i2c_ptp_write_buf(client, TPM_ACCESS, 1, &data); + if (rc < 0) + return rc; + + stop = jiffies + chip->timeout_a; + do { + if (i2c_ptp_locality_inactive(chip, l)) + return 0; + + tpm_msleep(TPM_TIMEOUT); + } while (time_before(jiffies, stop)); + + return -1; +} + +static int i2c_ptp_request_locality(struct tpm_chip *chip, int l) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + unsigned long stop; + int rc; + u8 data; + + if (i2c_ptp_check_locality(chip, l)) + return l; + + data = TPM_ACCESS_REQUEST_USE; + rc = i2c_ptp_write_buf(client, TPM_ACCESS, 1, &data); + if (rc < 0) + return rc; + + stop = jiffies + chip->timeout_a; + + do { + if (i2c_ptp_check_locality(chip, l)) + return l; + + rc = i2c_ptp_write_buf(client, TPM_ACCESS, 1, &data); + if (rc < 0) + return rc; + + tpm_msleep(TPM_TIMEOUT); + } while (time_before(jiffies, stop)); + + return -1; +} + +#define TPM_STS_RESPONSE_RETRY (BIT(1)) +#define TPM_STS_SELFTEST_DONE (BIT(2)) +#define TPM_STS_EXPECT (BIT(3)) +#define TPM_STS_DATA_AVAIL (BIT(4)) +#define TPM_STS_GO (BIT(5)) +#define TPM_STS_COMMAND_READY (BIT(6)) +#define TPM_STS_VALID (BIT(7)) +#define TPM_STS_COMMAND_CANCEL (BIT(24)) + +/* bit1...bit0 reads always 0 */ +#define TPM_STS_ERR_VAL (BIT(0) | BIT(1)) + +/* read TPM_STS register */ +static u8 i2c_ptp_status(struct tpm_chip *chip) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + int rc; + u8 status; + + rc = i2c_ptp_read_buf(client, TPM_STS, 1, &status); + if (rc < 0) + return 0; + + return status; +} + +/* write commandReady to TPM_STS register */ +static void i2c_ptp_ready(struct tpm_chip *chip) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + u8 data; + + data = TPM_STS_COMMAND_READY; + i2c_ptp_write_buf(client, TPM_STS, 1, &data); +} + +/* read Burst Count */ +static int i2c_ptp_get_burstcount(struct tpm_chip *chip) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + unsigned long stop; + int burstcnt = 0, rc; + + /* wait for burstcount */ + stop = jiffies + chip->timeout_a; + + do { + rc = i2c_ptp_read_buf(client, TPM_STS_BURST_COUNT, 2, + (u8 *)&burstcnt); + if (rc < 0) + return rc; + + if (burstcnt) + return burstcnt; + + usleep_range(TPM_TIMEOUT_USECS_MIN, TPM_TIMEOUT_USECS_MAX); + } while (time_before(jiffies, stop)); + + return -EBUSY; +} + +/* write commandCancel to TPM_STS register */ +static void i2c_ptp_cancel(struct tpm_chip *chip) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + u32 data; + + data = TPM_STS_COMMAND_CANCEL; + i2c_ptp_write_buf(client, TPM_STS, 4, (u8 *)&data); +} + +/* enable checksum */ +static int i2c_ptp_enable_csum(struct tpm_chip *chip) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + s32 rc; + u8 data = 1; + + /* Enable CSUM */ + rc = i2c_ptp_write_buf(client, TPM_DATA_CSUM_ENABLE, 1, &data); + if (rc < 0) { + dev_err(&chip->dev, + "%s() fail to read to TPM_DATA_CSUM_ENABLE: %d\n", + __func__, rc); + return rc; + } + return 0; +} + +/* Caclculate crc16 of the buffer and compares it to TPM_DATA_CSUM */ +static int i2c_ptp_check_crc(struct tpm_chip *chip, u8 *buf, size_t len) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + s32 status; + u16 tpm_csum = 0; + u16 crc = 0; + + crc = crc_ccitt(crc, buf, len); + crc = be16_to_cpu(crc); + status = i2c_ptp_read_buf(client, TPM_DATA_CSUM, 2, (u8 *)&tpm_csum); + if (status <= 0) { + dev_err(&chip->dev, "%s() fail to read to TPM_DATA_CSUM: %d\n", + __func__, status); + return -EIO; + } + + if (tpm_csum != crc) { + dev_err(&chip->dev, "%s() bad data csum\n", __func__); + return -EIO; + } + + return 0; +} + +static int i2c_ptp_recv_data(struct tpm_chip *chip, u8 *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + int size = 0, burstcnt, rc, bytes2read = count; + bool paramsize_flag = false; + + while (size < bytes2read) { + burstcnt = i2c_ptp_get_burstcount(chip); + if (burstcnt <= 0) + return burstcnt; + + burstcnt = min_t(int, (bytes2read - size), burstcnt); + rc = i2c_ptp_read_buf(client, TPM_DATA_FIFO, burstcnt, + buf + size); + if (rc < 0) + return rc; + + size += burstcnt; + + if (size >= 6 && !paramsize_flag) { + bytes2read = be32_to_cpu(*(__be32 *)(buf + 2)); + paramsize_flag = true; + } + } + + return size; +} + +static int i2c_ptp_recv(struct tpm_chip *chip, u8 *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + int size = 0; + int status, retries; + + if (count < TPM_HEADER_SIZE) { + status = -EIO; + goto out; + } + + for (retries = 0; retries < TPM_I2C_MAX_RETRY_CNT; retries++) { + size = 0; + + if (retries > 0) { + /* if this is not the first trial, set responseRetry */ + u8 data = TPM_STS_RESPONSE_RETRY; + + i2c_ptp_write_buf(client, TPM_STS, 1, &data); + } + + status = i2c_ptp_wait_for_tpm_stat(chip, + (TPM_STS_DATA_AVAIL | TPM_STS_VALID), + chip->timeout_b, NULL, + false); + if (status < 0) + return status; + + size = i2c_ptp_recv_data(chip, buf, TPM_HEADER_SIZE); + if (size < TPM_HEADER_SIZE) + continue; + + status = i2c_ptp_wait_for_tpm_stat(chip, TPM_STS_VALID, + chip->timeout_a, NULL, + false); + if (status < 0) + continue; + + /* check CRC */ + status = i2c_ptp_check_crc(chip, buf, size); + if (status < 0) + continue; + + status = size; + break; + } + +out: + i2c_ptp_ready(chip); + return status; +} + +/* + * If interrupts are used (signaled by an irq set in the vendor structure) + * tpm.c can skip polling for the data to be available as the interrupt is + * waited for here + */ +static int i2c_ptp_send_data(struct tpm_chip *chip, u8 *buf, size_t len) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + int rc, status, burstcnt, retries; + size_t count = 0, bytes2send; + u32 ordinal = be32_to_cpu(*((__be32 *)(buf + 6))); + u32 intmask; + + /* When NPCT7XX FU Mode command or startup (when TPM I2C may be has + * been reset), wait for STS_VALID and re-initialize I2C regs + */ + if (ordinal == 0x20000201 || ordinal == 0x144) { + // wait for I2C to be ready + if (wait_startup(chip, 0) != 0) + return -ENODEV; + + rc = i2c_ptp_enable_csum(chip); + if (rc < 0) { + dev_err(&chip->dev, "%s() fail to enable checksum\n", + __func__); + return rc; + } + + // in interrupt mode re-eanble and clear the interrupts + if (chip->flags & TPM_CHIP_FLAG_IRQ) { + intmask = (TPM_INT_TO_ACTIVATE | TPM_INT_GLOBAL); + i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4, + (u8 *)&intmask); + i2c_ptp_write_buf(client, TPM_INT_STATUS, 4, + (u8 *)&intmask); + } + } + + for (retries = 0; retries < TPM_I2C_MAX_RETRY_CNT; retries++) { + count = 0; + rc = -1; + + if (retries > 0) { + /* if this is not the first trial, abort the command */ + i2c_ptp_ready(chip); + } + + status = i2c_ptp_status(chip); + if ((status & TPM_STS_COMMAND_READY) == 0) { + i2c_ptp_ready(chip); + if (i2c_ptp_wait_for_tpm_stat + (chip, TPM_STS_COMMAND_READY, chip->timeout_b, + NULL, false) < 0) { + rc = -ETIME; + continue; + } + } + + while (count < len) { + burstcnt = i2c_ptp_get_burstcount(chip); + if (burstcnt < 0) { + dev_err(&chip->dev, "Unable to read burstcount\n"); + rc = burstcnt; + break; + } + + bytes2send = min_t(int, i2c_max_size, burstcnt); + bytes2send = min_t(int, bytes2send, (len - count)); + rc = i2c_ptp_write_buf(client, TPM_DATA_FIFO, + bytes2send, buf + count); + if (rc < 0) + break; + + count += bytes2send; + } + + if (rc < 0) + continue; + + if (i2c_ptp_wait_for_tpm_stat(chip, TPM_STS_VALID, + chip->timeout_a, NULL, + false) < 0) { + rc = -ETIME; + continue; + } + + /* check CRC */ + rc = i2c_ptp_check_crc(chip, buf, len); + if (rc < 0) + continue; + + return 0; + } + + i2c_ptp_ready(chip); + return rc; +} + +static void disable_interrupts(struct tpm_chip *chip) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + u32 intmask; + int rc; + + rc = i2c_ptp_read_buf(client, TPM_INT_ENABLE, 4, (u8 *)&intmask); + if (rc < 0) + intmask = 0; + + intmask &= ~TPM_INT_GLOBAL; + rc = i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4, (u8 *)&intmask); + if (rc < 0) + dev_err(&chip->dev, "%s() fail to write TPM_INT_ENABLE\n", + __func__); + + devm_free_irq(chip->dev.parent, priv->irq, chip); + priv->irq = 0; + chip->flags &= ~TPM_CHIP_FLAG_IRQ; +} + +/* + * If interrupts are used (signaled by an irq set in the vendor structure) + * tpm.c can skip polling for the data to be available as the interrupt is + * waited for here + */ +static int i2c_ptp_send_main(struct tpm_chip *chip, u8 *buf, size_t len) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + int rc; + u32 ordinal; + unsigned long dur; + u8 data; + + rc = i2c_ptp_send_data(chip, buf, len); + if (rc < 0) + return rc; + + /* go and do it */ + data = TPM_STS_GO; + rc = i2c_ptp_write_buf(client, TPM_STS, 1, &data); + if (rc < 0) + goto out_err; + + ordinal = be32_to_cpu(*((__be32 *)(buf + 6))); + + dur = tpm_calc_ordinal_duration(chip, ordinal); + if (i2c_ptp_wait_for_tpm_stat + (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur, + &priv->int_queue, false) < 0) { + rc = -ETIME; + goto out_err; + } + + return 0; +out_err: + i2c_ptp_ready(chip); + return rc; +} + +static int i2c_ptp_send(struct tpm_chip *chip, u8 *buf, size_t len) +{ + int rc, irq; + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + + if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested) + return i2c_ptp_send_main(chip, buf, len); + + /* Verify receipt of the expected IRQ */ + irq = priv->irq; + chip->flags &= ~TPM_CHIP_FLAG_IRQ; + rc = i2c_ptp_send_main(chip, buf, len); + priv->irq = irq; + chip->flags |= TPM_CHIP_FLAG_IRQ; + if (!priv->irq_tested) + tpm_msleep(1); + if (!priv->irq_tested) + disable_interrupts(chip); + priv->irq_tested = true; + return rc; +} + +static bool i2c_ptp_req_canceled(struct tpm_chip *chip, u8 status) +{ + return (status == TPM_STS_COMMAND_READY); +} + +/* The only purpose for the handler is to signal to any waiting threads that + * the interrupt is currently being asserted. The driver does not do any + * processing triggered by interrupts, and the chip provides no way to mask at + * the source (plus that would be slow over I2C). Run the IRQ as a one-shot, + * this means it cannot be shared. The processing of the interrupt status + * is done in i2c_ptp_wait_for_tpm_stat + */ +static irqreturn_t i2c_ptp_int_handler(int dummy, void *dev_id) +{ + struct tpm_chip *chip = dev_id; + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + + priv->irq_tested = true; + priv->intrs++; + wake_up_interruptible(&priv->int_queue); + disable_irq_nosync(priv->irq); + return IRQ_HANDLED; +} + +static int i2c_ptp_gen_interrupt(struct tpm_chip *chip) +{ + const char *desc = "attempting to generate an interrupt"; + u32 cap2; + + return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); +} + +/* Register the IRQ and issue a command that will cause an interrupt. If an + * irq is seen then leave the chip setup for IRQ operation, otherwise reverse + * everything and leave in polling mode. Returns 0 on success. + */ +static int i2c_ptp_probe_irq_single(struct tpm_chip *chip, u32 intmask, + int flags, int irq) +{ + struct i2c_client *client = to_i2c_client(chip->dev.parent); + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + int rc; + u32 int_support; + + if (devm_request_irq(chip->dev.parent, irq, i2c_ptp_int_handler, flags, + dev_name(&chip->dev), chip) != 0) { + dev_info(&chip->dev, "Unable to request irq: %d for probe\n", + irq); + return -1; + } + priv->irq = irq; + + chip->flags |= TPM_CHIP_FLAG_IRQ; + + /* check what are the interrupts that the TPM supports */ + rc = i2c_ptp_read_buf(client, TPM_INT_CAPABILITY, 4, (u8 *) + &int_support); + if (rc < 0) + return rc; + + /* Clear all existing */ + rc = i2c_ptp_write_buf(client, TPM_INT_STATUS, 4, (u8 *)&int_support); + if (rc < 0) + return rc; + + /* Turn on */ + int_support &= TPM_INT_TO_ACTIVATE; + int_support |= TPM_INT_GLOBAL; + rc = i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4, (u8 *)&int_support); + if (rc < 0) + return rc; + + priv->irq_tested = false; + + /* Generate an interrupt by having the core call through to + * i2c_ptp_send + */ + rc = i2c_ptp_gen_interrupt(chip); + if (rc < 0) + return rc; + + return 0; +} + +static int i2c_ptp_remove(struct i2c_client *client) +{ + u32 interrupt; + int rc; + + rc = i2c_ptp_read_buf(client, TPM_INT_ENABLE, 4, (u8 *)&interrupt); + if (rc < 0) + interrupt = 0; + + // Clear and disable interrupts + interrupt &= ~TPM_INT_GLOBAL; + rc = i2c_ptp_write_buf(client, TPM_INT_STATUS, 4, (u8 *)&interrupt); + rc = i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4, (u8 *)&interrupt); + + return rc; +} +EXPORT_SYMBOL_GPL(i2c_ptp_remove); + +static const struct tpm_class_ops tpm_i2c = { + .flags = TPM_OPS_AUTO_STARTUP, + .status = i2c_ptp_status, + .recv = i2c_ptp_recv, + .send = i2c_ptp_send, + .cancel = i2c_ptp_cancel, + .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, + .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, + .req_canceled = i2c_ptp_req_canceled, + .request_locality = i2c_ptp_request_locality, + .relinquish_locality = i2c_ptp_release_locality, +}; + +int i2c_ptp_core_init(struct i2c_client *client, struct tpm_tis_data *priv, + int irq, const struct tpm_tis_phy_ops *phy_ops, + acpi_handle acpi_dev_handle) +{ + u32 vendor; + u32 intmask; + u8 rid; + int rc; + struct tpm_chip *chip; + struct device *dev = &client->dev; + + chip = tpmm_chip_alloc(dev, &tpm_i2c); + if (IS_ERR(chip)) + return PTR_ERR(chip); + + chip->hwrng.quality = priv->rng_quality; + + /* Maximum timeouts */ + chip->timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A); + chip->timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B); + chip->timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C); + chip->timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D); + priv->phy_ops = phy_ops; + priv->intrs = 0; + dev_set_drvdata(&chip->dev, priv); + + if (wait_startup(chip, 0) != 0) { + rc = -ENODEV; + goto out_err; + } + + /* Take control of the TPM's interrupt hardware and shut it off */ + rc = i2c_ptp_read_buf(client, TPM_INT_ENABLE, 4, (u8 *)&intmask); + if (rc < 0) + goto out_err; + + intmask = TPM_INT_TO_ACTIVATE; // global should be off now + i2c_ptp_write_buf(client, TPM_INT_ENABLE, 4, (u8 *)&intmask); + + rc = i2c_ptp_enable_csum(chip); + if (rc < 0) { + dev_err(&chip->dev, "%s() fail to enable checksum\n", __func__); + rc = -ENODEV; + goto out_err; + } + + rc = tpm_chip_start(chip); + if (rc) + goto out_err; + + rc = tpm2_probe(chip); + if (rc) + goto out_err; + + rc = i2c_ptp_read_buf(client, TPM_VID_DID, 4, (u8 *)&vendor); + if (rc < 0) + goto out_err; + + priv->manufacturer_id = vendor; + + rc = i2c_ptp_read_buf(client, TPM_RID, 1, &rid); + if (rc < 0) + goto out_err; + + dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n", + (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2", + vendor >> 16, rid); + + /* INTERRUPT Setup */ + init_waitqueue_head(&priv->int_queue); + if (irq != -1) { + /* Before doing irq testing issue a command to the TPM in polling mode + * to make sure it works. May as well use that command to set the + * proper timeouts for the driver. + */ + if (tpm_get_timeouts(chip)) { + dev_err(dev, "Could not get TPM timeouts and durations\n"); + rc = -ENODEV; + goto out_err; + } + + i2c_ptp_probe_irq_single(chip, intmask, IRQF_TRIGGER_LOW, irq); + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) + dev_err(&chip->dev, FW_BUG + "TPM interrupt not working, polling instead\n"); + } + + rc = tpm_chip_register(chip); + if (rc) + goto out_err; + + return 0; + +out_err: + i2c_ptp_remove(client); + + return rc; +} + +static int i2c_ptp_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + int irq = -1, gpio; + struct tpm_tis_data *priv; + + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA)) + return -ENODEV; + + priv = devm_kzalloc(&client->dev, sizeof(struct tpm_tis_data), + GFP_KERNEL); + if (!priv) + return -ENOMEM; + + if (i2c_max_size < 1) + i2c_max_size = CONFIG_TCG_TIS_I2C_PTP_MAX_SIZE; + + // Get interrupt from board device + if (client->irq) { + irq = client->irq; + goto out; + } + + // If IRQ doesn't exists, get GPIO from device tree + gpio = of_get_named_gpio(client->dev.of_node, "tpm-pirq", 0); + if (gpio < 0) { + dev_err(&client->dev, "Failed to retrieve tpm-pirq\n"); + goto out; + } + + // GPIO request and configuration + if (devm_gpio_request_one(&client->dev, gpio, GPIOF_IN, "TPM PIRQ")) { + dev_err(&client->dev, "Failed to request tpm-pirq pin\n"); + goto out; + } + + irq = gpio_to_irq(gpio); + +out: + return i2c_ptp_core_init(client, priv, irq, NULL, 0); +} + +static const struct of_device_id of_i2c_ptp_match[] = { + {.compatible = "tcg,tpm_i2c_ptp", .data = OF_IS_TPM2}, + {}, +}; + +static const struct i2c_device_id i2c_ptp_id[] = { + {"tpm_i2c_ptp"}, + {"tpm2_i2c_ptp", .driver_data = I2C_IS_TPM2}, + {} +}; +MODULE_DEVICE_TABLE(i2c, i2c_ptp_id); + +static SIMPLE_DEV_PM_OPS(i2c_ptp_pm_ops, tpm_pm_suspend, tpm_pm_resume); + +static struct i2c_driver i2c_ptp_driver = { + .id_table = i2c_ptp_id, + .probe = i2c_ptp_probe, + .remove = i2c_ptp_remove, + .driver = { + .name = "tpm_i2c_ptp", + .pm = &i2c_ptp_pm_ops, + .of_match_table = of_match_ptr(of_i2c_ptp_match), + }, +}; +module_i2c_driver(i2c_ptp_driver); + +MODULE_AUTHOR("Oshri Alkoby (oshri.alkoby@nuvoton.com)"); +MODULE_DESCRIPTION("TPM I2C Driver"); +MODULE_VERSION("2.1.3"); +MODULE_LICENSE("GPL"); -- 2.18.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-06-28 15:13 [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp Oshri Alkoby 2019-06-28 15:13 ` [PATCH v2 1/2] dt-bindings: tpm: add the TPM I2C PTP device tree binding documentation Oshri Alkoby 2019-06-28 15:13 ` [PATCH v2 2/2] char: tpm: add new driver for tpm i2c ptp Oshri Alkoby @ 2019-07-04 8:43 ` Jarkko Sakkinen 2019-07-04 11:29 ` Alexander Steffen 2 siblings, 1 reply; 17+ messages in thread From: Jarkko Sakkinen @ 2019-07-04 8:43 UTC (permalink / raw) To: Oshri Alkoby, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby Cc: devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, dan.morav, tomer.maimon On Fri, 2019-06-28 at 18:13 +0300, Oshri Alkoby wrote: The long descriptions are still missing. Please take the time and write a proper commit messages that clearly tell what the patch does. Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements that spec so you should only implement a new physical layer. /Jarkko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-07-04 8:43 ` [PATCH v2 0/2] " Jarkko Sakkinen @ 2019-07-04 11:29 ` Alexander Steffen 2019-07-05 11:15 ` Jarkko Sakkinen [not found] ` <CAM9mBwJC2QD5-gV1eJUDzC2Fnnugr-oCZCoaH2sT_7ktFDkS-Q@mail.gmail.com> 0 siblings, 2 replies; 17+ messages in thread From: Alexander Steffen @ 2019-07-04 11:29 UTC (permalink / raw) To: Jarkko Sakkinen, Oshri Alkoby, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby Cc: devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, dan.morav, tomer.maimon On 04.07.2019 10:43, Jarkko Sakkinen wrote: > Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements > that spec so you should only implement a new physical layer. I had the same thought. Unfortunately, the I2C-TIS specification introduces two relevant changes compared to tpm_tis/tpm_tis_spi: 1. Locality is not encoded into register addresses anymore, but stored in a separate register. 2. Several register addresses have changed (but still contain compatible contents). I'd still prefer not to duplicate all the high-level logic from tpm_tis_core. But this will probably mean to introduce some new interfaces between tpm_tis_core and the physical layers. Also, shouldn't the new driver be called tpm_tis_i2c, to group it with all the other (TIS) drivers, that implement a vendor-independent protocol? With tpm_i2c_ptp users might assume that ptp is just another vendor. Alexander ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-07-04 11:29 ` Alexander Steffen @ 2019-07-05 11:15 ` Jarkko Sakkinen [not found] ` <CAM9mBwJC2QD5-gV1eJUDzC2Fnnugr-oCZCoaH2sT_7ktFDkS-Q@mail.gmail.com> 1 sibling, 0 replies; 17+ messages in thread From: Jarkko Sakkinen @ 2019-07-05 11:15 UTC (permalink / raw) To: Alexander Steffen, Oshri Alkoby, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby Cc: devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, dan.morav, tomer.maimon On Thu, 2019-07-04 at 13:29 +0200, Alexander Steffen wrote: > On 04.07.2019 10:43, Jarkko Sakkinen wrote: > > Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements > > that spec so you should only implement a new physical layer. > > I had the same thought. Unfortunately, the I2C-TIS specification > introduces two relevant changes compared to tpm_tis/tpm_tis_spi: I doubt that there was any comparison made. > 1. Locality is not encoded into register addresses anymore, but stored > in a separate register. > 2. Several register addresses have changed (but still contain compatible > contents). > > I'd still prefer not to duplicate all the high-level logic from > tpm_tis_core. But this will probably mean to introduce some new > interfaces between tpm_tis_core and the physical layers. Agreed. Some plumbing needs to be done in tpm_tis_core to make it work for this. We definitely do not want to duplicate code that has been field tested for years. > Also, shouldn't the new driver be called tpm_tis_i2c, to group it with > all the other (TIS) drivers, that implement a vendor-independent > protocol? With tpm_i2c_ptp users might assume that ptp is just another > vendor. Yes, absolutely. I guess the driver has been done without looking at what already exist in the TPM kernel stack. /Jarkko ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CAM9mBwJC2QD5-gV1eJUDzC2Fnnugr-oCZCoaH2sT_7ktFDkS-Q@mail.gmail.com>]
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp [not found] ` <CAM9mBwJC2QD5-gV1eJUDzC2Fnnugr-oCZCoaH2sT_7ktFDkS-Q@mail.gmail.com> @ 2019-07-05 11:28 ` Jarkko Sakkinen [not found] ` <6e7ff1b958d84f6e8e585fd3273ef295@NTILML02.nuvoton.com> 0 siblings, 1 reply; 17+ messages in thread From: Jarkko Sakkinen @ 2019-07-05 11:28 UTC (permalink / raw) To: Oshri Alkobi, Alexander Steffen Cc: robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, dan.morav, tomer.maimon On Thu, 2019-07-04 at 12:48 -0500, Oshri Alkobi wrote: > Alex, Jarkko, thank you very much for your feedbacks! Please configure your email client to use plain text. > I totally agree, there are some duplications that can be common, indeed it > will require some work in tpm_tis_core. > Since I believe it is not going to happen soon, I would suggest to examine > what duplications can currently be dropped from the new driver, so the kernel > will support the PTP I2C interface in the meantime. > I will appreciate getting ideas about any tpm_tis_core logic that currently > can be used as is by the new drive. I rather wait for a solution that integrates with our mature stack for TIS (or these days FIFO) than integrate something half-baked. If you want something in, please do right things right. What you are proposing would mean maintaining duplicate stacks forever. > Since the TIS is an old specification that mostly defines FIFO for TPM1.2 I > would say the name tpm_tis_i2c does not completely reflect its goal. However > we really don't have any problem with any name that the group will agree on. > Does tpm_ptp_i2c sound better than the current name? Absolutely not going to use that name. The naming convention is what it is for other drivers that are adapt tpm_tis_core to different HW interfaces. /Jarkko ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <6e7ff1b958d84f6e8e585fd3273ef295@NTILML02.nuvoton.com>]
[parent not found: <CAP6Zq1hPo9dG71YFyr7z9rjmi-DvoUZJOme4+2uqsfO+7nH+HQ@mail.gmail.com>]
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp [not found] ` <CAP6Zq1hPo9dG71YFyr7z9rjmi-DvoUZJOme4+2uqsfO+7nH+HQ@mail.gmail.com> @ 2019-07-15 9:45 ` Jarkko Sakkinen 2019-07-18 12:51 ` Eyal.Cohen 2019-07-17 7:48 ` Alexander Steffen 1 sibling, 1 reply; 17+ messages in thread From: Jarkko Sakkinen @ 2019-07-15 9:45 UTC (permalink / raw) To: Tomer Maimon Cc: Oshri Alkobi, Alexander Steffen, Rob Herring, Mark Rutland, peterhuewe, jgg, Arnd Bergmann, Greg KH, IS20 Oshri Alkoby, devicetree, AP MS30 Linux Kernel community, linux-integrity, gcwilson, kgoldman, nayna, IS30 Dan Morav, eyal.cohen On Mon, Jul 15, 2019 at 11:08:47AM +0300, Tomer Maimon wrote: > Thanks for your feedback and sorry for the late response. > > Due to the amount of work required to handle this technical feedback and > project constraints we need to put this task on hold for the near future. > > In the meantime, anyone from the community is welcome to take over this > code and handle the re-design for the benefit of the entire TPM community. Ok, so there is already driver for this called tpm_tis_core. So you go and create a new module, whose name given the framework of things that we already have deployed, is destined to be tpm_tis_i2c. Then you roughly implement a new physical layer by using a callback interface provided to you by tpm_tis_core. The so called re-design was already addressed by Alexander [1]. How hard can it be seriously? [1] https://lkml.org/lkml/2019/7/4/331 /Jarkko ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-07-15 9:45 ` Jarkko Sakkinen @ 2019-07-18 12:51 ` Eyal.Cohen 2019-07-18 17:10 ` Alexander Steffen 0 siblings, 1 reply; 17+ messages in thread From: Eyal.Cohen @ 2019-07-18 12:51 UTC (permalink / raw) To: jarkko.sakkinen, tmaimon77 Cc: oshrialkoby85, Alexander.Steffen, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, Dan.Morav, oren.tanami Hi Jarkko and Alexander, We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. At a minimum we thought of: 1. Handling TPM Localities in I2C is different 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry) Besides this, during development we might encounter additional differences between SPI and I2C. We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window. Regards, Eyal. -----Original Message----- From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Sent: Monday, July 15, 2019 12:46 PM To: Tomer Maimon <tmaimon77@gmail.com> Cc: Oshri Alkobi <oshrialkoby85@gmail.com>; Alexander Steffen <Alexander.Steffen@infineon.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; peterhuewe@gmx.de; jgg@ziepe.ca; Arnd Bergmann <arnd@arndb.de>; Greg KH <gregkh@linuxfoundation.org>; IS20 Oshri Alkoby <oshri.alkoby@nuvoton.com>; devicetree <devicetree@vger.kernel.org>; AP MS30 Linux Kernel community <linux-kernel@vger.kernel.org>; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; IS30 Dan Morav <Dan.Morav@nuvoton.com>; IS20 Eyal Cohen <Eyal.Cohen@nuvoton.com> Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp On Mon, Jul 15, 2019 at 11:08:47AM +0300, Tomer Maimon wrote: > Thanks for your feedback and sorry for the late response. > > Due to the amount of work required to handle this technical feedback and > project constraints we need to put this task on hold for the near future. > > In the meantime, anyone from the community is welcome to take over this > code and handle the re-design for the benefit of the entire TPM community. Ok, so there is already driver for this called tpm_tis_core. So you go and create a new module, whose name given the framework of things that we already have deployed, is destined to be tpm_tis_i2c. Then you roughly implement a new physical layer by using a callback interface provided to you by tpm_tis_core. The so called re-design was already addressed by Alexander [1]. How hard can it be seriously? [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2019_7_4_331&d=DwIBAg&c=ue8mO8zgC4VZ4q_aNVKt8G9MC01UFDmisvMR1k-EoDM&r=hjzIxEsS8wf3Ezr__r0ZOjw3kki8jIlQK-SQ5pWhXaM&m=aYs86sTFmxqvlI5rE2AWLGRl0lb9XRuiRKVtsCaXdRM&s=_3MMoq5UISFwMfK4OfgLbA2kMfZVgEfgkaWKqDEUXGw&e= /Jarkko =========================================================================================== The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-07-18 12:51 ` Eyal.Cohen @ 2019-07-18 17:10 ` Alexander Steffen 2019-07-30 8:39 ` Benoit HOUYERE 2019-08-15 17:03 ` Oshri Alkobi 0 siblings, 2 replies; 17+ messages in thread From: Alexander Steffen @ 2019-07-18 17:10 UTC (permalink / raw) To: Eyal.Cohen, jarkko.sakkinen, tmaimon77 Cc: oshrialkoby85, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, Dan.Morav, oren.tanami On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: > Hi Jarkko and Alexander, > > We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. Great :) In the meantime, I've done some experiments creating an I2C driver based on tpm_tis_core, see https://patchwork.kernel.org/patch/11049363/ Please have a look at that and provide your feedback (and/or use it as a basis for further implementations). > However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. > At a minimum we thought of: > 1. Handling TPM Localities in I2C is different It turned out not to be that different in the end, see the code mentioned above and my comment here: https://patchwork.kernel.org/cover/11049365/ > 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core That is completely optional, so there is no need to implement it in the beginning. Also, do you expect a huge benefit from that functionality? Are bit flips that much more likely on I2C compared to SPI, which has no CRC at all, but still works fine? > 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors Right, that seems similar to the cr50 issues (https://lkml.org/lkml/2019/7/17/677), so there should probably be a similar way to do it. > 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry) Optimizations are always welcome, but I'd expect basic communication to work already with the current code (though maybe not as efficiently as possible). > Besides this, during development we might encounter additional differences between SPI and I2C. > > We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window. > > Regards, > Eyal. ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-07-18 17:10 ` Alexander Steffen @ 2019-07-30 8:39 ` Benoit HOUYERE 2019-07-30 17:42 ` Alexander Steffen 2019-08-15 17:03 ` Oshri Alkobi 1 sibling, 1 reply; 17+ messages in thread From: Benoit HOUYERE @ 2019-07-30 8:39 UTC (permalink / raw) To: Alexander Steffen, Eyal.Cohen, jarkko.sakkinen, tmaimon77 Cc: oshrialkoby85, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, Dan.Morav, oren.tanami, Christophe Ricard (christophe.ricard@gmail.com), Elena WILLIS, Olivier COLLART Hi Alexander, Jarkko and Eyal, A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago. https://patchwork.kernel.org/patch/8628681/ At the time, we have had two concerns : 1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver. 2) Lots changing was already provided by tpm_tis_spi.c on 4.8. That's why Tpm_tis_i2c.c has been postponed. Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august). Best Regards, Benoit -----Original Message----- From: linux-integrity-owner@vger.kernel.org <linux-integrity-owner@vger.kernel.org> On Behalf Of Alexander Steffen Sent: jeudi 18 juillet 2019 19:10 To: Eyal.Cohen@nuvoton.com; jarkko.sakkinen@linux.intel.com; tmaimon77@gmail.com Cc: oshrialkoby85@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com; peterhuewe@gmx.de; jgg@ziepe.ca; arnd@arndb.de; gregkh@linuxfoundation.org; oshri.alkoby@nuvoton.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; Dan.Morav@nuvoton.com; oren.tanami@nuvoton.com Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: > Hi Jarkko and Alexander, > > We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. Great :) In the meantime, I've done some experiments creating an I2C driver based on tpm_tis_core, see https://patchwork.kernel.org/patch/11049363/ Please have a look at that and provide your feedback (and/or use it as a basis for further implementations). > However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. > At a minimum we thought of: > 1. Handling TPM Localities in I2C is different It turned out not to be that different in the end, see the code mentioned above and my comment here: https://patchwork.kernel.org/cover/11049365/ > 2. Handling I2C CRC - relevant only to I2C bus hence not supported > today by TIS core That is completely optional, so there is no need to implement it in the beginning. Also, do you expect a huge benefit from that functionality? Are bit flips that much more likely on I2C compared to SPI, which has no CRC at all, but still works fine? > 3. Handling Chip specific issues, since I2C implementation might be > slightly different across the various TPM vendors Right, that seems similar to the cr50 issues (https://lkml.org/lkml/2019/7/17/677), so there should probably be a similar way to do it. > 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according > the TCG Device Driver Guide (optimization on TPM_STS access and > send/recv retry) Optimizations are always welcome, but I'd expect basic communication to work already with the current code (though maybe not as efficiently as possible). > Besides this, during development we might encounter additional differences between SPI and I2C. > > We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window. > > Regards, > Eyal. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-07-30 8:39 ` Benoit HOUYERE @ 2019-07-30 17:42 ` Alexander Steffen 2019-09-06 12:16 ` Benoit HOUYERE 0 siblings, 1 reply; 17+ messages in thread From: Alexander Steffen @ 2019-07-30 17:42 UTC (permalink / raw) To: Benoit HOUYERE, Eyal.Cohen, jarkko.sakkinen, tmaimon77 Cc: oshrialkoby85, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, Dan.Morav, oren.tanami, Christophe Ricard (christophe.ricard@gmail.com), Elena WILLIS, Olivier COLLART Hi Benoit, good to see you're still around. On 30.07.2019 10:39, Benoit HOUYERE wrote: > Hi Alexander, Jarkko and Eyal, > > A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago. > > https://patchwork.kernel.org/patch/8628681/ Thanks for mentioning this. I forgot it exists, since it was still on the old mailing list. > At the time, we have had two concerns : > 1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver. > 2) Lots changing was already provided by tpm_tis_spi.c on 4.8. > > That's why Tpm_tis_i2c.c has been postponed. > > Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august). Could you run your tests against the simple implementation that I posted a while ago (https://patchwork.kernel.org/cover/11049365/) and provide your feedback? Since it is already based on the current tpm_tis_core, it is probably easier to integrate necessary changes there. By the way, has it gotten any easier in the meantime to get hold of your TPMs to use them for kernel tests? Alexander ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-07-30 17:42 ` Alexander Steffen @ 2019-09-06 12:16 ` Benoit HOUYERE 0 siblings, 0 replies; 17+ messages in thread From: Benoit HOUYERE @ 2019-09-06 12:16 UTC (permalink / raw) To: Alexander Steffen, Eyal.Cohen, jarkko.sakkinen, tmaimon77 Cc: oshrialkoby85, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, Dan.Morav, oren.tanami, Christophe Ricard (christophe.ricard@gmail.com), Elena WILLIS, Olivier COLLART Hi Steffen, We have performed test against your simple implementation. For basic test it was ok, however for stress test, your implementation does not take in account NACK and it failed. In PTP document, in chapter7.2.2.1.2 (Register write with address NACK), it indicates : "it's a good practice to repeat the current cycle using the correct I2C device address". In other implementation, https://patchwork.kernel.org/patch/8628681/ tpm_tis_i2c_read_bytes and tpm_tis_i2c_write_bytes handle NACK with a for loop. + for (i = 0; i < TPM_RETRY && ret < 0; i++) { + tpm_tis_i2c_sleep_guard_time(phy, TPM_I2C_SEND); + ret = i2c_master_send(phy->client, &i2c_reg, 1); + mod_timer(&phy->guard_timer, phy->guard_time); + } + + if (ret < 0) + goto exit; + + ret = -1; + for (i = 0; i < TPM_RETRY && ret < 0; i++) { + tpm_tis_i2c_sleep_guard_time(phy, TPM_I2C_RECV); + ret = i2c_master_recv(phy->client, result, len * size); + mod_timer(&phy->guard_timer, phy->guard_time); + } I think that we should implement it before to include tpm_tis_i2c.c officially. Thanks in advance, Best Regards, Benoit -----Original Message----- From: Alexander Steffen <Alexander.Steffen@infineon.com> Sent: mardi 30 juillet 2019 19:42 To: Benoit HOUYERE <benoit.houyere@st.com>; Eyal.Cohen@nuvoton.com; jarkko.sakkinen@linux.intel.com; tmaimon77@gmail.com Cc: oshrialkoby85@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com; peterhuewe@gmx.de; jgg@ziepe.ca; arnd@arndb.de; gregkh@linuxfoundation.org; oshri.alkoby@nuvoton.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; Dan.Morav@nuvoton.com; oren.tanami@nuvoton.com; Christophe Ricard (christophe.ricard@gmail.com) <christophe.ricard@gmail.com>; Elena WILLIS <elena.willis@st.com>; Olivier COLLART <olivier.collart@st.com> Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp Hi Benoit, good to see you're still around. On 30.07.2019 10:39, Benoit HOUYERE wrote: > Hi Alexander, Jarkko and Eyal, > > A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago. > > https://patchwork.kernel.org/patch/8628681/ Thanks for mentioning this. I forgot it exists, since it was still on the old mailing list. > At the time, we have had two concerns : > 1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver. > 2) Lots changing was already provided by tpm_tis_spi.c on 4.8. > > That's why Tpm_tis_i2c.c has been postponed. > > Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august). Could you run your tests against the simple implementation that I posted a while ago (https://patchwork.kernel.org/cover/11049365/) and provide your feedback? Since it is already based on the current tpm_tis_core, it is probably easier to integrate necessary changes there. By the way, has it gotten any easier in the meantime to get hold of your TPMs to use them for kernel tests? Alexander ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-07-18 17:10 ` Alexander Steffen 2019-07-30 8:39 ` Benoit HOUYERE @ 2019-08-15 17:03 ` Oshri Alkobi 2019-08-16 16:12 ` Alexander Steffen 1 sibling, 1 reply; 17+ messages in thread From: Oshri Alkobi @ 2019-08-15 17:03 UTC (permalink / raw) To: Alexander Steffen Cc: Eyal.Cohen, Jarkko Sakkinen, tmaimon77, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, Dan.Morav, oren.tanami, amir.mizinski On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen <Alexander.Steffen@infineon.com> wrote: > > On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: > > Hi Jarkko and Alexander, > > > > We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. > > Great :) In the meantime, I've done some experiments creating an I2C > driver based on tpm_tis_core, see > https://patchwork.kernel.org/patch/11049363/ Please have a look at that > and provide your feedback (and/or use it as a basis for further > implementations). > Sorry for the late response. Thanks Alexander, indeed it looks much simpler. I've checked it with Nuvoton's TPM - basic TPM commands work, I only had to remove the first msg from the read/write I2C transmitting (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in a sequence. Actually it is more efficient to set TPM_LOC_SEL only before locality check/request/relinquish - it is sticky. I still didn't manage to work with interrupts, will debug it. We weren't aware to the implementation of Christophe/ST which looks good and can be complement to yours. If no one is currently working on that, we can prepare a new patch that is based on both. Please let us know. > > However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. > > At a minimum we thought of: > > 1. Handling TPM Localities in I2C is different > > It turned out not to be that different in the end, see the code > mentioned above and my comment here: > https://patchwork.kernel.org/cover/11049365/ > > > 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core > > That is completely optional, so there is no need to implement it in the > beginning. Also, do you expect a huge benefit from that functionality? > Are bit flips that much more likely on I2C compared to SPI, which has no > CRC at all, but still works fine? > I2C is noisy bus with potentially more devices with larger variety than SPI. I2C may have more than one master and may have collisions and/or arbitration. Still we can start w/o CRC for the first stage and add it later. BTW, Christophe already did most of the work (https://patchwork.kernel.org/patch/8628661/) > > 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors > > Right, that seems similar to the cr50 issues > (https://lkml.org/lkml/2019/7/17/677), so there should probably be a > similar way to do it. Got it. We hope things will work for us without having another driver, but it's an option. > > > 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry) > > Optimizations are always welcome, but I'd expect basic communication to > work already with the current code (though maybe not as efficiently as > possible). > > > Besides this, during development we might encounter additional differences between SPI and I2C. > > > > We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window. > > > > Regards, > > Eyal. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-08-15 17:03 ` Oshri Alkobi @ 2019-08-16 16:12 ` Alexander Steffen 2019-08-25 11:25 ` Oshri Alkobi 0 siblings, 1 reply; 17+ messages in thread From: Alexander Steffen @ 2019-08-16 16:12 UTC (permalink / raw) To: Oshri Alkobi Cc: Eyal.Cohen, Jarkko Sakkinen, tmaimon77, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, Dan.Morav, oren.tanami, amir.mizinski On 15.08.2019 19:03, Oshri Alkobi wrote: > On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen > <Alexander.Steffen@infineon.com> wrote: >> >> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: >>> Hi Jarkko and Alexander, >>> >>> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. >> >> Great :) In the meantime, I've done some experiments creating an I2C >> driver based on tpm_tis_core, see >> https://patchwork.kernel.org/patch/11049363/ Please have a look at that >> and provide your feedback (and/or use it as a basis for further >> implementations). >> > > Sorry for the late response. > > Thanks Alexander, indeed it looks much simpler. > I've checked it with Nuvoton's TPM - basic TPM commands work Nice :) > I only > had to remove the first msg from the read/write I2C transmitting > (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in > a sequence. > Actually it is more efficient to set TPM_LOC_SEL only before locality > check/request/relinquish - it is sticky. There is one problem though: Do we assume that only the kernel driver will communicate with the TPM or might there be something else that also talks to the TPM? If it is only the kernel driver, we could probably skip setting TPM_LOC_SEL at all, since it defaults to 0 and the driver will never use anything else. If something else does its own I2C communication with the TPM, it might write different values to TPM_LOC_SEL at any time, causing the kernel driver to use a different locality than intended. This was the reason I always set TPM_LOC_SEL within the same transaction. > I still didn't manage to work with interrupts, will debug it. Interrupt support might be broken in general at the moment, see this thread: https://www.spinics.net/lists/linux-integrity/msg08663.html > We weren't aware to the implementation of Christophe/ST which looks > good and can be complement to yours. > If no one is currently working on that, we can prepare a new patch > that is based on both. > Please let us know. I won't have the time to do anything, at least for the next two weeks, so feel free to pick it up. >>> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. >>> At a minimum we thought of: >>> 1. Handling TPM Localities in I2C is different >> >> It turned out not to be that different in the end, see the code >> mentioned above and my comment here: >> https://patchwork.kernel.org/cover/11049365/ >> >>> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core >> >> That is completely optional, so there is no need to implement it in the >> beginning. Also, do you expect a huge benefit from that functionality? >> Are bit flips that much more likely on I2C compared to SPI, which has no >> CRC at all, but still works fine? >> > > I2C is noisy bus with potentially more devices with larger variety > than SPI. I2C may have more than one master and may have collisions > and/or arbitration. If multi-master usage is a concern, there are probably a lot more places in the driver that need to be adapted to deal with concurrent access/data corruption. For now, I'd assume a single master, similar to SPI. Alexander ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp 2019-08-16 16:12 ` Alexander Steffen @ 2019-08-25 11:25 ` Oshri Alkobi 0 siblings, 0 replies; 17+ messages in thread From: Oshri Alkobi @ 2019-08-25 11:25 UTC (permalink / raw) To: Alexander Steffen Cc: Eyal.Cohen, Jarkko Sakkinen, tmaimon77, robh+dt, mark.rutland, peterhuewe, jgg, arnd, gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity, gcwilson, kgoldman, nayna, Dan.Morav, oren.tanami, amir.mizinski On Fri, Aug 16, 2019 at 7:12 PM Alexander Steffen <Alexander.Steffen@infineon.com> wrote: > > On 15.08.2019 19:03, Oshri Alkobi wrote: > > On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen > > <Alexander.Steffen@infineon.com> wrote: > >> > >> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: > >>> Hi Jarkko and Alexander, > >>> > >>> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. > >> > >> Great :) In the meantime, I've done some experiments creating an I2C > >> driver based on tpm_tis_core, see > >> https://patchwork.kernel.org/patch/11049363/ Please have a look at that > >> and provide your feedback (and/or use it as a basis for further > >> implementations). > >> > > > > Sorry for the late response. > > > > Thanks Alexander, indeed it looks much simpler. > > I've checked it with Nuvoton's TPM - basic TPM commands work > > Nice :) > > > I only > > had to remove the first msg from the read/write I2C transmitting > > (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in > > a sequence. > > Actually it is more efficient to set TPM_LOC_SEL only before locality > > check/request/relinquish - it is sticky. > > There is one problem though: Do we assume that only the kernel driver > will communicate with the TPM or might there be something else that also > talks to the TPM? > > If it is only the kernel driver, we could probably skip setting > TPM_LOC_SEL at all, since it defaults to 0 and the driver will never use > anything else. If something else does its own I2C communication with the > TPM, it might write different values to TPM_LOC_SEL at any time, causing > the kernel driver to use a different locality than intended. This was > the reason I always set TPM_LOC_SEL within the same transaction. > > > I still didn't manage to work with interrupts, will debug it. > > Interrupt support might be broken in general at the moment, see this > thread: https://www.spinics.net/lists/linux-integrity/msg08663.html > > > We weren't aware to the implementation of Christophe/ST which looks > > good and can be complement to yours. > > If no one is currently working on that, we can prepare a new patch > > that is based on both. > > Please let us know. > > I won't have the time to do anything, at least for the next two weeks, > so feel free to pick it up. > Great, we will start working on it. > >>> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. > >>> At a minimum we thought of: > >>> 1. Handling TPM Localities in I2C is different > >> > >> It turned out not to be that different in the end, see the code > >> mentioned above and my comment here: > >> https://patchwork.kernel.org/cover/11049365/ > >> > >>> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core > >> > >> That is completely optional, so there is no need to implement it in the > >> beginning. Also, do you expect a huge benefit from that functionality? > >> Are bit flips that much more likely on I2C compared to SPI, which has no > >> CRC at all, but still works fine? > >> > > > > I2C is noisy bus with potentially more devices with larger variety > > than SPI. I2C may have more than one master and may have collisions > > and/or arbitration. > > If multi-master usage is a concern, there are probably a lot more places > in the driver that need to be adapted to deal with concurrent > access/data corruption. For now, I'd assume a single master, similar to SPI. > > Alexander Oshri ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp [not found] ` <CAP6Zq1hPo9dG71YFyr7z9rjmi-DvoUZJOme4+2uqsfO+7nH+HQ@mail.gmail.com> 2019-07-15 9:45 ` Jarkko Sakkinen @ 2019-07-17 7:48 ` Alexander Steffen 1 sibling, 0 replies; 17+ messages in thread From: Alexander Steffen @ 2019-07-17 7:48 UTC (permalink / raw) To: Tomer Maimon, Oshri Alkobi, Jarkko Sakkinen Cc: Rob Herring, Mark Rutland, peterhuewe, jgg, Arnd Bergmann, Greg KH, IS20 Oshri Alkoby, devicetree, AP MS30 Linux Kernel community, linux-integrity, gcwilson, kgoldman, nayna, IS30 Dan Morav, eyal.cohen On 15.07.2019 10:08, Tomer Maimon wrote: > Hi Jarkko and All, > > Thanks for your feedback and sorry for the late response. > > > Due to the amount of work required to handle this technical feedback and > project constraints we need to put this task on hold for the near future. > > In the meantime, anyone from the community is welcome to take over this > code and handle the re-design for the benefit of the entire TPM community. Too bad you lack the time to finish this. If somebody else were to pick it up, would it be possible for you to provide a couple of TPMs that implement this protocol, so that it can be properly tested? Alexander ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-09-06 12:16 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-28 15:13 [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp Oshri Alkoby
2019-06-28 15:13 ` [PATCH v2 1/2] dt-bindings: tpm: add the TPM I2C PTP device tree binding documentation Oshri Alkoby
2019-06-28 15:13 ` [PATCH v2 2/2] char: tpm: add new driver for tpm i2c ptp Oshri Alkoby
2019-07-04 8:43 ` [PATCH v2 0/2] " Jarkko Sakkinen
2019-07-04 11:29 ` Alexander Steffen
2019-07-05 11:15 ` Jarkko Sakkinen
[not found] ` <CAM9mBwJC2QD5-gV1eJUDzC2Fnnugr-oCZCoaH2sT_7ktFDkS-Q@mail.gmail.com>
2019-07-05 11:28 ` Jarkko Sakkinen
[not found] ` <6e7ff1b958d84f6e8e585fd3273ef295@NTILML02.nuvoton.com>
[not found] ` <CAP6Zq1hPo9dG71YFyr7z9rjmi-DvoUZJOme4+2uqsfO+7nH+HQ@mail.gmail.com>
2019-07-15 9:45 ` Jarkko Sakkinen
2019-07-18 12:51 ` Eyal.Cohen
2019-07-18 17:10 ` Alexander Steffen
2019-07-30 8:39 ` Benoit HOUYERE
2019-07-30 17:42 ` Alexander Steffen
2019-09-06 12:16 ` Benoit HOUYERE
2019-08-15 17:03 ` Oshri Alkobi
2019-08-16 16:12 ` Alexander Steffen
2019-08-25 11:25 ` Oshri Alkobi
2019-07-17 7:48 ` Alexander Steffen
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).