lcd: fix console address is not initialized

This commit 904672e (lcd: refactor lcd console stuff into its own file), which cause lcd console address is not initialized.

This patch split lcd console address initialize and lcd logo display into two functions.

Signed-off-by Bo Shen <[email protected]>

---

  common/lcd.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)

common/lcd.c +5 -3

index cc34b8a..f435e2a 100644
@@ -82,7 +82,8 @@ DECLARE_GLOBAL_DATA_PTR;
 
  static int lcd_init(void *lcdbase);
 
-static void *lcd_logo(void);
+static void lcd_logo(void);
+static void *lcd_console_address(void);
 
  static void lcd_setfgcolor(int color);
  static void lcd_setbgcolor(int color);

@@ -268,7 +269,8 @@ void lcd_clear(void)
  	console_rows = panel_info.vl_row / VIDEO_FONT_HEIGHT;
  #endif
  	console_cols = panel_info.vl_col / VIDEO_FONT_WIDTH;
-	lcd_init_console(lcd_logo(), console_rows, console_cols);
+	lcd_init_console(lcd_console_address(), console_rows, console_cols);
+	lcd_logo();
  	lcd_sync();
  }
 

@@ -849,7 +851,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
  }
  #endif
 
-static void *lcd_logo(void)
+static void lcd_logo(void)
  {
  #ifdef CONFIG_SPLASH_SCREEN
  	char *s;

Hi Bo,

Quoted 2 lines · Bo Shen

This commit 904672e (lcd: refactor lcd console stuff into its own file), which cause lcd console address is not initialized.

Based on your fix, I'm certain that the bug was introduced in a previous patch, perhaps 140beb9 (lcd: expand console api).

Also, can you provide a more detailed explanation of when this happens and how?

Quoted 54 lines

This patch split lcd console address initialize and lcd logo display into two functions.

Signed-off-by Bo Shen <[email protected]>

---

  common/lcd.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)

common/lcd.c +8 -3

index cc34b8a..f435e2a 100644
@@ -82,7 +82,8 @@ DECLARE_GLOBAL_DATA_PTR;
 
  static int lcd_init(void *lcdbase);
 
-static void *lcd_logo(void);
+static void lcd_logo(void);
+static void *lcd_console_address(void);
 
  static void lcd_setfgcolor(int color);
  static void lcd_setbgcolor(int color);

@@ -268,7 +269,8 @@ void lcd_clear(void)
      console_rows = panel_info.vl_row / VIDEO_FONT_HEIGHT;
  #endif
      console_cols = panel_info.vl_col / VIDEO_FONT_WIDTH;
-    lcd_init_console(lcd_logo(), console_rows, console_cols);
+    lcd_init_console(lcd_console_address(), console_rows, console_cols);
+    lcd_logo();
      lcd_sync();
  }
 

@@ -849,7 +851,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
  }
  #endif
 
-static void *lcd_logo(void)
+static void lcd_logo(void)
  {
  #ifdef CONFIG_SPLASH_SCREEN
      char *s;

@@ -879,7 +881,10 @@ static void *lcd_logo(void)
      lcd_set_row(LCD_INFO_Y / VIDEO_FONT_HEIGHT);
      lcd_show_board_info();
  #endif /* CONFIG_LCD_INFO */
+}
 
+static void *lcd_console_address(void)
+{
  #if defined(CONFIG_LCD_LOGO) && !defined(CONFIG_LCD_INFO_BELOW_LOGO)
      return (void *)((ulong)lcd_base + BMP_LOGO_HEIGHT *
 lcd_line_length);

Quoted 2 lines

  #else

I would like to see some mention of why it's ok to redefine the console address in such a way. At first glance it looks like there is a slight change of

behavior (the value no longer depends on whether splash image was loaded or not), but it seems to be ok if you go over the various cases. The only instance where I see the difference could manifest itself is if lcd console would start writing to the frame buffer while the splash screen is still on,
but that doesn't look good regardless of where the console starts, so I'm ok
with that.

Hi Nikita Kiryanov,

Quoted 11 lines · Nikita Kiryanov

Hi Bo,

Quoted 2 lines · Bo Shen

This commit 904672e (lcd: refactor lcd console stuff into its own file), which cause lcd console address is not initialized.

Based on your fix, I'm certain that the bug was introduced in a previous patch, perhaps 140beb9 (lcd: expand console api).

Also, can you provide a more detailed explanation of when this happens and how?

It will cause the system hang.

Before this patch, lcd_logo -> lcd_show_board_info (CONFIG_LCD_INFO) -> .. -> lcd_drawchars. It has the following lines:
--->8---
dest = (uchar *)(lcd_base + y * lcd_line_length + x * NBITS(LCD_BPP)/8); ---8<---

while with the patch,
--->8---
dest = (uchar *)(lcd_console_address +

                          y * lcd_line_length + x * NBITS(LCD_BPP) / 8);

---8<---

As the lcd_console_address is initialized after lcd_logo return, so the lcd_console_address is not initialized, it is 0. When try to write to address 0, the system hang.

Quoted 63 lines

Quoted 56 lines

This patch split lcd console address initialize and lcd logo display into two functions.

Signed-off-by Bo Shen <[email protected]>

---

  common/lcd.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)

common/lcd.c +5 -3

index cc34b8a..f435e2a 100644
@@ -82,7 +82,8 @@ DECLARE_GLOBAL_DATA_PTR;
 
  static int lcd_init(void *lcdbase);
 
-static void *lcd_logo(void);
+static void lcd_logo(void);
+static void *lcd_console_address(void);
 
  static void lcd_setfgcolor(int color);
  static void lcd_setbgcolor(int color);

@@ -268,7 +269,8 @@ void lcd_clear(void)
      console_rows = panel_info.vl_row / VIDEO_FONT_HEIGHT;
  #endif
      console_cols = panel_info.vl_col / VIDEO_FONT_WIDTH;
-    lcd_init_console(lcd_logo(), console_rows, console_cols);
+    lcd_init_console(lcd_console_address(), console_rows,
 console_cols);
+    lcd_logo();
      lcd_sync();
  }

@@ -849,7 +851,7 @@ int lcd_display_bitmap(ulong bmp_image, int x,
 int y)
  }
  #endif
 
-static void *lcd_logo(void)
+static void lcd_logo(void)
  {
  #ifdef CONFIG_SPLASH_SCREEN

      char *s;

common/lcd.c +3 -0

@@ -879,7 +881,10 @@ static void *lcd_logo(void)
      lcd_set_row(LCD_INFO_Y / VIDEO_FONT_HEIGHT);
      lcd_show_board_info();
  #endif /* CONFIG_LCD_INFO */
+}
 
+static void *lcd_console_address(void)
+{
  #if defined(CONFIG_LCD_LOGO) && !defined(CONFIG_LCD_INFO_BELOW_LOGO)
      return (void *)((ulong)lcd_base + BMP_LOGO_HEIGHT *
 lcd_line_length);

Quoted 2 lines

  #else

I would like to see some mention of why it's ok to redefine the console address in such a way. At first glance it looks like there is a slight change of
behavior (the value no longer depends on whether splash image was loaded

Quoted 9 lines

or not), but it seems to be ok if you go over the various cases. The only instance where I see the difference could manifest itself is if lcd console
would start writing to the frame buffer while the splash screen is still on,
but that doesn't look good regardless of where the console starts, so I'm ok
with that.

Thanks for reminder this, I will check whether this will break the splash screen.

Hi Nikita Kiryanov,
+ Andreas, Tom

Quoted 106 lines · Bo Shen

Hi Nikita Kiryanov,

Quoted 11 lines · Nikita Kiryanov

Hi Bo,

Quoted 2 lines · Bo Shen

This commit 904672e (lcd: refactor lcd console stuff into its own file), which cause lcd console address is not initialized.

Based on your fix, I'm certain that the bug was introduced in a previous patch, perhaps 140beb9 (lcd: expand console api).

Also, can you provide a more detailed explanation of when this happens and how?

It will cause the system hang.

Before this patch, lcd_logo -> lcd_show_board_info (CONFIG_LCD_INFO) -> .. -> lcd_drawchars. It has the following lines:
--->8---
dest = (uchar *)(lcd_base + y * lcd_line_length + x * NBITS(LCD_BPP)/8); ---8<---

while with the patch,
--->8---
dest = (uchar *)(lcd_console_address +

                          y * lcd_line_length + x * NBITS(LCD_BPP) / 8);

---8<---

As the lcd_console_address is initialized after lcd_logo return, so the lcd_console_address is not initialized, it is 0. When try to write to address 0, the system hang.

Quoted 63 lines

Quoted 56 lines

This patch split lcd console address initialize and lcd logo display into two functions.

Signed-off-by Bo Shen <[email protected]>

---

  common/lcd.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)

common/lcd.c +5 -3

index cc34b8a..f435e2a 100644
@@ -82,7 +82,8 @@ DECLARE_GLOBAL_DATA_PTR;
 
  static int lcd_init(void *lcdbase);
 
-static void *lcd_logo(void);
+static void lcd_logo(void);
+static void *lcd_console_address(void);
 
  static void lcd_setfgcolor(int color);
  static void lcd_setbgcolor(int color);

@@ -268,7 +269,8 @@ void lcd_clear(void)
      console_rows = panel_info.vl_row / VIDEO_FONT_HEIGHT;
  #endif
      console_cols = panel_info.vl_col / VIDEO_FONT_WIDTH;
-    lcd_init_console(lcd_logo(), console_rows, console_cols);
+    lcd_init_console(lcd_console_address(), console_rows,
 console_cols);
+    lcd_logo();
      lcd_sync();
  }

@@ -849,7 +851,7 @@ int lcd_display_bitmap(ulong bmp_image, int x,
 int y)
  }
  #endif
 
-static void *lcd_logo(void)
+static void lcd_logo(void)
  {
  #ifdef CONFIG_SPLASH_SCREEN

      char *s;

common/lcd.c +3 -0

@@ -879,7 +881,10 @@ static void *lcd_logo(void)
      lcd_set_row(LCD_INFO_Y / VIDEO_FONT_HEIGHT);
      lcd_show_board_info();
  #endif /* CONFIG_LCD_INFO */
+}
 
+static void *lcd_console_address(void)
+{
  #if defined(CONFIG_LCD_LOGO) && !defined(CONFIG_LCD_INFO_BELOW_LOGO)
      return (void *)((ulong)lcd_base + BMP_LOGO_HEIGHT *
 lcd_line_length);

Quoted 2 lines

  #else

I would like to see some mention of why it's ok to redefine the console address in such a way. At first glance it looks like there is a slight change of
behavior (the value no longer depends on whether splash image was loaded

Quoted 9 lines

or not), but it seems to be ok if you go over the various cases. The only instance where I see the difference could manifest itself is if lcd console
would start writing to the frame buffer while the splash screen is still on,
but that doesn't look good regardless of where the console starts, so I'm ok
with that.

Quoted 2 lines

Thanks for reminder this, I will check whether this will break the splash screen.

Can we use the following patch to fix this issue?
--->8---

common/lcd.c +1 -1

index cc34b8a..1195a54 100644
@@ -268,6 +268,7 @@ void lcd_clear(void)
         console_rows = panel_info.vl_row / VIDEO_FONT_HEIGHT;
  #endif
         console_cols = panel_info.vl_col / VIDEO_FONT_WIDTH;
+       lcd_init_console(lcd_base, console_rows, console_cols);
         lcd_init_console(lcd_logo(), console_rows, console_cols);
         lcd_sync();
  }
---8<---

It first initializes the lcd console with LCD base, if the splash screen is used, new address is updated.

I need this kind of fix to be applied as soon as possible, or else, most Atmel related board are broken on u-boot master branch.

Best Regards,
Bo Shen

Quoted 16 lines

Can we use the following patch to fix this issue?
--->8---

common/lcd.c +1 -1

index cc34b8a..1195a54 100644
@@ -268,6 +268,7 @@ void lcd_clear(void)
         console_rows = panel_info.vl_row / VIDEO_FONT_HEIGHT;
  #endif
         console_cols = panel_info.vl_col / VIDEO_FONT_WIDTH;
+       lcd_init_console(lcd_base, console_rows, console_cols);
         lcd_init_console(lcd_logo(), console_rows, console_cols);
         lcd_sync();
  }
---8<---

Quoted 2 lines

It first initializes the lcd console with LCD base, if the splash screen is used, new address is updated.

I think this is the best approach. I am very close to posting the next

step in my refactor of lcd.c, and I can
incorporate it in the series. I will greatly appreciate your help in testing this series, since it involves Atmel
related changes.

Quoted 4 lines

I need this kind of fix to be applied as soon as possible, or else, most Atmel related board are broken on u-boot master branch.

Quoted 2 lines

Best Regards,
Bo Shen

--
Regards,
Nikita Kiryanov