From: Michal Simek <michal.simek@xilinx.com>
To: "Wojciech Żmuda" <wzmuda@n7space.com>,
"Mathieu Poirier" <mathieu.poirier@linaro.org>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"rajan.vaja@xilinx.com" <rajan.vaja@xilinx.com>,
"Michal Mosdorf" <mmosdorf@n7space.com>,
"antoine.tenart@bootlin.com" <antoine.tenart@bootlin.com>,
"nava.manne@xilinx.com" <nava.manne@xilinx.com>,
"coresight@lists.linaro.org" <coresight@lists.linaro.org>,
"michal.simek@xilinx.com" <michal.simek@xilinx.com>,
"m.tretter@pengutronix.de" <m.tretter@pengutronix.de>,
"t-kristo@ti.com" <t-kristo@ti.com>,
"Krzysztof Pilch" <kpilch@n7space.com>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"Jolly Shah" <jolly.shah@xilinx.com>,
"Michał Kurowski" <mkurowski@n7space.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/1] arm64: zynqmp: Add CoreSight components
Date: Mon, 3 Feb 2020 08:12:14 +0100 [thread overview]
Message-ID: <a108d3ee-0959-d5f2-ecc7-a301929412ec@xilinx.com> (raw)
In-Reply-To: <CDE42254-9963-4D2B-B762-032B8C947810@n7space.com>
On 31. 01. 20 20:37, Wojciech Żmuda wrote:
> Hello Mathieu, thank you for quick review!
> Please see my responses inline
>
> On 31/01/2020, 19:47, "Mathieu Poirier" <mathieu.poirier@linaro.org> wrote:
>> On Fri, 31 Jan 2020 at 11:36, Mathieu Poirier
>> <mathieu.poirier@linaro.org> wrote:
>>
>>> Hi Wojciech,
>>>
>>> On Thu, Jan 30, 2020 at 03:36:27PM +0000, Wojciech Żmuda wrote:
>>>> From: Wojciech Zmuda <wzmuda@n7space.com>
>>>>
>>>> Add nodes for the following CoreSight components:
>>>> - ETMs for A53 cores
>>>> - debug components for A53 cores
>>>> - funnel gathering outputs from A53 ETMs and SoC-wide funnels
>>>> - the only replicator
>>>> - all TMCs: 4k ETF, 8k ETF and ETR
>>>> - TPIU
>>>>
>>>> Signed-off-by: Wojciech Zmuda <wzmuda@n7space.com>
>>>> ---
>>>> arch/arm64/boot/dts/xilinx/zynqmp-coresight.dtsi | 272 +++++++++++++++++++++++
>>>> arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 2 +
>>>> 2 files changed, 274 insertions(+)
>>>> create mode 100644 arch/arm64/boot/dts/xilinx/zynqmp-coresight.dtsi
>>>>
>>>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-coresight.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp-coresight.dtsi
>>>> new file mode 100644
>>>> index 000000000000..8b7579ad89cc
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp-coresight.dtsi
>>>> @@ -0,0 +1,272 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +
>>>
>>> Looking at other Xilinx DT files there is no space between the SPDX identifier
>>> and the header of the file.
>
> Ok, I'll remove the extra space.
>
>>>
>>>> +/*
>>>> + * dtsi for Xilinx Ultrascale+ MPSoC CoreSight components
>>>> + *
>>>> + * Copyright (C) 2019-2020 N7 Space Sp. z o.o.
>>>> + *
>>>> + * Author: Wojciech Zmuda <wzmuda@n7space.com>
>>>> + *
>>>> + */
>>>> +/ {
>>>> + etm0@fec40000 {
>>>> + compatible = "arm,coresight-etm4x", "arm,primecell";
>>>> + reg = <0 0xfec40000 0 0x1000>;
>>>> + cpu = <&cpu0>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + out-ports {
>>>> + port {
>>>> + etm0_out_port: endpoint {
>>>> + remote-endpoint = <&funnel1_in_port0>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + etm1@fed40000 {
>>>> + compatible = "arm,coresight-etm4x", "arm,primecell";
>>>> + reg = <0 0xfed40000 0 0x1000>;
>>>> + cpu = <&cpu1>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + out-ports {
>>>> + port {
>>>> + etm1_out_port: endpoint {
>>>> + remote-endpoint = <&funnel1_in_port1>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + etm2@fee40000 {
>>>> + compatible = "arm,coresight-etm4x", "arm,primecell";
>>>> + reg = <0 0xfee40000 0 0x1000>;
>>>> + cpu = <&cpu2>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + out-ports {
>>>> + port {
>>>> + etm2_out_port: endpoint {
>>>> + remote-endpoint = <&funnel1_in_port2>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + etm3@fef40000 {
>>>> + compatible = "arm,coresight-etm4x", "arm,primecell";
>>>> + reg = <0 0xfef40000 0 0x1000>;
>>>> + cpu = <&cpu3>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + out-ports {
>>>> + port {
>>>> + etm3_out_port: endpoint {
>>>> + remote-endpoint = <&funnel1_in_port3>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + debug0@fec10000 {
>>>> + compatible = "arm,coresight-cpu-debug", "arm,primecell";
>>>> + reg = <0 0xfec10000 0 0x1000>;
>>>> + cpu = <&cpu0>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + };
>>>> +
>>>> + debug1@fed10000 {
>>>> + compatible = "arm,coresight-cpu-debug", "arm,primecell";
>>>> + reg = <0 0xfed10000 0 0x1000>;
>>>> + cpu = <&cpu1>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + };
>>>> +
>>>> + debug2@fee10000 {
>>>> + compatible = "arm,coresight-cpu-debug", "arm,primecell";
>>>> + reg = <0 0xfee10000 0 0x1000>;
>>>> + cpu = <&cpu2>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + };
>>>> +
>>>> + debug3@fee10000 {
>>>> + compatible = "arm,coresight-cpu-debug", "arm,primecell";
>>>> + reg = <0 0xfef10000 0 0x1000>;
>>>> + cpu = <&cpu3>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + };
>>>> +
>>>> + funnel1@fe920000 {
>>>> + compatible = "arm,coresight-dynamic-funnel", "arm,primecell";
>>>> + reg = <0 0xfe920000 0 0x1000>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + in-ports {
>>>> + #address-cells = <0x1>;
>>>> + #size-cells = <0x0>;
>>>> + port@0 {
>>>> + reg = <0x0>;
>>>> + funnel1_in_port0: endpoint {
>>>> + remote-endpoint = <&etm0_out_port>;
>>>> + };
>>>> + };
>>>> + port@1 {
>>>> + reg = <0x1>;
>>>> + funnel1_in_port1: endpoint {
>>>> + remote-endpoint = <&etm1_out_port>;
>>>> + };
>>>> + };
>>>> + port@2 {
>>>> + reg = <0x2>;
>>>> + funnel1_in_port2: endpoint {
>>>> + remote-endpoint = <&etm2_out_port>;
>>>> + };
>>>> + };
>>>> + port@3 {
>>>> + reg = <0x3>;
>>>> + funnel1_in_port3: endpoint {
>>>> + remote-endpoint = <&etm3_out_port>;
>>>> + };
>>>> + };
>>>> + };
>>>> + out-ports {
>>>> + port {
>>>> + funnel1_out_port0: endpoint {
>>>> + remote-endpoint = <&etf1_in_port>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + funnel2@fe930000 {
>>>> + compatible = "arm,coresight-dynamic-funnel", "arm,primecell";
>>>> + reg = <0 0xfe930000 0 0x1000>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + in-ports {
>>>> + #address-cells = <0x1>;
>>>> + #size-cells = <0x0>;
>>>> + port@2 {
>>>> + reg = <0x2>;
>>>> + funnel2_in_port2: endpoint {
>>>> + remote-endpoint = <&etf1_out_port>;
>>>> + };
>>>> + };
>>>> + // Funnel2 has another input port connected to
>>>> + // funnel0's output. Funnel0 gathers Cortex-R5 ETMs.
>>>
>>> C++ style comments.
>>>
>
> Ok, I'll go with:
> /*
> * Funnel2 has another input port connected to
> * funnel0's output. Funnel0 gathers Cortex-R5 ETMs.
> */
>
>>>> + };
>>>> + out-ports {
>>>> + port {
>>>> + funnel2_out_port0: endpoint {
>>>> + remote-endpoint = <&etf2_in_port>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + etf1@fe940000 {
>>>> + compatible = "arm,coresight-tmc", "arm,primecell";
>>>> + reg = <0 0xfe940000 0 0x1000>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + in-ports {
>>>> + port {
>>>> + etf1_in_port: endpoint {
>>>> + remote-endpoint = <&funnel1_out_port0>;
>>>> + };
>>>> + };
>>>> + };
>>>> + out-ports {
>>>> + port {
>>>> + etf1_out_port: endpoint {
>>>> + remote-endpoint = <&funnel2_in_port2>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + etf2@fe950000 {
>>>> + compatible = "arm,coresight-tmc", "arm,primecell";
>>>> + reg = <0 0xfe950000 0 0x1000>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + in-ports {
>>>> + port {
>>>> + etf2_in_port: endpoint {
>>>> + remote-endpoint = <&funnel2_out_port0>;
>>>> + };
>>>> + };
>>>> + };
>>>> + out-ports {
>>>> + port {
>>>> + etf2_out_port: endpoint {
>>>> + remote-endpoint =
>>>> + <&replicator_in_port0>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + replicator {
>>>> + compatible = "arm,coresight-static-replicator";
>>>> + in-ports {
>>>> + port {
>>>> + replicator_in_port0: endpoint {
>>>> + remote-endpoint = <&etf2_out_port>;
>>>> + };
>>>> + };
>>>> + };
>>>> + out-ports {
>>>> + #address-cells = <0x1>;
>>>> + #size-cells = <0x0>;
>>>> + port@0 {
>>>> + reg = <0x0>;
>>>> + replicator_out_port0: endpoint {
>>>> + remote-endpoint = <&etr_in_port>;
>>>> + };
>>>> + };
>>>> + port@1 {
>>>> + reg = <0x1>;
>>>> + replicator_out_port1: endpoint {
>>>> + remote-endpoint = <&tpiu_in_port>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + etr@fe970000 {
>>>> + compatible = "arm,coresight-tmc", "arm,primecell";
>>>> + reg = <0 0xfe970000 0 0x1000>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + in-ports {
>>>> + port {
>>>> + etr_in_port: endpoint {
>>>> + remote-endpoint =
>>>> + <&replicator_out_port0>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + tpiu@fe980000 {
>>>> + compatible = "arm,coresight-tpiu", "arm,primecell";
>>>> + reg = <0 0xfe980000 0 0x1000>;
>>>> + clocks = <&clk100>;
>>>> + clock-names = "apb_pclk";
>>>> + in-ports {
>>>> + port {
>>>> + tpiu_in_port: endpoint {
>>>> + remote-endpoint =
>>>> + <&replicator_out_port1>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +};
>>>> +
>>>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>>>> index 3c731e73903a..ca0a6b9f4445 100644
>>>> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>>>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>>>> @@ -12,6 +12,8 @@
>>>> * the License, or (at your option) any later version.
>>>> */
>>> >
>>>> +#include "zynqmp-coresight.dtsi"
>>>> +
>>>
>>> Those bindings are correctly used. If Michal doesn't mind the nit-picks I have
>>> highlighted above then
>>>
>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>
>>> Otherwise feel free to add my tag to your next revision, which I advise you to
>>> send out once v5.6-rc1 comes out.
>
> Great, thanks. I'll keep an eye for v5.6-rc1.
>
>>
>> I forgot... How does power management work on this board? Is the
>> power domain where the CS blocks are powered up by the FW? And what
>> about state retention when processors go idle? Should this be taken
>> care of in the drivers or is the PMIC properly handling low retention
>> states? Failure to properly address both cases will likely hang the
>> board (at boot time of when processors are idled).
>
> To be honest, I don't know. I use kernel with CPU idle permanently disabled, as was
> advised somewhere in CoreSight-related documents I found at the beginning of my
> experiments. It might've been a presentation from Linaro Connect, either yours or Leo's.
> I didn't do any extra steps to power CS blocks on, so I guess they're either powered on
> by default, or it's taken care of by either FSBL, U-boot or ATF. I use those three provided
> by Xilinx. I didn't experiment with vanilla U-boot or ATF.
>
> Can you please suggest some experiments I in this matter? Would turning off CPU idle
> in kernel's config and tracing a migrating process be sufficient for such test?
Jolly: Can you please take a look how this block is powered and how
power stuff should be handled?
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-02-03 7:12 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-30 15:36 [PATCH 0/1] Xilinx Zynq Ultrascale+ MPSoc CoreSight DT nodes Wojciech Żmuda
2020-01-30 15:36 ` [PATCH 1/1] arm64: zynqmp: Add CoreSight components Wojciech Żmuda
2020-01-31 18:36 ` Mathieu Poirier
2020-01-31 18:47 ` Mathieu Poirier
2020-01-31 19:37 ` Wojciech Żmuda
2020-02-03 7:12 ` Michal Simek [this message]
2020-02-03 17:57 ` Mathieu Poirier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a108d3ee-0959-d5f2-ecc7-a301929412ec@xilinx.com \
--to=michal.simek@xilinx.com \
--cc=antoine.tenart@bootlin.com \
--cc=coresight@lists.linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=jolly.shah@xilinx.com \
--cc=kpilch@n7space.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=m.tretter@pengutronix.de \
--cc=mark.rutland@arm.com \
--cc=mathieu.poirier@linaro.org \
--cc=mkurowski@n7space.com \
--cc=mmosdorf@n7space.com \
--cc=nava.manne@xilinx.com \
--cc=rajan.vaja@xilinx.com \
--cc=robh+dt@kernel.org \
--cc=t-kristo@ti.com \
--cc=wzmuda@n7space.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox