This series patches refine pci-imx6 driver and do the following main changes.
- Encapsulate the clock enable into one standalone function
- Add the error propagation from host_init and resume
- Turn off regulator when the system is in suspend mode
- Let the probe successfully when link never comes up
- Do not hide the phy driver callbacks in core reset and clk_enable.
BTW, this series are verified on i.MX8MM EVK board when one NVME is used.
Main changes from v12 to v13:
- Call imx6_pcie_host_init() instead of duplicating codes in resume.
- Move the regulator enable out of imx6_pcie_deassert_core_reset().
Re-format the error handling in imx6_pcie_deassert_core_reset()
and imx6_pcie_host_init() accordingly.
Main changes from v11 to v12 issued by Bjorn:
- Add four intro patches to move code around to collect similar things
(PHY management, reset management) together. This makes the first
diff to collect clock enables simpler because it's not cluttered with
unrelated things that didn't actually change.
- Factor out ref clock disables so the disable function structure matches
the enable structure.
- Drop unused "ret" from "Reduce resume time ..." to avoid bisection
hole, then add it back in "Do not hide phy driver ..." where we start
using it again.
- Add patch to make imx6_pcie_clk_disable() symmetric with
imx6_pcie_clk_enable() in terms of enable/disable ordering.
Main changes from v10 to v11:
No code changes, just do the following operations refer to Bjorn's comments.
- Split #6 patch into two patches.
- Rebase to v5.19-rc1 based on for-next branch of Shawn's git.
Main changes from v9 to v10:
- Add the "Reviewed-by: Lucas Stach <[email protected]>" tag into #3
and #4 patches.
- Refer to Bjorn's comments:
- refine the commit of the first patch
- keep alignment of the message format in the second patch
- More specific commit and subject of the #5 and #7 patches.
- Move the regualtor_disable into suspend, turn off the regulator when bus
is powered off and system in suspend mode.
- Let the driver probe successfully, return zero in imx6_pcie_start_link()
when PCIe link is down.
In this link down scenario, only start the PCIe link training in resume
when the link is up before system suspend to avoid the long latency in
the link training period.
- Don't hide phy driver callbacks in core reset and clk_enable, and refine
the error handling accordingly.
- Drop the #8 patch of v9 series, since the clocks and powers are not gated
off anymore when link is down.
Main changes from v8 to v9:
- Don't change pcie-designware codes, and do the error exit process only in
pci-imx6 driver internally.
- Move the phy driver callbacks to the proper places
Main changes from v7 to v8:
Regarding Bjorn's review comments.
- Align the format of the dev_info message and refine commit log of
#6/7/8 patches.
- Rename the err_reset_phy label, since there is no PHY reset in the out
Main changes from v6 to v7:
- Keep the regulator usage counter balance in the #5 patch of v6 series.
Main changes from v5 to v6:
- Refer to the following discussion with Fabio, fix the dump by his patch.
https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
Refine and rebase this patch-set after Fabio' dump fix patch is merged.
- Add one new #4 patch to disable i.MX6QDL REF clock too when disable clocks
- Split the regulator refine codes into one standalone patch #5 in this version.
Main changes from v4 to v5:
- Since i.MX8MM PCIe support had been merged. Based on Lorenzo's git repos,
resend the patch-set after rebase.
Main changes from v3 to v4:
- Regarding Mark's comments, delete the regulator_is_enabled() check.
- Squash #3 and #6 of v3 patch into #5 patch of v4 set.
Main changes from v2 to v3:
- Add "Reviewed-by: Lucas Stach <[email protected]>" tag into
first two patches.
- Add a Fixes tag into #3 patch.
- Split the #4 of v2 to two patches, one is clock disable codes move,
the other one is the acutal clock unbalance fix.
- Add a new host_exit() callback into dw_pcie_host_ops, then it could be
invoked to handle the unbalance issue in the error handling after
host_init() function when link is down.
- Add a new host_exit() callback for i.MX PCIe driver to handle this case
in the error handling after host_init.
Main changes from v1 to v2:
Regarding Lucas' comments.
- Move the placement of the new imx6_pcie_clk_enable() to avoid the
forward declarition.
- Seperate the second patch of v1 patch-set to three patches.
- Use the module_param to replace the kernel command line.
Regarding Bjorn's comments:
- Use the cover-letter for a multi-patch series.
- Correct the subject line, and refine the commit logs. For example,
remove the timestamp of the logs.
Bjorn Helgaas (5):
PCI: imx6: Move imx6_pcie_grp_offset(), imx6_pcie_configure_type()
earlier
PCI: imx6: Move PHY management functions together
PCI: imx6: Move imx6_pcie_enable_ref_clk() earlier
PCI: imx6: Factor out ref clock disable to match enable
PCI: imx6: Disable clocks in reverse order of enable
Richard Zhu (10):
PCI: imx6: Move imx6_pcie_clk_disable() earlier
PCI: imx6: Collect clock enables in imx6_pcie_clk_enable()
PCI: imx6: Propagate .host_init() errors to caller
PCI: imx6: Disable i.MX6QDL clock when disabling ref clocks
PCI: imx6: Call host init function directly in resume
PCI: imx6: Turn off regulator when system is in suspend mode
PCI: imx6: Move regulator enable out of
imx6_pcie_deassert_core_reset()
PCI: imx6: Mark the link down as non-fatal error
PCI: imx6: Reduce resume time by only starting link if it was up
before suspend
PCI: imx6: Do not hide phy driver callbacks and refine the error
handling
drivers/pci/controller/dwc/pci-imx6.c | 574 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------
1 file changed, 308 insertions(+), 266 deletions(-)
Since dw_pcie_host_init() checks for errors from ops->host_init(),
check for errors when enabling power regulators and clocks and return them.
[bhelgaas: commit log]
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Richard Zhu <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Lucas Stach <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index ff5829e42ea7..78b839e92620 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -708,7 +708,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
imx6_pcie->gpio_active_high);
}
-static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
+static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
{
struct dw_pcie *pci = imx6_pcie->pci;
struct device *dev = pci->dev;
@@ -719,7 +719,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
if (ret) {
dev_err(dev, "failed to enable vpcie regulator: %d\n",
ret);
- return;
+ return ret;
}
}
@@ -784,7 +784,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
msleep(100);
}
- return;
+ return 0;
err_clks:
if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
@@ -793,6 +793,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
dev_err(dev, "failed to disable vpcie regulator: %d\n",
ret);
}
+ return ret;
}
static int imx6_pcie_wait_for_speed_change(struct imx6_pcie *imx6_pcie)
@@ -911,11 +912,18 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
static int imx6_pcie_host_init(struct pcie_port *pp)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct device *dev = pci->dev;
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+ int ret;
imx6_pcie_assert_core_reset(imx6_pcie);
imx6_pcie_init_phy(imx6_pcie);
- imx6_pcie_deassert_core_reset(imx6_pcie);
+ ret = imx6_pcie_deassert_core_reset(imx6_pcie);
+ if (ret < 0) {
+ dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
+ return ret;
+ }
+
imx6_setup_phy_mpll(imx6_pcie);
return 0;
--
2.25.1
From: Bjorn Helgaas <[email protected]>
The PCIe ref clocks are specific to different variants. The enables are
already split out into imx6_pcie_enable_ref_clk(), but the disables were
combined with the more generic bus/phy/pcie clock disables in
imx6_pcie_clk_disable().
Split out the variant-specific disables into imx6_pcie_disable_ref_clk() to
match imx6_pcie_enable_ref_clk().
No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
Acked-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 38f208eea2d7..f458461880dc 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -580,12 +580,8 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
return ret;
}
-static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
+static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
{
- clk_disable_unprepare(imx6_pcie->pcie);
- clk_disable_unprepare(imx6_pcie->pcie_phy);
- clk_disable_unprepare(imx6_pcie->pcie_bus);
-
switch (imx6_pcie->drvdata->variant) {
case IMX6SX:
clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
@@ -595,8 +591,8 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
break;
- case IMX8MQ:
case IMX8MM:
+ case IMX8MQ:
clk_disable_unprepare(imx6_pcie->pcie_aux);
break;
default:
@@ -604,6 +600,14 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
}
}
+static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
+{
+ clk_disable_unprepare(imx6_pcie->pcie);
+ clk_disable_unprepare(imx6_pcie->pcie_phy);
+ clk_disable_unprepare(imx6_pcie->pcie_bus);
+ imx6_pcie_disable_ref_clk(imx6_pcie);
+}
+
static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
{
struct device *dev = imx6_pcie->pci->dev;
--
2.25.1
From: Bjorn Helgaas <[email protected]>
Move imx6_pcie_grp_offset() and imx6_pcie_configure_type() earlier in the
file since they depend on nothing and are used by several other functions
that will be moved earlier. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
Acked-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 50 +++++++++++++--------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 7a285fb0f619..8653ca8cbfb9 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -146,6 +146,31 @@ struct imx6_pcie {
#define PHY_RX_OVRD_IN_LO_RX_DATA_EN BIT(5)
#define PHY_RX_OVRD_IN_LO_RX_PLL_EN BIT(3)
+static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
+{
+ WARN_ON(imx6_pcie->drvdata->variant != IMX8MQ &&
+ imx6_pcie->drvdata->variant != IMX8MM);
+ return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
+}
+
+static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
+{
+ unsigned int mask, val;
+
+ if (imx6_pcie->drvdata->variant == IMX8MQ &&
+ imx6_pcie->controller_id == 1) {
+ mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
+ val = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
+ PCI_EXP_TYPE_ROOT_PORT);
+ } else {
+ mask = IMX6Q_GPR12_DEVICE_TYPE;
+ val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
+ PCI_EXP_TYPE_ROOT_PORT);
+ }
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
+}
+
static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
{
struct dw_pcie *pci = imx6_pcie->pci;
@@ -415,13 +440,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
imx6_pcie->gpio_active_high);
}
-static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
-{
- WARN_ON(imx6_pcie->drvdata->variant != IMX8MQ &&
- imx6_pcie->drvdata->variant != IMX8MM);
- return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
-}
-
static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
{
struct dw_pcie *pci = imx6_pcie->pci;
@@ -617,24 +635,6 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
}
}
-static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
-{
- unsigned int mask, val;
-
- if (imx6_pcie->drvdata->variant == IMX8MQ &&
- imx6_pcie->controller_id == 1) {
- mask = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE;
- val = FIELD_PREP(IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
- PCI_EXP_TYPE_ROOT_PORT);
- } else {
- mask = IMX6Q_GPR12_DEVICE_TYPE;
- val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
- PCI_EXP_TYPE_ROOT_PORT);
- }
-
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
-}
-
static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
{
switch (imx6_pcie->drvdata->variant) {
--
2.25.1
Because i.MX PCIe doesn't support hot-plug feature. In the link down
scenario, only start the PCIe link training in resume when the link is up
before system suspend to avoid the long latency in the link training
period.
Signed-off-by: Richard Zhu <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index e236f824c808..abce427f5d3f 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -67,6 +67,7 @@ struct imx6_pcie {
struct dw_pcie *pci;
int reset_gpio;
bool gpio_active_high;
+ bool link_is_up;
struct clk *pcie_bus;
struct clk *pcie_phy;
struct clk *pcie_inbound_axi;
@@ -881,6 +882,7 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
dev_info(dev, "Link: Gen2 disabled\n");
}
+ imx6_pcie->link_is_up = true;
tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
return 0;
@@ -1032,9 +1034,8 @@ static int imx6_pcie_resume_noirq(struct device *dev)
return ret;
dw_pcie_setup_rc(pp);
- ret = imx6_pcie_start_link(imx6_pcie->pci);
- if (ret < 0)
- dev_info(dev, "pcie link is down after resume.\n");
+ if (imx6_pcie->link_is_up)
+ imx6_pcie_start_link(imx6_pcie->pci);
return 0;
}
--
2.25.1
The driver should undo any enables it did itself. The regulator disable
shouldn't be basing decisions on regulator_is_enabled().
Move the regulator_disable to the suspend function, turn off regulator when
the system is in suspend mode.
To keep the balance of the regulator usage counter, disable the regulator
in shutdown.
Link: https://lore.kernel.org/r/1655189942-12678-6-git-send-email-hongxing.z
[email protected]
Signed-off-by: Richard Zhu <[email protected]>
Signed-off-by: Bjorn Helgaas <[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 2b42c37f1617..f72eb609769b 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
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:
@@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
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);
- }
-
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio))
gpio_set_value_cansleep(imx6_pcie->reset_gpio,
@@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
struct device *dev = pci->dev;
int ret;
- 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",
@@ -795,7 +785,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",
@@ -1022,6 +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
break;
}
+ if (imx6_pcie->vpcie)
+ regulator_disable(imx6_pcie->vpcie);
+
return 0;
}
@@ -1268,6 +1261,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, Jun 17, 2022 at 06:31:09PM +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().
>
> Move the regulator_disable to the suspend function, turn off regulator when
> the system is in suspend mode.
>
> To keep the balance of the regulator usage counter, disable the regulator
> in shutdown.
>
> Link: https://lore.kernel.org/r/1655189942-12678-6-git-send-email-hongxing.z
> [email protected]
> Signed-off-by: Richard Zhu <[email protected]>
> Signed-off-by: Bjorn Helgaas <[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 2b42c37f1617..f72eb609769b 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
>
> 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:
> @@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> 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);
> - }
> -
> /* Some boards don't have PCIe reset GPIO. */
> if (gpio_is_valid(imx6_pcie->reset_gpio))
> gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> @@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> struct device *dev = pci->dev;
> int ret;
>
> - 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",
> @@ -795,7 +785,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",
> @@ -1022,6 +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> break;
> }
>
> + if (imx6_pcie->vpcie)
> + regulator_disable(imx6_pcie->vpcie);
> +
> return 0;
> }
The suspend and resume methods should be symmetric, and they should
*look* symmetric.
imx6_pcie_suspend_noirq() disables the regulator, so
imx6_pcie_resume_noirq() should enable it.
imx6_pcie_suspend_noirq() calls imx6_pcie_clk_disable() to disable
several clocks. imx6_pcie_resume_noirq() should call
imx6_pcie_clk_enable() to enable them.
imx6_pcie_clk_enable() *is* called in the resume path, but it's buried
inside imx6_pcie_host_init() and imx6_pcie_deassert_core_reset().
That makes it hard to analyze.
We should be able to look at imx6_pcie_suspend_noirq() and
imx6_pcie_resume_noirq() and easily see that the resume path resumes
everything that was suspended in the suspend path.
Bjorn
> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: 2022??6??24?? 6:20
> 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 v13 10/15] PCI: imx6: Turn off regulator when system is in
> suspend mode
>
> On Fri, Jun 17, 2022 at 06:31:09PM +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().
> >
> > Move the regulator_disable to the suspend function, turn off regulator
> > when the system is in suspend mode.
> >
> > To keep the balance of the regulator usage counter, disable the
> > regulator in shutdown.
> >
> > Link:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&d
> at
> >
> a=05%7C01%7Chongxing.zhu%40nxp.com%7C5633fa1bf3c443e203e108da55
> 667dc2%
> >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379161959277276
> 04%7CUnkn
> >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwi
> >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1Kbzn3XSVvt3gGPrEy%2
> BET8EZn4I
> > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0
> > [email protected]
> > Signed-off-by: Richard Zhu <[email protected]>
> > Signed-off-by: Bjorn Helgaas <[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 2b42c37f1617..f72eb609769b 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie
> > *imx6_pcie)
> >
> > 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:
> > @@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
> > 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);
> > - }
> > -
> > /* Some boards don't have PCIe reset GPIO. */
> > if (gpio_is_valid(imx6_pcie->reset_gpio))
> > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > @@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
> > struct device *dev = pci->dev;
> > int ret;
> >
> > - 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", @@
> -795,7
> > +785,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", @@
> -1022,6
> > +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > break;
> > }
> >
> > + if (imx6_pcie->vpcie)
> > + regulator_disable(imx6_pcie->vpcie);
> > +
> > return 0;
> > }
>
> The suspend and resume methods should be symmetric, and they should
> *look* symmetric.
>
> imx6_pcie_suspend_noirq() disables the regulator, so
> imx6_pcie_resume_noirq() should enable it.
>
> imx6_pcie_suspend_noirq() calls imx6_pcie_clk_disable() to disable several
> clocks. imx6_pcie_resume_noirq() should call
> imx6_pcie_clk_enable() to enable them.
>
> imx6_pcie_clk_enable() *is* called in the resume path, but it's buried inside
> imx6_pcie_host_init() and imx6_pcie_deassert_core_reset().
> That makes it hard to analyze.
>
> We should be able to look at imx6_pcie_suspend_noirq() and
> imx6_pcie_resume_noirq() and easily see that the resume path resumes
> everything that was suspended in the suspend path.
Hi Bjorn:
Thanks for your kindly help to review it.
Yes, it is. It's better to keep suspend/resume symmetric as much as possible.
In resume, the host_init is invoked, clocks, regulators and so on would be
initialized properly.
Unfortunately, there is no according host_exit() that can be called to do the
reversed clocks, regulators disable operations in the suspend.
So, the clocks and regulator disable are explicitly invoked in suspend callback.
How about to do the incremental updates if the .host_exit can be added later?
Best Regards
Richard Zhu
>
> Bjorn
On Fri, Jun 24, 2022 at 05:05:00AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <[email protected]>
> > Sent: 2022年6月24日 6:20
> > 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 v13 10/15] PCI: imx6: Turn off regulator when system is in
> > suspend mode
> >
> > On Fri, Jun 17, 2022 at 06:31:09PM +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().
> > >
> > > Move the regulator_disable to the suspend function, turn off regulator
> > > when the system is in suspend mode.
> > >
> > > To keep the balance of the regulator usage counter, disable the
> > > regulator in shutdown.
> > >
> > > Link:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&d
> > at
> > >
> > a=05%7C01%7Chongxing.zhu%40nxp.com%7C5633fa1bf3c443e203e108da55
> > 667dc2%
> > >
> > 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379161959277276
> > 04%7CUnkn
> > >
> > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > haWwi
> > >
> > LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1Kbzn3XSVvt3gGPrEy%2
> > BET8EZn4I
> > > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0
> > > [email protected]
> > > Signed-off-by: Richard Zhu <[email protected]>
> > > Signed-off-by: Bjorn Helgaas <[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 2b42c37f1617..f72eb609769b 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct imx6_pcie
> > > *imx6_pcie)
> > >
> > > 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:
> > > @@ -702,14 +700,6 @@ static void imx6_pcie_assert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > > 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);
> > > - }
> > > -
> > > /* Some boards don't have PCIe reset GPIO. */
> > > if (gpio_is_valid(imx6_pcie->reset_gpio))
> > > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > > @@ -722,7 +712,7 @@ static int imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > > struct device *dev = pci->dev;
> > > int ret;
> > >
> > > - 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", @@
> > -795,7
> > > +785,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", @@
> > -1022,6
> > > +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > > break;
> > > }
> > >
> > > + if (imx6_pcie->vpcie)
> > > + regulator_disable(imx6_pcie->vpcie);
> > > +
> > > return 0;
> > > }
> >
> > The suspend and resume methods should be symmetric, and they should
> > *look* symmetric.
> >
> > imx6_pcie_suspend_noirq() disables the regulator, so
> > imx6_pcie_resume_noirq() should enable it.
> >
> > imx6_pcie_suspend_noirq() calls imx6_pcie_clk_disable() to disable
> > several clocks. imx6_pcie_resume_noirq() should call
> > imx6_pcie_clk_enable() to enable them.
> >
> > imx6_pcie_clk_enable() *is* called in the resume path, but it's
> > buried inside imx6_pcie_host_init() and
> > imx6_pcie_deassert_core_reset(). That makes it hard to analyze.
> >
> > We should be able to look at imx6_pcie_suspend_noirq() and
> > imx6_pcie_resume_noirq() and easily see that the resume path
> > resumes everything that was suspended in the suspend path.
>
> Yes, it is. It's better to keep suspend/resume symmetric as much as
> possible. In resume, the host_init is invoked, clocks, regulators
> and so on would be initialized properly.
>
> Unfortunately, there is no according host_exit() that can be called
> to do the reversed clocks, regulators disable operations in the
> suspend. So, the clocks and regulator disable are explicitly
> invoked in suspend callback.
>
> How about to do the incremental updates if the .host_exit can be
> added later?
This doesn't seem very convincing because everything here is in the
imx6 domain. The only DWC core thing here is the dw_pcie_setup_rc()
called in imx6_pcie_resume_noirq(), and it doesn't call back to any
imx6 code.
So you should be able to make an imx6_pcie_host_exit() or whatever
that corresponds to imx6_pcie_host_init().
> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: 2022??6??28?? 3:52
> 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 v13 10/15] PCI: imx6: Turn off regulator when system is in
> suspend mode
>
> On Fri, Jun 24, 2022 at 05:05:00AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[email protected]>
> > > Sent: 2022??6??24?? 6:20
> > > 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 v13 10/15] PCI: imx6: Turn off regulator when
> > > system is in suspend mode
> > >
> > > On Fri, Jun 17, 2022 at 06:31:09PM +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().
> > > >
> > > > Move the regulator_disable to the suspend function, turn off
> > > > regulator when the system is in suspend mode.
> > > >
> > > > To keep the balance of the regulator usage counter, disable the
> > > > regulator in shutdown.
> > > >
> > > > Link:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > lore
> > > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&am
> p
> > > > ;d
> > > at
> > > >
> > >
> a=05%7C01%7Chongxing.zhu%40nxp.com%7C5633fa1bf3c443e203e108da55
> > > 667dc2%
> > > >
> > >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379161959277276
> > > 04%7CUnkn
> > > >
> > >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > > haWwi
> > > >
> > >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1Kbzn3XSVvt3gGPrEy%2
> > > BET8EZn4I
> > > > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0
> > > > [email protected]
> > > > Signed-off-by: Richard Zhu <[email protected]>
> > > > Signed-off-by: Bjorn Helgaas <[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 2b42c37f1617..f72eb609769b 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct
> > > > imx6_pcie
> > > > *imx6_pcie)
> > > >
> > > > 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:
> > > > @@ -702,14 +700,6 @@ static void
> > > > imx6_pcie_assert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > > 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);
> > > > - }
> > > > -
> > > > /* Some boards don't have PCIe reset GPIO. */
> > > > if (gpio_is_valid(imx6_pcie->reset_gpio))
> > > > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > > > @@ -722,7 +712,7 @@ static int
> > > > imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > > > struct device *dev = pci->dev;
> > > > int ret;
> > > >
> > > > - 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", @@
> > > -795,7
> > > > +785,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", @@
> > > -1022,6
> > > > +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > > > break;
> > > > }
> > > >
> > > > + if (imx6_pcie->vpcie)
> > > > + regulator_disable(imx6_pcie->vpcie);
> > > > +
> > > > return 0;
> > > > }
> > >
> > > The suspend and resume methods should be symmetric, and they should
> > > *look* symmetric.
> > >
> > > imx6_pcie_suspend_noirq() disables the regulator, so
> > > imx6_pcie_resume_noirq() should enable it.
> > >
> > > imx6_pcie_suspend_noirq() calls imx6_pcie_clk_disable() to disable
> > > several clocks. imx6_pcie_resume_noirq() should call
> > > imx6_pcie_clk_enable() to enable them.
> > >
> > > imx6_pcie_clk_enable() *is* called in the resume path, but it's
> > > buried inside imx6_pcie_host_init() and
> > > imx6_pcie_deassert_core_reset(). That makes it hard to analyze.
> > >
> > > We should be able to look at imx6_pcie_suspend_noirq() and
> > > imx6_pcie_resume_noirq() and easily see that the resume path resumes
> > > everything that was suspended in the suspend path.
> >
> > Yes, it is. It's better to keep suspend/resume symmetric as much as
> > possible. In resume, the host_init is invoked, clocks, regulators and
> > so on would be initialized properly.
> >
> > Unfortunately, there is no according host_exit() that can be called to
> > do the reversed clocks, regulators disable operations in the suspend.
> > So, the clocks and regulator disable are explicitly invoked in suspend
> > callback.
> >
> > How about to do the incremental updates if the .host_exit can be added
> > later?
>
> This doesn't seem very convincing because everything here is in the
> imx6 domain. The only DWC core thing here is the dw_pcie_setup_rc() called
> in imx6_pcie_resume_noirq(), and it doesn't call back to any
> imx6 code.
>
> So you should be able to make an imx6_pcie_host_exit() or whatever that
> corresponds to imx6_pcie_host_init().
Hi Bjorn:
Thanks for your kindly help to review it. That's reasonable.
So, to make it symmetric with imx6_pcie_host_init() and imx6_pcie_start_link().
The according local functions imx6_pcie_host_exit() and imx6_pcie_stop_link()
would be created.
BTW, to be symmetric with imx6_pcie_host_init(), the parameter of
imx6_pcie_host_exit() is same to the parameter of imx6_pcie_host_init().
So do imx6_pcie_stop_link() and imx6_pcie_start_link().
Are you satisfied with the following functions?
static void imx6_pcie_stop_link(struct dw_pcie *pci)
{
struct device *dev = pci->dev;
/* Turn off PCIe LTSSM */
imx6_pcie_ltssm_disable(dev);
}
static void imx6_pcie_host_exit(struct pcie_port *pp)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
imx6_pcie_clk_disable(imx6_pcie);
if (imx6_pcie->phy) {
if (phy_power_off(imx6_pcie->phy))
dev_err(pci->dev, "unable to power off PHY\n");
phy_exit(imx6_pcie->phy);
}
if (imx6_pcie->vpcie)
regulator_disable(imx6_pcie->vpcie);
}
Best Regards
Richard Zhu
On Tue, Jun 28, 2022 at 03:48:01AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <[email protected]>
> > Sent: 2022年6月28日 3:52
> > 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 v13 10/15] PCI: imx6: Turn off regulator when system is in
> > suspend mode
> >
> > On Fri, Jun 24, 2022 at 05:05:00AM +0000, Hongxing Zhu wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <[email protected]>
> > > > Sent: 2022年6月24日 6:20
> > > > 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 v13 10/15] PCI: imx6: Turn off regulator when
> > > > system is in suspend mode
> > > >
> > > > On Fri, Jun 17, 2022 at 06:31:09PM +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().
> > > > >
> > > > > Move the regulator_disable to the suspend function, turn off
> > > > > regulator when the system is in suspend mode.
> > > > >
> > > > > To keep the balance of the regulator usage counter, disable the
> > > > > regulator in shutdown.
> > > > >
> > > > > Link:
> > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > > lore
> > > > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z&am
> > p
> > > > > ;d
> > > > at
> > > > >
> > > >
> > a=05%7C01%7Chongxing.zhu%40nxp.com%7C5633fa1bf3c443e203e108da55
> > > > 667dc2%
> > > > >
> > > >
> > 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379161959277276
> > > > 04%7CUnkn
> > > > >
> > > >
> > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > > > haWwi
> > > > >
> > > >
> > LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1Kbzn3XSVvt3gGPrEy%2
> > > > BET8EZn4I
> > > > > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0
> > > > > [email protected]
> > > > > Signed-off-by: Richard Zhu <[email protected]>
> > > > > Signed-off-by: Bjorn Helgaas <[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 2b42c37f1617..f72eb609769b 100644
> > > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct
> > > > > imx6_pcie
> > > > > *imx6_pcie)
> > > > >
> > > > > 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:
> > > > > @@ -702,14 +700,6 @@ static void
> > > > > imx6_pcie_assert_core_reset(struct
> > > > imx6_pcie *imx6_pcie)
> > > > > 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);
> > > > > - }
> > > > > -
> > > > > /* Some boards don't have PCIe reset GPIO. */
> > > > > if (gpio_is_valid(imx6_pcie->reset_gpio))
> > > > > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > > > > @@ -722,7 +712,7 @@ static int
> > > > > imx6_pcie_deassert_core_reset(struct
> > > > imx6_pcie *imx6_pcie)
> > > > > struct device *dev = pci->dev;
> > > > > int ret;
> > > > >
> > > > > - 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", @@
> > > > -795,7
> > > > > +785,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", @@
> > > > -1022,6
> > > > > +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
> > > > > break;
> > > > > }
> > > > >
> > > > > + if (imx6_pcie->vpcie)
> > > > > + regulator_disable(imx6_pcie->vpcie);
> > > > > +
> > > > > return 0;
> > > > > }
> > > >
> > > > The suspend and resume methods should be symmetric, and they should
> > > > *look* symmetric.
> > > >
> > > > imx6_pcie_suspend_noirq() disables the regulator, so
> > > > imx6_pcie_resume_noirq() should enable it.
> > > >
> > > > imx6_pcie_suspend_noirq() calls imx6_pcie_clk_disable() to disable
> > > > several clocks. imx6_pcie_resume_noirq() should call
> > > > imx6_pcie_clk_enable() to enable them.
> > > >
> > > > imx6_pcie_clk_enable() *is* called in the resume path, but it's
> > > > buried inside imx6_pcie_host_init() and
> > > > imx6_pcie_deassert_core_reset(). That makes it hard to analyze.
> > > >
> > > > We should be able to look at imx6_pcie_suspend_noirq() and
> > > > imx6_pcie_resume_noirq() and easily see that the resume path resumes
> > > > everything that was suspended in the suspend path.
> > >
> > > Yes, it is. It's better to keep suspend/resume symmetric as much as
> > > possible. In resume, the host_init is invoked, clocks, regulators and
> > > so on would be initialized properly.
> > >
> > > Unfortunately, there is no according host_exit() that can be called to
> > > do the reversed clocks, regulators disable operations in the suspend.
> > > So, the clocks and regulator disable are explicitly invoked in suspend
> > > callback.
> > >
> > > How about to do the incremental updates if the .host_exit can be added
> > > later?
> >
> > This doesn't seem very convincing because everything here is in the
> > imx6 domain. The only DWC core thing here is the dw_pcie_setup_rc() called
> > in imx6_pcie_resume_noirq(), and it doesn't call back to any
> > imx6 code.
> >
> > So you should be able to make an imx6_pcie_host_exit() or whatever that
> > corresponds to imx6_pcie_host_init().
>
> Thanks for your kindly help to review it. That's reasonable.
>
> So, to make it symmetric with imx6_pcie_host_init() and
> imx6_pcie_start_link(). The according local functions
> imx6_pcie_host_exit() and imx6_pcie_stop_link() would be created.
>
> BTW, to be symmetric with imx6_pcie_host_init(), the parameter of
> imx6_pcie_host_exit() is same to the parameter of
> imx6_pcie_host_init(). So do imx6_pcie_stop_link() and
> imx6_pcie_start_link(). Are you satisfied with the following
> functions?
>
> static void imx6_pcie_stop_link(struct dw_pcie *pci)
> {
> struct device *dev = pci->dev;
>
> /* Turn off PCIe LTSSM */
> imx6_pcie_ltssm_disable(dev);
> }
>
> static void imx6_pcie_host_exit(struct pcie_port *pp)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
>
> imx6_pcie_clk_disable(imx6_pcie);
> if (imx6_pcie->phy) {
> if (phy_power_off(imx6_pcie->phy))
> dev_err(pci->dev, "unable to power off PHY\n");
> phy_exit(imx6_pcie->phy);
> }
>
> if (imx6_pcie->vpcie)
> regulator_disable(imx6_pcie->vpcie);
> }
After the current series, imx6_pcie_host_init() looks like:
imx6_pcie_host_init
phy_power_on
regulator_enable
imx6_pcie_deassert_core_reset
imx6_pcie_clk_enable
phy_init
and you propose:
imx6_pcie_host_exit
imx6_pcie_clk_disable
phy_power_off
phy_exit
regulator_disable
Generally they should do things in the reverse order.
imx6_pcie_host_init() does phy_power_on(), regulator_enable(),
imx6_pcie_clk_enable().
imx6_pcie_host_exit() should do imx6_pcie_clk_disable(),
regulator_disable(), phy_power_off().
(It looks like imx6_pcie_host_init() calls phy_power_on() and
phy_init() in the wrong order [1].)
IMO the imx6_pcie_clk_enable() should not be hidden inside
imx6_pcie_deassert_core_reset().
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/phy-core.c?id=v5.19-rc1#n233
> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: 2022??6??28?? 23:51
> 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 v13 10/15] PCI: imx6: Turn off regulator when system is in
> suspend mode
>
> On Tue, Jun 28, 2022 at 03:48:01AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[email protected]>
> > > Sent: 2022??6??28?? 3:52
> > > 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 v13 10/15] PCI: imx6: Turn off regulator when
> > > system is in suspend mode
> > >
> > > On Fri, Jun 24, 2022 at 05:05:00AM +0000, Hongxing Zhu wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <[email protected]>
> > > > > Sent: 2022??6??24?? 6:20
> > > > > 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 v13 10/15] PCI: imx6: Turn off regulator
> > > > > when system is in suspend mode
> > > > >
> > > > > On Fri, Jun 17, 2022 at 06:31:09PM +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().
> > > > > >
> > > > > > Move the regulator_disable to the suspend function, turn off
> > > > > > regulator when the system is in suspend mode.
> > > > > >
> > > > > > To keep the balance of the regulator usage counter, disable
> > > > > > the regulator in shutdown.
> > > > > >
> > > > > > Link:
> > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2
> > > > > > F%2F
> > > > > > lore
> > > > > > .kernel.org%2Fr%2F1655189942-12678-6-git-send-email-hongxing.z
> > > > > > &am
> > > p
> > > > > > ;d
> > > > > at
> > > > > >
> > > > >
> > >
> a=05%7C01%7Chongxing.zhu%40nxp.com%7C5633fa1bf3c443e203e108da55
> > > > > 667dc2%
> > > > > >
> > > > >
> > >
> 7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379161959277276
> > > > > 04%7CUnkn
> > > > > >
> > > > >
> > >
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> > > > > haWwi
> > > > > >
> > > > >
> > >
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1Kbzn3XSVvt3gGPrEy%2
> > > > > BET8EZn4I
> > > > > > dwS%2BhUZ3AalZ2YZ0%3D&reserved=0
> > > > > > [email protected]
> > > > > > Signed-off-by: Richard Zhu <[email protected]>
> > > > > > Signed-off-by: Bjorn Helgaas <[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 2b42c37f1617..f72eb609769b 100644
> > > > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > > > @@ -670,8 +670,6 @@ static void imx6_pcie_clk_disable(struct
> > > > > > imx6_pcie
> > > > > > *imx6_pcie)
> > > > > >
> > > > > > 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:
> > > > > > @@ -702,14 +700,6 @@ static void
> > > > > > imx6_pcie_assert_core_reset(struct
> > > > > imx6_pcie *imx6_pcie)
> > > > > > 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);
> > > > > > - }
> > > > > > -
> > > > > > /* Some boards don't have PCIe reset GPIO. */
> > > > > > if (gpio_is_valid(imx6_pcie->reset_gpio))
> > > > > > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > > > > > @@ -722,7 +712,7 @@ static int
> > > > > > imx6_pcie_deassert_core_reset(struct
> > > > > imx6_pcie *imx6_pcie)
> > > > > > struct device *dev = pci->dev;
> > > > > > int ret;
> > > > > >
> > > > > > - 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",
> @@
> > > > > -795,7
> > > > > > +785,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",
> @@
> > > > > -1022,6
> > > > > > +1012,9 @@ static int imx6_pcie_suspend_noirq(struct device
> > > > > > +*dev)
> > > > > > break;
> > > > > > }
> > > > > >
> > > > > > + if (imx6_pcie->vpcie)
> > > > > > + regulator_disable(imx6_pcie->vpcie);
> > > > > > +
> > > > > > return 0;
> > > > > > }
> > > > >
> > > > > The suspend and resume methods should be symmetric, and they
> > > > > should
> > > > > *look* symmetric.
> > > > >
> > > > > imx6_pcie_suspend_noirq() disables the regulator, so
> > > > > imx6_pcie_resume_noirq() should enable it.
> > > > >
> > > > > imx6_pcie_suspend_noirq() calls imx6_pcie_clk_disable() to
> > > > > disable several clocks. imx6_pcie_resume_noirq() should call
> > > > > imx6_pcie_clk_enable() to enable them.
> > > > >
> > > > > imx6_pcie_clk_enable() *is* called in the resume path, but it's
> > > > > buried inside imx6_pcie_host_init() and
> > > > > imx6_pcie_deassert_core_reset(). That makes it hard to analyze.
> > > > >
> > > > > We should be able to look at imx6_pcie_suspend_noirq() and
> > > > > imx6_pcie_resume_noirq() and easily see that the resume path
> > > > > resumes everything that was suspended in the suspend path.
> > > >
> > > > Yes, it is. It's better to keep suspend/resume symmetric as much
> > > > as possible. In resume, the host_init is invoked, clocks,
> > > > regulators and so on would be initialized properly.
> > > >
> > > > Unfortunately, there is no according host_exit() that can be
> > > > called to do the reversed clocks, regulators disable operations in the
> suspend.
> > > > So, the clocks and regulator disable are explicitly invoked in
> > > > suspend callback.
> > > >
> > > > How about to do the incremental updates if the .host_exit can be
> > > > added later?
> > >
> > > This doesn't seem very convincing because everything here is in the
> > > imx6 domain. The only DWC core thing here is the dw_pcie_setup_rc()
> > > called in imx6_pcie_resume_noirq(), and it doesn't call back to any
> > > imx6 code.
> > >
> > > So you should be able to make an imx6_pcie_host_exit() or whatever
> > > that corresponds to imx6_pcie_host_init().
> >
> > Thanks for your kindly help to review it. That's reasonable.
> >
> > So, to make it symmetric with imx6_pcie_host_init() and
> > imx6_pcie_start_link(). The according local functions
> > imx6_pcie_host_exit() and imx6_pcie_stop_link() would be created.
> >
> > BTW, to be symmetric with imx6_pcie_host_init(), the parameter of
> > imx6_pcie_host_exit() is same to the parameter of
> > imx6_pcie_host_init(). So do imx6_pcie_stop_link() and
> > imx6_pcie_start_link(). Are you satisfied with the following
> > functions?
> >
> > static void imx6_pcie_stop_link(struct dw_pcie *pci) {
> > struct device *dev = pci->dev;
> >
> > /* Turn off PCIe LTSSM */
> > imx6_pcie_ltssm_disable(dev);
> > }
> >
> > static void imx6_pcie_host_exit(struct pcie_port *pp) {
> > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> >
> > imx6_pcie_clk_disable(imx6_pcie);
> > if (imx6_pcie->phy) {
> > if (phy_power_off(imx6_pcie->phy))
> > dev_err(pci->dev, "unable to power off PHY\n");
> > phy_exit(imx6_pcie->phy);
> > }
> >
> > if (imx6_pcie->vpcie)
> > regulator_disable(imx6_pcie->vpcie);
> > }
>
> After the current series, imx6_pcie_host_init() looks like:
>
> imx6_pcie_host_init
> phy_power_on
> regulator_enable
> imx6_pcie_deassert_core_reset
> imx6_pcie_clk_enable
> phy_init
>
> and you propose:
>
> imx6_pcie_host_exit
> imx6_pcie_clk_disable
> phy_power_off
> phy_exit
> regulator_disable
>
> Generally they should do things in the reverse order.
>
Hi Bjorn:
Thanks a lot for your review.
> imx6_pcie_host_init() does phy_power_on(), regulator_enable(),
> imx6_pcie_clk_enable().
>
> imx6_pcie_host_exit() should do imx6_pcie_clk_disable(), regulator_disable(),
> phy_power_off().
>
> (It looks like imx6_pcie_host_init() calls phy_power_on() and
> phy_init() in the wrong order [1].)
Yes, it is . I notice the warning too in my local tests. I made a mistake that
I assumed the PHY should be powered on firstly, then be initialized.
Since it is bug fix, how about to issue another fix commit after this series?
>
> IMO the imx6_pcie_clk_enable() should not be hidden inside
> imx6_pcie_deassert_core_reset().
Okay, would move the imx6_pcie_clk_enable() from imx6_pcie_deassert_core_reset()
to imx6_pcie_host_init().
Since the 6/15 of v13 has already Lucas' reviewed-by tag.
Can I combine these changes with the creation of the imx6_pcie_host_exit() and
imx6_pcie_host_stop_link() into one patch?
>
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
> ee%2Fdrivers%2Fphy%2Fphy-core.c%3Fid%3Dv5.19-rc1%23n233&data=
> 05%7C01%7Chongxing.zhu%40nxp.com%7C0e7ce8e53ee0478aabd508da591
> e06f7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379202827
> 66167735%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdat
> a=BQQnn1ju47AGwfQW48%2Ba%2BnlLszha8P0QAynr4G3qeLM%3D&res
> erved=0
Thanks a lot for your kindly reminder.
Best Regards
Richard