PCI Express support for 2.4 kernel

PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

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

Please, ignore previous submission with the same subject. Patch file 
attached was wrong one. Now correct patch attached.

Hi,
PCI-Express platforms will soon appear on the market. It is worth to
support it.

Following is patch for 2.4.23 kernel. I tested it on my host, it works
properly.
I did it for i386 only, I have no other architecture to test.

It was patch on the same subject from* Seshadri, Harinarayanan*
(/harinarayanan.seshadri@intel.com/
<mailto:harinarayanan.seshadri@intel.com>)
http://www.cs.helsinki.fi/linux/linux-kernel/2003-17/0247.html
My version differ in several aspects: it is for 2.4 (vs. 2.6); it do not
ioremap/unmap page for each transaction.

How about inclusion in 2.4.24?

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

Vladimir.


[-- Attachment #2: pciexp.patch --]
[-- Type: text/plain, Size: 7703 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 -bBdur 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 -bBdur 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-14 21:27:52.000000000 +0200
@@ -20,7 +20,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 +427,155 @@
 	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 change 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 presense */
+  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;
+  }
+  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 +614,21 @@
 
 	__save_flags(flags); __cli();
 
+    /**
+     * Check if PCI-express access work
+     */
+    if (pci_express_init()) {
+        if (pci_sanity_check(&pci_express_conf)) {
+            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 +1562,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 +1655,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 -bBdur 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-14 14:18:58.000000000 +0200
@@ -16,7 +16,7 @@
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
 
-#define PCI_CFG_SPACE_SIZE 256
+int PCI_CFG_SPACE_SIZE=256;
 
 static loff_t
 proc_bus_pci_lseek(struct file *file, loff_t off, int whence)

Re: PCI Express support for 2.4 kernel

[Jeff Garzik]

Vladimir Kondratiev wrote:
> Please, ignore previous submission with the same subject. Patch file 
> attached was wrong one. Now correct patch attached.
> 
> Hi,
> PCI-Express platforms will soon appear on the market. It is worth to
> support it.
> 
> Following is patch for 2.4.23 kernel. I tested it on my host, it works
> properly.
> I did it for i386 only, I have no other architecture to test.
> 
> It was patch on the same subject from* Seshadri, Harinarayanan*
> (/harinarayanan.seshadri@intel.com/
> <mailto:harinarayanan.seshadri@intel.com>)
> http://www.cs.helsinki.fi/linux/linux-kernel/2003-17/0247.html
> My version differ in several aspects: it is for 2.4 (vs. 2.6); it do not
> ioremap/unmap page for each transaction.
> 
> How about inclusion in 2.4.24?
> 
> I am not subscribed to lkml, thus please CC me
> (Vladimir Kondratiev <vladimir.kondratiev@intel.com>) in replies.


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.

Particularly:
* use a single tab to indent at each level
* use whitespace liberally.  avoid "foo=bar".  prefer "foo = bar".



> +/**
> + * 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;

Insert spaces in the assignment statements.


> +/**
> + * 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.


> +/**
> + * 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.


> +/**
> + * 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.


> +  rrbar_virt=ioremap(rrbar_phys,rrbar_size);

add whitespace

> +  if (!rrbar_virt) return 0;

add a line break between two statements

> +  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)

I think these arguments should be 'unsigned int', not 'int'.

Let's be proactive, and protect against signed/unsigned problems now, 
before they appear.


> +{
> +  void* addr=rrbar_virt+(bus << 20)+(dev << 15)+(fn << 12)+reg;

Why do you prefer '+' to '|' here?

'|' normally has less side effects.


> +  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;
> +  }
> +  return 0;
> +}

ditto, comments above:  formatting, whitespace, signed/unsigned, logical or.

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.


> +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);
> +}

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)
...


> +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 +614,21 @@
>  
>  	__save_flags(flags); __cli();
>  
> +    /**
> +     * Check if PCI-express access work
> +     */
> +    if (pci_express_init()) {
> +        if (pci_sanity_check(&pci_express_conf)) {
> +            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 +1562,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 +1655,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 -bBdur 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-14 14:18:58.000000000 +0200
> @@ -16,7 +16,7 @@
>  #include <asm/uaccess.h>
>  #include <asm/byteorder.h>
>  
> -#define PCI_CFG_SPACE_SIZE 256
> +int PCI_CFG_SPACE_SIZE=256;

ditto comment at the top...  when changing to a variable, make it lowercase.

	Jeff

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

[-- 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;
 }
 

Re: PCI Express support for 2.4 kernel

[Gabriel Paubert]

	Hi, 
	
On Mon, Dec 15, 2003 at 12:01:53PM +0200, Vladimir Kondratiev wrote:
> >>+/**
> >>+ * 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.

Or a machine check on some architectures which make you know that 
they don't like a bus cycle which terminates on a master abort.

> Agree. Fixed, except 1-st "+", since virtual addres may be not aligned 
> on 256M boundary

The major problem I see is that using up 256 Mb of kernel virtual address 
space for accessing PCI config space is gross. Besides that it won't
work for 32 bit machines with 768 Mb of RAM or more.

I believe that it would be better to use kmap/kunmap like tricks, especially 
for something which is relatively infrequent. You could even reserve
a fixmap entry for this and keep somewhere a pointer to the PTE, which 
would be modified on every config space access (config space access was
already properly serialized last time I looked, I believe that all you need 
is a TLB flush after the PTE is updated).

I have no strong opinion on how to handle 64 bit archs.

> 
> >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.

Here I'm somehwat lost. Writes to uncacheable RAM will be in program 
order and never combined. The bridge itself should not post writes to 
config space. So it's a matter of pushing the write to the processor
bus, a PCI read looks very heavy for this. Isn't there a more
lightweight solution ?

	Regards,
	Gabriel

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Gabriel Paubert wrote:

>>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.
>>    
>>
>
>Or a machine check on some architectures which make you know that 
>they don't like a bus cycle which terminates on a master abort.
>  
>
Could you elaborate a bit? This code is for pci-pc.c, it is platform 
specific.
Are there surprises on pc platform? What is visible behavour in this case?

>The major problem I see is that using up 256 Mb of kernel virtual address 
>space for accessing PCI config space is gross. Besides that it won't
>work for 32 bit machines with 768 Mb of RAM or more.
>
>I believe that it would be better to use kmap/kunmap like tricks, especially 
>for something which is relatively infrequent. You could even reserve
>a fixmap entry for this and keep somewhere a pointer to the PTE, which 
>would be modified on every config space access (config space access was
>already properly serialized last time I looked, I believe that all you need 
>is a TLB flush after the PTE is updated).
>
>I have no strong opinion on how to handle 64 bit archs.
>  
>
I should be missing something here. You have 256M of physical address 
space at 0xe0000000 occupied.
You can do nothing with it, it is simply present. Then, ioremap maps it 
somewhere in high memory.
It should not conflict with kernel RAM, for which trivial mapping (+3G) 
used.

>  
>
>>>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.
>>    
>>
>
>Here I'm somehwat lost. Writes to uncacheable RAM will be in program 
>order and never combined. The bridge itself should not post writes to 
>config space. So it's a matter of pushing the write to the processor
>bus, a PCI read looks very heavy for this. Isn't there a more
>lightweight solution ?
>  
>
I am not expert here. I know that PCI-E bridge completes its transaction 
before data actually get to memory. Each intermediate bridge may do its 
own buffering. Besides read, I know nothing that will make you sure data 
reaches final destination. I'd appreciate if someone who knows better 
will step in. Config write is not very often procedure, I don't think 
extra read will make  any difference. I'll look for qualified person 
around, to consult on this issue.

Vladimir.

Re: PCI Express support for 2.4 kernel

[Arjan van de Ven]

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


> I should be missing something here. You have 256M of physical address 
> space at 0xe0000000 occupied.
> You can do nothing with it, it is simply present. Then, ioremap maps it 
> somewhere in high memory.
> It should not conflict with kernel RAM, for which trivial mapping (+3G) 
> used.

the thing is that typically you have a maximum of 168Mb or so of
ioremap/vmalloc space (they share the same pool). That is, ff your
system has >= 1Gb of ram, if it has less ran the ioremap/vmalloc space
is bigger....


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Got it.
Should I understand it this way: for system with >=1Gb RAM, I will be 
unable to ioremap 256Mb region?
It looks confusing. On my test system (don't ask details, I am not 
alowed to share this info), I see
video controller with 256Mb BAR. Does it mean this controller will not 
work as well?

There is alternative solution, for each transaction to ioremap/unmap 
corresponded page.
I don't like it, it involves huge overhead.

I thought about remapping only pages that have actual PCI devices behind,
but this is problematic: access to config goes not always through 
pci_exp_read_config_xxx and alike, raw access with bus/dev/fn numbers 
used as well. And in 2.6, correct me if I wrong, raw access using 
bus/dev/fn numbers goes to be the only way. Per-device access replaced 
with per-bus, at least.

I can statically remap only region for existing buses, this will be huge 
save. It is 1MB per bus, this lead to typical 2-3MB instead of 256. To 
be sure I can do this, I need to know that new bus can't be added on run 
time. I don't think it is true, isn't it? Or do we have single point to 
capture hot plug for new bus?

Vladimir.

Arjan van de Ven wrote:

>>I should be missing something here. You have 256M of physical address 
>>space at 0xe0000000 occupied.
>>You can do nothing with it, it is simply present. Then, ioremap maps it 
>>somewhere in high memory.
>>It should not conflict with kernel RAM, for which trivial mapping (+3G) 
>>used.
>>    
>>
>
>the thing is that typically you have a maximum of 168Mb or so of
>ioremap/vmalloc space (they share the same pool). That is, ff your
>system has >= 1Gb of ram, if it has less ran the ioremap/vmalloc space
>is bigger....
>
>  
>

Re: PCI Express support for 2.4 kernel

[Arjan van de Ven]

On Mon, Dec 15, 2003 at 03:58:22PM +0200, Vladimir Kondratiev wrote:
> Got it.
> Should I understand it this way: for system with >=1Gb RAM, I will be 
> unable to ioremap 256Mb region?
> It looks confusing. On my test system (don't ask details, I am not 
> alowed to share this info), I see
> video controller with 256Mb BAR. Does it mean this controller will not 
> work as well?

Video memory is generally not ioremap'd by the kernel (it may be mapped into
XFree's address space though)

> I thought about remapping only pages that have actual PCI devices behind,
> but this is problematic: access to config goes not always through 
> pci_exp_read_config_xxx and alike, raw access with bus/dev/fn numbers 
> used as well. And in 2.6, correct me if I wrong, raw access using 
> bus/dev/fn numbers goes to be the only way. Per-device access replaced 
> with per-bus, at least.

I would suspect you want to store the ioremap cookie for the config space in
the pci device struct; longer term that struct maybe needs to grow a few
function pointers to access config space too... 

Re: PCI Express support for 2.4 kernel

[Brian Gerst]

Vladimir Kondratiev wrote:

> Got it.
> Should I understand it this way: for system with >=1Gb RAM, I will be 
> unable to ioremap 256Mb region?
> It looks confusing. On my test system (don't ask details, I am not 
> alowed to share this info), I see
> video controller with 256Mb BAR. Does it mean this controller will not 
> work as well?

But that video card's BAR is not ioremapped into the kernel (XFree86 
will mmap it into userspace).

--
               Brian Gerst

Re: PCI Express support for 2.4 kernel

[Greg KH]

On Mon, Dec 15, 2003 at 03:58:22PM +0200, Vladimir Kondratiev wrote:
> I can statically remap only region for existing buses, this will be huge 
> save. It is 1MB per bus, this lead to typical 2-3MB instead of 256. To 
> be sure I can do this, I need to know that new bus can't be added on run 
> time. I don't think it is true, isn't it? Or do we have single point to 
> capture hot plug for new bus?

New busses can be added at run time, remember hotplug PCI systems.

thanks,

greg k-h

Re: PCI Express support for 2.4 kernel

[Alan Cox]

> It looks confusing. On my test system (don't ask details, I am not 
> alowed to share this info), I see
> video controller with 256Mb BAR. Does it mean this controller will not 
> work as well?

The 256Mb BAR turns up in a variety of graphic cards, and 512Mb in a few.
The kernel maps just enough memory for the frame buffer + cache. There isn't
enough space to map it all. User space apps may well have the 512Mb mapped 
into their address space, but that isnt the kernel one

> There is alternative solution, for each transaction to ioremap/unmap 
> corresponded page.
> I don't like it, it involves huge overhead.

ioremap means you can't use pci_exp_* from an interrupt so kmap is probably
needed. It is overhead but the design of the 386 left us with that problem on
x86 platforms and it isnt going to go away. On ia64/x86_64 etc its a non issue

Re: PCI Express support for 2.4 kernel

[Linus Torvalds]

On Mon, 15 Dec 2003, Vladimir Kondratiev wrote:
>
> Should I understand it this way: for system with >=1Gb RAM, I will be
> unable to ioremap 256Mb region?

Yes.

> It looks confusing. On my test system (don't ask details, I am not
> alowed to share this info), I see
> video controller with 256Mb BAR. Does it mean this controller will not
> work as well?

It works, but that's because the kernel doesn't even _try_ to map it into
the kernel virtual address space.

XFree86 maps it into its user address space (which is 3GB, and doesn't
have to try to map much else into it).

> There is alternative solution, for each transaction to ioremap/unmap
> corresponded page.

Nope, you can't do that. It's not really supported from interrupts and
early boot - both of which will want to access PCI config space.

> I don't like it, it involves huge overhead.

Indeed it would. But using fixmaps does _not_.

But a fixmap will be easy to use, and reasonable efficient: it will
allocate just one virtual page, and then it will force a TLB flush when it
switches over the mapping.

You can improve the efficiency by caching the "last fixmap entry" and only
doing "set_fixmap()" when changing devices.

You could also do a per-cpu fixmap, which would help further, but that
ends up being more work too. Probably not worth it, especially as it is
entirely possible that the hardware requires single-threaded access
anyway (ie I would not be surprised at all if the southbridge got very
confused if you tried to do overlapping config accesses)..

		Linus

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

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

I re-worked the patch. This time it uses fixmap; I added config 
parameters to
disable this code and to handle non-default base address.
Unless generic solution for auto detection found, it seems appropriate
way to go.

Autodetection problem isolated in 2 places: is_pcie_platform and, while 
it is not
do real auto detection, configuration option for custom base.

Vladimir.

Linus Torvalds wrote:

>On Mon, 15 Dec 2003, Vladimir Kondratiev wrote:
>  
>
>>There is alternative solution, for each transaction to ioremap/unmap
>>corresponded page.
>>    
>>
>
>Nope, you can't do that. It's not really supported from interrupts and
>early boot - both of which will want to access PCI config space.
>
>  
>
>>I don't like it, it involves huge overhead.
>>    
>>
>
>Indeed it would. But using fixmaps does _not_.
>
>But a fixmap will be easy to use, and reasonable efficient: it will
>allocate just one virtual page, and then it will force a TLB flush when it
>switches over the mapping.
>
>You can improve the efficiency by caching the "last fixmap entry" and only
>doing "set_fixmap()" when changing devices.
>
>You could also do a per-cpu fixmap, which would help further, but that
>ends up being more work too. Probably not worth it, especially as it is
>entirely possible that the hardware requires single-threaded access
>anyway (ie I would not be surprised at all if the southbridge got very
>confused if you tried to do overlapping config accesses)..
>
>		Linus
>  
>


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

Enable PCI Express access method for configuration space
This path includes:
 * configuration options
 * 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, fixmap used to create 4k window within config space.
For each access, window set to proper physical address. Last physical
address cached, to save page walk if the same device is accessed

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, with PCI-E enabled and disabled.

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

diff -dur linux-2.4.23/Documentation/Configure.help linux-2.4.23-pciexp/Documentation/Configure.help
--- linux-2.4.23/Documentation/Configure.help	2003-11-28 20:26:19.000000000 +0200
+++ linux-2.4.23-pciexp/Documentation/Configure.help	2003-12-16 11:44:11.000000000 +0200
@@ -4131,6 +4131,30 @@
   if that doesn't work. If unsure, go with the default, which is
   "Any".
 
+PCI-Express support
+CONFIG_PCI_EXPRESS
+  PCI-Express is standard for devices interconnection. It is compatible
+  with PCI. If your system is PCI-Express, and you want support for
+  extended PCI-Express features, set this option to Y. PCI-Express
+  is auto detected, thus saying Y here is safe.
+
+  This option provides access to 4k extended config space and uses
+  memory-mapped method to access config space. Kernel option
+  "pci=exp" allows to force PCI-Express mode.
+
+Enable PCI-E custom base address
+CONFIG_PCIEXP_USE_CUSTOM_BASE
+  Override autodetected base address for PCI-Express config space.
+
+  If you don't know really well what are you doing, say "N".
+
+PCI-Express base address
+CONFIG_PCI_EXPRESS_BASE
+  Override autodetected base address for PCI-Express config space.
+  Base will be this number * 0x10000000 (256Mb).
+
+  Default value is "0xe".
+
 PCI device name database
 CONFIG_PCI_NAMES
   By default, the kernel contains a database of all known PCI device
diff -dur linux-2.4.23/arch/i386/config.in linux-2.4.23-pciexp/arch/i386/config.in
--- linux-2.4.23/arch/i386/config.in	2003-11-28 20:26:19.000000000 +0200
+++ linux-2.4.23-pciexp/arch/i386/config.in	2003-12-16 11:18:46.000000000 +0200
@@ -292,6 +292,15 @@
       fi
       if [ "$CONFIG_PCI_GODIRECT" = "y" -o "$CONFIG_PCI_GOANY" = "y" ]; then
          define_bool CONFIG_PCI_DIRECT y
+         bool 'PCI-Express support' CONFIG_PCI_EXPRESS
+         if [ "$CONFIG_PCI_EXPRESS" = "y" ]; then
+            bool 'Enable PCI-E custom base address' CONFIG_PCIEXP_USE_CUSTOM_BASE
+            if [ "$CONFIG_PCIEXP_USE_CUSTOM_BASE" = "y" ]; then
+               hex 'PCI-Express base address' CONFIG_PCI_EXPRESS_BASE 0xe
+            else
+               define_hex CONFIG_PCI_EXPRESS_BASE 0xe
+            fi
+         fi
       fi
    fi
    bool 'ISA bus support' CONFIG_ISA
diff -dur linux-2.4.23/arch/i386/defconfig linux-2.4.23-pciexp/arch/i386/defconfig
--- linux-2.4.23/arch/i386/defconfig	2003-11-28 20:26:19.000000000 +0200
+++ linux-2.4.23-pciexp/arch/i386/defconfig	2003-12-16 11:32:40.000000000 +0200
@@ -79,6 +79,9 @@
 CONFIG_PCI_GOANY=y
 CONFIG_PCI_BIOS=y
 CONFIG_PCI_DIRECT=y
+# CONFIG_PCI_EXPRESS is not set
+# CONFIG_PCIEXP_USE_CUSTOM_BASE is not set
+CONFIG_PCI_EXPRESS_BASE=0xe
 CONFIG_PCI_NAMES=y
 # CONFIG_EISA is not set
 # CONFIG_MCA is not set
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-16 11:48:43.000000000 +0200
@@ -15,6 +15,11 @@
 #define PCI_PROBE_BIOS		0x0001
 #define PCI_PROBE_CONF1		0x0002
 #define PCI_PROBE_CONF2		0x0004
+
+#ifdef CONFIG_PCI_EXPRESS
+#define PCI_PROBE_EXP		0x0008
+#endif
+
 #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-16 12:00: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>
@@ -18,9 +21,17 @@
 #include <asm/smp.h>
 #include <asm/smpboot.h>
 
+#ifdef CONFIG_PCI_EXPRESS
+#include <asm/fixmap.h>
+#endif
+
 #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
+#ifdef CONFIG_PCI_EXPRESS
+ | PCI_PROBE_EXP
+#endif
+;
 
 int pcibios_last_bus = -1;
 struct pci_bus *pci_root_bus = NULL;
@@ -427,6 +438,211 @@
 	pci_conf2_write_config_dword
 };
 
+#ifdef CONFIG_PCI_EXPRESS
+/**
+ * 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 = CONFIG_PCI_EXPRESS_BASE * 0x10000000UL;
+
+/**
+ * Virtual address for RRBAR adjusted for the device being accessed.
+ */
+static void *rrbar_window_virt;
+
+/**
+ * It used to be #define, but I am going to modify it.
+ */
+extern int pci_cfg_space_size;
+
+/**
+ * Cached last accessed device.
+ * If the same device to be accessed, no need in set_fixmap
+ */
+static u32 pcie_last_accessed_device;
+
+/**
+ * I don't know how to detect it properly.
+ * assume it is PCI-E, sanity_check will
+ * stop me if it is not.
+ * 
+ * Also, this function supposed to set rrbar_phys
+ */
+static int is_pcie_platform(void)
+{ return 1; }
+
+/**
+ * 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.
+ * 
+ * @return 1 if OK, 0 if error
+ */
+static int pci_express_init(void)
+{
+	rrbar_window_virt = (void*)fix_to_virt(FIX_PCIE_CONFIG);
+	if (!is_pcie_platform())
+		return 0;
+	/**
+	 * adjust mapping for 1-st device (00:00.0)
+	 * to match pcie_last_accessed_device
+	 */
+	set_fixmap_nocache(FIX_PCIE_CONFIG, rrbar_phys);
+	return 1;
+}
+
+/**
+ * Shuts down PCI-E resources.
+ */
+static inline void pci_express_fini(void)
+{}
+
+/**
+ * Adjust window for the device.
+ * pci_config_lock should be held
+ */
+static inline void pci_exp_set_dev_base(int bus, int dev, int fn)
+{
+	u32 dev_base = (bus << 20) | (dev << 15) | (fn << 12);
+	if (dev_base != pcie_last_accessed_device) {
+		pcie_last_accessed_device = dev_base;
+		set_fixmap_nocache(FIX_PCIE_CONFIG, rrbar_phys | dev_base);
+	}
+}
+
+static int pci_exp_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
+{
+	unsigned long flags;
+	void *addr = rrbar_window_virt + reg;
+	if (((unsigned)bus > 255 || (unsigned)dev > 31
+	  || (unsigned)fn > 7 || (unsigned)reg > 4095)) 
+		return -EINVAL;
+	switch (len) {
+	case 2:
+		if (reg & 1)
+			return -EINVAL;
+		break;
+	case 4:
+		if (reg & 3)
+			return -EINVAL;
+		break;
+	}
+	spin_lock_irqsave(&pci_config_lock, flags);
+	pci_exp_set_dev_base(bus, dev, fn);
+	switch (len) {
+	case 1:
+		*value = readb(addr);
+		break;
+	case 2:
+		*value = readw(addr);
+		break;
+	case 4:
+		*value = readl(addr);
+		break;
+	}
+	spin_unlock_irqrestore(&pci_config_lock, flags);
+	return 0;
+}
+
+static int pci_exp_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
+{
+	unsigned long flags;
+	void *addr = rrbar_window_virt + reg;
+	if (((unsigned)bus > 255 || (unsigned)dev > 31
+	  || (unsigned)fn > 7 || (unsigned)reg > 4095)) 
+		return -EINVAL;
+	switch (len) {
+	case 2:
+		if (reg & 1)
+			return -EINVAL;
+		break;
+	case 4:
+		if (reg & 3)
+			return -EINVAL;
+		break;
+	}
+	spin_lock_irqsave(&pci_config_lock, flags);
+	pci_exp_set_dev_base(bus, dev, fn);
+	switch (len) {
+	case 1:
+		writeb(value, addr);
+		break;
+	case 2:
+		writew(value, addr);
+		break;
+	case 4:
+		writel(value, addr);
+		break;
+	}
+	/* dummy read to flush PCI write */
+	readb(addr);
+	spin_unlock_irqrestore(&pci_config_lock, flags);
+	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
+};
+#endif /* CONFIG_PCI_EXPRESS */
 
 /*
  * Before we decide to use direct hardware access mechanisms, we try to do some
@@ -465,6 +681,25 @@
 
 	__save_flags(flags); __cli();
 
+#ifdef CONFIG_PCI_EXPRESS
+	/**
+	 * 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");
+			printk(KERN_INFO "PCI-Express config at 0x%08x\n", rrbar_phys);
+			request_mem_region(rrbar_phys, 0x10000000UL, "PCI-Express config space");
+			return &pci_express_conf;
+		} else {
+			pci_express_fini();
+		}
+	}
+#endif /* CONFIG_PCI_EXPRESS */
+
 	/*
 	 * Check if configuration type 1 works.
 	 */
@@ -1398,17 +1633,25 @@
 #endif
 
 #ifdef CONFIG_PCI_DIRECT
-	if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2)) 
-		&& (tmp = pci_check_direct())) {
+	if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2
+#ifdef CONFIG_PCI_EXPRESS
+			| PCI_PROBE_EXP
+#endif
+		)) && (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;
+		} 
+#ifdef CONFIG_PCI_EXPRESS
+		else if (pci_root_ops == &pci_express_conf) {
+			pci_config_read = pci_exp_read;
+			pci_config_write = pci_exp_write;
 		}
+#endif
 	}
 #endif
 
@@ -1489,6 +1732,12 @@
 		pci_probe = PCI_PROBE_CONF2 | PCI_NO_CHECKS;
 		return NULL;
 	}
+#ifdef CONFIG_PCI_EXPRESS
+	else if (!strcmp(str, "exp")) {
+		pci_probe = PCI_PROBE_EXP | PCI_NO_CHECKS;
+		return NULL;
+	}
+#endif
 #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;
 }
 
diff -dur linux-2.4.23/include/asm-i386/fixmap.h linux-2.4.23-pciexp/include/asm-i386/fixmap.h
--- linux-2.4.23/include/asm-i386/fixmap.h	2003-12-11 14:52:05.000000000 +0200
+++ linux-2.4.23-pciexp/include/asm-i386/fixmap.h	2003-12-16 11:46:29.000000000 +0200
@@ -76,6 +76,9 @@
 	FIX_ACPI_BEGIN,
 	FIX_ACPI_END = FIX_ACPI_BEGIN + FIX_ACPI_PAGES - 1,
 #endif
+#ifdef CONFIG_PCI_EXPRESS
+	FIX_PCIE_CONFIG, /* Window within PCI-E config space*/
+#endif 
 	__end_of_permanent_fixed_addresses,
 	/* temporary boot-time mappings, used before ioremap() is functional */
 #define NR_FIX_BTMAPS	16

Re: PCI Express support for 2.4 kernel

[Linus Torvalds]

On Tue, 16 Dec 2003, Vladimir Kondratiev wrote:
>
> I re-worked the patch. This time it uses fixmap; I added config
> parameters to disable this code and to handle non-default base address.

Ok, this looks sane to me.

However, I have a totally independent question: do new PCI Express host
bridges really not just suppor the old PCI config access interface?

Quite frankly, if I was a manager in charge of a PCI Express host bridge,
and it didn't support the old C8C IO access patterns, I'd be so ashamed of
myself that I'd kill my whole development team with rat poison, and then
blame them for the mistake(*).

Do we know of any sudden suspicious death waves inside certain groups at
Intel?

		Linus

(*) That's how managers work, after all. Long gone are the days when
personal shame caused you to take personal responsibility.

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Linus,

Hopefully, they (or we, should I count myself?) are not completely brain 
dead.
Old method still works.
All game is about to give full 4k. It will be required for new deices.
If all you need is PCI devices behind PCI-E/PCI brigde,
it will work fine with old method.

Vladimir.

Linus Torvalds wrote:

>On Tue, 16 Dec 2003, Vladimir Kondratiev wrote:
>  
>
>>I re-worked the patch. This time it uses fixmap; I added config
>>parameters to disable this code and to handle non-default base address.
>>    
>>
>
>Ok, this looks sane to me.
>
>However, I have a totally independent question: do new PCI Express host
>bridges really not just suppor the old PCI config access interface?
>
>Quite frankly, if I was a manager in charge of a PCI Express host bridge,
>and it didn't support the old C8C IO access patterns, I'd be so ashamed of
>myself that I'd kill my whole development team with rat poison, and then
>blame them for the mistake(*).
>
>Do we know of any sudden suspicious death waves inside certain groups at
>Intel?
>
>		Linus
>
>(*) That's how managers work, after all. Long gone are the days when
>personal shame caused you to take personal responsibility.
>  
>

Re: PCI Express support for 2.4 kernel

[Linus Torvalds]

On Wed, 17 Dec 2003, Vladimir Kondratiev wrote:
> 
> Hopefully, they (or we, should I count myself?) are not completely brain
> dead. Old method still works.

Good. I just wanted to check from a timing perspective - it means that
this won't be an issue for most people for a while (ie until we start
seeing actual PCI-X-specific hardware and drivers rather than just the
support chipsets - and I obviously have no idea how long that will take)

So if this will only matter for PCI-X drivers and not for discovery etc, I
wonder if it wouldn't make sense to have this as a totally separate
function? Instead of trying to make the existing "pci_config_xxxx()" 
stuff work with PCI-X, wouldn't it be nicer to have the driver just map 
its config space on probe?

That way there would be no scalability issues (only as many pages mapped 
as there are actual physical PCI-X devices that care) _and_ there would be 
no performance issues (the users would not need to walk page tables or 
flush the TLB - they'd just have a direct-mapped pointer).

You could do it with just ioremap(), but you'd really want to abstract it 
out a bit, and have a "[un]map_pcix_config()" function?

			Linus

Re: PCI Express support for 2.4 kernel

[Jeff Garzik]

Linus Torvalds wrote:
> So if this will only matter for PCI-X drivers and not for discovery etc, I
> wonder if it wouldn't make sense to have this as a totally separate
> function? Instead of trying to make the existing "pci_config_xxxx()" 
> stuff work with PCI-X, wouldn't it be nicer to have the driver just map 
> its config space on probe?

Not a bad idea...  After posting yesterday on this thread, I had the 
thought:  Just like PCI has readl() and sbus has sbus_readl(), why not 
pciex_cfg_readl() ?

Any PCI-Ex drivers would obviously _know_ they are PCI Ex, and they 
could communicate that by virtue of simply using new functions.  Older 
drivers for older hardware would use the old API and not care... 
Further, PCI-Ex operations are already basically readl/writel anyway, so 
going through the forest of pci_cfg_ops pointers and such would just add 
needless layering.


> You could do it with just ioremap(), but you'd really want to abstract it 
> out a bit, and have a "[un]map_pcix_config()" function?

Why not just work within the existing API? 
pci_{enable,disable}_device() seems fairly appropriate, as that's a 
quite clear signal of the bounds within which the driver must work.

pci_enable_device() is already defined as "the PCI device's resources 
may not be available before <this> point."

	Jeff

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

I considered it in the beginning. For device driver itself, it is not a 
problem. For example, I can define
struct pcie_dev_config {
  void* base;
  int (*read)(pcie_dev_config* conf, unsigned reg, int len, u32* value);
  int (*write)(pcie_dev_config* conf, unsigned reg, int len, u32 value);
}
and have constructor/destructor functions that use device coordinates to 
calculate physical base and ioremap() it.

What you will miss, is uniform access for all devices, including those 
you are not managing as PCI-E. Notable
example is bridges. I can't provide more info (see prev. mail about 
brain dead, I don't want it to be my last day at work),
but you may found appropriate to tweak some stuff for bridges in 
extended space. One may use /proc/bus/pci/ or 'setpci'
for this. Obviously, in this case you have no driver, and generic access 
method would help you.
Also, 'lspci' don't know about device drivers, it need generic way to 
access config.

And, strictly say, if it is method that replaces old one, wouldn't it 
more appropriate to use it rather then rely
on backward compatibility hooks? I know, "work - don't touch", but...

Vladimir.

Jeff Garzik wrote:

> Linus Torvalds wrote:
>
>> So if this will only matter for PCI-X drivers and not for discovery 
>> etc, I
>> wonder if it wouldn't make sense to have this as a totally separate
>> function? Instead of trying to make the existing "pci_config_xxxx()" 
>> stuff work with PCI-X, wouldn't it be nicer to have the driver just 
>> map its config space on probe?
>
>
> Not a bad idea...  After posting yesterday on this thread, I had the 
> thought:  Just like PCI has readl() and sbus has sbus_readl(), why not 
> pciex_cfg_readl() ?
>
> Any PCI-Ex drivers would obviously _know_ they are PCI Ex, and they 
> could communicate that by virtue of simply using new functions.  Older 
> drivers for older hardware would use the old API and not care... 
> Further, PCI-Ex operations are already basically readl/writel anyway, 
> so going through the forest of pci_cfg_ops pointers and such would 
> just add needless layering.
>
>
>> You could do it with just ioremap(), but you'd really want to 
>> abstract it out a bit, and have a "[un]map_pcix_config()" function?
>
>
> Why not just work within the existing API? 
> pci_{enable,disable}_device() seems fairly appropriate, as that's a 
> quite clear signal of the bounds within which the driver must work.
>
> pci_enable_device() is already defined as "the PCI device's resources 
> may not be available before <this> point."
>
>     Jeff
>
>

Re: PCI Express support for 2.4 kernel

[Linus Torvalds]

On Wed, 17 Dec 2003, Vladimir Kondratiev wrote:
> 
> What you will miss, is uniform access for all devices, including those
> you are not managing as PCI-E. Notable example is bridges. I can't
> provide more info (see prev. mail about brain dead, I don't want it to
> be my last day at work), but you may found appropriate to tweak some
> stuff for bridges in extended space. One may use /proc/bus/pci/ or
> 'setpci' for this. Obviously, in this case you have no driver, and
> generic access method would help you. Also, 'lspci' don't know about
> device drivers, it need generic way to access config.

Ok, the /proc/bus/pci argument is a pretty good one, so having a uniform 
way to access the config space sounds fair. 

And it doesn't look that ugly any more, so I guess if just the detection 
can be fixed (and real devices enter the market) I don't see any reason 
not to do it that way.

		Linus

Re: PCI Express support for 2.4 kernel

[Arjan van de Ven]

On Wed, Dec 17, 2003 at 01:55:40AM -0500, Jeff Garzik wrote:
> Linus Torvalds wrote:
> >So if this will only matter for PCI-X drivers and not for discovery etc, I
> >wonder if it wouldn't make sense to have this as a totally separate
> >function? Instead of trying to make the existing "pci_config_xxxx()" 
> >stuff work with PCI-X, wouldn't it be nicer to have the driver just map 
> >its config space on probe?
> 
> Not a bad idea...  After posting yesterday on this thread, I had the 
> thought:  Just like PCI has readl() and sbus has sbus_readl(), why not 
> pciex_cfg_readl() ?
> 
> Any PCI-Ex drivers would obviously _know_ they are PCI Ex, and they 
> could communicate that by virtue of simply using new functions.  Older 
> drivers for older hardware would use the old API and not care... 
> Further, PCI-Ex operations are already basically readl/writel anyway, so 
> going through the forest of pci_cfg_ops pointers and such would just add 
> needless layering.

BUT powermanagement and co will need to potentially do stuff too with the
config space...

Re: PCI Express support for 2.4 kernel

[Martin Mares]

Hello!

> BUT powermanagement and co will need to potentially do stuff too with the
> config space...

Yes, but again they either access the registers already existing in
normal PCI where they can use the old access mechanism, or they need to
access the extended registers, but in this case it's a code specific for
PCI-X which can use different functions for that, isnt't?

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
If a train station is where the train stops, what is a work station?

Re: PCI Express support for 2.4 kernel

[Alan Cox]

On Wed, Dec 17, 2003 at 09:22:35AM +0100, Arjan van de Ven wrote:
> > Any PCI-Ex drivers would obviously _know_ they are PCI Ex, and they 
> > could communicate that by virtue of simply using new functions.  Older 
> > drivers for older hardware would use the old API and not care... 
> > Further, PCI-Ex operations are already basically readl/writel anyway, so 
> > going through the forest of pci_cfg_ops pointers and such would just add 
> > needless layering.
> 
> BUT powermanagement and co will need to potentially do stuff too with the
> config space...

And X11 will want to access it via /proc interfaces. And someone will eventually
go and design a different way to access PCI-EX for their hardware 8)

The PCI layer is a nice abstraction and config space is slow anyway

Re: PCI Express support for 2.4 kernel

[Geert Uytterhoeven]

On Tue, 16 Dec 2003, Linus Torvalds wrote:
> On Wed, 17 Dec 2003, Vladimir Kondratiev wrote:
> > Hopefully, they (or we, should I count myself?) are not completely brain
> > dead. Old method still works.
>
> Good. I just wanted to check from a timing perspective - it means that
> this won't be an issue for most people for a while (ie until we start
> seeing actual PCI-X-specific hardware and drivers rather than just the
> support chipsets - and I obviously have no idea how long that will take)

    [...]

For the record: PCI Express is _not_ PCI-X.

PCI Express is a completely new bus system with new physical connectors. From a
software point of view, it's (more or less) backwards-compatible with PCI,
while PCI-X is completely backwards compatible with PCI.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: PCI Express support for 2.4 kernel

[Linus Torvalds]

On Wed, 17 Dec 2003, Geert Uytterhoeven wrote:
> 
> For the record: PCI Express is _not_ PCI-X.

Ok, but "PCI Express" is too damn long to write, so we'll have to have 
_some_ sane name for it without typing for half an hour.

		Linus "retard on a keyboard" Torvalds

Re: PCI Express support for 2.4 kernel

[Geert Uytterhoeven]

On Wed, 17 Dec 2003, Linus Torvalds wrote:
> On Wed, 17 Dec 2003, Geert Uytterhoeven wrote:
> > For the record: PCI Express is _not_ PCI-X.
>
> Ok, but "PCI Express" is too damn long to write, so we'll have to have
> _some_ sane name for it without typing for half an hour.

3GIO? That was the original name :-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: PCI Express support for 2.4 kernel

[Dan Hopper]

Linus Torvalds <torvalds@osdl.org> remarked:
> 
> On Wed, 17 Dec 2003, Geert Uytterhoeven wrote:
> > 
> > For the record: PCI Express is _not_ PCI-X.
> 
> Ok, but "PCI Express" is too damn long to write, so we'll have to have 
> _some_ sane name for it without typing for half an hour.

FWIW, "PCI-E" is in common use in these parts.

Also, wrt the config space backward compatibility issue, it is my
understanding that the PCI-E root complex and PCI-E devices can be
enumerated and used successfully with no software changes, within
the constraints of PCI.  The BIOS or OS should be able to enumerate
devices, probe BARs and assign resources, perform basic error
handling, etc. without any PCI-E-specific changes. 

The hardware is required to be able to initialize PCI-E links, set
things up to sane states, and so forth without software assistance
in order to make this magic happen.  It would be interesting to hear
from Vladimir as to whether or not this is happening with his PCI-E
test system.

Having said all that, it's obviously preferable to end up with
native OS and BIOS support for the PCI-E extended configuration
space, extra error reporting mechanisms, etc.  Native PCI-E devices
hanging off the bus somewhere might not work (or, at least, work
well) with an OS that doesn't grok the PCI-E extensions.  

Dan

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Dan,

all your statements are correct. Indeed, all PCI devices works without 
extra knowledge about PCI-E.
 I have nothing to add.
Vladimir.

Dan Hopper wrote:

>Linus Torvalds <torvalds@osdl.org> remarked:
>  
>
>>On Wed, 17 Dec 2003, Geert Uytterhoeven wrote:
>>    
>>
>>>For the record: PCI Express is _not_ PCI-X.
>>>      
>>>
>>Ok, but "PCI Express" is too damn long to write, so we'll have to have 
>>_some_ sane name for it without typing for half an hour.
>>    
>>
>
>FWIW, "PCI-E" is in common use in these parts.
>
>Also, wrt the config space backward compatibility issue, it is my
>understanding that the PCI-E root complex and PCI-E devices can be
>enumerated and used successfully with no software changes, within
>the constraints of PCI.  The BIOS or OS should be able to enumerate
>devices, probe BARs and assign resources, perform basic error
>handling, etc. without any PCI-E-specific changes. 
>
>The hardware is required to be able to initialize PCI-E links, set
>things up to sane states, and so forth without software assistance
>in order to make this magic happen.  It would be interesting to hear
>from Vladimir as to whether or not this is happening with his PCI-E
>test system.
>
>Having said all that, it's obviously preferable to end up with
>native OS and BIOS support for the PCI-E extended configuration
>space, extra error reporting mechanisms, etc.  Native PCI-E devices
>hanging off the bus somewhere might not work (or, at least, work
>well) with an OS that doesn't grok the PCI-E extensions.  
>
>Dan
>  
>

Re: PCI Express support for 2.4 kernel

[Martin Mares]

> For the record: PCI Express is _not_ PCI-X.

Yes, that was just a typo, sorry for the confusion.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
A. G. Bell is alive and well in New York and still waiting for the dial tone.

Re: PCI Express support for 2.4 kernel

[Jeff Garzik]

Vladimir Kondratiev wrote:

Definitely looks a lot better, thanks.

Still a few problems to consider...


> diff -dur linux-2.4.23/arch/i386/config.in linux-2.4.23-pciexp/arch/i386/config.in
> --- linux-2.4.23/arch/i386/config.in	2003-11-28 20:26:19.000000000 +0200
> +++ linux-2.4.23-pciexp/arch/i386/config.in	2003-12-16 11:18:46.000000000 +0200
> @@ -292,6 +292,15 @@
>        fi
>        if [ "$CONFIG_PCI_GODIRECT" = "y" -o "$CONFIG_PCI_GOANY" = "y" ]; then
>           define_bool CONFIG_PCI_DIRECT y
> +         bool 'PCI-Express support' CONFIG_PCI_EXPRESS
> +         if [ "$CONFIG_PCI_EXPRESS" = "y" ]; then
> +            bool 'Enable PCI-E custom base address' CONFIG_PCIEXP_USE_CUSTOM_BASE
> +            if [ "$CONFIG_PCIEXP_USE_CUSTOM_BASE" = "y" ]; then
> +               hex 'PCI-Express base address' CONFIG_PCI_EXPRESS_BASE 0xe
> +            else
> +               define_hex CONFIG_PCI_EXPRESS_BASE 0xe
> +            fi
> +         fi
>        fi
>     fi
>     bool 'ISA bus support' CONFIG_ISA

This is OK for development...   But until (if ever?) there is a standard 
way to detect PCI Express, I think it is better to maintain a list of 
chipsets that support PCI Express.

Users are really going to hate this, once the first chipset comes out 
that uses a non-standard address.


> 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-16 12:00:46.000000000 +0200

> +/**
> + * I don't know how to detect it properly.
> + * assume it is PCI-E, sanity_check will
> + * stop me if it is not.
> + * 
> + * Also, this function supposed to set rrbar_phys
> + */
> +static int is_pcie_platform(void)
> +{ return 1; }
> +
> +/**
> + * 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.
> + * 
> + * @return 1 if OK, 0 if error
> + */

Well, I agree with the comment, but that's not what the code does.

Where is your check for 0xff?

Further, is_pcie_platform() unconditionally returns 1... and is only 
used once, in PCI-Ex-specific code.


> +static int pci_express_init(void)
> +{
> +	rrbar_window_virt = (void*)fix_to_virt(FIX_PCIE_CONFIG);
> +	if (!is_pcie_platform())
> +		return 0;
> +	/**
> +	 * adjust mapping for 1-st device (00:00.0)
> +	 * to match pcie_last_accessed_device
> +	 */
> +	set_fixmap_nocache(FIX_PCIE_CONFIG, rrbar_phys);
> +	return 1;
> +}

Longer term, we want to provide some way to have the read/write routines 
generic, but support arch-specific mapping methods, I would think...

64-bit arches probably wouldn't need a spinlock at all for each access, 
I bet, since it's just a single MMIO read or write.


> +/**
> + * Shuts down PCI-E resources.
> + */
> +static inline void pci_express_fini(void)
> +{}
> +
> +/**
> + * Adjust window for the device.
> + * pci_config_lock should be held
> + */
> +static inline void pci_exp_set_dev_base(int bus, int dev, int fn)
> +{
> +	u32 dev_base = (bus << 20) | (dev << 15) | (fn << 12);
> +	if (dev_base != pcie_last_accessed_device) {
> +		pcie_last_accessed_device = dev_base;
> +		set_fixmap_nocache(FIX_PCIE_CONFIG, rrbar_phys | dev_base);
> +	}
> +}
> +
> +static int pci_exp_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
> +{
> +	unsigned long flags;
> +	void *addr = rrbar_window_virt + reg;
> +	if (((unsigned)bus > 255 || (unsigned)dev > 31
> +	  || (unsigned)fn > 7 || (unsigned)reg > 4095)) 
> +		return -EINVAL;
> +	switch (len) {
> +	case 2:
> +		if (reg & 1)
> +			return -EINVAL;
> +		break;
> +	case 4:
> +		if (reg & 3)
> +			return -EINVAL;
> +		break;
> +	}
> +	spin_lock_irqsave(&pci_config_lock, flags);
> +	pci_exp_set_dev_base(bus, dev, fn);
> +	switch (len) {
> +	case 1:
> +		*value = readb(addr);
> +		break;
> +	case 2:
> +		*value = readw(addr);
> +		break;
> +	case 4:
> +		*value = readl(addr);
> +		break;
> +	}
> +	spin_unlock_irqrestore(&pci_config_lock, flags);
> +	return 0;
> +}
> +
> +static int pci_exp_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
> +{
> +	unsigned long flags;
> +	void *addr = rrbar_window_virt + reg;
> +	if (((unsigned)bus > 255 || (unsigned)dev > 31
> +	  || (unsigned)fn > 7 || (unsigned)reg > 4095)) 
> +		return -EINVAL;
> +	switch (len) {
> +	case 2:
> +		if (reg & 1)
> +			return -EINVAL;
> +		break;
> +	case 4:
> +		if (reg & 3)
> +			return -EINVAL;
> +		break;
> +	}

I don't see that read and write should ever return an error.

The above EINVAL conditions are a BUG().


> +	spin_lock_irqsave(&pci_config_lock, flags);
> +	pci_exp_set_dev_base(bus, dev, fn);
> +	switch (len) {
> +	case 1:
> +		writeb(value, addr);
> +		break;
> +	case 2:
> +		writew(value, addr);
> +		break;
> +	case 4:
> +		writel(value, addr);
> +		break;
> +	}
> +	/* dummy read to flush PCI write */
> +	readb(addr);

This is going to choke some hardware, I guarantee.

You always want to make sure your flush is of the same size at the 
write.  Reading a byte from an address that the hardware defines as 
"32-bit writes only" can get ugly real quick ;-)




> 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;
>  }
>  

Something I missed in the previous emails comments:

The above seems wrong, to blindly assume PCI-Ex means PCI config space 
will always be 4k.  What about downstream PCI bridges, and ancient 
devices with only 256 bytes of config registers?

It really seems like the config space size should be per-device or per-bus.

	Jeff

Re: PCI Express support for 2.4 kernel

[Arjan van de Ven]

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

On Tue, 2003-12-16 at 18:10, Jeff Garzik wrote:

> > +	spin_lock_irqsave(&pci_config_lock, flags);
> > +	pci_exp_set_dev_base(bus, dev, fn);
> > +	switch (len) {
> > +	case 1:
> > +		writeb(value, addr);
> > +		break;
> > +	case 2:
> > +		writew(value, addr);
> > +		break;
> > +	case 4:
> > +		writel(value, addr);
> > +		break;
> > +	}
> > +	/* dummy read to flush PCI write */
> > +	readb(addr);
> 
> This is going to choke some hardware, I guarantee.
> 
> You always want to make sure your flush is of the same size at the 
> write.  Reading a byte from an address that the hardware defines as 
> "32-bit writes only" can get ugly real quick ;-)

also reading back addr might not be the best choice in case some
registers have side effects on reading, it's probably better to read
back an address that is known to be ok to read (like the vendor ID
field)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: PCI Express support for 2.4 kernel

[Jeff Garzik]

Arjan van de Ven wrote:
> On Tue, 2003-12-16 at 18:10, Jeff Garzik wrote:
>>This is going to choke some hardware, I guarantee.
>>You always want to make sure your flush is of the same size at the 
>>write.  Reading a byte from an address that the hardware defines as 
>>"32-bit writes only" can get ugly real quick ;-)
> 
> 
> also reading back addr might not be the best choice in case some
> registers have side effects on reading, it's probably better to read
> back an address that is known to be ok to read (like the vendor ID
> field)


Hum.  Never seen this in a PCI config register, but it's certainly 
possible...  good point.

	Jeff

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Arjan van de Ven wrote:

>>>+	/* dummy read to flush PCI write */
>>>+	readb(addr);
>>>      
>>>
>>This is going to choke some hardware, I guarantee.
>>
>>You always want to make sure your flush is of the same size at the 
>>write.  Reading a byte from an address that the hardware defines as 
>>"32-bit writes only" can get ugly real quick ;-)
>>    
>>
>
>also reading back addr might not be the best choice in case some
>registers have side effects on reading, it's probably better to read
>back an address that is known to be ok to read (like the vendor ID
>field)
>
>  
>
Good idea!

Re: PCI Express support for 2.4 kernel

[Richard B. Johnson]

On Wed, 17 Dec 2003, Vladimir Kondratiev wrote:

> Arjan van de Ven wrote:
>
> >>>+	/* dummy read to flush PCI write */
> >>>+	readb(addr);
> >>>
> >>>
> >>This is going to choke some hardware, I guarantee.
> >>
> >>You always want to make sure your flush is of the same size at the
> >>write.  Reading a byte from an address that the hardware defines as
> >>"32-bit writes only" can get ugly real quick ;-)
> >>
> >>
> >
> >also reading back addr might not be the best choice in case some
> >registers have side effects on reading, it's probably better to read
> >back an address that is known to be ok to read (like the vendor ID
> >field)
> >
> >
> >
> Good idea!


You should not abitrarily flush all the writes with a read
after each write. The PCI/Bus is a FIFO, stuff will get to
the hardware in the order written. If you "flush" every write,
you will seriously hurt the performance. I don't like that
MACRO/Function, whatever it is with the switch shown previously.
All that code just for a "@(*^&$(*^(" write will screw up
performance. I've looked at the result of some the the MACRO
expansions, in particular the get_user, put_user, and copy_to/from
macros where there is a switch. It is not pre-processed out.


Cheers,
Dick Johnson
Penguin : Linux version 2.4.22 on an i686 machine (797.90 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Jeff Garzik wrote:

> Vladimir Kondratiev wrote:
>
> Definitely looks a lot better, thanks.
>
> Still a few problems to consider...
>
>
>> diff -dur linux-2.4.23/arch/i386/config.in 
>> linux-2.4.23-pciexp/arch/i386/config.in
>> --- linux-2.4.23/arch/i386/config.in    2003-11-28 20:26:19.000000000 
>> +0200
>> +++ linux-2.4.23-pciexp/arch/i386/config.in    2003-12-16 
>> 11:18:46.000000000 +0200
>> @@ -292,6 +292,15 @@
>>        fi
>>        if [ "$CONFIG_PCI_GODIRECT" = "y" -o "$CONFIG_PCI_GOANY" = "y" 
>> ]; then
>>           define_bool CONFIG_PCI_DIRECT y
>> +         bool 'PCI-Express support' CONFIG_PCI_EXPRESS
>> +         if [ "$CONFIG_PCI_EXPRESS" = "y" ]; then
>> +            bool 'Enable PCI-E custom base address' 
>> CONFIG_PCIEXP_USE_CUSTOM_BASE
>> +            if [ "$CONFIG_PCIEXP_USE_CUSTOM_BASE" = "y" ]; then
>> +               hex 'PCI-Express base address' 
>> CONFIG_PCI_EXPRESS_BASE 0xe
>> +            else
>> +               define_hex CONFIG_PCI_EXPRESS_BASE 0xe
>> +            fi
>> +         fi
>>        fi
>>     fi
>>     bool 'ISA bus support' CONFIG_ISA
>
>
> This is OK for development...   But until (if ever?) there is a 
> standard way to detect PCI Express, I think it is better to maintain a 
> list of chipsets that support PCI Express.
>
> Users are really going to hate this, once the first chipset comes out 
> that uses a non-standard address.
>
Sure, it need to be replaced, or at least complemented, with real auto 
detection. If I only could provide this list...

>> +/**
>> + * I don't know how to detect it properly.
>> + * assume it is PCI-E, sanity_check will
>> + * stop me if it is not.
>> + * + * Also, this function supposed to set rrbar_phys
>> + */
>> +static int is_pcie_platform(void)
>> +{ return 1; }
>> +
>> +/**
>> + * 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.
>> + * + * @return 1 if OK, 0 if error
>> + */
>
>
> Well, I agree with the comment, but that's not what the code does.
>
> Where is your check for 0xff?
>
sanity_check do it for me.

> Further, is_pcie_platform() unconditionally returns 1... and is only 
> used once, in PCI-Ex-specific code.
>
See above, it is placeholder for real auto detection routine.

>
> Longer term, we want to provide some way to have the read/write 
> routines generic, but support arch-specific mapping methods, I would 
> think...
>
> 64-bit arches probably wouldn't need a spinlock at all for each 
> access, I bet, since it's just a single MMIO read or write.
>
Yes, separation into generic and platform specific part seems nice, but 
besides memory mapping and locking, all you have is
very simple arithmetic. Does it worth the work for separation?

For 64-bit world, agree, it could and should be done uniform.

>> +    switch (len) {
>> +    case 2:
>> +        if (reg & 1)
>> +            return -EINVAL;
>> +        break;
>> +    case 4:
>> +        if (reg & 3)
>> +            return -EINVAL;
>> +        break;
>> +    }
>
>
> I don't see that read and write should ever return an error.
>
> The above EINVAL conditions are a BUG().
>
Indeed, it may be better to BUG() here. I did not paid attention. By the 
way, for other access methods alignment is simply not verified. 
Probable, it is ensured by some other way and I can just remove this test?

>>
>> +    /* dummy read to flush PCI write */
>> +    readb(addr);
>
>
> This is going to choke some hardware, I guarantee.
>
> You always want to make sure your flush is of the same size at the 
> write.  Reading a byte from an address that the hardware defines as 
> "32-bit writes only" can get ugly real quick ;-)
>
Good point. s/readb(addr)/readl(addr & ~3)/

> Something I missed in the previous emails comments:
>
> The above seems wrong, to blindly assume PCI-Ex means PCI config space 
> will always be 4k.  What about downstream PCI bridges, and ancient 
> devices with only 256 bytes of config registers?
>
> It really seems like the config space size should be per-device or 
> per-bus.
>
Not this bad. For PCI devices after bridges, 4k also provided. 
Everything after 256 bytes is simply useless, but present. It reads as 0.

Re: PCI Express support for 2.4 kernel

[Greg KH]

Minor code comments below:

On Tue, Dec 16, 2003 at 12:20:39PM +0200, Vladimir Kondratiev wrote:
> +
> +#ifdef CONFIG_PCI_EXPRESS
> +#define PCI_PROBE_EXP		0x0008
> +#endif
> +

If you change this to:
#ifdef CONFIG_PCI_EXPRESS
#define PCI_PROBE_EXP		0x0008
#else
#define PCI_PROBE_EXP		0x0000
#endif

You can get rid of a number of "#ifdef CONFIG_PCI_EXPRESS" statements
later on in the .c files by just always using the PCI_PROBE_EXP value.
That cleans up the patch a bit.

> -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
> +#ifdef CONFIG_PCI_EXPRESS
> + | PCI_PROBE_EXP
> +#endif
> +;

Like right there :)

> +/**
> + * 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 = CONFIG_PCI_EXPRESS_BASE * 0x10000000UL;

<snip>

> +/**
> + * I don't know how to detect it properly.
> + * assume it is PCI-E, sanity_check will
> + * stop me if it is not.
> + * 
> + * Also, this function supposed to set rrbar_phys
> + */
> +static int is_pcie_platform(void)
> +{ return 1; }

Shouldn't your comments at least match your code for the rrbar_phys
statement for your first release?  :)

> +/**
> + * 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.
> + * 
> + * @return 1 if OK, 0 if error
> + */
> +static int pci_express_init(void)
> +{
> +	rrbar_window_virt = (void*)fix_to_virt(FIX_PCIE_CONFIG);
> +	if (!is_pcie_platform())
> +		return 0;

Call fix_to_virt() after you do the check and not before?


> +/**
> + * Shuts down PCI-E resources.
> + */
> +static inline void pci_express_fini(void)
> +{}

If this isn't needed, why have it anymore?

> +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
> +};

C99 initializers here?

> +			printk(KERN_INFO "PCI-Express config at 0x%08x\n", rrbar_phys);

"%p" to show the address might be nicer.


thanks,

greg k-h

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Greg KH wrote:

>Minor code comments below:
>
>On Tue, Dec 16, 2003 at 12:20:39PM +0200, Vladimir Kondratiev wrote:
>  
>
>>+
>>+#ifdef CONFIG_PCI_EXPRESS
>>+#define PCI_PROBE_EXP		0x0008
>>+#endif
>>+
>>    
>>
>
>If you change this to:
>#ifdef CONFIG_PCI_EXPRESS
>#define PCI_PROBE_EXP		0x0008
>#else
>#define PCI_PROBE_EXP		0x0000
>#endif
>
>You can get rid of a number of "#ifdef CONFIG_PCI_EXPRESS" statements
>later on in the .c files by just always using the PCI_PROBE_EXP value.
>That cleans up the patch a bit.
>  
>
Good point. Agree.

>>+/**
>>+ * 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 = CONFIG_PCI_EXPRESS_BASE * 0x10000000UL;
>>    
>>
>
><snip>
>
>  
>
>>+/**
>>+ * I don't know how to detect it properly.
>>+ * assume it is PCI-E, sanity_check will
>>+ * stop me if it is not.
>>+ * 
>>+ * Also, this function supposed to set rrbar_phys
>>+ */
>>+static int is_pcie_platform(void)
>>+{ return 1; }
>>    
>>
>
>Shouldn't your comments at least match your code for the rrbar_phys
>statement for your first release?  :)
>  
>
Default moved to CONFIG_PCI_EXPRESS_BASE.
Idea is some time later we fill replace is_pcie_platform with real auto 
detection...

>>+static int pci_express_init(void)
>>+{
>>+	rrbar_window_virt = (void*)fix_to_virt(FIX_PCIE_CONFIG);
>>+	if (!is_pcie_platform())
>>+		return 0;
>>    
>>
>
>Call fix_to_virt() after you do the check and not before?
>
>  
>
... and I assume virtual address may be used by auto detection code.

>  
>
>>+/**
>>+ * Shuts down PCI-E resources.
>>+ */
>>+static inline void pci_express_fini(void)
>>+{}
>>    
>>
>
>If this isn't needed, why have it anymore?
>  
>
Placeholder, to not forget. It is called in case auto detection says it 
is not PCI-E.

>  
>
>>+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
>>+};
>>    
>>
>
>C99 initializers here?
>  
>
cut'n'paste from other methods, to keep style. If change, let's change all.

>  
>
>>+			printk(KERN_INFO "PCI-Express config at 0x%08x\n", rrbar_phys);
>>    
>>
>
>"%p" to show the address might be nicer.
>
>  
>
I print phys. address, it is u32. Do you mean (void*)rrbar_phys? Don't 
see why not to change,
I have no strong opinion for which format is better.

Re: PCI Express support for 2.4 kernel

[Geert Uytterhoeven]

On Wed, 17 Dec 2003, Vladimir Kondratiev wrote:
> Greg KH wrote:
> >Minor code comments below:
> >On Tue, Dec 16, 2003 at 12:20:39PM +0200, Vladimir Kondratiev wrote:
> >>+			printk(KERN_INFO "PCI-Express config at 0x%08x\n", rrbar_phys);
> >>
> >>
> >
> >"%p" to show the address might be nicer.
> >
> I print phys. address, it is u32. Do you mean (void*)rrbar_phys? Don't
> see why not to change,
> I have no strong opinion for which format is better.

Are you 100% sure the physical address value will be limited to 32-bit on
64-bit platforms, too?

If not (and IMHO even if yes, for clarity), you should use long or void *,
which is 64-bit on 64-bit platforms.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Gabriel Paubert wrote:

Gabriel,
I verified with PCI-E designers,
uncacheable memory relates to snoop/non-snoop, not to buffering in 
bridge. Bridge will still buffer writes.
The only way to be sure data has arrived, is to perform read.

Vladimir.

>>>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.
>>    
>>
>
>Here I'm somehwat lost. Writes to uncacheable RAM will be in program 
>order and never combined. The bridge itself should not post writes to 
>config space. So it's a matter of pushing the write to the processor
>bus, a PCI read looks very heavy for this. Isn't there a more
>lightweight solution ?
>
>	Regards,
>	Gabriel
>
>  
>

Re: PCI Express support for 2.4 kernel

[Martin Mares]

Hi!

> I did not found this feature in standard.

I did :-)  C99, section 6.7.8 "Initialization", constraint 10:

"... If an object that has static storage duration is not initialized
explicitly, then:

   - if it has pointer type, it is initialized to a null pointer;
   - if it has arithmetic type, it is initialized to (positive or unsigned) zero;

..."

> More, future versions of gcc will give at least warning, if not error, like
> "use of uninitialized variable".

Yes, such warning exists, but only for automatic variables.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
Quote of the day: '

Re: PCI Express support for 2.4 kernel

[Greg KH]

On Sun, Dec 14, 2003 at 10:00:49PM +0200, Vladimir Kondratiev wrote:
> Please, ignore previous submission with the same subject. Patch file 
> attached was wrong one. Now correct patch attached.
> 
> Hi,
> PCI-Express platforms will soon appear on the market. It is worth to
> support it.

Yes it is worth it, any chance to get access to hardware to test this
out on?

> Following is patch for 2.4.23 kernel. I tested it on my host, it works
> properly.
> I did it for i386 only, I have no other architecture to test.
> 
> It was patch on the same subject from* Seshadri, Harinarayanan*
> (/harinarayanan.seshadri@intel.com/
> <mailto:harinarayanan.seshadri@intel.com>)
> http://www.cs.helsinki.fi/linux/linux-kernel/2003-17/0247.html
> My version differ in several aspects: it is for 2.4 (vs. 2.6); it do not
> ioremap/unmap page for each transaction.
> 
> How about inclusion in 2.4.24?

No, we need to get this into 2.6 first.  Can you please forward port
this to 2.6, clean up the formatting and address the issues everyone
else has made so far and post it?

>  * command line argument "pci=exp" to force PCI Express, similar to "conf1" and "conf2"

We should be able to do this automatically, and not force this on the
boot command line, correct?

> +/**
> + * 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;

How about information on how to detect it as per chipset type?  We need
to do this automatically some how.

> +/**
> + * Initializes PCI Express method for config space access.
> + * 
> + * There is no standard method to recognize presence of PCI Express,

Are you sure?  I thought there was (don't have my spec in front of me
right now...)

thanks,

greg k-h

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Greg KH wrote:

>>Hi,
>>PCI-Express platforms will soon appear on the market. It is worth to
>>support it.
>>    
>>
>
>Yes it is worth it, any chance to get access to hardware to test this
>out on?
>  
>
If it were up to me, I will give away full specs and test platforms. But 
it is not...

>No, we need to get this into 2.6 first.  Can you please forward port
>this to 2.6, clean up the formatting and address the issues everyone
>else has made so far and post it?
>  
>
I will. It take some time, I did not installed 2.6 yet.

>> * command line argument "pci=exp" to force PCI Express, similar to "conf1" and "conf2"
>>    
>>
>
>We should be able to do this automatically, and not force this on the
>boot command line, correct?
>  
>
Yes. Default is autodetect. Command line is to suppress autodetection.

>How about information on how to detect it as per chipset type?  We need
>to do this automatically some how.
>+ * 
>+ * There is no standard method to recognize presence of PCI Express,
>  
>
>
>Are you sure?  I thought there was (don't have my spec in front of me
>right now...)
>  
>
I thought this way also. But I found that it is not. You may know 
several chipsets,
and do per-chipset stuff, but there is no generic procedure. At least 
authors of PCI-E
don't know (it is nice to have access to the authors ;-) ).

Vladimir.

Re: PCI Express support for 2.4 kernel

[Andi Kleen]

Vladimir Kondratiev <vladimir.kondratiev@intel.com> writes:
>
> As alternative between 1 page and 256M, I see also lazy allocation on
> per-bus basis: when bus is first accessed, ioremap 1Mb for it. On real
> system, it is no more then 3-4 buses. This way, we will end with
> several 1MB mappings. Finer granularity do not looks feasible, since
> bus scanning procedure tries to access all devices.

For bus scanning fixmaps are fine, but for the normal use of the
config space in the driver just doing ioremap once (e.g. at
pci_enable_device time) and caching it is preferable.  The number of
PCI-E devices in a given system should be bounded, so e.g. when you
have 100 devices you will only lose 400kB of vmalloc space this way
which is quite reasonable.

I don't think dynamic fixmaps at each access would be a good idea.

-Andi

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Andi,

My fist intention was exactly same as yours, but if all access were done 
through pci_dev...
Unfortunately, you can't store ioremap()'ed address for 4k within 
pci_dev and then simply use it.
In all places around, config accessed through (bus,dev,fn) indexes.

If you want to keep virt. addr. footprint small, the only alternative to 
fixmap is some kind of LRU cache for dozen of devices.
When choosing between fixmap and LRU cache, I would choose fixmaps. It 
is very simple and clear, low virt. addr. and comparable overhead.

Vladimir.

Andi Kleen wrote:

>Vladimir Kondratiev <vladimir.kondratiev@intel.com> writes:
>  
>
>>As alternative between 1 page and 256M, I see also lazy allocation on
>>per-bus basis: when bus is first accessed, ioremap 1Mb for it. On real
>>system, it is no more then 3-4 buses. This way, we will end with
>>several 1MB mappings. Finer granularity do not looks feasible, since
>>bus scanning procedure tries to access all devices.
>>    
>>
>
>For bus scanning fixmaps are fine, but for the normal use of the
>config space in the driver just doing ioremap once (e.g. at
>pci_enable_device time) and caching it is preferable.  The number of
>PCI-E devices in a given system should be bounded, so e.g. when you
>have 100 devices you will only lose 400kB of vmalloc space this way
>which is quite reasonable.
>
>I don't think dynamic fixmaps at each access would be a good idea.
>
>-Andi
>  
>

Re: PCI Express support for 2.4 kernel

[Andi Kleen]

On Tue, Dec 16, 2003 at 12:48:49AM +0200, Vladimir Kondratiev wrote:

> My fist intention was exactly same as yours, but if all access were done 
> through pci_dev...
> Unfortunately, you can't store ioremap()'ed address for 4k within 
> pci_dev and then simply use it.
> In all places around, config accessed through (bus,dev,fn) indexes.

Just use a small binary tree or hash table to look up the pci_dev
given (bus,dev,fn). That will be much faster than playing with
mappings and flushing TLBs. 
 
-Andi

Re: PCI Express support for 2.4 kernel

[Greg KH]

On Tue, Dec 16, 2003 at 12:48:49AM +0200, Vladimir Kondratiev wrote:
> 
> My fist intention was exactly same as yours, but if all access were done 
> through pci_dev...

Look at 2.6's version of pci_[read|write]_config_*()  :)

(yeah, it's just a wrapper around pci_bus_*() right now, but that's
easier to change if we have to...)

thanks,

greg k-h

RE: PCI Express support for 2.4 kernel

[Nakajima, Jun]

> > +/**
> > + * 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;

Sorry, this patch is not appropriate because it's hard coding this
chipset-specific address. The way of getting this address is being
defined by a public specification (I cannot tell which spec now). 

	Jun

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Greg KH
> Sent: Monday, December 15, 2003 2:07 AM
> To: Kondratiev, Vladimir
> Cc: linux-kernel@vger.kernel.org; Alan Cox; Marcelo Tosatti
> Subject: Re: PCI Express support for 2.4 kernel
> 
> On Sun, Dec 14, 2003 at 10:00:49PM +0200, Vladimir Kondratiev wrote:
> > Please, ignore previous submission with the same subject. Patch file
> > attached was wrong one. Now correct patch attached.
> >
> > Hi,
> > PCI-Express platforms will soon appear on the market. It is worth to
> > support it.
> 
> Yes it is worth it, any chance to get access to hardware to test this
> out on?
> 
> > Following is patch for 2.4.23 kernel. I tested it on my host, it
works
> > properly.
> > I did it for i386 only, I have no other architecture to test.
> >
> > It was patch on the same subject from* Seshadri, Harinarayanan*
> > (/harinarayanan.seshadri@intel.com/
> > <mailto:harinarayanan.seshadri@intel.com>)
> > http://www.cs.helsinki.fi/linux/linux-kernel/2003-17/0247.html
> > My version differ in several aspects: it is for 2.4 (vs. 2.6); it do
not
> > ioremap/unmap page for each transaction.
> >
> > How about inclusion in 2.4.24?
> 
> No, we need to get this into 2.6 first.  Can you please forward port
> this to 2.6, clean up the formatting and address the issues everyone
> else has made so far and post it?
> 
> >  * command line argument "pci=exp" to force PCI Express, similar to
> "conf1" and "conf2"
> 
> We should be able to do this automatically, and not force this on the
> boot command line, correct?
> 
> > +/**
> > + * 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;
> 
> How about information on how to detect it as per chipset type?  We
need
> to do this automatically some how.
> 
> > +/**
> > + * Initializes PCI Express method for config space access.
> > + *
> > + * There is no standard method to recognize presence of PCI
Express,
> 
> Are you sure?  I thought there was (don't have my spec in front of me
> right now...)
> 
> thanks,
> 
> greg k-h
> -
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

OK, I almost convinced it may be removed.
My point is, this initialization with 0 cost nothing. Readability and 
clearness of code do matter, on my opinion. I think when one states 
explicitly he expect variable to have 0 value, it is better then use 
implicit rules.

To illustrate zero cost, I did the following test:
[tmp]$ cat t.c; gcc -S t.c; cat t.s
static int a1=0;
static int a2;
/* EOF */

    .file    "t.c"
    .local    a1
    .comm    a1,4,4
    .local    a2
    .comm    a2,4,4
    .section    .note.GNU-stack,"",@progbits
    .ident    "GCC: (GNU) 3.3.1 20030811 (Red Hat Linux 3.3.1-1)"

As you can see, assembly code is identical, compiler did this trivial 
optimization for me.

Vladimir.

Mark Hahn wrote:

>>>>+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'll get your code rejected.  static variables are always, everywhere,
>initialized to zero (OK, probably not embedded environments).  this is 
>a code standard, not a matter of taste.
>
>  
>

Re: PCI Express support for 2.4 kernel

[Kevin P. Fleming]

Vladimir Kondratiev wrote:

> To illustrate zero cost, I did the following test:
> [tmp]$ cat t.c; gcc -S t.c; cat t.s
> static int a1=0;
> static int a2;
> /* EOF */
> 
>    .file    "t.c"
>    .local    a1
>    .comm    a1,4,4
>    .local    a2
>    .comm    a2,4,4
>    .section    .note.GNU-stack,"",@progbits
>    .ident    "GCC: (GNU) 3.3.1 20030811 (Red Hat Linux 3.3.1-1)"
> 
> As you can see, assembly code is identical, compiler did this trivial 
> optimization for me.

You've missed the point, though. Initializing a static variable to zero 
causes space to be consumed in the resulting object file (not 
instruction code to be generated). This is wasted space, because if you 
don't initialize to zero the variable will be allocated out of space 
that is _automatically_ zeroed for you. This reduces the size of the 
kernel image by not filling it with unnecessary zeroes.

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Kevin,
there is no black magic. Compilation goes .c -> .s -> .o; if assembly 
have nothing added, object have not as well.
To be sure, I did the following:

[tmp]$ gcc -c t.c; objdump -xs t.o

t.o:     file format elf32-i386
t.o
architecture: i386, flags 0x00000010:
HAS_SYMS
start address 0x00000000

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00000000  00000000  00000000  00000034  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .data         00000000  00000000  00000000  00000034  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  2 .bss          00000008  00000000  00000000  00000034  2**2
                  ALLOC
  3 .note.GNU-stack 00000000  00000000  00000000  00000034  2**0
                  CONTENTS, READONLY
  4 .comment      00000033  00000000  00000000  00000034  2**0
                  CONTENTS, READONLY
SYMBOL TABLE:
00000000 l    df *ABS*    00000000 t.c
00000000 l    d  .text    00000000
00000000 l    d  .data    00000000
00000000 l    d  .bss    00000000
00000000 l     O .bss    00000004 a1
00000004 l     O .bss    00000004 a2
00000000 l    d  .note.GNU-stack    00000000
00000000 l    d  .comment    00000000


Contents of section .text:
Contents of section .data:
Contents of section .note.GNU-stack:
Contents of section .comment:
 0000 00474343 3a202847 4e552920 332e332e  .GCC: (GNU) 3.3.
 0010 31203230 30333038 31312028 52656420  1 20030811 (Red
 0020 48617420 4c696e75 7820332e 332e312d  Hat Linux 3.3.1-
 0030 312900                               1).            

Kevin P. Fleming wrote:

> Vladimir Kondratiev wrote:
>
>> To illustrate zero cost, I did the following test:
>> [tmp]$ cat t.c; gcc -S t.c; cat t.s
>> static int a1=0;
>> static int a2;
>> /* EOF */
>>
>>    .file    "t.c"
>>    .local    a1
>>    .comm    a1,4,4
>>    .local    a2
>>    .comm    a2,4,4
>>    .section    .note.GNU-stack,"",@progbits
>>    .ident    "GCC: (GNU) 3.3.1 20030811 (Red Hat Linux 3.3.1-1)"
>>
>> As you can see, assembly code is identical, compiler did this trivial 
>> optimization for me.
>
>
> You've missed the point, though. Initializing a static variable to 
> zero causes space to be consumed in the resulting object file (not 
> instruction code to be generated). This is wasted space, because if 
> you don't initialize to zero the variable will be allocated out of 
> space that is _automatically_ zeroed for you. This reduces the size of 
> the kernel image by not filling it with unnecessary zeroes.
>

Re: PCI Express support for 2.4 kernel

[Richard B. Johnson]

On Mon, 15 Dec 2003, Vladimir Kondratiev wrote:

> Kevin,
> there is no black magic. Compilation goes .c -> .s -> .o; if assembly
> have nothing added, object have not as well.
> To be sure, I did the following:
>
> [tmp]$ gcc -c t.c; objdump -xs t.o
>
> t.o:     file format elf32-i386
> t.o
> architecture: i386, flags 0x00000010:
> HAS_SYMS
> start address 0x00000000
>
> Sections:
> Idx Name          Size      VMA       LMA       File off  Algn
>   0 .text         00000000  00000000  00000000  00000034  2**2
>                   CONTENTS, ALLOC, LOAD, READONLY, CODE
>   1 .data         00000000  00000000  00000000  00000034  2**2
>                   CONTENTS, ALLOC, LOAD, DATA
>   2 .bss          00000008  00000000  00000000  00000034  2**2
>                   ALLOC
>   3 .note.GNU-stack 00000000  00000000  00000000  00000034  2**0
>                   CONTENTS, READONLY
>   4 .comment      00000033  00000000  00000000  00000034  2**0
>                   CONTENTS, READONLY
> SYMBOL TABLE:
> 00000000 l    df *ABS*    00000000 t.c
> 00000000 l    d  .text    00000000
> 00000000 l    d  .data    00000000
> 00000000 l    d  .bss    00000000
> 00000000 l     O .bss    00000004 a1
> 00000004 l     O .bss    00000004 a2
> 00000000 l    d  .note.GNU-stack    00000000
> 00000000 l    d  .comment    00000000
>
>
> Contents of section .text:
> Contents of section .data:
> Contents of section .note.GNU-stack:
> Contents of section .comment:
>  0000 00474343 3a202847 4e552920 332e332e  .GCC: (GNU) 3.3.
>  0010 31203230 30333038 31312028 52656420  1 20030811 (Red
>  0020 48617420 4c696e75 7820332e 332e312d  Hat Linux 3.3.1-
>  0030 312900                               1).
>
> Kevin P. Fleming wrote:
>
> > Vladimir Kondratiev wrote:
> >
> >> To illustrate zero cost, I did the following test:
> >> [tmp]$ cat t.c; gcc -S t.c; cat t.s
> >> static int a1=0;
> >> static int a2;
> >> /* EOF */
> >>
> >>    .file    "t.c"
> >>    .local    a1
> >>    .comm    a1,4,4
> >>    .local    a2
> >>    .comm    a2,4,4
> >>    .section    .note.GNU-stack,"",@progbits
> >>    .ident    "GCC: (GNU) 3.3.1 20030811 (Red Hat Linux 3.3.1-1)"
> >>
> >> As you can see, assembly code is identical, compiler did this trivial
> >> optimization for me.
> >
> >
> > You've missed the point, though. Initializing a static variable to
> > zero causes space to be consumed in the resulting object file (not
> > instruction code to be generated). This is wasted space, because if
> > you don't initialize to zero the variable will be allocated out of
> > space that is _automatically_ zeroed for you. This reduces the size of
> > the kernel image by not filling it with unnecessary zeroes.
> >

Easy way to remember is that if you have either a static or a global
variable that is initialized, it will be in the .data segment and,
therefore take up space in the executable. If it is not initialized,
it will be in the .bss segment, automatically zeroed by the loader.
In this case, the executable contains length information, not the data.
Local variables are never initialized unless there's an '=' in the
code.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.22 on an i686 machine (797.90 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: PCI Express support for 2.4 kernel

[Tomas Szepe]

On Dec-15 2003, Mon, 11:55 -0500
Richard B. Johnson <root@chaos.analogic.com> wrote:

> Easy way to remember is that if you have either a static or a global
> variable that is initialized, it will be in the .data segment and,
> therefore take up space in the executable. If it is not initialized,
> it will be in the .bss segment, automatically zeroed by the loader.
> In this case, the executable contains length information, not the data.
> Local variables are never initialized unless there's an '=' in the
> code.

I think you guys are all missing Vladimir's point, which is that gcc
is able to detect that an explicit initialization of a static variable
happens to be to the value of ZERO, and place the variable in .bss
_in spite of the initialization_.

-- 
Tomas Szepe <szepe@pinerecords.com>

Re: PCI Express support for 2.4 kernel

[Richard B. Johnson]

On Mon, 15 Dec 2003, Tomas Szepe wrote:

> On Dec-15 2003, Mon, 11:55 -0500
> Richard B. Johnson <root@chaos.analogic.com> wrote:
>
> > Easy way to remember is that if you have either a static or a global
> > variable that is initialized, it will be in the .data segment and,
> > therefore take up space in the executable. If it is not initialized,
> > it will be in the .bss segment, automatically zeroed by the loader.
> > In this case, the executable contains length information, not the data.
> > Local variables are never initialized unless there's an '=' in the
> > code.
>
> I think you guys are all missing Vladimir's point, which is that gcc
> is able to detect that an explicit initialization of a static variable
> happens to be to the value of ZERO, and place the variable in .bss
> _in spite of the initialization_.
>
> --
> Tomas Szepe <szepe@pinerecords.com>
>

Well it doesn't, and if it did, it would be broken. GCC needs to
operate according to some well-defined rules because its output
must interoperate with the object-files created by other versions
including stuff written in other languages.

Therefore you make data exist in the .data segment by initializing
it. If GCC decides to put this initialized data in the .bss segment,
then it is broken. FYI, it doesn't.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.22 on an i686 machine (797.90 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: PCI Express support for 2.4 kernel

[Tomas Szepe]

On Dec-15 2003, Mon, 13:03 -0500
Richard B. Johnson <root@chaos.analogic.com> wrote:

> Therefore you make data exist in the .data segment by initializing
> it. If GCC decides to put this initialized data in the .bss segment,
> then it is broken. FYI, it doesn't.

Richard, stop denying reality, go check out what gcc 3.3.2 does.

-- 
Tomas Szepe <szepe@pinerecords.com>

Re: PCI Express support for 2.4 kernel

[Richard B. Johnson]

On Mon, 15 Dec 2003, Tomas Szepe wrote:

> On Dec-15 2003, Mon, 13:03 -0500
> Richard B. Johnson <root@chaos.analogic.com> wrote:
>
> > Therefore you make data exist in the .data segment by initializing
> > it. If GCC decides to put this initialized data in the .bss segment,
> > then it is broken. FYI, it doesn't.
>
> Richard, stop denying reality, go check out what gcc 3.3.2 does.
>
> --
> Tomas Szepe <szepe@pinerecords.com>
>

Well, it seems I am NOT denying reality. Others have just
parroted the contents of an ELF __Header__. I will show you the
actual allocation data.

Script started on Mon Dec 15 13:25:21 2003

quark:/home/johnson[1] cat xxx.c

int foo;   // Not initialized
int bar=0; // Initialized

quark:/home/johnson[2] gcc --version
gcc (GCC) 3.2 20020903 (Red Hat Linux 8.0 3.2-7)
Copyright (C) 2002 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

quark:/home/johnson[3] gcc -c -o xxx.o xxx.c
quark:/home/johnson[4] objdump --disassemble-all xxx.o

xxx.o:     file format elf32-i386

Disassembly of section .text:
Disassembly of section .data:

00000000 <bar>:
   0:	00 00                	add    %al,(%eax)
	...
quark:/home/johnson[5] exit

Script done on Mon Dec 15 13:26:17 2003


This clearly shows that "bar" is the only variable in that object
file. The variable, foo, is written in the header (not shown)
so that the loader knows its size and its relocation symbol.


Cheers,
Dick Johnson
Penguin : Linux version 2.4.22 on an i686 machine (797.90 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: PCI Express support for 2.4 kernel

[Keith Owens]

On Mon, 15 Dec 2003 13:35:40 -0500 (EST), 
"Richard B. Johnson" <root@chaos.analogic.com> wrote:
>Well, it seems I am NOT denying reality. Others have just
>parroted the contents of an ELF __Header__. I will show you the
>actual allocation data.

Sigh.  You are demonstrating gcc 3.2, others are demonstrating gcc 3.3.
The latter will optimize an assigment to 0 and automatically move the
variable from data to bss.

Re: PCI Express support for 2.4 kernel

[Tomas Szepe]

On Dec-15 2003, Mon, 13:35 -0500
Richard B. Johnson <root@chaos.analogic.com> wrote:

_Please_ pay more attention to what I've had to write.

> > Richard, stop denying reality, go check out what gcc 3.3.2 does.

                                                         ^^^^^

> Well, it seems I am NOT denying reality. Others have just
> parroted the contents of an ELF __Header__. I will show you the
> actual allocation data.
> 
> quark:/home/johnson[1] cat xxx.c
> 
> int foo;   // Not initialized
> int bar=0; // Initialized
>
> quark:/home/johnson[2] gcc --version
> gcc (GCC) 3.2 20020903 (Red Hat Linux 8.0 3.2-7)

            ^^^

> Disassembly of section .text:
> Disassembly of section .data:
> 
> 00000000 <bar>:
>    0:	00 00                	add    %al,(%eax)
> 	...
> 
> This clearly shows that "bar" is the only variable in that object
> file. The variable, foo, is written in the header (not shown)
> so that the loader knows its size and its relocation symbol.

$ # (gcc version is 3.3.2)
$ cat x.c
int foo;
int bar = 0;
$ gcc -c -o x.o x.c
$ objdump --disassemble-all x.o

x.o:     file format elf32-i386

Disassembly of section .bss:

00000000 <bar>:
   0:   00 00                   add    %al,(%eax)
        ...

...

-- 
Tomas Szepe <szepe@pinerecords.com>

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Richard B. Johnson wrote:
<discussion regarding initializers for static vars>

Let's stop this discussion, it leads to nowhere. Probable, yes, 
initializer do add bytes to the data segment. But it does not make 
difference for memory image after loading, do it?

Does this difference in executable size worth potential risk of error?

Anyway, common style in kernel seems to be to do initialize static vars, 
even to 0. There are plenty of examples, including the same file, (for 
2.4.23)

arch/i386/kernel/pci-pc.c:32
static int pci_using_acpi_prt = 0;

or

arch/i386/kernel/setup.c:1241
static int tsc_disable __initdata = 0;

Finally, let's stop this thread. Let it be up to person who will be (if 
it will happen) checking this code into kernel, to decide on coding 
style. I, personally, value code clarity more then 4 bytes in executable 
size. But I will not object if more experienced kernel maintainers have 
another priority.

Vladimir.

Re: PCI Express support for 2.4 kernel

[Randy.Dunlap]

On Mon, 15 Dec 2003 19:24:02 +0200 Vladimir Kondratiev <vladimir.kondratiev@intel.com> wrote:

| Richard B. Johnson wrote:
| <discussion regarding initializers for static vars>
| 
| Let's stop this discussion, it leads to nowhere. Probable, yes, 
| initializer do add bytes to the data segment. But it does not make 
| difference for memory image after loading, do it?
| 
| Does this difference in executable size worth potential risk of error?
| 
| Anyway, common style in kernel seems to be to do initialize static vars, 
| even to 0. There are plenty of examples, including the same file, (for 
| 2.4.23)
| 
| arch/i386/kernel/pci-pc.c:32
| static int pci_using_acpi_prt = 0;
| 
| or
| 
| arch/i386/kernel/setup.c:1241
| static int tsc_disable __initdata = 0;
| 
| Finally, let's stop this thread. Let it be up to person who will be (if 
| it will happen) checking this code into kernel, to decide on coding 
| style. I, personally, value code clarity more then 4 bytes in executable 
| size. But I will not object if more experienced kernel maintainers have 
| another priority.

You seem to be missing that you shouldn't be looking in 2.4.x but in
2.6.x instead.  2.6.x has had hundreds++ of 0 initializers removed from it.

--
~Randy
MOTD:  Always include version info.

Re: PCI Express support for 2.4 kernel

[Greg KH]

On Mon, Dec 15, 2003 at 07:24:02PM +0200, Vladimir Kondratiev wrote:
> 
> Finally, let's stop this thread. Let it be up to person who will be (if 
> it will happen) checking this code into kernel, to decide on coding 
> style.

For 2.6, that person would be me. 

> I, personally, value code clarity more then 4 bytes in executable
> size. But I will not object if more experienced kernel maintainers
> have another priority.

Please remove the initialized variable to conserve size (if you really
want to, a comment like /* initialized to NULL */ is ok to have to
remind you that this is how it works.)

greg k-h

Re: PCI Express support for 2.4 kernel

[Richard B. Johnson]

On Mon, 15 Dec 2003, Vladimir Kondratiev wrote:

> Richard B. Johnson wrote:
> <discussion regarding initializers for static vars>
>
> Let's stop this discussion, it leads to nowhere. Probable, yes,
> initializer do add bytes to the data segment. But it does not make
> difference for memory image after loading, do it?
>

It definitely does. It makes the difference between having stuff
work or not in many embedded systems.

> Does this difference in executable size worth potential risk of error?
>
> Anyway, common style in kernel seems to be to do initialize static vars,
> even to 0. There are plenty of examples, including the same file, (for
> 2.4.23)
>
> arch/i386/kernel/pci-pc.c:32
> static int pci_using_acpi_prt = 0;
>

This is a BUG. This wastes space that may end up being embedded
in NVRAM, then copied to RAM. If it was not initialized, it would
never exist in NVRAM at all.

> or
>
> arch/i386/kernel/setup.c:1241
> static int tsc_disable __initdata = 0;
>

This is not a BUG because "__initdata" is defined to be in a segment
that is initialized and discarded after use.

> Finally, let's stop this thread. Let it be up to person who will be (if
> it will happen) checking this code into kernel, to decide on coding
> style. I, personally, value code clarity more then 4 bytes in executable
> size. But I will not object if more experienced kernel maintainers have
> another priority.
>
> Vladimir.
>

It's not style. It's misuse of variables and well-defined
conventions. Often data is not some simple variable, but an
aggregate type such as a structure or an array. FYI "style"
relates to naming conventions and where you put curley braces.
Even that is supposed to be controlled to some liberal extent.

static long xtable[0x100] = {0,};

MUST be in the .data segment, while...

static long xtable[0x100];

MUST be in the .bss segment, while...

static const long xtable[0x100]={0,1,2,3,4,5,6,7,8,9,.....};

MUST be in the .const segment. It's that simple. If you
violate that, your code will only work by fiat, i.e., it
was defined to work, not designed to work.


Cheers,
Dick Johnson
Penguin : Linux version 2.4.22 on an i686 machine (797.90 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: PCI Express support for 2.4 kernel

[Keith Owens]

On Mon, 15 Dec 2003 17:52:38 +0200, 
Vladimir Kondratiev <vladimir.kondratiev@intel.com> wrote:
>OK, I almost convinced it may be removed.
>My point is, this initialization with 0 cost nothing. Readability and 
>clearness of code do matter, on my opinion. I think when one states 
>explicitly he expect variable to have 0 value, it is better then use 
>implicit rules.
>
>To illustrate zero cost, I did the following test:
>[tmp]$ cat t.c; gcc -S t.c; cat t.s
>static int a1=0;
>static int a2;
>/* EOF */
>
>    .file    "t.c"
>    .local    a1
>    .comm    a1,4,4
>    .local    a2
>    .comm    a2,4,4
>    .section    .note.GNU-stack,"",@progbits
>    .ident    "GCC: (GNU) 3.3.1 20030811 (Red Hat Linux 3.3.1-1)"
>
>As you can see, assembly code is identical, compiler did this trivial 
>optimization for me.

Try it with an older version of gcc, which most people are still using
to build the kernel.  With 3.2.2, you get

# gcc -S t.c

         .file   "t.c"
        .data
        .align 4
        .type   a1,@object
        .size   a1,4
a1:
        .long   0
        .local  a2
        .comm   a2,4,4
        .ident  "GCC: (GNU) 3.2.2 20030222 (Red Hat Linux 3.2.2-5)"

# gcc t.c -c -o t.o
# objdump -h t.o

t.o:     file format elf32-i386

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00000000  00000000  00000000  00000034  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .data         00000004  00000000  00000000  00000034  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  2 .bss          00000004  00000000  00000000  00000038  2**2
                  ALLOC
  3 .comment      00000033  00000000  00000000  00000038  2**0
                  CONTENTS, READONLY

a1 is in the data section, taking up space on disk.  a2 is in bss which
is allocated and zeroed at run time, no disk space is required.

Re: PCI Express support for 2.4 kernel

[Tomas Szepe]

On Dec-16 2003, Tue, 04:09 +1100
Keith Owens <kaos@ocs.com.au> wrote:

> a1:
>         .long   0
>         .local  a2  
>         .comm   a2,4,4
>         .ident  "GCC: (GNU) 3.2.2 20030222 (Red Hat Linux 3.2.2-5)"

...

> Try it with an older version of gcc, which most people are
> still using to build the kernel.  With 3.2.2, you get

And w/ gcc-3.3.2, you get what you'd actually expect :)

$ gcc -v
Reading specs from /usr/lib/gcc-lib/i386-slackware-linux/3.3.2/specs
Configured with: ../configure --enable-languages=c,c++ --prefix=/usr --enable-shared --enable-threads=posix --enable-__cxa_atexit --disable-checking --with-gnu-ld --verbose --disable-nls --target=i386-slackware-linux --host=i386-slackware-linux --build=i386-slackware-linux
Thread model: posix
gcc version 3.3.2
$ cat x.c
static int a1 = 0;
static int a2 = 1;
static int a3;
$ gcc -S x.c
$ cat x.s
        .file   "x.c"
        .local  a1
        .comm   a1,4,4
        .data
        .align 4
        .type   a2, @object
        .size   a2, 4
a2:
        .long   1
        .local  a3
        .comm   a3,4,4
        .ident  "GCC: (GNU) 3.3.2"

-- 
Tomas Szepe <szepe@pinerecords.com>

Re: PCI Express support for 2.4 kernel

[Keith Owens]

On Mon, 15 Dec 2003 18:38:16 +0100, 
Tomas Szepe <szepe@pinerecords.com> wrote:
>And w/ gcc-3.3.2, you get what you'd actually expect :)
>
>gcc version 3.3.2
>$ cat x.c
>static int a1 = 0;
>static int a2 = 1;
>static int a3;
>$ gcc -S x.c
>$ cat x.s
>        .file   "x.c"
>        .local  a1
>        .comm   a1,4,4
>        .data
>        .align 4
>        .type   a2, @object
>        .size   a2, 4
>a2:
>        .long   1
>        .local  a3
>        .comm   a3,4,4
>        .ident  "GCC: (GNU) 3.3.2"

Does gcc 3.3.2 handle sections correctly when it optimizes zero
assignments to use bss?  With just this line in x.c

static int __attribute__ ((__section__ (".init.data"))) a1 = 0;

what does objdump -h report?  If bss is 4 bytes then gcc 3.3.2 is
breaking the kernel's use of init data.  If bss is 0 and .init.data is
4 then we are OK.

Re: PCI Express support for 2.4 kernel

[Tomas Szepe]

On Dec-16 2003, Tue, 05:16 +1100
Keith Owens <kaos@ocs.com.au> wrote:

> Does gcc 3.3.2 handle sections correctly when it optimizes zero
> assignments to use bss?  With just this line in x.c
> 
> static int __attribute__ ((__section__ (".init.data"))) a1 = 0;
> 
> what does objdump -h report?  If bss is 4 bytes then gcc 3.3.2 is
> breaking the kernel's use of init data.  If bss is 0 and .init.data is
> 4 then we are OK.

$ gcc --version| grep GCC
gcc (GCC) 3.3.2
$ cat x.c
static int __attribute__ ((__section__ (".init.data"))) a1 = 0;
$ gcc -c -o x.o x.c
$ objdump -h x.o

x.o:     file format elf32-i386

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00000000  00000000  00000000  00000034  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .data         00000000  00000000  00000000  00000034  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  2 .bss          00000000  00000000  00000000  00000034  2**2
                  ALLOC
  3 .init.data    00000004  00000000  00000000  00000034  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  4 .comment      00000012  00000000  00000000  00000038  2**0
                  CONTENTS, READONLY

Good.

-- 
Tomas Szepe <szepe@pinerecords.com>

Re: PCI Express support for 2.4 kernel

[Andi Kleen]

Vladimir Kondratiev <vladimir.kondratiev@intel.com> writes:
>
> 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.

I don't think that's a good assumption. There will be surely be some 
machine that has an mapping there. You will need to find some fool
proof way to detect the presence of PCI-Express. Otherwise it will likely
fail the number one criterium for merging ("it shall not break 
anything else")

And in general new development should be done in 2.6 first...

-Andi

Re: PCI Express support for 2.4 kernel

[Andi Kleen]

Vladimir Kondratiev <vladimir.kondratiev@intel.com> writes:


>  #define PCI_PROBE_CONF1		0x0002
>  #define PCI_PROBE_CONF2		0x0004
> +
> +#ifdef CONFIG_PCI_EXPRESS
> +#define PCI_PROBE_EXP		0x0008
> +#endif

Drop the #ifdef here.

> +#ifdef CONFIG_PCI_EXPRESS
> +#include <asm/fixmap.h>
> +#endif

Same here.


> +static inline void pci_exp_set_dev_base(int bus, int dev, int fn)
> +{
> +	u32 dev_base = (bus << 20) | (dev << 15) | (fn << 12);
> +	if (dev_base != pcie_last_accessed_device) {
> +		pcie_last_accessed_device = dev_base;
> +		set_fixmap_nocache(FIX_PCIE_CONFIG, rrbar_phys | dev_base);
> +	}

When you use one fix map per CPU you can avoid the lock. All that would
be needed would be a local_irq_disable()/enable() then. While normally
config space accesses should not be performance critical sometimes
they are done frequently to work around hardware bugs.

I still think it would be better to cache mappings and use a hash table
though ...

-Andi

Re: PCI Express support for 2.4 kernel

[Andi Kleen]

Vladimir Kondratiev <vladimir.kondratiev@intel.com> writes:
>>
> Yes, separation into generic and platform specific part seems nice,
> but besides memory mapping and locking, all you have is
> very simple arithmetic. Does it worth the work for separation?
>
> For 64-bit world, agree, it could and should be done uniform.

Could you put the relevant code into an asm/ file for 2.6 then please?

Background: x86-64 shares the PCI code with i386 which just works
for both 32bit and 64bit currently. If it is all changeable in the asm/ 
file then I won't need to change it later again. Of course the fixmap
would work for x86-64 too, but just mapping the 256MB is much nicer
and preferable.

-Andi

Re: PCI Express support for 2.4 kernel

[Andi Kleen]

Alan Cox <alan@redhat.com> writes:
>
> And X11 will want to access it via /proc interfaces. And someone will eventually
> go and design a different way to access PCI-EX for their hardware 8)

It would be nice if it did. Just currently it uses racy port accesses
from user space :-(

-Andi

Re: PCI Express support for 2.4 kernel

[Alan Cox]

On Thu, Dec 18, 2003 at 12:22:02AM +0100, Andi Kleen wrote:
> Alan Cox <alan@redhat.com> writes:
> >
> > And X11 will want to access it via /proc interfaces. And someone will eventually
> > go and design a different way to access PCI-EX for their hardware 8)
> 
> It would be nice if it did. Just currently it uses racy port accesses
> from user space :-(

Depends what patches you applied and how you built it 8) However XFree meets
hotplug is so unfunny its not good to consider before dinner, nor XFree meets
irqs or XFree meets SAK and high security models.

Linus had a nice comment that the basic DRI/fb modules should be dealing with
the PCI layer and perhaps exposing the DMA engine for newer cards (to trusted
parties). For older cards there are all sorts of reasons you don't want to do
this on the performance side but even then a driver which refused to free up
the PCI map space until X noticed the file handle was returning errors and
selected EOF would make a lot of the stuff sort out.

Maybe in part the GGI folks were right all those years ago. We had two extreme
views and the middle may be closer

Alan

Re: PCI Express support for 2.4 kernel

[Andi Kleen]

Vladimir Kondratiev <vladimir.kondratiev@intel.com> writes:
>>
> I thought this way also. But I found that it is not. You may know
> several chipsets,
> and do per-chipset stuff, but there is no generic procedure. At least
> authors of PCI-E
> don't know (it is nice to have access to the authors ;-) ).

This sounds like a serious design flaw in PCI-Express. Can you 
ask them to address this before it is too late? 

-Andi

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

You know, I am also thinking in this direction. I already started 
conversation to implement some standard method.

Vladimir.

Andi Kleen wrote:

>Vladimir Kondratiev <vladimir.kondratiev@intel.com> writes:
>  
>
>>I thought this way also. But I found that it is not. You may know
>>several chipsets,
>>and do per-chipset stuff, but there is no generic procedure. At least
>>authors of PCI-E
>>don't know (it is nice to have access to the authors ;-) ).
>>    
>>
>
>This sounds like a serious design flaw in PCI-Express. Can you 
>ask them to address this before it is too late? 
>
>-Andi
>  
>

Re: PCI Express support for 2.4 kernel

[Pete Zaitcev]

> Date: Sun, 14 Dec 2003 19:28:37 +0200
> From: Vladimir Kondratiev <vladimir.kondratiev@intel.com>

> +++ linux-2.4.23-pciexp/arch/i386/kernel/pci-i386.h	2003-12-14 11:08:17.000000000 +0200

Generally I'm pretty tolerant of bizzare spacing, but in this
case tabs were mixed into the patch, which causes funny effects.

> +union pci_exp_data {
> +    u32 l;
> +    u16 w[2];
> +    u8  b[4];
> +} __attribute__((packed));

Bogus __attribute__((packed)), but it's only the begining.
Whole thing is used where shifts would have worked.

> +static void pci_express_fini(void)
> +{
> +    if (rrbar_virt) {	<====== Not trusting own code
> +        iounmap(rrbar_virt);
> +    }

All in all, raw. Also, a healthy dose of Itanium is prescribed.

-- Pete

PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

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

Hi,
PCI-Express platforms will soon appear on the market. It is worth to 
support it.

Following is patch for 2.4.23 kernel. I tested it on my host, it works 
properly.
I did it for i386 only, I have no other architecture to test.

It was patch on the same subject from* Seshadri, Harinarayanan* 
(/harinarayanan.seshadri@intel.com/ 
<mailto:harinarayanan.seshadri@intel.com>) 
http://www.cs.helsinki.fi/linux/linux-kernel/2003-17/0247.html
My version differ in several aspects: it is for 2.4 (vs. 2.6); it do not 
ioremap/unmap page for each transaction.

How about inclusion in 2.4.24?

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

Vladimir.

[-- Attachment #2: pciexp.patch --]
[-- Type: text/plain, Size: 8056 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 -bBdur 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 -bBdur 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-14 18:30:52.000000000 +0200
@@ -20,7 +20,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 +427,169 @@
 	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 change it.
+ */
+extern int PCI_CFG_SPACE_SIZE;
+
+union pci_exp_data {
+    u32 l;
+    u16 w[2];
+    u8  b[4];
+} __attribute__((packed));
+
+/**
+ * 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 */
+    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)
+{
+    union pci_exp_data d;
+    void* addr=rrbar_virt+(bus << 20)+(dev << 15)+(fn << 12)+(reg &~ 3);
+    d.l=readl(addr);
+    switch (len) {
+    case 1:
+        *value=d.b[reg & 3];
+        break;
+    case 2:
+        *value=d.w[(reg & 2)>>1];
+        break;
+    case 4:
+        *value=d.l;
+        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 &~ 3);
+	switch (len) {
+    case 1:
+    case 2:
+        {
+            unsigned long flags;
+            union pci_exp_data d;
+            spin_lock_irqsave(&pci_config_lock, flags);
+            switch (len) {
+            case 1:
+                d.l=readl(addr);
+                d.b[reg & 3]=value;
+                break;
+            case 2:
+                d.l=readl(addr);
+                d.w[(reg & 2)>>1]=value;
+                break;
+            }
+            writel(d.l,addr);
+            spin_unlock_irqrestore(&pci_config_lock, flags);
+        }
+        break;
+    case 4:
+        writel(value,addr);
+        break;
+    }
+	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 +628,21 @@
 
 	__save_flags(flags); __cli();
 
+    /**
+     * Check if PCI-express access work
+     */
+    if (pci_express_init()) {
+        if (pci_sanity_check(&pci_express_conf)) {
+            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 +1576,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 +1669,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 -bBdur 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-14 14:18:58.000000000 +0200
@@ -16,7 +16,7 @@
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
 
-#define PCI_CFG_SPACE_SIZE 256
+int PCI_CFG_SPACE_SIZE=256;
 
 static loff_t
 proc_bus_pci_lseek(struct file *file, loff_t off, int whence)

Re: PCI Express support for 2.4 kernel

[Christoph Hellwig]

On Sun, Dec 14, 2003 at 07:28:37PM +0200, Vladimir Kondratiev wrote:
> Hi,
> PCI-Express platforms will soon appear on the market. It is worth to 
> support it.
> 
> Following is patch for 2.4.23 kernel. I tested it on my host, it works 
> properly.
> I did it for i386 only, I have no other architecture to test.

Patch looks okay (except for totally broken indentation, that needs fixing)

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.

Re: PCI Express support for 2.4 kernel

[Linus Torvalds]

This really isn't appropriate. The

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

thing _really_ does not work on x86, since 256M of IO mapping is _way_ way
too much.

You _really_ need to allocate a FIXMAP entry (just one), and then use

	set_fixmap_nocache(FIX_PCIEXPRESS, phys);

to set it up for each device.

That's actually going to be a lot simpler than what you do now.

		Linus

Re: PCI Express support for 2.4 kernel

[Jeff Garzik]

Linus Torvalds wrote:
> You _really_ need to allocate a FIXMAP entry (just one), and then use
> 
> 	set_fixmap_nocache(FIX_PCIEXPRESS, phys);


neat.  dumb question though...  how portable is set_fixmap_nocache()?

I only see it on four architectures, and I'm sure PCI Express will 
appear on more than that eventually.

	Jeff

Re: PCI Express support for 2.4 kernel

[Linus Torvalds]

On Mon, 15 Dec 2003, Jeff Garzik wrote:
>
> neat.  dumb question though...  how portable is set_fixmap_nocache()?

Not very. Although it should generally be trivial to port if you need it.

> I only see it on four architectures, and I'm sure PCI Express will
> appear on more than that eventually.

On 64-bit architectures you're not likely to ever need to worry about it,
and then you can just map the whole thing directly (and use some special
large-page mapping for it, at that).

		Linus

Re: PCI Express support for 2.4 kernel

[Jeff Garzik]

Linus Torvalds wrote:
> On Mon, 15 Dec 2003, Jeff Garzik wrote:
>>neat.  dumb question though...  how portable is set_fixmap_nocache()?

> Not very. Although it should generally be trivial to port if you need it.
[...]
> On 64-bit architectures you're not likely to ever need to worry about it,
> and then you can just map the whole thing directly (and use some special
> large-page mapping for it, at that).

Yeah, good point.  IA64 with its 1001 different types of mappings and 
zones can go crazy, for example.

Sounds like PCI express needs a per-architecture policy for mapping the 
256MB region.  Some [64b] arches can direct-map the entire area.  IA32 
and others can go another route.

	Jeff

Re: PCI Express support for 2.4 kernel

[Vladimir Kondratiev]

Actually, besides discussion on how to save 4 bytes in executable size, 
I see 2 real problems with my PCI-E patch.

First problem is those addressed by Linus (thanks a lot!). I looked for 
proper way to do mapping, but it was either wasting lots of virtual 
space, or overhead for ioremap/unmap per transaction. I though 1-st 
variant is preferable and used it.

Linus, FIXMAP helps, it is lighter then ioremap, but it still requires 
page table walk. In addition, since several operations should be done 
atomically, lock/unlock required as well. Can it be faster method, 
without page table walk for each transaction? To what extent should one 
concern about performance here?

As alternative between 1 page and 256M, I see also lazy allocation on 
per-bus basis: when bus is first accessed, ioremap 1Mb for it. On real 
system, it is no more then 3-4 buses. This way, we will end with several 
1MB mappings. Finer granularity do not looks feasible, since bus 
scanning procedure tries to access all devices.

I am going to re-do PCI-E stuff with FIXMAP. To save extra page table 
walks, I'll keep last accessed page to not set_fixmap if we access the 
same device several times sequentially.

Second problem is generic way (lack of it) to recognize PCI-E presence 
and RRBAR physical address. Default is 0xe0000000, but theoretically it 
may be set to any 256M aligned. I used this default address and 
sanity_check for presence recognition. It is really not nice place in 
this patch. And worse, problem seems to be in PCI-E definition. I am 
working to address this problem before it is too late. What I want is 
some standard PCI capability that will be required in 00:00.0 device, 
its presence will indicate it is PCI-E, and its content will include 
RRBAR (and optionally any other appropriate stuff). Any alternative ideas?

Vladimir.

Linus Torvalds wrote:

>This really isn't appropriate. The
>
>	With PCI-E, config space accessed through memory. Each device gets
>	its own 4k memory mapped config, total 256M for all devices.
>
>thing _really_ does not work on x86, since 256M of IO mapping is _way_ way
>too much.
>
>You _really_ need to allocate a FIXMAP entry (just one), and then use
>
>	set_fixmap_nocache(FIX_PCIEXPRESS, phys);
>
>to set it up for each device.
>
>That's actually going to be a lot simpler than what you do now.
>
>		Linus
>
>  
>

Re: PCI Express support for 2.4 kernel

[Jeff Garzik]

Vladimir Kondratiev wrote:
> Linus, FIXMAP helps, it is lighter then ioremap, but it still requires 
> page table walk. In addition, since several operations should be done 
> atomically, lock/unlock required as well. Can it be faster method, 
> without page table walk for each transaction? To what extent should one 
> concern about performance here?
> 
> As alternative between 1 page and 256M, I see also lazy allocation on 
> per-bus basis: when bus is first accessed, ioremap 1Mb for it. On real 
> system, it is no more then 3-4 buses. This way, we will end with several 
> 1MB mappings. Finer granularity do not looks feasible, since bus 
> scanning procedure tries to access all devices.


Well, two things to consider:
* probing is not a performance-intensive operation
* even with a system loaded down with many PCI Express devices, I doubt 
you will need more than 1-2MB mapped total, during runtime

So, one alternative could be to keep a cache of ioremap() regions -- 
smaller than 1MB in my opinion -- and update that as needed.

Anybody doing large numbers of PCI config register reads/writes in a hot 
path should be shot (or the h/w designers, depending on situation), and 
PCI Express doesn't change that ;-)

	Jeff