From: "Wojciech Żmuda" <wzmuda@n7space.com>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: "linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"coresight@lists.linaro.org" <coresight@lists.linaro.org>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"mark.rutland@arm.com" <mark.rutland@arm.com>,
"michal.simek@xilinx.com" <michal.simek@xilinx.com>,
"m.tretter@pengutronix.de" <m.tretter@pengutronix.de>,
"nava.manne@xilinx.com" <nava.manne@xilinx.com>,
"antoine.tenart@bootlin.com" <antoine.tenart@bootlin.com>,
"t-kristo@ti.com" <t-kristo@ti.com>,
"rajan.vaja@xilinx.com" <rajan.vaja@xilinx.com>,
"Krzysztof Pilch" <kpilch@n7space.com>,
"Michal Mosdorf" <mmosdorf@n7space.com>,
"Michał Kurowski" <mkurowski@n7space.com>
Subject: Re: [PATCH 1/1] arm64: zynqmp: Add CoreSight components
Date: Fri, 31 Jan 2020 19:37:19 +0000 [thread overview]
Message-ID: <CDE42254-9963-4D2B-B762-032B8C947810@n7space.com> (raw)
In-Reply-To: <CANLsYkxOLdMjFkjJSZ7Y1OLSCefb0fFiNVnaWyrgOUyDfWtweg@mail.gmail.com>
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?
Best regards,
Wojciech
>
> >
> > Thanks,
> > Mathieu
> >
> > > / {
> > > compatible = "xlnx,zynqmp";
> > > #address-cells = <2>;
> > > --
> > > 2.11.0
> > >
next prev parent reply other threads:[~2020-01-31 19:37 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 [this message]
2020-02-03 7:12 ` Michal Simek
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=CDE42254-9963-4D2B-B762-032B8C947810@n7space.com \
--to=wzmuda@n7space.com \
--cc=antoine.tenart@bootlin.com \
--cc=coresight@lists.linaro.org \
--cc=devicetree@vger.kernel.org \
--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=michal.simek@xilinx.com \
--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 \
/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