[PATCH v2 0/3] ARM: keystone: add ecc error interrupt handling

[PATCH v2 0/3] ARM: keystone: add ecc error interrupt handling

[Vitaly Andrianov]

This patch series adds support for arm L1/L2 ecc and ddr3 ecc error handling
for Keystone devices

Change Log

v2: 
- removing unused and sorting headers of keystone.c are moved to a separate
  patch.
- l1l2 ecc and ddr3 ecc error handling are split it to separate patches
- removed unused headers from keystone_ecc.c
- platsmp.c removed from the patch.
- return IRQ_HANDLED for 1 bit error in l1l2 ecc handler
- checked and handled existing ecc error before enabling ddr3 interrupt
- 1 bit ddr3 interrupt is disabled, because it is handled by hardware and
  there is no reason to handle it by software

v1: initial version in one patch


Vitaly Andrianov (3):
  ARM: keystone: clean and sort keystone.c headers
  ARM: keystone: ecc: add ARM L1/L2 ecc interrupt handling
  ARM: keystone: ecc: add DDR3 ecc interrupt handling

 .../devicetree/bindings/arm/keystone/keystone.txt  |  17 +++
 arch/arm/mach-keystone/Makefile                    |   2 +-
 arch/arm/mach-keystone/keystone.c                  |  81 ++++++++++++--
 arch/arm/mach-keystone/keystone.h                  |   1 +
 arch/arm/mach-keystone/keystone_ecc.c              | 117 +++++++++++++++++++++
 5 files changed, 210 insertions(+), 8 deletions(-)
 create mode 100644 arch/arm/mach-keystone/keystone_ecc.c

-- 
1.9.1

[PATCH v2 1/3] ARM: keystone: clean and sort keystone.c headers

[Vitaly Andrianov]

This patch remove unused and sort remaining headers.

Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
---
 arch/arm/mach-keystone/keystone.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
index 0662087..4534c7d 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -10,18 +10,15 @@
  * version 2, as published by the Free Software Foundation.
  */
 #include <linux/io.h>
-#include <linux/of.h>
-#include <linux/init.h>
-#include <linux/of_platform.h>
-#include <linux/of_address.h>
 #include <linux/memblock.h>
+#include <linux/of_platform.h>
 
-#include <asm/setup.h>
-#include <asm/mach/map.h>
 #include <asm/mach/arch.h>
+#include <asm/mach/map.h>
 #include <asm/mach/time.h>
-#include <asm/smp_plat.h>
 #include <asm/memory.h>
+#include <asm/setup.h>
+#include <asm/smp_plat.h>
 
 #include "memory.h"
 
-- 
1.9.1

[PATCH v2 2/3] ARM: keystone: ecc: add ARM L1/L2 ecc interrupt handling

[Vitaly Andrianov]

This patch adds ARM L1/L2 ECC handler support interrupt handling
for Keystone II devices, the kernel will reboot if the error
is 2-bit error for L1/L2 ECC error.

Signed-off-by: Hao Zhang <hzhang@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
---
 arch/arm/mach-keystone/keystone.c | 69 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
index 4534c7d..0b28fca 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -9,9 +9,12 @@
  * under the terms and conditions of the GNU General Public License,
  * version 2, as published by the Free Software Foundation.
  */
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/memblock.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/reboot.h>
 
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
@@ -46,6 +49,72 @@ static int keystone_platform_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+#define L2_INTERN_ASYNC_ERROR  BIT(30)
+#define AXI_ASYNC_ERROR BIT(29)
+
+static void check_ecc_error(void)
+{
+	u32 status, fault;
+
+	/* read and clear L2ECTLR CP15 register for L2 ECC error */
+	asm("mrc p15, 1, %0, c9, c0, 3" : "=r"(status));
+
+	if (status & L2_INTERN_ASYNC_ERROR) {
+		status &= ~L2_INTERN_ASYNC_ERROR;
+		asm("mcr p15, 1, %0, c9, c0, 3" : : "r" (status));
+		asm("mcr p15, 0, %0, c5, c1, 0" : "=r" (fault));
+		/*
+		 * Do a machine restart as this is double bit ECC error
+		 * that can't be corrected
+		 */
+		pr_err("ARM Cortex A15 L1/L2 ECC error, CP15 ADFSR 0x%x\n",
+		       fault);
+		machine_restart(NULL);
+		/* we should never cone here*/
+	}
+
+	if (status & AXI_ASYNC_ERROR)
+		pr_err("ARM Cortex A15 AXI async error shouldn't cause an nterrupt\n");
+}
+
+static irqreturn_t arm_l1l2_ecc_err_irq_handler(int irq, void *reg_virt)
+{
+	check_ecc_error();
+
+	return IRQ_HANDLED;
+}
+
+static int __init keystone_init_misc(void)
+{
+	struct device_node *node = NULL;
+	int error_irq = 0;
+	int ret;
+
+	/*
+	 * check if we already have ecc error. Reboot if it is double bit
+	 * error.
+	 */
+	check_ecc_error();
+
+	/* add ARM ECC L1/L2 cache error handler */
+	node = of_find_compatible_node(NULL, NULL, "ti,keystone-sys");
+	if (node)
+		error_irq = irq_of_parse_and_map(node, 0);
+
+	if (!error_irq) {
+		pr_warn("Warning!! arm L1/L2 ECC irq number not defined\n");
+		return 0;
+	}
+
+	if (request_irq(error_irq, arm_l1l2_ecc_err_irq_handler, 0,
+		"a15-l1l2-ecc-err-irq", 0) < 0) {
+		WARN_ON("request_irq fail for arm L1/L2 ECC error irq\n");
+	}
+
+	return ret;
+}
+subsys_initcall(keystone_init_misc);
+
 static void __init keystone_init(void)
 {
 	keystone_pm_runtime_init();
-- 
1.9.1

[PATCH v2 3/3] ARM: keystone: ecc: add DDR3 ecc interrupt handling

[Vitaly Andrianov]

This patch adds DDR3 ECC handler support interrupt handling
for Keystone II devices, the kernel will reboot if the error
is 2-bit error for DDR3 ECC error.

Signed-off-by: Hao Zhang <hzhang@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
---
 .../devicetree/bindings/arm/keystone/keystone.txt  |  17 +++
 arch/arm/mach-keystone/Makefile                    |   2 +-
 arch/arm/mach-keystone/keystone.c                  |   3 +-
 arch/arm/mach-keystone/keystone.h                  |   1 +
 arch/arm/mach-keystone/keystone_ecc.c              | 117 +++++++++++++++++++++
 5 files changed, 138 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mach-keystone/keystone_ecc.c

diff --git a/Documentation/devicetree/bindings/arm/keystone/keystone.txt b/Documentation/devicetree/bindings/arm/keystone/keystone.txt
index 59d7a46..7d09cc5 100644
--- a/Documentation/devicetree/bindings/arm/keystone/keystone.txt
+++ b/Documentation/devicetree/bindings/arm/keystone/keystone.txt
@@ -18,3 +18,20 @@ Boards:
 
 -  Keystone 2 Edison EVM
    compatible = "ti,k2e-evm","ti,keystone"
+
+TI Keystone SoC specific Device Tree Bindings
+---------------------------------------------
+
+DDR3 ECC error interrupt handling
+---------------------------------
+
+Boards that has DDR3 ECC enabled will have a device node with following
+compatibility string to to allow handle double bit ECC error interrupts:-
+
+   compatible = "ti,keystone-ddr3-ecc"
+
+Required properties:
+
+- reg:  index 0 - base address of ECC specific register region
+
+- interrupts: DDR3 ECC error interrupt line
diff --git a/arch/arm/mach-keystone/Makefile b/arch/arm/mach-keystone/Makefile
index 25d9239..ea3b9a2 100644
--- a/arch/arm/mach-keystone/Makefile
+++ b/arch/arm/mach-keystone/Makefile
@@ -1,4 +1,4 @@
-obj-y					:= keystone.o smc.o
+obj-y					:= keystone.o smc.o keystone_ecc.o
 
 plus_sec := $(call as-instr,.arch_extension sec,+sec)
 AFLAGS_smc.o				:=-Wa,-march=armv7-a$(plus_sec)
diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
index 0b28fca..87f9bf1 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -97,7 +97,7 @@ static int __init keystone_init_misc(void)
 	check_ecc_error();
 
 	/* add ARM ECC L1/L2 cache error handler */
-	node = of_find_compatible_node(NULL, NULL, "ti,keystone-sys");
+	node = of_find_compatible_node(NULL, NULL, "ti,keystone-ddr3-ecc");
 	if (node)
 		error_irq = irq_of_parse_and_map(node, 0);
 
@@ -111,6 +111,7 @@ static int __init keystone_init_misc(void)
 		WARN_ON("request_irq fail for arm L1/L2 ECC error irq\n");
 	}
 
+	ret = keystone_init_ddr3_ecc(node);
 	return ret;
 }
 subsys_initcall(keystone_init_misc);
diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h
index cd04a1c..ff52243 100644
--- a/arch/arm/mach-keystone/keystone.h
+++ b/arch/arm/mach-keystone/keystone.h
@@ -19,6 +19,7 @@ extern struct smp_operations keystone_smp_ops;
 extern void secondary_startup(void);
 extern u32 keystone_cpu_smc(u32 command, u32 cpu, u32 addr);
 extern int keystone_pm_runtime_init(void);
+extern int keystone_init_ddr3_ecc(struct device_node *node);
 
 #endif /* __ASSEMBLER__ */
 #endif /* __KEYSTONE_H__ */
diff --git a/arch/arm/mach-keystone/keystone_ecc.c b/arch/arm/mach-keystone/keystone_ecc.c
new file mode 100644
index 0000000..cab3013
--- /dev/null
+++ b/arch/arm/mach-keystone/keystone_ecc.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright 2014 Texas Instruments, Inc.
+ *
+ * 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/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/reboot.h>
+#include "keystone.h"
+
+/* DDR3 controller registers */
+#define DDR3_EOI			0x0A0
+#define DDR3_IRQ_STATUS_RAW_SYS		0x0A4
+#define DDR3_IRQ_STATUS_SYS		0x0AC
+#define DDR3_IRQ_ENABLE_SET_SYS		0x0B4
+#define DDR3_IRQ_ENABLE_CLR_SYS		0x0BC
+#define DDR3_ECC_CTRL			0x110
+#define DDR3_ONE_BIT_ECC_ERR_CNT	0x130
+
+#define DDR3_1B_ECC_ERR			BIT(5)
+#define DDR3_2B_ECC_ERR			BIT(4)
+#define DDR3_WR_ECC_ERR			BIT(3)
+#define DDR3_SYS_ERR			BIT(0)
+
+static void check_ecc_error(void __iomem *ddr_reg)
+{
+	u32 irq_status;
+
+	irq_status = readl(ddr_reg + DDR3_IRQ_STATUS_SYS);
+	if ((irq_status & DDR3_2B_ECC_ERR) ||
+	    (irq_status & DDR3_WR_ECC_ERR)) {
+		pr_err("Unrecoverable DDR3 ECC error, irq status 0x%x, rebooting kernel ..\n",
+		       irq_status);
+		machine_restart(NULL);
+		/* we shouldn't return after that */
+	}
+}
+
+static irqreturn_t ddr3_ecc_err_irq_handler(int irq, void *reg_virt)
+{
+	void __iomem *ddr_reg = (void __iomem *)reg_virt;
+
+	check_ecc_error(ddr_reg);
+
+	/*
+	 * Other errors should be handled by hardware
+	 * So, nothing to do here
+	 */
+
+	return IRQ_HANDLED;
+}
+
+int keystone_init_ddr3_ecc(struct device_node *node)
+{
+	void __iomem *ddr_reg;
+	int error_irq = 0;
+	int ret;
+
+	/* ddr3 controller reg is configured in the sysctrl node at index 0 */
+	ddr_reg = of_iomap(node, 0);
+	if (!ddr_reg) {
+		pr_warn("Warning!! DDR3 controller regs not defined\n");
+		return -ENODEV;
+	}
+
+	/* disable and clear unused ECC interrupts */
+	writel(DDR3_1B_ECC_ERR | DDR3_SYS_ERR,
+	       ddr_reg + DDR3_IRQ_ENABLE_CLR_SYS);
+
+	writel(DDR3_1B_ECC_ERR | DDR3_SYS_ERR,
+	       ddr_reg + DDR3_IRQ_STATUS_SYS);
+
+	/*
+	 * check if we already have unrecoverable errors
+	 * reboot in that case
+	 */
+	check_ecc_error(ddr_reg);
+
+	writel(DDR3_2B_ECC_ERR | DDR3_WR_ECC_ERR,
+	       ddr_reg + DDR3_IRQ_ENABLE_CLR_SYS);
+
+	/* add DDR3 ECC error handler */
+	error_irq = irq_of_parse_and_map(node, 1);
+	if (!error_irq) {
+		/* No GIC interrupt, need to map CIC2 interrupt to GIC */
+		pr_warn("Warning!! DDR3 ECC irq number not defined\n");
+		ret = -ENODEV;
+		goto err;
+	}
+
+	ret = request_irq(error_irq, ddr3_ecc_err_irq_handler, 0,
+		"ddr3-ecc-err-irq", (void *)ddr_reg);
+	if (ret) {
+		WARN_ON("request_irq fail for DDR3 ECC error irq\n");
+		goto err;
+	}
+
+	writel(DDR3_2B_ECC_ERR | DDR3_WR_ECC_ERR,
+	       ddr_reg + DDR3_IRQ_ENABLE_SET_SYS);
+
+	return 0;
+err:
+	iounmap(ddr_reg);
+	return ret;
+}
-- 
1.9.1

[PATCH v2 0/3] ARM: keystone: add ecc error interrupt handling

[santosh shilimkar]

On 6/25/2015 7:31 AM, Vitaly Andrianov wrote:
> This patch series adds support for arm L1/L2 ecc and ddr3 ecc error handling
> for Keystone devices
>
> Change Log
>
> v2:
> - removing unused and sorting headers of keystone.c are moved to a separate
>    patch.
> - l1l2 ecc and ddr3 ecc error handling are split it to separate patches
> - removed unused headers from keystone_ecc.c
> - platsmp.c removed from the patch.
> - return IRQ_HANDLED for 1 bit error in l1l2 ecc handler
> - checked and handled existing ecc error before enabling ddr3 interrupt
> - 1 bit ddr3 interrupt is disabled, because it is handled by hardware and
>    there is no reason to handle it by software
>
This version looks good to me. As already commented, I would have liked
the patch 2/3(L2 ECC) code in ARM generic code so will give some more 
time for others to come back. Otherwise I will queue this up for next
window.

Thanks for follow up Vitaly.

Regards,
Santosh

[PATCH v2 0/3] ARM: keystone: add ecc error interrupt handling

[Stephen Boyd]

On 06/25/2015 08:04 AM, santosh shilimkar wrote:
> On 6/25/2015 7:31 AM, Vitaly Andrianov wrote:
>> This patch series adds support for arm L1/L2 ecc and ddr3 ecc error
>> handling
>> for Keystone devices
>>
>> Change Log
>>
>> v2:
>> - removing unused and sorting headers of keystone.c are moved to a
>> separate
>>    patch.
>> - l1l2 ecc and ddr3 ecc error handling are split it to separate patches
>> - removed unused headers from keystone_ecc.c
>> - platsmp.c removed from the patch.
>> - return IRQ_HANDLED for 1 bit error in l1l2 ecc handler
>> - checked and handled existing echttps://lwn.net/Articles/593336/c
>> error before enabling ddr3 interrupt
>> - 1 bit ddr3 interrupt is disabled, because it is handled by hardware
>> and
>>    there is no reason to handle it by software
>>
> This version looks good to me. As already commented, I would have liked
> the patch 2/3(L2 ECC) code in ARM generic code so will give some more
> time for others to come back. Otherwise I will queue this up for next
> window.

Why not make this into an edac driver? I sent out an L1/L2 error
detection edac driver for Krait processors a year ago, but it stalled
due to some DT binding stuff[1]. This looks fairly similar.

[1] https://lwn.net/Articles/593336/

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 0/3] ARM: keystone: add ecc error interrupt handling

[santosh shilimkar]

On 6/25/2015 2:02 PM, Stephen Boyd wrote:
> On 06/25/2015 08:04 AM, santosh shilimkar wrote:
>> On 6/25/2015 7:31 AM, Vitaly Andrianov wrote:
>>> This patch series adds support for arm L1/L2 ecc and ddr3 ecc error
>>> handling
>>> for Keystone devices
>>>
>>> Change Log
>>>
>>> v2:
>>> - removing unused and sorting headers of keystone.c are moved to a
>>> separate
>>>     patch.
>>> - l1l2 ecc and ddr3 ecc error handling are split it to separate patches
>>> - removed unused headers from keystone_ecc.c
>>> - platsmp.c removed from the patch.
>>> - return IRQ_HANDLED for 1 bit error in l1l2 ecc handler
>>> - checked and handled existing echttps://lwn.net/Articles/593336/c
>>> error before enabling ddr3 interrupt
>>> - 1 bit ddr3 interrupt is disabled, because it is handled by hardware
>>> and
>>>     there is no reason to handle it by software
>>>
>> This version looks good to me. As already commented, I would have liked
>> the patch 2/3(L2 ECC) code in ARM generic code so will give some more
>> time for others to come back. Otherwise I will queue this up for next
>> window.
>
> Why not make this into an edac driver? I sent out an L1/L2 error
> detection edac driver for Krait processors a year ago, but it stalled
> due to some DT binding stuff[1]. This looks fairly similar.
>
Indeed the error detection part is very similar(expected as well
considering the same processor L2 regs). I am not sure we need
full driver only for that but at least the IRQ error handler
related code can reside together. Lets see what RMK thinks
on this.


> [1] https://lwn.net/Articles/593336/
>

[PATCH v2 0/3] ARM: keystone: add ecc error interrupt handling

[Stephen Boyd]

On 06/25/2015 02:30 PM, santosh shilimkar wrote:
> On 6/25/2015 2:02 PM, Stephen Boyd wrote:
>> On 06/25/2015 08:04 AM, santosh shilimkar wrote:
>>> On 6/25/2015 7:31 AM, Vitaly Andrianov wrote:
>>>> This patch series adds support for arm L1/L2 ecc and ddr3 ecc error
>>>> handling
>>>> for Keystone devices
>>>>
>>>> Change Log
>>>>
>>>> v2:
>>>> - removing unused and sorting headers of keystone.c are moved to a
>>>> separate
>>>>     patch.
>>>> - l1l2 ecc and ddr3 ecc error handling are split it to separate
>>>> patches
>>>> - removed unused headers from keystone_ecc.c
>>>> - platsmp.c removed from the patch.
>>>> - return IRQ_HANDLED for 1 bit error in l1l2 ecc handler
>>>> - checked and handled existing echttps://lwn.net/Articles/593336/c
>>>> error before enabling ddr3 interrupt
>>>> - 1 bit ddr3 interrupt is disabled, because it is handled by hardware
>>>> and
>>>>     there is no reason to handle it by software
>>>>
>>> This version looks good to me. As already commented, I would have liked
>>> the patch 2/3(L2 ECC) code in ARM generic code so will give some more
>>> time for others to come back. Otherwise I will queue this up for next
>>> window.
>>
>> Why not make this into an edac driver? I sent out an L1/L2 error
>> detection edac driver for Krait processors a year ago, but it stalled
>> due to some DT binding stuff[1]. This looks fairly similar.
>>
> Indeed the error detection part is very similar(expected as well
> considering the same processor L2 regs). I am not sure we need
> full driver only for that but at least the IRQ error handler
> related code can reside together. Lets see what RMK thinks
> on this.
>

There's an existing one for highbank (drivers/edac/highbank_l2_edac.c)
and there was a patch set for the pl310 as well[1]. I don't think we
want any architecture specific code for this, just use the EDAC framework.

[1] https://lkml.org/lkml/2014/3/2/87

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 0/3] ARM: keystone: add ecc error interrupt handling

[Vitaly Andrianov]

On 06/25/2015 05:35 PM, Stephen Boyd wrote:
> On 06/25/2015 02:30 PM, santosh shilimkar wrote:
>> On 6/25/2015 2:02 PM, Stephen Boyd wrote:
>>> On 06/25/2015 08:04 AM, santosh shilimkar wrote:
>>>> On 6/25/2015 7:31 AM, Vitaly Andrianov wrote:
>>>>> This patch series adds support for arm L1/L2 ecc and ddr3 ecc error
>>>>> handling
>>>>> for Keystone devices
>>>>>
>>>>> Change Log
>>>>>
>>>>> v2:
>>>>> - removing unused and sorting headers of keystone.c are moved to a
>>>>> separate
>>>>>      patch.
>>>>> - l1l2 ecc and ddr3 ecc error handling are split it to separate
>>>>> patches
>>>>> - removed unused headers from keystone_ecc.c
>>>>> - platsmp.c removed from the patch.
>>>>> - return IRQ_HANDLED for 1 bit error in l1l2 ecc handler
>>>>> - checked and handled existing echttps://lwn.net/Articles/593336/c
>>>>> error before enabling ddr3 interrupt
>>>>> - 1 bit ddr3 interrupt is disabled, because it is handled by hardware
>>>>> and
>>>>>      there is no reason to handle it by software
>>>>>
>>>> This version looks good to me. As already commented, I would have liked
>>>> the patch 2/3(L2 ECC) code in ARM generic code so will give some more
>>>> time for others to come back. Otherwise I will queue this up for next
>>>> window.
>>>
>>> Why not make this into an edac driver? I sent out an L1/L2 error
>>> detection edac driver for Krait processors a year ago, but it stalled
>>> due to some DT binding stuff[1]. This looks fairly similar.
>>>
>> Indeed the error detection part is very similar(expected as well
>> considering the same processor L2 regs). I am not sure we need
>> full driver only for that but at least the IRQ error handler
>> related code can reside together. Lets see what RMK thinks
>> on this.
>>
>
> There's an existing one for highbank (drivers/edac/highbank_l2_edac.c)
> and there was a patch set for the pl310 as well[1]. I don't think we
> want any architecture specific code for this, just use the EDAC framework.
>
> [1] https://lkml.org/lkml/2014/3/2/87
>

Before porting that patch I was looking to implementation of the EDAC 
for L2 cache and tried to use the framework. Sorry, but I couldn't 
understand why would the Keystone platform may need it. Most likely 
because I didn't understand the framework itself :(

In order the Keystone ECC works u-boot has to initialize the entire DDR3 
and enable ECC. When kernel boots up it has to install the ECC interrupt 
handlers for Cortex-A15 L1/L2 ECC and Keystone2 DDR3 ECC. As far as 
1-bit errors handled and corrected by hardware the software even doesn't 
need to handle those errors. We need to handle 2-bit errors and just 
reboot the board.

As the ECC detection has to be enable on kernel boot time and cannot be 
disabled there is not sense to make it in a module.

So, shall Keystone use the EDAC framework to install the onetime working 
interrupt handler? What are advantages to use the framework?

I appreciate your opinion.

Thanks,
Vitaly

[PATCH v2 0/3] ARM: keystone: add ecc error interrupt handling

[Stephen Boyd]

On 06/26/2015 05:20 AM, Vitaly Andrianov wrote:
>
>
> On 06/25/2015 05:35 PM, Stephen Boyd wrote:
>>
>> There's an existing one for highbank (drivers/edac/highbank_l2_edac.c)
>> and there was a patch set for the pl310 as well[1]. I don't think we
>> want any architecture specific code for this, just use the EDAC
>> framework.
>>
>> [1] https://lkml.org/lkml/2014/3/2/87
>>
>
> Before porting that patch I was looking to implementation of the EDAC
> for L2 cache and tried to use the framework. Sorry, but I couldn't
> understand why would the Keystone platform may need it. Most likely
> because I didn't understand the framework itself :(
>
> In order the Keystone ECC works u-boot has to initialize the entire
> DDR3 and enable ECC. When kernel boots up it has to install the ECC
> interrupt handlers for Cortex-A15 L1/L2 ECC and Keystone2 DDR3 ECC. As
> far as 1-bit errors handled and corrected by hardware the software
> even doesn't need to handle those errors. We need to handle 2-bit
> errors and just reboot the board.
>
> As the ECC detection has to be enable on kernel boot time and cannot
> be disabled there is not sense to make it in a module.
>
> So, shall Keystone use the EDAC framework to install the onetime
> working interrupt handler? What are advantages to use the framework?
>

Some advantages that come to mind are user configurability and
statistics. Perhaps the user wants to know how many single bit errors
(correctable errors in EDAC language) have happened so they can figure
out if the part is going bad. Or perhaps the user wants to panic on
single bit errors to play it safe. The edac framework makes this sort of
thing standard, instead of requiring SoC specific tooling. It also
groups similar pieces of hardware support in one place and gets code out
of mach directories.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project