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

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

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

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

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

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

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

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

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

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