Am 04.09.2012 12:14 schrieb Stefan Tauner: > On Tue, 31 Jul 2012 09:27:47 +0200 > Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]> 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. */ -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
