Patchwork [U-Boot] VGA text console support

login
register
mail settings
Submitter Vladimir 'φ-coder/phcoder' Serbinenko
Date April 7, 2013, 3:36 p.m.
Message ID <51619287.6050707@gmail.com>
Download mbox | patch
Permalink /patch/234509/
State Changes Requested
Delegated to: Simon Glass
Headers show

Comments

Vladimir 'φ-coder/phcoder' Serbinenko - April 7, 2013, 3:36 p.m.
Most coreboot users use just VGA text console, not graphics. Support it.
Remaining problem is that software and hardware cursor blinking superimpose.
I disabled software blinking locally but it's not part of this patch since
it's not done properly.
Wolfgang Denk - April 7, 2013, 5:04 p.m.
Dear Vladimir 'φ-coder/phcoder' Serbinenko,

I cannot comment on the actual technical content of your patches, but
here are a few formal comments; I highly recommend to study
http://www.denx.de/wiki/U-Boot/Patches

> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: quoted-printable

Please send plain text messages.  Do not send multipart/mixed
messages.

> Most coreboot users use just VGA text console, not graphics. Support it.
> Remaining problem is that software and hardware cursor blinking superimpose.
> I disabled software blinking locally but it's not part of this patch since
> it's not done properly.
>
> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> index 61e1058..0c3c26d 100644

Signed-off-by: line missing.

> +static int VIDEO_VISIBLE_COLS;
> +static int VIDEO_VISIBLE_ROWS;
> +static int VIDEO_PIXEL_SIZE;

Illegal variable names - these must always be lower case.

Also, you are meddling here with a global file.  Are you sure your
changes will have no bad effects to other use cases?

Best regards,

Wolfgang Denk
Simon Glass - April 16, 2013, 12:13 a.m.
+Anatolij

Hi Vladimir,

On Sun, Apr 7, 2013 at 8:36 AM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> Most coreboot users use just VGA text console, not graphics. Support it.
> Remaining problem is that software and hardware cursor blinking superimpose.
> I disabled software blinking locally but it's not part of this patch since
> it's not done properly.
>

Further to the checkpatch errors (try using patman!) see below:

> diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
> index 61e1058..0c3c26d 100644
> --- a/drivers/video/cfb_console.c
> +++ b/drivers/video/cfb_console.c
> @@ -180,11 +180,10 @@
>  /*
>   * some Macros
>   */
> -#define VIDEO_VISIBLE_COLS     (pGD->winSizeX)
> -#define VIDEO_VISIBLE_ROWS     (pGD->winSizeY)
> -#define VIDEO_PIXEL_SIZE       (pGD->gdfBytesPP)
> +static int VIDEO_VISIBLE_COLS;
> +static int VIDEO_VISIBLE_ROWS;
> +static int VIDEO_PIXEL_SIZE;
>  #define VIDEO_DATA_FORMAT      (pGD->gdfIndex)
> -#define VIDEO_FB_ADRS          (pGD->frameAdrs)
>
>  /*
>   * Console device defines with i8042 keyboard controller
> @@ -384,6 +383,7 @@ static int console_row;             /* cursor row */
>  static u32 eorx, fgx, bgx;     /* color pats */
>
>  static int cfb_do_flush_cache;
> +static int vga_text;
>
>  #ifdef CONFIG_CFB_CONSOLE_ANSI
>  static char ansi_buf[10];
> @@ -450,6 +450,18 @@ static void video_drawchars(int xx, int yy, unsigned char *s, int count)
>         u8 *cdat, *dest, *dest0;
>         int rows, offset, c;
>
> +       if (vga_text)

I wonder if 'if (!vga_graphics)' might be clearer? But this is OK with
me if you prefer it.

> +       {
> +               xx /= VIDEO_FONT_WIDTH;
> +               yy /= VIDEO_FONT_HEIGHT;
> +               while (count--)
> +               {
> +                       ((uint16_t *)video_fb_address) [yy * 80 + xx] = 0x700 | *s++;
> +                       xx++;
> +               }
> +               return;
> +       }
> +
>         offset = yy * VIDEO_LINE_LEN + xx * VIDEO_PIXEL_SIZE;
>         dest0 = video_fb_address + offset;
>
> @@ -644,6 +656,20 @@ static void video_invertchar(int xx, int yy)
>         }
>  }
>
> +static inline void
> +cr_write (uint8_t val, uint8_t addr)
> +{
> +  outb (addr, 0x3d4);
> +  outb (val, 0x3d5);
> +}
> +
> +static inline uint8_t
> +cr_read (uint8_t addr)
> +{
> +  outb (addr, 0x3d4);
> +  return inb (0x3d5);
> +}
> +
>  void console_cursor(int state)
>  {
>  #ifdef CONFIG_CONSOLE_TIME
> @@ -667,6 +693,22 @@ void console_cursor(int state)
>         }
>  #endif
>
> +       if (vga_text)
> +       {
> +               uint8_t old;
> +               unsigned int pos = console_row * 80 + console_col;
> +               old = cr_read (0x0a);
> +               if (state)
> +                       cr_write (old & ~0x20, 0x0a);
> +               else
> +                       cr_write (old | 0x20, 0x0a);
> +
> +               cr_write (pos >> 8, 0x0e);
> +               cr_write (pos & 0xFF, 0x0f);
> +
> +               return;
> +       }
> +
>         if (cursor_state != state) {
>                 if (cursor_state) {
>                         /* turn off the cursor */
> @@ -704,6 +746,18 @@ static void memcpyl(int *d, int *s, int c)
>
>  static void console_clear_line(int line, int begin, int end)
>  {
> +       if (vga_text)
> +       {
> +               begin /= VIDEO_FONT_WIDTH;
> +               end /= VIDEO_FONT_WIDTH;
> +               line /= VIDEO_FONT_HEIGHT;
> +               for (; begin < end; begin++)
> +               {
> +                       ((uint16_t *)video_fb_address) [line * 80 + begin] = 0x700 | ' ';

Well looking at the number of times you repeat this code below, it
seems like this should go in its own function.

> +               }
> +               return;
> +       }
> +
>  #ifdef VIDEO_HW_RECTFILL
>         video_hw_rectfill(VIDEO_PIXEL_SIZE,             /* bytes per pixel */
>                           VIDEO_FONT_WIDTH * begin,     /* dest pos x */
> @@ -742,6 +796,15 @@ static void console_clear_line(int line, int begin, int end)
>  static void console_scrollup(void)
>  {
>         /* copy up rows ignoring the first one */
> +       if (vga_text)
> +       {
> +               int i;
> +               memcpyl(video_fb_address, ((int32_t *)video_fb_address) + 80 / 2,
> +                       (80 * 24 * 2) >> 2);

This is a big ugly. I think you should cast the video_fb_address to
char if you need to, and use * 2 instead of / 2.

Also I don't understand the number of bytes you are copying. It seems
like it should be 80 * (24 - 1) * 2?

> +               for (i = 0; i < 80; i++)
> +                       ((uint16_t *)video_fb_address) [24 * 80 + i] = 0x700 | ' ';
> +               return;
> +       }
>
>  #ifdef VIDEO_HW_BITBLT
>         video_hw_bitblt(VIDEO_PIXEL_SIZE,       /* bytes per pixel */
> @@ -779,6 +842,15 @@ static void console_back(void)
>
>  static void console_clear(void)
>  {
> +       if (vga_text)
> +       {
> +               int i;
> +               for (i = 0; i < 80 * 25; i++)
> +                       ((uint16_t *)video_fb_address) [i] = 0x700 | ' ';

How about creating a uint16_t pointer as a local variable?

> +
> +               return;
> +       }
> +
>  #ifdef VIDEO_HW_RECTFILL
>         video_hw_rectfill(VIDEO_PIXEL_SIZE,     /* bytes per pixel */
>                           0,                    /* dest pos x */
> @@ -1446,6 +1518,9 @@ int video_display_bitmap(ulong bmp_image, int x, int y)
>
>         WATCHDOG_RESET();
>
> +       if (vga_text)
> +               return 0;
> +
>         if (!((bmp->header.signature[0] == 'B') &&
>               (bmp->header.signature[1] == 'M'))) {
>
> @@ -1846,6 +1921,9 @@ static void plot_logo_or_black(void *screen, int width, int x, int y, int black)
>         unsigned char *source;
>         unsigned char *dest;
>
> +       if (vga_text)
> +               return;
> +
>  #ifdef CONFIG_SPLASH_SCREEN_ALIGN
>         if (x == BMP_ALIGN_CENTER)
>                 x = max(0, (VIDEO_VISIBLE_COLS - VIDEO_LOGO_WIDTH) / 2);
> @@ -2113,9 +2191,24 @@ static int video_init(void)
>
>         pGD = video_hw_init();
>         if (pGD == NULL)
> -               return -1;

Please add a comment here about what pGD == NULL means.

> +       {
> +               vga_text = 1;
> +               video_fb_address = (void *) 0xb8000;
> +               cfb_do_flush_cache = 0;
> +               console_col = 0;
> +               console_row = 0;
> +               video_logo_height = 0;
> +               VIDEO_VISIBLE_ROWS = 25 * VIDEO_FONT_HEIGHT;
> +               VIDEO_VISIBLE_COLS = 80 * VIDEO_FONT_WIDTH;
> +               VIDEO_PIXEL_SIZE = 2;
> +               return 0;
> +       }
> +
> +       video_fb_address = (void *) pGD->frameAdrs;
> +       VIDEO_VISIBLE_ROWS = pGD->winSizeY;
> +       VIDEO_VISIBLE_COLS = pGD->winSizeX;
> +       VIDEO_PIXEL_SIZE = (pGD->gdfBytesPP);
>
> -       video_fb_address = (void *) VIDEO_FB_ADRS;
>  #ifdef CONFIG_VIDEO_HW_CURSOR
>         video_init_hw_cursor(VIDEO_FONT_WIDTH, VIDEO_FONT_HEIGHT);
>  #endif
> @@ -2295,6 +2388,15 @@ void video_clear(void)
>  {
>         if (!video_fb_address)
>                 return;
> +
> +       if (vga_text)
> +       {
> +               int i;
> +               for (i = 0; i < 80 * 25; i++)

Is the display always 80 * 25, or can it sometimes be different?

Call console_clear() here instead of repeating the code?

Suggest a #define/enum constant for these values.

> +                       ((uint16_t *)video_fb_address) [i] = 0x700 | ' ';
> +               return;
> +       }
> +
>  #ifdef VIDEO_HW_RECTFILL
>         video_hw_rectfill(VIDEO_PIXEL_SIZE,     /* bytes per pixel */
>                           0,                    /* dest pos x */
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

Regards,
Simon

Patch

diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
index 61e1058..0c3c26d 100644
--- a/drivers/video/cfb_console.c
+++ b/drivers/video/cfb_console.c
@@ -180,11 +180,10 @@ 
 /*
  * some Macros
  */
-#define VIDEO_VISIBLE_COLS	(pGD->winSizeX)
-#define VIDEO_VISIBLE_ROWS	(pGD->winSizeY)
-#define VIDEO_PIXEL_SIZE	(pGD->gdfBytesPP)
+static int VIDEO_VISIBLE_COLS;
+static int VIDEO_VISIBLE_ROWS;
+static int VIDEO_PIXEL_SIZE;
 #define VIDEO_DATA_FORMAT	(pGD->gdfIndex)
-#define VIDEO_FB_ADRS		(pGD->frameAdrs)
 
 /*
  * Console device defines with i8042 keyboard controller
@@ -384,6 +383,7 @@  static int console_row;		/* cursor row */
 static u32 eorx, fgx, bgx;	/* color pats */
 
 static int cfb_do_flush_cache;
+static int vga_text;
 
 #ifdef CONFIG_CFB_CONSOLE_ANSI
 static char ansi_buf[10];
@@ -450,6 +450,18 @@  static void video_drawchars(int xx, int yy, unsigned char *s, int count)
 	u8 *cdat, *dest, *dest0;
 	int rows, offset, c;
 
+	if (vga_text)
+	{
+		xx /= VIDEO_FONT_WIDTH;
+		yy /= VIDEO_FONT_HEIGHT;
+		while (count--)
+		{
+			((uint16_t *)video_fb_address) [yy * 80 + xx] = 0x700 | *s++;
+			xx++;
+		}
+		return;
+	}
+
 	offset = yy * VIDEO_LINE_LEN + xx * VIDEO_PIXEL_SIZE;
 	dest0 = video_fb_address + offset;
 
@@ -644,6 +656,20 @@  static void video_invertchar(int xx, int yy)
 	}
 }
 
+static inline void
+cr_write (uint8_t val, uint8_t addr)
+{
+  outb (addr, 0x3d4);
+  outb (val, 0x3d5);
+}
+
+static inline uint8_t
+cr_read (uint8_t addr)
+{
+  outb (addr, 0x3d4);
+  return inb (0x3d5);
+}
+
 void console_cursor(int state)
 {
 #ifdef CONFIG_CONSOLE_TIME
@@ -667,6 +693,22 @@  void console_cursor(int state)
 	}
 #endif
 
+	if (vga_text)
+	{
+		uint8_t old;
+		unsigned int pos = console_row * 80 + console_col;
+		old = cr_read (0x0a);
+		if (state)
+			cr_write (old & ~0x20, 0x0a);
+		else
+			cr_write (old | 0x20, 0x0a);
+
+		cr_write (pos >> 8, 0x0e);
+		cr_write (pos & 0xFF, 0x0f);
+
+		return;
+	}
+
 	if (cursor_state != state) {
 		if (cursor_state) {
 			/* turn off the cursor */
@@ -704,6 +746,18 @@  static void memcpyl(int *d, int *s, int c)
 
 static void console_clear_line(int line, int begin, int end)
 {
+	if (vga_text)
+	{
+		begin /= VIDEO_FONT_WIDTH;
+		end /= VIDEO_FONT_WIDTH;
+		line /= VIDEO_FONT_HEIGHT;
+		for (; begin < end; begin++)
+		{
+			((uint16_t *)video_fb_address) [line * 80 + begin] = 0x700 | ' ';
+		}
+		return;
+	}
+
 #ifdef VIDEO_HW_RECTFILL
 	video_hw_rectfill(VIDEO_PIXEL_SIZE,		/* bytes per pixel */
 			  VIDEO_FONT_WIDTH * begin,	/* dest pos x */
@@ -742,6 +796,15 @@  static void console_clear_line(int line, int begin, int end)
 static void console_scrollup(void)
 {
 	/* copy up rows ignoring the first one */
+	if (vga_text)
+	{
+		int i;
+		memcpyl(video_fb_address, ((int32_t *)video_fb_address) + 80 / 2,
+			(80 * 24 * 2) >> 2);
+		for (i = 0; i < 80; i++)
+			((uint16_t *)video_fb_address) [24 * 80 + i] = 0x700 | ' ';
+		return;
+	}
 
 #ifdef VIDEO_HW_BITBLT
 	video_hw_bitblt(VIDEO_PIXEL_SIZE,	/* bytes per pixel */
@@ -779,6 +842,15 @@  static void console_back(void)
 
 static void console_clear(void)
 {
+	if (vga_text)
+	{
+		int i;
+		for (i = 0; i < 80 * 25; i++)
+			((uint16_t *)video_fb_address) [i] = 0x700 | ' ';
+
+		return;
+	}
+
 #ifdef VIDEO_HW_RECTFILL
 	video_hw_rectfill(VIDEO_PIXEL_SIZE,	/* bytes per pixel */
 			  0,			/* dest pos x */
@@ -1446,6 +1518,9 @@  int video_display_bitmap(ulong bmp_image, int x, int y)
 
 	WATCHDOG_RESET();
 
+	if (vga_text)
+		return 0;
+
 	if (!((bmp->header.signature[0] == 'B') &&
 	      (bmp->header.signature[1] == 'M'))) {
 
@@ -1846,6 +1921,9 @@  static void plot_logo_or_black(void *screen, int width, int x, int y, int black)
 	unsigned char *source;
 	unsigned char *dest;
 
+	if (vga_text)
+		return;
+
 #ifdef CONFIG_SPLASH_SCREEN_ALIGN
 	if (x == BMP_ALIGN_CENTER)
 		x = max(0, (VIDEO_VISIBLE_COLS - VIDEO_LOGO_WIDTH) / 2);
@@ -2113,9 +2191,24 @@  static int video_init(void)
 
 	pGD = video_hw_init();
 	if (pGD == NULL)
-		return -1;
+	{
+		vga_text = 1;
+		video_fb_address = (void *) 0xb8000;
+		cfb_do_flush_cache = 0;
+		console_col = 0;
+		console_row = 0;
+		video_logo_height = 0;
+		VIDEO_VISIBLE_ROWS = 25 * VIDEO_FONT_HEIGHT;
+		VIDEO_VISIBLE_COLS = 80 * VIDEO_FONT_WIDTH;
+		VIDEO_PIXEL_SIZE = 2;
+		return 0;
+	}
+
+	video_fb_address = (void *) pGD->frameAdrs;
+	VIDEO_VISIBLE_ROWS = pGD->winSizeY;
+	VIDEO_VISIBLE_COLS = pGD->winSizeX;
+	VIDEO_PIXEL_SIZE = (pGD->gdfBytesPP);
 
-	video_fb_address = (void *) VIDEO_FB_ADRS;
 #ifdef CONFIG_VIDEO_HW_CURSOR
 	video_init_hw_cursor(VIDEO_FONT_WIDTH, VIDEO_FONT_HEIGHT);
 #endif
@@ -2295,6 +2388,15 @@  void video_clear(void)
 {
 	if (!video_fb_address)
 		return;
+
+	if (vga_text)
+	{
+		int i;
+		for (i = 0; i < 80 * 25; i++)
+			((uint16_t *)video_fb_address) [i] = 0x700 | ' ';
+		return;
+	}
+
 #ifdef VIDEO_HW_RECTFILL
 	video_hw_rectfill(VIDEO_PIXEL_SIZE,	/* bytes per pixel */
 			  0,			/* dest pos x */