Hi Tapani,
On 13/11/2013 06:50, Tapani wrote:
Stefano,
I'll reply to this, since including the __attribute__ was my suggestion.
u32 res10[25];u32 mpmur0;+}__attribute__((packed, aligned(4)));
I am missing why the packed is needed.
This was discussed before, and I tried to explain it already then.
Short answer: to make a broken design slightly less likely to cause problems.
Long answer: Using structs the way required by u-boot maintainers is invalid C. It is a compiler quirk that it happens to work. This is clear from ANSI C89 standard: http://www.lysator.liu.se/c/rat/c5.html#3-5-2-1
However people (like Microsoft famously with Office) kept shooting themselves in the foot with structure members and padding, this was clarified even further in the C99 standard (see pt. 1424 in http://c0x.coding-guidelines.com/6.7.2.1.html )
Meanwhile, the gcc people added a compiler hint, __attribute__((packed)), to kindly ask the compiler not to add any padding structs. (So you guys certainly are not the first ones doing this kind of hacks :-).
The specific question should have been: why do we use packed when all fields in the structure are already u32 ? The compiler does not introduce padding, at least now : packed will be necessary if there are 16bit or 8 bit registers.
If gcc in the future will generate padding (let's say, all fields will become 64 bit), this become a general issue and must be globally fixed - I mean for all structures accessing to internal registers. But using this only here is not consistent.
While on topic, the aligned(4) is needed because of the packed attribute.
This is clear, but again, if we needed, the fix should go globally. Currently all structures in u-boot do not use it.
There is a drawback with the gcc __attribute__((packed)). The compiler might no longer be able to assume any alignment of the members (what if the struct resides on an odd address?).
In u-boot such as structures are not allocated, but there address is set in code, such as:
struct src *src_regs = (struct src *)SRC_BASE_ADDR;
of course, if SRC_ADDR is not aligned, it crashes - but this is a bug. I think (not tested) that if you force the compiler to align the address, and for example SRC_BASE_ADDR is not correct and set on an odd address, the compiler will force it to the nearest aligned address, that is presumably wrong and generates unpredictable results.
To remedy this some compilers create complicated code for accessing the struct members in a way that no CPU exceptions are raised, to make the structure accesses work (and some compilers just let the Bus Errors happen). The __attribute__((aligned(4))) tells the compiler that it may assume that the struct will always be aligned on a 4-byte boundary.
But in the way u-boot uses the structures, a crash or CPU exception must be raised because it signals a bug.
+/* MMDC P0/P1 Registers */ +struct mmdc_p_regs {
u32 mdctl;u32 mdpdc;u32 mdotc;u32 mdcfg0;u32 mdcfg1;u32 mdcfg2;u32 mdmisc;Ok - if these structure is to make it available for other components, it should replace the structure esd_mmdc_regs in cpu.c. We cannot have both.
So would you suggest we add a union inside the structure to differentiate imx5 and imx6? Or just make the structure imx6s/dl/q specific?
The main point is that I disagree on duplicating code. The same structure with your patch is defined twice, this cannot be.
The specific register layout is defined into the corresponding imx-regs.h (./arch/arm/include/asm/arch-mx5/imx-regs.h or ./arch/arm/include/asm/arch-mx6/imx-regs.h). If there is the same structure (let's say, the same controller with different layout) on different socs, you have to define the structures in the imx-regs.h for each SOC that uses that structure.
Best regards, Stefano Babic