[coreboot] libpayload: Geode video console support

Uwe Hermann uwe at hermann-uwe.de
Thu Apr 10 02:48:07 CEST 2008


On Fri, Apr 04, 2008 at 04:03:40PM -0600, Jordan Crouse wrote:
> +config VGA_VIDEO_CONSOLE
> +	bool "VGA video console driver"
> +	depends VIDEO_CONSOLE

Please use "depends on". This may no be required yet, but newer versions
of kconfig killed 'depends' and only allow 'depends on', I think.


> Index: libpayload/curses/tinycurses.c
> ===================================================================
> --- libpayload.orig/curses/tinycurses.c	2008-04-01 16:32:52.000000000 -0600
> +++ libpayload/curses/tinycurses.c	2008-04-01 17:11:41.000000000 -0600
> @@ -219,9 +219,11 @@
>  	// def_prog_mode();
>  
>  	if (curses_flags & F_ENABLE_CONSOLE) {
> -		/* Clear the screen and kill the cursor. */
> -		vga_clear();
> -		vga_cursor_enable(0);
> +
> +	  /* Clear the screen and kill the cursor */

Should end with full stop.


> +	  video_console_clear();
> +	  video_console_cursor_enable(0);

Indentation is broken.


> Index: libpayload/drivers/Makefile.inc
[...]
> +TARGETS-$(CONFIG_VIDEO_CONSOLE) += drivers/video/video.o
> +TARGETS-$(CONFIG_VGA_VIDEO_CONSOLE) += drivers/video/vga.o

> +TARGETS-$(CONFIG_GEODE_VIDEO_CONSOLE) += drivers/video/geode.o drivers/video/font8x16.o

Longer than 80 chars per line.


> Index: libpayload/drivers/pci.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ libpayload/drivers/pci.c	2008-04-01 17:11:41.000000000 -0600
> @@ -0,0 +1,90 @@
[...]
> +#include <libpayload.h>
> +#include <pci.h>
> +
> +static int find_on_bus(int bus, unsigned short vid, unsigned short did,

Why static? Might be useful elsewhere?


> +			pcidev_t *dev)
> +{
> +	int devfn;
> +	unsigned int val;
> +	unsigned char hdr;

I'd prefer to change all of these to u16 et. al., but not required for
the patch to go in, this can be done later.


> +	for(devfn = 0; devfn < 0x100; devfn++) {
> +		pci_read_dword(bus, devfn, REG_VENDOR_ID, &val);
> +
> +		if (val == 0xffffffff || val == 0x00000000 ||
> +                    val == 0x0000ffff || val == 0xffff0000)
> +			continue;
> +
> +		if (val == ((did << 16) | vid)) {
> +			*dev = PCIDEV(bus, devfn);							return 1;
> +		}

Run this file through indent please, various parts have broken coding style.


> Index: libpayload/drivers/video/font8x16.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ libpayload/drivers/video/font8x16.c	2008-04-04 15:53:02.000000000 -0600
> @@ -0,0 +1,4646 @@
> +/*
> + * This file is part of libpayload

Missing full stop.


> + *
> + * It has originally been taken from the HelenOS project

Ditto.


> + * (http://www.helenos.eu)

I'd drop this, and put in into LICENSING (same for other external files).


> Index: libpayload/drivers/video/font8x16.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ libpayload/drivers/video/font8x16.h	2008-04-01 17:11:41.000000000 -0600
> @@ -0,0 +1,45 @@
[...]
> +#ifndef	FONT_8X16_H_

s/TAB/space/ here.


> +#define FONT_8X16_H_

We should decide for an include guard format and use it consistently
everwhere. I propose FONT_8X16_H, as per v3 conventions (i.e. no
prefix/postfix underscores).


> +
> +#ifdef HAVE_CONSOLE_FONT
> +#error "You have already defined a console font!"

Shouldn't kconfig take care that invalid configs can happen?


> Index: libpayload/drivers/video/geode.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ libpayload/drivers/video/geode.c	2008-04-01 17:11:41.000000000 -0600
[...]
> +#include <libpayload.h>
> +#include <pci.h>

I propose to #include <pci.h> in libpayload.h, there's not much use
in complicating things everywhere in the code. IMHO just including
libpayload.h only is perfectly fine.

I'd also add <curses.h> in libpayload.h so a payload writer doesn't
have to add it explicitly. The _only_ header a payload writer should
ever have to worry about is libpayload.h IMO.


> +#include <video_console.h>
> +#include <arch/io.h>
> +#include <arch/msr.h>
> +#include "font8x16.h"

Drop? Should all be in libpayload.h.


> +/* This is the video mode that we're going to use for our VGA screen */
> +
> +static const struct mode {

Drop "mode"? It's not used anywhere AFAICS.


> +/* Addresses for the various components */
> +
> +static unsigned long dcaddr;
> +static unsigned long vgaddr;
> +static unsigned long gpaddr;
> +static unsigned long fbaddr;

Could all be in one line.


> +static void geode_putc(uint8_t row, uint8_t col, unsigned int ch)

uint8_t -> u8. Let's only use one form consistently, the short one.


> +static int geode_init(void)
> +{
> +	pcidev_t dev;
> +	int i;
> +
> +	if (!pci_find_device(0x1022, 0x2081, &dev))

Maybe add #defines for these in pci.h?
No need for an extra pci_id.h though...


> +		return -1;
> +
> +	fbaddr = pci_read_resource(dev, 0);
> +	gpaddr = pci_read_resource(dev, 1);
> +	dcaddr = pci_read_resource(dev, 2);
> +	vgaddr = pci_read_resource(dev, 3);
> +
> +	init_video_mode();
> +
> +	/* Set up the palette */
> +
> +	for(i = 0; i < 16; i++) {

16 -> ARRAY_SIZE(vga_colors). Just for readability.


> +		geode_set_palette(i, vga_colors[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +struct video_console_t geode_video_console = {

video_console_t -> video_console, the _t makes it look like it's
a typedef, which it isn't (and shouldn't be).


> +	.init = geode_init,
> +	.putc = geode_putc,
> +	.clear = geode_clear,
> +	.scroll_up = geode_scroll_up
> +};
> Index: libpayload/drivers/video/vga.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ libpayload/drivers/video/vga.c	2008-04-01 17:11:41.000000000 -0600
> @@ -0,0 +1,141 @@
[...]
> +#include <arch/types.h>

Drop, we alread have <libpayload.h>.


> +#include <libpayload.h>

> +#include <video_console.h>

Merge in libpayload.h.


> +
> +#define VGA_COLOR_WHITE 7
> +
> +#define CRTC_INDEX      0x3d4
> +#define CRTC_DATA       0x3d5
> +
> +#define VIDEO(_r, _c)\
> +  ((uint16_t *) (0xB8000 + ((_r) * (VIDEO_COLS * 2)) + ((_c) * 2)))

u16


> +
> +static inline uint8_t crtc_read(uint8_t index)

u8

Is the inline needed or useful? I guess gcc can figure that out by
itself.


> Index: libpayload/i386/timer.c
> ===================================================================
> --- libpayload.orig/i386/timer.c	2008-04-01 16:32:53.000000000 -0600
> +++ libpayload/i386/timer.c	2008-04-01 17:11:41.000000000 -0600
> @@ -74,7 +74,12 @@
>  
>  void ndelay(unsigned int n)
>  {
> -	_delay(n * cpu_khz / 1000000);
> + 	_delay(n * cpu_khz / 1000000);

Whitespace breakage.


All in all the patch looks quite nice. Please repost, but splitted in
multiple patches. I'd say one which only moves files around and one
which only adds files (while keeping libpayload in a compiling state).
The final patch should do all other random modifications.


Cheers, Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org




More information about the coreboot mailing list