diff mbox series

[1/4] PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

Message ID 20241210-pci-pwrctrl-slot-v1-1-eae45e488040@linaro.org (mailing list archive)
State New
Headers show
Series PCI/pwrctrl: Rework pwrctrl driver integration and add driver for PCI slot | expand

Commit Message

Manivannan Sadhasivam via B4 Relay Dec. 10, 2024, 9:55 a.m. UTC 
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>
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(-)

Comments

Lukas Wunner Dec. 15, 2024, 5:19 p.m. UTC | #1 
On Tue, Dec 10, 2024 at 03:25:24PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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 devices (if required) for the PCI devices to handle the power
> + * state.
> + */
> +static void pci_pwrctrl_create_devices(struct pci_bus *bus, int devfn)

Nit:  AFAICS this only creates a *single* platform device, so the
"devices" (plural) in the function name and in the code comment
doesn't seem to be accurate anymore.


> 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();
>  }

Remind me, what's the purpose of this?  I'm guessing that it
recursively creates the platform devices below the newly
powered up device, is that correct?  If so, is it still
necessary?  Doesn't the new approach automatically create
those devices upon their enumeration?

Overall it looks like a significant improvement, thanks for doing this!

Lukas
Manivannan Sadhasivam Dec. 16, 2024, 5:15 a.m. UTC | #2 
On Sun, Dec 15, 2024 at 06:19:22PM +0100, Lukas Wunner wrote:
> On Tue, Dec 10, 2024 at 03:25:24PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -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 devices (if required) for the PCI devices to handle the power
> > + * state.
> > + */
> > +static void pci_pwrctrl_create_devices(struct pci_bus *bus, int devfn)
> 
> Nit:  AFAICS this only creates a *single* platform device, so the
> "devices" (plural) in the function name and in the code comment
> doesn't seem to be accurate anymore.
> 

I missed it, thanks for pointing out.

> 
> > 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();
> >  }
> 
> Remind me, what's the purpose of this?  I'm guessing that it
> recursively creates the platform devices below the newly
> powered up device, is that correct?  If so, is it still
> necessary?  Doesn't the new approach automatically create
> those devices upon their enumeration?
> 

If the pwrctrl driver is available at the time of platform device creation, this
is not needed. But if the driver is not available at that time and probed
later, then we need to rescan the bus to enumerate the devices. This is pretty
much needed for platforms lacking hotplug support (most of the DT ones).

> Overall it looks like a significant improvement, thanks for doing this!
> 

Thanks a lot for your inputs too!

- Mani
Lukas Wunner Dec. 17, 2024, 1:14 p.m. UTC | #3 
On Mon, Dec 16, 2024 at 10:45:21AM +0530, Manivannan Sadhasivam wrote:
> On Sun, Dec 15, 2024 at 06:19:22PM +0100, Lukas Wunner wrote:
> > On Tue, Dec 10, 2024 at 03:25:24PM +0530, Manivannan Sadhasivam wrote:
> > > 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();
> > >  }
> > 
> > Remind me, what's the purpose of this?  I'm guessing that it
> > recursively creates the platform devices below the newly
> > powered up device, is that correct?  If so, is it still
> > necessary?  Doesn't the new approach automatically create
> > those devices upon their enumeration?
> 
> If the pwrctrl driver is available at the time of platform device creation,
> this is not needed. But if the driver is not available at that time and
> probed later, then we need to rescan the bus to enumerate the devices.

I see.  Sounds like this can be made conditional on the caller
being a module.  I think you could achieve this with the following
in pci_pwrctl_device_set_ready():

-	schedule_work(&pwrctl->work);
+	if (is_module_address(_RET_IP_))
+		schedule_work(&pwrctl->work);

Though you'd also have to declare pci_pwrctl_device_set_ready()
"__attribute__((always_inline))" so that it gets inlined into
devm_pci_pwrctl_device_set_ready() and the condition works there
as well.

I'm wondering whether the bus notifier is still necessary with
the new scheme.  Since the power control device is instantiated
and destroyed in unison with the pci_dev, can't the device link
always be created on instantiation of the power control device?

Thanks,

Lukas
Manivannan Sadhasivam Dec. 17, 2024, 2:50 p.m. UTC | #4 
On Tue, Dec 17, 2024 at 02:14:34PM +0100, Lukas Wunner wrote:
> On Mon, Dec 16, 2024 at 10:45:21AM +0530, Manivannan Sadhasivam wrote:
> > On Sun, Dec 15, 2024 at 06:19:22PM +0100, Lukas Wunner wrote:
> > > On Tue, Dec 10, 2024 at 03:25:24PM +0530, Manivannan Sadhasivam wrote:
> > > > 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();
> > > >  }
> > > 
> > > Remind me, what's the purpose of this?  I'm guessing that it
> > > recursively creates the platform devices below the newly
> > > powered up device, is that correct?  If so, is it still
> > > necessary?  Doesn't the new approach automatically create
> > > those devices upon their enumeration?
> > 
> > If the pwrctrl driver is available at the time of platform device creation,
> > this is not needed. But if the driver is not available at that time and
> > probed later, then we need to rescan the bus to enumerate the devices.
> 
> I see.  Sounds like this can be made conditional on the caller
> being a module.  I think you could achieve this with the following
> in pci_pwrctl_device_set_ready():
> 
> -	schedule_work(&pwrctl->work);
> +	if (is_module_address(_RET_IP_))
> +		schedule_work(&pwrctl->work);
> 
> Though you'd also have to declare pci_pwrctl_device_set_ready()
> "__attribute__((always_inline))" so that it gets inlined into
> devm_pci_pwrctl_device_set_ready() and the condition works there
> as well.
> 

I'd prefer to skip the rescan if the pwrctrl device is created and let the
pci_pwrctrl_device_set_ready() initiate rescan once the device is powered on.
This way, we could avoid scanning for the device twice.

> I'm wondering whether the bus notifier is still necessary with
> the new scheme.  Since the power control device is instantiated
> and destroyed in unison with the pci_dev, can't the device link
> always be created on instantiation of the power control device?
> 

I did move the devlink handling out of bus notifier callback with commit,
b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI
client drivers").

The bus notifier is only used to set 'of_node_reused' flag to indicate that the
corresponding DT node is already used.

- Mani
diff mbox series

Patch

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..db928d5cb753 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 devices (if required) for the PCI devices to handle the power
+ * state.
+ */
+static void pci_pwrctrl_create_devices(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 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_devices(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();
 }