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

[PATCHv5 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.

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

 .../bindings/arm/altera/socfpga-l2-edac.txt        |   15 +
 .../bindings/arm/altera/socfpga-ocram-edac.txt     |   16 +
 arch/arm/boot/dts/socfpga.dtsi                     |   15 +-
 arch/arm/mach-socfpga/Makefile                     |    2 +
 arch/arm/mach-socfpga/core.h                       |    3 +
 arch/arm/mach-socfpga/l2_cache.c                   |   41 ++
 arch/arm/mach-socfpga/ocram.c                      |   90 ++++
 arch/arm/mach-socfpga/socfpga.c                    |   13 +-
 drivers/edac/Kconfig                               |   18 +-
 drivers/edac/Makefile                              |    5 +-
 drivers/edac/altera_edac.c                         |  481 +++++++++++++++++++-
 11 files changed, 693 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
 create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-ocram-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

[PATCHv5 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.
---
 arch/arm/mach-socfpga/Makefile   |    1 +
 arch/arm/mach-socfpga/core.h     |    2 ++
 arch/arm/mach-socfpga/l2_cache.c |   41 ++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-socfpga/socfpga.c  |    4 +++-
 4 files changed, 47 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 572b8f7..385baba 100644
--- a/arch/arm/mach-socfpga/core.h
+++ b/arch/arm/mach-socfpga/core.h
@@ -44,4 +44,6 @@ extern unsigned long 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..3461745
--- /dev/null
+++ b/arch/arm/mach-socfpga/l2_cache.c
@@ -0,0 +1,41 @@
+/*
+ * Copyright Altera Corporation (C) 2014. 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);
+
+	pr_debug("SOCFPGA: Success Initializing L2 cache ECC\n");
+}
diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index adbf383..0954011 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-2014 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

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

[Dinh Nguyen]

On 11/11/14, 6:14 PM, 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.
> 
> 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.
> ---
>  arch/arm/mach-socfpga/Makefile   |    1 +
>  arch/arm/mach-socfpga/core.h     |    2 ++
>  arch/arm/mach-socfpga/l2_cache.c |   41 ++++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-socfpga/socfpga.c  |    4 +++-
>  4 files changed, 47 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/mach-socfpga/l2_cache.c
> 

Acked-by: Dinh Nguyen <dinguyen@opensource.altera.com>

[PATCHv5 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.
---
 arch/arm/mach-socfpga/Makefile  |    1 +
 arch/arm/mach-socfpga/core.h    |    1 +
 arch/arm/mach-socfpga/ocram.c   |   90 +++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-socfpga/socfpga.c |    9 ++++
 4 files changed, 101 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/core.h b/arch/arm/mach-socfpga/core.h
index 385baba..f3eee32 100644
--- a/arch/arm/mach-socfpga/core.h
+++ b/arch/arm/mach-socfpga/core.h
@@ -45,5 +45,6 @@ extern unsigned long cpu1start_addr;
 #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
 
 void socfpga_init_l2_ecc(void);
+void socfpga_init_ocram_ecc(void);
 
 #endif
diff --git a/arch/arm/mach-socfpga/ocram.c b/arch/arm/mach-socfpga/ocram.c
new file mode 100644
index 0000000..a83b34f
--- /dev/null
+++ b/arch/arm/mach-socfpga/ocram.c
@@ -0,0 +1,90 @@
+/*
+ * Copyright Altera Corporation (C) 2014. 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/of_platform.h>
+
+#define ALTR_OCRAM_CLEAR_ECC          0x00000018
+#define ALTR_OCRAM_ECC_EN             0x00000019
+
+void socfpga_init_ocram_ecc(void)
+{
+	struct device_node *np;
+	const __be32 *prop;
+	u32 ocr_edac_addr, iram_addr, len;
+	void __iomem  *mapped_ocr_edac_addr;
+	size_t size;
+	struct gen_pool *gp;
+
+	np = of_find_compatible_node(NULL, NULL, "altr,ocram-edac");
+	if (!np) {
+		pr_err("SOCFPGA: Unable to find altr,ocram-edac in dtb\n");
+		return;
+	}
+
+	prop = of_get_property(np, "reg", &size);
+	if (!prop || size < sizeof(*prop)) {
+		pr_err("SOCFPGA: Unable to find OCRAM ECC mapping in dtb\n");
+		return;
+	}
+	ocr_edac_addr = be32_to_cpup(prop++);
+	len = be32_to_cpup(prop);
+
+	gp = of_get_named_gen_pool(np, "iram", 0);
+	if (!gp) {
+		pr_err("SOCFPGA: OCRAM cannot find gen pool\n");
+		return;
+	}
+
+	np = of_find_compatible_node(NULL, NULL, "mmio-sram");
+	if (!np) {
+		pr_err("SOCFPGA: Unable to find mmio-sram in dtb\n");
+		return;
+	}
+
+	/* Determine the OCRAM address and size */
+	prop = of_get_property(np, "reg", &size);
+	if (!prop || size < sizeof(*prop)) {
+		pr_err("SOCFPGA: Unable to find OCRAM mapping in dtb\n");
+		return;
+	}
+	iram_addr = be32_to_cpup(prop++);
+	len = be32_to_cpup(prop);
+
+	iram_addr = gen_pool_alloc(gp, len);
+	if (iram_addr == 0) {
+		pr_err("SOCFPGA: cannot alloc from gen pool\n");
+		return;
+	}
+
+	memset((void *)iram_addr, 0, len);
+
+	gen_pool_free(gp, iram_addr, len);
+
+	mapped_ocr_edac_addr = ioremap(ocr_edac_addr, 4);
+	if (!mapped_ocr_edac_addr) {
+		pr_err("SOCFPGA: Unable to map OCRAM ecc regs.\n");
+		return;
+	}
+
+	/* 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);
+
+	iounmap(mapped_ocr_edac_addr);
+
+	pr_debug("SOCFPGA: Success Initializing OCRAM\n");
+}
diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index 0954011..065d80d 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -100,6 +100,14 @@ static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
 	writel(temp, rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL);
 }
 
+static void __init socfpga_cyclone5_init(void)
+{
+	of_platform_populate(NULL, of_default_bus_match_table,
+			     NULL, NULL);
+	if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
+		socfpga_init_ocram_ecc();
+}
+
 static const char *altera_dt_match[] = {
 	"altr,socfpga",
 	NULL
@@ -111,6 +119,7 @@ DT_MACHINE_START(SOCFPGA, "Altera SOCFPGA")
 	.smp		= smp_ops(socfpga_smp_ops),
 	.map_io		= socfpga_map_io,
 	.init_irq	= socfpga_init_irq,
+	.init_machine	= socfpga_cyclone5_init,
 	.restart	= socfpga_cyclone5_restart,
 	.dt_compat	= altera_dt_match,
 MACHINE_END
-- 
1.7.9.5

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

[Dinh Nguyen]

On 11/11/14, 6:14 PM, 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.
> 
> 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.
> ---
>  arch/arm/mach-socfpga/Makefile  |    1 +
>  arch/arm/mach-socfpga/core.h    |    1 +
>  arch/arm/mach-socfpga/ocram.c   |   90 +++++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-socfpga/socfpga.c |    9 ++++
>  4 files changed, 101 insertions(+)
>  create mode 100644 arch/arm/mach-socfpga/ocram.c
> 

Acked-by: Dinh Nguyen <dinguyen@opensource.altera.com>

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

[Mark Rutland]

On Wed, Nov 12, 2014 at 12:14:20AM +0000, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> 
> 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-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> ---
> 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.
> ---
>  arch/arm/mach-socfpga/Makefile  |    1 +
>  arch/arm/mach-socfpga/core.h    |    1 +
>  arch/arm/mach-socfpga/ocram.c   |   90 +++++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-socfpga/socfpga.c |    9 ++++
>  4 files changed, 101 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/core.h b/arch/arm/mach-socfpga/core.h
> index 385baba..f3eee32 100644
> --- a/arch/arm/mach-socfpga/core.h
> +++ b/arch/arm/mach-socfpga/core.h
> @@ -45,5 +45,6 @@ extern unsigned long cpu1start_addr;
>  #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
>  
>  void socfpga_init_l2_ecc(void);
> +void socfpga_init_ocram_ecc(void);
>  
>  #endif
> diff --git a/arch/arm/mach-socfpga/ocram.c b/arch/arm/mach-socfpga/ocram.c
> new file mode 100644
> index 0000000..a83b34f
> --- /dev/null
> +++ b/arch/arm/mach-socfpga/ocram.c
> @@ -0,0 +1,90 @@
> +/*
> + * Copyright Altera Corporation (C) 2014. 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/of_platform.h>
> +
> +#define ALTR_OCRAM_CLEAR_ECC          0x00000018
> +#define ALTR_OCRAM_ECC_EN             0x00000019
> +
> +void socfpga_init_ocram_ecc(void)
> +{
> +	struct device_node *np;
> +	const __be32 *prop;

Please don't use accessors which return raw __be32s in drivers,
typically it's the wrong thing to do and leaves horrible bugs and/or
quirks that are painful to fix up.

> +	u32 ocr_edac_addr, iram_addr, len;
> +	void __iomem  *mapped_ocr_edac_addr;
> +	size_t size;
> +	struct gen_pool *gp;
> +
> +	np = of_find_compatible_node(NULL, NULL, "altr,ocram-edac");
> +	if (!np) {
> +		pr_err("SOCFPGA: Unable to find altr,ocram-edac in dtb\n");
> +		return;
> +	}
> +
> +	prop = of_get_property(np, "reg", &size);
> +	if (!prop || size < sizeof(*prop)) {
> +		pr_err("SOCFPGA: Unable to find OCRAM ECC mapping in dtb\n");
> +		return;
> +	}
> +	ocr_edac_addr = be32_to_cpup(prop++);
> +	len = be32_to_cpup(prop);

Use of_iomap(np, 0). You don't seem to pass the address around, so that
should be sufficient.

> +
> +	gp = of_get_named_gen_pool(np, "iram", 0);
> +	if (!gp) {
> +		pr_err("SOCFPGA: OCRAM cannot find gen pool\n");
> +		return;
> +	}
> +
> +	np = of_find_compatible_node(NULL, NULL, "mmio-sram");
> +	if (!np) {
> +		pr_err("SOCFPGA: Unable to find mmio-sram in dtb\n");
> +		return;
> +	}
> +
> +	/* Determine the OCRAM address and size */
> +	prop = of_get_property(np, "reg", &size);
> +	if (!prop || size < sizeof(*prop)) {
> +		pr_err("SOCFPGA: Unable to find OCRAM mapping in dtb\n");
> +		return;
> +	}
> +	iram_addr = be32_to_cpup(prop++);
> +	len = be32_to_cpup(prop);

This address is overwritten below. What's going on?

> +
> +	iram_addr = gen_pool_alloc(gp, len);
> +	if (iram_addr == 0) {
> +		pr_err("SOCFPGA: cannot alloc from gen pool\n");
> +		return;
> +	}
> +
> +	memset((void *)iram_addr, 0, len);

How is the iram mapped? Is memset to it safe (e.g. it unaligned accesses
are made)?

> +
> +	gen_pool_free(gp, iram_addr, len);
> +
> +	mapped_ocr_edac_addr = ioremap(ocr_edac_addr, 4);
> +	if (!mapped_ocr_edac_addr) {
> +		pr_err("SOCFPGA: Unable to map OCRAM ecc regs.\n");
> +		return;
> +	}
> +
> +	/* 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);
> +
> +	iounmap(mapped_ocr_edac_addr);
> +
> +	pr_debug("SOCFPGA: Success Initializing OCRAM\n");
> +}
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index 0954011..065d80d 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -100,6 +100,14 @@ static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
>  	writel(temp, rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL);
>  }
>  
> +static void __init socfpga_cyclone5_init(void)
> +{
> +	of_platform_populate(NULL, of_default_bus_match_table,
> +			     NULL, NULL);
> +	if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
> +		socfpga_init_ocram_ecc();

If it's safe to do this after probing everything else, why can't this be
a normal device probe?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Thor Thayer]

On 12/02/2014 09:11 AM, Mark Rutland wrote:
> On Wed, Nov 12, 2014 at 12:14:20AM +0000, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>
>> 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-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>> ---
>> 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.
>> ---
<snip>

>> +
>> +void socfpga_init_ocram_ecc(void)
>> +{
>> +	struct device_node *np;
>> +	const __be32 *prop;
>
> Please don't use accessors which return raw __be32s in drivers,
> typically it's the wrong thing to do and leaves horrible bugs and/or
> quirks that are painful to fix up.

OK. Thanks.

>> +	u32 ocr_edac_addr, iram_addr, len;
>> +	void __iomem  *mapped_ocr_edac_addr;
>> +	size_t size;
>> +	struct gen_pool *gp;
>> +
>> +	np = of_find_compatible_node(NULL, NULL, "altr,ocram-edac");
>> +	if (!np) {
>> +		pr_err("SOCFPGA: Unable to find altr,ocram-edac in dtb\n");
>> +		return;
>> +	}
>> +
>> +	prop = of_get_property(np, "reg", &size);
>> +	if (!prop || size < sizeof(*prop)) {
>> +		pr_err("SOCFPGA: Unable to find OCRAM ECC mapping in dtb\n");
>> +		return;
>> +	}
>> +	ocr_edac_addr = be32_to_cpup(prop++);
>> +	len = be32_to_cpup(prop);
>
> Use of_iomap(np, 0). You don't seem to pass the address around, so that
> should be sufficient.
>
>> +
>> +	gp = of_get_named_gen_pool(np, "iram", 0);
>> +	if (!gp) {
>> +		pr_err("SOCFPGA: OCRAM cannot find gen pool\n");
>> +		return;
>> +	}
>> +
>> +	np = of_find_compatible_node(NULL, NULL, "mmio-sram");
>> +	if (!np) {
>> +		pr_err("SOCFPGA: Unable to find mmio-sram in dtb\n");
>> +		return;
>> +	}
>> +
>> +	/* Determine the OCRAM address and size */
>> +	prop = of_get_property(np, "reg", &size);
>> +	if (!prop || size < sizeof(*prop)) {
>> +		pr_err("SOCFPGA: Unable to find OCRAM mapping in dtb\n");
>> +		return;
>> +	}
>> +	iram_addr = be32_to_cpup(prop++);
>> +	len = be32_to_cpup(prop);
>
> This address is overwritten below. What's going on?
>

Thanks, leftovers from debugging. I will look for a different way of 
getting the information without the be32 stuff.

>> +
>> +	iram_addr = gen_pool_alloc(gp, len);
>> +	if (iram_addr == 0) {
>> +		pr_err("SOCFPGA: cannot alloc from gen pool\n");
>> +		return;
>> +	}
>> +
>> +	memset((void *)iram_addr, 0, len);
>
> How is the iram mapped? Is memset to it safe (e.g. it unaligned accesses
> are made)?
>

I will need to look at this further - good points. The entire iram 
should be used as a pool so I didn't expect anything unusual even though 
I just realized allocations are not contiguous.

>> +
>> +	gen_pool_free(gp, iram_addr, len);
>> +
>> +	mapped_ocr_edac_addr = ioremap(ocr_edac_addr, 4);
>> +	if (!mapped_ocr_edac_addr) {
>> +		pr_err("SOCFPGA: Unable to map OCRAM ecc regs.\n");
>> +		return;
>> +	}
>> +
>> +	/* 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);
>> +
>> +	iounmap(mapped_ocr_edac_addr);
>> +
>> +	pr_debug("SOCFPGA: Success Initializing OCRAM\n");
>> +}
>> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
>> index 0954011..065d80d 100644
>> --- a/arch/arm/mach-socfpga/socfpga.c
>> +++ b/arch/arm/mach-socfpga/socfpga.c
>> @@ -100,6 +100,14 @@ static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd)
>>   	writel(temp, rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL);
>>   }
>>
>> +static void __init socfpga_cyclone5_init(void)
>> +{
>> +	of_platform_populate(NULL, of_default_bus_match_table,
>> +			     NULL, NULL);
>> +	if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
>> +		socfpga_init_ocram_ecc();
>
> If it's safe to do this after probing everything else, why can't this be
> a normal device probe?
>
> Thanks,
> Mark.

Good point. Thank you.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv5 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/5: No change.
---
 drivers/edac/Kconfig |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 7072c28..1719975 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

[PATCHv5 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.

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.
---
 drivers/edac/Kconfig       |   14 ++
 drivers/edac/Makefile      |    5 +-
 drivers/edac/altera_edac.c |  481 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 498 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 1719975..b145a52 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -385,4 +385,18 @@ 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 && CACHE_L2X0
+	help
+	  Support for error detection and correction on the
+	  Altera L2 cache Memory for Altera SoCs.
+
+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 359aa49..20e9b6f 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..1655dd3 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -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,35 @@ 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)
+
+/* MPU L2 Register Defines */
+#define ALTR_MPUL2_CONTROL_OFFSET       0x100
+#define ALTR_MPUL2_CTL_CACHE_EN_MASK    BIT(0)
+
+/* 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       16    /* Line size x 2 */
+#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.       */
+
 static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id)
 {
 	struct mem_ctl_info *mci = dev_id;
@@ -405,6 +437,453 @@ static struct platform_driver altr_sdram_edac_driver = {
 
 module_platform_driver(altr_sdram_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))
+		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 -ENODEV;
+	}
+
+	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)
+		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) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "%s:Unable to map regs\n", ecc_name);
+		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:
+	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)
+		return NULL;
+
+	sram_addr = (void *)gen_pool_alloc(gp, size);
+	if (!sram_addr)
+		return NULL;
+
+	memset(sram_addr, 0, size);
+	wmb();	/* Ensure data is written out */
+
+	*other = gp;
+
+	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);
+}
+
+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();
+	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.
+ *	Can't turn on ECC here because accessing un-initialized
+ *	memory will cause CE/UE errors possibly causing an ABORT.
+ *	Bail if ECC is not on.
+ *	Test For 1) L2 ECC is enabled and 2) L2 Cache is enabled.
+ */
+static int altr_l2_dependencies(struct platform_device *pdev,
+				void __iomem *base)
+{
+	u32 control;
+	struct regmap *l2_vbase;
+
+	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;
+	}
+
+	l2_vbase = syscon_regmap_lookup_by_compatible("arm,pl310-cache");
+	if (IS_ERR(l2_vbase)) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "L2:regmap for arm,pl310-cache lookup failed.\n");
+		return -ENODEV;
+	}
+
+	regmap_read(l2_vbase, ALTR_MPUL2_CONTROL_OFFSET, &control);
+	if (!(control & ALTR_MPUL2_CTL_CACHE_EN_MASK)) {
+		edac_printk(KERN_ERR, EDAC_DEVICE, "L2: Cache 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

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

[Mark Rutland]

Hi,

> +/* MPU L2 Register Defines */
> +#define ALTR_MPUL2_CONTROL_OFFSET       0x100
> +#define ALTR_MPUL2_CTL_CACHE_EN_MASK    BIT(0)

These are just standard PL310 register definitions, no?

[...]

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

Don't you need to have a corresponding of_node_put?

Is this ever called before the code above has probed? Can't you keep
this information around rather than probing it every time?

> +
> +       gp = of_get_named_gen_pool(np, "iram", 0);
> +       if (!gp)
> +               return NULL;
> +
> +       sram_addr = (void *)gen_pool_alloc(gp, size);
> +       if (!sram_addr)
> +               return NULL;
> +
> +       memset(sram_addr, 0, size);

Is it safe to do a memset to the sram? How is it mapped?

[...]

> +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();
> +       flush_cache_all();

Doesn't this just flush out _to_ the L2 (and not beyond)? Even then I
don't think that's safe with the MMU enabled.

Why is the flush necessary?

[...]

> +static int altr_l2_dependencies(struct platform_device *pdev,
> +                               void __iomem *base)
> +{
> +       u32 control;
> +       struct regmap *l2_vbase;
> +
> +       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;
> +       }
> +
> +       l2_vbase = syscon_regmap_lookup_by_compatible("arm,pl310-cache");
> +       if (IS_ERR(l2_vbase)) {
> +               edac_printk(KERN_ERR, EDAC_DEVICE,
> +                           "L2:regmap for arm,pl310-cache lookup failed.\n");
> +               return -ENODEV;
> +       }

I must NAK any use of the L2 as a syscon device. It's simply not a
register file that was intended to be shared. I appreciate that you only
want to check something very simple, but we should use a higher level
API for that (and we should add one if we do not already have one)

> +
> +       regmap_read(l2_vbase, ALTR_MPUL2_CONTROL_OFFSET, &control);
> +       if (!(control & ALTR_MPUL2_CTL_CACHE_EN_MASK)) {
> +               edac_printk(KERN_ERR, EDAC_DEVICE, "L2: Cache 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
> 
> 

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

[Thor Thayer]

On 12/02/2014 09:25 AM, Mark Rutland wrote:
> Hi,
>
>> +/* MPU L2 Register Defines */
>> +#define ALTR_MPUL2_CONTROL_OFFSET       0x100
>> +#define ALTR_MPUL2_CTL_CACHE_EN_MASK    BIT(0)
>
> These are just standard PL310 register definitions, no?

Yes.

> [...]
>
>> +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;
>
> Don't you need to have a corresponding of_node_put?
>
> Is this ever called before the code above has probed? Can't you keep
> this information around rather than probing it every time?
>

This is only used for testing so it happens infrequently and always 
after the probe has finished.

These error injection functions will not be part of the standard 
distribution for our board so it was cleaner to leave it out.

>> +
>> +       gp = of_get_named_gen_pool(np, "iram", 0);
>> +       if (!gp)
>> +               return NULL;
>> +
>> +       sram_addr = (void *)gen_pool_alloc(gp, size);
>> +       if (!sram_addr)
>> +               return NULL;
>> +
>> +       memset(sram_addr, 0, size);
>
> Is it safe to do a memset to the sram? How is it mapped?
>

I checked the gen_pool and the memory may not be contiguous. I will fix 
this. Thanks!

> [...]
>
>> +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();
>> +       flush_cache_all();
>
> Doesn't this just flush out _to_ the L2 (and not beyond)? Even then I
> don't think that's safe with the MMU enabled.
>
> Why is the flush necessary?
>

flush_cache_all() maps to flush_kern_all() which cleans all the cache 
levels up to the level of coherency which includes L2 in it.

This flush ensures the data is in a known state (zero) before I inject 
the errors for testing L2 ECC.

> [...]
>
>> +static int altr_l2_dependencies(struct platform_device *pdev,
>> +                               void __iomem *base)
>> +{
>> +       u32 control;
>> +       struct regmap *l2_vbase;
>> +
>> +       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;
>> +       }
>> +
>> +       l2_vbase = syscon_regmap_lookup_by_compatible("arm,pl310-cache");
>> +       if (IS_ERR(l2_vbase)) {
>> +               edac_printk(KERN_ERR, EDAC_DEVICE,
>> +                           "L2:regmap for arm,pl310-cache lookup failed.\n");
>> +               return -ENODEV;
>> +       }
>
> I must NAK any use of the L2 as a syscon device. It's simply not a
> register file that was intended to be shared. I appreciate that you only
> want to check something very simple, but we should use a higher level
> API for that (and we should add one if we do not already have one)
>

OK. I didn't know of a way to read that register otherwise. I will 
rework this.

>> +
>> +       regmap_read(l2_vbase, ALTR_MPUL2_CONTROL_OFFSET, &control);
>> +       if (!(control & ALTR_MPUL2_CTL_CACHE_EN_MASK)) {
>> +               edac_printk(KERN_ERR, EDAC_DEVICE, "L2: Cache 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
>>
>>

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

[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
---
 .../bindings/arm/altera/socfpga-l2-edac.txt        |   15 +++++++++++++++
 .../bindings/arm/altera/socfpga-ocram-edac.txt     |   16 ++++++++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |   15 ++++++++++++++-
 3 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
 create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
new file mode 100644
index 0000000..35b19e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
@@ -0,0 +1,15 @@
+Altera SoCFPGA L2 cache Error Detection and Correction [EDAC]
+
+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.
+
+Example:
+
+	l2edac@ffd08140 {
+		compatible = "altr,l2-edac";
+		reg = <0xffd08140 0x4>;
+		interrupts = <0 36 1>, <0 37 1>;
+	};
diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
new file mode 100644
index 0000000..31ab205
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
@@ -0,0 +1,16 @@
+Altera SoCFPGA On-Chip RAM Error Detection and Correction [EDAC]
+
+OCRAM 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:
+	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 6af96ed..32c63a3 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -618,8 +618,21 @@
 			interrupts = <0 39 4>;
 		};
 
+		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";
+			compatible = "arm,pl310-cache", "syscon";
 			reg = <0xfffef000 0x1000>;
 			interrupts = <0 38 0x04>;
 			cache-unified;
-- 
1.7.9.5

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

[Thor Thayer]

Hi all,

On 11/11/2014 06:14 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

Any comments on these devicetree additions?

Thanks,
Thor
> Signed-off-by: Thor Thayer<tthayer@opensource.altera.com>
> ---
> v2: Remove OCRAM declaration and reference prior patch.
>
> v3-5: No Change
> ---
>   .../bindings/arm/altera/socfpga-l2-edac.txt        |   15 +++++++++++++++
>   .../bindings/arm/altera/socfpga-ocram-edac.txt     |   16 ++++++++++++++++
>   arch/arm/boot/dts/socfpga.dtsi                     |   15 ++++++++++++++-
>   3 files changed, 45 insertions(+), 1 deletion(-)
>   create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
>   create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
> new file mode 100644
> index 0000000..35b19e3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
> @@ -0,0 +1,15 @@
> +Altera SoCFPGA L2 cache Error Detection and Correction [EDAC]
> +
> +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.
> +
> +Example:
> +
> +	l2edac@ffd08140 {
> +		compatible = "altr,l2-edac";
> +		reg = <0xffd08140 0x4>;
> +		interrupts = <0 36 1>, <0 37 1>;
> +	};
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
> new file mode 100644
> index 0000000..31ab205
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
> @@ -0,0 +1,16 @@
> +Altera SoCFPGA On-Chip RAM Error Detection and Correction [EDAC]
> +
> +OCRAM 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:
> +	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 6af96ed..32c63a3 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -618,8 +618,21 @@
>   			interrupts = <0 39 4>;
>   		};
>   
> +		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";
> +			compatible = "arm,pl310-cache", "syscon";
>   			reg = <0xfffef000 0x1000>;
>   			interrupts = <0 38 0x04>;
>   			cache-unified;

Re: [RESEND PATCHv5 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC

[Thor Thayer]

Hi Boris,

On 11/18/2014 02:56 PM, Thor Thayer wrote:
> Hi all,
>
> On 11/11/2014 06:14 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
>
> Any comments on these devicetree additions?
>
> Thanks,
> Thor
>> Signed-off-by: Thor Thayer<tthayer@opensource.altera.com>
>> ---
>> v2: Remove OCRAM declaration and reference prior patch.
>>
>> v3-5: No Change
>> ---

I originally submitted this series on November 11, 2014.

I haven't received any comments or ACKs on the device tree patch portion 
(patch 5 of 5). According to 
Documentation/devicetree/bindings/submitting-patches.txt, if the patch 
hasn't been Acked after a couple of weeks, the maintainer can pull the 
changes if they are comfortable with those changes.

Dinh checked for merge conflicts in the DTS files in his November 7 
email (queued patches for 3.19).

Dinh acked patches 1 & 2 on November 12.

Thanks,

Thor

Re: [RESEND PATCHv5 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC

[Mark Rutland]

Hi Thor,

On Mon, Dec 01, 2014 at 08:47:41PM +0000, Thor Thayer wrote:
> Hi Boris,
> 
> On 11/18/2014 02:56 PM, Thor Thayer wrote:
> > Hi all,
> >
> > On 11/11/2014 06:14 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
> >
> > Any comments on these devicetree additions?
> >
> > Thanks,
> > Thor
> >> Signed-off-by: Thor Thayer<tthayer@opensource.altera.com>
> >> ---
> >> v2: Remove OCRAM declaration and reference prior patch.
> >>
> >> v3-5: No Change
> >> ---
> 
> I originally submitted this series on November 11, 2014.
> 
> I haven't received any comments or ACKs on the device tree patch portion 
> (patch 5 of 5). According to 
> Documentation/devicetree/bindings/submitting-patches.txt, if the patch 
> hasn't been Acked after a couple of weeks, the maintainer can pull the 
> changes if they are comfortable with those changes.

Apologies it has taken so long for this to be looked at. Unfortunately
my inbox is overflowing (as are all of ours) and things fall by the
wayside. I am sorry that this has lead to such a delayed response,
especially given that it was negative.

I'll try to keep up with this a bit more actively.

Thanks,
Mark.

Re: [RESEND PATCHv5 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC

[Thor Thayer]

On 12/02/2014 09:01 AM, Mark Rutland wrote:
> Hi Thor,
>
> On Mon, Dec 01, 2014 at 08:47:41PM +0000, Thor Thayer wrote:
>> Hi Boris,
>>
>> On 11/18/2014 02:56 PM, Thor Thayer wrote:
>>> Hi all,
>>>
>>> On 11/11/2014 06:14 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
>>>
>>> Any comments on these devicetree additions?
>>>
>>> Thanks,
>>> Thor
>>>> Signed-off-by: Thor Thayer<tthayer@opensource.altera.com>
>>>> ---
>>>> v2: Remove OCRAM declaration and reference prior patch.
>>>>
>>>> v3-5: No Change
>>>> ---
>>
>> I originally submitted this series on November 11, 2014.
>>
>> I haven't received any comments or ACKs on the device tree patch portion
>> (patch 5 of 5). According to
>> Documentation/devicetree/bindings/submitting-patches.txt, if the patch
>> hasn't been Acked after a couple of weeks, the maintainer can pull the
>> changes if they are comfortable with those changes.
>
> Apologies it has taken so long for this to be looked at. Unfortunately
> my inbox is overflowing (as are all of ours) and things fall by the
> wayside. I am sorry that this has lead to such a delayed response,
> especially given that it was negative.
>
> I'll try to keep up with this a bit more actively.
>
> Thanks,
> Mark.
>

Hi Mark,

Thank you for reviewing this. I have seen how busy the device tree ML is 
so I completely understand.

You have reviewed previous patch revisions in this series - thanks. I 
thought that I'd addressed most of the issues (in some cases by 
explaining my implementation) and since I hadn't heard anything, I 
assumed there weren't concerns.

Thanks again,

Thor

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

[Mark Rutland]

On Wed, Nov 12, 2014 at 12:14:23AM +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
> ---
>  .../bindings/arm/altera/socfpga-l2-edac.txt        |   15 +++++++++++++++
>  .../bindings/arm/altera/socfpga-ocram-edac.txt     |   16 ++++++++++++++++
>  arch/arm/boot/dts/socfpga.dtsi                     |   15 ++++++++++++++-
>  3 files changed, 45 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
> new file mode 100644
> index 0000000..35b19e3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
> @@ -0,0 +1,15 @@
> +Altera SoCFPGA L2 cache Error Detection and Correction [EDAC]
> +
> +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.
> +
> +Example:
> +
> +	l2edac@ffd08140 {
> +		compatible = "altr,l2-edac";
> +		reg = <0xffd08140 0x4>;
> +		interrupts = <0 36 1>, <0 37 1>;
> +	};

Judging by the size of the reg entry, this is part of a larger block
(the same one the OCRAM EDAC lives in). Why isn't that larger block
described?

EDAC is a Linux subsystem name, but typically not the HW block name.
What HW block does this live in?

> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
> new file mode 100644
> index 0000000..31ab205
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
> @@ -0,0 +1,16 @@
> +Altera SoCFPGA On-Chip RAM Error Detection and Correction [EDAC]
> +
> +OCRAM 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:
> +	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 6af96ed..32c63a3 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -618,8 +618,21 @@
>  			interrupts = <0 39 4>;
>  		};
>  
> +		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";
> +			compatible = "arm,pl310-cache", "syscon";

NAK. 

Why are you marking the PL310 as a syscon device? It is most definitely
_NOT_ a shared set of registers lumped together.

Thanks,
Mark.

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

[Thor Thayer]

On 12/02/2014 08:57 AM, Mark Rutland wrote:
> On Wed, Nov 12, 2014 at 12:14:23AM +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>

<snip>

>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
>> @@ -0,0 +1,15 @@
>> +Altera SoCFPGA L2 cache Error Detection and Correction [EDAC]
>> +
>> +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.
>> +
>> +Example:
>> +
>> +	l2edac@ffd08140 {
>> +		compatible = "altr,l2-edac";
>> +		reg = <0xffd08140 0x4>;
>> +		interrupts = <0 36 1>, <0 37 1>;
>> +	};
>
> Judging by the size of the reg entry, this is part of a larger block
> (the same one the OCRAM EDAC lives in). Why isn't that larger block
> described?
>
> EDAC is a Linux subsystem name, but typically not the HW block name.
> What HW block does this live in?
>

Yes, this register is part of the ECC block of registers. In order to 
get each probe function to run, the L2 EDAC and OCRAM EDAC had to be at 
the top level of the device tree. If they are children of a parent node, 
the probe functions aren't executed.
Is there a better way or example I should follow?

>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
>> new file mode 100644
>> index 0000000..31ab205
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt

<snip>

>> +
>>   		L2: l2-cache@fffef000 {
>> -			compatible = "arm,pl310-cache";
>> +			compatible = "arm,pl310-cache", "syscon";
>
> NAK.
>
> Why are you marking the PL310 as a syscon device? It is most definitely
> _NOT_ a shared set of registers lumped together.
>

Unfortunately, the register is locked for exclusive access by the L2 
cache driver. I read your comment on patch 4 and will find a better way.

> Thanks,
> Mark.
>