Hi Christophe, On 11 August 2015 at 15:42, christophe.ricard <christophe.ric...@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 <s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot