RTAS MSI

Forgot to export symbols for MSI.

Signed-off-by Jake Moilanen <[email protected]>

Index: 2.6-msi/drivers/pci/Makefile
===================================================================

drivers/pci/Makefile +0 -0

@@ -27,9 +27,12 @@ obj-$(CONFIG_PPC64) += setup-bus.o
 obj-$(CONFIG_MIPS) += setup-bus.o setup-irq.o

drivers/pci/Makefile +1 -0

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

#

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

drivers/pci/Makefile obj-$(CONFIG_PPC64) += setup-bus.o · line 33

Quoted 2 lines

drivers/pci/Makefile +2 -0

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

Quoted 13 lines

Index: 2.6-msi/drivers/pci/Makefile
===================================================================

drivers/pci/Makefile +2 -3

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

drivers/pci/Makefile obj-$(CONFIG_PPC64) += setup-bus.o · line 33

Quoted 2 lines

drivers/pci/Makefile +2 -0

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

drivers/pci/Makefile obj-$(CONFIG_PPC64) += setup-bus.o · line 34

Quoted 1 line

drivers/pci/Makefile +1 -0

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

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

drivers/pci/Makefile obj-$(CONFIG_PPC64) += setup-bus.o · line 38

Quoted 4 lines

drivers/pci/Makefile +1 -0

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

Quoted 1 line

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

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

Quoted 1 line

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

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

Quoted 4 lines

Quoted 1 line

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

Quoted 2 lines

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

Quoted 3 lines

Quoted 1 line

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.

drivers/pci/Makefile obj-$(CONFIG_PPC64) += setup-bus.o · line 33

Quoted 7 lines · Jake Moilanen

drivers/pci/Makefile +4 -3

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

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.

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

drivers/pci/msi-rtas.c new file +10 -0

@@ -0,0 +1,111 @@
+/*
+ * Jake Moilanen <[email protected]>
+ * 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.
+ *
+ */

drivers/pci/msi-rtas.c line 20

Quoted 4 lines

drivers/pci/msi-rtas.c new file +3 -0

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

drivers/pci/msi-rtas.c line 28

Quoted 2 lines

drivers/pci/msi-rtas.c new file +2 -0

+	int reglen;
+	int ret[3];

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

drivers/pci/msi-rtas.c line 34

Quoted 6 lines

drivers/pci/msi-rtas.c new file +6 -0

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

drivers/pci/msi-rtas.c line 40

Quoted 6 lines

drivers/pci/msi-rtas.c new file +6 -0

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

drivers/pci/msi-rtas.c line 50

Quoted 10 lines

drivers/pci/msi-rtas.c new file +10 -0

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

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

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.

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

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

drivers/pci/msi-rtas.c new file +59 -0

@@ -0,0 +1,99 @@
+/*
+ * Jake Moilanen <[email protected]>
+ * 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];
+

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

Quoted 9 lines

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

Quoted 2 lines

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.

Quoted 6 lines

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.

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

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

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.

Quoted 3 lines

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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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!

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.

Quoted 5 lines

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)

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.

Quoted 4 lines

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.

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.