Krait L1/L2 EDAC driver

Krait CPUs have a handful of L2 cache controller registers that live behind a cp15 based indirection register. First you program the indirection register (l2cpselr) to point the L2 'window' register (l2cpdr) at what you want to read/write. Then you read/write the 'window' register to do what you want. The l2cpselr register is not banked per-cpu so we must lock around accesses to it to prevent other CPUs from re-pointing l2cpdr underneath us.

Cc Mark Rutland <[email protected]>

Cc Russell King <[email protected]>

Cc Courtney Cavin <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 arch/arm/common/Kconfig                   |  3 ++
 arch/arm/common/Makefile                  |  1 +
 arch/arm/common/krait-l2-accessors.c      | 58 +++++++++++++++++++++++++++++++
 arch/arm/include/asm/krait-l2-accessors.h | 20 +++++++++++
 4 files changed, 82 insertions(+)
 create mode 100644 arch/arm/common/krait-l2-accessors.c
 create mode 100644 arch/arm/include/asm/krait-l2-accessors.h

arch/arm/common/Kconfig +3 -0

index c3a4e9ceba34..9da52dc6260b 100644
@@ -9,6 +9,9 @@ config DMABOUNCE
 	bool
 	select ZONE_DMA
 
+config KRAIT_L2_ACCESSORS
+	bool
+
 config SHARP_LOCOMO
 	bool
 

arch/arm/common/Makefile +1 -0

index 70b1eff477b3..6b2cddf6e8d0 100644
@@ -7,6 +7,7 @@ obj-y				+= firmware.o
 obj-$(CONFIG_ICST)		+= icst.o
 obj-$(CONFIG_SA1111)		+= sa1111.o
 obj-$(CONFIG_DMABOUNCE)		+= dmabounce.o
+obj-$(CONFIG_KRAIT_L2_ACCESSORS) += krait-l2-accessors.o
 obj-$(CONFIG_SHARP_LOCOMO)	+= locomo.o
 obj-$(CONFIG_SHARP_PARAM)	+= sharpsl_param.o
 obj-$(CONFIG_SHARP_SCOOP)	+= scoop.o

arch/arm/common/krait-l2-accessors.c new file +1 -0

new file mode 100644
index 000000000000..5d514bbc88a6
@@ -0,0 +1,58 @@
+/*

arch/arm/common/krait-l2-accessors.c new file +47 -0

+ */
+
+#include <linux/spinlock.h>
+#include <linux/export.h>
+
+#include <asm/barrier.h>
+#include <asm/krait-l2-accessors.h>
+
+static DEFINE_RAW_SPINLOCK(krait_l2_lock);
+
+void krait_set_l2_indirect_reg(u32 addr, u32 val)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&krait_l2_lock, flags);
+	/*
+	 * Select the L2 window by poking l2cpselr, then write to the window
+	 * via l2cpdr.
+	 */
+	asm volatile ("mcr p15, 3, %0, c15, c0, 6 @ l2cpselr" : : "r" (addr));
+	isb();
+	asm volatile ("mcr p15, 3, %0, c15, c0, 7 @ l2cpdr" : : "r" (val));
+	isb();
+
+	raw_spin_unlock_irqrestore(&krait_l2_lock, flags);
+}
+EXPORT_SYMBOL(krait_set_l2_indirect_reg);
+
+u32 krait_get_l2_indirect_reg(u32 addr)
+{
+	u32 val;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&krait_l2_lock, flags);
+	/*
+	 * Select the L2 window by poking l2cpselr, then read from the window
+	 * via l2cpdr.
+	 */
+	asm volatile ("mcr p15, 3, %0, c15, c0, 6 @ l2cpselr" : : "r" (addr));
+	isb();
+	asm volatile ("mrc p15, 3, %0, c15, c0, 7 @ l2cpdr" : "=r" (val));
+
+	raw_spin_unlock_irqrestore(&krait_l2_lock, flags);
+
+	return val;
+}
+EXPORT_SYMBOL(krait_get_l2_indirect_reg);

arch/arm/include/asm/krait-l2-accessors.h new file +1 -0

new file mode 100644
index 000000000000..48fe5527bc01
@@ -0,0 +1,20 @@
+/*

Quoted 12 lines · Stephen Boyd

arch/arm/common/krait-l2-accessors.c new file +6 -0

new file mode 100644
index 000000000000..5d514bbc88a6
@@ -0,0 +1,58 @@
+/*
+ * Copyright (c) 2011-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.

arch/arm/common/krait-l2-accessors.c line 11

Quoted 5 lines

arch/arm/common/krait-l2-accessors.c new file +5 -0

+ *
+ * This program is distributed in the hope that 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.

Can we refer to COPYING here instead of adding that boilerplate to every file?

No. This is the template I'm told to use.

Quoted 1 line · Stephen Boyd

No. This is the template I'm told to use.

By whom? And why?

As I understand it, the license authors. They find it important to maintain clarity even when files get copied into other projects.

http://www.gnu.org/licenses/gpl-howto.html

http://www.gnu.org/licenses/gpl-faq.html#NoticeInSourceFile

Quoted 5 lines · Christopher Covington

As I understand it, the license authors. They find it important to maintain clarity even when files get copied into other projects.

http://www.gnu.org/licenses/gpl-howto.html

Quoted 1 line

http://www.gnu.org/licenses/gpl-faq.html#NoticeInSourceFile

Right, so what is wrong with stating the same thing in two lines:

"Copyright (c) 2011-2013, The Linux Foundation. All rights reserved.

This file is licensed under GNU GPLv2. See COPYING for full license text.

Including the without warranty is standard practice at a lot of companies. It's not even wasting space - it'll compress beautifully as there are already lots of similar headers all over the tree.

Documentation/devicetree/bindings/arm/cache.txt

Quoted 16 lines · Stephen Boyd

The Krait L1/L2 error reporting hardware is made up a per-CPU interrupt for the L1 cache and a SPI interrupt for the L2.

Cc Lorenzo Pieralisi <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Kumar Gala <[email protected]>

Cc <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 Documentation/devicetree/bindings/arm/cache.txt | 48 ++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/arm/cache.txt +0 -0

index b90fcc7c53cf..d7357e777399 100644

Right, that's http://www.spinics.net/lists/arm-kernel/msg308540.html

So whoever picks those patches up, Lorenzo's doc needs to be in his tree first too.

How about I review the EDAC part and an arm maintainer picks the whole series up? Would that be easier, logistically?

Documentation/devicetree/bindings/arm/cache.txt

Quoted 16 lines · Stephen Boyd

The Krait L1/L2 error reporting hardware is made up a per-CPU interrupt for the L1 cache and a SPI interrupt for the L2.

Cc Lorenzo Pieralisi <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Kumar Gala <[email protected]>

Cc <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 Documentation/devicetree/bindings/arm/cache.txt | 48 ++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/arm/cache.txt +0 -0

index b90fcc7c53cf..d7357e777399 100644

Right, that's http://www.spinics.net/lists/arm-kernel/msg308540.html

So whoever picks those patches up, Lorenzo's doc needs to be in his tree first too.

Sorry for the delay in replying. Those cache bindings need an ACK to get merged, and were introduced so that idle states can retrieve power domain information for caches. I am going to revive the idle bindings thread to see what we can/should merge of these bindings as things stand, I really hope this won't block the series any further, otherwise we can rework the patches so that this series can get in first, or simplify my series to allow both to get merged as soon as possible without compromising future requirements.

Add support for the Krait CPU cache error detection. This is a simplified version of the code originally written by Stepan Moskovchenko[1] ported to the EDAC device framework.

[1] https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/arch/arm/mach-msm/cache_erp.c?h=msm-3.4

Cc Stepan Moskovchenko <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/edac/Kconfig      |   8 +
 drivers/edac/Makefile     |   2 +
 drivers/edac/krait_edac.c | 370 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 380 insertions(+)
 create mode 100644 drivers/edac/krait_edac.c

drivers/edac/Kconfig +8 -0

index 878f09005fad..4dae3d353ea9 100644
@@ -368,4 +368,12 @@ config EDAC_OCTEON_PCI
 	  Support for error detection and correction on the
 	  Cavium Octeon family of SOCs.
 
+config EDAC_KRAIT_CACHE
+	tristate "Krait L1/L2 Cache"
+	depends on EDAC_MM_EDAC && ARM && OF
+	select KRAIT_L2_ACCESSORS
+	help
+	  Support for error detection and correction on the
+	  Krait L1/L2 cache controller.
+
 endif # EDAC

drivers/edac/Makefile +2 -0

index 4154ed6a02c6..b6ea50564223 100644
@@ -64,3 +64,5 @@ obj-$(CONFIG_EDAC_OCTEON_PC)		+= octeon_edac-pc.o
 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_KRAIT_CACHE)		+= krait_edac.o

drivers/edac/krait_edac.c new file +56 -0

new file mode 100644
index 000000000000..90ec0e982927
@@ -0,0 +1,370 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/cpu.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+
+#include <asm/krait-l2-accessors.h>
+
+#include "edac_core.h"
+
+#define CESR_DCTPE		BIT(0)
+#define CESR_DCDPE		BIT(1)
+#define CESR_ICTPE		BIT(2)
+#define CESR_ICDPE		BIT(3)
+#define CESR_DCTE		(BIT(4) | BIT(5))
+#define CESR_ICTE		(BIT(6) | BIT(7))
+#define CESR_TLBMH		BIT(16)
+#define CESR_I_MASK		0x000000cc
+/* Print a message for everything but TLB MH events */
+#define CESR_PRINT_MASK		0x000000ff
+
+#define L2ESR			0x204
+#define L2ESR_MPDCD		BIT(0)
+#define L2ESR_MPSLV		BIT(1)
+#define L2ESR_TSESB		BIT(2)
+#define L2ESR_TSEDB		BIT(3)
+#define L2ESR_DSESB		BIT(4)
+#define L2ESR_DSEDB		BIT(5)
+#define L2ESR_MSE		BIT(6)
+#define L2ESR_MPLDREXNOK	BIT(8)
+#define L2ESR_CPU_MASK		0xf
+#define L2ESR_CPU_SHIFT		16
+#define L2ESR_SP		BIT(20)
+
+#define L2ESYNR0		0x208
+#define L2ESYNR1		0x209
+#define L2EAR0			0x20c
+#define L2EAR1			0x20d
+

drivers/edac/krait_edac.c new file +34 -0

+};
+
+static unsigned int read_cesr(void)
+{
+	unsigned int cesr;
+
+	asm volatile ("mrc p15, 7, %0, c15, c0, 1 @ cesr" : "=r" (cesr));
+	return cesr;
+}
+
+static void write_cesr(unsigned int cesr)
+{
+	asm volatile ("mcr p15, 7, %0, c15, c0, 1 @ cesr" : : "r" (cesr));
+}
+
+static unsigned int read_cesynr(void)
+{
+	unsigned int cesynr;
+
+	asm volatile ("mrc p15, 7, %0, c15, c0, 3 @ cesynr" : "=r" (cesynr));
+	return cesynr;
+}
+
+static irqreturn_t krait_l1_irq(int irq, void *dev_id)
+{
+	struct edac_device_ctl_info **edac_p = dev_id;
+	struct edac_device_ctl_info *edac = *edac_p;
+	unsigned int cesr = read_cesr();
+	unsigned int i_cesynr, d_cesynr;
+	unsigned int cpu = smp_processor_id();
+	int print_regs = cesr & CESR_PRINT_MASK;
+	int i;
+	static const struct krait_edac_error errors[] = {
+		{ "D-cache tag parity error", edac_device_handle_ue },

drivers/edac/krait_edac.c new file +24 -0

+	}
+
+	/* Clear the interrupt bits we processed */
+	write_cesr(cesr);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t krait_l2_irq(int irq, void *dev_id)
+{
+	struct edac_device_ctl_info *edac = dev_id;
+	unsigned int l2esr;
+	unsigned int l2esynr0;
+	unsigned int l2esynr1;
+	unsigned int l2ear0;
+	unsigned int l2ear1;
+	unsigned long cpu;
+	int i;
+	static const struct krait_edac_error errors[] = {
+		{ "master port decode error", edac_device_handle_ce },
+		{ "master port slave error", edac_device_handle_ce },
+		{ "tag soft error, single-bit", edac_device_handle_ce },
+		{ "tag soft error, double-bit", edac_device_handle_ue },
+		{ "data soft error, single-bit", edac_device_handle_ce },

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

+	}
+
+	cpu = (l2esr >> L2ESR_CPU_SHIFT) & L2ESR_CPU_MASK;
+	cpu = __ffs(cpu);
+	if (cpu)
+		cpu--;

drivers/edac/krait_edac.c line 66

Quoted 5 lines · Stephen Boyd

drivers/edac/krait_edac.c new file +5 -0

+
+struct krait_edac_error {
+	const char * const msg;
+	void (*func)(struct edac_device_ctl_info *edac_dev,
+			int inst_nr, int block_nr, const char *msg);

arg alignment (please start new line at the opening brace).

Quoted 4 lines

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

+	int print_regs = cesr & CESR_PRINT_MASK;
+	int i;
+	static const struct krait_edac_error errors[] = {
+		{ "D-cache tag parity error", edac_device_handle_ue },

drivers/edac/krait_edac.c line 110

Quoted 10 lines

drivers/edac/krait_edac.c new file +10 -0

+		{ "D-cache data parity error", edac_device_handle_ue },
+		{ "I-cache tag parity error", edac_device_handle_ce },
+		{ "I-cache data parity error", edac_device_handle_ce },
+		{ "D-cache tag timing error", edac_device_handle_ue },
+		{ "D-cache data timing error", edac_device_handle_ue },
+		{ "I-cache tag timing error", edac_device_handle_ce },
+		{ "I-cache data timing error", edac_device_handle_ce }
+	};
+
+	if (print_regs) {

This variable is used only once here, you can simply do the binary and test then and drop it:

if (cesr & CESR_PRINT_MASK)

drivers/edac/krait_edac.c line 112

Quoted 3 lines

drivers/edac/krait_edac.c new file +2 -0

+		pr_alert("L1 / TLB Error detected on CPU %d!\n", cpu);
+		pr_alert("CESR      = 0x%08x\n", cesr);

You can use the edac_*_printk with KERN_ALERT as level which adds proper prefixes.

drivers/edac/krait_edac.c line 126

Quoted 11 lines

drivers/edac/krait_edac.c new file +9 -0

+
+	if (cesr & CESR_TLBMH) {
+		asm ("mcr p15, 0, r0, c8, c7, 0");
+		edac_device_handle_ce(edac, cpu, 0, "TLB Multi-Hit error");
+	}
+
+	if (cesr & (CESR_ICTPE | CESR_ICDPE | CESR_ICTE)) {
+		i_cesynr = read_cesynr();
+		pr_alert("I-side CESYNR = 0x%08x\n", i_cesynr);

edac_printk

and also, this message looks a bit cryptic for issuing it at ALERT level. I'm ssuming people won't come to you and ask you what it means...? :)

Quoted 4 lines

drivers/edac/krait_edac.c new file +3 -0

+		write_cesr(CESR_I_MASK);
+
+		/*

drivers/edac/krait_edac.c line 139

Quoted 10 lines

drivers/edac/krait_edac.c new file +10 -0

+		 * Clear the I-side bits from the captured CESR value so that we
+		 * don't accidentally clear any new I-side errors when we do
+		 * the CESR write-clear operation.
+		 */
+		cesr &= ~CESR_I_MASK;
+	}
+
+	if (cesr & (CESR_DCTPE | CESR_DCDPE | CESR_DCTE)) {
+		d_cesynr = read_cesynr();
+		pr_alert("D-side CESYNR = 0x%08x\n", d_cesynr);

Ditto.

Quoted 28 lines

drivers/edac/krait_edac.c new file +28 -0

+	}
+
+	/* Clear the interrupt bits we processed */
+	write_cesr(cesr);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t krait_l2_irq(int irq, void *dev_id)
+{
+	struct edac_device_ctl_info *edac = dev_id;
+	unsigned int l2esr;
+	unsigned int l2esynr0;
+	unsigned int l2esynr1;
+	unsigned int l2ear0;
+	unsigned int l2ear1;
+	unsigned long cpu;
+	int i;
+	static const struct krait_edac_error errors[] = {
+		{ "master port decode error", edac_device_handle_ce },
+		{ "master port slave error", edac_device_handle_ce },
+		{ "tag soft error, single-bit", edac_device_handle_ce },
+		{ "tag soft error, double-bit", edac_device_handle_ue },
+		{ "data soft error, single-bit", edac_device_handle_ce },
+		{ "data soft error, double-bit", edac_device_handle_ue },
+		{ "modified soft error", edac_device_handle_ce },
+		{ "slave port exclusive monitor not available",
+			edac_device_handle_ue},

drivers/edac/krait_edac.c line 173

Quoted 6 lines

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

+		{ "master port LDREX received Normal OK response",
+			edac_device_handle_ce },
+	};
+
+	l2esr = krait_get_l2_indirect_reg(L2ESR);
+	pr_alert("Error detected!\n");

Why print this not very telling message here if errors[i].func() will get the proper .msg later?

drivers/edac/krait_edac.c line 185

Quoted 13 lines

drivers/edac/krait_edac.c new file +12 -0

+	pr_alert("L2ESR    = 0x%08x\n", l2esr);
+
+	if (l2esr & (L2ESR_TSESB | L2ESR_TSEDB | L2ESR_MSE | L2ESR_SP)) {
+		l2esynr0 = krait_get_l2_indirect_reg(L2ESYNR0);
+		l2esynr1 = krait_get_l2_indirect_reg(L2ESYNR1);
+		l2ear0 = krait_get_l2_indirect_reg(L2EAR0);
+		l2ear1 = krait_get_l2_indirect_reg(L2EAR1);
+
+		pr_alert("L2ESYNR0 = 0x%08x\n", l2esynr0);
+		pr_alert("L2ESYNR1 = 0x%08x\n", l2esynr1);
+		pr_alert("L2EAR0   = 0x%08x\n", l2ear0);
+		pr_alert("L2EAR1   = 0x%08x\n", l2ear1);

Also, please use edac_printk and consider making those messages human-readable, otherwise it only confuses users.

Quoted 3 lines

and also, this message looks a bit cryptic for issuing it at ALERT level. I'm ssuming people won't come to you and ask you what it means...? :)

Ok. I can lower it to error level?