[LinuxBIOS] VGA support for Geode GX1/CS5530

Uwe Hermann uwe at hermann-uwe.de
Fri Oct 5 23:50:00 CEST 2007


On Fri, Oct 05, 2007 at 10:43:37PM +0200, Juergen Beisert wrote:
> This patch will add support for the Geode GX1/CS5530 VGA feature. Its able
> to set up one of five screen resolutions (sorry no autodetection at runtime,
> resolution is selected at buildtime) and displays a graphic in the right
> bottom corner (splash screen).

Nice! So this is in-LinuxBIOS VGA support? I.e. without running any VGA
option ROM blob in an emulator?

I'll give it a try on my CS5530 box in a few days (no access right now).


> +#if CONFIG_GX1_VIDEO == 1
> +/*
> + * Some register descriptions that are no listed in cpu/amd/gx1def.h
> + */

Would it make sense to add them to cpu/amd/gx1def.h?
Probably not as they're not CPU related but VGA related?


> +/*
> + * what colour depth should be used as default (in bpp)
> + * Note: Currently no other value than 16 is supported
> + */
> +#define COLOUR_DEPTH 16

Maybe this should be a user-visible config-option in Confib.lb too?


> +/*
> + * Support for a few basic video modes
> + * Note: all modes only for CRT. The flatpanel feature is
> + * not supported here (due to the lack of hardware to test)
> + */
> +struct video_mode {
> +	int pixel_clock;		/*<< pixel clock in Hz */

What is the '/*<<' supposed to mean?


> +/* ModeLine "640x480" 31.5 640 664 704 832 480 489 491 520 -hsync -vsync */
> +static const struct video_mode mode_640x480 = {
> +	/*
> +	 * 640x480 @ 72Hz hsync: 37.9kHz
> +	 * VESA standard mode for classic 4:3 monitors
> +	 */
> +	.pixel_clock = 31500000,
> +	.pll_value = 0x33915801,
> +
> +	.visible_pixel = 640,
> +	.hsync_start = 664,
> +	.hsync_end = 704,	/* 1.27 us sync length */
> +	.line_length = 832,	/* 26.39us */
> +
> +	.visible_lines = 480,
> +	.vsync_start = 489,
> +	.vsync_end = 491,
> +	.picture_length = 520, /* 13.89ms */
> +
> +	.sync_pol = HSYNC_LOW_POL | VSYNC_LOW_POL

Add a trailing comma here please, it's convenient if stuff is added
later (you can easily forget about the comma).


> +/*
> + * Setup the pixel PLL in the companion chip
> + * base: register's base address
> + * pll_val: pll register value to be set
> + */

Please use Doxygen-style comments (not the same as the kerneldoc
format). In this case:

/**
 * Setup the pixel PLL in the companion chip.
 *
 * @param base Register's base address.
 * @param pll_val PLL register value to be set.
 */


> +static void cs5530_set_clock_frequency(void *io_base,unsigned long pll_val)
                                                        ^
                                           missing space

> +{
> +	unsigned long reg;

u64? u32? Not sure.

Please use the types with explicit width and signedness wherever it
possible and whereever it makes sense (registers etc).


> +/*
> + * Activate the current mode to be "visible" outside
> + * gx_base: GX register area
> + * mode: Data about the video mode to setup
> + */
> +static void cs5530_activate_video(void *io_base, const struct video_mode *mode)
> +{
> +	u32 val;
> +
> +	val = mode->sync_pol;
> +	val <<= 8;

Why not this?

  val = mode->sync_pol << 8;

Or maybe even

  u32 val = mode->sync_pol << 8;


> +	writel(val | 0x0020002F, io_base + CS5530_DISPLAY_CONFIG);
> +}
> +
> +/*
> + * This bitmap file must provide:
> + * int width: pixel count in one line
> + * int height: line count
> + * int colours: ount of used colour
> + * unsigned long colour_map[]: RGB 565 colours to be used
> + * unsigned char bitmap[]: index per pixel into colour_map[], width*height pixels
> + */
> +#include "bitmap.c"

Should be a bit more configurable later (specify filename in Config.lb
or so). In v3 this should probably be handled in Kconfig.


> +/*
> + * show a boot splash screen in the right lower corner of the screen
> + * swidth: screen width in pixel
> + * sheight: screen height in lines
> + * pitch: line pitch in bytes
> + * base: screen base address
> + *
> + * This routine assumes we are using a 16 bit colour depth!
> + */
> +static void show_boot_splash_16(u32 swidth,u32 sheight,u32 pitch,void *base)
> +{
> +	int word_count,i;
> +	unsigned short *adr;
> +	u32 xstart,ystart,x,y;
> +	/*
> +	 * fill the screen with the colour of the
> +	 * left top pixel in the graphic
> +	 */
> +	word_count = pitch*sheight;

A few whitespace issues here and in some other places in the code, please
fix that to comply with the coding guidelines.


> Index: LinuxBIOSv2/src/southbridge/amd/cs5530/bitmap.c
> ===================================================================
> --- /dev/null
> +++ LinuxBIOSv2/src/southbridge/amd/cs5530/bitmap.c
> @@ -0,0 +1,304 @@
> +/* do not edit
> +This is an image of size 51 x 60 with 234 colours */

How was this image generated? What's the source format?

Please also attach a license to it if you created it. If you didn't we
must get permission from the author, I guess (and/or state the license
which applies to it).


I can fix up a few of the cosmetic issues in the repository right away
if you want, or I'll wait for another patch. Please let me know...


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071005/10eb19a6/attachment.sig>


More information about the coreboot mailing list