[PATCH v2,1/9] irqchip/ls-scfg-msi: fix typo of MSI compatible strings

[PATCH v2,1/9] irqchip/ls-scfg-msi: fix typo of MSI compatible strings

[Minghuan Lian]

The patch is to fix typo of the Layerscape SCFG MSI dts compatible
strings. "1" is replaced by "l".

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
---
v2-v1:
- None

 .../devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt    | 6 +++---
 drivers/irqchip/irq-ls-scfg-msi.c                                   | 6 ++++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
index 9e38949..2755cd1 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
@@ -4,8 +4,8 @@ Required properties:
 
 - compatible: should be "fsl,<soc-name>-msi" to identify
 	      Layerscape PCIe MSI controller block such as:
-              "fsl,1s1021a-msi"
-              "fsl,1s1043a-msi"
+              "fsl,ls1021a-msi"
+              "fsl,ls1043a-msi"
 - msi-controller: indicates that this is a PCIe MSI controller node
 - reg: physical base address of the controller and length of memory mapped.
 - interrupts: an interrupt to the parent interrupt controller.
@@ -23,7 +23,7 @@ MSI controller node
 Examples:
 
 	msi1: msi-controller@1571000 {
-		compatible = "fsl,1s1043a-msi";
+		compatible = "fsl,ls1043a-msi";
 		reg = <0x0 0x1571000 0x0 0x8>,
 		msi-controller;
 		interrupts = <0 116 0x4>;
diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
index 02cca74c..cef67cc 100644
--- a/drivers/irqchip/irq-ls-scfg-msi.c
+++ b/drivers/irqchip/irq-ls-scfg-msi.c
@@ -219,8 +219,10 @@ static int ls_scfg_msi_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id ls_scfg_msi_id[] = {
-	{ .compatible = "fsl,1s1021a-msi", },
-	{ .compatible = "fsl,1s1043a-msi", },
+	{ .compatible = "fsl,1s1021a-msi", }, /* a typo */
+	{ .compatible = "fsl,1s1043a-msi", }, /* a typo */
+	{ .compatible = "fsl,ls1021a-msi", },
+	{ .compatible = "fsl,ls1043a-msi", },
 	{},
 };
 
-- 
1.9.1

[PATCH v2,2/9] arm: dts: ls1021a: fix typo of MSI compatible string

[Minghuan Lian]

"1" should be replaced by "l". This is a typo.
The patch is to fix it.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
v2-v1:
- None

 arch/arm/boot/dts/ls1021a.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index 282d854..6651938 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -122,14 +122,14 @@
 		};
 
 		msi1: msi-controller@1570e00 {
-			compatible = "fsl,1s1021a-msi";
+			compatible = "fsl,ls1021a-msi";
 			reg = <0x0 0x1570e00 0x0 0x8>;
 			msi-controller;
 			interrupts =  <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
 		msi2: msi-controller@1570e08 {
-			compatible = "fsl,1s1021a-msi";
+			compatible = "fsl,ls1021a-msi";
 			reg = <0x0 0x1570e08 0x0 0x8>;
 			msi-controller;
 			interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
-- 
1.9.1

[PATCH v2,3/9] arm64: dts: ls1043a: fix typo of MSI compatible string

[Minghuan Lian]

"1" should be replaced by "l". This is a typo.
The patch is to fix it.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
v2-v1:
- None

 arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
index ec13a6e..692fc35 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
@@ -589,21 +589,21 @@
 		};
 
 		msi1: msi-controller1@1571000 {
-			compatible = "fsl,1s1043a-msi";
+			compatible = "fsl,ls1043a-msi";
 			reg = <0x0 0x1571000 0x0 0x8>;
 			msi-controller;
 			interrupts = <0 116 0x4>;
 		};
 
 		msi2: msi-controller2@1572000 {
-			compatible = "fsl,1s1043a-msi";
+			compatible = "fsl,ls1043a-msi";
 			reg = <0x0 0x1572000 0x0 0x8>;
 			msi-controller;
 			interrupts = <0 126 0x4>;
 		};
 
 		msi3: msi-controller3@1573000 {
-			compatible = "fsl,1s1043a-msi";
+			compatible = "fsl,ls1043a-msi";
 			reg = <0x0 0x1573000 0x0 0x8>;
 			msi-controller;
 			interrupts = <0 160 0x4>;
-- 
1.9.1

[PATCH v2,4/9] arm: dts: ls1021a: share all MSIs

[Minghuan Lian]

In order to maximize the use of MSI, a PCIe controller will share
all MSI controllers. The patch changes msi-parent to refer to all
MSI controller dts nodes.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
v2-v1:
- None

 arch/arm/boot/dts/ls1021a.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index 6651938..1c82024 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -723,7 +723,7 @@
 			bus-range = <0x0 0xff>;
 			ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000   /* downstream I/O */
 				  0x82000000 0x0 0x40000000 0x40 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
-			msi-parent = <&msi1>;
+			msi-parent = <&msi1>, <&msi2>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic GIC_SPI 91  IRQ_TYPE_LEVEL_HIGH>,
@@ -746,7 +746,7 @@
 			bus-range = <0x0 0xff>;
 			ranges = <0x81000000 0x0 0x00000000 0x48 0x00010000 0x0 0x00010000   /* downstream I/O */
 				  0x82000000 0x0 0x40000000 0x48 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
-			msi-parent = <&msi2>;
+			msi-parent = <&msi1>, <&msi2>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic GIC_SPI 92  IRQ_TYPE_LEVEL_HIGH>,
-- 
1.9.1

[PATCH v2,5/9] arm64: dts: ls1043a: share all MSIs

[Minghuan Lian]

In order to maximize the use of MSI, a PCIe controller will share
all MSI controllers. The patch changes "msi-parent" to refer to all
MSI controller dts nodes.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
v2-v1:
- None

 arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
index 692fc35..3947220 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
@@ -625,7 +625,7 @@
 			bus-range = <0x0 0xff>;
 			ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000   /* downstream I/O */
 				  0x82000000 0x0 0x40000000 0x40 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
-			msi-parent = <&msi1>;
+			msi-parent = <&msi1>, <&msi2>, <&msi3>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic 0 110 0x4>,
@@ -650,7 +650,7 @@
 			bus-range = <0x0 0xff>;
 			ranges = <0x81000000 0x0 0x00000000 0x48 0x00010000 0x0 0x00010000   /* downstream I/O */
 				  0x82000000 0x0 0x40000000 0x48 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
-			msi-parent = <&msi2>;
+			msi-parent = <&msi1>, <&msi2>, <&msi3>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic 0 120  0x4>,
@@ -675,7 +675,7 @@
 			bus-range = <0x0 0xff>;
 			ranges = <0x81000000 0x0 0x00000000 0x50 0x00010000 0x0 0x00010000   /* downstream I/O */
 				  0x82000000 0x0 0x40000000 0x50 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
-			msi-parent = <&msi3>;
+			msi-parent = <&msi1>, <&msi2>, <&msi3>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic 0 154 0x4>,
-- 
1.9.1

[PATCH v2,6/9] arm64: dts: ls1046a: add MSI dts node

[Minghuan Lian]

LS1046a includes 3 MSI controllers.
Each controller supports 128 interrupts.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
Acked-by: Rob Herring <robh@kernel.org> 
---
v2-v1:
- change whitespace number

 .../interrupt-controller/fsl,ls-scfg-msi.txt       |  1 +
 arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi     | 31 ++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
index 2755cd1..dde4552 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
@@ -6,6 +6,7 @@ Required properties:
 	      Layerscape PCIe MSI controller block such as:
               "fsl,ls1021a-msi"
               "fsl,ls1043a-msi"
+              "fsl,ls1046a-msi"
 - msi-controller: indicates that this is a PCIe MSI controller node
 - reg: physical base address of the controller and length of memory mapped.
 - interrupts: an interrupt to the parent interrupt controller.
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
index 38806ca..49dbafc 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
@@ -511,5 +511,36 @@
 			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&clockgen 4 1>;
 		};
+
+		msi1: msi-controller@1580000 {
+			compatible = "fsl,ls1046a-msi";
+			msi-controller;
+			reg = <0x0 0x1580000 0x0 0x10000>;
+			interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		msi2: msi-controller@1590000 {
+			compatible = "fsl,ls1046a-msi";
+			msi-controller;
+			reg = <0x0 0x1590000 0x0 0x10000>;
+			interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		msi3: msi-controller@15a0000 {
+			compatible = "fsl,ls1046a-msi";
+			msi-controller;
+			reg = <0x0 0x15a0000 0x0 0x10000>;
+			interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
 	};
 };
-- 
1.9.1

[PATCH v2,7/9] irqchip/ls-scfg-msi: add LS1046a MSI support

[Minghuan Lian]

LS1046a includes 4 MSIRs, each MSIR is assigned a dedicate GIC
SPI interrupt and provides 32 MSI interrupts. Compared to previous
MSI, LS1046a's IBS(interrupt bit select) shift is changed to 2 and
total MSI interrupt number is changed to 128.

The patch adds structure 'ls_scfg_msir' to describe MSIR setting and
'ibs_shift' to store the different value between the SoCs.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
v2-v1:
- MSI dts node change has been merged into the patch 6/9

 drivers/irqchip/irq-ls-scfg-msi.c | 161 +++++++++++++++++++++++++++++---------
 1 file changed, 126 insertions(+), 35 deletions(-)

diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
index cef67cc..67547bd 100644
--- a/drivers/irqchip/irq-ls-scfg-msi.c
+++ b/drivers/irqchip/irq-ls-scfg-msi.c
@@ -17,13 +17,24 @@
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
+#include <linux/of_irq.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
 #include <linux/spinlock.h>
 
-#define MSI_MAX_IRQS	32
-#define MSI_IBS_SHIFT	3
-#define MSIR		4
+#define MSI_IRQS_PER_MSIR	32
+#define MSI_MSIR_OFFSET		4
+
+struct ls_scfg_msi_cfg {
+	u32 ibs_shift; /* Shift of interrupt bit select */
+};
+
+struct ls_scfg_msir {
+	struct ls_scfg_msi *msi_data;
+	unsigned int index;
+	unsigned int gic_irq;
+	void __iomem *reg;
+};
 
 struct ls_scfg_msi {
 	spinlock_t		lock;
@@ -32,8 +43,11 @@ struct ls_scfg_msi {
 	struct irq_domain	*msi_domain;
 	void __iomem		*regs;
 	phys_addr_t		msiir_addr;
-	int			irq;
-	DECLARE_BITMAP(used, MSI_MAX_IRQS);
+	struct ls_scfg_msi_cfg	*cfg;
+	u32			msir_num;
+	struct ls_scfg_msir	*msir;
+	u32			irqs_num;
+	unsigned long		*used;
 };
 
 static struct irq_chip ls_scfg_msi_irq_chip = {
@@ -55,7 +69,7 @@ static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
 
 	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
 	msg->address_lo = lower_32_bits(msi_data->msiir_addr);
-	msg->data = data->hwirq << MSI_IBS_SHIFT;
+	msg->data = data->hwirq;
 }
 
 static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
@@ -81,8 +95,8 @@ static int ls_scfg_msi_domain_irq_alloc(struct irq_domain *domain,
 	WARN_ON(nr_irqs != 1);
 
 	spin_lock(&msi_data->lock);
-	pos = find_first_zero_bit(msi_data->used, MSI_MAX_IRQS);
-	if (pos < MSI_MAX_IRQS)
+	pos = find_first_zero_bit(msi_data->used, msi_data->irqs_num);
+	if (pos < msi_data->irqs_num)
 		__set_bit(pos, msi_data->used);
 	else
 		err = -ENOSPC;
@@ -106,7 +120,7 @@ static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
 	int pos;
 
 	pos = d->hwirq;
-	if (pos < 0 || pos >= MSI_MAX_IRQS) {
+	if (pos < 0 || pos >= msi_data->irqs_num) {
 		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
 		return;
 	}
@@ -123,15 +137,17 @@ static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
 
 static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
 {
-	struct ls_scfg_msi *msi_data = irq_desc_get_handler_data(desc);
+	struct ls_scfg_msir *msir = irq_desc_get_handler_data(desc);
+	struct ls_scfg_msi *msi_data = msir->msi_data;
 	unsigned long val;
-	int pos, virq;
+	int pos, virq, hwirq;
 
 	chained_irq_enter(irq_desc_get_chip(desc), desc);
 
-	val = ioread32be(msi_data->regs + MSIR);
-	for_each_set_bit(pos, &val, MSI_MAX_IRQS) {
-		virq = irq_find_mapping(msi_data->parent, (31 - pos));
+	val = ioread32be(msir->reg);
+	for_each_set_bit(pos, &val, MSI_IRQS_PER_MSIR) {
+		hwirq = ((31 - pos) << msi_data->cfg->ibs_shift) | msir->index;
+		virq = irq_find_mapping(msi_data->parent, hwirq);
 		if (virq)
 			generic_handle_irq(virq);
 	}
@@ -143,7 +159,7 @@ static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
 {
 	/* Initialize MSI domain parent */
 	msi_data->parent = irq_domain_add_linear(NULL,
-						 MSI_MAX_IRQS,
+						 msi_data->irqs_num,
 						 &ls_scfg_msi_domain_ops,
 						 msi_data);
 	if (!msi_data->parent) {
@@ -164,16 +180,83 @@ static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
 	return 0;
 }
 
+static int ls_scfg_msi_setup_hwirq(struct ls_scfg_msi *msi_data, int index)
+{
+	struct ls_scfg_msir *msir;
+	int virq, i, hwirq;
+
+	virq = platform_get_irq(msi_data->pdev, index);
+	if (virq <= 0)
+		return -ENODEV;
+
+	msir = &msi_data->msir[index];
+	msir->index = index;
+	msir->msi_data = msi_data;
+	msir->gic_irq = virq;
+	msir->reg = msi_data->regs + MSI_MSIR_OFFSET + 4 * index;
+
+	irq_set_chained_handler_and_data(msir->gic_irq,
+					 ls_scfg_msi_irq_handler,
+					 msir);
+
+	/* Release the hwirqs corresponding to this MSIR */
+	for (i = 0; i < MSI_IRQS_PER_MSIR; i++) {
+		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
+		bitmap_clear(msi_data->used, hwirq, 1);
+	}
+
+	return 0;
+}
+
+static int ls_scfg_msi_teardown_hwirq(struct ls_scfg_msir *msir)
+{
+	struct ls_scfg_msi *msi_data = msir->msi_data;
+	int i, hwirq;
+
+	if (msir->gic_irq > 0)
+		irq_set_chained_handler_and_data(msir->gic_irq, NULL, NULL);
+
+	for (i = 0; i < MSI_IRQS_PER_MSIR; i++) {
+		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
+		bitmap_set(msi_data->used, hwirq, 1);
+	}
+
+	return 0;
+}
+
+static struct ls_scfg_msi_cfg ls1021_msi_cfg = {
+	.ibs_shift = 3,
+};
+
+static struct ls_scfg_msi_cfg ls1046_msi_cfg = {
+	.ibs_shift = 2,
+};
+
+static const struct of_device_id ls_scfg_msi_id[] = {
+	{ .compatible = "fsl,ls1021a-msi", .data = &ls1021_msi_cfg },
+	{ .compatible = "fsl,ls1043a-msi", .data = &ls1021_msi_cfg },
+	{ .compatible = "fsl,ls1046a-msi", .data = &ls1046_msi_cfg },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ls_scfg_msi_id);
+
 static int ls_scfg_msi_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
 	struct ls_scfg_msi *msi_data;
 	struct resource *res;
-	int ret;
+	int i, ret;
+
+	match = of_match_device(ls_scfg_msi_id, &pdev->dev);
+	if (!match)
+		return -ENODEV;
 
 	msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
 	if (!msi_data)
 		return -ENOMEM;
 
+	msi_data->cfg = (struct ls_scfg_msi_cfg *) match->data;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	msi_data->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(msi_data->regs)) {
@@ -182,23 +265,37 @@ static int ls_scfg_msi_probe(struct platform_device *pdev)
 	}
 	msi_data->msiir_addr = res->start;
 
-	msi_data->irq = platform_get_irq(pdev, 0);
-	if (msi_data->irq <= 0) {
-		dev_err(&pdev->dev, "failed to get MSI irq\n");
-		return -ENODEV;
-	}
-
 	msi_data->pdev = pdev;
 	spin_lock_init(&msi_data->lock);
 
+	msi_data->irqs_num = MSI_IRQS_PER_MSIR *
+			     (1 << msi_data->cfg->ibs_shift);
+	msi_data->used = devm_kcalloc(&pdev->dev,
+				    BITS_TO_LONGS(msi_data->irqs_num),
+				    sizeof(*msi_data->used),
+				    GFP_KERNEL);
+	if (!msi_data->used)
+		return -ENOMEM;
+	/*
+	 * Reserve all the hwirqs
+	 * The available hwirqs will be released in ls1_msi_setup_hwirq()
+	 */
+	bitmap_set(msi_data->used, 0, msi_data->irqs_num);
+
+	msi_data->msir_num = of_irq_count(pdev->dev.of_node);
+	msi_data->msir = devm_kcalloc(&pdev->dev, msi_data->msir_num,
+				      sizeof(*msi_data->msir),
+				      GFP_KERNEL);
+	if (!msi_data->msir)
+		return -ENOMEM;
+
+	for (i = 0; i < msi_data->msir_num; i++)
+		ls_scfg_msi_setup_hwirq(msi_data, i);
+
 	ret = ls_scfg_msi_domains_init(msi_data);
 	if (ret)
 		return ret;
 
-	irq_set_chained_handler_and_data(msi_data->irq,
-					 ls_scfg_msi_irq_handler,
-					 msi_data);
-
 	platform_set_drvdata(pdev, msi_data);
 
 	return 0;
@@ -207,8 +304,10 @@ static int ls_scfg_msi_probe(struct platform_device *pdev)
 static int ls_scfg_msi_remove(struct platform_device *pdev)
 {
 	struct ls_scfg_msi *msi_data = platform_get_drvdata(pdev);
+	int i;
 
-	irq_set_chained_handler_and_data(msi_data->irq, NULL, NULL);
+	for (i = 0; i < msi_data->msir_num; i++)
+		ls_scfg_msi_teardown_hwirq(&msi_data->msir[i]);
 
 	irq_domain_remove(msi_data->msi_domain);
 	irq_domain_remove(msi_data->parent);
@@ -218,14 +317,6 @@ static int ls_scfg_msi_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id ls_scfg_msi_id[] = {
-	{ .compatible = "fsl,1s1021a-msi", }, /* a typo */
-	{ .compatible = "fsl,1s1043a-msi", }, /* a typo */
-	{ .compatible = "fsl,ls1021a-msi", },
-	{ .compatible = "fsl,ls1043a-msi", },
-	{},
-};
-
 static struct platform_driver ls_scfg_msi_driver = {
 	.driver = {
 		.name = "ls-scfg-msi",
-- 
1.9.1

[PATCH v2,8/9] irqchip/ls-scfg-msi: add LS1043a v1.1 MSI support

[Minghuan Lian]

A MSI controller of LS1043a v1.0 only includes one MSIR and
is assigned one GIC interrupt. In order to support affinity,
LS1043a v1.1 MSI is assigned 4 MSIRs and 4 GIC interrupts.
But the MSIR has the different offset and only supports 8 MSIs.
The bits between variable bit_start and bit_end in structure
ls_scfg_msir are used to show 8 MSI interrupts. msir_irqs and
msir_base are added to describe the difference of MSI between
LS1043a v1.1 and other SoCs.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
Acked-by: Rob Herring <robh@kernel.org> 
---
v2-v1:
- None

 .../interrupt-controller/fsl,ls-scfg-msi.txt       |  1 +
 drivers/irqchip/irq-ls-scfg-msi.c                  | 45 +++++++++++++++++++---
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
index dde4552..49ccabb 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
@@ -7,6 +7,7 @@ Required properties:
               "fsl,ls1021a-msi"
               "fsl,ls1043a-msi"
               "fsl,ls1046a-msi"
+              "fsl,ls1043a-v1.1-msi"
 - msi-controller: indicates that this is a PCIe MSI controller node
 - reg: physical base address of the controller and length of memory mapped.
 - interrupts: an interrupt to the parent interrupt controller.
diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
index 67547bd..dc19569 100644
--- a/drivers/irqchip/irq-ls-scfg-msi.c
+++ b/drivers/irqchip/irq-ls-scfg-msi.c
@@ -25,14 +25,21 @@
 #define MSI_IRQS_PER_MSIR	32
 #define MSI_MSIR_OFFSET		4
 
+#define MSI_LS1043V1_1_IRQS_PER_MSIR	8
+#define MSI_LS1043V1_1_MSIR_OFFSET	0x10
+
 struct ls_scfg_msi_cfg {
 	u32 ibs_shift; /* Shift of interrupt bit select */
+	u32 msir_irqs; /* The irq number per MSIR */
+	u32 msir_base; /* The base address of MSIR */
 };
 
 struct ls_scfg_msir {
 	struct ls_scfg_msi *msi_data;
 	unsigned int index;
 	unsigned int gic_irq;
+	unsigned int bit_start;
+	unsigned int bit_end;
 	void __iomem *reg;
 };
 
@@ -140,13 +147,18 @@ static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
 	struct ls_scfg_msir *msir = irq_desc_get_handler_data(desc);
 	struct ls_scfg_msi *msi_data = msir->msi_data;
 	unsigned long val;
-	int pos, virq, hwirq;
+	int pos, size, virq, hwirq;
 
 	chained_irq_enter(irq_desc_get_chip(desc), desc);
 
 	val = ioread32be(msir->reg);
-	for_each_set_bit(pos, &val, MSI_IRQS_PER_MSIR) {
-		hwirq = ((31 - pos) << msi_data->cfg->ibs_shift) | msir->index;
+
+	pos = msir->bit_start;
+	size = msir->bit_end + 1;
+
+	for_each_set_bit_from(pos, &val, size) {
+		hwirq = ((msir->bit_end - pos) << msi_data->cfg->ibs_shift) |
+			msir->index;
 		virq = irq_find_mapping(msi_data->parent, hwirq);
 		if (virq)
 			generic_handle_irq(virq);
@@ -193,14 +205,24 @@ static int ls_scfg_msi_setup_hwirq(struct ls_scfg_msi *msi_data, int index)
 	msir->index = index;
 	msir->msi_data = msi_data;
 	msir->gic_irq = virq;
-	msir->reg = msi_data->regs + MSI_MSIR_OFFSET + 4 * index;
+	msir->reg = msi_data->regs + msi_data->cfg->msir_base + 4 * index;
+
+	if (msi_data->cfg->msir_irqs == MSI_LS1043V1_1_IRQS_PER_MSIR) {
+		msir->bit_start = 32 - ((msir->index + 1) *
+				  MSI_LS1043V1_1_IRQS_PER_MSIR);
+		msir->bit_end = msir->bit_start +
+				MSI_LS1043V1_1_IRQS_PER_MSIR - 1;
+	} else {
+		msir->bit_start = 0;
+		msir->bit_end = msi_data->cfg->msir_irqs - 1;
+	}
 
 	irq_set_chained_handler_and_data(msir->gic_irq,
 					 ls_scfg_msi_irq_handler,
 					 msir);
 
 	/* Release the hwirqs corresponding to this MSIR */
-	for (i = 0; i < MSI_IRQS_PER_MSIR; i++) {
+	for (i = 0; i < msi_data->cfg->msir_irqs; i++) {
 		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
 		bitmap_clear(msi_data->used, hwirq, 1);
 	}
@@ -216,7 +238,7 @@ static int ls_scfg_msi_teardown_hwirq(struct ls_scfg_msir *msir)
 	if (msir->gic_irq > 0)
 		irq_set_chained_handler_and_data(msir->gic_irq, NULL, NULL);
 
-	for (i = 0; i < MSI_IRQS_PER_MSIR; i++) {
+	for (i = 0; i < msi_data->cfg->msir_irqs; i++) {
 		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
 		bitmap_set(msi_data->used, hwirq, 1);
 	}
@@ -226,15 +248,26 @@ static int ls_scfg_msi_teardown_hwirq(struct ls_scfg_msir *msir)
 
 static struct ls_scfg_msi_cfg ls1021_msi_cfg = {
 	.ibs_shift = 3,
+	.msir_irqs = MSI_IRQS_PER_MSIR,
+	.msir_base = MSI_MSIR_OFFSET,
 };
 
 static struct ls_scfg_msi_cfg ls1046_msi_cfg = {
 	.ibs_shift = 2,
+	.msir_irqs = MSI_IRQS_PER_MSIR,
+	.msir_base = MSI_MSIR_OFFSET,
+};
+
+static struct ls_scfg_msi_cfg ls1043_v1_1_msi_cfg = {
+	.ibs_shift = 2,
+	.msir_irqs = MSI_LS1043V1_1_IRQS_PER_MSIR,
+	.msir_base = MSI_LS1043V1_1_MSIR_OFFSET,
 };
 
 static const struct of_device_id ls_scfg_msi_id[] = {
 	{ .compatible = "fsl,ls1021a-msi", .data = &ls1021_msi_cfg },
 	{ .compatible = "fsl,ls1043a-msi", .data = &ls1021_msi_cfg },
+	{ .compatible = "fsl,ls1043a-v1.1-msi", .data = &ls1043_v1_1_msi_cfg },
 	{ .compatible = "fsl,ls1046a-msi", .data = &ls1046_msi_cfg },
 	{},
 };
-- 
1.9.1

[PATCH v2,9/9] irqchip/ls-scfg-msi: add MSI affinity support

[Minghuan Lian]

For LS1046a and LS1043a v1.1, the MSI controller has 4 MSIRs and 4
CPUs. A GIC SPI interrupt of MSIR can be associated with a CPU.
When changing MSI interrupt affinity, this MSI will be moved to the
corresponding MSIR and MSI message data will be changed according to
MSIR. when requesting a MSI, the bits of all 4 MSIR will be reserved.
The parameter 'msi_affinity_flag' is provide to change this mode.
"lsmsi=no-affinity" will disable affinity, all MSI can only be
associated with CPU 0.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
v2-v1:
- None

 drivers/irqchip/irq-ls-scfg-msi.c | 75 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 70 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
index dc19569..753fe39 100644
--- a/drivers/irqchip/irq-ls-scfg-msi.c
+++ b/drivers/irqchip/irq-ls-scfg-msi.c
@@ -40,6 +40,7 @@ struct ls_scfg_msir {
 	unsigned int gic_irq;
 	unsigned int bit_start;
 	unsigned int bit_end;
+	unsigned int srs; /* Shared interrupt register select */
 	void __iomem *reg;
 };
 
@@ -70,6 +71,19 @@ struct ls_scfg_msi {
 	.chip	= &ls_scfg_msi_irq_chip,
 };
 
+static int msi_affinity_flag = 1;
+
+static int __init early_parse_ls_scfg_msi(char *p)
+{
+	if (p && strncmp(p, "no-affinity", 11) == 0)
+		msi_affinity_flag = 0;
+	else
+		msi_affinity_flag = 1;
+
+	return 0;
+}
+early_param("lsmsi", early_parse_ls_scfg_msi);
+
 static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data);
@@ -77,12 +91,43 @@ static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
 	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
 	msg->address_lo = lower_32_bits(msi_data->msiir_addr);
 	msg->data = data->hwirq;
+
+	if (msi_affinity_flag) {
+		u32 msir_index;
+
+		msir_index = cpumask_first(data->common->affinity);
+		if (msir_index >= msi_data->msir_num)
+			msir_index = 0;
+
+		msg->data |= msir_index;
+	}
 }
 
 static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
 				    const struct cpumask *mask, bool force)
 {
-	return -EINVAL;
+	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(irq_data);
+	u32 cpu;
+
+	if (!msi_affinity_flag)
+		return -EINVAL;
+
+	if (!force)
+		cpu = cpumask_any_and(mask, cpu_online_mask);
+	else
+		cpu = cpumask_first(mask);
+
+	if (cpu >= msi_data->msir_num)
+		return -EINVAL;
+
+	if (msi_data->msir[cpu].gic_irq <= 0) {
+		pr_warn("cannot bind the irq to cpu%d\n", cpu);
+		return -EINVAL;
+	}
+
+	cpumask_copy(irq_data->common->affinity, mask);
+
+	return IRQ_SET_MASK_OK;
 }
 
 static struct irq_chip ls_scfg_msi_parent_chip = {
@@ -158,7 +203,7 @@ static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
 
 	for_each_set_bit_from(pos, &val, size) {
 		hwirq = ((msir->bit_end - pos) << msi_data->cfg->ibs_shift) |
-			msir->index;
+			msir->srs;
 		virq = irq_find_mapping(msi_data->parent, hwirq);
 		if (virq)
 			generic_handle_irq(virq);
@@ -221,10 +266,19 @@ static int ls_scfg_msi_setup_hwirq(struct ls_scfg_msi *msi_data, int index)
 					 ls_scfg_msi_irq_handler,
 					 msir);
 
+	if (msi_affinity_flag) {
+		/* Associate MSIR interrupt to the cpu */
+		irq_set_affinity(msir->gic_irq, get_cpu_mask(index));
+		msir->srs = 0; /* This value is determined by the CPU */
+	} else
+		msir->srs = index;
+
 	/* Release the hwirqs corresponding to this MSIR */
-	for (i = 0; i < msi_data->cfg->msir_irqs; i++) {
-		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
-		bitmap_clear(msi_data->used, hwirq, 1);
+	if (!msi_affinity_flag || msir->index == 0) {
+		for (i = 0; i < msi_data->cfg->msir_irqs; i++) {
+			hwirq = i << msi_data->cfg->ibs_shift | msir->index;
+			bitmap_clear(msi_data->used, hwirq, 1);
+		}
 	}
 
 	return 0;
@@ -316,6 +370,17 @@ static int ls_scfg_msi_probe(struct platform_device *pdev)
 	bitmap_set(msi_data->used, 0, msi_data->irqs_num);
 
 	msi_data->msir_num = of_irq_count(pdev->dev.of_node);
+
+	if (msi_affinity_flag) {
+		u32 cpu_num;
+
+		cpu_num = num_possible_cpus();
+		if (msi_data->msir_num >= cpu_num)
+			msi_data->msir_num = cpu_num;
+		else
+			msi_affinity_flag = 0;
+	}
+
 	msi_data->msir = devm_kcalloc(&pdev->dev, msi_data->msir_num,
 				      sizeof(*msi_data->msir),
 				      GFP_KERNEL);
-- 
1.9.1

Re: [PATCH v2,7/9] irqchip/ls-scfg-msi: add LS1046a MSI support

[Marc Zyngier]

On 05/01/17 08:10, Minghuan Lian wrote:
> LS1046a includes 4 MSIRs, each MSIR is assigned a dedicate GIC
> SPI interrupt and provides 32 MSI interrupts. Compared to previous
> MSI, LS1046a's IBS(interrupt bit select) shift is changed to 2 and
> total MSI interrupt number is changed to 128.
> 
> The patch adds structure 'ls_scfg_msir' to describe MSIR setting and
> 'ibs_shift' to store the different value between the SoCs.
> 
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
> v2-v1:
> - MSI dts node change has been merged into the patch 6/9
> 
>  drivers/irqchip/irq-ls-scfg-msi.c | 161 +++++++++++++++++++++++++++++---------
>  1 file changed, 126 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
> index cef67cc..67547bd 100644
> --- a/drivers/irqchip/irq-ls-scfg-msi.c
> +++ b/drivers/irqchip/irq-ls-scfg-msi.c
> @@ -17,13 +17,24 @@
>  #include <linux/irq.h>
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
>  #include <linux/spinlock.h>
>  
> -#define MSI_MAX_IRQS	32
> -#define MSI_IBS_SHIFT	3
> -#define MSIR		4
> +#define MSI_IRQS_PER_MSIR	32
> +#define MSI_MSIR_OFFSET		4
> +
> +struct ls_scfg_msi_cfg {
> +	u32 ibs_shift; /* Shift of interrupt bit select */
> +};
> +
> +struct ls_scfg_msir {
> +	struct ls_scfg_msi *msi_data;
> +	unsigned int index;
> +	unsigned int gic_irq;
> +	void __iomem *reg;
> +};
>  
>  struct ls_scfg_msi {
>  	spinlock_t		lock;
> @@ -32,8 +43,11 @@ struct ls_scfg_msi {
>  	struct irq_domain	*msi_domain;
>  	void __iomem		*regs;
>  	phys_addr_t		msiir_addr;
> -	int			irq;
> -	DECLARE_BITMAP(used, MSI_MAX_IRQS);
> +	struct ls_scfg_msi_cfg	*cfg;
> +	u32			msir_num;
> +	struct ls_scfg_msir	*msir;
> +	u32			irqs_num;
> +	unsigned long		*used;
>  };
>  
>  static struct irq_chip ls_scfg_msi_irq_chip = {
> @@ -55,7 +69,7 @@ static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
>  
>  	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
>  	msg->address_lo = lower_32_bits(msi_data->msiir_addr);
> -	msg->data = data->hwirq << MSI_IBS_SHIFT;
> +	msg->data = data->hwirq;
>  }
>  
>  static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
> @@ -81,8 +95,8 @@ static int ls_scfg_msi_domain_irq_alloc(struct irq_domain *domain,
>  	WARN_ON(nr_irqs != 1);
>  
>  	spin_lock(&msi_data->lock);
> -	pos = find_first_zero_bit(msi_data->used, MSI_MAX_IRQS);
> -	if (pos < MSI_MAX_IRQS)
> +	pos = find_first_zero_bit(msi_data->used, msi_data->irqs_num);
> +	if (pos < msi_data->irqs_num)
>  		__set_bit(pos, msi_data->used);
>  	else
>  		err = -ENOSPC;
> @@ -106,7 +120,7 @@ static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
>  	int pos;
>  
>  	pos = d->hwirq;
> -	if (pos < 0 || pos >= MSI_MAX_IRQS) {
> +	if (pos < 0 || pos >= msi_data->irqs_num) {
>  		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
>  		return;
>  	}
> @@ -123,15 +137,17 @@ static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
>  
>  static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
>  {
> -	struct ls_scfg_msi *msi_data = irq_desc_get_handler_data(desc);
> +	struct ls_scfg_msir *msir = irq_desc_get_handler_data(desc);
> +	struct ls_scfg_msi *msi_data = msir->msi_data;
>  	unsigned long val;
> -	int pos, virq;
> +	int pos, virq, hwirq;
>  
>  	chained_irq_enter(irq_desc_get_chip(desc), desc);
>  
> -	val = ioread32be(msi_data->regs + MSIR);
> -	for_each_set_bit(pos, &val, MSI_MAX_IRQS) {
> -		virq = irq_find_mapping(msi_data->parent, (31 - pos));
> +	val = ioread32be(msir->reg);
> +	for_each_set_bit(pos, &val, MSI_IRQS_PER_MSIR) {
> +		hwirq = ((31 - pos) << msi_data->cfg->ibs_shift) | msir->index;
> +		virq = irq_find_mapping(msi_data->parent, hwirq);
>  		if (virq)
>  			generic_handle_irq(virq);
>  	}
> @@ -143,7 +159,7 @@ static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
>  {
>  	/* Initialize MSI domain parent */
>  	msi_data->parent = irq_domain_add_linear(NULL,
> -						 MSI_MAX_IRQS,
> +						 msi_data->irqs_num,
>  						 &ls_scfg_msi_domain_ops,
>  						 msi_data);
>  	if (!msi_data->parent) {
> @@ -164,16 +180,83 @@ static int ls_scfg_msi_domains_init(struct ls_scfg_msi *msi_data)
>  	return 0;
>  }
>  
> +static int ls_scfg_msi_setup_hwirq(struct ls_scfg_msi *msi_data, int index)
> +{
> +	struct ls_scfg_msir *msir;
> +	int virq, i, hwirq;
> +
> +	virq = platform_get_irq(msi_data->pdev, index);
> +	if (virq <= 0)
> +		return -ENODEV;
> +
> +	msir = &msi_data->msir[index];
> +	msir->index = index;
> +	msir->msi_data = msi_data;
> +	msir->gic_irq = virq;
> +	msir->reg = msi_data->regs + MSI_MSIR_OFFSET + 4 * index;
> +
> +	irq_set_chained_handler_and_data(msir->gic_irq,
> +					 ls_scfg_msi_irq_handler,
> +					 msir);
> +
> +	/* Release the hwirqs corresponding to this MSIR */
> +	for (i = 0; i < MSI_IRQS_PER_MSIR; i++) {
> +		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
> +		bitmap_clear(msi_data->used, hwirq, 1);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ls_scfg_msi_teardown_hwirq(struct ls_scfg_msir *msir)
> +{
> +	struct ls_scfg_msi *msi_data = msir->msi_data;
> +	int i, hwirq;
> +
> +	if (msir->gic_irq > 0)
> +		irq_set_chained_handler_and_data(msir->gic_irq, NULL, NULL);
> +
> +	for (i = 0; i < MSI_IRQS_PER_MSIR; i++) {
> +		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
> +		bitmap_set(msi_data->used, hwirq, 1);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct ls_scfg_msi_cfg ls1021_msi_cfg = {
> +	.ibs_shift = 3,
> +};
> +
> +static struct ls_scfg_msi_cfg ls1046_msi_cfg = {
> +	.ibs_shift = 2,
> +};
> +
> +static const struct of_device_id ls_scfg_msi_id[] = {
> +	{ .compatible = "fsl,ls1021a-msi", .data = &ls1021_msi_cfg },
> +	{ .compatible = "fsl,ls1043a-msi", .data = &ls1021_msi_cfg },
> +	{ .compatible = "fsl,ls1046a-msi", .data = &ls1046_msi_cfg },

So after 3 patches adding compatibility between the fsl,1s10 and
fsl,ls10 names, you discard them? How does it work again with the old DTs?

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ls_scfg_msi_id);
> +
>  static int ls_scfg_msi_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *match;
>  	struct ls_scfg_msi *msi_data;
>  	struct resource *res;
> -	int ret;
> +	int i, ret;
> +
> +	match = of_match_device(ls_scfg_msi_id, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
>  
>  	msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL);
>  	if (!msi_data)
>  		return -ENOMEM;
>  
> +	msi_data->cfg = (struct ls_scfg_msi_cfg *) match->data;
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	msi_data->regs = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(msi_data->regs)) {
> @@ -182,23 +265,37 @@ static int ls_scfg_msi_probe(struct platform_device *pdev)
>  	}
>  	msi_data->msiir_addr = res->start;
>  
> -	msi_data->irq = platform_get_irq(pdev, 0);
> -	if (msi_data->irq <= 0) {
> -		dev_err(&pdev->dev, "failed to get MSI irq\n");
> -		return -ENODEV;
> -	}
> -
>  	msi_data->pdev = pdev;
>  	spin_lock_init(&msi_data->lock);
>  
> +	msi_data->irqs_num = MSI_IRQS_PER_MSIR *
> +			     (1 << msi_data->cfg->ibs_shift);
> +	msi_data->used = devm_kcalloc(&pdev->dev,
> +				    BITS_TO_LONGS(msi_data->irqs_num),
> +				    sizeof(*msi_data->used),
> +				    GFP_KERNEL);
> +	if (!msi_data->used)
> +		return -ENOMEM;
> +	/*
> +	 * Reserve all the hwirqs
> +	 * The available hwirqs will be released in ls1_msi_setup_hwirq()
> +	 */
> +	bitmap_set(msi_data->used, 0, msi_data->irqs_num);
> +
> +	msi_data->msir_num = of_irq_count(pdev->dev.of_node);
> +	msi_data->msir = devm_kcalloc(&pdev->dev, msi_data->msir_num,
> +				      sizeof(*msi_data->msir),
> +				      GFP_KERNEL);
> +	if (!msi_data->msir)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < msi_data->msir_num; i++)
> +		ls_scfg_msi_setup_hwirq(msi_data, i);
> +
>  	ret = ls_scfg_msi_domains_init(msi_data);
>  	if (ret)
>  		return ret;
>  
> -	irq_set_chained_handler_and_data(msi_data->irq,
> -					 ls_scfg_msi_irq_handler,
> -					 msi_data);
> -
>  	platform_set_drvdata(pdev, msi_data);
>  
>  	return 0;
> @@ -207,8 +304,10 @@ static int ls_scfg_msi_probe(struct platform_device *pdev)
>  static int ls_scfg_msi_remove(struct platform_device *pdev)
>  {
>  	struct ls_scfg_msi *msi_data = platform_get_drvdata(pdev);
> +	int i;
>  
> -	irq_set_chained_handler_and_data(msi_data->irq, NULL, NULL);
> +	for (i = 0; i < msi_data->msir_num; i++)
> +		ls_scfg_msi_teardown_hwirq(&msi_data->msir[i]);
>  
>  	irq_domain_remove(msi_data->msi_domain);
>  	irq_domain_remove(msi_data->parent);
> @@ -218,14 +317,6 @@ static int ls_scfg_msi_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id ls_scfg_msi_id[] = {
> -	{ .compatible = "fsl,1s1021a-msi", }, /* a typo */
> -	{ .compatible = "fsl,1s1043a-msi", }, /* a typo */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
these entries definitely need to be restored.

Also, please make sure you have a cover letter at the beginning of the
series. I flag series to review by flagging the cover letter, and I
suspect many of us are doing something similar. Not doing so is likely
to leave your series unreviewed...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2,9/9] irqchip/ls-scfg-msi: add MSI affinity support

[Marc Zyngier]

On 05/01/17 08:10, Minghuan Lian wrote:
> For LS1046a and LS1043a v1.1, the MSI controller has 4 MSIRs and 4
> CPUs. A GIC SPI interrupt of MSIR can be associated with a CPU.
> When changing MSI interrupt affinity, this MSI will be moved to the
> corresponding MSIR and MSI message data will be changed according to
> MSIR. when requesting a MSI, the bits of all 4 MSIR will be reserved.
> The parameter 'msi_affinity_flag' is provide to change this mode.
> "lsmsi=no-affinity" will disable affinity, all MSI can only be
> associated with CPU 0.
> 
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
> v2-v1:
> - None
> 
>  drivers/irqchip/irq-ls-scfg-msi.c | 75 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
> index dc19569..753fe39 100644
> --- a/drivers/irqchip/irq-ls-scfg-msi.c
> +++ b/drivers/irqchip/irq-ls-scfg-msi.c
> @@ -40,6 +40,7 @@ struct ls_scfg_msir {
>  	unsigned int gic_irq;
>  	unsigned int bit_start;
>  	unsigned int bit_end;
> +	unsigned int srs; /* Shared interrupt register select */
>  	void __iomem *reg;
>  };
>  
> @@ -70,6 +71,19 @@ struct ls_scfg_msi {
>  	.chip	= &ls_scfg_msi_irq_chip,
>  };
>  
> +static int msi_affinity_flag = 1;
> +
> +static int __init early_parse_ls_scfg_msi(char *p)
> +{
> +	if (p && strncmp(p, "no-affinity", 11) == 0)
> +		msi_affinity_flag = 0;
> +	else
> +		msi_affinity_flag = 1;
> +
> +	return 0;
> +}
> +early_param("lsmsi", early_parse_ls_scfg_msi);

What is the point of this option? If feels like an unnecessary complexity.

> +
>  static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
>  {
>  	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data);
> @@ -77,12 +91,43 @@ static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
>  	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
>  	msg->address_lo = lower_32_bits(msi_data->msiir_addr);
>  	msg->data = data->hwirq;
> +
> +	if (msi_affinity_flag) {
> +		u32 msir_index;
> +
> +		msir_index = cpumask_first(data->common->affinity);
> +		if (msir_index >= msi_data->msir_num)
> +			msir_index = 0;

How can this happen?

> +
> +		msg->data |= msir_index;

How do you guarantee that the low bits were clear? Please document the
way you encode your MSI payload.

> +	}
>  }
>  
>  static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
>  				    const struct cpumask *mask, bool force)
>  {
> -	return -EINVAL;
> +	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(irq_data);
> +	u32 cpu;
> +
> +	if (!msi_affinity_flag)
> +		return -EINVAL;
> +
> +	if (!force)
> +		cpu = cpumask_any_and(mask, cpu_online_mask);
> +	else
> +		cpu = cpumask_first(mask);
> +
> +	if (cpu >= msi_data->msir_num)
> +		return -EINVAL;
> +
> +	if (msi_data->msir[cpu].gic_irq <= 0) {
> +		pr_warn("cannot bind the irq to cpu%d\n", cpu);

Please don't. Returning an error is enough. If you really want to have
something, turn it into a proper debug message.

> +		return -EINVAL;
> +	}
> +
> +	cpumask_copy(irq_data->common->affinity, mask);
> +
> +	return IRQ_SET_MASK_OK;
>  }
>  
>  static struct irq_chip ls_scfg_msi_parent_chip = {
> @@ -158,7 +203,7 @@ static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
>  
>  	for_each_set_bit_from(pos, &val, size) {
>  		hwirq = ((msir->bit_end - pos) << msi_data->cfg->ibs_shift) |
> -			msir->index;
> +			msir->srs;
>  		virq = irq_find_mapping(msi_data->parent, hwirq);
>  		if (virq)
>  			generic_handle_irq(virq);
> @@ -221,10 +266,19 @@ static int ls_scfg_msi_setup_hwirq(struct ls_scfg_msi *msi_data, int index)
>  					 ls_scfg_msi_irq_handler,
>  					 msir);
>  
> +	if (msi_affinity_flag) {
> +		/* Associate MSIR interrupt to the cpu */
> +		irq_set_affinity(msir->gic_irq, get_cpu_mask(index));
> +		msir->srs = 0; /* This value is determined by the CPU */
> +	} else
> +		msir->srs = index;
> +
>  	/* Release the hwirqs corresponding to this MSIR */
> -	for (i = 0; i < msi_data->cfg->msir_irqs; i++) {
> -		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
> -		bitmap_clear(msi_data->used, hwirq, 1);
> +	if (!msi_affinity_flag || msir->index == 0) {
> +		for (i = 0; i < msi_data->cfg->msir_irqs; i++) {
> +			hwirq = i << msi_data->cfg->ibs_shift | msir->index;
> +			bitmap_clear(msi_data->used, hwirq, 1);
> +		}
>  	}
>  
>  	return 0;
> @@ -316,6 +370,17 @@ static int ls_scfg_msi_probe(struct platform_device *pdev)
>  	bitmap_set(msi_data->used, 0, msi_data->irqs_num);
>  
>  	msi_data->msir_num = of_irq_count(pdev->dev.of_node);
> +
> +	if (msi_affinity_flag) {
> +		u32 cpu_num;
> +
> +		cpu_num = num_possible_cpus();
> +		if (msi_data->msir_num >= cpu_num)
> +			msi_data->msir_num = cpu_num;
> +		else
> +			msi_affinity_flag = 0;
> +	}
> +
>  	msi_data->msir = devm_kcalloc(&pdev->dev, msi_data->msir_num,
>  				      sizeof(*msi_data->msir),
>  				      GFP_KERNEL);
> 

This is a very confusing patch. Please get rid of this useless option
and document how you encode the routing in the hwirq.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v2,7/9] irqchip/ls-scfg-msi: add LS1046a MSI support

[M.H. Lian]

Hi Marc,

Thanks for your review.
Please see my comments inline.

Thanks,
Minghuan

> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Thursday, January 05, 2017 11:19 PM
> To: M.H. Lian <minghuan.lian@nxp.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>; Jason Cooper
> <jason@lakedaemon.net>; Roy Zang <roy.zang@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; Stuart Yoder <stuart.yoder@nxp.com>; Leo Li
> <leoyang.li@nxp.com>; Scott Wood <scott.wood@nxp.com>
> Subject: Re: [PATCH v2,7/9] irqchip/ls-scfg-msi: add LS1046a MSI support
> 
> On 05/01/17 08:10, Minghuan Lian wrote:
> > LS1046a includes 4 MSIRs, each MSIR is assigned a dedicate GIC SPI
> > interrupt and provides 32 MSI interrupts. Compared to previous MSI,
> > LS1046a's IBS(interrupt bit select) shift is changed to 2 and total
> > MSI interrupt number is changed to 128.
> >
> > The patch adds structure 'ls_scfg_msir' to describe MSIR setting and
> > 'ibs_shift' to store the different value between the SoCs.
> >
> > Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> > ---
> > v2-v1:
> > - MSI dts node change has been merged into the patch 6/9
> >
> >  drivers/irqchip/irq-ls-scfg-msi.c | 161
> > +++++++++++++++++++++++++++++---------
> >  1 file changed, 126 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-ls-scfg-msi.c
> > b/drivers/irqchip/irq-ls-scfg-msi.c
> > index cef67cc..67547bd 100644
> > --- a/drivers/irqchip/irq-ls-scfg-msi.c
> > +++ b/drivers/irqchip/irq-ls-scfg-msi.c
> > @@ -17,13 +17,24 @@
> >  #include <linux/irq.h>
> >  #include <linux/irqchip/chained_irq.h>  #include <linux/irqdomain.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/of_pci.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/spinlock.h>
> >
> > -#define MSI_MAX_IRQS	32
> > -#define MSI_IBS_SHIFT	3
> > -#define MSIR		4
> > +#define MSI_IRQS_PER_MSIR	32
> > +#define MSI_MSIR_OFFSET		4
> > +
> > +struct ls_scfg_msi_cfg {
> > +	u32 ibs_shift; /* Shift of interrupt bit select */ };
> > +
> > +struct ls_scfg_msir {
> > +	struct ls_scfg_msi *msi_data;
> > +	unsigned int index;
> > +	unsigned int gic_irq;
> > +	void __iomem *reg;
> > +};
> >
> >  struct ls_scfg_msi {
> >  	spinlock_t		lock;
> > @@ -32,8 +43,11 @@ struct ls_scfg_msi {
> >  	struct irq_domain	*msi_domain;
> >  	void __iomem		*regs;
> >  	phys_addr_t		msiir_addr;
> > -	int			irq;
> > -	DECLARE_BITMAP(used, MSI_MAX_IRQS);
> > +	struct ls_scfg_msi_cfg	*cfg;
> > +	u32			msir_num;
> > +	struct ls_scfg_msir	*msir;
> > +	u32			irqs_num;
> > +	unsigned long		*used;
> >  };
> >
> >  static struct irq_chip ls_scfg_msi_irq_chip = { @@ -55,7 +69,7 @@
> > static void ls_scfg_msi_compose_msg(struct irq_data *data, struct
> > msi_msg *msg)
> >
> >  	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
> >  	msg->address_lo = lower_32_bits(msi_data->msiir_addr);
> > -	msg->data = data->hwirq << MSI_IBS_SHIFT;
> > +	msg->data = data->hwirq;
> >  }
> >
> >  static int ls_scfg_msi_set_affinity(struct irq_data *irq_data, @@
> > -81,8 +95,8 @@ static int ls_scfg_msi_domain_irq_alloc(struct irq_domain
> *domain,
> >  	WARN_ON(nr_irqs != 1);
> >
> >  	spin_lock(&msi_data->lock);
> > -	pos = find_first_zero_bit(msi_data->used, MSI_MAX_IRQS);
> > -	if (pos < MSI_MAX_IRQS)
> > +	pos = find_first_zero_bit(msi_data->used, msi_data->irqs_num);
> > +	if (pos < msi_data->irqs_num)
> >  		__set_bit(pos, msi_data->used);
> >  	else
> >  		err = -ENOSPC;
> > @@ -106,7 +120,7 @@ static void ls_scfg_msi_domain_irq_free(struct
> irq_domain *domain,
> >  	int pos;
> >
> >  	pos = d->hwirq;
> > -	if (pos < 0 || pos >= MSI_MAX_IRQS) {
> > +	if (pos < 0 || pos >= msi_data->irqs_num) {
> >  		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> >  		return;
> >  	}
> > @@ -123,15 +137,17 @@ static void ls_scfg_msi_domain_irq_free(struct
> > irq_domain *domain,
> >
> >  static void ls_scfg_msi_irq_handler(struct irq_desc *desc)  {
> > -	struct ls_scfg_msi *msi_data = irq_desc_get_handler_data(desc);
> > +	struct ls_scfg_msir *msir = irq_desc_get_handler_data(desc);
> > +	struct ls_scfg_msi *msi_data = msir->msi_data;
> >  	unsigned long val;
> > -	int pos, virq;
> > +	int pos, virq, hwirq;
> >
> >  	chained_irq_enter(irq_desc_get_chip(desc), desc);
> >
> > -	val = ioread32be(msi_data->regs + MSIR);
> > -	for_each_set_bit(pos, &val, MSI_MAX_IRQS) {
> > -		virq = irq_find_mapping(msi_data->parent, (31 - pos));
> > +	val = ioread32be(msir->reg);
> > +	for_each_set_bit(pos, &val, MSI_IRQS_PER_MSIR) {
> > +		hwirq = ((31 - pos) << msi_data->cfg->ibs_shift) | msir-
> >index;
> > +		virq = irq_find_mapping(msi_data->parent, hwirq);
> >  		if (virq)
> >  			generic_handle_irq(virq);
> >  	}
> > @@ -143,7 +159,7 @@ static int ls_scfg_msi_domains_init(struct
> > ls_scfg_msi *msi_data)  {
> >  	/* Initialize MSI domain parent */
> >  	msi_data->parent = irq_domain_add_linear(NULL,
> > -						 MSI_MAX_IRQS,
> > +						 msi_data->irqs_num,
> >  						 &ls_scfg_msi_domain_ops,
> >  						 msi_data);
> >  	if (!msi_data->parent) {
> > @@ -164,16 +180,83 @@ static int ls_scfg_msi_domains_init(struct
> ls_scfg_msi *msi_data)
> >  	return 0;
> >  }
> >
> > +static int ls_scfg_msi_setup_hwirq(struct ls_scfg_msi *msi_data, int
> > +index) {
> > +	struct ls_scfg_msir *msir;
> > +	int virq, i, hwirq;
> > +
> > +	virq = platform_get_irq(msi_data->pdev, index);
> > +	if (virq <= 0)
> > +		return -ENODEV;
> > +
> > +	msir = &msi_data->msir[index];
> > +	msir->index = index;
> > +	msir->msi_data = msi_data;
> > +	msir->gic_irq = virq;
> > +	msir->reg = msi_data->regs + MSI_MSIR_OFFSET + 4 * index;
> > +
> > +	irq_set_chained_handler_and_data(msir->gic_irq,
> > +					 ls_scfg_msi_irq_handler,
> > +					 msir);
> > +
> > +	/* Release the hwirqs corresponding to this MSIR */
> > +	for (i = 0; i < MSI_IRQS_PER_MSIR; i++) {
> > +		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
> > +		bitmap_clear(msi_data->used, hwirq, 1);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ls_scfg_msi_teardown_hwirq(struct ls_scfg_msir *msir) {
> > +	struct ls_scfg_msi *msi_data = msir->msi_data;
> > +	int i, hwirq;
> > +
> > +	if (msir->gic_irq > 0)
> > +		irq_set_chained_handler_and_data(msir->gic_irq, NULL,
> NULL);
> > +
> > +	for (i = 0; i < MSI_IRQS_PER_MSIR; i++) {
> > +		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
> > +		bitmap_set(msi_data->used, hwirq, 1);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct ls_scfg_msi_cfg ls1021_msi_cfg = {
> > +	.ibs_shift = 3,
> > +};
> > +
> > +static struct ls_scfg_msi_cfg ls1046_msi_cfg = {
> > +	.ibs_shift = 2,
> > +};
> > +
> > +static const struct of_device_id ls_scfg_msi_id[] = {
> > +	{ .compatible = "fsl,ls1021a-msi", .data = &ls1021_msi_cfg },
> > +	{ .compatible = "fsl,ls1043a-msi", .data = &ls1021_msi_cfg },
> > +	{ .compatible = "fsl,ls1046a-msi", .data = &ls1046_msi_cfg },
> 
> So after 3 patches adding compatibility between the fsl,1s10 and
> fsl,ls10 names, you discard them? How does it work again with the old DTs?
> 
[Minghuan Lian]  ok, I will store the old compatible.
This typo makes me very uncomfortable  :(

> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, ls_scfg_msi_id);
> > +
> >  static int ls_scfg_msi_probe(struct platform_device *pdev)  {
> > +	const struct of_device_id *match;
> >  	struct ls_scfg_msi *msi_data;
> >  	struct resource *res;
> > -	int ret;
> > +	int i, ret;
> > +
> > +	match = of_match_device(ls_scfg_msi_id, &pdev->dev);
> > +	if (!match)
> > +		return -ENODEV;
> >
> >  	msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data),
> GFP_KERNEL);
> >  	if (!msi_data)
> >  		return -ENOMEM;
> >
> > +	msi_data->cfg = (struct ls_scfg_msi_cfg *) match->data;
> > +
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	msi_data->regs = devm_ioremap_resource(&pdev->dev, res);
> >  	if (IS_ERR(msi_data->regs)) {
> > @@ -182,23 +265,37 @@ static int ls_scfg_msi_probe(struct
> platform_device *pdev)
> >  	}
> >  	msi_data->msiir_addr = res->start;
> >
> > -	msi_data->irq = platform_get_irq(pdev, 0);
> > -	if (msi_data->irq <= 0) {
> > -		dev_err(&pdev->dev, "failed to get MSI irq\n");
> > -		return -ENODEV;
> > -	}
> > -
> >  	msi_data->pdev = pdev;
> >  	spin_lock_init(&msi_data->lock);
> >
> > +	msi_data->irqs_num = MSI_IRQS_PER_MSIR *
> > +			     (1 << msi_data->cfg->ibs_shift);
> > +	msi_data->used = devm_kcalloc(&pdev->dev,
> > +				    BITS_TO_LONGS(msi_data->irqs_num),
> > +				    sizeof(*msi_data->used),
> > +				    GFP_KERNEL);
> > +	if (!msi_data->used)
> > +		return -ENOMEM;
> > +	/*
> > +	 * Reserve all the hwirqs
> > +	 * The available hwirqs will be released in ls1_msi_setup_hwirq()
> > +	 */
> > +	bitmap_set(msi_data->used, 0, msi_data->irqs_num);
> > +
> > +	msi_data->msir_num = of_irq_count(pdev->dev.of_node);
> > +	msi_data->msir = devm_kcalloc(&pdev->dev, msi_data->msir_num,
> > +				      sizeof(*msi_data->msir),
> > +				      GFP_KERNEL);
> > +	if (!msi_data->msir)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < msi_data->msir_num; i++)
> > +		ls_scfg_msi_setup_hwirq(msi_data, i);
> > +
> >  	ret = ls_scfg_msi_domains_init(msi_data);
> >  	if (ret)
> >  		return ret;
> >
> > -	irq_set_chained_handler_and_data(msi_data->irq,
> > -					 ls_scfg_msi_irq_handler,
> > -					 msi_data);
> > -
> >  	platform_set_drvdata(pdev, msi_data);
> >
> >  	return 0;
> > @@ -207,8 +304,10 @@ static int ls_scfg_msi_probe(struct
> > platform_device *pdev)  static int ls_scfg_msi_remove(struct
> > platform_device *pdev)  {
> >  	struct ls_scfg_msi *msi_data = platform_get_drvdata(pdev);
> > +	int i;
> >
> > -	irq_set_chained_handler_and_data(msi_data->irq, NULL, NULL);
> > +	for (i = 0; i < msi_data->msir_num; i++)
> > +		ls_scfg_msi_teardown_hwirq(&msi_data->msir[i]);
> >
> >  	irq_domain_remove(msi_data->msi_domain);
> >  	irq_domain_remove(msi_data->parent);
> > @@ -218,14 +317,6 @@ static int ls_scfg_msi_remove(struct
> platform_device *pdev)
> >  	return 0;
> >  }
> >
> > -static const struct of_device_id ls_scfg_msi_id[] = {
> > -	{ .compatible = "fsl,1s1021a-msi", }, /* a typo */
> > -	{ .compatible = "fsl,1s1043a-msi", }, /* a typo */
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ^
> these entries definitely need to be restored.
> 
> Also, please make sure you have a cover letter at the beginning of the series.
> I flag series to review by flagging the cover letter, and I suspect many of us
> are doing something similar. Not doing so is likely to leave your series
> unreviewed...
[Minghuan Lian]  I see. I will add cover letter for this series. And I will
do this for all series of patches.

> 
> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

RE: [PATCH v2,9/9] irqchip/ls-scfg-msi: add MSI affinity support

[M.H. Lian]

Hi Marc,

Thanks for your review.
Please see my comments inline.

Thanks,
Minghuan

> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Thursday, January 05, 2017 11:33 PM
> To: M.H. Lian <minghuan.lian@nxp.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>; Jason Cooper
> <jason@lakedaemon.net>; Roy Zang <roy.zang@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; Stuart Yoder <stuart.yoder@nxp.com>; Leo Li
> <leoyang.li@nxp.com>; Scott Wood <scott.wood@nxp.com>
> Subject: Re: [PATCH v2,9/9] irqchip/ls-scfg-msi: add MSI affinity support
> 
> On 05/01/17 08:10, Minghuan Lian wrote:
> > For LS1046a and LS1043a v1.1, the MSI controller has 4 MSIRs and 4
> > CPUs. A GIC SPI interrupt of MSIR can be associated with a CPU.
> > When changing MSI interrupt affinity, this MSI will be moved to the
> > corresponding MSIR and MSI message data will be changed according to
> > MSIR. when requesting a MSI, the bits of all 4 MSIR will be reserved.
> > The parameter 'msi_affinity_flag' is provide to change this mode.
> > "lsmsi=no-affinity" will disable affinity, all MSI can only be
> > associated with CPU 0.
> >
> > Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> > ---
> > v2-v1:
> > - None
> >
> >  drivers/irqchip/irq-ls-scfg-msi.c | 75
> > ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 70 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-ls-scfg-msi.c
> > b/drivers/irqchip/irq-ls-scfg-msi.c
> > index dc19569..753fe39 100644
> > --- a/drivers/irqchip/irq-ls-scfg-msi.c
> > +++ b/drivers/irqchip/irq-ls-scfg-msi.c
> > @@ -40,6 +40,7 @@ struct ls_scfg_msir {
> >  	unsigned int gic_irq;
> >  	unsigned int bit_start;
> >  	unsigned int bit_end;
> > +	unsigned int srs; /* Shared interrupt register select */
> >  	void __iomem *reg;
> >  };
> >
> > @@ -70,6 +71,19 @@ struct ls_scfg_msi {
> >  	.chip	= &ls_scfg_msi_irq_chip,
> >  };
> >
> > +static int msi_affinity_flag = 1;
> > +
> > +static int __init early_parse_ls_scfg_msi(char *p) {
> > +	if (p && strncmp(p, "no-affinity", 11) == 0)
> > +		msi_affinity_flag = 0;
> > +	else
> > +		msi_affinity_flag = 1;
> > +
> > +	return 0;
> > +}
> > +early_param("lsmsi", early_parse_ls_scfg_msi);
> 
> What is the point of this option? If feels like an unnecessary complexity.
[Minghuan Lian] For LS1043a rev1.1, there are only 8 MSI interrupts for each MSI controller when enable affinity.
(32 MSI interrupts are evenly distributed to 4 cores).  3 MSI controllers can only provide 32 MSI interrupts.
When disable affinity, the MSI interrupts number of 3 controllers can up to 32*3= 96.
32 MSI interrupts may be not enough.
For example: an Ethernet card with 4 ports, each port needs 4 RX, 4TX and 1 error interrupts, totally need 4*(4+4+1)
36 MSI interrupts.
With the parameter "lsmsi=no-affinity" this Ethernet card can work well, although the performance is poor.
  
> 
> > +
> >  static void ls_scfg_msi_compose_msg(struct irq_data *data, struct
> > msi_msg *msg)  {
> >  	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data);
> @@
> > -77,12 +91,43 @@ static void ls_scfg_msi_compose_msg(struct irq_data
> *data, struct msi_msg *msg)
> >  	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
> >  	msg->address_lo = lower_32_bits(msi_data->msiir_addr);
> >  	msg->data = data->hwirq;
> > +
> > +	if (msi_affinity_flag) {
> > +		u32 msir_index;
> > +
> > +		msir_index = cpumask_first(data->common->affinity);
> > +		if (msir_index >= msi_data->msir_num)
> > +			msir_index = 0;
> 
> How can this happen?
[Minghuan Lian]  This will never happen. I will remove this "if" sentence. 

> 
> > +
> > +		msg->data |= msir_index;
> 
> How do you guarantee that the low bits were clear? Please document the
> way you encode your MSI payload.
[Minghuan Lian] the available message data is a bytes.
   7    6  5  4  3   2    1    0
| - |       ibs           |   srs |

SRS  bit 0-1  is used to select MSIR register/CPU.  A MSIR is associated with a CPU.
IBS bit2-6 of ls1046,  bit2-4 for ls1043 is used to select bit of the MSIR. 
When enable affinity,  only bits of MSIR0(srs = 0 cpu0) is be freed for requesting MSI.
all bits of the MSIR1-3(cpu1-3) are reserved to be sure this MSI can be successfully associated with cpu 1-3.
So, When requesting a MSI interrupt, srs always is 0. 
The hwirq always equals bits number of the MSIR0(SRS = 0).
First, MSI message data payload equals hwirq, and then plus the real SRS according to smp_affinity.
This MSI interrupt will be routed to the corresponding the MSIR/CPU.



> 
> > +	}
> >  }
> >
> >  static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,
> >  				    const struct cpumask *mask, bool force)  {
> > -	return -EINVAL;
> > +	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(irq_data);
> > +	u32 cpu;
> > +
> > +	if (!msi_affinity_flag)
> > +		return -EINVAL;
> > +
> > +	if (!force)
> > +		cpu = cpumask_any_and(mask, cpu_online_mask);
> > +	else
> > +		cpu = cpumask_first(mask);
> > +
> > +	if (cpu >= msi_data->msir_num)
> > +		return -EINVAL;
> > +
> > +	if (msi_data->msir[cpu].gic_irq <= 0) {
> > +		pr_warn("cannot bind the irq to cpu%d\n", cpu);
> 
> Please don't. Returning an error is enough. If you really want to have
> something, turn it into a proper debug message.
[Minghuan Lian] ok, I will change it.
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	cpumask_copy(irq_data->common->affinity, mask);
> > +
> > +	return IRQ_SET_MASK_OK;
> >  }
> >
> >  static struct irq_chip ls_scfg_msi_parent_chip = { @@ -158,7 +203,7
> > @@ static void ls_scfg_msi_irq_handler(struct irq_desc *desc)
> >
> >  	for_each_set_bit_from(pos, &val, size) {
> >  		hwirq = ((msir->bit_end - pos) << msi_data->cfg->ibs_shift) |
> > -			msir->index;
> > +			msir->srs;
> >  		virq = irq_find_mapping(msi_data->parent, hwirq);
> >  		if (virq)
> >  			generic_handle_irq(virq);
> > @@ -221,10 +266,19 @@ static int ls_scfg_msi_setup_hwirq(struct
> ls_scfg_msi *msi_data, int index)
> >  					 ls_scfg_msi_irq_handler,
> >  					 msir);
> >
> > +	if (msi_affinity_flag) {
> > +		/* Associate MSIR interrupt to the cpu */
> > +		irq_set_affinity(msir->gic_irq, get_cpu_mask(index));
> > +		msir->srs = 0; /* This value is determined by the CPU */
> > +	} else
> > +		msir->srs = index;
> > +
> >  	/* Release the hwirqs corresponding to this MSIR */
> > -	for (i = 0; i < msi_data->cfg->msir_irqs; i++) {
> > -		hwirq = i << msi_data->cfg->ibs_shift | msir->index;
> > -		bitmap_clear(msi_data->used, hwirq, 1);
> > +	if (!msi_affinity_flag || msir->index == 0) {
> > +		for (i = 0; i < msi_data->cfg->msir_irqs; i++) {
> > +			hwirq = i << msi_data->cfg->ibs_shift | msir->index;
> > +			bitmap_clear(msi_data->used, hwirq, 1);
> > +		}
> >  	}
> >
> >  	return 0;
> > @@ -316,6 +370,17 @@ static int ls_scfg_msi_probe(struct
> platform_device *pdev)
> >  	bitmap_set(msi_data->used, 0, msi_data->irqs_num);
> >
> >  	msi_data->msir_num = of_irq_count(pdev->dev.of_node);
> > +
> > +	if (msi_affinity_flag) {
> > +		u32 cpu_num;
> > +
> > +		cpu_num = num_possible_cpus();
> > +		if (msi_data->msir_num >= cpu_num)
> > +			msi_data->msir_num = cpu_num;
> > +		else
> > +			msi_affinity_flag = 0;
> > +	}
> > +
> >  	msi_data->msir = devm_kcalloc(&pdev->dev, msi_data->msir_num,
> >  				      sizeof(*msi_data->msir),
> >  				      GFP_KERNEL);
> >
> 
> This is a very confusing patch. Please get rid of this useless option and
> document how you encode the routing in the hwirq.
[Minghuan Lian]  Both LS1021a and LS1043av1.0 have only a MSIR, a gic interrupt.
MSI controllers cannot support affinity.
Then LS1046a/LS1043av1.1 extends MSIR number to 4 same to cpu number. So each MSIR with a GIC interrupt can be associated with a cpu.
To keep simple, the first patch for ls1046 and ls1043av1.1 keep the same way with ls1021 and ls1043av1.0 that does not support affinity and
all interrupts of MSIR0-3 are different and can be used for requesting MSI interrupts.
This patch is to enable affinity, that means, for ls1046a and ls1043av1.1, the same bit of MSIR0-3 will be looked as one interrupt using the same hwirq. And MSIRN  only is used when the MSI interrupt is associated with the corresponding cpu.

> 
> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...