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 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.
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 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 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 flashrom@flashrom.org\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);