[LinuxBIOS] [PATCH] BCOM Winnet100 support

Uwe Hermann uwe at hermann-uwe.de
Tue Oct 9 20:36:05 CEST 2007


On Tue, Oct 09, 2007 at 07:51:48PM +0200, Juergen Beisert wrote:
> this patch adds main support for BCOM's Winnet100 Geode GX1 based terminal.
> It activates the VGA support so it would be possible to run this target as a
> small (and silent) X terminal. With faster SDRAM timing (not yet supported in
> mainline) it can run graphical resolutions up to SXGA (1280x1024) at 64k
> colours.
> 
> I'm not sure if all files I add to src/mainboard/bcom/winnet100 are required.
> Most of them are copies from other GX1 based mainboards.

Nope, not all are required. And most miss a GPL header which we should add.


> Index: src/mainboard/bcom/winnet100/Config.lb
> ===================================================================
> --- src/mainboard/bcom/winnet100/Config.lb	(Revision 0)
> +++ src/mainboard/bcom/winnet100/Config.lb	(Revision 0)

Missing GPL header. This is just (relatively small) config file (which should
have a common part somewhere global btw, but that's for another patch)
so I'm not sure if we have to track down all of this to the first author of
this code (which is used in basically _every_ board).

I'd just slap a '(C) Juergen Beisert' on it and that's it.
Counter-opinions welcome, but only with a patch to add the original
author to all Config.lb files then ;-)


> +##
> +## Romcc output
> +##
> +makerule ./failover.E
> +	depends "$(MAINBOARD)/failover.c ./romcc"
> +	action "./romcc -E -O --label-prefix=failover -I$(TOP)/src -I. $(CPPFLAGS) $(MAINBOARD)/failover.c -o $@"
> +end
> +
> +makerule ./failover.inc
> +	depends "$(MAINBOARD)/failover.c ./romcc"
> +	action "./romcc    -O --label-prefix=failover -I$(TOP)/src -I. $(CPPFLAGS) $(MAINBOARD)/failover.c -o $@"
> +end

Please use the global src/arch/i386/lib/failover.c instead of a local
duplicate of the file (i.e. drop failover.c).

You'll have to modify this part a bit, see
http://tracker.linuxbios.org/trac/LinuxBIOS/changeset/2772


> Index: src/mainboard/bcom/winnet100/irq_tables.c
> ===================================================================
> --- src/mainboard/bcom/winnet100/irq_tables.c	(Revision 0)
> +++ src/mainboard/bcom/winnet100/irq_tables.c	(Revision 0)

Please add the usual GPL header, see
http://linuxbios.org/Development_Guidelines#Common_License_Header


> @@ -0,0 +1,116 @@
> +/**
> + * @file
> + * @brief Interrupt routing description for BCOM's Winnet100 board

You can drop the @brief, we use the JavaDoc-like autobrief config option
in Doxygen (in v3 at least, but also here I think).

Not sure if you can/want to drop the @file.


> +/**
> + * Routing desciption.
> + * Documentation at : http://www.microsoft.com/whdc/archive/pciirq.mspx
> + */
> +const struct irq_routing_table intel_irq_routing_table = {
> +	.signature = PIRQ_SIGNATURE,	/* u32 signature */
> +	.version = PIRQ_VERSION,	/* u16 version   */
> +	.size = 32+16*IRQ_SLOT_COUNT,	/* there can be total 2 devices on the bus */

2 -> IRQ_SLOT_COUNT, otherwise it's confusing


> +	.rtr_bus = 0x00,		/* Where the interrupt router lies (bus) */
> +	.rtr_devfn = (0x12<<3)|0x0,	/* Where the interrupt router lies (dev) */
> +	.exclusive_irqs = 0x8800,	/* IRQs devoted exclusively to PCI usage */

Where does this value come from?


> +/**
> + * copy the IRQ routing table to memory

Start all sentences with capital letter and end them with full stop,
please (I'm not kidding ;-))


> + * @param[in] addr destination address (between 0xF0000...0x100000)

Capital letters and full-stop here, too, please. Looks better in the
Doxygen output, IMO.

  *
  * @param[in] addr Destination address for the IRQ routing table
  *                 (between 0xF0000...0x100000).
  *

Missing @return line, btw.


> + **/

One '*' too many.


> +unsigned long write_pirq_routing_table(unsigned long addr)
> +{
> +	return copy_pirq_routing_table(addr);
> +}

This should actually be some global function, but in this case that
would be really overkill, I guess ;-)


> +/* end of file irq_tables.c */

Not needed.


> Index: src/mainboard/bcom/winnet100/Options.lb
> ===================================================================
> --- src/mainboard/bcom/winnet100/Options.lb	(Revision 0)
> +++ src/mainboard/bcom/winnet100/Options.lb	(Revision 0)

Missing GPL header.


> +##
> +## Enable VGA with a splash screen
> +## but only 640x480 to run on nearly all monitors
> +##
> +
> +default CONFIG_GX1_VIDEO=1
> +default CONFIG_GX1_VIDEOMODE=0
> +default CONFIG_SPLASH_GRAPHIC=1
[...]
> +##
> +## We want to support up to 1024x768 at 16 so we need
> +## 2MiB video memory. Note: Higher resolutions might
> +## need faster SDRAM speed.
> +##
> +
> +default CONFIG_VIDEO_MB=2

Please also add these as comments to targets/.../Config.lb,
as users are likely to want to modify them.

# option CONFIG_GX1_VIDEO=1
# option CONFIG_GX1_VIDEOMODE=0
# option CONFIG_SPLASH_GRAPHIC=1
# option CONFIG_VIDEO_MB=2


> +##
> +## Build code to export a CMOS option table
> +##
> +default HAVE_OPTION_TABLE=0
[...]
> +##
> +## Only use the option table in a normal image
> +##
> +default USE_OPTION_TABLE = 0

If you're not using the CMOS option table, you can drop the cmos.layout file.


> +###
> +### LinuxBIOS layout values
> +###
> +
> +## ROM_IMAGE_SIZE is the amount of space to allow linuxBIOS to occupy.
> +default ROM_IMAGE_SIZE = 65536
> +default FALLBACK_SIZE = 131072

Please make those x*1024 format too, for consistency (_all_ sizes should
use that format, IMO).


> Index: src/mainboard/bcom/winnet100/failover.c

Should be dropped, see above.


> Index: src/mainboard/bcom/winnet100/auto.c
> ===================================================================
> --- src/mainboard/bcom/winnet100/auto.c	(Revision 0)
> +++ src/mainboard/bcom/winnet100/auto.c	(Revision 0)
> @@ -0,0 +1,54 @@
> +/*
> + * This file is part of the LinuxBIOS project.
> + *
> + * Copyright (C) 2007 Uwe Hermann <uwe at hermann-uwe.de>
> + * Modyfied by Juergen Beisert <juergen at kreuzholzen.de>

Nah, drop me, this is way too trivial to list my name here. Just use

 Copyright (C) 2007 Juergen Beisert <juergen at kreuzholzen.de>


> +static void main(unsigned long bist)
> +{
> +	/* Initialize the serial console. */
> +	pc97317_enable_serial(SERIAL_DEV, TTYS0_BASE);
> +	uart_init();
> +	console_init();
> +
> +	/* Halt if there was a built in self test failure. */
> +	report_bist_failure(bist);
> +
> +	/* Initialize RAM. */
> +	sdram_init();
> +
> +	/* Check whether RAM works. */
> +	/* ram_check(0x00000000, 0x4000); */

ram_check(0x00000000, 640 * 1024);

(just in case anybody want to uncomment it; I think it'll fail in the
current form)


> Index: src/mainboard/bcom/winnet100/chip.h

Missing GPL header (yes, trivial file, but please add it anyway).


> Index: src/mainboard/bcom/winnet100/cmos.layout
> ===================================================================
> --- src/mainboard/bcom/winnet100/cmos.layout	(Revision 0)
> +++ src/mainboard/bcom/winnet100/cmos.layout	(Revision 0)

Unused, so can be dropped (or, alternatively, it should be tested
and the CMOS config option above be enabled).


> Index: src/mainboard/bcom/winnet100/mainboard.c
> ===================================================================
> --- src/mainboard/bcom/winnet100/mainboard.c	(Revision 0)
> +++ src/mainboard/bcom/winnet100/mainboard.c	(Revision 0)
> @@ -0,0 +1,11 @@
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ids.h>
> +#include <device/pci_ops.h>
> +#include <arch/io.h>
> +#include "chip.h"

All except
  #include <device/device.h>
  #include "chip.h"
can be dropped, I think.


> +struct chip_operations mainboard_bcom_winnet100_ops = {
> +	CHIP_NAME("BCOM winterm100 mainboard ")

winterm100 -> winnet100?

What's the exact spelling in the manual or on the PCB? We should try to
use the "correct" name, if possible.


> Index: targets/bcom/winnet100/Config.lb
> ===================================================================
> --- targets/bcom/winnet100/Config.lb	(Revision 0)
> +++ targets/bcom/winnet100/Config.lb	(Revision 0)

Missing GPL header.


> @@ -0,0 +1,24 @@
> +# Config file for the BCOM WINNET100 motherboard
> +# (available as IGEL-316 for example)
> +# Refer: http://www.linuxbios.org/index.php/BCOM_WINNET100_Build_Tutorial

Drop the "index.php/" from the URL, it'll work too, and the ugly
index.php is only a temporary fix, will be gone soon.


> +#
> +target winnet100
> +mainboard bcom/winnet100
> +
> +option ROM_SIZE=1024*256

Hm, I'd do it the other way around (but it doesn't matter much):

  option ROM_SIZE = 256 * 1024


> +
> +romimage "normal"
> +	option USE_FALLBACK_IMAGE=0
> +	option ROM_IMAGE_SIZE=0x10000

  option ROM_IMAGE_SIZE = 64 * 1024


> +	option LINUXBIOS_EXTRA_VERSION=".0Normal"
> +	payload ../../../../../../../images/etherboot.elf
> +end
> +
> +romimage "fallback"
> +	option USE_FALLBACK_IMAGE=1
> +	option ROM_IMAGE_SIZE=0x10000

Same here.



Patch looks good, with the above fixes I think we can commit it.

Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071009/c0fa42bf/attachment.sig>


More information about the coreboot mailing list