Hi All,
Thanks for your comments. I am posting this patch after incorporating
the review comments.
Please find the attached patch file. Please review this and send your
comments.
Thanks,
Sundar
Note:
This is the patch on PCI Express Enhanced configuration for 2.6.0 test11
kernel following up to the Vladimir ([email protected]) and
Harinarayanan ([email protected]) and my previous
patches .
I tested it on our i386 platform.
This patch also implements a mechanism for the kernel to find the
chipset specific mmcfg base address. The kernel will detect the base
address of the chipset through the ACPI table entry and based on that
the PCI subsystem will be initialized.
My inputs:
- I do not like pci_express_read implemented as inline function. It is
called only in one place. It is more appropriate, on my opinion, to
merge all stuff added to include/asm-i386/pci.h , into
arch/i386/pci/direct.c.
- if you will present 4k config space for all devices, it will save lots
of work: you do not need to modify struct pci_dev, do not need almost
all stuff in drivers/pci/proc.c. By presenting 4k config for PCI device
you should not broke anything.
- Here and in _write function:
+static int pci_express_conf_read(int seg, int bus,
+ int devfn, int reg, int len, u32 *value)
+{
+ if (!value || (bus > 255) || (devfn > 255) || (reg > 4095)) {
+ printk(KERN_ERR "pci_express_conf_read: "
+ "Invalid Parameter\n");
Worth to use
printk(KERN_ERR "%s: Invalid Parameter\n",__FUNCTION__);
Durairaj, Sundarapandian wrote:
>Hi All,
>
>Thanks for your comments. I am posting this patch after incorporating
>the review comments.
>
>Please find the attached patch file. Please review this and send your
>comments.
>
>Thanks,
>Sundar
>
>Note:
>This is the patch on PCI Express Enhanced configuration for 2.6.0 test11
>kernel following up to the Vladimir ([email protected]) and
>Harinarayanan ([email protected]) and my previous
>patches .
>I tested it on our i386 platform.
>
>This patch also implements a mechanism for the kernel to find the
>chipset specific mmcfg base address. The kernel will detect the base
>address of the chipset through the ACPI table entry and based on that
>the PCI subsystem will be initialized.
>
>
On Wed, Jan 28, 2004 at 04:42:52PM +0200, Vladimir Kondratiev wrote:
> My inputs:
>
> - I do not like pci_express_read implemented as inline function. It is
> called only in one place. It is more appropriate, on my opinion, to
> merge all stuff added to include/asm-i386/pci.h , into
> arch/i386/pci/direct.c.
Actually it should be in a file of it's own, e.g. arch/i386/pci/express.c
Hello!
> - if you will present 4k config space for all devices, it will save lots
> of work: you do not need to modify struct pci_dev, do not need almost
> all stuff in drivers/pci/proc.c. By presenting 4k config for PCI device
> you should not broke anything.
For example the sizes of files in /sys/bus/pci should reflect the real size of
configuration space.
Have a nice fortnight
--
Martin `MJ' Mares <[email protected]> http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
"Anyone can build a fast CPU. The trick is to build a fast system." -- S. Cray
On Wed, Jan 28, 2004 at 03:08:01PM +0530, Durairaj, Sundarapandian wrote:
> -menu "Bus options (PCI, PCMCIA, EISA, MCA, ISA)"
> +menu "Bus options (PCI, PCMCIA, EISA, MCA, ISA, PCI_EXPRESS)"
I think this is unnecessary. For users, PCI Express is just another
form of PCI.
> +config PCI_EXPRESS
> + bool "PCI_EXPRESS (EXPERIMENTAL)"
> + depends on EXPERIMENTAL && ACPI_BOOT
Can't we do this with select? ie:
config PCI_EXPRESS
bool "PCI Express (EXPERIMENTAL)"
depends on EXPERIMENTAL
select ACPI_BOOT
> + help
> + PCI Express extends the configuration space from 256 bytes to
> + 4k bytes. It also defines an enhanced configuration mechanism
> + to access the extended configuration space. With this option,
> + you can specify that Linux will first attempt to access the
> + PCI configuration space through enhanced config access
> + mechanism (will work only on PCI Express based system)
> + otherwise other standard PCI access mechanism will be used.
I don't think this help is terribly helpful to the user. How about:
help
PCI Express is a new I/O architecture that is used in many
systems from 2004 onwards. Even if there are no PCI Express
slots on your motherboard, it may use PCI Express internally.
If you don't know, it is safe to say Y here.
Also, I would place this entry after PCI and make it depend on PCI (since
all the PCI infrastructure is relevant to PCI Express).
> +#ifdef CONFIG_PCI_EXPRESS
> + result = acpi_table_parse(ACPI_MCFG, acpi_parse_mcfg);
> + if (!result) {
> + printk(KERN_WARNING PREFIX "MCFG not present\n");
> + return 0;
> + }
> + else if (result < 0) {
CodingStyle recommends joining these last two lines together.
> +static int pci_express_conf_read(int seg, int bus,
> + int devfn, int reg, int len, u32 *value)
> +{
> + if (!value || (bus > 255) || (devfn > 255) || (reg > 4095)) {
> + printk(KERN_ERR "pci_express_conf_read: "
> + "Invalid Parameter\n");
> + return -EINVAL;
> + }
> +
> + /* Shoot misaligned transaction now */
> + if (reg & (len-1)) {
> + printk(KERN_ERR "pci_express_conf_read: "
> + "misaligned transaction\n");
> + return -EINVAL;
> + }
This last bit is not needed; Linux doesn't let misaligned requests get
this far. See drivers/pci/access.c::pci_bus_read_config_##size
> @@ -90,6 +90,8 @@
> * %PCI_CAP_ID_CHSWP CompactPCI HotSwap
> *
> * %PCI_CAP_ID_PCIX PCI-X
> + * %PCI_CAP_ID_EXP PCI-EXP
> +
seems like a stray blank line?
> */
> int
> pci_find_capability(struct pci_dev *dev, int cap)
> diff -Naur linux-2.6.0/drivers/pci/probe.c linux_pciexpress/drivers/pci/probe.c
> --- linux-2.6.0/drivers/pci/probe.c 2003-12-18 08:29:06.000000000 +0530
> +++ linux_pciexpress/drivers/pci/probe.c 2004-01-28 12:06:39.000000000 +0530
> @@ -17,6 +17,8 @@
>
> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
> #define CARDBUS_RESERVE_BUSNR 3
> +#define PCI_CFG_SPACE_SIZE 256
> +#define PCI_CFG_SPACE_EXP_SIZE 4096
fwiw, PCI-X 2 also has 4096 bytes of config space. Perhaps we can just
agree that 'EXP' stands for 'Expanded', not 'Express' in this instance?
;-)
> +static int pci_cfg_space_size(struct pci_dev *dev)
> +{
> +#ifdef CONFIG_PCI_EXPRESS
> + /* Find whether the device is PCI Express device */
> + int is_pci_express_dev =
> + pci_find_capability(dev, PCI_CAP_ID_EXP);
> + if (is_pci_express_dev)
> + return PCI_CFG_SPACE_EXP_SIZE;
> + else
I would drop the `else' here.
> +#endif
> + return PCI_CFG_SPACE_SIZE;
> +}
> +
> /*
> * Read the config data for a PCI device, sanity-check it
> * and fill in the dev structure...
> @@ -515,6 +533,7 @@
> dev->multifunction = !!(hdr_type & 0x80);
> dev->vendor = l & 0xffff;
> dev->device = (l >> 16) & 0xffff;
> + dev->cfg_size = pci_cfg_space_size(dev);
Good idea to cache it in the pci_dev.
> +static inline void pci_express_read(int bus, int devfn, int reg,
> + int len, u32 *value)
> +{
> + unsigned long flags;
> + spin_lock_irqsave(&pci_config_lock, flags);
You're already under the pci_lock spinlock (again see drivers/pci/access.c),
so I think this is unnecessary.
This is coming together nicely.
--
"Next the statesmen will invent cheap lies, putting the blame upon
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince
himself that the war is just, and will thank God for the better sleep
he enjoys after this process of grotesque self-deception." -- Mark Twain