Message-ID: <yzhvxyjb75epv4mkkocjqsqkus44c55zwnxta6ac3aauvswv3x@knyhszmrgfc3>
Date: Mon, 24 Mar 2025 13:45:18 +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>, 
	Frank Li <frank.li@....com>
Subject: Re: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further
 if pwrctrl device is created

On Sat, Mar 22, 2025 at 11:39:06AM +0000, Wei Fang wrote:
> > 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.
> 
> It is not a standard property defined in ethernet-controller.yaml. Maybe
> for other vendors, it’s called "vdd-supply" or something else.
> 

Ah, then why is it used at all in the first place (if not defined in the
binding)? This makes me wonder if I really have to fix anything since everything
we are talking about are out of tree.

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

Does your platform have a serdes connected to the PCIe port? I doubt so. Again,
these are all non-standard properties, not available in upstream. So I'm not
going to worry about them.

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

Pwrctrl at the moment supports only the PCIe bus. And also, checking for the
compatible property in the pwtctrl core doesn't work/scale as the kernel has no
idea whether the pwtctrl driver is going to be available or not. That's the
reason why we ended up checking for the -supply property.

But I want to clarify that the intention of the pwrctrl framework is to control
the power to the device itself. Those supplies cannot be controlled by the
device driver themselves as the device first need to be available on the bus to
the driver to get loaded (if the device was powered on by the bootloader it is
not true, but kernel should not depend on the bootloader here). Traditionally,
such device power supplies were controlled by the PCIe controller drivers as of
now. This caused issues on multiple platforms/devices and that resulted in the
pwrctrl framework.

However, as I said earlier, pwrctrl can ignore several supplies like the
'phy-supply' that controls the supply to a sub-IP inside the PCIe device and
that could well be controlled by the device driver itself.

So I do think that the below patch is a valid one (once such devices start
appearing in the mainline). However, I'm not doing to post it for now as there
is nothing to fix in mainline afaik.

- Mani

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

Powered by blists - more mailing lists