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
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 <[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 | 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
Krait CPUs have a handful of L2 cache controller registers that
live behind a cp15 based indirection register. First you program
the indirection register (l2cpselr) to point the L2 'window'
register (l2cpdr) at what you want to read/write. Then you
read/write the 'window' register to do what you want. The
l2cpselr register is not banked per-cpu so we must lock around
accesses to it to prevent other CPUs from re-pointing l2cpdr
underneath us.
Cc: Mark Rutland <[email protected]>
Cc: Russell King <[email protected]>
Cc: Courtney Cavin <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
arch/arm/common/Kconfig | 3 ++
arch/arm/common/Makefile | 1 +
arch/arm/common/krait-l2-accessors.c | 58 +++++++++++++++++++++++++++++++
arch/arm/include/asm/krait-l2-accessors.h | 20 +++++++++++
4 files changed, 82 insertions(+)
create mode 100644 arch/arm/common/krait-l2-accessors.c
create mode 100644 arch/arm/include/asm/krait-l2-accessors.h
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
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
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
This allows us to probe the krait-edac driver.
Cc: David Brown <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
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
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.
--
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
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 <[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 | 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
>
>
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 <[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 | 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
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 <[email protected]>
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 <[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 | 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
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 <[email protected]>
> 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 <[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 | 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
>
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 <[email protected]>
> > 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 <[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 | 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
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 <[email protected]>
> > > 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 <[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 | 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
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
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
(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
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
On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <[email protected]> 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
On Tue, Feb 25, 2014 at 08:48:38PM +0000, Kumar Gala wrote:
>
> On Feb 25, 2014, at 5:16 AM, Lorenzo Pieralisi <[email protected]> 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