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-B29...).
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