* [PATCH] Add IPIC MSI interrupt support
@ 2007-11-30 3:48 Li Li
2007-12-03 1:52 ` David Gibson
2007-12-03 4:02 ` Michael Ellerman
0 siblings, 2 replies; 13+ messages in thread
From: Li Li @ 2007-11-30 3:48 UTC (permalink / raw)
To: Kumar Gala, linuxppc-dev
The IPIC MSI is introduced on MPC837x chip.
Implements the IPIC MSI as two level interrupt controller.
Signed-off-by: Tony Li <tony.li@freescale.com>
---
arch/powerpc/boot/dts/mpc8377_mds.dts | 14 ++
arch/powerpc/boot/dts/mpc8378_mds.dts | 14 ++
arch/powerpc/boot/dts/mpc8379_mds.dts | 14 ++
arch/powerpc/platforms/83xx/Kconfig | 6 +
arch/powerpc/platforms/83xx/mpc837x_mds.c | 11 +
arch/powerpc/sysdev/Makefile | 1 +
arch/powerpc/sysdev/ipic_msi.c | 359 +++++++++++++++++++++++++++++
include/asm-powerpc/ipic_msi.h | 54 +++++
8 files changed, 473 insertions(+), 0 deletions(-)
create mode 100644 arch/powerpc/sysdev/ipic_msi.c
create mode 100644 include/asm-powerpc/ipic_msi.h
diff --git a/arch/powerpc/boot/dts/mpc8377_mds.dts b/arch/powerpc/boot/dts/mpc8377_mds.dts
index 1f7819e..1068fe2 100644
--- a/arch/powerpc/boot/dts/mpc8377_mds.dts
+++ b/arch/powerpc/boot/dts/mpc8377_mds.dts
@@ -210,6 +210,20 @@
#interrupt-cells = <2>;
reg = <700 100>;
};
+
+ ipic-msi@7c0 {
+ compatible = "fsl,ipic-msi";
+ reg = <7c0 40>;
+ interrupts = < 43 8
+ 4 8
+ 51 8
+ 52 8
+ 56 8
+ 57 8
+ 58 8
+ 59 8 >;
+ interrupt-parent = < &ipic >;
+ };
};
pci@e0008500 {
diff --git a/arch/powerpc/boot/dts/mpc8378_mds.dts b/arch/powerpc/boot/dts/mpc8378_mds.dts
index 1503ae3..f12658f 100644
--- a/arch/powerpc/boot/dts/mpc8378_mds.dts
+++ b/arch/powerpc/boot/dts/mpc8378_mds.dts
@@ -192,6 +192,20 @@
#interrupt-cells = <2>;
reg = <700 100>;
};
+
+ ipic-msi@7c0 {
+ compatible = "fsl,ipic-msi";
+ reg = <7c0 40>;
+ interrupts = < 43 8
+ 4 8
+ 51 8
+ 52 8
+ 56 8
+ 57 8
+ 58 8
+ 59 8 >;
+ interrupt-parent = < &ipic >;
+ };
};
pci@e0008500 {
diff --git a/arch/powerpc/boot/dts/mpc8379_mds.dts b/arch/powerpc/boot/dts/mpc8379_mds.dts
index cdb4426..9fe4bd2 100644
--- a/arch/powerpc/boot/dts/mpc8379_mds.dts
+++ b/arch/powerpc/boot/dts/mpc8379_mds.dts
@@ -236,6 +236,20 @@
#interrupt-cells = <2>;
reg = <700 100>;
};
+
+ ipic-msi@7c0 {
+ compatible = "fsl,ipic-msi";
+ reg = <7c0 40>;
+ interrupts = < 43 8
+ 4 8
+ 51 8
+ 52 8
+ 56 8
+ 57 8
+ 58 8
+ 59 8 >;
+ interrupt-parent = < &ipic >;
+ };
};
pci@e0008500 {
diff --git a/arch/powerpc/platforms/83xx/Kconfig b/arch/powerpc/platforms/83xx/Kconfig
index 00154c5..4c51f78 100644
--- a/arch/powerpc/platforms/83xx/Kconfig
+++ b/arch/powerpc/platforms/83xx/Kconfig
@@ -88,6 +88,12 @@ config PPC_MPC837x
select FSL_SERDES
default y if MPC837x_MDS
+config IPIC_MSI
+ bool
+ depends on PCI_MSI
+ default y if PPC_MPC837x
+ default n
+
config PPC_MPC83XX_PCIE
bool "MPC837X PCI Express support"
depends on PCIEPORTBUS && PPC_MPC837x
diff --git a/arch/powerpc/platforms/83xx/mpc837x_mds.c b/arch/powerpc/platforms/83xx/mpc837x_mds.c
index 6048f1b..dbea34b 100644
--- a/arch/powerpc/platforms/83xx/mpc837x_mds.c
+++ b/arch/powerpc/platforms/83xx/mpc837x_mds.c
@@ -17,6 +17,9 @@
#include <asm/time.h>
#include <asm/ipic.h>
+#if CONFIG_IPIC_MSI
+#include <asm/ipic_msi.h>
+#endif
#include <asm/udbg.h>
#include <asm/prom.h>
@@ -84,6 +87,14 @@ static void __init mpc837x_mds_init_IRQ(void)
* in case the boot rom changed something on us.
*/
ipic_set_default_priority();
+
+#if CONFIG_IPIC_MSI
+ np = of_find_compatible_node(NULL, NULL, "fsl,ipic-msi");
+ if (!np)
+ return;
+
+ ipic_msi_init(np, ipic_msi_cascade);
+#endif
}
/*
diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
index 99a77d7..5946b6a 100644
--- a/arch/powerpc/sysdev/Makefile
+++ b/arch/powerpc/sysdev/Makefile
@@ -25,6 +25,7 @@ ifeq ($(CONFIG_PPC_MERGE),y)
obj-$(CONFIG_PPC_INDIRECT_PCI) += indirect_pci.o
obj-$(CONFIG_PPC_I8259) += i8259.o
obj-$(CONFIG_PPC_83xx) += ipic.o
+obj-$(CONFIG_IPIC_MSI) += ipic_msi.o
obj-$(CONFIG_4xx) += uic.o
obj-$(CONFIG_XILINX_VIRTEX) += xilinx_intc.o
endif
diff --git a/arch/powerpc/sysdev/ipic_msi.c b/arch/powerpc/sysdev/ipic_msi.c
new file mode 100644
index 0000000..57758f7
--- /dev/null
+++ b/arch/powerpc/sysdev/ipic_msi.c
@@ -0,0 +1,359 @@
+/*
+ * arch/powerpc/sysdev/ipic_msi.c
+ *
+ * IPIC MSI routines implementations.
+ *
+ * Auther: Tony Li <tony.li@freescale.com>
+ *
+ * Copyright (c) 2007 Freescale Semiconductor, Inc.
+ *
+ * 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.
+ */
+
+
+#include <linux/pci.h>
+#include <linux/msi.h>
+#include <linux/irq.h>
+
+#include <asm/ipic_msi.h>
+
+#define MSIR0 0x43
+#define MSIR1 0x4
+#define MSIR2 0x51
+#define MSIR3 0x52
+#define MSIR4 0x56
+#define MSIR5 0x57
+#define MSIR6 0x58
+#define MSIR7 0x59
+
+
+static struct ipic_msi *ipic_msi;
+static DEFINE_SPINLOCK(ipic_msi_lock);
+static DEFINE_SPINLOCK(ipic_msi_bitmap_lock);
+
+static inline u32 ipic_msi_read(volatile u32 __iomem *base, unsigned int reg)
+{
+ return in_be32(base + (reg >> 2));
+}
+
+static inline void ipic_msi_write(volatile u32 __iomem *base,
+ unsigned int reg, u32 value)
+{
+ out_be32(base + (reg >> 2), value);
+}
+
+static struct ipic_msi * ipic_msi_from_irq(unsigned int virq)
+{
+ return ipic_msi;
+}
+
+#define ipic_msi_irq_to_hw(virq) ((unsigned int)irq_map[virq].hwirq)
+static void ipic_msi_unmask(unsigned int virq)
+{
+ struct ipic_msi *msi = ipic_msi_from_irq(virq);
+ unsigned int src = ipic_msi_irq_to_hw(virq);
+ unsigned long flags;
+ u32 temp;
+
+ spin_lock_irqsave(&ipic_msi_lock, flags);
+ temp = ipic_msi_read(msi->regs, IPIC_MSIMR);
+ ipic_msi_write(msi->regs, IPIC_MSIMR,
+ temp & ~(1 << (src / msi->int_per_msir)));
+
+ spin_unlock_irqrestore(&ipic_msi_lock, flags);
+}
+
+static void ipic_msi_mask(unsigned int virq)
+{
+ struct ipic_msi *msi = ipic_msi_from_irq(virq);
+ unsigned int src = ipic_msi_irq_to_hw(virq);
+ unsigned long flags;
+ u32 temp;
+
+ spin_lock_irqsave(&ipic_msi_lock, flags);
+
+ temp = ipic_msi_read(msi->regs, IPIC_MSIMR);
+ ipic_msi_write(msi->regs, IPIC_MSIMR,
+ temp | (1 << (src / msi->int_per_msir)));
+
+ spin_unlock_irqrestore(&ipic_msi_lock, flags);
+}
+/*
+ * We do not need this actually. The MSIR register has been read once
+ * in ipic_msi_get_irq. So, this MSI interrupt has been acked
+ */
+static void ipic_msi_ack(unsigned int virq)
+{
+ struct ipic_msi *msi = ipic_msi_from_irq(virq);
+ unsigned int src = ipic_msi_irq_to_hw(virq);
+ unsigned long flags;
+
+ spin_lock_irqsave(&ipic_msi_lock, flags);
+
+ ipic_msi_read(msi->regs, IPIC_MSIR0 + (4 * (src / msi->int_per_msir)));
+
+ spin_unlock_irqrestore(&ipic_msi_lock,flags);
+}
+
+static struct irq_chip ipic_msi_chip = {
+ .typename = " IPIC MSI ",
+ .unmask = ipic_msi_unmask,
+ .mask = ipic_msi_mask,
+ .ack = ipic_msi_ack,
+};
+
+static int ipic_msi_host_map(struct irq_host *h, unsigned int virq,
+ irq_hw_number_t hw)
+{
+ struct ipic_msi *msi = h->host_data;
+ struct irq_chip *chip = msi->hc_irq;
+
+ set_irq_chip_data(virq, msi);
+ get_irq_desc(virq)->status |= IRQ_TYPE_EDGE_FALLING;
+
+ set_irq_chip_and_handler(virq, chip, handle_edge_irq);
+ return 0;
+}
+
+static struct irq_host_ops ipic_msi_host_ops = {
+ .map = ipic_msi_host_map,
+};
+
+irq_hw_number_t ipic_msi_alloc_hwirqs(struct ipic_msi *msi, int num)
+{
+ unsigned long flags;
+ int offset, order = get_count_order(num);
+
+ spin_lock_irqsave(&ipic_msi_bitmap_lock, flags);
+
+ offset = bitmap_find_free_region(msi->ipic_msi_bitmap,
+ msi->nr_msir * msi->int_per_msir, order);
+
+ spin_unlock_irqrestore(&ipic_msi_bitmap_lock, flags);
+
+ pr_debug("%s: allocated 0x%x (2^%d) at offset 0x%x\n",
+ __FUNCTION__, num, order, offset);
+
+ return offset;
+}
+
+void ipic_msi_free_hwirqs(struct ipic_msi *msi, int offset, int num)
+{
+ unsigned long flags;
+ int order = get_count_order(num);
+
+ pr_debug("%s: freeing 0x%x (2^%d) at offset 0x%x\n",
+ __FUNCTION__, num, order, offset);
+
+ spin_lock_irqsave(&ipic_msi_bitmap_lock, flags);
+ bitmap_release_region(msi->ipic_msi_bitmap, offset, order);
+ spin_unlock_irqrestore(&ipic_msi_bitmap_lock, flags);
+}
+
+static int ipic_msi_init_allocator(struct ipic_msi *msi)
+{
+ int size;
+
+ size = BITS_TO_LONGS(msi->nr_msir * msi->int_per_msir) * sizeof(long);
+ msi->ipic_msi_bitmap = alloc_maybe_bootmem(size, GFP_KERNEL);
+
+ if (msi->ipic_msi_bitmap == NULL) {
+ pr_debug("%s: ENOMEM allocating allocator bitmap!\n",
+ __FUNCTION__);
+ return -ENOMEM;
+ }
+ memset(msi->ipic_msi_bitmap, 0, size);
+
+ return 0;
+}
+
+static void ipic_msi_compose_msg(struct ipic_msi *msi, int hwirq,
+ struct msi_msg *msg)
+{
+ unsigned int srs;
+ unsigned int ibs;
+
+ srs = hwirq / msi->int_per_msir;
+ ibs = hwirq - srs * msi->int_per_msir;
+
+ msg->address_lo = msi->msi_addr_lo;
+ msg->address_hi = msi->msi_addr_hi;
+ msg->data = (srs << 5) | (ibs & 0x1F);
+
+ pr_debug("%s: allocated srs: %d, ibs: %d\n",
+ __FUNCTION__, srs, ibs);
+
+}
+
+static int ipic_msi_setup_irqs(struct pci_dev *pdev, int nvec, int type)
+{
+ struct ipic_msi *msi = ipic_msi;
+ irq_hw_number_t hwirq;
+ unsigned int virq;
+ struct msi_desc *entry;
+ struct msi_msg msg;
+
+ list_for_each_entry(entry, &pdev->msi_list, list) {
+ hwirq = ipic_msi_alloc_hwirqs(msi, 1);
+ if (hwirq < 0) {
+ pr_debug("%s: fail allocating msi interrupt\n",
+ __FUNCTION__);
+ return hwirq;
+ }
+
+ /* This hwirq belongs to the irq_host other than irq_host of IPIC
+ * So, it is independent to hwirq of IPIC */
+ virq = irq_create_mapping(msi->irqhost, hwirq);
+ if (virq == NO_IRQ) {
+ pr_debug("%s: fail mapping hwirq 0x%lx\n",
+ __FUNCTION__, hwirq);
+ ipic_msi_free_hwirqs(msi, hwirq, 1);
+ return -ENOSPC;
+ }
+ set_irq_msi(virq, entry);
+ ipic_msi_compose_msg(msi, hwirq, &msg);
+ write_msi_msg(virq, &msg);
+
+ hwirq++;
+ }
+
+ return 0;
+}
+
+static void ipic_msi_teardown_irqs(struct pci_dev *pdev)
+{
+ struct msi_desc *entry;
+ struct ipic_msi *msi = ipic_msi;
+
+ list_for_each_entry(entry, &pdev->msi_list, list) {
+ if (entry->irq == NO_IRQ)
+ continue;
+ set_irq_msi(entry->irq, NULL);
+ ipic_msi_free_hwirqs(msi, virq_to_hw(entry->irq), 1);
+ irq_dispose_mapping(entry->irq);
+ }
+
+ return;
+}
+
+void __init ipic_msi_init(struct device_node *node,
+ void (*handler)(unsigned int irq, struct irq_desc *desc))
+{
+ struct ipic_msi *msi;
+ struct resource res;
+ int i;
+ int rc = 0;
+
+ msi = alloc_maybe_bootmem(sizeof(*msi), GFP_KERNEL);
+ if (msi == NULL)
+ return;
+
+ memset(msi, 0, sizeof(*msi));
+
+
+ msi->irqhost = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LINEAR,
+ NR_MSIR, &ipic_msi_host_ops, 0);
+ if (msi->irqhost == NULL) {
+ of_node_put(node);
+ goto out;
+ }
+
+ rc = of_address_to_resource(node, 0, &res);
+ if (rc) {
+ of_node_put(node);
+ goto out;
+ }
+
+ msi->regs = ioremap(res.start, res.end - res.start + 1);
+ msi->irqhost->host_data = msi;
+ msi->hc_irq = &ipic_msi_chip;
+
+ msi->msi_addr_lo = 0xE00007F8;
+ msi->msi_addr_hi = 0;
+ msi->nr_msir = ARRAY_SIZE(msi->msir);
+ msi->int_per_msir = 32;
+ for (i = 0; i < msi->nr_msir; i++) {
+ unsigned int virt_msir = irq_of_parse_and_map(node, i);
+ if (virt_msir != NO_IRQ) {
+ set_irq_data(virt_msir, msi);
+ set_irq_chained_handler(virt_msir, handler);
+ msi->msir[i] = virt_msir;
+ } else
+ msi->msir[i] = NO_IRQ;
+ }
+
+ rc = ipic_msi_init_allocator(msi);
+ if (rc)
+ goto out;
+
+ ipic_msi = msi;
+
+ WARN_ON(ppc_md.setup_msi_irqs);
+ ppc_md.setup_msi_irqs = ipic_msi_setup_irqs;
+ ppc_md.teardown_msi_irqs = ipic_msi_teardown_irqs;
+
+out:
+ if (mem_init_done)
+ kfree(msi);
+
+ return;
+}
+
+static int ipic_msi_get_irq(struct ipic_msi *msi, int virt_msir)
+{
+ int msir = -1;
+ unsigned int temp;
+ unsigned int offset;
+ int i;
+
+ for (i = 0; i < msi->nr_msir; i++)
+ if (virt_msir == msi->msir[i]) {
+ msir = i;
+ break;
+ }
+
+ if (i >= msi->nr_msir)
+ return NO_IRQ;
+
+ temp = ipic_msi_read(msi->regs, IPIC_MSIR0 + (i * 4));
+ offset = ffs(temp) - 1;
+
+ return irq_linear_revmap(msi->irqhost, (msir * msi->int_per_msir + offset));
+}
+
+void ipic_msi_cascade(unsigned int irq, struct irq_desc *desc)
+{
+ struct ipic_msi *msi;
+ unsigned int cascade_irq;
+
+ spin_lock(&desc->lock);
+ if (desc->chip->mask_ack)
+ desc->chip->mask_ack(irq);
+ else {
+ desc->chip->mask(irq);
+ desc->chip->ack(irq);
+ }
+
+ if (unlikely(desc->status & IRQ_INPROGRESS))
+ goto unlock;
+
+ desc->status |= IRQ_INPROGRESS;
+ msi = desc->handler_data;
+ cascade_irq = ipic_msi_get_irq(msi, irq);
+
+ spin_unlock(&desc->lock);
+
+ if (cascade_irq != NO_IRQ)
+ generic_handle_irq(cascade_irq);
+
+ spin_lock(&desc->lock);
+ desc->status &= ~IRQ_INPROGRESS;
+ if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
+ desc->chip->unmask(irq);
+
+unlock:
+ spin_unlock(&desc->lock);
+}
diff --git a/include/asm-powerpc/ipic_msi.h b/include/asm-powerpc/ipic_msi.h
new file mode 100644
index 0000000..ba952df
--- /dev/null
+++ b/include/asm-powerpc/ipic_msi.h
@@ -0,0 +1,54 @@
+/*
+ * include/asm-powerpc/ipic_msi.h
+ *
+ * IPIC MSI structure.
+ *
+ * Author: Tony Li <tony.li@freescale.com>
+ *
+ * Copyright (c) 2007 Freescale Semiconductor, Inc
+ *
+ * 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_IPIC_MSI_H
+#define _ASM_POWERPC_IPIC_MSI_H
+
+#define NR_MSIR 8
+/* MSI interrupt register offset compared to MSI base address (0xc0) */
+#define IPIC_MSIR0 0x0 /* MSI Interrupt Reg 0 */
+#define IPIC_MSIR1 0x4 /* MSI Interrupt Reg 1 */
+#define IPIC_MSIR2 0x8 /* MSI Interrupt Reg 2 */
+#define IPIC_MSIR3 0xC /* MSI Interrupt Reg 3 */
+#define IPIC_MSIR4 0x10 /* MSI Interrupt Reg 4 */
+#define IPIC_MSIR5 0x14 /* MSI Interrupt Reg 5 */
+#define IPIC_MSIR6 0x18 /* MSI Interrupt Reg 6 */
+#define IPIC_MSIR7 0x1C /* MSI Interrupt Reg 7 */
+#define IPIC_MSIMR 0x30 /* MSI Interrupt Mask Reg */
+#define IPIC_MSISR 0x34 /* MSI Interrupt Status Reg */
+#define IPIC_MSIIR 0x38 /* MSI Interrupt Index Reg */
+
+struct ipic_msi {
+ volatile u32 __iomem *regs;
+
+ struct irq_host *irqhost;
+ struct irq_chip *hc_irq;
+
+ unsigned long cascade_irq;
+ unsigned int msir[8];
+
+ u32 msi_addr_lo;
+ u32 msi_addr_hi;
+
+ int nr_msir;
+ int int_per_msir;
+
+ void *ipic_msi_bitmap;
+};
+
+extern void ipic_msi_cascade(unsigned int irq, struct irq_desc *desc);
+extern void __init ipic_msi_init(struct device_node *node,
+ void (*handler)(unsigned int irq, struct irq_desc *desc));
+
+#endif /* _ASM_POWERPC_IPIC_MSI_H */
--
1.5.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-11-30 3:48 [PATCH] Add IPIC MSI interrupt support Li Li
@ 2007-12-03 1:52 ` David Gibson
2007-12-03 4:59 ` Li Li
2007-12-03 4:02 ` Michael Ellerman
1 sibling, 1 reply; 13+ messages in thread
From: David Gibson @ 2007-12-03 1:52 UTC (permalink / raw)
To: Li Li; +Cc: linuxppc-dev, Kumar Gala
On Fri, Nov 30, 2007 at 11:48:39AM +0800, Li Li wrote:
> The IPIC MSI is introduced on MPC837x chip.
> Implements the IPIC MSI as two level interrupt controller.
>
> Signed-off-by: Tony Li <tony.li@freescale.com>
> ---
> arch/powerpc/boot/dts/mpc8377_mds.dts | 14 ++
> arch/powerpc/boot/dts/mpc8378_mds.dts | 14 ++
> arch/powerpc/boot/dts/mpc8379_mds.dts | 14 ++
> arch/powerpc/platforms/83xx/Kconfig | 6 +
> arch/powerpc/platforms/83xx/mpc837x_mds.c | 11 +
> arch/powerpc/sysdev/Makefile | 1 +
> arch/powerpc/sysdev/ipic_msi.c | 359 +++++++++++++++++++++++++++++
> include/asm-powerpc/ipic_msi.h | 54 +++++
> 8 files changed, 473 insertions(+), 0 deletions(-)
> create mode 100644 arch/powerpc/sysdev/ipic_msi.c
> create mode 100644 include/asm-powerpc/ipic_msi.h
>
> diff --git a/arch/powerpc/boot/dts/mpc8377_mds.dts b/arch/powerpc/boot/dts/mpc8377_mds.dts
> index 1f7819e..1068fe2 100644
> --- a/arch/powerpc/boot/dts/mpc8377_mds.dts
> +++ b/arch/powerpc/boot/dts/mpc8377_mds.dts
> @@ -210,6 +210,20 @@
> #interrupt-cells = <2>;
> reg = <700 100>;
> };
> +
> + ipic-msi@7c0 {
> + compatible = "fsl,ipic-msi";
> + reg = <7c0 40>;
> + interrupts = < 43 8
> + 4 8
> + 51 8
> + 52 8
> + 56 8
> + 57 8
> + 58 8
> + 59 8 >;
> + interrupt-parent = < &ipic >;
> + };
I'm not sure why the MSI needs a device node of its own. What actual
piece of hardware is this representing?
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-11-30 3:48 [PATCH] Add IPIC MSI interrupt support Li Li
2007-12-03 1:52 ` David Gibson
@ 2007-12-03 4:02 ` Michael Ellerman
2007-12-03 9:07 ` Li Li
1 sibling, 1 reply; 13+ messages in thread
From: Michael Ellerman @ 2007-12-03 4:02 UTC (permalink / raw)
To: Li Li; +Cc: linuxppc-dev, Kumar Gala
[-- Attachment #1: Type: text/plain, Size: 15736 bytes --]
Hi r64360,
A few comments below :)
On Fri, 2007-11-30 at 11:48 +0800, Li Li wrote:
> The IPIC MSI is introduced on MPC837x chip.
> Implements the IPIC MSI as two level interrupt controller.
>
> Signed-off-by: Tony Li <tony.li@freescale.com>
> ---
> arch/powerpc/boot/dts/mpc8377_mds.dts | 14 ++
> arch/powerpc/boot/dts/mpc8378_mds.dts | 14 ++
> arch/powerpc/boot/dts/mpc8379_mds.dts | 14 ++
> arch/powerpc/platforms/83xx/Kconfig | 6 +
> arch/powerpc/platforms/83xx/mpc837x_mds.c | 11 +
> arch/powerpc/sysdev/Makefile | 1 +
> arch/powerpc/sysdev/ipic_msi.c | 359 +++++++++++++++++++++++++++++
> include/asm-powerpc/ipic_msi.h | 54 +++++
> 8 files changed, 473 insertions(+), 0 deletions(-)
> create mode 100644 arch/powerpc/sysdev/ipic_msi.c
> create mode 100644 include/asm-powerpc/ipic_msi.h
>
> diff --git a/arch/powerpc/boot/dts/mpc8377_mds.dts b/arch/powerpc/boot/dts/mpc8377_mds.dts
> index 1f7819e..1068fe2 100644
> --- a/arch/powerpc/boot/dts/mpc8377_mds.dts
> +++ b/arch/powerpc/boot/dts/mpc8377_mds.dts
> @@ -210,6 +210,20 @@
> #interrupt-cells = <2>;
> reg = <700 100>;
> };
> +
> + ipic-msi@7c0 {
> + compatible = "fsl,ipic-msi";
> + reg = <7c0 40>;
> + interrupts = < 43 8
> + 4 8
> + 51 8
> + 52 8
> + 56 8
> + 57 8
> + 58 8
> + 59 8 >;
> + interrupt-parent = < &ipic >;
> + };
> };
>
> pci@e0008500 {
> diff --git a/arch/powerpc/boot/dts/mpc8378_mds.dts b/arch/powerpc/boot/dts/mpc8378_mds.dts
> index 1503ae3..f12658f 100644
> --- a/arch/powerpc/boot/dts/mpc8378_mds.dts
> +++ b/arch/powerpc/boot/dts/mpc8378_mds.dts
> @@ -192,6 +192,20 @@
> #interrupt-cells = <2>;
> reg = <700 100>;
> };
> +
> + ipic-msi@7c0 {
> + compatible = "fsl,ipic-msi";
> + reg = <7c0 40>;
> + interrupts = < 43 8
> + 4 8
> + 51 8
> + 52 8
> + 56 8
> + 57 8
> + 58 8
> + 59 8 >;
> + interrupt-parent = < &ipic >;
> + };
> };
>
> pci@e0008500 {
> diff --git a/arch/powerpc/boot/dts/mpc8379_mds.dts b/arch/powerpc/boot/dts/mpc8379_mds.dts
> index cdb4426..9fe4bd2 100644
> --- a/arch/powerpc/boot/dts/mpc8379_mds.dts
> +++ b/arch/powerpc/boot/dts/mpc8379_mds.dts
> @@ -236,6 +236,20 @@
> #interrupt-cells = <2>;
> reg = <700 100>;
> };
> +
> + ipic-msi@7c0 {
> + compatible = "fsl,ipic-msi";
> + reg = <7c0 40>;
> + interrupts = < 43 8
> + 4 8
> + 51 8
> + 52 8
> + 56 8
> + 57 8
> + 58 8
> + 59 8 >;
> + interrupt-parent = < &ipic >;
> + };
> };
>
> pci@e0008500 {
> diff --git a/arch/powerpc/platforms/83xx/Kconfig b/arch/powerpc/platforms/83xx/Kconfig
> index 00154c5..4c51f78 100644
> --- a/arch/powerpc/platforms/83xx/Kconfig
> +++ b/arch/powerpc/platforms/83xx/Kconfig
> @@ -88,6 +88,12 @@ config PPC_MPC837x
> select FSL_SERDES
> default y if MPC837x_MDS
>
> +config IPIC_MSI
> + bool
> + depends on PCI_MSI
> + default y if PPC_MPC837x
> + default n
> +
> config PPC_MPC83XX_PCIE
> bool "MPC837X PCI Express support"
> depends on PCIEPORTBUS && PPC_MPC837x
> diff --git a/arch/powerpc/platforms/83xx/mpc837x_mds.c b/arch/powerpc/platforms/83xx/mpc837x_mds.c
> index 6048f1b..dbea34b 100644
> --- a/arch/powerpc/platforms/83xx/mpc837x_mds.c
> +++ b/arch/powerpc/platforms/83xx/mpc837x_mds.c
> @@ -17,6 +17,9 @@
>
> #include <asm/time.h>
> #include <asm/ipic.h>
> +#if CONFIG_IPIC_MSI
> +#include <asm/ipic_msi.h>
> +#endif
> #include <asm/udbg.h>
> #include <asm/prom.h>
I'd rather you just include it unconditionally.
> @@ -84,6 +87,14 @@ static void __init mpc837x_mds_init_IRQ(void)
> * in case the boot rom changed something on us.
> */
> ipic_set_default_priority();
> +
> +#if CONFIG_IPIC_MSI
> + np = of_find_compatible_node(NULL, NULL, "fsl,ipic-msi");
> + if (!np)
> + return;
> +
> + ipic_msi_init(np, ipic_msi_cascade);
> +#endif
If you have a no-op version of ipic_msi_init() in your header file then
you can remove the #ifdef in the C code - which I think is nicer.
Why do you pass the handler into the init routine, rather than have the
init routine just set the handler. Then you could make the handler
static.
> }
>
> /*
> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> index 99a77d7..5946b6a 100644
> --- a/arch/powerpc/sysdev/Makefile
> +++ b/arch/powerpc/sysdev/Makefile
> @@ -25,6 +25,7 @@ ifeq ($(CONFIG_PPC_MERGE),y)
> obj-$(CONFIG_PPC_INDIRECT_PCI) += indirect_pci.o
> obj-$(CONFIG_PPC_I8259) += i8259.o
> obj-$(CONFIG_PPC_83xx) += ipic.o
> +obj-$(CONFIG_IPIC_MSI) += ipic_msi.o
> obj-$(CONFIG_4xx) += uic.o
> obj-$(CONFIG_XILINX_VIRTEX) += xilinx_intc.o
> endif
> diff --git a/arch/powerpc/sysdev/ipic_msi.c b/arch/powerpc/sysdev/ipic_msi.c
> new file mode 100644
> index 0000000..57758f7
> --- /dev/null
> +++ b/arch/powerpc/sysdev/ipic_msi.c
> @@ -0,0 +1,359 @@
> +/*
> + * arch/powerpc/sysdev/ipic_msi.c
> + *
> + * IPIC MSI routines implementations.
> + *
> + * Auther: Tony Li <tony.li@freescale.com>
> + *
> + * Copyright (c) 2007 Freescale Semiconductor, Inc.
> + *
> + * 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.
> + */
> +
> +
> +#include <linux/pci.h>
> +#include <linux/msi.h>
> +#include <linux/irq.h>
> +
> +#include <asm/ipic_msi.h>
> +
> +#define MSIR0 0x43
> +#define MSIR1 0x4
> +#define MSIR2 0x51
> +#define MSIR3 0x52
> +#define MSIR4 0x56
> +#define MSIR5 0x57
> +#define MSIR6 0x58
> +#define MSIR7 0x59
> +
> +
> +static struct ipic_msi *ipic_msi;
> +static DEFINE_SPINLOCK(ipic_msi_lock);
> +static DEFINE_SPINLOCK(ipic_msi_bitmap_lock);
> +
> +static inline u32 ipic_msi_read(volatile u32 __iomem *base, unsigned int reg)
> +{
> + return in_be32(base + (reg >> 2));
> +}
> +
> +static inline void ipic_msi_write(volatile u32 __iomem *base,
> + unsigned int reg, u32 value)
> +{
> + out_be32(base + (reg >> 2), value);
> +}
> +
> +static struct ipic_msi * ipic_msi_from_irq(unsigned int virq)
> +{
> + return ipic_msi;
> +}
> +
> +#define ipic_msi_irq_to_hw(virq) ((unsigned int)irq_map[virq].hwirq)
What's wrong with virq_to_hw() ?
> +static void ipic_msi_unmask(unsigned int virq)
> +{
> + struct ipic_msi *msi = ipic_msi_from_irq(virq);
> + unsigned int src = ipic_msi_irq_to_hw(virq);
> + unsigned long flags;
> + u32 temp;
> +
> + spin_lock_irqsave(&ipic_msi_lock, flags);
> + temp = ipic_msi_read(msi->regs, IPIC_MSIMR);
> + ipic_msi_write(msi->regs, IPIC_MSIMR,
> + temp & ~(1 << (src / msi->int_per_msir)));
> +
> + spin_unlock_irqrestore(&ipic_msi_lock, flags);
> +}
> +
> +static void ipic_msi_mask(unsigned int virq)
> +{
> + struct ipic_msi *msi = ipic_msi_from_irq(virq);
> + unsigned int src = ipic_msi_irq_to_hw(virq);
> + unsigned long flags;
> + u32 temp;
> +
> + spin_lock_irqsave(&ipic_msi_lock, flags);
> +
> + temp = ipic_msi_read(msi->regs, IPIC_MSIMR);
> + ipic_msi_write(msi->regs, IPIC_MSIMR,
> + temp | (1 << (src / msi->int_per_msir)));
> +
> + spin_unlock_irqrestore(&ipic_msi_lock, flags);
> +}
> +/*
> + * We do not need this actually. The MSIR register has been read once
> + * in ipic_msi_get_irq. So, this MSI interrupt has been acked
> + */
> +static void ipic_msi_ack(unsigned int virq)
> +{
> + struct ipic_msi *msi = ipic_msi_from_irq(virq);
> + unsigned int src = ipic_msi_irq_to_hw(virq);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ipic_msi_lock, flags);
> +
> + ipic_msi_read(msi->regs, IPIC_MSIR0 + (4 * (src / msi->int_per_msir)));
> +
> + spin_unlock_irqrestore(&ipic_msi_lock,flags);
> +}
> +
> +static struct irq_chip ipic_msi_chip = {
> + .typename = " IPIC MSI ",
> + .unmask = ipic_msi_unmask,
> + .mask = ipic_msi_mask,
> + .ack = ipic_msi_ack,
> +};
> +
> +static int ipic_msi_host_map(struct irq_host *h, unsigned int virq,
> + irq_hw_number_t hw)
> +{
> + struct ipic_msi *msi = h->host_data;
> + struct irq_chip *chip = msi->hc_irq;
> +
> + set_irq_chip_data(virq, msi);
> + get_irq_desc(virq)->status |= IRQ_TYPE_EDGE_FALLING;
> +
> + set_irq_chip_and_handler(virq, chip, handle_edge_irq);
> + return 0;
> +}
> +
> +static struct irq_host_ops ipic_msi_host_ops = {
> + .map = ipic_msi_host_map,
> +};
> +
> +irq_hw_number_t ipic_msi_alloc_hwirqs(struct ipic_msi *msi, int num)
> +{
> + unsigned long flags;
> + int offset, order = get_count_order(num);
> +
> + spin_lock_irqsave(&ipic_msi_bitmap_lock, flags);
> +
> + offset = bitmap_find_free_region(msi->ipic_msi_bitmap,
> + msi->nr_msir * msi->int_per_msir, order);
> +
> + spin_unlock_irqrestore(&ipic_msi_bitmap_lock, flags);
> +
> + pr_debug("%s: allocated 0x%x (2^%d) at offset 0x%x\n",
> + __FUNCTION__, num, order, offset);
> +
> + return offset;
> +}
> +
> +void ipic_msi_free_hwirqs(struct ipic_msi *msi, int offset, int num)
> +{
> + unsigned long flags;
> + int order = get_count_order(num);
> +
> + pr_debug("%s: freeing 0x%x (2^%d) at offset 0x%x\n",
> + __FUNCTION__, num, order, offset);
> +
> + spin_lock_irqsave(&ipic_msi_bitmap_lock, flags);
> + bitmap_release_region(msi->ipic_msi_bitmap, offset, order);
> + spin_unlock_irqrestore(&ipic_msi_bitmap_lock, flags);
> +}
> +
> +static int ipic_msi_init_allocator(struct ipic_msi *msi)
> +{
> + int size;
> +
> + size = BITS_TO_LONGS(msi->nr_msir * msi->int_per_msir) * sizeof(long);
> + msi->ipic_msi_bitmap = alloc_maybe_bootmem(size, GFP_KERNEL);
> +
> + if (msi->ipic_msi_bitmap == NULL) {
> + pr_debug("%s: ENOMEM allocating allocator bitmap!\n",
> + __FUNCTION__);
> + return -ENOMEM;
> + }
> + memset(msi->ipic_msi_bitmap, 0, size);
> +
> + return 0;
> +}
The last three routines are almost identical to the mpic_msi.c ones, if
in future we have a third implementation that looks almost identical
maybe we should consolidate them.
> +
> +static void ipic_msi_compose_msg(struct ipic_msi *msi, int hwirq,
> + struct msi_msg *msg)
> +{
> + unsigned int srs;
> + unsigned int ibs;
> +
> + srs = hwirq / msi->int_per_msir;
> + ibs = hwirq - srs * msi->int_per_msir;
> +
> + msg->address_lo = msi->msi_addr_lo;
> + msg->address_hi = msi->msi_addr_hi;
> + msg->data = (srs << 5) | (ibs & 0x1F);
> +
> + pr_debug("%s: allocated srs: %d, ibs: %d\n",
> + __FUNCTION__, srs, ibs);
> +
> +}
> +
> +static int ipic_msi_setup_irqs(struct pci_dev *pdev, int nvec, int type)
> +{
> + struct ipic_msi *msi = ipic_msi;
> + irq_hw_number_t hwirq;
> + unsigned int virq;
> + struct msi_desc *entry;
> + struct msi_msg msg;
> +
> + list_for_each_entry(entry, &pdev->msi_list, list) {
> + hwirq = ipic_msi_alloc_hwirqs(msi, 1);
> + if (hwirq < 0) {
> + pr_debug("%s: fail allocating msi interrupt\n",
> + __FUNCTION__);
> + return hwirq;
> + }
> +
> + /* This hwirq belongs to the irq_host other than irq_host of IPIC
> + * So, it is independent to hwirq of IPIC */
> + virq = irq_create_mapping(msi->irqhost, hwirq);
> + if (virq == NO_IRQ) {
> + pr_debug("%s: fail mapping hwirq 0x%lx\n",
> + __FUNCTION__, hwirq);
> + ipic_msi_free_hwirqs(msi, hwirq, 1);
> + return -ENOSPC;
> + }
> + set_irq_msi(virq, entry);
> + ipic_msi_compose_msg(msi, hwirq, &msg);
> + write_msi_msg(virq, &msg);
> +
> + hwirq++;
^^^^ this looks like my bug
> + }
> +
> + return 0;
> +}
> +
> +static void ipic_msi_teardown_irqs(struct pci_dev *pdev)
> +{
> + struct msi_desc *entry;
> + struct ipic_msi *msi = ipic_msi;
> +
> + list_for_each_entry(entry, &pdev->msi_list, list) {
> + if (entry->irq == NO_IRQ)
> + continue;
> + set_irq_msi(entry->irq, NULL);
> + ipic_msi_free_hwirqs(msi, virq_to_hw(entry->irq), 1);
> + irq_dispose_mapping(entry->irq);
> + }
> +
> + return;
> +}
> +
> +void __init ipic_msi_init(struct device_node *node,
> + void (*handler)(unsigned int irq, struct irq_desc *desc))
> +{
> + struct ipic_msi *msi;
> + struct resource res;
> + int i;
> + int rc = 0;
> +
> + msi = alloc_maybe_bootmem(sizeof(*msi), GFP_KERNEL);
> + if (msi == NULL)
> + return;
> +
> + memset(msi, 0, sizeof(*msi));
> +
> +
> + msi->irqhost = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LINEAR,
> + NR_MSIR, &ipic_msi_host_ops, 0);
> + if (msi->irqhost == NULL) {
> + of_node_put(node);
> + goto out;
> + }
> +
> + rc = of_address_to_resource(node, 0, &res);
> + if (rc) {
> + of_node_put(node);
> + goto out;
> + }
> +
> + msi->regs = ioremap(res.start, res.end - res.start + 1);
> + msi->irqhost->host_data = msi;
> + msi->hc_irq = &ipic_msi_chip;
> +
> + msi->msi_addr_lo = 0xE00007F8;
> + msi->msi_addr_hi = 0;
> + msi->nr_msir = ARRAY_SIZE(msi->msir);
> + msi->int_per_msir = 32;
> + for (i = 0; i < msi->nr_msir; i++) {
> + unsigned int virt_msir = irq_of_parse_and_map(node, i);
> + if (virt_msir != NO_IRQ) {
> + set_irq_data(virt_msir, msi);
> + set_irq_chained_handler(virt_msir, handler);
> + msi->msir[i] = virt_msir;
> + } else
> + msi->msir[i] = NO_IRQ;
> + }
> +
> + rc = ipic_msi_init_allocator(msi);
> + if (rc)
< missing of_node_put() ? >
> + goto out;
> +
> + ipic_msi = msi;
> +
> + WARN_ON(ppc_md.setup_msi_irqs);
> + ppc_md.setup_msi_irqs = ipic_msi_setup_irqs;
> + ppc_md.teardown_msi_irqs = ipic_msi_teardown_irqs;
> +
return ?
> +out:
> + if (mem_init_done)
> + kfree(msi);
> +
> + return;
> +}
> +
> +static int ipic_msi_get_irq(struct ipic_msi *msi, int virt_msir)
> +{
> + int msir = -1;
> + unsigned int temp;
> + unsigned int offset;
> + int i;
> +
> + for (i = 0; i < msi->nr_msir; i++)
> + if (virt_msir == msi->msir[i]) {
> + msir = i;
> + break;
> + }
> +
> + if (i >= msi->nr_msir)
> + return NO_IRQ;
> +
> + temp = ipic_msi_read(msi->regs, IPIC_MSIR0 + (i * 4));
> + offset = ffs(temp) - 1;
> +
> + return irq_linear_revmap(msi->irqhost, (msir * msi->int_per_msir + offset));
> +}
> +
> +void ipic_msi_cascade(unsigned int irq, struct irq_desc *desc)
> +{
> + struct ipic_msi *msi;
> + unsigned int cascade_irq;
> +
> + spin_lock(&desc->lock);
> + if (desc->chip->mask_ack)
> + desc->chip->mask_ack(irq);
> + else {
> + desc->chip->mask(irq);
> + desc->chip->ack(irq);
> + }
> +
> + if (unlikely(desc->status & IRQ_INPROGRESS))
> + goto unlock;
> +
> + desc->status |= IRQ_INPROGRESS;
> + msi = desc->handler_data;
> + cascade_irq = ipic_msi_get_irq(msi, irq);
> +
> + spin_unlock(&desc->lock);
> +
> + if (cascade_irq != NO_IRQ)
> + generic_handle_irq(cascade_irq);
> +
> + spin_lock(&desc->lock);
> + desc->status &= ~IRQ_INPROGRESS;
> + if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
> + desc->chip->unmask(irq);
> +
> +unlock:
> + spin_unlock(&desc->lock);
> +}
I don't know your hardware, but this looks a bit weird. Doesn't the
upstream handler do most of this logic for you?
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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-12-03 1:52 ` David Gibson
@ 2007-12-03 4:59 ` Li Li
0 siblings, 0 replies; 13+ messages in thread
From: Li Li @ 2007-12-03 4:59 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev, Gala Kumar, Li Tony
On Mon, 2007-12-03 at 09:52 +0800, David Gibson wrote:
> On Fri, Nov 30, 2007 at 11:48:39AM +0800, Li Li wrote:
> > The IPIC MSI is introduced on MPC837x chip.
> > Implements the IPIC MSI as two level interrupt controller.
> >
> > Signed-off-by: Tony Li <tony.li@freescale.com>
> > ---
> > arch/powerpc/boot/dts/mpc8377_mds.dts | 14 ++
> > arch/powerpc/boot/dts/mpc8378_mds.dts | 14 ++
> > arch/powerpc/boot/dts/mpc8379_mds.dts | 14 ++
> > arch/powerpc/platforms/83xx/Kconfig | 6 +
> > arch/powerpc/platforms/83xx/mpc837x_mds.c | 11 +
> > arch/powerpc/sysdev/Makefile | 1 +
> > arch/powerpc/sysdev/ipic_msi.c | 359
> +++++++++++++++++++++++++++++
> > include/asm-powerpc/ipic_msi.h | 54 +++++
> > 8 files changed, 473 insertions(+), 0 deletions(-)
> > create mode 100644 arch/powerpc/sysdev/ipic_msi.c
> > create mode 100644 include/asm-powerpc/ipic_msi.h
> >
> > diff --git a/arch/powerpc/boot/dts/mpc8377_mds.dts
> b/arch/powerpc/boot/dts/mpc8377_mds.dts
> > index 1f7819e..1068fe2 100644
> > --- a/arch/powerpc/boot/dts/mpc8377_mds.dts
> > +++ b/arch/powerpc/boot/dts/mpc8377_mds.dts
> > @@ -210,6 +210,20 @@
> > #interrupt-cells = <2>;
> > reg = <700 100>;
> > };
> > +
> > + ipic-msi@7c0 {
> > + compatible = "fsl,ipic-msi";
> > + reg = <7c0 40>;
> > + interrupts = < 43 8
> > + 4 8
> > + 51 8
> > + 52 8
> > + 56 8
> > + 57 8
> > + 58 8
> > + 59 8 >;
> > + interrupt-parent = < &ipic >;
> > + };
>
> I'm not sure why the MSI needs a device node of its own. What actual
> piece of hardware is this representing?
>
The IPIC MSI has its own registers which control the mask and ack of MSI
interrupt. Furthermore, IPIC MSI is the second level interrupt
coontorller which connected to IPIC, to compatible with IPIC OF routine
interface, we need a device node.
> --
> David Gibson | I'll have my music baroque, and my
> code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>
- Tony
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-12-03 4:02 ` Michael Ellerman
@ 2007-12-03 9:07 ` Li Li
2007-12-03 21:03 ` Benjamin Herrenschmidt
2007-12-04 5:38 ` Michael Ellerman
0 siblings, 2 replies; 13+ messages in thread
From: Li Li @ 2007-12-03 9:07 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev, Gala Kumar, Li Tony
Hi Michael,
I emulate mpic to write this IPIC MSI routines. :)
> > diff --git a/arch/powerpc/platforms/83xx/mpc837x_mds.c b/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > index 6048f1b..dbea34b 100644
> > --- a/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > +++ b/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > @@ -17,6 +17,9 @@
> >
> > #include <asm/time.h>
> > #include <asm/ipic.h>
> > +#if CONFIG_IPIC_MSI
> > +#include <asm/ipic_msi.h>
> > +#endif
> > #include <asm/udbg.h>
> > #include <asm/prom.h>
>
> I'd rather you just include it unconditionally.
Yes. that is ok for me.
>
> > @@ -84,6 +87,14 @@ static void __init mpc837x_mds_init_IRQ(void)
> > * in case the boot rom changed something on us.
> > */
> > ipic_set_default_priority();
> > +
> > +#if CONFIG_IPIC_MSI
> > + np = of_find_compatible_node(NULL, NULL, "fsl,ipic-msi");
> > + if (!np)
> > + return;
> > +
> > + ipic_msi_init(np, ipic_msi_cascade);
> > +#endif
>
> If you have a no-op version of ipic_msi_init() in your header file then
> you can remove the #ifdef in the C code - which I think is nicer.
>
Seems you do not like #ifdef. :) I agree it.
So, I move this #ifdef into header file.
> Why do you pass the handler into the init routine, rather than have the
> init routine just set the handler. Then you could make the handler
> static.
>
In IPIC, the 8 MSI interrupts is handled as level intrrupt.
I just provide a versatile in case it is changed.
> > }
> >
> > /*
> > diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> > index 99a77d7..5946b6a 100644
> > --- a/arch/powerpc/sysdev/Makefile
> > +++ b/arch/powerpc/sysdev/Makefile
> > @@ -25,6 +25,7 @@ ifeq ($(CONFIG_PPC_MERGE),y)
> > obj-$(CONFIG_PPC_INDIRECT_PCI) += indirect_pci.o
> > obj-$(CONFIG_PPC_I8259) += i8259.o
> > obj-$(CONFIG_PPC_83xx) += ipic.o
> > +obj-$(CONFIG_IPIC_MSI) += ipic_msi.o
> > obj-$(CONFIG_4xx) += uic.o
> > obj-$(CONFIG_XILINX_VIRTEX) += xilinx_intc.o
> > endif
> > diff --git a/arch/powerpc/sysdev/ipic_msi.c b/arch/powerpc/sysdev/ipic_msi.c
> > new file mode 100644
> > index 0000000..57758f7
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/ipic_msi.c
> > @@ -0,0 +1,359 @@
> > +/*
> > + * arch/powerpc/sysdev/ipic_msi.c
> > + *
> > + * IPIC MSI routines implementations.
> > + *
> > + * Auther: Tony Li <tony.li@freescale.com>
> > + *
> > + * Copyright (c) 2007 Freescale Semiconductor, Inc.
> > + *
> > + * 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.
> > + */
> > +
> > +
> > +#include <linux/pci.h>
> > +#include <linux/msi.h>
> > +#include <linux/irq.h>
> > +
> > +#include <asm/ipic_msi.h>
> > +
> > +#define MSIR0 0x43
> > +#define MSIR1 0x4
> > +#define MSIR2 0x51
> > +#define MSIR3 0x52
> > +#define MSIR4 0x56
> > +#define MSIR5 0x57
> > +#define MSIR6 0x58
> > +#define MSIR7 0x59
> > +
> > +
> > +static struct ipic_msi *ipic_msi;
> > +static DEFINE_SPINLOCK(ipic_msi_lock);
> > +static DEFINE_SPINLOCK(ipic_msi_bitmap_lock);
> > +
> > +static inline u32 ipic_msi_read(volatile u32 __iomem *base, unsigned int reg)
> > +{
> > + return in_be32(base + (reg >> 2));
> > +}
> > +
> > +static inline void ipic_msi_write(volatile u32 __iomem *base,
> > + unsigned int reg, u32 value)
> > +{
> > + out_be32(base + (reg >> 2), value);
> > +}
> > +
> > +static struct ipic_msi * ipic_msi_from_irq(unsigned int virq)
> > +{
> > + return ipic_msi;
> > +}
> > +
> > +#define ipic_msi_irq_to_hw(virq) ((unsigned int)irq_map[virq].hwirq)
>
> What's wrong with virq_to_hw() ?
>
viqr_to_hw is not __inline__.
>
> > +static void ipic_msi_unmask(unsigned int virq)
> > +{
> > + struct ipic_msi *msi = ipic_msi_from_irq(virq);
> > + unsigned int src = ipic_msi_irq_to_hw(virq);
> > + unsigned long flags;
> > + u32 temp;
> > +
> > + spin_lock_irqsave(&ipic_msi_lock, flags);
> > + temp = ipic_msi_read(msi->regs, IPIC_MSIMR);
> > + ipic_msi_write(msi->regs, IPIC_MSIMR,
> > + temp & ~(1 << (src / msi->int_per_msir)));
> > +
> > + spin_unlock_irqrestore(&ipic_msi_lock, flags);
> > +}
> > +
> > +static void ipic_msi_mask(unsigned int virq)
> > +{
> > + struct ipic_msi *msi = ipic_msi_from_irq(virq);
> > + unsigned int src = ipic_msi_irq_to_hw(virq);
> > + unsigned long flags;
> > + u32 temp;
> > +
> > + spin_lock_irqsave(&ipic_msi_lock, flags);
> > +
> > + temp = ipic_msi_read(msi->regs, IPIC_MSIMR);
> > + ipic_msi_write(msi->regs, IPIC_MSIMR,
> > + temp | (1 << (src / msi->int_per_msir)));
> > +
> > + spin_unlock_irqrestore(&ipic_msi_lock, flags);
> > +}
> > +/*
> > + * We do not need this actually. The MSIR register has been read once
> > + * in ipic_msi_get_irq. So, this MSI interrupt has been acked
> > + */
> > +static void ipic_msi_ack(unsigned int virq)
> > +{
> > + struct ipic_msi *msi = ipic_msi_from_irq(virq);
> > + unsigned int src = ipic_msi_irq_to_hw(virq);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&ipic_msi_lock, flags);
> > +
> > + ipic_msi_read(msi->regs, IPIC_MSIR0 + (4 * (src / msi->int_per_msir)));
> > +
> > + spin_unlock_irqrestore(&ipic_msi_lock,flags);
> > +}
> > +
> > +static struct irq_chip ipic_msi_chip = {
> > + .typename = " IPIC MSI ",
> > + .unmask = ipic_msi_unmask,
> > + .mask = ipic_msi_mask,
> > + .ack = ipic_msi_ack,
> > +};
> > +
> > +static int ipic_msi_host_map(struct irq_host *h, unsigned int virq,
> > + irq_hw_number_t hw)
> > +{
> > + struct ipic_msi *msi = h->host_data;
> > + struct irq_chip *chip = msi->hc_irq;
> > +
> > + set_irq_chip_data(virq, msi);
> > + get_irq_desc(virq)->status |= IRQ_TYPE_EDGE_FALLING;
> > +
> > + set_irq_chip_and_handler(virq, chip, handle_edge_irq);
> > + return 0;
> > +}
> > +
> > +static struct irq_host_ops ipic_msi_host_ops = {
> > + .map = ipic_msi_host_map,
> > +};
> > +
> > +irq_hw_number_t ipic_msi_alloc_hwirqs(struct ipic_msi *msi, int num)
> > +{
> > + unsigned long flags;
> > + int offset, order = get_count_order(num);
> > +
> > + spin_lock_irqsave(&ipic_msi_bitmap_lock, flags);
> > +
> > + offset = bitmap_find_free_region(msi->ipic_msi_bitmap,
> > + msi->nr_msir * msi->int_per_msir, order);
> > +
> > + spin_unlock_irqrestore(&ipic_msi_bitmap_lock, flags);
> > +
> > + pr_debug("%s: allocated 0x%x (2^%d) at offset 0x%x\n",
> > + __FUNCTION__, num, order, offset);
> > +
> > + return offset;
> > +}
> > +
> > +void ipic_msi_free_hwirqs(struct ipic_msi *msi, int offset, int num)
> > +{
> > + unsigned long flags;
> > + int order = get_count_order(num);
> > +
> > + pr_debug("%s: freeing 0x%x (2^%d) at offset 0x%x\n",
> > + __FUNCTION__, num, order, offset);
> > +
> > + spin_lock_irqsave(&ipic_msi_bitmap_lock, flags);
> > + bitmap_release_region(msi->ipic_msi_bitmap, offset, order);
> > + spin_unlock_irqrestore(&ipic_msi_bitmap_lock, flags);
> > +}
> > +
> > +static int ipic_msi_init_allocator(struct ipic_msi *msi)
> > +{
> > + int size;
> > +
> > + size = BITS_TO_LONGS(msi->nr_msir * msi->int_per_msir) * sizeof(long);
> > + msi->ipic_msi_bitmap = alloc_maybe_bootmem(size, GFP_KERNEL);
> > +
> > + if (msi->ipic_msi_bitmap == NULL) {
> > + pr_debug("%s: ENOMEM allocating allocator bitmap!\n",
> > + __FUNCTION__);
> > + return -ENOMEM;
> > + }
> > + memset(msi->ipic_msi_bitmap, 0, size);
> > +
> > + return 0;
> > +}
>
> The last three routines are almost identical to the mpic_msi.c ones, if
> in future we have a third implementation that looks almost identical
> maybe we should consolidate them.
>
Yes. I agree.
> > +
> > +static void ipic_msi_compose_msg(struct ipic_msi *msi, int hwirq,
> > + struct msi_msg *msg)
> > +{
> > + unsigned int srs;
> > + unsigned int ibs;
> > +
> > + srs = hwirq / msi->int_per_msir;
> > + ibs = hwirq - srs * msi->int_per_msir;
> > +
> > + msg->address_lo = msi->msi_addr_lo;
> > + msg->address_hi = msi->msi_addr_hi;
> > + msg->data = (srs << 5) | (ibs & 0x1F);
> > +
> > + pr_debug("%s: allocated srs: %d, ibs: %d\n",
> > + __FUNCTION__, srs, ibs);
> > +
> > +}
> > +
> > +static int ipic_msi_setup_irqs(struct pci_dev *pdev, int nvec, int type)
> > +{
> > + struct ipic_msi *msi = ipic_msi;
> > + irq_hw_number_t hwirq;
> > + unsigned int virq;
> > + struct msi_desc *entry;
> > + struct msi_msg msg;
> > +
> > + list_for_each_entry(entry, &pdev->msi_list, list) {
> > + hwirq = ipic_msi_alloc_hwirqs(msi, 1);
> > + if (hwirq < 0) {
> > + pr_debug("%s: fail allocating msi interrupt\n",
> > + __FUNCTION__);
> > + return hwirq;
> > + }
> > +
> > + /* This hwirq belongs to the irq_host other than irq_host of IPIC
> > + * So, it is independent to hwirq of IPIC */
> > + virq = irq_create_mapping(msi->irqhost, hwirq);
> > + if (virq == NO_IRQ) {
> > + pr_debug("%s: fail mapping hwirq 0x%lx\n",
> > + __FUNCTION__, hwirq);
> > + ipic_msi_free_hwirqs(msi, hwirq, 1);
> > + return -ENOSPC;
> > + }
> > + set_irq_msi(virq, entry);
> > + ipic_msi_compose_msg(msi, hwirq, &msg);
> > + write_msi_msg(virq, &msg);
> > +
> > + hwirq++;
>
> ^^^^ this looks like my bug
I have a question here. Do we support more MSI interrupts on ONE pci
device?
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void ipic_msi_teardown_irqs(struct pci_dev *pdev)
> > +{
> > + struct msi_desc *entry;
> > + struct ipic_msi *msi = ipic_msi;
> > +
> > + list_for_each_entry(entry, &pdev->msi_list, list) {
> > + if (entry->irq == NO_IRQ)
> > + continue;
> > + set_irq_msi(entry->irq, NULL);
> > + ipic_msi_free_hwirqs(msi, virq_to_hw(entry->irq), 1);
> > + irq_dispose_mapping(entry->irq);
> > + }
> > +
> > + return;
> > +}
> > +
> > +void __init ipic_msi_init(struct device_node *node,
> > + void (*handler)(unsigned int irq, struct irq_desc *desc))
> > +{
> > + struct ipic_msi *msi;
> > + struct resource res;
> > + int i;
> > + int rc = 0;
> > +
> > + msi = alloc_maybe_bootmem(sizeof(*msi), GFP_KERNEL);
> > + if (msi == NULL)
> > + return;
> > +
> > + memset(msi, 0, sizeof(*msi));
> > +
> > +
> > + msi->irqhost = irq_alloc_host(of_node_get(node), IRQ_HOST_MAP_LINEAR,
> > + NR_MSIR, &ipic_msi_host_ops, 0);
> > + if (msi->irqhost == NULL) {
> > + of_node_put(node);
> > + goto out;
> > + }
> > +
> > + rc = of_address_to_resource(node, 0, &res);
> > + if (rc) {
> > + of_node_put(node);
> > + goto out;
> > + }
> > +
> > + msi->regs = ioremap(res.start, res.end - res.start + 1);
> > + msi->irqhost->host_data = msi;
> > + msi->hc_irq = &ipic_msi_chip;
> > +
> > + msi->msi_addr_lo = 0xE00007F8;
> > + msi->msi_addr_hi = 0;
> > + msi->nr_msir = ARRAY_SIZE(msi->msir);
> > + msi->int_per_msir = 32;
> > + for (i = 0; i < msi->nr_msir; i++) {
> > + unsigned int virt_msir = irq_of_parse_and_map(node, i);
> > + if (virt_msir != NO_IRQ) {
> > + set_irq_data(virt_msir, msi);
> > + set_irq_chained_handler(virt_msir, handler);
> > + msi->msir[i] = virt_msir;
> > + } else
> > + msi->msir[i] = NO_IRQ;
> > + }
> > +
> > + rc = ipic_msi_init_allocator(msi);
> > + if (rc)
> < missing of_node_put() ? >
> > + goto out;
> > +
> > + ipic_msi = msi;
> > +
> > + WARN_ON(ppc_md.setup_msi_irqs);
> > + ppc_md.setup_msi_irqs = ipic_msi_setup_irqs;
> > + ppc_md.teardown_msi_irqs = ipic_msi_teardown_irqs;
> > +
> return ?
>
Sorry for that. It is a mistake.
> > +out:
> > + if (mem_init_done)
> > + kfree(msi);
> > +
> > + return;
> > +}
> > +
> > +static int ipic_msi_get_irq(struct ipic_msi *msi, int virt_msir)
> > +{
> > + int msir = -1;
> > + unsigned int temp;
> > + unsigned int offset;
> > + int i;
> > +
> > + for (i = 0; i < msi->nr_msir; i++)
> > + if (virt_msir == msi->msir[i]) {
> > + msir = i;
> > + break;
> > + }
> > +
> > + if (i >= msi->nr_msir)
> > + return NO_IRQ;
> > +
> > + temp = ipic_msi_read(msi->regs, IPIC_MSIR0 + (i * 4));
> > + offset = ffs(temp) - 1;
> > +
> > + return irq_linear_revmap(msi->irqhost, (msir * msi->int_per_msir + offset));
> > +}
> > +
> > +void ipic_msi_cascade(unsigned int irq, struct irq_desc *desc)
> > +{
> > + struct ipic_msi *msi;
> > + unsigned int cascade_irq;
> > +
> > + spin_lock(&desc->lock);
> > + if (desc->chip->mask_ack)
> > + desc->chip->mask_ack(irq);
> > + else {
> > + desc->chip->mask(irq);
> > + desc->chip->ack(irq);
> > + }
> > +
> > + if (unlikely(desc->status & IRQ_INPROGRESS))
> > + goto unlock;
> > +
> > + desc->status |= IRQ_INPROGRESS;
> > + msi = desc->handler_data;
> > + cascade_irq = ipic_msi_get_irq(msi, irq);
> > +
> > + spin_unlock(&desc->lock);
> > +
> > + if (cascade_irq != NO_IRQ)
> > + generic_handle_irq(cascade_irq);
> > +
> > + spin_lock(&desc->lock);
> > + desc->status &= ~IRQ_INPROGRESS;
> > + if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
> > + desc->chip->unmask(irq);
> > +
> > +unlock:
> > + spin_unlock(&desc->lock);
> > +}
>
> I don't know your hardware, but this looks a bit weird. Doesn't the
> upstream handler do most of this logic for you?
>
I just emulate the handle_level_irq to write this.
But I need get irq number from MSI controller (ipic_msi_get_irq) and
then call interrupt routine of that irq number.
- Tony
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-12-03 9:07 ` Li Li
@ 2007-12-03 21:03 ` Benjamin Herrenschmidt
2007-12-04 1:41 ` Li Li
2007-12-04 5:38 ` Michael Ellerman
1 sibling, 1 reply; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2007-12-03 21:03 UTC (permalink / raw)
To: Li Li; +Cc: linuxppc-dev, Gala Kumar, Li Tony
On Mon, 2007-12-03 at 17:07 +0800, Li Li wrote:
>
> In IPIC, the 8 MSI interrupts is handled as level intrrupt.
> I just provide a versatile in case it is changed.
Level ? Are you sure ? MSIs are by definition edge interrupts... Or do
you have some weird logic that asserts a level input until you go ack
something ? Sounds weird...
Ben.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-12-03 21:03 ` Benjamin Herrenschmidt
@ 2007-12-04 1:41 ` Li Li
0 siblings, 0 replies; 13+ messages in thread
From: Li Li @ 2007-12-04 1:41 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, Gala Kumar, Li Tony
On Tue, 2007-12-04 at 05:03 +0800, Benjamin Herrenschmidt wrote:
>
> On Mon, 2007-12-03 at 17:07 +0800, Li Li wrote:
> >
> > In IPIC, the 8 MSI interrupts is handled as level intrrupt.
> > I just provide a versatile in case it is changed.
>
> Level ? Are you sure ? MSIs are by definition edge interrupts... Or
> do
> you have some weird logic that asserts a level input until you go ack
> something ? Sounds weird...
>
The second one.
The MSI is edge interrupt. The 256 MSI interrupts are divided into 8
groups. Each group can generate a interrupt to core. This interrupts are
level and asserted untile ack MSI interrupt. A little weird.
> Ben.
>
>
- Tony
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-12-03 9:07 ` Li Li
2007-12-03 21:03 ` Benjamin Herrenschmidt
@ 2007-12-04 5:38 ` Michael Ellerman
2007-12-04 7:51 ` Benjamin Herrenschmidt
2007-12-04 10:34 ` Li Tony
1 sibling, 2 replies; 13+ messages in thread
From: Michael Ellerman @ 2007-12-04 5:38 UTC (permalink / raw)
To: Li Li; +Cc: linuxppc-dev, Gala Kumar, Li Tony
[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]
On Mon, 2007-12-03 at 17:07 +0800, Li Li wrote:
> Hi Michael,
>
> I emulate mpic to write this IPIC MSI routines. :)
>
>
> > > diff --git a/arch/powerpc/platforms/83xx/mpc837x_mds.c b/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > > index 6048f1b..dbea34b 100644
> > > --- a/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > > +++ b/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > > +
> > > +#define ipic_msi_irq_to_hw(virq) ((unsigned int)irq_map[virq].hwirq)
> >
> > What's wrong with virq_to_hw() ?
> >
>
> viqr_to_hw is not __inline__.
Hmm, ok. The three places you use it you also take a spin lock, so I'm
not sure the function call's really going to kill you performance wise.
> > > +
> > > +static void ipic_msi_compose_msg(struct ipic_msi *msi, int hwirq,
> > > + struct msi_msg *msg)
> > > +{
> > > + unsigned int srs;
> > > + unsigned int ibs;
> > > +
> > > + srs = hwirq / msi->int_per_msir;
> > > + ibs = hwirq - srs * msi->int_per_msir;
> > > +
> > > + msg->address_lo = msi->msi_addr_lo;
> > > + msg->address_hi = msi->msi_addr_hi;
> > > + msg->data = (srs << 5) | (ibs & 0x1F);
> > > +
> > > + pr_debug("%s: allocated srs: %d, ibs: %d\n",
> > > + __FUNCTION__, srs, ibs);
> > > +
> > > +}
> > > +
> > > +static int ipic_msi_setup_irqs(struct pci_dev *pdev, int nvec, int type)
> > > +{
> > > + struct ipic_msi *msi = ipic_msi;
> > > + irq_hw_number_t hwirq;
> > > + unsigned int virq;
> > > + struct msi_desc *entry;
> > > + struct msi_msg msg;
> > > +
> > > + list_for_each_entry(entry, &pdev->msi_list, list) {
> > > + hwirq = ipic_msi_alloc_hwirqs(msi, 1);
> > > + if (hwirq < 0) {
> > > + pr_debug("%s: fail allocating msi interrupt\n",
> > > + __FUNCTION__);
> > > + return hwirq;
> > > + }
> > > +
> > > + /* This hwirq belongs to the irq_host other than irq_host of IPIC
> > > + * So, it is independent to hwirq of IPIC */
> > > + virq = irq_create_mapping(msi->irqhost, hwirq);
> > > + if (virq == NO_IRQ) {
> > > + pr_debug("%s: fail mapping hwirq 0x%lx\n",
> > > + __FUNCTION__, hwirq);
> > > + ipic_msi_free_hwirqs(msi, hwirq, 1);
> > > + return -ENOSPC;
> > > + }
> > > + set_irq_msi(virq, entry);
> > > + ipic_msi_compose_msg(msi, hwirq, &msg);
> > > + write_msi_msg(virq, &msg);
> > > +
> > > + hwirq++;
> >
> > ^^^^ this looks like my bug
>
> I have a question here. Do we support more MSI interrupts on ONE pci
> device?
I'm not sure what you mean? For MSI there is only one MSI per device,
but this code is used also for MSI-X which supports > 1 MSI per device.
Either way we shouldn't be incrementing hwirq by hand, it's reassigned
at the top of the loop. I think that's left over from old code that
allocated nvec hwirqs in a block and then created virq mappings for each
one, whereas the new code allocates each hwirq separately.
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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-12-04 5:38 ` Michael Ellerman
@ 2007-12-04 7:51 ` Benjamin Herrenschmidt
2007-12-04 9:10 ` Li Li
2007-12-04 10:34 ` Li Tony
1 sibling, 1 reply; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2007-12-04 7:51 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev, Gala Kumar, Li Li, Li Tony
> I'm not sure what you mean? For MSI there is only one MSI per device,
> but this code is used also for MSI-X which supports > 1 MSI per device.
Or more specifically, for MSI, -linux- supports only one per device (in
theory, it's possible to have multiple MSI non-X but it's a mess).
> Either way we shouldn't be incrementing hwirq by hand, it's reassigned
> at the top of the loop. I think that's left over from old code that
> allocated nvec hwirqs in a block and then created virq mappings for each
> one, whereas the new code allocates each hwirq separately.
>
> cheers
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-12-04 7:51 ` Benjamin Herrenschmidt
@ 2007-12-04 9:10 ` Li Li
0 siblings, 0 replies; 13+ messages in thread
From: Li Li @ 2007-12-04 9:10 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, Gala Kumar, Li Tony
Yes. According to the PCI spec, a PCI device can request multi MSI
interrupts and require that interrupts are consecutive.
But it is ok if only allocate one to it.
Anyway, the hwirq should be allocated from bitmap instead of increment
by hand.
I will correct this and resend the patch.
- Tony
On Tue, 2007-12-04 at 15:51 +0800, Benjamin Herrenschmidt wrote:
> > I'm not sure what you mean? For MSI there is only one MSI per
> device,
> > but this code is used also for MSI-X which supports > 1 MSI per
> device.
>
> Or more specifically, for MSI, -linux- supports only one per device
> (in
> theory, it's possible to have multiple MSI non-X but it's a mess).
>
> > Either way we shouldn't be incrementing hwirq by hand, it's
> reassigned
> > at the top of the loop. I think that's left over from old code that
> > allocated nvec hwirqs in a block and then created virq mappings for
> each
> > one, whereas the new code allocates each hwirq separately.
> >
> > cheers
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@ozlabs.org
> > https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] Add IPIC MSI interrupt support
2007-12-04 5:38 ` Michael Ellerman
2007-12-04 7:51 ` Benjamin Herrenschmidt
@ 2007-12-04 10:34 ` Li Tony
2007-12-04 12:04 ` Michael Ellerman
1 sibling, 1 reply; 13+ messages in thread
From: Li Tony @ 2007-12-04 10:34 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev, Gala Kumar
=20
> -----Original Message-----
> From: Michael Ellerman [mailto:michael@ellerman.id.au]=20
> Sent: 2007=C4=EA12=D4=C24=C8=D5 13:38
> To: Li Tony
> Cc: Li Tony; Gala Kumar; linuxppc-dev
> Subject: Re: [PATCH] Add IPIC MSI interrupt support
>=20
> On Mon, 2007-12-03 at 17:07 +0800, Li Li wrote:
> > Hi Michael,
> >=20
> > I emulate mpic to write this IPIC MSI routines. :)
> >=20
> >=20
> > > > diff --git a/arch/powerpc/platforms/83xx/mpc837x_mds.c=20
> > > > b/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > > > index 6048f1b..dbea34b 100644
> > > > --- a/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > > > +++ b/arch/powerpc/platforms/83xx/mpc837x_mds.c
>=20
> > > > +
> > > > +#define ipic_msi_irq_to_hw(virq)=09
> ((unsigned int)irq_map[virq].hwirq)
> > >=20
> > > What's wrong with virq_to_hw() ?
> > >=20
> >=20
> > viqr_to_hw is not __inline__.
>=20
> Hmm, ok. The three places you use it you also take a spin=20
> lock, so I'm not sure the function call's really going to=20
> kill you performance wise.
>=20
I am not very sure about spin_lock influence.
But maybe somebody will change the virq_to_hw implementation.
I will take virq_to_hw instead.=20
I see that the virq_to_hw is do inline in 2.6.22.
Why remove it?
> > > > +
> > > > +static void ipic_msi_compose_msg(struct ipic_msi *msi,=20
> int hwirq,
> > > > + struct=20
> msi_msg *msg)
> > > > +{
> > > > + unsigned int srs;
> > > > + unsigned int ibs;
> > > > +
> > > > + srs =3D hwirq / msi->int_per_msir;
> > > > + ibs =3D hwirq - srs * msi->int_per_msir;
> > > > +
> > > > + msg->address_lo =3D msi->msi_addr_lo;
> > > > + msg->address_hi =3D msi->msi_addr_hi;
> > > > + msg->data =3D (srs << 5) | (ibs & 0x1F);
> > > > +
> > > > + pr_debug("%s: allocated srs: %d, ibs: %d\n",
> > > > + __FUNCTION__, srs, ibs);
> > > > +
> > > > +}
> > > > +
> > > > +static int ipic_msi_setup_irqs(struct pci_dev *pdev, int nvec,=20
> > > > +int type) {
> > > > + struct ipic_msi *msi =3D ipic_msi;
> > > > + irq_hw_number_t hwirq;
> > > > + unsigned int virq;
> > > > + struct msi_desc *entry;
> > > > + struct msi_msg msg;
> > > > +
> > > > + list_for_each_entry(entry, &pdev->msi_list, list) {
> > > > + hwirq =3D ipic_msi_alloc_hwirqs(msi, 1);
> > > > + if (hwirq < 0) {
> > > > + pr_debug("%s: fail allocating=20
> msi interrupt\n",
> > > > + __FUNCTION__);
> > > > + return hwirq;
> > > > + }
> > > > +
> > > > + /* This hwirq belongs to the irq_host=20
> other than irq_host of IPIC
> > > > + * So, it is independent to hwirq of IPIC */
> > > > + virq =3D irq_create_mapping(msi->irqhost, hwirq);
> > > > + if (virq =3D=3D NO_IRQ) {
> > > > + pr_debug("%s: fail mapping=20
> hwirq 0x%lx\n",
> > > > + __FUNCTION__, hwirq);
> > > > + ipic_msi_free_hwirqs(msi, hwirq, 1);
> > > > + return -ENOSPC;
> > > > + }
> > > > + set_irq_msi(virq, entry);
> > > > + ipic_msi_compose_msg(msi, hwirq, &msg);
> > > > + write_msi_msg(virq, &msg);
> > > > +
> > > > + hwirq++;
> > >=20
> > > ^^^^ this looks like my bug
> >=20
> > I have a question here. Do we support more MSI interrupts=20
> on ONE pci=20
> > device?
>=20
> I'm not sure what you mean? For MSI there is only one MSI per=20
> device, but this code is used also for MSI-X which supports >=20
> 1 MSI per device.
>=20
> Either way we shouldn't be incrementing hwirq by hand, it's=20
> reassigned at the top of the loop. I think that's left over=20
> from old code that allocated nvec hwirqs in a block and then=20
> created virq mappings for each one, whereas the new code=20
> allocates each hwirq separately.
>=20
> cheers
>=20
> --
> Michael Ellerman
> OzLabs, IBM Australia Development Lab
>=20
> wwweb: http://michael.ellerman.id.au
> phone: +61 2 6212 1183 (tie line 70 21183)
>=20
> We do not inherit the earth from our ancestors, we borrow it=20
> from our children. - S.M.A.R.T Person
>=20
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] Add IPIC MSI interrupt support
2007-12-04 10:34 ` Li Tony
@ 2007-12-04 12:04 ` Michael Ellerman
2007-12-04 13:13 ` Scott Wood
0 siblings, 1 reply; 13+ messages in thread
From: Michael Ellerman @ 2007-12-04 12:04 UTC (permalink / raw)
To: Li Tony; +Cc: linuxppc-dev, Gala Kumar
[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]
On Tue, 2007-12-04 at 18:34 +0800, Li Tony wrote:
> > From: Michael Ellerman [mailto:michael@ellerman.id.au]
> > Sent: 2007年12月4日 13:38
> > To: Li Tony
> > Cc: Li Tony; Gala Kumar; linuxppc-dev
> > Subject: Re: [PATCH] Add IPIC MSI interrupt support
> >
> > On Mon, 2007-12-03 at 17:07 +0800, Li Li wrote:
> > > Hi Michael,
> > >
> > > I emulate mpic to write this IPIC MSI routines. :)
> > >
> > >
> > > > > diff --git a/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > > > > b/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > > > > index 6048f1b..dbea34b 100644
> > > > > --- a/arch/powerpc/platforms/83xx/mpc837x_mds.c
> > > > > +++ b/arch/powerpc/platforms/83xx/mpc837x_mds.c
> >
> > > > > +
> > > > > +#define ipic_msi_irq_to_hw(virq)
> > ((unsigned int)irq_map[virq].hwirq)
> > > >
> > > > What's wrong with virq_to_hw() ?
> > > >
> > >
> > > viqr_to_hw is not __inline__.
> >
> > Hmm, ok. The three places you use it you also take a spin
> > lock, so I'm not sure the function call's really going to
> > kill you performance wise.
> >
>
> I am not very sure about spin_lock influence.
> But maybe somebody will change the virq_to_hw implementation.
> I will take virq_to_hw instead.
I mean the time to take the function call should be pretty small
compared to taking and releasing a spinlock - but if you have
performance numbers to prove otherwise let me know :)
> I see that the virq_to_hw is do inline in 2.6.22.
> Why remove it?
It was made non-inline because modules want to use it, and we didn't
want to export the irq_map to modules as well.
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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Add IPIC MSI interrupt support
2007-12-04 12:04 ` Michael Ellerman
@ 2007-12-04 13:13 ` Scott Wood
0 siblings, 0 replies; 13+ messages in thread
From: Scott Wood @ 2007-12-04 13:13 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Gala Kumar, Li Tony
On Tue, Dec 04, 2007 at 11:04:18PM +1100, Michael Ellerman wrote:
> On Tue, 2007-12-04 at 18:34 +0800, Li Tony wrote:
> > I am not very sure about spin_lock influence.
> > But maybe somebody will change the virq_to_hw implementation.
> > I will take virq_to_hw instead.
>
> I mean the time to take the function call should be pretty small
> compared to taking and releasing a spinlock - but if you have
> performance numbers to prove otherwise let me know :)
I don't think the IPIC is used on any SMP platforms at the moment, so
there's no spinlock overhead other than disabling interrupts...
That said, it's not worth reimplementing to avoid a function call.
-Scott
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2007-12-04 13:13 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-11-30 3:48 [PATCH] Add IPIC MSI interrupt support Li Li
2007-12-03 1:52 ` David Gibson
2007-12-03 4:59 ` Li Li
2007-12-03 4:02 ` Michael Ellerman
2007-12-03 9:07 ` Li Li
2007-12-03 21:03 ` Benjamin Herrenschmidt
2007-12-04 1:41 ` Li Li
2007-12-04 5:38 ` Michael Ellerman
2007-12-04 7:51 ` Benjamin Herrenschmidt
2007-12-04 9:10 ` Li Li
2007-12-04 10:34 ` Li Tony
2007-12-04 12:04 ` Michael Ellerman
2007-12-04 13:13 ` Scott Wood
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).