[U-Boot] [PATCH 2/4] mx6: add structs for mmdc and ddr iomux registers

[Stefano Babic]

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

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================