The driver should undo any enables it did itself. The regulator disable
shouldn't be basing decisions on regulator_is_enabled().
To keep the balance of the regulator usage counter, disable the regulator
just behind of imx6_pcie_assert_core_reset() in resume and shutdown.
Signed-off-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 0f1b8c873a4a..160a0bd02098 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
{
- struct device *dev = imx6_pcie->pci->dev;
-
switch (imx6_pcie->drvdata->variant) {
case IMX7D:
case IMX8MQ:
@@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
break;
}
-
- if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
- int ret = regulator_disable(imx6_pcie->vpcie);
-
- if (ret)
- dev_err(dev, "failed to disable vpcie regulator: %d\n",
- ret);
- }
}
static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
@@ -584,7 +574,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
struct device *dev = pci->dev;
int ret, err;
- if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
+ if (imx6_pcie->vpcie) {
ret = regulator_enable(imx6_pcie->vpcie);
if (ret) {
dev_err(dev, "failed to enable vpcie regulator: %d\n",
@@ -657,7 +647,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
return 0;
err_clks:
- if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
+ if (imx6_pcie->vpcie) {
ret = regulator_disable(imx6_pcie->vpcie);
if (ret)
dev_err(dev, "failed to disable vpcie regulator: %d\n",
@@ -1029,6 +1019,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
return 0;
imx6_pcie_assert_core_reset(imx6_pcie);
+ if (imx6_pcie->vpcie)
+ regulator_disable(imx6_pcie->vpcie);
+
imx6_pcie_init_phy(imx6_pcie);
imx6_pcie_deassert_core_reset(imx6_pcie);
dw_pcie_setup_rc(pp);
@@ -1262,6 +1255,8 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
/* bring down link, so bootloader gets clean state in case of reboot */
imx6_pcie_assert_core_reset(imx6_pcie);
+ if (imx6_pcie->vpcie)
+ regulator_disable(imx6_pcie->vpcie);
}
static const struct imx6_pcie_drvdata drvdata[] = {
--
2.25.1
On Fri, Feb 25, 2022 at 11:44:24AM +0800, Richard Zhu wrote:
> The driver should undo any enables it did itself. The regulator disable
> shouldn't be basing decisions on regulator_is_enabled().
>
> To keep the balance of the regulator usage counter, disable the regulator
> just behind of imx6_pcie_assert_core_reset() in resume and shutdown.
While this patch does not look wrong and improving the existing code, I
do have a couple of concern.
1. It's weird that regulator enable/disable are really non symmetric in
the code, enable is in imx6_pcie_deassert_core_reset(), while disable is
not in imx6_pcie_assert_core_reset() but spread around.
2. We are playing around with vpcie during regulator during
suspend/resume, why? We currently have issue with suspend/resume when a
PCIe switch is there, with the final effect of PCIe being not usable
afterward. I fear like this is related to our issue.
What about vpcie enable in probe, and vpcie disable in shutdown and
that's all?
Francesco
>
> Signed-off-by: Richard Zhu <[email protected]>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 0f1b8c873a4a..160a0bd02098 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
>
> static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> {
> - struct device *dev = imx6_pcie->pci->dev;
> -
> switch (imx6_pcie->drvdata->variant) {
> case IMX7D:
> case IMX8MQ:
> @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> break;
> }
> -
> - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> - int ret = regulator_disable(imx6_pcie->vpcie);
> -
> - if (ret)
> - dev_err(dev, "failed to disable vpcie regulator: %d\n",
> - ret);
> - }
> }
>
> static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
> @@ -584,7 +574,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> struct device *dev = pci->dev;
> int ret, err;
>
> - if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
> + if (imx6_pcie->vpcie) {
> ret = regulator_enable(imx6_pcie->vpcie);
> if (ret) {
> dev_err(dev, "failed to enable vpcie regulator: %d\n",
> @@ -657,7 +647,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> return 0;
>
> err_clks:
> - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> + if (imx6_pcie->vpcie) {
> ret = regulator_disable(imx6_pcie->vpcie);
> if (ret)
> dev_err(dev, "failed to disable vpcie regulator: %d\n",
> @@ -1029,6 +1019,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
> return 0;
>
> imx6_pcie_assert_core_reset(imx6_pcie);
> + if (imx6_pcie->vpcie)
> + regulator_disable(imx6_pcie->vpcie);
> +
> imx6_pcie_init_phy(imx6_pcie);
> imx6_pcie_deassert_core_reset(imx6_pcie);
> dw_pcie_setup_rc(pp);
> @@ -1262,6 +1255,8 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
>
> /* bring down link, so bootloader gets clean state in case of reboot */
> imx6_pcie_assert_core_reset(imx6_pcie);
> + if (imx6_pcie->vpcie)
> + regulator_disable(imx6_pcie->vpcie);
> }
>
> static const struct imx6_pcie_drvdata drvdata[] = {
> --
> 2.25.1
>
> -----Original Message-----
> From: Francesco Dolcini <[email protected]>
> Sent: 2022??2??25?? 17:43
> To: Hongxing Zhu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH v8 5/8] PCI: imx6: Refine the regulator usage
>
> On Fri, Feb 25, 2022 at 11:44:24AM +0800, Richard Zhu wrote:
> > The driver should undo any enables it did itself. The regulator
> > disable shouldn't be basing decisions on regulator_is_enabled().
> >
> > To keep the balance of the regulator usage counter, disable the
> > regulator just behind of imx6_pcie_assert_core_reset() in resume and
> shutdown.
>
> While this patch does not look wrong and improving the existing code, I do
> have a couple of concern.
>
> 1. It's weird that regulator enable/disable are really non symmetric in the code,
> enable is in imx6_pcie_deassert_core_reset(), while disable is not in
> imx6_pcie_assert_core_reset() but spread around.
Hi Francesco:
This regulator disable was a part of imx6_pcie_assert_core_reset() before,
just had been moved out of imx6_pcie_assert_core_reset() in this refine
commit.
>
> 2. We are playing around with vpcie during regulator during suspend/resume,
> why? We currently have issue with suspend/resume when a PCIe switch is
> there, with the final effect of PCIe being not usable afterward. I fear like this is
> related to our issue.
>
To my understands, the VPCIE is the abstract of the DIESABLE# signal.
Refer to the Chapter 3.1.12.3 W_DISABLE# Signal of PCI Express M.2 SPEC.
These signals are used to disable the Adapter's radio operation to meet public
Safety regulations or when otherwise desired.
It's reasonable to assert/de-assert W_DISABLE# signal during suspend/resume.
> What about vpcie enable in probe, and vpcie disable in shutdown and that's
> all?
The W_DISABLE# Signal is used to control the radios on the Adapters.
I think you can do that, if the radios are always on in your system.
Best Regards
Richard
>
> Francesco
>
> >
> > Signed-off-by: Richard Zhu <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 19 +++++++------------
> > 1 file changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 0f1b8c873a4a..160a0bd02098 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -369,8 +369,6 @@ static int imx6_pcie_attach_pd(struct device *dev)
> >
> > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > {
> > - struct device *dev = imx6_pcie->pci->dev;
> > -
> > switch (imx6_pcie->drvdata->variant) {
> > case IMX7D:
> > case IMX8MQ:
> > @@ -400,14 +398,6 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
> > IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16);
> > break;
> > }
> > -
> > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > - int ret = regulator_disable(imx6_pcie->vpcie);
> > -
> > - if (ret)
> > - dev_err(dev, "failed to disable vpcie regulator: %d\n",
> > - ret);
> > - }
> > }
> >
> > static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie
> > *imx6_pcie) @@ -584,7 +574,7 @@ static int
> imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > struct device *dev = pci->dev;
> > int ret, err;
> >
> > - if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
> > + if (imx6_pcie->vpcie) {
> > ret = regulator_enable(imx6_pcie->vpcie);
> > if (ret) {
> > dev_err(dev, "failed to enable vpcie regulator: %d\n", @@
> -657,7
> > +647,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie
> *imx6_pcie)
> > return 0;
> >
> > err_clks:
> > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
> > + if (imx6_pcie->vpcie) {
> > ret = regulator_disable(imx6_pcie->vpcie);
> > if (ret)
> > dev_err(dev, "failed to disable vpcie regulator: %d\n", @@
> -1029,6
> > +1019,9 @@ static int imx6_pcie_resume_noirq(struct device *dev)
> > return 0;
> >
> > imx6_pcie_assert_core_reset(imx6_pcie);
> > + if (imx6_pcie->vpcie)
> > + regulator_disable(imx6_pcie->vpcie);
> > +
> > imx6_pcie_init_phy(imx6_pcie);
> > imx6_pcie_deassert_core_reset(imx6_pcie);
> > dw_pcie_setup_rc(pp);
> > @@ -1262,6 +1255,8 @@ static void imx6_pcie_shutdown(struct
> > platform_device *pdev)
> >
> > /* bring down link, so bootloader gets clean state in case of reboot */
> > imx6_pcie_assert_core_reset(imx6_pcie);
> > + if (imx6_pcie->vpcie)
> > + regulator_disable(imx6_pcie->vpcie);
> > }
> >
> > static const struct imx6_pcie_drvdata drvdata[] = {
> > --
> > 2.25.1
> >
> -----Original Message-----
> From: Francesco Dolcini <[email protected]>
> Sent: 2022??3??4?? 15:46
> To: Hongxing Zhu <[email protected]>
> Cc: Francesco Dolcini <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>
> Subject: Re: [PATCH v8 5/8] PCI: imx6: Refine the regulator usage
>
> On Mon, Feb 28, 2022 at 03:43:03AM +0000, Hongxing Zhu wrote:
> > > 2. We are playing around with vpcie during regulator during
> > > suspend/resume, why? We currently have issue with suspend/resume
> > > when a PCIe switch is there, with the final effect of PCIe being not
> > > usable afterward. I fear like this is related to our issue.
> > >
> > To my understands, the VPCIE is the abstract of the DIESABLE# signal.
> > Refer to the Chapter 3.1.12.3 W_DISABLE# Signal of PCI Express M.2 SPEC.
> > These signals are used to disable the Adapter's radio operation to
> > meet public Safety regulations or when otherwise desired.
> > It's reasonable to assert/de-assert W_DISABLE# signal during
> suspend/resume.
> >
> > > What about vpcie enable in probe, and vpcie disable in shutdown and
> > > that's all?
> > The W_DISABLE# Signal is used to control the radios on the Adapters.
> > I think you can do that, if the radios are always on in your system.
>
> According to the binding documentation (imx6q-pcie.yaml)
>
> vpcie-supply:
> description: Should specify the regulator in charge of PCIe port power.
> The regulator will be enabled when initializing the PCIe host and
> disabled either as part of the init process or when shutting down
> the host (optional required).
>
> This is how it's documented to be used, if this is used according to the
> documentation (and this is happening) what you are already doing (not
> changed in this patch, you are right) is not correct.
>
> Abusing this for the W_DISABLE# disable is new to me and does not look legit.
The vpcie-supply is one gpio regulator actually, used to control remote Endpoint
device, through the DISABLE# signal (defined in Mini CEM/M.2 SPEC) of the port.
Best Regards
Richard Zhu
>
> Francesco
On Mon, Feb 28, 2022 at 03:43:03AM +0000, Hongxing Zhu wrote:
> > 2. We are playing around with vpcie during regulator during suspend/resume,
> > why? We currently have issue with suspend/resume when a PCIe switch is
> > there, with the final effect of PCIe being not usable afterward. I fear like this is
> > related to our issue.
> >
> To my understands, the VPCIE is the abstract of the DIESABLE# signal.
> Refer to the Chapter 3.1.12.3 W_DISABLE# Signal of PCI Express M.2 SPEC.
> These signals are used to disable the Adapter's radio operation to meet public
> Safety regulations or when otherwise desired.
> It's reasonable to assert/de-assert W_DISABLE# signal during suspend/resume.
>
> > What about vpcie enable in probe, and vpcie disable in shutdown and that's
> > all?
> The W_DISABLE# Signal is used to control the radios on the Adapters.
> I think you can do that, if the radios are always on in your system.
According to the binding documentation (imx6q-pcie.yaml)
vpcie-supply:
description: Should specify the regulator in charge of PCIe port power.
The regulator will be enabled when initializing the PCIe host and
disabled either as part of the init process or when shutting down
the host (optional required).
This is how it's documented to be used, if this is used according to the
documentation (and this is happening) what you are already doing (not
changed in this patch, you are right) is not correct.
Abusing this for the W_DISABLE# disable is new to me and does not look
legit.
Francesco