This series patches refine pci-imx6 driver and do the following changes.
- Encapsulate the clock enable into one standalone function
- Add the error propagation from host_init
- Balance the usage of the regulator and clocks when link never came up
- Add the compliance tests mode support
Main changes from v4 to v5:
- Since i.MX8MM PCIe support had been merged. Based on Lorenzo's git repos,
rebase and resend the patch-set.
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.
drivers/pci/controller/dwc/pci-imx6.c | 197 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------
drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++-
drivers/pci/controller/dwc/pcie-designware.h | 1 +
3 files changed, 128 insertions(+), 75 deletions(-)
[PATCH v5 1/6] PCI: imx6: Encapsulate the clock enable into one
[PATCH v5 2/6] PCI: imx6: Add the error propagation from host_init
[PATCH v5 3/6] PCI: imx6: PCI: imx6: Move imx6_pcie_clk_disable()
[PATCH v5 4/6] PCI: dwc: Add dw_pcie_host_ops.host_exit() callback
[PATCH v5 5/6] PCI: imx6: Fix the regulator dump when link never came
[PATCH v5 6/6] PCI: imx6: Add the compliance tests mode support
Since there is error return check of the host_init callback, add error
check to imx6_pcie_deassert_core_reset() function, and change the
function type accordingly.
Signed-off-by: Richard Zhu <[email protected]>
Reviewed-by: Lucas Stach <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 1d324fd05d7f..13a4328bea19 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -546,24 +546,24 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
dev_err(dev, "PCIe PLL lock timeout\n");
}
-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;
- int ret;
+ int ret, err;
if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
ret = regulator_enable(imx6_pcie->vpcie);
if (ret) {
dev_err(dev, "failed to enable vpcie regulator: %d\n",
ret);
- return;
+ return ret;
}
}
- ret = imx6_pcie_clk_enable(imx6_pcie);
- if (ret) {
- dev_err(dev, "unable to enable pcie clocks\n");
+ err = imx6_pcie_clk_enable(imx6_pcie);
+ if (err) {
+ dev_err(dev, "unable to enable pcie clocks: %d\n", err);
goto err_clks;
}
@@ -622,7 +622,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
break;
}
- return;
+ return 0;
err_clks:
if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
@@ -631,6 +631,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 err;
}
static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
@@ -888,11 +889,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 host init failed: %d.\n", ret);
+ return ret;
+ }
+
imx6_setup_phy_mpll(imx6_pcie);
return 0;
--
2.25.1
Refer to the system board signal Quality of PCIe archiecture PHY test
specification. Signal quality tests(for example: jitters, differential
eye opening and so on ) can be executed with devices in the
polling.compliance state.
To let the device support polling.compliance stat, the clocks and powers
shouldn't be turned off when the probe of device driver is failed.
Based on CLB(Compliance Load Board) Test Fixture and so on test
equipments, the PHY link would be down during the compliance tests.
Refer to this scenario, add the i.MX PCIe compliance tests mode enable
support, and keep the clocks and powers on, and finish the driver probe
without error return.
Use the "pci_imx6.compliance=1" in kernel command line to enable the
compliance tests mode.
Signed-off-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 47 ++++++++++++++++++---------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 61b25f5cbf5c..773b9e45806b 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -146,6 +146,10 @@ 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 bool imx6_pcie_cmp_mode;
+module_param_named(compliance, imx6_pcie_cmp_mode, bool, 0644);
+MODULE_PARM_DESC(compliance, "i.MX PCIe compliance test mode (1=compliance test mode enabled)");
+
static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
{
struct dw_pcie *pci = imx6_pcie->pci;
@@ -832,10 +836,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
* started in Gen2 mode, there is a possibility the devices on the
* bus will not be detected at all. This happens with PCIe switches.
*/
- tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
- tmp &= ~PCI_EXP_LNKCAP_SLS;
- tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
- dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
+ if (!imx6_pcie_cmp_mode) {
+ tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
+ tmp &= ~PCI_EXP_LNKCAP_SLS;
+ tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
+ dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
+ }
/* Start LTSSM. */
imx6_pcie_ltssm_enable(dev);
@@ -924,18 +930,20 @@ static void imx6_pcie_host_exit(struct pcie_port *pp)
struct device *dev = pci->dev;
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
- imx6_pcie_reset_phy(imx6_pcie);
- imx6_pcie_clk_disable(imx6_pcie);
- switch (imx6_pcie->drvdata->variant) {
- case IMX8MM:
- if (phy_power_off(imx6_pcie->phy))
- dev_err(dev, "unable to power off phy\n");
- break;
- default:
- break;
+ if (!imx6_pcie_cmp_mode) {
+ imx6_pcie_reset_phy(imx6_pcie);
+ imx6_pcie_clk_disable(imx6_pcie);
+ switch (imx6_pcie->drvdata->variant) {
+ case IMX8MM:
+ if (phy_power_off(imx6_pcie->phy))
+ dev_err(dev, "unable to power off phy\n");
+ break;
+ default:
+ break;
+ }
+ if (imx6_pcie->vpcie)
+ regulator_disable(imx6_pcie->vpcie);
}
- if (imx6_pcie->vpcie)
- regulator_disable(imx6_pcie->vpcie);
}
static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
@@ -1250,8 +1258,15 @@ 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_cmp_mode) {
+ dev_info(dev, "Driver loaded with compliance test mode enabled.\n");
+ ret = 0;
+ } else {
+ dev_err(dev, "Unable to add pcie port.\n");
+ }
return ret;
+ }
if (pci_msi_enabled()) {
u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
--
2.25.1
No function changes, just encapsulate the i.MX PCIe clocks enable
operations into one standalone function
Signed-off-by: Richard Zhu <[email protected]>
Reviewed-by: Lucas Stach <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 79 ++++++++++++++++-----------
1 file changed, 48 insertions(+), 31 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index cf139e02c376..1d324fd05d7f 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -481,38 +481,16 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
return ret;
}
-static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
-{
- u32 val;
- struct device *dev = imx6_pcie->pci->dev;
-
- if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
- IOMUXC_GPR22, val,
- val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
- PHY_PLL_LOCK_WAIT_USLEEP_MAX,
- PHY_PLL_LOCK_WAIT_TIMEOUT))
- dev_err(dev, "PCIe PLL lock timeout\n");
-}
-
-static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
+static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
{
struct dw_pcie *pci = imx6_pcie->pci;
struct device *dev = pci->dev;
int ret;
- if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
- ret = regulator_enable(imx6_pcie->vpcie);
- if (ret) {
- dev_err(dev, "failed to enable vpcie regulator: %d\n",
- ret);
- return;
- }
- }
-
ret = clk_prepare_enable(imx6_pcie->pcie_phy);
if (ret) {
dev_err(dev, "unable to enable pcie_phy clock\n");
- goto err_pcie_phy;
+ return ret;
}
ret = clk_prepare_enable(imx6_pcie->pcie_bus);
@@ -543,6 +521,51 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
}
/* allow the clocks to stabilize */
usleep_range(200, 500);
+ return 0;
+
+err_ref_clk:
+ clk_disable_unprepare(imx6_pcie->pcie);
+err_pcie:
+ clk_disable_unprepare(imx6_pcie->pcie_bus);
+err_pcie_bus:
+ clk_disable_unprepare(imx6_pcie->pcie_phy);
+
+ return ret;
+}
+
+static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
+{
+ u32 val;
+ struct device *dev = imx6_pcie->pci->dev;
+
+ if (regmap_read_poll_timeout(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR22, val,
+ val & IMX7D_GPR22_PCIE_PHY_PLL_LOCKED,
+ PHY_PLL_LOCK_WAIT_USLEEP_MAX,
+ PHY_PLL_LOCK_WAIT_TIMEOUT))
+ dev_err(dev, "PCIe PLL lock timeout\n");
+}
+
+static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
+{
+ struct dw_pcie *pci = imx6_pcie->pci;
+ struct device *dev = pci->dev;
+ int ret;
+
+ if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
+ ret = regulator_enable(imx6_pcie->vpcie);
+ if (ret) {
+ dev_err(dev, "failed to enable vpcie regulator: %d\n",
+ ret);
+ return;
+ }
+ }
+
+ ret = imx6_pcie_clk_enable(imx6_pcie);
+ if (ret) {
+ dev_err(dev, "unable to enable pcie clocks\n");
+ goto err_clks;
+ }
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio)) {
@@ -601,13 +624,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
return;
-err_ref_clk:
- clk_disable_unprepare(imx6_pcie->pcie);
-err_pcie:
- clk_disable_unprepare(imx6_pcie->pcie_bus);
-err_pcie_bus:
- clk_disable_unprepare(imx6_pcie->pcie_phy);
-err_pcie_phy:
+err_clks:
if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
ret = regulator_disable(imx6_pcie->vpcie);
if (ret)
--
2.25.1
When PCIe PHY link never came up and vpcie regulator is present, there
would be following dump when try to put the regulator.
Add a new host_exit() callback for i.MX PCIe driver to disable this
regulator and fix this dump when link never came up.
The driver should undo any enables it did itself, and not undo any
enables that anything else did which means it should never be basing
decisions on regulator_is_enabled().
To keep usage counter balance of the clocks, powers and so on. Do the
clock disable in the error handling after host_init too.
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/[email protected]
Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
Signed-off-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 36 +++++++++++++++++----------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 15908c184953..61b25f5cbf5c 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)
@@ -575,7 +565,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",
@@ -648,7 +638,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",
@@ -905,7 +895,6 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
- imx6_pcie_reset_phy(imx6_pcie);
return ret;
}
@@ -929,8 +918,29 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
return 0;
}
+static void imx6_pcie_host_exit(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);
+
+ imx6_pcie_reset_phy(imx6_pcie);
+ imx6_pcie_clk_disable(imx6_pcie);
+ switch (imx6_pcie->drvdata->variant) {
+ case IMX8MM:
+ if (phy_power_off(imx6_pcie->phy))
+ dev_err(dev, "unable to power off phy\n");
+ break;
+ default:
+ break;
+ }
+ if (imx6_pcie->vpcie)
+ regulator_disable(imx6_pcie->vpcie);
+}
+
static const struct dw_pcie_host_ops imx6_pcie_host_ops = {
.host_init = imx6_pcie_host_init,
+ .host_exit = imx6_pcie_host_exit,
};
static const struct dw_pcie_ops dw_pcie_ops = {
--
2.25.1
Just move the imx6_pcie_clk_disable() to an earlier place without function
changes, since it wouldn't be only used in imx6_pcie_suspend_noirq() later.
Signed-off-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 47 +++++++++++++--------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 13a4328bea19..15908c184953 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -533,6 +533,29 @@ static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie)
return ret;
}
+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);
+
+ switch (imx6_pcie->drvdata->variant) {
+ case IMX6SX:
+ clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
+ break;
+ case IMX7D:
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
+ IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
+ break;
+ case IMX8MQ:
+ clk_disable_unprepare(imx6_pcie->pcie_aux);
+ break;
+ default:
+ break;
+ }
+}
+
static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
{
u32 val;
@@ -971,30 +994,6 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
usleep_range(1000, 10000);
}
-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);
-
- switch (imx6_pcie->drvdata->variant) {
- case IMX6SX:
- clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
- break;
- case IMX7D:
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
- IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
- break;
- case IMX8MQ:
- case IMX8MM:
- clk_disable_unprepare(imx6_pcie->pcie_aux);
- break;
- default:
- break;
- }
-}
-
static int imx6_pcie_suspend_noirq(struct device *dev)
{
struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
--
2.25.1
When link is never came up in the link training after host_init.
The clocks and power supplies usage counter balance should be handled
properly on some DWC platforms (for example, i.MX PCIe).
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.
Signed-off-by: Richard Zhu <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f4755f3a03be..461863bde3c9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -405,7 +405,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
if (!dw_pcie_link_up(pci) && pci->ops && pci->ops->start_link) {
ret = pci->ops->start_link(pci);
if (ret)
- goto err_free_msi;
+ goto err_host_init;
}
/* Ignore errors, the link may come up later */
@@ -417,6 +417,9 @@ int dw_pcie_host_init(struct pcie_port *pp)
if (!ret)
return 0;
+err_host_init:
+ if (pp->ops->host_exit)
+ pp->ops->host_exit(pp);
err_free_msi:
if (pp->has_msi_ctrl)
dw_pcie_free_msi(pp);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 7d6e9b7576be..1153687ea9a6 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -174,6 +174,7 @@ enum dw_pcie_device_mode {
struct dw_pcie_host_ops {
int (*host_init)(struct pcie_port *pp);
+ void (*host_exit)(struct pcie_port *pp);
int (*msi_host_init)(struct pcie_port *pp);
};
--
2.25.1
On Wed, Jan 05, 2022 at 03:43:16PM +0800, Richard Zhu wrote:
> This series patches refine pci-imx6 driver and do the following changes.
> - Encapsulate the clock enable into one standalone function
> - Add the error propagation from host_init
> - Balance the usage of the regulator and clocks when link never came up
> - Add the compliance tests mode support
>
> Main changes from v4 to v5:
> - Since i.MX8MM PCIe support had been merged. Based on Lorenzo's git repos,
> rebase and resend the patch-set.
>
> 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.
>
> drivers/pci/controller/dwc/pci-imx6.c | 197 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------
> drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++-
> drivers/pci/controller/dwc/pcie-designware.h | 1 +
> 3 files changed, 128 insertions(+), 75 deletions(-)
>
> [PATCH v5 1/6] PCI: imx6: Encapsulate the clock enable into one
> [PATCH v5 2/6] PCI: imx6: Add the error propagation from host_init
> [PATCH v5 3/6] PCI: imx6: PCI: imx6: Move imx6_pcie_clk_disable()
> [PATCH v5 4/6] PCI: dwc: Add dw_pcie_host_ops.host_exit() callback
> [PATCH v5 5/6] PCI: imx6: Fix the regulator dump when link never came
> [PATCH v5 6/6] PCI: imx6: Add the compliance tests mode support
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
Remove repeated words from subject line.
On Wed, Jan 05, 2022 at 03:43:19PM +0800, Richard Zhu wrote:
> Just move the imx6_pcie_clk_disable() to an earlier place without function
> changes, since it wouldn't be only used in imx6_pcie_suspend_noirq() later.
Hi Richard,
On Wed, Jan 5, 2022 at 5:12 AM Richard Zhu <[email protected]> wrote:
>
> When PCIe PHY link never came up and vpcie regulator is present, there
> would be following dump when try to put the regulator.
> Add a new host_exit() callback for i.MX PCIe driver to disable this
> regulator and fix this dump when link never came up.
>
> The driver should undo any enables it did itself, and not undo any
> enables that anything else did which means it should never be basing
> decisions on regulator_is_enabled().
>
> To keep usage counter balance of the clocks, powers and so on. Do the
> clock disable in the error handling after host_init too.
>
> imx6q-pcie 33800000.pcie: Phy link never came up
> imx6q-pcie: probe of 33800000.pcie failed with error -110
Shouldn't we ignore the dw_pcie_wait_for_link() error?
At least, this was the intention of 886a9c134755 ("PCI: dwc: Move link
handling into
common code").
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
> Modules linked in:
My concern is that this issue is still present in 5.15, which is LTS.
You only address this problem in 5/6 and I am not sure if the previous
patches could be applied to stable as they are cleanups.
How can we fix this for 5.15 stable?
Could you make a minimal fix as the first patch of the series and
cleanup patches later?
> -----Original Message-----
> From: Fabio Estevam <[email protected]>
> Sent: Thursday, January 6, 2022 5:52 AM
> To: Hongxing Zhu <[email protected]>
> Cc: Lucas Stach <[email protected]>; Bjorn Helgaas
> <[email protected]>; Mark Brown <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v5 5/6] PCI: imx6: Fix the regulator dump when link
> never came up
>
> Hi Richard,
>
> On Wed, Jan 5, 2022 at 5:12 AM Richard Zhu <[email protected]>
> wrote:
> >
> > When PCIe PHY link never came up and vpcie regulator is present, there
> > would be following dump when try to put the regulator.
> > Add a new host_exit() callback for i.MX PCIe driver to disable this
> > regulator and fix this dump when link never came up.
> >
> > The driver should undo any enables it did itself, and not undo any
> > enables that anything else did which means it should never be basing
> > decisions on regulator_is_enabled().
> >
> > To keep usage counter balance of the clocks, powers and so on. Do the
> > clock disable in the error handling after host_init too.
> >
> > imx6q-pcie 33800000.pcie: Phy link never came up
> > imx6q-pcie: probe of 33800000.pcie failed with error -110
>
> Shouldn't we ignore the dw_pcie_wait_for_link() error?
>
> At least, this was the intention of 886a9c134755 ("PCI: dwc: Move link
> handling into common code").
>
> > ------------[ cut here ]------------
> > WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256
> _regulator_put.part.0+0x14c/0x158
> > Modules linked in:
>
> My concern is that this issue is still present in 5.15, which is LTS.
>
> You only address this problem in 5/6 and I am not sure if the previous
> patches could be applied to stable as they are cleanups.
>
> How can we fix this for 5.15 stable?
>
> Could you make a minimal fix as the first patch of the series and cleanup
> patches later?
[Richard Zhu] Remove the stable kernel mail list firstly.
Yes, you're right.
It's difficult to merge this series back to 5.15 stable kernel, since the
i.MX8MM PCIe support patch-set had been merged, and there are codes
conflictions.
How about continue proceed your previous fix firstly?
https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
Then, I clean up the fix patch, and re-send my series later.
Because that I still want to save power consumption as much as possible
when link is down.
BTW, please remove the timestamp in your patch if you agree this method,
and resend your patch.
Best Regards
Richard Zhu
> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: Thursday, January 6, 2022 3:04 AM
> To: Hongxing Zhu <[email protected]>
> Cc: [email protected]; [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 v5 3/6] PCI: imx6: PCI: imx6: Move
> imx6_pcie_clk_disable() earlier
>
> Remove repeated words from subject line.
[Richard Zhu] Remove the stable kernel mail list.
Sorry to add one edit mistake here.
Would remove them later, thanks.
Best Regards
Richard Zhu
>
> On Wed, Jan 05, 2022 at 03:43:19PM +0800, Richard Zhu wrote:
> > Just move the imx6_pcie_clk_disable() to an earlier place without
> > function changes, since it wouldn't be only used in
> imx6_pcie_suspend_noirq() later.
Hi Richard,
On Thu, Jan 6, 2022 at 12:49 AM Hongxing Zhu <[email protected]> wrote:
> How about continue proceed your previous fix firstly?
> https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
> Then, I clean up the fix patch, and re-send my series later.
> Because that I still want to save power consumption as much as possible
> when link is down.
> BTW, please remove the timestamp in your patch if you agree this method,
> and resend your patch.
Sure, this sounds good. I have just sent a v2.
Thanks