[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