Hello Nikita,

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:

Add useful omap3 dss defines for: polarity, TFT data lines, lcd display type, gfx burst size, and gfx format

Signed-off-by: Nikita Kiryanov nikita@compulab.co.il Signed-off-by: Igor Grinberg grinberg@compulab.co.il

---

arch/arm/include/asm/arch-omap3/dss.h | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)

diff --git a/arch/arm/include/asm/arch-omap3/dss.h b/arch/arm/include/asm/arch-omap3/dss.h index ffaffbb..cb6d746 100644 --- a/arch/arm/include/asm/arch-omap3/dss.h +++ b/arch/arm/include/asm/arch-omap3/dss.h @@ -167,6 +167,41 @@ struct venc_regs { #define VENC_OUT_SEL (1 << 6) #define DIG_LPP_SHIFT 16

+/* LCD display type */ +#define PASSIVE_DISPLAY 0 +#define ACTIVE_DISPLAY 1

+/* TFTDATALINES */ +#define LCD_INTERFACE_12_BIT 0 +#define LCD_INTERFACE_16_BIT 1 +#define LCD_INTERFACE_18_BIT 2 +#define LCD_INTERFACE_24_BIT 3

+/* Polarity */ +#define DSS_IVS (1 << 12) +#define DSS_IHS (1 << 13) +#define DSS_IPC (1 << 14) +#define DSS_IEO (1 << 15)

+/* GFX format */ +#define GFXFORMAT_BITMAP1 0x0 +#define GFXFORMAT_BITMAP2 0x1 +#define GFXFORMAT_BITMAP4 0x2 +#define GFXFORMAT_BITMAP8 0x3 +#define GFXFORMAT_RGB12 0x4 +#define GFXFORMAT_ARGB16 0x5 +#define GFXFORMAT_RGB16 0x6 +#define GFXFORMAT_RGB24_UNPACKED 0x8 +#define GFXFORMAT_RGB24_PACKED 0x9 +#define GFXFORMAT_ARGB32 0xC +#define GFXFORMAT_RGBA32 0xD +#define GFXFORMAT_RGBx32 0xE

+/* GFX burst size */ +#define GFXBURSTSIZE4 0 +#define GFXBURSTSIZE8 1 +#define GFXBURSTSIZE16 2

• /* Panel Configuration */ struct panel_config { u32 timing_h;

most defines in omap dss use the location in the silicon itself. For consistency you might want to shift these values to the appropriate place. (or just use 32 mode so you can drop most if not all of them)

Regards, Jeroen

Hi Jeroen,

On 01/20/2013 10:34 PM, Jeroen Hofstee wrote: [...]

...

diff --git a/include/lcd.h b/include/lcd.h index c24164a..4ac4ddd 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; extern void lcd_ctrl_init (void *lcdbase); extern void lcd_enable (void); +extern int board_splash_screen_prepare(void); /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,

Other boards seem to do this in lcd_enable. Perhaps that is also an option.

The problem with doing it in lcd_enable is that it's akin to a side effect, given what the function's name advertises. Preparing the splash image can, however, be a natural part of the process that displays it.

Regards, Jeroen

On 01/21/2013 09:14 PM, Jeroen Hofstee wrote:

Hello Nikita,

On 01/21/2013 08:51 AM, Nikita Kiryanov wrote:

...

Hi Jeroen,

On 01/20/2013 10:34 PM, Jeroen Hofstee wrote: [...]

...

...

diff --git a/include/lcd.h b/include/lcd.h index c24164a..4ac4ddd 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; extern void lcd_ctrl_init (void *lcdbase); extern void lcd_enable (void); +extern int board_splash_screen_prepare(void); /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,

Other boards seem to do this in lcd_enable. Perhaps that is also an option.

The problem with doing it in lcd_enable is that it's akin to a side effect, given what the function's name advertises. Preparing the splash image can, however, be a natural part of the process that displays it.

mmm, I am not so sure I agree that loading a bitmap in lcd_enable is a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_.

Well, it is a problem. If we don't respect the abstractions we create then things like function names become meaningless. A function called "lcd_enable" should do just that- enable lcd. Not load stuff from storage to memory or manipulate BMPs.

But anyway, can't this at least be changed to a __weak function, so the CONFIG and ifdef stuff can be dropped?

The motivation behind the CONFIG was to make it a documentable user setting, rather than an undocumented feature buried in the code.

Regards, Jeroen

On 01/24/13 00:13, Jeroen Hofstee wrote:

Hello Nikita,

On 01/23/2013 09:31 AM, Nikita Kiryanov wrote:

...

On 01/21/2013 09:14 PM, Jeroen Hofstee wrote:

...

Hello Nikita,

On 01/21/2013 08:51 AM, Nikita Kiryanov wrote:

...

Hi Jeroen,

On 01/20/2013 10:34 PM, Jeroen Hofstee wrote: [...]

...

...

diff --git a/include/lcd.h b/include/lcd.h index c24164a..4ac4ddd 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -47,6 +47,7 @@ extern struct vidinfo panel_info; extern void lcd_ctrl_init (void *lcdbase); extern void lcd_enable (void); +extern int board_splash_screen_prepare(void); /* setcolreg used in 8bpp/16bpp; initcolregs used in monochrome */ extern void lcd_setcolreg (ushort regno,

Other boards seem to do this in lcd_enable. Perhaps that is also an option.

The problem with doing it in lcd_enable is that it's akin to a side effect, given what the function's name advertises. Preparing the splash image can, however, be a natural part of the process that displays it.

mmm, I am not so sure I agree that loading a bitmap in lcd_enable is a _problem_, while loading it in show logo and requiring a CONFIG_* is _natural_.

Well, it is a problem. If we don't respect the abstractions we create then things like function names become meaningless. A function called "lcd_enable" should do just that- enable lcd. Not load stuff from storage to memory or manipulate BMPs.

my point is that lcd_clear will e.g. call lcd_logo. Although I haven't tested it, it seems you're make a side effect of a function only called once a side effect of another function (which could be called multiple times). So you make things even worse (loading an bitmap while the function is just named to display it).

So what's your point? Do you think we should add a splash screen specific callback inside the board.c U-Boot boot flow? Please, be more specific, as both approaches are not suitable according to what was said above...

...

...

But anyway, can't this at least be changed to a __weak function, so the CONFIG and ifdef stuff can be dropped?

The motivation behind the CONFIG was to make it a documentable user setting, rather than an undocumented feature buried in the code.

then document the callback...

Sorry, may be I've missed something, but I can't see any callback being documented in the README file...

I don't see the improvement of this patch..

What does that suppose to mean? Either be constructive or don't bother... I'd like to hear Anatolij's opinion on this.

Hi Jeroen,

On 01/20/2013 10:59 PM, Jeroen Hofstee wrote:

Hello Nikita,

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:

...

Add support for dvi displays with user selectable dvi presets.

Signed-off-by: Nikita Kiryanov nikita@compulab.co.il Signed-off-by: Igor Grinberg grinberg@compulab.co.il

---

[...]

...

+/*

• • The frame buffer is allocated before we have the chance to parse

user input.

• • To make sure enough memory is allocated for all resolutions, we

define

• • vl_{col | row} to the maximal resolution supported by OMAP3.
• */

+vidinfo_t panel_info = {

• .vl_col = 1400,
• .vl_row = 1050,
• .vl_bpix = 4, /* 16-bits pixel data */

There is no need to hardcode the magic 4, just use LCD_BPP

Good point.

...

• .cmap = (ushort *)0x80100000,

+};

+static struct panel_config panel_cfg; +static enum display_type lcd_def;

+static const struct panel_config preset_dvi_640X480 = {

• .lcd_size = PANEL_LCD_SIZE(640, 480),
• .timing_h = DSS_HBP(48) | DSS_HFP(16) | DSS_HSW(96),
• .timing_v = DSS_VBP(33) | DSS_VFP(10) | DSS_VSW(2),
• .divisor = 12 | (1 << 16),
• .data_lines = LCD_INTERFACE_24_BIT,
• .panel_type = ACTIVE_DISPLAY,
• .load_mode = 2,

+};

[...]

...

• else if (!strncmp(displaytype, "dvi800x600", 10))

•    return set_dvi_preset(preset_dvi_800X600, 800, 600);
  

• else if (!strncmp(displaytype, "dvi1024x768", 11))

•    return set_dvi_preset(preset_dvi_1024X768, 1024, 768);
  

• else if (!strncmp(displaytype, "dvi1152x864", 11))

•    return set_dvi_preset(preset_dvi_1152X864, 1152, 864);
  

• else if (!strncmp(displaytype, "dvi1280x960", 11))

•    return set_dvi_preset(preset_dvi_1280X960, 1280, 960);
  

• else if (!strncmp(displaytype, "dvi1280x1024", 12))

•    return set_dvi_preset(preset_dvi_1280X1024, 1280, 1024);
  

• return NONE;

+}

I think the lcd_line_length is no longer needed. ( but I haven't checked) Wondering if this should be board specific..

These variables are here because at the time the implementation of lcd.c required them to be defined by the board. If you succeed in removing them it will be a simple matter of a clean up patch.

...

+int lcd_line_length; +int lcd_color_fg; +int lcd_color_bg; +void *lcd_base; +short console_col; +short console_row; +void *lcd_console_address;

fyi, I try to get rid of those, see http://patchwork.ozlabs.org/patch/211562/. Will repost v2 after this... No idea how this gets merged, but you might end with some unused globals.

Yes that should be the extent of the "damage".

...

+void lcd_ctrl_init(void *lcdbase) +{

• struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE;
• struct prcm *prcm = (struct prcm *)PRCM_BASE;
• char *displaytype = getenv("displaytype");
• if (displaytype == NULL)

•    return;
  

• lcd_def = env_parse_displaytype(displaytype);
• if (lcd_def == NONE)

•    return;
  

• panel_cfg.frame_buffer = lcdbase;
• omap3_dss_panel_config(&panel_cfg);
• /*

• * Pixel clock is defined with many divisions and only few
  

• * multiplications of the system clock. Since DSS FCLK divisor is
  

set

• * to 16 by default, we need to set it to a smaller value, like 3
  

• * (chosen via trial and error).
  

• */
  

bah, guessing timer values, this might help you https://github.com/jhofstee/u-boot/blob/bpp3_dev2/board/victron/bpp3/lcd_tim...

(a bit old, but simply downloading the file should work, and yes it might warn a bit)

Thanks. I'll consider it for future extensions to this code, but for the time being the guess serves its purpose.

The whole routine should go to the video driver in my opinion.

What this function is, is predefines + call to omap3_dss_panel_config() + some corrections. Anything related to the predefines is not generic. They have many assumptions in them (such as whether the screen is active or passive) and they are selected using a user interface that is also specific to our board.

The adjustment after the call to omap3_dss_panel_config() is, once again, specific to our own choices.

So overall, I don't think this is fit to be a generic driver function.

...

• clrsetbits_le32(&prcm->clksel_dss, 0xF, 3);
• /*

• * Some of what the omap3_dss_panel_config() function did, needs
  

to be

• * adjusted, otherwise the image will be messed up/not appear at
  

all.

• */
  

• clrsetbits_le32(&dispc->gfx_attributes, 0xF << 1, GFXFORMAT_RGB16

<< 1);

• clrsetbits_le32(&dispc->gfx_attributes, 0x3 << 6, GFXBURSTSIZE16

<< 6); +}

mmm, do you really need 16 bit support?

Yes, we do want 16 bit support.

lcd.c is easily extended to 32 bit support (I have a patch for it) and then you can drop the driver adjustment. Anyway, if you want 16 bit support it should go into the driver and not in your board in my opinion.

Addressed above.

...

+void lcd_enable(void) +{

• if (lcd_def == DVI) {

•    gpio_direction_output(54, 0); /* Turn on DVI */
  

•    omap3_dss_enable();
  

• }

+}

+void lcd_setcolreg(ushort regno, ushort red, ushort green, ushort blue) {} diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h index fa6eb4e..8544b15 100644 --- a/include/configs/cm_t35.h +++ b/include/configs/cm_t35.h @@ -339,4 +339,10 @@ #define CONFIG_OMAP3_GPIO_6 /* GPIO186 is in GPIO bank 6 */ #endif +/* Display Configuration */ +#define CONFIG_OMAP3_GPIO_2 +#define CONFIG_VIDEO_OMAP3

+#define CONFIG_LCD

• #endif /* __CONFIG_H */

Regards, Jeroen

On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:

Add support for user defined lcd parameters for cm-t35 splash screen.

Signed-off-by: Nikita Kiryanov nikita@compulab.co.il Signed-off-by: Igor Grinberg grinberg@compulab.co.il

---

board/cm_t35/display.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 210 insertions(+), 3 deletions(-)

diff --git a/board/cm_t35/display.c b/board/cm_t35/display.c index 11b8ed9..7b09bce 100644 --- a/board/cm_t35/display.c +++ b/board/cm_t35/display.c @@ -3,6 +3,8 @@

• Authors: Nikita Kiryanov nikita@compulab.co.il

• • Parsing code based on linux/drivers/video/pxafb.c
• • See file CREDITS for list of people who contributed to this
  • project.

@@ -33,6 +35,7 @@ DECLARE_GLOBAL_DATA_PTR; enum display_type { NONE, DVI,

• DVI_CUSTOM, };

  /*

@@ -138,6 +141,205 @@ static enum display_type set_dvi_preset(const struct panel_config preset, }

/*

• • parse_mode() - parse the mode parameter of custom lcd settings
• • @mode: <res_x>x<res_y>
• • Returns -1 on error, 0 on success.
• */

+static int parse_mode(const char *mode) +{

• unsigned int modelen = strlen(mode);
• int res_specified = 0;
• unsigned int xres = 0, yres = 0;
• int yres_specified = 0;
• int i;
• for (i = modelen - 1; i >= 0; i--) {

• switch (mode[i]) {
  

• case 'x':
  

• 	if (!yres_specified) {
  

• 		yres = simple_strtoul(&mode[i + 1], NULL, 0);
  

• 		yres_specified = 1;
  

• 	} else {
  

• 		goto done_parsing;
  

• 	}
  

• 	break;
  

• case '0' ... '9':
  

• 	break;
  

• default:
  

• 	goto done_parsing;
  

• }
  

• }
• if (i < 0 && yres_specified) {

• xres = simple_strtoul(mode, NULL, 0);
  

• res_specified = 1;
  

• }

+done_parsing:

• if (res_specified) {

• set_resolution_params(xres, yres);
  

• } else {

• printf("LCD: invalid mode: %s\n", mode);
  

• return -1;
  

• }
• return 0;

+}

+#define PIXEL_CLK_NUMERATOR (26 * 432 / 39) +/*

• • parse_pixclock() - Parse the pixclock parameter of custom lcd settings
• • @pixclock: the desired pixel clock
• • Returns -1 on error, 0 on success.
• • Handling the pixel_clock:
• • Pixel clock is defined in the OMAP35x TRM as follows:
• • pixel_clock =
• • (SYS_CLK * 2 * PRCM.CM_CLKSEL2_PLL[18:8]) /
• • (DSS.DISPC_DIVISOR[23:16] * DSS.DISPC_DIVISOR[6:0] *
• • PRCM.CM_CLKSEL_DSS[4:0] * (PRCM.CM_CLKSEL2_PLL[6:0] + 1))
• • In practice, this means that in order to set the
• • divisor for the desired pixel clock one needs to
• • solve the following equation:
• • 26 * 432 / (39 * <pixel_clock>) = DSS.DISPC_DIVISOR[6:0]
• • NOTE: the explicit equation above is reduced. Do not
• • try to infer anything from these numbers.
• */

+static int parse_pixclock(char *pixclock) +{

• int divisor, pixclock_val;
• char *pixclk_start = pixclock;
• pixclock_val = simple_strtoul(pixclock, &pixclock, 10);
• divisor = DIV_ROUND_UP(PIXEL_CLK_NUMERATOR, pixclock_val);
• /* 0 and 1 are illegal values for PCD */
• if (divisor <= 1)

• divisor = 2;
  

• panel_cfg.divisor = divisor | (1 << 16);
• if (pixclock[0] != '\0') {

• printf("LCD: invalid value for pixclock:%s\n", pixclk_start);
  

• return -1;
  

• }
• return 0;

+}

+/*

• • parse_setting() - parse a single setting of custom lcd parameters
• • @setting: The custom lcd setting <name>:<value>
• • Returns -1 on failure, 0 on success.
• */

+static int parse_setting(char *setting) +{

• int num_val;
• char *setting_start = setting;
• if (!strncmp(setting, "mode:", 5)) {

• return parse_mode(setting + 5);
  

• } else if (!strncmp(setting, "pixclock:", 9)) {

• return parse_pixclock(setting + 9);
  

• } else if (!strncmp(setting, "left:", 5)) {

• num_val = simple_strtoul(setting + 5, &setting, 0);
  

• panel_cfg.timing_h |= DSS_HBP(num_val);
  

• } else if (!strncmp(setting, "right:", 6)) {

• num_val = simple_strtoul(setting + 6, &setting, 0);
  

• panel_cfg.timing_h |= DSS_HFP(num_val);
  

• } else if (!strncmp(setting, "upper:", 6)) {

• num_val = simple_strtoul(setting + 6, &setting, 0);
  

• panel_cfg.timing_v |= DSS_VBP(num_val);
  

• } else if (!strncmp(setting, "lower:", 6)) {

• num_val = simple_strtoul(setting + 6, &setting, 0);
  

• panel_cfg.timing_v |= DSS_VFP(num_val);
  

• } else if (!strncmp(setting, "hsynclen:", 9)) {

• num_val = simple_strtoul(setting + 9, &setting, 0);
  

• panel_cfg.timing_h |= DSS_HSW(num_val);
  

• } else if (!strncmp(setting, "vsynclen:", 9)) {

• num_val = simple_strtoul(setting + 9, &setting, 0);
  

• panel_cfg.timing_v |= DSS_VSW(num_val);
  

• } else if (!strncmp(setting, "hsync:", 6)) {

• if (simple_strtoul(setting + 6, &setting, 0) == 0)
  

• 	panel_cfg.pol_freq |= DSS_IHS;
  

• else
  

• 	panel_cfg.pol_freq &= ~DSS_IHS;
  

• } else if (!strncmp(setting, "vsync:", 6)) {

• if (simple_strtoul(setting + 6, &setting, 0) == 0)
  

• 	panel_cfg.pol_freq |= DSS_IVS;
  

• else
  

• 	panel_cfg.pol_freq &= ~DSS_IVS;
  

• } else if (!strncmp(setting, "outputen:", 9)) {

• if (simple_strtoul(setting + 9, &setting, 0) == 0)
  

• 	panel_cfg.pol_freq |= DSS_IEO;
  

• else
  

• 	panel_cfg.pol_freq &= ~DSS_IEO;
  

• } else if (!strncmp(setting, "pixclockpol:", 12)) {

• if (simple_strtoul(setting + 12, &setting, 0) == 0)
  

• 	panel_cfg.pol_freq |= DSS_IPC;
  

• else
  

• 	panel_cfg.pol_freq &= ~DSS_IPC;
  

• } else if (!strncmp(setting, "active", 6)) {

• panel_cfg.panel_type = ACTIVE_DISPLAY;
  

• return 0; /* Avoid sanity check below */
  

• } else if (!strncmp(setting, "passive", 7)) {

• panel_cfg.panel_type = PASSIVE_DISPLAY;
  

• return 0; /* Avoid sanity check below */
  

• } else if (!strncmp(setting, "display:", 8)) {

• if (!strncmp(setting + 8, "dvi", 3)) {
  

• 	lcd_def = DVI_CUSTOM;
  

• 	return 0; /* Avoid sanity check below */
  

• }
  

• } else {

• printf("LCD: unknown option %s\n", setting_start);
  

• return -1;
  

• }
• if (setting[0] != '\0') {

• printf("LCD: invalid value for %s\n", setting_start);
  

• return -1;
  

• }
• return 0;

+}

+/*

• • env_parse_customlcd() - parse custom lcd params from an environment variable.
• • @custom_lcd_params: The environment variable containing the lcd params.
• • Returns -1 on failure, 0 on success.
• */

+static int parse_customlcd(char *custom_lcd_params) +{

• char params_cpy[160];
• char *setting;
• strncpy(params_cpy, custom_lcd_params, 160);

I fail to understand why you want to copy this.

• setting = strtok(params_cpy, ",");
• while (setting) {

• if (parse_setting(setting) < 0)
  

• 	return -1;
  

• setting = strtok(NULL, ",");
  

• }
• /* Currently we don't support changing this via custom lcd params */
• panel_cfg.data_lines = LCD_INTERFACE_24_BIT;

again, if you only support 24 panels, why not drive them as such?

• return 0;

+}

Is above really board specific or should it be in omap_videomodes.c or whatever?

+/*

• env_parse_displaytype() - parse display type.
• Parses the environment variable "displaytype", which contains the

@@ -176,14 +378,19 @@ void lcd_ctrl_init(void *lcdbase) { struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE; struct prcm *prcm = (struct prcm *)PRCM_BASE;

• char *custom_lcd; char *displaytype = getenv("displaytype");

  if (displaytype == NULL) return;

  lcd_def = env_parse_displaytype(displaytype);

• if (lcd_def == NONE)

• return;
  

• /* If we did not recognize the preset, check if it's an env variable */

• if (lcd_def == NONE) {

• custom_lcd = getenv(displaytype);
  

• if (custom_lcd == NULL || parse_customlcd(custom_lcd) < 0)
  

• 	return;
  

• }

  panel_cfg.frame_buffer = lcdbase; omap3_dss_panel_config(&panel_cfg);

@@ -204,7 +411,7 @@ void lcd_ctrl_init(void *lcdbase)

void lcd_enable(void) {

• if (lcd_def == DVI) {

• if (lcd_def == DVI || lcd_def == DVI_CUSTOM) { gpio_direction_output(54, 0); /* Turn on DVI */ omap3_dss_enable(); }

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