[U-Boot] [PATCH 5/5] cm-t35: add support for loading splash image from NAND

[Albert ARIBAUD]

Hi Nikita,

On Sun, 30 Dec 2012 16:39:06 +0200, Nikita Kiryanov
<nikita at compulab.co.il> wrote:

> On 12/26/2012 04:27 PM, Jeroen Hofstee wrote:
> > Hello Nikita,
> >
> > On 12/25/2012 09:56 AM, Nikita Kiryanov wrote:
> >>
> >>> fyi, I noticed that my board compiled with gcc 4.7.3 from ELDK 5.3 will
> >>> trap if the bitmap is not aligned. Aligned is a bit tricky though
> >>> since the
> >>> bitmap has the signature, e.g. "BM" prepended and is thereafter 32 bit
> >>> aligned (or at least combined fields of 32 bits). In my case
> >>> displaying the
> >>> bitmap now only works when loaded to an aligned address - 2. Since
> >>> you accept the value from the user, which has no ability to restore
> >>> it once
> >>> set "incorrectly", you might want to check the load address (well
> >>> obviously
> >>> only if it is a problem in your case as well).
> >>
> >> Thanks for verifying this issue. I did encounter it during testing, but
> >> I assumed it to be a compiler problem because it worked when compiling
> >> with a different version.
> >>
> >> Loading to aligned address - 2 works for me as well when compiling with
> >> the problematic compiler, but this is strange to me. Isn't the "packed"
> >> attribute that is applied to bmp_header_t supposed to prevent these
> >> types of problems by effectively forcing the compiler to assume byte
> >> alignment?
> > Not per definition, while the pack does place most members at unaligned
> > addresses, it does not control if the data can be loaded from it. Since the
> > 4.7+ compilers by default assume that the chip does support unaligned
> > accesses, it just generates ldr instruction to get the uint32_t members
> > (and thus trap on this mcu). Older versions (or 4.7 with
> > -mno-unaligned-access)
> > will generate byte fetches due to the pack since it assumes the chip does
> > not support unaligned accesses.
> >
> > So when compiled with a older compiler the bitmap could be loaded anywhere.
> > With 4.7+ it is faster but needs to be aligned (in a bit weird manner).
> 
> Hmm... Then this means that lcd.c is similarly broken; it makes the same
> accesses and the same assumptions about the load address.
> 
> Personally, I don't like the idea that board users should be aware of
> the architecture's capabilities or the internal structure of BMP files
> in order to select a correct load address, so requiring them to load it
> to aligned address - 2 really irks me.
> 
> README.arm-unaligned-accesses does list standard compliance as a
> possible reason for allowing emulated unaligned accesses, and the
> format of the BMP header is certainly a standard of the BMP file format,
> so I wonder if this constitutes a good reason to allow emulated
> unaligned accesses for lcd.c?

IMO no, it does not, for two reasons:

- generally speaking, standard formats and known formats should not be
  confused, and here, we are dealing with a known, but not standard,
  format;

- concerning the exception in README.arm-unaligned-accesses exception,
  it is about cases where the structure is going to be used by hardware
  (either local or external to the system) which requires conformance,
  for instance, network frames. Here, the BMB header is not used by any
  hardware.

> Barring that, we should at least protect lcd.c from this issue by
> making some sort of check for affected targets, or maybe limit the
> possible values for splashimage... This issue makes it way too easy
> to accidentally break the boot process in a way that's hard to recover
> from.

I suggest a few solutions:

1) enforce given load address alignment so that BMP header fields are
natively aligned, with a clear error message. Simple to implement,
difficut for users to understand and accept.

2) once the address provided by the user is known, if it is not
properly aligned, then the next properly aligned address should be
used, and the byte at given address should contain the offset from the
given address to the used address. This is a general solution that
works for any given load address, odd ones included:

Given address:  First bytes:    Used address:
10000000        2 x 'B' 'M'     10000002
10000001        1 'B' 'M'       10000002
10000002        'B' 'M'         10000002
10000003        3 x x 'B' 'M'   10000006
10000004        2 x 'B' 'M'     10000006
...

Note that if the user address is constrained to be 4-byte-aligned,
then only the "2 x 'B' 'M'" case would apply.

3) define an internal 'BMP holder' structure which contains a
two-byte prefix before the 'BMP file' header and data. This way, if
the overall structure is aligned, then the fields in the BMP header are
aligned too.

4) Build a time machine and tell the designers of the BMP header format,
in great inventive and colorful detail, what horrible things will happen
to them if they don't order and size their fields so that they naturally
land on aligned offsets from the header start. This solution gives the
best results IMO.

5) if none the above (including 4) is feasible for some reason, then
use unaligned accessors for this BMP fields, with a Big Fat Comment
about why this is so.

Note that all solutions except 2 (and 4) depend on the given address
being constrained in some way -- such a constraint does not seem
excessive to me.

Amicalement,
-- 
Albert.