Message-ID: <a68fa63d-9d1d-d295-02a4-6f5405ae1d7c@linaro.org>
Date:   Mon, 6 Mar 2023 11:33:49 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH 4/6] ARM: dts: qcom: sdx65: Add support for PCIe EP
Content-Language: en-US
To:     Rohit Agarwal <quic_rohiagar@quicinc.com>, agross@kernel.org,
        andersson@kernel.org, lee@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, mani@kernel.org,
        lpieralisi@kernel.org, kw@linux.com, bhelgaas@google.com,
        manivannan.sadhasivam@linaro.org
Cc:     linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
References: <1678080302-29691-1-git-send-email-quic_rohiagar@quicinc.com>
 <1678080302-29691-5-git-send-email-quic_rohiagar@quicinc.com>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <1678080302-29691-5-git-send-email-quic_rohiagar@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6.03.2023 06:25, Rohit Agarwal wrote:
> Add support for PCIe Endpoint controller on the Qualcomm SDX65 platform.
> 
> Signed-off-by: Rohit Agarwal <quic_rohiagar@quicinc.com>
> ---
>  arch/arm/boot/dts/qcom-sdx65.dtsi | 44 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/qcom-sdx65.dtsi b/arch/arm/boot/dts/qcom-sdx65.dtsi
> index 246290d..93ea94e 100644
> --- a/arch/arm/boot/dts/qcom-sdx65.dtsi
> +++ b/arch/arm/boot/dts/qcom-sdx65.dtsi
> @@ -11,6 +11,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/power/qcom-rpmpd.h>
>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
> +#include <dt-bindings/gpio/gpio.h>
>  
>  / {
>  	#address-cells = <1>;
> @@ -292,6 +293,44 @@
>  			status = "disabled";
>  		};
>  
> +		pcie_ep: pcie-ep@1c00000 {
> +			compatible = "qcom,sdx65-pcie-ep", "qcom,sdx55-pcie-ep";
> +			reg = <0x01c00000 0x3000>,
> +				  <0x40000000 0xf1d>,
> +				  <0x40000f20 0xa8>,
> +				  <0x40001000 0x1000>,
> +				  <0x40200000 0x100000>,
> +				  <0x01c03000 0x3000>;
The indentation here seems incorrect. The kernel uses 8-wide tabs.

> +			reg-names = "parf", "dbi", "elbi", "atu", "addr_space",
> +						"mmio";
Please turn this into a vertical list, like this:

"parf",
"dbi",
...

> +
> +			qcom,perst-regs = <&tcsr 0xb258 0xb270>;
> +
> +			clocks = <&gcc GCC_PCIE_AUX_CLK>,
> +					 <&gcc GCC_PCIE_CFG_AHB_CLK>,
> +					 <&gcc GCC_PCIE_MSTR_AXI_CLK>,
> +					 <&gcc GCC_PCIE_SLV_AXI_CLK>,
> +					 <&gcc GCC_PCIE_SLV_Q2A_AXI_CLK>,
> +					 <&gcc GCC_PCIE_SLEEP_CLK>,
> +					 <&gcc GCC_PCIE_0_CLKREF_EN>;
The indentation here seems incorrect.

> +			clock-names = "aux", "cfg", "bus_master", "bus_slave",
> +							"slave_q2a", "sleep", "ref";
Please turn this into a vertical list too.

> +
> +			interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>,
> +						 <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
The indentation seems off here.

Konrad
> +			interrupt-names = "global", "doorbell";
> +			reset-gpios = <&tlmm 57 GPIO_ACTIVE_LOW>;
> +			wake-gpios = <&tlmm 53 GPIO_ACTIVE_LOW>;
> +			resets = <&gcc GCC_PCIE_BCR>;
> +			reset-names = "core";
> +			power-domains = <&gcc PCIE_GDSC>;
> +			phys = <&pcie0_lane>;
> +			phy-names = "pciephy";
> +			max-link-speed = <3>;
> +			num-lanes = <2>;
> +			status = "disabled";
> +		};
> +
>  		pcie0_phy: phy@1c07000 {
>  			compatible = "qcom,sdx65-qmp-pcie-phy";
>  			reg = <0x01c07000 0x1e4>;
> @@ -330,6 +369,11 @@
>  			#hwlock-cells = <1>;
>  		};
>  
> +		tcsr: syscon@1fcb000 {
> +			compatible = "qcom,sdx65-tcsr", "syscon";
> +			reg = <0x01fc0000 0x1000>;
> +		};
> +
>  		remoteproc_mpss: remoteproc@4080000 {
>  			compatible = "qcom,sdx55-mpss-pas";
>  			reg = <0x04080000 0x4040>;