Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 5/5] ARM: dts: qcom: msm8974: add interconnect nodes
To:     Brian Masney <masneyb@onstation.org>, agross@kernel.org,
        bjorn.andersson@linaro.org
Cc:     robh+dt@kernel.org, mark.rutland@arm.com,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
References: <20191013080804.10231-1-masneyb@onstation.org>
 <20191013080804.10231-6-masneyb@onstation.org>
From:   Georgi Djakov <georgi.djakov@linaro.org>
Openpgp: preference=signencrypt
Message-ID: <d154b0c6-fc39-bebc-d1b5-cc179fb6055d@linaro.org>
Date:   Wed, 23 Oct 2019 14:50:19 +0300
MIME-Version: 1.0
In-Reply-To: <20191013080804.10231-6-masneyb@onstation.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Brian,

Thanks for the patch!

On 13.10.19 г. 11:08 ч., Brian Masney wrote:
> Add interconnect nodes that's needed to support bus scaling.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  arch/arm/boot/dts/qcom-msm8974.dtsi | 60 +++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
> index bdbde5125a56..ed98d14a88b1 100644
> --- a/arch/arm/boot/dts/qcom-msm8974.dtsi
> +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /dts-v1/;
>  
> +#include <dt-bindings/interconnect/qcom,msm8974.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,gcc-msm8974.h>
>  #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> @@ -1106,6 +1107,60 @@
>  			};
>  		};
>  
> +		bimc: interconnect@fc380000 {
> +			reg = <0xfc380000 0x6a000>;
> +			compatible = "qcom,msm8974-bimc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_BIMC_CLK>,
> +			         <&rpmcc RPM_SMD_BIMC_A_CLK>;
> +		};
> +
> +		cnoc: interconnect@fc480000 {
> +			reg = <0xfc480000 0x4000>;
> +			compatible = "qcom,msm8974-cnoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_CNOC_CLK>,
> +			         <&rpmcc RPM_SMD_CNOC_A_CLK>;
> +		};
> +
> +		mmssnoc: interconnect@fc478000 {
> +			reg = <0xfc478000 0x4000>;
> +			compatible = "qcom,msm8974-mmssnoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&mmcc MMSS_S0_AXI_CLK>,
> +			         <&mmcc MMSS_S0_AXI_CLK>;
> +		};
> +
> +		ocmemnoc: interconnect@fc470000 {
> +			reg = <0xfc470000 0x4000>;
> +			compatible = "qcom,msm8974-ocmemnoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_OCMEMGX_CLK>,
> +			         <&rpmcc RPM_SMD_OCMEMGX_A_CLK>;
> +		};
> +
> +		pnoc: interconnect@fc468000 {
> +			reg = <0xfc468000 0x4000>;
> +			compatible = "qcom,msm8974-pnoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_PNOC_CLK>,
> +			         <&rpmcc RPM_SMD_PNOC_A_CLK>;
> +		};
> +
> +		snoc: interconnect@fc460000 {
> +			reg = <0xfc460000 0x4000>;
> +			compatible = "qcom,msm8974-snoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_SNOC_CLK>,
> +			         <&rpmcc RPM_SMD_SNOC_A_CLK>;
> +		};

It would have been nice to have the DT nodes sorted by address, but i suppose it
doesn't make much difference, as the rest of the nodes in this file are unsorted
anyway.

> +
>  		mdss: mdss@fd900000 {
>  			status = "disabled";
>  
> @@ -1152,6 +1207,11 @@
>  				              "core",
>  				              "vsync";
>  
> +				interconnects = <&mmssnoc MNOC_MAS_GRAPHICS_3D &bimc BIMC_SLV_EBI_CH0>,
> +				                <&ocmemnoc OCMEM_VNOC_MAS_GFX3D &ocmemnoc OCMEM_SLV_OCMEM>;

Who will be the requesting bandwidth to DDR and ocmem? Is it the display or GPU
or both? The above seem like GPU-related interconnects, so maybe these
properties should be in the GPU DT node.

> +				interconnect-names = "mdp0-mem",
> +				                     "mdp1-mem";

As the second path is not to DDR, but to ocmem, it might be better to call it
something like "gpu-ocmem".

Thanks,
Georgi

> +
>  				ports {
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>