From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Agner Date: Wed, 15 Jul 2015 12:41:59 +0200 Subject: [U-Boot] [PATCH 4/5] tools: mkimage: fix imximage header size In-Reply-To: <20150715095442.3e2504ad@lilith> References: <1434716311-26189-1-git-send-email-albert.aribaud@3adev.fr> <1434716311-26189-2-git-send-email-albert.aribaud@3adev.fr> <1434716311-26189-3-git-send-email-albert.aribaud@3adev.fr> <1434716311-26189-4-git-send-email-albert.aribaud@3adev.fr> <1434716311-26189-5-git-send-email-albert.aribaud@3adev.fr> <559F7EC8.6020605@denx.de> <874a6782a666faec3520e1e9a58f7092@agner.ch> <55A6099B.6050908@denx.de> <20150715095442.3e2504ad@lilith> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Albert, On 2015-07-15 09:54, Albert ARIBAUD wrote: > Hello Stefano, > > On Wed, 15 Jul 2015 09:19:55 +0200, Stefano Babic > wrote: > >> > I think my patch back then solves the issue nicer. The struct dcd_v2_t >> > is not the reason the whole header is not aligned by 8-byte, it is a >> > problem of the boot_data_t header which is 12 bytes long. So inserting >> > the padding just after struct boot_data_t (or inside of struct >> > boot_data_t) seems to be more appropriate. >> >> I see. Albert, can you test on your board if Stefan's patch solves your >> issue, too ? I could then revert this one and apply Stefan's. > > I can test it, but I'm sure that boot data does not need size alignment > since in my case there is no such alignment and Vybrid boots. To me > this means aligning boot_data_t size would introduce a constraint that > is not really there. I quote here from both emails, since its about the same topic. > After reading the U-Boot and Freescale discussions, IIUC you have come > to the conclusion that the missing 4 bytes were in boot_data_t because > it was the only structure in the header which was not 8-bytes-aligned. > > However, the available documentation does not specify this constraint, > and from a more experimental vewpoint, my patch adds 4 bytes at the end > of the overall header, thus leaving boot data at 12 bytes (therefore > leaving the dcd_table not 8-byte-aligned) and yet Vybrid can boot. Yes, I agree on that. Also the boot data seems not to have the 8-byte alignment requirement (see the graphics I posted on the Freescale community article). > To me, this proves that the size alignment problem is not with > boot_data_t but with the overall header. I agree. Still, boot_data_t is the "offender", hence I would prefer that solution over your solution. > If Stefan would test my patch as well, I reckon he would find it to > work as well as his. > > So we have two patches which fix Vybrid booting: > > - one which aligns the boot_data_t /size/ but keeps its /offset/ > unaligned; > > - one which aligns the the boot_data_t /offset/ but keeps its /size/ > unaligned. I don't get this classification. The complete header struct shows that dcd_v2_t is _after_ boot_data_t. typedef struct { flash_header_v2_t fhdr; boot_data_t boot_data; dcd_v2_t dcd_table; } imx_header_v2_t; Your patch changes the size of dcd_v2_t, hence compensating the "unaligned" size of boot_data_t. As far as I see we have these two patches which fix Vybrid booting: - one which changes the boot_data_t /size/ which keeps the offset of dcd_v2_t and the image aligned. - one which changes the dcd_v2_t /size/ which compensates the unaligned size of boot_data_t and keeps the image aligned. > Seems to me that the conclusion is that the actual alignment of > boot_data_t does not matter and that only the alignment of the > whole imx_header_v2_t size (and, consequently, offset) matters. > > How about just adding an attribute((align(8))) to imx_header_v2_t? Hm this sounds tempting, but it does not seem to work here. I think because it only aligns the beginning of imx_header_v2_t in to 8-byte, however it does not align the size of the whole struct to 8 bytes, I guess? Hence the header size is still "unaligned". > >> It remains doubious how the ROMs on Freescale are interpretating the >> header, but we can only test it. > > See my other answer. We could prove it by disassembling the ROM code. > Any volunteer? :) Hehe, not me :-) -- Stefan