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

[Stefan Reinauer]

On 1/16/10 9:12 PM, Peter Stuge wrote:
> 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_.. ?
>   
Absolutely. You caught me ice cold using only "newconfig" with this board.

I fixed your findings and a few more in the commit. It compiles fine
with Kconfig builds now (and chooses the right board ;-)

>> +	/* Pack GNVS into the ACPI table area */
>> +	for (i=0; i < dsdt->length; i++) {
>> +		if (*(u32*)(((u32)dsdt) + i) == 0xC0DEBABE) {
>>     
> :)
>   
The alternative would be to create an SSDT during boot time with the
constants needed by the DSDT.
Not sure what's the way to go here.

>> +	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().
>   
Yes. The same applies for MP and PIRQ ... We should try to generalize a
lot of mainboard code during our "v4" development cycle.

> +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..
>   
I thought I was bold by putting the two lines in a separate function
already, but you are probably right.

>> +#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?
>   
Sounds like this was renamed automatically during __ROMCC__ -->
__PRE_RAM__ but I am not sure.

I think we still have "moving away from nasty c file in c file in c file
includes" somewhere on our TODO
which would address this, too.
>> +	/* This has to happen after i945_early_initialization() */
>> +	init_artec_dongle();
>>     
> Should this call be there?
>   

It certainly does not hurt, as there is no device on the LPC bus at 0x88
if the dongle is not connected. One could argue about a 2us delay this
is causing, but...

>> +++ 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?
>   
Or in device/ ... I'm not entirely sure. So far there has been an
rtl816x chip on every i945+ICH7 system I've seen, but I don't know about
i975 + ich7 and other combinations. Having it live in the mainboard
directory spared us mentioning the device in the device tree at all
(which is good because we don't have to know where it lives) but with
Kconfig things are a bit easier, so we could consider moving it.

>> +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.
>   
Then you'd have to explicitly describe where the device, which would
make it harder to reuse the code. Maybe it's time to carefully think
what information needs to be added to the device tree (also think pci
slots and interrupt routing information)

Stefan