arm64: enable EDAC on arm64
PATCH v2 1/1 arm64: enable EDAC on arm64
From: Rob Herring <[email protected]>
Implement atomic_scrub and enable EDAC for arm64.
---
v2:
- Add loop for exclusive store success
- Fix size to be 32-bits at a time. The framework gives no alignment guarantees.
arch/arm64/Kconfig | 1 + arch/arm64/include/asm/edac.h | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 arch/arm64/include/asm/edac.h
arch/arm64/Kconfig +1 -0
@@ -11,6 +11,7 @@ config ARM64 select BUILDTIME_EXTABLE_SORT select CLONE_BACKWARDS select COMMON_CLK + select EDAC_SUPPORT select GENERIC_CLOCKEVENTS select GENERIC_IOMAP select GENERIC_IRQ_PROBE
arch/arm64/include/asm/edac.h new file +43 -0
43 lines not on this page · raw message
Signature · 1 line
1.8.1.2
PATCH v2 1/1 arm64: enable EDAC on arm64
Quoted 35 lines · Rob Herring
arch/arm64/include/asm/edac.h new file +29
-0@@ -0,0 +1,43 @@ +/* +* Copyright 2013 Calxeda, Inc. +* Based on PPC version Copyright 2007 MontaVista Software, 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/>. +*/ +#ifndef ASM_EDAC_H +#define ASM_EDAC_H +/* +* ECC atomic, DMA, SMP and interrupt safe scrub function. +* Implements the per arch atomic_scrub() that EDAC use for software +* ECC scrubbing. It reads memory and then writes back the original +* value, allowing the hardware to detect and correct memory errors. +*/ +static inline void atomic_scrub (void *va,u32 size) +{ +unsigned int *virt_addr = va; +unsigned int i; +
arch/arm64/include/asm/edac.h line 39
Quoted 10 lines
arch/arm64/include/asm/edac.h new file +10
-0+for (i =0 ; i < size /sizeof (*virt_addr); i++, virt_addr++) { +long result; +unsigned long tmp; + + asmvolatile ("// atomic_scrub\n" +"1: ldxr %w0, %2\n" +" stxr %w1, %w0, %2\n" +" cbnz %w1, 1b" + :"=&r" (result),"=&r" (tmp),"+Q" (*virt_addr) + : :"cc" );
You still don't need the "cc" clobber (cbnz doesn't update the flags and, yes, we have redundant clobbers in our atomic.h which I'll fix...).
Will
PATCH v2 1/1 arm64: enable EDAC on arm64
Quoted 41 lines · Will Deacon
Quoted 40 lines · Rob Herring
arch/arm64/include/asm/edac.h new file +34
-0@@ -0,0 +1,43 @@ +/* +* Copyright 2013 Calxeda, Inc. +* Based on PPC version Copyright 2007 MontaVista Software, 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/>. +*/ +#ifndef ASM_EDAC_H +#define ASM_EDAC_H +/* +* ECC atomic, DMA, SMP and interrupt safe scrub function. +* Implements the per arch atomic_scrub() that EDAC use for software +* ECC scrubbing. It reads memory and then writes back the original +* value, allowing the hardware to detect and correct memory errors. +*/ +static inline void atomic_scrub (void *va,u32 size) +{ +unsigned int *virt_addr = va; +unsigned int i; + +for (i =0 ; i < size /sizeof (*virt_addr); i++, virt_addr++) { +long result; +unsigned long tmp; + + asm volatile("// atomic_scrub\n"
Quoted 8 lines
arch/arm64/include/asm/edac.h line 39
Quoted 5 lines
arch/arm64/include/asm/edac.h new file +5
-0+"1: ldxr %w0, %2\n" +" stxr %w1, %w0, %2\n" +" cbnz %w1, 1b" + : "=&r" (result), "=&r" (tmp), "+Q" (*virt_addr) + : : "cc");You still don't need the "cc" clobber (cbnz doesn't update the flags and, yes, we have redundant clobbers in our
atomic.hwhich I'll fix...).
Okay, I thought since the first version did not have the cbnz was why you said to remove it.
Rob
Catalin Marinas
maintainerPATCH v2 1/1 arm64: enable EDAC on arm64
In reply to Rob Herringarch/arm64/include/asm/edac.h line 30
Quoted 6 lines · Rob Herring
arch/arm64/include/asm/edac.h new file +6
-0+static inline void atomic_scrub (void *va,u32 size) +{ +unsigned int *virt_addr = va; +unsigned int i; + +for (i =0 ; i < size /sizeof (*virt_addr); i++, virt_addr++) {
BTW, maybe the compiler is smart enough to drop the i but why not just use a int *last_addr = va + size - 3; and just compare against this?
Signature · 1 line
Catalin
PATCH v2 1/1 arm64: enable EDAC on arm64
Quoted 10 lines · Catalin Marinas
arch/arm64/include/asm/edac.h line 30
Quoted 6 lines · Rob Herring
arch/arm64/include/asm/edac.h new file +6
-0+static inline void atomic_scrub (void *va,u32 size) +{ +unsigned int *virt_addr = va; +unsigned int i; + +for (i =0 ; i < size /sizeof (*virt_addr); i++, virt_addr++) {BTW, maybe the compiler is smart enough to drop the i but why not just use a int
*last_addr= va + size - 3; and just compare against this?
Evidently, the compiler is not that smart. Here's what I ended up with. There's no need to subtract 3 that I can see:
unsigned int *last_addr = va + size;
for (; virt_addr < last_addr; virt_addr++) {
original:
ffffffc0001b0280 <atomic_scrub>:
ffffffc0001b0280: 53027c21 lsr w1, w1, #2 ffffffc0001b0284: 34000141 cbz w1, ffffffc0001b02ac <atomic_scrub+0x2c> ffffffc0001b0288: 51000421 sub w1, w1, #0x1 ffffffc0001b028c: 91000421 add x1, x1, #0x1 ffffffc0001b0290: 8b010801 add x1, x0, x1, lsl #2 ffffffc0001b0294: 885f7c02 ldxr w2, [x0] ffffffc0001b0298: 88037c02 stxr w3, w2, [x0] ffffffc0001b029c: 35ffffc3 cbnz w3, ffffffc0001b0294 <atomic_scrub+0x14> ffffffc0001b02a0: 91001000 add x0, x0, #0x4 ffffffc0001b02a4: eb01001f cmp x0, x1 ffffffc0001b02a8: 54ffff61 b.ne ffffffc0001b0294
<atomic_scrub+0x14>ffffffc0001b02ac: d65f03c0 ret
using last_addr:
ffffffc0001b0280 <atomic_scrub>:
ffffffc0001b0280: 8b214001 add x1, x0, w1, uxtw ffffffc0001b0284: eb01001f cmp x0, x1 ffffffc0001b0288: 540000e2 b.cs ffffffc0001b02a4 <atomic_scrub+0x24> ffffffc0001b028c: 885f7c02 ldxr w2, [x0] ffffffc0001b0290: 88037c02 stxr w3, w2, [x0] ffffffc0001b0294: 35ffffc3 cbnz w3, ffffffc0001b028c <atomic_scrub+0xc> ffffffc0001b0298: 91001000 add x0, x0, #0x4 ffffffc0001b029c: eb00003f cmp x1, x0 ffffffc0001b02a0: 54ffff68 b.hi ffffffc0001b028c
<atomic_scrub+0xc>ffffffc0001b02a4: d65f03c0 ret
Rob
Catalin Marinas
maintainerPATCH v2 1/1 arm64: enable EDAC on arm64
Quoted 13 lines · Rob Herring
Quoted 10 lines · Catalin Marinas
arch/arm64/include/asm/edac.h line 30
Quoted 6 lines · Rob Herring
arch/arm64/include/asm/edac.h new file +6
-0+static inline void atomic_scrub (void *va,u32 size) +{ +unsigned int *virt_addr = va; +unsigned int i; + +for (i =0 ; i < size /sizeof (*virt_addr); i++, virt_addr++) {BTW, maybe the compiler is smart enough to drop the i but why not just use a int
*last_addr= va + size - 3; and just compare against this?
Quoted 5 lines
Evidently, the compiler is not that smart. Here's what I ended up with. There's no need to subtract 3 that I can see:
unsigned int
*last_addr= va + size;
for (;virt_addr<last_addr; virt_addr++) {
The 3 thing was for size not multiple of 4. Do we have any guarantees here?
Signature · 1 line
Catalin
PATCH v2 1/1 arm64: enable EDAC on arm64
Quoted 14 lines · Catalin Marinas
Quoted 13 lines · Rob Herring
Quoted 10 lines · Catalin Marinas
arch/arm64/include/asm/edac.h line 30
Quoted 6 lines · Rob Herring
arch/arm64/include/asm/edac.h new file +6
-0+static inline void atomic_scrub (void *va,u32 size) +{ +unsigned int *virt_addr = va; +unsigned int i; + +for (i =0 ; i < size /sizeof (*virt_addr); i++, virt_addr++) {BTW, maybe the compiler is smart enough to drop the i but why not just use a int
*last_addr= va + size - 3; and just compare against this?
Quoted 8 lines
Quoted 5 lines
Evidently, the compiler is not that smart. Here's what I ended up with. There's no need to subtract 3 that I can see:
unsigned int
*last_addr= va + size;
for (;virt_addr<last_addr; virt_addr++) {The 3 thing was for size not multiple of 4. Do we have any guarantees here?
Only that no one does ECC on less that 64-bit wide DDR. I'm only assuming 32-bit alignment and that is what all the other implementations assume.
If we have a non-multiple of 4 size, then we want to expand the size to include the partial bytes. This function is a nop effectively, so the fact that we may touch 1-3 extra bytes at the end does not matter. The only thing subtracting 3 does is mean the comparison needs to be '<=' instead of '<'. It would also break if you had a size of 1 or 2. I think what you intended was to align size, but you really need to add 3 and mask it to do that. But that is not necessary in my version.
Rob
Catalin Marinas
maintainerPATCH v2 1/1 arm64: enable EDAC on arm64
Quoted 28 lines · Rob Herring
Quoted 14 lines · Catalin Marinas
Quoted 13 lines · Rob Herring
Quoted 10 lines · Catalin Marinas
arch/arm64/include/asm/edac.h line 30
Quoted 6 lines · Rob Herring
arch/arm64/include/asm/edac.h new file +6
-0+static inline void atomic_scrub (void *va,u32 size) +{ +unsigned int *virt_addr = va; +unsigned int i; + +for (i =0 ; i < size /sizeof (*virt_addr); i++, virt_addr++) {BTW, maybe the compiler is smart enough to drop the i but why not just use a int
*last_addr= va + size - 3; and just compare against this?Quoted 8 lines
Quoted 5 lines
Evidently, the compiler is not that smart. Here's what I ended up with. There's no need to subtract 3 that I can see:
unsigned int
*last_addr= va + size;
for (;virt_addr<last_addr; virt_addr++) {The 3 thing was for size not multiple of 4. Do we have any guarantees here?
Only that no one does ECC on less that 64-bit wide DDR. I'm only assuming 32-bit alignment and that is what all the other implementations assume.
Quoted 7 lines
If we have a non-multiple of 4 size, then we want to expand the size to include the partial bytes. This function is a nop effectively, so the fact that we may touch 1-3 extra bytes at the end does not matter. The only thing subtracting 3 does is mean the comparison needs to be '<=' instead of '<'. It would also break if you had a size of 1 or 2. I think what you intended was to align size, but you really need to add 3 and mask it to do that. But that is not necessary in my version.
What I wanted was to make sure it doesn't write more bytes than size. If you think it's safe to write the extra bytes, than you don't need any subtraction.
Signature · 1 line
Catalin
PATCH v2 1/1 arm64: enable EDAC on arm64
In reply to Rob HerringQuoted 5 lines · Rob Herring <robherring2 <at> gmail.com> writes:
From: Rob Herring <rob.herring <at> calxeda.com>
Implement
atomic_scruband enable EDAC for arm64.
Quoted 9 lines
Signed-off-by Rob Herring <rob.herring <at> calxeda.com>Cc Catalin Marinas <catalin.marinas <at> arm.com>Cc Will Deacon <will.deacon <at> arm.com>---
v2:
- Add loop for exclusive store success
- Fix size to be 32-bits at a time. The framework gives no alignment guarantees.
Hi Rob,
I am working on memory error for ARM64 and wondering what is your plans regarding this patch. It looks essential for my further work.
Regards,
Tomasz
PATCH v2 1/1 arm64: enable EDAC on arm64
Quoted 15 lines · Tomasz Nowicki
Quoted 4 lines · Rob Herring <robherring2 <at> gmail.com> writes:
From: Rob Herring <rob.herring <at> calxeda.com>
Implement
atomic_scruband enable EDAC for arm64.Quoted 9 lines
Signed-off-by Rob Herring <rob.herring <at> calxeda.com>Cc Catalin Marinas <catalin.marinas <at> arm.com>Cc Will Deacon <will.deacon <at> arm.com>---
v2:
- Add loop for exclusive store success
- Fix size to be 32-bits at a time. The framework gives no alignment guarantees.
Quoted 8 lines
Hi Rob,
I am working on memory error for ARM64 and wondering what is your plans regarding this patch. It looks essential for my further work.
Regards,
Tomasz
Hi Tomasz, Rob,
I am looking at similar information for A53 and A57 for the L1/L2 single and double bit ecc errors.
Is there an upstream/linaro effort for EDAC driver in progress ?
-Rohit
Quoted 5 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Thanks,
Rohit Vaswani
Signature · 1 line
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
PATCH v2 1/1 arm64: enable EDAC on arm64
Quoted 22 lines · Rohit Vaswani
Quoted 21 lines · Tomasz Nowicki
Quoted 4 lines · Rob Herring <robherring2 <at> gmail.com> writes:
From: Rob Herring <rob.herring <at> calxeda.com>
Implement
atomic_scruband enable EDAC for arm64.Quoted 9 lines
Signed-off-by Rob Herring <rob.herring <at> calxeda.com>Cc Catalin Marinas <catalin.marinas <at> arm.com>Cc Will Deacon <will.deacon <at> arm.com>---
v2:
- Add loop for exclusive store success
- Fix size to be 32-bits at a time. The framework gives no alignment guarantees.Hi Rob,
I am working on memory error for ARM64 and wondering what is your plans regarding this patch. It looks essential for my further work.
Quoted 8 lines
Quoted 3 lines
Regards,
TomaszHi Tomasz, Rob,
I am looking at similar information for A53 and A57 for the L1/L2 single and double bit ecc errors.
Is there an upstream/linaro effort for EDAC driver in progress ?
I just sent out v3.
There is work to develop a generic RAS framework based on perf which would hopefully replace EDAC and mcelog(x86 only). Jean Pihet has picked up this work recently. There probably needs to be some infrastructure work to handle SError events as I believe that is how cache ECC errors are reported. Although those may get handled by firmware and then passed to the OS via some other mechanism like ACPI.
Rob
PATCH v2 1/1 arm64: enable EDAC on arm64
Hi Rob,
Thanks for taking care of that. Please see question below.
Quoted 28 lines · Rob Herring
Quoted 27 lines · Rohit Vaswani
Quoted 16 lines · Tomasz Nowicki
Quoted 4 lines · Rob Herring <robherring2 <at> gmail.com> writes:
From: Rob Herring <rob.herring <at> calxeda.com>
Implement
atomic_scruband enable EDAC for arm64.Quoted 9 lines
Signed-off-by Rob Herring <rob.herring <at> calxeda.com>Cc Catalin Marinas <catalin.marinas <at> arm.com>Cc Will Deacon <will.deacon <at> arm.com>---
v2:
- Add loop for exclusive store success
- Fix size to be 32-bits at a time. The framework gives no alignment guarantees.Quoted 8 lines
Hi Rob,
I am working on memory error for ARM64 and wondering what is your plans regarding this patch. It looks essential for my further work.
Regards,
TomaszHi Tomasz, Rob,
Quoted 8 lines
Quoted 3 lines
I am looking at similar information for A53 and A57 for the L1/L2 single and double bit ecc errors.
Is there an upstream/linaro effort for EDAC driver in progress ?I just sent out v3.
There is work to develop a generic RAS framework based on perf which would hopefully replace EDAC and mcelog(x86 only).
What do you mean by EDAC and mcelog replacement with RAS framework?
Quoted 5 lines
Jean Pihet has
picked up this work recently. There probably needs to be some infrastructure work to handle SError events as I believe that is how cache ECC errors are reported. Although those may get handled by firmware and then passed to the OS via some other mechanism like ACPI.
Tomasz