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@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@denx.de Hegel was right when he said that we learn from history that man can never learn anything from history. - George Bernard Shaw