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

[Albert ARIBAUD]

Hello Stefan,

On Wed, 15 Jul 2015 14:36:16 +0200, Stefan Agner <stefan at agner.ch>
wrote:

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

Does it? Then I too was under the same misconception that it did not.

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

Ok, so let me look into the __attribute__((aligned(8))) issue. If that
can be solved, I would give it my preference; if it cannot, then I will
settle for manually padding the end of imx_header_v2_t.

> --
> Stefan

Amicalement,
-- 
Albert.