linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Add DT bindings and DT nodes for PCIe and PHY in SC7280
@ 2021-05-07 10:17 Prasad Malisetty
  2021-05-07 10:17 ` [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720 Prasad Malisetty
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Prasad Malisetty @ 2021-05-07 10:17 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Bjorn Helgaas
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	swboyd, dianders, mka, Prasad Malisetty

This series includes PCIe controller and phy binding updates
for SC7280 SoC and DT changes for SC7280 SoC and SC7280 IDP board.

Prasad Malisetty (3):
  dt-bindings: pci: qcom: Document PCIe bindings for SC720
  arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board

 .../devicetree/bindings/pci/qcom,pcie.txt          |  17 +++
 arch/arm64/boot/dts/qcom/sc7280-idp.dts            |  13 ++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 138 +++++++++++++++++++++
 3 files changed, 168 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720
  2021-05-07 10:17 [PATCH 0/3] Add DT bindings and DT nodes for PCIe and PHY in SC7280 Prasad Malisetty
@ 2021-05-07 10:17 ` Prasad Malisetty
  2021-05-07 19:59   ` Stephen Boyd
  2021-05-10 15:48   ` Rob Herring
  2021-05-07 10:17 ` [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes Prasad Malisetty
  2021-05-07 10:17 ` [PATCH 3/3] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board Prasad Malisetty
  2 siblings, 2 replies; 13+ messages in thread
From: Prasad Malisetty @ 2021-05-07 10:17 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Bjorn Helgaas
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	swboyd, dianders, mka, Prasad Malisetty

Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
to the one used on SM8250. Add the compatible for SC7280.

Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 0da458a..e5245ed 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -12,6 +12,7 @@
 			- "qcom,pcie-ipq4019" for ipq4019
 			- "qcom,pcie-ipq8074" for ipq8074
 			- "qcom,pcie-qcs404" for qcs404
+			- "qcom,pcie-sc7280" for sc7280
 			- "qcom,pcie-sdm845" for sdm845
 			- "qcom,pcie-sm8250" for sm8250
 
@@ -133,6 +134,22 @@
 			- "slave_bus"	AXI Slave clock
 
 - clock-names:
+	Usage: required for sc7280
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "aux"         Auxiliary clock
+			- "cfg"         Configuration clock
+			- "bus_master"  Master AXI clock
+			- "bus_slave"   Slave AXI clock
+			- "slave_q2a"   Slave Q2A clock
+			- "tbu"         PCIe TBU clock
+			- "ddrss_sf_tbu" PCIe SF TBU clock
+			- "pipe"        PIPE clock
+			- "pipe_src"    PIPE MUX
+			- "pipe_ext"    PIPE output clock
+			- "ref"         REFERENCE clock
+
+- clock-names:
 	Usage: required for sdm845
 	Value type: <stringlist>
 	Definition: Should contain the following entries
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  2021-05-07 10:17 [PATCH 0/3] Add DT bindings and DT nodes for PCIe and PHY in SC7280 Prasad Malisetty
  2021-05-07 10:17 ` [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720 Prasad Malisetty
@ 2021-05-07 10:17 ` Prasad Malisetty
  2021-05-07 20:06   ` Stephen Boyd
  2021-05-07 10:17 ` [PATCH 3/3] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board Prasad Malisetty
  2 siblings, 1 reply; 13+ messages in thread
From: Prasad Malisetty @ 2021-05-07 10:17 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Bjorn Helgaas
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	swboyd, dianders, mka, Prasad Malisetty

Add PCIe controller and PHY nodes for sc7280 SOC.

Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 138 +++++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 2cc4785..a9f25fc1 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -12,6 +12,7 @@
 #include <dt-bindings/power/qcom-aoss-qmp.h>
 #include <dt-bindings/power/qcom-rpmpd.h>
 #include <dt-bindings/soc/qcom,rpmh-rsc.h>
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	interrupt-parent = <&intc>;
@@ -316,6 +317,118 @@
 			};
 		};
 
+	pcie1: pci@1c08000 {
+			compatible = "qcom,pcie-sc7280", "qcom,pcie-sm8250", "snps,dw-pcie";
+			reg = <0 0x01c08000 0 0x3000>,
+			      <0 0x40000000 0 0xf1d>,
+			      <0 0x40000f20 0 0xa8>,
+			      <0 0x40001000 0 0x1000>,
+			      <0 0x40100000 0 0x100000>;
+
+			reg-names = "parf", "dbi", "elbi", "atu", "config";
+			device_type = "pci";
+			linux,pci-domain = <1>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <2>;
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			ranges = <0x01000000 0x0 0x40200000 0x0 0x40200000 0x0 0x100000>,
+				 <0x02000000 0x0 0x40300000 0x0 0x40300000 0x0 0x1fd00000>;
+
+			interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi";
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 434 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+					<0 0 0 2 &intc 0 435 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+					<0 0 0 3 &intc 0 438 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+					<0 0 0 4 &intc 0 439 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
+				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
+				 <&pcie1_lane 0>,
+				 <&rpmhcc RPMH_CXO_CLK>,
+				 <&gcc GCC_PCIE_1_AUX_CLK>,
+				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
+				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
+				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,
+				 <&gcc GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
+				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,
+				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;
+
+			clock-names = "pipe",
+				      "pipe_src",
+				      "pipe_ext",
+				      "ref",
+				      "aux",
+				      "cfg",
+				      "bus_master",
+				      "bus_slave",
+				      "slave_q2a",
+				      "tbu",
+				      "ddrss_sf_tbu";
+
+			assigned-clocks = <&gcc GCC_PCIE_1_AUX_CLK>;
+			assigned-clock-rates = <19200000>;
+
+			resets = <&gcc GCC_PCIE_1_BCR>;
+			reset-names = "pci";
+
+			power-domains = <&gcc GCC_PCIE_1_GDSC>;
+
+			phys = <&pcie1_lane>;
+			phy-names = "pciephy";
+
+			perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pcie1_default_state>;
+
+			iommus = <&apps_smmu 0x1c80 0x1>;
+
+			iommu-map = <0x0 &apps_smmu 0x1c80 0x1>,
+				    <0x100 &apps_smmu 0x1c81 0x1>;
+
+			status = "disabled";
+		};
+
+		pcie1_phy: phy@1c0e000 {
+			compatible = "qcom,sm8250-qmp-gen3x2-pcie-phy";
+			reg = <0 0x01c0e000 0 0x1c0>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+			clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
+				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
+				 <&gcc GCC_PCIE_CLKREF_EN>,
+				 <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
+			clock-names = "aux", "cfg_ahb", "ref", "refgen";
+
+			resets = <&gcc GCC_PCIE_1_PHY_BCR>;
+			reset-names = "phy";
+
+			assigned-clocks = <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
+			assigned-clock-rates = <100000000>;
+
+			status = "disabled";
+
+			pcie1_lane: lanes@1c0e200 {
+				reg = <0 0x1c0e200 0 0x170>, /* tx0 */
+				      <0 0x1c0e400 0 0x200>, /* rx0 */
+				      <0 0x1c0ea00 0 0x1f0>, /* pcs */
+				      <0 0x1c0e600 0 0x170>, /* tx1 */
+				      <0 0x1c0e800 0 0x200>, /* rx1 */
+				      <0 0x1c0ee00 0 0xf4>; /* "pcs_com" same as pcs_misc? */
+				clocks = <&rpmhcc RPMH_CXO_CLK>;
+				clock-names = "pipe0";
+
+				#phy-cells = <0>;
+				#clock-cells = <1>;
+				clock-output-names = "pcie_1_pipe_clk";
+			};
+		};
+
 		stm@6002000 {
 			compatible = "arm,coresight-stm", "arm,primecell";
 			reg = <0 0x06002000 0 0x1000>,
@@ -871,6 +984,31 @@
 				pins = "gpio46", "gpio47";
 				function = "qup13";
 			};
+
+			pcie1_default_state: pcie1-default {
+				clkreq {
+					pins = "gpio79";
+					function = "pcie1_clkreqn";
+					bias-pull-up;
+				};
+
+				reset-n {
+					pins = "gpio2";
+					function = "gpio";
+
+					drive-strength = <16>;
+					output-low;
+					bias-disable;
+				};
+
+				wake-n {
+					pins = "gpio3";
+					function = "gpio";
+
+					drive-strength = <2>;
+					bias-pull-up;
+				};
+			};
 		};
 
 		apps_smmu: iommu@15000000 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
  2021-05-07 10:17 [PATCH 0/3] Add DT bindings and DT nodes for PCIe and PHY in SC7280 Prasad Malisetty
  2021-05-07 10:17 ` [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720 Prasad Malisetty
  2021-05-07 10:17 ` [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes Prasad Malisetty
@ 2021-05-07 10:17 ` Prasad Malisetty
  2 siblings, 0 replies; 13+ messages in thread
From: Prasad Malisetty @ 2021-05-07 10:17 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Bjorn Helgaas
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	swboyd, dianders, mka, Prasad Malisetty

Add PCIe nodes for sc7280 IDP board.

Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
index 54d2cb3..9105a74 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
@@ -234,6 +234,19 @@
 	};
 };
 
+&pcie1 {
+	status = "okay";
+
+	vdda-supply = <&vreg_l10c_0p8>;
+};
+
+&pcie1_phy {
+	status = "okay";
+
+	vdda-phy-supply = <&vreg_l10c_0p8>;
+	vdda-pll-supply = <&vreg_l6b_1p2>;
+};
+
 &qupv3_id_0 {
 	status = "okay";
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720
  2021-05-07 10:17 ` [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720 Prasad Malisetty
@ 2021-05-07 19:59   ` Stephen Boyd
  2021-06-04 11:26     ` Prasad Malisetty
  2021-05-10 15:48   ` Rob Herring
  1 sibling, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2021-05-07 19:59 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Bjorn Helgaas, Prasad Malisetty,
	Rob Herring
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	dianders, mka

Quoting Prasad Malisetty (2021-05-07 03:17:26)
> Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
> to the one used on SM8250. Add the compatible for SC7280.
>
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 0da458a..e5245ed 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -12,6 +12,7 @@
>                         - "qcom,pcie-ipq4019" for ipq4019
>                         - "qcom,pcie-ipq8074" for ipq8074
>                         - "qcom,pcie-qcs404" for qcs404
> +                       - "qcom,pcie-sc7280" for sc7280
>                         - "qcom,pcie-sdm845" for sdm845
>                         - "qcom,pcie-sm8250" for sm8250
>
> @@ -133,6 +134,22 @@
>                         - "slave_bus"   AXI Slave clock
>
>  - clock-names:
> +       Usage: required for sc7280
> +       Value type: <stringlist>
> +       Definition: Should contain the following entries
> +                       - "aux"         Auxiliary clock
> +                       - "cfg"         Configuration clock
> +                       - "bus_master"  Master AXI clock
> +                       - "bus_slave"   Slave AXI clock
> +                       - "slave_q2a"   Slave Q2A clock
> +                       - "tbu"         PCIe TBU clock
> +                       - "ddrss_sf_tbu" PCIe SF TBU clock
> +                       - "pipe"        PIPE clock
> +                       - "pipe_src"    PIPE MUX

Is pipe_src necessary? Is it the parent of the pipe clk? If so, please
remove it and do whatever is necessary on the pipe clk instead of the
parent of the clk.

> +                       - "pipe_ext"    PIPE output clock

Is pipe output different from pipe?

> +                       - "ref"         REFERENCE clock
> +
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  2021-05-07 10:17 ` [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes Prasad Malisetty
@ 2021-05-07 20:06   ` Stephen Boyd
  2021-05-21  9:57     ` Prasad Malisetty
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2021-05-07 20:06 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Bjorn Helgaas, Prasad Malisetty,
	Rob Herring
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	dianders, mka

Quoting Prasad Malisetty (2021-05-07 03:17:27)
> Add PCIe controller and PHY nodes for sc7280 SOC.
>
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138 +++++++++++++++++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 2cc4785..a9f25fc1 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -12,6 +12,7 @@
>  #include <dt-bindings/power/qcom-aoss-qmp.h>
>  #include <dt-bindings/power/qcom-rpmpd.h>
>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
> +#include <dt-bindings/gpio/gpio.h>
>
>  / {
>         interrupt-parent = <&intc>;
> @@ -316,6 +317,118 @@
>                         };
>                 };
>
[...]
> +
> +               pcie1_phy: phy@1c0e000 {
> +                       compatible = "qcom,sm8250-qmp-gen3x2-pcie-phy";
> +                       reg = <0 0x01c0e000 0 0x1c0>;
> +                       #address-cells = <2>;
> +                       #size-cells = <2>;
> +                       ranges;
> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> +                                <&gcc GCC_PCIE_CLKREF_EN>,
> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> +                       clock-names = "aux", "cfg_ahb", "ref", "refgen";
> +
> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
> +                       reset-names = "phy";
> +
> +                       assigned-clocks = <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> +                       assigned-clock-rates = <100000000>;
> +
> +                       status = "disabled";

I think the style is to put status disabled close to the compatible?

> +
> +                       pcie1_lane: lanes@1c0e200 {
> +                               reg = <0 0x1c0e200 0 0x170>, /* tx0 */

Please pad reg addresses to 8 characters.

> +                                     <0 0x1c0e400 0 0x200>, /* rx0 */
> +                                     <0 0x1c0ea00 0 0x1f0>, /* pcs */
> +                                     <0 0x1c0e600 0 0x170>, /* tx1 */
> +                                     <0 0x1c0e800 0 0x200>, /* rx1 */
> +                                     <0 0x1c0ee00 0 0xf4>; /* "pcs_com" same as pcs_misc? */

Is this a TODO? I'd prefer all the comments on the reg properties to be
removed.

> +                               clocks = <&rpmhcc RPMH_CXO_CLK>;
> +                               clock-names = "pipe0";
> +
> +                               #phy-cells = <0>;
> +                               #clock-cells = <1>;
> +                               clock-output-names = "pcie_1_pipe_clk";
> +                       };
> +               };
> +
>                 stm@6002000 {
>                         compatible = "arm,coresight-stm", "arm,primecell";
>                         reg = <0 0x06002000 0 0x1000>,
> @@ -871,6 +984,31 @@
>                                 pins = "gpio46", "gpio47";
>                                 function = "qup13";
>                         };
> +
> +                       pcie1_default_state: pcie1-default {
> +                               clkreq {
> +                                       pins = "gpio79";
> +                                       function = "pcie1_clkreqn";
> +                                       bias-pull-up;

Move this bias-pull-up to the idp file?

> +                               };
> +
> +                               reset-n {
> +                                       pins = "gpio2";
> +                                       function = "gpio";
> +
> +                                       drive-strength = <16>;
> +                                       output-low;
> +                                       bias-disable;
> +                               };
> +
> +                               wake-n {
> +                                       pins = "gpio3";
> +                                       function = "gpio";
> +
> +                                       drive-strength = <2>;
> +                                       bias-pull-up;
> +                               };

These last two nodes with the pull-up and drive-strength settings should
be in the board files, like the idp one, instead of here in the SoC
file. That way board designers can take the SoC and connect the pcie to
an external device using these pins and set the configuration they want
on these pins, or choose not to connect them to the SoC at all and use
those pins for something else.

In addition, it looks like the reset could be a reset-gpios property
instead of an output-low config.

> +                       };
>                 };
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720
  2021-05-07 10:17 ` [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720 Prasad Malisetty
  2021-05-07 19:59   ` Stephen Boyd
@ 2021-05-10 15:48   ` Rob Herring
  1 sibling, 0 replies; 13+ messages in thread
From: Rob Herring @ 2021-05-10 15:48 UTC (permalink / raw)
  To: Prasad Malisetty
  Cc: Rob Herring, mka, Bjorn Helgaas, swboyd, linux-arm-msm,
	linux-kernel, devicetree, Andy Gross, linux-pci, mgautam,
	dianders, Bjorn Andersson

On Fri, 07 May 2021 15:47:26 +0530, Prasad Malisetty wrote:
> Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
> to the one used on SM8250. Add the compatible for SC7280.
> 
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  2021-05-07 20:06   ` Stephen Boyd
@ 2021-05-21  9:57     ` Prasad Malisetty
  2021-06-04 21:43       ` Stephen Boyd
  0 siblings, 1 reply; 13+ messages in thread
From: Prasad Malisetty @ 2021-05-21  9:57 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Bjorn Andersson, Bjorn Helgaas, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	dianders, mka

On 2021-05-08 01:36, Stephen Boyd wrote:
> Quoting Prasad Malisetty (2021-05-07 03:17:27)
>> Add PCIe controller and PHY nodes for sc7280 SOC.
>> 
>> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138 
>> +++++++++++++++++++++++++++++++++++
>>  1 file changed, 138 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 2cc4785..a9f25fc1 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -12,6 +12,7 @@
>>  #include <dt-bindings/power/qcom-aoss-qmp.h>
>>  #include <dt-bindings/power/qcom-rpmpd.h>
>>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> 
>>  / {
>>         interrupt-parent = <&intc>;
>> @@ -316,6 +317,118 @@
>>                         };
>>                 };
>> 
> [...]
>> +
>> +               pcie1_phy: phy@1c0e000 {
>> +                       compatible = 
>> "qcom,sm8250-qmp-gen3x2-pcie-phy";
>> +                       reg = <0 0x01c0e000 0 0x1c0>;
>> +                       #address-cells = <2>;
>> +                       #size-cells = <2>;
>> +                       ranges;
>> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
>> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
>> +                                <&gcc GCC_PCIE_CLKREF_EN>,
>> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
>> +                       clock-names = "aux", "cfg_ahb", "ref", 
>> "refgen";
>> +
>> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
>> +                       reset-names = "phy";
>> +
>> +                       assigned-clocks = <&gcc 
>> GCC_PCIE1_PHY_RCHNG_CLK>;
>> +                       assigned-clock-rates = <100000000>;
>> +
>> +                       status = "disabled";
> 
> I think the style is to put status disabled close to the compatible?

Generally I have added status disabled in end as like many nodes. just 
curious to ask is there any specific reason to put close to compatible.
> 
>> +
>> +                       pcie1_lane: lanes@1c0e200 {
>> +                               reg = <0 0x1c0e200 0 0x170>, /* tx0 */
> 
> Please pad reg addresses to 8 characters.

Done
> 
>> +                                     <0 0x1c0e400 0 0x200>, /* rx0 */
>> +                                     <0 0x1c0ea00 0 0x1f0>, /* pcs */
>> +                                     <0 0x1c0e600 0 0x170>, /* tx1 */
>> +                                     <0 0x1c0e800 0 0x200>, /* rx1 */
>> +                                     <0 0x1c0ee00 0 0xf4>; /* 
>> "pcs_com" same as pcs_misc? */
> 
> Is this a TODO? I'd prefer all the comments on the reg properties to be
> removed.
> 
Done
>> +                               clocks = <&rpmhcc RPMH_CXO_CLK>;
>> +                               clock-names = "pipe0";
>> +
>> +                               #phy-cells = <0>;
>> +                               #clock-cells = <1>;
>> +                               clock-output-names = 
>> "pcie_1_pipe_clk";
>> +                       };
>> +               };
>> +
>>                 stm@6002000 {
>>                         compatible = "arm,coresight-stm", 
>> "arm,primecell";
>>                         reg = <0 0x06002000 0 0x1000>,
>> @@ -871,6 +984,31 @@
>>                                 pins = "gpio46", "gpio47";
>>                                 function = "qup13";
>>                         };
>> +
>> +                       pcie1_default_state: pcie1-default {
>> +                               clkreq {
>> +                                       pins = "gpio79";
>> +                                       function = "pcie1_clkreqn";
>> +                                       bias-pull-up;
> 
> Move this bias-pull-up to the idp file?

Done
> 
>> +                               };
>> +
>> +                               reset-n {
>> +                                       pins = "gpio2";
>> +                                       function = "gpio";
>> +
>> +                                       drive-strength = <16>;
>> +                                       output-low;
>> +                                       bias-disable;
>> +                               };
>> +
>> +                               wake-n {
>> +                                       pins = "gpio3";
>> +                                       function = "gpio";
>> +
>> +                                       drive-strength = <2>;
>> +                                       bias-pull-up;
>> +                               };
> 
> These last two nodes with the pull-up and drive-strength settings 
> should
> be in the board files, like the idp one, instead of here in the SoC
> file. That way board designers can take the SoC and connect the pcie to
> an external device using these pins and set the configuration they want
> on these pins, or choose not to connect them to the SoC at all and use
> those pins for something else.
> 
> In addition, it looks like the reset could be a reset-gpios property
> instead of an output-low config.
> 
we are using reset property as perst gpio in pcie node.
>> +                       };
>>                 };
>> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720
  2021-05-07 19:59   ` Stephen Boyd
@ 2021-06-04 11:26     ` Prasad Malisetty
  2021-06-04 21:44       ` Stephen Boyd
  0 siblings, 1 reply; 13+ messages in thread
From: Prasad Malisetty @ 2021-06-04 11:26 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Bjorn Andersson, Bjorn Helgaas, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	dianders, mka

On 2021-05-08 01:29, Stephen Boyd wrote:
> Quoting Prasad Malisetty (2021-05-07 03:17:26)
>> Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
>> to the one used on SM8250. Add the compatible for SC7280.
>> 
>> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17 
>> +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt 
>> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>> index 0da458a..e5245ed 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>> @@ -12,6 +12,7 @@
>>                         - "qcom,pcie-ipq4019" for ipq4019
>>                         - "qcom,pcie-ipq8074" for ipq8074
>>                         - "qcom,pcie-qcs404" for qcs404
>> +                       - "qcom,pcie-sc7280" for sc7280
>>                         - "qcom,pcie-sdm845" for sdm845
>>                         - "qcom,pcie-sm8250" for sm8250
>> 
>> @@ -133,6 +134,22 @@
>>                         - "slave_bus"   AXI Slave clock
>> 
>>  - clock-names:
>> +       Usage: required for sc7280
>> +       Value type: <stringlist>
>> +       Definition: Should contain the following entries
>> +                       - "aux"         Auxiliary clock
>> +                       - "cfg"         Configuration clock
>> +                       - "bus_master"  Master AXI clock
>> +                       - "bus_slave"   Slave AXI clock
>> +                       - "slave_q2a"   Slave Q2A clock
>> +                       - "tbu"         PCIe TBU clock
>> +                       - "ddrss_sf_tbu" PCIe SF TBU clock
>> +                       - "pipe"        PIPE clock
>> +                       - "pipe_src"    PIPE MUX
> 
> Is pipe_src necessary? Is it the parent of the pipe clk? If so, please
> remove it and do whatever is necessary on the pipe clk instead of the
> parent of the clk.

Here pipe_src is MUX. Newer targets require changing pipe-clk mux to 
switch between pipe_clk and XO for GDSC enable.
After PHY init, need to configure MUX.
> 
>> +                       - "pipe_ext"    PIPE output clock
> 
> Is pipe output different from pipe?
> 
Yes, pipe_ext clock will generate after PHY init.
>> +                       - "ref"         REFERENCE clock
>> +
>> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  2021-05-21  9:57     ` Prasad Malisetty
@ 2021-06-04 21:43       ` Stephen Boyd
  2021-06-06  4:02         ` Bjorn Andersson
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2021-06-04 21:43 UTC (permalink / raw)
  To: Prasad Malisetty
  Cc: Andy Gross, Bjorn Andersson, Bjorn Helgaas, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	dianders, mka

Quoting Prasad Malisetty (2021-05-21 02:57:00)
> On 2021-05-08 01:36, Stephen Boyd wrote:
> > Quoting Prasad Malisetty (2021-05-07 03:17:27)
> >> Add PCIe controller and PHY nodes for sc7280 SOC.
> >>
> >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> >> ---
> >>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138
> >> +++++++++++++++++++++++++++++++++++
> >>  1 file changed, 138 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> index 2cc4785..a9f25fc1 100644
> >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> @@ -12,6 +12,7 @@
> >>  #include <dt-bindings/power/qcom-aoss-qmp.h>
> >>  #include <dt-bindings/power/qcom-rpmpd.h>
> >>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
> >> +#include <dt-bindings/gpio/gpio.h>
> >>
> >>  / {
> >>         interrupt-parent = <&intc>;
> >> @@ -316,6 +317,118 @@
> >>                         };
> >>                 };
> >>
> > [...]
> >> +
> >> +               pcie1_phy: phy@1c0e000 {
> >> +                       compatible =
> >> "qcom,sm8250-qmp-gen3x2-pcie-phy";
> >> +                       reg = <0 0x01c0e000 0 0x1c0>;
> >> +                       #address-cells = <2>;
> >> +                       #size-cells = <2>;
> >> +                       ranges;
> >> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> >> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> >> +                                <&gcc GCC_PCIE_CLKREF_EN>,
> >> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> >> +                       clock-names = "aux", "cfg_ahb", "ref",
> >> "refgen";
> >> +
> >> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
> >> +                       reset-names = "phy";
> >> +
> >> +                       assigned-clocks = <&gcc
> >> GCC_PCIE1_PHY_RCHNG_CLK>;
> >> +                       assigned-clock-rates = <100000000>;
> >> +
> >> +                       status = "disabled";
> >
> > I think the style is to put status disabled close to the compatible?
>
> Generally I have added status disabled in end as like many nodes. just
> curious to ask is there any specific reason to put close to compatible.

It's really up to qcom maintainers, which I am not.

> >> +                               };
> >> +
> >> +                               reset-n {
> >> +                                       pins = "gpio2";
> >> +                                       function = "gpio";
> >> +
> >> +                                       drive-strength = <16>;
> >> +                                       output-low;
> >> +                                       bias-disable;
> >> +                               };
> >> +
> >> +                               wake-n {
> >> +                                       pins = "gpio3";
> >> +                                       function = "gpio";
> >> +
> >> +                                       drive-strength = <2>;
> >> +                                       bias-pull-up;
> >> +                               };
> >
> > These last two nodes with the pull-up and drive-strength settings
> > should
> > be in the board files, like the idp one, instead of here in the SoC
> > file. That way board designers can take the SoC and connect the pcie to
> > an external device using these pins and set the configuration they want
> > on these pins, or choose not to connect them to the SoC at all and use
> > those pins for something else.
> >
> > In addition, it looks like the reset could be a reset-gpios property
> > instead of an output-low config.
> >
> we are using reset property as perst gpio in pcie node.

Ok, perst-gpios should be fine. Presumably perst-gpios should be in the
board and not in the SoC because of what I wrote up above.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720
  2021-06-04 11:26     ` Prasad Malisetty
@ 2021-06-04 21:44       ` Stephen Boyd
  0 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2021-06-04 21:44 UTC (permalink / raw)
  To: Prasad Malisetty
  Cc: Andy Gross, Bjorn Andersson, Bjorn Helgaas, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	dianders, mka

Quoting Prasad Malisetty (2021-06-04 04:26:57)
> On 2021-05-08 01:29, Stephen Boyd wrote:
> > Quoting Prasad Malisetty (2021-05-07 03:17:26)
> >> Document the PCIe DT bindings for SC7280 SoC.The PCIe IP is similar
> >> to the one used on SM8250. Add the compatible for SC7280.
> >>
> >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> >> ---
> >>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 17
> >> +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >> index 0da458a..e5245ed 100644
> >> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >> @@ -12,6 +12,7 @@
> >>                         - "qcom,pcie-ipq4019" for ipq4019
> >>                         - "qcom,pcie-ipq8074" for ipq8074
> >>                         - "qcom,pcie-qcs404" for qcs404
> >> +                       - "qcom,pcie-sc7280" for sc7280
> >>                         - "qcom,pcie-sdm845" for sdm845
> >>                         - "qcom,pcie-sm8250" for sm8250
> >>
> >> @@ -133,6 +134,22 @@
> >>                         - "slave_bus"   AXI Slave clock
> >>
> >>  - clock-names:
> >> +       Usage: required for sc7280
> >> +       Value type: <stringlist>
> >> +       Definition: Should contain the following entries
> >> +                       - "aux"         Auxiliary clock
> >> +                       - "cfg"         Configuration clock
> >> +                       - "bus_master"  Master AXI clock
> >> +                       - "bus_slave"   Slave AXI clock
> >> +                       - "slave_q2a"   Slave Q2A clock
> >> +                       - "tbu"         PCIe TBU clock
> >> +                       - "ddrss_sf_tbu" PCIe SF TBU clock
> >> +                       - "pipe"        PIPE clock
> >> +                       - "pipe_src"    PIPE MUX
> >
> > Is pipe_src necessary? Is it the parent of the pipe clk? If so, please
> > remove it and do whatever is necessary on the pipe clk instead of the
> > parent of the clk.
>
> Here pipe_src is MUX. Newer targets require changing pipe-clk mux to
> switch between pipe_clk and XO for GDSC enable.
> After PHY init, need to configure MUX.

Ok. I see, so we have to change the parent of the parent of the pipe
clk?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  2021-06-04 21:43       ` Stephen Boyd
@ 2021-06-06  4:02         ` Bjorn Andersson
  2021-06-15  5:26           ` Prasad Malisetty
  0 siblings, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2021-06-06  4:02 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Prasad Malisetty, Andy Gross, Bjorn Helgaas, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	dianders, mka

On Fri 04 Jun 16:43 CDT 2021, Stephen Boyd wrote:

> Quoting Prasad Malisetty (2021-05-21 02:57:00)
> > On 2021-05-08 01:36, Stephen Boyd wrote:
> > > Quoting Prasad Malisetty (2021-05-07 03:17:27)
> > >> Add PCIe controller and PHY nodes for sc7280 SOC.
> > >>
> > >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> > >> ---
> > >>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138
> > >> +++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 138 insertions(+)
> > >>
> > >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > >> index 2cc4785..a9f25fc1 100644
> > >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > >> @@ -12,6 +12,7 @@
> > >>  #include <dt-bindings/power/qcom-aoss-qmp.h>
> > >>  #include <dt-bindings/power/qcom-rpmpd.h>
> > >>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
> > >> +#include <dt-bindings/gpio/gpio.h>
> > >>
> > >>  / {
> > >>         interrupt-parent = <&intc>;
> > >> @@ -316,6 +317,118 @@
> > >>                         };
> > >>                 };
> > >>
> > > [...]
> > >> +
> > >> +               pcie1_phy: phy@1c0e000 {
> > >> +                       compatible =
> > >> "qcom,sm8250-qmp-gen3x2-pcie-phy";
> > >> +                       reg = <0 0x01c0e000 0 0x1c0>;
> > >> +                       #address-cells = <2>;
> > >> +                       #size-cells = <2>;
> > >> +                       ranges;
> > >> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> > >> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> > >> +                                <&gcc GCC_PCIE_CLKREF_EN>,
> > >> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> > >> +                       clock-names = "aux", "cfg_ahb", "ref",
> > >> "refgen";
> > >> +
> > >> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
> > >> +                       reset-names = "phy";
> > >> +
> > >> +                       assigned-clocks = <&gcc
> > >> GCC_PCIE1_PHY_RCHNG_CLK>;
> > >> +                       assigned-clock-rates = <100000000>;
> > >> +
> > >> +                       status = "disabled";
> > >
> > > I think the style is to put status disabled close to the compatible?
> >
> > Generally I have added status disabled in end as like many nodes. just
> > curious to ask is there any specific reason to put close to compatible.
> 
> It's really up to qcom maintainers, which I am not.
> 

I like when it's the last item, as it lends itself nicely to be
surrounded by empty lines and thereby easy to spot...

Regards,
Bjorn

> > >> +                               };
> > >> +
> > >> +                               reset-n {
> > >> +                                       pins = "gpio2";
> > >> +                                       function = "gpio";
> > >> +
> > >> +                                       drive-strength = <16>;
> > >> +                                       output-low;
> > >> +                                       bias-disable;
> > >> +                               };
> > >> +
> > >> +                               wake-n {
> > >> +                                       pins = "gpio3";
> > >> +                                       function = "gpio";
> > >> +
> > >> +                                       drive-strength = <2>;
> > >> +                                       bias-pull-up;
> > >> +                               };
> > >
> > > These last two nodes with the pull-up and drive-strength settings
> > > should
> > > be in the board files, like the idp one, instead of here in the SoC
> > > file. That way board designers can take the SoC and connect the pcie to
> > > an external device using these pins and set the configuration they want
> > > on these pins, or choose not to connect them to the SoC at all and use
> > > those pins for something else.
> > >
> > > In addition, it looks like the reset could be a reset-gpios property
> > > instead of an output-low config.
> > >
> > we are using reset property as perst gpio in pcie node.
> 
> Ok, perst-gpios should be fine. Presumably perst-gpios should be in the
> board and not in the SoC because of what I wrote up above.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  2021-06-06  4:02         ` Bjorn Andersson
@ 2021-06-15  5:26           ` Prasad Malisetty
  0 siblings, 0 replies; 13+ messages in thread
From: Prasad Malisetty @ 2021-06-15  5:26 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Stephen Boyd, Andy Gross, Bjorn Helgaas, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, linux-pci, mgautam,
	dianders, mka

On 2021-06-06 09:32, Bjorn Andersson wrote:
> On Fri 04 Jun 16:43 CDT 2021, Stephen Boyd wrote:
> 
>> Quoting Prasad Malisetty (2021-05-21 02:57:00)
>> > On 2021-05-08 01:36, Stephen Boyd wrote:
>> > > Quoting Prasad Malisetty (2021-05-07 03:17:27)
>> > >> Add PCIe controller and PHY nodes for sc7280 SOC.
>> > >>
>> > >> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> > >> ---
>> > >>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138
>> > >> +++++++++++++++++++++++++++++++++++
>> > >>  1 file changed, 138 insertions(+)
>> > >>
>> > >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> > >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> > >> index 2cc4785..a9f25fc1 100644
>> > >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> > >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> > >> @@ -12,6 +12,7 @@
>> > >>  #include <dt-bindings/power/qcom-aoss-qmp.h>
>> > >>  #include <dt-bindings/power/qcom-rpmpd.h>
>> > >>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
>> > >> +#include <dt-bindings/gpio/gpio.h>
>> > >>
>> > >>  / {
>> > >>         interrupt-parent = <&intc>;
>> > >> @@ -316,6 +317,118 @@
>> > >>                         };
>> > >>                 };
>> > >>
>> > > [...]
>> > >> +
>> > >> +               pcie1_phy: phy@1c0e000 {
>> > >> +                       compatible =
>> > >> "qcom,sm8250-qmp-gen3x2-pcie-phy";
>> > >> +                       reg = <0 0x01c0e000 0 0x1c0>;
>> > >> +                       #address-cells = <2>;
>> > >> +                       #size-cells = <2>;
>> > >> +                       ranges;
>> > >> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
>> > >> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
>> > >> +                                <&gcc GCC_PCIE_CLKREF_EN>,
>> > >> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
>> > >> +                       clock-names = "aux", "cfg_ahb", "ref",
>> > >> "refgen";
>> > >> +
>> > >> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
>> > >> +                       reset-names = "phy";
>> > >> +
>> > >> +                       assigned-clocks = <&gcc
>> > >> GCC_PCIE1_PHY_RCHNG_CLK>;
>> > >> +                       assigned-clock-rates = <100000000>;
>> > >> +
>> > >> +                       status = "disabled";
>> > >
>> > > I think the style is to put status disabled close to the compatible?
>> >
>> > Generally I have added status disabled in end as like many nodes. just
>> > curious to ask is there any specific reason to put close to compatible.
>> 
>> It's really up to qcom maintainers, which I am not.
>> 
> 
> I like when it's the last item, as it lends itself nicely to be
> surrounded by empty lines and thereby easy to spot...
> 
>> Sure, I will change as like previous one.

> Regards,
> Bjorn
> 
>> > >> +                               };
>> > >> +
>> > >> +                               reset-n {
>> > >> +                                       pins = "gpio2";
>> > >> +                                       function = "gpio";
>> > >> +
>> > >> +                                       drive-strength = <16>;
>> > >> +                                       output-low;
>> > >> +                                       bias-disable;
>> > >> +                               };
>> > >> +
>> > >> +                               wake-n {
>> > >> +                                       pins = "gpio3";
>> > >> +                                       function = "gpio";
>> > >> +
>> > >> +                                       drive-strength = <2>;
>> > >> +                                       bias-pull-up;
>> > >> +                               };
>> > >
>> > > These last two nodes with the pull-up and drive-strength settings
>> > > should
>> > > be in the board files, like the idp one, instead of here in the SoC
>> > > file. That way board designers can take the SoC and connect the pcie to
>> > > an external device using these pins and set the configuration they want
>> > > on these pins, or choose not to connect them to the SoC at all and use
>> > > those pins for something else.
>> > >
>> > > In addition, it looks like the reset could be a reset-gpios property
>> > > instead of an output-low config.
>> > >
>> > we are using reset property as perst gpio in pcie node.
>> 
>> Ok, perst-gpios should be fine. Presumably perst-gpios should be in 
>> the
>> board and not in the SoC because of what I wrote up above.

>> Sure, I will move perst into board specific file

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-06-15  5:26 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07 10:17 [PATCH 0/3] Add DT bindings and DT nodes for PCIe and PHY in SC7280 Prasad Malisetty
2021-05-07 10:17 ` [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720 Prasad Malisetty
2021-05-07 19:59   ` Stephen Boyd
2021-06-04 11:26     ` Prasad Malisetty
2021-06-04 21:44       ` Stephen Boyd
2021-05-10 15:48   ` Rob Herring
2021-05-07 10:17 ` [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes Prasad Malisetty
2021-05-07 20:06   ` Stephen Boyd
2021-05-21  9:57     ` Prasad Malisetty
2021-06-04 21:43       ` Stephen Boyd
2021-06-06  4:02         ` Bjorn Andersson
2021-06-15  5:26           ` Prasad Malisetty
2021-05-07 10:17 ` [PATCH 3/3] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board Prasad Malisetty

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).