Re: PCI Express support for 2.4 kernel

Vladimir Kondratiev
2003-12-15T10:02:33Z

[-- Attachment #1: Type: text/plain, Size: 4863 bytes --]

Hi all,

Thanks for comments. Jeff, I specially appreciate time you spent walking
through code.

I am not subscribed to lkml, thus please CC me
(Vladimir Kondratiev <vladimir.kondratiev@intel.com>) in replies.

To keep posting short, I skipped most of original mail.
I attached fixed version for the patch. Sure, I tested it prior to posting.

> <Jeff>
> The patch could use some cleaning up, for coding style. The general
> style is documented in CodingStyle file in the kernel source tree.
> But also, you should consider the "golden rule of style" for
> programmers: When modifying a file, follow the same formatting as is
> found in the rest of the file.
>
Agree. It was my fault to not do it from the beginning. I habited to
trust Emacs. Fixed.

>> +/**
>> + * Virtual address for RRBAR
>> + */
>> +static void* rrbar_virt=NULL;
>
>
> Do not bother initializing static variables to zero. This just wastes
> bss space, since these variables are automatically zeroed for you,
> anyway.

I did not found this feature in standard. More, future versions of gcc
will give at least warning, if not error, like "use of uninitialized
variable". Many good sources also say it is good practice to initialize
all variables. I rely on its value later. I' ll keep it as is unless
really strong arguments provided.

>> +/**
>> + * It used to be #define, but I am going to change it.
>> + */
>> +extern int PCI_CFG_SPACE_SIZE;
>
>
> Hopefully this is temporary... variables should not be ALL CAPS. If
> you change it to a variable, also change its case.

Agree. Fixed.

>> +/**
>> + * Initializes PCI Express method for config space access.
>> + * + * There is no standard method to recognize presence of PCI
>> Express,
>> + * thus we will assume it is PCI-E, and rely on sanity check to
>> + * deassert PCI-E presense. If PCI-E not present,
>> + * there is no physical RAM on RRBAR address, and we should read
>> + * something like 0xff.
>> + * + * Creates mapping for whole 256M area.
>> + * + * @return 1 if OK, 0 if error
>> + */
>> +static int pci_express_init(void)
>> +{
>> + /* TODO: check PCI-Ex presense */
>
>
> Um, this sounds critical. We should not merge this patch until this
> 'TODO' is complete, in my opinion.

As I explained in comment to function, it is not really critical. The
problem is, there is no generic way (or I don't know it) to recognize
PCI-E. One suggest to go over all devices and see whether PCI-E
capability present for at least one of them. I don't think it is good
way to do. Sanity check do pretty good job here. If it is not PCI-E
platform, this address in physical memory will not be connected to
anything real. You will get 0xff's.

>> +static int pci_exp_read (int seg, int bus, int dev, int fn, int reg,
>> int len, u32 *value)
>
>
> I think these arguments should be 'unsigned int', not 'int'.
>
> Let's be proactive, and protect against signed/unsigned problems now,
> before they appear.

Sound reasonable. But this is how this function prototyped:

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;

I think it is better to ask author (Martin) of this code, may be he had
some reason for this prototype?
I added him to CC list.

>> + void* addr=rrbar_virt+(bus << 20)+(dev << 15)+(fn << 12)+reg;
>
>
> Why do you prefer '+' to '|' here?
>
> '|' normally has less side effects.
>
Agree. Fixed, except 1-st "+", since virtual addres may be not aligned
on 256M boundary

> Further, PCI posting: a writeb() / writew() / writel() will not be
> flushed immediately to the processor. The CPU and/or PCI bridge may
> post (delay/combine) such writes. I do not think this is a desireable
> effect, for PCI config register accesses.
>
Good point. Fixed.

> As with other code, define a macro that creates these functions. Then
> add a series of lines such as
>
> DEF_PCIEX_READ(word, 2)
> DEF_PCIEX_READ(dword, 4)
> ...
>
2 'read' functions (one do not follow pattern) and 3 'write' functions
do not worth 2 macros. Also, coding style for
the rest of file is different. Other methods defined w/o macros.

<Pete>

>+ if (rrbar_virt) { <====== Not trusting own code
>> + iounmap(rrbar_virt);
>> + }
>
>
>
>All in all, raw. Also, a healthy dose of Itanium is prescribed.
>
>
>
Extra check is here for the case code will be modified. In not
frequently executed code, it is better to check twice rather then forget
to check.

<Christoph>

>It should go into 2.6 first. though. And while you're at it add your
>copyright info to the top of the file instead of the middle.
>
Copyright - done, thanks for input.
Regarding 2.6 - you are right, but now I need it for 2.4, and I have no
2.6 handy. I will do it in a week or 2.

Vladimir.

[-- Attachment #2: pciexp.patch --]
[-- Type: text/plain, Size: 9159 bytes --]

Enable PCI Express access method for configuration space
This path includes:
* access routines itself
* command line argument "pci=exp" to force PCI Express, similar to "conf1" and "conf2"
* full 4k config accessed through /proc/bus/pci/...

How it works:

With PCI-E, config space accessed through memory. Each device gets its own 4k memory mapped config,
total 256M for all devices.

At init time, I map whole region to not spent time for mapping later.

For /proc/bus/pci/..., I changed PCI_CFG_SPACE_SIZE to variable and changed it to 4k for PCI-E.

It is tested on 1 platform.

Author: "Vladimir Kondratiev" <vladimir.kondratiev@intel.com>

diff -dur linux-2.4.23/arch/i386/kernel/pci-i386.h linux-2.4.23-pciexp/arch/i386/kernel/pci-i386.h
--- linux-2.4.23/arch/i386/kernel/pci-i386.h 2003-11-28 20:26:19.000000000 +0200
+++ linux-2.4.23-pciexp/arch/i386/kernel/pci-i386.h 2003-12-14 11:08:17.000000000 +0200
@@ -15,6 +15,7 @@
#define PCI_PROBE_BIOS 0x0001
#define PCI_PROBE_CONF1 0x0002
#define PCI_PROBE_CONF2 0x0004
+#define PCI_PROBE_EXP 0x0008
#define PCI_NO_SORT 0x0100
#define PCI_BIOS_SORT 0x0200
#define PCI_NO_CHECKS 0x0400
diff -dur linux-2.4.23/arch/i386/kernel/pci-pc.c linux-2.4.23-pciexp/arch/i386/kernel/pci-pc.c
--- linux-2.4.23/arch/i386/kernel/pci-pc.c 2003-11-28 20:26:19.000000000 +0200
+++ linux-2.4.23-pciexp/arch/i386/kernel/pci-pc.c 2003-12-15 10:35:46.000000000 +0200
@@ -2,6 +2,9 @@
* Low-Level PCI Support for PC
*
* (c) 1999--2000 Martin Mares <mj@ucw.cz>
+ *
+ * (c) 2003 Vladimir Kondratiev <vladimir.kondratiev@intel.com>
+ * PCI Express
*/

#include <linux/config.h>
@@ -20,7 +23,7 @@

#include "pci-i386.h"

-unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | PCI_PROBE_CONF2;
+unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | PCI_PROBE_CONF2 | PCI_PROBE_EXP;

int pcibios_last_bus = -1;
struct pci_bus *pci_root_bus = NULL;
@@ -427,6 +430,159 @@
pci_conf2_write_config_dword
};

+/**
+ * PCI Express routines
+ * "Vladimir Kondratiev" <vladimir.kondratiev@intel.com>
+ */
+/**
+ * RRBAR (memory base for PCI-E config space) resides here.
+ * Initialized to default address. Actually, it is platform specific, and
+ * value may vary.
+ * I don't know how to detect it properly, it is chipset specific.
+ */
+static u32 rrbar_phys = 0xe0000000UL;
+/**
+ * RRBAR is always 256M
+ */
+static u32 rrbar_size = 0x10000000UL;
+/**
+ * Virtual address for RRBAR
+ */
+static void *rrbar_virt = NULL;
+/**
+ * It used to be #define, but I am going to modify it.
+ */
+extern int pci_cfg_space_size;
+
+/**
+ * Initializes PCI Express method for config space access.
+ *
+ * There is no standard method to recognize presence of PCI Express,
+ * thus we will assume it is PCI-E, and rely on sanity check to
+ * deassert PCI-E presense. If PCI-E not present,
+ * there is no physical RAM on RRBAR address, and we should read
+ * something like 0xff.
+ *
+ * Creates mapping for whole 256M area.
+ *
+ * @return 1 if OK, 0 if error
+ */
+static int pci_express_init(void)
+{
+ /* TODO: check PCI-Ex presence */
+ rrbar_virt = ioremap(rrbar_phys, rrbar_size);
+ if (!rrbar_virt)
+ return 0;
+ return 1;
+}
+
+/**
+ * Shuts down PCI-E resources.
+ */
+static void pci_express_fini(void)
+{
+ if (rrbar_virt)
+ iounmap(rrbar_virt);
+}
+
+static int pci_exp_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+{
+ void *addr = rrbar_virt + ((bus << 20) | (dev << 15) | (fn << 12) | reg);
+ if ((bus > 255 || dev > 31 || fn > 7 || reg > 4095))
+ return -EINVAL;
+ switch (len) {
+ case 1:
+ *value = readb(addr);
+ break;
+ case 2:
+ if (reg & 1)
+ return -EINVAL;
+ *value = readw(addr);
+ break;
+ case 4:
+ if (reg & 3)
+ return -EINVAL;
+ *value = readl(addr);
+ break;
+ }
+ return 0;
+}
+
+static int pci_exp_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+{
+ void *addr = rrbar_virt + ((bus << 20) | (dev << 15) | (fn << 12) | reg);
+ if ((bus > 255 || dev > 31 || fn > 7 || reg > 4095))
+ return -EINVAL;
+ switch (len) {
+ case 1:
+ writeb(value, addr);
+ break;
+ case 2:
+ if (reg & 1) return -EINVAL;
+ writew(value, addr);
+ break;
+ case 4:
+ if (reg & 3) return -EINVAL;
+ writel(value, addr);
+ break;
+ }
+ /* dummy read to flush PCI write */
+ readb(addr);
+ return 0;
+}
+
+static int pci_exp_read_config_byte(struct pci_dev *dev, int where, u8 *value)
+{
+ int result;
+ u32 data;
+ result = pci_exp_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, 1, &data);
+ *value = (u8)data;
+ return result;
+}
+
+static int pci_exp_read_config_word(struct pci_dev *dev, int where, u16 *value)
+{
+ int result;
+ u32 data;
+ result = pci_exp_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, 2, &data);
+ *value = (u16)data;
+ return result;
+}
+
+static int pci_exp_read_config_dword(struct pci_dev *dev, int where, u32 *value)
+{
+ return pci_exp_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, 4, value);
+}
+
+static int pci_exp_write_config_byte(struct pci_dev *dev, int where, u8 value)
+{
+ return pci_exp_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, 1, value);
+}
+
+static int pci_exp_write_config_word(struct pci_dev *dev, int where, u16 value)
+{
+ return pci_exp_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, 2, value);
+}
+
+static int pci_exp_write_config_dword(struct pci_dev *dev, int where, u32 value)
+{
+ return pci_exp_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, 4, value);
+}
+
+static struct pci_ops pci_express_conf = {
+ pci_exp_read_config_byte,
+ pci_exp_read_config_word,
+ pci_exp_read_config_dword,
+ pci_exp_write_config_byte,
+ pci_exp_write_config_word,
+ pci_exp_write_config_dword
+};

/*
* Before we decide to use direct hardware access mechanisms, we try to do some
@@ -465,6 +621,22 @@

__save_flags(flags); __cli();

+ /**
+ * Check if PCI-express access work
+ */
+ if (pci_express_init()) {
+ if (pci_sanity_check(&pci_express_conf)) {
+ /* PCI-E provides 4k config space */
+ pci_cfg_space_size = 4096;
+ __restore_flags(flags);
+ printk(KERN_INFO "PCI: Using configuration type PCI Express\n");
+ request_mem_region(rrbar_phys, rrbar_size, "PCI-Express config space");
+ return &pci_express_conf;
+ } else {
+ pci_express_fini();
+ }
+ }
+
/*
* Check if configuration type 1 works.
*/
@@ -1398,16 +1570,18 @@
#endif

#ifdef CONFIG_PCI_DIRECT
- if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2))
+ if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2 | PCI_PROBE_EXP))
&& (tmp = pci_check_direct())) {
pci_root_ops = tmp;
if (pci_root_ops == &pci_direct_conf1) {
pci_config_read = pci_conf1_read;
pci_config_write = pci_conf1_write;
- }
- else {
+ } else if (pci_root_ops == &pci_direct_conf2) {
pci_config_read = pci_conf2_read;
pci_config_write = pci_conf2_write;
+ } else if (pci_root_ops == &pci_express_conf) {
+ pci_config_read = pci_exp_read;
+ pci_config_write = pci_exp_write;
}
}
#endif
@@ -1489,6 +1663,10 @@
pci_probe = PCI_PROBE_CONF2 | PCI_NO_CHECKS;
return NULL;
}
+ else if (!strcmp(str, "exp")) {
+ pci_probe = PCI_PROBE_EXP | PCI_NO_CHECKS;
+ return NULL;
+ }
#endif
else if (!strcmp(str, "rom")) {
pci_probe |= PCI_ASSIGN_ROMS;
diff -dur linux-2.4.23/drivers/pci/proc.c linux-2.4.23-pciexp/drivers/pci/proc.c
--- linux-2.4.23/drivers/pci/proc.c 2002-11-29 01:53:14.000000000 +0200
+++ linux-2.4.23-pciexp/drivers/pci/proc.c 2003-12-15 11:48:16.000000000 +0200
@@ -16,7 +16,10 @@
#include <asm/uaccess.h>
#include <asm/byteorder.h>

-#define PCI_CFG_SPACE_SIZE 256
+/**
+ * For PCI Express, it will be set to 4096 during PCI init
+ */
+int pci_cfg_space_size=256;

static loff_t
proc_bus_pci_lseek(struct file *file, loff_t off, int whence)
@@ -31,12 +34,12 @@
new = file->f_pos + off;
break;
case 2:
- new = PCI_CFG_SPACE_SIZE + off;
+ new = pci_cfg_space_size + off;
break;
default:
return -EINVAL;
}
- if (new < 0 || new > PCI_CFG_SPACE_SIZE)
+ if (new < 0 || new > pci_cfg_space_size)
return -EINVAL;
return (file->f_pos = new);
}
@@ -57,7 +60,7 @@
*/

if (capable(CAP_SYS_ADMIN))
- size = PCI_CFG_SPACE_SIZE;
+ size = pci_cfg_space_size;
else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)
size = 128;
else
@@ -132,12 +135,12 @@
int pos = *ppos;
int cnt;

- if (pos >= PCI_CFG_SPACE_SIZE)
+ if (pos >= pci_cfg_space_size)
return 0;
- if (nbytes >= PCI_CFG_SPACE_SIZE)
- nbytes = PCI_CFG_SPACE_SIZE;
- if (pos + nbytes > PCI_CFG_SPACE_SIZE)
- nbytes = PCI_CFG_SPACE_SIZE - pos;
+ if (nbytes >= pci_cfg_space_size)
+ nbytes = pci_cfg_space_size;
+ if (pos + nbytes > pci_cfg_space_size)
+ nbytes = pci_cfg_space_size - pos;
cnt = nbytes;

if (!access_ok(VERIFY_READ, buf, cnt))
@@ -389,7 +392,7 @@
return -ENOMEM;
e->proc_fops = &proc_bus_pci_operations;
e->data = dev;
- e->size = PCI_CFG_SPACE_SIZE;
+ e->size = pci_cfg_space_size;
return 0;
}