Re: [coreboot] mainboard.c: init() still being called?

[Jens Rottmann]

Hi,

sorry for the delay, I'm continually jumping between projects and especially if
a customer wants something I have to immediately drop everything else.

I wrote:
> > [...] separate the configuration _data_ from the _code_ that writes it into
> > the chip.  [...]  I had guessed that this is the purpose why there
> > is a struct mainboard_config in the mainboard's chip.h, just as there is a
> > struct superio_..._config in all superio's chip.h.

Myles wrote:
> The device tree isn't for options. It's for immutable properties of the
> mainboard.

I'm not sure why things like 'register "com1_address" = "0x3E8"', which is in
devicetree.cb, and which is changable in most standard BIOSes' Setup, are
considered more immutable than an option switching RS232 to RS485 (via some
mainboard logic).

In this sense I experimented doing this:

chip mainboard/CONFIG_MAINBOARD_DIR
    # Bit0 turns off Live LED, bit1 switches Com1 to RS485, bit2 same for Com2
    register "sio_gp1x_config" = "0x01"
    chip northbridge/amd/lx
        device pci_domain 0 on
        ...

i.e. make the mainboard the root device of the device tree - seemed plausible to
me, somehow. This compiles and boots fine, but the 'register' statement still
doesn't have any effect, sio_gp1x_config in the mainboard's chip.h still isn't
set - hmm, tough luck ... Why have a struct mainboard_config if it can't be
used? Maybe it should be removed completely??
> I think you need Kconfig variables for both of those things.

Ok, then I'll take this approach instead - and tripped over something else:

The vendor's Kconfig looks like this:
choice
    prompt "Mainboard model"
    source board/1
    ...
endchoice

This means you can't put any (visible) options in the board's Kconfig, because
they'd all end up _inside_ the choice instead of as seperate options. So you'd
have to put them all in the vendor Kconfig - but this would soon become very
ugly with all the "depends on"s once the list of boards and options grows. And
board specific options just belong in the board specific Kconfig, not in the
vendor's.

So I did this:
choice
    prompt "Mainboard model"
    config BOARD_1
    	bool "Board 1"
endchoice
source board/1

This made the board Kconfig:
config BOARD_1
    bool "Board 1"
    select FOO

lose the BOARD_1 option. But I needed something to attach the selects to:
config BOARD_SPECIFIC_OPTIONS # dummy
    def_bool y
    select FOO

(Dependencies skipped for simplicity, see attached full patch.) This works fine,
but I don't like the dummy config option BOARD_SPECIFIC_OPTIONS, which is always
y and doesn't actually configure anything.

- Does anyone have a better idea, avoiding the dummy option?
- I'm going to use the same scheme for the RoadRunner-LX and also for the 2
additional boards I'm planning to base on this. Maybe this restructuring should
be applied to all boards/vendors?? I've seen there is one other board with a
private config option, which is so far kept in the vendor's Kconfig.

Any comments are most welcome.

Thanks and cheers,
Jens

sio_gp1x_config can no longer be set from devicetree.cb, so convert this into a
Kconfig option instead. And while I'm at it, also add option for SMC_CONFIG so
user doesn't have to mess around in C files.

Signed-off-by: Jens Rottmann 
---

--- src/mainboard/lippert/Kconfig	(rev 5739)
+++ src/mainboard/lippert/Kconfig	(working copy)
@@ -1,10 +1,16 @@
+if VENDOR_LIPPERT
+
 choice
    prompt "Mainboard model"
-	depends on VENDOR_LIPPERT

source "src/mainboard/lippert/frontrunner/Kconfig"
 source "src/mainboard/lippert/roadrunner-lx/Kconfig"
-source "src/mainboard/lippert/spacerunner-lx/Kconfig"
+
+config BOARD_LIPPERT_SPACERUNNER_LX
+	bool "Cool SpaceRunner-LX"

endchoice

+source "src/mainboard/lippert/spacerunner-lx/Kconfig"
+
+endif # VENDOR_LIPPERT
--- src/mainboard/lippert/spacerunner-lx/Kconfig	(rev 5739)
+++ src/mainboard/lippert/spacerunner-lx/Kconfig	(working copy)
@@ -1,5 +1,7 @@
-config BOARD_LIPPERT_SPACERUNNER_LX
-	bool "Cool SpaceRunner-LX"
+if BOARD_LIPPERT_SPACERUNNER_LX
+
+config BOARD_SPECIFIC_OPTIONS # dummy
+	def_bool y
    select ARCH_X86
    select CPU_AMD_LX
    select NORTHBRIDGE_AMD_LX
@@ -9,24 +11,37 @@
    select PIRQ_ROUTE
    select UDELAY_TSC
    select USE_DCACHE_RAM
+	# Board is equipped with a 1 MB SPI flash, however, due to limitations
+	# of the IT8712F Super I/O, only the top 512 KB are directly mapped.
    select BOARD_ROMSIZE_KB_512

config MAINBOARD_DIR
    string
    default lippert/spacerunner-lx
-	depends on BOARD_LIPPERT_SPACERUNNER_LX

config MAINBOARD_PART_NUMBER
    string
    default "Cool SpaceRunner-LX"
-	depends on BOARD_LIPPERT_SPACERUNNER_LX

config IRQ_SLOT_COUNT
    int
    default 7
-	depends on BOARD_LIPPERT_SPACERUNNER_LX

config RAMBASE
    hex
    default 0x4000
-	depends on BOARD_LIPPERT_SPACERUNNER_LX
+
+config ONBOARD_UARTS_RS485
+	bool "Switch on-board serial ports to RS485"
+	default n
+	help
+	  If selected, both on-board serial ports will operate in RS485 mode
+	  instead of RS232.
+
+config ONBOARD_IDE_SLAVE
+	bool "Make on-board SSD act as Slave"
+	default n
+	help
+	  If selected, the on-board SSD will act as IDE Slave instead of Master.
+
+endif # BOARD_LIPPERT_SPACERUNNER_LX
--- src/mainboard/lippert/spacerunner-lx/chip.h	(rev 5739)
+++ src/mainboard/lippert/spacerunner-lx/chip.h	(working copy)
@@ -20,11 +20,7 @@

/* Based on chip.h from AMD's DB800 mainboard. */

-#include <stdint.h>
-
 extern struct chip_operations mainboard_ops;

struct mainboard_config {
-	/* bit2 = RS485_EN2, bit1 = RS485_EN1, bit0 = Live LED */
-	u8 sio_gp1x_config;
 };
--- src/mainboard/lippert/spacerunner-lx/mainboard.c	(rev 5739)
+++ src/mainboard/lippert/spacerunner-lx/mainboard.c	(working copy)
@@ -29,6 +29,13 @@
 #include <device/pci_ids.h>
 #include "chip.h"

+/* Bit0 turns off Live LED, bit1 switches Com1 to RS485, bit2 same for Com2. */
+#if CONFIG_ONBOARD_UARTS_RS485
+	#define SIO_GP1X_CONFIG	0x07
+#else
+	#define SIO_GP1X_CONFIG	0x01
+#endif
+
 static const u16 ec_init_table[] = { /* hi=data, lo=index */
    0x1900,		/* Enable monitoring */
    0x3050,		/* VIN4,5 enabled */
@@ -41,7 +48,6 @@

static void init(struct device *dev)
 {
-	struct mainboard_config *mb = dev->chip_info;
    unsigned int gpio_base, i;
    printk(BIOS_DEBUG, "LiPPERT SpaceRunner-LX ENTER %s\n", __func__);

@@ -65,7 +71,9 @@
    	outb(val >> 8, 0x0296);
    }

-	outb(mb->sio_gp1x_config, 0x1220); /* Simple-I/O GP17-10 */
+	/* bit2 = RS485_EN2, bit1 = RS485_EN1, bit0 = Live LED */
+	outb(SIO_GP1X_CONFIG, 0x1220); /* Simple-I/O GP17-10 */
+
    printk(BIOS_DEBUG, "LiPPERT SpaceRunner-LX EXIT %s\n", __func__);
 }

--- src/mainboard/lippert/spacerunner-lx/romstage.c	(rev 5739)
+++ src/mainboard/lippert/spacerunner-lx/romstage.c	(working copy)
@@ -41,7 +41,11 @@
 #include "superio/ite/it8712f/it8712f_early_serial.c"

/* Bit0 enables Spread Spectrum, bit1 makes on-board SSD act as IDE slave. */
-#define SMC_CONFIG 0x01
+#if CONFIG_ONBOARD_IDE_SLAVE
+	#define SMC_CONFIG	0x03
+#else
+	#define SMC_CONFIG	0x01
+#endif

#define ManualConf 1		/* No automatic strapped PLL config */
 #define PLLMSRhi 0x0000059C	/* Manual settings for the PLL */
@@ -201,4 +205,3 @@
    /* Memory is setup. Return to cache_as_ram.inc and continue to boot. */
    return;
 }
-
_