[U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot

[Marek Vasut]

On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
> On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut <marek.vasut at 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.vasut at 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

Are you sure you're not relicensing code you don't own ? I'm just curious, 
what's the origin of the code ? I'd prefer to avoid legal crap.

> [..]
> 
> >> >> +
> >> >> +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.

can't you pass the locality array ?

> 
> [..]
> 
> >> > 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?

It doesn't do typechecking.
> 
> >> >> +
> >> >> +/*
> >> >> + * 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.

This seems wrong like hell.
> 
> __v is needed to avoid side effects when invoking the macro.

Side effects ? What side effects ?

[...]

Cheers