On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut <marek.va...@gmail.com> wrote:
> 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 !!!
removed the BSD part
>>
[..]
>> >> +
>> >> +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?
>
I think it is appropriate in this case to encapsulate the entire chip
description in a structure. Among other things makes it easier to pass
a pointer to the chip description around.
[..]
>> >
>> > Dot missing at the end.
>>
>> ok.
>
> Please fix globally.
>
done
>>
>> >> +#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.
>
I am not sure what is wrong with a short macro in this case - is this
against the coding style?
>>
>> >> +
>> >> +/*
>> >> + * 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 ?
macro vs function: need to be able to tell the pointed object size at run time.
__v is needed to avoid side effects when invoking the macro.
>>
>> > [...]
>> >
>> >> +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
> ?
>
done
>>
>> >> + 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
>
cheers,
/vb
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
