Hi Christophe,
On 13 August 2015 at 14:22, Christophe Ricard christophe.ricard@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.ricard@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-B29...) 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