Re: [PATCH 3/3] Enable MSI support for 85xxds board.

Re: [PATCH 3/3] Enable MSI support for 85xxds board.

[Segher Boessenkool]

> +		msi@41600 {
> +			compatible = "fsl,MPIC-msi";

You need a more exact name here.

Also, where's the binding documentation?


Segher

[PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Jason Jin]

This MSI driver can be used on 83xx/85xx/86xx board.
In this driver, virtual interrupt host and chip were
setup. There are 256 MSI interrupts in this host, Every 32
MSI interrupts cascaded to one IPIC/MPIC interrupt.
The chip was treated as edge sensitive and some necessary
functions were setup for this chip.

Before using the MSI interrupt, PCI/PCIE device need to
ask for a MSI interrupt in the 256 MSI interrupts. A 256bit
bitmap show which MSI interrupt was used, reserve bit in
the bitmap can be used to force the device use some designate
MSI interrupt in the 256 MSI interrupts. Sometimes this is useful
for testing the all the MSI interrupts. The msi-available-ranges
property in the dts file was used for this purpose.

Signed-off-by: Jason Jin <Jason.jin@freescale.com>
---
 arch/powerpc/sysdev/Makefile  |    1 +
 arch/powerpc/sysdev/fsl_msi.c |  457 +++++++++++++++++++++++++++++++++++++++++
 arch/powerpc/sysdev/fsl_msi.h |   40 ++++
 arch/powerpc/sysdev/fsl_pci.c |   14 ++
 4 files changed, 512 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/sysdev/fsl_msi.c
 create mode 100644 arch/powerpc/sysdev/fsl_msi.h

diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
index 6d386d0..bfd3fe4 100644
--- a/arch/powerpc/sysdev/Makefile
+++ b/arch/powerpc/sysdev/Makefile
@@ -4,6 +4,7 @@ endif
 
 mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
 obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
+obj-$(CONFIG_PCI_MSI)		+= fsl_msi.o
 
 obj-$(CONFIG_PPC_MPC106)	+= grackle.o
 obj-$(CONFIG_PPC_DCR_NATIVE)	+= dcr-low.o
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
new file mode 100644
index 0000000..e8132cf
--- /dev/null
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -0,0 +1,457 @@
+/*
+ * Copyright (C) 2007-2008 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * Author: Tony Li <tony.li@freescale.com>
+ *	   Jason Jin <Jason.jin@freescale.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 of the
+ * License.
+ *
+ */
+
+#include <linux/irq.h>
+#include <linux/bootmem.h>
+#include <linux/bitmap.h>
+#include <linux/msi.h>
+#include <asm/prom.h>
+#include <asm/hw_irq.h>
+#include <linux/pci.h>
+#include <asm/ppc-pci.h>
+#include <linux/of_platform.h>
+
+#include <sysdev/fsl_soc.h>
+#include "fsl_msi.h"
+
+#ifdef DEBUG
+#define pr_debug(fmt...) printk(fmt)
+#else
+#define pr_debug(fmt...)
+#endif
+
+/* A bit ugly, can we get this from the pci_dev somehow? */
+static struct fsl_msi *fsl_msi;
+
+static inline u32 fsl_msi_read(u32 __iomem *base,
+				unsigned int reg)
+{
+	return in_be32(base + (reg >> 2));
+}
+
+static inline void fsl_msi_write(u32 __iomem *base,
+				unsigned int reg, u32 value)
+{
+	out_be32(base + (reg >> 2), value);
+}
+
+#define	fsl_msi_irq_to_hw(virq)	 ((unsigned int)irq_map[virq].hwirq)
+
+/*
+ * We do not need this actually. The MSIR register has been read once
+ * in the cascade interrupt. So, this MSI interrupt has been acked
+*/
+static void fsl_msi_end_irq(unsigned int virq)
+{
+}
+
+static struct irq_chip fsl_msi_chip = {
+	.mask		= mask_msi_irq,
+	.unmask		= unmask_msi_irq,
+	.ack		= fsl_msi_end_irq,
+	.typename	= " FSL-MSI  ",
+};
+
+static int fsl_msi_host_match(struct irq_host *h, struct device_node *node)
+{
+	struct fsl_msi *msi = h->host_data;
+
+	/* Exact match, unless node is NULL */
+	return msi->of_node == NULL || msi->of_node == node;
+}
+
+
+static int fsl_msi_host_map(struct irq_host *h, unsigned int virq,
+				irq_hw_number_t hw)
+{
+	struct fsl_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 fsl_msi_host_ops = {
+	.match = fsl_msi_host_match,
+	.map = fsl_msi_host_map,
+};
+
+irq_hw_number_t fsl_msi_alloc_hwirqs(struct fsl_msi *msi, int num)
+{
+	unsigned long flags;
+	int offset, order = get_count_order(num);
+
+	spin_lock_irqsave(&msi->bitmap_lock, flags);
+
+	offset = bitmap_find_free_region(msi->fsl_msi_bitmap,
+					NR_MSI_IRQS, order);
+
+	spin_unlock_irqrestore(&msi->bitmap_lock, flags);
+
+	pr_debug("%s: allocated 0x%x (2^%d) at offset 0x%x\n",
+		__func__, num, order, offset);
+
+	return offset;
+}
+
+void fsl_msi_free_hwirqs(struct fsl_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",
+		__func__, num, order, offset);
+
+	spin_lock_irqsave(&msi->bitmap_lock, flags);
+	bitmap_release_region(msi->fsl_msi_bitmap, offset, order);
+	spin_unlock_irqrestore(&msi->bitmap_lock, flags);
+}
+
+static int fsl_msi_reserve_dt_hwirqs(struct fsl_msi *msi)
+{
+	int i, len;
+	const u32 *p;
+
+	p = of_get_property(msi->of_node, "msi-available-ranges", &len);
+	if (!p) {
+		pr_debug("fsl_msi: no msi-available-ranges property found \
+				on %s\n", msi->of_node->full_name);
+		return -ENODEV;
+	}
+
+	if (len & 0x8 != 0) {
+		printk(KERN_WARNING "fsl_msi: Malformed msi-available-ranges "
+		       "property on %s\n", msi->of_node->full_name);
+		return -EINVAL;
+	}
+
+	bitmap_allocate_region(msi->fsl_msi_bitmap, 0,
+			       get_count_order(msi->irq_count));
+
+	/* Format is: (<u32 start> <u32 count>)+ */
+	len /= sizeof(u32);
+	len /= 2;
+	for (i = 0; i < len; i++, p += 2)
+		fsl_msi_free_hwirqs(msi, *p, *(p + 1));
+
+	return 0;
+}
+
+static int fsl_msi_init_allocator(struct fsl_msi *msi)
+{
+	int rc, size;
+
+	size = BITS_TO_LONGS(NR_MSI_IRQS) * sizeof(long);
+
+	msi->fsl_msi_bitmap = kmalloc(size, GFP_KERNEL);
+
+	if (msi->fsl_msi_bitmap == NULL) {
+		pr_debug("%s: ENOMEM allocating allocator bitmap!\n",
+				__func__);
+		return -ENOMEM;
+	}
+	memset(msi->fsl_msi_bitmap, 0, size);
+
+	rc = fsl_msi_reserve_dt_hwirqs(msi);
+	if (rc)
+		goto out_free;
+
+	return 0;
+out_free:
+	if (mem_init_done)
+		kfree(msi->fsl_msi_bitmap);
+
+	msi->fsl_msi_bitmap = NULL;
+	return rc;
+
+}
+
+static int fsl_msi_check_device(struct pci_dev *pdev, int nvec, int type)
+{
+	if (type == PCI_CAP_ID_MSIX)
+		pr_debug("fslmsi: MSI-X untested, trying anyway.\n");
+
+	return 0;
+}
+
+static void fsl_teardown_msi_irqs(struct pci_dev *pdev)
+{
+	struct msi_desc *entry;
+	struct fsl_msi *msi = fsl_msi;
+
+	list_for_each_entry(entry, &pdev->msi_list, list) {
+		if (entry->irq == NO_IRQ)
+			continue;
+		set_irq_msi(entry->irq, NULL);
+		fsl_msi_free_hwirqs(msi, virq_to_hw(entry->irq), 1);
+		irq_dispose_mapping(entry->irq);
+	}
+
+	return;
+}
+
+static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
+				  struct msi_msg *msg)
+{
+	unsigned int srs;
+	unsigned int ibs;
+	struct fsl_msi *msi = fsl_msi;
+
+	srs = hwirq / INT_PER_MSIR;
+	ibs = hwirq % 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",
+		__func__, srs, ibs);
+}
+
+static int fsl_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
+{
+	irq_hw_number_t hwirq;
+	int rc;
+	unsigned int virq;
+	struct msi_desc *entry;
+	struct msi_msg msg;
+	struct fsl_msi *msi = fsl_msi;
+
+	list_for_each_entry(entry, &pdev->msi_list, list) {
+		hwirq = fsl_msi_alloc_hwirqs(msi, 1);
+		if (hwirq < 0) {
+			rc = hwirq;
+			pr_debug("%s: fail allocating msi interrupt\n",
+					__func__);
+			goto out_free;
+		}
+
+		virq = irq_create_mapping(msi->irqhost, hwirq);
+
+		if (virq == NO_IRQ) {
+			pr_debug("%s: fail mapping hwirq 0x%lx\n",
+					__func__, hwirq);
+			fsl_msi_free_hwirqs(msi, hwirq, 1);
+			rc = -ENOSPC;
+			goto out_free;
+		}
+		set_irq_msi(virq, entry);
+
+		fsl_compose_msi_msg(pdev, hwirq, &msg);
+		write_msi_msg(virq, &msg);
+	}
+	return 0;
+
+out_free:
+	fsl_teardown_msi_irqs(pdev);
+	return rc;
+}
+
+void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
+{
+	unsigned int cascade_irq;
+	struct fsl_msi *msi = fsl_msi;
+	int msir_index = -1;
+	int i;
+	u32 msir_value = 0;
+	u32 intr_index;
+	u32 have_shift = 0;
+
+	spin_lock(&desc->lock);
+	if ((msi->feature &  FSL_PIC_IP_MASK) == FSL_PIC_IP_IPIC) {
+		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;
+
+	for (i = 0; i < NR_MSIR; i++)
+		if (irq == msi->msir[i]) {
+			msir_index = i;
+			break;
+		}
+
+	if (i >= NR_MSIR)
+		cascade_irq = NO_IRQ;
+
+	desc->status |= IRQ_INPROGRESS;
+	switch (fsl_msi->feature & FSL_PIC_IP_MASK) {
+	case FSL_PIC_IP_MPIC:
+		msir_value = fsl_msi_read(msi->msi_regs, msir_index * 0x10);
+		break;
+	case FSL_PIC_IP_IPIC:
+		msir_value = fsl_msi_read(msi->msi_regs, msir_index * 0x4);
+		break;
+	}
+
+	while (msir_value) {
+		intr_index = ffs(msir_value) - 1;
+
+		cascade_irq = irq_linear_revmap(msi->irqhost,
+			(msir_index * INT_PER_MSIR + intr_index + have_shift));
+
+		if (cascade_irq != NO_IRQ)
+			generic_handle_irq(cascade_irq);
+		have_shift += (intr_index + 1);
+		msir_value = (msir_value >> (intr_index + 1));
+	}
+	desc->status &= ~IRQ_INPROGRESS;
+
+	switch (fsl_msi->feature & FSL_PIC_IP_MASK) {
+	case FSL_PIC_IP_MPIC:
+		desc->chip->eoi(irq);
+		break;
+	case FSL_PIC_IP_IPIC:
+		if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
+		desc->chip->unmask(irq);
+		break;
+	}
+unlock:
+	spin_unlock(&desc->lock);
+}
+
+static int __devinit fsl_of_msi_probe(struct of_device *dev,
+				const struct of_device_id *match)
+{
+	struct fsl_msi *msi;
+	struct resource res;
+	int err, i, count;
+	int rc;
+	int virt_msir;
+	const u32 *p;
+	struct fsl_msi_data *tmp_data;
+
+	printk(KERN_INFO "Setting up fsl msi support\n");
+
+	msi = kzalloc(sizeof(struct fsl_msi), GFP_KERNEL);
+	if (!msi) {
+		dev_err(&dev->dev, "No memory for MSI structure\n");
+		err = -ENOMEM;
+		goto error_out;
+	}
+
+	msi->of_node = dev->node;
+
+	msi->irqhost = irq_alloc_host(of_node_get(dev->node),
+				IRQ_HOST_MAP_LINEAR,
+				NR_MSI_IRQS, &fsl_msi_host_ops, 0);
+	if (msi->irqhost == NULL) {
+		dev_err(&dev->dev, "No memory for MSI irqhost\n");
+		err = -ENOMEM;
+		goto error_out;
+	}
+
+	/*Get the MSI reg base */
+	err = of_address_to_resource(dev->node, 0, &res);
+	if (err) {
+		dev_err(&dev->dev, "Can't get %s property 'reg'\n",
+				dev->node->full_name);
+		goto error_out;
+	}
+
+	msi->msi_regs = ioremap(res.start, res.end - res.start + 1);
+
+	tmp_data = (struct fsl_msi_data *)match->data;
+
+	msi->feature = tmp_data->fsl_pic_ip;
+
+	msi->irqhost->host_data = msi;
+	msi->hc_irq = &fsl_msi_chip;
+
+	msi->msi_addr_hi = 0x0;
+	msi->msi_addr_lo = res.start + tmp_data->msiir_offset;
+
+	msi->irq_count = NR_MSI_IRQS;
+
+	p = of_get_property(dev->node, "interrupts", &count);
+	if (!p) {
+		dev_err(&dev->dev, "no msi-interrupts property found on %s\n",
+				dev->node->full_name);
+		err = -ENODEV;
+		goto error_out;
+	}
+	if (count % 8 != 0) {
+		dev_err(&dev->dev, "Malformed msi-interrupts property on %s\n",
+				dev->node->full_name);
+		err = -EINVAL;
+		goto error_out;
+	}
+
+	count /= sizeof(u32);
+	for (i = 0; i < count / 2; i++) {
+		if (i > NR_MSIR)
+			break;
+		virt_msir = irq_of_parse_and_map(dev->node, i);
+		if (virt_msir != NO_IRQ) {
+			set_irq_data(virt_msir, msi);
+			set_irq_chained_handler(virt_msir, fsl_msi_cascade);
+			msi->msir[i] = virt_msir;
+		} else
+			msi->msir[i] = NO_IRQ;
+	}
+
+	rc = fsl_msi_init_allocator(msi);
+	if (rc) {
+		dev_err(&dev->dev, "Error allocating MSI bitmap\n");
+		goto error_out;
+	}
+
+	fsl_msi = msi;
+
+	WARN_ON(ppc_md.setup_msi_irqs);
+	ppc_md.setup_msi_irqs = fsl_setup_msi_irqs;
+	ppc_md.teardown_msi_irqs = fsl_teardown_msi_irqs;
+	ppc_md.msi_check_device = fsl_msi_check_device;
+	return 0;
+error_out:
+	if (msi)
+		kfree(msi);
+	return err;
+}
+
+static const struct fsl_msi_data mpic_msi_feature = {FSL_PIC_IP_MPIC, 0x140};
+static const struct fsl_msi_data ipic_msi_feature = {FSL_PIC_IP_IPIC, 0x38};
+
+static const struct of_device_id fsl_of_msi_ids[] = {
+	{
+		.compatible = "fsl,MPIC-MSI",
+		.data = (void *)&mpic_msi_feature,
+	},
+	{
+		.compatible = "fsl,IPIC-MSI",
+		.data = (void *)&ipic_msi_feature,
+	},
+	{}
+};
+
+static struct of_platform_driver fsl_of_msi_driver = {
+	.name = "fsl-of-msi",
+	.match_table = fsl_of_msi_ids,
+	.probe = fsl_of_msi_probe,
+};
+
+static __init int fsl_of_msi_init(void)
+{
+	return of_register_platform_driver(&fsl_of_msi_driver);
+}
+
+subsys_initcall(fsl_of_msi_init);
diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h
new file mode 100644
index 0000000..7eef9ec
--- /dev/null
+++ b/arch/powerpc/sysdev/fsl_msi.h
@@ -0,0 +1,40 @@
+#ifndef _ASM_POWERPC_MSI_H
+#define _ASM_POWERPC_MSI_H
+
+#define NR_MSIR	8
+#define INT_PER_MSIR	32
+#define NR_MSI_IRQS	(NR_MSIR * INT_PER_MSIR)
+
+#define FSL_PIC_IP_MASK	0x0000000F
+#define FSL_PIC_IP_MPIC	0x00000001
+#define FSL_PIC_IP_IPIC	0x00000002
+
+struct fsl_msi {
+	/* Device node of the MSI interrupt*/
+	struct device_node *of_node;
+
+	struct irq_host *irqhost;
+	struct irq_chip *hc_irq;
+
+	unsigned long cascade_irq;
+	unsigned int msir[NR_MSIR];
+
+	u32 msi_addr_lo;
+	u32 msi_addr_hi;
+	void __iomem *msi_regs;
+	u32 irq_count;
+	u32 feature;
+
+	spinlock_t fsl_msi_lock;
+	unsigned long *fsl_msi_bitmap;
+	spinlock_t bitmap_lock;
+	const char *name;
+};
+
+struct fsl_msi_data {
+	u32 fsl_pic_ip;
+	u32 msiir_offset;
+};
+
+#endif /* _ASM_POWERPC_MSI_H */
+
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index bf13c21..fede767 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -106,6 +106,16 @@ void __init setup_pci_cmd(struct pci_controller *hose)
 	}
 }
 
+#ifdef CONFIG_PCI_MSI
+void __init setup_pci_pcsrbar(struct pci_controller *hose)
+{
+	phys_addr_t immr_base;
+
+	immr_base = get_immrbase();
+	early_write_config_dword(hose, 0, 0, PCI_BASE_ADDRESS_0, immr_base);
+}
+#endif
+
 static int fsl_pcie_bus_fixup;
 
 static void __init quirk_fsl_pcie_header(struct pci_dev *dev)
@@ -211,6 +221,10 @@ int __init fsl_add_bridge(struct device_node *dev, int is_primary)
 	/* Setup PEX window registers */
 	setup_pci_atmu(hose, &rsrc);
 
+	/*Setup PEXCSRBAR */
+#ifdef CONFIG_PCI_MSI
+	setup_pci_pcsrbar(hose);
+#endif
 	return 0;
 }
 
-- 
1.5.4

[PATCH 3/3] Enable MSI support for 85xxds board.

[Jason Jin]

This patch enabled MSI on 8544ds and 8572ds board.
only one MSI interrupt can generate on 8544 board.

Signed-off-by: Jason Jin <Jason.jin@freescale.com>
---
 arch/powerpc/boot/dts/mpc8544ds.dts      |   16 ++++++++++++++++
 arch/powerpc/boot/dts/mpc8572ds.dts      |   16 ++++++++++++++++
 arch/powerpc/platforms/85xx/mpc85xx_ds.c |    7 ++++++-
 3 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8544ds.dts b/arch/powerpc/boot/dts/mpc8544ds.dts
index 6a0d8db..1059281 100644
--- a/arch/powerpc/boot/dts/mpc8544ds.dts
+++ b/arch/powerpc/boot/dts/mpc8544ds.dts
@@ -219,6 +219,22 @@
 			device_type = "open-pic";
 			big-endian;
 		};
+
+		msi@41600 {
+			compatible = "fsl,MPIC-msi";
+			reg = <0x41600 0x80>;
+			msi-available-ranges = <0 0x100>;
+			interrupts = <
+				0xb0 0
+				0xb1 0
+				0xb2 0
+				0xb3 0
+				0xb4 0
+				0xb5 0
+				0xb6 0
+				0xb7 0>;
+			interrupt-parent = <&mpic>;
+		};
 	};
 
 	pci0: pci@e0008000 {
diff --git a/arch/powerpc/boot/dts/mpc8572ds.dts b/arch/powerpc/boot/dts/mpc8572ds.dts
index 66f27ab..1c5ae9d 100644
--- a/arch/powerpc/boot/dts/mpc8572ds.dts
+++ b/arch/powerpc/boot/dts/mpc8572ds.dts
@@ -221,6 +221,22 @@
 			fsl,has-rstcr;
 		};
 
+		msi@41600 {
+			compatible = "fsl,MPIC-msi";
+			reg = <0x41600 0x80>;
+			msi-available-ranges = <0 0x100>;
+			interrupts = <
+				0xb0 0
+				0xb1 0
+				0xb2 0
+				0xb3 0
+				0xb4 0
+				0xb5 0
+				0xb6 0
+				0xb7 0>;
+			interrupt-parent = <&mpic>;
+		};
+
 		mpic: pic@40000 {
 			clock-frequency = <0>;
 			interrupt-controller;
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
index dfd8b4a..2696d2f 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
@@ -79,9 +79,13 @@ void __init mpc85xx_ds_pic_init(void)
 
 	mpic = mpic_alloc(np, r.start,
 			  MPIC_PRIMARY | MPIC_WANTS_RESET | MPIC_BIG_ENDIAN,
-			0, 256, " OpenPIC  ");
+			64, 256, " OpenPIC  ");
 	BUG_ON(mpic == NULL);
 
+	mpic_assign_isu(mpic, 0, r.start + 0x10000);
+	mpic_assign_isu(mpic, 1, r.start + 0x10800);
+	mpic_assign_isu(mpic, 2, r.start + 0x11600);
+
 	mpic_init(mpic);
 
 #ifdef CONFIG_PPC_I8259
@@ -195,6 +199,7 @@ static int __init mpc85xxds_publish_devices(void)
 	return of_platform_bus_probe(NULL, mpc85xxds_ids, NULL);
 }
 machine_device_initcall(mpc8544_ds, mpc85xxds_publish_devices);
+machine_device_initcall(mpc8572_ds, mpc85xxds_publish_devices);
 
 /*
  * Called very early, device-tree isn't unflattened
-- 
1.5.4

Re: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Michael Ellerman]

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

> This MSI driver can be used on 83xx/85xx/86xx board.
> In this driver, virtual interrupt host and chip were
> setup. There are 256 MSI interrupts in this host, Every 32
> MSI interrupts cascaded to one IPIC/MPIC interrupt.
> The chip was treated as edge sensitive and some necessary
> functions were setup for this chip.
> 
> Before using the MSI interrupt, PCI/PCIE device need to
> ask for a MSI interrupt in the 256 MSI interrupts. A 256bit
> bitmap show which MSI interrupt was used, reserve bit in
> the bitmap can be used to force the device use some designate
> MSI interrupt in the 256 MSI interrupts. Sometimes this is useful
> for testing the all the MSI interrupts. The msi-available-ranges
> property in the dts file was used for this purpose.
 

Hi Jason, some comments inline ...

 
> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> index 6d386d0..bfd3fe4 100644
> --- a/arch/powerpc/sysdev/Makefile
> +++ b/arch/powerpc/sysdev/Makefile
> @@ -4,6 +4,7 @@ endif
>  
>  mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
>  obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
> +obj-$(CONFIG_PCI_MSI)		+= fsl_msi.o

Do we really always want to build this if we have MSI? Might it depend
on something else as well? CONFIG_FSL_PCI maybe?

> diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
> new file mode 100644
> index 0000000..e8132cf
> --- /dev/null
> +++ b/arch/powerpc/sysdev/fsl_msi.c
> @@ -0,0 +1,457 @@
> +/*
> + * Copyright (C) 2007-2008 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: Tony Li <tony.li@freescale.com>
> + *	   Jason Jin <Jason.jin@freescale.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2 of the
> + * License.
> + *
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/bootmem.h>
> +#include <linux/bitmap.h>
> +#include <linux/msi.h>
> +#include <asm/prom.h>
> +#include <asm/hw_irq.h>
> +#include <linux/pci.h>
> +#include <asm/ppc-pci.h>
> +#include <linux/of_platform.h>
> +
> +#include <sysdev/fsl_soc.h>
> +#include "fsl_msi.h"

People consider it good style to have the linux includes before the
asm includes.

> +#ifdef DEBUG
> +#define pr_debug(fmt...) printk(fmt)
> +#else
> +#define pr_debug(fmt...)
> +#endif

Please don't do this, just use pr_debug(). In fact I don't see where you
do use it :)

> +/* A bit ugly, can we get this from the pci_dev somehow? */
> +static struct fsl_msi *fsl_msi;

I recognise that comment :)  The answer is "no" we can't (easily) get
this from the pci_dev.

> +static inline u32 fsl_msi_read(u32 __iomem *base,
> +				unsigned int reg)

This would fit in 80 chars wouldn't it?

> +{
> +	return in_be32(base + (reg >> 2));
> +}
> +
> +static inline void fsl_msi_write(u32 __iomem *base,
> +				unsigned int reg, u32 value)
> +{
> +	out_be32(base + (reg >> 2), value);
> +}
> +
> +#define	fsl_msi_irq_to_hw(virq)	 ((unsigned int)irq_map[virq].hwirq)

I can't see where this is used, you probably don't need it.

> +/*
> + * We do not need this actually. The MSIR register has been read once
> + * in the cascade interrupt. So, this MSI interrupt has been acked
> +*/
> +static void fsl_msi_end_irq(unsigned int virq)
> +{
> +}

I guess the generic code assumes you have an ack, bummer.

> +static struct irq_chip fsl_msi_chip = {
> +	.mask		= mask_msi_irq,
> +	.unmask		= unmask_msi_irq,
> +	.ack		= fsl_msi_end_irq,
> +	.typename	= " FSL-MSI  ",

I'd rather you didn't try and pad the name by hand, if we want /proc/interrupts
to look pretty we should do that in show_interrupts().

> +static int fsl_msi_host_match(struct irq_host *h, struct device_node *node)
> +{
> +	struct fsl_msi *msi = h->host_data;
> +
> +	/* Exact match, unless node is NULL */
> +	return msi->of_node == NULL || msi->of_node == node;
> +}

Do you really want the MSI to be the default irq host?

> +static int fsl_msi_host_map(struct irq_host *h, unsigned int virq,
> +				irq_hw_number_t hw)
> +{
> +	struct fsl_msi *msi = h->host_data;
> +	struct irq_chip *chip = msi->hc_irq;
> +
> +	set_irq_chip_data(virq, msi);

You don't seem to use chip_data anywhere?

> +	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 fsl_msi_host_ops = {
> +	.match = fsl_msi_host_match,
> +	.map = fsl_msi_host_map,
> +};
> +
> +irq_hw_number_t fsl_msi_alloc_hwirqs(struct fsl_msi *msi, int num)
> +{
> +	unsigned long flags;
> +	int offset, order = get_count_order(num);
> +
> +	spin_lock_irqsave(&msi->bitmap_lock, flags);
> +
> +	offset = bitmap_find_free_region(msi->fsl_msi_bitmap,
> +					NR_MSI_IRQS, order);
> +
> +	spin_unlock_irqrestore(&msi->bitmap_lock, flags);
> +
> +	pr_debug("%s: allocated 0x%x (2^%d) at offset 0x%x\n",
> +		__func__, num, order, offset);
> +
> +	return offset;
> +}
> +
> +void fsl_msi_free_hwirqs(struct fsl_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",
> +		__func__, num, order, offset);
> +
> +	spin_lock_irqsave(&msi->bitmap_lock, flags);
> +	bitmap_release_region(msi->fsl_msi_bitmap, offset, order);
> +	spin_unlock_irqrestore(&msi->bitmap_lock, flags);
> +}
> +
> +static int fsl_msi_reserve_dt_hwirqs(struct fsl_msi *msi)
> +{
> +	int i, len;
> +	const u32 *p;
> +
> +	p = of_get_property(msi->of_node, "msi-available-ranges", &len);
> +	if (!p) {
> +		pr_debug("fsl_msi: no msi-available-ranges property found \
> +				on %s\n", msi->of_node->full_name);
> +		return -ENODEV;
> +	}
> +
> +	if (len & 0x8 != 0) {
> +		printk(KERN_WARNING "fsl_msi: Malformed msi-available-ranges "
> +		       "property on %s\n", msi->of_node->full_name);
> +		return -EINVAL;
> +	}

Do you really want a bitwise and with 0x8?

> +
> +	bitmap_allocate_region(msi->fsl_msi_bitmap, 0,
> +			       get_count_order(msi->irq_count));
> +
> +	/* Format is: (<u32 start> <u32 count>)+ */
> +	len /= sizeof(u32);
> +	len /= 2;
> +	for (i = 0; i < len; i++, p += 2)
> +		fsl_msi_free_hwirqs(msi, *p, *(p + 1));
> +
> +	return 0;
> +}

We could share a bunch of that code with mpic_msi.c, but that's not your
job - I'll look at doing a patch once this goes in.

> +static int fsl_msi_init_allocator(struct fsl_msi *msi)
> +{
> +	int rc, size;
> +
> +	size = BITS_TO_LONGS(NR_MSI_IRQS) * sizeof(long);
> +
> +	msi->fsl_msi_bitmap = kmalloc(size, GFP_KERNEL);
> +
> +	if (msi->fsl_msi_bitmap == NULL) {
> +		pr_debug("%s: ENOMEM allocating allocator bitmap!\n",
> +				__func__);
> +		return -ENOMEM;
> +	}
> +	memset(msi->fsl_msi_bitmap, 0, size);

Use kzalloc() and you can lose the memset.

> +	rc = fsl_msi_reserve_dt_hwirqs(msi);

This routine is badly named (my fault), it really frees the available
MSI ranges.

> +	if (rc)
> +		goto out_free;
> +
> +	return 0;
> +out_free:
> +	if (mem_init_done)
> +		kfree(msi->fsl_msi_bitmap);

You don't need to test mem_init_done, this is running at initcall time
which is way later than mem_init.

> +	msi->fsl_msi_bitmap = NULL;
> +	return rc;
> +
> +}
> +
> +static int fsl_msi_check_device(struct pci_dev *pdev, int nvec, int type)
> +{
> +	if (type == PCI_CAP_ID_MSIX)
> +		pr_debug("fslmsi: MSI-X untested, trying anyway.\n");
> +
> +	return 0;
> +}
> +
> +static void fsl_teardown_msi_irqs(struct pci_dev *pdev)
> +{
> +	struct msi_desc *entry;
> +	struct fsl_msi *msi = fsl_msi;
> +
> +	list_for_each_entry(entry, &pdev->msi_list, list) {
> +		if (entry->irq == NO_IRQ)
> +			continue;
> +		set_irq_msi(entry->irq, NULL);
> +		fsl_msi_free_hwirqs(msi, virq_to_hw(entry->irq), 1);
> +		irq_dispose_mapping(entry->irq);
> +	}
> +
> +	return;
> +}
> +
> +static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> +				  struct msi_msg *msg)
> +{
> +	unsigned int srs;
> +	unsigned int ibs;
> +	struct fsl_msi *msi = fsl_msi;
> +
> +	srs = hwirq / INT_PER_MSIR;
> +	ibs = hwirq % INT_PER_MSIR;
> +
> +	msg->address_lo = msi->msi_addr_lo;
> +	msg->address_hi = msi->msi_addr_hi;
> +	msg->data = (srs << 5) | (ibs & 0x1F);

Is the 5 and 0x1F independent of the INT_PER_MSIR value? Given the
current values isn't this a no-op, or am I missing something?

> +	pr_debug("%s: allocated srs: %d, ibs: %d\n",
> +		__func__, srs, ibs);
> +}
> +
> +static int fsl_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
> +{
> +	irq_hw_number_t hwirq;
> +	int rc;
> +	unsigned int virq;
> +	struct msi_desc *entry;
> +	struct msi_msg msg;
> +	struct fsl_msi *msi = fsl_msi;

A couple of places you put this into a local called "msi" which is not the
greatest name in the world IMHO :)

> +	list_for_each_entry(entry, &pdev->msi_list, list) {
> +		hwirq = fsl_msi_alloc_hwirqs(msi, 1);
> +		if (hwirq < 0) {
> +			rc = hwirq;
> +			pr_debug("%s: fail allocating msi interrupt\n",
> +					__func__);
> +			goto out_free;
> +		}
> +
> +		virq = irq_create_mapping(msi->irqhost, hwirq);
> +
> +		if (virq == NO_IRQ) {
> +			pr_debug("%s: fail mapping hwirq 0x%lx\n",
> +					__func__, hwirq);
> +			fsl_msi_free_hwirqs(msi, hwirq, 1);
> +			rc = -ENOSPC;
> +			goto out_free;
> +		}
> +		set_irq_msi(virq, entry);
> +
> +		fsl_compose_msi_msg(pdev, hwirq, &msg);
> +		write_msi_msg(virq, &msg);
> +	}
> +	return 0;
> +
> +out_free:
> +	fsl_teardown_msi_irqs(pdev);

You don't need to call teardown, the generic code does that for you.

> +	return rc;
> +}
> +
> +void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
> +{
> +	unsigned int cascade_irq;
> +	struct fsl_msi *msi = fsl_msi;
> +	int msir_index = -1;
> +	int i;
> +	u32 msir_value = 0;
> +	u32 intr_index;
> +	u32 have_shift = 0;
> +
> +	spin_lock(&desc->lock);
> +	if ((msi->feature &  FSL_PIC_IP_MASK) == FSL_PIC_IP_IPIC) {
> +		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;
> +
> +	for (i = 0; i < NR_MSIR; i++)
> +		if (irq == msi->msir[i]) {
> +			msir_index = i;
> +			break;
> +		}

This is a bit ugly :)  Because you get the *msi from fsl_msi (above), you could
store the msir_index in the handler_data (with set_irq_data), and save doing
this loop.

> +	if (i >= NR_MSIR)
> +		cascade_irq = NO_IRQ;
> +
> +	desc->status |= IRQ_INPROGRESS;
> +	switch (fsl_msi->feature & FSL_PIC_IP_MASK) {
> +	case FSL_PIC_IP_MPIC:
> +		msir_value = fsl_msi_read(msi->msi_regs, msir_index * 0x10);
> +		break;
> +	case FSL_PIC_IP_IPIC:
> +		msir_value = fsl_msi_read(msi->msi_regs, msir_index * 0x4);
> +		break;
> +	}
> +
> +	while (msir_value) {
> +		intr_index = ffs(msir_value) - 1;
> +
> +		cascade_irq = irq_linear_revmap(msi->irqhost,
> +			(msir_index * INT_PER_MSIR + intr_index + have_shift));
> +
> +		if (cascade_irq != NO_IRQ)
> +			generic_handle_irq(cascade_irq);
> +		have_shift += (intr_index + 1);
> +		msir_value = (msir_value >> (intr_index + 1));
> +	}

It took me a while to grok all the shifting and so on here, I don't know if
there's a cleaner way to do it.

> +	desc->status &= ~IRQ_INPROGRESS;
> +
> +	switch (fsl_msi->feature & FSL_PIC_IP_MASK) {
> +	case FSL_PIC_IP_MPIC:
> +		desc->chip->eoi(irq);
> +		break;
> +	case FSL_PIC_IP_IPIC:
> +		if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
> +		desc->chip->unmask(irq);

Missing indent.

> +		break;
> +	}
> +unlock:
> +	spin_unlock(&desc->lock);
> +}
> +
> +static int __devinit fsl_of_msi_probe(struct of_device *dev,
> +				const struct of_device_id *match)
> +{
> +	struct fsl_msi *msi;
> +	struct resource res;
> +	int err, i, count;
> +	int rc;
> +	int virt_msir;
> +	const u32 *p;
> +	struct fsl_msi_data *tmp_data;
> +
> +	printk(KERN_INFO "Setting up fsl msi support\n");

KERN_DEBUG would do here IMHO, users don't usually need to see it.

> +	msi = kzalloc(sizeof(struct fsl_msi), GFP_KERNEL);
> +	if (!msi) {
> +		dev_err(&dev->dev, "No memory for MSI structure\n");
> +		err = -ENOMEM;
> +		goto error_out;
> +	}
> +
> +	msi->of_node = dev->node;
> +
> +	msi->irqhost = irq_alloc_host(of_node_get(dev->node),
> +				IRQ_HOST_MAP_LINEAR,
> +				NR_MSI_IRQS, &fsl_msi_host_ops, 0);
> +	if (msi->irqhost == NULL) {
> +		dev_err(&dev->dev, "No memory for MSI irqhost\n");
> +		err = -ENOMEM;

You need an of_node_put(dev->node) in the error case here.

> +		goto error_out;
> +	}
> +
> +	/*Get the MSI reg base */

Missing space after /*

> +	err = of_address_to_resource(dev->node, 0, &res);
> +	if (err) {
> +		dev_err(&dev->dev, "Can't get %s property 'reg'\n",
> +				dev->node->full_name);

That's a little misleading, aren't there's lots of reasons
of_address_to_resource() might fail?

> +		goto error_out;
> +	}
> +
> +	msi->msi_regs = ioremap(res.start, res.end - res.start + 1);

ioremap() can fail.

> +	tmp_data = (struct fsl_msi_data *)match->data;
> +
> +	msi->feature = tmp_data->fsl_pic_ip;
> +
> +	msi->irqhost->host_data = msi;
> +	msi->hc_irq = &fsl_msi_chip;

Any reason this needs to be a variable, isn't it always &fsl_msi_chip?

> +	msi->msi_addr_hi = 0x0;
> +	msi->msi_addr_lo = res.start + tmp_data->msiir_offset;
> +
> +	msi->irq_count = NR_MSI_IRQS;

Ditto.

> +	p = of_get_property(dev->node, "interrupts", &count);
> +	if (!p) {
> +		dev_err(&dev->dev, "no msi-interrupts property found on %s\n",
> +				dev->node->full_name);
> +		err = -ENODEV;
> +		goto error_out;
> +	}
> +	if (count % 8 != 0) {
> +		dev_err(&dev->dev, "Malformed msi-interrupts property on %s\n",
> +				dev->node->full_name);

Messages don't match the code, "interrupts" vs "msi-interrupts".

> +		err = -EINVAL;
> +		goto error_out;
> +	}
> +
> +	count /= sizeof(u32);
> +	for (i = 0; i < count / 2; i++) {
> +		if (i > NR_MSIR)
> +			break;
> +		virt_msir = irq_of_parse_and_map(dev->node, i);
> +		if (virt_msir != NO_IRQ) {
> +			set_irq_data(virt_msir, msi);
> +			set_irq_chained_handler(virt_msir, fsl_msi_cascade);
> +			msi->msir[i] = virt_msir;
> +		} else
> +			msi->msir[i] = NO_IRQ;
> +	}
> +
> +	rc = fsl_msi_init_allocator(msi);
> +	if (rc) {
> +		dev_err(&dev->dev, "Error allocating MSI bitmap\n");

If you hit this then the error case needs to get rid of all the irq mappings
you created just above (which it doesn't). It would be simpler if you just move
this call above the for loop, then you don't need to worry about it.

> +		goto error_out;
> +	}
> +
> +	fsl_msi = msi;
> +
> +	WARN_ON(ppc_md.setup_msi_irqs);
> +	ppc_md.setup_msi_irqs = fsl_setup_msi_irqs;
> +	ppc_md.teardown_msi_irqs = fsl_teardown_msi_irqs;
> +	ppc_md.msi_check_device = fsl_msi_check_device;
> +	return 0;
> +error_out:
> +	if (msi)
> +		kfree(msi);

kfree(NULL) is fine.

> +	return err;
> +}
> +
> +static const struct fsl_msi_data mpic_msi_feature = {FSL_PIC_IP_MPIC, 0x140};
> +static const struct fsl_msi_data ipic_msi_feature = {FSL_PIC_IP_IPIC, 0x38};
> +
> +static const struct of_device_id fsl_of_msi_ids[] = {
> +	{
> +		.compatible = "fsl,MPIC-MSI",
> +		.data = (void *)&mpic_msi_feature,
> +	},
> +	{
> +		.compatible = "fsl,IPIC-MSI",
> +		.data = (void *)&ipic_msi_feature,
> +	},
> +	{}
> +};
> +
> +static struct of_platform_driver fsl_of_msi_driver = {
> +	.name = "fsl-of-msi",
> +	.match_table = fsl_of_msi_ids,
> +	.probe = fsl_of_msi_probe,
> +};
> +
> +static __init int fsl_of_msi_init(void)
> +{
> +	return of_register_platform_driver(&fsl_of_msi_driver);
> +}
> +
> +subsys_initcall(fsl_of_msi_init);
> diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h
> new file mode 100644
> index 0000000..7eef9ec
> --- /dev/null
> +++ b/arch/powerpc/sysdev/fsl_msi.h
> @@ -0,0 +1,40 @@
> +#ifndef _ASM_POWERPC_MSI_H
> +#define _ASM_POWERPC_MSI_H

Include guards don't match the filename, should be _POWERPC_SYSDEV_FSL_MSI_H

> +#define NR_MSIR	8
> +#define INT_PER_MSIR	32
> +#define NR_MSI_IRQS	(NR_MSIR * INT_PER_MSIR)
> +
> +#define FSL_PIC_IP_MASK	0x0000000F
> +#define FSL_PIC_IP_MPIC	0x00000001
> +#define FSL_PIC_IP_IPIC	0x00000002
> +
> +struct fsl_msi {
> +	/* Device node of the MSI interrupt*/
> +	struct device_node *of_node;
> +
> +	struct irq_host *irqhost;
> +	struct irq_chip *hc_irq;
> +
> +	unsigned long cascade_irq;
> +	unsigned int msir[NR_MSIR];
> +
> +	u32 msi_addr_lo;
> +	u32 msi_addr_hi;
> +	void __iomem *msi_regs;
> +	u32 irq_count;
> +	u32 feature;
> +
> +	spinlock_t fsl_msi_lock;

Unused?

> +	unsigned long *fsl_msi_bitmap;

Do we need fsl_msi in the name?

> +	spinlock_t bitmap_lock;
> +	const char *name;

Unused?

> +};
> +
> +struct fsl_msi_data {
> +	u32 fsl_pic_ip;
> +	u32 msiir_offset;
> +};
> +
> +#endif /* _ASM_POWERPC_MSI_H */
> +
> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
> index bf13c21..fede767 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -106,6 +106,16 @@ void __init setup_pci_cmd(struct pci_controller *hose)
>  	}
>  }
>  
> +#ifdef CONFIG_PCI_MSI
> +void __init setup_pci_pcsrbar(struct pci_controller *hose)
> +{
> +	phys_addr_t immr_base;
> +
> +	immr_base = get_immrbase();
> +	early_write_config_dword(hose, 0, 0, PCI_BASE_ADDRESS_0, immr_base);
> +}
> +#endif

Up to you, but I prefer an empty static inline here which saves an #ifdef
at the call site.

>  static int fsl_pcie_bus_fixup;
>  
>  static void __init quirk_fsl_pcie_header(struct pci_dev *dev)
> @@ -211,6 +221,10 @@ int __init fsl_add_bridge(struct device_node *dev, int is_primary)
>  	/* Setup PEX window registers */
>  	setup_pci_atmu(hose, &rsrc);
>  
> +	/*Setup PEXCSRBAR */
> +#ifdef CONFIG_PCI_MSI
> +	setup_pci_pcsrbar(hose);
> +#endif
>  	return 0;
>  }


cheers

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

RE: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Jin Zhengxiong]

Hi, Michael,

Thank you very much for you input, please see my inline answer.

B.R
Jason=20

> -----Original Message-----
> From: Michael Ellerman [mailto:michael@ellerman.id.au]=20
> Sent: Monday, April 21, 2008 2:22 PM
> To: Jin Zhengxiong
> Cc: linuxppc-dev@ozlabs.org; Gala Kumar
> Subject: Re: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu
>=20
> > This MSI driver can be used on 83xx/85xx/86xx board.
> > In this driver, virtual interrupt host and chip were
> > setup. There are 256 MSI interrupts in this host, Every 32
> > MSI interrupts cascaded to one IPIC/MPIC interrupt.
> > The chip was treated as edge sensitive and some necessary
> > functions were setup for this chip.
> >=20
> > Before using the MSI interrupt, PCI/PCIE device need to
> > ask for a MSI interrupt in the 256 MSI interrupts. A 256bit
> > bitmap show which MSI interrupt was used, reserve bit in
> > the bitmap can be used to force the device use some designate
> > MSI interrupt in the 256 MSI interrupts. Sometimes this is useful
> > for testing the all the MSI interrupts. The msi-available-ranges
> > property in the dts file was used for this purpose.
> =20
>=20
> Hi Jason, some comments inline ...
>=20
> =20
> > diff --git a/arch/powerpc/sysdev/Makefile=20
> b/arch/powerpc/sysdev/Makefile
> > index 6d386d0..bfd3fe4 100644
> > --- a/arch/powerpc/sysdev/Makefile
> > +++ b/arch/powerpc/sysdev/Makefile
> > @@ -4,6 +4,7 @@ endif
> > =20
> >  mpic-msi-obj-$(CONFIG_PCI_MSI)	+=3D mpic_msi.o=20
> mpic_u3msi.o mpic_pasemi_msi.o
> >  obj-$(CONFIG_MPIC)		+=3D mpic.o $(mpic-msi-obj-y)
> > +obj-$(CONFIG_PCI_MSI)		+=3D fsl_msi.o
>=20
> Do we really always want to build this if we have MSI? Might it depend
> on something else as well? CONFIG_FSL_PCI maybe?
>=20
I'll try to change the depend.

> > diff --git a/arch/powerpc/sysdev/fsl_msi.c=20
> b/arch/powerpc/sysdev/fsl_msi.c
> > new file mode 100644
> > index 0000000..e8132cf
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/fsl_msi.c
> > @@ -0,0 +1,457 @@
> > +/*
> > + * Copyright (C) 2007-2008 Freescale Semiconductor, Inc.=20
> All rights reserved.
> > + *
> > + * Author: Tony Li <tony.li@freescale.com>
> > + *	   Jason Jin <Jason.jin@freescale.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; version 2 of the
> > + * License.
> > + *
> > + */
> > +
> > +#include <linux/irq.h>
> > +#include <linux/bootmem.h>
> > +#include <linux/bitmap.h>
> > +#include <linux/msi.h>
> > +#include <asm/prom.h>
> > +#include <asm/hw_irq.h>
> > +#include <linux/pci.h>
> > +#include <asm/ppc-pci.h>
> > +#include <linux/of_platform.h>
> > +
> > +#include <sysdev/fsl_soc.h>
> > +#include "fsl_msi.h"
>=20
> People consider it good style to have the linux includes before the
> asm includes.
>=20
OK

> > +#ifdef DEBUG
> > +#define pr_debug(fmt...) printk(fmt)
> > +#else
> > +#define pr_debug(fmt...)
> > +#endif
>=20
> Please don't do this, just use pr_debug(). In fact I don't=20
> see where you
> do use it :)
>=20
OK.

> > +/* A bit ugly, can we get this from the pci_dev somehow? */
> > +static struct fsl_msi *fsl_msi;
>=20
> I recognise that comment :)  The answer is "no" we can't (easily) get
> this from the pci_dev.
>=20
> > +static inline u32 fsl_msi_read(u32 __iomem *base,
> > +				unsigned int reg)
>=20
> This would fit in 80 chars wouldn't it?
>=20
OK

> > +{
> > +	return in_be32(base + (reg >> 2));
> > +}
> > +
> > +static inline void fsl_msi_write(u32 __iomem *base,
> > +				unsigned int reg, u32 value)
> > +{
> > +	out_be32(base + (reg >> 2), value);
> > +}
> > +
> > +#define	fsl_msi_irq_to_hw(virq)	 ((unsigned=20
> int)irq_map[virq].hwirq)
>=20
> I can't see where this is used, you probably don't need it.
>
I'll remove this.
=20
> > +/*
> > + * We do not need this actually. The MSIR register has=20
> been read once
> > + * in the cascade interrupt. So, this MSI interrupt has been acked
> > +*/
> > +static void fsl_msi_end_irq(unsigned int virq)
> > +{
> > +}
>=20
> I guess the generic code assumes you have an ack, bummer.
>=20
Yes, the generic code did not check it.

> > +static struct irq_chip fsl_msi_chip =3D {
> > +	.mask		=3D mask_msi_irq,
> > +	.unmask		=3D unmask_msi_irq,
> > +	.ack		=3D fsl_msi_end_irq,
> > +	.typename	=3D " FSL-MSI  ",
>=20
> I'd rather you didn't try and pad the name by hand, if we=20
> want /proc/interrupts
> to look pretty we should do that in show_interrupts().
>=20
Thanks, but I feel it's easier and cleaner to add the typename here to
make=20
the /proc/interrupts clear.=20

> > +static int fsl_msi_host_match(struct irq_host *h, struct=20
> device_node *node)
> > +{
> > +	struct fsl_msi *msi =3D h->host_data;
> > +
> > +	/* Exact match, unless node is NULL */
> > +	return msi->of_node =3D=3D NULL || msi->of_node =3D=3D node;
> > +}
>=20
> Do you really want the MSI to be the default irq host?
>=20
Thanks. I'll change the host match function.

> > +static int fsl_msi_host_map(struct irq_host *h, unsigned int virq,
> > +				irq_hw_number_t hw)
> > +{
> > +	struct fsl_msi *msi =3D h->host_data;
> > +	struct irq_chip *chip =3D msi->hc_irq;
> > +
> > +	set_irq_chip_data(virq, msi);
>=20
> You don't seem to use chip_data anywhere?
>=20
I'll check this.

> > +	get_irq_desc(virq)->status |=3D IRQ_TYPE_EDGE_FALLING;
> > +
> > +	set_irq_chip_and_handler(virq, chip,  handle_edge_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct irq_host_ops fsl_msi_host_ops =3D {
> > +	.match =3D fsl_msi_host_match,
> > +	.map =3D fsl_msi_host_map,
> > +};
> > +
> > +irq_hw_number_t fsl_msi_alloc_hwirqs(struct fsl_msi *msi, int num)
> > +{
> > +	unsigned long flags;
> > +	int offset, order =3D get_count_order(num);
> > +
> > +	spin_lock_irqsave(&msi->bitmap_lock, flags);
> > +
> > +	offset =3D bitmap_find_free_region(msi->fsl_msi_bitmap,
> > +					NR_MSI_IRQS, order);
> > +
> > +	spin_unlock_irqrestore(&msi->bitmap_lock, flags);
> > +
> > +	pr_debug("%s: allocated 0x%x (2^%d) at offset 0x%x\n",
> > +		__func__, num, order, offset);
> > +
> > +	return offset;
> > +}
> > +
> > +void fsl_msi_free_hwirqs(struct fsl_msi *msi, int offset, int num)
> > +{
> > +	unsigned long flags;
> > +	int order =3D get_count_order(num);
> > +
> > +	pr_debug("%s: freeing 0x%x (2^%d) at offset 0x%x\n",
> > +		__func__, num, order, offset);
> > +
> > +	spin_lock_irqsave(&msi->bitmap_lock, flags);
> > +	bitmap_release_region(msi->fsl_msi_bitmap, offset, order);
> > +	spin_unlock_irqrestore(&msi->bitmap_lock, flags);
> > +}
> > +
> > +static int fsl_msi_reserve_dt_hwirqs(struct fsl_msi *msi)
> > +{
> > +	int i, len;
> > +	const u32 *p;
> > +
> > +	p =3D of_get_property(msi->of_node, "msi-available-ranges", &len);
> > +	if (!p) {
> > +		pr_debug("fsl_msi: no msi-available-ranges=20
> property found \
> > +				on %s\n", msi->of_node->full_name);
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (len & 0x8 !=3D 0) {
> > +		printk(KERN_WARNING "fsl_msi: Malformed=20
> msi-available-ranges "
> > +		       "property on %s\n", msi->of_node->full_name);
> > +		return -EINVAL;
> > +	}
>=20
> Do you really want a bitwise and with 0x8?
>=20
The range for the msi interrupt can be seperated to several part.=20
This can used to check the if the ranges is correct.=20

> > +
> > +	bitmap_allocate_region(msi->fsl_msi_bitmap, 0,
> > +			       get_count_order(msi->irq_count));
> > +
> > +	/* Format is: (<u32 start> <u32 count>)+ */
> > +	len /=3D sizeof(u32);
> > +	len /=3D 2;
> > +	for (i =3D 0; i < len; i++, p +=3D 2)
> > +		fsl_msi_free_hwirqs(msi, *p, *(p + 1));
> > +
> > +	return 0;
> > +}
>=20
> We could share a bunch of that code with mpic_msi.c, but=20
> that's not your
> job - I'll look at doing a patch once this goes in.
>=20
Thanks for the code you write for the MPIC msi.
 I'll use this until you share the code.

> > +static int fsl_msi_init_allocator(struct fsl_msi *msi)
> > +{
> > +	int rc, size;
> > +
> > +	size =3D BITS_TO_LONGS(NR_MSI_IRQS) * sizeof(long);
> > +
> > +	msi->fsl_msi_bitmap =3D kmalloc(size, GFP_KERNEL);
> > +
> > +	if (msi->fsl_msi_bitmap =3D=3D NULL) {
> > +		pr_debug("%s: ENOMEM allocating allocator bitmap!\n",
> > +				__func__);
> > +		return -ENOMEM;
> > +	}
> > +	memset(msi->fsl_msi_bitmap, 0, size);
>=20
> Use kzalloc() and you can lose the memset.
>=20
OK.

> > +	rc =3D fsl_msi_reserve_dt_hwirqs(msi);
>=20
> This routine is badly named (my fault), it really frees the available
> MSI ranges.
>=20
> > +	if (rc)
> > +		goto out_free;
> > +
> > +	return 0;
> > +out_free:
> > +	if (mem_init_done)
> > +		kfree(msi->fsl_msi_bitmap);
>=20
> You don't need to test mem_init_done, this is running at initcall time
> which is way later than mem_init.
>=20
Yes, I'll remove this. This code once used at just after the mpic was
intialized,and this
was left here.

> > +	msi->fsl_msi_bitmap =3D NULL;
> > +	return rc;
> > +
> > +}
> > +
> > +static int fsl_msi_check_device(struct pci_dev *pdev, int=20
> nvec, int type)
> > +{
> > +	if (type =3D=3D PCI_CAP_ID_MSIX)
> > +		pr_debug("fslmsi: MSI-X untested, trying anyway.\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static void fsl_teardown_msi_irqs(struct pci_dev *pdev)
> > +{
> > +	struct msi_desc *entry;
> > +	struct fsl_msi *msi =3D fsl_msi;
> > +
> > +	list_for_each_entry(entry, &pdev->msi_list, list) {
> > +		if (entry->irq =3D=3D NO_IRQ)
> > +			continue;
> > +		set_irq_msi(entry->irq, NULL);
> > +		fsl_msi_free_hwirqs(msi, virq_to_hw(entry->irq), 1);
> > +		irq_dispose_mapping(entry->irq);
> > +	}
> > +
> > +	return;
> > +}
> > +
> > +static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> > +				  struct msi_msg *msg)
> > +{
> > +	unsigned int srs;
> > +	unsigned int ibs;
> > +	struct fsl_msi *msi =3D fsl_msi;
> > +
> > +	srs =3D hwirq / INT_PER_MSIR;
> > +	ibs =3D hwirq % 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);
>=20
> Is the 5 and 0x1F independent of the INT_PER_MSIR value? Given the
> current values isn't this a no-op, or am I missing something?
>=20
Do you mean there're another way to get the msg->data from the hwirq? =20

> > +	pr_debug("%s: allocated srs: %d, ibs: %d\n",
> > +		__func__, srs, ibs);
> > +}
> > +
> > +static int fsl_setup_msi_irqs(struct pci_dev *pdev, int=20
> nvec, int type)
> > +{
> > +	irq_hw_number_t hwirq;
> > +	int rc;
> > +	unsigned int virq;
> > +	struct msi_desc *entry;
> > +	struct msi_msg msg;
> > +	struct fsl_msi *msi =3D fsl_msi;
>=20
> A couple of places you put this into a local called "msi"=20
> which is not the
> greatest name in the world IMHO :)
>=20
Thank you, I'll try to use another name, Do you have any suggestion?

> > +	list_for_each_entry(entry, &pdev->msi_list, list) {
> > +		hwirq =3D fsl_msi_alloc_hwirqs(msi, 1);
> > +		if (hwirq < 0) {
> > +			rc =3D hwirq;
> > +			pr_debug("%s: fail allocating msi interrupt\n",
> > +					__func__);
> > +			goto out_free;
> > +		}
> > +
> > +		virq =3D irq_create_mapping(msi->irqhost, hwirq);
> > +
> > +		if (virq =3D=3D NO_IRQ) {
> > +			pr_debug("%s: fail mapping hwirq 0x%lx\n",
> > +					__func__, hwirq);
> > +			fsl_msi_free_hwirqs(msi, hwirq, 1);
> > +			rc =3D -ENOSPC;
> > +			goto out_free;
> > +		}
> > +		set_irq_msi(virq, entry);
> > +
> > +		fsl_compose_msi_msg(pdev, hwirq, &msg);
> > +		write_msi_msg(virq, &msg);
> > +	}
> > +	return 0;
> > +
> > +out_free:
> > +	fsl_teardown_msi_irqs(pdev);
>=20
> You don't need to call teardown, the generic code does that for you.
>=20
OK.

> > +	return rc;
> > +}
> > +
> > +void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
> > +{
> > +	unsigned int cascade_irq;
> > +	struct fsl_msi *msi =3D fsl_msi;
> > +	int msir_index =3D -1;
> > +	int i;
> > +	u32 msir_value =3D 0;
> > +	u32 intr_index;
> > +	u32 have_shift =3D 0;
> > +
> > +	spin_lock(&desc->lock);
> > +	if ((msi->feature &  FSL_PIC_IP_MASK) =3D=3D FSL_PIC_IP_IPIC) {
> > +		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;
> > +
> > +	for (i =3D 0; i < NR_MSIR; i++)
> > +		if (irq =3D=3D msi->msir[i]) {
> > +			msir_index =3D i;
> > +			break;
> > +		}
>=20
> This is a bit ugly :)  Because you get the *msi from fsl_msi=20
> (above), you could
> store the msir_index in the handler_data (with set_irq_data),=20
> and save doing
> this loop.
>=20
I'll find if the handler data *msi was used somewhere. if not I change
to
save the msir_index to the handler data.=20

> > +	if (i >=3D NR_MSIR)
> > +		cascade_irq =3D NO_IRQ;
> > +
> > +	desc->status |=3D IRQ_INPROGRESS;
> > +	switch (fsl_msi->feature & FSL_PIC_IP_MASK) {
> > +	case FSL_PIC_IP_MPIC:
> > +		msir_value =3D fsl_msi_read(msi->msi_regs,=20
> msir_index * 0x10);
> > +		break;
> > +	case FSL_PIC_IP_IPIC:
> > +		msir_value =3D fsl_msi_read(msi->msi_regs,=20
> msir_index * 0x4);
> > +		break;
> > +	}
> > +
> > +	while (msir_value) {
> > +		intr_index =3D ffs(msir_value) - 1;
> > +
> > +		cascade_irq =3D irq_linear_revmap(msi->irqhost,
> > +			(msir_index * INT_PER_MSIR + intr_index=20
> + have_shift));
> > +
> > +		if (cascade_irq !=3D NO_IRQ)
> > +			generic_handle_irq(cascade_irq);
> > +		have_shift +=3D (intr_index + 1);
> > +		msir_value =3D (msir_value >> (intr_index + 1));
> > +	}
>=20
> It took me a while to grok all the shifting and so on here, I=20
> don't know if
> there's a cleaner way to do it.
>=20
To improve the efficency here, I tried to use the bit operation which
can=20
carry out by the cpu with one instruction. Maybe I also need to improve=20
readability :).

> > +	desc->status &=3D ~IRQ_INPROGRESS;
> > +
> > +	switch (fsl_msi->feature & FSL_PIC_IP_MASK) {
> > +	case FSL_PIC_IP_MPIC:
> > +		desc->chip->eoi(irq);
> > +		break;
> > +	case FSL_PIC_IP_IPIC:
> > +		if (!(desc->status & IRQ_DISABLED) &&=20
> desc->chip->unmask)
> > +		desc->chip->unmask(irq);
>=20
> Missing indent.
>=20
OK.

> > +		break;
> > +	}
> > +unlock:
> > +	spin_unlock(&desc->lock);
> > +}
> > +
> > +static int __devinit fsl_of_msi_probe(struct of_device *dev,
> > +				const struct of_device_id *match)
> > +{
> > +	struct fsl_msi *msi;
> > +	struct resource res;
> > +	int err, i, count;
> > +	int rc;
> > +	int virt_msir;
> > +	const u32 *p;
> > +	struct fsl_msi_data *tmp_data;
> > +
> > +	printk(KERN_INFO "Setting up fsl msi support\n");
>=20
> KERN_DEBUG would do here IMHO, users don't usually need to see it.
>=20
> > +	msi =3D kzalloc(sizeof(struct fsl_msi), GFP_KERNEL);
> > +	if (!msi) {
> > +		dev_err(&dev->dev, "No memory for MSI structure\n");
> > +		err =3D -ENOMEM;
> > +		goto error_out;
> > +	}
> > +
> > +	msi->of_node =3D dev->node;
> > +
> > +	msi->irqhost =3D irq_alloc_host(of_node_get(dev->node),
> > +				IRQ_HOST_MAP_LINEAR,
> > +				NR_MSI_IRQS, &fsl_msi_host_ops, 0);
> > +	if (msi->irqhost =3D=3D NULL) {
> > +		dev_err(&dev->dev, "No memory for MSI irqhost\n");
> > +		err =3D -ENOMEM;
>=20
> You need an of_node_put(dev->node) in the error case here.
>=20
Thanks, I'll add it.

> > +		goto error_out;
> > +	}
> > +
> > +	/*Get the MSI reg base */
>=20
> Missing space after /*
>=20
> > +	err =3D of_address_to_resource(dev->node, 0, &res);
> > +	if (err) {
> > +		dev_err(&dev->dev, "Can't get %s property 'reg'\n",
> > +				dev->node->full_name);
>=20
> That's a little misleading, aren't there's lots of reasons
> of_address_to_resource() might fail?
>=20
> > +		goto error_out;
> > +	}
> > +
> > +	msi->msi_regs =3D ioremap(res.start, res.end - res.start + 1);
>=20
> ioremap() can fail.
>=20
OK,  I'll add the error process code here.

> > +	tmp_data =3D (struct fsl_msi_data *)match->data;
> > +
> > +	msi->feature =3D tmp_data->fsl_pic_ip;
> > +
> > +	msi->irqhost->host_data =3D msi;
> > +	msi->hc_irq =3D &fsl_msi_chip;
>=20
> Any reason this needs to be a variable, isn't it always &fsl_msi_chip?
>=20
> > +	msi->msi_addr_hi =3D 0x0;
> > +	msi->msi_addr_lo =3D res.start + tmp_data->msiir_offset;
> > +
> > +	msi->irq_count =3D NR_MSI_IRQS;
>=20
> Ditto.
>=20
> > +	p =3D of_get_property(dev->node, "interrupts", &count);
> > +	if (!p) {
> > +		dev_err(&dev->dev, "no msi-interrupts property=20
> found on %s\n",
> > +				dev->node->full_name);
> > +		err =3D -ENODEV;
> > +		goto error_out;
> > +	}
> > +	if (count % 8 !=3D 0) {
> > +		dev_err(&dev->dev, "Malformed msi-interrupts=20
> property on %s\n",
> > +				dev->node->full_name);
>=20
> Messages don't match the code, "interrupts" vs "msi-interrupts".
>=20
Thanks, I'll change the description.

> > +		err =3D -EINVAL;
> > +		goto error_out;
> > +	}
> > +
> > +	count /=3D sizeof(u32);
> > +	for (i =3D 0; i < count / 2; i++) {
> > +		if (i > NR_MSIR)
> > +			break;
> > +		virt_msir =3D irq_of_parse_and_map(dev->node, i);
> > +		if (virt_msir !=3D NO_IRQ) {
> > +			set_irq_data(virt_msir, msi);
> > +			set_irq_chained_handler(virt_msir,=20
> fsl_msi_cascade);
> > +			msi->msir[i] =3D virt_msir;
> > +		} else
> > +			msi->msir[i] =3D NO_IRQ;
> > +	}
> > +
> > +	rc =3D fsl_msi_init_allocator(msi);
> > +	if (rc) {
> > +		dev_err(&dev->dev, "Error allocating MSI bitmap\n");
>=20
> If you hit this then the error case needs to get rid of all=20
> the irq mappings
> you created just above (which it doesn't). It would be=20
> simpler if you just move
> this call above the for loop, then you don't need to worry about it.
>
Thanks, good idea.
=20
> > +		goto error_out;
> > +	}
> > +
> > +	fsl_msi =3D msi;
> > +
> > +	WARN_ON(ppc_md.setup_msi_irqs);
> > +	ppc_md.setup_msi_irqs =3D fsl_setup_msi_irqs;
> > +	ppc_md.teardown_msi_irqs =3D fsl_teardown_msi_irqs;
> > +	ppc_md.msi_check_device =3D fsl_msi_check_device;
> > +	return 0;
> > +error_out:
> > +	if (msi)
> > +		kfree(msi);
>=20
> kfree(NULL) is fine.
>=20
> > +	return err;
> > +}
> > +
> > +static const struct fsl_msi_data mpic_msi_feature =3D=20
> {FSL_PIC_IP_MPIC, 0x140};
> > +static const struct fsl_msi_data ipic_msi_feature =3D=20
> {FSL_PIC_IP_IPIC, 0x38};
> > +
> > +static const struct of_device_id fsl_of_msi_ids[] =3D {
> > +	{
> > +		.compatible =3D "fsl,MPIC-MSI",
> > +		.data =3D (void *)&mpic_msi_feature,
> > +	},
> > +	{
> > +		.compatible =3D "fsl,IPIC-MSI",
> > +		.data =3D (void *)&ipic_msi_feature,
> > +	},
> > +	{}
> > +};
> > +
> > +static struct of_platform_driver fsl_of_msi_driver =3D {
> > +	.name =3D "fsl-of-msi",
> > +	.match_table =3D fsl_of_msi_ids,
> > +	.probe =3D fsl_of_msi_probe,
> > +};
> > +
> > +static __init int fsl_of_msi_init(void)
> > +{
> > +	return of_register_platform_driver(&fsl_of_msi_driver);
> > +}
> > +
> > +subsys_initcall(fsl_of_msi_init);
> > diff --git a/arch/powerpc/sysdev/fsl_msi.h=20
> b/arch/powerpc/sysdev/fsl_msi.h
> > new file mode 100644
> > index 0000000..7eef9ec
> > --- /dev/null
> > +++ b/arch/powerpc/sysdev/fsl_msi.h
> > @@ -0,0 +1,40 @@
> > +#ifndef _ASM_POWERPC_MSI_H
> > +#define _ASM_POWERPC_MSI_H
>=20
> Include guards don't match the filename, should be=20
> _POWERPC_SYSDEV_FSL_MSI_H
>=20
OK.

> > +#define NR_MSIR	8
> > +#define INT_PER_MSIR	32
> > +#define NR_MSI_IRQS	(NR_MSIR * INT_PER_MSIR)
> > +
> > +#define FSL_PIC_IP_MASK	0x0000000F
> > +#define FSL_PIC_IP_MPIC	0x00000001
> > +#define FSL_PIC_IP_IPIC	0x00000002
> > +
> > +struct fsl_msi {
> > +	/* Device node of the MSI interrupt*/
> > +	struct device_node *of_node;
> > +
> > +	struct irq_host *irqhost;
> > +	struct irq_chip *hc_irq;
> > +
> > +	unsigned long cascade_irq;
> > +	unsigned int msir[NR_MSIR];
> > +
> > +	u32 msi_addr_lo;
> > +	u32 msi_addr_hi;
> > +	void __iomem *msi_regs;
> > +	u32 irq_count;
> > +	u32 feature;
> > +
> > +	spinlock_t fsl_msi_lock;
>=20
> Unused?
>=20
> > +	unsigned long *fsl_msi_bitmap;
>=20
> Do we need fsl_msi in the name?
>=20
> > +	spinlock_t bitmap_lock;
> > +	const char *name;
>=20
> Unused?
>=20
> > +};
> > +
> > +struct fsl_msi_data {
> > +	u32 fsl_pic_ip;
> > +	u32 msiir_offset;
> > +};
> > +
> > +#endif /* _ASM_POWERPC_MSI_H */
> > +
> > diff --git a/arch/powerpc/sysdev/fsl_pci.c=20
> b/arch/powerpc/sysdev/fsl_pci.c
> > index bf13c21..fede767 100644
> > --- a/arch/powerpc/sysdev/fsl_pci.c
> > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > @@ -106,6 +106,16 @@ void __init setup_pci_cmd(struct=20
> pci_controller *hose)
> >  	}
> >  }
> > =20
> > +#ifdef CONFIG_PCI_MSI
> > +void __init setup_pci_pcsrbar(struct pci_controller *hose)
> > +{
> > +	phys_addr_t immr_base;
> > +
> > +	immr_base =3D get_immrbase();
> > +	early_write_config_dword(hose, 0, 0,=20
> PCI_BASE_ADDRESS_0, immr_base);
> > +}
> > +#endif
>=20
> Up to you, but I prefer an empty static inline here which=20
> saves an #ifdef
> at the call site.
>=20
> >  static int fsl_pcie_bus_fixup;
> > =20
> >  static void __init quirk_fsl_pcie_header(struct pci_dev *dev)
> > @@ -211,6 +221,10 @@ int __init fsl_add_bridge(struct=20
> device_node *dev, int is_primary)
> >  	/* Setup PEX window registers */
> >  	setup_pci_atmu(hose, &rsrc);
> > =20
> > +	/*Setup PEXCSRBAR */
> > +#ifdef CONFIG_PCI_MSI
> > +	setup_pci_pcsrbar(hose);
> > +#endif
> >  	return 0;
> >  }
>=20
>=20
> cheers
>=20

RE: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Michael Ellerman]

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

On Mon, 2008-04-21 at 18:01 +0800, Jin Zhengxiong wrote:
> Hi, Michael,
> 
> Thank you very much for you input, please see my inline answer.

No worries.

> > > +static int fsl_msi_reserve_dt_hwirqs(struct fsl_msi *msi)
> > > +{
> > > +	int i, len;
> > > +	const u32 *p;
> > > +
> > > +	p = of_get_property(msi->of_node, "msi-available-ranges", &len);
> > > +	if (!p) {
> > > +		pr_debug("fsl_msi: no msi-available-ranges 
> > property found \
> > > +				on %s\n", msi->of_node->full_name);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	if (len & 0x8 != 0) {
> > > +		printk(KERN_WARNING "fsl_msi: Malformed 
> > msi-available-ranges "
> > > +		       "property on %s\n", msi->of_node->full_name);
> > > +		return -EINVAL;
> > > +	}
> > 
> > Do you really want a bitwise and with 0x8?
> > 
> The range for the msi interrupt can be seperated to several part. 
> This can used to check the if the ranges is correct. 

I don't see how. AFAIK the "msi-available-ranges" property is just a
list of u32 pairs, so the only thing that makes sense is to check that
the length is a multiple of 8, not that it has the 3rd bit set.

> > > +static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> > > +				  struct msi_msg *msg)
> > > +{
> > > +	unsigned int srs;
> > > +	unsigned int ibs;
> > > +	struct fsl_msi *msi = fsl_msi;
> > > +
> > > +	srs = hwirq / INT_PER_MSIR;
> > > +	ibs = hwirq % INT_PER_MSIR;
> > > +
> > > +	msg->address_lo = msi->msi_addr_lo;
> > > +	msg->address_hi = msi->msi_addr_hi;
> > > +	msg->data = (srs << 5) | (ibs & 0x1F);
> > 
> > Is the 5 and 0x1F independent of the INT_PER_MSIR value? Given the
> > current values isn't this a no-op, or am I missing something?
> > 
> Do you mean there're another way to get the msg->data from the hwirq?  

No I mean I'm confused about the maths here. If we pull out the
variables it boils down to:

data = ((hwirq / 32) << 5) | ((hwirq % 32) & 0x1F)

Which doesn't seem to actually do anything?

> > > +static int fsl_setup_msi_irqs(struct pci_dev *pdev, int 
> > nvec, int type)
> > > +{
> > > +	irq_hw_number_t hwirq;
> > > +	int rc;
> > > +	unsigned int virq;
> > > +	struct msi_desc *entry;
> > > +	struct msi_msg msg;
> > > +	struct fsl_msi *msi = fsl_msi;
> > 
> > A couple of places you put this into a local called "msi" 
> > which is not the
> > greatest name in the world IMHO :)
> > 
> Thank you, I'll try to use another name, Do you have any suggestion?

Oh I dunno, maybe msi_data or msi_state ? 


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: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Jin Zhengxiong]

=20

> -----Original Message-----
> From: Michael Ellerman [mailto:michael@ellerman.id.au]=20
> Sent: Tuesday, April 22, 2008 12:35 PM
> To: Jin Zhengxiong
> Cc: linuxppc-dev list; Gala Kumar
> Subject: RE: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu
>=20
> On Mon, 2008-04-21 at 18:01 +0800, Jin Zhengxiong wrote:
> > Hi, Michael,
> >=20
> > Thank you very much for you input, please see my inline answer.
>=20
> No worries.
>=20
> > > > +static int fsl_msi_reserve_dt_hwirqs(struct fsl_msi *msi) {
> > > > +	int i, len;
> > > > +	const u32 *p;
> > > > +
> > > > +	p =3D of_get_property(msi->of_node,=20
> "msi-available-ranges", &len);
> > > > +	if (!p) {
> > > > +		pr_debug("fsl_msi: no msi-available-ranges
> > > property found \
> > > > +				on %s\n",=20
> msi->of_node->full_name);
> > > > +		return -ENODEV;
> > > > +	}
> > > > +
> > > > +	if (len & 0x8 !=3D 0) {
> > > > +		printk(KERN_WARNING "fsl_msi: Malformed
> > > msi-available-ranges "
> > > > +		       "property on %s\n",=20
> msi->of_node->full_name);
> > > > +		return -EINVAL;
> > > > +	}
> > >=20
> > > Do you really want a bitwise and with 0x8?
> > >=20
> > The range for the msi interrupt can be seperated to several part.=20
> > This can used to check the if the ranges is correct.=20
>=20
> I don't see how. AFAIK the "msi-available-ranges" property is=20
> just a list of u32 pairs, so the only thing that makes sense=20
> is to check that the length is a multiple of 8, not that it=20
> has the 3rd bit set.
>=20
I found the problem. I'll check the length if it's a multiple of 8.
Thanks

> > > > +static void fsl_compose_msi_msg(struct pci_dev *pdev,=20
> int hwirq,
> > > > +				  struct msi_msg *msg)
> > > > +{
> > > > +	unsigned int srs;
> > > > +	unsigned int ibs;
> > > > +	struct fsl_msi *msi =3D fsl_msi;
> > > > +
> > > > +	srs =3D hwirq / INT_PER_MSIR;
> > > > +	ibs =3D hwirq % 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);
> > >=20
> > > Is the 5 and 0x1F independent of the INT_PER_MSIR value?=20
> Given the=20
> > > current values isn't this a no-op, or am I missing something?
> > >=20
> > Do you mean there're another way to get the msg->data from=20
> the hwirq? =20
>=20
> No I mean I'm confused about the maths here. If we pull out=20
> the variables it boils down to:
>=20
> data =3D ((hwirq / 32) << 5) | ((hwirq % 32) & 0x1F)
>=20
> Which doesn't seem to actually do anything?
>=20
Thanks, The hwirq number can stand for the data to the msiir in this
case.
I'll change this.

Re: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Segher Boessenkool]

>>>> +static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
>>>> +				  struct msi_msg *msg)

> No I mean I'm confused about the maths here. If we pull out the
> variables it boils down to:
>
> data = ((hwirq / 32) << 5) | ((hwirq % 32) & 0x1F)
>
> Which doesn't seem to actually do anything?

It's not a no-op, because hwirq is signed.  It probably should be
unsigned, like most things.


Segher

Re: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Michael Ellerman]

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

On Tue, 2008-04-22 at 15:22 +0200, Segher Boessenkool wrote:
> >>>> +static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> >>>> +				  struct msi_msg *msg)
> 
> > No I mean I'm confused about the maths here. If we pull out the
> > variables it boils down to:
> >
> > data = ((hwirq / 32) << 5) | ((hwirq % 32) & 0x1F)
> >
> > Which doesn't seem to actually do anything?
> 
> It's not a no-op, because hwirq is signed.  It probably should be
> unsigned, like most things.

You'll have to draw me a picture.

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: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Segher Boessenkool]

>>> data = ((hwirq / 32) << 5) | ((hwirq % 32) & 0x1F)
>>>
>>> Which doesn't seem to actually do anything?
>>
>> It's not a no-op, because hwirq is signed.  It probably should be
>> unsigned, like most things.
>
> You'll have to draw me a picture.

In C, signed division is round-towards-zero, while unsigned division
is round-towards-negative-infinity.  Suppose hwirq is -1, then
hwirq/32 is 0 and hwirq%32 is -1, so that the full expression above
will be 0x1f, not -1.  There is no such problem if hwirq would be
unsigned; the compiler can generate better code in that case.


Segher