Krait L1/L2 EDAC driver

We only setup a workqueue for edac devices that use the polling method. We still try to cancel the workqueue if an edac_device uses the irq method though. This causes a warning from debug objects when we remove an edac device:

WARNING: CPU: 0 PID: 56 at lib/debugobjects.c:260 debug_print_object+0x98/0xc0()
ODEBUG: assert_init not available (active state 0) object type: timer_list hint: stub_timer+0x0/0x28
Modules linked in: krait_edac(-)
CPU: 0 PID: 56 Comm: rmmod Not tainted 3.12.0-rc2-00035-g15292a0 #69
(unwind_backtrace+0x0/0x144)
(show_stack+0x20/0x24)
(dump_stack+0x74/0xb4)
(warn_slowpath_common+0x78/0x9c)
(warn_slowpath_fmt+0x40/0x48)
(debug_print_object+0x98/0xc0)
(debug_object_assert_init+0xdc/0xec)
(del_timer+0x24/0x7c)
(try_to_grab_pending+0xc0/0x1b0)
(cancel_delayed_work+0x2c/0xa0)
(edac_device_workq_teardown+0x1c/0x38)
(edac_device_del_device+0xb8/0xe4)
(krait_edac_remove+0x50/0x70 [krait_edac])
(platform_drv_remove+0x24/0x28)
(__device_release_driver+0x68/0xc0)
(driver_detach+0xc4/0xc8)
(bus_remove_driver+0xac/0x114)
(driver_unregister+0x38/0x58)
(platform_driver_unregister+0x1c/0x20)
(krait_edac_driver_exit+0x14/0x1c [krait_edac])
(SyS_delete_module+0x178/0x2b4)

Fix it by skipping the workqueue teardown for such devices.

Acked-by Borislav Petkov <[email protected]>

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

In the near future we're going to use these percpu irq functions in the Krait CPU EDAC driver. Export them so that the EDAC driver can be compiled as a module.

Acked-by Thomas Gleixner <[email protected]>

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

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]>

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 c3a4e9c..9da52dc 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 4bdc4162..2836f99 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 +58 -0

new file mode 100644
index 0000000..f17c361
@@ -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.
+ *
+ * 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/spinlock.h>
+#include <linux/export.h>
+
+#include <asm/barrier.h>
+#include <asm/krait-l2-accessors.h>
+
+static DEFINE_RAW_SPINLOCK(krait_l2_lock);
+
+void 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(set_l2_indirect_reg);
+
+u32 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(get_l2_indirect_reg);

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

new file mode 100644
index 0000000..d5305c4
@@ -0,0 +1,20 @@
+/*
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,

Quoted 9 lines · Stephen Boyd

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.

Quoted 8 lines

Cc Mark Rutland <[email protected]>

Cc Russell King <[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 +++++++++++

I'm no ARM guy but out of curiosity, why is this code not part of the krait edac driver? IOW, is there a compelling reason for it to be in arch/arm/common/?

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]>

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

Quoted 12 lines · Stephen Boyd

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]>

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

Quoted 8 lines

---
 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/include/asm/krait-l2-accessors.h line 18

Quoted 3 lines

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

+
+extern void set_l2_indirect_reg(u32 addr, u32 val);
+extern u32 get_l2_indirect_reg(u32 addr);

As these are Krait specific, please rename the functions to reflect this.

Ok. Borislav, should I resend to add krait_ before these functions?

The Krait L1/L2 error reporting device is made up of two interrupts, one per-CPU interrupt for the L1 caches and one interrupt for the L2 cache.

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/cpus.txt | 72 ++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

Documentation/devicetree/bindings/arm/cpus.txt +1 -0

index 9130435..54de94b 100644
@@ -191,6 +191,35 @@ nodes to be present and contain the properties described below.
 			  property identifying a 64-bit zero-initialised
 			  memory location.
 
+	- interrupts

Not sure this binding (cache node) belongs in cpus.txt

I am working on defining cache bindings for ARM within the C-state standardization effort:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/215543.html

Documentation/devicetree/bindings/arm/cpus.txt nodes to be present and contain the properties described below. · line 211

Quoted 8 lines

Documentation/devicetree/bindings/arm/cpus.txt +7 -0

+
+	Description: Describes a cache in an ARM based system
+
+	- compatible
+		Usage: required
+		Value type: <string>
+		Definition: shall contain at least "cache"

It is a bit vague, can't we just follow the ePAPR compatible definition ? See posting above.

Documentation/devicetree/bindings/arm/cpus.txt nodes to be present and contain the properties described below. · line 216

Quoted 6 lines

Documentation/devicetree/bindings/arm/cpus.txt +5 -0

+
+	- cache-level
+		Usage: required
+		Value type: <u32>
+		Definition: level in the cache heirachy

"hierarchy".

I have a problem with the cache level definition, and in particular the numbering, ie what the level number represents. If we mean the cache level seen through the CLIDR and co., it is hard to use it for shared caches since the level seen by different CPUs can actually be different, or put it differently the level number might not be unique for a shared cache. I need to think about a proper way to sort this out.

Quoted 7 lines · Lorenzo Pieralisi

Not sure this binding (cache node) belongs in cpus.txt

I am working on defining cache bindings for ARM within the C-state standardization effort:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/215543.html

Thanks I'll take a look.

Quoted 11 lines

Documentation/devicetree/bindings/arm/cpus.txt nodes to be present and contain the properties described below. · line 211

Quoted 8 lines

Documentation/devicetree/bindings/arm/cpus.txt +7 -0

+
+	Description: Describes a cache in an ARM based system
+
+	- compatible
+		Usage: required
+		Value type: <string>
+		Definition: shall contain at least "cache"

It is a bit vague, can't we just follow the ePAPR compatible definition ? See posting above.

Hm.. I thought this did follow the ePAPR spec. I see 'compatible, required, string, A standard property. The value shall include the string "cache".' Looks the same?

And I see 'cache-level, required, u32, Specifies the level in the cache hierarchy. For example, a level 2 cache has a value of <2>.'

Quoted 8 lines

Documentation/devicetree/bindings/arm/cpus.txt nodes to be present and contain the properties described below. · line 216

Quoted 6 lines

Documentation/devicetree/bindings/arm/cpus.txt +5 -0

+
+	- cache-level
+		Usage: required
+		Value type: <u32>
+		Definition: level in the cache heirachy

"hierarchy".

Thanks.

Quoted 7 lines

I have a problem with the cache level definition, and in particular the numbering, ie what the level number represents. If we mean the cache level seen through the CLIDR and co., it is hard to use it for shared caches since the level seen by different CPUs can actually be different, or put it differently the level number might not be unique for a shared cache. I need to think about a proper way to sort this out.

Ok. I don't even use this property in my driver. All I really need is the phandle from cpus pointing to the L2 and the interrupts property in the L2 node.

How do you want to proceed here? If your cache binding goes through I would just need to add the interrupts part. Or you could even add that part in the same patch, you could have my signed-off-by for that.

Quoted 9 lines · Stephen Boyd

Quoted 7 lines · Lorenzo Pieralisi

I have a problem with the cache level definition, and in particular the numbering, ie what the level number represents. If we mean the cache level seen through the CLIDR and co., it is hard to use it for shared caches since the level seen by different CPUs can actually be different, or put it differently the level number might not be unique for a shared cache. I need to think about a proper way to sort this out.

Quoted 8 lines

Ok. I don't even use this property in my driver. All I really need is the phandle from cpus pointing to the L2 and the interrupts property in the L2 node.

How do you want to proceed here? If your cache binding goes through I would just need to add the interrupts part. Or you could even add that part in the same patch, you could have my signed-off-by for that.

Ok, I will try to update the bindings with the interrupt part and copy you in, even though the level definition worries me a bit, it is an important property for power management and I need to find a proper solution before bindings can get accepted (basically the problem is:
if different CPUs can see a cache at different levels as defined in the CLIDR we cannot describe a cache with a single cache level or put it differently, level can not represent the value in the CLIDR hence we need to describe it differently).

Quoted 18 lines · Lorenzo Pieralisi

Quoted 9 lines · Stephen Boyd

Quoted 7 lines · Lorenzo Pieralisi

I have a problem with the cache level definition, and in particular the numbering, ie what the level number represents. If we mean the cache level seen through the CLIDR and co., it is hard to use it for shared caches since the level seen by different CPUs can actually be different, or put it differently the level number might not be unique for a shared cache. I need to think about a proper way to sort this out.

Quoted 8 lines

Ok. I don't even use this property in my driver. All I really need is the phandle from cpus pointing to the L2 and the interrupts property in the L2 node.

How do you want to proceed here? If your cache binding goes through I would just need to add the interrupts part. Or you could even add that part in the same patch, you could have my signed-off-by for that.

Quoted 8 lines

Ok, I will try to update the bindings with the interrupt part and copy you in, even though the level definition worries me a bit, it is an important property for power management and I need to find a proper solution before bindings can get accepted (basically the problem is:
if different CPUs can see a cache at different levels as defined in the CLIDR we cannot describe a cache with a single cache level or put it differently, level can not represent the value in the CLIDR hence we need to describe it differently).

Ok. I've dropped the cache part from this patch. I left the example as is minus the cache-level attribute.

Understanding how the cache-level value would be used might help. I wonder if the cache-level can just be a number that describes the largest value that the cache could be assigned. Then if you have different CPUs seeing different levels of cache they can traverse from their CPU node to the cache and count how many phandles they went through.

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 | 346 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 356 insertions(+)
 create mode 100644 drivers/edac/krait_edac.c

This patchset adds support for the Krait L1/L2 cache error detection hardware. The first patch fixes a generic framework bug. The next two patches lay the groundwork for this driver to be added by exporting percpu irq functions as well as adding the Krait l2 indirection register code. The next two patches add the driver and the binding and the final patch hooks it all up by adding the device tree node.

I'm not sure which tree this is supposed to go through. Ideally we could send the first 3 plus the 5th one through an edac tree.

Quoted 7 lines · Stephen Boyd

This patchset adds support for the Krait L1/L2 cache error detection hardware. The first patch fixes a generic framework bug. The next two patches lay the groundwork for this driver to be added by exporting percpu irq functions as well as adding the Krait l2 indirection register code. The next two patches add the driver and the binding and the final patch hooks it all up by adding the device tree node.

Quoted 2 lines

I'm not sure which tree this is supposed to go through. Ideally we could send the first 3 plus the 5th one through an edac tree.

Sure, I can take a look at the drivers/edac/ changes but I'd need an ack for the arch/arm/ stuff before/if I pick it up, i.e. patch 3.