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@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.
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
--
மணிவண்ணன் சதாசிவம்
