From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tapani <tapani@technexion.com>
Date: Wed, 13 Nov 2013 13:50:13 +0800
Subject: [U-Boot] [PATCH 2/4] mx6: add structs for mmdc and ddr iomux
	registers
In-Reply-To: <52824554.3060409@denx.de>
References: <1383902865.3157.485.camel@edward-x220-laptop>
	<1383903325.3157.493.camel@edward-x220-laptop>
	<52824554.3060409@denx.de>
Message-ID: <20131113135013.2fda2fb6@triceratops>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de


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 :-).

While on topic, the aligned(4) is needed because of the packed attribute.

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?). 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.

> > +
> > +/* 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? 

regards,

//Tapani