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_TPMSupport for generic parallel port TPM devices.CONFIG_TPM_TIS_BASE_ADDRESSBase address where the generic TPM device is mappedto. 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