Hi Christophe, On 13 August 2015 at 14:22, Christophe Ricard <christophe.ric...@gmail.com> wrote: > Hi Simon, > > Thanks for the review and your comments. > Please see mine below: > > > On 13/08/2015 03:30, Simon Glass wrote: >> >> Hi Christophe, >> >> On 11 August 2015 at 15:50, christophe.ricard >> <christophe.ric...@gmail.com> wrote: >>> >>> Hi Simon, >>> >>> I pretty much like the move to driver model for TPM. >>> However, i have some few remarks: >>> >>> The current i2c driver stick to Infineon TPMs and will not support any >>> other >>> vendors like ST(in my case). >>> The main reason for this is that there is no transport protocol over I2C >>> specification defined by the Trusted Computing Group for TPM1.2. >>> You can take a look at my release tentative here: >>> http://lists.denx.de/pipermail/u-boot/2015-August/222596.html >> >> Yes I agree it's probably better to rename it. One more patch... >> >>> The tpm.c file was delivering a way to ajust best the waiting time for >>> command duration to receive a command answer from a TPM command. It was >>> ported from Linux to u-boot. >>> 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 is defined by the TCG and followed by TPM vendors. In u-boot, this >>> is >>> used only by Infineon i2c driver but it could/should(?) be used >>> by all other drivers (i2c and lpc). >>> >>> In short, the idea is to keep the way TPM commands are transfered giving >>> the hands to drivers for handling the communication over a specified or >>> proprietary transport protocol. >>> >>> I fear the current approach would lead to duplicated codes on may TPM >>> drivers and 2 very differents kind of drivers (Linux/u-boot) very far >>> from >>> each other. >> >> In that case we should define what the interface is for the TPM. My >> approach is to provide a low-level interface which takes care of >> open/close, and sending and receiving bytes. >> >> Since that interface doesn't understand the actual commands it can't >> attach different timeouts to each. On the other hand as you say only >> one driver uses it. >> >> But since tpm_transmit() currently looks inside the packet, I don't >> see why the new xfer() method could not do that also. It removes one >> layer of itnerfaces. > > I think your approach is acceptable and simplifies the code as well. > I would use the send/recv low level interfaces in tis_xfer reproducing the > tpm_transmit behavior. >> >> >> Do all TPMs use the same commands and timeouts? > > My answer is yes with some few informations or highlights. > > When it goes with TPM commands, the litteratures is using term "duration". > Term "timeout" is used for a lower level layer that may not be used by all > TPMs (TIS). > > Durations are classified into 3 categories: short, medium and long. > Undefined is for non used Ordinals. > Each categories have a predefined value in the standard but TPM vendors are > free to modified(greater or lower) them according to their implementation. > Those values can/may be updated using a tpm_getcapability command requiring > at least a prior TPM_Startup. > I believe TPM_Startup and the tpm_getcapability(duration) could be executed > in tis_open. > > Just a nitpick, i believe prefix tis_ for the main TPM class functions may > not be appropriate. > What about tcg_ or tpm_ ?
Let's go with tpm then, at least it is clear. > > Also, we will have TPM 2.0 would it be acceptable to build a second TPM > class to support additional features ? Is it incompatible with the driver API we are talking about (open, close, send, recv)? >> >> >> In general Linux has ad-hoc interfaces for different things, but in >> U-Boot we are trying to standardise on driver model, so normally >> function pointers would end up implemented there. > > [...] > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
