| Message ID | 1488520809-31670-4-git-send-email-leo.yan@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/5] coresight: bindings for debug module | expand |
Hi Suziku, Thanks for reviewing, please see some replying. On Thu, Mar 09, 2017 at 04:53:05PM +0000, Suzuki K Poulose wrote: > On 03/03/17 06:00, Leo Yan wrote: > >Coresight includes debug module and usually the module connects with CPU > >debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has > >description for related info in "Part H: External Debug". > > > >Chapter H7 "The Sample-based Profiling Extension" introduces several > >sampling registers, e.g. we can check program counter value with > >combined CPU exception level, secure state, etc. So this is helpful for > >analysis CPU lockup scenarios, e.g. if one CPU has run into infinite > >loop with IRQ disabled. In this case the CPU cannot switch context and > >handle any interrupt (including IPIs), as the result it cannot handle > >SMP call for stack dump. > > > >This patch is to enable coresight debug module, so firstly this driver > >is to bind apb clock for debug module and this is to ensure the debug > >module can be accessed from program or external debugger. And the driver > >uses sample-based registers for debug purpose, e.g. when system detects > >the CPU lockup and trigger panic, the driver will dump program counter > >and combined context registers (EDCIDSR, EDVIDSR); by parsing context > >registers so can quickly get to know CPU secure state, exception level, > >etc. > > The problem is, it is not guaranteed that the EDPCSR_Hi, EDCIDSR & EDVIDSR are > updated as a side effect of a memory mapped access (which is what we do here) to the > EDPCSR_Lo. > > Section H.7.1.2 : Reads of EDPCSRs (in ARM DDI 0487A.k) : > > "The indirect writes to EDCIDSR, EDVIDSR, and EDPCSRhi might not occur for a memory-mapped access > to the external debug interface. For more information, see Memory-mapped accesses to the external debug > interface on page H8-4968." > > So we cannot really rely on the values in EDVIDSR which we use to make further decisions. So I > am wondering if this is really guranteed to be useful. So this is caused by Software lock is locked? Section H8.4.1: "Reads and writes have no side-effects. A side-effect is where a direct read or a direct write of a register creates an indirect write of the same or another register. When the Software Lock is locked, the indirect write does not occur." > >Some of the debug module registers are located in CPU power domain, so > >in the driver it has checked the power state for CPU before accessing > >registers within CPU power domain. For most safe way to use this driver, > >it's suggested to disable CPU low power states, this can simply set > >"nohlt" in kernel command line. > > > >Signed-off-by: Leo Yan <leo.yan@linaro.org> > >--- > > drivers/hwtracing/coresight/Kconfig | 10 + > > drivers/hwtracing/coresight/Makefile | 1 + > > drivers/hwtracing/coresight/coresight-debug.c | 377 ++++++++++++++++++++++++++ > > 3 files changed, 388 insertions(+) > > create mode 100644 drivers/hwtracing/coresight/coresight-debug.c > > > >diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > >index 130cb21..3ed651e 100644 > >--- a/drivers/hwtracing/coresight/Kconfig > >+++ b/drivers/hwtracing/coresight/Kconfig > >@@ -89,4 +89,14 @@ config CORESIGHT_STM > > logging useful software events or data coming from various entities > > in the system, possibly running different OSs > > > >+config CORESIGHT_DEBUG > > To make it more specific, may be CORESIGHT_CPU_DEBUG ? Will fix. > >+ bool "CoreSight debug driver" > > "Coresight CPU Debug driver" Will fix. > >+ depends on ARM || ARM64 > >+ help > >+ This driver provides support for coresight debugging module. This > >+ is primarily used to dump sample-based profiling registers for > >+ panic. To avoid lockups when accessing debug module registers, > >+ it is safer to disable CPU low power states (like "nohlt" on the > >+ kernel command line) when using this feature. > >+ > > >+#define EDPCSR_THUMB BIT(0) > >+#define EDPCSR_ARM_INST_MASK GENMASK(31, 2) > >+#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1) > > We don't need two different masks. {ED/DBG}PCSR has only bit 0 reserved > for instruction set indication. I think we need this two different masks. Please review below extra doc for PC offset analysis in ARM ARM. > >+#endif > >+ > >+/* bits definition for EDPRSR */ > >+#define EDPRSR_DLK BIT(6) > >+#define EDPRSR_PU BIT(0) > >+ > >+ > >+static void debug_read_regs(struct debug_drvdata *drvdata) > >+{ > >+ drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR); > >+ > >+ if (!debug_access_permitted(drvdata)) > >+ return; > >+ > >+ if (!drvdata->edpcsr_present) > >+ return; > >+ > >+ CS_UNLOCK(drvdata->base); > >+ > >+ debug_os_unlock(drvdata); > >+ > >+ drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR); > >+ > >+ /* > >+ * As described in ARM DDI 0487A.k, if the processing > >+ * element (PE) is in debug state, or sample-based > >+ * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF; > >+ * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become > >+ * UNKNOWN state. So directly bail out for this case. > >+ */ > >+ if (drvdata->edpcsr == EDPCSR_PROHIBITED) { > >+ CS_LOCK(drvdata->base); > >+ return; > >+ } > >+ > >+ /* > >+ * A read of the EDPCSR normally has the side-effect of > >+ * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI; > >+ * at this point it's safe to read value from them. > >+ */ > > See my comment above about the side effects of memory mapped access. Yeah. > >+ drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR); > >+#ifdef CONFIG_64BIT > >+ drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI); > >+#endif > > >+ > >+ if (drvdata->edvidsr_present) > >+ drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR); > >+ > >+ CS_LOCK(drvdata->base); > >+} > >+ > > >+#ifndef CONFIG_64BIT > > I guess this doesn't help for an ARMv8 32bit only core (e.g, Cortex-A32). And > unfortunately, there are conflicting definitions for the values for PCSROffset w.r.t > ARMv8 and ARMv7. > > DBGDEVID1[3:0] For ARMv7 : > > 0000 - Sample offset applies based on the instruction state. > 0001 - No offset applies. > > EDDEVID1[3:0] For ARMv8 : > 0000 - EDPCSR not implemented > 0010 - EDPCSR implemented without offsets, but do not use in AArch32 state! > > So there is no easy way to make sense of the value, unless you know which version > of the architecture is in use. Or may be we could co-relate it with the value from > DEVID. > > i.e, EDPCSR is not implemented do not register this device, see comments on debug_probe(). > ( And we should also include the following test for 32bit code to see if edpcsr is implemented. > See comments on debug_init_arch_data() ) > > > That way, we could use the following inference from the PCSROffset value : > > 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0]) > 0001 - No offset applies. > 0010 - No offset applies, but do not use in AArch32 mode Just now I went through ARM ARM and ARMv8 ARM, this makes sense to me. Thanks for good pointing for this. > >+static bool debug_pc_has_offset(struct debug_drvdata *drvdata) > >+{ > >+ u32 pcsr_offset; > >+ > >+ pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK; > >+ > >+ return (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET); > >+} > >+ > >+static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata, > >+ unsigned long pc) > >+{ > >+ unsigned long arm_inst_offset = 0, thumb_inst_offset = 0; > >+ > >+ if (debug_pc_has_offset(drvdata)) { > >+ arm_inst_offset = 8; > >+ thumb_inst_offset = 4; > >+ } > >+ > >+ /* Handle thumb instruction */ > >+ if (pc & EDPCSR_THUMB) { > >+ pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset; > >+ return pc; > >+ } > >+ > >+ /* > >+ * Handle arm instruction offset, if the arm instruction > >+ * is not 4 byte alignment then it's possible the case > >+ * for implementation defined; keep original value for this > >+ * case and print info for notice. > >+ */ > >+ if (pc & BIT(1)) > >+ pr_emerg("Instruction offset is implementation defined\n"); > > I am struggling to find the any mention about this in the ARM ARM. Please could > you point me to it. Sure, please see ARM DDI 0406C.b, chapter C11.11.34 " DBGPCSR, Program Counter Sampling Register": A profiling tool can use the value of the T bit to calculate the instruction address as follows: When an offset is applied to the sampled address - if T is 0 and DBGPCSR[1] is 0, ((DBGPCSR[31:2] << 2) - 8) is the address of the sampled ARM instruction - if T is 0 and DBGPCSR[1] is 1, the instruction address is IMPLEMENTATION DEFINED - if T is 1, ((DBGPCSR[31:1] << 1) - 4) is the address of the sampled Thumb or ThumbEE instruction. When no offset is applied to the sampled address - if T is 0 and DBGPCSR[1] is 0, (DBGPCSR[31:2] << 2) is the address of the sampled ARM instruction - if T is 0 and DBGPCSR[1] is 1, the instruction address is IMPLEMENTATION DEFINED - if T is 1, (DBGPCSR[31:1] << 1) is the address of the sampled Thumb or ThumbEE instruction. > >+static void debug_init_arch_data(void *info) > >+{ > >+ struct debug_drvdata *drvdata = info; > >+ u32 mode; > >+ > >+ CS_UNLOCK(drvdata->base); > >+ > >+ debug_os_unlock(drvdata); > >+ > >+ /* Read device info */ > >+ drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID); > >+ drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1); > >+ > >+ /* Parse implementation feature */ > >+ mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE; > >+ if (mode == EDDEVID_IMPL_FULL) { > >+ drvdata->edpcsr_present = true; > >+ drvdata->edvidsr_present = true; > >+ } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) { > >+ drvdata->edpcsr_present = true; > >+ drvdata->edvidsr_present = false; > > As discussed above, we need to consult the DEVID1:PCSROffset for AArch32 to decide > if we have the edpcsr implemented on ARMv8. Yeah. > >+ } else { > >+ drvdata->edpcsr_present = false; > >+ drvdata->edvidsr_present = false; > >+ } > >+ > >+ CS_LOCK(drvdata->base); > >+} > >+ > >+static int debug_probe(struct amba_device *adev, const struct amba_id *id) > >+{ > >+ void __iomem *base; > >+ struct device *dev = &adev->dev; > >+ struct debug_drvdata *drvdata; > >+ struct resource *res = &adev->res; > >+ struct device_node *np = adev->dev.of_node; > >+ char buf[32]; > >+ static int debug_count; > >+ > >+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > >+ if (!drvdata) > >+ return -ENOMEM; > >+ > >+ drvdata->cpu = np ? of_coresight_get_cpu(np) : 0; > >+ 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; > >+ > >+ get_online_cpus(); > >+ per_cpu(debug_drvdata, drvdata->cpu) = drvdata; > >+ > >+ if (smp_call_function_single(drvdata->cpu, > >+ debug_init_arch_data, drvdata, 1)) > >+ dev_err(dev, "Debug arch init failed\n"); > > If this fails (say the CPU was offline), should we still return success ? > And may be we should check if the drvdata->edpcsr_present to detect if the CPU > implements the PC Sampling and return failure here if it doesn't. Will fix. > >+ > >+ put_online_cpus(); > >+ > >+ if (!debug_count++) > >+ atomic_notifier_chain_register(&panic_notifier_list, > >+ &debug_notifier); > >+ > > >+ sprintf(buf, (char *)id->data, drvdata->cpu); > >+ dev_info(dev, "%s initialized\n", buf); > > This could simply be : > dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu); > > and get rid of the static string and the buffer, see below. > > >+ return 0; > >+} > >+ > >+static struct amba_id debug_ids[] = { > >+ { /* Debug for Cortex-A53 */ > >+ .id = 0x000bbd03, > >+ .mask = 0x000fffff, > > ... > > >+ .data = "Coresight debug-CPU%d", > > I think this is pointless, as the debug area we are interested in is always associated > with a CPU, we could as well figure out what to print from the drvdata->cpu above. I prefer to follow your suggestion for upper two comments; but I'd like check with Mathieu, due I followed up Mathieu's suggestion to write current code. Thanks, Leo Yan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Suzuki, On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote: [...] > >>So we cannot really rely on the values in EDVIDSR which we use to make further decisions. So I > >>am wondering if this is really guranteed to be useful. > > > >So this is caused by Software lock is locked? > > > >Section H8.4.1: > > > >"Reads and writes have no side-effects. A side-effect is where a > >direct read or a direct write of a register creates > >an indirect write of the same or another register. When the Software > >Lock is locked, the indirect write does > >not occur." > > Yes, thats correct, further : > > Section H9.2.32: EDPCSR > > "For a read of EDPCSRlo from the memory-mapped interface, if EDLSR.SLK == 1, meaning > the Software Lock is locked, then the access has no side-effects. That is, EDCIDSR, > EDVIDSR, and EDPCSRhi are unchanged." > > And since we do a CS_UNLOCK, that should be fine. Please ignore my comment. Thanks a lot for confirmation. [...] > >>>+ > >>>+ put_online_cpus(); > >>>+ > >>>+ if (!debug_count++) > >>>+ atomic_notifier_chain_register(&panic_notifier_list, > >>>+ &debug_notifier); > >>>+ > >> > >>>+ sprintf(buf, (char *)id->data, drvdata->cpu); > >>>+ dev_info(dev, "%s initialized\n", buf); > >> > >>This could simply be : > >> dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu); > >> > >>and get rid of the static string and the buffer, see below. > > Also we need pm_runtime_put() here to balance the pm_runtime_get_ from AMBA > device probe. More on that below. [...] > Btw, I don't see any PM calls to make sure the power domain (at least the debug domain) > is up, which could cause problems with accesses to some of these registers (leave alone the > ones in CPU power domain), especially the EDPRSR. We could also do pm_runtime_get on the > CPU's power domain, if the CPU is online, before we access the pcsr. I will add pm_runtime_get/pm_runtime_put for apb clock. But for CPU power domain, AFAIK this part is managed by PSCI but is not controlled by pm_runtime_{put|get} pairs. So at beginning, we suggest to use "nohlt" to ensure CPU power domain is enabled. Please let me know if I miss some thing for this? Thanks, Leo Yan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 10, 2017 at 01:59:15AM +0800, Leo Yan wrote: > Hi Suziku, > > Thanks for reviewing, please see some replying. > > On Thu, Mar 09, 2017 at 04:53:05PM +0000, Suzuki K Poulose wrote: > > On 03/03/17 06:00, Leo Yan wrote: > > >Coresight includes debug module and usually the module connects with CPU > > >debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has > > >description for related info in "Part H: External Debug". > > > > > >Chapter H7 "The Sample-based Profiling Extension" introduces several > > >sampling registers, e.g. we can check program counter value with > > >combined CPU exception level, secure state, etc. So this is helpful for > > >analysis CPU lockup scenarios, e.g. if one CPU has run into infinite > > >loop with IRQ disabled. In this case the CPU cannot switch context and > > >handle any interrupt (including IPIs), as the result it cannot handle > > >SMP call for stack dump. > > > > > >This patch is to enable coresight debug module, so firstly this driver > > >is to bind apb clock for debug module and this is to ensure the debug > > >module can be accessed from program or external debugger. And the driver > > >uses sample-based registers for debug purpose, e.g. when system detects > > >the CPU lockup and trigger panic, the driver will dump program counter > > >and combined context registers (EDCIDSR, EDVIDSR); by parsing context > > >registers so can quickly get to know CPU secure state, exception level, > > >etc. > > > > The problem is, it is not guaranteed that the EDPCSR_Hi, EDCIDSR & EDVIDSR are > > updated as a side effect of a memory mapped access (which is what we do here) to the > > EDPCSR_Lo. > > > > Section H.7.1.2 : Reads of EDPCSRs (in ARM DDI 0487A.k) : > > > > "The indirect writes to EDCIDSR, EDVIDSR, and EDPCSRhi might not occur for a memory-mapped access > > to the external debug interface. For more information, see Memory-mapped accesses to the external debug > > interface on page H8-4968." > > > > So we cannot really rely on the values in EDVIDSR which we use to make further decisions. So I > > am wondering if this is really guranteed to be useful. > > So this is caused by Software lock is locked? > > Section H8.4.1: > > "Reads and writes have no side-effects. A side-effect is where a > direct read or a direct write of a register creates > an indirect write of the same or another register. When the Software > Lock is locked, the indirect write does > not occur." > > > >Some of the debug module registers are located in CPU power domain, so > > >in the driver it has checked the power state for CPU before accessing > > >registers within CPU power domain. For most safe way to use this driver, > > >it's suggested to disable CPU low power states, this can simply set > > >"nohlt" in kernel command line. > > > > > >Signed-off-by: Leo Yan <leo.yan@linaro.org> > > >--- > > > drivers/hwtracing/coresight/Kconfig | 10 + > > > drivers/hwtracing/coresight/Makefile | 1 + > > > drivers/hwtracing/coresight/coresight-debug.c | 377 ++++++++++++++++++++++++++ > > > 3 files changed, 388 insertions(+) > > > create mode 100644 drivers/hwtracing/coresight/coresight-debug.c > > > > > >diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > > >index 130cb21..3ed651e 100644 > > >--- a/drivers/hwtracing/coresight/Kconfig > > >+++ b/drivers/hwtracing/coresight/Kconfig > > >@@ -89,4 +89,14 @@ config CORESIGHT_STM > > > logging useful software events or data coming from various entities > > > in the system, possibly running different OSs > > > > > >+config CORESIGHT_DEBUG > > > > To make it more specific, may be CORESIGHT_CPU_DEBUG ? > > Will fix. > > > >+ bool "CoreSight debug driver" > > > > "Coresight CPU Debug driver" > > Will fix. > > > >+ depends on ARM || ARM64 > > >+ help > > >+ This driver provides support for coresight debugging module. This > > >+ is primarily used to dump sample-based profiling registers for > > >+ panic. To avoid lockups when accessing debug module registers, > > >+ it is safer to disable CPU low power states (like "nohlt" on the > > >+ kernel command line) when using this feature. > > >+ > > > > >+#define EDPCSR_THUMB BIT(0) > > >+#define EDPCSR_ARM_INST_MASK GENMASK(31, 2) > > >+#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1) > > > > We don't need two different masks. {ED/DBG}PCSR has only bit 0 reserved > > for instruction set indication. > > I think we need this two different masks. Please review below extra doc > for PC offset analysis in ARM ARM. > > > >+#endif > > >+ > > >+/* bits definition for EDPRSR */ > > >+#define EDPRSR_DLK BIT(6) > > >+#define EDPRSR_PU BIT(0) > > >+ > > >+ > > >+static void debug_read_regs(struct debug_drvdata *drvdata) > > >+{ > > >+ drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR); > > >+ > > >+ if (!debug_access_permitted(drvdata)) > > >+ return; > > >+ > > >+ if (!drvdata->edpcsr_present) > > >+ return; > > >+ > > >+ CS_UNLOCK(drvdata->base); > > >+ > > >+ debug_os_unlock(drvdata); > > >+ > > >+ drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR); > > >+ > > >+ /* > > >+ * As described in ARM DDI 0487A.k, if the processing > > >+ * element (PE) is in debug state, or sample-based > > >+ * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF; > > >+ * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become > > >+ * UNKNOWN state. So directly bail out for this case. > > >+ */ > > >+ if (drvdata->edpcsr == EDPCSR_PROHIBITED) { > > >+ CS_LOCK(drvdata->base); > > >+ return; > > >+ } > > >+ > > >+ /* > > >+ * A read of the EDPCSR normally has the side-effect of > > >+ * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI; > > >+ * at this point it's safe to read value from them. > > >+ */ > > > > See my comment above about the side effects of memory mapped access. > > Yeah. > > > >+ drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR); > > >+#ifdef CONFIG_64BIT > > >+ drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI); > > >+#endif > > > > >+ > > >+ if (drvdata->edvidsr_present) > > >+ drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR); > > >+ > > >+ CS_LOCK(drvdata->base); > > >+} > > >+ > > > > >+#ifndef CONFIG_64BIT > > > > I guess this doesn't help for an ARMv8 32bit only core (e.g, Cortex-A32). And > > unfortunately, there are conflicting definitions for the values for PCSROffset w.r.t > > ARMv8 and ARMv7. > > > > DBGDEVID1[3:0] For ARMv7 : > > > > 0000 - Sample offset applies based on the instruction state. > > 0001 - No offset applies. > > > > EDDEVID1[3:0] For ARMv8 : > > 0000 - EDPCSR not implemented > > 0010 - EDPCSR implemented without offsets, but do not use in AArch32 state! > > > > So there is no easy way to make sense of the value, unless you know which version > > of the architecture is in use. Or may be we could co-relate it with the value from > > DEVID. > > > > i.e, EDPCSR is not implemented do not register this device, see comments on debug_probe(). > > ( And we should also include the following test for 32bit code to see if edpcsr is implemented. > > See comments on debug_init_arch_data() ) > > > > > > That way, we could use the following inference from the PCSROffset value : > > > > 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0]) > > 0001 - No offset applies. > > 0010 - No offset applies, but do not use in AArch32 mode > > Just now I went through ARM ARM and ARMv8 ARM, this makes sense to me. > Thanks for good pointing for this. > > > >+static bool debug_pc_has_offset(struct debug_drvdata *drvdata) > > >+{ > > >+ u32 pcsr_offset; > > >+ > > >+ pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK; > > >+ > > >+ return (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET); > > >+} > > >+ > > >+static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata, > > >+ unsigned long pc) > > >+{ > > >+ unsigned long arm_inst_offset = 0, thumb_inst_offset = 0; > > >+ > > >+ if (debug_pc_has_offset(drvdata)) { > > >+ arm_inst_offset = 8; > > >+ thumb_inst_offset = 4; > > >+ } > > >+ > > >+ /* Handle thumb instruction */ > > >+ if (pc & EDPCSR_THUMB) { > > >+ pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset; > > >+ return pc; > > >+ } > > >+ > > >+ /* > > >+ * Handle arm instruction offset, if the arm instruction > > >+ * is not 4 byte alignment then it's possible the case > > >+ * for implementation defined; keep original value for this > > >+ * case and print info for notice. > > >+ */ > > >+ if (pc & BIT(1)) > > >+ pr_emerg("Instruction offset is implementation defined\n"); > > > > I am struggling to find the any mention about this in the ARM ARM. Please could > > you point me to it. > > Sure, please see ARM DDI 0406C.b, chapter C11.11.34 " > DBGPCSR, Program Counter Sampling Register": > > A profiling tool can use the value of the T bit to calculate the > instruction address as follows: > > When an offset is applied to the sampled address > - if T is 0 and DBGPCSR[1] is 0, ((DBGPCSR[31:2] << 2) - 8) is the > address of the sampled ARM instruction > - if T is 0 and DBGPCSR[1] is 1, the instruction address is > IMPLEMENTATION DEFINED > - if T is 1, ((DBGPCSR[31:1] << 1) - 4) is the address of the sampled > Thumb or ThumbEE instruction. > > When no offset is applied to the sampled address > - if T is 0 and DBGPCSR[1] is 0, (DBGPCSR[31:2] << 2) is the address > of the sampled ARM instruction > - if T is 0 and DBGPCSR[1] is 1, the instruction address is > IMPLEMENTATION DEFINED > - if T is 1, (DBGPCSR[31:1] << 1) is the address of the sampled Thumb > or ThumbEE instruction. > > > >+static void debug_init_arch_data(void *info) > > >+{ > > >+ struct debug_drvdata *drvdata = info; > > >+ u32 mode; > > >+ > > >+ CS_UNLOCK(drvdata->base); > > >+ > > >+ debug_os_unlock(drvdata); > > >+ > > >+ /* Read device info */ > > >+ drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID); > > >+ drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1); > > >+ > > >+ /* Parse implementation feature */ > > >+ mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE; > > >+ if (mode == EDDEVID_IMPL_FULL) { > > >+ drvdata->edpcsr_present = true; > > >+ drvdata->edvidsr_present = true; > > >+ } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) { > > >+ drvdata->edpcsr_present = true; > > >+ drvdata->edvidsr_present = false; > > > > As discussed above, we need to consult the DEVID1:PCSROffset for AArch32 to decide > > if we have the edpcsr implemented on ARMv8. > > Yeah. > > > >+ } else { > > >+ drvdata->edpcsr_present = false; > > >+ drvdata->edvidsr_present = false; > > >+ } > > >+ > > >+ CS_LOCK(drvdata->base); > > >+} > > >+ > > >+static int debug_probe(struct amba_device *adev, const struct amba_id *id) > > >+{ > > >+ void __iomem *base; > > >+ struct device *dev = &adev->dev; > > >+ struct debug_drvdata *drvdata; > > >+ struct resource *res = &adev->res; > > >+ struct device_node *np = adev->dev.of_node; > > >+ char buf[32]; > > >+ static int debug_count; > > >+ > > >+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > > >+ if (!drvdata) > > >+ return -ENOMEM; > > >+ > > >+ drvdata->cpu = np ? of_coresight_get_cpu(np) : 0; > > >+ 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; > > >+ > > >+ get_online_cpus(); > > >+ per_cpu(debug_drvdata, drvdata->cpu) = drvdata; > > >+ > > >+ if (smp_call_function_single(drvdata->cpu, > > >+ debug_init_arch_data, drvdata, 1)) > > >+ dev_err(dev, "Debug arch init failed\n"); > > > > If this fails (say the CPU was offline), should we still return success ? > > And may be we should check if the drvdata->edpcsr_present to detect if the CPU > > implements the PC Sampling and return failure here if it doesn't. > > Will fix. > > > >+ > > >+ put_online_cpus(); > > >+ > > >+ if (!debug_count++) > > >+ atomic_notifier_chain_register(&panic_notifier_list, > > >+ &debug_notifier); > > >+ > > > > >+ sprintf(buf, (char *)id->data, drvdata->cpu); > > >+ dev_info(dev, "%s initialized\n", buf); > > > > This could simply be : > > dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu); > > > > and get rid of the static string and the buffer, see below. > > > > >+ return 0; > > >+} > > >+ > > >+static struct amba_id debug_ids[] = { > > >+ { /* Debug for Cortex-A53 */ > > >+ .id = 0x000bbd03, > > >+ .mask = 0x000fffff, > > > > ... > > > > >+ .data = "Coresight debug-CPU%d", > > > > I think this is pointless, as the debug area we are interested in is always associated > > with a CPU, we could as well figure out what to print from the drvdata->cpu above. > > I prefer to follow your suggestion for upper two comments; but I'd like > check with Mathieu, due I followed up Mathieu's suggestion to write > current code. The end result is the same - I'm good either way. Thanks, Mathieu > > Thanks, > Leo Yan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote: > On 09/03/17 17:59, Leo Yan wrote: > >Hi Suziku, > >>The problem is, it is not guaranteed that the EDPCSR_Hi, EDCIDSR & EDVIDSR are > >>updated as a side effect of a memory mapped access (which is what we do here) to the > >>EDPCSR_Lo. > >> > >>Section H.7.1.2 : Reads of EDPCSRs (in ARM DDI 0487A.k) : > >> > >>"The indirect writes to EDCIDSR, EDVIDSR, and EDPCSRhi might not occur for a memory-mapped access > >>to the external debug interface. For more information, see Memory-mapped accesses to the external debug > >>interface on page H8-4968." > >> > >>So we cannot really rely on the values in EDVIDSR which we use to make further decisions. So I > >>am wondering if this is really guranteed to be useful. > > > >So this is caused by Software lock is locked? > > > >Section H8.4.1: > > > >"Reads and writes have no side-effects. A side-effect is where a > >direct read or a direct write of a register creates > >an indirect write of the same or another register. When the Software > >Lock is locked, the indirect write does > >not occur." > > Yes, thats correct, further : > > Section H9.2.32: EDPCSR > > "For a read of EDPCSRlo from the memory-mapped interface, if EDLSR.SLK == 1, meaning > the Software Lock is locked, then the access has no side-effects. That is, EDCIDSR, > EDVIDSR, and EDPCSRhi are unchanged." > > And since we do a CS_UNLOCK, that should be fine. Please ignore my comment. > > >>>diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > >>>index 130cb21..3ed651e 100644 > >>>--- a/drivers/hwtracing/coresight/Kconfig > >>>+++ b/drivers/hwtracing/coresight/Kconfig > >>>@@ -89,4 +89,14 @@ config CORESIGHT_STM > >>> logging useful software events or data coming from various entities > >>> in the system, possibly running different OSs > >>> > >>>+config CORESIGHT_DEBUG > >> > >>To make it more specific, may be CORESIGHT_CPU_DEBUG ? > > > >Will fix. > > > >>>+ bool "CoreSight debug driver" > >> > >>"Coresight CPU Debug driver" > > > >Will fix. > > > >>>+ depends on ARM || ARM64 > >>>+ help > >>>+ This driver provides support for coresight debugging module. This > >>>+ is primarily used to dump sample-based profiling registers for > >>>+ panic. To avoid lockups when accessing debug module registers, > >>>+ it is safer to disable CPU low power states (like "nohlt" on the > >>>+ kernel command line) when using this feature. > >>>+ > >> > >>>+#define EDPCSR_THUMB BIT(0) > >>>+#define EDPCSR_ARM_INST_MASK GENMASK(31, 2) > >>>+#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1) > >> > >>We don't need two different masks. {ED/DBG}PCSR has only bit 0 reserved > >>for instruction set indication. > > > >I think we need this two different masks. Please review below extra doc > >for PC offset analysis in ARM ARM. > > You're correct. Thanks for the pointer. I got confused, as there was no bit > dedicated in the DBGPCSR bit assignment figure. > > >>>+ /* > >>>+ * Handle arm instruction offset, if the arm instruction > >>>+ * is not 4 byte alignment then it's possible the case > >>>+ * for implementation defined; keep original value for this > >>>+ * case and print info for notice. > >>>+ */ > >>>+ if (pc & BIT(1)) > >>>+ pr_emerg("Instruction offset is implementation defined\n"); > >> > >>I am struggling to find the any mention about this in the ARM ARM. Please could > >>you point me to it. > > > >Sure, please see ARM DDI 0406C.b, chapter C11.11.34 " > >DBGPCSR, Program Counter Sampling Register": > > > >A profiling tool can use the value of the T bit to calculate the > >instruction address as follows: > > > >When an offset is applied to the sampled address > >- if T is 0 and DBGPCSR[1] is 0, ((DBGPCSR[31:2] << 2) - 8) is the > >address of the sampled ARM instruction > >- if T is 0 and DBGPCSR[1] is 1, the instruction address is > >IMPLEMENTATION DEFINED > >- if T is 1, ((DBGPCSR[31:1] << 1) - 4) is the address of the sampled > >Thumb or ThumbEE instruction. > > > >When no offset is applied to the sampled address > >- if T is 0 and DBGPCSR[1] is 0, (DBGPCSR[31:2] << 2) is the address > >of the sampled ARM instruction > >- if T is 0 and DBGPCSR[1] is 1, the instruction address is > >IMPLEMENTATION DEFINED > >- if T is 1, (DBGPCSR[31:1] << 1) is the address of the sampled Thumb > >or ThumbEE instruction. > > > > Ok. > > > >>>+ > >>>+ put_online_cpus(); > >>>+ > >>>+ if (!debug_count++) > >>>+ atomic_notifier_chain_register(&panic_notifier_list, > >>>+ &debug_notifier); > >>>+ > >> > >>>+ sprintf(buf, (char *)id->data, drvdata->cpu); > >>>+ dev_info(dev, "%s initialized\n", buf); > >> > >>This could simply be : > >> dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu); > >> > >>and get rid of the static string and the buffer, see below. > > Also we need pm_runtime_put() here to balance the pm_runtime_get_ from AMBA > device probe. Good point. > More on that below. > > >> > >>>+ return 0; > >>>+} > >>>+ > >>>+static struct amba_id debug_ids[] = { > >>>+ { /* Debug for Cortex-A53 */ > >>>+ .id = 0x000bbd03, > >>>+ .mask = 0x000fffff, > >> > >>... > >> > >>>+ .data = "Coresight debug-CPU%d", > >> > >>I think this is pointless, as the debug area we are interested in is always associated > >>with a CPU, we could as well figure out what to print from the drvdata->cpu above. > > > >I prefer to follow your suggestion for upper two comments; but I'd like > >check with Mathieu, due I followed up Mathieu's suggestion to write > >current code. > > Btw, I don't see any PM calls to make sure the power domain (at least the debug domain) > is up, which could cause problems with accesses to some of these registers (leave alone the > ones in CPU power domain), especially the EDPRSR. We could also do pm_runtime_get on the > CPU's power domain, if the CPU is online, before we access the pcsr. I thought about PM runtime operations a little while back but wondered if it is really a good thing to have them around. When this code is called the system has crashed and as such making PM runtimes call isn't a good idea. One thing we could do is _not_ call pm_runtime_put() at the end of the probe() operation. That way we wouldn't have to mess around with PM runtime operations on an unstable system. This, of course, is costly in terms of power consumption but the system is under test/debug anyway. Thoughts? > > Suzuki > > > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 March 2017 at 10:44, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > On 13/03/17 16:56, Mathieu Poirier wrote: >> >> On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote: >>>>>> >>>>>> + >>>>>> + put_online_cpus(); >>>>>> + >>>>>> + if (!debug_count++) >>>>>> + atomic_notifier_chain_register(&panic_notifier_list, >>>>>> + &debug_notifier); >>>>>> + >>>>> >>>>> >>>>>> + sprintf(buf, (char *)id->data, drvdata->cpu); >>>>>> + dev_info(dev, "%s initialized\n", buf); >>>>> >>>>> >>>>> This could simply be : >>>>> dev_info(dev, "Coresight debug-CPU%d initialized\n", >>>>> drvdata->cpu); >>>>> >>>>> and get rid of the static string and the buffer, see below. >>> >>> >>> Also we need pm_runtime_put() here to balance the pm_runtime_get_ from >>> AMBA >>> device probe. >> >> >> Good point. >> >>> More on that below. >>> >>>>> >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static struct amba_id debug_ids[] = { >>>>>> + { /* Debug for Cortex-A53 */ >>>>>> + .id = 0x000bbd03, >>>>>> + .mask = 0x000fffff, >>>>> >>>>> >>>>> ... >>>>> >>>>>> + .data = "Coresight debug-CPU%d", >>>>> >>>>> >>>>> I think this is pointless, as the debug area we are interested in is >>>>> always associated >>>>> with a CPU, we could as well figure out what to print from the >>>>> drvdata->cpu above. >>>> >>>> >>>> I prefer to follow your suggestion for upper two comments; but I'd like >>>> check with Mathieu, due I followed up Mathieu's suggestion to write >>>> current code. >>> >>> >>> Btw, I don't see any PM calls to make sure the power domain (at least the >>> debug domain) >>> is up, which could cause problems with accesses to some of these >>> registers (leave alone the >>> ones in CPU power domain), especially the EDPRSR. We could also do >>> pm_runtime_get on the >>> CPU's power domain, if the CPU is online, before we access the pcsr. >> >> >> I thought about PM runtime operations a little while back but wondered if >> it is >> really a good thing to have them around. When this code is called the >> system >> has crashed and as such making PM runtimes call isn't a good idea. > > > You are right. It is not safe to make such calls when we have crashed. > The other side effect is, if we don't have the debug power domain up, > we could possibly hang the system and prevent other registered notifiers > from running, which doesn't sound good either. > >> >> One thing we could do is _not_ call pm_runtime_put() at the end of the >> probe() >> operation. That way we wouldn't have to mess around with PM runtime >> operations >> on an unstable system. This, of course, is costly in terms of power >> consumption >> but the system is under test/debug anyway. > > > May be control the behavior via kernel command line ? Something like > coresight_debug={on or 1} or > even use the "nohlt" ? We need to deal with the debug and CPU power domains. For the former I suggest we do what coresight does and use the "power-domains" binding[1]. For the CPU power domain we can re-use the "nohlt" flag. In the probe function if the "nohlt" cmd line flag is not set the code bails out. If it is set pm_runtime_put() is _not_ called and the driver can be used without worries of hanging the system when the panic handler is invoked. Am I forgetting something? [1]. http://lxr.free-electrons.com/source/arch/arm64/boot/dts/arm/juno-base.dtsi#L137 > > Suzuki -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 15, 2017 at 02:41:59PM -0600, Mathieu Poirier wrote: > On 15 March 2017 at 10:44, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > > On 13/03/17 16:56, Mathieu Poirier wrote: > >> On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote: [...] > >>> Btw, I don't see any PM calls to make sure the power domain (at least the > >>> debug domain) > >>> is up, which could cause problems with accesses to some of these > >>> registers (leave alone the > >>> ones in CPU power domain), especially the EDPRSR. We could also do > >>> pm_runtime_get on the > >>> CPU's power domain, if the CPU is online, before we access the pcsr. > >> > >> > >> I thought about PM runtime operations a little while back but wondered if > >> it is > >> really a good thing to have them around. When this code is called the > >> system > >> has crashed and as such making PM runtimes call isn't a good idea. > > > > > > You are right. It is not safe to make such calls when we have crashed. > > The other side effect is, if we don't have the debug power domain up, > > we could possibly hang the system and prevent other registered notifiers > > from running, which doesn't sound good either. > > > >> > >> One thing we could do is _not_ call pm_runtime_put() at the end of the > >> probe() > >> operation. That way we wouldn't have to mess around with PM runtime > >> operations > >> on an unstable system. This, of course, is costly in terms of power > >> consumption > >> but the system is under test/debug anyway. > > > > > > May be control the behavior via kernel command line ? Something like > > coresight_debug={on or 1} or > > even use the "nohlt" ? > > We need to deal with the debug and CPU power domains. > > For the former I suggest we do what coresight does and use the > "power-domains" binding[1]. For the CPU power domain we can re-use > the "nohlt" flag. In the probe function if the "nohlt" cmd line flag > is not set the code bails out. If it is set pm_runtime_put() is _not_ > called and the driver can be used without worries of hanging the > system when the panic handler is invoked. > > Am I forgetting something? I tested this drvier on Hikey and DB410c. For Hikey we must pass "nohlt" to disable low power states, otherwise the kernel will hang during initialization. But for DB410c, this driver even can work well without "nohlt" and I checked the CPUIdle has been really enabled with below sysfs entries: root@linaro-developer:~# cat /sys/devices/system/cpu/cpu*/cpuidle/state*/usage 5992 6988 4225 4547 2790 23696 4202 3899 And from my previous experience, I'm quite sure some SoCs can access the debug module registers with CPUIdle enabled, and it will read back 0xFFFF_FFFF_FFFF_FFFF when the CPU stays in low power state. So I prefer we could keep current method to suggest to use "nohlt" in Kconfig's help description but it's not mandotory to check this in the code. How about you think for this? Thanks, Leo Yan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 17, 2017 at 06:13:28PM +0800, Leo Yan wrote: > On Wed, Mar 15, 2017 at 02:41:59PM -0600, Mathieu Poirier wrote: > > On 15 March 2017 at 10:44, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > > > On 13/03/17 16:56, Mathieu Poirier wrote: > > >> On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote: > > [...] > > > >>> Btw, I don't see any PM calls to make sure the power domain (at least the > > >>> debug domain) > > >>> is up, which could cause problems with accesses to some of these > > >>> registers (leave alone the > > >>> ones in CPU power domain), especially the EDPRSR. We could also do > > >>> pm_runtime_get on the > > >>> CPU's power domain, if the CPU is online, before we access the pcsr. > > >> > > >> > > >> I thought about PM runtime operations a little while back but wondered if > > >> it is > > >> really a good thing to have them around. When this code is called the > > >> system > > >> has crashed and as such making PM runtimes call isn't a good idea. > > > > > > > > > You are right. It is not safe to make such calls when we have crashed. > > > The other side effect is, if we don't have the debug power domain up, > > > we could possibly hang the system and prevent other registered notifiers > > > from running, which doesn't sound good either. > > > > > >> > > >> One thing we could do is _not_ call pm_runtime_put() at the end of the > > >> probe() > > >> operation. That way we wouldn't have to mess around with PM runtime > > >> operations > > >> on an unstable system. This, of course, is costly in terms of power > > >> consumption > > >> but the system is under test/debug anyway. > > > > > > > > > May be control the behavior via kernel command line ? Something like > > > coresight_debug={on or 1} or > > > even use the "nohlt" ? > > > > We need to deal with the debug and CPU power domains. > > > > For the former I suggest we do what coresight does and use the > > "power-domains" binding[1]. For the CPU power domain we can re-use > > the "nohlt" flag. In the probe function if the "nohlt" cmd line flag > > is not set the code bails out. If it is set pm_runtime_put() is _not_ > > called and the driver can be used without worries of hanging the > > system when the panic handler is invoked. > > > > Am I forgetting something? > > I tested this drvier on Hikey and DB410c. For Hikey we must pass > "nohlt" to disable low power states, otherwise the kernel will hang > during initialization. But for DB410c, this driver even can work well > without "nohlt" and I checked the CPUIdle has been really enabled with > below sysfs entries: > > root@linaro-developer:~# cat /sys/devices/system/cpu/cpu*/cpuidle/state*/usage > 5992 > 6988 > 4225 > 4547 > 2790 > 23696 > 4202 > 3899 > > And from my previous experience, I'm quite sure some SoCs can access > the debug module registers with CPUIdle enabled, and it will read > back 0xFFFF_FFFF_FFFF_FFFF when the CPU stays in low power state. > So I prefer we could keep current method to suggest to use "nohlt" in > Kconfig's help description but it's not mandotory to check this in > the code. How about you think for this? If we don't check for "nohlt" some platform may freeze, others may work. If we mandate that "nohlt" be present on the kernel cmd line it works in all cases. As such mandating that "nohlt" be present is a better way to go. Mathieu > > Thanks, > Leo Yan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 17, 2017 at 09:50:07AM -0600, Mathieu Poirier wrote: [...] > If we don't check for "nohlt" some platform may freeze, others may work. If we > mandate that "nohlt" be present on the kernel cmd line it works in all cases. > As such mandating that "nohlt" be present is a better way to go. Sure, so I will add below checking code in the probe function, please let me know if you have alter better way to implement this: + if (IS_ENABLED(CONFIG_CPU_IDLE) && + !strstr(boot_command_line, "nohlt")) { + dev_err(dev, "May not be accessible in CPU power domain.\n"); + return -EPERM; + } Thanks, Leo Yan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 21, 2017 at 10:16:45AM +0000, Suzuki K Poulose wrote: [...] > >>In my opinion booting with "nohlt" on the cmd line is sufficient to > >>determine if we should use the driver or not. That way we also avoid > >>declaring yet another sysfs flag, something I really want to avoid. > > > >Agree. > > > >I did spend some time to implement coresight core framework to support > >debug module, you could see it on:http://termbin.com/k2fj; this also > >gives me more sense which is better choice. If declaring another sysfs > >flag to support debug module in coresight framework, this lets the > >codes and interfaces more complex. E.g. for best fit into coresight > >framework, finally we can get 8 sysfs nodes for 8 CPUs in system; so > >that means we need enable every CPU one by one. > > Having a node for each debug area indeed doesn't look good. We could > as will stick a single node under /sys/kernel/debug/ which would enable/disable > the debug component. > > I am OK with it being tied to nohlt. In that case we will have to add > a Kconfig dependency on GENERIC_IDLE_POLL_SETUP (though it is selected > by default on ARM/ARM64). Parsing the boot command line for nohlt doesn't > look like a good idea. We may have to figure out a way to do that. I remembered that we can use QoS constraint for CPUIdle: pm_qos_add_request(pm_qos_req, PM_QOS_CPU_DMA_LATENCY, 0); Using this way we can disable all low power states and don't depend on nohlt anymore. > Also, please could you add support for building this as a module ? Since it > doesn't depend on the coresight bus anyway, it should be pretty straight > forward. Yeah. Will support module building. Thanks, Leo Yan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 21, 2017 at 07:47:11PM +0800, Leo Yan wrote: > On Tue, Mar 21, 2017 at 10:16:45AM +0000, Suzuki K Poulose wrote: > > [...] > > > >>In my opinion booting with "nohlt" on the cmd line is sufficient to > > >>determine if we should use the driver or not. That way we also avoid > > >>declaring yet another sysfs flag, something I really want to avoid. > > > > > >Agree. > > > > > >I did spend some time to implement coresight core framework to support > > >debug module, you could see it on:http://termbin.com/k2fj; this also > > >gives me more sense which is better choice. If declaring another sysfs > > >flag to support debug module in coresight framework, this lets the > > >codes and interfaces more complex. E.g. for best fit into coresight > > >framework, finally we can get 8 sysfs nodes for 8 CPUs in system; so > > >that means we need enable every CPU one by one. > > > > Having a node for each debug area indeed doesn't look good. We could > > as will stick a single node under /sys/kernel/debug/ which would enable/disable > > the debug component. > > > > I am OK with it being tied to nohlt. In that case we will have to add > > a Kconfig dependency on GENERIC_IDLE_POLL_SETUP (though it is selected > > by default on ARM/ARM64). Parsing the boot command line for nohlt doesn't > > look like a good idea. We may have to figure out a way to do that. > > I remembered that we can use QoS constraint for CPUIdle: > pm_qos_add_request(pm_qos_req, PM_QOS_CPU_DMA_LATENCY, 0); > > Using this way we can disable all low power states and don't depend on > nohlt anymore. The idea is to use "nohlt" as a trigger option. If it is set then we use this driver - if it isn't then it is kept under wrap. > > > Also, please could you add support for building this as a module ? Since it > > doesn't depend on the coresight bus anyway, it should be pretty straight > > forward. > > Yeah. Will support module building. > > Thanks, > Leo Yan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/03/17 12:54, Mike Leach wrote: > > > On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com > <mailto:sudeep.holla@arm.com>> wrote: > [...] > I disagree with this approach. One of the main usefulness of such > self hosted debug feature is to debug issues around features like > cpuidle. Adding constraints like "cpuidle needs to be disabled" is > not good IMO. There are ways to make it work with cpuidle enabled. > Please explore them. In particular refer H9.2.39 EDPRCR, External > Debug Power/Reset Control Register. > > So, "nohlt" option is not an option. I prefer some sysfs option like > Suzuki suggested to enable this feature on demand if power saving in > normal usecase is the concern. Using "nohlt" just disables idle and > doesn't ensure the debug power domain is ON. Using the flag directly > in this driver to enable debug power domain also sounds misuse of > that flag for me. > > I think the key issue to remember here is that experience with > external debug shows that CPU Idle means different things to > different SoC designs / power management schemes. (and we are using > external debug in a self hosted way here). > Yes agreed on the point that meaning of "cpuidle" differs on each SoC. > Some designs will power down an entire cluster if all CPUs on the > cluster are powered down - including the parts of the debug > registers that should remain powered in the debug power domain. Interesting, at-least ETMv4 or some other coresight specification clearly classify the power domains and the register access. The actual power domain itself may vary depending on implementation. > The bits in EDPRCR are not respected in these cases - these designs > do not really support debug over power down in the way that the > CoreSight / Debug designers anticipated. This means that even > checking EDPRSR has the potential to cause a bus hang if the target > register is unpowered. (and if the debug power domain is unpowered > then the PC data is also lost). > Agreed, but can we start supporting the sane designs in sane way first. We can always add compatible and handle deviations. I agree we may need to support such deviations but starting with that seems setting a bad example. > In these cases, accessing to the debug registers while they are not > powered is a recipe for disaster - so preventing CPUIdle ​and the > subsequent cluster power down ​ allows investigation on this class of > system - ​and allowing the CPUs of interest be interrogated without > hanging the crash log process.​ > Agreed. But my point is that many issues are around cpuidle and some usecase and just eliminating that use-case sounds bad. For me, core-sight was most useful to debug issues around cpu power management and lockups where we can't stop cores but examine these registers. There are other alternatives for other use-cases IMO. > > ​On systems that do behave correctly with respect to debug power > domains, then disabling CPUIdle is unnecessary - these can be > controlled by ​EDPRCR - perhaps; per the specification it is > "implementation defined" if writing bits to this register have an > effect on the system anyway even if the debug domain is correctly > powered. > We can always do that unconditionally. If implementations don't honor those bits, it's different. If they hang on accessing something which is on debug power domain and not on core power domain, then you have much bigger issue to solve. How can you even trust and make any other register accesses that are in debug power domain then ? > ​While it is true to say that disabling CPUIdle does not guarantee > that the debug power domain is on, it does in a certain class of > designs prevent it being powered off (Juno historically - not sure if > that is still the case.). > Again it's completely platform specific. All you need to care is that the debug power domain is on or not. Disabling CPUIdle to achieve that is simply wrong and may work only on few platforms. > However, I do agree that the use of the driver should not be > triggered _only_ on the existence of /nohlt on the command line - ​ > there is a class of designs where this will not be required. > Thanks > When enabing the driver as a kernel config the user needs to > decide:- 1) do I need this to debug the issue I am seeing 2) does the > power management on my system require I use /nohlt as well. Please don't *misuse* nohlt to disable idle. There are other ways to do the same either from the user-space or from the driver. > > I think that the use of /nohlt as an option, and the reasons why it > might be needed should be part of the configuration help in this > case. > > There is also a case for considering if there should be an option to > configure it to be enabled or disabled at boot time. It is easy to > imagine cases I want to have this running from the start as a crash > happens early - and cases I can enable it on demand later. > Also consider with cpuidle enabled ;). I can help testing if needed. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 March 2017 at 14:07, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 22/03/17 12:54, Mike Leach wrote: >> >> >> On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com >> <mailto:sudeep.holla@arm.com>> wrote: >> > [...] > >> I disagree with this approach. One of the main usefulness of such >> self hosted debug feature is to debug issues around features like >> cpuidle. Adding constraints like "cpuidle needs to be disabled" is >> not good IMO. There are ways to make it work with cpuidle enabled. >> Please explore them. In particular refer H9.2.39 EDPRCR, External >> Debug Power/Reset Control Register. >> >> So, "nohlt" option is not an option. I prefer some sysfs option like >> Suzuki suggested to enable this feature on demand if power saving in >> normal usecase is the concern. Using "nohlt" just disables idle and >> doesn't ensure the debug power domain is ON. Using the flag directly >> in this driver to enable debug power domain also sounds misuse of >> that flag for me. >> >> I think the key issue to remember here is that experience with >> external debug shows that CPU Idle means different things to >> different SoC designs / power management schemes. (and we are using >> external debug in a self hosted way here). >> > > Yes agreed on the point that meaning of "cpuidle" differs on each SoC. > >> Some designs will power down an entire cluster if all CPUs on the >> cluster are powered down - including the parts of the debug >> registers that should remain powered in the debug power domain. > > Interesting, at-least ETMv4 or some other coresight specification > clearly classify the power domains and the register access. The actual > power domain itself may vary depending on implementation. Yes - the ETMv4 spec defines what should be in the core / debug power domains, but there is no architectural requirement for these to be separate. Most of power management is "implementation defined", & hw designers seem to have different criteria than sw engineers wanting to debug stuff. > >> The bits in EDPRCR are not respected in these cases - these designs >> do not really support debug over power down in the way that the >> CoreSight / Debug designers anticipated. This means that even >> checking EDPRSR has the potential to cause a bus hang if the target >> register is unpowered. (and if the debug power domain is unpowered >> then the PC data is also lost). >> > > Agreed, but can we start supporting the sane designs in sane way first. > We can always add compatible and handle deviations. I agree we may need > to support such deviations but starting with that seems setting a bad > example. > From a pragmatic point of view, we have to support the designs that we have and are currently using. Sadly this might include some that do not behave in an ideal way. I'm not saying disabling CPUIdle is right for all cases, or perhaps many, but it has in the past been useful in specific instances - not just for external debug, but to use CoreSight trace etc, were powering down the a CPU / cluster takes out ETM accesses and breaks stuff. The key is that to use this driver, the user has to be aware of the PM implications on their specific system - the kernel may not take care of it all for them - as SCP type power controllers are often external and may have unique firmware and capabilites. Historically CPUIdle disable has been used as a blunt instrument to handle power management problems in real debug use cases - but it is one that has been successful. Where there are better methods then I am all for using these. >> In these cases, accessing to the debug registers while they are not >> powered is a recipe for disaster - so preventing CPUIdle and the >> subsequent cluster power down allows investigation on this class of >> system - and allowing the CPUs of interest be interrogated without >> hanging the crash log process. >> > > Agreed. But my point is that many issues are around cpuidle and some > usecase and just eliminating that use-case sounds bad. For me, > core-sight was most useful to debug issues around cpu power management > and lockups where we can't stop cores but examine these registers. > There are other alternatives for other use-cases IMO. For your case, removing what you are interested in debugging is evidently counter productive, so other techniques need to be used to ensure the CS regs remain alive. But equally there could be use cases where this might be just fine or even the only way. > >> >> On systems that do behave correctly with respect to debug power >> domains, then disabling CPUIdle is unnecessary - these can be >> controlled by EDPRCR - perhaps; per the specification it is >> "implementation defined" if writing bits to this register have an >> effect on the system anyway even if the debug domain is correctly >> powered. >> > > We can always do that unconditionally. If implementations don't honor > those bits, it's different. If they hang on accessing something which is > on debug power domain and not on core power domain, then you have much > bigger issue to solve. How can you even trust and make any other > register accesses that are in debug power domain then ? > It is difficult and highly platform dependent. For external debug we might have per-platform rules built into the debugger on what can and cannot be done and when, plus on occasion some power management scripts. Those platforms that get closest to the "standard" CoreSight power management are easiest to debug. >> While it is true to say that disabling CPUIdle does not guarantee >> that the debug power domain is on, it does in a certain class of >> designs prevent it being powered off (Juno historically - not sure if >> that is still the case.). >> > > Again it's completely platform specific. All you need to care is that > the debug power domain is on or not. Disabling CPUIdle to achieve that > is simply wrong and may work only on few platforms. > >> However, I do agree that the use of the driver should not be >> triggered _only_ on the existence of /nohlt on the command line - >> there is a class of designs where this will not be required. >> > > Thanks > >> When enabing the driver as a kernel config the user needs to >> decide:- 1) do I need this to debug the issue I am seeing 2) does the >> power management on my system require I use /nohlt as well. > > Please don't *misuse* nohlt to disable idle. There are other ways to > do the same either from the user-space or from the driver. I'm not advocating /nohlt above anything else - it's just what is being discussed here. Furthermore, no one debug technique is ever going to be appropriate in all circumstances - debug and trace are always a compromise. I initially raised the issue of clusters powering down, and possibility that no CPUIdle might prevent this, to ensure that awareness is built in to driver / config / help text /documentation that these are real issues seen in the external debug world. The key point is that the caveat in using this driver is that the power management has to be considered on a platform specific basis before it is configured; and appropriate actions may be needed for it to work correctly. Without this then the driver could cause more issues than it debugs. A user selecting this _must_ be told about these issues > >> >> I think that the use of /nohlt as an option, and the reasons why it >> might be needed should be part of the configuration help in this >> case. >> >> There is also a case for considering if there should be an option to >> configure it to be enabled or disabled at boot time. It is easy to >> imagine cases I want to have this running from the start as a crash >> happens early - and cases I can enable it on demand later. >> > > Also consider with cpuidle enabled ;). I can help testing if needed. > > -- > Regards, > Sudeep Regards Mike -- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/03/17 15:45, Mike Leach wrote: > On 22 March 2017 at 14:07, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 22/03/17 12:54, Mike Leach wrote: >>> >>> >>> On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com >>> <mailto:sudeep.holla@arm.com>> wrote: >>> >> [...] >> >>> I disagree with this approach. One of the main usefulness of such >>> self hosted debug feature is to debug issues around features like >>> cpuidle. Adding constraints like "cpuidle needs to be disabled" is >>> not good IMO. There are ways to make it work with cpuidle enabled. >>> Please explore them. In particular refer H9.2.39 EDPRCR, External >>> Debug Power/Reset Control Register. >>> >>> So, "nohlt" option is not an option. I prefer some sysfs option like >>> Suzuki suggested to enable this feature on demand if power saving in >>> normal usecase is the concern. Using "nohlt" just disables idle and >>> doesn't ensure the debug power domain is ON. Using the flag directly >>> in this driver to enable debug power domain also sounds misuse of >>> that flag for me. >>> >>> I think the key issue to remember here is that experience with >>> external debug shows that CPU Idle means different things to >>> different SoC designs / power management schemes. (and we are using >>> external debug in a self hosted way here). >>> >> >> Yes agreed on the point that meaning of "cpuidle" differs on each SoC. >> >>> Some designs will power down an entire cluster if all CPUs on the >>> cluster are powered down - including the parts of the debug >>> registers that should remain powered in the debug power domain. >> >> Interesting, at-least ETMv4 or some other coresight specification >> clearly classify the power domains and the register access. The actual >> power domain itself may vary depending on implementation. > > Yes - the ETMv4 spec defines what should be in the core / debug power > domains, but there is no architectural requirement for these to be > separate. > Most of power management is "implementation defined", & hw designers > seem to have different criteria than sw engineers wanting to debug > stuff. > Yes I agree, no argument there. >>> The bits in EDPRCR are not respected in these cases - these designs >>> do not really support debug over power down in the way that the >>> CoreSight / Debug designers anticipated. This means that even >>> checking EDPRSR has the potential to cause a bus hang if the target >>> register is unpowered. (and if the debug power domain is unpowered >>> then the PC data is also lost). >>> >> >> Agreed, but can we start supporting the sane designs in sane way first. >> We can always add compatible and handle deviations. I agree we may need >> to support such deviations but starting with that seems setting a bad >> example. >> > > From a pragmatic point of view, we have to support the designs that we > have and are currently using. > Sadly this might include some that do not behave in an ideal way. > We will have to support them. But I don't want that to be defacto standard. They should be treated as deviations. Though specification says the behavior is IMPDEF, it does provide standard interface(EDPRCR) to use and we should have that in the driver IMO. > I'm not saying disabling CPUIdle is right for all cases, or perhaps > many, but it has in the past been useful in specific instances - not > just for external debug, Yes I know but that's either issue with the firmware or the debugger and we can work them around in user-space too instead of baking the solution to kernel. > but to use CoreSight trace etc, were powering > down the a CPU / cluster takes out ETM accesses and breaks stuff. > FYI, I added support in ETMv4 to emulate power-down during active trace session and so far I have not seen any report on things breaking because of that register access(may be no one has tested it on other platforms yet :() > The key is that to use this driver, the user has to be aware of the PM > implications on their specific system - the kernel may not take care > of it all for them - as SCP type power controllers are often external > and may have unique firmware and capabilites. > Sure > Historically CPUIdle disable has been used as a blunt instrument to > handle power management problems in real debug use cases - but it is > one that has been successful. Where there are better methods then I am > all for using these. > Cool, thanks. Lets try out that and see if it helps first before bluntly advertising CPUIdle disabling and that too in misleading ways like "nohlt" >>> In these cases, accessing to the debug registers while they are not >>> powered is a recipe for disaster - so preventing CPUIdle and the >>> subsequent cluster power down allows investigation on this class of >>> system - and allowing the CPUs of interest be interrogated without >>> hanging the crash log process. >>> >> >> Agreed. But my point is that many issues are around cpuidle and some >> usecase and just eliminating that use-case sounds bad. For me, >> core-sight was most useful to debug issues around cpu power management >> and lockups where we can't stop cores but examine these registers. >> There are other alternatives for other use-cases IMO. > > For your case, removing what you are interested in debugging is > evidently counter productive, so other techniques need to be used to > ensure the CS regs remain alive. > But equally there could be use cases where this might be just fine or > even the only way. > Again not arguing on that. Just saying we can try out if that solves the issue. As I said there are other ways to disable idle and we can advertise those instead of such misuse. Also those alternative methods are runtime and can be used when you need them. >>> >>> On systems that do behave correctly with respect to debug power >>> domains, then disabling CPUIdle is unnecessary - these can be >>> controlled by EDPRCR - perhaps; per the specification it is >>> "implementation defined" if writing bits to this register have an >>> effect on the system anyway even if the debug domain is correctly >>> powered. >>> >> >> We can always do that unconditionally. If implementations don't honor >> those bits, it's different. If they hang on accessing something which is >> on debug power domain and not on core power domain, then you have much >> bigger issue to solve. How can you even trust and make any other >> register accesses that are in debug power domain then ? >> > > It is difficult and highly platform dependent. For external debug we > might have per-platform rules built into the debugger on what can and > cannot be done and when, plus on occasion some power management > scripts. Those platforms that get closest to the "standard" CoreSight > power management are easiest to debug. > Absolutely, so just mandating might just solve issue *accidentally* not *intentionally*. So I don't want it to be advertised in that way and that becomes defacto. [...] > > I'm not advocating /nohlt above anything else - it's just what is > being discussed here. Furthermore, no one debug technique is ever > going to be appropriate in all circumstances - debug and trace are > always a compromise. > True. > I initially raised the issue of clusters powering down, and > possibility that no CPUIdle might prevent this, to ensure that > awareness is built in to driver / config / help text /documentation > that these are real issues seen in the external debug world. > Point taken. So we could just specify that all necessary power domains need to be on for proper functionality for this feature and that it's highly platform specific instead of mixing cpu/cluster idle details here. > The key point is that the caveat in using this driver is that the > power management has to be considered on a platform specific basis > before it is configured; and appropriate actions may be needed for it > to work correctly. Without this then the driver could cause more > issues than it debugs. A user selecting this _must_ be told about > these issues > Exactly and the help text could be something on the similar lines as you mention/summarize above. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/03/17 16:17, Sudeep Holla wrote: > > > On 22/03/17 15:45, Mike Leach wrote: >> On 22 March 2017 at 14:07, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> >>> >>> On 22/03/17 12:54, Mike Leach wrote: >>>> >>>> >>>> On 21 March 2017 at 15:39, Sudeep Holla <sudeep.holla@arm.com >>>> <mailto:sudeep.holla@arm.com>> wrote: >>>> >>> [...] >>> >>>> I disagree with this approach. One of the main usefulness of such >>>> self hosted debug feature is to debug issues around features like >>>> cpuidle. Adding constraints like "cpuidle needs to be disabled" is >>>> not good IMO. There are ways to make it work with cpuidle enabled. >>>> Please explore them. In particular refer H9.2.39 EDPRCR, External >>>> Debug Power/Reset Control Register. >>>> >>>> So, "nohlt" option is not an option. I prefer some sysfs option like >>>> Suzuki suggested to enable this feature on demand if power saving in >>>> normal usecase is the concern. Using "nohlt" just disables idle and >>>> doesn't ensure the debug power domain is ON. Using the flag directly >>>> in this driver to enable debug power domain also sounds misuse of >>>> that flag for me. >>>> >>>> I think the key issue to remember here is that experience with >>>> external debug shows that CPU Idle means different things to >>>> different SoC designs / power management schemes. (and we are using >>>> external debug in a self hosted way here). >>>> >>> >>> Yes agreed on the point that meaning of "cpuidle" differs on each SoC. >>> >>>> Some designs will power down an entire cluster if all CPUs on the >>>> cluster are powered down - including the parts of the debug >>>> registers that should remain powered in the debug power domain. >>> >>> Interesting, at-least ETMv4 or some other coresight specification >>> clearly classify the power domains and the register access. The actual >>> power domain itself may vary depending on implementation. >> >> Yes - the ETMv4 spec defines what should be in the core / debug power >> domains, but there is no architectural requirement for these to be >> separate. >> Most of power management is "implementation defined", & hw designers >> seem to have different criteria than sw engineers wanting to debug >> stuff. >> > > Yes I agree, no argument there. > >>>> The bits in EDPRCR are not respected in these cases - these designs >>>> do not really support debug over power down in the way that the >>>> CoreSight / Debug designers anticipated. This means that even >>>> checking EDPRSR has the potential to cause a bus hang if the target >>>> register is unpowered. (and if the debug power domain is unpowered >>>> then the PC data is also lost). >>>> >>> >>> Agreed, but can we start supporting the sane designs in sane way first. >>> We can always add compatible and handle deviations. I agree we may need >>> to support such deviations but starting with that seems setting a bad >>> example. >>> >> >> From a pragmatic point of view, we have to support the designs that we >> have and are currently using. >> Sadly this might include some that do not behave in an ideal way. >> > > We will have to support them. But I don't want that to be defacto > standard. They should be treated as deviations. Though specification > says the behavior is IMPDEF, it does provide standard interface(EDPRCR) > to use and we should have that in the driver IMO. > >> I'm not saying disabling CPUIdle is right for all cases, or perhaps >> many, but it has in the past been useful in specific instances - not >> just for external debug, > > Yes I know but that's either issue with the firmware or the debugger > and we can work them around in user-space too instead of baking the > solution to kernel. > >> but to use CoreSight trace etc, were powering >> down the a CPU / cluster takes out ETM accesses and breaks stuff. >> > > FYI, I added support in ETMv4 to emulate power-down during active trace > session and so far I have not seen any report on things breaking because > of that register access(may be no one has tested it on other platforms > yet :() > >> The key is that to use this driver, the user has to be aware of the PM >> implications on their specific system - the kernel may not take care >> of it all for them - as SCP type power controllers are often external >> and may have unique firmware and capabilites. >> > > Sure > >> Historically CPUIdle disable has been used as a blunt instrument to >> handle power management problems in real debug use cases - but it is >> one that has been successful. Where there are better methods then I am >> all for using these. >> > > Cool, thanks. Lets try out that and see if it helps first before bluntly > advertising CPUIdle disabling and that too in misleading ways like "nohlt" > >>>> In these cases, accessing to the debug registers while they are not >>>> powered is a recipe for disaster - so preventing CPUIdle and the >>>> subsequent cluster power down allows investigation on this class of >>>> system - and allowing the CPUs of interest be interrogated without >>>> hanging the crash log process. >>>> >>> >>> Agreed. But my point is that many issues are around cpuidle and some >>> usecase and just eliminating that use-case sounds bad. For me, >>> core-sight was most useful to debug issues around cpu power management >>> and lockups where we can't stop cores but examine these registers. >>> There are other alternatives for other use-cases IMO. >> >> For your case, removing what you are interested in debugging is >> evidently counter productive, so other techniques need to be used to >> ensure the CS regs remain alive. >> But equally there could be use cases where this might be just fine or >> even the only way. >> > > Again not arguing on that. Just saying we can try out if that solves the > issue. As I said there are other ways to disable idle and we can > advertise those instead of such misuse. Also those alternative methods > are runtime and can be used when you need them. > >>>> >>>> On systems that do behave correctly with respect to debug power >>>> domains, then disabling CPUIdle is unnecessary - these can be >>>> controlled by EDPRCR - perhaps; per the specification it is >>>> "implementation defined" if writing bits to this register have an >>>> effect on the system anyway even if the debug domain is correctly >>>> powered. >>>> >>> >>> We can always do that unconditionally. If implementations don't honor >>> those bits, it's different. If they hang on accessing something which is >>> on debug power domain and not on core power domain, then you have much >>> bigger issue to solve. How can you even trust and make any other >>> register accesses that are in debug power domain then ? >>> >> >> It is difficult and highly platform dependent. For external debug we >> might have per-platform rules built into the debugger on what can and >> cannot be done and when, plus on occasion some power management >> scripts. Those platforms that get closest to the "standard" CoreSight >> power management are easiest to debug. >> > > Absolutely, so just mandating might just solve issue *accidentally* > not *intentionally*. So I don't want it to be advertised in that way > and that becomes defacto. > > [...] >> >> I'm not advocating /nohlt above anything else - it's just what is >> being discussed here. Furthermore, no one debug technique is ever >> going to be appropriate in all circumstances - debug and trace are >> always a compromise. >> > > True. > >> I initially raised the issue of clusters powering down, and >> possibility that no CPUIdle might prevent this, to ensure that >> awareness is built in to driver / config / help text /documentation >> that these are real issues seen in the external debug world. >> > > Point taken. So we could just specify that all necessary power domains > need to be on for proper functionality for this feature and that it's > highly platform specific instead of mixing cpu/cluster idle details here. > >> The key point is that the caveat in using this driver is that the >> power management has to be considered on a platform specific basis >> before it is configured; and appropriate actions may be needed for it >> to work correctly. Without this then the driver could cause more >> issues than it debugs. A user selecting this _must_ be told about >> these issues >> So given all the possible caveats, I think we : 1) Shouldn't enable the driver by default at runtime even if it is built-in. 2) Should provide mechanisms to turn it on at boot (via kernel commandline) or anytime later (via sysfs), which kind of puts the responsibility back on the user : "You know what you are doing". 3) Shouldn't turn the driver on based on "nohlt" which the user could use it for some other purposes, without explicit intention of turning this driver on). 4) Should document the fact that, on some platforms, the user may have to disable CPUidle explicitly to get the driver working. But let us not make it the default. The user with a not so ideal platform could add "nohlt" and get it working. Suzuki -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/03/17 17:09, Suzuki K Poulose wrote: > On 22/03/17 16:17, Sudeep Holla wrote: [...] >> >> Point taken. So we could just specify that all necessary power >> domains need to be on for proper functionality for this feature and >> that it's highly platform specific instead of mixing cpu/cluster >> idle details here. >> >>> The key point is that the caveat in using this driver is that >>> the power management has to be considered on a platform specific >>> basis before it is configured; and appropriate actions may be >>> needed for it to work correctly. Without this then the driver >>> could cause more issues than it debugs. A user selecting this >>> _must_ be told about these issues >>> > > So given all the possible caveats, I think we : > > 1) Shouldn't enable the driver by default at runtime even if it is > built-in. > 2) Should provide mechanisms to turn it on at boot (via > kernel commandline) or anytime later (via sysfs), which kind of puts > the responsibility back on the user : "You know what you are doing". > 3) Shouldn't turn the driver on based on "nohlt" which the user > could use it for some other purposes, without explicit intention of > turning this driver on). > 4) Should document the fact that, on some > platforms, the user may have to disable CPUidle explicitly to get the > driver working. But let us not make it the default. The user with a > not so ideal platform could add "nohlt" and get it working. > Agreed on all points and well summarized. I would like to highlight (3) and (4) as it needs to be well understood. "nohlt" has a *different* meaning already, so using that in this driver for something else is simple wrong as it affects the system in unintended ways. And yes if user (mis)uses it to get things working, it's fine but shouldn't be recommended way. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Also agree with Suzuki's excellent summary.
A couple of notes on using EDPRCR....
The logic for this assuming a correctly implemented system should be
something like.....
if (prsr() == powered_down) { // if we are powered down request power up.
sw_unlock() // ensure that the sw lock register on the device is unlocked
write(prcr(bit3=1))
wait_with_timeout(prsr()==powered_up) // must wait to see if the
system does power up - otherwise bad things happen!
if(timed_out)
fail("power up request for CPU N failed.") // nothing to do
after this point though if a CPU can't be powered is should not be
included in the list to be checked on crash.
}
ensure_os_lock_unlocked() // os lock has to be unlocked for bit 0 of
prcr to be writeable.
sw_unlock() // ensure that the sw lock register on the device is unlocked
write(prcr(bit3=1 | bit0=1)) // the core is powered, set the
nopowerdown request bit so we don't lose it & emulate power down
I think that some of the above logic is already in the driver, so
needs to be adapted for the PRCR handling.
On the "bad" systems, the initial prsr check is likely to fail (crash
/ buslock) if the debug logic is not correctly powered. The user will
then have to use one or more of the PM mitigations previously
discussed that are appropriate for that specific platform and retry
Regards
Mike
On 23 March 2017 at 05:43, Leo Yan <leo.yan@linaro.org> wrote:
> On Wed, Mar 22, 2017 at 05:25:50PM +0000, Sudeep Holla wrote:
>>
>>
>> On 22/03/17 17:09, Suzuki K Poulose wrote:
>> > On 22/03/17 16:17, Sudeep Holla wrote:
>>
>> [...]
>>
>> >>
>> >> Point taken. So we could just specify that all necessary power
>> >> domains need to be on for proper functionality for this feature and
>> >> that it's highly platform specific instead of mixing cpu/cluster
>> >> idle details here.
>> >>
>> >>> The key point is that the caveat in using this driver is that
>> >>> the power management has to be considered on a platform specific
>> >>> basis before it is configured; and appropriate actions may be
>> >>> needed for it to work correctly. Without this then the driver
>> >>> could cause more issues than it debugs. A user selecting this
>> >>> _must_ be told about these issues
>> >>>
>> >
>> > So given all the possible caveats, I think we :
>> >
>> > 1) Shouldn't enable the driver by default at runtime even if it is
>> > built-in.
>> > 2) Should provide mechanisms to turn it on at boot (via
>> > kernel commandline) or anytime later (via sysfs), which kind of puts
>> > the responsibility back on the user : "You know what you are doing".
>> > 3) Shouldn't turn the driver on based on "nohlt" which the user
>> > could use it for some other purposes, without explicit intention of
>> > turning this driver on).
>> > 4) Should document the fact that, on some
>> > platforms, the user may have to disable CPUidle explicitly to get the
>> > driver working. But let us not make it the default. The user with a
>> > not so ideal platform could add "nohlt" and get it working.
>> >
>>
>> Agreed on all points and well summarized.
>> I would like to highlight (3) and (4) as it needs to be well understood.
>>
>> "nohlt" has a *different* meaning already, so using that in this
>> driver for something else is simple wrong as it affects the system in
>> unintended ways. And yes if user (mis)uses it to get things working,
>> it's fine but shouldn't be recommended way.
>
> Understand this point.
>
> I will try to use general way to constraint CPUIdle like other
> drivers.
>
> Thanks all for these good suggestions :)
>
> Thanks,
> Leo Yan
--
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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index 130cb21..3ed651e 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -89,4 +89,14 @@ 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 to dump sample-based profiling registers for + panic. To avoid lockups when accessing debug module registers, + it is safer to disable CPU low power states (like "nohlt" on the + kernel command line) when using this feature. + 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..9553fb2 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-debug.c @@ -0,0 +1,377 @@ +/* + * 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/amba/bus.h> +#include <linux/coresight.h> +#include <linux/cpu.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/moduleparam.h> +#include <linux/slab.h> +#include <linux/smp.h> +#include <linux/types.h> +#include <linux/uaccess.h> + +#include "coresight-priv.h" + +#define EDPCSR 0x0A0 +#define EDCIDSR 0x0A4 +#define EDVIDSR 0x0A8 +#define EDPCSR_HI 0x0AC +#define EDOSLAR 0x300 +#define EDPRSR 0x314 +#define EDDEVID1 0xFC4 +#define EDDEVID 0xFC8 + +#define EDPCSR_PROHIBITED 0xFFFFFFFF + +/* bits definition for EDPCSR */ +#ifndef CONFIG_64BIT +#define EDPCSR_THUMB BIT(0) +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2) +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1) +#endif + +/* bits definition for EDPRSR */ +#define EDPRSR_DLK BIT(6) +#define EDPRSR_PU BIT(0) + +/* bits definition for EDVIDSR */ +#define EDVIDSR_NS BIT(31) +#define EDVIDSR_E2 BIT(30) +#define EDVIDSR_E3 BIT(29) +#define EDVIDSR_HV BIT(28) +#define EDVIDSR_VMID GENMASK(7, 0) + +/* bits definition for EDDEVID1 */ +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0) +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0) + +/* bits definition for EDDEVID */ +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0) +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2) +#define EDDEVID_IMPL_FULL (0x3) + +struct debug_drvdata { + void __iomem *base; + struct device *dev; + int cpu; + + bool edpcsr_present; + bool edvidsr_present; + bool pc_has_offset; + + u32 eddevid; + u32 eddevid1; + + u32 edpcsr; + u32 edpcsr_hi; + u32 edprsr; + u32 edvidsr; + u32 edcidsr; +}; + +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata); + +static void debug_os_unlock(struct debug_drvdata *drvdata) +{ + /* Unlocks the debug registers */ + writel_relaxed(0x0, drvdata->base + EDOSLAR); + wmb(); +} + +/* + * According to ARM DDI 0487A.k, before access external debug + * registers should firstly check the access permission; if any + * below condition has been met then cannot access debug + * registers to avoid lockup issue: + * + * - CPU power domain is powered off; + * - The OS Double Lock is locked; + * + * By checking EDPRSR can get to know if meet these conditions. + */ +static bool debug_access_permitted(struct debug_drvdata *drvdata) +{ + /* CPU is powered off */ + if (!(drvdata->edprsr & EDPRSR_PU)) + return false; + + /* The OS Double Lock is locked */ + if (drvdata->edprsr & EDPRSR_DLK) + return false; + + return true; +} + +static void debug_read_regs(struct debug_drvdata *drvdata) +{ + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR); + + if (!debug_access_permitted(drvdata)) + return; + + if (!drvdata->edpcsr_present) + return; + + CS_UNLOCK(drvdata->base); + + debug_os_unlock(drvdata); + + drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR); + + /* + * As described in ARM DDI 0487A.k, if the processing + * element (PE) is in debug state, or sample-based + * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF; + * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become + * UNKNOWN state. So directly bail out for this case. + */ + if (drvdata->edpcsr == EDPCSR_PROHIBITED) { + CS_LOCK(drvdata->base); + return; + } + + /* + * A read of the EDPCSR normally has the side-effect of + * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI; + * at this point it's safe to read value from them. + */ + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR); +#ifdef CONFIG_64BIT + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI); +#endif + + if (drvdata->edvidsr_present) + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR); + + CS_LOCK(drvdata->base); +} + +#ifndef CONFIG_64BIT +static bool debug_pc_has_offset(struct debug_drvdata *drvdata) +{ + u32 pcsr_offset; + + pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK; + + return (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET); +} + +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata, + unsigned long pc) +{ + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0; + + if (debug_pc_has_offset(drvdata)) { + arm_inst_offset = 8; + thumb_inst_offset = 4; + } + + /* Handle thumb instruction */ + if (pc & EDPCSR_THUMB) { + pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset; + return pc; + } + + /* + * Handle arm instruction offset, if the arm instruction + * is not 4 byte alignment then it's possible the case + * for implementation defined; keep original value for this + * case and print info for notice. + */ + if (pc & BIT(1)) + pr_emerg("Instruction offset is implementation defined\n"); + else + pc = (pc & EDPCSR_ARM_INST_MASK) - arm_inst_offset; + + return pc; +} +#endif + +static void debug_dump_regs(struct debug_drvdata *drvdata) +{ + unsigned long pc; + + pr_emerg("\tEDPRSR: %08x (Power:%s DLK:%s)\n", drvdata->edprsr, + drvdata->edprsr & EDPRSR_PU ? "On" : "Off", + drvdata->edprsr & EDPRSR_DLK ? "Lock" : "Unlock"); + + if (!debug_access_permitted(drvdata) || !drvdata->edpcsr_present) { + pr_emerg("No permission to access debug registers!\n"); + return; + } + + if (drvdata->edpcsr == EDPCSR_PROHIBITED) { + pr_emerg("CPU is in Debug state or profiling is prohibited!\n"); + return; + } + +#ifdef CONFIG_64BIT + pc = (unsigned long)drvdata->edpcsr_hi << 32 | + (unsigned long)drvdata->edpcsr; +#else + pc = debug_adjust_pc(drvdata, (unsigned long)drvdata->edpcsr); +#endif + + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc); + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr); + + if (!drvdata->edvidsr_present) + return; + + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%s VMID:%x)\n", + drvdata->edvidsr, + drvdata->edvidsr & EDVIDSR_NS ? "Non-secure" : "Secure", + drvdata->edvidsr & EDVIDSR_E3 ? "EL3" : + (drvdata->edvidsr & EDVIDSR_E2 ? "EL2" : "EL1/0"), + drvdata->edvidsr & EDVIDSR_HV ? "64bits" : "32bits", + drvdata->edvidsr & (u32)EDVIDSR_VMID); +} + +/* + * Dump out information on panic. + */ +static int debug_notifier_call(struct notifier_block *self, + unsigned long v, void *p) +{ + int cpu; + + pr_emerg("ARM external debug module:\n"); + + for_each_possible_cpu(cpu) { + if (!per_cpu(debug_drvdata, cpu)) + continue; + + pr_emerg("CPU[%d]:\n", per_cpu(debug_drvdata, cpu)->cpu); + + debug_read_regs(per_cpu(debug_drvdata, cpu)); + debug_dump_regs(per_cpu(debug_drvdata, cpu)); + } + + return 0; +} + +static struct notifier_block debug_notifier = { + .notifier_call = debug_notifier_call, +}; + +static void debug_init_arch_data(void *info) +{ + struct debug_drvdata *drvdata = info; + u32 mode; + + CS_UNLOCK(drvdata->base); + + debug_os_unlock(drvdata); + + /* Read device info */ + drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID); + drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1); + + /* Parse implementation feature */ + mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE; + if (mode == EDDEVID_IMPL_FULL) { + drvdata->edpcsr_present = true; + drvdata->edvidsr_present = true; + } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) { + drvdata->edpcsr_present = true; + drvdata->edvidsr_present = false; + } else { + drvdata->edpcsr_present = false; + drvdata->edvidsr_present = false; + } + + CS_LOCK(drvdata->base); +} + +static int debug_probe(struct amba_device *adev, const struct amba_id *id) +{ + void __iomem *base; + struct device *dev = &adev->dev; + struct debug_drvdata *drvdata; + struct resource *res = &adev->res; + struct device_node *np = adev->dev.of_node; + char buf[32]; + static int debug_count; + + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); + if (!drvdata) + return -ENOMEM; + + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0; + 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; + + get_online_cpus(); + per_cpu(debug_drvdata, drvdata->cpu) = drvdata; + + if (smp_call_function_single(drvdata->cpu, + debug_init_arch_data, drvdata, 1)) + dev_err(dev, "Debug arch init failed\n"); + + put_online_cpus(); + + if (!debug_count++) + atomic_notifier_chain_register(&panic_notifier_list, + &debug_notifier); + + sprintf(buf, (char *)id->data, drvdata->cpu); + dev_info(dev, "%s initialized\n", buf); + return 0; +} + +static struct amba_id debug_ids[] = { + { /* Debug for Cortex-A53 */ + .id = 0x000bbd03, + .mask = 0x000fffff, + .data = "Coresight debug-CPU%d", + }, + { /* Debug for Cortex-A57 */ + .id = 0x000bbd07, + .mask = 0x000fffff, + .data = "Coresight debug-CPU%d", + }, + { /* Debug for Cortex-A72 */ + .id = 0x000bbd08, + .mask = 0x000fffff, + .data = "Coresight debug-CPU%d", + }, + { 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);
Coresight includes debug module and usually the module connects with CPU debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has description for related info in "Part H: External Debug". Chapter H7 "The Sample-based Profiling Extension" introduces several sampling registers, e.g. we can check program counter value with combined CPU exception level, secure state, etc. So this is helpful for analysis CPU lockup scenarios, e.g. if one CPU has run into infinite loop with IRQ disabled. In this case the CPU cannot switch context and handle any interrupt (including IPIs), as the result it cannot handle SMP call for stack dump. This patch is to enable coresight debug module, so firstly this driver is to bind apb clock for debug module and this is to ensure the debug module can be accessed from program or external debugger. And the driver uses sample-based registers for debug purpose, e.g. when system detects the CPU lockup and trigger panic, the driver will dump program counter and combined context registers (EDCIDSR, EDVIDSR); by parsing context registers so can quickly get to know CPU secure state, exception level, etc. Some of the debug module registers are located in CPU power domain, so in the driver it has checked the power state for CPU before accessing registers within CPU power domain. For most safe way to use this driver, it's suggested to disable CPU low power states, this can simply set "nohlt" in kernel command line. Signed-off-by: Leo Yan <leo.yan@linaro.org> --- drivers/hwtracing/coresight/Kconfig | 10 + drivers/hwtracing/coresight/Makefile | 1 + drivers/hwtracing/coresight/coresight-debug.c | 377 ++++++++++++++++++++++++++ 3 files changed, 388 insertions(+) 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html