[PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI

[PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI

[Minghuan Lian]

1. The different version of a SoC may have different MSI
implementation. But compatible "fsl,<soc-name>-msi" can not describe
the SoC version. The MSI driver will use SoC match interface to get
SoC type and version instead of compatible string. So all MSI node
can use the common compatible "fsl,ls-scfg-msi" and the original
compatible is unnecessary.

2. Layerscape SoCs may have one or several MSI controllers.
In order to increase MSI interrupt number of a PCIe, the patch
moves all MSI node into the parent node "msi-controller". So a
PCIe can request MSI from all the MSI controllers.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
 .../interrupt-controller/fsl,ls-scfg-msi.txt       | 57 +++++++++++++++++++---
 1 file changed, 49 insertions(+), 8 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..29f95fd 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
@@ -1,18 +1,28 @@
 * Freescale Layerscape SCFG PCIe MSI controller
 
+Layerscape SoCs may have one or multiple MSI controllers.
+Each MSI controller must be showed as a child node.
+
 Required properties:
 
-- compatible: should be "fsl,<soc-name>-msi" to identify
-	      Layerscape PCIe MSI controller block such as:
-              "fsl,1s1021a-msi"
-              "fsl,1s1043a-msi"
+- compatible: should be "fsl,ls-scfg-msi"
+- #address-cells: must be 2
+- #size-cells: must be 2
+- ranges: allows valid 1:1 translation between child's address space and
+	  parent's address space
 - msi-controller: indicates that this is a PCIe MSI controller node
+
+Required child node:
+A child node must exist to represent the MSI controller.
+The following are properties specific to those nodes:
+
 - reg: physical base address of the controller and length of memory mapped.
 - interrupts: an interrupt to the parent interrupt controller.
 
 Optional properties:
 - interrupt-parent: the phandle to the parent interrupt controller.
 
+Notes:
 This interrupt controller hardware is a second level interrupt controller that
 is hooked to a parent interrupt controller: e.g: ARM GIC for ARM-based
 platforms. If interrupt-parent is not provided, the default parent interrupt
@@ -22,9 +32,40 @@ MSI controller node
 
 Examples:
 
-	msi1: msi-controller@1571000 {
-		compatible = "fsl,1s1043a-msi";
-		reg = <0x0 0x1571000 0x0 0x8>,
+	msi: msi-controller {
+		compatible = "fsl,ls-scfg-msi";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
 		msi-controller;
-		interrupts = <0 116 0x4>;
+
+		msi0@1580000 {
+			reg = <0x0 0x1580000 0x0 0x10000>;
+			interrupts = <0 116 0x4>,
+				     <0 111 0x4>,
+				     <0 112 0x4>,
+				     <0 113 0x4>;
+		};
+
+		msi1@1590000 {
+			reg = <0x0 0x1590000 0x0 0x10000>;
+			interrupts = <0 126 0x4>,
+				     <0 121 0x4>,
+				     <0 122 0x4>,
+				     <0 123 0x4>;
+		};
+
+		msi2@15a0000 {
+			reg = <0x0 0x15a0000 0x0 0x10000>;
+			interrupts = <0 160 0x4>,
+				     <0 155 0x4>,
+				     <0 156 0x4>,
+				     <0 157 0x4>;
+		};
+	};
+
+	pcie@3400000 {
+			...
+			msi-parent = <&msi>;
+			...
 	};
-- 
1.9.1

[PATCH 2/6] arm: dts: ls1021a: update MSI node

[Minghuan Lian]

1. Change compatible to "fsl,ls-scfg-msi"
2. Move two MSI dts node into the parent node "msi-controller".
So a PCIe device can request the MSI from the two MSI controllers.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
 arch/arm/boot/dts/ls1021a.dtsi | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index 368e219..7a3b510 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -119,18 +119,22 @@
 
 		};
 
-		msi1: msi-controller@1570e00 {
-			compatible = "fsl,1s1021a-msi";
-			reg = <0x0 0x1570e00 0x0 0x8>;
+		msi: msi-controller {
+			compatible = "fsl,ls-scfg-msi";
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
 			msi-controller;
-			interrupts =  <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>;
-		};
 
-		msi2: msi-controller@1570e08 {
-			compatible = "fsl,1s1021a-msi";
-			reg = <0x0 0x1570e08 0x0 0x8>;
-			msi-controller;
-			interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
+			msi0@1570e00 {
+				reg = <0x0 0x1570e00 0x0 0x8>;
+				interrupts = <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>;
+			};
+
+			msi1@1570e08 {
+				reg = <0x0 0x1570e08 0x0 0x8>;
+				interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		ifc: ifc@1530000 {
@@ -643,7 +647,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 = <&msi>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic GIC_SPI 91  IRQ_TYPE_LEVEL_HIGH>,
@@ -666,7 +670,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 = <&msi>;
 			#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 3/6] arm64: dts: ls1043a: update MSI and PCIe node

[Minghuan Lian]

1. Change compatible to "fsl,ls-scfg-msi"
2. Move three MSI dts node into the parent node "msi-controller".
So a PCIe device can request the MSI from the three MSI controllers.
3. The rev1.1 of LS1043a moves PCIe INTB/C/D interrupts to MSI controller.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
 arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 64 +++++++++++++-------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
index 41e5dc1..5295bb9 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
@@ -587,25 +587,36 @@
 			dma-coherent;
 		};
 
-		msi1: msi-controller1@1571000 {
-			compatible = "fsl,1s1043a-msi";
-			reg = <0x0 0x1571000 0x0 0x8>;
+		msi: msi-controller {
+			compatible = "fsl,ls-scfg-msi";
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
 			msi-controller;
-			interrupts = <0 116 0x4>;
-		};
 
-		msi2: msi-controller2@1572000 {
-			compatible = "fsl,1s1043a-msi";
-			reg = <0x0 0x1572000 0x0 0x8>;
-			msi-controller;
-			interrupts = <0 126 0x4>;
-		};
+			msi0@1571000 {
+				reg = <0x0 0x1571000 0x0 0x1000>;
+				interrupts = <0 116 0x4>,
+					     <0 111 0x4>,
+					     <0 112 0x4>,
+					     <0 113 0x4>;
+			};
 
-		msi3: msi-controller3@1573000 {
-			compatible = "fsl,1s1043a-msi";
-			reg = <0x0 0x1573000 0x0 0x8>;
-			msi-controller;
-			interrupts = <0 160 0x4>;
+			msi1@1572000 {
+				reg = <0x0 0x1572000 0x0 0x1000>;
+				interrupts = <0 126 0x4>,
+					     <0 121 0x4>,
+					     <0 122 0x4>,
+					     <0 123 0x4>;
+			};
+
+			msi2@1573000 {
+				reg = <0x0 0x1573000 0x0 0x1000>;
+				interrupts = <0 160 0x4>,
+					     <0 155 0x4>,
+					     <0 156 0x4>,
+					     <0 157 0x4>;
+			};
 		};
 
 		pcie@3400000 {
@@ -624,13 +635,10 @@
 			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 = <&msi>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
-			interrupt-map = <0000 0 0 1 &gic 0 110 0x4>,
-					<0000 0 0 2 &gic 0 111 0x4>,
-					<0000 0 0 3 &gic 0 112 0x4>,
-					<0000 0 0 4 &gic 0 113 0x4>;
+			interrupt-map = <0000 0 0 1 &gic 0 110 0x4>;
 		};
 
 		pcie@3500000 {
@@ -649,13 +657,10 @@
 			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 = <&msi>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
-			interrupt-map = <0000 0 0 1 &gic 0 120  0x4>,
-					<0000 0 0 2 &gic 0 121 0x4>,
-					<0000 0 0 3 &gic 0 122 0x4>,
-					<0000 0 0 4 &gic 0 123 0x4>;
+			interrupt-map = <0000 0 0 1 &gic 0 120  0x4>;
 		};
 
 		pcie@3600000 {
@@ -674,13 +679,10 @@
 			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 = <&msi>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
-			interrupt-map = <0000 0 0 1 &gic 0 154 0x4>,
-					<0000 0 0 2 &gic 0 155 0x4>,
-					<0000 0 0 3 &gic 0 156 0x4>,
-					<0000 0 0 4 &gic 0 157 0x4>;
+			interrupt-map = <0000 0 0 1 &gic 0 154 0x4>;
 		};
 	};
 
-- 
1.9.1

[PATCH 4/6] arm64: dts: ls1046a: add MSI dts node

[Minghuan Lian]

LS1046a has three MSI controllers. each controller is assigned
four SPI interrupts.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
 arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 32 ++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
index 38806ca..5509dca 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
@@ -511,5 +511,37 @@
 			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&clockgen 4 1>;
 		};
+
+		msi: msi-controller {
+			compatible = "fsl,ls-scfg-msi";
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+			msi-controller;
+
+			msi0@1580000 {
+				reg = <0x0 0x1580000 0x0 0x10000>;
+				interrupts = <0 116 0x4>,
+					     <0 111 0x4>,
+					     <0 112 0x4>,
+					     <0 113 0x4>;
+			};
+
+			msi1@1590000 {
+				reg = <0x0 0x1590000 0x0 0x10000>;
+				interrupts = <0 126 0x4>,
+					     <0 121 0x4>,
+					     <0 122 0x4>,
+					     <0 123 0x4>;
+			};
+
+			msi2@15a0000 {
+				reg = <0x0 0x15a0000 0x0 0x10000>;
+				interrupts = <0 160 0x4>,
+					     <0 155 0x4>,
+					     <0 156 0x4>,
+					     <0 157 0x4>;
+			};
+		};
 	};
 };
-- 
1.9.1

[PATCH 5/6] arm64: dts: ls1043a: update gic dts node

[Minghuan Lian]

From: Gong Qianyu <Qianyu.Gong@nxp.com>

In order to support kvm, rev1.1 LS1043a GIC register has been
changed to align as 64K. The patch updates GIC node according to
the rev1.1 hardware.

Signed-off-by: Gong Qianyu <Qianyu.Gong@nxp.com>
Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
 arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
index 5295bb9..da1809d 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
@@ -144,10 +144,10 @@
 		compatible = "arm,gic-400";
 		#interrupt-cells = <3>;
 		interrupt-controller;
-		reg = <0x0 0x1401000 0 0x1000>, /* GICD */
-		      <0x0 0x1402000 0 0x2000>, /* GICC */
-		      <0x0 0x1404000 0 0x2000>, /* GICH */
-		      <0x0 0x1406000 0 0x2000>; /* GICV */
+		reg = <0x0 0x1410000 0 0x10000>, /* GICD */
+		      <0x0 0x1420000 0 0x20000>, /* GICC */
+		      <0x0 0x1440000 0 0x20000>, /* GICH */
+		      <0x0 0x1460000 0 0x20000>; /* GICV */
 		interrupts = <1 9 0xf08>;
 	};
 
-- 
1.9.1

[PATCH 6/6] arm64: dts: ls1046a: add PCIe dts node

[Minghuan Lian]

LS1046a has three PCIe controllers.

Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
---
 arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 66 ++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
index 5509dca..427cba4 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
@@ -543,5 +543,71 @@
 					     <0 157 0x4>;
 			};
 		};
+
+		pcie@3400000 {
+			compatible = "fsl,ls1046a-pcie", "snps,dw-pcie";
+			reg = <0x00 0x03400000 0x0 0x00100000   /* controller registers */
+			       0x40 0x00000000 0x0 0x00002000>; /* configuration space */
+			reg-names = "regs", "config";
+			interrupts = <0 118 0x4>, /* controller interrupt */
+				     <0 117 0x4>; /* PME interrupt */
+			interrupt-names = "aer", "pme";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			dma-coherent;
+			num-lanes = <4>;
+			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 = <&msi>;
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0000 0 0 1 &gic 0 110 0x4>;
+		};
+
+		pcie@3500000 {
+			compatible = "fsl,ls1046a-pcie", "snps,dw-pcie";
+			reg = <0x00 0x03500000 0x0 0x00100000   /* controller registers */
+			       0x48 0x00000000 0x0 0x00002000>; /* configuration space */
+			reg-names = "regs", "config";
+			interrupts = <0 128 0x4>,
+				     <0 127 0x4>;
+			interrupt-names = "aer", "pme";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			dma-coherent;
+			num-lanes = <2>;
+			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 = <&msi>;
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0000 0 0 1 &gic 0 120 0x4>;
+		};
+
+		pcie@3600000 {
+			compatible = "fsl,ls1046a-pcie", "snps,dw-pcie";
+			reg = <0x00 0x03600000 0x0 0x00100000   /* controller registers */
+			       0x50 0x00000000 0x0 0x00002000>; /* configuration space */
+			reg-names = "regs", "config";
+			interrupts = <0 162 0x4>,
+				     <0 161 0x4>;
+			interrupt-names = "aer", "pme";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			dma-coherent;
+			num-lanes = <2>;
+			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 = <&msi>;
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0000 0 0 1 &gic 0 154 0x4>;
+		};
 	};
 };
-- 
1.9.1

Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI

[Robin Murphy]

On 25/10/16 13:35, Minghuan Lian wrote:
> 1. The different version of a SoC may have different MSI
> implementation. But compatible "fsl,<soc-name>-msi" can not describe
> the SoC version.

Can't it?

	compatible = "fsl-ls1043a-rev11-msi";

Oh, I guess it can!

Joking aside, if there are multiple versions of a piece of hardware
which require *different* treatment by drivers, then it is obviously
wrong to use the same compatible string because *they are not compatible*.

> The MSI driver will use SoC match interface to get
> SoC type and version instead of compatible string. So all MSI node
> can use the common compatible "fsl,ls-scfg-msi" and the original
> compatible is unnecessary.

If there is some common level of functionality that *all* variants
support without the driver having to know which one is which, then there
might be some sense in having an additional common compatible to
represent that level of functionality, e.g.

	compatible = "fsl-ls1043a-rev11-msi", "fsl,ls-scfg-msi";

But if, say, new variants turn out to have less functionality, rather
than more, then there's probably not much point, and we should stick to
specific, accurate, compatible strings.

DT is not specific to a kernel version, nor even to Linux. A string
which triggers some board-specific magic in a specific version of a
Linux driver does not describe the hardware.

Robin.

> 2. Layerscape SoCs may have one or several MSI controllers.
> In order to increase MSI interrupt number of a PCIe, the patch
> moves all MSI node into the parent node "msi-controller". So a
> PCIe can request MSI from all the MSI controllers.
> 
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
>  .../interrupt-controller/fsl,ls-scfg-msi.txt       | 57 +++++++++++++++++++---
>  1 file changed, 49 insertions(+), 8 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..29f95fd 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
> @@ -1,18 +1,28 @@
>  * Freescale Layerscape SCFG PCIe MSI controller
>  
> +Layerscape SoCs may have one or multiple MSI controllers.
> +Each MSI controller must be showed as a child node.
> +
>  Required properties:
>  
> -- compatible: should be "fsl,<soc-name>-msi" to identify
> -	      Layerscape PCIe MSI controller block such as:
> -              "fsl,1s1021a-msi"
> -              "fsl,1s1043a-msi"
> +- compatible: should be "fsl,ls-scfg-msi"
> +- #address-cells: must be 2
> +- #size-cells: must be 2
> +- ranges: allows valid 1:1 translation between child's address space and
> +	  parent's address space
>  - msi-controller: indicates that this is a PCIe MSI controller node
> +
> +Required child node:
> +A child node must exist to represent the MSI controller.
> +The following are properties specific to those nodes:
> +
>  - reg: physical base address of the controller and length of memory mapped.
>  - interrupts: an interrupt to the parent interrupt controller.
>  
>  Optional properties:
>  - interrupt-parent: the phandle to the parent interrupt controller.
>  
> +Notes:
>  This interrupt controller hardware is a second level interrupt controller that
>  is hooked to a parent interrupt controller: e.g: ARM GIC for ARM-based
>  platforms. If interrupt-parent is not provided, the default parent interrupt
> @@ -22,9 +32,40 @@ MSI controller node
>  
>  Examples:
>  
> -	msi1: msi-controller@1571000 {
> -		compatible = "fsl,1s1043a-msi";
> -		reg = <0x0 0x1571000 0x0 0x8>,
> +	msi: msi-controller {
> +		compatible = "fsl,ls-scfg-msi";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
>  		msi-controller;
> -		interrupts = <0 116 0x4>;
> +
> +		msi0@1580000 {
> +			reg = <0x0 0x1580000 0x0 0x10000>;
> +			interrupts = <0 116 0x4>,
> +				     <0 111 0x4>,
> +				     <0 112 0x4>,
> +				     <0 113 0x4>;
> +		};
> +
> +		msi1@1590000 {
> +			reg = <0x0 0x1590000 0x0 0x10000>;
> +			interrupts = <0 126 0x4>,
> +				     <0 121 0x4>,
> +				     <0 122 0x4>,
> +				     <0 123 0x4>;
> +		};
> +
> +		msi2@15a0000 {
> +			reg = <0x0 0x15a0000 0x0 0x10000>;
> +			interrupts = <0 160 0x4>,
> +				     <0 155 0x4>,
> +				     <0 156 0x4>,
> +				     <0 157 0x4>;
> +		};
> +	};
> +
> +	pcie@3400000 {
> +			...
> +			msi-parent = <&msi>;
> +			...
>  	};
> 

RE: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI

[M.H. Lian]

Hi Robin,

Please see my comments inline.

Thanks,
Minghuan

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: Tuesday, October 25, 2016 9:01 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: Marc Zyngier <marc.zyngier@arm.com>; Stuart Yoder
> <stuart.yoder@nxp.com>; Leo Li <leoyang.li@nxp.com>; Scott Wood
> <scott.wood@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Mingkai Hu
> <mingkai.hu@nxp.com>
> Subject: Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG
> MSI
> 
> On 25/10/16 13:35, Minghuan Lian wrote:
> > 1. The different version of a SoC may have different MSI
> > implementation. But compatible "fsl,<soc-name>-msi" can not describe
> > the SoC version.
> 
> Can't it?
> 
> 	compatible = "fsl-ls1043a-rev11-msi";
> 
> Oh, I guess it can!
> 
> Joking aside, if there are multiple versions of a piece of hardware which
> require *different* treatment by drivers, then it is obviously wrong to use
> the same compatible string because *they are not compatible*.
> 
[Minghuan Lian]  Yes, but Rev1.0 and Rev1.1 SoC will use the same dts files.
We cannot create different dts files for each revision of the same kind of SoC.
It means that there are different variants in the different versions of the same SoC that will use the same compatible string.
So I have to use SoC match interface to get the versions.

I'm too radical. I do not want to first check SoC family via compatible string and then check revision via SoC match or SVR.
I selected the "SoC match" like the following to get the related information at only one place. 

static struct soc_device_attribute soc_msi_matches[] = {
	{ .family = "QorIQ LS1021A",
	  .data = &ls1021_msi_cfg },
	{ .family = "QorIQ LS1012A",
	  .data = &ls1021_msi_cfg },
	{ .family = "QorIQ LS1043A", .revision = "1.0",
	  .data = &ls1021_msi_cfg },
	{ .family = "QorIQ LS1043A", .revision = "1.1",
	  .data = &ls1043_rev11_msi_cfg },
	{ .family = "QorIQ LS1046A",
	  .data = &ls1046_msi_cfg },
	{ },
};

I will remain the SoC related compatible and try to describe the difference via some kind of the property.

> > The MSI driver will use SoC match interface to get SoC type and
> > version instead of compatible string. So all MSI node can use the
> > common compatible "fsl,ls-scfg-msi" and the original compatible is
> > unnecessary.
> 
> If there is some common level of functionality that *all* variants support
> without the driver having to know which one is which, then there might be
> some sense in having an additional common compatible to represent that
> level of functionality, e.g.
> 
> 	compatible = "fsl-ls1043a-rev11-msi", "fsl,ls-scfg-msi";
> 
> But if, say, new variants turn out to have less functionality, rather than more,
> then there's probably not much point, and we should stick to specific,
> accurate, compatible strings.
> 
> DT is not specific to a kernel version, nor even to Linux. A string which triggers
> some board-specific magic in a specific version of a Linux driver does not
> describe the hardware.
> 
> Robin.
> 
> > 2. Layerscape SoCs may have one or several MSI controllers.
> > In order to increase MSI interrupt number of a PCIe, the patch moves
> > all MSI node into the parent node "msi-controller". So a PCIe can
> > request MSI from all the MSI controllers.
> >
> > Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> > ---
> >  .../interrupt-controller/fsl,ls-scfg-msi.txt       | 57 +++++++++++++++++++--
> -
> >  1 file changed, 49 insertions(+), 8 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-m
> > si.txt
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-m
> > si.txt
> > index 9e38949..29f95fd 100644
> > ---
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-m
> > si.txt
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-sc
> > +++ fg-msi.txt
> > @@ -1,18 +1,28 @@
> >  * Freescale Layerscape SCFG PCIe MSI controller
> >
> > +Layerscape SoCs may have one or multiple MSI controllers.
> > +Each MSI controller must be showed as a child node.
> > +
> >  Required properties:
> >
> > -- compatible: should be "fsl,<soc-name>-msi" to identify
> > -	      Layerscape PCIe MSI controller block such as:
> > -              "fsl,1s1021a-msi"
> > -              "fsl,1s1043a-msi"
> > +- compatible: should be "fsl,ls-scfg-msi"
> > +- #address-cells: must be 2
> > +- #size-cells: must be 2
> > +- ranges: allows valid 1:1 translation between child's address space and
> > +	  parent's address space
> >  - msi-controller: indicates that this is a PCIe MSI controller node
> > +
> > +Required child node:
> > +A child node must exist to represent the MSI controller.
> > +The following are properties specific to those nodes:
> > +
> >  - reg: physical base address of the controller and length of memory
> mapped.
> >  - interrupts: an interrupt to the parent interrupt controller.
> >
> >  Optional properties:
> >  - interrupt-parent: the phandle to the parent interrupt controller.
> >
> > +Notes:
> >  This interrupt controller hardware is a second level interrupt
> > controller that  is hooked to a parent interrupt controller: e.g: ARM
> > GIC for ARM-based  platforms. If interrupt-parent is not provided, the
> > default parent interrupt @@ -22,9 +32,40 @@ MSI controller node
> >
> >  Examples:
> >
> > -	msi1: msi-controller@1571000 {
> > -		compatible = "fsl,1s1043a-msi";
> > -		reg = <0x0 0x1571000 0x0 0x8>,
> > +	msi: msi-controller {
> > +		compatible = "fsl,ls-scfg-msi";
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> >  		msi-controller;
> > -		interrupts = <0 116 0x4>;
> > +
> > +		msi0@1580000 {
> > +			reg = <0x0 0x1580000 0x0 0x10000>;
> > +			interrupts = <0 116 0x4>,
> > +				     <0 111 0x4>,
> > +				     <0 112 0x4>,
> > +				     <0 113 0x4>;
> > +		};
> > +
> > +		msi1@1590000 {
> > +			reg = <0x0 0x1590000 0x0 0x10000>;
> > +			interrupts = <0 126 0x4>,
> > +				     <0 121 0x4>,
> > +				     <0 122 0x4>,
> > +				     <0 123 0x4>;
> > +		};
> > +
> > +		msi2@15a0000 {
> > +			reg = <0x0 0x15a0000 0x0 0x10000>;
> > +			interrupts = <0 160 0x4>,
> > +				     <0 155 0x4>,
> > +				     <0 156 0x4>,
> > +				     <0 157 0x4>;
> > +		};
> > +	};
> > +
> > +	pcie@3400000 {
> > +			...
> > +			msi-parent = <&msi>;
> > +			...
> >  	};
> >

Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI

[Mark Rutland]

On Wed, Oct 26, 2016 at 06:55:22AM +0000, M.H. Lian wrote:
> Hi Robin,
> 
> Please see my comments inline.
> 
> Thanks,
> Minghuan
> 
> > -----Original Message-----
> > From: Robin Murphy [mailto:robin.murphy@arm.com]
> > Sent: Tuesday, October 25, 2016 9:01 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: Marc Zyngier <marc.zyngier@arm.com>; Stuart Yoder
> > <stuart.yoder@nxp.com>; Leo Li <leoyang.li@nxp.com>; Scott Wood
> > <scott.wood@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Mingkai Hu
> > <mingkai.hu@nxp.com>
> > Subject: Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG
> > MSI
> > 
> > On 25/10/16 13:35, Minghuan Lian wrote:
> > > 1. The different version of a SoC may have different MSI
> > > implementation. But compatible "fsl,<soc-name>-msi" can not describe
> > > the SoC version.
> > 
> > Can't it?
> > 
> > 	compatible = "fsl-ls1043a-rev11-msi";
> > 
> > Oh, I guess it can!
> > 
> > Joking aside, if there are multiple versions of a piece of hardware which
> > require *different* treatment by drivers, then it is obviously wrong to use
> > the same compatible string because *they are not compatible*.
> > 
> [Minghuan Lian]  Yes, but Rev1.0 and Rev1.1 SoC will use the same dts files.
> We cannot create different dts files for each revision of the same kind of SoC.

... why?

The DT should describe the hardware; if hardware differs then it should
have a different DT.

> It means that there are different variants in the different versions
> of the same SoC that will use the same compatible string.

Why can you not add a string for each variant, in addition to the SoC
string? We do that elsewhere.

> So I have to use SoC match interface to get the versions.
> 
> I'm too radical. I do not want to first check SoC family via
> compatible string and then check revision via SoC match or SVR.

You can have *both* in the the compatible string list, e.g. at the top
level:

	compatible = "vendor,soc-rev", "vendor-soc";

For devices which differ, this can be encoded similarly in the device
compatible string list.

> I selected the "SoC match" like the following to get the related information at only one place. 
> 
> static struct soc_device_attribute soc_msi_matches[] = {
> 	{ .family = "QorIQ LS1021A",
> 	  .data = &ls1021_msi_cfg },
> 	{ .family = "QorIQ LS1012A",
> 	  .data = &ls1021_msi_cfg },
> 	{ .family = "QorIQ LS1043A", .revision = "1.0",
> 	  .data = &ls1021_msi_cfg },
> 	{ .family = "QorIQ LS1043A", .revision = "1.1",
> 	  .data = &ls1043_rev11_msi_cfg },
> 	{ .family = "QorIQ LS1046A",
> 	  .data = &ls1046_msi_cfg },
> 	{ },
> };
> 
> I will remain the SoC related compatible and try to describe the
> difference via some kind of the property.

As commented on the driver side, this should be described with DT
properties on the devices.

Thanks,
Mark.

Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI

[Mark Rutland]

On Tue, Oct 25, 2016 at 08:35:40PM +0800, Minghuan Lian wrote:
> 1. The different version of a SoC may have different MSI
> implementation. But compatible "fsl,<soc-name>-msi" can not describe
> the SoC version. 

Surely, "fsl,<soc-name>-<rev>-msi" can describe this?

If the hardware differs, it needs a new compatible string.

If there's some configuration value that varies across revisions (e.g.
number of slots), you can add a proeprty to describe that explciitly.

> The MSI driver will use SoC match interface to get
> SoC type and version instead of compatible string. So all MSI node
> can use the common compatible "fsl,ls-scfg-msi" and the original
> compatible is unnecessary.
> 
> 2. Layerscape SoCs may have one or several MSI controllers.
> In order to increase MSI interrupt number of a PCIe, the patch
> moves all MSI node into the parent node "msi-controller". So a
> PCIe can request MSI from all the MSI controllers.

This is not necessary, and does not represent a real block of hardware.
So NAK for this approach.

The msi-parent property can contain a list of MSI controllers. See the
examples in
Documentation/devicetree/bindings/interrupt-controller/msi.txt.
Likewise, the msi-map property can map to a number of MSI controllers.

If the core code can only consider one at a time, then that's an issue
to be addressed in core code, not one to be bodged around in bindings.

> 
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
>  .../interrupt-controller/fsl,ls-scfg-msi.txt       | 57 +++++++++++++++++++---
>  1 file changed, 49 insertions(+), 8 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..29f95fd 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-msi.txt
> @@ -1,18 +1,28 @@
>  * Freescale Layerscape SCFG PCIe MSI controller
>  
> +Layerscape SoCs may have one or multiple MSI controllers.
> +Each MSI controller must be showed as a child node.
> +
>  Required properties:
>  
> -- compatible: should be "fsl,<soc-name>-msi" to identify
> -	      Layerscape PCIe MSI controller block such as:
> -              "fsl,1s1021a-msi"
> -              "fsl,1s1043a-msi"
> +- compatible: should be "fsl,ls-scfg-msi"

This breaks old DTBs, and throws away information which you describe
above as valuable. So another NAK for that.

> +- #address-cells: must be 2
> +- #size-cells: must be 2
> +- ranges: allows valid 1:1 translation between child's address space and
> +	  parent's address space
>  - msi-controller: indicates that this is a PCIe MSI controller node
> +
> +Required child node:
> +A child node must exist to represent the MSI controller.
> +The following are properties specific to those nodes:

Also, as above, the approach of gathering MSI controllers in this manner
is wrong.

Thanks,
Mark.

Re: [PATCH 3/6] arm64: dts: ls1043a: update MSI and PCIe node

[Mark Rutland]

On Tue, Oct 25, 2016 at 08:35:42PM +0800, Minghuan Lian wrote:
> 3. The rev1.1 of LS1043a moves PCIe INTB/C/D interrupts to MSI controller.

[...]

> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> index 41e5dc1..5295bb9 100644

> -			interrupt-map = <0000 0 0 1 &gic 0 110 0x4>,
> -					<0000 0 0 2 &gic 0 111 0x4>,
> -					<0000 0 0 3 &gic 0 112 0x4>,
> -					<0000 0 0 4 &gic 0 113 0x4>;
> +			interrupt-map = <0000 0 0 1 &gic 0 110 0x4>;

[...]

> -			interrupt-map = <0000 0 0 1 &gic 0 120  0x4>,
> -					<0000 0 0 2 &gic 0 121 0x4>,
> -					<0000 0 0 3 &gic 0 122 0x4>,
> -					<0000 0 0 4 &gic 0 123 0x4>;
> +			interrupt-map = <0000 0 0 1 &gic 0 120  0x4>;
>  		};

[...]

> -			interrupt-map = <0000 0 0 1 &gic 0 154 0x4>,
> -					<0000 0 0 2 &gic 0 155 0x4>,
> -					<0000 0 0 3 &gic 0 156 0x4>,
> -					<0000 0 0 4 &gic 0 157 0x4>;
> +			interrupt-map = <0000 0 0 1 &gic 0 154 0x4>;

Per the description above, this breaks revisions prior to 1.1.

Please, split the rev 1.1 changes into a new DTS. Share the common parts
in a common dtsi.

Thanks,
Mark.

Re: [PATCH 5/6] arm64: dts: ls1043a: update gic dts node

[Mark Rutland]

On Tue, Oct 25, 2016 at 08:35:44PM +0800, Minghuan Lian wrote:
> From: Gong Qianyu <Qianyu.Gong@nxp.com>
> 
> In order to support kvm, rev1.1 LS1043a GIC register has been
> changed to align as 64K. The patch updates GIC node according to
> the rev1.1 hardware.
> 
> Signed-off-by: Gong Qianyu <Qianyu.Gong@nxp.com>
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> index 5295bb9..da1809d 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> @@ -144,10 +144,10 @@
>  		compatible = "arm,gic-400";
>  		#interrupt-cells = <3>;
>  		interrupt-controller;
> -		reg = <0x0 0x1401000 0 0x1000>, /* GICD */
> -		      <0x0 0x1402000 0 0x2000>, /* GICC */
> -		      <0x0 0x1404000 0 0x2000>, /* GICH */
> -		      <0x0 0x1406000 0 0x2000>; /* GICV */
> +		reg = <0x0 0x1410000 0 0x10000>, /* GICD */
> +		      <0x0 0x1420000 0 0x20000>, /* GICC */
> +		      <0x0 0x1440000 0 0x20000>, /* GICH */
> +		      <0x0 0x1460000 0 0x20000>; /* GICV */

... this breaks HW prior to rev1.1, then.

Thanks,
Mark.

RE: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI

[Leo Li]

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: Wednesday, October 26, 2016 5:31 AM
> To: M.H. Lian <minghuan.lian@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Stuart
> Yoder <stuart.yoder@nxp.com>; Leo Li <leoyang.li@nxp.com>; Scott Wood
> <scott.wood@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Mingkai Hu
> <mingkai.hu@nxp.com>
> Subject: Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI
> 
> On Tue, Oct 25, 2016 at 08:35:40PM +0800, Minghuan Lian wrote:
> > 1. The different version of a SoC may have different MSI
> > implementation. But compatible "fsl,<soc-name>-msi" can not describe
> > the SoC version.
> 
> Surely, "fsl,<soc-name>-<rev>-msi" can describe this?
> 
> If the hardware differs, it needs a new compatible string.
> 
> If there's some configuration value that varies across revisions (e.g.
> number of slots), you can add a proeprty to describe that explciitly.
> 
> > The MSI driver will use SoC match interface to get SoC type and
> > version instead of compatible string. So all MSI node can use the
> > common compatible "fsl,ls-scfg-msi" and the original compatible is
> > unnecessary.
> >
> > 2. Layerscape SoCs may have one or several MSI controllers.
> > In order to increase MSI interrupt number of a PCIe, the patch moves
> > all MSI node into the parent node "msi-controller". So a PCIe can
> > request MSI from all the MSI controllers.
> 
> This is not necessary, and does not represent a real block of hardware.
> So NAK for this approach.
> 
> The msi-parent property can contain a list of MSI controllers. See the examples
> in Documentation/devicetree/bindings/interrupt-controller/msi.txt.
> Likewise, the msi-map property can map to a number of MSI controllers.
> 
> If the core code can only consider one at a time, then that's an issue to be
> addressed in core code, not one to be bodged around in bindings.
> 
> >
> > Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> > ---
> >  .../interrupt-controller/fsl,ls-scfg-msi.txt       | 57 +++++++++++++++++++---
> >  1 file changed, 49 insertions(+), 8 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-m
> > si.txt
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-m
> > si.txt
> > index 9e38949..29f95fd 100644
> > ---
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-scfg-m
> > si.txt
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-sc
> > +++ fg-msi.txt
> > @@ -1,18 +1,28 @@
> >  * Freescale Layerscape SCFG PCIe MSI controller
> >
> > +Layerscape SoCs may have one or multiple MSI controllers.
> > +Each MSI controller must be showed as a child node.
> > +
> >  Required properties:
> >
> > -- compatible: should be "fsl,<soc-name>-msi" to identify
> > -	      Layerscape PCIe MSI controller block such as:
> > -              "fsl,1s1021a-msi"
> > -              "fsl,1s1043a-msi"
> > +- compatible: should be "fsl,ls-scfg-msi"
> 
> This breaks old DTBs, and throws away information which you describe above as
> valuable. So another NAK for that.

I agree with you that we should maintain the backward compatibility.  But on the other hand, I just found that there is a silly typo in the original binding that "ls" is wrongly spelled as "1s" and they look too close to be noticed in previous patch reviews.  :(  The driver and all the DTSes used the binding with the typo which covered up the problem.  So even if we want to keep the "fsl,<soc-name>-msi" binding, we probably want to fix the typo, right?  And that breaks the backward compatibility too.

Regards,
Leo

Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI

[Mark Rutland]

On Wed, Oct 26, 2016 at 10:09:07PM +0000, Leo Li wrote:
> 
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > Sent: Wednesday, October 26, 2016 5:31 AM
> > To: M.H. Lian <minghuan.lian@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Stuart
> > Yoder <stuart.yoder@nxp.com>; Leo Li <leoyang.li@nxp.com>; Scott Wood
> > <scott.wood@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Mingkai Hu
> > <mingkai.hu@nxp.com>
> > Subject: Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI
> > 
> > On Tue, Oct 25, 2016 at 08:35:40PM +0800, Minghuan Lian wrote:

> > > -- compatible: should be "fsl,<soc-name>-msi" to identify
> > > -	      Layerscape PCIe MSI controller block such as:
> > > -              "fsl,1s1021a-msi"
> > > -              "fsl,1s1043a-msi"
> > > +- compatible: should be "fsl,ls-scfg-msi"
> > 
> > This breaks old DTBs, and throws away information which you describe above as
> > valuable. So another NAK for that.
> 
> I agree with you that we should maintain the backward compatibility.
> But on the other hand, I just found that there is a silly typo in the
> original binding that "ls" is wrongly spelled as "1s" and they look
> too close to be noticed in previous patch reviews.  :(

Sure, that's annoying, but we're stuck with it.

> The driver and all the DTSes used the binding with the typo which
> covered up the problem.  So even if we want to keep the
> "fsl,<soc-name>-msi" binding, we probably want to fix the typo, right?
> And that breaks the backward compatibility too.

Regardless of what we do, we should *not* break compatibility. The old
strings must remain.

However, we can *add* correctly-spelt variants, and mark the existing
strings as deprecated (in both the binding and driver). The in-kernel
dts can be updated to use the correctly-spelt strings.

Thanks,
Mark.

Re: [PATCH 2/6] arm: dts: ls1021a: update MSI node

[Rob Herring]

On Tue, Oct 25, 2016 at 08:35:41PM +0800, Minghuan Lian wrote:
> 1. Change compatible to "fsl,ls-scfg-msi"

That is obvious from the diff. Write your commit message to answer the 
question Why?

This also breaks compatibility with old DTBs.

> 2. Move two MSI dts node into the parent node "msi-controller".
> So a PCIe device can request the MSI from the two MSI controllers.
> 
> Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com>
> ---
>  arch/arm/boot/dts/ls1021a.dtsi | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
> index 368e219..7a3b510 100644
> --- a/arch/arm/boot/dts/ls1021a.dtsi
> +++ b/arch/arm/boot/dts/ls1021a.dtsi
> @@ -119,18 +119,22 @@
>  
>  		};
>  
> -		msi1: msi-controller@1570e00 {
> -			compatible = "fsl,1s1021a-msi";
> -			reg = <0x0 0x1570e00 0x0 0x8>;
> +		msi: msi-controller {
> +			compatible = "fsl,ls-scfg-msi";
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
>  			msi-controller;
> -			interrupts =  <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>;
> -		};
>  
> -		msi2: msi-controller@1570e08 {
> -			compatible = "fsl,1s1021a-msi";
> -			reg = <0x0 0x1570e08 0x0 0x8>;
> -			msi-controller;
> -			interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
> +			msi0@1570e00 {
> +				reg = <0x0 0x1570e00 0x0 0x8>;
> +				interrupts = <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>;
> +			};
> +
> +			msi1@1570e08 {
> +				reg = <0x0 0x1570e08 0x0 0x8>;
> +				interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
> +			};
>  		};
>  
>  		ifc: ifc@1530000 {
> @@ -643,7 +647,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 = <&msi>;
>  			#interrupt-cells = <1>;
>  			interrupt-map-mask = <0 0 0 7>;
>  			interrupt-map = <0000 0 0 1 &gic GIC_SPI 91  IRQ_TYPE_LEVEL_HIGH>,
> @@ -666,7 +670,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 = <&msi>;
>  			#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
>