[coreboot] [PATCH] [7/7] Roda RK886EX support: mainboard and build target

[Peter Stuge]

Stefan Reinauer wrote:
> +++ src/mainboard/roda/rk886ex/Kconfig	(revision 0)
> @@ -0,0 +1,62 @@
> +config BOARD_RODA_RK886EX
> +	bool "RK886EX"
> +	select ARCH_X86
> +	select CPU_INTEL_CORE
> +	select CPU_INTEL_SOCKET_MFCPGA478
> +	select NORTHBRIDGE_INTEL_I945
> +	select SOUTHBRIDGE_INTEL_I82801GX
> +	select SUPERIO_WINBOND_W83627THG

Shouldn't this also select the EC with SUPERIO_RENESAS_.. ?


> +config MAINBOARD_DIR
> +	string
> +	default kontron/986lcd-m
> +	depends on BOARD_RODA_RK886EX

Hmm.


> +	/* Pack GNVS into the ACPI table area */
> +	for (i=0; i < dsdt->length; i++) {
> +		if (*(u32*)(((u32)dsdt) + i) == 0xC0DEBABE) {

:)


> +	printk_info("ACPI: done.\n");

For another day it would be nice to somehow factor out most of this
ACPI stuff into a generic acpi_write_tables().



> +++ src/mainboard/roda/rk886ex/auto.c	(revision 0)
..
> +static void init_artec_dongle(void)
> +{
> +	// Enable 4MB decoding
> +	outb(0xf1, 0x88);
> +	outb(0xf4, 0x88);
> +}

Could this go in lib/ or something? It's useful for all boards after
all..


> +#include <cbmem.h>
> +
> +// Now, this needs to be included because it relies on the symbol
> +// __PRE_RAM__ being set during CAR stage (in order to compile the 
> +// BSS free versions of the functions). Either rewrite the code
> +// to be always BSS free, or invent a flag that's better suited than
> +// __PRE_RAM__ to determine whether we're in ram init stage (stage 1)
> +//
> +#include "lib/cbmem.c"

Huh?


> +void real_main(unsigned long bist)
> +{
..
> +	/* This has to happen after i945_early_initialization() */
> +	init_artec_dongle();

Should this call be there?


> +++ src/mainboard/roda/rk886ex/rtl8168.c	(revision 0)
..
> +/* This code should work for all ICH* southbridges with a NIC. */

Better to have it in southbridge/ than mainboard/ then?


> +static void nic_init(struct device *dev)
> +{
> +	printk_debug("Initializing RTL8168 Gigabit Ethernet\n");
> +	// Nothing to do yet, but this has to be here to keep 
> +	// coreboot from trying to execute an option ROM.
> +}

Ouuch.. Really? That's something I would like to set in devicetree.cb
instead.


In general though, it is

Acked-by: Peter Stuge <peter at stuge.se>