[coreboot] [PATCH] flashrom: Force -p nic3com=bb:dd.f if multiple cards found
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Fri May 15 00:16:09 CEST 2009
On 14.05.2009 23:49, Uwe Hermann wrote:
> Make the nic3com code check how many supported NICs are found. If we find
> multiple ones, abort with a message to the user, suggesting to use the
>
> flashrom -p nic3com=bb:dd.f
>
> syntax. If exactly one supported NIC is found, use it. If none is found,
> abort with an error.
>
> Print the bb:dd.f numbers for all supported NICs we find, so the user
> doesn't have to poke around in lspci output to find the desired bb:dd.f.
>
> Also, drop one pci_read_long() in favor of using the already existing
> base_addr[0] struct field.
>
> While I'm at it, update the manpage some more to mention and fully document
> the external programmer support we have (or will have soon).
>
> The patch is tested on hardware:
>
> $ flashrom -p nic3com
> flashrom v0.9.0-r511
> Found NIC "3COM 3C905C: EtherLink 10/100 PCI (TX)" (10b7:9200, 05:04.0, 0xa800)
> Found NIC "3COM 3C905C: EtherLink 10/100 PCI (TX)" (10b7:9200, 05:03.0, 0xa400)
> Error: Multiple supported NICs found. Please use 'flashrom -p nic3com=bb:dd.f'.
>
> $ flashrom -p nic3com=05:04.0
> flashrom v0.9.0-r511
> Found NIC "3COM 3C905C: EtherLink 10/100 PCI (TX)" (10b7:9200, 05:04.0, 0xa800)
> Calibrating delay loop... OK.
> Found chip "Atmel AT49BV512" (64 KB) at physical address 0xffff0000.
> No operations were specified.
>
> Signed-off-by: Uwe Hermann <uwe at hermann-uwe.de>
>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
with the changes below.
> Index: flashrom.8
> ===================================================================
> --- flashrom.8 (revision 512)
> +++ flashrom.8 (working copy)
> @@ -1,13 +1,17 @@
> -.TH FLASHROM 8 "April 11, 2009"
> +.TH FLASHROM 8 "May 14, 2009"
> .SH NAME
> -flashrom \- read, write, verify and erase BIOS/ROM/flash chips
> +flashrom \- detect, read, write, verify and erase flash chips
> .SH SYNOPSIS
> -.B flashrom \fR[\fB\-rwvEVfLhR\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]
> +.B flashrom \fR[\fB\-EVfLhR\fR] [\fB\-r\fR file] [\fB\-w\fR file] [\fB\-v\fR file]
> + [\fB\-c\fR chipname] [\fB\-s\fR addr] [\fB\-e\fR addr] [\fB\-m\fR [vendor:]part]
> + [\fB\-l\fR file] [\fB\-i\fR image] [\fB\-p\fR programmer] [file]
> .SH DESCRIPTION
> .B flashrom
> -is a utility for reading, writing, verifying and erasing flash ROM chips.
> -It's often used to flash BIOS/coreboot/firmware images.
> +is a utility for detecting, reading, writing, verifying and erasing flash ROM
> +chips. It's often used to flash BIOS/EFI/coreboot/firmware images in-system
> +using a supported mainboard, but it also supports (or will support) flashing
> +of network cards (NICs), SATA controller cards, graphics cards, CD-ROM/DVD
>
Please remove graphics cards and CD-ROM/DVD drives unless you have
matching docs. I know that ATI graphics cards don't have sufficient
documentation for flashing and I just checked with the contact I had who
extended flashrom a bit. He was only able to read from ATI graphics
cards, and even the reads were unreliable. ID and other stuff was a no-go.
We can always raise expectations later if we actually see this stuff on
the horizon.
> +drives, and other external devices which can program flash chips.
> .PP
> It supports a wide range of DIP32, PLCC32, DIP8, SO8/SOIC8, TSOP32, and
> TSOP40 chips, which use various protocols such as LPC, FWH, parallel flash,
> @@ -126,11 +130,24 @@
> .B "\-p, \-\-programmer <name>"
> Specify the programmer device. Currently supported are:
> .sp
> -.BR " internal" " (default, for in-system flashing in the mainboard)"
> -.br
> -.BR " nic3com" " (for flash ROMs on 3COM network cards)"
> -.br
> -.BR " dummy" " (just prints all operations and accesses)"
> +.BR "* internal" " (default, for in-system flashing in the mainboard)"
> +.sp
> +.BR "* nic3com" " (for flash ROMs on 3COM network cards)"
> +.sp
> +If you have multiple supported NICs in your system, you must use
> +.B "flashrom -p nic3com=bb:dd.f"
> +to explicitly select one of them, where
> +.B bb
> +is the PCI bus number,
> +.B dd
> +is the PCI device number, and
> +.B f
> +is the PCI function number of the desired NIC.
> +.sp
> +Example:
> +.B "flashrom -p nic3com=05:04.0"
> +.sp
> +.BR "* dummy" " (just prints all operations and accesses)"
> .TP
> .B "\-h, \-\-help"
> Show a help text and exit.
> Index: flash.h
> ===================================================================
> --- flash.h (revision 512)
> +++ flash.h (working copy)
> @@ -594,6 +594,7 @@
> uint8_t internal_chip_readb(const volatile void *addr);
> uint16_t internal_chip_readw(const volatile void *addr);
> uint32_t internal_chip_readl(const volatile void *addr);
> +void get_io_perms(void);
> #if defined(__FreeBSD__) || defined(__DragonFly__)
> extern int io_fd;
> #endif
>
Drop this hunk. It's already part of r512.
> Index: nic3com.c
> ===================================================================
> --- nic3com.c (revision 512)
> +++ nic3com.c (working copy)
> @@ -67,18 +67,18 @@
>
> uint32_t nic3com_validate(struct pci_dev *dev)
> {
> - int i = 0;
> - uint32_t addr = -1;
> + int i;
> + uint32_t addr;
>
> for (i = 0; nics[i].device_name != NULL; i++) {
> if (dev->device_id != nics[i].device_id)
> continue;
>
> - addr = pci_read_long(dev, PCI_IO_BASE_ADDRESS) & ~0x03;
> + addr = (uint32_t)(dev->base_addr[0] & ~0x03);
>
> - printf("Found NIC \"3COM %s\" (%04x:%04x), addr = 0x%x\n",
> - nics[i].device_name, PCI_VENDOR_ID_3COM,
> - nics[i].device_id, addr);
> + printf("Found NIC \"3COM %s\" (%04x:%04x, %02x:%02x.%x, "
> + "0x%x)\n", nics[i].device_name, dev->vendor_id,
> + dev->device_id, dev->bus, dev->dev, dev->func, addr);
>
Can you please extend this message to tell the user what is what? I had
to read the code to understand what the third value (BAR) was.
>
> if (nics[i].status == NT) {
> printf("===\nThis NIC is UNTESTED. Please email a "
> @@ -90,41 +90,47 @@
> return addr;
> }
>
> - return addr;
> + return -1;
> }
>
> int nic3com_init(void)
> {
> struct pci_dev *dev;
> char *msg = NULL;
> + int found = 0;
>
> get_io_perms();
>
> pacc = pci_alloc(); /* Get the pci_access structure */
> pci_init(pacc); /* Initialize the PCI library */
> pci_scan_bus(pacc); /* We want to get the list of devices */
> + pci_filter_init(pacc, &filter);
>
> + /* Filter by vendor, or bb:dd.f if supplied by the user. */
> if (nic_pcidev != NULL) {
> - pci_filter_init(pacc, &filter);
> -
> if ((msg = pci_filter_parse_slot(&filter, nic_pcidev))) {
> fprintf(stderr, "Error: %s\n", msg);
> exit(1);
> }
> + } else {
> + filter.vendor = PCI_VENDOR_ID_3COM;
> }
>
I'm unhappy with the new logic here. We either filter by bus:dev.fn or
by vendor ID. The vendor ID filter should always be active. Otherwise,
an incorrectly specified bdf can match a non-3com card by accident as
long as the device ID matches.
Please move the content of the else branch out of the if clause and run
it unconditionally.
>
> - if (!filter.vendor && !filter.device) {
> - pci_filter_init(pacc, &filter);
> - filter.vendor = PCI_VENDOR_ID_3COM;
> + for (dev = pacc->devices; dev; dev = dev->next) {
> + if (pci_filter_match(&filter, dev)) {
> + if ((io_base_addr = nic3com_validate(dev)) != -1)
> + found++;
> + }
> }
>
> - dev = pci_dev_find_filter(filter);
> -
> - if (dev && (dev->vendor_id == PCI_VENDOR_ID_3COM))
> - io_base_addr = nic3com_validate(dev);
> - else {
> + /* Only continue if exactly one supported NIC has been found. */
> + if (found == 0) {
> fprintf(stderr, "Error: No supported 3COM NIC found.\n");
> exit(1);
> + } else if (found > 1) {
> + fprintf(stderr, "Error: Multiple supported NICs found. "
> + "Please use 'flashrom -p nic3com=bb:dd.f'.\n");
> + exit(1);
> }
>
> /*
>
>
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list