diff -Nur linux-2.5.30-vanilla/arch/i386/pci/direct.c linux-2.5.30-patched/arch/i386/pci/direct.c
--- linux-2.5.30-vanilla/arch/i386/pci/direct.c Thu Aug 1 14:16:14 2002
+++ linux-2.5.30-patched/arch/i386/pci/direct.c Mon Aug 12 10:41:32 2002
@@ -69,76 +69,8 @@
return 0;
}
-
#undef PCI_CONF1_ADDRESS
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
-{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
-
- *value = (u8)data;
-
- return result;
-}
-
-static int pci_conf1_read_config_word(struct pci_dev *dev, int where, u16 *value)
-{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, &data);
-
- *value = (u16)data;
-
- return result;
-}
-
-static int pci_conf1_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
- if (!value)
- return -EINVAL;
-
- return pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf1_write_config_byte(struct pci_dev *dev, int where, u8 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, value);
-}
-
-static int pci_conf1_write_config_word(struct pci_dev *dev, int where, u16 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf1_write_config_dword(struct pci_dev *dev, int where, u32 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static struct pci_ops pci_direct_conf1 = {
- pci_conf1_read_config_byte,
- pci_conf1_read_config_word,
- pci_conf1_read_config_dword,
- pci_conf1_write_config_byte,
- pci_conf1_write_config_word,
- pci_conf1_write_config_dword
-};
/*
@@ -217,57 +149,70 @@
#undef PCI_CONF2_ADDRESS
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+
+
+static int pci_config_read_byte(struct pci_dev *dev, int where, u8 *value)
{
int result;
u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+
+ if (!value)
+ return -EINVAL;
+
+ result = pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 1, &data);
*value = (u8)data;
return result;
}
-static int pci_conf2_read_config_word(struct pci_dev *dev, int where, u16 *value)
+static int pci_config_read_word(struct pci_dev *dev, int where, u16 *value)
{
int result;
u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+
+ if (!value)
+ return -EINVAL;
+
+ result = pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 2, &data);
*value = (u16)data;
return result;
}
-static int pci_conf2_read_config_dword(struct pci_dev *dev, int where, u32 *value)
+static int pci_config_read_dword(struct pci_dev *dev, int where, u32 *value)
{
- return pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ if (!value)
+ return -EINVAL;
+
+ return pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 4, value);
}
-static int pci_conf2_write_config_byte(struct pci_dev *dev, int where, u8 value)
+static int pci_config_write_byte(struct pci_dev *dev, int where, u8 value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ return pci_config_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 1, value);
}
-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value)
+static int pci_config_write_word(struct pci_dev *dev, int where, u16 value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ return pci_config_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 2, value);
}
-static int pci_conf2_write_config_dword(struct pci_dev *dev, int where, u32 value)
+static int pci_config_write_dword(struct pci_dev *dev, int where, u32 value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ return pci_config_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 4, value);
}
-static struct pci_ops pci_direct_conf2 = {
- pci_conf2_read_config_byte,
- pci_conf2_read_config_word,
- pci_conf2_read_config_dword,
- pci_conf2_write_config_byte,
- pci_conf2_write_config_word,
- pci_conf2_write_config_dword
+static struct pci_ops pci_direct = {
+ pci_config_read_byte,
+ pci_config_read_word,
+ pci_config_read_dword,
+ pci_config_write_byte,
+ pci_config_write_word,
+ pci_config_write_dword
};
@@ -301,7 +246,7 @@
return 0;
}
-static struct pci_ops * __devinit pci_check_direct(void)
+static int __init pci_direct_init(void)
{
unsigned int tmp;
unsigned long flags;
@@ -312,17 +257,21 @@
* Check if configuration type 1 works.
*/
if (pci_probe & PCI_PROBE_CONF1) {
+ pci_config_read = pci_conf1_read;
+ pci_config_write = pci_conf1_write;
outb (0x01, 0xCFB);
tmp = inl (0xCF8);
outl (0x80000000, 0xCF8);
if (inl (0xCF8) == 0x80000000 &&
- pci_sanity_check(&pci_direct_conf1)) {
+ pci_sanity_check(&pci_direct)) {
outl (tmp, 0xCF8);
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 1\n");
if (!request_region(0xCF8, 8, "PCI conf1"))
- return NULL;
- return &pci_direct_conf1;
+ pci_root_ops = NULL;
+ else
+ pci_root_ops = &pci_direct;
+ return 0;
}
outl (tmp, 0xCF8);
}
@@ -331,36 +280,25 @@
* Check if configuration type 2 works.
*/
if (pci_probe & PCI_PROBE_CONF2) {
+ pci_config_read = pci_conf2_read;
+ pci_config_write = pci_conf2_write;
outb (0x00, 0xCFB);
outb (0x00, 0xCF8);
outb (0x00, 0xCFA);
if (inb (0xCF8) == 0x00 && inb (0xCFA) == 0x00 &&
- pci_sanity_check(&pci_direct_conf2)) {
+ pci_sanity_check(&pci_direct)) {
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 2\n");
if (!request_region(0xCF8, 4, "PCI conf2"))
- return NULL;
- return &pci_direct_conf2;
+ pci_root_ops = NULL;
+ else
+ pci_root_ops = &pci_direct;
+ return 0;
}
}
local_irq_restore(flags);
- return NULL;
-}
-
-static int __init pci_direct_init(void)
-{
- if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2))
- && (pci_root_ops = pci_check_direct())) {
- if (pci_root_ops == &pci_direct_conf1) {
- pci_config_read = pci_conf1_read;
- pci_config_write = pci_conf1_write;
- }
- else {
- pci_config_read = pci_conf2_read;
- pci_config_write = pci_conf2_write;
- }
- }
+ pci_root_ops = NULL;
return 0;
}
On Tue, 2002-08-13 at 01:08, Matthew Dobson wrote:
> - if (!value)
> - return -EINVAL;
> -
> - result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
> - PCI_FUNC(dev->devfn), where, 2, &data);
> -
This stil has the same problems as it did last time you posted it. The
pointless NULL check and the increased complexity over duplicating about
60 lines of code and having pci_conf1 ops cleanly as we do in 2.4.
The !value check is extremely bad because it turns a critical debuggable
software error into a silent unnoticed mistake.
Fixing the code instead of just resending it might improve the changes
of it being merged no end.
Alan
> On Tue, 2002-08-13 at 01:08, Matthew Dobson wrote:
>
>> - if (!value)
>> - return -EINVAL;
>> -
>> - result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
>> - PCI_FUNC(dev->devfn), where, 2, &data);
>> -
>
> This stil has the same problems as it did last time you posted it. The
> pointless NULL check and the increased complexity over duplicating about
> 60 lines of code and having pci_conf1 ops cleanly as we do in 2.4.
>
> The !value check is extremely bad because it turns a critical debuggable
> software error into a silent unnoticed mistake.
>
> Fixing the code instead of just resending it might improve the changes
> of it being merged no end.
Alan, please *look* at the patch. The NULL check is already
there, he's REMOVING about 60 lines of duplicated code,
reducing the complexity, and shifting the indirection up one
level to get rid of redundancy.
If you want to delete the NULL check as well, that's fine, but
totally a side issue. Ironically, the very snippet of code you
quoted is all prefaced with "-", no?
M.
> This stil has the same problems as it did last time you posted it. The
> pointless NULL check and the increased complexity over duplicating about
> 60 lines of code and having pci_conf1 ops cleanly as we do in 2.4.
>
> The !value check is extremely bad because it turns a critical debuggable
> software error into a silent unnoticed mistake.
>
> Fixing the code instead of just resending it might improve the changes
> of it being merged no end.
BTW, it wasn't just resent, it had all the NUMA-Q stuff stripped out
of it, and was just the core code cleanup. He's replacing
pci_conf1_read_config_byte and pci_conf2_read_config_byte
with pci_conf_read_config_byte. The indirection level has now switched
from pci_root_ops to pci_config_(read|write). These were set up before,
but never seemed to be actually used.
I'm perfectly prepared to accept that the patch doesn't work, or is
bad, but not for the reasons you gave. To make the thing easier to
read, what he's effectively doing is this
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
-{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
-
- *value = (u8)data;
-
- return result;
-}
-
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value)
- {
- int result;
- u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
- *value = (u8)data;
- return result;
- }
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+
+static int pci_config_read_byte(struct pci_dev *dev, int where, u8 *value)
+ {
+ int result;
+ u32 data;
+
+ if (!value)
+ return -EINVAL;
+
+ result = pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, 1, &data);
+ *value = (u8)data;
+ return result;
+}
On Tue, 2002-08-13 at 15:17, Martin J. Bligh wrote:
> Alan, please *look* at the patch. The NULL check is already
> there, he's REMOVING about 60 lines of duplicated code,
> reducing the complexity, and shifting the indirection up one
> level to get rid of redundancy.
>
> If you want to delete the NULL check as well, that's fine, but
> totally a side issue. Ironically, the very snippet of code you
> quoted is all prefaced with "-", no?
I pointed out before the null check was flawed. And all I see is the
same identical patch churned out again. Regardless of whether that
paticular stupid error was in the old code, not fixing it in the new
code when its pointed out is a bit of a mess.
I'm not sure its a simplification either. More function pointers don't
always make for neater - but thats a side issue. If the NULL check goes
I'm not too worried about the other stuff.
On Tue, 2002-08-13 at 15:55, Martin J. Bligh wrote:
> BTW, it wasn't just resent, it had all the NUMA-Q stuff stripped out
> of it, and was just the core code cleanup. He's replacing
> pci_conf1_read_config_byte and pci_conf2_read_config_byte
Apologies that part of the changing between the two diffs I didn't
notice. I've just stripped the value check out of 2.4 except for the
BIOS mode ones (they tend to produce -weird- tracebacks) where it
BUG()'s on that.
> I pointed out before the null check was flawed. And all I see is the
> same identical patch churned out again. Regardless of whether that
> paticular stupid error was in the old code, not fixing it in the new
> code when its pointed out is a bit of a mess.
>
> I'm not sure its a simplification either. More function pointers don't
> always make for neater - but thats a side issue. If the NULL check goes
> I'm not too worried about the other stuff.
OK, that's easily disposed of, and it'll remove even more lines of
redundant code.
Thanks,
M.
diff -Nur linux-2.5.31-vanilla/arch/i386/pci/direct.c linux-2.5.31-patched/arch/i386/pci/direct.c
--- linux-2.5.31-vanilla/arch/i386/pci/direct.c Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-patched/arch/i386/pci/direct.c Tue Aug 13 09:45:12 2002
@@ -69,76 +69,7 @@
return 0;
}
-
#undef PCI_CONF1_ADDRESS
-
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
-{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
-
- *value = (u8)data;
-
- return result;
-}
-
-static int pci_conf1_read_config_word(struct pci_dev *dev, int where, u16 *value)
-{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, &data);
-
- *value = (u16)data;
-
- return result;
-}
-
-static int pci_conf1_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
- if (!value)
- return -EINVAL;
-
- return pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf1_write_config_byte(struct pci_dev *dev, int where, u8 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, value);
-}
-
-static int pci_conf1_write_config_word(struct pci_dev *dev, int where, u16 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf1_write_config_dword(struct pci_dev *dev, int where, u32 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static struct pci_ops pci_direct_conf1 = {
- pci_conf1_read_config_byte,
- pci_conf1_read_config_word,
- pci_conf1_read_config_dword,
- pci_conf1_write_config_byte,
- pci_conf1_write_config_word,
- pci_conf1_write_config_dword
-};
/*
@@ -217,57 +149,58 @@
#undef PCI_CONF2_ADDRESS
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+
+static int pci_config_read_byte(struct pci_dev *dev, int where, u8 *value)
{
int result;
u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ result = pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 1, &data);
*value = (u8)data;
return result;
}
-static int pci_conf2_read_config_word(struct pci_dev *dev, int where, u16 *value)
+static int pci_config_read_word(struct pci_dev *dev, int where, u16 *value)
{
int result;
u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ result = pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 2, &data);
*value = (u16)data;
return result;
}
-static int pci_conf2_read_config_dword(struct pci_dev *dev, int where, u32 *value)
+static int pci_config_read_dword(struct pci_dev *dev, int where, u32 *value)
{
- return pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ return pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 4, value);
}
-static int pci_conf2_write_config_byte(struct pci_dev *dev, int where, u8 value)
+static int pci_config_write_byte(struct pci_dev *dev, int where, u8 value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ return pci_config_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 1, value);
}
-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value)
+static int pci_config_write_word(struct pci_dev *dev, int where, u16 value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ return pci_config_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 2, value);
}
-static int pci_conf2_write_config_dword(struct pci_dev *dev, int where, u32 value)
+static int pci_config_write_dword(struct pci_dev *dev, int where, u32 value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ return pci_config_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
PCI_FUNC(dev->devfn), where, 4, value);
}
-static struct pci_ops pci_direct_conf2 = {
- pci_conf2_read_config_byte,
- pci_conf2_read_config_word,
- pci_conf2_read_config_dword,
- pci_conf2_write_config_byte,
- pci_conf2_write_config_word,
- pci_conf2_write_config_dword
+static struct pci_ops pci_direct = {
+ pci_config_read_byte,
+ pci_config_read_word,
+ pci_config_read_dword,
+ pci_config_write_byte,
+ pci_config_write_word,
+ pci_config_write_dword
};
@@ -301,7 +235,7 @@
return 0;
}
-static struct pci_ops * __devinit pci_check_direct(void)
+static int __init pci_direct_init(void)
{
unsigned int tmp;
unsigned long flags;
@@ -312,17 +246,21 @@
* Check if configuration type 1 works.
*/
if (pci_probe & PCI_PROBE_CONF1) {
+ pci_config_read = pci_conf1_read;
+ pci_config_write = pci_conf1_write;
outb (0x01, 0xCFB);
tmp = inl (0xCF8);
outl (0x80000000, 0xCF8);
if (inl (0xCF8) == 0x80000000 &&
- pci_sanity_check(&pci_direct_conf1)) {
+ pci_sanity_check(&pci_direct)) {
outl (tmp, 0xCF8);
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 1\n");
if (!request_region(0xCF8, 8, "PCI conf1"))
- return NULL;
- return &pci_direct_conf1;
+ pci_root_ops = NULL;
+ else
+ pci_root_ops = &pci_direct;
+ return 0;
}
outl (tmp, 0xCF8);
}
@@ -331,36 +269,25 @@
* Check if configuration type 2 works.
*/
if (pci_probe & PCI_PROBE_CONF2) {
+ pci_config_read = pci_conf2_read;
+ pci_config_write = pci_conf2_write;
outb (0x00, 0xCFB);
outb (0x00, 0xCF8);
outb (0x00, 0xCFA);
if (inb (0xCF8) == 0x00 && inb (0xCFA) == 0x00 &&
- pci_sanity_check(&pci_direct_conf2)) {
+ pci_sanity_check(&pci_direct)) {
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 2\n");
if (!request_region(0xCF8, 4, "PCI conf2"))
- return NULL;
- return &pci_direct_conf2;
+ pci_root_ops = NULL;
+ else
+ pci_root_ops = &pci_direct;
+ return 0;
}
}
local_irq_restore(flags);
- return NULL;
-}
-
-static int __init pci_direct_init(void)
-{
- if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2))
- && (pci_root_ops = pci_check_direct())) {
- if (pci_root_ops == &pci_direct_conf1) {
- pci_config_read = pci_conf1_read;
- pci_config_write = pci_conf1_write;
- }
- else {
- pci_config_read = pci_conf2_read;
- pci_config_write = pci_conf2_write;
- }
- }
+ pci_root_ops = NULL;
return 0;
}
On Tue, 13 Aug 2002, Matthew Dobson wrote:
>
> I think that it is definitely a simplification, although I am a bit biased ;)
> It makes it easier for other configuration types to hook into the system as
> well (I'm partial to NUMA-Q as well ;). All they have to do is hijack the
> pci_config_(read|write) function pointers.
Hmm..
Quite frankly, to me it looks like the real issue is that you don't want
to have the byte/word/dword stuff replicated three times.
And your cleanup avoids the replication, but I think it has other badness
in it: in particular it depends on those two global pointers. Which makes
it really hard to have (for example) per-segment functions, or hard for
drivers to hook into it (there's one IDE driver in particular that wants
to do conf2 accesses, and nothing else. So it duplicates its own conf2
functions right now, because it has no way to hook into the generic ones).
If that is the case, wouldn't the _real_ cleanup be to just change the
format of pci_config_read() entirely, and just make it get the size.
Another way of saying this:
Right now the config interface is all based around having unique
functions for all sizes. So callers do "pci_read_config_word()" and that
name-based size calling convention is propagated all the way down.
Your patch multiplexes the sizes at the lowest level.
And I'd suggest multiplexing them at a higher level. Instead of
six different pcibios_read_config_byte etc functions, move the
multiplexing up, make make them just two functions at _that_ level (and
make siz #defines to make it compatible with existing users).
Ehh?
Linus
> Quite frankly, to me it looks like the real issue is that you don't want
> to have the byte/word/dword stuff replicated three times.
Exactly - we can do that, but it just seems messy.
> And your cleanup avoids the replication, but I think it has other badness
> in it: in particular it depends on those two global pointers. Which makes
> it really hard to have (for example) per-segment functions, or hard for
> drivers to hook into it (there's one IDE driver in particular that wants
> to do conf2 accesses, and nothing else. So it duplicates its own conf2
> functions right now, because it has no way to hook into the generic ones).
OK, that IDE thing smacks of unmitigated evil to me, but if things are relying
on it, we shouldn't change it.
> And I'd suggest multiplexing them at a higher level. Instead of
> six different pcibios_read_config_byte etc functions, move the
> multiplexing up, make make them just two functions at _that_ level (and
> make siz #defines to make it compatible with existing users).
Yup, that sounds like ultimately the correct thing to do. Will try to get
that way done instead. Should clean things up nicely ....
M.
On Tue, 2002-08-13 at 20:57, Martin J. Bligh wrote:
> > to do conf2 accesses, and nothing else. So it duplicates its own conf2
> > functions right now, because it has no way to hook into the generic ones).
>
> OK, that IDE thing smacks of unmitigated evil to me, but if things are relying
> on it, we shouldn't change it.
It wants to force its own conf1/conf2 over the BIOS even if BIOS is
preferred because some BIOSes dont honour the size requested and the
hardware has bugs.
That to me says there may well be cleaner approaches.
Alan
On 13 Aug 2002, Alan Cox wrote:
> >
> > OK, that IDE thing smacks of unmitigated evil to me, but if things are relying
> > on it, we shouldn't change it.
>
> It wants to force its own conf1/conf2 over the BIOS even if BIOS is
> preferred because some BIOSes dont honour the size requested and the
> hardware has bugs.
>
> That to me says there may well be cleaner approaches.
The thing I liked about the separate structures for function pointers for
conf1/conf2 is that I could at least _see_ that the IDE driver might some
day be changed to just do
..
conf2_struct->pci_config_read_byte(..)
..
even if (judging by past performance) this would never happen ;)
This is why I'd like to continue with the notion of having a well-defined
structure that contains all the pointers (and one default case). Now,
shrinking those structures down to 2 entries instead of 6 sounds like a
fine idea to me, but short-circuiting them internally sounds bad because
it loses the ability to use the pci config space functions independently
of each other.
Linus
diff -Nur linux-2.5.31-vanilla/arch/i386/pci/direct.c linux-2.5.31-patched/arch/i386/pci/direct.c
--- linux-2.5.31-vanilla/arch/i386/pci/direct.c Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-patched/arch/i386/pci/direct.c Tue Aug 13 15:22:03 2002
@@ -69,75 +69,23 @@
return 0;
}
-
#undef PCI_CONF1_ADDRESS
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+static int pci_conf1_read_size_indep(struct pci_dev *dev, int where, int size, u32 *value)
{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
-
- *value = (u8)data;
-
- return result;
-}
-
-static int pci_conf1_read_config_word(struct pci_dev *dev, int where, u16 *value)
-{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, &data);
-
- *value = (u16)data;
-
- return result;
-}
-
-static int pci_conf1_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
- if (!value)
- return -EINVAL;
-
return pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
+ PCI_FUNC(dev->devfn), where, size, value);
}
-static int pci_conf1_write_config_byte(struct pci_dev *dev, int where, u8 value)
+static int pci_conf1_write_size_indep(struct pci_dev *dev, int where, int size, u32 value)
{
return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, value);
-}
-
-static int pci_conf1_write_config_word(struct pci_dev *dev, int where, u16 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf1_write_config_dword(struct pci_dev *dev, int where, u32 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
+ PCI_FUNC(dev->devfn), where, size, value);
}
static struct pci_ops pci_direct_conf1 = {
- pci_conf1_read_config_byte,
- pci_conf1_read_config_word,
- pci_conf1_read_config_dword,
- pci_conf1_write_config_byte,
- pci_conf1_write_config_word,
- pci_conf1_write_config_dword
+ pci_conf1_read_size_indep,
+ pci_conf1_write_size_indep
};
@@ -217,57 +165,21 @@
#undef PCI_CONF2_ADDRESS
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value)
-{
- int result;
- u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
- *value = (u8)data;
- return result;
-}
-
-static int pci_conf2_read_config_word(struct pci_dev *dev, int where, u16 *value)
-{
- int result;
- u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, &data);
- *value = (u16)data;
- return result;
-}
-
-static int pci_conf2_read_config_dword(struct pci_dev *dev, int where, u32 *value)
+static int pci_conf2_read_size_indep(struct pci_dev *dev, int where, int size, u32 *value)
{
return pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf2_write_config_byte(struct pci_dev *dev, int where, u8 value)
-{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, value);
+ PCI_FUNC(dev->devfn), where, size, value);
}
-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value)
+static int pci_conf2_write_size_indep(struct pci_dev *dev, int where, int size, u32 value)
{
return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf2_write_config_dword(struct pci_dev *dev, int where, u32 value)
-{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
+ PCI_FUNC(dev->devfn), where, size, value);
}
static struct pci_ops pci_direct_conf2 = {
- pci_conf2_read_config_byte,
- pci_conf2_read_config_word,
- pci_conf2_read_config_dword,
- pci_conf2_write_config_byte,
- pci_conf2_write_config_word,
- pci_conf2_write_config_dword
+ pci_conf2_read_size_indep,
+ pci_conf2_write_size_indep
};
@@ -292,16 +204,16 @@
bus.number = 0;
dev.bus = &bus;
for(dev.devfn=0; dev.devfn < 0x100; dev.devfn++)
- if ((!o->read_word(&dev, PCI_CLASS_DEVICE, &x) &&
+ if ((!o->read(&dev, PCI_CLASS_DEVICE, 2, &x) &&
(x == PCI_CLASS_BRIDGE_HOST || x == PCI_CLASS_DISPLAY_VGA)) ||
- (!o->read_word(&dev, PCI_VENDOR_ID, &x) &&
+ (!o->read(&dev, PCI_VENDOR_ID, 2, &x) &&
(x == PCI_VENDOR_ID_INTEL || x == PCI_VENDOR_ID_COMPAQ)))
return 1;
DBG("PCI: Sanity check failed\n");
return 0;
}
-static struct pci_ops * __devinit pci_check_direct(void)
+static int __init pci_direct_init(void)
{
unsigned int tmp;
unsigned long flags;
@@ -312,6 +224,8 @@
* Check if configuration type 1 works.
*/
if (pci_probe & PCI_PROBE_CONF1) {
+ pci_config_read = pci_conf1_read;
+ pci_config_write = pci_conf1_write;
outb (0x01, 0xCFB);
tmp = inl (0xCF8);
outl (0x80000000, 0xCF8);
@@ -321,8 +235,10 @@
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 1\n");
if (!request_region(0xCF8, 8, "PCI conf1"))
- return NULL;
- return &pci_direct_conf1;
+ pci_root_ops = NULL;
+ else
+ pci_root_ops = &pci_direct_conf1;
+ return 0;
}
outl (tmp, 0xCF8);
}
@@ -331,6 +247,8 @@
* Check if configuration type 2 works.
*/
if (pci_probe & PCI_PROBE_CONF2) {
+ pci_config_read = pci_conf2_read;
+ pci_config_write = pci_conf2_write;
outb (0x00, 0xCFB);
outb (0x00, 0xCF8);
outb (0x00, 0xCFA);
@@ -339,28 +257,15 @@
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 2\n");
if (!request_region(0xCF8, 4, "PCI conf2"))
- return NULL;
- return &pci_direct_conf2;
+ pci_root_ops = NULL;
+ else
+ pci_root_ops = &pci_direct_conf2;
+ return 0;
}
}
local_irq_restore(flags);
- return NULL;
-}
-
-static int __init pci_direct_init(void)
-{
- if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2))
- && (pci_root_ops = pci_check_direct())) {
- if (pci_root_ops == &pci_direct_conf1) {
- pci_config_read = pci_conf1_read;
- pci_config_write = pci_conf1_write;
- }
- else {
- pci_config_read = pci_conf2_read;
- pci_config_write = pci_conf2_write;
- }
- }
+ pci_root_ops = NULL;
return 0;
}
diff -Nur linux-2.5.31-vanilla/drivers/pci/access.c linux-2.5.31-patched/drivers/pci/access.c
--- linux-2.5.31-vanilla/drivers/pci/access.c Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-patched/drivers/pci/access.c Tue Aug 13 15:09:56 2002
@@ -19,24 +19,38 @@
#define PCI_word_BAD (pos & 1)
#define PCI_dword_BAD (pos & 3)
-#define PCI_OP(rw,size,type) \
-int pci_##rw##_config_##size (struct pci_dev *dev, int pos, type value) \
+#define PCI_OP_READ(size,type,len) \
+int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \
{ \
int res; \
unsigned long flags; \
+ u32 data; \
if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
spin_lock_irqsave(&pci_lock, flags); \
- res = dev->bus->ops->rw##_##size(dev, pos, value); \
+ res = dev->bus->ops->read(dev, pos, len, &data); \
+ *value = (type)data; \
spin_unlock_irqrestore(&pci_lock, flags); \
return res; \
}
-PCI_OP(read, byte, u8 *)
-PCI_OP(read, word, u16 *)
-PCI_OP(read, dword, u32 *)
-PCI_OP(write, byte, u8)
-PCI_OP(write, word, u16)
-PCI_OP(write, dword, u32)
+#define PCI_OP_WRITE(size,type,len) \
+int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \
+{ \
+ int res; \
+ unsigned long flags; \
+ if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
+ spin_lock_irqsave(&pci_lock, flags); \
+ res = dev->bus->ops->write(dev, pos, len, value); \
+ spin_unlock_irqrestore(&pci_lock, flags); \
+ return res; \
+}
+
+PCI_OP_READ(byte, u8, 1)
+PCI_OP_READ(word, u16, 2)
+PCI_OP_READ(dword, u32, 4)
+PCI_OP_WRITE(byte, u8, 1)
+PCI_OP_WRITE(word, u16, 2)
+PCI_OP_WRITE(dword, u32, 4)
EXPORT_SYMBOL(pci_read_config_byte);
EXPORT_SYMBOL(pci_read_config_word);
diff -Nur linux-2.5.31-vanilla/include/linux/pci.h linux-2.5.31-patched/include/linux/pci.h
--- linux-2.5.31-vanilla/include/linux/pci.h Sat Aug 10 18:41:28 2002
+++ linux-2.5.31-patched/include/linux/pci.h Tue Aug 13 14:49:36 2002
@@ -456,12 +456,8 @@
/* Low-level architecture-dependent routines */
struct pci_ops {
- int (*read_byte)(struct pci_dev *, int where, u8 *val);
- int (*read_word)(struct pci_dev *, int where, u16 *val);
- int (*read_dword)(struct pci_dev *, int where, u32 *val);
- int (*write_byte)(struct pci_dev *, int where, u8 val);
- int (*write_word)(struct pci_dev *, int where, u16 val);
- int (*write_dword)(struct pci_dev *, int where, u32 val);
+ int (*read)(struct pci_dev *, int where, int size, u32 *val);
+ int (*write)(struct pci_dev *, int where, int size, u32 val);
};
struct pbus_set_ranges_data
On Tue, 13 Aug 2002, Matthew Dobson wrote:
> I think I'm getting what you're aiming for here. See if this patch comes close
> to what you're looking for.
This seems to be more what I was thinking of, yes (although please drop
the "size_independent" part of the names, that just looks too horrible ;)
Linus
Linus Torvalds wrote:
> On Tue, 13 Aug 2002, Matthew Dobson wrote:
>
>> I think I'm getting what you're aiming for here. See if this patch comes close
>>to what you're looking for.
>
> This seems to be more what I was thinking of, yes (although please drop
> the "size_independent" part of the names, that just looks too horrible ;)
Fair enough.. Couldn't think of a fun creative name anyway... Just took the
road more traveled and renamed the old pci_conf1_read __pci_conf1_read and the
old pci_conf1_read_size_indep pci_conf1_read. Much prettier. And it even
seems to work on i386. I definitely wouldn't try booting this on any other
arch's though! ;)
-Matt
diff -Nur linux-2.5.31-vanilla/arch/i386/pci/direct.c
linux-2.5.31-patched/arch/i386/pci/direct.c
--- linux-2.5.31-vanilla/arch/i386/pci/direct.c Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-patched/arch/i386/pci/direct.c Tue Aug 13 16:14:40 2002
@@ -13,7 +13,7 @@
#define PCI_CONF1_ADDRESS(bus, dev, fn, reg) \
(0x80000000 | (bus << 16) | (dev << 11) | (fn << 8) | (reg & ~3))
-static int pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int
len, u32 *value)
+static int __pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int
len, u32 *value)
{
unsigned long flags;
@@ -41,7 +41,7 @@
return 0;
}
-static int pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int
len, u32 value)
+static int __pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int
len, u32 value)
{
unsigned long flags;
@@ -69,75 +69,23 @@
return 0;
}
-
#undef PCI_CONF1_ADDRESS
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+static int pci_conf1_read(struct pci_dev *dev, int where, int size, u32 *value)
{
-
int result;
-
u32 data;
-
-
if (!value)
-
return -EINVAL;
-
-
result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 1, &data);
-
-
*value = (u8)data;
-
-
return result;
+
return __pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+
PCI_FUNC(dev->devfn), where, size, value);
}
-static int pci_conf1_read_config_word(struct pci_dev *dev, int where, u16 *value)
+static int pci_conf1_write(struct pci_dev *dev, int where, int size, u32 value)
{
-
int result;
-
u32 data;
-
-
if (!value)
-
return -EINVAL;
-
-
result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 2, &data);
-
-
*value = (u16)data;
-
-
return result;
-}
-
-static int pci_conf1_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
-
if (!value)
-
return -EINVAL;
-
-
return pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf1_write_config_byte(struct pci_dev *dev, int where, u8 value)
-{
-
return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 1, value);
-}
-
-static int pci_conf1_write_config_word(struct pci_dev *dev, int where, u16 value)
-{
-
return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf1_write_config_dword(struct pci_dev *dev, int where, u32 value)
-{
-
return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 4, value);
+
return __pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+
PCI_FUNC(dev->devfn), where, size, value);
}
static struct pci_ops pci_direct_conf1 = {
-
pci_conf1_read_config_byte,
-
pci_conf1_read_config_word,
-
pci_conf1_read_config_dword,
-
pci_conf1_write_config_byte,
-
pci_conf1_write_config_word,
-
pci_conf1_write_config_dword
+
read:
pci_conf1_read,
+
write:
pci_conf1_write
};
@@ -147,7 +95,7 @@
#define PCI_CONF2_ADDRESS(dev, reg) (u16)(0xC000 | (dev << 8) | reg)
-static int pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int
len, u32 *value)
+static int __pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int
len, u32 *value)
{
unsigned long flags;
@@ -181,7 +129,7 @@
return 0;
}
-static int pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int
len, u32 value)
+static int __pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int
len, u32 value)
{
unsigned long flags;
@@ -217,57 +165,21 @@
#undef PCI_CONF2_ADDRESS
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value)
-{
-
int result;
-
u32 data;
-
result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 1, &data);
-
*value = (u8)data;
-
return result;
-}
-
-static int pci_conf2_read_config_word(struct pci_dev *dev, int where, u16 *value)
-{
-
int result;
-
u32 data;
-
result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 2, &data);
-
*value = (u16)data;
-
return result;
-}
-
-static int pci_conf2_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
-
return pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf2_write_config_byte(struct pci_dev *dev, int where, u8 value)
+static int pci_conf2_read(struct pci_dev *dev, int where, int size, u32 *value)
{
-
return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 1, value);
+
return __pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+
PCI_FUNC(dev->devfn), where, size, value);
}
-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value)
+static int pci_conf2_write(struct pci_dev *dev, int where, int size, u32 value)
{
-
return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf2_write_config_dword(struct pci_dev *dev, int where, u32 value)
-{
-
return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 4, value);
+
return __pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+
PCI_FUNC(dev->devfn), where, size, value);
}
static struct pci_ops pci_direct_conf2 = {
-
pci_conf2_read_config_byte,
-
pci_conf2_read_config_word,
-
pci_conf2_read_config_dword,
-
pci_conf2_write_config_byte,
-
pci_conf2_write_config_word,
-
pci_conf2_write_config_dword
+
read:
pci_conf2_read,
+
write:
pci_conf2_write
};
@@ -292,16 +204,16 @@
bus.number = 0;
dev.bus = &bus;
for(dev.devfn=0; dev.devfn < 0x100; dev.devfn++)
-
if ((!o->read_word(&dev, PCI_CLASS_DEVICE, &x) &&
+
if ((!o->read(&dev, PCI_CLASS_DEVICE, 2, &x) &&
(x == PCI_CLASS_BRIDGE_HOST || x == PCI_CLASS_DISPLAY_VGA)) ||
-
(!o->read_word(&dev, PCI_VENDOR_ID, &x) &&
+
(!o->read(&dev, PCI_VENDOR_ID, 2, &x) &&
(x == PCI_VENDOR_ID_INTEL || x == PCI_VENDOR_ID_COMPAQ)))
return 1;
DBG("PCI: Sanity check failed\n");
return 0;
}
-static struct pci_ops * __devinit pci_check_direct(void)
+static int __init pci_direct_init(void)
{
unsigned int tmp;
unsigned long flags;
@@ -321,8 +233,10 @@
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 1\n");
if (!request_region(0xCF8, 8, "PCI conf1"))
-
return NULL;
-
return &pci_direct_conf1;
+
pci_root_ops = NULL;
+
else
+
pci_root_ops = &pci_direct_conf1;
+
return 0;
}
outl (tmp, 0xCF8);
}
@@ -339,28 +253,15 @@
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 2\n");
if (!request_region(0xCF8, 4, "PCI conf2"))
-
return NULL;
-
return &pci_direct_conf2;
+
pci_root_ops = NULL;
+
else
+
pci_root_ops = &pci_direct_conf2;
+
return 0;
}
}
local_irq_restore(flags);
-
return NULL;
-}
-
-static int __init pci_direct_init(void)
-{
-
if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2))
-
&& (pci_root_ops = pci_check_direct())) {
-
if (pci_root_ops == &pci_direct_conf1) {
-
pci_config_read = pci_conf1_read;
-
pci_config_write = pci_conf1_write;
-
}
-
else {
-
pci_config_read = pci_conf2_read;
-
pci_config_write = pci_conf2_write;
-
}
-
}
+
pci_root_ops = NULL;
return 0;
}
diff -Nur linux-2.5.31-vanilla/drivers/pci/access.c
linux-2.5.31-patched/drivers/pci/access.c
--- linux-2.5.31-vanilla/drivers/pci/access.c Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-patched/drivers/pci/access.c Tue Aug 13 15:09:56 2002
@@ -19,24 +19,38 @@
#define PCI_word_BAD (pos & 1)
#define PCI_dword_BAD (pos & 3)
-#define PCI_OP(rw,size,type) \
-int pci_##rw##_config_##size (struct pci_dev *dev, int pos, type value) \
+#define PCI_OP_READ(size,type,len) \
+int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \
{
\
int res;
\
unsigned long flags; \
+
u32 data;
\
if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
spin_lock_irqsave(&pci_lock, flags); \
-
res = dev->bus->ops->rw##_##size(dev, pos, value); \
+
res = dev->bus->ops->read(dev, pos, len, &data); \
+
*value = (type)data; \
spin_unlock_irqrestore(&pci_lock, flags); \
return res; \
}
-PCI_OP(read, byte, u8 *)
-PCI_OP(read, word, u16 *)
-PCI_OP(read, dword, u32 *)
-PCI_OP(write, byte, u8)
-PCI_OP(write, word, u16)
-PCI_OP(write, dword, u32)
+#define PCI_OP_WRITE(size,type,len) \
+int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \
+{
\
+
int res;
\
+
unsigned long flags; \
+
if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
+
spin_lock_irqsave(&pci_lock, flags); \
+
res = dev->bus->ops->write(dev, pos, len, value); \
+
spin_unlock_irqrestore(&pci_lock, flags); \
+
return res; \
+}
+
+PCI_OP_READ(byte, u8, 1)
+PCI_OP_READ(word, u16, 2)
+PCI_OP_READ(dword, u32, 4)
+PCI_OP_WRITE(byte, u8, 1)
+PCI_OP_WRITE(word, u16, 2)
+PCI_OP_WRITE(dword, u32, 4)
EXPORT_SYMBOL(pci_read_config_byte);
EXPORT_SYMBOL(pci_read_config_word);
diff -Nur linux-2.5.31-vanilla/include/linux/pci.h
linux-2.5.31-patched/include/linux/pci.h
--- linux-2.5.31-vanilla/include/linux/pci.h Sat Aug 10 18:41:28 2002
+++ linux-2.5.31-patched/include/linux/pci.h Tue Aug 13 14:49:36 2002
@@ -456,12 +456,8 @@
/* Low-level architecture-dependent routines */
struct pci_ops {
-
int (*read_byte)(struct pci_dev *, int where, u8 *val);
-
int (*read_word)(struct pci_dev *, int where, u16 *val);
-
int (*read_dword)(struct pci_dev *, int where, u32 *val);
-
int (*write_byte)(struct pci_dev *, int where, u8 val);
-
int (*write_word)(struct pci_dev *, int where, u16 val);
-
int (*write_dword)(struct pci_dev *, int where, u32 val);
+
int (*read)(struct pci_dev *, int where, int size, u32 *val);
+
int (*write)(struct pci_dev *, int where, int size, u32 val);
};
struct pbus_set_ranges_data
Hi!
> It wants to force its own conf1/conf2 over the BIOS even if BIOS is
> preferred because some BIOSes dont honour the size requested and the
> hardware has bugs.
>
> That to me says there may well be cleaner approaches.
I haven't looked at the current source yet, but the PCI access code
used to work this way: if both BIOS and direct access were enabled
(which was the default) and both worked, BIOS was used only for getting
information about interrupt routing and last bus number and direct
access for everything else. Hence the problematic BIOS calls were used
only if there were no other possibility or the user explicitly told
us to.
Have a nice fortnight
--
Martin `MJ' Mares <[email protected]> http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
No fortune, no luck, no sig!
diff -Nur linux-2.5.31-vanilla/arch/i386/pci/common.c linux-2.5.31-patched/arch/i386/pci/common.c
--- linux-2.5.31-vanilla/arch/i386/pci/common.c Sat Aug 10 18:41:27 2002
+++ linux-2.5.31-patched/arch/i386/pci/common.c Wed Aug 14 10:57:40 2002
@@ -25,9 +25,6 @@
struct pci_bus *pci_root_bus = NULL;
struct pci_ops *pci_root_ops = NULL;
-int (*pci_config_read)(int seg, int bus, int dev, int fn, int reg, int len, u32 *value) = NULL;
-int (*pci_config_write)(int seg, int bus, int dev, int fn, int reg, int len, u32 value) = NULL;
-
/*
* legacy, numa, and acpi all want to call pcibios_scan_root
* from their initcalls. This flag prevents that.
diff -Nur linux-2.5.31-vanilla/arch/i386/pci/direct.c linux-2.5.31-patched/arch/i386/pci/direct.c
--- linux-2.5.31-vanilla/arch/i386/pci/direct.c Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-patched/arch/i386/pci/direct.c Tue Aug 13 16:28:13 2002
@@ -13,7 +13,7 @@
#define PCI_CONF1_ADDRESS(bus, dev, fn, reg) \
(0x80000000 | (bus << 16) | (dev << 11) | (fn << 8) | (reg & ~3))
-static int pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+static int __pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
{
unsigned long flags;
@@ -41,7 +41,7 @@
return 0;
}
-static int pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+static int __pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
{
unsigned long flags;
@@ -69,75 +69,23 @@
return 0;
}
-
#undef PCI_CONF1_ADDRESS
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+static int pci_conf1_read(struct pci_dev *dev, int where, int size, u32 *value)
{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
-
- *value = (u8)data;
-
- return result;
+ return __pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, size, value);
}
-static int pci_conf1_read_config_word(struct pci_dev *dev, int where, u16 *value)
+static int pci_conf1_write(struct pci_dev *dev, int where, int size, u32 value)
{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, &data);
-
- *value = (u16)data;
-
- return result;
-}
-
-static int pci_conf1_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
- if (!value)
- return -EINVAL;
-
- return pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf1_write_config_byte(struct pci_dev *dev, int where, u8 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, value);
-}
-
-static int pci_conf1_write_config_word(struct pci_dev *dev, int where, u16 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf1_write_config_dword(struct pci_dev *dev, int where, u32 value)
-{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
+ return __pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, size, value);
}
static struct pci_ops pci_direct_conf1 = {
- pci_conf1_read_config_byte,
- pci_conf1_read_config_word,
- pci_conf1_read_config_dword,
- pci_conf1_write_config_byte,
- pci_conf1_write_config_word,
- pci_conf1_write_config_dword
+ read: pci_conf1_read,
+ write: pci_conf1_write
};
@@ -147,7 +95,7 @@
#define PCI_CONF2_ADDRESS(dev, reg) (u16)(0xC000 | (dev << 8) | reg)
-static int pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+static int __pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
{
unsigned long flags;
@@ -181,7 +129,7 @@
return 0;
}
-static int pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+static int __pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
{
unsigned long flags;
@@ -217,57 +165,21 @@
#undef PCI_CONF2_ADDRESS
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value)
-{
- int result;
- u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
- *value = (u8)data;
- return result;
-}
-
-static int pci_conf2_read_config_word(struct pci_dev *dev, int where, u16 *value)
-{
- int result;
- u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, &data);
- *value = (u16)data;
- return result;
-}
-
-static int pci_conf2_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
- return pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf2_write_config_byte(struct pci_dev *dev, int where, u8 value)
+static int pci_conf2_read(struct pci_dev *dev, int where, int size, u32 *value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, value);
+ return __pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, size, value);
}
-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value)
+static int pci_conf2_write(struct pci_dev *dev, int where, int size, u32 value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf2_write_config_dword(struct pci_dev *dev, int where, u32 value)
-{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
+ return __pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, size, value);
}
static struct pci_ops pci_direct_conf2 = {
- pci_conf2_read_config_byte,
- pci_conf2_read_config_word,
- pci_conf2_read_config_dword,
- pci_conf2_write_config_byte,
- pci_conf2_write_config_word,
- pci_conf2_write_config_dword
+ read: pci_conf2_read,
+ write: pci_conf2_write
};
@@ -283,7 +195,7 @@
*/
static int __devinit pci_sanity_check(struct pci_ops *o)
{
- u16 x;
+ u32 x;
struct pci_bus bus; /* Fake bus and device */
struct pci_dev dev;
@@ -292,16 +204,16 @@
bus.number = 0;
dev.bus = &bus;
for(dev.devfn=0; dev.devfn < 0x100; dev.devfn++)
- if ((!o->read_word(&dev, PCI_CLASS_DEVICE, &x) &&
+ if ((!o->read(&dev, PCI_CLASS_DEVICE, 2, &x) &&
(x == PCI_CLASS_BRIDGE_HOST || x == PCI_CLASS_DISPLAY_VGA)) ||
- (!o->read_word(&dev, PCI_VENDOR_ID, &x) &&
+ (!o->read(&dev, PCI_VENDOR_ID, 2, &x) &&
(x == PCI_VENDOR_ID_INTEL || x == PCI_VENDOR_ID_COMPAQ)))
return 1;
DBG("PCI: Sanity check failed\n");
return 0;
}
-static struct pci_ops * __devinit pci_check_direct(void)
+static int __init pci_direct_init(void)
{
unsigned int tmp;
unsigned long flags;
@@ -321,8 +233,10 @@
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 1\n");
if (!request_region(0xCF8, 8, "PCI conf1"))
- return NULL;
- return &pci_direct_conf1;
+ pci_root_ops = NULL;
+ else
+ pci_root_ops = &pci_direct_conf1;
+ return 0;
}
outl (tmp, 0xCF8);
}
@@ -339,28 +253,15 @@
local_irq_restore(flags);
printk(KERN_INFO "PCI: Using configuration type 2\n");
if (!request_region(0xCF8, 4, "PCI conf2"))
- return NULL;
- return &pci_direct_conf2;
+ pci_root_ops = NULL;
+ else
+ pci_root_ops = &pci_direct_conf2;
+ return 0;
}
}
local_irq_restore(flags);
- return NULL;
-}
-
-static int __init pci_direct_init(void)
-{
- if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2))
- && (pci_root_ops = pci_check_direct())) {
- if (pci_root_ops == &pci_direct_conf1) {
- pci_config_read = pci_conf1_read;
- pci_config_write = pci_conf1_write;
- }
- else {
- pci_config_read = pci_conf2_read;
- pci_config_write = pci_conf2_write;
- }
- }
+ pci_root_ops = NULL;
return 0;
}
diff -Nur linux-2.5.31-vanilla/arch/i386/pci/pcbios.c linux-2.5.31-patched/arch/i386/pci/pcbios.c
--- linux-2.5.31-vanilla/arch/i386/pci/pcbios.c Sat Aug 10 18:41:19 2002
+++ linux-2.5.31-patched/arch/i386/pci/pcbios.c Wed Aug 14 10:55:20 2002
@@ -185,7 +185,7 @@
return (int) (ret & 0xff00) >> 8;
}
-static int pci_bios_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+static int __pci_bios_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
{
unsigned long result = 0;
unsigned long flags;
@@ -240,7 +240,7 @@
return (int)((result & 0xff00) >> 8);
}
-static int pci_bios_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+static int __pci_bios_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
{
unsigned long result = 0;
unsigned long flags;
@@ -295,63 +295,16 @@
return (int)((result & 0xff00) >> 8);
}
-static int pci_bios_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+static int pci_bios_read(struct pci_dev *dev, int where, int size, u32 *value)
{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
-
- *value = (u8)data;
-
- return result;
-}
-
-static int pci_bios_read_config_word(struct pci_dev *dev, int where, u16 *value)
-{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, &data);
-
- *value = (u16)data;
-
- return result;
-}
-
-static int pci_bios_read_config_dword(struct pci_dev *dev, int where, u32 *value)
-{
- if (!value)
- return -EINVAL;
-
- return pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_bios_write_config_byte(struct pci_dev *dev, int where, u8 value)
-{
- return pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, value);
-}
-
-static int pci_bios_write_config_word(struct pci_dev *dev, int where, u16 value)
-{
- return pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 2, value);
+ return __pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, size, value);
}
-static int pci_bios_write_config_dword(struct pci_dev *dev, int where, u32 value)
+static int pci_bios_write(struct pci_dev *dev, int where, int size, u32 value)
{
- return pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 4, value);
+ return __pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, size, value);
}
@@ -360,12 +313,8 @@
*/
static struct pci_ops pci_bios_access = {
- pci_bios_read_config_byte,
- pci_bios_read_config_word,
- pci_bios_read_config_dword,
- pci_bios_write_config_byte,
- pci_bios_write_config_word,
- pci_bios_write_config_dword
+ read: pci_bios_read,
+ write: pci_bios_write
};
/*
@@ -551,8 +500,6 @@
&& ((pci_root_ops = pci_find_bios()))) {
pci_probe |= PCI_BIOS_SORT;
pci_bios_present = 1;
- pci_config_read = pci_bios_read;
- pci_config_write = pci_bios_write;
}
return 0;
}
diff -Nur linux-2.5.31-vanilla/drivers/pci/access.c linux-2.5.31-patched/drivers/pci/access.c
--- linux-2.5.31-vanilla/drivers/pci/access.c Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-patched/drivers/pci/access.c Tue Aug 13 15:09:56 2002
@@ -19,24 +19,38 @@
#define PCI_word_BAD (pos & 1)
#define PCI_dword_BAD (pos & 3)
-#define PCI_OP(rw,size,type) \
-int pci_##rw##_config_##size (struct pci_dev *dev, int pos, type value) \
+#define PCI_OP_READ(size,type,len) \
+int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \
{ \
int res; \
unsigned long flags; \
+ u32 data; \
if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
spin_lock_irqsave(&pci_lock, flags); \
- res = dev->bus->ops->rw##_##size(dev, pos, value); \
+ res = dev->bus->ops->read(dev, pos, len, &data); \
+ *value = (type)data; \
spin_unlock_irqrestore(&pci_lock, flags); \
return res; \
}
-PCI_OP(read, byte, u8 *)
-PCI_OP(read, word, u16 *)
-PCI_OP(read, dword, u32 *)
-PCI_OP(write, byte, u8)
-PCI_OP(write, word, u16)
-PCI_OP(write, dword, u32)
+#define PCI_OP_WRITE(size,type,len) \
+int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \
+{ \
+ int res; \
+ unsigned long flags; \
+ if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
+ spin_lock_irqsave(&pci_lock, flags); \
+ res = dev->bus->ops->write(dev, pos, len, value); \
+ spin_unlock_irqrestore(&pci_lock, flags); \
+ return res; \
+}
+
+PCI_OP_READ(byte, u8, 1)
+PCI_OP_READ(word, u16, 2)
+PCI_OP_READ(dword, u32, 4)
+PCI_OP_WRITE(byte, u8, 1)
+PCI_OP_WRITE(word, u16, 2)
+PCI_OP_WRITE(dword, u32, 4)
EXPORT_SYMBOL(pci_read_config_byte);
EXPORT_SYMBOL(pci_read_config_word);
diff -Nur linux-2.5.31-vanilla/include/asm-i386/pci.h linux-2.5.31-patched/include/asm-i386/pci.h
--- linux-2.5.31-vanilla/include/asm-i386/pci.h Sat Aug 10 18:41:27 2002
+++ linux-2.5.31-patched/include/asm-i386/pci.h Wed Aug 14 10:58:52 2002
@@ -22,8 +22,6 @@
void pcibios_config_init(void);
struct pci_bus * pcibios_scan_root(int bus);
-extern int (*pci_config_read)(int seg, int bus, int dev, int fn, int reg, int len, u32 *value);
-extern int (*pci_config_write)(int seg, int bus, int dev, int fn, int reg, int len, u32 value);
void pcibios_set_master(struct pci_dev *dev);
void pcibios_penalize_isa_irq(int irq);
diff -Nur linux-2.5.31-vanilla/include/linux/pci.h linux-2.5.31-patched/include/linux/pci.h
--- linux-2.5.31-vanilla/include/linux/pci.h Sat Aug 10 18:41:28 2002
+++ linux-2.5.31-patched/include/linux/pci.h Tue Aug 13 14:49:36 2002
@@ -456,12 +456,8 @@
/* Low-level architecture-dependent routines */
struct pci_ops {
- int (*read_byte)(struct pci_dev *, int where, u8 *val);
- int (*read_word)(struct pci_dev *, int where, u16 *val);
- int (*read_dword)(struct pci_dev *, int where, u32 *val);
- int (*write_byte)(struct pci_dev *, int where, u8 val);
- int (*write_word)(struct pci_dev *, int where, u16 val);
- int (*write_dword)(struct pci_dev *, int where, u32 val);
+ int (*read)(struct pci_dev *, int where, int size, u32 *val);
+ int (*write)(struct pci_dev *, int where, int size, u32 val);
};
struct pbus_set_ranges_data
> From: Matthew Dobson [mailto:[email protected]]
> OK... Here's the latest version. Sorry about that last
> posting... Stupid line
> wrapping broke the patch! :( This patch also removes the
> pci_config_(read|write) function pointers. People shouldn't
> be using these (I
> don't think) and should be using the pci_ops structure linked
> through the
> pci_dev structure. These end up calling the same functions that the
> pci_config_(read|write) pointers refer to anyway. The only
> places I can see
> that these are being used in the kernel are in
> drivers/acpi/osl.c... Anyone
> care to comment on the use there or if it can be changed?
> I've cc'd the
> authors of the file...
Hi Matthew,
ACPI needs access to PCI config space, and it doesn't have a struct pci_dev
to pass to access functions. It doesn't look like your patch exposes an
interface that 1) doesn't require a pci_dev and 2) abstracts the PCI config
access method, does it?
Regards -- Andy
Hi,
Here's just the patch related to the problem I reported yesterday. It's
against 2.4.19, but I think it will apply to any of the latest
2.4.20-pre2 trees.
Let's hope it gets included into the next -pre release.
Regards,
--
Steffen Persvold | Scalable Linux Systems | Try out the world's best
mailto:[email protected] | http://www.scali.com | performing MPI implementation:
Tel: (+47) 2262 8950 | Olaf Helsets vei 6 | - ScaMPI 1.13.8 -
Fax: (+47) 2262 8951 | N0621 Oslo, NORWAY | >320MBytes/s and <4uS latency
Hi Andy,
> ACPI needs access to PCI config space, and it doesn't have a struct pci_dev
> to pass to access functions. It doesn't look like your patch exposes an
> interface that 1) doesn't require a pci_dev and 2) abstracts the PCI config
> access method, does it?
What interface would you like to see?
Have a nice fortnight
--
Martin `MJ' Mares <[email protected]> http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
No fortune, no luck, no sig!
On Wed, 14 Aug 2002, Grover, Andrew wrote:
> ACPI needs access to PCI config space, and it doesn't have a struct pci_dev
> to pass to access functions. It doesn't look like your patch exposes an
> interface that 1) doesn't require a pci_dev and 2) abstracts the PCI config
> access method, does it?
I think drivers/hotplug/pci_hotplug_util.c implements something like you
need, pci_read_config_byte_nodev().
Of course that's currently only available for PCI hotplug, and for all I
can see the concept is somewhat messed up, but maybe that's an opportunity
to clean things up?
Currently, pci_read_config_byte_nodev() will construct a fake struct
pci_dev, and then use the normal pci_read_config_byte(). I think it
makes more sense to actually do things the other way around.
For reading/writing config space, we need to know (dev, fn), and need the
access method (struct pci_ops), which is a property of the bridge plus
possibly some private data (the arch's sysdata). So the member
struct pci_ops *ops;
of struct pci_dev is actually not necessary, it will always be
pdev->ops == pdev->bus->ops AFAICS.
So we could instead have
pci_bus_read_config_byte(struct pci_bus *bus, u8 dev, u8 fn, ...)
and for common use
static inline pci_read_config_byte(struct pci_dev, *pdev, ...)
{
return pci_bus_read_config_byte(pdev->bus,
PCI_SLOT(pdev->devfn),
PCI_FUNC(pdev->devfn));
}
The PCI hotplug controllers / ACPI could then use the pci_bus_* variants,
when they don't have a struct pci_dev available. They would need at least
the root bridge's struct pci_bus, though.
--Kai
On Thu, Aug 15, 2002 at 10:58:17AM -0500, Kai Germaschewski wrote:
> On Wed, 14 Aug 2002, Grover, Andrew wrote:
>
> > ACPI needs access to PCI config space, and it doesn't have a struct pci_dev
> > to pass to access functions. It doesn't look like your patch exposes an
> > interface that 1) doesn't require a pci_dev and 2) abstracts the PCI config
> > access method, does it?
>
> I think drivers/hotplug/pci_hotplug_util.c implements something like you
> need, pci_read_config_byte_nodev().
>
> Of course that's currently only available for PCI hotplug, and for all I
> can see the concept is somewhat messed up, but maybe that's an opportunity
> to clean things up?
I would love to clean those functions up.
> Currently, pci_read_config_byte_nodev() will construct a fake struct
> pci_dev, and then use the normal pci_read_config_byte(). I think it
> makes more sense to actually do things the other way around.
>
> For reading/writing config space, we need to know (dev, fn), and need the
> access method (struct pci_ops), which is a property of the bridge plus
> possibly some private data (the arch's sysdata). So the member
>
> struct pci_ops *ops;
>
> of struct pci_dev is actually not necessary, it will always be
> pdev->ops == pdev->bus->ops AFAICS.
>
> So we could instead have
>
> pci_bus_read_config_byte(struct pci_bus *bus, u8 dev, u8 fn, ...)
>
> and for common use
>
> static inline pci_read_config_byte(struct pci_dev, *pdev, ...)
> {
> return pci_bus_read_config_byte(pdev->bus,
> PCI_SLOT(pdev->devfn),
> PCI_FUNC(pdev->devfn));
> }
>
> The PCI hotplug controllers / ACPI could then use the pci_bus_* variants,
> when they don't have a struct pci_dev available. They would need at least
> the root bridge's struct pci_bus, though.
Thats a good idea. The hotplug controllers do have acess to the pci_bus
structure. Andy, does ACPI have access to this when you are needing to
do these kinds of calls?
If there are no complaints, I think I'll go implement this, and move the
functions into the main pci code so that other parts of the kernel (like
ACPI) can use them.
Thanks for the idea!
greg k-h
> ACPI needs access to PCI config space, and it doesn't have a struct pci_dev
> to pass to access functions. It doesn't look like your patch exposes an
> interface that 1) doesn't require a pci_dev and 2) abstracts the PCI config
> access method, does it?
I think your dependencies are backwards. IIRC, and based on a recent
conversation, ACPI needs to access PCI config space when ACPI finds a
_INI method for a device in the ACPI namespace. That assumes that it can
access the root bus that the device is on.
You don't have a PCI device because you haven't implement lockstep
enumeration yet in ACPI. With lockstep enumeration, you would add devices
to the device tree and let the bus drivers initialize them. With a bit a
glue, you would have a pointer to the PCI device correlating to the ACPI
namespace object, and a pointer to the PCI bus on which each PCI
device/namespace object resides.
To spell it out a bit more explicitly, you would start to parse the ACPI
namespace and find a Host/PCI bridge. You would tell the PCI subsystem to
probe for a device at that address. It would come back successful, and you
would obtain a pointer to that bridge device (and bus object). For all the
subordinate devices to that bridge, you then have access to the config
space via a real struct pci_bus.
There is no need to modify the PCI interface to support a fake device. The
PCI driver needs to be able to add and probe for devices as dictated by
someone else, but it's not that difficult.
If you remember, I sent you a patch that did most of this about 5 months
ago. It's a bit out of date, and I guarantee that it doesn't apply
anymore. But the concept is the same: we should fix the drivers, not hack
them to support a broken interface.
-pat
> From: Patrick Mochel [mailto:[email protected]]
> > ACPI needs access to PCI config space, and it doesn't have
> a struct pci_dev
> > to pass to access functions. It doesn't look like your
> patch exposes an
> > interface that 1) doesn't require a pci_dev and 2)
> abstracts the PCI config
> > access method, does it?
>
> I think your dependencies are backwards. IIRC, and based on a recent
> conversation, ACPI needs to access PCI config space when ACPI finds a
> _INI method for a device in the ACPI namespace. That assumes
> that it can
> access the root bus that the device is on.
>
> You don't have a PCI device because you haven't implement lockstep
> enumeration yet in ACPI. With lockstep enumeration, you would
> add devices
> to the device tree and let the bus drivers initialize them.
> With a bit a
> glue, you would have a pointer to the PCI device correlating
> to the ACPI
> namespace object, and a pointer to the PCI bus on which each PCI
> device/namespace object resides.
>
> To spell it out a bit more explicitly, you would start to
> parse the ACPI
> namespace and find a Host/PCI bridge. You would tell the PCI
> subsystem to
> probe for a device at that address. It would come back
> successful, and you
> would obtain a pointer to that bridge device (and bus
> object). For all the
> subordinate devices to that bridge, you then have access to the config
> space via a real struct pci_bus.
Yes, except that to find the host/pci bridge for bus 0, for example, I need
to run _INI on the device before I run _HID (which is the method that
returns the PNPID). _INI can theoretically access a bus 0 pci config
operation region.
People have mentioned to me that this is unpleasant and I agree, but the
ACPI spec *specifically* says that bus 0 pci config access is always
available.
That said, maybe it is better to keep ugliness caused by ACPI in the ACPI
driver, so if you want to have interfaces that depend on struct pci_dev or
pci_bus, fine, and the ACPI driver can generate a temporary one in order to
call the function. This completely violates the abstraction you are creating
but sssh we won't tell anyone. ;)
BTW this is not just a matter of spec compliance. Some machines actually
didn't work until this was implemented originally.
> If you remember, I sent you a patch that did most of this
> about 5 months
> ago. It's a bit out of date, and I guarantee that it doesn't apply
> anymore. But the concept is the same: we should fix the
> drivers, not hack
> them to support a broken interface.
Is this the one that was on bkbits.net for a while? I liked a lot of what
you did with that, but I was so busy with other stuff I didn't get a chance
to pull it in before it got too stale... :(
Regards -- Andy
[ cc list trimmed ]
> > To spell it out a bit more explicitly, you would start to
> > parse the ACPI
> > namespace and find a Host/PCI bridge. You would tell the PCI
> > subsystem to
> > probe for a device at that address. It would come back
> > successful, and you
> > would obtain a pointer to that bridge device (and bus
> > object). For all the
> > subordinate devices to that bridge, you then have access to the config
> > space via a real struct pci_bus.
>
> Yes, except that to find the host/pci bridge for bus 0, for example, I need
> to run _INI on the device before I run _HID (which is the method that
> returns the PNPID). _INI can theoretically access a bus 0 pci config
> operation region.
Side question: if you can't run _HID, how do you know it's a PCI bridge?
Or, if you can't run _HID, is there any way to determine that it's a
Host/PCI bridge?
> People have mentioned to me that this is unpleasant and I agree, but the
> ACPI spec *specifically* says that bus 0 pci config access is always
> available.
I thought it was just the root bus that a device was on: either bus 0, or
the bus specified by _BBN. For most systems it will be bus 0.
> That said, maybe it is better to keep ugliness caused by ACPI in the ACPI
> driver, so if you want to have interfaces that depend on struct pci_dev or
> pci_bus, fine, and the ACPI driver can generate a temporary one in order to
> call the function. This completely violates the abstraction you are creating
> but sssh we won't tell anyone. ;)
Actually, there shouldn't be much magic involved, if any at all.
The idea behind the patch I sent you before went something like this:
The ACPI namespace parser starts scans the namespace, before any PCI
config access has been determined. This won't get you very many objects,
because you won't be able to scan behind the bridges.
You take that list and start adding devices to the device tree. This uses
some new interfaces that allow you to tell the bus drivers that a bridge
or device exists at a specific address.
When you add a bridge, the bus driver will scan behind it. For each device
it finds, it calls platform_notify(), which you will implement. This can
then scan the namespace behind that object, and find more child objects,
and add them into the device tree.
There is some implicit recursion in that in the way I described it. It
doesn't have to, though I don't remember exactly what I did.
So, for bus 0, you don't have to wait and find it. You can hardcode it in
there, and tell the system to look for it when you first start up. This is
basically what the x86 arch code does: it assumes there is a bus 0 and
starts scanning behind it.
You'd do the same thing, which sounds like a step backward. But, with the
lockstep process, you would get to do everything you need to do. Right?
> > If you remember, I sent you a patch that did most of this
> > about 5 months
> > ago. It's a bit out of date, and I guarantee that it doesn't apply
> > anymore. But the concept is the same: we should fix the
> > drivers, not hack
> > them to support a broken interface.
>
> Is this the one that was on bkbits.net for a while? I liked a lot of what
> you did with that, but I was so busy with other stuff I didn't get a chance
> to pull it in before it got too stale... :(
Yes, that's the one. It's on my short list of things to update, which is
long enough as it is. :/
-pat
On Thu, Aug 15, 2002 at 09:36:45AM -0700, Greg KH wrote:
>
> If there are no complaints, I think I'll go implement this, and move the
> functions into the main pci code so that other parts of the kernel (like
> ACPI) can use them.
Ok, here's the patch that goes on top of Matt's previous patch, moving
the pci_ops functions to be pci_bus based instead of pci_dev, like they
are today. This enables us to get rid of all of the pci_*_nodev
functions in the pci hotplug core (that's 245 lines removed :) It's
currently running on the machine I'm typing this from.
I used the devfn as a paramater instead of the function and slot, as
some archs just take the devfn and move it around before using it. If
there were two paramaters, we would be splitting the values apart, and
then putting them back together for every function.
This is only for i386, if there's no complaints I'll knock out the other
archs before submitting it.
thanks,
greg k-h
diff -Nru a/arch/i386/pci/direct.c b/arch/i386/pci/direct.c
--- a/arch/i386/pci/direct.c Fri Aug 16 15:31:06 2002
+++ b/arch/i386/pci/direct.c Fri Aug 16 15:31:06 2002
@@ -71,16 +71,16 @@
#undef PCI_CONF1_ADDRESS
-static int pci_conf1_read(struct pci_dev *dev, int where, int size, u32 *value)
+static int pci_conf1_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *value)
{
- return __pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, size, value);
+ return __pci_conf1_read(0, bus->number, PCI_SLOT(devfn),
+ PCI_FUNC(devfn), where, size, value);
}
-static int pci_conf1_write(struct pci_dev *dev, int where, int size, u32 value)
+static int pci_conf1_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 value)
{
- return __pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, size, value);
+ return __pci_conf1_write(0, bus->number, PCI_SLOT(devfn),
+ PCI_FUNC(devfn), where, size, value);
}
static struct pci_ops pci_direct_conf1 = {
@@ -165,16 +165,16 @@
#undef PCI_CONF2_ADDRESS
-static int pci_conf2_read(struct pci_dev *dev, int where, int size, u32 *value)
+static int pci_conf2_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *value)
{
- return __pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, size, value);
+ return __pci_conf2_read(0, bus->number, PCI_SLOT(devfn),
+ PCI_FUNC(devfn), where, size, value);
}
-static int pci_conf2_write(struct pci_dev *dev, int where, int size, u32 value)
+static int pci_conf2_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 value)
{
- return __pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, size, value);
+ return __pci_conf2_write(0, bus->number, PCI_SLOT(devfn),
+ PCI_FUNC(devfn), where, size, value);
}
static struct pci_ops pci_direct_conf2 = {
@@ -204,9 +204,9 @@
bus.number = 0;
dev.bus = &bus;
for(dev.devfn=0; dev.devfn < 0x100; dev.devfn++)
- if ((!o->read(&dev, PCI_CLASS_DEVICE, 2, &x) &&
+ if ((!o->read(&bus, dev.devfn, PCI_CLASS_DEVICE, 2, &x) &&
(x == PCI_CLASS_BRIDGE_HOST || x == PCI_CLASS_DISPLAY_VGA)) ||
- (!o->read(&dev, PCI_VENDOR_ID, 2, &x) &&
+ (!o->read(&bus, dev.devfn, PCI_VENDOR_ID, 2, &x) &&
(x == PCI_VENDOR_ID_INTEL || x == PCI_VENDOR_ID_COMPAQ)))
return 1;
DBG("PCI: Sanity check failed\n");
diff -Nru a/arch/i386/pci/pcbios.c b/arch/i386/pci/pcbios.c
--- a/arch/i386/pci/pcbios.c Fri Aug 16 15:31:06 2002
+++ b/arch/i386/pci/pcbios.c Fri Aug 16 15:31:06 2002
@@ -295,16 +295,16 @@
return (int)((result & 0xff00) >> 8);
}
-static int pci_bios_read(struct pci_dev *dev, int where, int size, u32 *value)
+static int pci_bios_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *value)
{
- return __pci_bios_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, size, value);
+ return __pci_bios_read(0, bus->number, PCI_SLOT(devfn),
+ PCI_FUNC(devfn), where, size, value);
}
-static int pci_bios_write(struct pci_dev *dev, int where, int size, u32 value)
+static int pci_bios_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 value)
{
- return __pci_bios_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, size, value);
+ return __pci_bios_write(0, bus->number, PCI_SLOT(devfn),
+ PCI_FUNC(devfn), where, size, value);
}
diff -Nru a/drivers/pci/access.c b/drivers/pci/access.c
--- a/drivers/pci/access.c Fri Aug 16 15:31:06 2002
+++ b/drivers/pci/access.c Fri Aug 16 15:31:06 2002
@@ -20,27 +20,29 @@
#define PCI_dword_BAD (pos & 3)
#define PCI_OP_READ(size,type,len) \
-int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \
+int pci_bus_read_config_##size \
+ (struct pci_bus *bus, unsigned int devfn, int pos, type *value) \
{ \
int res; \
unsigned long flags; \
u32 data = 0; \
if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
spin_lock_irqsave(&pci_lock, flags); \
- res = dev->bus->ops->read(dev, pos, len, &data); \
+ res = bus->ops->read(bus, devfn, pos, len, &data); \
*value = (type)data; \
spin_unlock_irqrestore(&pci_lock, flags); \
return res; \
}
#define PCI_OP_WRITE(size,type,len) \
-int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \
+int pci_bus_write_config_##size \
+ (struct pci_bus *bus, unsigned int devfn, int pos, type value) \
{ \
int res; \
unsigned long flags; \
if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
spin_lock_irqsave(&pci_lock, flags); \
- res = dev->bus->ops->write(dev, pos, len, value); \
+ res = bus->ops->write(bus, devfn, pos, len, value); \
spin_unlock_irqrestore(&pci_lock, flags); \
return res; \
}
@@ -52,10 +54,10 @@
PCI_OP_WRITE(word, u16, 2)
PCI_OP_WRITE(dword, u32, 4)
-EXPORT_SYMBOL(pci_read_config_byte);
-EXPORT_SYMBOL(pci_read_config_word);
-EXPORT_SYMBOL(pci_read_config_dword);
-EXPORT_SYMBOL(pci_write_config_byte);
-EXPORT_SYMBOL(pci_write_config_word);
-EXPORT_SYMBOL(pci_write_config_dword);
+EXPORT_SYMBOL(pci_bus_read_config_byte);
+EXPORT_SYMBOL(pci_bus_read_config_word);
+EXPORT_SYMBOL(pci_bus_read_config_dword);
+EXPORT_SYMBOL(pci_bus_write_config_byte);
+EXPORT_SYMBOL(pci_bus_write_config_word);
+EXPORT_SYMBOL(pci_bus_write_config_dword);
EXPORT_SYMBOL(pci_lock);
diff -Nru a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h Fri Aug 16 15:31:06 2002
+++ b/include/linux/pci.h Fri Aug 16 15:31:06 2002
@@ -457,8 +457,8 @@
/* Low-level architecture-dependent routines */
struct pci_ops {
- int (*read)(struct pci_dev *, int where, int size, u32 *val);
- int (*write)(struct pci_dev *, int where, int size, u32 val);
+ int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
+ int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
};
struct pbus_set_ranges_data
@@ -557,12 +557,37 @@
struct pci_dev *pci_find_slot (unsigned int bus, unsigned int devfn);
int pci_find_capability (struct pci_dev *dev, int cap);
-int pci_read_config_byte(struct pci_dev *dev, int where, u8 *val);
-int pci_read_config_word(struct pci_dev *dev, int where, u16 *val);
-int pci_read_config_dword(struct pci_dev *dev, int where, u32 *val);
-int pci_write_config_byte(struct pci_dev *dev, int where, u8 val);
-int pci_write_config_word(struct pci_dev *dev, int where, u16 val);
-int pci_write_config_dword(struct pci_dev *dev, int where, u32 val);
+int pci_bus_read_config_byte (struct pci_bus *bus, unsigned int devfn, int where, u8 *val);
+int pci_bus_read_config_word (struct pci_bus *bus, unsigned int devfn, int where, u16 *val);
+int pci_bus_read_config_dword (struct pci_bus *bus, unsigned int devfn, int where, u32 *val);
+int pci_bus_write_config_byte (struct pci_bus *bus, unsigned int devfn, int where, u8 val);
+int pci_bus_write_config_word (struct pci_bus *bus, unsigned int devfn, int where, u16 val);
+int pci_bus_write_config_dword (struct pci_bus *bus, unsigned int devfn, int where, u32 val);
+
+static inline int pci_read_config_byte(struct pci_dev *dev, int where, u8 *val)
+{
+ return pci_bus_read_config_byte (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_read_config_word(struct pci_dev *dev, int where, u16 *val)
+{
+ return pci_bus_read_config_word (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_read_config_dword(struct pci_dev *dev, int where, u32 *val)
+{
+ return pci_bus_read_config_dword (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_write_config_byte(struct pci_dev *dev, int where, u8 val)
+{
+ return pci_bus_write_config_byte (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_write_config_word(struct pci_dev *dev, int where, u16 val)
+{
+ return pci_bus_write_config_word (dev->bus, dev->devfn, where, val);
+}
+static int inline pci_write_config_dword(struct pci_dev *dev, int where, u32 val)
+{
+ return pci_bus_write_config_dword (dev->bus, dev->devfn, where, val);
+}
extern spinlock_t pci_lock;
diff -Nur linux-2.5.31-patched/arch/i386/pci/numa.c linux-2.5.31-numaq/arch/i386/pci/numa.c
--- linux-2.5.31-patched/arch/i386/pci/numa.c Wed Aug 14 11:07:13 2002
+++ linux-2.5.31-numaq/arch/i386/pci/numa.c Wed Aug 14 11:11:58 2002
@@ -1,19 +1,19 @@
/*
* numa.c - Low-level PCI access for NUMA-Q machines
*/
+
#include <linux/pci.h>
#include <linux/init.h>
-
#include "pci.h"
#define BUS2QUAD(global) (mp_bus_id_to_node[global])
#define BUS2LOCAL(global) (mp_bus_id_to_local[global])
#define QUADLOCAL2BUS(quad,local) (quad_local_to_mp_bus_id[quad][local])
-#define PCI_CONF1_ADDRESS(bus, dev, fn, reg) \
+#define PCI_CONF1_MQ_ADDRESS(bus, dev, fn, reg) \
(0x80000000 | (BUS2LOCAL(bus) << 16) | (dev << 11) | (fn << 8) | (reg & ~3))
-static int pci_conf1_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+static int __pci_conf1_mq_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
{
unsigned long flags;
@@ -22,7 +22,7 @@
spin_lock_irqsave(&pci_config_lock, flags);
- outl_quad(PCI_CONF1_ADDRESS(bus, dev, fn, reg), 0xCF8, BUS2QUAD(bus));
+ outl_quad(PCI_CONF1_MQ_ADDRESS(bus, dev, fn, reg), 0xCF8, BUS2QUAD(bus));
switch (len) {
case 1:
@@ -41,7 +41,7 @@
return 0;
}
-static int pci_conf1_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+static int __pci_conf1_mq_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
{
unsigned long flags;
@@ -50,7 +50,7 @@
spin_lock_irqsave(&pci_config_lock, flags);
- outl_quad(PCI_CONF1_ADDRESS(bus, dev, fn, reg), 0xCF8, BUS2QUAD(bus));
+ outl_quad(PCI_CONF1_MQ_ADDRESS(bus, dev, fn, reg), 0xCF8, BUS2QUAD(bus));
switch (len) {
case 1:
@@ -69,6 +69,25 @@
return 0;
}
+#undef PCI_CONF1_MQ_ADDRESS
+
+static int pci_conf1_mq_read(struct pci_dev *dev, int where, int size, u32 *value)
+{
+ return __pci_conf1_mq_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, size, value);
+}
+
+static int pci_conf1_mq_write(struct pci_dev *dev, int where, int size, u32 value)
+{
+ return __pci_conf1_mq_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, size, value);
+}
+
+static struct pci_ops pci_direct_conf1_mq = {
+ read: pci_conf1_mq_read,
+ write: pci_conf1_mq_write
+};
+
static void __devinit pci_fixup_i450nx(struct pci_dev *d)
{
@@ -102,8 +121,7 @@
{
int quad;
- pci_config_read = pci_conf1_read;
- pci_config_write = pci_conf1_write;
+ pci_root_ops = &pci_direct_conf1_mq;
if (pcibios_scanned++)
return 0;