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

[Nikita Kiryanov <nikita@compulab.co.il>]

Hi Bo,

On 01/26/2015 07:55 AM, Bo Shen wrote:
> Hi Nikita Kiryanov,
>   + Andreas, Tom
>
> On 01/23/2015 09:20 AM, Bo Shen wrote:
>> 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@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.
>
> Can we use the following patch to fix this issue?
> --->8---
> diff --git a/common/lcd.c b/common/lcd.c
> index cc34b8a..1195a54 100644
> --- a/common/lcd.c
> +++ b/common/lcd.c
> @@ -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 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.

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

-- 
Regards,
Nikita Kiryanov