On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote: > Dear Marek Vasut, > > thank you for your comments, please see below: > > On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut <marek.va...@gmail.com> wrote: > > On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury 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. > > > > [...] > > > > Quick points: > > * The license > > Please suggest the appropriate file header text.
Uh ... you should know the license !!! > > > * Use debug() when fitting > > It seems to me debug/puts/printf are used where appropriate - do you > have some cases in mind which need to be changed? Ok > > >> diff --git a/drivers/tpm/generic_lpc_tpm.c > >> b/drivers/tpm/generic_lpc_tpm.c new file mode 100644 > >> index 0000000..8319286 > >> --- /dev/null > >> +++ b/drivers/tpm/generic_lpc_tpm.c > > > > [...] > > > >> + > >> +struct lpc_tpm { > >> + struct tpm_locality locality[TPM_TOTAL_LOCALITIES]; > >> +}; > > > > Do you need such envelope ? > > I think I do - this accurately describes the structure of the chip. There's just one member ... it's weird? > > >> + > >> +static struct lpc_tpm *lpc_tpm_dev = > >> + (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS; > >> + > >> +/* 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 */ > >> + > >> +#define TIS_ACCESS_TPM_REG_VALID_STS (1 << 7) /* 0x80 */ > >> +#define TIS_ACCESS_ACTIVE_LOCALITY (1 << 5) /* 0x20 */ > >> +#define TIS_ACCESS_BEEN_SEIZED (1 << 4) /* 0x10 */ > >> +#define TIS_ACCESS_SEIZE (1 << 3) /* 0x08 */ > >> +#define TIS_ACCESS_PENDING_REQUEST (1 << 2) /* 0x04 */ > >> +#define TIS_ACCESS_REQUEST_USE (1 << 1) /* 0x02 */ > >> +#define TIS_ACCESS_TPM_ESTABLISHMENT (1 << 0) /* 0x01 */ > >> + > >> +#define TIS_STS_BURST_COUNT_MASK (0xffff) > >> +#define TIS_STS_BURST_COUNT_SHIFT (8) > >> + > >> +/* > >> + * Error value returned if a tpm register does not enter the expected > >> state + * after continuous polling. No actual TPM register reading ever > >> returns ~0, + * so this value is a safe error indication to be mixed > >> with possible status + * register values. > >> + */ > >> +#define TPM_TIMEOUT_ERR (~0) > >> + > >> +/* Error value returned on various TPM driver errors */ > > > > Dot missing at the end. > > ok. Please fix globally. > > >> +#define TPM_DRIVER_ERR (-1) > >> + > >> + /* 1 second is plenty for anything TPM does.*/ > >> +#define MAX_DELAY_US (1000 * 1000) > >> + > >> +/* Retrieve burst count value out of the status register contents. */ > >> +#define BURST_COUNT(status) ((u16)(((status) >> > >> TIS_STS_BURST_COUNT_SHIFT) & \ + > >> TIS_STS_BURST_COUNT_MASK)) > > > > Do you need the cast ? > > I think it demonstrates the intentional truncation of the value, it > gets assigned to u16 values down the road, prevents compiler warnings > about assigning incompatible values in some cases. Make it an inline function then, this will do the typechecking for you. > > >> + > >> +/* > >> + * Structures defined below allow creating descriptions of TPM > >> vendor/device + * ID information for run time discovery. The only device > >> the system knows + * about at this time is Infineon slb9635 > >> + */ > >> +struct device_name { > >> + u16 dev_id; > >> + const char * const dev_name; > >> +}; > >> + > >> +struct vendor_name { > >> + u16 vendor_id; > >> + const char *vendor_name; > >> + const struct device_name *dev_names; > >> +}; > >> + > >> +static const struct device_name infineon_devices[] = { > >> + {0xb, "SLB9635 TT 1.2"}, > >> + {0} > >> +}; > >> + > >> +static const struct vendor_name vendor_names[] = { > >> + {0x15d1, "Infineon", infineon_devices}, > >> +}; > >> + > >> +/* > >> + * Cached vendor/device ID pair to indicate that the device has been > >> already + * discovered > >> + */ > >> +static u32 vendor_dev_id; > >> + > >> +/* TPM access going through macros to make tracing easier. */ > >> +#define tpm_read(ptr) ({ \ > >> + u32 __ret; \ > >> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \ > >> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \ > >> + (u32)ptr - (u32)lpc_tpm_dev, __ret); \ > >> + __ret; }) > >> + > > > > Make this inline function > > > >> +#define tpm_write(value, ptr) ({ \ > >> + u32 __v = value; \ > >> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \ > >> + (u32)ptr - (u32)lpc_tpm_dev, __v); \ > >> + if (sizeof(*ptr) == 1) \ > >> + writeb(__v, ptr); \ > >> + else \ > >> + writel(__v, ptr); }) > >> + > > > > DTTO > > Are you sure these will work as inline functions? Why not ? Also, why do you introduce the __v ? > > > [...] > > > >> +static u32 tis_senddata(const u8 * const data, u32 len) > >> +{ > >> + u32 offset = 0; > >> + u16 burst = 0; > >> + u32 max_cycles = 0; > >> + u8 locality = 0; > >> + u32 value; > >> + > >> + value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status, > >> + TIS_STS_COMMAND_READY, > >> TIS_STS_COMMAND_READY); + if (value == TPM_TIMEOUT_ERR) { > >> + printf("%s:%d - failed to get 'command_ready' status\n", > >> + __FILE__, __LINE__); > >> + return TPM_DRIVER_ERR; > >> + } > >> + burst = BURST_COUNT(value); > >> + > >> + while (1) { > > > > No endless loops please. > > You are the third person looking at this, but the first one > uncomfortable with this. Obviously this is not an endless loop, there > are three points where the loop terminates, two on timeout errors and > one on success. So there's no case where this can loop endlessly ? fine. > > >> + unsigned count; > >> + > >> + /* Wait till the device is ready to accept more data. */ > >> + while (!burst) { > >> + if (max_cycles++ == MAX_DELAY_US) { > >> + printf("%s:%d failed to feed %d bytes of > >> %d\n", + __FILE__, __LINE__, len - > >> offset, len); + return TPM_DRIVER_ERR; > >> + } > >> + udelay(1); > >> + burst = > >> BURST_COUNT(tpm_read(&lpc_tpm_dev->locality + > >> [locality].tpm_status)); + } > >> + > >> + max_cycles = 0; > >> + > >> + /* > >> + * Calculate number of bytes the TPM is ready to accept in > >> one + * shot. > >> + * > >> + * We want to send the last byte outside of the loop > >> (hence + * the -1 below) to make sure that the 'expected' > >> status bit + * changes to zero exactly after the last byte > >> is fed into the + * FIFO. > >> + */ > >> + count = min(burst, len - offset - 1); > >> + while (count--) > >> + tpm_write(data[offset++], > >> + &lpc_tpm_dev->locality[locality].data); > >> + > >> + value = tis_wait_reg(&lpc_tpm_dev->locality > >> + [locality].tpm_status, > >> + TIS_STS_VALID, TIS_STS_VALID); > >> + > >> + if ((value == TPM_TIMEOUT_ERR) || !(value & > >> TIS_STS_EXPECT)) { + printf("%s:%d TPM command feed > >> overflow\n", + __FILE__, __LINE__); > >> + return TPM_DRIVER_ERR; > >> + } > >> + > >> + burst = BURST_COUNT(value); > >> + if ((offset == (len - 1)) && burst) > >> + /* > >> + * We need to be able to send the last byte to the > >> + * device, so burst size must be nonzero before we > >> + * break out. > >> + */ > >> + break; > >> + } > >> + > >> + /* Send the last byte. */ > >> + tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data); > >> + /* > >> + * Verify that TPM does not expect any more data as part of this > >> + * command. > >> + */ > >> + value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status, > >> + TIS_STS_VALID, TIS_STS_VALID); > >> + if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) { > >> + printf("%s:%d unexpected TPM status 0x%x\n", > >> + __FILE__, __LINE__, value); > >> + return TPM_DRIVER_ERR; > >> + } > >> + > >> + /* OK, sitting pretty, let's start the command execution. */ > >> + tpm_write(TIS_STS_TPM_GO, > >> &lpc_tpm_dev->locality[locality].tpm_status); + return 0; > >> +} > >> + > >> +/* > >> + * tis_readresponse() > >> + * > >> + * read the TPM device response after a command was issued. > >> + * > >> + * @buffer - address where to read the response, byte by byte. > >> + * @len - pointer to the size of buffer > >> + * > >> + * On success stores the number of received bytes to len and returns 0. > >> On + * errors (misformatted TPM data or synchronization problems) > >> returns + * TPM_DRIVER_ERR. > >> + */ > >> +static u32 tis_readresponse(u8 *buffer, u32 *len) > >> +{ > >> + u16 burst_count; > >> + u32 status; > >> + u32 offset = 0; > >> + u8 locality = 0; > >> + const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID; > >> + u32 expected_count = *len; > >> + int max_cycles = 0; > >> + > >> + /* Wait for the TPM to process the command */ > >> + status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status, > >> + has_data, has_data); > >> + if (status == TPM_TIMEOUT_ERR) { > > > > Just make it return non-zero for timeout and check for non-zero. And > > unify the variable names please. > > What variable names are you referring to, can you please elaborate. "value" in the previous function, "status" in this one. Why the inconsistence ? > > >> + printf("%s:%d failed processing command\n", > >> + __FILE__, __LINE__); > >> + return TPM_DRIVER_ERR; > >> + } > >> + > >> + do { > >> + while ((burst_count = BURST_COUNT(status)) == 0) { > >> + if (max_cycles++ == MAX_DELAY_US) { > >> + printf("%s:%d TPM stuck on read\n", > >> + __FILE__, __LINE__); > >> + return TPM_DRIVER_ERR; > >> + } > >> + udelay(1); > >> + status = tpm_read(&lpc_tpm_dev->locality > >> + [locality].tpm_status); > >> + } > >> + > >> + max_cycles = 0; > >> + > >> + while (burst_count-- && (offset < expected_count)) { > >> + buffer[offset++] = (u8) > >> tpm_read(&lpc_tpm_dev->locality + > >> [locality].data); + > >> + if (offset == 6) { > >> + /* > >> + * We got the first six bytes of the > >> reply, + * let's figure out how many bytes > >> to expect + * total - it is stored as a 4 > >> byte number in + * network order, starting > >> with offset 2 into + * the body of the > >> reply. > >> + */ > >> + u32 real_length; > >> + memcpy(&real_length, > >> + buffer + 2, > >> + sizeof(real_length)); > >> + expected_count = be32_to_cpu(real_length); > >> + > >> + if ((expected_count < offset) || > >> + (expected_count > *len)) { > >> + printf("%s:%d bad response size > >> %d\n", + __FILE__, __LINE__, > >> + expected_count); > >> + return TPM_DRIVER_ERR; > >> + } > >> + } > >> + } > >> + > >> + /* Wait for the next portion */ > >> + status = tis_wait_reg(&lpc_tpm_dev->locality > >> + [locality].tpm_status, > >> + TIS_STS_VALID, TIS_STS_VALID); > >> + if (status == TPM_TIMEOUT_ERR) { > >> + printf("%s:%d failed to read response\n", > >> + __FILE__, __LINE__); > >> + return TPM_DRIVER_ERR; > >> + } > >> + > >> + if (offset == expected_count) > >> + break; /* We got all we need */ > >> + > >> + } while ((status & has_data) == has_data); > > > > No endless loop please. > > I am not sure why you call this endless - it terminates on a few > events. The thing is that it is not even known in advance how many > cycles the loop will have to run, but it has necessary stop gaps to > prevent it from running forever. See above, is it possible for this to hang ? If not, ok. > > >> + > >> + /* > >> + * Make sure we indeed read all there was. The TIS_STS_VALID bit > >> is + * known to be set. > >> + */ > >> + if (status & TIS_STS_DATA_AVAILABLE) { > >> + printf("%s:%d wrong receive status %x\n", > >> + __FILE__, __LINE__, status); > >> + return TPM_DRIVER_ERR; > >> + } > >> + > >> + /* Tell the TPM that we are done. */ > >> + tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality > >> + [locality].tpm_status); > >> + *len = offset; > >> + return 0; > >> +} > >> + > >> +int tis_init(void) > >> +{ > >> + return tis_probe(); > >> +} > > > > Make tis_probe into tis_init and be done with it ? > > ok > > >> + > >> +int tis_open(void) > >> +{ > >> + u8 locality = 0; /* we use locality zero for everything */ > >> + > >> + if (tis_close()) > >> + return TPM_DRIVER_ERR; > >> + > >> + /* now request access to locality */ > >> + tpm_write(TIS_ACCESS_REQUEST_USE, > >> + &lpc_tpm_dev->locality[locality].access); > >> + > >> + > >> + /* did we get a lock? */ > >> + if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access, > >> + TIS_ACCESS_ACTIVE_LOCALITY, > >> + TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) { > >> + printf("%s:%d - failed to lock locality %d\n", > >> + __FILE__, __LINE__, locality); > >> + return TPM_DRIVER_ERR; > >> + } > >> + > >> + tpm_write(TIS_STS_COMMAND_READY, > >> + &lpc_tpm_dev->locality[locality].tpm_status); > >> + return 0; > >> +} > > > > [...] > > > > Cheers > > cheers, > /vb Cheers _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot