Message-ID: <20250322032344.uypqhi3kg6nqixay@thinkpad>
Date: Sat, 22 Mar 2025 08:53:44 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Wei Fang <wei.fang@....com>
Cc: "devnull+manivannan.sadhasivam.linaro.org@...nel.org" <devnull+manivannan.sadhasivam.linaro.org@...nel.org>,
	"bartosz.golaszewski@...aro.org" <bartosz.golaszewski@...aro.org>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	"brgl@...ev.pl" <brgl@...ev.pl>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"robh@...nel.org" <robh@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
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@....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

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

Powered by blists - more mailing lists