Re: [PATCH 5/6] arm64: dts: fsd: Add PCIe support for Tesla FSD SoC

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 21/11/2022 11:52, Shradha Todi wrote:
> Add the support for PCIe controller driver and phy driver
> for Tesla FSD. It includes support for both RC and EP.
> 
> Signed-off-by: Niyas Ahmed S T <niyas.ahmed@samsung.com>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  arch/arm64/boot/dts/tesla/fsd-evb.dts      |  48 ++++++
>  arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi |  65 ++++++++
>  arch/arm64/boot/dts/tesla/fsd.dtsi         | 171 +++++++++++++++++++++
>  3 files changed, 284 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> index 1db6ddf03f01..cda72b0f76f8 100644
> --- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> +++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> @@ -41,3 +41,51 @@
>  &ufs {
>  	status = "okay";
>  };
> +
> +&pcie_phy0 {
> +	status = "disabled";

It's a double disable, isn't it?

> +};
> +
> +&pcie_phy1 {
> +	status = "disabled";
> +};
> +
> +&pcie4_rc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie1_clkreq>, <&pcie1_wake>, <&pcie1_preset>,
> +			<&pcie0_slot1>;
> +	status = "disabled";

???

> +};
> +
> +&pcie4_ep {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie1_clkreq>, <&pcie1_wake>, <&pcie1_preset>,
> +			<&pcie0_slot1>;
> +	status = "disabled";
> +};
> +
> +&pcie0_rc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie0_clkreq>, <&pcie0_wake0>, <&pcie0_preset0>,
> +			 <&pcie0_slot0>;
> +	status = "disabled";
> +};
> +
> +&pcie0_ep {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie0_clkreq>, <&pcie0_wake0>, <&pcie0_preset0>,
> +			 <&pcie0_slot0>;
> +	status = "disabled";
> +};
> +
> +&pcie1_rc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie0_clkreq>, <&pcie0_wake1>, <&pcie0_preset0>;
> +	status = "disabled";
> +};
> +
> +&pcie1_ep {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie0_clkreq>, <&pcie0_wake1>, <&pcie0_preset0>;
> +	status = "disabled";

Ordering is broken. All overrides/extends are ordered by label name.

> +};
> diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> index d0abb9aa0e9e..edae62dfa987 100644
> --- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> +++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> @@ -64,6 +64,27 @@
>  		samsung,pin-pud = <FSD_PIN_PULL_NONE>;
>  		samsung,pin-drv = <FSD_PIN_DRV_LV2>;
>  	};
> +
> +	pcie1_clkreq: pcie1-clkreq {

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

(...)

>  
>  &pinctrl_pmu {
> diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
> index f35bc5a288c2..2177f6964553 100644
> --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> @@ -32,6 +32,14 @@
>  		spi0 = &spi_0;
>  		spi1 = &spi_1;
>  		spi2 = &spi_2;
> +		pciephy0 = &pcie_phy0;
> +		pciephy1 = &pcie_phy1;
> +		pcierc0 = &pcie0_rc;
> +		pcieep0 = &pcie0_ep;
> +		pcierc1 = &pcie1_rc;
> +		pcieep1 = &pcie1_ep;
> +		pcierc2 = &pcie4_rc;
> +		pcieep2 = &pcie4_ep;

Since these are disabled, aliases do not belong to DTSI, but to board.

Also, explain why do you need them.

>  	};
>  
>  	cpus {
> @@ -860,6 +868,169 @@
>  			clocks = <&clock_fsys0 UFS0_MPHY_REFCLK_IXTAL26>;
>  			clock-names = "ref_clk";
>  		};
> +
> +		pcie_phy0: pcie-phy@15080000 {
> +			compatible = "tesla,fsd-pcie-phy";
> +			#phy-cells = <0>;
> +			reg = <0x0 0x15080000 0x0 0x2000>,
> +			      <0x0 0x150A0000 0x0 0x1000>;
> +			reg-names = "phy", "pcs";
> +			samsung,pmureg-phandle = <&pmu_system_controller>;
> +			tesla,pcie-sysreg = <&sysreg_fsys0>;
> +			phy-mode = <0>;
> +			status = "disabled";
> +		};
> +
> +		pcie_phy1: pcie-phy@16880000 {
> +			compatible = "tesla,fsd-pcie-phy";
> +			#phy-cells = <0>;
> +			reg = <0x0 0x16880000 0x0 0x2000>,
> +			      <0x0 0x16860000 0x0 0x1000>;
> +			reg-names = "phy", "pcs";
> +			samsung,pmureg-phandle = <&pmu_system_controller>;
> +			tesla,pcie-sysreg = <&sysreg_fsys1>;
> +			phy-mode = <0>;
> +			status = "disabled";
> +		};
> +
> +		pcie4_rc: pcie@15400000 {

Not ordered. Keep nodes sorted by unit address, at least within the
group of devices you add.

> +			compatible = "tesla,fsd-pcie";

reg is second property. reg-names and ranges follow.

> +			clocks = <&clock_fsys0 PCIE_SUBCTRL_INST0_AUX_CLK_SOC>,
> +				 <&clock_fsys0 PCIE_SUBCTRL_INST0_DBI_ACLK_SOC>,
> +				 <&clock_fsys0 PCIE_SUBCTRL_INST0_MSTR_ACLK_SOC>,
> +				 <&clock_fsys0 PCIE_SUBCTRL_INST0_SLV_ACLK_SOC>;
> +			clock-names = "aux_clk", "dbi_clk", "mstr_clk", "slv_clk";
> +			#address-cells = <3>;
> +			#size-cells = <2>;

Best regards,
Krzysztof