Re: [PATCH v1 1/2] coresight: bindings for debug module

Hi Mike,

On Mon, Feb 27, 2017 at 12:37:45PM +0000, Mike Leach wrote:

Hi Leo

On 23 February 2017 at 01:57, Leo Yan [off-list ref] wrote:

quoted

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 two
clocks, one is apb clock for bus and another is debug clock for debug
module self; and also need specify the CPU node which the debug module
is dedicated to specific CPU.

Signed-off-by: Leo Yan <redacted>
---
 .../devicetree/bindings/arm/coresight-debug.txt    | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/coresight-debug.txt

diff --git a/Documentation/devicetree/bindings/arm/coresight-debug.txt b/Documentation/devicetree/bindings/arm/coresight-debug.txt
new file mode 100644
index 0000000..6e03e9b
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/coresight-debug.txt

@@ -0,0 +1,39 @@
+* CoreSight Debug Component:
+
+CoreSight 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-debug", "arm,primecell"; supplemented with
+              "arm,primecell" as driver is using the AMBA bus interface.
+
+- reg : physical base address and length of the register set.
+
+- clocks : the clocks associated to this component.
+
+- clock-names : the name of the clocks 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 debug module
+               has an additional clock "dbg_clk", which is used to provide
+               clock for debug module itself. Both clocks are mandatory.
+

Perhaps I am misunderstanding the nature of the .dts files, but I'm
puzzled about the dbg_clk. I cannot see anything in the architecture
or normal A53 implementation that mentions this.
To access external debug from the core/external debug agent then we do
need the APB clock, but the interface between the debug logic and the
processor core is clocked by the internal CPU clocks.

You are right and sorry I may introduce confusion at here.

When I wrote this doc for binding, I assumed every debug logic has
its own clock source. This is because there have one register
ACPU_SC_PDBGUP_MBIST with eight bits, every bit is used to control
every CPU's DBGPWRDUP signal. I wrongly understood this bit is used
for debug logic clock gating.

I read CA53 TRM, the DBGPWRDUP signal actually is used between power
controller and CPU so can ensure CPU can be safely enter and exit low
power states; and "The EDPRSR.PU bit reflects the value of this
DBGPWRDUP signal". So I made mistake here and the bits in
ACPU_SC_PDBGUP_MBIST is no matter with debug logic clocks.

I will remove the "dbg_clk" from binding doc. Please correct me if my
understanding is still wrong or blur.

quoted

+- cpu : the cpu phandle the debug module is affined to. When omitted
+       the source is considered to belong to CPU0.
+
+Example:
+
+       debug@f6590000 {
+               compatible = "arm,coresight-debug","arm,primecell";
+               reg = <0 0xf6590000 0 0x1000>;
+               clocks = <&sys_ctrl HI6220_CS_ATB>, <&acpu_ctrl HI6220_ACPU_DBG_CLK0>;
+               clock-names = "apb_pclk", "dbg_clk";
+               cpu = <&cpu0>;
+       };
--
2.7.4

When I was looking at clocks for trace on the HiKey board I noted:
HI6220_CS_DAPB -- which I assumed was the debug APB clock.
HI6220_CS_ATB - which I assumed was the ATB (trace bus) clock.

The clock HI6220_CS_DAPB is a mux; and after went through the spec for
register definition, the clock driver misses the dapb gate clock in
its system controller 'sysctrl'; And I checked this bit is luckily
enabled by bootloaders (I have not checked it's enabled by ARM-TF or
UEFI) so the driver can access debug module registers.

diff --git a/drivers/clk/hisilicon/clk-hi6220.c b/drivers/clk/hisilicon/clk-hi6220.c
index 553d083..a4ac75e 100644
--- a/drivers/clk/hisilicon/clk-hi6220.c
+++ b/drivers/clk/hisilicon/clk-hi6220.c

@@ -134,6 +134,7 @@ static struct hisi_gate_clock hi6220_separated_gate_clks_sys[] __initdata = {
        { HI6220_UART4_PCLK,    "uart4_pclk",    "uart4_src",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 8,  0, },
        { HI6220_SPI_CLK,       "spi_clk",       "clk_150m",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 9,  0, },
        { HI6220_TSENSOR_CLK,   "tsensor_clk",   "clk_bus",        CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 12, 0, },
+       { HI6220_DAPB_CLK,      "dapb_clk",      "cs_dapb",        CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x230, 18, 0, },
        { HI6220_MMU_CLK,       "mmu_clk",       "ddrc_axi1",      CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x240, 11, 0, },
        { HI6220_HIFI_SEL,      "hifi_sel",      "hifi_src",       CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 0,  0, },
        { HI6220_MMC0_SYSPLL,   "mmc0_syspll",   "syspll",         CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x270, 1,  0, },

Thanks,
Leo Yan