edac: Add APM X-Gene SoC EDAC driver

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

Signed-off-by Loc Ho <[email protected]>

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

drivers/edac/xgene_edac.c new file +26 -0

+#ifdef CONFIG_EDAC_DEBUG
+static ssize_t xgene_edac_mc_err_inject_write(struct file *file,
+					      const char __user *data,
+					      size_t count, loff_t *ppos)
+{
+	struct mem_ctl_info *mci = file->private_data;
+	struct xgene_edac_mc_ctx *ctx = mci->pvt_info;
+	int i;
+
+	for (i = 0; i < MCU_MAX_RANK; i++) {
+		writel(MCU_ESRR_MULTUCERR_MASK | MCU_ESRR_BACKUCERR_MASK |
+		       MCU_ESRR_DEMANDUCERR_MASK | MCU_ESRR_CERR_MASK,
+		       ctx->mcu_csr + MCUESRRA0 + i * MCU_RANK_STRIDE);
+	}
+	return count;
+}
+
+static const struct file_operations xgene_edac_mc_debug_inject_fops = {
+	.open = simple_open,
+	.write = xgene_edac_mc_err_inject_write,
+	.llseek = generic_file_llseek,
+};
+
+static void xgene_edac_mc_create_debugfs_node(struct mem_ctl_info *mci)
+{
+	if (!mci->debugfs)

drivers/edac/xgene_edac.c new file +21 -0

+static int xgene_edac_csw_rd(struct xgene_edac *edac, u32 reg, u32 *val)
+{
+	if (edac == NULL)
+		return -EINVAL;
+
+	return regmap_read(edac->csw_map, reg, val);
+}
+
+static int xgene_edac_mcba_rd(struct xgene_edac *edac, u32 reg, u32 *val)
+{
+	if (edac == NULL)
+		return -EINVAL;
+
+	return regmap_read(edac->mcba_map, reg, val);
+}
+
+static int xgene_edac_mcbb_rd(struct xgene_edac *edac, u32 reg, u32 *val)
+{
+	if (edac == NULL)
+		return -EINVAL;
+

drivers/edac/xgene_edac.c new file +6 -0

+
+static int xgene_edac_pcp_setbits(struct xgene_edac *edac, u32 reg,
+				  u32 bits_mask)
+{
+	u32 val;
+

drivers/edac/xgene_edac.c line 84

Quoted 4 lines

drivers/edac/xgene_edac.c new file +4 -0

+static int edac_mc_idx;
+static int edac_mc_active_mask;
+static int edac_mc_registered_mask;
+static DEFINE_MUTEX(xgene_edac_mc_lock);

It would also be best to avoid global variables, but it seems that at least the edac_mc_idx is needed to work with the EDAC subsystem. Maybe Boris has an idea for how to avoid it.

Quoted 8 lines · Arnd Bergmann

drivers/edac/xgene_edac.c line 84

Quoted 4 lines

drivers/edac/xgene_edac.c new file +4 -0

+static int edac_mc_idx;
+static int edac_mc_active_mask;
+static int edac_mc_registered_mask;
+static DEFINE_MUTEX(xgene_edac_mc_lock);

It would also be best to avoid global variables, but it seems that at least the edac_mc_idx is needed to work with the EDAC subsystem. Maybe Boris has an idea for how to avoid it.

Right, so AFAICT this is used in

        edac_mc_alloc(edac_mc_idx++

and basically we're supplying the memory controller numbers in the order we're calling xgene_edac_mc_add(). Perhaps we need to make the memory controller numbering explicit with DT...

Quoted 9 lines

Quoted 8 lines

drivers/edac/xgene_edac.c line 84

Quoted 4 lines

drivers/edac/xgene_edac.c new file +4 -0

+static int edac_mc_idx;
+static int edac_mc_active_mask;
+static int edac_mc_registered_mask;
+static DEFINE_MUTEX(xgene_edac_mc_lock);

It would also be best to avoid global variables, but it seems that at least the edac_mc_idx is needed to work with the EDAC subsystem. Maybe Boris has an idea for how to avoid it.

Quoted 7 lines

Right, so AFAICT this is used in

        edac_mc_alloc(edac_mc_idx++

and basically we're supplying the memory controller numbers in the order we're calling xgene_edac_mc_add(). Perhaps we need to make the memory controller numbering explicit with DT...

I see now that the number is also used in xgene_edac_mc_is_active() as an index for the MCBADDRMR register, which seems very fragile, as it relies on the DT nodes getting seen in the same order by the driver that the hardware blocks are used in.

Better add an explicit DT property for the index that is defined to be the same as MCBADDRMR bits, e.g.

                       edacmc at 7e800000 {
                             compatible = "apm,xgene-edac-mc";
                             reg = <0x0 0x7e800000 0x0 0x1000>;
                              memory-controller = <0>;
                       };

or use a reference to the normal memory controller DT node outside of the EDAC, and put a number in that one.

Is there a strict 1:1 relation between the DIMM slot numbering on the board and the memory controller number within the EDAC registers?

Quoted 8 lines

drivers/edac/xgene_edac.c line 84

Quoted 4 lines

drivers/edac/xgene_edac.c new file +4 -0

+static int edac_mc_idx;
+static int edac_mc_active_mask;
+static int edac_mc_registered_mask;
+static DEFINE_MUTEX(xgene_edac_mc_lock);

It would also be best to avoid global variables, but it seems that at least the edac_mc_idx is needed to work with the EDAC subsystem. Maybe Boris has an idea for how to avoid it.

Right, so AFAICT this is used in

        edac_mc_alloc(edac_mc_idx++

and basically we're supplying the memory controller numbers in the order we're calling xgene_edac_mc_add(). Perhaps we need to make the memory controller numbering explicit with DT...

+ tmp_ctx.mcu_id = ((res.start >> 16) & 0xF) / 4;

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

Signed-off-by Loc Ho <[email protected]>

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt new file +10 -0

+Example:
+	csw: csw at 7e200000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e200000 0x0 0x1000>;
+	};
+
+	mcba: mcba at 7e700000 {
+		compatible = "syscon";
+		reg = <0x0 0x7e700000 0x0 0x1000>;
+	};

Add an stub atomic_scrub function and enable EDAC for arm64.

Signed-off-by Loc Ho <[email protected]>

---
 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

arch/arm64/Kconfig +1 -0

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

arch/arm64/include/asm/edac.h new file +16 -0

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

Quoted 43 lines · Loc Ho

Add an stub atomic_scrub function and enable EDAC for arm64.

Signed-off-by Loc Ho <[email protected]>

---
 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

arch/arm64/Kconfig +1 -0

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

arch/arm64/include/asm/edac.h new file +16 -0

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

arch/arm64/include/asm/edac.h line 26

Quoted 10 lines

arch/arm64/include/asm/edac.h new file +10 -0

+#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");
+}

So you're adding this empty function, it issues a warning for which the user can't really do anything about and on top of that, it is not even used in this patchset.

So please kill it from this submission.

You can add a full-fledged version later after you test it successfully on real hw.

This was discussed here
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/326986.html.
Without this, it will have compiler error as this function is called by.function edac_mc_scrub_block in file drivers/edac/edac_mc.c.

Quoted 4 lines · Loc Ho

This was discussed here
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/326986.html.
Without this, it will have compiler error as this function is called by.function edac_mc_scrub_block in file drivers/edac/edac_mc.c.

Argh, that's just silly. We should have a __weak atomic_scrub() in the edac core which arches can override but not have to supply a dummy just because...

I'll try to hack up something later.

From: Borislav Petkov<[email protected]>

So first of all, this atomic_scrub() function's naming is bad. It looks like an atomic_t helper. Change it to edac_atomic_scrub().

The bigger problem is that this function is arch-specific and every new arch which doesn't necessarily need that functionality still needs to define it, otherwise EDAC doesn't compile.

So instead of doing that and including arch-specific headers, have each arch define an EDAC_ATOMIC_SCRUB symbol which can be used in edac_mc.c for ifdeffery. Much cleaner.

We already are doing this with another symbol - EDAC_SUPPORT. This is also much cleaner than having CONFIG_EDAC explicitly depend on all the arches which need/have EDAC support and drivers.

This way I can kill the useless edac.h header in tile too.

Signed-off-by Borislav Petkov<[email protected]>

Quoted 17 lines · Borislav Petkov

From: Borislav Petkov<[email protected]>

So first of all, this atomic_scrub() function's naming is bad. It looks like an atomic_t helper. Change it to edac_atomic_scrub().

The bigger problem is that this function is arch-specific and every new arch which doesn't necessarily need that functionality still needs to define it, otherwise EDAC doesn't compile.

So instead of doing that and including arch-specific headers, have each arch define an EDAC_ATOMIC_SCRUB symbol which can be used in edac_mc.c for ifdeffery. Much cleaner.

We already are doing this with another symbol - EDAC_SUPPORT. This is also much cleaner than having CONFIG_EDAC explicitly depend on all the arches which need/have EDAC support and drivers.

Quoted 3 lines

This way I can kill the useless edac.h header in tile too.

Signed-off-by Borislav Petkov<[email protected]>

Acked-by Chris Metcalf <[email protected]> [for tile]

Quoted 18 lines · Chris Metcalf

Quoted 17 lines · Borislav Petkov

From: Borislav Petkov<[email protected]>

So first of all, this atomic_scrub() function's naming is bad. It looks like an atomic_t helper. Change it to edac_atomic_scrub().

The bigger problem is that this function is arch-specific and every new arch which doesn't necessarily need that functionality still needs to define it, otherwise EDAC doesn't compile.

So instead of doing that and including arch-specific headers, have each arch define an EDAC_ATOMIC_SCRUB symbol which can be used in edac_mc.c for ifdeffery. Much cleaner.

We already are doing this with another symbol - EDAC_SUPPORT. This is also much cleaner than having CONFIG_EDAC explicitly depend on all the arches which need/have EDAC support and drivers.

Quoted 5 lines

Quoted 3 lines

This way I can kill the useless edac.h header in tile too.

Signed-off-by Borislav Petkov<[email protected]>

Acked-by Chris Metcalf <[email protected]> [for tile]

Thanks.

Just to clarify after today's discussion on IRC: this patch doesn't change current DRAM scrubbing behavior on the relevant arches - it simply makes the definition of that atomic_scrub thing non-mandatory on new arches or on those which don't need it.

In the meantime, patch has been build-tested on arm and ppc - the two I'm missing an ACK for.

From: Borislav Petkov <[email protected]>

So first of all, this atomic_scrub() function's naming is bad. It looks like an atomic_t helper. Change it to edac_atomic_scrub().

The bigger problem is that this function is arch-specific and every new arch which doesn't necessarily need that functionality still needs to define it, otherwise EDAC doesn't compile.

So instead of doing that and including arch-specific headers, have each arch define an EDAC_ATOMIC_SCRUB symbol which can be used in edac_mc.c for ifdeffery. Much cleaner.

We already are doing this with another symbol - EDAC_SUPPORT. This is also much cleaner than having CONFIG_EDAC explicitly depend on all the arches which need/have EDAC support and drivers.

This way I can kill the useless edac.h header in tile too.

Signed-off-by Borislav Petkov <[email protected]>

Cc Russell King <[email protected]>

arch/arm/Kconfig +0 -0

 	select GENERIC_ALLOCATOR
 	select GENERIC_ATOMIC64 if (CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI)
 	select GENERIC_CLOCKEVENTS_BROADCAST if SMP

arch/arm/include/asm/edac.h +3 -2

index 0df7a2c1fc3d..5189fa819b60 100644
@@ -18,11 +18,12 @@
 #define ASM_EDAC_H
 /*
  * ECC atomic, DMA, SMP and interrupt safe scrub function.
- * Implements the per arch atomic_scrub() that EDAC use for software
+ * Implements the per arch edac_atomic_scrub() that EDAC use for software
  * ECC scrubbing.  It reads memory and then writes back the original
  * value, allowing the hardware to detect and correct memory errors.
  */
-static inline void atomic_scrub(void *va, u32 size)
+
+static inline void edac_atomic_scrub(void *va, u32 size)
 {
 #if __LINUX_ARM_ARCH__ >= 6
 	unsigned int *virt_addr = va;

Acked-by Russell King <[email protected]>

arch/arm/Kconfig config ARM · line 19

Quoted 11 lines

arch/arm/Kconfig +2 -0

index 45df48ba0b12..325d6f3a596a 100644
@@ -15,6 +15,8 @@ config ARM
 	select CLONE_BACKWARDS
 	select CPU_PM if (SUSPEND || CPU_IDLE)
 	select DCACHE_WORD_ACCESS if HAVE_EFFICIENT_UNALIGNED_ACCESS
+	select EDAC_SUPPORT
+	select EDAC_ATOMIC_SCRUB

I wonder if it would make sense to conditionalise EDAC_SUPPORT on...

		if CPU_32v6 || CPU_32v7

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

v10:
* Move regmap for CSW, MCBA, MCBB, and efuse to top driver node and add accessor support functions
* Change retrieval OF address proerty for subnode to use of_address_to_resource
* Add COMPILE_TEST to Kconfig for EDAC_XGENE
* Update DT binding documentation and DT node accordingly
* Remove DT node naming and number from nodes