[RFC 0/9] power: sequencing: implement the subsystem and add first users

[RFC 0/9] power: sequencing: implement the subsystem and add first users

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

-- 
2.40.1

[RFC 1/9] of: provide a cleanup helper for OF nodes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/of.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 331e05918f11..5462ed47f25b 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -11,6 +11,8 @@
  * Updates for SPARC64 by David S. Miller
  * Derived from PowerPC and Sparc prom.h files by Stephen Rothwell, IBM Corp.
  */
+
+#include <linux/cleanup.h>
 #include <linux/types.h>
 #include <linux/bitops.h>
 #include <linux/errno.h>
@@ -887,6 +889,8 @@ static inline const void *of_device_get_match_data(const struct device *dev)
 #define of_match_node(_matches, _node)	NULL
 #endif /* CONFIG_OF */
 
+DEFINE_FREE(of_node, struct device_node *, if (_T) of_node_put(_T))
+
 /* Default string compare functions, Allow arch asm/prom.h to override */
 #if !defined(of_compat_cmp)
 #define of_compat_cmp(s1, s2, l)	strcasecmp((s1), (s2))
-- 
2.40.1

Re: [RFC 1/9] of: provide a cleanup helper for OF nodes

[Rob Herring]

On Thu, Feb 1, 2024 at 9:55 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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/20240128160542.178315-1-jic23@kernel.org/

Re: [RFC 1/9] of: provide a cleanup helper for OF nodes

[Bartosz Golaszewski]

On Thu, Feb 1, 2024 at 11:18 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Feb 1, 2024 at 9:55 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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/20240128160542.178315-1-jic23@kernel.org/

Thanks, I will watch this but for now I'll have to stick to carrying
it in my series until it gets upstream.

Bart

[RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
 arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
 2 files changed, 127 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
index cd0db4f31d4a..fab5bebafbad 100644
--- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
+++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
@@ -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 {
+				regulator-name = "vreg_pmu_rfa_cmn";
+				regulator-min-microvolt = <760000>;
+				regulator-max-microvolt = <840000>;
+			};
+
+			vreg_pmu_aon_0p59: ldo1 {
+				regulator-name = "vreg_pmu_aon_0p59";
+				regulator-min-microvolt = <540000>;
+				regulator-max-microvolt = <840000>;
+			};
+
+			vreg_pmu_wlcx_0p8: ldo2 {
+				regulator_name = "vreg_pmu_wlcx_0p8";
+				regulator-min-microvolt = <760000>;
+				regulator-max-microvolt = <840000>;
+			};
+
+			vreg_pmu_wlmx_0p85: ldo3 {
+				regulator-name = "vreg_pmu_wlmx_0p85";
+				regulator-min-microvolt = <810000>;
+				regulator-max-microvolt = <890000>;
+			};
+
+			vreg_pmu_btcmx_0p85: ldo4 {
+				regulator-name = "vreg_pmu_btcmx_0p85";
+				regulator-min-microvolt = <810000>;
+				regulator-max-microvolt = <890000>;
+			};
+
+			vreg_pmu_rfa_0p8: ldo5 {
+				regulator-name = "vreg_pmu_rfa_0p8";
+				regulator-min-microvolt = <760000>;
+				regulator-max-microvolt = <840000>;
+			};
+
+			vreg_pmu_rfa_1p2: ldo6 {
+				regulator-name = "vreg_pmu_rfa_1p2";
+				regulator-min-microvolt = <1187000>;
+				regulator-max-microvolt = <1313000>;
+			};
+
+			vreg_pmu_rfa_1p7: ldo7 {
+				regulator_name = "vreg_pmu_rfa_1p7";
+				regulator-min-microvolt = <1710000>;
+				regulator-max-microvolt = <1890000>;
+			};
+
+			vreg_pmu_pcie_0p9: ldo8 {
+				regulator_name = "vreg_pmu_pcie_0p9";
+				regulator-min-microvolt = <870000>;
+				regulator-max-microvolt = <970000>;
+			};
+
+			vreg_pmu_pcie_1p8: ldo9 {
+				regulator_name = "vreg_pmu_pcie_1p8";
+				regulator-min-microvolt = <1710000>;
+				regulator-max-microvolt = <1890000>;
+			};
+		};
+	};
+
 	thermal-zones {
 		conn-thermal {
 			polling-delay-passive = <0>;
@@ -734,6 +815,24 @@ &pcie0_phy {
 	vdda-pll-supply = <&vreg_l9a_1p2>;
 };
 
+&pcieport0 {
+	wifi@0 {
+		compatible = "pci17cb,1101";
+		reg = <0x10000 0x0 0x0 0x0 0x0>;
+
+		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>;
+	};
+};
+
 &pcie1 {
 	status = "okay";
 };
@@ -1303,6 +1402,14 @@ sdc2_card_det_n: sd-card-det-n-state {
 		function = "gpio";
 		bias-pull-up;
 	};
+
+	wlan_en_state: wlan-default-state {
+		pins = "gpio20";
+		function = "gpio";
+		drive-strength = <16>;
+		output-low;
+		bias-pull-up;
+	};
 };
 
 &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>;
 	};
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 4d849e98bf9b..7cd21d4e7278 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -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

Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391

[Bjorn Andersson]

On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
>  arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
>  2 files changed, 127 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> index cd0db4f31d4a..fab5bebafbad 100644
> --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> @@ -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 {
> +				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".

[..]
>  
>  &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>;

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

Regards,
Bjorn

>  	};
>  };
>  
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index 4d849e98bf9b..7cd21d4e7278 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -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
> 

Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391

[Dmitry Baryshkov]

On Fri, 2 Feb 2024 at 06:34, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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 <bartosz.golaszewski@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
> >  arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
> >  2 files changed, 127 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> > index cd0db4f31d4a..fab5bebafbad 100644
> > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> > @@ -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 {
> > +                             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.

>
> [..]
> >
> >  &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>;
>
> 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.

>
> Regards,
> Bjorn
>
> >       };
> >  };
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > index 4d849e98bf9b..7cd21d4e7278 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > @@ -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
> >
>


-- 
With best wishes
Dmitry

Re: Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391

[Bjorn Andersson]

On Fri, Feb 02, 2024 at 06:59:48AM +0200, Dmitry Baryshkov wrote:
> On Fri, 2 Feb 2024 at 06:34, Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > 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 <bartosz.golaszewski@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
> > >  arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
> > >  2 files changed, 127 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> > > index cd0db4f31d4a..fab5bebafbad 100644
> > > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> > > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> > > @@ -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 {
> > > +                             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.
> 

You're correct, thank you for the pointer and clarification. I now agree
with you, the PMU consumes what I saw as the chip input supplies, and
based on WL_EN and BT_EN will provide power on pads, which are then
externally routed to respective block.

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

I agree, this is 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.
> 

To me this really looks like a fancy "regulator-fixed" with multiple
inputs, two gpios and multiple outputs.

This would also imply that we don't need to invent the power sequence
framework to tie WiFi and BT to the PMU's state.

The PMU is a thing, so we can represent that in DeviceTree, it consumes
M input power rails, and two gpios, it provides N WiFi supplies and O BT
supplies (with some overlap between N and O). The WiFi node consumes its
N supplies, the BT node consumes its O supplies.

If any of the N regulators are requested enabled the qca6390-pmu driver
enables all M input rails, then enables WL_EN. If any of the O BT
regulators are requested enabled, the driver enables all M input rails,
then enables BT_EN.

> >
> > [..]
> > >
> > >  &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>;
> >
> > 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.
> 

You're right, the power sequence diagram in the docs do indicate that
VDD13_PMU_PCIE_I and VDD19_PMU_PCIE_I should be enabled before either
WL_EN or BT_EN are driven high.

But I don't see anything stating that the output from the PMU
(VDD09_PMU_PCIE) in turn is fed to the bluetooth block.

Regards,
Bjorn

> >
> > Regards,
> > Bjorn
> >
> > >       };
> > >  };
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > index 4d849e98bf9b..7cd21d4e7278 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > @@ -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
> > >
> >
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391

[Bartosz Golaszewski]

On Fri, Feb 2, 2024 at 5:34 AM Bjorn Andersson <andersson@kernel.org> wrote:
>

[snip]

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

> [..]
> >
> >  &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>;
>
> 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]

Re: Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391

[Bjorn Andersson]

On Fri, Feb 02, 2024 at 02:23:49PM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 2, 2024 at 5:34 AM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> 
> [snip]
> 
> > > +
> > > +             wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
> > > +             bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
> > > +
> > > +             regulators {
> > > +                     vreg_pmu_rfa_cmn: ldo0 {
> > > +                             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?

I'm sorry, I must have missed this question. I'm not questioning why the
DT needs to match the Linux implementation, I was really questioning if
the hardware you describe here existed.

> 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.

Then describe that in your commit messages.

> 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.

I don't agree. The concerns that I saw being raised with Dmitry's
proposed design was that he used connected the WiFi controller to the
QCA6391 using power-domains, etc.

> 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.
> 

Performing device-specific power sequences is extremely common, but we
so far don't have a separate abstraction of this because it's generally
not an matter external to any given device.

If we're going to introduce a power sequence framework, it needs to be
made very clear that it is there to solve the problem that you have
devices on separate busses that need to share that sequence.

This also implies that for most examples out there where we have a need
for doing "PCI power sequencing", I don't think we would use the
power-sequence framework.

Regards,
Bjorn

> Bartosz
> 
> > [..]
> > >
> > >  &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>;
> >
> > 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]

Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391

[Bjorn Andersson]

On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
>  arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
>  2 files changed, 127 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> index cd0db4f31d4a..fab5bebafbad 100644
> --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> @@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 {
>  		regulator-always-on;
>  	};
>  
> +	qca6390_pmu: pmu@0 {

The node doesn't have an address, so why does it have a unit address?
Also, the node isn't referenced, so please skip the label.

> +		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>;

So, after studying the datasheet for this. The names of the pins seems
to come from the existing binding(s). As you're introducing a new
binding (and driver) for qcom,qca6390-pmu, please use the pad names from
qca6390.

> +
> +		wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
> +		bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
> +
> +		regulators {
> +			vreg_pmu_rfa_cmn: ldo0 {
> +				regulator-name = "vreg_pmu_rfa_cmn";
> +				regulator-min-microvolt = <760000>;
> +				regulator-max-microvolt = <840000>;

The min/max operating range of the regulator is something we provide in
the implementation, in DeviceTree you should specify the expected
operating voltage. Based on the TYP value this should be
regulator-min-microvolt = regulator-max-microvolt = 0.8V.

> +			};
> +
> +			vreg_pmu_aon_0p59: ldo1 {
> +				regulator-name = "vreg_pmu_aon_0p59";
> +				regulator-min-microvolt = <540000>;
> +				regulator-max-microvolt = <840000>;
> +			};
[..]
> @@ -1303,6 +1402,14 @@ sdc2_card_det_n: sd-card-det-n-state {
>  		function = "gpio";
>  		bias-pull-up;
>  	};
> +
> +	wlan_en_state: wlan-default-state {
> +		pins = "gpio20";
> +		function = "gpio";
> +		drive-strength = <16>;
> +		output-low;

Please omit output-low here.

> +		bias-pull-up;

Why do you drive it low and pull it high? bias-disable sounds more
appropriate.

Regards,
Bjorn

> +	};
>  };
>  
>  &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>;
>  	};
>  };
>  
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index 4d849e98bf9b..7cd21d4e7278 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -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
> 

Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391

[Krzysztof Kozlowski]

On 01/02/2024 16:55, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
>  arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
>  2 files changed, 127 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> index cd0db4f31d4a..fab5bebafbad 100644
> --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> @@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 {
>  		regulator-always-on;
>  	};
>  
> +	qca6390_pmu: pmu@0 {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof

[RFC 3/9] power: sequencing: new subsystem

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <bartosz.golaszewski@linaro.org>
---
 drivers/power/Kconfig             |   1 +
 drivers/power/Makefile            |   1 +
 drivers/power/sequencing/Kconfig  |  12 +
 drivers/power/sequencing/Makefile |   4 +
 drivers/power/sequencing/core.c   | 482 ++++++++++++++++++++++++++++++
 include/linux/pwrseq/consumer.h   |  53 ++++
 include/linux/pwrseq/provider.h   |  41 +++
 7 files changed, 594 insertions(+)
 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 include/linux/pwrseq/consumer.h
 create mode 100644 include/linux/pwrseq/provider.h

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 696bf77a7042..9a8e44ca9ae4 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 source "drivers/power/reset/Kconfig"
+source "drivers/power/sequencing/Kconfig"
 source "drivers/power/supply/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index effbf0377f32..962a2cd30a51 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_POWER_RESET)	+= reset/
+obj-$(CONFIG_POWER_SEQUENCING)	+= sequencing/
 obj-$(CONFIG_POWER_SUPPLY)	+= supply/
diff --git a/drivers/power/sequencing/Kconfig b/drivers/power/sequencing/Kconfig
new file mode 100644
index 000000000000..ba5732b1dbf8
--- /dev/null
+++ b/drivers/power/sequencing/Kconfig
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+menuconfig POWER_SEQUENCING
+	tristate "Power Sequencing support"
+	help
+	  Say Y here to enable the Power Sequencing subsystem.
+
+	  This subsystem is designed to control power to devices that share
+	  complex resources and/or require specific power sequences to be run
+	  during power-up.
+
+	  If unsure, say no.
diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
new file mode 100644
index 000000000000..dcdf8c0c159e
--- /dev/null
+++ b/drivers/power/sequencing/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_POWER_SEQUENCING)		+= pwrseq-core.o
+pwrseq-core-y				:= core.o
diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
new file mode 100644
index 000000000000..f035caed0e4e
--- /dev/null
+++ b/drivers/power/sequencing/core.c
@@ -0,0 +1,482 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#include <linux/bug.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/pwrseq/consumer.h>
+#include <linux/pwrseq/provider.h>
+#include <linux/rwsem.h>
+
+static DEFINE_IDA(pwrseq_ida);
+/*
+ * Protects the device list on the bus from concurrent modifications but allows
+ * simultaneous read-only access.
+ */
+static DECLARE_RWSEM(pwrseq_sem);
+
+/**
+ * struct pwrseq_device - Private power sequencing data.
+ * @dev: Device struct associated with this sequencer.
+ * @id: Device ID.
+ * @owner: Prevents removal of active power sequencing providers.
+ * @pwrup_count: Keeps track of power state change requests.
+ * @sem: Protects the device against being unregistered while in use.
+ * @drvdata: Provider driver private data.
+ * @match: Power sequencer matching callback.
+ * @power_on: Power-on callback.
+ * @power_off: Power-off callback.
+ */
+struct pwrseq_device {
+	struct device dev;
+	int id;
+	struct module *owner;
+	unsigned int pwrup_count;
+	struct rw_semaphore dev_sem;
+	struct mutex state_lock;
+	void *drvdata;
+	pwrseq_match_func match;
+	pwrseq_power_state_func power_on;
+	pwrseq_power_state_func power_off;
+};
+
+/**
+ * struct pwrseq_desc - Wraps access to the pwrseq_device and ensures that one
+ *                      user cannot break the reference counting for others.
+ * @pwrseq: Reference to the power sequencing device.
+ * @powered_up: Power state set by the holder of the descriptor (not necessarily
+ * corresponding to the actual power state of the device).
+ */
+struct pwrseq_desc {
+	struct pwrseq_device *pwrseq;
+	bool powered_up;
+};
+
+static struct pwrseq_device *to_pwrseq_device(struct device *dev)
+{
+	return container_of(dev, struct pwrseq_device, dev);
+}
+
+static struct pwrseq_device *pwrseq_device_get(struct pwrseq_device *pwrseq)
+{
+	get_device(&pwrseq->dev);
+
+	return pwrseq;
+}
+
+static void pwrseq_device_put(struct pwrseq_device *pwrseq)
+{
+	put_device(&pwrseq->dev);
+}
+
+static struct bus_type pwrseq_bus = {
+	.name = "pwrseq",
+};
+
+static void pwrseq_release(struct device *dev)
+{
+	struct pwrseq_device *pwrseq = to_pwrseq_device(dev);
+
+	mutex_destroy(&pwrseq->state_lock);
+	ida_free(&pwrseq_ida, pwrseq->id);
+	kfree(pwrseq);
+}
+
+static const struct device_type pwrseq_device_type = {
+	.name = "power_sequencer",
+	.release = pwrseq_release,
+};
+
+/**
+ * pwrseq_device_register() - Register a new power sequencer.
+ * @config: Configuration of the new power sequencing device.
+ *
+ * The config structure is only used during the call and can be freed after
+ * the function returns. The config structure *must* have the parent device
+ * as well as the match(), power_on() and power_off() callbacks registered.
+ *
+ * Returns:
+ * Returns the address of the new pwrseq device or ERR_PTR() on failure.
+ */
+struct pwrseq_device *pwrseq_device_register(struct pwrseq_config *config)
+{
+	struct pwrseq_device *pwrseq;
+	int ret;
+
+	/*
+	 * Power sequencer must have a parent device and at least the power-on,
+	 * power-off and match callbacks.
+	 */
+	if (!config->parent || !config->match || !config->power_on ||
+	    !config->power_off)
+		return ERR_PTR(-EINVAL);
+
+	pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL);
+	if (!pwrseq)
+		return ERR_PTR(-ENOMEM);
+
+	pwrseq->dev.type = &pwrseq_device_type;
+	pwrseq->dev.bus = &pwrseq_bus;
+	pwrseq->dev.parent = config->parent;
+	device_set_node(&pwrseq->dev, dev_fwnode(config->parent));
+
+	pwrseq->id = ida_alloc(&pwrseq_ida, GFP_KERNEL);
+	if (pwrseq->id < 0) {
+		kfree(pwrseq);
+		return ERR_PTR(pwrseq->id);
+	}
+
+	/*
+	 * From this point onwards the device's release() callback is
+	 * responsible for freeing resources.
+	 */
+	device_initialize(&pwrseq->dev);
+
+	ret = dev_set_name(&pwrseq->dev, "pwrseq.%d", pwrseq->id);
+	if (ret)
+		goto err_put_pwrseq;
+
+	pwrseq->owner = config->owner ?: THIS_MODULE;
+	pwrseq->drvdata = config->drvdata;
+	pwrseq->match = config->match;
+	pwrseq->power_on = config->power_on;
+	pwrseq->power_off = config->power_off;
+
+	init_rwsem(&pwrseq->dev_sem);
+	mutex_init(&pwrseq->state_lock);
+
+	scoped_guard(rwsem_write, &pwrseq_sem) {
+		ret = device_add(&pwrseq->dev);
+		if (ret)
+			goto err_put_pwrseq;
+	}
+
+	return pwrseq;
+
+err_put_pwrseq:
+	pwrseq_device_put(pwrseq);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(pwrseq_device_register);
+
+/**
+ * pwrseq_device_unregister() - Unregister the power sequencer.
+ * @pwrseq: Power sequencer to unregister.
+ */
+void pwrseq_device_unregister(struct pwrseq_device *pwrseq)
+{
+	struct device *dev = &pwrseq->dev;
+
+	scoped_guard(mutex, &pwrseq->state_lock) {
+		WARN_ONCE(pwrseq->pwrup_count > 0,
+			  "%s: UNREGISTERING POWER SEQUENCER WITH ACTIVE USERS\n",
+			  dev_name(&pwrseq->dev));
+
+		scoped_guard(rwsem_write, &pwrseq_sem) {
+			scoped_guard(rwsem_write, &pwrseq->dev_sem)
+				device_del(dev);
+		}
+	}
+
+	pwrseq_device_put(pwrseq);
+}
+EXPORT_SYMBOL_GPL(pwrseq_device_unregister);
+
+static void devm_pwrseq_device_unregister(void *data)
+{
+	struct pwrseq_device *pwrseq = data;
+
+	pwrseq_device_unregister(pwrseq);
+}
+
+/**
+ * devm_pwrseq_device_register() - Managed variant of pwrseq_device_register().
+ * @dev: Managing device.
+ * @config: Configuration of the new power sequencing device.
+ *
+ * Returns:
+ * Returns the address of the new pwrseq device or ERR_PTR() on failure.
+ */
+struct pwrseq_device *
+devm_pwrseq_device_register(struct device *dev, struct pwrseq_config *config)
+{
+	struct pwrseq_device *pwrseq;
+	int ret;
+
+	pwrseq = pwrseq_device_register(config);
+	if (IS_ERR(pwrseq))
+		return pwrseq;
+
+	ret = devm_add_action_or_reset(dev, devm_pwrseq_device_unregister,
+				       pwrseq);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return pwrseq;
+}
+EXPORT_SYMBOL_GPL(devm_pwrseq_device_register);
+
+/**
+ * pwrseq_device_get_data() - Get the driver private data associated with this
+ *                            sequencer.
+ * @pwrseq: Power sequencer object.
+ *
+ * Returns:
+ * Address of the private driver data.
+ */
+void *pwrseq_device_get_data(struct pwrseq_device *pwrseq)
+{
+	return pwrseq->drvdata;
+}
+EXPORT_SYMBOL_GPL(pwrseq_device_get_data);
+
+struct pwrseq_match_data {
+	struct pwrseq_device *matched;
+	struct device *dev;
+};
+
+static int pwrseq_match_device(struct device *pwrseq_dev, void *data)
+{
+	struct pwrseq_device *pwrseq = to_pwrseq_device(pwrseq_dev);
+	struct pwrseq_match_data *match_data = data;
+	int ret;
+
+	guard(rwsem_read)(&pwrseq->dev_sem);
+	if (!device_is_registered(&pwrseq->dev))
+		return 0;
+
+	ret = pwrseq->match(pwrseq, match_data->dev);
+	if (ret <= 0)
+		return ret;
+
+	match_data->matched = pwrseq;
+
+	return 1;
+}
+
+/**
+ * pwrseq_get() - Get the power sequencer associated with this device.
+ * @dev: Device for which to get the sequencer.
+ *
+ * Returns:
+ * New power sequencer descriptor for use by the consumer driver or ERR_PTR()
+ * on failure.
+ */
+struct pwrseq_desc *pwrseq_get(struct device *dev)
+{
+	struct pwrseq_match_data match_data;
+	struct pwrseq_device *pwrseq;
+	int ret;
+
+	struct pwrseq_desc *desc __free(kfree) = kzalloc(sizeof(*desc),
+							 GFP_KERNEL);
+	if (!desc)
+		return ERR_PTR(-ENOMEM);
+
+	match_data.matched = NULL;
+	match_data.dev = dev;
+
+	guard(rwsem_read)(&pwrseq_sem);
+
+	ret = bus_for_each_dev(&pwrseq_bus, NULL, &match_data,
+			       pwrseq_match_device);
+	if (ret < 0)
+		return ERR_PTR(ret);
+	if (ret == 0)
+		/* No device matched. */
+		return ERR_PTR(-EPROBE_DEFER);
+
+	pwrseq = match_data.matched;
+
+	if (!try_module_get(pwrseq->owner))
+		return ERR_PTR(-EPROBE_DEFER);
+
+	desc->pwrseq = pwrseq_device_get(pwrseq);
+
+	return no_free_ptr(desc);
+}
+EXPORT_SYMBOL_GPL(pwrseq_get);
+
+/**
+ * pwrseq_put() - Release the power sequencer descriptor.
+ * @desc: Descriptor to release.
+ */
+void pwrseq_put(struct pwrseq_desc *desc)
+{
+	struct pwrseq_device *pwrseq;
+
+	if (!desc)
+		return;
+
+	pwrseq = desc->pwrseq;
+
+	if (desc->powered_up)
+		pwrseq_power_off(desc);
+
+	kfree(desc);
+	module_put(pwrseq->owner);
+	pwrseq_device_put(pwrseq);
+}
+EXPORT_SYMBOL_GPL(pwrseq_put);
+
+static void devm_pwrseq_put(void *data)
+{
+	struct pwrseq_desc *desc = data;
+
+	pwrseq_put(desc);
+}
+
+/**
+ * devm_pwrseq_get() - Managed variant of pwrseq_get().
+ * @dev: Device for which to get the sequencer and which also manages its
+ *       lifetime.
+ *
+ * Returns:
+ * New power sequencer descriptor for use by the consumer driver or ERR_PTR()
+ * on failure.
+ */
+struct pwrseq_desc *devm_pwrseq_get(struct device *dev)
+{
+	struct pwrseq_desc *desc;
+	int ret;
+
+	desc = pwrseq_get(dev);
+	if (IS_ERR(desc))
+		return desc;
+
+	ret = devm_add_action_or_reset(dev, devm_pwrseq_put, desc);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return desc;
+}
+EXPORT_SYMBOL_GPL(devm_pwrseq_get);
+
+/**
+ * pwrseq_power_on() - Issue a power-on request on behalf of the consumer
+ *                     device.
+ * @desc: Descriptor referencing the power sequencer.
+ *
+ * This function tells the power sequencer that the consumer wants to be
+ * powered-up. The sequencer may already have powered-up the device in which
+ * case the function returns 0. If the power-up sequence is already in
+ * progress, the function will block until it's done and return 0. If this is
+ * the first request, the device will be powered up.
+ *
+ * Returns:
+ * 0 on success, negative error number on failure.
+ */
+int pwrseq_power_on(struct pwrseq_desc *desc)
+{
+	struct pwrseq_device *pwrseq;
+	int ret;
+
+	might_sleep();
+
+	if (!desc || desc->powered_up)
+		return 0;
+
+	pwrseq = desc->pwrseq;
+
+	guard(rwsem_read)(&pwrseq->dev_sem);
+	if (!device_is_registered(&pwrseq->dev))
+		return -ENODEV;
+
+	guard(mutex)(&pwrseq->state_lock);
+
+	pwrseq->pwrup_count++;
+	if (pwrseq->pwrup_count != 1) {
+		desc->powered_up = true;
+		return 0;
+	}
+
+	ret = pwrseq->power_on(pwrseq);
+	if (!ret)
+		desc->powered_up = true;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwrseq_power_on);
+
+/**
+ * pwrseq_power_off() - Issue a power-off request on behalf of the consumer
+ *                      device.
+ * @desc: Descriptor referencing the power sequencer.
+ *
+ * This undoes the effects of pwrseq_power_on(). It issues a power-off request
+ * on behalf of the consumer and when the last remaining user does so, the
+ * power-down sequence will be started. If one is in progress, the function
+ * will block until it's complete and then return.
+ *
+ * Returns:
+ * 0 on success, negative error number on failure.
+ */
+int pwrseq_power_off(struct pwrseq_desc *desc)
+{
+	struct pwrseq_device *pwrseq;
+	int ret;
+
+	might_sleep();
+
+	if (!desc || !desc->powered_up)
+		return 0;
+
+	pwrseq = desc->pwrseq;
+
+	guard(rwsem_read)(&pwrseq->dev_sem);
+	if (!device_is_registered(&pwrseq->dev))
+		return -ENODEV;
+
+	guard(mutex)(&pwrseq->state_lock);
+
+	if (pwrseq->pwrup_count == 0) {
+		WARN_ONCE(1, "Unmatched power-off\n");
+		return -EBUSY;
+	}
+
+	pwrseq->pwrup_count--;
+	if (pwrseq->pwrup_count != 0) {
+		desc->powered_up = false;
+		return 0;
+	}
+
+	ret = pwrseq->power_off(pwrseq);
+	if (!ret)
+		desc->powered_up = false;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwrseq_power_off);
+
+static int __init pwrseq_init(void)
+{
+	int ret;
+
+	ret = bus_register(&pwrseq_bus);
+	if (ret) {
+		pr_err("Failed to register the power sequencer bus\n");
+		return ret;
+	}
+
+	return 0;
+}
+subsys_initcall(pwrseq_init);
+
+static void __exit pwrseq_exit(void)
+{
+	bus_unregister(&pwrseq_bus);
+}
+module_exit(pwrseq_exit);
+
+MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>");
+MODULE_DESCRIPTION("Power Sequencing subsystem core");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/pwrseq/consumer.h b/include/linux/pwrseq/consumer.h
new file mode 100644
index 000000000000..f207b8b2864d
--- /dev/null
+++ b/include/linux/pwrseq/consumer.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#ifndef __POWER_SEQUENCING_CONSUMER_H__
+#define __POWER_SEQUENCING_CONSUMER_H__
+
+#include <linux/err.h>
+
+struct device;
+struct pwrseq_desc;
+
+#if IS_ENABLED(CONFIG_POWER_SEQUENCING)
+
+struct pwrseq_desc * __must_check pwrseq_get(struct device *dev);
+void pwrseq_put(struct pwrseq_desc *desc);
+
+struct pwrseq_desc * __must_check devm_pwrseq_get(struct device *dev);
+
+int pwrseq_power_on(struct pwrseq_desc *desc);
+int pwrseq_power_off(struct pwrseq_desc *desc);
+
+#else /* CONFIG_POWER_SEQUENCING */
+
+static inline struct pwrseq_desc * __must_check pwrseq_get(struct device *dev)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline void pwrseq_put(struct pwrseq_desc *desc)
+{
+}
+
+static inline struct pwrseq_desc * __must_check
+devm_pwrseq_get(struct device *dev)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline int pwrseq_power_on(struct pwrseq_desc *desc)
+{
+	return -ENOSYS;
+}
+
+static inline int pwrseq_power_off(struct pwrseq_desc *desc)
+{
+	return -ENOSYS;
+}
+
+#endif /* CONFIG_POWER_SEQUENCING */
+
+#endif /* __POWER_SEQUENCING_CONSUMER_H__ */
diff --git a/include/linux/pwrseq/provider.h b/include/linux/pwrseq/provider.h
new file mode 100644
index 000000000000..8696a89afa43
--- /dev/null
+++ b/include/linux/pwrseq/provider.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#ifndef __POWER_SEQUENCING_PROVIDER_H__
+#define __POWER_SEQUENCING_PROVIDER_H__
+
+struct device;
+struct module;
+struct pwrseq_device;
+
+typedef int (*pwrseq_power_state_func)(struct pwrseq_device *);
+typedef int (*pwrseq_match_func)(struct pwrseq_device *, struct device *);
+
+/**
+ * struct pwrseq_config - Configuration used for registering a new provider.
+ * @parent: Parent device for the sequencer.
+ * @owner: Module providing this device.
+ * @drvdata: Private driver data.
+ * @match: Provider callback used to match the consumer device to the sequencer.
+ * @power_on: Callback running the power-on sequence.
+ * @power_off: Callback running the power-off sequence.
+ */
+struct pwrseq_config {
+	struct device *parent;
+	struct module *owner;
+	void *drvdata;
+	pwrseq_match_func match;
+	pwrseq_power_state_func power_on;
+	pwrseq_power_state_func power_off;
+};
+
+struct pwrseq_device *pwrseq_device_register(struct pwrseq_config *config);
+void pwrseq_device_unregister(struct pwrseq_device *pwrseq);
+struct pwrseq_device *
+devm_pwrseq_device_register(struct device *dev, struct pwrseq_config *config);
+
+void *pwrseq_device_get_data(struct pwrseq_device *pwrseq);
+
+#endif /* __POWER_SEQUENCING_PROVIDER_H__ */
-- 
2.40.1

Re: [RFC 3/9] power: sequencing: new subsystem

[Rob Herring]

On Thu, Feb 1, 2024 at 9:55 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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 <bartosz.golaszewski@linaro.org>

> +++ b/drivers/power/sequencing/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0-only


> diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
> new file mode 100644
> index 000000000000..dcdf8c0c159e
> --- /dev/null
> +++ b/drivers/power/sequencing/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0

GPL-2.0-only to be consistent.

> +
> +obj-$(CONFIG_POWER_SEQUENCING)         += pwrseq-core.o
> +pwrseq-core-y                          := core.o
> diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> new file mode 100644
> index 000000000000..f035caed0e4e
> --- /dev/null
> +++ b/drivers/power/sequencing/core.c
> @@ -0,0 +1,482 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

Why the deviation from the kernel's default license?

Rob

[RFC 4/9] power: pwrseq: add a driver for the QCA6390 PMU module

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This adds the power sequencing driver for the QCA6390's PMU module. It
uses the pwrseq subsystem and knows how to match the sequencer to the
consumer device by verifying the relevant properties and DT layout.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/power/sequencing/Kconfig          |  16 ++
 drivers/power/sequencing/Makefile         |   2 +
 drivers/power/sequencing/pwrseq-qca6390.c | 232 ++++++++++++++++++++++
 3 files changed, 250 insertions(+)
 create mode 100644 drivers/power/sequencing/pwrseq-qca6390.c

diff --git a/drivers/power/sequencing/Kconfig b/drivers/power/sequencing/Kconfig
index ba5732b1dbf8..84ddf3b4ae56 100644
--- a/drivers/power/sequencing/Kconfig
+++ b/drivers/power/sequencing/Kconfig
@@ -10,3 +10,19 @@ menuconfig POWER_SEQUENCING
 	  during power-up.
 
 	  If unsure, say no.
+
+if POWER_SEQUENCING
+
+config POWER_SEQUENCING_QCA6390
+	tristate "QCA6390 PMU driver"
+	default m if ARCH_QCOM
+	help
+	  Say U here to enable the power sequencing driver for Qualcomm
+	  QCA6390.
+
+	  The QCA6390 package contains the BT and WLAN modules whose power
+	  is controlled by the PMU module. As the former two share the power-up
+	  sequence which is executed by the PMU, this driver is needed for
+	  correct power control.
+
+endif
diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
index dcdf8c0c159e..628345c4e7ae 100644
--- a/drivers/power/sequencing/Makefile
+++ b/drivers/power/sequencing/Makefile
@@ -2,3 +2,5 @@
 
 obj-$(CONFIG_POWER_SEQUENCING)		+= pwrseq-core.o
 pwrseq-core-y				:= core.o
+
+obj-$(CONFIG_POWER_SEQUENCING_QCA6390)	+= pwrseq-qca6390.o
diff --git a/drivers/power/sequencing/pwrseq-qca6390.c b/drivers/power/sequencing/pwrseq-qca6390.c
new file mode 100644
index 000000000000..6c930e3e88ec
--- /dev/null
+++ b/drivers/power/sequencing/pwrseq-qca6390.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/pwrseq/provider.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+struct pwrseq_qca6390_vreg {
+	const char *name;
+	unsigned int load_uA;
+};
+
+struct pwrseq_qca6390_pdata {
+	const struct pwrseq_qca6390_vreg *vregs;
+	size_t num_vregs;
+	unsigned int pwup_delay_msec;
+};
+
+struct pwrseq_qca6390_ctx {
+	struct pwrseq_device *pwrseq;
+	struct device_node *of_node;
+	const struct pwrseq_qca6390_pdata *pdata;
+	struct regulator_bulk_data *regs;
+	struct gpio_desc *bt_gpio;
+	struct gpio_desc *wlan_gpio;
+};
+
+static const struct pwrseq_qca6390_vreg pwrseq_qca6390_vregs[] = {
+	{
+		.name = "vddio",
+		.load_uA = 20000,
+	},
+	{
+		.name = "vddaon",
+		.load_uA = 100000,
+	},
+	{
+		.name = "vddpmu",
+		.load_uA = 1250000,
+	},
+	{
+		.name = "vddpcie1",
+		.load_uA = 35000,
+	},
+	{
+		.name = "vddpcie2",
+		.load_uA = 15000,
+	},
+	{
+		.name = "vddrfa1",
+		.load_uA = 200000,
+	},
+	{
+		.name = "vddrfa2",
+		.load_uA = 400000,
+	},
+	{
+		.name = "vddrfa3",
+		.load_uA = 400000,
+	},
+};
+
+static const struct pwrseq_qca6390_pdata pwrseq_qca6390_of_data = {
+	.vregs = pwrseq_qca6390_vregs,
+	.num_vregs = ARRAY_SIZE(pwrseq_qca6390_vregs),
+	.pwup_delay_msec = 16,
+};
+
+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);
+
+	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);
+
+	gpiod_set_value_cansleep(ctx->bt_gpio, 0);
+	gpiod_set_value_cansleep(ctx->wlan_gpio, 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)
+		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;
+
+	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;
+
+			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);
+	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 <bartosz.golaszewski@linaro.org>");
+MODULE_DESCRIPTION("QCA6390 PMU power sequencing driver");
+MODULE_LICENSE("GPL");
-- 
2.40.1

Re: [RFC 4/9] power: pwrseq: add a driver for the QCA6390 PMU module

[Bjorn Andersson]

On Thu, Feb 01, 2024 at 04:55:27PM +0100, Bartosz Golaszewski wrote:
> diff --git a/drivers/power/sequencing/pwrseq-qca6390.c b/drivers/power/sequencing/pwrseq-qca6390.c
[..]
> +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?

> +
> +	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);
> +
> +	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.

> +	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)
> +		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.

> +
> +	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;
> +
> +			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

> +	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 <bartosz.golaszewski@linaro.org>");
> +MODULE_DESCRIPTION("QCA6390 PMU power sequencing driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.40.1
> 

Re: [RFC 4/9] power: pwrseq: add a driver for the QCA6390 PMU module

[Dmitry Baryshkov]

On Fri, 2 Feb 2024 at 06:54, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Feb 01, 2024 at 04:55:27PM +0100, Bartosz Golaszewski wrote:
> > diff --git a/drivers/power/sequencing/pwrseq-qca6390.c b/drivers/power/sequencing/pwrseq-qca6390.c
> [..]
> > +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.

>
> > +
> > +     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);
> > +
> > +     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.


>
> 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.
>
> > +     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)
> > +             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.
>
> > +
> > +     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;
> > +
> > +                     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
>
> > +     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 <bartosz.golaszewski@linaro.org>");
> > +MODULE_DESCRIPTION("QCA6390 PMU power sequencing driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.40.1
> >
>


--
With best wishes
Dmitry

Re: [RFC 4/9] power: pwrseq: add a driver for the QCA6390 PMU module

[Bartosz Golaszewski]

On Fri, Feb 2, 2024 at 8:48 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 2 Feb 2024 at 06:54, Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Thu, Feb 01, 2024 at 04:55:27PM +0100, Bartosz Golaszewski wrote:
> > > diff --git a/drivers/power/sequencing/pwrseq-qca6390.c b/drivers/power/sequencing/pwrseq-qca6390.c
> > [..]
> > > +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.
>

Makes sense, I'll think about it. I'm thinking about adding a flags
argument for this kind of switching.

> >
> > > +
> > > +     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);
> > > +
> > > +     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.
>

The docs for QCA6390 also mention the 100ms delay but it doesn't seem
to be necessary. But yes, this was done after Dmitry raised concerns
about the QCA6490.

Bart

>
> >
> > 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.
> >
> > > +     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)
> > > +             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.
> >
> > > +
> > > +     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;
> > > +
> > > +                     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
> >
> > > +     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 <bartosz.golaszewski@linaro.org>");
> > > +MODULE_DESCRIPTION("QCA6390 PMU power sequencing driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.40.1
> > >
> >
>
>
> --
> With best wishes
> Dmitry

[RFC 5/9] Bluetooth: qca: use the power sequencer for QCA6390

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use the pwrseq subsystem's consumer API to run the power-up sequence for
the Bluetooth module of the QCA6390 package.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/bluetooth/hci_qca.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 94b8c406f0c0..21c306caafbc 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -28,6 +28,7 @@
 #include <linux/of.h>
 #include <linux/acpi.h>
 #include <linux/platform_device.h>
+#include <linux/pwrseq/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/serdev.h>
 #include <linux/mutex.h>
@@ -214,6 +215,7 @@ struct qca_power {
 	struct regulator_bulk_data *vreg_bulk;
 	int num_vregs;
 	bool vregs_on;
+	struct pwrseq_desc *pwrseq;
 };
 
 struct qca_serdev {
@@ -1791,6 +1793,11 @@ static int qca_power_on(struct hci_dev *hdev)
 		ret = qca_regulator_init(hu);
 		break;
 
+	case QCA_QCA6390:
+		qcadev = serdev_device_get_drvdata(hu->serdev);
+		ret = pwrseq_power_on(qcadev->bt_power->pwrseq);
+		break;
+
 	default:
 		qcadev = serdev_device_get_drvdata(hu->serdev);
 		if (qcadev->bt_en) {
@@ -2160,6 +2167,10 @@ static void qca_power_shutdown(struct hci_uart *hu)
 		}
 		break;
 
+	case QCA_QCA6390:
+		pwrseq_power_off(qcadev->bt_power->pwrseq);
+		break;
+
 	default:
 		gpiod_set_value_cansleep(qcadev->bt_en, 0);
 	}
@@ -2298,12 +2309,25 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6390:
 		qcadev->bt_power = devm_kzalloc(&serdev->dev,
 						sizeof(struct qca_power),
 						GFP_KERNEL);
 		if (!qcadev->bt_power)
 			return -ENOMEM;
+		break;
+	default:
+		break;
+	}
 
+	switch (qcadev->btsoc_type) {
+	case QCA_WCN3988:
+	case QCA_WCN3990:
+	case QCA_WCN3991:
+	case QCA_WCN3998:
+	case QCA_WCN6750:
+	case QCA_WCN6855:
+	case QCA_WCN7850:
 		qcadev->bt_power->dev = &serdev->dev;
 		err = qca_init_regulators(qcadev->bt_power, data->vregs,
 					  data->num_vregs);
@@ -2344,6 +2368,12 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		}
 		break;
 
+	case QCA_QCA6390:
+		qcadev->bt_power->pwrseq = devm_pwrseq_get(&serdev->dev);
+		if (IS_ERR(qcadev->bt_power->pwrseq))
+			return PTR_ERR(qcadev->bt_power->pwrseq);
+		fallthrough;
+
 	default:
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
-- 
2.40.1

[RFC 6/9] PCI: create platform devices for child OF nodes of the port node

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <bartosz.golaszewski@linaro.org>
---
 drivers/pci/bus.c    | 9 ++++++++-
 drivers/pci/remove.c | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 826b5016a101..17ab41094c4e 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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>
 
@@ -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);
+		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);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index d749ea8250d6..fc9db2805888 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -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

Re: [RFC 6/9] PCI: create platform devices for child OF nodes of the port node

[Bjorn Andersson]

On Thu, Feb 01, 2024 at 04:55:29PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> In order to introduce PCI power-sequencing,

Please provide a proper problem description.

> we need to create platform

And properly express why this is a "need".

> 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.

> (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.

> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/pci/bus.c    | 9 ++++++++-
>  drivers/pci/remove.c | 2 ++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 826b5016a101..17ab41094c4e 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -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>
>  
> @@ -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

> +		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);
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index d749ea8250d6..fc9db2805888 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -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
> 

Re: [RFC 6/9] PCI: create platform devices for child OF nodes of the port node

[Bartosz Golaszewski]

On Fri, Feb 2, 2024 at 3:59 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Feb 01, 2024 at 04:55:29PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > In order to introduce PCI power-sequencing,
>
> Please provide a proper problem description.
>
> > we need to create platform
>
> And properly express why this is a "need".
>
> > 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.
>
> > (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.
>

Noted all of the above. Thanks!

> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/pci/bus.c    | 9 ++++++++-
> >  drivers/pci/remove.c | 2 ++
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 826b5016a101..17ab41094c4e 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -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>
> >
> > @@ -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?
>

I'll clarify that. I'm not that well versed with PCI code either but
will get help from the right people.

Bart

> Regards,
> Bjorn
>
> > +             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);
> > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> > index d749ea8250d6..fc9db2805888 100644
> > --- a/drivers/pci/remove.c
> > +++ b/drivers/pci/remove.c
> > @@ -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
> >

[RFC 7/9] PCI: hold the rescan mutex when scanning for the first time

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With the introduction of the power sequencing drivers that will be able
to trigger the port rescan, we need to hold the rescan mutex during the
initial pci_host_probe() too or the two could get in each other's way.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/probe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b7335be56008..957f7afee7ba 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3122,7 +3122,9 @@ int pci_host_probe(struct pci_host_bridge *bridge)
 	struct pci_bus *bus, *child;
 	int ret;
 
+	pci_lock_rescan_remove();
 	ret = pci_scan_root_bus_bridge(bridge);
+	pci_unlock_rescan_remove();
 	if (ret < 0) {
 		dev_err(bridge->dev.parent, "Scanning root bridge failed");
 		return ret;
-- 
2.40.1

[RFC 8/9] PCI/pwrctl: add PCI power control core code

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <bartosz.golaszewski@linaro.org>
---
 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

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 74147262625b..5b9b84f8774f 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -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
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index cc8b4e01e29d..6ae202d950f8 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -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
diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
new file mode 100644
index 000000000000..e2dc5e5d2af1
--- /dev/null
+++ b/drivers/pci/pwrctl/Kconfig
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0
+
+menu "PCI Power control drivers"
+
+config PCI_PWRCTL
+	bool
+
+endmenu
diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
new file mode 100644
index 000000000000..4381cfbf3f21
--- /dev/null
+++ b/drivers/pci/pwrctl/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_PCI_PWRCTL)		+= core.o
diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
new file mode 100644
index 000000000000..312e6fe95c31
--- /dev/null
+++ b/drivers/pci/pwrctl/core.c
@@ -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)
+{
+	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);
+		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);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl)
+{
+	if (!pwrctl->dev)
+		return -ENODEV;
+
+	pwrctl->nb.notifier_call = pci_pwrctl_notify;
+	bus_register_notifier(&pci_bus_type, &pwrctl->nb);
+
+	pci_lock_rescan_remove();
+	pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
+	pci_unlock_rescan_remove();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_pwrctl_device_enable);
+
+void pci_pwrctl_device_disable(struct pci_pwrctl *pwrctl)
+{
+	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);
diff --git a/include/linux/pci-pwrctl.h b/include/linux/pci-pwrctl.h
new file mode 100644
index 000000000000..8d16d27cbfeb
--- /dev/null
+++ b/include/linux/pci-pwrctl.h
@@ -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__ */
-- 
2.40.1

Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Bjorn Andersson]

On Thu, Feb 01, 2024 at 04:55:31PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <bartosz.golaszewski@linaro.org>
> ---
>  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
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 74147262625b..5b9b84f8774f 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -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
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index cc8b4e01e29d..6ae202d950f8 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -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
> diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
> new file mode 100644
> index 000000000000..e2dc5e5d2af1
> --- /dev/null
> +++ b/drivers/pci/pwrctl/Kconfig
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +menu "PCI Power control drivers"
> +
> +config PCI_PWRCTL
> +	bool
> +
> +endmenu
> diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
> new file mode 100644
> index 000000000000..4381cfbf3f21
> --- /dev/null
> +++ b/drivers/pci/pwrctl/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_PCI_PWRCTL)		+= core.o
> diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
> new file mode 100644
> index 000000000000..312e6fe95c31
> --- /dev/null
> +++ b/drivers/pci/pwrctl/core.c
> @@ -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)
> +{
> +	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)

> +		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...

> +		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"...

> +{
> +	if (!pwrctl->dev)
> +		return -ENODEV;
> +
> +	pwrctl->nb.notifier_call = pci_pwrctl_notify;
> +	bus_register_notifier(&pci_bus_type, &pwrctl->nb);
> +
> +	pci_lock_rescan_remove();
> +	pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
> +	pci_unlock_rescan_remove();
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_pwrctl_device_enable);
> +
> +void pci_pwrctl_device_disable(struct pci_pwrctl *pwrctl)

Similarly this doesn't disable the device, the code calling this is
doing so...

Regards,
Bjorn

> +{
> +	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);
> diff --git a/include/linux/pci-pwrctl.h b/include/linux/pci-pwrctl.h
> new file mode 100644
> index 000000000000..8d16d27cbfeb
> --- /dev/null
> +++ b/include/linux/pci-pwrctl.h
> @@ -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__ */
> -- 
> 2.40.1
> 

Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Bartosz Golaszewski]

On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote:
>

[snip]

> > +
> > +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)

That's exactly what will happen and the end result will be the same.

>
> > +             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...
>

Yes, I'll address it.

> > +             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!]

Re: Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Bjorn Andersson]

On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote:
[..]
> > > +             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().

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()?

Regards,
Bjorn

> 
> Bart
> 
> [snip!]

Re: Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Bartosz Golaszewski]

On Fri, Feb 2, 2024 at 5:52 PM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote:
> [..]
> > > > +             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().
>
> 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()?
>

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?

Bart

Re: Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Lukas Wunner]

On Wed, Feb 07, 2024 at 05:26:16PM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 2, 2024 at 5:52???PM Bjorn Andersson <andersson@kernel.org> wrote:
> > On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote:
> > > 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()?
> 
> 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

Re: Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Manivannan Sadhasivam]

On Fri, Feb 09, 2024 at 10:04:33AM +0100, Lukas Wunner wrote:
> On Wed, Feb 07, 2024 at 05:26:16PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Feb 2, 2024 at 5:52???PM Bjorn Andersson <andersson@kernel.org> wrote:
> > > On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote:
> > > > 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()?
> > 
> > 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.
> 

This will work if the sole control of the resources lies in these platform_*()
APIs. But in reality, the controller drivers are the ones controlling the power
supply to the devices  and with this series, the control would be shifted partly
to pwrctl driver.

I think what we need is to call in the callbacks of the drivers in a hierarchial
order.

- Mani

> 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

-- 
மணிவண்ணன் சதாசிவம்

Re: Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Manivannan Sadhasivam]

On Fri, Feb 02, 2024 at 10:52:11AM -0600, Bjorn Andersson wrote:
> On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote:
> [..]
> > > > +             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().
> 
> 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()?
> 

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.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: Re: Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Bjorn Andersson]

On Thu, Feb 08, 2024 at 05:02:01PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Feb 02, 2024 at 10:52:11AM -0600, Bjorn Andersson wrote:
> > On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote:
> > [..]
> > > > > +             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().
> > 
> > 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()?
> > 
> 
> 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.

Regards,
Bjorn

Re: Re: Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Manivannan Sadhasivam]

On Fri, Feb 09, 2024 at 05:43:56PM -0600, Bjorn Andersson wrote:
> On Thu, Feb 08, 2024 at 05:02:01PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Feb 02, 2024 at 10:52:11AM -0600, Bjorn Andersson wrote:
> > > On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote:
> > > [..]
> > > > > > +             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().
> > > 
> > > 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()?
> > > 
> > 
> > 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.
> 

Agree. I'm not ignoring this issue, but this is a separate topic IMO (or an
incremental change). Because, power cycling the device in the event of a
firmware crash or even upon receiving AER Fatal errors is valid for platforms
not making use of this driver and an existing issue.

For sure we can accomodate that functionality in this series itself, but that's
going to drag this series to many releases (you already know how long it took
for us to get to the current state). Instead, I'd recommend to merge it in its
current form and have Bartosz or someone work on incremental features such as:

1. Runtime/System PM
2. Resetting the device in the event of fw crash etc...

Wdyt?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [RFC 8/9] PCI/pwrctl: add PCI power control core code

[Bartosz Golaszewski]

On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote:
>

[snip]

>
> > +             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.
>

Right. And it also makes me think that right after registering with
the notifier, we should iterate over the PCI bus devices and see if
the relevant one is already there.

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

Right.

Bart

[snip]

[RFC 9/9] PCI/pwrctl: add a PCI power control driver for power sequenced devices

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <bartosz.golaszewski@linaro.org>
---
 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

diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
index e2dc5e5d2af1..bca72dc08e79 100644
--- a/drivers/pci/pwrctl/Kconfig
+++ b/drivers/pci/pwrctl/Kconfig
@@ -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
diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
index 4381cfbf3f21..919c0f704ee9 100644
--- a/drivers/pci/pwrctl/Makefile
+++ b/drivers/pci/pwrctl/Makefile
@@ -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
diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
new file mode 100644
index 000000000000..510598c4edc4
--- /dev/null
+++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
@@ -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 <bartosz.golaszewski@linaro.org>");
+MODULE_DESCRIPTION("Generic PCI Power Control module for power sequenced devices");
+MODULE_LICENSE("GPL");
-- 
2.40.1

Re: [RFC 9/9] PCI/pwrctl: add a PCI power control driver for power sequenced devices

[Bjorn Andersson]

On Thu, Feb 01, 2024 at 04:55:32PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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).

Regards,
Bjorn

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  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
> 
> diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
> index e2dc5e5d2af1..bca72dc08e79 100644
> --- a/drivers/pci/pwrctl/Kconfig
> +++ b/drivers/pci/pwrctl/Kconfig
> @@ -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
> diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
> index 4381cfbf3f21..919c0f704ee9 100644
> --- a/drivers/pci/pwrctl/Makefile
> +++ b/drivers/pci/pwrctl/Makefile
> @@ -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
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> new file mode 100644
> index 000000000000..510598c4edc4
> --- /dev/null
> +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> @@ -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 <bartosz.golaszewski@linaro.org>");
> +MODULE_DESCRIPTION("Generic PCI Power Control module for power sequenced devices");
> +MODULE_LICENSE("GPL");
> -- 
> 2.40.1
> 

Re: [RFC 9/9] PCI/pwrctl: add a PCI power control driver for power sequenced devices

[Bartosz Golaszewski]

On Fri, Feb 2, 2024 at 5:03 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Feb 01, 2024 at 04:55:32PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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).

> 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.

> 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!

>
> 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/20240117160748.37682-7-brgl@bgdev.pl/T/#m72f52254a52fcb8a8a44de0702cad1087d4bcfa1

Re: Re: [RFC 9/9] PCI/pwrctl: add a PCI power control driver for power sequenced devices

[Bjorn Andersson]

On Fri, Feb 02, 2024 at 02:05:59PM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 2, 2024 at 5:03 AM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Thu, Feb 01, 2024 at 04:55:32PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > 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).
> 
> > 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.
> 

If the PMU registers the "internal" output regulators, then PCI device
would consume the PCI outputs of the PMU, the BT device would consume
the BT outputs of the PMU. The PMU requests inputs enabled and drives
BT_EN and WLAN_EN according to which subset of these output regulators
are enabled.

Pretty much exactly as "regulator-fixes" isn't a pwrseq device.

Regards,
Bjorn

> > 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!
> 
> >
> > 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/20240117160748.37682-7-brgl@bgdev.pl/T/#m72f52254a52fcb8a8a44de0702cad1087d4bcfa1

Re: [RFC 0/9] power: sequencing: implement the subsystem and add first users

[Bjorn Andersson]

On Thu, Feb 01, 2024 at 04:55:23PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 

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...

> 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.
> 

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

Regards,
Bjorn

> [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
> 

Re: [RFC 0/9] power: sequencing: implement the subsystem and add first users

[Bartosz Golaszewski]

On Fri, Feb 2, 2024 at 1:40 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Feb 01, 2024 at 04:55:23PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
>
> 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...
>

So there was an RFC for the PCI power sequencing (now renamed to PCI
power control - pci_pwrctl), then a proper series for it (with changes
listed). Then an RFC with just proposed DT changes (sent mostly to DT
maintainers to clear it with them) and now an RFC with power
sequencing + PCI power control. Hard to figure out how to version it
if these are pretty much separate developments.

I'll provide links to everything next time.

> > 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.
> >
>
> How can we tell? There are still no Documentation/dt-bindings changes in
> your series.

Noted.

Bartosz

>
> Regards,
> Bjorn
>
> > [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
> >

Re: [RFC 0/9] power: sequencing: implement the subsystem and add first users

[Bjorn Andersson]

On Thu, Feb 01, 2024 at 04:55:23PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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

FWIW, booting RB5 with this patch series does give me working working
WiFI. But I do see the following splat during boot:

[    5.880411] sysfs: cannot create duplicate filename '/devices/platform/soc@0/1c00000.pcie/pci0000:00/0000:00:00.0/resource0'
[    5.891938] CPU: 5 PID: 68 Comm: kworker/u16:4 Not tainted 6.8.0-rc2-next-20240131-00009-g079fdad54c8f #199
[    5.901927] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
[    5.908808] Workqueue: events_unbound async_run_entry_fn
[    5.914274] Call trace:
[    5.916794]  dump_backtrace+0xec/0x108
[    5.920649]  show_stack+0x18/0x24
[    5.924062]  dump_stack_lvl+0x50/0x68
[    5.927826]  dump_stack+0x18/0x24
[    5.931238]  sysfs_warn_dup+0x64/0x80
[    5.935004]  sysfs_create_bin_file+0xf4/0x130
[    5.939480]  pci_create_attr+0x100/0x168
[    5.943509]  pci_create_sysfs_dev_files+0x6c/0xc0
[    5.948337]  pci_bus_add_device+0x60/0x114
[    5.952551]  pci_bus_add_devices+0x4c/0x7c
[    5.956762]  pci_host_probe+0x138/0x188
[    5.960700]  dw_pcie_host_init+0x290/0x334
[    5.964914]  qcom_pcie_probe+0x1f8/0x23c
[    5.968942]  platform_probe+0xa8/0xd0
[    5.972707]  really_probe+0x130/0x2e4
[    5.976469]  __driver_probe_device+0xa0/0x128
[    5.980944]  driver_probe_device+0x3c/0x1f8
[    5.985245]  __device_attach_driver+0x118/0x140
[    5.989897]  bus_for_each_drv+0xf4/0x14c
[    5.993923]  __device_attach_async_helper+0x78/0xd0
[    5.998937]  async_run_entry_fn+0x24/0xdc
[    6.003051]  process_scheduled_works+0x210/0x328
[    6.007793]  worker_thread+0x28c/0x450
[    6.011642]  kthread+0xfc/0x184
[    6.014865]  ret_from_fork+0x10/0x20
[    6.018572] ------------[ cut here ]------------
[    6.023339] proc_dir_entry '0000:00/00.0' already registered

Regards,
Bjorn