[flashrom] Improving pcidev.c
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Sat Jan 5 18:56:33 CET 2013
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 at 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 at 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 at 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);
--
http://www.hailfinger.org/
More information about the flashrom
mailing list