Hi,
Here are the logs: # ./flashrom -p gfxnvidia -VVV flashrom v0.9.5.2-r1546 on Linux 3.0.0-19-generic-pae (i686) flashrom is free software, get the source code at http://www.flashrom.org
flashrom was built with libpci 3.1.7, GCC 4.6.1, little endian Command line (3 args): ./flashrom -p gfxnvidia -VVV Calibrating delay loop... OS timer resolution is 1 usecs, 1503M loops per second, 10 myus = 10 us, 100 myus = 100 us, 1000 myus = 1000 us, 10000 myus = 10007 us, 4 myus = 4 us, OK. Initializing gfxnvidia programmer Found "NVIDIA RIVA TNT2 Ultra" (168c:0029, BDF 03:06.0). PCI header type 0x00 Requested BAR is MEM, 32bit, not prefetchable === This PCI device is UNTESTED. Please report the 'flashrom -p xxxx' output to flashrom@flashrom.org if it works for you. Please add the name of your PCI device to the subject. Thank you for your help! === Detected NVIDIA I/O base address: 0xd8600000. The following protocols are supported: Parallel. Probing for AMD Am29F010A/B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F002(N)BB, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F002(N)BT, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F016D, 2048 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F080B, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV001BB, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV001BT, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV002BB, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV002BT, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV004BB, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV004BT, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV008BB, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV008BT, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV081B, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMIC A29002B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMIC A29002T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMIC A29040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT29C512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT29C010A, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT29C020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT29C040A, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49BV512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49F002(N), 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49F002(N)T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49F020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49F040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Catalyst CAT28F512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Bright BM29F040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for EMST F49B002UA, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Eon EN29F010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Eon EN29F002(A)(N)B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Eon EN29F002(A)(N)T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Eon EN29LV640B, 8192 kB: probe_en29lv640b: id1 0xffff, id2 0x00ff Probing for Fujitsu MBM29F004BC, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Fujitsu MBM29F004TC, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Fujitsu MBM29F400BC, 512 kB: probe_m29f400bt: id1 0xff, id2 0xff Probing for Fujitsu MBM29F400TC, 512 kB: probe_m29f400bt: id1 0xff, id2 0xff Probing for Hyundai HY29F002T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Hyundai HY29F002B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Hyundai HY29F040A, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F001BN/BX-B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F001BN/BX-T, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F002BC/BL/BV/BX-T, 256 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F008S3/S5/SC, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F004B5/BE/BV/BX-B, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F004B5/BE/BV/BX-T, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F400BV/BX/CE/CV-B, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F400BV/BX/CE/CV-T, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F001B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F001T, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F002(N)B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F002(N)T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29LV040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29C51000B, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29C51000T, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29C51400B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29C51400T, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29LC51000, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29LC51001, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29LC51002, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm29F002T, 256 kB: Chip lacks correct probe timing information, using default 10mS/40uS. probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm29F002B, 256 kB: Chip lacks correct probe timing information, using default 10mS/40uS. probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm39LV010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm39LV020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm39LV040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm39LV512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Sharp LH28F008BJT-BTLZ1, 1024 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST28SF040A, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST29EE010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST29LE010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST29EE020A, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST29LE020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39SF512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39SF010A, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39SF020A, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39SF040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF080, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29F002B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29F002T/NT, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29F040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29F400BB, 512 kB: probe_m29f400bt: id1 0xff, id2 0xff Probing for ST M29F400BT, 512 kB: probe_m29f400bt: id1 0xff, id2 0xff Probing for ST M29W010B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29W040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29W512B, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51001B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51001T, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51002B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51002T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51004B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51004T, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {S,V}29C31004B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {S,V}29C31004T, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for TI TMS29F002RB, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for TI TMS29F002RT, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W29C010(M)/W29C011A/W29EE011/W29EE012-old, 128 kB: Old Winbond W29* probe method disabled because the probing sequence puts the AMIC A49LF040A in a funky state. Use 'flashrom -c W29C010(M)/W29C011A/W29EE011/W29EE012-old' if you have a board with such a chip. Probing for Winbond W29C010(M)/W29C011A/W29EE011/W29EE012, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W29C020(C)/W29C022, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W29C040/P, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W39L040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W49F002U/N, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W49F020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content No EEPROM/flash device found. Note: flashrom can never write if the flash chip isn't found automatically. Restoring PCI config space for 03:06:0 reg 0x50
# lspci -n 00:00.0 0600: 1022:9601 00:01.0 0604: 1022:9602 00:0a.0 0604: 1022:9609 00:11.0 0106: 1002:4390 00:12.0 0c03: 1002:4397 00:12.1 0c03: 1002:4398 00:12.2 0c03: 1002:4396 00:13.0 0c03: 1002:4397 00:13.1 0c03: 1002:4398 00:13.2 0c03: 1002:4396 00:14.0 0c05: 1002:4385 (rev 3c) 00:14.1 0101: 1002:439c 00:14.2 0403: 1002:4383 00:14.3 0601: 1002:439d 00:14.4 0604: 1002:4384 00:14.5 0c03: 1002:4399 00:18.0 0600: 1022:1200 00:18.1 0600: 1022:1201 00:18.2 0600: 1022:1202 00:18.3 0600: 1022:1203 00:18.4 0600: 1022:1204 01:05.0 0300: 1002:9710 01:05.1 0403: 1002:970f 02:00.0 0200: 10ec:8168 (rev 03) 03:06.0 0280: 168c:0029 (rev 01)
# lspci 00:00.0 Host bridge: Advanced Micro Devices [AMD] RS880 Host Bridge 00:01.0 PCI bridge: Advanced Micro Devices [AMD] RS780/RS880 PCI to PCI bridge (int gfx) 00:0a.0 PCI bridge: Advanced Micro Devices [AMD] RS780/RS880 PCI to PCI bridge (PCIE port 5) 00:11.0 SATA controller: ATI Technologies Inc SB7x0/SB8x0/SB9x0 SATA Controller [IDE mode] 00:12.0 USB Controller: ATI Technologies Inc SB7x0/SB8x0/SB9x0 USB OHCI0 Controller 00:12.1 USB Controller: ATI Technologies Inc SB7x0 USB OHCI1 Controller 00:12.2 USB Controller: ATI Technologies Inc SB7x0/SB8x0/SB9x0 USB EHCI Controller 00:13.0 USB Controller: ATI Technologies Inc SB7x0/SB8x0/SB9x0 USB OHCI0 Controller 00:13.1 USB Controller: ATI Technologies Inc SB7x0 USB OHCI1 Controller 00:13.2 USB Controller: ATI Technologies Inc SB7x0/SB8x0/SB9x0 USB EHCI Controller 00:14.0 SMBus: ATI Technologies Inc SBx00 SMBus Controller (rev 3c) 00:14.1 IDE interface: ATI Technologies Inc SB7x0/SB8x0/SB9x0 IDE Controller 00:14.2 Audio device: ATI Technologies Inc SBx00 Azalia (Intel HDA) 00:14.3 ISA bridge: ATI Technologies Inc SB7x0/SB8x0/SB9x0 LPC host controller 00:14.4 PCI bridge: ATI Technologies Inc SBx00 PCI to PCI Bridge 00:14.5 USB Controller: ATI Technologies Inc SB7x0/SB8x0/SB9x0 USB OHCI2 Controller 00:18.0 Host bridge: Advanced Micro Devices [AMD] Family 10h Processor HyperTransport Configuration 00:18.1 Host bridge: Advanced Micro Devices [AMD] Family 10h Processor Address Map 00:18.2 Host bridge: Advanced Micro Devices [AMD] Family 10h Processor DRAM Controller 00:18.3 Host bridge: Advanced Micro Devices [AMD] Family 10h Processor Miscellaneous Control 00:18.4 Host bridge: Advanced Micro Devices [AMD] Family 10h Processor Link Control 01:05.0 VGA compatible controller: ATI Technologies Inc RS880 [Radeon HD 4200] 01:05.1 Audio device: ATI Technologies Inc RS880 Audio Device [Radeon HD 4200] 02:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller (rev 03) 03:06.0 Network controller: Atheros Communications Inc. AR922X Wireless Network Adapter (rev 01)
Denis.
Hi Denis,
Am 26.06.2012 01:53 schrieb Denis 'GNUtoo' Carikli:
# ./flashrom -p gfxnvidia -VVV flashrom v0.9.5.2-r1546 on Linux 3.0.0-19-generic-pae (i686) flashrom was built with libpci 3.1.7, GCC 4.6.1, little endian Command line (3 args): ./flashrom -p gfxnvidia -VVV Calibrating delay loop... OK. Initializing gfxnvidia programmer Found "NVIDIA RIVA TNT2 Ultra" (168c:0029, BDF 03:06.0). [...]
This should not have happened. Apparently the PCI code (either in flashrom, or libpci) ignores the vendor, and only matches the device in this case.
Ah yes. pcidev.c:pcidev_validate() only checks device_id match, not vendor_id match. Ouch!
Thanks for reporting this bug!
# lspci -n [...] 03:06.0 0280: 168c:0029 (rev 01)
# lspci [...] 03:06.0 Network controller: Atheros Communications Inc. AR922X Wireless Network Adapter (rev 01)
Can you confirm that no Nvidia card is in that machine? I want to make 100% sure that there are not two PCI cards fighting with each other.
Regards, Carl-Daniel
On Wed, 2012-06-27 at 09:05 +0200, Carl-Daniel Hailfinger wrote:
Hi Denis,
Am 26.06.2012 01:53 schrieb Denis 'GNUtoo' Carikli:
# ./flashrom -p gfxnvidia -VVV flashrom v0.9.5.2-r1546 on Linux 3.0.0-19-generic-pae (i686) flashrom was built with libpci 3.1.7, GCC 4.6.1, little endian Command line (3 args): ./flashrom -p gfxnvidia -VVV Calibrating delay loop... OK. Initializing gfxnvidia programmer Found "NVIDIA RIVA TNT2 Ultra" (168c:0029, BDF 03:06.0). [...]
This should not have happened. Apparently the PCI code (either in flashrom, or libpci) ignores the vendor, and only matches the device in this case.
Ah yes. pcidev.c:pcidev_validate() only checks device_id match, not vendor_id match. Ouch!
Thanks for reporting this bug!
# lspci -n [...] 03:06.0 0280: 168c:0029 (rev 01)
# lspci [...] 03:06.0 Network controller: Atheros Communications Inc. AR922X Wireless Network Adapter (rev 01)
Can you confirm that no Nvidia card is in that machine? I want to make 100% sure that there are not two PCI cards fighting with each other.
I removed the nvidia card that was present(but not detected by coreboot) and I still get the same: # ./flashrom -p gfxnvidia -V flashrom v0.9.5.2-r1546 on Linux 3.0.0-19-generic-pae (i686) flashrom is free software, get the source code at http://www.flashrom.org
flashrom was built with libpci 3.1.7, GCC 4.6.1, little endian Command line (3 args): ./flashrom -p gfxnvidia -V Calibrating delay loop... OS timer resolution is 1 usecs, 1502M loops per second, 10 myus = 11 us, 100 myus = 100 us, 1000 myus = 1002 us, 10000 myus = 9995 us, 4 myus = 4 us, OK. Initializing gfxnvidia programmer Found "NVIDIA RIVA TNT2 Ultra" (168c:0029, BDF 03:06.0). Requested BAR is MEM, 32bit, not prefetchable === This PCI device is UNTESTED. Please report the 'flashrom -p xxxx' output to flashrom@flashrom.org if it works for you. Please add the name of your PCI device to the subject. Thank you for your help! === Detected NVIDIA I/O base address: 0xd8600000. The following protocols are supported: Parallel. Probing for AMD Am29F010A/B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F002(N)BB, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F002(N)BT, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F016D, 2048 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29F080B, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV001BB, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV001BT, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV002BB, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV002BT, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV004BB, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV004BT, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV008BB, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV008BT, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMD Am29LV081B, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMIC A29002B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMIC A29002T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for AMIC A29040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT29C512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT29C010A, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT29C020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT29C040A, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49BV512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49F002(N), 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49F002(N)T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49F020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Atmel AT49F040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Catalyst CAT28F512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Bright BM29F040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for EMST F49B002UA, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Eon EN29F010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Eon EN29F002(A)(N)B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Eon EN29F002(A)(N)T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Eon EN29LV640B, 8192 kB: probe_en29lv640b: id1 0xffff, id2 0x00ff Probing for Fujitsu MBM29F004BC, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Fujitsu MBM29F004TC, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Fujitsu MBM29F400BC, 512 kB: probe_m29f400bt: id1 0xff, id2 0xff Probing for Fujitsu MBM29F400TC, 512 kB: probe_m29f400bt: id1 0xff, id2 0xff Probing for Hyundai HY29F002T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Hyundai HY29F002B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Hyundai HY29F040A, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F001BN/BX-B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F001BN/BX-T, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F002BC/BL/BV/BX-T, 256 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F008S3/S5/SC, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F004B5/BE/BV/BX-B, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F004B5/BE/BV/BX-T, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F400BV/BX/CE/CV-B, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Intel 28F400BV/BX/CE/CV-T, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F001B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F001T, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F002(N)B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F002(N)T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29F040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Macronix MX29LV040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29C51000B, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29C51000T, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29C51400B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29C51400T, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29LC51000, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29LC51001, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for MoselVitelic V29LC51002, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm29F002T, 256 kB: Chip lacks correct probe timing information, using default 10mS/40uS. probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm29F002B, 256 kB: Chip lacks correct probe timing information, using default 10mS/40uS. probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm39LV010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm39LV020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm39LV040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for PMC Pm39LV512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Sharp LH28F008BJT-BTLZ1, 1024 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST28SF040A, 512 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST29EE010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST29LE010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST29EE020A, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST29LE020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39SF512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39SF010A, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39SF020A, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39SF040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF512, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF010, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SST SST39VF080, 1024 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29F002B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29F002T/NT, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29F040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29F400BB, 512 kB: probe_m29f400bt: id1 0xff, id2 0xff Probing for ST M29F400BT, 512 kB: probe_m29f400bt: id1 0xff, id2 0xff Probing for ST M29W010B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29W040B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for ST M29W512B, 64 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51001B, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51001T, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51002B, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51002T, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51004B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {F,S,V}29C51004T, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {S,V}29C31004B, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for SyncMOS/MoselVitelic {S,V}29C31004T, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for TI TMS29F002RB, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for TI TMS29F002RT, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W29C010(M)/W29C011A/W29EE011/W29EE012-old, 128 kB: Old Winbond W29* probe method disabled because the probing sequence puts the AMIC A49LF040A in a funky state. Use 'flashrom -c W29C010(M)/W29C011A/W29EE011/W29EE012-old' if you have a board with such a chip. Probing for Winbond W29C010(M)/W29C011A/W29EE011/W29EE012, 128 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W29C020(C)/W29C022, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W29C040/P, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W39L040, 512 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W49F002U/N, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content Probing for Winbond W49F020, 256 kB: probe_jedec_common: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content No EEPROM/flash device found. Note: flashrom can never write if the flash chip isn't found automatically. Restoring PCI config space for 03:06:0 reg 0x50
Denis.
Am 28.06.2012 20:28 schrieb Denis 'GNUtoo' Carikli:
On Wed, 2012-06-27 at 09:05 +0200, Carl-Daniel Hailfinger wrote:
Hi Denis,
Am 26.06.2012 01:53 schrieb Denis 'GNUtoo' Carikli:
# ./flashrom -p gfxnvidia -VVV flashrom v0.9.5.2-r1546 on Linux 3.0.0-19-generic-pae (i686) flashrom was built with libpci 3.1.7, GCC 4.6.1, little endian Command line (3 args): ./flashrom -p gfxnvidia -VVV Calibrating delay loop... OK. Initializing gfxnvidia programmer Found "NVIDIA RIVA TNT2 Ultra" (168c:0029, BDF 03:06.0). [...]
This should not have happened. Apparently the PCI code (either in flashrom, or libpci) ignores the vendor, and only matches the device in this case.
Ah yes. pcidev.c:pcidev_validate() only checks device_id match, not vendor_id match. Ouch!
Thanks for reporting this bug!
I'd be very interested in the flashrom verbose output for the following patch:
Check vendor_id for PCI based external programmers. Clean up PCI device detection code.
Note: Slight changes in behaviour are possible, especially on dual/quad chip NICs which appear as more than one PCI device. Found devices are no longer printed at _pinfo level, but rather at _pdbg level.
This patch is essentially a small code move, better device counting and lots of indentation changes (look at diff -uw to see real code changes).
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-pcidev_check_vendorid_cleanup/pcidev.c =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/pcidev.c (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/pcidev.c (Arbeitskopie) @@ -35,157 +35,130 @@ TYPE_UNKNOWN };
-uintptr_t pcidev_validate(struct pci_dev *dev, int bar, - const struct pcidev_status *devs) +uintptr_t pcidev_validate(struct pci_dev *dev, int bar) { - int i; uint64_t addr; uint32_t upperaddr; uint8_t headertype; uint16_t supported_cycles; enum pci_bartype bartype = TYPE_UNKNOWN;
- for (i = 0; devs[i].device_name != NULL; i++) { - if (dev->device_id != devs[i].device_id) - continue;
- msg_pinfo("Found "%s %s" (%04x:%04x, BDF %02x:%02x.%x).\n", - devs[i].vendor_name, devs[i].device_name, - dev->vendor_id, dev->device_id, dev->bus, dev->dev, - dev->func); + headertype = pci_read_byte(dev, PCI_HEADER_TYPE) & 0x7f; + msg_pspew("PCI header type 0x%02x\n", headertype);
- headertype = pci_read_byte(dev, PCI_HEADER_TYPE) & 0x7f; - msg_pspew("PCI header type 0x%02x\n", headertype); + /* Don't use dev->base_addr[x] (as value for 'bar'), won't work on older libpci. */ + addr = pci_read_long(dev, bar);
- /* - * Don't use dev->base_addr[x] (as value for 'bar'), won't - * work on older libpci. - */ - addr = pci_read_long(dev, bar); - - /* Sanity checks. */ - switch (headertype) { - case PCI_HEADER_TYPE_NORMAL: - switch (bar) { - case PCI_BASE_ADDRESS_0: - case PCI_BASE_ADDRESS_1: - case PCI_BASE_ADDRESS_2: - case PCI_BASE_ADDRESS_3: - case PCI_BASE_ADDRESS_4: - case PCI_BASE_ADDRESS_5: - if ((addr & PCI_BASE_ADDRESS_SPACE) == - PCI_BASE_ADDRESS_SPACE_IO) - bartype = TYPE_IOBAR; - else - bartype = TYPE_MEMBAR; - break; - case PCI_ROM_ADDRESS: - bartype = TYPE_ROMBAR; - break; - } + /* Sanity checks. */ + switch (headertype) { + case PCI_HEADER_TYPE_NORMAL: + switch (bar) { + case PCI_BASE_ADDRESS_0: + case PCI_BASE_ADDRESS_1: + case PCI_BASE_ADDRESS_2: + case PCI_BASE_ADDRESS_3: + case PCI_BASE_ADDRESS_4: + case PCI_BASE_ADDRESS_5: + if ((addr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) + bartype = TYPE_IOBAR; + else + bartype = TYPE_MEMBAR; break; - case PCI_HEADER_TYPE_BRIDGE: - switch (bar) { - case PCI_BASE_ADDRESS_0: - case PCI_BASE_ADDRESS_1: - if ((addr & PCI_BASE_ADDRESS_SPACE) == - PCI_BASE_ADDRESS_SPACE_IO) - bartype = TYPE_IOBAR; - else - bartype = TYPE_MEMBAR; - break; - case PCI_ROM_ADDRESS1: - bartype = TYPE_ROMBAR; - break; - } + case PCI_ROM_ADDRESS: + bartype = TYPE_ROMBAR; break; - case PCI_HEADER_TYPE_CARDBUS: + } + break; + case PCI_HEADER_TYPE_BRIDGE: + switch (bar) { + case PCI_BASE_ADDRESS_0: + case PCI_BASE_ADDRESS_1: + if ((addr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) + bartype = TYPE_IOBAR; + else + bartype = TYPE_MEMBAR; break; - default: - msg_perr("Unknown PCI header type 0x%02x, BAR type " - "cannot be determined reliably.\n", headertype); + case PCI_ROM_ADDRESS1: + bartype = TYPE_ROMBAR; break; } + break; + case PCI_HEADER_TYPE_CARDBUS: + break; + default: + msg_perr("Unknown PCI header type 0x%02x, BAR type cannot be determined reliably.\n", + headertype); + break; + }
- supported_cycles = pci_read_word(dev, PCI_COMMAND); + supported_cycles = pci_read_word(dev, PCI_COMMAND);
- msg_pdbg("Requested BAR is "); - switch (bartype) { - case TYPE_MEMBAR: - msg_pdbg("MEM"); - if (!(supported_cycles & PCI_COMMAND_MEMORY)) { - msg_perr("MEM BAR access requested, but device " - "has MEM space accesses disabled.\n"); - /* TODO: Abort here? */ - } - msg_pdbg(", %sbit, %sprefetchable\n", - ((addr & 0x6) == 0x0) ? "32" : - (((addr & 0x6) == 0x4) ? "64" : "reserved"), - (addr & 0x8) ? "" : "not "); - if ((addr & 0x6) == 0x4) { - /* The spec says that a 64-bit register consumes - * two subsequent dword locations. - */ - upperaddr = pci_read_long(dev, bar + 4); - if (upperaddr != 0x00000000) { - /* Fun! A real 64-bit resource. */ - if (sizeof(uintptr_t) != sizeof(uint64_t)) { - msg_perr("BAR unreachable!"); - /* TODO: Really abort here? If - * multiple PCI devices match, - * we might never tell the user - * about the other devices. - */ - return 0; - } - addr |= (uint64_t)upperaddr << 32; + msg_pdbg("Requested BAR is "); + switch (bartype) { + case TYPE_MEMBAR: + msg_pdbg("MEM"); + if (!(supported_cycles & PCI_COMMAND_MEMORY)) { + msg_perr("MEM BAR access requested, but device " + "has MEM space accesses disabled.\n"); + /* TODO: Abort here? */ + } + msg_pdbg(", %sbit, %sprefetchable\n", + ((addr & 0x6) == 0x0) ? "32" : + (((addr & 0x6) == 0x4) ? "64" : "reserved"), + (addr & 0x8) ? "" : "not "); + if ((addr & 0x6) == 0x4) { + /* The spec says that a 64-bit register consumes + * two subsequent dword locations. + */ + upperaddr = pci_read_long(dev, bar + 4); + if (upperaddr != 0x00000000) { + /* Fun! A real 64-bit resource. */ + if (sizeof(uintptr_t) != sizeof(uint64_t)) { + msg_perr("BAR unreachable!"); + /* TODO: Really abort here? If + * multiple PCI devices match, + * we might never tell the user + * about the other devices. + */ + return 0; } + addr |= (uint64_t)upperaddr << 32; } - addr &= PCI_BASE_ADDRESS_MEM_MASK; - break; - case TYPE_IOBAR: - msg_pdbg("I/O\n"); + } + addr &= PCI_BASE_ADDRESS_MEM_MASK; + break; + case TYPE_IOBAR: + msg_pdbg("I/O\n"); #if __FLASHROM_HAVE_OUTB__ - if (!(supported_cycles & PCI_COMMAND_IO)) { - msg_perr("I/O BAR access requested, but device " - "has I/O space accesses disabled.\n"); - /* TODO: Abort here? */ - } + if (!(supported_cycles & PCI_COMMAND_IO)) { + msg_perr("I/O BAR access requested, but device " + "has I/O space accesses disabled.\n"); + /* TODO: Abort here? */ + } #else - msg_perr("I/O BAR access requested, but flashrom does " - "not support I/O BAR access on this platform " - "(yet).\n"); + msg_perr("I/O BAR access requested, but flashrom does " + "not support I/O BAR access on this platform " + "(yet).\n"); #endif - addr &= PCI_BASE_ADDRESS_IO_MASK; - break; - case TYPE_ROMBAR: - msg_pdbg("ROM\n"); - /* Not sure if this check is needed. */ - if (!(supported_cycles & PCI_COMMAND_MEMORY)) { - msg_perr("MEM BAR access requested, but device " - "has MEM space accesses disabled.\n"); - /* TODO: Abort here? */ - } - addr &= PCI_ROM_ADDRESS_MASK; - break; - case TYPE_UNKNOWN: - msg_perr("BAR type unknown, please report a bug at " - "flashrom@flashrom.org\n"); + addr &= PCI_BASE_ADDRESS_IO_MASK; + break; + case TYPE_ROMBAR: + msg_pdbg("ROM\n"); + /* Not sure if this check is needed. */ + if (!(supported_cycles & PCI_COMMAND_MEMORY)) { + msg_perr("MEM BAR access requested, but device " + "has MEM space accesses disabled.\n"); + /* TODO: Abort here? */ } - - if (devs[i].status == NT) { - msg_pinfo("===\nThis PCI device is UNTESTED. Please " - "report the 'flashrom -p xxxx' output \n" - "to flashrom@flashrom.org if it works " - "for you. Please add the name of your\n" - "PCI device to the subject. Thank you for " - "your help!\n===\n"); - } - - return (uintptr_t)addr; + addr &= PCI_ROM_ADDRESS_MASK; + break; + case TYPE_UNKNOWN: + msg_perr("BAR type unknown, please report a bug at " + "flashrom@flashrom.org\n"); }
- return 0; + return (uintptr_t)addr; }
uintptr_t pcidev_init(int bar, const struct pcidev_status *devs) @@ -195,6 +168,7 @@ char *pcidev_bdf; char *msg = NULL; int found = 0; + int i; uintptr_t addr = 0, curaddr = 0;
pacc = pci_alloc(); /* Get the pci_access structure */ @@ -214,10 +188,29 @@
for (dev = pacc->devices; dev; dev = dev->next) { if (pci_filter_match(&filter, dev)) { + /* Check against list of supported devices. */ + for (i = 0; devs[i].device_name != NULL; i++) + if ((dev->vendor_id == devs[i].vendor_id) && + (dev->device_id == devs[i].device_id)) + break; + /* Not supported, try the next one. */ + if (devs[i].device_name == NULL) + continue; + + msg_pdbg("Found "%s %s" (%04x:%04x, BDF %02x:%02x.%x).\n", devs[i].vendor_name, + devs[i].device_name, dev->vendor_id, dev->device_id, dev->bus, dev->dev, + dev->func); + if (devs[i].status == NT) + msg_pinfo("===\nThis PCI device is UNTESTED. Please report the 'flashrom -p " + "xxxx' output \n" + "to flashrom@flashrom.org if it works for you. Please add the name " + "of your\n" + "PCI device to the subject. Thank you for your help!\n===\n"); + /* FIXME: We should count all matching devices, not * just those with a valid BAR. */ - if ((addr = pcidev_validate(dev, bar, devs)) != 0) { + if ((addr = pcidev_validate(dev, bar)) != 0) { curaddr = addr; pcidev_dev = dev; found++; Index: flashrom-pcidev_check_vendorid_cleanup/nicintel.c =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/nicintel.c (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/nicintel.c (Arbeitskopie) @@ -87,7 +87,7 @@ goto error_out_unmap;
/* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */ - addr = pcidev_validate(pcidev_dev, PCI_BASE_ADDRESS_0, nics_intel); + addr = pcidev_validate(pcidev_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_check_vendorid_cleanup/programmer.h =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/programmer.h (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/programmer.h (Arbeitskopie) @@ -227,7 +227,7 @@ const char *vendor_name; const char *device_name; }; -uintptr_t pcidev_validate(struct pci_dev *dev, int bar, const struct pcidev_status *devs); +uintptr_t pcidev_validate(struct pci_dev *dev, int bar); uintptr_t pcidev_init(int bar, const struct pcidev_status *devs); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown.
Am Samstag, den 30.06.2012, 01:40 +0200 schrieb Carl-Daniel Hailfinger:
This patch is essentially a small code move, better device counting and lots of indentation changes (look at diff -uw to see real code changes).
So basically you are moving the device test level check from pcidev_validate to pcidev_init. The result is that pcidev_validate does no longer in fact validate whether a candidate device is the correct device, as the name implies. A name like "pcidev_readbar" would make more sense, IMHO.
Now, check where pcidev_validate is called from. It is called from pcidev_init to do its original job, namely validating a device, and it is called from nicintel code to get a second BAR, because the first BAR returned by pcidev_init is not really enough. That invocation of pcidev_validate already carries a nice comment reading "/* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */". This indicates that something is wrong with this code. And in my oppinion, indeed, it is. The problem is that the current code contains an abstraction violation. Neither pcidev_validate (in the pre-patch form) is really meant to be called from higher level code, but it is meant to be called by pcidev_init only (doing it's job, validating a PCI device, poorly), nor is using a global variable pcidev_dev a good idea for communication between pcidev.c and the clients of that code.
Furthermore, pcidev_init (conveniently, I admit) does two things at once: It finds a PCI device, and it reads one BAR. What about having pcidev_init return the struct pci_dev* instead, so it is up to the client driver to handle that the value instead of putting it in a global variable? Of course, this implies all clients have to use something like pcidev_readbar (the post-patch pcidev_validate) function, even if just a single address is needed. Considering our current codebase, which uses pci_read_long to get base addresses in satamv and satasii, I think giving pcidev_readbar some extra exposure would even be a good educational thing.
And as I go along ranting about the current state of that code, I think it is nice that pcidev_validate does so many sanity checks, but there is one thing I do *not* like about it, you could call it a "missing vital sanity check". pcidev_validate checks whether the given BAR is I/O mapped or memory mapped, and handles each of those types correctly. Great! Finally, it returns the obtained base address, which might either be in I/O or memory space to the caller, *not* indicating whether it just returned an I/O or a memory address. This is quite "convenient", as the callers (should) know what type they expect, and they are not prepared to handle mismatches[1], but it would be even more convenient if this quite important verification of the address space would be performed. So pcidev_readbar (the new pcidev_validate) needs an extra parameter, which should indicate I/O or memory space (or, alternatively, provide pcidev_readbar_io and pcidev_readbar_mem, as there are some strict coding styles claiming the flag parameters are evil, because if there are two different ways a function can be executed (selected by that flag parameter), it really means there are two functions which should have their own name) and have it fail if the BAR type does not match the callers expectation.
Considering all the stuff I was writing, I think your current patch might be the first of a series leading towards a better pcidev interface, if you apply the name fix for pcidev_validate.
I guess the second step would be removing the global pcidev_dev variable, which implies all callers of pcidev_init need to be adjusted. Maybe for modules that really only need one BAR, we can still provide the old behaviour as "pcidev_init_getbar" or "pcidev_init_simple".
In a third step, add I/O vs. memory validation to pcidev_readbar (ex pcidev_validate), and remove all pci_read_long(...) &~7; invocations to read BARs.
So the final vote is:
If you rename pcidev_validate to pcidev_readbar, this is Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
[1]: Remember the old saying: "Never check for an error condition you don't know how to handle"
Am 14.07.2012 22:04 schrieb Michael Karcher:
Am Samstag, den 30.06.2012, 01:40 +0200 schrieb Carl-Daniel Hailfinger:
This patch is essentially a small code move, better device counting and lots of indentation changes (look at diff -uw to see real code changes).
So basically you are moving the device test level check from pcidev_validate to pcidev_init. The result is that pcidev_validate does no longer in fact validate whether a candidate device is the correct device, as the name implies. A name like "pcidev_readbar" would make more sense, IMHO.
Indeed. Fixed.
Now, check where pcidev_validate is called from. It is called from pcidev_init to do its original job, namely validating a device, and it is called from nicintel code to get a second BAR, because the first BAR returned by pcidev_init is not really enough. That invocation of pcidev_validate already carries a nice comment reading "/* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */". This indicates that something is wrong with this code. And in my opinion, indeed, it is. The problem is that the current code contains an abstraction violation. Neither pcidev_validate (in the pre-patch form) is really meant to be called from higher level code, but it is meant to be called by pcidev_init only (doing it's job, validating a PCI device, poorly), nor is using a global variable pcidev_dev a good idea for communication between pcidev.c and the clients of that code.
Furthermore, pcidev_init (conveniently, I admit) does two things at once: It finds a PCI device, and it reads one BAR. What about having pcidev_init return the struct pci_dev* instead, so it is up to the client driver to handle that the value instead of putting it in a global variable? Of course, this implies all clients have to use something like pcidev_readbar (the post-patch pcidev_validate) function, even if just a single address is needed. Considering our current codebase, which uses pci_read_long to get base addresses in satamv and satasii, I think giving pcidev_readbar some extra exposure would even be a good educational thing.
And as I go along ranting about the current state of that code, I think it is nice that pcidev_validate does so many sanity checks, but there is one thing I do *not* like about it, you could call it a "missing vital sanity check". pcidev_validate checks whether the given BAR is I/O mapped or memory mapped, and handles each of those types correctly. Great! Finally, it returns the obtained base address, which might either be in I/O or memory space to the caller, *not* indicating whether it just returned an I/O or a memory address. This is quite "convenient", as the callers (should) know what type they expect, and they are not prepared to handle mismatches[1], but it would be even more convenient if this quite important verification of the address space would be performed. So pcidev_readbar (the new pcidev_validate) needs an extra parameter, which should indicate I/O or memory space (or, alternatively, provide pcidev_readbar_io and pcidev_readbar_mem, as there are some strict coding styles claiming the flag parameters are evil, because if there are two different ways a function can be executed (selected by that flag parameter), it really means there are two functions which should have their own name) and have it fail if the BAR type does not match the callers expectation.
I completely agree with all your points.
Considering all the stuff I was writing, I think your current patch might be the first of a series leading towards a better pcidev interface, if you apply the name fix for pcidev_validate.
Done.
I guess the second step would be removing the global pcidev_dev variable, which implies all callers of pcidev_init need to be adjusted.
Yes.
Maybe for modules that really only need one BAR, we can still provide the old behaviour as "pcidev_init_getbar" or "pcidev_init_simple".
I'd rather not introduce a special case here because some people might use it in ways we didn't anticipate.
In a third step, add I/O vs. memory validation to pcidev_readbar (ex pcidev_validate), and remove all pci_read_long(...) &~7; invocations to read BARs.
So the final vote is:
If you rename pcidev_validate to pcidev_readbar, this is Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks! Updated patch below (only changes from the previous version are 112 column reformattings and your suggested name change. Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-pcidev_check_vendorid_cleanup/pcidev.c =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/pcidev.c (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/pcidev.c (Arbeitskopie) @@ -35,157 +35,122 @@ TYPE_UNKNOWN };
-uintptr_t pcidev_validate(struct pci_dev *dev, int bar, - const struct pcidev_status *devs) +uintptr_t pcidev_readbar(struct pci_dev *dev, int bar) { - int i; uint64_t addr; uint32_t upperaddr; uint8_t headertype; uint16_t supported_cycles; enum pci_bartype bartype = TYPE_UNKNOWN;
- for (i = 0; devs[i].device_name != NULL; i++) { - if (dev->device_id != devs[i].device_id) - continue;
- msg_pinfo("Found "%s %s" (%04x:%04x, BDF %02x:%02x.%x).\n", - devs[i].vendor_name, devs[i].device_name, - dev->vendor_id, dev->device_id, dev->bus, dev->dev, - dev->func); + headertype = pci_read_byte(dev, PCI_HEADER_TYPE) & 0x7f; + msg_pspew("PCI header type 0x%02x\n", headertype);
- headertype = pci_read_byte(dev, PCI_HEADER_TYPE) & 0x7f; - msg_pspew("PCI header type 0x%02x\n", headertype); + /* Don't use dev->base_addr[x] (as value for 'bar'), won't work on older libpci. */ + addr = pci_read_long(dev, bar);
- /* - * Don't use dev->base_addr[x] (as value for 'bar'), won't - * work on older libpci. - */ - addr = pci_read_long(dev, bar); - - /* Sanity checks. */ - switch (headertype) { - case PCI_HEADER_TYPE_NORMAL: - switch (bar) { - case PCI_BASE_ADDRESS_0: - case PCI_BASE_ADDRESS_1: - case PCI_BASE_ADDRESS_2: - case PCI_BASE_ADDRESS_3: - case PCI_BASE_ADDRESS_4: - case PCI_BASE_ADDRESS_5: - if ((addr & PCI_BASE_ADDRESS_SPACE) == - PCI_BASE_ADDRESS_SPACE_IO) - bartype = TYPE_IOBAR; - else - bartype = TYPE_MEMBAR; - break; - case PCI_ROM_ADDRESS: - bartype = TYPE_ROMBAR; - break; - } + /* Sanity checks. */ + switch (headertype) { + case PCI_HEADER_TYPE_NORMAL: + switch (bar) { + case PCI_BASE_ADDRESS_0: + case PCI_BASE_ADDRESS_1: + case PCI_BASE_ADDRESS_2: + case PCI_BASE_ADDRESS_3: + case PCI_BASE_ADDRESS_4: + case PCI_BASE_ADDRESS_5: + if ((addr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) + bartype = TYPE_IOBAR; + else + bartype = TYPE_MEMBAR; break; - case PCI_HEADER_TYPE_BRIDGE: - switch (bar) { - case PCI_BASE_ADDRESS_0: - case PCI_BASE_ADDRESS_1: - if ((addr & PCI_BASE_ADDRESS_SPACE) == - PCI_BASE_ADDRESS_SPACE_IO) - bartype = TYPE_IOBAR; - else - bartype = TYPE_MEMBAR; - break; - case PCI_ROM_ADDRESS1: - bartype = TYPE_ROMBAR; - break; - } + case PCI_ROM_ADDRESS: + bartype = TYPE_ROMBAR; break; - case PCI_HEADER_TYPE_CARDBUS: + } + break; + case PCI_HEADER_TYPE_BRIDGE: + switch (bar) { + case PCI_BASE_ADDRESS_0: + case PCI_BASE_ADDRESS_1: + if ((addr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) + bartype = TYPE_IOBAR; + else + bartype = TYPE_MEMBAR; break; - default: - msg_perr("Unknown PCI header type 0x%02x, BAR type " - "cannot be determined reliably.\n", headertype); + case PCI_ROM_ADDRESS1: + bartype = TYPE_ROMBAR; break; } + break; + case PCI_HEADER_TYPE_CARDBUS: + break; + default: + msg_perr("Unknown PCI header type 0x%02x, BAR type cannot be determined reliably.\n", + headertype); + break; + }
- supported_cycles = pci_read_word(dev, PCI_COMMAND); + supported_cycles = pci_read_word(dev, PCI_COMMAND);
- msg_pdbg("Requested BAR is "); - switch (bartype) { - case TYPE_MEMBAR: - msg_pdbg("MEM"); - if (!(supported_cycles & PCI_COMMAND_MEMORY)) { - msg_perr("MEM BAR access requested, but device " - "has MEM space accesses disabled.\n"); - /* TODO: Abort here? */ - } - msg_pdbg(", %sbit, %sprefetchable\n", - ((addr & 0x6) == 0x0) ? "32" : - (((addr & 0x6) == 0x4) ? "64" : "reserved"), - (addr & 0x8) ? "" : "not "); - if ((addr & 0x6) == 0x4) { - /* The spec says that a 64-bit register consumes - * two subsequent dword locations. - */ - upperaddr = pci_read_long(dev, bar + 4); - if (upperaddr != 0x00000000) { - /* Fun! A real 64-bit resource. */ - if (sizeof(uintptr_t) != sizeof(uint64_t)) { - msg_perr("BAR unreachable!"); - /* TODO: Really abort here? If - * multiple PCI devices match, - * we might never tell the user - * about the other devices. - */ - return 0; - } - addr |= (uint64_t)upperaddr << 32; + msg_pdbg("Requested BAR is "); + switch (bartype) { + case TYPE_MEMBAR: + msg_pdbg("MEM"); + if (!(supported_cycles & PCI_COMMAND_MEMORY)) { + msg_perr("MEM BAR access requested, but device has MEM space accesses disabled.\n"); + /* TODO: Abort here? */ + } + msg_pdbg(", %sbit, %sprefetchable\n", + ((addr & 0x6) == 0x0) ? "32" : (((addr & 0x6) == 0x4) ? "64" : "reserved"), + (addr & 0x8) ? "" : "not "); + if ((addr & 0x6) == 0x4) { + /* The spec says that a 64-bit register consumes + * two subsequent dword locations. + */ + upperaddr = pci_read_long(dev, bar + 4); + if (upperaddr != 0x00000000) { + /* Fun! A real 64-bit resource. */ + if (sizeof(uintptr_t) != sizeof(uint64_t)) { + msg_perr("BAR unreachable!"); + /* TODO: Really abort here? If multiple PCI devices match, + * we might never tell the user about the other devices. + */ + return 0; } + addr |= (uint64_t)upperaddr << 32; } - addr &= PCI_BASE_ADDRESS_MEM_MASK; - break; - case TYPE_IOBAR: - msg_pdbg("I/O\n"); + } + addr &= PCI_BASE_ADDRESS_MEM_MASK; + break; + case TYPE_IOBAR: + msg_pdbg("I/O\n"); #if __FLASHROM_HAVE_OUTB__ - if (!(supported_cycles & PCI_COMMAND_IO)) { - msg_perr("I/O BAR access requested, but device " - "has I/O space accesses disabled.\n"); - /* TODO: Abort here? */ - } + if (!(supported_cycles & PCI_COMMAND_IO)) { + msg_perr("I/O BAR access requested, but device has I/O space accesses disabled.\n"); + /* TODO: Abort here? */ + } #else - msg_perr("I/O BAR access requested, but flashrom does " - "not support I/O BAR access on this platform " - "(yet).\n"); + msg_perr("I/O BAR access requested, but flashrom does not support I/O BAR access on this " + "platform (yet).\n"); #endif - addr &= PCI_BASE_ADDRESS_IO_MASK; - break; - case TYPE_ROMBAR: - msg_pdbg("ROM\n"); - /* Not sure if this check is needed. */ - if (!(supported_cycles & PCI_COMMAND_MEMORY)) { - msg_perr("MEM BAR access requested, but device " - "has MEM space accesses disabled.\n"); - /* TODO: Abort here? */ - } - addr &= PCI_ROM_ADDRESS_MASK; - break; - case TYPE_UNKNOWN: - msg_perr("BAR type unknown, please report a bug at " - "flashrom@flashrom.org\n"); + addr &= PCI_BASE_ADDRESS_IO_MASK; + break; + case TYPE_ROMBAR: + msg_pdbg("ROM\n"); + /* Not sure if this check is needed. */ + if (!(supported_cycles & PCI_COMMAND_MEMORY)) { + msg_perr("MEM BAR access requested, but device has MEM space accesses disabled.\n"); + /* TODO: Abort here? */ } - - if (devs[i].status == NT) { - msg_pinfo("===\nThis PCI device is UNTESTED. Please " - "report the 'flashrom -p xxxx' output \n" - "to flashrom@flashrom.org if it works " - "for you. Please add the name of your\n" - "PCI device to the subject. Thank you for " - "your help!\n===\n"); - } - - return (uintptr_t)addr; + addr &= PCI_ROM_ADDRESS_MASK; + break; + case TYPE_UNKNOWN: + msg_perr("BAR type unknown, please report a bug at flashrom@flashrom.org\n"); }
- return 0; + return (uintptr_t)addr; }
uintptr_t pcidev_init(int bar, const struct pcidev_status *devs) @@ -195,6 +160,7 @@ char *pcidev_bdf; char *msg = NULL; int found = 0; + int i; uintptr_t addr = 0, curaddr = 0;
pacc = pci_alloc(); /* Get the pci_access structure */ @@ -214,10 +180,29 @@
for (dev = pacc->devices; dev; dev = dev->next) { if (pci_filter_match(&filter, dev)) { + /* Check against list of supported devices. */ + for (i = 0; devs[i].device_name != NULL; i++) + if ((dev->vendor_id == devs[i].vendor_id) && + (dev->device_id == devs[i].device_id)) + break; + /* Not supported, try the next one. */ + if (devs[i].device_name == NULL) + continue; + + msg_pdbg("Found "%s %s" (%04x:%04x, BDF %02x:%02x.%x).\n", devs[i].vendor_name, + devs[i].device_name, dev->vendor_id, dev->device_id, dev->bus, dev->dev, + dev->func); + if (devs[i].status == NT) + msg_pinfo("===\nThis PCI device is UNTESTED. Please report the 'flashrom -p " + "xxxx' output \n" + "to flashrom@flashrom.org if it works for you. Please add the name " + "of your\n" + "PCI device to the subject. Thank you for your help!\n===\n"); + /* FIXME: We should count all matching devices, not * just those with a valid BAR. */ - if ((addr = pcidev_validate(dev, bar, devs)) != 0) { + if ((addr = pcidev_readbar(dev, bar)) != 0) { curaddr = addr; pcidev_dev = dev; found++; @@ -230,10 +215,8 @@ msg_perr("Error: No supported PCI device found.\n"); exit(1); } 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"); + 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); }
Index: flashrom-pcidev_check_vendorid_cleanup/nicintel.c =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/nicintel.c (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/nicintel.c (Arbeitskopie) @@ -87,7 +87,7 @@ goto error_out_unmap;
/* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */ - addr = pcidev_validate(pcidev_dev, PCI_BASE_ADDRESS_0, nics_intel); + addr = pcidev_readbar(pcidev_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_check_vendorid_cleanup/programmer.h =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/programmer.h (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/programmer.h (Arbeitskopie) @@ -227,7 +227,7 @@ const char *vendor_name; const char *device_name; }; -uintptr_t pcidev_validate(struct pci_dev *dev, int bar, const struct pcidev_status *devs); +uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); uintptr_t pcidev_init(int bar, const struct pcidev_status *devs); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown.
Am 16.07.2012 09:15 schrieb Carl-Daniel Hailfinger:
If you rename pcidev_validate to pcidev_readbar, this is Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks! Updated patch below (only changes from the previous version are 112 column reformattings and your suggested name change. Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Thanks for the review. Committed in r1548.
Regards, Carl-Daniel
Am 14.07.2012 22:04 schrieb Michael Karcher:
Am Samstag, den 30.06.2012, 01:40 +0200 schrieb Carl-Daniel Hailfinger:
This patch is essentially a small code move, better device counting and lots of indentation changes (look at diff -uw to see real code changes).
So basically you are moving the device test level check from pcidev_validate to pcidev_init. The result is that pcidev_validate does no longer in fact validate whether a candidate device is the correct device, as the name implies. A name like "pcidev_readbar" would make more sense, IMHO.
Now, check where pcidev_validate is called from. It is called from pcidev_init to do its original job, namely validating a device, and it is called from nicintel code to get a second BAR, because the first BAR returned by pcidev_init is not really enough. That invocation of pcidev_validate already carries a nice comment reading "/* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */". This indicates that something is wrong with this code. And in my oppinion, indeed, it is. The problem is that the current code contains an abstraction violation. Neither pcidev_validate (in the pre-patch form) is really meant to be called from higher level code, but it is meant to be called by pcidev_init only (doing it's job, validating a PCI device, poorly), nor is using a global variable pcidev_dev a good idea for communication between pcidev.c and the clients of that code.
Furthermore, pcidev_init (conveniently, I admit) does two things at once: It finds a PCI device, and it reads one BAR. What about having pcidev_init return the struct pci_dev* instead, so it is up to the client driver to handle that the value instead of putting it in a global variable? Of course, this implies all clients have to use something like pcidev_readbar (the post-patch pcidev_validate) function, even if just a single address is needed. Considering our current codebase, which uses pci_read_long to get base addresses in satamv and satasii, I think giving pcidev_readbar some extra exposure would even be a good educational thing.
And as I go along ranting about the current state of that code, I think it is nice that pcidev_validate does so many sanity checks, but there is one thing I do *not* like about it, you could call it a "missing vital sanity check". pcidev_validate checks whether the given BAR is I/O mapped or memory mapped, and handles each of those types correctly. Great! Finally, it returns the obtained base address, which might either be in I/O or memory space to the caller, *not* indicating whether it just returned an I/O or a memory address. This is quite "convenient", as the callers (should) know what type they expect, and they are not prepared to handle mismatches[1], but it would be even more convenient if this quite important verification of the address space would be performed. So pcidev_readbar (the new pcidev_validate) needs an extra parameter, which should indicate I/O or memory space (or, alternatively, provide pcidev_readbar_io and pcidev_readbar_mem, as there are some strict coding styles claiming the flag parameters are evil, because if there are two different ways a function can be executed (selected by that flag parameter), it really means there are two functions which should have their own name) and have it fail if the BAR type does not match the callers expectation.
Considering all the stuff I was writing, I think your current patch might be the first of a series leading towards a better pcidev interface, if you apply the name fix for pcidev_validate.
I guess the second step would be removing the global pcidev_dev variable, which implies all callers of pcidev_init need to be adjusted. Maybe for modules that really only need one BAR, we can still provide the old behaviour as "pcidev_init_getbar" or "pcidev_init_simple".
In a third step, add I/O vs. memory validation to pcidev_readbar (ex pcidev_validate), and remove all pci_read_long(...) &~7; invocations to read BARs.
Second patch in the series. Error checking has not changed at all, and I intend to fix that in a second spin of this patch. Right now I just want to check if we're on the same page.
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 1548) +++ 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"); @@ -130,7 +131,8 @@
get_io_perms();
- 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 1548) +++ flashrom-pcidev_init_do_not_return_bar/drkaiser.c (Arbeitskopie) @@ -65,15 +65,16 @@
int drkaiser_init(void) { + struct pci_dev *dev = NULL; uint32_t addr;
get_io_perms();
- 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 1548) +++ flashrom-pcidev_init_do_not_return_bar/pcidev.c (Arbeitskopie) @@ -26,7 +26,6 @@
uint32_t io_base_addr; struct pci_access *pacc; -struct pci_dev *pcidev_dev = NULL;
enum pci_bartype { TYPE_MEMBAR, @@ -153,15 +152,16 @@ return (uintptr_t)addr; }
-uintptr_t pcidev_init(int bar, const struct pcidev_status *devs) +struct pci_dev *pcidev_init(int bar, const struct pcidev_status *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 */ @@ -203,8 +203,7 @@ * just those with a valid BAR. */ if ((addr = pcidev_readbar(dev, bar)) != 0) { - curaddr = addr; - pcidev_dev = dev; + found_dev = dev; found++; } } @@ -220,7 +219,7 @@ exit(1); }
- return curaddr; + return found_dev; }
void print_supported_pcidevs(const struct pcidev_status *devs) Index: flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c (Arbeitskopie) @@ -89,11 +89,13 @@
int gfxnvidia_init(void) { + struct pci_dev *dev = NULL; uint32_t reg32;
get_io_perms();
- io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia); + dev = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia); + 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); @@ -105,9 +107,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 1548) +++ flashrom-pcidev_init_do_not_return_bar/nicrealtek.c (Arbeitskopie) @@ -61,9 +61,12 @@
int nicrealtek_init(void) { + struct pci_dev *dev = NULL; + get_io_perms();
- 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; Index: flashrom-pcidev_init_do_not_return_bar/satamv.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/satamv.c (Revision 1548) +++ 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;
@@ -91,7 +92,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) @@ -143,8 +145,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 1548) +++ flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c (Arbeitskopie) @@ -165,11 +165,13 @@
int nicintel_spi_init(void) { + struct pci_dev *dev = NULL; uint32_t tmp;
get_io_perms();
- 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, 4096); Index: flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Arbeitskopie) @@ -59,9 +59,12 @@
int nicnatsemi_init(void) { + struct pci_dev *dev = NULL; + get_io_perms();
- 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 1548) +++ flashrom-pcidev_init_do_not_return_bar/atahpt.c (Arbeitskopie) @@ -65,16 +65,18 @@
int atahpt_init(void) { + struct pci_dev *dev = NULL; uint32_t reg32;
get_io_perms();
- 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);
/* 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);
if (register_shutdown(atahpt_shutdown, NULL)) return 1; Index: flashrom-pcidev_init_do_not_return_bar/nic3com.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nic3com.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/nic3com.c (Arbeitskopie) @@ -87,11 +87,14 @@
int nic3com_init(void) { + struct pci_dev *dev = NULL; + get_io_perms();
- 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 1548) +++ flashrom-pcidev_init_do_not_return_bar/satasii.c (Arbeitskopie) @@ -67,20 +67,21 @@
int satasii_init(void) { + struct pci_dev *dev = NULL; uint32_t addr; uint16_t reg_offset;
get_io_perms();
- 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 1548) +++ 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. @@ -80,14 +81,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 1548) +++ flashrom-pcidev_init_do_not_return_bar/programmer.h (Arbeitskopie) @@ -219,7 +219,6 @@ // 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; struct pcidev_status { uint16_t vendor_id; uint16_t device_id; @@ -228,7 +227,7 @@ const char *device_name; }; uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); -uintptr_t pcidev_init(int bar, const struct pcidev_status *devs); +struct pci_dev *pcidev_init(int bar, const struct pcidev_status *devs); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. */
Am Dienstag, den 17.07.2012, 09:17 +0200 schrieb Carl-Daniel Hailfinger:
Second patch in the series. Error checking has not changed at all, and I intend to fix that in a second spin of this patch. Right now I just want to check if we're on the same page.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
-struct pci_dev *pcidev_dev = NULL;
Yeah!
if ((addr = pcidev_readbar(dev, bar)) != 0) {
curaddr = addr;pcidev_dev = dev;
found_dev = dev; found++; }
I wonder what to do about this: While the goal of my suggestion was to decouple BAR access from the PCI device scan, it is an integral part of the scan loop, probably to avoid disabled chips (e.g. for onboard components that are not used). The nice property of the code as-is is that you can be sure reading the "primary" BAR will not fail.
As we sometimes need two BARs, having one valid BAR does not mean the device is necessarily usable for us, so this check is only half of what we need. As already discussed on IRC, passing a set of BARs into this function is not really the direction we want to head to, so client code needs to be prepared to find unusable BARs anyway. Still, we like autoskip. Several ideas come to my mind - Use the PCI command word for autoskip - Implement a set-of-BAR (bitmask, array) anyway - Probe that all BARs (except ROM perhaps) that are not unused contain sensible values - Hand in a "is_usable" callback (a standard callback for testing a single BAR could be provided)
All of these approaches of course complicate pcidev_init, but it seems like the only choices we have is: - do the BAR check the right way (TM) - loose the autoskip of disabled PCI devices
Regards, Michael Karcher
Am 21.07.2012 16:02 schrieb Michael Karcher:
Am Dienstag, den 17.07.2012, 09:17 +0200 schrieb Carl-Daniel Hailfinger:
Second patch in the series. Error checking has not changed at all, and I intend to fix that in a second spin of this patch. Right now I just want to check if we're on the same page.
Actually, I plan to commit the error checking change separately because Niklas Söderlund already sent a patch doing that.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
-struct pci_dev *pcidev_dev = NULL;
Yeah!
if ((addr = pcidev_readbar(dev, bar)) != 0) {
curaddr = addr;pcidev_dev = dev;
found_dev = dev; found++; }I wonder what to do about this: While the goal of my suggestion was to decouple BAR access from the PCI device scan, it is an integral part of the scan loop, probably to avoid disabled chips (e.g. for onboard components that are not used).
Intel dual port NICs are represented by two PCI devices, but only one of them has an active BAR for our purposes. It would be unfair of us to have users guess the right PCI device.
The nice property of the code as-is is that you can be sure reading the "primary" BAR will not fail.
As we sometimes need two BARs, having one valid BAR does not mean the device is necessarily usable for us, so this check is only half of what we need. As already discussed on IRC, passing a set of BARs into this function is not really the direction we want to head to, so client code needs to be prepared to find unusable BARs anyway. Still, we like autoskip. Several ideas come to my mind
- Use the PCI command word for autoskip
How would that work?
- Implement a set-of-BAR (bitmask, array) anyway
Depending on the PCI header type, BARs can be at completely different locations. Still, set-of-BAR sounds like a good idea.
- Probe that all BARs (except ROM perhaps) that are not unused contain
sensible values
How do we determine which values are sensible?
- Hand in a "is_usable" callback (a standard callback for testing a
single BAR could be provided)
Do you really trust new driver authors to get that right?
All of these approaches of course complicate pcidev_init, but it seems like the only choices we have is:
- do the BAR check the right way (TM)
- lose the autoskip of disabled PCI devices
Or we handle this in a completely different way...not sure how, but maybe there really is some magic third way.
Regards, Carl-Daniel
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.
Regards, Carl-Daniel
Am 22.07.2012 04:39 schrieb Carl-Daniel Hailfinger:
Am 21.07.2012 16:02 schrieb Michael Karcher:
Am Dienstag, den 17.07.2012, 09:17 +0200 schrieb Carl-Daniel Hailfinger:
Second patch in the series. Error checking has not changed at all, and I intend to fix that in a second spin of this patch. Right now I just want to check if we're on the same page.
Actually, I plan to commit the error checking change separately because Niklas Söderlund already sent a patch doing that.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net -struct pci_dev *pcidev_dev = NULL;
Yeah!
if ((addr = pcidev_readbar(dev, bar)) != 0) {
curaddr = addr;pcidev_dev = dev;
found_dev = dev; found++; }I wonder what to do about this: While the goal of my suggestion was to decouple BAR access from the PCI device scan, it is an integral part of the scan loop, probably to avoid disabled chips (e.g. for onboard components that are not used).
Intel dual port NICs are represented by two PCI devices, but only one of them has an active BAR for our purposes. It would be unfair of us to have users guess the right PCI device.
The nice property of the code as-is is that you can be sure reading the "primary" BAR will not fail.
As we sometimes need two BARs, having one valid BAR does not mean the device is necessarily usable for us, so this check is only half of what we need. As already discussed on IRC, passing a set of BARs into this function is not really the direction we want to head to, so client code needs to be prepared to find unusable BARs anyway. Still, we like autoskip. Several ideas come to my mind [...] All of these approaches of course complicate pcidev_init, but it seems like the only choices we have is:
- do the BAR check the right way (TM)
- lose the autoskip of disabled PCI devices
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.
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!).
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.
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 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 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. */
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
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. */
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);
On Sat, 05 Jan 2013 18:56:33 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
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
looks good enough to me to commit it ;) Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Am 05.01.2013 23:45 schrieb Stefan Tauner:
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
looks good enough to me to commit it ;) Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Thanks a lot for the review.
Changelog: Decouple BAR reading from pci device init, handle errors gracefully.
pcidev_init() now returns struct pci_device * instead of a BAR stored in PCI config space. This allows for real error checking instead of having exit(1) everywhere in pcidev.c. Thanks to Niklas Söderlund for coming up with the original error handling patch which was slightly modified and folded into this patch. Move the declaration of struct pci_device in programmer.h before the first user.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Committed in r1644.
Regards, Carl-Daniel
Michael? This patch implements your "callback solution" suggestion. Comments welcome!
Am 22.07.2012 04:39 schrieb Carl-Daniel Hailfinger:
Am 21.07.2012 16:02 schrieb Michael Karcher:
Am Dienstag, den 17.07.2012, 09:17 +0200 schrieb Carl-Daniel Hailfinger:
if ((addr = pcidev_readbar(dev, bar)) != 0) {
curaddr = addr;pcidev_dev = dev;
found_dev = dev; found++; }I wonder what to do about this: While the goal of my suggestion was to decouple BAR access from the PCI device scan, it is an integral part of the scan loop, probably to avoid disabled chips (e.g. for onboard components that are not used).
Intel dual port NICs are represented by two PCI devices, but only one of them has an active BAR for our purposes. It would be unfair of us to have users guess the right PCI device.
The nice property of the code as-is is that you can be sure reading the "primary" BAR will not fail.
As we sometimes need two BARs, having one valid BAR does not mean the device is necessarily usable for us, so this check is only half of what we need. As already discussed on IRC, passing a set of BARs into this function is not really the direction we want to head to, so client code needs to be prepared to find unusable BARs anyway. Still, we like autoskip. Several ideas come to my mind
- Use the PCI command word for autoskip
How would that work?
I still haven't figured that out.
- Implement a set-of-BAR (bitmask, array) anyway
Depending on the PCI header type, BARs can be at completely different locations. Still, set-of-BAR sounds like a good idea.
That doesn't work for satasii which uses different BARs for different PCI IDs.
- Probe that all BARs (except ROM perhaps) that are not unused contain
sensible values
How do we determine which values are sensible?
That doesn't work either for external programmers which need more than one BAR, and one of those BARs may be disabled by default (i.e. unusable).
- Hand in a "is_usable" callback (a standard callback for testing a
single BAR could be provided)
Do you really trust new driver authors to get that right?
I think this one is the only viable solution, and with my automatic driver writer script (and plenty of good code to copy from) I think that there is a good chance new driver authors will get it right. I have named the callback "validator" instead of "is_usable", but I have no preference for either name.
All of these approaches of course complicate pcidev_init, but it seems like the only choices we have is:
- do the BAR check the right way (TM)
- lose the autoskip of disabled PCI devices
Am 04.09.2012 12:14 schrieb Stefan Tauner:
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, this is now taken care of.
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!).
Do you feel the comment level is adequate now or should I add some more stuff?
Changelog: Change pcidev_init() to take a validator function as parameter instead of a BAR specifier. This allows for code like atavia (which doesn't even use a BAR) and satasii (which uses different BARs depending on PCI ID).
NOTE: satamv is not finished, and won't compile. No other known problems.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-pcidev_init_validator_function/ogp_spi.c =================================================================== --- flashrom-pcidev_init_validator_function/ogp_spi.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/ogp_spi.c (Arbeitskopie) @@ -103,6 +103,12 @@ return 0; }
+static int ogp_spi_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int ogp_spi_init(void) { struct pci_dev *dev = NULL; @@ -132,7 +138,7 @@ if (rget_io_perms()) return 1;
- dev = pcidev_init(ogp_spi, PCI_BASE_ADDRESS_0); + dev = pcidev_init(ogp_spi, ogp_spi_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/drkaiser.c =================================================================== --- flashrom-pcidev_init_validator_function/drkaiser.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/drkaiser.c (Arbeitskopie) @@ -62,6 +62,12 @@ return 0; }
+static int drkaiser_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_2); +} + int drkaiser_init(void) { struct pci_dev *dev = NULL; @@ -70,7 +76,7 @@ if (rget_io_perms()) return 1;
- dev = pcidev_init(drkaiser_pcidev, PCI_BASE_ADDRESS_2); + dev = pcidev_init(drkaiser_pcidev, drkaiser_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/pcidev.c =================================================================== --- flashrom-pcidev_init_validator_function/pcidev.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/pcidev.c (Arbeitskopie) @@ -184,7 +184,7 @@ * 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 *pcidev_init(const struct dev_entry *devs, int (*validator) (struct pci_dev *dev)) { struct pci_dev *dev; struct pci_dev *found_dev = NULL; @@ -193,7 +193,6 @@ char *msg = NULL; int found = 0; int i; - uintptr_t addr = 0;
if (pci_init_common() != 0) return NULL; @@ -233,7 +232,7 @@ /* FIXME: We should count all matching devices, not * just those with a valid BAR. */ - if ((addr = pcidev_readbar(dev, bar)) != 0) { + if (!(*validator)(dev)) { found_dev = dev; found++; } Index: flashrom-pcidev_init_validator_function/gfxnvidia.c =================================================================== --- flashrom-pcidev_init_validator_function/gfxnvidia.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/gfxnvidia.c (Arbeitskopie) @@ -83,6 +83,12 @@ return 0; }
+static int gfxnvidia_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int gfxnvidia_init(void) { struct pci_dev *dev = NULL; @@ -91,7 +97,7 @@ if (rget_io_perms()) return 1;
- dev = pcidev_init(gfx_nvidia, PCI_BASE_ADDRESS_0); + dev = pcidev_init(gfx_nvidia, gfxnvidia_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/nicrealtek.c =================================================================== --- flashrom-pcidev_init_validator_function/nicrealtek.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nicrealtek.c (Arbeitskopie) @@ -57,6 +57,12 @@ return 0; }
+static int nicrealtek_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int nicrealtek_init(void) { struct pci_dev *dev = NULL; @@ -64,7 +70,7 @@ if (rget_io_perms()) return 1;
- dev = pcidev_init(nics_realtek, PCI_BASE_ADDRESS_0); + dev = pcidev_init(nics_realtek, nicrealtek_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/satamv.c =================================================================== --- flashrom-pcidev_init_validator_function/satamv.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/satamv.c (Arbeitskopie) @@ -63,6 +63,13 @@ return 0; }
+static int satamv_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ +#error Check multiple BARs! + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_); +} + /* * Random notes: * FCE# Flash Chip Enable @@ -89,7 +96,7 @@ return 1;
/* BAR0 has all internal registers memory mapped. */ - dev = pcidev_init(satas_mv, PCI_BASE_ADDRESS_0); + dev = pcidev_init(satas_mv, satamv_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/nicintel_spi.c =================================================================== --- flashrom-pcidev_init_validator_function/nicintel_spi.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nicintel_spi.c (Arbeitskopie) @@ -164,6 +164,12 @@ return 0; }
+static int nicintel_spi_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int nicintel_spi_init(void) { struct pci_dev *dev = NULL; @@ -172,7 +178,7 @@ if (rget_io_perms()) return 1;
- dev = pcidev_init(nics_intel_spi, PCI_BASE_ADDRESS_0); + dev = pcidev_init(nics_intel_spi, nicintel_spi_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/nicnatsemi.c =================================================================== --- flashrom-pcidev_init_validator_function/nicnatsemi.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nicnatsemi.c (Arbeitskopie) @@ -52,6 +52,12 @@ .chip_writen = fallback_chip_writen, };
+static int nicnatsemi_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int nicnatsemi_init(void) { struct pci_dev *dev = NULL; @@ -59,7 +65,7 @@ if (rget_io_perms()) return 1;
- dev = pcidev_init(nics_natsemi, PCI_BASE_ADDRESS_0); + dev = pcidev_init(nics_natsemi, nicnatsemi_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/atahpt.c =================================================================== --- flashrom-pcidev_init_validator_function/atahpt.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/atahpt.c (Arbeitskopie) @@ -56,6 +56,12 @@ .chip_writen = fallback_chip_writen, };
+static int atahpt_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_4); +} + int atahpt_init(void) { struct pci_dev *dev = NULL; @@ -64,7 +70,7 @@ if (rget_io_perms()) return 1;
- dev = pcidev_init(ata_hpt, PCI_BASE_ADDRESS_4); + dev = pcidev_init(ata_hpt, atahpt_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/nic3com.c =================================================================== --- flashrom-pcidev_init_validator_function/nic3com.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nic3com.c (Arbeitskopie) @@ -84,6 +84,12 @@ return 0; }
+static int nic3com_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int nic3com_init(void) { struct pci_dev *dev = NULL; @@ -91,7 +97,7 @@ if (rget_io_perms()) return 1;
- dev = pcidev_init(nics_3com, PCI_BASE_ADDRESS_0); + dev = pcidev_init(nics_3com, nic3com_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/satasii.c =================================================================== --- flashrom-pcidev_init_validator_function/satasii.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/satasii.c (Arbeitskopie) @@ -74,6 +74,19 @@ return ctrl_reg; }
+static int satasii_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + switch (dev->device_id) { + case 0x3132: + case 0x3124: + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + break; + default: + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_5); + } +} + int satasii_init(void) { struct pci_dev *dev = NULL; @@ -83,16 +96,19 @@ if (rget_io_perms()) return 1;
- dev = pcidev_init(satas_sii, PCI_BASE_ADDRESS_0); + dev = pcidev_init(satas_sii, satasii_validator); if (!dev) return 1;
id = dev->device_id;
- if ((id == 0x3132) || (id == 0x3124)) { + switch (id) { + case 0x3132: + case 0x3124: addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); reg_offset = 0x70; - } else { + break; + default: addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_5); reg_offset = 0x50; } Index: flashrom-pcidev_init_validator_function/nicintel.c =================================================================== --- flashrom-pcidev_init_validator_function/nicintel.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nicintel.c (Arbeitskopie) @@ -66,6 +66,12 @@ return 0; }
+static int nicintel_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BARs. */ + return !(pcidev_readbar(dev, PCI_BASE_ADDRESS_0) && pcidev_readbar(dev, PCI_BASE_ADDRESS_2)); +} + int nicintel_init(void) { struct pci_dev *dev = NULL; @@ -78,7 +84,7 @@ return 1;
/* FIXME: BAR2 is not available if the device uses the CardBus function. */ - dev = pcidev_init(nics_intel, PCI_BASE_ADDRESS_2); + dev = pcidev_init(nics_intel, nicintel_validator); if (!dev) return 1;
Index: flashrom-pcidev_init_validator_function/programmer.h =================================================================== --- flashrom-pcidev_init_validator_function/programmer.h (Revision 1644) +++ flashrom-pcidev_init_validator_function/programmer.h (Arbeitskopie) @@ -169,7 +169,7 @@ 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); +struct pci_dev *pcidev_init(const struct dev_entry *devs, int (*validator) (struct pci_dev *dev)); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. */
Hello Carl-Daniel,
all-in-all, your patch looks good. Still I have some comments to that mail
Am Sonntag, den 06.01.2013, 22:13 +0100 schrieb Carl-Daniel Hailfinger:
- Use the PCI command word for autoskip
How would that work?
I still haven't figured that out.
If a PCI device is completely disabled (and thus not eligible as flasher), I expected the PCI command word to not enable I/O or memory decoding while it is needed for flashing.
- Implement a set-of-BAR (bitmask, array) anyway
Depending on the PCI header type, BARs can be at completely different locations. Still, set-of-BAR sounds like a good idea.
That doesn't work for satasii which uses different BARs for different PCI IDs.
That could be solved by moving the bitmask into the pcidev_status structure. Which means the mask can be different for each ID.
- Probe that all BARs (except ROM perhaps) that are not unused contain
sensible values
How do we determine which values are sensible?
That doesn't work either for external programmers which need more than one BAR, and one of those BARs may be disabled by default (i.e. unusable).
If things like that really happen, this does not work. I expected a device to either get ressources assigned by the BIOS or PCI core of the OS or none at all.
- Hand in a "is_usable" callback (a standard callback for testing a
single BAR could be provided)
A standard pcidev_validator_bar0 in pcidev.c still might make sense to avoid reimplement it in every second programmer driver.
Do you really trust new driver authors to get that right?
I think this one is the only viable solution, and with my automatic driver writer script (and plenty of good code to copy from) I think that there is a good chance new driver authors will get it right. I have named the callback "validator" instead of "is_usable", but I have no preference for either name.
validator also sounds nice. go with it.
+static int ogp_spi_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
this would be pcidev_validator_bar0 (or pcidev_validate_bar0, depending on whether you want a nominal clause or an action as function name).
-struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar) +struct pci_dev *pcidev_init(const struct dev_entry *devs, int (*validator) (struct pci_dev *dev))
Function declarations including function pointer types are quite difficult to read in my oppinion. What about "typedef int (*pcidev_validator)(struct pci_dev *dev)"?
if ((addr = pcidev_readbar(dev, bar)) != 0) {
if (!(*validator)(dev)) {
The star and the parenthesis are not needed in C. "if (!validator(dev))" would do as well. Furthermore, we don't use this explicit indirection syntax for all those function pointers in structures.
+static int gfxnvidia_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
A second instance of pcidev_validator_bar0
+static int nicrealtek_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
A third one
+static int nicintel_spi_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
A fourth one
+static int nicnatsemi_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
Another one, no sense in counting them anymore ;)
+static int nic3com_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
Well, yes.
- dev = pcidev_init(satas_sii, PCI_BASE_ADDRESS_0);
dev = pcidev_init(satas_sii, satasii_validator); if (!dev) return 1;
id = dev->device_id;
- if ((id == 0x3132) || (id == 0x3124)) {
Well, this works for sure, but couldn't we somehow associate this info into the pci device ID list, instead of repeating some IDs here? Of course this would also mean that pcidev_init needs to return a pointer to the matched pcidev_status entry. Possibly not worth the hassle.
+static int nicintel_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BARs. */
- return !(pcidev_readbar(dev, PCI_BASE_ADDRESS_0) && pcidev_readbar(dev, PCI_BASE_ADDRESS_2));
+}
int nicintel_init(void) { struct pci_dev *dev = NULL; @@ -78,7 +84,7 @@ return 1;
/* FIXME: BAR2 is not available if the device uses the CardBus function. */
Maybe you want to move the FIXME comment into the validator declaration.
-struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar); +struct pci_dev *pcidev_init(const struct dev_entry *devs, int (*validator) (struct pci_dev *dev));
typedef, as above.
Regards, Michael Karcher
-
Carl-Daniel Hailfinger -
Denis 'GNUtoo' Carikli -
Michael Karcher -
Stefan Tauner