[PATCH v5 0/4] Krait L1/L2 EDAC driver

[PATCH v5 0/4] Krait L1/L2 EDAC driver

[Stephen Boyd]

This patchset adds support for the Krait L1/L2 cache error detection
hardware. The first patch adds the Krait l2 indirection
register code. This patch is in need of an ACK from ARM folks.
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 think Boris will pick up the first and third patches since they depend
on each other. The final dts change could go through arm-soc via davidb's tree
and the Documentation patch could go through the devicetree tree. Or patches 1
through 3 can go through Boris' tree.

Changes since v4:
 * Prefixed l2 accessors functions with krait_
 * Dropped first two patches as Boris says he picked them up

Changes since v3:
 * Fixed l1_irq handler to properly dereference dev_id

Changes since v2:
 * Picked up acks
 * s/an/a/ in DT binding

Stephen Boyd (4):
  ARM: Add Krait L2 register accessor functions
  devicetree: bindings: Document Krait CPU/L1 EDAC
  edac: Add support for Krait CPU cache error detection
  ARM: dts: msm: Add Krait CPU/L2 nodes

 Documentation/devicetree/bindings/arm/cpus.txt |  52 ++++
 arch/arm/boot/dts/qcom-msm8974.dtsi            |  41 +++
 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 ++
 drivers/edac/Kconfig                           |   8 +
 drivers/edac/Makefile                          |   2 +
 drivers/edac/krait_edac.c                      | 346 +++++++++++++++++++++++++
 9 files changed, 531 insertions(+)
 create mode 100644 arch/arm/common/krait-l2-accessors.c
 create mode 100644 arch/arm/include/asm/krait-l2-accessors.h
 create mode 100644 drivers/edac/krait_edac.c

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/4] ARM: Add Krait L2 register accessor functions

[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 <mark.rutland@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 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

diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
index c3a4e9ceba34..9da52dc6260b 100644
--- a/arch/arm/common/Kconfig
+++ b/arch/arm/common/Kconfig
@@ -9,6 +9,9 @@ config DMABOUNCE
 	bool
 	select ZONE_DMA
 
+config KRAIT_L2_ACCESSORS
+	bool
+
 config SHARP_LOCOMO
 	bool
 
diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
index 4bdc41622c36..2836f992ee5d 100644
--- a/arch/arm/common/Makefile
+++ b/arch/arm/common/Makefile
@@ -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
diff --git a/arch/arm/common/krait-l2-accessors.c b/arch/arm/common/krait-l2-accessors.c
new file mode 100644
index 000000000000..5d514bbc88a6
--- /dev/null
+++ b/arch/arm/common/krait-l2-accessors.c
@@ -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 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);
diff --git a/arch/arm/include/asm/krait-l2-accessors.h b/arch/arm/include/asm/krait-l2-accessors.h
new file mode 100644
index 000000000000..48fe5527bc01
--- /dev/null
+++ b/arch/arm/include/asm/krait-l2-accessors.h
@@ -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,
+ * 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.
+ */
+
+#ifndef __ASMARM_KRAIT_L2_ACCESSORS_H
+#define __ASMARM_KRAIT_L2_ACCESSORS_H
+
+extern void krait_set_l2_indirect_reg(u32 addr, u32 val);
+extern u32 krait_get_l2_indirect_reg(u32 addr);
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

The Krait CPU/L1 error reporting device is made up a per-CPU
interrupt. While we're here, document the next-level-cache
property that's used by the Krait EDAC driver.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: <devicetree@vger.kernel.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 52 ++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 91304353eea4..c332b5168456 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -191,6 +191,16 @@ nodes to be present and contain the properties described below.
 			  property identifying a 64-bit zero-initialised
 			  memory location.
 
+	- interrupts
+		Usage: required for cpus with compatible string "qcom,krait".
+		Value type: <prop-encoded-array>
+		Definition: L1/CPU error interrupt
+
+	- next-level-cache
+		Usage: optional
+		Value type: <phandle>
+		Definition: phandle pointing to the next level cache
+
 Example 1 (dual-cluster big.LITTLE system 32-bit):
 
 	cpus {
@@ -382,3 +392,45 @@ cpus {
 		cpu-release-addr = <0 0x20000000>;
 	};
 };
+
+
+Example 5 (Krait 32-bit system):
+
+cpus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	interrupts = <1 9 0xf04>;
+
+	cpu@0 {
+		device_type = "cpu";
+		compatible = "qcom,krait";
+		reg = <0>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@1 {
+		device_type = "cpu";
+		compatible = "qcom,krait";
+		reg = <1>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@2 {
+		device_type = "cpu";
+		compatible = "qcom,krait";
+		reg = <2>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@3 {
+		device_type = "cpu";
+		compatible = "qcom,krait";
+		reg = <3>;
+		next-level-cache = <&L2>;
+	};
+
+	L2: l2-cache {
+		compatible = "cache";
+		interrupts = <0 2 0x4>;
+	};
+};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 3/4] edac: Add support for Krait CPU cache error detection

[Stephen Boyd]

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 <stepanm@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 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

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f09005fad..68f612d06eed 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -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 && ARCH_MSM
+	select KRAIT_L2_ACCESSORS
+	help
+	  Support for error detection and correction on the
+	  Krait L1/L2 cache controller.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 4154ed6a02c6..b6ea50564223 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -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
diff --git a/drivers/edac/krait_edac.c b/drivers/edac/krait_edac.c
new file mode 100644
index 000000000000..80a363e15b5b
--- /dev/null
+++ b/drivers/edac/krait_edac.c
@@ -0,0 +1,346 @@
+/* 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 <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
+
+struct krait_edac {
+	int l1_irq;
+	struct edac_device_ctl_info * __percpu *edev;
+	struct notifier_block notifier;
+};
+
+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);
+};
+
+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 },
+		{ "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) {
+		pr_alert("L1 / TLB Error detected on CPU %d!\n", cpu);
+		pr_alert("CESR      = 0x%08x\n", cesr);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(errors); i++)
+		if (BIT(i) & cesr)
+			errors[i].func(edac, cpu, 0, errors[i].msg);
+
+	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);
+		write_cesr(CESR_I_MASK);
+
+		/*
+		 * 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);
+	}
+
+	/* 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},
+		{ "master port LDREX received Normal OK response",
+			edac_device_handle_ce },
+	};
+
+	l2esr = krait_get_l2_indirect_reg(L2ESR);
+	pr_alert("Error detected!\n");
+	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);
+	}
+
+	cpu = (l2esr >> L2ESR_CPU_SHIFT) & L2ESR_CPU_MASK;
+	cpu = __ffs(cpu);
+	if (cpu)
+		cpu--;
+	for (i = 0; i < ARRAY_SIZE(errors); i++)
+		if (BIT(i) & l2esr)
+			errors[i].func(edac, cpu, 1, errors[i].msg);
+
+	krait_set_l2_indirect_reg(L2ESR, l2esr);
+
+	return IRQ_HANDLED;
+}
+
+static void enable_l1_irq(void *info)
+{
+	const struct krait_edac *k = info;
+
+	enable_percpu_irq(k->l1_irq, IRQ_TYPE_LEVEL_HIGH);
+}
+
+static void disable_l1_irq(void *info)
+{
+	const struct krait_edac *k = info;
+
+	disable_percpu_irq(k->l1_irq);
+}
+
+static int
+krait_edac_notify(struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+	struct krait_edac *p = container_of(nfb, struct krait_edac, notifier);
+
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_STARTING:
+		enable_l1_irq(p);
+		break;
+
+	case CPU_DYING:
+		disable_l1_irq(p);
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static int krait_edac_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct edac_device_ctl_info *edev;
+	struct krait_edac *p;
+	int l1_irq, l2_irq;
+	int ret, cpu;
+	struct device_node *node;
+
+	node = of_get_cpu_node(0, NULL);
+	if (!node)
+		return -ENODEV;
+
+	node = of_parse_phandle(node, "next-level-cache", 0);
+	if (!node)
+		return -ENODEV;
+
+	l2_irq = irq_of_parse_and_map(node, 0);
+	of_node_put(node);
+	if (l2_irq < 0)
+		return l2_irq;
+
+	l1_irq = platform_get_irq(pdev, 0);
+	if (l1_irq < 0)
+		return l1_irq;
+
+	p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, p);
+	p->l1_irq = l1_irq;
+
+	p->edev = alloc_percpu(struct edac_device_ctl_info *);
+	if (!p->edev)
+		return -ENOMEM;
+
+	edev = edac_device_alloc_ctl_info(0, "cpu", num_possible_cpus(),
+						 "L", 2, 1, NULL, 0,
+						 edac_device_alloc_index());
+	if (!edev) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
+
+	edev->dev = dev;
+	edev->mod_name = dev_name(dev);
+	edev->dev_name = dev_name(dev);
+	edev->ctl_name = "cache";
+
+	for_each_possible_cpu(cpu)
+		*per_cpu_ptr(p->edev, cpu) = edev;
+
+	ret = edac_device_add_device(edev);
+	if (ret)
+		goto err_add;
+
+	ret = request_percpu_irq(l1_irq, krait_l1_irq, "L1 err",
+				p->edev);
+	if (ret)
+		goto err_l1_irq;
+
+	ret = devm_request_irq(dev, l2_irq, krait_l2_irq, 0, "L2 err",
+				edev);
+	if (ret)
+		goto err_l2_irq;
+
+	p->notifier.notifier_call = krait_edac_notify;
+	register_hotcpu_notifier(&p->notifier);
+	on_each_cpu(enable_l1_irq, p, true);
+
+	return 0;
+err_l2_irq:
+	free_percpu_irq(p->l1_irq, p->edev);
+err_l1_irq:
+	edac_device_del_device(dev);
+err_add:
+	edac_device_free_ctl_info(edev);
+err_alloc:
+	free_percpu(p->edev);
+	return ret;
+}
+
+static int krait_edac_remove(struct platform_device *pdev)
+{
+	struct krait_edac *p = platform_get_drvdata(pdev);
+
+	unregister_hotcpu_notifier(&p->notifier);
+	on_each_cpu(disable_l1_irq, p, true);
+	free_percpu_irq(p->l1_irq, p->edev);
+	edac_device_del_device(&pdev->dev);
+	edac_device_free_ctl_info(*__this_cpu_ptr(p->edev));
+	free_percpu(p->edev);
+
+	return 0;
+}
+
+static const struct of_device_id krait_edac_match_table[] = {
+	{ .compatible = "qcom,krait" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, krait_edac_match_table);
+
+static struct platform_driver krait_edac_driver = {
+	.probe = krait_edac_probe,
+	.remove = krait_edac_remove,
+	.driver = {
+		.name = "krait_edac",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(krait_edac_match_table),
+	},
+};
+module_platform_driver(krait_edac_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Krait CPU cache error reporting driver");
+MODULE_ALIAS("platform:krait_edac");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 4/4] ARM: dts: msm: Add Krait CPU/L2 nodes

[Stephen Boyd]

This allows us to probe the krait-edac driver.

Cc: David Brown <davidb@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/boot/dts/qcom-msm8974.dtsi | 41 +++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index 9fa57d72f41e..7a494eaabd24 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -9,6 +9,47 @@
 	compatible = "qcom,msm8974";
 	interrupt-parent = <&intc>;
 
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interrupts = <1 9 0xf04>;
+		compatible = "qcom,krait";
+
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "qcom,krait";
+			reg = <0>;
+			next-level-cache = <&L2>;
+		};
+
+		cpu@1 {
+			device_type = "cpu";
+			compatible = "qcom,krait";
+			reg = <1>;
+			next-level-cache = <&L2>;
+		};
+
+		cpu@2 {
+			device_type = "cpu";
+			compatible = "qcom,krait";
+			reg = <2>;
+			next-level-cache = <&L2>;
+		};
+
+		cpu@3 {
+			device_type = "cpu";
+			compatible = "qcom,krait";
+			reg = <3>;
+			next-level-cache = <&L2>;
+		};
+
+		L2: l2-cache {
+			compatible = "cache";
+			cache-level = <2>;
+			interrupts = <0 2 0x4>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv7-timer";
 		interrupts = <1 2 0xf08>,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 0/4] Krait L1/L2 EDAC driver

[Borislav Petkov]

On Tue, Jan 14, 2014 at 01:30:30PM -0800, Stephen Boyd wrote:
> This patchset adds support for the Krait L1/L2 cache error detection
> hardware. The first patch adds the Krait l2 indirection
> register code. This patch is in need of an ACK from ARM folks.
> 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 think Boris will pick up the first and third patches since they depend
> on each other. The final dts change could go through arm-soc via davidb's tree
> and the Documentation patch could go through the devicetree tree. Or patches 1
> through 3 can go through Boris' tree.
> 
> Changes since v4:
>  * Prefixed l2 accessors functions with krait_
>  * Dropped first two patches as Boris says he picked them up

Hey Stephen,

I'm going to send upstream only

[PATCH v4 1/6] edac: Don't try to cancel workqueue when it's never setup

as it is a clear bugfix.

The second one

[PATCH v4 2/6] genirq: export percpu irq functions for module usage

will have to wait for when the driver goes in. But the driver depends on
"ARM: Add Krait L2 register accessor functions" which still waits for
ACKs, right?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v5 0/4] Krait L1/L2 EDAC driver

[Stephen Boyd]

On 01/14/14 13:48, Borislav Petkov wrote:
> On Tue, Jan 14, 2014 at 01:30:30PM -0800, Stephen Boyd wrote:
>> This patchset adds support for the Krait L1/L2 cache error detection
>> hardware. The first patch adds the Krait l2 indirection
>> register code. This patch is in need of an ACK from ARM folks.
>> 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 think Boris will pick up the first and third patches since they depend
>> on each other. The final dts change could go through arm-soc via davidb's tree
>> and the Documentation patch could go through the devicetree tree. Or patches 1
>> through 3 can go through Boris' tree.
>>
>> Changes since v4:
>>  * Prefixed l2 accessors functions with krait_
>>  * Dropped first two patches as Boris says he picked them up
> Hey Stephen,
>
> I'm going to send upstream only
>
> [PATCH v4 1/6] edac: Don't try to cancel workqueue when it's never setup
>
> as it is a clear bugfix.
>
> The second one
>
> [PATCH v4 2/6] genirq: export percpu irq functions for module usage
>
> will have to wait for when the driver goes in. But the driver depends on
> "ARM: Add Krait L2 register accessor functions" which still waits for
> ACKs, right?
>

Ok that sounds fine. The ARM patch is fairly minor so I'm not sure if
anyone is willing to ack it, but I'm willing to wait a little while to
see if anyone will.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

On Tue, Jan 14, 2014 at 09:30:32PM +0000, Stephen Boyd wrote:
> The Krait CPU/L1 error reporting device is made up a per-CPU
> interrupt. While we're here, document the next-level-cache
> property that's used by the Krait EDAC driver.
> 
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 52 ++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 91304353eea4..c332b5168456 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -191,6 +191,16 @@ nodes to be present and contain the properties described below.
>  			  property identifying a 64-bit zero-initialised
>  			  memory location.
>  
> +	- interrupts
> +		Usage: required for cpus with compatible string "qcom,krait".
> +		Value type: <prop-encoded-array>
> +		Definition: L1/CPU error interrupt

I reckon you want this property to belong in the cpus node (example below),
not in cpu nodes, right ?

Are you relying on a platform device to be created for /cpus node in
order for this series to work ? I guess that's why you want the
interrupts property to be defined in /cpus so that it becomes a platform
device resource (and you also add a compatible property in /cpus that is
missing in these bindings).

> +
> +	- next-level-cache
> +		Usage: optional
> +		Value type: <phandle>
> +		Definition: phandle pointing to the next level cache
> +
>  Example 1 (dual-cluster big.LITTLE system 32-bit):
>  
>  	cpus {
> @@ -382,3 +392,45 @@ cpus {
>  		cpu-release-addr = <0 0x20000000>;
>  	};
>  };
> +
> +
> +Example 5 (Krait 32-bit system):
> +
> +cpus {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	interrupts = <1 9 0xf04>;

In patch 4 you also add a compatible property here, and that's not documented,
and honestly I do not think that's acceptable either. I guess you want a
compatible property here to match the platform driver, right ?

Thank you,
Lorenzo

> +
> +	cpu@0 {
> +		device_type = "cpu";
> +		compatible = "qcom,krait";
> +		reg = <0>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@1 {
> +		device_type = "cpu";
> +		compatible = "qcom,krait";
> +		reg = <1>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@2 {
> +		device_type = "cpu";
> +		compatible = "qcom,krait";
> +		reg = <2>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@3 {
> +		device_type = "cpu";
> +		compatible = "qcom,krait";
> +		reg = <3>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	L2: l2-cache {
> +		compatible = "cache";
> +		interrupts = <0 2 0x4>;
> +	};
> +};
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

On 01/15, Lorenzo Pieralisi wrote:
> On Tue, Jan 14, 2014 at 09:30:32PM +0000, Stephen Boyd wrote:
> > The Krait CPU/L1 error reporting device is made up a per-CPU
> > interrupt. While we're here, document the next-level-cache
> > property that's used by the Krait EDAC driver.
> > 
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Kumar Gala <galak@codeaurora.org>
> > Cc: <devicetree@vger.kernel.org>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > ---
> >  Documentation/devicetree/bindings/arm/cpus.txt | 52 ++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> > index 91304353eea4..c332b5168456 100644
> > --- a/Documentation/devicetree/bindings/arm/cpus.txt
> > +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> > @@ -191,6 +191,16 @@ nodes to be present and contain the properties described below.
> >  			  property identifying a 64-bit zero-initialised
> >  			  memory location.
> >  
> > +	- interrupts
> > +		Usage: required for cpus with compatible string "qcom,krait".
> > +		Value type: <prop-encoded-array>
> > +		Definition: L1/CPU error interrupt
> 
> I reckon you want this property to belong in the cpus node (example below),
> not in cpu nodes, right ?

Yes.

> 
> Are you relying on a platform device to be created for /cpus node in
> order for this series to work ? I guess that's why you want the
> interrupts property to be defined in /cpus so that it becomes a platform
> device resource (and you also add a compatible property in /cpus that is
> missing in these bindings).

Ah yes. I'll move this to the /cpus section.

> 
> > +
> > +	- next-level-cache
> > +		Usage: optional
> > +		Value type: <phandle>
> > +		Definition: phandle pointing to the next level cache
> > +
> >  Example 1 (dual-cluster big.LITTLE system 32-bit):
> >  
> >  	cpus {
> > @@ -382,3 +392,45 @@ cpus {
> >  		cpu-release-addr = <0 0x20000000>;
> >  	};
> >  };
> > +
> > +
> > +Example 5 (Krait 32-bit system):
> > +
> > +cpus {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	interrupts = <1 9 0xf04>;
> 
> In patch 4 you also add a compatible property here, and that's not documented,
> and honestly I do not think that's acceptable either. I guess you want a
> compatible property here to match the platform driver, right ?

Ah sorry, I forgot to put the compatible property here like in
the dts change. I'll do that in the next revision. Yes we need a
compatible property here to match the platform driver.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

On 01/15, Stephen Boyd wrote:
> 
> Ah sorry, I forgot to put the compatible property here like in
> the dts change. I'll do that in the next revision. Yes we need a
> compatible property here to match the platform driver.
> 

This is the replacement patch

-----8<------
From: Stephen Boyd <sboyd@codeaurora.org>
Subject: [PATCH v9] devicetree: bindings: Document Krait CPU/L1 EDAC

The Krait CPU/L1 error reporting device is made up a per-CPU
interrupt. While we're here, document the next-level-cache
property that's used by the Krait EDAC driver.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: <devicetree@vger.kernel.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 58 ++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 91304353eea4..03a529e791c4 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -62,6 +62,20 @@ nodes to be present and contain the properties described below.
 		Value type: <u32>
 		Definition: must be set to 0
 
+	- compatible
+		Usage: optional
+		Value type: <string>
+		Definition: should be one of the compatible strings listed
+			    in the cpu node compatible property. This property
+			    shall only be present if all the cpu nodes have the
+			    same compatible property.
+
+	- interrupts
+		Usage: required when node contains cpus with compatible
+		       string "qcom,krait".
+		Value type: <prop-encoded-array>
+		Definition: L1/CPU error interrupt
+
 - cpu node
 
 	Description: Describes a CPU in an ARM based system
@@ -191,6 +205,11 @@ nodes to be present and contain the properties described below.
 			  property identifying a 64-bit zero-initialised
 			  memory location.
 
+	- next-level-cache
+		Usage: optional
+		Value type: <phandle>
+		Definition: phandle pointing to the next level cache
+
 Example 1 (dual-cluster big.LITTLE system 32-bit):
 
 	cpus {
@@ -382,3 +401,42 @@ cpus {
 		cpu-release-addr = <0 0x20000000>;
 	};
 };
+
+
+Example 5 (Krait 32-bit system):
+
+cpus {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	interrupts = <1 9 0xf04>;
+	compatible = "qcom,krait";
+
+	cpu@0 {
+		device_type = "cpu";
+		reg = <0>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@1 {
+		device_type = "cpu";
+		reg = <1>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@2 {
+		device_type = "cpu";
+		reg = <2>;
+		next-level-cache = <&L2>;
+	};
+
+	cpu@3 {
+		device_type = "cpu";
+		reg = <3>;
+		next-level-cache = <&L2>;
+	};
+
+	L2: l2-cache {
+		compatible = "cache";
+		interrupts = <0 2 0x4>;
+	};
+};
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

On Thu, Jan 16, 2014 at 01:38:40AM +0000, Stephen Boyd wrote:
> On 01/15, Stephen Boyd wrote:
> > 
> > Ah sorry, I forgot to put the compatible property here like in
> > the dts change. I'll do that in the next revision. Yes we need a
> > compatible property here to match the platform driver.
> > 
> 
> This is the replacement patch
> 
> -----8<------
> From: Stephen Boyd <sboyd@codeaurora.org>
> Subject: [PATCH v9] devicetree: bindings: Document Krait CPU/L1 EDAC
> 
> The Krait CPU/L1 error reporting device is made up a per-CPU
> interrupt. While we're here, document the next-level-cache
> property that's used by the Krait EDAC driver.
> 
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 58 ++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 91304353eea4..03a529e791c4 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -62,6 +62,20 @@ nodes to be present and contain the properties described below.
>  		Value type: <u32>
>  		Definition: must be set to 0
>  
> +	- compatible
> +		Usage: optional
> +		Value type: <string>
> +		Definition: should be one of the compatible strings listed
> +			    in the cpu node compatible property. This property
> +			    shall only be present if all the cpu nodes have the
> +			    same compatible property.

Do we really want to do that ? I am not sure. A cpus node is supposed to
be a container node, we should not define this binding just because we
know the kernel creates a platform device for it then.

interrupts is a cpu node property and I think it should be kept as such.

I know it will be duplicated and I know you can't rely on a platform
device for probing (since if I am not mistaken, removing a compatible
string from cpus prevents its platform device creation), but that's an issue
related to how the kernel works, you should not define DT bindings to solve
that IMHO.

Lorenzo

> +
> +	- interrupts
> +		Usage: required when node contains cpus with compatible
> +		       string "qcom,krait".
> +		Value type: <prop-encoded-array>
> +		Definition: L1/CPU error interrupt
> +
>  - cpu node
>  
>  	Description: Describes a CPU in an ARM based system
> @@ -191,6 +205,11 @@ nodes to be present and contain the properties described below.
>  			  property identifying a 64-bit zero-initialised
>  			  memory location.
>  
> +	- next-level-cache
> +		Usage: optional
> +		Value type: <phandle>
> +		Definition: phandle pointing to the next level cache
> +
>  Example 1 (dual-cluster big.LITTLE system 32-bit):
>  
>  	cpus {
> @@ -382,3 +401,42 @@ cpus {
>  		cpu-release-addr = <0 0x20000000>;
>  	};
>  };
> +
> +
> +Example 5 (Krait 32-bit system):
> +
> +cpus {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	interrupts = <1 9 0xf04>;
> +	compatible = "qcom,krait";
> +
> +	cpu@0 {
> +		device_type = "cpu";
> +		reg = <0>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@1 {
> +		device_type = "cpu";
> +		reg = <1>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@2 {
> +		device_type = "cpu";
> +		reg = <2>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	cpu@3 {
> +		device_type = "cpu";
> +		reg = <3>;
> +		next-level-cache = <&L2>;
> +	};
> +
> +	L2: l2-cache {
> +		compatible = "cache";
> +		interrupts = <0 2 0x4>;
> +	};
> +};
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

On 01/16, Lorenzo Pieralisi wrote:
> On Thu, Jan 16, 2014 at 01:38:40AM +0000, Stephen Boyd wrote:
> > On 01/15, Stephen Boyd wrote:
> > > 
> > > Ah sorry, I forgot to put the compatible property here like in
> > > the dts change. I'll do that in the next revision. Yes we need a
> > > compatible property here to match the platform driver.
> > > 
> > 
> > This is the replacement patch
> > 
> > -----8<------
> > From: Stephen Boyd <sboyd@codeaurora.org>
> > Subject: [PATCH v9] devicetree: bindings: Document Krait CPU/L1 EDAC
> > 
> > The Krait CPU/L1 error reporting device is made up a per-CPU
> > interrupt. While we're here, document the next-level-cache
> > property that's used by the Krait EDAC driver.
> > 
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Kumar Gala <galak@codeaurora.org>
> > Cc: <devicetree@vger.kernel.org>
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > ---
> >  Documentation/devicetree/bindings/arm/cpus.txt | 58 ++++++++++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> > index 91304353eea4..03a529e791c4 100644
> > --- a/Documentation/devicetree/bindings/arm/cpus.txt
> > +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> > @@ -62,6 +62,20 @@ nodes to be present and contain the properties described below.
> >  		Value type: <u32>
> >  		Definition: must be set to 0
> >  
> > +	- compatible
> > +		Usage: optional
> > +		Value type: <string>
> > +		Definition: should be one of the compatible strings listed
> > +			    in the cpu node compatible property. This property
> > +			    shall only be present if all the cpu nodes have the
> > +			    same compatible property.
> 
> Do we really want to do that ? I am not sure. A cpus node is supposed to
> be a container node, we should not define this binding just because we
> know the kernel creates a platform device for it then.

This is just copying more of the ePAPR spec into this document.
It just so happens that having a compatible field here allows a
platform device to be created. I don't see why that's a problem.

> 
> interrupts is a cpu node property and I think it should be kept as such.
> 
> I know it will be duplicated and I know you can't rely on a platform
> device for probing (since if I am not mistaken, removing a compatible
> string from cpus prevents its platform device creation), but that's an issue
> related to how the kernel works, you should not define DT bindings to solve
> that IMHO.

The interrupts property is also common for all cpus so it seems
fine to collapse the value down into a PPI specifier indicating
that all CPUs get the interrupt, similar to how we compress the
information about the compatible string.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote:
> On 01/16, Lorenzo Pieralisi wrote:
> > On Thu, Jan 16, 2014 at 01:38:40AM +0000, Stephen Boyd wrote:
> > > On 01/15, Stephen Boyd wrote:
> > > > 
> > > > Ah sorry, I forgot to put the compatible property here like in
> > > > the dts change. I'll do that in the next revision. Yes we need a
> > > > compatible property here to match the platform driver.
> > > > 
> > > 
> > > This is the replacement patch
> > > 
> > > -----8<------
> > > From: Stephen Boyd <sboyd@codeaurora.org>
> > > Subject: [PATCH v9] devicetree: bindings: Document Krait CPU/L1 EDAC
> > > 
> > > The Krait CPU/L1 error reporting device is made up a per-CPU
> > > interrupt. While we're here, document the next-level-cache
> > > property that's used by the Krait EDAC driver.
> > > 
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Kumar Gala <galak@codeaurora.org>
> > > Cc: <devicetree@vger.kernel.org>
> > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > > ---
> > >  Documentation/devicetree/bindings/arm/cpus.txt | 58 ++++++++++++++++++++++++++
> > >  1 file changed, 58 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> > > index 91304353eea4..03a529e791c4 100644
> > > --- a/Documentation/devicetree/bindings/arm/cpus.txt
> > > +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> > > @@ -62,6 +62,20 @@ nodes to be present and contain the properties described below.
> > >  		Value type: <u32>
> > >  		Definition: must be set to 0
> > >  
> > > +	- compatible
> > > +		Usage: optional
> > > +		Value type: <string>
> > > +		Definition: should be one of the compatible strings listed
> > > +			    in the cpu node compatible property. This property
> > > +			    shall only be present if all the cpu nodes have the
> > > +			    same compatible property.
> > 
> > Do we really want to do that ? I am not sure. A cpus node is supposed to
> > be a container node, we should not define this binding just because we
> > know the kernel creates a platform device for it then.
> 
> This is just copying more of the ePAPR spec into this document.
> It just so happens that having a compatible field here allows a
> platform device to be created. I don't see why that's a problem.

I do not see why you cannot define a node like pmu or arch-timer and stick
a compatible property in there. cpus node does not represent a device, and
must not be created as a platform device, that's my opinion.

What would you do for big.LITTLE systems ? We are going to create two
cpus node because we need two platform devices ? I really think there
must be a better way to implement this, but I will let DT maintainers
make a decision.

> > interrupts is a cpu node property and I think it should be kept as such.
> > 
> > I know it will be duplicated and I know you can't rely on a platform
> > device for probing (since if I am not mistaken, removing a compatible
> > string from cpus prevents its platform device creation), but that's an issue
> > related to how the kernel works, you should not define DT bindings to solve
> > that IMHO.
> 
> The interrupts property is also common for all cpus so it seems
> fine to collapse the value down into a PPI specifier indicating
> that all CPUs get the interrupt, similar to how we compress the
> information about the compatible string.

I think it is nicer to create a device node (as I said, like a pmu or an
arch-timer) and define interrupts there along with a proper compatible
property. This would serve the same purpose without adding properties in
the cpus node.

cpu-edac {
	compatible = "qcom,cpu-edac";
	interrupts = <...>;
};

Thanks,
Lorenzo

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

On 01/16, Lorenzo Pieralisi wrote:
> On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote:
> > On 01/16, Lorenzo Pieralisi wrote:
> > > Do we really want to do that ? I am not sure. A cpus node is supposed to
> > > be a container node, we should not define this binding just because we
> > > know the kernel creates a platform device for it then.
> > 
> > This is just copying more of the ePAPR spec into this document.
> > It just so happens that having a compatible field here allows a
> > platform device to be created. I don't see why that's a problem.
> 
> I do not see why you cannot define a node like pmu or arch-timer and stick
> a compatible property in there. cpus node does not represent a device, and
> must not be created as a platform device, that's my opinion.
> 

I had what you're suggesting before in the original revision of
this patch. Please take a look at the original patch series[1]. I
suppose it could be tweaked slightly to still have a cache node
for the L2 interrupt and the next-level-cache pointer from the
CPUs.

> What would you do for big.LITTLE systems ? We are going to create two
> cpus node because we need two platform devices ? I really think there
> must be a better way to implement this, but I will let DT maintainers
> make a decision.

There is no such thing as big.LITTLE for Krait, so this is not a
concern.

> 
> > > interrupts is a cpu node property and I think it should be kept as such.
> > > 
> > > I know it will be duplicated and I know you can't rely on a platform
> > > device for probing (since if I am not mistaken, removing a compatible
> > > string from cpus prevents its platform device creation), but that's an issue
> > > related to how the kernel works, you should not define DT bindings to solve
> > > that IMHO.
> > 
> > The interrupts property is also common for all cpus so it seems
> > fine to collapse the value down into a PPI specifier indicating
> > that all CPUs get the interrupt, similar to how we compress the
> > information about the compatible string.
> 
> I think it is nicer to create a device node (as I said, like a pmu or an
> arch-timer) and define interrupts there along with a proper compatible
> property. This would serve the same purpose without adding properties in
> the cpus node.
> 
> cpu-edac {
> 	compatible = "qcom,cpu-edac";
> 	interrupts = <...>;
> };

Yes, please see the original thread.

[1] https://lkml.org/lkml/2013/10/29/134

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote:
> On 01/16, Lorenzo Pieralisi wrote:
> > On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote:
> > > On 01/16, Lorenzo Pieralisi wrote:
> > > > Do we really want to do that ? I am not sure. A cpus node is supposed to
> > > > be a container node, we should not define this binding just because we
> > > > know the kernel creates a platform device for it then.
> > > 
> > > This is just copying more of the ePAPR spec into this document.
> > > It just so happens that having a compatible field here allows a
> > > platform device to be created. I don't see why that's a problem.
> > 
> > I do not see why you cannot define a node like pmu or arch-timer and stick
> > a compatible property in there. cpus node does not represent a device, and
> > must not be created as a platform device, that's my opinion.
> > 
> 
> I had what you're suggesting before in the original revision of
> this patch. Please take a look at the original patch series[1]. I
> suppose it could be tweaked slightly to still have a cache node
> for the L2 interrupt and the next-level-cache pointer from the
> CPUs.

Ok, sorry, we are running around in circles here, basically you moved
the node to cpus according to reviews. I still think that treating cpus
as a device is not a great idea, even though I am in the same
position with C-states and probably will add C-state tables in the cpus
node.

http://comments.gmane.org/gmane.linux.power-management.general/41012

I just would like to see under cpus nodes and properties that apply to
all ARM systems, and avoid defining properties (eg interrupts) that
have different meanings for different ARM cores.

The question related to why the kernel should create a platform device
out of cpus is still open. I really do not want to block your series
for these simple issues but we have to make a decision and stick to that,
I am fine either way if we have a plan.

> > What would you do for big.LITTLE systems ? We are going to create two
> > cpus node because we need two platform devices ? I really think there
> > must be a better way to implement this, but I will let DT maintainers
> > make a decision.
> 
> There is no such thing as big.LITTLE for Krait, so this is not a
> concern.

Yes, but if a core supporting big.LITTLE requires the same concept you
need here we have to cater for that because after all cpus is a node that
exists on ALL ARM systems, we should not rush and find a solution du
jour without thinking about all foreseeable use cases.

Thank you,
Lorenzo

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

(Sorry, this discussion stalled due to merge window + life events)

On 01/17, Lorenzo Pieralisi wrote:
> On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote:
> > On 01/16, Lorenzo Pieralisi wrote:
> > > On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote:
> > > > On 01/16, Lorenzo Pieralisi wrote:
> > > > > Do we really want to do that ? I am not sure. A cpus node is supposed to
> > > > > be a container node, we should not define this binding just because we
> > > > > know the kernel creates a platform device for it then.
> > > > 
> > > > This is just copying more of the ePAPR spec into this document.
> > > > It just so happens that having a compatible field here allows a
> > > > platform device to be created. I don't see why that's a problem.
> > > 
> > > I do not see why you cannot define a node like pmu or arch-timer and stick
> > > a compatible property in there. cpus node does not represent a device, and
> > > must not be created as a platform device, that's my opinion.
> > > 
> > 
> > I had what you're suggesting before in the original revision of
> > this patch. Please take a look at the original patch series[1]. I
> > suppose it could be tweaked slightly to still have a cache node
> > for the L2 interrupt and the next-level-cache pointer from the
> > CPUs.
> 
> Ok, sorry, we are running around in circles here, basically you moved
> the node to cpus according to reviews. I still think that treating cpus
> as a device is not a great idea, even though I am in the same
> position with C-states and probably will add C-state tables in the cpus
> node.
> 
> http://comments.gmane.org/gmane.linux.power-management.general/41012
> 
> I just would like to see under cpus nodes and properties that apply to
> all ARM systems, and avoid defining properties (eg interrupts) that
> have different meanings for different ARM cores.
> 
> The question related to why the kernel should create a platform device
> out of cpus is still open. I really do not want to block your series
> for these simple issues but we have to make a decision and stick to that,
> I am fine either way if we have a plan.
> 

Do you just want a backup plan in case we don't make a platform
device out of the cpus node? I believe we can always add code
somewhere to create a platform device at runtime if we detect the
cpus node has a compatible string equal to "qcom,krait". We could
probably change this driver's module_init() to scan the DT for
such a compatible string and create the platform device right
there. If we get more than one interrupt in the cpus node we can
add interrupt-names and then have software look for interrupts by
name instead of number.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

Hi Stephen,

On Wed, Feb 19, 2014 at 12:20:43AM +0000, Stephen Boyd wrote:
> (Sorry, this discussion stalled due to merge window + life events)

Sorry for the delay in replying on my side too.

> On 01/17, Lorenzo Pieralisi wrote:
> > On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote:
> > > On 01/16, Lorenzo Pieralisi wrote:
> > > > On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote:
> > > > > On 01/16, Lorenzo Pieralisi wrote:
> > > > > > Do we really want to do that ? I am not sure. A cpus node is supposed to
> > > > > > be a container node, we should not define this binding just because we
> > > > > > know the kernel creates a platform device for it then.
> > > > > 
> > > > > This is just copying more of the ePAPR spec into this document.
> > > > > It just so happens that having a compatible field here allows a
> > > > > platform device to be created. I don't see why that's a problem.
> > > > 
> > > > I do not see why you cannot define a node like pmu or arch-timer and stick
> > > > a compatible property in there. cpus node does not represent a device, and
> > > > must not be created as a platform device, that's my opinion.
> > > > 
> > > 
> > > I had what you're suggesting before in the original revision of
> > > this patch. Please take a look at the original patch series[1]. I
> > > suppose it could be tweaked slightly to still have a cache node
> > > for the L2 interrupt and the next-level-cache pointer from the
> > > CPUs.
> > 
> > Ok, sorry, we are running around in circles here, basically you moved
> > the node to cpus according to reviews. I still think that treating cpus
> > as a device is not a great idea, even though I am in the same
> > position with C-states and probably will add C-state tables in the cpus
> > node.
> > 
> > http://comments.gmane.org/gmane.linux.power-management.general/41012
> > 
> > I just would like to see under cpus nodes and properties that apply to
> > all ARM systems, and avoid defining properties (eg interrupts) that
> > have different meanings for different ARM cores.
> > 
> > The question related to why the kernel should create a platform device
> > out of cpus is still open. I really do not want to block your series
> > for these simple issues but we have to make a decision and stick to that,
> > I am fine either way if we have a plan.
> > 
> 
> Do you just want a backup plan in case we don't make a platform
> device out of the cpus node? I believe we can always add code
> somewhere to create a platform device at runtime if we detect the
> cpus node has a compatible string equal to "qcom,krait". We could
> probably change this driver's module_init() to scan the DT for
> such a compatible string and create the platform device right
> there. If we get more than one interrupt in the cpus node we can
> add interrupt-names and then have software look for interrupts by
> name instead of number.

As I mentioned, I do not like the idea of adding compatible properties
just to force the kernel to create platform devices out of device tree
nodes. On top of that I would avoid adding a compatible property
to the cpus node (after all properties like enable-method are common for all
cpus but still duplicated), my only concern being backward compatibility
here (ie if we do that for interrupts, we should do that also for other
common cpu nodes properties, otherwise we have different rules for
different properties).

I think you can then add interrupts to cpu nodes ("qcom,krait" specific),
and as you mentioned create a platform device for that.

Thanks,
Lorenzo

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Kumar Gala]

On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> Hi Stephen,
> 
> On Wed, Feb 19, 2014 at 12:20:43AM +0000, Stephen Boyd wrote:
>> (Sorry, this discussion stalled due to merge window + life events)
> 
> Sorry for the delay in replying on my side too.
> 
>> On 01/17, Lorenzo Pieralisi wrote:
>>> On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote:
>>>> On 01/16, Lorenzo Pieralisi wrote:
>>>>> On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote:
>>>>>> On 01/16, Lorenzo Pieralisi wrote:
>>>>>>> Do we really want to do that ? I am not sure. A cpus node is supposed to
>>>>>>> be a container node, we should not define this binding just because we
>>>>>>> know the kernel creates a platform device for it then.
>>>>>> 
>>>>>> This is just copying more of the ePAPR spec into this document.
>>>>>> It just so happens that having a compatible field here allows a
>>>>>> platform device to be created. I don't see why that's a problem.
>>>>> 
>>>>> I do not see why you cannot define a node like pmu or arch-timer and stick
>>>>> a compatible property in there. cpus node does not represent a device, and
>>>>> must not be created as a platform device, that's my opinion.
>>>>> 
>>>> 
>>>> I had what you're suggesting before in the original revision of
>>>> this patch. Please take a look at the original patch series[1]. I
>>>> suppose it could be tweaked slightly to still have a cache node
>>>> for the L2 interrupt and the next-level-cache pointer from the
>>>> CPUs.
>>> 
>>> Ok, sorry, we are running around in circles here, basically you moved
>>> the node to cpus according to reviews. I still think that treating cpus
>>> as a device is not a great idea, even though I am in the same
>>> position with C-states and probably will add C-state tables in the cpus
>>> node.
>>> 
>>> http://comments.gmane.org/gmane.linux.power-management.general/41012
>>> 
>>> I just would like to see under cpus nodes and properties that apply to
>>> all ARM systems, and avoid defining properties (eg interrupts) that
>>> have different meanings for different ARM cores.
>>> 
>>> The question related to why the kernel should create a platform device
>>> out of cpus is still open. I really do not want to block your series
>>> for these simple issues but we have to make a decision and stick to that,
>>> I am fine either way if we have a plan.
>>> 
>> 
>> Do you just want a backup plan in case we don't make a platform
>> device out of the cpus node? I believe we can always add code
>> somewhere to create a platform device at runtime if we detect the
>> cpus node has a compatible string equal to "qcom,krait". We could
>> probably change this driver's module_init() to scan the DT for
>> such a compatible string and create the platform device right
>> there. If we get more than one interrupt in the cpus node we can
>> add interrupt-names and then have software look for interrupts by
>> name instead of number.
> 
> As I mentioned, I do not like the idea of adding compatible properties
> just to force the kernel to create platform devices out of device tree
> nodes. On top of that I would avoid adding a compatible property
> to the cpus node (after all properties like enable-method are common for all
> cpus but still duplicated), my only concern being backward compatibility
> here (ie if we do that for interrupts, we should do that also for other
> common cpu nodes properties, otherwise we have different rules for
> different properties).
> 
> I think you can then add interrupts to cpu nodes ("qcom,krait" specific),
> and as you mentioned create a platform device for that.
> 
> Thanks,
> Lorenzo

So I agree with the statement about adding compatibles just to create platform devices is wrong.  However its seems perfectly reasonable for a cpu node to have a compatible property.  I don’t see why a CPU is any different from any other device described in a DT.

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

On Tue, Feb 25, 2014 at 08:48:38PM +0000, Kumar Gala wrote:
> 
> On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> > Hi Stephen,
> > 
> > On Wed, Feb 19, 2014 at 12:20:43AM +0000, Stephen Boyd wrote:
> >> (Sorry, this discussion stalled due to merge window + life events)
> > 
> > Sorry for the delay in replying on my side too.
> > 
> >> On 01/17, Lorenzo Pieralisi wrote:
> >>> On Thu, Jan 16, 2014 at 07:26:17PM +0000, Stephen Boyd wrote:
> >>>> On 01/16, Lorenzo Pieralisi wrote:
> >>>>> On Thu, Jan 16, 2014 at 06:05:05PM +0000, Stephen Boyd wrote:
> >>>>>> On 01/16, Lorenzo Pieralisi wrote:
> >>>>>>> Do we really want to do that ? I am not sure. A cpus node is supposed to
> >>>>>>> be a container node, we should not define this binding just because we
> >>>>>>> know the kernel creates a platform device for it then.
> >>>>>> 
> >>>>>> This is just copying more of the ePAPR spec into this document.
> >>>>>> It just so happens that having a compatible field here allows a
> >>>>>> platform device to be created. I don't see why that's a problem.
> >>>>> 
> >>>>> I do not see why you cannot define a node like pmu or arch-timer and stick
> >>>>> a compatible property in there. cpus node does not represent a device, and
> >>>>> must not be created as a platform device, that's my opinion.
> >>>>> 
> >>>> 
> >>>> I had what you're suggesting before in the original revision of
> >>>> this patch. Please take a look at the original patch series[1]. I
> >>>> suppose it could be tweaked slightly to still have a cache node
> >>>> for the L2 interrupt and the next-level-cache pointer from the
> >>>> CPUs.
> >>> 
> >>> Ok, sorry, we are running around in circles here, basically you moved
> >>> the node to cpus according to reviews. I still think that treating cpus
> >>> as a device is not a great idea, even though I am in the same
> >>> position with C-states and probably will add C-state tables in the cpus
> >>> node.
> >>> 
> >>> http://comments.gmane.org/gmane.linux.power-management.general/41012
> >>> 
> >>> I just would like to see under cpus nodes and properties that apply to
> >>> all ARM systems, and avoid defining properties (eg interrupts) that
> >>> have different meanings for different ARM cores.
> >>> 
> >>> The question related to why the kernel should create a platform device
> >>> out of cpus is still open. I really do not want to block your series
> >>> for these simple issues but we have to make a decision and stick to that,
> >>> I am fine either way if we have a plan.
> >>> 
> >> 
> >> Do you just want a backup plan in case we don't make a platform
> >> device out of the cpus node? I believe we can always add code
> >> somewhere to create a platform device at runtime if we detect the
> >> cpus node has a compatible string equal to "qcom,krait". We could
> >> probably change this driver's module_init() to scan the DT for
> >> such a compatible string and create the platform device right
> >> there. If we get more than one interrupt in the cpus node we can
> >> add interrupt-names and then have software look for interrupts by
> >> name instead of number.
> > 
> > As I mentioned, I do not like the idea of adding compatible properties
> > just to force the kernel to create platform devices out of device tree
> > nodes. On top of that I would avoid adding a compatible property
> > to the cpus node (after all properties like enable-method are common for all
> > cpus but still duplicated), my only concern being backward compatibility
> > here (ie if we do that for interrupts, we should do that also for other
> > common cpu nodes properties, otherwise we have different rules for
> > different properties).
> > 
> > I think you can then add interrupts to cpu nodes ("qcom,krait" specific),
> > and as you mentioned create a platform device for that.
> > 
> > Thanks,
> > Lorenzo
> 
> So I agree with the statement about adding compatibles just to create platform devices is wrong.  However its seems perfectly reasonable for a cpu node to have a compatible property.  I don't see why a CPU is any different from any other device described in a DT.

I was referring to the /cpus node, not to individual cpu nodes, where
the compatible property is already present now.

Lorenzo

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

On 02/26, Lorenzo Pieralisi wrote:
> On Tue, Feb 25, 2014 at 08:48:38PM +0000, Kumar Gala wrote:
> > 
> > On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > > 
> > > As I mentioned, I do not like the idea of adding compatible properties
> > > just to force the kernel to create platform devices out of device tree
> > > nodes. On top of that I would avoid adding a compatible property
> > > to the cpus node (after all properties like enable-method are common for all
> > > cpus but still duplicated), my only concern being backward compatibility
> > > here (ie if we do that for interrupts, we should do that also for other
> > > common cpu nodes properties, otherwise we have different rules for
> > > different properties).
> > > 
> > > I think you can then add interrupts to cpu nodes ("qcom,krait" specific),
> > > and as you mentioned create a platform device for that.
> > > 
> > > Thanks,
> > > Lorenzo
> > 
> > So I agree with the statement about adding compatibles just to create platform devices is wrong.  However its seems perfectly reasonable for a cpu node to have a compatible property.  I don't see why a CPU is any different from any other device described in a DT.
> 
> I was referring to the /cpus node, not to individual cpu nodes, where
> the compatible property is already present now.
> 

Ok I think I'll go ahead with moving the interrupts into each cpu node, i.e.:

        cpus {  
                #address-cells = <1>;
                #size-cells = <0>;

                cpu@0 { 
                        compatible = "qcom,krait";
                        device_type = "cpu";
                        reg = <0>;
                        interrupts = <1 14 0x304>;
                        next-level-cache = <&L2>;
                };

                cpu@1 { 
                        compatible = "qcom,krait";
                        device_type = "cpu";
                        reg = <1>;
                        interrupts = <1 14 0x304>;
                        next-level-cache = <&L2>;
                };

                L2: l2-cache {
                        compatible = "cache";
                        interrupts = <0 2 0x4>;
		};
	};

Or should we be expressing the L1 cache as well? Something like:

        cpus {  
                #address-cells = <1>;
                #size-cells = <0>;

                cpu@0 { 
                        compatible = "qcom,krait";
                        device_type = "cpu";
                        reg = <0>;
                        next-level-cache = <&L1_0>;

			L1_0: l1-cache {
				compatible = "arm,arch-cache";
				interrupts = <1 14 0x304>;
				next-level-cache = <&L2>;
			}
                };

                cpu@1 { 
                        compatible = "qcom,krait";
                        device_type = "cpu";
                        reg = <1>;
                        next-level-cache = <&L1_1>;

			L1_1: l1-cache {
				compatible = "arm,arch-cache";
				interrupts = <1 14 0x304>;
				next-level-cache = <&L2>;
			}
                };

                L2: l2-cache {
                        compatible = "arm,arch-cache";
                        interrupts = <0 2 0x4>;
		};
	};

(I'm also wondering if the 3rd cell of the interrupt binding
should only indicate the CPU that the interrupt property is
inside?)

Finally we can have the edac driver look for a "qcom,krait"
compatible node in cpus that it can create a platform device for,
i.e..

static int __init krait_edac_driver_init(void)
{
        struct device_node *np;

        np = of_get_cpu_node(0, NULL);
        if (!np)
                return 0;

        if (!krait_edacp && of_device_is_compatible(np, "qcom,krait"))
                krait_edacp = of_platform_device_create(np, "krait_edac", NULL);
        of_node_put(np);

        return platform_driver_register(&krait_edac_driver);
}
module_init(krait_edac_driver_init);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Lorenzo Pieralisi]

On Fri, Mar 07, 2014 at 11:08:56PM +0000, Stephen Boyd wrote:
> On 02/26, Lorenzo Pieralisi wrote:
> > On Tue, Feb 25, 2014 at 08:48:38PM +0000, Kumar Gala wrote:
> > > 
> > > On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > > > 
> > > > As I mentioned, I do not like the idea of adding compatible properties
> > > > just to force the kernel to create platform devices out of device tree
> > > > nodes. On top of that I would avoid adding a compatible property
> > > > to the cpus node (after all properties like enable-method are common for all
> > > > cpus but still duplicated), my only concern being backward compatibility
> > > > here (ie if we do that for interrupts, we should do that also for other
> > > > common cpu nodes properties, otherwise we have different rules for
> > > > different properties).
> > > > 
> > > > I think you can then add interrupts to cpu nodes ("qcom,krait" specific),
> > > > and as you mentioned create a platform device for that.
> > > > 
> > > > Thanks,
> > > > Lorenzo
> > > 
> > > So I agree with the statement about adding compatibles just to create platform devices is wrong.  However its seems perfectly reasonable for a cpu node to have a compatible property.  I don't see why a CPU is any different from any other device described in a DT.
> > 
> > I was referring to the /cpus node, not to individual cpu nodes, where
> > the compatible property is already present now.
> > 
> 
> Ok I think I'll go ahead with moving the interrupts into each cpu node, i.e.:
> 
>         cpus {  
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 cpu@0 { 
>                         compatible = "qcom,krait";
>                         device_type = "cpu";
>                         reg = <0>;
>                         interrupts = <1 14 0x304>;
>                         next-level-cache = <&L2>;
>                 };
> 
>                 cpu@1 { 
>                         compatible = "qcom,krait";
>                         device_type = "cpu";
>                         reg = <1>;
>                         interrupts = <1 14 0x304>;
>                         next-level-cache = <&L2>;
>                 };
> 
>                 L2: l2-cache {
>                         compatible = "cache";
>                         interrupts = <0 2 0x4>;
> 		};
> 	};
> 
> Or should we be expressing the L1 cache as well? Something like:
> 
>         cpus {  
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 cpu@0 { 
>                         compatible = "qcom,krait";
>                         device_type = "cpu";
>                         reg = <0>;
>                         next-level-cache = <&L1_0>;
> 
> 			L1_0: l1-cache {
> 				compatible = "arm,arch-cache";
> 				interrupts = <1 14 0x304>;
> 				next-level-cache = <&L2>;
> 			}
>                 };
> 
>                 cpu@1 { 
>                         compatible = "qcom,krait";
>                         device_type = "cpu";
>                         reg = <1>;
>                         next-level-cache = <&L1_1>;
> 
> 			L1_1: l1-cache {
> 				compatible = "arm,arch-cache";
> 				interrupts = <1 14 0x304>;
> 				next-level-cache = <&L2>;
> 			}
>                 };
> 
>                 L2: l2-cache {
>                         compatible = "arm,arch-cache";
>                         interrupts = <0 2 0x4>;
> 		};
> 	};
> 
> (I'm also wondering if the 3rd cell of the interrupt binding
> should only indicate the CPU that the interrupt property is
> inside?)

I am not aware of interrupts associated with vanilla :) "arm,arch-cache"
objects, so I think that should be handled as a "qcom,krait" specific property
(in the cpu node), or you should add another cache binding (compatible) for
that.

As you might have noticed (idle states thread) I am keen on defining objects
for L1 caches explicitly, that patch still requires an ACK though (and
you need to update it since you cannot add an interrupt property for all
"arm,arch-cache" objects. I am sorry for being a pain, but I do not
think that's correct from a HW description standpoint).

> Finally we can have the edac driver look for a "qcom,krait"
> compatible node in cpus that it can create a platform device for,
> i.e..
> 
> static int __init krait_edac_driver_init(void)
> {
>         struct device_node *np;
> 
>         np = of_get_cpu_node(0, NULL);
>         if (!np)
>                 return 0;
> 
>         if (!krait_edacp && of_device_is_compatible(np, "qcom,krait"))
>                 krait_edacp = of_platform_device_create(np, "krait_edac", NULL);
>         of_node_put(np);
> 
>         return platform_driver_register(&krait_edac_driver);
> }
> module_init(krait_edac_driver_init);

It seems fine to me, but it requires an ACK from platform bus and DT
maintainers.

Lorenzo

Re: [PATCH v5 2/4] devicetree: bindings: Document Krait CPU/L1 EDAC

[Stephen Boyd]

On 03/11, Lorenzo Pieralisi wrote:
> On Fri, Mar 07, 2014 at 11:08:56PM +0000, Stephen Boyd wrote:
> > 
> > Or should we be expressing the L1 cache as well? Something like:
> > 
> >         cpus {  
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> > 
> >                 cpu@0 { 
> >                         compatible = "qcom,krait";
> >                         device_type = "cpu";
> >                         reg = <0>;
> >                         next-level-cache = <&L1_0>;
> > 
> > 			L1_0: l1-cache {
> > 				compatible = "arm,arch-cache";
> > 				interrupts = <1 14 0x304>;
> > 				next-level-cache = <&L2>;
> > 			}
> >                 };
> > 
> >                 cpu@1 { 
> >                         compatible = "qcom,krait";
> >                         device_type = "cpu";
> >                         reg = <1>;
> >                         next-level-cache = <&L1_1>;
> > 
> > 			L1_1: l1-cache {
> > 				compatible = "arm,arch-cache";
> > 				interrupts = <1 14 0x304>;
> > 				next-level-cache = <&L2>;
> > 			}
> >                 };
> > 
> >                 L2: l2-cache {
> >                         compatible = "arm,arch-cache";
> >                         interrupts = <0 2 0x4>;
> > 		};
> > 	};
> > 
> > (I'm also wondering if the 3rd cell of the interrupt binding
> > should only indicate the CPU that the interrupt property is
> > inside?)
> 
> I am not aware of interrupts associated with vanilla :) "arm,arch-cache"
> objects, so I think that should be handled as a "qcom,krait" specific property
> (in the cpu node), or you should add another cache binding (compatible) for
> that.
> 
> As you might have noticed (idle states thread) I am keen on defining objects
> for L1 caches explicitly, that patch still requires an ACK though (and
> you need to update it since you cannot add an interrupt property for all
> "arm,arch-cache" objects. I am sorry for being a pain, but I do not
> think that's correct from a HW description standpoint).
> 

Ok. s/arm,arch-cache/qcom,arch-cache/ then. I imagine it is easy
enough to add some bits in the cache binding once it's accepted.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation