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

Add support for loading splash image from NAND

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 board/cm_t35/cm_t35.c    |   61 ++++++++++++++++++++++++++++++++++++++++++++++
 include/configs/cm_t35.h |    4 +++
 2 files changed, 65 insertions(+)

Hi Nikita,

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
> Add support for loading splash image from NAND
>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>   board/cm_t35/cm_t35.c    |   61 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/configs/cm_t35.h |    4 +++
>   2 files changed, 65 insertions(+)
>
> diff --git a/board/cm_t35/cm_t35.c b/board/cm_t35/cm_t35.c
> index 8f3d735..8dbdb44 100644
> --- a/board/cm_t35/cm_t35.c
> +++ b/board/cm_t35/cm_t35.c
> @@ -33,7 +33,9 @@
>   #include <net.h>
>   #include <i2c.h>
>   #include <usb.h>
> +#include <nand.h>
>   #include <twl4030.h>
> +#include <bmp_layout.h>
>   #include <linux/compiler.h>
>   
>   #include <asm/io.h>
> @@ -75,6 +77,65 @@ static u32 gpmc_nand_config[GPMC_MAX_REG] = {
>   	0,
>   };
>   
> +#ifdef CONFIG_LCD
> +#ifdef CONFIG_CMD_NAND
> +static int splash_load_from_nand(u32 bmp_load_addr)
> +{
> +	struct bmp_header *bmp_hdr;
> +	int res, splash_screen_nand_offset = 0x100000;
> +	size_t bmp_size, bmp_header_size = sizeof(struct bmp_header);
> +
> +	if (bmp_load_addr + bmp_header_size >= gd->start_addr_sp)
> +		goto splash_address_too_high;
> +
> +	res = nand_read_skip_bad(&nand_info[nand_curr_device],
> +			splash_screen_nand_offset, &bmp_header_size,
> +			(u_char *)bmp_load_addr);
> +	if (res < 0)
> +		return res;
> +
> +	bmp_hdr = (struct bmp_header *)bmp_load_addr;
> +	bmp_size = le32_to_cpu(bmp_hdr->file_size);
> +
> +	if (bmp_load_addr + bmp_size >= gd->start_addr_sp)
> +		goto splash_address_too_high;
> +
> +	return nand_read_skip_bad(&nand_info[nand_curr_device],
> +			splash_screen_nand_offset, &bmp_size,
> +			(u_char *)bmp_load_addr);
> +
> +splash_address_too_high:
> +	printf("Error: splashimage address too high. Data overwrites U-Boot "
> +		"and/or placed beyond DRAM boundaries.\n");
> +
> +	return -1;
> +}
> +#else
> +static inline int splash_load_from_nand(void)
> +{
> +	return -1;
> +}
> +#endif /* CONFIG_CMD_NAND */
> +
> +int board_splash_screen_prepare(void)
> +{
> +	char *env_splashimage_value;
> +	u32 bmp_load_addr;
> +
> +	env_splashimage_value = getenv("splashimage");
> +	if (env_splashimage_value == NULL)
> +		return -1;
> +
> +	bmp_load_addr = simple_strtoul(env_splashimage_value, 0, 16);
> +	if (bmp_load_addr == 0) {
> +		printf("Error: bad splashimage address specified\n");
> +		return -1;
> +	}
> +
> +	return splash_load_from_nand(bmp_load_addr);
> +}
> +#endif /* CONFIG_LCD */
> +
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).

Regards,
Jeroen

Hi Jeroen,

On 12/24/2012 10:55 AM, Jeroen Hofstee wrote:
> Hi Nikita,
>
> On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
>> Add support for loading splash image from NAND
>>
>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
>> ---
>>   board/cm_t35/cm_t35.c    |   61
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/configs/cm_t35.h |    4 +++
>>   2 files changed, 65 insertions(+)
>>
>> diff --git a/board/cm_t35/cm_t35.c b/board/cm_t35/cm_t35.c
>> index 8f3d735..8dbdb44 100644
>> --- a/board/cm_t35/cm_t35.c
>> +++ b/board/cm_t35/cm_t35.c
>> @@ -33,7 +33,9 @@
>>   #include <net.h>
>>   #include <i2c.h>
>>   #include <usb.h>
>> +#include <nand.h>
>>   #include <twl4030.h>
>> +#include <bmp_layout.h>
>>   #include <linux/compiler.h>
>>   #include <asm/io.h>
>> @@ -75,6 +77,65 @@ static u32 gpmc_nand_config[GPMC_MAX_REG] = {
>>       0,
>>   };
>> +#ifdef CONFIG_LCD
>> +#ifdef CONFIG_CMD_NAND
>> +static int splash_load_from_nand(u32 bmp_load_addr)
>> +{
>> +    struct bmp_header *bmp_hdr;
>> +    int res, splash_screen_nand_offset = 0x100000;
>> +    size_t bmp_size, bmp_header_size = sizeof(struct bmp_header);
>> +
>> +    if (bmp_load_addr + bmp_header_size >= gd->start_addr_sp)
>> +        goto splash_address_too_high;
>> +
>> +    res = nand_read_skip_bad(&nand_info[nand_curr_device],
>> +            splash_screen_nand_offset, &bmp_header_size,
>> +            (u_char *)bmp_load_addr);
>> +    if (res < 0)
>> +        return res;
>> +
>> +    bmp_hdr = (struct bmp_header *)bmp_load_addr;
>> +    bmp_size = le32_to_cpu(bmp_hdr->file_size);
>> +
>> +    if (bmp_load_addr + bmp_size >= gd->start_addr_sp)
>> +        goto splash_address_too_high;
>> +
>> +    return nand_read_skip_bad(&nand_info[nand_curr_device],
>> +            splash_screen_nand_offset, &bmp_size,
>> +            (u_char *)bmp_load_addr);
>> +
>> +splash_address_too_high:
>> +    printf("Error: splashimage address too high. Data overwrites
>> U-Boot "
>> +        "and/or placed beyond DRAM boundaries.\n");
>> +
>> +    return -1;
>> +}
>> +#else
>> +static inline int splash_load_from_nand(void)
>> +{
>> +    return -1;
>> +}
>> +#endif /* CONFIG_CMD_NAND */
>> +
>> +int board_splash_screen_prepare(void)
>> +{
>> +    char *env_splashimage_value;
>> +    u32 bmp_load_addr;
>> +
>> +    env_splashimage_value = getenv("splashimage");
>> +    if (env_splashimage_value == NULL)
>> +        return -1;
>> +
>> +    bmp_load_addr = simple_strtoul(env_splashimage_value, 0, 16);
>> +    if (bmp_load_addr == 0) {
>> +        printf("Error: bad splashimage address specified\n");
>> +        return -1;
>> +    }
>> +
>> +    return splash_load_from_nand(bmp_load_addr);
>> +}
>> +#endif /* CONFIG_LCD */
>> +
> 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?

Albert, can you shed some light on this?

>
> Regards,
> Jeroen
>

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

Regards,
Jeroen

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?

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.

>
> Regards,
> Jeroen
>
>
>

Hi Nikita,

On Sun, 30 Dec 2012 16:39:06 +0200, Nikita Kiryanov
<nikita@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,

Hi Albert,

On 01/22/2013 09:37 AM, Albert ARIBAUD wrote:
> Hi Nikita,
>

[...]

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

Yes I agree that from a user point of view this looks terrible, which is
why I prefer not to do something like this.

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

I think a simpler way to implement something like this is to just
use modulo 4 to check alignment and fix the address dynamically;
perhaps even fixing it in the environment.

This is a localized approach though. We will have to do this from
all the code paths that lead to a bmp header being probed in memory.
I would prefer a more localized solution.

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

The problem with 3 (and 4) is that it still doesn't protect the user
from bricking their board by choosing a non-aligned address for their
BMP. This might happen because the user:
- is unaware of the dangers of choosing a non-aligned address
- made a typo
- relies on a script or program that runs under U-Boot to setup stuff
- I"m sure there are other possibilities

In terms of usability it's a *big* regression. If we do not actually
prevent the user from setting splashimage to an unaligned address we
should make sure that all usable addresses are safe.

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

I think, once this feature is merged, I'll try to see if something nice
can be done with an approach like this.
For now I'll add a suggestion-#2-style check in lcd.c

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

On 01/23/2013 12:47 PM, Nikita Kiryanov wrote:
> Hi Albert,
>
> On 01/22/2013 09:37 AM, Albert ARIBAUD wrote:
>> Hi Nikita,
>>
>
> [...]
>
>> Note that if the user address is constrained to be 4-byte-aligned,
>> then only the "2 x 'B' 'M'" case would apply.
>
> I think a simpler way to implement something like this is to just
> use modulo 4 to check alignment and fix the address dynamically;
> perhaps even fixing it in the environment.
>
> This is a localized approach though. We will have to do this from
> all the code paths that lead to a bmp header being probed in memory.
> I would prefer a more localized solution.

Correction:
"I would prefer a more global solution."

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

On Sat, Dec 22, 2012 at 09:03:48PM -0000, Nikita Kiryanov wrote:

> Add support for loading splash image from NAND
> 
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>

Applied to u-boot-ti/master (and already pulled into u-boot-arm),
thanks!