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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed May 13 01:05:17 CEST 2009


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 <uwe at hermann-uwe.de>
>
> 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 <uwe at hermann-uwe.de>
> + *
> + * 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 " : "—",
> +		       (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\" |  \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&fMTYPE=Socket%20939&pMODEL_NAME=AX8" },
> +	{ "Advantech",		"PCM-5820", 		"http://taiwan.advantech.com.tw/products/Model_Detail.asp?model_id=1-1TGZL8&BU=ACG&PD=" },
> +	{ "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&model=455&modelmenu=2" },
> +	{ "ASUS",		"A8NE-FM/S",		"http://www.hardwareschotte.de/hardware/preise/proid_1266090/preis_ASUS+A8NE-FM" },
> +	{ "ASUS",		"A8N-SLI Premium",	"http://www.asus.com.tw/products.aspx?l1=3&l2=15&l3=148&l4=0&model=539&modelmenu=1" },
> +	{ "ASUS",		"A8V-E Deluxe",		"http://www.asus.com.tw/products.aspx?l1=3&l2=15&l3=143&l4=0&model=376&modelmenu=1" },
> +	{ "ASUS",		"M2A-VM",		"http://www.asus.com.tw/products.aspx?l1=3&l2=101&l3=496&l4=0&model=1568&modelmenu=1" },
> +	{ "ASUS",		"M2N-E",		"http://www.asus.com/products.aspx?l1=3&l2=101&l3=308&l4=0&model=1181&modelmenu=1" },
> +	{ "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&model=228&modelmenu=1" },
> +	{ "ASUS",		"A7V8X-MX",		"http://www.asus.com.tw/products.aspx?l1=3&l2=13&l3=64&l4=0&model=229&modelmenu=1" },
> +	{ "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&model=576&modelmenu=1" },
> +	{ "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&model=1909&modelmenu=1" },
> +	{ "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?ClassValue=Motherboard&ProductID=1445&ProductName=GA-6BXC" },
> +	{ "GIGABYTE",		"GA-6BXDU",		"http://www.gigabyte.com.tw/Products/Motherboard/Products_Spec.aspx?ProductID=1429" },
> +	{ "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_no=&cat3_no=&prod_no=1173#menu" },
> +	{ "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=81" },
> +	{ "VIA",		"EPIA-MII",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id=202" },
> +	{ "VIA",		"EPIA-CN",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id=400" },
> +	{ "VIA",		"EPIA-LN",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id=473" },
> +	{ "VIA",		"VB700X",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id=490" },
> +	{ "VIA",		"NAB74X0",		"http://www.via.com.tw/en/products/mainboards/motherboards.jsp?motherboard_id=590" },
> +	{ "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&model=217&modelmenu=1" },
> +	{ "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_no=1" },
> +	{ "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\n");
>  }
>  
>
>   

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

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list