Message-ID: <n74zsz7ae5eks5qccmqgsz72onuecp2cbyrmuod66f472isaxj@vhxq36t7b7uy>
Date: Wed, 2 Apr 2025 13:44:14 +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 Thu, Mar 27, 2025 at 06:02:27AM +0000, Wei Fang wrote:
> > > > > > >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.
> 
> "phy-supply" is a vendor defined property, we have added it to fsl,fec.yaml,
> but fec is not a PCIe device. And this property is also added to other Ethernet
> devices such as allwinner,sun4i-a10-mdio.yaml and rockchip,emac.yaml, etc.
> But they are all not a PCIe device. So there is no need to fix it in upstream.
> 
> > 
> > > >
> > > > > 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.
> 
> No, the serdes is inside the Ethernet MAC. I was wondering how to bypass
> pwrctrl in the future if we add such a "xxx-supply" to a PCIe device node,
> so that our drivers can be smoothly accepted by upstream.
> 

In that case, the logic should be changed to only look for bindings defined
supplies for the endpoint devices. Like, "v*pcie*-supply" properties instead of
all supplies.

- Mani

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

Powered by blists - more mailing lists