From: Stefan Agner <stefan@agner.ch>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 4/5] tools: mkimage: fix imximage header size
Date: Wed, 15 Jul 2015 14:36:16 +0200 [thread overview]
Message-ID: <23c8142da99dbf70b335a0792c363641@agner.ch> (raw)
In-Reply-To: <20150715134422.70514c09@lilith>
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 <stefan@agner.ch>
> 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 <sbabic@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.
>
> 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
next prev parent reply other threads:[~2015-07-15 12:36 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-19 12:18 [U-Boot] [PATCH 0/5] Add support for Vybrid VF610-based PCM052 Albert ARIBAUD
2015-06-19 12:18 ` [U-Boot] [PATCH 1/5] net: fec_mxc: remove useless struct nbuf Albert ARIBAUD
2015-06-19 12:18 ` [U-Boot] [PATCH 2/5] vf610: refactor DDRMC code Albert ARIBAUD
2015-06-19 12:18 ` [U-Boot] [PATCH 3/5] i2c: fix vf610 support Albert ARIBAUD
2015-06-19 12:18 ` [U-Boot] [PATCH 4/5] tools: mkimage: fix imximage header size Albert ARIBAUD
2015-06-19 12:18 ` [U-Boot] [PATCH 5/5] vf610: add support for Phytec PCM052 Albert ARIBAUD
2015-07-10 8:14 ` [U-Boot] [PATCH 4/5] tools: mkimage: fix imximage header size Stefano Babic
2015-07-14 10:29 ` Stefan Agner
2015-07-15 7:19 ` Stefano Babic
2015-07-15 7:54 ` Albert ARIBAUD
2015-07-15 10:41 ` Stefan Agner
2015-07-15 11:44 ` Albert ARIBAUD
2015-07-15 12:36 ` Stefan Agner [this message]
2015-07-15 12:54 ` Albert ARIBAUD
2015-07-15 7:37 ` Albert ARIBAUD
2015-07-10 8:11 ` [U-Boot] [PATCH 3/5] i2c: fix vf610 support Stefano Babic
2015-06-19 15:13 ` [U-Boot] [PATCH 2/5] vf610: refactor DDRMC code Stefan Agner
2015-06-19 16:50 ` Albert ARIBAUD
2015-06-19 17:33 ` Albert ARIBAUD
2015-07-10 8:09 ` Stefano Babic
2015-07-13 19:01 ` Stefan Agner
2015-07-14 7:16 ` [U-Boot] (rather [LONG], sorry) " Albert ARIBAUD
2015-06-19 15:38 ` [U-Boot] [PATCH 1/5] net: fec_mxc: remove useless struct nbuf Joe Hershberger
2015-07-10 8:03 ` Stefano Babic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=23c8142da99dbf70b335a0792c363641@agner.ch \
--to=stefan@agner.ch \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.