[PATCH v3 0/5] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[PATCH v3 0/5] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series reworks the PCI pwrctrl integration (again) by moving the creation
and removal of pwrctrl devices to pci_scan_device() and pci_destroy_dev() APIs.
This is based on the suggestion provided by Lukas Wunner [1][2]. With this
change, it is now possible to create pwrctrl devices for PCI bridges as well.
This is required to control the power state of the PCI slots in a system. Since
the PCI slots are not explicitly defined in devicetree, the agreement is to
define the supplies for PCI slots in PCI bridge nodes itself [3].

Based on this, a pwrctrl driver to control the supplies of PCI slots are also
added in patch 4. With this driver, it is now possible to control the voltage
regulators powering the PCI slots defined in PCI bridge nodes as below:

```
pcie@0 {
	compatible "pciclass,0604"
	...

	vpcie12v-supply = <&vpcie12v_reg>;
	vpcie3v3-supply = <&vpcie3v3_reg>;
	vpcie3v3aux-supply = <&vpcie3v3aux_reg>;
};
```

To make use of this driver, the PCI bridge DT node should also have the
compatible "pciclass,0604". But adding this compatible triggers the following
checkpatch warning:

WARNING: DT compatible string vendor "pciclass" appears un-documented --
check ./Documentation/devicetree/bindings/vendor-prefixes.yaml

For fixing it, I added patch 3. But due to some reason, checkpatch is not
picking the 'pciclass' vendor prefix alone, and requires adding the full
compatible 'pciclass,0604' in the vendor-prefixes list. Since my perl skills are
not great, I'm leaving it in the hands of Rob to fix the checkpatch script.

[1] https://lore.kernel.org/linux-pci/Z0yLDBMAsh0yKWf2@wunner.de
[2] https://lore.kernel.org/linux-pci/Z0xAdQ2ozspEnV5g@wunner.de
[3] https://github.com/devicetree-org/dt-schema/issues/145

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Changes in v3:
- Fixed the Kbuild bot issue with a regulator patch and submitted it separately
  along with one more regulator patch from this series. Both patches got merged
  and are in linux-next.
- Collected tags

Changes in v2:
- Added new patch to skip the device scan if pwrctrl device is found
- Added a new patch to fix the build issue
- Collected tags
- Link to v1: https://lore.kernel.org/r/20241210-pci-pwrctrl-slot-v1-0-eae45e488040@linaro.org

---
Manivannan Sadhasivam (5):
      PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()
      PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()
      PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created
      dt-bindings: vendor-prefixes: Document the 'pciclass' prefix
      PCI/pwrctrl: Add pwrctrl driver for PCI Slots

 .../devicetree/bindings/vendor-prefixes.yaml       |  2 +-
 drivers/pci/bus.c                                  | 43 ----------
 drivers/pci/probe.c                                | 41 ++++++++++
 drivers/pci/pwrctrl/Kconfig                        | 11 +++
 drivers/pci/pwrctrl/Makefile                       |  3 +
 drivers/pci/pwrctrl/core.c                         |  2 +-
 drivers/pci/pwrctrl/slot.c                         | 93 ++++++++++++++++++++++
 drivers/pci/remove.c                               |  2 +-
 8 files changed, 151 insertions(+), 46 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241210-pci-pwrctrl-slot-02c0ec63172f

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

[PATCH v3 1/5] PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Current way of creating pwrctrl devices requires iterating through the
child devicetree nodes of the PCI bridge in pci_pwrctrl_create_devices().
Even though it works, it creates confusion as there is no symmetry between
this and pci_pwrctrl_unregister() function that removes the pwrctrl
devices.

So to make these two functions symmetric, move the creation of pwrctrl
devices to pci_scan_device(). During the scan of each device in a slot,
the devicetree node (if exists) for the PCI device will be checked. If it
has the supplies populated, then the pwrctrl device will be created.

Since the PCI device scan happens so early, there would be no 'struct
pci_dev' available for the device. So the host bridge is used as the parent
of all pwrctrl devices.

One nice side effect of this move is that, it is now possible to have
pwrctrl devices for PCI bridges as well (to control the supplies of PCI
slots).

Suggested-by: Lukas Wunner <lukas@wunner.de>
Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/bus.c          | 43 -------------------------------------------
 drivers/pci/probe.c        | 34 ++++++++++++++++++++++++++++++++++
 drivers/pci/pwrctrl/core.c |  2 +-
 3 files changed, 35 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 98910bc0fcc4..b6851101ac36 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -331,47 +331,6 @@ void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
 
 void __weak pcibios_bus_add_device(struct pci_dev *pdev) { }
 
-/*
- * Create pwrctrl devices (if required) for the PCI devices to handle the power
- * state.
- */
-static void pci_pwrctrl_create_devices(struct pci_dev *dev)
-{
-	struct device_node *np = dev_of_node(&dev->dev);
-	struct device *parent = &dev->dev;
-	struct platform_device *pdev;
-
-	/*
-	 * First ensure that we are starting from a PCI bridge and it has a
-	 * corresponding devicetree node.
-	 */
-	if (np && pci_is_bridge(dev)) {
-		/*
-		 * Now look for the child PCI device nodes and create pwrctrl
-		 * devices for them. The pwrctrl device drivers will manage the
-		 * power state of the devices.
-		 */
-		for_each_available_child_of_node_scoped(np, child) {
-			/*
-			 * First check whether the pwrctrl device really
-			 * needs to be created or not. This is decided
-			 * based on at least one of the power supplies
-			 * being defined in the devicetree node of the
-			 * device.
-			 */
-			if (!of_pci_supply_present(child)) {
-				pci_dbg(dev, "skipping OF node: %s\n", child->name);
-				return;
-			}
-
-			/* Now create the pwrctrl device */
-			pdev = of_platform_device_create(child, NULL, parent);
-			if (!pdev)
-				pci_err(dev, "failed to create OF node: %s\n", child->name);
-		}
-	}
-}
-
 /**
  * pci_bus_add_device - start driver for a single device
  * @dev: device to add
@@ -396,8 +355,6 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);
 
-	pci_pwrctrl_create_devices(dev);
-
 	/*
 	 * If the PCI device is associated with a pwrctrl device with a
 	 * power supply, create a device link between the PCI device and
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2e81ab0f5a25..91bdb2727c8e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -9,6 +9,8 @@
 #include <linux/pci.h>
 #include <linux/msi.h>
 #include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
 #include <linux/pci_hotplug.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -2446,6 +2448,36 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
 }
 EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
 
+/*
+ * Create pwrctrl device (if required) for the PCI device to handle the power
+ * state.
+ */
+static void pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
+{
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
+	struct platform_device *pdev;
+	struct device_node *np;
+
+	np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
+	if (!np || of_find_device_by_node(np))
+		return;
+
+	/*
+	 * First check whether the pwrctrl device really needs to be created or
+	 * not. This is decided based on at least one of the power supplies
+	 * being defined in the devicetree node of the device.
+	 */
+	if (!of_pci_supply_present(np)) {
+		pr_debug("PCI/pwrctrl: Skipping OF node: %s\n", np->name);
+		return;
+	}
+
+	/* Now create the pwrctrl device */
+	pdev = of_platform_device_create(np, NULL, &host->dev);
+	if (!pdev)
+		pr_err("PCI/pwrctrl: Failed to create pwrctrl device for OF node: %s\n", np->name);
+}
+
 /*
  * Read the config data for a PCI device, sanity-check it,
  * and fill in the dev structure.
@@ -2455,6 +2487,8 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	struct pci_dev *dev;
 	u32 l;
 
+	pci_pwrctrl_create_device(bus, devfn);
+
 	if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
 		return NULL;
 
diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
index 2fb174db91e5..9cc7e2b7f2b5 100644
--- a/drivers/pci/pwrctrl/core.c
+++ b/drivers/pci/pwrctrl/core.c
@@ -44,7 +44,7 @@ static void rescan_work_func(struct work_struct *work)
 						   struct pci_pwrctrl, work);
 
 	pci_lock_rescan_remove();
-	pci_rescan_bus(to_pci_dev(pwrctrl->dev->parent)->bus);
+	pci_rescan_bus(to_pci_host_bridge(pwrctrl->dev->parent)->bus);
 	pci_unlock_rescan_remove();
 }
 

-- 
2.25.1

[PATCH v3 2/5] PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

PCI core will try to access the devices even after pci_stop_dev() for
things like Data Object Exchange (DOE), ASPM etc... So move
pci_pwrctrl_unregister() to the near end of pci_destroy_dev() to make sure
that the devices are powered down only after the PCI core is done with
them.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/remove.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index efc37fcb73e2..58859f9d92f7 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -41,7 +41,6 @@ static void pci_stop_dev(struct pci_dev *dev)
 	if (!pci_dev_test_and_clear_added(dev))
 		return;
 
-	pci_pwrctrl_unregister(&dev->dev);
 	device_release_driver(&dev->dev);
 	pci_proc_detach_device(dev);
 	pci_remove_sysfs_dev_files(dev);
@@ -64,6 +63,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	pci_doe_destroy(dev);
 	pcie_aspm_exit_link_state(dev);
 	pci_bridge_d3_update(dev);
+	pci_pwrctrl_unregister(&dev->dev);
 	pci_free_resources(dev);
 	put_device(&dev->dev);
 }

-- 
2.25.1

[PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

The pwrctrl core will rescan the bus once the device is powered on. So
there is no need to continue scanning for the device further.

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/probe.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 91bdb2727c8e..6121f81f7c98 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2448,11 +2448,7 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
 }
 EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
 
-/*
- * Create pwrctrl device (if required) for the PCI device to handle the power
- * state.
- */
-static void pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
+static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
 {
 	struct pci_host_bridge *host = pci_find_host_bridge(bus);
 	struct platform_device *pdev;
@@ -2460,7 +2456,7 @@ static void pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
 
 	np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
 	if (!np || of_find_device_by_node(np))
-		return;
+		return NULL;
 
 	/*
 	 * First check whether the pwrctrl device really needs to be created or
@@ -2469,13 +2465,17 @@ static void pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
 	 */
 	if (!of_pci_supply_present(np)) {
 		pr_debug("PCI/pwrctrl: Skipping OF node: %s\n", np->name);
-		return;
+		return NULL;
 	}
 
 	/* Now create the pwrctrl device */
 	pdev = of_platform_device_create(np, NULL, &host->dev);
-	if (!pdev)
-		pr_err("PCI/pwrctrl: Failed to create pwrctrl device for OF node: %s\n", np->name);
+	if (!pdev) {
+		pr_err("PCI/pwrctrl: Failed to create pwrctrl device for node: %s\n", np->name);
+		return NULL;
+	}
+
+	return pdev;
 }
 
 /*
@@ -2487,7 +2487,14 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	struct pci_dev *dev;
 	u32 l;
 
-	pci_pwrctrl_create_device(bus, devfn);
+	/*
+	 * Create pwrctrl device (if required) for the PCI device to handle the
+	 * power state. If the pwrctrl device is created, then skip scanning
+	 * further as the pwrctrl core will rescan the bus after powering on
+	 * the device.
+	 */
+	if (pci_pwrctrl_create_device(bus, devfn))
+		return NULL;
 
 	if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
 		return NULL;

-- 
2.25.1

[PATCH v3 4/5] dt-bindings: vendor-prefixes: Document the 'pciclass' prefix

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

'pciclass' is an existing prefix used to identify the PCI bridge devices,
but it is not a vendor prefix. So document it in the non-vendor prefix
list.

Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index da01616802c7..0539aea6af5a 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -18,7 +18,7 @@ patternProperties:
   # DO NOT ADD NEW PROPERTIES TO THIS LIST
   "^(at25|bm|devbus|dmacap|dsa|exynos|fsi[ab]|gpio-fan|gpio-key|gpio|gpmc|hdmi|i2c-gpio),.*": true
   "^(keypad|m25p|max8952|max8997|max8998|mpmc),.*": true
-  "^(pinctrl-single|#pinctrl-single|PowerPC),.*": true
+  "^(pciclass|pinctrl-single|#pinctrl-single|PowerPC),.*": true
   "^(pl022|pxa-mmc|rcar_sound|rotary-encoder|s5m8767|sdhci),.*": true
   "^(simple-audio-card|st-plgpio|st-spics|ts),.*": true
 

-- 
2.25.1

[PATCH v3 5/5] PCI/pwrctrl: Add pwrctrl driver for PCI Slots

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

This driver is used to control the power state of the devices attached to
the PCI slots. Currently, it controls the voltage rails of the PCI slots
defined in the devicetree node of the root port.

The voltage rails for PCI slots are documented in the dt-schema:
https://github.com/devicetree-org/dt-schema/blob/v2024.11/dtschema/schemas/pci/pci-bus-common.yaml#L153

Since this driver has to work with different kind of slots (x{1/4/8/16}
PCIe, Mini PCIe, PCI etc...), the driver is using the
of_regulator_bulk_get_all() API to obtain the voltage regulators defined
in the DT node, instead of hardcoding them. The DT node of the root port
should define the relevant supply properties corresponding to the voltage
rails of the PCI slot.

Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pwrctrl/Kconfig  | 11 ++++++
 drivers/pci/pwrctrl/Makefile |  3 ++
 drivers/pci/pwrctrl/slot.c   | 93 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)

diff --git a/drivers/pci/pwrctrl/Kconfig b/drivers/pci/pwrctrl/Kconfig
index 54589bb2403b..990cab67d413 100644
--- a/drivers/pci/pwrctrl/Kconfig
+++ b/drivers/pci/pwrctrl/Kconfig
@@ -10,3 +10,14 @@ config PCI_PWRCTL_PWRSEQ
 	tristate
 	select POWER_SEQUENCING
 	select PCI_PWRCTL
+
+config PCI_PWRCTL_SLOT
+	tristate "PCI Power Control driver for PCI slots"
+	select PCI_PWRCTL
+	help
+	  Say Y here to enable the PCI Power Control driver to control the power
+	  state of PCI slots.
+
+	  This is a generic driver that controls the power state of different
+	  PCI slots. The voltage regulators powering the rails of the PCI slots
+	  are expected to be defined in the devicetree node of the PCI bridge.
diff --git a/drivers/pci/pwrctrl/Makefile b/drivers/pci/pwrctrl/Makefile
index 75c7ce531c7e..ddfb12c5aadf 100644
--- a/drivers/pci/pwrctrl/Makefile
+++ b/drivers/pci/pwrctrl/Makefile
@@ -4,3 +4,6 @@ obj-$(CONFIG_PCI_PWRCTL)		+= pci-pwrctrl-core.o
 pci-pwrctrl-core-y			:= core.o
 
 obj-$(CONFIG_PCI_PWRCTL_PWRSEQ)		+= pci-pwrctrl-pwrseq.o
+
+obj-$(CONFIG_PCI_PWRCTL_SLOT)		+= pci-pwrctl-slot.o
+pci-pwrctl-slot-y			:= slot.o
diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
new file mode 100644
index 000000000000..18becc144913
--- /dev/null
+++ b/drivers/pci/pwrctrl/slot.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+ */
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pci-pwrctrl.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+struct pci_pwrctrl_slot_data {
+	struct pci_pwrctrl ctx;
+	struct regulator_bulk_data *supplies;
+	int num_supplies;
+};
+
+static void devm_pci_pwrctrl_slot_power_off(void *data)
+{
+	struct pci_pwrctrl_slot_data *slot = data;
+
+	regulator_bulk_disable(slot->num_supplies, slot->supplies);
+	regulator_bulk_free(slot->num_supplies, slot->supplies);
+}
+
+static int pci_pwrctrl_slot_probe(struct platform_device *pdev)
+{
+	struct pci_pwrctrl_slot_data *slot;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	slot = devm_kzalloc(dev, sizeof(*slot), GFP_KERNEL);
+	if (!slot)
+		return -ENOMEM;
+
+	ret = of_regulator_bulk_get_all(dev, dev_of_node(dev),
+					&slot->supplies);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to get slot regulators\n");
+		return ret;
+	}
+
+	slot->num_supplies = ret;
+	ret = regulator_bulk_enable(slot->num_supplies, slot->supplies);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to enable slot regulators\n");
+		goto err_regulator_free;
+	}
+
+	ret = devm_add_action_or_reset(dev, devm_pci_pwrctrl_slot_power_off,
+				       slot);
+	if (ret)
+		goto err_regulator_disable;
+
+	pci_pwrctrl_init(&slot->ctx, dev);
+
+	ret = devm_pci_pwrctrl_device_set_ready(dev, &slot->ctx);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register pwrctrl driver\n");
+
+	return 0;
+
+err_regulator_disable:
+	regulator_bulk_disable(slot->num_supplies, slot->supplies);
+err_regulator_free:
+	regulator_bulk_free(slot->num_supplies, slot->supplies);
+
+	return ret;
+}
+
+static const struct of_device_id pci_pwrctrl_slot_of_match[] = {
+	{
+		.compatible = "pciclass,0604",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pci_pwrctrl_slot_of_match);
+
+static struct platform_driver pci_pwrctrl_slot_driver = {
+	.driver = {
+		.name = "pci-pwrctrl-slot",
+		.of_match_table = pci_pwrctrl_slot_of_match,
+	},
+	.probe = pci_pwrctrl_slot_probe,
+};
+module_platform_driver(pci_pwrctrl_slot_driver);
+
+MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
+MODULE_DESCRIPTION("Generic PCI Power Control driver for PCI Slots");
+MODULE_LICENSE("GPL");

-- 
2.25.1

Re: [PATCH v3 0/5] PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot

[Krzysztof Wilczyński]

Hello,

> This series reworks the PCI pwrctrl integration (again) by moving the creation
> and removal of pwrctrl devices to pci_scan_device() and pci_destroy_dev() APIs.
> This is based on the suggestion provided by Lukas Wunner [1][2]. With this
> change, it is now possible to create pwrctrl devices for PCI bridges as well.
> This is required to control the power state of the PCI slots in a system. Since
> the PCI slots are not explicitly defined in devicetree, the agreement is to
> define the supplies for PCI slots in PCI bridge nodes itself [3].

Applied to pwrctrl, thank you!

	Krzysztof

Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Bjorn Helgaas]

On Thu, Jan 16, 2025 at 07:39:13PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> The pwrctrl core will rescan the bus once the device is powered on. So
> there is no need to continue scanning for the device further.

> @@ -2487,7 +2487,14 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
>  	struct pci_dev *dev;
>  	u32 l;
>  
> -	pci_pwrctrl_create_device(bus, devfn);
> +	/*
> +	 * Create pwrctrl device (if required) for the PCI device to handle the
> +	 * power state. If the pwrctrl device is created, then skip scanning
> +	 * further as the pwrctrl core will rescan the bus after powering on
> +	 * the device.
> +	 */
> +	if (pci_pwrctrl_create_device(bus, devfn))
> +		return NULL;

I assume it's possible for the PCI device to already be powered on
even if there's a pwrctrl device for it?

Does this change the enumeration order in that case?  It sounds like
it may delay enumeration of the PCI device until the pwrctrl core
rescans the bus?

I hope that the enumeration order of all devices that are already
powered on at boot time is unchanged.

>  	if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
>  		return NULL;
> 
> -- 
> 2.25.1
> 
> 

Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Bjorn Helgaas]

On Thu, Jan 16, 2025 at 07:39:13PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> The pwrctrl core will rescan the bus once the device is powered on. So
> there is no need to continue scanning for the device further.

> -/*
> - * Create pwrctrl device (if required) for the PCI device to handle the power
> - * state.
> - */
> -static void pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> +static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)

> +	 * Create pwrctrl device (if required) for the PCI device to handle the
> +	 * power state. If the pwrctrl device is created, then skip scanning
> +	 * further as the pwrctrl core will rescan the bus after powering on
> +	 * the device.

I know you only moved the first sentence, but I think "power state" is
likely to be confused with the D0, D1, D2, D3hot, D3cold power
management states.  Maybe we could reword this to talk about power
control, power sequencing, power supplies, power regulators or
something?

> +	 */
> +	if (pci_pwrctrl_create_device(bus, devfn))
> +		return NULL;
>  
>  	if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
>  		return NULL;
> 
> -- 
> 2.25.1
> 
> 

Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Thu, Feb 20, 2025 at 05:24:51PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 16, 2025 at 07:39:13PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > The pwrctrl core will rescan the bus once the device is powered on. So
> > there is no need to continue scanning for the device further.
> 
> > @@ -2487,7 +2487,14 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
> >  	struct pci_dev *dev;
> >  	u32 l;
> >  
> > -	pci_pwrctrl_create_device(bus, devfn);
> > +	/*
> > +	 * Create pwrctrl device (if required) for the PCI device to handle the
> > +	 * power state. If the pwrctrl device is created, then skip scanning
> > +	 * further as the pwrctrl core will rescan the bus after powering on
> > +	 * the device.
> > +	 */
> > +	if (pci_pwrctrl_create_device(bus, devfn))
> > +		return NULL;
> 
> I assume it's possible for the PCI device to already be powered on
> even if there's a pwrctrl device for it?
> 

Yes, if the device was powered on by the bootloader.

> Does this change the enumeration order in that case?  It sounds like
> it may delay enumeration of the PCI device until the pwrctrl core
> rescans the bus?
> 

So previously, while enumerating a PCI device that requires a pwrctrl device
(indicated by DT), its client driver won't be probed until the pwrctrl driver is
probed (thanks to devlink). This was required to make sure that there would be
no race between client drivers and pwrctrl drivers probing parallely.

So in that case, there is no reason to enumerate the such devices in the first
place. That's why this patch is skipping the enumeration for those devices.

- Mani

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

Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Thu, Feb 20, 2025 at 05:35:20PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 16, 2025 at 07:39:13PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > The pwrctrl core will rescan the bus once the device is powered on. So
> > there is no need to continue scanning for the device further.
> 
> > -/*
> > - * Create pwrctrl device (if required) for the PCI device to handle the power
> > - * state.
> > - */
> > -static void pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> > +static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, int devfn)
> 
> > +	 * Create pwrctrl device (if required) for the PCI device to handle the
> > +	 * power state. If the pwrctrl device is created, then skip scanning
> > +	 * further as the pwrctrl core will rescan the bus after powering on
> > +	 * the device.
> 
> I know you only moved the first sentence, but I think "power state" is
> likely to be confused with the D0, D1, D2, D3hot, D3cold power
> management states.  Maybe we could reword this to talk about power
> control, power sequencing, power supplies, power regulators or
> something?
> 

Sure. It could be changed to "power control." I hope Krzysztof can ammend it in
the branch.

- Mani

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

Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Wei Fang]

@@ -2487,7 +2487,14 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	struct pci_dev *dev;
 	u32 l;
 
-	pci_pwrctrl_create_device(bus, devfn);
+	/*
+	 * Create pwrctrl device (if required) for the PCI device to handle the
+	 * power state. If the pwrctrl device is created, then skip scanning
+	 * further as the pwrctrl core will rescan the bus after powering on
+	 * the device.
+	 */
+	if (pci_pwrctrl_create_device(bus, devfn))
+		return NULL;

Hi Manivannan,

The current patch logic is that if the pcie device node is found to have
the "xxx-supply" property, the scan will be skipped, and then the pwrctrl
driver will rescan and enable the regulators. However, after merging this
patch, there is a problem on our platform. The .probe() of our device
driver will not be called. The reason is that CONFIG_PCI_PWRCTL_SLOT is
not enabled at all in our configuration file, and the compatible string
of the device is also not added to the pwrctrl driver. I think other
platforms should also have similar problems, which undoubtedly make these
platforms be unstable. This patch has been applied, and I am not familiar
with this. Can you fix this problem? I mean that those platforms that do
not use pwrctrl can avoid skipping the scan.

Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

Hi,

On March 21, 2025 8:29:40 AM GMT+05:30, Wei Fang <wei.fang@nxp.com> wrote:
>@@ -2487,7 +2487,14 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
> 	struct pci_dev *dev;
> 	u32 l;
> 
>-	pci_pwrctrl_create_device(bus, devfn);
>+	/*
>+	 * Create pwrctrl device (if required) for the PCI device to handle the
>+	 * power state. If the pwrctrl device is created, then skip scanning
>+	 * further as the pwrctrl core will rescan the bus after powering on
>+	 * the device.
>+	 */
>+	if (pci_pwrctrl_create_device(bus, devfn))
>+		return NULL;
>
>Hi Manivannan,
>
>The current patch logic is that if the pcie device node is found to have
>the "xxx-supply" property, the scan will be skipped, and then the pwrctrl
>driver will rescan and enable the regulators. However, after merging this
>patch, there is a problem on our platform. The .probe() of our device
>driver will not be called. The reason is that CONFIG_PCI_PWRCTL_SLOT is
>not enabled at all in our configuration file, and the compatible string
>of the device is also not added to the pwrctrl driver.

Hmm. So I guess the controller driver itself is enabling the supplies I believe (which I failed to spot). May I know what platforms are affected?

> I think other
>platforms should also have similar problems, which undoubtedly make these
>platforms be unstable. This patch has been applied, and I am not familiar
>with this. Can you fix this problem? I mean that those platforms that do
>not use pwrctrl can avoid skipping the scan.

Sure. It makes sense to add a check to see if the pwrctrl driver is enabled or not. If it is not enabled, then the pwrctrl device creation could be skipped. I'll send a patch once I'm infront of my computer.

But in the long run, we would like to move all platforms to use pwrctrl instead of fiddling the power supplies in the controller driver (well that was the motivation to introduce it in the first place).

Once you share the platform details, I'll try to migrate it to use pwrctrl. Also, please note that we are going to enable pwrctrl in ARM64 defconfig very soon. So I'll try to fix the affected platforms before that happens.

- Mani

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

RE: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Wei Fang]

> Hi,
> 
> On March 21, 2025 8:29:40 AM GMT+05:30, Wei Fang <wei.fang@nxp.com>
> wrote:
> >@@ -2487,7 +2487,14 @@ static struct pci_dev *pci_scan_device(struct
> pci_bus *bus, int devfn)
> > 	struct pci_dev *dev;
> > 	u32 l;
> >
> >-	pci_pwrctrl_create_device(bus, devfn);
> >+	/*
> >+	 * Create pwrctrl device (if required) for the PCI device to handle the
> >+	 * power state. If the pwrctrl device is created, then skip scanning
> >+	 * further as the pwrctrl core will rescan the bus after powering on
> >+	 * the device.
> >+	 */
> >+	if (pci_pwrctrl_create_device(bus, devfn))
> >+		return NULL;
> >
> >Hi Manivannan,
> >
> >The current patch logic is that if the pcie device node is found to
> >have the "xxx-supply" property, the scan will be skipped, and then the
> >pwrctrl driver will rescan and enable the regulators. However, after
> >merging this patch, there is a problem on our platform. The .probe() of
> >our device driver will not be called. The reason is that
> >CONFIG_PCI_PWRCTL_SLOT is not enabled at all in our configuration file,
> >and the compatible string of the device is also not added to the pwrctrl driver.
> 
> Hmm. So I guess the controller driver itself is enabling the supplies I believe
> (which I failed to spot). May I know what platforms are affected?

Yes, the affected device is an Ethernet controller on our i.MX95
platform, it has a "phy-supply" property to control the power of the
external Ethernet PHY chip in the device driver. This part has not been
pushed upstream yet. So for upstream tree, there is no need to fix our
platform, but I am not sure whether other platforms are affected by
this on the upstream tree.

> 
> > I think other
> >platforms should also have similar problems, which undoubtedly make
> >these platforms be unstable. This patch has been applied, and I am not
> >familiar with this. Can you fix this problem? I mean that those
> >platforms that do not use pwrctrl can avoid skipping the scan.
> 
> Sure. It makes sense to add a check to see if the pwrctrl driver is enabled or not.
> If it is not enabled, then the pwrctrl device creation could be skipped. I'll send a
> patch once I'm infront of my computer.
> 

I don't know whether check the pwrctrl driver is enabled is a good idea,
for some devices it is more convenient to manage these regulators in
their drivers, for some devices, we may want pwrctrl driver to manage
the regulators. If both types of devices appear on the same platform,
it is not enough to just check whether the pinctrl driver is enabled.

> But in the long run, we would like to move all platforms to use pwrctrl instead of
> fiddling the power supplies in the controller driver (well that was the motivation
> to introduce it in the first place).
> 

I understand this, but it should be compatible with the old method
instead of completely making the old method inoperable unless it
can be confirmed that all platforms in the upstream have completed
the conversion to use pwrctrl driver. Obviously, this is difficult to
confirm. :(

> Once you share the platform details, I'll try to migrate it to use pwrctrl. Also,
> please note that we are going to enable pwrctrl in ARM64 defconfig very soon.
> So I'll try to fix the affected platforms before that happens.

I think the current pwrctrl driver should still be in the early stage. If I
understand correctly, it should only enable the regulators when probing
and disable the regulators when removing. This does not meet all the use
cases at present. So I'm not sure how you can do the fixes for all the affected
platforms in pwrctrl driver for different use cases?

For example, some Ethernet devices need to support suspend/resume and
Wake-on-LAN (WOL). If WOL is not enabled, the power of the external PHY
needs to be turned off when it enters suspend state. If WOL is enabled, the
power of the external PHY needs to be kept on. So for this case, I think you
need to at least add PM interfaces in pwrctrl driver. For the other use cases,
other solutions may be needed.

Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Fri, Mar 21, 2025 at 07:04:24AM +0000, Wei Fang wrote:
> > Hi,
> > 
> > On March 21, 2025 8:29:40 AM GMT+05:30, Wei Fang <wei.fang@nxp.com>
> > wrote:
> > >@@ -2487,7 +2487,14 @@ static struct pci_dev *pci_scan_device(struct
> > pci_bus *bus, int devfn)
> > > 	struct pci_dev *dev;
> > > 	u32 l;
> > >
> > >-	pci_pwrctrl_create_device(bus, devfn);
> > >+	/*
> > >+	 * Create pwrctrl device (if required) for the PCI device to handle the
> > >+	 * power state. If the pwrctrl device is created, then skip scanning
> > >+	 * further as the pwrctrl core will rescan the bus after powering on
> > >+	 * the device.
> > >+	 */
> > >+	if (pci_pwrctrl_create_device(bus, devfn))
> > >+		return NULL;
> > >
> > >Hi Manivannan,
> > >
> > >The current patch logic is that if the pcie device node is found to
> > >have the "xxx-supply" property, the scan will be skipped, and then the
> > >pwrctrl driver will rescan and enable the regulators. However, after
> > >merging this patch, there is a problem on our platform. The .probe() of
> > >our device driver will not be called. The reason is that
> > >CONFIG_PCI_PWRCTL_SLOT is not enabled at all in our configuration file,
> > >and the compatible string of the device is also not added to the pwrctrl driver.
> > 
> > Hmm. So I guess the controller driver itself is enabling the supplies I believe
> > (which I failed to spot). May I know what platforms are affected?
> 
> Yes, the affected device is an Ethernet controller on our i.MX95
> platform, it has a "phy-supply" property to control the power of the
> external Ethernet PHY chip in the device driver.

Ah, I was not aware of any devices using 'phy-supply' in the pcie device node.

> This part has not been
> pushed upstream yet. So for upstream tree, there is no need to fix our
> platform, but I am not sure whether other platforms are affected by
> this on the upstream tree.
> 

Ok, this makes sense and proves that my grep skills are not bad :) I don't think
there is any platform in upstream that has the 'phy-supply' in the pcie node.
But I do not want to ignore this property since it is pretty valid for existing
ethernet drivers to control the ethernet device attached via PCIe.

> > 
> > > I think other
> > >platforms should also have similar problems, which undoubtedly make
> > >these platforms be unstable. This patch has been applied, and I am not
> > >familiar with this. Can you fix this problem? I mean that those
> > >platforms that do not use pwrctrl can avoid skipping the scan.
> > 
> > Sure. It makes sense to add a check to see if the pwrctrl driver is enabled or not.
> > If it is not enabled, then the pwrctrl device creation could be skipped. I'll send a
> > patch once I'm infront of my computer.
> > 
> 
> I don't know whether check the pwrctrl driver is enabled is a good idea,
> for some devices it is more convenient to manage these regulators in
> their drivers, for some devices, we may want pwrctrl driver to manage
> the regulators. If both types of devices appear on the same platform,
> it is not enough to just check whether the pinctrl driver is enabled.
> 

Hmm. Now that I got the problem clearly, I think more elegant fix would be to
ignore the device nodes that has the 'phy-supply' property. I do not envision
device nodes to mix 'phy-supply' and other '-supply' properties though.

Can you please try this untested diff:

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 7a806f5c0d20..f3c43a91e71c 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -734,6 +734,10 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
  * Check if the power supply for the PCI device is present in the device tree
  * node or not.
  *
+ * NOTE: This API currently excludes the 'phy-supply' property as it is not a
+ * standard PCI supply, but rather the supply to the external PHY like in the
+ * case of ethernet devices.
+ *
  * Return: true if at least one power supply exists; false otherwise.
  */
 bool of_pci_supply_present(struct device_node *np)
@@ -746,7 +750,8 @@ bool of_pci_supply_present(struct device_node *np)
 
        for_each_property_of_node(np, prop) {
                supply = strrchr(prop->name, '-');
-               if (supply && !strcmp(supply, "-supply"))
+               if (supply && !strcmp(supply, "-supply") &&
+                   strcmp(prop, "phy-supply"))
                        return true;
        }

> > But in the long run, we would like to move all platforms to use pwrctrl instead of
> > fiddling the power supplies in the controller driver (well that was the motivation
> > to introduce it in the first place).
> > 
> 
> I understand this, but it should be compatible with the old method
> instead of completely making the old method inoperable unless it
> can be confirmed that all platforms in the upstream have completed
> the conversion to use pwrctrl driver. Obviously, this is difficult to
> confirm. :(
> 

The motive is to get rid of the supply handling from the controller drivers. But
if there are some exceptions like the ethernet drivers, we can exclude them.

> > Once you share the platform details, I'll try to migrate it to use pwrctrl. Also,
> > please note that we are going to enable pwrctrl in ARM64 defconfig very soon.
> > So I'll try to fix the affected platforms before that happens.
> 
> I think the current pwrctrl driver should still be in the early stage. If I
> understand correctly, it should only enable the regulators when probing
> and disable the regulators when removing. This does not meet all the use
> cases at present. So I'm not sure how you can do the fixes for all the affected
> platforms in pwrctrl driver for different use cases?
> 
> For example, some Ethernet devices need to support suspend/resume and
> Wake-on-LAN (WOL). If WOL is not enabled, the power of the external PHY
> needs to be turned off when it enters suspend state. If WOL is enabled, the
> power of the external PHY needs to be kept on. So for this case, I think you
> need to at least add PM interfaces in pwrctrl driver. For the other use cases,
> other solutions may be needed.
> 

Yes, PM support is something I have on my todo list and required for other
usecases too.

- Mani

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

RE: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Wei Fang]

> Subject: Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if
> pwrctrl device is created
> 
> On Fri, Mar 21, 2025 at 07:04:24AM +0000, Wei Fang wrote:
> > > Hi,
> > >
> > > On March 21, 2025 8:29:40 AM GMT+05:30, Wei Fang
> <wei.fang@nxp.com>
> > > wrote:
> > > >@@ -2487,7 +2487,14 @@ static struct pci_dev
> > > >*pci_scan_device(struct
> > > pci_bus *bus, int devfn)
> > > > 	struct pci_dev *dev;
> > > > 	u32 l;
> > > >
> > > >-	pci_pwrctrl_create_device(bus, devfn);
> > > >+	/*
> > > >+	 * Create pwrctrl device (if required) for the PCI device to handle the
> > > >+	 * power state. If the pwrctrl device is created, then skip scanning
> > > >+	 * further as the pwrctrl core will rescan the bus after powering on
> > > >+	 * the device.
> > > >+	 */
> > > >+	if (pci_pwrctrl_create_device(bus, devfn))
> > > >+		return NULL;
> > > >
> > > >Hi Manivannan,
> > > >
> > > >The current patch logic is that if the pcie device node is found to
> > > >have the "xxx-supply" property, the scan will be skipped, and then
> > > >the pwrctrl driver will rescan and enable the regulators. However,
> > > >after merging this patch, there is a problem on our platform. The
> > > >.probe() of our device driver will not be called. The reason is
> > > >that CONFIG_PCI_PWRCTL_SLOT is not enabled at all in our
> > > >configuration file, and the compatible string of the device is also not
> added to the pwrctrl driver.
> > >
> > > Hmm. So I guess the controller driver itself is enabling the
> > > supplies I believe (which I failed to spot). May I know what platforms are
> affected?
> >
> > Yes, the affected device is an Ethernet controller on our i.MX95
> > platform, it has a "phy-supply" property to control the power of the
> > external Ethernet PHY chip in the device driver.
> 
> Ah, I was not aware of any devices using 'phy-supply' in the pcie device node.

It is not a standard property defined in ethernet-controller.yaml. Maybe
for other vendors, it’s called "vdd-supply" or something else.

> 
> > This part has not been
> > pushed upstream yet. So for upstream tree, there is no need to fix our
> > platform, but I am not sure whether other platforms are affected by
> > this on the upstream tree.
> >
> 
> Ok, this makes sense and proves that my grep skills are not bad :) I don't think
> there is any platform in upstream that has the 'phy-supply' in the pcie node.
> But I do not want to ignore this property since it is pretty valid for existing
> ethernet drivers to control the ethernet device attached via PCIe.
> 
> > >
> > > > I think other
> > > >platforms should also have similar problems, which undoubtedly make
> > > >these platforms be unstable. This patch has been applied, and I am
> > > >not familiar with this. Can you fix this problem? I mean that those
> > > >platforms that do not use pwrctrl can avoid skipping the scan.
> > >
> > > Sure. It makes sense to add a check to see if the pwrctrl driver is enabled or
> not.
> > > If it is not enabled, then the pwrctrl device creation could be
> > > skipped. I'll send a patch once I'm infront of my computer.
> > >
> >
> > I don't know whether check the pwrctrl driver is enabled is a good
> > idea, for some devices it is more convenient to manage these
> > regulators in their drivers, for some devices, we may want pwrctrl
> > driver to manage the regulators. If both types of devices appear on
> > the same platform, it is not enough to just check whether the pinctrl driver is
> enabled.
> >
> 
> Hmm. Now that I got the problem clearly, I think more elegant fix would be to
> ignore the device nodes that has the 'phy-supply' property. I do not envision
> device nodes to mix 'phy-supply' and other '-supply' properties though.
> 

I think the below solution is not generic, "phy-supply" is just an example,
the following modification is only for this case. In fact, there is also a
"serdes-supply" on our platform, of course, this is not included in the
upstream, because we haven't had time to complete these. So for the
"serdes-supply" case, the below solution won't take effect.

In addition, for some existing devices, the pwrctrl driver can indeed
meet their needs for regulator management, but their compatible
strings have not been added to pwrctrl, so they are currently
unavailable. The below solution also not resolves this issue. For these
devices, I think it's necessary to keep the previous approach (regulators
are managed by the device driver) until the maintainers of these devices
switch to using pwrctrl.

A generic solution I think of is to add a static compatible string table
to core.c (pwrctrl) to indicate which devices currently use pwrctrl. If
the compatible string of the current device matches the table, then
skip the scan. Or add an property to the node to indicate the use of
pwrctl, but this may be opposed by DT maintainers because this
property is not used to describe hardware.

> Can you please try this untested diff:
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c index
> 7a806f5c0d20..f3c43a91e71c 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -734,6 +734,10 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
>   * Check if the power supply for the PCI device is present in the device tree
>   * node or not.
>   *
> + * NOTE: This API currently excludes the 'phy-supply' property as it is
> + not a
> + * standard PCI supply, but rather the supply to the external PHY like
> + in the
> + * case of ethernet devices.
> + *
>   * Return: true if at least one power supply exists; false otherwise.
>   */
>  bool of_pci_supply_present(struct device_node *np) @@ -746,7 +750,8 @@
> bool of_pci_supply_present(struct device_node *np)
> 
>         for_each_property_of_node(np, prop) {
>                 supply = strrchr(prop->name, '-');
> -               if (supply && !strcmp(supply, "-supply"))
> +               if (supply && !strcmp(supply, "-supply") &&
> +                   strcmp(prop, "phy-supply"))
>                         return true;
>         }
> 
> > > But in the long run, we would like to move all platforms to use
> > > pwrctrl instead of fiddling the power supplies in the controller
> > > driver (well that was the motivation to introduce it in the first place).
> > >
> >
> > I understand this, but it should be compatible with the old method
> > instead of completely making the old method inoperable unless it can
> > be confirmed that all platforms in the upstream have completed the
> > conversion to use pwrctrl driver. Obviously, this is difficult to
> > confirm. :(
> >
> 
> The motive is to get rid of the supply handling from the controller drivers. But
> if there are some exceptions like the ethernet drivers, we can exclude them.
> 

I agree with you, some exceptions are still handled by device drivers,
and some exceptions may need to add a corresponding pwrctrl driver
to the pwrctl subsystem.

> > > Once you share the platform details, I'll try to migrate it to use
> > > pwrctrl. Also, please note that we are going to enable pwrctrl in ARM64
> defconfig very soon.
> > > So I'll try to fix the affected platforms before that happens.
> >
> > I think the current pwrctrl driver should still be in the early stage.
> > If I understand correctly, it should only enable the regulators when
> > probing and disable the regulators when removing. This does not meet
> > all the use cases at present. So I'm not sure how you can do the fixes
> > for all the affected platforms in pwrctrl driver for different use cases?
> >
> > For example, some Ethernet devices need to support suspend/resume and
> > Wake-on-LAN (WOL). If WOL is not enabled, the power of the external
> > PHY needs to be turned off when it enters suspend state. If WOL is
> > enabled, the power of the external PHY needs to be kept on. So for
> > this case, I think you need to at least add PM interfaces in pwrctrl
> > driver. For the other use cases, other solutions may be needed.
> >
> 
> Yes, PM support is something I have on my todo list and required for other
> usecases too.
> 
> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Sat, Mar 22, 2025 at 11:39:06AM +0000, Wei Fang wrote:
> > Subject: Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if
> > pwrctrl device is created
> > 
> > On Fri, Mar 21, 2025 at 07:04:24AM +0000, Wei Fang wrote:
> > > > Hi,
> > > >
> > > > On March 21, 2025 8:29:40 AM GMT+05:30, Wei Fang
> > <wei.fang@nxp.com>
> > > > wrote:
> > > > >@@ -2487,7 +2487,14 @@ static struct pci_dev
> > > > >*pci_scan_device(struct
> > > > pci_bus *bus, int devfn)
> > > > > 	struct pci_dev *dev;
> > > > > 	u32 l;
> > > > >
> > > > >-	pci_pwrctrl_create_device(bus, devfn);
> > > > >+	/*
> > > > >+	 * Create pwrctrl device (if required) for the PCI device to handle the
> > > > >+	 * power state. If the pwrctrl device is created, then skip scanning
> > > > >+	 * further as the pwrctrl core will rescan the bus after powering on
> > > > >+	 * the device.
> > > > >+	 */
> > > > >+	if (pci_pwrctrl_create_device(bus, devfn))
> > > > >+		return NULL;
> > > > >
> > > > >Hi Manivannan,
> > > > >
> > > > >The current patch logic is that if the pcie device node is found to
> > > > >have the "xxx-supply" property, the scan will be skipped, and then
> > > > >the pwrctrl driver will rescan and enable the regulators. However,
> > > > >after merging this patch, there is a problem on our platform. The
> > > > >.probe() of our device driver will not be called. The reason is
> > > > >that CONFIG_PCI_PWRCTL_SLOT is not enabled at all in our
> > > > >configuration file, and the compatible string of the device is also not
> > added to the pwrctrl driver.
> > > >
> > > > Hmm. So I guess the controller driver itself is enabling the
> > > > supplies I believe (which I failed to spot). May I know what platforms are
> > affected?
> > >
> > > Yes, the affected device is an Ethernet controller on our i.MX95
> > > platform, it has a "phy-supply" property to control the power of the
> > > external Ethernet PHY chip in the device driver.
> > 
> > Ah, I was not aware of any devices using 'phy-supply' in the pcie device node.
> 
> It is not a standard property defined in ethernet-controller.yaml. Maybe
> for other vendors, it’s called "vdd-supply" or something else.
> 

Ah, then why is it used at all in the first place (if not defined in the
binding)? This makes me wonder if I really have to fix anything since everything
we are talking about are out of tree.

> > 
> > > This part has not been
> > > pushed upstream yet. So for upstream tree, there is no need to fix our
> > > platform, but I am not sure whether other platforms are affected by
> > > this on the upstream tree.
> > >
> > 
> > Ok, this makes sense and proves that my grep skills are not bad :) I don't think
> > there is any platform in upstream that has the 'phy-supply' in the pcie node.
> > But I do not want to ignore this property since it is pretty valid for existing
> > ethernet drivers to control the ethernet device attached via PCIe.
> > 
> > > >
> > > > > I think other
> > > > >platforms should also have similar problems, which undoubtedly make
> > > > >these platforms be unstable. This patch has been applied, and I am
> > > > >not familiar with this. Can you fix this problem? I mean that those
> > > > >platforms that do not use pwrctrl can avoid skipping the scan.
> > > >
> > > > Sure. It makes sense to add a check to see if the pwrctrl driver is enabled or
> > not.
> > > > If it is not enabled, then the pwrctrl device creation could be
> > > > skipped. I'll send a patch once I'm infront of my computer.
> > > >
> > >
> > > I don't know whether check the pwrctrl driver is enabled is a good
> > > idea, for some devices it is more convenient to manage these
> > > regulators in their drivers, for some devices, we may want pwrctrl
> > > driver to manage the regulators. If both types of devices appear on
> > > the same platform, it is not enough to just check whether the pinctrl driver is
> > enabled.
> > >
> > 
> > Hmm. Now that I got the problem clearly, I think more elegant fix would be to
> > ignore the device nodes that has the 'phy-supply' property. I do not envision
> > device nodes to mix 'phy-supply' and other '-supply' properties though.
> > 
> 
> I think the below solution is not generic, "phy-supply" is just an example,
> the following modification is only for this case. In fact, there is also a
> "serdes-supply" on our platform, of course, this is not included in the
> upstream, because we haven't had time to complete these. So for the
> "serdes-supply" case, the below solution won't take effect.
> 

Does your platform have a serdes connected to the PCIe port? I doubt so. Again,
these are all non-standard properties, not available in upstream. So I'm not
going to worry about them.

> In addition, for some existing devices, the pwrctrl driver can indeed
> meet their needs for regulator management, but their compatible
> strings have not been added to pwrctrl, so they are currently
> unavailable. The below solution also not resolves this issue. For these
> devices, I think it's necessary to keep the previous approach (regulators
> are managed by the device driver) until the maintainers of these devices
> switch to using pwrctrl.
> 
> A generic solution I think of is to add a static compatible string table
> to core.c (pwrctrl) to indicate which devices currently use pwrctrl. If
> the compatible string of the current device matches the table, then
> skip the scan. Or add an property to the node to indicate the use of
> pwrctl, but this may be opposed by DT maintainers because this
> property is not used to describe hardware.
> 

Pwrctrl at the moment supports only the PCIe bus. And also, checking for the
compatible property in the pwtctrl core doesn't work/scale as the kernel has no
idea whether the pwtctrl driver is going to be available or not. That's the
reason why we ended up checking for the -supply property.

But I want to clarify that the intention of the pwrctrl framework is to control
the power to the device itself. Those supplies cannot be controlled by the
device driver themselves as the device first need to be available on the bus to
the driver to get loaded (if the device was powered on by the bootloader it is
not true, but kernel should not depend on the bootloader here). Traditionally,
such device power supplies were controlled by the PCIe controller drivers as of
now. This caused issues on multiple platforms/devices and that resulted in the
pwrctrl framework.

However, as I said earlier, pwrctrl can ignore several supplies like the
'phy-supply' that controls the supply to a sub-IP inside the PCIe device and
that could well be controlled by the device driver itself.

So I do think that the below patch is a valid one (once such devices start
appearing in the mainline). However, I'm not doing to post it for now as there
is nothing to fix in mainline afaik.

- Mani

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

RE: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Wei Fang]

> > > > > >The current patch logic is that if the pcie device node is found to
> > > > > >have the "xxx-supply" property, the scan will be skipped, and then
> > > > > >the pwrctrl driver will rescan and enable the regulators. However,
> > > > > >after merging this patch, there is a problem on our platform. The
> > > > > >.probe() of our device driver will not be called. The reason is
> > > > > >that CONFIG_PCI_PWRCTL_SLOT is not enabled at all in our
> > > > > >configuration file, and the compatible string of the device is also not
> > > added to the pwrctrl driver.
> > > > >
> > > > > Hmm. So I guess the controller driver itself is enabling the
> > > > > supplies I believe (which I failed to spot). May I know what platforms are
> > > affected?
> > > >
> > > > Yes, the affected device is an Ethernet controller on our i.MX95
> > > > platform, it has a "phy-supply" property to control the power of the
> > > > external Ethernet PHY chip in the device driver.
> > >
> > > Ah, I was not aware of any devices using 'phy-supply' in the pcie device node.
> >
> > It is not a standard property defined in ethernet-controller.yaml. Maybe
> > for other vendors, it’s called "vdd-supply" or something else.
> >
> 
> Ah, then why is it used at all in the first place (if not defined in the
> binding)? This makes me wonder if I really have to fix anything since everything
> we are talking about are out of tree.

"phy-supply" is a vendor defined property, we have added it to fsl,fec.yaml,
but fec is not a PCIe device. And this property is also added to other Ethernet
devices such as allwinner,sun4i-a10-mdio.yaml and rockchip,emac.yaml, etc.
But they are all not a PCIe device. So there is no need to fix it in upstream.

> 
> > >
> > > > This part has not been
> > > > pushed upstream yet. So for upstream tree, there is no need to fix our
> > > > platform, but I am not sure whether other platforms are affected by
> > > > this on the upstream tree.
> > > >
> > >
> > > Ok, this makes sense and proves that my grep skills are not bad :) I don't
> think
> > > there is any platform in upstream that has the 'phy-supply' in the pcie node.
> > > But I do not want to ignore this property since it is pretty valid for existing
> > > ethernet drivers to control the ethernet device attached via PCIe.
> > >
> > > > >
> > > > > > I think other
> > > > > >platforms should also have similar problems, which undoubtedly make
> > > > > >these platforms be unstable. This patch has been applied, and I am
> > > > > >not familiar with this. Can you fix this problem? I mean that those
> > > > > >platforms that do not use pwrctrl can avoid skipping the scan.
> > > > >
> > > > > Sure. It makes sense to add a check to see if the pwrctrl driver is enabled
> or
> > > not.
> > > > > If it is not enabled, then the pwrctrl device creation could be
> > > > > skipped. I'll send a patch once I'm infront of my computer.
> > > > >
> > > >
> > > > I don't know whether check the pwrctrl driver is enabled is a good
> > > > idea, for some devices it is more convenient to manage these
> > > > regulators in their drivers, for some devices, we may want pwrctrl
> > > > driver to manage the regulators. If both types of devices appear on
> > > > the same platform, it is not enough to just check whether the pinctrl driver
> is
> > > enabled.
> > > >
> > >
> > > Hmm. Now that I got the problem clearly, I think more elegant fix would be
> to
> > > ignore the device nodes that has the 'phy-supply' property. I do not envision
> > > device nodes to mix 'phy-supply' and other '-supply' properties though.
> > >
> >
> > I think the below solution is not generic, "phy-supply" is just an example,
> > the following modification is only for this case. In fact, there is also a
> > "serdes-supply" on our platform, of course, this is not included in the
> > upstream, because we haven't had time to complete these. So for the
> > "serdes-supply" case, the below solution won't take effect.
> >
> 
> Does your platform have a serdes connected to the PCIe port? I doubt so. Again,
> these are all non-standard properties, not available in upstream. So I'm not
> going to worry about them.

No, the serdes is inside the Ethernet MAC. I was wondering how to bypass
pwrctrl in the future if we add such a "xxx-supply" to a PCIe device node,
so that our drivers can be smoothly accepted by upstream.


> 
> > In addition, for some existing devices, the pwrctrl driver can indeed
> > meet their needs for regulator management, but their compatible
> > strings have not been added to pwrctrl, so they are currently
> > unavailable. The below solution also not resolves this issue. For these
> > devices, I think it's necessary to keep the previous approach (regulators
> > are managed by the device driver) until the maintainers of these devices
> > switch to using pwrctrl.
> >
> > A generic solution I think of is to add a static compatible string table
> > to core.c (pwrctrl) to indicate which devices currently use pwrctrl. If
> > the compatible string of the current device matches the table, then
> > skip the scan. Or add an property to the node to indicate the use of
> > pwrctl, but this may be opposed by DT maintainers because this
> > property is not used to describe hardware.
> >
> 
> Pwrctrl at the moment supports only the PCIe bus. And also, checking for the
> compatible property in the pwtctrl core doesn't work/scale as the kernel has no
> idea whether the pwtctrl driver is going to be available or not. That's the
> reason why we ended up checking for the -supply property.
> 
> But I want to clarify that the intention of the pwrctrl framework is to control
> the power to the device itself. Those supplies cannot be controlled by the
> device driver themselves as the device first need to be available on the bus to
> the driver to get loaded (if the device was powered on by the bootloader it is
> not true, but kernel should not depend on the bootloader here). Traditionally,
> such device power supplies were controlled by the PCIe controller drivers as of
> now. This caused issues on multiple platforms/devices and that resulted in the
> pwrctrl framework.
> 
> However, as I said earlier, pwrctrl can ignore several supplies like the
> 'phy-supply' that controls the supply to a sub-IP inside the PCIe device and
> that could well be controlled by the device driver itself.
> 
> So I do think that the below patch is a valid one (once such devices start
> appearing in the mainline). However, I'm not doing to post it for now as there
> is nothing to fix in mainline afaik.
> 

Yeah, no fix is needed at present. Thanks for your clarification of the
pwrctl framework.

Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Thu, Mar 27, 2025 at 06:02:27AM +0000, Wei Fang wrote:
> > > > > > >The current patch logic is that if the pcie device node is found to
> > > > > > >have the "xxx-supply" property, the scan will be skipped, and then
> > > > > > >the pwrctrl driver will rescan and enable the regulators. However,
> > > > > > >after merging this patch, there is a problem on our platform. The
> > > > > > >.probe() of our device driver will not be called. The reason is
> > > > > > >that CONFIG_PCI_PWRCTL_SLOT is not enabled at all in our
> > > > > > >configuration file, and the compatible string of the device is also not
> > > > added to the pwrctrl driver.
> > > > > >
> > > > > > Hmm. So I guess the controller driver itself is enabling the
> > > > > > supplies I believe (which I failed to spot). May I know what platforms are
> > > > affected?
> > > > >
> > > > > Yes, the affected device is an Ethernet controller on our i.MX95
> > > > > platform, it has a "phy-supply" property to control the power of the
> > > > > external Ethernet PHY chip in the device driver.
> > > >
> > > > Ah, I was not aware of any devices using 'phy-supply' in the pcie device node.
> > >
> > > It is not a standard property defined in ethernet-controller.yaml. Maybe
> > > for other vendors, it’s called "vdd-supply" or something else.
> > >
> > 
> > Ah, then why is it used at all in the first place (if not defined in the
> > binding)? This makes me wonder if I really have to fix anything since everything
> > we are talking about are out of tree.
> 
> "phy-supply" is a vendor defined property, we have added it to fsl,fec.yaml,
> but fec is not a PCIe device. And this property is also added to other Ethernet
> devices such as allwinner,sun4i-a10-mdio.yaml and rockchip,emac.yaml, etc.
> But they are all not a PCIe device. So there is no need to fix it in upstream.
> 
> > 
> > > >
> > > > > This part has not been
> > > > > pushed upstream yet. So for upstream tree, there is no need to fix our
> > > > > platform, but I am not sure whether other platforms are affected by
> > > > > this on the upstream tree.
> > > > >
> > > >
> > > > Ok, this makes sense and proves that my grep skills are not bad :) I don't
> > think
> > > > there is any platform in upstream that has the 'phy-supply' in the pcie node.
> > > > But I do not want to ignore this property since it is pretty valid for existing
> > > > ethernet drivers to control the ethernet device attached via PCIe.
> > > >
> > > > > >
> > > > > > > I think other
> > > > > > >platforms should also have similar problems, which undoubtedly make
> > > > > > >these platforms be unstable. This patch has been applied, and I am
> > > > > > >not familiar with this. Can you fix this problem? I mean that those
> > > > > > >platforms that do not use pwrctrl can avoid skipping the scan.
> > > > > >
> > > > > > Sure. It makes sense to add a check to see if the pwrctrl driver is enabled
> > or
> > > > not.
> > > > > > If it is not enabled, then the pwrctrl device creation could be
> > > > > > skipped. I'll send a patch once I'm infront of my computer.
> > > > > >
> > > > >
> > > > > I don't know whether check the pwrctrl driver is enabled is a good
> > > > > idea, for some devices it is more convenient to manage these
> > > > > regulators in their drivers, for some devices, we may want pwrctrl
> > > > > driver to manage the regulators. If both types of devices appear on
> > > > > the same platform, it is not enough to just check whether the pinctrl driver
> > is
> > > > enabled.
> > > > >
> > > >
> > > > Hmm. Now that I got the problem clearly, I think more elegant fix would be
> > to
> > > > ignore the device nodes that has the 'phy-supply' property. I do not envision
> > > > device nodes to mix 'phy-supply' and other '-supply' properties though.
> > > >
> > >
> > > I think the below solution is not generic, "phy-supply" is just an example,
> > > the following modification is only for this case. In fact, there is also a
> > > "serdes-supply" on our platform, of course, this is not included in the
> > > upstream, because we haven't had time to complete these. So for the
> > > "serdes-supply" case, the below solution won't take effect.
> > >
> > 
> > Does your platform have a serdes connected to the PCIe port? I doubt so. Again,
> > these are all non-standard properties, not available in upstream. So I'm not
> > going to worry about them.
> 
> No, the serdes is inside the Ethernet MAC. I was wondering how to bypass
> pwrctrl in the future if we add such a "xxx-supply" to a PCIe device node,
> so that our drivers can be smoothly accepted by upstream.
> 

In that case, the logic should be changed to only look for bindings defined
supplies for the endpoint devices. Like, "v*pcie*-supply" properties instead of
all supplies.

- Mani

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