[coreboot] [PATCH][cbv2]Add serial port information to lbtable

[Uwe Hermann]

Looks good, but a few comments:

On Wed, Jan 16, 2008 at 12:34:14PM +0100, Patrick Georgi wrote:
> the attached patch makes cbv2 add information about the serial port to
> lbtable. For this, a new record type is created. Payloads can then parse
> lbtable to figure out where to find the serial port (which is usually
> used by lb as console already).
> 
> A patch for grub2 exists, too, and I'll implement a compatible change
> for cbv3 once the record format (and overall design etc) is stable.

Have you checked lxbios continues to work after the changes?


> Index: src/arch/i386/boot/linuxbios_table.c
> ===================================================================
> --- src/arch/i386/boot/linuxbios_table.c	(Revision 3049)
> +++ src/arch/i386/boot/linuxbios_table.c	(Arbeitskopie)
> @@ -74,6 +74,18 @@
>  	return mem;
>  }
>  
> +struct lb_serial *lb_serial(struct lb_header *header)
> +{
> +	struct lb_record *rec;
> +	struct lb_serial *serial;
> +	rec = lb_new_record(header);
> +	serial = (struct lb_serial *)rec;
> +	serial->tag = LB_TAG_SERIAL;
> +	serial->size = sizeof(*serial);

> +	serial->ioport = TTYS0_BASE;

This doesn't look generic enough(?) Where does TTYS0_BASE come from?
Does the code expect it to be defined in auto.c or cache_as_ram_auto.c?

I think not all of the boards define it, and even if they did,
TTYS0_BASE implies COM1, which is not always what you want.
We need a mechanism to allow other ports here (COM2, COM3, COM4).

Yes, I think it's just a name, in theory you could assign any value
to TTYS0_BASE, but you get the point.


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