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 <[email protected]>
---
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 <[email protected]>
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: 2023年7月4日 0:23
> To: Hongxing Zhu <[email protected]>; Lucas Stach
> <[email protected]>; Lorenzo Pieralisi <[email protected]>; Krzysztof
> Wilczyński <[email protected]>; Rob Herring <[email protected]>; Bjorn Helgaas
> <[email protected]>; Shawn Guo <[email protected]>; Sascha Hauer
> <[email protected]>; Pengutronix Kernel Team <[email protected]>;
> Fabio Estevam <[email protected]>; dl-linux-imx <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Mark Brown <[email protected]>
> 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 <[email protected]>
Reviewed-by: Richard Zhu <[email protected]>
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 <[email protected]>
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).
Hi Mark,
On Mon, Jul 3, 2023 at 1:30 PM Mark Brown <[email protected]> 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 4, 2023 at 8:55 AM Mark Brown <[email protected]> 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 <[email protected]>
> > 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 <[email protected]> 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.