[PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Bartosz Golaszewski]

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

The responses to the RFC were rather positive so here's a proper series.

During last year's Linux Plumbers we had several discussions centered
around the need to power-on PCI devices before they can be detected on
the bus.

The consensus during the conference was that we need to introduce a
class of "PCI slot drivers" that would handle the power-sequencing.

After some additional brain-storming with Manivannan and the realization
that DT maintainers won't like adding any "fake" nodes not representing
actual devices, we decided to reuse existing PCI infrastructure.

The general idea is to instantiate platform devices for child nodes of
the PCIe port DT node. 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 this approach is not modifying the existing DT in
any way and especially not adding any "fake" platform devices.

Changes since RFC:
- move the pwrseq functionality out of the port driver and into PCI core
- add support for WCN7850 to the first pwrseq driver (and update bindings)
- describe the WLAN modules in sm8550-qrd and sm8650-qrd
- rework Kconfig options, drop the defconfig changes from the series as
  they are no longer needed
- drop the dt-binding changes for PCI vendor codes
- extend the DT bindings for ath11k_pci with strict property checking
- various minor tweaks and fixes

Bartosz Golaszewski (7):
  arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of 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/pwrseq: add pwrseq core code
  dt-bindings: wireless: ath11k: describe QCA6390
  dt-bindings: wireless: ath11k: describe WCN7850
  PCI/pwrseq: add a pwrseq driver for QCA6390

Neil Armstrong (2):
  arm64: dts: qcom: sm8550-qrd: add Wifi nodes
  arm64: dts: qcom: sm8650-qrd: add Wifi nodes

 .../net/wireless/qcom,ath11k-pci.yaml         |  89 ++++++
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      |  29 ++
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |  10 +
 arch/arm64/boot/dts/qcom/sm8550-qrd.dts       |  37 +++
 arch/arm64/boot/dts/qcom/sm8550.dtsi          |  10 +
 arch/arm64/boot/dts/qcom/sm8650-qrd.dts       |  29 ++
 arch/arm64/boot/dts/qcom/sm8650.dtsi          |  10 +
 drivers/pci/Kconfig                           |   1 +
 drivers/pci/Makefile                          |   1 +
 drivers/pci/bus.c                             |   9 +-
 drivers/pci/probe.c                           |   2 +
 drivers/pci/pwrseq/Kconfig                    |  16 ++
 drivers/pci/pwrseq/Makefile                   |   4 +
 drivers/pci/pwrseq/pci-pwrseq-qca6390.c       | 267 ++++++++++++++++++
 drivers/pci/pwrseq/pwrseq.c                   |  82 ++++++
 drivers/pci/remove.c                          |   3 +-
 include/linux/pci-pwrseq.h                    |  24 ++
 17 files changed, 621 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pci/pwrseq/Kconfig
 create mode 100644 drivers/pci/pwrseq/Makefile
 create mode 100644 drivers/pci/pwrseq/pci-pwrseq-qca6390.c
 create mode 100644 drivers/pci/pwrseq/pwrseq.c
 create mode 100644 include/linux/pci-pwrseq.h

-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/9] arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of QCA6390

[Bartosz Golaszewski]

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

Describe the ath11k WLAN on-board the QCA6390 module. Include the
relevant regulators and the enable GPIO.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 29 ++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sm8250.dtsi     | 10 ++++++++
 2 files changed, 39 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
index cd0db4f31d4a..35a5d1ee45e5 100644
--- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
+++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
@@ -734,6 +734,27 @@ &pcie0_phy {
 	vdda-pll-supply = <&vreg_l9a_1p2>;
 };
 
+&pcieport0 {
+	wifi@0 {
+		compatible = "pci17cb,1101";
+		reg = <0x10000 0x0 0x0 0x0 0x0>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&wlan_en_state>;
+
+		enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
+
+		vddio-supply = <&vreg_s4a_1p8>;
+		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>;
+	};
+};
+
 &pcie1 {
 	status = "okay";
 };
@@ -1303,6 +1324,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 {
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 760501c1301a..fef9c314ce55 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2197,6 +2197,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/9] arm64: dts: qcom: sm8550-qrd: add Wifi nodes

[Bartosz Golaszewski]

From: Neil Armstrong <neil.armstrong@linaro.org>

Describe the ath12k WLAN on-board the WCN7850 module present on the
board.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
[Bartosz:
  - move the pcieport0 node into the .dtsi
  - make regulator naming consistent with existing DT code
  - add commit message]
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 37 +++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sm8550.dtsi    | 10 +++++++
 2 files changed, 47 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
index d401d63e5c4d..c07e2ea1c95c 100644
--- a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
@@ -813,6 +813,25 @@ &pcie0 {
 	status = "okay";
 };
 
+&pcieport0 {
+	wifi@0 {
+		compatible = "pci17cb,1107";
+		reg = <0x10000 0x0 0x0 0x0 0x0>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&wlan_en>, <&pmk8550_sleep_clk>;
+
+		enable-gpios = <&tlmm 80 GPIO_ACTIVE_HIGH>;
+
+		vddio-supply = <&vreg_l15b_1p8>;
+		vdd-supply = <&vreg_s5g_0p85>;
+		vddaon-supply = <&vreg_s2g_0p85>;
+		vdddig-supply = <&vreg_s4e_0p95>;
+		vddrfa1-supply = <&vreg_s6g_1p86>;
+		vddrfa2-supply = <&vreg_s4g_1p25>;
+	};
+};
+
 &pcie0_phy {
 	vdda-phy-supply = <&vreg_l1e_0p88>;
 	vdda-pll-supply = <&vreg_l3e_1p2>;
@@ -900,6 +919,17 @@ &pcie_1_phy_aux_clk {
 	clock-frequency = <1000>;
 };
 
+&pmk8550_gpios {
+	pmk8550_sleep_clk: sleep-clk-state {
+		pins = "gpio3";
+		function = "func1";
+		input-disable;
+		output-enable;
+		bias-disable;
+		power-source = <0>;
+	};
+};
+
 &qupv3_id_0 {
 	status = "okay";
 };
@@ -1035,6 +1065,13 @@ wcd_default: wcd-reset-n-active-state {
 		bias-disable;
 		output-low;
 	};
+
+	wlan_en: wlan-en-state {
+		pins = "gpio80";
+		function = "gpio";
+		drive-strength = <8>;
+		bias-pull-down;
+	};
 };
 
 &uart7 {
diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index ee1ba5a8c8fc..1f2dd4262eb9 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -1754,6 +1754,16 @@ pcie0: pcie@1c00000 {
 			phy-names = "pciephy";
 
 			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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/9] arm64: dts: qcom: sm8650-qrd: add Wifi nodes

[Bartosz Golaszewski]

From: Neil Armstrong <neil.armstrong@linaro.org>

Describe the ath12k WLAN on-board the WCN7850 module present on the
board.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
[Bartosz:
  - move the pcieport0 node into the .dtsi
  - make regulator naming consistent with existing DT code
  - add commit message]
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 29 +++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sm8650.dtsi    | 10 +++++++++
 2 files changed, 39 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
index 592a67a47c78..5f960d90e7d2 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
@@ -513,6 +513,28 @@ &pcie0 {
 	status = "okay";
 };
 
+&pcieport0 {
+	wifi@0 {
+		compatible = "pci17cb,1107";
+		reg = <0x10000 0x0 0x0 0x0 0x0>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&wlan_en>;
+
+		enable-gpios = <&tlmm 16 GPIO_ACTIVE_HIGH>;
+
+		vddio12-supply = <&vreg_l3c_1p2>;
+		vddio-supply = <&vreg_l15b_1p8>;
+		vdd-supply = <&vreg_s4i_0p85>;
+		vddaon-supply = <&vreg_s2c_0p8>;
+		vdddig-supply = <&vreg_s3c_0p9>;
+		vddrfa1-supply = <&vreg_s6c_1p8>;
+		vddrfa2-supply = <&vreg_s1c_1p2>;
+
+		clocks = <&rpmhcc RPMH_RF_CLK1>;
+	};
+};
+
 &pcie0_phy {
 	vdda-phy-supply = <&vreg_l1i_0p88>;
 	vdda-pll-supply = <&vreg_l3i_1p2>;
@@ -718,6 +740,13 @@ ts_reset: ts-reset-state {
 		drive-strength = <8>;
 		bias-pull-up;
 	};
+
+	wlan_en: wlan-en-state {
+		pins = "gpio16";
+		function = "gpio";
+		drive-strength = <8>;
+		bias-pull-down;
+	};
 };
 
 &uart14 {
diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 2df77123a8c7..1da8b7844224 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -2270,6 +2270,16 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/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 | 3 ++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 9c2137dae429..8ab07f711834 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..77be0630b7b3 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)
@@ -18,11 +19,11 @@ static void pci_stop_dev(struct pci_dev *dev)
 	pci_pme_active(dev, false);
 
 	if (pci_dev_is_added(dev)) {
-
 		device_release_driver(&dev->dev);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
 		of_pci_remove_node(dev);
+		of_platform_depopulate(&dev->dev);
 
 		pci_dev_assign_added(dev, false);
 	}
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/9] PCI/pwrseq: add pwrseq 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 sequencing platform driver. If
the match succeeds, the driver is responsible for powering-up the device
and calling pcie_pwrseq_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 sequencing device.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/Kconfig         |  1 +
 drivers/pci/Makefile        |  1 +
 drivers/pci/pwrseq/Kconfig  |  8 ++++
 drivers/pci/pwrseq/Makefile |  3 ++
 drivers/pci/pwrseq/pwrseq.c | 82 +++++++++++++++++++++++++++++++++++++
 include/linux/pci-pwrseq.h  | 24 +++++++++++
 6 files changed, 119 insertions(+)
 create mode 100644 drivers/pci/pwrseq/Kconfig
 create mode 100644 drivers/pci/pwrseq/Makefile
 create mode 100644 drivers/pci/pwrseq/pwrseq.c
 create mode 100644 include/linux/pci-pwrseq.h

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 74147262625b..e0fd5caa1ffc 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/pwrseq/Kconfig"
 
 endif
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index cc8b4e01e29d..0a1673ef2c9e 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)		+= pwrseq/
 
 ifdef CONFIG_PCI
 obj-$(CONFIG_PROC_FS)		+= proc.o
diff --git a/drivers/pci/pwrseq/Kconfig b/drivers/pci/pwrseq/Kconfig
new file mode 100644
index 000000000000..a721a8a955c3
--- /dev/null
+++ b/drivers/pci/pwrseq/Kconfig
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0
+
+menu "PCI Power sequencing drivers"
+
+config PCI_PWRSEQ
+	bool
+
+endmenu
diff --git a/drivers/pci/pwrseq/Makefile b/drivers/pci/pwrseq/Makefile
new file mode 100644
index 000000000000..4052b6bb5aa5
--- /dev/null
+++ b/drivers/pci/pwrseq/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_PCI_PWRSEQ)		+= pwrseq.o
diff --git a/drivers/pci/pwrseq/pwrseq.c b/drivers/pci/pwrseq/pwrseq.c
new file mode 100644
index 000000000000..a750c7bc6830
--- /dev/null
+++ b/drivers/pci/pwrseq/pwrseq.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-pwrseq.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+static int pci_pwrseq_notify(struct notifier_block *nb, unsigned long action,
+			     void *data)
+{
+	struct pci_pwrseq *pwrseq = container_of(nb, struct pci_pwrseq, nb);
+	struct device *dev = data;
+
+	if (dev_fwnode(dev) != dev_fwnode(pwrseq->dev))
+		return NOTIFY_DONE;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		device_set_of_node_from_dev(dev, pwrseq->dev);
+		break;
+	case BUS_NOTIFY_BOUND_DRIVER:
+		pwrseq->link = device_link_add(dev, pwrseq->dev,
+					       DL_FLAG_AUTOREMOVE_CONSUMER);
+		if (!pwrseq->link)
+			dev_err(pwrseq->dev, "Failed to add device link\n");
+		break;
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		device_link_del(pwrseq->link);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+int pci_pwrseq_device_enable(struct pci_pwrseq *pwrseq)
+{
+	if (!pwrseq->dev)
+		return -ENODEV;
+
+	pwrseq->nb.notifier_call = pci_pwrseq_notify;
+	bus_register_notifier(&pci_bus_type, &pwrseq->nb);
+
+	pci_lock_rescan_remove();
+	pci_rescan_bus(to_pci_dev(pwrseq->dev->parent)->bus);
+	pci_unlock_rescan_remove();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_pwrseq_device_enable);
+
+void pci_pwrseq_device_disable(struct pci_pwrseq *pwrseq)
+{
+	bus_unregister_notifier(&pci_bus_type, &pwrseq->nb);
+}
+EXPORT_SYMBOL_GPL(pci_pwrseq_device_disable);
+
+static void devm_pci_pwrseq_device_disable(void *data)
+{
+	struct pci_pwrseq *pwrseq = data;
+
+	pci_pwrseq_device_disable(pwrseq);
+}
+
+int devm_pci_pwrseq_device_enable(struct device *dev,
+				  struct pci_pwrseq *pwrseq)
+{
+	int ret;
+
+	ret = pci_pwrseq_device_enable(pwrseq);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_pci_pwrseq_device_disable,
+					pwrseq);
+}
+EXPORT_SYMBOL_GPL(devm_pci_pwrseq_device_enable);
diff --git a/include/linux/pci-pwrseq.h b/include/linux/pci-pwrseq.h
new file mode 100644
index 000000000000..137b82b99d1c
--- /dev/null
+++ b/include/linux/pci-pwrseq.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#ifndef __PCI_PWRSEQ_H__
+#define __PCI_PWRSEQ_H__
+
+#include <linux/notifier.h>
+
+struct device;
+
+struct pci_pwrseq {
+	struct notifier_block nb;
+	struct device *dev;
+	struct device_link *link;
+};
+
+int pci_pwrseq_device_enable(struct pci_pwrseq *pwrseq);
+void pci_pwrseq_device_disable(struct pci_pwrseq *pwrseq);
+int devm_pci_pwrseq_device_enable(struct device *dev,
+				  struct pci_pwrseq *pwrseq);
+
+#endif /* __PCI_PWRSEQ_H__ */
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/9] dt-bindings: wireless: ath11k: describe QCA6390

[Bartosz Golaszewski]

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

Describe the ath11k variant present on the QCA6390 module.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../net/wireless/qcom,ath11k-pci.yaml         | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
index 817f02a8b481..c8ec9d313d93 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
@@ -16,6 +16,7 @@ description: |
 properties:
   compatible:
     enum:
+      - pci17cb,1101  # QCA6390
       - pci17cb,1103  # WCN6855
 
   reg:
@@ -27,10 +28,57 @@ properties:
       string to uniquely identify variant of the calibration data for designs
       with colliding bus and device ids
 
+  enable-gpios:
+    description: GPIO line enabling the ATH11K module when asserted.
+    maxItems: 1
+
+  vddio-supply:
+    description: VDD_IO supply regulator handle
+
+  vddaon-supply:
+    description: VDD_AON supply regulator handle
+
+  vddpmu-supply:
+    description: VDD_PMU supply regulator handle
+
+  vddpcie1-supply:
+    description: VDD_PCIE1 supply regulator handle
+
+  vddpcie2-supply:
+    description: VDD_PCIE2 supply regulator handle
+
+  vddrfa1-supply:
+    description: VDD_RFA1 supply regulator handle
+
+  vddrfa2-supply:
+    description: VDD_RFA2 supply regulator handle
+
+  vddrfa3-supply:
+    description: VDD_RFA3 supply regulator handle
+
 required:
   - compatible
   - reg
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - pci17cb,1103
+    then:
+      properties:
+        enable-gpios: false
+        vddio-supply: false
+        vddaon-supply: false
+        vddpmu-supply: false
+        vddrfa1-supply: false
+        vddrfa2-supply: false
+        vddrfa3-supply: false
+        vddpcie1-supply: false
+        vddpcie2-supply: false
+
 additionalProperties: false
 
 examples:
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 8/9] dt-bindings: wireless: ath11k: describe WCN7850

[Bartosz Golaszewski]

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

Describe the ath11k variant present on the WCN7850 module.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../net/wireless/qcom,ath11k-pci.yaml         | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
index c8ec9d313d93..5648c855a122 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
@@ -18,6 +18,7 @@ properties:
     enum:
       - pci17cb,1101  # QCA6390
       - pci17cb,1103  # WCN6855
+      - pci17cb,1107  # WCN7850
 
   reg:
     maxItems: 1
@@ -28,13 +29,25 @@ properties:
       string to uniquely identify variant of the calibration data for designs
       with colliding bus and device ids
 
+  clocks:
+    maxItems: 1
+
   enable-gpios:
     description: GPIO line enabling the ATH11K module when asserted.
     maxItems: 1
 
+  vdd-supply:
+    description: VDD supply regulator handle
+
+  vdddig-supply:
+    description: VDD_DIG supply regulator handle
+
   vddio-supply:
     description: VDD_IO supply regulator handle
 
+  vddio12-supply:
+    description: VDD_IO12 supply regulator handle
+
   vddaon-supply:
     description: VDD_AON supply regulator handle
 
@@ -61,6 +74,18 @@ required:
   - reg
 
 allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - pci17cb,1101
+    then:
+      properties:
+        clocks: false
+        vdd-supply: false
+        vdddig-supply: false
+        vddio12-supply: false
   - if:
       properties:
         compatible:
@@ -69,7 +94,11 @@ allOf:
               - pci17cb,1103
     then:
       properties:
+        clocks: false
         enable-gpios: false
+        vdd-supply: false
+        vddio12-supply: false
+        vdddig-supply: false
         vddio-supply: false
         vddaon-supply: false
         vddpmu-supply: false
@@ -78,6 +107,18 @@ allOf:
         vddrfa3-supply: false
         vddpcie1-supply: false
         vddpcie2-supply: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - pci17cb,1107
+    then:
+      properties:
+        vddpmu-supply: false
+        vddrfa3-supply: false
+        vddpcie1-supply: false
+        vddpcie2-supply: false
 
 additionalProperties: false
 
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 9/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Bartosz Golaszewski]

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

Add a PCI power sequencing driver that's capable of correctly powering
up the ath11k module on QCA6390 and WCN7850 using the PCI pwrseq
functionality.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
[Neil: add support for WCN7850]
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/pci/pwrseq/Kconfig              |   8 +
 drivers/pci/pwrseq/Makefile             |   1 +
 drivers/pci/pwrseq/pci-pwrseq-qca6390.c | 267 ++++++++++++++++++++++++
 3 files changed, 276 insertions(+)
 create mode 100644 drivers/pci/pwrseq/pci-pwrseq-qca6390.c

diff --git a/drivers/pci/pwrseq/Kconfig b/drivers/pci/pwrseq/Kconfig
index a721a8a955c3..667c9c121f34 100644
--- a/drivers/pci/pwrseq/Kconfig
+++ b/drivers/pci/pwrseq/Kconfig
@@ -5,4 +5,12 @@ menu "PCI Power sequencing drivers"
 config PCI_PWRSEQ
 	bool
 
+config PCI_PWRSEQ_QCA6390
+	tristate "PCI Power Sequencing driver for QCA6390"
+	select PCI_PWRSEQ
+	default (ATH11K_PCI && ARCH_QCOM)
+	help
+	  Enable support for the PCI power sequencing driver for the
+	  ath11k module of the QCA6390 WLAN/BT chip.
+
 endmenu
diff --git a/drivers/pci/pwrseq/Makefile b/drivers/pci/pwrseq/Makefile
index 4052b6bb5aa5..5cf8cce01e82 100644
--- a/drivers/pci/pwrseq/Makefile
+++ b/drivers/pci/pwrseq/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_PCI_PWRSEQ)		+= pwrseq.o
+obj-$(CONFIG_PCI_PWRSEQ_QCA6390)	+= pci-pwrseq-qca6390.o
diff --git a/drivers/pci/pwrseq/pci-pwrseq-qca6390.c b/drivers/pci/pwrseq/pci-pwrseq-qca6390.c
new file mode 100644
index 000000000000..cdf3639ea29f
--- /dev/null
+++ b/drivers/pci/pwrseq/pci-pwrseq-qca6390.c
@@ -0,0 +1,267 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Linaro Ltd.
+ */
+
+#include <linux/bitmap.h>
+#include <linux/clk.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pci-pwrseq.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+struct pci_pwrseq_qca6390_vreg {
+	const char *name;
+	unsigned int load_uA;
+};
+
+struct pci_pwrseq_qca6390_pdata {
+	struct pci_pwrseq_qca6390_vreg *vregs;
+	size_t num_vregs;
+	unsigned int delay_msec;
+};
+
+struct pci_pwrseq_qca6390_ctx {
+	struct pci_pwrseq pwrseq;
+	const struct pci_pwrseq_qca6390_pdata *pdata;
+	struct regulator_bulk_data *regs;
+	struct gpio_descs *en_gpios;
+	unsigned long *en_gpios_values;
+	struct clk *clk;
+};
+
+static struct pci_pwrseq_qca6390_vreg pci_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 struct pci_pwrseq_qca6390_pdata pci_pwrseq_qca6390_of_data = {
+	.vregs = pci_pwrseq_qca6390_vregs,
+	.num_vregs = ARRAY_SIZE(pci_pwrseq_qca6390_vregs),
+	.delay_msec = 16,
+};
+
+static struct pci_pwrseq_qca6390_vreg pci_pwrseq_wcn7850_vregs[] = {
+	{
+		.name = "vdd",
+	},
+	{
+		.name = "vddio",
+	},
+	{
+		.name = "vddio12",
+	},
+	{
+		.name = "vddaon",
+	},
+	{
+		.name = "vdddig",
+	},
+	{
+		.name = "vddrfa1",
+	},
+	{
+		.name = "vddrfa2",
+	},
+};
+
+static struct pci_pwrseq_qca6390_pdata pci_pwrseq_wcn7850_of_data = {
+	.vregs = pci_pwrseq_wcn7850_vregs,
+	.num_vregs = ARRAY_SIZE(pci_pwrseq_wcn7850_vregs),
+	.delay_msec = 50,
+};
+
+static int pci_pwrseq_qca6390_power_on(struct pci_pwrseq_qca6390_ctx *ctx)
+{
+	int ret;
+
+	ret = regulator_bulk_enable(ctx->pdata->num_vregs, ctx->regs);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(ctx->clk);
+	if (ret)
+		return ret;
+
+	bitmap_fill(ctx->en_gpios_values, ctx->en_gpios->ndescs);
+
+	ret = gpiod_set_array_value_cansleep(ctx->en_gpios->ndescs,
+					     ctx->en_gpios->desc,
+					     ctx->en_gpios->info,
+					     ctx->en_gpios_values);
+	if (ret) {
+		regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
+		return ret;
+	}
+
+	if (ctx->pdata->delay_msec)
+		msleep(ctx->pdata->delay_msec);
+
+	return 0;
+}
+
+static int pci_pwrseq_qca6390_power_off(struct pci_pwrseq_qca6390_ctx *ctx)
+{
+	int ret;
+
+	bitmap_zero(ctx->en_gpios_values, ctx->en_gpios->ndescs);
+
+	ret = gpiod_set_array_value_cansleep(ctx->en_gpios->ndescs,
+					     ctx->en_gpios->desc,
+					     ctx->en_gpios->info,
+					     ctx->en_gpios_values);
+	if (ret)
+		return ret;
+
+	clk_disable_unprepare(ctx->clk);
+
+	return regulator_bulk_disable(ctx->pdata->num_vregs, ctx->regs);
+}
+
+static void devm_pci_pwrseq_qca6390_power_off(void *data)
+{
+	struct pci_pwrseq_qca6390_ctx *ctx = data;
+
+	pci_pwrseq_qca6390_power_off(ctx);
+}
+
+static int pci_pwrseq_qca6390_probe(struct platform_device *pdev)
+{
+	struct pci_pwrseq_qca6390_ctx *ctx;
+	struct device *dev = &pdev->dev;
+	int ret, i;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	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->clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(ctx->clk))
+		return dev_err_probe(dev, PTR_ERR(ctx->clk),
+				     "Failed to get clock\n");
+
+	ctx->en_gpios = devm_gpiod_get_array_optional(dev, "enable",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->en_gpios))
+		return dev_err_probe(dev, PTR_ERR(ctx->en_gpios),
+				     "Failed to get enable GPIOs\n");
+
+	ctx->en_gpios_values = devm_bitmap_zalloc(dev, ctx->en_gpios->ndescs,
+						  GFP_KERNEL);
+	if (!ctx->en_gpios_values)
+		return -ENOMEM;
+
+	ret = pci_pwrseq_qca6390_power_on(ctx);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to power on the device\n");
+
+	ret = devm_add_action_or_reset(dev, devm_pci_pwrseq_qca6390_power_off,
+				       ctx);
+	if (ret)
+		return ret;
+
+	ctx->pwrseq.dev = dev;
+
+	ret = devm_pci_pwrseq_device_enable(dev, &ctx->pwrseq);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register the pwrseq wrapper\n");
+
+	return 0;
+}
+
+static const struct of_device_id pci_pwrseq_qca6390_of_match[] = {
+	{
+		.compatible = "pci17cb,1101",
+		.data = &pci_pwrseq_qca6390_of_data,
+	},
+	{
+		.compatible = "pci17cb,1107",
+		.data = &pci_pwrseq_wcn7850_of_data,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pci_pwrseq_qca6390_of_match);
+
+static struct platform_driver pci_pwrseq_qca6390_driver = {
+	.driver = {
+		.name = "pci-pwrseq-qca6390",
+		.of_match_table = pci_pwrseq_qca6390_of_match,
+	},
+	.probe = pci_pwrseq_qca6390_probe,
+};
+module_platform_driver(pci_pwrseq_qca6390_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>");
+MODULE_DESCRIPTION("PCI Power Sequencing module for QCA6390");
+MODULE_LICENSE("GPL");
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 4/9] PCI: create platform devices for child OF nodes of the port node

[Dan Williams]

Bartosz Golaszewski wrote:
> 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>
[..]
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index d749ea8250d6..77be0630b7b3 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)
> @@ -18,11 +19,11 @@ static void pci_stop_dev(struct pci_dev *dev)
>  	pci_pme_active(dev, false);
>  
>  	if (pci_dev_is_added(dev)) {
> -
>  		device_release_driver(&dev->dev);
>  		pci_proc_detach_device(dev);
>  		pci_remove_sysfs_dev_files(dev);
>  		of_pci_remove_node(dev);
> +		of_platform_depopulate(&dev->dev);
>  
>  		pci_dev_assign_added(dev, false);

Why is pci_stop_dev() not in strict reverse order of
pci_bus_add_device()? I see that pci_dev_assign_added() was already not
in reverse "add" order before your change, but I otherwise would have
expected of_platform_depopulate() before of_pci_remove_node() (assumed
paired with of_pci_make_dev_node()).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg Kroah-Hartman]

On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
> 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.

Ick, why a platform device?  What is the parent of this device, a PCI
device?  If so, then this can't be a platform device, as that's not what
it is, it's something else so make it a device of that type,.

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

Reuse it how?

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/pci/bus.c    | 9 ++++++++-
>  drivers/pci/remove.c | 3 ++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 9c2137dae429..8ab07f711834 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);

So this is a pci bridge device, not a platform device, please don't do
this, make it a real device of a new type.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Neil Armstrong]

On 17/01/2024 17:07, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The responses to the RFC were rather positive so here's a proper series.
> 
> During last year's Linux Plumbers we had several discussions centered
> around the need to power-on PCI devices before they can be detected on
> the bus.
> 
> The consensus during the conference was that we need to introduce a
> class of "PCI slot drivers" that would handle the power-sequencing.
> 
> After some additional brain-storming with Manivannan and the realization
> that DT maintainers won't like adding any "fake" nodes not representing
> actual devices, we decided to reuse existing PCI infrastructure.
> 
> The general idea is to instantiate platform devices for child nodes of
> the PCIe port DT node. 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 this approach is not modifying the existing DT in
> any way and especially not adding any "fake" platform devices.
> 
> Changes since RFC:
> - move the pwrseq functionality out of the port driver and into PCI core
> - add support for WCN7850 to the first pwrseq driver (and update bindings)
> - describe the WLAN modules in sm8550-qrd and sm8650-qrd
> - rework Kconfig options, drop the defconfig changes from the series as
>    they are no longer needed
> - drop the dt-binding changes for PCI vendor codes
> - extend the DT bindings for ath11k_pci with strict property checking
> - various minor tweaks and fixes
> 
> Bartosz Golaszewski (7):
>    arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of 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/pwrseq: add pwrseq core code
>    dt-bindings: wireless: ath11k: describe QCA6390
>    dt-bindings: wireless: ath11k: describe WCN7850
>    PCI/pwrseq: add a pwrseq driver for QCA6390
> 
> Neil Armstrong (2):
>    arm64: dts: qcom: sm8550-qrd: add Wifi nodes
>    arm64: dts: qcom: sm8650-qrd: add Wifi nodes
> 
>   .../net/wireless/qcom,ath11k-pci.yaml         |  89 ++++++
>   arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      |  29 ++
>   arch/arm64/boot/dts/qcom/sm8250.dtsi          |  10 +
>   arch/arm64/boot/dts/qcom/sm8550-qrd.dts       |  37 +++
>   arch/arm64/boot/dts/qcom/sm8550.dtsi          |  10 +
>   arch/arm64/boot/dts/qcom/sm8650-qrd.dts       |  29 ++
>   arch/arm64/boot/dts/qcom/sm8650.dtsi          |  10 +
>   drivers/pci/Kconfig                           |   1 +
>   drivers/pci/Makefile                          |   1 +
>   drivers/pci/bus.c                             |   9 +-
>   drivers/pci/probe.c                           |   2 +
>   drivers/pci/pwrseq/Kconfig                    |  16 ++
>   drivers/pci/pwrseq/Makefile                   |   4 +
>   drivers/pci/pwrseq/pci-pwrseq-qca6390.c       | 267 ++++++++++++++++++
>   drivers/pci/pwrseq/pwrseq.c                   |  82 ++++++
>   drivers/pci/remove.c                          |   3 +-
>   include/linux/pci-pwrseq.h                    |  24 ++
>   17 files changed, 621 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/pci/pwrseq/Kconfig
>   create mode 100644 drivers/pci/pwrseq/Makefile
>   create mode 100644 drivers/pci/pwrseq/pci-pwrseq-qca6390.c
>   create mode 100644 drivers/pci/pwrseq/pwrseq.c
>   create mode 100644 include/linux/pci-pwrseq.h
> 

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD

Thanks,
Neil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/9] dt-bindings: wireless: ath11k: describe WCN7850

[Kalle Valo]

Bartosz Golaszewski <brgl@bgdev.pl> writes:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Describe the ath11k variant present on the WCN7850 module.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

ath12k supports WCN7850 (a Wi-Fi 7 chipset), not ath11k.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Dmitry Baryshkov]

On Wed, 17 Jan 2024 at 18:08, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> The responses to the RFC were rather positive so here's a proper series.
>
> During last year's Linux Plumbers we had several discussions centered
> around the need to power-on PCI devices before they can be detected on
> the bus.
>
> The consensus during the conference was that we need to introduce a
> class of "PCI slot drivers" that would handle the power-sequencing.
>
> After some additional brain-storming with Manivannan and the realization
> that DT maintainers won't like adding any "fake" nodes not representing
> actual devices, we decided to reuse existing PCI infrastructure.
>
> The general idea is to instantiate platform devices for child nodes of
> the PCIe port DT node. 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 this approach is not modifying the existing DT in
> any way and especially not adding any "fake" platform devices.

I'd still like to see how this can be extended to handle BT power up,
having a single entity driving both of the BT and WiFI.

The device tree changes behave in exactly the opposite way: they
define regulators for the WiFi device, while the WiFi is not being
powered by these regulators. Both WiFi and BT are powered by the PMU,
which in turn consumes all specified regulators.

>
> Changes since RFC:
> - move the pwrseq functionality out of the port driver and into PCI core
> - add support for WCN7850 to the first pwrseq driver (and update bindings)
> - describe the WLAN modules in sm8550-qrd and sm8650-qrd
> - rework Kconfig options, drop the defconfig changes from the series as
>   they are no longer needed
> - drop the dt-binding changes for PCI vendor codes
> - extend the DT bindings for ath11k_pci with strict property checking
> - various minor tweaks and fixes
>
> Bartosz Golaszewski (7):
>   arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of 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/pwrseq: add pwrseq core code
>   dt-bindings: wireless: ath11k: describe QCA6390
>   dt-bindings: wireless: ath11k: describe WCN7850
>   PCI/pwrseq: add a pwrseq driver for QCA6390
>
> Neil Armstrong (2):
>   arm64: dts: qcom: sm8550-qrd: add Wifi nodes
>   arm64: dts: qcom: sm8650-qrd: add Wifi nodes
>
>  .../net/wireless/qcom,ath11k-pci.yaml         |  89 ++++++
>  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      |  29 ++
>  arch/arm64/boot/dts/qcom/sm8250.dtsi          |  10 +
>  arch/arm64/boot/dts/qcom/sm8550-qrd.dts       |  37 +++
>  arch/arm64/boot/dts/qcom/sm8550.dtsi          |  10 +
>  arch/arm64/boot/dts/qcom/sm8650-qrd.dts       |  29 ++
>  arch/arm64/boot/dts/qcom/sm8650.dtsi          |  10 +
>  drivers/pci/Kconfig                           |   1 +
>  drivers/pci/Makefile                          |   1 +
>  drivers/pci/bus.c                             |   9 +-
>  drivers/pci/probe.c                           |   2 +
>  drivers/pci/pwrseq/Kconfig                    |  16 ++
>  drivers/pci/pwrseq/Makefile                   |   4 +
>  drivers/pci/pwrseq/pci-pwrseq-qca6390.c       | 267 ++++++++++++++++++
>  drivers/pci/pwrseq/pwrseq.c                   |  82 ++++++
>  drivers/pci/remove.c                          |   3 +-
>  include/linux/pci-pwrseq.h                    |  24 ++
>  17 files changed, 621 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/pci/pwrseq/Kconfig
>  create mode 100644 drivers/pci/pwrseq/Makefile
>  create mode 100644 drivers/pci/pwrseq/pci-pwrseq-qca6390.c
>  create mode 100644 drivers/pci/pwrseq/pwrseq.c
>  create mode 100644 include/linux/pci-pwrseq.h
>
> --
> 2.40.1
>
>


-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/9] dt-bindings: wireless: ath11k: describe WCN7850

[Bartosz Golaszewski]

On Wed, Jan 17, 2024 at 7:07 PM Kalle Valo <kvalo@kernel.org> wrote:
>
> Bartosz Golaszewski <brgl@bgdev.pl> writes:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Describe the ath11k variant present on the WCN7850 module.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> ath12k supports WCN7850 (a Wi-Fi 7 chipset), not ath11k.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Eek! Indeed. So most of the ifs in the bindings are not really needed
after all... which is good actually.

Bart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 9/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Kalle Valo]

Bartosz Golaszewski <brgl@bgdev.pl> writes:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Add a PCI power sequencing driver that's capable of correctly powering
> up the ath11k module on QCA6390 and WCN7850 using the PCI pwrseq
> functionality.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> [Neil: add support for WCN7850]
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

Here also: ath12k supports WCN7850, not ath11k.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bartosz Golaszewski]

On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
> > 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.
>
> Ick, why a platform device?  What is the parent of this device, a PCI
> device?  If so, then this can't be a platform device, as that's not what
> it is, it's something else so make it a device of that type,.
>

Greg,

This is literally what we agreed on at LPC. In fact: during one of the
hall track discussions I said that you typically NAK any attempts at
using the platform bus for "fake" devices but you responded that this
is what the USB on-board HUB does and while it's not pretty, this is
what we need to do.

Now as for the implementation, the way I see it we have two solutions:
either we introduce a fake, top-level PCI slot platform device device
that will reference the PCI host controller by phandle or we will live
with a secondary, "virtual" platform device for power sequencing that
is tied to the actual PCI device. The former requires us to add DT
bindings, add a totally fake DT node representing the "slot" which
doesn't really exist (and Krzysztof already expressed his negative
opinion of that) and then have code that will be more complex than it
needs to be. The latter allows us to not change DT at all (other than
adding regulators, clocks and GPIOs to already existing WLAN nodes),
reuse the existing parent-child relationship between the port node and
the instantiated platform device as well as result in simpler code.

Given that DT needs to be stable while the underlying C code can
freely change if we find a better solution, I think that the second
option is a no-brainer here.

> > 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.
>
> Reuse it how?
>

By consuming the same DT node using device_set_of_node_from_dev() when
the PCI device is registered. This ensures we don't try to bind
pinctrl twice etc.

> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/pci/bus.c    | 9 ++++++++-
> >  drivers/pci/remove.c | 3 ++-
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 9c2137dae429..8ab07f711834 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);
>
> So this is a pci bridge device, not a platform device, please don't do
> this, make it a real device of a new type.
>

Not sure what you mean. Are you suggesting adding a new bus? Or do we
already have a concept of PCI bridge devices in the kernel?

Bartosz

> thanks,
>
> greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg Kroah-Hartman]

On Thu, Jan 18, 2024 at 11:58:50AM +0100, Bartosz Golaszewski wrote:
> On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
> > > 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.
> >
> > Ick, why a platform device?  What is the parent of this device, a PCI
> > device?  If so, then this can't be a platform device, as that's not what
> > it is, it's something else so make it a device of that type,.
> >
> 
> Greg,
> 
> This is literally what we agreed on at LPC. In fact: during one of the
> hall track discussions I said that you typically NAK any attempts at
> using the platform bus for "fake" devices but you responded that this
> is what the USB on-board HUB does and while it's not pretty, this is
> what we need to do.

Ah, you need to remind me of these things, this changelog was pretty
sparse :)

> Now as for the implementation, the way I see it we have two solutions:
> either we introduce a fake, top-level PCI slot platform device device
> that will reference the PCI host controller by phandle or we will live
> with a secondary, "virtual" platform device for power sequencing that
> is tied to the actual PCI device. The former requires us to add DT
> bindings, add a totally fake DT node representing the "slot" which
> doesn't really exist (and Krzysztof already expressed his negative
> opinion of that) and then have code that will be more complex than it
> needs to be. The latter allows us to not change DT at all (other than
> adding regulators, clocks and GPIOs to already existing WLAN nodes),
> reuse the existing parent-child relationship between the port node and
> the instantiated platform device as well as result in simpler code.
> 
> Given that DT needs to be stable while the underlying C code can
> freely change if we find a better solution, I think that the second
> option is a no-brainer here.

Ok, I remove my objections, sorry about that, my confusion.

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bartosz Golaszewski]

On Wed, Jan 17, 2024 at 5:22 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Bartosz Golaszewski wrote:
> > 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>
> [..]
> > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> > index d749ea8250d6..77be0630b7b3 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)
> > @@ -18,11 +19,11 @@ static void pci_stop_dev(struct pci_dev *dev)
> >       pci_pme_active(dev, false);
> >
> >       if (pci_dev_is_added(dev)) {
> > -
> >               device_release_driver(&dev->dev);
> >               pci_proc_detach_device(dev);
> >               pci_remove_sysfs_dev_files(dev);
> >               of_pci_remove_node(dev);
> > +             of_platform_depopulate(&dev->dev);
> >
> >               pci_dev_assign_added(dev, false);
>
> Why is pci_stop_dev() not in strict reverse order of
> pci_bus_add_device()? I see that pci_dev_assign_added() was already not
> in reverse "add" order before your change, but I otherwise would have
> expected of_platform_depopulate() before of_pci_remove_node() (assumed
> paired with of_pci_make_dev_node()).

The naming here is confusing but the two have nothing in common. One
is used by CONFIG_PCI_DYNAMIC_OF_NODES to *create* new DT nodes for
detected PCI devices. The one I'm adding, creates power sequencing
*devices* (no nodes) for *existing* DT nodes.

So the order is not really relevant here but I can change in v2.

Bartosz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Rob Herring]

On Wed, Jan 17, 2024 at 10:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> The responses to the RFC were rather positive so here's a proper series.

Thanks for tackling this.

> During last year's Linux Plumbers we had several discussions centered
> around the need to power-on PCI devices before they can be detected on
> the bus.
>
> The consensus during the conference was that we need to introduce a
> class of "PCI slot drivers" that would handle the power-sequencing.
>
> After some additional brain-storming with Manivannan and the realization
> that DT maintainers won't like adding any "fake" nodes not representing
> actual devices, we decided to reuse existing PCI infrastructure.

Thank you. :)

> The general idea is to instantiate platform devices for child nodes of
> the PCIe port DT node. 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 this approach is not modifying the existing DT in
> any way and especially not adding any "fake" platform devices.

Suspend/resume has been brought up already, but I disagree we can
worry about that later unless there is and always will be no power
sequencing during suspend/resume for all devices ever. Given the
supplies aren't standard, it wouldn't surprise me if standard PCI
power management isn't either. The primary issue I see with this
design is we will end up with 2 drivers doing the same power
sequencing: the platform driver for initial power on and the device's
PCI driver for suspend/resume.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Bartosz Golaszewski]

On Thu, 18 Jan 2024 15:29:01 +0100, Rob Herring <robh+dt@kernel.org> said:
> On Wed, Jan 17, 2024 at 10:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>

[snip]

>
>> The general idea is to instantiate platform devices for child nodes of
>> the PCIe port DT node. 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 this approach is not modifying the existing DT in
>> any way and especially not adding any "fake" platform devices.
>
> Suspend/resume has been brought up already, but I disagree we can
> worry about that later unless there is and always will be no power
> sequencing during suspend/resume for all devices ever. Given the
> supplies aren't standard, it wouldn't surprise me if standard PCI
> power management isn't either. The primary issue I see with this
> design is we will end up with 2 drivers doing the same power
> sequencing: the platform driver for initial power on and the device's
> PCI driver for suspend/resume.
>
> Rob
>

I admit that I don't have any HW where I could test it but I my thinking was
that with the following relationships between the devices:

                  ┌─────────────────────┐
                  │                     │
                  │   PCI Port device   │
                  │                     │
                  └───┬───────────┬─────┘
                      │           │
                      │           │
                      │           │
┌─────────────────────▼─────┐     │
│                           │     │
│   QCA6390 pwrseq device   │     │
│                           │     │
└─────────────────────┬─────┘     │
                      │           │
                      │           │
                      │           │
                ┌─────▼───────────▼───┐
                │                     │
                │  ath11k_pci device  │
                │                     │
                └─────────────────────┘

the PM subsystem would handle the dependencies automatically and correctly
setup the sequence for suspend and resume. Also: the PCI ath11k driver does
not deal with the kind of resources that the power sequencing platform driver
handles: regulators, GPIOs and clocks.

I agree, it would be useful to have a working case of handling suspend/resume
with this code though.

Bartosz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Manivannan Sadhasivam]

On Thu, Jan 18, 2024 at 08:29:01AM -0600, Rob Herring wrote:
> On Wed, Jan 17, 2024 at 10:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > The responses to the RFC were rather positive so here's a proper series.
> 
> Thanks for tackling this.
> 
> > During last year's Linux Plumbers we had several discussions centered
> > around the need to power-on PCI devices before they can be detected on
> > the bus.
> >
> > The consensus during the conference was that we need to introduce a
> > class of "PCI slot drivers" that would handle the power-sequencing.
> >
> > After some additional brain-storming with Manivannan and the realization
> > that DT maintainers won't like adding any "fake" nodes not representing
> > actual devices, we decided to reuse existing PCI infrastructure.
> 
> Thank you. :)
> 
> > The general idea is to instantiate platform devices for child nodes of
> > the PCIe port DT node. 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 this approach is not modifying the existing DT in
> > any way and especially not adding any "fake" platform devices.
> 
> Suspend/resume has been brought up already, but I disagree we can
> worry about that later unless there is and always will be no power
> sequencing during suspend/resume for all devices ever. Given the
> supplies aren't standard, it wouldn't surprise me if standard PCI
> power management isn't either. The primary issue I see with this
> design is we will end up with 2 drivers doing the same power
> sequencing: the platform driver for initial power on and the device's
> PCI driver for suspend/resume.
> 

There are actually 3 drivers need to do their own power management operations:

1. PCIe device driver - Handle the PM of the device itself (shutdown, low power)
2. PCIe pwrseq driver (this one) - Control resources of the PCIe devices
3. PCIe controller driver - Control resources of PCIe controller and Link

And all of them has different responsibilities, so I do not see an issue with
that. But what is really important is that all 3 has to work in sync and that's
quite involved. That's why I thought of dealing with that later.

Moreover, even if driver (2) doesn't do any PM operations now, it won't break
anything on the currently supported platforms (Qcom). It will be a problem once
people start adding pwrseq drivers for platforms whose controller drivers are
already handling the job which is now offloaded by this driver.

- Mani

> Rob
> 

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 9/9] PCI/pwrseq: add a pwrseq driver for QCA6390

[Konrad Dybcio]

On 1/17/24 17:07, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add a PCI power sequencing driver that's capable of correctly powering
> up the ath11k module on QCA6390 and WCN7850 using the PCI pwrseq
> functionality.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> [Neil: add support for WCN7850]
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---

[...]

> +static struct pci_pwrseq_qca6390_vreg pci_pwrseq_wcn7850_vregs[] = {
> +	{
> +		.name = "vdd",
> +	},

Weird there's no .load here.. On Qualcomm they're used for asking
the regluators to enter the high power mode, so it'd be useful.

Konrad

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Dmitry Baryshkov]

On Wed, 17 Jan 2024 at 20:16, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, 17 Jan 2024 at 18:08, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > The responses to the RFC were rather positive so here's a proper series.
> >
> > During last year's Linux Plumbers we had several discussions centered
> > around the need to power-on PCI devices before they can be detected on
> > the bus.
> >
> > The consensus during the conference was that we need to introduce a
> > class of "PCI slot drivers" that would handle the power-sequencing.
> >
> > After some additional brain-storming with Manivannan and the realization
> > that DT maintainers won't like adding any "fake" nodes not representing
> > actual devices, we decided to reuse existing PCI infrastructure.
> >
> > The general idea is to instantiate platform devices for child nodes of
> > the PCIe port DT node. 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 this approach is not modifying the existing DT in
> > any way and especially not adding any "fake" platform devices.
>
> I'd still like to see how this can be extended to handle BT power up,
> having a single entity driving both of the BT and WiFI.
>
> The device tree changes behave in exactly the opposite way: they
> define regulators for the WiFi device, while the WiFi is not being
> powered by these regulators. Both WiFi and BT are powered by the PMU,
> which in turn consumes all specified regulators.

Some additional justification, why I think that this should be
modelled as a single instance instead of two different items.

This is from msm-5.10 kernel:


===== CUT HERE =====
/**
 * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status
 * @plat_priv: Platform private data structure pointer
 *
 * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and
 * WLAN_EN_GPIO on. This is done to avoid power up issues.
 *
 * Return: Status of pinctrl select operation. 0 - Success.
 */
static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv)
===== CUT HERE =====


Also see the bt_configure_gpios() function in the same kernel.


>
> >
> > Changes since RFC:
> > - move the pwrseq functionality out of the port driver and into PCI core
> > - add support for WCN7850 to the first pwrseq driver (and update bindings)
> > - describe the WLAN modules in sm8550-qrd and sm8650-qrd
> > - rework Kconfig options, drop the defconfig changes from the series as
> >   they are no longer needed
> > - drop the dt-binding changes for PCI vendor codes
> > - extend the DT bindings for ath11k_pci with strict property checking
> > - various minor tweaks and fixes
> >
> > Bartosz Golaszewski (7):
> >   arm64: dts: qcom: qrb5165-rb5: describe the WLAN module of 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/pwrseq: add pwrseq core code
> >   dt-bindings: wireless: ath11k: describe QCA6390
> >   dt-bindings: wireless: ath11k: describe WCN7850
> >   PCI/pwrseq: add a pwrseq driver for QCA6390
> >
> > Neil Armstrong (2):
> >   arm64: dts: qcom: sm8550-qrd: add Wifi nodes
> >   arm64: dts: qcom: sm8650-qrd: add Wifi nodes
> >
> >  .../net/wireless/qcom,ath11k-pci.yaml         |  89 ++++++
> >  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      |  29 ++
> >  arch/arm64/boot/dts/qcom/sm8250.dtsi          |  10 +
> >  arch/arm64/boot/dts/qcom/sm8550-qrd.dts       |  37 +++
> >  arch/arm64/boot/dts/qcom/sm8550.dtsi          |  10 +
> >  arch/arm64/boot/dts/qcom/sm8650-qrd.dts       |  29 ++
> >  arch/arm64/boot/dts/qcom/sm8650.dtsi          |  10 +
> >  drivers/pci/Kconfig                           |   1 +
> >  drivers/pci/Makefile                          |   1 +
> >  drivers/pci/bus.c                             |   9 +-
> >  drivers/pci/probe.c                           |   2 +
> >  drivers/pci/pwrseq/Kconfig                    |  16 ++
> >  drivers/pci/pwrseq/Makefile                   |   4 +
> >  drivers/pci/pwrseq/pci-pwrseq-qca6390.c       | 267 ++++++++++++++++++
> >  drivers/pci/pwrseq/pwrseq.c                   |  82 ++++++
> >  drivers/pci/remove.c                          |   3 +-
> >  include/linux/pci-pwrseq.h                    |  24 ++
> >  17 files changed, 621 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/pci/pwrseq/Kconfig
> >  create mode 100644 drivers/pci/pwrseq/Makefile
> >  create mode 100644 drivers/pci/pwrseq/pci-pwrseq-qca6390.c
> >  create mode 100644 drivers/pci/pwrseq/pwrseq.c
> >  create mode 100644 include/linux/pci-pwrseq.h
> >
> > --
> > 2.40.1
> >
> >
>
>
> --
> With best wishes
> Dmitry



-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Bartosz Golaszewski]

On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>

[snip]

> >
> > I'd still like to see how this can be extended to handle BT power up,
> > having a single entity driving both of the BT and WiFI.
> >
> > The device tree changes behave in exactly the opposite way: they
> > define regulators for the WiFi device, while the WiFi is not being
> > powered by these regulators. Both WiFi and BT are powered by the PMU,
> > which in turn consumes all specified regulators.
>
> Some additional justification, why I think that this should be
> modelled as a single instance instead of two different items.
>
> This is from msm-5.10 kernel:
>
>
> ===== CUT HERE =====
> /**
>  * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status
>  * @plat_priv: Platform private data structure pointer
>  *
>  * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and
>  * WLAN_EN_GPIO on. This is done to avoid power up issues.
>  *
>  * Return: Status of pinctrl select operation. 0 - Success.
>  */
> static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv)
> ===== CUT HERE =====
>
>
> Also see the bt_configure_gpios() function in the same kernel.
>

You are talking about a different problem. Unfortunately we're using
similar naming here but I don't have a better alternative in mind.

We have two separate issues: one is powering-up a PCI device so that
it can be detected and the second is dealing with a device that has
multiple modules in it which share a power sequence. The two are
independent and this series isn't trying to solve the latter.

But I am aware of this and so I actually have an idea for a
generalized power sequencing framework. Let's call it pwrseq as
opposed to pci_pwrseq.

Krzysztof is telling me that there cannot be any power sequencing
information contained in DT. Also: modelling the PMU in DT would just
over complicate stuff for now reason. We'd end up having the PMU node
consuming the regulators but it too would need to expose regulators
for WLAN and BT or be otherwise referenced by their nodes.

So I'm thinking that the DT representation should remain as it is:
with separate WLAN and BT nodes consuming resources relevant to their
functionality (BT does not need to enable PCIe regulators). Now how to
handle the QCA6490 model you brought up? How about pwrseq drivers that
would handle the sequence based on compatibles?

We'd add a new subsystem at drivers/pwrseq/. Inside there would be:
drivers/pwrseq/pwrseq-qca6490.c. The pwrseq framework would expose an
API to "sub-drivers" (in this case: BT serdev driver and the qca6490
power sequencing driver). Now the latter goes:

struct pwrseq_desc *pwrseq = pwrseq_get(dev);

And the pwrseq subsystem matches the device's compatible against the
correct, *shared* sequence. The BT driver can do the same at any time.
The pwrseq driver then gets regulators, GPIOs, clocks etc. and will be
responsible for dealing with them.

In sub-drivers we now do:

ret = pwrseq_power_on(pwrseq);

or

ret = pwrseq_power_off(pwrseq);

in the sub-device drivers and no longer interact with each regulator
on our own. The pwrseq subsystem is now in charge of adding delays
etc.

That's only an idea and I haven't done any real work yet but I'm
throwing it out there for discussion.

Bartosz

[snip]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Dmitry Baryshkov]

On Fri, 19 Jan 2024 at 13:52, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
>
> [snip]
>
> > >
> > > I'd still like to see how this can be extended to handle BT power up,
> > > having a single entity driving both of the BT and WiFI.
> > >
> > > The device tree changes behave in exactly the opposite way: they
> > > define regulators for the WiFi device, while the WiFi is not being
> > > powered by these regulators. Both WiFi and BT are powered by the PMU,
> > > which in turn consumes all specified regulators.
> >
> > Some additional justification, why I think that this should be
> > modelled as a single instance instead of two different items.
> >
> > This is from msm-5.10 kernel:
> >
> >
> > ===== CUT HERE =====
> > /**
> >  * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status
> >  * @plat_priv: Platform private data structure pointer
> >  *
> >  * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and
> >  * WLAN_EN_GPIO on. This is done to avoid power up issues.
> >  *
> >  * Return: Status of pinctrl select operation. 0 - Success.
> >  */
> > static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv)
> > ===== CUT HERE =====
> >
> >
> > Also see the bt_configure_gpios() function in the same kernel.
> >
>
> You are talking about a different problem. Unfortunately we're using
> similar naming here but I don't have a better alternative in mind.
>
> We have two separate issues: one is powering-up a PCI device so that
> it can be detected and the second is dealing with a device that has
> multiple modules in it which share a power sequence. The two are
> independent and this series isn't trying to solve the latter.

I see it from a different angle: a power up of the WiFi+BT chips. This
includes devices like wcn3990 (which have platform + serial parts) and
qca6390 / qca6490 / wcn6750 / etc. (which have PCI and serial parts).

From my point of view, the PCIe-only part was nice for an RFC, but for
v1 I have expected to see a final solution that we can reuse for
wcn3990.

>
> But I am aware of this and so I actually have an idea for a
> generalized power sequencing framework. Let's call it pwrseq as
> opposed to pci_pwrseq.
>
> Krzysztof is telling me that there cannot be any power sequencing
> information contained in DT. Also: modelling the PMU in DT would just
> over complicate stuff for now reason. We'd end up having the PMU node
> consuming the regulators but it too would need to expose regulators
> for WLAN and BT or be otherwise referenced by their nodes.

Yes. And it is a correct representation of the device. The WiFi and BT
parts are powered up by the outputs from PMU. We happen to have three
different pieces (WiFi, BT and PMU) squashed on a single physical
device.

>
> So I'm thinking that the DT representation should remain as it is:
> with separate WLAN and BT nodes consuming resources relevant to their
> functionality (BT does not need to enable PCIe regulators).

Is it so? The QCA6390 docs clearly say that all regulators should be
enabled before asserting BT_EN / WLAN_EN. See the powerup timing
diagram and the t2 note to that diagram.

> Now how to
> handle the QCA6490 model you brought up? How about pwrseq drivers that
> would handle the sequence based on compatibles?

The QCA6490 is also known as WCN6855. So this problem applies to
Qualcomm sm8350 / sm8450 platforms.

And strictly speaking I don't see any significant difference between
QCA6390 and WCN6855. The regulators might be different, but the
implementation should be the same.

>
> We'd add a new subsystem at drivers/pwrseq/. Inside there would be:
> drivers/pwrseq/pwrseq-qca6490.c. The pwrseq framework would expose an
> API to "sub-drivers" (in this case: BT serdev driver and the qca6490
> power sequencing driver). Now the latter goes:
>
> struct pwrseq_desc *pwrseq = pwrseq_get(dev);
>
> And the pwrseq subsystem matches the device's compatible against the
> correct, *shared* sequence. The BT driver can do the same at any time.
> The pwrseq driver then gets regulators, GPIOs, clocks etc. and will be
> responsible for dealing with them.
>
> In sub-drivers we now do:
>
> ret = pwrseq_power_on(pwrseq);
>
> or
>
> ret = pwrseq_power_off(pwrseq);
>
> in the sub-device drivers and no longer interact with each regulator
> on our own. The pwrseq subsystem is now in charge of adding delays
> etc.
>
> That's only an idea and I haven't done any real work yet but I'm
> throwing it out there for discussion.

I've been there and I had implemented it in the same way, but rather
having the pwrseq as a primary device in DT and parsing end-devices
only as a fallback / compatibility case.



-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[brgl]

On Fri, 19 Jan 2024 13:31:53 +0100, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> said:
> On Fri, 19 Jan 2024 at 13:52, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>
>> On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov
>> <dmitry.baryshkov@linaro.org> wrote:
>> >
>>
>> [snip]
>>
>> > >
>> > > I'd still like to see how this can be extended to handle BT power up,
>> > > having a single entity driving both of the BT and WiFI.
>> > >
>> > > The device tree changes behave in exactly the opposite way: they
>> > > define regulators for the WiFi device, while the WiFi is not being
>> > > powered by these regulators. Both WiFi and BT are powered by the PMU,
>> > > which in turn consumes all specified regulators.
>> >
>> > Some additional justification, why I think that this should be
>> > modelled as a single instance instead of two different items.
>> >
>> > This is from msm-5.10 kernel:
>> >
>> >
>> > ===== CUT HERE =====
>> > /**
>> >  * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status
>> >  * @plat_priv: Platform private data structure pointer
>> >  *
>> >  * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and
>> >  * WLAN_EN_GPIO on. This is done to avoid power up issues.
>> >  *
>> >  * Return: Status of pinctrl select operation. 0 - Success.
>> >  */
>> > static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv)
>> > ===== CUT HERE =====
>> >
>> >
>> > Also see the bt_configure_gpios() function in the same kernel.
>> >
>>
>> You are talking about a different problem. Unfortunately we're using
>> similar naming here but I don't have a better alternative in mind.
>>
>> We have two separate issues: one is powering-up a PCI device so that
>> it can be detected and the second is dealing with a device that has
>> multiple modules in it which share a power sequence. The two are
>> independent and this series isn't trying to solve the latter.
>
> I see it from a different angle: a power up of the WiFi+BT chips. This
> includes devices like wcn3990 (which have platform + serial parts) and
> qca6390 / qca6490 / wcn6750 / etc. (which have PCI and serial parts).
>
> From my point of view, the PCIe-only part was nice for an RFC, but for
> v1 I have expected to see a final solution that we can reuse for
> wcn3990.
>

The submodules are represented as independent devices on the DT and I don't
think this will change. It's not even possible as they operate on different
buses so it's not like we can MFD it with a top-level platform device and two
sub-nodes of which one is PCI and another serdev. With that in mind, I'm
insisting that there are two separate issues and a generic power sequencing
can be built on top of the PCI-specific pwrseq added here.

>>
>> But I am aware of this and so I actually have an idea for a
>> generalized power sequencing framework. Let's call it pwrseq as
>> opposed to pci_pwrseq.
>>
>> Krzysztof is telling me that there cannot be any power sequencing
>> information contained in DT. Also: modelling the PMU in DT would just
>> over complicate stuff for now reason. We'd end up having the PMU node
>> consuming the regulators but it too would need to expose regulators
>> for WLAN and BT or be otherwise referenced by their nodes.
>
> Yes. And it is a correct representation of the device. The WiFi and BT
> parts are powered up by the outputs from PMU. We happen to have three
> different pieces (WiFi, BT and PMU) squashed on a single physical
> device.
>

Alright, so let's imagine we do model the PMU on the device tree. It would
look something like this:

qca6390_pmu: pmic@0 {
	compatible = "qcom,qca6390-pmu";

	bt-gpios = <...>;
	wlan-gpios = <...>;

	vdd-supply = <&vreg...>;
	...

	regulators-0 {
		vreg_x: foo {
			...
		};

		...
	};
};

Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously we
cannot go:

wlan {
	pwrseq = &qca6390_pmu;
};

But it's enough to:

wlan {
	vdd-supply = <&vreg_x>;
};

But the pwrseq driver for "qcom,qca6390-pmu" could map BT and WLAN compatibles
to the correct power sequence and then the relevant drivers could enable it
using pwrseq_power_on().

But that comes back to what I'm doing here: the PCI part for ath11k still
needs the platform driver that will trigger the power sequence and that could
be the PCI pwrseq driver for which the framework is introduced in this series.

As I said: the two are largely orthogonal.

>>
>> So I'm thinking that the DT representation should remain as it is:
>> with separate WLAN and BT nodes consuming resources relevant to their
>> functionality (BT does not need to enable PCIe regulators).
>
> Is it so? The QCA6390 docs clearly say that all regulators should be
> enabled before asserting BT_EN / WLAN_EN. See the powerup timing
> diagram and the t2 note to that diagram.
>

Fair enough.

>> Now how to
>> handle the QCA6490 model you brought up? How about pwrseq drivers that
>> would handle the sequence based on compatibles?
>
> The QCA6490 is also known as WCN6855. So this problem applies to
> Qualcomm sm8350 / sm8450 platforms.
>
> And strictly speaking I don't see any significant difference between
> QCA6390 and WCN6855. The regulators might be different, but the
> implementation should be the same.
>
>>
>> We'd add a new subsystem at drivers/pwrseq/. Inside there would be:
>> drivers/pwrseq/pwrseq-qca6490.c. The pwrseq framework would expose an
>> API to "sub-drivers" (in this case: BT serdev driver and the qca6490
>> power sequencing driver). Now the latter goes:
>>
>> struct pwrseq_desc *pwrseq = pwrseq_get(dev);
>>
>> And the pwrseq subsystem matches the device's compatible against the
>> correct, *shared* sequence. The BT driver can do the same at any time.
>> The pwrseq driver then gets regulators, GPIOs, clocks etc. and will be
>> responsible for dealing with them.
>>
>> In sub-drivers we now do:
>>
>> ret = pwrseq_power_on(pwrseq);
>>
>> or
>>
>> ret = pwrseq_power_off(pwrseq);
>>
>> in the sub-device drivers and no longer interact with each regulator
>> on our own. The pwrseq subsystem is now in charge of adding delays
>> etc.
>>
>> That's only an idea and I haven't done any real work yet but I'm
>> throwing it out there for discussion.
>
> I've been there and I had implemented it in the same way, but rather
> having the pwrseq as a primary device in DT and parsing end-devices
> only as a fallback / compatibility case.
>

Would you mind posting an example DT code here? I'm not sure if I understand
what "primary device" means in this context.

Bartosz

>
>
> --
> With best wishes
> Dmitry
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Dmitry Baryshkov]

On Fri, 19 Jan 2024 at 15:35, <brgl@bgdev.pl> wrote:
>
> On Fri, 19 Jan 2024 13:31:53 +0100, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> said:
> > On Fri, 19 Jan 2024 at 13:52, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>
> >> On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov
> >> <dmitry.baryshkov@linaro.org> wrote:
> >> >
> >>
> >> [snip]
> >>
> >> > >
> >> > > I'd still like to see how this can be extended to handle BT power up,
> >> > > having a single entity driving both of the BT and WiFI.
> >> > >
> >> > > The device tree changes behave in exactly the opposite way: they
> >> > > define regulators for the WiFi device, while the WiFi is not being
> >> > > powered by these regulators. Both WiFi and BT are powered by the PMU,
> >> > > which in turn consumes all specified regulators.
> >> >
> >> > Some additional justification, why I think that this should be
> >> > modelled as a single instance instead of two different items.
> >> >
> >> > This is from msm-5.10 kernel:
> >> >
> >> >
> >> > ===== CUT HERE =====
> >> > /**
> >> >  * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl status
> >> >  * @plat_priv: Platform private data structure pointer
> >> >  *
> >> >  * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off and
> >> >  * WLAN_EN_GPIO on. This is done to avoid power up issues.
> >> >  *
> >> >  * Return: Status of pinctrl select operation. 0 - Success.
> >> >  */
> >> > static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_priv)
> >> > ===== CUT HERE =====
> >> >
> >> >
> >> > Also see the bt_configure_gpios() function in the same kernel.
> >> >
> >>
> >> You are talking about a different problem. Unfortunately we're using
> >> similar naming here but I don't have a better alternative in mind.
> >>
> >> We have two separate issues: one is powering-up a PCI device so that
> >> it can be detected and the second is dealing with a device that has
> >> multiple modules in it which share a power sequence. The two are
> >> independent and this series isn't trying to solve the latter.
> >
> > I see it from a different angle: a power up of the WiFi+BT chips. This
> > includes devices like wcn3990 (which have platform + serial parts) and
> > qca6390 / qca6490 / wcn6750 / etc. (which have PCI and serial parts).
> >
> > From my point of view, the PCIe-only part was nice for an RFC, but for
> > v1 I have expected to see a final solution that we can reuse for
> > wcn3990.
> >
>
> The submodules are represented as independent devices on the DT and I don't
> think this will change. It's not even possible as they operate on different
> buses so it's not like we can MFD it with a top-level platform device and two
> sub-nodes of which one is PCI and another serdev. With that in mind, I'm
> insisting that there are two separate issues and a generic power sequencing
> can be built on top of the PCI-specific pwrseq added here.
>
> >>
> >> But I am aware of this and so I actually have an idea for a
> >> generalized power sequencing framework. Let's call it pwrseq as
> >> opposed to pci_pwrseq.
> >>
> >> Krzysztof is telling me that there cannot be any power sequencing
> >> information contained in DT. Also: modelling the PMU in DT would just
> >> over complicate stuff for now reason. We'd end up having the PMU node
> >> consuming the regulators but it too would need to expose regulators
> >> for WLAN and BT or be otherwise referenced by their nodes.
> >
> > Yes. And it is a correct representation of the device. The WiFi and BT
> > parts are powered up by the outputs from PMU. We happen to have three
> > different pieces (WiFi, BT and PMU) squashed on a single physical
> > device.
> >
>
> Alright, so let's imagine we do model the PMU on the device tree. It would
> look something like this:
>
> qca6390_pmu: pmic@0 {
>         compatible = "qcom,qca6390-pmu";
>
>         bt-gpios = <...>;
>         wlan-gpios = <...>;
>
>         vdd-supply = <&vreg...>;
>         ...
>
>         regulators-0 {
>                 vreg_x: foo {
>                         ...
>                 };
>
>                 ...
>         };
> };
>
> Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously we
> cannot go:
>
> wlan {
>         pwrseq = &qca6390_pmu;
> };
>
> But it's enough to:
>
> wlan {
>         vdd-supply = <&vreg_x>;
> };

I'm not sure this will fly. This means expecting that regulator
framework is reentrant, which I think is not the case.

> But the pwrseq driver for "qcom,qca6390-pmu" could map BT and WLAN compatibles
> to the correct power sequence and then the relevant drivers could enable it
> using pwrseq_power_on().
>
> But that comes back to what I'm doing here: the PCI part for ath11k still
> needs the platform driver that will trigger the power sequence and that could
> be the PCI pwrseq driver for which the framework is introduced in this series.
>
> As I said: the two are largely orthogonal.

I'm fine with that as long as it stays as an RFC. We need to fix both
issues before committing qca6390 power up support.

>
> >>
> >> So I'm thinking that the DT representation should remain as it is:
> >> with separate WLAN and BT nodes consuming resources relevant to their
> >> functionality (BT does not need to enable PCIe regulators).
> >
> > Is it so? The QCA6390 docs clearly say that all regulators should be
> > enabled before asserting BT_EN / WLAN_EN. See the powerup timing
> > diagram and the t2 note to that diagram.
> >
>
> Fair enough.
>
> >> Now how to
> >> handle the QCA6490 model you brought up? How about pwrseq drivers that
> >> would handle the sequence based on compatibles?
> >
> > The QCA6490 is also known as WCN6855. So this problem applies to
> > Qualcomm sm8350 / sm8450 platforms.
> >
> > And strictly speaking I don't see any significant difference between
> > QCA6390 and WCN6855. The regulators might be different, but the
> > implementation should be the same.
> >
> >>
> >> We'd add a new subsystem at drivers/pwrseq/. Inside there would be:
> >> drivers/pwrseq/pwrseq-qca6490.c. The pwrseq framework would expose an
> >> API to "sub-drivers" (in this case: BT serdev driver and the qca6490
> >> power sequencing driver). Now the latter goes:
> >>
> >> struct pwrseq_desc *pwrseq = pwrseq_get(dev);
> >>
> >> And the pwrseq subsystem matches the device's compatible against the
> >> correct, *shared* sequence. The BT driver can do the same at any time.
> >> The pwrseq driver then gets regulators, GPIOs, clocks etc. and will be
> >> responsible for dealing with them.
> >>
> >> In sub-drivers we now do:
> >>
> >> ret = pwrseq_power_on(pwrseq);
> >>
> >> or
> >>
> >> ret = pwrseq_power_off(pwrseq);
> >>
> >> in the sub-device drivers and no longer interact with each regulator
> >> on our own. The pwrseq subsystem is now in charge of adding delays
> >> etc.
> >>
> >> That's only an idea and I haven't done any real work yet but I'm
> >> throwing it out there for discussion.
> >
> > I've been there and I had implemented it in the same way, but rather
> > having the pwrseq as a primary device in DT and parsing end-devices
> > only as a fallback / compatibility case.
> >
>
> Would you mind posting an example DT code here? I'm not sure if I understand
> what "primary device" means in this context.

 qca_pwrseq: qca-pwrseq {
  compatible = "qcom,qca6390-pwrseq";

  #pwrseq-cells = <1>;

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

  bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
  wifi-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
  swctrl-gpios = <&tlmm 124 GPIO_ACTIVE_HIGH>;
 };

&uart6 {
 status = "okay";
 bluetooth {
  compatible = "qcom,qca6390-bt";
  clocks = <&sleep_clk>;

  bt-pwrseq = <&qca_pwrseq 1>;
 };
};

See https://lore.kernel.org/linux-arm-msm/20211006035407.1147909-13-dmitry.baryshkov@linaro.org/

-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Bartosz Golaszewski]

On Fri, Jan 19, 2024 at 3:07 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>

[snip]

> > >
> >
> > Alright, so let's imagine we do model the PMU on the device tree. It would
> > look something like this:
> >
> > qca6390_pmu: pmic@0 {
> >         compatible = "qcom,qca6390-pmu";
> >
> >         bt-gpios = <...>;
> >         wlan-gpios = <...>;
> >
> >         vdd-supply = <&vreg...>;
> >         ...
> >
> >         regulators-0 {
> >                 vreg_x: foo {
> >                         ...
> >                 };
> >
> >                 ...
> >         };
> > };
> >
> > Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously we
> > cannot go:
> >
> > wlan {
> >         pwrseq = &qca6390_pmu;
> > };
> >
> > But it's enough to:
> >
> > wlan {
> >         vdd-supply = <&vreg_x>;
> > };
>
> I'm not sure this will fly. This means expecting that regulator
> framework is reentrant, which I think is not the case.
>

Oh maybe I didn't make myself clear. That's the DT representation of
HW. With pwrseq, the BT or ATH11K drivers wouldn't use the regulator
framework. They would use the pwrseq framework and it could use the
phandle of the regulator to get into the correct pwrseq device without
making Rob and Krzysztof angry.

Bart

[snip]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Bartosz Golaszewski]

On Fri, Jan 19, 2024 at 3:11 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Jan 19, 2024 at 3:07 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
>
> [snip]
>
> > > >
> > >
> > > Alright, so let's imagine we do model the PMU on the device tree. It would
> > > look something like this:
> > >
> > > qca6390_pmu: pmic@0 {
> > >         compatible = "qcom,qca6390-pmu";
> > >
> > >         bt-gpios = <...>;
> > >         wlan-gpios = <...>;
> > >
> > >         vdd-supply = <&vreg...>;
> > >         ...
> > >
> > >         regulators-0 {
> > >                 vreg_x: foo {
> > >                         ...
> > >                 };
> > >
> > >                 ...
> > >         };
> > > };
> > >
> > > Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously we
> > > cannot go:
> > >
> > > wlan {
> > >         pwrseq = &qca6390_pmu;
> > > };
> > >
> > > But it's enough to:
> > >
> > > wlan {
> > >         vdd-supply = <&vreg_x>;
> > > };
> >
> > I'm not sure this will fly. This means expecting that regulator
> > framework is reentrant, which I think is not the case.
> >
>
> Oh maybe I didn't make myself clear. That's the DT representation of
> HW. With pwrseq, the BT or ATH11K drivers wouldn't use the regulator
> framework. They would use the pwrseq framework and it could use the
> phandle of the regulator to get into the correct pwrseq device without
> making Rob and Krzysztof angry.
>
> Bart
>
> [snip]

I'm working on a PoC of the generic pwrseq framework but without the
explicit pwrseq modelling in DT. Should have an RFC ready early next
week.

Bart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[Rob Herring]

On Fri, Jan 19, 2024 at 10:34 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Fri, Jan 19, 2024 at 12:52:00PM +0100, Bartosz Golaszewski wrote:
> > We have two separate issues: one is powering-up a PCI device so that
> > it can be detected
>
> Just wondering, I note in really_probe() we configure the pin controller,
> active the pm_domain etc before probing a driver.
>
> Would it make sense for the issue you mention above to similarly
> amend pci_scan_device() to enable whatever clocks or regulators
> are described in the devicetree as providers for the given PCI device?

If you mean via a callback to some device specific code, then yes, I
think that's exactly what should be done here. That's roughly what
MDIO does. If firmware says there is a device present, then probe it
anyways even if not detected. I don't think that will work for PCI
because it accesses a lot of registers before probe. We'd need some
sort of pre-probe hook instead called after reading vendor and device
ID, but before anything else.

If you mean PCI core just enable whatever clocks and regulators (and
GPIOs), then no, because what is the correct order and timing for each
of those? You don't know because that is device specific information
just as how to program a device is.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Andersson]

On Thu, Jan 18, 2024 at 12:15:27PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 18, 2024 at 11:58:50AM +0100, Bartosz Golaszewski wrote:
> > On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
> > > > 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.
> > >
> > > Ick, why a platform device?  What is the parent of this device, a PCI
> > > device?  If so, then this can't be a platform device, as that's not what
> > > it is, it's something else so make it a device of that type,.
> > >
> > 
> > Greg,
> > 
> > This is literally what we agreed on at LPC. In fact: during one of the
> > hall track discussions I said that you typically NAK any attempts at
> > using the platform bus for "fake" devices but you responded that this
> > is what the USB on-board HUB does and while it's not pretty, this is
> > what we need to do.
> 
> Ah, you need to remind me of these things, this changelog was pretty
> sparse :)
> 

I believe I missed this part of the discussion, why does this need to be
a platform_device? What does the platform_bus bring that can't be
provided by some other bus?

(I'm not questioning the need for having a bus, creating devices, and
matching/binding them to a set of drivers)

Regards,
Bjorn

> > Now as for the implementation, the way I see it we have two solutions:
> > either we introduce a fake, top-level PCI slot platform device device
> > that will reference the PCI host controller by phandle or we will live
> > with a secondary, "virtual" platform device for power sequencing that
> > is tied to the actual PCI device. The former requires us to add DT
> > bindings, add a totally fake DT node representing the "slot" which
> > doesn't really exist (and Krzysztof already expressed his negative
> > opinion of that) and then have code that will be more complex than it
> > needs to be. The latter allows us to not change DT at all (other than
> > adding regulators, clocks and GPIOs to already existing WLAN nodes),
> > reuse the existing parent-child relationship between the port node and
> > the instantiated platform device as well as result in simpler code.
> > 
> > Given that DT needs to be stable while the underlying C code can
> > freely change if we find a better solution, I think that the second
> > option is a no-brainer here.
> 
> Ok, I remove my objections, sorry about that, my confusion.
> 
> greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bartosz Golaszewski]

On Tue, Jan 30, 2024 at 10:54 PM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Jan 18, 2024 at 12:15:27PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 18, 2024 at 11:58:50AM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
> > > > > 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.
> > > >
> > > > Ick, why a platform device?  What is the parent of this device, a PCI
> > > > device?  If so, then this can't be a platform device, as that's not what
> > > > it is, it's something else so make it a device of that type,.
> > > >
> > >
> > > Greg,
> > >
> > > This is literally what we agreed on at LPC. In fact: during one of the
> > > hall track discussions I said that you typically NAK any attempts at
> > > using the platform bus for "fake" devices but you responded that this
> > > is what the USB on-board HUB does and while it's not pretty, this is
> > > what we need to do.
> >
> > Ah, you need to remind me of these things, this changelog was pretty
> > sparse :)
> >
>
> I believe I missed this part of the discussion, why does this need to be
> a platform_device? What does the platform_bus bring that can't be
> provided by some other bus?
>

Does it need to be a platform_device? No, of course not. Does it make
sense for it to be one? Yes, for two reasons:

1. The ATH11K WLAN module is represented on the device tree like a
platform device, we know it's always there and it consumes regulators
from another platform device. The fact it uses PCIe doesn't change the
fact that it is logically a platform device.
2. The platform bus already provides us with the entire infrastructure
that we'd now need to duplicate (possibly adding bugs) in order to
introduce a "power sequencing bus".

Bart

> (I'm not questioning the need for having a bus, creating devices, and
> matching/binding them to a set of drivers)
>
> Regards,
> Bjorn
>

[snip]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Andersson]

On Wed, Jan 31, 2024 at 12:04:14PM +0100, Bartosz Golaszewski wrote:
> On Tue, Jan 30, 2024 at 10:54 PM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Thu, Jan 18, 2024 at 12:15:27PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Jan 18, 2024 at 11:58:50AM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
> > > > > > 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.
> > > > >
> > > > > Ick, why a platform device?  What is the parent of this device, a PCI
> > > > > device?  If so, then this can't be a platform device, as that's not what
> > > > > it is, it's something else so make it a device of that type,.
> > > > >
> > > >
> > > > Greg,
> > > >
> > > > This is literally what we agreed on at LPC. In fact: during one of the
> > > > hall track discussions I said that you typically NAK any attempts at
> > > > using the platform bus for "fake" devices but you responded that this
> > > > is what the USB on-board HUB does and while it's not pretty, this is
> > > > what we need to do.
> > >
> > > Ah, you need to remind me of these things, this changelog was pretty
> > > sparse :)
> > >
> >
> > I believe I missed this part of the discussion, why does this need to be
> > a platform_device? What does the platform_bus bring that can't be
> > provided by some other bus?
> >
> 
> Does it need to be a platform_device? No, of course not. Does it make
> sense for it to be one? Yes, for two reasons:
> 
> 1. The ATH11K WLAN module is represented on the device tree like a
> platform device, we know it's always there and it consumes regulators
> from another platform device. The fact it uses PCIe doesn't change the
> fact that it is logically a platform device.

Are you referring to the ath11k SNOC (firmware running on co-processor
in the SoC) variant?

Afaict the PCIe-attached ath11k is not represented as a platform_device
in DeviceTree.

Said platform_device is also not a child under the PCIe bus, so this
would be a different platform_device...

> 2. The platform bus already provides us with the entire infrastructure
> that we'd now need to duplicate (possibly adding bugs) in order to
> introduce a "power sequencing bus".
> 

This is a perfectly reasonable desire. Look at our PMICs, they are full
of platform_devices. But through the years it's been said many times,
that this is not a valid or good reason for using platform_devices, and
as a result we have e.g. auxiliary bus.

Anyway, (please) don't claim that "we need to", when it actually is "we
want to use platform_device because that's more convenient"!

Regards,
Bjorn

> Bart
> 
> > (I'm not questioning the need for having a bus, creating devices, and
> > matching/binding them to a set of drivers)
> >
> > Regards,
> > Bjorn
> >
> 
> [snip]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jeff Johnson]

On 2/1/2024 4:03 PM, Bjorn Andersson wrote:
> On Wed, Jan 31, 2024 at 12:04:14PM +0100, Bartosz Golaszewski wrote:
>> On Tue, Jan 30, 2024 at 10:54 PM Bjorn Andersson <andersson@kernel.org> wrote:
>>>
>>> On Thu, Jan 18, 2024 at 12:15:27PM +0100, Greg Kroah-Hartman wrote:
>>>> On Thu, Jan 18, 2024 at 11:58:50AM +0100, Bartosz Golaszewski wrote:
>>>>> On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
>>>>> <gregkh@linuxfoundation.org> wrote:
>>>>>>
>>>>>> On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
>>>>>>> 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.
>>>>>>
>>>>>> Ick, why a platform device?  What is the parent of this device, a PCI
>>>>>> device?  If so, then this can't be a platform device, as that's not what
>>>>>> it is, it's something else so make it a device of that type,.
>>>>>>
>>>>>
>>>>> Greg,
>>>>>
>>>>> This is literally what we agreed on at LPC. In fact: during one of the
>>>>> hall track discussions I said that you typically NAK any attempts at
>>>>> using the platform bus for "fake" devices but you responded that this
>>>>> is what the USB on-board HUB does and while it's not pretty, this is
>>>>> what we need to do.
>>>>
>>>> Ah, you need to remind me of these things, this changelog was pretty
>>>> sparse :)
>>>>
>>>
>>> I believe I missed this part of the discussion, why does this need to be
>>> a platform_device? What does the platform_bus bring that can't be
>>> provided by some other bus?
>>>
>>
>> Does it need to be a platform_device? No, of course not. Does it make
>> sense for it to be one? Yes, for two reasons:
>>
>> 1. The ATH11K WLAN module is represented on the device tree like a
>> platform device, we know it's always there and it consumes regulators
>> from another platform device. The fact it uses PCIe doesn't change the
>> fact that it is logically a platform device.
> 
> Are you referring to the ath11k SNOC (firmware running on co-processor
> in the SoC) variant?
> 
> Afaict the PCIe-attached ath11k is not represented as a platform_device
> in DeviceTree.

Are you considering out-of-tree drivers? My understanding is that there
are different PCIe modules, ones that don't have GPIO-control for x86
and ones that do have GPIO-control for ARM. The out-of-tree cnss
platform driver used for Android has a large amount of DT control
<https://git.codelinaro.org/clo/la/platform/vendor/qcom-opensource/wlan/platform/-/blob/wlan-platform.lnx.1.0.r1-rel/cnss2/power.c?ref_type=heads>

(not sure off hand where the DT files themselves are)

/jeff

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bartosz Golaszewski]

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

[snip]

> > >
> > > I believe I missed this part of the discussion, why does this need to be
> > > a platform_device? What does the platform_bus bring that can't be
> > > provided by some other bus?
> > >
> >
> > Does it need to be a platform_device? No, of course not. Does it make
> > sense for it to be one? Yes, for two reasons:
> >
> > 1. The ATH11K WLAN module is represented on the device tree like a
> > platform device, we know it's always there and it consumes regulators
> > from another platform device. The fact it uses PCIe doesn't change the
> > fact that it is logically a platform device.
>
> Are you referring to the ath11k SNOC (firmware running on co-processor
> in the SoC) variant?
>
> Afaict the PCIe-attached ath11k is not represented as a platform_device
> in DeviceTree.
>

My bad. In RB5 it isn't (yet - I want to add it in the power
sequencing series). It is in X13s though[1].

> Said platform_device is also not a child under the PCIe bus, so this
> would be a different platform_device...
>

It's the child of the PCIe port node but there's a reason for it to
have the `compatible` property. It's because it's an entity of whose
existence we are aware before the system boots.

> > 2. The platform bus already provides us with the entire infrastructure
> > that we'd now need to duplicate (possibly adding bugs) in order to
> > introduce a "power sequencing bus".
> >
>
> This is a perfectly reasonable desire. Look at our PMICs, they are full
> of platform_devices. But through the years it's been said many times,
> that this is not a valid or good reason for using platform_devices, and
> as a result we have e.g. auxiliary bus.
>

Ok, so I cannot find this information anywhere (nor any example). Do
you happen to know if the auxiliary bus offers any software node
integration so that the `compatible` property from DT can get
seamlessly mapped to auxiliary device IDs?

> Anyway, (please) don't claim that "we need to", when it actually is "we
> want to use platform_device because that's more convenient"!

Bart

[snip]

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n744

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bartosz Golaszewski]

On Fri, Feb 2, 2024 at 11:02 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Feb 2, 2024 at 1:03 AM Bjorn Andersson <andersson@kernel.org> wrote:
> >
>
> [snip]
>
> > > >
> > > > I believe I missed this part of the discussion, why does this need to be
> > > > a platform_device? What does the platform_bus bring that can't be
> > > > provided by some other bus?
> > > >
> > >
> > > Does it need to be a platform_device? No, of course not. Does it make
> > > sense for it to be one? Yes, for two reasons:
> > >
> > > 1. The ATH11K WLAN module is represented on the device tree like a
> > > platform device, we know it's always there and it consumes regulators
> > > from another platform device. The fact it uses PCIe doesn't change the
> > > fact that it is logically a platform device.
> >
> > Are you referring to the ath11k SNOC (firmware running on co-processor
> > in the SoC) variant?
> >
> > Afaict the PCIe-attached ath11k is not represented as a platform_device
> > in DeviceTree.
> >
>
> My bad. In RB5 it isn't (yet - I want to add it in the power
> sequencing series). It is in X13s though[1].
>
> > Said platform_device is also not a child under the PCIe bus, so this
> > would be a different platform_device...
> >
>
> It's the child of the PCIe port node but there's a reason for it to
> have the `compatible` property. It's because it's an entity of whose
> existence we are aware before the system boots.
>
> > > 2. The platform bus already provides us with the entire infrastructure
> > > that we'd now need to duplicate (possibly adding bugs) in order to
> > > introduce a "power sequencing bus".
> > >
> >
> > This is a perfectly reasonable desire. Look at our PMICs, they are full
> > of platform_devices. But through the years it's been said many times,
> > that this is not a valid or good reason for using platform_devices, and
> > as a result we have e.g. auxiliary bus.
> >
>
> Ok, so I cannot find this information anywhere (nor any example). Do
> you happen to know if the auxiliary bus offers any software node
> integration so that the `compatible` property from DT can get
> seamlessly mapped to auxiliary device IDs?
>

So I was just trying to port this to using the auxiliary bus, only to
find myself literally reimplementing functions from
drivers/of/device.c. I have a feeling that this is simply wrong. If
we're instantiating devices well defined on the device-tree then IMO
we *should* make them platform devices. Anything else and we'll be
reimplementing drivers/of/ because we will need to parse the device
nodes, check the compatible, match it against drivers etc. Things that
are already implemented for the platform bus and of_* APIs.

Greg: Could you chime in and confirm that it's alright to use the
platform bus here? Or maybe there is some infrastructure to create
auxiliary devices from software nodes?

Bartosz

> > Anyway, (please) don't claim that "we need to", when it actually is "we
> > want to use platform_device because that's more convenient"!
>
> Bart
>
> [snip]
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n744

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg Kroah-Hartman]

On Wed, Feb 07, 2024 at 05:32:38PM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 2, 2024 at 11:02 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Fri, Feb 2, 2024 at 1:03 AM Bjorn Andersson <andersson@kernel.org> wrote:
> > >
> >
> > [snip]
> >
> > > > >
> > > > > I believe I missed this part of the discussion, why does this need to be
> > > > > a platform_device? What does the platform_bus bring that can't be
> > > > > provided by some other bus?
> > > > >
> > > >
> > > > Does it need to be a platform_device? No, of course not. Does it make
> > > > sense for it to be one? Yes, for two reasons:
> > > >
> > > > 1. The ATH11K WLAN module is represented on the device tree like a
> > > > platform device, we know it's always there and it consumes regulators
> > > > from another platform device. The fact it uses PCIe doesn't change the
> > > > fact that it is logically a platform device.
> > >
> > > Are you referring to the ath11k SNOC (firmware running on co-processor
> > > in the SoC) variant?
> > >
> > > Afaict the PCIe-attached ath11k is not represented as a platform_device
> > > in DeviceTree.
> > >
> >
> > My bad. In RB5 it isn't (yet - I want to add it in the power
> > sequencing series). It is in X13s though[1].
> >
> > > Said platform_device is also not a child under the PCIe bus, so this
> > > would be a different platform_device...
> > >
> >
> > It's the child of the PCIe port node but there's a reason for it to
> > have the `compatible` property. It's because it's an entity of whose
> > existence we are aware before the system boots.
> >
> > > > 2. The platform bus already provides us with the entire infrastructure
> > > > that we'd now need to duplicate (possibly adding bugs) in order to
> > > > introduce a "power sequencing bus".
> > > >
> > >
> > > This is a perfectly reasonable desire. Look at our PMICs, they are full
> > > of platform_devices. But through the years it's been said many times,
> > > that this is not a valid or good reason for using platform_devices, and
> > > as a result we have e.g. auxiliary bus.
> > >
> >
> > Ok, so I cannot find this information anywhere (nor any example). Do
> > you happen to know if the auxiliary bus offers any software node
> > integration so that the `compatible` property from DT can get
> > seamlessly mapped to auxiliary device IDs?
> >
> 
> So I was just trying to port this to using the auxiliary bus, only to
> find myself literally reimplementing functions from
> drivers/of/device.c. I have a feeling that this is simply wrong. If
> we're instantiating devices well defined on the device-tree then IMO
> we *should* make them platform devices. Anything else and we'll be
> reimplementing drivers/of/ because we will need to parse the device
> nodes, check the compatible, match it against drivers etc. Things that
> are already implemented for the platform bus and of_* APIs.
> 
> Greg: Could you chime in and confirm that it's alright to use the
> platform bus here? Or maybe there is some infrastructure to create
> auxiliary devices from software nodes?

Note, I HATE the use of the platform bus here, but I don't have a better
suggestion.

I'd love for the auxbus to work, and if you can create that from
software nodes, all the better!  But I don't think that's possible just
yet, and you would end up implementing all the same stuff that the
platform bus has today for this functionality, so I doubt it would be
worth it.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel