Am 05.01.2013 03:46 schrieb Carl-Daniel Hailfinger: > Am 01.01.2013 19:46 schrieb Carl-Daniel Hailfinger: >> > 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. > Yes, there is still quite a bit of work ahead. > > That said, your PCI init cleanup patch had a tremendous effect on code > readability. Thanks for creating that patch. > > >>> >> 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). > This patch has changed quite a bit since then... it would be great if > you could take another look. > > >>> >> 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. > Very good point. I have changed the argument order, and it really looks > nicer and more consistent > > >>> >> 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!). > Hm yes... to be honest, I want to get rid of the bar parameter in a > followup patch, and supply a validator function instead. That one can > handle everything we might ever need in that direction.
I have a followup patch which adds a validator function and should address all remaining comments raised during the various reviews. Updated patch: Fix a compile failure with disabled internal programmer. We were using a struct before declaring it first, so comapred to the previous patch this is only some code rearranging in programmer.h. 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 1643) +++ flashrom-pcidev_init_do_not_return_bar/ogp_spi.c (Arbeitskopie) @@ -105,6 +105,7 @@ int ogp_spi_init(void) { + struct pci_dev *dev = NULL; char *type; type = extract_programmer_param("rom"); @@ -131,8 +132,11 @@ if (rget_io_perms()) return 1; - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, ogp_spi); + dev = pcidev_init(ogp_spi, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); ogp_spibar = physmap("OGP registers", io_base_addr, 4096); if (register_shutdown(ogp_spi_shutdown, NULL)) Index: flashrom-pcidev_init_do_not_return_bar/drkaiser.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/drkaiser.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/drkaiser.c (Arbeitskopie) @@ -64,17 +64,20 @@ int drkaiser_init(void) { + struct pci_dev *dev = NULL; uint32_t addr; if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - addr = pcidev_init(PCI_BASE_ADDRESS_2, drkaiser_pcidev); + dev = pcidev_init(drkaiser_pcidev, PCI_BASE_ADDRESS_2); + if (!dev) + return 1; + 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 1643) +++ 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, @@ -156,7 +155,6 @@ static int pcidev_shutdown(void *data) { - pcidev_dev = NULL; if (pacc == NULL) { msg_perr("%s: Tried to cleanup an invalid PCI context!\n" "Please report a bug at [email protected]\n", __func__); @@ -181,18 +179,24 @@ return 0; } -uintptr_t pcidev_init(int bar, const struct dev_entry *devs) +/* 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(const struct dev_entry *devs, int bar) { 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; - if(pci_init_common() != 0) - return 1; + if (pci_init_common() != 0) + return NULL; pci_filter_init(pacc, &filter); /* Filter by bb:dd.f (if supplied by the user). */ @@ -200,7 +204,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); @@ -230,8 +234,7 @@ * just those with a valid BAR. */ if ((addr = pcidev_readbar(dev, bar)) != 0) { - curaddr = addr; - pcidev_dev = dev; + found_dev = dev; found++; } } @@ -240,14 +243,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 1643) +++ flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c (Arbeitskopie) @@ -85,14 +85,17 @@ int gfxnvidia_init(void) { + struct pci_dev *dev = NULL; uint32_t reg32; if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia); + dev = pcidev_init(gfx_nvidia, PCI_BASE_ADDRESS_0); + 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); @@ -102,9 +105,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 1643) +++ flashrom-pcidev_init_do_not_return_bar/nicrealtek.c (Arbeitskopie) @@ -59,16 +59,19 @@ int nicrealtek_init(void) { + struct pci_dev *dev = NULL; + if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_realtek); - if (register_shutdown(nicrealtek_shutdown, NULL)) + dev = pcidev_init(nics_realtek, PCI_BASE_ADDRESS_0); + if (!dev) return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + /* Beware, this ignores the vendor ID! */ - switch (pcidev_dev->device_id) { + switch (dev->device_id) { case 0x8139: /* RTL8139 */ case 0x1211: /* SMC 1211TX */ default: @@ -81,6 +84,9 @@ break; } + if (register_shutdown(nicrealtek_shutdown, NULL)) + return 1; + register_par_programmer(&par_programmer_nicrealtek, BUS_PARALLEL); return 0; Index: flashrom-pcidev_init_do_not_return_bar/satamv.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/satamv.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/satamv.c (Arbeitskopie) @@ -81,6 +81,7 @@ */ int satamv_init(void) { + struct pci_dev *dev = NULL; uintptr_t addr; uint32_t tmp; @@ -88,11 +89,11 @@ return 1; /* BAR0 has all internal registers memory mapped. */ - /* 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(satas_mv, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); mv_bar = physmap("Marvell 88SX7042 registers", addr, 0x20000); if (mv_bar == ERROR_PTR) return 1; @@ -143,8 +144,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 1643) +++ flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c (Arbeitskopie) @@ -166,14 +166,17 @@ int nicintel_spi_init(void) { + struct pci_dev *dev = NULL; uint32_t tmp; if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_intel_spi); + dev = pcidev_init(nics_intel_spi, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); nicintel_spibar = physmap("Intel Gigabit NIC w/ SPI flash", io_base_addr, MEMMAP_SIZE); /* Automatic restore of EECD on shutdown is not possible because EECD Index: flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Arbeitskopie) @@ -52,22 +52,19 @@ .chip_writen = fallback_chip_writen, }; -static int nicnatsemi_shutdown(void *data) -{ - pci_cleanup(pacc); - return 0; -} - 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); - - if (register_shutdown(nicnatsemi_shutdown, NULL)) + dev = pcidev_init(nics_natsemi, PCI_BASE_ADDRESS_0); + if (!dev) return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + /* The datasheet shows address lines MA0-MA16 in one place and MA0-MA15 * in another. My NIC has MA16 connected to A16 on the boot ROM socket * so I'm assuming it is accessible. If not then next line wants to be Index: flashrom-pcidev_init_do_not_return_bar/atahpt.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/atahpt.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/atahpt.c (Arbeitskopie) @@ -58,17 +58,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(ata_hpt, PCI_BASE_ADDRESS_4); + if (!dev) + return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_4); + /* 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 1643) +++ flashrom-pcidev_init_do_not_return_bar/nic3com.c (Arbeitskopie) @@ -86,14 +86,19 @@ int nic3com_init(void) { + struct pci_dev *dev = NULL; + if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_3com); + dev = pcidev_init(nics_3com, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; - id = pcidev_dev->device_id; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + id = dev->device_id; + /* 3COM 3C90xB cards need a special fixup. */ if (id == 0x9055 || id == 0x9001 || id == 0x9004 || id == 0x9005 || id == 0x9006 || id == 0x900a || id == 0x905a || id == 0x9058) { Index: flashrom-pcidev_init_do_not_return_bar/satasii.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/satasii.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/satasii.c (Arbeitskopie) @@ -76,21 +76,24 @@ 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(satas_sii, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; - 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 1643) +++ flashrom-pcidev_init_do_not_return_bar/nicintel.c (Arbeitskopie) @@ -68,6 +68,7 @@ int nicintel_init(void) { + struct pci_dev *dev = NULL; uintptr_t addr; /* Needed only for PCI accesses on some platforms. @@ -76,17 +77,17 @@ if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. - * FIXME: BAR2 is not available if the device uses the CardBus function. - */ - addr = pcidev_init(PCI_BASE_ADDRESS_2, nics_intel); + /* FIXME: BAR2 is not available if the device uses the CardBus function. */ + dev = pcidev_init(nics_intel, PCI_BASE_ADDRESS_2); + if (!dev) + return 1; + 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 1643) +++ flashrom-pcidev_init_do_not_return_bar/programmer.h (Arbeitskopie) @@ -160,8 +160,25 @@ unsigned int half_period; }; +#if NEED_PCI == 1 +struct pci_dev; + +/* pcidev.c */ +// FIXME: These need to be local, not global +extern uint32_t io_base_addr; +extern struct pci_access *pacc; +int pci_init_common(void); +uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); +struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar); +/* rpci_write_* are reversible writes. The original PCI config space register + * contents will be restored on shutdown. + */ +int rpci_write_byte(struct pci_dev *dev, int reg, uint8_t data); +int rpci_write_word(struct pci_dev *dev, int reg, uint16_t data); +int rpci_write_long(struct pci_dev *dev, int reg, uint32_t data); +#endif + #if CONFIG_INTERNAL == 1 -struct pci_dev; struct penable { uint16_t vendor_id; uint16_t device_id; @@ -232,23 +249,6 @@ void myusec_calibrate_delay(void); void internal_delay(int usecs); -#if NEED_PCI == 1 -/* pcidev.c */ -// 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; -int pci_init_common(void); -uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); -uintptr_t 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. - */ -int rpci_write_byte(struct pci_dev *dev, int reg, uint8_t data); -int rpci_write_word(struct pci_dev *dev, int reg, uint16_t data); -int rpci_write_long(struct pci_dev *dev, int reg, uint32_t data); -#endif - #if CONFIG_INTERNAL == 1 /* board_enable.c */ int board_parse_parameter(const char *boardstring, const char **vendor, const char **model); -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
