On Monday 10 October 2011 16:00:18 Vadim Bendebury wrote:
On Mon, Oct 10, 2011 at 3:45 AM, Wolfgang Denk 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.
yes, we deal with this in many places. the pseudo C structs you propose above sound fine to me.
i'm hoping that pseudo locality struct doesn't reflect reality though as field_c is going to be unaligned by 1 byte :).
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.
i think unions are fine. i've done this before like: union { u8 reg8; u16 reg16; u32 reg32; };
and then the code just uses the relevant one based on its needs
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.
every conversion i've seen so far from base+reg offsets was much more readable (imo) when converted to C structs. i'm not seeing why this tpm code would be any different ? -mike