Re: [coreboot] [PATCH] new board: LiPPERT Cool SpaceRunner-LX

[Uwe Hermann]

Hi,

thanks a lot for your patch!

On Tue, Oct 21, 2008 at 03:55:24PM +0200, Jens Rottmann wrote:
> Add support for the LiPPERT Cool SpaceRunner-LX embedded PC board:
> PC/104+ form factor
> AMD Geode-LX CPU/northbridge
> AMD CS5536 southbridge
> ITE IT8712F superio
> Signed-off-by: Jens Rottmann
>
> My previous mail still awaits list moderator approval, but I'm sending
> this anyway. :) Maybe you'll have to apply the patches from my previous
> mail first for this to compile.
>
> There are still large parts of coreboot's design that I don't fully
> understand yet, but I hope I did all right. All mainboard files are
> based on the AMD DB800.
>
> By the way, cmos.layout doesn't seem to be used (HAVE_OPTION_TABLE=0),
> still coreboot doesn't compile without it. Is there a designated way to
> use a global, generic file instead?

Yes, I'll send a patch along those lines soon. But you can also
completely drop the cmos.layout from your board if you don't want to use
it at all. See below for details.
> Index: src/mainboard/lippert/spacerunner-lx/cache_as_ram_auto.c
>
> --- src/mainboard/lippert/spacerunner-lx/cache_as_ram_auto.c	(revision 0)
> +++ src/mainboard/lippert/spacerunner-lx/cache_as_ram_auto.c	(working copy)
> @@ -0,0 +1,213 @@
> +/*
> (C)2008 LiPPERT Embedded Computers GmbH, released under the GNU GPLv2
> Based on cache_as_ram_auto.c from AMD's DB800 and DBM690T mainboards.
> */

Please use the usual license header format for further patches, see:
http://www.coreboot.org/Development_Guidelines#Common_License_Header

Not a big issue, this is only for consistency reasons, as long as the
(1) exact license, (2) copyright owner, (3) copyright year(s) are
mentioned in the header (which they are in your patch).
> +static const unsigned short sio_init_table[] = { // hi=data, lo=index
> 0x0707,		// select LDN 7 (GPIO, SPI, watchdog, ...)

Is there a reason to encode this in an u16? You could also use a struct
like this, which is more readable IMHO:

const struct foo {
          u8 data;
          u8 index;
  } foo_table[] = {
          {0x07, 0x07},
          {0x1e, 0x2c},
          {0x04, 0x23},
          [...]
  };
> 0x1E2C,		// disable ATXPowerGood
> 0x0423,		// don't delay POWerOK1/2
> 0x9072,		// watchdog triggers POWOK, counts seconds
>
> +#if !USE_WATCHDOG_ON_BOOT
> 0x0073, 0x0074,	// disable watchdog by setting timeout to 0
>
> +#endif
> 0xBF25, 0x172A, 0xF326,	// select GPIO function for most pins
> 0xFF27, 0xDF28, 0x2729,	// (GP45=SUSB, GP23,22,16,15=SPI, GP13=PWROK1)
> 0x072C,		// VIN6=enabled?, FAN4/5 disabled, VIN7=internal, VIN3=internal
> 0x66B8, 0x0CB9,	// enable pullups
> 0x07C0,		// enable Simple-I/O for GP12-10= RS485_EN2,1, LIVE_LED
> 0x07C8,		// config GP12-10 as output
> 0x2DF5,		// map Hw Monitor Thermal Output to GP55
> 0x08F8,		// map GP LED Blinking 1 to GP10=LIVE_LED (deactivate Simple I/O to use)
> 0x0202		// exit conf mode
>
> +};
>
> +/* Early mainboard specific GPIO setup. */
> +static void mb_gpio_init(void)
> +{
> int i;
> /* Init SuperIO WDT, GPIOs. Done early, WDT init may trigger reset! */
> it8712f_enter_conf();
> for (i=0; i<ARRAY_SIZE(sio_init_table); i++) {
> unsigned short val = sio_init_table[i];
> outb((unsigned char)val, SIO_INDEX); outb(val>>8, SIO_DATA);
> }
> //it8712f_exit_conf() is included in sio_init_table

Why that? I'd call it8712f_exit_conf() here instead, makes more sense
and is more readable, IMHO.
> +}
>
> +void cache_as_ram_main(void)
> +{
> int err;
> POST_CODE(0x01);
> static const struct mem_controller memctrl[] = {
> {.channel0 = {(0xa << 3) | 0, (0xa << 3) | 1}}
> };
> SystemPreInit();
> msr_init();
> cs5536_early_setup();
> /* Note: must do this AFTER the early_setup! It is counting on some
> * early MSR setup for CS5536.
> */
> it8712f_enable_serial(0, TTYS0_BASE); // does not use its 1st parameter

Yes, this needs some cleanups, but that's for another patch.
> mb_gpio_init();
> uart_init();
> console_init();
> pll_reset(ManualConf);
> cpuRegInit();
> if ((err=smc_send_config(SMC_CONFIG))) { // bit1=on-board IDE is Slave, bit0=Spread Spectrum
> print_err("ERROR ");
> print_err_char('0'+err);
> print_err(" sending config data to SMC\r\n");
> }
> sdram_initialize(1, memctrl);
> /* Check memory. */
> /* ram_check(0x00000000, 640 * 1024); */
> /* Memory is setup. Return to cache_as_ram.inc and continue to boot. */
> return;
>
> +}
> Index: src/mainboard/lippert/spacerunner-lx/chip.h
> ===================================================================
> --- src/mainboard/lippert/spacerunner-lx/chip.h	(revision 0)
> +++ src/mainboard/lippert/spacerunner-lx/chip.h	(working copy)
> @@ -0,0 +1,11 @@
> +/*
> (C)2008 LiPPERT Embedded Computers GmbH, released under the GNU GPLv2
> Based on chip.h from AMD's DB800 mainboard.

This line is not needed, way too trivial file. The license header should
stay though.
> */
>
> +extern struct chip_operations mainboard_lippert_spacerunner_lx_ops;
>
> +struct mainboard_lippert_spacerunner_lx_config {
> unsigned char sio_gp1x_config;	// bit2=RS485_EN2, bit1=RS485_EN1, bit0=Live LED
>
> +};
> Index: src/mainboard/lippert/spacerunner-lx/cmos.layout
>
> --- src/mainboard/lippert/spacerunner-lx/cmos.layout	(revision 0)
> +++ src/mainboard/lippert/spacerunner-lx/cmos.layout	(working copy)

You can drop this file completely if you don't want to use it at all,
see below.
> Index: src/mainboard/lippert/spacerunner-lx/Config.lb
>
> --- src/mainboard/lippert/spacerunner-lx/Config.lb	(revision 0)
> +++ src/mainboard/lippert/spacerunner-lx/Config.lb	(working copy)

Please also add the usual license header to this file (_all_ files for
that matter).
> +driver mainboard.o
>
> +if HAVE_PIRQ_TABLE
> object irq_tables.o
>
> +end
>
> +if USE_DCACHE_RAM
> #compile cache_as_ram.c to auto.inc
> makerule ./cache_as_ram_auto.inc
> 	depends "$(MAINBOARD)/cache_as_ram_auto.c option_table.h"

Remove the "option_table.h" here and the board should compile fine
if you remove cmos.layout.
> 	action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS) $(MAINBOARD)/cache_as_ram_auto.c -Os -nostdinc -nostdlib -fno-builtin -Wall -c -S -o $@"
> 	action "perl -e 's/.rodata/.rom.data/g' -pi $@"
> 	action "perl -e 's/.text/.section .rom.text/g' -pi $@"
> end
>
> +end
> +dir /pc80
> +config chip.h
> +# see also SMC_CONFIG in cache_as_ram_auto.c
> +register "sio_gp1x_config" = "0x01" # bit0 turns off Live LED, bit1 switches Com1 to RS485, bit2 same for Com2
>
> +chip northbridge/amd/lx
> device pci_domain 0 on
> device pci 1.0 on end				# Northbridge
> device pci 1.1 on end				# Graphics
> device pci 1.2 on end				# AES
> chip southbridge/amd/cs5536
> 	# IRQ 12 and 1 unmasked,  Keyboard and Mouse IRQs. OK
> 	# SIRQ Mode = Active(Quiet) mode. Save power....
> 	# Invert mask = IRQ 12 and 1 are active high. Keyboard and Mouse, UARTs, etc IRQs. OK
> 	register "lpc_serirq_enable"   = "0x000012DA"	# 00010010 11011010
> 	register "lpc_serirq_polarity" = "0x0000ED25"	# inverse of above
> 	register "lpc_serirq_mode" = "1"
> 	register "enable_gpio_int_route" = "0x0D0C0700"
> 	register "enable_ide_nand_flash" = "0"	# 0:ide mode, 1:flash
> 	register "enable_USBP4_device" = "0"	# 0: host, 1:device
> 	register "enable_USBP4_overcurrent" = "0" #0:off, xxxx:overcurrent setting CS5536 Data Book (pages 380-381)
> 	register "com1_enable" = "0"
> 	register "com1_address" = "0x3E8"
> 	register "com1_irq" = "6"
> 	register "com2_enable" = "0"
> 	register "com2_address" = "0x2E8"
> 	register "com2_irq" = "6"

Are your sure about these ports? Usually serial uses 0x2f8 and 0x3f8
(not 0x2e8/0x3e8). Or is this on purpose? Also, why are both IRQs 6?
And finally, these are disabled, as serial is done by the IT8712F below?
> 			device pnp 2e.1 on #  Com1
> 				io 0x60 = 0x3f8
> 				irq 0x70 = 4
> 			end
> 			device pnp 2e.2 on #  Com2
> 				io 0x60 = 0x2f8
> 				irq 0x70 = 3
> 			end
> +static const unsigned short ec_init_table[] = { // hi=data, lo=index

Same as above here, a struct may be more readable.
> 0x1900,		// enable monitoring
> 0x3050,		// VIN4,5 enabled
> 0x0351,		// TMPIN1,2 diode mode, TMPIN3 off
> 0x805C,		// unlock zero adjust
> 0x7056, 0x3C57,	// zero adjust TMPIN1,2
> 0x005C,		// lock zero adjust
> 0xD014		// also set FAN_CTL polarity to Active High
>
> +};
> Index: src/mainboard/lippert/spacerunner-lx/Options.lb
>
> --- src/mainboard/lippert/spacerunner-lx/Options.lb	(revision 0)
> +++ src/mainboard/lippert/spacerunner-lx/Options.lb	(working copy)
> @@ -0,0 +1,190 @@

Add the usual license header in this file, too, please.
> +# Compile extra debugging code
> +default DEBUG=1

Not sure about this, I'd rather drop it. We already have 9 (!)
loglevels for DEFAULT_CONSOLE_LOGLEVEL/MAXIMUM_CONSOLE_LOGLEVEL,
no need to add yet another debug option.
> +# Saves space on ROM_IMAGE_SIZE, but decompression costs a second on boot
> +option CONFIG_COMPRESS = 1
>
> +romimage "fallback"

Minor issue, but "image" is a bit more readable (instead of "fallback")
if you don't use the normal/fallback mechanism anyway.
> option USE_FALLBACK_IMAGE=1
> option ROM_IMAGE_SIZE=64*1024
> option COREBOOT_EXTRA_VERSION=".0Fallback"
> payload ../payload.elf
> # If getting payload from IDE
> #payload /dev/null
>
> +end
>
> +buildrom ./coreboot.rom ROM_SIZE "fallback"

This needs to be changed to "image" too, in that case.

Uwe.