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.