Hi Uwe,
On Friday 05 October 2007 23:50, Uwe Hermann wrote:
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?
This "driver" really knows the hardware, so it does not need any help from any kind of SMM.
I'll give it a try on my CS5530 box in a few days (no access right now).
Visit my BCOM WINNET100 Tutorial, scroll to the buttom of this document and download the Board Support Package. Extract it, you will need some patches to use the VGA console feature within Linux. Also for the Linux kernel you will need a specific console driver for the GX1, as the generic driver expects some help from the SMM. See patches/linux-2.6.22/generic for the whole patch stack, apply it to a fresh 2.6.22 kernel and build it with the igel316-kernelconfig.target as its .config. Also if you like to run X on it, you will need a special driver. You can find it in the BSP in the local_src/xf86-video-geode_gx1 directory.
+#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?
To be discussed. As this CPU and the chipset is one silicon (Note: only VGA interface support is in the companion chip, but the graphic feature is part of the Geode device!) you can put both defines together.
+/*
- 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?
Makes no sense yet, as my routines do not support the 8 bit CLUT mode. And the Geode hardware only supports 16 and 8 bit mode. Nothing else.
+/*
- 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?
Doxygen style. I like it, but I was not sure if it will be accepted in LBv2. But it seems I forgot it to remove...
+/* 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).
IMHO: I hate this leading comma. But all right, I will add it for you.
+/*
- 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:
I'm up for it!
/**
- 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
Ohhh, the spaces, always the spaces...
+{
- 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).
Hmm, all right.
+/*
- 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;
Hmmm, mode->sync_pol is an signed int. So shift operations are a bad idea?
- 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.
Yeees. This bitmap is a gimmick only. I was sure everyone would reject it on this list.
+/*
- 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.
Ack.
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).
Gimmick only. This graphic file is autogenerated with a small program that converts xpm into C code. This one shows one of the icons from the linuxbios website (one of the chips with the penguin in it). So I have no clue what kind of license this file should have. The small converter tool is part of my BSP. Refer in local_src/xpm_converter for the source.
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...
I will resend the patch.
Juergen