| Message ID | 1634886750-13861-4-git-send-email-hongxing.zhu@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: imx6: refine codes and add compliance tests mode support | expand |
Hello Richard, please see this comment from Mark, https://lore.kernel.org/all/YXaGve1ZJq0DGZ9l@sirena.org.uk/. Francesco On Fri, Oct 22, 2021 at 03:12:26PM +0800, Richard Zhu wrote: > When PCIe PHY link never came up and vpcie regulator is present, there > would be following dump when try to put the regulator. > Disable this regulator to fix this dump when 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: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158 > Modules linked in: > CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10 > Hardware name: FSL i.MX8MM EVK board (DT) > Workqueue: events_unbound async_run_entry_fn > pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--) > pc : _regulator_put.part.0+0x14c/0x158 > lr : regulator_put+0x34/0x48 > sp : ffff8000122ebb30 > x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000 > x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc > x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18 > x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010 > x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108 > x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000 > x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540 > x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8 > x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30 > x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001 > Call trace: > _regulator_put.part.0+0x14c/0x158 > regulator_put+0x34/0x48 > devm_regulator_release+0x10/0x18 > release_nodes+0x38/0x60 > devres_release_all+0x88/0xd0 > really_probe+0xd0/0x2e8 > __driver_probe_device+0x74/0xd8 > driver_probe_device+0x7c/0x108 > __device_attach_driver+0x8c/0xd0 > bus_for_each_drv+0x74/0xc0 > __device_attach_async_helper+0xb4/0xd8 > async_run_entry_fn+0x30/0x100 > process_one_work+0x19c/0x320 > worker_thread+0x48/0x418 > kthread+0x14c/0x158 > ret_from_fork+0x10/0x18 > ---[ end trace 3664ca4a50ce849b ]--- > > Link: https://lore.kernel.org/r/20201105211159.1814485-11-robh@kernel.org > Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code") > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 3372775834a2..39a485bfc676 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -1180,8 +1180,12 @@ static int imx6_pcie_probe(struct platform_device *pdev) > return ret; > > ret = dw_pcie_host_init(&pci->pp); > - if (ret < 0) > + if (ret < 0) { > + if (imx6_pcie->vpcie > + && regulator_is_enabled(imx6_pcie->vpcie) > 0) > + regulator_disable(imx6_pcie->vpcie); > return ret; > + } > > if (pci_msi_enabled()) { > u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI); > -- > 2.25.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Oct 25, 2021 at 01:13:12PM +0200, Francesco Dolcini wrote: > Hello Richard, > please see this comment from Mark, https://lore.kernel.org/all/YXaGve1ZJq0DGZ9l@sirena.org.uk/. > > + if (imx6_pcie->vpcie > > + && regulator_is_enabled(imx6_pcie->vpcie) > 0) > > + regulator_disable(imx6_pcie->vpcie); > > return ret; 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. :(
> -----Original Message----- > From: Francesco Dolcini <francesco.dolcini@toradex.com> > Sent: Monday, October 25, 2021 7:13 PM > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: 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; Mark Brown <broonie@kernel.org> > Subject: Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never > came up > > Hello Richard, > please see this comment from Mark, > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke > rnel.org%2Fall%2FYXaGve1ZJq0DGZ9l%40sirena.org.uk%2F&data=04%7 > C01%7Chongxing.zhu%40nxp.com%7C27ddf15a4ef34496eff708d997a87097 > %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637707571965246 > 405%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=nLZcGoLwXWEr > HR14ZLg8prtPuotNWHBrQb8J99HiNT0%3D&reserved=0. > > Francesco [Richard Zhu] Got that, thanks for your reminder. Best Regards Richard Zhu > > > On Fri, Oct 22, 2021 at 03:12:26PM +0800, Richard Zhu wrote: > > When PCIe PHY link never came up and vpcie regulator is present, there > > would be following dump when try to put the regulator. > > Disable this regulator to fix this dump when 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: 3 PID: 119 at drivers/regulator/core.c:2256 > _regulator_put.part.0+0x14c/0x158 > > Modules linked in: > > CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted > 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10 > > Hardware name: FSL i.MX8MM EVK board (DT) > > Workqueue: events_unbound async_run_entry_fn > > pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--) > > pc : _regulator_put.part.0+0x14c/0x158 > > lr : regulator_put+0x34/0x48 > > sp : ffff8000122ebb30 > > x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000 > > x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc > > x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18 > > x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010 > > x17: 000000040044ffff x16: 00400032b5503510 x15: > 0000000000000108 > > x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000 > > x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540 > > x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8 > > x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30 > > x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001 > > Call trace: > > _regulator_put.part.0+0x14c/0x158 > > regulator_put+0x34/0x48 > > devm_regulator_release+0x10/0x18 > > release_nodes+0x38/0x60 > > devres_release_all+0x88/0xd0 > > really_probe+0xd0/0x2e8 > > __driver_probe_device+0x74/0xd8 > > driver_probe_device+0x7c/0x108 > > __device_attach_driver+0x8c/0xd0 > > bus_for_each_drv+0x74/0xc0 > > __device_attach_async_helper+0xb4/0xd8 > > async_run_entry_fn+0x30/0x100 > > process_one_work+0x19c/0x320 > > worker_thread+0x48/0x418 > > kthread+0x14c/0x158 > > ret_from_fork+0x10/0x18 > > ---[ end trace 3664ca4a50ce849b ]--- > > > > Link: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore > > .kernel.org%2Fr%2F20201105211159.1814485-11-robh%40kernel.org&am > p;data > > > =04%7C01%7Chongxing.zhu%40nxp.com%7C27ddf15a4ef34496eff708d997a > 87097%7 > > > C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63770757196524640 > 5%7CUnkno > > > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h > aWwiL > > > CJXVCI6Mn0%3D%7C1000&sdata=mJbSEVYuoGCbCOqqXdcUG00JXu74l > 5%2BYxpzCX > > yK96pE%3D&reserved=0 > > Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code") > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > b/drivers/pci/controller/dwc/pci-imx6.c > > index 3372775834a2..39a485bfc676 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -1180,8 +1180,12 @@ static int imx6_pcie_probe(struct > platform_device *pdev) > > return ret; > > > > ret = dw_pcie_host_init(&pci->pp); > > - if (ret < 0) > > + if (ret < 0) { > > + if (imx6_pcie->vpcie > > + && regulator_is_enabled(imx6_pcie->vpcie) > 0) > > + regulator_disable(imx6_pcie->vpcie); > > return ret; > > + } > > > > if (pci_msi_enabled()) { > > u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI); > > -- > > 2.25.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists > > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=04% > 7C0 > > > 1%7Chongxing.zhu%40nxp.com%7C27ddf15a4ef34496eff708d997a87097%7 > C686ea1 > > > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637707571965246405%7CUn > known%7CTW > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > VCI6 > > > Mn0%3D%7C1000&sdata=SHLkdzTt2F1Nht9eoefHlEH9Klsnw3%2F7KOP > uGGeI%2Ba > > w%3D&reserved=0
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Monday, October 25, 2021 7:24 PM > To: Francesco Dolcini <francesco.dolcini@toradex.com> > Cc: Richard Zhu <hongxing.zhu@nxp.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 Mon, Oct 25, 2021 at 01:13:12PM +0200, Francesco Dolcini wrote: > > > Hello Richard, > > please see this comment from Mark, > https://lore.kernel.org/all/YXaGve1ZJq0DGZ9l@sirena.org.uk/. > > > > + if (imx6_pcie->vpcie > > > + && regulator_is_enabled(imx6_pcie->vpcie) > 0) > > > + regulator_disable(imx6_pcie->vpcie); > > > return ret; > > 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. 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. Best Regards Richard Zhu
On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote: > 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. > > 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. Hello Richard, I have one curiosity on this topic. Does this works well in case the regulator is shared? I just want to be sure that if the regulator is shared everything is working fine even if the PCI-E link is not used or not coming up for any reason. Francesco
> -----Original Message----- > From: Francesco Dolcini <francesco.dolcini@toradex.com> > Sent: Tuesday, October 26, 2021 4:52 PM > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: Mark Brown <broonie@kernel.org>; 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: > > 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. > > > > 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. > > Hello Richard, > I have one curiosity on this topic. Does this works well in case the regulator is > shared? I just want to be sure that if the regulator is shared everything is > working fine even if the PCI-E link is not used or not coming up for any reason. > [Richard Zhu] Hi Francesco: Actually, this regulator used by i.MX PCIe controller driver is one fixed gpio regulator. Refer to my understand, this regulator is not shared by different devices. Up to now, it works fine to power up the PCIe slot populated on the HW board. BR Richard > Francesco
On Tue, Oct 26, 2021 at 09:06:56AM +0000, Richard Zhu wrote: > > -----Original Message----- > > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > Sent: Tuesday, October 26, 2021 4:52 PM > > To: Richard Zhu <hongxing.zhu@nxp.com> > > Cc: Mark Brown <broonie@kernel.org>; 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: > > > 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. > > > > > > 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. > > > > Hello Richard, > > I have one curiosity on this topic. Does this works well in case the regulator is > > shared? I just want to be sure that if the regulator is shared everything is > > working fine even if the PCI-E link is not used or not coming up for any reason. > > > [Richard Zhu] Hi Francesco: > Actually, this regulator used by i.MX PCIe controller driver is one fixed gpio regulator. > Refer to my understand, this regulator is not shared by different devices. > > Up to now, it works fine to power up the PCIe slot populated on the HW board. Isn't this something that depend on the actual board design? From the driver point of view you should not silently enforce such design requirement on the board. Am I missing something here? Would be glad to you if you can clarify in case. Francesco
Snipped... > > > > > > Hello Richard, > > > I have one curiosity on this topic. Does this works well in case the > > > regulator is shared? I just want to be sure that if the regulator is > > > shared everything is working fine even if the PCI-E link is not used or not > coming up for any reason. > > > > > [Richard Zhu] Hi Francesco: > > Actually, this regulator used by i.MX PCIe controller driver is one fixed gpio > regulator. > > Refer to my understand, this regulator is not shared by different devices. > > > > Up to now, it works fine to power up the PCIe slot populated on the HW > board. > > Isn't this something that depend on the actual board design? From the driver > point of view you should not silently enforce such design requirement on the > board. > Am I missing something here? Would be glad to you if you can clarify in case. > [Richard Zhu] Yes, it is relied on the actual HW board design. This regulator is one optional, not mandatory required for all the board designs. So, there is one _enabled or not check before manipulate this regulator. BR Richard > Francesco >
On Tue, Oct 26, 2021 at 09:18:39AM +0000, Richard Zhu wrote: > > Isn't this something that depend on the actual board design? From the driver > > point of view you should not silently enforce such design requirement on the > > board. > > Am I missing something here? Would be glad to you if you can clarify in case. > > > [Richard Zhu] Yes, it is relied on the actual HW board design. > This regulator is one optional, not mandatory required for all the board designs. > So, there is one _enabled or not check before manipulate this regulator. I think I was not clear in my question. I'm asking what's is going to happen if the vpci-e supply is used in the actual board design AND the same regulator is shared with another device (to my understanding this should be just fine from the regulator API point of view, correct me if I'm wrong). I'm not talking about board designed by NXP in which such use case might not exist. Francesco
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. > 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. 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.
> -----Original Message----- > From: Francesco Dolcini <francesco.dolcini@toradex.com> > Sent: Tuesday, October 26, 2021 5:49 PM > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: Francesco Dolcini <francesco.dolcini@toradex.com>; Mark Brown > <broonie@kernel.org>; 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 09:18:39AM +0000, Richard Zhu wrote: > > > Isn't this something that depend on the actual board design? From > > > the driver point of view you should not silently enforce such design > > > requirement on the board. > > > Am I missing something here? Would be glad to you if you can clarify > in case. > > > > > [Richard Zhu] Yes, it is relied on the actual HW board design. > > This regulator is one optional, not mandatory required for all the board > designs. > > So, there is one _enabled or not check before manipulate this regulator. > I think I was not clear in my question. > > I'm asking what's is going to happen if the vpci-e supply is used in the > actual board design AND the same regulator is shared with another > device (to my understanding this should be just fine from the regulator API > point of view, correct me if I'm wrong). [Richard Zhu] Yes, agree with you. It should be fine from the regulator API point of view. BR Richard > > I'm not talking about board designed by NXP in which such use case might > not exist. > > Francesco
> -----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
On Thu, Oct 28, 2021 at 06:50:58AM +0000, Richard Zhu wrote: > > 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. No, it's not. The regulator API supports the systems where the regualtor is always on perfectly well, the client driver should not need to do anything to support them. > > 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. The driver should undo any enables it did itself, it should not undo any enables that anything else did which means it should never be basing decisions on regulator_is_enabled(). While the regulator may not be shared in the particular board you're looking at it may be shared in other systems.
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Thursday, October 28, 2021 7:50 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 Thu, Oct 28, 2021 at 06:50:58AM +0000, Richard Zhu wrote: > > > > 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. > > No, it's not. The regulator API supports the systems where the regualtor > is always on perfectly well, the client driver should not need to do > anything to support them. [Richard Zhu] Hi Mark: Thanks for your explains. To disable the regulator explicitly, is a part of power save of i.MX PCIe port usage when link is down. Because that this regulator might not be present at all on some boards (e.x: powered directly when board is powered up), so this regulator is optional from SW view. > > > > 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. > > The driver should undo any enables it did itself, it should not undo any > enables that anything else did which means it should never be basing > decisions on regulator_is_enabled(). While the regulator may not be > shared in the particular board you're looking at it may be shared in other > systems. [Richard Zhu] Understood. Thanks. Can I disabled this regulator in PCIe probe failure handler without the regulator_is_enabled() check? BR Richard
On Fri, Oct 29, 2021 at 03:58:41AM +0000, Richard Zhu wrote: > > The driver should undo any enables it did itself, it should not undo any > > enables that anything else did which means it should never be basing > > decisions on regulator_is_enabled(). While the regulator may not be > > shared in the particular board you're looking at it may be shared in other > > systems. > [Richard Zhu] Understood. Thanks. > Can I disabled this regulator in PCIe probe failure handler without the > regulator_is_enabled() check? If the driver called regulator_enable() (and that didn't return an error) it can always call regulator_disable() as many times as it called regulator_enable(), no need to check if the regulator is still enabled. When the driver hasn't successfully called regulator_enable() it shouldn't call regulator_disable() even if the regualtor is enabled. This means that after your driver has enabled the regulator it can just disable it but between the regulator_get() and regulator_enable() it shouldn't do that.
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Friday, October 29, 2021 7:46 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 Fri, Oct 29, 2021 at 03:58:41AM +0000, Richard Zhu wrote: > > > > The driver should undo any enables it did itself, it should not undo > > > any enables that anything else did which means it should never be > > > basing decisions on regulator_is_enabled(). While the regulator may > > > not be shared in the particular board you're looking at it may be > > > shared in other systems. > > > [Richard Zhu] Understood. Thanks. > > Can I disabled this regulator in PCIe probe failure handler without > > the > > regulator_is_enabled() check? > > If the driver called regulator_enable() (and that didn't return an > error) it can always call regulator_disable() as many times as it called > regulator_enable(), no need to check if the regulator is still enabled. > When the driver hasn't successfully called regulator_enable() it shouldn't > call regulator_disable() even if the regualtor is enabled. > This means that after your driver has enabled the regulator it can just > disable it but between the regulator_get() and regulator_enable() it > shouldn't do that. [Richard Zhu] Got that, thanks a lot. BR Richard
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 3372775834a2..39a485bfc676 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -1180,8 +1180,12 @@ static int imx6_pcie_probe(struct platform_device *pdev) return ret; ret = dw_pcie_host_init(&pci->pp); - if (ret < 0) + if (ret < 0) { + if (imx6_pcie->vpcie + && regulator_is_enabled(imx6_pcie->vpcie) > 0) + regulator_disable(imx6_pcie->vpcie); return ret; + } if (pci_msi_enabled()) { u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
When PCIe PHY link never came up and vpcie regulator is present, there would be following dump when try to put the regulator. Disable this regulator to fix this dump when 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: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158 Modules linked in: CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10 Hardware name: FSL i.MX8MM EVK board (DT) Workqueue: events_unbound async_run_entry_fn pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--) pc : _regulator_put.part.0+0x14c/0x158 lr : regulator_put+0x34/0x48 sp : ffff8000122ebb30 x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000 x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18 x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010 x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108 x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000 x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540 x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8 x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30 x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001 Call trace: _regulator_put.part.0+0x14c/0x158 regulator_put+0x34/0x48 devm_regulator_release+0x10/0x18 release_nodes+0x38/0x60 devres_release_all+0x88/0xd0 really_probe+0xd0/0x2e8 __driver_probe_device+0x74/0xd8 driver_probe_device+0x7c/0x108 __device_attach_driver+0x8c/0xd0 bus_for_each_drv+0x74/0xc0 __device_attach_async_helper+0xb4/0xd8 async_run_entry_fn+0x30/0x100 process_one_work+0x19c/0x320 worker_thread+0x48/0x418 kthread+0x14c/0x158 ret_from_fork+0x10/0x18 ---[ end trace 3664ca4a50ce849b ]--- Link: https://lore.kernel.org/r/20201105211159.1814485-11-robh@kernel.org Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code") Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> --- drivers/pci/controller/dwc/pci-imx6.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)