[PATCHv2 0/5] Update Stratix10 EDAC Bindings

[PATCHv2 0/5] Update Stratix10 EDAC Bindings

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Instead of using the Arria10 (ARM32) EDAC bindings for
Stratix10 (ARM64), create Stratix10 specific EDAC bindings
to capture architecture differences between ARM32 and ARM64.
This requires fixing the previous Stratix10 bindings.
Also add the peripheral bindings for the Stratix10.

Thor Thayer (5):
  Documentation: dt: edac: Fix Stratix10 IRQ bindings
  Documentation: dt: edac: Add Stratix10 Peripheral bindings
  EDAC, altera: Skip DB IRQ for Stratix10
  arm64: dts: stratix10: Use new Stratix10 EDAC bindings
  EDAC, altera: Remove Stratix10 Machine compatible check

 .../devicetree/bindings/edac/socfpga-eccmgr.txt    | 129 +++++++++++++++++++--
 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi  |  25 ++--
 drivers/edac/altera_edac.c                         |  49 ++++----
 3 files changed, 158 insertions(+), 45 deletions(-)

-- 
2.7.4

[PATCHv2 1/5] Documentation: dt: edac: Fix Stratix10 IRQ bindings

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Fix Stratix10 ECC bindings to specify only the single
bit error. On Stratix10 double bit errors are handled
as SErrors instead of interrupts.
Indicate the differences between the ARM64 and ARM32
EDAC architecture in the bindings.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2 No change
---
 .../devicetree/bindings/edac/socfpga-eccmgr.txt    | 23 +++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
index 5626560a6cfd..a0ac50e15912 100644
--- a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
+++ b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
@@ -236,33 +236,42 @@ Stratix10 SoCFPGA ECC Manager
 The Stratix10 SoC ECC Manager handles the IRQs for each peripheral
 in a shared register similar to the Arria10. However, ECC requires
 access to registers that can only be read from Secure Monitor with
-SMC calls. Therefore the device tree is slightly different.
+SMC calls. Therefore the device tree is slightly different. Note that
+only 1 interrupt is sent because the double bit errors are treated as
+SErrors instead of IRQ.
 
 Required Properties:
 - compatible : Should be "altr,socfpga-s10-ecc-manager"
-- interrupts : Should be single bit error interrupt, then double bit error
-	interrupt.
+- altr,sysgr-syscon : phandle to Stratix10 System Manager Block
+	              containing the ECC manager registers.
+- interrupts : Should be single bit error interrupt.
 - interrupt-controller : boolean indicator that ECC Manager is an interrupt controller
 - #interrupt-cells : must be set to 2.
+- #address-cells: must be 1
+- #size-cells: must be 1
+- ranges : standard definition, should translate from local addresses
 
 Subcomponents:
 
 SDRAM ECC
 Required Properties:
 - compatible : Should be "altr,sdram-edac-s10"
-- interrupts : Should be single bit error interrupt, then double bit error
-	interrupt, in this order.
+- interrupts : Should be single bit error interrupt.
 
 Example:
 
 	eccmgr {
 		compatible = "altr,socfpga-s10-ecc-manager";
-		interrupts = <0 15 4>, <0 95 4>;
+		altr,sysmgr-syscon = <&sysmgr>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		interrupts = <0 15 4>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
+		ranges;
 
 		sdramedac {
 			compatible = "altr,sdram-edac-s10";
-			interrupts = <16 4>, <48 4>;
+			interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
 		};
 	};
-- 
2.7.4

[PATCHv2 2/5] Documentation: dt: edac: Add Stratix10 Peripheral bindings

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Add peripheral bindings for Stratix10 EDAC to capture
the differences between the ARM64 and ARM32 architecture.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2 No change
---
 .../devicetree/bindings/edac/socfpga-eccmgr.txt    | 106 +++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
index a0ac50e15912..a0fa80c53d2a 100644
--- a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
+++ b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
@@ -258,6 +258,49 @@ Required Properties:
 - compatible : Should be "altr,sdram-edac-s10"
 - interrupts : Should be single bit error interrupt.
 
+On-Chip RAM ECC
+Required Properties:
+- compatible      : Should be "altr,socfpga-s10-ocram-ecc"
+- reg             : Address and size for ECC block registers.
+- altr,ecc-parent : phandle to parent OCRAM node.
+- interrupts      : Should be single bit error interrupt.
+
+Ethernet FIFO ECC
+Required Properties:
+- compatible      : Should be "altr,socfpga-s10-eth-mac-ecc"
+- reg             : Address and size for ECC block registers.
+- altr,ecc-parent : phandle to parent Ethernet node.
+- interrupts      : Should be single bit error interrupt.
+
+NAND FIFO ECC
+Required Properties:
+- compatible      : Should be "altr,socfpga-s10-nand-ecc"
+- reg             : Address and size for ECC block registers.
+- altr,ecc-parent : phandle to parent NAND node.
+- interrupts      : Should be single bit error interrupt.
+
+DMA FIFO ECC
+Required Properties:
+- compatible      : Should be "altr,socfpga-s10-dma-ecc"
+- reg             : Address and size for ECC block registers.
+- altr,ecc-parent : phandle to parent DMA node.
+- interrupts      : Should be single bit error interrupt.
+
+USB FIFO ECC
+Required Properties:
+- compatible      : Should be "altr,socfpga-s10-usb-ecc"
+- reg             : Address and size for ECC block registers.
+- altr,ecc-parent : phandle to parent USB node.
+- interrupts      : Should be single bit error interrupt.
+
+SDMMC FIFO ECC
+Required Properties:
+- compatible      : Should be "altr,socfpga-s10-sdmmc-ecc"
+- reg             : Address and size for ECC block registers.
+- altr,ecc-parent : phandle to parent SD/MMC node.
+- interrupts      : Should be single bit error interrupt for port A
+		    and then single bit error interrupt for port B.
+
 Example:
 
 	eccmgr {
@@ -274,4 +317,67 @@ Example:
 			compatible = "altr,sdram-edac-s10";
 			interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
 		};
+
+		ocram-ecc@ff8cc000 {
+			compatible = "altr,socfpga-s10-ocram-ecc";
+			reg = <ff8cc000 0x100>;
+			altr,ecc-parent = <&ocram>;
+			interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		emac0-rx-ecc@ff8c0000 {
+			compatible = "altr,socfpga-s10-eth-mac-ecc";
+			reg = <0xff8c0000 0x100>;
+			altr,ecc-parent = <&gmac0>;
+			interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		emac0-tx-ecc@ff8c0400 {
+			compatible = "altr,socfpga-s10-eth-mac-ecc";
+			reg = <0xff8c0400 0x100>;
+			altr,ecc-parent = <&gmac0>;
+			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>'
+		};
+
+		nand-buf-ecc@ff8c8000 {
+			compatible = "altr,socfpga-s10-nand-ecc";
+			reg = <0xff8c8000 0x100>;
+			altr,ecc-parent = <&nand>;
+			interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		nand-rd-ecc@ff8c8400 {
+			compatible = "altr,socfpga-s10-nand-ecc";
+			reg = <0xff8c8400 0x100>;
+			altr,ecc-parent = <&nand>;
+			interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		nand-wr-ecc@ff8c8800 {
+			compatible = "altr,socfpga-s10-nand-ecc";
+			reg = <0xff8c8800 0x100>;
+			altr,ecc-parent = <&nand>;
+			interrupts = <12 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		dma-ecc@ff8c9000 {
+			compatible = "altr,socfpga-s10-dma-ecc";
+			reg = <0xff8c9000 0x100>;
+			altr,ecc-parent = <&pdma>;
+			interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
+
+		usb0-ecc@ff8c4000 {
+			compatible = "altr,socfpga-s10-usb-ecc";
+			reg = <0xff8c4000 0x100>;
+			altr,ecc-parent = <&usb0>;
+			interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		sdmmc-ecc@ff8c8c00 {
+			compatible = "altr,socfpga-s10-sdmmc-ecc";
+			reg = <0xff8c8c00 0x100>;
+			altr,ecc-parent = <&mmc>;
+			interrupts = <14 IRQ_TYPE_LEVEL_HIGH>,
+				     <15 IRQ_TYPE_LEVEL_HIGH>;
+		};
 	};
-- 
2.7.4

[PATCHv2 3/5] EDAC, altera: Skip DB IRQ for Stratix10

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Stratix10 Double Bit errors are configured as SErrors
so skip the Double Bit IRQ initialization if Stratix10.

Since all the ECC peripherals are handled in this routine,
the machine compatible device tree test is used here
instead of multiple ECC block device tree compatible
tests.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2 No change. Add explanation for machine compatible test
   in commit description.
---
 drivers/edac/altera_edac.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 3127e927bcec..a92259d8afdc 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1927,20 +1927,25 @@ static int altr_edac_a10_device_add(struct altr_arria10_edac *edac,
 		goto err_release_group1;
 	}
 
-	altdev->db_irq = irq_of_parse_and_map(np, 1);
-	if (!altdev->db_irq) {
-		edac_printk(KERN_ERR, EDAC_DEVICE, "Error allocating DBIRQ\n");
-		rc = -ENODEV;
-		goto err_release_group1;
-	}
-	rc = devm_request_irq(edac->dev, altdev->db_irq, prv->ecc_irq_handler,
-			      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
-			      ecc_name, altdev);
-	if (rc) {
-		edac_printk(KERN_ERR, EDAC_DEVICE, "No DBERR IRQ resource\n");
-		goto err_release_group1;
+	/* Arria10 has double bit error IRQs. Stratix10 uses SErrors */
+	if (socfpga_is_a10()) {
+		altdev->db_irq = irq_of_parse_and_map(np, 1);
+		if (!altdev->db_irq) {
+			edac_printk(KERN_ERR, EDAC_DEVICE,
+				    "Error allocating DBIRQ\n");
+			rc = -ENODEV;
+			goto err_release_group1;
+		}
+		rc = devm_request_irq(edac->dev, altdev->db_irq,
+				      prv->ecc_irq_handler,
+				      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
+				      ecc_name, altdev);
+		if (rc) {
+			edac_printk(KERN_ERR, EDAC_DEVICE,
+				    "No DBERR IRQ resource\n");
+			goto err_release_group1;
+		}
 	}
-
 	rc = edac_device_add_device(dci);
 	if (rc) {
 		dev_err(edac->dev, "edac_device_add_device failed\n");
-- 
2.7.4

[PATCHv2 4/5] arm64: dts: stratix10: Use new Stratix10 EDAC bindings

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Use the new Stratix10 binding format for EDAC nodes.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2 No change
---
 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 25 ++++++++++++-----------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
index 8253a1a9e985..de6f0507333d 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
@@ -479,11 +479,12 @@
 		};
 
 		eccmgr {
-			compatible = "altr,socfpga-a10-ecc-manager";
+			compatible = "altr,socfpga-s10-ecc-manager",
+				     "altr,socfpga-a10-ecc-manager";
 			altr,sysmgr-syscon = <&sysmgr>;
 			#address-cells = <1>;
 			#size-cells = <1>;
-			interrupts = <0 15 4>, <0 95 4>;
+			interrupts = <0 15 4>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
 			ranges;
@@ -491,31 +492,31 @@
 			sdramedac {
 				compatible = "altr,sdram-edac-s10";
 				altr,sdr-syscon = <&sdr>;
-				interrupts = <16 4>, <48 4>;
+				interrupts = <16 4>;
 			};
 
 			usb0-ecc@ff8c4000 {
-				compatible = "altr,socfpga-usb-ecc";
+				compatible = "altr,socfpga-s10-usb-ecc",
+					     "altr,socfpga-usb-ecc";
 				reg = <0xff8c4000 0x100>;
 				altr,ecc-parent = <&usb0>;
-				interrupts = <2 4>,
-					     <34 4>;
+				interrupts = <2 4>;
 			};
 
 			emac0-rx-ecc@ff8c0000 {
-				compatible = "altr,socfpga-eth-mac-ecc";
+				compatible = "altr,socfpga-s10-eth-mac-ecc",
+					     "altr,socfpga-eth-mac-ecc";
 				reg = <0xff8c0000 0x100>;
 				altr,ecc-parent = <&gmac0>;
-				interrupts = <4 4>,
-					     <36 4>;
+				interrupts = <4 4>;
 			};
 
 			emac0-tx-ecc@ff8c0400 {
-				compatible = "altr,socfpga-eth-mac-ecc";
+				compatible = "altr,socfpga-s10-eth-mac-ecc",
+					     "altr,socfpga-eth-mac-ecc";
 				reg = <0xff8c0400 0x100>;
 				altr,ecc-parent = <&gmac0>;
-				interrupts = <5 4>,
-					     <37 4>;
+				interrupts = <5 4>;
 			};
 
 		};
-- 
2.7.4

[PATCHv2 5/5] EDAC, altera: Remove Stratix10 Machine compatible check

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Replace the Stratix10 Machine compatible check with
specific ECC block compatible tests.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2 New patch
---
 drivers/edac/altera_edac.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index a92259d8afdc..3ff222a0c852 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1013,11 +1013,6 @@ static int socfpga_is_a10(void)
 	return of_machine_is_compatible("altr,socfpga-arria10");
 }
 
-static int socfpga_is_s10(void)
-{
-	return of_machine_is_compatible("altr,socfpga-stratix10");
-}
-
 static __init int __maybe_unused
 altr_init_a10_ecc_block(struct device_node *np, u32 irq_mask,
 			u32 ecc_ctrl_en_mask, bool dual_port)
@@ -1122,13 +1117,14 @@ static int __init __maybe_unused altr_init_a10_ecc_device_type(char *compat)
 	int irq;
 	struct device_node *child, *np;
 
-	if (!socfpga_is_a10() && !socfpga_is_s10())
-		return -ENODEV;
-
 	np = of_find_compatible_node(NULL, NULL,
 				     "altr,socfpga-a10-ecc-manager");
 	if (!np) {
-		edac_printk(KERN_ERR, EDAC_DEVICE, "ECC Manager not found\n");
+		/* Error only valid for Arria10 and Stratix10 */
+		if (!of_find_compatible_node(NULL, NULL,
+					     "altr,socfpga-ecc-manager"))
+			edac_printk(KERN_ERR, EDAC_DEVICE,
+				    "ECC Manager not found\n");
 		return -ENODEV;
 	}
 
@@ -1644,12 +1640,8 @@ static int __init socfpga_init_sdmmc_ecc(void)
 	int rc = -ENODEV;
 	struct device_node *child;
 
-	if (!socfpga_is_a10() && !socfpga_is_s10())
-		return -ENODEV;
-
 	child = of_find_compatible_node(NULL, NULL, "altr,socfpga-sdmmc-ecc");
 	if (!child) {
-		edac_printk(KERN_WARNING, EDAC_DEVICE, "SDMMC node not found\n");
 		return -ENODEV;
 	}
 
-- 
2.7.4

Re: [PATCHv2 1/5] Documentation: dt: edac: Fix Stratix10 IRQ bindings

[Rob Herring]

On Wed, Feb 27, 2019 at 11:27:21AM -0600, thor.thayer@linux.intel.com wrote:
> From: Thor Thayer <thor.thayer@linux.intel.com>
> 
> Fix Stratix10 ECC bindings to specify only the single
> bit error. On Stratix10 double bit errors are handled
> as SErrors instead of interrupts.
> Indicate the differences between the ARM64 and ARM32
> EDAC architecture in the bindings.
> 
> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
> ---
> v2 No change
> ---
>  .../devicetree/bindings/edac/socfpga-eccmgr.txt    | 23 +++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
> index 5626560a6cfd..a0ac50e15912 100644
> --- a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
> +++ b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
> @@ -236,33 +236,42 @@ Stratix10 SoCFPGA ECC Manager
>  The Stratix10 SoC ECC Manager handles the IRQs for each peripheral
>  in a shared register similar to the Arria10. However, ECC requires
>  access to registers that can only be read from Secure Monitor with
> -SMC calls. Therefore the device tree is slightly different.
> +SMC calls. Therefore the device tree is slightly different. Note that
> +only 1 interrupt is sent because the double bit errors are treated as
> +SErrors instead of IRQ.
>  
>  Required Properties:
>  - compatible : Should be "altr,socfpga-s10-ecc-manager"
> -- interrupts : Should be single bit error interrupt, then double bit error
> -	interrupt.
> +- altr,sysgr-syscon : phandle to Stratix10 System Manager Block
> +	              containing the ECC manager registers.

Seems this was already in use, but why not just make this node a child 
of the System Manager Block and remove this phandle?

> +- interrupts : Should be single bit error interrupt.
>  - interrupt-controller : boolean indicator that ECC Manager is an interrupt controller
>  - #interrupt-cells : must be set to 2.
> +- #address-cells: must be 1
> +- #size-cells: must be 1
> +- ranges : standard definition, should translate from local addresses
>  
>  Subcomponents:
>  
>  SDRAM ECC
>  Required Properties:
>  - compatible : Should be "altr,sdram-edac-s10"
> -- interrupts : Should be single bit error interrupt, then double bit error
> -	interrupt, in this order.
> +- interrupts : Should be single bit error interrupt.
>  
>  Example:
>  
>  	eccmgr {
>  		compatible = "altr,socfpga-s10-ecc-manager";
> -		interrupts = <0 15 4>, <0 95 4>;
> +		altr,sysmgr-syscon = <&sysmgr>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		interrupts = <0 15 4>;
>  		interrupt-controller;
>  		#interrupt-cells = <2>;
> +		ranges;
>  
>  		sdramedac {
>  			compatible = "altr,sdram-edac-s10";
> -			interrupts = <16 4>, <48 4>;
> +			interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
>  		};
>  	};
> -- 
> 2.7.4
> 

Re: [PATCHv2 2/5] Documentation: dt: edac: Add Stratix10 Peripheral bindings

[Rob Herring]

On Wed, Feb 27, 2019 at 11:27:22AM -0600, thor.thayer@linux.intel.com wrote:
> From: Thor Thayer <thor.thayer@linux.intel.com>
> 
> Add peripheral bindings for Stratix10 EDAC to capture
> the differences between the ARM64 and ARM32 architecture.

What's the difference? Sounds like 2 different chips, so Stratix10 or 
s10 is not specific enough perhaps.

> 
> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
> ---
> v2 No change
> ---
>  .../devicetree/bindings/edac/socfpga-eccmgr.txt    | 106 +++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
> index a0ac50e15912..a0fa80c53d2a 100644
> --- a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
> +++ b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
> @@ -258,6 +258,49 @@ Required Properties:
>  - compatible : Should be "altr,sdram-edac-s10"
>  - interrupts : Should be single bit error interrupt.
>  
> +On-Chip RAM ECC
> +Required Properties:
> +- compatible      : Should be "altr,socfpga-s10-ocram-ecc"
> +- reg             : Address and size for ECC block registers.
> +- altr,ecc-parent : phandle to parent OCRAM node.
> +- interrupts      : Should be single bit error interrupt.
> +
> +Ethernet FIFO ECC
> +Required Properties:
> +- compatible      : Should be "altr,socfpga-s10-eth-mac-ecc"
> +- reg             : Address and size for ECC block registers.
> +- altr,ecc-parent : phandle to parent Ethernet node.
> +- interrupts      : Should be single bit error interrupt.
> +
> +NAND FIFO ECC
> +Required Properties:
> +- compatible      : Should be "altr,socfpga-s10-nand-ecc"
> +- reg             : Address and size for ECC block registers.
> +- altr,ecc-parent : phandle to parent NAND node.
> +- interrupts      : Should be single bit error interrupt.
> +
> +DMA FIFO ECC
> +Required Properties:
> +- compatible      : Should be "altr,socfpga-s10-dma-ecc"
> +- reg             : Address and size for ECC block registers.
> +- altr,ecc-parent : phandle to parent DMA node.
> +- interrupts      : Should be single bit error interrupt.
> +
> +USB FIFO ECC
> +Required Properties:
> +- compatible      : Should be "altr,socfpga-s10-usb-ecc"
> +- reg             : Address and size for ECC block registers.
> +- altr,ecc-parent : phandle to parent USB node.
> +- interrupts      : Should be single bit error interrupt.
> +
> +SDMMC FIFO ECC
> +Required Properties:
> +- compatible      : Should be "altr,socfpga-s10-sdmmc-ecc"
> +- reg             : Address and size for ECC block registers.
> +- altr,ecc-parent : phandle to parent SD/MMC node.
> +- interrupts      : Should be single bit error interrupt for port A
> +		    and then single bit error interrupt for port B.
> +
>  Example:
>  
>  	eccmgr {
> @@ -274,4 +317,67 @@ Example:
>  			compatible = "altr,sdram-edac-s10";
>  			interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
>  		};
> +
> +		ocram-ecc@ff8cc000 {
> +			compatible = "altr,socfpga-s10-ocram-ecc";
> +			reg = <ff8cc000 0x100>;
> +			altr,ecc-parent = <&ocram>;
> +			interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		emac0-rx-ecc@ff8c0000 {
> +			compatible = "altr,socfpga-s10-eth-mac-ecc";
> +			reg = <0xff8c0000 0x100>;
> +			altr,ecc-parent = <&gmac0>;
> +			interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		emac0-tx-ecc@ff8c0400 {
> +			compatible = "altr,socfpga-s10-eth-mac-ecc";
> +			reg = <0xff8c0400 0x100>;
> +			altr,ecc-parent = <&gmac0>;
> +			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>'
> +		};
> +
> +		nand-buf-ecc@ff8c8000 {
> +			compatible = "altr,socfpga-s10-nand-ecc";
> +			reg = <0xff8c8000 0x100>;
> +			altr,ecc-parent = <&nand>;
> +			interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		nand-rd-ecc@ff8c8400 {
> +			compatible = "altr,socfpga-s10-nand-ecc";
> +			reg = <0xff8c8400 0x100>;
> +			altr,ecc-parent = <&nand>;
> +			interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		nand-wr-ecc@ff8c8800 {
> +			compatible = "altr,socfpga-s10-nand-ecc";
> +			reg = <0xff8c8800 0x100>;
> +			altr,ecc-parent = <&nand>;
> +			interrupts = <12 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		dma-ecc@ff8c9000 {
> +			compatible = "altr,socfpga-s10-dma-ecc";
> +			reg = <0xff8c9000 0x100>;
> +			altr,ecc-parent = <&pdma>;
> +			interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
> +
> +		usb0-ecc@ff8c4000 {
> +			compatible = "altr,socfpga-s10-usb-ecc";
> +			reg = <0xff8c4000 0x100>;
> +			altr,ecc-parent = <&usb0>;
> +			interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		sdmmc-ecc@ff8c8c00 {
> +			compatible = "altr,socfpga-s10-sdmmc-ecc";
> +			reg = <0xff8c8c00 0x100>;
> +			altr,ecc-parent = <&mmc>;
> +			interrupts = <14 IRQ_TYPE_LEVEL_HIGH>,
> +				     <15 IRQ_TYPE_LEVEL_HIGH>;
> +		};
>  	};
> -- 
> 2.7.4
> 

Re: [PATCHv2 1/5] Documentation: dt: edac: Fix Stratix10 IRQ bindings

[Thor Thayer]

Hi Rob,

On 3/12/19 11:00 AM, Rob Herring wrote:
> On Wed, Feb 27, 2019 at 11:27:21AM -0600, thor.thayer@linux.intel.com wrote:
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> Fix Stratix10 ECC bindings to specify only the single
>> bit error. On Stratix10 double bit errors are handled
>> as SErrors instead of interrupts.
>> Indicate the differences between the ARM64 and ARM32
>> EDAC architecture in the bindings.
>>
>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
>> ---
>> v2 No change
>> ---
>>   .../devicetree/bindings/edac/socfpga-eccmgr.txt    | 23 +++++++++++++++-------
>>   1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
>> index 5626560a6cfd..a0ac50e15912 100644
>> --- a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
>> +++ b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
>> @@ -236,33 +236,42 @@ Stratix10 SoCFPGA ECC Manager
>>   The Stratix10 SoC ECC Manager handles the IRQs for each peripheral
>>   in a shared register similar to the Arria10. However, ECC requires
>>   access to registers that can only be read from Secure Monitor with
>> -SMC calls. Therefore the device tree is slightly different.
>> +SMC calls. Therefore the device tree is slightly different. Note that
>> +only 1 interrupt is sent because the double bit errors are treated as
>> +SErrors instead of IRQ.
>>   
>>   Required Properties:
>>   - compatible : Should be "altr,socfpga-s10-ecc-manager"
>> -- interrupts : Should be single bit error interrupt, then double bit error
>> -	interrupt.
>> +- altr,sysgr-syscon : phandle to Stratix10 System Manager Block
>> +	              containing the ECC manager registers.
> 
> Seems this was already in use, but why not just make this node a child
> of the System Manager Block and remove this phandle?
> 
Yes, this was already in use but I'm trying to fix that oversight with 
this patch.

The System Manager is a collection of registers used by different 
peripherals including EMAC and ECC.

I view ECC Manager as a separate entity as is the Ethernet MAC which is 
why I have it separate. Using the phandle also follows the convention 
established with the Arria10 ECC Manager.

Thanks for the comments and for reviewing!

Thor

>> +- interrupts : Should be single bit error interrupt.
>>   - interrupt-controller : boolean indicator that ECC Manager is an interrupt controller
>>   - #interrupt-cells : must be set to 2.
>> +- #address-cells: must be 1
>> +- #size-cells: must be 1
>> +- ranges : standard definition, should translate from local addresses
>>   
>>   Subcomponents:
>>   
>>   SDRAM ECC
>>   Required Properties:
>>   - compatible : Should be "altr,sdram-edac-s10"
>> -- interrupts : Should be single bit error interrupt, then double bit error
>> -	interrupt, in this order.
>> +- interrupts : Should be single bit error interrupt.
>>   
>>   Example:
>>   
>>   	eccmgr {
>>   		compatible = "altr,socfpga-s10-ecc-manager";
>> -		interrupts = <0 15 4>, <0 95 4>;
>> +		altr,sysmgr-syscon = <&sysmgr>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		interrupts = <0 15 4>;
>>   		interrupt-controller;
>>   		#interrupt-cells = <2>;
>> +		ranges;
>>   
>>   		sdramedac {
>>   			compatible = "altr,sdram-edac-s10";
>> -			interrupts = <16 4>, <48 4>;
>> +			interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
>>   		};
>>   	};
>> -- 
>> 2.7.4
>>
> 

Re: [PATCHv2 2/5] Documentation: dt: edac: Add Stratix10 Peripheral bindings

[Thor Thayer]

Hi Rob,

On 3/12/19 11:04 AM, Rob Herring wrote:
> On Wed, Feb 27, 2019 at 11:27:22AM -0600, thor.thayer@linux.intel.com wrote:
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> Add peripheral bindings for Stratix10 EDAC to capture
>> the differences between the ARM64 and ARM32 architecture.
> 
> What's the difference? Sounds like 2 different chips, so Stratix10 or
> s10 is not specific enough perhaps.
> 

Yes, our ARM32 chips are the Cyclone5 and Arria10. The Stratix10 is 
ARM64 and I'm using S10 as shorthand for the Stratix10.

The ECC blocks are very similar between Arria10 and Stratix10 but there 
are differences as a result of the ARM32 vs ARM64 architecture 
differences. The major difference is how Double Bit Errors are handled. 
In the ARM32, the DBE is mapped to an IRQ. On ARM64, the DBE is mapped 
to a SError.

I had started out re-using the Arria10 bindings for Stratix10 since they 
were very similar. Dinh pointed out that having separate bindings for 
ARM64 would allow separation between the architectures and make future 
changes easier.

I'm unclear on the comment about being specific enough. Are you 
suggesting that I use arm64 in the binding name instead of s10? Or is 
there a better naming convention I should follow?

Thanks for your comments and for reviewing!

Thor
>>
>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
>> ---
>> v2 No change
>> ---
>>   .../devicetree/bindings/edac/socfpga-eccmgr.txt    | 106 +++++++++++++++++++++
>>   1 file changed, 106 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
>> index a0ac50e15912..a0fa80c53d2a 100644
>> --- a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
>> +++ b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
>> @@ -258,6 +258,49 @@ Required Properties:
>>   - compatible : Should be "altr,sdram-edac-s10"
>>   - interrupts : Should be single bit error interrupt.
>>   
>> +On-Chip RAM ECC
>> +Required Properties:
>> +- compatible      : Should be "altr,socfpga-s10-ocram-ecc"
>> +- reg             : Address and size for ECC block registers.
>> +- altr,ecc-parent : phandle to parent OCRAM node.
>> +- interrupts      : Should be single bit error interrupt.
>> +
>> +Ethernet FIFO ECC
>> +Required Properties:
>> +- compatible      : Should be "altr,socfpga-s10-eth-mac-ecc"
>> +- reg             : Address and size for ECC block registers.
>> +- altr,ecc-parent : phandle to parent Ethernet node.
>> +- interrupts      : Should be single bit error interrupt.
>> +
>> +NAND FIFO ECC
>> +Required Properties:
>> +- compatible      : Should be "altr,socfpga-s10-nand-ecc"
>> +- reg             : Address and size for ECC block registers.
>> +- altr,ecc-parent : phandle to parent NAND node.
>> +- interrupts      : Should be single bit error interrupt.
>> +
>> +DMA FIFO ECC
>> +Required Properties:
>> +- compatible      : Should be "altr,socfpga-s10-dma-ecc"
>> +- reg             : Address and size for ECC block registers.
>> +- altr,ecc-parent : phandle to parent DMA node.
>> +- interrupts      : Should be single bit error interrupt.
>> +
>> +USB FIFO ECC
>> +Required Properties:
>> +- compatible      : Should be "altr,socfpga-s10-usb-ecc"
>> +- reg             : Address and size for ECC block registers.
>> +- altr,ecc-parent : phandle to parent USB node.
>> +- interrupts      : Should be single bit error interrupt.
>> +
>> +SDMMC FIFO ECC
>> +Required Properties:
>> +- compatible      : Should be "altr,socfpga-s10-sdmmc-ecc"
>> +- reg             : Address and size for ECC block registers.
>> +- altr,ecc-parent : phandle to parent SD/MMC node.
>> +- interrupts      : Should be single bit error interrupt for port A
>> +		    and then single bit error interrupt for port B.
>> +
>>   Example:
>>   
>>   	eccmgr {
>> @@ -274,4 +317,67 @@ Example:
>>   			compatible = "altr,sdram-edac-s10";
>>   			interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
>>   		};
>> +
>> +		ocram-ecc@ff8cc000 {
>> +			compatible = "altr,socfpga-s10-ocram-ecc";
>> +			reg = <ff8cc000 0x100>;
>> +			altr,ecc-parent = <&ocram>;
>> +			interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		emac0-rx-ecc@ff8c0000 {
>> +			compatible = "altr,socfpga-s10-eth-mac-ecc";
>> +			reg = <0xff8c0000 0x100>;
>> +			altr,ecc-parent = <&gmac0>;
>> +			interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		emac0-tx-ecc@ff8c0400 {
>> +			compatible = "altr,socfpga-s10-eth-mac-ecc";
>> +			reg = <0xff8c0400 0x100>;
>> +			altr,ecc-parent = <&gmac0>;
>> +			interrupts = <5 IRQ_TYPE_LEVEL_HIGH>'
>> +		};
>> +
>> +		nand-buf-ecc@ff8c8000 {
>> +			compatible = "altr,socfpga-s10-nand-ecc";
>> +			reg = <0xff8c8000 0x100>;
>> +			altr,ecc-parent = <&nand>;
>> +			interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		nand-rd-ecc@ff8c8400 {
>> +			compatible = "altr,socfpga-s10-nand-ecc";
>> +			reg = <0xff8c8400 0x100>;
>> +			altr,ecc-parent = <&nand>;
>> +			interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		nand-wr-ecc@ff8c8800 {
>> +			compatible = "altr,socfpga-s10-nand-ecc";
>> +			reg = <0xff8c8800 0x100>;
>> +			altr,ecc-parent = <&nand>;
>> +			interrupts = <12 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		dma-ecc@ff8c9000 {
>> +			compatible = "altr,socfpga-s10-dma-ecc";
>> +			reg = <0xff8c9000 0x100>;
>> +			altr,ecc-parent = <&pdma>;
>> +			interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +		usb0-ecc@ff8c4000 {
>> +			compatible = "altr,socfpga-s10-usb-ecc";
>> +			reg = <0xff8c4000 0x100>;
>> +			altr,ecc-parent = <&usb0>;
>> +			interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		sdmmc-ecc@ff8c8c00 {
>> +			compatible = "altr,socfpga-s10-sdmmc-ecc";
>> +			reg = <0xff8c8c00 0x100>;
>> +			altr,ecc-parent = <&mmc>;
>> +			interrupts = <14 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <15 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>>   	};
>> -- 
>> 2.7.4
>>
> 

Re: [PATCHv2 2/5] Documentation: dt: edac: Add Stratix10 Peripheral bindings

[Rob Herring]

On Tue, Mar 12, 2019 at 2:28 PM Thor Thayer <thor.thayer@linux.intel.com> wrote:
>
> Hi Rob,
>
> On 3/12/19 11:04 AM, Rob Herring wrote:
> > On Wed, Feb 27, 2019 at 11:27:22AM -0600, thor.thayer@linux.intel.com wrote:
> >> From: Thor Thayer <thor.thayer@linux.intel.com>
> >>
> >> Add peripheral bindings for Stratix10 EDAC to capture
> >> the differences between the ARM64 and ARM32 architecture.
> >
> > What's the difference? Sounds like 2 different chips, so Stratix10 or
> > s10 is not specific enough perhaps.
> >
>
> Yes, our ARM32 chips are the Cyclone5 and Arria10. The Stratix10 is
> ARM64 and I'm using S10 as shorthand for the Stratix10.

So it's really just differences between one chip and another... ARM32
vs 64 really has nothing to do with that.

>
> The ECC blocks are very similar between Arria10 and Stratix10 but there
> are differences as a result of the ARM32 vs ARM64 architecture
> differences. The major difference is how Double Bit Errors are handled.
> In the ARM32, the DBE is mapped to an IRQ. On ARM64, the DBE is mapped
> to a SError.

Okay, I guess that's why arm64 matters...

> I had started out re-using the Arria10 bindings for Stratix10 since they
> were very similar. Dinh pointed out that having separate bindings for
> ARM64 would allow separation between the architectures and make future
> changes easier.
>
> I'm unclear on the comment about being specific enough. Are you
> suggesting that I use arm64 in the binding name instead of s10? Or is
> there a better naming convention I should follow?

NM, it was me that was confused. It was that Stratix10 was already
mentioned in the doc that confused me.

Rob

Re: [PATCHv2 1/5] Documentation: dt: edac: Fix Stratix10 IRQ bindings

[Rob Herring]

On Tue, Mar 12, 2019 at 2:13 PM Thor Thayer <thor.thayer@linux.intel.com> wrote:
>
> Hi Rob,
>
> On 3/12/19 11:00 AM, Rob Herring wrote:
> > On Wed, Feb 27, 2019 at 11:27:21AM -0600, thor.thayer@linux.intel.com wrote:
> >> From: Thor Thayer <thor.thayer@linux.intel.com>
> >>
> >> Fix Stratix10 ECC bindings to specify only the single
> >> bit error. On Stratix10 double bit errors are handled
> >> as SErrors instead of interrupts.
> >> Indicate the differences between the ARM64 and ARM32
> >> EDAC architecture in the bindings.
> >>
> >> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
> >> ---
> >> v2 No change
> >> ---
> >>   .../devicetree/bindings/edac/socfpga-eccmgr.txt    | 23 +++++++++++++++-------
> >>   1 file changed, 16 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
> >> index 5626560a6cfd..a0ac50e15912 100644
> >> --- a/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
> >> +++ b/Documentation/devicetree/bindings/edac/socfpga-eccmgr.txt
> >> @@ -236,33 +236,42 @@ Stratix10 SoCFPGA ECC Manager
> >>   The Stratix10 SoC ECC Manager handles the IRQs for each peripheral
> >>   in a shared register similar to the Arria10. However, ECC requires
> >>   access to registers that can only be read from Secure Monitor with
> >> -SMC calls. Therefore the device tree is slightly different.
> >> +SMC calls. Therefore the device tree is slightly different. Note that
> >> +only 1 interrupt is sent because the double bit errors are treated as
> >> +SErrors instead of IRQ.
> >>
> >>   Required Properties:
> >>   - compatible : Should be "altr,socfpga-s10-ecc-manager"
> >> -- interrupts : Should be single bit error interrupt, then double bit error
> >> -    interrupt.
> >> +- altr,sysgr-syscon : phandle to Stratix10 System Manager Block
> >> +                  containing the ECC manager registers.
> >
> > Seems this was already in use, but why not just make this node a child
> > of the System Manager Block and remove this phandle?
> >
> Yes, this was already in use but I'm trying to fix that oversight with
> this patch.
>
> The System Manager is a collection of registers used by different
> peripherals including EMAC and ECC.

The EMAC has its own registers too, right? But the ECC does not it seems.

> I view ECC Manager as a separate entity as is the Ethernet MAC which is
> why I have it separate. Using the phandle also follows the convention
> established with the Arria10 ECC Manager.

I guess this ship has sailed, so:

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCHv2 2/5] Documentation: dt: edac: Add Stratix10 Peripheral bindings

[Thor Thayer]

Hi Rob,

On 3/13/19 2:20 PM, Rob Herring wrote:
> On Tue, Mar 12, 2019 at 2:28 PM Thor Thayer <thor.thayer@linux.intel.com> wrote:
>>
>> Hi Rob,
>>
>> On 3/12/19 11:04 AM, Rob Herring wrote:
>>> On Wed, Feb 27, 2019 at 11:27:22AM -0600, thor.thayer@linux.intel.com wrote:
>>>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>>>
>>>> Add peripheral bindings for Stratix10 EDAC to capture
>>>> the differences between the ARM64 and ARM32 architecture.
>>>
>>> What's the difference? Sounds like 2 different chips, so Stratix10 or
>>> s10 is not specific enough perhaps.
>>>
>>
>> Yes, our ARM32 chips are the Cyclone5 and Arria10. The Stratix10 is
>> ARM64 and I'm using S10 as shorthand for the Stratix10.
> 
> So it's really just differences between one chip and another... ARM32
> vs 64 really has nothing to do with that.
> 
>>
>> The ECC blocks are very similar between Arria10 and Stratix10 but there
>> are differences as a result of the ARM32 vs ARM64 architecture
>> differences. The major difference is how Double Bit Errors are handled.
>> In the ARM32, the DBE is mapped to an IRQ. On ARM64, the DBE is mapped
>> to a SError.
> 
> Okay, I guess that's why arm64 matters...
> 
>> I had started out re-using the Arria10 bindings for Stratix10 since they
>> were very similar. Dinh pointed out that having separate bindings for
>> ARM64 would allow separation between the architectures and make future
>> changes easier.
>>
>> I'm unclear on the comment about being specific enough. Are you
>> suggesting that I use arm64 in the binding name instead of s10? Or is
>> there a better naming convention I should follow?
> 
> NM, it was me that was confused. It was that Stratix10 was already
> mentioned in the doc that confused me.
> 
> Rob

I can reword this to make it clearer. Do you have any additional 
suggestions for clarification aside from ARM64 vs ARM32 IRQ handling as 
we discuss above that you would need to ack this patch?

Thanks,

Thor