[coreboot] libpayload: Geode video console support

Jordan Crouse jordan.crouse at amd.com
Thu Apr 10 17:51:07 CEST 2008


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

-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.





More information about the coreboot mailing list