> Hi, > > On March 21, 2025 8:29:40 AM GMT+05:30, Wei Fang <wei.fang@xxxxxxx> > 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.
