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

From: Krzysztof Kozlowski
Date: Mon Nov 21 2022 - 07:11:45 EST


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@xxxxxxxxxxx>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
> Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx>
> ---
> 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