[U-Boot,06/25] tpm: Move the I2C TPM code into one file

The current Infineon I2C TPM driver is written in two parts, intended to
support use with other I2C devices. However we don't have any users and the
Atmel I2C TPM device does not use this file.

We should simplify this and remove the unused abstration. As a first step,
move the code into one file.

Also the name tpm_private.h suggests that the header file is generic to all
TPMs but it is not. Rename it indicate that it relates only to this driver

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/tpm/Makefile                         |   2 -
 drivers/tpm/tpm.c                            | 594 ---------------------------
 drivers/tpm/tpm_tis_i2c.c                    | 548 +++++++++++++++++++++++-
 drivers/tpm/{tpm_private.h => tpm_tis_i2c.h} |  24 +-
 4 files changed, 550 insertions(+), 618 deletions(-)
 delete mode 100644 drivers/tpm/tpm.c
 rename drivers/tpm/{tpm_private.h => tpm_tis_i2c.h} (73%)

Hi Simon,

I would basically disagree with this one.
The code from tpm.c you are merging into tpm_tis_i2c may not only be 
used by tpm_tis_i2c as it is using data from TPM standards (e.g: Trusted 
computing group) that should be used by other TPM drivers.
You can find in chapter 17 how the table tpm_protected_ordinal_duration, 
tpm_ordinal_duration were build. 
(https://www.trustedcomputinggroup.org/files/resource_files/E14876A3-1A4B-B294-D086297A1ED38F96/mainP2Structrev103.pdf).

This come from a Linux port.

As a result, we can imagine tis_sendrecv as a generic function where 
tpm_transmit manage the way tpm commands are sent following 
specification giving the hand to drivers thanks to the 
tpm_vendor_specific field {send,recv} for handling the communication 
over a specified or proprietary transport protocol.
As an example in tpm_tis_lpc, a 1s command duration might be too short 
or too long for some commands and might be really hard to debug in case 
someone decide to had a new TPM command support in u-boot.
Also 1s might be enought for the current commands or for evaluated TPM 
but it may require a longer duration for some other.
By reading the duration TPM capability, that will be just generic.

Also tpm_tis_i2c is Infineon specific and does not fit to any TCG 
standard for TPM1.2, i would recommand to rename this driver 
tpm_i2c_infineon to be inline with Linux naming and not confuse a future 
user on what it support.

At the end from my delivery tentative, a Linux port as well, you may see 
that i also rely on those functions. However I am not doing any check on 
the command duration. This is to be improved...

May you prefer an approach that would not lead to duplicated code ?

Best Regards
Christophe
On 11/08/2015 16:47, Simon Glass wrote:
> The current Infineon I2C TPM driver is written in two parts, intended to
> support use with other I2C devices. However we don't have any users and the
> Atmel I2C TPM device does not use this file.
>
> We should simplify this and remove the unused abstration. As a first step,
> move the code into one file.
>
> Also the name tpm_private.h suggests that the header file is generic to all
> TPMs but it is not. Rename it indicate that it relates only to this driver
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   drivers/tpm/Makefile                         |   2 -
>   drivers/tpm/tpm.c                            | 594 ---------------------------
>   drivers/tpm/tpm_tis_i2c.c                    | 548 +++++++++++++++++++++++-
>   drivers/tpm/{tpm_private.h => tpm_tis_i2c.h} |  24 +-
>   4 files changed, 550 insertions(+), 618 deletions(-)
>   delete mode 100644 drivers/tpm/tpm.c
>   rename drivers/tpm/{tpm_private.h => tpm_tis_i2c.h} (73%)
>
> diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile
> index 150570e..597966c 100644
> --- a/drivers/tpm/Makefile
> +++ b/drivers/tpm/Makefile
> @@ -3,9 +3,7 @@
>   # SPDX-License-Identifier:	GPL-2.0+
>   #
>   
> -# TODO: Merge tpm_tis_lpc.c with tpm.c
>   obj-$(CONFIG_TPM_ATMEL_TWI) += tpm_atmel_twi.o
> -obj-$(CONFIG_TPM_TIS_I2C) += tpm.o
>   obj-$(CONFIG_TPM_TIS_I2C) += tpm_tis_i2c.o
>   obj-$(CONFIG_TPM_TIS_LPC) += tpm_tis_lpc.o
>   obj-$(CONFIG_TPM_TIS_SANDBOX) += tpm_tis_sandbox.o
> diff --git a/drivers/tpm/tpm.c b/drivers/tpm/tpm.c
> deleted file mode 100644
> index 24997f6..0000000
> --- a/drivers/tpm/tpm.c
> +++ /dev/null
> @@ -1,594 +0,0 @@
> -/*
> - * Copyright (C) 2011 Infineon Technologies
> - *
> - * Authors:
> - * Peter Huewe <huewe.external@infineon.com>
> - *
> - * Description:
> - * Device driver for TCG/TCPA TPM (trusted platform module).
> - * Specifications at www.trustedcomputinggroup.org
> - *
> - * It is based on the Linux kernel driver tpm.c from Leendert van
> - * Dorn, Dave Safford, Reiner Sailer, and Kyleen Hall.
> - *
> - * Version: 2.1.1
> - *
> - * See file CREDITS for list of people who contributed to this
> - * project.
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License as
> - * published by the Free Software Foundation, version 2 of the
> - * License.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> - * MA 02111-1307 USA
> - */
> -
> -#include <config.h>
> -#include <common.h>
> -#include <dm.h>
> -#include <linux/compiler.h>
> -#include <fdtdec.h>
> -#include <i2c.h>
> -#include <tpm.h>
> -#include <asm-generic/errno.h>
> -#include <linux/types.h>
> -#include <linux/unaligned/be_byteshift.h>
> -
> -#include "tpm_private.h"
> -
> -DECLARE_GLOBAL_DATA_PTR;
> -
> -/* TPM configuration */
> -struct tpm {
> -	struct udevice *dev;
> -	char inited;
> -} tpm;
> -
> -/* Global structure for tpm chip data */
> -static struct tpm_chip g_chip;
> -
> -enum tpm_duration {
> -	TPM_SHORT = 0,
> -	TPM_MEDIUM = 1,
> -	TPM_LONG = 2,
> -	TPM_UNDEFINED,
> -};
> -
> -/* Extended error numbers from linux (see errno.h) */
> -#define ECANCELED	125	/* Operation Canceled */
> -
> -/* Timer frequency. Corresponds to msec timer resolution*/
> -#define HZ		1000
> -
> -#define TPM_MAX_ORDINAL			243
> -#define TPM_MAX_PROTECTED_ORDINAL	12
> -#define TPM_PROTECTED_ORDINAL_MASK	0xFF
> -
> -#define TPM_CMD_COUNT_BYTE	2
> -#define TPM_CMD_ORDINAL_BYTE	6
> -
> -/*
> - * Array with one entry per ordinal defining the maximum amount
> - * of time the chip could take to return the result.  The ordinal
> - * designation of short, medium or long is defined in a table in
> - * TCG Specification TPM Main Part 2 TPM Structures Section 17. The
> - * values of the SHORT, MEDIUM, and LONG durations are retrieved
> - * from the chip during initialization with a call to tpm_get_timeouts.
> - */
> -static const u8 tpm_protected_ordinal_duration[TPM_MAX_PROTECTED_ORDINAL] = {
> -	TPM_UNDEFINED,		/* 0 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 5 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 10 */
> -	TPM_SHORT,
> -};
> -
> -static const u8 tpm_ordinal_duration[TPM_MAX_ORDINAL] = {
> -	TPM_UNDEFINED,		/* 0 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 5 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 10 */
> -	TPM_SHORT,
> -	TPM_MEDIUM,
> -	TPM_LONG,
> -	TPM_LONG,
> -	TPM_MEDIUM,		/* 15 */
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_MEDIUM,
> -	TPM_LONG,
> -	TPM_SHORT,		/* 20 */
> -	TPM_SHORT,
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,
> -	TPM_SHORT,		/* 25 */
> -	TPM_SHORT,
> -	TPM_MEDIUM,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_MEDIUM,		/* 30 */
> -	TPM_LONG,
> -	TPM_MEDIUM,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,		/* 35 */
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_MEDIUM,		/* 40 */
> -	TPM_LONG,
> -	TPM_MEDIUM,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,		/* 45 */
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_LONG,
> -	TPM_MEDIUM,		/* 50 */
> -	TPM_MEDIUM,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 55 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_MEDIUM,		/* 60 */
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_MEDIUM,		/* 65 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 70 */
> -	TPM_SHORT,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 75 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_LONG,		/* 80 */
> -	TPM_UNDEFINED,
> -	TPM_MEDIUM,
> -	TPM_LONG,
> -	TPM_SHORT,
> -	TPM_UNDEFINED,		/* 85 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 90 */
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_UNDEFINED,		/* 95 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_MEDIUM,		/* 100 */
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 105 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 110 */
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,		/* 115 */
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_LONG,		/* 120 */
> -	TPM_LONG,
> -	TPM_MEDIUM,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,
> -	TPM_SHORT,		/* 125 */
> -	TPM_SHORT,
> -	TPM_LONG,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,		/* 130 */
> -	TPM_MEDIUM,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,
> -	TPM_MEDIUM,
> -	TPM_UNDEFINED,		/* 135 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 140 */
> -	TPM_SHORT,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 145 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 150 */
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_UNDEFINED,		/* 155 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 160 */
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 165 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_LONG,		/* 170 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 175 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_MEDIUM,		/* 180 */
> -	TPM_SHORT,
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,		/* 185 */
> -	TPM_SHORT,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 190 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 195 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 200 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,
> -	TPM_SHORT,		/* 205 */
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_MEDIUM,		/* 210 */
> -	TPM_UNDEFINED,
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,
> -	TPM_MEDIUM,
> -	TPM_UNDEFINED,		/* 215 */
> -	TPM_MEDIUM,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,
> -	TPM_SHORT,		/* 220 */
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_SHORT,
> -	TPM_UNDEFINED,		/* 225 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 230 */
> -	TPM_LONG,
> -	TPM_MEDIUM,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,		/* 235 */
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_UNDEFINED,
> -	TPM_SHORT,		/* 240 */
> -	TPM_UNDEFINED,
> -	TPM_MEDIUM,
> -};
> -
> -/* Returns max number of milliseconds to wait */
> -static unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip,
> -		u32 ordinal)
> -{
> -	int duration_idx = TPM_UNDEFINED;
> -	int duration = 0;
> -
> -	if (ordinal < TPM_MAX_ORDINAL) {
> -		duration_idx = tpm_ordinal_duration[ordinal];
> -	} else if ((ordinal & TPM_PROTECTED_ORDINAL_MASK) <
> -			TPM_MAX_PROTECTED_ORDINAL) {
> -		duration_idx = tpm_protected_ordinal_duration[
> -				ordinal & TPM_PROTECTED_ORDINAL_MASK];
> -	}
> -
> -	if (duration_idx != TPM_UNDEFINED)
> -		duration = chip->vendor.duration[duration_idx];
> -
> -	if (duration <= 0)
> -		return 2 * 60 * HZ; /* Two minutes timeout */
> -	else
> -		return duration;
> -}
> -
> -static ssize_t tpm_transmit(const unsigned char *buf, size_t bufsiz)
> -{
> -	int rc;
> -	u32 count, ordinal;
> -	unsigned long start, stop;
> -
> -	struct tpm_chip *chip = &g_chip;
> -
> -	/* switch endianess: big->little */
> -	count = get_unaligned_be32(buf + TPM_CMD_COUNT_BYTE);
> -	ordinal = get_unaligned_be32(buf + TPM_CMD_ORDINAL_BYTE);
> -
> -	if (count == 0) {
> -		error("no data\n");
> -		return -ENODATA;
> -	}
> -	if (count > bufsiz) {
> -		error("invalid count value %x %zx\n", count, bufsiz);
> -		return -E2BIG;
> -	}
> -
> -	debug("Calling send\n");
> -	rc = chip->vendor.send(chip, (u8 *)buf, count);
> -	debug("   ... done calling send\n");
> -	if (rc < 0) {
> -		error("tpm_transmit: tpm_send: error %d\n", rc);
> -		goto out;
> -	}
> -
> -	if (chip->vendor.irq)
> -		goto out_recv;
> -
> -	start = get_timer(0);
> -	stop = tpm_calc_ordinal_duration(chip, ordinal);
> -	do {
> -		debug("waiting for status... %ld %ld\n", start, stop);
> -		u8 status = chip->vendor.status(chip);
> -		if ((status & chip->vendor.req_complete_mask) ==
> -		    chip->vendor.req_complete_val) {
> -			debug("...got it;\n");
> -			goto out_recv;
> -		}
> -
> -		if (status == chip->vendor.req_canceled) {
> -			error("Operation Canceled\n");
> -			rc = -ECANCELED;
> -			goto out;
> -		}
> -		udelay(TPM_TIMEOUT * 1000);
> -	} while (get_timer(start) < stop);
> -
> -	chip->vendor.cancel(chip);
> -	error("Operation Timed out\n");
> -	rc = -ETIME;
> -	goto out;
> -
> -out_recv:
> -	debug("out_recv: reading response...\n");
> -	rc = chip->vendor.recv(chip, (u8 *)buf, TPM_BUFSIZE);
> -	if (rc < 0)
> -		error("tpm_transmit: tpm_recv: error %d\n", rc);
> -
> -out:
> -	return rc;
> -}
> -
> -static int tpm_open_dev(struct udevice *dev)
> -{
> -	int rc;
> -
> -	debug("%s: start\n", __func__);
> -	if (g_chip.is_open)
> -		return -EBUSY;
> -	rc = tpm_vendor_init(dev);
> -	if (rc < 0)
> -		g_chip.is_open = 0;
> -	return rc;
> -}
> -
> -static void tpm_close(void)
> -{
> -	if (g_chip.is_open) {
> -		tpm_vendor_cleanup(&g_chip);
> -		g_chip.is_open = 0;
> -	}
> -}
> -
> -/**
> - * Decode TPM configuration.
> - *
> - * @param dev	Returns a configuration of TPM device
> - * @return 0 if ok, -1 on error
> - */
> -static int tpm_decode_config(struct tpm *dev)
> -{
> -	const void *blob = gd->fdt_blob;
> -	struct udevice *bus;
> -	int chip_addr;
> -	int parent;
> -	int node;
> -	int ret;
> -
> -	node = fdtdec_next_compatible(blob, 0, COMPAT_INFINEON_SLB9635_TPM);
> -	if (node < 0) {
> -		node = fdtdec_next_compatible(blob, 0,
> -				COMPAT_INFINEON_SLB9645_TPM);
> -	}
> -	if (node < 0) {
> -		debug("%s: Node not found\n", __func__);
> -		return -1;
> -	}
> -	parent = fdt_parent_offset(blob, node);
> -	if (parent < 0) {
> -		debug("%s: Cannot find node parent\n", __func__);
> -		return -1;
> -	}
> -
> -	/*
> -	 * TODO(sjg@chromium.org): Remove this when driver model supports
> -	 * TPMs
> -	 */
> -	ret = uclass_get_device_by_of_offset(UCLASS_I2C, parent, &bus);
> -	if (ret) {
> -		debug("Cannot find bus for node '%s: ret=%d'\n",
> -		      fdt_get_name(blob, parent, NULL), ret);
> -		return ret;
> -	}
> -
> -	chip_addr = fdtdec_get_int(blob, node, "reg", -1);
> -	if (chip_addr == -1) {
> -		debug("Cannot find reg property for node '%s: ret=%d'\n",
> -		      fdt_get_name(blob, node, NULL), ret);
> -		return ret;
> -	}
> -	/*
> -	 * TODO(sjg@chromium.org): Older TPMs will need to use the older method
> -	 * in iic_tpm_read() so the offset length needs to be 0 here.
> -	 */
> -	ret = i2c_get_chip(bus, chip_addr, 1, &dev->dev);
> -	if (ret) {
> -		debug("Cannot find device for node '%s: ret=%d'\n",
> -		      fdt_get_name(blob, node, NULL), ret);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -struct tpm_chip *tpm_register_hardware(const struct tpm_vendor_specific *entry)
> -{
> -	struct tpm_chip *chip;
> -
> -	/* Driver specific per-device data */
> -	chip = &g_chip;
> -	memcpy(&chip->vendor, entry, sizeof(struct tpm_vendor_specific));
> -	chip->is_open = 1;
> -
> -	return chip;
> -}
> -
> -int tis_init(void)
> -{
> -	if (tpm.inited)
> -		return 0;
> -
> -	if (tpm_decode_config(&tpm))
> -		return -1;
> -
> -	debug("%s: done\n", __func__);
> -
> -	tpm.inited = 1;
> -
> -	return 0;
> -}
> -
> -int tis_open(void)
> -{
> -	int rc;
> -
> -	if (!tpm.inited)
> -		return -1;
> -
> -	rc = tpm_open_dev(tpm.dev);
> -
> -	return rc;
> -}
> -
> -int tis_close(void)
> -{
> -	if (!tpm.inited)
> -		return -1;
> -
> -	tpm_close();
> -
> -	return 0;
> -}
> -
> -int tis_sendrecv(const uint8_t *sendbuf, size_t sbuf_size,
> -		uint8_t *recvbuf, size_t *rbuf_len)
> -{
> -	int len;
> -	uint8_t buf[4096];
> -
> -	if (!tpm.inited)
> -		return -1;
> -
> -	if (sizeof(buf) < sbuf_size)
> -		return -1;
> -
> -	memcpy(buf, sendbuf, sbuf_size);
> -
> -	len = tpm_transmit(buf, sbuf_size);
> -
> -	if (len < 10) {
> -		*rbuf_len = 0;
> -		return -1;
> -	}
> -
> -	memcpy(recvbuf, buf, len);
> -	*rbuf_len = len;
> -
> -	return 0;
> -}
> diff --git a/drivers/tpm/tpm_tis_i2c.c b/drivers/tpm/tpm_tis_i2c.c
> index a43e80d..2b3ce8c 100644
> --- a/drivers/tpm/tpm_tis_i2c.c
> +++ b/drivers/tpm/tpm_tis_i2c.c
> @@ -30,7 +30,7 @@
>   #include <linux/types.h>
>   #include <linux/unaligned/be_byteshift.h>
>   
> -#include "tpm_private.h"
> +#include "tpm_tis_i2c.h"
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> @@ -95,6 +95,304 @@ static const char * const chip_name[] = {
>   #define	TPM_DATA_FIFO(l)		(0x0005 | ((l) << 4))
>   #define	TPM_DID_VID(l)			(0x0006 | ((l) << 4))
>   
> +enum tpm_duration {
> +	TPM_SHORT = 0,
> +	TPM_MEDIUM = 1,
> +	TPM_LONG = 2,
> +	TPM_UNDEFINED,
> +};
> +
> +/* Extended error numbers from linux (see errno.h) */
> +#define ECANCELED	125	/* Operation Canceled */
> +
> +/* Timer frequency. Corresponds to msec timer resolution*/
> +#define HZ		1000
> +
> +#define TPM_MAX_ORDINAL			243
> +#define TPM_MAX_PROTECTED_ORDINAL	12
> +#define TPM_PROTECTED_ORDINAL_MASK	0xFF
> +
> +#define TPM_CMD_COUNT_BYTE	2
> +#define TPM_CMD_ORDINAL_BYTE	6
> +
> +/*
> + * Array with one entry per ordinal defining the maximum amount
> + * of time the chip could take to return the result.  The ordinal
> + * designation of short, medium or long is defined in a table in
> + * TCG Specification TPM Main Part 2 TPM Structures Section 17. The
> + * values of the SHORT, MEDIUM, and LONG durations are retrieved
> + * from the chip during initialization with a call to tpm_get_timeouts.
> + */
> +static const u8 tpm_protected_ordinal_duration[TPM_MAX_PROTECTED_ORDINAL] = {
> +	TPM_UNDEFINED,		/* 0 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 5 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 10 */
> +	TPM_SHORT,
> +};
> +
> +static const u8 tpm_ordinal_duration[TPM_MAX_ORDINAL] = {
> +	TPM_UNDEFINED,		/* 0 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 5 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 10 */
> +	TPM_SHORT,
> +	TPM_MEDIUM,
> +	TPM_LONG,
> +	TPM_LONG,
> +	TPM_MEDIUM,		/* 15 */
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_MEDIUM,
> +	TPM_LONG,
> +	TPM_SHORT,		/* 20 */
> +	TPM_SHORT,
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,
> +	TPM_SHORT,		/* 25 */
> +	TPM_SHORT,
> +	TPM_MEDIUM,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_MEDIUM,		/* 30 */
> +	TPM_LONG,
> +	TPM_MEDIUM,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,		/* 35 */
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_MEDIUM,		/* 40 */
> +	TPM_LONG,
> +	TPM_MEDIUM,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,		/* 45 */
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_LONG,
> +	TPM_MEDIUM,		/* 50 */
> +	TPM_MEDIUM,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 55 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_MEDIUM,		/* 60 */
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_MEDIUM,		/* 65 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 70 */
> +	TPM_SHORT,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 75 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_LONG,		/* 80 */
> +	TPM_UNDEFINED,
> +	TPM_MEDIUM,
> +	TPM_LONG,
> +	TPM_SHORT,
> +	TPM_UNDEFINED,		/* 85 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 90 */
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_UNDEFINED,		/* 95 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_MEDIUM,		/* 100 */
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 105 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 110 */
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,		/* 115 */
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_LONG,		/* 120 */
> +	TPM_LONG,
> +	TPM_MEDIUM,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,
> +	TPM_SHORT,		/* 125 */
> +	TPM_SHORT,
> +	TPM_LONG,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,		/* 130 */
> +	TPM_MEDIUM,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,
> +	TPM_MEDIUM,
> +	TPM_UNDEFINED,		/* 135 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 140 */
> +	TPM_SHORT,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 145 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 150 */
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_UNDEFINED,		/* 155 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 160 */
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 165 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_LONG,		/* 170 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 175 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_MEDIUM,		/* 180 */
> +	TPM_SHORT,
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,		/* 185 */
> +	TPM_SHORT,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 190 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 195 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 200 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,
> +	TPM_SHORT,		/* 205 */
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_MEDIUM,		/* 210 */
> +	TPM_UNDEFINED,
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,
> +	TPM_MEDIUM,
> +	TPM_UNDEFINED,		/* 215 */
> +	TPM_MEDIUM,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,
> +	TPM_SHORT,		/* 220 */
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_SHORT,
> +	TPM_UNDEFINED,		/* 225 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 230 */
> +	TPM_LONG,
> +	TPM_MEDIUM,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,		/* 235 */
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_UNDEFINED,
> +	TPM_SHORT,		/* 240 */
> +	TPM_UNDEFINED,
> +	TPM_MEDIUM,
> +};
> +
> +/* TPM configuration */
> +struct tpm {
> +	struct udevice *dev;
> +	char inited;
> +} tpm;
> +
> +/* Global structure for tpm chip data */
> +static struct tpm_chip g_chip;
> +
>   /* Structure to store I2C TPM specific stuff */
>   struct tpm_dev {
>   	struct udevice *dev;
> @@ -595,3 +893,251 @@ void tpm_vendor_cleanup(struct tpm_chip *chip)
>   {
>   	release_locality(chip, chip->vendor.locality, 1);
>   }
> +
> +/* Returns max number of milliseconds to wait */
> +static unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip,
> +		u32 ordinal)
> +{
> +	int duration_idx = TPM_UNDEFINED;
> +	int duration = 0;
> +
> +	if (ordinal < TPM_MAX_ORDINAL) {
> +		duration_idx = tpm_ordinal_duration[ordinal];
> +	} else if ((ordinal & TPM_PROTECTED_ORDINAL_MASK) <
> +			TPM_MAX_PROTECTED_ORDINAL) {
> +		duration_idx = tpm_protected_ordinal_duration[
> +				ordinal & TPM_PROTECTED_ORDINAL_MASK];
> +	}
> +
> +	if (duration_idx != TPM_UNDEFINED)
> +		duration = chip->vendor.duration[duration_idx];
> +
> +	if (duration <= 0)
> +		return 2 * 60 * HZ; /* Two minutes timeout */
> +	else
> +		return duration;
> +}
> +
> +static ssize_t tpm_transmit(const unsigned char *buf, size_t bufsiz)
> +{
> +	int rc;
> +	u32 count, ordinal;
> +	unsigned long start, stop;
> +
> +	struct tpm_chip *chip = &g_chip;
> +
> +	/* switch endianess: big->little */
> +	count = get_unaligned_be32(buf + TPM_CMD_COUNT_BYTE);
> +	ordinal = get_unaligned_be32(buf + TPM_CMD_ORDINAL_BYTE);
> +
> +	if (count == 0) {
> +		error("no data\n");
> +		return -ENODATA;
> +	}
> +	if (count > bufsiz) {
> +		error("invalid count value %x %zx\n", count, bufsiz);
> +		return -E2BIG;
> +	}
> +
> +	debug("Calling send\n");
> +	rc = chip->vendor.send(chip, (u8 *)buf, count);
> +	debug("   ... done calling send\n");
> +	if (rc < 0) {
> +		error("tpm_transmit: tpm_send: error %d\n", rc);
> +		goto out;
> +	}
> +
> +	if (chip->vendor.irq)
> +		goto out_recv;
> +
> +	start = get_timer(0);
> +	stop = tpm_calc_ordinal_duration(chip, ordinal);
> +	do {
> +		debug("waiting for status... %ld %ld\n", start, stop);
> +		u8 status = chip->vendor.status(chip);
> +		if ((status & chip->vendor.req_complete_mask) ==
> +		    chip->vendor.req_complete_val) {
> +			debug("...got it;\n");
> +			goto out_recv;
> +		}
> +
> +		if (status == chip->vendor.req_canceled) {
> +			error("Operation Canceled\n");
> +			rc = -ECANCELED;
> +			goto out;
> +		}
> +		udelay(TPM_TIMEOUT * 1000);
> +	} while (get_timer(start) < stop);
> +
> +	chip->vendor.cancel(chip);
> +	error("Operation Timed out\n");
> +	rc = -ETIME;
> +	goto out;
> +
> +out_recv:
> +	debug("out_recv: reading response...\n");
> +	rc = chip->vendor.recv(chip, (u8 *)buf, TPM_BUFSIZE);
> +	if (rc < 0)
> +		error("tpm_transmit: tpm_recv: error %d\n", rc);
> +
> +out:
> +	return rc;
> +}
> +
> +static int tpm_open_dev(struct udevice *dev)
> +{
> +	int rc;
> +
> +	debug("%s: start\n", __func__);
> +	if (g_chip.is_open)
> +		return -EBUSY;
> +	rc = tpm_vendor_init(dev);
> +	if (rc < 0)
> +		g_chip.is_open = 0;
> +	return rc;
> +}
> +
> +static void tpm_close(void)
> +{
> +	if (g_chip.is_open) {
> +		tpm_vendor_cleanup(&g_chip);
> +		g_chip.is_open = 0;
> +	}
> +}
> +
> +/**
> + * Decode TPM configuration.
> + *
> + * @param dev	Returns a configuration of TPM device
> + * @return 0 if ok, -1 on error
> + */
> +static int tpm_decode_config(struct tpm *dev)
> +{
> +	const void *blob = gd->fdt_blob;
> +	struct udevice *bus;
> +	int chip_addr;
> +	int parent;
> +	int node;
> +	int ret;
> +
> +	node = fdtdec_next_compatible(blob, 0, COMPAT_INFINEON_SLB9635_TPM);
> +	if (node < 0) {
> +		node = fdtdec_next_compatible(blob, 0,
> +				COMPAT_INFINEON_SLB9645_TPM);
> +	}
> +	if (node < 0) {
> +		debug("%s: Node not found\n", __func__);
> +		return -1;
> +	}
> +	parent = fdt_parent_offset(blob, node);
> +	if (parent < 0) {
> +		debug("%s: Cannot find node parent\n", __func__);
> +		return -1;
> +	}
> +
> +	/*
> +	 * TODO(sjg@chromium.org): Remove this when driver model supports
> +	 * TPMs
> +	 */
> +	ret = uclass_get_device_by_of_offset(UCLASS_I2C, parent, &bus);
> +	if (ret) {
> +		debug("Cannot find bus for node '%s: ret=%d'\n",
> +		      fdt_get_name(blob, parent, NULL), ret);
> +		return ret;
> +	}
> +
> +	chip_addr = fdtdec_get_int(blob, node, "reg", -1);
> +	if (chip_addr == -1) {
> +		debug("Cannot find reg property for node '%s: ret=%d'\n",
> +		      fdt_get_name(blob, node, NULL), ret);
> +		return ret;
> +	}
> +	/*
> +	 * TODO(sjg@chromium.org): Older TPMs will need to use the older method
> +	 * in iic_tpm_read() so the offset length needs to be 0 here.
> +	 */
> +	ret = i2c_get_chip(bus, chip_addr, 1, &dev->dev);
> +	if (ret) {
> +		debug("Cannot find device for node '%s: ret=%d'\n",
> +		      fdt_get_name(blob, node, NULL), ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +struct tpm_chip *tpm_register_hardware(const struct tpm_vendor_specific *entry)
> +{
> +	struct tpm_chip *chip;
> +
> +	/* Driver specific per-device data */
> +	chip = &g_chip;
> +	memcpy(&chip->vendor, entry, sizeof(struct tpm_vendor_specific));
> +	chip->is_open = 1;
> +
> +	return chip;
> +}
> +
> +int tis_init(void)
> +{
> +	if (tpm.inited)
> +		return 0;
> +
> +	if (tpm_decode_config(&tpm))
> +		return -1;
> +
> +	debug("%s: done\n", __func__);
> +
> +	tpm.inited = 1;
> +
> +	return 0;
> +}
> +
> +int tis_open(void)
> +{
> +	int rc;
> +
> +	if (!tpm.inited)
> +		return -1;
> +
> +	rc = tpm_open_dev(tpm.dev);
> +
> +	return rc;
> +}
> +
> +int tis_close(void)
> +{
> +	if (!tpm.inited)
> +		return -1;
> +
> +	tpm_close();
> +
> +	return 0;
> +}
> +
> +int tis_sendrecv(const uint8_t *sendbuf, size_t sbuf_size,
> +		uint8_t *recvbuf, size_t *rbuf_len)
> +{
> +	int len;
> +	uint8_t buf[4096];
> +
> +	if (!tpm.inited)
> +		return -1;
> +
> +	if (sizeof(buf) < sbuf_size)
> +		return -1;
> +
> +	memcpy(buf, sendbuf, sbuf_size);
> +
> +	len = tpm_transmit(buf, sbuf_size);
> +
> +	if (len < 10) {
> +		*rbuf_len = 0;
> +		return -1;
> +	}
> +
> +	memcpy(recvbuf, buf, len);
> +	*rbuf_len = len;
> +
> +	return 0;
> +}
> diff --git a/drivers/tpm/tpm_private.h b/drivers/tpm/tpm_tis_i2c.h
> similarity index 73%
> rename from drivers/tpm/tpm_private.h
> rename to drivers/tpm/tpm_tis_i2c.h
> index daaf8b8..75fa829 100644
> --- a/drivers/tpm/tpm_private.h
> +++ b/drivers/tpm/tpm_tis_i2c.h
> @@ -13,28 +13,11 @@
>    * It is based on the Linux kernel driver tpm.c from Leendert van
>    * Dorn, Dave Safford, Reiner Sailer, and Kyleen Hall.
>    *
> - *
> - * See file CREDITS for list of people who contributed to this
> - * project.
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License as
> - * published by the Free Software Foundation, version 2 of the
> - * License.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> - * MA 02111-1307 USA
> + * SPDX-License-Identifier:	GPL-2.0
>    */
>   
> -#ifndef _TPM_PRIVATE_H_
> -#define _TPM_PRIVATE_H_
> +#ifndef _TPM_TIS_I2C_H
> +#define _TPM_TIS_I2C_H
>   
>   #include <linux/compiler.h>
>   #include <linux/types.h>
> @@ -134,5 +117,4 @@ int tpm_vendor_init(struct udevice *dev);
>   
>   void tpm_vendor_cleanup(struct tpm_chip *chip);
>   
> -
>   #endif

Hi Christophe,

On 11 August 2015 at 15:42, christophe.ricard
<christophe.ricard@gmail.com> wrote:
> Hi Simon,
>
> I would basically disagree with this one.
> The code from tpm.c you are merging into tpm_tis_i2c may not only be used by
> tpm_tis_i2c as it is using data from TPM standards (e.g: Trusted computing
> group) that should be used by other TPM drivers.
> You can find in chapter 17 how the table tpm_protected_ordinal_duration,
> tpm_ordinal_duration were build.
> (https://www.trustedcomputinggroup.org/files/resource_files/E14876A3-1A4B-B294-D086297A1ED38F96/mainP2Structrev103.pdf).
>
> This come from a Linux port.
>
> As a result, we can imagine tis_sendrecv as a generic function where
> tpm_transmit manage the way tpm commands are sent following specification
> giving the hand to drivers thanks to the tpm_vendor_specific field
> {send,recv} for handling the communication over a specified or proprietary
> transport protocol.
> As an example in tpm_tis_lpc, a 1s command duration might be too short or
> too long for some commands and might be really hard to debug in case someone
> decide to had a new TPM command support in u-boot.
> Also 1s might be enought for the current commands or for evaluated TPM but
> it may require a longer duration for some other.
> By reading the duration TPM capability, that will be just generic.

But this code is only used by the Infineon driver.

>
> Also tpm_tis_i2c is Infineon specific and does not fit to any TCG standard
> for TPM1.2, i would recommand to rename this driver tpm_i2c_infineon to be
> inline with Linux naming and not confuse a future user on what it support.

Yes we should do that.

>
> At the end from my delivery tentative, a Linux port as well, you may see
> that i also rely on those functions. However I am not doing any check on the
> command duration. This is to be improved...
>
> May you prefer an approach that would not lead to duplicated code ?

I wonder if the timing of each command can be attached to the uclass
and handled there. We might have a function like:

tpm_xfer()

which sends data to the TPM and receives a rseponse. This can be
implemented by the uclass.

Then the driver interface (which I currently have as xfer()) could be
send() and receive(). The time delay measurement could happen in the
uclass.

So in summary:

tpm-uclass.c:
tpm_xfer()
   - calls driver->send()
   - checks timeout based on data supplied by the driver
    -calls driver->receive()
   - checks timeout based on data supplied by the driver
    - returns result

Then the drivers only need to implement simple send and receive functions.

>
> Best Regards
> Christophe
>
> On 11/08/2015 16:47, Simon Glass wrote:
>>
>> The current Infineon I2C TPM driver is written in two parts, intended to
>> support use with other I2C devices. However we don't have any users and
>> the
>> Atmel I2C TPM device does not use this file.
>>
>> We should simplify this and remove the unused abstration. As a first step,
>> move the code into one file.
>>
>> Also the name tpm_private.h suggests that the header file is generic to
>> all
>> TPMs but it is not. Rename it indicate that it relates only to this driver
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>   drivers/tpm/Makefile                         |   2 -
>>   drivers/tpm/tpm.c                            | 594
>> ---------------------------
>>   drivers/tpm/tpm_tis_i2c.c                    | 548
>> +++++++++++++++++++++++-
>>   drivers/tpm/{tpm_private.h => tpm_tis_i2c.h} |  24 +-
>>   4 files changed, 550 insertions(+), 618 deletions(-)
>>   delete mode 100644 drivers/tpm/tpm.c
>>   rename drivers/tpm/{tpm_private.h => tpm_tis_i2c.h} (73%)
>>
[snip]

Regards,
Simon

Hi Simon,

On 13/08/2015 03:30, Simon Glass wrote:
> Hi Christophe,
>
> On 11 August 2015 at 15:42, christophe.ricard
> <christophe.ricard@gmail.com> wrote:
>> Hi Simon,
>>
>> I would basically disagree with this one.
>> The code from tpm.c you are merging into tpm_tis_i2c may not only be used by
>> tpm_tis_i2c as it is using data from TPM standards (e.g: Trusted computing
>> group) that should be used by other TPM drivers.
>> You can find in chapter 17 how the table tpm_protected_ordinal_duration,
>> tpm_ordinal_duration were build.
>> (https://www.trustedcomputinggroup.org/files/resource_files/E14876A3-1A4B-B294-D086297A1ED38F96/mainP2Structrev103.pdf).
>>
>> This come from a Linux port.
>>
>> As a result, we can imagine tis_sendrecv as a generic function where
>> tpm_transmit manage the way tpm commands are sent following specification
>> giving the hand to drivers thanks to the tpm_vendor_specific field
>> {send,recv} for handling the communication over a specified or proprietary
>> transport protocol.
>> As an example in tpm_tis_lpc, a 1s command duration might be too short or
>> too long for some commands and might be really hard to debug in case someone
>> decide to had a new TPM command support in u-boot.
>> Also 1s might be enought for the current commands or for evaluated TPM but
>> it may require a longer duration for some other.
>> By reading the duration TPM capability, that will be just generic.
> But this code is only used by the Infineon driver.
My idea is to standardize the way a tpm does a command transfer in u-boot.
The approach is applicable to all the existing u-boot tpm drivers.
>
>> Also tpm_tis_i2c is Infineon specific and does not fit to any TCG standard
>> for TPM1.2, i would recommand to rename this driver tpm_i2c_infineon to be
>> inline with Linux naming and not confuse a future user on what it support.
> Yes we should do that.
>
>> At the end from my delivery tentative, a Linux port as well, you may see
>> that i also rely on those functions. However I am not doing any check on the
>> command duration. This is to be improved...
>>
>> May you prefer an approach that would not lead to duplicated code ?
> I wonder if the timing of each command can be attached to the uclass
> and handled there. We might have a function like:
>
> tpm_xfer()
>
> which sends data to the TPM and receives a rseponse. This can be
> implemented by the uclass.
>
> Then the driver interface (which I currently have as xfer()) could be
> send() and receive(). The time delay measurement could happen in the
> uclass.
>
> So in summary:
>
> tpm-uclass.c:
> tpm_xfer()
>     - calls driver->send()
>     - checks timeout based on data supplied by the driver
>      -calls driver->receive()
>     - checks timeout based on data supplied by the driver
>      - returns result
>
> Then the drivers only need to implement simple send and receive functions.
I agree with this approach.

[...]

Best REgards
Christophe