The driver should undo any enables it did itself. The regulator disable
shouldn't be basing decisions on regulator_is_enabled().
Signed-off-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 0aca762d88a3..e165ad00989c 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)
@@ -583,7 +573,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",
@@ -656,7 +646,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",
--
2.25.1
Hello Richard,
On Tue, Feb 08, 2022 at 11:25:32AM +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().
>
> Signed-off-by: Richard Zhu <[email protected]>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 0aca762d88a3..e165ad00989c 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);
> - }
This commit is not just cleaning up the regulator usage as you state in
the commit message, this is removing the vpcie regulator_disable
from imx6_pcie_assert_core_reset().
I would not do it, this is called for example on the shutdown callback
where it makes sense.
Francesco
On Mon, Feb 14, 2022 at 04:52:25AM +0000, Hongxing Zhu wrote:
> > > This commit is not just cleaning up the regulator usage as you state
> > > in the commit message, this is removing the vpcie regulator_disable
> > > from imx6_pcie_assert_core_reset().
> > >
> > > I would not do it, this is called for example on the shutdown callback
> > > where it makes sense.
> > Hi Francesco:
> > Thanks for your review.
> > Do you means that we should keep regulator_disable() here?
> > Okay, I would change it later.
> Hi Francesco:
> One more complementary that we can't disable this regulator here, because
> that the regulator might not be enabled at all.
>
> But in the case of suspend/resume operations, the regulator_disable() should
> be invoked behind of imx6_pcie_assert_core_reset () in resume callback to
> balance the enable/disable usage counter.
Understood, please do not forget about the imx6_pcie_shutdown() path,
having this regulator switched off there is important IMO.
A small side comment on the topic, at the moment suspend/resume is
not working correctly for me when the PCIe port is connected to a
switch, after resume only the upstream port is working correctly.
The issue is present on the current mainline driver, but also on
the downstream NXP kernel. Not sure what's the problem and at the
moment is not a priority for me to investigate.
Francesco
> -----Original Message-----
> From: Francesco Dolcini <[email protected]>
> Sent: 2022??2??12?? 0:28
> To: Hongxing Zhu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>;
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v6 5/8] PCI: imx6: Refine the regulator usage
>
> Hello Richard,
>
> On Tue, Feb 08, 2022 at 11:25:32AM +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().
> >
> > Signed-off-by: Richard Zhu <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
> > 1 file changed, 2 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 0aca762d88a3..e165ad00989c 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);
> > - }
>
> This commit is not just cleaning up the regulator usage as you state in the
> commit message, this is removing the vpcie regulator_disable from
> imx6_pcie_assert_core_reset().
>
> I would not do it, this is called for example on the shutdown callback where it
> makes sense.
Hi Francesco:
Thanks for your review.
Do you means that we should keep regulator_disable() here?
Okay, I would change it later.
Best Regards
Richard Zhu
>
> Francesco
> -----Original Message-----
> From: Hongxing Zhu
> Sent: 2022??2??14?? 11:08
> To: Francesco Dolcini <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>;
> [email protected]; [email protected];
> [email protected]
> Subject: RE: [PATCH v6 5/8] PCI: imx6: Refine the regulator usage
>
> > -----Original Message-----
> > From: Francesco Dolcini <[email protected]>
> > Sent: 2022??2??12?? 0:28
> > To: Hongxing Zhu <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; dl-linux-imx <[email protected]>;
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v6 5/8] PCI: imx6: Refine the regulator usage
> >
> > Hello Richard,
> >
> > On Tue, Feb 08, 2022 at 11:25:32AM +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().
> > >
> > > Signed-off-by: Richard Zhu <[email protected]>
> > > ---
> > > drivers/pci/controller/dwc/pci-imx6.c | 14 ++------------
> > > 1 file changed, 2 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 0aca762d88a3..e165ad00989c 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);
> > > - }
> >
> > This commit is not just cleaning up the regulator usage as you state
> > in the commit message, this is removing the vpcie regulator_disable
> > from imx6_pcie_assert_core_reset().
> >
> > I would not do it, this is called for example on the shutdown callback
> > where it makes sense.
> Hi Francesco:
> Thanks for your review.
> Do you means that we should keep regulator_disable() here?
> Okay, I would change it later.
Hi Francesco:
One more complementary that we can't disable this regulator here, because
that the regulator might not be enabled at all.
But in the case of suspend/resume operations, the regulator_disable() should
be invoked behind of imx6_pcie_assert_core_reset () in resume callback to
balance the enable/disable usage counter.
I would add this change later.
Thanks again for your review comments.
Best Regards
Richard
>
> Best Regards
> Richard Zhu
>
> >
> > Francesco