[U-Boot] [PATCH 00/25] dm: Convert TPM drivers to driver model

[Christophe Ricard]

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 at 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_ ?

Also, we will have TPM 2.0 would it be acceptable to build a second TPM 
class to support additional features ?
>
> 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
Christophe