Hi Christophe, On 13 August 2015 at 16:52, Simon Glass <s...@chromium.org> wrote: > 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. >> >> [...]
I have a rough crack at this (at u-boot-dm/tpm-working) but have not had a lot of time for this. It will likely be next week before I tidy up and resend the patches. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot