[coreboot] [PATCH] flashrom: Force -p nic3com=bb:dd.f if multiple cards found

Uwe Hermann uwe at hermann-uwe.de
Fri May 15 01:02:14 CEST 2009


On Fri, May 15, 2009 at 12:16:09AM +0200, Carl-Daniel Hailfinger wrote:
> 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.

Yeah, dropped all but the SATA cards ruik is working on (for now).

 
> > +void get_io_perms(void);
> >  #if defined(__FreeBSD__) || defined(__DragonFly__)
> >  extern int io_fd;
> >  #endif
> 
> Drop this hunk. It's already part of r512.

Done.


> > -		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.

As per IRC discussion the BAR/addr is no longer printed, it's useless,
BDF format is explained better now (I hope).


> > -	return addr;
> > +	return -1;

Changed to 0 as per IRC discussion, the return value is uint32_t.


> > +	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.

Done.

Committed in r513.

 
Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org




More information about the coreboot mailing list