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.
> * 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?
>> 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.
>> +
>> +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.
>> +#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.
>> +
>> +/*
>> + * 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?
> [...]
>
>> +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.
>> + 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.
>> + 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.
>> +
>> + /*
>> + * 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
