[U-Boot] [PATCH] Introduce generic TPM support in u-boot

[Wolfgang Denk]

Dear Vadim Bendebury,

In message <20111010025327.119EB40B40 at eskimo.mtv.corp.google.com> you wrote:
> TPM (Trusted Platform Module) is an integrated circuit and
> software platform that provides computer manufacturers with the
> core components of a subsystem used to assure authenticity,
> integrity and confidentiality.
> 
> This driver supports version 1.2 of the TCG (Trusted Computing
> Group) specifications.
> 
> The TCG specification defines several so called localities in a
> TPM chip, to be controlled by different software layers. When
> used on a typical x86 platform during the firmware phase, only
> locality 0 can be accessed by the CPU, so this driver even while
> supporting the locality concept presumes that only locality zero
> is used.
> 
> This implementation is loosely based on the article "Writing a
> TPM Device Driver" published on http://ptgmedia.pearsoncmg.com
> and a submission by Stefan Berger on Qemu-devel mailing list
> (http://lists.gnu.org/archive/html/qemu-devel).
> 
> Compiling this driver with DEBUG defined will generate trace of
> all accesses to TMP registers.
> 
> This driver has been tested and is being used in three different
> functional ChromeOS machines (Pinetrail and Sandy Bridge Intel
> chipsets) all using the same Infineon SLB 9635 TT 1.2 device.
> 
> A u-boot cli command allowing access to the TPM was also
> implemented and will be submitted separately.
> 
> Change-Id: I22a33c3e5b2e20eec9557a7621bd463b30389d73
> Signed-off-by: Vadim Bendebury <vbendeb at chromium.org>
> CC: Wolfgang Denk <wd at denx.de>
...

As is, there are no users of this code, so it would be just dead code.
Please resubmit in a patch series that also includes code to use this
feature.

> +++ b/drivers/tpm/Makefile
> @@ -0,0 +1,27 @@
> +# Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
> +# Use of this source code is governed by a BSD-style license that can be
> +# found in the LICENSE file.

There is no LICENSE file.  Please provide exact license information;
for details please see bullet # 1 at
http://www.denx.de/wiki/view/U-Boot/Patches#Notes

Please fix globally.

> +/* #define DEBUG */

Please remove dead code like this.

> +#define PREFIX "lpc_tpm: "
> +#define	TPM_DEBUG(fmt, args...)		\
> +	if (TPM_DEBUG_ON) {		\
> +		printf(PREFIX);		\
> +		printf(fmt , ##args);	\
> +	}

Can you not use standard debug() code?

> +#ifndef CONFIG_TPM_TIS_BASE_ADDRESS
> +/* Base TPM address standard for x86 systems */
> +#define CONFIG_TPM_TIS_BASE_ADDRESS        0xfed40000
> +#endif

I think this should be removed.

> +#define TIS_REG(LOCALITY, REG) \
> +	(void *)(CONFIG_TPM_TIS_BASE_ADDRESS + (LOCALITY << 12) + REG)

We do not allow to access device registers through base address +
offset. Please always use C structs instead.

...
> +/* TPM access functions are carved out to make tracing easier. */
> +static u32 tpm_read(int locality, u32 reg)
> +{
> +	u32 value;
> +	/*
> +	 * Data FIFO register must be read and written in byte access mode,
> +	 * otherwise the FIFO values are returned 4 bytes at a time.
> +	 */

Please insert blank line between declarations and code. Please fix
globally.

...
> +	/* this will have to be converted into debug printout */
> +	TPM_DEBUG("Found TPM %s by %s\n", device_name, vendor_name);

Is this comment still correct?

> +int tis_init(void)
> +{
> +	if (tis_probe())
> +		return TPM_DRIVER_ERR;
> +	return 0;
> +}

Or simply:

	return tis_probe();


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Hegel was right when he said that we learn from history that man  can
never learn anything from history.              - George Bernard Shaw