PCI: dwc: Provide deinit callback for i.MX

The i.MX integration for the DesignWare PCI controller has a _host_exit()
operation which undoes everything that the _host_init() operation does but
does not wire this up as the host_deinit callback for the core, or call it
in any path other than suspend. This means that if we ever unwind the
initial probe of the device, for example because it fails, the regulator
core complains that the regulators for the device were left enabled:

imx6q-pcie 33800000.pcie: iATU: unroll T, 4 ob, 4 ib, align 64K, limit 16G
imx6q-pcie 33800000.pcie: Phy link never came up
imx6q-pcie 33800000.pcie: Phy link never came up
imx6q-pcie: probe of 33800000.pcie failed with error -110
------------[ cut here ]------------
WARNING: CPU: 2 PID: 46 at drivers/regulator/core.c:2396 _regulator_put+0x110/0x128

Wire up the callback so that the core can clean up after itself.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/pci/controller/dwc/pci-imx6.c | 1 +
 1 file changed, 1 insertion(+)


---
base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1
change-id: 20230703-pci-imx-regulator-cleanup-a17c8fd15ec5

Best regards,

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 2023年7月4日 0:23
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas Stach
> <l.stach@pengutronix.de>; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof
> Wilczyński <kw@linux.com>; Rob Herring <robh@kernel.org>; Bjorn Helgaas
> <bhelgaas@google.com>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> <s.hauer@pengutronix.de>; Pengutronix Kernel Team <kernel@pengutronix.de>;
> Fabio Estevam <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>
> Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Mark Brown <broonie@kernel.org>
> Subject: [PATCH] PCI: dwc: Provide deinit callback for i.MX
> 
> The i.MX integration for the DesignWare PCI controller has a _host_exit()
> operation which undoes everything that the _host_init() operation does but does
> not wire this up as the host_deinit callback for the core, or call it in any path other
> than suspend. This means that if we ever unwind the initial probe of the device,
> for example because it fails, the regulator core complains that the regulators for
> the device were left enabled:
> 
> imx6q-pcie 33800000.pcie: iATU: unroll T, 4 ob, 4 ib, align 64K, limit 16G
> imx6q-pcie 33800000.pcie: Phy link never came up imx6q-pcie 33800000.pcie:
> Phy link never came up
> imx6q-pcie: probe of 33800000.pcie failed with error -110 ------------[ cut
> here ]------------
> WARNING: CPU: 2 PID: 46 at drivers/regulator/core.c:2396
> _regulator_put+0x110/0x128
> 
> Wire up the callback so that the core can clean up after itself.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Thanks.

Best Regards
Richard Zhu

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 52906f999f2b..e4942bd2598d 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1039,6 +1039,7 @@ static void imx6_pcie_host_exit(struct dw_pcie_rp
> *pp)
> 
>  static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
>  	.host_init = imx6_pcie_host_init,
> +	.host_deinit = imx6_pcie_host_exit,
>  };
> 
>  static const struct dw_pcie_ops dw_pcie_ops = {
> 
> ---
> base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1
> change-id: 20230703-pci-imx-regulator-cleanup-a17c8fd15ec5
> 
> Best regards,
> --
> Mark Brown <broonie@kernel.org>

Hi Mark,

On Mon, Jul 3, 2023 at 1:30 PM Mark Brown <broonie@kernel.org> wrote:
>
> The i.MX integration for the DesignWare PCI controller has a _host_exit()
> operation which undoes everything that the _host_init() operation does but
> does not wire this up as the host_deinit callback for the core, or call it
> in any path other than suspend. This means that if we ever unwind the
> initial probe of the device, for example because it fails, the regulator
> core complains that the regulators for the device were left enabled:
>
> imx6q-pcie 33800000.pcie: iATU: unroll T, 4 ob, 4 ib, align 64K, limit 16G
> imx6q-pcie 33800000.pcie: Phy link never came up
> imx6q-pcie 33800000.pcie: Phy link never came up
> imx6q-pcie: probe of 33800000.pcie failed with error -110
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 46 at drivers/regulator/core.c:2396 _regulator_put+0x110/0x128

Your change looks good, but I have a more basic question.

I recall seeing this regulator warning before:
f81f095e8771 ("PCI: imx6: Allow to probe when dw_pcie_wait_for_link() fails")

I think the real issue is why does the probe fail when the link is not up?

It should not fail to probe. At least, this was the original intention
with Rob's commit:

886a9c134755 ("PCI: dwc: Move link handling into common code")

Should we do something like this?

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index cf61733bf78d..6bd6ffef900d 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -509,6 +509,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)

 err_stop_link:
        dw_pcie_stop_link(pci);
+       return 0;

 err_remove_edma:
        dw_pcie_edma_remove(pci);

On Tue, Jul 04, 2023 at 08:50:15AM -0300, Fabio Estevam wrote:

> I recall seeing this regulator warning before:
> f81f095e8771 ("PCI: imx6: Allow to probe when dw_pcie_wait_for_link() fails")

> I think the real issue is why does the probe fail when the link is not up?

Well, they're both issues - if someone unloads or unbinds the driver we
can still trigger this, even if the probe succeeded.

> It should not fail to probe. At least, this was the original intention
> with Rob's commit:

> 886a9c134755 ("PCI: dwc: Move link handling into common code")

> +       return 0;
> 
>  err_remove_edma:
>         dw_pcie_edma_remove(pci);

I have no idea about doing this, all the PCI on the board that I care
about seems to work fine (and worked fine even with the failure, I'm not
sure this is being generated by a link that's in use).

On Tue, Jul 4, 2023 at 8:55 AM Mark Brown <broonie@kernel.org> wrote:

> Well, they're both issues - if someone unloads or unbinds the driver we
> can still trigger this, even if the probe succeeded.

Yes, for your change:

Tested-by: Fabio Estevam <festevam@gmail.com>

> > It should not fail to probe. At least, this was the original intention
> > with Rob's commit:
>
> > 886a9c134755 ("PCI: dwc: Move link handling into common code")
>
> > +       return 0;
> >
> >  err_remove_edma:
> >         dw_pcie_edma_remove(pci);
>
> I have no idea about doing this, all the PCI on the board that I care
> about seems to work fine (and worked fine even with the failure, I'm not
> sure this is being generated by a link that's in use).

The probe failure when link is not up happens in linux-next due to commit:

da56a1bfbab5 ("PCI: dwc: Wait for link up only if link is started")

Prior this commit the PCI driver probed successfully even when the
link was down.

On Tue, Jul 04, 2023 at 09:13:09AM -0300, Fabio Estevam wrote:
> On Tue, Jul 4, 2023 at 8:55 AM Mark Brown <broonie@kernel.org> wrote:

> > I have no idea about doing this, all the PCI on the board that I care
> > about seems to work fine (and worked fine even with the failure, I'm not
> > sure this is being generated by a link that's in use).

> The probe failure when link is not up happens in linux-next due to commit:

> da56a1bfbab5 ("PCI: dwc: Wait for link up only if link is started")

> Prior this commit the PCI driver probed successfully even when the
> link was down.

Ah, I see.  I don't know what's going on here enough to say if that's
good or bad but it does at least seem plausible.