power: sequencing: implement the subsystem and add first users

From: Bartosz Golaszewski <[email protected]>

Allow to use __free() to automatically put references to OF nodes.

Quoted 3 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Quoted 1 line

Allow to use __free() to automatically put references to OF nodes.

Jonathan has already been working on this[1].

Rob

[1] https://lore.kernel.org/all/[email protected]/

From: Bartosz Golaszewski <[email protected]>

Add a node for the PMU module of the QCA6391 present on the RB5 board. Assign its LDO power outputs to the existing Bluetooth module. Add a node for the PCIe port to sm8250.dtsi and define the WLAN node on it in the board's .dts and also make it consume the power outputs of the PMU.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
 arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
 2 files changed, 127 insertions(+), 11 deletions(-)

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +13 -0

index cd0db4f31d4a..fab5bebafbad 100644
@@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 {
 		regulator-always-on;
 	};
 
+	qca6390_pmu: pmu@0 {
+		compatible = "qcom,qca6390-pmu";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&bt_en_state>, <&wlan_en_state>;
+
+		vddaon-supply = <&vreg_s6a_0p95>;
+		vddpmu-supply = <&vreg_s2f_0p95>;
+		vddrfa1-supply = <&vreg_s2f_0p95>;
+		vddrfa2-supply = <&vreg_s8c_1p3>;
+		vddrfa3-supply = <&vreg_s5a_1p9>;
+		vddpcie1-supply = <&vreg_s8c_1p3>;
+		vddpcie2-supply = <&vreg_s5a_1p9>;

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +0 -11

 &uart6 {

@@ -1311,17 +1418,16 @@ &uart6 {
 	bluetooth {
 		compatible = "qcom,qca6390-bt";
 
-		pinctrl-names = "default";
-		pinctrl-0 = <&bt_en_state>;
-
-		enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
-
-		vddio-supply = <&vreg_s4a_1p8>;
-		vddpmu-supply = <&vreg_s2f_0p95>;
-		vddaon-supply = <&vreg_s6a_0p95>;
-		vddrfa0p9-supply = <&vreg_s2f_0p95>;
-		vddrfa1p3-supply = <&vreg_s8c_1p3>;
-		vddrfa1p9-supply = <&vreg_s5a_1p9>;

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +0 -0

 	};
 };
 

arch/arm64/boot/dts/qcom/sm8250.dtsi +10 -0

index 4d849e98bf9b..7cd21d4e7278 100644
@@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 {
 			dma-coherent;
 
 			status = "disabled";
+
+			pcieport0: pcie@0 {
+				device_type = "pci";
+				reg = <0x0 0x0 0x0 0x0 0x0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+
+				bus-range = <0x01 0xff>;
+			};
 		};
 
 		pcie0_phy: phy@1c06000 {

Signature · 1 line

2.40.1

Quoted 41 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Add a node for the PMU module of the QCA6391 present on the RB5 board. Assign its LDO power outputs to the existing Bluetooth module. Add a node for the PCIe port to sm8250.dtsi and define the WLAN node on it in the board's .dts and also make it consume the power outputs of the PMU.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
 arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
 2 files changed, 127 insertions(+), 11 deletions(-)

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +20 -0

index cd0db4f31d4a..fab5bebafbad 100644
@@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 {
              regulator-always-on;
      };
 
+     qca6390_pmu: pmu@0 {
+             compatible = "qcom,qca6390-pmu";
+
+             pinctrl-names = "default";
+             pinctrl-0 = <&bt_en_state>, <&wlan_en_state>;
+
+             vddaon-supply = <&vreg_s6a_0p95>;
+             vddpmu-supply = <&vreg_s2f_0p95>;
+             vddrfa1-supply = <&vreg_s2f_0p95>;
+             vddrfa2-supply = <&vreg_s8c_1p3>;
+             vddrfa3-supply = <&vreg_s5a_1p9>;
+             vddpcie1-supply = <&vreg_s8c_1p3>;
+             vddpcie2-supply = <&vreg_s5a_1p9>;
+             vddio-supply = <&vreg_s4a_1p8>;
+
+             wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
+             bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
+
+             regulators {
+                     vreg_pmu_rfa_cmn: ldo0 {

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts lt9611_3v3: lt9611-3v3 { · line 133

Quoted 3 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +3 -0

+                             regulator-name = "vreg_pmu_rfa_cmn";
+                             regulator-min-microvolt = <760000>;
+                             regulator-max-microvolt = <840000>;

I'm still not convinced that the PMU has a set of LDOs, and looking at your implementation you neither register these with the regulator framework, nor provide any means of controlling the state or voltage of these "regulators".

[..]

Quoted 21 lines

&uart6 {

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +4 -11

@@ -1311,17 +1418,16 @@ &uart6 {
      bluetooth {
              compatible = "qcom,qca6390-bt";
 
-             pinctrl-names = "default";
-             pinctrl-0 = <&bt_en_state>;
-
-             enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
-
-             vddio-supply = <&vreg_s4a_1p8>;
-             vddpmu-supply = <&vreg_s2f_0p95>;
-             vddaon-supply = <&vreg_s6a_0p95>;
-             vddrfa0p9-supply = <&vreg_s2f_0p95>;
-             vddrfa1p3-supply = <&vreg_s8c_1p3>;
-             vddrfa1p9-supply = <&vreg_s5a_1p9>;
+             vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
+             vddaon-supply = <&vreg_pmu_aon_0p59>;
+             vddwlcx-supply = <&vreg_pmu_wlcx_0p8>;
+             vddwlmx-supply = <&vreg_pmu_wlmx_0p85>;

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts &pcie0_phy { · line 832

Quoted 6 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +6 -0

+             vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>;
+             vddrfa0-supply = <&vreg_pmu_rfa_0p8>;
+             vddrfa1-supply = <&vreg_pmu_rfa_1p2>;
+             vddrfa2-supply = <&vreg_pmu_rfa_1p7>;
+             vddpcie0-supply = <&vreg_pmu_pcie_0p9>;
+             vddpcie1-supply = <&vreg_pmu_pcie_1p8>;

As I asked before, why does bluetooth suddenly care about PCIe supplies?

Regards,
Bjorn

Quoted 27 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +0 -0

      };
 };
 

arch/arm64/boot/dts/qcom/sm8250.dtsi +10 -1

index 4d849e98bf9b..7cd21d4e7278 100644
@@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 {
                      dma-coherent;
 
                      status = "disabled";
+
+                     pcieport0: pcie@0 {
+                             device_type = "pci";
+                             reg = <0x0 0x0 0x0 0x0 0x0>;
+                             #address-cells = <3>;
+                             #size-cells = <2>;
+                             ranges;
+
+                             bus-range = <0x01 0xff>;
+                     };
              };
 
              pcie0_phy: phy@1c06000 {
--

2.40.1

Quoted 43 lines · Bjorn Andersson

Quoted 41 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Add a node for the PMU module of the QCA6391 present on the RB5 board. Assign its LDO power outputs to the existing Bluetooth module. Add a node for the PCIe port to sm8250.dtsi and define the WLAN node on it in the board's .dts and also make it consume the power outputs of the PMU.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
 arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
 2 files changed, 127 insertions(+), 11 deletions(-)

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +20 -0

index cd0db4f31d4a..fab5bebafbad 100644
@@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 {
              regulator-always-on;
      };
 
+     qca6390_pmu: pmu@0 {
+             compatible = "qcom,qca6390-pmu";
+
+             pinctrl-names = "default";
+             pinctrl-0 = <&bt_en_state>, <&wlan_en_state>;
+
+             vddaon-supply = <&vreg_s6a_0p95>;
+             vddpmu-supply = <&vreg_s2f_0p95>;
+             vddrfa1-supply = <&vreg_s2f_0p95>;
+             vddrfa2-supply = <&vreg_s8c_1p3>;
+             vddrfa3-supply = <&vreg_s5a_1p9>;
+             vddpcie1-supply = <&vreg_s8c_1p3>;
+             vddpcie2-supply = <&vreg_s5a_1p9>;
+             vddio-supply = <&vreg_s4a_1p8>;
+
+             wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
+             bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
+
+             regulators {
+                     vreg_pmu_rfa_cmn: ldo0 {

Quoted 8 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts lt9611_3v3: lt9611-3v3 { · line 133

Quoted 3 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +3 -0

+                             regulator-name = "vreg_pmu_rfa_cmn";
+                             regulator-min-microvolt = <760000>;
+                             regulator-max-microvolt = <840000>;

I'm still not convinced that the PMU has a set of LDOs, and looking at your implementation you neither register these with the regulator framework, nor provide any means of controlling the state or voltage of these "regulators".

Please take a look at the description of VDD08_PMU_RFA_CMN and VDD_PMU_AON_I pins in the spec (80-WL522-1, page 25). I'm not sure if I'm allowed to quote it, so I won't. But the spec clearly describes VDD_PMU_AON_I as 0.95V LDO input and VDD08_PMU_RFA_CMN as 0.8 LDO output generated using that input. I think this proves that the on-chip PMU has actual LDOs.

I must admit, I find this representation very verbose, but on the other hand Bartosz is right, it represents actual hardware.

Maybe we
can drop some of the properties of corresponding regulator blocks, as we don't actually need them and they are internal properties of the hardware.

Quoted 23 lines

[..]

Quoted 21 lines

&uart6 {

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +4 -11

@@ -1311,17 +1418,16 @@ &uart6 {
      bluetooth {
              compatible = "qcom,qca6390-bt";
 
-             pinctrl-names = "default";
-             pinctrl-0 = <&bt_en_state>;
-
-             enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
-
-             vddio-supply = <&vreg_s4a_1p8>;
-             vddpmu-supply = <&vreg_s2f_0p95>;
-             vddaon-supply = <&vreg_s6a_0p95>;
-             vddrfa0p9-supply = <&vreg_s2f_0p95>;
-             vddrfa1p3-supply = <&vreg_s8c_1p3>;
-             vddrfa1p9-supply = <&vreg_s5a_1p9>;
+             vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
+             vddaon-supply = <&vreg_pmu_aon_0p59>;
+             vddwlcx-supply = <&vreg_pmu_wlcx_0p8>;
+             vddwlmx-supply = <&vreg_pmu_wlmx_0p85>;

Quoted 8 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts &pcie0_phy { · line 832

Quoted 6 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +6 -0

+             vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>;
+             vddrfa0-supply = <&vreg_pmu_rfa_0p8>;
+             vddrfa1-supply = <&vreg_pmu_rfa_1p2>;
+             vddrfa2-supply = <&vreg_pmu_rfa_1p7>;
+             vddpcie0-supply = <&vreg_pmu_pcie_0p9>;
+             vddpcie1-supply = <&vreg_pmu_pcie_1p8>;

As I asked before, why does bluetooth suddenly care about PCIe supplies?

Power sequencing in the same spec describes that PCIe voltages should be up even if only BT is being brought up. PMU itself handles distributing voltages according to the actual load needs.

Quoted 32 lines

Regards,
Bjorn

Quoted 27 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +0 -0

      };
 };
 

arch/arm64/boot/dts/qcom/sm8250.dtsi +10 -1

index 4d849e98bf9b..7cd21d4e7278 100644
@@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 {
                      dma-coherent;
 
                      status = "disabled";
+
+                     pcieport0: pcie@0 {
+                             device_type = "pci";
+                             reg = <0x0 0x0 0x0 0x0 0x0>;
+                             #address-cells = <3>;
+                             #size-cells = <2>;
+                             ranges;
+
+                             bus-range = <0x01 0xff>;
+                     };
              };
 
              pcie0_phy: phy@1c06000 {
--

2.40.1

Signature · 2 lines

With best wishes
Dmitry

Quoted 6 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +6 -0

+
+             wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
+             bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
+
+             regulators {
+                     vreg_pmu_rfa_cmn: ldo0 {

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts lt9611_3v3: lt9611-3v3 { · line 133

Quoted 3 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +3 -0

+                             regulator-name = "vreg_pmu_rfa_cmn";
+                             regulator-min-microvolt = <760000>;
+                             regulator-max-microvolt = <840000>;

I'm still not convinced that the PMU has a set of LDOs, and looking at your implementation you neither register these with the regulator framework, nor provide any means of controlling the state or voltage of these "regulators".

[..]

Quoted 21 lines

&uart6 {

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +4 -11

@@ -1311,17 +1418,16 @@ &uart6 {
      bluetooth {
              compatible = "qcom,qca6390-bt";
 
-             pinctrl-names = "default";
-             pinctrl-0 = <&bt_en_state>;
-
-             enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
-
-             vddio-supply = <&vreg_s4a_1p8>;
-             vddpmu-supply = <&vreg_s2f_0p95>;
-             vddaon-supply = <&vreg_s6a_0p95>;
-             vddrfa0p9-supply = <&vreg_s2f_0p95>;
-             vddrfa1p3-supply = <&vreg_s8c_1p3>;
-             vddrfa1p9-supply = <&vreg_s5a_1p9>;
+             vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
+             vddaon-supply = <&vreg_pmu_aon_0p59>;
+             vddwlcx-supply = <&vreg_pmu_wlcx_0p8>;
+             vddwlmx-supply = <&vreg_pmu_wlmx_0p85>;

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts &pcie0_phy { · line 832

Quoted 6 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +6 -0

+             vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>;
+             vddrfa0-supply = <&vreg_pmu_rfa_0p8>;
+             vddrfa1-supply = <&vreg_pmu_rfa_1p2>;
+             vddrfa2-supply = <&vreg_pmu_rfa_1p7>;
+             vddpcie0-supply = <&vreg_pmu_pcie_0p9>;
+             vddpcie1-supply = <&vreg_pmu_pcie_1p8>;

As I asked before, why does bluetooth suddenly care about PCIe supplies?

Quoted 1 line · Bjorn Andersson

[snip]

Quoted 6 lines

Quoted 6 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +6 -0

+
+             wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
+             bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
+
+             regulators {
+                     vreg_pmu_rfa_cmn: ldo0 {

Quoted 9 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts lt9611_3v3: lt9611-3v3 { · line 133

Quoted 3 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +3 -0

+                             regulator-name = "vreg_pmu_rfa_cmn";
+                             regulator-min-microvolt = <760000>;
+                             regulator-max-microvolt = <840000>;

I'm still not convinced that the PMU has a set of LDOs, and looking at your implementation you neither register these with the regulator framework, nor provide any means of controlling the state or voltage of these "regulators".

Why are you so fixated on the driver implementation matching the device-tree 1:1? I asked that question before - what does it matter if we use the regulator subsystem or not?

This is just what HW there is.
What we do with that knowledge in C is irrelevant. Yes, I don't use the regulator subsystem because it's unnecessary and would actually get in the way of the power sequencing.

But it doesn't change the fact
that the regulators *are* there so let's show them.

What isn't there is a "power sequencer device". This was the main concern about Dmitry's implementation before.

We must not have
"bt-pwrseq = <&...>;" -like properties in device-tree because there is no device that this would represent. But there *are* LDO outputs of the PMU which can be modelled and then used in C to retrieve the power sequencer and this is what I'm proposing.

Bartosz

Quoted 22 lines

[..]

Quoted 21 lines

&uart6 {

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +4 -11

@@ -1311,17 +1418,16 @@ &uart6 {
      bluetooth {
              compatible = "qcom,qca6390-bt";
 
-             pinctrl-names = "default";
-             pinctrl-0 = <&bt_en_state>;
-
-             enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
-
-             vddio-supply = <&vreg_s4a_1p8>;
-             vddpmu-supply = <&vreg_s2f_0p95>;
-             vddaon-supply = <&vreg_s6a_0p95>;
-             vddrfa0p9-supply = <&vreg_s2f_0p95>;
-             vddrfa1p3-supply = <&vreg_s8c_1p3>;
-             vddrfa1p9-supply = <&vreg_s5a_1p9>;
+             vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
+             vddaon-supply = <&vreg_pmu_aon_0p59>;
+             vddwlcx-supply = <&vreg_pmu_wlcx_0p8>;
+             vddwlmx-supply = <&vreg_pmu_wlmx_0p85>;

Quoted 9 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts &pcie0_phy { · line 832

Quoted 6 lines

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +6 -0

+             vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>;
+             vddrfa0-supply = <&vreg_pmu_rfa_0p8>;
+             vddrfa1-supply = <&vreg_pmu_rfa_1p2>;
+             vddrfa2-supply = <&vreg_pmu_rfa_1p7>;
+             vddpcie0-supply = <&vreg_pmu_pcie_0p9>;
+             vddpcie1-supply = <&vreg_pmu_pcie_1p8>;

As I asked before, why does bluetooth suddenly care about PCIe supplies?

Yes, I forgot to remove it, I'll do it next time.

Bartosz

[snip]

From: Bartosz Golaszewski <[email protected]>

Add a node for the PMU module of the QCA6391 present on the RB5 board. Assign its LDO power outputs to the existing Bluetooth module. Add a node for the PCIe port to sm8250.dtsi and define the WLAN node on it in the board's .dts and also make it consume the power outputs of the PMU.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
 arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
 2 files changed, 127 insertions(+), 11 deletions(-)

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +0 -0

index cd0db4f31d4a..fab5bebafbad 100644

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +7 -0

+			};
+
+			vreg_pmu_aon_0p59: ldo1 {
+				regulator-name = "vreg_pmu_aon_0p59";
+				regulator-min-microvolt = <540000>;
+				regulator-max-microvolt = <840000>;
+			};

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +11 -11

+	};
 };
 
 &uart6 {

@@ -1311,17 +1418,16 @@ &uart6 {
 	bluetooth {
 		compatible = "qcom,qca6390-bt";
 
-		pinctrl-names = "default";
-		pinctrl-0 = <&bt_en_state>;
-
-		enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
-
-		vddio-supply = <&vreg_s4a_1p8>;
-		vddpmu-supply = <&vreg_s2f_0p95>;
-		vddaon-supply = <&vreg_s6a_0p95>;
-		vddrfa0p9-supply = <&vreg_s2f_0p95>;
-		vddrfa1p3-supply = <&vreg_s8c_1p3>;
-		vddrfa1p9-supply = <&vreg_s5a_1p9>;
+		vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
+		vddaon-supply = <&vreg_pmu_aon_0p59>;
+		vddwlcx-supply = <&vreg_pmu_wlcx_0p8>;
+		vddwlmx-supply = <&vreg_pmu_wlmx_0p85>;
+		vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>;
+		vddrfa0-supply = <&vreg_pmu_rfa_0p8>;
+		vddrfa1-supply = <&vreg_pmu_rfa_1p2>;
+		vddrfa2-supply = <&vreg_pmu_rfa_1p7>;
+		vddpcie0-supply = <&vreg_pmu_pcie_0p9>;
+		vddpcie1-supply = <&vreg_pmu_pcie_1p8>;
 	};
 };
 

arch/arm64/boot/dts/qcom/sm8250.dtsi +10 -0

index 4d849e98bf9b..7cd21d4e7278 100644
@@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 {
 			dma-coherent;
 
 			status = "disabled";
+
+			pcieport0: pcie@0 {
+				device_type = "pci";
+				reg = <0x0 0x0 0x0 0x0 0x0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+
+				bus-range = <0x01 0xff>;
+			};
 		};
 
 		pcie0_phy: phy@1c06000 {

Signature · 1 line

2.40.1

From: Bartosz Golaszewski <[email protected]>

Add a node for the PMU module of the QCA6391 present on the RB5 board. Assign its LDO power outputs to the existing Bluetooth module. Add a node for the PCIe port to sm8250.dtsi and define the WLAN node on it in the board's .dts and also make it consume the power outputs of the PMU.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
 arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
 2 files changed, 127 insertions(+), 11 deletions(-)

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +0 -0

index cd0db4f31d4a..fab5bebafbad 100644

From: Bartosz Golaszewski <[email protected]>

Implement the power sequencing subsystem allowing devices to share complex powering-up and down procedures. It's split into the consumer and provider parts but does not implement any new DT bindings so that the actual power sequencing is never revealed in the DT representation.

Signed-off-by Bartosz Golaszewski <[email protected]>

+++ b/drivers/power/sequencing/Kconfig

drivers/power/sequencing/Kconfig new file +1 -0

@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-only

drivers/power/sequencing/Makefile new file +0 -0

new file mode 100644
index 000000000000..dcdf8c0c159e

drivers/power/sequencing/pwrseq-qca6390.c +0 -0

drivers/power/sequencing/pwrseq-qca6390.c +4 -0

+
+	if (ctx->pdata->pwup_delay_msec)
+		msleep(ctx->pdata->pwup_delay_msec);
+

drivers/power/sequencing/pwrseq-qca6390.c +21 -0

+	return regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
+}
+
+static int pwrseq_qca6390_match(struct pwrseq_device *pwrseq,
+				struct device *dev)
+{
+	struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);
+	struct device_node *dev_node = dev->of_node;
+
+	/*
+	 * The PMU supplies power to the Bluetooth and WLAN modules. both
+	 * consume the PMU AON output so check the presence of the
+	 * 'vddaon-supply' property and whether it leads us to the right
+	 * device.
+	 */
+	if (!of_property_present(dev_node, "vddaon-supply"))
+		return 0;
+
+	struct device_node *reg_node __free(of_node) =
+			of_parse_phandle(dev_node, "vddaon-supply", 0);
+	if (!reg_node)

drivers/power/sequencing/pwrseq-qca6390.c +40 -0

+
+	return 1;
+}
+
+static int pwrseq_qca6390_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pwrseq_qca6390_ctx *ctx;
+	struct pwrseq_config config;
+	int ret, i;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->of_node = dev->of_node;
+
+	ctx->pdata = of_device_get_match_data(dev);
+	if (!ctx->pdata)
+		return dev_err_probe(dev, -ENODEV,
+				     "Failed to obtain platform data\n");
+
+	if (ctx->pdata->vregs) {
+		ctx->regs = devm_kcalloc(dev, ctx->pdata->num_vregs,
+					 sizeof(*ctx->regs), GFP_KERNEL);
+		if (!ctx->regs)
+			return -ENOMEM;
+
+		for (i = 0; i < ctx->pdata->num_vregs; i++)
+			ctx->regs[i].supply = ctx->pdata->vregs[i].name;
+
+		ret = devm_regulator_bulk_get(dev, ctx->pdata->num_vregs,
+					      ctx->regs);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "Failed to get all regulators\n");
+
+		for (i = 0; i < ctx->pdata->num_vregs; i++) {
+			if (!ctx->pdata->vregs[1].load_uA)
+				continue;

drivers/power/sequencing/pwrseq-qca6390.c +48 -0

+	if (IS_ERR(ctx->bt_gpio))
+		return dev_err_probe(dev, PTR_ERR(ctx->bt_gpio),
+				     "Failed to get the Bluetooth enable GPIO\n");
+
+	ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
+						 GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->wlan_gpio))
+		return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
+				     "Failed to get the WLAN enable GPIO\n");
+
+	memset(&config, 0, sizeof(config));
+
+	config.parent = dev;
+	config.owner = THIS_MODULE;
+	config.drvdata = ctx;
+	config.match = pwrseq_qca6390_match;
+	config.power_on = pwrseq_qca6390_power_on;
+	config.power_off = pwrseq_qca6390_power_off;
+
+	ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
+	if (IS_ERR(ctx->pwrseq))
+		return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
+				     "Failed to register the power sequencer\n");
+
+	return 0;
+}
+
+static const struct of_device_id pwrseq_qca6390_of_match[] = {
+	{
+		.compatible = "qcom,qca6390-pmu",
+		.data = &pwrseq_qca6390_of_data,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pwrseq_qca6390_of_match);
+
+static struct platform_driver pwrseq_qca6390_driver = {
+	.driver = {
+		.name = "pwrseq-qca6390",
+		.of_match_table = pwrseq_qca6390_of_match,
+	},
+	.probe = pwrseq_qca6390_probe,
+};
+module_platform_driver(pwrseq_qca6390_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <[email protected]>");
+MODULE_DESCRIPTION("QCA6390 PMU power sequencing driver");
+MODULE_LICENSE("GPL");

Signature · 1 line

2.40.1

Quoted 1 line · Bartosz Golaszewski

drivers/power/sequencing/pwrseq-qca6390.c +0 -0

[..]

drivers/power/sequencing/pwrseq-qca6390.c line 89

Quoted 11 lines

drivers/power/sequencing/pwrseq-qca6390.c +11 -0

+static int pwrseq_qca6390_power_on(struct pwrseq_device *pwrseq)
+{
+     struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);
+     int ret;
+
+     ret = regulator_bulk_enable(ctx->pdata->num_vregs, ctx->regs);
+     if (ret)
+             return ret;
+
+     gpiod_set_value_cansleep(ctx->bt_gpio, 1);
+     gpiod_set_value_cansleep(ctx->wlan_gpio, 1);

So it's no longer possible to power these independently?

Quoted 11 lines

drivers/power/sequencing/pwrseq-qca6390.c +10 -0

+
+     if (ctx->pdata->pwup_delay_msec)
+             msleep(ctx->pdata->pwup_delay_msec);
+
+     return 0;
+}
+
+static int pwrseq_qca6390_power_off(struct pwrseq_device *pwrseq)
+{
+     struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);

drivers/power/sequencing/pwrseq-qca6390.c line 103

Quoted 4 lines

drivers/power/sequencing/pwrseq-qca6390.c +4 -0

+
+     gpiod_set_value_cansleep(ctx->bt_gpio, 0);
+     gpiod_set_value_cansleep(ctx->wlan_gpio, 0);
+

The answer that was provided recently was that the WiFi and BT modules absolutely must be modelled together, because there must be a 100ms delay between bt_gpio going low and wlan_gpio going high.

If you're not going to address that concern, then I fail to see the reason for adding the power sequence framework - just let the BT and PCI power control (WiFi) do their thing independently.

Quoted 21 lines

drivers/power/sequencing/pwrseq-qca6390.c +21 -0

+     return regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
+}
+
+static int pwrseq_qca6390_match(struct pwrseq_device *pwrseq,
+                             struct device *dev)
+{
+     struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);
+     struct device_node *dev_node = dev->of_node;
+
+     /*
+      * The PMU supplies power to the Bluetooth and WLAN modules. both
+      * consume the PMU AON output so check the presence of the
+      * 'vddaon-supply' property and whether it leads us to the right
+      * device.
+      */
+     if (!of_property_present(dev_node, "vddaon-supply"))
+             return 0;
+
+     struct device_node *reg_node __free(of_node) =
+                     of_parse_phandle(dev_node, "vddaon-supply", 0);
+     if (!reg_node)

drivers/power/sequencing/pwrseq-qca6390.c line 134

Quoted 10 lines

drivers/power/sequencing/pwrseq-qca6390.c +10 -0

+             return 0;
+
+     /*
+      * `reg_node` is the PMU AON regulator, its parent is the `regulators`
+      * node and finally its grandparent is the PMU device node that we're
+      * looking for.
+      */
+     if (!reg_node->parent || !reg_node->parent->parent ||
+         reg_node->parent->parent != ctx->of_node)
+             return 0;

Your DeviceTree example gives a sense that a set of supplies feeds the PMU, which then supplies power to the BT and WiFi nodes through some entity that can switch power on and off, and adjust the voltage level.

Then comes this function, which indicates that the DeviceTree model was just for show.

Quoted 40 lines

drivers/power/sequencing/pwrseq-qca6390.c +40 -0

+
+     return 1;
+}
+
+static int pwrseq_qca6390_probe(struct platform_device *pdev)
+{
+     struct device *dev = &pdev->dev;
+     struct pwrseq_qca6390_ctx *ctx;
+     struct pwrseq_config config;
+     int ret, i;
+
+     ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+     if (!ctx)
+             return -ENOMEM;
+
+     ctx->of_node = dev->of_node;
+
+     ctx->pdata = of_device_get_match_data(dev);
+     if (!ctx->pdata)
+             return dev_err_probe(dev, -ENODEV,
+                                  "Failed to obtain platform data\n");
+
+     if (ctx->pdata->vregs) {
+             ctx->regs = devm_kcalloc(dev, ctx->pdata->num_vregs,
+                                      sizeof(*ctx->regs), GFP_KERNEL);
+             if (!ctx->regs)
+                     return -ENOMEM;
+
+             for (i = 0; i < ctx->pdata->num_vregs; i++)
+                     ctx->regs[i].supply = ctx->pdata->vregs[i].name;
+
+             ret = devm_regulator_bulk_get(dev, ctx->pdata->num_vregs,
+                                           ctx->regs);
+             if (ret < 0)
+                     return dev_err_probe(dev, ret,
+                                          "Failed to get all regulators\n");
+
+             for (i = 0; i < ctx->pdata->num_vregs; i++) {
+                     if (!ctx->pdata->vregs[1].load_uA)
+                             continue;

drivers/power/sequencing/pwrseq-qca6390.c line 184

Quoted 10 lines

drivers/power/sequencing/pwrseq-qca6390.c +10 -0

+
+                     ret = regulator_set_load(ctx->regs[i].consumer,
+                                              ctx->pdata->vregs[i].load_uA);
+                     if (ret)
+                             return dev_err_probe(dev, ret,
+                                                  "Failed to set vreg load\n");
+             }
+     }
+
+     ctx->bt_gpio = devm_gpiod_get_optional(dev, "bt-enable", GPIOD_OUT_LOW);

Why are these optional? Does it make sense to have a qca6390 without both of these gpios connected?

Regards,
Bjorn

Quoted 43 lines

drivers/power/sequencing/pwrseq-qca6390.c +43 -0

+     if (IS_ERR(ctx->bt_gpio))
+             return dev_err_probe(dev, PTR_ERR(ctx->bt_gpio),
+                                  "Failed to get the Bluetooth enable GPIO\n");
+
+     ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
+                                              GPIOD_OUT_LOW);
+     if (IS_ERR(ctx->wlan_gpio))
+             return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
+                                  "Failed to get the WLAN enable GPIO\n");
+
+     memset(&config, 0, sizeof(config));
+
+     config.parent = dev;
+     config.owner = THIS_MODULE;
+     config.drvdata = ctx;
+     config.match = pwrseq_qca6390_match;
+     config.power_on = pwrseq_qca6390_power_on;
+     config.power_off = pwrseq_qca6390_power_off;
+
+     ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
+     if (IS_ERR(ctx->pwrseq))
+             return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
+                                  "Failed to register the power sequencer\n");
+
+     return 0;
+}
+
+static const struct of_device_id pwrseq_qca6390_of_match[] = {
+     {
+             .compatible = "qcom,qca6390-pmu",
+             .data = &pwrseq_qca6390_of_data,
+     },
+     { }
+};
+MODULE_DEVICE_TABLE(of, pwrseq_qca6390_of_match);
+
+static struct platform_driver pwrseq_qca6390_driver = {
+     .driver = {
+             .name = "pwrseq-qca6390",
+             .of_match_table = pwrseq_qca6390_of_match,
+     },
+     .probe = pwrseq_qca6390_probe,
+};

Quoted 8 lines

drivers/power/sequencing/pwrseq-qca6390.c +5 -1

+module_platform_driver(pwrseq_qca6390_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <[email protected]>");
+MODULE_DESCRIPTION("QCA6390 PMU power sequencing driver");
+MODULE_LICENSE("GPL");
--

2.40.1

Quoted 4 lines · Bjorn Andersson

Quoted 1 line · Bartosz Golaszewski

drivers/power/sequencing/pwrseq-qca6390.c +0 -0

[..]

Quoted 13 lines

drivers/power/sequencing/pwrseq-qca6390.c line 89

Quoted 11 lines

drivers/power/sequencing/pwrseq-qca6390.c +11 -0

+static int pwrseq_qca6390_power_on(struct pwrseq_device *pwrseq)
+{
+     struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);
+     int ret;
+
+     ret = regulator_bulk_enable(ctx->pdata->num_vregs, ctx->regs);
+     if (ret)
+             return ret;
+
+     gpiod_set_value_cansleep(ctx->bt_gpio, 1);
+     gpiod_set_value_cansleep(ctx->wlan_gpio, 1);

So it's no longer possible to power these independently?

I'd second this, there must be a way to power them on and off separately. In the end, this provides a good way to restart the BT core if it gets sick.

Quoted 11 lines

Quoted 11 lines

drivers/power/sequencing/pwrseq-qca6390.c +10 -0

+
+     if (ctx->pdata->pwup_delay_msec)
+             msleep(ctx->pdata->pwup_delay_msec);
+
+     return 0;
+}
+
+static int pwrseq_qca6390_power_off(struct pwrseq_device *pwrseq)
+{
+     struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);

Quoted 8 lines

drivers/power/sequencing/pwrseq-qca6390.c line 103

Quoted 4 lines

drivers/power/sequencing/pwrseq-qca6390.c +4 -0

+
+     gpiod_set_value_cansleep(ctx->bt_gpio, 0);
+     gpiod_set_value_cansleep(ctx->wlan_gpio, 0);
+

The answer that was provided recently was that the WiFi and BT modules absolutely must be modelled together, because there must be a 100ms delay between bt_gpio going low and wlan_gpio going high.

For the reference, it was for the QCA6490 (not QCA6390, next revision), which maps to WCN6855.

Quoted 145 lines

If you're not going to address that concern, then I fail to see the reason for adding the power sequence framework - just let the BT and PCI power control (WiFi) do their thing independently.

Quoted 21 lines

drivers/power/sequencing/pwrseq-qca6390.c +21 -0

+     return regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
+}
+
+static int pwrseq_qca6390_match(struct pwrseq_device *pwrseq,
+                             struct device *dev)
+{
+     struct pwrseq_qca6390_ctx *ctx = pwrseq_device_get_data(pwrseq);
+     struct device_node *dev_node = dev->of_node;
+
+     /*
+      * The PMU supplies power to the Bluetooth and WLAN modules. both
+      * consume the PMU AON output so check the presence of the
+      * 'vddaon-supply' property and whether it leads us to the right
+      * device.
+      */
+     if (!of_property_present(dev_node, "vddaon-supply"))
+             return 0;
+
+     struct device_node *reg_node __free(of_node) =
+                     of_parse_phandle(dev_node, "vddaon-supply", 0);
+     if (!reg_node)

drivers/power/sequencing/pwrseq-qca6390.c line 134

Quoted 10 lines

drivers/power/sequencing/pwrseq-qca6390.c +10 -0

+             return 0;
+
+     /*
+      * `reg_node` is the PMU AON regulator, its parent is the `regulators`
+      * node and finally its grandparent is the PMU device node that we're
+      * looking for.
+      */
+     if (!reg_node->parent || !reg_node->parent->parent ||
+         reg_node->parent->parent != ctx->of_node)
+             return 0;

Your DeviceTree example gives a sense that a set of supplies feeds the PMU, which then supplies power to the BT and WiFi nodes through some entity that can switch power on and off, and adjust the voltage level.

Then comes this function, which indicates that the DeviceTree model was just for show.

Quoted 40 lines

drivers/power/sequencing/pwrseq-qca6390.c +40 -0

+
+     return 1;
+}
+
+static int pwrseq_qca6390_probe(struct platform_device *pdev)
+{
+     struct device *dev = &pdev->dev;
+     struct pwrseq_qca6390_ctx *ctx;
+     struct pwrseq_config config;
+     int ret, i;
+
+     ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+     if (!ctx)
+             return -ENOMEM;
+
+     ctx->of_node = dev->of_node;
+
+     ctx->pdata = of_device_get_match_data(dev);
+     if (!ctx->pdata)
+             return dev_err_probe(dev, -ENODEV,
+                                  "Failed to obtain platform data\n");
+
+     if (ctx->pdata->vregs) {
+             ctx->regs = devm_kcalloc(dev, ctx->pdata->num_vregs,
+                                      sizeof(*ctx->regs), GFP_KERNEL);
+             if (!ctx->regs)
+                     return -ENOMEM;
+
+             for (i = 0; i < ctx->pdata->num_vregs; i++)
+                     ctx->regs[i].supply = ctx->pdata->vregs[i].name;
+
+             ret = devm_regulator_bulk_get(dev, ctx->pdata->num_vregs,
+                                           ctx->regs);
+             if (ret < 0)
+                     return dev_err_probe(dev, ret,
+                                          "Failed to get all regulators\n");
+
+             for (i = 0; i < ctx->pdata->num_vregs; i++) {
+                     if (!ctx->pdata->vregs[1].load_uA)
+                             continue;

drivers/power/sequencing/pwrseq-qca6390.c line 184

Quoted 10 lines

drivers/power/sequencing/pwrseq-qca6390.c +10 -0

+
+                     ret = regulator_set_load(ctx->regs[i].consumer,
+                                              ctx->pdata->vregs[i].load_uA);
+                     if (ret)
+                             return dev_err_probe(dev, ret,
+                                                  "Failed to set vreg load\n");
+             }
+     }
+
+     ctx->bt_gpio = devm_gpiod_get_optional(dev, "bt-enable", GPIOD_OUT_LOW);

Why are these optional? Does it make sense to have a qca6390 without both of these gpios connected?

Regards,
Bjorn

Quoted 43 lines

drivers/power/sequencing/pwrseq-qca6390.c +43 -0

+     if (IS_ERR(ctx->bt_gpio))
+             return dev_err_probe(dev, PTR_ERR(ctx->bt_gpio),
+                                  "Failed to get the Bluetooth enable GPIO\n");
+
+     ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
+                                              GPIOD_OUT_LOW);
+     if (IS_ERR(ctx->wlan_gpio))
+             return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
+                                  "Failed to get the WLAN enable GPIO\n");
+
+     memset(&config, 0, sizeof(config));
+
+     config.parent = dev;
+     config.owner = THIS_MODULE;
+     config.drvdata = ctx;
+     config.match = pwrseq_qca6390_match;
+     config.power_on = pwrseq_qca6390_power_on;
+     config.power_off = pwrseq_qca6390_power_off;
+
+     ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
+     if (IS_ERR(ctx->pwrseq))
+             return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
+                                  "Failed to register the power sequencer\n");
+
+     return 0;
+}
+
+static const struct of_device_id pwrseq_qca6390_of_match[] = {
+     {
+             .compatible = "qcom,qca6390-pmu",
+             .data = &pwrseq_qca6390_of_data,
+     },
+     { }
+};
+MODULE_DEVICE_TABLE(of, pwrseq_qca6390_of_match);
+
+static struct platform_driver pwrseq_qca6390_driver = {
+     .driver = {
+             .name = "pwrseq-qca6390",
+             .of_match_table = pwrseq_qca6390_of_match,
+     },
+     .probe = pwrseq_qca6390_probe,
+};

Quoted 9 lines

Quoted 8 lines

drivers/power/sequencing/pwrseq-qca6390.c +5 -1

+module_platform_driver(pwrseq_qca6390_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <[email protected]>");
+MODULE_DESCRIPTION("QCA6390 PMU power sequencing driver");
+MODULE_LICENSE("GPL");
--

2.40.1

--
With best wishes
Dmitry

From: Bartosz Golaszewski <[email protected]>

In order to introduce PCI power-sequencing,

we need to create platform

devices for child nodes of the port node. They will get matched against the pwrseq drivers

(if one exists) and then the actual PCI device will reuse the node once it's detected on the bus.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/pci/bus.c    | 9 ++++++++-
 drivers/pci/remove.c | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

drivers/pci/bus.c +1 -0

index 826b5016a101..17ab41094c4e 100644
@@ -12,6 +12,7 @@
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/proc_fs.h>
 #include <linux/slab.h>
 

drivers/pci/bus.c +4 -0

+		if (retval)
+			pci_err(dev, "failed to populate child OF nodes (%d)\n",
+				retval);
+	}
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);

drivers/pci/remove.c +2 -0

index d749ea8250d6..fc9db2805888 100644
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/pci.h>
 #include <linux/module.h>
+#include <linux/of_platform.h>
 #include "pci.h"
 
 static void pci_free_resources(struct pci_dev *dev)

@@ -22,6 +23,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 		device_release_driver(&dev->dev);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
+		of_platform_depopulate(&dev->dev);
 		of_pci_remove_node(dev);
 
 		pci_dev_assign_added(dev, false);

Signature · 1 line

2.40.1

Quoted 3 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

In order to introduce PCI power-sequencing,

Please provide a proper problem description.

Quoted 1 line

we need to create platform

And properly express why this is a "need".

Quoted 2 lines

devices for child nodes of the port node. They will get matched against the pwrseq drivers

That's not what happens in your code, the child nodes of the bridge node in DeviceTree will match against arbitrary platform_drivers.

I also would like this commit message to express that the job of the matched device is to:

1) power up said device, followed by triggering a scan on the parent PCI bus during it's probe function.

2) power down said device, during its remove function.

Quoted 2 lines

(if one exists) and then the actual PCI device will reuse the node once it's detected on the bus.

I think the "reuse" deserves to be clarified as there will be both a pci and a platform device associated with the same of_node.

Quoted 19 lines

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/pci/bus.c    | 9 ++++++++-
 drivers/pci/remove.c | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

drivers/pci/bus.c +1 -0

index 826b5016a101..17ab41094c4e 100644
@@ -12,6 +12,7 @@
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/proc_fs.h>
 #include <linux/slab.h>
 

drivers/pci/bus.c void pci_bus_add_device(struct pci_dev *dev) · line 349

Quoted 9 lines

drivers/pci/bus.c +3 -1

@@ -342,8 +343,14 @@ void pci_bus_add_device(struct pci_dev *dev)
       */
      pcibios_bus_add_device(dev);
      pci_fixup_device(pci_fixup_final, dev);
-     if (pci_is_bridge(dev))
+     if (pci_is_bridge(dev)) {
              of_pci_make_dev_node(dev);
+             retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
+                                           &dev->dev);

I'm not familiar enough with the ins and outs of the PCI code. Can you confirm that there are no problems with this (possibly) calling pci_rescan_bus() before the bridge device is fully initialized below?

Regards,
Bjorn

Quoted 30 lines

drivers/pci/bus.c +4 -0

+             if (retval)
+                     pci_err(dev, "failed to populate child OF nodes (%d)\n",
+                             retval);
+     }
      pci_create_sysfs_dev_files(dev);
      pci_proc_attach_device(dev);
      pci_bridge_d3_update(dev);

drivers/pci/remove.c +2 -1

index d749ea8250d6..fc9db2805888 100644
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/pci.h>
 #include <linux/module.h>
+#include <linux/of_platform.h>
 #include "pci.h"
 
 static void pci_free_resources(struct pci_dev *dev)

@@ -22,6 +23,7 @@ static void pci_stop_dev(struct pci_dev *dev)
              device_release_driver(&dev->dev);
              pci_proc_detach_device(dev);
              pci_remove_sysfs_dev_files(dev);
+             of_platform_depopulate(&dev->dev);
              of_pci_remove_node(dev);
 
              pci_dev_assign_added(dev, false);
--

2.40.1

From: Bartosz Golaszewski <[email protected]>

Some PCI devices must be powered-on before they can be detected on the bus. Introduce a simple framework reusing the existing PCI OF infrastructure.

The way this works is: a DT node representing a PCI device connected to the port can be matched against its power control platform driver. If the match succeeds, the driver is responsible for powering-up the device and calling pcie_pwrctl_device_enable() which will trigger a PCI bus rescan as well as subscribe to PCI bus notifications.

When the device is detected and created, we'll make it consume the same DT node that the platform device did. When the device is bound, we'll create a device link between it and the parent power control device.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/pci/Kconfig         |  1 +
 drivers/pci/Makefile        |  1 +
 drivers/pci/pwrctl/Kconfig  |  8 ++++
 drivers/pci/pwrctl/Makefile |  3 ++
 drivers/pci/pwrctl/core.c   | 82 +++++++++++++++++++++++++++++++++++++
 include/linux/pci-pwrctl.h  | 24 +++++++++++
 6 files changed, 119 insertions(+)
 create mode 100644 drivers/pci/pwrctl/Kconfig
 create mode 100644 drivers/pci/pwrctl/Makefile
 create mode 100644 drivers/pci/pwrctl/core.c
 create mode 100644 include/linux/pci-pwrctl.h

drivers/pci/Kconfig +1 -0

index 74147262625b..5b9b84f8774f 100644
@@ -291,5 +291,6 @@ source "drivers/pci/hotplug/Kconfig"
 source "drivers/pci/controller/Kconfig"
 source "drivers/pci/endpoint/Kconfig"
 source "drivers/pci/switch/Kconfig"
+source "drivers/pci/pwrctl/Kconfig"
 
 endif

drivers/pci/Makefile +1 -0

index cc8b4e01e29d..6ae202d950f8 100644
@@ -9,6 +9,7 @@ obj-$(CONFIG_PCI)		+= access.o bus.o probe.o host-bridge.o \
 
 obj-$(CONFIG_PCI)		+= msi/
 obj-$(CONFIG_PCI)		+= pcie/
+obj-$(CONFIG_PCI)		+= pwrctl/
 
 ifdef CONFIG_PCI
 obj-$(CONFIG_PROC_FS)		+= proc.o

drivers/pci/pwrctl/Kconfig new file +8 -0

new file mode 100644
index 000000000000..e2dc5e5d2af1
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0
+
+menu "PCI Power control drivers"
+
+config PCI_PWRCTL
+	bool
+
+endmenu

drivers/pci/pwrctl/Makefile new file +3 -0

new file mode 100644
index 000000000000..4381cfbf3f21
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_PCI_PWRCTL)		+= core.o

drivers/pci/pwrctl/core.c new file +15 -0

new file mode 100644
index 000000000000..312e6fe95c31
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci-pwrctl.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
+			     void *data)

drivers/pci/pwrctl/core.c new file +6 -0

+{
+	if (!pwrctl->dev)
+		return -ENODEV;
+
+	pwrctl->nb.notifier_call = pci_pwrctl_notify;
+	bus_register_notifier(&pci_bus_type, &pwrctl->nb);

drivers/pci/pwrctl/core.c new file +25 -0

+{
+	bus_unregister_notifier(&pci_bus_type, &pwrctl->nb);
+}
+EXPORT_SYMBOL_GPL(pci_pwrctl_device_disable);
+
+static void devm_pci_pwrctl_device_disable(void *data)
+{
+	struct pci_pwrctl *pwrctl = data;
+
+	pci_pwrctl_device_disable(pwrctl);
+}
+
+int devm_pci_pwrctl_device_enable(struct device *dev,
+				  struct pci_pwrctl *pwrctl)
+{
+	int ret;
+
+	ret = pci_pwrctl_device_enable(pwrctl);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_pci_pwrctl_device_disable,
+					pwrctl);
+}
+EXPORT_SYMBOL_GPL(devm_pci_pwrctl_device_enable);

include/linux/pci-pwrctl.h new file +24 -0

new file mode 100644
index 000000000000..8d16d27cbfeb
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#ifndef __PCI_PWRCTL_H__
+#define __PCI_PWRCTL_H__
+
+#include <linux/notifier.h>
+
+struct device;
+
+struct pci_pwrctl {
+	struct notifier_block nb;
+	struct device *dev;
+	struct device_link *link;
+};
+
+int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl);
+void pci_pwrctl_device_disable(struct pci_pwrctl *pwrctl);
+int devm_pci_pwrctl_device_enable(struct device *dev,
+				  struct pci_pwrctl *pwrctl);
+
+#endif /* __PCI_PWRCTL_H__ */

Signature · 1 line

2.40.1

drivers/pci/pwrctl/core.c line 25

Quoted 13 lines

drivers/pci/pwrctl/core.c new file +13 -0

+
+static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
+                          void *data)
+{
+     struct pci_pwrctl *pwrctl = container_of(nb, struct pci_pwrctl, nb);
+     struct device *dev = data;
+
+     if (dev_fwnode(dev) != dev_fwnode(pwrctl->dev))
+             return NOTIFY_DONE;
+
+     switch (action) {
+     case BUS_NOTIFY_ADD_DEVICE:
+             device_set_of_node_from_dev(dev, pwrctl->dev);

What happens if the bootloader left the power on, and the of_platform_populate() got probe deferred because the pwrseq wasn't ready, so this happens after pci_set_of_node() has been called?

(I think dev->of_node will be put, then get and then node_reused assigned...but I'm not entirely sure)

drivers/pci/pwrctl/core.c line 34

Quoted 10 lines

drivers/pci/pwrctl/core.c new file +9 -0

+             break;
+     case BUS_NOTIFY_BOUND_DRIVER:
+             pwrctl->link = device_link_add(dev, pwrctl->dev,
+                                            DL_FLAG_AUTOREMOVE_CONSUMER);
+             if (!pwrctl->link)
+                     dev_err(pwrctl->dev, "Failed to add device link\n");
+             break;
+     case BUS_NOTIFY_UNBOUND_DRIVER:
+             device_link_del(pwrctl->link);

This might however become a NULL-pointer dereference, if dev was bound to its driver before the pci_pwrctl_notify() was registered for the pwrctl and then the PCI device is unbound.

This would also happen if device_link_add() failed when the PCI device was bound...

drivers/pci/pwrctl/core.c line 41

Quoted 7 lines

drivers/pci/pwrctl/core.c new file +7 -0

+             break;
+     }
+
+     return NOTIFY_DONE;
+}
+
+int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl)

This function doesn't really "enable the device", looking at the example driver it's rather "device_enabled" than "device_enable"...

On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <[email protected]> wrote:

Quoted 11 lines

drivers/pci/pwrctl/core.c line 41

Quoted 7 lines

drivers/pci/pwrctl/core.c new file +7 -0

+             break;
+     }
+
+     return NOTIFY_DONE;
+}
+
+int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl)

This function doesn't really "enable the device", looking at the example driver it's rather "device_enabled" than "device_enable"...

I was also thinking about pci_pwrctl_device_ready() or pci_pwrctl_device_prepared().

Bart

[snip!]

Quoted 1 line · Bartosz Golaszewski

On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <[email protected]> wrote:

[..]

Quoted 12 lines

Quoted 11 lines

drivers/pci/pwrctl/core.c line 41

Quoted 7 lines

drivers/pci/pwrctl/core.c new file +7 -0

+             break;
+     }
+
+     return NOTIFY_DONE;
+}
+
+int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl)

This function doesn't really "enable the device", looking at the example driver it's rather "device_enabled" than "device_enable"...

Quoted 2 lines

I was also thinking about pci_pwrctl_device_ready() or pci_pwrctl_device_prepared().

I like both of these.

I guess the bigger question is how the flow would look like in the event that we need to power-cycle the attached PCIe device, e.g. because firmware has gotten into a really bad state.

Will we need an operation that removes the device first, and then cut the power, or do we cut the power and then call unprepared()?

Quoted 10 lines · On Fri, Feb 2, 2024 at 5:52???PM Bjorn Andersson <[email protected]> wrote:

Quoted 2 lines · Bartosz Golaszewski

I was also thinking about pci_pwrctl_device_ready() or pci_pwrctl_device_prepared().

I like both of these.

I guess the bigger question is how the flow would look like in the event that we need to power-cycle the attached PCIe device, e.g. because firmware has gotten into a really bad state.

Quoted 2 lines

Will we need an operation that removes the device first, and then cut the power, or do we cut the power and then call unprepared()?

How would the core be notified about this power-cycle from the PCI subsystem? I honestly don't know. Is there a notifier we could subscribe to? Is the device unbound and rebound in such case?

Quoted 11 lines · Bartosz Golaszewski

Quoted 10 lines · On Fri, Feb 2, 2024 at 5:52???PM Bjorn Andersson <[email protected]> wrote:

Quoted 2 lines · Bartosz Golaszewski

I was also thinking about pci_pwrctl_device_ready() or pci_pwrctl_device_prepared().

I like both of these.

I guess the bigger question is how the flow would look like in the event that we need to power-cycle the attached PCIe device, e.g. because firmware has gotten into a really bad state.

Quoted 6 lines

Quoted 2 lines

Will we need an operation that removes the device first, and then cut the power, or do we cut the power and then call unprepared()?

How would the core be notified about this power-cycle from the PCI subsystem? I honestly don't know. Is there a notifier we could subscribe to? Is the device unbound and rebound in such case?

To power-manage the PCI device for runtime PM (suspend to D3cold) or system sleep, you need to amend:

platform_pci_power_manageable()
platform_pci_set_power_state()
platform_pci_get_power_state()
platform_pci_refresh_power_state()
platform_pci_choose_state()

E.g. platform_pci_power_manageable() would check for presence of a regulator in the DT and platform_pci_set_power_state() would disable or enable the regulator.

To reset the device by power cycling it, amend pci_reset_fn_methods[] to provide a reset method which disables and re-enables the regulator. Then you can choose that reset method via sysfs and power-cycle the device. The PCI core will also automatically use that reset method if there's nothing else available (e.g. if no Secondary Bus Reset is available because the device has siblings or children, or if FLR is not supported).

Thanks,

Lukas

Quoted 1 line · Bartosz Golaszewski

On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <[email protected]> wrote:

[..]

Quoted 12 lines

Quoted 11 lines

drivers/pci/pwrctl/core.c line 41

Quoted 7 lines

drivers/pci/pwrctl/core.c new file +7 -0

+             break;
+     }
+
+     return NOTIFY_DONE;
+}
+
+int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl)

This function doesn't really "enable the device", looking at the example driver it's rather "device_enabled" than "device_enable"...

Quoted 2 lines

I was also thinking about pci_pwrctl_device_ready() or pci_pwrctl_device_prepared().

I like both of these.

I guess the bigger question is how the flow would look like in the event that we need to power-cycle the attached PCIe device, e.g. because firmware has gotten into a really bad state.

Will we need an operation that removes the device first, and then cut the power, or do we cut the power and then call unprepared()?

Quoted 24 lines · Bjorn Andersson

Quoted 1 line · Bartosz Golaszewski

On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <[email protected]> wrote:

[..]

Quoted 12 lines

Quoted 11 lines

drivers/pci/pwrctl/core.c line 41

Quoted 7 lines

drivers/pci/pwrctl/core.c new file +7 -0

+             break;
+     }
+
+     return NOTIFY_DONE;
+}
+
+int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl)

This function doesn't really "enable the device", looking at the example driver it's rather "device_enabled" than "device_enable"...

Quoted 2 lines

I was also thinking about pci_pwrctl_device_ready() or pci_pwrctl_device_prepared().

I like both of these.

I guess the bigger question is how the flow would look like in the event that we need to power-cycle the attached PCIe device, e.g. because firmware has gotten into a really bad state.

Quoted 3 lines

Will we need an operation that removes the device first, and then cut the power, or do we cut the power and then call unprepared()?

Currently, we don't power cycle while resetting the devices. Most of the drivers just do a software reset using some register writes. Part of the reason for that is, the drivers themselves don't control the power to the devices and there would be no way to let the parent know about the firmware crash.

Quoted 25 lines · Manivannan Sadhasivam

Quoted 24 lines · Bjorn Andersson

Quoted 1 line · Bartosz Golaszewski

On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <[email protected]> wrote:

[..]

Quoted 12 lines

Quoted 11 lines

drivers/pci/pwrctl/core.c line 41

Quoted 7 lines

drivers/pci/pwrctl/core.c new file +7 -0

+             break;
+     }
+
+     return NOTIFY_DONE;
+}
+
+int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl)

This function doesn't really "enable the device", looking at the example driver it's rather "device_enabled" than "device_enable"...

Quoted 2 lines

I was also thinking about pci_pwrctl_device_ready() or pci_pwrctl_device_prepared().

I like both of these.

I guess the bigger question is how the flow would look like in the event that we need to power-cycle the attached PCIe device, e.g. because firmware has gotten into a really bad state.

Quoted 9 lines

Quoted 3 lines

Will we need an operation that removes the device first, and then cut the power, or do we cut the power and then call unprepared()?

Currently, we don't power cycle while resetting the devices. Most of the drivers just do a software reset using some register writes. Part of the reason for that is, the drivers themselves don't control the power to the devices and there would be no way to let the parent know about the firmware crash.

I don't know what the appropriate design for this is, but we do have a need for being able to recover from this state by the means of power-cycling the device.

If it's not possible to let the device do it (in any fashion), then perhaps a user-space-assisted model is needed?

Turning on power is an important first step, but please do consider the full scope of the known problem space.

drivers/pci/pwrctl/core.c line 34

Quoted 10 lines

drivers/pci/pwrctl/core.c new file +9 -0

+             break;
+     case BUS_NOTIFY_BOUND_DRIVER:
+             pwrctl->link = device_link_add(dev, pwrctl->dev,
+                                            DL_FLAG_AUTOREMOVE_CONSUMER);
+             if (!pwrctl->link)
+                     dev_err(pwrctl->dev, "Failed to add device link\n");
+             break;
+     case BUS_NOTIFY_UNBOUND_DRIVER:
+             device_link_del(pwrctl->link);

This might however become a NULL-pointer dereference, if dev was bound to its driver before the pci_pwrctl_notify() was registered for the pwrctl and then the PCI device is unbound.

This would also happen if device_link_add() failed when the PCI device was bound...

From: Bartosz Golaszewski <[email protected]>

Add a PCI power control driver that's capable of correctly powering up devices using the power sequencing subsystem. For now we support the ath11k module on QCA6390.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/pci/pwrctl/Kconfig             |  9 +++
 drivers/pci/pwrctl/Makefile            |  1 +
 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 83 ++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c

drivers/pci/pwrctl/Kconfig +9 -0

index e2dc5e5d2af1..bca72dc08e79 100644
@@ -5,4 +5,13 @@ menu "PCI Power control drivers"
 config PCI_PWRCTL
 	bool
 
+config PCI_PWRCTL_PWRSEQ
+	tristate "PCI Power Control driver using the Power Sequencing subsystem"
+	select POWER_SEQUENCING
+	select PCI_PWRCTL
+	default m if (ATH11K_PCI && ARCH_QCOM)
+	help
+	  Enable support for the PCI power control driver for device
+	  drivers using the Power Sequencing subsystem.
+
 endmenu

drivers/pci/pwrctl/Makefile +1 -0

index 4381cfbf3f21..919c0f704ee9 100644
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_PCI_PWRCTL)		+= core.o
+obj-$(CONFIG_PCI_PWRCTL_PWRSEQ)		+= pci-pwrctl-pwrseq.o

drivers/pci/pwrctl/pci-pwrctl-pwrseq.c new file +83 -0

new file mode 100644
index 000000000000..510598c4edc4
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023-2024 Linaro Ltd.
+ */
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pci-pwrctl.h>
+#include <linux/platform_device.h>
+#include <linux/pwrseq/consumer.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+struct pci_pwrctl_pwrseq_data {
+	struct pci_pwrctl ctx;
+	struct pwrseq_desc *pwrseq;
+};
+
+static void devm_pci_pwrctl_pwrseq_power_off(void *data)
+{
+	struct pwrseq_desc *pwrseq = data;
+
+	pwrseq_power_off(pwrseq);
+}
+
+static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
+{
+	struct pci_pwrctl_pwrseq_data *data;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->pwrseq = devm_pwrseq_get(dev);
+	if (IS_ERR(data->pwrseq))
+		return dev_err_probe(dev, PTR_ERR(data->pwrseq),
+				     "Failed to get the power sequencer\n");
+
+	ret = pwrseq_power_on(data->pwrseq);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to power-on the device\n");
+
+	ret = devm_add_action_or_reset(dev, devm_pci_pwrctl_pwrseq_power_off,
+				       data->pwrseq);
+	if (ret)
+		return ret;
+
+	data->ctx.dev = dev;
+
+	ret = devm_pci_pwrctl_device_enable(dev, &data->ctx);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register the pwrctl wrapper\n");
+
+	return 0;
+}
+
+static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = {
+	{
+		/* ATH11K in QCA6390 package. */
+		.compatible = "pci17cb,1101",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pci_pwrctl_pwrseq_of_match);
+
+static struct platform_driver pci_pwrctl_pwrseq_driver = {
+	.driver = {
+		.name = "pci-pwrctl-pwrseq",
+		.of_match_table = pci_pwrctl_pwrseq_of_match,
+	},
+	.probe = pci_pwrctl_pwrseq_probe,
+};
+module_platform_driver(pci_pwrctl_pwrseq_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <[email protected]>");
+MODULE_DESCRIPTION("Generic PCI Power Control module for power sequenced devices");
+MODULE_LICENSE("GPL");

Signature · 1 line

2.40.1

Quoted 2 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Quoted 4 lines

Add a PCI power control driver that's capable of correctly powering up devices using the power sequencing subsystem. For now we support the ath11k module on QCA6390.

For a PCI device which doesn't share resources with something on another bus, the whole power sequencing would be implemented in a driver like this - without the involvement of the power sequence framework.

I think it would be nice to see this series introduce a simple pci_pwrctl driver, and then (in the same series) introduce the power sequence framework and your PMU driver.

One case where such model would be appropriate is the XHCI controller (uPD720201) on db845c. Today we describe vddpe-3p3-supply as a supply on the PCI controller, but it should have been vdd33-supply, vdd10-supply, avdd33-supply on the PCI device.

That would provide an example for how a simple PCI power control driver can/should look like, and we can discuss the PCI pieces separate from the introduction of the new power sequence framework (which is unrelated to PCI).

Quoted 4 lines · Bjorn Andersson

Quoted 2 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Quoted 9 lines

Quoted 4 lines

Add a PCI power control driver that's capable of correctly powering up devices using the power sequencing subsystem. For now we support the ath11k module on QCA6390.

For a PCI device which doesn't share resources with something on another bus, the whole power sequencing would be implemented in a driver like this - without the involvement of the power sequence framework.

Yes, this is what I did in the previous incarnation of this code[1].

(I know, I should have linked it here. My bad, I will do it next time).

Quoted 4 lines

I think it would be nice to see this series introduce a simple pci_pwrctl driver, and then (in the same series) introduce the power sequence framework and your PMU driver.

I disagree. I was initially annoyed by Dmitry asking me to do a lot more work than anticipated but he's right after all. WLAN and BT consuming what is really the PMU's inputs is simply not the actual representation. That's why we should make it a pwrseq user IMO.

Quoted 4 lines

One case where such model would be appropriate is the XHCI controller (uPD720201) on db845c. Today we describe vddpe-3p3-supply as a supply on the PCI controller, but it should have been vdd33-supply, vdd10-supply, avdd33-supply on the PCI device.

Sounds like a good second user then!

Quoted 5 lines

That would provide an example for how a simple PCI power control driver can/should look like, and we can discuss the PCI pieces separate from the introduction of the new power sequence framework (which is unrelated to PCI).

I agree it's unrelated and it could possibly go upstream separately but the particular use-case on RB5 (and other Qcom platforms) requires both the PCI and generic power sequencing to be addressed.

Bart

[snip]

[1] https://lore.kernel.org/netdev/[email protected]/T/#m72f52254a52fcb8a8a44de0702cad1087d4bcfa1

From: Bartosz Golaszewski <[email protected]>

I'd like to preface the cover letter by saying right away that this series is not complete. It's an RFC that presents my approach and is sent to the list for discussion. There are no DT bindings nor docs in Documentation/ yet. Please review it as an RFC and not an upstreambound series. If the approach is accepted as correct, I'll add missing bits.

The RFC[1] presenting my proposed device-tree representation of the QCA6391 package present on the RB5 board - while not really officially accepted - was not outright rejected which is a good sign.

This series incorporates it and builds a proposed power sequencing subsystem together with the first dedicated driver around it. Then it adds first two users: the Bluetooth and WLAN modules of the QCA6391.

The Bluetooth part is pretty straightforward. The WLAN however is a PCIe device and as such needs to be powered-up *before* it's detected on the PCI bus. To that end, we modify the PCI core to instantiate platform devices for existing DT child nodes of the PCIe ports. For those nodes for which a power-sequencing driver exists, we bind it and let it probe. The driver then triggers a rescan of the PCI bus with the aim of detecting the now powered-on device. The device will consume the same DT node as the platform, power-sequencing device. We use device links to make the latter become the parent of the former.

The main advantage of the above approach (both for PCI as well as generic power sequencers) is that we don't introduce significant changes in DT bindings and don't introduce new properties. We merely define new resources.

[1] https://lore.kernel.org/all/CAMRc=MckG32DQv7b1AQL-mbnYdx4fsdYWtLwCyXc5Ma7EeSAKw@mail.gmail.com/T/#md5dc62007d12f6833d4e51658b14e0493954ba68

Bartosz Golaszewski (9):
of: provide a cleanup helper for OF nodes
arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
power: sequencing: new subsystem
power: pwrseq: add a driver for the QCA6390 PMU module
Bluetooth: qca: use the power sequencer for QCA6390
PCI: create platform devices for child OF nodes of the port node
PCI: hold the rescan mutex when scanning for the first time PCI/pwrctl: add PCI power control core code
PCI/pwrctl: add a PCI power control driver for power sequenced devices

 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts  | 128 +++++-
 arch/arm64/boot/dts/qcom/sm8250.dtsi      |  10 +
 drivers/bluetooth/hci_qca.c               |  30 ++
 drivers/pci/Kconfig                       |   1 +
 drivers/pci/Makefile                      |   1 +
 drivers/pci/bus.c                         |   9 +-
 drivers/pci/probe.c                       |   2 +
 drivers/pci/pwrctl/Kconfig                |  17 +
 drivers/pci/pwrctl/Makefile               |   4 +
 drivers/pci/pwrctl/core.c                 |  82 ++++
 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c    |  83 ++++
 drivers/pci/remove.c                      |   2 +
 drivers/power/Kconfig                     |   1 +
 drivers/power/Makefile                    |   1 +
 drivers/power/sequencing/Kconfig          |  28 ++
 drivers/power/sequencing/Makefile         |   6 +
 drivers/power/sequencing/core.c           | 482 ++++++++++++++++++++++
 drivers/power/sequencing/pwrseq-qca6390.c | 232 +++++++++++
 include/linux/of.h                        |   4 +
 include/linux/pci-pwrctl.h                |  24 ++
 include/linux/pwrseq/consumer.h           |  53 +++
 include/linux/pwrseq/provider.h           |  41 ++
 22 files changed, 1229 insertions(+), 12 deletions(-)
 create mode 100644 drivers/pci/pwrctl/Kconfig
 create mode 100644 drivers/pci/pwrctl/Makefile
 create mode 100644 drivers/pci/pwrctl/core.c
 create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
 create mode 100644 drivers/power/sequencing/Kconfig
 create mode 100644 drivers/power/sequencing/Makefile
 create mode 100644 drivers/power/sequencing/core.c
 create mode 100644 drivers/power/sequencing/pwrseq-qca6390.c
 create mode 100644 include/linux/pci-pwrctl.h
 create mode 100644 include/linux/pwrseq/consumer.h
 create mode 100644 include/linux/pwrseq/provider.h

Signature · 1 line

2.40.1

Quoted 2 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

We now have 3 RFC and 1 PATCH versions of these patches on the list in under a month. Please at least add a version to your subject...

Quoted 24 lines

I'd like to preface the cover letter by saying right away that this series is not complete. It's an RFC that presents my approach and is sent to the list for discussion. There are no DT bindings nor docs in Documentation/ yet. Please review it as an RFC and not an upstreambound series. If the approach is accepted as correct, I'll add missing bits.

The RFC[1] presenting my proposed device-tree representation of the QCA6391 package present on the RB5 board - while not really officially accepted - was not outright rejected which is a good sign.

This series incorporates it and builds a proposed power sequencing subsystem together with the first dedicated driver around it. Then it adds first two users: the Bluetooth and WLAN modules of the QCA6391.

The Bluetooth part is pretty straightforward. The WLAN however is a PCIe device and as such needs to be powered-up *before* it's detected on the PCI bus. To that end, we modify the PCI core to instantiate platform devices for existing DT child nodes of the PCIe ports. For those nodes for which a power-sequencing driver exists, we bind it and let it probe. The driver then triggers a rescan of the PCI bus with the aim of detecting the now powered-on device. The device will consume the same DT node as the platform, power-sequencing device. We use device links to make the latter become the parent of the former.

Quoted 5 lines

The main advantage of the above approach (both for PCI as well as generic power sequencers) is that we don't introduce significant changes in DT bindings and don't introduce new properties. We merely define new resources.

How can we tell? There are still no Documentation/dt-bindings changes in your series.

Regards,
Bjorn

Quoted 51 lines

[1] https://lore.kernel.org/all/CAMRc=MckG32DQv7b1AQL-mbnYdx4fsdYWtLwCyXc5Ma7EeSAKw@mail.gmail.com/T/#md5dc62007d12f6833d4e51658b14e0493954ba68

Bartosz Golaszewski (9):
of: provide a cleanup helper for OF nodes
arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391
power: sequencing: new subsystem
power: pwrseq: add a driver for the QCA6390 PMU module
Bluetooth: qca: use the power sequencer for QCA6390
PCI: create platform devices for child OF nodes of the port node
PCI: hold the rescan mutex when scanning for the first time PCI/pwrctl: add PCI power control core code
PCI/pwrctl: add a PCI power control driver for power sequenced devices

 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts  | 128 +++++-
 arch/arm64/boot/dts/qcom/sm8250.dtsi      |  10 +
 drivers/bluetooth/hci_qca.c               |  30 ++
 drivers/pci/Kconfig                       |   1 +
 drivers/pci/Makefile                      |   1 +
 drivers/pci/bus.c                         |   9 +-
 drivers/pci/probe.c                       |   2 +
 drivers/pci/pwrctl/Kconfig                |  17 +
 drivers/pci/pwrctl/Makefile               |   4 +
 drivers/pci/pwrctl/core.c                 |  82 ++++
 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c    |  83 ++++
 drivers/pci/remove.c                      |   2 +
 drivers/power/Kconfig                     |   1 +
 drivers/power/Makefile                    |   1 +
 drivers/power/sequencing/Kconfig          |  28 ++
 drivers/power/sequencing/Makefile         |   6 +
 drivers/power/sequencing/core.c           | 482 ++++++++++++++++++++++
 drivers/power/sequencing/pwrseq-qca6390.c | 232 +++++++++++
 include/linux/of.h                        |   4 +
 include/linux/pci-pwrctl.h                |  24 ++
 include/linux/pwrseq/consumer.h           |  53 +++
 include/linux/pwrseq/provider.h           |  41 ++
 22 files changed, 1229 insertions(+), 12 deletions(-)
 create mode 100644 drivers/pci/pwrctl/Kconfig
 create mode 100644 drivers/pci/pwrctl/Makefile
 create mode 100644 drivers/pci/pwrctl/core.c
 create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
 create mode 100644 drivers/power/sequencing/Kconfig
 create mode 100644 drivers/power/sequencing/Makefile
 create mode 100644 drivers/power/sequencing/core.c
 create mode 100644 drivers/power/sequencing/pwrseq-qca6390.c
 create mode 100644 include/linux/pci-pwrctl.h
 create mode 100644 include/linux/pwrseq/consumer.h
 create mode 100644 include/linux/pwrseq/provider.h

--
2.40.1

From: Bartosz Golaszewski <[email protected]>

I'd like to preface the cover letter by saying right away that this series is not complete. It's an RFC that presents my approach and is sent to the list for discussion. There are no DT bindings nor docs in Documentation/ yet. Please review it as an RFC and not an upstreambound series. If the approach is accepted as correct, I'll add missing bits.

The RFC[1] presenting my proposed device-tree representation of the QCA6391 package present on the RB5 board - while not really officially accepted - was not outright rejected which is a good sign.

This series incorporates it and builds a proposed power sequencing subsystem together with the first dedicated driver around it. Then it adds first two users: the Bluetooth and WLAN modules of the QCA6391.

The Bluetooth part is pretty straightforward. The WLAN however is a PCIe device and as such needs to be powered-up *before* it's detected on the PCI bus. To that end, we modify the PCI core to instantiate platform devices for existing DT child nodes of the PCIe ports. For those nodes for which a power-sequencing driver exists, we bind it and let it probe. The driver then triggers a rescan of the PCI bus with the aim of detecting the now powered-on device. The device will consume the same DT node as the platform, power-sequencing device. We use device links to make the latter become the parent of the former.

The main advantage of the above approach (both for PCI as well as generic power sequencers) is that we don't introduce significant changes in DT bindings and don't introduce new properties. We merely define new resources.

[1] https://lore.kernel.org/all/CAMRc=MckG32DQv7b1AQL-mbnYdx4fsdYWtLwCyXc5Ma7EeSAKw@mail.gmail.com/T/#md5dc62007d12f6833d4e51658b14e0493954ba68