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