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 <[EMAIL PROTECTED]>
> ---
> 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>;
> };
> +
> + [EMAIL PROTECTED] {
> + 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 >;
> + };
> };
>
> [EMAIL PROTECTED] {
> 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>;
> };
> +
> + [EMAIL PROTECTED] {
> + 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 >;
> + };
> };
>
> [EMAIL PROTECTED] {
> 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>;
> };
> +
> + [EMAIL PROTECTED] {
> + 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 >;
> + };
> };
>
> [EMAIL PROTECTED] {
> 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 <[EMAIL PROTECTED]>
> + *
> + * 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
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
