[flashrom] flashrom -p gfxnvidia detect my atheros ath9k wifi card
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Tue Jul 17 09:17:13 CEST 2012
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 at gmx.net>
Index: flashrom-pcidev_init_do_not_return_bar/ogp_spi.c
===================================================================
--- flashrom-pcidev_init_do_not_return_bar/ogp_spi.c (Revision 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.
*/
--
http://www.hailfinger.org/
More information about the flashrom
mailing list