> 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@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.
It is not a standard property defined in ethernet-controller.yaml. Maybe
for other vendors, it’s called "vdd-supply" or something else.
>
> > 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.
>
I think the below solution is not generic, "phy-supply" is just an example,
the following modification is only for this case. In fact, there is also a
"serdes-supply" on our platform, of course, this is not included in the
upstream, because we haven't had time to complete these. So for the
"serdes-supply" case, the below solution won't take effect.
In addition, for some existing devices, the pwrctrl driver can indeed
meet their needs for regulator management, but their compatible
strings have not been added to pwrctrl, so they are currently
unavailable. The below solution also not resolves this issue. For these
devices, I think it's necessary to keep the previous approach (regulators
are managed by the device driver) until the maintainers of these devices
switch to using pwrctrl.
A generic solution I think of is to add a static compatible string table
to core.c (pwrctrl) to indicate which devices currently use pwrctrl. If
the compatible string of the current device matches the table, then
skip the scan. Or add an property to the node to indicate the use of
pwrctl, but this may be opposed by DT maintainers because this
property is not used to describe hardware.
> 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.
>
I agree with you, some exceptions are still handled by device drivers,
and some exceptions may need to add a corresponding pwrctrl driver
to the pwrctl subsystem.
> > > 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
>
> --
> மணிவண்ணன் சதாசிவம்
