Hello Mathieu, thank you for quick review!
Please see my responses inline
On 31/01/2020, 19:47, "Mathieu Poirier" <mathieu.poirier@xxxxxxxxxx> wrote:
> On Fri, 31 Jan 2020 at 11:36, Mathieu Poirier
> <mathieu.poirier@xxxxxxxxxx> wrote:
>
> > Hi Wojciech,
> >
> > On Thu, Jan 30, 2020 at 03:36:27PM +0000, Wojciech Żmuda wrote:
> > > From: Wojciech Zmuda <wzmuda@xxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxx>
> > > + *
> > > + */
> > > +/ {
> > > + 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@xxxxxxxxxx>
> >
> > 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
> > >
