[PATCHv6 0/5] Add Altera peripheral memories to EDAC framework

[PATCHv6 0/5] Add Altera peripheral memories to EDAC framework

[tthayer]

From: Thor Thayer <tthayer@opensource.altera.com>

This patch adds the L2 cache and OCRAM peripherals to the EDAC framework
using the EDAC device framework. The ECC is enabled early in the boot
process in the platform specific code.

v2 changes:
- Split On-Chip RAM ECC platform initialization into separate patch from
  L2 ECC platform initialization.
- Fix L2 cache dependency comments.
- Remove OCRAM node from dts and reference prior patch.

v3 changes:
- Move L2 cache & On-Chip RAM EDAC code into altera_edac.c
- Remove SDRAM module compile.

v4 changes:
- Change mask defines to use BIT().
- Fix comment style to agree with kernel coding style.
- Better printk description for read != write in trigger.
- Remove SysFS debugging message.
- Better dci->mod_name
- Move gen_pool pointer assignment to end of function.
- Invert logic to reduce indent in ocram depenency check.
- Change from dev_err() to edac_printk()
- Replace magic numbers with defines & comments.
- Improve error injection test.
- Change Makefile intermediary name to altr (from alt)

v5 changes:
- Remove l2cache.h by using if (IS_ENABLED(CONFIG_EDAC_ALTERA_L2C))
- Remove ocram.h by using if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
- Check prop variable before using. Include io.h.
- Add defines for better readability. Remove MAINTAINERS changes.

v6 changes:
- Simplify OCRAM initialization. Remove be32_to_cpup() calls.
- Remove syscon from L2 Cache. Force L2 Cache on if ECC enabled.
- Convert to nested ECC in device tree. 
- Additional comments to clarify debug error injection.

Thor Thayer (5):
  arm: socfpga: Enable L2 Cache ECC on startup.
  arm: socfpga: Enable OCRAM ECC on startup.
  edac: altera: Remove SDRAM module compile
  edac: altera: Add Altera L2 Cache and OCRAM EDAC Support
  arm: dts: Add Altera L2 Cache and OCRAM EDAC entries

 .../bindings/arm/altera/socfpga-edac.txt           |   46 ++
 arch/arm/boot/dts/socfpga.dtsi                     |   20 +
 arch/arm/mach-socfpga/Makefile                     |    2 +
 arch/arm/mach-socfpga/core.h                       |    2 +
 arch/arm/mach-socfpga/l2_cache.c                   |   39 ++
 arch/arm/mach-socfpga/ocram.c                      |   97 ++++
 arch/arm/mach-socfpga/socfpga.c                    |    4 +-
 drivers/edac/Kconfig                               |   20 +-
 drivers/edac/Makefile                              |    5 +-
 drivers/edac/altera_edac.c                         |  506 +++++++++++++++++++-
 10 files changed, 735 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
 create mode 100644 arch/arm/mach-socfpga/l2_cache.c
 create mode 100644 arch/arm/mach-socfpga/ocram.c

-- 
1.7.9.5

[PATCHv6 1/5] arm: socfpga: Enable L2 Cache ECC on startup.

[tthayer]

From: Thor Thayer <tthayer@opensource.altera.com>

This patch enables the ECC for L2 cache on machine
startup.  The ECC has to be enabled before data is
is stored in memory otherwise the ECC will fail on
reads.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2: Split OCRAM initialization into separate patch.

v3/4: No change

v5: Remove l2cache.h, use io.h instead of clk-provider.h
    Make copyright header inclusive. Remove MAINTAINERS entry.

v6: Remove pr_debug() & update year in header.
---
 arch/arm/mach-socfpga/Makefile   |    1 +
 arch/arm/mach-socfpga/core.h     |    2 ++
 arch/arm/mach-socfpga/l2_cache.c |   39 ++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-socfpga/socfpga.c  |    4 +++-
 4 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-socfpga/l2_cache.c

diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
index 6dd7a93..142609e 100644
--- a/arch/arm/mach-socfpga/Makefile
+++ b/arch/arm/mach-socfpga/Makefile
@@ -4,3 +4,4 @@
 
 obj-y					:= socfpga.o
 obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
+obj-$(CONFIG_EDAC_ALTERA_L2C) += l2_cache.o
diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
index 483cb46..28c8a15 100644
--- a/arch/arm/mach-socfpga/core.h
+++ b/arch/arm/mach-socfpga/core.h
@@ -47,4 +47,6 @@ extern unsigned long socfpga_cpu1start_addr;
 
 #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
 
+void socfpga_init_l2_ecc(void);
+
 #endif
diff --git a/arch/arm/mach-socfpga/l2_cache.c b/arch/arm/mach-socfpga/l2_cache.c
new file mode 100644
index 0000000..047759d
--- /dev/null
+++ b/arch/arm/mach-socfpga/l2_cache.c
@@ -0,0 +1,39 @@
+/*
+ * Copyright Altera Corporation (C) 2015. All rights reserved.
+ *
+ * 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/>.
+ */
+#include <linux/io.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+
+void socfpga_init_l2_ecc(void)
+{
+	struct device_node *np;
+	void __iomem  *mapped_l2_edac_addr;
+
+	np = of_find_compatible_node(NULL, NULL, "altr,l2-edac");
+	if (!np) {
+		pr_err("SOCFPGA: Unable to find altr,l2-edac in dtb\n");
+		return;
+	}
+
+	mapped_l2_edac_addr = of_iomap(np, 0);
+	if (!mapped_l2_edac_addr) {
+		pr_err("SOCFPGA: Unable to find L2 ECC mapping in dtb\n");
+		return;
+	}
+
+	/* Enable ECC */
+	writel(0x01, mapped_l2_edac_addr);
+}
diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index 383d61e..ce04313 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -1,5 +1,5 @@
 /*
- *  Copyright (C) 2012 Altera Corporation
+ *  Copyright (C) 2012-2015 Altera Corporation
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -83,6 +83,8 @@ static void __init socfpga_init_irq(void)
 {
 	irqchip_init();
 	socfpga_sysmgr_init();
+	if (IS_ENABLED(CONFIG_EDAC_ALTERA_L2C))
+		socfpga_init_l2_ecc();
 }
 
 static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
-- 
1.7.9.5

[PATCHv6 2/5] arm: socfpga: Enable OCRAM ECC on startup.

[tthayer]

From: Thor Thayer <tthayer@opensource.altera.com>

This patch enables the ECC for On-Chip RAM on machine
startup.  The ECC has to be enabled before data is
is stored in memory otherwise the ECC will fail on
reads.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2: Split OCRAM ECC portion separately. Addition of iounmap()
    and reorganization of gen_pool_free. Remove defconfig from patch.

v3/4: No change

v5: Remove ocram.h, use io.h instead of clk-provider.h
    Check prop in correct place. Add ECC EN defines.

v6: Implement OCRAM discovery changes from community. Add
    of_node_put(). Remove be32_to_cpup(). Use __init() which
    allows removal of .init_machine(). Update year in header.
---
 arch/arm/mach-socfpga/Makefile |    1 +
 arch/arm/mach-socfpga/ocram.c  |   97 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100644 arch/arm/mach-socfpga/ocram.c

diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
index 142609e..1552ca5 100644
--- a/arch/arm/mach-socfpga/Makefile
+++ b/arch/arm/mach-socfpga/Makefile
@@ -5,3 +5,4 @@
 obj-y					:= socfpga.o
 obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
 obj-$(CONFIG_EDAC_ALTERA_L2C) += l2_cache.o
+obj-$(CONFIG_EDAC_ALTERA_OCRAM) += ocram.o
diff --git a/arch/arm/mach-socfpga/ocram.c b/arch/arm/mach-socfpga/ocram.c
new file mode 100644
index 0000000..1f12a38
--- /dev/null
+++ b/arch/arm/mach-socfpga/ocram.c
@@ -0,0 +1,97 @@
+/*
+ * Copyright Altera Corporation (C) 2015. All rights reserved.
+ *
+ * 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/>.
+ */
+#include <linux/io.h>
+#include <linux/genalloc.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#define ALTR_OCRAM_CLEAR_ECC          0x00000018
+#define ALTR_OCRAM_ECC_EN             0x00000019
+
+static int __init socfpga_init_ocram_ecc(void)
+{
+	struct device_node *np;
+	struct resource    res;
+	u32                iram_addr;
+	void __iomem       *mapped_ocr_edac_addr;
+	resource_size_t    size;
+	struct gen_pool    *gp;
+	int                ret;
+
+	/* Get the size of the on-chip RAM */
+	np = of_find_compatible_node(NULL, NULL, "mmio-sram");
+	if (!np) {
+		pr_err("%s: Unable to find mmio-sram in dtb\n", __func__);
+		return -ENODEV;
+	}
+
+	ret = of_address_to_resource(np, 0, &res);
+	if (ret) {
+		of_node_put(np);
+		pr_err("%s: Problem getting SRAM address in dtb\n", __func__);
+		return -ENODEV;
+	}
+	size = resource_size(&res);
+	of_node_put(np);
+
+	/* Find the OCRAM EDAC device tree node */
+	np = of_find_compatible_node(NULL, NULL, "altr,ocram-edac");
+	if (!np) {
+		pr_err("%s: Unable to find altr,ocram-edac\n", __func__);
+		return -ENODEV;
+	}
+
+	mapped_ocr_edac_addr = of_iomap(np, 0);
+	if (!mapped_ocr_edac_addr) {
+		of_node_put(np);
+		pr_err("%s: Unable to map OCRAM ecc regs.\n", __func__);
+		return -ENODEV;
+	}
+
+	gp = of_get_named_gen_pool(np, "iram", 0);
+	if (!gp) {
+		of_node_put(np);
+		pr_err("%s: OCRAM cannot find gen pool\n", __func__);
+		return -ENODEV;
+	}
+	of_node_put(np);
+
+	iram_addr = gen_pool_alloc(gp, size / sizeof(size_t));
+	if (iram_addr == 0) {
+		pr_err("%s: cannot alloc from gen pool\n", __func__);
+		return -ENODEV;
+	}
+
+	/* Clear any pending OCRAM ECC interrupts, then enable ECC */
+	writel(ALTR_OCRAM_CLEAR_ECC, mapped_ocr_edac_addr);
+	writel(ALTR_OCRAM_ECC_EN, mapped_ocr_edac_addr);
+
+	memset((void *)iram_addr, 0, size);
+
+	gen_pool_free(gp, iram_addr, size / sizeof(size_t));
+
+	iounmap(mapped_ocr_edac_addr);
+
+	return 0;
+}
+
+static void __exit socfpga_exit_ocram_ecc(void)
+{
+}
+
+module_init(socfpga_init_ocram_ecc);
+module_exit(socfpga_exit_ocram_ecc);
-- 
1.7.9.5

[PATCHv6 3/5] edac: altera: Remove SDRAM module compile

[tthayer]

From: Thor Thayer <tthayer@opensource.altera.com>

The SDRAM EDAC requires SDRAM configuration/initialization before
SDRAM is accessed (in the preloader). Having a module compile is
not desired so force to be built into kernel.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v3: Added in this version as a separate patch.

v4-6: No change.
---
 drivers/edac/Kconfig |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 49c2652..4d7285e 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -377,8 +377,8 @@ config EDAC_OCTEON_PCI
 	  Cavium Octeon family of SOCs.
 
 config EDAC_ALTERA_MC
-	tristate "Altera SDRAM Memory Controller EDAC"
-	depends on EDAC_MM_EDAC && ARCH_SOCFPGA
+	bool "Altera SDRAM Memory Controller EDAC"
+	depends on EDAC_MM_EDAC=y && ARCH_SOCFPGA
 	help
 	  Support for error detection and correction on the
 	  Altera SDRAM memory controller. Note that the
-- 
1.7.9.5

[PATCHv6 4/5] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support

[tthayer]

From: Thor Thayer <tthayer@opensource.altera.com>

Adding L2 Cache and On-Chip RAM EDAC support for the
Altera SoCs using the EDAC device  model. The SDRAM
controller is using the Memory Controller model.

Each type of ECC is individually configurable.

The SDRAM ECC is a separate Kconfig option because:
1) the SDRAM preparation can take almost 2 seconds on boot and some
customers need a faster boot time.
2) the SDRAM has an ECC initialization dependency on the preloader
which is outside the kernel. It is desirable to be able to turn the
SDRAM on & off separately.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2: Fix L2 dependency comments.

v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c
    instead of separate files.

v4: Change mask defines to use BIT().
    Fix comment style to agree with kernel coding style.
    Better printk description for read != write in trigger.
    Remove SysFS debugging message.
    Better dci->mod_name
    Move gen_pool pointer assignment to end of function.
    Invert logic to reduce indent in ocram depenency check.
    Change from dev_err() to edac_printk()
    Replace magic numbers with defines & comments.
    Improve error injection test.
    Change Makefile intermediary name to altr (from alt)

v5: No change.

v6: Convert to nested EDAC in device tree. Force L2 cache
    on for L2Cache ECC & remove L2 cache syscon for checking
    enable bit. Update year in header.
---
 drivers/edac/Kconfig       |   16 ++
 drivers/edac/Makefile      |    5 +-
 drivers/edac/altera_edac.c |  506 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 524 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 4d7285e..edf97ed 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -385,4 +385,20 @@ config EDAC_ALTERA_MC
 	  preloader must initialize the SDRAM before loading
 	  the kernel.
 
+config EDAC_ALTERA_L2C
+	bool "Altera L2 Cache EDAC"
+	depends on EDAC_MM_EDAC=y && ARCH_SOCFPGA
+	select CACHE_L2X0
+	help
+	  Support for error detection and correction on the
+	  Altera L2 cache Memory for Altera SoCs. This option
+          requires L2 cache so it will force that selection.
+
+config EDAC_ALTERA_OCRAM
+	bool "Altera On-Chip RAM EDAC"
+	depends on EDAC_MM_EDAC=y && ARCH_SOCFPGA && SRAM && GENERIC_ALLOCATOR
+	help
+	  Support for error detection and correction on the
+	  Altera On-Chip RAM Memory for Altera SoCs.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index d40c69a..b9a67c0 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -66,4 +66,7 @@ obj-$(CONFIG_EDAC_OCTEON_L2C)		+= octeon_edac-l2c.o
 obj-$(CONFIG_EDAC_OCTEON_LMC)		+= octeon_edac-lmc.o
 obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
 
-obj-$(CONFIG_EDAC_ALTERA_MC)		+= altera_edac.o
+altr_edac-y				:= altera_edac.o
+obj-$(CONFIG_EDAC_ALTERA_MC)		+= altr_edac.o
+obj-$(CONFIG_EDAC_ALTERA_L2C)		+= altr_edac.o
+obj-$(CONFIG_EDAC_ALTERA_OCRAM)		+= altr_edac.o
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 3c4929f..083cd20 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1,5 +1,5 @@
 /*
- *  Copyright Altera Corporation (C) 2014. All rights reserved.
+ *  Copyright Altera Corporation (C) 2014-2015. All rights reserved.
  *  Copyright 2011-2012 Calxeda, Inc.
  *
  * This program is free software; you can redistribute it and/or modify it
@@ -17,8 +17,10 @@
  * Adapted from the highbank_mc_edac driver.
  */
 
+#include <asm/cacheflush.h>
 #include <linux/ctype.h>
 #include <linux/edac.h>
+#include <linux/genalloc.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
@@ -33,6 +35,7 @@
 
 #define EDAC_MOD_STR		"altera_edac"
 #define EDAC_VERSION		"1"
+#define EDAC_DEVICE		"ALTR_MEM"
 
 /* SDRAM Controller CtrlCfg Register */
 #define CTLCFG_OFST             0x00
@@ -107,6 +110,33 @@ struct altr_sdram_mc_data {
 	struct regmap *mc_vbase;
 };
 
+/************************** EDAC Device Defines **************************/
+
+/* OCRAM ECC Management Group Defines */
+#define ALTR_MAN_GRP_OCRAM_ECC_OFFSET   0x04
+#define ALTR_OCR_ECC_EN_MASK            BIT(0)
+#define ALTR_OCR_ECC_INJS_MASK          BIT(1)
+#define ALTR_OCR_ECC_INJD_MASK          BIT(2)
+#define ALTR_OCR_ECC_SERR_MASK          BIT(3)
+#define ALTR_OCR_ECC_DERR_MASK          BIT(4)
+
+/* L2 ECC Management Group Defines */
+#define ALTR_MAN_GRP_L2_ECC_OFFSET      0x00
+#define ALTR_L2_ECC_EN_MASK             BIT(0)
+#define ALTR_L2_ECC_INJS_MASK           BIT(1)
+#define ALTR_L2_ECC_INJD_MASK           BIT(2)
+
+#define ALTR_UE_TRIGGER_CHAR            'U'   /* Trigger for UE */
+#define ALTR_TRIGGER_READ_WRD_CNT       32    /* Line size x 4 */
+#define ALTR_TRIG_OCRAM_BYTE_SIZE       128   /* Line size x 4 */
+#define ALTR_TRIG_L2C_BYTE_SIZE         4096  /* Full Page */
+
+/*********************** EDAC Memory Controller Functions ****************/
+
+/* The SDRAM controller uses the EDAC Memory Controller framework.       */
+
+#ifdef CONFIG_EDAC_ALTERA_MC
+
 static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id)
 {
 	struct mem_ctl_info *mci = dev_id;
@@ -405,6 +435,478 @@ static struct platform_driver altr_sdram_edac_driver = {
 
 module_platform_driver(altr_sdram_edac_driver);
 
+#endif		/* #ifdef CONFIG_EDAC_ALTERA_MC */
+/************************* EDAC Parent Probe *************************/
+
+static const struct of_device_id altr_edac_device_of_match[];
+
+static const struct of_device_id altr_edac_of_match[] = {
+	{ .compatible = "altr,edac" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, altr_edac_of_match);
+
+static int altr_edac_probe(struct platform_device *pdev)
+{
+	of_platform_populate(pdev->dev.of_node, altr_edac_device_of_match,
+			     NULL, &pdev->dev);
+	return 0;
+}
+
+static struct platform_driver altr_edac_driver = {
+	.probe =  altr_edac_probe,
+	.driver = {
+		.name = "altr_edac",
+		.of_match_table = altr_edac_of_match,
+	},
+};
+module_platform_driver(altr_edac_driver);
+
+/************************* EDAC Device Functions *************************/
+
+/*
+ * EDAC Device Functions (shared between various IPs).
+ * The discrete memories use the EDAC Device framework. The probe
+ * and error handling functions are very similar between memories
+ * so they are shared. The memory allocation and free for EDAC trigger
+ * testing are different for each memory.
+ */
+
+const struct edac_device_prv_data ocramecc_data;
+const struct edac_device_prv_data l2ecc_data;
+
+struct edac_device_prv_data {
+	int (*setup)(struct platform_device *pdev, void __iomem *base);
+	int ce_clear_mask;
+	int ue_clear_mask;
+#ifdef CONFIG_EDAC_DEBUG
+	struct edac_dev_sysfs_attribute *eccmgr_sysfs_attr;
+	void * (*alloc_mem)(size_t size, void **other);
+	void (*free_mem)(void *p, size_t size, void *other);
+	int ecc_enable_mask;
+	int ce_set_mask;
+	int ue_set_mask;
+	int trig_alloc_sz;
+#endif
+};
+
+struct altr_edac_device_dev {
+	void __iomem *base;
+	int sb_irq;
+	int db_irq;
+	const struct edac_device_prv_data *data;
+	char *edac_dev_name;
+};
+
+static irqreturn_t altr_edac_device_handler(int irq, void *dev_id)
+{
+	struct edac_device_ctl_info *dci = dev_id;
+	struct altr_edac_device_dev *drvdata = dci->pvt_info;
+	const struct edac_device_prv_data *priv = drvdata->data;
+
+	if (irq == drvdata->sb_irq) {
+		if (priv->ce_clear_mask)
+			writel(priv->ce_clear_mask, drvdata->base);
+		edac_device_handle_ce(dci, 0, 0, drvdata->edac_dev_name);
+	}
+	if (irq == drvdata->db_irq) {
+		if (priv->ue_clear_mask)
+			writel(priv->ue_clear_mask, drvdata->base);
+		edac_device_handle_ue(dci, 0, 0, drvdata->edac_dev_name);
+		panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_EDAC_DEBUG
+ssize_t altr_edac_device_trig(struct edac_device_ctl_info *edac_dci,
+			      const char *buffer, size_t count)
+{
+	u32 *ptemp, i, error_mask;
+	int result = 0;
+	unsigned long flags;
+	struct altr_edac_device_dev *drvdata = edac_dci->pvt_info;
+	const struct edac_device_prv_data *priv = drvdata->data;
+	void *generic_ptr = edac_dci->dev;
+
+	if (!priv->alloc_mem)
+		return -ENOMEM;
+
+	/*
+	 * Note that generic_ptr is initialized to the device * but in
+	 * some alloc_functions, this is overridden and returns data.
+	 */
+	ptemp = priv->alloc_mem(priv->trig_alloc_sz, &generic_ptr);
+	if (!ptemp) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Inject: Buffer Allocation error\n");
+		return -ENOMEM;
+	}
+
+	if (buffer[0] == ALTR_UE_TRIGGER_CHAR)
+		error_mask = priv->ue_set_mask;
+	else
+		error_mask = priv->ce_set_mask;
+
+	edac_printk(KERN_ALERT, EDAC_DEVICE,
+		    "Trigger Error Mask (0x%X)\n", error_mask);
+
+	local_irq_save(flags);
+	/* write ECC corrupted data out. */
+	for (i = 0; i < (priv->trig_alloc_sz / sizeof(*ptemp)); i++) {
+		/* Read data so we're in the correct state */
+		rmb();
+		if (ACCESS_ONCE(ptemp[i]))
+			result = -1;
+		/* Toggle Error bit (it is latched), leave ECC enabled */
+		writel(error_mask, drvdata->base);
+		writel(priv->ecc_enable_mask, drvdata->base);
+		ptemp[i] = i;
+	}
+	/* Ensure it has been written out */
+	wmb();
+	local_irq_restore(flags);
+
+	if (result)
+		edac_printk(KERN_ERR, EDAC_DEVICE, "Mem Not Cleared\n");
+
+	/* Read out written data. ECC error caused here */
+	for (i = 0; i < ALTR_TRIGGER_READ_WRD_CNT; i++)
+		if (ACCESS_ONCE(ptemp[i]) != i)
+			edac_printk(KERN_ERR, EDAC_DEVICE,
+				    "Read doesn't match written data\n");
+
+	if (priv->free_mem)
+		priv->free_mem(ptemp, priv->trig_alloc_sz, generic_ptr);
+
+	return count;
+}
+
+static void altr_set_sysfs_attr(struct edac_device_ctl_info *edac_dci,
+				const struct edac_device_prv_data *priv)
+{
+	struct edac_dev_sysfs_attribute *ecc_attr = priv->eccmgr_sysfs_attr;
+
+	if (ecc_attr)
+		edac_dci->sysfs_attributes = ecc_attr;
+}
+#else
+static void altr_set_sysfs_attr(struct edac_device_ctl_info *edac_dci,
+				const struct edac_device_prv_data *priv)
+{}
+#endif	/* #ifdef CONFIG_EDAC_DEBUG */
+
+static const struct of_device_id altr_edac_device_of_match[] = {
+#ifdef CONFIG_EDAC_ALTERA_L2C
+	{ .compatible = "altr,l2-edac", .data = (void *)&l2ecc_data },
+#endif
+#ifdef CONFIG_EDAC_ALTERA_OCRAM
+	{ .compatible = "altr,ocram-edac", .data = (void *)&ocramecc_data },
+#endif
+	{},
+};
+MODULE_DEVICE_TABLE(of, altr_edac_device_of_match);
+
+/*
+ * altr_edac_device_probe()
+ *	This is a generic EDAC device driver that will support
+ *	various Altera memory devices such as the L2 cache ECC and
+ *	OCRAM ECC as well as the memories for other peripherals.
+ *	Module specific initialization is done by passing the
+ *	function index in the device tree.
+ */
+static int altr_edac_device_probe(struct platform_device *pdev)
+{
+	struct edac_device_ctl_info *dci;
+	struct altr_edac_device_dev *drvdata;
+	struct resource *r;
+	int res = 0;
+	struct device_node *np = pdev->dev.of_node;
+	char *ecc_name = (char *)np->name;
+	static int dev_instance;
+
+	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Unable to open devm\n");
+		return -ENOMEM;
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Unable to get mem resource\n");
+		return -EPROBE_DEFER;
+	}
+
+	if (!devm_request_mem_region(&pdev->dev, r->start, resource_size(r),
+				     dev_name(&pdev->dev))) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "%s:Error requesting mem region\n", ecc_name);
+		return -EBUSY;
+	}
+
+	dci = edac_device_alloc_ctl_info(sizeof(*drvdata), ecc_name,
+					 1, ecc_name, 1, 0, NULL, 0,
+					 dev_instance++);
+
+	if (!dci) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "%s: Unable to allocate EDAC device\n", ecc_name);
+		return -ENOMEM;
+	}
+
+	drvdata = dci->pvt_info;
+	dci->dev = &pdev->dev;
+	platform_set_drvdata(pdev, dci);
+	drvdata->edac_dev_name = ecc_name;
+
+	drvdata->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
+	if (!drvdata->base)
+		goto err;
+
+	/* Get driver specific data for this EDAC device */
+	drvdata->data = of_match_node(altr_edac_device_of_match, np)->data;
+
+	/* Check specific dependencies for the module */
+	if (drvdata->data->setup) {
+		res = drvdata->data->setup(pdev, drvdata->base);
+		if (res < 0)
+			goto err;
+	}
+
+	drvdata->sb_irq = platform_get_irq(pdev, 0);
+	res = devm_request_irq(&pdev->dev, drvdata->sb_irq,
+			       altr_edac_device_handler,
+			       0, dev_name(&pdev->dev), dci);
+	if (res < 0)
+		goto err;
+
+	drvdata->db_irq = platform_get_irq(pdev, 1);
+	res = devm_request_irq(&pdev->dev, drvdata->db_irq,
+			       altr_edac_device_handler,
+			       0, dev_name(&pdev->dev), dci);
+	if (res < 0)
+		goto err;
+
+	dci->mod_name = "Altera EDAC Memory";
+	dci->dev_name = drvdata->edac_dev_name;
+
+	altr_set_sysfs_attr(dci, drvdata->data);
+
+	if (edac_device_add_device(dci))
+		goto err;
+
+	devres_close_group(&pdev->dev, NULL);
+
+	return 0;
+err:
+	edac_printk(KERN_ERR, EDAC_DEVICE,
+		    "%s:Error setting up EDAC device: %d\n", ecc_name, res);
+	devres_release_group(&pdev->dev, NULL);
+	edac_device_free_ctl_info(dci);
+
+	return res;
+}
+
+static int altr_edac_device_remove(struct platform_device *pdev)
+{
+	struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
+
+	edac_device_del_device(&pdev->dev);
+	edac_device_free_ctl_info(dci);
+
+	return 0;
+}
+
+static struct platform_driver altr_edac_device_driver = {
+	.probe =  altr_edac_device_probe,
+	.remove = altr_edac_device_remove,
+	.driver = {
+		.name = "altr_edac_device",
+		.of_match_table = altr_edac_device_of_match,
+	},
+};
+module_platform_driver(altr_edac_device_driver);
+
+/*********************** OCRAM EDAC Device Functions *********************/
+
+#ifdef CONFIG_EDAC_ALTERA_OCRAM
+
+#ifdef CONFIG_EDAC_DEBUG
+static void *ocram_alloc_mem(size_t size, void **other)
+{
+	struct device_node *np;
+	struct gen_pool *gp;
+	void *sram_addr;
+
+	np = of_find_compatible_node(NULL, NULL, "altr,ocram-edac");
+	if (!np)
+		return NULL;
+
+	gp = of_get_named_gen_pool(np, "iram", 0);
+	if (!gp) {
+		of_node_put(np);
+		return NULL;
+	}
+	of_node_put(np);
+
+	sram_addr = (void *)gen_pool_alloc(gp, size / sizeof(size_t));
+	if (!sram_addr)
+		return NULL;
+
+	memset(sram_addr, 0, size);
+	wmb();	/* Ensure data is written out */
+
+	*other = gp;	/* Remember this handle for freeing  later */
+
+	return sram_addr;
+}
+
+static void ocram_free_mem(void *p, size_t size, void *other)
+{
+	gen_pool_free((struct gen_pool *)other, (u32)p, size / sizeof(size_t));
+}
+
+static struct edac_dev_sysfs_attribute altr_ocr_sysfs_attributes[] = {
+	{
+		.attr = { .name = "altr_ocram_trigger",
+			  .mode = (S_IRUGO | S_IWUSR) },
+		.show = NULL,
+		.store = altr_edac_device_trig
+	},
+	{
+		.attr = {.name = NULL }
+	}
+};
+#endif	/* #ifdef CONFIG_EDAC_DEBUG */
+
+/*
+ * altr_ocram_dependencies()
+ *	Test for OCRAM cache ECC dependencies upon entry because
+ *	platform specific startup should have initialized the
+ *	On-Chip RAM memory and enabled the ECC.
+ *	Can't turn on ECC here because accessing un-initialized
+ *	memory will cause CE/UE errors possibly causing an ABORT.
+ */
+static int altr_ocram_dependencies(struct platform_device *pdev,
+				   void __iomem *base)
+{
+	u32 control;
+
+	control = readl(base) & ALTR_OCR_ECC_EN_MASK;
+	if (control)
+		return 0;
+
+	edac_printk(KERN_ERR, EDAC_DEVICE,
+		    "OCRAM: No ECC present or ECC disabled.\n");
+	return -ENODEV;
+}
+
+const struct edac_device_prv_data ocramecc_data = {
+	.setup = altr_ocram_dependencies,
+	.ce_clear_mask = (ALTR_OCR_ECC_EN_MASK | ALTR_OCR_ECC_SERR_MASK),
+	.ue_clear_mask = (ALTR_OCR_ECC_EN_MASK | ALTR_OCR_ECC_DERR_MASK),
+#ifdef CONFIG_EDAC_DEBUG
+	.eccmgr_sysfs_attr = altr_ocr_sysfs_attributes,
+	.alloc_mem = ocram_alloc_mem,
+	.free_mem = ocram_free_mem,
+	.ecc_enable_mask = ALTR_OCR_ECC_EN_MASK,
+	.ce_set_mask = (ALTR_OCR_ECC_EN_MASK | ALTR_OCR_ECC_INJS_MASK),
+	.ue_set_mask = (ALTR_OCR_ECC_EN_MASK | ALTR_OCR_ECC_INJD_MASK),
+	.trig_alloc_sz = ALTR_TRIG_OCRAM_BYTE_SIZE,
+#endif
+};
+
+#endif	/* #ifdef CONFIG_EDAC_ALTERA_OCRAM */
+
+/********************* L2 Cache EDAC Device Functions ********************/
+
+#ifdef CONFIG_EDAC_ALTERA_L2C
+
+#ifdef CONFIG_EDAC_DEBUG
+static void *l2_alloc_mem(size_t size, void **other)
+{
+	struct device *dev = *other;
+	void *ptemp = devm_kzalloc(dev, size, GFP_KERNEL);
+
+	if (!ptemp)
+		return NULL;
+
+	/* Make sure everything is written out */
+	wmb();
+
+	/*
+	 * Clean all cache levels up to LoC (includes L2)
+	 * This ensures the corrupted data is written into
+	 * L2 cache for readback test (which causes ECC error).
+	 */
+	flush_cache_all();
+
+	return ptemp;
+}
+
+static void l2_free_mem(void *p, size_t size, void *other)
+{
+	struct device *dev = other;
+
+	if (dev && p)
+		devm_kfree(dev, p);
+}
+
+static struct edac_dev_sysfs_attribute altr_l2_sysfs_attributes[] = {
+	{
+		.attr = { .name = "altr_l2_trigger",
+			  .mode = (S_IRUGO | S_IWUSR) },
+		.show = NULL,
+		.store = altr_edac_device_trig
+	},
+	{
+		.attr = {.name = NULL }
+	}
+};
+#endif	/* #ifdef CONFIG_EDAC_DEBUG */
+
+/*
+ * altr_l2_dependencies()
+ *	Test for L2 cache ECC dependencies upon entry because
+ *	platform specific startup should have initialized the L2
+ *	memory and enabled the ECC.
+ *	Bail if ECC is not enabled.
+ *	Note that L2 Cache Enable is forced at build time.
+ */
+static int altr_l2_dependencies(struct platform_device *pdev,
+				void __iomem *base)
+{
+	u32 control;
+
+	control = readl(base) & ALTR_L2_ECC_EN_MASK;
+	if (!control) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "L2: No ECC present, or ECC disabled\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+const struct edac_device_prv_data l2ecc_data = {
+	.setup = altr_l2_dependencies,
+	.ce_clear_mask = 0,
+	.ue_clear_mask = 0,
+#ifdef CONFIG_EDAC_DEBUG
+	.eccmgr_sysfs_attr = altr_l2_sysfs_attributes,
+	.alloc_mem = l2_alloc_mem,
+	.free_mem = l2_free_mem,
+	.ecc_enable_mask = ALTR_L2_ECC_EN_MASK,
+	.ce_set_mask = (ALTR_L2_ECC_EN_MASK | ALTR_L2_ECC_INJS_MASK),
+	.ue_set_mask = (ALTR_L2_ECC_EN_MASK | ALTR_L2_ECC_INJD_MASK),
+	.trig_alloc_sz = ALTR_TRIG_L2C_BYTE_SIZE,
+#endif
+};
+
+#endif	/* #ifdef CONFIG_EDAC_ALTERA_L2C */
+
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Thor Thayer");
-MODULE_DESCRIPTION("EDAC Driver for Altera SDRAM Controller");
+MODULE_DESCRIPTION("EDAC Driver for Altera Memories");
-- 
1.7.9.5

[PATCHv6 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC entries

[tthayer]

From: Thor Thayer <tthayer@opensource.altera.com>

Adding the device tree entries and bindings needed to support
the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.html

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
v2: Remove OCRAM declaration and reference prior patch.

v3-5: No Change

v6: Change to nested EDAC device nodes based on community
    feedback. Remove L2 syscon. Use consolidated binding.
---
 .../bindings/arm/altera/socfpga-edac.txt           |   46 ++++++++++++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |   20 +++++++++
 2 files changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
new file mode 100644
index 0000000..4bf32e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
@@ -0,0 +1,46 @@
+Altera SoCFPGA Error Detection and Correction [EDAC]
+
+Required Properties:
+- compatible : Should be "altr,edac"
+- #address-cells: must be 1
+- #size-cells: must be 1
+- ranges : standard definition, should translate from local addresses
+
+Subcomponents:
+
+L2 Cache ECC
+Required Properties:
+- compatible : Should be "altr,l2-edac"
+- reg : Address and size for ECC error interrupt clear registers.
+- interrupts : Should be single bit error interrupt, then double bit error
+	interrupt. Note the rising edge type.
+
+On Chip RAM ECC
+Required Properties:
+- compatible : Should be "altr,ocram-edac"
+- reg : Address and size for ECC error interrupt clear registers.
+- iram : phandle to On-Chip RAM definition.
+- interrupts : Should be single bit error interrupt, then double bit error
+	interrupt. Note the rising edge type.
+
+Example:
+
+	soc_ecc {
+		compatible = "altr,edac";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		l2edac@ffd08140 {
+			compatible = "altr,l2-edac";
+			reg = <0xffd08140 0x4>;
+			interrupts = <0 36 1>, <0 37 1>;
+		};
+
+		ocramedac@ffd08144 {
+			compatible = "altr,ocram-edac";
+			reg = <0xffd08144 0x4>;
+			iram = <&ocram>;
+			interrupts = <0 178 1>, <0 179 1>;
+		};
+	};
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 252c3d1..e546e47 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -618,6 +618,26 @@
 			interrupts = <0 39 4>;
 		};
 
+		soc_ecc {
+			compatible = "altr,edac";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			l2edac@ffd08140 {
+				compatible = "altr,l2-edac";
+				reg = <0xffd08140 0x4>;
+				interrupts = <0 36 1>, <0 37 1>;
+			};
+
+			ocramedac@ffd08144 {
+				compatible = "altr,ocram-edac";
+				reg = <0xffd08144 0x4>;
+				iram = <&ocram>;
+				interrupts = <0 178 1>, <0 179 1>;
+			};
+		};
+
 		L2: l2-cache@fffef000 {
 			compatible = "arm,pl310-cache";
 			reg = <0xfffef000 0x1000>;
-- 
1.7.9.5

Re: [PATCHv6 0/5] Add Altera peripheral memories to EDAC framework

[Thor Thayer]

Hi Device Tree Maintainers,

On 01/08/2015 08:53 PM, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> This patch adds the L2 cache and OCRAM peripherals to the EDAC framework
> using the EDAC device framework. The ECC is enabled early in the boot
> process in the platform specific code.
>

The changes in this patch series revision were mainly to address device 
tree concerns. There were changes in other areas of the code to address 
these changes but I believe the other maintainers are waiting to see if 
these changes are accepted before they will review (they had approved 
the previous patch changes).

How does the this patch series appear from a device tree perspective?

Thank you for your time,

Thor


> v2 changes:
> - Split On-Chip RAM ECC platform initialization into separate patch from
>    L2 ECC platform initialization.
> - Fix L2 cache dependency comments.
> - Remove OCRAM node from dts and reference prior patch.
>
> v3 changes:
> - Move L2 cache & On-Chip RAM EDAC code into altera_edac.c
> - Remove SDRAM module compile.
>
> v4 changes:
> - Change mask defines to use BIT().
> - Fix comment style to agree with kernel coding style.
> - Better printk description for read != write in trigger.
> - Remove SysFS debugging message.
> - Better dci->mod_name
> - Move gen_pool pointer assignment to end of function.
> - Invert logic to reduce indent in ocram depenency check.
> - Change from dev_err() to edac_printk()
> - Replace magic numbers with defines & comments.
> - Improve error injection test.
> - Change Makefile intermediary name to altr (from alt)
>
> v5 changes:
> - Remove l2cache.h by using if (IS_ENABLED(CONFIG_EDAC_ALTERA_L2C))
> - Remove ocram.h by using if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> - Check prop variable before using. Include io.h.
> - Add defines for better readability. Remove MAINTAINERS changes.
>
> v6 changes:
> - Simplify OCRAM initialization. Remove be32_to_cpup() calls.
> - Remove syscon from L2 Cache. Force L2 Cache on if ECC enabled.
> - Convert to nested ECC in device tree.
> - Additional comments to clarify debug error injection.
>
> Thor Thayer (5):
>    arm: socfpga: Enable L2 Cache ECC on startup.
>    arm: socfpga: Enable OCRAM ECC on startup.
>    edac: altera: Remove SDRAM module compile
>    edac: altera: Add Altera L2 Cache and OCRAM EDAC Support
>    arm: dts: Add Altera L2 Cache and OCRAM EDAC entries
>
>   .../bindings/arm/altera/socfpga-edac.txt           |   46 ++
>   arch/arm/boot/dts/socfpga.dtsi                     |   20 +
>   arch/arm/mach-socfpga/Makefile                     |    2 +
>   arch/arm/mach-socfpga/core.h                       |    2 +
>   arch/arm/mach-socfpga/l2_cache.c                   |   39 ++
>   arch/arm/mach-socfpga/ocram.c                      |   97 ++++
>   arch/arm/mach-socfpga/socfpga.c                    |    4 +-
>   drivers/edac/Kconfig                               |   20 +-
>   drivers/edac/Makefile                              |    5 +-
>   drivers/edac/altera_edac.c                         |  506 +++++++++++++++++++-
>   10 files changed, 735 insertions(+), 6 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
>   create mode 100644 arch/arm/mach-socfpga/l2_cache.c
>   create mode 100644 arch/arm/mach-socfpga/ocram.c
>

[RESEND PATCHv6 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC entries

[Thor Thayer]

Hi Device Tree Maintainers,

On 01/08/2015 08:53 PM, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Adding the device tree entries and bindings needed to support
> the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
> an earlier patch to declare and setup On-chip RAM properly.
> http://www.spinics.net/lists/devicetree/msg51117.html
>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2: Remove OCRAM declaration and reference prior patch.
>
> v3-5: No Change
>
> v6: Change to nested EDAC device nodes based on community
>      feedback. Remove L2 syscon. Use consolidated binding.

I'm requesting comments on this patch series. The changes in this patch 
series are based upon feedback from Mark Rutland for patch series 
version 5 on December 2, 2014.

I believe this patch set addresses the concerns that Mark had with my 
previous patch. Primarily, syscon was removed from the L2 cache and a 
top level device tree node with L2 and OCRAM children is used instead of 
individual top level nodes. Some concerns were addressed in an email 
reply on December 2, 2014.

This change also created a new edac parent probe function which is in 
[PATCHv6 4/5] which should be reviewed along with this device tree change.

Thanks,

Thor

> ---
>   .../bindings/arm/altera/socfpga-edac.txt           |   46 ++++++++++++++++++++
>   arch/arm/boot/dts/socfpga.dtsi                     |   20 +++++++++
>   2 files changed, 66 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
> new file mode 100644
> index 0000000..4bf32e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
> @@ -0,0 +1,46 @@
> +Altera SoCFPGA Error Detection and Correction [EDAC]
> +
> +Required Properties:
> +- compatible : Should be "altr,edac"
> +- #address-cells: must be 1
> +- #size-cells: must be 1
> +- ranges : standard definition, should translate from local addresses
> +
> +Subcomponents:
> +
> +L2 Cache ECC
> +Required Properties:
> +- compatible : Should be "altr,l2-edac"
> +- reg : Address and size for ECC error interrupt clear registers.
> +- interrupts : Should be single bit error interrupt, then double bit error
> +	interrupt. Note the rising edge type.
> +
> +On Chip RAM ECC
> +Required Properties:
> +- compatible : Should be "altr,ocram-edac"
> +- reg : Address and size for ECC error interrupt clear registers.
> +- iram : phandle to On-Chip RAM definition.
> +- interrupts : Should be single bit error interrupt, then double bit error
> +	interrupt. Note the rising edge type.
> +
> +Example:
> +
> +	soc_ecc {
> +		compatible = "altr,edac";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		l2edac@ffd08140 {
> +			compatible = "altr,l2-edac";
> +			reg = <0xffd08140 0x4>;
> +			interrupts = <0 36 1>, <0 37 1>;
> +		};
> +
> +		ocramedac@ffd08144 {
> +			compatible = "altr,ocram-edac";
> +			reg = <0xffd08144 0x4>;
> +			iram = <&ocram>;
> +			interrupts = <0 178 1>, <0 179 1>;
> +		};
> +	};
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 252c3d1..e546e47 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -618,6 +618,26 @@
>   			interrupts = <0 39 4>;
>   		};
>
> +		soc_ecc {
> +			compatible = "altr,edac";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			l2edac@ffd08140 {
> +				compatible = "altr,l2-edac";
> +				reg = <0xffd08140 0x4>;
> +				interrupts = <0 36 1>, <0 37 1>;
> +			};
> +
> +			ocramedac@ffd08144 {
> +				compatible = "altr,ocram-edac";
> +				reg = <0xffd08144 0x4>;
> +				iram = <&ocram>;
> +				interrupts = <0 178 1>, <0 179 1>;
> +			};
> +		};
> +
>   		L2: l2-cache@fffef000 {
>   			compatible = "arm,pl310-cache";
>   			reg = <0xfffef000 0x1000>;
>

Re: [PATCHv6 2/5] arm: socfpga: Enable OCRAM ECC on startup.

[Mark Rutland]

On Fri, Jan 09, 2015 at 02:53:53AM +0000, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> This patch enables the ECC for On-Chip RAM on machine
> startup.  The ECC has to be enabled before data is
> is stored in memory otherwise the ECC will fail on
> reads.

Where else is this OCRAM used?

If we need the ECC to be enabled before use, a module_init call that
seems to be unrelated to any other usage doesn't seem right to me.
Hopefully I've just misunderstood something here.

> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2: Split OCRAM ECC portion separately. Addition of iounmap()
>     and reorganization of gen_pool_free. Remove defconfig from patch.
> 
> v3/4: No change
> 
> v5: Remove ocram.h, use io.h instead of clk-provider.h
>     Check prop in correct place. Add ECC EN defines.
> 
> v6: Implement OCRAM discovery changes from community. Add
>     of_node_put(). Remove be32_to_cpup(). Use __init() which
>     allows removal of .init_machine(). Update year in header.
> ---
>  arch/arm/mach-socfpga/Makefile |    1 +
>  arch/arm/mach-socfpga/ocram.c  |   97 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 98 insertions(+)
>  create mode 100644 arch/arm/mach-socfpga/ocram.c
> 
> diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
> index 142609e..1552ca5 100644
> --- a/arch/arm/mach-socfpga/Makefile
> +++ b/arch/arm/mach-socfpga/Makefile
> @@ -5,3 +5,4 @@
>  obj-y					:= socfpga.o
>  obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
>  obj-$(CONFIG_EDAC_ALTERA_L2C) += l2_cache.o
> +obj-$(CONFIG_EDAC_ALTERA_OCRAM) += ocram.o
> diff --git a/arch/arm/mach-socfpga/ocram.c b/arch/arm/mach-socfpga/ocram.c
> new file mode 100644
> index 0000000..1f12a38
> --- /dev/null
> +++ b/arch/arm/mach-socfpga/ocram.c
> @@ -0,0 +1,97 @@
> +/*
> + * Copyright Altera Corporation (C) 2015. All rights reserved.
> + *
> + * 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/>.
> + */
> +#include <linux/io.h>
> +#include <linux/genalloc.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#define ALTR_OCRAM_CLEAR_ECC          0x00000018
> +#define ALTR_OCRAM_ECC_EN             0x00000019
> +
> +static int __init socfpga_init_ocram_ecc(void)
> +{
> +	struct device_node *np;
> +	struct resource    res;
> +	u32                iram_addr;
> +	void __iomem       *mapped_ocr_edac_addr;
> +	resource_size_t    size;
> +	struct gen_pool    *gp;
> +	int                ret;
> +
> +	/* Get the size of the on-chip RAM */
> +	np = of_find_compatible_node(NULL, NULL, "mmio-sram");
> +	if (!np) {
> +		pr_err("%s: Unable to find mmio-sram in dtb\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +	if (ret) {
> +		of_node_put(np);
> +		pr_err("%s: Problem getting SRAM address in dtb\n", __func__);
> +		return -ENODEV;
> +	}
> +	size = resource_size(&res);
> +	of_node_put(np);
> +
> +	/* Find the OCRAM EDAC device tree node */
> +	np = of_find_compatible_node(NULL, NULL, "altr,ocram-edac");
> +	if (!np) {
> +		pr_err("%s: Unable to find altr,ocram-edac\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	mapped_ocr_edac_addr = of_iomap(np, 0);
> +	if (!mapped_ocr_edac_addr) {
> +		of_node_put(np);
> +		pr_err("%s: Unable to map OCRAM ecc regs.\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	gp = of_get_named_gen_pool(np, "iram", 0);
> +	if (!gp) {
> +		of_node_put(np);
> +		pr_err("%s: OCRAM cannot find gen pool\n", __func__);
> +		return -ENODEV;
> +	}
> +	of_node_put(np);
> +
> +	iram_addr = gen_pool_alloc(gp, size / sizeof(size_t));

Why divide by sizeof(size_t) here? As far as I am aware, resource_size
gives you a size in bytes...

> +	if (iram_addr == 0) {
> +		pr_err("%s: cannot alloc from gen pool\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	/* Clear any pending OCRAM ECC interrupts, then enable ECC */
> +	writel(ALTR_OCRAM_CLEAR_ECC, mapped_ocr_edac_addr);
> +	writel(ALTR_OCRAM_ECC_EN, mapped_ocr_edac_addr);
> +
> +	memset((void *)iram_addr, 0, size);

...and here we write size bytes, not (size / sizeof(size_t)) bytes, so
we're poking memory we weren't allocated.

How is this memory mapped exactly? Is memset safe?

Thanks,
Mark.

> +
> +	gen_pool_free(gp, iram_addr, size / sizeof(size_t));
> +
> +	iounmap(mapped_ocr_edac_addr);
> +
> +	return 0;
> +}
> +
> +static void __exit socfpga_exit_ocram_ecc(void)
> +{
> +}
> +
> +module_init(socfpga_init_ocram_ecc);
> +module_exit(socfpga_exit_ocram_ecc);
> -- 
> 1.7.9.5
> 
> 

Re: [PATCHv6 1/5] arm: socfpga: Enable L2 Cache ECC on startup.

[Mark Rutland]

On Fri, Jan 09, 2015 at 02:53:52AM +0000, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> This patch enables the ECC for L2 cache on machine
> startup.  The ECC has to be enabled before data is
> is stored in memory otherwise the ECC will fail on
> reads.

I take it you mean before the L2 cache is enabled (and its memories are
used), rather than before main memory is in use?

> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2: Split OCRAM initialization into separate patch.
> 
> v3/4: No change
> 
> v5: Remove l2cache.h, use io.h instead of clk-provider.h
>     Make copyright header inclusive. Remove MAINTAINERS entry.
> 
> v6: Remove pr_debug() & update year in header.
> ---
>  arch/arm/mach-socfpga/Makefile   |    1 +
>  arch/arm/mach-socfpga/core.h     |    2 ++
>  arch/arm/mach-socfpga/l2_cache.c |   39 ++++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-socfpga/socfpga.c  |    4 +++-
>  4 files changed, 45 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/mach-socfpga/l2_cache.c
> 
> diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
> index 6dd7a93..142609e 100644
> --- a/arch/arm/mach-socfpga/Makefile
> +++ b/arch/arm/mach-socfpga/Makefile
> @@ -4,3 +4,4 @@
>  
>  obj-y					:= socfpga.o
>  obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
> +obj-$(CONFIG_EDAC_ALTERA_L2C) += l2_cache.o
> diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
> index 483cb46..28c8a15 100644
> --- a/arch/arm/mach-socfpga/core.h
> +++ b/arch/arm/mach-socfpga/core.h
> @@ -47,4 +47,6 @@ extern unsigned long socfpga_cpu1start_addr;
>  
>  #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
>  
> +void socfpga_init_l2_ecc(void);
> +
>  #endif
> diff --git a/arch/arm/mach-socfpga/l2_cache.c b/arch/arm/mach-socfpga/l2_cache.c
> new file mode 100644
> index 0000000..047759d
> --- /dev/null
> +++ b/arch/arm/mach-socfpga/l2_cache.c
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright Altera Corporation (C) 2015. All rights reserved.
> + *
> + * 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/>.
> + */
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +
> +void socfpga_init_l2_ecc(void)
> +{
> +	struct device_node *np;
> +	void __iomem  *mapped_l2_edac_addr;
> +
> +	np = of_find_compatible_node(NULL, NULL, "altr,l2-edac");
> +	if (!np) {
> +		pr_err("SOCFPGA: Unable to find altr,l2-edac in dtb\n");
> +		return;
> +	}
> +
> +	mapped_l2_edac_addr = of_iomap(np, 0);
> +	if (!mapped_l2_edac_addr) {
> +		pr_err("SOCFPGA: Unable to find L2 ECC mapping in dtb\n");
> +		return;
> +	}
> +
> +	/* Enable ECC */
> +	writel(0x01, mapped_l2_edac_addr);

Missing of_node_put(np)?

Surely you're leaking the mapping here? It's locally scoped and you
never unmap it.

Thanks,
Mark.

Re: [PATCHv6 4/5] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support

[Mark Rutland]

On Fri, Jan 09, 2015 at 02:53:55AM +0000, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> Adding L2 Cache and On-Chip RAM EDAC support for the
> Altera SoCs using the EDAC device  model. The SDRAM
> controller is using the Memory Controller model.
> 
> Each type of ECC is individually configurable.
> 
> The SDRAM ECC is a separate Kconfig option because:
> 1) the SDRAM preparation can take almost 2 seconds on boot and some
> customers need a faster boot time.
> 2) the SDRAM has an ECC initialization dependency on the preloader
> which is outside the kernel. It is desirable to be able to turn the
> SDRAM on & off separately.
> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2: Fix L2 dependency comments.
> 
> v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c
>     instead of separate files.
> 
> v4: Change mask defines to use BIT().
>     Fix comment style to agree with kernel coding style.
>     Better printk description for read != write in trigger.
>     Remove SysFS debugging message.
>     Better dci->mod_name
>     Move gen_pool pointer assignment to end of function.
>     Invert logic to reduce indent in ocram depenency check.
>     Change from dev_err() to edac_printk()
>     Replace magic numbers with defines & comments.
>     Improve error injection test.
>     Change Makefile intermediary name to altr (from alt)
> 
> v5: No change.
> 
> v6: Convert to nested EDAC in device tree. Force L2 cache
>     on for L2Cache ECC & remove L2 cache syscon for checking
>     enable bit. Update year in header.
> ---
>  drivers/edac/Kconfig       |   16 ++
>  drivers/edac/Makefile      |    5 +-
>  drivers/edac/altera_edac.c |  506 +++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 524 insertions(+), 3 deletions(-)

[...]

> +/************************* EDAC Parent Probe *************************/
> +
> +static const struct of_device_id altr_edac_device_of_match[];

Huh? What's this for?

> +static const struct of_device_id altr_edac_of_match[] = {
> +       { .compatible = "altr,edac" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, altr_edac_of_match);

I know it may seem like a minor thing, but the documentation really
should have come in an earlier patch. It's painful to review a patch
series when you have to randomly just to the end of hte series to see
the documentation.

The name is _very_ generic here. Do we not have a more specific name for
the EDAC block in your SoC?

Is there actually a specific EDAC device, or are you just grouping some
portions of HW blocks into an EDAC device to match what the Linux EDAC
framework wants?

[...]

> +ssize_t altr_edac_device_trig(struct edac_device_ctl_info *edac_dci,
> +                             const char *buffer, size_t count)
> +{
> +       u32 *ptemp, i, error_mask;
> +       int result = 0;
> +       unsigned long flags;
> +       struct altr_edac_device_dev *drvdata = edac_dci->pvt_info;
> +       const struct edac_device_prv_data *priv = drvdata->data;
> +       void *generic_ptr = edac_dci->dev;

Huh? What's hidden behind this "generic_ptr"?

> +
> +       if (!priv->alloc_mem)
> +               return -ENOMEM;
> +
> +       /*
> +        * Note that generic_ptr is initialized to the device * but in
> +        * some alloc_functions, this is overridden and returns data.
> +        */
> +       ptemp = priv->alloc_mem(priv->trig_alloc_sz, &generic_ptr);
> +       if (!ptemp) {
> +               edac_printk(KERN_ERR, EDAC_DEVICE,
> +                           "Inject: Buffer Allocation error\n");
> +               return -ENOMEM;
> +       }
> +
> +       if (buffer[0] == ALTR_UE_TRIGGER_CHAR)
> +               error_mask = priv->ue_set_mask;
> +       else
> +               error_mask = priv->ce_set_mask;
> +
> +       edac_printk(KERN_ALERT, EDAC_DEVICE,
> +                   "Trigger Error Mask (0x%X)\n", error_mask);
> +
> +       local_irq_save(flags);
> +       /* write ECC corrupted data out. */
> +       for (i = 0; i < (priv->trig_alloc_sz / sizeof(*ptemp)); i++) {
> +               /* Read data so we're in the correct state */
> +               rmb();
> +               if (ACCESS_ONCE(ptemp[i]))
> +                       result = -1;

Perhaps s/result/err/? You could even have an err_count, which would
give you slightly more useful output.

> +               /* Toggle Error bit (it is latched), leave ECC enabled */
> +               writel(error_mask, drvdata->base);
> +               writel(priv->ecc_enable_mask, drvdata->base);
> +               ptemp[i] = i;
> +       }
> +       /* Ensure it has been written out */
> +       wmb();
> +       local_irq_restore(flags);
> +
> +       if (result)
> +               edac_printk(KERN_ERR, EDAC_DEVICE, "Mem Not Cleared\n");
> +
> +       /* Read out written data. ECC error caused here */
> +       for (i = 0; i < ALTR_TRIGGER_READ_WRD_CNT; i++)
> +               if (ACCESS_ONCE(ptemp[i]) != i)
> +                       edac_printk(KERN_ERR, EDAC_DEVICE,
> +                                   "Read doesn't match written data\n");
> +
> +       if (priv->free_mem)
> +               priv->free_mem(ptemp, priv->trig_alloc_sz, generic_ptr);
> +
> +       return count;
> +}

[...]

> +static void *ocram_alloc_mem(size_t size, void **other)
> +{
> +       struct device_node *np;
> +       struct gen_pool *gp;
> +       void *sram_addr;
> +
> +       np = of_find_compatible_node(NULL, NULL, "altr,ocram-edac");
> +       if (!np)
> +               return NULL;
> +
> +       gp = of_get_named_gen_pool(np, "iram", 0);
> +       if (!gp) {
> +               of_node_put(np);
> +               return NULL;
> +       }
> +       of_node_put(np);
> +
> +       sram_addr = (void *)gen_pool_alloc(gp, size / sizeof(size_t));

I'm still very much confused by this sizeof(size_t) division, but
hopefully your response to my earlier query will answer that.

[...]

> +static void *l2_alloc_mem(size_t size, void **other)
> +{
> +       struct device *dev = *other;
> +       void *ptemp = devm_kzalloc(dev, size, GFP_KERNEL);
> +
> +       if (!ptemp)
> +               return NULL;
> +
> +       /* Make sure everything is written out */
> +       wmb();
> +
> +       /*
> +        * Clean all cache levels up to LoC (includes L2)
> +        * This ensures the corrupted data is written into
> +        * L2 cache for readback test (which causes ECC error).
> +        */
> +       flush_cache_all();

I'm a little confused by this comment (especially w.r.t. the L2 being
before the LoC). Are we attempting to flush everything _to_ the L2, or
beyond/out-of the L2? As far as I can see flush_cache_all will only
achieve the former, and not the latter, as it doesn't seem to perform
any outer cache operations.

Per the architecture, Set/Way maintenance of this form won't always act
as you expect (though I believe that for Cortex-A9 it does).

> +
> +       return ptemp;
> +}
> +
> +static void l2_free_mem(void *p, size_t size, void *other)
> +{
> +       struct device *dev = other;
> +
> +       if (dev && p)
> +               devm_kfree(dev, p);

Is this ever called in that case?

Thanks,
Mark.

Re: [PATCHv6 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC entries

[Mark Rutland]

On Fri, Jan 09, 2015 at 02:53:56AM +0000, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> Adding the device tree entries and bindings needed to support
> the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
> an earlier patch to declare and setup On-chip RAM properly.
> http://www.spinics.net/lists/devicetree/msg51117.html
> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> v2: Remove OCRAM declaration and reference prior patch.
> 
> v3-5: No Change
> 
> v6: Change to nested EDAC device nodes based on community
>     feedback. Remove L2 syscon. Use consolidated binding.
> ---
>  .../bindings/arm/altera/socfpga-edac.txt           |   46 ++++++++++++++++++++
>  arch/arm/boot/dts/socfpga.dtsi                     |   20 +++++++++
>  2 files changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
> new file mode 100644
> index 0000000..4bf32e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
> @@ -0,0 +1,46 @@
> +Altera SoCFPGA Error Detection and Correction [EDAC]
> +
> +Required Properties:
> +- compatible : Should be "altr,edac"

Is there a single large EDAC block that happens to perform EDAC
functions for various components, or various small EDAC blocks that
happen to be close to each other in the MMIO space?

> +- #address-cells: must be 1
> +- #size-cells: must be 1
> +- ranges : standard definition, should translate from local addresses

Surely these just need to be suitable for mapping the child nodes, and
you shouldn't care about their precise values?

> +
> +Subcomponents:
> +
> +L2 Cache ECC
> +Required Properties:
> +- compatible : Should be "altr,l2-edac"
> +- reg : Address and size for ECC error interrupt clear registers.
> +- interrupts : Should be single bit error interrupt, then double bit error
> +	interrupt. Note the rising edge type.
> +
> +On Chip RAM ECC
> +Required Properties:
> +- compatible : Should be "altr,ocram-edac"
> +- reg : Address and size for ECC error interrupt clear registers.
> +- iram : phandle to On-Chip RAM definition.
> +- interrupts : Should be single bit error interrupt, then double bit error
> +	interrupt. Note the rising edge type.

Do these actually differ in programming interface, or just w.r.t. the
component they are used to monitor?

It would be good to have an explicit link to what they monitor rather
than relying on there being single instances with particular compatible
strings. That will make this easier to generalise for multiple instances
in future SoCs that may reuse the EDAC block.

This doesn't look too bad, but I'd like to hear some response to this
and my other queries before I can say whether this is a good way of
describing the hardware; it all looks a bit vague.

As an aside, please place the documentation at the start of the series,
before the driver rework. The dts patches can come after the docs and
code patches.

Thanks,
Mark.

> +
> +Example:
> +
> +	soc_ecc {
> +		compatible = "altr,edac";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		l2edac@ffd08140 {
> +			compatible = "altr,l2-edac";
> +			reg = <0xffd08140 0x4>;
> +			interrupts = <0 36 1>, <0 37 1>;
> +		};
> +
> +		ocramedac@ffd08144 {
> +			compatible = "altr,ocram-edac";
> +			reg = <0xffd08144 0x4>;
> +			iram = <&ocram>;
> +			interrupts = <0 178 1>, <0 179 1>;
> +		};
> +	};
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 252c3d1..e546e47 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -618,6 +618,26 @@
>  			interrupts = <0 39 4>;
>  		};
>  
> +		soc_ecc {
> +			compatible = "altr,edac";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			l2edac@ffd08140 {
> +				compatible = "altr,l2-edac";
> +				reg = <0xffd08140 0x4>;
> +				interrupts = <0 36 1>, <0 37 1>;
> +			};
> +
> +			ocramedac@ffd08144 {
> +				compatible = "altr,ocram-edac";
> +				reg = <0xffd08144 0x4>;
> +				iram = <&ocram>;
> +				interrupts = <0 178 1>, <0 179 1>;
> +			};
> +		};
> +
>  		L2: l2-cache@fffef000 {
>  			compatible = "arm,pl310-cache";
>  			reg = <0xfffef000 0x1000>;
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCHv6 0/5] Add Altera peripheral memories to EDAC framework

[Mark Rutland]

On Thu, Jan 29, 2015 at 08:53:06PM +0000, Thor Thayer wrote:
> Hi Device Tree Maintainers,

Hi Thor,

Apologies for being silent for so long on this.

> On 01/08/2015 08:53 PM, tthayer@opensource.altera.com wrote:
> > From: Thor Thayer <tthayer@opensource.altera.com>
> >
> > This patch adds the L2 cache and OCRAM peripherals to the EDAC framework
> > using the EDAC device framework. The ECC is enabled early in the boot
> > process in the platform specific code.
> >
> 
> The changes in this patch series revision were mainly to address device 
> tree concerns. There were changes in other areas of the code to address 
> these changes but I believe the other maintainers are waiting to see if 
> these changes are accepted before they will review (they had approved 
> the previous patch changes).
> 
> How does the this patch series appear from a device tree perspective?

While this looks better than before (thank you for no longer treating
the PL310 registers as a syscon), I have a couple of quibbles with the
binding:

* It's not clear to me whether the L2 and OCRAM EDACs are fundamentally
  different in interface, or whether they simply happen to be monitoring
  different memories. The former would mean a common compatible string
  for both EDAC block instances, and another property to determine
  what the other end was.

* It relies on a single instance of OCRAM or L2 existing, rather than
  describing the relationship with the particular memory explicitly. In
  general it's better to be explicit -- this means it can more easily be
  generalised to multiple OCRAMs as might occur in a later SoC (in
  addition to the reasons laid out in the first point.

Thanks,
Mark.

Re: [PATCHv6 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC entries

[Thor Thayer]

Hi Mark

On 02/06/2015 01:24 PM, Mark Rutland wrote:
> On Fri, Jan 09, 2015 at 02:53:56AM +0000, tthayer@opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Adding the device tree entries and bindings needed to support
>> the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
>> an earlier patch to declare and setup On-chip RAM properly.
>> http://www.spinics.net/lists/devicetree/msg51117.html
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v2: Remove OCRAM declaration and reference prior patch.
>>
>> v3-5: No Change
>>
>> v6: Change to nested EDAC device nodes based on community
>>      feedback. Remove L2 syscon. Use consolidated binding.
>> ---
>>   .../bindings/arm/altera/socfpga-edac.txt           |   46 ++++++++++++++++++++
>>   arch/arm/boot/dts/socfpga.dtsi                     |   20 +++++++++
>>   2 files changed, 66 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
>> new file mode 100644
>> index 0000000..4bf32e1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
>> @@ -0,0 +1,46 @@
>> +Altera SoCFPGA Error Detection and Correction [EDAC]
>> +
>> +Required Properties:
>> +- compatible : Should be "altr,edac"
>
> Is there a single large EDAC block that happens to perform EDAC
> functions for various components, or various small EDAC blocks that
> happen to be close to each other in the MMIO space?
>

There is a sequential grouping of ECC registers containing IP specific 
ECC enable and ECC error injection bits. A number of these registers 
have the same bit definitions but a few such as L2 cache are different.

If I understand your question, I'd characterize these as various small 
EDAC blocks that happen to be close to each other in the MMIO space.

There is no master - these IP memories are meant to be individually enabled.

>> +- #address-cells: must be 1
>> +- #size-cells: must be 1
>> +- ranges : standard definition, should translate from local addresses
>
> Surely these just need to be suitable for mapping the child nodes, and
> you shouldn't care about their precise values?
>

If you are saying that I place the base address in the parent and then 
just include the offset from the parent registers [0 for L2 cache and 4 
for OCRAM] in each child then I'll need to research how to do that. I 
think our clock manager may do this.

>> +
>> +Subcomponents:
>> +
>> +L2 Cache ECC
>> +Required Properties:
>> +- compatible : Should be "altr,l2-edac"
>> +- reg : Address and size for ECC error interrupt clear registers.
>> +- interrupts : Should be single bit error interrupt, then double bit error
>> +	interrupt. Note the rising edge type.
>> +
>> +On Chip RAM ECC
>> +Required Properties:
>> +- compatible : Should be "altr,ocram-edac"
>> +- reg : Address and size for ECC error interrupt clear registers.
>> +- iram : phandle to On-Chip RAM definition.
>> +- interrupts : Should be single bit error interrupt, then double bit error
>> +	interrupt. Note the rising edge type.
>
> Do these actually differ in programming interface, or just w.r.t. the
> component they are used to monitor?

The interface is similar but the bit definitions for ECC Enable and bit 
injection are different between these two IPs. However, these 
differences are taken care of with different const data structures in 
altera_edac.c.

>
> It would be good to have an explicit link to what they monitor rather
> than relying on there being single instances with particular compatible
> strings. That will make this easier to generalise for multiple instances
> in future SoCs that may reuse the EDAC block.
>

You're suggesting a phandle to the L2 cache similar to the phandle the 
OCRAM uses? The probe function would then grab the appropriate const 
data structure based on where the phandle pointed, correct?

> This doesn't look too bad, but I'd like to hear some response to this
> and my other queries before I can say whether this is a good way of
> describing the hardware; it all looks a bit vague.
>
> As an aside, please place the documentation at the start of the series,
> before the driver rework. The dts patches can come after the docs and
> code patches.
>

I will do that. Thanks for reviewing.

> Thanks,
> Mark.
>
>> +
>> +Example:
>> +
>> +	soc_ecc {
>> +		compatible = "altr,edac";
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges;
>> +
>> +		l2edac@ffd08140 {
>> +			compatible = "altr,l2-edac";
>> +			reg = <0xffd08140 0x4>;
>> +			interrupts = <0 36 1>, <0 37 1>;
>> +		};
>> +
>> +		ocramedac@ffd08144 {
>> +			compatible = "altr,ocram-edac";
>> +			reg = <0xffd08144 0x4>;
>> +			iram = <&ocram>;
>> +			interrupts = <0 178 1>, <0 179 1>;
>> +		};
>> +	};
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index 252c3d1..e546e47 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -618,6 +618,26 @@
>>   			interrupts = <0 39 4>;
>>   		};
>>
>> +		soc_ecc {
>> +			compatible = "altr,edac";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			ranges;
>> +
>> +			l2edac@ffd08140 {
>> +				compatible = "altr,l2-edac";
>> +				reg = <0xffd08140 0x4>;
>> +				interrupts = <0 36 1>, <0 37 1>;
>> +			};
>> +
>> +			ocramedac@ffd08144 {
>> +				compatible = "altr,ocram-edac";
>> +				reg = <0xffd08144 0x4>;
>> +				iram = <&ocram>;
>> +				interrupts = <0 178 1>, <0 179 1>;
>> +			};
>> +		};
>> +
>>   		L2: l2-cache@fffef000 {
>>   			compatible = "arm,pl310-cache";
>>   			reg = <0xfffef000 0x1000>;
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCHv6 2/5] arm: socfpga: Enable OCRAM ECC on startup.

[Thor Thayer]

Hi Mark,

On 02/06/2015 12:45 PM, Mark Rutland wrote:
> On Fri, Jan 09, 2015 at 02:53:53AM +0000, tthayer@opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> This patch enables the ECC for On-Chip RAM on machine
>> startup.  The ECC has to be enabled before data is
>> is stored in memory otherwise the ECC will fail on
>> reads.
>
> Where else is this OCRAM used?
>
> If we need the ECC to be enabled before use, a module_init call that
> seems to be unrelated to any other usage doesn't seem right to me.
> Hopefully I've just misunderstood something here.
>

Thank you for reviewing this.

The OCRAM will be used to store data and functions for putting and 
removing the SoC from sleep. However, in this scenario, we won't have 
the ECC enabled.

To initialize ECC, the OCRAM needs to enable ECC then clear the entire 
memory to zero before using it. Doing this early in the startup sequence 
seemed appropriate. Maybe I'm misunderstanding your concern.

< snip>

>> +static int __init socfpga_init_ocram_ecc(void)
>> +{
>> +	struct device_node *np;
>> +	struct resource    res;
>> +	u32                iram_addr;
>> +	void __iomem       *mapped_ocr_edac_addr;
>> +	resource_size_t    size;
>> +	struct gen_pool    *gp;
>> +	int                ret;
>> +
>> +	/* Get the size of the on-chip RAM */
>> +	np = of_find_compatible_node(NULL, NULL, "mmio-sram");
>> +	if (!np) {
>> +		pr_err("%s: Unable to find mmio-sram in dtb\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = of_address_to_resource(np, 0, &res);
>> +	if (ret) {
>> +		of_node_put(np);
>> +		pr_err("%s: Problem getting SRAM address in dtb\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +	size = resource_size(&res);
>> +	of_node_put(np);
>> +
>> +	/* Find the OCRAM EDAC device tree node */
>> +	np = of_find_compatible_node(NULL, NULL, "altr,ocram-edac");
>> +	if (!np) {
>> +		pr_err("%s: Unable to find altr,ocram-edac\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	mapped_ocr_edac_addr = of_iomap(np, 0);
>> +	if (!mapped_ocr_edac_addr) {
>> +		of_node_put(np);
>> +		pr_err("%s: Unable to map OCRAM ecc regs.\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	gp = of_get_named_gen_pool(np, "iram", 0);
>> +	if (!gp) {
>> +		of_node_put(np);
>> +		pr_err("%s: OCRAM cannot find gen pool\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +	of_node_put(np);
>> +
>> +	iram_addr = gen_pool_alloc(gp, size / sizeof(size_t));
>
> Why divide by sizeof(size_t) here? As far as I am aware, resource_size
> gives you a size in bytes...
>

Yes, you are right. I shouldn't have changed this - for some reason when 
I re-read the function prototype, I thought this function was allocating 
integers. Thank you.


>> +	if (iram_addr == 0) {
>> +		pr_err("%s: cannot alloc from gen pool\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* Clear any pending OCRAM ECC interrupts, then enable ECC */
>> +	writel(ALTR_OCRAM_CLEAR_ECC, mapped_ocr_edac_addr);
>> +	writel(ALTR_OCRAM_ECC_EN, mapped_ocr_edac_addr);
>> +
>> +	memset((void *)iram_addr, 0, size);
>
> ...and here we write size bytes, not (size / sizeof(size_t)) bytes, so
> we're poking memory we weren't allocated.
>
> How is this memory mapped exactly? Is memset safe?
>
> Thanks,
> Mark.
>

Yes, thank you for catching that.

OK. I think I understand your point now. I need to enable the OCRAM ECC 
before the gen_pool_create() which is called by the 
"ocram:sram@ffff0000" node.

In that case, I should move this OCRAM ECC enable into the same place as 
L2 ecc is enabled.

Thor

>> +
>> +	gen_pool_free(gp, iram_addr, size / sizeof(size_t));
>> +
>> +	iounmap(mapped_ocr_edac_addr);
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit socfpga_exit_ocram_ecc(void)
>> +{
>> +}
>> +
>> +module_init(socfpga_init_ocram_ecc);
>> +module_exit(socfpga_exit_ocram_ecc);
>> --
>> 1.7.9.5
>>
>>

Re: [PATCHv6 4/5] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

On 02/06/2015 01:17 PM, Mark Rutland wrote:
> On Fri, Jan 09, 2015 at 02:53:55AM +0000, tthayer@opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Adding L2 Cache and On-Chip RAM EDAC support for the
>> Altera SoCs using the EDAC device  model. The SDRAM
>> controller is using the Memory Controller model.
>>
>> Each type of ECC is individually configurable.
>>
>> The SDRAM ECC is a separate Kconfig option because:
>> 1) the SDRAM preparation can take almost 2 seconds on boot and some
>> customers need a faster boot time.
>> 2) the SDRAM has an ECC initialization dependency on the preloader
>> which is outside the kernel. It is desirable to be able to turn the
>> SDRAM on & off separately.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>> v2: Fix L2 dependency comments.
>>
>> v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c
>>      instead of separate files.
>>
>> v4: Change mask defines to use BIT().
>>      Fix comment style to agree with kernel coding style.
>>      Better printk description for read != write in trigger.
>>      Remove SysFS debugging message.
>>      Better dci->mod_name
>>      Move gen_pool pointer assignment to end of function.
>>      Invert logic to reduce indent in ocram depenency check.
>>      Change from dev_err() to edac_printk()
>>      Replace magic numbers with defines & comments.
>>      Improve error injection test.
>>      Change Makefile intermediary name to altr (from alt)
>>
>> v5: No change.
>>
>> v6: Convert to nested EDAC in device tree. Force L2 cache
>>      on for L2Cache ECC & remove L2 cache syscon for checking
>>      enable bit. Update year in header.
>> ---
>>   drivers/edac/Kconfig       |   16 ++
>>   drivers/edac/Makefile      |    5 +-
>>   drivers/edac/altera_edac.c |  506 +++++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 524 insertions(+), 3 deletions(-)
>
> [...]
>
>> +/************************* EDAC Parent Probe *************************/
>> +
>> +static const struct of_device_id altr_edac_device_of_match[];
>
> Huh? What's this for?

I needed this as a parameter for the of_platform_populate() function in 
altr_edac_probe(). I will change the naming to prevent misunderstanding.

>
>> +static const struct of_device_id altr_edac_of_match[] = {
>> +       { .compatible = "altr,edac" },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, altr_edac_of_match);
>
> I know it may seem like a minor thing, but the documentation really
> should have come in an earlier patch. It's painful to review a patch
> series when you have to randomly just to the end of hte series to see
> the documentation.
>
> The name is _very_ generic here. Do we not have a more specific name for
> the EDAC block in your SoC?
>
> Is there actually a specific EDAC device, or are you just grouping some
> portions of HW blocks into an EDAC device to match what the Linux EDAC
> framework wants?
>
> [...]
>

Yes, I can move the documentation - I understand better from the 5/5 
email what you are looking for.

I can change the name to ECC Manager. There are a group of consecutive 
registers to enable and disable ECC for peripherals. Unfortunately, the 
SDRAM register is in a completely different area (not grouped with these 
peripherals) and seems better suited to the Memory Controller EDAC 
instead of a device EDAC for the peripherals.

There is not a specific EDAC device. I grouped these HW blocks into 
different instances of an EDAC device to match what I though the Linux 
EDAC framework wanted.

>> +ssize_t altr_edac_device_trig(struct edac_device_ctl_info *edac_dci,
>> +                             const char *buffer, size_t count)
>> +{
>> +       u32 *ptemp, i, error_mask;
>> +       int result = 0;
>> +       unsigned long flags;
>> +       struct altr_edac_device_dev *drvdata = edac_dci->pvt_info;
>> +       const struct edac_device_prv_data *priv = drvdata->data;
>> +       void *generic_ptr = edac_dci->dev;
>
> Huh? What's hidden behind this "generic_ptr"?
>

There are 2 memory allocation functions (ocram & L2) that return a 
pointer to the data. The OCRAM allocation function returns the gen_pool 
pointer in the generic_ptr so that it can be freed later.

>> +
>> +       if (!priv->alloc_mem)
>> +               return -ENOMEM;
>> +
>> +       /*
>> +        * Note that generic_ptr is initialized to the device * but in
>> +        * some alloc_functions, this is overridden and returns data.
>> +        */
>> +       ptemp = priv->alloc_mem(priv->trig_alloc_sz, &generic_ptr);
>> +       if (!ptemp) {
>> +               edac_printk(KERN_ERR, EDAC_DEVICE,
>> +                           "Inject: Buffer Allocation error\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       if (buffer[0] == ALTR_UE_TRIGGER_CHAR)
>> +               error_mask = priv->ue_set_mask;
>> +       else
>> +               error_mask = priv->ce_set_mask;
>> +
>> +       edac_printk(KERN_ALERT, EDAC_DEVICE,
>> +                   "Trigger Error Mask (0x%X)\n", error_mask);
>> +
>> +       local_irq_save(flags);
>> +       /* write ECC corrupted data out. */
>> +       for (i = 0; i < (priv->trig_alloc_sz / sizeof(*ptemp)); i++) {
>> +               /* Read data so we're in the correct state */
>> +               rmb();
>> +               if (ACCESS_ONCE(ptemp[i]))
>> +                       result = -1;
>
> Perhaps s/result/err/? You could even have an err_count, which would
> give you slightly more useful output.

No problem.

>
>> +               /* Toggle Error bit (it is latched), leave ECC enabled */
>> +               writel(error_mask, drvdata->base);
>> +               writel(priv->ecc_enable_mask, drvdata->base);
>> +               ptemp[i] = i;
>> +       }
>> +       /* Ensure it has been written out */

<snip>

>> +
>> +       sram_addr = (void *)gen_pool_alloc(gp, size / sizeof(size_t));
>
> I'm still very much confused by this sizeof(size_t) division, but
> hopefully your response to my earlier query will answer that.
>
> [...]

Yes, my mistake.

>
>> +static void *l2_alloc_mem(size_t size, void **other)
>> +{
>> +       struct device *dev = *other;
>> +       void *ptemp = devm_kzalloc(dev, size, GFP_KERNEL);
>> +
>> +       if (!ptemp)
>> +               return NULL;
>> +
>> +       /* Make sure everything is written out */
>> +       wmb();
>> +
>> +       /*
>> +        * Clean all cache levels up to LoC (includes L2)
>> +        * This ensures the corrupted data is written into
>> +        * L2 cache for readback test (which causes ECC error).
>> +        */
>> +       flush_cache_all();
>
> I'm a little confused by this comment (especially w.r.t. the L2 being
> before the LoC). Are we attempting to flush everything _to_ the L2, or
> beyond/out-of the L2? As far as I can see flush_cache_all will only
> achieve the former, and not the latter, as it doesn't seem to perform
> any outer cache operations.
>
> Per the architecture, Set/Way maintenance of this form won't always act
> as you expect (though I believe that for Cortex-A9 it does).
>

Yes, these functions are for triggering an ECC error for testing L2 
cache reads. I need to ensure the cleared data is written to L2 cache 
because that is what I'm testing. The PL310 manual said the L2 cache was 
included in the LoC.

Our SoC is a Cortex-A9 but if there is a better way, I'm open to it.

>> +
>> +       return ptemp;
>> +}
>> +
>> +static void l2_free_mem(void *p, size_t size, void *other)
>> +{
>> +       struct device *dev = other;
>> +
>> +       if (dev && p)
>> +               devm_kfree(dev, p);
>
> Is this ever called in that case?

Yes, in altr_edac_device_trig(), the memory is allocated, the ECC errors 
are injected, the data is read back causing ECC error interrupts and the 
memory is freed. Would the standard kzalloc() and kfree() be betteer?

>
> Thanks,
> Mark.
>

Re: [PATCHv6 4/5] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support

[Russell King - ARM Linux]

On Thu, Jan 08, 2015 at 08:53:55PM -0600, tthayer@opensource.altera.com wrote:
> +static int altr_edac_device_probe(struct platform_device *pdev)
> +{
> +	struct edac_device_ctl_info *dci;
> +	struct altr_edac_device_dev *drvdata;
> +	struct resource *r;
> +	int res = 0;
> +	struct device_node *np = pdev->dev.of_node;
> +	char *ecc_name = (char *)np->name;
> +	static int dev_instance;
> +
> +	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE,
> +			    "Unable to open devm\n");
> +		return -ENOMEM;
> +	}

Why are you opening a devres group here?  The device model already opens
a devres group ready for the ->probe callback, which is released when
the device is unbound... hmm, see below though...

> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!r) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE,
> +			    "Unable to get mem resource\n");
> +		return -EPROBE_DEFER;

Why EPROBE_DEFER for a missing resource?

> +	}
> +
> +	if (!devm_request_mem_region(&pdev->dev, r->start, resource_size(r),
> +				     dev_name(&pdev->dev))) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE,
> +			    "%s:Error requesting mem region\n", ecc_name);
> +		return -EBUSY;
> +	}
> +
> +	dci = edac_device_alloc_ctl_info(sizeof(*drvdata), ecc_name,
> +					 1, ecc_name, 1, 0, NULL, 0,
> +					 dev_instance++);
> +
> +	if (!dci) {
> +		edac_printk(KERN_ERR, EDAC_DEVICE,
> +			    "%s: Unable to allocate EDAC device\n", ecc_name);
> +		return -ENOMEM;
> +	}
> +
> +	drvdata = dci->pvt_info;
> +	dci->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, dci);
> +	drvdata->edac_dev_name = ecc_name;
> +
> +	drvdata->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> +	if (!drvdata->base)
> +		goto err;

You could use devm_ioremap_resource() to simplify the mapping, resource
allocation and resource error handling.

> +
> +	/* Get driver specific data for this EDAC device */
> +	drvdata->data = of_match_node(altr_edac_device_of_match, np)->data;
> +
> +	/* Check specific dependencies for the module */
> +	if (drvdata->data->setup) {
> +		res = drvdata->data->setup(pdev, drvdata->base);
> +		if (res < 0)
> +			goto err;
> +	}
> +
> +	drvdata->sb_irq = platform_get_irq(pdev, 0);
> +	res = devm_request_irq(&pdev->dev, drvdata->sb_irq,
> +			       altr_edac_device_handler,
> +			       0, dev_name(&pdev->dev), dci);
> +	if (res < 0)
> +		goto err;
> +
> +	drvdata->db_irq = platform_get_irq(pdev, 1);
> +	res = devm_request_irq(&pdev->dev, drvdata->db_irq,
> +			       altr_edac_device_handler,
> +			       0, dev_name(&pdev->dev), dci);
> +	if (res < 0)
> +		goto err;
> +
> +	dci->mod_name = "Altera EDAC Memory";
> +	dci->dev_name = drvdata->edac_dev_name;
> +
> +	altr_set_sysfs_attr(dci, drvdata->data);
> +
> +	if (edac_device_add_device(dci))
> +		goto err;
> +
> +	devres_close_group(&pdev->dev, NULL);
> +
> +	return 0;
> +err:
> +	edac_printk(KERN_ERR, EDAC_DEVICE,
> +		    "%s:Error setting up EDAC device: %d\n", ecc_name, res);
> +	devres_release_group(&pdev->dev, NULL);
> +	edac_device_free_ctl_info(dci);

Hmm, I guess this is why you're using devm groups - it seems like
edac allocation/freeing needs to be improved to work cooperatively
with devm_* APIs rather than having to add devm group complexity
into drivers.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.