[PATCH v1 0/3] arm64: qcom: x1e80100-qcp: Add power supply and sideband signals config for PCIe3

[PATCH v1 0/3] arm64: qcom: x1e80100-qcp: Add power supply and sideband signals config for PCIe3

[Wenbin Yao]

The first patch enable the PCI Power Control driver to control the power
state of PCI slots. The second patch add the bus topology of PCIe domain 3
on x1e80100 platform. The third patch add perst, wake and clkreq sideband
signals, and describe the regulators powering the rails of the PCI slots in
the devicetree for PCIe3 controller and PHY device.

Qiang Yu (3):
  arm64: defconfig: enable PCI Power Control for PCIe3
  arm64: dts: qcom: x1e80100: add bus topology for PCIe domain 3
  arm64: dts: qcom: x1e80100-qcp: Add power control and sideband signals
    for PCIe3

 arch/arm64/boot/dts/qcom/x1e80100-qcp.dts | 119 ++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/x1e80100.dtsi    |  10 ++
 arch/arm64/configs/defconfig              |   1 +
 3 files changed, 130 insertions(+)


base-commit: 0a2f889128969dab41861b6e40111aa03dc57014
-- 
2.34.1

[PATCH v1 1/3] arm64: defconfig: enable PCI Power Control for PCIe3

[Wenbin Yao]

From: Qiang Yu <quic_qianyu@quicinc.com>

Enable the pwrctrl driver, which is utilized to manage the power supplies
of the devices connected to the PCI slots. This ensures that the voltage
rails of the x8 PCI slots on the X1E80100 - QCP can be correctly turned
on/off if they are described under PCIe port device tree node.

Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 85ec2fba1..de86d1121 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -245,6 +245,7 @@ CONFIG_PCIE_LAYERSCAPE_GEN4=y
 CONFIG_PCI_ENDPOINT=y
 CONFIG_PCI_ENDPOINT_CONFIGFS=y
 CONFIG_PCI_EPF_TEST=m
+CONFIG_PCI_PWRCTL_SLOT=y
 CONFIG_DEVTMPFS=y
 CONFIG_DEVTMPFS_MOUNT=y
 CONFIG_FW_LOADER_USER_HELPER=y
-- 
2.34.1

[PATCH v1 2/3] arm64: dts: qcom: x1e80100: add bus topology for PCIe domain 3

[Wenbin Yao]

From: Qiang Yu <quic_qianyu@quicinc.com>

Add pcie3port node to represent the PCIe bridge of PCIe3 so that PCI slot
voltage rails can be described under this node in the board's dts.

Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
---
 arch/arm64/boot/dts/qcom/x1e80100.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index 46b79fce9..32e8d400a 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -3287,6 +3287,16 @@ opp-128000000 {
 					opp-peak-kBps = <15753000 1>;
 				};
 			};
+			pcie3port: pcie@0 {
+				device_type = "pci";
+				compatible = "pciclass,0604";
+				reg = <0x0 0x0 0x0 0x0 0x0>;
+				bus-range = <0x01 0xff>;
+
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+			};
 		};
 
 		pcie3_phy: phy@1be0000 {
-- 
2.34.1

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

[Wenbin Yao]

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

Re: [PATCH v1 1/3] arm64: defconfig: enable PCI Power Control for PCIe3

[Bryan O'Donoghue]

On 20/03/2025 05:55, Wenbin Yao wrote:
> From: Qiang Yu <quic_qianyu@quicinc.com>
> 
> Enable the pwrctrl driver, which is utilized to manage the power supplies
> of the devices connected to the PCI slots. This ensures that the voltage
> rails of the x8 PCI slots on the X1E80100 - QCP can be correctly turned
> on/off if they are described under PCIe port device tree node.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
> ---
>   arch/arm64/configs/defconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 85ec2fba1..de86d1121 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -245,6 +245,7 @@ CONFIG_PCIE_LAYERSCAPE_GEN4=y
>   CONFIG_PCI_ENDPOINT=y
>   CONFIG_PCI_ENDPOINT_CONFIGFS=y
>   CONFIG_PCI_EPF_TEST=m
> +CONFIG_PCI_PWRCTL_SLOT=y
>   CONFIG_DEVTMPFS=y
>   CONFIG_DEVTMPFS_MOUNT=y
>   CONFIG_FW_LOADER_USER_HELPER=y
> --
> 2.34.1
> 
> 

PCI_PWRCTL_SLOT is a tristate symbol why be a "y" instead of an "m" i.e. 
compile this into the kernel instead of having it be a module ?

---
bod

Re: [PATCH v1 2/3] arm64: dts: qcom: x1e80100: add bus topology for PCIe domain 3

[Bryan O'Donoghue]

On 20/03/2025 05:55, Wenbin Yao wrote:
> From: Qiang Yu <quic_qianyu@quicinc.com>
> 
> Add pcie3port node to represent the PCIe bridge of PCIe3 so that PCI slot
> voltage rails can be described under this node in the board's dts.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/x1e80100.dtsi | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> index 46b79fce9..32e8d400a 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -3287,6 +3287,16 @@ opp-128000000 {
>   					opp-peak-kBps = <15753000 1>;
>   				};
>   			};
> +			pcie3port: pcie@0 { 

Missing newline, please check your dtb checks.


> +				device_type = "pci";
> +				compatible = "pciclass,0604";
> +				reg = <0x0 0x0 0x0 0x0 0x0>;
> +				bus-range = <0x01 0xff>;
> +
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +			};
>   		};

Why is pice3port the only port to be enabled ?

What about the other ports ?
>   		pcie3_phy: phy@1be0000 {
> --
> 2.34.1
> 
> 

---
bod

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

[Bryan O'Donoghue]

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.

---
bod

Re: [PATCH v1 1/3] arm64: defconfig: enable PCI Power Control for PCIe3

[Krzysztof Kozlowski]

On 20/03/2025 06:55, Wenbin Yao wrote:
> From: Qiang Yu <quic_qianyu@quicinc.com>
> 
> Enable the pwrctrl driver, which is utilized to manage the power supplies
> of the devices connected to the PCI slots. This ensures that the voltage
> rails of the x8 PCI slots on the X1E80100 - QCP can be correctly turned
> on/off if they are described under PCIe port device tree node.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
> ---
>  arch/arm64/configs/defconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 85ec2fba1..de86d1121 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -245,6 +245,7 @@ CONFIG_PCIE_LAYERSCAPE_GEN4=y
>  CONFIG_PCI_ENDPOINT=y
>  CONFIG_PCI_ENDPOINT_CONFIGFS=y
>  CONFIG_PCI_EPF_TEST=m
> +CONFIG_PCI_PWRCTL_SLOT=y
Bartosz,

Wasn't the intention to select it the same way as PCI_PWRCTL_PWRSEQ is
selected?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 20/03/2025 06: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 {

Don't send the downstream code.

Underscore are not allowed in node names plus:

Please use name for all fixed regulators which matches current format
recommendation: 'regulator-[0-9]v[0-9]'

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?h=v6.11-rc1#n46

Best regards,
Krzysztof

Re: [PATCH v1 1/3] arm64: defconfig: enable PCI Power Control for PCIe3

[Bartosz Golaszewski]

On Fri, Mar 21, 2025 at 8:37 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 20/03/2025 06:55, Wenbin Yao wrote:
> > From: Qiang Yu <quic_qianyu@quicinc.com>
> >
> > Enable the pwrctrl driver, which is utilized to manage the power supplies
> > of the devices connected to the PCI slots. This ensures that the voltage
> > rails of the x8 PCI slots on the X1E80100 - QCP can be correctly turned
> > on/off if they are described under PCIe port device tree node.
> >
> > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> > Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
> > ---
> >  arch/arm64/configs/defconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index 85ec2fba1..de86d1121 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -245,6 +245,7 @@ CONFIG_PCIE_LAYERSCAPE_GEN4=y
> >  CONFIG_PCI_ENDPOINT=y
> >  CONFIG_PCI_ENDPOINT_CONFIGFS=y
> >  CONFIG_PCI_EPF_TEST=m
> > +CONFIG_PCI_PWRCTL_SLOT=y
> Bartosz,
>
> Wasn't the intention to select it the same way as PCI_PWRCTL_PWRSEQ is
> selected?
>
> Best regards,
> Krzysztof
>

For sure. I would expect there to be something like:

select PCI_PWRCTL_SLOT if ARCH_QCOM

in Kconfig and nothing in defconfig.

Bartosz

Re: [PATCH v1 2/3] arm64: dts: qcom: x1e80100: add bus topology for PCIe domain 3

[Dmitry Baryshkov]

On Thu, Mar 20, 2025 at 01:55:01PM +0800, Wenbin Yao wrote:
> From: Qiang Yu <quic_qianyu@quicinc.com>
> 
> Add pcie3port node to represent the PCIe bridge of PCIe3 so that PCI slot
> voltage rails can be described under this node in the board's dts.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/x1e80100.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> index 46b79fce9..32e8d400a 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -3287,6 +3287,16 @@ opp-128000000 {
>  					opp-peak-kBps = <15753000 1>;
>  				};
>  			};
> +			pcie3port: pcie@0 {

Empty line between nodes, please.

> +				device_type = "pci";
> +				compatible = "pciclass,0604";
> +				reg = <0x0 0x0 0x0 0x0 0x0>;
> +				bus-range = <0x01 0xff>;
> +
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +			};
>  		};
>  
>  		pcie3_phy: phy@1be0000 {
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] arm64: dts: qcom: x1e80100: add bus topology for PCIe domain 3

[Wenbin Yao (Consultant)]

On 3/21/2025 6:06 AM, Bryan O'Donoghue wrote:
> On 20/03/2025 05:55, Wenbin Yao wrote:
>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>
>> Add pcie3port node to represent the PCIe bridge of PCIe3 so that PCI 
>> slot
>> voltage rails can be described under this node in the board's dts.
>>
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/x1e80100.dtsi | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi 
>> b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> index 46b79fce9..32e8d400a 100644
>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> @@ -3287,6 +3287,16 @@ opp-128000000 {
>>                       opp-peak-kBps = <15753000 1>;
>>                   };
>>               };
>> +            pcie3port: pcie@0 { 
>
> Missing newline, please check your dtb checks.

Will fix in the next version.

>
>
>> +                device_type = "pci";
>> +                compatible = "pciclass,0604";
>> +                reg = <0x0 0x0 0x0 0x0 0x0>;
>> +                bus-range = <0x01 0xff>;
>> +
>> +                #address-cells = <3>;
>> +                #size-cells = <2>;
>> +                ranges;
>> +            };
>>           };
>
> Why is pice3port the only port to be enabled ?
>
> What about the other ports ?

Only PCIe3 requires PCI slot power driver to power on its slots, other
ports don‘t need it.

>>           pcie3_phy: phy@1be0000 {
>> -- 
>> 2.34.1
>>
>>
>
> ---
> bod

-- 
With best wishes
Wenbin

Re: [PATCH v1 1/3] arm64: defconfig: enable PCI Power Control for PCIe3

[Wenbin Yao (Consultant)]

On 3/21/2025 6:01 AM, Bryan O'Donoghue wrote:
> On 20/03/2025 05:55, Wenbin Yao wrote:
>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>
>> Enable the pwrctrl driver, which is utilized to manage the power 
>> supplies
>> of the devices connected to the PCI slots. This ensures that the voltage
>> rails of the x8 PCI slots on the X1E80100 - QCP can be correctly turned
>> on/off if they are described under PCIe port device tree node.
>>
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>> ---
>>   arch/arm64/configs/defconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>> index 85ec2fba1..de86d1121 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -245,6 +245,7 @@ CONFIG_PCIE_LAYERSCAPE_GEN4=y
>>   CONFIG_PCI_ENDPOINT=y
>>   CONFIG_PCI_ENDPOINT_CONFIGFS=y
>>   CONFIG_PCI_EPF_TEST=m
>> +CONFIG_PCI_PWRCTL_SLOT=y
>>   CONFIG_DEVTMPFS=y
>>   CONFIG_DEVTMPFS_MOUNT=y
>>   CONFIG_FW_LOADER_USER_HELPER=y
>> -- 
>> 2.34.1
>>
>>
>
> PCI_PWRCTL_SLOT is a tristate symbol why be a "y" instead of an "m" 
> i.e. compile this into the kernel instead of having it be a module ?

It can be compiled as module if this is preferred.

>
> ---
> bod

-- 
With best wishes
Wenbin

Re: [PATCH v1 1/3] arm64: defconfig: enable PCI Power Control for PCIe3

[Wenbin Yao (Consultant)]

On 3/21/2025 5:43 PM, Bartosz Golaszewski wrote:
> On Fri, Mar 21, 2025 at 8:37 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 20/03/2025 06:55, Wenbin Yao wrote:
>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>
>>> Enable the pwrctrl driver, which is utilized to manage the power supplies
>>> of the devices connected to the PCI slots. This ensures that the voltage
>>> rails of the x8 PCI slots on the X1E80100 - QCP can be correctly turned
>>> on/off if they are described under PCIe port device tree node.
>>>
>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>>> ---
>>>   arch/arm64/configs/defconfig | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>>> index 85ec2fba1..de86d1121 100644
>>> --- a/arch/arm64/configs/defconfig
>>> +++ b/arch/arm64/configs/defconfig
>>> @@ -245,6 +245,7 @@ CONFIG_PCIE_LAYERSCAPE_GEN4=y
>>>   CONFIG_PCI_ENDPOINT=y
>>>   CONFIG_PCI_ENDPOINT_CONFIGFS=y
>>>   CONFIG_PCI_EPF_TEST=m
>>> +CONFIG_PCI_PWRCTL_SLOT=y
>> Bartosz,
>>
>> Wasn't the intention to select it the same way as PCI_PWRCTL_PWRSEQ is
>> selected?
>>
>> Best regards,
>> Krzysztof
>>
> For sure. I would expect there to be something like:
>
> select PCI_PWRCTL_SLOT if ARCH_QCOM
>
> in Kconfig and nothing in defconfig.
>
> Bartosz

IIUC, pci slot power driver is a common driver that could be used by all DT
based platform.

-- 
With best wishes
Wenbin

Re: [PATCH v1 2/3] arm64: dts: qcom: x1e80100: add bus topology for PCIe domain 3

[Wenbin Yao (Consultant)]

On 3/22/2025 1:19 AM, Dmitry Baryshkov wrote:
> On Thu, Mar 20, 2025 at 01:55:01PM +0800, Wenbin Yao wrote:
>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>
>> Add pcie3port node to represent the PCIe bridge of PCIe3 so that PCI slot
>> voltage rails can be described under this node in the board's dts.
>>
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/x1e80100.dtsi | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> index 46b79fce9..32e8d400a 100644
>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> @@ -3287,6 +3287,16 @@ opp-128000000 {
>>   					opp-peak-kBps = <15753000 1>;
>>   				};
>>   			};
>> +			pcie3port: pcie@0 {
> Empty line between nodes, please.

Will fix in the next version.

>
>> +				device_type = "pci";
>> +				compatible = "pciclass,0604";
>> +				reg = <0x0 0x0 0x0 0x0 0x0>;
>> +				bus-range = <0x01 0xff>;
>> +
>> +				#address-cells = <3>;
>> +				#size-cells = <2>;
>> +				ranges;
>> +			};
>>   		};
>>   
>>   		pcie3_phy: phy@1be0000 {
>> -- 
>> 2.34.1
>>
-- 
With best wishes
Wenbin

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

[Wenbin Yao (Consultant)]

On 3/21/2025 3:39 PM, Krzysztof Kozlowski wrote:
> On 20/03/2025 06: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 {
> Don't send the downstream code.
>
> Underscore are not allowed in node names plus:
>
> Please use name for all fixed regulators which matches current format
> recommendation: 'regulator-[0-9]v[0-9]'
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?h=v6.11-rc1#n46
>
> Best regards,
> Krzysztof

Will fix in the next version.

-- 
With best wishes
Wenbin

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

[Wenbin Yao (Consultant)]

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

Re: [PATCH v1 1/3] arm64: defconfig: enable PCI Power Control for PCIe3

[Krzysztof Kozlowski]

On 24/03/2025 08:09, Wenbin Yao (Consultant) wrote:
> On 3/21/2025 5:43 PM, Bartosz Golaszewski wrote:
>> On Fri, Mar 21, 2025 at 8:37 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> On 20/03/2025 06:55, Wenbin Yao wrote:
>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>
>>>> Enable the pwrctrl driver, which is utilized to manage the power supplies
>>>> of the devices connected to the PCI slots. This ensures that the voltage
>>>> rails of the x8 PCI slots on the X1E80100 - QCP can be correctly turned
>>>> on/off if they are described under PCIe port device tree node.
>>>>
>>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>>>> ---
>>>>   arch/arm64/configs/defconfig | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>>>> index 85ec2fba1..de86d1121 100644
>>>> --- a/arch/arm64/configs/defconfig
>>>> +++ b/arch/arm64/configs/defconfig
>>>> @@ -245,6 +245,7 @@ CONFIG_PCIE_LAYERSCAPE_GEN4=y
>>>>   CONFIG_PCI_ENDPOINT=y
>>>>   CONFIG_PCI_ENDPOINT_CONFIGFS=y
>>>>   CONFIG_PCI_EPF_TEST=m
>>>> +CONFIG_PCI_PWRCTL_SLOT=y
>>> Bartosz,
>>>
>>> Wasn't the intention to select it the same way as PCI_PWRCTL_PWRSEQ is
>>> selected?
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> For sure. I would expect there to be something like:
>>
>> select PCI_PWRCTL_SLOT if ARCH_QCOM
>>
>> in Kconfig and nothing in defconfig.
>>
>> Bartosz
> 
> IIUC, pci slot power driver is a common driver that could be used by all DT
> based platform.


You are not responding to the raised problem.

Best regards,
Krzysztof

Re: [PATCH v1 1/3] arm64: defconfig: enable PCI Power Control for PCIe3

[Wenbin Yao (Consultant)]

On 3/24/2025 3:38 PM, Krzysztof Kozlowski wrote:
> On 24/03/2025 08:09, Wenbin Yao (Consultant) wrote:
>> On 3/21/2025 5:43 PM, Bartosz Golaszewski wrote:
>>> On Fri, Mar 21, 2025 at 8:37 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> On 20/03/2025 06:55, Wenbin Yao wrote:
>>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>
>>>>> Enable the pwrctrl driver, which is utilized to manage the power supplies
>>>>> of the devices connected to the PCI slots. This ensures that the voltage
>>>>> rails of the x8 PCI slots on the X1E80100 - QCP can be correctly turned
>>>>> on/off if they are described under PCIe port device tree node.
>>>>>
>>>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>>>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>>>>> ---
>>>>>    arch/arm64/configs/defconfig | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>>>>> index 85ec2fba1..de86d1121 100644
>>>>> --- a/arch/arm64/configs/defconfig
>>>>> +++ b/arch/arm64/configs/defconfig
>>>>> @@ -245,6 +245,7 @@ CONFIG_PCIE_LAYERSCAPE_GEN4=y
>>>>>    CONFIG_PCI_ENDPOINT=y
>>>>>    CONFIG_PCI_ENDPOINT_CONFIGFS=y
>>>>>    CONFIG_PCI_EPF_TEST=m
>>>>> +CONFIG_PCI_PWRCTL_SLOT=y
>>>> Bartosz,
>>>>
>>>> Wasn't the intention to select it the same way as PCI_PWRCTL_PWRSEQ is
>>>> selected?
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>> For sure. I would expect there to be something like:
>>>
>>> select PCI_PWRCTL_SLOT if ARCH_QCOM
>>>
>>> in Kconfig and nothing in defconfig.
>>>
>>> Bartosz
>> IIUC, pci slot power driver is a common driver that could be used by all DT
>> based platform.
>
> You are not responding to the raised problem.

There is a slight difference between PCI_PWRCTL_SLOT and PCI_PWRCTL_PWRSEQ.
PCI_PWRCTL_PWRSEQ is selected by ath11k/ath12k, but PCI_PWRCTL_SLOT has no
specific endpoint device driver to select it. Could PCI_PWRCTL_SLOT be
selected along with HAVE_PWRCTL when ARCH_QCOM is enabled? Or can we add
select PCI_PWRCTL_SLOT if HAVE_PWRCTL in the Kconfig of portdrv?

>
> Best regards,
> Krzysztof

-- 
With best wishes
Wenbin