[PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node

[PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node

[Minghuan Lian]

For the latest platform T4 and B4, MPIC controller has been updated
to v4.3. This patch adds a new file to describe the latest MPIC.
The MSI blocks number is increased to four, the registers number
of each block is increased to sixteen. MSIIR1 has been added to
access these sixteen MSI registers.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 arch/powerpc/boot/dts/fsl/b4si-post.dtsi     |   2 +-
 arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi | 153 +++++++++++++++++++++++++++
 arch/powerpc/boot/dts/fsl/t4240si-post.dtsi  |   2 +-
 3 files changed, 155 insertions(+), 2 deletions(-)
 create mode 100644 arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi

diff --git a/arch/powerpc/boot/dts/fsl/b4si-post.dtsi b/arch/powerpc/boot/dts/fsl/b4si-post.dtsi
index 7399154..4c617bf 100644
--- a/arch/powerpc/boot/dts/fsl/b4si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/b4si-post.dtsi
@@ -204,7 +204,7 @@
 		};
 	};
 
-/include/ "qoriq-mpic.dtsi"
+/include/ "qoriq-mpic4.3.dtsi"
 
 	guts: global-utilities@e0000 {
 		compatible = "fsl,b4-device-config";
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi
new file mode 100644
index 0000000..e2665b8
--- /dev/null
+++ b/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi
@@ -0,0 +1,153 @@
+/*
+ * QorIQ MPIC device tree stub [ controller @ offset 0x40000 ]
+ *
+ * Copyright 2013 Freescale Semiconductor Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *     * Neither the name of Freescale Semiconductor nor the
+ *       names of its contributors may be used to endorse or promote products
+ *       derived from this software without specific prior written permission.
+ *
+ *
+ * ALTERNATIVELY, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") as published by the Free Software
+ * Foundation, either version 2 of that License or (at your option) any
+ * later version.
+ *
+ * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+mpic: pic@40000 {
+	interrupt-controller;
+	#address-cells = <0>;
+	#interrupt-cells = <4>;
+	reg = <0x40000 0x40000>;
+	compatible = "fsl,mpic";
+	device_type = "open-pic";
+	clock-frequency = <0x0>;
+};
+
+timer@41100 {
+	compatible = "fsl,mpic-global-timer";
+	reg = <0x41100 0x100 0x41300 4>;
+	interrupts = <0 0 3 0
+		      1 0 3 0
+		      2 0 3 0
+		      3 0 3 0>;
+};
+
+msi0: msi@41600 {
+	compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
+	reg = <0x41600 0x200 0x44148 4>;
+	msi-available-ranges = <0 0x200>;
+	interrupts = <
+		0xe0 0 0 0
+		0xe1 0 0 0
+		0xe2 0 0 0
+		0xe3 0 0 0
+		0xe4 0 0 0
+		0xe5 0 0 0
+		0xe6 0 0 0
+		0xe7 0 0 0
+		0x100 0 0 0
+		0x101 0 0 0
+		0x102 0 0 0
+		0x103 0 0 0
+		0x104 0 0 0
+		0x105 0 0 0
+		0x106 0 0 0
+		0x107 0 0 0>;
+};
+
+msi1: msi@41800 {
+	compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
+	reg = <0x41800 0x200 0x45148 4>;
+	msi-available-ranges = <0 0x200>;
+	interrupts = <
+		0xe8 0 0 0
+		0xe9 0 0 0
+		0xea 0 0 0
+		0xeb 0 0 0
+		0xec 0 0 0
+		0xed 0 0 0
+		0xee 0 0 0
+		0xef 0 0 0
+		0x108 0 0 0
+		0x109 0 0 0
+		0x10a 0 0 0
+		0x10b 0 0 0
+		0x10c 0 0 0
+		0x10d 0 0 0
+		0x10e 0 0 0
+		0x10f 0 0 0>;
+};
+
+msi2: msi@41a00 {
+	compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
+	reg = <0x41a00 0x200 0x46148 4>;
+	msi-available-ranges = <0 0x200>;
+	interrupts = <
+		0xf0 0 0 0
+		0xf1 0 0 0
+		0xf2 0 0 0
+		0xf3 0 0 0
+		0xf4 0 0 0
+		0xf5 0 0 0
+		0xf6 0 0 0
+		0xf7 0 0 0
+		0x110 0 0 0
+		0x111 0 0 0
+		0x112 0 0 0
+		0x113 0 0 0
+		0x114 0 0 0
+		0x115 0 0 0
+		0x116 0 0 0
+		0x117 0 0 0>;
+};
+
+msi3: msi@41c00 {
+	compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
+	reg = <0x41c00 0x200 0x47148 4>;
+	msi-available-ranges = <0 0x200>;
+	interrupts = <
+		0xf8 0 0 0
+		0xf9 0 0 0
+		0xfa 0 0 0
+		0xfb 0 0 0
+		0xfc 0 0 0
+		0xfd 0 0 0
+		0xfe 0 0 0
+		0xff 0 0 0
+		0x118 0 0 0
+		0x119 0 0 0
+		0x11a 0 0 0
+		0x11b 0 0 0
+		0x11c 0 0 0
+		0x11d 0 0 0
+		0x11e 0 0 0
+		0x11f 0 0 0>;
+};
+
+timer@42100 {
+	compatible = "fsl,mpic-global-timer";
+	reg = <0x42100 0x100 0x42300 4>;
+	interrupts = <4 0 3 0
+		      5 0 3 0
+		      6 0 3 0
+		      7 0 3 0>;
+};
diff --git a/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi b/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
index bd611a9..510afa3 100644
--- a/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
@@ -358,7 +358,7 @@
 			16 2 1 30>;
 	};
 
-/include/ "qoriq-mpic.dtsi"
+/include/ "qoriq-mpic4.3.dtsi"
 
 	guts: global-utilities@e0000 {
 		compatible = "fsl,t4240-device-config", "fsl,qoriq-device-config-2.0";
-- 
1.8.1.2

[PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3

[Minghuan Lian]

MPIC controller v4.3 provides MSIIR1 to index 16 MSI registers.
MSIIR can only index 8 MSI registers. MSIIR1 uses different bits
definition than MSIIR. This patch adds ibs_shift and srs_shift to
indicate the bits definition of the MSIIR and MSIIR1, so the same
code can handle the MSIIR and MSIIR1 simultaneously.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 arch/powerpc/sysdev/fsl_msi.c | 62 ++++++++++++++++++++++++++++++++++---------
 arch/powerpc/sysdev/fsl_msi.h |  4 ++-
 2 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
index ab02db3..34510b7 100644
--- a/arch/powerpc/sysdev/fsl_msi.c
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -28,6 +28,18 @@
 #include "fsl_msi.h"
 #include "fsl_pci.h"
 
+#define MSIIR_OFFSET_MASK	0xfffff
+#define MSIIR_IBS_SHIFT		0
+#define MSIIR_SRS_SHIFT		5
+#define MSIIR1_IBS_SHIFT	4
+#define MSIIR1_SRS_SHIFT	0
+#define MSI_SRS_MASK		0xf
+#define MSI_IBS_MASK		0x1f
+
+#define msi_hwirq(msi, msir_index, intr_index) \
+		((msir_index) << (msi)->srs_shift | \
+		 ((intr_index) << (msi)->ibs_shift))
+
 static LIST_HEAD(msi_head);
 
 struct fsl_msi_feature {
@@ -80,18 +92,19 @@ static const struct irq_domain_ops fsl_msi_host_ops = {
 
 static int fsl_msi_init_allocator(struct fsl_msi *msi_data)
 {
-	int rc;
+	int rc, hwirq;
 
 	rc = msi_bitmap_alloc(&msi_data->bitmap, NR_MSI_IRQS,
 			      msi_data->irqhost->of_node);
 	if (rc)
 		return rc;
 
-	rc = msi_bitmap_reserve_dt_hwirqs(&msi_data->bitmap);
-	if (rc < 0) {
-		msi_bitmap_free(&msi_data->bitmap);
-		return rc;
-	}
+	/*
+	 * Reserve all the hwirqs
+	 * The available hwirqs will be released in fsl_msi_setup_hwirq()
+	 */
+	for (hwirq = 0; hwirq < NR_MSI_IRQS; hwirq++)
+		msi_bitmap_reserve_hwirq(&msi_data->bitmap, hwirq);
 
 	return 0;
 }
@@ -144,8 +157,9 @@ static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
 
 	msg->data = hwirq;
 
-	pr_debug("%s: allocated srs: %d, ibs: %d\n",
-		__func__, hwirq / IRQS_PER_MSI_REG, hwirq % IRQS_PER_MSI_REG);
+	pr_debug("%s: allocated srs: %d, ibs: %d\n", __func__,
+		 (hwirq >> msi_data->srs_shift) & MSI_SRS_MASK,
+		 (hwirq >> msi_data->ibs_shift) & MSI_IBS_MASK);
 }
 
 static int fsl_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
@@ -285,8 +299,8 @@ static void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
 		intr_index = ffs(msir_value) - 1;
 
 		cascade_irq = irq_linear_revmap(msi_data->irqhost,
-				msir_index * IRQS_PER_MSI_REG +
-					intr_index + have_shift);
+				msi_hwirq(msi_data, msir_index,
+					  intr_index + have_shift));
 		if (cascade_irq != NO_IRQ)
 			generic_handle_irq(cascade_irq);
 		have_shift += intr_index + 1;
@@ -339,7 +353,7 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev,
 			       int offset, int irq_index)
 {
 	struct fsl_msi_cascade_data *cascade_data = NULL;
-	int virt_msir;
+	int virt_msir, i;
 
 	virt_msir = irq_of_parse_and_map(dev->dev.of_node, irq_index);
 	if (virt_msir == NO_IRQ) {
@@ -360,6 +374,11 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev,
 	irq_set_handler_data(virt_msir, cascade_data);
 	irq_set_chained_handler(virt_msir, fsl_msi_cascade);
 
+	/* Release the hwirqs corresponding to this MSI register */
+	for (i = 0; i < IRQS_PER_MSI_REG; i++)
+		msi_bitmap_free_hwirqs(&msi->bitmap,
+				       msi_hwirq(msi, offset, i), 1);
+
 	return 0;
 }
 
@@ -368,7 +387,7 @@ static int fsl_of_msi_probe(struct platform_device *dev)
 {
 	const struct of_device_id *match;
 	struct fsl_msi *msi;
-	struct resource res;
+	struct resource res, msiir;
 	int err, i, j, irq_index, count;
 	int rc;
 	const u32 *p;
@@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct platform_device *dev)
 		}
 		msi->msiir_offset =
 			features->msiir_offset + (res.start & 0xfffff);
+
+		/*
+		 * First read the MSIIR/MSIIR1 offset from dts
+		 * If failure use the hardcode MSIIR offset
+		 */
+		if (of_address_to_resource(dev->dev.of_node, 1, &msiir))
+			msi->msiir_offset = features->msiir_offset +
+					    (res.start & MSIIR_OFFSET_MASK);
+		else
+			msi->msiir_offset = msiir.start & MSIIR_OFFSET_MASK;
 	}
 
 	msi->feature = features->fsl_pic_ip;
 
+	if (of_device_is_compatible(dev->dev.of_node, "fsl,mpic-msi-v4.3")) {
+		msi->srs_shift = MSIIR1_SRS_SHIFT;
+		msi->ibs_shift = MSIIR1_IBS_SHIFT;
+
+	} else {
+		msi->srs_shift = MSIIR_SRS_SHIFT;
+		msi->ibs_shift = MSIIR_IBS_SHIFT;
+	}
+
 	/*
 	 * Remember the phandle, so that we can match with any PCI nodes
 	 * that have an "fsl,msi" property.
diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h
index 8225f86..43a9d99 100644
--- a/arch/powerpc/sysdev/fsl_msi.h
+++ b/arch/powerpc/sysdev/fsl_msi.h
@@ -16,7 +16,7 @@
 #include <linux/of.h>
 #include <asm/msi_bitmap.h>
 
-#define NR_MSI_REG		8
+#define NR_MSI_REG		16
 #define IRQS_PER_MSI_REG	32
 #define NR_MSI_IRQS	(NR_MSI_REG * IRQS_PER_MSI_REG)
 
@@ -31,6 +31,8 @@ struct fsl_msi {
 	unsigned long cascade_irq;
 
 	u32 msiir_offset; /* Offset of MSIIR, relative to start of CCSR */
+	u32 ibs_shift; /* Shift of interrupt bit select */
+	u32 srs_shift; /* Shift of the shared interrupt register select */
 	void __iomem *msi_regs;
 	u32 feature;
 	int msi_virqs[NR_MSI_REG];
-- 
1.8.1.2

[PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3

[Minghuan Lian]

Add compatible "fsl,mpic-msi-v4.3" for MPIC v4.3. MPIC v4.3 contains
MSIIR and MSIIR1. MSIIR supports 8 MSI registers and MSIIR1 supports
16 MSI registers, but uses different IBS and SRS shift. When using
MSIR1, the interrupt number is not consecutive. It is hard to use
'msi-available-ranges' to describe the ranges of the available
interrupt and the ranges are related to the application, rather than
the description of the hardware. this patch also removes
'msi-available-ranges' property.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 .../devicetree/bindings/powerpc/fsl/msi-pic.txt    | 49 ++++++++++------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
index 5693877..e851e93 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
+++ b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
@@ -1,26 +1,23 @@
 * Freescale MSI interrupt controller
 
 Required properties:
-- compatible : compatible list, contains 2 entries,
+- compatible : compatible list, may contains one or two entries,
   first is "fsl,CHIP-msi", where CHIP is the processor(mpc8610, mpc8572,
-  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" depending on
-  the parent type.
+  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" or
+  "fsl,mpic-msi-v4.3" depending on the parent type and version. If mpic
+  version is 4.3, the number of MSI registers is increased to 16, MSIIR1 is
+  provided to access these 16 registers, compatible "fsl,mpic-msi-v4.3"
+  should be used.
 
 - reg : It may contain one or two regions. The first region should contain
   the address and the length of the shared message interrupt register set.
-  The second region should contain the address of aliased MSIIR register for
-  platforms that have such an alias.
-
-- msi-available-ranges: use <start count> style section to define which
-  msi interrupt can be used in the 256 msi interrupts. This property is
-  optional, without this, all the 256 MSI interrupts can be used.
-  Each available range must begin and end on a multiple of 32 (i.e.
-  no splitting an individual MSI register or the associated PIC interrupt).
+  The second region should contain the address of aliased MSIIR or MSIIR1
+  register for platforms that have such an alias, if using MSIIR1, the second
+  region must be added because different MSI group has different MSIRR1 offset.
 
 - interrupts : each one of the interrupts here is one entry per 32 MSIs,
   and routed to the host interrupt controller. the interrupts should
-  be set as edge sensitive.  If msi-available-ranges is present, only
-  the interrupts that correspond to available ranges shall be present.
+  be set as edge sensitive.
 
 - interrupt-parent: the phandle for the interrupt controller
   that services interrupts for this device. for 83xx cpu, the interrupts
@@ -39,20 +36,18 @@ Optional properties:
 
 Example:
 	msi@41600 {
-		compatible = "fsl,mpc8610-msi", "fsl,mpic-msi";
-		reg = <0x41600 0x80>;
-		msi-available-ranges = <0 0x100>;
-		interrupts = <
-			0xe0 0
-			0xe1 0
-			0xe2 0
-			0xe3 0
-			0xe4 0
-			0xe5 0
-			0xe6 0
-			0xe7 0>;
-		interrupt-parent = <&mpic>;
-	};
+	compatible = "fsl,mpic-msi";
+	reg = <0x41600 0x200 0x44140 4>;
+	interrupts = <
+		0xe0 0 0 0
+		0xe1 0 0 0
+		0xe2 0 0 0
+		0xe3 0 0 0
+		0xe4 0 0 0
+		0xe5 0 0 0
+		0xe6 0 0 0
+		0xe7 0 0 0>;
+};
 
 The Freescale hypervisor and msi-address-64
 -------------------------------------------
-- 
1.8.1.2

[PATCH 4/5] powerpc/dts: remove msi-available-ranges property

[Minghuan Lian]

For MPIC v4.3 MSIIR supports 8 MSI registers and MSIIR1 supports
16 MSI registers, but uses different IBS and SRS shift. For the
first register, when using MSIIR we will get the irqs 0x0 0x1 0x2
...0x1f, but when using MSIIR1, the irqs are 0x0 0x10 0x20 ... 0x1f0
It is hard to describe the available irqs using property
'msi-available-ranges'. The patch removes this property.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi      | 1 -
 arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi    | 3 ---
 arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi | 4 ----
 3 files changed, 8 deletions(-)

diff --git a/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi b/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
index 71c30eb..1ac4f23 100644
--- a/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
+++ b/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
@@ -66,7 +66,6 @@ message@41400 {
 msi@41600 {
 	compatible = "fsl,mpic-msi";
 	reg = <0x41600 0x80>;
-	msi-available-ranges = <0 0x100>;
 	interrupts = <
 		0xe0 0 0 0
 		0xe1 0 0 0
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi
index 08f4227..cf7355c 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-mpic.dtsi
@@ -54,7 +54,6 @@ timer@41100 {
 msi0: msi@41600 {
 	compatible = "fsl,mpic-msi";
 	reg = <0x41600 0x200 0x44140 4>;
-	msi-available-ranges = <0 0x100>;
 	interrupts = <
 		0xe0 0 0 0
 		0xe1 0 0 0
@@ -69,7 +68,6 @@ msi0: msi@41600 {
 msi1: msi@41800 {
 	compatible = "fsl,mpic-msi";
 	reg = <0x41800 0x200 0x45140 4>;
-	msi-available-ranges = <0 0x100>;
 	interrupts = <
 		0xe8 0 0 0
 		0xe9 0 0 0
@@ -84,7 +82,6 @@ msi1: msi@41800 {
 msi2: msi@41a00 {
 	compatible = "fsl,mpic-msi";
 	reg = <0x41a00 0x200 0x46140 4>;
-	msi-available-ranges = <0 0x100>;
 	interrupts = <
 		0xf0 0 0 0
 		0xf1 0 0 0
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi
index e2665b8..8a997ea 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-mpic4.3.dtsi
@@ -54,7 +54,6 @@ timer@41100 {
 msi0: msi@41600 {
 	compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
 	reg = <0x41600 0x200 0x44148 4>;
-	msi-available-ranges = <0 0x200>;
 	interrupts = <
 		0xe0 0 0 0
 		0xe1 0 0 0
@@ -77,7 +76,6 @@ msi0: msi@41600 {
 msi1: msi@41800 {
 	compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
 	reg = <0x41800 0x200 0x45148 4>;
-	msi-available-ranges = <0 0x200>;
 	interrupts = <
 		0xe8 0 0 0
 		0xe9 0 0 0
@@ -100,7 +98,6 @@ msi1: msi@41800 {
 msi2: msi@41a00 {
 	compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
 	reg = <0x41a00 0x200 0x46148 4>;
-	msi-available-ranges = <0 0x200>;
 	interrupts = <
 		0xf0 0 0 0
 		0xf1 0 0 0
@@ -123,7 +120,6 @@ msi2: msi@41a00 {
 msi3: msi@41c00 {
 	compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
 	reg = <0x41c00 0x200 0x47148 4>;
-	msi-available-ranges = <0 0x200>;
 	interrupts = <
 		0xf8 0 0 0
 		0xf9 0 0 0
-- 
1.8.1.2

[PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter

[Minghuan Lian]

1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
the IRQs of a register are not continuous. for example, the first
register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
is hard to use 'msi-available-ranges' property to indicate the
available ranges and 'msi-available-ranges' property has been
removed from dts node, so this patch removes the related code.

2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
functionality. 'msiregs' is used to indicate the available MSI
registers ranges and uses a colon ':' to separate the multiple
banks. The range representation format is 'start-end', 'start'
and 'end' are integers describe the start and end register index,
the available registers lies between start and end and not include
end. For example, the available register x satisfying
start <= x < end.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 arch/powerpc/sysdev/fsl_msi.c | 118 ++++++++++++++++++++++++++++--------------
 arch/powerpc/sysdev/fsl_msi.h |   1 +
 2 files changed, 80 insertions(+), 39 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
index 34510b7..db382ef9b 100644
--- a/arch/powerpc/sysdev/fsl_msi.c
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -52,6 +52,60 @@ struct fsl_msi_cascade_data {
 	int index;
 };
 
+struct msi_reg_range {
+	u32 start;
+	u32 end;
+};
+
+static struct msi_reg_range msiregs[NR_MSI_BANK] = {
+	{.start = 0, .end = NR_MSI_REG },
+	{.start = 0, .end = NR_MSI_REG },
+	{.start = 0, .end = NR_MSI_REG },
+	{.start = 0, .end = NR_MSI_REG },
+};
+
+/*
+ * Handle 'msiregs' parameter.
+ * msiregs is used to indicate the available MSI registers range and
+ * uses colon ':' to separate the multiple banks ranges.
+ * For each bank, the registers range format is 'start-end'
+ * start and end are integers, used to the indicate the start and end
+ * register index. The range is a set of real numbers that lies between
+ * start and end but not include end. For example, the set of all numbers
+ * x satisfying start <= x < end.
+ * if no range specified, driver will use the default range including all
+ * the registers.
+ * if you do no want to use this bank, you can set range as '0-0'
+ * For example msiregs=0-16:0-0::0-2
+ */
+static int msi_regs_setup(char *s)
+{
+	int bank = 0;
+	char *p;
+	struct msi_reg_range *range;
+
+	while ((p = strsep(&s, ":")) != NULL) {
+		int start = 0, end = NR_MSI_REG;
+
+		if (bank >= NR_MSI_BANK)
+			break;
+		range = &msiregs[bank];
+
+		if ((*p != '\0') && (sscanf(p, "%d-%d", &start, &end) < 1))
+			pr_err("msiregs correct format is: start-end\n");
+
+		/* Ok, gets the specified value */
+		range->start = start;
+		range->end = end;
+		pr_info("MSI bank%d available regs range is %d-%d\n",
+			 bank, range->start, range->end);
+		bank++;
+	}
+	return 1;
+}
+
+__setup("msiregs=", msi_regs_setup);
+
 static inline u32 fsl_msi_read(u32 __iomem *base, unsigned int reg)
 {
 	return in_be32(base + (reg >> 2));
@@ -350,7 +404,7 @@ static int fsl_of_msi_remove(struct platform_device *ofdev)
 static struct lock_class_key fsl_msi_irq_class;
 
 static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev,
-			       int offset, int irq_index)
+			       int irq_index)
 {
 	struct fsl_msi_cascade_data *cascade_data = NULL;
 	int virt_msir, i;
@@ -369,7 +423,7 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev,
 	}
 	irq_set_lockdep_class(virt_msir, &fsl_msi_irq_class);
 	msi->msi_virqs[irq_index] = virt_msir;
-	cascade_data->index = offset;
+	cascade_data->index = irq_index;
 	cascade_data->msi_data = msi;
 	irq_set_handler_data(virt_msir, cascade_data);
 	irq_set_chained_handler(virt_msir, fsl_msi_cascade);
@@ -377,7 +431,7 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev,
 	/* Release the hwirqs corresponding to this MSI register */
 	for (i = 0; i < IRQS_PER_MSI_REG; i++)
 		msi_bitmap_free_hwirqs(&msi->bitmap,
-				       msi_hwirq(msi, offset, i), 1);
+				       msi_hwirq(msi, irq_index, i), 1);
 
 	return 0;
 }
@@ -387,21 +441,29 @@ static int fsl_of_msi_probe(struct platform_device *dev)
 {
 	const struct of_device_id *match;
 	struct fsl_msi *msi;
+	static int bank;
+	struct msi_reg_range *range;
 	struct resource res, msiir;
-	int err, i, j, irq_index, count;
+	int err, irq_index, count;
 	int rc;
-	const u32 *p;
 	const struct fsl_msi_feature *features;
-	int len;
-	u32 offset;
-	static const u32 all_avail[] = { 0, NR_MSI_IRQS };
 
 	match = of_match_device(fsl_of_msi_ids, &dev->dev);
 	if (!match)
 		return -EINVAL;
 	features = match->data;
 
-	printk(KERN_DEBUG "Setting up Freescale MSI support\n");
+	if (bank >= NR_MSI_BANK)
+		return -EINVAL;
+	range = &msiregs[bank];
+	pr_debug("Setting up Freescale MSI bank%d support\n", bank);
+
+	count = of_irq_count(dev->dev.of_node);
+	if (!count)
+		return -ENODEV;
+
+	if (count > NR_MSI_REG)
+		count = NR_MSI_REG;
 
 	msi = kzalloc(sizeof(struct fsl_msi), GFP_KERNEL);
 	if (!msi) {
@@ -475,39 +537,17 @@ static int fsl_of_msi_probe(struct platform_device *dev)
 		goto error_out;
 	}
 
-	p = of_get_property(dev->dev.of_node, "msi-available-ranges", &len);
-	if (p && len % (2 * sizeof(u32)) != 0) {
-		dev_err(&dev->dev, "%s: Malformed msi-available-ranges property\n",
-			__func__);
-		err = -EINVAL;
-		goto error_out;
-	}
-
-	if (!p) {
-		p = all_avail;
-		len = sizeof(all_avail);
-	}
-
-	for (irq_index = 0, i = 0; i < len / (2 * sizeof(u32)); i++) {
-		if (p[i * 2] % IRQS_PER_MSI_REG ||
-		    p[i * 2 + 1] % IRQS_PER_MSI_REG) {
-			printk(KERN_WARNING "%s: %s: msi available range of %u at %u is not IRQ-aligned\n",
-			       __func__, dev->dev.of_node->full_name,
-			       p[i * 2 + 1], p[i * 2]);
-			err = -EINVAL;
+	for (irq_index = 0; irq_index < count; irq_index++) {
+		/* Check whether the register is contained in range */
+		if (irq_index < range->start ||
+			irq_index >= range->end)
+			continue;
+		err = fsl_msi_setup_hwirq(msi, dev, irq_index);
+		if (err)
 			goto error_out;
-		}
-
-		offset = p[i * 2] / IRQS_PER_MSI_REG;
-		count = p[i * 2 + 1] / IRQS_PER_MSI_REG;
-
-		for (j = 0; j < count; j++, irq_index++) {
-			err = fsl_msi_setup_hwirq(msi, dev, offset + j, irq_index);
-			if (err)
-				goto error_out;
-		}
 	}
 
+	bank++;
 	list_add_tail(&msi->list, &msi_head);
 
 	/* The multiple setting ppc_md.setup_msi_irqs will not harm things */
diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h
index 43a9d99..6048415 100644
--- a/arch/powerpc/sysdev/fsl_msi.h
+++ b/arch/powerpc/sysdev/fsl_msi.h
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <asm/msi_bitmap.h>
 
+#define NR_MSI_BANK		4
 #define NR_MSI_REG		16
 #define IRQS_PER_MSI_REG	32
 #define NR_MSI_IRQS	(NR_MSI_REG * IRQS_PER_MSI_REG)
-- 
1.8.1.2

Re: [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node

[Scott Wood]

On 06/14/2013 02:15:55 AM, Minghuan Lian wrote:
> +msi0: msi@41600 {
> +	compatible =3D "fsl,mpic-msi", "fsl,mpic-msi-v4.3";

More specific compatibles come first -- and I don't think this is 100% =20
backwards compatible with "fsl,mpic-msi" anyway.

-Scott=

Re: [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node

[Scott Wood]

On 06/14/2013 03:39:26 PM, Scott Wood wrote:
> On 06/14/2013 02:15:55 AM, Minghuan Lian wrote:
>> +msi0: msi@41600 {
>> +	compatible =3D "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
>=20
> More specific compatibles come first -- and I don't think this is =20
> 100% backwards compatible with "fsl,mpic-msi" anyway.

Also please update the binding.

-Scott=

Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3

[Scott Wood]

On 06/14/2013 02:15:57 AM, Minghuan Lian wrote:
> Add compatible "fsl,mpic-msi-v4.3" for MPIC v4.3. MPIC v4.3 contains
> MSIIR and MSIIR1. MSIIR supports 8 MSI registers and MSIIR1 supports
> 16 MSI registers, but uses different IBS and SRS shift. When using
> MSIR1, the interrupt number is not consecutive. It is hard to use
> 'msi-available-ranges' to describe the ranges of the available
> interrupt and the ranges are related to the application, rather than
> the description of the hardware. this patch also removes
> 'msi-available-ranges' property.
>=20
> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
> ---
>  .../devicetree/bindings/powerpc/fsl/msi-pic.txt    | 49 =20
> ++++++++++------------
>  1 file changed, 22 insertions(+), 27 deletions(-)
>=20
> diff --git =20
> a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt =20
> b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
> index 5693877..e851e93 100644
> --- a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
> +++ b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
> @@ -1,26 +1,23 @@
>  * Freescale MSI interrupt controller
>=20
>  Required properties:
> -- compatible : compatible list, contains 2 entries,
> +- compatible : compatible list, may contains one or two entries,
>    first is "fsl,CHIP-msi", where CHIP is the processor(mpc8610, =20
> mpc8572,
> -  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" depending =20
> on
> -  the parent type.
> +  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" or
> +  "fsl,mpic-msi-v4.3" depending on the parent type and version. If =20
> mpic
> +  version is 4.3, the number of MSI registers is increased to 16, =20
> MSIIR1 is
> +  provided to access these 16 registers, compatible =20
> "fsl,mpic-msi-v4.3"
> +  should be used.

Why "one or two"?  What does it look like in the case where there's =20
just one?

>  - reg : It may contain one or two regions. The first region should =20
> contain
>    the address and the length of the shared message interrupt =20
> register set.
> -  The second region should contain the address of aliased MSIIR =20
> register for
> -  platforms that have such an alias.
> -
> -- msi-available-ranges: use <start count> style section to define =20
> which
> -  msi interrupt can be used in the 256 msi interrupts. This property =20
> is
> -  optional, without this, all the 256 MSI interrupts can be used.
> -  Each available range must begin and end on a multiple of 32 (i.e.
> -  no splitting an individual MSI register or the associated PIC =20
> interrupt).
> +  The second region should contain the address of aliased MSIIR or =20
> MSIIR1
> +  register for platforms that have such an alias, if using MSIIR1, =20
> the second
> +  region must be added because different MSI group has different =20
> MSIRR1 offset.

Why are you removing msi-available-ranges?  It's not valid for MPIC =20
v4.3, but it's still valid for older MPICs.  It should move to the =20
optional section, though.

>  - interrupts : each one of the interrupts here is one entry per 32 =20
> MSIs,
>    and routed to the host interrupt controller. the interrupts should
> -  be set as edge sensitive.  If msi-available-ranges is present, only
> -  the interrupts that correspond to available ranges shall be =20
> present.
> +  be set as edge sensitive.
>=20
>  - interrupt-parent: the phandle for the interrupt controller
>    that services interrupts for this device. for 83xx cpu, the =20
> interrupts
> @@ -39,20 +36,18 @@ Optional properties:
>=20
>  Example:
>  	msi@41600 {
> -		compatible =3D "fsl,mpc8610-msi", "fsl,mpic-msi";
> -		reg =3D <0x41600 0x80>;
> -		msi-available-ranges =3D <0 0x100>;
> -		interrupts =3D <
> -			0xe0 0
> -			0xe1 0
> -			0xe2 0
> -			0xe3 0
> -			0xe4 0
> -			0xe5 0
> -			0xe6 0
> -			0xe7 0>;
> -		interrupt-parent =3D <&mpic>;
> -	};
> +	compatible =3D "fsl,mpic-msi";
> +	reg =3D <0x41600 0x200 0x44140 4>;

Why 0x200?

-Scott=

Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3

[Scott Wood]

On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
> @@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct =20
> platform_device *dev)
>  		}
>  		msi->msiir_offset =3D
>  			features->msiir_offset + (res.start & 0xfffff);
> +
> +		/*
> +		 * First read the MSIIR/MSIIR1 offset from dts
> +		 * If failure use the hardcode MSIIR offset

"On failure"

> +		 */
> +		if (of_address_to_resource(dev->dev.of_node, 1, &msiir))
> +			msi->msiir_offset =3D features->msiir_offset +
> +					    (res.start & =20
> MSIIR_OFFSET_MASK);
> +		else
> +			msi->msiir_offset =3D msiir.start & =20
> MSIIR_OFFSET_MASK;
>  	}
>=20
>  	msi->feature =3D features->fsl_pic_ip;
>=20
> +	if (of_device_is_compatible(dev->dev.of_node, =20
> "fsl,mpic-msi-v4.3")) {
> +		msi->srs_shift =3D MSIIR1_SRS_SHIFT;
> +		msi->ibs_shift =3D MSIIR1_IBS_SHIFT;
> +
> +	} else {
> +		msi->srs_shift =3D MSIIR_SRS_SHIFT;
> +		msi->ibs_shift =3D MSIIR_IBS_SHIFT;
> +	}

Remove the blank line just before the "} else {".

> diff --git a/arch/powerpc/sysdev/fsl_msi.h =20
> b/arch/powerpc/sysdev/fsl_msi.h
> index 8225f86..43a9d99 100644
> --- a/arch/powerpc/sysdev/fsl_msi.h
> +++ b/arch/powerpc/sysdev/fsl_msi.h
> @@ -16,7 +16,7 @@
>  #include <linux/of.h>
>  #include <asm/msi_bitmap.h>
>=20
> -#define NR_MSI_REG		8
> +#define NR_MSI_REG		16
>  #define IRQS_PER_MSI_REG	32
>  #define NR_MSI_IRQS	(NR_MSI_REG * IRQS_PER_MSI_REG)

I don't see where you update all_avail in fsl_of_msi_probe.

We should also be bounds-checking the contents of msi-available-ranges.
Currently it looks like we just silently overrun the bitmap if we get =20
bad
input from the device tree.

-Scott=

Re: [PATCH 4/5] powerpc/dts: remove msi-available-ranges property

[Scott Wood]

On 06/14/2013 02:15:58 AM, Minghuan Lian wrote:
> For MPIC v4.3 MSIIR supports 8 MSI registers and MSIIR1 supports
> 16 MSI registers, but uses different IBS and SRS shift. For the
> first register, when using MSIIR we will get the irqs 0x0 0x1 0x2
> ...0x1f, but when using MSIIR1, the irqs are 0x0 0x10 0x20 ... 0x1f0
> It is hard to describe the available irqs using property
> 'msi-available-ranges'. The patch removes this property.

Only remove it from mpic 4.3.  And since you introduced =20
qoriq-mpic4.3.dtsi earlier in the patchset, why didn't you just avoid =20
adding it then?

-Scott=

Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter

[Scott Wood]

On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
> the IRQs of a register are not continuous. for example, the first
> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
> is hard to use 'msi-available-ranges' property to indicate the
> available ranges and 'msi-available-ranges' property has been
> removed from dts node, so this patch removes the related code.
>=20
> 2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
> functionality.

The reason we used a device tree property was because this is for =20
virtualization and AMP scenarios where this instance of Linux does not =20
own all of the MSI registers.

I don't see any reasonable way to partition an MPIC v4.3 MSI group -- =20
but there are more groups, so it's not that bad.  What's the use case =20
for this patch?

-Scott=

Re: [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node

[Lian Minghuan-b31939]

Hi Scott,

please see my comments.

On 06/15/2013 05:53 AM, Scott Wood wrote:
> On 06/14/2013 03:39:26 PM, Scott Wood wrote:
>> On 06/14/2013 02:15:55 AM, Minghuan Lian wrote:
>>> +msi0: msi@41600 {
>>> +    compatible = "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
>>
>> More specific compatibles come first -- and I don't think this is 
>> 100% backwards compatible with "fsl,mpic-msi" anyway.
>
> Also please update the binding.
[Minghuan] Yes, maybe I should remove "fsl,mpic-msi". What do you think?
>
> -Scott

Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3

[Lian Minghuan-b31939]

Hi Scott,

please see my comments inline.

On 06/15/2013 06:09 AM, Scott Wood wrote:
> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
>> @@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct 
>> platform_device *dev)
>>          }
>>          msi->msiir_offset =
>>              features->msiir_offset + (res.start & 0xfffff);
>> +
>> +        /*
>> +         * First read the MSIIR/MSIIR1 offset from dts
>> +         * If failure use the hardcode MSIIR offset
>
> "On failure"
>
[Minghuan] OK, thanks.
>> +         */
>> +        if (of_address_to_resource(dev->dev.of_node, 1, &msiir))
>> +            msi->msiir_offset = features->msiir_offset +
>> +                        (res.start & MSIIR_OFFSET_MASK);
>> +        else
>> +            msi->msiir_offset = msiir.start & MSIIR_OFFSET_MASK;
>>      }
>>
>>      msi->feature = features->fsl_pic_ip;
>>
>> +    if (of_device_is_compatible(dev->dev.of_node, 
>> "fsl,mpic-msi-v4.3")) {
>> +        msi->srs_shift = MSIIR1_SRS_SHIFT;
>> +        msi->ibs_shift = MSIIR1_IBS_SHIFT;
>> +
>> +    } else {
>> +        msi->srs_shift = MSIIR_SRS_SHIFT;
>> +        msi->ibs_shift = MSIIR_IBS_SHIFT;
>> +    }
>
> Remove the blank line just before the "} else {".
>
[Minghuan] OK, Thanks.
>> diff --git a/arch/powerpc/sysdev/fsl_msi.h 
>> b/arch/powerpc/sysdev/fsl_msi.h
>> index 8225f86..43a9d99 100644
>> --- a/arch/powerpc/sysdev/fsl_msi.h
>> +++ b/arch/powerpc/sysdev/fsl_msi.h
>> @@ -16,7 +16,7 @@
>>  #include <linux/of.h>
>>  #include <asm/msi_bitmap.h>
>>
>> -#define NR_MSI_REG        8
>> +#define NR_MSI_REG        16
>>  #define IRQS_PER_MSI_REG    32
>>  #define NR_MSI_IRQS    (NR_MSI_REG * IRQS_PER_MSI_REG)
>
> I don't see where you update all_avail in fsl_of_msi_probe.
>
> We should also be bounds-checking the contents of msi-available-ranges.
> Currently it looks like we just silently overrun the bitmap if we get bad
> input from the device tree.
>
[Minghuan] all_avail definition: static const u32 all_avail[] = { 0, 
NR_MSI_IRQS };
When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to 16*32, 
and all_avail also is updated.

Before calling fsl_msi_setup_hwirq(), the code has checked 
'msi-available-ranges',  only the interrupts lied in 
'msi-available-ranges' will be initialized by call fsl_msi_setup_hwirq() 
, and the corresponding bitmap will be freed. I moved 
msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the code 
would generate different bitmap when using MSIIR or MSIIR1.

> -Scott

Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3

[Lian Minghuan-b31939]

Hi Soctt,

please see my comments.

On 06/15/2013 06:06 AM, Scott Wood wrote:
> On 06/14/2013 02:15:57 AM, Minghuan Lian wrote:
>> Add compatible "fsl,mpic-msi-v4.3" for MPIC v4.3. MPIC v4.3 contains
>> MSIIR and MSIIR1. MSIIR supports 8 MSI registers and MSIIR1 supports
>> 16 MSI registers, but uses different IBS and SRS shift. When using
>> MSIR1, the interrupt number is not consecutive. It is hard to use
>> 'msi-available-ranges' to describe the ranges of the available
>> interrupt and the ranges are related to the application, rather than
>> the description of the hardware. this patch also removes
>> 'msi-available-ranges' property.
>>
>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
>> ---
>>  .../devicetree/bindings/powerpc/fsl/msi-pic.txt    | 49 
>> ++++++++++------------
>>  1 file changed, 22 insertions(+), 27 deletions(-)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt 
>> b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> index 5693877..e851e93 100644
>> --- a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> +++ b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>> @@ -1,26 +1,23 @@
>>  * Freescale MSI interrupt controller
>>
>>  Required properties:
>> -- compatible : compatible list, contains 2 entries,
>> +- compatible : compatible list, may contains one or two entries,
>>    first is "fsl,CHIP-msi", where CHIP is the processor(mpc8610, 
>> mpc8572,
>> -  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" depending on
>> -  the parent type.
>> +  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" or
>> +  "fsl,mpic-msi-v4.3" depending on the parent type and version. If mpic
>> +  version is 4.3, the number of MSI registers is increased to 16, 
>> MSIIR1 is
>> +  provided to access these 16 registers, compatible "fsl,mpic-msi-v4.3"
>> +  should be used.
>
> Why "one or two"?  What does it look like in the case where there's 
> just one?
>
[Minghuan] The original doc said 'contains 2 entries', but I notcie 
pq3-mpic.dtsi and qoriq-mpic.dtsi have only one entry "fsl,mpic-msi", do 
not have "fsl,CHIP-msi".
for example:
mpc8610_hpcd.dts: compatible = "fsl,mpc8610-msi", "fsl,mpic-msi";
fsl/qoriq-mpic.dtsi:  compatible = "fsl,mpic-msi"

Maybe I should say " For some platforms, "fsl,CHIP-msi' is optional."

>>  - reg : It may contain one or two regions. The first region should 
>> contain
>>    the address and the length of the shared message interrupt 
>> register set.
>> -  The second region should contain the address of aliased MSIIR 
>> register for
>> -  platforms that have such an alias.
>> -
>> -- msi-available-ranges: use <start count> style section to define which
>> -  msi interrupt can be used in the 256 msi interrupts. This property is
>> -  optional, without this, all the 256 MSI interrupts can be used.
>> -  Each available range must begin and end on a multiple of 32 (i.e.
>> -  no splitting an individual MSI register or the associated PIC 
>> interrupt).
>> +  The second region should contain the address of aliased MSIIR or 
>> MSIIR1
>> +  register for platforms that have such an alias, if using MSIIR1, 
>> the second
>> +  region must be added because different MSI group has different 
>> MSIRR1 offset.
>
> Why are you removing msi-available-ranges?  It's not valid for MPIC 
> v4.3, but it's still valid for older MPICs.  It should move to the 
> optional section, though.
[Minghuan] Because I would like to add kernel parameter 'msiregs' 
instead of "msi-available-ranges", for all the MPICs, we will have a 
uniform way to configure
For older MPICs, we can use "msi-available-ranges" and "msiregs", but 
for MPCI4.3 we can only use "msiregs", this may easily lead to confusion.
>>  - interrupts : each one of the interrupts here is one entry per 32 
>> MSIs,
>>    and routed to the host interrupt controller. the interrupts should
>> -  be set as edge sensitive.  If msi-available-ranges is present, only
>> -  the interrupts that correspond to available ranges shall be present.
>> +  be set as edge sensitive.
>>
>>  - interrupt-parent: the phandle for the interrupt controller
>>    that services interrupts for this device. for 83xx cpu, the 
>> interrupts
>> @@ -39,20 +36,18 @@ Optional properties:
>>
>>  Example:
>>      msi@41600 {
>> -        compatible = "fsl,mpc8610-msi", "fsl,mpic-msi";
>> -        reg = <0x41600 0x80>;
>> -        msi-available-ranges = <0 0x100>;
>> -        interrupts = <
>> -            0xe0 0
>> -            0xe1 0
>> -            0xe2 0
>> -            0xe3 0
>> -            0xe4 0
>> -            0xe5 0
>> -            0xe6 0
>> -            0xe7 0>;
>> -        interrupt-parent = <&mpic>;
>> -    };
>> +    compatible = "fsl,mpic-msi";
>> +    reg = <0x41600 0x200 0x44140 4>;
>
> Why 0x200?
>
[Minghuan] The offsets of the MSIA registers are from 0x41600 to 
0x417ff, and the size is 0x200.
offset 0x41600-0x4170 are MSIIRA1-7.
0x41720 is MSISRA,
0x41750 is MSIIR.
The others are reserved.

-Scott

Re: [PATCH 4/5] powerpc/dts: remove msi-available-ranges property

[Lian Minghuan-b31939]

On 06/15/2013 06:10 AM, Scott Wood wrote:
> On 06/14/2013 02:15:58 AM, Minghuan Lian wrote:
>> For MPIC v4.3 MSIIR supports 8 MSI registers and MSIIR1 supports
>> 16 MSI registers, but uses different IBS and SRS shift. For the
>> first register, when using MSIIR we will get the irqs 0x0 0x1 0x2
>> ...0x1f, but when using MSIIR1, the irqs are 0x0 0x10 0x20 ... 0x1f0
>> It is hard to describe the available irqs using property
>> 'msi-available-ranges'. The patch removes this property.
>
> Only remove it from mpic 4.3.  And since you introduced 
> qoriq-mpic4.3.dtsi earlier in the patchset, why didn't you just avoid 
> adding it then?
>
[Minghuan] If adding it in qoriq-mpic4.3, and the 3-5 patches are not 
accepted, mpic4.3 can also work.
> -Scott

Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter

[Lian Minghuan-b31939]

Hi Scott,

please see my comments inline.

On 06/15/2013 06:13 AM, Scott Wood wrote:
> On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
>> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
>> the IRQs of a register are not continuous. for example, the first
>> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
>> is hard to use 'msi-available-ranges' property to indicate the
>> available ranges and 'msi-available-ranges' property has been
>> removed from dts node, so this patch removes the related code.
>>
>> 2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
>> functionality.
>
> The reason we used a device tree property was because this is for 
> virtualization and AMP scenarios where this instance of Linux does not 
> own all of the MSI registers.
>
> I don't see any reasonable way to partition an MPIC v4.3 MSI group -- 
> but there are more groups, so it's not that bad.  What's the use case 
> for this patch?
>
[Minghuan] I do not known any case about this patch. I add 'msiregs' 
just for achieving "msi-available-ranges" functionality. I do not want 
to remove partition functionality when updating to mpic4.3, although I 
do not see virtualization and AMP cases on T4(KVM does not need this 
functionality).
If you hope to keep 'msi-available-ranges' property for older mpic, can 
I add 'msi-available-regs' property for mpic4.3.
Using 'msi-available-regs' to implement partition is similar to 
'msi-available-ranges'. When considering the consistency, 
'msi-available-regs' is better than msiregs.
> -Scott

Re: [PATCH 4/5] powerpc/dts: remove msi-available-ranges property

[Scott Wood]

On 06/17/2013 12:15:36 AM, Lian Minghuan-b31939 wrote:
> On 06/15/2013 06:10 AM, Scott Wood wrote:
>> On 06/14/2013 02:15:58 AM, Minghuan Lian wrote:
>>> For MPIC v4.3 MSIIR supports 8 MSI registers and MSIIR1 supports
>>> 16 MSI registers, but uses different IBS and SRS shift. For the
>>> first register, when using MSIIR we will get the irqs 0x0 0x1 0x2
>>> ...0x1f, but when using MSIIR1, the irqs are 0x0 0x10 0x20 ... 0x1f0
>>> It is hard to describe the available irqs using property
>>> 'msi-available-ranges'. The patch removes this property.
>>=20
>> Only remove it from mpic 4.3.  And since you introduced =20
>> qoriq-mpic4.3.dtsi earlier in the patchset, why didn't you just =20
>> avoid adding it then?
>>=20
> [Minghuan] If adding it in qoriq-mpic4.3, and the 3-5 patches are not =20
> accepted, mpic4.3 can also work.

mpic 4.3 cannot work with msi-available-ranges, at all.  The hardware =20
just doesn't work that way.

-Scott=

Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3

[Scott Wood]

On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote:
> Hi Scott,
>=20
> please see my comments inline.
>=20
> On 06/15/2013 06:09 AM, Scott Wood wrote:
>> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
>>> diff --git a/arch/powerpc/sysdev/fsl_msi.h =20
>>> b/arch/powerpc/sysdev/fsl_msi.h
>>> index 8225f86..43a9d99 100644
>>> --- a/arch/powerpc/sysdev/fsl_msi.h
>>> +++ b/arch/powerpc/sysdev/fsl_msi.h
>>> @@ -16,7 +16,7 @@
>>>  #include <linux/of.h>
>>>  #include <asm/msi_bitmap.h>
>>>=20
>>> -#define NR_MSI_REG        8
>>> +#define NR_MSI_REG        16
>>>  #define IRQS_PER_MSI_REG    32
>>>  #define NR_MSI_IRQS    (NR_MSI_REG * IRQS_PER_MSI_REG)
>>=20
>> I don't see where you update all_avail in fsl_of_msi_probe.
>>=20
>> We should also be bounds-checking the contents of =20
>> msi-available-ranges.
>> Currently it looks like we just silently overrun the bitmap if we =20
>> get bad
>> input from the device tree.
>>=20
> [Minghuan] all_avail definition: static const u32 all_avail[] =3D { 0, =20
> NR_MSI_IRQS };
> When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to =20
> 16*32, and all_avail also is updated.

That's my point.  It shouldn't change for older hardware.

> Before calling fsl_msi_setup_hwirq(), the code has checked =20
> 'msi-available-ranges',  only the interrupts lied in =20
> 'msi-available-ranges' will be initialized by call =20
> fsl_msi_setup_hwirq() , and the corresponding bitmap will be freed. I =20
> moved msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the =20
> code would generate different bitmap when using MSIIR or MSIIR1.

And what happens if msi-available-ranges is bad, and refers to =20
non-existent MSIs past the end of the bitmap?

-Scott=

Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter

[Scott Wood]

On 06/17/2013 12:36:50 AM, Lian Minghuan-b31939 wrote:
> Hi Scott,
>=20
> please see my comments inline.
>=20
> On 06/15/2013 06:13 AM, Scott Wood wrote:
>> On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
>>> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
>>> the IRQs of a register are not continuous. for example, the first
>>> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
>>> is hard to use 'msi-available-ranges' property to indicate the
>>> available ranges and 'msi-available-ranges' property has been
>>> removed from dts node, so this patch removes the related code.
>>>=20
>>> 2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
>>> functionality.
>>=20
>> The reason we used a device tree property was because this is for =20
>> virtualization and AMP scenarios where this instance of Linux does =20
>> not own all of the MSI registers.
>>=20
>> I don't see any reasonable way to partition an MPIC v4.3 MSI group =20
>> -- but there are more groups, so it's not that bad.  What's the use =20
>> case for this patch?
>>=20
> [Minghuan] I do not known any case about this patch. I add 'msiregs' =20
> just for achieving "msi-available-ranges" functionality. I do not =20
> want to remove partition functionality when updating to mpic4.3, =20
> although I do not see virtualization and AMP cases on T4(KVM does not =20
> need this functionality).

Such functionality does not work on mpic v4.3.  There are conflicting =20
requirements of contiguous MSIs (because PCI devices can use them that =20
way) and the inability to partition a single register (because they all =20
go to the same MPIC interrupt).

Keep msi-available-ranges as is for older hardware, and just ignore it =20
(with a warning printed) if it's present on MPIC v4.3.

-Scott=

Re: [PATCH 1/5] powerpc/dts: add MPIC v4.3 dts node

[Scott Wood]

On 06/16/2013 09:23:37 PM, Lian Minghuan-b31939 wrote:
> Hi Scott,
>=20
> please see my comments.
>=20
> On 06/15/2013 05:53 AM, Scott Wood wrote:
>> On 06/14/2013 03:39:26 PM, Scott Wood wrote:
>>> On 06/14/2013 02:15:55 AM, Minghuan Lian wrote:
>>>> +msi0: msi@41600 {
>>>> +    compatible =3D "fsl,mpic-msi", "fsl,mpic-msi-v4.3";
>>>=20
>>> More specific compatibles come first -- and I don't think this is =20
>>> 100% backwards compatible with "fsl,mpic-msi" anyway.
>>=20
>> Also please update the binding.
> [Minghuan] Yes, maybe I should remove "fsl,mpic-msi". What do you =20
> think?

Yes.

-Scott=

Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3

[Scott Wood]

On 06/17/2013 12:07:41 AM, Lian Minghuan-b31939 wrote:
> Hi Soctt,
>=20
> please see my comments.
>=20
> On 06/15/2013 06:06 AM, Scott Wood wrote:
>> On 06/14/2013 02:15:57 AM, Minghuan Lian wrote:
>>> Add compatible "fsl,mpic-msi-v4.3" for MPIC v4.3. MPIC v4.3 contains
>>> MSIIR and MSIIR1. MSIIR supports 8 MSI registers and MSIIR1 supports
>>> 16 MSI registers, but uses different IBS and SRS shift. When using
>>> MSIR1, the interrupt number is not consecutive. It is hard to use
>>> 'msi-available-ranges' to describe the ranges of the available
>>> interrupt and the ranges are related to the application, rather than
>>> the description of the hardware. this patch also removes
>>> 'msi-available-ranges' property.
>>>=20
>>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
>>> ---
>>>  .../devicetree/bindings/powerpc/fsl/msi-pic.txt    | 49 =20
>>> ++++++++++------------
>>>  1 file changed, 22 insertions(+), 27 deletions(-)
>>>=20
>>> diff --git =20
>>> a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt =20
>>> b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>>> index 5693877..e851e93 100644
>>> --- a/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>>> +++ b/Documentation/devicetree/bindings/powerpc/fsl/msi-pic.txt
>>> @@ -1,26 +1,23 @@
>>>  * Freescale MSI interrupt controller
>>>=20
>>>  Required properties:
>>> -- compatible : compatible list, contains 2 entries,
>>> +- compatible : compatible list, may contains one or two entries,
>>>    first is "fsl,CHIP-msi", where CHIP is the processor(mpc8610, =20
>>> mpc8572,
>>> -  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" =20
>>> depending on
>>> -  the parent type.
>>> +  etc.) and the second is "fsl,mpic-msi" or "fsl,ipic-msi" or
>>> +  "fsl,mpic-msi-v4.3" depending on the parent type and version. If =20
>>> mpic
>>> +  version is 4.3, the number of MSI registers is increased to 16, =20
>>> MSIIR1 is
>>> +  provided to access these 16 registers, compatible =20
>>> "fsl,mpic-msi-v4.3"
>>> +  should be used.
>>=20
>> Why "one or two"?  What does it look like in the case where there's =20
>> just one?
>>=20
> [Minghuan] The original doc said 'contains 2 entries', but I notcie =20
> pq3-mpic.dtsi and qoriq-mpic.dtsi have only one entry "fsl,mpic-msi", =20
> do not have "fsl,CHIP-msi".
> for example:
> mpc8610_hpcd.dts: compatible =3D "fsl,mpc8610-msi", "fsl,mpic-msi";
> fsl/qoriq-mpic.dtsi:  compatible =3D "fsl,mpic-msi"
>=20
> Maybe I should say " For some platforms, "fsl,CHIP-msi' is optional."

Well, this is more a matter of some device trees not complying with the =20
binding, rather than an update for MPIC v4.3.

In any case, if the plan is to update the binding to match what we've =20
been doing in the actual trees, at least word it so that it's clear =20
which one of the two is optional.

>> Why are you removing msi-available-ranges?  It's not valid for MPIC =20
>> v4.3, but it's still valid for older MPICs.  It should move to the =20
>> optional section, though.
> [Minghuan] Because I would like to add kernel parameter 'msiregs' =20
> instead of "msi-available-ranges", for all the MPICs, we will have a =20
> uniform way to configure

I've responded elsewhere to this, but I'd also like to add that we =20
don't break compatibility with older device tree bindings just for "a =20
uniform way".

>>>  Example:
>>>      msi@41600 {
>>> -        compatible =3D "fsl,mpc8610-msi", "fsl,mpic-msi";
>>> -        reg =3D <0x41600 0x80>;
>>> -        msi-available-ranges =3D <0 0x100>;
>>> -        interrupts =3D <
>>> -            0xe0 0
>>> -            0xe1 0
>>> -            0xe2 0
>>> -            0xe3 0
>>> -            0xe4 0
>>> -            0xe5 0
>>> -            0xe6 0
>>> -            0xe7 0>;
>>> -        interrupt-parent =3D <&mpic>;
>>> -    };
>>> +    compatible =3D "fsl,mpic-msi";
>>> +    reg =3D <0x41600 0x200 0x44140 4>;
>>=20
>> Why 0x200?
>>=20
> [Minghuan] The offsets of the MSIA registers are from 0x41600 to =20
> 0x417ff, and the size is 0x200.
> offset 0x41600-0x4170 are MSIIRA1-7.
> 0x41720 is MSISRA,
> 0x41750 is MSIIR.
> The others are reserved.

There is no MSIIRA on fsl,mpic-msi.

If you want to show an fsl,mpic-msi-v4.3 example, update the compatible =20
and add the extra 8 interrupts.  We should probably show an example of =20
each.

BTW, why are you changing/breaking the whitespace in the example?

-Scott=

Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3

[Scott Wood]

On 06/17/2013 07:28:07 PM, Scott Wood wrote:
> On 06/17/2013 12:07:41 AM, Lian Minghuan-b31939 wrote:
>>>> +    compatible =3D "fsl,mpic-msi";
>>>> +    reg =3D <0x41600 0x200 0x44140 4>;
>>>=20
>>> Why 0x200?
>>>=20
>> [Minghuan] The offsets of the MSIA registers are from 0x41600 to =20
>> 0x417ff, and the size is 0x200.
>> offset 0x41600-0x4170 are MSIIRA1-7.
>> 0x41720 is MSISRA,
>> 0x41750 is MSIIR.
>> The others are reserved.
>=20
> There is no MSIIRA on fsl,mpic-msi.

Sigh, I was thinking of MSIIR1A -- which of course is distinct from =20
both MSIIRA1 and MSIIRA. :-P

So it's just a bug that pq3-mpic.dtsi has a length of 0x80?

-Scott=

Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3

[Lian Minghuan-b31939]

Hi Soctt,

please see my comments inline.

On 06/18/2013 08:15 AM, Scott Wood wrote:
> On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote:
>> Hi Scott,
>>
>> please see my comments inline.
>>
>> On 06/15/2013 06:09 AM, Scott Wood wrote:
>>> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
>>>> diff --git a/arch/powerpc/sysdev/fsl_msi.h 
>>>> b/arch/powerpc/sysdev/fsl_msi.h
>>>> index 8225f86..43a9d99 100644
>>>> --- a/arch/powerpc/sysdev/fsl_msi.h
>>>> +++ b/arch/powerpc/sysdev/fsl_msi.h
>>>> @@ -16,7 +16,7 @@
>>>>  #include <linux/of.h>
>>>>  #include <asm/msi_bitmap.h>
>>>>
>>>> -#define NR_MSI_REG        8
>>>> +#define NR_MSI_REG        16
>>>>  #define IRQS_PER_MSI_REG    32
>>>>  #define NR_MSI_IRQS    (NR_MSI_REG * IRQS_PER_MSI_REG)
>>>
>>> I don't see where you update all_avail in fsl_of_msi_probe.
>>>
>>> We should also be bounds-checking the contents of msi-available-ranges.
>>> Currently it looks like we just silently overrun the bitmap if we 
>>> get bad
>>> input from the device tree.
>>>
>> [Minghuan] all_avail definition: static const u32 all_avail[] = { 0, 
>> NR_MSI_IRQS };
>> When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to 
>> 16*32, and all_avail also is updated.
>
> That's my point.  It shouldn't change for older hardware.
[Minghaun] the older hardware has 8 registers, mipcv4.3 has 16 
registers. If we do not use 16*32 bitmap to indicate 8*32 irqs.(this way 
just only wastes some memory and has no other harm)
we have two choice I think.
1. Use a variable assigned value 8 or 16 based on compatible, then 
dynamically create bitmap
2. Add a new file for mpic v4.3.
What do you think?
>
>> Before calling fsl_msi_setup_hwirq(), the code has checked 
>> 'msi-available-ranges',  only the interrupts lied in 
>> 'msi-available-ranges' will be initialized by call 
>> fsl_msi_setup_hwirq() , and the corresponding bitmap will be freed. I 
>> moved msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the 
>> code would generate different bitmap when using MSIIR or MSIIR1.
>
> And what happens if msi-available-ranges is bad, and refers to 
> non-existent MSIs past the end of the bitmap?
[Minghuan] If msi-available-ranges is bad,  the below code will get error.
virt_msir = irq_of_parse_and_map(dev->dev.of_node, irq_index);
And the related error will be printed out and fsl_msi_setup_hwirq() will 
return error directly. There is no chance to set non-existent MSIs past 
the end of the bitmap.

>
> -Scott

Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3

[Lian Minghuan-b31939]

On 06/18/2013 08:42 AM, Scott Wood wrote:
> On 06/17/2013 07:28:07 PM, Scott Wood wrote:
>> On 06/17/2013 12:07:41 AM, Lian Minghuan-b31939 wrote:
>>>>> +    compatible = "fsl,mpic-msi";
>>>>> +    reg = <0x41600 0x200 0x44140 4>;
>>>>
>>>> Why 0x200?
>>>>
>>> [Minghuan] The offsets of the MSIA registers are from 0x41600 to 
>>> 0x417ff, and the size is 0x200.
>>> offset 0x41600-0x4170 are MSIIRA1-7.
>>> 0x41720 is MSISRA,
>>> 0x41750 is MSIIR.
>>> The others are reserved.
>>
>> There is no MSIIRA on fsl,mpic-msi.
>
> Sigh, I was thinking of MSIIR1A -- which of course is distinct from 
> both MSIIRA1 and MSIIRA. :-P
>
> So it's just a bug that pq3-mpic.dtsi has a length of 0x80?
[Minghuan] I am sorry, there is a typo.
offset 0x41600-0x4170 should be MSIRA0-7.
The MSI bank size is 0x200.
The MSIR 0-7 size is 0x80.
So the first region of 'reg' should indicate bank size or MSIR size?
I think it should be a bank size. So MSI driver can access MSISR and 
MSIIR, and provide some new features in feature.
>
> -Scott

Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter

[Lian Minghuan-b31939]

Hi Scott,

please see my comments inline.

On 06/18/2013 08:18 AM, Scott Wood wrote:
> On 06/17/2013 12:36:50 AM, Lian Minghuan-b31939 wrote:
>> Hi Scott,
>>
>> please see my comments inline.
>>
>> On 06/15/2013 06:13 AM, Scott Wood wrote:
>>> On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
>>>> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
>>>> the IRQs of a register are not continuous. for example, the first
>>>> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
>>>> is hard to use 'msi-available-ranges' property to indicate the
>>>> available ranges and 'msi-available-ranges' property has been
>>>> removed from dts node, so this patch removes the related code.
>>>>
>>>> 2. Add 'msiregs' kernel parameter instead of 'msi-available-ranges'
>>>> functionality.
>>>
>>> The reason we used a device tree property was because this is for 
>>> virtualization and AMP scenarios where this instance of Linux does 
>>> not own all of the MSI registers.
>>>
>>> I don't see any reasonable way to partition an MPIC v4.3 MSI group 
>>> -- but there are more groups, so it's not that bad. What's the use 
>>> case for this patch?
>>>
>> [Minghuan] I do not known any case about this patch. I add 'msiregs' 
>> just for achieving "msi-available-ranges" functionality. I do not 
>> want to remove partition functionality when updating to mpic4.3, 
>> although I do not see virtualization and AMP cases on T4(KVM does not 
>> need this functionality).
>
> Such functionality does not work on mpic v4.3.  There are conflicting 
> requirements of contiguous MSIs (because PCI devices can use them that 
> way) and the inability to partition a single register (because they 
> all go to the same MPIC interrupt).
>
> Keep msi-available-ranges as is for older hardware, and just ignore it 
> (with a warning printed) if it's present on MPIC v4.3.
>
[Minghuan] Thanks for your guidance.
But 'msireg' should be remained or removed?  if remaining 'msiregs', it 
should be for all mpic or only for mpic v4.3?

> -Scott

Re: [PATCH 3/5] powerpc/dts: update MSI bindings doc for MPIC v4.3

[Scott Wood]

On 06/17/2013 09:49:19 PM, Lian Minghuan-b31939 wrote:
> On 06/18/2013 08:42 AM, Scott Wood wrote:
>> On 06/17/2013 07:28:07 PM, Scott Wood wrote:
>>> On 06/17/2013 12:07:41 AM, Lian Minghuan-b31939 wrote:
>>>>>> +    compatible =3D "fsl,mpic-msi";
>>>>>> +    reg =3D <0x41600 0x200 0x44140 4>;
>>>>>=20
>>>>> Why 0x200?
>>>>>=20
>>>> [Minghuan] The offsets of the MSIA registers are from 0x41600 to =20
>>>> 0x417ff, and the size is 0x200.
>>>> offset 0x41600-0x4170 are MSIIRA1-7.
>>>> 0x41720 is MSISRA,
>>>> 0x41750 is MSIIR.
>>>> The others are reserved.
>>>=20
>>> There is no MSIIRA on fsl,mpic-msi.
>>=20
>> Sigh, I was thinking of MSIIR1A -- which of course is distinct from =20
>> both MSIIRA1 and MSIIRA. :-P
>>=20
>> So it's just a bug that pq3-mpic.dtsi has a length of 0x80?
> [Minghuan] I am sorry, there is a typo.
> offset 0x41600-0x4170 should be MSIRA0-7.
> The MSI bank size is 0x200.
> The MSIR 0-7 size is 0x80.
> So the first region of 'reg' should indicate bank size or MSIR size?
> I think it should be a bank size. So MSI driver can access MSISR and =20
> MSIIR, and provide some new features in feature.

It should be the bank size.  There's already inconsistency between =20
pq3-mpic.dtsi and qoriq-mpic.dtsi (the latter has 0x200).  I think =20
pq3-mpic.dtsi is just wrong and should be fixed.

-Scott=

Re: [PATCH 5/5] powerpc/fsl_msi: add 'msiregs' kernel parameter

[Scott Wood]

On 06/17/2013 10:10:17 PM, Lian Minghuan-b31939 wrote:
> Hi Scott,
>=20
> please see my comments inline.
>=20
> On 06/18/2013 08:18 AM, Scott Wood wrote:
>> On 06/17/2013 12:36:50 AM, Lian Minghuan-b31939 wrote:
>>> Hi Scott,
>>>=20
>>> please see my comments inline.
>>>=20
>>> On 06/15/2013 06:13 AM, Scott Wood wrote:
>>>> On 06/14/2013 02:15:59 AM, Minghuan Lian wrote:
>>>>> 1. Only MSIIR1 can index 16 MSI registers, but when using MSIIR1
>>>>> the IRQs of a register are not continuous. for example, the first
>>>>> register irq values are 0x0, 0x10, 0x20, 0x30 ... 0x1f0. So it
>>>>> is hard to use 'msi-available-ranges' property to indicate the
>>>>> available ranges and 'msi-available-ranges' property has been
>>>>> removed from dts node, so this patch removes the related code.
>>>>>=20
>>>>> 2. Add 'msiregs' kernel parameter instead of =20
>>>>> 'msi-available-ranges'
>>>>> functionality.
>>>>=20
>>>> The reason we used a device tree property was because this is for =20
>>>> virtualization and AMP scenarios where this instance of Linux does =20
>>>> not own all of the MSI registers.
>>>>=20
>>>> I don't see any reasonable way to partition an MPIC v4.3 MSI group =20
>>>> -- but there are more groups, so it's not that bad. What's the use =20
>>>> case for this patch?
>>>>=20
>>> [Minghuan] I do not known any case about this patch. I add =20
>>> 'msiregs' just for achieving "msi-available-ranges" functionality. =20
>>> I do not want to remove partition functionality when updating to =20
>>> mpic4.3, although I do not see virtualization and AMP cases on =20
>>> T4(KVM does not need this functionality).
>>=20
>> Such functionality does not work on mpic v4.3.  There are =20
>> conflicting requirements of contiguous MSIs (because PCI devices can =20
>> use them that way) and the inability to partition a single register =20
>> (because they all go to the same MPIC interrupt).
>>=20
>> Keep msi-available-ranges as is for older hardware, and just ignore =20
>> it (with a warning printed) if it's present on MPIC v4.3.
>>=20
> [Minghuan] Thanks for your guidance.
> But 'msireg' should be remained or removed?

Removed.

-Scott=

Re: [PATCH 2/5] powerpc/fsl_msi: add MSIIR1 support for MPIC v4.3

[Scott Wood]

On 06/17/2013 09:34:49 PM, Lian Minghuan-b31939 wrote:
> Hi Soctt,
>=20
> please see my comments inline.
>=20
> On 06/18/2013 08:15 AM, Scott Wood wrote:
>> On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote:
>>> Hi Scott,
>>>=20
>>> please see my comments inline.
>>>=20
>>> On 06/15/2013 06:09 AM, Scott Wood wrote:
>>>> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote:
>>>>> diff --git a/arch/powerpc/sysdev/fsl_msi.h =20
>>>>> b/arch/powerpc/sysdev/fsl_msi.h
>>>>> index 8225f86..43a9d99 100644
>>>>> --- a/arch/powerpc/sysdev/fsl_msi.h
>>>>> +++ b/arch/powerpc/sysdev/fsl_msi.h
>>>>> @@ -16,7 +16,7 @@
>>>>>  #include <linux/of.h>
>>>>>  #include <asm/msi_bitmap.h>
>>>>>=20
>>>>> -#define NR_MSI_REG        8
>>>>> +#define NR_MSI_REG        16
>>>>>  #define IRQS_PER_MSI_REG    32
>>>>>  #define NR_MSI_IRQS    (NR_MSI_REG * IRQS_PER_MSI_REG)
>>>>=20
>>>> I don't see where you update all_avail in fsl_of_msi_probe.
>>>>=20
>>>> We should also be bounds-checking the contents of =20
>>>> msi-available-ranges.
>>>> Currently it looks like we just silently overrun the bitmap if we =20
>>>> get bad
>>>> input from the device tree.
>>>>=20
>>> [Minghuan] all_avail definition: static const u32 all_avail[] =3D { =20
>>> 0, NR_MSI_IRQS };
>>> When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to =20
>>> 16*32, and all_avail also is updated.
>>=20
>> That's my point.  It shouldn't change for older hardware.
> [Minghaun] the older hardware has 8 registers, mipcv4.3 has 16 =20
> registers. If we do not use 16*32 bitmap to indicate 8*32 irqs.(this =20
> way just only wastes some memory and has no other harm)

Using the larger bitmap unconditionally is fine.  What is not fine is, =20
on older hardware, acting as if all 16 irqs are present.

In other words, I'm talking about the contents of the bitmap, not its =20
size.

> we have two choice I think.
> 1. Use a variable assigned value 8 or 16 based on compatible, then =20
> dynamically create bitmap

If we have the mpic4.3 compatible, then we don't even support =20
msi-available-ranges, so we'd skip this code and free everything in the =20
bitmap.

> 2. Add a new file for mpic v4.3.

No. :-)

-Scott=