[PATCH RFC 0/3] coresight: enable debug module

[PATCH RFC 0/3] coresight: enable debug module

[Leo Yan]

This patch series is to enable coresight debug module. With debug
module we can check CPU state and PC value, etc. So this is helpful for
CPU lockup bugs, e.g. if one CPU has run into infinite loop with IRQ
disabled. The hang CPU cannot switch context and handle any interrupt,
so it cannot handle SMP call for stack dump, etc.

Furthermore, now ARMv8 introduces some runtime firmwares like ARM
trusted firmware BL31, so sometime CPU lockup may happen in the
firmware and cannot return back to kernel.

This initial patch series enable debug module and registers call back
notifier for PCSR register dumping when panic happens, so we can see
below dumping info for panic:

[   13.751777] Coresight debug module:
[   13.755275] CPU[0]: PSCR=0xffff000008090cbc
[   13.759469] CPU[1]: PSCR=0xffff0000088bf9e4
[   13.763662] CPU[2]: PSCR=0xffff000008090cc0
[   13.767856] CPU[3]: PSCR=0xffff000008090cb8
[   13.772049] CPU[4]: PSCR=0xffff000008090cc0
[   13.776243] CPU[5]: PSCR=0xffff000008090cbc
[   13.780436] CPU[6]: PSCR=0xffff000008090cc0
[   13.784630] CPU[7]: PSCR=0xffff000008090cbc

This patch series has been verified on 96boards Hikey.


Leo Yan (3):
  coresight: binding for coresight debug driver
  coresight: add support for debug module
  arm64: dts: register Hi6220's coresight debug module

 .../devicetree/bindings/arm/coresight.txt          |   9 +-
 .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   |  73 +++++++++
 drivers/hwtracing/coresight/Kconfig                |   8 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-debug.c      | 169 +++++++++++++++++++++
 5 files changed, 258 insertions(+), 2 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-debug.c

-- 
2.7.4

[PATCH RFC 0/3] coresight: enable debug module

[Leo Yan]

This patch series is to enable coresight debug module. With debug
module we can check CPU state and PC value, etc. So this is helpful for
CPU lockup bugs, e.g. if one CPU has run into infinite loop with IRQ
disabled. The hang CPU cannot switch context and handle any interrupt,
so it cannot handle SMP call for stack dump, etc.

Furthermore, now ARMv8 introduces some runtime firmwares like ARM
trusted firmware BL31, so sometime CPU lockup may happen in the
firmware and cannot return back to kernel.

This initial patch series enable debug module and registers call back
notifier for PCSR register dumping when panic happens, so we can see
below dumping info for panic:

[   13.751777] Coresight debug module:
[   13.755275] CPU[0]: PSCR=0xffff000008090cbc
[   13.759469] CPU[1]: PSCR=0xffff0000088bf9e4
[   13.763662] CPU[2]: PSCR=0xffff000008090cc0
[   13.767856] CPU[3]: PSCR=0xffff000008090cb8
[   13.772049] CPU[4]: PSCR=0xffff000008090cc0
[   13.776243] CPU[5]: PSCR=0xffff000008090cbc
[   13.780436] CPU[6]: PSCR=0xffff000008090cc0
[   13.784630] CPU[7]: PSCR=0xffff000008090cbc

This patch series has been verified on 96boards Hikey.


Leo Yan (3):
  coresight: binding for coresight debug driver
  coresight: add support for debug module
  arm64: dts: register Hi6220's coresight debug module

 .../devicetree/bindings/arm/coresight.txt          |   9 +-
 .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   |  73 +++++++++
 drivers/hwtracing/coresight/Kconfig                |   8 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-debug.c      | 169 +++++++++++++++++++++
 5 files changed, 258 insertions(+), 2 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-debug.c

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 0/3] coresight: enable debug module

[Leo Yan]

This patch series is to enable coresight debug module. With debug
module we can check CPU state and PC value, etc. So this is helpful for
CPU lockup bugs, e.g. if one CPU has run into infinite loop with IRQ
disabled. The hang CPU cannot switch context and handle any interrupt,
so it cannot handle SMP call for stack dump, etc.

Furthermore, now ARMv8 introduces some runtime firmwares like ARM
trusted firmware BL31, so sometime CPU lockup may happen in the
firmware and cannot return back to kernel.

This initial patch series enable debug module and registers call back
notifier for PCSR register dumping when panic happens, so we can see
below dumping info for panic:

[   13.751777] Coresight debug module:
[   13.755275] CPU[0]: PSCR=0xffff000008090cbc
[   13.759469] CPU[1]: PSCR=0xffff0000088bf9e4
[   13.763662] CPU[2]: PSCR=0xffff000008090cc0
[   13.767856] CPU[3]: PSCR=0xffff000008090cb8
[   13.772049] CPU[4]: PSCR=0xffff000008090cc0
[   13.776243] CPU[5]: PSCR=0xffff000008090cbc
[   13.780436] CPU[6]: PSCR=0xffff000008090cc0
[   13.784630] CPU[7]: PSCR=0xffff000008090cbc

This patch series has been verified on 96boards Hikey.


Leo Yan (3):
  coresight: binding for coresight debug driver
  coresight: add support for debug module
  arm64: dts: register Hi6220's coresight debug module

 .../devicetree/bindings/arm/coresight.txt          |   9 +-
 .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   |  73 +++++++++
 drivers/hwtracing/coresight/Kconfig                |   8 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-debug.c      | 169 +++++++++++++++++++++
 5 files changed, 258 insertions(+), 2 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-debug.c

-- 
2.7.4

[PATCH RFC 1/3] coresight: binding for coresight debug driver

[Leo Yan]

Adding compatible string for new coresight debug driver.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
index fcbae6a..3ff15fd 100644
--- a/Documentation/devicetree/bindings/arm/coresight.txt
+++ b/Documentation/devicetree/bindings/arm/coresight.txt
@@ -40,6 +40,9 @@ its hardware characteristcs.
 		- System Trace Macrocell:
 			"arm,coresight-stm", "arm,primecell"; [1]
 
+		- Debug Unit:
+			"arm,coresight-debug", "arm,primecell";
+
 	* reg: physical base address and length of the register
 	  set(s) of the component.
 
@@ -78,8 +81,10 @@ its hardware characteristcs.
 	* arm,cp14: must be present if the system accesses ETM/PTM management
 	  registers via co-processor 14.
 
-	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
-	  source is considered to belong to CPU0.
+* Optional properties for ETM/PTM/Debugs:
+
+	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
+	  the source is considered to belong to CPU0.
 
 * Optional property for TMC:
 
-- 
2.7.4

[PATCH RFC 1/3] coresight: binding for coresight debug driver

[Leo Yan]

Adding compatible string for new coresight debug driver.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
index fcbae6a..3ff15fd 100644
--- a/Documentation/devicetree/bindings/arm/coresight.txt
+++ b/Documentation/devicetree/bindings/arm/coresight.txt
@@ -40,6 +40,9 @@ its hardware characteristcs.
 		- System Trace Macrocell:
 			"arm,coresight-stm", "arm,primecell"; [1]
 
+		- Debug Unit:
+			"arm,coresight-debug", "arm,primecell";
+
 	* reg: physical base address and length of the register
 	  set(s) of the component.
 
@@ -78,8 +81,10 @@ its hardware characteristcs.
 	* arm,cp14: must be present if the system accesses ETM/PTM management
 	  registers via co-processor 14.
 
-	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
-	  source is considered to belong to CPU0.
+* Optional properties for ETM/PTM/Debugs:
+
+	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
+	  the source is considered to belong to CPU0.
 
 * Optional property for TMC:
 
-- 
2.7.4

[PATCH RFC 2/3] coresight: add support for debug module

[Leo Yan]

Coresight includes debug module and usually the module connects with CPU
debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
the debug registers in the chapter "H9: External Debug Register
Descriptions".

After enable the debug module we can check CPU state and PC value, etc.
So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
into infinite loop with IRQ disabled. So the CPU cannot switch context
and handle any interrupt, so it cannot handle SMP call for stack dump,
etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
firmware and cannot return back to kernel.

This patch is to enable coresight debug module and register callback
notifier for panic; so when system detect the CPU lockup we can utilize
debug module registers to get to know PC value for all CPUs; so we can
quickly know the hang address for CPUs.

This is initial driver for coresight debug module and could enhance it
later according to debugging requirement.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/Kconfig           |   8 ++
 drivers/hwtracing/coresight/Makefile          |   1 +
 drivers/hwtracing/coresight/coresight-debug.c | 169 ++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 drivers/hwtracing/coresight/coresight-debug.c

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 130cb21..dcf59cc 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -89,4 +89,12 @@ config CORESIGHT_STM
 	  logging useful software events or data coming from various entities
 	  in the system, possibly running different OSs
 
+config CORESIGHT_DEBUG
+	bool "CoreSight debug driver"
+	depends on ARM || ARM64
+	help
+	  This driver provides support for coresight debugging module. This
+	  is primarily used for printing out debug registers for panic and
+	  soft and hard lockup.
+
 endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index af480d9..d540d45 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
 					coresight-etm4x-sysfs.o
 obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
 obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
+obj-$(CONFIG_CORESIGHT_DEBUG) += coresight-debug.o
diff --git a/drivers/hwtracing/coresight/coresight-debug.c b/drivers/hwtracing/coresight/coresight-debug.c
new file mode 100644
index 0000000..28206a83
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-debug.c
@@ -0,0 +1,169 @@
+/*
+ * Copyright(C) 2017 Linaro Limited. All rights reserved.
+ * Author: Leo Yan <leo.yan@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/smp.h>
+#include <linux/sysfs.h>
+#include <linux/stat.h>
+#include <linux/clk.h>
+#include <linux/cpu.h>
+#include <linux/coresight.h>
+#include <linux/amba/bus.h>
+#include <linux/uaccess.h>
+
+#include "coresight-priv.h"
+
+#define EDPCSR_LO	0x0A0
+#define EDPCSR_HI	0x0AC
+#define EDOSLAR		0x300
+#define EDOSLSR		0x304
+#define EDPDCR		0x310
+#define EDPDSR		0x314
+
+struct debug_drvdata {
+	void __iomem	*base;
+	struct device	*dev;
+	int		cpu;
+};
+
+static struct debug_drvdata *debug_drvdata[NR_CPUS];
+
+static void debug_os_unlock(struct debug_drvdata *drvdata)
+{
+	/* Unlocks the debug registers */
+	writel_relaxed(0x0, drvdata->base + EDOSLAR);
+	isb();
+}
+
+static void debug_read_pcsr(struct debug_drvdata *drvdata)
+{
+	u32 pcsr_hi, pcsr_lo;
+
+	CS_UNLOCK(drvdata->base);
+
+	debug_os_unlock(drvdata);
+
+#ifdef CONFIG_64BIT
+	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
+	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
+
+	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
+		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
+#else
+	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
+
+	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
+#endif
+
+	CS_LOCK(drvdata->base);
+}
+
+/*
+ * Dump out memory limit information on panic.
+ */
+static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
+{
+	int i;
+
+	pr_emerg("Coresight debug module:\n");
+
+	for_each_possible_cpu(i) {
+
+		if (!debug_drvdata[i])
+			continue;
+
+		debug_read_pcsr(debug_drvdata[i]);
+	}
+
+	return 0;
+}
+
+static struct notifier_block debug_notifier = {
+	.notifier_call = dump_debug,
+};
+
+static int __init register_coresight_debug_dumper(void)
+{
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &debug_notifier);
+	return 0;
+}
+__initcall(register_coresight_debug_dumper);
+
+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	void __iomem *base;
+	struct device *dev = &adev->dev;
+	struct coresight_platform_data *pdata = NULL;
+	struct debug_drvdata *drvdata;
+	struct resource *res = &adev->res;
+	struct device_node *np = adev->dev.of_node;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	if (np) {
+		pdata = of_get_coresight_platform_data(dev, np);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+		adev->dev.platform_data = pdata;
+	}
+
+	drvdata->dev = &adev->dev;
+	dev_set_drvdata(dev, drvdata);
+
+	/* Validity for the resource is already checked by the AMBA core */
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	drvdata->base = base;
+	drvdata->cpu = pdata ? pdata->cpu : 0;
+	debug_drvdata[drvdata->cpu] = drvdata;
+
+	dev_info(dev, "%s initialized\n", (char *)id->data);
+	return 0;
+}
+
+static struct amba_id debug_ids[] = {
+	{       /* Debug for Cortex-A53 */
+		.id	= 0x000bbd03,
+		.mask	= 0x000fffff,
+		.data	= "debug",
+	},
+	{ 0, 0},
+};
+
+static struct amba_driver debug_driver = {
+	.drv = {
+		.name   = "coresight-debug",
+		.suppress_bind_attrs = true,
+	},
+	.probe		= debug_probe,
+	.id_table	= debug_ids,
+};
+builtin_amba_driver(debug_driver);
-- 
2.7.4

[PATCH RFC 2/3] coresight: add support for debug module

[Leo Yan]

Coresight includes debug module and usually the module connects with CPU
debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
the debug registers in the chapter "H9: External Debug Register
Descriptions".

After enable the debug module we can check CPU state and PC value, etc.
So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
into infinite loop with IRQ disabled. So the CPU cannot switch context
and handle any interrupt, so it cannot handle SMP call for stack dump,
etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
firmware and cannot return back to kernel.

This patch is to enable coresight debug module and register callback
notifier for panic; so when system detect the CPU lockup we can utilize
debug module registers to get to know PC value for all CPUs; so we can
quickly know the hang address for CPUs.

This is initial driver for coresight debug module and could enhance it
later according to debugging requirement.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/Kconfig           |   8 ++
 drivers/hwtracing/coresight/Makefile          |   1 +
 drivers/hwtracing/coresight/coresight-debug.c | 169 ++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 drivers/hwtracing/coresight/coresight-debug.c

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 130cb21..dcf59cc 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -89,4 +89,12 @@ config CORESIGHT_STM
 	  logging useful software events or data coming from various entities
 	  in the system, possibly running different OSs
 
+config CORESIGHT_DEBUG
+	bool "CoreSight debug driver"
+	depends on ARM || ARM64
+	help
+	  This driver provides support for coresight debugging module. This
+	  is primarily used for printing out debug registers for panic and
+	  soft and hard lockup.
+
 endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index af480d9..d540d45 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
 					coresight-etm4x-sysfs.o
 obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
 obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
+obj-$(CONFIG_CORESIGHT_DEBUG) += coresight-debug.o
diff --git a/drivers/hwtracing/coresight/coresight-debug.c b/drivers/hwtracing/coresight/coresight-debug.c
new file mode 100644
index 0000000..28206a83
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-debug.c
@@ -0,0 +1,169 @@
+/*
+ * Copyright(C) 2017 Linaro Limited. All rights reserved.
+ * Author: Leo Yan <leo.yan@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/smp.h>
+#include <linux/sysfs.h>
+#include <linux/stat.h>
+#include <linux/clk.h>
+#include <linux/cpu.h>
+#include <linux/coresight.h>
+#include <linux/amba/bus.h>
+#include <linux/uaccess.h>
+
+#include "coresight-priv.h"
+
+#define EDPCSR_LO	0x0A0
+#define EDPCSR_HI	0x0AC
+#define EDOSLAR		0x300
+#define EDOSLSR		0x304
+#define EDPDCR		0x310
+#define EDPDSR		0x314
+
+struct debug_drvdata {
+	void __iomem	*base;
+	struct device	*dev;
+	int		cpu;
+};
+
+static struct debug_drvdata *debug_drvdata[NR_CPUS];
+
+static void debug_os_unlock(struct debug_drvdata *drvdata)
+{
+	/* Unlocks the debug registers */
+	writel_relaxed(0x0, drvdata->base + EDOSLAR);
+	isb();
+}
+
+static void debug_read_pcsr(struct debug_drvdata *drvdata)
+{
+	u32 pcsr_hi, pcsr_lo;
+
+	CS_UNLOCK(drvdata->base);
+
+	debug_os_unlock(drvdata);
+
+#ifdef CONFIG_64BIT
+	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
+	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
+
+	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
+		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
+#else
+	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
+
+	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
+#endif
+
+	CS_LOCK(drvdata->base);
+}
+
+/*
+ * Dump out memory limit information on panic.
+ */
+static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
+{
+	int i;
+
+	pr_emerg("Coresight debug module:\n");
+
+	for_each_possible_cpu(i) {
+
+		if (!debug_drvdata[i])
+			continue;
+
+		debug_read_pcsr(debug_drvdata[i]);
+	}
+
+	return 0;
+}
+
+static struct notifier_block debug_notifier = {
+	.notifier_call = dump_debug,
+};
+
+static int __init register_coresight_debug_dumper(void)
+{
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &debug_notifier);
+	return 0;
+}
+__initcall(register_coresight_debug_dumper);
+
+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	void __iomem *base;
+	struct device *dev = &adev->dev;
+	struct coresight_platform_data *pdata = NULL;
+	struct debug_drvdata *drvdata;
+	struct resource *res = &adev->res;
+	struct device_node *np = adev->dev.of_node;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	if (np) {
+		pdata = of_get_coresight_platform_data(dev, np);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+		adev->dev.platform_data = pdata;
+	}
+
+	drvdata->dev = &adev->dev;
+	dev_set_drvdata(dev, drvdata);
+
+	/* Validity for the resource is already checked by the AMBA core */
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	drvdata->base = base;
+	drvdata->cpu = pdata ? pdata->cpu : 0;
+	debug_drvdata[drvdata->cpu] = drvdata;
+
+	dev_info(dev, "%s initialized\n", (char *)id->data);
+	return 0;
+}
+
+static struct amba_id debug_ids[] = {
+	{       /* Debug for Cortex-A53 */
+		.id	= 0x000bbd03,
+		.mask	= 0x000fffff,
+		.data	= "debug",
+	},
+	{ 0, 0},
+};
+
+static struct amba_driver debug_driver = {
+	.drv = {
+		.name   = "coresight-debug",
+		.suppress_bind_attrs = true,
+	},
+	.probe		= debug_probe,
+	.id_table	= debug_ids,
+};
+builtin_amba_driver(debug_driver);
-- 
2.7.4

[PATCH RFC 3/3] arm64: dts: register Hi6220's coresight debug module

[Leo Yan]

Bind coresight debug driver for Hi6220.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   | 73 ++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
index 77c2aab..e14d75c 100644
--- a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
@@ -15,6 +15,79 @@
 		#size-cells = <2>;
 		compatible = "arm,amba-bus";
 		ranges;
+
+		debug@0,f6590000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf6590000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu0>;
+		};
+
+		debug@1,f6592000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf6592000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu1>;
+		};
+
+		debug@2,f6594000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf6594000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu2>;
+		};
+
+		debug@3,f6596000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf6596000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu3>;
+		};
+
+		debug@4,f65d0000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf65d0000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu4>;
+		};
+
+		debug@5,f65d2000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf65d2000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu5>;
+		};
+
+		debug@6,f65d4000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf65d4000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu6>;
+		};
+
+		debug@7,f65d6000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf65d6000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu7>;
+		};
+
 		etm@0,f659c000 {
 			compatible = "arm,coresight-etm4x","arm,primecell";
 			reg = <0 0xf659c000 0 0x1000>;
-- 
2.7.4

[PATCH RFC 3/3] arm64: dts: register Hi6220's coresight debug module

[Leo Yan]

Bind coresight debug driver for Hi6220.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   | 73 ++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
index 77c2aab..e14d75c 100644
--- a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
@@ -15,6 +15,79 @@
 		#size-cells = <2>;
 		compatible = "arm,amba-bus";
 		ranges;
+
+		debug at 0,f6590000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf6590000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu0>;
+		};
+
+		debug at 1,f6592000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf6592000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu1>;
+		};
+
+		debug at 2,f6594000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf6594000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu2>;
+		};
+
+		debug at 3,f6596000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf6596000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu3>;
+		};
+
+		debug at 4,f65d0000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf65d0000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu4>;
+		};
+
+		debug at 5,f65d2000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf65d2000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu5>;
+		};
+
+		debug at 6,f65d4000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf65d4000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu6>;
+		};
+
+		debug at 7,f65d6000 {
+			compatible = "arm,coresight-debug","arm,primecell";
+			reg = <0 0xf65d6000 0 0x1000>;
+			default_enable;
+			clocks = <&sys_ctrl HI6220_CS_ATB>;
+			clock-names = "apb_pclk";
+			cpu = <&cpu7>;
+		};
+
 		etm at 0,f659c000 {
 			compatible = "arm,coresight-etm4x","arm,primecell";
 			reg = <0 0xf659c000 0 0x1000>;
-- 
2.7.4

Re: [PATCH RFC 0/3] coresight: enable debug module

[Mike Leach]

[-- Attachment #1: Type: text/plain, Size: 4384 bytes --]

Hi Leo,

A few comments about your driver RFC.

i) As it stands this looks like it will work for v8 cores, but would need
refining for v7. There are subtle differences in the PC sampling between
the two architectures.

ii) the routine that dumps the PCSR register values across all the
available cores, appears to assume that all the cores are powered and as
such the registers are accessible.
I think it would be useful in the Kconfig help to point this out.

At ARM we have encountered systems that have quite aggressive power
management policies which will disable both the core power domain and debug
power domain for unused cores / clusters. On such a system I think that
there would be an slave error return provided by the memory access to
unpowered elements, resulting in undesirable behavior, or even a bus
lockup. These are both errors we have seen using the external debug
registers via an external debugger.

At the very least the user needs to be warned so he can adjust his system
accordingly (e.g. when we are debugging a Juno based Linux system using the
external debug registers / debugger. we will often disable power management
using a script to ensure that cores stay up and debuggable.)

iii) I would recommend that you take note of the relevant bits in EDPRSR
which may indicate that reading EDPCSR will cause an access error.
Specifically DLK, PU would gate valid access to OSLAR -> the state of which
you can also determine using EDPRSR.

iv) When running in higher EL levels, such as might be the case with
trusted firmware, then PC sampling may not be permitted by the OS, when the
EDPCSR value will return 0xFFFFFFFF. The PCSR dump message should reflect
this.

v) EDVIDSR - which is sampled at the same time as EDPCSR_lo contains
interesting information regarding the validity of the EDPCSR_hi registers
and current EL and security state.
I think it would be worth reading and interpreting this register in
addition to the PC sample.

vi) The set of registers you are using are the "PC sample-based profiling
extension". (ARM v8 manual section H7.) It might be more accurate to
describe the driver as implementing support for this rather than full debug.

Best Regards

Mike Leach



On 13 February 2017 at 06:11, Leo Yan <leo.yan@linaro.org> wrote:

> This patch series is to enable coresight debug module. With debug
> module we can check CPU state and PC value, etc. So this is helpful for
> CPU lockup bugs, e.g. if one CPU has run into infinite loop with IRQ
> disabled. The hang CPU cannot switch context and handle any interrupt,
> so it cannot handle SMP call for stack dump, etc.
>
> Furthermore, now ARMv8 introduces some runtime firmwares like ARM
> trusted firmware BL31, so sometime CPU lockup may happen in the
> firmware and cannot return back to kernel.
>
> This initial patch series enable debug module and registers call back
> notifier for PCSR register dumping when panic happens, so we can see
> below dumping info for panic:
>
> [   13.751777] Coresight debug module:
> [   13.755275] CPU[0]: PSCR=0xffff000008090cbc
> [   13.759469] CPU[1]: PSCR=0xffff0000088bf9e4
> [   13.763662] CPU[2]: PSCR=0xffff000008090cc0
> [   13.767856] CPU[3]: PSCR=0xffff000008090cb8
> [   13.772049] CPU[4]: PSCR=0xffff000008090cc0
> [   13.776243] CPU[5]: PSCR=0xffff000008090cbc
> [   13.780436] CPU[6]: PSCR=0xffff000008090cc0
> [   13.784630] CPU[7]: PSCR=0xffff000008090cbc
>
> This patch series has been verified on 96boards Hikey.
>
>
> Leo Yan (3):
>   coresight: binding for coresight debug driver
>   coresight: add support for debug module
>   arm64: dts: register Hi6220's coresight debug module
>
>  .../devicetree/bindings/arm/coresight.txt          |   9 +-
>  .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   |  73 +++++++++
>  drivers/hwtracing/coresight/Kconfig                |   8 +
>  drivers/hwtracing/coresight/Makefile               |   1 +
>  drivers/hwtracing/coresight/coresight-debug.c      | 169
> +++++++++++++++++++++
>  5 files changed, 258 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
>
> --
> 2.7.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Blackburn Design Centre. UK

[-- Attachment #2: Type: text/html, Size: 6523 bytes --]

Re: [PATCH RFC 0/3] coresight: enable debug module

[Leo Yan]

Hi Mike,

On Mon, Feb 13, 2017 at 03:58:47PM +0000, Mike Leach wrote:
> Hi Leo,
> 
> A few comments about your driver RFC.
> 
> i) As it stands this looks like it will work for v8 cores, but would need
> refining for v7. There are subtle differences in the PC sampling between
> the two architectures.

Thanks a lot for your suggestions, I totally accept them.

For itme i) I can find detailed description in ARM-ARM 'C11.11.34
DBGPCSR, Program Counter Sampling Register', and below items I can
find corresponding description in ARMv8-ARM.

Will fix code according these comments.

Thanks,
Leo Yan

> ii) the routine that dumps the PCSR register values across all the
> available cores, appears to assume that all the cores are powered and as
> such the registers are accessible.
> I think it would be useful in the Kconfig help to point this out.
> 
> At ARM we have encountered systems that have quite aggressive power
> management policies which will disable both the core power domain and debug
> power domain for unused cores / clusters. On such a system I think that
> there would be an slave error return provided by the memory access to
> unpowered elements, resulting in undesirable behavior, or even a bus
> lockup. These are both errors we have seen using the external debug
> registers via an external debugger.
> 
> At the very least the user needs to be warned so he can adjust his system
> accordingly (e.g. when we are debugging a Juno based Linux system using the
> external debug registers / debugger. we will often disable power management
> using a script to ensure that cores stay up and debuggable.)
> 
> iii) I would recommend that you take note of the relevant bits in EDPRSR
> which may indicate that reading EDPCSR will cause an access error.
> Specifically DLK, PU would gate valid access to OSLAR -> the state of which
> you can also determine using EDPRSR.
> 
> iv) When running in higher EL levels, such as might be the case with
> trusted firmware, then PC sampling may not be permitted by the OS, when the
> EDPCSR value will return 0xFFFFFFFF. The PCSR dump message should reflect
> this.
> 
> v) EDVIDSR - which is sampled at the same time as EDPCSR_lo contains
> interesting information regarding the validity of the EDPCSR_hi registers
> and current EL and security state.
> I think it would be worth reading and interpreting this register in
> addition to the PC sample.
> 
> vi) The set of registers you are using are the "PC sample-based profiling
> extension". (ARM v8 manual section H7.) It might be more accurate to
> describe the driver as implementing support for this rather than full debug.
> 
> Best Regards
> 
> Mike Leach
> 
> 
> 
> On 13 February 2017 at 06:11, Leo Yan <leo.yan@linaro.org> wrote:
> 
> > This patch series is to enable coresight debug module. With debug
> > module we can check CPU state and PC value, etc. So this is helpful for
> > CPU lockup bugs, e.g. if one CPU has run into infinite loop with IRQ
> > disabled. The hang CPU cannot switch context and handle any interrupt,
> > so it cannot handle SMP call for stack dump, etc.
> >
> > Furthermore, now ARMv8 introduces some runtime firmwares like ARM
> > trusted firmware BL31, so sometime CPU lockup may happen in the
> > firmware and cannot return back to kernel.
> >
> > This initial patch series enable debug module and registers call back
> > notifier for PCSR register dumping when panic happens, so we can see
> > below dumping info for panic:
> >
> > [   13.751777] Coresight debug module:
> > [   13.755275] CPU[0]: PSCR=0xffff000008090cbc
> > [   13.759469] CPU[1]: PSCR=0xffff0000088bf9e4
> > [   13.763662] CPU[2]: PSCR=0xffff000008090cc0
> > [   13.767856] CPU[3]: PSCR=0xffff000008090cb8
> > [   13.772049] CPU[4]: PSCR=0xffff000008090cc0
> > [   13.776243] CPU[5]: PSCR=0xffff000008090cbc
> > [   13.780436] CPU[6]: PSCR=0xffff000008090cc0
> > [   13.784630] CPU[7]: PSCR=0xffff000008090cbc
> >
> > This patch series has been verified on 96boards Hikey.
> >
> >
> > Leo Yan (3):
> >   coresight: binding for coresight debug driver
> >   coresight: add support for debug module
> >   arm64: dts: register Hi6220's coresight debug module
> >
> >  .../devicetree/bindings/arm/coresight.txt          |   9 +-
> >  .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   |  73 +++++++++
> >  drivers/hwtracing/coresight/Kconfig                |   8 +
> >  drivers/hwtracing/coresight/Makefile               |   1 +
> >  drivers/hwtracing/coresight/coresight-debug.c      | 169
> > +++++++++++++++++++++
> >  5 files changed, 258 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
> >
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> 
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Blackburn Design Centre. UK

Re: [PATCH RFC 0/3] coresight: enable debug module

[Leo Yan]

Hi Mike,

On Mon, Feb 13, 2017 at 03:58:47PM +0000, Mike Leach wrote:
> Hi Leo,
> 
> A few comments about your driver RFC.
> 
> i) As it stands this looks like it will work for v8 cores, but would need
> refining for v7. There are subtle differences in the PC sampling between
> the two architectures.

Thanks a lot for your suggestions, I totally accept them.

For itme i) I can find detailed description in ARM-ARM 'C11.11.34
DBGPCSR, Program Counter Sampling Register', and below items I can
find corresponding description in ARMv8-ARM.

Will fix code according these comments.

Thanks,
Leo Yan

> ii) the routine that dumps the PCSR register values across all the
> available cores, appears to assume that all the cores are powered and as
> such the registers are accessible.
> I think it would be useful in the Kconfig help to point this out.
> 
> At ARM we have encountered systems that have quite aggressive power
> management policies which will disable both the core power domain and debug
> power domain for unused cores / clusters. On such a system I think that
> there would be an slave error return provided by the memory access to
> unpowered elements, resulting in undesirable behavior, or even a bus
> lockup. These are both errors we have seen using the external debug
> registers via an external debugger.
> 
> At the very least the user needs to be warned so he can adjust his system
> accordingly (e.g. when we are debugging a Juno based Linux system using the
> external debug registers / debugger. we will often disable power management
> using a script to ensure that cores stay up and debuggable.)
> 
> iii) I would recommend that you take note of the relevant bits in EDPRSR
> which may indicate that reading EDPCSR will cause an access error.
> Specifically DLK, PU would gate valid access to OSLAR -> the state of which
> you can also determine using EDPRSR.
> 
> iv) When running in higher EL levels, such as might be the case with
> trusted firmware, then PC sampling may not be permitted by the OS, when the
> EDPCSR value will return 0xFFFFFFFF. The PCSR dump message should reflect
> this.
> 
> v) EDVIDSR - which is sampled at the same time as EDPCSR_lo contains
> interesting information regarding the validity of the EDPCSR_hi registers
> and current EL and security state.
> I think it would be worth reading and interpreting this register in
> addition to the PC sample.
> 
> vi) The set of registers you are using are the "PC sample-based profiling
> extension". (ARM v8 manual section H7.) It might be more accurate to
> describe the driver as implementing support for this rather than full debug.
> 
> Best Regards
> 
> Mike Leach
> 
> 
> 
> On 13 February 2017 at 06:11, Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > This patch series is to enable coresight debug module. With debug
> > module we can check CPU state and PC value, etc. So this is helpful for
> > CPU lockup bugs, e.g. if one CPU has run into infinite loop with IRQ
> > disabled. The hang CPU cannot switch context and handle any interrupt,
> > so it cannot handle SMP call for stack dump, etc.
> >
> > Furthermore, now ARMv8 introduces some runtime firmwares like ARM
> > trusted firmware BL31, so sometime CPU lockup may happen in the
> > firmware and cannot return back to kernel.
> >
> > This initial patch series enable debug module and registers call back
> > notifier for PCSR register dumping when panic happens, so we can see
> > below dumping info for panic:
> >
> > [   13.751777] Coresight debug module:
> > [   13.755275] CPU[0]: PSCR=0xffff000008090cbc
> > [   13.759469] CPU[1]: PSCR=0xffff0000088bf9e4
> > [   13.763662] CPU[2]: PSCR=0xffff000008090cc0
> > [   13.767856] CPU[3]: PSCR=0xffff000008090cb8
> > [   13.772049] CPU[4]: PSCR=0xffff000008090cc0
> > [   13.776243] CPU[5]: PSCR=0xffff000008090cbc
> > [   13.780436] CPU[6]: PSCR=0xffff000008090cc0
> > [   13.784630] CPU[7]: PSCR=0xffff000008090cbc
> >
> > This patch series has been verified on 96boards Hikey.
> >
> >
> > Leo Yan (3):
> >   coresight: binding for coresight debug driver
> >   coresight: add support for debug module
> >   arm64: dts: register Hi6220's coresight debug module
> >
> >  .../devicetree/bindings/arm/coresight.txt          |   9 +-
> >  .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   |  73 +++++++++
> >  drivers/hwtracing/coresight/Kconfig                |   8 +
> >  drivers/hwtracing/coresight/Makefile               |   1 +
> >  drivers/hwtracing/coresight/coresight-debug.c      | 169
> > +++++++++++++++++++++
> >  5 files changed, 258 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
> >
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> 
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Blackburn Design Centre. UK
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 0/3] coresight: enable debug module

[Leo Yan]

Hi Mike,

On Mon, Feb 13, 2017 at 03:58:47PM +0000, Mike Leach wrote:
> Hi Leo,
> 
> A few comments about your driver RFC.
> 
> i) As it stands this looks like it will work for v8 cores, but would need
> refining for v7. There are subtle differences in the PC sampling between
> the two architectures.

Thanks a lot for your suggestions, I totally accept them.

For itme i) I can find detailed description in ARM-ARM 'C11.11.34
DBGPCSR, Program Counter Sampling Register', and below items I can
find corresponding description in ARMv8-ARM.

Will fix code according these comments.

Thanks,
Leo Yan

> ii) the routine that dumps the PCSR register values across all the
> available cores, appears to assume that all the cores are powered and as
> such the registers are accessible.
> I think it would be useful in the Kconfig help to point this out.
> 
> At ARM we have encountered systems that have quite aggressive power
> management policies which will disable both the core power domain and debug
> power domain for unused cores / clusters. On such a system I think that
> there would be an slave error return provided by the memory access to
> unpowered elements, resulting in undesirable behavior, or even a bus
> lockup. These are both errors we have seen using the external debug
> registers via an external debugger.
> 
> At the very least the user needs to be warned so he can adjust his system
> accordingly (e.g. when we are debugging a Juno based Linux system using the
> external debug registers / debugger. we will often disable power management
> using a script to ensure that cores stay up and debuggable.)
> 
> iii) I would recommend that you take note of the relevant bits in EDPRSR
> which may indicate that reading EDPCSR will cause an access error.
> Specifically DLK, PU would gate valid access to OSLAR -> the state of which
> you can also determine using EDPRSR.
> 
> iv) When running in higher EL levels, such as might be the case with
> trusted firmware, then PC sampling may not be permitted by the OS, when the
> EDPCSR value will return 0xFFFFFFFF. The PCSR dump message should reflect
> this.
> 
> v) EDVIDSR - which is sampled at the same time as EDPCSR_lo contains
> interesting information regarding the validity of the EDPCSR_hi registers
> and current EL and security state.
> I think it would be worth reading and interpreting this register in
> addition to the PC sample.
> 
> vi) The set of registers you are using are the "PC sample-based profiling
> extension". (ARM v8 manual section H7.) It might be more accurate to
> describe the driver as implementing support for this rather than full debug.
> 
> Best Regards
> 
> Mike Leach
> 
> 
> 
> On 13 February 2017 at 06:11, Leo Yan <leo.yan@linaro.org> wrote:
> 
> > This patch series is to enable coresight debug module. With debug
> > module we can check CPU state and PC value, etc. So this is helpful for
> > CPU lockup bugs, e.g. if one CPU has run into infinite loop with IRQ
> > disabled. The hang CPU cannot switch context and handle any interrupt,
> > so it cannot handle SMP call for stack dump, etc.
> >
> > Furthermore, now ARMv8 introduces some runtime firmwares like ARM
> > trusted firmware BL31, so sometime CPU lockup may happen in the
> > firmware and cannot return back to kernel.
> >
> > This initial patch series enable debug module and registers call back
> > notifier for PCSR register dumping when panic happens, so we can see
> > below dumping info for panic:
> >
> > [   13.751777] Coresight debug module:
> > [   13.755275] CPU[0]: PSCR=0xffff000008090cbc
> > [   13.759469] CPU[1]: PSCR=0xffff0000088bf9e4
> > [   13.763662] CPU[2]: PSCR=0xffff000008090cc0
> > [   13.767856] CPU[3]: PSCR=0xffff000008090cb8
> > [   13.772049] CPU[4]: PSCR=0xffff000008090cc0
> > [   13.776243] CPU[5]: PSCR=0xffff000008090cbc
> > [   13.780436] CPU[6]: PSCR=0xffff000008090cc0
> > [   13.784630] CPU[7]: PSCR=0xffff000008090cbc
> >
> > This patch series has been verified on 96boards Hikey.
> >
> >
> > Leo Yan (3):
> >   coresight: binding for coresight debug driver
> >   coresight: add support for debug module
> >   arm64: dts: register Hi6220's coresight debug module
> >
> >  .../devicetree/bindings/arm/coresight.txt          |   9 +-
> >  .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   |  73 +++++++++
> >  drivers/hwtracing/coresight/Kconfig                |   8 +
> >  drivers/hwtracing/coresight/Makefile               |   1 +
> >  drivers/hwtracing/coresight/coresight-debug.c      | 169
> > +++++++++++++++++++++
> >  5 files changed, 258 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
> >
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> 
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Blackburn Design Centre. UK

Re: [PATCH RFC 1/3] coresight: binding for coresight debug driver

[Mark Rutland]

On Mon, Feb 13, 2017 at 02:11:36PM +0800, Leo Yan wrote:
> Adding compatible string for new coresight debug driver.

Please give a more thorough description of the hardware this binding
this is intended to describe.

It's not entirely clear which component this binding is intended to
describe.

Thanks,
Mark.

> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index fcbae6a..3ff15fd 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -40,6 +40,9 @@ its hardware characteristcs.
>  		- System Trace Macrocell:
>  			"arm,coresight-stm", "arm,primecell"; [1]
>  
> +		- Debug Unit:
> +			"arm,coresight-debug", "arm,primecell";
> +
>  	* reg: physical base address and length of the register
>  	  set(s) of the component.
>  
> @@ -78,8 +81,10 @@ its hardware characteristcs.
>  	* arm,cp14: must be present if the system accesses ETM/PTM management
>  	  registers via co-processor 14.
>  
> -	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
> -	  source is considered to belong to CPU0.
> +* Optional properties for ETM/PTM/Debugs:
> +
> +	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
> +	  the source is considered to belong to CPU0.
>  
>  * Optional property for TMC:
>  
> -- 
> 2.7.4
> 

Re: [PATCH RFC 1/3] coresight: binding for coresight debug driver

[Mark Rutland]

On Mon, Feb 13, 2017 at 02:11:36PM +0800, Leo Yan wrote:
> Adding compatible string for new coresight debug driver.

Please give a more thorough description of the hardware this binding
this is intended to describe.

It's not entirely clear which component this binding is intended to
describe.

Thanks,
Mark.

> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index fcbae6a..3ff15fd 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -40,6 +40,9 @@ its hardware characteristcs.
>  		- System Trace Macrocell:
>  			"arm,coresight-stm", "arm,primecell"; [1]
>  
> +		- Debug Unit:
> +			"arm,coresight-debug", "arm,primecell";
> +
>  	* reg: physical base address and length of the register
>  	  set(s) of the component.
>  
> @@ -78,8 +81,10 @@ its hardware characteristcs.
>  	* arm,cp14: must be present if the system accesses ETM/PTM management
>  	  registers via co-processor 14.
>  
> -	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
> -	  source is considered to belong to CPU0.
> +* Optional properties for ETM/PTM/Debugs:
> +
> +	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
> +	  the source is considered to belong to CPU0.
>  
>  * Optional property for TMC:
>  
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 1/3] coresight: binding for coresight debug driver

[Mark Rutland]

On Mon, Feb 13, 2017 at 02:11:36PM +0800, Leo Yan wrote:
> Adding compatible string for new coresight debug driver.

Please give a more thorough description of the hardware this binding
this is intended to describe.

It's not entirely clear which component this binding is intended to
describe.

Thanks,
Mark.

> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index fcbae6a..3ff15fd 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -40,6 +40,9 @@ its hardware characteristcs.
>  		- System Trace Macrocell:
>  			"arm,coresight-stm", "arm,primecell"; [1]
>  
> +		- Debug Unit:
> +			"arm,coresight-debug", "arm,primecell";
> +
>  	* reg: physical base address and length of the register
>  	  set(s) of the component.
>  
> @@ -78,8 +81,10 @@ its hardware characteristcs.
>  	* arm,cp14: must be present if the system accesses ETM/PTM management
>  	  registers via co-processor 14.
>  
> -	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
> -	  source is considered to belong to CPU0.
> +* Optional properties for ETM/PTM/Debugs:
> +
> +	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
> +	  the source is considered to belong to CPU0.
>  
>  * Optional property for TMC:
>  
> -- 
> 2.7.4
> 

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Mark Rutland]

On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> the debug registers in the chapter "H9: External Debug Register
> Descriptions".

This should have been in the binding description also.

The layout of the ARM ARM can change over time, so please refer to the
full document number, which can be found at the bottom of each page
(e.g. ARM DDI 0487A.j).

> After enable the debug module we can check CPU state and PC value, etc.
> So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> into infinite loop with IRQ disabled. So the CPU cannot switch context
> and handle any interrupt, so it cannot handle SMP call for stack dump,
> etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> firmware and cannot return back to kernel.

I would generally expect that the secure world would lock down
debugging, as this poses a security risk.

I take it that this is only unlocked on development firmware.

Given that cores can be powered down outside of our control, I'm not
sure that accesses to these registers is safe in general.

> This patch is to enable coresight debug module and register callback
> notifier for panic; so when system detect the CPU lockup we can utilize
> debug module registers to get to know PC value for all CPUs; so we can
> quickly know the hang address for CPUs.
>
> This is initial driver for coresight debug module and could enhance it
> later according to debugging requirement.

How does this interact with an external debugger making use of these
registers?

[...]

> +static struct debug_drvdata *debug_drvdata[NR_CPUS];

A per-cpu variable is preferred to an NR_CPUS sized array.

> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> +     /* Unlocks the debug registers */
> +     writel_relaxed(0x0, drvdata->base + EDOSLAR);
> +     isb();
> +}

I do not believe this barrier is correct.

[...]

> +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> +{
> +     u32 pcsr_hi, pcsr_lo;
> +
> +     CS_UNLOCK(drvdata->base);
> +
> +     debug_os_unlock(drvdata);
> +
> +#ifdef CONFIG_64BIT
> +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +     pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> +              ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> +#else
> +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +
> +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu, pcsr_lo);
> +#endif
> +
> +     CS_LOCK(drvdata->base);
> +}

Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
Counter Sample Register":

        Implemented only if the OPTIONAL PC Sample-based Profiling
        Extension is implemented.

So even if we have access to an MMIO debug interface, we cannot
necessarily acecess this register.

[...]

> +/*
> + * Dump out memory limit information on panic.
> + */
> +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> +{
> +     int i;
> +
> +     pr_emerg("Coresight debug module:\n");
> +
> +     for_each_possible_cpu(i) {
> +
> +             if (!debug_drvdata[i])
> +                     continue;
> +
> +             debug_read_pcsr(debug_drvdata[i]);
> +     }

Is there no potential for deadlock with a CPU reading its own debug
interface registers?

[...]

> +static struct amba_id debug_ids[] = {
> +     {       /* Debug for Cortex-A53 */
> +             .id     = 0x000bbd03,
> +             .mask   = 0x000fffff,
> +             .data   = "debug",
> +     },
> +     { 0, 0},
> +};

The DT binding said nothing about Cortex-A53.

How variable are the MMIO registers in practice? Do we need to know the
particular CPU?

Thanks,
Mark.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Mark Rutland]

On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> the debug registers in the chapter "H9: External Debug Register
> Descriptions".

This should have been in the binding description also.

The layout of the ARM ARM can change over time, so please refer to the
full document number, which can be found at the bottom of each page
(e.g. ARM DDI 0487A.j).

> After enable the debug module we can check CPU state and PC value, etc.
> So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> into infinite loop with IRQ disabled. So the CPU cannot switch context
> and handle any interrupt, so it cannot handle SMP call for stack dump,
> etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> firmware and cannot return back to kernel.

I would generally expect that the secure world would lock down
debugging, as this poses a security risk.

I take it that this is only unlocked on development firmware.

Given that cores can be powered down outside of our control, I'm not
sure that accesses to these registers is safe in general.

> This patch is to enable coresight debug module and register callback
> notifier for panic; so when system detect the CPU lockup we can utilize
> debug module registers to get to know PC value for all CPUs; so we can
> quickly know the hang address for CPUs.
>
> This is initial driver for coresight debug module and could enhance it
> later according to debugging requirement.

How does this interact with an external debugger making use of these
registers?

[...]

> +static struct debug_drvdata *debug_drvdata[NR_CPUS];

A per-cpu variable is preferred to an NR_CPUS sized array.

> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> +     /* Unlocks the debug registers */
> +     writel_relaxed(0x0, drvdata->base + EDOSLAR);
> +     isb();
> +}

I do not believe this barrier is correct.

[...]

> +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> +{
> +     u32 pcsr_hi, pcsr_lo;
> +
> +     CS_UNLOCK(drvdata->base);
> +
> +     debug_os_unlock(drvdata);
> +
> +#ifdef CONFIG_64BIT
> +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +     pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> +              ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> +#else
> +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +
> +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu, pcsr_lo);
> +#endif
> +
> +     CS_LOCK(drvdata->base);
> +}

Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
Counter Sample Register":

        Implemented only if the OPTIONAL PC Sample-based Profiling
        Extension is implemented.

So even if we have access to an MMIO debug interface, we cannot
necessarily acecess this register.

[...]

> +/*
> + * Dump out memory limit information on panic.
> + */
> +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> +{
> +     int i;
> +
> +     pr_emerg("Coresight debug module:\n");
> +
> +     for_each_possible_cpu(i) {
> +
> +             if (!debug_drvdata[i])
> +                     continue;
> +
> +             debug_read_pcsr(debug_drvdata[i]);
> +     }

Is there no potential for deadlock with a CPU reading its own debug
interface registers?

[...]

> +static struct amba_id debug_ids[] = {
> +     {       /* Debug for Cortex-A53 */
> +             .id     = 0x000bbd03,
> +             .mask   = 0x000fffff,
> +             .data   = "debug",
> +     },
> +     { 0, 0},
> +};

The DT binding said nothing about Cortex-A53.

How variable are the MMIO registers in practice? Do we need to know the
particular CPU?

Thanks,
Mark.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH RFC 2/3] coresight: add support for debug module

[Mark Rutland]

On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> the debug registers in the chapter "H9: External Debug Register
> Descriptions".

This should have been in the binding description also.

The layout of the ARM ARM can change over time, so please refer to the
full document number, which can be found at the bottom of each page
(e.g. ARM DDI 0487A.j).

> After enable the debug module we can check CPU state and PC value, etc.
> So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> into infinite loop with IRQ disabled. So the CPU cannot switch context
> and handle any interrupt, so it cannot handle SMP call for stack dump,
> etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> firmware and cannot return back to kernel.

I would generally expect that the secure world would lock down
debugging, as this poses a security risk.

I take it that this is only unlocked on development firmware.

Given that cores can be powered down outside of our control, I'm not
sure that accesses to these registers is safe in general.

> This patch is to enable coresight debug module and register callback
> notifier for panic; so when system detect the CPU lockup we can utilize
> debug module registers to get to know PC value for all CPUs; so we can
> quickly know the hang address for CPUs.
>
> This is initial driver for coresight debug module and could enhance it
> later according to debugging requirement.

How does this interact with an external debugger making use of these
registers?

[...]

> +static struct debug_drvdata *debug_drvdata[NR_CPUS];

A per-cpu variable is preferred to an NR_CPUS sized array.

> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> +     /* Unlocks the debug registers */
> +     writel_relaxed(0x0, drvdata->base + EDOSLAR);
> +     isb();
> +}

I do not believe this barrier is correct.

[...]

> +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> +{
> +     u32 pcsr_hi, pcsr_lo;
> +
> +     CS_UNLOCK(drvdata->base);
> +
> +     debug_os_unlock(drvdata);
> +
> +#ifdef CONFIG_64BIT
> +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +     pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> +              ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> +#else
> +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +
> +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu, pcsr_lo);
> +#endif
> +
> +     CS_LOCK(drvdata->base);
> +}

Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
Counter Sample Register":

        Implemented only if the OPTIONAL PC Sample-based Profiling
        Extension is implemented.

So even if we have access to an MMIO debug interface, we cannot
necessarily acecess this register.

[...]

> +/*
> + * Dump out memory limit information on panic.
> + */
> +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> +{
> +     int i;
> +
> +     pr_emerg("Coresight debug module:\n");
> +
> +     for_each_possible_cpu(i) {
> +
> +             if (!debug_drvdata[i])
> +                     continue;
> +
> +             debug_read_pcsr(debug_drvdata[i]);
> +     }

Is there no potential for deadlock with a CPU reading its own debug
interface registers?

[...]

> +static struct amba_id debug_ids[] = {
> +     {       /* Debug for Cortex-A53 */
> +             .id     = 0x000bbd03,
> +             .mask   = 0x000fffff,
> +             .data   = "debug",
> +     },
> +     { 0, 0},
> +};

The DT binding said nothing about Cortex-A53.

How variable are the MMIO registers in practice? Do we need to know the
particular CPU?

Thanks,
Mark.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Mark Rutland]

[resending due to a mail server snafu]

On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> the debug registers in the chapter "H9: External Debug Register
> Descriptions".

This should have been in the binding description also.

The layout of the ARM ARM can change over time, so please refer to the
full document number, which can be found at the bottom of each page
(e.g. ARM DDI 0487A.j).

> After enable the debug module we can check CPU state and PC value, etc.
> So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> into infinite loop with IRQ disabled. So the CPU cannot switch context
> and handle any interrupt, so it cannot handle SMP call for stack dump,
> etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> firmware and cannot return back to kernel.

I would generally expect that the secure world would lock down
debugging, as this poses a security risk.

I take it that this is only unlocked on development firmware.

Given that cores can be powered down outside of our control, I'm not
sure that accesses to these registers is safe in general.

> This patch is to enable coresight debug module and register callback
> notifier for panic; so when system detect the CPU lockup we can utilize
> debug module registers to get to know PC value for all CPUs; so we can
> quickly know the hang address for CPUs.
> 
> This is initial driver for coresight debug module and could enhance it
> later according to debugging requirement.

How does this interact with an external debugger making use of these
registers?

[...]

> +static struct debug_drvdata *debug_drvdata[NR_CPUS];

A per-cpu variable is preferred to an NR_CPUS sized array.

> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> +	/* Unlocks the debug registers */
> +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> +	isb();
> +}

I do not believe this barrier is correct.

[...]

> +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> +{
> +	u32 pcsr_hi, pcsr_lo;
> +
> +	CS_UNLOCK(drvdata->base);
> +
> +	debug_os_unlock(drvdata);
> +
> +#ifdef CONFIG_64BIT
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> +#else
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> +#endif
> +
> +	CS_LOCK(drvdata->base);
> +}

Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
Counter Sample Register":

	Implemented only if the OPTIONAL PC Sample-based Profiling
	Extension is implemented.

So even if we have access to an MMIO debug interface, we cannot
necessarily acecess this register.

[...]

> +/*
> + * Dump out memory limit information on panic.
> + */
> +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> +{
> +	int i;
> +
> +	pr_emerg("Coresight debug module:\n");
> +
> +	for_each_possible_cpu(i) {
> +
> +		if (!debug_drvdata[i])
> +			continue;
> +
> +		debug_read_pcsr(debug_drvdata[i]);
> +	}

Is there no potential for deadlock with a CPU reading its own debug
interface registers?

[...]

> +static struct amba_id debug_ids[] = {
> +	{       /* Debug for Cortex-A53 */
> +		.id	= 0x000bbd03,
> +		.mask	= 0x000fffff,
> +		.data	= "debug",
> +	},
> +	{ 0, 0},
> +};

The DT binding said nothing about Cortex-A53.

How variable are the MMIO registers in practice? Do we need to know the
particular CPU?

Thanks,
Mark.

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Mark Rutland]

[resending due to a mail server snafu]

On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> the debug registers in the chapter "H9: External Debug Register
> Descriptions".

This should have been in the binding description also.

The layout of the ARM ARM can change over time, so please refer to the
full document number, which can be found at the bottom of each page
(e.g. ARM DDI 0487A.j).

> After enable the debug module we can check CPU state and PC value, etc.
> So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> into infinite loop with IRQ disabled. So the CPU cannot switch context
> and handle any interrupt, so it cannot handle SMP call for stack dump,
> etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> firmware and cannot return back to kernel.

I would generally expect that the secure world would lock down
debugging, as this poses a security risk.

I take it that this is only unlocked on development firmware.

Given that cores can be powered down outside of our control, I'm not
sure that accesses to these registers is safe in general.

> This patch is to enable coresight debug module and register callback
> notifier for panic; so when system detect the CPU lockup we can utilize
> debug module registers to get to know PC value for all CPUs; so we can
> quickly know the hang address for CPUs.
> 
> This is initial driver for coresight debug module and could enhance it
> later according to debugging requirement.

How does this interact with an external debugger making use of these
registers?

[...]

> +static struct debug_drvdata *debug_drvdata[NR_CPUS];

A per-cpu variable is preferred to an NR_CPUS sized array.

> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> +	/* Unlocks the debug registers */
> +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> +	isb();
> +}

I do not believe this barrier is correct.

[...]

> +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> +{
> +	u32 pcsr_hi, pcsr_lo;
> +
> +	CS_UNLOCK(drvdata->base);
> +
> +	debug_os_unlock(drvdata);
> +
> +#ifdef CONFIG_64BIT
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> +#else
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> +#endif
> +
> +	CS_LOCK(drvdata->base);
> +}

Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
Counter Sample Register":

	Implemented only if the OPTIONAL PC Sample-based Profiling
	Extension is implemented.

So even if we have access to an MMIO debug interface, we cannot
necessarily acecess this register.

[...]

> +/*
> + * Dump out memory limit information on panic.
> + */
> +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> +{
> +	int i;
> +
> +	pr_emerg("Coresight debug module:\n");
> +
> +	for_each_possible_cpu(i) {
> +
> +		if (!debug_drvdata[i])
> +			continue;
> +
> +		debug_read_pcsr(debug_drvdata[i]);
> +	}

Is there no potential for deadlock with a CPU reading its own debug
interface registers?

[...]

> +static struct amba_id debug_ids[] = {
> +	{       /* Debug for Cortex-A53 */
> +		.id	= 0x000bbd03,
> +		.mask	= 0x000fffff,
> +		.data	= "debug",
> +	},
> +	{ 0, 0},
> +};

The DT binding said nothing about Cortex-A53.

How variable are the MMIO registers in practice? Do we need to know the
particular CPU?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/3] coresight: add support for debug module

[Mark Rutland]

[resending due to a mail server snafu]

On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> the debug registers in the chapter "H9: External Debug Register
> Descriptions".

This should have been in the binding description also.

The layout of the ARM ARM can change over time, so please refer to the
full document number, which can be found at the bottom of each page
(e.g. ARM DDI 0487A.j).

> After enable the debug module we can check CPU state and PC value, etc.
> So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> into infinite loop with IRQ disabled. So the CPU cannot switch context
> and handle any interrupt, so it cannot handle SMP call for stack dump,
> etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> firmware and cannot return back to kernel.

I would generally expect that the secure world would lock down
debugging, as this poses a security risk.

I take it that this is only unlocked on development firmware.

Given that cores can be powered down outside of our control, I'm not
sure that accesses to these registers is safe in general.

> This patch is to enable coresight debug module and register callback
> notifier for panic; so when system detect the CPU lockup we can utilize
> debug module registers to get to know PC value for all CPUs; so we can
> quickly know the hang address for CPUs.
> 
> This is initial driver for coresight debug module and could enhance it
> later according to debugging requirement.

How does this interact with an external debugger making use of these
registers?

[...]

> +static struct debug_drvdata *debug_drvdata[NR_CPUS];

A per-cpu variable is preferred to an NR_CPUS sized array.

> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> +	/* Unlocks the debug registers */
> +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> +	isb();
> +}

I do not believe this barrier is correct.

[...]

> +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> +{
> +	u32 pcsr_hi, pcsr_lo;
> +
> +	CS_UNLOCK(drvdata->base);
> +
> +	debug_os_unlock(drvdata);
> +
> +#ifdef CONFIG_64BIT
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> +#else
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> +#endif
> +
> +	CS_LOCK(drvdata->base);
> +}

Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
Counter Sample Register":

	Implemented only if the OPTIONAL PC Sample-based Profiling
	Extension is implemented.

So even if we have access to an MMIO debug interface, we cannot
necessarily acecess this register.

[...]

> +/*
> + * Dump out memory limit information on panic.
> + */
> +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> +{
> +	int i;
> +
> +	pr_emerg("Coresight debug module:\n");
> +
> +	for_each_possible_cpu(i) {
> +
> +		if (!debug_drvdata[i])
> +			continue;
> +
> +		debug_read_pcsr(debug_drvdata[i]);
> +	}

Is there no potential for deadlock with a CPU reading its own debug
interface registers?

[...]

> +static struct amba_id debug_ids[] = {
> +	{       /* Debug for Cortex-A53 */
> +		.id	= 0x000bbd03,
> +		.mask	= 0x000fffff,
> +		.data	= "debug",
> +	},
> +	{ 0, 0},
> +};

The DT binding said nothing about Cortex-A53.

How variable are the MMIO registers in practice? Do we need to know the
particular CPU?

Thanks,
Mark.

Re: [PATCH RFC 1/3] coresight: binding for coresight debug driver

[Mathieu Poirier]

On Mon, Feb 13, 2017 at 02:11:36PM +0800, Leo Yan wrote:
> Adding compatible string for new coresight debug driver.
> 

Hi Leo,

I agree with Mark, this will need a better description.

> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index fcbae6a..3ff15fd 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -40,6 +40,9 @@ its hardware characteristcs.
>  		- System Trace Macrocell:
>  			"arm,coresight-stm", "arm,primecell"; [1]
>  
> +		- Debug Unit:
> +			"arm,coresight-debug", "arm,primecell";
> +

Humm...  The current CoreSight bindings are meant to describe IPs included in
the HW assisted trace architecture.  This new driver, althought considered to be
part of the CoreSight umbrella, falls under the debugging domain.

Adding the bindings in this file may lead people to beleive this driver fits
into the CoreSight framework currently supported, which isn't the case.

As such it is probably a good idea to spin off a new file, "coresight-debug.txt"
to handle this driver. 

Mark, what's your take on this?

>  	* reg: physical base address and length of the register
>  	  set(s) of the component.
>  
> @@ -78,8 +81,10 @@ its hardware characteristcs.
>  	* arm,cp14: must be present if the system accesses ETM/PTM management
>  	  registers via co-processor 14.
>  
> -	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
> -	  source is considered to belong to CPU0.
> +* Optional properties for ETM/PTM/Debugs:
> +
> +	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
> +	  the source is considered to belong to CPU0.
>  
>  * Optional property for TMC:
>  
> -- 
> 2.7.4
> 

Re: [PATCH RFC 1/3] coresight: binding for coresight debug driver

[Mathieu Poirier]

On Mon, Feb 13, 2017 at 02:11:36PM +0800, Leo Yan wrote:
> Adding compatible string for new coresight debug driver.
> 

Hi Leo,

I agree with Mark, this will need a better description.

> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index fcbae6a..3ff15fd 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -40,6 +40,9 @@ its hardware characteristcs.
>  		- System Trace Macrocell:
>  			"arm,coresight-stm", "arm,primecell"; [1]
>  
> +		- Debug Unit:
> +			"arm,coresight-debug", "arm,primecell";
> +

Humm...  The current CoreSight bindings are meant to describe IPs included in
the HW assisted trace architecture.  This new driver, althought considered to be
part of the CoreSight umbrella, falls under the debugging domain.

Adding the bindings in this file may lead people to beleive this driver fits
into the CoreSight framework currently supported, which isn't the case.

As such it is probably a good idea to spin off a new file, "coresight-debug.txt"
to handle this driver. 

Mark, what's your take on this?

>  	* reg: physical base address and length of the register
>  	  set(s) of the component.
>  
> @@ -78,8 +81,10 @@ its hardware characteristcs.
>  	* arm,cp14: must be present if the system accesses ETM/PTM management
>  	  registers via co-processor 14.
>  
> -	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
> -	  source is considered to belong to CPU0.
> +* Optional properties for ETM/PTM/Debugs:
> +
> +	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
> +	  the source is considered to belong to CPU0.
>  
>  * Optional property for TMC:
>  
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 1/3] coresight: binding for coresight debug driver

[Mathieu Poirier]

On Mon, Feb 13, 2017 at 02:11:36PM +0800, Leo Yan wrote:
> Adding compatible string for new coresight debug driver.
> 

Hi Leo,

I agree with Mark, this will need a better description.

> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index fcbae6a..3ff15fd 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -40,6 +40,9 @@ its hardware characteristcs.
>  		- System Trace Macrocell:
>  			"arm,coresight-stm", "arm,primecell"; [1]
>  
> +		- Debug Unit:
> +			"arm,coresight-debug", "arm,primecell";
> +

Humm...  The current CoreSight bindings are meant to describe IPs included in
the HW assisted trace architecture.  This new driver, althought considered to be
part of the CoreSight umbrella, falls under the debugging domain.

Adding the bindings in this file may lead people to beleive this driver fits
into the CoreSight framework currently supported, which isn't the case.

As such it is probably a good idea to spin off a new file, "coresight-debug.txt"
to handle this driver. 

Mark, what's your take on this?

>  	* reg: physical base address and length of the register
>  	  set(s) of the component.
>  
> @@ -78,8 +81,10 @@ its hardware characteristcs.
>  	* arm,cp14: must be present if the system accesses ETM/PTM management
>  	  registers via co-processor 14.
>  
> -	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
> -	  source is considered to belong to CPU0.
> +* Optional properties for ETM/PTM/Debugs:
> +
> +	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
> +	  the source is considered to belong to CPU0.
>  
>  * Optional property for TMC:
>  
> -- 
> 2.7.4
> 

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Mathieu Poirier]

On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> the debug registers in the chapter "H9: External Debug Register
> Descriptions".
> 
> After enable the debug module we can check CPU state and PC value, etc.
> So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> into infinite loop with IRQ disabled. So the CPU cannot switch context
> and handle any interrupt, so it cannot handle SMP call for stack dump,
> etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> firmware and cannot return back to kernel.
> 
> This patch is to enable coresight debug module and register callback
> notifier for panic; so when system detect the CPU lockup we can utilize
> debug module registers to get to know PC value for all CPUs; so we can
> quickly know the hang address for CPUs.
> 
> This is initial driver for coresight debug module and could enhance it
> later according to debugging requirement.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  drivers/hwtracing/coresight/Kconfig           |   8 ++
>  drivers/hwtracing/coresight/Makefile          |   1 +
>  drivers/hwtracing/coresight/coresight-debug.c | 169 ++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..dcf59cc 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,12 @@ config CORESIGHT_STM
>  	  logging useful software events or data coming from various entities
>  	  in the system, possibly running different OSs
>  
> +config CORESIGHT_DEBUG
> +	bool "CoreSight debug driver"
> +	depends on ARM || ARM64
> +	help
> +	  This driver provides support for coresight debugging module. This
> +	  is primarily used for printing out debug registers for panic and
> +	  soft and hard lockup.
> +
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index af480d9..d540d45 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
>  					coresight-etm4x-sysfs.o
>  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
>  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> +obj-$(CONFIG_CORESIGHT_DEBUG) += coresight-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-debug.c b/drivers/hwtracing/coresight/coresight-debug.c
> new file mode 100644
> index 0000000..28206a83
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-debug.c
> @@ -0,0 +1,169 @@
> +/*
> + * Copyright(C) 2017 Linaro Limited. All rights reserved.
> + * Author: Leo Yan <leo.yan@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/moduleparam.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/smp.h>
> +#include <linux/sysfs.h>
> +#include <linux/stat.h>
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/coresight.h>
> +#include <linux/amba/bus.h>
> +#include <linux/uaccess.h>

When possible, please try to order the header files alphabetically.

> +
> +#include "coresight-priv.h"
> +
> +#define EDPCSR_LO	0x0A0
> +#define EDPCSR_HI	0x0AC
> +#define EDOSLAR		0x300
> +#define EDOSLSR		0x304
> +#define EDPDCR		0x310
> +#define EDPDSR		0x314

EDOSLSR, DEPDCR and EDPDSR aren't used in the code presented below.

> +
> +struct debug_drvdata {
> +	void __iomem	*base;
> +	struct device	*dev;
> +	int		cpu;
> +};
> +
> +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> +	/* Unlocks the debug registers */
> +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> +	isb();
> +}
> +
> +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> +{
> +	u32 pcsr_hi, pcsr_lo;
> +
> +	CS_UNLOCK(drvdata->base);
> +
> +	debug_os_unlock(drvdata);
> +
> +#ifdef CONFIG_64BIT
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> +#else
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> +#endif
> +
> +	CS_LOCK(drvdata->base);
> +}
> +
> +/*
> + * Dump out memory limit information on panic.
> + */
> +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> +{
> +	int i;
> +
> +	pr_emerg("Coresight debug module:\n");
> +
> +	for_each_possible_cpu(i) {
> +
> +		if (!debug_drvdata[i])
> +			continue;
> +
> +		debug_read_pcsr(debug_drvdata[i]);

Mark and Mike have raised a number of problems with this.  If I get things
correctly the pseudocode (CreatePCSample() and EDPCSRlo[]), found in the
reference manual, should be dealing with most of them. 

One thing that is subtle in the peudocode is the memory-mapped access to these
registers and something else that should be checked here before proceeding. 

> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block debug_notifier = {
> +	.notifier_call = dump_debug,
> +};
> +
> +static int __init register_coresight_debug_dumper(void)
> +{
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &debug_notifier);
> +	return 0;
> +}
> +__initcall(register_coresight_debug_dumper);
> +
> +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> +	void __iomem *base;
> +	struct device *dev = &adev->dev;
> +	struct coresight_platform_data *pdata = NULL;
> +	struct debug_drvdata *drvdata;
> +	struct resource *res = &adev->res;
> +	struct device_node *np = adev->dev.of_node;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	if (np) {
> +		pdata = of_get_coresight_platform_data(dev, np);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);
> +		adev->dev.platform_data = pdata;
> +	}
> +
> +	drvdata->dev = &adev->dev;
> +	dev_set_drvdata(dev, drvdata);
> +
> +	/* Validity for the resource is already checked by the AMBA core */
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	drvdata->base = base;
> +	drvdata->cpu = pdata ? pdata->cpu : 0;
> +	debug_drvdata[drvdata->cpu] = drvdata;

Add atomic_notifier_chain_register() here rather than letting the infrastructure
deal with it in an initcall.  That way we don't clutter the panic_notifier_list
needlessly if the IP is not present.

Thanks,
Mathieu

> +
> +	dev_info(dev, "%s initialized\n", (char *)id->data);
> +	return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> +	{       /* Debug for Cortex-A53 */
> +		.id	= 0x000bbd03,
> +		.mask	= 0x000fffff,
> +		.data	= "debug",
> +	},
> +	{ 0, 0},
> +};
> +
> +static struct amba_driver debug_driver = {
> +	.drv = {
> +		.name   = "coresight-debug",
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe		= debug_probe,
> +	.id_table	= debug_ids,
> +};
> +builtin_amba_driver(debug_driver);
> -- 
> 2.7.4
> 

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Mathieu Poirier]

On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> the debug registers in the chapter "H9: External Debug Register
> Descriptions".
> 
> After enable the debug module we can check CPU state and PC value, etc.
> So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> into infinite loop with IRQ disabled. So the CPU cannot switch context
> and handle any interrupt, so it cannot handle SMP call for stack dump,
> etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> firmware and cannot return back to kernel.
> 
> This patch is to enable coresight debug module and register callback
> notifier for panic; so when system detect the CPU lockup we can utilize
> debug module registers to get to know PC value for all CPUs; so we can
> quickly know the hang address for CPUs.
> 
> This is initial driver for coresight debug module and could enhance it
> later according to debugging requirement.
> 
> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/hwtracing/coresight/Kconfig           |   8 ++
>  drivers/hwtracing/coresight/Makefile          |   1 +
>  drivers/hwtracing/coresight/coresight-debug.c | 169 ++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..dcf59cc 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,12 @@ config CORESIGHT_STM
>  	  logging useful software events or data coming from various entities
>  	  in the system, possibly running different OSs
>  
> +config CORESIGHT_DEBUG
> +	bool "CoreSight debug driver"
> +	depends on ARM || ARM64
> +	help
> +	  This driver provides support for coresight debugging module. This
> +	  is primarily used for printing out debug registers for panic and
> +	  soft and hard lockup.
> +
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index af480d9..d540d45 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
>  					coresight-etm4x-sysfs.o
>  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
>  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> +obj-$(CONFIG_CORESIGHT_DEBUG) += coresight-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-debug.c b/drivers/hwtracing/coresight/coresight-debug.c
> new file mode 100644
> index 0000000..28206a83
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-debug.c
> @@ -0,0 +1,169 @@
> +/*
> + * Copyright(C) 2017 Linaro Limited. All rights reserved.
> + * Author: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/moduleparam.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/smp.h>
> +#include <linux/sysfs.h>
> +#include <linux/stat.h>
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/coresight.h>
> +#include <linux/amba/bus.h>
> +#include <linux/uaccess.h>

When possible, please try to order the header files alphabetically.

> +
> +#include "coresight-priv.h"
> +
> +#define EDPCSR_LO	0x0A0
> +#define EDPCSR_HI	0x0AC
> +#define EDOSLAR		0x300
> +#define EDOSLSR		0x304
> +#define EDPDCR		0x310
> +#define EDPDSR		0x314

EDOSLSR, DEPDCR and EDPDSR aren't used in the code presented below.

> +
> +struct debug_drvdata {
> +	void __iomem	*base;
> +	struct device	*dev;
> +	int		cpu;
> +};
> +
> +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> +	/* Unlocks the debug registers */
> +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> +	isb();
> +}
> +
> +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> +{
> +	u32 pcsr_hi, pcsr_lo;
> +
> +	CS_UNLOCK(drvdata->base);
> +
> +	debug_os_unlock(drvdata);
> +
> +#ifdef CONFIG_64BIT
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> +#else
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> +#endif
> +
> +	CS_LOCK(drvdata->base);
> +}
> +
> +/*
> + * Dump out memory limit information on panic.
> + */
> +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> +{
> +	int i;
> +
> +	pr_emerg("Coresight debug module:\n");
> +
> +	for_each_possible_cpu(i) {
> +
> +		if (!debug_drvdata[i])
> +			continue;
> +
> +		debug_read_pcsr(debug_drvdata[i]);

Mark and Mike have raised a number of problems with this.  If I get things
correctly the pseudocode (CreatePCSample() and EDPCSRlo[]), found in the
reference manual, should be dealing with most of them. 

One thing that is subtle in the peudocode is the memory-mapped access to these
registers and something else that should be checked here before proceeding. 

> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block debug_notifier = {
> +	.notifier_call = dump_debug,
> +};
> +
> +static int __init register_coresight_debug_dumper(void)
> +{
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &debug_notifier);
> +	return 0;
> +}
> +__initcall(register_coresight_debug_dumper);
> +
> +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> +	void __iomem *base;
> +	struct device *dev = &adev->dev;
> +	struct coresight_platform_data *pdata = NULL;
> +	struct debug_drvdata *drvdata;
> +	struct resource *res = &adev->res;
> +	struct device_node *np = adev->dev.of_node;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	if (np) {
> +		pdata = of_get_coresight_platform_data(dev, np);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);
> +		adev->dev.platform_data = pdata;
> +	}
> +
> +	drvdata->dev = &adev->dev;
> +	dev_set_drvdata(dev, drvdata);
> +
> +	/* Validity for the resource is already checked by the AMBA core */
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	drvdata->base = base;
> +	drvdata->cpu = pdata ? pdata->cpu : 0;
> +	debug_drvdata[drvdata->cpu] = drvdata;

Add atomic_notifier_chain_register() here rather than letting the infrastructure
deal with it in an initcall.  That way we don't clutter the panic_notifier_list
needlessly if the IP is not present.

Thanks,
Mathieu

> +
> +	dev_info(dev, "%s initialized\n", (char *)id->data);
> +	return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> +	{       /* Debug for Cortex-A53 */
> +		.id	= 0x000bbd03,
> +		.mask	= 0x000fffff,
> +		.data	= "debug",
> +	},
> +	{ 0, 0},
> +};
> +
> +static struct amba_driver debug_driver = {
> +	.drv = {
> +		.name   = "coresight-debug",
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe		= debug_probe,
> +	.id_table	= debug_ids,
> +};
> +builtin_amba_driver(debug_driver);
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/3] coresight: add support for debug module

[Mathieu Poirier]

On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> the debug registers in the chapter "H9: External Debug Register
> Descriptions".
> 
> After enable the debug module we can check CPU state and PC value, etc.
> So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> into infinite loop with IRQ disabled. So the CPU cannot switch context
> and handle any interrupt, so it cannot handle SMP call for stack dump,
> etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> firmware and cannot return back to kernel.
> 
> This patch is to enable coresight debug module and register callback
> notifier for panic; so when system detect the CPU lockup we can utilize
> debug module registers to get to know PC value for all CPUs; so we can
> quickly know the hang address for CPUs.
> 
> This is initial driver for coresight debug module and could enhance it
> later according to debugging requirement.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  drivers/hwtracing/coresight/Kconfig           |   8 ++
>  drivers/hwtracing/coresight/Makefile          |   1 +
>  drivers/hwtracing/coresight/coresight-debug.c | 169 ++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+)
>  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..dcf59cc 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,12 @@ config CORESIGHT_STM
>  	  logging useful software events or data coming from various entities
>  	  in the system, possibly running different OSs
>  
> +config CORESIGHT_DEBUG
> +	bool "CoreSight debug driver"
> +	depends on ARM || ARM64
> +	help
> +	  This driver provides support for coresight debugging module. This
> +	  is primarily used for printing out debug registers for panic and
> +	  soft and hard lockup.
> +
>  endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index af480d9..d540d45 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
>  					coresight-etm4x-sysfs.o
>  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
>  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> +obj-$(CONFIG_CORESIGHT_DEBUG) += coresight-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-debug.c b/drivers/hwtracing/coresight/coresight-debug.c
> new file mode 100644
> index 0000000..28206a83
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-debug.c
> @@ -0,0 +1,169 @@
> +/*
> + * Copyright(C) 2017 Linaro Limited. All rights reserved.
> + * Author: Leo Yan <leo.yan@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/moduleparam.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/smp.h>
> +#include <linux/sysfs.h>
> +#include <linux/stat.h>
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/coresight.h>
> +#include <linux/amba/bus.h>
> +#include <linux/uaccess.h>

When possible, please try to order the header files alphabetically.

> +
> +#include "coresight-priv.h"
> +
> +#define EDPCSR_LO	0x0A0
> +#define EDPCSR_HI	0x0AC
> +#define EDOSLAR		0x300
> +#define EDOSLSR		0x304
> +#define EDPDCR		0x310
> +#define EDPDSR		0x314

EDOSLSR, DEPDCR and EDPDSR aren't used in the code presented below.

> +
> +struct debug_drvdata {
> +	void __iomem	*base;
> +	struct device	*dev;
> +	int		cpu;
> +};
> +
> +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> +	/* Unlocks the debug registers */
> +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> +	isb();
> +}
> +
> +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> +{
> +	u32 pcsr_hi, pcsr_lo;
> +
> +	CS_UNLOCK(drvdata->base);
> +
> +	debug_os_unlock(drvdata);
> +
> +#ifdef CONFIG_64BIT
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> +#else
> +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> +
> +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> +#endif
> +
> +	CS_LOCK(drvdata->base);
> +}
> +
> +/*
> + * Dump out memory limit information on panic.
> + */
> +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> +{
> +	int i;
> +
> +	pr_emerg("Coresight debug module:\n");
> +
> +	for_each_possible_cpu(i) {
> +
> +		if (!debug_drvdata[i])
> +			continue;
> +
> +		debug_read_pcsr(debug_drvdata[i]);

Mark and Mike have raised a number of problems with this.  If I get things
correctly the pseudocode (CreatePCSample() and EDPCSRlo[]), found in the
reference manual, should be dealing with most of them. 

One thing that is subtle in the peudocode is the memory-mapped access to these
registers and something else that should be checked here before proceeding. 

> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block debug_notifier = {
> +	.notifier_call = dump_debug,
> +};
> +
> +static int __init register_coresight_debug_dumper(void)
> +{
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &debug_notifier);
> +	return 0;
> +}
> +__initcall(register_coresight_debug_dumper);
> +
> +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> +	void __iomem *base;
> +	struct device *dev = &adev->dev;
> +	struct coresight_platform_data *pdata = NULL;
> +	struct debug_drvdata *drvdata;
> +	struct resource *res = &adev->res;
> +	struct device_node *np = adev->dev.of_node;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	if (np) {
> +		pdata = of_get_coresight_platform_data(dev, np);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);
> +		adev->dev.platform_data = pdata;
> +	}
> +
> +	drvdata->dev = &adev->dev;
> +	dev_set_drvdata(dev, drvdata);
> +
> +	/* Validity for the resource is already checked by the AMBA core */
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	drvdata->base = base;
> +	drvdata->cpu = pdata ? pdata->cpu : 0;
> +	debug_drvdata[drvdata->cpu] = drvdata;

Add atomic_notifier_chain_register() here rather than letting the infrastructure
deal with it in an initcall.  That way we don't clutter the panic_notifier_list
needlessly if the IP is not present.

Thanks,
Mathieu

> +
> +	dev_info(dev, "%s initialized\n", (char *)id->data);
> +	return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> +	{       /* Debug for Cortex-A53 */
> +		.id	= 0x000bbd03,
> +		.mask	= 0x000fffff,
> +		.data	= "debug",
> +	},
> +	{ 0, 0},
> +};
> +
> +static struct amba_driver debug_driver = {
> +	.drv = {
> +		.name   = "coresight-debug",
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe		= debug_probe,
> +	.id_table	= debug_ids,
> +};
> +builtin_amba_driver(debug_driver);
> -- 
> 2.7.4
> 

Re: [PATCH RFC 3/3] arm64: dts: register Hi6220's coresight debug module

[Mathieu Poirier]

On Mon, Feb 13, 2017 at 02:11:38PM +0800, Leo Yan wrote:
> Bind coresight debug driver for Hi6220.

Bindings for the coresight debug driver...

> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
> index 77c2aab..e14d75c 100644
> --- a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
> @@ -15,6 +15,79 @@
>  		#size-cells = <2>;
>  		compatible = "arm,amba-bus";
>  		ranges;
> +
> +		debug@0,f6590000 {

Simply use "debug@f6590000", the "0," isn't required.

> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6590000 0 0x1000>;
> +			default_enable;

What is the "default_enable" for ?

> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu0>;
> +		};
> +
> +		debug@1,f6592000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6592000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu1>;
> +		};
> +
> +		debug@2,f6594000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6594000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu2>;
> +		};
> +
> +		debug@3,f6596000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6596000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu3>;
> +		};
> +
> +		debug@4,f65d0000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d0000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu4>;
> +		};
> +
> +		debug@5,f65d2000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d2000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu5>;
> +		};
> +
> +		debug@6,f65d4000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d4000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu6>;
> +		};
> +
> +		debug@7,f65d6000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d6000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu7>;
> +		};
> +
>  		etm@0,f659c000 {
>  			compatible = "arm,coresight-etm4x","arm,primecell";
>  			reg = <0 0xf659c000 0 0x1000>;
> -- 
> 2.7.4
> 

Re: [PATCH RFC 3/3] arm64: dts: register Hi6220's coresight debug module

[Mathieu Poirier]

On Mon, Feb 13, 2017 at 02:11:38PM +0800, Leo Yan wrote:
> Bind coresight debug driver for Hi6220.

Bindings for the coresight debug driver...

> 
> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
> index 77c2aab..e14d75c 100644
> --- a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
> @@ -15,6 +15,79 @@
>  		#size-cells = <2>;
>  		compatible = "arm,amba-bus";
>  		ranges;
> +
> +		debug@0,f6590000 {

Simply use "debug@f6590000", the "0," isn't required.

> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6590000 0 0x1000>;
> +			default_enable;

What is the "default_enable" for ?

> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu0>;
> +		};
> +
> +		debug@1,f6592000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6592000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu1>;
> +		};
> +
> +		debug@2,f6594000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6594000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu2>;
> +		};
> +
> +		debug@3,f6596000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6596000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu3>;
> +		};
> +
> +		debug@4,f65d0000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d0000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu4>;
> +		};
> +
> +		debug@5,f65d2000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d2000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu5>;
> +		};
> +
> +		debug@6,f65d4000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d4000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu6>;
> +		};
> +
> +		debug@7,f65d6000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d6000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu7>;
> +		};
> +
>  		etm@0,f659c000 {
>  			compatible = "arm,coresight-etm4x","arm,primecell";
>  			reg = <0 0xf659c000 0 0x1000>;
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 3/3] arm64: dts: register Hi6220's coresight debug module

[Mathieu Poirier]

On Mon, Feb 13, 2017 at 02:11:38PM +0800, Leo Yan wrote:
> Bind coresight debug driver for Hi6220.

Bindings for the coresight debug driver...

> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  .../boot/dts/hisilicon/hikey_6220_coresight.dtsi   | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
> index 77c2aab..e14d75c 100644
> --- a/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hikey_6220_coresight.dtsi
> @@ -15,6 +15,79 @@
>  		#size-cells = <2>;
>  		compatible = "arm,amba-bus";
>  		ranges;
> +
> +		debug at 0,f6590000 {

Simply use "debug at f6590000", the "0," isn't required.

> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6590000 0 0x1000>;
> +			default_enable;

What is the "default_enable" for ?

> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu0>;
> +		};
> +
> +		debug at 1,f6592000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6592000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu1>;
> +		};
> +
> +		debug at 2,f6594000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6594000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu2>;
> +		};
> +
> +		debug at 3,f6596000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf6596000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu3>;
> +		};
> +
> +		debug at 4,f65d0000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d0000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu4>;
> +		};
> +
> +		debug at 5,f65d2000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d2000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu5>;
> +		};
> +
> +		debug at 6,f65d4000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d4000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu6>;
> +		};
> +
> +		debug at 7,f65d6000 {
> +			compatible = "arm,coresight-debug","arm,primecell";
> +			reg = <0 0xf65d6000 0 0x1000>;
> +			default_enable;
> +			clocks = <&sys_ctrl HI6220_CS_ATB>;
> +			clock-names = "apb_pclk";
> +			cpu = <&cpu7>;
> +		};
> +
>  		etm at 0,f659c000 {
>  			compatible = "arm,coresight-etm4x","arm,primecell";
>  			reg = <0 0xf659c000 0 0x1000>;
> -- 
> 2.7.4
> 

Re: [PATCH RFC 1/3] coresight: binding for coresight debug driver

[Leo Yan]

On Wed, Feb 15, 2017 at 01:08:58PM -0700, Mathieu Poirier wrote:
> On Mon, Feb 13, 2017 at 02:11:36PM +0800, Leo Yan wrote:
> > Adding compatible string for new coresight debug driver.
> > 
> 
> Hi Leo,
> 
> I agree with Mark, this will need a better description.
> 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> > index fcbae6a..3ff15fd 100644
> > --- a/Documentation/devicetree/bindings/arm/coresight.txt
> > +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> > @@ -40,6 +40,9 @@ its hardware characteristcs.
> >  		- System Trace Macrocell:
> >  			"arm,coresight-stm", "arm,primecell"; [1]
> >  
> > +		- Debug Unit:
> > +			"arm,coresight-debug", "arm,primecell";
> > +
> 
> Humm...  The current CoreSight bindings are meant to describe IPs included in
> the HW assisted trace architecture.  This new driver, althought considered to be
> part of the CoreSight umbrella, falls under the debugging domain.
> 
> Adding the bindings in this file may lead people to beleive this driver fits
> into the CoreSight framework currently supported, which isn't the case.
> 
> As such it is probably a good idea to spin off a new file, "coresight-debug.txt"
> to handle this driver. 

I think this is good suggestion; if Mark has no objection, I will
follow up it.

> Mark, what's your take on this?
> 
> >  	* reg: physical base address and length of the register
> >  	  set(s) of the component.
> >  
> > @@ -78,8 +81,10 @@ its hardware characteristcs.
> >  	* arm,cp14: must be present if the system accesses ETM/PTM management
> >  	  registers via co-processor 14.
> >  
> > -	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
> > -	  source is considered to belong to CPU0.
> > +* Optional properties for ETM/PTM/Debugs:
> > +
> > +	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
> > +	  the source is considered to belong to CPU0.
> >  
> >  * Optional property for TMC:
> >  
> > -- 
> > 2.7.4
> > 

[PATCH RFC 1/3] coresight: binding for coresight debug driver

[Leo Yan]

On Wed, Feb 15, 2017 at 01:08:58PM -0700, Mathieu Poirier wrote:
> On Mon, Feb 13, 2017 at 02:11:36PM +0800, Leo Yan wrote:
> > Adding compatible string for new coresight debug driver.
> > 
> 
> Hi Leo,
> 
> I agree with Mark, this will need a better description.
> 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/arm/coresight.txt | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> > index fcbae6a..3ff15fd 100644
> > --- a/Documentation/devicetree/bindings/arm/coresight.txt
> > +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> > @@ -40,6 +40,9 @@ its hardware characteristcs.
> >  		- System Trace Macrocell:
> >  			"arm,coresight-stm", "arm,primecell"; [1]
> >  
> > +		- Debug Unit:
> > +			"arm,coresight-debug", "arm,primecell";
> > +
> 
> Humm...  The current CoreSight bindings are meant to describe IPs included in
> the HW assisted trace architecture.  This new driver, althought considered to be
> part of the CoreSight umbrella, falls under the debugging domain.
> 
> Adding the bindings in this file may lead people to beleive this driver fits
> into the CoreSight framework currently supported, which isn't the case.
> 
> As such it is probably a good idea to spin off a new file, "coresight-debug.txt"
> to handle this driver. 

I think this is good suggestion; if Mark has no objection, I will
follow up it.

> Mark, what's your take on this?
> 
> >  	* reg: physical base address and length of the register
> >  	  set(s) of the component.
> >  
> > @@ -78,8 +81,10 @@ its hardware characteristcs.
> >  	* arm,cp14: must be present if the system accesses ETM/PTM management
> >  	  registers via co-processor 14.
> >  
> > -	* cpu: the cpu phandle this ETM/PTM is affined to. When omitted the
> > -	  source is considered to belong to CPU0.
> > +* Optional properties for ETM/PTM/Debugs:
> > +
> > +	* cpu: the cpu phandle this ETM/PTM/Debug is affined to. When omitted
> > +	  the source is considered to belong to CPU0.
> >  
> >  * Optional property for TMC:
> >  
> > -- 
> > 2.7.4
> > 

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Leo Yan]

Hi Mark,

On Wed, Feb 15, 2017 at 11:44:16AM +0000, Mark Rutland wrote:
> [resending due to a mail server snafu]
> 
> On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> > the debug registers in the chapter "H9: External Debug Register
> > Descriptions".
> 
> This should have been in the binding description also.
> 
> The layout of the ARM ARM can change over time, so please refer to the
> full document number, which can be found at the bottom of each page
> (e.g. ARM DDI 0487A.j).

Good to know this. Will use this way to connect code with spec.

> > After enable the debug module we can check CPU state and PC value, etc.
> > So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> > into infinite loop with IRQ disabled. So the CPU cannot switch context
> > and handle any interrupt, so it cannot handle SMP call for stack dump,
> > etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> > ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> > firmware and cannot return back to kernel.
> 
> I would generally expect that the secure world would lock down
> debugging, as this poses a security risk.
> 
> I take it that this is only unlocked on development firmware.
> 
> Given that cores can be powered down outside of our control, I'm not
> sure that accesses to these registers is safe in general.

Mike Leach has gave method in another email to check CPU power state in
register EDPRSR.PU before access EDPCSR register.

For security state, I will check if EDSCR.NS bit can be use to
distinguish CPU is secure or non-secure state. And also follow Mike's
suggestion to use EDPCSR = 0xFFFF_FFFF as hint that the CPU is in
secure state.

> > This patch is to enable coresight debug module and register callback
> > notifier for panic; so when system detect the CPU lockup we can utilize
> > debug module registers to get to know PC value for all CPUs; so we can
> > quickly know the hang address for CPUs.
> > 
> > This is initial driver for coresight debug module and could enhance it
> > later according to debugging requirement.
> 
> How does this interact with an external debugger making use of these
> registers?

IIUC, external debugger also will access these registers, like use
EDPCSR to display PC value. I have no idea for the connection between
this driver with external debugger, except I think we can use this
driver to enable clock for CPU debug module.

> [...]
> 
> > +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> 
> A per-cpu variable is preferred to an NR_CPUS sized array.

Will do.

> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > +	/* Unlocks the debug registers */
> > +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > +	isb();
> > +}
> 
> I do not believe this barrier is correct.

I copied the code piece from coresight-etm4x.c.

I guess here is to ensure the os unlock operations is finished before
any sequential register accessing. I think isb() is redundant for
debug module and we can only use writel_relaxed() will be enough for
accessing EDOSLAR and EDPCSR, these two registers are in the same
endpoint so we don't need add extra barrier for them. Is this correct?

> [...]
> 
> > +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> > +{
> > +	u32 pcsr_hi, pcsr_lo;
> > +
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	debug_os_unlock(drvdata);
> > +
> > +#ifdef CONFIG_64BIT
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> > +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> > +#else
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> > +#endif
> > +
> > +	CS_LOCK(drvdata->base);
> > +}
> 
> Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
> Counter Sample Register":
> 
> 	Implemented only if the OPTIONAL PC Sample-based Profiling
> 	Extension is implemented.
> 
> So even if we have access to an MMIO debug interface, we cannot
> necessarily acecess this register.

Eventually this is a hardware feature, right? If so we can use one DT
property to describe this.

> [...]
> 
> > +/*
> > + * Dump out memory limit information on panic.
> > + */
> > +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> > +{
> > +	int i;
> > +
> > +	pr_emerg("Coresight debug module:\n");
> > +
> > +	for_each_possible_cpu(i) {
> > +
> > +		if (!debug_drvdata[i])
> > +			continue;
> > +
> > +		debug_read_pcsr(debug_drvdata[i]);
> > +	}
> 
> Is there no potential for deadlock with a CPU reading its own debug
> interface registers?

Have checked this, does not introduce deadlock.

> [...]
> 
> > +static struct amba_id debug_ids[] = {
> > +	{       /* Debug for Cortex-A53 */
> > +		.id	= 0x000bbd03,
> > +		.mask	= 0x000fffff,
> > +		.data	= "debug",
> > +	},
> > +	{ 0, 0},
> > +};
> 
> The DT binding said nothing about Cortex-A53.
> 
> How variable are the MMIO registers in practice? Do we need to know the
> particular CPU?

We don't need know particular CPU for current driver; but I think the
amba id is different for different CPU variants. Should I remove
related binding for this?

Thanks,
Leo Yan

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Leo Yan]

Hi Mark,

On Wed, Feb 15, 2017 at 11:44:16AM +0000, Mark Rutland wrote:
> [resending due to a mail server snafu]
> 
> On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> > the debug registers in the chapter "H9: External Debug Register
> > Descriptions".
> 
> This should have been in the binding description also.
> 
> The layout of the ARM ARM can change over time, so please refer to the
> full document number, which can be found at the bottom of each page
> (e.g. ARM DDI 0487A.j).

Good to know this. Will use this way to connect code with spec.

> > After enable the debug module we can check CPU state and PC value, etc.
> > So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> > into infinite loop with IRQ disabled. So the CPU cannot switch context
> > and handle any interrupt, so it cannot handle SMP call for stack dump,
> > etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> > ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> > firmware and cannot return back to kernel.
> 
> I would generally expect that the secure world would lock down
> debugging, as this poses a security risk.
> 
> I take it that this is only unlocked on development firmware.
> 
> Given that cores can be powered down outside of our control, I'm not
> sure that accesses to these registers is safe in general.

Mike Leach has gave method in another email to check CPU power state in
register EDPRSR.PU before access EDPCSR register.

For security state, I will check if EDSCR.NS bit can be use to
distinguish CPU is secure or non-secure state. And also follow Mike's
suggestion to use EDPCSR = 0xFFFF_FFFF as hint that the CPU is in
secure state.

> > This patch is to enable coresight debug module and register callback
> > notifier for panic; so when system detect the CPU lockup we can utilize
> > debug module registers to get to know PC value for all CPUs; so we can
> > quickly know the hang address for CPUs.
> > 
> > This is initial driver for coresight debug module and could enhance it
> > later according to debugging requirement.
> 
> How does this interact with an external debugger making use of these
> registers?

IIUC, external debugger also will access these registers, like use
EDPCSR to display PC value. I have no idea for the connection between
this driver with external debugger, except I think we can use this
driver to enable clock for CPU debug module.

> [...]
> 
> > +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> 
> A per-cpu variable is preferred to an NR_CPUS sized array.

Will do.

> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > +	/* Unlocks the debug registers */
> > +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > +	isb();
> > +}
> 
> I do not believe this barrier is correct.

I copied the code piece from coresight-etm4x.c.

I guess here is to ensure the os unlock operations is finished before
any sequential register accessing. I think isb() is redundant for
debug module and we can only use writel_relaxed() will be enough for
accessing EDOSLAR and EDPCSR, these two registers are in the same
endpoint so we don't need add extra barrier for them. Is this correct?

> [...]
> 
> > +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> > +{
> > +	u32 pcsr_hi, pcsr_lo;
> > +
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	debug_os_unlock(drvdata);
> > +
> > +#ifdef CONFIG_64BIT
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> > +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> > +#else
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> > +#endif
> > +
> > +	CS_LOCK(drvdata->base);
> > +}
> 
> Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
> Counter Sample Register":
> 
> 	Implemented only if the OPTIONAL PC Sample-based Profiling
> 	Extension is implemented.
> 
> So even if we have access to an MMIO debug interface, we cannot
> necessarily acecess this register.

Eventually this is a hardware feature, right? If so we can use one DT
property to describe this.

> [...]
> 
> > +/*
> > + * Dump out memory limit information on panic.
> > + */
> > +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> > +{
> > +	int i;
> > +
> > +	pr_emerg("Coresight debug module:\n");
> > +
> > +	for_each_possible_cpu(i) {
> > +
> > +		if (!debug_drvdata[i])
> > +			continue;
> > +
> > +		debug_read_pcsr(debug_drvdata[i]);
> > +	}
> 
> Is there no potential for deadlock with a CPU reading its own debug
> interface registers?

Have checked this, does not introduce deadlock.

> [...]
> 
> > +static struct amba_id debug_ids[] = {
> > +	{       /* Debug for Cortex-A53 */
> > +		.id	= 0x000bbd03,
> > +		.mask	= 0x000fffff,
> > +		.data	= "debug",
> > +	},
> > +	{ 0, 0},
> > +};
> 
> The DT binding said nothing about Cortex-A53.
> 
> How variable are the MMIO registers in practice? Do we need to know the
> particular CPU?

We don't need know particular CPU for current driver; but I think the
amba id is different for different CPU variants. Should I remove
related binding for this?

Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/3] coresight: add support for debug module

[Leo Yan]

Hi Mark,

On Wed, Feb 15, 2017 at 11:44:16AM +0000, Mark Rutland wrote:
> [resending due to a mail server snafu]
> 
> On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> > the debug registers in the chapter "H9: External Debug Register
> > Descriptions".
> 
> This should have been in the binding description also.
> 
> The layout of the ARM ARM can change over time, so please refer to the
> full document number, which can be found at the bottom of each page
> (e.g. ARM DDI 0487A.j).

Good to know this. Will use this way to connect code with spec.

> > After enable the debug module we can check CPU state and PC value, etc.
> > So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> > into infinite loop with IRQ disabled. So the CPU cannot switch context
> > and handle any interrupt, so it cannot handle SMP call for stack dump,
> > etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> > ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> > firmware and cannot return back to kernel.
> 
> I would generally expect that the secure world would lock down
> debugging, as this poses a security risk.
> 
> I take it that this is only unlocked on development firmware.
> 
> Given that cores can be powered down outside of our control, I'm not
> sure that accesses to these registers is safe in general.

Mike Leach has gave method in another email to check CPU power state in
register EDPRSR.PU before access EDPCSR register.

For security state, I will check if EDSCR.NS bit can be use to
distinguish CPU is secure or non-secure state. And also follow Mike's
suggestion to use EDPCSR = 0xFFFF_FFFF as hint that the CPU is in
secure state.

> > This patch is to enable coresight debug module and register callback
> > notifier for panic; so when system detect the CPU lockup we can utilize
> > debug module registers to get to know PC value for all CPUs; so we can
> > quickly know the hang address for CPUs.
> > 
> > This is initial driver for coresight debug module and could enhance it
> > later according to debugging requirement.
> 
> How does this interact with an external debugger making use of these
> registers?

IIUC, external debugger also will access these registers, like use
EDPCSR to display PC value. I have no idea for the connection between
this driver with external debugger, except I think we can use this
driver to enable clock for CPU debug module.

> [...]
> 
> > +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> 
> A per-cpu variable is preferred to an NR_CPUS sized array.

Will do.

> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > +	/* Unlocks the debug registers */
> > +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > +	isb();
> > +}
> 
> I do not believe this barrier is correct.

I copied the code piece from coresight-etm4x.c.

I guess here is to ensure the os unlock operations is finished before
any sequential register accessing. I think isb() is redundant for
debug module and we can only use writel_relaxed() will be enough for
accessing EDOSLAR and EDPCSR, these two registers are in the same
endpoint so we don't need add extra barrier for them. Is this correct?

> [...]
> 
> > +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> > +{
> > +	u32 pcsr_hi, pcsr_lo;
> > +
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	debug_os_unlock(drvdata);
> > +
> > +#ifdef CONFIG_64BIT
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> > +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> > +#else
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> > +#endif
> > +
> > +	CS_LOCK(drvdata->base);
> > +}
> 
> Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
> Counter Sample Register":
> 
> 	Implemented only if the OPTIONAL PC Sample-based Profiling
> 	Extension is implemented.
> 
> So even if we have access to an MMIO debug interface, we cannot
> necessarily acecess this register.

Eventually this is a hardware feature, right? If so we can use one DT
property to describe this.

> [...]
> 
> > +/*
> > + * Dump out memory limit information on panic.
> > + */
> > +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> > +{
> > +	int i;
> > +
> > +	pr_emerg("Coresight debug module:\n");
> > +
> > +	for_each_possible_cpu(i) {
> > +
> > +		if (!debug_drvdata[i])
> > +			continue;
> > +
> > +		debug_read_pcsr(debug_drvdata[i]);
> > +	}
> 
> Is there no potential for deadlock with a CPU reading its own debug
> interface registers?

Have checked this, does not introduce deadlock.

> [...]
> 
> > +static struct amba_id debug_ids[] = {
> > +	{       /* Debug for Cortex-A53 */
> > +		.id	= 0x000bbd03,
> > +		.mask	= 0x000fffff,
> > +		.data	= "debug",
> > +	},
> > +	{ 0, 0},
> > +};
> 
> The DT binding said nothing about Cortex-A53.
> 
> How variable are the MMIO registers in practice? Do we need to know the
> particular CPU?

We don't need know particular CPU for current driver; but I think the
amba id is different for different CPU variants. Should I remove
related binding for this?

Thanks,
Leo Yan

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Leo Yan]

Hi Mathieu,

On Wed, Feb 15, 2017 at 02:08:05PM -0700, Mathieu Poirier wrote:
> On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> > the debug registers in the chapter "H9: External Debug Register
> > Descriptions".
> > 
> > After enable the debug module we can check CPU state and PC value, etc.
> > So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> > into infinite loop with IRQ disabled. So the CPU cannot switch context
> > and handle any interrupt, so it cannot handle SMP call for stack dump,
> > etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> > ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> > firmware and cannot return back to kernel.
> > 
> > This patch is to enable coresight debug module and register callback
> > notifier for panic; so when system detect the CPU lockup we can utilize
> > debug module registers to get to know PC value for all CPUs; so we can
> > quickly know the hang address for CPUs.
> > 
> > This is initial driver for coresight debug module and could enhance it
> > later according to debugging requirement.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  drivers/hwtracing/coresight/Kconfig           |   8 ++
> >  drivers/hwtracing/coresight/Makefile          |   1 +
> >  drivers/hwtracing/coresight/coresight-debug.c | 169 ++++++++++++++++++++++++++
> >  3 files changed, 178 insertions(+)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
> > 
> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> > index 130cb21..dcf59cc 100644
> > --- a/drivers/hwtracing/coresight/Kconfig
> > +++ b/drivers/hwtracing/coresight/Kconfig
> > @@ -89,4 +89,12 @@ config CORESIGHT_STM
> >  	  logging useful software events or data coming from various entities
> >  	  in the system, possibly running different OSs
> >  
> > +config CORESIGHT_DEBUG
> > +	bool "CoreSight debug driver"
> > +	depends on ARM || ARM64
> > +	help
> > +	  This driver provides support for coresight debugging module. This
> > +	  is primarily used for printing out debug registers for panic and
> > +	  soft and hard lockup.
> > +
> >  endif
> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> > index af480d9..d540d45 100644
> > --- a/drivers/hwtracing/coresight/Makefile
> > +++ b/drivers/hwtracing/coresight/Makefile
> > @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> >  					coresight-etm4x-sysfs.o
> >  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
> >  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> > +obj-$(CONFIG_CORESIGHT_DEBUG) += coresight-debug.o
> > diff --git a/drivers/hwtracing/coresight/coresight-debug.c b/drivers/hwtracing/coresight/coresight-debug.c
> > new file mode 100644
> > index 0000000..28206a83
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-debug.c
> > @@ -0,0 +1,169 @@
> > +/*
> > + * Copyright(C) 2017 Linaro Limited. All rights reserved.
> > + * Author: Leo Yan <leo.yan@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License 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.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/init.h>
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/err.h>
> > +#include <linux/fs.h>
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
> > +#include <linux/smp.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/stat.h>
> > +#include <linux/clk.h>
> > +#include <linux/cpu.h>
> > +#include <linux/coresight.h>
> > +#include <linux/amba/bus.h>
> > +#include <linux/uaccess.h>
> 
> When possible, please try to order the header files alphabetically.

Will fix.

> > +
> > +#include "coresight-priv.h"
> > +
> > +#define EDPCSR_LO	0x0A0
> > +#define EDPCSR_HI	0x0AC
> > +#define EDOSLAR		0x300
> > +#define EDOSLSR		0x304
> > +#define EDPDCR		0x310
> > +#define EDPDSR		0x314
> 
> EDOSLSR, DEPDCR and EDPDSR aren't used in the code presented below.

Will remove them.

> > +
> > +struct debug_drvdata {
> > +	void __iomem	*base;
> > +	struct device	*dev;
> > +	int		cpu;
> > +};
> > +
> > +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > +	/* Unlocks the debug registers */
> > +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > +	isb();
> > +}
> > +
> > +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> > +{
> > +	u32 pcsr_hi, pcsr_lo;
> > +
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	debug_os_unlock(drvdata);
> > +
> > +#ifdef CONFIG_64BIT
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> > +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> > +#else
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> > +#endif
> > +
> > +	CS_LOCK(drvdata->base);
> > +}
> > +
> > +/*
> > + * Dump out memory limit information on panic.
> > + */
> > +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> > +{
> > +	int i;
> > +
> > +	pr_emerg("Coresight debug module:\n");
> > +
> > +	for_each_possible_cpu(i) {
> > +
> > +		if (!debug_drvdata[i])
> > +			continue;
> > +
> > +		debug_read_pcsr(debug_drvdata[i]);
> 
> Mark and Mike have raised a number of problems with this.  If I get things
> correctly the pseudocode (CreatePCSample() and EDPCSRlo[]), found in the
> reference manual, should be dealing with most of them. 

I reviewed the code for CreatePCSample() and EDPCSRlo[], they are
likely the pseudo code for hardware logic for PC sampling, but not
software code for how to read EDPCSR. But I think the code still
can give us some hints.

> One thing that is subtle in the peudocode is the memory-mapped access to these
> registers and something else that should be checked here before proceeding. 

Agree, I will follow Mike's suggestion.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct notifier_block debug_notifier = {
> > +	.notifier_call = dump_debug,
> > +};
> > +
> > +static int __init register_coresight_debug_dumper(void)
> > +{
> > +	atomic_notifier_chain_register(&panic_notifier_list,
> > +				       &debug_notifier);
> > +	return 0;
> > +}
> > +__initcall(register_coresight_debug_dumper);
> > +
> > +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> > +{
> > +	void __iomem *base;
> > +	struct device *dev = &adev->dev;
> > +	struct coresight_platform_data *pdata = NULL;
> > +	struct debug_drvdata *drvdata;
> > +	struct resource *res = &adev->res;
> > +	struct device_node *np = adev->dev.of_node;
> > +
> > +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > +	if (!drvdata)
> > +		return -ENOMEM;
> > +
> > +	if (np) {
> > +		pdata = of_get_coresight_platform_data(dev, np);
> > +		if (IS_ERR(pdata))
> > +			return PTR_ERR(pdata);
> > +		adev->dev.platform_data = pdata;
> > +	}
> > +
> > +	drvdata->dev = &adev->dev;
> > +	dev_set_drvdata(dev, drvdata);
> > +
> > +	/* Validity for the resource is already checked by the AMBA core */
> > +	base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	drvdata->base = base;
> > +	drvdata->cpu = pdata ? pdata->cpu : 0;
> > +	debug_drvdata[drvdata->cpu] = drvdata;
> 
> Add atomic_notifier_chain_register() here rather than letting the infrastructure
> deal with it in an initcall.  That way we don't clutter the panic_notifier_list
> needlessly if the IP is not present.

Good point. Will fix.

[...]

Thanks,
Leo Yan

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Leo Yan]

Hi Mathieu,

On Wed, Feb 15, 2017 at 02:08:05PM -0700, Mathieu Poirier wrote:
> On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> > the debug registers in the chapter "H9: External Debug Register
> > Descriptions".
> > 
> > After enable the debug module we can check CPU state and PC value, etc.
> > So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> > into infinite loop with IRQ disabled. So the CPU cannot switch context
> > and handle any interrupt, so it cannot handle SMP call for stack dump,
> > etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> > ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> > firmware and cannot return back to kernel.
> > 
> > This patch is to enable coresight debug module and register callback
> > notifier for panic; so when system detect the CPU lockup we can utilize
> > debug module registers to get to know PC value for all CPUs; so we can
> > quickly know the hang address for CPUs.
> > 
> > This is initial driver for coresight debug module and could enhance it
> > later according to debugging requirement.
> > 
> > Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/hwtracing/coresight/Kconfig           |   8 ++
> >  drivers/hwtracing/coresight/Makefile          |   1 +
> >  drivers/hwtracing/coresight/coresight-debug.c | 169 ++++++++++++++++++++++++++
> >  3 files changed, 178 insertions(+)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
> > 
> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> > index 130cb21..dcf59cc 100644
> > --- a/drivers/hwtracing/coresight/Kconfig
> > +++ b/drivers/hwtracing/coresight/Kconfig
> > @@ -89,4 +89,12 @@ config CORESIGHT_STM
> >  	  logging useful software events or data coming from various entities
> >  	  in the system, possibly running different OSs
> >  
> > +config CORESIGHT_DEBUG
> > +	bool "CoreSight debug driver"
> > +	depends on ARM || ARM64
> > +	help
> > +	  This driver provides support for coresight debugging module. This
> > +	  is primarily used for printing out debug registers for panic and
> > +	  soft and hard lockup.
> > +
> >  endif
> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> > index af480d9..d540d45 100644
> > --- a/drivers/hwtracing/coresight/Makefile
> > +++ b/drivers/hwtracing/coresight/Makefile
> > @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> >  					coresight-etm4x-sysfs.o
> >  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
> >  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> > +obj-$(CONFIG_CORESIGHT_DEBUG) += coresight-debug.o
> > diff --git a/drivers/hwtracing/coresight/coresight-debug.c b/drivers/hwtracing/coresight/coresight-debug.c
> > new file mode 100644
> > index 0000000..28206a83
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-debug.c
> > @@ -0,0 +1,169 @@
> > +/*
> > + * Copyright(C) 2017 Linaro Limited. All rights reserved.
> > + * Author: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License 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.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/init.h>
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/err.h>
> > +#include <linux/fs.h>
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
> > +#include <linux/smp.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/stat.h>
> > +#include <linux/clk.h>
> > +#include <linux/cpu.h>
> > +#include <linux/coresight.h>
> > +#include <linux/amba/bus.h>
> > +#include <linux/uaccess.h>
> 
> When possible, please try to order the header files alphabetically.

Will fix.

> > +
> > +#include "coresight-priv.h"
> > +
> > +#define EDPCSR_LO	0x0A0
> > +#define EDPCSR_HI	0x0AC
> > +#define EDOSLAR		0x300
> > +#define EDOSLSR		0x304
> > +#define EDPDCR		0x310
> > +#define EDPDSR		0x314
> 
> EDOSLSR, DEPDCR and EDPDSR aren't used in the code presented below.

Will remove them.

> > +
> > +struct debug_drvdata {
> > +	void __iomem	*base;
> > +	struct device	*dev;
> > +	int		cpu;
> > +};
> > +
> > +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > +	/* Unlocks the debug registers */
> > +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > +	isb();
> > +}
> > +
> > +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> > +{
> > +	u32 pcsr_hi, pcsr_lo;
> > +
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	debug_os_unlock(drvdata);
> > +
> > +#ifdef CONFIG_64BIT
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> > +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> > +#else
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> > +#endif
> > +
> > +	CS_LOCK(drvdata->base);
> > +}
> > +
> > +/*
> > + * Dump out memory limit information on panic.
> > + */
> > +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> > +{
> > +	int i;
> > +
> > +	pr_emerg("Coresight debug module:\n");
> > +
> > +	for_each_possible_cpu(i) {
> > +
> > +		if (!debug_drvdata[i])
> > +			continue;
> > +
> > +		debug_read_pcsr(debug_drvdata[i]);
> 
> Mark and Mike have raised a number of problems with this.  If I get things
> correctly the pseudocode (CreatePCSample() and EDPCSRlo[]), found in the
> reference manual, should be dealing with most of them. 

I reviewed the code for CreatePCSample() and EDPCSRlo[], they are
likely the pseudo code for hardware logic for PC sampling, but not
software code for how to read EDPCSR. But I think the code still
can give us some hints.

> One thing that is subtle in the peudocode is the memory-mapped access to these
> registers and something else that should be checked here before proceeding. 

Agree, I will follow Mike's suggestion.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct notifier_block debug_notifier = {
> > +	.notifier_call = dump_debug,
> > +};
> > +
> > +static int __init register_coresight_debug_dumper(void)
> > +{
> > +	atomic_notifier_chain_register(&panic_notifier_list,
> > +				       &debug_notifier);
> > +	return 0;
> > +}
> > +__initcall(register_coresight_debug_dumper);
> > +
> > +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> > +{
> > +	void __iomem *base;
> > +	struct device *dev = &adev->dev;
> > +	struct coresight_platform_data *pdata = NULL;
> > +	struct debug_drvdata *drvdata;
> > +	struct resource *res = &adev->res;
> > +	struct device_node *np = adev->dev.of_node;
> > +
> > +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > +	if (!drvdata)
> > +		return -ENOMEM;
> > +
> > +	if (np) {
> > +		pdata = of_get_coresight_platform_data(dev, np);
> > +		if (IS_ERR(pdata))
> > +			return PTR_ERR(pdata);
> > +		adev->dev.platform_data = pdata;
> > +	}
> > +
> > +	drvdata->dev = &adev->dev;
> > +	dev_set_drvdata(dev, drvdata);
> > +
> > +	/* Validity for the resource is already checked by the AMBA core */
> > +	base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	drvdata->base = base;
> > +	drvdata->cpu = pdata ? pdata->cpu : 0;
> > +	debug_drvdata[drvdata->cpu] = drvdata;
> 
> Add atomic_notifier_chain_register() here rather than letting the infrastructure
> deal with it in an initcall.  That way we don't clutter the panic_notifier_list
> needlessly if the IP is not present.

Good point. Will fix.

[...]

Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/3] coresight: add support for debug module

[Leo Yan]

Hi Mathieu,

On Wed, Feb 15, 2017 at 02:08:05PM -0700, Mathieu Poirier wrote:
> On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> > the debug registers in the chapter "H9: External Debug Register
> > Descriptions".
> > 
> > After enable the debug module we can check CPU state and PC value, etc.
> > So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> > into infinite loop with IRQ disabled. So the CPU cannot switch context
> > and handle any interrupt, so it cannot handle SMP call for stack dump,
> > etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> > ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> > firmware and cannot return back to kernel.
> > 
> > This patch is to enable coresight debug module and register callback
> > notifier for panic; so when system detect the CPU lockup we can utilize
> > debug module registers to get to know PC value for all CPUs; so we can
> > quickly know the hang address for CPUs.
> > 
> > This is initial driver for coresight debug module and could enhance it
> > later according to debugging requirement.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  drivers/hwtracing/coresight/Kconfig           |   8 ++
> >  drivers/hwtracing/coresight/Makefile          |   1 +
> >  drivers/hwtracing/coresight/coresight-debug.c | 169 ++++++++++++++++++++++++++
> >  3 files changed, 178 insertions(+)
> >  create mode 100644 drivers/hwtracing/coresight/coresight-debug.c
> > 
> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> > index 130cb21..dcf59cc 100644
> > --- a/drivers/hwtracing/coresight/Kconfig
> > +++ b/drivers/hwtracing/coresight/Kconfig
> > @@ -89,4 +89,12 @@ config CORESIGHT_STM
> >  	  logging useful software events or data coming from various entities
> >  	  in the system, possibly running different OSs
> >  
> > +config CORESIGHT_DEBUG
> > +	bool "CoreSight debug driver"
> > +	depends on ARM || ARM64
> > +	help
> > +	  This driver provides support for coresight debugging module. This
> > +	  is primarily used for printing out debug registers for panic and
> > +	  soft and hard lockup.
> > +
> >  endif
> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> > index af480d9..d540d45 100644
> > --- a/drivers/hwtracing/coresight/Makefile
> > +++ b/drivers/hwtracing/coresight/Makefile
> > @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> >  					coresight-etm4x-sysfs.o
> >  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
> >  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> > +obj-$(CONFIG_CORESIGHT_DEBUG) += coresight-debug.o
> > diff --git a/drivers/hwtracing/coresight/coresight-debug.c b/drivers/hwtracing/coresight/coresight-debug.c
> > new file mode 100644
> > index 0000000..28206a83
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-debug.c
> > @@ -0,0 +1,169 @@
> > +/*
> > + * Copyright(C) 2017 Linaro Limited. All rights reserved.
> > + * Author: Leo Yan <leo.yan@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License 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.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/init.h>
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/err.h>
> > +#include <linux/fs.h>
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
> > +#include <linux/smp.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/stat.h>
> > +#include <linux/clk.h>
> > +#include <linux/cpu.h>
> > +#include <linux/coresight.h>
> > +#include <linux/amba/bus.h>
> > +#include <linux/uaccess.h>
> 
> When possible, please try to order the header files alphabetically.

Will fix.

> > +
> > +#include "coresight-priv.h"
> > +
> > +#define EDPCSR_LO	0x0A0
> > +#define EDPCSR_HI	0x0AC
> > +#define EDOSLAR		0x300
> > +#define EDOSLSR		0x304
> > +#define EDPDCR		0x310
> > +#define EDPDSR		0x314
> 
> EDOSLSR, DEPDCR and EDPDSR aren't used in the code presented below.

Will remove them.

> > +
> > +struct debug_drvdata {
> > +	void __iomem	*base;
> > +	struct device	*dev;
> > +	int		cpu;
> > +};
> > +
> > +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > +	/* Unlocks the debug registers */
> > +	writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > +	isb();
> > +}
> > +
> > +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> > +{
> > +	u32 pcsr_hi, pcsr_lo;
> > +
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	debug_os_unlock(drvdata);
> > +
> > +#ifdef CONFIG_64BIT
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +	pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> > +		 ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> > +#else
> > +	pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +
> > +	pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,	pcsr_lo);
> > +#endif
> > +
> > +	CS_LOCK(drvdata->base);
> > +}
> > +
> > +/*
> > + * Dump out memory limit information on panic.
> > + */
> > +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> > +{
> > +	int i;
> > +
> > +	pr_emerg("Coresight debug module:\n");
> > +
> > +	for_each_possible_cpu(i) {
> > +
> > +		if (!debug_drvdata[i])
> > +			continue;
> > +
> > +		debug_read_pcsr(debug_drvdata[i]);
> 
> Mark and Mike have raised a number of problems with this.  If I get things
> correctly the pseudocode (CreatePCSample() and EDPCSRlo[]), found in the
> reference manual, should be dealing with most of them. 

I reviewed the code for CreatePCSample() and EDPCSRlo[], they are
likely the pseudo code for hardware logic for PC sampling, but not
software code for how to read EDPCSR. But I think the code still
can give us some hints.

> One thing that is subtle in the peudocode is the memory-mapped access to these
> registers and something else that should be checked here before proceeding. 

Agree, I will follow Mike's suggestion.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct notifier_block debug_notifier = {
> > +	.notifier_call = dump_debug,
> > +};
> > +
> > +static int __init register_coresight_debug_dumper(void)
> > +{
> > +	atomic_notifier_chain_register(&panic_notifier_list,
> > +				       &debug_notifier);
> > +	return 0;
> > +}
> > +__initcall(register_coresight_debug_dumper);
> > +
> > +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> > +{
> > +	void __iomem *base;
> > +	struct device *dev = &adev->dev;
> > +	struct coresight_platform_data *pdata = NULL;
> > +	struct debug_drvdata *drvdata;
> > +	struct resource *res = &adev->res;
> > +	struct device_node *np = adev->dev.of_node;
> > +
> > +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > +	if (!drvdata)
> > +		return -ENOMEM;
> > +
> > +	if (np) {
> > +		pdata = of_get_coresight_platform_data(dev, np);
> > +		if (IS_ERR(pdata))
> > +			return PTR_ERR(pdata);
> > +		adev->dev.platform_data = pdata;
> > +	}
> > +
> > +	drvdata->dev = &adev->dev;
> > +	dev_set_drvdata(dev, drvdata);
> > +
> > +	/* Validity for the resource is already checked by the AMBA core */
> > +	base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	drvdata->base = base;
> > +	drvdata->cpu = pdata ? pdata->cpu : 0;
> > +	debug_drvdata[drvdata->cpu] = drvdata;
> 
> Add atomic_notifier_chain_register() here rather than letting the infrastructure
> deal with it in an initcall.  That way we don't clutter the panic_notifier_list
> needlessly if the IP is not present.

Good point. Will fix.

[...]

Thanks,
Leo Yan

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Mathieu Poirier]

On Wed, Feb 15, 2017 at 11:43:27AM +0000, Mark Rutland wrote:
> On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> > the debug registers in the chapter "H9: External Debug Register
> > Descriptions".
> 
> This should have been in the binding description also.
> 
> The layout of the ARM ARM can change over time, so please refer to the
> full document number, which can be found at the bottom of each page
> (e.g. ARM DDI 0487A.j).
> 
> > After enable the debug module we can check CPU state and PC value, etc.
> > So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> > into infinite loop with IRQ disabled. So the CPU cannot switch context
> > and handle any interrupt, so it cannot handle SMP call for stack dump,
> > etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> > ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> > firmware and cannot return back to kernel.
> 
> I would generally expect that the secure world would lock down
> debugging, as this poses a security risk.
> 
> I take it that this is only unlocked on development firmware.
> 
> Given that cores can be powered down outside of our control, I'm not
> sure that accesses to these registers is safe in general.
> 
> > This patch is to enable coresight debug module and register callback
> > notifier for panic; so when system detect the CPU lockup we can utilize
> > debug module registers to get to know PC value for all CPUs; so we can
> > quickly know the hang address for CPUs.
> >
> > This is initial driver for coresight debug module and could enhance it
> > later according to debugging requirement.
> 
> How does this interact with an external debugger making use of these
> registers?
> 
> [...]
> 
> > +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> 
> A per-cpu variable is preferred to an NR_CPUS sized array.
> 
> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > +     /* Unlocks the debug registers */
> > +     writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > +     isb();
> > +}
> 
> I do not believe this barrier is correct.

Mark has a point - isb() sounds like an overkill to prevent
re-ordering (same for the ETM4x driver).  smb_wmb() should be sufficient.  

> 
> [...]
> 
> > +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> > +{
> > +     u32 pcsr_hi, pcsr_lo;
> > +
> > +     CS_UNLOCK(drvdata->base);
> > +
> > +     debug_os_unlock(drvdata);
> > +
> > +#ifdef CONFIG_64BIT
> > +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +     pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> > +              ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> > +#else
> > +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +
> > +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu, pcsr_lo);
> > +#endif
> > +
> > +     CS_LOCK(drvdata->base);
> > +}
> 
> Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
> Counter Sample Register":
> 
>         Implemented only if the OPTIONAL PC Sample-based Profiling
>         Extension is implemented.
> 
> So even if we have access to an MMIO debug interface, we cannot
> necessarily acecess this register.
> 
> [...]
> 
> > +/*
> > + * Dump out memory limit information on panic.
> > + */
> > +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> > +{
> > +     int i;
> > +
> > +     pr_emerg("Coresight debug module:\n");
> > +
> > +     for_each_possible_cpu(i) {
> > +
> > +             if (!debug_drvdata[i])
> > +                     continue;
> > +
> > +             debug_read_pcsr(debug_drvdata[i]);
> > +     }
> 
> Is there no potential for deadlock with a CPU reading its own debug
> interface registers?
> 
> [...]
> 
> > +static struct amba_id debug_ids[] = {
> > +     {       /* Debug for Cortex-A53 */
> > +             .id     = 0x000bbd03,
> > +             .mask   = 0x000fffff,
> > +             .data   = "debug",
> > +     },
> > +     { 0, 0},
> > +};
> 
> The DT binding said nothing about Cortex-A53.
> 
> How variable are the MMIO registers in practice? Do we need to know the
> particular CPU?
> 
> Thanks,
> Mark.
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH RFC 2/3] coresight: add support for debug module

[Mathieu Poirier]

On Wed, Feb 15, 2017 at 11:43:27AM +0000, Mark Rutland wrote:
> On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> > the debug registers in the chapter "H9: External Debug Register
> > Descriptions".
> 
> This should have been in the binding description also.
> 
> The layout of the ARM ARM can change over time, so please refer to the
> full document number, which can be found at the bottom of each page
> (e.g. ARM DDI 0487A.j).
> 
> > After enable the debug module we can check CPU state and PC value, etc.
> > So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> > into infinite loop with IRQ disabled. So the CPU cannot switch context
> > and handle any interrupt, so it cannot handle SMP call for stack dump,
> > etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> > ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> > firmware and cannot return back to kernel.
> 
> I would generally expect that the secure world would lock down
> debugging, as this poses a security risk.
> 
> I take it that this is only unlocked on development firmware.
> 
> Given that cores can be powered down outside of our control, I'm not
> sure that accesses to these registers is safe in general.
> 
> > This patch is to enable coresight debug module and register callback
> > notifier for panic; so when system detect the CPU lockup we can utilize
> > debug module registers to get to know PC value for all CPUs; so we can
> > quickly know the hang address for CPUs.
> >
> > This is initial driver for coresight debug module and could enhance it
> > later according to debugging requirement.
> 
> How does this interact with an external debugger making use of these
> registers?
> 
> [...]
> 
> > +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> 
> A per-cpu variable is preferred to an NR_CPUS sized array.
> 
> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > +     /* Unlocks the debug registers */
> > +     writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > +     isb();
> > +}
> 
> I do not believe this barrier is correct.

Mark has a point - isb() sounds like an overkill to prevent
re-ordering (same for the ETM4x driver).  smb_wmb() should be sufficient.  

> 
> [...]
> 
> > +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> > +{
> > +     u32 pcsr_hi, pcsr_lo;
> > +
> > +     CS_UNLOCK(drvdata->base);
> > +
> > +     debug_os_unlock(drvdata);
> > +
> > +#ifdef CONFIG_64BIT
> > +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +     pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> > +              ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> > +#else
> > +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +
> > +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu, pcsr_lo);
> > +#endif
> > +
> > +     CS_LOCK(drvdata->base);
> > +}
> 
> Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
> Counter Sample Register":
> 
>         Implemented only if the OPTIONAL PC Sample-based Profiling
>         Extension is implemented.
> 
> So even if we have access to an MMIO debug interface, we cannot
> necessarily acecess this register.
> 
> [...]
> 
> > +/*
> > + * Dump out memory limit information on panic.
> > + */
> > +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> > +{
> > +     int i;
> > +
> > +     pr_emerg("Coresight debug module:\n");
> > +
> > +     for_each_possible_cpu(i) {
> > +
> > +             if (!debug_drvdata[i])
> > +                     continue;
> > +
> > +             debug_read_pcsr(debug_drvdata[i]);
> > +     }
> 
> Is there no potential for deadlock with a CPU reading its own debug
> interface registers?
> 
> [...]
> 
> > +static struct amba_id debug_ids[] = {
> > +     {       /* Debug for Cortex-A53 */
> > +             .id     = 0x000bbd03,
> > +             .mask   = 0x000fffff,
> > +             .data   = "debug",
> > +     },
> > +     { 0, 0},
> > +};
> 
> The DT binding said nothing about Cortex-A53.
> 
> How variable are the MMIO registers in practice? Do we need to know the
> particular CPU?
> 
> Thanks,
> Mark.
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/3] coresight: add support for debug module

[Mathieu Poirier]

On Wed, Feb 15, 2017 at 11:43:27AM +0000, Mark Rutland wrote:
> On Mon, Feb 13, 2017 at 02:11:37PM +0800, Leo Yan wrote:
> > Coresight includes debug module and usually the module connects with CPU
> > debug logic. ARMv8 architecture reference manual (ARMv8-ARM) has defined
> > the debug registers in the chapter "H9: External Debug Register
> > Descriptions".
> 
> This should have been in the binding description also.
> 
> The layout of the ARM ARM can change over time, so please refer to the
> full document number, which can be found at the bottom of each page
> (e.g. ARM DDI 0487A.j).
> 
> > After enable the debug module we can check CPU state and PC value, etc.
> > So this is helpful for some CPU lockup bugs, e.g. if one CPU has run
> > into infinite loop with IRQ disabled. So the CPU cannot switch context
> > and handle any interrupt, so it cannot handle SMP call for stack dump,
> > etc. Furthermore, now ARMv8 introduces some other runtime firmwares like
> > ARM trusted firmware BL31, so sometime CPU hard lock may happen in the
> > firmware and cannot return back to kernel.
> 
> I would generally expect that the secure world would lock down
> debugging, as this poses a security risk.
> 
> I take it that this is only unlocked on development firmware.
> 
> Given that cores can be powered down outside of our control, I'm not
> sure that accesses to these registers is safe in general.
> 
> > This patch is to enable coresight debug module and register callback
> > notifier for panic; so when system detect the CPU lockup we can utilize
> > debug module registers to get to know PC value for all CPUs; so we can
> > quickly know the hang address for CPUs.
> >
> > This is initial driver for coresight debug module and could enhance it
> > later according to debugging requirement.
> 
> How does this interact with an external debugger making use of these
> registers?
> 
> [...]
> 
> > +static struct debug_drvdata *debug_drvdata[NR_CPUS];
> 
> A per-cpu variable is preferred to an NR_CPUS sized array.
> 
> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > +     /* Unlocks the debug registers */
> > +     writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > +     isb();
> > +}
> 
> I do not believe this barrier is correct.

Mark has a point - isb() sounds like an overkill to prevent
re-ordering (same for the ETM4x driver).  smb_wmb() should be sufficient.  

> 
> [...]
> 
> > +static void debug_read_pcsr(struct debug_drvdata *drvdata)
> > +{
> > +     u32 pcsr_hi, pcsr_lo;
> > +
> > +     CS_UNLOCK(drvdata->base);
> > +
> > +     debug_os_unlock(drvdata);
> > +
> > +#ifdef CONFIG_64BIT
> > +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +     pcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu,
> > +              ((unsigned long)pcsr_hi << 32 | (unsigned long)pcsr_lo));
> > +#else
> > +     pcsr_lo = readl_relaxed(drvdata->base + EDPCSR_LO);
> > +
> > +     pr_emerg("CPU[%d]: PSCR=0x%lx\n", drvdata->cpu, pcsr_lo);
> > +#endif
> > +
> > +     CS_LOCK(drvdata->base);
> > +}
> 
> Per ARM DDI 0487A.k_iss10775, H9.2.32, "EDPCSR, External Debug Program
> Counter Sample Register":
> 
>         Implemented only if the OPTIONAL PC Sample-based Profiling
>         Extension is implemented.
> 
> So even if we have access to an MMIO debug interface, we cannot
> necessarily acecess this register.
> 
> [...]
> 
> > +/*
> > + * Dump out memory limit information on panic.
> > + */
> > +static int dump_debug(struct notifier_block *self, unsigned long v, void *p)
> > +{
> > +     int i;
> > +
> > +     pr_emerg("Coresight debug module:\n");
> > +
> > +     for_each_possible_cpu(i) {
> > +
> > +             if (!debug_drvdata[i])
> > +                     continue;
> > +
> > +             debug_read_pcsr(debug_drvdata[i]);
> > +     }
> 
> Is there no potential for deadlock with a CPU reading its own debug
> interface registers?
> 
> [...]
> 
> > +static struct amba_id debug_ids[] = {
> > +     {       /* Debug for Cortex-A53 */
> > +             .id     = 0x000bbd03,
> > +             .mask   = 0x000fffff,
> > +             .data   = "debug",
> > +     },
> > +     { 0, 0},
> > +};
> 
> The DT binding said nothing about Cortex-A53.
> 
> How variable are the MMIO registers in practice? Do we need to know the
> particular CPU?
> 
> Thanks,
> Mark.
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.