Re: [coreboot] [PATCH] flashrom: Add --list-supported-wiki, list known-good/-bad boards

[Carl-Daniel Hailfinger]

On 12.05.2009 22:06, Uwe Hermann wrote:
> On Tue, May 12, 2009 at 10:03:56PM +0200, Uwe Hermann wrote:
> > On Tue, May 12, 2009 at 06:31:15PM +0200, Carl-Daniel Hailfinger wrote:
> > > Please make sure the "... unknown SPI chip" variants are not listed.
> > > Status: "Broken" ends up as "?".
> >
> > True, but I'd like to fix these issues in a different patch, this one's
> > quite huge and invasive already.

Since that code seems to be new, I figured you could fix it in the first
commit, but I'm not picky as long as you promise to send a followup patch.
> > > IMHO the wiki output code uglifies quite a lot of files. Please stick it
> > > into one common file called wikifier.c or something like that.
> >
> > Yeah, good idea. Attached is the second iteration of the patch which has
> > the wiki printing code in wikify.c.
> > That requires to put some structs into flash.h though and make some
> > stuff non-'static'.
>
> Add a --list-supported-wiki / -z option which outputs the currently
> supported flash chips (and their status), chipsets (plus status),
> and mainboards (plus status) to stdout.
>
> This allows for very easy pasting into the http://coreboot.org/Flashrom
> page, so we can keep that page up-to-date without much hassle.
>
> The list of boards is mostly new (known good ones which don't need
> write-enable code, and known-bad ones) and also lists URLs to the
> vendor's mainboard pages.
> These URLs are NOT printed in -L output per default, but they are
> if you use verbose mode, e.g. via -LV.
> They're also used in the --list-supported-wiki output for the wiki,
> of course.
>
> Signed-off-by: Uwe Hermann
> Index: flashrom.8
>
> --- flashrom.8	(Revision 498)
> +++ flashrom.8	(Arbeitskopie)
> @@ -2,7 +2,7 @@
>  .SH NAME
>  flashrom - read, write, verify and erase BIOS/ROM/flash chips
>  .SH SYNOPSIS
> -.B flashrom \fR[\fB-rwvEVfLhR\fR] [\fB-c\fR chipname] [\fB-s\fR exclude_start] [\fB-e\fR exclude_end]
> +.B flashrom \fR[\fB-rwvEVfLzhR\fR] [\fB-c\fR chipname] [\fB-s\fR exclude_start] [\fB-e\fR exclude_end]
>           [\fB-m\fR [vendor:]part] [\fB-l\fR file.layout] [\fB-i\fR image_name] [file]
>  .SH DESCRIPTION
>  .B flashrom
> @@ -123,6 +123,12 @@
>  Please let us know if you can verify other boards to work or not work out
>  of the box.
>  .TP
> +.B "-z, --list-supported-wiki"
> +Same as
> +.BR --list-supported ,
> +but outputs the supported hardware in MediaWiki syntax, so that it can be
> +easily pasted into the wiki page at http://coreboot.org/Flashrom.
> +.TP
>  .B "-p, --programmer <name>"
>  Specify the programmer device. Currently supported are:
>  .sp
> Index: flash.h
> ===================================================================
> --- flash.h	(Revision 498)
> +++ flash.h	(Arbeitskopie)
> @@ -29,6 +29,7 @@
>  #include <unistd.h>
>  #include <stdint.h>
>  #include <stdio.h>
> +#include <pci/pci.h>
>
> /* for iopl and outb under Solaris */
>  #if defined (__sun) && (defined(__i386) || defined(__amd64))
> @@ -201,6 +202,75 @@
>
> extern struct flashchip flashchips[];
>
> +typedef struct penable {
> uint16_t vendor_id;
> uint16_t device_id;
> int status;
> const char *vendor_name;
> const char *device_name;
> int (*doit) (struct pci_dev *dev, const char *name);
>
> +} FLASH_ENABLE;

Please kill that ugly typedef while you're at it.
> +#define OK 0
> +#define NT 1	/* Not tested */

Prefix please. CHIPSET_OK etc.
> +extern const FLASH_ENABLE enables[];

And rename enables to chipset_enables if at all possible. I never saw a
more ambiguous variable name in flashrom. Sure, it's not your fault, but
since you're touching that code anyway... no good deed goes unpunished.
> +/**
> We use 2 sets of IDs here, you're free to choose which is which. This
> is to provide a very high degree of certainty when matching a board on
> the basis of subsystem/card IDs. As not every vendor handles
> subsystem/card IDs in a sane manner.
> Keep the second set NULLed if it should be ignored. Keep the subsystem IDs
> NULLed if they don't identify the board fully. But please take care to
> provide an as complete set of pci ids as possible; autodetection is the
> preferred behaviour and we would like to make sure that matches are unique.
> The coreboot ids are used two fold. When running with a coreboot firmware,
> the ids uniquely matches the coreboot board identification string. When a
> legacy bios is installed and when autodetection is not possible, these ids
> can be used to identify the board through the -m command line argument.
> When a board is identified through its coreboot ids (in both cases), the
> main pci ids are still required to match, as a safeguard.
> */
>
> +struct board_pciid_enable {
> /* Any device, but make it sensible, like the ISA bridge. */
> uint16_t first_vendor;
> uint16_t first_device;
> uint16_t first_card_vendor;
> uint16_t first_card_device;
> /* Any device, but make it sensible, like
> * the host bridge. May be NULL.
> */
> uint16_t second_vendor;
> uint16_t second_device;
> uint16_t second_card_vendor;
> uint16_t second_card_device;
> /* The vendor / part name from the coreboot table. */
> const char *lb_vendor;
> const char *lb_part;
> const char *vendor_name;
> const char *board_name;
> int (*enable) (const char *name);
>
> +};
>
> +extern struct board_pciid_enable board_pciid_enables[];
>
> +struct board_info {
> const char *vendor;
> const char *name;
> const char *url;
>
> +};
>
> +extern const struct board_info boards_ok[];
> +extern const struct board_info boards_no[];

boards_bad maybe?
> /*
> Please keep this list sorted alphabetically by manufacturer. The first
> entry of each section should be the manufacturer ID, followed by the
>
> @@ -537,6 +607,11 @@
>  #define W_49V002A		0xB0
>  #define W_49V002FA		0x32
>
> +/* wikify.c */
> +void print_supported_chips_wiki(void);
> +void print_supported_boards_wiki(void);
> +void print_supported_chipsets_wiki(void);
>
> /* udelay.c */
>  void myusec_delay(int time);
>  void myusec_calibrate_delay(void);
> Index: Makefile
> ===================================================================
> --- Makefile	(Revision 498)
> +++ Makefile	(Arbeitskopie)
> @@ -35,7 +35,7 @@
>    sst49lfxxxc.o sst_fwhub.o layout.o cbtable.o flashchips.o physmap.o \
>    flashrom.o w39v080fa.o sharplhf00l04.o w29ee011.o spi.o it87spi.o \
>    ichspi.o w39v040c.o sb600spi.o wbsio_spi.o m29f002.o internal.o \
> dummyflasher.o
> dummyflasher.o wikify.o
>
> all: pciutils dep $(PROGRAM)
> Index: wikify.c
>
> --- wikify.c	(Revision 0)
> +++ wikify.c	(Revision 0)
> @@ -0,0 +1,196 @@
> +/*
> This file is part of the flashrom project.
> Copyright (C) 2009 Uwe Hermann
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <string.h>
> +#include "flash.h"
>
> +#define CHIPSET_TH "{| border="0" style="font-size: smaller"\n\
> +|- bgcolor="#6699dd"\n! align="left" | Vendor\n\
> +! align="left" | Southbridge\n! align="left" | Status\n\n"
>
> +void print_supported_chipsets_wiki(void)
> +{
> int i, j, enablescount = 0, color = 1;
> const FLASH_ENABLE *e = enables;
> printf("\n== Supported chipsets ==\n\n{| border="0" "
>       "valign=\"top\"\n| valign=\"top\"|\n\n" CHIPSET_TH);
> for (e = enables; e->vendor_name != NULL; e++)
> enablescount++;
> for (i = 0, j = 0; enables[i].vendor_name != NULL; i++, j++) {
> /* Alternate colors if the vendor changes. */
> if (i > 0 && strcmp(enables[i].vendor_name,
>     enables[i - 1].vendor_name))
> 	color = !color;
> printf("|- bgcolor=\"#%s\" valign=\"top\"\n| %s\n| %s"
>        "\n| %s\n", (color) ? "eeeeee" : "dddddd",
>        enables[i].vendor_name, enables[i].device_name,
>        (enables[i].status == OK) ? "{{OK}}" : "?");
> /* Split table in three columns. */
> if (j >= (enablescount / 3 + 1)) {
> 	printf("\n|}\n\n| valign=\"top\"|\n\n" CHIPSET_TH);
> 	j = 0;
> }
> }
> printf("\n|}\n\n|}\n");
>
> +}
>
> +#define BOARD_TH "{| border="0" style="font-size: smaller" valign="top"\
> +\n|- bgcolor="#6699dd"\n! align="left" | Vendor\n\
> +! align="left" | Mainboard\n! align="left" | Status\n\n"
>
> +#define BOARD_TH2 "{| border="0" style="font-size: smaller" valign="top"\
> +\n|- bgcolor="#6699dd"\n! align="left" | Vendor\n\
> +! align="left" | Mainboard\n! align="left" | Required option\n\
> +! align="left" | Status\n\n"
>
> +static void wiki_helper(const char *heading, const char *status,
> 	       int cols, const struct board_info boards[])
>
> +{
> int i, j, boardcount = 0, color = 1;
> const struct board_info *b;
> printf("\n'''%s'''\n\n{| border="0" "
>       "valign=\"top\"\n| valign=\"top\"|\n\n%s", heading, BOARD_TH);
> for (b = boards; b->vendor != NULL; b++)
> boardcount++;
>    for (i = 0, j = 0, b = boards; b[i].vendor != NULL; i++, j++) {
> /* Alternate colors if the vendor changes. */
> if (i > 0 && strcmp(b[i].vendor, b[i - 1].vendor))
> 	color = !color;
> printf("|- bgcolor=\"#%s\" valign=\"top\"\n| %s\n|%s%s %s%s\n|"
>        " {{%s}}\n", (color) ? "eeeeee" : "dddddd", b[i].vendor,
>        (b[i].url) ? " [" : "", (b[i].url) ? b[i].url : "",
>        b[i].name, (b[i].url) ? "]" : "", status);
> /* Split table in three columns. */
> if (j >= (boardcount / cols + 1)) {
> 	printf("\n|}\n\n| valign=\"top\"|\n\n" BOARD_TH);
> 	j = 0;
> }
> }
> printf("\n|}\n\n|}\n");
>
> +}
>
> +static void wiki_helper2(const char *heading)
> +{
> int i, j, boardcount = 0, color = 1;
> struct board_pciid_enable *b;
> printf("\n'''%s'''\n\n{| border="0" "
>       "valign=\"top\"\n| valign=\"top\"|\n\n%s", heading, BOARD_TH2);
> for (b = board_pciid_enables; b->vendor_name != NULL; b++)
> boardcount++;
> b = board_pciid_enables;
> for (i = 0, j = 0; b[i].vendor_name != NULL; i++, j++) {
> /* Alternate colors if the vendor changes. */
> if (i > 0 && strcmp(b[i].vendor_name, b[i - 1].vendor_name))
> 	color = !color;
> printf("|- bgcolor=\"#%s\" valign=\"top\"\n| %s\n| %s\n| "
>        "%s%s%s%s\n| {{OK}}\n", (color) ? "eeeeee" : "dddddd",
>        b[i].vendor_name, b[i].board_name,
>        (b[i].lb_vendor) ? "-m " : "&mdash;",
>        (b[i].lb_vendor) ? b[i].lb_vendor : "",
>        (b[i].lb_vendor) ? ":" : "",
>        (b[i].lb_vendor) ? b[i].lb_part : "");
> /* Split table in two columns. */
> if (j >= (boardcount / 2 + 1)) {
> 	printf("\n|}\n\n| valign=\"top\"|\n\n" BOARD_TH2);

To be honest, I don't understand the criteria of how the string above
was split into a string and a #defined string.
> 	j = 0;
> }
> }
> printf("\n|}\n\n|}\n");
>
> +}
>
> +#define BOARD_INTRO "\
> +\n== Supported mainboards ==\n\n\
> +In general, it is very likely that flashrom works out of the box even if your \
> +mainboard is not listed below.\n\nThis is a list of mainboards where we have \
> +verified that they either do or do not need any special initialization to \
> +make flashrom work (given flashrom supports the respective chipset and flash \
> +chip), or that they do not yet work at all. If they do not work, support may \
> +or may not be added later.\n\n\
> +Mainboards which don't appear in the list may or may not work (we don't \
> +know, someone has to give it a try). Please report any further verified \
> +mainboards on the [[Mailinglist|mailing list]].\n"

Maybe insert that statement into the printf instead? Same for all other
multiline statements.
> +void print_supported_boards_wiki(void)
> +{
> printf("%s", BOARD_INTRO);
> wiki_helper("Known good (worked out of the box)", "OK", 3, boards_ok);
> wiki_helper2("Known good (with write-enable code in flashrom)");
> wiki_helper("Not supported (yet)", "No", 1, boards_no);
>
> +}
>
> +#define CHIP_TH "{| border="0" style="font-size: smaller" valign="top"\n\
> +|- bgcolor="#6699dd"\n! align="left" | Vendor\n\
> +! align="left" | Flash part\n! align="left" colspan="4" | Status\n\n\
> +|- bgcolor="#6699ff"\n| colspan="2" | &nbsp;\n\
> +| Probe\n| Read\n| Write\n| Erase\n\n"
>
> +void print_supported_chips_wiki(void)
> +{
> int i = 0, c = 1, chipcount = 0;
> struct flashchip *f, *old = NULL;
> printf("\n== Supported chips ==\n\n{| border="0" valign="top"\n"
> "| valign=\"top\"|\n\n" CHIP_TH);
> for (f = flashchips; f->name != NULL; f++)
> chipcount++;
> for (f = flashchips; f->name != NULL; f++, i++) {
> /* Alternate colors if the vendor changes. */
> if (old != NULL && strcmp(old->vendor, f->vendor))
> 	c = !c;
> printf("|- bgcolor=\"#%s\" valign=\"top\"\n| %s"
>        "\n| %s\n| {{%s}}\n| {{%s}}\n| {{%s}}\n| {{%s}}\n",
>        (c == 1) ? "eeeeee" : "dddddd", f->vendor, f->name,
>        ((f->tested & TEST_OK_PROBE) ? "OK" : (c) ? "?2" : "?"),
>        ((f->tested & TEST_OK_READ)  ? "OK" : (c) ? "?2" : "?"),
>        ((f->tested & TEST_OK_ERASE) ? "OK" : (c) ? "?2" : "?"),
>        ((f->tested & TEST_OK_WRITE) ? "OK" : (c) ? "?2" : "?"));
> /* Split table into three columns. */
> if (i >= (chipcount / 3 + 1)) {
> 	printf("\n|}\n\n| valign=\"top\"|\n\n" CHIP_TH);
> 	i = 0;
> }
> old = f;
> }
> printf("\n|}\n\n|}\n");
>
> +}
> Index: chipset_enable.c
> ===================================================================
> --- chipset_enable.c	(Revision 498)
> +++ chipset_enable.c	(Arbeitskopie)
> @@ -33,6 +33,7 @@
>  #include <sys/mman.h>
>  #include <fcntl.h>
>  #include <unistd.h>
> +#include <string.h>
>  #include "flash.h"
>
> unsigned long flashbase = 0;
> @@ -903,20 +904,8 @@
>    return 0;
>  }
>
> -typedef struct penable {
> uint16_t vendor_id;
> uint16_t device_id;
> int status;
> const char *vendor_name;
> const char *device_name;
> int (*doit) (struct pci_dev *dev, const char *name);
>
> -} FLASH_ENABLE;
>
> -#define OK 0
> -#define NT 1	/* Not tested */
>
> /* Please keep this list alphabetically sorted by vendor/device. */
> -static const FLASH_ENABLE enables[] = {
> +const FLASH_ENABLE enables[] = {
>    {0x10B9, 0x1533, OK, "ALi", "M1533",		enable_flash_ali_m1533},
>    {0x1022, 0x7440, OK, "AMD", "AMD-768",		enable_flash_amd8111},
>    {0x1022, 0x7468, OK, "AMD", "AMD8111",		enable_flash_amd8111},
> @@ -993,6 +982,8 @@
>    {0x1106, 0x3372, OK, "VIA", "VT8237S",		enable_flash_vt8237s_spi},
>    {0x1106, 0x0586, OK, "VIA", "VT82C586A/B",	enable_flash_amd8111},
>    {0x1106, 0x0686, NT, "VIA", "VT82C686A/B",	enable_flash_amd8111},
> {0x0000, 0x0000, 0,  NULL,  NULL,               NULL},
>
> };
>
> void print_supported_chipsets(void)
> @@ -1001,11 +992,12 @@
>
> printf("\nSupported chipsets:\n\n");
> for (i = 0; i < ARRAY_SIZE(enables); i++)
> for (i = 0; enables[i].vendor_name != NULL; i++) {

Good find!
> printf("%s %s [%04x:%04x]%s\n", enables[i].vendor_name,
>        enables[i].device_name, enables[i].vendor_id,
>        enables[i].device_id,
>        (enables[i].status == OK) ? "" : " (untested)");
> }
>
> }
>
> int chipset_flash_enable(void)
> @@ -1015,7 +1007,7 @@
>    int i;
>
> /* Now let's try to find the chipset we have... */
> for (i = 0; i < ARRAY_SIZE(enables); i++) {
> for (i = 0; enables[i].vendor_name != NULL; i++) {
> dev = pci_dev_find(enables[i].vendor_id, enables[i].device_id);
> if (dev)
> 	break;
> Index: flashrom.c
>
> --- flashrom.c	(Revision 498)
> +++ flashrom.c	(Arbeitskopie)
> @@ -292,7 +292,7 @@
>
> void usage(const char *name)
>  {
> printf("usage: %s [-rwvEVfLhR] [-c chipname] [-s exclude_start]\n",
> printf("usage: %s [-rwvEVfLzhR] [-c chipname] [-s exclude_start]\n",
>      name);
> printf("       [-e exclude_end] [-m [vendor:]part] [-l file.layout] [-i imagename] [file]\n");
> printf("Please  note that the command line interface for flashrom will "
>
> @@ -313,6 +313,7 @@
>         "   -l | --layout <file.layout>:      read rom layout from file\n"
>         "   -i | --image <name>:              only flash image name from flash layout\n"
>         "   -L | --list-supported:            print supported devices\n"
>     "   -z | --list-supported-wiki:       print supported devices in wiki syntax\n"
>   "   -p | --programmer <name>:         specify the programmer device\n"
>   "   -h | --help:                      print this help text\n"
>   "   -R | --version:                   print the version (release)\n"
>
> @@ -337,6 +338,7 @@
>    int option_index = 0;
>    int force = 0;
>    int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0;
>
> int list_supported = 0, list_supported_wiki = 0;
> int ret = 0, i;
>
> static struct option long_options[] = {
>
> @@ -353,6 +355,7 @@
>    	{"layout", 1, 0, 'l'},
>    	{"image", 1, 0, 'i'},
>    	{"list-supported", 0, 0, 'L'},
> {"list-supported-wiki", 0, 0, 'z'},
> {"programmer", 1, 0, 'p'},
> {"help", 0, 0, 'h'},
> {"version", 0, 0, 'R'},
>
> @@ -375,7 +378,7 @@
>    }
>
> setbuf(stdout, NULL);
> while ((opt = getopt_long(argc, argv, "rRwvVEfc:s:e:m:l:i:p:Lh",
> while ((opt = getopt_long(argc, argv, "rRwvVEfc:s:e:m:l:i:p:Lzh",
> 		  long_options, &option_index)) != EOF) {
> switch (opt) {
> case 'r':
>
> @@ -429,11 +432,11 @@
>    		find_romentry(tempstr);
>    		break;
>    	case 'L':
> 	print_supported_chips();
> 	print_supported_chipsets();
> 	print_supported_boards();
> 	exit(0);
> 	list_supported = 1;
> break;
> case 'z':
> 	list_supported_wiki = 1;
> 	break;
> case 'p':
> 	if (strncmp(optarg, "internal", 8) == 0) {
> 		programmer = PROGRAMMER_INTERNAL;
>
> @@ -455,6 +458,21 @@
>    	}
>    }
> if (list_supported) {
> print_supported_chips();
> print_supported_chipsets();
> print_supported_boards();
> exit(0);
> }
> if (list_supported_wiki) {
> printf("= Supported devices =\n");
> print_supported_chips_wiki();
> print_supported_chipsets_wiki();
> print_supported_boards_wiki();
> exit(0);
> }
> if (read_it && write_it) {
>   printf("Error: -r and -w are mutually exclusive.\n");
>   usage(argv[0]);
> Index: board_enable.c
>
> --- board_enable.c	(Revision 498)
> +++ board_enable.c	(Arbeitskopie)
> @@ -645,50 +645,6 @@
>    return 0;
>  }
>
> -/**
> We use 2 sets of IDs here, you're free to choose which is which. This
> is to provide a very high degree of certainty when matching a board on
> the basis of subsystem/card IDs. As not every vendor handles
> subsystem/card IDs in a sane manner.
> Keep the second set NULLed if it should be ignored. Keep the subsystem IDs
> NULLed if they don't identify the board fully. But please take care to
> provide an as complete set of pci ids as possible; autodetection is the
> preferred behaviour and we would like to make sure that matches are unique.
> The coreboot ids are used two fold. When running with a coreboot firmware,
> the ids uniquely matches the coreboot board identification string. When a
> legacy bios is installed and when autodetection is not possible, these ids
> can be used to identify the board through the -m command line argument.
> When a board is identified through its coreboot ids (in both cases), the
> main pci ids are still required to match, as a safeguard.
> */
>
> -struct board_pciid_enable {
> /* Any device, but make it sensible, like the ISA bridge. */
> uint16_t first_vendor;
> uint16_t first_device;
> uint16_t first_card_vendor;
> uint16_t first_card_device;
> /* Any device, but make it sensible, like
> * the host bridge. May be NULL.
> */
> uint16_t second_vendor;
> uint16_t second_device;
> uint16_t second_card_vendor;
> uint16_t second_card_device;
> /* The vendor / part name from the coreboot table. */
> const char *lb_vendor;
> const char *lb_part;
> const char *vendor_name;
> const char *board_name;
> int (*enable) (const char *name);
>
> -};
>
> /* Please keep this list alphabetically ordered by vendor/board name. */
>  struct board_pciid_enable board_pciid_enables[] = {
>    /* first pci-id set [4],          second pci-id set [4],          coreboot id [2],             vendor name    board name        flash enable */
> @@ -728,26 +684,131 @@
>    {     0,      0,      0,      0,       0,      0,      0,      0, NULL,         NULL,          NULL,          NULL,             NULL}, /* end marker */
>  };
>
> +/* Please keep this list alphabetically ordered by vendor/board. */
> +const struct board_info boards_ok[] = {
> /* Verified working boards that don't need write-enables. */
> { "Abit",		"AX8",			"http://www.abit.com.tw/page/en/motherboard/motherboard_detail.php?DEFTITLE=Y..." },
> { "Advantech",		"PCM-5820", 		"http://taiwan.advantech.com.tw/products/Model_Detail.asp?model_id=1-1TGZL8&a..." },
> { "ASI",		"MB-5BLMP",		"http://www.hojerteknik.com/winnet.htm" },
> { "ASUS",		"A8N-E",		"http://www.asus.com.tw/products.aspx?l1=3&l2=15&l3=171&l4=0&..." },
> { "ASUS",		"A8NE-FM/S",		"http://www.hardwareschotte.de/hardware/preise/proid_1266090/preis_ASUS+A8NE-..." },
> { "ASUS",		"A8N-SLI Premium",	"http://www.asus.com.tw/products.aspx?l1=3&l2=15&l3=148&l4=0&..." },
> { "ASUS",		"A8V-E Deluxe",		"http://www.asus.com.tw/products.aspx?l1=3&l2=15&l3=143&l4=0&..." },
> { "ASUS",		"M2A-VM",		"http://www.asus.com.tw/products.aspx?l1=3&l2=101&l3=496&l4=0&..." },
> { "ASUS",		"M2N-E",		"http://www.asus.com/products.aspx?l1=3&l2=101&l3=308&l4=0&mo..." },
> { "ASUS",		"P2B",			"http://www.motherboard.cz/mb/asus/P2B.htm" },
> { "ASUS",		"P2B-F",		"http://www.motherboard.cz/mb/asus/P2B-F.htm" },
> { "ASUS",		"P2B-D",		"ftp://ftp.asus.com.tw/pub/ASUS/mb/slot1/440bx/p2b-d/" },
> { "ASUS",		"P2B-DS",		"ftp://ftp.asus.com.tw/pub/ASUS/mb/slot1/440bx/p2b-ds/" },
> { "ASUS",		"A7V400-MX",		"http://www.asus.com.tw/products.aspx?l1=3&l2=13&l3=63&l4=0&m..." },
> { "ASUS",		"A7V8X-MX",		"http://www.asus.com.tw/products.aspx?l1=3&l2=13&l3=64&l4=0&m..." },
> { "ASUS",		"P4B266",		"http://www.ciao.co.uk/ASUS_Intel_845D_Chipset_P4B266__5409807#productdetail" },
> { "ASUS",		"A8V-E SE",		"http://www.asus.com.tw/products.aspx?l1=3&l2=15&l3=143&l4=0&..." },
> { "ASUS",		"P2L97-S",		"http://www.motherboard.cz/mb/asus/P2L97-S.htm" },
> { "ASUS",		"M2A-MX",		"http://www.asus.com/products.aspx?l1=3&l2=101&l3=583&l4=0&mo..." },
> { "A-Trend",		"ATC-6220",		"http://www.motherboard.cz/mb/atrend/atc6220.htm" },
> { "BCOM",		"WinNET100",		"http://www.coreboot.org/BCOM_WINNET100_Build_Tutorial" },
> { "GIGABYTE",		"GA-6BXC",		"http://www.gigabyte.com.tw/Products/Motherboard/Products_Spec.aspx?ClassValu..." },
> { "GIGABYTE",		"GA-6BXDU",		"http://www.gigabyte.com.tw/Products/Motherboard/Products_Spec.aspx?ProductID..." },
> { "MSI",		"KT4V",			NULL },
> { "MSI",		"MS-7046",		NULL },
> { "MSI",		"MS-7065",		NULL },
> { "MSI",		"MS-7236 (945PL Neo3)",	"http://global.msi.com.tw/index.php?func=prodmbspec&maincat_no=1&cat2..." },
> { "NEC",		"PowerMate 2000",	"http://support.necam.com/mobilesolutions/hardware/Desktops/pm2000/celeron/" },
> { "PC Engines",		"Alix.1c",		"http://pcengines.ch/alix1c.htm" },
> { "PC Engines",		"Alix.2c2",		"http://pcengines.ch/alix2c2.htm" },
> { "PC Engines",		"Alix.2c3",		"http://pcengines.ch/alix2c3.htm" },
> { "PC Engines",		"Alix.3c3",		"http://pcengines.ch/alix3c3.htm" },
> { "RCA",		"RM4100",		"http://www.settoplinux.org" },
> { "Sun",		"Blade x6250",		"http://www.sun.com/servers/blades/x6250/" },
> { "Thomson",		"IP1000",		"http://www.settoplinux.org" },
> { "T-Online",		"S-100",		"http://wiki.freifunk-hannover.de/T-Online_S_100" },
> { "Tyan",		"S1846",		"http://www.tyan.com/archive/products/html/tsunamiatx.html" },
> { "Tyan",		"S2498 (Tomcat K7M)",	"http://www.tyan.com/archive/products/html/tomcatk7m.html" },
> { "Tyan",		"S2881",		"http://www.tyan.com/product_board_detail.aspx?pid=115" },
> { "Tyan",		"S2882",		"http://www.tyan.com/product_board_detail.aspx?pid=121" },
> { "Tyan",		"S2882-D",		"http://www.tyan.com/product_board_detail.aspx?pid=127" },
> { "Tyan",		"S3095",		"http://www.tyan.com/product_board_detail.aspx?pid=181" },
> { "Tyan",		"S5180",		"http://www.tyan.com/product_board_detail.aspx?pid=456" },
> { "Tyan",		"S5191",		"http://www.tyan.com/product_board_detail.aspx?pid=343" },
> { "Tyan",		"S5197",		"http://www.tyan.com/product_board_detail.aspx?pid=349" },
> { "Tyan",		"S5211",		"http://www.tyan.com/product_board_detail.aspx?pid=591" },
> { "Tyan",		"S5211-1U",		"http://www.tyan.com/product_board_detail.aspx?pid=593" },
> { "Tyan",		"S5220",		"http://www.tyan.com/product_board_detail.aspx?pid=597" },
> { "Tyan",		"S5375",		"http://www.tyan.com/product_board_detail.aspx?pid=566" },
> { "Tyan",		"iS5375-1U",		"http://www.tyan.com/product_board_detail.aspx?pid=610" },
> { "Tyan",		"S5376G2NR/S5376WAG2NR","http://www.tyan.com/product_board_detail.aspx?pid=605" },
> { "Tyan",		"S5377",		"http://www.tyan.com/product_board_detail.aspx?pid=601" },
> { "Tyan",		"S5397",		"http://www.tyan.com/product_board_detail.aspx?pid=560" },
> { "VIA",		"EPIA-M",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id..." },
> { "VIA",		"EPIA-MII",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id..." },
> { "VIA",		"EPIA-CN",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id..." },
> { "VIA",		"EPIA-LN",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id..." },
> { "VIA",		"VB700X",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id..." },
> { "VIA",		"NAB74X0",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id..." },
> { "VIA",		"pc2500e",		"http://www.via.com.tw/en/initiatives/empowered/pc2500_mainboard/index.jsp" },
> { NULL,			NULL,			0 },
>
> +};
>
> +/* Please keep this list alphabetically ordered by vendor/board. */
> +const struct board_info boards_no[] = {
> /* Verified non-working boards (for now). */
> { "ASUS",		"A7N8X-E Deluxe",	"http://www.asus.com/products.aspx?l1=3&l2=13&l3=56&l4=0&mode..." },
> { "ASUS",		"MEW-AM",		"ftp://ftp.asus.com.tw/pub/ASUS/mb/sock370/810/mew-am/" },
> { "ASUS",		"MEW-VM",		"http://www.elhvb.com/mboards/OEM/HP/manual/ASUS%20MEW-VM.htm" },
> { "ASUS",		"P3B-F",		"ftp://ftp.asus.com.tw/pub/ASUS/mb/slot1/440bx/p3b-f/" },
> { "Biostar",		"M6TBA",		"ftp://ftp.biostar-usa.com/manuals/M6TBA/" },
> { "FIC",		"VA-502",		"ftp://ftp.fic.com.tw/motherboard/manual/socket7/va-502/" },
> { "MSI",		"MS-7260 (K9N Neo)",	"http://global.msi.com.tw/index.php?func=proddesc&prod_no=255&maincat..." },
> { "PCCHIPS",		"M537DMA33",		"http://motherboards.mbarron.net/models/pcchips/m537dma.htm" },
> { "Soyo",		"SY-5VD",		"http://www.soyo.com/content/Downloads/163/&c=80&p=464&l=English" },
> { "Sun",		"Fire x4540",		"http://www.sun.com/servers/x64/x4540/" },
> { "Sun",		"Fire x4150",		"http://www.sun.com/servers/x64/x4150/" },
> { "Sun",		"Fire x4200",		"http://www.sun.com/servers/entry/x4200/" },
> { "Sun",		"Fire x4600",		"http://www.sun.com/servers/x64/x4600/" },
> { NULL,			NULL,			0 },
>
> +};

I still don't like the fact that files outside wikify.c are polluted
with URLs.
> +void print_supported_boards_helper(const struct board_info *b)
> +{
> int i, j;
> for (i = 0; b[i].vendor != NULL; i++) {
> printf("%s", b[i].vendor);
> for (j = 0; j < 25 - strlen(b[i].vendor); j++)
> 	printf(" ");
> printf("%s", b[i].name);
> for (j = 0; j < 23 - strlen(b[i].name); j++)
> 	printf(" ");
> printf("%s\n", (verbose) ? (b[i].url) ? b[i].url : "" : "");
> }
>
> +}
>
> void print_supported_boards(void)
>  {
> int i;
> int i, j;
> struct board_pciid_enable *b = board_pciid_enables;
> printf("\nSupported mainboards (this list is not exhaustive!):\n\n");
> for (i = 0; board_pciid_enables[i].vendor_name != NULL; i++) {
> if (board_pciid_enables[i].lb_vendor != NULL) {
> 	printf("%s %s (-m %s:%s)\n",
> 	       board_pciid_enables[i].vendor_name,
> 	       board_pciid_enables[i].board_name,
> 	       board_pciid_enables[i].lb_vendor,
> 	       board_pciid_enables[i].lb_part);
> } else {
> 	printf("%s %s (autodetected)\n",
> 	       board_pciid_enables[i].vendor_name,
> 	       board_pciid_enables[i].board_name);
> }
>
> printf("\nSupported boards which need write-enable code:\n\n");
>
> for (i = 0; b[i].vendor_name != NULL; i++) {
> printf("%s", b[i].vendor_name);
> for (j = 0; j < 25 - strlen(b[i].vendor_name); j++)
> 	printf(" ");
> printf("%s", b[i].board_name);
> for (j = 0; j < 25 - strlen(b[i].board_name); j++)
> 	printf(" ");
> if (b[i].lb_vendor != NULL)
> 	printf("(-m %s:%s)\n", b[i].lb_vendor, b[i].lb_part);
> else
> 	printf("(autodetected)\n");
>
> }
>
> printf("\nSupported boards which don't need write-enable code:\n\n");
>
> print_supported_boards_helper(boards_ok);
>
> printf("\nBoards which have been verified to NOT work (yet):\n\n");
>
> print_supported_boards_helper(boards_no);
>
> printf("\nSee also: http://coreboot.org/Flashrom%5Cn");
>
> }

In general, it seems you used #defines for strings quite heavily and
sometimes the criteria for splitting a string into multiple strings
(some of them as #define) is not clear to me. I'd feel better if these
strigns were either placed inside the printfs as literal strings or at
least declared const strings, removing the #defines.

Moving the code to wikify.c helped me a lot when reviewing the patch. It
also allows other coders to ignore the wiki stuff without it ruining
readability of the code in files outside wikify.c (yes, I consider wiki
table code to be horrible). Thank you for keeping the other files clean!

I think the code is committable after one more iteration, but I still
don't know whether to cry when I look at the URLs in arrays and would
appreciate it a lot if you could kill the URLs from arrays outside wikify.c.

Regards,
Carl-Daniel