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@xxxxxxxxxxxxxx>
---
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.
+ };
};
