From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bo Shen <voice.shen@atmel.com>
Date: Mon, 26 Jan 2015 13:55:39 +0800
Subject: [U-Boot] [PATCH] lcd: fix console address is not initialized
In-Reply-To: <54C1A1E7.4090208@atmel.com>
References: <1421815024-24771-1-git-send-email-voice.shen@atmel.com>	<54C0F6B4.1000208@compulab.co.il>
	<54C1A1E7.4090208@atmel.com>
Message-ID: <54C5D6DB.8080000@atmel.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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