RE: [PATCH v3 3/5] PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Wei Fang]

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

Yeah, no fix is needed at present. Thanks for your clarification of the
pwrctl framework.