RE: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Richard Zhu <hongxing.zhu@nxp.com>]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Tuesday, October 26, 2021 6:58 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Francesco Dolcini <francesco.dolcini@toradex.com>;
> l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; jingoohan1@gmail.com;
> linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link
> never came up
> 
> On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:
> 
> > > I should probably also say that the check for the regulator looks
> > > buggy as well, regulators should only be optional if they can be
> > > physically absent which does not seem likely for PCI devices.  If
> > > the driver is not doing something to reconfigure the hardware to
> > > account for a missing supply this is generally a big warning sign.
> > >
> > > I really don't understand why regulator support is so frequently
> > > problematic for PCI controllers.  :(
> 
> > [Richard Zhu] Hi Mark:
> > The _enabled check is used because that this regulator is optional in the
> HW design.
> > To make the codes clean and aligned on different HW boards, the
> _enabled check is added.
> 
> I would be really surprised to see PCI hardware that was able to support a
> supply being physically absent, and this use of _is_enabled() is quite
> simply not how any of this is supposed to work in the regulator API even
> for regulators that can be optional.
[Richard Zhu] Actually, this regulator is one GPIO fixed regulator.
Controlled by SW to turn on (GPIO high) or turn off (GPIO low) the supply.
In some boards designs, this supply might be always on(GPIO high).
So, in point of SW driver view, this regulator is optional.

> 
> > The root cause is that the error return is not handled properly by the
> controller driver.
> > I.MX PCIe controller doesn't support the Hot-Plug, and it would return
> > -110 error when PCIe link never came up. Thus, the _probe would be
> failed in the end.
> > The clocks/regulator usage balance should be considered by i.MX PCIe
> controller, that's all.
> > It's not a general case, and the problem is not caused by the regulator
> support.
> 
> Perhaps it's not causing problems in this design but if the supply is ever
> shared with anything else then the software will run into trouble.
> There will also be problems with the error handling on a system where
> the regulator needs to be controlled.
[Richard Zhu] This GPIO fixed regulator is only used by controller driver.
It makes sense to disable the enabled regulator when driver probe is failed.

> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages
> much easier to read and reply to.
[Richard] Sorry about that.
BR
Richard