edac: Add APM X-Gene SoC memory controller EDAC driver

This patch adds support for the APM X-Gene SoC memory controller EDAC driver for DT.

v8:
* Change ASM_EDAC_H to __ASM_EDAC_H in file edac.h
* Add WARN_ONCE in stub function atomic_scrub
* Update DTS binding documentation (with only memory controller node)
* Temporary remove L1/L2, L3, and SoC driver code and update memory driver code accordingly

v7:
* Update binding documentation for memory controller, PMD, L3, and SoC EDAC
* Change resource for PCP, CSW, MCBA, MCBB, and efuse to syscon type nodes and updae driver accordingly
* Fix clearing L2 RTO register properly
* Fix the MODULE_DEVICE_TABLE for OF
* Update DT accordingly to binding documentation

v6:
* Rebase to 4.0.0-rc3
* Add memory scrub stub function and enable ARM64 EDAC support patch
* Add bit definition defines for L2RTOS registers
* Remove un-necessary clearing of all L1/L2 software generated registers
* Remove wrong notification of LSU un-correctable error
* Change L2 reporting of un-correcable error to correctable error
* Change clearing of the L2C L2RTO registers
* Add support for L2 HW version 1 and version 2 or above
* Add support for L3 HW version 1 and version 2 or above

v5:
* Rebase to 3.17.rc1 (next)
* Update binding documentation for additional SoC node binding resource
* Enable MCU correctable and uncorrectable interrupts if not enabled by firmware
* Enable top level interrupt only after all MCU registered. Otherwise, error interrupt will never get cleared by the corresponding MCU.
* Remove clearing of L1 and L2 errors during initialization time. Otherwise, they will not be captured between firmware booting and error configuration.
* Add capture and clearing SoC register bus errors
* Add register bus resource to SoC DT node

v4:
* Fix PMD l1/l2 error reading address due to wrong variable type
* Fix clearing of software generated and HW errors for l1/l2

v3:
* Update binding documentation for PMD DT node and exampples
* Add binding documentation for SoC DT node
* Change MC, PMD, and L3C driver error injection to use debugfs
* Add missing IRQ for MC correctable error (code and DT)
* Use true/false where appropriate instead 1/0
* Add bit definition for L1 MMUESR register and fully decode this error
* Remove the un-necessary dev variable from xgene_edac_pmd_ctx structure
* Add check for disabled PMD (code and DT)
* Switch to edac_printk instead pr_err
* Some minor comments update

v2:
* Add EDAC entry in MAINTAINERS for APM EDAC driver
* Remove the MC scrub patch
* Remove the word 'Caches' from Kconfig
* Change all MASK defines to use BIT(x)
* Update comment or remove them
* Wrap error injection code around CONFIG_EDAC_DEBUG
* Change function name xgene_edac_mc_hw_init to xgene_edac_mc_irq_ctl
* Change all function XXX_hw_init to XXX_hw_ctl
* Fix typo 'activie'
* Move calling function edac_mc_alloc after resource retrieval
* Check for NULL on platform_get_resource return if reference directly
* Add documentation for struct xgene_edac_pmd_ctx
* Move L1 and L2 check out of function xgene_edac_pmd_check to its own functions
* Use for loop for configure each CPU of an PMD
* Replace /2 by >> 1
* Remove unnecessary comment on edac_device_add_device failure
* Make mem_err_ip static const
* Unwind EDAC register correctly if failed
---
Loc Ho (5):
arm64: Enable EDAC on ARM64
MAINTAINERS: Add entry for APM X-Gene SoC EDAC driver
Documentation: Add documentation for the APM X-Gene SoC EDAC DTS
binding
edac: Add APM X-Gene SoC EDAC driver
arm64: Add APM X-Gene SoC EDAC DTS entries

 .../devicetree/bindings/edac/apm-xgene-edac.txt    |   50 ++
 MAINTAINERS                                        |    8 +
 arch/arm64/Kconfig                                 |    1 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   64 +++
 arch/arm64/include/asm/edac.h                      |   28 +
 drivers/edac/Kconfig                               |    7 +
 drivers/edac/Makefile                              |    1 +
 drivers/edac/xgene_edac_mc.c                       |  552 ++++++++++++++++++++
 8 files changed, 711 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
 create mode 100644 arch/arm64/include/asm/edac.h
 create mode 100644 drivers/edac/xgene_edac_mc.c

Add an stub atomic_scrub function and enable EDAC for arm64.

---
 arch/arm64/Kconfig            |    1 +
 arch/arm64/include/asm/edac.h |   28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm64/include/asm/edac.h

index 4269dba..577078f 100644
@@ -22,6 +22,7 @@ config ARM64
 	select BUILDTIME_EXTABLE_SORT
 	select CLONE_BACKWARDS
 	select COMMON_CLK
+	select EDAC_SUPPORT
 	select CPU_PM if (SUSPEND || CPU_IDLE)
 	select DCACHE_WORD_ACCESS
 	select GENERIC_ALLOCATOR

new file mode 100644
index 0000000..683495b
@@ -0,0 +1,28 @@
+/*
+ * ARM64 EDAC Header File
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __ASM_EDAC_H
+#define __ASM_EDAC_H
+
+/*
+ * ECC atomic, DMA, SMP and interrupt safe scrub function.
+ */
+static inline void atomic_scrub(void *va, u32 size)
+{
+	/* Stub function for now until an ARM64 HW has a way to test it. */
+	WARN_ONCE(1, "not implemented");
+}
+
+#endif

1.7.1

This patch adds a MAINTAINERS entry for APM X-Gene SoC EDAC driver.

---
 MAINTAINERS |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

index 00a586b..06d3387 100644
@@ -3735,6 +3735,14 @@ W:	bluesmoke.sourceforge.net
 S:	Maintained
 F:	drivers/edac/sb_edac.c
 
+EDAC-XGENE
+APPLIED MICRO (APM) X-GENE SOC EDAC
+M:     Loc Ho <[email protected]>
+M:     Feng Kan <[email protected]>
+S:     Supported
+F:     drivers/edac/xgene_edac_mc.c
+F:     Documentation/devicetree/bindings/edac/apm-xgene-edac.txt
+
 EDIROL UA-101/UA-1000 DRIVER
 M:	Clemens Ladisch <[email protected]>
 L:	alsa-devel at alsa-project.org (moderated for non-subscribers)

--
1.7.1

This patch adds documentation for the APM X-Gene SoC EDAC DTS binding.

---
 .../devicetree/bindings/edac/apm-xgene-edac.txt    |   50 ++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/apm-xgene-edac.txt

new file mode 100644
index 0000000..b27c5a2
@@ -0,0 +1,50 @@
+* APM X-Gene SoC EDAC nodes
+
+EDAC nodes are defined to describe on-chip error detection and correction.
+The follow type is supported:
+
+  memory controller	- Memory controller
+
+The following section describes the memory controller DT node binding.
+
+Required properties:
+- compatible		: Shall be "apm,xgene-edac-mc".
+- regmap-pcp		: Regmap of the CPU bus (PCP) resource.
+- regmap-csw		: Regmap of the CPU switch fabric (CSW) resource.
+- regmap-mcba		: Regmap of the MCB-A (memory bridge) resource.
+- regmap-mcbb		: Regmap of the MCB-B (memory bridge) resource.
+- reg			: First resource shall be the memory controller unit
+                          (MCU) resource.
+- interrupts            : Interrupt-specifier for MCU error IRQ(s).
+
+Example:
+	pcp: pcp at 78800000 {
+		compatible = "syscon";
+		reg = <0x0 0x78800000 0x0 0x100>;
+	};
+
+	csw: csw at 7e200000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e200000 0x0 0x1000>;
+	};
+
+	mcba: mcba at 7e700000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e700000 0x0 0x1000>;
+	};
+
+	mcbb: mcbb at 7e720000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e720000 0x0 0x1000>;
+	};
+
+	edacmc0: edacmc0 at 7e800000 {
+		compatible = "apm,xgene-edac-mc";
+		regmap-pcp = <&pcp>;
+		regmap-csw = <&csw>;
+		regmap-mcba = <&mcba>;
+		regmap-mcbb = <&mcbb>;
+		reg = <0x0 0x7e800000 0x0 0x1000>;
+		interrupts = <0x0 0x20 0x4>,
+			     <0x0 0x21 0x4>;
+	};

1.7.1

This patch adds support for the APM X-Gene SoC memory controller EDAC driver.

---
 drivers/edac/Kconfig         |    7 +
 drivers/edac/Makefile        |    1 +
 drivers/edac/xgene_edac_mc.c |  552 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 560 insertions(+), 0 deletions(-)
 create mode 100644 drivers/edac/xgene_edac_mc.c

index cb59619..8ab74cc 100644
@@ -392,4 +392,11 @@ config EDAC_SYNOPSYS
 	  Support for error detection and correction on the Synopsys DDR
 	  memory controller.
 
+config EDAC_XGENE
+	tristate "APM X-Gene SoC"
+	depends on EDAC_MM_EDAC && ARM64
+	help
+	  Support for error detection and correction on the
+	  APM X-Gene family of SOCs.
+
 endif # EDAC

index b255f36..ff5c316 100644
@@ -68,3 +68,4 @@ obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
 
 obj-$(CONFIG_EDAC_ALTERA_MC)		+= altera_edac.o
 obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
+obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac_mc.o

--
1.7.1

This patch adds APM X-Gene SoC memory controller EDAC DTS entries.

---
 arch/arm64/boot/dts/apm/apm-storm.dtsi |   64 ++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)

index c8d3e0e..e0270a6 100644
@@ -374,6 +374,70 @@
 			};
 		};
 
+		pcp: pcp at 78800000 {
+			compatible = "syscon";
+			reg = <0x0 0x78800000 0x0 0x100>;
+		};
+
+		csw: csw at 7e200000 {
+			compatible = "syscon";
+			reg = <0x0 0x7e200000 0x0 0x1000>;
+		};
+
+		mcba: mcba at 7e700000 {
+			compatible = "syscon";
+			reg = <0x0 0x7e700000 0x0 0x1000>;
+		};
+
+		mcbb: mcbb at 7e720000 {
+			compatible = "syscon";
+			reg = <0x0 0x7e720000 0x0 0x1000>;
+		};
+
+		edacmc0: edacmc0 at 7e800000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e800000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
+		edacmc1: edacmc1 at 7e840000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e840000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
+		edacmc2: edacmc2 at 7e880000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e880000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
+		edacmc3: edacmc3 at 7e8c0000 {
+			compatible = "apm,xgene-edac-mc";
+			regmap-pcp = <&pcp>;
+			regmap-csw = <&csw>;
+			regmap-mcba = <&mcba>;
+			regmap-mcbb = <&mcbb>;
+			reg = <0x0 0x7e8c0000 0x0 0x1000>;
+			interrupts = <0x0 0x20 0x4>,
+				     <0x0 0x21 0x4>;
+		};
+
 		pcie0: pcie at 1f2b0000 {
 			status = "disabled";
 			device_type = "pci";

--
1.7.1

Hi Loc,

Great to see v8 of the patches :) A comment below.

nit: I expect you'll still get a little push back on that (similar to last time) because it doesn't actually implement anything, so while it is "safe" it's only because it's empty :) Might be better to indicate that those are the features it is supposed to have when implemented.

What does that mean exactly? They'll get added later?

It is called now xgene_edac_mc.c. Am I to expect more xgene_edac_<functional_unit>.c submissions? What happened to building everything around the shared IRQ handler?

Also, I see this SOB chain in some of the patches:

and it is not clear who did what, from looking at the chain.

If you both authored the patches, you can write

Originally-by: Feng
[ Loc: did this and that]
Signed-off-by: Loc

for example.

See Documentation/SubmittingPatches for more info.

Thanks.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Hi,

It is not include in this first patch set. Yes, it will be added later. I will response along with Arnd subsequent email.​

​Yes​

​It was discussed with Rob already in previous email. As we don't touch the shared status registers besides reading status, it will just register as shared interrupt.​

​Okay... Feng was the originally author.

-Loc

I don't see why that justifies the addition of an edac_xgene-<functional_unit> compilation unit and thus an .ko module per shared interrupt handler.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Hi,

If you are concern about multiple module files, I can either combine them as previous version or combine the multiple ko to an single ko file.

-Loc

I don't see how this is helping. You still use a syscon reference for the pcp node after I told you not to, and you are completely leaving out the other nodes, which makes it impossible to tell what your plan is for those.

Please come up with a plan that makes it possible to have proper support for all the devices in the future. Leaving out bits because you know that adding them later will be hard is not a good solution: if you screw up the design now, adding them later will be even harder.

Arnd

Hi Arnd/Borislav/Rob,

Okay... Let me summary the issue at hand and let us all agree:

1. Whether to have an single driver for APM EDAC or multiple instance of 4 different drivers. With single driver, it does not scale in the future when we add/remove memory controllers and CPU domains. This is also agreed by Rob Herring from review of the DTS nodes. For L3 and SoC EDAC, they are less of an issue as I don't see a situation that we would have multiple instances.

2. With regard to the top level PCP interrupt, they are just for status and once configured, it will not be touch. Therefore, I keep the current implementation. With an single driver, there is no need to worry about read/modify/write as it will be guarded with an lock. For multiple instance, I am thinking that the xgene_edac_mc module will provide exported lock functions for the other drivers.

If I am missing anything, let me know.

-Loc

Why doesn't it scale? Please explain this to me more verbosely.

Doing this would be a very bad design and it would be a homegrown case only for this driver. Then other ARM drivers will appear which will do their own locking too. No no no. No ad-hoc hackery.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Hi Borislav,

Let me explain a bit more. We have four memory controller today. Therefore, I would like to have 4 DTS node and the same driver probe function called 4 times. If there is only one driver for the entire APM EDAC, I would have to merge all the resource registers into an single DTS node and its probe function called one time. In this one driver design, what would I do in future chip or variant of the chip in which it remove or add an addition memory controller? I would have to change the driver as well as the DTS node. In the four instance probe design, all I need is to add an additional DTS node.

What if I move the locking function into a common module to be included by the EDAC framework? Or would you prefer that I go and write an common driver that all it does is provide locking?

Any suggestion as all I need is an way to share access to an CSR which can't use atomic operations? It isn't an actual interrupt controller either.

-Loc

There are lots of possible representations in DT that would solve this.

First of all, a device node can be turned into a platform_device but does not have to, so you you could have one DT node for the common parts (the pcp), and then subnodes underneath it, and have the driver attach to the main device but parse the subnodes manually to see what registers are there. There is no magic involved here, and this would address the concerns that Boris and I have.

Another option would be to have multiple drivers: have one driver that handles the common parts here, and make that export a shared interface to which the other drivers can register, then create five drivers that each do one thing. In my opinion that would work just as well, and be a nice abstraction, but I suspect that Boris would prefer doing it the other way.

I don't think there is a huge risk that other ARM drivers would copy a bad design, when it's obviously bad. In particular, other chips will most likely have other requirements in this area.

If this is something that does not fit into existing categories, you probably want to create your own high-level abstraction between the accesses to the shared parts of the driver and the parts that can exist in multiple instances. This will work for either of the approaches I've described above.

Arnd

Hi,

Thanks all for the comment. I will follow the design as with gpio-dwapb driver. This would take care of Borislav concern as mentioned by Arnd.

-Loc