From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Agner Date: Wed, 15 Jul 2015 14:36:16 +0200 Subject: [U-Boot] [PATCH 4/5] tools: mkimage: fix imximage header size In-Reply-To: <20150715134422.70514c09@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> <20150715134422.70514c09@lilith> Message-ID: <23c8142da99dbf70b335a0792c363641@agner.ch> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 2015-07-15 13:44, Albert ARIBAUD wrote: > Hello Stefan (and sorry for the duplicate), > > On Wed, 15 Jul 2015 12:41:59 +0200, Stefan Agner > wrote: >> 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. > > Compensating the unaligned size of the overall header for sure, since > I'm adding a last field in the last structure. But precisely since I'm > adding 4 bytes at the very end of the construct, we cannot tell whether > I fixed the 'size' of the flash header, boot data or DCD. > > What we can tell, though, is that Vybrid will boot as long as we > increase the size of the overall header to an 8-byte boundary, and also > that it will boot regardless to whether these 4 bytes are added between > boot data and DCD or after DCD. > > (note, btw, that in the Vybrid RM, the IVT itself, and the DCD, contain > their own size explicitly in their tag-length-version headers, but the > boot data does not contain its own size, nor is it contained elsewhere. > What we call 'the size of the boot data' is 'the difference between the > boot data and dcd addresses', but that's a definition for convenience.) Good point, agreed. >> 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". > > Correct -- my fault, this aligns the address, not size, and there is > no attribute that will control size alignment. Actually I just discovered that "aligned" also changes the size (make sure using the correct syntax "__attribute__((aligned(8)))"). However, imximage.c seems to do some calculations using sizeof(struct imx_header) and sizeof(imx_header_v2_t) which lead to this error message: ./tools/mkimage: header error Adding __attribute__((aligned(8))) to struct imx_header seems not to alleviate the problem... Any idea? > > So we really need to add manual padding. > > And if we want to not introduce any artificial constaints, I suggest we > define the padding as a separate field in the overall structure, i.e.: > > typedef struct { > flash_header_v2_t fhdr; > boot_data_t boot_data; > dcd_v2_t dcd_table; > uint32_t padding; /* make size an 8-byte multiple */ > } imx_header_v2_t; That change would also sound good to me. > > I think this is the 'least inexact' solution: it does not enforce any > address or size alignment constraint that is not defined in the RM, and > it does show that the constraint is not on the boot data or DCD but on > the header as a whole. > > Functionally, it is identical to what I did, so I'm pretty sure it > works. :) Agreed. -- Stefan