Patchwork Improving pcidev.c

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2013-01-05 02:46:38
Message ID <50E7940E.8070404@gmx.net>
Download mbox | patch
Permalink /patch/3845/
State Superseded
Headers show

Comments

Carl-Daniel Hailfinger - 2013-01-05 02:46:38
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.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>

Patch

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)
@@ -60,11 +60,17 @@ 
 
 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(nics_natsemi, PCI_BASE_ADDRESS_0);
+	if (!dev)
+		return 1;
 
+	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 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)
@@ -237,10 +237,9 @@ 
 // 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);
+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.
  */