[U-Boot] [PATCH 4/5] tools: mkimage: fix imximage header size

[Stefan Agner]

Hi Albert,

On 2015-07-15 09:54, Albert ARIBAUD wrote:
> Hello Stefano,
> 
> On Wed, 15 Jul 2015 09:19:55 +0200, Stefano Babic <sbabic at denx.de>
> 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