Re: [PATCH v1 3/3] arm64: dts: qcom: x1e80100-qcp: Add power control and sideband signals for PCIe3

["Wenbin Yao (Consultant)" <quic_wenbyao@quicinc.com>]

On 3/21/2025 6:08 AM, Bryan O'Donoghue wrote:
> On 20/03/2025 05:55, Wenbin Yao wrote:
>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>
>> Add perst, wake and clkreq sideband signals and required regulators in
>> PCIe3 controller and PHY device tree node. Describe the voltage rails of
>> the x8 PCI slots for PCIe3 port.
>>
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/x1e80100-qcp.dts | 119 ++++++++++++++++++++++
>>   1 file changed, 119 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100-qcp.dts 
>> b/arch/arm64/boot/dts/qcom/x1e80100-qcp.dts
>> index 28086a2bc..9cd313802 100644
>> --- a/arch/arm64/boot/dts/qcom/x1e80100-qcp.dts
>> +++ b/arch/arm64/boot/dts/qcom/x1e80100-qcp.dts
>> @@ -318,6 +318,48 @@ vreg_wcn_3p3: regulator-wcn-3p3 {
>>           regulator-boot-on;
>>       };
>>
>> +    vreg_pcie_12v: regulator-pcie_12v {
>> +        compatible = "regulator-fixed";
>> +
>> +        regulator-name = "VREG_PCIE_12V";
>> +        regulator-min-microvolt = <12000000>;
>> +        regulator-max-microvolt = <12000000>;
>> +
>> +        gpio = <&pm8550ve_8_gpios 8 GPIO_ACTIVE_HIGH>;
>> +        enable-active-high;
>> +
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&pcie_x8_12v>;
>> +    };
>> +
>> +    vreg_pcie_3v3_aux: regulator-pcie_3v3_aux {
>> +        compatible = "regulator-fixed";
>> +
>> +        regulator-name = "VREG_PCIE_3P3_AUX";
>> +        regulator-min-microvolt = <3300000>;
>> +        regulator-max-microvolt = <3300000>;
>> +
>> +        gpio = <&pmc8380_3_gpios 8 GPIO_ACTIVE_HIGH>;
>> +        enable-active-high;
>> +
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&pm_sde7_aux_3p3_en>;
>> +    };
>> +
>> +    vreg_pcie_3v3: regulator-pcie_3v3 {
>> +        compatible = "regulator-fixed";
>> +
>> +        regulator-name = "VREG_PCIE_3P3";
>> +        regulator-min-microvolt = <3300000>;
>> +        regulator-max-microvolt = <3300000>;
>> +
>> +        gpio = <&pmc8380_3_gpios 6 GPIO_ACTIVE_HIGH>;
>> +        enable-active-high;
>> +
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&pm_sde7_main_3p3_en>;
>> +};
>> +
>>       usb-1-ss0-sbu-mux {
>>           compatible = "onnn,fsusb42", "gpio-sbu-mux";
>>
>> @@ -907,6 +949,60 @@ &mdss_dp3_phy {
>>       status = "okay";
>>   };
>>
>> +&pm8550ve_8_gpios {
>> +    pcie_x8_12v: pcie-12v-default-state {
>> +        pins = "gpio8";
>> +        function = "normal";
>> +        output-enable;
>> +        output-high;
>> +        bias-pull-down;
>> +        power-source = <0>;
>> +    };
>> +};
>> +
>> +&pmc8380_3_gpios {
>> +    pm_sde7_aux_3p3_en: pcie-aux-3p3-default-state {
>> +        pins = "gpio8";
>> +        function = "normal";
>> +        output-enable;
>> +        output-high;
>> +        bias-pull-down;
>> +        power-source = <0>;
>> +    };
>> +
>> +    pm_sde7_main_3p3_en: pcie-main-3p3-default-state {
>> +        pins = "gpio6";
>> +        function = "normal";
>> +        output-enable;
>> +        output-high;
>> +        bias-pull-down;
>> +        power-source = <0>;
>> +    };
>> +};
>> +
>> +&pcie3 {
>> +
>> +    pinctrl-names = "default";
>> +    pinctrl-0 = <&pcie3_default>;
>> +    perst-gpios = <&tlmm 143 GPIO_ACTIVE_LOW>;
>> +    wake-gpios = <&tlmm 145 GPIO_ACTIVE_LOW>;
>> +    status = "okay";
>> +};
>> +
>> +&pcie3_phy {
>> +    vdda-phy-supply = <&vreg_l3j_0p8>;
>> +    vdda-pll-supply = <&vreg_l3e_1p2>;
>> +    vdda-qref-supply = <&vreg_l3c_0p8>;
>> +
>> +    status = "okay";
>> +};
>> +
>> +&pcie3port {
>> +    vpcie12v-supply = <&vreg_pcie_12v>;
>> +    vpcie3v3-supply = <&vreg_pcie_3v3>;
>> +    vpcie3v3aux-supply = <&vreg_pcie_3v3_aux>;
>> +};
>> +
>>   &pcie4 {
>>       perst-gpios = <&tlmm 146 GPIO_ACTIVE_LOW>;
>>       wake-gpios = <&tlmm 148 GPIO_ACTIVE_LOW>;
>> @@ -1118,6 +1214,29 @@ nvme_reg_en: nvme-reg-en-state {
>>           bias-disable;
>>       };
>>
>> +    pcie3_default: pcie3-default-state {
>> +        clkreq-n-pins {
>> +            pins = "gpio144";
>> +            function = "pcie3_clk";
>> +            drive-strength = <2>;
>> +            bias-pull-up;
>> +        };
>> +
>> +        perst-n-pins {
>> +            pins = "gpio143";
>> +            function = "gpio";
>> +            drive-strength = <2>;
>> +            bias-pull-down;
>> +        };
>> +
>> +        wake-n-pins {
>> +               pins = "gpio145";
>> +               function = "gpio";
>> +               drive-strength = <2>;
>> +               bias-pull-up;
>> +        };
>> +    };
>> +
>>       pcie4_default: pcie4-default-state {
>>           clkreq-n-pins {
>>               pins = "gpio147";
>> -- 
>> 2.34.1
>>
>>
>
> Recommend breaking this patch into at least two patches @ the and
>
> -> Add power control
> -> Add sideband signals
>
> if your patch title requires an and its usually a good indicator of a 
> place to break that patch into different parts.

Will fix in the next version.

>
> ---
> bod

-- 
With best wishes
Wenbin