Am 04.09.2012 12:14 schrieb Stefan Tauner:
On Tue, 31 Jul 2012 09:27:47 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Ping?
IMHO this patch fixes a few structural problems, and although it probably isn't the final desired result, it is a step in the right direction.
After my short adventure into trying to understand the PCI code while refining the atavia patch by Jonathan, i have to agree 100% with Michael's first mail regarding the old code base.
Something that hasn't been mentioned (or i have missed) is that there are some drivers that don't need a BAR at all because they work by accessing the configuration space only. atavia is one case (at least i think so). and satasii is even more special as it needs different BARs for different PCI IDs, but you are aware of that afaics.
I still don't have a complete overview on how all pcidev_init callers work, but the current patch seems to improve things and hence should go in ASAP (you can read that as an ACK if you think it is safe).
Since you touch all pcidev_init calls in this patch, it would be great if you could switch the parameters though. The PCI dev table should be first since it is the most important argument and the bar should IMHO even be optional in the future or some kind of flag/mask as you discussed... that argument apparently confused not only me but also Jonathan and Idwer in the past. A comment explaining the parameters would certainly improve the situation too (e.g. mention that the bar parameter is not the number of the BAR!).
OK, so I did address only some of the comments, but I wanted to send the current version out there (and it kills a boatload of exit(1) as well).
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-pcidev_init_do_not_return_bar/ogp_spi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/ogp_spi.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/ogp_spi.c (Arbeitskopie) @@ -107,6 +107,7 @@
int ogp_spi_init(void) { + struct pci_dev *dev = NULL; char *type;
type = extract_programmer_param("rom"); @@ -133,7 +134,8 @@ if (rget_io_perms()) return 1;
- io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, ogp_spi); + dev = pcidev_init(PCI_BASE_ADDRESS_0, ogp_spi); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
ogp_spibar = physmap("OGP registers", io_base_addr, 4096);
Index: flashrom-pcidev_init_do_not_return_bar/drkaiser.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/drkaiser.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/drkaiser.c (Arbeitskopie) @@ -66,16 +66,17 @@
int drkaiser_init(void) { + struct pci_dev *dev = NULL; uint32_t addr;
if (rget_io_perms()) return 1;
- addr = pcidev_init(PCI_BASE_ADDRESS_2, drkaiser_pcidev); + dev = pcidev_init(PCI_BASE_ADDRESS_2, drkaiser_pcidev); + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_2);
/* Write magic register to enable flash write. */ - rpci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, - PCI_MAGIC_DRKAISER_VALUE); + rpci_write_word(dev, PCI_MAGIC_DRKAISER_ADDR, PCI_MAGIC_DRKAISER_VALUE);
/* Map 128kB flash memory window. */ drkaiser_bar = physmap("Dr. Kaiser PC-Waechter flash memory", Index: flashrom-pcidev_init_do_not_return_bar/pcidev.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/pcidev.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/pcidev.c (Arbeitskopie) @@ -27,7 +27,6 @@
uint32_t io_base_addr; struct pci_access *pacc; -struct pci_dev *pcidev_dev = NULL;
enum pci_bartype { TYPE_MEMBAR, @@ -154,18 +153,31 @@ return (uintptr_t)addr; }
-uintptr_t pcidev_init(int bar, const struct dev_entry *devs) +int pci_cleanup_wrapper(void *pa) { + pci_cleanup(pa); + return 0; +} + +/* pcidev_init gets an array of allowed PCI device IDs and returns a pointer to struct pci_dev iff exactly one + * match was found. If the "pci=bb:dd.f" programmer parameter was specified, a match is only considered if it + * also matches the specified bus:device.function. + * For convenience, this function also registers its own undo handlers. + */ +struct pci_dev *pcidev_init(int bar, const struct dev_entry *devs) +{ struct pci_dev *dev; + struct pci_dev *found_dev = NULL; struct pci_filter filter; char *pcidev_bdf; char *msg = NULL; int found = 0; int i; - uintptr_t addr = 0, curaddr = 0; + uintptr_t addr = 0;
pacc = pci_alloc(); /* Get the pci_access structure */ pci_init(pacc); /* Initialize the PCI library */ + register_shutdown(pci_cleanup_wrapper, pacc); pci_scan_bus(pacc); /* We want to get the list of devices */ pci_filter_init(pacc, &filter);
@@ -174,7 +186,7 @@ if (pcidev_bdf != NULL) { if ((msg = pci_filter_parse_slot(&filter, pcidev_bdf))) { msg_perr("Error: %s\n", msg); - exit(1); + return NULL; } } free(pcidev_bdf); @@ -204,8 +216,7 @@ * just those with a valid BAR. */ if ((addr = pcidev_readbar(dev, bar)) != 0) { - curaddr = addr; - pcidev_dev = dev; + found_dev = dev; found++; } } @@ -214,14 +225,14 @@ /* Only continue if exactly one supported PCI dev has been found. */ if (found == 0) { msg_perr("Error: No supported PCI device found.\n"); - exit(1); + return NULL; } else if (found > 1) { msg_perr("Error: Multiple supported PCI devices found. Use 'flashrom -p xxxx:pci=bb:dd.f' \n" "to explicitly select the card with the given BDF (PCI bus, device, function).\n"); - exit(1); + return NULL; }
- return curaddr; + return found_dev; }
enum pci_write_type { Index: flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c (Arbeitskopie) @@ -89,12 +89,17 @@
int gfxnvidia_init(void) { + struct pci_dev *dev = NULL; uint32_t reg32;
if (rget_io_perms()) return 1;
- io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia); + dev = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia); + if (!dev) { + return 1; + } + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
io_base_addr += 0x300000; msg_pinfo("Detected NVIDIA I/O base address: 0x%x.\n", io_base_addr); @@ -106,9 +111,9 @@ return 1;
/* Allow access to flash interface (will disable screen). */ - reg32 = pci_read_long(pcidev_dev, 0x50); + reg32 = pci_read_long(dev, 0x50); reg32 &= ~(1 << 0); - rpci_write_long(pcidev_dev, 0x50, reg32); + rpci_write_long(dev, 0x50, reg32);
/* Write/erase doesn't work. */ programmer_may_write = 0; Index: flashrom-pcidev_init_do_not_return_bar/nicrealtek.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicrealtek.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/nicrealtek.c (Arbeitskopie) @@ -60,16 +60,19 @@
int nicrealtek_init(void) { + struct pci_dev *dev = NULL; + if (rget_io_perms()) return 1;
- io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_realtek); + dev = pcidev_init(PCI_BASE_ADDRESS_0, nics_realtek); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
if (register_shutdown(nicrealtek_shutdown, NULL)) return 1;
/* Beware, this ignores the vendor ID! */ - switch (pcidev_dev->device_id) { + switch (dev->device_id) { case 0x8139: /* RTL8139 */ case 0x1211: /* SMC 1211TX */ default: Index: flashrom-pcidev_init_do_not_return_bar/satamv.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/satamv.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/satamv.c (Arbeitskopie) @@ -82,6 +82,7 @@ */ int satamv_init(void) { + struct pci_dev *dev = NULL; uintptr_t addr; uint32_t tmp;
@@ -92,7 +93,8 @@ /* No need to check for errors, pcidev_init() will not return in case * of errors. */ - addr = pcidev_init(PCI_BASE_ADDRESS_0, satas_mv); + dev = pcidev_init(PCI_BASE_ADDRESS_0, satas_mv); + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
mv_bar = physmap("Marvell 88SX7042 registers", addr, 0x20000); if (mv_bar == ERROR_PTR) @@ -144,8 +146,7 @@ pci_rmmio_writel(tmp, mv_bar + GPIO_PORT_CONTROL);
/* Get I/O BAR location. */ - tmp = pci_read_long(pcidev_dev, PCI_BASE_ADDRESS_2) & - PCI_BASE_ADDRESS_IO_MASK; + tmp = pcidev_readbar(dev, PCI_BASE_ADDRESS_2); /* Truncate to reachable range. * FIXME: Check if the I/O BAR is actually reachable. * This is an arch specific check. Index: flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c (Arbeitskopie) @@ -167,12 +167,14 @@
int nicintel_spi_init(void) { + struct pci_dev *dev = NULL; uint32_t tmp;
if (rget_io_perms()) return 1;
- io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_intel_spi); + dev = pcidev_init(PCI_BASE_ADDRESS_0, nics_intel_spi); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
nicintel_spibar = physmap("Intel Gigabit NIC w/ SPI flash", io_base_addr, MEMMAP_SIZE); Index: flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Arbeitskopie) @@ -60,10 +60,13 @@
int nicnatsemi_init(void) { + struct pci_dev *dev = NULL; + if (rget_io_perms()) return 1;
- io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_natsemi); + dev = pcidev_init(PCI_BASE_ADDRESS_0, nics_natsemi); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
if (register_shutdown(nicnatsemi_shutdown, NULL)) return 1; Index: flashrom-pcidev_init_do_not_return_bar/atahpt.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/atahpt.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/atahpt.c (Arbeitskopie) @@ -65,20 +65,22 @@
int atahpt_init(void) { + struct pci_dev *dev = NULL; uint32_t reg32;
if (rget_io_perms()) return 1;
- io_base_addr = pcidev_init(PCI_BASE_ADDRESS_4, ata_hpt); + dev = pcidev_init(PCI_BASE_ADDRESS_4, ata_hpt); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_4);
if (register_shutdown(atahpt_shutdown, NULL)) return 1;
/* Enable flash access. */ - reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS); + reg32 = pci_read_long(dev, REG_FLASH_ACCESS); reg32 |= (1 << 24); - rpci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32); + rpci_write_long(dev, REG_FLASH_ACCESS, reg32);
register_par_programmer(&par_programmer_atahpt, BUS_PARALLEL);
Index: flashrom-pcidev_init_do_not_return_bar/nic3com.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nic3com.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/nic3com.c (Arbeitskopie) @@ -87,12 +87,15 @@
int nic3com_init(void) { + struct pci_dev *dev = NULL; + if (rget_io_perms()) return 1;
- io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_3com); + dev = pcidev_init(PCI_BASE_ADDRESS_0, nics_3com); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
- id = pcidev_dev->device_id; + id = dev->device_id;
/* 3COM 3C90xB cards need a special fixup. */ if (id == 0x9055 || id == 0x9001 || id == 0x9004 || id == 0x9005 Index: flashrom-pcidev_init_do_not_return_bar/satasii.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/satasii.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/satasii.c (Arbeitskopie) @@ -77,21 +77,22 @@
int satasii_init(void) { + struct pci_dev *dev = NULL; uint32_t addr; uint16_t reg_offset;
if (rget_io_perms()) return 1;
- pcidev_init(PCI_BASE_ADDRESS_0, satas_sii); + dev = pcidev_init(PCI_BASE_ADDRESS_0, satas_sii);
- id = pcidev_dev->device_id; + id = dev->device_id;
if ((id == 0x3132) || (id == 0x3124)) { - addr = pci_read_long(pcidev_dev, PCI_BASE_ADDRESS_0) & ~0x07; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); reg_offset = 0x70; } else { - addr = pci_read_long(pcidev_dev, PCI_BASE_ADDRESS_5) & ~0x07; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_5); reg_offset = 0x50; }
Index: flashrom-pcidev_init_do_not_return_bar/nicintel.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicintel.c (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/nicintel.c (Arbeitskopie) @@ -69,6 +69,7 @@
int nicintel_init(void) { + struct pci_dev *dev = NULL; uintptr_t addr;
/* Needed only for PCI accesses on some platforms. @@ -81,14 +82,14 @@ * of errors. * FIXME: BAR2 is not available if the device uses the CardBus function. */ - addr = pcidev_init(PCI_BASE_ADDRESS_2, nics_intel); + dev = pcidev_init(PCI_BASE_ADDRESS_2, nics_intel);
+ addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_2); nicintel_bar = physmap("Intel NIC flash", addr, NICINTEL_MEMMAP_SIZE); if (nicintel_bar == ERROR_PTR) goto error_out_unmap;
- /* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */ - addr = pcidev_readbar(pcidev_dev, PCI_BASE_ADDRESS_0); + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); /* FIXME: This is not an aligned mapping. Use 4k? */ nicintel_control_bar = physmap("Intel NIC control/status reg", addr, NICINTEL_CONTROL_MEMMAP_SIZE); Index: flashrom-pcidev_init_do_not_return_bar/programmer.h =================================================================== --- flashrom-pcidev_init_do_not_return_bar/programmer.h (Revision 1640) +++ flashrom-pcidev_init_do_not_return_bar/programmer.h (Arbeitskopie) @@ -237,9 +237,8 @@ // FIXME: These need to be local, not global extern uint32_t io_base_addr; extern struct pci_access *pacc; -extern struct pci_dev *pcidev_dev; uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); -uintptr_t pcidev_init(int bar, const struct dev_entry *devs); +struct pci_dev *pcidev_init(int bar, const struct dev_entry *devs); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. */