[U-Boot] [PATCH] lcd: fix console address is not initialized

[Bo Shen]

Hi Nikita Kiryanov,

On 01/22/2015 09:10 PM, Nikita Kiryanov wrote:
> Hi Bo,
>
> On 01/21/2015 06:37 AM, Bo Shen wrote:
>> 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.

>>
>> This patch split lcd console address initialize and lcd logo
>> display into two functions.
>>
>> Signed-off-by: Bo Shen <voice.shen at atmel.com>
>> ---
>>
>>   common/lcd.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/lcd.c b/common/lcd.c
>> index cc34b8a..f435e2a 100644
>> --- a/common/lcd.c
>> +++ b/common/lcd.c
>> @@ -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);
>>   #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.
>

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

Best Regards,
Bo Shen