[U-Boot] [PATCH 06/25] tpm: Move the I2C TPM code into one file

[Simon Glass]

Hi Christophe,

On 11 August 2015 at 15:42, christophe.ricard
<christophe.ricard at 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 <sjg at 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