* [PATCH v6 1/8] coresight: bindings for CPU debug module
2017-04-06 13:30 [PATCH v6 0/8] coresight: enable debug module Leo Yan
@ 2017-04-06 13:30 ` Leo Yan
2017-04-06 13:30 ` [PATCH v6 2/8] doc: Add documentation for Coresight CPU debug Leo Yan
` (6 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Leo Yan @ 2017-04-06 13:30 UTC (permalink / raw)
To: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel, devicetree, linux-arm-kernel, linux-arm-msm,
linux-soc, Mike Leach, Sudeep Holla
Cc: Leo Yan
According to ARMv8 architecture reference manual (ARM DDI 0487A.k)
Chapter 'Part H: External debug', the CPU can integrate debug module
and it can support self-hosted debug and external debug. Especially
for supporting self-hosted debug, this means the program can access
the debug module from mmio region; and usually the mmio region is
integrated with coresight.
So add document for binding debug component, includes binding to APB
clock; and also need specify the CPU node which the debug module is
dedicated to specific CPU.
Suggested-by: Mike Leach <mike.leach@linaro.org>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
.../bindings/arm/coresight-cpu-debug.txt | 49 ++++++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/coresight-cpu-debug.txt
diff --git a/Documentation/devicetree/bindings/arm/coresight-cpu-debug.txt b/Documentation/devicetree/bindings/arm/coresight-cpu-debug.txt
new file mode 100644
index 0000000..2982912
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/coresight-cpu-debug.txt
@@ -0,0 +1,49 @@
+* CoreSight CPU Debug Component:
+
+CoreSight CPU debug component are compliant with the ARMv8 architecture
+reference manual (ARM DDI 0487A.k) Chapter 'Part H: External debug'. The
+external debug module is mainly used for two modes: self-hosted debug and
+external debug, and it can be accessed from mmio region from Coresight
+and eventually the debug module connects with CPU for debugging. And the
+debug module provides sample-based profiling extension, which can be used
+to sample CPU program counter, secure state and exception level, etc;
+usually every CPU has one dedicated debug module to be connected.
+
+Required properties:
+
+- compatible : should be "arm,coresight-cpu-debug"; supplemented with
+ "arm,primecell" since this driver is using the AMBA bus
+ interface.
+
+- reg : physical base address and length of the register set.
+
+- clocks : the clock associated to this component.
+
+- clock-names : the name of the clock referenced by the code. Since we are
+ using the AMBA framework, the name of the clock providing
+ the interconnect should be "apb_pclk" and the clock is
+ mandatory. The interface between the debug logic and the
+ processor core is clocked by the internal CPU clock, so it
+ is enabled with CPU clock by default.
+
+- cpu : the CPU phandle the debug module is affined to. When omitted
+ the module is considered to belong to CPU0.
+
+Optional properties:
+
+- power-domains: a phandle to the debug power domain. We use "power-domains"
+ binding to turn on the debug logic if it has own dedicated
+ power domain and if necessary to use "cpuidle.off=1" or
+ "nohlt" in the kernel command line or sysfs node to
+ constrain idle states to ensure registers in the CPU power
+ domain are accessible.
+
+Example:
+
+ debug@f6590000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0 0xf6590000 0 0x1000>;
+ clocks = <&sys_ctrl HI6220_DAPB_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&cpu0>;
+ };
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v6 2/8] doc: Add documentation for Coresight CPU debug
2017-04-06 13:30 [PATCH v6 0/8] coresight: enable debug module Leo Yan
2017-04-06 13:30 ` [PATCH v6 1/8] coresight: bindings for CPU " Leo Yan
@ 2017-04-06 13:30 ` Leo Yan
2017-04-19 17:25 ` Mathieu Poirier
2017-04-06 13:30 ` [PATCH v6 3/8] coresight: of_get_coresight_platform_data: Add missing of_node_put Leo Yan
` (5 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Leo Yan @ 2017-04-06 13:30 UTC (permalink / raw)
To: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel, devicetree, linux-arm-kernel, linux-arm-msm,
linux-soc, Mike Leach, Sudeep Holla
Cc: Leo Yan
Update kernel-parameters.txt to add new parameter:
coresight_cpu_debug.enable is a knob to enable debugging at boot time.
Add detailed documentation, which contains the implementation, Mike
Leach excellent summary for "clock and power domain". At the end some
examples on how to enable the debugging functionality are provided.
Suggested-by: Mike Leach <mike.leach@linaro.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
Documentation/admin-guide/kernel-parameters.txt | 7 +
Documentation/trace/coresight-cpu-debug.txt | 173 ++++++++++++++++++++++++
2 files changed, 180 insertions(+)
create mode 100644 Documentation/trace/coresight-cpu-debug.txt
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index facc20a..cf90146 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -650,6 +650,13 @@
/proc/<pid>/coredump_filter.
See also Documentation/filesystems/proc.txt.
+ coresight_cpu_debug.enable
+ [ARM,ARM64]
+ Format: <bool>
+ Enable/disable the CPU sampling based debugging.
+ 0: default value, disable debugging
+ 1: enable debugging at boot time
+
cpuidle.off=1 [CPU_IDLE]
disable the cpuidle sub-system
diff --git a/Documentation/trace/coresight-cpu-debug.txt b/Documentation/trace/coresight-cpu-debug.txt
new file mode 100644
index 0000000..e7ad05e
--- /dev/null
+++ b/Documentation/trace/coresight-cpu-debug.txt
@@ -0,0 +1,173 @@
+ Coresight CPU Debug Module
+ ==========================
+
+ Author: Leo Yan <leo.yan@linaro.org>
+ Date: April 5th, 2017
+
+Introduction
+------------
+
+Coresight CPU debug module is defined in ARMv8-a architecture reference manual
+(ARM DDI 0487A.k) Chapter 'Part H: External debug', the CPU can integrate
+debug module and it is mainly used for two modes: self-hosted debug and
+external debug. Usually the external debug mode is well known as the external
+debugger connects with SoC from JTAG port; on the other hand the program can
+explore debugging method which rely on self-hosted debug mode, this document
+is to focus on this part.
+
+The debug module provides sample-based profiling extension, which can be used
+to sample CPU program counter, secure state and exception level, etc; usually
+every CPU has one dedicated debug module to be connected. Based on self-hosted
+debug mechanism, Linux kernel can access these related registers from mmio
+region when the kernel panic happens. The callback notifier for kernel panic
+will dump related registers for every CPU; finally this is good for assistant
+analysis for panic.
+
+
+Implementation
+--------------
+
+- During driver registration, use EDDEVID and EDDEVID1 two device ID
+ registers to decide if sample-based profiling is implemented or not. On some
+ platforms this hardware feature is fully or partialy implemented; and if
+ this feature is not supported then registration will fail.
+
+- When write this doc, the debug driver mainly relies on three sampling
+ registers. The kernel panic callback notifier gathers info from EDPCSR
+ EDVIDSR and EDCIDSR; from EDPCSR we can get program counter, EDVIDSR has
+ information for secure state, exception level, bit width, etc; EDCIDSR is
+ context ID value which contains the sampled value of CONTEXTIDR_EL1.
+
+- The driver supports CPU running mode with either AArch64 or AArch32. The
+ registers naming convention is a bit different between them, AArch64 uses
+ 'ED' for register prefix (ARM DDI 0487A.k, chapter H9.1) and AArch32 uses
+ 'DBG' as prefix (ARM DDI 0487A.k, chapter G5.1). The driver is unified to
+ use AArch64 naming convention.
+
+- ARMv8-a (ARM DDI 0487A.k) and ARMv7-a (ARM DDI 0406C.b) have different
+ register bits definition. So the driver consolidates two difference:
+
+ If PCSROffset=0b0000. ARMv8-a doesn't apply offset on PCSR, but ARMv7-a
+ defines "PCSR samples are offset by a value that depends on the instruction
+ set state". For ARMv7-a, the driver checks furthermore if CPU runs with ARM
+ or thumb instruction set and calibrate PCSR value, the detailed description
+ for offset is in ARMv7-a ARM (ARM DDI 0406C.b) chapter C11.11.34 "DBGPCSR,
+ Program Counter Sampling Register".
+
+ If PCSROffset=0b0010, ARMv8-a defines "EDPCSR implemented, and samples have
+ no offset applied and do not sample the instruction set state in AArch32
+ state". So for the case when CPU runs with AArch32 state and PCSROffset=
+ 0b0010, the driver considers PCSR doesn't really work.
+
+
+Clock and power domain
+----------------------
+
+Before accessing debug registers, we should ensure the clock and power domain
+have been enabled properly. In ARMv8-a ARM (ARM DDI 0487A.k) chapter 'H9.1
+Debug registers', the debug registers are spread into two domains: the debug
+domain and the CPU domain.
+
+ +---------------+
+ | |
+ | |
+ +----------+--+ |
+ dbg_clk -->| |**| |<-- cpu_clk
+ | Debug |**| CPU |
+ dbg_pd -->| |**| |<-- cpu_pd
+ +----------+--+ |
+ | |
+ | |
+ +---------------+
+
+For debug domain, the user uses DT binding "clocks" and "power-domains" to
+specify the corresponding clock source and power supply for the debug logic.
+The driver calls the pm_runtime_{put|get} operations as needed to handle the
+debug power domain.
+
+For CPU domain, the different SoC designs have different power management
+schemes and finally this heavily impacts external debug module. So we can
+divide into below cases:
+
+- On systems with a sane power controller which can behave correctly with
+ respect to CPU power domain, the CPU power domain can be controlled by
+ register EDPRCR in driver. The driver firstly writes bit EDPRCR.COREPURQ
+ to power up the CPU, and then writes bit EDPRCR.CORENPDRQ for emulation
+ of CPU power down. As result, this can ensure the CPU power domain is
+ powered on properly during the period when access debug related registers;
+
+- 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. The bits in EDPRCR are not
+ respected in these cases, so 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.
+
+ In this case, accessing to the debug registers while they are not powered
+ is a recipe for disaster; so we need preventing CPU low power states at boot
+ time or when user enable module at the run time. Please see chapter
+ "How to use the module" for detailed usage info for this.
+
+
+Device Tree Bindings
+--------------------
+
+See Documentation/devicetree/bindings/arm/coresight-cpu-debug.txt for details.
+
+
+How to use the module
+---------------------
+
+If you want to enable debugging functionality at boot time, you can add
+"coresight_cpu_debug.enable=1" to the kernel command line parameter.
+
+The driver also can work as module, so can enable the debugging when insmod
+module:
+# insmod coresight_cpu_debug.ko debug=1
+
+When boot time or insmod module you have not enabled the debugging, the driver
+uses the debugfs file system to provide a knob to dynamically enable or disable
+debugging:
+
+To enable it, write a '1' into /sys/kernel/debug/coresight_cpu_debug/enable:
+# echo 1 > /sys/kernel/debug/coresight_cpu_debug/enable
+
+To disable it, write a '0' into /sys/kernel/debug/coresight_cpu_debug/enable:
+# echo 0 > /sys/kernel/debug/coresight_cpu_debug/enable
+
+As explained in chapter "Clock and power domain", if you are working on one
+platform which has idle states to power off debug logic and the power
+controller cannot work well for the request from EDPRCR, then you should
+firstly constraint CPU idle states before enable CPU debugging feature; so can
+ensure the accessing to debug logic.
+
+If you want to limit idle states at boot time, you can use "nohlt" or
+"cpuidle.off=1" in the kernel command line.
+
+At the runtime you can disable idle states with below methods:
+
+Set latency request to /dev/cpu_dma_latency to disable all CPUs specific idle
+states (if latency = 0uS then disable all idle states):
+# echo "what_ever_latency_you_need_in_uS" > /dev/cpu_dma_latency
+
+Disable specific CPU's specific idle state:
+# echo 1 > /sys/devices/system/cpu/cpu$cpu/cpuidle/state$state/disable
+
+
+Output format
+-------------
+
+Here is an example of the debugging output format:
+
+ARM external debug module:
+CPU[0]:
+ EDPRSR: 0000000b (Power:On DLK:Unlock)
+ EDPCSR: [<ffff00000808eb54>] handle_IPI+0xe4/0x150
+ EDCIDSR: 00000000
+ EDVIDSR: 90000000 (State:Non-secure Mode:EL1/0 Width:64bits VMID:0)
+CPU[1]:
+ EDPRSR: 0000000b (Power:On DLK:Unlock)
+ EDPCSR: [<ffff0000087a64c0>] debug_notifier_call+0x108/0x288
+ EDCIDSR: 00000000
+ EDVIDSR: 90000000 (State:Non-secure Mode:EL1/0 Width:64bits VMID:0)
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 2/8] doc: Add documentation for Coresight CPU debug
2017-04-06 13:30 ` [PATCH v6 2/8] doc: Add documentation for Coresight CPU debug Leo Yan
@ 2017-04-19 17:25 ` Mathieu Poirier
0 siblings, 0 replies; 23+ messages in thread
From: Mathieu Poirier @ 2017-04-19 17:25 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On 6 April 2017 at 07:30, Leo Yan <leo.yan@linaro.org> wrote:
> Update kernel-parameters.txt to add new parameter:
> coresight_cpu_debug.enable is a knob to enable debugging at boot time.
>
> Add detailed documentation, which contains the implementation, Mike
> Leach excellent summary for "clock and power domain". At the end some
> examples on how to enable the debugging functionality are provided.
>
> Suggested-by: Mike Leach <mike.leach@linaro.org>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 7 +
> Documentation/trace/coresight-cpu-debug.txt | 173 ++++++++++++++++++++++++
> 2 files changed, 180 insertions(+)
> create mode 100644 Documentation/trace/coresight-cpu-debug.txt
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index facc20a..cf90146 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -650,6 +650,13 @@
> /proc/<pid>/coredump_filter.
> See also Documentation/filesystems/proc.txt.
>
> + coresight_cpu_debug.enable
> + [ARM,ARM64]
> + Format: <bool>
> + Enable/disable the CPU sampling based debugging.
> + 0: default value, disable debugging
> + 1: enable debugging at boot time
> +
> cpuidle.off=1 [CPU_IDLE]
> disable the cpuidle sub-system
>
> diff --git a/Documentation/trace/coresight-cpu-debug.txt b/Documentation/trace/coresight-cpu-debug.txt
> new file mode 100644
> index 0000000..e7ad05e
> --- /dev/null
> +++ b/Documentation/trace/coresight-cpu-debug.txt
> @@ -0,0 +1,173 @@
> + Coresight CPU Debug Module
> + ==========================
> +
> + Author: Leo Yan <leo.yan@linaro.org>
> + Date: April 5th, 2017
> +
> +Introduction
> +------------
> +
> +Coresight CPU debug module is defined in ARMv8-a architecture reference manual
> +(ARM DDI 0487A.k) Chapter 'Part H: External debug', the CPU can integrate
> +debug module and it is mainly used for two modes: self-hosted debug and
> +external debug. Usually the external debug mode is well known as the external
> +debugger connects with SoC from JTAG port; on the other hand the program can
> +explore debugging method which rely on self-hosted debug mode, this document
> +is to focus on this part.
> +
> +The debug module provides sample-based profiling extension, which can be used
> +to sample CPU program counter, secure state and exception level, etc; usually
> +every CPU has one dedicated debug module to be connected. Based on self-hosted
> +debug mechanism, Linux kernel can access these related registers from mmio
> +region when the kernel panic happens. The callback notifier for kernel panic
> +will dump related registers for every CPU; finally this is good for assistant
> +analysis for panic.
> +
> +
> +Implementation
> +--------------
> +
> +- During driver registration, use EDDEVID and EDDEVID1 two device ID
> + registers to decide if sample-based profiling is implemented or not. On some
> + platforms this hardware feature is fully or partialy implemented; and if
> + this feature is not supported then registration will fail.
> +
> +- When write this doc, the debug driver mainly relies on three sampling
> + registers. The kernel panic callback notifier gathers info from EDPCSR
> + EDVIDSR and EDCIDSR; from EDPCSR we can get program counter, EDVIDSR has
> + information for secure state, exception level, bit width, etc; EDCIDSR is
> + context ID value which contains the sampled value of CONTEXTIDR_EL1.
> +
> +- The driver supports CPU running mode with either AArch64 or AArch32. The
> + registers naming convention is a bit different between them, AArch64 uses
> + 'ED' for register prefix (ARM DDI 0487A.k, chapter H9.1) and AArch32 uses
> + 'DBG' as prefix (ARM DDI 0487A.k, chapter G5.1). The driver is unified to
> + use AArch64 naming convention.
> +
> +- ARMv8-a (ARM DDI 0487A.k) and ARMv7-a (ARM DDI 0406C.b) have different
> + register bits definition. So the driver consolidates two difference:
> +
> + If PCSROffset=0b0000. ARMv8-a doesn't apply offset on PCSR, but ARMv7-a
> + defines "PCSR samples are offset by a value that depends on the instruction
> + set state".
I had to look in the documentation (ID092916) a little here. On ARMv8
if DEVID1.PCSROffset is 0 then the feature is not implemented - as
such the first part of the sentence should be corrected. The rest of
the sentence about ARMv7 is correct.
> For ARMv7-a, the driver checks furthermore if CPU runs with ARM
> + or thumb instruction set and calibrate PCSR value, the detailed description
> + for offset is in ARMv7-a ARM (ARM DDI 0406C.b) chapter C11.11.34 "DBGPCSR,
> + Program Counter Sampling Register".
> +
> + If PCSROffset=0b0010, ARMv8-a defines "EDPCSR implemented, and samples have
> + no offset applied and do not sample the instruction set state in AArch32
> + state". So for the case when CPU runs with AArch32 state and PCSROffset=
> + 0b0010, the driver considers PCSR doesn't really work.
Please remove the word "really" - it either works or it doesn't. So
on ARMv8 if DEVID1.PCSROffset is 0x2 and the CPU operates in AArch32
state, EDPCSR is not sampled. When the CPU operates in AArch64 state
EDPCSR is sampled and no offset are applied.
> +
> +
> +Clock and power domain
> +----------------------
> +
> +Before accessing debug registers, we should ensure the clock and power domain
> +have been enabled properly. In ARMv8-a ARM (ARM DDI 0487A.k) chapter 'H9.1
> +Debug registers', the debug registers are spread into two domains: the debug
> +domain and the CPU domain.
> +
> + +---------------+
> + | |
> + | |
> + +----------+--+ |
> + dbg_clk -->| |**| |<-- cpu_clk
> + | Debug |**| CPU |
> + dbg_pd -->| |**| |<-- cpu_pd
> + +----------+--+ |
> + | |
> + | |
> + +---------------+
> +
> +For debug domain, the user uses DT binding "clocks" and "power-domains" to
> +specify the corresponding clock source and power supply for the debug logic.
> +The driver calls the pm_runtime_{put|get} operations as needed to handle the
> +debug power domain.
> +
> +For CPU domain, the different SoC designs have different power management
> +schemes and finally this heavily impacts external debug module. So we can
> +divide into below cases:
> +
> +- On systems with a sane power controller which can behave correctly with
> + respect to CPU power domain, the CPU power domain can be controlled by
> + register EDPRCR in driver. The driver firstly writes bit EDPRCR.COREPURQ
> + to power up the CPU, and then writes bit EDPRCR.CORENPDRQ for emulation
> + of CPU power down. As result, this can ensure the CPU power domain is
> + powered on properly during the period when access debug related registers;
> +
> +- 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. The bits in EDPRCR are not
> + respected in these cases, so 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.
> +
> + In this case, accessing to the debug registers while they are not powered
> + is a recipe for disaster; so we need preventing CPU low power states at boot
> + time or when user enable module at the run time. Please see chapter
> + "How to use the module" for detailed usage info for this.
> +
> +
> +Device Tree Bindings
> +--------------------
> +
> +See Documentation/devicetree/bindings/arm/coresight-cpu-debug.txt for details.
> +
> +
> +How to use the module
> +---------------------
> +
> +If you want to enable debugging functionality at boot time, you can add
> +"coresight_cpu_debug.enable=1" to the kernel command line parameter.
> +
> +The driver also can work as module, so can enable the debugging when insmod
> +module:
> +# insmod coresight_cpu_debug.ko debug=1
> +
> +When boot time or insmod module you have not enabled the debugging, the driver
> +uses the debugfs file system to provide a knob to dynamically enable or disable
> +debugging:
> +
> +To enable it, write a '1' into /sys/kernel/debug/coresight_cpu_debug/enable:
> +# echo 1 > /sys/kernel/debug/coresight_cpu_debug/enable
> +
> +To disable it, write a '0' into /sys/kernel/debug/coresight_cpu_debug/enable:
> +# echo 0 > /sys/kernel/debug/coresight_cpu_debug/enable
> +
> +As explained in chapter "Clock and power domain", if you are working on one
> +platform which has idle states to power off debug logic and the power
> +controller cannot work well for the request from EDPRCR, then you should
> +firstly constraint CPU idle states before enable CPU debugging feature; so can
> +ensure the accessing to debug logic.
> +
> +If you want to limit idle states at boot time, you can use "nohlt" or
> +"cpuidle.off=1" in the kernel command line.
> +
> +At the runtime you can disable idle states with below methods:
> +
> +Set latency request to /dev/cpu_dma_latency to disable all CPUs specific idle
> +states (if latency = 0uS then disable all idle states):
> +# echo "what_ever_latency_you_need_in_uS" > /dev/cpu_dma_latency
> +
> +Disable specific CPU's specific idle state:
> +# echo 1 > /sys/devices/system/cpu/cpu$cpu/cpuidle/state$state/disable
> +
> +
> +Output format
> +-------------
> +
> +Here is an example of the debugging output format:
> +
> +ARM external debug module:
> +CPU[0]:
> + EDPRSR: 0000000b (Power:On DLK:Unlock)
> + EDPCSR: [<ffff00000808eb54>] handle_IPI+0xe4/0x150
> + EDCIDSR: 00000000
> + EDVIDSR: 90000000 (State:Non-secure Mode:EL1/0 Width:64bits VMID:0)
> +CPU[1]:
> + EDPRSR: 0000000b (Power:On DLK:Unlock)
> + EDPCSR: [<ffff0000087a64c0>] debug_notifier_call+0x108/0x288
> + EDCIDSR: 00000000
> + EDVIDSR: 90000000 (State:Non-secure Mode:EL1/0 Width:64bits VMID:0)
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 3/8] coresight: of_get_coresight_platform_data: Add missing of_node_put
2017-04-06 13:30 [PATCH v6 0/8] coresight: enable debug module Leo Yan
2017-04-06 13:30 ` [PATCH v6 1/8] coresight: bindings for CPU " Leo Yan
2017-04-06 13:30 ` [PATCH v6 2/8] doc: Add documentation for Coresight CPU debug Leo Yan
@ 2017-04-06 13:30 ` Leo Yan
2017-04-18 15:09 ` Mathieu Poirier
2017-04-06 13:30 ` [PATCH v6 4/8] coresight: refactor with function of_coresight_get_cpu Leo Yan
` (4 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Leo Yan @ 2017-04-06 13:30 UTC (permalink / raw)
To: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel, devicetree, linux-arm-kernel, linux-arm-msm,
linux-soc, Mike Leach, Sudeep Holla
Cc: Leo Yan
From: Suzuki K Poulose <suzuki.poulose@arm.com>
The of_get_coresight_platform_data iterates over the possible CPU nodes
to find a given cpu phandle. However it does not drop the reference
to the node pointer returned by the of_get_coresight_platform_data.
This patch also introduces another minor fix is to use
of_cpu_device_node_get() to replace of_get_cpu_node().
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
[Leo: minor tweaks for of_get_coresight_platform_data]
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
drivers/hwtracing/coresight/of_coresight.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
index 629e031..1a77280 100644
--- a/drivers/hwtracing/coresight/of_coresight.c
+++ b/drivers/hwtracing/coresight/of_coresight.c
@@ -108,7 +108,8 @@ struct coresight_platform_data *of_get_coresight_platform_data(
struct coresight_platform_data *pdata;
struct of_endpoint endpoint, rendpoint;
struct device *rdev;
- struct device_node *dn;
+ bool found;
+ struct device_node *dn, *np;
struct device_node *ep = NULL;
struct device_node *rparent = NULL;
struct device_node *rport = NULL;
@@ -175,17 +176,19 @@ struct coresight_platform_data *of_get_coresight_platform_data(
} while (ep);
}
- /* Affinity defaults to CPU0 */
- pdata->cpu = 0;
dn = of_parse_phandle(node, "cpu", 0);
- for (cpu = 0; dn && cpu < nr_cpu_ids; cpu++) {
- if (dn == of_get_cpu_node(cpu, NULL)) {
- pdata->cpu = cpu;
+ for_each_possible_cpu(cpu) {
+ np = of_cpu_device_node_get(cpu);
+ found = (dn == np);
+ of_node_put(np);
+ if (found)
break;
- }
}
of_node_put(dn);
+ /* Affinity to CPU0 if no cpu nodes are found */
+ pdata->cpu = found ? cpu : 0;
+
return pdata;
}
EXPORT_SYMBOL_GPL(of_get_coresight_platform_data);
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 3/8] coresight: of_get_coresight_platform_data: Add missing of_node_put
2017-04-06 13:30 ` [PATCH v6 3/8] coresight: of_get_coresight_platform_data: Add missing of_node_put Leo Yan
@ 2017-04-18 15:09 ` Mathieu Poirier
0 siblings, 0 replies; 23+ messages in thread
From: Mathieu Poirier @ 2017-04-18 15:09 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On Thu, Apr 06, 2017 at 09:30:56PM +0800, Leo Yan wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>
> The of_get_coresight_platform_data iterates over the possible CPU nodes
> to find a given cpu phandle. However it does not drop the reference
> to the node pointer returned by the of_get_coresight_platform_data.
>
> This patch also introduces another minor fix is to use
> of_cpu_device_node_get() to replace of_get_cpu_node().
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> [Leo: minor tweaks for of_get_coresight_platform_data]
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
Suzuki sent a Reviewed-by for this, it should be added here.
> ---
> drivers/hwtracing/coresight/of_coresight.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index 629e031..1a77280 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -108,7 +108,8 @@ struct coresight_platform_data *of_get_coresight_platform_data(
> struct coresight_platform_data *pdata;
> struct of_endpoint endpoint, rendpoint;
> struct device *rdev;
> - struct device_node *dn;
> + bool found;
> + struct device_node *dn, *np;
> struct device_node *ep = NULL;
> struct device_node *rparent = NULL;
> struct device_node *rport = NULL;
> @@ -175,17 +176,19 @@ struct coresight_platform_data *of_get_coresight_platform_data(
> } while (ep);
> }
>
> - /* Affinity defaults to CPU0 */
> - pdata->cpu = 0;
> dn = of_parse_phandle(node, "cpu", 0);
> - for (cpu = 0; dn && cpu < nr_cpu_ids; cpu++) {
> - if (dn == of_get_cpu_node(cpu, NULL)) {
> - pdata->cpu = cpu;
> + for_each_possible_cpu(cpu) {
> + np = of_cpu_device_node_get(cpu);
> + found = (dn == np);
> + of_node_put(np);
> + if (found)
> break;
> - }
> }
> of_node_put(dn);
>
> + /* Affinity to CPU0 if no cpu nodes are found */
> + pdata->cpu = found ? cpu : 0;
> +
> return pdata;
> }
> EXPORT_SYMBOL_GPL(of_get_coresight_platform_data);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 4/8] coresight: refactor with function of_coresight_get_cpu
2017-04-06 13:30 [PATCH v6 0/8] coresight: enable debug module Leo Yan
` (2 preceding siblings ...)
2017-04-06 13:30 ` [PATCH v6 3/8] coresight: of_get_coresight_platform_data: Add missing of_node_put Leo Yan
@ 2017-04-06 13:30 ` Leo Yan
2017-04-06 13:30 ` [PATCH v6 5/8] coresight: use const for device_node structures Leo Yan
` (3 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Leo Yan @ 2017-04-06 13:30 UTC (permalink / raw)
To: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel, devicetree, linux-arm-kernel, linux-arm-msm,
linux-soc, Mike Leach, Sudeep Holla
Cc: Leo Yan
This is refactor to add function of_coresight_get_cpu(), so it's used to
retrieve CPU id for coresight component. Finally can use it as a common
function for multiple places.
Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
drivers/hwtracing/coresight/of_coresight.c | 43 +++++++++++++++++++-----------
include/linux/coresight.h | 2 ++
2 files changed, 30 insertions(+), 15 deletions(-)
diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
index 1a77280..78d2399 100644
--- a/drivers/hwtracing/coresight/of_coresight.c
+++ b/drivers/hwtracing/coresight/of_coresight.c
@@ -101,15 +101,39 @@ static int of_coresight_alloc_memory(struct device *dev,
return 0;
}
+int of_coresight_get_cpu(struct device_node *node)
+{
+ int cpu;
+ bool found;
+ struct device_node *dn, *np;
+
+ dn = of_parse_phandle(node, "cpu", 0);
+
+ /* Affinity defaults to CPU0 */
+ if (!dn)
+ return 0;
+
+ for_each_possible_cpu(cpu) {
+ np = of_cpu_device_node_get(cpu);
+ found = (dn == np);
+ of_node_put(np);
+ if (found)
+ break;
+ }
+ of_node_put(dn);
+
+ /* Affinity to CPU0 if no cpu nodes are found */
+ return found ? cpu : 0;
+}
+EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
+
struct coresight_platform_data *of_get_coresight_platform_data(
struct device *dev, struct device_node *node)
{
- int i = 0, ret = 0, cpu;
+ int i = 0, ret = 0;
struct coresight_platform_data *pdata;
struct of_endpoint endpoint, rendpoint;
struct device *rdev;
- bool found;
- struct device_node *dn, *np;
struct device_node *ep = NULL;
struct device_node *rparent = NULL;
struct device_node *rport = NULL;
@@ -176,18 +200,7 @@ struct coresight_platform_data *of_get_coresight_platform_data(
} while (ep);
}
- dn = of_parse_phandle(node, "cpu", 0);
- for_each_possible_cpu(cpu) {
- np = of_cpu_device_node_get(cpu);
- found = (dn == np);
- of_node_put(np);
- if (found)
- break;
- }
- of_node_put(dn);
-
- /* Affinity to CPU0 if no cpu nodes are found */
- pdata->cpu = found ? cpu : 0;
+ pdata->cpu = of_coresight_get_cpu(node);
return pdata;
}
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 2a5982c..bf96678 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -263,9 +263,11 @@ static inline int coresight_timeout(void __iomem *addr, u32 offset,
#endif
#ifdef CONFIG_OF
+extern int of_coresight_get_cpu(struct device_node *node);
extern struct coresight_platform_data *of_get_coresight_platform_data(
struct device *dev, struct device_node *node);
#else
+static inline int of_coresight_get_cpu(struct device_node *node) { return 0; }
static inline struct coresight_platform_data *of_get_coresight_platform_data(
struct device *dev, struct device_node *node) { return NULL; }
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v6 5/8] coresight: use const for device_node structures
2017-04-06 13:30 [PATCH v6 0/8] coresight: enable debug module Leo Yan
` (3 preceding siblings ...)
2017-04-06 13:30 ` [PATCH v6 4/8] coresight: refactor with function of_coresight_get_cpu Leo Yan
@ 2017-04-06 13:30 ` Leo Yan
2017-04-18 15:24 ` Mathieu Poirier
2017-04-06 13:30 ` [PATCH v6 6/8] coresight: add support for CPU debug module Leo Yan
` (2 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Leo Yan @ 2017-04-06 13:30 UTC (permalink / raw)
To: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel, devicetree, linux-arm-kernel, linux-arm-msm,
linux-soc, Mike Leach, Sudeep Holla
Cc: Leo Yan
Almost low level functions from open firmware have used const to
qualify device_node structures, so add const for device_node
parameters in of_coresight related functions.
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
drivers/hwtracing/coresight/of_coresight.c | 6 +++---
include/linux/coresight.h | 8 ++++----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
index 78d2399..46eec0f 100644
--- a/drivers/hwtracing/coresight/of_coresight.c
+++ b/drivers/hwtracing/coresight/of_coresight.c
@@ -52,7 +52,7 @@ of_coresight_get_endpoint_device(struct device_node *endpoint)
endpoint, of_dev_node_match);
}
-static void of_coresight_get_ports(struct device_node *node,
+static void of_coresight_get_ports(const struct device_node *node,
int *nr_inport, int *nr_outport)
{
struct device_node *ep = NULL;
@@ -101,7 +101,7 @@ static int of_coresight_alloc_memory(struct device *dev,
return 0;
}
-int of_coresight_get_cpu(struct device_node *node)
+int of_coresight_get_cpu(const struct device_node *node)
{
int cpu;
bool found;
@@ -128,7 +128,7 @@ int of_coresight_get_cpu(struct device_node *node)
EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
struct coresight_platform_data *of_get_coresight_platform_data(
- struct device *dev, struct device_node *node)
+ struct device *dev, const struct device_node *node)
{
int i = 0, ret = 0;
struct coresight_platform_data *pdata;
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index bf96678..4915254 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -263,13 +263,13 @@ static inline int coresight_timeout(void __iomem *addr, u32 offset,
#endif
#ifdef CONFIG_OF
-extern int of_coresight_get_cpu(struct device_node *node);
+extern int of_coresight_get_cpu(const struct device_node *node);
extern struct coresight_platform_data *of_get_coresight_platform_data(
- struct device *dev, struct device_node *node);
+ struct device *dev, const struct device_node *node);
#else
-static inline int of_coresight_get_cpu(struct device_node *node) { return 0; }
+static inline int of_coresight_get_cpu(const struct device_node *node) { return 0; }
static inline struct coresight_platform_data *of_get_coresight_platform_data(
- struct device *dev, struct device_node *node) { return NULL; }
+ struct device *dev, const struct device_node *node) { return NULL; }
#endif
#ifdef CONFIG_PID_NS
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 5/8] coresight: use const for device_node structures
2017-04-06 13:30 ` [PATCH v6 5/8] coresight: use const for device_node structures Leo Yan
@ 2017-04-18 15:24 ` Mathieu Poirier
2017-04-18 23:13 ` Leo Yan
0 siblings, 1 reply; 23+ messages in thread
From: Mathieu Poirier @ 2017-04-18 15:24 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On Thu, Apr 06, 2017 at 09:30:58PM +0800, Leo Yan wrote:
> Almost low level functions from open firmware have used const to
> qualify device_node structures, so add const for device_node
> parameters in of_coresight related functions.
>
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
I agree with these changes but the patch needs to be split up - please see
below.
> ---
> drivers/hwtracing/coresight/of_coresight.c | 6 +++---
> include/linux/coresight.h | 8 ++++----
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index 78d2399..46eec0f 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -52,7 +52,7 @@ of_coresight_get_endpoint_device(struct device_node *endpoint)
> endpoint, of_dev_node_match);
> }
>
> -static void of_coresight_get_ports(struct device_node *node,
> +static void of_coresight_get_ports(const struct device_node *node,
> int *nr_inport, int *nr_outport)
Move this to a patch by itself as it is not related to this driver.
> {
> struct device_node *ep = NULL;
> @@ -101,7 +101,7 @@ static int of_coresight_alloc_memory(struct device *dev,
> return 0;
> }
>
> -int of_coresight_get_cpu(struct device_node *node)
> +int of_coresight_get_cpu(const struct device_node *node)
Move this to the previous patch in this set. There is not need to undo what you
just did there.
> {
> int cpu;
> bool found;
> @@ -128,7 +128,7 @@ int of_coresight_get_cpu(struct device_node *node)
> EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
>
> struct coresight_platform_data *of_get_coresight_platform_data(
> - struct device *dev, struct device_node *node)
> + struct device *dev, const struct device_node *node)
Same here, move this to a new patch (the same one as of_coresight_get_ports()).
> {
> int i = 0, ret = 0;
> struct coresight_platform_data *pdata;
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index bf96678..4915254 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -263,13 +263,13 @@ static inline int coresight_timeout(void __iomem *addr, u32 offset,
> #endif
>
> #ifdef CONFIG_OF
> -extern int of_coresight_get_cpu(struct device_node *node);
> +extern int of_coresight_get_cpu(const struct device_node *node);
> extern struct coresight_platform_data *of_get_coresight_platform_data(
> - struct device *dev, struct device_node *node);
> + struct device *dev, const struct device_node *node);
> #else
> -static inline int of_coresight_get_cpu(struct device_node *node) { return 0; }
> +static inline int of_coresight_get_cpu(const struct device_node *node) { return 0; }
> static inline struct coresight_platform_data *of_get_coresight_platform_data(
> - struct device *dev, struct device_node *node) { return NULL; }
> + struct device *dev, const struct device_node *node) { return NULL; }
> #endif
>
> #ifdef CONFIG_PID_NS
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 5/8] coresight: use const for device_node structures
2017-04-18 15:24 ` Mathieu Poirier
@ 2017-04-18 23:13 ` Leo Yan
0 siblings, 0 replies; 23+ messages in thread
From: Leo Yan @ 2017-04-18 23:13 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
Hi Mathieu,
On Tue, Apr 18, 2017 at 09:24:47AM -0600, Mathieu Poirier wrote:
> On Thu, Apr 06, 2017 at 09:30:58PM +0800, Leo Yan wrote:
> > Almost low level functions from open firmware have used const to
> > qualify device_node structures, so add const for device_node
> > parameters in of_coresight related functions.
> >
> > Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>
> I agree with these changes but the patch needs to be split up - please see
> below.
>
> > ---
> > drivers/hwtracing/coresight/of_coresight.c | 6 +++---
> > include/linux/coresight.h | 8 ++++----
> > 2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> > index 78d2399..46eec0f 100644
> > --- a/drivers/hwtracing/coresight/of_coresight.c
> > +++ b/drivers/hwtracing/coresight/of_coresight.c
> > @@ -52,7 +52,7 @@ of_coresight_get_endpoint_device(struct device_node *endpoint)
> > endpoint, of_dev_node_match);
> > }
> >
> > -static void of_coresight_get_ports(struct device_node *node,
> > +static void of_coresight_get_ports(const struct device_node *node,
> > int *nr_inport, int *nr_outport)
>
> Move this to a patch by itself as it is not related to this driver.
>
> > {
> > struct device_node *ep = NULL;
> > @@ -101,7 +101,7 @@ static int of_coresight_alloc_memory(struct device *dev,
> > return 0;
> > }
> >
> > -int of_coresight_get_cpu(struct device_node *node)
> > +int of_coresight_get_cpu(const struct device_node *node)
>
> Move this to the previous patch in this set. There is not need to undo what you
> just did there.
>
> > {
> > int cpu;
> > bool found;
> > @@ -128,7 +128,7 @@ int of_coresight_get_cpu(struct device_node *node)
> > EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
> >
> > struct coresight_platform_data *of_get_coresight_platform_data(
> > - struct device *dev, struct device_node *node)
> > + struct device *dev, const struct device_node *node)
>
> Same here, move this to a new patch (the same one as of_coresight_get_ports()).
Yeah, this is better than my form. Will change.
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-06 13:30 [PATCH v6 0/8] coresight: enable debug module Leo Yan
` (4 preceding siblings ...)
2017-04-06 13:30 ` [PATCH v6 5/8] coresight: use const for device_node structures Leo Yan
@ 2017-04-06 13:30 ` Leo Yan
2017-04-18 17:40 ` Mathieu Poirier
` (2 more replies)
2017-04-06 13:31 ` [PATCH v6 7/8] arm64: dts: hi6220: register " Leo Yan
2017-04-06 13:31 ` [PATCH v6 8/8] arm64: dts: qcom: msm8916: Add debug unit Leo Yan
7 siblings, 3 replies; 23+ messages in thread
From: Leo Yan @ 2017-04-06 13:30 UTC (permalink / raw)
To: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel, devicetree, linux-arm-kernel, linux-arm-msm,
linux-soc, Mike Leach, Sudeep Holla
Cc: Leo Yan
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 triggers
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
this requires the CPU power domain stays on when access related debug
registers, but the power management for CPU power domain is quite
dependent on SoC integration for power management. For the platforms
which with sane power controller implementations, this driver follows
the method to set EDPRCR to try to pull the CPU out of low power state
and then set 'no power down request' bit so the CPU has no chance to
lose power.
If the SoC has not followed up this design well for power management
controller, the user should use the command line parameter or sysfs
to constrain all or partial idle states to ensure the CPU power
domain is enabled and access coresight CPU debug component safely.
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
drivers/hwtracing/coresight/Kconfig | 14 +
drivers/hwtracing/coresight/Makefile | 1 +
drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
3 files changed, 682 insertions(+)
create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 130cb21..8d55d6d 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -89,4 +89,18 @@ config CORESIGHT_STM
logging useful software events or data coming from various entities
in the system, possibly running different OSs
+config CORESIGHT_CPU_DEBUG
+ tristate "CoreSight CPU Debug driver"
+ depends on ARM || ARM64
+ depends on DEBUG_FS
+ help
+ This driver provides support for coresight debugging module. This
+ is primarily used to dump sample-based profiling registers when
+ system triggers panic, the driver will parse context registers so
+ can quickly get to know program counter (PC), secure state,
+ exception level, etc. Before use debugging functionality, platform
+ needs to ensure the clock domain and power domain are enabled
+ properly, please refer Documentation/trace/coresight-cpu-debug.txt
+ for detailed description and the example for usage.
+
endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index af480d9..433d590 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_CPU_DEBUG) += coresight-cpu-debug.o
diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
new file mode 100644
index 0000000..8470e31
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -0,0 +1,667 @@
+/*
+ * 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/debugfs.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/pm_qos.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 EDPRCR 0x310
+#define EDPRSR 0x314
+#define EDDEVID1 0xFC4
+#define EDDEVID 0xFC8
+
+#define EDPCSR_PROHIBITED 0xFFFFFFFF
+
+/* bits definition for EDPCSR */
+#define EDPCSR_THUMB BIT(0)
+#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
+#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
+
+/* bits definition for EDPRCR */
+#define EDPRCR_COREPURQ BIT(3)
+#define EDPRCR_CORENPDRQ BIT(0)
+
+/* 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:PSCROffset
+ *
+ * NOTE: armv8 and armv7 have different definition for the register,
+ * so consolidate the bits definition as below:
+ *
+ * 0b0000 - Sample offset applies based on the instruction state, we
+ * rely on EDDEVID to check if EDPCSR is implemented or not
+ * 0b0001 - No offset applies.
+ * 0b0010 - No offset applies, but do not use in AArch32 mode
+ *
+ */
+#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
+#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
+#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
+
+/* bits definition for EDDEVID */
+#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
+#define EDDEVID_IMPL_EDPCSR (0x1)
+#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
+#define EDDEVID_IMPL_FULL (0x3)
+
+#define DEBUG_WAIT_SLEEP 1000
+#define DEBUG_WAIT_TIMEOUT 32000
+
+struct debug_drvdata {
+ void __iomem *base;
+ struct device *dev;
+ int cpu;
+
+ bool edpcsr_present;
+ bool edcidsr_present;
+ bool edvidsr_present;
+ bool pc_has_offset;
+
+ u32 edpcsr;
+ u32 edpcsr_hi;
+ u32 edprsr;
+ u32 edvidsr;
+ u32 edcidsr;
+};
+
+static DEFINE_MUTEX(debug_lock);
+static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
+static int debug_count;
+static struct dentry *debug_debugfs_dir;
+
+static bool debug_enable;
+module_param_named(enable, debug_enable, bool, 0600);
+MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
+ "(default is 0, which means is disabled by default)");
+
+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_force_cpu_powered_up(struct debug_drvdata *drvdata)
+{
+ bool retried = false;
+ u32 edprcr;
+
+try_again:
+
+ /*
+ * Send request to power management controller and assert
+ * DBGPWRUPREQ signal; if power management controller has
+ * sane implementation, it should enable CPU power domain
+ * in case CPU is in low power state.
+ */
+ edprcr = readl_relaxed(drvdata->base + EDPRCR);
+ edprcr |= EDPRCR_COREPURQ;
+ writel_relaxed(edprcr, drvdata->base + EDPRCR);
+
+ /* Wait for CPU to be powered up (timeout~=32ms) */
+ if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
+ drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
+ DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
+ /*
+ * Unfortunately the CPU cannot be powered up, so return
+ * back and later has no permission to access other
+ * registers. For this case, should disable CPU low power
+ * states to ensure CPU power domain is enabled!
+ */
+ pr_err("%s: power up request for CPU%d failed\n",
+ __func__, drvdata->cpu);
+ return;
+ }
+
+ /*
+ * At this point the CPU is powered up, so set the no powerdown
+ * request bit so we don't lose power and emulate power down.
+ */
+ edprcr = readl_relaxed(drvdata->base + EDPRCR);
+ edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
+ writel_relaxed(edprcr, drvdata->base + EDPRCR);
+
+ drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
+
+ /* Bail out if CPU is powered up */
+ if (likely(drvdata->edprsr & EDPRSR_PU))
+ return;
+
+ /*
+ * Handle race condition if CPU has been waken up but it sleeps
+ * again if EDPRCR_CORENPDRQ has been flipped, so try to run
+ * waken flow one more time.
+ */
+ if (!retried) {
+ retried = true;
+ goto try_again;
+ }
+}
+
+static void debug_read_regs(struct debug_drvdata *drvdata)
+{
+ u32 save_edprcr;
+
+ CS_UNLOCK(drvdata->base);
+
+ /* Unlock os lock */
+ debug_os_unlock(drvdata);
+
+ /* Save EDPRCR register */
+ save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
+
+ /*
+ * Ensure CPU power domain is enabled to let registers
+ * are accessiable.
+ */
+ debug_force_cpu_powered_up(drvdata);
+
+ if (!debug_access_permitted(drvdata))
+ goto out;
+
+ 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)
+ goto out;
+
+ /*
+ * 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.
+ */
+ if (IS_ENABLED(CONFIG_64BIT))
+ drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
+
+ if (drvdata->edcidsr_present)
+ drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
+
+ if (drvdata->edvidsr_present)
+ drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
+
+out:
+ /* Restore EDPRCR register */
+ writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
+
+ CS_LOCK(drvdata->base);
+}
+
+static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
+{
+ unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
+ unsigned long pc;
+
+ if (IS_ENABLED(CONFIG_64BIT))
+ return (unsigned long)drvdata->edpcsr_hi << 32 |
+ (unsigned long)drvdata->edpcsr;
+
+ pc = (unsigned long)drvdata->edpcsr;
+
+ if (drvdata->pc_has_offset) {
+ 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;
+}
+
+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)) {
+ 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;
+ }
+
+ pc = debug_adjust_pc(drvdata);
+ pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
+
+ if (drvdata->edcidsr_present)
+ pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
+
+ if (drvdata->edvidsr_present)
+ pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits 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 ? 64 : 32,
+ drvdata->edvidsr & (u32)EDVIDSR_VMID);
+}
+
+static void debug_init_arch_data(void *info)
+{
+ struct debug_drvdata *drvdata = info;
+ u32 mode, pcsr_offset;
+ u32 eddevid, eddevid1;
+
+ CS_UNLOCK(drvdata->base);
+
+ /* Read device info */
+ eddevid = readl_relaxed(drvdata->base + EDDEVID);
+ eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
+
+ CS_LOCK(drvdata->base);
+
+ /* Parse implementation feature */
+ mode = eddevid & EDDEVID_PCSAMPLE_MODE;
+ pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
+
+ drvdata->edpcsr_present = false;
+ drvdata->edcidsr_present = false;
+ drvdata->edvidsr_present = false;
+ drvdata->pc_has_offset = false;
+
+ switch (mode) {
+ case EDDEVID_IMPL_FULL:
+ drvdata->edvidsr_present = true;
+ /* Fall through */
+ case EDDEVID_IMPL_EDPCSR_EDCIDSR:
+ drvdata->edcidsr_present = true;
+ /* Fall through */
+ case EDDEVID_IMPL_EDPCSR:
+ /*
+ * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
+ * define if has the offset for PC sampling value; if read
+ * back EDDEVID1.PCSROffset == 0x2, then this means the debug
+ * module does not sample the instruction set state when
+ * armv8 CPU in AArch32 state.
+ */
+ drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
+ (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
+
+ drvdata->pc_has_offset =
+ (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
+ break;
+ default:
+ break;
+ }
+}
+
+/*
+ * Dump out information on panic.
+ */
+static int debug_notifier_call(struct notifier_block *self,
+ unsigned long v, void *p)
+{
+ int cpu;
+ struct debug_drvdata *drvdata;
+
+ pr_emerg("ARM external debug module:\n");
+
+ for_each_possible_cpu(cpu) {
+ drvdata = per_cpu(debug_drvdata, cpu);
+ if (!drvdata)
+ continue;
+
+ pr_emerg("CPU[%d]:\n", drvdata->cpu);
+
+ debug_read_regs(drvdata);
+ debug_dump_regs(drvdata);
+ }
+
+ return 0;
+}
+
+static struct notifier_block debug_notifier = {
+ .notifier_call = debug_notifier_call,
+};
+
+static int debug_enable_func(void)
+{
+ struct debug_drvdata *drvdata;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ drvdata = per_cpu(debug_drvdata, cpu);
+ if (!drvdata)
+ continue;
+
+ pm_runtime_get_sync(drvdata->dev);
+ }
+
+ return atomic_notifier_chain_register(&panic_notifier_list,
+ &debug_notifier);
+}
+
+static int debug_disable_func(void)
+{
+ struct debug_drvdata *drvdata;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ drvdata = per_cpu(debug_drvdata, cpu);
+ if (!drvdata)
+ continue;
+
+ pm_runtime_put(drvdata->dev);
+ }
+
+ return atomic_notifier_chain_unregister(&panic_notifier_list,
+ &debug_notifier);
+}
+
+static ssize_t debug_func_knob_write(struct file *f,
+ const char __user *buf, size_t count, loff_t *ppos)
+{
+ u8 val;
+ int ret;
+
+ ret = kstrtou8_from_user(buf, count, 2, &val);
+ if (ret)
+ return ret;
+
+ mutex_lock(&debug_lock);
+
+ if (val == debug_enable)
+ goto out;
+
+ if (val)
+ ret = debug_enable_func();
+ else
+ ret = debug_disable_func();
+
+ if (ret) {
+ pr_err("%s: unable to %s debug function: %d\n",
+ __func__, val ? "enable" : "disable", ret);
+ goto err;
+ }
+
+ debug_enable = val;
+out:
+ ret = count;
+err:
+ mutex_unlock(&debug_lock);
+ return ret;
+}
+
+static ssize_t debug_func_knob_read(struct file *f,
+ char __user *ubuf, size_t count, loff_t *ppos)
+{
+ ssize_t ret;
+ char buf[2];
+
+ mutex_lock(&debug_lock);
+
+ buf[0] = '0' + debug_enable;
+ buf[1] = '\n';
+ ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
+
+ mutex_unlock(&debug_lock);
+ return ret;
+}
+
+static const struct file_operations debug_func_knob_fops = {
+ .open = simple_open,
+ .read = debug_func_knob_read,
+ .write = debug_func_knob_write,
+};
+
+static int debug_func_init(void)
+{
+ struct dentry *file;
+ int ret;
+
+ /* Create debugfs node */
+ debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
+ if (!debug_debugfs_dir) {
+ pr_err("%s: unable to create debugfs directory\n", __func__);
+ return -ENOMEM;
+ }
+
+ file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
+ debug_debugfs_dir, NULL, &debug_func_knob_fops);
+ if (!file) {
+ pr_err("%s: unable to create enable knob file\n", __func__);
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ /* Use sysfs node to enable functionality */
+ if (!debug_enable)
+ return 0;
+
+ /* Register function to be called for panic */
+ ret = atomic_notifier_chain_register(&panic_notifier_list,
+ &debug_notifier);
+ if (ret) {
+ pr_err("%s: unable to register notifier: %d\n",
+ __func__, ret);
+ goto err;
+ }
+
+ return 0;
+
+err:
+ debugfs_remove_recursive(debug_debugfs_dir);
+ return ret;
+}
+
+static void debug_func_exit(void)
+{
+ debugfs_remove_recursive(debug_debugfs_dir);
+
+ /* Unregister panic notifier callback */
+ if (debug_enable)
+ atomic_notifier_chain_unregister(&panic_notifier_list,
+ &debug_notifier);
+}
+
+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;
+ int ret;
+
+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
+ if (per_cpu(debug_drvdata, drvdata->cpu)) {
+ dev_err(dev, "CPU%d drvdata has been initialized\n",
+ drvdata->cpu);
+ return -EBUSY;
+ }
+
+ drvdata->dev = &adev->dev;
+ amba_set_drvdata(adev, 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;
+ ret = smp_call_function_single(drvdata->cpu,
+ debug_init_arch_data, drvdata, 1);
+ put_online_cpus();
+
+ if (ret) {
+ dev_err(dev, "CPU%d debug arch init failed\n", drvdata->cpu);
+ goto err;
+ }
+
+ if (!drvdata->edpcsr_present) {
+ dev_err(dev, "CPU%d sample-based profiling isn't implemented\n",
+ drvdata->cpu);
+ ret = -ENXIO;
+ goto err;
+ }
+
+ if (!debug_count++) {
+ ret = debug_func_init();
+ if (ret)
+ goto err_func_init;
+ }
+
+ if (!debug_enable)
+ pm_runtime_put(dev);
+
+ dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu);
+ return 0;
+
+err_func_init:
+ debug_count--;
+err:
+ per_cpu(debug_drvdata, drvdata->cpu) = NULL;
+ return ret;
+}
+
+static int debug_remove(struct amba_device *adev)
+{
+ struct device *dev = &adev->dev;
+ struct debug_drvdata *drvdata = amba_get_drvdata(adev);
+
+ per_cpu(debug_drvdata, drvdata->cpu) = NULL;
+
+ if (debug_enable)
+ pm_runtime_put(dev);
+
+ if (!--debug_count)
+ debug_func_exit();
+
+ return 0;
+}
+
+static struct amba_id debug_ids[] = {
+ { /* Debug for Cortex-A53 */
+ .id = 0x000bbd03,
+ .mask = 0x000fffff,
+ },
+ { /* Debug for Cortex-A57 */
+ .id = 0x000bbd07,
+ .mask = 0x000fffff,
+ },
+ { /* Debug for Cortex-A72 */
+ .id = 0x000bbd08,
+ .mask = 0x000fffff,
+ },
+ { 0, 0 },
+};
+
+static struct amba_driver debug_driver = {
+ .drv = {
+ .name = "coresight-cpu-debug",
+ .suppress_bind_attrs = true,
+ },
+ .probe = debug_probe,
+ .remove = debug_remove,
+ .id_table = debug_ids,
+};
+
+module_amba_driver(debug_driver);
+
+MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
+MODULE_DESCRIPTION("ARM Coresight CPU Debug Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-06 13:30 ` [PATCH v6 6/8] coresight: add support for CPU debug module Leo Yan
@ 2017-04-18 17:40 ` Mathieu Poirier
2017-04-19 0:18 ` Leo Yan
2017-04-19 13:23 ` Suzuki K Poulose
2017-04-19 17:49 ` Mathieu Poirier
2 siblings, 1 reply; 23+ messages in thread
From: Mathieu Poirier @ 2017-04-18 17:40 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On Thu, Apr 06, 2017 at 09:30:59PM +0800, 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 triggers
> 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
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
>
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
This is coming along well - a few comment below. In your next revision please
add GKH to the 'To' list.
Thanks,
Mathieu
> ---
> drivers/hwtracing/coresight/Kconfig | 14 +
> drivers/hwtracing/coresight/Makefile | 1 +
> drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
> 3 files changed, 682 insertions(+)
> create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..8d55d6d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,18 @@ config CORESIGHT_STM
> logging useful software events or data coming from various entities
> in the system, possibly running different OSs
>
> +config CORESIGHT_CPU_DEBUG
> + tristate "CoreSight CPU Debug driver"
> + depends on ARM || ARM64
> + depends on DEBUG_FS
> + help
> + This driver provides support for coresight debugging module. This
> + is primarily used to dump sample-based profiling registers when
> + system triggers panic, the driver will parse context registers so
> + can quickly get to know program counter (PC), secure state,
> + exception level, etc. Before use debugging functionality, platform
> + needs to ensure the clock domain and power domain are enabled
> + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> + for detailed description and the example for usage.
> +
> endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index af480d9..433d590 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_CPU_DEBUG) += coresight-cpu-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> new file mode 100644
> index 0000000..8470e31
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,667 @@
> +/*
> + * 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/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pm_qos.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 EDPRCR 0x310
> +#define EDPRSR 0x314
> +#define EDDEVID1 0xFC4
> +#define EDDEVID 0xFC8
> +
> +#define EDPCSR_PROHIBITED 0xFFFFFFFF
> +
> +/* bits definition for EDPCSR */
> +#define EDPCSR_THUMB BIT(0)
> +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
> +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
> +
> +/* bits definition for EDPRCR */
> +#define EDPRCR_COREPURQ BIT(3)
> +#define EDPRCR_CORENPDRQ BIT(0)
> +
> +/* 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:PSCROffset
> + *
> + * NOTE: armv8 and armv7 have different definition for the register,
> + * so consolidate the bits definition as below:
> + *
> + * 0b0000 - Sample offset applies based on the instruction state, we
> + * rely on EDDEVID to check if EDPCSR is implemented or not
> + * 0b0001 - No offset applies.
> + * 0b0010 - No offset applies, but do not use in AArch32 mode
> + *
> + */
> +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
> +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
> +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
> +
> +/* bits definition for EDDEVID */
> +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
> +#define EDDEVID_IMPL_EDPCSR (0x1)
> +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
> +#define EDDEVID_IMPL_FULL (0x3)
> +
> +#define DEBUG_WAIT_SLEEP 1000
> +#define DEBUG_WAIT_TIMEOUT 32000
> +
> +struct debug_drvdata {
> + void __iomem *base;
> e struct device *dev;
> + int cpu;
> +
> + bool edpcsr_present;
> + bool edcidsr_present;
> + bool edvidsr_present;
> + bool pc_has_offset;
> +
> + u32 edpcsr;
> + u32 edpcsr_hi;
> + u32 edprsr;
> + u32 edvidsr;
> + u32 edcidsr;
> +};
> +
> +static DEFINE_MUTEX(debug_lock);
> +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
> +static int debug_count;
> +static struct dentry *debug_debugfs_dir;
> +
> +static bool debug_enable;
> +module_param_named(enable, debug_enable, bool, 0600);
> +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> + "(default is 0, which means is disabled by default)");
For this driver we have a debugFS interface so I question the validity of a
kernel module parameter. Other than adding complexity to the code it offers no
real added value. If a user is to insmod a module, it is just as easy to switch
on the functionality using debugFS in a second step.
> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> + /* Unlocks the debug registers */
> + writel_relaxed(0x0, drvdata->base + EDOSLAR);
Here the wmb is to make sure reordering (either at compile time or in the CPU)
doesn't happend. You need a comment here otherwise checkpatch will complain.
Speaking of which, did you run this through checkpatch?
> + 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_force_cpu_powered_up(struct debug_drvdata *drvdata)
> +{
> + bool retried = false;
> + u32 edprcr;
> +
> +try_again:
> +
> + /*
> + * Send request to power management controller and assert
> + * DBGPWRUPREQ signal; if power management controller has
> + * sane implementation, it should enable CPU power domain
> + * in case CPU is in low power state.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + /* Wait for CPU to be powered up (timeout~=32ms) */
> + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
> + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
> + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
> + /*
> + * Unfortunately the CPU cannot be powered up, so return
> + * back and later has no permission to access other
> + * registers. For this case, should disable CPU low power
> + * states to ensure CPU power domain is enabled!
> + */
> + pr_err("%s: power up request for CPU%d failed\n",
> + __func__, drvdata->cpu);
> + return;
> + }
> +
> + /*
> + * At this point the CPU is powered up, so set the no powerdown
> + * request bit so we don't lose power and emulate power down.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> +
> + /* Bail out if CPU is powered up */
> + if (likely(drvdata->edprsr & EDPRSR_PU))
> + return;
/* The core power domain got switched off on use, try again */
if (unlikely(!drvdata->edprsr & EDPRSR_PU))
goto try_again;
I understand you don't want to introduce a infinite loop but if that happens
here, something else has gone very wrong. The above readx_poll_timeout_atomic
loop should take care of bailing out in case of problems. That way you also get
to rid of the retried variable and the code is more simple.
> +
> + /*
> + * Handle race condition if CPU has been waken up but it sleeps
> + * again if EDPRCR_CORENPDRQ has been flipped, so try to run
> + * waken flow one more time.
> + */
> + if (!retried) {
> + retried = true;
> + goto try_again;
> + }
> +}
> +
> +static void debug_read_regs(struct debug_drvdata *drvdata)
> +{
> + u32 save_edprcr;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Unlock os lock */
> + debug_os_unlock(drvdata);
> +
> + /* Save EDPRCR register */
> + save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
> +
> + /*
> + * Ensure CPU power domain is enabled to let registers
> + * are accessiable.
> + */
> + debug_force_cpu_powered_up(drvdata);
> +
> + if (!debug_access_permitted(drvdata))
> + goto out;
> +
> + 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)
> + goto out;
> +
> + /*
> + * 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.
> + */
> + if (IS_ENABLED(CONFIG_64BIT))
> + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> + if (drvdata->edcidsr_present)
> + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
> +
> + if (drvdata->edvidsr_present)
> + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
> +
> +out:
> + /* Restore EDPRCR register */
> + writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
> +
> + CS_LOCK(drvdata->base);
> +}
> +
> +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
> +{
> + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
> + unsigned long pc;
> +
> + if (IS_ENABLED(CONFIG_64BIT))
> + return (unsigned long)drvdata->edpcsr_hi << 32 |
> + (unsigned long)drvdata->edpcsr;
> +
> + pc = (unsigned long)drvdata->edpcsr;
> +
> + if (drvdata->pc_has_offset) {
> + 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;
> +}
> +
> +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)) {
> + 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;
> + }
> +
> + pc = debug_adjust_pc(drvdata);
> + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
> +
> + if (drvdata->edcidsr_present)
> + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
> +
> + if (drvdata->edvidsr_present)
> + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits 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 ? 64 : 32,
> + drvdata->edvidsr & (u32)EDVIDSR_VMID);
> +}
> +
> +static void debug_init_arch_data(void *info)
> +{
> + struct debug_drvdata *drvdata = info;
> + u32 mode, pcsr_offset;
> + u32 eddevid, eddevid1;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Read device info */
> + eddevid = readl_relaxed(drvdata->base + EDDEVID);
> + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> +
> + CS_LOCK(drvdata->base);
> +
> + /* Parse implementation feature */
> + mode = eddevid & EDDEVID_PCSAMPLE_MODE;
> + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> +
> + drvdata->edpcsr_present = false;
> + drvdata->edcidsr_present = false;
> + drvdata->edvidsr_present = false;
> + drvdata->pc_has_offset = false;
> +
> + switch (mode) {
> + case EDDEVID_IMPL_FULL:
> + drvdata->edvidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR_EDCIDSR:
> + drvdata->edcidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR:
> + /*
> + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
> + * define if has the offset for PC sampling value; if read
> + * back EDDEVID1.PCSROffset == 0x2, then this means the debug
> + * module does not sample the instruction set state when
> + * armv8 CPU in AArch32 state.
> + */
> + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
> + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
> +
> + drvdata->pc_has_offset =
> + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * Dump out information on panic.
> + */
> +static int debug_notifier_call(struct notifier_block *self,
> + unsigned long v, void *p)
> +{
> + int cpu;
> + struct debug_drvdata *drvdata;
> +
> + pr_emerg("ARM external debug module:\n");
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pr_emerg("CPU[%d]:\n", drvdata->cpu);
> +
> + debug_read_regs(drvdata);
> + debug_dump_regs(drvdata);
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block debug_notifier = {
> + .notifier_call = debug_notifier_call,
> +};
> +
> +static int debug_enable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_get_sync(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_disable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_put(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static ssize_t debug_func_knob_write(struct file *f,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + u8 val;
> + int ret;
> +
> + ret = kstrtou8_from_user(buf, count, 2, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&debug_lock);
> +
> + if (val == debug_enable)
> + goto out;
> +
> + if (val)
> + ret = debug_enable_func();
> + else
> + ret = debug_disable_func();
I don't think you need to install the handler every time the functionality is
switched on/off. I suggest to install the handler at boot time (or module
insertion time) and in the notifier handler, check the debug_enable flag before
moving on with the output.
> +
> + if (ret) {
> + pr_err("%s: unable to %s debug function: %d\n",
> + __func__, val ? "enable" : "disable", ret);
> + goto err;
> + }
> +
> + debug_enable = val;
Using a true/false value is probably better here. That way you don't end up
with miscellaneous values in debugFS.
> +out:
> + ret = count;
> +err:
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static ssize_t debug_func_knob_read(struct file *f,
> + char __user *ubuf, size_t count, loff_t *ppos)
> +{
> + ssize_t ret;
> + char buf[2];
> +
> + mutex_lock(&debug_lock);
> +
> + buf[0] = '0' + debug_enable;
> + buf[1] = '\n';
snprintf()
> + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
> +
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static const struct file_operations debug_func_knob_fops = {
> + .open = simple_open,
> + .read = debug_func_knob_read,
> + .write = debug_func_knob_write,
> +};
> +
> +static int debug_func_init(void)
> +{
> + struct dentry *file;
> + int ret;
> +
> + /* Create debugfs node */
> + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> + if (!debug_debugfs_dir) {
> + pr_err("%s: unable to create debugfs directory\n", __func__);
> + return -ENOMEM;
> + }
> +
> + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> + debug_debugfs_dir, NULL, &debug_func_knob_fops);
Please align this properly.
> + if (!file) {
> + pr_err("%s: unable to create enable knob file\n", __func__);
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* Use sysfs node to enable functionality */
> + if (!debug_enable)
> + return 0;
> +
> + /* Register function to be called for panic */
> + ret = atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> + if (ret) {
> + pr_err("%s: unable to register notifier: %d\n",
> + __func__, ret);
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + debugfs_remove_recursive(debug_debugfs_dir);
> + return ret;
> +}
> +
> +static void debug_func_exit(void)
> +{
> + debugfs_remove_recursive(debug_debugfs_dir);
> +
> + /* Unregister panic notifier callback */
> + if (debug_enable)
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +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;
> + int ret;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> + if (per_cpu(debug_drvdata, drvdata->cpu)) {
> + dev_err(dev, "CPU%d drvdata has been initialized\n",
> + drvdata->cpu);
> + return -EBUSY;
> + }
> +
> + drvdata->dev = &adev->dev;
> + amba_set_drvdata(adev, 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;
> + ret = smp_call_function_single(drvdata->cpu,
> + debug_init_arch_data, drvdata, 1);
> + put_online_cpus();
> +
> + if (ret) {
> + dev_err(dev, "CPU%d debug arch init failed\n", drvdata->cpu);
> + goto err;
> + }
> +
> + if (!drvdata->edpcsr_present) {
> + dev_err(dev, "CPU%d sample-based profiling isn't implemented\n",
> + drvdata->cpu);
> + ret = -ENXIO;
> + goto err;
> + }
> +
> + if (!debug_count++) {
> + ret = debug_func_init();
> + if (ret)
> + goto err_func_init;
> + }
> +
> + if (!debug_enable)
> + pm_runtime_put(dev);
> +
> + dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu);
> + return 0;
> +
> +err_func_init:
> + debug_count--;
> +err:
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> + return ret;
> +}
> +
> +static int debug_remove(struct amba_device *adev)
> +{
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata = amba_get_drvdata(adev);
> +
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> +
> + if (debug_enable)
> + pm_runtime_put(dev);
> +
> + if (!--debug_count)
> + debug_func_exit();
> +
> + return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> + { /* Debug for Cortex-A53 */
> + .id = 0x000bbd03,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A57 */
> + .id = 0x000bbd07,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A72 */
> + .id = 0x000bbd08,
> + .mask = 0x000fffff,
> + },
> + { 0, 0 },
> +};
> +
> +static struct amba_driver debug_driver = {
> + .drv = {
> + .name = "coresight-cpu-debug",
> + .suppress_bind_attrs = true,
> + },
> + .probe = debug_probe,
> + .remove = debug_remove,
> + .id_table = debug_ids,
> +};
> +
> +module_amba_driver(debug_driver);
> +
> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
> +MODULE_DESCRIPTION("ARM Coresight CPU Debug Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-18 17:40 ` Mathieu Poirier
@ 2017-04-19 0:18 ` Leo Yan
2017-04-19 14:52 ` Mathieu Poirier
0 siblings, 1 reply; 23+ messages in thread
From: Leo Yan @ 2017-04-19 0:18 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On Tue, Apr 18, 2017 at 11:40:50AM -0600, Mathieu Poirier wrote:
> On Thu, Apr 06, 2017 at 09:30:59PM +0800, 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 triggers
> > 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
> > this requires the CPU power domain stays on when access related debug
> > registers, but the power management for CPU power domain is quite
> > dependent on SoC integration for power management. For the platforms
> > which with sane power controller implementations, this driver follows
> > the method to set EDPRCR to try to pull the CPU out of low power state
> > and then set 'no power down request' bit so the CPU has no chance to
> > lose power.
> >
> > If the SoC has not followed up this design well for power management
> > controller, the user should use the command line parameter or sysfs
> > to constrain all or partial idle states to ensure the CPU power
> > domain is enabled and access coresight CPU debug component safely.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>
> This is coming along well - a few comment below. In your next revision please
> add GKH to the 'To' list.
Sure. Will send out in this week and add Greg in 'To' list.
> > ---
> > drivers/hwtracing/coresight/Kconfig | 14 +
> > drivers/hwtracing/coresight/Makefile | 1 +
> > drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
> > 3 files changed, 682 insertions(+)
> > create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
> >
> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> > index 130cb21..8d55d6d 100644
> > --- a/drivers/hwtracing/coresight/Kconfig
> > +++ b/drivers/hwtracing/coresight/Kconfig
> > @@ -89,4 +89,18 @@ config CORESIGHT_STM
> > logging useful software events or data coming from various entities
> > in the system, possibly running different OSs
> >
> > +config CORESIGHT_CPU_DEBUG
> > + tristate "CoreSight CPU Debug driver"
> > + depends on ARM || ARM64
> > + depends on DEBUG_FS
> > + help
> > + This driver provides support for coresight debugging module. This
> > + is primarily used to dump sample-based profiling registers when
> > + system triggers panic, the driver will parse context registers so
> > + can quickly get to know program counter (PC), secure state,
> > + exception level, etc. Before use debugging functionality, platform
> > + needs to ensure the clock domain and power domain are enabled
> > + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> > + for detailed description and the example for usage.
> > +
> > endif
> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> > index af480d9..433d590 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_CPU_DEBUG) += coresight-cpu-debug.o
> > diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> > new file mode 100644
> > index 0000000..8470e31
> > --- /dev/null
> > +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> > @@ -0,0 +1,667 @@
> > +/*
> > + * 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/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/pm_qos.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 EDPRCR 0x310
> > +#define EDPRSR 0x314
> > +#define EDDEVID1 0xFC4
> > +#define EDDEVID 0xFC8
> > +
> > +#define EDPCSR_PROHIBITED 0xFFFFFFFF
> > +
> > +/* bits definition for EDPCSR */
> > +#define EDPCSR_THUMB BIT(0)
> > +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
> > +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
> > +
> > +/* bits definition for EDPRCR */
> > +#define EDPRCR_COREPURQ BIT(3)
> > +#define EDPRCR_CORENPDRQ BIT(0)
> > +
> > +/* 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:PSCROffset
> > + *
> > + * NOTE: armv8 and armv7 have different definition for the register,
> > + * so consolidate the bits definition as below:
> > + *
> > + * 0b0000 - Sample offset applies based on the instruction state, we
> > + * rely on EDDEVID to check if EDPCSR is implemented or not
> > + * 0b0001 - No offset applies.
> > + * 0b0010 - No offset applies, but do not use in AArch32 mode
> > + *
> > + */
> > +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
> > +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
> > +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
> > +
> > +/* bits definition for EDDEVID */
> > +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
> > +#define EDDEVID_IMPL_EDPCSR (0x1)
> > +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
> > +#define EDDEVID_IMPL_FULL (0x3)
> > +
> > +#define DEBUG_WAIT_SLEEP 1000
> > +#define DEBUG_WAIT_TIMEOUT 32000
> > +
> > +struct debug_drvdata {
> > + void __iomem *base;
> > e struct device *dev;
> > + int cpu;
> > +
> > + bool edpcsr_present;
> > + bool edcidsr_present;
> > + bool edvidsr_present;
> > + bool pc_has_offset;
> > +
> > + u32 edpcsr;
> > + u32 edpcsr_hi;
> > + u32 edprsr;
> > + u32 edvidsr;
> > + u32 edcidsr;
> > +};
> > +
> > +static DEFINE_MUTEX(debug_lock);
> > +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
> > +static int debug_count;
> > +static struct dentry *debug_debugfs_dir;
> > +
> > +static bool debug_enable;
> > +module_param_named(enable, debug_enable, bool, 0600);
> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> > + "(default is 0, which means is disabled by default)");
>
> For this driver we have a debugFS interface so I question the validity of a
> kernel module parameter. Other than adding complexity to the code it offers no
> real added value. If a user is to insmod a module, it is just as easy to switch
> on the functionality using debugFS in a second step.
This module parameter can be used for kernel command line, so
it's useful when user wants to dynamically turn on/off the
functionality at boot up time.
Does this make sense for you? Removing this parameter is okay for
me, but this means users need to decide if use it by Kernel config
with static building in. This is a bit contradictory with before's
discussion.
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > + /* Unlocks the debug registers */
> > + writel_relaxed(0x0, drvdata->base + EDOSLAR);
>
> Here the wmb is to make sure reordering (either at compile time or in the CPU)
> doesn't happend. You need a comment here otherwise checkpatch will complain.
> Speaking of which, did you run this through checkpatch?
Right. Everytime I run checkpatch before sending patch and it has
complain for this. Will add comment. Sorry for imprecise working.
> > + 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_force_cpu_powered_up(struct debug_drvdata *drvdata)
> > +{
> > + bool retried = false;
> > + u32 edprcr;
> > +
> > +try_again:
> > +
> > + /*
> > + * Send request to power management controller and assert
> > + * DBGPWRUPREQ signal; if power management controller has
> > + * sane implementation, it should enable CPU power domain
> > + * in case CPU is in low power state.
> > + */
> > + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> > + edprcr |= EDPRCR_COREPURQ;
> > + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> > +
> > + /* Wait for CPU to be powered up (timeout~=32ms) */
> > + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
> > + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
> > + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
> > + /*
> > + * Unfortunately the CPU cannot be powered up, so return
> > + * back and later has no permission to access other
> > + * registers. For this case, should disable CPU low power
> > + * states to ensure CPU power domain is enabled!
> > + */
> > + pr_err("%s: power up request for CPU%d failed\n",
> > + __func__, drvdata->cpu);
> > + return;
> > + }
> > +
> > + /*
> > + * At this point the CPU is powered up, so set the no powerdown
> > + * request bit so we don't lose power and emulate power down.
> > + */
> > + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> > + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> > + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> > +
> > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> > +
> > + /* Bail out if CPU is powered up */
> > + if (likely(drvdata->edprsr & EDPRSR_PU))
> > + return;
>
> /* The core power domain got switched off on use, try again */
> if (unlikely(!drvdata->edprsr & EDPRSR_PU))
> goto try_again;
>
> I understand you don't want to introduce a infinite loop but if that happens
> here, something else has gone very wrong. The above readx_poll_timeout_atomic
> loop should take care of bailing out in case of problems. That way you also get
> to rid of the retried variable and the code is more simple.
Okay. I will follow your method. Thinking the duration of CPU power
on/off is not same magnitude with this piece code, the infinite loop
will not happen.
> > +
> > + /*
> > + * Handle race condition if CPU has been waken up but it sleeps
> > + * again if EDPRCR_CORENPDRQ has been flipped, so try to run
> > + * waken flow one more time.
> > + */
> > + if (!retried) {
> > + retried = true;
> > + goto try_again;
> > + }
> > +}
> > +
> > +static void debug_read_regs(struct debug_drvdata *drvdata)
> > +{
> > + u32 save_edprcr;
> > +
> > + CS_UNLOCK(drvdata->base);
> > +
> > + /* Unlock os lock */
> > + debug_os_unlock(drvdata);
> > +
> > + /* Save EDPRCR register */
> > + save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
> > +
> > + /*
> > + * Ensure CPU power domain is enabled to let registers
> > + * are accessiable.
> > + */
> > + debug_force_cpu_powered_up(drvdata);
> > +
> > + if (!debug_access_permitted(drvdata))
> > + goto out;
> > +
> > + 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)
> > + goto out;
> > +
> > + /*
> > + * 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.
> > + */
> > + if (IS_ENABLED(CONFIG_64BIT))
> > + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +
> > + if (drvdata->edcidsr_present)
> > + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
> > +
> > + if (drvdata->edvidsr_present)
> > + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
> > +
> > +out:
> > + /* Restore EDPRCR register */
> > + writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
> > +
> > + CS_LOCK(drvdata->base);
> > +}
> > +
> > +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
> > +{
> > + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
> > + unsigned long pc;
> > +
> > + if (IS_ENABLED(CONFIG_64BIT))
> > + return (unsigned long)drvdata->edpcsr_hi << 32 |
> > + (unsigned long)drvdata->edpcsr;
> > +
> > + pc = (unsigned long)drvdata->edpcsr;
> > +
> > + if (drvdata->pc_has_offset) {
> > + 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;
> > +}
> > +
> > +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)) {
> > + 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;
> > + }
> > +
> > + pc = debug_adjust_pc(drvdata);
> > + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
> > +
> > + if (drvdata->edcidsr_present)
> > + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
> > +
> > + if (drvdata->edvidsr_present)
> > + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits 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 ? 64 : 32,
> > + drvdata->edvidsr & (u32)EDVIDSR_VMID);
> > +}
> > +
> > +static void debug_init_arch_data(void *info)
> > +{
> > + struct debug_drvdata *drvdata = info;
> > + u32 mode, pcsr_offset;
> > + u32 eddevid, eddevid1;
> > +
> > + CS_UNLOCK(drvdata->base);
> > +
> > + /* Read device info */
> > + eddevid = readl_relaxed(drvdata->base + EDDEVID);
> > + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> > +
> > + CS_LOCK(drvdata->base);
> > +
> > + /* Parse implementation feature */
> > + mode = eddevid & EDDEVID_PCSAMPLE_MODE;
> > + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> > +
> > + drvdata->edpcsr_present = false;
> > + drvdata->edcidsr_present = false;
> > + drvdata->edvidsr_present = false;
> > + drvdata->pc_has_offset = false;
> > +
> > + switch (mode) {
> > + case EDDEVID_IMPL_FULL:
> > + drvdata->edvidsr_present = true;
> > + /* Fall through */
> > + case EDDEVID_IMPL_EDPCSR_EDCIDSR:
> > + drvdata->edcidsr_present = true;
> > + /* Fall through */
> > + case EDDEVID_IMPL_EDPCSR:
> > + /*
> > + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
> > + * define if has the offset for PC sampling value; if read
> > + * back EDDEVID1.PCSROffset == 0x2, then this means the debug
> > + * module does not sample the instruction set state when
> > + * armv8 CPU in AArch32 state.
> > + */
> > + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
> > + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
> > +
> > + drvdata->pc_has_offset =
> > + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
> > + break;
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +/*
> > + * Dump out information on panic.
> > + */
> > +static int debug_notifier_call(struct notifier_block *self,
> > + unsigned long v, void *p)
> > +{
> > + int cpu;
> > + struct debug_drvdata *drvdata;
> > +
> > + pr_emerg("ARM external debug module:\n");
> > +
> > + for_each_possible_cpu(cpu) {
> > + drvdata = per_cpu(debug_drvdata, cpu);
> > + if (!drvdata)
> > + continue;
> > +
> > + pr_emerg("CPU[%d]:\n", drvdata->cpu);
> > +
> > + debug_read_regs(drvdata);
> > + debug_dump_regs(drvdata);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct notifier_block debug_notifier = {
> > + .notifier_call = debug_notifier_call,
> > +};
> > +
> > +static int debug_enable_func(void)
> > +{
> > + struct debug_drvdata *drvdata;
> > + int cpu;
> > +
> > + for_each_possible_cpu(cpu) {
> > + drvdata = per_cpu(debug_drvdata, cpu);
> > + if (!drvdata)
> > + continue;
> > +
> > + pm_runtime_get_sync(drvdata->dev);
> > + }
> > +
> > + return atomic_notifier_chain_register(&panic_notifier_list,
> > + &debug_notifier);
> > +}
> > +
> > +static int debug_disable_func(void)
> > +{
> > + struct debug_drvdata *drvdata;
> > + int cpu;
> > +
> > + for_each_possible_cpu(cpu) {
> > + drvdata = per_cpu(debug_drvdata, cpu);
> > + if (!drvdata)
> > + continue;
> > +
> > + pm_runtime_put(drvdata->dev);
> > + }
> > +
> > + return atomic_notifier_chain_unregister(&panic_notifier_list,
> > + &debug_notifier);
> > +}
> > +
> > +static ssize_t debug_func_knob_write(struct file *f,
> > + const char __user *buf, size_t count, loff_t *ppos)
> > +{
> > + u8 val;
> > + int ret;
> > +
> > + ret = kstrtou8_from_user(buf, count, 2, &val);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&debug_lock);
> > +
> > + if (val == debug_enable)
> > + goto out;
> > +
> > + if (val)
> > + ret = debug_enable_func();
> > + else
> > + ret = debug_disable_func();
>
> I don't think you need to install the handler every time the functionality is
> switched on/off. I suggest to install the handler at boot time (or module
> insertion time) and in the notifier handler, check the debug_enable flag before
> moving on with the output.
Will do.
> > +
> > + if (ret) {
> > + pr_err("%s: unable to %s debug function: %d\n",
> > + __func__, val ? "enable" : "disable", ret);
> > + goto err;
> > + }
> > +
> > + debug_enable = val;
>
> Using a true/false value is probably better here. That way you don't end up
> with miscellaneous values in debugFS.
>From my testing here will not assign other values rather than 0 or 1.
This is controlled by function kstrtou8_from_user(), the 3rd parameter
'2' is to define the value range [0..1], so other values will report
error after return back from kstrtou8_from_user().
> > +out:
> > + ret = count;
> > +err:
> > + mutex_unlock(&debug_lock);
> > + return ret;
> > +}
> > +
> > +static ssize_t debug_func_knob_read(struct file *f,
> > + char __user *ubuf, size_t count, loff_t *ppos)
> > +{
> > + ssize_t ret;
> > + char buf[2];
> > +
> > + mutex_lock(&debug_lock);
> > +
> > + buf[0] = '0' + debug_enable;
> > + buf[1] = '\n';
>
> snprintf()
Will fix.
> > + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
> > +
> > + mutex_unlock(&debug_lock);
> > + return ret;
> > +}
> > +
> > +static const struct file_operations debug_func_knob_fops = {
> > + .open = simple_open,
> > + .read = debug_func_knob_read,
> > + .write = debug_func_knob_write,
> > +};
> > +
> > +static int debug_func_init(void)
> > +{
> > + struct dentry *file;
> > + int ret;
> > +
> > + /* Create debugfs node */
> > + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> > + if (!debug_debugfs_dir) {
> > + pr_err("%s: unable to create debugfs directory\n", __func__);
> > + return -ENOMEM;
> > + }
> > +
> > + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> > + debug_debugfs_dir, NULL, &debug_func_knob_fops);
>
> Please align this properly.
Will fix. Thanks a lot for detailed reviewing.
[...]
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-19 0:18 ` Leo Yan
@ 2017-04-19 14:52 ` Mathieu Poirier
2017-04-19 15:30 ` Leo Yan
0 siblings, 1 reply; 23+ messages in thread
From: Mathieu Poirier @ 2017-04-19 14:52 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On 18 April 2017 at 18:18, Leo Yan <leo.yan@linaro.org> wrote:
> On Tue, Apr 18, 2017 at 11:40:50AM -0600, Mathieu Poirier wrote:
>> On Thu, Apr 06, 2017 at 09:30:59PM +0800, 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 triggers
>> > 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
>> > this requires the CPU power domain stays on when access related debug
>> > registers, but the power management for CPU power domain is quite
>> > dependent on SoC integration for power management. For the platforms
>> > which with sane power controller implementations, this driver follows
>> > the method to set EDPRCR to try to pull the CPU out of low power state
>> > and then set 'no power down request' bit so the CPU has no chance to
>> > lose power.
>> >
>> > If the SoC has not followed up this design well for power management
>> > controller, the user should use the command line parameter or sysfs
>> > to constrain all or partial idle states to ensure the CPU power
>> > domain is enabled and access coresight CPU debug component safely.
>> >
>> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>
>> This is coming along well - a few comment below. In your next revision please
>> add GKH to the 'To' list.
>
> Sure. Will send out in this week and add Greg in 'To' list.
>
>> > ---
>> > drivers/hwtracing/coresight/Kconfig | 14 +
>> > drivers/hwtracing/coresight/Makefile | 1 +
>> > drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
>> > 3 files changed, 682 insertions(+)
>> > create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>> >
>> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> > index 130cb21..8d55d6d 100644
>> > --- a/drivers/hwtracing/coresight/Kconfig
>> > +++ b/drivers/hwtracing/coresight/Kconfig
>> > @@ -89,4 +89,18 @@ config CORESIGHT_STM
>> > logging useful software events or data coming from various entities
>> > in the system, possibly running different OSs
>> >
>> > +config CORESIGHT_CPU_DEBUG
>> > + tristate "CoreSight CPU Debug driver"
>> > + depends on ARM || ARM64
>> > + depends on DEBUG_FS
>> > + help
>> > + This driver provides support for coresight debugging module. This
>> > + is primarily used to dump sample-based profiling registers when
>> > + system triggers panic, the driver will parse context registers so
>> > + can quickly get to know program counter (PC), secure state,
>> > + exception level, etc. Before use debugging functionality, platform
>> > + needs to ensure the clock domain and power domain are enabled
>> > + properly, please refer Documentation/trace/coresight-cpu-debug.txt
>> > + for detailed description and the example for usage.
>> > +
>> > endif
>> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> > index af480d9..433d590 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_CPU_DEBUG) += coresight-cpu-debug.o
>> > diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> > new file mode 100644
>> > index 0000000..8470e31
>> > --- /dev/null
>> > +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> > @@ -0,0 +1,667 @@
>> > +/*
>> > + * 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/debugfs.h>
>> > +#include <linux/delay.h>
>> > +#include <linux/device.h>
>> > +#include <linux/err.h>
>> > +#include <linux/init.h>
>> > +#include <linux/io.h>
>> > +#include <linux/iopoll.h>
>> > +#include <linux/kernel.h>
>> > +#include <linux/module.h>
>> > +#include <linux/moduleparam.h>
>> > +#include <linux/pm_qos.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 EDPRCR 0x310
>> > +#define EDPRSR 0x314
>> > +#define EDDEVID1 0xFC4
>> > +#define EDDEVID 0xFC8
>> > +
>> > +#define EDPCSR_PROHIBITED 0xFFFFFFFF
>> > +
>> > +/* bits definition for EDPCSR */
>> > +#define EDPCSR_THUMB BIT(0)
>> > +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
>> > +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
>> > +
>> > +/* bits definition for EDPRCR */
>> > +#define EDPRCR_COREPURQ BIT(3)
>> > +#define EDPRCR_CORENPDRQ BIT(0)
>> > +
>> > +/* 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:PSCROffset
>> > + *
>> > + * NOTE: armv8 and armv7 have different definition for the register,
>> > + * so consolidate the bits definition as below:
>> > + *
>> > + * 0b0000 - Sample offset applies based on the instruction state, we
>> > + * rely on EDDEVID to check if EDPCSR is implemented or not
>> > + * 0b0001 - No offset applies.
>> > + * 0b0010 - No offset applies, but do not use in AArch32 mode
>> > + *
>> > + */
>> > +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
>> > +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
>> > +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
>> > +
>> > +/* bits definition for EDDEVID */
>> > +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
>> > +#define EDDEVID_IMPL_EDPCSR (0x1)
>> > +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
>> > +#define EDDEVID_IMPL_FULL (0x3)
>> > +
>> > +#define DEBUG_WAIT_SLEEP 1000
>> > +#define DEBUG_WAIT_TIMEOUT 32000
>> > +
>> > +struct debug_drvdata {
>> > + void __iomem *base;
>> > e struct device *dev;
>> > + int cpu;
>> > +
>> > + bool edpcsr_present;
>> > + bool edcidsr_present;
>> > + bool edvidsr_present;
>> > + bool pc_has_offset;
>> > +
>> > + u32 edpcsr;
>> > + u32 edpcsr_hi;
>> > + u32 edprsr;
>> > + u32 edvidsr;
>> > + u32 edcidsr;
>> > +};
>> > +
>> > +static DEFINE_MUTEX(debug_lock);
>> > +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
>> > +static int debug_count;
>> > +static struct dentry *debug_debugfs_dir;
>> > +
>> > +static bool debug_enable;
>> > +module_param_named(enable, debug_enable, bool, 0600);
>> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
>> > + "(default is 0, which means is disabled by default)");
>>
>> For this driver we have a debugFS interface so I question the validity of a
>> kernel module parameter. Other than adding complexity to the code it offers no
>> real added value. If a user is to insmod a module, it is just as easy to switch
>> on the functionality using debugFS in a second step.
>
> This module parameter can be used for kernel command line, so
> it's useful when user wants to dynamically turn on/off the
> functionality at boot up time.
>
> Does this make sense for you? Removing this parameter is okay for
> me, but this means users need to decide if use it by Kernel config
> with static building in. This is a bit contradictory with before's
> discussion.
My hope was to use the kernel command line and the debugFS interface,
avoiding the module parameter. Look at what the baycom_par and
blacklist drivers are doing with the "__setup()" function and see if
we can void a module parameter. If not then let it be, unless someone
else has a better idea.
[1]. drivers/net/hamradio/baycom_par.c
[2]. drivers/se90/cio/blacklist.c
>
>> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
>> > +{
>> > + /* Unlocks the debug registers */
>> > + writel_relaxed(0x0, drvdata->base + EDOSLAR);
>>
>> Here the wmb is to make sure reordering (either at compile time or in the CPU)
>> doesn't happend. You need a comment here otherwise checkpatch will complain.
>> Speaking of which, did you run this through checkpatch?
>
> Right. Everytime I run checkpatch before sending patch and it has
> complain for this. Will add comment. Sorry for imprecise working.
>
>> > + 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_force_cpu_powered_up(struct debug_drvdata *drvdata)
>> > +{
>> > + bool retried = false;
>> > + u32 edprcr;
>> > +
>> > +try_again:
>> > +
>> > + /*
>> > + * Send request to power management controller and assert
>> > + * DBGPWRUPREQ signal; if power management controller has
>> > + * sane implementation, it should enable CPU power domain
>> > + * in case CPU is in low power state.
>> > + */
>> > + edprcr = readl_relaxed(drvdata->base + EDPRCR);
>> > + edprcr |= EDPRCR_COREPURQ;
>> > + writel_relaxed(edprcr, drvdata->base + EDPRCR);
>> > +
>> > + /* Wait for CPU to be powered up (timeout~=32ms) */
>> > + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
>> > + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
>> > + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
>> > + /*
>> > + * Unfortunately the CPU cannot be powered up, so return
>> > + * back and later has no permission to access other
>> > + * registers. For this case, should disable CPU low power
>> > + * states to ensure CPU power domain is enabled!
>> > + */
>> > + pr_err("%s: power up request for CPU%d failed\n",
>> > + __func__, drvdata->cpu);
>> > + return;
>> > + }
>> > +
>> > + /*
>> > + * At this point the CPU is powered up, so set the no powerdown
>> > + * request bit so we don't lose power and emulate power down.
>> > + */
>> > + edprcr = readl_relaxed(drvdata->base + EDPRCR);
>> > + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
>> > + writel_relaxed(edprcr, drvdata->base + EDPRCR);
>> > +
>> > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
>> > +
>> > + /* Bail out if CPU is powered up */
>> > + if (likely(drvdata->edprsr & EDPRSR_PU))
>> > + return;
>>
>> /* The core power domain got switched off on use, try again */
>> if (unlikely(!drvdata->edprsr & EDPRSR_PU))
>> goto try_again;
>>
>> I understand you don't want to introduce a infinite loop but if that happens
>> here, something else has gone very wrong. The above readx_poll_timeout_atomic
>> loop should take care of bailing out in case of problems. That way you also get
>> to rid of the retried variable and the code is more simple.
>
> Okay. I will follow your method. Thinking the duration of CPU power
> on/off is not same magnitude with this piece code, the infinite loop
> will not happen.
>
>> > +
>> > + /*
>> > + * Handle race condition if CPU has been waken up but it sleeps
>> > + * again if EDPRCR_CORENPDRQ has been flipped, so try to run
>> > + * waken flow one more time.
>> > + */
>> > + if (!retried) {
>> > + retried = true;
>> > + goto try_again;
>> > + }
>> > +}
>> > +
>> > +static void debug_read_regs(struct debug_drvdata *drvdata)
>> > +{
>> > + u32 save_edprcr;
>> > +
>> > + CS_UNLOCK(drvdata->base);
>> > +
>> > + /* Unlock os lock */
>> > + debug_os_unlock(drvdata);
>> > +
>> > + /* Save EDPRCR register */
>> > + save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
>> > +
>> > + /*
>> > + * Ensure CPU power domain is enabled to let registers
>> > + * are accessiable.
>> > + */
>> > + debug_force_cpu_powered_up(drvdata);
>> > +
>> > + if (!debug_access_permitted(drvdata))
>> > + goto out;
>> > +
>> > + 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)
>> > + goto out;
>> > +
>> > + /*
>> > + * 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.
>> > + */
>> > + if (IS_ENABLED(CONFIG_64BIT))
>> > + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
>> > +
>> > + if (drvdata->edcidsr_present)
>> > + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
>> > +
>> > + if (drvdata->edvidsr_present)
>> > + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
>> > +
>> > +out:
>> > + /* Restore EDPRCR register */
>> > + writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
>> > +
>> > + CS_LOCK(drvdata->base);
>> > +}
>> > +
>> > +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
>> > +{
>> > + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
>> > + unsigned long pc;
>> > +
>> > + if (IS_ENABLED(CONFIG_64BIT))
>> > + return (unsigned long)drvdata->edpcsr_hi << 32 |
>> > + (unsigned long)drvdata->edpcsr;
>> > +
>> > + pc = (unsigned long)drvdata->edpcsr;
>> > +
>> > + if (drvdata->pc_has_offset) {
>> > + 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;
>> > +}
>> > +
>> > +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)) {
>> > + 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;
>> > + }
>> > +
>> > + pc = debug_adjust_pc(drvdata);
>> > + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
>> > +
>> > + if (drvdata->edcidsr_present)
>> > + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
>> > +
>> > + if (drvdata->edvidsr_present)
>> > + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits 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 ? 64 : 32,
>> > + drvdata->edvidsr & (u32)EDVIDSR_VMID);
>> > +}
>> > +
>> > +static void debug_init_arch_data(void *info)
>> > +{
>> > + struct debug_drvdata *drvdata = info;
>> > + u32 mode, pcsr_offset;
>> > + u32 eddevid, eddevid1;
>> > +
>> > + CS_UNLOCK(drvdata->base);
>> > +
>> > + /* Read device info */
>> > + eddevid = readl_relaxed(drvdata->base + EDDEVID);
>> > + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
>> > +
>> > + CS_LOCK(drvdata->base);
>> > +
>> > + /* Parse implementation feature */
>> > + mode = eddevid & EDDEVID_PCSAMPLE_MODE;
>> > + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
>> > +
>> > + drvdata->edpcsr_present = false;
>> > + drvdata->edcidsr_present = false;
>> > + drvdata->edvidsr_present = false;
>> > + drvdata->pc_has_offset = false;
>> > +
>> > + switch (mode) {
>> > + case EDDEVID_IMPL_FULL:
>> > + drvdata->edvidsr_present = true;
>> > + /* Fall through */
>> > + case EDDEVID_IMPL_EDPCSR_EDCIDSR:
>> > + drvdata->edcidsr_present = true;
>> > + /* Fall through */
>> > + case EDDEVID_IMPL_EDPCSR:
>> > + /*
>> > + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
>> > + * define if has the offset for PC sampling value; if read
>> > + * back EDDEVID1.PCSROffset == 0x2, then this means the debug
>> > + * module does not sample the instruction set state when
>> > + * armv8 CPU in AArch32 state.
>> > + */
>> > + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
>> > + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
>> > +
>> > + drvdata->pc_has_offset =
>> > + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
>> > + break;
>> > + default:
>> > + break;
>> > + }
>> > +}
>> > +
>> > +/*
>> > + * Dump out information on panic.
>> > + */
>> > +static int debug_notifier_call(struct notifier_block *self,
>> > + unsigned long v, void *p)
>> > +{
>> > + int cpu;
>> > + struct debug_drvdata *drvdata;
>> > +
>> > + pr_emerg("ARM external debug module:\n");
>> > +
>> > + for_each_possible_cpu(cpu) {
>> > + drvdata = per_cpu(debug_drvdata, cpu);
>> > + if (!drvdata)
>> > + continue;
>> > +
>> > + pr_emerg("CPU[%d]:\n", drvdata->cpu);
>> > +
>> > + debug_read_regs(drvdata);
>> > + debug_dump_regs(drvdata);
>> > + }
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static struct notifier_block debug_notifier = {
>> > + .notifier_call = debug_notifier_call,
>> > +};
>> > +
>> > +static int debug_enable_func(void)
>> > +{
>> > + struct debug_drvdata *drvdata;
>> > + int cpu;
>> > +
>> > + for_each_possible_cpu(cpu) {
>> > + drvdata = per_cpu(debug_drvdata, cpu);
>> > + if (!drvdata)
>> > + continue;
>> > +
>> > + pm_runtime_get_sync(drvdata->dev);
>> > + }
>> > +
>> > + return atomic_notifier_chain_register(&panic_notifier_list,
>> > + &debug_notifier);
>> > +}
>> > +
>> > +static int debug_disable_func(void)
>> > +{
>> > + struct debug_drvdata *drvdata;
>> > + int cpu;
>> > +
>> > + for_each_possible_cpu(cpu) {
>> > + drvdata = per_cpu(debug_drvdata, cpu);
>> > + if (!drvdata)
>> > + continue;
>> > +
>> > + pm_runtime_put(drvdata->dev);
>> > + }
>> > +
>> > + return atomic_notifier_chain_unregister(&panic_notifier_list,
>> > + &debug_notifier);
>> > +}
>> > +
>> > +static ssize_t debug_func_knob_write(struct file *f,
>> > + const char __user *buf, size_t count, loff_t *ppos)
>> > +{
>> > + u8 val;
>> > + int ret;
>> > +
>> > + ret = kstrtou8_from_user(buf, count, 2, &val);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + mutex_lock(&debug_lock);
>> > +
>> > + if (val == debug_enable)
>> > + goto out;
>> > +
>> > + if (val)
>> > + ret = debug_enable_func();
>> > + else
>> > + ret = debug_disable_func();
>>
>> I don't think you need to install the handler every time the functionality is
>> switched on/off. I suggest to install the handler at boot time (or module
>> insertion time) and in the notifier handler, check the debug_enable flag before
>> moving on with the output.
>
> Will do.
>
>> > +
>> > + if (ret) {
>> > + pr_err("%s: unable to %s debug function: %d\n",
>> > + __func__, val ? "enable" : "disable", ret);
>> > + goto err;
>> > + }
>> > +
>> > + debug_enable = val;
>>
>> Using a true/false value is probably better here. That way you don't end up
>> with miscellaneous values in debugFS.
>
> From my testing here will not assign other values rather than 0 or 1.
> This is controlled by function kstrtou8_from_user(), the 3rd parameter
> '2' is to define the value range [0..1], so other values will report
> error after return back from kstrtou8_from_user().
Ok that works.
>
>> > +out:
>> > + ret = count;
>> > +err:
>> > + mutex_unlock(&debug_lock);
>> > + return ret;
>> > +}
>> > +
>> > +static ssize_t debug_func_knob_read(struct file *f,
>> > + char __user *ubuf, size_t count, loff_t *ppos)
>> > +{
>> > + ssize_t ret;
>> > + char buf[2];
>> > +
>> > + mutex_lock(&debug_lock);
>> > +
>> > + buf[0] = '0' + debug_enable;
>> > + buf[1] = '\n';
>>
>> snprintf()
>
> Will fix.
>
>> > + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
>> > +
>> > + mutex_unlock(&debug_lock);
>> > + return ret;
>> > +}
>> > +
>> > +static const struct file_operations debug_func_knob_fops = {
>> > + .open = simple_open,
>> > + .read = debug_func_knob_read,
>> > + .write = debug_func_knob_write,
>> > +};
>> > +
>> > +static int debug_func_init(void)
>> > +{
>> > + struct dentry *file;
>> > + int ret;
>> > +
>> > + /* Create debugfs node */
>> > + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
>> > + if (!debug_debugfs_dir) {
>> > + pr_err("%s: unable to create debugfs directory\n", __func__);
>> > + return -ENOMEM;
>> > + }
>> > +
>> > + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
>> > + debug_debugfs_dir, NULL, &debug_func_knob_fops);
>>
>> Please align this properly.
>
> Will fix. Thanks a lot for detailed reviewing.
>
> [...]
>
> Thanks,
> Leo Yan
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-19 14:52 ` Mathieu Poirier
@ 2017-04-19 15:30 ` Leo Yan
2017-04-19 16:50 ` Mathieu Poirier
0 siblings, 1 reply; 23+ messages in thread
From: Leo Yan @ 2017-04-19 15:30 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On Wed, Apr 19, 2017 at 08:52:12AM -0600, Mathieu Poirier wrote:
[...]
> >> > +static bool debug_enable;
> >> > +module_param_named(enable, debug_enable, bool, 0600);
> >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> >> > + "(default is 0, which means is disabled by default)");
> >>
> >> For this driver we have a debugFS interface so I question the validity of a
> >> kernel module parameter. Other than adding complexity to the code it offers no
> >> real added value. If a user is to insmod a module, it is just as easy to switch
> >> on the functionality using debugFS in a second step.
> >
> > This module parameter can be used for kernel command line, so
> > it's useful when user wants to dynamically turn on/off the
> > functionality at boot up time.
> >
> > Does this make sense for you? Removing this parameter is okay for
> > me, but this means users need to decide if use it by Kernel config
> > with static building in. This is a bit contradictory with before's
> > discussion.
>
> My hope was to use the kernel command line and the debugFS interface,
> avoiding the module parameter. Look at what the baycom_par and
> blacklist drivers are doing with the "__setup()" function and see if
> we can void a module parameter. If not then let it be, unless someone
> else has a better idea.
>
> [1]. drivers/net/hamradio/baycom_par.c
> [2]. drivers/s390/cio/blacklist.c
This driver supports module mode. So we can choose to use either module
parameter or __setup(). But as described in the file
Documentation/admin-guide/kernel-parameters.rst, the module parameter
is more flexible to be used at boot time or insmod the module:
"Parameters for modules which are built into the kernel need to be
specified on the kernel command line. modprobe looks through the
kernel command line (/proc/cmdline) and collects module parameters
when it loads a module, so the kernel command line can be used for
loadable modules too."
__setup() cannot support module mode, and when use __setup(), we need
register one callback function for it. The callback function is
friendly to parse complex parameters rather than module parameter.
but it's not necessary for this case.
So I'm bias to use module parameter :)
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-19 15:30 ` Leo Yan
@ 2017-04-19 16:50 ` Mathieu Poirier
0 siblings, 0 replies; 23+ messages in thread
From: Mathieu Poirier @ 2017-04-19 16:50 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On 19 April 2017 at 09:30, Leo Yan <leo.yan@linaro.org> wrote:
> On Wed, Apr 19, 2017 at 08:52:12AM -0600, Mathieu Poirier wrote:
>
> [...]
>
>> >> > +static bool debug_enable;
>> >> > +module_param_named(enable, debug_enable, bool, 0600);
>> >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
>> >> > + "(default is 0, which means is disabled by default)");
>> >>
>> >> For this driver we have a debugFS interface so I question the validity of a
>> >> kernel module parameter. Other than adding complexity to the code it offers no
>> >> real added value. If a user is to insmod a module, it is just as easy to switch
>> >> on the functionality using debugFS in a second step.
>> >
>> > This module parameter can be used for kernel command line, so
>> > it's useful when user wants to dynamically turn on/off the
>> > functionality at boot up time.
>> >
>> > Does this make sense for you? Removing this parameter is okay for
>> > me, but this means users need to decide if use it by Kernel config
>> > with static building in. This is a bit contradictory with before's
>> > discussion.
>>
>> My hope was to use the kernel command line and the debugFS interface,
>> avoiding the module parameter. Look at what the baycom_par and
>> blacklist drivers are doing with the "__setup()" function and see if
>> we can void a module parameter. If not then let it be, unless someone
>> else has a better idea.
>>
>> [1]. drivers/net/hamradio/baycom_par.c
>> [2]. drivers/s390/cio/blacklist.c
>
> This driver supports module mode. So we can choose to use either module
> parameter or __setup(). But as described in the file
> Documentation/admin-guide/kernel-parameters.rst, the module parameter
> is more flexible to be used at boot time or insmod the module:
>
> "Parameters for modules which are built into the kernel need to be
> specified on the kernel command line. modprobe looks through the
> kernel command line (/proc/cmdline) and collects module parameters
> when it loads a module, so the kernel command line can be used for
> loadable modules too."
>
> __setup() cannot support module mode, and when use __setup(), we need
> register one callback function for it. The callback function is
> friendly to parse complex parameters rather than module parameter.
> but it's not necessary for this case.
__setup() definitely supports module - the baycom driver is a good
example of that.
But as you pointed out kernel-parameters.rst is pretty clear on how to
proceed. As such disregard my comment and proceed with a module
parameter.
>
> So I'm bias to use module parameter :)
>
> Thanks,
> Leo Yan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-06 13:30 ` [PATCH v6 6/8] coresight: add support for CPU debug module Leo Yan
2017-04-18 17:40 ` Mathieu Poirier
@ 2017-04-19 13:23 ` Suzuki K Poulose
2017-04-19 14:28 ` Leo Yan
2017-04-19 17:49 ` Mathieu Poirier
2 siblings, 1 reply; 23+ messages in thread
From: Suzuki K Poulose @ 2017-04-19 13:23 UTC (permalink / raw)
To: Leo Yan, Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On 06/04/17 14:30, 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 triggers
> 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
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
>
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.
Hi Leo,
This version looks good to me. I have two minor comments below.
> +
> +static struct notifier_block debug_notifier = {
> + .notifier_call = debug_notifier_call,
> +};
> +
> +static int debug_enable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_get_sync(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_disable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_put(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
I believe you should, reverse the order of these operations in debug_disable_func()
to prevent getting a panic notifier after we have released the power domain for the
debug.
i.e, :
atomic_notifier_chain_unregister(...);
for_each_possible_cpu(cpu) {}
> +
> +static ssize_t debug_func_knob_write(struct file *f,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + u8 val;
> + int ret;
> +
> + ret = kstrtou8_from_user(buf, count, 2, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&debug_lock);
> +
> + if (val == debug_enable)
> + goto out;
> +
> + if (val)
> + ret = debug_enable_func();
> + else
> + ret = debug_disable_func();
> +
> + if (ret) {
> + pr_err("%s: unable to %s debug function: %d\n",
> + __func__, val ? "enable" : "disable", ret);
> + goto err;
> + }
> +
> + debug_enable = val;
> +out:
> + ret = count;
> +err:
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static ssize_t debug_func_knob_read(struct file *f,
> + char __user *ubuf, size_t count, loff_t *ppos)
> +{
> + ssize_t ret;
> + char buf[2];
> +
> + mutex_lock(&debug_lock);
> +
> + buf[0] = '0' + debug_enable;
> + buf[1] = '\n';
> + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
> +
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static const struct file_operations debug_func_knob_fops = {
> + .open = simple_open,
> + .read = debug_func_knob_read,
> + .write = debug_func_knob_write,
> +};
> +
> +static int debug_func_init(void)
> +{
> + struct dentry *file;
> + int ret;
> +
> + /* Create debugfs node */
> + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> + if (!debug_debugfs_dir) {
> + pr_err("%s: unable to create debugfs directory\n", __func__);
> + return -ENOMEM;
> + }
> +
> + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> + debug_debugfs_dir, NULL, &debug_func_knob_fops);
> + if (!file) {
> + pr_err("%s: unable to create enable knob file\n", __func__);
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* Use sysfs node to enable functionality */
> + if (!debug_enable)
> + return 0;
> +
> + /* Register function to be called for panic */
> + ret = atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> + if (ret) {
> + pr_err("%s: unable to register notifier: %d\n",
> + __func__, ret);
> + goto err;
> + }
> +
Since we depend on the value of debug_enable above, below in debug_probe()
and in debug_remove(), we should protect these paths using the debug_lock mutex,
like we do above, to make sure we don't create a race.
> + return 0;
> +
> +err:
> + debugfs_remove_recursive(debug_debugfs_dir);
> + return ret;
> +}
> +
> +static void debug_func_exit(void)
> +{
> + debugfs_remove_recursive(debug_debugfs_dir);
> +
> + /* Unregister panic notifier callback */
> + if (debug_enable)
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +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;
> + int ret;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> + if (per_cpu(debug_drvdata, drvdata->cpu)) {
> + dev_err(dev, "CPU%d drvdata has been initialized\n",
> + drvdata->cpu);
May be we could warn about a possible issue in the DT ?
Cheers
Suzuki
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-19 13:23 ` Suzuki K Poulose
@ 2017-04-19 14:28 ` Leo Yan
2017-04-19 14:32 ` Suzuki K Poulose
0 siblings, 1 reply; 23+ messages in thread
From: Leo Yan @ 2017-04-19 14:28 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
Hi Suzuki,
On Wed, Apr 19, 2017 at 02:23:04PM +0100, Suzuki K Poulose wrote:
> Hi Leo,
>
> This version looks good to me. I have two minor comments below.
Thanks for reviewing. Will take the suggestions. Just check a bit for
last comment.
[...]
> >+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;
> >+ int ret;
> >+
> >+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >+ if (!drvdata)
> >+ return -ENOMEM;
> >+
> >+ drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> >+ if (per_cpu(debug_drvdata, drvdata->cpu)) {
> >+ dev_err(dev, "CPU%d drvdata has been initialized\n",
> >+ drvdata->cpu);
>
> May be we could warn about a possible issue in the DT ?
So can I understand the suggestion is to add warning in function
of_coresight_get_cpu() when cannot find CPU number, and here directly
bail out?
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-19 14:28 ` Leo Yan
@ 2017-04-19 14:32 ` Suzuki K Poulose
2017-04-19 15:07 ` Leo Yan
0 siblings, 1 reply; 23+ messages in thread
From: Suzuki K Poulose @ 2017-04-19 14:32 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On 19/04/17 15:28, Leo Yan wrote:
> Hi Suzuki,
>
> On Wed, Apr 19, 2017 at 02:23:04PM +0100, Suzuki K Poulose wrote:
>> Hi Leo,
>>
>> This version looks good to me. I have two minor comments below.
>
> Thanks for reviewing. Will take the suggestions. Just check a bit for
> last comment.
>
> [...]
>
>>> +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;
>>> + int ret;
>>> +
>>> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>>> + if (!drvdata)
>>> + return -ENOMEM;
>>> +
>>> + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
>>> + if (per_cpu(debug_drvdata, drvdata->cpu)) {
>>> + dev_err(dev, "CPU%d drvdata has been initialized\n",
>>> + drvdata->cpu);
>>
>> May be we could warn about a possible issue in the DT ?
>
> So can I understand the suggestion is to add warning in function
> of_coresight_get_cpu() when cannot find CPU number, and here directly
> bail out?
No. One could have single CPU DT, where he doesn't need to provide the CPU number.
Hence, it doesn't make sense to WARN in of_coresight_get_cpu().
But when we hit the case above, we find that the some node was registered for
the given CPU (be it 0 or any other), which is definitely an error in DT. Due to
1) Hasn't specified the CPU number for more than one node
OR
2) CPU number duplicated in the more than one nodes.
Cheers
Suzuki
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-19 14:32 ` Suzuki K Poulose
@ 2017-04-19 15:07 ` Leo Yan
0 siblings, 0 replies; 23+ messages in thread
From: Leo Yan @ 2017-04-19 15:07 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On Wed, Apr 19, 2017 at 03:32:39PM +0100, Suzuki K Poulose wrote:
[...]
> >>>+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;
> >>>+ int ret;
> >>>+
> >>>+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >>>+ if (!drvdata)
> >>>+ return -ENOMEM;
> >>>+
> >>>+ drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> >>>+ if (per_cpu(debug_drvdata, drvdata->cpu)) {
> >>>+ dev_err(dev, "CPU%d drvdata has been initialized\n",
> >>>+ drvdata->cpu);
> >>
> >>May be we could warn about a possible issue in the DT ?
> >
> >So can I understand the suggestion is to add warning in function
> >of_coresight_get_cpu() when cannot find CPU number, and here directly
> >bail out?
>
> No. One could have single CPU DT, where he doesn't need to provide the CPU number.
> Hence, it doesn't make sense to WARN in of_coresight_get_cpu().
>
> But when we hit the case above, we find that the some node was registered for
> the given CPU (be it 0 or any other), which is definitely an error in DT. Due to
>
> 1) Hasn't specified the CPU number for more than one node
>
> OR
>
> 2) CPU number duplicated in the more than one nodes.
Thanks for explaination. It's clear for me now.
> Cheers
> Suzuki
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
2017-04-06 13:30 ` [PATCH v6 6/8] coresight: add support for CPU debug module Leo Yan
2017-04-18 17:40 ` Mathieu Poirier
2017-04-19 13:23 ` Suzuki K Poulose
@ 2017-04-19 17:49 ` Mathieu Poirier
2 siblings, 0 replies; 23+ messages in thread
From: Mathieu Poirier @ 2017-04-19 17:49 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
On 6 April 2017 at 07:30, Leo Yan <leo.yan@linaro.org> 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 triggers
> 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
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
>
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> drivers/hwtracing/coresight/Kconfig | 14 +
> drivers/hwtracing/coresight/Makefile | 1 +
> drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
> 3 files changed, 682 insertions(+)
> create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..8d55d6d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,18 @@ config CORESIGHT_STM
> logging useful software events or data coming from various entities
> in the system, possibly running different OSs
>
> +config CORESIGHT_CPU_DEBUG
> + tristate "CoreSight CPU Debug driver"
> + depends on ARM || ARM64
> + depends on DEBUG_FS
> + help
> + This driver provides support for coresight debugging module. This
> + is primarily used to dump sample-based profiling registers when
> + system triggers panic, the driver will parse context registers so
> + can quickly get to know program counter (PC), secure state,
> + exception level, etc. Before use debugging functionality, platform
> + needs to ensure the clock domain and power domain are enabled
> + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> + for detailed description and the example for usage.
> +
> endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index af480d9..433d590 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_CPU_DEBUG) += coresight-cpu-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> new file mode 100644
> index 0000000..8470e31
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,667 @@
> +/*
> + * 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/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pm_qos.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 EDPRCR 0x310
> +#define EDPRSR 0x314
> +#define EDDEVID1 0xFC4
> +#define EDDEVID 0xFC8
> +
> +#define EDPCSR_PROHIBITED 0xFFFFFFFF
> +
> +/* bits definition for EDPCSR */
> +#define EDPCSR_THUMB BIT(0)
> +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
> +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
> +
> +/* bits definition for EDPRCR */
> +#define EDPRCR_COREPURQ BIT(3)
> +#define EDPRCR_CORENPDRQ BIT(0)
> +
> +/* 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:PSCROffset
> + *
> + * NOTE: armv8 and armv7 have different definition for the register,
> + * so consolidate the bits definition as below:
> + *
> + * 0b0000 - Sample offset applies based on the instruction state, we
> + * rely on EDDEVID to check if EDPCSR is implemented or not
> + * 0b0001 - No offset applies.
> + * 0b0010 - No offset applies, but do not use in AArch32 mode
> + *
> + */
> +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
> +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
> +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
> +
> +/* bits definition for EDDEVID */
> +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
> +#define EDDEVID_IMPL_EDPCSR (0x1)
> +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
> +#define EDDEVID_IMPL_FULL (0x3)
> +
> +#define DEBUG_WAIT_SLEEP 1000
> +#define DEBUG_WAIT_TIMEOUT 32000
> +
> +struct debug_drvdata {
> + void __iomem *base;
> + struct device *dev;
> + int cpu;
> +
> + bool edpcsr_present;
> + bool edcidsr_present;
> + bool edvidsr_present;
> + bool pc_has_offset;
> +
> + u32 edpcsr;
> + u32 edpcsr_hi;
> + u32 edprsr;
> + u32 edvidsr;
> + u32 edcidsr;
> +};
> +
> +static DEFINE_MUTEX(debug_lock);
> +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
> +static int debug_count;
> +static struct dentry *debug_debugfs_dir;
> +
> +static bool debug_enable;
> +module_param_named(enable, debug_enable, bool, 0600);
> +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> + "(default is 0, which means is disabled by default)");
> +
> +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_force_cpu_powered_up(struct debug_drvdata *drvdata)
> +{
> + bool retried = false;
> + u32 edprcr;
> +
> +try_again:
> +
> + /*
> + * Send request to power management controller and assert
> + * DBGPWRUPREQ signal; if power management controller has
> + * sane implementation, it should enable CPU power domain
> + * in case CPU is in low power state.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + /* Wait for CPU to be powered up (timeout~=32ms) */
> + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
> + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
> + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
> + /*
> + * Unfortunately the CPU cannot be powered up, so return
> + * back and later has no permission to access other
> + * registers. For this case, should disable CPU low power
> + * states to ensure CPU power domain is enabled!
> + */
> + pr_err("%s: power up request for CPU%d failed\n",
> + __func__, drvdata->cpu);
> + return;
> + }
> +
> + /*
> + * At this point the CPU is powered up, so set the no powerdown
> + * request bit so we don't lose power and emulate power down.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> +
> + /* Bail out if CPU is powered up */
> + if (likely(drvdata->edprsr & EDPRSR_PU))
> + return;
> +
> + /*
> + * Handle race condition if CPU has been waken up but it sleeps
> + * again if EDPRCR_CORENPDRQ has been flipped, so try to run
> + * waken flow one more time.
> + */
> + if (!retried) {
> + retried = true;
> + goto try_again;
> + }
> +}
> +
> +static void debug_read_regs(struct debug_drvdata *drvdata)
> +{
> + u32 save_edprcr;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Unlock os lock */
> + debug_os_unlock(drvdata);
> +
> + /* Save EDPRCR register */
> + save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
> +
> + /*
> + * Ensure CPU power domain is enabled to let registers
> + * are accessiable.
> + */
> + debug_force_cpu_powered_up(drvdata);
> +
> + if (!debug_access_permitted(drvdata))
> + goto out;
> +
> + 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)
> + goto out;
> +
> + /*
> + * 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.
> + */
> + if (IS_ENABLED(CONFIG_64BIT))
> + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> + if (drvdata->edcidsr_present)
> + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
> +
> + if (drvdata->edvidsr_present)
> + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
> +
> +out:
> + /* Restore EDPRCR register */
> + writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
> +
> + CS_LOCK(drvdata->base);
> +}
> +
> +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
> +{
> + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
> + unsigned long pc;
> +
> + if (IS_ENABLED(CONFIG_64BIT))
> + return (unsigned long)drvdata->edpcsr_hi << 32 |
> + (unsigned long)drvdata->edpcsr;
> +
> + pc = (unsigned long)drvdata->edpcsr;
> +
> + if (drvdata->pc_has_offset) {
> + 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;
> +}
> +
> +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)) {
> + 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;
> + }
> +
> + pc = debug_adjust_pc(drvdata);
> + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
> +
> + if (drvdata->edcidsr_present)
> + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
> +
> + if (drvdata->edvidsr_present)
> + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits 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 ? 64 : 32,
> + drvdata->edvidsr & (u32)EDVIDSR_VMID);
> +}
> +
> +static void debug_init_arch_data(void *info)
> +{
> + struct debug_drvdata *drvdata = info;
> + u32 mode, pcsr_offset;
> + u32 eddevid, eddevid1;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Read device info */
> + eddevid = readl_relaxed(drvdata->base + EDDEVID);
> + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> +
> + CS_LOCK(drvdata->base);
> +
> + /* Parse implementation feature */
> + mode = eddevid & EDDEVID_PCSAMPLE_MODE;
> + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> +
> + drvdata->edpcsr_present = false;
> + drvdata->edcidsr_present = false;
> + drvdata->edvidsr_present = false;
> + drvdata->pc_has_offset = false;
> +
> + switch (mode) {
> + case EDDEVID_IMPL_FULL:
> + drvdata->edvidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR_EDCIDSR:
> + drvdata->edcidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR:
> + /*
> + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
> + * define if has the offset for PC sampling value; if read
> + * back EDDEVID1.PCSROffset == 0x2, then this means the debug
> + * module does not sample the instruction set state when
> + * armv8 CPU in AArch32 state.
> + */
> + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
> + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
Following my previous comment in patch 2/6 this condition needs to be
reviewed to deal with conditions where pcsr_offset == 0 on AArch64.
Probably
drvdata->edpcsr_present = ((IS_ENABLED(CONFIG_64BIT) &&
pcsr_offset != 0)) ||
(pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
> +
> + drvdata->pc_has_offset =
> + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * Dump out information on panic.
> + */
> +static int debug_notifier_call(struct notifier_block *self,
> + unsigned long v, void *p)
> +{
> + int cpu;
> + struct debug_drvdata *drvdata;
> +
> + pr_emerg("ARM external debug module:\n");
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pr_emerg("CPU[%d]:\n", drvdata->cpu);
> +
> + debug_read_regs(drvdata);
> + debug_dump_regs(drvdata);
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block debug_notifier = {
> + .notifier_call = debug_notifier_call,
> +};
> +
> +static int debug_enable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_get_sync(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_disable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_put(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static ssize_t debug_func_knob_write(struct file *f,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + u8 val;
> + int ret;
> +
> + ret = kstrtou8_from_user(buf, count, 2, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&debug_lock);
> +
> + if (val == debug_enable)
> + goto out;
> +
> + if (val)
> + ret = debug_enable_func();
> + else
> + ret = debug_disable_func();
> +
> + if (ret) {
> + pr_err("%s: unable to %s debug function: %d\n",
> + __func__, val ? "enable" : "disable", ret);
> + goto err;
> + }
> +
> + debug_enable = val;
> +out:
> + ret = count;
> +err:
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static ssize_t debug_func_knob_read(struct file *f,
> + char __user *ubuf, size_t count, loff_t *ppos)
> +{
> + ssize_t ret;
> + char buf[2];
> +
> + mutex_lock(&debug_lock);
> +
> + buf[0] = '0' + debug_enable;
> + buf[1] = '\n';
> + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
> +
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static const struct file_operations debug_func_knob_fops = {
> + .open = simple_open,
> + .read = debug_func_knob_read,
> + .write = debug_func_knob_write,
> +};
> +
> +static int debug_func_init(void)
> +{
> + struct dentry *file;
> + int ret;
> +
> + /* Create debugfs node */
> + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> + if (!debug_debugfs_dir) {
> + pr_err("%s: unable to create debugfs directory\n", __func__);
> + return -ENOMEM;
> + }
> +
> + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> + debug_debugfs_dir, NULL, &debug_func_knob_fops);
> + if (!file) {
> + pr_err("%s: unable to create enable knob file\n", __func__);
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* Use sysfs node to enable functionality */
> + if (!debug_enable)
> + return 0;
> +
> + /* Register function to be called for panic */
> + ret = atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> + if (ret) {
> + pr_err("%s: unable to register notifier: %d\n",
> + __func__, ret);
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + debugfs_remove_recursive(debug_debugfs_dir);
> + return ret;
> +}
> +
> +static void debug_func_exit(void)
> +{
> + debugfs_remove_recursive(debug_debugfs_dir);
> +
> + /* Unregister panic notifier callback */
> + if (debug_enable)
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +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;
> + int ret;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> + if (per_cpu(debug_drvdata, drvdata->cpu)) {
> + dev_err(dev, "CPU%d drvdata has been initialized\n",
> + drvdata->cpu);
> + return -EBUSY;
> + }
> +
> + drvdata->dev = &adev->dev;
> + amba_set_drvdata(adev, 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;
> + ret = smp_call_function_single(drvdata->cpu,
> + debug_init_arch_data, drvdata, 1);
> + put_online_cpus();
> +
> + if (ret) {
> + dev_err(dev, "CPU%d debug arch init failed\n", drvdata->cpu);
> + goto err;
> + }
> +
> + if (!drvdata->edpcsr_present) {
> + dev_err(dev, "CPU%d sample-based profiling isn't implemented\n",
> + drvdata->cpu);
> + ret = -ENXIO;
> + goto err;
> + }
> +
> + if (!debug_count++) {
> + ret = debug_func_init();
> + if (ret)
> + goto err_func_init;
> + }
> +
> + if (!debug_enable)
> + pm_runtime_put(dev);
> +
> + dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu);
> + return 0;
> +
> +err_func_init:
> + debug_count--;
> +err:
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> + return ret;
> +}
> +
> +static int debug_remove(struct amba_device *adev)
> +{
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata = amba_get_drvdata(adev);
> +
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> +
> + if (debug_enable)
> + pm_runtime_put(dev);
> +
> + if (!--debug_count)
> + debug_func_exit();
> +
> + return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> + { /* Debug for Cortex-A53 */
> + .id = 0x000bbd03,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A57 */
> + .id = 0x000bbd07,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A72 */
> + .id = 0x000bbd08,
> + .mask = 0x000fffff,
> + },
> + { 0, 0 },
> +};
> +
> +static struct amba_driver debug_driver = {
> + .drv = {
> + .name = "coresight-cpu-debug",
> + .suppress_bind_attrs = true,
> + },
> + .probe = debug_probe,
> + .remove = debug_remove,
> + .id_table = debug_ids,
> +};
> +
> +module_amba_driver(debug_driver);
> +
> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
> +MODULE_DESCRIPTION("ARM Coresight CPU Debug Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 7/8] arm64: dts: hi6220: register debug module
2017-04-06 13:30 [PATCH v6 0/8] coresight: enable debug module Leo Yan
` (5 preceding siblings ...)
2017-04-06 13:30 ` [PATCH v6 6/8] coresight: add support for CPU debug module Leo Yan
@ 2017-04-06 13:31 ` Leo Yan
2017-04-06 13:31 ` [PATCH v6 8/8] arm64: dts: qcom: msm8916: Add debug unit Leo Yan
7 siblings, 0 replies; 23+ messages in thread
From: Leo Yan @ 2017-04-06 13:31 UTC (permalink / raw)
To: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel, devicetree, linux-arm-kernel, linux-arm-msm,
linux-soc, Mike Leach, Sudeep Holla
Cc: Leo Yan
Bind debug module driver for Hi6220.
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 64 +++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 470461d..467aa15 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -913,5 +913,69 @@
};
};
};
+
+ debug@f6590000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0 0xf6590000 0 0x1000>;
+ clocks = <&sys_ctrl HI6220_DAPB_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&cpu0>;
+ };
+
+ debug@f6592000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0 0xf6592000 0 0x1000>;
+ clocks = <&sys_ctrl HI6220_DAPB_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&cpu1>;
+ };
+
+ debug@f6594000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0 0xf6594000 0 0x1000>;
+ clocks = <&sys_ctrl HI6220_DAPB_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&cpu2>;
+ };
+
+ debug@f6596000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0 0xf6596000 0 0x1000>;
+ clocks = <&sys_ctrl HI6220_DAPB_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&cpu3>;
+ };
+
+ debug@f65d0000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0 0xf65d0000 0 0x1000>;
+ clocks = <&sys_ctrl HI6220_DAPB_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&cpu4>;
+ };
+
+ debug@f65d2000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0 0xf65d2000 0 0x1000>;
+ clocks = <&sys_ctrl HI6220_DAPB_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&cpu5>;
+ };
+
+ debug@f65d4000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0 0xf65d4000 0 0x1000>;
+ clocks = <&sys_ctrl HI6220_DAPB_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&cpu6>;
+ };
+
+ debug@f65d6000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0 0xf65d6000 0 0x1000>;
+ clocks = <&sys_ctrl HI6220_DAPB_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&cpu7>;
+ };
};
};
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v6 8/8] arm64: dts: qcom: msm8916: Add debug unit
2017-04-06 13:30 [PATCH v6 0/8] coresight: enable debug module Leo Yan
` (6 preceding siblings ...)
2017-04-06 13:31 ` [PATCH v6 7/8] arm64: dts: hi6220: register " Leo Yan
@ 2017-04-06 13:31 ` Leo Yan
7 siblings, 0 replies; 23+ messages in thread
From: Leo Yan @ 2017-04-06 13:31 UTC (permalink / raw)
To: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Mathieu Poirier, Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel, devicetree, linux-arm-kernel, linux-arm-msm,
linux-soc, Mike Leach, Sudeep Holla
Cc: Leo Yan
Add debug unit on Qualcomm msm8916 based platforms, including the
DragonBoard 410c board.
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
arch/arm64/boot/dts/qcom/msm8916.dtsi | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 68a8e67..3af814b 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -1104,6 +1104,38 @@
};
};
+ debug@850000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0x850000 0x1000>;
+ clocks = <&rpmcc RPM_QDSS_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&CPU0>;
+ };
+
+ debug@852000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0x852000 0x1000>;
+ clocks = <&rpmcc RPM_QDSS_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&CPU1>;
+ };
+
+ debug@854000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0x854000 0x1000>;
+ clocks = <&rpmcc RPM_QDSS_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&CPU2>;
+ };
+
+ debug@856000 {
+ compatible = "arm,coresight-cpu-debug","arm,primecell";
+ reg = <0x856000 0x1000>;
+ clocks = <&rpmcc RPM_QDSS_CLK>;
+ clock-names = "apb_pclk";
+ cpu = <&CPU3>;
+ };
+
etm@85c000 {
compatible = "arm,coresight-etm4x", "arm,primecell";
reg = <0x85c000 0x1000>;
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread