Add SDRAM ECC support for Stratix10

PATCH Superseded thor.thayer at linux.intel.com 4 participants 12 messages Apr 24, 2018 – Apr 26, 2018

Subsystems (4)

Device tree bindings EDAC-core Arm/socfpga architecture Arm/socfpga EDAC support

lore

Share & tools

lore

thor.thayer at linux.intel.com

author

2018-04-24 18:35 UTC #0 lore raw

cover Add SDRAM ECC support for Stratix10

From: Thor Thayer <[email protected]>

The Intel Stratix10 platform is an ARM64 but still has many register definitions that are similar to the Arria10. One significant difference is the Stratix10 hypervisor which requires handling registers that may be shared by guest OSes at a different exception level. Register access is through an ARM SMC call. Currently, SMC handling is implemented in U-Boot.

Thor Thayer (3):
Documentation: dt: socfpga: Add Stratix10 ECC Manager binding
edac: altera: Add support for Stratix10 SDRAM EDAC
arm64: dts: stratix10: add sdram ecc

 .../bindings/arm/altera/socfpga-eccmgr.txt         |  47 +++
 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi  |  17 +
 drivers/edac/Kconfig                               |   2 +-
 drivers/edac/altera_edac.c                         | 459 +++++++++++++++++++++
 drivers/edac/altera_edac.h                         | 114 +++++
 5 files changed, 638 insertions(+), 1 deletion(-)

Signature · 1 line

2.7.4

thor.thayer at linux.intel.com

author

2018-04-24 18:35 UTC #1 lore raw

PATCH 1/3 Documentation: dt: socfpga: Add Stratix10 ECC Manager binding

From: Thor Thayer <[email protected]>

Add the device tree bindings needed to support the Stratix10 ECC Manager and SDRAM ECC to the existing bindings.

Signed-off-by Thor Thayer <[email protected]>

---
 .../bindings/arm/altera/socfpga-eccmgr.txt         | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)

Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +47 -0

index 4a1714f96bab..fe48ad293a24 100644
@@ -231,3 +231,50 @@ Example:
 				     <48 IRQ_TYPE_LEVEL_HIGH>;
 		};
 	};
+
+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 in EL3 with SMC calls.
+Therefore the device tree is slightly different.
+
+Required Properties:
+- compatible : Should be "altr,socfpga-s10-ecc-manager"
+- altr,sysgr-syscon : phandle to Stratix10 System Manager Block
+	containing the ECC manager registers.
+- #address-cells: must be 1
+- #size-cells: must be 1
+- interrupts : Should be single bit error interrupt, then double bit error
+	interrupt.
+- interrupt-controller : boolean indicator that ECC Manager is an interrupt controller
+- #interrupt-cells : must be set to 2.
+- ranges : standard definition, should translate from local addresses
+
+Subcomponents:
+
+SDRAM ECC
+Required Properties:
+- compatible : Should be "altr,sdram-edac-s10"
+- reg : Address and size for ECC error interrupt clear registers.
+- interrupts : Should be single bit error interrupt, then double bit error
+	interrupt, in this order.
+
+Example:
+
+	eccmgr: eccmgr at ffd12000 {
+		compatible = "altr,socfpga-s10-ecc-manager";
+		altr,sysmgr-syscon = <&sysmgr>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		interrupts = <0 15 4>, <0 95 4>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		ranges;
+
+		sdramedac at f8011100 {
+			compatible = "altr,sdram-edac-s10";
+			reg = <0xf8011100 0xC0>;
+			interrupts = <16 4>, <48 4>;
+		};
+	};
+

Signature · 1 line

2.7.4

Dinh Nguyen

maintainer

2018-04-26 02:16 UTC #2 lore raw

PATCH 1/3 Documentation: dt: socfpga: Add Stratix10 ECC Manager binding

Quoted 17 lines · thor.thayer at linux.intel.com

From: Thor Thayer <[email protected]>
Add the device tree bindings needed to support the Stratix10 ECC Manager and SDRAM ECC to the existing bindings.
Signed-off-by Thor Thayer <[email protected]>
---
 .../bindings/arm/altera/socfpga-eccmgr.txt         | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +0 -0
index 4a1714f96bab..fe48ad293a24 100644
@@ -231,3 +231,50 @@ Example:
 				     <48 IRQ_TYPE_LEVEL_HIGH>;
 		};

Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt Example: · line 242

Quoted 10 lines

Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +9 -0

 	};
+
+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 in EL3 with SMC calls.
+Therefore the device tree is slightly different.
+
+Required Properties:
+- compatible : Should be "altr,socfpga-s10-ecc-manager"

Altera technically doesn't exist anymore, should this be "intel,stratix10-ecc-manager"?

Dinh

Thor Thayer

author

2018-04-26 14:43 UTC #3 lore raw

PATCH 1/3 Documentation: dt: socfpga: Add Stratix10 ECC Manager binding

Quoted 25 lines · Dinh Nguyen

Quoted 22 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
Add the device tree bindings needed to support the Stratix10 ECC Manager and SDRAM ECC to the existing bindings.
Signed-off-by Thor Thayer <[email protected]>
---
  .../bindings/arm/altera/socfpga-eccmgr.txt         | 47 ++++++++++++++++++++++
  1 file changed, 47 insertions(+)
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +4 -0
index 4a1714f96bab..fe48ad293a24 100644
@@ -231,3 +231,50 @@ Example:
  				     <48 IRQ_TYPE_LEVEL_HIGH>;
  		};
  	};
+
+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

Quoted 9 lines

Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt Example: · line 242
Quoted 5 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +5 -0
+access to registers that can only be read in EL3 with SMC calls.
+Therefore the device tree is slightly different.
+
+Required Properties:
+- compatible : Should be "altr,socfpga-s10-ecc-manager"
Altera technically doesn't exist anymore, should this be "intel,stratix10-ecc-manager"?

I was trying to be consistent with the older names but I agree with your argument. I will change this. Thanks

Quoted 6 lines

Dinh
--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thor Thayer

author

2018-04-26 14:58 UTC #4 lore raw

PATCH 1/3 Documentation: dt: socfpga: Add Stratix10 ECC Manager binding

Quoted 38 lines · Thor Thayer

Quoted 28 lines · Dinh Nguyen
Quoted 25 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
Add the device tree bindings needed to support the Stratix10 ECC Manager and SDRAM ECC to the existing bindings.
Signed-off-by Thor Thayer <[email protected]>
---
? .../bindings/arm/altera/socfpga-eccmgr.txt???????? | 47
++++++++++++++++++++++
? 1 file changed, 47 insertions(+)
(quoted diff) +0 -0
a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt index 4a1714f96bab..fe48ad293a24 100644
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +4 -0
@@ -231,3 +231,50 @@ Example:
 ?????????????????????? <48 IRQ_TYPE_LEVEL_HIGH>;
 ????????? };
 ????? };
+
+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
Quoted 9 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt Example: · line 242
Quoted 5 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +5 -0
+access to registers that can only be read in EL3 with SMC calls.
+Therefore the device tree is slightly different.
+
+Required Properties:
+- compatible : Should be "altr,socfpga-s10-ecc-manager"
Altera technically doesn't exist anymore, should this be "intel,stratix10-ecc-manager"?

Quoted 3 lines

I was trying to be consistent with the older names but I agree with your argument. I will change this. Thanks

After looking at the Stratix10 device tree, there are only "altr," in there. For instance the top node is:

compatible = "altr,socfpga-stratix10";

and even the directory that the device tree is in is named "altera"

so I'll stick with the existing "altr," designation to be consistent.

Thanks,

Thor

Quoted 12 lines

Quoted 6 lines

Dinh

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at? http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at? http://vger.kernel.org/majordomo-info.html

Dinh Nguyen

maintainer

2018-04-26 15:01 UTC #5 lore raw

PATCH 1/3 Documentation: dt: socfpga: Add Stratix10 ECC Manager binding

Quoted 42 lines · Thor Thayer

Quoted 38 lines · Thor Thayer
Quoted 28 lines · Dinh Nguyen
Quoted 25 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
Add the device tree bindings needed to support the Stratix10 ECC Manager and SDRAM ECC to the existing bindings.
Signed-off-by Thor Thayer <[email protected]>
---
? .../bindings/arm/altera/socfpga-eccmgr.txt???????? | 47
++++++++++++++++++++++
? 1 file changed, 47 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt index 4a1714f96bab..fe48ad293a24 100644
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +4 -0
@@ -231,3 +231,50 @@ Example:
 ?????????????????????? <48 IRQ_TYPE_LEVEL_HIGH>;
 ????????? };
 ????? };
+
+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
Quoted 9 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt Example: · line 242
Quoted 5 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +5 -0
+access to registers that can only be read in EL3 with SMC calls.
+Therefore the device tree is slightly different.
+
+Required Properties:
+- compatible : Should be "altr,socfpga-s10-ecc-manager"
Altera technically doesn't exist anymore, should this be "intel,stratix10-ecc-manager"?
Quoted 3 lines
I was trying to be consistent with the older names but I agree with your argument. I will change this. Thanks

Quoted 9 lines

After looking at the Stratix10 device tree, there are only "altr," in there. For instance the top node is:
compatible = "altr,socfpga-stratix10";
and even the directory that the device tree is in is named "altera"
so I'll stick with the existing "altr," designation to be consistent.

That's fine and I don't have any strong inclination for 1 way or another. But it's not quite true that there are only "altr" in the dts file.

I did recently switch the clock manager to an intel designation. I upstreamed the original stratix10 DTS file back in 2015, before Altera was purchased by Intel.

My only argument is that if you're adding new support for devices, it should be an intel binding.

Dinh

Rob Herring

2018-04-26 15:06 UTC #6 lore raw

PATCH 1/3 Documentation: dt: socfpga: Add Stratix10 ECC Manager binding

Quoted 58 lines · Dinh Nguyen

Quoted 42 lines · Thor Thayer
Quoted 38 lines · Thor Thayer
Quoted 28 lines · Dinh Nguyen
Quoted 25 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
Add the device tree bindings needed to support the Stratix10 ECC Manager and SDRAM ECC to the existing bindings.
Signed-off-by Thor Thayer <[email protected]>
---
  .../bindings/arm/altera/socfpga-eccmgr.txt         | 47
++++++++++++++++++++++
  1 file changed, 47 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt index 4a1714f96bab..fe48ad293a24 100644
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +4 -0
@@ -231,3 +231,50 @@ Example:
                       <48 IRQ_TYPE_LEVEL_HIGH>;
          };
      };
+
+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
Quoted 9 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt Example: · line 242
Quoted 5 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt +5 -0
+access to registers that can only be read in EL3 with SMC calls.
+Therefore the device tree is slightly different.
+
+Required Properties:
+- compatible : Should be "altr,socfpga-s10-ecc-manager"
Altera technically doesn't exist anymore, should this be "intel,stratix10-ecc-manager"?
Quoted 3 lines
I was trying to be consistent with the older names but I agree with your argument. I will change this. Thanks
Quoted 9 lines
After looking at the Stratix10 device tree, there are only "altr," in there. For instance the top node is:
compatible = "altr,socfpga-stratix10";
and even the directory that the device tree is in is named "altera"
so I'll stick with the existing "altr," designation to be consistent.
That's fine and I don't have any strong inclination for 1 way or another. But it's not quite true that there are only "altr" in the dts file.

Quoted 6 lines

I did recently switch the clock manager to an intel designation. I upstreamed the original stratix10 DTS file back in 2015, before Altera was purchased by Intel.
My only argument is that if you're adding new support for devices, it should be an intel binding.

I think I would make the switch when you change it at the SoC level (for a new SoC). IOW, if the top level compatible is using 'intel', then use that for the device bindings otherwise stick with 'altr' for that SoC.

Rob

thor.thayer at linux.intel.com

author

2018-04-24 18:35 UTC #7 lore raw

PATCH 2/3 edac: altera: Add support for Stratix10 SDRAM EDAC

In reply to thor.thayer at linux.intel.com

From: Thor Thayer <[email protected]>

Add support for SDRAM ECC on the Stratix10 platform to the EDAC device driver. Although Stratix10 is very similar to the Arria10, hypervisor support for Stratix10 SDRAM ECC requires the use of SMC calls to a higher priority exception level to handle some register reads/writes.

Signed-off-by Thor Thayer <[email protected]>

---
 drivers/edac/Kconfig       |   2 +-
 drivers/edac/altera_edac.c | 459 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/edac/altera_edac.h | 114 +++++++++++
 3 files changed, 574 insertions(+), 1 deletion(-)

drivers/edac/Kconfig +1 -1

index 3c4017007647..3c66b02b2473 100644
@@ -379,7 +379,7 @@ config EDAC_THUNDERX
 
 config EDAC_ALTERA
 	bool "Altera SOCFPGA ECC"
-	depends on EDAC=y && ARCH_SOCFPGA
+	depends on EDAC=y && (ARCH_SOCFPGA || ARM64)
 	help
 	  Support for error detection and correction on the
 	  Altera SOCs. This must be selected for SDRAM ECC.

drivers/edac/altera_edac.c +459 -0

493 lines not on this page · raw message

drivers/edac/altera_edac.h +114 -0

148 lines not on this page · raw message

Signature · 1 line

2.7.4

Dinh Nguyen

maintainer

2018-04-26 02:14 UTC #8 lore raw

PATCH 2/3 edac: altera: Add support for Stratix10 SDRAM EDAC

Quoted 8 lines · thor.thayer at linux.intel.com

From: Thor Thayer <[email protected]>
Add support for SDRAM ECC on the Stratix10 platform to the EDAC device driver. Although Stratix10 is very similar to the Arria10, hypervisor support for Stratix10 SDRAM ECC requires the use of SMC calls to a higher priority exception level to handle some register reads/writes.

Quoted 5 lines

Signed-off-by Thor Thayer <[email protected]>

---
 drivers/edac/Kconfig       |   2 +-
 drivers/edac/altera_edac.c | 459 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/edac/altera_edac.h | 114 +++++++++++

Does it make sense to have the support for this in separate files?

drivers/edac/Kconfig config EDAC_THUNDERX · line 382

Quoted 12 lines

 3 files changed, 574 insertions(+), 1 deletion(-)

drivers/edac/Kconfig +1 -1

index 3c4017007647..3c66b02b2473 100644
@@ -379,7 +379,7 @@ config EDAC_THUNDERX
 
 config EDAC_ALTERA
 	bool "Altera SOCFPGA ECC"
-	depends on EDAC=y && ARCH_SOCFPGA
+	depends on EDAC=y && (ARCH_SOCFPGA || ARM64)

Should the ARM64 dependency just be ARCH_STRATIX10?

drivers/edac/altera_edac.c line 2

Quoted 10 lines

drivers/edac/Kconfig +0 -0

 	help
 	  Support for error detection and correction on the
 	  Altera SOCs. This must be selected for SDRAM ECC.

drivers/edac/altera_edac.c +1 -0

index 11d6419788c2..7ed885379719 100644
@@ -1,4 +1,5 @@
 /*
+ *  Copyright (C) 2017-2018, Intel Corporation

If you're updating the license header, would it make sense to convert to SPDX?

Dinh

Borislav Petkov

maintainer

2018-04-26 06:08 UTC #9 lore raw

PATCH 2/3 edac: altera: Add support for Stratix10 SDRAM EDAC

Quoted 1 line · Dinh Nguyen

Does it make sense to have the support for this in separate files?

No. Per vendor or per uarch/processor type, standalone EDAC drivers are fine. But not per the RAS capability of a functional unit.

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Thor Thayer

author

2018-04-26 14:42 UTC #10 lore raw

PATCH 2/3 edac: altera: Add support for Stratix10 SDRAM EDAC

In reply to Dinh Nguyen

Quoted 27 lines · Dinh Nguyen

Quoted 8 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
Add support for SDRAM ECC on the Stratix10 platform to the EDAC device driver. Although Stratix10 is very similar to the Arria10, hypervisor support for Stratix10 SDRAM ECC requires the use of SMC calls to a higher priority exception level to handle some register reads/writes.
Quoted 5 lines
Signed-off-by Thor Thayer <[email protected]>
---
  drivers/edac/Kconfig       |   2 +-
  drivers/edac/altera_edac.c | 459 +++++++++++++++++++++++++++++++++++++++++++++
  drivers/edac/altera_edac.h | 114 +++++++++++
Does it make sense to have the support for this in separate files?
Quoted 8 lines
  3 files changed, 574 insertions(+), 1 deletion(-)
drivers/edac/Kconfig +0 -0
index 3c4017007647..3c66b02b2473 100644
@@ -379,7 +379,7 @@ config EDAC_THUNDERX
 

Quoted 7 lines

drivers/edac/Kconfig config EDAC_THUNDERX · line 382
Quoted 4 lines
drivers/edac/Kconfig +1 -1
  config EDAC_ALTERA
  	bool "Altera SOCFPGA ECC"
-	depends on EDAC=y && ARCH_SOCFPGA
+	depends on EDAC=y && (ARCH_SOCFPGA || ARM64)
Should the ARM64 dependency just be ARCH_STRATIX10?

Yes, I will update this.

Quoted 5 lines

drivers/edac/Kconfig +0 -0

  	help
  	  Support for error detection and correction on the
  	  Altera SOCs. This must be selected for SDRAM ECC.

drivers/edac/altera_edac.c +0 -0

index 11d6419788c2..7ed885379719 100644

Quoted 9 lines

drivers/edac/altera_edac.c line 2
Quoted 5 lines
drivers/edac/altera_edac.c +1 -0
@@ -1,4 +1,5 @@
  /*
+ *  Copyright (C) 2017-2018, Intel Corporation
If you're updating the license header, would it make sense to convert to SPDX?

Yes, I can do that.

Quoted 6 lines

Dinh
--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

thor.thayer at linux.intel.com

author

2018-04-24 18:35 UTC #11 lore raw

PATCH 3/3 arm64: dts: stratix10: add sdram ecc

In reply to thor.thayer at linux.intel.com

From: Thor Thayer <[email protected]>

Add the Stratix10 ECC Manager and SDRAM EDAC nodes to the device tree.

Signed-off-by Thor Thayer <[email protected]>

---
 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +17 -0

index 76d5db4138b0..3a7da19216bb 100644
@@ -924,6 +924,23 @@
 			status = "disabled";
 		};
 
+		eccmgr: eccmgr at ffd12000 {
+			compatible = "altr,socfpga-s10-ecc-manager";
+			altr,sysmgr-syscon = <&sysmgr>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			interrupts = <0 15 4>, <0 95 4>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			ranges;
+
+			sdramedac at f8011100 {
+				compatible = "altr,sdram-edac-s10";
+				reg = <0xf8011100 0xC0>;
+				interrupts = <16 4>, <48 4>;
+			};
+		};
+
 		qspi: spi at ff8d2000 {
 			compatible = "cdns,qspi-nor";
 			#address-cells = <1>;

Signature · 1 line

2.7.4