Add Infineon v05 TPM driver

This is the preliminary v05 TPM driver from Infineon. With a wrapper layer added to auto detect v03 and v05 chips. This is a version that just work. Cleanup will start after the initial push.


BUG=chromium-os:10497
TEST=Manual

Build
1. build a compatible lib first
   emerge-tegra2_seaboard vboot_reference-firmware
2. use MAKEALL script to check all cfg
   CROSS_COMPILE=armv7a-cros-linux-gnueabi- VBOOT=/build/tegra2_seaboard/usr ./MAKEALL chromeos
3. check result. 25 passed, no warning, no error

u-boot command
1. build a compatible version
   USE-debug VBOOT_DEBUG=1
emerge-tegra2_seaboard -av vboot_reference-firmware chromeos-u-boot-next
2. flash it using proper tools
   [board specific]
3. reset system and boot into command prompt
4. test with cros_tpm_test command sets:
   cros_tpm_test enable
   cros_tpm_test fast_enable
   cros_tpm_test startup
   cros_tpm_test timing

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=10a6dde

[Che-Liang Chiou, 2011-03-14 06:29:46]

Hi Rong,

LGTM with nits, but please wait Luigi's LGTM.

It is a pretty big CL and I don't fully understand its context. So please divide
it into separate CLs if possible next time.

[Che-Liang Chiou, 2011-03-14 08:49:54]

Hi Rong,

I forgot to publish my comments. Please see below.

http://codereview.chromium.org/6683023/diff/1/common/cmd_vboot.c
File common/cmd_vboot.c (right):

http://codereview.chromium.org/6683023/diff/1/common/cmd_vboot.c#newcode31
common/cmd_vboot.c:31: return TPM_SUCCESS;
Logic here is a little bit different. (When result == TPM_XXX, old logic returns
TPM_XXX, new logic returns TPM_SUCCESS) Would you mind explain why the logic
changed here?

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/Makefile
File drivers/tpm/Makefile (right):

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/Makefile#newcode1
drivers/tpm/Makefile:1: include $(TOPDIR)/config.mk
Copyright header

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/Makefile#newcode22
drivers/tpm/Makefile:22: EXTRA_CFLAGS += -I. -I$(TOPDIR)/include
Is "-I${TOPDIR}/include" necessary?

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
File drivers/tpm/slb9635_i2c/compatibility.c (right):

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.c:16: struct device_private p = {
Should we add a "static" prefix here?

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.c:21: struct device d = {
Same as above.

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.c:34: unsigned long jiffies;
Same as above.

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
File drivers/tpm/slb9635_i2c/compatibility.h (right):

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.h:67: (void)(&__dummy == &__dummy2); \
Is it void or void*?

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.h:68: 1; \
Does this mean typecheck() alway be evaluated to 1?

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.h:75: ((long)(b) - (long)(a) < 0))
Why not ((unsigned long)(b) < (unsigned long)(c)) ?

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/ifx_v03.c
File drivers/tpm/slb9635_i2c/ifx_v03.c (right):

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/ifx_v03...
drivers/tpm/slb9635_i2c/ifx_v03.c:14: }
Where is tpm_init_v03 and tpm_sendrecv_v03?

http://codereview.chromium.org/6683023/diff/1/lib/chromeos/tlcl_stub.c
File lib/chromeos/tlcl_stub.c (right):

http://codereview.chromium.org/6683023/diff/1/lib/chromeos/tlcl_stub.c#newcode2
lib/chromeos/tlcl_stub.c:2: * Copyright 2010, Google Inc.
Please use "chromium os authors..." copyright header.

[rongchang, 2011-03-23 11:44:57]

Merged with Infineon's new U-Boot driver release.

Next time I will try to divide it into smaller CLs.

http://codereview.chromium.org/6683023/diff/1/common/cmd_vboot.c
File common/cmd_vboot.c (right):

http://codereview.chromium.org/6683023/diff/1/common/cmd_vboot.c#newcode31
common/cmd_vboot.c:31: return TPM_SUCCESS;
On 2011/03/14 08:49:54, Che-Liang Chiou wrote:
> Logic here is a little bit different. (When result == TPM_XXX, old logic
returns
> TPM_XXX, new logic returns TPM_SUCCESS) Would you mind explain why the logic
> changed here?

The TlclStartupIfNeeded's return value can be ignored.
Previous result check would stop exec in TTPM_CHECK macro.
And you are right. I should keep the logic here and change
TTPM_CHECK macro.

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/Makefile
File drivers/tpm/Makefile (right):

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/Makefile#newcode1
drivers/tpm/Makefile:1: include $(TOPDIR)/config.mk
On 2011/03/14 08:49:54, Che-Liang Chiou wrote:
> Copyright header

Done.

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/Makefile#newcode22
drivers/tpm/Makefile:22: EXTRA_CFLAGS += -I. -I$(TOPDIR)/include
On 2011/03/14 08:49:54, Che-Liang Chiou wrote:
> Is "-I${TOPDIR}/include" necessary?

Done.
No, not necessary.

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
File drivers/tpm/slb9635_i2c/compatibility.c (right):

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.c:16: struct device_private p = {
On 2011/03/14 08:49:54, Che-Liang Chiou wrote:
> Should we add a "static" prefix here?

Same as compatibility.h. But this file was removed from Infineon's latest
driver.

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
File drivers/tpm/slb9635_i2c/compatibility.h (right):

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.h:67: (void)(&__dummy == &__dummy2); \
On 2011/03/14 08:49:54, Che-Liang Chiou wrote:
> Is it void or void*?

This file is provided by Infineon. Hence will be kept as is.

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.h:68: 1; \
On 2011/03/14 08:49:54, Che-Liang Chiou wrote:
> Does this mean typecheck() alway be evaluated to 1?

Same as above.

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/compati...
drivers/tpm/slb9635_i2c/compatibility.h:75: ((long)(b) - (long)(a) < 0))
On 2011/03/14 08:49:54, Che-Liang Chiou wrote:
> Why not ((unsigned long)(b) < (unsigned long)(c)) ?

Same as above.

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/ifx_v03.c
File drivers/tpm/slb9635_i2c/ifx_v03.c (right):

http://codereview.chromium.org/6683023/diff/1/drivers/tpm/slb9635_i2c/ifx_v03...
drivers/tpm/slb9635_i2c/ifx_v03.c:14: }
On 2011/03/14 08:49:54, Che-Liang Chiou wrote:
> Where is tpm_init_v03 and tpm_sendrecv_v03?

in tpm_proto.c

http://codereview.chromium.org/6683023/diff/1/lib/chromeos/tlcl_stub.c
File lib/chromeos/tlcl_stub.c (right):

http://codereview.chromium.org/6683023/diff/1/lib/chromeos/tlcl_stub.c#newcode2
lib/chromeos/tlcl_stub.c:2: * Copyright 2010, Google Inc.
On 2011/03/14 08:49:54, Che-Liang Chiou wrote:
> Please use "chromium os authors..." copyright header.

Done.

[rongchang, 2011-03-23 11:53:13]

Infineon will send the driver to upstream. But before that, we need it in
current U-Boot. The files from Infineon are:
tpm.c
tpm.h
tpm_tis_i2c.c
compatibility.h
tddl.c
tddl.h

On 2011/03/23 11:44:57, rongchang wrote:
> Merged with Infineon's new U-Boot driver release.
>

[Che-Liang Chiou, 2011-03-24 07:41:49]

LGTM. Assuming that infineon's upstreamed codes are the same as they are here.

On 2011/03/23 11:53:13, rongchang wrote:
> Infineon will send the driver to upstream. But before that, we need it in
> current U-Boot. The files from Infineon are:
> tpm.c
> tpm.h
> tpm_tis_i2c.c
> compatibility.h
> tddl.c
> tddl.h
> 
> On 2011/03/23 11:44:57, rongchang wrote:
> > Merged with Infineon's new U-Boot driver release.
> >

[Luigi Semenzato, 2011-03-24 22:17:44]

LGTM

Rong, this is good (with a couple of nits below).  If Infineon has succeeded in
upstreaming their driver, can you make sure that this is consistent with their
version?  If not, just go ahead and commit it, with the understanding that we
will have to re-merge our changes when they upstream.  In any case it's good to
support both v03 and v05 because we still have a lot of the former around, and
we will for a while.

Sorry for the delay.

http://codereview.chromium.org/6683023/diff/7001/common/cmd_vboot.c
File common/cmd_vboot.c (right):

http://codereview.chromium.org/6683023/diff/7001/common/cmd_vboot.c#newcode33
common/cmd_vboot.c:33: 
Why did you change the semantics here?  If we get other kinds of errors, we may
want to propagate them.

http://codereview.chromium.org/6683023/diff/7001/include/configs/chromeos/teg...
File include/configs/chromeos/tegra2/aebl/parts/tpm.h (right):

http://codereview.chromium.org/6683023/diff/7001/include/configs/chromeos/teg...
include/configs/chromeos/tegra2/aebl/parts/tpm.h:12: #define
CONFIG_INFINEON_TPM_I2C_BUS		(2)
I wouldn't use parentheses here (and below).
They don't hurt, but they don't help either.

[rongchang, 2011-03-29 07:08:03]

Thanks a lot for the review.

http://codereview.chromium.org/6683023/diff/7001/common/cmd_vboot.c
File common/cmd_vboot.c (right):

http://codereview.chromium.org/6683023/diff/7001/common/cmd_vboot.c#newcode33
common/cmd_vboot.c:33: 
On 2011/03/24 22:17:44, Luigi Semenzato wrote:
> Why did you change the semantics here?  If we get other kinds of errors, we
may
> want to propagate them.

Done.
Changed back to original.

http://codereview.chromium.org/6683023/diff/7001/include/configs/chromeos/teg...
File include/configs/chromeos/tegra2/aebl/parts/tpm.h (right):

http://codereview.chromium.org/6683023/diff/7001/include/configs/chromeos/teg...
include/configs/chromeos/tegra2/aebl/parts/tpm.h:12: #define
CONFIG_INFINEON_TPM_I2C_BUS		(2)
On 2011/03/24 22:17:44, Luigi Semenzato wrote:
> I wouldn't use parentheses here (and below).
> They don't hurt, but they don't help either.

Done.
Unnecessary parentheses removed from tpm.h [...tegra2/$BOARD/parts/]

[rongchang, 2011-03-29 07:13:57]

Hi Luigi,
We still need to patch Infineon's driver (5 lines) to make it work. We will sort
it out ASAP.

On 2011/03/24 22:17:44, Luigi Semenzato wrote:
> LGTM
> 
> Rong, this is good (with a couple of nits below).  If Infineon has succeeded
in
> upstreaming their driver, can you make sure that this is consistent with their
> version?  If not, just go ahead and commit it, with the understanding that we
> will have to re-merge our changes when they upstream.  In any case it's good
to
> support both v03 and v05 because we still have a lot of the former around, and
> we will for a while.
> 
> Sorry for the delay.
> 
> http://codereview.chromium.org/6683023/diff/7001/common/cmd_vboot.c
> File common/cmd_vboot.c (right):
> 
> http://codereview.chromium.org/6683023/diff/7001/common/cmd_vboot.c#newcode33
> common/cmd_vboot.c:33: 
> Why did you change the semantics here?  If we get other kinds of errors, we
may
> want to propagate them.
> 
>
http://codereview.chromium.org/6683023/diff/7001/include/configs/chromeos/teg...
> File include/configs/chromeos/tegra2/aebl/parts/tpm.h (right):
> 
>
http://codereview.chromium.org/6683023/diff/7001/include/configs/chromeos/teg...
> include/configs/chromeos/tegra2/aebl/parts/tpm.h:12: #define
> CONFIG_INFINEON_TPM_I2C_BUS		(2)
> I wouldn't use parentheses here (and below).
> They don't hurt, but they don't help either.