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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot