[RFC/PATCH 0/7] Powerpc MSI Implementation

[RFC/PATCH 0/7] Powerpc MSI Implementation

[Michael Ellerman]

Another cut at a Powerpc MSI implementation. This is working with the MPIC
backend, still haven't tested the RTAS backend.

Things I remember changing since last time:
 * Fixed a few bugs pointed out by Jake.
 * We discover the MPIC magic address via the PCI config space rather than
   hard coding it.
 * We reuse pdev->irq in the MPIC backend for now. This is nasty but works.
 * Moved msi_info into the pci_dev. This is ugly, but not sure where else
   we can put it, pci_dn is no longer an option.
 * Added a place holder msi_info->msix_base pointer.
 * Still haven't implement raw MSIX enabled, todo.
 * Removed the msi_info->priv. No on was using it, can always put it back.
 * Added lots of debugging in anticipation of getting some firware to test on.
 ...

cheers

[RFC/PATCH 1/7] Add #defines for Hypertransport MSI fields

[Michael Ellerman]

Add a few #defines for grabbing and working with the address fields
in a HT_CAPTYPE_MSI_MAPPING capability. All from the HT spec v3.00.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

 include/linux/pci_regs.h |    7 +++++++
 1 file changed, 7 insertions(+)

Index: msi/include/linux/pci_regs.h
===================================================================
--- msi.orig/include/linux/pci_regs.h
+++ msi/include/linux/pci_regs.h
@@ -480,6 +480,13 @@
 #define HT_CAPTYPE_UNITID_CLUMP	0x90	/* Unit ID clumping */
 #define HT_CAPTYPE_EXTCONF	0x98	/* Extended Configuration Space Access */
 #define HT_CAPTYPE_MSI_MAPPING	0xA8	/* MSI Mapping Capability */
+#define  HT_MSI_FLAGS		0x02		/* Offset to flags */
+#define  HT_MSI_FLAGS_ENABLE	0x1		/* Mapping enable */
+#define  HT_MSI_FLAGS_FIXED	0x2		/* Fixed mapping only */
+#define  HT_MSI_FIXED_ADDR	0x0000000FEE0000ULL	/* Fixed addr */
+#define  HT_MSI_ADDR_LO		0x04		/* Offset to low addr bits */
+#define  HT_MSI_ADDR_LO_MASK	0xFFF00000	/* Low address bit mask */
+#define  HT_MSI_ADDR_HI		0x08		/* Offset to high addr bits */
 #define HT_CAPTYPE_DIRECT_ROUTE	0xB0	/* Direct routing configuration */
 #define HT_CAPTYPE_VCSET	0xB8	/* Virtual Channel configuration */
 #define HT_CAPTYPE_ERROR_RETRY	0xC0	/* Retry on error configuration */

[RFC/PATCH 2/7] Make some MSI-X #defines generic

[Michael Ellerman]

Move some MSI-X #defines into pci_regs.h so they can be used
outside of drivers/pci.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

 drivers/pci/msi.h        |    8 --------
 include/linux/pci_regs.h |    6 ++++++
 2 files changed, 6 insertions(+), 8 deletions(-)

Index: msi/drivers/pci/msi.h
===================================================================
--- msi.orig/drivers/pci/msi.h
+++ msi/drivers/pci/msi.h
@@ -6,14 +6,6 @@
 #ifndef MSI_H
 #define MSI_H
 
-/*
- * MSI-X Address Register
- */
-#define PCI_MSIX_FLAGS_QSIZE		0x7FF
-#define PCI_MSIX_FLAGS_ENABLE		(1 << 15)
-#define PCI_MSIX_FLAGS_BIRMASK		(7 << 0)
-#define PCI_MSIX_FLAGS_BITMASK		(1 << 0)
-
 #define PCI_MSIX_ENTRY_SIZE			16
 #define  PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET	0
 #define  PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET	4
Index: msi/include/linux/pci_regs.h
===================================================================
--- msi.orig/include/linux/pci_regs.h
+++ msi/include/linux/pci_regs.h
@@ -292,6 +292,12 @@
 #define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
 #define PCI_MSI_MASK_BIT	16	/* Mask bits register */
 
+/* MSI-X registers (these are at offset PCI_MSI_FLAGS) */
+#define PCI_MSIX_FLAGS_QSIZE	0x7FF
+#define PCI_MSIX_FLAGS_ENABLE	(1 << 15)
+#define PCI_MSIX_FLAGS_BIRMASK	(7 << 0)
+#define PCI_MSIX_FLAGS_BITMASK	(1 << 0)
+
 /* CompactPCI Hotswap Register */
 
 #define PCI_CHSWP_CSR		2	/* Control and Status Register */

[RFC/PATCH 3/7] Rip out the existing powerpc msi stubs

[Michael Ellerman]

Rip out the existing powerpc msi stubs. These were the start of an
implementation based on ppc_md calls, but were never used.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

 arch/powerpc/kernel/irq.c     |   28 ----------------------------
 include/asm-powerpc/machdep.h |    5 -----
 2 files changed, 33 deletions(-)

Index: msi/arch/powerpc/kernel/irq.c
===================================================================
--- msi.orig/arch/powerpc/kernel/irq.c
+++ msi/arch/powerpc/kernel/irq.c
@@ -871,34 +871,6 @@ arch_initcall(irq_late_init);
 
 #endif /* CONFIG_PPC_MERGE */
 
-#ifdef CONFIG_PCI_MSI
-int pci_enable_msi(struct pci_dev * pdev)
-{
-	if (ppc_md.enable_msi)
-		return ppc_md.enable_msi(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;}
-void pci_disable_msix(struct pci_dev *dev) {}
-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
-
 #ifdef CONFIG_PPC64
 static int __init setup_noirqdistrib(char *str)
 {
Index: msi/include/asm-powerpc/machdep.h
===================================================================
--- msi.orig/include/asm-powerpc/machdep.h
+++ msi/include/asm-powerpc/machdep.h
@@ -239,11 +239,6 @@ struct machdep_calls {
 	 */
 	void (*machine_kexec)(struct kimage *image);
 #endif /* CONFIG_KEXEC */
-
-#ifdef CONFIG_PCI_MSI
-	int (*enable_msi)(struct pci_dev *pdev);
-	void (*disable_msi)(struct pci_dev *pdev);
-#endif /* CONFIG_PCI_MSI */
 };
 
 extern void power4_idle(void);

[RFC/PATCH 4/7] Powerpc MSI implementation

[Michael Ellerman]

Powerpc MSI implementation, based on a collection of "ops" callbacks.
We have to take the ops approach to accomodate RTAS, where firmware
handles almost all details of MSI setup/teardown. Bare-metal MSI
can be accomodated also.

We currently (ab)use the pci_dev to store our msi_info structure. We
were hoping to stash it in the pci_dn, but that's not a goer, so the
pci_dev seems like the right place, even though it seems naughty to
bloat generic structs.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

 arch/powerpc/kernel/msi.c     |  316 ++++++++++++++++++++++++++++++++++++++++++
 include/asm-powerpc/machdep.h |    6 
 include/asm-powerpc/msi.h     |  173 ++++++++++++++++++++++
 include/linux/pci.h           |    7 
 4 files changed, 502 insertions(+)

Index: msi/arch/powerpc/kernel/msi.c
===================================================================
--- /dev/null
+++ msi/arch/powerpc/kernel/msi.c
@@ -0,0 +1,316 @@
+/*
+ * Copyright 2006 (C), Michael Ellerman, IBM Corporation.
+ *
+ * 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; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#undef DEBUG
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <asm/msi.h>
+#include <asm/machdep.h>
+
+static struct ppc_msi_ops *get_msi_ops(struct pci_dev *pdev)
+{
+	if (ppc_md.get_msi_ops)
+		return ppc_md.get_msi_ops(pdev);
+
+	return NULL;
+}
+
+/* Activated by pci=nomsi on the command line. */
+static int no_msi;
+
+void pci_no_msi(void)
+{
+	msi_debug("MSI disabled by user\n");
+	no_msi = 1;
+}
+
+
+/* msi_info helpers */
+
+static int alloc_msi_info(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type)
+{
+	struct msi_info *info;
+	unsigned int entries_size;
+
+	entries_size = sizeof(struct msix_entry) * num;
+
+	info = kzalloc(sizeof(struct msi_info) + entries_size, GFP_KERNEL);
+	if (!info) {
+		msi_debug("kzalloc failed for %s\n", pci_name(pdev));
+		return -ENOMEM;
+	}
+
+	info->type = type;
+	info->num = num;
+	info->entries = (struct msix_entry *)(info + 1);
+	memcpy(info->entries, entries, entries_size);
+
+	BUG_ON(pdev->msi_info); /* don't leak info structs */
+	pdev->msi_info = info;
+
+	return 0;
+}
+
+static struct msi_info *get_msi_info(struct pci_dev *pdev)
+{
+	return pdev->msi_info;
+}
+
+static void free_msi_info(struct pci_dev *pdev)
+{
+	kfree(pdev->msi_info);
+	pdev->msi_info = NULL;
+}
+
+
+/* Generic helpers */
+
+static int generic_msi_enable(struct pci_dev *pdev, int nvec,
+				struct msix_entry *entries, int type)
+{
+	struct ppc_msi_ops *ops;
+	int i, rc;
+
+	if (no_msi || !pdev || !entries || !nvec || get_msi_info(pdev)) {
+		msi_debug("predcondition failed for %p\n", pdev);
+		return -EINVAL;
+	}
+
+	ops = get_msi_ops(pdev);
+	if (!ops) {
+		msi_debug("no ops for %s\n", pci_name(pdev));
+		return -EINVAL;
+	}
+
+	for (i = 0; i < nvec; i++)
+		entries[i].vector = NO_IRQ;
+
+	rc = ops->check(pdev, nvec, entries, type);
+	if (rc) {
+		msi_debug("check failed (%d) for %s\n", rc, pci_name(pdev));
+		return rc;
+	}
+
+	rc = ops->alloc(pdev, nvec, entries, type);
+	if (rc) {
+		msi_debug("alloc failed (%d) for %s\n", rc, pci_name(pdev));
+		return rc;
+	}
+
+	if (ops->enable) {
+		rc = ops->enable(pdev, nvec, entries, type);
+		if (rc) {
+			msi_debug("enable failed (%d) for %s\n", rc,
+				pci_name(pdev));
+			goto out_free;
+		}
+	}
+
+	rc = alloc_msi_info(pdev, nvec, entries, type);
+	if (rc)
+		goto out_free;
+
+	return 0;
+
+ out_free:
+	ops->free(pdev, nvec, entries, type);
+
+	return rc;
+}
+
+static int generic_msi_disable(struct pci_dev *pdev, int type)
+{
+	struct ppc_msi_ops *ops;
+	struct msi_info *info;
+
+	if (no_msi || !pdev) {
+		msi_debug("predcondition failed for %p\n", pdev);
+		return -1;
+	}
+
+	info = get_msi_info(pdev);
+	if (!info) {
+		msi_debug("No info for %s\n", pci_name(pdev));
+		return -1;
+	}
+
+	ops = get_msi_ops(pdev);
+	if (!ops) {
+		msi_debug("no ops for %s\n", pci_name(pdev));
+		return -1;
+	}
+
+	if (ops->disable)
+		ops->disable(pdev, info->num, info->entries, type);
+
+	ops->free(pdev, info->num, info->entries, type);
+
+	return 0;
+}
+
+
+/* MSI */
+
+int pci_enable_msi(struct pci_dev *pdev)
+{
+	struct msix_entry entry;
+	int rc;
+
+	entry.entry = 0;
+
+	rc = generic_msi_enable(pdev, 1, &entry, PCI_CAP_ID_MSI);
+	if (rc)
+		return rc;
+
+	get_msi_info(pdev)->saved_irq = pdev->irq;
+	pdev->irq = entry.vector;
+	pdev->msi_enabled = 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_enable_msi);
+
+void pci_disable_msi(struct pci_dev *pdev)
+{
+	if (generic_msi_disable(pdev, PCI_CAP_ID_MSI) != 0)
+		return;
+
+	pdev->irq = get_msi_info(pdev)->saved_irq;
+	free_msi_info(pdev);
+	pdev->msi_enabled = 0;
+}
+EXPORT_SYMBOL_GPL(pci_disable_msi);
+
+
+/* MSI-X */
+
+int pci_enable_msix(struct pci_dev *pdev, struct msix_entry *entries, int nvec)
+{
+	int rc;
+
+	rc = generic_msi_enable(pdev, nvec, entries, PCI_CAP_ID_MSIX);
+	if (rc)
+		return rc;
+
+	pdev->msix_enabled = 1;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_enable_msix);
+
+void pci_disable_msix(struct pci_dev *pdev)
+{
+	if (generic_msi_disable(pdev, PCI_CAP_ID_MSIX) != 0)
+		return;
+
+	free_msi_info(pdev);
+	pdev->msix_enabled = 0;
+}
+EXPORT_SYMBOL_GPL(pci_disable_msix);
+
+
+/* Stubs for now */
+
+void disable_msi_mode(struct pci_dev *dev, int pos, int type)
+{
+	return;
+}
+
+void pci_scan_msi_device(struct pci_dev *dev)
+{
+	return;
+}
+
+void msi_remove_pci_irq_vectors(struct pci_dev* dev)
+{
+	return;
+}
+
+
+/* Bare metal enable/disable */
+
+int msi_raw_enable(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type)
+{
+	struct ppc_msi_ops *ops;
+	struct msi_msg msg;
+	int pos;
+	u16 control;
+
+	pos = pci_find_capability(pdev, type);
+	if (!pos) {
+		msi_debug("cap (%d) not found for %s\n", type, pci_name(pdev));
+		return -1;
+	}
+
+	ops = get_msi_ops(pdev);
+	BUG_ON(!ops);
+
+	pci_read_config_word(pdev, pos + PCI_MSI_FLAGS, &control);
+
+	switch (type) {
+	case PCI_CAP_ID_MSI:
+		BUG_ON(!ops->setup_msi_msg);
+
+		ops->setup_msi_msg(pdev, &entries[0], &msg, type);
+
+		pci_write_config_dword(pdev, pos + PCI_MSI_ADDRESS_LO,
+			msg.address_lo);
+
+		if (control & PCI_MSI_FLAGS_64BIT) {
+			pci_write_config_dword(pdev, pos + PCI_MSI_ADDRESS_HI,
+						msg.address_hi);
+			pci_write_config_dword(pdev, pos + PCI_MSI_DATA_64,
+						msg.data);
+		} else {
+			pci_write_config_dword(pdev, pos + PCI_MSI_DATA_32,
+						msg.data);
+		}
+
+		control |= PCI_MSI_FLAGS_ENABLE;
+		break;
+	case PCI_CAP_ID_MSIX:
+		WARN_ON(1); /* XXX implement me */
+		return -1;
+	default:
+		BUG();
+	}
+
+	pci_write_config_word(pdev, pos + PCI_MSI_FLAGS, control);
+
+	return 0;
+}
+
+void msi_raw_disable(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type)
+{
+	int pos;
+	u16 control;
+
+	pos = pci_find_capability(pdev, type);
+	BUG_ON(!pos);
+
+	pci_read_config_word(pdev, pos + PCI_MSI_FLAGS, &control);
+
+	switch (type) {
+	case PCI_CAP_ID_MSI:
+		control &= ~PCI_MSI_FLAGS_ENABLE;
+		break;
+	case PCI_CAP_ID_MSIX:
+		control &= ~PCI_MSIX_FLAGS_ENABLE;
+		break;
+	default:
+		BUG();
+	}
+
+	pci_write_config_word(pdev, pos + PCI_MSI_FLAGS, control);
+
+	return;
+}
Index: msi/include/asm-powerpc/machdep.h
===================================================================
--- msi.orig/include/asm-powerpc/machdep.h
+++ msi/include/asm-powerpc/machdep.h
@@ -29,6 +29,9 @@ struct file;
 #ifdef CONFIG_KEXEC
 struct kimage;
 #endif
+#ifdef CONFIG_PCI_MSI
+struct ppc_msi_ops;
+#endif
 
 #ifdef CONFIG_SMP
 struct smp_ops_t {
@@ -106,6 +109,9 @@ struct machdep_calls {
 	/* Called after scanning the bus, before allocating resources */
 	void		(*pcibios_fixup)(void);
 	int		(*pci_probe_mode)(struct pci_bus *);
+#ifdef CONFIG_PCI_MSI
+	struct ppc_msi_ops*	(*get_msi_ops)(struct pci_dev *pdev);
+#endif
 
 	void		(*restart)(char *cmd);
 	void		(*power_off)(void);
Index: msi/include/asm-powerpc/msi.h
===================================================================
--- /dev/null
+++ msi/include/asm-powerpc/msi.h
@@ -0,0 +1,173 @@
+/*
+ * Copyright (C) 2006 Michael Ellerman, IBM Corporation.
+ *
+ * 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; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _ASM_POWERPC_MSI_H
+#define _ASM_POWERPC_MSI_H
+
+#ifdef __KERNEL__
+#ifndef __ASSEMBLY__
+
+#include <linux/pci.h>
+#include <linux/msi.h>
+
+/*
+ * MSI and MSI-X although different in some details, are also similar in
+ * many respects, and ultimately achieve the same end. Given that, this code
+ * tries as far as possible to implement both MSI and MSI-X with a minimum
+ * of code duplication. We will use "MSI" to refer to both MSI and MSI-X,
+ * except where it is important to differentiate between the two.
+ *
+ * Enabling MSI for a device can be broken down into:
+ *  1) Checking the device can support the type/number of MSIs requested.
+ *  2) Allocating irqs for the MSIs and setting up the irq_descs.
+ *  3) Writing the appropriate configuration to the device and enabling MSIs.
+ *
+ * To implement that we have the following callbacks:
+ *  1) check(pdev, num, msix_entries, type)
+ *  2) alloc(pdev, num, msix_entries, type)
+ *  3) enable(pdev, num, msix_entries, type)
+ *	a) setup_msi_msg(pdev, msix_entry, msi_msg, type)
+ *
+ * We give platforms full control over the enable step. However many
+ * platforms will simply want to program the device using standard PCI
+ * accessors. These platforms can use a generic enable callback and define
+ * a setup_msi_msg() callback which simply fills in the "magic" address and
+ * data values. Other platforms may leave setup_msi_msg() empty.
+ *
+ * Disabling MSI requires:
+ *  1) Disabling MSI on the device.
+ *  2) Freeing the irqs and any associated accounting information.
+ *
+ * Which maps directly to the two callbacks:
+ *  1) disable(pdev, num, msix_entries, type)
+ *  2) free(pdev, num, msix_entries, type)
+ */
+
+struct ppc_msi_ops
+{
+	/* check - Check that the requested MSI allocation is OK.
+	 *
+	 * @pdev:	PCI device structure.
+	 * @num:	The number of MSIs being requested.
+	 * @entries:	An array of @num msix_entry structures.
+	 * @type:	The type, MSI or MSI-X.
+	 *
+	 * This routine is responsible for checking that the given PCI device
+	 * can be allocated the requested type and number of MSIs.
+	 *
+	 * It is up to this routine to determine if the requested number of
+	 * MSIs is valid for the device in question. If the number of MSIs,
+	 * or the particular MSI entries, can not be supported for any
+	 * reason this routine must return non-zero.
+	 *
+	 * If the check is succesful this routine must return 0.
+	 */
+	int (*check) (struct pci_dev *pdev, int num,
+				struct msix_entry *entries, int type);
+
+	/* alloc - Allocate MSIs for the given device.
+	 *
+	 * @pdev:	PCI device structure.
+	 * @num:	The number of MSIs being requested.
+	 * @entries:	An array of @num msix_entry structures.
+	 * @type:	The type, MSI or MSI-X.
+	 *
+	 * This routine is responsible for allocating the number of
+	 * MSIs to the given PCI device.
+	 *
+	 * Upon completion there must be @num MSIs assigned to this device,
+	 * the "vector" member of each struct msix_entry must be filled in
+	 * with the Linux irq number allocated to it. The corresponding
+	 * irq_descs must also be setup with an appropriate handler if
+	 * required.
+	 *
+	 * If the allocation completes succesfully this routine must return 0.
+	 */
+	int (*alloc) (struct pci_dev *pdev, int num,
+				struct msix_entry *entries, int type);
+
+	/* enable - Enable the MSIs on the given device.
+	 *
+	 * @pdev:	PCI device structure.
+	 * @num:	The number of MSIs being requested.
+	 * @entries:	An array of @num msix_entry structures.
+	 * @type:	The type, MSI or MSI-X.
+	 *
+	 * This routine enables the MSIs on the given PCI device.
+	 *
+	 * If the enable completes succesfully this routine must return 0.
+	 *
+	 * This callback is optional.
+	 */
+	int (*enable) (struct pci_dev *pdev, int num,
+				struct msix_entry *entries, int type);
+
+	/* setup_msi_msg - Setup an MSI message for the given device.
+	 *
+	 * @pdev:	PCI device structure.
+	 * @entry:	The MSI entry to create a msi_msg for.
+	 * @msg:	Written with the magic address and data.
+	 * @type:	The type, MSI or MSI-X.
+	 *
+	 * Returns the "magic address and data" used to trigger the msi.
+	 * If the setup is succesful this routine must return 0.
+	 *
+	 * This callback is optional.
+	 */
+	int (*setup_msi_msg) (struct pci_dev *pdev, struct msix_entry *entry,
+				struct msi_msg *msg, int type);
+
+	/* disable - disable the MSI for the given device.
+	 *
+	 * @pdev:	PCI device structure.
+	 * @num:	The number of MSIs to disable.
+	 * @entries:	An array of @num msix_entry structures.
+	 * @type:	The type, MSI or MSI-X.
+	 *
+         * This routine should perform the inverse of enable.
+	 */
+	void (*disable) (struct pci_dev *pdev, int num,
+				struct msix_entry *entries, int type);
+
+	/* free - free the MSIs assigned to the device.
+	 *
+	 * @pdev:	PCI device structure.
+	 * @num:	The number of MSIs.
+	 * @entries:	An array of @num msix_entry structures.
+	 * @type:	The type, MSI or MSI-X.
+	 *
+	 * Free all MSIs and associated resources for the device. If any
+	 * MSIs have been enabled they will have been disabled already by
+	 * the generic code.
+	 */
+	void (*free) (struct pci_dev *pdev, int num,
+				struct msix_entry *entries, int type);
+};
+
+
+/* Used by the MSI code to track MSI info for a pci_dev */
+struct msi_info {
+	int type;
+	unsigned int saved_irq;
+	unsigned int num;
+	struct msix_entry *entries;
+	void __iomem *msix_base;
+};
+
+extern int msi_raw_enable(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type);
+extern void msi_raw_disable(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type);
+
+#define msi_debug(fmt, args...)	\
+	pr_debug("MSI:%s:%d: " fmt, __FUNCTION__, __LINE__, ## args)
+
+#endif /* __ASSEMBLY__ */
+#endif /* __KERNEL__ */
+#endif /* _ASM_POWERPC_MSI_H */
Index: msi/include/linux/pci.h
===================================================================
--- msi.orig/include/linux/pci.h
+++ msi/include/linux/pci.h
@@ -106,6 +106,10 @@ struct pci_cap_saved_state {
 	u32 data[0];
 };
 
+#if defined(CONFIG_PCI_MSI) && defined(CONFIG_PPC_MERGE)
+struct msi_info;
+#endif
+
 /*
  * The pci_dev structure is used to describe PCI devices.
  */
@@ -173,6 +177,9 @@ struct pci_dev {
 	struct bin_attribute *rom_attr; /* attribute descriptor for sysfs ROM entry */
 	int rom_attr_enabled;		/* has display of the rom attribute been enabled? */
 	struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file for resources */
+#if defined(CONFIG_PCI_MSI) && defined(CONFIG_PPC_MERGE)
+	struct	msi_info *msi_info;
+#endif
 };
 
 #define pci_dev_g(n) list_entry(n, struct pci_dev, global_list)

[RFC/PATCH 5/7] RTAS MSI implementation

[Michael Ellerman]

Powerpc MSI support via RTAS. Based on Jake's code.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

 arch/powerpc/kernel/msi-rtas.c |  265 +++++++++++++++++++++++++++++++++++++++++
 include/asm-powerpc/msi.h      |    6 
 2 files changed, 271 insertions(+)

Index: msi/arch/powerpc/kernel/msi-rtas.c
===================================================================
--- /dev/null
+++ msi/arch/powerpc/kernel/msi-rtas.c
@@ -0,0 +1,265 @@
+/*
+ * Copyright (C) 2006 Jake Moilanen <moilanen@austin.ibm.com>, IBM Corp.
+ * Copyright (C) 2006 Michael Ellerman, IBM Corp.
+ *
+ * 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.
+ *
+ */
+
+#undef DEBUG
+
+#include <linux/irq.h>
+#include <asm/msi.h>
+#include <asm/rtas.h>
+#include <asm/hw_irq.h>
+#include <asm/ppc-pci.h>
+
+static int query_token, change_token;
+
+#define RTAS_QUERY_MSI_FN	0
+#define RTAS_CHANGE_MSI_FN	1
+#define RTAS_RESET_MSI_FN	2
+
+
+static struct pci_dn *get_pdn(struct pci_dev *pdev)
+{
+	struct device_node *dn;
+	struct pci_dn *pdn;
+
+	dn = pci_device_to_OF_node(pdev);
+	if (!dn) {
+		msi_debug("No OF device node for %s\n", pci_name(pdev));
+		return NULL;
+	}
+
+	pdn = PCI_DN(dn);
+	if (!pdn) {
+		msi_debug("No PCI DN for %s\n", pci_name(pdev));
+		return NULL;
+	}
+
+	return pdn;
+}
+
+/* RTAS Helpers */
+
+static int rtas_change_msi(struct pci_dn *pdn, u32 function, u32 num_irqs)
+{
+	u32 addr, seq_num, rtas_ret[2];
+	unsigned long buid;
+	int rc;
+
+	addr = rtas_config_addr(pdn->busno, pdn->devfn, 0);
+	buid = pdn->phb->buid;
+
+	seq_num = 1;
+	do {
+		rc = rtas_call(change_token, 6, 3, rtas_ret, addr,
+				BUID_HI(buid), BUID_LO(buid),
+				function, num_irqs, seq_num);
+
+		seq_num = rtas_ret[1];
+	} while (rtas_busy_delay(rc));
+
+	if (rc) {
+		msi_debug("error (%d) for %s\n", rc, pci_name(pdn->pcidev));
+		return rc;
+	}
+
+	return rtas_ret[0];
+}
+
+static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
+{
+	u32 addr, rtas_ret[2];
+	unsigned long buid;
+	int rc;
+
+	addr = rtas_config_addr(pdn->busno, pdn->devfn, 0);
+	buid = pdn->phb->buid;
+
+	do {
+		rc = rtas_call(query_token, 4, 3, rtas_ret, addr,
+			       BUID_HI(buid), BUID_LO(buid), offset);
+	} while (rtas_busy_delay(rc));
+
+	if (rc) {
+		msi_debug("error (%d) querying source number for %s\n",
+				rc, pci_name(pdn->pcidev));
+		return rc;
+	}
+
+	return rtas_ret[0];
+}
+
+/*
+ * The spec gives firmware the option to enable either MSI or MSI-X,
+ * this doesn't wash with the Linux API. For the time beinging, we
+ * kludge around that by checking ourselves the right type is enabled.
+ */
+static int check_msi_type(struct pci_dev *pdev, int type)
+{
+	int pos, msi_enabled, msix_enabled;
+	u16 reg;
+
+	pos = pci_find_capability(pdev, PCI_CAP_ID_MSI);
+	if (!pos) {
+		msi_debug("cap (%d) not found for %s\n", type, pci_name(pdev));
+		return -1;
+	}
+
+	pci_read_config_word(pdev, pos + PCI_MSI_FLAGS, &reg);
+
+	msi_enabled = msix_enabled = 0;
+
+	if (reg & PCI_MSI_FLAGS_ENABLE)
+		msi_enabled = 1;
+
+	if (reg & PCI_MSIX_FLAGS_ENABLE)
+		msix_enabled = 1;
+
+	if (type == PCI_CAP_ID_MSI && (msix_enabled || !msi_enabled)) {
+		msi_debug("Expected MSI but got %s.\n",
+				msix_enabled ? "MSI-X" : "none");
+		return -1;
+	}
+
+	if (type == PCI_CAP_ID_MSIX && (msi_enabled || !msix_enabled)) {
+		msi_debug("Expected MSI-X but got %s.\n",
+				msi_enabled ? "MSI" : "none");
+		return -1;
+	}
+
+	return 0;
+}
+
+static void msi_rtas_free(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type)
+{
+	struct pci_dn *pdn;
+	int i;
+
+	pdn = get_pdn(pdev);
+	if (!pdn)
+		return;
+
+	for (i = 0; i < num; i++) {
+		irq_dispose_mapping(entries[i].vector);
+	}
+
+	if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
+		msi_debug("Setting MSIs to 0 failed!\n");
+		BUG();
+	}
+}
+
+static int msi_rtas_check(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type)
+{
+	struct device_node *dn;
+	int i;
+
+	/* We must have a pci_dn for the RTAS code. */
+	if (!get_pdn(pdev))
+		return -1;
+
+	dn = pci_device_to_OF_node(pdev);
+	if (!of_find_property(dn, "ibm,req#msi", NULL)) {
+		msi_debug("No ibm,req#msi for %s\n", pci_name(pdev));
+		return -1;
+	}
+
+	/*
+	 * Firmware gives us no control over which entries are allocated
+	 * for MSI-X, it seems to assume we want 0 - n. For now just insist
+	 * that the entries array entry members are 0 - n.
+	 */
+	for (i = 0; i < num; i++) {
+		if (entries[i].entry != i) {
+			msi_debug("entries[%d].entry (%d) != %d\n", i
+					entries[i].entry, i);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
+static int msi_rtas_alloc(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type)
+{
+	struct pci_dn *pdn;
+	int hwirq, virq, i;
+
+	pdn = get_pdn(pdev);
+	if (!pdn)
+		return -1;
+
+	/*
+	 * In the case of an error it's not clear whether the device is left
+	 * with MSI enabled or not, I think we should explicitly disable.
+	 */
+	if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, num) != num) {
+		msi_debug("rtas_change_msi() failed for %s\n", pci_name(pdev));
+		goto out_free;
+	}
+
+	if (check_msi_type(pdev, type))
+		goto out_free;
+
+	for (i = 0; i < num; i++) {
+		hwirq = rtas_query_irq_number(pdn, i);
+		if (hwirq < 0) {
+			msi_debug("error (%d) getting hwirq for %s\n",
+					hwirq, pci_name(pdev));
+			goto out_free;
+		}
+
+		virq = irq_create_mapping(NULL, hwirq);
+
+		if (virq == NO_IRQ) {
+			msi_debug("Failed mapping hwirq %d\n", hwirq);
+			goto out_free;
+		}
+
+		entries[i].vector = virq;
+	}
+
+	return 0;
+
+ out_free:
+	msi_rtas_free(pdev, num, entries, type);
+	return -1;
+}
+
+static struct ppc_msi_ops rtas_msi_ops = {
+	.check = msi_rtas_check,
+	.alloc = msi_rtas_alloc,
+	.free  = msi_rtas_free
+};
+
+static struct ppc_msi_ops *rtas_get_msi_ops(struct pci_dev *pdev)
+{
+	return &rtas_msi_ops;
+}
+
+int msi_rtas_init(void)
+{
+	query_token  = rtas_token("ibm,query-interrupt-source-number");
+	change_token = rtas_token("ibm,change-msi");
+
+	if ((query_token == RTAS_UNKNOWN_SERVICE) ||
+			(change_token == RTAS_UNKNOWN_SERVICE)) {
+		msi_debug("Couldn't find RTAS tokens, no MSI support.\n");
+		return -1;
+	}
+
+	msi_debug("Registering RTAS MSI ops.\n");
+
+	ppc_md.get_msi_ops = rtas_get_msi_ops;
+
+	return 0;
+}
Index: msi/include/asm-powerpc/msi.h
===================================================================
--- msi.orig/include/asm-powerpc/msi.h
+++ msi/include/asm-powerpc/msi.h
@@ -168,6 +168,12 @@ extern void msi_raw_disable(struct pci_d
 #define msi_debug(fmt, args...)	\
 	pr_debug("MSI:%s:%d: " fmt, __FUNCTION__, __LINE__, ## args)
 
+#ifdef CONFIG_PCI_MSI
+extern int msi_rtas_init(void);
+#else
+static inline int msi_rtas_init(void) { return -1; };
+#endif
+
 #endif /* __ASSEMBLY__ */
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_MSI_H */

[RFC/PATCH 6/7] MPIC MSI backend

[Michael Ellerman]

MPIC MSI backend. Based on code from Segher, heavily hacked by me.
This version discovers the magic address by reading the config space.
Still slightly hacky in that we reuse pdev->irq, pending an irq
allocator for MPIC.

Tested, succesfully getting MSIs from the tg3 via HT/PCI-X on a JS21
running SLOF.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

 arch/powerpc/kernel/msi-mpic.c |  138 +++++++++++++++++++++++++++++++++++++++++
 include/asm-powerpc/msi.h      |    2 
 2 files changed, 140 insertions(+)

Index: msi/arch/powerpc/kernel/msi-mpic.c
===================================================================
--- /dev/null
+++ msi/arch/powerpc/kernel/msi-mpic.c
@@ -0,0 +1,138 @@
+/*
+ * Copyright (C) 2006 Segher Boessenkool, IBM Corp.
+ * Copyright (C) 2006 Michael Ellerman, IBM Corp.
+ *
+ * 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.
+ *
+ */
+
+#undef DEBUG
+
+#include <linux/irq.h>
+#include <asm/msi.h>
+#include <asm/hw_irq.h>
+#include <asm/ppc-pci.h>
+
+static unsigned int find_ht_msi_capability(struct pci_dev *pdev)
+{
+	u8 subcap;
+	unsigned int pos = pci_find_capability(pdev, PCI_CAP_ID_HT);
+
+	while (pos) {
+		pci_read_config_byte(pdev, pos + HT_SUBCAP_OFFSET, &subcap);
+		if (subcap == HT_CAPTYPE_MSI_MAPPING)
+			return pos;
+		pos = pci_find_next_capability(pdev, pos, PCI_CAP_ID_HT);
+	}
+
+	return 0;
+}
+
+static u64 read_ht_magic_addr(struct pci_dev *pdev, unsigned int pos)
+{
+	u8 flags;
+	u32 tmp;
+	u64 addr;
+
+	pci_read_config_byte(pdev, pos + HT_MSI_FLAGS, &flags);
+
+	if (flags & HT_MSI_FLAGS_FIXED)
+		return HT_MSI_FIXED_ADDR;
+
+	pci_read_config_dword(pdev, pos + HT_MSI_ADDR_LO, &tmp);
+	addr = tmp & HT_MSI_ADDR_LO_MASK;
+	pci_read_config_dword(pdev, pos + HT_MSI_ADDR_HI, &tmp);
+	addr = addr | ((u64)tmp << 32);
+
+	return addr;
+}
+
+static u64 find_ht_magic_addr(struct pci_dev *pdev)
+{
+	struct pci_bus *bus;
+	unsigned int pos;
+
+	for (bus = pdev->bus; bus; bus = bus->parent) {
+		pos = find_ht_msi_capability(bus->self);
+		if (pos)
+			return read_ht_magic_addr(bus->self, pos);
+	}
+
+	return 0;
+}
+
+static int msi_mpic_check(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type)
+{
+	/* We need an irq allocator before we can support multi-MSI & MSI-X */
+	if (num != 1 || type == PCI_CAP_ID_MSIX) {
+		msi_debug("precondition failure for %s\n", pci_name(pdev));
+		return 1;
+	}
+
+	/* If we can't find a magic address then MSI ain't gonna work */
+	if (find_ht_magic_addr(pdev) == 0) {
+		msi_debug("no magic address found for %s\n", pci_name(pdev));
+		return 1;
+	}
+
+	return 0;
+}
+
+static void msi_mpic_free(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type)
+{
+	/* We borrowed the LSI irq, so don't unmap it! */
+	return;
+}
+
+static int msi_mpic_alloc(struct pci_dev *pdev, int num,
+			struct msix_entry *entries, int type)
+{
+	/* For now we reuse the assigned LSI, this is a hack. */
+	set_irq_type(pdev->irq, IRQ_TYPE_EDGE_RISING);
+	entries[0].vector = pdev->irq;
+
+	return 0;
+}
+
+static int msi_mpic_setup_msi_msg(struct pci_dev *pdev,
+		struct msix_entry *entry, struct msi_msg *msg, int type)
+{
+	u64 addr;
+
+	addr = find_ht_magic_addr(pdev);
+	msg->address_lo = addr & 0xFFFFFFFF;
+	msg->address_hi = addr >> 32;
+	msg->data = irq_map[pdev->irq].hwirq;
+
+	msi_debug("allocated irq %d at 0x%lx for %s\n", pdev->irq,
+			addr, pci_name(pdev));
+
+	return 0;
+}
+
+static struct ppc_msi_ops mpic_msi_ops = {
+	.check = msi_mpic_check,
+	.alloc = msi_mpic_alloc,
+	.free = msi_mpic_free,
+	.enable = msi_raw_enable,
+	.disable = msi_raw_disable,
+	.setup_msi_msg = msi_mpic_setup_msi_msg,
+};
+
+static struct ppc_msi_ops *mpic_get_msi_ops(struct pci_dev *pdev)
+{
+	return &mpic_msi_ops;
+}
+
+int msi_mpic_init(void)
+{
+	pr_debug("mpic_msi_init: Registering MPIC MSI ops.\n");
+	ppc_md.get_msi_ops = mpic_get_msi_ops;
+
+	return 0;
+}
Index: msi/include/asm-powerpc/msi.h
===================================================================
--- msi.orig/include/asm-powerpc/msi.h
+++ msi/include/asm-powerpc/msi.h
@@ -170,8 +170,10 @@ extern void msi_raw_disable(struct pci_d
 
 #ifdef CONFIG_PCI_MSI
 extern int msi_rtas_init(void);
+extern int msi_mpic_init(void);
 #else
 static inline int msi_rtas_init(void) { return -1; };
+static inline int msi_mpic_init(void) { return -1; };
 #endif
 
 #endif /* __ASSEMBLY__ */

[RFC/PATCH 7/7] Enable MSI on Powerpc

[Michael Ellerman]

Allow PCI_MSI to build on Powerpc. Hook up a few platforms to use
the appropriate MSI backend.

We still need CONFIG_POWERPC.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

 arch/powerpc/kernel/Makefile           |    6 ++++++
 arch/powerpc/platforms/maple/setup.c   |    3 +++
 arch/powerpc/platforms/powermac/pic.c  |    3 +++
 arch/powerpc/platforms/pseries/setup.c |    3 +++
 drivers/pci/Kconfig                    |    2 +-
 drivers/pci/Makefile                   |    4 +++-
 6 files changed, 19 insertions(+), 2 deletions(-)

Index: msi/arch/powerpc/kernel/Makefile
===================================================================
--- msi.orig/arch/powerpc/kernel/Makefile
+++ msi/arch/powerpc/kernel/Makefile
@@ -66,6 +66,12 @@ pci64-$(CONFIG_PPC64)		+= pci_64.o pci_d
 				   pci_direct_iommu.o iomap.o
 pci32-$(CONFIG_PPC32)		:= pci_32.o
 obj-$(CONFIG_PCI)		+= $(pci64-y) $(pci32-y)
+
+msiobj-y			:= msi.o
+msiobj-$(CONFIG_PPC_RTAS)	+= msi-rtas.o
+msiobj-$(CONFIG_MPIC)		+= msi-mpic.o
+obj-$(CONFIG_PCI_MSI)		+= $(msiobj-y)
+
 kexec-$(CONFIG_PPC64)		:= machine_kexec_64.o
 kexec-$(CONFIG_PPC32)		:= machine_kexec_32.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o crash.o $(kexec-y)
Index: msi/arch/powerpc/platforms/maple/setup.c
===================================================================
--- msi.orig/arch/powerpc/platforms/maple/setup.c
+++ msi/arch/powerpc/platforms/maple/setup.c
@@ -61,6 +61,7 @@
 #include <asm/lmb.h>
 #include <asm/mpic.h>
 #include <asm/udbg.h>
+#include <asm/msi.h>
 
 #include "maple.h"
 
@@ -275,6 +276,8 @@ static void __init maple_init_IRQ(void)
 	ppc_md.get_irq = mpic_get_irq;
 	of_node_put(mpic_node);
 	of_node_put(root);
+
+	msi_mpic_init();
 }
 
 static void __init maple_progress(char *s, unsigned short hex)
Index: msi/arch/powerpc/platforms/powermac/pic.c
===================================================================
--- msi.orig/arch/powerpc/platforms/powermac/pic.c
+++ msi/arch/powerpc/platforms/powermac/pic.c
@@ -34,6 +34,7 @@
 #include <asm/time.h>
 #include <asm/pmac_feature.h>
 #include <asm/mpic.h>
+#include <asm/msi.h>
 
 #include "pmac.h"
 
@@ -562,6 +563,8 @@ static int __init pmac_pic_probe_mpic(vo
 	set_irq_data(cascade, mpic2);
 	set_irq_chained_handler(cascade, pmac_u3_cascade);
 
+	msi_mpic_init();
+
 	of_node_put(slave);
 	return 0;
 }
Index: msi/arch/powerpc/platforms/pseries/setup.c
===================================================================
--- msi.orig/arch/powerpc/platforms/pseries/setup.c
+++ msi/arch/powerpc/platforms/pseries/setup.c
@@ -65,6 +65,7 @@
 #include <asm/i8259.h>
 #include <asm/udbg.h>
 #include <asm/smp.h>
+#include <asm/msi.h>
 
 #include "plpar_wrappers.h"
 #include "ras.h"
@@ -275,6 +276,7 @@ static void __init pseries_discover_pic(
 #ifdef CONFIG_SMP
 			smp_init_pseries_mpic();
 #endif
+			msi_mpic_init();
 			return;
 		} else if (strstr(typep, "ppc-xicp")) {
 			ppc_md.init_IRQ       = xics_init_IRQ;
@@ -284,6 +286,7 @@ static void __init pseries_discover_pic(
 #ifdef CONFIG_SMP
 			smp_init_pseries_xics();
 #endif
+			msi_rtas_init();
 			return;
 		}
 	}
Index: msi/drivers/pci/Kconfig
===================================================================
--- msi.orig/drivers/pci/Kconfig
+++ 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_MERGE
 	help
 	   This allows device drivers to enable MSI (Message Signaled
 	   Interrupts).  Message Signaled Interrupts enable a device to
Index: msi/drivers/pci/Makefile
===================================================================
--- msi.orig/drivers/pci/Makefile
+++ msi/drivers/pci/Makefile
@@ -14,8 +14,10 @@ obj-$(CONFIG_HOTPLUG) += hotplug.o
 # Build the PCI Hotplug drivers if we were asked to
 obj-$(CONFIG_HOTPLUG_PCI) += hotplug/
 
-# Build the PCI MSI interrupt support
+# Build the PCI MSI interrupt support, but not for arch/powerpc
+ifndef CONFIG_PPC_MERGE
 obj-$(CONFIG_PCI_MSI) += msi.o
+endif
 
 # Build the Hypertransport interrupt support
 obj-$(CONFIG_HT_IRQ) += htirq.o

Re: [RFC/PATCH 0/7] Powerpc MSI Implementation

[Benjamin Herrenschmidt]

>  * Moved msi_info into the pci_dev. This is ugly, but not sure where else
>    we can put it, pci_dn is no longer an option.

It's only ugly because it's currently powerpc specific (and thus needs
an ifdef). It's the right way to go in the long run for the generic
code. 

Now the question is wether Eric is interested in our work and thus
wether it's worth trying to port the intel & altix MSI stuff to our
infrastructure, in which case there is no more reason not to add the
msi_info to pci_dev without any ifdef (just define it as an empty struct
if CONFIG_PCI_MSI is not set).

Cheers,
Ben.

Re: [RFC/PATCH 1/7] Add #defines for Hypertransport MSI fields

[Segher Boessenkool]

> +#define  HT_MSI_FIXED_ADDR	0x0000000FEE0000ULL	/* Fixed addr */

You forgot a zero at the end (and one on front, but that doesn't
matter all that much ;-) ).


Segher

Re: [RFC/PATCH 0/7] Powerpc MSI Implementation

[Greg KH]

On Tue, Nov 07, 2006 at 06:41:16PM +1100, Benjamin Herrenschmidt wrote:
> 
> >  * Moved msi_info into the pci_dev. This is ugly, but not sure where else
> >    we can put it, pci_dn is no longer an option.
> 
> It's only ugly because it's currently powerpc specific (and thus needs
> an ifdef). It's the right way to go in the long run for the generic
> code. 
> 
> Now the question is wether Eric is interested in our work and thus
> wether it's worth trying to port the intel & altix MSI stuff to our
> infrastructure, in which case there is no more reason not to add the
> msi_info to pci_dev without any ifdef (just define it as an empty struct
> if CONFIG_PCI_MSI is not set).

I have no objection to taking this core change, and I do think the intel
and altix stuff should be moved over to this new model.

Oh, and I think your first two patches can be applied now to the tree
(header file changes).  Any objection to me doing this?

thanks,

greg k-h

Re: [RFC/PATCH 6/7] MPIC MSI backend

[Segher Boessenkool]

Looks lovely, thanks Michael!

> +static unsigned int find_ht_msi_capability(struct pci_dev *pdev)
> +{
> +	u8 subcap;
> +	unsigned int pos = pci_find_capability(pdev, PCI_CAP_ID_HT);
> +
> +	while (pos) {
> +		pci_read_config_byte(pdev, pos + HT_SUBCAP_OFFSET, &subcap);
> +		if (subcap == HT_CAPTYPE_MSI_MAPPING)
> +			return pos;
> +		pos = pci_find_next_capability(pdev, pos, PCI_CAP_ID_HT);
> +	}
> +
> +	return 0;
> +}

It would be nice to have a generic find_ht_capability(pdev, type).

> +static void msi_mpic_free(struct pci_dev *pdev, int num,
> +			struct msix_entry *entries, int type)
> +{
> +	/* We borrowed the LSI irq, so don't unmap it! */
> +	return;
> +}
> +
> +static int msi_mpic_alloc(struct pci_dev *pdev, int num,
> +			struct msix_entry *entries, int type)
> +{
> +	/* For now we reuse the assigned LSI, this is a hack. */
> +	set_irq_type(pdev->irq, IRQ_TYPE_EDGE_RISING);
> +	entries[0].vector = pdev->irq;
> +
> +	return 0;
> +}

If msi_mpic_alloc() changes the sense/polarity, msi_mpic_free()
should set it back.  This doesn't actually matter on CPC925/945,
as LSI interrupts are rising edge as well (on the MPIC); maybe
the sense/polarity should be configurable per MPIC controller
somewhere (or just mark the code as "FIXME: only correct for
some MPICs").

> ++static int msi_mpic_setup_msi_msg(struct pci_dev *pdev,
> +		struct msix_entry *entry, struct msi_msg *msg, int type)
> +{
> +	u64 addr;
> +
> +	addr = find_ht_magic_addr(pdev);
> +	msg->address_lo = addr & 0xFFFFFFFF;
> +	msg->address_hi = addr >> 32;

You don't seem the check whether the "magic address" is outside
of 32-bit range and the device can do 32-bit only?


Segher

Re: [RFC/PATCH 6/7] MPIC MSI backend

[Benjamin Herrenschmidt]

> It would be nice to have a generic find_ht_capability(pdev, type).

Agreed, though I prefer the naming to be ht_find_capability().

> If msi_mpic_alloc() changes the sense/polarity, msi_mpic_free()
> should set it back.  This doesn't actually matter on CPC925/945,
> as LSI interrupts are rising edge as well (on the MPIC); maybe
> the sense/polarity should be configurable per MPIC controller
> somewhere (or just mark the code as "FIXME: only correct for
> some MPICs").

Yes, I will do a cleanup pass on the MPIC code there. I think it should
only program the chip for polarity/sense in startup() and thus can
"override" the native LSI setting of that vector when the interrupt is
flagged as MSI without actually losing the original information.

Right now, as you noticed, it's a non-issue for MSIs coming off HT, as
both HT interrupts and MSIs are programmed to be edge in the MPIC
itself, however, this is not ok with interrupts from the PCIe slot fow
which, according to the device-tree on my Quad G5, the LSI is level low.

Something else I'd like to do is add a generic IRQF_MSI purely for the
sake of displaying "MSI" instead of "Edge" in /proc/interrupts :-)
Though it might be useful for some backends to track what interrupts
have been configured as MSI too.
  
> > ++static int msi_mpic_setup_msi_msg(struct pci_dev *pdev,
> > +		struct msix_entry *entry, struct msi_msg *msg, int type)
> > +{
> > +	u64 addr;
> > +
> > +	addr = find_ht_magic_addr(pdev);
> > +	msg->address_lo = addr & 0xFFFFFFFF;
> > +	msg->address_hi = addr >> 32;
> 
> You don't seem the check whether the "magic address" is outside
> of 32-bit range and the device can do 32-bit only?

Good point. It happens not to be on our chipset / firmware but we should
probably check in the check() callback. If it's the case, then we refuse
to enable MSI for that device.

Cheers,
Ben.

Re: [RFC/PATCH 6/7] MPIC MSI backend

[Segher Boessenkool]

> Agreed, though I prefer the naming to be ht_find_capability().

Of course, I just took "_msi" out of the name here ;-)

> Yes, I will do a cleanup pass on the MPIC code there. I think it  
> should
> only program the chip for polarity/sense in startup() and thus can
> "override" the native LSI setting of that vector when the interrupt is
> flagged as MSI without actually losing the original information.

You mean when we (in the future) reserve a range of vectors
for MSI allocation at MPIC startup?  Sure, that works.

> Right now, as you noticed, it's a non-issue for MSIs coming off HT, as
> both HT interrupts and MSIs are programmed to be edge in the MPIC
> itself, however, this is not ok with interrupts from the PCIe slot fow
> which, according to the device-tree on my Quad G5, the LSI is level  
> low.

["The PCIe slot" == the 16x PCIe slot on PowerMacs here, for
the record; it isn't connected over a bridge to HT, but is
directly connected to the CPC945 (U4) chip.]

There are more problems there (with this current code): the LSI
IRQ will always be #3, which cannot be used as an MSI IRQ; and
the MSI address to use can not be found on a parent HT bridge
(as there is none, duh).  That last problem makes the patch
perfectly safe for the CPC945 PCIe port though :-)

> Something else I'd like to do is add a generic IRQF_MSI purely for the
> sake of displaying "MSI" instead of "Edge" in /proc/interrupts :-)

Yes definitely.

> Though it might be useful for some backends to track what interrupts
> have been configured as MSI too.

That, too.

>>> ++static int msi_mpic_setup_msi_msg(struct pci_dev *pdev,
>>> +		struct msix_entry *entry, struct msi_msg *msg, int type)
>>> +{
>>> +	u64 addr;
>>> +
>>> +	addr = find_ht_magic_addr(pdev);
>>> +	msg->address_lo = addr & 0xFFFFFFFF;
>>> +	msg->address_hi = addr >> 32;
>>
>> You don't seem the check whether the "magic address" is outside
>> of 32-bit range and the device can do 32-bit only?
>
> Good point. It happens not to be on our chipset / firmware but we  
> should
> probably check in the check() callback. If it's the case, then we  
> refuse
> to enable MSI for that device.

Can't we just check here?  If not, what's the point of having
a return code here?  although doing the check in check() might
be conceptually nicer, sure.


Segher

Re: [RFC/PATCH 6/7] MPIC MSI backend

[Benjamin Herrenschmidt]

> > Yes, I will do a cleanup pass on the MPIC code there. I think it  
> > should
> > only program the chip for polarity/sense in startup() and thus can
> > "override" the native LSI setting of that vector when the interrupt is
> > flagged as MSI without actually losing the original information.
> 
> You mean when we (in the future) reserve a range of vectors
> for MSI allocation at MPIC startup?  Sure, that works.

Not only, even with the current code... basically keep the
sense/polarity flags of the LSI intact in the irq_desc, just add the
IRQF_MSI bit to "override" them when we configure the interrupt to be an
MSI. Then, irq_chip->startup() instead of set_irq_type() does the actual
configuration of the MPIC and, seeing the IRQF_MSI flags, does the right
thing.

That way, when restoring the IRQ back to LSI, we just clear IRQF_MSI and
the old sense/polarity settings will still be there in the descriptor
and MPIC will do the right thing on the next startup().

> ["The PCIe slot" == the 16x PCIe slot on PowerMacs here, for
> the record; it isn't connected over a bridge to HT, but is
> directly connected to the CPC945 (U4) chip.]

Yup, the Attu if you prefer :-)

> There are more problems there (with this current code): the LSI
> IRQ will always be #3, which cannot be used as an MSI IRQ;

Are you sure about that ? In this case, we need a special kludge for
now. I though the MSI stuff could override any of the MPIC vectors, but
you might well be right there, it might not work with the U4 internal
ones.

> and the MSI address to use can not be found on a parent HT bridge
> (as there is none, duh).  That last problem makes the patch
> perfectly safe for the CPC945 PCIe port though :-)

Yes, I know :-) We need specific code to discover that a device is
hanging off the Attu instead of HT. I already explained it all to
Michael, it shouldn't be too hard. I even have a card setup to test it,
just didn't have time to do it just yet.

> Can't we just check here?  If not, what's the point of having
> a return code here?  although doing the check in check() might
> be conceptually nicer, sure.

Yes, nicer and avoids going all the way to allocating stuff and then
having to de-allocate.

Ben.

Re: [RFC/PATCH 6/7] MPIC MSI backend

[Segher Boessenkool]

> Not only, even with the current code... basically keep the
> sense/polarity flags of the LSI intact in the irq_desc, just add the
> IRQF_MSI bit to "override" them when we configure the interrupt to  
> be an
> MSI. Then, irq_chip->startup() instead of set_irq_type() does the  
> actual
> configuration of the MPIC and, seeing the IRQF_MSI flags, does the  
> right
> thing.
>
> That way, when restoring the IRQ back to LSI, we just clear  
> IRQF_MSI and
> the old sense/polarity settings will still be there in the descriptor
> and MPIC will do the right thing on the next startup().

Cute trick, and not MPIC (or even PowerPC) specific either :-)

> Are you sure about that ?

Yes.  "Legacy PCI" INTx interrupts are send as "virtual wire"
interrupts over PCIe; Attu asserts interrupt #3 on Kodiak's
MPIC if any of INTA..INTD is hot (I have no idea how to
distinguish between those four, though -- and that seems to be
pretty important, heh).

> In this case, we need a special kludge for
> now. I though the MSI stuff could override any of the MPIC vectors,  
> but
> you might well be right there, it might not work with the U4 internal
> ones.

U4 interrupts 0..7 from HT (or PCIe) are not decoded on U4;
0..3 on U3 (which supports MSI-via-HT just fine).

> Yes, I know :-) We need specific code to discover that a device is
> hanging off the Attu instead of HT. I already explained it all to
> Michael, it shouldn't be too hard. I even have a card setup to test  
> it,
> just didn't have time to do it just yet.

I never tested Attu interrupts at all (just the MSI write
port), glad _someone_ is testing it :-)


Segher

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Matthew Wilcox]

On Tue, Nov 07, 2006 at 06:21:23PM +1100, Michael Ellerman wrote:
> We currently (ab)use the pci_dev to store our msi_info structure. We
> were hoping to stash it in the pci_dn, but that's not a goer, so the
> pci_dev seems like the right place, even though it seems naughty to
> bloat generic structs.

We have the per-irq void *chip_data; could this be the right place to
keep it instead?  That way, it won't take up space in the pci_dev for
devices which don't use MSI.

Or do you rely on mpic_from_irq() working for MSI interrupts too?

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Russell King]

On Tue, Nov 07, 2006 at 01:07:30PM -0700, Matthew Wilcox wrote:
> On Tue, Nov 07, 2006 at 06:21:23PM +1100, Michael Ellerman wrote:
> > We currently (ab)use the pci_dev to store our msi_info structure. We
> > were hoping to stash it in the pci_dn, but that's not a goer, so the
> > pci_dev seems like the right place, even though it seems naughty to
> > bloat generic structs.
> 
> We have the per-irq void *chip_data; could this be the right place to
> keep it instead?  That way, it won't take up space in the pci_dev for
> devices which don't use MSI.

Bah.  chip_data is supposed to be __iomem.  I bet if you build ARM
with sparse it'll kick out lots of warnings as a result of that loss.

Grumble, generic irq, pah.  Grumble.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Benjamin Herrenschmidt]

On Tue, 2006-11-07 at 13:07 -0700, Matthew Wilcox wrote:
> On Tue, Nov 07, 2006 at 06:21:23PM +1100, Michael Ellerman wrote:
> > We currently (ab)use the pci_dev to store our msi_info structure. We
> > were hoping to stash it in the pci_dn, but that's not a goer, so the
> > pci_dev seems like the right place, even though it seems naughty to
> > bloat generic structs.
> 
> We have the per-irq void *chip_data; could this be the right place to
> keep it instead?  That way, it won't take up space in the pci_dev for
> devices which don't use MSI.
> 
> Or do you rely on mpic_from_irq() working for MSI interrupts too?

Yes, we rely on that.

The chip_data is for use by the IRQ controller. In our cases, for some
controllers at least, MSIs are just an add-on to an existing controller
(they toggle existing inputs).

In general, I think it just make sense to have pci_dev contain MSI
tracking data for the device though.

Cheers,
Ben.

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Benjamin Herrenschmidt]

On Tue, 2006-11-07 at 20:14 +0000, Russell King wrote:
> On Tue, Nov 07, 2006 at 01:07:30PM -0700, Matthew Wilcox wrote:
> > On Tue, Nov 07, 2006 at 06:21:23PM +1100, Michael Ellerman wrote:
> > > We currently (ab)use the pci_dev to store our msi_info structure. We
> > > were hoping to stash it in the pci_dn, but that's not a goer, so the
> > > pci_dev seems like the right place, even though it seems naughty to
> > > bloat generic structs.
> > 
> > We have the per-irq void *chip_data; could this be the right place to
> > keep it instead?  That way, it won't take up space in the pci_dev for
> > devices which don't use MSI.
> 
> Bah.  chip_data is supposed to be __iomem.  I bet if you build ARM
> with sparse it'll kick out lots of warnings as a result of that loss.
> 
> Grumble, generic irq, pah.  Grumble.

Oh well... on various cases, on ppc, I use it not as __iomem but as a
pointer to a irq controller "instance" (which itself contains the
__iomem pointer, along with a bunch of other things).

But yeah, you were there first ;)

Ben.

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Matthew Wilcox]

On Tue, Nov 07, 2006 at 08:14:36PM +0000, Russell King wrote:
> On Tue, Nov 07, 2006 at 01:07:30PM -0700, Matthew Wilcox wrote:
> > We have the per-irq void *chip_data; could this be the right place to
> > keep it instead?  That way, it won't take up space in the pci_dev for
> > devices which don't use MSI.
> 
> Bah.  chip_data is supposed to be __iomem.  I bet if you build ARM
> with sparse it'll kick out lots of warnings as a result of that loss.

Erm, since when?  When I introduced it (back in January 2005 [1]), it
was called handler_data and pointed to a struct which is chip-type
dependent.  One of the items in that struct is an IO address, but we
need more than that.

Now we have something else called handler_data, and I must
admit to being quite confused what the difference is.  The comment in
the header files and the DocBook genericirq leave me quite confused.

[1] http://git.kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commit;h=1dd15c35b6bc179cd9c2a47e13360052c1e938a3

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Russell King]

On Tue, Nov 07, 2006 at 01:44:32PM -0700, Matthew Wilcox wrote:
> On Tue, Nov 07, 2006 at 08:14:36PM +0000, Russell King wrote:
> > On Tue, Nov 07, 2006 at 01:07:30PM -0700, Matthew Wilcox wrote:
> > > We have the per-irq void *chip_data; could this be the right place to
> > > keep it instead?  That way, it won't take up space in the pci_dev for
> > > devices which don't use MSI.
> > 
> > Bah.  chip_data is supposed to be __iomem.  I bet if you build ARM
> > with sparse it'll kick out lots of warnings as a result of that loss.
> 
> Erm, since when?  When I introduced it (back in January 2005 [1]), it
> was called handler_data and pointed to a struct which is chip-type
> dependent.

Since before the generic irq merge.  If I was more expert with git
I'd post a URL, but I'm not so I won't.  But I'm sure you can find
it - look at the history of include/asm-arm/mach/irq.h.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Matthew Wilcox]

On Tue, Nov 07, 2006 at 08:48:53PM +0000, Russell King wrote:
> On Tue, Nov 07, 2006 at 01:44:32PM -0700, Matthew Wilcox wrote:
> > On Tue, Nov 07, 2006 at 08:14:36PM +0000, Russell King wrote:
> > > Bah.  chip_data is supposed to be __iomem.  I bet if you build ARM
> > > with sparse it'll kick out lots of warnings as a result of that loss.
> > 
> > Erm, since when?  When I introduced it (back in January 2005 [1]), it
> > was called handler_data and pointed to a struct which is chip-type
> > dependent.
> 
> Since before the generic irq merge.  If I was more expert with git
> I'd post a URL, but I'm not so I won't.  But I'm sure you can find
> it - look at the history of include/asm-arm/mach/irq.h.

OK.  Looks like the first mention of this is in 
4a2581a080098ca3a0c4e416d7a282e96c75ebf8 from July 2006
which is signed-off by you, Ingo Molnar and Thomas Gleixner:

-       void __iomem    *base;
[...]
+#define set_irq_chipdata(irq, d)       set_irq_chip_data(irq, d)
+#define get_irq_chipdata(irq)          get_irq_chip_data(irq)
-#define set_irq_chipdata(irq,d)                        do { irq_desc[irq].base = d; } while (0)
-#define get_irq_chipdata(irq)                  (irq_desc[irq].base)

Now, true, the __iomem has disappeared.  But there was never an __iomem
on chip_data, nor on the handler_data before it.  It went away back in
July when you signed off on the conversion to use the generic irq
handler, and apparently haven't noticed the problem since then, so I
don't see it as being a big problem.

As other architectures, you could embed the iomem pointer in a
struct (would it be useful to you to have more than an iomem pointer
in your handlers?), or you could put the cast adding __iomem in
get_irq_chipdata().

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Russell King]

On Tue, Nov 07, 2006 at 02:02:02PM -0700, Matthew Wilcox wrote:
> On Tue, Nov 07, 2006 at 08:48:53PM +0000, Russell King wrote:
> > On Tue, Nov 07, 2006 at 01:44:32PM -0700, Matthew Wilcox wrote:
> > > On Tue, Nov 07, 2006 at 08:14:36PM +0000, Russell King wrote:
> > > > Bah.  chip_data is supposed to be __iomem.  I bet if you build ARM
> > > > with sparse it'll kick out lots of warnings as a result of that loss.
> > > 
> > > Erm, since when?  When I introduced it (back in January 2005 [1]), it
> > > was called handler_data and pointed to a struct which is chip-type
> > > dependent.
> > 
> > Since before the generic irq merge.  If I was more expert with git
> > I'd post a URL, but I'm not so I won't.  But I'm sure you can find
> > it - look at the history of include/asm-arm/mach/irq.h.
> 
> OK.  Looks like the first mention of this is in 
> 4a2581a080098ca3a0c4e416d7a282e96c75ebf8 from July 2006
> which is signed-off by you, Ingo Molnar and Thomas Gleixner:
> 
> -       void __iomem    *base;
> [...]
> +#define set_irq_chipdata(irq, d)       set_irq_chip_data(irq, d)
> +#define get_irq_chipdata(irq)          get_irq_chip_data(irq)
> -#define set_irq_chipdata(irq,d)                        do { irq_desc[irq].base = d; } while (0)
> -#define get_irq_chipdata(irq)                  (irq_desc[irq].base)

Yes and now I'm regretting having switched ARM over to the generic IRQ
stuff - the old stuff did _precisely_ what we wanted the way we wanted.
None of these compromises.

> As other architectures, you could embed the iomem pointer in a
> struct (would it be useful to you to have more than an iomem pointer
> in your handlers?), or you could put the cast adding __iomem in
> get_irq_chipdata().

That's extremely wasteful - the only way to do that would be to kmalloc
some memory, and that's 32 bytes minimum, just to store 4 bytes of
pointer.  There's no other information which needs to be stored.

I guess we'll just have to add __force casts all over the code. ;(

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Benjamin Herrenschmidt]

> I guess we'll just have to add __force casts all over the code. ;(

Or just do it in a single accessor function/macro...

Ben.

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Eric W. Biederman]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

>> I guess we'll just have to add __force casts all over the code. ;(
>
> Or just do it in a single accessor function/macro...

Or treat struct irq_desc like struct page and have a members that
depend on features that you enable or disable depending on the architecture
dependent tradeoffs.

We already have a few members depend on things like SMP.

With the cache line alignment of each array entry we take up a fair amount of
space, so we don't have to micro optimize just do a decent job of not having
anything to terrible.

For message signalled interrupts and hyptertransport interrupts we probably want
to add an additional field for pointing to their per irq state.  Per device
interrupt controllers are truly peculiar.

Eric

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Benjamin Herrenschmidt]

> For message signalled interrupts and hyptertransport interrupts we probably want
> to add an additional field for pointing to their per irq state.  Per device
> interrupt controllers are truly peculiar.

A per-irq addition field for the MSI data might indeed be useful. I
still want to store it in the pci_dev, but it would be good to have a
pointer to it in the irq_desc.

Ben.
 

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Eric W. Biederman]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

>> For message signalled interrupts and hyptertransport interrupts we probably
> want
>> to add an additional field for pointing to their per irq state.  Per device
>> interrupt controllers are truly peculiar.
>
> A per-irq addition field for the MSI data might indeed be useful. I
> still want to store it in the pci_dev, but it would be good to have a
> pointer to it in the irq_desc.

The msi_info in pci_dev is reasonable.  But storing all 4096 possible
msi_desc's in pci_dev seems a little much.

Hopefully I can look at this a little more this evening and see what we
need to do to harmonize the two msi implementations.

Eric

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Benjamin Herrenschmidt]

On Tue, 2006-11-07 at 18:33 -0700, Eric W. Biederman wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> >> For message signalled interrupts and hyptertransport interrupts we probably
> > want
> >> to add an additional field for pointing to their per irq state.  Per device
> >> interrupt controllers are truly peculiar.
> >
> > A per-irq addition field for the MSI data might indeed be useful. I
> > still want to store it in the pci_dev, but it would be good to have a
> > pointer to it in the irq_desc.
> 
> The msi_info in pci_dev is reasonable.  But storing all 4096 possible
> msi_desc's in pci_dev seems a little much.

My initial idea was that the msi_info structure would have a variable
lenght, that is, would end with msi_desc[0] and would be allocated to
the the right size, but it might suck a bit :-)

> Hopefully I can look at this a little more this evening and see what we
> need to do to harmonize the two msi implementations.

Thanks !

Ben.

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Eric W. Biederman]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> My initial idea was that the msi_info structure would have a variable
> lenght, that is, would end with msi_desc[0] and would be allocated to
> the the right size, but it might suck a bit :-)

That could work.  The whole array of interrupts thing allocated on
device probe, I question a little bit.  I can just about see allocating
another interrupt if you can when a network device start getting another
flow through it.  But it is working well enough at the moment it doesn't
appear time to run out and change the drivers.

>> Hopefully I can look at this a little more this evening and see what we
>> need to do to harmonize the two msi implementations.
>
> Thanks !

I'm still having a hard time wrapping my head around the sanity of not letting
the kernel touch the hardware in the presence of a hypervisor.  How do you cope
with hardware that follows a specification the hypervisor is not aware of?

This seems to be a violation of the principle that the driver knows the hardware
better than some generic layer.

Eric

Re: [RFC/PATCH 4/7] Powerpc MSI implementation

[Benjamin Herrenschmidt]

> I'm still having a hard time wrapping my head around the sanity of not letting
> the kernel touch the hardware in the presence of a hypervisor.  How do you cope
> with hardware that follows a specification the hypervisor is not aware of?

We don't cope :-)

> This seems to be a violation of the principle that the driver knows the hardware
> better than some generic layer.

Well, we got at least the HV folks to change their interfaces so we
could explicitely tell them wether we want MSI or MSI-X ... in the
initial definition of that stuff, we couldn't (the HV did what it
wanted) and we didn't even know what it did (we still don't, but at
least we can now request specifically MSI or MSI-X).

Ben.

Re: [RFC/PATCH 0/7] Powerpc MSI Implementation

[Michael Ellerman]

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

On Tue, 2006-11-07 at 17:02 +0900, Greg KH wrote:
> On Tue, Nov 07, 2006 at 06:41:16PM +1100, Benjamin Herrenschmidt wrote:
> > 
> > >  * Moved msi_info into the pci_dev. This is ugly, but not sure where else
> > >    we can put it, pci_dn is no longer an option.
> > 
> > It's only ugly because it's currently powerpc specific (and thus needs
> > an ifdef). It's the right way to go in the long run for the generic
> > code. 
> > 
> > Now the question is wether Eric is interested in our work and thus
> > wether it's worth trying to port the intel & altix MSI stuff to our
> > infrastructure, in which case there is no more reason not to add the
> > msi_info to pci_dev without any ifdef (just define it as an empty struct
> > if CONFIG_PCI_MSI is not set).
> 
> I have no objection to taking this core change, and I do think the intel
> and altix stuff should be moved over to this new model.

Cool.

> Oh, and I think your first two patches can be applied now to the tree
> (header file changes).  Any objection to me doing this?

There's a trivial bug in 1/7 (HT #defines), so hold back on that one
until I resend. The movement of MSI-X #defines is good to go. I also
sent you the patch to add HT_SUBCAP_OFFSET yesterday that I think is
ready to merge.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

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: 189 bytes --]

Re: [RFC/PATCH 0/7] Powerpc MSI Implementation

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

>> Oh, and I think your first two patches can be applied now to the tree
>> (header file changes).  Any objection to me doing this?
>
> There's a trivial bug in 1/7 (HT #defines), so hold back on that one
> until I resend. The movement of MSI-X #defines is good to go. I also
> sent you the patch to add HT_SUBCAP_OFFSET yesterday that I think is
> ready to merge.

Actually now that I am thinking about it I'm not at all certain about
the HT_SUBCAP_OFFSET patch.  The basic issue is that it suggests that
the field that specifies which type of hypertransport capability has a
fixed number of bits.  While in reality the encoding is variable
length.

Eric

Re: [RFC/PATCH 5/7] RTAS MSI implementation

[Jake Moilanen]

On Tue, 2006-11-07 at 18:21 +1100, Michael Ellerman wrote:
> Powerpc MSI support via RTAS. Based on Jake's code.
> 

Missing the CAS flags to indicate to firmware that we support MSI.

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

Index: git/arch/powerpc/kernel/prom_init.c
===================================================================
--- git.orig/arch/powerpc/kernel/prom_init.c
+++ git/arch/powerpc/kernel/prom_init.c
@@ -634,6 +634,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 supported */
+#else
+#define OV5_MSI			0x00
+#endif
 
 /*
  * The architecture vector has an array of PVR mask/value pairs,
@@ -677,7 +683,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 */

Re: [RFC/PATCH 0/7] Powerpc MSI Implementation

[Michael Ellerman]

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

On Wed, 2006-11-08 at 03:26 -0700, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
> 
> >> Oh, and I think your first two patches can be applied now to the tree
> >> (header file changes).  Any objection to me doing this?
> >
> > There's a trivial bug in 1/7 (HT #defines), so hold back on that one
> > until I resend. The movement of MSI-X #defines is good to go. I also
> > sent you the patch to add HT_SUBCAP_OFFSET yesterday that I think is
> > ready to merge.
> 
> Actually now that I am thinking about it I'm not at all certain about
> the HT_SUBCAP_OFFSET patch.  The basic issue is that it suggests that
> the field that specifies which type of hypertransport capability has a
> fixed number of bits.  While in reality the encoding is variable
> length.

Yeah OK, I did notice that was a bit weird, I had a quick look and
assumed that because all the HT_CAPTYPE's were #defined as byte values
it was OK.

Looking closer most of them are 5 bits, the high 5 bits, and happen to
sit next to reserved fields (which must be zero), so reading the byte
should work in practice. But for a few of them you'll get cruft in the
low bits.

I don't know what they were thinking when they decided to have 3 and 5
bit capability fields, and then specify some of them as being a byte
wide as well. Perhaps the spec committee had a big night out ;)

I was going to write a generic version of pci_find_ht_capability() (as
suggested by Segher), so along with that I'll clean up the #defines to
just be the 3 or 5 bit capability codes, and then have a shift for
getting the capability out of the byte.

Users will still need to know if they're looking for a 3 or 5 bit
capability, but we can encapsulate that in pci_find_ht_capability() and
hopefully most people won't have to see the difference.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

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: 189 bytes --]

Re: [RFC/PATCH 5/7] RTAS MSI implementation

[Michael Ellerman]

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

On Wed, 2006-11-08 at 14:16 -0600, Jake Moilanen wrote:
> On Tue, 2006-11-07 at 18:21 +1100, Michael Ellerman wrote:
> > Powerpc MSI support via RTAS. Based on Jake's code.
> > 
> 
> Missing the CAS flags to indicate to firmware that we support MSI.
> 
> Signed-off-by: Jake Moilanen <moilanen@austin.ibm.com>

Thanks, I'll stick that in the series. Unless Paulus wants to merge it
straight away.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

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: 189 bytes --]

Re: [RFC/PATCH 0/7] Powerpc MSI Implementation

[Segher Boessenkool]

> Looking closer most of them are 5 bits, the high 5 bits, and happen to
> sit next to reserved fields (which must be zero), so reading the byte
> should work in practice. But for a few of them you'll get cruft in the
> low bits.
>
> I don't know what they were thinking when they decided to have 3 and 5
> bit capability fields, and then specify some of them as being a byte
> wide as well. Perhaps the spec committee had a big night out ;)
>
> I was going to write a generic version of pci_find_ht_capability() (as
> suggested by Segher), so along with that I'll clean up the #defines to
> just be the 3 or 5 bit capability codes, and then have a shift for
> getting the capability out of the byte.
>
> Users will still need to know if they're looking for a 3 or 5 bit
> capability, but we can encapsulate that in pci_find_ht_capability()  
> and
> hopefully most people won't have to see the difference.

Keep the defines the full 8 bits and put all the knowledge
about which bits are relevant into pci_find_ht_capability()?
If you do shifting you can get clashes (say, 0b00110 vs. 0b110),
and this would make for simpler code, too.


Segher

Re: [RFC/PATCH 0/7] Powerpc MSI Implementation

[Michael Ellerman]

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

On Thu, 2006-11-09 at 08:36 +0100, Segher Boessenkool wrote:
> > Looking closer most of them are 5 bits, the high 5 bits, and happen to
> > sit next to reserved fields (which must be zero), so reading the byte
> > should work in practice. But for a few of them you'll get cruft in the
> > low bits.
> >
> > I don't know what they were thinking when they decided to have 3 and 5
> > bit capability fields, and then specify some of them as being a byte
> > wide as well. Perhaps the spec committee had a big night out ;)
> >
> > I was going to write a generic version of pci_find_ht_capability() (as
> > suggested by Segher), so along with that I'll clean up the #defines to
> > just be the 3 or 5 bit capability codes, and then have a shift for
> > getting the capability out of the byte.
> >
> > Users will still need to know if they're looking for a 3 or 5 bit
> > capability, but we can encapsulate that in pci_find_ht_capability()  
> > and
> > hopefully most people won't have to see the difference.
> 
> Keep the defines the full 8 bits and put all the knowledge
> about which bits are relevant into pci_find_ht_capability()?
> If you do shifting you can get clashes (say, 0b00110 vs. 0b110),
> and this would make for simpler code, too.

That's what I did .. I think? :)

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

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: 189 bytes --]

patch pci-make-some-msi-x-defines-generic.patch added to gregkh-2.6 tree

[gregkh]

This is a note to let you know that I've just added the patch titled

     Subject: PCI: Make some MSI-X #defines generic

to my gregkh-2.6 tree.  Its filename is

     pci-make-some-msi-x-defines-generic.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/


>From michael@ozlabs.org Mon Nov  6 23:21:30 2006
To: <linuxppc-dev@ozlabs.org>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Greg Kroah-Hartman <greg@kroah.com>,
	linux-pci@atrey.karlin.mff.cuni.cz,
	Jake Moilanen <moilanen@austin.ibm.com>,
	Segher Boessenkool <segher@kernel.crashing.org>,
	Eric W. Biederman <ebiederm@xmission.com>,
	David S. Miller <davem@davemloft.net>
From: Michael Ellerman <michael@ellerman.id.au>
Date: Tue, 07 Nov 2006 18:21:21 +1100
Subject: PCI: Make some MSI-X #defines generic
Message-Id: <20061107072123.CF52F67C35@ozlabs.org>

Move some MSI-X #defines into pci_regs.h so they can be used
outside of drivers/pci.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/pci/msi.h        |    8 --------
 include/linux/pci_regs.h |    6 ++++++
 2 files changed, 6 insertions(+), 8 deletions(-)

--- gregkh-2.6.orig/drivers/pci/msi.h
+++ gregkh-2.6/drivers/pci/msi.h
@@ -6,14 +6,6 @@
 #ifndef MSI_H
 #define MSI_H
 
-/*
- * MSI-X Address Register
- */
-#define PCI_MSIX_FLAGS_QSIZE		0x7FF
-#define PCI_MSIX_FLAGS_ENABLE		(1 << 15)
-#define PCI_MSIX_FLAGS_BIRMASK		(7 << 0)
-#define PCI_MSIX_FLAGS_BITMASK		(1 << 0)
-
 #define PCI_MSIX_ENTRY_SIZE			16
 #define  PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET	0
 #define  PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET	4
--- gregkh-2.6.orig/include/linux/pci_regs.h
+++ gregkh-2.6/include/linux/pci_regs.h
@@ -292,6 +292,12 @@
 #define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
 #define PCI_MSI_MASK_BIT	16	/* Mask bits register */
 
+/* MSI-X registers (these are at offset PCI_MSI_FLAGS) */
+#define PCI_MSIX_FLAGS_QSIZE	0x7FF
+#define PCI_MSIX_FLAGS_ENABLE	(1 << 15)
+#define PCI_MSIX_FLAGS_BIRMASK	(7 << 0)
+#define PCI_MSIX_FLAGS_BITMASK	(1 << 0)
+
 /* CompactPCI Hotswap Register */
 
 #define PCI_CHSWP_CSR		2	/* Control and Status Register */


Patches currently in gregkh-2.6 which might be from linuxppc-dev@ozlabs.org are

pci/pci-make-some-msi-x-defines-generic.patch