From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikita Kiryanov Date: Tue, 27 Jan 2015 16:45:52 +0200 Subject: [U-Boot] [PATCH] lcd: fix console address is not initialized In-Reply-To: <54C5D6DB.8080000@atmel.com> References: <1421815024-24771-1-git-send-email-voice.shen@atmel.com> <54C0F6B4.1000208@compulab.co.il> <54C1A1E7.4090208@atmel.com> <54C5D6DB.8080000@atmel.com> Message-ID: <54C7A4A0.8070001@compulab.co.il> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>> --- >>>> >>>> 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