[PATCH][0/2] RTAS MSI

[PATCH][0/2] RTAS MSI

[Jake Moilanen]

Here is a rebased RTAS MSI which uses the IRQ layer rewrite.  Please try
getting it into 2.6.18.  

I mistakenly forgot to export the MSI symbols.  So modules weren't too
happy...

The RTAS patch skips the intel-centric MSI layer and uses
pci_enable/disable_msi() calls directly.  It does not correctly handle
multi-vector MSI either.  

[PATCH][1/2] export msi symbols

[Jake Moilanen]

Forgot to export symbols for MSI.

Signed-off-by: Jake Moilanen <moilanen@austin.ibm.com>

 arch/powerpc/kernel/irq.c |    5 +++++
 1 files changed, 5 insertions(+)

Index: 2.6-msi/arch/powerpc/kernel/irq.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/kernel/irq.c
+++ 2.6-msi/arch/powerpc/kernel/irq.c
@@ -52,6 +52,7 @@
 #include <linux/radix-tree.h>
 #include <linux/mutex.h>
 #include <linux/bootmem.h>
+#include <linux/pci.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -818,12 +819,14 @@ int pci_enable_msi(struct pci_dev * pdev
 	else
 		return -1;
 }
+EXPORT_SYMBOL(pci_enable_msi);
 
 void pci_disable_msi(struct pci_dev * pdev)
 {
 	if (ppc_md.disable_msi)
 		ppc_md.disable_msi(pdev);
 }
+EXPORT_SYMBOL(pci_disable_msi);
 
 void pci_scan_msi_device(struct pci_dev *dev) {}
 int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries,
int nvec) {return -1;}
@@ -831,6 +834,8 @@ void pci_disable_msix(struct pci_dev *de
 void msi_remove_pci_irq_vectors(struct pci_dev *dev) {}
 void disable_msi_mode(struct pci_dev *dev, int pos, int type) {}
 void pci_no_msi(void) {}
+EXPORT_SYMBOL(pci_enable_msix);
+EXPORT_SYMBOL(pci_disable_msix);
 
 #endif
 

Re: [PATCH][1/2] export msi symbols

[Segher Boessenkool]

> Forgot to export symbols for MSI.
>
> Signed-off-by: Jake Moilanen <moilanen@austin.ibm.com>

Acked-by: Segher Boessenkool <segher@kernel.crashing.org>

[PATCH][2/2] RTAS MSI

[Jake Moilanen]

Rebased with the IRQ layer rewrite.  The code is not deallocating the
vectors on a pci_disable_msi().  This is to work around a firmware
vector release bug.  Plus it is really not needed, as
irq_create_mapping() just returns mappings to irqs that it knows of.

Additionally, the patch includes the client architecture bit for MSI,
and correctly identifying that MSI is edge triggered.  

Signed-off-by: Jake Moilanen <moilanen@austin.ibm.com>

 arch/powerpc/kernel/prom_init.c        |    8 ++
 arch/powerpc/platforms/pseries/setup.c |    4 +
 arch/powerpc/platforms/pseries/xics.c  |    5 -
 drivers/pci/Kconfig                    |    2 
 drivers/pci/Makefile                   |    9 +-
 drivers/pci/msi-rtas.c                 |  111
+++++++++++++++++++++++++++++++++
 include/asm-powerpc/rtas.h             |    4 +
 7 files changed, 136 insertions(+), 7 deletions(-)

Index: 2.6-msi/drivers/pci/Makefile
===================================================================
--- 2.6-msi.orig/drivers/pci/Makefile
+++ 2.6-msi/drivers/pci/Makefile
@@ -27,9 +27,12 @@ obj-$(CONFIG_PPC64) += setup-bus.o
 obj-$(CONFIG_MIPS) += setup-bus.o setup-irq.o
 obj-$(CONFIG_X86_VISWS) += setup-irq.o
 
-msiobj-y := msi.o msi-apic.o
-msiobj-$(CONFIG_IA64_GENERIC) += msi-altix.o
-msiobj-$(CONFIG_IA64_SGI_SN2) += msi-altix.o
+msiobj-$(CONFIG_X86) += msi.o msi-apic.o
+msiobj-$(CONFIG_IA64) += msi.o msi-apic.o
+msiobj-$(CONFIG_IA64_GENERIC) += msi.o msi-apic.o msi-altix.o
+msiobj-$(CONFIG_IA64_SGI_SN2) += msi.o msi-apic.o msi-altix.o
+msiobj-$(CONFIG_PPC_PSERIES) += msi-rtas.o
+
 obj-$(CONFIG_PCI_MSI) += $(msiobj-y)
 
 #
Index: 2.6-msi/drivers/pci/msi-rtas.c
===================================================================
--- /dev/null
+++ 2.6-msi/drivers/pci/msi-rtas.c
@@ -0,0 +1,111 @@
+/*
+ * Jake Moilanen <moilanen@austin.ibm.com>
+ * Copyright (C) 2006 IBM
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 of the
+ * License.
+ *
+ */
+
+#include <linux/pci.h>
+#include <linux/irq.h>
+#include <asm/rtas.h>
+#include <asm/hw_irq.h>
+#include <asm/ppc-pci.h>
+
+int rtas_enable_msi(struct pci_dev* pdev)
+{
+	static int seq_num = 1;
+	int i;
+	int rc;
+	int query_token = rtas_token("ibm,query-interrupt-source-number");
+	int devfn;
+	int busno;
+	u32 *reg;
+	int reglen;
+	int ret[3];
+	int dummy;
+	int n_intr;
+	int last_virq = NO_IRQ;
+	int virq;
+	unsigned int addr;
+	unsigned long buid = -1;
+	struct device_node * dn;
+
+	dn = pci_device_to_OF_node(pdev);
+
+	if (!of_find_property(dn, "ibm,req#msi", &dummy))
+		return -ENOENT;
+
+	reg = (u32 *) get_property(dn, "reg", &reglen);
+	if (reg == NULL || reglen < 20)
+		return -ENXIO;
+
+	devfn = (reg[0] >> 8) & 0xff;
+	busno = (reg[0] >> 16) & 0xff;
+
+	buid = get_phb_buid(dn->parent);
+	addr = (busno << 16) | (devfn << 8);
+
+	do {
+		rc = rtas_call(rtas_token("ibm,change-msi"), 6, 3, ret, addr,
+			       buid >> 32, buid & 0xffffffff,
+			       0, 0, seq_num);
+
+		seq_num = ret[1];
+	} while (rtas_busy_delay(rc));
+
+	if (rc)	{
+		printk(KERN_WARNING "error[%d]: getting the number of "
+		       "MSI interrupts for %s\n", rc, dn->name);
+		return -EIO;
+	}
+
+	/* Return if there's no MSI interrupts */
+	if (!ret[0])
+		return -ENOENT;
+
+	n_intr = ret[0];
+
+	for (i = 0; i < n_intr; i++) {
+		do {
+			rc = rtas_call(query_token, 4, 3, ret, addr,
+				       buid >> 32, buid & 0xffffffff, i);
+		} while (rtas_busy_delay(rc));
+
+		if (!rc) {
+			virq = irq_create_mapping(irq_find_host(dn), ret[0],
+						ret[1] ? IRQ_TYPE_EDGE_RISING :
+						IRQ_TYPE_LEVEL_LOW);
+
+			/* for now, take the last valid vector given out */
+			if (virq != NO_IRQ)
+				last_virq = virq;
+
+		} else {
+			printk(KERN_WARNING "error[%d]: "
+			       "query-interrupt-source-number for %s\n",
+			       rc, dn->name);
+		}
+	}
+
+	/*
+	 * If we can't get any MSI vectors, fail and try falling
+	 * back to LSI
+	 */
+	if (last_virq == NO_IRQ)
+		return -EIO;
+
+	pdev->irq = last_virq;
+
+	return 0;
+}
+
+void rtas_disable_msi(struct pci_dev* pdev)
+{
+	/*
+	 * for now, we don't give firmware back vectors to their pool
+	 */
+}
Index: 2.6-msi/drivers/pci/Kconfig
===================================================================
--- 2.6-msi.orig/drivers/pci/Kconfig
+++ 2.6-msi/drivers/pci/Kconfig
@@ -4,7 +4,7 @@
 config PCI_MSI
 	bool "Message Signaled Interrupts (MSI and MSI-X)"
 	depends on PCI
-	depends on (X86_LOCAL_APIC && X86_IO_APIC) || IA64
+	depends on (X86_LOCAL_APIC && X86_IO_APIC) || IA64 || PPC_PSERIES
 	help
 	   This allows device drivers to enable MSI (Message Signaled
 	   Interrupts).  Message Signaled Interrupts enable a device to
Index: 2.6-msi/arch/powerpc/platforms/pseries/setup.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/platforms/pseries/setup.c
+++ 2.6-msi/arch/powerpc/platforms/pseries/setup.c
@@ -267,6 +267,10 @@ static void __init pseries_discover_pic(
 			return;
 		} else if (strstr(typep, "ppc-xicp")) {
 			ppc_md.init_IRQ       = xics_init_IRQ;
+#ifdef CONFIG_PCI_MSI
+			ppc_md.enable_msi	= rtas_enable_msi;
+			ppc_md.disable_msi	= rtas_disable_msi;
+#endif
 #ifdef CONFIG_KEXEC
 			ppc_md.kexec_cpu_down = pseries_kexec_cpu_down_xics;
 #endif
Index: 2.6-msi/include/asm-powerpc/rtas.h
===================================================================
--- 2.6-msi.orig/include/asm-powerpc/rtas.h
+++ 2.6-msi/include/asm-powerpc/rtas.h
@@ -4,6 +4,7 @@
 
 #include <linux/spinlock.h>
 #include <asm/page.h>
+#include <linux/pci.h>
 
 /*
  * Definitions for talking to the RTAS on CHRP machines.
@@ -186,6 +187,9 @@ extern int early_init_dt_scan_rtas(unsig
 
 extern void pSeries_log_error(char *buf, unsigned int err_type, int
fatal);
 
+extern int rtas_enable_msi(struct pci_dev* pdev);
+extern void rtas_disable_msi(struct pci_dev * pdev);
+
 /* Error types logged.  */
 #define ERR_FLAG_ALREADY_LOGGED	0x0
 #define ERR_FLAG_BOOT		0x1 	/* log was pulled from NVRAM on boot */
Index: 2.6-msi/arch/powerpc/kernel/prom_init.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/kernel/prom_init.c
+++ 2.6-msi/arch/powerpc/kernel/prom_init.c
@@ -632,6 +632,12 @@ static void __init early_cmdline_parse(v
 /* ibm,dynamic-reconfiguration-memory property supported */
 #define OV5_DRCONF_MEMORY	0x20
 #define OV5_LARGE_PAGES		0x10	/* large pages supported */
+/* PCIe/MSI support.  Without MSI full PCIe is not supported */
+#ifdef CONFIG_PCI_MSI
+#define OV5_MSI			0x01	/* PCIe/MSI support */
+#else
+#define OV5_MSI			0x00
+#endif /* CONFIG_PCI_MSI */
 
 /*
  * The architecture vector has an array of PVR mask/value pairs,
@@ -675,7 +681,7 @@ static unsigned char ibm_architecture_ve
 	/* option vector 5: PAPR/OF options */
 	3 - 1,				/* length */
 	0,				/* don't ignore, don't halt */
-	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES,
+	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES | OV5_MSI,
 };
 
 /* Old method - ELF header with PT_NOTE sections */
Index: 2.6-msi/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/platforms/pseries/xics.c
+++ 2.6-msi/arch/powerpc/platforms/pseries/xics.c
@@ -526,11 +526,12 @@ static int xics_host_map_lpar(struct irq
 	pr_debug("xics: map_lpar virq %d, hwirq 0x%lx, flags: 0x%x\n",
 		 virq, hw, flags);
 
-	if (sense && sense != IRQ_TYPE_LEVEL_LOW)
+	if (sense && !(sense & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_RISING)))
 		printk(KERN_WARNING "xics: using unsupported sense 0x%x"
 		       " for irq %d (h: 0x%lx)\n", flags, virq, hw);
 
-	get_irq_desc(virq)->status |= IRQ_LEVEL;
+	if (sense && sense & IRQ_TYPE_LEVEL_LOW)
+		get_irq_desc(virq)->status |= IRQ_LEVEL;
 	set_irq_chip_and_handler(virq, &xics_pic_lpar, handle_fasteoi_irq);
 	return 0;
 }

Re: [PATCH][2/2] RTAS MSI

[Segher Boessenkool]

> Index: 2.6-msi/drivers/pci/Makefile
> ===================================================================
> --- 2.6-msi.orig/drivers/pci/Makefile
> +++ 2.6-msi/drivers/pci/Makefile
> @@ -27,9 +27,12 @@ obj-$(CONFIG_PPC64) += setup-bus.o
>  obj-$(CONFIG_MIPS) += setup-bus.o setup-irq.o
>  obj-$(CONFIG_X86_VISWS) += setup-irq.o
>
> -msiobj-y := msi.o msi-apic.o
> -msiobj-$(CONFIG_IA64_GENERIC) += msi-altix.o
> -msiobj-$(CONFIG_IA64_SGI_SN2) += msi-altix.o
> +msiobj-$(CONFIG_X86) += msi.o msi-apic.o
> +msiobj-$(CONFIG_IA64) += msi.o msi-apic.o

> +msiobj-$(CONFIG_IA64_GENERIC) += msi.o msi-apic.o msi-altix.o
> +msiobj-$(CONFIG_IA64_SGI_SN2) += msi.o msi-apic.o msi-altix.o

These two lines don't need the msi.o and msi-altix.o AFAICS.

> +msiobj-$(CONFIG_PPC_PSERIES) += msi-rtas.o

I think this file should live in arch/powerpc, and so should this
Makefile fragment.

> +
>  obj-$(CONFIG_PCI_MSI) += $(msiobj-y)
>
>  #

Other than that, can we please have the part that doesn't build the
"generic" MSI stuff included ASAP?

> Index: 2.6-msi/drivers/pci/msi-rtas.c

Maybe msi-papr.c is a better name btw?  Not that I care :-)

No comments on your actual code, sorry.


Segher

Re: [PATCH][2/2] RTAS MSI

[Segher Boessenkool]

>> +msiobj-$(CONFIG_IA64_GENERIC) += msi.o msi-apic.o msi-altix.o
>> +msiobj-$(CONFIG_IA64_SGI_SN2) += msi.o msi-apic.o msi-altix.o
>
> These two lines don't need the msi.o and msi-altix.o AFAICS.

s/altix/apic/.  Duh.


Segher

Re: [PATCH][2/2] RTAS MSI

[Jake Moilanen]

On Thu, 2006-07-27 at 20:46 +0200, Segher Boessenkool wrote:
> > Index: 2.6-msi/drivers/pci/Makefile
> > ===================================================================
> > --- 2.6-msi.orig/drivers/pci/Makefile
> > +++ 2.6-msi/drivers/pci/Makefile
> > @@ -27,9 +27,12 @@ obj-$(CONFIG_PPC64) += setup-bus.o
> >  obj-$(CONFIG_MIPS) += setup-bus.o setup-irq.o
> >  obj-$(CONFIG_X86_VISWS) += setup-irq.o
> >
> > -msiobj-y := msi.o msi-apic.o
> > -msiobj-$(CONFIG_IA64_GENERIC) += msi-altix.o
> > -msiobj-$(CONFIG_IA64_SGI_SN2) += msi-altix.o
> > +msiobj-$(CONFIG_X86) += msi.o msi-apic.o
> > +msiobj-$(CONFIG_IA64) += msi.o msi-apic.o
> 
> > +msiobj-$(CONFIG_IA64_GENERIC) += msi.o msi-apic.o msi-altix.o
> > +msiobj-$(CONFIG_IA64_SGI_SN2) += msi.o msi-apic.o msi-altix.o
> 
> These two lines don't need the msi.o and msi-altix.o AFAICS.

Yup, you are right...updated patch included below.

> > +msiobj-$(CONFIG_PPC_PSERIES) += msi-rtas.o
> 
> I think this file should live in arch/powerpc, and so should this
> Makefile fragment.

I'm following the current MSI methodologies where arch specific MSI code
lives in drivers/pci.  This is just like msi-altix.c.

> > +
> >  obj-$(CONFIG_PCI_MSI) += $(msiobj-y)
> >
> >  #
> 
> Other than that, can we please have the part that doesn't build the
> "generic" MSI stuff included ASAP?
> 
> > Index: 2.6-msi/drivers/pci/msi-rtas.c
> 
> Maybe msi-papr.c is a better name btw?  Not that I care :-)

IMHO I don't like PAPR names because they like changing them on a whim.
RTAS is a bit more persistent.

I don't really care...If anyone feels real strongly about it, I'll
change it.

Signed-off-by: Jake Moilanen <moilanen@austin.ibm.com>

 arch/powerpc/kernel/prom_init.c        |    8 ++
 arch/powerpc/platforms/pseries/setup.c |    4 +
 arch/powerpc/platforms/pseries/xics.c  |    5 -
 drivers/pci/Kconfig                    |    2 
 drivers/pci/Makefile                   |    5 +
 drivers/pci/msi-rtas.c                 |  111
+++++++++++++++++++++++++++++++++
 include/asm-powerpc/rtas.h             |    4 +
 7 files changed, 134 insertions(+), 5 deletions(-)

Index: 2.6-msi/drivers/pci/Makefile
===================================================================
--- 2.6-msi.orig/drivers/pci/Makefile
+++ 2.6-msi/drivers/pci/Makefile
@@ -27,9 +27,12 @@ obj-$(CONFIG_PPC64) += setup-bus.o
 obj-$(CONFIG_MIPS) += setup-bus.o setup-irq.o
 obj-$(CONFIG_X86_VISWS) += setup-irq.o
 
-msiobj-y := msi.o msi-apic.o
+msiobj-$(CONFIG_X86) += msi.o msi-apic.o
+msiobj-$(CONFIG_IA64) += msi.o msi-apic.o
 msiobj-$(CONFIG_IA64_GENERIC) += msi-altix.o
 msiobj-$(CONFIG_IA64_SGI_SN2) += msi-altix.o
+msiobj-$(CONFIG_PPC_PSERIES) += msi-rtas.o
+
 obj-$(CONFIG_PCI_MSI) += $(msiobj-y)
 
 #
Index: 2.6-msi/drivers/pci/msi-rtas.c
===================================================================
--- /dev/null
+++ 2.6-msi/drivers/pci/msi-rtas.c
@@ -0,0 +1,111 @@
+/*
+ * Jake Moilanen <moilanen@austin.ibm.com>
+ * Copyright (C) 2006 IBM
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 of the
+ * License.
+ *
+ */
+
+#include <linux/pci.h>
+#include <linux/irq.h>
+#include <asm/rtas.h>
+#include <asm/hw_irq.h>
+#include <asm/ppc-pci.h>
+
+int rtas_enable_msi(struct pci_dev* pdev)
+{
+	static int seq_num = 1;
+	int i;
+	int rc;
+	int query_token = rtas_token("ibm,query-interrupt-source-number");
+	int devfn;
+	int busno;
+	u32 *reg;
+	int reglen;
+	int ret[3];
+	int dummy;
+	int n_intr;
+	int last_virq = NO_IRQ;
+	int virq;
+	unsigned int addr;
+	unsigned long buid = -1;
+	struct device_node * dn;
+
+	dn = pci_device_to_OF_node(pdev);
+
+	if (!of_find_property(dn, "ibm,req#msi", &dummy))
+		return -ENOENT;
+
+	reg = (u32 *) get_property(dn, "reg", &reglen);
+	if (reg == NULL || reglen < 20)
+		return -ENXIO;
+
+	devfn = (reg[0] >> 8) & 0xff;
+	busno = (reg[0] >> 16) & 0xff;
+
+	buid = get_phb_buid(dn->parent);
+	addr = (busno << 16) | (devfn << 8);
+
+	do {
+		rc = rtas_call(rtas_token("ibm,change-msi"), 6, 3, ret, addr,
+			       buid >> 32, buid & 0xffffffff,
+			       0, 0, seq_num);
+
+		seq_num = ret[1];
+	} while (rtas_busy_delay(rc));
+
+	if (rc)	{
+		printk(KERN_WARNING "error[%d]: getting the number of "
+		       "MSI interrupts for %s\n", rc, dn->name);
+		return -EIO;
+	}
+
+	/* Return if there's no MSI interrupts */
+	if (!ret[0])
+		return -ENOENT;
+
+	n_intr = ret[0];
+
+	for (i = 0; i < n_intr; i++) {
+		do {
+			rc = rtas_call(query_token, 4, 3, ret, addr,
+				       buid >> 32, buid & 0xffffffff, i);
+		} while (rtas_busy_delay(rc));
+
+		if (!rc) {
+			virq = irq_create_mapping(irq_find_host(dn), ret[0],
+						ret[1] ? IRQ_TYPE_EDGE_RISING :
+						IRQ_TYPE_LEVEL_LOW);
+
+			/* for now, take the last valid vector given out */
+			if (virq != NO_IRQ)
+				last_virq = virq;
+
+		} else {
+			printk(KERN_WARNING "error[%d]: "
+			       "query-interrupt-source-number for %s\n",
+			       rc, dn->name);
+		}
+	}
+
+	/*
+	 * If we can't get any MSI vectors, fail and try falling
+	 * back to LSI
+	 */
+	if (last_virq == NO_IRQ)
+		return -EIO;
+
+	pdev->irq = last_virq;
+
+	return 0;
+}
+
+void rtas_disable_msi(struct pci_dev* pdev)
+{
+	/*
+	 * for now, we don't give firmware back vectors to their pool
+	 */
+}
Index: 2.6-msi/drivers/pci/Kconfig
===================================================================
--- 2.6-msi.orig/drivers/pci/Kconfig
+++ 2.6-msi/drivers/pci/Kconfig
@@ -4,7 +4,7 @@
 config PCI_MSI
 	bool "Message Signaled Interrupts (MSI and MSI-X)"
 	depends on PCI
-	depends on (X86_LOCAL_APIC && X86_IO_APIC) || IA64
+	depends on (X86_LOCAL_APIC && X86_IO_APIC) || IA64 || PPC_PSERIES
 	help
 	   This allows device drivers to enable MSI (Message Signaled
 	   Interrupts).  Message Signaled Interrupts enable a device to
Index: 2.6-msi/arch/powerpc/platforms/pseries/setup.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/platforms/pseries/setup.c
+++ 2.6-msi/arch/powerpc/platforms/pseries/setup.c
@@ -267,6 +267,10 @@ static void __init pseries_discover_pic(
 			return;
 		} else if (strstr(typep, "ppc-xicp")) {
 			ppc_md.init_IRQ       = xics_init_IRQ;
+#ifdef CONFIG_PCI_MSI
+			ppc_md.enable_msi	= rtas_enable_msi;
+			ppc_md.disable_msi	= rtas_disable_msi;
+#endif
 #ifdef CONFIG_KEXEC
 			ppc_md.kexec_cpu_down = pseries_kexec_cpu_down_xics;
 #endif
Index: 2.6-msi/include/asm-powerpc/rtas.h
===================================================================
--- 2.6-msi.orig/include/asm-powerpc/rtas.h
+++ 2.6-msi/include/asm-powerpc/rtas.h
@@ -4,6 +4,7 @@
 
 #include <linux/spinlock.h>
 #include <asm/page.h>
+#include <linux/pci.h>
 
 /*
  * Definitions for talking to the RTAS on CHRP machines.
@@ -186,6 +187,9 @@ extern int early_init_dt_scan_rtas(unsig
 
 extern void pSeries_log_error(char *buf, unsigned int err_type, int
fatal);
 
+extern int rtas_enable_msi(struct pci_dev* pdev);
+extern void rtas_disable_msi(struct pci_dev * pdev);
+
 /* Error types logged.  */
 #define ERR_FLAG_ALREADY_LOGGED	0x0
 #define ERR_FLAG_BOOT		0x1 	/* log was pulled from NVRAM on boot */
Index: 2.6-msi/arch/powerpc/kernel/prom_init.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/kernel/prom_init.c
+++ 2.6-msi/arch/powerpc/kernel/prom_init.c
@@ -632,6 +632,12 @@ static void __init early_cmdline_parse(v
 /* ibm,dynamic-reconfiguration-memory property supported */
 #define OV5_DRCONF_MEMORY	0x20
 #define OV5_LARGE_PAGES		0x10	/* large pages supported */
+/* PCIe/MSI support.  Without MSI full PCIe is not supported */
+#ifdef CONFIG_PCI_MSI
+#define OV5_MSI			0x01	/* PCIe/MSI support */
+#else
+#define OV5_MSI			0x00
+#endif /* CONFIG_PCI_MSI */
 
 /*
  * The architecture vector has an array of PVR mask/value pairs,
@@ -675,7 +681,7 @@ static unsigned char ibm_architecture_ve
 	/* option vector 5: PAPR/OF options */
 	3 - 1,				/* length */
 	0,				/* don't ignore, don't halt */
-	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES,
+	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES | OV5_MSI,
 };
 
 /* Old method - ELF header with PT_NOTE sections */
Index: 2.6-msi/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/platforms/pseries/xics.c
+++ 2.6-msi/arch/powerpc/platforms/pseries/xics.c
@@ -526,11 +526,12 @@ static int xics_host_map_lpar(struct irq
 	pr_debug("xics: map_lpar virq %d, hwirq 0x%lx, flags: 0x%x\n",
 		 virq, hw, flags);
 
-	if (sense && sense != IRQ_TYPE_LEVEL_LOW)
+	if (sense && !(sense & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_RISING)))
 		printk(KERN_WARNING "xics: using unsupported sense 0x%x"
 		       " for irq %d (h: 0x%lx)\n", flags, virq, hw);
 
-	get_irq_desc(virq)->status |= IRQ_LEVEL;
+	if (sense && sense & IRQ_TYPE_LEVEL_LOW)
+		get_irq_desc(virq)->status |= IRQ_LEVEL;
 	set_irq_chip_and_handler(virq, &xics_pic_lpar, handle_fasteoi_irq);
 	return 0;
 }

Re: [PATCH][2/2] RTAS MSI

[Segher Boessenkool]

>> These two lines don't need the msi.o and msi-altix.o AFAICS.
>
> Yup, you are right...updated patch included below.

Looks fine now, thanks.

>>> +msiobj-$(CONFIG_PPC_PSERIES) += msi-rtas.o
>>
>> I think this file should live in arch/powerpc, and so should this
>> Makefile fragment.
>
> I'm following the current MSI methodologies where arch specific MSI  
> code
> lives in drivers/pci.  This is just like msi-altix.c.

True, but our core support lives in arch/.  The same is true for PHB
code etc.; if Altix messed this up, that doesn't mean we need to :-)

>> Other than that, can we please have the part that doesn't build the
>> "generic" MSI stuff included ASAP?

This would naturally fall out as a separate patch then, as well.

>>> Index: 2.6-msi/drivers/pci/msi-rtas.c
>>
>> Maybe msi-papr.c is a better name btw?  Not that I care :-)
>
> IMHO I don't like PAPR names because they like changing them on a  
> whim.
> RTAS is a bit more persistent.

Maybe msi-pseries then?  Oh wait, they changed that name too ;-)

> I don't really care...If anyone feels real strongly about it, I'll
> change it.

If you move this code to arch/powerpc/platforms/pseries/, you could
just call it "msi.c".  I don't like the msi-rtas name -- but, I don't
feel that strongly about it, just a suggestion.


Segher

Re: [PATCH][2/2] RTAS MSI

[Paul Mackerras]

Jake Moilanen writes:

> -msiobj-y := msi.o msi-apic.o
> -msiobj-$(CONFIG_IA64_GENERIC) += msi-altix.o
> -msiobj-$(CONFIG_IA64_SGI_SN2) += msi-altix.o
> +msiobj-$(CONFIG_X86) += msi.o msi-apic.o
> +msiobj-$(CONFIG_IA64) += msi.o msi-apic.o
> +msiobj-$(CONFIG_IA64_GENERIC) += msi.o msi-apic.o msi-altix.o
> +msiobj-$(CONFIG_IA64_SGI_SN2) += msi.o msi-apic.o msi-altix.o

Doesn't this mean that ia64 configs might now get msi.o and msi-apic.o
listed twice?  Why did you need to change the
msiobj-$(CONFIG_IA64_GENERIC) and msiobj-$(CONFIG_IA64_SGI_SN2) lines
at all?

Paul.

Re: [PATCH][2/2] RTAS MSI

[Jake Moilanen]

On Mon, 2006-07-31 at 14:07 +1000, Paul Mackerras wrote:
> Jake Moilanen writes:
> 
> > -msiobj-y := msi.o msi-apic.o
> > -msiobj-$(CONFIG_IA64_GENERIC) += msi-altix.o
> > -msiobj-$(CONFIG_IA64_SGI_SN2) += msi-altix.o
> > +msiobj-$(CONFIG_X86) += msi.o msi-apic.o
> > +msiobj-$(CONFIG_IA64) += msi.o msi-apic.o
> > +msiobj-$(CONFIG_IA64_GENERIC) += msi.o msi-apic.o msi-altix.o
> > +msiobj-$(CONFIG_IA64_SGI_SN2) += msi.o msi-apic.o msi-altix.o
> 
> Doesn't this mean that ia64 configs might now get msi.o and msi-apic.o
> listed twice?  Why did you need to change the
> msiobj-$(CONFIG_IA64_GENERIC) and msiobj-$(CONFIG_IA64_SGI_SN2) lines
> at all?

Yup...see the second patch I posted where I fixed up the ia64 configs so
msi.o and msi-apic.o are listed once and msiobj-$(CONFIG_IA64_GENERIC)
and msiobj-$(CONFIG_IA64_SGI_SN2) are not changed.

In a few, I'll be posting another version of the patch which address
Michael's concerns.

Re: [PATCH][2/2] RTAS MSI

[Michael Ellerman]

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

On Thu, 2006-07-27 at 13:27 -0500, Jake Moilanen wrote:
> Rebased with the IRQ layer rewrite.  The code is not deallocating the
> vectors on a pci_disable_msi().  This is to work around a firmware
> vector release bug.  Plus it is really not needed, as
> irq_create_mapping() just returns mappings to irqs that it knows of.
> 
> Additionally, the patch includes the client architecture bit for MSI,
> and correctly identifying that MSI is edge triggered.  

Hey Jake, just a few niggles below ..

> Index: 2.6-msi/drivers/pci/msi-rtas.c
> ===================================================================
> --- /dev/null
> +++ 2.6-msi/drivers/pci/msi-rtas.c
> @@ -0,0 +1,111 @@
> +/*
> + * Jake Moilanen <moilanen@austin.ibm.com>
> + * Copyright (C) 2006 IBM
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2 of the
> + * License.
> + *
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/irq.h>
> +#include <asm/rtas.h>
> +#include <asm/hw_irq.h>
> +#include <asm/ppc-pci.h>
> +
> +int rtas_enable_msi(struct pci_dev* pdev)
> +{
> +	static int seq_num = 1;

Do we really want seq_num to be static? By my reading of PAPR we only
need to maintain the seq number across calls that return -2/990x, and we
handle that all inside this function. If it does need to be unique for
_all_ calls, then I don't see how seq_num being static is going to work,
a different cpu could stomp on the seq_num value between calls, which
presumably would make firmware mad.

> +	int i;
> +	int rc;
> +	int query_token = rtas_token("ibm,query-interrupt-source-number");
> +	int devfn;
> +	int busno;
> +	u32 *reg;
> +	int reglen;
> +	int ret[3];

You only need ret[2] I think, the first return value (status) is handled
inside rtas_call for you.

> +	int dummy;
> +	int n_intr;
> +	int last_virq = NO_IRQ;
> +	int virq;
> +	unsigned int addr;
> +	unsigned long buid = -1;

No need to set buid to -1 as you unconditionally assign to it later.

> +	struct device_node * dn;
> +
> +	dn = pci_device_to_OF_node(pdev);
> +
> +	if (!of_find_property(dn, "ibm,req#msi", &dummy))
> +		return -ENOENT;

You don't need dummy, just pass NULL.

> +
> +	reg = (u32 *) get_property(dn, "reg", &reglen);
> +	if (reg == NULL || reglen < 20)
> +		return -ENXIO;
> +
> +	devfn = (reg[0] >> 8) & 0xff;
> +	busno = (reg[0] >> 16) & 0xff;
> +
> +	buid = get_phb_buid(dn->parent);
> +	addr = (busno << 16) | (devfn << 8);

Why do we need to read the reg here, can't we just use the existing
fields? ie:

        addr = (pdev->bus->number << 16) | (pdev->devfn << 8);

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

Re: [PATCH][2/2] RTAS MSI

[Jake Moilanen]

> > +int rtas_enable_msi(struct pci_dev* pdev)
> > +{
> > +	static int seq_num = 1;
> 
> Do we really want seq_num to be static? By my reading of PAPR we only
> need to maintain the seq number across calls that return -2/990x, and we
> handle that all inside this function. If it does need to be unique for
> _all_ calls, then I don't see how seq_num being static is going to work,
> a different cpu could stomp on the seq_num value between calls, which
> presumably would make firmware mad.

Bah...I thought I fixed this a long time ago....must have gotten dropped
somewhere in the mix.

> > +	int reglen;
> > +	int ret[3];
> 
> You only need ret[2] I think, the first return value (status) is handled
> inside rtas_call for you.

Yup...

> > +	int dummy;
> > +	int n_intr;
> > +	int last_virq = NO_IRQ;
> > +	int virq;
> > +	unsigned int addr;
> > +	unsigned long buid = -1;
> 
> No need to set buid to -1 as you unconditionally assign to it later.

Agreed

> > +	struct device_node * dn;
> > +
> > +	dn = pci_device_to_OF_node(pdev);
> > +
> > +	if (!of_find_property(dn, "ibm,req#msi", &dummy))
> > +		return -ENOENT;
> 
> You don't need dummy, just pass NULL.

Yup.

> > +
> > +	reg = (u32 *) get_property(dn, "reg", &reglen);
> > +	if (reg == NULL || reglen < 20)
> > +		return -ENXIO;
> > +
> > +	devfn = (reg[0] >> 8) & 0xff;
> > +	busno = (reg[0] >> 16) & 0xff;
> > +
> > +	buid = get_phb_buid(dn->parent);
> > +	addr = (busno << 16) | (devfn << 8);
> 
> Why do we need to read the reg here, can't we just use the existing
> fields? ie:
> 
>         addr = (pdev->bus->number << 16) | (pdev->devfn << 8);

Patch coming w/ all these fixes.

Re: [PATCH][2/2] RTAS MSI

[Jake Moilanen]

Here's version 3 which addresses all of Michael's comments.


Signed-off-by: Jake Moilanen <moilanen@austin.ibm.com>

 arch/powerpc/kernel/prom_init.c        |    8 ++
 arch/powerpc/platforms/pseries/setup.c |    4 +
 arch/powerpc/platforms/pseries/xics.c  |    5 +
 drivers/pci/Kconfig                    |    2 
 drivers/pci/Makefile                   |    5 +
 drivers/pci/msi-rtas.c                 |   99
+++++++++++++++++++++++++++++++++
 include/asm-powerpc/rtas.h             |    4 +
 7 files changed, 122 insertions(+), 5 deletions(-)

Index: 2.6-msi/drivers/pci/Makefile
===================================================================
--- 2.6-msi.orig/drivers/pci/Makefile
+++ 2.6-msi/drivers/pci/Makefile
@@ -27,9 +27,12 @@ obj-$(CONFIG_PPC64) += setup-bus.o
 obj-$(CONFIG_MIPS) += setup-bus.o setup-irq.o
 obj-$(CONFIG_X86_VISWS) += setup-irq.o
 
-msiobj-y := msi.o msi-apic.o
+msiobj-$(CONFIG_X86) += msi.o msi-apic.o
+msiobj-$(CONFIG_IA64) += msi.o msi-apic.o
 msiobj-$(CONFIG_IA64_GENERIC) += msi-altix.o
 msiobj-$(CONFIG_IA64_SGI_SN2) += msi-altix.o
+msiobj-$(CONFIG_PPC_PSERIES) += msi-rtas.o
+
 obj-$(CONFIG_PCI_MSI) += $(msiobj-y)
 
 #
Index: 2.6-msi/drivers/pci/msi-rtas.c
===================================================================
--- /dev/null
+++ 2.6-msi/drivers/pci/msi-rtas.c
@@ -0,0 +1,99 @@
+/*
+ * Jake Moilanen <moilanen@austin.ibm.com>
+ * Copyright (C) 2006 IBM
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 of the
+ * License.
+ *
+ */
+
+#include <linux/pci.h>
+#include <linux/irq.h>
+#include <asm/rtas.h>
+#include <asm/hw_irq.h>
+#include <asm/ppc-pci.h>
+
+int rtas_enable_msi(struct pci_dev* pdev)
+{
+	int seq_num = 1;
+	int i;
+	int rc;
+	int query_token = rtas_token("ibm,query-interrupt-source-number");
+	int ret[2];
+	int n_intr;
+	int last_virq = NO_IRQ;
+	int virq;
+	unsigned int addr;
+	unsigned long buid;
+	struct device_node * dn;
+
+	dn = pci_device_to_OF_node(pdev);
+
+	if (!of_find_property(dn, "ibm,req#msi", NULL))
+		return -ENOENT;
+
+	buid = get_phb_buid(dn->parent);
+	addr = (pdev->bus->number << 16) | (pdev->devfn << 8);
+
+	do {
+		rc = rtas_call(rtas_token("ibm,change-msi"), 6, 3, ret, addr,
+			       buid >> 32, buid & 0xffffffff,
+			       0, 0, seq_num);
+
+		seq_num = ret[1];
+	} while (rtas_busy_delay(rc));
+
+	if (rc)	{
+		printk(KERN_WARNING "error[%d]: getting the number of "
+		       "MSI interrupts for %s\n", rc, dn->name);
+		return -EIO;
+	}
+
+	/* Return if there's no MSI interrupts */
+	if (!ret[0])
+		return -ENOENT;
+
+	n_intr = ret[0];
+
+	for (i = 0; i < n_intr; i++) {
+		do {
+			rc = rtas_call(query_token, 4, 3, ret, addr,
+				       buid >> 32, buid & 0xffffffff, i);
+		} while (rtas_busy_delay(rc));
+
+		if (!rc) {
+			virq = irq_create_mapping(irq_find_host(dn), ret[0],
+						ret[1] ? IRQ_TYPE_EDGE_RISING :
+						IRQ_TYPE_LEVEL_LOW);
+
+			/* for now, take the last valid vector given out */
+			if (virq != NO_IRQ)
+				last_virq = virq;
+
+		} else {
+			printk(KERN_WARNING "error[%d]: "
+			       "query-interrupt-source-number for %s\n",
+			       rc, dn->name);
+		}
+	}
+
+	/*
+	 * If we can't get any MSI vectors, fail and try falling
+	 * back to LSI
+	 */
+	if (last_virq == NO_IRQ)
+		return -EIO;
+
+	pdev->irq = last_virq;
+
+	return 0;
+}
+
+void rtas_disable_msi(struct pci_dev* pdev)
+{
+	/*
+	 * for now, we don't give firmware back vectors to their pool
+	 */
+}
Index: 2.6-msi/drivers/pci/Kconfig
===================================================================
--- 2.6-msi.orig/drivers/pci/Kconfig
+++ 2.6-msi/drivers/pci/Kconfig
@@ -4,7 +4,7 @@
 config PCI_MSI
 	bool "Message Signaled Interrupts (MSI and MSI-X)"
 	depends on PCI
-	depends on (X86_LOCAL_APIC && X86_IO_APIC) || IA64
+	depends on (X86_LOCAL_APIC && X86_IO_APIC) || IA64 || PPC_PSERIES
 	help
 	   This allows device drivers to enable MSI (Message Signaled
 	   Interrupts).  Message Signaled Interrupts enable a device to
Index: 2.6-msi/arch/powerpc/platforms/pseries/setup.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/platforms/pseries/setup.c
+++ 2.6-msi/arch/powerpc/platforms/pseries/setup.c
@@ -267,6 +267,10 @@ static void __init pseries_discover_pic(
 			return;
 		} else if (strstr(typep, "ppc-xicp")) {
 			ppc_md.init_IRQ       = xics_init_IRQ;
+#ifdef CONFIG_PCI_MSI
+			ppc_md.enable_msi	= rtas_enable_msi;
+			ppc_md.disable_msi	= rtas_disable_msi;
+#endif
 #ifdef CONFIG_KEXEC
 			ppc_md.kexec_cpu_down = pseries_kexec_cpu_down_xics;
 #endif
Index: 2.6-msi/include/asm-powerpc/rtas.h
===================================================================
--- 2.6-msi.orig/include/asm-powerpc/rtas.h
+++ 2.6-msi/include/asm-powerpc/rtas.h
@@ -4,6 +4,7 @@
 
 #include <linux/spinlock.h>
 #include <asm/page.h>
+#include <linux/pci.h>
 
 /*
  * Definitions for talking to the RTAS on CHRP machines.
@@ -186,6 +187,9 @@ extern int early_init_dt_scan_rtas(unsig
 
 extern void pSeries_log_error(char *buf, unsigned int err_type, int
fatal);
 
+extern int rtas_enable_msi(struct pci_dev* pdev);
+extern void rtas_disable_msi(struct pci_dev * pdev);
+
 /* Error types logged.  */
 #define ERR_FLAG_ALREADY_LOGGED	0x0
 #define ERR_FLAG_BOOT		0x1 	/* log was pulled from NVRAM on boot */
Index: 2.6-msi/arch/powerpc/kernel/prom_init.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/kernel/prom_init.c
+++ 2.6-msi/arch/powerpc/kernel/prom_init.c
@@ -632,6 +632,12 @@ static void __init early_cmdline_parse(v
 /* ibm,dynamic-reconfiguration-memory property supported */
 #define OV5_DRCONF_MEMORY	0x20
 #define OV5_LARGE_PAGES		0x10	/* large pages supported */
+/* PCIe/MSI support.  Without MSI full PCIe is not supported */
+#ifdef CONFIG_PCI_MSI
+#define OV5_MSI			0x01	/* PCIe/MSI support */
+#else
+#define OV5_MSI			0x00
+#endif /* CONFIG_PCI_MSI */
 
 /*
  * The architecture vector has an array of PVR mask/value pairs,
@@ -675,7 +681,7 @@ static unsigned char ibm_architecture_ve
 	/* option vector 5: PAPR/OF options */
 	3 - 1,				/* length */
 	0,				/* don't ignore, don't halt */
-	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES,
+	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES | OV5_MSI,
 };
 
 /* Old method - ELF header with PT_NOTE sections */
Index: 2.6-msi/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- 2.6-msi.orig/arch/powerpc/platforms/pseries/xics.c
+++ 2.6-msi/arch/powerpc/platforms/pseries/xics.c
@@ -526,11 +526,12 @@ static int xics_host_map_lpar(struct irq
 	pr_debug("xics: map_lpar virq %d, hwirq 0x%lx, flags: 0x%x\n",
 		 virq, hw, flags);
 
-	if (sense && sense != IRQ_TYPE_LEVEL_LOW)
+	if (sense && !(sense & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_RISING)))
 		printk(KERN_WARNING "xics: using unsupported sense 0x%x"
 		       " for irq %d (h: 0x%lx)\n", flags, virq, hw);
 
-	get_irq_desc(virq)->status |= IRQ_LEVEL;
+	if (sense && sense & IRQ_TYPE_LEVEL_LOW)
+		get_irq_desc(virq)->status |= IRQ_LEVEL;
 	set_irq_chip_and_handler(virq, &xics_pic_lpar, handle_fasteoi_irq);
 	return 0;
 }

Re: [PATCH][2/2] RTAS MSI

[Michael Ellerman]

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

On Mon, 2006-07-31 at 16:01 -0500, Jake Moilanen wrote:
> Here's version 3 which addresses all of Michael's comments.

Looks good, nice one.

I was thinking about disable, can you give us any more details on the
firmware bug that is blocking that?

I'm not sure what disable really needs to do, but I think drivers might
expect it to set dev->irq back to the LSI value, even if we don't free
anything internally. Not sure.

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

Re: [PATCH][2/2] RTAS MSI

[Segher Boessenkool]

> I'm not sure what disable really needs to do, but I think drivers  
> might
> expect it to set dev->irq back to the LSI value, even if we don't free
> anything internally. Not sure.

Yes, that's required by the MSI API.


Segher

Re: [PATCH][2/2] RTAS MSI

[Michael Ellerman]

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

On Mon, 2006-07-31 at 16:01 -0500, Jake Moilanen wrote:
> Here's version 3 which addresses all of Michael's comments.

Sorry, more questions :)

> Index: 2.6-msi/drivers/pci/msi-rtas.c
> ===================================================================
> --- /dev/null
> +++ 2.6-msi/drivers/pci/msi-rtas.c
> @@ -0,0 +1,99 @@
> +/*
> + * Jake Moilanen <moilanen@austin.ibm.com>
> + * Copyright (C) 2006 IBM
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2 of the
> + * License.
> + *
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/irq.h>
> +#include <asm/rtas.h>
> +#include <asm/hw_irq.h>
> +#include <asm/ppc-pci.h>
> +
> +int rtas_enable_msi(struct pci_dev* pdev)
> +{
> +	int seq_num = 1;
> +	int i;
> +	int rc;
> +	int query_token = rtas_token("ibm,query-interrupt-source-number");
> +	int ret[2];
> +	int n_intr;
> +	int last_virq = NO_IRQ;
> +	int virq;
> +	unsigned int addr;
> +	unsigned long buid;
> +	struct device_node * dn;
> +
> +	dn = pci_device_to_OF_node(pdev);
> +
> +	if (!of_find_property(dn, "ibm,req#msi", NULL))
> +		return -ENOENT;
> +
> +	buid = get_phb_buid(dn->parent);
> +	addr = (pdev->bus->number << 16) | (pdev->devfn << 8);
> +
> +	do {
> +		rc = rtas_call(rtas_token("ibm,change-msi"), 6, 3, ret, addr,
> +			       buid >> 32, buid & 0xffffffff,
> +			       0, 0, seq_num);
> +
> +		seq_num = ret[1];
> +	} while (rtas_busy_delay(rc));
> +
> +	if (rc)	{
> +		printk(KERN_WARNING "error[%d]: getting the number of "
> +		       "MSI interrupts for %s\n", rc, dn->name);
> +		return -EIO;
> +	}
> +
> +	/* Return if there's no MSI interrupts */
> +	if (!ret[0])
> +		return -ENOENT;
> +
> +	n_intr = ret[0];
> +
> +	for (i = 0; i < n_intr; i++) {
> +		do {
> +			rc = rtas_call(query_token, 4, 3, ret, addr,
> +				       buid >> 32, buid & 0xffffffff, i);
> +		} while (rtas_busy_delay(rc));
> +
> +		if (!rc) {
> +			virq = irq_create_mapping(irq_find_host(dn), ret[0],
> +						ret[1] ? IRQ_TYPE_EDGE_RISING :
> +						IRQ_TYPE_LEVEL_LOW);

I'm only just starting to get benh's new irq code, but I think
irq_find_host(dn) isn't doing what we want here. It's probably harmless,
but AFAICT irq_find_host() is only meant to be called when you have the
node of the irq controller, not for an arbitrary dn. The doco's a bit
ambiguous:

 * irq_find_host - Locates a host for a given device node
 * @node: device-tree node of the interrupt controller

But looking at the implementation, it doesn't do a search up the tree or
anything, it just checks node against each host.

Also, since's benh's latest patch went in we'll have to split this into
two calls, I think we want:

virq = irq_create_mapping(NULL ???, ret[0]);
set_irq_type(virq, ret[1] ? IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_LOW);

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

Re: [PATCH][2/2] RTAS MSI

[Benjamin Herrenschmidt]

> I'm only just starting to get benh's new irq code, but I think
> irq_find_host(dn) isn't doing what we want here. It's probably harmless,
> but AFAICT irq_find_host() is only meant to be called when you have the
> node of the irq controller, not for an arbitrary dn. The doco's a bit
> ambiguous:
> 
>  * irq_find_host - Locates a host for a given device node
>  * @node: device-tree node of the interrupt controller
> 
> But looking at the implementation, it doesn't do a search up the tree or
> anything, it just checks node against each host.

For pSeries, passing NULL is fine for host anyway as there is only one
domain that is relevant for MSIs (there might be a 8259 legacy domain
too but it's not relevant) and that domain is set to be the default
host.

> Also, since's benh's latest patch went in we'll have to split this into
> two calls, I think we want:
> 
> virq = irq_create_mapping(NULL ???, ret[0]);
> set_irq_type(virq, ret[1] ? IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_LOW);

MSIs are always edge (though there might be an issue with some P5IOC
errata lurking here...). The xics code doesn't care much at this point
though.

Ben.

Re: [PATCH][2/2] RTAS MSI

[Michael Ellerman]

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

On Wed, 2006-08-09 at 11:50 +0200, Benjamin Herrenschmidt wrote:
> > I'm only just starting to get benh's new irq code, but I think
> > irq_find_host(dn) isn't doing what we want here. It's probably harmless,
> > but AFAICT irq_find_host() is only meant to be called when you have the
> > node of the irq controller, not for an arbitrary dn. The doco's a bit
> > ambiguous:
> > 
> >  * irq_find_host - Locates a host for a given device node
> >  * @node: device-tree node of the interrupt controller
> > 
> > But looking at the implementation, it doesn't do a search up the tree or
> > anything, it just checks node against each host.
> 
> For pSeries, passing NULL is fine for host anyway as there is only one
> domain that is relevant for MSIs (there might be a 8259 legacy domain
> too but it's not relevant) and that domain is set to be the default
> host.

OK, will fix it. In the long run we probably want a function that takes
any dn and finds the host for it.

> 
> > Also, since's benh's latest patch went in we'll have to split this into
> > two calls, I think we want:
> > 
> > virq = irq_create_mapping(NULL ???, ret[0]);
> > set_irq_type(virq, ret[1] ? IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_LOW);
> 
> MSIs are always edge (though there might be an issue with some P5IOC
> errata lurking here...). The xics code doesn't care much at this point
> though.

Err, great. Anymore info? I'll just set it to EDGE for now.

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

Re: [PATCH][2/2] RTAS MSI

[Benjamin Herrenschmidt]

On Thu, 2006-08-10 at 18:03 +1000, Michael Ellerman wrote:

> OK, will fix it. In the long run we probably want a function that takes
> any dn and finds the host for it.

That's what the OF irq parsing functions do...  You can't just walk up
the device-tree for interrupts, you have to walk up the interrupt
tree...

> Err, great. Anymore info? I'll just set it to EDGE for now.

Or just don't call set_irq_type()... xics doesn't care.

Ben.

Re: [PATCH][0/2] RTAS MSI

[Benjamin Herrenschmidt]

On Thu, 2006-07-27 at 13:15 -0500, Jake Moilanen wrote:
> Here is a rebased RTAS MSI which uses the IRQ layer rewrite.  Please try
> getting it into 2.6.18.  
> 
> I mistakenly forgot to export the MSI symbols.  So modules weren't too
> happy...
> 
> The RTAS patch skips the intel-centric MSI layer and uses
> pci_enable/disable_msi() calls directly.  It does not correctly handle
> multi-vector MSI either.  

Multi-vector MSIs aren't handled by the linux API anyway it seems (the
doc says pci_enable_msi() only ever enables one MSI) so that's fine if
you don't handle them :) The word on the street is that multivector MSIs
aren't useful, MSI-X are.

Ben.

Re: [PATCH][0/2] RTAS MSI

[Segher Boessenkool]

>> The RTAS patch skips the intel-centric MSI layer and uses
>> pci_enable/disable_msi() calls directly.  It does not correctly  
>> handle
>> multi-vector MSI either.
>
> Multi-vector MSIs aren't handled by the linux API anyway it seems (the
> doc says pci_enable_msi() only ever enables one MSI) so that's fine if
> you don't handle them :) The word on the street is that multivector  
> MSIs
> aren't useful, MSI-X are.

pci_enable_msi() should always enable exactly one or zero MSIs.  Maybe
Jake's patch doesn't follow this rule, and that is what he alluded to?


Segher

Re: [PATCH][0/2] RTAS MSI

[Jake Moilanen]

On Fri, 2006-07-28 at 20:43 +0200, Segher Boessenkool wrote:
> >> The RTAS patch skips the intel-centric MSI layer and uses
> >> pci_enable/disable_msi() calls directly.  It does not correctly  
> >> handle
> >> multi-vector MSI either.
> >
> > Multi-vector MSIs aren't handled by the linux API anyway it seems (the
> > doc says pci_enable_msi() only ever enables one MSI) so that's fine if
> > you don't handle them :) The word on the street is that multivector  
> > MSIs
> > aren't useful, MSI-X are.
> 
> pci_enable_msi() should always enable exactly one or zero MSIs.  Maybe
> Jake's patch doesn't follow this rule, and that is what he alluded to?

Sorry, I was less-than-clear.  I was going to use this same framework
for MSI-X, and I was saying that it is not being correctly handled at
this point.

Jake

Re: [PATCH][0/2] RTAS MSI

[Segher Boessenkool]

> Sorry, I was less-than-clear.  I was going to use this same framework
> for MSI-X, and I was saying that it is not being correctly handled at
> this point.

Well it's _correct_ -- pci_enable_msix() always returns failure, that's
fine from a correctness perspective (although suboptimal from a  
performance
or functionality perspective, heh).


Segher

Re: [PATCH][0/2] RTAS MSI

[Michael Ellerman]

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

On Fri, 2006-07-28 at 20:43 +0200, Segher Boessenkool wrote:
> >> The RTAS patch skips the intel-centric MSI layer and uses
> >> pci_enable/disable_msi() calls directly.  It does not correctly  
> >> handle
> >> multi-vector MSI either.
> >
> > Multi-vector MSIs aren't handled by the linux API anyway it seems (the
> > doc says pci_enable_msi() only ever enables one MSI) so that's fine if
> > you don't handle them :) The word on the street is that multivector  
> > MSIs
> > aren't useful, MSI-X are.
> 
> pci_enable_msi() should always enable exactly one or zero MSIs.  Maybe
> Jake's patch doesn't follow this rule, and that is what he alluded to?

I was just re-reading this thread and this got me thinking. I think the
current code does violate this rule if firmware has allocated more than
one MSI to the device.

In rtas_enable_msi() we ask firmware how many MSIs the device has been
given (by firmware), we then return one to the driver, but leave any
extras configured.

So this might lead to the state where the card has been configured to
use x MSIs, but we only tell the driver about 1 of them. I don't know
enough PCI to grok if that's going to be a problem.

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

Re: [PATCH][0/2] RTAS MSI

[Segher Boessenkool]

> I was just re-reading this thread and this got me thinking. I think  
> the
> current code does violate this rule if firmware has allocated more  
> than
> one MSI to the device.
>
> In rtas_enable_msi() we ask firmware how many MSIs the device has been
> given (by firmware), we then return one to the driver, but leave any
> extras configured.
>
> So this might lead to the state where the card has been configured to
> use x MSIs, but we only tell the driver about 1 of them. I don't know
> enough PCI to grok if that's going to be a problem.

A device could in principle happily start using all MSIs it has been
assigned as soon as its global interrupt enable bit is set.  So lying
to the driver about what MSIs are enabled can in principle cause nasty
problems.

In reality however, on any (recent) device, every interrupt cause also
has its own enable bits that the driver needs to set.  So we're sort-of
safe here.

Eventually, we should change the API for pci_enable_msi() so that it
can enable multiple MSIs as well; an arch implementation can always
choose to do just one.  This lets us roll the APIs for MSI and MSI-X
into one as well btw -- always a good thing!


Segher

Re: [PATCH][0/2] RTAS MSI

[Michael Ellerman]

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

On Wed, 2006-08-09 at 11:52 +0200, Segher Boessenkool wrote:
> > I was just re-reading this thread and this got me thinking. I think  
> > the
> > current code does violate this rule if firmware has allocated more  
> > than
> > one MSI to the device.
> >
> > In rtas_enable_msi() we ask firmware how many MSIs the device has been
> > given (by firmware), we then return one to the driver, but leave any
> > extras configured.
> >
> > So this might lead to the state where the card has been configured to
> > use x MSIs, but we only tell the driver about 1 of them. I don't know
> > enough PCI to grok if that's going to be a problem.
> 
> A device could in principle happily start using all MSIs it has been
> assigned as soon as its global interrupt enable bit is set.  So lying
> to the driver about what MSIs are enabled can in principle cause nasty
> problems.
> 
> In reality however, on any (recent) device, every interrupt cause also
> has its own enable bits that the driver needs to set.  So we're sort-of
> safe here.

Yeah ok, I thought that was probably the answer - it's possible, but
probably not a problem IRL.

> Eventually, we should change the API for pci_enable_msi() so that it
> can enable multiple MSIs as well; an arch implementation can always
> choose to do just one.  This lets us roll the APIs for MSI and MSI-X
> into one as well btw -- always a good thing!

Yeah. Looking at the way drivers use the current API (and there's only a
few), they generally seem to try to enable n MSI-X vectors, then
fallback to a single MSI then LSI. And then there's some that just want
a single MSI as a drop-in replacement for LSI.

So I think we could have pci_enable_msi(dev), which would enable a
single MSI _or_ MSI-X vector, depending on what's available. That'd be
used by drivers that just want a simple replacement for LSI. And then
pci_enable_multi_msi(dev, num_irqs) which would give the driver num_irqs
MSI or MSI-X vectors.

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

Re: [PATCH][0/2] RTAS MSI

[Benjamin Herrenschmidt]

> So I think we could have pci_enable_msi(dev), which would enable a
> single MSI _or_ MSI-X vector, depending on what's available. That'd be
> used by drivers that just want a simple replacement for LSI. And then
> pci_enable_multi_msi(dev, num_irqs) which would give the driver num_irqs
> MSI or MSI-X vectors.

You cannot lie and have pci_enable_msi() enable an MSI-X vector. Some
cards need additional tweaking when enabling MSI or MSI-X and if the
system enables MSI-X while the driver thinks it's MSI, bad things might
happen.

pci_enable_msi() should call the firmware to reconfigure for only one
MSI and enable just that. MSI-X is the only really sexy thing anyway
(that and a way to spread MSI-X accross CPUs from the kernel but that's
another topic)

Ben.

Re: [PATCH][0/2] RTAS MSI

[Michael Ellerman]

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

On Wed, 2006-08-09 at 17:41 +0200, Benjamin Herrenschmidt wrote:
> > So I think we could have pci_enable_msi(dev), which would enable a
> > single MSI _or_ MSI-X vector, depending on what's available. That'd be
> > used by drivers that just want a simple replacement for LSI. And then
> > pci_enable_multi_msi(dev, num_irqs) which would give the driver num_irqs
> > MSI or MSI-X vectors.
> 
> You cannot lie and have pci_enable_msi() enable an MSI-X vector. Some
> cards need additional tweaking when enabling MSI or MSI-X and if the
> system enables MSI-X while the driver thinks it's MSI, bad things might
> happen.

Hmm. As I read the PAPR the firmware calls may do just that (pp 122).
ie. it doesn't differentiate between MSIs and MSI-Xs as far as I can
tell. So if we implement pci_enable_msi() via the RTAS calls we might be
violating that constraint.

> pci_enable_msi() should call the firmware to reconfigure for only one
> MSI and enable just that. MSI-X is the only really sexy thing anyway
> (that and a way to spread MSI-X accross CPUs from the kernel but that's
> another topic)

And the current implementation doesn't do that either, so we should fix
it to only allocate 1 MSI, regardless of what firmware has set.

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

Re: [PATCH][0/2] RTAS MSI

[Benjamin Herrenschmidt]

> Hmm. As I read the PAPR the firmware calls may do just that (pp 122).
> ie. it doesn't differentiate between MSIs and MSI-Xs as far as I can
> tell. So if we implement pci_enable_msi() via the RTAS calls we might be
> violating that constraint.

It's pretty screwed up .... I don't know what is a proper way out there.

> > pci_enable_msi() should call the firmware to reconfigure for only one
> > MSI and enable just that. MSI-X is the only really sexy thing anyway
> > (that and a way to spread MSI-X accross CPUs from the kernel but that's
> > another topic)
> 
> And the current implementation doesn't do that either, so we should fix
> it to only allocate 1 MSI, regardless of what firmware has set.

Yes.

Ben.

Re: [PATCH][0/2] RTAS MSI

[Benjamin Herrenschmidt]

> So this might lead to the state where the card has been configured to
> use x MSIs, but we only tell the driver about 1 of them. I don't know
> enough PCI to grok if that's going to be a problem.

It probably will

Ben.