[coreboot] Coreboot patches for v2 with SeaBIOS

[Kevin O'Connor]

On Wed, Jan 21, 2009 at 12:08:57AM +0100, Stefan Reinauer wrote:
> Here's my latest patch. It contains generic parts, northbridge specific
> parts and board specific parts.
> 
> Please let me know what you think.

Thanks Stefan,

A couple of comments:

> --- src/mainboard/kontron/986lcd-m/mainboard.c	(revision 3886)
> +++ src/mainboard/kontron/986lcd-m/mainboard.c	(working copy)
> @@ -21,8 +21,31 @@
>  
>  
>  #include <device/device.h>
> +#include <console/console.h>
> +#include <boot/tables.h>
>  #include "chip.h"
>  
> +/* in arch/i386/boot/tables.c */
> +extern uint64_t high_tables_base, high_tables_size;
> +
> +/* in northbridge/intel/i945/northbridge.c */
> +extern uint64_t uma_memory_base, uma_memory_size;
> +
> +int add_mainboard_resources(struct lb_memory *mem)
> +{
> +#if HAVE_HIGH_TABLES == 1
> +	printk_debug("Adding high table area\n");
> +	lb_add_memory_range(mem, LB_MEM_TABLE,
> +		high_tables_base, high_tables_size);
> +#endif

Isn't there a way we can add this support without having to change
every platform?

> --- src/arch/i386/boot/tables.c	(revision 3886)
> +++ src/arch/i386/boot/tables.c	(working copy)
[...]
>  	/* Write ACPI tables */
>  	/* write them in the rom area because DSDT can be large (8K on epia-m) which
>  	 * pushes coreboot table out of first 4K if set up in low table area 
>  	 */
> +#if HAVE_LOW_TABLES == 1
>  	rom_table_end = write_acpi_tables(rom_table_end);
>  	rom_table_end = (rom_table_end+1023) & ~1023;
> -
> +#endif
> +#if HAVE_HIGH_TABLES == 1
> +	if (high_tables_base) {
> +		high_table_end = write_acpi_tables(high_table_end);
> +		high_table_end = (high_table_end+1023) & ~1023;
> +	}
> +#endif

I'm a little concerned about generating the data twice.  That could
confuse debugging efforts (the OS might see one version while the user
investigates another).  The data wont be identical because acpi has
pointers in the data structures.

I like the idea of always keeping support for tables in 0xf0000 -
maybe we could just copy the 36 byte ACPI RSDP struct to low memory
when HIGH_TABLES are enabled.  (And similarly, the 12 byte "mptable
floating structure".)

>         /* The smp table must be in 0-1K, 639K-640K, or 960K-1M */
> +#if HAVE_LOW_TABLES == 1
>         new_low_table_end = write_smp_table(low_table_end); // low_table_end is 0x10 at this point
> +#endif
> +#if HAVE_HIGH_TABLES == 1
> +       if (high_tables_base) {
> +               high_table_end = write_smp_table(high_table_end);
> +               high_table_end = (high_table_end+1023) & ~1023;
> +       }
> +#endif
> 
> -#if HAVE_MP_TABLE==1
> +#if HAVE_MP_TABLE == 1
>          /* Don't write anything in the traditional x86 BIOS data segment,
>           * for example the linux kernel smp need to use 0x467 to pass reset vector
>           * or use 0x40e/0x413 for EBDA finding...

I think the above "#if HAVE_MP_TABLE" branch doesn't make any sense.
Currently, coreboot tries to put the mptable in the first 1KiB of ram.
If it can't fit, it will put it into the 0xf0000 segment.  This
complexity doesn't make any sense, because there is no reason why we
can't always put the mptable in the 0xf0000 segment.

That is, in the HAVE_LOW_TABLES case, we can use:

	rom_table_end = write_smp_table(rom_table_end);
	rom_table_end = (rom_table_end+1023) & ~1023;

and completely remove the HAVE_MP_TABLE case.

-Kevin