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 <[email protected]> >
Acked-by: Carl-Daniel Hailfinger <[email protected]> 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/ -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
