libpayload: Geode video console support

[Jordan Crouse]

As just demonstrated in Denver, support for mulitple video console
backends in libpayload, as well as Geode support for said video console
infrastructure.

Jordan

[Uwe Hermann]

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.

[Jordan Crouse]

On 10/04/08 02:48 +0200, Uwe Hermann wrote:
> 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.

This will need to be fixed in the rest of Config.in too.  Please send a patch.

Jordan

[Jordan Crouse]

On 10/04/08 02:48 +0200, Uwe Hermann wrote:
> >  /* Clear the screen and kill the cursor */
>
> Should end with full stop.

We need to have a serious discussion about this.  I realize that
you prefer a period after all statements - but many of us don't.
Thats okay - the style guide doesn't care one way or the other.
You shouldn't point them out, and especially not as a reason to
reject a patch (I know that wasn't your intent for this patch,
since there were other flaws, but still - it was almost the first
thing you mentioned).

Its okay to be strict about coding style - there is nothing worse
then a poorly styled project.  But its another thing to be too
aggressive, especially with personal quirks that half the developers
will do one way and the other half will do the other way.

Don't get me wrong, I really appreciate your comments, but it can be
frustrating when you send out a patch of somewhat complex code and
all you get back are comments about the coding style.  Did you not
have any concerns at all about how the code worked?

All of us have to be better about critically analyzing the code that
goes by us.  Ron and Stefan found a bug in the PCI system that has been
there for nearly 8 years, and had been ported forward into v3.  Yes,
bugs can get through, but we need to be as diligent as our Acked-By lines
say we are.  Well formatted source code is nothing if it doesn't work.
> > 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?

Its an internal recursive function used by pci_find_device.  pci_find_device
is the real useful function, and it is part of the external API.
> > 	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.

Okay - we can debate this.  The value of unsigned int and unsigned char
are guaranteed, and my editor nicely highlights them for me, but if we
prefer to use typedefs, then thats fine.
> > (http://www.helenos.eu)
>
> I'd drop this, and put in into LICENSING (same for other external files).

I'm worried about this - I'm sure people will do to us what we did to
HelenOS, i.e. borrow code. I think we need to make sure the entire legacy
of the code is in each of the files so that if somebody comes across it
on Google code then the chain of ownership is maintained.
> > +#define FONT_8X16_H_
>
> We should decide for an include guard format and use it consistently
> everywhere. I propose FONT_8X16_H, as per v3 conventions (i.e. no
> prefix/postfix underscores).

I prefer the kernel format of _NAME_H - though as you can see, I use
both prefix and postfix underscores sort of interchangeably.  I would
be happy to standardize in a later patch, but I think we need either
the prefix or the postfix to avoid potentially contaminating the
namespace.
> > +#ifdef HAVE_CONSOLE_FONT
> > +#error "You have already defined a console font!"
>
> Shouldn't kconfig take care that invalid configs can happen?

We'll see - for now, the developer can chose which fontset they
want to use (even though we only have one).  I'm not sure if we
are going to eventually get to the point where the builder would
chose a fontset through the Kconfig.  Lets just leave this for now.
> > +#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.

I would rather keep them separate.  libpayload.h is the main external API
for the library.  PCI in particular defines static functions and other
bits that nearly all payload writers will not care about.  Rather then
have this implicitly brought in by libpayload.h, I would prefer that
the developer be asked to explicitly include it.
> > +#include <video_console.h>
> > +#include <arch/io.h>
> > +#include <arch/msr.h>
> > +#include "font8x16.h"
>
> Drop? Should all be in libpayload.h.

Again, same problem.  I might be okay with <arch/io.h>, though I don't
really buy into the idea of automatically including architecture specific
code.  <arch/msr.h> is only x86 specific, and so of course should not be
included automatically.

Like I said, libpayload.h is the external API for the library. 
video_console.h and the font8x16.h are internal header files, they would
never be used by the payload writer, so they should remain separate.
> > +/* 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.

They could be, but why?
> > if (!pci_find_device(0x1022, 0x2081, &dev))
>
> Maybe add #defines for these in pci.h?
> No need for an extra pci_id.h though...

Nah - we'll only use these values once.  I can define them locally
in the file, but pci.h doesn't need to know.
> > +#include <libpayload.h>
> > +#include <video_console.h>
>
> Merge in libpayload.h.

See above.
> > +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.

Yes.  We need to remove all inlines.
> 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.

I can realistically divide it into two patches, one for the vga / 
video console infrastructure, and one for Geode.  I will do that.

Jordan

[Uwe Hermann]

On Thu, Apr 10, 2008 at 09:51:07AM -0600, Jordan Crouse wrote:
> > > (http://www.helenos.eu)
> >
> > I'd drop this, and put in into LICENSING (same for other external files).
>
> I'm worried about this - I'm sure people will do to us what we did to
> HelenOS, i.e. borrow code. I think we need to make sure the entire legacy
> of the code is in each of the files so that if somebody comes across it
> on Google code then the chain of ownership is maintained.

Full ACK, I only meant to move the URL into LICENSING. The sentence
'It has originally been taken from the HelenOS project.' should stay,
of course. We also say "taken from OpenBSD" without mentioning the URL
every time, LICENSING already has the exact URL to the svn file etc.
> > > +#define FONT_8X16_H_
> >
> > We should decide for an include guard format and use it consistently
> > everywhere. I propose FONT_8X16_H, as per v3 conventions (i.e. no
> > prefix/postfix underscores).
>
> I prefer the kernel format of _NAME_H - though as you can see, I use
> both prefix and postfix underscores sort of interchangeably.  I would
> be happy to standardize in a later patch, but I think we need either
> the prefix or the postfix to avoid potentially contaminating the
> namespace.

Ok, let's use _FOO_H consistently then.
> I would rather keep them separate.  libpayload.h is the main external API
> for the library.  PCI in particular defines static functions and other
> bits that nearly all payload writers will not care about.  Rather then
> have this implicitly brought in by libpayload.h, I would prefer that
> the developer be asked to explicitly include it.

OK, if it has implications for code size let's keep pci.h extra.
> > > +static unsigned long dcaddr;
> > > +static unsigned long vgaddr;
> > > +static unsigned long gpaddr;
> > > +static unsigned long fbaddr;
> >
> > Could all be in one line.
>
> They could be, but why?

Just cosmetics, one line vs. four. Nevermind, either is fine.
> > > if (!pci_find_device(0x1022, 0x2081, &dev))
> >
> > Maybe add #defines for these in pci.h?
> > No need for an extra pci_id.h though...
>
> Nah - we'll only use these values once.  I can define them locally
> in the file, but pci.h doesn't need to know.

OK, having them locally is fine too.
> > Is the inline needed or useful? I guess gcc can figure that out by
> > itself.
>
> Yes.  We need to remove all inlines.

OK.

Uwe.

[Jordan Crouse]

> 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.

I split them into three patches - one adding the video console, the
other adding the support functions, and the third adding the Geode 
driver.  They are sent in reply to this email.

Jordan

[1/3] libpayload: Add video console framework

[Jordan Crouse]

Video console framework + VGA driver.

Jordan

[1/3] libpayload: Add video console framework

[Uwe Hermann]

On Thu, Apr 10, 2008 at 10:20:17AM -0600, Jordan Crouse wrote:
> libpayload:  Add video console framework
>
> Add a framework for multiple video console drivers.  This is to prepare
> for the Geode driver.
>
> Signed-off-by: Jordan Crouse

Acked-by: Uwe Hermann

(but see below)
> Index: libpayload/drivers/video/vga.c
>
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ libpayload/drivers/video/vga.c	2008-04-10 08:58:26.000000000 -0600
> @@ -0,0 +1,141 @@

[...]
> +struct video_console_t vga_video_console = {

This must be 'video_console' without '_t', otherwise it won't compile.

Uwe.

libpayload: Add video console framework

[Jordan Crouse]

On 11/04/08 00:42 +0200, Uwe Hermann wrote:
> On Thu, Apr 10, 2008 at 10:20:17AM -0600, Jordan Crouse wrote:
> > libpayload:  Add video console framework
> >
> > Add a framework for multiple video console drivers.  This is to prepare
> > for the Geode driver.
> >
> > Signed-off-by: Jordan Crouse
>
> Acked-by: Uwe Hermann

Fixed and r3230.  Thanks.
> (but see below)
> > Index: libpayload/drivers/video/vga.c
> >
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ libpayload/drivers/video/vga.c	2008-04-10 08:58:26.000000000 -0600
> > @@ -0,0 +1,141 @@
>
> [...]
> > +struct video_console_t vga_video_console = {
>
> This must be 'video_console' without '_t', otherwise it won't compile.
> Uwe.
>
> http://www.hermann-uwe.de  | http://www.holsham-traders.de
> http://www.crazy-hacks.org | http://www.unmaintained-free-software.org

libpayload: Support functions for Geode

[Jordan Crouse]

PCI, MSR and udelay().

Jordan

libpayload: Support functions for Geode

[Uwe Hermann]

On Thu, Apr 10, 2008 at 10:21:04AM -0600, Jordan Crouse wrote:
> libpayload: Support functions for Geode
>
> The Geode video driver will require a number of support functions,
> including udelay(), PCI bus walking and MSRs.  This adds those functions
> in preparation for the actual code.
>
> Signed-off-by: Jordan Crouse

Acked-by: Uwe Hermann

Uwe.

libpayload: Support functions for Geode

[Jordan Crouse]

On 11/04/08 00:46 +0200, Uwe Hermann wrote:
> On Thu, Apr 10, 2008 at 10:21:04AM -0600, Jordan Crouse wrote:
> > libpayload: Support functions for Geode
> >
> > The Geode video driver will require a number of support functions,
> > including udelay(), PCI bus walking and MSRs.  This adds those functions
> > in preparation for the actual code.
> >
> > Signed-off-by: Jordan Crouse
>
> Acked-by: Uwe Hermann

r3231.  Thanks.
> Uwe.
>
> http://www.hermann-uwe.de  | http://www.holsham-traders.de
> http://www.crazy-hacks.org | http://www.unmaintained-free-software.org

libpayload: Support functions for Geode

[Jordan Crouse]

On 11/04/08 00:46 +0200, Uwe Hermann wrote:
> On Thu, Apr 10, 2008 at 10:21:04AM -0600, Jordan Crouse wrote:
> > libpayload: Support functions for Geode
> >
> > The Geode video driver will require a number of support functions,
> > including udelay(), PCI bus walking and MSRs.  This adds those functions
> > in preparation for the actual code.
> >
> > Signed-off-by: Jordan Crouse
>
> Acked-by: Uwe Hermann

r3233 - thanks.
> Uwe.
>
> http://www.hermann-uwe.de  | http://www.holsham-traders.de
> http://www.crazy-hacks.org | http://www.unmaintained-free-software.org

[3/3] libpayload: Add a Geode video driver

[Jordan Crouse]

The actual Geode driver.

Jordan

[3/3] libpayload: Add a Geode video driver

[Carl-Daniel Hailfinger]

On 10.04.2008 18:22, Jordan Crouse wrote:
> The actual Geode driver.
>
> Jordan

Does that mean we can have invaders as a payload on real GeodeLX
hardware? That would make a great demo for LinuxTag!

Regards,
Carl-Daniel

libpayload: Add a Geode video driver

[Jordan Crouse]

On 10/04/08 18:24 +0200, Carl-Daniel Hailfinger wrote:
> On 10.04.2008 18:22, Jordan Crouse wrote:
> > The actual Geode driver.
> >
> > Jordan
>
> Does that mean we can have invaders as a payload on real GeodeLX
> hardware? That would make a great demo for LinuxTag!

Yep - assuming you get your board before then, I'll work with you
to get it set up and working.

Hopefully soon we'll have both invaders (and tint) in buildrom, so it
should be an easy build.

Jordan

[3/3] libpayload: Add a Geode video driver

[Uwe Hermann]

On Thu, Apr 10, 2008 at 10:22:09AM -0600, Jordan Crouse wrote:
> libpayload:  Add a Geode video driver
>
> Add a Geode video driver in lieu of VGA on Geode LX devices
>
> Signed-off-by: Jordan Crouse

Acked-by: Uwe Hermann

Uwe.