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

[Simon Glass]

Hi Christophe,

On 13 August 2015 at 14:22, Christophe Ricard
<christophe.ricard at 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 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_ ?

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