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

[Mike Frysinger]

On Sunday 09 October 2011 22:53:26 Vadim Bendebury wrote:
> Change-Id: I22a33c3e5b2e20eec9557a7621bd463b30389d73

these don't make sense outside of gerrit ;)

> --- a/Makefile
> +++ b/Makefile
>
> +ifneq ($(CONFIG_GENERIC_LPC_TPM),)

please use:
	ifeq ($(CONFIG_...),y)

> --- a/README
> +++ b/README
> 
> +- TPM Support:
> +		CONFIG_GENERIC_LPC_TPM
> +		Support for generic parallel port TPM devices.
> +
> +			CONFIG_TPM_TIS_BASE_ADDRESS
> +			Base address where the generic TPM device is mapped
> +			to. Default value is 0xfed40000.

i'm not sure a default address makes sense at all for all of u-boot.  best to 
just force people to define it, or keep the default in cpu/soc-specific configs.

> --- /dev/null
> +++ b/drivers/tpm/Makefile
>
> +# 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.

this is incorrect for the u-boot tree.  the "LICENSE" file doesn't exist, and 
the "COPYING" file is GPLv2.  please use something like:
	# Copyright (c) 2011 The Chromium OS Authors.
	# Released under the 2-clause BSD license.

> +$(LIB): $(obj).depend $(OBJS)
> +	$(AR) $(ARFLAGS) $@ $(OBJS)

this is an old makefile.  you have to use cmd_link_o_target now and not AR.

> index 0000000..939f715
> --- /dev/null
> +++ b/drivers/tpm/generic_lpc_tpm.c
>
> +/* #define DEBUG */

just delete it

> +#ifdef DEBUG
> +#define TPM_DEBUG_ON	1
> +#else
> +#define TPM_DEBUG_ON	0
> +#endif
> +
> +#define PREFIX "lpc_tpm: "
> +#define	TPM_DEBUG(fmt, args...)		\
> +	if (TPM_DEBUG_ON) {		\
> +		printf(PREFIX);		\
> +		printf(fmt , ##args);	\
> +	}

simplify this with one line:
#define tpm_debug(fmt, args...) debug("lpc_tpm: " fmt, ## args)

> +/* the macro accepts the locality value, but only locality 0 is
> operational */
> +#define TIS_REG(LOCALITY, REG) \
> +	(void *)(CONFIG_TPM_TIS_BASE_ADDRESS + (LOCALITY << 12) + REG)
> +
> +/* hardware registers' offsets */
> +#define TIS_REG_ACCESS                 0x0
> +#define TIS_REG_INT_ENABLE             0x8
> +#define TIS_REG_INT_VECTOR             0xc
> +#define TIS_REG_INT_STATUS             0x10
> +#define TIS_REG_INTF_CAPABILITY        0x14
> +#define TIS_REG_STS                    0x18
> +#define TIS_REG_DATA_FIFO              0x24
> +#define TIS_REG_DID_VID                0xf00
> +#define TIS_REG_RID                    0xf04

you need to use C structs here and not offsets.  same feedback as i gave you 
when you posted it to gerrit :P.

> +/* Some registers' bit field definitions */
> +#define TIS_STS_VALID                  (1 << 7) /* 0x80 */
> +#define TIS_STS_COMMAND_READY          (1 << 6) /* 0x40 */
> +#define TIS_STS_TPM_GO                 (1 << 5) /* 0x20 */
> +#define TIS_STS_DATA_AVAILABLE         (1 << 4) /* 0x10 */
> +#define TIS_STS_EXPECT                 (1 << 3) /* 0x08 */
> +#define TIS_STS_RESPONSE_RETRY         (1 << 1) /* 0x02 */

having both values is a bit silly.  pick one.  if you think 0x80 is more clear 
than 1<<7, then define that.

> +struct device_name {
> +	u16 dev_id;
> +	const char * const dev_name;
> +};
> +
> +struct vendor_name {
> +	u16 vendor_id;
> +	const char *vendor_name;
> +	struct device_name *dev_names;
> +};
> +
> +static struct device_name infineon_devices[] = {
> +	{0xb, "SLB9635 TT 1.2"},
> +	{0}
> +};
> +
> +static const struct vendor_name vendor_names[] = {
> +	{0x15d1, "Infineon", infineon_devices},
> +};

the infineon_devices (and thus dev_names) should be constified

> +
> +/*
> + * Cached vendor/device ID pair to indicate that the device has been
> + * already discovered
> + */
> +static u32 vendor_dev_id;

so you're assuming only one TPM device per system.  probably not a realistic 
problem, but this should be documented in the README.

> +static u32 tis_senddata(const u8 * const data, u32 len)
> ...
> +		printf("%s:%d - failed to get 'command_ready' status\n",
> +		       __FILE__, __LINE__);

__FILE__/__LINE__ don't really belong in common output.  please replace with 
__func__.  same feedback as in gerrit :P.

> +/*
> + * tis_init()
> + *
> + * Initialize the TPM device. Returns 0 on success or TPM_DRIVER_ERR on
> + * failure (in case device probing did not succeed).
> + */
> +int tis_init(void)
> +{
> +	if (tis_probe())
> +		return TPM_DRIVER_ERR;
> +	return 0;
> +}

but TPM_DRIVER_ERR is only defined in this file.  please just use the more u-
boot normal "0 on success, non-zero on failure".

> +/*
> + * tis_sendrecv()
> + *
> + * Send the requested data to the TPM and then try to get its response
> + *
> + * @sendbuf - buffer of the data to send
> + * @send_size size of the data to send
> + * @recvbuf - memory to save the response to
> + * @recv_len - pointer to the size of the response buffer
> + *
> + * Returns 0 on success (and places the number of response bytes at
> recv_len) + * or TPM_DRIVER_ERR on failure.
> + */

i'd think this API documentation really belongs in the API header ... that's 
generally what people who use the API are going to read first ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111010/1cfad4ac/attachment.pgp