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@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?
Jolly: Can you please take a look how this block is powered and how
power stuff should be handled?
Thanks,
Michal
