[U-Boot] [PATCH] Introduce generic TPM support in u-boot

[Vadim Bendebury]

Wolfgang, thank you for your comments, I'll address them in a
follow-up submission, but I have a question regarding the register
access (and the issue was indeed brought up by vapier@ at an earlier
review on a different submission).

On Mon, Oct 10, 2011 at 3:45 AM, Wolfgang Denk <wd at denx.de> wrote:
>
> > +#define TIS_REG(LOCALITY, REG) \
> > +     (void *)(CONFIG_TPM_TIS_BASE_ADDRESS + (LOCALITY << 12) + REG)
>
> We do not allow to access device registers through base address +
> offset. Please always use C structs instead.
>

so, this chip has five different areas (localities) which have the
same structure, and are mapped at certain regular offsets inside the
chip.

thus the structure describing the chip would be something like

struct locality {
  u16 field_a;
  u8 field_b;
  u32 field_c;
  ..
  u8 padding[<padding size>];
} __packed;

struct tmp_chip {
      struct locality localities[5];
} __packed;


which is very compiler dependent, but probably not the only place in
u-boot, so I could live with that if you could.

Yet another inconvenience though is the requirement to be able to
trace accesses to the registers. Some of the registers can be accessed
in 32 bit mode or 8 bit mode, and this determines how many bytes get
sent into/read from a data fifo. The tracing function should be able
to tell what mode the register was accessed in. A way I see to do it
through a structure layout is to define the same fields through
unions.

What I am getting at is that the code is much better readable as it is
now even though it is in violation of the 'use structure to access
registers' requirement.

Or maybe I am missing an obvious way to do it? Can you please elaborate,

cheers,
/vb


> ...
> > +/* TPM access functions are carved out to make tracing easier. */
> > +static u32 tpm_read(int locality, u32 reg)
> > +{
> > +     u32 value;
> > +     /*
> > +      * Data FIFO register must be read and written in byte access mode,
> > +      * otherwise the FIFO values are returned 4 bytes at a time.
> > +      */
>
> Please insert blank line between declarations and code. Please fix
> globally.
>
> ...
> > +     /* this will have to be converted into debug printout */
> > +     TPM_DEBUG("Found TPM %s by %s\n", device_name, vendor_name);
>
> Is this comment still correct?
>
> > +int tis_init(void)
> > +{
> > +     if (tis_probe())
> > +             return TPM_DRIVER_ERR;
> > +     return 0;
> > +}
>
> Or simply:
>
>        return tis_probe();
>
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Hegel was right when he said that we learn from history that man  can
> never learn anything from history.              - George Bernard Shaw